fix(sync): content_preview / content_text から HTML タグを除去 - #498
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughYjs XMLテキストから平文を抽出する新しい Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 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 implements plain text extraction from Yjs XML fragments using the toDelta method to avoid HTML tags, updating both the server-side storage logic and the client-side collaboration manager. Feedback identifies a potential bug in CollaborationManager.ts where delta operations could cause unintended newlines and suggests trimming the extracted text for consistency.
| for (const op of node.toDelta()) { | ||
| if (typeof op.insert === "string") { | ||
| parts.push(op.insert); | ||
| } | ||
| } |
There was a problem hiding this comment.
node.toDelta() は、テキスト内の書式（太字、斜体など）が切り替わる箇所で複数の op を返します。現在の実装では、各 op.insert を個別に parts 配列に push していますが、最終的に parts.join("\n")（307行目）で連結されるため、書式の境界ごとに意図しない改行が挿入されてしまいます。
XmlText ノード内のテキストは、改行を挟まずに結合してから parts に追加することで、以前の toJSON() を使用していた時と同様の構造（1つの XmlText ノードにつき1つの parts 要素）を維持しつつ、HTML タグのみを除去できます。
let text = "";
for (const op of node.toDelta()) {
if (typeof op.insert === "string") {
text += op.insert;
}
}
parts.push(text);| }; | ||
|
|
||
| walk(fragment); | ||
| return parts.join(""); |
There was a problem hiding this comment.
クライアント側の CollaborationManager.extractText との整合性を保ち、検索インデックスや一覧プレビューでの表示品質を向上させるため、抽出したテキストの前後にある不要な空白や改行を除去（trim()）することを推奨します。
| return parts.join(""); | |
| return parts.join("").trim(); |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/lib/collaboration/CollaborationManager.ts (1)
285-307:⚠️ Potential issue | 🟠 Major
toDelta()使用により、同一Y.XmlText内に不要な改行が混入しています。Lines 289–293 で
toDelta()の各opごとにparts.pushし、Line 307 でparts.join("\n")しているため、"Hello " + "world" + "!"のように同一Y.XmlText内に複数の操作がある場合、Hello \nworld\n!になってしまいます。Y.XmlTextごとに内部の文字列を先に連結してからpartsに積む形に修正してください。修正案
private extractText(fragment: Y.XmlFragment): string { const parts: string[] = []; const walk = (node: Y.XmlFragment | Y.XmlElement | Y.XmlText) => { if (node instanceof Y.XmlText) { // toDelta() を使い書式属性なしの純粋なテキストのみを抽出する。 // toString() / toJSON() は <bold> 等の HTML タグを返すため使用しない。 // Use toDelta() to extract pure text without formatting attributes. // toString() / toJSON() return HTML-like tags (<bold>, etc.) so we avoid them. - for (const op of node.toDelta()) { - if (typeof op.insert === "string") { - parts.push(op.insert); - } - } + let text = ""; + for (const op of node.toDelta()) { + if (typeof op.insert === "string") { + text += op.insert; + } + } + if (text.length > 0) { + parts.push(text); + } } else { for (const child of node.toArray()) { if (🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/collaboration/CollaborationManager.ts` around lines 285 - 307, The current walk(fragment) logic pushes each op.insert from node.toDelta() individually causing intra-Y.XmlText fragments to become separated by "\n"; change the Y.XmlText/Y.XmlElement handling so that for each node (where you currently iterate for (const op of node.toDelta())), you first concatenate all string inserts from that single node into a temp string (e.g., accumulate op.insert values), then push that combined string once into parts; keep the existing behavior of walking child nodes and the final parts.join("\n").trim() unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@server/hocuspocus/src/index.ts`:
- Around line 206-223: The two separate queries updating page_contents and pages
must be executed atomically to avoid partial updates: wrap the INSERT/ON
CONFLICT into page_contents and the UPDATE of pages (setting content_preview) in
a single DB transaction (BEGIN ... COMMIT) using the same client, and ensure you
ROLLBACK on any error; reference the page_contents insert/ON CONFLICT statement
and the UPDATE pages SET content_preview operation that use pageId,
encodedState, contentText and contentPreview so both succeed or both are rolled
back.
---
Outside diff comments:
In `@src/lib/collaboration/CollaborationManager.ts`:
- Around line 285-307: The current walk(fragment) logic pushes each op.insert
from node.toDelta() individually causing intra-Y.XmlText fragments to become
separated by "\n"; change the Y.XmlText/Y.XmlElement handling so that for each
node (where you currently iterate for (const op of node.toDelta())), you first
concatenate all string inserts from that single node into a temp string (e.g.,
accumulate op.insert values), then push that combined string once into parts;
keep the existing behavior of walking child nodes and the final
parts.join("\n").trim() unchanged.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e0efb7ba-7008-4622-abd2-f0167ef17c06
📒 Files selected for processing (4)
server/hocuspocus/src/extractPlainTextFromYXml.test.tsserver/hocuspocus/src/extractPlainTextFromYXml.tsserver/hocuspocus/src/index.tssrc/lib/collaboration/CollaborationManager.ts
| await client.query( | ||
| ` | ||
| INSERT INTO page_contents (page_id, ydoc_state, version, content_text, updated_at) | ||
| VALUES ($1, $2, 1, '', NOW()) | ||
| VALUES ($1, $2, 1, $3, NOW()) | ||
| ON CONFLICT (page_id) DO UPDATE | ||
| SET ydoc_state = EXCLUDED.ydoc_state, | ||
| content_text = EXCLUDED.content_text, | ||
| version = page_contents.version + 1, | ||
| updated_at = NOW() | ||
| `, | ||
| [pageId, encodedState], | ||
| [pageId, encodedState, contentText], | ||
| ); | ||
| // content_preview は pages テーブルに格納 / content_preview is stored in the pages table | ||
| await client.query(`UPDATE pages SET content_preview = $2, updated_at = NOW() WHERE id = $1`, [ | ||
| pageId, | ||
| contentPreview, | ||
| ]); | ||
| } finally { |
There was a problem hiding this comment.
page_contents と pages 更新は同一トランザクションで行うべきです。
Line [206]-[223] は 2 クエリが分離しており、途中失敗で content_text だけ更新される不整合が起きます。保存処理は原子的にしてください。
💡 修正案
async function saveDocumentToDb(pageId: string, document: Y.Doc): Promise<void> {
@@
const client = await getPool().connect();
try {
+ await client.query("BEGIN");
await client.query(
@@
[pageId, encodedState, contentText],
);
@@
contentPreview,
]);
+ await client.query("COMMIT");
+ } catch (error) {
+ await client.query("ROLLBACK");
+ throw error;
} finally {
client.release();
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| await client.query( | |
| ` | |
| INSERT INTO page_contents (page_id, ydoc_state, version, content_text, updated_at) | |
| VALUES ($1, $2, 1, '', NOW()) | |
| VALUES ($1, $2, 1, $3, NOW()) | |
| ON CONFLICT (page_id) DO UPDATE | |
| SET ydoc_state = EXCLUDED.ydoc_state, | |
| content_text = EXCLUDED.content_text, | |
| version = page_contents.version + 1, | |
| updated_at = NOW() | |
| `, | |
| [pageId, encodedState], | |
| [pageId, encodedState, contentText], | |
| ); | |
| // content_preview は pages テーブルに格納 / content_preview is stored in the pages table | |
| await client.query(`UPDATE pages SET content_preview = $2, updated_at = NOW() WHERE id = $1`, [ | |
| pageId, | |
| contentPreview, | |
| ]); | |
| } finally { | |
| await client.query("BEGIN"); | |
| await client.query( | |
| ` | |
| INSERT INTO page_contents (page_id, ydoc_state, version, content_text, updated_at) | |
| VALUES ($1, $2, 1, $3, NOW()) | |
| ON CONFLICT (page_id) DO UPDATE | |
| SET ydoc_state = EXCLUDED.ydoc_state, | |
| content_text = EXCLUDED.content_text, | |
| version = page_contents.version + 1, | |
| updated_at = NOW() | |
| `, | |
| [pageId, encodedState, contentText], | |
| ); | |
| // content_preview は pages テーブルに格納 / content_preview is stored in the pages table | |
| await client.query(`UPDATE pages SET content_preview = $2, updated_at = NOW() WHERE id = $1`, [ | |
| pageId, | |
| contentPreview, | |
| ]); | |
| await client.query("COMMIT"); | |
| } catch (error) { | |
| await client.query("ROLLBACK"); | |
| throw error; | |
| } finally { |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@server/hocuspocus/src/index.ts` around lines 206 - 223, The two separate
queries updating page_contents and pages must be executed atomically to avoid
partial updates: wrap the INSERT/ON CONFLICT into page_contents and the UPDATE
of pages (setting content_preview) in a single DB transaction (BEGIN ... COMMIT)
using the same client, and ensure you ROLLBACK on any error; reference the
page_contents insert/ON CONFLICT statement and the UPDATE pages SET
content_preview operation that use pageId, encodedState, contentText and
contentPreview so both succeed or both are rolled back.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| for (const op of node.toDelta()) { | ||
| if (typeof op.insert === "string") { | ||
| parts.push(op.insert); | ||
| } | ||
| } | ||
| } else { | ||
| for (const child of node.toArray()) { | ||
| if ( |
There was a problem hiding this comment.
🔴 Client-side extractText inserts spurious newlines between formatted text runs
The old code pushed one string per XmlText node via node.toJSON(), so parts.join("\n") only added newlines between block-level text nodes (paragraphs). The new code pushes one string per delta operation (one per formatting run), but parts.join("\n") is unchanged. This means for text like "Hello world!", toDelta() produces 3 entries ("Hello ", "world", "!"), and the join produces "Hello \nworld\n!" instead of "Hello world!".
This corrupts content_text sent to the server in local-mode saves (used for full-text search at server/api/src/routes/search.ts:53), breaks content duplication detection (detectContentDuplication), and also affects the fireAndForgetSave on page unload. The server-side implementation at server/hocuspocus/src/extractPlainTextFromYXml.ts:45 correctly uses parts.join("") with explicit newline insertion between block elements.
(Refers to lines 289-307)
Prompt for agents
In CollaborationManager.ts, the extractText method at line 281 now pushes one string per delta op into `parts`, but still uses `parts.join("\n")` to concatenate. This inserts newlines between formatting runs within the same XmlText node (e.g. bold/italic text). The server-side implementation in extractPlainTextFromYXml.ts solves this correctly by using `parts.join("")` with explicit newline insertion between block elements.
Two approaches to fix:
1. Align the client implementation with the server-side one: use `parts.join("")` and insert explicit newlines between block-level elements (like XmlElement children that are not the last child).
2. Alternatively, import and reuse `extractTextFromYXml` from a shared location, though currently the server utility is in server/hocuspocus/src/ which isn't accessible from the frontend. Consider extracting the utility to a shared package or duplicating the correct algorithm.
The key change needed: within the XmlText branch, accumulate all delta inserts into a single string (or push them individually), but do NOT join all parts with "\n". Only add newlines between block-level elements.
Was this helpful? React with 👍 or 👎 to provide feedback.
…ntent_text Y.XmlText.toString() / toJSON() return XML-formatted strings with tags like <bold>, <italic> etc. This caused HTML tags to leak into content_preview (page list) and content_text (search index). Replace with toDelta() which returns raw insert strings without formatting attributes. Also add extractTextFromYXml utility to hocuspocus server so it persists content_text and content_preview on document save. Closes #497 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
aa05f19 to
98e967f
Compare
1 participant
Summary
Y.XmlText.toString()/toJSON()が<bold>等の HTML タグを返す問題を修正CollaborationManager.extractText()でtoDelta()を使用し、書式属性なしの純粋なテキストのみを抽出するように変更extractTextFromYXmlユーティリティを追加し、ドキュメント保存時にcontent_textとcontent_previewを正しく永続化Changes
src/lib/collaboration/CollaborationManager.ts:node.toJSON()→node.toDelta()ベースに変更server/hocuspocus/src/extractPlainTextFromYXml.ts(新規): Y.XmlFragment から再帰的にプレーンテキストを抽出するユーティリティserver/hocuspocus/src/index.ts:saveDocumentToDbでcontent_text（検索用）とcontent_preview（一覧表示用）を抽出・保存server/hocuspocus/src/extractPlainTextFromYXml.test.ts(新規): 6 テストケース（空 fragment、単純テキスト、複数パラグラフ、bold/italic ストリップ、ネスト要素）Test plan
extractPlainTextFromYXml.test.ts— 6 テスト全通過CollaborationManager.test.ts— 7 テスト全通過Closes #497
🤖 Generated with Claude Code
Summary by CodeRabbit
リリースノート
新機能
バグ修正
テスト