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.
| return objectIncludes(normalizedLeft, normalizedRight) && objectIncludes(normalizedRight, normalizedLeft); | ||
| }; | ||
|
|
||
| export const markSnapshotMatchesStepMark = (snapshot, stepMark, exact = true) => { |
There was a problem hiding this comment.
three matching functions that are the same logic with different parameter types markSnapshotMatchesStepMark(snapshot, stepMark), markMatchesSnapshot(mark, snapshot), and hasMatchingMark(marks, stepMark) all do "does type match + do attrs match?"
but each handles different input shapes (snapshot.type vs mark.type.name vs mark.type). A single function with a type-name extractor would eliminate the duplication:
const getTypeName = (m) => m.type?.name ?? m.type;
const marksMatch = (a, b, exact = true) => {
if (getTypeName(a) !== getTypeName(b)) return false;
return !exact || attrsExactlyMatch(a.attrs || {}, b.attrs || {});
};
There was a problem hiding this comment.
good idea. updated.
| const marks = []; | ||
| const seen = new Set(); | ||
|
|
||
| doc.nodesBetween(from, to, (node) => { |
There was a problem hiding this comment.
Both iterate doc.nodesBetween, check node.isInline, then deduplicate marks with mark.type.name + separator + JSON.stringify(mark.attrs) into a Set.
The unsetAllMarks.js version uses \0 separator, this one uses :.
Not blocking since they're in different domains, but if more consumers appear this should be a shared utility in core/helpers/.
There was a problem hiding this comment.
Ok makes sense will keep an eye out for this pattern
| 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?
| let typeOnlyMatch = null; | ||
| const shouldFallbackToTypeOnly = !snapshot?.attrs || Object.keys(snapshot.attrs).length === 0; | ||
|
|
||
| doc.nodesBetween(from, to, (node) => { |
There was a problem hiding this comment.
return false in nodesBetween only stops descent, not iteration. After exactMatch is found, sibling nodes are still visited needlessly.
Should we add if (exactMatch) return at the top of the callback?
There was a problem hiding this comment.
Yes good idea. Added a short circuit return in the function.
…lor-change-suggestion-not-revertible-on' into colep/sd-1770-tracked-changes-color-change-suggestion-not-revertible-on
3 participants
