Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

🐛 Fix draft trips not showing up for (i) unpushed transitions (ii) fresh user #1189

Merged
merged 4 commits into from
Dec 10, 2024

Conversation

JGreenlee
Copy link
Collaborator

@JGreenlee JGreenlee commented Dec 9, 2024

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.

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.
@JGreenlee JGreenlee force-pushed the timeline-fix-dec2024 branch from 061f9bb to 0d60712 Compare December 9, 2024 18:26
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.
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
@JGreenlee
Copy link
Collaborator Author

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_trips 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.


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

@JGreenlee JGreenlee marked this pull request as ready for review December 9, 2024 20:02
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 e-mission#1189 sooner.
@JGreenlee
Copy link
Collaborator Author

Tests pass locally. Merging

@JGreenlee JGreenlee merged commit 97726aa into e-mission:master Dec 10, 2024
5 of 6 checks passed
@shankari
Copy link
Contributor

@JGreenlee is this is the first PR that you have merged using your new powers?

@JGreenlee
Copy link
Collaborator Author

@JGreenlee is this is the first PR that you have merged using your new powers?

There have been one or two! 🦸

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Tasks completed
Development

Successfully merging this pull request may close these issues.

2 participants