feat: add sheets +write-image shortcut#343
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughAdds a new Changes
Sequence DiagramsequenceDiagram
participant CLI as CLI Client
participant Handler as SheetWriteImage Handler
participant FS as File System
participant API as Sheets v2 API
CLI->>Handler: invoke +write-image (flags: --image, --range/--sheet-id, --url/--spreadsheet-token, --name?, --dry-run?)
Handler->>Handler: resolve spreadsheet token, normalize/validate single-cell range
alt validation error
Handler-->>CLI: return validation error
else continue
Handler->>FS: validate path safety, existence, regular file, size ≤ 20MB
FS-->>Handler: OK / Error
alt FS OK
Handler->>FS: read file bytes
FS-->>Handler: image bytes
alt dry-run
Handler-->>CLI: emit dry-run JSON with placeholder image field
else execute
Handler->>API: POST /open-apis/sheets/v2/spreadsheets/{token}/values_image (JSON: range, image bytes, name)
API-->>Handler: JSON response (spreadsheetToken, updateRange, revision)
Handler-->>CLI: write response JSON to stdout
end
else FS error
Handler-->>CLI: return FS error
end
end
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 docstrings
🧪 Generate unit tests (beta)
Comment |
Greptile SummaryThis PR adds a Confidence Score: 5/5Safe to merge — all previously raised P1 concerns have been addressed and no new blocking issues were found. The three P1 concerns from prior review rounds are resolved: the 20 MB size cap is in place, shortcuts/sheets/sheet_write_image.go — minor: Validate does not pre-check image file existence, but this is an existing known concern and does not block merge.
|
| Filename | Overview |
|---|---|
| shortcuts/sheets/sheet_write_image.go | Core shortcut implementation: Validate/DryRun/Execute handlers with path safety, 20 MB size check, and range normalization; --range is marked Required so the bare-sheet-ID and empty-range divergence concerns from earlier threads are addressed by design. |
| shortcuts/sheets/helpers.go | Adds validateSingleCellRange to reject multi-cell spans; logic is correct and well-tested — A1:A1 (same-cell span) passes while A1:B2 is rejected. |
| shortcuts/sheets/sheet_write_image_test.go | Comprehensive test suite covering validate, dry-run, execute happy paths, API error, oversized file, directory input, absolute path rejection, and custom name handling. |
| shortcuts/sheets/shortcuts.go | Registers SheetWriteImage between SheetWrite and SheetAppend; one-line addition, no issues. |
| skills/lark-sheets/SKILL.md | Adds +write-image row in the shortcut reference table; correctly positioned after +write. |
| skills/lark-sheets/references/lark-sheets-write-image.md | New usage guide (Chinese) with examples, parameter table, output schema, and links to related skills; accurate and consistent with implementation. |
Sequence Diagram
sequenceDiagram
participant User
participant CLI
participant Validate
participant Execute
participant LarkAPI
User->>CLI: "sheets +write-image --token --range --image [--name] [--dry-run]"
CLI->>Validate: "check token, validateSheetRangeInput, validateSingleCellRange"
Validate-->>CLI: "ok / flag error"
alt dry-run
CLI-->>User: "JSON preview with binary placeholder"
else execute
CLI->>Execute: "SafeInputPath check"
Execute->>Execute: "vfs.Stat: exists, regular file, size <= 20MB"
Execute->>Execute: "vfs.ReadFile -> []byte (base64 in JSON)"
Execute->>LarkAPI: "POST /open-apis/sheets/v2/spreadsheets/{token}/values_image"
LarkAPI-->>Execute: "{spreadsheetToken, updateRange, revision}"
Execute-->>User: "JSON output"
end
Reviews (5): Last reviewed commit: "feat: add sheets +write-image shortcut" | Re-trigger Greptile
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@7fdf656ba8b2ff8027685b28877b45406a8182a9🧩 Skill updatenpx skills add larksuite/cli#feat/sheet_image -y -g |
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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/sheets/sheet_write_image_test.go`:
- Around line 154-156: Replace the direct os.WriteFile call in the test with the
project VFS API: call vfs.WriteFile (or the configured test filesystem
instance's WriteFile function) instead of os.WriteFile, e.g. vfs.WriteFile(fs,
"test.png", imgData, 0644) or fs.WriteFile("test.png", imgData, 0644);
add/import the vfs/test filesystem variable if needed and remove the os import
if it becomes unused so the test uses the filesystem abstraction rather than
os.* directly.
- Around line 43-218: Tests leak shared config because they don't set
LARKSUITE_CLI_CONFIG_DIR; fix by setting t.Setenv("LARKSUITE_CLI_CONFIG_DIR",
t.TempDir()) at the start of each test (e.g., in
TestSheetWriteImageValidateRequiresToken, TestSheetWriteImageDryRun,
TestSheetWriteImageExecuteSendsJSON,
TestSheetWriteImageExecuteRejectsNonexistentFile, etc.) or centrally inside
helpers used by these tests (newSheetsTestRuntime and/or the setup path that
calls cmdutil.TestFactory/mountAndRunSheets) so each test gets an isolated
config directory.
In `@shortcuts/sheets/sheet_write_image.go`:
- Around line 73-83: The fallback sets pointRange to a sheet ID only (via
getFirstSheetID and the sheet-id flag) which after normalizePointRange can still
be non-cell, but values_image requires a concrete A1 range; change the logic
around pointRange (the code that assigns pointRange, calls getFirstSheetID, and
then normalizePointRange) so that if pointRange contains only a sheet identifier
(no '!' or no cell reference) you append a default cell (for example "!A1" or
another single-cell A1 range) before calling normalizePointRange, ensuring
values_image receives a concrete cell range.
- Around line 40-42: validateSheetRangeInput currently allows multi-cell ranges
so the sheet_write_image handler must enforce a single-cell constraint: after
the existing call to validateSheetRangeInput(validate runtime.Str("sheet-id"),
runtime.Str("range")) add a check that parses the provided range
(runtime.Str("range")) and ensures it represents exactly one cell (start == end
for row and column) or call/implement a helper like ensureSingleCellRange(range)
and return an error when the range spans multiple cells; update
sheet_write_image.go to perform this single-cell validation before proceeding
with image write logic.
In `@skills/lark-sheets/references/lark-sheets-write-image.md`:
- Line 49: The table row for the `--image <path>` option is missing the trailing
pipe which violates MD055; edit the markdown row that currently reads "|
`--image <path>` | 是 | 本地图片文件的**相对路径**(必须在当前目录下,如 `./logo.png`;不支持绝对路径)" and
append a final "|" so the row ends with "...不支持绝对路径) |"; this restores the
required pipe-style table formatting for the table in
lark-sheets-write-image.md.
🪄 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: c23ad3cc-43ea-4f19-91ce-78cb444fa8ca
📒 Files selected for processing (5)
shortcuts/sheets/sheet_write_image.goshortcuts/sheets/sheet_write_image_test.goshortcuts/sheets/shortcuts.goskills/lark-sheets/SKILL.mdskills/lark-sheets/references/lark-sheets-write-image.md
9371625 to
e0bf215
Compare
|
Tip: Greploops — Automatically fix all review issues by running Use the Greptile plugin for Claude Code to query reviews, search comments, and manage custom context directly from your terminal. |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
shortcuts/sheets/sheet_write_image_test.go (1)
10-10: 🛠️ Refactor suggestion | 🟠 MajorPlease stop using
os.*in the test fixtures.These setup paths write fixtures via
os.WriteFile,os.Mkdir, andos.Create, which bypasses the same filesystem abstraction the shortcut is expected to use. Swapping them tovfs.*keeps the tests aligned with the production path.As per coding guidelines
**/*.go: Usevfs.*instead ofos.*for all filesystem access.Also applies to: 261-263, 334-336, 374-376, 414-416, 460-462, 526-528, 566-568, 589-596, 619-621, 655-657
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/sheets/sheet_write_image_test.go` at line 10, Tests in sheet_write_image_test.go are using os.* (os.WriteFile, os.Mkdir, os.Create, etc.) which bypasses the virtual FS; replace all os filesystem calls with the vfs equivalents (vfs.WriteFile, vfs.MkdirAll/Mkdir, vfs.Create or vfs.OpenFile as appropriate), update the import to remove "os" and add the vfs package, and ensure any file handle handling (Close) and permission/flag arguments are adapted to the vfs APIs; check the helper/test functions referenced in this file (setup fixtures and any functions that create or write files) and apply the same swap for the other occurrences noted in the comment.
🤖 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/sheets/helpers.go`:
- Around line 125-132: The current single-cell validation only rejects
multi-cell spans with cellSpanRangePattern but still allows whole-row/column or
partial spans like "A:B", "1:2", or "A1:B"; update the check around
cellSpanRangePattern.MatchString(subRange) to split subRange into parts, and
validate that both parts match a strict single-cell regex (e.g., cellRefPattern
like ^[A-Za-z]+[0-9]+$) before accepting; if either side does not match the
single-cell pattern, return the existing common.FlagErrorf("--range %q must be a
single cell (e.g. A1 or A1:A1), got a multi-cell span", input). Ensure you
reference and reuse cellSpanRangePattern, subRange, parts (from strings.SplitN),
and common.FlagErrorf so the fix integrates with the surrounding code.
In `@skills/lark-sheets/references/lark-sheets-write-image.md`:
- Around line 47-51: Add a clear sentence to lark-sheets-write-image.md
describing the default target when `--range` is omitted: state exactly where the
image will be written (per the new tests) and how `--sheet-id` influences that
default (e.g., default sheet vs first sheet and default cell), so users know the
behavior when `--range` is left blank; reference the `--range`, `--sheet-id`,
`--image`, and `--name` options when clarifying this default.
---
Duplicate comments:
In `@shortcuts/sheets/sheet_write_image_test.go`:
- Line 10: Tests in sheet_write_image_test.go are using os.* (os.WriteFile,
os.Mkdir, os.Create, etc.) which bypasses the virtual FS; replace all os
filesystem calls with the vfs equivalents (vfs.WriteFile, vfs.MkdirAll/Mkdir,
vfs.Create or vfs.OpenFile as appropriate), update the import to remove "os" and
add the vfs package, and ensure any file handle handling (Close) and
permission/flag arguments are adapted to the vfs APIs; check the helper/test
functions referenced in this file (setup fixtures and any functions that create
or write files) and apply the same swap for the other occurrences noted in the
comment.
🪄 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: f72be6bb-7238-4754-a116-c5be14eefa24
📒 Files selected for processing (6)
shortcuts/sheets/helpers.goshortcuts/sheets/sheet_write_image.goshortcuts/sheets/sheet_write_image_test.goshortcuts/sheets/shortcuts.goskills/lark-sheets/SKILL.mdskills/lark-sheets/references/lark-sheets-write-image.md
✅ Files skipped from review due to trivial changes (2)
- skills/lark-sheets/SKILL.md
- shortcuts/sheets/shortcuts.go
🚧 Files skipped from review as they are similar to previous changes (1)
- shortcuts/sheets/sheet_write_image.go
e0bf215 to
4db4c34
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
shortcuts/sheets/sheet_write_image_test.go (1)
468-471: Consider checking error fromfh.Close().While not critical for test cleanup, explicitly checking or deferring the close with error handling is slightly more robust.
- fh.Close() + if err := fh.Close(); err != nil { + t.Logf("Close() warning: %v", err) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/sheets/sheet_write_image_test.go` around lines 468 - 471, The test currently calls fh.Close() without checking its error; update the teardown to handle Close errors by either deferring fh.Close() and checking the returned error (e.g., `defer func(){ if err := fh.Close(); err != nil { t.Fatalf("Close() error: %v", err) } }()`) or capture and assert the error after Truncate (replace the bare `fh.Close()` with an error check), referencing the file handle `fh` and the existing `Truncate` call to place the check immediately after that operation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@skills/lark-sheets/references/lark-sheets-write-image.md`:
- Around line 57-59: The docs list an output field named `updatedRange` but the
actual API/test response uses `updateRange` (no "d"); update the documentation
to match the API by replacing `updatedRange` with `updateRange` in the output
field descriptions for the Lark Sheets write-image response so examples and
field listings align with the real response.
---
Nitpick comments:
In `@shortcuts/sheets/sheet_write_image_test.go`:
- Around line 468-471: The test currently calls fh.Close() without checking its
error; update the teardown to handle Close errors by either deferring fh.Close()
and checking the returned error (e.g., `defer func(){ if err := fh.Close(); err
!= nil { t.Fatalf("Close() error: %v", err) } }()`) or capture and assert the
error after Truncate (replace the bare `fh.Close()` with an error check),
referencing the file handle `fh` and the existing `Truncate` call to place the
check immediately after that operation.
🪄 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: e12c5a33-d127-4284-bfc1-d2047bb388e1
📒 Files selected for processing (6)
shortcuts/sheets/helpers.goshortcuts/sheets/sheet_write_image.goshortcuts/sheets/sheet_write_image_test.goshortcuts/sheets/shortcuts.goskills/lark-sheets/SKILL.mdskills/lark-sheets/references/lark-sheets-write-image.md
✅ Files skipped from review due to trivial changes (2)
- skills/lark-sheets/SKILL.md
- shortcuts/sheets/shortcuts.go
4db4c34 to
ee7dc27
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
shortcuts/sheets/sheet_write_image_test.go (1)
491-504: Use the temp file’s absolute path instead of/etc/passwd.This keeps the test hermetic and still exercises the same
validate.SafeInputPathrejection. It also avoids baking a Unix-specific path into a CLI test.♻️ Proposed refactor
import ( "bytes" "context" "encoding/json" "os" + "path/filepath" "strings" "testing" @@ - err := mountAndRunSheets(t, SheetWriteImage, []string{ + absPath := filepath.Join(tmpDir, "abs.png") + + err := mountAndRunSheets(t, SheetWriteImage, []string{ "+write-image", "--spreadsheet-token", "shtTOKEN", "--range", "sheet1!A1:A1", - "--image", "/etc/passwd", + "--image", absPath, "--as", "user", }, f, nil)Based on learnings: absolute paths are intentionally unsupported by design, and
validate.SafeInputPathis the intended rejection gate.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/sheets/sheet_write_image_test.go` around lines 491 - 504, The test uses a hard-coded Unix path "/etc/passwd" which makes it non-hermetic; change the mountAndRunSheets invocation for the SheetWriteImage test to pass the absolute path of the temp file created earlier (the "abs.png" in the tmpDir) instead of "/etc/passwd" so the test remains platform-independent yet still exercises validate.SafeInputPath's rejection; locate the mountAndRunSheets call in this test and replace the "--image" argument with the computed absolute path for the temp file (use the temp file's path from the tmpDir/abs.png or filepath.Abs on that filename) so the rest of the test and SheetWriteImage behavior remain unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@skills/lark-sheets/references/lark-sheets-write-image.md`:
- Around line 47-50: Add a note to the `--image <path>` option line stating
there is a 20MB file size cap (rejects image files larger than 20MB) and that
the path must be a relative path in the current directory (no absolute paths);
this mirrors the runtime validation that rejects files above 20MB in the image
upload validation logic and will make the error predictable for users.
---
Nitpick comments:
In `@shortcuts/sheets/sheet_write_image_test.go`:
- Around line 491-504: The test uses a hard-coded Unix path "/etc/passwd" which
makes it non-hermetic; change the mountAndRunSheets invocation for the
SheetWriteImage test to pass the absolute path of the temp file created earlier
(the "abs.png" in the tmpDir) instead of "/etc/passwd" so the test remains
platform-independent yet still exercises validate.SafeInputPath's rejection;
locate the mountAndRunSheets call in this test and replace the "--image"
argument with the computed absolute path for the temp file (use the temp file's
path from the tmpDir/abs.png or filepath.Abs on that filename) so the rest of
the test and SheetWriteImage behavior remain unchanged.
🪄 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: 650844f3-2436-4fb5-b219-167e1a5a2d68
📒 Files selected for processing (6)
shortcuts/sheets/helpers.goshortcuts/sheets/sheet_write_image.goshortcuts/sheets/sheet_write_image_test.goshortcuts/sheets/shortcuts.goskills/lark-sheets/SKILL.mdskills/lark-sheets/references/lark-sheets-write-image.md
✅ Files skipped from review due to trivial changes (1)
- skills/lark-sheets/SKILL.md
🚧 Files skipped from review as they are similar to previous changes (2)
- shortcuts/sheets/shortcuts.go
- shortcuts/sheets/helpers.go
Send image bytes as JSON body (not multipart/form-data) to match the v2 values_image API contract. Use CallAPI for consistent response envelope handling. Includes skill reference doc and unit tests for validate, dry-run, and execute paths.
ee7dc27 to
7fdf656
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
skills/lark-sheets/references/lark-sheets-write-image.md (1)
49-49:⚠️ Potential issue | 🟡 MinorDocument the 20MB cap on
--image.The shortcut rejects files above 20MB at
shortcuts/sheets/sheet_write_image.goLine 90-Line 93, but this row only documents the path restriction. Please add the size limit here so users can predict that validation failure.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@skills/lark-sheets/references/lark-sheets-write-image.md` at line 49, Update the table row for the `--image <path>` flag to also document the 20MB file-size limit (e.g., append "且不大于20MB" or "最大 20MB") so users know the CLI will reject images larger than 20MB; this mirrors the validation implemented in sheet_write_image.go that rejects files above 20MB and prevents surprise failures when using `--image`.
🤖 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/sheets/sheet_write_image.go`:
- Around line 32-46: The validation for the --image path currently lives in
Execute and allows DryRun to accept paths the real command will reject; move the
non-I/O safety check into the Validate function by invoking
validate.SafeInputPath on the runtime string for the image flag (e.g.,
runtime.Str("image") or the flag name used in this file) and return the error if
it fails, while leaving file existence/size/read checks in Execute; ensure you
reference the Validate, Execute and DryRun functions and use
validate.SafeInputPath for the pre-I/O path validation.
---
Duplicate comments:
In `@skills/lark-sheets/references/lark-sheets-write-image.md`:
- Line 49: Update the table row for the `--image <path>` flag to also document
the 20MB file-size limit (e.g., append "且不大于20MB" or "最大 20MB") so users know
the CLI will reject images larger than 20MB; this mirrors the validation
implemented in sheet_write_image.go that rejects files above 20MB and prevents
surprise failures when using `--image`.
🪄 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: 8661c5de-c223-4b59-abdf-bb7c188abcdd
📒 Files selected for processing (6)
shortcuts/sheets/helpers.goshortcuts/sheets/sheet_write_image.goshortcuts/sheets/sheet_write_image_test.goshortcuts/sheets/shortcuts.goskills/lark-sheets/SKILL.mdskills/lark-sheets/references/lark-sheets-write-image.md
✅ Files skipped from review due to trivial changes (1)
- skills/lark-sheets/SKILL.md
🚧 Files skipped from review as they are similar to previous changes (3)
- shortcuts/sheets/helpers.go
- shortcuts/sheets/sheet_write_image_test.go
- shortcuts/sheets/shortcuts.go
Summary
Add a new
sheets +write-imageshortcut that writes a local image file intoa spreadsheet cell via the Sheets v2
values_imageAPI. Supports PNG, JPEG,GIF, BMP, TIFF, HEIC and other common image formats.
Changes
SheetWriteImageshortcut inshortcuts/sheets/sheet_write_image.gowith validate, dry-run, and execute handlers
--url/--spreadsheet-token,--range,--sheet-id,--image(required), and--nameflagsshortcuts/sheets/shortcuts.goskills/lark-sheets/references/lark-sheets-write-image.mdskills/lark-sheets/SKILL.mdshortcut tableTest Plan
make unit-testpassedTestSheetWriteImage*covering validate, dry-run, and execute paths:--urlor--spreadsheet-tokenRelated Issues
N/A
Label: feature
Summary by CodeRabbit
New Features
Documentation
Tests