feat(track-changes): block-level structural tracked changes for tables#3343
feat(track-changes): block-level structural tracked changes for tables#3343shri-scale wants to merge 11 commits into
Conversation
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".
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
cb240a2 to
70003e4
Compare
067e5d9 to
2b176d2
Compare
|
Hi @caio-pizzol, I know its big change, just kind request |
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>
2b176d2 to
48bdef8
Compare
When insertTrackedChange is called with from === to at a position where bare text can't sit (e.g. at doc end past the last block), PM auto-wraps the inserted text in the schema's required parents (paragraph + run). The previous mark range used insertedNode.nodeSize -- the bare text length -- which then covered the wrapper's open tokens instead of the trailing characters of the actual text. Those trailing chars escaped the trackInsert mark and survived accept/reject as orphan text nodes (ALPMO-245). Capture the doc size delta around the insert step and use it as the mark range. addMark only attaches to text nodes inside the range, so marking wrapper boundaries is a no-op there but pulls in the trailing chars that PM's auto-wrap pushed out of the bare-nodeSize window. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Block-level diff replay sets `trackedChange` on a tableRow via the StructuralTrackChanges extension. The three caches that drive painted output -- the painter's per-page fingerprint in renderer.ts, the measure cache in layout-bridge, and the canonical block version in layout-resolved -- previously hashed only the cells, missing the row-level attribute. Result: applyHunks-style transactions that only mutated `row.trackChange` reused stale cache entries, the page never re-measured or repainted, and visible cells never received their `data-track-change` attribute (so the row stayed unmarked until a full scroll forced a remeasure). Mirror the row.trackedChange fingerprint into all three caches so the same transaction invalidates each one.
…print
computeStructuralDiff previously matched top-level blocks by
sdBlockId. That works in-process but breaks across imports: the docx
importer assigns a fresh sdBlockId on every load (there's no standard
OOXML attribute for tables to carry a stable id), so the AI-review
flow -- where the proposal is a separately-imported docx -- saw every
block as new and flagged the whole document as changed.
Switch the default identityKey to a normalized content fingerprint
(`${typeName}:${normalizedText}`) so identical blocks match across
imports. Document the limitations in the JSDoc: two truly-identical
blocks share a fingerprint and the algorithm treats them as one
(acceptable for current AI-review flow; consumers needing precise
duplicate handling can pass a custom `identityKey`). Consumers whose
proposal preserves sdBlockIds can opt back in via the option.
There was a problem hiding this comment.
2 issues found across 38 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/super-editor/src/editors/v1/extensions/track-changes/blockTrackedChangeAttr.js">
<violation number="1" location="packages/super-editor/src/editors/v1/extensions/track-changes/blockTrackedChangeAttr.js:13">
P2: Return `null` from `parseDOM` when `data-track-change-id` is absent. A tracked change without an id cannot be resolved by `acceptRejectRowTrackedChange` (which matches by id), leaving an unactionable entry in the document. Guarding on `id` also keeps the runtime shape consistent with the documented type (`id: string` when non-null).</violation>
</file>
<file name="packages/super-editor/src/editors/v1/extensions/track-changes/trackChangesHelpers/trackedTransaction.js">
<violation number="1" location="packages/super-editor/src/editors/v1/extensions/track-changes/trackChangesHelpers/trackedTransaction.js:381">
P1: Missing `map.appendMap(step.getMap())` after applying the step. In multi-step transactions, subsequent steps will be mapped using stale positions that don't account for the document changes made by this block-level step, potentially causing position corruption or silently dropped steps.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Fix all with cubic | Re-trigger cubic
| // Wrapping the inner cell content with inline trackInsert marks would | ||
| // double-track. Apply such steps as-is. | ||
| if (step instanceof ReplaceStep && sliceContainsPreMarkedBlockTrackedChange(step.slice)) { | ||
| newTr.step(step); |
There was a problem hiding this comment.
P1: Missing map.appendMap(step.getMap()) after applying the step. In multi-step transactions, subsequent steps will be mapped using stale positions that don't account for the document changes made by this block-level step, potentially causing position corruption or silently dropped steps.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/super-editor/src/editors/v1/extensions/track-changes/trackChangesHelpers/trackedTransaction.js, line 381:
<comment>Missing `map.appendMap(step.getMap())` after applying the step. In multi-step transactions, subsequent steps will be mapped using stale positions that don't account for the document changes made by this block-level step, potentially causing position corruption or silently dropped steps.</comment>
<file context>
@@ -349,6 +373,15 @@ export const trackedTransaction = ({ tr, state, user, replacements = 'paired' })
+ // Wrapping the inner cell content with inline trackInsert marks would
+ // double-track. Apply such steps as-is.
+ if (step instanceof ReplaceStep && sliceContainsPreMarkedBlockTrackedChange(step.slice)) {
+ newTr.step(step);
+ return;
+ }
</file context>
| newTr.step(step); | |
| newTr.step(step); | |
| map.appendMap(step.getMap()); |
| trackChange: { | ||
| default: null, | ||
| parseDOM: (el) => { | ||
| const kind = el.getAttribute('data-track-change'); |
There was a problem hiding this comment.
P2: Return null from parseDOM when data-track-change-id is absent. A tracked change without an id cannot be resolved by acceptRejectRowTrackedChange (which matches by id), leaving an unactionable entry in the document. Guarding on id also keeps the runtime shape consistent with the documented type (id: string when non-null).
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/super-editor/src/editors/v1/extensions/track-changes/blockTrackedChangeAttr.js, line 13:
<comment>Return `null` from `parseDOM` when `data-track-change-id` is absent. A tracked change without an id cannot be resolved by `acceptRejectRowTrackedChange` (which matches by id), leaving an unactionable entry in the document. Guarding on `id` also keeps the runtime shape consistent with the documented type (`id: string` when non-null).</comment>
<file context>
@@ -0,0 +1,33 @@
+ trackChange: {
+ default: null,
+ parseDOM: (el) => {
+ const kind = el.getAttribute('data-track-change');
+ if (kind !== 'insert' && kind !== 'delete') return null;
+ return {
</file context>
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