Repository navigation
Add UI router scroll restoration - #609
Conversation
📝 WalkthroughWalkthroughAdds scroll restoration for client-side navigation, including router event details and target selection, a ChangesScroll Restoration Feature
Estimated code review effort: 4 (Complex) | ~60 minutes 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 |
|
🔎 Preview deployed: https://kody-pr-609.kentcdodds.workers.dev Worker: Mocks:
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
e2e/ssr-hydration.spec.ts (1)
61-114: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNo coverage for hash-target scroll restoration.
The docs (
docs/contributing/architecture/request-lifecycle.md, Lines 138-144) describe scrolling to hash targets as part of this feature, but this expanded flow only covers reset-to-top, restore-on-back, and prevent-scroll-reset. Consider adding an assertion for navigating to a URL with a hash to confirm the target scrolls into view, to fully cover the documented behavior.🤖 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 `@e2e/ssr-hydration.spec.ts` around lines 61 - 114, The SSR hydration test in ssr-hydration.spec.ts covers reset-to-top, back-navigation restore, and prevent-scroll-reset, but it does not verify hash-target scrolling. Extend the existing navigation flow around the community listing interactions to include a URL with a hash and assert the page scrolls to the anchored element, using the current page, scrollY checks, and the existing link/navigation helpers to confirm the documented hash scroll behavior.
🤖 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/worker/client/scroll-restoration.tsx`:
- Around line 131-137: The scroll restoration flow in scroll-restoration.tsx is
saving the outgoing page’s position using the destination history state during
navigationstart, which overwrites the entry for the page being returned to.
Update saveWindowScrollPosition and the router event wiring so the current entry
is captured before the history move, or switch the persistence logic to scroll
plus pagehide, and keep restoreWindowScroll using the correct saved key.
---
Nitpick comments:
In `@e2e/ssr-hydration.spec.ts`:
- Around line 61-114: The SSR hydration test in ssr-hydration.spec.ts covers
reset-to-top, back-navigation restore, and prevent-scroll-reset, but it does not
verify hash-target scrolling. Extend the existing navigation flow around the
community listing interactions to include a URL with a hash and assert the page
scrolls to the anchored element, using the current page, scrollY checks, and the
existing link/navigation helpers to confirm the documented hash scroll behavior.
🪄 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: 5a8ce3b5-7e40-4be0-88c8-54c87b68bf2e
📒 Files selected for processing (8)
docs/contributing/architecture/request-lifecycle.mde2e/d1-utils.tse2e/ssr-hydration.spec.tspackages/worker/client/app.tsxpackages/worker/client/client-router.tsxpackages/worker/client/router-scroll-state.node.test.tspackages/worker/client/router-scroll-state.tspackages/worker/client/scroll-restoration.tsx
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
There are 2 total unresolved issues (including 1 from previous review).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit a7286ed. Configure here.

Summary
preventScrollResetsupport for programmaticnavigate(...)anddata-prevent-scroll-resetfor intercepted links/forms.Walkthrough
Validation
npm run validatenpx playwright test e2e/ssr-hydration.spec.tsnpx vitest run --config vitest.node.config.ts packages/worker/client/router-scroll-state.node.test.ts packages/worker/client/client-router.node.test.tsSystem recap — extends existing primitives (medium risk)
Mode: recap · Base:
main@2a539a7· Head:c84d04eClassification: extends — this PR changes browser app routing behavior by adding scroll restoration semantics to SPA navigations.
Primitives touched
app-uicommunity-listingsSystem map
Change flow
Summary by CodeRabbit
data-prevent-scroll-resetorpreventScrollReset.