Repository navigation
fix: give the delivery detail screen the route id as its key - #28
Merged
Merged
Conversation
devinder-dev
approved these changes
Nov 1, 2026
devinder-dev
left a comment
Owner
There was a problem hiding this comment.
Approved. The key is the right tool here: React's own way to reset a screen when its identity changes, and it restarts the refresh timer for free. The wrapper keeps useParams in one place. Agreed on no test: no UI path reaches it yet, and the comment says why. Thanks for saying out loud what happened with feat/dashboard-screens; I checked, it's gone.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The "nice to have" from your review of #9: reset the detail screen when the route's id changes. One file, 16 lines, on top of
mainnow that #9 is in.The bug
useState's initializer runs once per mount. Going from/deliveries/Ato/deliveries/Bis the same route pattern, so React Router keeps the same component instance — the initializer never runs again and the screen keeps showing A.The fix
A thin wrapper that reads the id and hands it down as the React key, so the id is the screen's identity:
Picked
keyover a reset effect because it is one concept rather than two, and it fixes the 2 s poll for free: a different key is a different component, so the old effect is cleaned up and the new one starts from scratch. AuseEffectthat compared ids would have had to reset the state and leave the timer to the existing[delivery?.status, id]dependency, which works but is two things to keep in step instead of one.What I did not do, and why
No test for it. It is latent: nothing links one detail screen to another (
DeliveryDetail.tsxhas only the two "back to the log" links, and there are nonavigate()calls outside Connect and App), and typing a URL reloads the page, which remounts anyway. So there is no user path to it today.A test would have to drive
history.pushStatedirectly, which is testing a route the UI cannot take — the same objection as writing the key intosessionStorageinstead of going through the Connect form. I would rather have the reason recorded than a test that proves the mechanism exists rather than that the app behaves.So, honestly: the 35 passing tests show this broke nothing, not that it works. What it rests on is React's documented
keysemantics, and the reason it is worth having anyway is that it makes "add a next-delivery link" a safe change instead of one that silently brings the bug with it. The comment in the file says that.Checked
npm run build,npm run lint,npm run test:e2e(35 passed) on this branch, againstmain.One thing worth knowing about this branch
I nearly lost this commit.
feat/dashboard-screenswas deleted when you squash-merged #9, and I pushed the stale local branch before noticing — which recreated it on the remote, carrying the pre-squash commits that are already onmainunder one squash. I have deleted it again and moved the fix onto this branch, cut from currentmain.Nothing was lost and
mainwas never touched, but it is worth saying out loud rather than quietly cleaning up: if you saw afeat/dashboard-screensreappear for a few minutes, that was me, and it was junk.