feat(sheets): add sheets.appendRow write tool#342
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
There was a problem hiding this comment.
Code Review
This pull request introduces a new appendRow tool for Google Sheets, enabling the addition of multiple rows to a spreadsheet. The implementation includes the core service logic, tool registration with Zod validation, and unit tests. Review feedback suggests renaming the tool and its associated methods to appendRows to accurately reflect its multi-row capability and recommends broadening the input schema and TypeScript types to support numbers, booleans, and null values in addition to strings.
Implements the ability to append rows to a Google Sheets spreadsheet via the MCP sheets.appendRow tool. The tool is gated behind the sheets write feature group (disabled by default, scoped to spreadsheets OAuth scope).
f1ea1b6 to
7ecb606
Compare
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Incorporates review feedback: plural name better reflects that multiple rows can be appended at once, and values now accepts numbers, booleans, and nulls in addition to strings.
allenhutchison
left a comment
There was a problem hiding this comment.
Review summary
Clean, idiomatic addition — appendRows mirrors the sibling getRange method exactly (ID extraction via extractDocId, logToFile start/finish/error, the try/catch result shape), the feature gating is correct (sheets/write, defaultEnabled: false, spreadsheets scope), and the happy-path test asserts the full request shape including valueInputOption: 'USER_ENTERED'. Nothing here is merge-blocking. I've left inline notes on a phantom-success edge case, the isError convention, and an empty-values guard; grouping the docs and test items here.
Docs parity (please update before merge)
The repo enumerates every tool per feature group, but the new write tool isn't listed:
docs/index.md(Google Sheets section, ~line 42): add a bullet, e.g.- `sheets.appendRows`: Appends rows to a Google Sheets spreadsheet.docs/feature-configuration.md: there's a### sheets.readsection (line 194) but no### sheets.writesection — every other write group (docs.write,drive.write,gmail.write, …) has one. The scope table (line 25) also lists onlysheets | readwith nosheets | writerow. Both need asheets.writeentry listingsheets.appendRows.
Test coverage
The new tests cover the happy path (with full request-shape assertion), URL-to-ID extraction, and the thrown-error path — good, and consistent with the existing getRange/getMetadata style. Two gaps worth closing:
response.data.updates === undefined— the highest-value addition, directly exercising the phantom-success path flagged inline. Mock{ data: {} }and assert the result reflects "no rows written."- Empty
values: []— documents whether an empty append is rejected locally (it would be, if.min(1)is added) or passed through. - If
isError: trueis added, a regression test assertingresult.isError === trueon the error path would lock it in (seeDriveService.test.ts/DocsService.comments.test.tsfor the pattern).
Minor
- The PR title/body say
sheets.appendRow(singular), but the tool is registered assheets.appendRows(plural) everywhere in code — the code is internally consistent; just the description is off. - The URL-extraction test uses an unrealistically short ID (
abc123); a real-style ID like1A2b-_C3dwould also guard the-/_character class in the ID regex.
Strengths
- Faithful to the established
getRangepattern, so it reads as a natural sibling. - Correct off-by-default gating; the existing feature-config test still passes.
USER_ENTEREDmatches the stated intent, and thestring | number | boolean | nullcell union is exactly right (allowsnull, excludesundefined).
Review assisted by automated analysis agents; findings verified against the diff.
| { | ||
| type: 'text' as const, | ||
| text: JSON.stringify({ | ||
| updatedRange: response.data.updates?.updatedRange, |
There was a problem hiding this comment.
The four fields are read with response.data.updates?.… optional chaining. If updates is undefined — which the API can return on a 200 where nothing was written — JSON.stringify drops all four undefined fields and the tool returns {}, plus the "Finished appendRows" log line, which is indistinguishable from a real success. For a write tool, a phantom success can mean silently lost data with no signal to the caller. Suggest treating a missing updates as a failure rather than masking it:
const updates = response.data.updates;
if (!updates) {
logToFile(`[SheetsService] appendRows returned no update metadata for: ${id}`);
return {
isError: true,
content: [{ type: 'text' as const, text: JSON.stringify({
error: 'Append returned no update information; rows may not have been written.',
}) }],
};
}
// then read updates.updatedRange, etc. directly (no `?.`)| }, | ||
| ], | ||
| }; | ||
| } catch (error) { |
There was a problem hiding this comment.
This error result isn't flagged with isError: true, so the MCP client treats the call as a success and has to string-parse the JSON body to notice the error key. DriveService (handleError) and DocsService already set isError: true on failures. Note this is consistent with the other Sheets methods (getText/getRange/getMetadata all omit it), so it's a pre-existing pattern rather than something this PR introduces — but since appendRows is a write operation where a false "success" is more costly, it's worth adding here (ideally to all four in one pass):
return {
isError: true,
content: [{ type: 'text' as const, text: JSON.stringify({ error: errorMessage }) }],
};| 'The A1 notation range to append to (e.g., "Sheet1!A1"). Data is appended after the last row with data in this range.', | ||
| ), | ||
| values: z | ||
| .array(z.array(z.union([z.string(), z.number(), z.boolean(), z.null()]))) |
There was a problem hiding this comment.
values: [] currently passes validation and forwards an empty requestBody.values to the API — a wasted no-op call. A .min(1) guard rejects it at the tool boundary with a clear message: z.array(z.array(z.union([...]))).min(1). (Don't enforce rectangular rows — ragged rows are legal in Sheets, where shorter rows just leave trailing cells untouched.)
Summary
sheets.appendRowMCP tool to append rows to a Google Sheets spreadsheetdefaultEnabled: false, requiresspreadsheetsOAuth scope)USER_ENTEREDvalue input option so formulas and dates are parsed correctlyTest Plan
SheetsService.test.ts)npm run test && npm run lintto verify all checks passupdatedRange,updatedRows,updatedColumns,updatedCellsIssue #341