19 allow successive navigation without intermediate rendering #20
Conversation
WalkthroughAdds NavMeta.skipRender and plumbing, introduces router.render(defer?) to batch/defer renders, extends router.go() to accept numeric targets forwarded to history.go(), bumps package versions to 0.11.0, updates tests and README documenting deferred rendering and wrapped history navigation. Changes
Sequence Diagram(s)sequenceDiagram
participant Dev
participant Router
participant History
participant PopState
rect #E8F0FF
Note over Dev,Router: Deferred rendering session (router.render)
Dev->>Router: render(async () => { ... })
activate Router
Router->>Router: set skipRender = true
Router->>Defer: invoke defer()
activate Defer
par Multiple navigations (skipRender = true)
Defer->>Router: go("/a", {skipRender:true})
Router->>History: pushState("/a")
Defer->>Router: go(-3)
Router->>History: go(-3)
History->>PopState: emit popstate
PopState->>Router: popstate event
Router->>Router: await popstate → resolve state
end
deactivate Defer
Router->>Router: set skipRender = false
Router->>Router: perform single render() → onResolve callbacks
deactivate Router
Note right of Dev: UI updated once
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (4 passed)
✨ Finishing touches
🧪 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: 4
🧹 Nitpick comments (1)
packages/esroute/src/router.ts (1)
135-141: Document the behavior of numeric navigation with skipRender.The numeric navigation path (
history.go(target)) waits for thepopstateevent before checkingskipRender. This means:
- The history navigation completes
- The URL changes
- Then skipRender determines whether to resolve/render
This is different from regular navigation where
skipRenderprevents resolution entirely.Consider adding a comment to clarify this behavior difference:
if (typeof target === "number") { + // Wait for browser history navigation to complete via popstate event const waiting = waitForPopState(); history.go(target); await waiting; + // After navigation completes, optionally skip resolution/rendering if ((opts && opts.skipRender) || skipRender) return; return resolveCurrent(); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (7)
packages/esroute-lit/package.json(2 hunks)packages/esroute/README.md(3 hunks)packages/esroute/package.json(1 hunks)packages/esroute/src/nav-opts.ts(4 hunks)packages/esroute/src/router.spec.ts(2 hunks)packages/esroute/src/router.ts(8 hunks)vitest.config.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
packages/esroute/src/router.spec.ts (2)
packages/esroute/src/router.ts (3)
onResolve(152-156)Router(6-58)createRouter(83-229)packages/esroute/src/nav-opts.ts (1)
NavOpts(32-89)
packages/esroute/src/router.ts (1)
packages/esroute/src/nav-opts.ts (5)
StrictNavMeta(22-30)NavOpts(32-89)NavMeta(3-20)PathOrHref(1-1)href(72-79)
🪛 LanguageTool
packages/esroute/README.md
[grammar] ~219-~219: Use a hyphen to join words.
Context: ...rmediate navigation. So any specified in guards or redirects within the render fu...
(QB_NEW_EN_HYPHEN)
🪛 markdownlint-cli2 (0.18.1)
packages/esroute/README.md
22-22: Link fragments should be valid
(MD051, link-fragments)
🔇 Additional comments (14)
vitest.config.ts (1)
9-9: LGTM! Timeout increase aligns with async navigation tests.The increased timeout accommodates the new asynchronous navigation and rendering tests that involve
PopStateEventsimulation and promise resolution.packages/esroute/README.md (1)
100-237: LGTM! Clear documentation of new navigation features.The documentation clearly explains:
- The
skipRenderoption for individual navigations- The
render(defer)method for batching navigations- Numeric navigation targets forwarding to
history.go()- Important behavioral notes about guards and render functions
packages/esroute/package.json (1)
3-3: LGTM! Version bump aligns with new feature additions.The minor version bump to 0.11.0 appropriately reflects the new public API additions (
render()method,skipRenderflag, numericgo()targets).packages/esroute-lit/package.json (1)
3-3: LGTM! Coordinated version updates.The version bump and dependency update correctly align with the esroute package changes.
Also applies to: 23-23
packages/esroute/src/nav-opts.ts (1)
18-19: LGTM! skipRender flag properly integrated.The
skipRenderflag is correctly added to both theNavMetainterface andNavOptsclass, with proper handling in the constructor following the same pattern as other optional flags.Also applies to: 37-37, 46-46, 68-68
packages/esroute/src/router.spec.ts (5)
1-1: LGTM! Improved test setup with typed mocks.The test setup now properly uses typed
Mockfrom vitest and passesonResolvethrough the router configuration, which is cleaner than the previous approach.Also applies to: 6-6, 9-9, 16-16
12-14: Verify timing reliability of the history.go mock.The mock uses
setTimeout(..., 0)to dispatch thepopstateevent asynchronously. This approach may be fragile in test environments with varying execution speeds.Consider whether this timing is reliable across different test environments. If tests become flaky, you may need to:
- Use a slightly longer timeout or
- Implement a more deterministic event dispatch mechanism in tests
96-101: LGTM! Test correctly validates skipRender behavior.The test confirms that when
skipRender: trueis passed,pushStateis called butonResolveis not triggered, which is the expected behavior.
103-116: Test validates deferred rendering correctly.The test confirms that:
- Multiple navigations inside
render(defer)don't trigger intermediate resolves- Only one final
onResolvecall occurs with the correct final state- History operations are batched appropriately
118-124: LGTM! Test confirms numeric navigation forwarding.The test validates that numeric targets are correctly forwarded to
history.go()and that resolution occurs afterward.packages/esroute/src/router.ts (4)
22-32: LGTM! Well-documented method overloads.The JSDoc clearly documents the new numeric target support and the function signature overloads properly handle all navigation patterns.
52-57: LGTM! render() method clearly documented.The documentation clearly explains the defer parameter and its purpose for batching navigations.
222-226: LGTM! Clean helper for async history navigation.The
waitForPopStatehelper correctly uses{ once: true }to ensure the listener is automatically removed after firing once.
183-203: LGTM! resolveCurrent properly handles both initial and popstate-driven resolution.The function correctly differentiates between:
- Initial resolution (no event, use
history.state)- Popstate-driven resolution (event with state)
sv2dev
force-pushed
the
19-allow-successive-navigation-without-intermediate-rendering
branch
from
c77bfe7 to
62f72a3
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/esroute/README.md (1)
21-21: Fix invalid link fragment (duplicate of previous review).The anchor
#-deferred-renderingdoes not match the heading at line 183. Markdown generates the anchor from the heading text without the leading dash.Apply this diff to fix the anchor:
-- [⏱️ Deferred rendering](#-deferred-rendering) +- [⏱️ Deferred rendering](#deferred-rendering)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (3)
packages/esroute-lit/package.json(2 hunks)packages/esroute/README.md(4 hunks)packages/esroute/package.json(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/esroute/package.json
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/esroute-lit/package.json
🧰 Additional context used
🪛 LanguageTool
packages/esroute/README.md
[grammar] ~196-~196: Use a hyphen to join words.
Context: ...rmediate navigation. So any specified in guards or redirects within the render fu...
(QB_NEW_EN_HYPHEN)
🪛 markdownlint-cli2 (0.18.1)
packages/esroute/README.md
21-21: Link fragments should be valid
(MD051, link-fragments)
🔇 Additional comments (2)
packages/esroute/README.md (2)
42-54: Documentation of wrapped history API is clear and accurate.The new subsection properly explains numeric target support in
router.go()and the relationship with theskipRenderflag. Examples and async behavior notes are helpful.
183-213: Deferred rendering documentation is comprehensive and accurate.The new section clearly explains both the callback-based (
router.render()) and option-based (skipRender) APIs for deferring renders. The examples effectively demonstrate the three equivalent patterns and the behavioral note about guard/render execution for intermediate navigations is important and correct. This aligns well with the PR objectives.
Also forward to history go, if target is a number.
sv2dev
force-pushed
the
19-allow-successive-navigation-without-intermediate-rendering
branch
from
62f72a3 to
1a4cdfb
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/esroute/src/router.ts (1)
157-166: Good defensive cleanup around deferred rendering.Wrapping defer() in try/finally ensures skipRender resets on errors. This addresses the earlier hazard.
🧹 Nitpick comments (6)
packages/esroute/README.md (1)
195-196: Clarify the sentence structure for better readability.The phrase "any specified guards or redirects within the render functions" is still somewhat ambiguous and could be restructured for clarity.
Apply this diff to improve the sentence:
-One thing to note is that no guards and render functions will be executed for intermediate navigation. -So any specified guards or redirects within the render functions will only be executed for the last navigation. +One thing to note is that guards and render functions will not be executed for intermediate navigation. +Therefore, only guards and redirects specified for the final navigation will be executed.vitest.config.ts (1)
9-9: 1s global test timeout risks flakiness for async/popstate tests.Prefer default (usually 5s) or set tighter timeouts per test/suite that truly need it.
If you intend to lower globally, confirm Vitest’s current default to avoid accidental tightening.
packages/esroute/src/router.ts (1)
120-123: Navigation “serialization” can still race on first calls.Awaiting this.resolution is a no-op when it’s undefined; two concurrent go() before the first resolution may proceed in parallel. Initialize resolution or queue to guarantee ordering.
Apply one of:
- let resolution: Promise<Resolved<T>>; + let resolution: Promise<Resolved<T>> = Promise.resolve(undefined as unknown as Resolved<T>);Or wrap updates through a micro-queue:
- const prevRes = await this.resolution; + const prevRes = await (this.resolution ?? Promise.resolve(_current));packages/esroute/src/router.spec.ts (3)
12-14: Mocking history.go via timed popstate is fine; keep time budget in mind.With a 1s global timeout, additional async work could flake; consider using vi.useFakeTimers() with runAllTimers() to make this deterministic.
40-56: Duplicate test title.Both tests are titled “should subscribe to popstate and anchor click events”. Rename the second for clarity (e.g., “…via anchor click”).
- it("should subscribe to popstate and anchor click events", async () => { + it("should handle navigation via anchor click", async () => {
103-116: Assertion on NavOpts instance may be brittle (state null vs undefined).Prefer partial/objectContaining to avoid strict equality on optional fields.
- expect(onResolve).toHaveBeenCalledWith({ - opts: new NavOpts("/foo", { pop: false }), - value: "foo", - }); + expect(onResolve).toHaveBeenCalledWith({ + value: "foo", + opts: expect.objectContaining(new NavOpts("/foo", { pop: false })), + });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (7)
packages/esroute-lit/package.json(2 hunks)packages/esroute/README.md(4 hunks)packages/esroute/package.json(1 hunks)packages/esroute/src/nav-opts.ts(4 hunks)packages/esroute/src/router.spec.ts(2 hunks)packages/esroute/src/router.ts(8 hunks)vitest.config.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/esroute/package.json
- packages/esroute-lit/package.json
- packages/esroute/src/nav-opts.ts
🧰 Additional context used
🧬 Code graph analysis (2)
packages/esroute/src/router.spec.ts (2)
packages/esroute/src/router.ts (3)
onResolve(152-156)Router(6-58)createRouter(83-232)packages/esroute/src/nav-opts.ts (1)
NavOpts(32-89)
packages/esroute/src/router.ts (1)
packages/esroute/src/nav-opts.ts (5)
StrictNavMeta(22-30)NavOpts(32-89)NavMeta(3-20)PathOrHref(1-1)href(72-79)
🪛 markdownlint-cli2 (0.18.1)
packages/esroute/README.md
21-21: Link fragments should be valid
(MD051, link-fragments)
🔇 Additional comments (6)
packages/esroute/README.md (2)
42-54: Verify the subsection heading level is intentional.The new "Wrapped history navigation API" subsection uses
####(level 4), which differs from the sibling subsections like "Simple configuration" (line 56, level 3) and "Typesafe value resolution" (line 93, level 3). If this is meant to be at the same hierarchical level as other top-level features, it should use###(level 3) instead.Check whether this subsection should be promoted to level 3 to maintain consistent hierarchy with the other navigation-related subsections:
-#### Wrapped history navigation API +### Wrapped history navigation API
42-213: Documentation of deferred rendering feature is well-structured.The new subsections effectively communicate the deferred rendering capability with clear examples showing both the
render(callback)andskipRenderoption approaches. The examples at lines 189–192, 203–205, and 210–213 successfully illustrate the three equivalent patterns.packages/esroute/src/router.ts (3)
135-141: Numeric navigation flow and skip condition look correct.waitForPopState before history.go avoids missed events; early return on skipRender or opts?.skipRender prevents double render during batching.
169-172: Popstate gating aligns with deferred rendering contract.Early exit when skipRender is true avoids intermediate renders while batching.
148-148: Skip-render branch uses push/replace without resolution.This matches the contract; just note that listeners won’t fire until a later render() or popstate-triggered resolve.
Ensure README clearly states that skipRender suppresses onResolve until a later render.
packages/esroute/src/router.spec.ts (1)
118-124: Numeric go() expectations look right.Single onResolve invocation is expected since init() isn’t called and go() triggers resolveCurrent itself.
sv2dev
deleted the
19-allow-successive-navigation-without-intermediate-rendering
branch
Resolves #19
Summary by CodeRabbit
New Features
Documentation
Tests
Chores