Fix Android waypoint jumping when dragging starts - #80068
Conversation
Signed-off-by: Tsaqif <tsaiinkwa@yahoo.com>
|
@situchan The changes are similar in |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 21d2e770b4
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Codecov Report❌ Looks like you've decreased code coverage for some files. Please write tests to increase, or at least maintain, the existing level of code coverage. See our documentation here for how to interpret this table.
|
Signed-off-by: Tsaqif <tsaiinkwa@yahoo.com>
tsa321
changed the title
trjExpensify
left a comment
trjExpensify
left a comment
There was a problem hiding this comment.
Does look smoother! 👍
| const key = `waypoint${i}`; | ||
| const waypoint = waypoints[key]; | ||
|
|
||
| const keyForList = waypoint?.keyForList ?? Math.random().toString(36).substring(2, 8); |
There was a problem hiding this comment.
I’m using a random string here because when a user removes the first or second waypoint address after inserting one, the waypoint data becomes an empty object ({}). Using waypoint${index} or start_waypoint / stop_waypoint (based on the index) for keyForList would cause the same issue on Android.
There was a problem hiding this comment.
the waypoint data becomes an empty object (
{})
Why can't we avoid this in the first place?
|
Please merge main |
|
@situchan I have merged main |
yes |
Signed-off-by: Tsaqif <tsaiinkwa@yahoo.com>
|
@situchan I’ve moved the logic into a new hook. |
| const key = `waypoint${i}`; | ||
| const waypoint = waypoints[key]; | ||
|
|
||
| const keyForList = waypoint?.keyForList ?? Math.random().toString(36).substring(2, 8); |
There was a problem hiding this comment.
I am getting this lint locally
| * Hook that generates waypoint items based on its `keyForList` for the draggable list, | ||
| * and provides helper functions to retrieve a waypoint or its key. | ||
| */ | ||
| function useWaypointItems(waypoints: WaypointCollection): UseWaypointItemsParams { |
There was a problem hiding this comment.
Please add unit tests for this new hook
Signed-off-by: Tsaqif <tsaiinkwa@yahoo.com>
Signed-off-by: Tsaqif <tsaiinkwa@yahoo.com>
Signed-off-by: Tsaqif <tsaiinkwa@yahoo.com>
|
@situchan I’ve updated the code and added unit test. |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppandroid.movAndroid: mWeb ChromeiOS: HybridAppios.moviOS: mWeb Safarimsafari.movMacOS: Chrome / Safariweb.mov |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 99845c8cd5
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| const keyForList = waypoint?.keyForList ?? ''; | ||
|
|
||
| waypointItems.push(keyForList); | ||
| waypointsByKeyForList[keyForList] = {key, waypoint}; | ||
| } |
There was a problem hiding this comment.
In useWaypointItems, keyForList is coerced to '' when missing and then used both to build waypointItems and as the key in waypointsByKeyForList. If the user adds multiple stops before filling addresses (so more than one waypoint has no keyForList yet), all of those entries share the same '' key: the later waypoint overwrites the earlier one, getWaypoint('') returns only the last empty waypoint, and keyExtractor produces duplicate keys. That can cause the draggable list to lose or misorder empty waypoints when dragging or re-rendering. Consider incorporating the waypoint index (e.g., waypoint${i}) into the list key when keyForList is missing to keep items unique.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
If the user adds multiple stops before filling addresses
This is not possible in the app.
Consider incorporating the waypoint index (e.g., waypoint${i})
This will cause the same android bug.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 99845c8cd5
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| const keyForList = waypoint?.keyForList ?? ''; | ||
|
|
||
| waypointItems.push(keyForList); | ||
| waypointsByKeyForList[keyForList] = {key, waypoint}; | ||
| } |
There was a problem hiding this comment.
When a waypoint has no keyForList (e.g., a user clears start/stop or adds a blank waypoint), keyForList becomes '' for each empty item. Because waypointsByKeyForList is keyed by keyForList, the later empty waypoint overwrites earlier ones, so both list entries resolve to the same waypoint/key and extractKey returns identical keys. This can make drag/reorder or edit actions target the wrong waypoint and effectively drop one. Consider using a unique identifier (such as the waypoint index) in the list item identity or in the map key to avoid collisions for empty waypoints.
Useful? React with 👍 / 👎.
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚀 Deployed to staging by https://github.com/mjasikowski in version: 9.3.11-16 🚀
|
|
🚀 Deployed to production by https://github.com/Julesssss in version: 9.3.12-1 🚀
|
5 participants
Explanation of Change
Fixed Issues
$ #78809
PROPOSAL: #78809 (comment)
Tests
Test step:
Track distance.Next→ choose a workspace → create the track expense.Distance.Offline tests
QA Steps
Test step:
Track distance.Next→ choose a workspace → create the track expense.Distance.PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectioncanBeMissingparam foruseOnyxtoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
android_native_d.mp4
Android: mWeb Chrome
android_mweb_d.mp4
iOS: Native
ios_native_d.mp4
iOS: mWeb Safari
ios_msafari_d.mp4
MacOS: Chrome / Safari
macos_web_d.mp4