chore(notes): drop pages[] from note shell; add /page-titles + /pages/:id (#860 Phase 6)#868
Merged
Merged
Conversation
…/:id (#860 Phase 6) Removes the `pages[]` slice from `GET /api/notes/:noteId` (note shell). Visible page lists already use the Phase 1 cursor-paginated `/pages` endpoint, but wiki-link / AI-chat scope / add-dialog dedup need the *complete* title set, so this PR adds two replacement routes instead of the shell array: - `GET /api/notes/:noteId/page-titles` — minimal `{ id, title, is_deleted, updated_at }` payload for full-set title consumers (~50 bytes/page; small even for 10k-page notes). `authOptional` + `getNoteRole`, with weak ETag mixing version + role + `(MAX(updated_at), COUNT(*))` so renames / deletes still bust 304s. - `GET /api/pages/:pageId` — single-page metadata fetch needed by `useNotePage` after losing the shell `pages[]`. Same `authOptional` + `getNoteRole` model. Note shell `RESPONSE_VERSION` bumped `v2` → `v3` so old `If-None-Match` validators do not revive stale `pages[]` bodies via 304. Frontend: - Replace `useNotePages` with the new `useNoteTitleIndex` (`{ id, title, isDeleted, updatedAt }[]`) and migrate every consumer (`useWikiLinkNavigation`, `useWikiLinkStatusSync`, `useTagStatusSync`, `useWikiLinkCandidates`, `useAIChatActions`). - Repoint `useNotePage` to the new `/api/pages/:id` route, with an explicit `noteId` mismatch guard so a stale URL after a cross-note move resolves to `null` instead of rendering a mismatched page. - Extend the SSE handler (`useNotePageEvents`) to mirror window-cache patches into the title-index cache: `page.added` prepends, `page.updated` move-to-head with title overwrite, `page.deleted` removes. `ready` and `note.permission_changed` invalidate the title-index too. Tests: 9 new title-index endpoint tests, 4 new single-page metadata tests, title-index SSE patch coverage, and updated note-shell tests that now lock in the "no `pages[]`" contract and the `v2 → v3` cache-bust path.
Adds first-class note-scoped search to the note detail page and turns the existing `/api/notes/:noteId/search` endpoint into a keyset-paginated route that public / unlisted guests can also use, completing the last leftover Phase 5 items of the issue #860 epic. Backend (`/api/notes/:noteId/search`): - Auth model switches from `authRequired` to `authOptional` + `getNoteRole`, mirroring `/pages` and `/page-titles`. Public / unlisted notes are searchable by guests; private notes still 403 without a resolved role. - Adds `?cursor=` keyset pagination on `(updated_at, id)` using the same microsecond-precision ISO string the `/pages` window already uses (Phase 1) — pg `to_char(...)` round-trips through the cursor and casts back via `::timestamptz`, so consecutive `defaultNow()` rows never get dropped. Response shape is now `{ results, next_cursor }`; the route fetches `limit + 1` rows to detect tails without an extra count query. - Cursors are opaque base64url JSON and validated end-to-end (UUID + ISO timestamp), so malformed values become deterministic 400s instead of pg `22P02` 500s. Frontend: - New `useInfiniteNoteSearch(noteId, q, opts)` hook (built on `useInfiniteQuery`) flattens windows into a `NoteSearchResultItem[]` while exposing `hasNextPage` / `fetchNextPage` / `isFetchingNextPage`. Query key namespaces the cache per `(noteId, q, pageSize, principal)`. - New `<NoteSearchBar />` component rendered above `<PageGrid />` on the note detail page. Debounced input (250 ms default; configurable for tests), results render as a click-to-navigate list with optional `content_preview` snippet, and a "show more" button drives `fetchNextPage()` when `hasNextPage` is true. Empty queries render nothing so the regular grid layout stays in place. New i18n keys live under `notes.search.*` in both ja and en. - `NoteAddPageDialog` finally retires its old all-pages dedup TODO: it now reads the Phase 6 `useNoteTitleIndex` and surfaces an inline warning ("a page with this title already exists") on exact-match collisions, without blocking the add action. Tests: 6 new backend tests cover cursor emission, exhaustion semantics, malformed/UUID-less cursor rejection, the keyset SQL predicate, and the 401 → 403 auth-model change. Existing limit-clamp tests adjusted for the `limit + 1` fetch (20 → 21, 100 → 101). NoteView test mock extended with the new hooks so the search bar and add-dialog title-index render cleanly.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
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 (6)
🚧 Files skipped from review as they are similar to previous changes (6)
📝 WalkthroughWalkthroughPhase 6 splits page data from the note shell: removes ChangesPhase 6 API and Query Restructuring
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 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 Phase 5 and 6 of Issue #860 by removing the pages[] array from the note shell response and introducing a lightweight page-titles endpoint for full-set title lookups. It also adds keyset cursor pagination to note-scoped search, makes search accessible to guests on public notes, and introduces a NoteSearchBar component. The NoteAddPageDialog now utilizes the new title index for duplicate title warnings. Review feedback highlighted the need to improve accessibility in the search bar by replacing incorrect ARIA listbox roles with standard list semantics for navigation links.
Comment on lines
+125
to
+126
| role="listbox" | ||
| aria-label={t("notes.search.resultsLabel")} |
There was a problem hiding this comment.
Using role="listbox" for a list of navigation links is semantically incorrect. A listbox is intended for selecting values (like in a form), whereas these are search results that navigate to different routes. This can confuse screen reader users who expect selection behavior. It is better to use standard list semantics for navigation results. Consider removing the role and moving the aria-label to the ul element.
Suggested change
| role="listbox" | |
| aria-label={t("notes.search.resultsLabel")} | |
| aria-label={t("notes.search.resultsLabel")} |
| {!error && results.length > 0 && ( | ||
| <ul className="divide-border divide-y"> | ||
| {results.map((row, idx) => ( | ||
| <li key={ids[idx]} role="option"> |
There was a problem hiding this comment.
Interactive elements like Link should not be nested inside an option role. Removing role="option" ensures the list items are treated as standard list items, which is more appropriate for a list of navigation links and avoids violating ARIA patterns.
Suggested change
| <li key={ids[idx]} role="option"> | |
| <li key={ids[idx]}> |
) Drops `role="listbox"` / `role="option"` from the note-scoped search bar's result container. The dropdown is a list of navigation links, not a select-like selection widget, so the ARIA roles were misleading screen readers into expecting selection behavior. Moves `aria-label` onto the real `<ul>` and leaves the outer `<div>` as a role-less styling container (it still hosts the loading / empty / load-more affordances around the list, so it cannot itself be the `<ul>`). Addresses gemini-code-assist's medium-priority comments on PR #868 (NoteSearchBar.tsx:126 and :148).
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
server/api/src/routes/notes/search.ts (1)
176-215:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep
content_textout of the public search payload.This query now selects
pc.content_textand returns it verbatim in every hit. On long pages,limit=20/100turns note search into a full-body download path and can balloon each keystroke or pagination fetch by megabytes. If this field is only reserved for future snippet work, keep it server-side until there is an explicit opt-in consumer.🤖 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 `@server/api/src/routes/notes/search.ts` around lines 176 - 215, The query is returning pc.content_text and exposing it in the public results; remove that by stopping selection of pc.content_text in the SQL, deleting the content_text property from the SearchRow type, and ensuring the public payload from results (the visible.map code) does not include content_text; specifically, remove "pc.content_text" from the SELECT, remove content_text: string | null from the SearchRow type, and verify the results mapping (currently destructuring updated_at_iso out) will no longer carry content_text to the wire (so nextCursor/encodeSearchCursor logic remains unchanged).src/hooks/useNoteQueries.ts (1)
970-999:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAllow guests to fetch public note pages.
GET /api/pages/:pageIdnow followsauthOptional + getNoteRole, but this hook still short-circuits onisSignedIn. Signed-out viewers on public/unlisted notes never issue the request, souseNotePagestays empty on routes the server now allows.Suggested fix
export function useNotePage( noteId: string, pageId: string, _source?: "local" | "remote", enabled: boolean = true, ) { - const { api, isLoaded, isSignedIn } = useNoteApi(); + const { api, isLoaded } = useNoteApi(); return useQuery({ queryKey: noteKeys.page(noteId, pageId), queryFn: async (): Promise<Page | null> => { const p = (await api.getPage(pageId)) as ApiPageMetadataRow | null; @@ - enabled: enabled && isLoaded && isSignedIn && !!noteId && !!pageId, + enabled: enabled && isLoaded && !!noteId && !!pageId, }); }🤖 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 970 - 999, The hook useNotePage currently prevents unsigned users from fetching pages by including isSignedIn in the enabled condition; remove the isSignedIn gate so guests can call GET /api/pages/:pageId (which is now authOptional). Update the query's enabled flag in useNotePage to be enabled && isLoaded && !!noteId && !!pageId (leave other logic like the note_id check and the queryFn using api.getPage and apiPageToPage unchanged) so unsigned viewers can fetch public/unlisted pages while preserving the existing safety checks.
🤖 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/page-titles.test.ts`:
- Around line 156-168: The test name is misleading: the it(...) description
currently says "returns 401 when private note is accessed unauthenticated" but
the assertion checks for 403; update the test title string in the it(...) call
to "returns 403 when private note is accessed unauthenticated" (or equivalent)
so the description matches the asserted status code; ensure you only change the
test description and not the assertion or test logic in this test case.
In `@src/components/note/NoteSearchBar.tsx`:
- Around line 124-149: The component NoteSearchBar.tsx is using ARIA listbox
semantics (div role="listbox" and li role="option") for a simple navigable list
of links; remove or change these roles to plain list semantics by eliminating
role="listbox" on the wrapping div and role="option" on the li (or alternatively
use role="list" on the container and role="listitem" on each li) so the DOM
reflects a normal list of links and not a widget requiring listbox keyboard
behavior; update any tests or accessibility checks that expect the old roles.
In `@src/lib/api/types.ts`:
- Around line 336-355: The NoteSearchResultItem interface and its documentation
still treat note_id as nullable despite the server now hard-filtering p.note_id
= :noteId; update the interface and comment so note_id is non-nullable (change
note_id: string | null -> note_id: string) and adjust the JSDoc to state this
endpoint returns only note-scoped hits (remove the legacy linked-personal
explanation); search for NoteSearchResultItem to update any consumer typings or
tests that expect nullable note_id and fix unreachable branches accordingly.
In `@src/pages/NoteView/NoteViewAddPageDialogContent.tsx`:
- Around line 81-83: The aria-invalid attribute is incorrectly used to mark an
advisory duplicate warning as an input error; in NoteViewAddPageDialogContent
remove the aria-invalid={duplicateTitleExists || undefined} prop (or change it
to only truthy when you actually block submit) so the input isn’t announced as
invalid while still keeping aria-describedby={duplicateTitleExists ?
`${newPageTitleFieldId}-dup` : undefined} and the duplicateTitleExists logic for
the warning text.
---
Outside diff comments:
In `@server/api/src/routes/notes/search.ts`:
- Around line 176-215: The query is returning pc.content_text and exposing it in
the public results; remove that by stopping selection of pc.content_text in the
SQL, deleting the content_text property from the SearchRow type, and ensuring
the public payload from results (the visible.map code) does not include
content_text; specifically, remove "pc.content_text" from the SELECT, remove
content_text: string | null from the SearchRow type, and verify the results
mapping (currently destructuring updated_at_iso out) will no longer carry
content_text to the wire (so nextCursor/encodeSearchCursor logic remains
unchanged).
In `@src/hooks/useNoteQueries.ts`:
- Around line 970-999: The hook useNotePage currently prevents unsigned users
from fetching pages by including isSignedIn in the enabled condition; remove the
isSignedIn gate so guests can call GET /api/pages/:pageId (which is now
authOptional). Update the query's enabled flag in useNotePage to be enabled &&
isLoaded && !!noteId && !!pageId (leave other logic like the note_id check and
the queryFn using api.getPage and apiPageToPage unchanged) so unsigned viewers
can fetch public/unlisted pages while preserving the existing safety checks.
🪄 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: 22c5f30e-4df1-4009-b8e7-239b5e82b6bb
📒 Files selected for processing (31)
server/api/src/__tests__/routes/notes/crud.test.tsserver/api/src/__tests__/routes/notes/page-titles.test.tsserver/api/src/__tests__/routes/notes/search.test.tsserver/api/src/__tests__/routes/pages.test.tsserver/api/src/routes/notes/crud.tsserver/api/src/routes/notes/index.tsserver/api/src/routes/notes/search.tsserver/api/src/routes/notes/titleIndex.tsserver/api/src/routes/notes/types.tsserver/api/src/routes/pages.tssrc/components/editor/TiptapEditor/useTagStatusSync.test.tsxsrc/components/editor/TiptapEditor/useTagStatusSync.tssrc/components/editor/TiptapEditor/useWikiLinkNavigation.test.tssrc/components/editor/TiptapEditor/useWikiLinkNavigation.tssrc/components/editor/TiptapEditor/useWikiLinkStatusSync.test.tsxsrc/components/editor/TiptapEditor/useWikiLinkStatusSync.tssrc/components/note/NoteSearchBar.tsxsrc/hooks/useAIChatActions.tssrc/hooks/useNotePageEvents.test.tssrc/hooks/useNotePageEvents.tssrc/hooks/useNoteQueries.tssrc/hooks/useWikiLinkCandidates.test.tssrc/hooks/useWikiLinkCandidates.tssrc/i18n/locales/en/notes.jsonsrc/i18n/locales/ja/notes.jsonsrc/lib/api/apiClient.tssrc/lib/api/types.tssrc/pages/NoteView/NoteAddPageDialog.tsxsrc/pages/NoteView/NoteView.test.tsxsrc/pages/NoteView/NoteViewAddPageDialogContent.tsxsrc/pages/NoteView/index.tsx
Five small fixes from CodeRabbit's review on PR #868: - `/api/notes/:noteId/search` no longer SELECTs `pc.content_text`. The join stays for the ILIKE-on-body match, but the full body is dropped from the wire — at limit=100 with long pages this could otherwise balloon each search response into multiple megabytes. The matching field is also removed from `SearchRow` and `NoteSearchResultItem`. - `NoteSearchResultItem.note_id` becomes non-nullable. The SQL hard- filters `p.note_id = :noteId`, so the legacy linked-personal (`null`) branch is unreachable here and the type was inviting unreachable consumer code. Stale mock rows that pretended to return `note_id: null` are dropped from the search tests, and a new assertion locks in the "no `content_text` on the wire" contract. - `useNotePage` drops the `isSignedIn` gate. `GET /api/pages/:pageId` has been `authOptional` + `getNoteRole` since Phase 6, so guests should be able to fetch pages on public / unlisted notes — the hook was slamming a door the server explicitly opens. - `NoteViewAddPageDialogContent`'s title input no longer sets `aria-invalid`. A duplicate title is an advisory warning that does not block submit, so announcing the field as invalid was a semantically wrong cue. `aria-describedby` and the warning text stay intact. - Renames the misleading "returns 401 when private note is accessed unauthenticated" page-titles test to "returns 403" so the description matches the actual assertion (auth model is `authOptional` + `getNoteRole`, which 403s on no-role).
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.
Removes the
pages[]slice fromGET /api/notes/:noteId(note shell). Visiblepage lists already use the Phase 1 cursor-paginated
/pagesendpoint, butwiki-link / AI-chat scope / add-dialog dedup need the complete title set, so
this PR adds two replacement routes instead of the shell array:
GET /api/notes/:noteId/page-titles— minimal{ id, title, is_deleted, updated_at }payload for full-set title consumers (~50 bytes/page; smalleven for 10k-page notes).
authOptional+getNoteRole, with weak ETagmixing version + role +
(MAX(updated_at), COUNT(*))so renames / deletesstill bust 304s.
GET /api/pages/:pageId— single-page metadata fetch needed byuseNotePageafter losing the shellpages[]. SameauthOptional+getNoteRolemodel.Note shell
RESPONSE_VERSIONbumpedv2→v3so oldIf-None-Matchvalidators do not revive stale
pages[]bodies via 304.Frontend:
useNotePageswith the newuseNoteTitleIndex({ id, title, isDeleted, updatedAt }[]) and migrate every consumer(
useWikiLinkNavigation,useWikiLinkStatusSync,useTagStatusSync,useWikiLinkCandidates,useAIChatActions).useNotePageto the new/api/pages/:idroute, with an explicitnoteIdmismatch guard so a stale URL after a cross-note move resolves tonullinstead of rendering a mismatched page.useNotePageEvents) to mirror window-cachepatches into the title-index cache:
page.addedprepends,page.updatedmove-to-head with title overwrite,
page.deletedremoves.readyandnote.permission_changedinvalidate the title-index too.Tests: 9 new title-index endpoint tests, 4 new single-page metadata tests,
title-index SSE patch coverage, and updated note-shell tests that now lock
in the "no
pages[]" contract and thev2 → v3cache-bust path.Summary by CodeRabbit
New Features
Improvements