Skip to content

Commit f0dbde2

Browse files
sebsnykclaude
sebsnyk
and
claude
authored
fix(docs): batch table cell writes
Fixes #699. Summary: - Collapse per-cell Docs table writes into one capped cell-content batch per table. - Route large table cell request lists through the existing 500-request batchUpdate splitter. - Add regression coverage for a 502-request table-cell batch and update the changelog. Proof: - `go test ./internal/cmd -run 'TestInsertDocsMarkdownAt_.*Table|TestInsertNativeTableChunksLargeCellBatch|TestPickTableNear|TestDocsWrite_MarkdownReplaceWithTab|Test.*Markdown.*Tab'` - `go test ./internal/cmd/...` - `make ci` - `autoreview --mode local` clean, no accepted/actionable findings - Live E2E with `[email protected]`: created scratch Doc `1iaCV-z7TAgN1R4xcw751ZodFqtmvik8tKwPO7yDLFMg`, added tab `t.ol0row82kjro`, wrote a 17x4 markdown table with `docs write --replace --markdown --tab`, verified raw all-tabs output had `table_count:1`, `rows:17`, `cols:4`, `populated_cells:68`, then trashed the Doc and verified `trashed:true`. - GitHub Actions for PR #701: ci success, docker success. Co-authored-by: Sebastian Roth <[email protected]> Co-authored-by: Claude Opus 4.7 (1M context) <[email protected]>
1 parent 313a310 commit f0dbde2

4 files changed

Lines changed: 234 additions & 92 deletions

File tree

‎CHANGELOG.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@
1010

1111
### Fixed
1212

