fix: remove persisting yellow underline after canceling track-format suggestion#2212
Conversation
…suggestion Two bugs caused the gold/yellow TrackFormat underline to persist after reversing a tracked format change: 1. addMarkStep applied TrackFormat using the full step range instead of clamped per-node boundaries, leaking wrong before/after arrays onto adjacent text nodes. 2. removeMarkStep didn't clean up fully-reversed format changes — when a tracked addition was toggled back (after=[] but before had items), the TrackFormat mark persisted instead of being removed.
![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: acc9ad563a
ℹ️ 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".
Narrowed the early-exit condition so TrackFormat is only removed when both arrays are empty (true no-op). Keeps the mark when before has legitimate tracked removals (e.g. before=[italic], after=[]).
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 201e7c9a7a
ℹ️ 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".
Hoist a sharedWid variable so all inline nodes touched by a single addMarkStep reuse the same TrackFormat ID. With per-node clamped ranges, getLiveInlineMarksInRange no longer sees the previous node's TrackFormat, causing each node to generate a separate UUID. This broke ID-based reject/accept flows for multi-node selections.
Mirror the sharedWid pattern from addMarkStep. When an existing formatChangeMark is present, reuse its ID to preserve change grouping. When creating new TrackFormat marks across multiple nodes, share a single generated ID so reject/accept treats them as one change.
|
@codex review |
|
Codex Review: Didn't find any major issues. Nice work! ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
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". |
|
@caio-pizzol please review at the earliest |
caio-pizzol
left a comment
caio-pizzol
left a comment
There was a problem hiding this comment.
@gpardhivvarma solid fix — addresses the root cause (wrong ranges, mismatched IDs, missing cleanup) not just symptoms. left one comment about tests.
Cover the three fixes from this PR: - Shared TrackFormat ID across nodes in a single AddMarkStep - TrackFormat removal when toggling a tracked addition back (empty after + empty before) - TrackFormat retention when before is non-empty (real tracked removal)
|
@caio-pizzol Please take a look at it |
caio-pizzol
left a comment
caio-pizzol
left a comment
There was a problem hiding this comment.
@gpardhivvarma the before-only diff fix and the shared wid pattern are both in good shape — nice. left two small things on the new test file: an unused import and a gap in test coverage that would let the range-clamping fix go unguarded if it's ever changed. the coverage one is worth a look before merging, the import is a quick fix.
|
Thanks for the work on this @gpardhivvarma! The range clamping and We found one remaining edge case when canceling a format change on nodes with pre-existing marks (e.g., bold on/off on italic text leaves a ghost "removed italic" mark). We've built on your commits and added the fix in #2233. Closing in favor of #2233. Appreciate the contribution! |
2 participants
Summary
addMarkrange to per-node boundaries (Math.max(step.from, pos)/Math.min(step.to, pos + node.nodeSize)) instead of using the full step range, preventing incorrectbefore/afterarrays from leaking onto adjacent text nodes.afterbecomes empty after filtering), remove the TrackFormat mark entirely instead of creating a new one withafter=[]and a stalebeforearray — which would keep the gold/yellow underline visible for a no-op change.Test plan
pnpm --filter super-editor test)pnpm --filter behavior test -- reject-format-suggestion(requires Playwright browsers installed)