refactor: consolidate page editor into note-centric architecture (Issue #889 Phase 3)#894
Merged
Merged
Conversation
…se 3) Issue #889 段階的リファクタの Phase 3。Phase 1 (#888) でメタデータ専用 `PUT /api/pages/:id` + 読み取り専用 GET ルートを、Phase 2 (#893) で `NotePagePublicView` を整備した上で、残った大本命のクリーンアップとして 以下を完全に廃止する。 - `CollaborationManager` の `local` モード（IndexedDB 同期と並行に `GET/PUT /api/pages/:id/content` を debounce で叩いて Y.Doc を REST 保存する経路）を撤去。全ページは所属ノートを持つので、Hocuspocus WebSocket 同期に統一する。 - `/pages/:id` および `/page/:id` ルートを撤去。ノートネイティブ経路 `/notes/:noteId/:pageId` に統合し、`useCreatePage` の戻り値が常に `noteId` を持つことを前提に 16 箇所の `navigate(...)` を書き換えた。 - `useCollaboration` API から `mode` / `flushSave` / `setPageTitle` を撤去 し、`PageEditor/` 配下の重複コンポーネント・フック 22 ファイルを削除。 - Web Clipper / AI チャット / PromoteToWiki / WikiLink dialog などの 作成フローは `navigate("/notes/:noteId/:pageId", { state: { initialContent } })` 経由で `NotePageView` に seed を渡し、Hocuspocus `synced` 後に Y.Doc に 反映する形式に切り替えた。 - サーバ側は `POST /api/pages` レスポンスに `note_id` を追加。PDF 派生 ページ・ハイライト一覧・グローバル検索の各レスポンスに `note_id` を 同梱して、クライアントが `/notes/:noteId/:pageId` を組み立てられるように した。`GET/PUT /api/pages/:id/content` 本体や `snapshotService.ts` の 削除は Phase 4 に温存（移行期セーフネット）。 - Phase 3 で `/pages/:id` 専用の `e2e/page-editor.spec.ts` と `e2e/wikilink-create-dialog.spec.ts` を削除し、`e2e/auth-mock.ts` の `createNewPage` helper を `/notes/:noteId/:pageId` URL を待つように更新。 Issue #889 phase 3 — retires the legacy `local` Y.js REST path and the top-level `/pages/:id` route. Every page now syncs through Hocuspocus and navigates under its owning note. `useCollaboration` loses `mode` / `flushSave` / `setPageTitle`; create flows pass an `initialContent` seed via React Router state and the editor applies it after the initial sync. The server's `POST /api/pages` response, the derive-page handler, the highlight list, and the global search rows now all carry the derived page's `note_id` so the client never has to ask twice. Phase 4 will delete the now-orphaned `/api/pages/:id/content` endpoints. Test plan: - `bun run lint` — 0 errors (621 pre-existing warnings) - `bun run format:check` — clean - `bunx vitest run` (main) — 229 files / 2327 tests pass - `cd server/api && bunx vitest run` — 95 files / 1280 tests pass Refs: Issue #892, Phase 1 (#888), Phase 2 (#893) https://claude.ai/code/session_01CVtupQrUS23UEerQJgPgLH
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
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? |
|
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 (4)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughStandardize routing and data to /notes/:noteId/:pageId across client and server, add note identifiers to API responses and highlight/search rows, remove legacy /pages/:id routes and PageEditor stack, retire local REST collab sync and local collab mode, and update navigation, hooks, types, fixtures, and tests accordingly. ChangesIssue
Estimated code review effort 🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
![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: 6307f99266
ℹ️ 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".
| @@ -258,10 +247,6 @@ const App = () => ( | |||
| ページのリンクが 404 にならないよう URL を確保する。 */} | |||
| <Route path="/notes/official-guide" element={<OfficialGuidePlaceholder />} /> | |||
| <Route path="/notes" element={<Notes />} /> | |||
There was a problem hiding this comment.
Removing the /pages/:id and /page/:id routes here creates immediate 404s because the repo still has active emitters that generate those URLs (for example AIChatWikiLink uses to="/pages/${page.id}", onboarding completion navigates to /pages/${welcome_page_id}, and the index page links to /pages/${data.pageId}). Any user hitting those existing entry points now lands on NotFound, so compatibility routing (or a migration of all emitters in the same change) is still required.
Useful? React with 👍 / 👎.
Comment on lines
+742
to
+744
| const [pendingInitialContent, setPendingInitialContent] = useState<string | undefined>( | ||
| locationStateInitialContent, | ||
| ); |
There was a problem hiding this comment.
pendingInitialContent is initialized from location.state only once, so when the user is already on NotePageView and navigates to another /notes/:noteId/:pageId with { state: { initialContent } } (e.g. clip/image create flows), the parent component stays mounted and this state is not rehydrated for the new page. That drops the seed content and opens a blank editor despite the navigation state carrying initial content.
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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/hooks/useCollaboration.ts (1)
65-117:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winGuard the queued state updates against stale effects.
Both
queueMicrotaskcalls can outlive this effect’s cleanup. In Strict Mode, or during a fastenabled/pageIdflip, an older effect can still publishdisconnectedor a destroyed manager snapshot after a newer manager is already active.Suggested fix
useEffect(() => { + let cancelled = false; + if (!enabled || !pageId) { queueMicrotask(() => { + if (cancelled) return; setState({ ...initialState, status: "disconnected" }); setManagerSnapshot(emptyManagerSnapshot); }); - return; + return () => { + cancelled = true; + }; } const userName = isSignedIn && user ? user.fullName || user.firstName || "Anonymous" : "Guest"; const manager = new CollaborationManager(pageId, effectiveUserId, userName, async () => { @@ managerRef.current = manager; - queueMicrotask(() => - setManagerSnapshot({ - ydoc: manager.document, - xmlFragment: manager.xmlFragment, - awareness: manager.getAwareness() ?? undefined, - }), - ); + queueMicrotask(() => { + if (cancelled) return; + setManagerSnapshot({ + ydoc: manager.document, + xmlFragment: manager.xmlFragment, + awareness: manager.getAwareness() ?? undefined, + }); + }); const unsubscribe = manager.subscribe((newState) => { + if (cancelled) return; setState(newState); const nextAwareness = manager.getAwareness() ?? undefined; setManagerSnapshot((prev) => @@ return () => { + cancelled = true; unsubscribe(); manager.destroy(); managerRef.current = null; setManagerSnapshot(emptyManagerSnapshot); }; }, [pageId, effectiveUserId, enabled, getToken, isSignedIn, user?.fullName, user?.firstName]);🤖 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/useCollaboration.ts` around lines 65 - 117, The queued microtask callbacks and the subscription callback can run after this effect has cleaned up; capture an "active" flag or the specific manager instance and guard updates: create a local let active = true (or const thisManager = manager) at the top of the effect, set active = false in the returned cleanup, then in both queueMicrotask callbacks and inside the manager.subscribe handler check that active is still true (or managerRef.current === thisManager) before calling setState, setManagerSnapshot, or any other updates; also ensure the early-return branch that queues the disconnected state checks the same active flag before calling setState so stale effects cannot overwrite a newer manager snapshot.
🧹 Nitpick comments (7)
src/pages/NotePageView.tsx (1)
738-741: 💤 Low valueRedundant nullish coalescing after type guard.
The
?? undefinedon line 740 is unnecessary since thetypeof ... === "string"check guaranteesinitialContentis a string (never null/undefined).♻️ Suggested simplification
const locationStateInitialContent = typeof (location.state as { initialContent?: unknown } | null)?.initialContent === "string" - ? ((location.state as { initialContent: string }).initialContent ?? undefined) + ? (location.state as { initialContent: string }).initialContent : undefined;🤖 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/pages/NotePageView.tsx` around lines 738 - 741, The variable locationStateInitialContent uses a redundant nullish coalescing (?? undefined) after a type guard; remove the `?? undefined` and simply return the casted `initialContent` when `typeof (location.state as { initialContent?: unknown } | null)?.initialContent === "string"` is true so `locationStateInitialContent` becomes that string value directly (keep the existing cast to `{ initialContent: string }` to satisfy the type assertion), eliminating the unnecessary fallback.src/hooks/runAIChatAction.test.ts (1)
50-50: ⚡ Quick winAdd a navigation guard test for missing
noteId.Now that routing requires both params, add a case where create returns
{ id }withoutnoteIdand assertnavigateis not called.As per coding guidelines, "Tests serve as a source of truth for specifications alongside implementation code TSDoc/JSDoc".
Also applies to: 67-67, 100-100, 132-132
🤖 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/runAIChatAction.test.ts` at line 50, Add a test case in src/hooks/runAIChatAction.test.ts that covers the navigation guard when createPageMutateAsync returns an object missing noteId: change or add a mock for createPageMutateAsync (the vi.fn used in the tests) to mockResolvedValue({ id: "new-page-1" }) and assert that navigate was not called (expect(navigate).not.toHaveBeenCalled()); replicate the same missing-noteId assertion in the other affected test blocks referenced around the existing createPageMutateAsync mocks (the instances near the current mocks at the spots corresponding to lines 67, 100, and 132) so each scenario verifies navigation is skipped when noteId is absent.src/hooks/useLinkedPages.test.ts (1)
12-12: ⚡ Quick winAdd an explicit
noteIdassertion in mapper tests.You updated fixtures for the new contract, but there’s still no assertion that
pageToCard(and summary fallback paths) actually preservenoteId. Please add a direct expectation to lock this behavior.As per coding guidelines, "Tests serve as a source of truth for specifications alongside implementation code TSDoc/JSDoc".
Also applies to: 514-514
🤖 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/useLinkedPages.test.ts` at line 12, Add an explicit assertion checking that the noteId from the fixture is preserved by the mapping logic: in the test that uses the updated fixture (the one passing noteId: "test-note"), assert that pageToCard(...) returns an object whose noteId equals the fixture's noteId; do the same for the summary-fallback path (the code path that generates a card from a summary) so both pageToCard and the summary fallback have direct expectations validating noteId preservation.src/hooks/useAIChatActions.test.ts (1)
82-82: ⚡ Quick winAdd a missing-
noteIdcreate-page test.Please add a case where mutation resolves with
idbut withoutnoteId, and assert navigation is skipped (or failure toast shown), to prevent/notes/undefined/...regressions.As per coding guidelines, "Tests serve as a source of truth for specifications alongside implementation code TSDoc/JSDoc".
Also applies to: 113-113
🤖 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/useAIChatActions.test.ts` at line 82, Add a test in useAIChatActions.test.ts that covers the mutation resolving with an id but no noteId: set mockCreatePageMutateAsync.mockResolvedValue({ id: "new-page-id" }) and then call the action that triggers createPage; assert that the navigation mock (e.g., mockNavigate or router.push) is NOT called and/or the failure toast mock is called, ensuring we don't navigate to /notes/undefined/...; locate helpers by name (mockCreatePageMutateAsync, useAIChatActions, mockNavigate/mockToast) and mirror the existing successful-create test structure to keep setup/teardown consistent.src/components/pdf-reader/saveAndDeriveFlow.test.ts (1)
31-137: ⚡ Quick winAdd a regression test for the
noteId-missing error branch.
runSaveAndDeriveFlownow hard-fails whenderivePageomitsnoteId, but this new guard path is not asserted yet.🧪 Proposed test case
describe("runSaveAndDeriveFlow", () => { + it("returns an error result and does not navigate when derivePage omits noteId", async () => { + const navigate = vi.fn(); + const createHighlight = vi.fn().mockResolvedValue({ highlight: makeHighlight() }); + const derivePage = vi.fn().mockResolvedValue({ + pageId: "page-without-note", + templateContent: '{"type":"doc","content":[]}', + }); + + const result = await runSaveAndDeriveFlow({ + sourceId: "src-1", + createBody: { + pdfPage: 2, + rects: [{ x1: 0, y1: 0, x2: 1, y2: 1 }], + text: "Long enough body text for the citation.", + }, + createHighlight, + derivePage, + navigate, + }); + + expect(navigate).not.toHaveBeenCalled(); + expect(result.status).toBe("error"); + if (result.status === "error") { + expect(result.error.message).toBe("derivePage response missing noteId"); + } + });🤖 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/components/pdf-reader/saveAndDeriveFlow.test.ts` around lines 31 - 137, Add a regression test that mocks derivePage to resolve with a response that includes pageId but omits noteId and assert runSaveAndDeriveFlow rejects; specifically, create mocks for createHighlight (returning makeHighlight()), derivePage.mockResolvedValue({ pageId: "page-missing-note" }), and navigate = vi.fn(), then await expect(runSaveAndDeriveFlow(...)).rejects.toThrow() (or rejects with the specific error if known) and assert navigate was not called and no successful result is returned; reference the runSaveAndDeriveFlow function and the derivePage/createHighlight/navigate mocks to locate where to add this case.src/hooks/useCollaboration.ts (1)
40-51: ⚡ Quick winMake the exported hook TSDoc bilingual.
This updated JSDoc only documents
useCollaborationin Japanese, but exported TypeScript APIs in this repo need both Japanese and English docs. As per coding guidelines, "Comments and documentation should include both Japanese and English" and "Add TSDoc/JSDoc to all exported functions, types, and interfaces".🤖 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/useCollaboration.ts` around lines 40 - 51, The exported hook useCollaboration currently has only Japanese TSDoc — update the comment block to include both Japanese and English descriptions, parameters, return values, and example(s) using TSDoc/JSDoc conventions so both languages appear (e.g., Japanese first then English or vice versa), and ensure tags like `@example`, `@param`, and `@returns` are present for the exported function signature; reference the useCollaboration hook name so the bilingual docs are colocated with the existing comment and follow the repository's documentation style guidelines.src/lib/collaboration/CollaborationManager.test.ts (1)
114-119: ⚡ Quick winAdd replacement coverage for the new startup contract.
This prose explains what was removed, but the suite no longer pins the new Phase 3 invariants:
HocuspocusProvidershould be created only after IndexedDB emitssynced, and a latesynced/token resolution must not connect afterdestroy(). Those are the behaviors most likely to regress in this refactor. As per coding guidelines, "Tests serve as a source of truth for specifications alongside implementation code TSDoc/JSDoc".🤖 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/collaboration/CollaborationManager.test.ts` around lines 114 - 119, Add tests in CollaborationManager.test.ts to assert the new Phase 3 startup contract: (1) verify HocuspocusProvider is instantiated only after the IndexedDB mock emits the 'synced' event (use the CollaborationManager startup path or the function that currently creates HocuspocusProvider), and (2) simulate a delayed 'synced' emission or token resolution, call destroy() before it resolves, and assert that no connection/instantiation of HocuspocusProvider occurs afterward. Reference the symbols HocuspocusProvider, 'synced' event emission on the IndexedDB mock, the token resolution/promise used in startup, and destroy() to locate where to add these assertions.
🤖 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/ai-chat/PromoteToWikiDialog.tsx`:
- Around line 263-265: The navigation call uses firstCreated.noteId without
checking it, which can produce an invalid URL; before calling
navigate(`/notes/${firstCreated.noteId}/${firstCreated.id}`, ...) inside
PromoteToWikiDialog, guard that firstCreated.noteId (and still ensure
firstCreated.id) and only perform note-scoped navigation when noteId is defined;
if noteId is missing, handle gracefully (e.g., navigate to a non-note route,
skip navigation, or surface an error) and preserve the existing state payload
(state: { pendingChatPageGeneration: pending }) when performing the valid
navigate call.
In `@src/hooks/runAIChatAction.ts`:
- Around line 61-66: The navigation call builds a route using result.id and
result.noteId but only checks id; update the logic around deps.navigate to
validate both result?.id and result?.noteId (and guard any other flow that
constructs `/notes/${result.noteId}/${result.id}` such as the similar block at
lines ~97-106) before calling deps.navigate, and if either is missing avoid
navigation (or handle the error path) while preserving the pending state passed
in state.pendingChatPageGeneration; locate the checks around result and the call
to deps.navigate to implement this guard.
In `@src/hooks/useAIChatActions.ts`:
- Around line 165-170: The navigate call in useAIChatActions.ts currently runs
when result?.id exists even if result.noteId is undefined, producing invalid
URLs; update the guard around navigate(`/notes/${result.noteId}/${result.id}`)
in the function where result is evaluated (the block containing the existing if
(result?.id) check) to require a valid noteId as well (e.g., check
result?.noteId or result.noteId != null) and only call navigate when both
result.id and result.noteId are present; optionally handle or log the missing
noteId case (do not call navigate) so invalid routes are never constructed.
In `@src/hooks/useCreateNewPage.ts`:
- Around line 48-62: The toast is misleading because
addPageToNoteMutation.mutateAsync failed but the page (newPage) was created;
change the error toast in the catch block to say the page was created but
attaching to the note failed (e.g., "ページは作成されましたが、ノートへの添付に失敗しました"), and after
showing the toast still navigate to the newly created page (use
navigate(`/notes/${newPage.noteId}/${newPage.id}`) as a fallback) so the user
can access the orphaned page; update the catch handling around
addPageToNoteMutation.mutateAsync, referencing
addPageToNoteMutation.mutateAsync, newPage, navigate, and toast.
---
Outside diff comments:
In `@src/hooks/useCollaboration.ts`:
- Around line 65-117: The queued microtask callbacks and the subscription
callback can run after this effect has cleaned up; capture an "active" flag or
the specific manager instance and guard updates: create a local let active =
true (or const thisManager = manager) at the top of the effect, set active =
false in the returned cleanup, then in both queueMicrotask callbacks and inside
the manager.subscribe handler check that active is still true (or
managerRef.current === thisManager) before calling setState, setManagerSnapshot,
or any other updates; also ensure the early-return branch that queues the
disconnected state checks the same active flag before calling setState so stale
effects cannot overwrite a newer manager snapshot.
---
Nitpick comments:
In `@src/components/pdf-reader/saveAndDeriveFlow.test.ts`:
- Around line 31-137: Add a regression test that mocks derivePage to resolve
with a response that includes pageId but omits noteId and assert
runSaveAndDeriveFlow rejects; specifically, create mocks for createHighlight
(returning makeHighlight()), derivePage.mockResolvedValue({ pageId:
"page-missing-note" }), and navigate = vi.fn(), then await
expect(runSaveAndDeriveFlow(...)).rejects.toThrow() (or rejects with the
specific error if known) and assert navigate was not called and no successful
result is returned; reference the runSaveAndDeriveFlow function and the
derivePage/createHighlight/navigate mocks to locate where to add this case.
In `@src/hooks/runAIChatAction.test.ts`:
- Line 50: Add a test case in src/hooks/runAIChatAction.test.ts that covers the
navigation guard when createPageMutateAsync returns an object missing noteId:
change or add a mock for createPageMutateAsync (the vi.fn used in the tests) to
mockResolvedValue({ id: "new-page-1" }) and assert that navigate was not called
(expect(navigate).not.toHaveBeenCalled()); replicate the same missing-noteId
assertion in the other affected test blocks referenced around the existing
createPageMutateAsync mocks (the instances near the current mocks at the spots
corresponding to lines 67, 100, and 132) so each scenario verifies navigation is
skipped when noteId is absent.
In `@src/hooks/useAIChatActions.test.ts`:
- Line 82: Add a test in useAIChatActions.test.ts that covers the mutation
resolving with an id but no noteId: set
mockCreatePageMutateAsync.mockResolvedValue({ id: "new-page-id" }) and then call
the action that triggers createPage; assert that the navigation mock (e.g.,
mockNavigate or router.push) is NOT called and/or the failure toast mock is
called, ensuring we don't navigate to /notes/undefined/...; locate helpers by
name (mockCreatePageMutateAsync, useAIChatActions, mockNavigate/mockToast) and
mirror the existing successful-create test structure to keep setup/teardown
consistent.
In `@src/hooks/useCollaboration.ts`:
- Around line 40-51: The exported hook useCollaboration currently has only
Japanese TSDoc — update the comment block to include both Japanese and English
descriptions, parameters, return values, and example(s) using TSDoc/JSDoc
conventions so both languages appear (e.g., Japanese first then English or vice
versa), and ensure tags like `@example`, `@param`, and `@returns` are present for the
exported function signature; reference the useCollaboration hook name so the
bilingual docs are colocated with the existing comment and follow the
repository's documentation style guidelines.
In `@src/hooks/useLinkedPages.test.ts`:
- Line 12: Add an explicit assertion checking that the noteId from the fixture
is preserved by the mapping logic: in the test that uses the updated fixture
(the one passing noteId: "test-note"), assert that pageToCard(...) returns an
object whose noteId equals the fixture's noteId; do the same for the
summary-fallback path (the code path that generates a card from a summary) so
both pageToCard and the summary fallback have direct expectations validating
noteId preservation.
In `@src/lib/collaboration/CollaborationManager.test.ts`:
- Around line 114-119: Add tests in CollaborationManager.test.ts to assert the
new Phase 3 startup contract: (1) verify HocuspocusProvider is instantiated only
after the IndexedDB mock emits the 'synced' event (use the CollaborationManager
startup path or the function that currently creates HocuspocusProvider), and (2)
simulate a delayed 'synced' emission or token resolution, call destroy() before
it resolves, and assert that no connection/instantiation of HocuspocusProvider
occurs afterward. Reference the symbols HocuspocusProvider, 'synced' event
emission on the IndexedDB mock, the token resolution/promise used in startup,
and destroy() to locate where to add these assertions.
In `@src/pages/NotePageView.tsx`:
- Around line 738-741: The variable locationStateInitialContent uses a redundant
nullish coalescing (?? undefined) after a type guard; remove the `?? undefined`
and simply return the casted `initialContent` when `typeof (location.state as {
initialContent?: unknown } | null)?.initialContent === "string"` is true so
`locationStateInitialContent` becomes that string value directly (keep the
existing cast to `{ initialContent: string }` to satisfy the type assertion),
eliminating the unnecessary fallback.
🪄 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: 17ad14e6-f81a-4260-9e99-84e3dc3b9cc5
📒 Files selected for processing (68)
e2e/auth-mock.tse2e/page-editor.spec.tse2e/wikilink-create-dialog.spec.tsserver/api/src/__tests__/routes/pages.test.tsserver/api/src/routes/pages.tsserver/api/src/routes/pdfSources.tsserver/api/src/routes/search.tssrc/App.tsxsrc/components/ai-chat/PromoteToWikiDialog.tsxsrc/components/editor/PageEditor/PageEditorAlerts.tsxsrc/components/editor/PageEditor/PageEditorDialogs.tsxsrc/components/editor/PageEditor/PageEditorLayout.test.tsxsrc/components/editor/PageEditor/PageEditorLayout.tsxsrc/components/editor/PageEditor/types.tssrc/components/editor/PageEditor/useEditorAutoSave.test.tssrc/components/editor/PageEditor/useEditorAutoSave.tssrc/components/editor/PageEditor/usePageDeletion.test.tssrc/components/editor/PageEditor/usePageDeletion.tssrc/components/editor/PageEditor/usePageEditor.tssrc/components/editor/PageEditor/usePageEditorAIEffects.tssrc/components/editor/PageEditor/usePageEditorAutoSaveWithMutation.tssrc/components/editor/PageEditor/usePageEditorEffects.test.tsxsrc/components/editor/PageEditor/usePageEditorEffects.tssrc/components/editor/PageEditor/usePageEditorHandlers.tssrc/components/editor/PageEditor/usePageEditorKeyboard.tssrc/components/editor/PageEditor/usePageEditorState.tssrc/components/editor/PageEditor/usePageEditorStateAndSync.tssrc/components/editor/PageEditor/usePageEditorStateAndSyncReturnSlices.tssrc/components/editor/PageEditor/usePageEditorWikiCollab.tssrc/components/editor/PageEditor/usePendingChatPageGeneration.test.tsxsrc/components/editor/PageEditor/usePendingChatPageGeneration.tssrc/components/editor/PageEditorView.tsxsrc/components/editor/TiptapEditor/useWikiLinkNavigation.test.tssrc/components/editor/TiptapEditor/useWikiLinkNavigation.tssrc/components/layout/useFloatingActionButtonHandlers.tssrc/components/page/LinkGroupRow.tsxsrc/components/page/LinkSection.tsxsrc/components/page/LinkedPagesSection.test.tsxsrc/components/page/LinkedPagesSection.tsxsrc/components/page/PageCard.tsxsrc/components/page/PageLinkCard.test.tsxsrc/components/pdf-reader/HighlightLayer.test.tsxsrc/components/pdf-reader/HighlightSidebar.test.tsxsrc/components/pdf-reader/HighlightSidebar.tsxsrc/components/pdf-reader/PdfReader.tsxsrc/components/pdf-reader/saveAndDeriveFlow.test.tssrc/components/pdf-reader/saveAndDeriveFlow.tssrc/components/search/SearchResultCard.tsxsrc/contexts/GlobalSearchContext.test.tssrc/contexts/GlobalSearchContext.tsxsrc/hooks/runAIChatAction.test.tssrc/hooks/runAIChatAction.tssrc/hooks/useAIChatActions.test.tssrc/hooks/useAIChatActions.tssrc/hooks/useCollaboration.tssrc/hooks/useCreateNewPage.tssrc/hooks/useGlobalSearch.test.tssrc/hooks/useGlobalSearch.tssrc/hooks/useLinkedPages.test.tssrc/hooks/useLinkedPages.tssrc/lib/api/types.tssrc/lib/collaboration/CollaborationManager.test.tssrc/lib/collaboration/CollaborationManager.tssrc/lib/collaboration/types.tssrc/lib/pdfKnowledge/highlightsApi.tssrc/pages/NotePageView.tsxsrc/pages/PageEditor.tsxsrc/pages/SearchResults.tsx
💤 Files with no reviewable changes (27)
- src/components/editor/PageEditorView.tsx
- src/pages/PageEditor.tsx
- src/components/editor/PageEditor/usePendingChatPageGeneration.ts
- src/components/editor/PageEditor/PageEditorLayout.tsx
- src/components/editor/PageEditor/usePageEditorAIEffects.ts
- e2e/wikilink-create-dialog.spec.ts
- src/components/editor/PageEditor/usePageDeletion.ts
- src/components/editor/PageEditor/types.ts
- e2e/page-editor.spec.ts
- src/components/editor/PageEditor/PageEditorLayout.test.tsx
- src/components/editor/PageEditor/usePageEditorWikiCollab.ts
- src/components/editor/PageEditor/usePageEditorEffects.ts
- src/components/editor/PageEditor/usePageEditorStateAndSync.ts
- src/components/editor/PageEditor/usePageEditorAutoSaveWithMutation.ts
- src/components/editor/PageEditor/usePageEditorState.ts
- src/components/editor/PageEditor/usePageEditorHandlers.ts
- src/components/editor/PageEditor/useEditorAutoSave.ts
- src/components/editor/PageEditor/usePageEditorKeyboard.ts
- src/components/editor/PageEditor/PageEditorDialogs.tsx
- src/components/editor/PageEditor/usePageEditorStateAndSyncReturnSlices.ts
- src/components/editor/PageEditor/PageEditorAlerts.tsx
- src/components/editor/PageEditor/usePageDeletion.test.ts
- src/components/editor/PageEditor/usePageEditor.ts
- src/components/editor/PageEditor/useEditorAutoSave.test.ts
- src/components/editor/PageEditor/usePageEditorEffects.test.tsx
- src/components/editor/PageEditor/usePendingChatPageGeneration.test.tsx
- src/App.tsx
PR #894 のレビューフィードバックを反映。Codex P1 二件と CodeRabbit Major 複数件への対応。 - 取り残されていた `/pages/:id` リンク発火を全て撤去（Codex P1）: - `AIChatWikiLink` の `<Link to="/pages/...">` を `page.noteId` 付きに。 - `Onboarding` 完了後の `welcome_page_id` 遷移は新規 `welcome_page_note_id` と組で `/notes/:noteId/:pageId` へ。サーバ側 `WelcomePageCreationResult` に `noteId` を追加し、`POST /api/onboarding/complete` / `GET /api/onboarding/status` も `welcome_page_note_id` を返す。 - `IndexPage` の `__index__` ページリンクは `/api/activity/index` / `/api/activity/index/rebuild` が新たに返す `noteId` を使う。 `PersistIndexResult` / `rebuildIndexForOwner` も `noteId` を伝搬。 - `NotePageView` の `pendingInitialContent` を location.key 変更で再水和 できるよう同期 derived-state 化（Codex P1: NotePageView マウント中の intra-route navigation で seed が落ちていた）。`useEffect + setState` は `you-might-not-need-an-effect` ルールに引っかかるため避け、ref 経由の 最終適用キー記録は新 ルールの "Cannot access refs during render" を 踏むため、適用後の navigate クリアが `locationStateInitialContent` を undefined にする副作用に依存して二重適用を防ぐ。 - `noteId` 欠落時に `/notes/undefined/...` を組み立てるのを防ぐガードを AI チャット作成系・PromoteToWikiDialog に追加（CodeRabbit Major）。 - `useCreateNewPage` / `useFloatingActionButtonHandlers` の link-failed パスを「ページは作成済み」前提でデフォルトノート配下にフォールバック 遷移し、`common.attachPageToNoteFailed` (新規 i18n key) で正確な失敗 内容を伝える（CodeRabbit Minor）。 - `NotePageView` の冗長な `?? undefined` を削除（CodeRabbit nitpick）。 - Knip 失敗 (`src/hooks/useTitleValidation.ts` が unused) を解消。Phase 3 で `PageEditor/usePageEditorState.ts` が唯一の consumer だったので ファイル自体も削除した。 PR #894 review responses (Codex P1 + CodeRabbit Major): - Migrate the remaining `/pages/:id` emitters Codex caught: `AIChatWikiLink`, `Onboarding` welcome-page landing, and the `IndexPage` `__index__` link. Server-side adds `welcome_page_note_id` to the onboarding endpoints and `noteId` to the activity-index GET / rebuild responses + the `PersistIndexResult` shape. - Rehydrate `pendingInitialContent` on intra-mount `/notes/:noteId/:pageId` navigations (Codex P1). Uses the synchronous "derived state from location.key" pattern to satisfy the new `you-might-not-need-an-effect` lint rule without reading refs during render. - Guard `noteId` (alongside `id`) before building `/notes/:noteId/:pageId` in `runAIChatAction`, `useAIChatActions`, and `PromoteToWikiDialog` so partial backend responses never produce `/notes/undefined/...`. - `useCreateNewPage` / `useFloatingActionButtonHandlers` now fall back to the page's default-note URL when `addPageToNoteMutation` fails (the page itself was created successfully) and surface a new `common.attachPageToNoteFailed` toast so the message matches the actual failure (CodeRabbit Minor). - Drop the unused `useTitleValidation` hook (only consumer was the Phase-3-deleted `usePageEditorState.ts`) — this was the Knip CI failure. Test plan: - `bun run lint` — 0 errors - `bun run format:check` — clean - `DATABASE_URL=postgres://... bunx knip` — clean, exit 0 - `bunx vitest run` (main) — 229 files / 2327 tests pass - `cd server/api && bunx vitest run` — 95 files / 1280 tests pass Refs: PR #894 (#894) https://claude.ai/code/session_01CVtupQrUS23UEerQJgPgLH
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/lib/welcomePageService.ts`:
- Line 193: When the insert conflicts and existingId is truthy, don't return
defaultNote.id; instead load the persisted page's noteId and return that so the
response uses the actual stored noteId. Modify the conflict-read path around
existingId to fetch the existing page record (the stored noteId for
pageId/existingId) and return { pageId: existingId, noteId: persistedNoteId,
locale } rather than { pageId: existingId, noteId: defaultNote.id, locale }; use
the same DB access utilities already in welcomePageService to retrieve the
persisted noteId.
In `@src/lib/api/apiClient.ts`:
- Around line 884-885: Update the inline documentation for the property
welcome_page_note_id so it includes both English and Japanese per repo
standards: add a Japanese translation line alongside the existing English
comment in the same comment block immediately above the welcome_page_note_id
declaration, mirroring the style/format used elsewhere in apiClient.ts (e.g.,
bilingual comments for other fields) so the comment contains both languages.
In `@src/pages/IndexPage.tsx`:
- Around line 48-49: The new shorthand comments around the IndexFetchResponse
fields (e.g., the comment above noteId and the similar comment at the fields
around lines 62–63) are English-only; update those inline comments to be
bilingual by adding concise Japanese translations alongside the existing English
(match style of surrounding docs), e.g., prepend or append a Japanese sentence
for the same meaning for the comment that references "Issue `#889` Phase 3: see
`IndexFetchResponse.noteId`" and the other shorthand comment so both English and
Japanese descriptions appear for the same symbols (`noteId` and the nearby
fields).
In `@src/pages/NotePageView.tsx`:
- Around line 772-776: The branch that updates trackedLocationKey should also
reset pendingInitialContent unconditionally so stale seeds are not retained:
when trackedLocationKey !== location.key call
setTrackedLocationKey(location.key) and then call setPendingInitialContent with
the current locationStateInitialContent or a cleared value (e.g.,
null/undefined) even if locationStateInitialContent is falsy; update the logic
around trackedLocationKey, location.key, locationStateInitialContent,
setTrackedLocationKey and setPendingInitialContent to always synchronize/reset
pendingInitialContent on every route-key change.
🪄 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: a26e12fa-6e06-4c96-859b-f634e740c4d3
📒 Files selected for processing (19)
server/api/src/__tests__/routes/activity.test.tsserver/api/src/__tests__/routes/onboarding.test.tsserver/api/src/lib/welcomePageService.tsserver/api/src/routes/activity.tsserver/api/src/routes/onboarding.tsserver/api/src/services/indexBuilder.tssrc/components/ai-chat/AIChatWikiLink.tsxsrc/components/ai-chat/PromoteToWikiDialog.tsxsrc/components/layout/useFloatingActionButtonHandlers.tssrc/hooks/runAIChatAction.tssrc/hooks/useAIChatActions.tssrc/hooks/useCreateNewPage.tssrc/hooks/useTitleValidation.tssrc/i18n/locales/en/common.jsonsrc/i18n/locales/ja/common.jsonsrc/lib/api/apiClient.tssrc/pages/IndexPage.tsxsrc/pages/NotePageView.tsxsrc/pages/Onboarding.tsx
💤 Files with no reviewable changes (1)
- src/hooks/useTitleValidation.ts
✅ Files skipped from review due to trivial changes (1)
- src/i18n/locales/ja/common.json
🚧 Files skipped from review as they are similar to previous changes (5)
- src/hooks/useAIChatActions.ts
- src/hooks/useCreateNewPage.ts
- src/components/ai-chat/PromoteToWikiDialog.tsx
- src/hooks/runAIChatAction.ts
- src/components/layout/useFloatingActionButtonHandlers.ts
…lingual) PR #894 の追加レビュー（CodeRabbit）への対応。 - `NotePageView` で `location.key` が変化するたびに `pendingInitialContent` を現在の location.state から無条件に再水和する ように修正。state なしの隣ページ遷移で未消費 seed が残り、 `<NotePageEditorEditable key={page.id}>` 再マウント先に古い initialContent が渡る回帰を防ぐ。 - `welcomePageService.findExistingWelcomePage` の戻り値を `{ id, noteId }` に拡張し、conflict-read パスでも永続化済みの note_id を返すように。 並行 insert で勝者ノートと自前で resolve したデフォルトノートがずれた 場合に、不正な遷移先 URL を生成しないようにする。 - `apiClient.getOnboardingStatus` の `welcome_page_note_id` と `IndexPage` の `IndexRebuildResponse.noteId` / `IndexViewModel.noteId` のコメントをバイリンガル化（プロジェクト規約 `**/*.{ts,tsx,js,md}` は 日本語・英語の両方を要求）。 CodeRabbit follow-up review fixes: - `NotePageView`: unconditionally re-sync `pendingInitialContent` from the current location on every `location.key` change. Without this, a route change without `location.state.initialContent` retained the prior pending seed and could leak it into the next `key={page.id}` remount. - `welcomePageService`: return the persisted `note_id` from the conflict-read branch so a concurrent insert that landed in a different note than the local `ensureDefaultNote` resolution still produces a valid `/notes/:noteId/:pageId` URL. - `apiClient.ts` / `IndexPage.tsx`: extend the new shorthand TSDoc to be bilingual per the repo coding guideline. Test plan: - `bun run lint` — 0 errors - `bun run format:check` — clean - `bunx vitest run src/pages/NotePageView` — 29 tests pass - `cd server/api && bunx vitest run src/__tests__/routes/onboarding.test.ts` — 11 tests pass Refs: PR #894 (#894) https://claude.ai/code/session_01CVtupQrUS23UEerQJgPgLH
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.
概要
Issue #889 Phase 3 として、レガシーな
/pages/:idルートと個人ページ専用エディタを廃止し、全ページを所属ノート経由でアクセスする統一的なアーキテクチャに移行します。ページエディタの複雑な状態管理レイヤーを削除し、NotePageView に統合することで、コードベースを大幅に簡潔化します。変更点
削除されたファイル
PageEditorView.tsx,PageEditor.tsx(ルート),usePageEditor.ts,usePageEditorState.ts,usePageEditorStateAndSync.ts,usePageEditorHandlers.ts,usePageEditorEffects.ts,usePageEditorAIEffects.ts,usePageEditorAutoSaveWithMutation.ts,usePageEditorWikiCollab.ts,usePageEditorKeyboard.tsPageEditorLayout.tsx,PageEditorAlerts.tsx,PageEditorDialogs.tsxuseEditorAutoSave.ts,usePendingChatPageGeneration.ts,usePageDeletion.ts.test.ts(x)ファイル群（合計 ~2,800 行）e2e/page-editor.spec.ts,e2e/wikilink-create-dialog.spec.ts修正・更新されたファイル
localモード廃止、REST 経由 Y.Doc 保存経路削除。全ページは Hocuspocus WebSocket 経由でリアルタイム同期local-userフォールバック説明を更新localY.js mode and route all pages through Hocuspocus #889 Phase 3 対応）/notes/:noteId/:pageIdに統一/pages/:idルート削除/notes/:noteId/:pageIdに統一runAIChatAction.ts,useAIChatActions.ts,PromoteToWikiDialog.tsx,PdfReader.tsxなどPageCard,LinkedPagesSection,SearchResultCardにnoteIdフィールド追加CreatePageBodyにnote_idフィールド追加、PDF ハイライト派生ページにderivedPageNoteId追加noteId値を更新（null→"default-note"など）変更の種類
/pages/:idルート廃止https://claude.ai/code/session_01CVtupQrUS23UEerQJgPgLH
Summary by CodeRabbit