feat(sheets): add filter view and condition shortcuts#422
Open
caojie0621 wants to merge 1 commit intomainfrom
Open
feat(sheets): add filter view and condition shortcuts#422caojie0621 wants to merge 1 commit intomainfrom
caojie0621 wants to merge 1 commit intomainfrom
Conversation
github-actions
bot
added
domain/ccm
📝 WalkthroughWalkthroughAdds 10 new Sheets CLI shortcuts (5 for filter views, 5 for filter-view conditions), their implementations, path helpers and token validation, a comprehensive test suite, updates shortcut registration, and related documentation pages. Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 |
Add 10 new sheet shortcuts for filter view management: Filter views: - +create-filter-view, +update-filter-view, +list-filter-views - +get-filter-view, +delete-filter-view Filter view conditions: - +create-filter-view-condition, +update-filter-view-condition - +list-filter-view-conditions, +get-filter-view-condition - +delete-filter-view-condition Includes unit tests (39 cases, 88-93% coverage) and skill reference docs.
caojie0621
force-pushed
the
feat/filter_view
branch
from
0f2dce2 to
19324bf
Compare
Greptile SummaryAdds 10 new
Confidence Score: 4/5Not safe to merge as-is — the +update-filter-view-condition PUT endpoint can silently wipe condition data when no update fields are supplied. One P1 defect: +update-filter-view-condition uses PUT (full-replace) without validating that at least one update field is provided, which can wipe existing condition state. One P2: +update-filter-view allows empty-body PATCH inconsistently. Both are straightforward one-line validation fixes, so no broad architectural concern, but the P1 must be addressed before merge. shortcuts/sheets/sheet_filter_view_condition.go (Validate for update), shortcuts/sheets/sheet_filter_view.go (Validate for update) Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[CLI invocation] --> B{--url or\n--spreadsheet-token?}
B -- neither --> C[FlagError: specify --url or --spreadsheet-token]
B -- url --> D[extractSpreadsheetToken]
B -- token --> E[token OK]
D --> E
E --> F{Command}
F --> FV[Filter View ops]
F --> FC[Filter Condition ops]
FV --> FV1["+create-filter-view\nPOST /filter_views"]
FV --> FV2["+update-filter-view\nPATCH /filter_views/:id\n⚠ no at-least-one-field guard"]
FV --> FV3["+list-filter-views\nGET /filter_views/query"]
FV --> FV4["+get-filter-view\nGET /filter_views/:id"]
FV --> FV5["+delete-filter-view\nDELETE /filter_views/:id"]
FC --> FC1["+create-filter-view-condition\nPOST /conditions\nbuildConditionBody includeID=true"]
FC --> FC2["+update-filter-view-condition\nPUT /conditions/:id\n🔴 no at-least-one-field guard\nbuildConditionBody includeID=false"]
FC --> FC3["+list-filter-view-conditions\nGET /conditions/query"]
FC --> FC4["+get-filter-view-condition\nGET /conditions/:id"]
FC --> FC5["+delete-filter-view-condition\nDELETE /conditions/:id"]
Reviews (1): Last reviewed commit: "feat(sheets): add filter view and condit..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+80
to
+83
| Validate: func(ctx context.Context, runtime *common.RuntimeContext) error { | ||
| _, err := validateFilterViewToken(runtime) | ||
| return err | ||
| }, |
There was a problem hiding this comment.
The Validate function for +update-filter-view-condition only checks for the spreadsheet token. All three update fields (--filter-type, --compare-type, --expected) are optional, so a user can run the command without any of them. Because the underlying HTTP method is PUT (full-replace semantics, not PATCH), issuing PUT {} may silently clear the current filter_type, compare_type, and expected values on the condition. At minimum, the user gets a confusing silent no-op; at worst, data is lost.
SheetUpdateDimension.Validate shows the established pattern: explicitly check that at least one optional field was changed before allowing the call through.
Validate: func(ctx context.Context, runtime *common.RuntimeContext) error {
_, err := validateFilterViewToken(runtime)
if err != nil {
return err
}
if !runtime.Cmd.Flags().Changed("filter-type") &&
!runtime.Cmd.Flags().Changed("compare-type") &&
!runtime.Cmd.Flags().Changed("expected") {
return common.FlagErrorf("specify at least one of --filter-type, --compare-type, or --expected")
}
return nil
},
Comment on lines
+99
to
+101
| Validate: func(ctx context.Context, runtime *common.RuntimeContext) error { | ||
| _, err := validateFilterViewToken(runtime) | ||
| return err |
There was a problem hiding this comment.
SheetUpdateFilterView.Validate only checks for a token. If a user runs +update-filter-view without providing --range or --filter-view-name, the execute path issues PATCH {} — a no-op call that gives no feedback that nothing was updated. The comparable SheetUpdateDimension.Validate explicitly returns an error when no update field is set, and the same guard belongs here:
if !runtime.Cmd.Flags().Changed("range") && !runtime.Cmd.Flags().Changed("filter-view-name") {
return common.FlagErrorf("specify at least one of --range or --filter-view-name")
}![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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_filter_view_condition.go`:
- Around line 80-103: The Validate implementation in the
sheet_filter_view_condition.go command only checks auth token and allows no-op
updates; change Validate (the Validate func) to also require at least one of the
update flags before returning success: inspect runtime.Str or
runtime.Has/appropriate accessors for "filter-type", "compare-type", and
"expected" (the same keys used by buildConditionBody) and if all three are
empty/absent return a descriptive error (e.g., "must provide at least one of
--filter-type, --compare-type, or --expected"); keep the existing token
validation by calling validateFilterViewToken(runtime) first and return its
error if present.
In `@shortcuts/sheets/sheet_filter_view.go`:
- Around line 99-125: The current Validate in the +update-filter-view shortcut
only checks token and allows an empty PATCH body; update the Validate function
(the Validate closure that calls validateFilterViewToken) to also require that
at least one of runtime.Str("range") or runtime.Str("filter-view-name") is
non-empty and return a clear error if both are empty so users get an immediate
CLI error; keep the DryRun and Execute body-building logic (which reads
runtime.Str("range") and runtime.Str("filter-view-name")) unchanged so the
validation matches what filterViewItemPath/runtime.CallAPI expect.
In `@skills/lark-sheets/references/lark-sheets-get-filter-view.md`:
- Around line 19-22: Update the documentation for the lark-sheets Get Filter
View flags to indicate that the `--url` and `--spreadsheet-token` flags are
mutually optional but one is required: clarify in the table row for `--url` and
`--spreadsheet-token` (and/or add a short note below the table) that at least
one of `--url` or `--spreadsheet-token` must be provided to avoid the flag
validation error when both are omitted.
In `@skills/lark-sheets/references/lark-sheets-list-filter-view-conditions.md`:
- Around line 21-24: The doc incorrectly marks both `--url` and
`--spreadsheet-token` as optional for the `+list-filter-view-conditions`
command; update the wording and table so it clearly states that one of these two
auth inputs is mandatory (i.e., "one of `--url` or `--spreadsheet-token` must be
provided"), and adjust the description for `--url` and `--spreadsheet-token` in
the table to indicate "one required" or add a note that the command rejects
requests when both are omitted; reference the `+list-filter-view-conditions`
command and the `--url`/`--spreadsheet-token` flags when making this change.
🪄 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: 41e4ea8f-0bea-4b3e-a7fe-fe5dad00c480
📒 Files selected for processing (15)
shortcuts/sheets/sheet_filter_view.goshortcuts/sheets/sheet_filter_view_condition.goshortcuts/sheets/sheet_filter_view_test.goshortcuts/sheets/shortcuts.goskills/lark-sheets/SKILL.mdskills/lark-sheets/references/lark-sheets-create-filter-view-condition.mdskills/lark-sheets/references/lark-sheets-create-filter-view.mdskills/lark-sheets/references/lark-sheets-delete-filter-view-condition.mdskills/lark-sheets/references/lark-sheets-delete-filter-view.mdskills/lark-sheets/references/lark-sheets-get-filter-view-condition.mdskills/lark-sheets/references/lark-sheets-get-filter-view.mdskills/lark-sheets/references/lark-sheets-list-filter-view-conditions.mdskills/lark-sheets/references/lark-sheets-list-filter-views.mdskills/lark-sheets/references/lark-sheets-update-filter-view-condition.mdskills/lark-sheets/references/lark-sheets-update-filter-view.md
Comment on lines
+80
to
+103
| Validate: func(ctx context.Context, runtime *common.RuntimeContext) error { | ||
| _, err := validateFilterViewToken(runtime) | ||
| return err | ||
| }, | ||
| DryRun: func(ctx context.Context, runtime *common.RuntimeContext) *common.DryRunAPI { | ||
| token, _ := validateFilterViewToken(runtime) | ||
| body := buildConditionBody(runtime, false) | ||
| return common.NewDryRunAPI(). | ||
| PUT("/open-apis/sheets/v3/spreadsheets/:token/sheets/:sheet_id/filter_views/:filter_view_id/conditions/:condition_id"). | ||
| Body(body).Set("token", token).Set("sheet_id", runtime.Str("sheet-id")). | ||
| Set("filter_view_id", runtime.Str("filter-view-id")).Set("condition_id", runtime.Str("condition-id")) | ||
| }, | ||
| Execute: func(ctx context.Context, runtime *common.RuntimeContext) error { | ||
| token, _ := validateFilterViewToken(runtime) | ||
| body := buildConditionBody(runtime, false) | ||
| data, err := runtime.CallAPI("PUT", | ||
| filterViewConditionItemPath(token, runtime.Str("sheet-id"), runtime.Str("filter-view-id"), runtime.Str("condition-id")), | ||
| nil, body) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| runtime.Out(data, nil) | ||
| return nil | ||
| }, |
There was a problem hiding this comment.
Reject no-op condition updates before calling the API.
Right now +update-filter-view-condition accepts a request with only IDs and auth, then sends PUT {}. That turns a local usage error into a backend failure. Please require at least one of --filter-type, --compare-type, or --expected in Validate.
Suggested fix
Validate: func(ctx context.Context, runtime *common.RuntimeContext) error {
- _, err := validateFilterViewToken(runtime)
- return err
+ if _, err := validateFilterViewToken(runtime); err != nil {
+ return err
+ }
+ if runtime.Str("filter-type") == "" &&
+ runtime.Str("compare-type") == "" &&
+ runtime.Str("expected") == "" {
+ return common.FlagErrorf("specify at least one of --filter-type, --compare-type, or --expected")
+ }
+ return nil
},📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| Validate: func(ctx context.Context, runtime *common.RuntimeContext) error { | |
| _, err := validateFilterViewToken(runtime) | |
| return err | |
| }, | |
| DryRun: func(ctx context.Context, runtime *common.RuntimeContext) *common.DryRunAPI { | |
| token, _ := validateFilterViewToken(runtime) | |
| body := buildConditionBody(runtime, false) | |
| return common.NewDryRunAPI(). | |
| PUT("/open-apis/sheets/v3/spreadsheets/:token/sheets/:sheet_id/filter_views/:filter_view_id/conditions/:condition_id"). | |
| Body(body).Set("token", token).Set("sheet_id", runtime.Str("sheet-id")). | |
| Set("filter_view_id", runtime.Str("filter-view-id")).Set("condition_id", runtime.Str("condition-id")) | |
| }, | |
| Execute: func(ctx context.Context, runtime *common.RuntimeContext) error { | |
| token, _ := validateFilterViewToken(runtime) | |
| body := buildConditionBody(runtime, false) | |
| data, err := runtime.CallAPI("PUT", | |
| filterViewConditionItemPath(token, runtime.Str("sheet-id"), runtime.Str("filter-view-id"), runtime.Str("condition-id")), | |
| nil, body) | |
| if err != nil { | |
| return err | |
| } | |
| runtime.Out(data, nil) | |
| return nil | |
| }, | |
| Validate: func(ctx context.Context, runtime *common.RuntimeContext) error { | |
| if _, err := validateFilterViewToken(runtime); err != nil { | |
| return err | |
| } | |
| if runtime.Str("filter-type") == "" && | |
| runtime.Str("compare-type") == "" && | |
| runtime.Str("expected") == "" { | |
| return common.FlagErrorf("specify at least one of --filter-type, --compare-type, or --expected") | |
| } | |
| return nil | |
| }, | |
| DryRun: func(ctx context.Context, runtime *common.RuntimeContext) *common.DryRunAPI { | |
| token, _ := validateFilterViewToken(runtime) | |
| body := buildConditionBody(runtime, false) | |
| return common.NewDryRunAPI(). | |
| PUT("/open-apis/sheets/v3/spreadsheets/:token/sheets/:sheet_id/filter_views/:filter_view_id/conditions/:condition_id"). | |
| Body(body).Set("token", token).Set("sheet_id", runtime.Str("sheet-id")). | |
| Set("filter_view_id", runtime.Str("filter-view-id")).Set("condition_id", runtime.Str("condition-id")) | |
| }, | |
| Execute: func(ctx context.Context, runtime *common.RuntimeContext) error { | |
| token, _ := validateFilterViewToken(runtime) | |
| body := buildConditionBody(runtime, false) | |
| data, err := runtime.CallAPI("PUT", | |
| filterViewConditionItemPath(token, runtime.Str("sheet-id"), runtime.Str("filter-view-id"), runtime.Str("condition-id")), | |
| nil, body) | |
| if err != nil { | |
| return err | |
| } | |
| runtime.Out(data, nil) | |
| return nil | |
| }, |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@shortcuts/sheets/sheet_filter_view_condition.go` around lines 80 - 103, The
Validate implementation in the sheet_filter_view_condition.go command only
checks auth token and allows no-op updates; change Validate (the Validate func)
to also require at least one of the update flags before returning success:
inspect runtime.Str or runtime.Has/appropriate accessors for "filter-type",
"compare-type", and "expected" (the same keys used by buildConditionBody) and if
all three are empty/absent return a descriptive error (e.g., "must provide at
least one of --filter-type, --compare-type, or --expected"); keep the existing
token validation by calling validateFilterViewToken(runtime) first and return
its error if present.
Comment on lines
+99
to
+125
| Validate: func(ctx context.Context, runtime *common.RuntimeContext) error { | ||
| _, err := validateFilterViewToken(runtime) | ||
| return err | ||
| }, | ||
| DryRun: func(ctx context.Context, runtime *common.RuntimeContext) *common.DryRunAPI { | ||
| token, _ := validateFilterViewToken(runtime) | ||
| body := map[string]interface{}{} | ||
| if s := runtime.Str("range"); s != "" { | ||
| body["range"] = s | ||
| } | ||
| if s := runtime.Str("filter-view-name"); s != "" { | ||
| body["filter_view_name"] = s | ||
| } | ||
| return common.NewDryRunAPI(). | ||
| PATCH("/open-apis/sheets/v3/spreadsheets/:token/sheets/:sheet_id/filter_views/:filter_view_id"). | ||
| Body(body).Set("token", token).Set("sheet_id", runtime.Str("sheet-id")).Set("filter_view_id", runtime.Str("filter-view-id")) | ||
| }, | ||
| Execute: func(ctx context.Context, runtime *common.RuntimeContext) error { | ||
| token, _ := validateFilterViewToken(runtime) | ||
| body := map[string]interface{}{} | ||
| if s := runtime.Str("range"); s != "" { | ||
| body["range"] = s | ||
| } | ||
| if s := runtime.Str("filter-view-name"); s != "" { | ||
| body["filter_view_name"] = s | ||
| } | ||
| data, err := runtime.CallAPI("PATCH", filterViewItemPath(token, runtime.Str("sheet-id"), runtime.Str("filter-view-id")), nil, body) |
There was a problem hiding this comment.
Require at least one field for +update-filter-view.
This shortcut currently allows PATCH {} when neither --range nor --filter-view-name is passed. That should be rejected in Validate so users get a clear CLI error instead of a backend rejection.
Suggested fix
Validate: func(ctx context.Context, runtime *common.RuntimeContext) error {
- _, err := validateFilterViewToken(runtime)
- return err
+ if _, err := validateFilterViewToken(runtime); err != nil {
+ return err
+ }
+ if runtime.Str("range") == "" && runtime.Str("filter-view-name") == "" {
+ return common.FlagErrorf("specify --range or --filter-view-name")
+ }
+ return nil
},📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| Validate: func(ctx context.Context, runtime *common.RuntimeContext) error { | |
| _, err := validateFilterViewToken(runtime) | |
| return err | |
| }, | |
| DryRun: func(ctx context.Context, runtime *common.RuntimeContext) *common.DryRunAPI { | |
| token, _ := validateFilterViewToken(runtime) | |
| body := map[string]interface{}{} | |
| if s := runtime.Str("range"); s != "" { | |
| body["range"] = s | |
| } | |
| if s := runtime.Str("filter-view-name"); s != "" { | |
| body["filter_view_name"] = s | |
| } | |
| return common.NewDryRunAPI(). | |
| PATCH("/open-apis/sheets/v3/spreadsheets/:token/sheets/:sheet_id/filter_views/:filter_view_id"). | |
| Body(body).Set("token", token).Set("sheet_id", runtime.Str("sheet-id")).Set("filter_view_id", runtime.Str("filter-view-id")) | |
| }, | |
| Execute: func(ctx context.Context, runtime *common.RuntimeContext) error { | |
| token, _ := validateFilterViewToken(runtime) | |
| body := map[string]interface{}{} | |
| if s := runtime.Str("range"); s != "" { | |
| body["range"] = s | |
| } | |
| if s := runtime.Str("filter-view-name"); s != "" { | |
| body["filter_view_name"] = s | |
| } | |
| data, err := runtime.CallAPI("PATCH", filterViewItemPath(token, runtime.Str("sheet-id"), runtime.Str("filter-view-id")), nil, body) | |
| Validate: func(ctx context.Context, runtime *common.RuntimeContext) error { | |
| if _, err := validateFilterViewToken(runtime); err != nil { | |
| return err | |
| } | |
| if runtime.Str("range") == "" && runtime.Str("filter-view-name") == "" { | |
| return common.FlagErrorf("specify --range or --filter-view-name") | |
| } | |
| return nil | |
| }, | |
| DryRun: func(ctx context.Context, runtime *common.RuntimeContext) *common.DryRunAPI { | |
| token, _ := validateFilterViewToken(runtime) | |
| body := map[string]interface{}{} | |
| if s := runtime.Str("range"); s != "" { | |
| body["range"] = s | |
| } | |
| if s := runtime.Str("filter-view-name"); s != "" { | |
| body["filter_view_name"] = s | |
| } | |
| return common.NewDryRunAPI(). | |
| PATCH("/open-apis/sheets/v3/spreadsheets/:token/sheets/:sheet_id/filter_views/:filter_view_id"). | |
| Body(body).Set("token", token).Set("sheet_id", runtime.Str("sheet-id")).Set("filter_view_id", runtime.Str("filter-view-id")) | |
| }, | |
| Execute: func(ctx context.Context, runtime *common.RuntimeContext) error { | |
| token, _ := validateFilterViewToken(runtime) | |
| body := map[string]interface{}{} | |
| if s := runtime.Str("range"); s != "" { | |
| body["range"] = s | |
| } | |
| if s := runtime.Str("filter-view-name"); s != "" { | |
| body["filter_view_name"] = s | |
| } | |
| data, err := runtime.CallAPI("PATCH", filterViewItemPath(token, runtime.Str("sheet-id"), runtime.Str("filter-view-id")), nil, body) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@shortcuts/sheets/sheet_filter_view.go` around lines 99 - 125, The current
Validate in the +update-filter-view shortcut only checks token and allows an
empty PATCH body; update the Validate function (the Validate closure that calls
validateFilterViewToken) to also require that at least one of
runtime.Str("range") or runtime.Str("filter-view-name") is non-empty and return
a clear error if both are empty so users get an immediate CLI error; keep the
DryRun and Execute body-building logic (which reads runtime.Str("range") and
runtime.Str("filter-view-name")) unchanged so the validation matches what
filterViewItemPath/runtime.CallAPI expect.
Comment on lines
+19
to
+22
| | `--url` | 否 | 电子表格 URL | | ||
| | `--spreadsheet-token` | 否 | 表格 token | | ||
| | `--sheet-id` | 是 | 工作表 ID | | ||
| | `--filter-view-id` | 是 | 筛选视图 ID | |
There was a problem hiding this comment.
Document the one-of token requirement.
The table currently says both --url and --spreadsheet-token are optional, but this shortcut fails validation when neither is provided. Please note that at least one of them is required, otherwise users will hit a flag error immediately.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@skills/lark-sheets/references/lark-sheets-get-filter-view.md` around lines 19
- 22, Update the documentation for the lark-sheets Get Filter View flags to
indicate that the `--url` and `--spreadsheet-token` flags are mutually optional
but one is required: clarify in the table row for `--url` and
`--spreadsheet-token` (and/or add a short note below the table) that at least
one of `--url` or `--spreadsheet-token` must be provided to avoid the flag
validation error when both are omitted.
Comment on lines
+21
to
+24
| | `--url` | 否 | 电子表格 URL | | ||
| | `--spreadsheet-token` | 否 | 表格 token | | ||
| | `--sheet-id` | 是 | 工作表 ID | | ||
| | `--filter-view-id` | 是 | 筛选视图 ID | |
There was a problem hiding this comment.
Clarify that one auth input is mandatory.
+list-filter-view-conditions also rejects requests when both --url and --spreadsheet-token are omitted, so marking both as optional is misleading here. Call out that users must pass one of the two.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@skills/lark-sheets/references/lark-sheets-list-filter-view-conditions.md`
around lines 21 - 24, The doc incorrectly marks both `--url` and
`--spreadsheet-token` as optional for the `+list-filter-view-conditions`
command; update the wording and table so it clearly states that one of these two
auth inputs is mandatory (i.e., "one of `--url` or `--spreadsheet-token` must be
provided"), and adjust the description for `--url` and `--spreadsheet-token` in
the table to indicate "one required" or add a note that the command rejects
requests when both are omitted; reference the `+list-filter-view-conditions`
command and the `--url`/`--spreadsheet-token` flags when making this change.
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@19324bfae048905e11458d7b6542f087d5b95692🧩 Skill updatenpx skills add larksuite/cli#feat/filter_view -y -g |
There was a problem hiding this comment.
♻️ Duplicate comments (2)
shortcuts/sheets/sheet_filter_view_condition.go (1)
80-83:⚠️ Potential issue | 🟡 MinorReject empty
+update-filter-view-conditionupdates inValidate.This path currently allows
PUT {}when none of--filter-type,--compare-type, or--expectedis provided. Please validate this upfront.Suggested fix
Validate: func(ctx context.Context, runtime *common.RuntimeContext) error { - _, err := validateFilterViewToken(runtime) - return err + if _, err := validateFilterViewToken(runtime); err != nil { + return err + } + if runtime.Str("filter-type") == "" && + runtime.Str("compare-type") == "" && + runtime.Str("expected") == "" { + return common.FlagErrorf("specify at least one of --filter-type, --compare-type, or --expected") + } + return nil },Also applies to: 84-97
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/sheets/sheet_filter_view_condition.go` around lines 80 - 83, The Validate function in sheet_filter_view_condition.go currently returns success for empty update payloads; update it to explicitly reject requests that don't provide any of the updatable fields by checking the runtime for at least one of the flags/values (--filter-type, --compare-type, --expected) before calling validateFilterViewToken, and return an error if none are present; apply the same check for the other Validate blocks around the 84-97 region so both update paths enforce "no-empty-update" validation using the same flag presence logic and error message.shortcuts/sheets/sheet_filter_view.go (1)
99-102:⚠️ Potential issue | 🟡 MinorReject no-op
+update-filter-viewrequests in validation.
+update-filter-viewcurrently allowsPATCH {}when neither--rangenor--filter-view-nameis set. Please fail fast inValidatewith a flag error.Suggested fix
Validate: func(ctx context.Context, runtime *common.RuntimeContext) error { - _, err := validateFilterViewToken(runtime) - return err + if _, err := validateFilterViewToken(runtime); err != nil { + return err + } + if runtime.Str("range") == "" && runtime.Str("filter-view-name") == "" { + return common.FlagErrorf("specify --range or --filter-view-name") + } + return nil },Also applies to: 103-115, 116-126
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/sheets/sheet_filter_view.go` around lines 99 - 102, The Validate implementation for the +update-filter-view command currently only calls validateFilterViewToken(runtime) and permits no-op PATCH requests; update Validate (the anonymous Validate func and the related validators around validateFilterViewToken) to check that at least one of the change flags is present (e.g., runtime.Flags or the parsed flags for --range and --filter-view-name) and return a flag error when both are empty, so the command fails fast instead of allowing PATCH {}. Locate the anonymous Validate function(s) that call validateFilterViewToken and add a pre-check that inspects the runtime/flags for --range or --filter-view-name, returning a descriptive flag error if neither is provided. Ensure the same change is applied to the other similar Validate blocks referenced (lines handling the same command).
🧹 Nitpick comments (1)
shortcuts/sheets/sheet_filter_view.go (1)
53-75: Extract duplicated filter-view body construction to one helper.Create/update each build the same body twice (DryRun + Execute). A shared helper will reduce drift risk.
Also applies to: 103-125
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/sheets/sheet_filter_view.go` around lines 53 - 75, Both DryRun and Execute duplicate the same body construction (using runtime.Str("range"), "filter-view-name", "filter-view-id"); create a small helper function (e.g., buildFilterViewBody(runtime *common.RuntimeContext) map[string]interface{}) and replace the inline body creation in DryRun and Execute with a call to that helper, keeping validateFilterViewToken and filterViewBasePath usage unchanged; also update the other similar block (lines ~103-125) to use the same helper so both cases share one source of truth.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@shortcuts/sheets/sheet_filter_view_condition.go`:
- Around line 80-83: The Validate function in sheet_filter_view_condition.go
currently returns success for empty update payloads; update it to explicitly
reject requests that don't provide any of the updatable fields by checking the
runtime for at least one of the flags/values (--filter-type, --compare-type,
--expected) before calling validateFilterViewToken, and return an error if none
are present; apply the same check for the other Validate blocks around the 84-97
region so both update paths enforce "no-empty-update" validation using the same
flag presence logic and error message.
In `@shortcuts/sheets/sheet_filter_view.go`:
- Around line 99-102: The Validate implementation for the +update-filter-view
command currently only calls validateFilterViewToken(runtime) and permits no-op
PATCH requests; update Validate (the anonymous Validate func and the related
validators around validateFilterViewToken) to check that at least one of the
change flags is present (e.g., runtime.Flags or the parsed flags for --range and
--filter-view-name) and return a flag error when both are empty, so the command
fails fast instead of allowing PATCH {}. Locate the anonymous Validate
function(s) that call validateFilterViewToken and add a pre-check that inspects
the runtime/flags for --range or --filter-view-name, returning a descriptive
flag error if neither is provided. Ensure the same change is applied to the
other similar Validate blocks referenced (lines handling the same command).
---
Nitpick comments:
In `@shortcuts/sheets/sheet_filter_view.go`:
- Around line 53-75: Both DryRun and Execute duplicate the same body
construction (using runtime.Str("range"), "filter-view-name", "filter-view-id");
create a small helper function (e.g., buildFilterViewBody(runtime
*common.RuntimeContext) map[string]interface{}) and replace the inline body
creation in DryRun and Execute with a call to that helper, keeping
validateFilterViewToken and filterViewBasePath usage unchanged; also update the
other similar block (lines ~103-125) to use the same helper so both cases share
one source of truth.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1e9c1b69-74f4-444c-b260-8477bf035145
📒 Files selected for processing (15)
shortcuts/sheets/sheet_filter_view.goshortcuts/sheets/sheet_filter_view_condition.goshortcuts/sheets/sheet_filter_view_test.goshortcuts/sheets/shortcuts.goskills/lark-sheets/SKILL.mdskills/lark-sheets/references/lark-sheets-create-filter-view-condition.mdskills/lark-sheets/references/lark-sheets-create-filter-view.mdskills/lark-sheets/references/lark-sheets-delete-filter-view-condition.mdskills/lark-sheets/references/lark-sheets-delete-filter-view.mdskills/lark-sheets/references/lark-sheets-get-filter-view-condition.mdskills/lark-sheets/references/lark-sheets-get-filter-view.mdskills/lark-sheets/references/lark-sheets-list-filter-view-conditions.mdskills/lark-sheets/references/lark-sheets-list-filter-views.mdskills/lark-sheets/references/lark-sheets-update-filter-view-condition.mdskills/lark-sheets/references/lark-sheets-update-filter-view.md
✅ Files skipped from review due to trivial changes (10)
- skills/lark-sheets/references/lark-sheets-list-filter-view-conditions.md
- skills/lark-sheets/references/lark-sheets-get-filter-view.md
- skills/lark-sheets/references/lark-sheets-get-filter-view-condition.md
- skills/lark-sheets/references/lark-sheets-list-filter-views.md
- skills/lark-sheets/references/lark-sheets-delete-filter-view-condition.md
- skills/lark-sheets/references/lark-sheets-update-filter-view.md
- skills/lark-sheets/references/lark-sheets-create-filter-view-condition.md
- skills/lark-sheets/references/lark-sheets-create-filter-view.md
- skills/lark-sheets/references/lark-sheets-delete-filter-view.md
- skills/lark-sheets/references/lark-sheets-update-filter-view-condition.md
🚧 Files skipped from review as they are similar to previous changes (3)
- shortcuts/sheets/shortcuts.go
- skills/lark-sheets/SKILL.md
- shortcuts/sheets/sheet_filter_view_test.go
| {Name: "range", Desc: "new filter range"}, | ||
| {Name: "filter-view-name", Desc: "new display name (max 100 chars)"}, | ||
| }, | ||
| Validate: func(ctx context.Context, runtime *common.RuntimeContext) error { |
Collaborator
There was a problem hiding this comment.
Should we add a CLI-side validation here to require at least one update field?
Right now +update-filter-view can still send an empty body if the user only passes --sheet-id and --filter-view-id. In similar update-style shortcuts in this repo, we usually fail early in the CLI instead of sending a no-op request to the API.
It may be better to enforce that at least one of --range or --filter-view-name is provided in Validate.
| {Name: "compare-type", Desc: "comparison operator"}, | ||
| {Name: "expected", Desc: "filter values JSON array"}, | ||
| }, | ||
| Validate: func(ctx context.Context, runtime *common.RuntimeContext) error { |
Collaborator
There was a problem hiding this comment.
It may also be worth adding a CLI-side validation here to require at least one update field.
At the moment +update-filter-view-condition can generate an empty body when none of --filter-type, --compare-type, or --expected is provided, so the user only gets feedback from the server side.
Suggest checking in Validate that at least one updatable field is explicitly set.
| if s := runtime.Str("compare-type"); s != "" { | ||
| body["compare_type"] = s | ||
| } | ||
| if s := runtime.Str("expected"); s != "" { |
Collaborator
There was a problem hiding this comment.
Is the --expected parsing here a bit too permissive?
From the flag description and the skill docs, expected looks like it is intended to be a JSON array, but this implementation currently:
- accepts any JSON value (for example object, number, or string)
- silently falls back to
[]string{s}when the input is not valid JSON
That makes the CLI behavior differ from the documented contract, and it may also hide user input errors. Would it be better to validate --expected explicitly in Validate as a proper JSON array, instead of auto-coercing invalid input into a single-element array?
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
2 participants
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 10 new
sheetsshortcuts for filter view and filter condition CRUD, covering the full lifecycle of spreadsheet filter views.Changes
Filter views (5)
+create-filter-view: create a filter view with range, optional name and custom ID+update-filter-view: update filter view range or name+list-filter-views: list all filter views in a sheet+get-filter-view: get a filter view by ID+delete-filter-view: delete a filter viewFilter view conditions (5)
+create-filter-view-condition: create a filter condition on a column (number/text/color/hiddenValue)+update-filter-view-condition: update a filter condition+list-filter-view-conditions: list all conditions of a filter view+get-filter-view-condition: get a condition by column+delete-filter-view-condition: delete a conditionOther
Test Plan
go test ./shortcuts/sheets/)lark-cli sheets +xxxcommands work as expectedRelated Issues
Summary by CodeRabbit
New Features
Documentation
Tests