feat(web): restore last visited route across reloads#12
Conversation
Persist resolved navigations to localStorage under citadel:lastRoute and replace the URL before the router boots so reloads and full close+reopen land back on the user's last view. Deep links always win (only bare / triggers restore). A stale route hits the new NotFound view, which clears the saved key so the next boot is clean. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Address self-review findings on #12: - Clear the persisted route from inside a useEffect rather than during render. Render functions must be pure; StrictMode would otherwise invoke the side effect twice. - Reject protocol-relative ("//evil/x") and backslash-prefixed ("/\\…") paths in both loadLastRoute and saveLastRoute, so a tampered localStorage value can never reach history.replaceState with an origin-spoofing URL. - Drop the unused _data field on the test helper and add coverage for the new rejection cases. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
ovdmar
left a comment
There was a problem hiding this comment.
Self-review pass on the current diff. Three findings below — each tied to a specific line.
| // only routes the user actually reached enter the store — aborted loads and | ||
| // in-flight navigations are skipped, so we never restore into a broken state. | ||
| router.subscribe("onResolved", (event) => { | ||
| saveLastRoute(event.toLocation.href); |
There was a problem hiding this comment.
Finding 1 — saves the path of a 404 before NotFoundView clears it.
When the persisted route is stale (route removed, etc.), the sequence is: onResolved fires → subscriber writes the bad path back to localStorage → React commits NotFoundView → useEffect calls clearLastRoute. The clear happens last so the steady state is correct, but there is a window during which localStorage holds an invalid value. If the tab is closed in that window, the next boot replays the same bad route.
Cleaner: short-circuit the subscriber when router.state.statusCode is non-2xx, so we never write a path that didn't resolve to a real match.
There was a problem hiding this comment.
Resolved in c8fb32e. Subscriber now reads router.state.statusCode and short-circuits on >= 400 — bad paths never make it into localStorage, even transiently.
| if (saved && saved !== "/") { | ||
| window.history.replaceState(null, "", saved); | ||
| } | ||
| } |
There was a problem hiding this comment.
Finding 2 — boot-time restore decision is not tested.
The unit tests cover the storage layer (loadLastRoute/saveLastRoute/isBareRootLanding) but not the actual decision being made here: "bare root + saved value ≠ '/' → replaceState". That logic is what the user-facing requirement hinges on. Extract it into a small pure function (bootstrapLastRoute(location, history, storage)) and add tests covering: bare root with saved route, bare root with no saved route, bare root where saved equals '/', and deep-link landing.
There was a problem hiding this comment.
Resolved in c8fb32e. Extracted bootstrapLastRoute(location, history, storage) and added five tests: bare-root restore, no-saved-value, saved-equals-/, deep-link non-root pathname, deep-link on / with query string. main.tsx now just calls the function.
| ); | ||
| } | ||
|
|
||
| function NotFoundView() { |
There was a problem hiding this comment.
Finding 3 — NotFoundView is referenced on line 90 before its declaration on line 114.
Works via function hoisting, but readers tracing the createRouter config have to scan past the bottom of the file. Move the declaration up so the reading order matches the dependency order.
There was a problem hiding this comment.
Resolved in c8fb32e. Shell and NotFoundView moved above the route table / createRouter call so reading order matches dependency order.
Resolves the three findings posted as PR review comments: > Finding 1 — saves the path of a 404 before NotFoundView clears it. Short-circuit the onResolved subscriber when router.state.statusCode is >= 400 so stale or unmatched routes never get written back to storage, even transiently. > Finding 2 — boot-time restore decision is not tested. Extract the decision logic into bootstrapLastRoute(location, history, storage) — a pure function. Add five tests covering bare-root restore, no-saved-value, saved-equals-/, deep-link non-root pathname, and deep-link with a query string. > Finding 3 — NotFoundView is referenced before its declaration. Move Shell and NotFoundView above the createRouter call so reading order matches dependency order. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
ovdmar
left a comment
There was a problem hiding this comment.
Re-review pass after c8fb32e. All three previous findings have been addressed (replies posted on each comment). No new findings on the current diff:
- Subscriber timing against
router.state.statusCodeverified against TanStack source — statusCode is updated by loadMatches beforeonResolvedis emitted. useEffectdeps are stable (module-level imports).- Router lives for app lifetime so the discarded unsubscribe handle is intentional.
- typecheck + lint + 103 tests green locally.
Ready for human review/merge.
Summary
localStorage["citadel:lastRoute"]viarouter.subscribe("onResolved", …). Saving on every navigation (not justbeforeunload) makes this robust under reloads, full close+reopen, and PWA mode where unload events are unreliable.createRouter()boots, if the browser landed on the bare app root (pathname === "/", no query, no hash), callhistory.replaceStatewith the saved route so the router's initial location is already the restored URL — no flash of the cockpit, no second navigation that could feed back into the subscriber.defaultNotFoundComponentclears the saved key, so a stale persisted route (removed page, etc.) can't loop the user into 404 on every boot.Why
User feedback (verbatim): "reloading or changing page and coming back should always lead to your last visited page — even if you completely close it and reopen it should still have you on the latest page." Today the app always lands on the default cockpit view regardless of where the user was.
Excluded routes
None deliberately excluded —
EXCLUDED_PREFIXESinapps/web/src/lib/last-route.tsis intentionally empty so/onboardingis restored too (a user who closes mid-setup resumes in the wizard). Hook is in place for future per-route exclusions.Test plan
pnpm typecheckpnpm lintpnpm test— full suite green, including 5 new unit tests forlast-route.ts(round-trip, clear, non-absolute href rejection, bare-root landing detection)/operations, reload → still on/operations/operations, close tab, reopen/→ lands on/operations/settingsdirectly (deep link) → stays on/settings, doesn't get overridden by any previously saved value/no-such-route) and reload → renders NotFound, key is cleared, next reload lands on/🤖 Generated with Claude Code