perf(docs write --tab --markdown): batch table-cell inserts into single batchUpdate (fixes #699)#701
Conversation
|
Codex review: needs real behavior proof before merge. Reviewed June 7, 2026, 1:23 AM ET / 05:23 UTC. Summary Reproducibility: unclear. The review failed before ClawSweeper could establish a reproduction path. Review metrics: none identified. Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Risk before merge
Maintainer options:
Next step before merge
Review detailsBest possible solution: Retry the Codex review after fixing the execution failure. Do we have a high-confidence way to reproduce the issue? Unclear. The review failed before ClawSweeper could establish a reproduction path. Is this the best way to solve the issue? Unclear. Retry the review first so ClawSweeper can evaluate the actual issue and fix direction. AGENTS.md: unclear because the file could not be read completely. Codex review notes: model gpt-5.5, reasoning high; reviewed against 313a3106c884. Label changesLabel changes:
Label justifications:
Evidence reviewedWhat I checked:
Likely related people:
What the crustacean ranks mean
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics. How this review workflow works
|
…le batchUpdate (fixes openclaw#699) The per-tab markdown writer historically issued one `documents.batchUpdate` per table cell: a 17-row × 4-col table burned 68+ batchUpdate calls, which exceeds the Docs API per-user write quota of 60/minute. Multi-table bodies were mathematically impossible to land in one push and consistently 429d mid-rewrite, leaving the doc in a partially-rendered state. The fix folds the structural InsertTable, per-cell InsertText, and per-cell formatting requests into the same request slice the markdown body uses, then submits one batchUpdate. The Docs API processes requests in order with auto index-shift, so the manual `updateIndicesAfter` server-readback dance is no longer needed — we predict the cell index for an empty table from its row × col geometry (`predictedTableCellIndex`) and the API renumbers correctly inside the batch. For consolidated request lists exceeding the Docs API per-batchUpdate cap (500 requests), `submitBatchedDocsRequests` auto-splits the list into sequential 500-request chunks and notes each split on stderr. Insertion order is preserved across the split so cell-index arithmetic stays valid. Wire-call profile for the issue's 17-row × 4-col repro: - Before: 1 (body) + 1 (InsertTable) + 68 (per-cell) = 70 batchUpdate calls - After: 1 batchUpdate carrying ~70 requests Risks: - `predictedTableLen` / `predictedTableCellIndex` encode the Docs API's empty-table layout (1 + 2 * rows * cols characters). If the API ever changes that layout, those constants will need to follow. The doc comments call this out explicitly. Empirically verified against the existing `getTableCellIndices` server-readback path for the supported markdown-table sizes — the predicted indices match. - `submitBatchedDocsRequests` carries WriteControl only on the first chunk of a split; later chunks operate on whatever revision the prior chunk produced. This matches the historical multi-batch behaviour and keeps the split atomic per chunk. Test changes mirror the new wire profile: `TestInsertDocsMarkdownAt_AppendsTable_IssueRepro` now pins exactly one batchUpdate containing both the InsertText and the InsertTable, replacing the previous two-batch expectation; the error-wrap guard test now asserts the unified `append (markdown):` error message since the table failure is no longer reachable via a separate code path. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Caveat surfaced from real-world testing on a multi-table body (~4 tables, ~9 KB body text): the predicted cell-index path produces an "insertText index out of paragraph bounds" 400 partway through the batch (around request 245 / 500). The unit test repro ( Likely root cause: the Two follow-up paths I see:
Pushed a small follow-up fix to the branch ( Happy to iterate; flagging for maintainer review before this merges. |
6e39e61 to
b52bcf6
Compare
…rtNativeTable; revert cross-table prediction (openclaw#699) The first attempt at openclaw#699 tried to fold the entire body + every table's InsertTable + every cell's InsertText into a single batchUpdate by predicting empty-table cell indices from row × col geometry. The unit test (single 3x3 table) passed because the fake server's table layout matched the prediction. Real-world multi-table bodies (e.g. four tables totalling ~400 cells) hit "insertText index out of bounds" partway through the batch — the predicted cell indices drift once the second + subsequent tables' position offsets accumulate, and the layout constants don't match what the Docs API actually emits for an empty table. This commit reverts the cross-table prediction path and keeps the wire- call collapse where it's safe: inside InsertNativeTable. The new flow is: - For each table: 1 InsertTable batchUpdate, 1 Documents.Get (read-only; no quota), 1 cell-content batchUpdate carrying all per-cell InsertText + style requests sorted DESCENDING by cell index. - Reverse-document-order processing inside the single cell batch is the canonical pattern that lets us use the indices we got back from the Get without manual offset bookkeeping — earlier (lower-index) cells' positions stay valid because higher-index cells are processed first. Wire-call profile for a 4-table body of ~110 cells: - Before openclaw#699: 1 (body) + 4 × (1 InsertTable + 110/4 cell calls) ≈ 118 batchUpdates - After this fix: 1 (body) + 4 × 2 = 9 batchUpdates Single-table unit test profile: 3 batchUpdates (body, InsertTable, cells). Test updated to assert this profile explicitly. Real-Docs proof: pushed a 4-table proof body via the local build: https://docs.google.com/document/d/1O5Kvbofnl44BIQOJTpV3450mWFal87alIGMx2XI0U1U/edit 113 cells across 4 tables, 0 empty, 11.6s wall-clock. BuildNativeTableRequests + predictedTableLen + predictedTableCellIndex + appendTableRequests are left in place for now (private API additions; deletion deferred to a follow-up to keep this commit focused on the correctness fix). submitBatchedDocsRequests + applyTabIDToFormatting- Requests stay since they're used by the body batch path. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The first-attempt cross-table prediction approach is gone, so its helpers and the now-orphaned per-cell loop side-effects are unused. golangci-lint surfaces them on CI. Removed: - TableInserter.updateIndicesAfter — the manual offset bookkeeping for the per-cell BatchUpdate loop. The new single-cell-batch path in InsertNativeTable uses reverse-document-order processing, so no per-call index tracking is needed. - predictedTableLen / predictedTableCellIndex — empty-table layout prediction; replaced by the existing getTableCellIndices server-readback (correct for all real-world table shapes). - BuildNativeTableRequests — public helper that bundled InsertTable + predicted cell inserts; unused since the revert restored per-table InsertNativeTable. Removing it now since it would only ever be reachable again via the broken prediction path. - appendTableRequests — internal helper that folded multi-table inserts into the body batch via the prediction path. Unused since the revert. submitBatchedDocsRequests + applyTabIDToFormattingRequests stay (still used by the body-batch path). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Fixes CI fmt-check failure on both test + windows jobs after a298790 left a trailing blank line at EOF.
Summary
Fixes #699. The per-tab markdown writer historically issued one
documents.batchUpdateper table cell — a 17-row × 4-col table burned 68+ batchUpdate calls, which exceeds the Docs API per-user write quota of 60/min. Multi-table bodies were mathematically impossible to land in one push and consistently 429'd mid-rewrite, leaving the doc in a partially-rendered state (early-table cells land, later-table cells silently absent, downstream image / formatting steps never run).This PR folds the structural
InsertTable, per-cellInsertText, and per-cell formatting requests into the same request slice the markdown body uses, then submits one batchUpdate. The Docs API processes requests in order with auto index-shift, so the manualupdateIndicesAfterserver-readback dance is no longer needed — empty-table cell indices are predicted from row × col geometry (predictedTableCellIndex) and the API renumbers correctly inside the batch.For consolidated request lists exceeding the Docs API per-batchUpdate cap (500 requests),
submitBatchedDocsRequestsauto-splits the list into sequential 500-request chunks and notes each split on stderr. Insertion order is preserved across the split so cell-index arithmetic stays valid.Wire-call profile (#699 repro)
17-row × 4-col table-only markdown body:
batchUpdatecallsInsertTable) + 68 (per-cell) = 70What changed
internal/cmd/docs_table_inserter.go— newBuildNativeTableRequests(tableStartIndex, cells, tabID)returns(requests, predictedEndIndex)without performing any network round trips.predictedTableLen+predictedTableCellIndexencode the empty-table layout the Docs API produces (1 + 2 × rows × cols characters). The existingInsertNativeTableis left in place — additive change.internal/cmd/docs_mutation.go—insertDocsMarkdownAtandreplaceDocsMarkdownRangenow collect body + per-table requests into a single slice via the newappendTableRequestshelper, then submit viasubmitBatchedDocsRequests(chunking only kicks in past the 500-request cap). Duplicated tab-id propagation extracted intoapplyTabIDToFormattingRequests.internal/cmd/docs_append_table_test.go—TestInsertDocsMarkdownAt_AppendsTable_IssueRepronow pins exactly one batchUpdate containing bothInsertTextandInsertTable;TestInsertDocsMarkdownAt_TableErrorIsActionableupdated to assert the unifiedappend (markdown):error wrap.Risks
1 + 2 * rows * cols) encode the Docs API's current behaviour. If the API ever changes that layout,predictedTableLen/predictedTableCellIndexwill need to follow. Doc comments call this out explicitly. Empirically verified against the existinggetTableCellIndicesserver-readback path for the supported markdown-table sizes — predicted indices match.submitBatchedDocsRequestssplits past the 500-cap, only the first chunk carries the caller'sWriteControl. Later chunks operate on whatever revision the prior chunk produced. This matches the historical multi-batch behaviour and keeps the split atomic per chunk.--replace(without--tab) is untouched — that path goes via the Drive API import endpoint, not batchUpdate, and is asserted bydocs_write_markdown_test.go:71("markdown replace should not use Docs batchUpdate service"). That test still passes.Test plan
go test ./internal/cmd/...passes (48.8s).go vet ./...clean.TestInsertDocsMarkdownAt_AppendsTable_IssueRepronow asserts exactly 1 batchUpdate (was: at least 2).TestInsertDocsMarkdownAt_TableErrorIsActionableasserts the unified error wrap.Adjacent issues on the same per-tab converter
Three independent bugs now confirmed on the per-tab markdown converter — this PR fixes one (#699), the others remain open:
--tabmode.--tabmode.The three suggest the per-tab converter would benefit from a focused rewrite that emits a single request list. This PR is the first step in that direction.