fix: preserve search scroll and recover blank virtual grid - #658
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthrough検索結果のキャッシュ保持、 Changes検索結果とメディア結果の保持
検索状態とスクロール位置の復元
ギャラリーの復元と追加取得
仮想グリッドとスクロール対象の更新
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant VirtualGridGallery
participant useScrollRestoration
participant SourceMediaGrid
participant Browser
VirtualGridGallery->>useScrollRestoration: 復元位置と追加取得状態を渡す
useScrollRestoration->>VirtualGridGallery: 追加取得処理を実行する
VirtualGridGallery->>SourceMediaGrid: ページ結果と取得状態を渡す
SourceMediaGrid->>Browser: 画像をeager loadingで有効化する
Browser-->>SourceMediaGrid: 画像読み込み状態を返す
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/ui/src/hooks/use-current-search-persistence.test.ts (1)
188-200: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winベクターモードの
scrollY保持をテストしてください。現在の
mode: "vector"のテストは legacy のselectedSource分岐だけを検証しています。mode: "vector"でsearchState.scrollYを設定してからscrollYが保持されることを単体テストで追加してください。🤖 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/use-current-search-persistence.test.ts` around lines 188 - 200, Extend the current scroll-position persistence test coverage for vector mode by configuring the test state with mode: "vector" and setting searchState.scrollY before restoring the persisted search state. Assert that the vector-mode restore path preserves the same scrollY value, alongside the existing legacy selectedSource coverage.
🤖 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.
Nitpick comments:
In `@packages/ui/src/hooks/use-current-search-persistence.test.ts`:
- Around line 188-200: Extend the current scroll-position persistence test
coverage for vector mode by configuring the test state with mode: "vector" and
setting searchState.scrollY before restoring the persisted search state. Assert
that the vector-mode restore path preserves the same scrollY value, alongside
the existing legacy selectedSource coverage.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 7c113cb3-6825-4ffb-9c65-a78214979ded
📒 Files selected for processing (3)
packages/ui/src/hooks/use-current-search-persistence.test.tspackages/ui/src/hooks/use-current-search-persistence.tspackages/ui/src/source-media-grid.tsx
Keep mounted virtual rows loadable while navigation restoration settles and persist user-owned scroll positions when restoration is cancelled. Add a paginated deep-scroll regression fixture covering post-restore scrolling.
Re-measure the nested virtualizer when its scroll container and content offset become available, and eagerly load mounted rows while the initial range is settling.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@apps/server/src/tests/e2e/ui-components.gallery.spec.ts`:
- Around line 268-270: Strengthen the scroll restoration assertion in the
scroller polling block by requiring scrollTop to reach a lower bound close to
the intended 20,000 px restoration target, rather than merely exceeding 1,000.
Keep the existing polling behavior and ensure the threshold still allows only a
near-target restored position to pass.
🪄 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: 2ff29b0c-ce75-4421-b57e-db3f1407ee5c
📒 Files selected for processing (4)
apps/server/src/tests/e2e/ui-components.gallery.spec.tsapps/server/src/tests/e2e/ui-gallery/src.tsxpackages/ui/src/hooks/scroll-container.tspackages/ui/src/source-media-grid.tsx
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/source-media-page.tsx`:
- Around line 125-128: Update the scrollContainerSelector assignment in
V2SourceMediaScreen to apply the same fallback as the rendered data-media-scroll
attribute: use props.mediaSourceId() with "v2-source" when undefined, so the
selector targets the actual scroll container.
🪄 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: 832cb305-cbf7-494d-b7c7-3ceba062e54a
📒 Files selected for processing (4)
apps/server/src/routes/v2/search.tsxpackages/ui/src/screens/v2-search-screen.tsxpackages/ui/src/screens/v2-source-media-screen.tsxpackages/ui/src/source-media-page.tsx
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@apps/server/src/tests/e2e/ui-components.gallery.spec.ts`:
- Around line 279-329: Strengthen the final scroll-position assertion in the
test “virtual media grid preserves offset after a route-style remount and page
update” to validate that scrollTop reaches a value close to the computed nextTop
target, rather than only exceeding restoredTop by 100. Keep the existing append
and image-loading checks 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: 790e44e8-c65e-4405-afdb-5e0ee172ee24
📒 Files selected for processing (4)
apps/server/src/tests/e2e/ui-components.gallery.spec.tsapps/server/src/tests/e2e/ui-gallery/src.tsxpackages/ui/src/hooks/scroll-container.tspackages/ui/src/source-media-grid.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/ui/src/source-media-grid.tsx
- apps/server/src/tests/e2e/ui-gallery/src.tsx
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/ui/src/hooks/use-search-page.ts (1)
1-1: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
dataUpdatedAtへの裸のアクセスにvoidを付けてください。 両ファイルの新しいキャッシュ同期用createEffectは、リアクティブな依存関係を張るためだけにdataUpdatedAtを参照しますが、戻り値を使わない式文のままです。use-source-media-page.ts の347行目にある既存のvoid hasNext;と同じ表記に揃えてください。
packages/ui/src/hooks/use-search-page.ts#L184-204:searchResultQuery.dataUpdatedAt;をvoid searchResultQuery.dataUpdatedAt;に変更してください。packages/ui/src/hooks/use-source-media-page.ts#L272-289:mediaQuery.dataUpdatedAt;をvoid mediaQuery.dataUpdatedAt;に変更してください。🤖 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/use-search-page.ts` at line 1, Update the cache-synchronization createEffect blocks in use-search-page.ts and use-source-media-page.ts to prefix the unused reactive accesses searchResultQuery.dataUpdatedAt and mediaQuery.dataUpdatedAt with void, matching the existing void hasNext style while preserving their reactive dependency behavior.Source: Coding guidelines
🧹 Nitpick comments (3)
packages/ui/src/hooks/use-search-page.ts (2)
1-1: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winキャッシュ同期とデデュープのロジックが2ファイルで重複しています。 どちらのフックも、
queryClient.getQueryDataによるキャッシュ読み取り、InfiniteDataシグナルへの同期、Setを使った重複除去、contentStateの組み立てまで、ほぼ同一の実装です。共有フック(例:useCachedInfiniteQueryResult)への抽出を検討してください。
packages/ui/src/hooks/use-search-page.ts#L184-227:searchResultQueryOptions/searchResultQuery/searchResultData周りのキャッシュ同期・デデュープ・contentState生成ロジックを共有ヘルパーへ切り出してください。packages/ui/src/hooks/use-source-media-page.ts#L260-313:mediaQueryOptions/mediaQuery/mediaQueryData周りの同一ロジックを同じ共有ヘルパーに置き換えてください。🤖 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/use-search-page.ts` at line 1, Extract the duplicated cache synchronization, InfiniteData signal updates, Set-based deduplication, and contentState construction from use-search-page.ts and use-source-media-page.ts into a shared hook such as useCachedInfiniteQueryResult. Update the searchResultQuery/searchResultData and mediaQuery/mediaQueryData flows to use that helper while preserving their existing query options and resulting content state behavior.
184-227: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winキャッシュ同期処理が use-source-media-page.ts と重複しています。
この効果とデデュープ処理(184-227行)は、use-source-media-page.ts の272-313行とほぼ同一の実装です。共通ロジックを共有フックへ抽出することを検討してください。詳細は本レビューの
consolidated_commentsセクションを参照してください。🤖 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/use-search-page.ts` around lines 184 - 227, Extract the duplicated cache-synchronization effect and page-result deduplication currently used by searchResults/contentState into a shared hook or utility, then reuse it from both use-search-page and use-source-media-page. Preserve the existing query-key cache updates, placeholder handling, deduplication by media id, and UI state behavior while removing the duplicated implementations.packages/ui/src/hooks/use-source-media-page.ts (1)
260-313: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winキャッシュ同期処理が use-search-page.ts と重複しています。
この効果とデデュープ処理(260-313行)は、use-search-page.ts の184-227行とほぼ同一の実装です。詳細は本レビューの
consolidated_commentsセクションを参照してください。🤖 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/use-source-media-page.ts` around lines 260 - 313, Remove the duplicated cache-synchronization and result-deduplication logic from the media page flow around mediaQueryData, the createEffect, mediaResults, and contentState. Reuse the existing shared implementation or abstraction established by use-search-page.ts, preserving the current paginated data behavior and query UI state handling.
🤖 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 `@apps/server/src/tests/e2e/ui-components.gallery.spec.ts`:
- Around line 331-364: このテストのスクロール位置アサーションが低すぎます。テスト「virtual media grid
preserves offset while fast scrolling starts a page
fetch」で、1,500pxへ設定した位置の復元確認を実際の目標値に近い下限へ引き上げ、追加読み込み後の確認も restoredTop
から想定される実際の増加量に近い下限へ強化してください。
In `@packages/ui/src/hooks/use-source-media-page.ts`:
- Around line 272-289: Update the createEffect block that reads
mediaQuery.dataUpdatedAt to use the same explicit void-expression style as the
nearby hasNext reference, without changing the surrounding cache synchronization
logic.
---
Outside diff comments:
In `@packages/ui/src/hooks/use-search-page.ts`:
- Line 1: Update the cache-synchronization createEffect blocks in
use-search-page.ts and use-source-media-page.ts to prefix the unused reactive
accesses searchResultQuery.dataUpdatedAt and mediaQuery.dataUpdatedAt with void,
matching the existing void hasNext style while preserving their reactive
dependency behavior.
---
Nitpick comments:
In `@packages/ui/src/hooks/use-search-page.ts`:
- Line 1: Extract the duplicated cache synchronization, InfiniteData signal
updates, Set-based deduplication, and contentState construction from
use-search-page.ts and use-source-media-page.ts into a shared hook such as
useCachedInfiniteQueryResult. Update the searchResultQuery/searchResultData and
mediaQuery/mediaQueryData flows to use that helper while preserving their
existing query options and resulting content state behavior.
- Around line 184-227: Extract the duplicated cache-synchronization effect and
page-result deduplication currently used by searchResults/contentState into a
shared hook or utility, then reuse it from both use-search-page and
use-source-media-page. Preserve the existing query-key cache updates,
placeholder handling, deduplication by media id, and UI state behavior while
removing the duplicated implementations.
In `@packages/ui/src/hooks/use-source-media-page.ts`:
- Around line 260-313: Remove the duplicated cache-synchronization and
result-deduplication logic from the media page flow around mediaQueryData, the
createEffect, mediaResults, and contentState. Reuse the existing shared
implementation or abstraction established by use-search-page.ts, preserving the
current paginated data behavior and query UI state handling.
🪄 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: eb6ceff8-965b-4dc7-b191-54d5eafa5792
📒 Files selected for processing (8)
apps/server/src/tests/e2e/ui-components.gallery.spec.tsapps/server/src/tests/e2e/ui-gallery/src.tsxpackages/ui/src/hooks/use-search-page.tspackages/ui/src/hooks/use-source-media-page.tspackages/ui/src/screens/search-screen.tsxpackages/ui/src/screens/source-media-screen.tsxpackages/ui/src/screens/v2-search-screen.tsxpackages/ui/src/screens/v2-source-media-screen.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/ui/src/screens/v2-search-screen.tsx
- packages/ui/src/screens/v2-source-media-screen.tsx
Owner
Author
原因と検証結果本番で発生していた主因は、メディア一覧の高速スクロール時に実行される次ページ取得が、複数の長時間SSE接続と競合して遅延・滞留していたことでした。
加えて、UI戻り直後に検索状態保存の1秒debounceが未完了だと、戻り時に初期状態へ復元される競合もありました。 対応内容:
再発防止として、600件超の実データを使い、検索/source × ブラウザ戻る/UI戻るの4ケースで、offset: 200のAPIを遅延させた高速スクロールE2Eを追加しました。mainとscroll elementのDOM identity、scrollTop維持、page追加後のscrollHeight増加を検証し、開発環境・production buildともに4/4通過しています。 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
概要
取り込み済みのPR #657後に確認された、検索画面のスクロール未保存とsource一覧の復帰後のグリッド白画面を修正します。
変更内容
検証
Summary by CodeRabbit