Skip to content

fix: track new lines in suggested mode#3602

Merged
harbournick merged 1 commit into
mainfrom
nick/it-1136-track-changes-paragraph-split-enter-and-paragraph-style
Jun 1, 2026
Merged

fix: track new lines in suggested mode#3602
harbournick merged 1 commit into
mainfrom
nick/it-1136-track-changes-paragraph-split-enter-and-paragraph-style

Conversation

@harbournick
Copy link
Copy Markdown
Collaborator

No description provided.

@harbournick harbournick self-assigned this Jun 1, 2026
@harbournick harbournick requested a review from a team as a code owner June 1, 2026 21:40
@linear-code
Copy link
Copy Markdown

linear-code Bot commented Jun 1, 2026

IT-1136

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Jun 1, 2026

I have everything I need. The ecma-spec MCP server is denying permission in this environment, so I verified the schema reasoning against ECMA-376 directly. The OOXML-generating code is straightforward to assess.


Status: PASS

I focused on the handlers that actually emit OOXML (the test files and editor-command/UI files don't produce XML). Here's what I checked:

w:rPrChangecreateRunPropertiesChangeElement (helpers.js:106)
The PR drops w:authorEmail from the emitted attribute set, leaving w:id, w:author, w:date. This is correct — w:rPrChange is a CT_TrackChange (w:id from the CT_Markup base, plus author/date), and w:authorEmail is not in that attribute set. The old code was emitting a non-spec attribute; this change fixes that violation. (spec)

w:id now forced to a decimal — getTrackFormatChangeWordId (helpers.js:22)
w:id on track-change elements is ST_DecimalNumber (an integer). The previous sourceId || logicalId path could emit a value like format-1, which is invalid. The new decimal-coercion (allocator → decimal source/logical id → hash fallback) keeps it schema-valid. The test changes (expect.stringMatching(/^\d+$/)) confirm this. Compliance improvement.

w:ins paragraph-mark insertion — createParagraphSplitInsertionElement (helpers.js:62) + generate-paragraph-properties.js
A tracked paragraph-mark insertion is correctly represented as <w:ins> inside the paragraph mark's <w:rPr> (within <w:pPr>). That's exactly how CT_ParaRPr models it via EG_ParaRPrTrackChanges, and the w:ins carries the right CT_TrackChange attributes (w:id/w:author/w:date). It's unshift-ed to the front of w:rPr, which matches the schema order (the ins/del group precedes the run-property children). (spec)

w:rPr placement in w:pPrinsertRunPropertiesInOrder (generate-paragraph-properties.js)
CT_PPr orders the paragraph-mark w:rPr after the paragraph-level properties and before the terminal w:sectPr/w:pPrChange. The insert-before-first-sectPr/pPrChange logic (and the two ordering tests asserting ['w:pStyle','w:spacing','w:rPr','w:pPrChange'] and [...,'w:rPr','w:sectPr','w:pPrChange']) is correct. (spec)

w:pPrChange (test-only changes)
The added tests confirm SuperDoc-private sd:*/xmlns:sd attributes are neither decoded nor emitted, and w:pPrChange stays within {w:id, w:author, w:date}. Consistent with CT_TrackChange. (spec)

Minor non-issues: w:date/w:author may be empty/undefined, but date is optional and an empty required author still satisfies the schema's presence requirement — no violation.

No non-existent elements/attributes, no missing required attributes, no bad defaults. The PR removes one prior spec violation (w:authorEmail) and tightens w:id to a valid decimal.

Note: I couldn't get live confirmation from the ecma-spec MCP server — every call returned a permission-denied error in this environment — so the above reflects the ECMA-376 schema directly rather than a tool round-trip. If you want, re-run me with that tool approved and I'll attach the exact attribute/child listings.

Copy link
Copy Markdown

@cubic-dev-ai cubic-dev-ai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cubic analysis

1 issue found across 19 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/core/super-converter/v3/handlers/w/t/helpers/translate-text-node.js">

<violation number="1" location="packages/super-editor/src/editors/v1/core/super-converter/v3/handlers/w/t/helpers/translate-text-node.js:13">
P2: The `resolveExportPartPath` function is duplicated identically in `generate-paragraph-properties.js` and the newly-modified `translate-text-node.js`. Both files already import from `@converter/v3/handlers/helpers.js`, making it a natural shared home. If the path-resolution logic ever needs to change (e.g., to support additional path sources or formats), both copies must be kept in sync.</violation>
</file>

Linked issue analysis

Linked issue: IT-1136: Track Changes: paragraph split (Enter) and paragraph-style changes are applied directly, not recorded as tracked changes

Status Acceptance criteria Notes
Paragraph splits (Enter/new line) are recorded as a tracked change (paragraph-mark insertion) when Track Changes / Suggesting mode is active The split path now creates a track-format mark representing a paragraphSplit (split-run.js adds addTrackedParagraphSplitMark; tests exercise creation), and importer/helpers preserve/restore paragraphSplit snapshots.
Paragraph-split tracked changes are shown in the UI as a 'new line' tracked-change display Tracked-format display handling and comment dialog were extended to detect paragraphSplit snapshots and render them as a 'new line' change, with tests verifying the behavior.
Accepting/rejecting a tracked paragraph split updates document structure (join on reject, preserved on accept) Decision engine was extended to plan and apply structural joins for rejecting paragraph splits; split-run tests cover rejecting single and multiple splits and restoring original paragraph structure.
Paragraph-split tracked changes round-trip to/from DOCX (export emits Word-visible paragraph-mark insertion, import restores snapshot) Export now injects a w:ins inside paragraph run-properties for paragraphSplit marks and uses the Word id allocator; importer restores paragraphSplit snapshots from run metadata; tests verify export element attributes and import behavior.
Paragraph-style changes (e.g. styles.paragraph.setStyle) are supported as tracked changes (so paragraph-style edits can be suggested) The linked issue asked for tracked paragraph-style changes as well, but this PR's diffs and tests focus on paragraph-split/new-line tracking. I see no explicit change enabling styles.paragraph.setStyle to operate in tracked mode or tests demonstrating tracked paragraph-style changes.

Reply with feedback, questions, or to request a fix.

Fix all with cubic | Re-trigger cubic

import { combineRunProperties, decodeRPrFromMarks } from '@converter/styles.js';
import { appendTrackFormatChangeToRunProperties, findTrackFormatMark } from '@converter/v3/handlers/helpers.js';

function resolveExportPartPath(params = {}) {
Copy link
Copy Markdown

@cubic-dev-ai cubic-dev-ai Bot Jun 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2: The resolveExportPartPath function is duplicated identically in generate-paragraph-properties.js and the newly-modified translate-text-node.js. Both files already import from @converter/v3/handlers/helpers.js, making it a natural shared home. If the path-resolution logic ever needs to change (e.g., to support additional path sources or formats), both copies must be kept in sync.

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/t/helpers/translate-text-node.js, line 13:

<comment>The `resolveExportPartPath` function is duplicated identically in `generate-paragraph-properties.js` and the newly-modified `translate-text-node.js`. Both files already import from `@converter/v3/handlers/helpers.js`, making it a natural shared home. If the path-resolution logic ever needs to change (e.g., to support additional path sources or formats), both copies must be kept in sync.</comment>

<file context>
@@ -10,6 +10,14 @@ import { translator as wRPrNodeTranslator } from '../../rpr/rpr-translator.js';
 import { combineRunProperties, decodeRPrFromMarks } from '@converter/styles.js';
 import { appendTrackFormatChangeToRunProperties, findTrackFormatMark } from '@converter/v3/handlers/helpers.js';
 
+function resolveExportPartPath(params = {}) {
+  if (typeof params.currentPartPath === 'string' && params.currentPartPath.length > 0) return params.currentPartPath;
+  if (typeof params.filename === 'string' && params.filename.length > 0) {
</file context>
Fix with Cubic

Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 9178dd3024

ℹ️ 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".

}
: null;
let pPr = wPPrNodeTranslator.decode({ node: { ...node, attrs: { paragraphProperties } } });
if (!params?.isFinalDoc && paragraphSplitTrackFormatMark) {
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Omit paragraph-split revision carriers in final exports

When exportDocx({ isFinalDoc: true }) is used after a user presses Enter in suggesting mode, this branch suppresses only the paragraph-mark <w:ins>, but the same paragraph-split trackFormat mark remains on the text and is still serialized by the run/text export path as a <w:rPrChange> carrier. That leaves a tracked revision in a supposedly final/accepted export instead of producing just the split paragraphs; the final-doc path needs to drop or skip the paragraph-split trackFormat carrier as well.

Useful? React with 👍 / 👎.

@codecov-commenter
Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@harbournick harbournick merged commit 4c74ec6 into main Jun 1, 2026
73 checks passed
@harbournick harbournick deleted the nick/it-1136-track-changes-paragraph-split-enter-and-paragraph-style branch June 1, 2026 22:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Track Changes: paragraph split (Enter) and paragraph-style changes are applied directly, not recorded as tracked changes

2 participants