feat: llm tools for tables#3119
Merged
caio-pizzol merged 10 commits intomainfrom May 7, 2026
Merged
Conversation
- Implemented a set of llm tools for the tables. - Introduced `lists.delete` operation to delete entire lists, including all items in the same sequence. - Added `tables.applyPreset` operation to apply visual presets to tables, enhancing styling options. - Updated documentation to reflect new operations and their usage. - Adjusted references in the manifest and index files to include the new operations.
Contributor
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
…Shading undefined color column-clone (tables-adapter.column-clone-merge.test.js): when inserting a column adjacent to a merged source cell, normalizeClonedColumnInsertCellAttrs only resets colspan, so the inserted cell carries tableCellProperties.gridSpan / vMerge (and rowspan) from the source. Test loads table-merged-cells.docx, inserts a column, and asserts the rightmost column has no merge metadata. setShading undefined color (tables-adapter.set-shading-undefined-color.test.js): the merged superdoc_table tool schema only requires action, leaving color as a documented-but-not-enforced field. tablesSetShadingAdapter calls normalizeColorInput(input.color) before its try/catch, so undefined input throws TypeError instead of returning a structured INVALID_INPUT failure. Both tests fail today; they will pass once the underlying behavior is fixed.
caio-pizzol
reviewed
May 6, 2026
Contributor
caio-pizzol
left a comment
caio-pizzol
left a comment
There was a problem hiding this comment.
hey @andrii-harbour! solid scope here.
i pushed two failing regression tests on the branch (e0b1f28) that lock in the column-clone and setShading findings.
three things inline.
Updated the tables adapter to enforce the presence of a color parameter in the setShading operation, preventing calls with undefined color values. This change addresses a regression where the merged superdoc_table tool schema allowed calls without a specified color, leading to runtime errors. Added tests to verify that the required color is included in every requiredOneOf branch for the set_shading operation.
Contributor
Author
|
@caio-pizzol fixed |
…t textStyle attrs row-clone-hmerge: round-2 helper now resets colspan and strips gridSpan/vMerge in normalizeClonedRowInsertCellAttrs. cloning a row below a horizontally merged source row produces a too-narrow row that PM auto-pads to N singletons, silently losing the merge geometry. test asserts the inserted row preserves source cell shapes. set-cell-text-textstyle-attrs: round-2 NO_OP fix excludes textStyle by mark name, but Color/FontFamily/FontSize all addGlobalAttributes onto the textStyle mark itself. a cell whose 'hi' carries a user-set color via setColor returns NO_OP - same shape as the round-1 bold case. test asserts the rewrite happens and the color is cleared. Both fail today; they will pass once the helper is reverted to round-1 shape and the NO_OP check looks at mark.attrs values instead of mark name.
caio-pizzol
reviewed
May 7, 2026
Contributor
caio-pizzol
left a comment
caio-pizzol
left a comment
There was a problem hiding this comment.
hey @andrii-harbour! thanks for addressing last round.
i pushed two failing tests on the branch (130eea522) for the row-clone regression and the textStyle blind spot.
Important
needs work, three things inline.
the round-1 P1 fixes themselves all land - bold NO_OP, column merge metadata, setShading crash. but the round-2 changes have three new edge-case holes: cloning a row below a horizontally merged source row loses the merge geometry; setCellText still NO_OPs when the only difference is a textStyle attr like color or font; and setBorders SDK validation no longer enforces mode / applyTo / border / edges, so incomplete tool calls reach runtime instead of being rejected upfront.
agent context
{
"schema": "review_handoff_v1",
"review_id": "pr-3119-r2",
"findings": [
{
"id": "row-clone-loses-merge-geometry",
"severity": "important",
"anchors": [{ "file": "packages/super-editor/src/editors/v1/document-api-adapters/tables-adapter.ts", "line": 300, "quote": "colspan: 1," }],
"claim": "normalizeClonedRowInsertCellAttrs now resets colspan to 1 and strips gridSpan/vMerge via the shared helper. when the source row contains a horizontally merged cell (gridSpan>1), the insertRowInTable loop pushes one singleton cell while advancing col by the source colspan, leaving a too-narrow row. ProseMirror table normalization auto-pads to the table width, but the merge is silently replaced by N singletons.",
"failure_mode": "regression",
"verification": [{ "command": "pnpm --filter @superdoc/super-editor vitest run src/editors/v1/document-api-adapters/tables-adapter.row-clone-hmerge.test.js", "result_summary": "source row [colspan: 3] produces inserted row [1, 1, 1] - test fails", "status": "executed" }],
"ruled_out": [{ "hypothesis": "PM table-map ends up actually corrupt", "why_ruled_out": "prosemirror-tables fixTables / appendTransaction auto-pads the row to match map.width. the resulting table has consistent geometry; only the merge intent is lost." }],
"handoff": {
"recommended_start": "normalizeClonedRowInsertCellAttrs at tables-adapter.ts:293",
"suggested_fix": "don't apply stripMergeMetadataFromTableCellProperties and colspan: 1 inside normalizeClonedRowInsertCellAttrs - column-insert needs a singleton clone, but row-insert should mirror the source's geometry. revert lines 298 and 300 to the round-1 shape (only rowspan: 1, sourceAttrs spread without strip).",
"expected_files": ["packages/super-editor/src/editors/v1/document-api-adapters/tables-adapter.ts"]
},
"verification_after_fix": [{ "command": "pnpm --filter @superdoc/super-editor vitest run src/editors/v1/document-api-adapters/tables-adapter.row-clone-hmerge.test.js src/editors/v1/document-api-adapters/tables-adapter.column-clone-merge.test.js", "expected": "both tests pass - row-clone preserves source colspans, column-clone still strips merge metadata" }]
},
{
"id": "setCellText-textStyle-attr-blind-spot",
"severity": "important",
"anchors": [{ "file": "packages/super-editor/src/editors/v1/document-api-adapters/tables-adapter.ts", "line": 2948, "quote": "const DEFAULT_INHERITED_MARK_NAMES = new Set(['textStyle']);" }],
"claim": "the round-2 NO_OP fix excludes textStyle by mark name, but Color, FontFamily, and FontSize all addGlobalAttributes onto the textStyle mark itself. so a cell whose 'hi' carries a user-set color via setColor returns NO_OP and leaves the red. same shape of bug as the round-1 bold case, just for color/font instead of bold.",
"failure_mode": "incomplete-fix",
"verification": [{ "command": "pnpm --filter @superdoc/super-editor vitest run src/editors/v1/document-api-adapters/tables-adapter.set-cell-text-textstyle-attrs.test.js", "result_summary": "setCellText('hi') on a cell with red 'hi' returns NO_OP failure; color stays - test fails", "status": "executed" }],
"handoff": {
"recommended_start": "paragraphHasUserAppliedMarks at tables-adapter.ts:2956",
"suggested_fix": "check mark.attrs values rather than mark name. for textStyle, treat any attr whose value is non-default as user-applied. or replace the allowlist with an explicit denylist of formatting marks plus textStyle-with-attrs.",
"expected_files": ["packages/super-editor/src/editors/v1/document-api-adapters/tables-adapter.ts"]
}
},
{
"id": "setBorders-codegen-drops-branch-required",
"severity": "important",
"anchors": [{ "file": "packages/sdk/codegen/src/generate-intent-tools.mjs", "line": 297, "quote": "if (Array.isArray(branch.oneOf)) {" }],
"claim": "for schemas where each top-level oneOf branch carries its OWN required (mode/applyTo/border for setBorders' applyTo branch, mode/edges for the edges branch) AND a nested locator oneOf, the codegen walks only the inner sub-branches and pulls just target/nodeId. baseRequired comes from the schema's top-level required which is empty here. result: catalog.json for set_borders has requiredOneOf: [['target'],['nodeId'],['target'],['nodeId']] - mode, applyTo, border, edges all dropped. SDK validator at tools.ts:326 lets {nodeId: 't1'} through; runtime executeTablesSetBorders then throws.",
"failure_mode": "validator-bypass",
"verification": [
{ "command": "jq '.tools[] | select(.toolName==\"superdoc_table\") | .operations[] | select(.intentAction==\"set_borders\") | .requiredOneOf' packages/sdk/tools/catalog.json", "result_summary": "[['target'],['nodeId'],['target'],['nodeId']] - mode/applyTo/border/edges absent", "status": "executed" }
],
"handoff": {
"recommended_start": "extractRequiredConstraints at generate-intent-tools.mjs:284",
"suggested_fix": "when descending into a nested branch.oneOf, merge the OUTER branch's required (not just the schema's top-level required) into each sub-branch's requirement set.",
"expected_files": ["packages/sdk/codegen/src/generate-intent-tools.mjs", "packages/sdk/codegen/src/__tests__/intent-tool-merge.test.ts"]
},
"verification_after_fix": [{ "command": "pnpm run generate:all && jq '.tools[] | select(.toolName==\"superdoc_table\") | .operations[] | select(.intentAction==\"set_borders\") | .requiredOneOf' packages/sdk/tools/catalog.json", "expected": "every requiredOneOf branch contains 'mode' (and either 'applyTo'+'border' or 'edges')" }]
}
]
}
…ity attrs
Two small fixes on top of round 3:
1. dryRun NO_OP mirroring (tables-adapter.ts:3007)
The new project-and-compare NO_OP detection sat below the dryRun
early-return. Live calls correctly NO_OP, but dryRun returned success
even when the equivalent live call would NO_OP. Move the dryRun
branch below the NO_OP check so the preview matches reality.
Includes regression test using editor.doc.tables.setCellText(..., { dryRun: true }).
2. Extend IDENTITY_BLOCK_ATTRS with imported rsid family
paragraph.js declares rsidR / rsidRDefault / rsidP / rsidRPr / rsidDel
as paragraph attrs the importer fills from source XML. They differ
between an imported paragraph and a freshly built one, so the NO_OP
comparator should ignore them just like the auto-regenerated
sdBlockId/paraId family. (Note: deeper run-level attrs like
runProperties also re-derive on dispatch and are not yet covered;
that is a separate concern around imported metadata preservation.)
caio-pizzol
approved these changes
May 7, 2026
Contributor
caio-pizzol
left a comment
caio-pizzol
left a comment
There was a problem hiding this comment.
hey @andrii-harbour! thanks for addressing last round - the codegen merge, row-clone hmerge fix, and the new project-and-compare NO_OP all land cleanly.
i pushed a small follow-up commit (a2e46d0) that moves the dryRun branch below the NO_OP check (so dryRun mirrors live) and adds the imported rsidR / rsidRDefault / rsidP / rsidRPr / rsidDel family to IDENTITY_BLOCK_ATTRS. while looking at this i also noticed setCellText on imported cells re-derives run-level metadata (runProperties like kern / lang / ligatures that don't surface as marks). that's a separate concern from this PR's scope; worth a follow-up ticket so a same-text setCellText on imported docs doesn't quietly drop OOXML-level run props on dispatch.
lgtm.
Contributor
|
🎉 This PR is included in @superdoc-dev/mcp v0.3.0-next.69 The release is available on GitHub release |
Contributor
|
🎉 This PR is included in @superdoc-dev/react v1.2.0-next.111 The release is available on GitHub release |
Contributor
|
🎉 This PR is included in vscode-ext v2.3.0-next.113 |
Contributor
|
🎉 This PR is included in superdoc-cli v0.8.0-next.85 The release is available on GitHub release |
Contributor
|
🎉 This PR is included in superdoc v1.30.0-next.66 The release is available on GitHub release |
Contributor
|
🎉 This PR is included in superdoc-sdk v1.8.0-next.67 |
Contributor
|
🎉 This PR is included in superdoc-cli v0.9.0 The release is available on GitHub release |
Contributor
|
🎉 This PR is included in superdoc v1.32.0 The release is available on GitHub release |
Contributor
|
🎉 This PR is included in @superdoc-dev/mcp v0.4.0 The release is available on GitHub release |
Contributor
|
🎉 This PR is included in @superdoc-dev/react v1.3.0 The release is available on GitHub release |
Contributor
|
🎉 This PR is included in vscode-ext v2.4.0 |
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.
lists.deleteoperation to delete entire lists, including all items in the same sequence.tables.applyPresetoperation to apply visual presets to tables, enhancing styling options.