fix: address PR #757 review comments - #759
Merged
Merged
Conversation
Made-with: Cursor
📝 WalkthroughWalkthroughThis PR contains targeted improvements across multiple areas: fixing content preview truncation logic to respect max length constraints with ellipsis appending, strengthening HTML parsing validation for wiki-link and tag Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 |
![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 updates documentation for database migrations and refines the buildContentPreview utility to strictly enforce length limits. It also improves Tiptap editor extensions by handling empty targetId attributes and refactoring test contexts. Furthermore, numerous test files were updated with bilingual comments and enhanced mock behavior. Feedback highlights the need for safer string slicing to handle surrogate pairs in previews and recommends closing stream controllers immediately upon abortion in tests to avoid potential hangs.
| if (trimmed.length <= CONTENT_PREVIEW_MAX_LENGTH) return trimmed; | ||
| return trimmed.slice(0, CONTENT_PREVIEW_MAX_LENGTH).trim() + "..."; | ||
| const headLength = Math.max(0, CONTENT_PREVIEW_MAX_LENGTH - 3); | ||
| return `${trimmed.slice(0, headLength).trim()}...`; |
There was a problem hiding this comment.
trimmed.slice(0, headLength) は UTF-16 コードユニット単位で文字列を切り出すため、サロゲートペア（絵文字や一部の漢字など）の途中で切断され、不正な文字列（文字化け）が発生する可能性があります。プレビュー用途では致命的ではありませんが、より堅牢にするには Intl.Segmenter を使用するか、切り出し位置がサロゲートペアの間でないかを確認する処理を追加することを検討してください。なお、この修正を行う場合は server/hocuspocus 側の同名ファイルも同様に更新する必要があります。
| if (trimmed.length <= CONTENT_PREVIEW_MAX_LENGTH) return trimmed; | ||
| return trimmed.slice(0, CONTENT_PREVIEW_MAX_LENGTH).trim() + "..."; | ||
| const headLength = Math.max(0, CONTENT_PREVIEW_MAX_LENGTH - 3); | ||
| return `${trimmed.slice(0, headLength).trim()}...`; |
There was a problem hiding this comment.
trimmed.slice(0, headLength) は UTF-16 コードユニット単位で文字列を切り出すため、サロゲートペア（絵文字や一部の漢字など）の途中で切断され、不正な文字列（文字化け）が発生する可能性があります。プレビュー用途では致命的ではありませんが、より堅牢にするには Intl.Segmenter を使用するか、切り出し位置がサロゲートペアの間でないかを確認する処理を追加することを検討してください。なお、この修正を行う場合は server/api 側の同名ファイルも同様に更新する必要があります。
Comment on lines
+361
to
363
| // Abort before the next pull. | ||
| abortController.abort(); | ||
| return; |
There was a problem hiding this comment.
この pull メソッド内での abort() 呼び出し直後に controller.close() を実行することを検討してください。現在の実装では、2 回目の pull が呼ばれた際に close() されるようになっていますが、コンシューマ（callAIWithServer）が abort を検知してストリームの読み取りを即座に停止した場合、2 回目の pull が発生せず、ストリームが閉じられないままハングやリークの原因になる可能性があります。この PR の目的である「ハング防止」をより確実にするための修正です。
// Abort before the next pull.
abortController.abort();
controller.close();
return;![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (7)
server/hocuspocus/src/extractPlainTextFromYXml.test.ts (1)
143-143: Use the exported max-length constant in the expectation.Line 143 hardcodes
120; importingCONTENT_PREVIEW_MAX_LENGTHkeeps spec and implementation aligned if the limit changes.Proposed refactor
-import { buildContentPreview, extractTextFromYXml } from "./extractPlainTextFromYXml.js"; +import { + buildContentPreview, + CONTENT_PREVIEW_MAX_LENGTH, + extractTextFromYXml, +} from "./extractPlainTextFromYXml.js"; @@ - expect(prev.length).toBeLessThanOrEqual(120); + expect(prev.length).toBeLessThanOrEqual(CONTENT_PREVIEW_MAX_LENGTH);As per coding guidelines
**/*.{test,spec}.{ts,tsx,js,jsx}: 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/hocuspocus/src/extractPlainTextFromYXml.test.ts` at line 143, The test currently hardcodes the preview length check with expect(prev.length).toBeLessThanOrEqual(120); — replace the literal with the exported constant CONTENT_PREVIEW_MAX_LENGTH by importing it into the test file and using expect(prev.length).toBeLessThanOrEqual(CONTENT_PREVIEW_MAX_LENGTH); so the spec follows the implementation constant (ensure the import name matches the exported symbol).server/api/src/lib/extractPlainTextFromYXml.ts (1)
76-77: Mirror-side maintainability: avoid magic ellipsis values here too.Line 76/Line 77 should follow the same suffix-constant pattern to avoid drift between this mirrored file and the Hocuspocus copy.
Proposed refactor
+const PREVIEW_ELLIPSIS = "..."; + export function buildContentPreview(text: string): string { const trimmed = text.trim().replace(/\s+/g, " "); if (trimmed.length <= CONTENT_PREVIEW_MAX_LENGTH) return trimmed; - const headLength = Math.max(0, CONTENT_PREVIEW_MAX_LENGTH - 3); - return `${trimmed.slice(0, headLength).trim()}...`; + const headLength = Math.max(0, CONTENT_PREVIEW_MAX_LENGTH - PREVIEW_ELLIPSIS.length); + return `${trimmed.slice(0, headLength).trim()}${PREVIEW_ELLIPSIS}`; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/api/src/lib/extractPlainTextFromYXml.ts` around lines 76 - 77, Replace the magic ellipsis string ("...") in the return of the function that computes preview text with the shared suffix constant used elsewhere to avoid drift; locate the logic around CONTENT_PREVIEW_MAX_LENGTH and headLength in extractPlainTextFromYXml.ts and return the trimmed slice concatenated with the existing SUFFIX constant (or a similarly named PREVIEW_SUFFIX used in the Hocuspocus copy) instead of the literal "..." so both mirrored implementations use the same suffix symbol.server/hocuspocus/src/extractPlainTextFromYXml.ts (1)
75-76: Avoid duplicated magic values for ellipsis truncation.Line 75/Line 76 hardcode both
3and"...". Derive head length from a single suffix constant so future changes can’t desync behavior.Proposed refactor
+const PREVIEW_ELLIPSIS = "..."; + export function buildContentPreview(text: string): string { const trimmed = text.trim().replace(/\s+/g, " "); if (trimmed.length <= CONTENT_PREVIEW_MAX_LENGTH) return trimmed; - const headLength = Math.max(0, CONTENT_PREVIEW_MAX_LENGTH - 3); - return `${trimmed.slice(0, headLength).trim()}...`; + const headLength = Math.max(0, CONTENT_PREVIEW_MAX_LENGTH - PREVIEW_ELLIPSIS.length); + return `${trimmed.slice(0, headLength).trim()}${PREVIEW_ELLIPSIS}`; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/hocuspocus/src/extractPlainTextFromYXml.ts` around lines 75 - 76, The truncation logic in extractPlainTextFromYXml.ts uses two hardcoded values (3 and "...") which can drift; introduce a single suffix constant (e.g., const ELLIPSIS = "..." or const SUFFIX = "...") and derive the head length from CONTENT_PREVIEW_MAX_LENGTH - ELLIPSIS.length, then use that suffix constant in the template return instead of the literal "..." (update the variables headLength and the return string in the function that computes the preview).src/components/editor/extensions/TagExtension.ts (1)
170-182: Apply the sametargetIdnormalization inrenderHTMLto match parse behavior.
parseHTMLtrims and null-normalizes, butrenderHTMLcan still serialize whitespace-only strings. Mirroring normalization avoids writing non-canonicaldata-target-idvalues.♻️ Proposed fix
targetId: { default: null, parseHTML: (element) => { const raw = element.getAttribute("data-target-id"); if (typeof raw !== "string") return null; const normalized = raw.trim(); return normalized.length > 0 ? normalized : null; }, renderHTML: (attributes) => { const value = attributes.targetId; - if (typeof value !== "string" || value.length === 0) { + if (typeof value !== "string") { return {}; } - return { "data-target-id": value }; + const normalized = value.trim(); + if (normalized.length === 0) { + return {}; + } + return { "data-target-id": normalized }; }, },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/editor/extensions/TagExtension.ts` around lines 170 - 182, The renderHTML in TagExtension currently serializes attributes.targetId verbatim; update renderHTML to mirror parseHTML by first checking typeof attributes.targetId === "string", then creating a normalized = attributes.targetId.trim(), and only return { "data-target-id": normalized } when normalized.length > 0 (otherwise return {}). This keeps TagExtension.renderHTML behavior consistent with parseHTML and avoids writing whitespace-only data-target-id values.src/components/editor/extensions/WikiLinkExtension.ts (1)
121-133: NormalizetargetIdon render as well to keep serialization canonical.
parseHTMLtrims whitespace-only values tonull, butrenderHTMLstill emits whitespace strings. Trimming inrenderHTMLtoo keeps round-trip behavior consistent.♻️ Proposed fix
targetId: { default: null, parseHTML: (element) => { const raw = element.getAttribute("data-target-id"); if (typeof raw !== "string") return null; const normalized = raw.trim(); return normalized.length > 0 ? normalized : null; }, renderHTML: (attributes) => { const value = attributes.targetId; - if (typeof value !== "string" || value.length === 0) { + if (typeof value !== "string") { return {}; } - return { "data-target-id": value }; + const normalized = value.trim(); + if (normalized.length === 0) { + return {}; + } + return { "data-target-id": normalized }; }, },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/editor/extensions/WikiLinkExtension.ts` around lines 121 - 133, The renderHTML logic for WikiLinkExtension should mirror parseHTML by normalizing attributes.targetId before serializing: trim the string and treat empty/whitespace-only results as absent; in renderHTML (which currently reads attributes.targetId) call .trim(), check length > 0, and only return { "data-target-id": trimmed } when non-empty, otherwise return {} so serialization round-trips consistently with parseHTML.src/components/editor/extensions/TagExtension.test.ts (1)
246-251: Consider asserting whitespace-onlydata-target-idnormalization too.You already cover
""; adding" "would fully lock the trim-to-nullbehavior introduced in the parser.✅ Suggested test addition
const empty = document.createElement("span"); expect(spec.parseHTML(empty)).toBeNull(); empty.setAttribute("data-target-id", ""); expect(spec.parseHTML(empty)).toBeNull(); + + empty.setAttribute("data-target-id", " "); + expect(spec.parseHTML(empty)).toBeNull();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/editor/extensions/TagExtension.test.ts` around lines 246 - 251, Add a test case to assert that spec.parseHTML trims whitespace-only values for the data-target-id attribute and returns null for them; specifically, after creating the empty span and before finishing the current test, set empty.setAttribute("data-target-id", " ") and expect(spec.parseHTML(empty)).toBeNull() to verify the trim-to-null normalization implemented in parseHTML.src/components/editor/extensions/WikiLinkExtension.test.ts (1)
134-139: Add a whitespace-onlydata-target-idparse assertion.
parseHTMLnow trims values, so pinning" "→nullhere would protect against regressions beyond just"".✅ Suggested test addition
const empty = document.createElement("span"); expect(spec.parseHTML(empty)).toBeNull(); empty.setAttribute("data-target-id", ""); expect(spec.parseHTML(empty)).toBeNull(); + + empty.setAttribute("data-target-id", " "); + expect(spec.parseHTML(empty)).toBeNull();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/editor/extensions/WikiLinkExtension.test.ts` around lines 134 - 139, The test currently asserts that an empty data-target-id string returns null; add an assertion that a whitespace-only value also returns null to guard against regressions in trimming logic: set the span's data-target-id to a string of spaces (e.g., " ") and call spec.parseHTML(empty) expect it toBeNull(). Refer to the existing test variable empty and the spec.parseHTML function in WikiLinkExtension.test.ts when adding this assertion.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@server/api/src/lib/extractPlainTextFromYXml.ts`:
- Around line 76-77: Replace the magic ellipsis string ("...") in the return of
the function that computes preview text with the shared suffix constant used
elsewhere to avoid drift; locate the logic around CONTENT_PREVIEW_MAX_LENGTH and
headLength in extractPlainTextFromYXml.ts and return the trimmed slice
concatenated with the existing SUFFIX constant (or a similarly named
PREVIEW_SUFFIX used in the Hocuspocus copy) instead of the literal "..." so both
mirrored implementations use the same suffix symbol.
In `@server/hocuspocus/src/extractPlainTextFromYXml.test.ts`:
- Line 143: The test currently hardcodes the preview length check with
expect(prev.length).toBeLessThanOrEqual(120); — replace the literal with the
exported constant CONTENT_PREVIEW_MAX_LENGTH by importing it into the test file
and using expect(prev.length).toBeLessThanOrEqual(CONTENT_PREVIEW_MAX_LENGTH);
so the spec follows the implementation constant (ensure the import name matches
the exported symbol).
In `@server/hocuspocus/src/extractPlainTextFromYXml.ts`:
- Around line 75-76: The truncation logic in extractPlainTextFromYXml.ts uses
two hardcoded values (3 and "...") which can drift; introduce a single suffix
constant (e.g., const ELLIPSIS = "..." or const SUFFIX = "...") and derive the
head length from CONTENT_PREVIEW_MAX_LENGTH - ELLIPSIS.length, then use that
suffix constant in the template return instead of the literal "..." (update the
variables headLength and the return string in the function that computes the
preview).
In `@src/components/editor/extensions/TagExtension.test.ts`:
- Around line 246-251: Add a test case to assert that spec.parseHTML trims
whitespace-only values for the data-target-id attribute and returns null for
them; specifically, after creating the empty span and before finishing the
current test, set empty.setAttribute("data-target-id", " ") and
expect(spec.parseHTML(empty)).toBeNull() to verify the trim-to-null
normalization implemented in parseHTML.
In `@src/components/editor/extensions/TagExtension.ts`:
- Around line 170-182: The renderHTML in TagExtension currently serializes
attributes.targetId verbatim; update renderHTML to mirror parseHTML by first
checking typeof attributes.targetId === "string", then creating a normalized =
attributes.targetId.trim(), and only return { "data-target-id": normalized }
when normalized.length > 0 (otherwise return {}). This keeps
TagExtension.renderHTML behavior consistent with parseHTML and avoids writing
whitespace-only data-target-id values.
In `@src/components/editor/extensions/WikiLinkExtension.test.ts`:
- Around line 134-139: The test currently asserts that an empty data-target-id
string returns null; add an assertion that a whitespace-only value also returns
null to guard against regressions in trimming logic: set the span's
data-target-id to a string of spaces (e.g., " ") and call spec.parseHTML(empty)
expect it toBeNull(). Refer to the existing test variable empty and the
spec.parseHTML function in WikiLinkExtension.test.ts when adding this assertion.
In `@src/components/editor/extensions/WikiLinkExtension.ts`:
- Around line 121-133: The renderHTML logic for WikiLinkExtension should mirror
parseHTML by normalizing attributes.targetId before serializing: trim the string
and treat empty/whitespace-only results as absent; in renderHTML (which
currently reads attributes.targetId) call .trim(), check length > 0, and only
return { "data-target-id": trimmed } when non-empty, otherwise return {} so
serialization round-trips consistently with parseHTML.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 573367c1-ed70-4333-a386-7c9f09bfeecb
📒 Files selected for processing (15)
AGENTS.mdserver/api/src/lib/extractPlainTextFromYXml.tsserver/hocuspocus/src/extractPlainTextFromYXml.test.tsserver/hocuspocus/src/extractPlainTextFromYXml.tssrc/components/editor/TiptapEditor/useWikiLinkStatusSync.test.tsxsrc/components/editor/extensions/TagExtension.test.tssrc/components/editor/extensions/TagExtension.tssrc/components/editor/extensions/WikiLinkExtension.test.tssrc/components/editor/extensions/WikiLinkExtension.tssrc/components/editor/extensions/slashSuggestionPlugin.test.tssrc/hooks/useMermaidGenerator.test.tssrc/hooks/useWorkflowDraft.test.tssrc/hooks/useWorkflowRunSession.test.tssrc/lib/aiServiceServer.test.tssrc/stores/aiChatStore.test.ts
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
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
1 participant
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.
概要
PR #757 のレビューコメントのうち、リリース前に取り込むべきバグ修正・型安全性・テスト安定化・コメント整備をまとめて対応します。
変更点
buildContentPreviewが最大長を超えないよう、ellipsis 分を含めて切り詰めるよう修正WikiLink/Tagのdata-target-idパースで空文字をnullに正規化し、対応テストを追加addAttributes.call()周辺のas anyを明示的な context 型へ置換ReadableStreamを明示的に close してハングを防止変更の種類
テスト方法
bun run lintbunx prettier --check AGENTS.md server/api/src/lib/extractPlainTextFromYXml.ts server/hocuspocus/src/extractPlainTextFromYXml.ts server/hocuspocus/src/extractPlainTextFromYXml.test.ts src/components/editor/TiptapEditor/useWikiLinkStatusSync.test.tsx src/components/editor/extensions/TagExtension.ts src/components/editor/extensions/TagExtension.test.ts src/components/editor/extensions/WikiLinkExtension.ts src/components/editor/extensions/WikiLinkExtension.test.ts src/components/editor/extensions/slashSuggestionPlugin.test.ts src/hooks/useMermaidGenerator.test.ts src/hooks/useWorkflowDraft.test.ts src/hooks/useWorkflowRunSession.test.ts src/lib/aiServiceServer.test.ts src/stores/aiChatStore.test.tsbunx vitest run src/components/editor/extensions/WikiLinkExtension.test.ts src/components/editor/extensions/TagExtension.test.ts src/components/editor/extensions/slashSuggestionPlugin.test.ts src/components/editor/TiptapEditor/useWikiLinkStatusSync.test.tsx src/hooks/useMermaidGenerator.test.ts src/hooks/useWorkflowDraft.test.ts src/hooks/useWorkflowRunSession.test.ts src/lib/aiServiceServer.test.ts src/stores/aiChatStore.test.ts(cd server/hocuspocus && bunx vitest run src/extractPlainTextFromYXml.test.ts)Note:
bun run format:checkはリポジトリ既存の未整形ファイル多数により失敗するため、今回変更したファイル限定で Prettier check を実行しています。チェックリスト
スクリーンショット（UI 変更がある場合）
UI 変更なし。
関連 Issue
Related to #757
Made with Cursor
Summary by CodeRabbit
Bug Fixes
Documentation