Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
sort transitions by ts before mapping them into unprocessed trips
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 [2024-10-27T03:31:59.717365+00:00]> >>> arrow.get(1729997963.941101) <Arrow [2024-10-27T02:59:23.941101+00:00]> >>> arrow.get(1729996352.1965032) <Arrow [2024-10-27T02:32:32.196503+00:00]> >>> arrow.get(1729962952.943046) <Arrow [2024-10-26T17:15:52.943046+00:00]> >>> arrow.get(1729962183.1975121) <Arrow [2024-10-26T17:03:03.197512+00:00]> >>> arrow.get(1729959148.916346) <Arrow [2024-10-26T16:12:28.916346+00:00]> >>> arrow.get(1729957924.3998609) <Arrow [2024-10-26T15:52:04.399861+00:00]> >>> arrow.get(1729830532.382776) <Arrow [2024-10-25T04:28:52.382776+00:00]> @ 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.
- Loading branch information