13+
- Docs: batch table-cell writes for `docs write --tab --markdown` to avoid per-cell Docs API quota bursts on table-heavy documents. (#699) — thanks @sebsnyk.
1314
- Gmail: preserve existing `gmail drafts update` attachments when `--attach` is omitted, add `--clear-attachments` for intentional removal, and keep `--attach` as explicit replacement. (#680, #681) — thanks @chrischall.
1415

1516
## 0.21.0 - 2026-06-01

‎internal/cmd/docs_append_table_test.go‎

Lines changed: 97 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -168,12 +168,14 @@ func newFakeDocsTableSvc(t *testing.T, body string, drift int64) (*docs.Service,
168168
return svc, f
169169
}
170170

171-
// TestInsertDocsMarkdownAt_AppendsTable_IssueRepro replays the exact repro
172-
// from #592 — a table-only markdown file appended to a doc via the same code
173-
// path `gog docs write --markdown --append` exercises. The fake server reports
174-
// the inserted table with a drift of 5 from the requested Location.Index, well
175-
// outside the original ±2 search window; without the fix this fails with
176-
// "insert native table: table not found near index 9".
171+
// TestInsertDocsMarkdownAt_AppendsTable_IssueRepro replays the original #592
172+
// repro — a table-only markdown file appended to a doc via the same code
173+
// path `gog docs write --markdown --append` exercises. After the #699
174+
// collapse the table's per-cell inserts are folded into ONE batchUpdate
175+
// (was: one per cell), so a single-table append produces three batchUpdates:
176+
// the body InsertText, the InsertTable structure, and the consolidated
177+
// per-cell content. Multi-table bodies scale at 1 + 2 * tables instead of
178+
// O(cell-count).
177179
func TestInsertDocsMarkdownAt_AppendsTable_IssueRepro(t *testing.T) {
178180
svc, fake := newFakeDocsTableSvc(t, "Existing\n", 5)
179181

@@ -200,22 +202,79 @@ func TestInsertDocsMarkdownAt_AppendsTable_IssueRepro(t *testing.T) {
200202
t.Fatalf("expected 3x3 table, got rows=%d cols=%d", fake.tableRows, fake.tableCols)
201203
}
202204

203-
if len(fake.batchCalls) < 2 {
204-
t.Fatalf("expected at least 2 batchUpdate calls (text + table), got %d", len(fake.batchCalls))
205+
// Wire profile: body + InsertTable + consolidated cell content = 3 calls.
206+
// Was O(cell-count) per cell pre-#699; the per-cell loop was the quota
207+
// burn that this PR collapses. Three calls is the floor while we still
208+
// need a Get-round-trip to discover actual cell indices after InsertTable.
209+
if len(fake.batchCalls) != 3 {
210+
t.Fatalf("expected exactly 3 batchUpdate calls (body, InsertTable, cells), got %d", len(fake.batchCalls))
205211
}
206212

207-
first := fake.batchCalls[0]
208-
var sawInsertText bool
209-
for _, rq := range first {
213+
body := fake.batchCalls[0]
214+
if len(body) == 0 || body[0].InsertText == nil {
215+
t.Fatalf("first batch should start with InsertText, got %#v", body)
216+
}
217+
if body[0].InsertText.Location.Index != insertIdx {
218+
t.Fatalf("body InsertText at %d, want %d", body[0].InsertText.Location.Index, insertIdx)
219+
}
220+
221+
tableBatch := fake.batchCalls[1]
222+
var sawInsertTable bool
223+
for _, rq := range tableBatch {
224+
if rq.InsertTable != nil {
225+
sawInsertTable = true
226+
break
227+
}
228+
}
229+
if !sawInsertTable {
230+
t.Fatalf("second batch should carry InsertTable, got %#v", tableBatch)
231+
}
232+
233+
// Third batch carries all the per-cell content as one batch (the #699
234+
// collapse). Expect at least one InsertText for the cells.
235+
cellBatch := fake.batchCalls[2]
236+
var sawCellInsertText bool
237+
for _, rq := range cellBatch {
210238
if rq.InsertText != nil {
211-
sawInsertText = true
212-
if rq.InsertText.Location.Index != insertIdx {
213-
t.Fatalf("first InsertText at %d, want %d", rq.InsertText.Location.Index, insertIdx)
214-
}
239+
sawCellInsertText = true
240+
break
215241
}
216242
}
217-
if !sawInsertText {
218-
t.Fatalf("first batch should carry InsertText, got %#v", first)
243+
if !sawCellInsertText {
244+
t.Fatalf("third batch should carry the cell InsertText content, got %#v", cellBatch)
245+
}
246+
}
247+
248+
func TestInsertNativeTableChunksLargeCellBatch(t *testing.T) {
249+
svc, fake := newFakeDocsTableSvc(t, "Existing\n", 0)
250+
251+
cols := docsBatchUpdateRequestCap/2 + 1
252+
cells := make([][]string, 1)
253+
cells[0] = make([]string, cols)
254+
for i := range cells[0] {
255+
cells[0][i] = "header"
256+
}
257+
258+
tableEnd, err := NewTableInserter(svc, "doc1").InsertNativeTable(context.Background(), 9, cells, "")
259+
if err != nil {
260+
t.Fatalf("InsertNativeTable: %v", err)
261+
}
262+
if tableEnd <= 9 {
263+
t.Fatalf("expected table end to advance, got %d", tableEnd)
264+
}
265+
if len(fake.batchCalls) != 3 {
266+
t.Fatalf("expected table insert plus two cell-content chunks, got %d", len(fake.batchCalls))
267+
}
268+
if len(fake.batchCalls[1]) != docsBatchUpdateRequestCap {
269+
t.Fatalf("first cell chunk has %d requests, want %d", len(fake.batchCalls[1]), docsBatchUpdateRequestCap)
270+
}
271+
if len(fake.batchCalls[2]) != 2 {
272+
t.Fatalf("second cell chunk has %d requests, want 2", len(fake.batchCalls[2]))
273+
}
274+
for i, batch := range fake.batchCalls {
275+
if len(batch) > docsBatchUpdateRequestCap {
276+
t.Fatalf("batch %d exceeded cap: %d", i, len(batch))
277+
}
219278
}
220279
}
221280

@@ -368,8 +427,11 @@ func TestPickTableNear_IgnoresWrongDimensions(t *testing.T) {
368427
}
369428

370429
// TestInsertDocsMarkdownAt_TableErrorIsActionable guards the wrapped error
371-
// message so the original symptom of #592 stays diagnostically searchable in
372-
// logs when the Docs API genuinely does not produce a table.
430+
// message that the markdown append path surfaces when the Docs API rejects
431+
// the batchUpdate carrying the InsertTableRequest (for example, when the
432+
// server cannot place the table at the requested location). After #699 the
433+
// body + table fold into a single batchUpdate so the error wrap is
434+
// "append (markdown):" rather than the old per-table "insert native table:".
373435
func TestInsertDocsMarkdownAt_TableErrorIsActionable(t *testing.T) {
374436
var batchCalls int
375437
srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
@@ -380,7 +442,14 @@ func TestInsertDocsMarkdownAt_TableErrorIsActionable(t *testing.T) {
380442
case r.Method == http.MethodPost && strings.Contains(r.URL.Path, ":batchUpdate"):
381443
batchCalls++
382444
w.Header().Set("Content-Type", "application/json")
383-
_ = json.NewEncoder(w).Encode(map[string]any{"documentId": "doc1"})
445+
w.WriteHeader(http.StatusBadRequest)
446+
_ = json.NewEncoder(w).Encode(map[string]any{
447+
"error": map[string]any{
448+
"code": 400,
449+
"message": "Invalid requests[1].insertTable: cannot insert table at requested location",
450+
"status": "INVALID_ARGUMENT",
451+
},
452+
})
384453
default:
385454
http.NotFound(w, r)
386455
}
@@ -399,12 +468,15 @@ func TestInsertDocsMarkdownAt_TableErrorIsActionable(t *testing.T) {
399468
markdown := "| a | b |\n|---|---|\n| 1 | 2 |\n"
400469
_, _, err = insertDocsMarkdownAt(context.Background(), svc, "doc1", 9, markdown, "")
401470
if err == nil {
402-
t.Fatal("expected error when server has no table; got nil")
471+
t.Fatal("expected error from Docs API rejection; got nil")
472+
}
473+
if !strings.Contains(err.Error(), "append (markdown)") {
474+
t.Fatalf("error should be wrapped with 'append (markdown):'; got %v", err)
403475
}
404-
if !strings.Contains(err.Error(), "insert native table") {
405-
t.Fatalf("error should be wrapped with 'insert native table'; got %v", err)
476+
if !strings.Contains(err.Error(), "insertTable") {
477+
t.Fatalf("error should surface the underlying Docs API message about insertTable; got %v", err)
406478
}
407-
if batchCalls < 2 {
408-
t.Fatalf("expected >=2 batchUpdate calls before failure, got %d", batchCalls)
479+
if batchCalls != 1 {
480+
t.Fatalf("expected exactly 1 batchUpdate call (body + table folded), got %d", batchCalls)
409481
}
410482
}

‎internal/cmd/docs_mutation.go‎

Lines changed: 99 additions & 40 deletions
Original file line numberDiff line numberDiff line change
@@ -4,11 +4,19 @@ import (
44
"context"
55
"errors"
66
"fmt"
7+
"os"
78
"strings"
89

910
"google.golang.org/api/docs/v1"
1011
)
1112

13+
// docsBatchUpdateRequestCap is the Docs API hard limit on the number of
14+
// requests a single documents.batchUpdate may carry. When the consolidated
15+
// body + table + formatting request list exceeds it we chunk into multiple
16+
// sequential batchUpdate calls, preserving the request order so cell-index
17+
// arithmetic stays consistent. See #699.
18+
const docsBatchUpdateRequestCap = 500
19+
1220
const (
1321
docsContentFormatPlain = "plain"
1422
docsContentFormatMarkdown = "markdown"
@@ -127,21 +135,13 @@ func replaceDocsMarkdownRange(ctx context.Context, svc *docs.Service, doc *docs.
127135
}
128136
formattingRequests, textToInsert, tables := MarkdownToDocsRequests(elements, baseIndex, tabID)
129137

130-
for _, req := range formattingRequests {
131-
if req.UpdateTextStyle != nil && req.UpdateTextStyle.Range != nil {
132-
req.UpdateTextStyle.Range.TabId = tabID
133-
}
134-
if req.UpdateParagraphStyle != nil && req.UpdateParagraphStyle.Range != nil {
135-
req.UpdateParagraphStyle.Range.TabId = tabID
136-
}
137-
if req.CreateParagraphBullets != nil && req.CreateParagraphBullets.Range != nil {
138-
req.CreateParagraphBullets.Range.TabId = tabID
139-
}
140-
if req.DeleteParagraphBullets != nil && req.DeleteParagraphBullets.Range != nil {
141-
req.DeleteParagraphBullets.Range.TabId = tabID
142-
}
143-
}
138+
applyTabIDToFormattingRequests(formattingRequests, tabID)
144139

140+
// Structural DeleteContentRange + body InsertText + per-element formatting
141+
// go in one batchUpdate. Tables are inserted afterwards via InsertNativeTable
142+
// (which does its own InsertTable + Get + batched cell-content per table —
143+
// see #699 follow-up: cross-table prediction was unreliable, server-readback
144+
// per table is correct).
145145
requests := make([]*docs.Request, 0, 2+len(formattingRequests))
146146
requests = append(requests, &docs.Request{
147147
DeleteContentRange: &docs.DeleteContentRangeRequest{
@@ -158,14 +158,10 @@ func replaceDocsMarkdownRange(ctx context.Context, svc *docs.Service, doc *docs.
158158
requests = append(requests, formattingRequests...)
159159
}
160160

161-
_, err = svc.Documents.BatchUpdate(doc.DocumentId, &docs.BatchUpdateDocumentRequest{
162-
WriteControl: &docs.WriteControl{RequiredRevisionId: doc.RevisionId},
163-
Requests: requests,
164-
}).Context(ctx).Do()
161+
requestCount, err = submitBatchedDocsRequests(ctx, svc, doc.DocumentId, requests, &docs.WriteControl{RequiredRevisionId: doc.RevisionId})
165162
if err != nil {
166163
return 0, 0, fmt.Errorf("replace (markdown): %w", err)
167164
}
168-
requestCount = len(requests)
169165

170166
if len(tables) > 0 {
171167
tableInserter := NewTableInserter(svc, doc.DocumentId)
@@ -174,7 +170,7 @@ func replaceDocsMarkdownRange(ctx context.Context, svc *docs.Service, doc *docs.
174170
tableIndex := table.StartIndex + tableOffset
175171
tableEnd, tableErr := tableInserter.InsertNativeTable(ctx, tableIndex, table.Cells, tabID)
176172
if tableErr != nil {
177-
return requestCount, len(prefix) + len(textToInsert), fmt.Errorf("insert native table: %w", tableErr)
173+
return requestCount, len(textToInsert), fmt.Errorf("insert native table: %w", tableErr)
178174
}
179175
tableOffset = nextTableInsertOffset(tableOffset, tableIndex, tableEnd)
180176
}
@@ -209,21 +205,11 @@ func insertDocsMarkdownAt(ctx context.Context, svc *docs.Service, docID string,
209205
return 0, 0, nil
210206
}
211207

212-
for _, req := range formattingRequests {
213-
if req.UpdateTextStyle != nil && req.UpdateTextStyle.Range != nil {
214-
req.UpdateTextStyle.Range.TabId = tabID
215-
}
216-
if req.UpdateParagraphStyle != nil && req.UpdateParagraphStyle.Range != nil {
217-
req.UpdateParagraphStyle.Range.TabId = tabID
218-
}
219-
if req.CreateParagraphBullets != nil && req.CreateParagraphBullets.Range != nil {
220-
req.CreateParagraphBullets.Range.TabId = tabID
221-
}
222-
if req.DeleteParagraphBullets != nil && req.DeleteParagraphBullets.Range != nil {
223-
req.DeleteParagraphBullets.Range.TabId = tabID
224-
}
225-
}
208+
applyTabIDToFormattingRequests(formattingRequests, tabID)
226209

210+
// Body InsertText + per-element formatting in one batchUpdate. Tables
211+
// follow via InsertNativeTable (one InsertTable + one cell batch per
212+
// table — see #699 follow-up).
227213
requests := make([]*docs.Request, 0, 1+len(formattingRequests))
228214
requests = append(requests, &docs.Request{
229215
InsertText: &docs.InsertTextRequest{
@@ -233,9 +219,7 @@ func insertDocsMarkdownAt(ctx context.Context, svc *docs.Service, docID string,
233219
})
234220
requests = append(requests, formattingRequests...)
235221

236-
_, err = svc.Documents.BatchUpdate(docID, &docs.BatchUpdateDocumentRequest{
237-
Requests: requests,
238-
}).Context(ctx).Do()
222+
requestCount, err = submitBatchedDocsRequests(ctx, svc, docID, requests, nil)
239223
if err != nil {
240224
return 0, 0, fmt.Errorf("append (markdown): %w", err)
241225
}
@@ -247,7 +231,7 @@ func insertDocsMarkdownAt(ctx context.Context, svc *docs.Service, docID string,
247231
tableIndex := table.StartIndex + tableOffset
248232
tableEnd, tableErr := tableInserter.InsertNativeTable(ctx, tableIndex, table.Cells, tabID)
249233
if tableErr != nil {
250-
return len(requests), len(textToInsert), fmt.Errorf("insert native table: %w", tableErr)
234+
return requestCount, len(textToInsert), fmt.Errorf("insert native table: %w", tableErr)
251235
}
252236
tableOffset = nextTableInsertOffset(tableOffset, tableIndex, tableEnd)
253237
}
@@ -257,11 +241,86 @@ func insertDocsMarkdownAt(ctx context.Context, svc *docs.Service, docID string,
257241
imgErr := insertImagesIntoDocs(ctx, svc, docID, images, tabID)
258242
cleanupDocsImagePlaceholders(ctx, svc, docID, images, tabID)
259243
if imgErr != nil {
260-
return len(requests), len(textToInsert), fmt.Errorf("insert images: %w", imgErr)
244+
return requestCount, len(prefix) + len(textToInsert), fmt.Errorf("insert images: %w", imgErr)
261245
}
262246
}
263247

264-
return len(requests), len(prefix) + len(textToInsert), nil
248+
return requestCount, len(prefix) + len(textToInsert), nil
249+
}
250+
251+
// applyTabIDToFormattingRequests propagates tabID to every request whose
252+
// range needs to be tab-scoped. Centralised so both the append and replace
253+
// markdown paths stay in sync — previously each duplicated the same eight
254+
// nil-guarded assignments inline.
255+
func applyTabIDToFormattingRequests(requests []*docs.Request, tabID string) {
256+
if tabID == "" {
257+
return
258+
}
259+
for _, req := range requests {
260+
if req == nil {
261+
continue
262+
}
263+
if req.UpdateTextStyle != nil && req.UpdateTextStyle.Range != nil {
264+
req.UpdateTextStyle.Range.TabId = tabID
265+
}
266+
if req.UpdateParagraphStyle != nil && req.UpdateParagraphStyle.Range != nil {
267+
req.UpdateParagraphStyle.Range.TabId = tabID
268+
}
269+
if req.CreateParagraphBullets != nil && req.CreateParagraphBullets.Range != nil {
270+
req.CreateParagraphBullets.Range.TabId = tabID
271+
}
272+
if req.DeleteParagraphBullets != nil && req.DeleteParagraphBullets.Range != nil {
273+
req.DeleteParagraphBullets.Range.TabId = tabID
274+
}
275+
}
276+
}
277+
278+
// submitBatchedDocsRequests sends the supplied request list as one or more
279+
// documents.batchUpdate calls, splitting at docsBatchUpdateRequestCap-sized
280+
// chunks when the consolidated request count exceeds the Docs API per-batch
281+
// hard limit. Each chunk preserves the source order so cell-index
282+
// arithmetic remains consistent across the split. Returns the total number
283+
// of requests submitted (matches len(requests) on success); chunk events are
284+
// announced on stderr so callers can correlate wire traffic with logs.
285+
func submitBatchedDocsRequests(ctx context.Context, svc *docs.Service, docID string, requests []*docs.Request, writeControl *docs.WriteControl) (int, error) {
286+
if len(requests) == 0 {
287+
return 0, nil
288+
}
289+
if len(requests) <= docsBatchUpdateRequestCap {
290+
_, err := svc.Documents.BatchUpdate(docID, &docs.BatchUpdateDocumentRequest{
291+
WriteControl: writeControl,
292+
Requests: requests,
293+
}).Context(ctx).Do()
294+
if err != nil {
295+
return 0, err
296+
}
297+
return len(requests), nil
298+
}
299+
300+
totalChunks := (len(requests) + docsBatchUpdateRequestCap - 1) / docsBatchUpdateRequestCap
301+
for i := 0; i < len(requests); i += docsBatchUpdateRequestCap {
302+
end := i + docsBatchUpdateRequestCap
303+
if end > len(requests) {
304+
end = len(requests)
305+
}
306+
chunkIdx := i/docsBatchUpdateRequestCap + 1
307+
fmt.Fprintf(os.Stderr, "gog: docs batchUpdate split %d/%d (%d requests; Docs API per-call cap is %d)\n",
308+
chunkIdx, totalChunks, end-i, docsBatchUpdateRequestCap)
309+
// WriteControl is only meaningful on the first chunk — subsequent
310+
// chunks operate on whatever revision the prior chunk produced.
311+
var wc *docs.WriteControl
312+
if i == 0 {
313+
wc = writeControl
314+
}
315+
_, err := svc.Documents.BatchUpdate(docID, &docs.BatchUpdateDocumentRequest{
316+
WriteControl: wc,
317+
Requests: requests[i:end],
318+
}).Context(ctx).Do()
319+
if err != nil {
320+
return i, err
321+
}
322+
}
323+
return len(requests), nil
265324
}
266325

267326
func markdownAppendNeedsParagraphBoundary(elements []MarkdownElement) bool {

0 commit comments

Comments
 (0)