fix: restore virtualized list scroll positions - #657
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughスクロール位置の保存・復元処理を Changesスクロール位置復元
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant PageHook as ページフック
participant Restoration as useScrollRestoration
participant FetchNextPage as fetchNextPage
participant ScrollContainer as スクロールコンテナ
PageHook->>Restoration: 復元キー、準備状態、ページ取得状態を渡す
Restoration->>ScrollContainer: 現在の最大位置を測定
Restoration->>FetchNextPage: 保存位置が未ロード範囲なら追加取得
FetchNextPage-->>Restoration: 取得状態を更新
Restoration->>ScrollContainer: 保存位置までスクロール
ScrollContainer-->>Restoration: ユーザー操作で復元をキャンセル
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/ui/src/hooks/scroll-container.ts (2)
129-141: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win同期的な世代比較は常に成立します。
Line 131 で
generationにrestoreGenerationを代入します。Line 138 のgeneration !== restoreGenerationは同じ同期ブロック内の比較です。この条件は常に false です。ガードとして機能しません。
awaitの後で世代を検証する意図であれば、Line 168 の検証のみが有効です。Line 138 の条件は削除してください。♻️ 提案する修正
const attemptRestore = async () => { const key = activeKey; const generation = restoreGeneration; if ( !key || cancelled || isRestored() || settleTimer !== undefined || - !options.isReady() || - generation !== restoreGeneration + !options.isReady() ) { return; }🤖 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/ui/src/hooks/scroll-container.ts` around lines 129 - 141, Remove the ineffective generation !== restoreGeneration guard from attemptRestore’s initial synchronous condition; retain the later generation validation after the await, where restoreGeneration may have changed.
209-212: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
history.scrollRestorationを元の値に戻していません。このフックは
history.scrollRestorationを"manual"に変更します。onCleanupで元の値に戻しません。historyはグローバルな状態です。このフックを使わない他のページへ遷移した後も、ブラウザの自動スクロール復元は無効のままです。元の値を保存し、cleanup で復帰させてください。
♻️ 提案する修正
onMount(() => { + const previousScrollRestoration = + "scrollRestoration" in history ? history.scrollRestoration : undefined; if ("scrollRestoration" in history) { history.scrollRestoration = "manual"; } @@ onCleanup(() => { + if (previousScrollRestoration !== undefined) { + history.scrollRestoration = previousScrollRestoration; + } window.removeEventListener("pointerdown", cancelRestore);🤖 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/ui/src/hooks/scroll-container.ts` around lines 209 - 212, In the onMount callback, save the existing history.scrollRestoration value before setting it to "manual", and register an onCleanup handler that restores that saved value. Keep the history feature check and existing scroll-container behavior unchanged.
🤖 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/ui/src/hooks/scroll-container.ts`:
- Around line 178-184:
復元完了前の現在位置保存を防ぐため、packages/ui/src/hooks/scroll-container.ts の178-184行にある
savePosition の先頭で isRestored() を確認し、未完了なら即時 return
してください。packages/ui/src/hooks/use-source-media-page.ts の302-312行の
setPosition/setScrollPosition
は直接変更せず、修正後にソース切替から元のソースへ戻った際に保存済み位置が保持されることを確認してください。
---
Nitpick comments:
In `@packages/ui/src/hooks/scroll-container.ts`:
- Around line 129-141: Remove the ineffective generation !== restoreGeneration
guard from attemptRestore’s initial synchronous condition; retain the later
generation validation after the await, where restoreGeneration may have changed.
- Around line 209-212: In the onMount callback, save the existing
history.scrollRestoration value before setting it to "manual", and register an
onCleanup handler that restores that saved value. Keep the history feature check
and existing scroll-container behavior unchanged.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: b75f9aa7-5e4c-4e3b-b117-73d53b7d2d63
📒 Files selected for processing (3)
packages/ui/src/hooks/scroll-container.tspackages/ui/src/hooks/use-search-page.tspackages/ui/src/hooks/use-source-media-page.ts
概要
検索画面とsourceメディア一覧で、個別画面などへ遷移して戻った際のスクロール位置復元を仮想スクロール対応にします。
変更内容
検証
route-reload E2Eは既存のSSR側 template is not a function エラーにより実行時エラーとなります。
Summary by CodeRabbit
新機能
改善