fix(router): fix loader rerun on params and search change#69
Conversation
📝 WalkthroughWalkthroughRouter navigation now revalidates loaders when pathname or search changes within the same route pattern, while ignoring hash-only changes. SPA and hydration tests cover these flows, package versions move to 0.8.6, and persistence gains synchronous hydration coverage. ChangesRouter navigation revalidation
Store persistence validation
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Browser
participant NavHandler
participant RouterLoader
participant RouteView
Browser->>NavHandler: Navigate to pathname or search change
NavHandler->>RouterLoader: Revalidate route loader
RouterLoader-->>NavHandler: Return loader result
NavHandler->>RouteView: Mount updated route view
RouteView-->>Browser: Render updated content
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/router/src/index.ts (1)
2383-2404: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winPrevent race conditions when applying navigation state.
When a navigation is superseded by a newer one, the fetch promise could still resolve successfully (e.g., if the abort happens exactly as the fetch completes). To prevent leaking the unmount function or incorrectly resetting
currentMountedPathto the older URL, verify thatthisNav === navVersionbefore applying the mount results. This bringsNavHandlerin line with the race-condition guards already present in_revalidate(line 2478) and the initial loader render (line 2438).🐛 Proposed fix
- try { - unmountView = await mountRouteWithHydration( - current, - viewHost, - pathWithSearch, - signal, - registry, - reverseRegistry, - ); - } catch (e: any) { + let um: (() => void) | undefined; + try { + um = await mountRouteWithHydration( + current, + viewHost, + pathWithSearch, + signal, + registry, + reverseRegistry, + ); + } catch (e: any) { if (e?.name === "AbortError") return; // Contain navigation failures — an unhandled rejection here // would leave a blank view with no diagnostics. console.error("[ilha-router] navigation failed:", e); - viewHost.innerHTML = `<div data-router-view data-router-error="500"></div>`; + if (thisNav === navVersion) { + viewHost.innerHTML = `<div data-router-view data-router-error="500"></div>`; + } return; } finally { settle(); } - currentMountedIsland = current; - currentMountedPath = pathWithSearch; + if (thisNav === navVersion) { + unmountView = um; + currentMountedIsland = current; + currentMountedPath = pathWithSearch; + } else { + um?.(); + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/router/src/index.ts` around lines 2383 - 2404, Guard the successful mount-result application in the navigation flow with a `thisNav === navVersion` check before assigning `unmountView`, `currentMountedIsland`, or `currentMountedPath`. If the navigation is stale, discard the returned unmount function and do not update mounted state, matching the existing guards in `_revalidate` and the initial loader render.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/store/src/index.test.ts`:
- Around line 1341-1345: Wrap the assertion following persist(s, "p1b") in a
try/finally block so stop() always executes, including when expect(s.getState())
fails. Keep the existing state expectation unchanged and place the cleanup call
in the finally block.
---
Outside diff comments:
In `@packages/router/src/index.ts`:
- Around line 2383-2404: Guard the successful mount-result application in the
navigation flow with a `thisNav === navVersion` check before assigning
`unmountView`, `currentMountedIsland`, or `currentMountedPath`. If the
navigation is stale, discard the returned unmount function and do not update
mounted state, matching the existing guards in `_revalidate` and the initial
loader render.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 79acb320-dd5a-4833-8ad4-3d4e49390817
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (9)
apps/website/package.jsonpackages/router/package.jsonpackages/router/src/index.test.tspackages/router/src/index.tspackages/store/src/index.test.tstemplates/elysia/package.jsontemplates/hono/package.jsontemplates/nitro/package.jsontemplates/vite/package.json
| const stop = persist(s, "p1b"); | ||
| // Eager, non-reactive read right after persist() — no effect/subscribe involved. | ||
| expect(s.getState()).toEqual({ count: 42, label: "restored" }); | ||
| stop(); | ||
| }); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Guarantee persistence cleanup when the assertion fails.
If the assertion throws, stop() is skipped, leaving the subscription and storage event listener registered for subsequent tests. Wrap the assertion in try/finally.
Proposed fix
const stop = persist(s, "p1b");
- // Eager, non-reactive read right after persist() — no effect/subscribe involved.
- expect(s.getState()).toEqual({ count: 42, label: "restored" });
- stop();
+ try {
+ // Eager, non-reactive read right after persist() — no effect/subscribe involved.
+ expect(s.getState()).toEqual({ count: 42, label: "restored" });
+ } finally {
+ stop();
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const stop = persist(s, "p1b"); | |
| // Eager, non-reactive read right after persist() — no effect/subscribe involved. | |
| expect(s.getState()).toEqual({ count: 42, label: "restored" }); | |
| stop(); | |
| }); | |
| const stop = persist(s, "p1b"); | |
| try { | |
| // Eager, non-reactive read right after persist() — no effect/subscribe involved. | |
| expect(s.getState()).toEqual({ count: 42, label: "restored" }); | |
| } finally { | |
| stop(); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/store/src/index.test.ts` around lines 1341 - 1345, Wrap the
assertion following persist(s, "p1b") in a try/finally block so stop() always
executes, including when expect(s.getState()) fails. Keep the existing state
expectation unchanged and place the cleanup call in the finally block.
1 participant
Summary by CodeRabbit
Bug Fixes
Documentation
Chores