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 2 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
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.