fix(tracked-changes): sync tracked changes store on undo and redo#2164
fix(tracked-changes): sync tracked changes store on undo and redo#2164
Conversation
palmer-cl
commented
Feb 24, 2026
palmer-cl
commented
- fix undo and redo logic with tracked changes
- add behavior tests and visual tests
![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: 48eb61710e
ℹ️ 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/superdoc/src/components/CommentsLayer/FloatingComments.vue
Outdated
Show resolved
Hide resolved
caio-pizzol
left a comment
caio-pizzol
left a comment
There was a problem hiding this comment.
solid fix - guard removal + explicit sync on undo/redo addresses the actual data flow gap (PR #2195 will prob conflict here)
| * @param {DOMElement} parentElement The parent element of the editor | ||
| * @returns {void} | ||
| */ | ||
| const handleEditorLocationsUpdate = (allCommentPositions) => { |
There was a problem hiding this comment.
correct for undo, but changes behavior for all callers.
heads up: #2195 is incoming and touches the same files (FloatingComments.vue, comments-store.js, SuperDoc.vue) - it adds stale cleanup in FloatingComments that mitigates any flicker from transient empty payloads, so should be safe once both land
There was a problem hiding this comment.
@caio-pizzol can you elaborate on:
correct for undo, but changes behavior for all callers.
so should be safe once both land
There was a problem hiding this comment.
I merged your changes in as well. Just make sure thats working as you would expect.
caio-pizzol
requested review from
VladaHarbour,
artem-harbour,
harbournick and
tupizz
and removed request for
tupizz
…o-tracked-change-leaves-suggestion-bubble-side-panel
caio-pizzol
left a comment
caio-pizzol
left a comment
There was a problem hiding this comment.
@tupizz null-handling and RTL comment from last round are fixed, nice. no blocking correctness issues.
a few things still open — left inline comments: the fallback path + resolvePositionAtGoalX have zero test coverage (the null-skip fix lives there, so it really should have cases), and the test duplication concern from round 1 is still worth considering. also flagged two questions about #scrollCaretIntoViewIfNeeded scope.
| #scrollCaretIntoViewIfNeeded(caretLayout: { pageIndex: number }): void { | ||
| const caretEl = this.#localSelectionLayer?.querySelector( | ||
| '.presentation-editor__selection-caret', | ||
| ) as HTMLElement | null; |
There was a problem hiding this comment.
the caret element can exist even when the page isn't actually visible (the rendering has a fallback that creates it with estimated coordinates). when that happens, this skips #scrollPageIntoView and scrolls to the wrong spot. worth checking if the page is actually mounted before using the caret rect?
| @@ -4124,6 +3923,7 @@ export class PresentationEditor extends EventEmitter { | |||
| console.warn('[PresentationEditor] Failed to render caret overlay:', error); | |||
| } | |||
| } | |||
| this.#scrollCaretIntoViewIfNeeded(caretLayout); | |||
There was a problem hiding this comment.
this scrolls on every selection change, not just arrow keys — collab edits, undo, find-and-replace all trigger it. the margin check avoids scrolling when the caret is already visible, but are there cases where this causes unexpected jumps?
| // lands on the current line's fragment start instead of the adjacent | ||
| // line — this causes the cursor to appear stuck since the "new" position | ||
| // equals the current one. | ||
| if (adjacent.pmStart != null && adjacent.pmEnd != null) { |
There was a problem hiding this comment.
the fallback path (resolvePositionAtGoalX) has zero test coverage — no test sets data-pm-start/data-pm-end, so this block never runs. two cases would cover it: hit lands in range (fallback skipped) and hit lands out of range (fallback runs). the null-skip fix should get a case too.
| * This mirrors the implementation to allow direct unit testing without | ||
| * bootstrapping the full PresentationEditor. | ||
| */ | ||
| function scrollCaretIntoViewIfNeeded( |
There was a problem hiding this comment.
re-flagging from last round — the test copies the full method. if the real code changes, these tests still pass on the old version. worth extracting into a shared function both can use?
3 participants
