feat(doc): adjust Markdown pipe-table column widths from cell content#557
feat(doc): adjust Markdown pipe-table column widths from cell content#557flyinghanger wants to merge 6 commits intolarksuite:mainfrom
Conversation
`docs +create` and `docs +update --mode overwrite` currently upload Markdown tables that the server renders with equal column widths (e.g. 244/244/244 for a 3-column table), regardless of how uneven the cell contents actually are. Users have to manually drag columns in the UI to get a readable layout. This change adds a local pipe-table parser plus a content-aware width-ratio calculator. After the MCP create/update call succeeds the CLI walks the document's table blocks in order, matches each against the corresponding Markdown table by column count, and PATCHes `update_grid_column_width_ratio` with ratios summing to 100. Column weights come from the visual width of each cell (CJK / emoji / full-width forms count 2; narrow forms count 1; control chars 0). The widest column absorbs rounding error so the array sums exactly. Behaviour is gated by a new `--auto-table-widths` bool flag on both commands; it defaults to `true` so users get the better layout out of the box, and can opt out with `--auto-table-widths=false`. For `docs +update`, only `--mode overwrite` is handled in this pass because other modes (`append`, `insert_*`, `replace_range`) would require identifying which table blocks are new vs pre-existing; that is left as a follow-up. Test Plan - `go test ./shortcuts/doc/...` passes (new tests cover table parsing, ratio calculation, visual-width edge cases) - `golangci-lint run --new-from-rev=origin/main ./shortcuts/doc/...`: 0 issues - `go mod tidy` produced no changes
github-actions
bot
added
domain/ccm
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds an opt-out CLI flag Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI as CLI Handler
participant Parser as Markdown Parser
participant Compute as Width Computation
participant API as DOCX API
participant Remote as Remote Document
User->>CLI: Run +create/+update (--auto-table-widths=true)
CLI->>CLI: Create/Update document and normalize markdown result
CLI->>Parser: Extract pipe tables from markdown
Parser->>Parser: Parse tables (skip fenced code blocks, handle \|)
Parser-->>CLI: Return extracted tables
alt Tables found and flag enabled
CLI->>Compute: Compute per-column visual widths and ratios
Compute-->>CLI: Return width ratios (sum = 100)
CLI->>API: Fetch remote DOCX table blocks (paginated)
API-->>CLI: Return table blocks in document order
CLI->>API: PATCH each block with update_grid_column_width_ratio.width_ratios
API->>Remote: Apply patches
Remote-->>API: Confirm updates
API-->>CLI: Return patch results
else Flag disabled or no tables or token missing
CLI-->>CLI: Skip patching, log message if applicable
end
CLI-->>User: Output final result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
shortcuts/doc/table_widths.go (2)
332-343: Extract theblock_type == 31magic number into a named constant.
block_type == 31is the DOCX table block type. A named constant (e.g.docxBlockTypeTable = 31) besidewidthRatioTotalmakes intent readable at the call site and eases future additions (e.g. quote-container tables).♻️ Suggested refactor
const widthRatioTotal = 100 + +// docxBlockTypeTable is the block_type value for DOCX table blocks +// as returned by /open-apis/docx/v1/documents/{id}/blocks. +const docxBlockTypeTable = 31 ... - bt, _ := m["block_type"].(float64) - if int(bt) != 31 { + bt, _ := m["block_type"].(float64) + if int(bt) != docxBlockTypeTable { continue }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/doc/table_widths.go` around lines 332 - 343, Introduce a named constant for the DOCX table block type (e.g. docxBlockTypeTable = 31) next to the existing widthRatioTotal constant, then replace the literal `31` in the block type check inside the loop (the `bt` / `block_type` check) with that constant; update the conditional `if int(bt) != 31 {` to `if int(bt) != docxBlockTypeTable {` so intent is clear and future table-type additions are easier to manage (leave all other logic, variable names like blockID, table, prop, cs and the remoteTable append unchanged).
173-189: Minor: zero-width joiners and other Cf format chars count as width 1.
unicode.IsControlmatches category Cc only. ZWJ (U+200D), variation selectors (U+FE0E/F), and other Cf characters fall through to the defaultw++branch, so a ZWJ emoji sequence like 👨👩👧 gets counted as 2+1+2+1+2 = 8 instead of 2. Not a correctness issue for column ratios (it just over-weights cells with emoji families), but an easy win:♻️ Suggested tweak
- case unicode.IsControl(r): + case unicode.IsControl(r) || unicode.In(r, unicode.Cf, unicode.Mn, unicode.Me): continueAlso consider U+FE0F (emoji VS-16) etc. Optional; fine to defer.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/doc/table_widths.go` around lines 173 - 189, The visualWidth function currently treats only Cc control chars as zero-width; update its checks (in visualWidth) to also treat format characters (Unicode category Cf) and common emoji variation selectors/ZWJ as zero-width so they don't increment w; e.g., add unicode.Is(unicode.Cf, r) to the control branch and/or explicitly check runes like U+200D and U+FE0F (and other variation selectors) alongside the existing unicode.IsControl and isWideRune checks so emoji ZWJ sequences are counted correctly while leaving isWideRune handling intact.shortcuts/doc/table_widths_test.go (2)
34-51: Consider adding a blank-line separator case.The current "two tables" sub-test separates tables with a prose line, but the common case in real docs is two tables separated only by a blank line. That exercises the
current != nilflush path on an empty-line (non-pipe-row) iteration and would catch regressions where accumulation continues across a blank line. Similar for a table immediately followed by a fence.📝 Suggested additional sub-test
t.Run("blank line separates two tables", func(t *testing.T) { md := "| a | b |\n|---|---|\n| 1 | 2 |\n\n| c | d |\n|---|---|\n| 3 | 4 |\n" got := extractMarkdownTables(md) if len(got) != 2 { t.Fatalf("expected 2 tables, got %d", len(got)) } })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/doc/table_widths_test.go` around lines 34 - 51, Add a new sub-test to table_widths_test.go that verifies a blank line (and optionally a fence) separates two tables so extractMarkdownTables correctly flushes the current table on an empty non-pipe row; specifically, add a t.Run like "blank line separates two tables" that supplies markdown with two tables separated only by a blank line (and assert len(got) == 2), and optionally add a similar case where a table is immediately followed by a fenced block to ensure the same flush path in extractMarkdownTables is exercised.
127-150: Consider also coveringapplyMarkdownTableColumnWidthsend-to-end.The extraction/computation helpers are well covered, but the integration glue that actually drives the feature (error handling in
fetchDocumentTableBlocks, mismatchedColumnSize, partial PATCH failures, the "column widths applied to N/M tables" summary) has no direct test. A fake that implementsruntime.CallAPIcould exercise these paths without touching the network. Per the repo guideline "Every behavior change must have an accompanying test", at least the non-fatal-failure and mismatched-columns skip paths would be worth locking down.As per coding guidelines:
**/*.go: Every behavior change must have an accompanying test.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/doc/table_widths_test.go` around lines 127 - 150, Add an end-to-end test that exercises applyMarkdownTableColumnWidths by stubbing runtime.CallAPI with a fake implementation to simulate: successful fetchDocumentTableBlocks, error/non-fatal fetch paths, mismatched ColumnSize (N/M) responses, and partial PATCH failures; drive the code through applyMarkdownTableColumnWidths and assert the returned summary/messages and that error-handling branches in fetchDocumentTableBlocks and the ColumnSize skip logic are exercised (use the fake CallAPI to return crafted responses for fetch, compute, and PATCH calls so you can verify non-fatal failures and summary text without network access).shortcuts/doc/docs_update.go (1)
116-120: LGTM — conditional wiring is clean.Guard chain (
mode == "overwrite"→ flag enabled → docx-token resolvable) is tight, and failures insideapplyMarkdownTableColumnWidthsare already non-fatal, so this can only add log-level noise on the unhappy path. Worth noting that for wiki-backed docsdocxTokenForUpdatesilently returnsfalseso the column-width step is skipped with no indication to the user — fine for now, but if a user explicitly passes--auto-table-widths=trueon a wiki URL, a one-liner toruntime.IO().ErrOutexplaining "skipped for wiki docs" could save a support question later.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/doc/docs_update.go` around lines 116 - 120, When runtime.Bool("auto-table-widths") is true and runtime.Str("mode") == "overwrite" but docxTokenForUpdate(runtime.Str("doc")) returns (docID, false), emit a one-line user-facing message via runtime.IO().ErrOut explaining the auto-table-widths step was skipped for wiki-backed docs; locate the guard around docxTokenForUpdate and add a short ErrOut call in the else-path before/after skipping applyMarkdownTableColumnWidths so users who passed --auto-table-widths=true get a clear "skipped for wiki docs" notice.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@shortcuts/doc/table_widths.go`:
- Around line 233-283: The current index-based pairing in
applyMarkdownTableColumnWidths can misalign local tables (tables) and remote
blocks (remote) when counts differ; add a strict precondition: if len(remote) !=
len(tables) then log a single warning/error to runtime.IO().ErrOut and return
(skip the whole adjustment) instead of proceeding with partial or index-shifted
updates; reference the variables/tokens tables, remote, limit, patched and the
function applyMarkdownTableColumnWidths to locate where to add the
length-equality check before the for-loop that computes ratios and PATCHes
blocks.
---
Nitpick comments:
In `@shortcuts/doc/docs_update.go`:
- Around line 116-120: When runtime.Bool("auto-table-widths") is true and
runtime.Str("mode") == "overwrite" but docxTokenForUpdate(runtime.Str("doc"))
returns (docID, false), emit a one-line user-facing message via
runtime.IO().ErrOut explaining the auto-table-widths step was skipped for
wiki-backed docs; locate the guard around docxTokenForUpdate and add a short
ErrOut call in the else-path before/after skipping
applyMarkdownTableColumnWidths so users who passed --auto-table-widths=true get
a clear "skipped for wiki docs" notice.
In `@shortcuts/doc/table_widths_test.go`:
- Around line 34-51: Add a new sub-test to table_widths_test.go that verifies a
blank line (and optionally a fence) separates two tables so
extractMarkdownTables correctly flushes the current table on an empty non-pipe
row; specifically, add a t.Run like "blank line separates two tables" that
supplies markdown with two tables separated only by a blank line (and assert
len(got) == 2), and optionally add a similar case where a table is immediately
followed by a fenced block to ensure the same flush path in
extractMarkdownTables is exercised.
- Around line 127-150: Add an end-to-end test that exercises
applyMarkdownTableColumnWidths by stubbing runtime.CallAPI with a fake
implementation to simulate: successful fetchDocumentTableBlocks, error/non-fatal
fetch paths, mismatched ColumnSize (N/M) responses, and partial PATCH failures;
drive the code through applyMarkdownTableColumnWidths and assert the returned
summary/messages and that error-handling branches in fetchDocumentTableBlocks
and the ColumnSize skip logic are exercised (use the fake CallAPI to return
crafted responses for fetch, compute, and PATCH calls so you can verify
non-fatal failures and summary text without network access).
In `@shortcuts/doc/table_widths.go`:
- Around line 332-343: Introduce a named constant for the DOCX table block type
(e.g. docxBlockTypeTable = 31) next to the existing widthRatioTotal constant,
then replace the literal `31` in the block type check inside the loop (the `bt`
/ `block_type` check) with that constant; update the conditional `if int(bt) !=
31 {` to `if int(bt) != docxBlockTypeTable {` so intent is clear and future
table-type additions are easier to manage (leave all other logic, variable names
like blockID, table, prop, cs and the remoteTable append unchanged).
- Around line 173-189: The visualWidth function currently treats only Cc control
chars as zero-width; update its checks (in visualWidth) to also treat format
characters (Unicode category Cf) and common emoji variation selectors/ZWJ as
zero-width so they don't increment w; e.g., add unicode.Is(unicode.Cf, r) to the
control branch and/or explicitly check runes like U+200D and U+FE0F (and other
variation selectors) alongside the existing unicode.IsControl and isWideRune
checks so emoji ZWJ sequences are counted correctly while leaving isWideRune
handling intact.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 932b24d7-ea95-4e58-a34d-6e570dd795e0
📒 Files selected for processing (4)
shortcuts/doc/docs_create.goshortcuts/doc/docs_update.goshortcuts/doc/table_widths.goshortcuts/doc/table_widths_test.go
Address review feedback on larksuite#557: - Abort applyMarkdownTableColumnWidths with a single stderr warning when the remote table block count doesn't match the local markdown table count. Silently truncating to min(local,remote) could leave every later table mispaired whenever the extractor and renderer disagreed on counts (e.g. blockquote-nested tables, non-conforming separator rows, pre-existing blocks under non-overwrite update modes). - Extract the DOCX table block_type literal 31 into a named constant `docxBlockTypeTable` alongside `widthRatioTotal`. - Treat Unicode Cf (format, e.g. ZWJ U+200D), Mn (combining marks) and Me (enclosing marks) as zero-width in `visualWidth`, so ZWJ emoji sequences and emoji variation selectors (U+FE0F) no longer inflate per-cell widths. - Emit a user-facing stderr notice when `--auto-table-widths=true` is passed to `docs +update` with a wiki-backed --doc, explaining why the adjustment was skipped. - Add test coverage for the blank-line-separated and fence-terminated table flush paths in extractMarkdownTables. Test Plan - go test ./shortcuts/doc/... - golangci-lint run --new-from-rev=origin/main ./shortcuts/doc/...: 0 issues
|
Addressed the review in 78c9276: Actionable
Nitpicks handled
Deferred
|
There was a problem hiding this comment.
🧹 Nitpick comments (3)
shortcuts/doc/table_widths.go (3)
271-299: Silent skip on per-table column-count mismatch is hard to diagnose.When
remote[i].ColumnSize != len(ratios)the table is dropped without any notice, and the finalcolumn widths applied to %d/%dline just reports a lower numerator with no hint why. Given the whole-document count precondition already emits a clear reason when it aborts, it's odd that individual skips here are invisible — a user seeing4/6 tableshas no actionable signal. A one-liner toErrOut(once per skipped table, or aggregated) would close that gap cheaply.♻️ Proposed change
if remote[i].ColumnSize != len(ratios) { + fmt.Fprintf( + runtime.IO().ErrOut, + "column-width skipped for table %d: local has %d columns but remote block %s has %d\n", + i+1, len(ratios), remote[i].BlockID, remote[i].ColumnSize, + ) continue }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/doc/table_widths.go` around lines 271 - 299, When iterating tables in the loop in table_widths.go, add a diagnostic log to report when a table is skipped due to a column-count mismatch (the check remote[i].ColumnSize != len(ratios)); specifically, before the continue, write a single-line message to runtime.IO().ErrOut (same sink used elsewhere) that includes the BlockID (remote[i].BlockID), expected ColumnSize and actual len(ratios) so users know why that table was not patched; keep the existing computeWidthRatios, body construction, and runtime.CallAPI flow unchanged and only add the ErrOut write for the mismatch case.
30-68: Consider requiring a GFM separator row before committing a table.
extractMarkdownTablesaccepts any run ofisPipeTableRowlines as a table, without ever verifying that the second row is a separator (|---|---|). Prose like a stray| foo | bar |line, a pipe-formatted log excerpt, or even a quoted"| a | b |"sentence will therefore count as a 1-row "table" locally, while the Lark renderer — which follows GFM and requires a separator — will not produce a corresponding table block. Because the count-mismatch guard at Line 263 is now an all-or-nothing gate, a single false-positive anywhere in the document aborts width adjustment for every real table. Requiring a valid separator row on the second line (using the existingisPipeSeparatorRow) before flushing would make extraction match server-side table recognition and keep the happy path robust.♻️ Sketch
Track header-candidate state and only commit a table once a separator is confirmed (or drop
currentif the next line isn't a separator and the current buffer has only the header).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/doc/table_widths.go` around lines 30 - 68, extractMarkdownTables currently treats any consecutive isPipeTableRow lines as a table which yields false positives; change the parsing to require a GFM separator row (use isPipeSeparatorRow) as the second line before committing a table: when you see the first pipe row, treat it as a header candidate (do not yet append to tables), on the next non-fence line check isPipeSeparatorRow — if true, then keep header and subsequent rows (use splitPipeRow as now) and append rows to current until a non-pipe row ends the table; if the next line is not a separator and current only contains the header candidate, discard it (do not append to tables); update state tracking around current/inFence accordingly in extractMarkdownTables so only confirmed header+separator sequences produce a table.
202-232:isWideRunemisses several double-width emoji/symbol blocks that render as 2 columns in terminals.The current ranges cover BMP CJK and part of the SMP emoji block (
0x1F300–0x1FAFF), but omit Unicode ranges that are marked East Asian Wide and render double-width in monospace terminals:
0x1F1E6–0x1F1FF— Regional Indicator Symbols (flag pairs like 🇺🇸 🇯🇵)0x2600–0x26FF— Miscellaneous Symbols (☀️ ☁️ ⚡ ⭐ ♻️)0x2700–0x27BF— Dingbats (✅✈️ ✂️ ✏️)0x1F000–0x1F2FF— Mahjong/Domino/Playing Cards + Enclosed Alphanumeric (🀄 🃏🅰️ 🆗)Cells containing these characters count as 1 rune each, causing columns with emoji-heavy content to be under-sized. For emoji-text tables, visible column widths will not match their content. Expanding the switch statement or importing
golang.org/x/text/width(already a project dependency) would align with Unicode East Asian Width standards and improve rendering fidelity.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/doc/table_widths.go` around lines 202 - 232, The isWideRune function misses several emoji/symbol blocks; replace its manual-range-only logic by using golang.org/x/text/width to detect East Asian Wide/Fullwidth (e.g. width.LookupRune(r).Kind() == width.EastAsianWide or width.EastAsianFullwidth) and additionally include explicit checks for the emoji/symbol ranges that may not be classified as wide by lookup: 0x1F1E6–0x1F1FF (Regional Indicator), 0x2600–0x26FF (Misc Symbols), 0x2700–0x27BF (Dingbats) and 0x1F000–0x1F2FF (Mahjong/Playing Cards/Enclosed), so update isWideRune to return true for those ranges or when width.LookupRune reports EastAsianWide/Fullwidth.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@shortcuts/doc/table_widths.go`:
- Around line 271-299: When iterating tables in the loop in table_widths.go, add
a diagnostic log to report when a table is skipped due to a column-count
mismatch (the check remote[i].ColumnSize != len(ratios)); specifically, before
the continue, write a single-line message to runtime.IO().ErrOut (same sink used
elsewhere) that includes the BlockID (remote[i].BlockID), expected ColumnSize
and actual len(ratios) so users know why that table was not patched; keep the
existing computeWidthRatios, body construction, and runtime.CallAPI flow
unchanged and only add the ErrOut write for the mismatch case.
- Around line 30-68: extractMarkdownTables currently treats any consecutive
isPipeTableRow lines as a table which yields false positives; change the parsing
to require a GFM separator row (use isPipeSeparatorRow) as the second line
before committing a table: when you see the first pipe row, treat it as a header
candidate (do not yet append to tables), on the next non-fence line check
isPipeSeparatorRow — if true, then keep header and subsequent rows (use
splitPipeRow as now) and append rows to current until a non-pipe row ends the
table; if the next line is not a separator and current only contains the header
candidate, discard it (do not append to tables); update state tracking around
current/inFence accordingly in extractMarkdownTables so only confirmed
header+separator sequences produce a table.
- Around line 202-232: The isWideRune function misses several emoji/symbol
blocks; replace its manual-range-only logic by using golang.org/x/text/width to
detect East Asian Wide/Fullwidth (e.g. width.LookupRune(r).Kind() ==
width.EastAsianWide or width.EastAsianFullwidth) and additionally include
explicit checks for the emoji/symbol ranges that may not be classified as wide
by lookup: 0x1F1E6–0x1F1FF (Regional Indicator), 0x2600–0x26FF (Misc Symbols),
0x2700–0x27BF (Dingbats) and 0x1F000–0x1F2FF (Mahjong/Playing Cards/Enclosed),
so update isWideRune to return true for those ranges or when width.LookupRune
reports EastAsianWide/Fullwidth.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7781b5f9-2f5f-431a-b4b9-f5236d72cb89
📒 Files selected for processing (3)
shortcuts/doc/docs_update.goshortcuts/doc/table_widths.goshortcuts/doc/table_widths_test.go
✅ Files skipped from review due to trivial changes (1)
- shortcuts/doc/table_widths_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- shortcuts/doc/docs_update.go
…-table diag Address the second review pass on larksuite#557: - extractMarkdownTables now requires a GFM separator row (`|---|---|`) after a candidate header before committing a table. Stray pipe lines (prose, log excerpts, single-row tables without a separator) no longer count, so the whole-document count precondition in applyMarkdownTableColumnWidths isn't tripped by false positives. Added tests: pipe row without separator, two pipe rows without separator, and a dangling pipe line followed by a real table. - isWideRune now covers Misc Symbols (U+2600-26FF), Dingbats (U+2700-27BF), Mahjong/Playing Cards/Enclosed Alphanumeric (U+1F000-1F2FF) and Regional Indicator Symbols (U+1F1E6-1F1FF). Cells with flags (🇺🇸), dingbats (✅), or mahjong tiles no longer under-weight their columns. Added visualWidth cases for each. - applyMarkdownTableColumnWidths now prints a per-table diagnostic on stderr when a column-count mismatch causes the block to be skipped, so users seeing "column widths applied to N/M" have an actionable signal for the gap. Test Plan - go test ./shortcuts/doc/... - golangci-lint run --new-from-rev=origin/main ./shortcuts/doc/...: 0 issues
|
R2 review addressed in 6fab603:
All new tests pass, |
Use width.LookupRune for the Unicode East Asian Width classification (already a direct dependency via go.mod) rather than hand-maintained BMP ranges. The CJK / Hangul / full-width coverage is now standards- backed and no longer silently skews when new Unicode versions land. Keep explicit range checks only for blocks Unicode classifies as "neutral" but terminals and the Lark renderer show at 2 columns: Misc Symbols, Dingbats, Mahjong/Playing Cards/Enclosed Alphanumeric, Emoji pictographs, and Regional Indicator Symbols. Test Plan - go test ./shortcuts/doc/... - golangci-lint run --new-from-rev=origin/main ./shortcuts/doc/...: 0 issues - go mod tidy: no go.mod / go.sum changes (x/text was already direct dep)
|
Correction to my last comment — |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@shortcuts/doc/table_widths_test.go`:
- Around line 72-78: Add the suggested regression test case that asserts a table
inside a longer fence (````md ... ````) is ignored by extractMarkdownTables and
then refactor the fence scanning logic used by extractMarkdownTables to use a
helper detectFence(line string) (isFence bool, prefix string, fenceChar rune,
count int); detectFence must correctly identify fence character (` and ~), the
fence prefix (language id), and the fence length so the parser only closes a
fence when an equal-or-longer fence of the same char is seen — update the parser
to store the active fenceChar and count and to call detectFence for each line to
decide entering/exiting fences, preventing shorter inner fence markers from
terminating a longer fence.
- Around line 173-196: The test must cover the impossible case where number of
columns exceeds widthRatioTotal (so the per-column floor of 1 makes sum>100):
add a sample with >100 columns in the "sum always equals 100..." table test and
assert the behavior you choose (either expect computeWidthRatios(rows) to return
nil/skip or update computeWidthRatios to detect columns > widthRatioTotal and
return nil or an error before computing ratios); modify computeWidthRatios to
explicitly handle this condition (check len(rows[0]) > widthRatioTotal) and
return nil or an error to satisfy the new regression test.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ef94c96a-2e66-4382-b2b8-f9c38c02bf1b
📒 Files selected for processing (2)
shortcuts/doc/table_widths.goshortcuts/doc/table_widths_test.go
✅ Files skipped from review due to trivial changes (1)
- shortcuts/doc/table_widths.go
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@shortcuts/doc/table_widths.go`:
- Around line 56-61: The fence handling toggles inFence on any line starting
with "```" or "~~~" which allows a tilde fence inside a backtick fence to
prematurely close it; update the block that checks strings.HasPrefix(trimmed,
"```") || strings.HasPrefix(trimmed, "~~~") to capture the opening marker
(marker := trimmed[:3]) and, if already inFence, ignore markers that don't match
fenceMarker, then set/clear inFence and fenceMarker explicitly instead of blind
toggling (use flushCurrent(), header = nil, and then if inFence set
inFence=false and fenceMarker="" else set inFence=true and fenceMarker=marker)
so fences only close with their original marker and nested/different markers are
ignored.
- Around line 187-209: Detect and short-circuit the impossible case where cols >
widthRatioTotal before doing the "at least 1 per column" apportionment: in the
function that computes ratios (uses variables cols, widthRatioTotal, weights,
ratios, total) add an early check if cols > widthRatioTotal and return a
sensible value (e.g., nil/empty slice or an error/zeroed ratios depending on the
function's contract) so we don't enforce a minimum of 1 per column and produce a
sum > widthRatioTotal; this keeps the apportionment loop and the widest-column
adjustment unchanged but avoids creating impossible positive-ratio tables.
- Around line 77-95: When confirming a GFM table (the branch that sets current =
[][]string{header}), lock the expected column count to len(header) and ensure
subsequent body rows appended via splitPipeRow(trimmed) are
normalized/truncated/padded to that column count before adding to current so
computeWidthRatios receives the correct width for the rendered table; also when
detecting a separator with isPipeSeparatorRow(trimmed) while header != nil,
reject the separator if splitPipeRow(trimmed) yields a different cell count than
header (i.e., treat it as not a separator and continue treating the line as a
new header candidate). Apply these changes around the header/current handling
and where rows are appended so computeWidthRatios only sees rows matching the
confirmed column count.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0421c941-3bf6-44a5-ac64-4cb21ccca18b
📒 Files selected for processing (1)
shortcuts/doc/table_widths.go
Track opening fence character and minimum run-length so that a mismatched marker (~~~ inside ```) or a shorter backtick run (``` inside `````) no longer prematurely closes a fenced code block, preventing false table extraction from code content. Also return nil from computeWidthRatios when the column count exceeds widthRatioTotal (100): each column must receive at least ratio 1, so the sum would exceed 100 and the resulting slice would be invalid. Add two regression tests for the fence cases and one for the >100-column invariant. https://claude.ai/code/session_019vpaETo31vQCjMfprmakYd
|
|
fangshuyu-768
left a comment
fangshuyu-768
left a comment
There was a problem hiding this comment.
Nice feature — the core parser/ratio logic is well-scoped and the tests cover the tricky GFM edge cases. A few things worth a second look before merge: one correctness nit in computeWidthRatios (sum-to-100 invariant can break under the <1 clamp), the wiki-URL gap on docs +update, over-counting of ZWJ/flag emoji in visualWidth, and no tests around the orchestration path (where index-matching and per-table failures actually live). Nothing blocking if the team is fine with those caveats; inline comments below.
| ratios[widest] += diff | ||
| if ratios[widest] < 1 { | ||
| ratios[widest] = 1 | ||
| } |
There was a problem hiding this comment.
Nit — correctness: this <1 clamp after the diff adjustment silently breaks the documented sum-to-100 invariant. The widest column's ratio is usually large enough that diff can't push it below 1, but the clamp exists specifically for the case where it could — and when it fires, the returned slice no longer sums to 100, which is what the API expects.
Constructing a real failure is hard (you'd need many clamped-up tiny columns driving allocated far above 100), but if it ever happens the CLI would quietly write an invalid width_ratios array. Two options:
- Drop the clamp here and, if
ratios[widest]+diff < 1, returnnil(skip the patch) — safer than writing a bad ratio. - Redistribute the leftover deficit across the other columns instead of piling it all on the widest.
Either way, a short assertion-style test (sum == 100 on a pathological weight distribution) would lock it in.
There was a problem hiding this comment.
Went with option 1 in ae21ab0: if ratios[widest]+diff < 1 after the apportionment adjustment, computeWidthRatios now returns nil rather than silently writing an invalid width_ratios array. Added a pathological sample (one wide column + 49 skinny ones) to the sum always equals 100 subtest so future regressions trip the assertion. Combined with the new cols > widthRatioTotal short-circuit, both paths out of the invariant are locked down.
| case r >= 0x1F300 && r <= 0x1FAFF: // Emoji & pictographs | ||
| return true | ||
| case r >= 0x1F1E6 && r <= 0x1F1FF: // Regional Indicator Symbols (flag halves) | ||
| return true |
There was a problem hiding this comment.
Minor: Regional Indicator pairs (flags) and emoji ZWJ sequences overcount here. 🇺🇸 yields 4 (two RIs × 2), and 👨👩👧👦 yields 8 (four base emoji × 2, ZWJ=0), but both render as one grapheme cluster of visual width 2. Since these widths are only used as weights for proportional ratios, it's not catastrophic — but a cell that's mostly flags will claim disproportionately more column width than the eye expects.
Not necessarily a blocker (tables of flags are rare), but either (a) a TODO comment pointing at golang.org/x/text/unicode/norm + grapheme-cluster segmentation, or (b) a test asserting the current (known-overcounting) behavior so it doesn't silently change, would be worth adding.
There was a problem hiding this comment.
Added explicit documentation of the known overcount on visualWidth in ae21ab0 (Regional-Indicator pairs count 4 instead of 2, four-person ZWJ family counts 8 instead of 2) pointing at grapheme-cluster segmentation (rivo/uniseg) as the follow-up. The existing visualWidth test cases now pin the current behaviour (🇺🇸 → 4) so any change to this policy trips a failing test. Kept it in-tree rather than pulling a new dep for a proportional-weighting edge case.
| // Only tables whose local column count equals the remote table's column_size | ||
| // are patched; mismatches are skipped. Tables are matched to remote table | ||
| // blocks by document-order index. | ||
| func applyMarkdownTableColumnWidths(runtime *common.RuntimeContext, documentID, markdown string) { |
There was a problem hiding this comment.
Coverage gap: extractMarkdownTables, computeWidthRatios, and visualWidth all have solid unit tests, but applyMarkdownTableColumnWidths — where the actual risk lives (index-based matching, strict remote-vs-local count check, per-table PATCH-failure handling, column_size mismatch skip) — has none.
A table-driven test with a fake runtime.CallAPI (or a narrow interface around the blocks fetch + PATCH) would let you pin:
- skip when
len(remote) != len(tables), - skip (and continue) when
column_sizediffers for a given pair, - non-fatal on per-block PATCH error,
- exit status unchanged on any failure.
Given this is the subtlest part of the change and runs on every +create/overwrite, I'd strongly prefer at least one integration-style test here.
There was a problem hiding this comment.
Addressed in ae21ab0: added TestApplyTableColumnWidths with a fakeAPI stubbing runtime.CallAPI. Covers happy path, whole-doc count mismatch, per-table column_size mismatch, non-fatal per-block PATCH error, and blocks-fetch error. Also added TestDocxTokenForAutoWidths / TestResolveDocxTokenForCreateResult for the token resolvers. The orchestration code was refactored to accept an internal apiCaller function type that *common.RuntimeContext.CallAPI already satisfies, so the public entry points didn't change.
| normalizeDocsUpdateResult(result, runtime.Str("markdown")) | ||
| if runtime.Bool("auto-table-widths") { | ||
| if docID := strings.TrimSpace(common.GetString(result, "doc_id")); docID != "" { | ||
| applyMarkdownTableColumnWidths(runtime, docID, runtime.Str("markdown")) |
There was a problem hiding this comment.
Consider validating doc_id is actually a docx token before calling into the docx blocks API. For --wiki-node / --wiki-space creates, if the MCP response ever surfaces a wiki token (or anything non-docx) in doc_id, the subsequent GET /open-apis/docx/v1/documents/{id}/blocks 404s and we emit an error-shaped log line — which is then non-fatal per design, but reads like a real failure to the user.
Cleaner: pass doc_url (or doc_id) through parseDocumentRef, and only proceed if ref.Kind == "docx". That mirrors what selectDocsPermissionTarget already does and produces a clean skip message instead of an API error log in the wiki-create path.
There was a problem hiding this comment.
Addressed in ae21ab0: docs +create now routes the docx token via resolveDocxTokenForCreateResult, which prefers the MCP result's doc_url + parseDocumentRef (same pattern as selectDocsPermissionTarget). Wiki URLs take one /open-apis/wiki/v2/spaces/get_node hop and fall back to doc_id only when no doc_url is returned. No more spurious 404 on wiki-backed creates.
| fmt.Fprintln( | ||
| runtime.IO().ErrOut, | ||
| "auto-table-widths skipped: --doc is not a docx token/URL (wiki-backed docs require an extra resolve step that is not implemented yet)", | ||
| ) |
There was a problem hiding this comment.
The skip message is honest, but docs +update already accepts wiki URLs end-to-end in the main path (the MCP update-doc handles them), so any user who habitually passes /wiki/... URLs will always hit this skip on overwrite — silently getting the old equal-width behavior while the create path auto-adjusts. That's an easy-to-miss inconsistency.
There's already a wiki→obj resolution pattern in shortcuts/wiki (/open-apis/wiki/v2/spaces/get_node) returning the obj_token / obj_type. One extra call on the overwrite path would let you handle wiki URLs uniformly instead of punting. If you'd rather not expand scope in this PR, please at least link a follow-up issue — the PR description mentions this as future work but there's no tracking link.
There was a problem hiding this comment.
Addressed in ae21ab0: docs +update --mode overwrite now resolves wiki URLs to their backing docx via the same /open-apis/wiki/v2/spaces/get_node call, so --auto-table-widths behaves uniformly across create and update, docx and wiki. The skip path only fires for genuinely non-docx inputs (e.g. sheet-backed wiki nodes), and its message was trimmed to 'could not resolve --doc to a docx token'.
| {Name: "folder-token", Desc: "parent folder token"}, | ||
| {Name: "wiki-node", Desc: "wiki node token"}, | ||
| {Name: "wiki-space", Desc: "wiki space ID (use my_library for personal library)"}, | ||
| {Name: "auto-table-widths", Type: "bool", Default: "true", Desc: "after create, adjust Markdown pipe-table column widths based on cell content (use --auto-table-widths=false to keep server default equal widths)"}, |
There was a problem hiding this comment.
The default is true, which changes the rendered shape of every newly-created/overwritten doc with tables. Users with scripted workflows (golden fixtures, screenshot regressions, reviewer tooling that diffs rendered docs) will see output change without opting in.
The opt-out knob is clearly documented in the flag description — good — but I'd also call this behavior change out explicitly in the CHANGELOG / release notes so it doesn't surprise anyone who doesn't read flag help. Not blocking.
There was a problem hiding this comment.
Fair point — keeping the default as true because the old equal-width rendering is often visibly worse and the opt-out is documented in the flag description, but the behaviour change will be called out in the release notes when this ships. I didn't edit CHANGELOG.md directly because the repo's convention is to accumulate release notes in the release-cut PR (see the v1.0.14 section header); happy to add an explicit CHANGELOG entry here if maintainers prefer.
Third review pass on larksuite#557. Covers coderabbit R3/R4 and the human review from @fangshuyu-768. - computeWidthRatios: short-circuit to nil when the column count exceeds widthRatioTotal, since honouring the ≥1-per-column floor and the "sum == widthRatioTotal" contract is impossible beyond 100 columns. Also return nil if the final rounding adjustment would push the widest column's ratio below 1 — safer than writing a width_ratios array the API will reject. - extractMarkdownTables: track the opening fence character (backtick vs tilde) and length; a fence only closes on a matching marker of equal-or-longer length so longer fences containing shorter inner markers stay open. Reject separator rows whose cell count disagrees with the pending header. Normalise body rows to the confirmed column count so stray extra cells don't produce a ratio list that the subsequent PATCH would skip over. - docs +create: resolve the docx token via parseDocumentRef on the MCP result's doc_url first, so wiki-created docs route through a single /open-apis/wiki/v2/spaces/get_node call instead of 404-ing on the docx blocks API. - docs +update --mode overwrite: wiki URLs now resolve to their backing docx and run auto-widths uniformly with the docx path; the skip message only fires for truly non-docx inputs. - Introduced an internal `apiCaller` function type that `*common .RuntimeContext.CallAPI` already satisfies, and split the inner logic of applyMarkdownTableColumnWidths / fetchTableBlocks / docxTokenForUpdate / resolveDocxTokenForCreateResult to accept it. This keeps the public entry points unchanged while letting tests exercise the orchestration without touching the network. - New TestApplyTableColumnWidths: happy path, count mismatch abort, per-table column-size skip with diagnostic, non-fatal per-block PATCH error, blocks-fetch error skips the pass. - New TestDocxTokenForAutoWidths / TestResolveDocxTokenForCreate Result: docx short-circuit, wiki resolution, non-docx wiki node skip, resolve error skip. - Added regression tests for the extractor hardening: longer fence survives inner shorter marker; tilde inner marker doesn't close a backtick fence; separator-row cell count disagreement rejects the table; body row truncated to confirmed column count. - Added computeWidthRatios tests: nil for >widthRatioTotal columns, exact-widthRatioTotal case still sums to 100, and a pathological many-skinny-columns sample driving the clamp repeatedly. - Added a TODO note to visualWidth explaining the known Regional-Indicator / ZWJ-sequence overcount and pointing at grapheme-cluster segmentation as the follow-up. Test Plan - go test ./shortcuts/doc/... - golangci-lint run --new-from-rev=origin/main ./shortcuts/doc/...: 0 issues - go mod tidy: no go.mod / go.sum changes
|
Third review pass (coderabbit R3/R4 + @fangshuyu-768) addressed in ae21ab0. Also picked up commit 53771a7 from the earlier scheduled pass via rebase — its fence-marker / >100-col handling is retained but extended with row-normalisation, separator-cell-count validation, and additional tests. Correctness
Orchestration & wiki support
Testability
Docs
All new tests pass; |
4 participants
Summary
docs +createanddocs +update --mode overwritecurrently upload Markdown tables that render with equal column widths (e.g. 244/244/244 for a 3-column table) regardless of how uneven the cell content is. Users have to manually drag columns in the UI to get a readable layout.This PR adds a local pipe-table parser and a content-aware width-ratio calculator. After the MCP create/update call succeeds, the CLI walks the document's table blocks in order, matches each against the corresponding Markdown table by column count, and PATCHes `update_grid_column_width_ratio` with ratios summing to 100.
Changes
Why only overwrite for `+update`
For `append` / `insert_*` / `replace_range`, only some table blocks in the document are newly created and identifying them from the MCP response is ambiguous (the response does not return the new block_ids). Rather than patch existing tables incorrectly, those modes ignore `--auto-table-widths` for now. Follow-up: a before/after table-count diff could cover `append`.
Test Plan
Related
Notes
Summary by CodeRabbit
New Features
Tests