refactor(front+server): 個人ページ（noteId === null）概念を根絶し Page.noteId を non-null 化 (#1020) - #1023
Conversation
…n-null 化 (#1020) - server: GET /api/sync/pages の各ページ行に note_id を追加し、トップレベルに default_note_id を返す（クライアントのレガシー null 行移行用） - front(sync): pull 適用前に reassignNullNotePages で既存の noteId:null 行を デフォルトノートへ付け替え。push はデフォルトノート配下のみに限定 - front(IndexedDB): PageMetadata.noteId を string に tighten。未移行の レガシー null 行は読み出しから除外し、同期時に自動移行 - front(types): Page.noteId / PageSummary.noteId を string（non-null）へ再 tighten し、#1011 で追加した消費側 null ガード・null 分岐を撤去 - ゲストのローカルページ作成を廃止（#889 以降 /notes/null/:pageId に遷移して 実質機能していなかった）。createPageLocal / pageStore を削除し、FAB は 未サインイン時に非表示 - WikiLink ホバープレビューを pageStore からリポジトリ（IndexedDB）ベースの usePageByTitle / useGhostLinkReferenced に置き換え - 検索結果の「共有」バッジは noteId の有無ではなく結果ソースで判定 - 呼び出し元のない importPersonalPageFromApi（note_id != null を拒否する ガード）を削除 [skip drizzle-check] サーバはルート実装のみの変更で、スキーマ変更なし Closes #1020 https://claude.ai/code/session_01FH8KekbbgxZxsw5LLknKCQ
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
![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 eradicates the legacy "personal page" concept (where noteId was null) by ensuring all pages belong to a note (Issue #1020). It retires the guest-local pageStore, updates the storage adapter and sync engine to migrate legacy rows to the default note, and restricts page creation to signed-in users. The review feedback highlights several key improvements: optimizing performance by avoiding redundant IndexedDB queries when page titles are empty in WikiLinkHoverCardLayer.tsx and AIChatWikiLink.tsx, caching the legacy migration in syncWithApi.ts using a localStorage flag to prevent scanning the database on every sync pull, and resolving a potential UI glitch in NotePageView.tsx by aligning isTitleEditable with canEdit.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| ? Boolean(access?.canEdit) | ||
| : canEdit && canEditTitle(userId ?? undefined, page); | ||
| const canEdit = canEditPage(access ?? undefined, page); | ||
| const isTitleEditable = Boolean(access?.canEdit); |
There was a problem hiding this comment.
If page is not yet loaded (i.e., null or undefined during loading states), isTitleEditable will evaluate to true if access?.canEdit is true. This can lead to UI rendering glitches or console errors when trying to edit a non-existent page title. Since canEdit already encapsulates both the access.canEdit check and the safety check for page existence, isTitleEditable should simply be assigned to canEdit.
| const isTitleEditable = Boolean(access?.canEdit); | |
| const isTitleEditable = canEdit; |
There was a problem hiding this comment.
ee0b8e2 で修正しました。isTitleEditable = canEdit に揃え、canEdit（= canView + page 存在 + canEdit）を単一の判定源にしています。
Fixed in ee0b8e2 — isTitleEditable now aliases canEdit, which already encapsulates the canView / page existence checks.
Generated by Claude Code
| // Issue #1020: adopt legacy `noteId: null` rows into the default note | ||
| // before applying the pull. Incremental pulls never resend old rows, so | ||
| // skipping this would leave legacy rows permanently unreadable. | ||
| await adapter.reassignNullNotePages(res.default_note_id); |
There was a problem hiding this comment.
Calling reassignNullNotePages on every single sync pull triggers a full scan of the my_pages object store via an IndexedDB cursor. As the number of pages grows, this will become a significant performance bottleneck. Since this migration only needs to run once per user database, consider wrapping it in a one-time check using a localStorage flag keyed by the default_note_id.
const migrationKey = `migration_null_note_pages_done_${res.default_note_id}`;
if (!localStorage.getItem(migrationKey)) {
await adapter.reassignNullNotePages(res.default_note_id);
localStorage.setItem(migrationKey, "true");
}There was a problem hiding this comment.
問題意識は正しいですが、localStorage フラグは採用せず ee0b8e2 で代替実装にしました。
- localStorage はユーザー（IndexedDB は
zedi-storage-{userId}単位）と分離されておらず、別ストレージにフラグを持つと「DB は消えたがフラグだけ残る」（resetDatabase後など）といった分裂が起きます。 - 代わりに IndexedDB の仕様（
by_noteindex は noteId が null / 欠落の行を含まない）を利用し、store.count()とindex.count()の比較でレガシー行の有無を O(log n) で判定します。一致すればカーソル走査をスキップ。フラグ方式と違い、ロールアウト中の旧タブが null 行を書き込んでも次回同期で自己修復します。
Valid concern, but addressed with an alternative in ee0b8e2: instead of a localStorage flag (which lives outside the per-user IndexedDB and can desync after resetDatabase), reassignNullNotePages now compares store.count() vs the by_note index count — legacy rows are exactly the ones missing from the index — and skips the cursor scan when they match. Steady-state cost is two O(log n) counts per sync, and it self-heals if an old tab writes a null row during rollout.
Generated by Claude Code
| // （IndexedDB）ベースのクエリでページ解決とゴースト参照判定を行う。 | ||
| // The legacy guest store (`pageStore`) was retired by issue #1020; resolve | ||
| // the page and the ghost-reference state via the repository (IndexedDB). | ||
| const { data: resolvedPage } = usePageByTitle(target?.title ?? ""); |
There was a problem hiding this comment.
When target is undefined (e.g., when the hover card is not active), target?.title evaluates to undefined, defaulting to "". This will trigger an unnecessary IndexedDB query for an empty title on every render. To optimize performance, consider passing an enabled option to usePageByTitle so the query is skipped when the title is empty.
| const { data: resolvedPage } = usePageByTitle(target?.title ?? ""); | |
| const { data: resolvedPage } = usePageByTitle(target?.title ?? "", { enabled: !!target?.title }); |
There was a problem hiding this comment.
対応不要と判断しました。usePageByTitle は内部で enabled: isLoaded && title.trim().length > 0 をガードしており、空タイトルではクエリは発火しません（また、このフックは options 引数を受け取らないため提案のシグネチャはコンパイルできません）。useGhostLinkReferenced 側も同様に normalized.length > 0 でガード済みです。
No change needed: usePageByTitle already guards internally with enabled: isLoaded && title.trim().length > 0, so an empty title never triggers an IndexedDB query (and the hook takes no options parameter, so the suggested signature would not compile). useGhostLinkReferenced has the same internal guard.
Generated by Claude Code
| // （IndexedDB）ベースのクエリでページ解決とゴースト参照判定を行う。 | ||
| // The legacy guest store (`pageStore`) was retired by issue #1020; resolve | ||
| // the page and the ghost-reference state via the repository (IndexedDB). | ||
| const { data: resolvedPage } = usePageByTitle(normalizedTitle); |
There was a problem hiding this comment.
If normalizedTitle is empty, calling usePageByTitle will trigger an unnecessary IndexedDB query. To prevent redundant database access, consider passing an enabled option to skip the query when the title is empty.
| const { data: resolvedPage } = usePageByTitle(normalizedTitle); | |
| const { data: resolvedPage } = usePageByTitle(normalizedTitle, { enabled: !!normalizedTitle }); |
There was a problem hiding this comment.
対応不要と判断しました。usePageByTitle は内部で enabled: isLoaded && title.trim().length > 0 をガードしており、空タイトルでクエリは発火しません（options 引数も受け取らないため提案のシグネチャはコンパイルできません）。
No change needed: usePageByTitle already has the internal guard enabled: isLoaded && title.trim().length > 0, so an empty title never hits IndexedDB (and the hook takes no options parameter).
Generated by Claude Code
|
Warning Review limit reached
More reviews will be available in 27 minutes and 6 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (22)
📝 WalkthroughWalkthroughThis PR retires the legacy "personal page" concept (noteId===null) by ensuring all pages belong to exactly one note. The server now provides default_note_id and per-row note_id, the storage layer gains migration logic for legacy null-noteId rows, sync orchestrates the migration, and all component/hook interactions eliminate nullable noteId checks. ChangesPersonal Page Concept Retirement and noteId Non-Nullification
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 unit tests (beta)
Comment |
- NotePageView: isTitleEditable を canEdit に揃え、page 未ロード時の 編集可能判定を排除 - IndexedDBStorageAdapter.reassignNullNotePages: store.count() と by_note index の count 比較によるレガシー行ゼロ時の高速パスを追加し、毎同期の 全行カーソル走査を回避（localStorage フラグ案はユーザー間の漏れと ストレージ分裂があるため不採用） https://claude.ai/code/session_01FH8KekbbgxZxsw5LLknKCQ
![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
🧹 Nitpick comments (1)
src/lib/sync/syncWithApi.test.ts (1)
686-724: 💤 Low valueConsider making the call ordering assertion more explicit.
The test verifies that
reassignhappens first viaexpect(callOrder[0]).toBe("reassign"), but doesn't explicitly assert thatupsertalso occurred. While the test setup will trigger both calls, adding an explicit check would make the test's intent clearer and more robust against future refactoring.📝 Optional improvement
expect(adapter.reassignNullNotePages).toHaveBeenCalledWith(DEFAULT_NOTE_ID); expect(callOrder[0]).toBe("reassign"); + expect(callOrder).toContain("upsert"); + expect(callOrder.indexOf("reassign")).toBeLessThan(callOrder.indexOf("upsert"));🤖 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/lib/sync/syncWithApi.test.ts` around lines 686 - 724, The test currently only asserts that "reassign" occurred first via callOrder[0]; update the test around syncWithApi(adapter, api, TEST_USER_ID) to also explicitly assert that the adapter.upsertPage path was invoked and that callOrder shows the expected sequence (e.g., callOrder[1] === "upsert" and/or expect(adapter.upsertPage).toHaveBeenCalled()). Reference the existing symbols adapter.reassignNullNotePages, adapter.upsertPage, callOrder, and syncWithApi to locate and update the assertions so the test verifies both invocation and ordering explicitly.
🤖 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/components/layout/FloatingActionButton.tsx`:
- Around line 81-87: Before returning null when not signed in, explicitly reset
the transient FAB/dialog state so it can't persist across sign-in transitions:
call the state setters to close menu/dialogs and clear seeded clip URL (e.g.
setIsMenuOpen(false), setIsImageDialogOpen(false), setInitialClipUrl(undefined
or ''), and ensure isWebClipperOpen is cleared via setIsWebClipperOpen(false))
before the early return that checks isSignedIn.
In `@src/pages/NotePageView.tsx`:
- Around line 862-863: The inline spec/comment claiming only the page owner can
rename is outdated; update the JSDoc/TSDoc near the title-related prop (the doc
referenced around Line 136) to reflect the new permission model where
isTitleEditable is derived from access?.canEdit (and canEdit =
canEditPage(access ?? undefined, page)), i.e. title edits are permitted whenever
access.canEdit is true. Locate references to isTitleEditable, canEditPage, and
the title/rename prop doc in NotePageView.tsx and replace the owner-only wording
with a concise statement that title renaming is allowed when access.canEdit is
true.
---
Nitpick comments:
In `@src/lib/sync/syncWithApi.test.ts`:
- Around line 686-724: The test currently only asserts that "reassign" occurred
first via callOrder[0]; update the test around syncWithApi(adapter, api,
TEST_USER_ID) to also explicitly assert that the adapter.upsertPage path was
invoked and that callOrder shows the expected sequence (e.g., callOrder[1] ===
"upsert" and/or expect(adapter.upsertPage).toHaveBeenCalled()). Reference the
existing symbols adapter.reassignNullNotePages, adapter.upsertPage, callOrder,
and syncWithApi to locate and update the assertions so the test verifies both
invocation and ordering explicitly.
🪄 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: a2e52e33-818c-464e-92f6-0d701fea9df8
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (40)
knip.jsonpackage.jsonserver/api/src/__tests__/routes/syncPages.test.tsserver/api/src/routes/syncPages.tssrc/components/aiChat/AIChatWikiLink.tsxsrc/components/aiChat/PromoteToWikiDialog.tsxsrc/components/editor/TiptapEditor/WikiLinkHoverCardLayer.tsxsrc/components/editor/TiptapEditor/useWikiLinkNavigation.tssrc/components/layout/FloatingActionButton.tsxsrc/components/note/NotePagePublicView.tsxsrc/components/page/LinkGroupRow.tsxsrc/components/page/LinkSection.tsxsrc/components/page/LinkedPagesSection.tsxsrc/components/search/SearchResultCard.tsxsrc/components/wikiLink/WikiLinkPreviewContent.test.tsxsrc/hooks/useGlobalSearch.tssrc/hooks/useLinkedPages.tssrc/hooks/usePageQueries.tssrc/hooks/useSyncWikiLinks.test.tssrc/hooks/useWikiLinkCandidates.test.tssrc/hooks/useWikiLinkCandidates.tssrc/lib/api/types.tssrc/lib/dateUtils.test.tssrc/lib/pageRepository.test.tssrc/lib/pageRepository/StorageAdapterPageRepository.test.tssrc/lib/pageRepository/StorageAdapterPageRepository.tssrc/lib/searchUtils.test.tssrc/lib/storageAdapter/IndexedDBStorageAdapter.test.tssrc/lib/storageAdapter/IndexedDBStorageAdapter.tssrc/lib/storageAdapter/StorageAdapter.tssrc/lib/storageAdapter/types.tssrc/lib/sync/syncWithApi.test.tssrc/lib/sync/syncWithApi.tssrc/lib/syncWikiLinks.tssrc/pages/NotePageView.test.tsxsrc/pages/NotePageView.tsxsrc/pages/SearchResults.tsxsrc/stores/pageStore.test.tssrc/stores/pageStore.tssrc/types/page.ts
💤 Files with no reviewable changes (5)
- src/stores/pageStore.test.ts
- src/stores/pageStore.ts
- package.json
- knip.json
- src/pages/NotePageView.test.tsx
- FloatingActionButton: 未サインイン遷移時に FAB のローカル UI 状態 （メニュー / 画像ダイアログ）も畳む。initialClipUrl はサインイン往復 （#826 handoff）を壊すため破棄しない - NotePageView: isTitleEditable の prop ドキュメントを新権限モデル （access.canEdit ベース）に更新 - syncWithApi.test: reassign → upsert の呼び出し順アサーションを明示化 https://claude.ai/code/session_01FH8KekbbgxZxsw5LLknKCQ
2 participants
default_note_id を返す（クライアントのレガシー null 行移行用）
デフォルトノートへ付け替え。push はデフォルトノート配下のみに限定
レガシー null 行は読み出しから除外し、同期時に自動移行
し、refactor(front): src/ の TypeScript strict 化（既存 tsc エラー解消 → CI typecheck 追加 → strict 有効化） #1011 で追加した消費側 null ガード・null 分岐を撤去
localY.js mode and route all pages through Hocuspocus #889 以降 /notes/null/:pageId に遷移して実質機能していなかった）。createPageLocal / pageStore を削除し、FAB は
未サインイン時に非表示
usePageByTitle / useGhostLinkReferenced に置き換え
ガード）を削除
[skip drizzle-check] サーバはルート実装のみの変更で、スキーマ変更なし
Closes #1020
https://claude.ai/code/session_01FH8KekbbgxZxsw5LLknKCQ
Summary by CodeRabbit
Refactor
Chores
uuidand@types/uuiddependencies.