Repository navigation
fix(docs): reject container text writes and preserve table-cell text #6468
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
6ccac5e
902083d
86dec9a
313144f
53f76ea
1bd7111
032fe42
2031a29
a59591f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,113 @@ | ||
| import { $getId } from '@macro-inc/lexical-core/plugins/nodeIdPlugin'; | ||
| import { $getRoot, $isElementNode } from 'lexical'; | ||
| import { describe, expect, it, vi } from 'vitest'; | ||
| import { read, setup } from '../ai-toolkit/_test-helpers'; | ||
| import { type DocumentOp, EditError } from '../editor'; | ||
| import { Doc } from './doc'; | ||
|
|
||
| const textWrites: DocumentOp[] = [ | ||
| { kind: 'setText', node: '', text: 'X' }, | ||
| { kind: 'appendText', node: '', text: 'X' }, | ||
| { kind: 'prependText', node: '', text: 'X' }, | ||
| { kind: 'insertText', node: '', at: 0, text: 'X' }, | ||
| { | ||
| kind: 'insertInline', | ||
| node: '', | ||
| at: 0, | ||
| ref: 'break', | ||
| spec: { inline: 'linebreak' }, | ||
| }, | ||
| { kind: 'setBlockType', node: '', block: 'heading', level: 2 }, | ||
| ]; | ||
|
|
||
| function tableFixture(text = 'original') { | ||
| const { session, ids } = setup('intro\n\n- one\n- two'); | ||
| const propagate = vi.fn(); | ||
| const doc = new Doc(session, propagate); | ||
| doc.apply({ | ||
| kind: 'insertNode', | ||
| ref: 'table', | ||
| spec: { block: 'table', rows: [[text]] }, | ||
| at: { after: ids[0]! }, | ||
| }); | ||
| const targets = read(session, () => { | ||
| const table = $getRoot() | ||
| .getChildren() | ||
| .find((n) => n.getType() === 'table'); | ||
| if (!$isElementNode(table)) throw new Error('missing table'); | ||
| const row = table.getFirstChild(); | ||
| if (!$isElementNode(row)) throw new Error('missing row'); | ||
| const cell = row.getFirstChild(); | ||
| if (!$isElementNode(cell)) throw new Error('missing cell'); | ||
| const paragraph = cell.getFirstChild()!; | ||
| const list = $getRoot() | ||
| .getChildren() | ||
| .find((n) => n.getType() === 'list'); | ||
| if (!$isElementNode(list)) throw new Error('missing list'); | ||
| return { | ||
| table: 'table', | ||
| row: $getId(row)!, | ||
| cell: $getId(cell)!, | ||
| list: $getId(list)!, | ||
| paragraph: $getId(paragraph)!, | ||
| item: $getId(list.getFirstChild()!)!, | ||
| }; | ||
| }); | ||
| propagate.mockClear(); | ||
| return { session, doc, targets, propagate, intro: ids[0]! }; | ||
| } | ||
|
|
||
| describe('text writers reject container ids before changing content', () => { | ||
| for (const target of ['table', 'row', 'cell', 'list'] as const) { | ||
| for (const op of textWrites) { | ||
| it(`${op.kind} rejects a ${target} id, including when empty`, () => { | ||
| for (const text of ['original', '']) { | ||
| const { session, doc, targets, propagate } = tableFixture(text); | ||
| const before = session.editor.getEditorState().toJSON(); | ||
| const apply = () => | ||
| doc.apply({ ...op, node: targets[target] } as DocumentOp); | ||
| expect(apply).toThrow(EditError); | ||
| expect(apply).toThrow('target a paragraph or list item id'); | ||
| expect(session.editor.getEditorState().toJSON()).toEqual(before); | ||
| expect(propagate).not.toHaveBeenCalled(); | ||
| } | ||
| }); | ||
| } | ||
| } | ||
|
|
||
| for (const target of ['paragraph', 'item'] as const) { | ||
| for (const op of textWrites) { | ||
| it(`${op.kind} still accepts a ${target} id`, () => { | ||
| const { doc, targets, propagate } = tableFixture(); | ||
| expect(() => | ||
| doc.apply({ ...op, node: targets[target] } as DocumentOp) | ||
| ).not.toThrow(); | ||
| expect(propagate).toHaveBeenCalledOnce(); | ||
| }); | ||
| } | ||
| } | ||
|
|
||
| it('keeps setCell, container search/replace, and table moves working', () => { | ||
| const { session, doc, targets, intro } = tableFixture(); | ||
| doc.apply({ | ||
| kind: 'setCell', | ||
| table: targets.table, | ||
| row: 0, | ||
| col: 0, | ||
| text: 'updated', | ||
| }); | ||
| expect(doc.locate(targets.cell, 'updated')).toHaveLength(1); | ||
| doc.apply({ | ||
| kind: 'replaceText', | ||
| node: targets.table, | ||
| find: 'updated', | ||
| to: 'replaced', | ||
| scope: { kind: 'all' }, | ||
| }); | ||
| doc.apply({ kind: 'moveNode', node: targets.table, at: { before: intro } }); | ||
| read(session, () => { | ||
| expect($getRoot().getFirstChild()?.getType()).toBe('table'); | ||
| expect($getRoot().getTextContent()).toContain('replaced'); | ||
| }); | ||
| }); | ||
| }); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -237,6 +237,24 @@ export class Doc implements DocReader, DocWriter { | |
| return locate.$blockById(this.session, node); | ||
| } | ||
|
|
||
| /** Text writers need a content block, not a table/list container. Reject | ||
| * ambiguous container ids instead of choosing one of their descendants. */ | ||
| private textBlock(id: NodeRef): ElementNode { | ||
| const block = this.block(id); | ||
| if ( | ||
| $isTableNode(block) || | ||
| $isTableRowNode(block) || | ||
| $isTableCellNode(block) || | ||
| $isListNode(block) || | ||
| block.getType() === 'root' | ||
| ) { | ||
| throw new EditError( | ||
| `id "${id}" is a ${block.getType()} container — target a paragraph or list item id; use setCell to replace a table cell.` | ||
| ); | ||
| } | ||
| return block; | ||
| } | ||
|
|
||
| /** Resolve a table by id, rejecting non-table ids. `setCell`/cell resolution | ||
| * otherwise walk up to the enclosing table, so a <tr>/<td> id "works" but | ||
| * reindexes row/col against the table — silently clobbering the wrong cell | ||
|
|
@@ -293,7 +311,7 @@ export class Doc implements DocReader, DocWriter { | |
| const c = target.getTextContent(); | ||
| target.setTextContent(c.slice(0, at) + text + c.slice(at)); | ||
| } else { | ||
| insertTextAt(this.block(node), at, text); | ||
| insertTextAt(this.textBlock(node), at, text); | ||
| } | ||
| }); | ||
| } | ||
|
|
@@ -332,16 +350,16 @@ export class Doc implements DocReader, DocWriter { | |
| else target.setTextContent(text); | ||
| return; | ||
| } | ||
| blocks.$setText(this.block(node), text); | ||
| blocks.$setText(this.textBlock(node), text); | ||
| }); | ||
| } | ||
|
|
||
| private appendText(node: NodeRef, text: string): void { | ||
| this.tx(() => inline.$appendText(this.block(node), text)); | ||
| this.tx(() => inline.$appendText(this.textBlock(node), text)); | ||
| } | ||
|
|
||
| private prependText(node: NodeRef, text: string): void { | ||
| this.tx(() => inline.$prependText(this.block(node), text)); | ||
| this.tx(() => inline.$prependText(this.textBlock(node), text)); | ||
| } | ||
|
|
||
| private insertTextAfterInline( | ||
|
|
@@ -489,12 +507,13 @@ export class Doc implements DocReader, DocWriter { | |
| : block === 'code' | ||
| ? { type: 'code', language: opts.language } | ||
| : { type: block }; | ||
| this.tx(() => | ||
| this.tx(() => { | ||
| this.textBlock(node); | ||
| modify.$modifyNode(this.session, node, { | ||
| op: 'blockType', | ||
| block: data, | ||
| }) | ||
| ); | ||
| }); | ||
| }); | ||
| } | ||
|
|
||
| private setListType(nodes: NodeRef[], list: ListKind): void { | ||
|
|
@@ -586,7 +605,7 @@ export class Doc implements DocReader, DocWriter { | |
| spec: NodeSpec | ||
| ): void { | ||
| this.tx(() => { | ||
| const block = this.block(node); | ||
| const block = this.textBlock(node); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. mergeBlocks still writes bare cell textMedium Severity
Additional Locations (1)Reviewed by Cursor Bugbot for commit a59591f. Configure here. |
||
| const inline = buildNode(spec); | ||
| insertInlineAt(block, at, inline); | ||
| this.assignRef(ref, inline); | ||
|
|
||


There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
setText on containers deletes content
High Severity
textBlockrejects table, row, cell, and list ids forsetTextandinsertText, butremoveTextstill goes throughthis.block(). The live queue expandssetTextviaretypeintoremoveTexttheninsertText. A cell or table id therefore clears every descendant text node and syncs that wipe, theninsertTextthrows and the replacement never lands. Directdoc.apply(setText)tests miss this path.Additional Locations (1)
services/ai-editing-worker/src/ai-editing/doc/doc.ts#L241-L256Reviewed by Cursor Bugbot for commit a59591f. Configure here.