feat(track-changes): block-level structural tracked changes for tables#3343
Open
shri-scale wants to merge 8 commits into
Open
feat(track-changes): block-level structural tracked changes for tables#3343shri-scale wants to merge 8 commits into
shri-scale wants to merge 8 commits into
Conversation
![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: b9d84246ae
ℹ️ 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".
| // Merge: keep existing inline entries, add block-level entries. | ||
| // Block-level keys (operationId / row-id) shouldn't collide with | ||
| // inline ones (mark-id), but if they ever do, inline wins. | ||
| pluginState.trackedChanges = { ...blockTracked, ...pluginState.trackedChanges }; |
There was a problem hiding this comment.
When a row-level tracked change is accepted or rejected, getBlockTrackedChanges(newEditorState) no longer returns that operation, but this merge keeps every previous pluginState.trackedChanges entry. In that scenario the comments/review state still contains the resolved operationId/row id, so the sidebar/bubble can continue to treat an already-resolved structural change as active. Rebuild the block portion by removing old isBlockLevel entries before merging in the current blockTracked entries.
Useful? React with 👍 / 👎.
| }, | ||
| renderDOM: (attrs) => { | ||
| const tc = attrs?.trackChange; | ||
| return tc ? { 'data-track-change': tc.kind } : {}; |
There was a problem hiding this comment.
For any path that serializes a tracked table row to HTML and reparses it (for example getHTML/setContent, clipboard, or DOM-backed state reconstruction), this renderer emits only the kind while parseDOM expects data-track-change-id and data-track-change-operation. The reparsed row gets id: null, and getBlockTrackedChanges ignores it, so the structural tracked change can no longer be surfaced or accepted/rejected. Include the id and operation attributes when present.
Useful? React with 👍 / 👎.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
shri-scale
force-pushed
the
feature/block-level-tracked-changes
branch
from
cb240a2 to
70003e4
Compare
Adds a new StructuralTrackChanges extension that gives block-level (table) add/remove operations the same review pipeline as inline tracked changes - same data-track-change rendering, same accept/reject command surface, plus block-level entry registration in the comments-plugin. Consumer pattern: compute structural hunks via computeStructuralDiff (or construct them yourself), dispatch via editor.commands.setStructuralDiff (hunks). The extension stamps a trackChange PM attribute on each affected tableRow. Rendering is handled by the painter (reads row.trackedChange, stamps data-track-change* on cells). Accept/reject operates on PM attrs via getBlockTrackedChanges + applyRowTrackedChangeResolution. Pipeline pieces: - Shared blockTrackedChangeAttrSpec helper + TableRow.addAttributes spread - TableRow.trackedChange contract field - pm-adapter populates TableRow.trackedChange from PM attr - painter renderTableRow stamps data-track-change on cells - CSS rules for [data-track-change="insert" | "delete"] - getBlockTrackedChanges walks the doc for block-level entries - applyRowTrackedChangeResolution handles accept/reject by id - acceptTrackedChangeById / rejectTrackedChangeById extended to route inline marks, row attrs, or operationId - trackedTransaction passes through ReplaceSteps that already carry block-level metadata so inline marks don't double-track - comments-plugin walks block-level entries alongside inline marks Existing Diffing extension, compareDocuments, replayDifferences, and inline TrackChanges are untouched. The new extension is opt-in via editorExtensions: [StructuralTrackChanges] (not in starter extensions). Tests: ~30 new unit tests across the touched modules, plus an end-to-end test using a real .docx fixture pair that exercises compute -> set -> accept-all and compute -> set -> reject-all. Out of scope (separate follow-ups): - DOCX round-trip of block-level tracked changes - Block-level entries surfacing in the default SuperDoc bubble (requires mirroring inline's commentsUpdate event payload pipeline) - Cell-level / column-level tracking - A combined acceptAllChanges that batches inline + structural into one transaction (currently two sequential commands; two undo steps) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ed changes - Move painter-targeting CSS into BLOCK_TRACK_CHANGE_STYLES in the painter's styles module so document visuals live behind the painter boundary; keep contenteditable-side rules scoped under .sd-editor-scoped .ProseMirror tr. - Drop stale isBlockLevel entries from previous state before merging in freshly-computed block entries, so accept/reject clears the bubble. - renderDOM now emits data-track-change-id and data-track-change-operation alongside data-track-change so HTML round-trips preserve enough state for getBlockTrackedChanges and acceptTrackedChangeById to resolve the row. Adds regression tests: stale-entry pruning in comments-plugin apply(), renderDOM/parseDOM round-trip in blockTrackedChangeAttr, and a styles.ts guard that fails if a bare [data-track-change=...] selector regresses. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ensions Consumers (Superdoc Vue host, @superdoc-dev/react wrapper) take their extension list from getStarterExtensions(); without this entry the new block-level extension was exported but never loaded into the editor. Updates the dedicated test that previously asserted opt-in-only behavior. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The comments-plugin apply() reducer walked the entire doc for block-level tracked rows on every tr.docChanged, even on docs that had no tracked rows. Combined with in-place mutation of pluginState.trackedChanges and the view.update decoration rebuild path, this created a stream of state references that downstream consumers reacted to, contributing to focus steal in side-by-side layouts where the editor sat next to an unrelated input control. Two structural changes: - Track a hasBlockChanges bit on plugin state, computed once at init and refreshed only when a walk runs. Gate the block walk on (hasBlockChanges OR inputType='acceptReject' meta) so typing in a doc with no tracked rows never triggers the walk. - Stop mutating pluginState.trackedChanges and pluginState.activeThreadId inside apply(); compute next-values locally and return them via the spread at the end. Plugin state must be treated as immutable across apply() per ProseMirror convention. Adds regression tests that lock the gate (hasBlockChanges stays false through typing, flips true once a tracked row appears). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ed editors Track which DOMs the bridge has owned and only redirect keystrokes to a stale ProseMirror[contenteditable=true] candidate if it is in that set. Without this guard, an unrelated PM-based editor (Tiptap, Remirror) mounted alongside SuperDoc would receive redirected input. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…nd export structural API acceptAllTrackedChanges and rejectAllTrackedChanges now also dispatch acceptAllStructuralChanges / rejectAllStructuralChanges when block-level row entries exist, so the single accept-all/reject-all surface resolves both inline marks and table-row tracked changes. Without this, a table-delete operation would leave the row, cell, and table shell in place after accept-all. Also re-export StructuralTrackChanges and computeStructuralDiff from the superdoc package entry so SDK consumers can wire up structural diffs without importing from super-editor directly. git commit -F /tmp/cmsg2.txt
The WeakSet-based ownership check broke header/footer/footnote/endnote
accept/reject + undo (6 behavior tests in story-surface-tracked-change-decide):
story editors weren't registered in the bridge's owned set when sidebar
operations were the first interaction, so the bridge dropped the
stale-target redirect that those flows depend on.
Switch the check to a class-based ancestor lookup
(`closest('.sd-editor-scoped')`). Every SuperDoc editor — including
header/footer/footnote/endnote — is rendered inside a `.sd-editor-scoped`
container, so they're recognized without needing prior bridge activation;
foreign PM editors (Tiptap, Remirror, raw PM) are not, so the focus-steal
fix is preserved.
Bridge unit tests updated to wrap stale editors in a `.sd-editor-scoped`
ancestor (matches production DOM); foreign-editor regression test
unchanged (its `.tiptap.ProseMirror` element has no SuperDoc ancestor and
is correctly ignored).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Adds StructuralTrackChanges + computeStructuralDiff to the SD-3176
legacy-exports snapshot (intentional growth on superdoc/super-editor).
The named export is what consumers will import via:
import { StructuralTrackChanges, computeStructuralDiff } from "superdoc";
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
shri-scale
force-pushed
the
feature/block-level-tracked-changes
branch
from
067e5d9 to
2b176d2
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds a new
StructuralTrackChangesextension that gives block-level (table) add/remove operations the same review pipeline as inline tracked changes — samedata-track-changerendering, same accept/reject command surface, plus block-level entry registration in the comments-plugin so the review UI surface has the data it needs.computeStructuralDiffor their own logic) and dispatches viaeditor.commands.setStructuralDiff(hunks).trackChangePM attribute on each affectedtableRow; painter reads it and stampsdata-track-change*on cells; CSS styles them.Diffing,compareDocuments,replayDifferences, and inlineTrackChangesare untouched. New extension is opt-in viaeditorExtensions: [StructuralTrackChanges](not in starter extensions).What's in this PR
blockTrackedChangeAttrSpechelper; spread intoTableRow.addAttributesTrackedChangeMeta.operationId?+TableRow.trackedChange?parseTableRowpopulatesTableRow.trackedChangefrom the PM attrrenderTableRowstampsdata-track-change*on each cell of a tracked row[data-track-change="insert" | "delete"]rules, themable via--sd-block-tracked-*getBlockTrackedChanges,applyRowTrackedChangeResolution(in track-changes)StructuralTrackChangeswithsetStructuralDiff, accept/reject by id, bulk, and operation-grouped commandsacceptTrackedChangeById/rejectTrackedChangeByIdtrackedTransactioninterceptorReplaceSteps whose slice already carries block-leveltrackChangeattrs (no double-tracking)pluginState.trackedChangesandallCommentPositionsStructuralTrackChanges,computeStructuralDiffre-exported from the v1 barrelTest plan
pnpm testfrompackages/super-editor/— should be green (~13,000 tests).pnpm vitest run blockTrackedChangeAttr getBlockTrackedChanges acceptRejectRowTrackedChange blockTrackedChangePassthrough computeStructuralDiff applyHunks structural-track-changes blockTrackedChangeBubblepnpm vitest run structural-track-changes-e2e— exercisescomputeStructuralDiff→setStructuralDiff→acceptAllStructuralChanges/rejectAllStructuralChangesagainst a real.docxpair.editorExtensions: [StructuralTrackChanges], dispatch a remove hunk viasetStructuralDiff→ the matching table's rows turn red.acceptAllStructuralChangesremoves the table.Out of scope (separate follow-ups)
renderDOM/parseDOM, but no OOXML writer/reader is wired)editor.emit('commentsUpdate', ...)payload pipeline that block-level changes don't yet emit. Consumers with custom review UI (al-pmo today) can wire fromgetBlockTrackedChangesdirectly.acceptAllChangesthat batches inline + structural into one transaction (currently two sequential commands; two undo steps)computeStructuralDiffwhensdBlockIds differ across imports (consumer can supply a customidentityKey)🤖 Generated with Claude Code