fix: prevent same-title page rewrites by matching targetId (issue #737)#738
Merged
Conversation
) Implements the two follow-ups tracked by issue #737: 1. Same-title rename ambiguity (approach A — `targetId` on marks) - Add `targetId` attribute to `wikiLink` and `tag` Tiptap marks. - Populate it during link resolution (`useWikiLinkStatusSync`, `useTagStatusSync`, `useBubbleMenuWikiLink`); `useWikiLinkExistsChecker` now returns a `pageTitleToId` map. - Switch `rewriteTitleRefsInDoc` to id-strict matching when a mark carries `targetId`; id-less marks still fall back to title matching for lazy migration of pre-existing Y.Docs. - Plumb `renamedPageId` through `propagateTitleRename` → `rewriteSourcePage` → `rewriteTitleRefsInDoc`. 2. Shared tag character class - New `@zedi/shared` workspace package owns `TAG_NAME_CHAR_CLASS` (single source of truth for client + admin). - Client `TagExtension` builds its regex from the shared constant. - `server/api` (intentionally outside the workspace, see AGENTS.md) keeps a duplicated `TAG_NAME_CHAR_CLASS_STRING`; a new client-side drift-check vitest (`src/lib/tagCharacterClassSync.test.ts`) reads the server file and asserts both literals match in CI. - AGENTS.md documents the cross-workspace sharing pattern and the server-side drift-check workaround.
The previous commit's lint-staged auto-fix added blank `/**\n *\n */` stubs to every variable declaration in `useBubbleMenuWikiLink.ts` and to `WikiLinkInfo` in `wikiLinkUtils.ts`. Replace those with real JSDoc descriptions on the public exports so `jsdoc/require-jsdoc` is satisfied without the stub noise polluting the file body.
|
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)
📝 WalkthroughWalkthroughAdds a new workspace package Changes
Sequence Diagram(s)sequenceDiagram
participant Editor as Editor (Client)
participant Checker as ExistsChecker / usePageQueries
participant Server as API (Rename/Save)
participant YDoc as Y.Doc / DB
Editor->>Checker: request existence for titles
Checker-->>Editor: { pageTitles, referencedTitles, pageTitleToId }
Editor->>Editor: insert/convert mark (title, exists, targetId?)
Editor->>Server: save page (page contents with marks)
Server->>YDoc: load source Y.Doc(s)
Server->>Server: rewriteTitleRefsInDoc(oldTitle,newTitle,{renamedPageId})
Server->>YDoc: persist updated Y.Doc(s)
Server-->>Editor: persist ack
Estimated code review effort🎯 4 (Complex) | ⏱️ ~65 minutes Possibly related PRs
Poem
🚥 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 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 introduces a shared workspace package for constants and implements strict ID-based matching for WikiLinks and tags to prevent incorrect renames when duplicate titles exist. The changes span the Tiptap editor extensions, client-side status synchronization hooks, and server-side rename propagation services. Feedback highlights an opportunity to refactor the existence-checking logic in the client hooks to reduce code duplication and improve data processing efficiency.
Comment on lines
842
to
860
| if (pageNoteId !== null) { | ||
| if (notePages === undefined) { | ||
| return { pageTitles: new Set(), referencedTitles: new Set() }; | ||
| return { | ||
| pageTitles: new Set(), | ||
| referencedTitles: new Set(), | ||
| pageTitleToId: new Map(), | ||
| }; | ||
| } | ||
| pageTitles = new Set(notePages.map((p) => p.title.toLowerCase().trim())); | ||
| for (const p of notePages) { | ||
| pageTitleToId.set(p.title.toLowerCase().trim(), p.id); | ||
| } | ||
| } else { | ||
| const pages = await repo.getPagesSummary(userId); | ||
| pageTitles = new Set(pages.map((p) => p.title.toLowerCase().trim())); | ||
| for (const p of pages) { | ||
| pageTitleToId.set(p.title.toLowerCase().trim(), p.id); | ||
| } | ||
| } |
There was a problem hiding this comment.
The logic for populating pageTitles and pageTitleToId is duplicated across the if/else branches and involves multiple iterations over the same data. This can be refactored into a single pass over a unified source list to improve maintainability and efficiency.
const sourcePages = pageNoteId !== null ? notePages : await repo.getPagesSummary(userId);
if (pageNoteId !== null && sourcePages === undefined) {
return {
pageTitles: new Set(),
referencedTitles: new Set(),
pageTitleToId: new Map(),
};
}
pageTitles = new Set();
for (const p of sourcePages ?? []) {
const normalized = p.title.toLowerCase().trim();
pageTitles.add(normalized);
pageTitleToId.set(normalized, p.id);
}…cker Address Gemini review on PR #738: collapse the duplicated note / personal branches into one walk that builds both `pageTitles` and `pageTitleToId` from a unified `sourcePages` list. The previous shape called `.map()` to seed the Set and a separate `for` loop to populate the Map for each branch, walking the candidate list twice. Behaviour is preserved (unloaded `notePages` in note scope still returns empty sets).
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (4)
packages/shared/src/tagCharacterClass.ts (1)
18-20: Minor documentation note: Unicode range comment covers two distinct blocks.The comment states
U+3040..U+309F (ヿ の前半)andU+30A0..U+30FF (ヿ の後半), but the actual range-ヿspans U+3040–U+30FF continuously (both hiragana and katakana together). This is functionally correct but the comment structure might be slightly confusing since it describes them as "前半/後半" (first/second half) of the same range rather than two separate Unicode blocks.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/shared/src/tagCharacterClass.ts` around lines 18 - 20, The Unicode-range comment in tagCharacterClass.ts is misleading: clarify that hiragana (U+3040..U+309F) and katakana (U+30A0..U+30FF) are two distinct Unicode blocks even though the visual range "-ヿ" spans them contiguously; update the comment to either list the blocks separately (hiragana: U+3040..U+309F, katakana: U+30A0..U+30FF) or explicitly state that "-ヿ (U+3040..U+30FF) covers both hiragana and katakana" so readers aren’t confused by the "前半/後半" phrasing.src/components/editor/TiptapEditor/useBubbleMenuWikiLink.ts (1)
28-28: Add an explicit return type to the exported hook.The hook’s public contract is currently inferred from the returned object. Making that shape explicit will keep downstream consumers stable as this payload evolves.
As per coding guidelines, "Use TypeScript strict mode; forbid `any` type and require explicit type annotations."♻️ Suggested typing cleanup
+interface UseBubbleMenuWikiLinkResult { + isWikiLinkSelection: boolean; + convertToWikiLink: () => Promise<void>; + unsetWikiLink: () => void; + isConverting: boolean; +} + -export function useBubbleMenuWikiLink({ editor, pageId }: UseBubbleMenuWikiLinkOptions) { +export function useBubbleMenuWikiLink({ + editor, + pageId, +}: UseBubbleMenuWikiLinkOptions): UseBubbleMenuWikiLinkResult {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/editor/TiptapEditor/useBubbleMenuWikiLink.ts` at line 28, The exported hook useBubbleMenuWikiLink lacks an explicit return type; define a specific return type (e.g., interface or type alias like UseBubbleMenuWikiLinkReturn) describing the exact shape of the returned object and annotate the function signature as export function useBubbleMenuWikiLink(...): UseBubbleMenuWikiLinkReturn { ... }; ensure the defined type avoids any usage, reflects all returned properties/methods, and is exported if it is part of the public API so downstream consumers remain stable.server/api/src/__tests__/services/ydocRenameRewrite.test.ts (1)
335-405: Add tag parity cases for the fallback branches.The new tag suite covers the strict
targetIdmatch, but the backward-compatible branches are only asserted forwikiLink. A tag-specific regression in the “notargetId” / “norenamedPageId” paths could still slip through.Based on learnings, "Tests serve as a source of truth for specifications alongside implementation code TSDoc/JSDoc."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/api/src/__tests__/services/ydocRenameRewrite.test.ts` around lines 335 - 405, Add parity tests for tag marks mirroring the two wikiLink fallback cases: call rewriteTitleRefsInDoc with and without the renamedPageId and assert behavior on tag marks — first, create a tag-marked insert with a targetId (e.g. targetId: OTHER_PAGE_ID) and call rewriteTitleRefsInDoc("Foo","Bar") without renamedPageId, then assert result.tagMarksUpdated === 0 and plainText stays "Foo"; second, create a tag-marked insert without targetId and call rewriteTitleRefsInDoc("Foo","Bar") without renamedPageId, then assert result.tagMarksUpdated === 1 (and/or result.tagTextUpdated) and plainText becomes "Bar". Ensure the tests reference rewriteTitleRefsInDoc and check result.tagMarksUpdated/result.tagTextUpdated so tag-specific fallback behavior is covered.server/api/src/services/ydocRenameRewrite.ts (1)
79-81: Static analysis false positive — regex is safe.The ast-grep ReDoS warning is a false positive:
TAG_NAME_CHAR_CLASS_STRINGis a hardcoded constant, not user input- The pattern
^[...]+$has no nested quantifiers or alternations that could cause catastrophic backtracking- Time complexity is linear in input length
Consider adding a brief comment to preempt future tooling warnings:
📝 Optional: Add safety comment
export const TAG_NAME_CHAR_CLASS_STRING = "A-Za-z0-9_\\--ヿ㐀-鿿"; +// ReDoS-safe: constant pattern with no nested quantifiers or alternations. const VALID_TAG_NAME_REGEX = new RegExp(`^[${TAG_NAME_CHAR_CLASS_STRING}]+$`);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/api/src/services/ydocRenameRewrite.ts` around lines 79 - 81, The regex warning is a false positive; add a brief explanatory comment above TAG_NAME_CHAR_CLASS_STRING and VALID_TAG_NAME_REGEX stating that the character class is a hardcoded constant (not user input), the pattern is a simple anchored character-class with a single quantifier and has linear-time complexity, so it is safe from ReDoS; reference TAG_NAME_CHAR_CLASS_STRING and VALID_TAG_NAME_REGEX in the comment to make intent explicit for future reviewers and tooling.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/shared/src/tagCharacterClass.ts`:
- Around line 18-20: The Unicode-range comment in tagCharacterClass.ts is
misleading: clarify that hiragana (U+3040..U+309F) and katakana (U+30A0..U+30FF)
are two distinct Unicode blocks even though the visual range "-ヿ" spans them
contiguously; update the comment to either list the blocks separately (hiragana:
U+3040..U+309F, katakana: U+30A0..U+30FF) or explicitly state that "-ヿ
(U+3040..U+30FF) covers both hiragana and katakana" so readers aren’t confused
by the "前半/後半" phrasing.
In `@server/api/src/__tests__/services/ydocRenameRewrite.test.ts`:
- Around line 335-405: Add parity tests for tag marks mirroring the two wikiLink
fallback cases: call rewriteTitleRefsInDoc with and without the renamedPageId
and assert behavior on tag marks — first, create a tag-marked insert with a
targetId (e.g. targetId: OTHER_PAGE_ID) and call
rewriteTitleRefsInDoc("Foo","Bar") without renamedPageId, then assert
result.tagMarksUpdated === 0 and plainText stays "Foo"; second, create a
tag-marked insert without targetId and call rewriteTitleRefsInDoc("Foo","Bar")
without renamedPageId, then assert result.tagMarksUpdated === 1 (and/or
result.tagTextUpdated) and plainText becomes "Bar". Ensure the tests reference
rewriteTitleRefsInDoc and check result.tagMarksUpdated/result.tagTextUpdated so
tag-specific fallback behavior is covered.
In `@server/api/src/services/ydocRenameRewrite.ts`:
- Around line 79-81: The regex warning is a false positive; add a brief
explanatory comment above TAG_NAME_CHAR_CLASS_STRING and VALID_TAG_NAME_REGEX
stating that the character class is a hardcoded constant (not user input), the
pattern is a simple anchored character-class with a single quantifier and has
linear-time complexity, so it is safe from ReDoS; reference
TAG_NAME_CHAR_CLASS_STRING and VALID_TAG_NAME_REGEX in the comment to make
intent explicit for future reviewers and tooling.
In `@src/components/editor/TiptapEditor/useBubbleMenuWikiLink.ts`:
- Line 28: The exported hook useBubbleMenuWikiLink lacks an explicit return
type; define a specific return type (e.g., interface or type alias like
UseBubbleMenuWikiLinkReturn) describing the exact shape of the returned object
and annotate the function signature as export function
useBubbleMenuWikiLink(...): UseBubbleMenuWikiLinkReturn { ... }; ensure the
defined type avoids any usage, reflects all returned properties/methods, and is
exported if it is part of the public API so downstream consumers remain stable.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7da982af-845e-421f-a7db-e4f330f77744
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (30)
AGENTS.mdpackage.jsonpackages/shared/package.jsonpackages/shared/src/index.tspackages/shared/src/tagCharacterClass.test.tspackages/shared/src/tagCharacterClass.tspackages/shared/tsconfig.jsonpackages/shared/vitest.config.tsserver/api/src/__tests__/services/titleRenamePropagationService.test.tsserver/api/src/__tests__/services/ydocRenameRewrite.test.tsserver/api/src/services/titleRenamePropagationService.tsserver/api/src/services/ydocRenameRewrite.tssrc/components/editor/TiptapEditor/EditorBubbleMenu.test.tsxsrc/components/editor/TiptapEditor/useBubbleMenuWikiLink.test.tssrc/components/editor/TiptapEditor/useBubbleMenuWikiLink.tssrc/components/editor/TiptapEditor/useEditorBubbleMenu.test.tssrc/components/editor/TiptapEditor/useTagStatusSync.test.tsxsrc/components/editor/TiptapEditor/useTagStatusSync.tssrc/components/editor/TiptapEditor/useWikiLinkStatusSync.test.tsxsrc/components/editor/TiptapEditor/useWikiLinkStatusSync.tssrc/components/editor/extensions/TagExtension.test.tssrc/components/editor/extensions/TagExtension.tssrc/components/editor/extensions/WikiLinkExtension.test.tssrc/components/editor/extensions/WikiLinkExtension.tssrc/hooks/usePageQueries.tssrc/lib/tagCharacterClassSync.test.tssrc/lib/tagUtils.test.tssrc/lib/tagUtils.tssrc/lib/wikiLinkUtils.test.tssrc/lib/wikiLinkUtils.ts
- packages/shared/src/tagCharacterClass.ts: clarify the hiragana / katakana Unicode comment so it no longer reads as "前半/後半" of a single block; list the two distinct blocks (U+3040..U+309F / U+30A0..U+30FF) and note that `-ヿ` covers them contiguously. - src/components/editor/TiptapEditor/useBubbleMenuWikiLink.ts: add an explicit `UseBubbleMenuWikiLinkResult` return type to the exported hook so the public contract is no longer inferred (matches project rule on explicit type annotations). - server/api/src/services/ydocRenameRewrite.ts: add a comment justifying why `VALID_TAG_NAME_REGEX` is ReDoS-safe (constant char class, single quantifier, no nested alternations) to preempt static-analysis false positives. - server/api/src/__tests__/services/ydocRenameRewrite.test.ts: add tag parity tests for the two `renamedPageId`-omitted fallback branches that previously only covered wikiLink marks (skip-when-targetId-present and rewrite-when-id-less).
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
… TSDoc Address two CodeRabbit review comments on PR #738: 1. `rewriteTitleRefsInDoc` accepted a `fragmentName: string` as its 4th argument before issue #737 and now expects an options object. Any surviving caller passing a string would silently retarget the default fragment because `"foo".fragmentName === undefined`. Add a compatibility shim that normalizes a string 4th argument into `{ fragmentName }`. Two new tests lock in the legacy call form and guard against a regression that would silently rewrite the default fragment. 2. The TSDoc on `RewriteTitleRefsOptions.renamedPageId` claimed that omitting it falls back to title-only matching for *every* mark, but the implementation (and the issue #737 tests) skip marks that already carry a `targetId` because they cannot be safely verified. Update both the Japanese and English descriptions to match the real contract.
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 #737）を修正します。WikiLink と tag マークに
targetId属性を追加し、ID 一致による厳密なマッチングを実装しました。既存データとの後方互換性を保つため、targetIdを持たないマークはタイトル文字列でのフォールバック書き換えを継続します。変更点
コア実装
ydocRenameRewrite.ts: マッチング優先順位を実装targetIdを持つマークは ID 一致のみで書き換え（issue タイトル重複時の rename 伝播曖昧性とタグ文字クラス共有 (Phase 2 follow-up) #737 の核心）targetIdを持たないマークはタイトル文字列でフォールバック（lazy migration）TAG_NAME_CHAR_CLASS_STRINGを新規定義し、サーバ側の文字クラスをクライアント側と同期可能にtitleRenamePropagationService.ts:renamedPageIdをrewriteTitleRefsInDocに渡すように修正クライアント側マーク属性
WikiLinkExtension.ts:targetId属性を追加（既定値null、HTML ラウンドトリップ対応）TagExtension.ts:targetId属性を追加（既定値null、HTML ラウンドトリップ対応）属性値の埋め込み
wikiLinkUtils.ts:updateWikiLinkAttributesにpageTitleToIdマップを追加tagUtils.ts:updateTagAttributesにpageTitleToIdマップを追加useWikiLinkStatusSync.ts:collectWikiLinkUpdatesでtargetIdを埋め込みusePageQueries.ts:checkExistenceの戻り値にpageTitleToIdマップを追加useBubbleMenuWikiLink.ts: バブルメニューから WikiLink 変換時にtargetIdを埋め込み共有定数モジュール
packages/shared/: 新規作成tagCharacterClass.ts: タグ名の許容文字集合を定義（クライアント・サーバ間で同期）tagCharacterClass.test.ts: 文字集合の仕様テストindex.ts,package.json,tsconfig.json,vitest.config.ts: モジュール基盤src/lib/tagCharacterClassSync.test.ts: CI で@zedi/sharedとサーバ側の文字クラスが一致することを検証テスト
ydocRenameRewrite.test.ts: 新規テストスイート「targetId-based matching (issue タイトル重複時の rename 伝播曖昧性とタグ文字クラス共有 (Phase 2 follow-up) #737)」targetId無しマークのフォールバックtargetIdの扱いrenamedPageId省略時の安全な動作titleRenamePropagationService.test.ts: 同名リンク 2 つのシナリオテスト**`WikiLinkExt
https://claude.ai/code/session_01Mc7saJM6qTX4XZx3FzwvuN
Summary by CodeRabbit
New Features
Bug Fixes
Tests