fix(plan-engine): date.setValue and checkbox.setState update the visible content control, not just its stored value#3802
fix(plan-engine): date.setValue and checkbox.setState update the visible content control, not just its stored value#3802AKhoo wants to merge 5 commits into
Conversation
AKhoo
changed the title
![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: beb7b91a22
ℹ️ 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".
| // ...but updating w:fullDate alone leaves the SDT showing its placeholder | ||
| // ("Click or tap to enter a date.") forever. Mirror textSetValueWrapper and | ||
| // also rewrite the visible content so the rendered date actually changes. | ||
| const contentUpdated = replaceSdtTextContent(editor, input.target, input.value); |
There was a problem hiding this comment.
When this runs on a date SDT imported from Word while it is still showing placeholder content (the added fixture carries w:showingPlcHdr), the code replaces the visible text but leaves w:showingPlcHdr in the passthrough sdtPr. The exporter preserves unhandled sdtPr children, so a saved DOCX can still mark the newly written value as placeholder text even though w:fullDate and the visible content changed. Clear that placeholder marker as part of date.setValue when replacing the placeholder content.
Useful? React with 👍 / 👎.
| // updateStructuredContentById a bare text node, which a block SDT's schema | ||
| // rejects (block content must be wrapped in a paragraph). | ||
| // Instead use replaceSdtTextContent to swap the glyph. | ||
| const visualUpdated = replaceSdtTextContent(editor, input.target, symbol.char); |
There was a problem hiding this comment.
For block checkboxes whose checked/unchecked symbols depend on the w14:font entry (for example Wingdings/private-use glyphs), this replaces the content with plain unmarked text via replaceSdtTextContent. The inline path and checkbox creation path apply symbol.font, but this branch drops it, so toggling a block checkbox can render/export the wrong glyph even though the metadata state was updated.
Useful? React with 👍 / 👎.
Code Review by Qodo
1. Checkbox ignores content lock
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| } else if (sdt.kind === 'block') { | ||
| // Block-scope checkboxes can't reuse the inline branch above: it feeds | ||
| // updateStructuredContentById a bare text node, which a block SDT's schema | ||
| // rejects (block content must be wrapped in a paragraph). | ||
| // Instead use replaceSdtTextContent to swap the glyph. | ||
| const visualUpdated = replaceSdtTextContent(editor, input.target, symbol.char); | ||
| return visualUpdated || checkboxUpdated; |
There was a problem hiding this comment.
1. Checkbox ignores content lock 🐞 Bug ≡ Correctness
checkboxSetStateWrapper and dateSetValueWrapper rewrite SDT inner content via replaceSdtTextContent without enforcing assertNotContentLocked, so contentLocked/sdtContentLocked controls can report success while the lock plugin blocks the glyph/text update. This can leave SDT metadata updated but visible content unchanged, creating out-of-sync states or silent partial failures.
Agent Prompt
## Issue description
`checkboxSetStateWrapper` and `dateSetValueWrapper` now mutate SDT inner content via `replaceSdtTextContent`, but they only check `assertNotSdtLocked` and do not enforce `assertNotContentLocked`. This bypasses the required content-lock contract and can produce partial updates under the structured-content lock plugin (metadata changes succeed while visible glyph/text remains stale), leaving controls in an inconsistent state.
## Issue Context
- `replaceSdtTextContent` relies on callers to enforce `contentLocked` / `sdtContentLocked` before mutating the SDT inner range.
- The structured-content lock plugin blocks content modifications inside `contentLocked` / `sdtContentLocked` SDTs, so missing `assertNotContentLocked` can cause silent failures or metadata/visible-content divergence.
## Fix Focus Areas
- packages/super-editor/src/editors/v1/document-api-adapters/plan-engine/content-controls-wrappers.ts[1425-1465]
- packages/super-editor/src/editors/v1/document-api-adapters/plan-engine/content-controls-wrappers.ts[1324-1349]
- packages/super-editor/src/editors/v1/document-api-adapters/plan-engine/content-controls-wrappers.ts[349-385]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| // Instead use replaceSdtTextContent to swap the glyph. | ||
| const visualUpdated = replaceSdtTextContent(editor, input.target, symbol.char); | ||
| return visualUpdated || checkboxUpdated; |
There was a problem hiding this comment.
2. Checkbox glyph loses font 🐞 Bug ≡ Correctness
The new block-scope checkbox visual update inserts only symbol.char via replaceSdtTextContent, dropping the symbol.font styling that other checkbox rendering paths apply (textStyle fontFamily), which can change the rendered glyph or strip intended styling.
Agent Prompt
### Issue description
For block-scope checkboxes, `checkbox.setState` swaps the glyph using `replaceSdtTextContent(editor, target, symbol.char)`, but this replacement does not apply `symbol.font` as a `textStyle` mark. Other checkbox paths (inline updates and create.checkbox) explicitly apply `fontFamily`.
### Issue Context
- `buildCheckboxTextJson(symbol)` includes a `textStyle` mark with `fontFamily: symbol.font`.
- `replaceSdtTextContent`’s block path builds plain text with `marks=undefined`.
### Fix Focus Areas
- packages/super-editor/src/editors/v1/document-api-adapters/plan-engine/content-controls-wrappers.ts[1425-1465]
- packages/super-editor/src/editors/v1/document-api-adapters/plan-engine/content-controls-wrappers.ts[769-801]
- packages/super-editor/src/editors/v1/document-api-adapters/plan-engine/content-controls-wrappers.ts[349-385]
### Suggested change
1. In the block checkbox branch, replace the paragraph content with a node that applies `textStyle`/fontFamily (e.g., reuse `createTextWithOptionalFont(editor, symbol.char, symbol.font)` and wrap in a paragraph), instead of calling the generic plain-text helper.
2. Alternatively, extend `replaceSdtTextContent` to accept optional marks (or a factory callback) so checkbox can supply the appropriate font mark while other callers remain unchanged.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
looksLikeHTML anchored its tag-detection regexes to the start and end of
the string, so content that mixes text with inline elements (e.g. a
checkbox glyph followed by "<strong>yes</strong>") was misclassified as
plain text and its tags were inserted as literal characters. Match a tag
pair, self-closing tag, or void element anywhere in the string instead,
while still rejecting prose with stray angle brackets ("5 < 10").
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
choiceListSetSelectedWrapper synced the SDT's visible text via updateStructuredContentById for every control, but that command feeds a bare inline text node into the SDT. For a block-scope dropdown (structuredContentBlock, schema block*) PM's content check rejects the inline content and rolls back the whole transaction — including the w:lastValue write made just before — so the selection failed silently while still reporting success. Branch on sdt.kind like checkboxSetStateWrapper: inline keeps updateStructuredContentById; block uses replaceSdtTextContent, which wraps the display text in a paragraph the block schema accepts. Adds a block_dropdown_control.docx fixture and an end-to-end spec that drives import -> setSelected -> read, and splits the wrapper unit tests into inline and block scopes. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…h boundary A legacy HYPERLINK field code (w:fldChar/w:instrText) can begin in one paragraph and close in a later one — common when a link fills a table cell. The field preprocessor wrapped all of the collected content, whole <w:p> blocks included, in a single inline <w:hyperlink>. Because <w:hyperlink> cannot contain a paragraph, the importer dropped the paragraphs and left the cell with zero block children, violating the tableCell `block+` schema and aborting the whole document load with "Invalid content for node type tableCell". Detect when the collected content spans a paragraph boundary and, instead of hoisting one hyperlink around the paragraphs, keep each paragraph and wrap only its runs in their own <w:hyperlink> sharing the same target — the representation Word uses for a link that covers more than one line. Adds a minimal fixture that reproduces the crash through the full import path, plus unit coverage for the per-paragraph wrapping. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bac3af5794
ℹ️ 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".
| if (child?.name === 'w:r') { | ||
| if (!pendingRuns) pendingRuns = []; | ||
| pendingRuns.push(child); |
There was a problem hiding this comment.
When a HYPERLINK field begins after ordinary text in its first paragraph or ends before ordinary text in its last paragraph, preProcessNodesForFldChar passes the whole boundary paragraphs here, including those outside-field runs. This loop wraps every direct w:r, so the prefix and suffix unexpectedly become clickable; preserve the field boundaries when splitting the hyperlink across paragraphs rather than treating every run in each collected paragraph as field content.
Useful? React with 👍 / 👎.
| // ...but updating w:fullDate alone leaves the SDT showing its placeholder | ||
| // ("Click or tap to enter a date.") forever. Mirror textSetValueWrapper and | ||
| // also rewrite the visible content so the rendered date actually changes. | ||
| const contentUpdated = replaceSdtTextContent(editor, input.target, input.value); |
There was a problem hiding this comment.
For a date control with lockMode: 'contentLocked', assertNotSdtLocked permits this operation and the preceding attribute update succeeds, but the structured-content lock plugin rejects this content-range replacement. replaceSdtTextContent nevertheless returns true, so date.setValue reports success while the visible value remains unchanged; this new content mutation must enforce the content lock before dispatch (the same issue affects the newly added block checkbox and dropdown replacement paths).
Useful? React with 👍 / 👎.
| // ...but updating w:fullDate alone leaves the SDT showing its placeholder | ||
| // ("Click or tap to enter a date.") forever. Mirror textSetValueWrapper and | ||
| // also rewrite the visible content so the rendered date actually changes. | ||
| const contentUpdated = replaceSdtTextContent(editor, input.target, input.value); |
There was a problem hiding this comment.
When the date control has a non-ISO w:dateFormat or locale, copying the stored value verbatim produces the wrong displayed date. For example, the added fixture declares dd/MM/yyyy with en-GB, but setting 2026-05-24 now displays that literal ISO string instead of 24/05/2026; derive the visible text from the control's date format, locale, and calendar while retaining the input as w:fullDate.
Useful? React with 👍 / 👎.
| // Paragraph properties (w:pPr) and any other structural child must stay a | ||
| // direct child of the paragraph, outside the inline hyperlink. | ||
| flushPendingRuns(); | ||
| wrappedChildren.push(child); |
There was a problem hiding this comment.
When a cross-paragraph field's visible text is inside an inline wrapper such as w:sdt, this loop leaves the wrapper outside any hyperlink and therefore imports its text without a link mark. The changed preProcessNodesForFldChar test demonstrates this exact w:sdtContent shape and now expects the link to disappear; recurse into inline wrappers or otherwise apply the hyperlink to their visible runs while keeping the paragraph structure valid.
Useful? React with 👍 / 👎.
2 participants
Summary
Two content-control operations in the plan-engine document-API adapter mutated the underlying OOXML but left the rendered document unchanged, so callers saw the placeholder/old glyph even though the operation reported success:
date.setValuewrotew:sdtPr/w:date/@w:fullDatebut never rewrote the SDT's run content. A date control that had never been filled in kept showing its placeholder ("Click or tap to enter a date.") forever, and a control with an existing value kept displaying the old text.checkbox.setStateupdated the checkbox state for inline SDTs but did nothing for block-scope checkboxes, so the visible glyph (☐ / ☒) never changed.Both are fixed by also rewriting the visible content, mirroring how
textSetValueWrapperalready keeps the stored value and the rendered text in sync.Changes
date.setValueAfter updating
w:fullDate, also callreplaceSdtTextContentto rewrite the visible run text. The mutation is reported as successful if either the metadata update or the content update lands a change.checkbox.setStateAdd a
block-kind branch. The existing inline branch feedsupdateStructuredContentByIda bare text node, which a block SDT's schema rejects (block content must be wrapped in a paragraph). The block branch usesreplaceSdtTextContentto swap the glyph instead.