refactor(api): retire GET/PUT /api/pages/:id/content (Issue #889 Phase 4)#895
Conversation
…e 4) Phase 4 of #889 removes the legacy REST Y.Doc round-trip and the API-side duplicate implementations now that all editing flows through Hocuspocus. - Delete `GET/PUT /api/pages/:id/content` route handlers (along with their lazy-migration / defense-in-depth normalization helpers). - Delete duplicated `server/api/src/services/snapshotService.ts` and `server/api/src/services/ydocWikiLinkNormalizer.ts`; the prune helper used by snapshot restore is inlined into `pageSnapshots.ts`. - Drop the cross-server drift detector `ydocWikiLinkNormalizerSync.test.ts`; Hocuspocus is now the sole owner of these implementations. - Migrate `NotePageView` title persistence from `getPageContent` + `putPageContent` to the metadata-only `updatePageMetadata` (`PUT /api/pages/:id`); remove `getPageContent` / `putPageContent` from the API client and types. - Refresh route / service / cache-key doc comments to reference the retained metadata route and the Hocuspocus normalization path. https://claude.ai/code/session_018GADvyHXYY4Zem7Q7y8T6G
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? |
📝 WalkthroughWalkthroughThis PR retires the legacy REST Y.Doc sync endpoints ( ChangesPhase 4 Endpoint Retirement & Service Consolidation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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 |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 17e57e5025
ℹ️ 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".
| * Issue #889 Phase 4: `local` コラボレーションモード廃止に伴い、Y.Doc バイト列を | ||
| * 直接やり取りする `GET/PUT /api/pages/:id/content` ルートは削除した。本文の | ||
| * 編集はすべて Hocuspocus WebSocket 経由、読み取り専用閲覧は | ||
| * `GET /api/pages/:id/public-content` 経由に統一されている。 |
There was a problem hiding this comment.
Preserve /api/pages/:id/content for MCP callers
Removing GET/PUT /api/pages/:id/content here breaks the in-repo MCP service, which still calls those routes in production code (server/mcp/src/client/httpClient.ts and server/mcp/src/tools/index.ts). After this commit, MCP tools like zedi_get_page and zedi_update_page_content will receive 404s against the updated API, so page read/update operations through MCP regress immediately unless the MCP client/tools are migrated in the same change.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
server/api/src/services/pageGraphSyncService.ts (1)
13-18: ⚡ Quick winAlign remaining caller docs with the retired
/contentroute statement.Line 13 says internal HTTP is now the only trigger, but later in this file the
syncPageGraphFromStoredYDocdoc still mentions the RESTPUT /contentpath. Please update that stale line to keep the caller contract unambiguous.Suggested doc-only patch
- * and by the REST PUT /content path (fire-and-forget). + * and by API-side fire-and-forget callers that schedule graph sync after persistence.🤖 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/services/pageGraphSyncService.ts` around lines 13 - 18, Update the stale doc comment for syncPageGraphFromStoredYDoc to remove the reference to the retired REST route `PUT /api/pages/:id/content` and instead state that the function is invoked only via Hocuspocus internal HTTP after persistence; locate the docblock above the syncPageGraphFromStoredYDoc function and replace the sentence that mentions the REST `PUT /content` path so the caller contract clearly reflects the current single trigger (Hocuspocus internal HTTP).
🤖 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.
Nitpick comments:
In `@server/api/src/services/pageGraphSyncService.ts`:
- Around line 13-18: Update the stale doc comment for
syncPageGraphFromStoredYDoc to remove the reference to the retired REST route
`PUT /api/pages/:id/content` and instead state that the function is invoked only
via Hocuspocus internal HTTP after persistence; locate the docblock above the
syncPageGraphFromStoredYDoc function and replace the sentence that mentions the
REST `PUT /content` path so the caller contract clearly reflects the current
single trigger (Hocuspocus internal HTTP).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a51bdd4f-80b8-4409-b6da-7dffd8cf3c6d
📒 Files selected for processing (22)
server/api/src/__tests__/routes/notes/crud.test.tsserver/api/src/__tests__/routes/pages.test.tsserver/api/src/__tests__/services/snapshotService.test.tsserver/api/src/__tests__/services/ydocWikiLinkNormalizer.test.tsserver/api/src/routes/notes/crud.tsserver/api/src/routes/pageSnapshots.tsserver/api/src/routes/pages.tsserver/api/src/services/pageGraphSyncService.tsserver/api/src/services/snapshotService.tsserver/api/src/services/ydocWikiLinkNormalizer.tsserver/hocuspocus/src/snapshotUtils.tsserver/hocuspocus/src/ydocWikiLinkNormalizer.test.tsserver/hocuspocus/src/ydocWikiLinkNormalizer.tssrc/components/editor/TiptapEditor/useEditorLifecycle.tssrc/lib/api/apiClient.test.tssrc/lib/api/apiClient.tssrc/lib/api/types.tssrc/lib/pageRepository/StorageAdapterPageRepository.test.tssrc/lib/sync/syncWithApi.test.tssrc/lib/ydocWikiLinkNormalizerSync.test.tssrc/pages/NotePageView.test.tsxsrc/pages/NotePageView.tsx
💤 Files with no reviewable changes (8)
- src/lib/ydocWikiLinkNormalizerSync.test.ts
- server/api/src/tests/services/ydocWikiLinkNormalizer.test.ts
- server/api/src/services/ydocWikiLinkNormalizer.ts
- server/api/src/tests/services/snapshotService.test.ts
- server/api/src/services/snapshotService.ts
- src/lib/sync/syncWithApi.test.ts
- src/lib/api/types.ts
- src/lib/pageRepository/StorageAdapterPageRepository.test.ts
There was a problem hiding this comment.
Code Review
This pull request retires the 'local' collaboration mode by removing the legacy REST endpoints for Y.Doc content and consolidating metadata updates into the PUT /api/pages/:id endpoint. Content editing is now handled exclusively via Hocuspocus over WebSockets, allowing for the removal of duplicate auto-snapshotting and Y.Doc normalization logic from the API server. I have no feedback to provide.
Phase 4 of #889 removes the legacy REST Y.Doc round-trip and the
API-side duplicate implementations now that all editing flows through
Hocuspocus.
GET/PUT /api/pages/:id/contentroute handlers (along withtheir lazy-migration / defense-in-depth normalization helpers).
server/api/src/services/snapshotService.tsandserver/api/src/services/ydocWikiLinkNormalizer.ts; the prune helperused by snapshot restore is inlined into
pageSnapshots.ts.ydocWikiLinkNormalizerSync.test.ts;Hocuspocus is now the sole owner of these implementations.
NotePageViewtitle persistence fromgetPageContent+putPageContentto the metadata-onlyupdatePageMetadata(PUT /api/pages/:id); removegetPageContent/putPageContentfrom the API client and types.retained metadata route and the Hocuspocus normalization path.
https://claude.ai/code/session_018GADvyHXYY4Zem7Q7y8T6G
Summary by CodeRabbit
Release Notes
Refactor
Documentation