feat(word-diff): implement word-level diffing for granular text changes#2817
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 38564682d3
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| const textNode = editor.state.schema.text(trimmedNew, asProseMirrorMarks(marks)); | ||
| tr.replaceWith(trimmedFrom, trimmedTo, textNode); |
There was a problem hiding this comment.
Avoid creating empty text nodes for delete-only rewrites
In executeTextRewrite, the single-change path always does schema.text(trimmedNew) even when the trimmed diff is a pure deletion. For inputs like removing a word ("The quick fox" → "The fox"), trimmedNew becomes "", and ProseMirror rejects empty text nodes at runtime, aborting the rewrite instead of applying the delete. This regression is introduced by the new prefix/suffix trimming flow and will surface on common delete-only edits.
Useful? React with 👍 / 👎.
| const segments = this.collectTextSegments(from, to); | ||
| const nodes = this.buildTextNodes(from, to, change.newText, segments); |
There was a problem hiding this comment.
Read replacement marks from transaction doc after remapping
applyGranularChanges remaps later change positions through prior tr.steps, but then calls collectTextSegments(from, to) on this.editor.state.doc (the original document). After an earlier insert/delete changes length, those remapped positions no longer point to the same content in the original doc, so subsequent replacements can inherit marks from the wrong region (or none), causing formatting loss/corruption in multi-change tracked edits.
Useful? React with 👍 / 👎.
|
🎉 This PR is included in vscode-ext v2.3.0-next.18 |
|
🎉 This PR is included in @superdoc-dev/react v1.2.0-next.15 The release is available on GitHub release |
|
🎉 This PR is included in template-builder v1.5.0-next.18 The release is available on GitHub release |
|
🎉 This PR is included in esign v2.3.0-next.18 The release is available on GitHub release |
|
🎉 This PR is included in superdoc v1.26.0-next.18 The release is available on GitHub release |
|
🎉 This PR is included in superdoc-cli v0.7.0-next.19 The release is available on GitHub release |
|
🎉 This PR is included in superdoc-sdk v1.6.0-next.16 |
The word-diff executor added in #2817 calls `tr.doc.textBetween(...)` and `tr.doc.nodesBetween(...)` in the text.rewrite path, but the `tr.doc` mocks in determinism-stress, executor, and preview-parity tests were never updated to match. That broke determinism-stress, executeCompiledPlan, previewPlan, and revision-drift-guard tests on main with `TypeError: tr.doc.textBetween is not a function` / `doc.nodesBetween is not a function`.
* test(superdoc): add unit tests for pdf, comments, and pdf-viewer Adds test coverage for previously untested files: - core/pdf/pdf-adapter.js — factory, adapter, CDN worker helper - components/CommentsLayer/use-conversation.js — conversation composable - components/CommentsLayer/use-floating-comment.js — floating comment state - components/CommentsLayer/CommentGroup.vue — grouped comment rendering - components/PdfViewer/PdfViewer.vue — top-level viewer with adapter mocks - components/PdfViewer/PdfViewerDocument.vue — page list wrapper - components/PdfViewer/PdfViewerPage.vue — page render + text layer 62 new tests. Lifts filtered coverage from 78.6% to 81.1%. * test(superdoc): add tests for composables and comments list - composables/use-field.js — useField + sub-handlers (SELECT/IMAGE/CHECKBOX) - composables/use-ai.js — AI writer cursor positioning, fallbacks, tool click - components/CommentsLayer/commentsList/super-comments-list.js — Vue app lifecycle - components/Whiteboard/use-whiteboard.js — event wiring, page ready, teardown 46 new tests. Lifts filtered coverage 81.1% → 83.3%. * test(superdoc): cover WhiteboardPage canvas interactions Mock Konva Stage/Layer/Line/Text/Image/Transformer and trigger stage/text-node event listeners to exercise the interactive code paths: draw/erase flows, stage click handlers (text + select), text node selection + drag + Delete, image node events. 38 tests. Lifts WhiteboardPage coverage 40% → 81%, package 83.3% → 88.5%. * fix(test): add textBetween/nodesBetween to plan-engine tr.doc mocks The word-diff executor added in #2817 calls `tr.doc.textBetween(...)` and `tr.doc.nodesBetween(...)` in the text.rewrite path, but the `tr.doc` mocks in determinism-stress, executor, and preview-parity tests were never updated to match. That broke determinism-stress, executeCompiledPlan, previewPlan, and revision-drift-guard tests on main with `TypeError: tr.doc.textBetween is not a function` / `doc.nodesBetween is not a function`. * test(superdoc): push coverage past 90% Add targeted tests to close remaining gaps: - use-comment extended — resolveComment, setIsInternal, setText, mentions - superdoc-store extended — reset, getDocument, handlePageReady, getPageBounds, areDocumentsReady, setExceptionHandler, collaboration mode - Whiteboard extended — tool/enabled propagation, opacity clamping, callbacks - New small files: comment-schemas, DocumentUsers.vue, SuperToolbar.vue, use-selection, CommentsLayer/helpers formatDate Lifts package coverage from 83.3% to 90.1%.
PR #2817 added word-level diffing so multi-word tracked replacements fragment into per-word insert/delete pairs — "a tracked style" → "new fancy" becomes two tracked changes ("a" → "new" around a preserved space, then "tracked style" → "fancy"). The `getDocumentText()` assertion in programmatic-tracked-change.spec.ts still expected the pre-word-diff behaviour where "new fancy" appeared as one contiguous string, which produced this on all three browsers: Expected substring: "new fancy" Received string: "Here is anew tracked stylefancy change" Assert the inserted words "new" and "fancy" each appear in the document text instead, and leave a comment explaining the new semantics so the next reader doesn't have to retrace this. Ran tests/comments/programmatic-tracked-change.spec.ts in chromium locally — all 5 cases pass.
…or (#2851) PR #2817 added word-level diffing so multi-word tracked replacements fragment into per-word insert/delete pairs — "a tracked style" → "new fancy" becomes two tracked changes ("a" → "new" around a preserved space, then "tracked style" → "fancy"). The `getDocumentText()` assertion in programmatic-tracked-change.spec.ts still expected the pre-word-diff behaviour where "new fancy" appeared as one contiguous string, which produced this on all three browsers: Expected substring: "new fancy" Received string: "Here is anew tracked stylefancy change" Assert the inserted words "new" and "fancy" each appear in the document text instead, and leave a comment explaining the new semantics so the next reader doesn't have to retrace this. Ran tests/comments/programmatic-tracked-change.spec.ts in chromium locally — all 5 cases pass.
|
🎉 This PR is included in superdoc-cli v0.7.0 The release is available on GitHub release |
|
🎉 This PR is included in superdoc v1.27.0 The release is available on GitHub release |
|
🎉 This PR is included in @superdoc-dev/react v1.2.0 The release is available on GitHub release |
|
🎉 This PR is included in superdoc-sdk v1.6.0 |
|
🎉 This PR is included in vscode-ext v2.3.0 |
…validation errors (SD-3287) (#3584) Closes the two highest-volume LLM-tooling error classes from production traffic. **executor.ts — empty text nodes on tracked rewrite (1001/wk).** The word-diff prefix/suffix trim optimization (#2817/#2832) could reduce a non-empty replacement to an empty delta (e.g. "best endeavours to:" → "endeavours to:" leaves trimmedNew === ""), then the single-change branch called schema.text(""), which ProseMirror rejects with `RangeError: Empty text nodes are not allowed`. Add an `else if (trimmedNew.length === 0)` branch that uses `tr.delete(trimmedFrom, trimmedTo)` instead — same semantics as the existing tracked-review-state guard. Regression tests in tracked-rewrite.integration.test.ts cover both layers (direct executor and end-to-end doc.replace in tracked mode). The remap-length empty-replacement test was asserting the buggy `tr.replaceWith(5, 12, <empty node>)` call — the mocked schema masked the crash — corrected to assert `tr.delete(5, 12)` and `tr.replaceWith` not called. **operation-args.ts — opaque oneOf validation errors (2000+/wk cluster).** `validateValueAgainstTypeSpec`'s oneOf branch returned the unactionable `X must match one of the allowed schema variants.` for every object-union failure. LLM agents looped on the same malformed shape because the error named no specific issue. Make the branch discriminator-aware: * When object variants share a const discriminator (kind/op/type) and the value carries it matching exactly one variant, re-validate against that variant and surface its specific failure (e.g. `at.target is required.`, `target.nodeType is required.`). * When the value carries the discriminator but matches no variant, list the allowed tag values and echo what was sent. * When no discriminator is shared (or the value isn't an object), enumerate the accepted variant shapes via `describeVariant` and report the received value via `describeReceived`. Behavior is message-only: identical pass/fail across every input. Same fix benefits every oneOf-validated parameter (target/at/within/steps[]) across insert/create/lists/styles/format/query/mutations at once. Defensive details: * `return;` after the inner re-validate in the discriminator-match path (unreachable in practice — the matched variant already failed in the outer loop — but pins the invariant against future fast-path refactors). * `details.selectedError` set consistently across all three error paths (matched-variant, unmatched-tag, generic fallback). * `truncateForError` caps serialized values at 64 chars so an accidentally- large payload can't blow up logs or LLM context. Matches the same truncation pattern used by REPAIR_BLOCKED. New tests in validate-type-spec.test.ts cover real contract schemas (doc.create.paragraph:at and doc.insert:target) plus a non-discriminated repeated-error case that pins the `selectRepeatedActionableOneOfError` fallback (which the new discriminator path would otherwise mask). The existing "oneOf with mixed schemas" generic-fallback assertion was updated to the enumerated message.
No description provided.