From 0d60712b85a9bc98b5eec04530e9ad9fd81da987 Mon Sep 17 00:00:00 2001 From: Jack Greenlee Date: Mon, 9 Dec 2024 13:26:28 -0500 Subject: [PATCH 1/4] sort transitions by ts before mapping them into unprocessed trips MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit fix a regression causing "unpushed draft trips not showing up on iOS" (issue tracked on internal repo) Details from the issue explaining the regression: ``` I checked the timestamps in my log to see which days to pull, and I get (logs) Mapping out the timestamps there, I get >>> import arrow >>> arrow.get(1729999919.717365) >>> arrow.get(1729997963.941101) >>> arrow.get(1729996352.1965032) >>> arrow.get(1729962952.943046) >>> arrow.get(1729962183.1975121) >>> arrow.get(1729959148.916346) >>> arrow.get(1729957924.3998609) >>> arrow.get(1729830532.382776) @ jgreenle it looks like the issue is just that we are reading entries in reverse order... It seems like we must have changed the return order or stopped reversing the transitions or something... ``` ``` I used the Simulator Location feature to take a “City Run”. (image) I simulated a 10-minute trip 3:18 -> 3:28 (image) I wanted to check the label tab to see whether this shows up. I could not do this offline, as the Label tab does not load if it cannot reach the server. But now that the fake trip is over, I should be able to “come online” without the sensed data being automatically pushed. After doing so and loading the diary, the trip did not show up (as expected) (image) The logs do look similar to Shankari’s, as timestamps are backwards. Looks promising! Now, I just need to climb up the stack trace and find where entries are being read backwards ``` ``` getUnifiedDataForInterval is called for statemachine/transition entries in the time range 1732942800 -> 1733547599. That looks fine (image) It should get unpushed transitions via the “localGetMethod”, which corresponds to getMessagesForInterval of the UserCache plugin (image) Inspecting the result of this promise, we get this: (image) Sure enough, the entries we get from the UserCache plugin are in reverse order. I can probably fix this by just reversing the array. But I’d really like to understand what caused the regression here. I don’t think the UserCache plugin has been changed. I also don’t recall any changes to unifiedDataLoader or the unprocessed trip functions in timelineHelper ``` ``` It may be overkill, but I jumped all the way back to the end of last year (commit 07f4534) to verify that this worked then. It did; today's simulated trip shows up: (image) This at least proves that a regression happened in e-mission-phone sometime in 2024 We used to sort the transitions by ts, as I can see in the code at https://github.com/e-mission/e-mission-phone/blob/07f4534524365a0310fb410d2b965dd41e4aa07b/www/js/diary/services.js#L347 What's interesting is that sortedTransitionList is never used as indicated by the linter (line 347) (image) transitionList is used just below that, though, and Array.sort is mutable. So this statement modifies transitionList as well as assigning the result to sortedTransitionList I'm willing to bet that whoever modified this – probably me – saw that sortedTransitionList is not used and just took out the whole line. (it seems to be a common pitfall with mutable functions that return their value, which is why emcascript has now introduced immutable versions of these functions e.g. toSorted, toSpliced, toReversed) ``` ``` It actually appears that I picked just about the right spot in the code history to look. We stopped sorting the retrieved transitions when we rewrote the readUnprocessedTrips function in commit 7a4b6fb. This was merged into master on Feb 1. A bit hard to believe this went unnoticed for 10 months, but it definitely seems to be the cause for this specific bug. ``` To fix this we cannot safely use `Array.toSorted` because it has only been supported on iOS Safari since 16.0. So, I am just using `transitionList.sort()`. To make it clearer that this mutates `transitionList`, I am not assigning the result of the operation to another variable. I did the same for the other place where we similarly call `.sort`, which is for `locationList`. I can now simulate an offline trip in the Simulator and have it show up as a draft trip. --- www/js/diary/timelineHelper.ts | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/www/js/diary/timelineHelper.ts b/www/js/diary/timelineHelper.ts index e3e9d379c..f70353668 100644 --- a/www/js/diary/timelineHelper.ts +++ b/www/js/diary/timelineHelper.ts @@ -424,7 +424,7 @@ function points2UnprocessedTrip( }; } -const tsEntrySort = (e1: BEMData, e2: BEMData) => +const tsEntrySort = (e1: BEMData<{ ts: number }>, e2: BEMData<{ ts: number }>) => e1.data.ts - e2.data.ts; // compare timestamps /** @@ -449,10 +449,10 @@ function tripTransitions2UnprocessedTrip( if (locationList.length == 0) { return undefined; } - const sortedLocationList = locationList.sort(tsEntrySort); + locationList.sort(tsEntrySort); const retainInRange = (loc) => tripStartTransition.data.ts <= loc.data.ts && loc.data.ts <= tripEndTransition.data.ts; - const filteredLocationList = sortedLocationList.filter(retainInRange); + const filteredLocationList = locationList.filter(retainInRange); // Fix for https://github.com/e-mission/e-mission-docs/issues/417 if (filteredLocationList.length == 0) { @@ -600,6 +600,7 @@ export function readUnprocessedTrips( return []; } else { logDebug(`Found ${transitionList.length} transitions. yay!`); + transitionList.sort(tsEntrySort); const tripsList = transitions2TripTransitions(transitionList); logDebug(`Mapped into ${tripsList.length} trips. yay!`); const tripFillPromises = tripsList.map((t) => From 970e50c0049a2e24cf6c129f907624f3e462f742 Mon Sep 17 00:00:00 2001 From: Jack Greenlee Date: Mon, 9 Dec 2024 14:54:45 -0500 Subject: [PATCH 2/4] allow draft trips to show up if pipeline has never been run If a user has just joined and none of their data has ever been through the pipeline, they will have a pipelineRange of `{start_ts: null, end_ts: null}`. In this case, there is no point in querying for `analysis/composite_trip`s because no analysis entries have been created yet However, the user might have travel that hasn't been through the pipeline yet. So instead of immediately returning early from fetchTripsInRange, let's allow readUnprocessedPromise to execute, using 0 as the lower ts bound since pipelineRange end_ts will be null. I tested this by creating a brand new user, simulating a trip, and reloading the diary. The draft trip shows up without ever running the pipeline. --- www/js/TimelineContext.ts | 25 +++++++++++++------------ 1 file changed, 13 insertions(+), 12 deletions(-) diff --git a/www/js/TimelineContext.ts b/www/js/TimelineContext.ts index fc463bbee..7a17552be 100644 --- a/www/js/TimelineContext.ts +++ b/www/js/TimelineContext.ts @@ -221,19 +221,22 @@ export const useTimelineContext = (): ContextProps => { } async function fetchTripsInRange(dateRange: [string, string]) { - if (!pipelineRange?.start_ts || !pipelineRange?.end_ts) { - logDebug('No pipelineRange yet, returning empty lists'); - return [[], []]; - } logDebug('Timeline: fetchTripsInRange from ' + dateRange[0] + ' to ' + dateRange[1]); - const [startTs, endTs] = isoDateRangeToTsRange(dateRange); - const maxStartTs = Math.max(startTs, pipelineRange.start_ts); // ensure that we don't read before the pipeline start - const minEndTs = Math.min(endTs, pipelineRange.end_ts); // ensure that we don't read after the pipeline end - const readCompositePromise = readAllCompositeTrips(maxStartTs, minEndTs); + let readCompositePromise; // comment + if (!pipelineRange?.start_ts || !pipelineRange?.end_ts) { + readCompositePromise = Promise.resolve([]); + } else { + const maxStartTs = Math.max(startTs, pipelineRange.start_ts); // ensure that we don't read before the pipeline start + const minEndTs = Math.min(endTs, pipelineRange.end_ts); // ensure that we don't read after the pipeline end + readCompositePromise = readAllCompositeTrips(maxStartTs, minEndTs); + } + let readUnprocessedPromise; - if (endTs >= pipelineRange.end_ts) { + if (pipelineRange?.end_ts && pipelineRange.end_ts > endTs) { + readUnprocessedPromise = Promise.resolve([]); + } else { let lastProcessedTrip: CompositeTrip | undefined; if (timelineMap) { lastProcessedTrip = [...timelineMap?.values()] @@ -241,13 +244,11 @@ export const useTimelineContext = (): ContextProps => { .find((trip) => trip.origin_key.includes('trip')) as CompositeTrip; } readUnprocessedPromise = readUnprocessedTrips( - Math.max(pipelineRange.end_ts, startTs), + Math.max(pipelineRange?.end_ts || 0, startTs), endTs, appConfig, lastProcessedTrip, ); - } else { - readUnprocessedPromise = Promise.resolve([]); } const results = await Promise.all([readCompositePromise, readUnprocessedPromise]); From 34fbe4b43cee99c172a2f9ccff6b5c36fa4a26b3 Mon Sep 17 00:00:00 2001 From: Jack Greenlee Date: Mon, 9 Dec 2024 14:59:59 -0500 Subject: [PATCH 3/4] allow paste opcode to continue if clipboard Cordova plugin is missing Until we generate a new release of em-devapp, it does not have the cordova clipboard plugin. To make local development easier I'm adding this small change that will allow auto-paste to be no-op if the clipboard plugin is not present; we then fall back to manual text entry of the opcode --- www/js/onboarding/WelcomePage.tsx | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/www/js/onboarding/WelcomePage.tsx b/www/js/onboarding/WelcomePage.tsx index ff8214578..36b27ded8 100644 --- a/www/js/onboarding/WelcomePage.tsx +++ b/www/js/onboarding/WelcomePage.tsx @@ -62,7 +62,9 @@ const WelcomePage = () => { } function pasteCode() { - window['cordova'].plugins.clipboard.paste((clipboardContent: string) => { + // if clipboard plugin not available, the callback will be a no-op + const pasteFn = window['cordova'].plugins.clipboard?.paste || ((cb) => cb('')); + pasteFn((clipboardContent: string) => { addStatReading('paste_token'); try { if (!clipboardContent?.startsWith('nrelop_') && !clipboardContent?.includes('://')) { From 478fc1878b4a419100b31d10cbc48786c67146c1 Mon Sep 17 00:00:00 2001 From: Jack Greenlee Date: Tue, 10 Dec 2024 15:28:50 -0500 Subject: [PATCH 4/4] for timelineHelper test, put mockTransitions in reverse order When the usercache is queried, it returns entries in reverse chronological order; the mock should reflect this so the test ensures they are sorted correctly. It would have helped us catch https://github.com/e-mission/e-mission-phone/pull/1189 sooner. --- www/__mocks__/timelineHelperMocks.ts | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/www/__mocks__/timelineHelperMocks.ts b/www/__mocks__/timelineHelperMocks.ts index 66011c4dd..ecee21ec5 100644 --- a/www/__mocks__/timelineHelperMocks.ts +++ b/www/__mocks__/timelineHelperMocks.ts @@ -165,19 +165,19 @@ export const mockCompDataTwo = { export const mockTransitions: Array> = [ { data: { - // mock of a startTransition + // mock of an endTransition currstate: '', - transition: 'T_EXITED_GEOFENCE', - ts: 1, + transition: 'T_TRIP_ENDED', + ts: 9999, }, metadata: mockMetaData, }, { data: { - // mock of an endTransition + // mock of a startTransition currstate: '', - transition: 'T_TRIP_ENDED', - ts: 9999, + transition: 'T_EXITED_GEOFENCE', + ts: 1, }, metadata: mockMetaData, },