fix(tracked-changes): colors should be restored when format rejected#1970
fix(tracked-changes): colors should be restored when format rejected#1970
Conversation
palmer-cl
commented
Feb 8, 2026
palmer-cl
commented
- Refactored track-change mark snapshot logic into shared helpers
- Fixed format suggestion reversibility using exact mark attr snapshot matching and consistent before/after upsert behavior, so rejecting a color suggestion restores original styling correctly.
- Added regression coverage: an interaction test for “suggest color -> reject -> no inline color left” and new unit tests for markSnapshotHelpers (exact match, type fallback, upsert behavior, and range lookup).
![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: 9d402911ed
ℹ️ 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".
packages/super-editor/src/extensions/track-changes/trackChangesHelpers/markSnapshotHelpers.js
Outdated
Show resolved
Hide resolved
caio-pizzol
left a comment
caio-pizzol
left a comment
There was a problem hiding this comment.
Very clean implementation!
Unfortunately, it is not fully fixing it
Font color is "rejected" but the font family is not returning to the original one "Times New Roman" but to "Arial"
Original:
After reject:
packages/super-editor/src/extensions/track-changes/trackChangesHelpers/markSnapshotHelpers.js
Show resolved
Hide resolved
…-changes-color-change-suggestion-not-revertible-on
@caio-pizzol this should be fixed now. Refactored to make it more robust when tracking formatting |
There was a problem hiding this comment.
Feel free to merge it once relevant comments are addressed.
One thing to note: the interaction tests cover PM state correctly, but there's no interaction regression test for the complete scenario. Since rejecting format suggestions affects what the user sees, a visual test (import doc, suggest color, reject, screenshot-compare) would catch divergence.
Just added a story - let's sync if you have any question on adding this to other PRs.
packages/super-editor/src/extensions/track-changes/trackChangesHelpers/markSnapshotHelpers.js
Outdated
Show resolved
Hide resolved
...s/super-editor/src/extensions/track-changes/trackChangesHelpers/getLiveInlineMarksInRange.js
Show resolved
Hide resolved
| type: mark.type.name, | ||
| attrs: { ...mark.attrs }, | ||
| })); | ||
| before = liveMarks |
There was a problem hiding this comment.
liveMarks is fetched from newTr.doc (which already includes the addMark from line 42), so the new mark attrs are captured in the snapshot.
Then track-change marks are filtered out. This means the before snapshot already reflects the incoming style change, not the state before it. Was this tested for the "add formatting on top of existing tracked formatting" scenario?
There was a problem hiding this comment.
Was this tested for the "add formatting on top of existing tracked formatting" scenario?
Can you elaborate on this one? do we have a VRT for that scenario that would fail in the test suite?
There was a problem hiding this comment.
My concern about liveMarks was wrong - it's read before newTr.addMark, so the before snapshot is correct.
No VRT for this. The "rejecting multi-format suggestions" test covers it partially but doesn't assert fontFamily restoration after rejection.
packages/super-editor/src/extensions/track-changes/trackChangesHelpers/markSnapshotHelpers.js
Show resolved
Hide resolved
…lor-change-suggestion-not-revertible-on' into colep/sd-1770-tracked-changes-color-change-suggestion-not-revertible-on
afn
left a comment
afn
left a comment
There was a problem hiding this comment.
Looks good apart from @caio-pizzol's previous comments about tests. Just a couple of minor suggestions to add.
| export const attrsExactlyMatch = (left = {}, right = {}) => { | ||
| const normalizedLeft = normalizeAttrs(left); | ||
| const normalizedRight = normalizeAttrs(right); | ||
| return objectIncludes(normalizedLeft, normalizedRight) && objectIncludes(normalizedRight, normalizedLeft); |
There was a problem hiding this comment.
Could use lodash's isEqual instead.
| import { objectIncludes } from '@core/utilities/objectIncludes.js'; | ||
|
|
||
| const normalizeAttrs = (attrs = {}) => { | ||
| return Object.fromEntries(Object.entries(attrs).filter(([, value]) => value !== null && value !== undefined)); |
There was a problem hiding this comment.
Could use lodash's omitBy to be a bit more succinct:
| return Object.fromEntries(Object.entries(attrs).filter(([, value]) => value !== null && value !== undefined)); | |
| return omitBy(attrs, value => value === null || value === undefined); |
4 participants
