feat(sheets): add cell operation shortcuts for merge, replace, and style#412
Open
caojie0621 wants to merge 1 commit intomainfrom
Open
feat(sheets): add cell operation shortcuts for merge, replace, and style#412caojie0621 wants to merge 1 commit intomainfrom
caojie0621 wants to merge 1 commit intomainfrom
Conversation
📝 WalkthroughWalkthroughAdds five new Sheets shortcuts— Changes
Sequence Diagram(s)sequenceDiagram
participant User as CLI User
participant CLI as CLI Handler
participant Validator as Validator
participant Executor as Executor
participant API as Sheets API
participant Output as Output Handler
User->>CLI: Invoke sheets +<operation>\n(--url/--spreadsheet-token, flags)
CLI->>Validator: Validate flags
Validator->>Validator: Derive spreadsheet token\n(from --spreadsheet-token or --url)
Validator->>Validator: Validate operation-specific inputs
Validator-->>CLI: Return validation result
alt DryRun
CLI->>Executor: Build DryRun request
Executor->>Executor: Normalize ranges / parse JSON
Executor-->>CLI: Return request preview
CLI-->>User: Display request preview
else Execute
CLI->>Executor: Execute operation
Executor->>Executor: Prepare payload / path-encode token
Executor->>API: POST/PUT /open-apis/sheets/...
API-->>Executor: Return response
Executor->>Output: Write result to runtime.Out
Output-->>User: Print JSON output
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
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 |
github-actions
bot
added
domain/ccm
Greptile SummaryThis PR adds 5 new Confidence Score: 5/5Safe to merge — all previously raised concerns are addressed and the single new observation is low-risk All five shortcuts follow established patterns, errors are properly propagated in Execute, tests cover 30 cases including validation/dry-run/success/error paths, and documentation is updated. The one new comment about the bare sheet-ID default for find_condition.range is P2 — it may be intentional and the author manually verified the commands work end-to-end. shortcuts/sheets/sheet_replace.go — the find_condition.range default behavior when --range is omitted Important Files Changed
Sequence DiagramsequenceDiagram
participant CLI as lark-cli sheets
participant Validate
participant DryRun
participant Execute
participant LarkAPI as Lark Sheets API
CLI->>Validate: token, range, flags
Validate-->>CLI: error | nil
alt --dry-run flag
CLI->>DryRun: token, range, flags
DryRun-->>CLI: DryRunAPI (method, url, body preview)
else execute
CLI->>Execute: token, range, flags
Execute->>LarkAPI: POST/PUT /open-apis/sheets/v2|v3/...
Note over Execute,LarkAPI: merge_cells / unmerge_cells (v2) style / styles_batch_update (v2) sheets/{id}/replace (v3)
LarkAPI-->>Execute: JSON response
Execute-->>CLI: runtime.Out(result)
end
Reviews (2): Last reviewed commit: "feat(sheets): add cell operation shortcu..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@e9456db8c9d2992ecd2c1be02ebf7e349a0b2ee1🧩 Skill updatenpx skills add larksuite/cli#feat/sheet_cell -y -g |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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/sheets/sheet_replace.go`:
- Around line 41-43: validateSheetRangeInput currently lets absolute ranges like
"sheet2!A1:B2" bypass validation, so calling SheetReplace with --sheet-id sheet1
and --range sheet2!A1:B2 produces a contradictory request; update
validateSheetRangeInput to parse the range for an explicit sheet name and, if
present, ensure it matches the provided sheet-id (rejecting with an error on
mismatch), or change the function signature used by the three call sites (the
validateSheetRangeInput calls in SheetReplace flow) to accept both sheet-id and
range and perform this check; add a regression test that calls the SheetReplace
entrypoint with conflicting --sheet-id and absolute --range and asserts it
returns an error.
In `@shortcuts/sheets/sheet_set_style.go`:
- Around line 37-40: The current unmarshalling into a plain interface{} allows
arrays, strings, and null for runtime.Str("style"); change the validation so
that after json.Unmarshal you assert the result is an object
(map[string]interface{} or map[string]any) and return a FlagErrorf if it's not;
specifically update the block around variable style, the json.Unmarshal call,
and the place that assigns to appendStyle.style to only accept an object-shaped
map and reject other JSON types with an error like "--style must be a JSON
object".
🪄 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: 74ad2f72-1fe0-4362-84ca-7fe86f9a0343
📒 Files selected for processing (13)
shortcuts/sheets/sheet_batch_set_style.goshortcuts/sheets/sheet_cell_ops_test.goshortcuts/sheets/sheet_merge_cells.goshortcuts/sheets/sheet_replace.goshortcuts/sheets/sheet_set_style.goshortcuts/sheets/sheet_unmerge_cells.goshortcuts/sheets/shortcuts.goskills/lark-sheets/SKILL.mdskills/lark-sheets/references/lark-sheets-batch-set-style.mdskills/lark-sheets/references/lark-sheets-merge-cells.mdskills/lark-sheets/references/lark-sheets-replace.mdskills/lark-sheets/references/lark-sheets-set-style.mdskills/lark-sheets/references/lark-sheets-unmerge-cells.md
Add 5 new sheet shortcuts for cell operations: - +merge-cells: merge cells with MERGE_ALL/MERGE_ROWS/MERGE_COLUMNS - +unmerge-cells: split merged cells - +replace: find and replace cell values - +set-style: set cell style (font, color, alignment, border) - +batch-set-style: batch set styles for multiple ranges Includes unit tests (81-89% coverage) and skill reference docs.
caojie0621
force-pushed
the
feat/sheet_cell
branch
from
8961893 to
e9456db
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
shortcuts/sheets/sheet_unmerge_cells.go (1)
28-31: Consider centralizing spreadsheet token resolution to avoid drift.The same
--url/--spreadsheet-tokenresolution logic is duplicated inValidate,DryRun, andExecute. A small helper (e.g.,resolveSpreadsheetToken(runtime)) would reduce maintenance risk.Refactor sketch
+func resolveSpreadsheetToken(runtime *common.RuntimeContext) string { + token := runtime.Str("spreadsheet-token") + if runtime.Str("url") != "" { + token = extractSpreadsheetToken(runtime.Str("url")) + } + return token +} ... - token := runtime.Str("spreadsheet-token") - if runtime.Str("url") != "" { - token = extractSpreadsheetToken(runtime.Str("url")) - } + token := resolveSpreadsheetToken(runtime)Also applies to: 41-44, 54-57
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/sheets/sheet_unmerge_cells.go` around lines 28 - 31, The spreadsheet token resolution logic (using runtime.Str("spreadsheet-token") and overriding with extractSpreadsheetToken(runtime.Str("url"))) is duplicated across Validate, DryRun, and Execute; create a small helper function (e.g., resolveSpreadsheetToken(runtime) or resolveSpreadsheetTokenFromRuntime) that accepts the runtime object, encapsulates the token selection logic and extractSpreadsheetToken call, and replace the duplicated blocks in Validate, DryRun, and Execute with calls to this helper to centralize behavior and avoid drift.shortcuts/sheets/sheet_set_style.go (1)
75-77: KeepExecutestyle-type checks consistent withValidate.
Validateenforces object-only JSON, butExecutecurrently re-parses without re-checking the object type. Consider using one shared parser/validator to keep behavior consistent and prevent future drift.Based on learnings: “In the `larksuite/cli` shortcut runner, `Validate` is always executed before `DryRun` … guards are intentionally kept in `Validate` and not duplicated in `DryRun`.”Refactor sketch
+func parseStyleObject(raw string) (map[string]interface{}, error) { + var style map[string]interface{} + if err := json.Unmarshal([]byte(raw), &style); err != nil { + return nil, common.FlagErrorf("--style must be valid JSON: %v", err) + } + if style == nil { + return nil, common.FlagErrorf("--style must be a JSON object") + } + return style, nil +} ... - var style interface{} - if err := json.Unmarshal([]byte(runtime.Str("style")), &style); err != nil { - return common.FlagErrorf("--style must be valid JSON: %v", err) - } + style, err := parseStyleObject(runtime.Str("style")) + if err != nil { + return err + }Also applies to: 83-86
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/sheets/sheet_set_style.go` around lines 75 - 77, Execute currently unmarshals runtime.Str("style") with json.Unmarshal into style without re-checking that the JSON is an object like Validate enforces; update Execute (in sheet_set_style.go, the Execute method that calls json.Unmarshal([]byte(runtime.Str("style")), &style)) to reuse the same parsing/validation logic used by Validate (or call a shared helper) so the parsed value is verified to be a JSON object (map[string]interface{}) before proceeding and return a FlagError if it is not; ensure the helper accepts the raw string (runtime.Str("style")), returns the parsed object, and is used by both Validate and Execute to keep behavior consistent.
🤖 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/sheets/sheet_set_style.go`:
- Around line 75-77: Execute currently unmarshals runtime.Str("style") with
json.Unmarshal into style without re-checking that the JSON is an object like
Validate enforces; update Execute (in sheet_set_style.go, the Execute method
that calls json.Unmarshal([]byte(runtime.Str("style")), &style)) to reuse the
same parsing/validation logic used by Validate (or call a shared helper) so the
parsed value is verified to be a JSON object (map[string]interface{}) before
proceeding and return a FlagError if it is not; ensure the helper accepts the
raw string (runtime.Str("style")), returns the parsed object, and is used by
both Validate and Execute to keep behavior consistent.
In `@shortcuts/sheets/sheet_unmerge_cells.go`:
- Around line 28-31: The spreadsheet token resolution logic (using
runtime.Str("spreadsheet-token") and overriding with
extractSpreadsheetToken(runtime.Str("url"))) is duplicated across Validate,
DryRun, and Execute; create a small helper function (e.g.,
resolveSpreadsheetToken(runtime) or resolveSpreadsheetTokenFromRuntime) that
accepts the runtime object, encapsulates the token selection logic and
extractSpreadsheetToken call, and replace the duplicated blocks in Validate,
DryRun, and Execute with calls to this helper to centralize behavior and avoid
drift.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: bc1b952d-7b47-41b3-baec-313caf16957e
📒 Files selected for processing (13)
shortcuts/sheets/sheet_batch_set_style.goshortcuts/sheets/sheet_cell_ops_test.goshortcuts/sheets/sheet_merge_cells.goshortcuts/sheets/sheet_replace.goshortcuts/sheets/sheet_set_style.goshortcuts/sheets/sheet_unmerge_cells.goshortcuts/sheets/shortcuts.goskills/lark-sheets/SKILL.mdskills/lark-sheets/references/lark-sheets-batch-set-style.mdskills/lark-sheets/references/lark-sheets-merge-cells.mdskills/lark-sheets/references/lark-sheets-replace.mdskills/lark-sheets/references/lark-sheets-set-style.mdskills/lark-sheets/references/lark-sheets-unmerge-cells.md
✅ Files skipped from review due to trivial changes (7)
- shortcuts/sheets/shortcuts.go
- skills/lark-sheets/SKILL.md
- skills/lark-sheets/references/lark-sheets-replace.md
- skills/lark-sheets/references/lark-sheets-merge-cells.md
- skills/lark-sheets/references/lark-sheets-batch-set-style.md
- skills/lark-sheets/references/lark-sheets-unmerge-cells.md
- skills/lark-sheets/references/lark-sheets-set-style.md
🚧 Files skipped from review as they are similar to previous changes (4)
- shortcuts/sheets/sheet_batch_set_style.go
- shortcuts/sheets/sheet_merge_cells.go
- shortcuts/sheets/sheet_cell_ops_test.go
- shortcuts/sheets/sheet_replace.go
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Add 5 new
sheetsshortcuts for cell operations: merge/unmerge cells, find-and-replace, set style, and batch set style.Changes
+merge-cells: merge cells with MERGE_ALL/MERGE_ROWS/MERGE_COLUMNS+unmerge-cells: split merged cells+replace: find and replace cell values with regex/case-sensitive options+set-style: set cell style (font, color, alignment, border)+batch-set-style: batch set styles for multiple rangesTest Plan
go test ./shortcuts/sheets/)lark-cli sheets +xxxcommands work as expectedRelated Issues
Summary by CodeRabbit
New Features
Documentation
Tests