fix: track new lines in suggested mode#3602
Conversation
|
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:
Minor non-issues: No non-existent elements/attributes, no missing required attributes, no bad defaults. The PR removes one prior spec violation (
|
There was a problem hiding this comment.
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
| 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 = {}) { |
There was a problem hiding this comment.
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>
There was a problem hiding this comment.
💡 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) { |
There was a problem hiding this comment.
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 Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
No description provided.