Skip to content

fix: give the delivery detail screen the route id as its key - #28

Merged
AbdikhaniAbdirashid merged 1 commit into
mainfrom
fix/detail-screen-key
Nov 2, 2026
Merged

AbdikhaniAbdirashid merged 1 commit into
mainfrom
fix/detail-screen-key

Conversation

@AbdikhaniAbdirashid

Copy link
Copy Markdown
Collaborator

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 main now that #9 is in.

The bug

const [delivery, setDelivery] = useState(() => getDelivery(id))

useState's initializer runs once per mount. Going from /deliveries/A to /deliveries/B is 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:

export default function DeliveryDetail() {
  const { id } = useParams()
  return <Screen key={id} id={id} />
}

Picked key over 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. A useEffect that 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.tsx has only the two "back to the log" links, and there are no navigate() 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.pushState directly, which is testing a route the UI cannot take — the same objection as writing the key into sessionStorage instead 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 key semantics, 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, against main.

One thing worth knowing about this branch

I nearly lost this commit. feat/dashboard-screens was 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 on main under one squash. I have deleted it again and moved the fix onto this branch, cut from current main.

Nothing was lost and main was never touched, but it is worth saying out loud rather than quietly cleaning up: if you saw a feat/dashboard-screens reappear for a few minutes, that was me, and it was junk.

@devinder-dev devinder-dev left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@AbdikhaniAbdirashid
AbdikhaniAbdirashid merged commit 1e47987 into main Nov 2, 2026
4 checks passed
@AbdikhaniAbdirashid
AbdikhaniAbdirashid deleted the fix/detail-screen-key branch November 2, 2026 15:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants