Fix: prevent duplicate waypoint creation when reopening sidebar#366
Fix: prevent duplicate waypoint creation when reopening sidebar#366Shyam-123pandey wants to merge 1 commit intovalhalla:masterfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes a bug where reopening the directions sidebar would trigger duplicate waypoint creation when loading a permalink with 3 or more waypoints. Previously, the reverseGeocode function unconditionally added a new empty waypoint for any permalink index > 1. When the sidebar was reopened while the Zustand store already retained the previously loaded waypoints, these waypoints were added again, producing duplicates. The fix adds a guard that checks whether the store already has a slot for the given index before calling addEmptyWaypointToEnd.
Changes:
- Added a guard condition in
useReverseGeocodeDirectionsso thataddEmptyWaypointToEndis only called when the store does not yet have a waypoint at the given index.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| if ( | ||
| options?.isPermalink && | ||
| index > 1 && | ||
| useDirectionsStore.getState().waypoints.length <= index | ||
| ) { | ||
| addEmptyWaypointToEnd(); |
There was a problem hiding this comment.
There is no test coverage for the new guard condition added to reverseGeocode. Specifically, the key scenario that motivated this fix — loading a permalink with 3 or more waypoints when the sidebar is reopened (i.e., when waypoints already exist in the store) — is not exercised by any test. The existing directions.spec.tsx URL-parsing tests mock useReverseGeocodeDirections entirely and therefore do not validate the internal logic of the guard condition. A test for reverseGeocode (in the hook itself or via an integration test) should:
- Verify that
addEmptyWaypointToEndis called when loading a 3+ waypoint permalink with a fresh (default 2-waypoint) store. - Verify that
addEmptyWaypointToEndis NOT called when the store already has a waypoint at the given index (the reopen scenario).
There was a problem hiding this comment.
You're right that the current tests in directions.spec.tsx mock useReverseGeocodeDirections, so they don't exercise the internal logic of reverseGeocode. I’ll add a dedicated test to cover the guard condition if You want???
Specifically, I plan to add tests that verify:
addEmptyWaypointToEndis called when loading a permalink with 3+ waypoints on a fresh store (default 2-waypoint state).addEmptyWaypointToEndis not called when the store already has a waypoint at the given index (e.g., when reopening the sidebar).
Respond if you want i can add this?
|
Please review this PR, Have been successfully fixed this issue. |
|
Hello Sir Please don't be angry , from today onwards i won't do such type of mistake like reminding again again. Can i reopen this PR again? |
🛠️ Fixes Issue
Closes #365
👨💻 Changes proposed
addEmptyWaypointToEnd().📍 Where the change was made
src/hooks/use-directions-queries.tsreverseGeocode()📄 Note to reviewers
Tested by adding multiple waypoints and reopening the sidebar; no duplicate waypoints appear.
📷 Screenshots
Explore Video of Fix:
add.waypont.fix.mp4