feat: implement keyset cursor pagination for note pages (issue #860 Phase 3)#866
Merged
Conversation
…Phase 3) PageGrid now consumes `GET /api/notes/:noteId/pages` via the new `useInfiniteNotePages` hook (keyset cursor pagination from Phase 1/2). The virtualizer triggers `fetchNextPage()` when the visible range approaches the loaded tail, so initial render is bounded to one window regardless of note size. Server order (`updated_at DESC, id DESC`) is trusted verbatim; the client only reapplies sort/filter for personal pages backed by IndexedDB. NoteAddPageDialog drops the `notePages: PageSummary[]` prop. The dedup filter was a post-#823 no-op (note-native ids never collide with personal page ids), so removing it severs the last full-array dependency from the note view. Mutations (`useAddPageToNote`, `useCopyPersonalPageToNote`, `useRemovePageFromNote`) now also invalidate `noteKeys.pagesWindowByNoteId` so the windowed cache refreshes after add / copy / remove. https://claude.ai/code/session_013vsh2Xaig91WcEvsKCzzDb
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds keyset cursor pagination for note page windows: new API types and client ChangesInfinite Cursor Pagination for Note Pages
Sequence DiagramsequenceDiagram
participant PageGrid
participant useInfiniteNotePages
participant ReactQuery
participant ApiClient
PageGrid->>useInfiniteNotePages: call hook with noteId
useInfiniteNotePages->>ReactQuery: useInfiniteQuery(cursor=null)
ReactQuery->>ApiClient: GET /api/notes/:noteId/pages?cursor=null&limit=50&include=...
ApiClient-->>ReactQuery: { items, next_cursor }
ReactQuery-->>useInfiniteNotePages: pages, hasNextPage, fetchNextPage
useInfiniteNotePages-->>PageGrid: flattened pages array + controls
PageGrid->>PageGrid: detect virtual range near tail
alt hasNextPage && !isFetchingNextPage
PageGrid->>useInfiniteNotePages: fetchNextPage()
useInfiniteNotePages->>ReactQuery: fetch next cursor
ReactQuery->>ApiClient: GET /api/notes/:noteId/pages?cursor=...
ApiClient-->>ReactQuery: { items, next_cursor }
ReactQuery-->>PageGrid: append pages to state
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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
🧹 Nitpick comments (1)
src/hooks/useNoteQueries.ts (1)
80-95: ⚡ Quick winNormalize
includetokens inpagesWindowkeys to avoid cache fragmentation.The key currently sorts but does not de-duplicate
include, while the request layer de-duplicates before sending. That can create multiple cache entries for the same effective query.♻️ Proposed fix
pagesWindow: ( noteId: string, userId: string, userEmail: string | undefined, include: ReadonlyArray<NotePageWindowInclude>, pageSize: number, ) => + { + const includeKey = Array.from(new Set(include)).sort().join(","); + return [ + ...noteKeys.pages(), + "window", + noteId, + userId, + userEmail ?? "", + includeKey, + pageSize, + ] as const; + }, - [ - ...noteKeys.pages(), - "window", - noteId, - userId, - userEmail ?? "", - [...include].sort().join(","), - pageSize, - ] as const,🤖 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 `@src/hooks/useNoteQueries.ts` around lines 80 - 95, pagesWindow key generation sorts the include tokens but doesn't remove duplicates, causing cache fragmentation; update the pagesWindow implementation to first de-duplicate the include array (e.g., using new Set or similar) then sort and join it (replace [...include].sort().join(",") with something like Array.from(new Set(include)).sort().join(",")) so the key represents the same effective query as the request layer; ensure you reference the pagesWindow function and keep the rest of the tuple 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 `@src/hooks/useNoteQueries.ts`:
- Line 629: The JSDoc comment currently only contains Japanese ("ISO 文字列 → ms
整数。パースできなければ 0 を返す。"); add an English counterpart in the same comment block so
it is bilingual — e.g. append or prepend "ISO string → milliseconds (number).
Returns 0 if parsing fails." — ensuring the comment remains a single /** ... */
doc comment above the function that converts ISO strings to ms.
---
Nitpick comments:
In `@src/hooks/useNoteQueries.ts`:
- Around line 80-95: pagesWindow key generation sorts the include tokens but
doesn't remove duplicates, causing cache fragmentation; update the pagesWindow
implementation to first de-duplicate the include array (e.g., using new Set or
similar) then sort and join it (replace [...include].sort().join(",") with
something like Array.from(new Set(include)).sort().join(",")) so the key
represents the same effective query as the request layer; ensure you reference
the pagesWindow function and keep the rest of the tuple 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
Run ID: 481196da-880d-4023-aa74-56c1f20e4adf
📒 Files selected for processing (10)
src/components/page/PageGrid.test.tsxsrc/components/page/PageGrid.tsxsrc/hooks/useInfiniteNotePages.test.tsxsrc/hooks/useNoteQueries.tssrc/lib/api/apiClient.test.tssrc/lib/api/apiClient.tssrc/lib/api/types.tssrc/pages/NoteView/NoteAddPageDialog.tsxsrc/pages/NoteView/NoteView.test.tsxsrc/pages/NoteView/index.tsx
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements keyset cursor pagination for note pages (Issue #860 Phase 3). It introduces the useInfiniteNotePages hook, adds a corresponding getNotePages method to the API client, and updates the PageGrid component to support infinite scrolling via TanStack Virtual. Additionally, redundant page filtering logic was removed from the NoteAddPageDialog. Feedback was provided regarding the efficiency of a useEffect dependency in PageGrid.tsx, suggesting that depending on the last visible index instead of the entire virtualRows array would prevent unnecessary effect executions during scrolling.
Comment on lines
+260
to
+276
| useEffect(() => { | ||
| if (!isNoteContext) return; | ||
| if (!noteInfinite.hasNextPage || noteInfinite.isFetchingNextPage) return; | ||
| if (virtualRows.length === 0) return; | ||
| const lastVisibleRow = virtualRows[virtualRows.length - 1]; | ||
| if (!lastVisibleRow) return; | ||
| if (lastVisibleRow.index >= rowCount - INFINITE_FETCH_THRESHOLD_ROWS) { | ||
| noteInfinite.fetchNextPage(); | ||
| } | ||
| }, [ | ||
| isNoteContext, | ||
| noteInfinite.hasNextPage, | ||
| noteInfinite.isFetchingNextPage, | ||
| noteInfinite.fetchNextPage, | ||
| virtualRows, | ||
| rowCount, | ||
| ]); |
There was a problem hiding this comment.
The useEffect dependency array includes virtualRows, which is an array returned by rowVirtualizer.getVirtualItems(). This array is recreated on every scroll event, causing the effect to run very frequently. While the internal logic is guarded, it's more efficient to depend only on the specific value that triggers the fetch, such as the index of the last visible row.
Suggested change
| useEffect(() => { | |
| if (!isNoteContext) return; | |
| if (!noteInfinite.hasNextPage || noteInfinite.isFetchingNextPage) return; | |
| if (virtualRows.length === 0) return; | |
| const lastVisibleRow = virtualRows[virtualRows.length - 1]; | |
| if (!lastVisibleRow) return; | |
| if (lastVisibleRow.index >= rowCount - INFINITE_FETCH_THRESHOLD_ROWS) { | |
| noteInfinite.fetchNextPage(); | |
| } | |
| }, [ | |
| isNoteContext, | |
| noteInfinite.hasNextPage, | |
| noteInfinite.isFetchingNextPage, | |
| noteInfinite.fetchNextPage, | |
| virtualRows, | |
| rowCount, | |
| ]); | |
| const lastVirtualIndex = virtualRows.length > 0 ? virtualRows[virtualRows.length - 1].index : -1; | |
| useEffect(() => { | |
| if (!isNoteContext) return; | |
| if (!noteInfinite.hasNextPage || noteInfinite.isFetchingNextPage) return; | |
| if (lastVirtualIndex === -1) return; | |
| if (lastVirtualIndex >= rowCount - INFINITE_FETCH_THRESHOLD_ROWS) { | |
| noteInfinite.fetchNextPage(); | |
| } | |
| }, [ | |
| isNoteContext, | |
| noteInfinite.hasNextPage, | |
| noteInfinite.isFetchingNextPage, | |
| noteInfinite.fetchNextPage, | |
| lastVirtualIndex, | |
| rowCount, | |
| ]); |
- Depend on `lastVirtualIndex` (scalar) instead of the `virtualRows` array in `PageGrid`'s tail-detection effect. `getVirtualItems()` returns a fresh array on every scroll, so the previous dependency made the effect re-run every frame even when the trigger condition could not have changed (gemini-code-assist on PR #866). - Replace the misplaced JP-only docstring on `notePageWindowItemToSummary` in `useNoteQueries.ts`. The old comment described `parseTs`, not this mapper. Now a proper bilingual JSDoc explains the snake_case → camelCase mapping and why `addedByUserId` is sourced from `owner_id` post-#823 (coderabbitai on PR #866). https://claude.ai/code/session_013vsh2Xaig91WcEvsKCzzDb
This was referenced May 14, 2026
Merged
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
2 participants
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.
概要
ノートのページ一覧取得を keyset cursor pagination で段階取得する機能を実装しました。
useInfiniteNotePagesフックを新規追加し、React Query のuseInfiniteQueryを使用して大規模ノートでも効率的にページを読み込めるようにしました。変更点
useInfiniteNotePages: keyset cursor pagination で段階取得するフック。fetchNextPage()で次の window を取得でき、仮想スクロール対応が容易getNotePages()メソッドを追加。cursor,limit,includeパラメータをサポートnoteKeys.pagesWindow()とnoteKeys.pagesWindowByNoteId()を追加。include / pageSize の組み合わせごとにキャッシュを分離useNotePages()からuseInfiniteNotePages()に切り替え。仮想スクロール末尾検知で自動的に次の window を取得pagesWindowByNoteIdキーで全 window キャッシュを無効化変更の種類
テスト方法
bun run testで単体テストが全てパスuseInfiniteNotePages.test.tsx: cursor スレッド、フラット化、include / pageSize 反映を検証apiClient.test.ts:getNotePages()のクエリパラメータ エンコーディングを検証PageGrid.test.tsx: 仮想スクロール末尾検知でfetchNextPage()が呼ばれることを検証bun run lintとbun run format:checkが通るチェックリスト
関連 Issue
Closes #860 Phase 3
https://claude.ai/code/session_013vsh2Xaig91WcEvsKCzzDb
Summary by CodeRabbit
New Features
Refactor
Behavior
Tests