feat(track-changes): structural tracked changes for whole-table insert/delete#3614
Conversation
…t/delete (SD-3360) Support importing, listing, deciding, and exporting whole-table insert/delete tracked changes in v1. A whole inserted/deleted table is encoded in OOXML as <w:ins>/<w:del> inside each row's <w:trPr> (rows may carry distinct w:ids); v1 previously dropped these on import, never listed them, could not accept/reject them, and lost them on export. - Schema: add a `trackChange` revision slot to the tableRow node. - Import: tr-translator reads <w:ins>/<w:del> in <w:trPr> into the row attr. - Enumerate: group a table's rows into ONE structural change (kind.type='structural', subtype table-insert/table-delete) when every row is tracked and shares one side; ids may differ. Partial/mixed rows are surfaced but undecidable. - Decide: accept-insert/reject-delete clear the rows; reject-insert/ accept-delete remove the table; partial-range fails closed with INVALID_INPUT; non-whole-table shapes fail closed with CAPABILITY_UNAVAILABLE; inline changes inside a removed table are retired. Graph identity is table-scoped to avoid id collisions. - Export: tr-translator emits <w:ins>/<w:del> in <w:trPr> for roundtrip. - Contract: add 'structural' type + table-insert/table-delete subtype to the document-api surface; regenerate artifacts. Validated against the Labs oracle (sdk-v1): the eight failing-known STRUCT-TABLE conformance/roundtrip/safety cases now pass, zero regression. Deferred: visual paint, and row/column/cell + property structural changes. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
I wasn't able to run the live ECMA-376 lookups this session — the Status: FAIL The core idea is sound — a whole inserted/deleted table really is encoded as 1. 2. 3. Note: the If you can grant the |
Visual + review-UI layer for whole-table insert/delete tracked changes,
on top of the import/list/decide/export logic from the prior commit.
Rendering (both modes):
- Editor mode: trackChangesBasePlugin emits node decorations on tracked
<tr> rows; prosemirror.css styles inserted (green) / deleted
(red + strikethrough) rows.
- Layout-engine mode (presentation/playground view): TableRowAttrs gains
`trackedChange` (shared TrackedChangeMeta); the v1 layout-adapter
populates it, the contracts author-color stamper colors it, and the
DomPainter paints it on the row's cells via applyRowTrackedChangeToCell
— reusing inline tracked-change classes, author-color CSS variables,
and show-original/show-modified modes.
Right-rail bubble:
- Whole-table changes surface as a right-rail card ("Inserted table" /
"Deleted table"), reusing the inline TC bubble pipeline. Positioned by
anchoring on the table PM range via the tracked-change index, so it
works in layout-engine viewing mode. Accept/reject routes through
trackChanges.decide by the public structural id.
- Created during the initial import bootstrap (parity with inline/story
bubbles) so it shows on first render, not only after a later edit.
Coalescing (Word/Google-Docs model — one change per table):
- Decide cascade: deciding a whole-table change cascades the same decision
to inline tracked changes inside the table; accepting an inserted table
also accepts its cell text, rejecting removes the whole table.
Whole-object partial-range still fails closed.
- UI: inline tracked-change bubbles inside a tracked table are suppressed
so only the single structural bubble shows; real comments and inline
changes in non-tracked tables are unaffected.
Labs sdk-v1 unchanged at 363/147 with the structural cases green.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… changes (SD-3360) Authoring: inserting a table in suggesting mode now produces a tracked whole-table insertion. A structural-insert branch in the tracked transaction (replaceStep.js `tryStructuralTableInsert`) inserts the table and stamps each row's `trackChange` (rowInsert) via `stampInsertedTableRows`, matching the imported model. The inline overlap-compiler can't represent an empty (text-free) table, so it fails closed and would otherwise land the table untracked. Guarded to the no-real-content case so replacing a non-empty selection never deletes content untracked. Subsumption: a whole tracked table is ONE review item. Cell text typed in a tracked-inserted row inherits the row's revision id, so the inline change and the structural change share an id. Fixes: - decide resolver prefers the structural change for a shared id, and the staying-table cascade dedups by OBJECT identity (not change.id) so accepting/rejecting the table clears the contained cell text in ONE action (no leftover marks, no second bubble). - comments-store suppresses the inline cell-text comment at creation (and prunes stale ones) so it never becomes a separate floating/active bubble; the structural "Inserted/Deleted table" bubble is exempt. The id-set match is gated on "no inline range" so an inline change outside the table that merely shares a row id is never wrongly suppressed. Reviewed by Codex; must-fix correctness items (untracked content deletion, id-set over-suppression, nested-table descend) addressed. super-editor and superdoc suites green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… (SD-3360) Collapse three copies of the "inline change inside a tracked whole-table change is subsumed" logic into the single central guard in `handleTrackedChangeUpdate` — the chokepoint every comment-creation path (full resync, targeted resync, live comments-plugin handler) funnels through. Removes the duplicated suppress+prune blocks from `createCommentForTrackChanges` and `refreshTrackedChangeCommentsByIds` (~50 lines) with no behavior change. Also drops dead code in decision-engine (unused `Mapping` import and `replacements` param). super-editor track-changes (433) and superdoc (1294) suites green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Deleting a whole table in suggesting mode is now a tracked deletion: the table stays VISIBLE (struck-through), every row gets `trackChange` rowDelete, cell text gets `trackDelete`, and one "Deleted table" bubble shows; Accept removes the table, Reject restores it. Empty tables are kept + tracked instead of being removed untracked; track-changes OFF deletes normally. A new structural-delete branch in `tryCompileStep` (replaceStep.js) intercepts an empty-slice deletion whose range brackets a whole table and routes it to `tryStructuralTableDelete`, which marks cell text via `markDeletion` and stamps rows via the new `stampDeletedTableRows` WITHOUT applying the removal — mirroring the insert authoring. Downstream (enumerate → decide cascade → paint → bubble → suppression) already handles rowDelete. Reviewed by Codex; must-fix edges addressed: a mixed selection (text + table) is guarded so it never shares one deletion id across inside/outside text (falls through), and the un-applied removal's positional effect is cancelled on the outer map so later steps in a multi-step transaction don't drift (no-op for single-step deleteTable). Track-changes + table suites green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Remove the SD-3360 task-number references from code comments and test descriptions added by the structural table tracked-changes work. Comments only; no behavior change. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- Fix range/cursor decide resolving the inline cell-text child instead of
the structural table change on a shared revision id (resolveLogicalChangeById)
- Drop hidden tracked rows from the layout in original/final view modes so an
inserted/deleted table reserves no blank space (was CSS-hidden only)
- Show the imported author on the structural bubble: pass importedAuthor as
{ name } to match the inline shape (was a raw string → "(Imported)")
- Rename the insert label "Inserted table" -> "Added table" to match the
existing "Added ..." insert vocabulary
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Behavior-preserving simplification of the structural whole-table code:
- Merge stampInsertedTableRows + stampDeletedTableRows into a single
stampTableRows({ type }) helper
- Extract collectWholeTablesInRange, the shared "whole tables fully bracketed
by [from,to)" walk, now used by stampTableRows, rangeContainsWholeTable, and
tryStructuralTableDelete
- Consolidate computeTrackedTableRangesForState + computeStructuralChangeIdsForState
into one cached computeTrackedTableSummaryForState returning { ranges, ids }
(single enumeration, single per-state memo)
~250 fewer lines of production code; no behavior change. Cross-reviewed by Codex.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…t-structural-tracked-changes-for-tables-whole-table
The whole-table structural segment sets `structural: true`, but the TrackedSegment typedef did not declare it, so `tsc -b` failed with TS2353 during the build (unit tests don't typecheck, so it passed locally). Add the optional `structural?: boolean` property to the typedef. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The MCP `bun build` (and the Release MCP workflow) failed with "Could not resolve: @superdoc/word-layout" because its exports resolved to ./dist/index.js, which is emitted only by `tsc -b` (project references). The CI MCP job builds superdoc + SDK but never runs that step, so the dist is absent and bun cannot resolve it. This has kept main's MCP builds red since word-layout was introduced. Resolve word-layout from its TypeScript source instead — the same pattern the private, bundler-only `document-api` package uses. word-layout is `private: true` and consumed exclusively by bundlers (super-editor vite, MCP bun, layout-bridge), so there is no node consumer that needs prebuilt JS, and the hidden build-order dependency goes away. Verified with dist absent: MCP build + 37 MCP tests pass; check:types and build:superdoc remain green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The structural-row decoration work made trackChangesBasePlugin.js a public JSDoc surface, tripping the jsdoc-ratchet (new public file without // @ts-check). Add // @ts-check (the preferred resolution) and narrow the Replace-step access (slice/from/to live on ReplaceStep, not the base Step type) so the file typechecks clean. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
…ddition Adding the structural `subtype` field to the track-changes output schema shifted the auto-generated example for trackChanges.get/list (subtype now occupies the example's optional-field budget, displacing pairedWithChangeId). The committed reference docs were regenerated accordingly, but the reference-docs-artifacts test still asserted the old `"pairedWithChangeId": null` example value. Keep every nullable type-label assertion (still valid) and move the "nullable primitive renders as null" example check to header-footers/get, which still surfaces a nullable `refId` in its generated example. Full document-api suite: 1481 pass; check:public:docapi green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
chittolinag
left a comment
chittolinag
left a comment
There was a problem hiding this comment.
Great job here @andrii-harbour ! For this one I decided to focus on manual testing for sake of time. As a side note, please don't forget to update the labs specs around tables too.
Finding 1
- Create a new document and add a table
- Switch to suggesting mode
- Press
cmd + aand hit delete - Verify the tracked changed is add instead of delete
Finding 2
- Create a new document in Word
- Switch to suggesting mode
- Add a table
- Verify that in the right sidebar it shows the change dialog -> Formatted table
- Now, create a document the same way you did in Word, but in SuperDoc
- Export the document
- Open in Word
- Verify that the right sidebar doesn't show the change dialog.
See screenshot of expected dialog in Word:
Finding 3
- Create a new document in Word
- Switch to suggesting mode
- Create a list
- In one of its items, add a table
- Export the document & open in SuperDoc
- Verify that the table is not rendered
SuperDoc vs Word
Finding 4 (unrelated but affects testing)
This doesn't seem to be related to your PR but depending on how I try to select the entire table by keyboard or mouse, it doesn't work. Did you notice something similar?
Finding 5 (possibly out of scope)
- Delete a row in Word in suggesting mode
- Open in SuperDoc
- See the row is "red" as expected, but there's no tracked change in the right sidebar.
From the ticket description it seems like row/column insert/delete are out of scope, but I figured I'd flag just to confirm!
Final notes
From my POV the most important ones here are 1 and 3. For 4 I don't think it's related to your changes - but maybe a follow up ticket? And for 5 please confirm whether that's expected.
|
@chittolinag thanks for the review. Most of the items are existing bugs and not related to tables. I'm creating tickets for them now to address in separate PRs. Other things (related to my PR) I'm fixing now and PR for labs is created as well |
…evisions Two export fixes so Word recognizes a whole inserted/deleted table as a tracked change: - Place the <w:ins>/<w:del> marker inside <w:trPr> after the base row props and before any <w:trPrChange>, per the CT_TrPr sequence (was prepended as the first child, which is schema-invalid when the row also carries e.g. <w:hidden>/<w:trHeight> and causes Word to drop the row revision). - Synthesize the cell-level tracking Word requires from the row revision: each cell paragraph mark gets <w:ins>/<w:del> in <w:pPr><w:rPr> (before any terminal <w:sectPr>/<w:pPrChange>), and each run is wrapped — including runs nested in <w:hyperlink>/<w:smartTag>/<w:sdt> — converting <w:t> to <w:delText> for deletions. Idempotent against already-tracked cell content. Import models only the row revision, so cells are bare on export; without the cell-level markers Word saw an inconsistent state and showed no change dialog. Known follow-ups (not Word-corrupting, just untracked): cell-level <w:sdt> cells and tables nested inside a cell. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…t-structural-tracked-changes-for-tables-whole-table # Conflicts: # apps/docs/document-api/reference/_generated-manifest.json # apps/mcp/src/generated/catalog.ts
![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: 303952691f
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…t-structural-tracked-changes-for-tables-whole-table # Conflicts: # packages/layout-engine/contracts/src/index.ts # packages/layout-engine/painters/dom/src/table/renderTableRow.ts # packages/super-editor/src/editors/v1/core/layout-adapter/converters/table.ts
…anges-for-tables-whole-table
…anges-for-tables-whole-table
…anges-for-tables-whole-table
…anges-for-tables-whole-table
chittolinag
left a comment
chittolinag
left a comment
There was a problem hiding this comment.
LGTM! Please double check the failing behavior tests though, just to make sure they were not introduced here.
|
@chittolinag failed behavior tests were merged in other PR, so it's a part of the main branch now. Since I made a rebase - they are in my branch too |
…anges-for-tables-whole-table
|
🎉 This PR is included in superdoc-cli v0.16.0 The release is available on GitHub release |
|
🎉 This PR is included in superdoc-sdk v1.15.0 |
|
🎉 This PR is included in @superdoc-dev/mcp v0.11.0 The release is available on GitHub release |
|
🎉 This PR is included in superdoc v1.39.0 The release is available on GitHub release |
|
🎉 This PR is included in @superdoc-dev/react v1.10.0 The release is available on GitHub release |
|
🎉 This PR is included in vscode-ext v2.11.0 |
6 participants
No description provided.