fix: track new lines in suggested mode#3598
Conversation
superdoc-bot
Bot
added
review: thorough
review: careful
and removed
review: thorough
labels
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 663198e717
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } | ||
| if (op.kind === 'rejectParagraphSplit' && op.mark) { | ||
| tr.step(new RemoveMarkStep(op.from, op.to, op.mark)); | ||
| if (!rejectParagraphSplitAt(tr, op.from, op.anchor)) { |
There was a problem hiding this comment.
When rejecting a paragraph-split change as part of a larger decision, e.g. rejectTrackedChanges({ scope: 'all' }) with another tracked change later in the document, this tr.join(...) changes document positions while the rest of sortedOps still use the original positions computed by buildMutationPlan. Existing mark ops were position-stable and content deletions are applied in reverse, but this new doc-changing op runs in the forward mark pass, so later remove/add/replace operations can target the wrong range or fail after the join shrinks the document. The join should be applied in an order that preserves positions or subsequent op ranges should be mapped through tr.mapping.
Useful? React with 👍 / 👎.
|
I wasn't able to get the ecma-spec MCP calls approved in this session (every call and the follow-up question returned "haven't granted it yet"), so the verification below rests on established ECMA-376 schema knowledge plus the code/tests in the diff, not live spec queries. I've flagged anything I'd normally have double-checked against the tool. Here's the review. Status: FAIL The paragraph-split tracking mechanism is mostly well-designed, but there are a couple of spec-type issues plus a non-standard extension worth calling out. 1. Non-numeric
The PR's own test demonstrates the bad output: 2. Foreign
3. Required
What's correct: Representing a tracked paragraph split by placing Please re-run the ecma-spec lookups ( |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
cubic analysis
4 issues found across 16 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/super-editor/src/editors/v1/extensions/comment/tracked-change-display.js">
<violation number="1" location="packages/super-editor/src/editors/v1/extensions/comment/tracked-change-display.js:121">
P2: New `paragraphSplit` display type is emitted without updating the shared `trackedChangeDisplayType` union, causing a typed API-contract drift.</violation>
</file>
<file name="packages/super-editor/src/editors/v1/core/super-converter/v3/handlers/w/p/helpers/generate-paragraph-properties.js">
<violation number="1" location="packages/super-editor/src/editors/v1/core/super-converter/v3/handlers/w/p/helpers/generate-paragraph-properties.js:28">
P1: `findParagraphSplitTrackFormatMark` can lose an already-found split mark because the descendants callback overwrites `found` on later siblings, causing paragraph split tracking metadata to be skipped.</violation>
</file>
<file name="packages/super-editor/src/editors/v1/extensions/track-changes/review-model/decision-engine.js">
<violation number="1" location="packages/super-editor/src/editors/v1/extensions/track-changes/review-model/decision-engine.js:733">
P1: Rejecting a tracked paragraph split is planned per mark-run instead of per logical split, causing repeated joins and reject failures on multi-run spans.</violation>
<violation number="2" location="packages/super-editor/src/editors/v1/extensions/track-changes/review-model/decision-engine.js:992">
P1: `tr.join()` inside `rejectParagraphSplitAt` alters the document structure and shifts all subsequent positions, but remaining operations in the plan still reference their original pre-computed `from`/`to` values. When multiple changes are rejected together (e.g., `rejectAll`) and a paragraph-split is not the last operation, later mark-removal or content-deletion steps will target stale positions—causing incorrect edits or runtime failures. Either apply paragraph-split rejections in a separate pass (reverse order), or map subsequent operation positions through `tr.mapping` after the join.</violation>
</file>
Linked issue analysis
| Status | Acceptance criteria | Notes |
|---|---|---|
| ✅ | Pressing Enter (split paragraph) in Suggesting mode records a tracked paragraph-split change rather than applying the split directly | split-run.js adds addTrackedParagraphSplitMark to create a trackFormat mark on the Enter path; tests assert a tracked change is created and that paragraph texts reflect the split. |
| ✅ | Tracked paragraph-split changes are rendered in the UI as a ‘new line’ / 'Added new line' change | tracked-change-display recognizes paragraphSplit snapshots and CommentDialog.vue renders a specific 'Added new line' message; tests verify the display text. |
| ✅ | Import restores paragraph-split snapshots from SuperDoc/DOCX metadata into a trackFormat mark | markImporter handles paragraph split metadata and creates paragraphSplit snapshots; tests verify handleStyleChangeMarksV2 restores paragraphSplit snapshots from inline paragraph metadata. |
| ✅ | Export emits Word-visible paragraph insertion (w:ins) and pPrChange with SuperDoc paragraphSplit metadata, using the Word id allocator when provided | generate-paragraph-properties and helpers create w:ins insertion element and pPrChange attributes with SuperDoc namespace/attrs; tests assert insertion element, change attrs, and that the allocator is called. |
| ✅ | Rejecting a tracked paragraph-split restores (joins) the paragraphs and removes the trackFormat mark | decision-engine plans and applies a 'rejectParagraphSplit' op which removes the mark and joins the paragraphs; tests exercise rejecting the tracked split and assert the join and removal of the change. |
Reply with feedback, questions, or to request a fix.
Fix all with cubic | Re-trigger cubic
| found = childMarks.find((mark) => isParagraphSplitTrackFormatMark(mark)) || null; | ||
| return !found; |
There was a problem hiding this comment.
P1: findParagraphSplitTrackFormatMark can lose an already-found split mark because the descendants callback overwrites found on later siblings, causing paragraph split tracking metadata to be skipped.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/super-editor/src/editors/v1/core/super-converter/v3/handlers/w/p/helpers/generate-paragraph-properties.js, line 28:
<comment>`findParagraphSplitTrackFormatMark` can lose an already-found split mark because the descendants callback overwrites `found` on later siblings, causing paragraph split tracking metadata to be skipped.</comment>
<file context>
@@ -1,5 +1,70 @@
+ let found = null;
+ node.descendants((child) => {
+ const childMarks = Array.isArray(child?.marks) ? child.marks : [];
+ found = childMarks.find((mark) => isParagraphSplitTrackFormatMark(mark)) || null;
+ return !found;
+ });
</file context>
| found = childMarks.find((mark) => isParagraphSplitTrackFormatMark(mark)) || null; | |
| return !found; | |
| if (!found) { | |
| found = childMarks.find((mark) => isParagraphSplitTrackFormatMark(mark)) || null; | |
| } | |
| return !found; |
| for (const run of getSegmentMarkRuns(seg)) { | ||
| const paragraphSplit = snapshotAttrsForType(run.mark.attrs?.before, 'paragraphSplit'); | ||
| if (paragraphSplit) { | ||
| ops.push({ | ||
| kind: 'rejectParagraphSplit', | ||
| from: run.from, | ||
| to: run.to, | ||
| changeId: change.id, | ||
| side: SegmentSide.Formatting, | ||
| mark: run.mark, | ||
| anchor: paragraphSplit.anchor === 'source' ? 'source' : 'inserted', | ||
| }); | ||
| } else { | ||
| ops.push({ | ||
| kind: 'restoreFormat', | ||
| from: run.from, |
There was a problem hiding this comment.
P1: Rejecting a tracked paragraph split is planned per mark-run instead of per logical split, causing repeated joins and reject failures on multi-run spans.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/super-editor/src/editors/v1/extensions/track-changes/review-model/decision-engine.js, line 733:
<comment>Rejecting a tracked paragraph split is planned per mark-run instead of per logical split, causing repeated joins and reject failures on multi-run spans.</comment>
<file context>
@@ -726,8 +727,22 @@ const planFormattingDecision = ({ ops, change, decision, retired }) => {
+ continue;
+ }
+
+ for (const run of getSegmentMarkRuns(seg)) {
+ const paragraphSplit = snapshotAttrsForType(run.mark.attrs?.before, 'paragraphSplit');
+ if (paragraphSplit) {
</file context>
| tr.step(new RemoveMarkStep(op.from, op.to, op.mark)); | ||
| continue; | ||
| } | ||
| if (op.kind === 'rejectParagraphSplit' && op.mark) { |
There was a problem hiding this comment.
P1: tr.join() inside rejectParagraphSplitAt alters the document structure and shifts all subsequent positions, but remaining operations in the plan still reference their original pre-computed from/to values. When multiple changes are rejected together (e.g., rejectAll) and a paragraph-split is not the last operation, later mark-removal or content-deletion steps will target stale positions—causing incorrect edits or runtime failures. Either apply paragraph-split rejections in a separate pass (reverse order), or map subsequent operation positions through tr.mapping after the join.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/super-editor/src/editors/v1/extensions/track-changes/review-model/decision-engine.js, line 992:
<comment>`tr.join()` inside `rejectParagraphSplitAt` alters the document structure and shifts all subsequent positions, but remaining operations in the plan still reference their original pre-computed `from`/`to` values. When multiple changes are rejected together (e.g., `rejectAll`) and a paragraph-split is not the last operation, later mark-removal or content-deletion steps will target stale positions—causing incorrect edits or runtime failures. Either apply paragraph-split rejections in a separate pass (reverse order), or map subsequent operation positions through `tr.mapping` after the join.</comment>
<file context>
@@ -947,6 +989,13 @@ const applyPlan = ({ state, plan }) => {
tr.step(new RemoveMarkStep(op.from, op.to, op.mark));
continue;
}
+ if (op.kind === 'rejectParagraphSplit' && op.mark) {
+ tr.step(new RemoveMarkStep(op.from, op.to, op.mark));
+ if (!rejectParagraphSplitAt(tr, op.from, op.anchor)) {
</file context>
| const paragraphSplit = findSnapshotByType(before, 'paragraphSplit') || findSnapshotByType(after, 'paragraphSplit'); | ||
| if (paragraphSplit) { | ||
| return { | ||
| trackedChangeDisplayType: ParagraphSplitDisplayType, |
There was a problem hiding this comment.
P2: New paragraphSplit display type is emitted without updating the shared trackedChangeDisplayType union, causing a typed API-contract drift.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/super-editor/src/editors/v1/extensions/comment/tracked-change-display.js, line 121:
<comment>New `paragraphSplit` display type is emitted without updating the shared `trackedChangeDisplayType` union, causing a typed API-contract drift.</comment>
<file context>
@@ -109,6 +115,14 @@ const snapshotAttrsEqual = (a, b) => {
+ const paragraphSplit = findSnapshotByType(before, 'paragraphSplit') || findSnapshotByType(after, 'paragraphSplit');
+ if (paragraphSplit) {
+ return {
+ trackedChangeDisplayType: ParagraphSplitDisplayType,
+ trackedChangeText: 'new line',
+ };
</file context>
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
3 participants
No description provided.