feat(api): keyset pagination + index for note pages (#860 Phase 1/2)#865
Conversation
Phase 1 — `GET /api/notes/:noteId/pages` を `authRequired` から
`authOptional` + `getNoteRole` に切り替え、公開 / unlisted ノートの guest
でも取得可能にする。同時に keyset cursor pagination
(`?cursor=&limit=&include=`) を導入し、レスポンスを
`{ items, next_cursor }` の新形状にする。`content_preview` /
`thumbnail_url` は `include=preview` / `include=thumbnail` 時のみ
セットする。
Phase 2 — keyset pagination 用に
`pages (note_id, updated_at DESC, id DESC) WHERE is_deleted = false`
部分複合インデックスを追加する。`(updated_at, id)` の tie-break
予測子も index 内で解決できるよう既存
`idx_pages_note_active_updated` の隣に併設する。
https://claude.ai/code/session_016XUBJqunaZPnLTdB4eFTzt
|
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)
📝 WalkthroughWalkthroughThis PR implements keyset cursor pagination for GET /api/notes/:noteId/pages by adding a partial index, new response types, cursor encoding/decoding and query parsing, handler logic that returns { items, next_cursor } with optional preview/thumbnail inclusion, switching the route to authOptional, and updated tests. ChangesNote pages cursor pagination
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![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 the GET /api/notes/:noteId/pages endpoint, supported by a new partial composite index on the pages table. The implementation includes cursor encoding/decoding, limit clamping, and optional field inclusion for previews and thumbnails. Feedback from the review identifies a critical precision loss issue where JavaScript's millisecond-precision Date objects may skip records when compared against PostgreSQL's microsecond-precision timestamps; it is recommended to use high-precision strings for cursor values. Additionally, a suggestion was made to use the Drizzle eq operator instead of raw SQL for timestamp comparisons to maintain consistency and readability.
| const keysetPredicate = or( | ||
| lt(pages.updatedAt, cursorTs), | ||
| and(sql`${pages.updatedAt} = ${cursorTs}`, lt(pages.id, cursor.id)), | ||
| ); |
There was a problem hiding this comment.
The current keyset pagination logic is susceptible to precision loss. JavaScript's Date object and toISOString() only support millisecond precision, whereas PostgreSQL's timestamp columns (and the defaultNow() used in the schema) support microsecond precision.
When a cursor is generated from a row with microseconds (e.g., .123456), it is truncated to milliseconds (e.g., .123) in the cursor payload. The subsequent query using lt(pages.updatedAt, cursorTs) will then skip any rows that fall within that same millisecond but have higher microsecond values than the truncated cursor.
To fix this, you should fetch the updated_at value as a high-precision string in the select clause (using to_char) and use that string directly in the SQL comparison with a cast, rather than converting it to a JS Date object.
| hasMore && last | ||
| ? encodePagesCursor({ | ||
| updatedAt: | ||
| last.updatedAt instanceof Date ? last.updatedAt.toISOString() : String(last.updatedAt), |
There was a problem hiding this comment.
As mentioned in the previous comment regarding precision, toISOString() truncates microseconds. For a robust keyset cursor, consider selecting updated_at as a formatted string with full microsecond precision directly from the database.
| // without resorting to a non-null assertion. | ||
| const keysetPredicate = or( | ||
| lt(pages.updatedAt, cursorTs), | ||
| and(sql`${pages.updatedAt} = ${cursorTs}`, lt(pages.id, cursor.id)), |
There was a problem hiding this comment.
Using sql for a simple equality check is unnecessary here. You can use the eq operator from Drizzle for better readability and consistency with the rest of the codebase.
| and(sql`${pages.updatedAt} = ${cursorTs}`, lt(pages.id, cursor.id)), | |
| and(eq(pages.updatedAt, cursorTs), lt(pages.id, cursor.id)), |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4fcd712c90
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| updatedAt: | ||
| last.updatedAt instanceof Date ? last.updatedAt.toISOString() : String(last.updatedAt), |
There was a problem hiding this comment.
next_cursor is built from last.updatedAt.toISOString(), which only keeps millisecond precision, but pages.updated_at is timestamp with time zone (microsecond precision by default). When two rows on a page boundary share the same millisecond but different microseconds, the follow-up predicate (updated_at < cursorTs OR (updated_at = cursorTs AND id < ...)) can skip valid rows entirely because the cursor timestamp was truncated. This makes keyset pagination lossy under real write patterns (e.g., fast inserts/updates). Store a lossless sort key in the cursor (e.g., raw DB timestamp string or epoch micros) and compare against that exact precision.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@server/api/src/__tests__/routes/notes/pages.test.ts`:
- Line 151: Update the English-only test comments in
server/api/src/__tests__/routes/notes/pages.test.ts to include Japanese
translations alongside the English text; specifically replace/comment the line
containing "Builds a page row matching the new SELECT in `pages.ts`." and the
other referenced comment lines (the ones around the same test block and the
comments that appear near the SELECT-related tests) with bilingual comments —
English followed by Japanese — keeping the same meaning and context for each
comment.
In `@server/api/src/routes/notes/pages.ts`:
- Around line 85-92: The decoded cursor validation in decodePagesCursor
currently only checks that id is a string but not that it's a valid UUID, which
can leak invalid values into the DB keyset predicate; update decodePagesCursor
to validate cursor.id as a proper UUID (e.g., using a UUID regex or util/isUuid
helper) and throw HTTPException(400, { message: "Invalid cursor" }) when it
fails; ensure the function (decodePagesCursor) returns only { updatedAt, id }
with id guaranteed to be a valid UUID so downstream code that builds the keyset
predicate (using cursor.id) never receives invalid input.
🪄 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: b6af5ef8-84ec-4c4b-89da-50d56c2d7f95
📒 Files selected for processing (7)
server/api/drizzle/0027_add_pages_note_active_updated_id_index.sqlserver/api/drizzle/meta/_journal.jsonserver/api/src/__tests__/routes/notes/pages.test.tsserver/api/src/__tests__/routes/notes/setup.tsserver/api/src/routes/notes/pages.tsserver/api/src/routes/notes/types.tsserver/api/src/schema/pages.ts
💤 Files with no reviewable changes (1)
- server/api/src/tests/routes/notes/setup.ts
PR #865 のレビュー指摘への対応: - keyset cursor の `updatedAt` を pg 側で `to_char(... 'YYYY-MM-DD"T"HH24:MI:SS.US"Z"')` 経由のマイクロ秒 ISO 文字列として持ち回し、比較は `::timestamptz` キャストで 突合する。pg ドライバ経由で JS Date に丸まる経路を断ち、同一ミリ秒で別マイクロ秒 の行を取りこぼす不具合を防ぐ (gemini-code-assist / chatgpt-codex on #865)。 - cursor.id を RFC 4122 UUID 正規表現で 400 に倒し、pg `uuid` キャストの `22P02` 500 を未然に防ぐ (coderabbitai on #865)。 - tie-break を `sql\`=\`` から `eq()` に統一 (gemini-code-assist medium)。 - 新規追加した英語コメントに日本語訳を併記 (coderabbitai on #865)。 - マイクロ秒精度保持と非 UUID 拒否のリグレッションテストを追加。 https://claude.ai/code/session_016XUBJqunaZPnLTdB4eFTzt
2 participants
Phase 1 —
GET /api/notes/:noteId/pagesをauthRequiredからauthOptional+getNoteRoleに切り替え、公開 / unlisted ノートの guestでも取得可能にする。同時に keyset cursor pagination
(
?cursor=&limit=&include=) を導入し、レスポンスを{ items, next_cursor }の新形状にする。content_preview/thumbnail_urlはinclude=preview/include=thumbnail時のみセットする。
Phase 2 — keyset pagination 用に
pages (note_id, updated_at DESC, id DESC) WHERE is_deleted = false部分複合インデックスを追加する。
(updated_at, id)の tie-break予測子も index 内で解決できるよう既存
idx_pages_note_active_updatedの隣に併設する。https://claude.ai/code/session_016XUBJqunaZPnLTdB4eFTzt
Summary by CodeRabbit
New Features
{ items, next_cursor }; preview and thumbnail fields are returned only when explicitly requested.Tests