Skip to content

fix(sheets): allow +pivot-create to omit both sheet selectors#1130

Merged
zhengzhijiej-tech merged 1 commit into
larksuite:feat/lark-sheets-refactorfrom
zhengzhijiej-tech:fix/pivot-create-schema-alignment
May 27, 2026
Merged

fix(sheets): allow +pivot-create to omit both sheet selectors#1130
zhengzhijiej-tech merged 1 commit into
larksuite:feat/lark-sheets-refactorfrom
zhengzhijiej-tech:fix/pivot-create-schema-alignment

Conversation

@zhengzhijiej-tech
Copy link
Copy Markdown
Collaborator

Summary

manage_pivot_table_object treats sheet_id / sheet_name as the placement target — when both are absent, handleCreate() auto-creates a new sub-sheet to host the pivot table. The CLI's flag schema didn't reflect that:

  • A third flag --target-sheet-id duplicated the same wire field as --sheet-id, leaving callers unsure which one to use.
  • --sheet-id / --sheet-name had "XOR with the other" descriptions that read like "operation context", so callers (especially LLM tool callers) felt obligated to set one. Frequently that ended up being the source sheet, which silently disabled the backend's auto-create guardrail and dropped the pivot at A1 — overlapping the source data with #REF!.

This PR aligns the CLI to the backend's actual contract.

Changes

Wire schema (synced from sheet-skill-spec companion change):

  • Drop the duplicate --target-sheet-id flag from +pivot-create.
  • Rewrite --sheet-id / --sheet-name descriptions to make placement-target semantics explicit, and call out that omitting both is the recommended path.

Implementation:

  • helpers.go: new optionalSheetSelector ("at most one" — both empty allowed; both set still rejected). requireSheetSelector is untouched, so every existing caller keeps the exactly-one contract.
  • lark_sheet_object_crud.go: objectCRUDSpec gains allowEmptySheetSelectorOnCreate; objectCreateInput dispatches to optionalSheetSelector when set. Only pivotSpec opts in; chart / cond-format / sparkline / filter-view / float-image continue to require a sheet selector.
  • DryRun/Execute switch to direct flag extraction (same pattern Validate already used) so the XOR check happens in exactly one place — the input builder.
  • pivotSpec.enhanceCreateInput: remove the branch that read the now-removed --target-sheet-id flag.

Test plan

  • TestPivotCreate_SheetSelectorSemantics covers both-empty (accepted), both-set (rejected with "mutually exclusive"), and single-set (accepted) paths.
  • TestObjectCreate_RequiresSheetSelector regresses chart / cond-format / sparkline / filter-view to confirm the relaxation is scoped to pivot only.
  • Existing TestObjectCRUDShortcuts_DryRun updated: removed --target-sheet-id from the pivot case, added a both-empty case asserting the input has no sheet_id / sheet_name.
  • go build ./... && go test ./shortcuts/sheets/... pass.

manage_pivot_table_object treats sheet_id / sheet_name as the placement
target — when both are absent, handleCreate() auto-creates a new sub-sheet
to host the pivot table. The CLI's flag schema didn't reflect this:

- Exposed a third flag --target-sheet-id that mapped to the same wire
  field as --sheet-id, leaving the caller unsure which one to use
- --sheet-id / --sheet-name had "XOR with the other" descriptions that
  read like "operation context", so callers (especially LLM tool callers)
  felt obligated to set one — frequently the source sheet — which
  silently disabled the backend's auto-create guardrail and dropped the
  pivot at A1, overlapping the source data

Wire change (synced from sheet-skill-spec): drop the duplicate
--target-sheet-id flag; rewrite --sheet-id / --sheet-name descriptions
to make the placement-target semantics explicit and call out that
omitting both is the recommended path.

Implementation change (this PR): add an at-most-one sheet-selector
helper and let object create-shortcuts opt into it.

- helpers.go: new optionalSheetSelector (both empty allowed; both set
  still rejected; control-char validation unchanged). requireSheetSelector
  is untouched — every existing caller keeps the exactly-one contract.
- lark_sheet_object_crud.go: objectCRUDSpec gains
  allowEmptySheetSelectorOnCreate; objectCreateInput dispatches to
  optionalSheetSelector when it's set. Only pivotSpec opts in;
  chart / cond-format / sparkline / filter-view / float-image keep
  the existing require semantics. DryRun and Execute switch to direct
  flag extraction (same pattern Validate already used) so the XOR
  check happens in exactly one place (the builder).
- pivotSpec: drop the enhanceCreateInput branch that read the now-removed
  --target-sheet-id flag.
- Tests: TestPivotCreate_SheetSelectorSemantics covers both-empty /
  both-set / single-set; TestObjectCreate_RequiresSheetSelector
  regresses chart / cond-format / sparkline / filter-view to lock the
  scope of the relaxation.
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 27, 2026

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: fd43c8e3-b476-480b-896b-fab6ae014269

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@github-actions github-actions Bot added domain/ccm PR touches the ccm domain size/L Large or sensitive change across domains or core paths labels May 27, 2026
@zhengzhijiej-tech zhengzhijiej-tech merged commit e85afd6 into larksuite:feat/lark-sheets-refactor May 27, 2026
3 checks passed
zhengzhijiej-tech added a commit to zhengzhijiej-tech/cli that referenced this pull request May 27, 2026
Companion sync from sheet-skill-spec — the canonical reference rewrites
+pivot-create's "5 placement-related flags" rundown into a clearer
"4 placement-related flags" form (--target-sheet-id was already removed
in larksuite#1130, this updates the prose accordingly), and clarifies that
--sheet-id / --sheet-name on +pivot-create are the *placement* sheet
(not the source-data sheet), with omit-both as the strongly-recommended
default.

Also picks up a base-side --target-position description tweak that
dropped the now-stale "与 --target-sheet-id 配套" reference.

No CLI surface change.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

domain/ccm PR touches the ccm domain size/L Large or sensitive change across domains or core paths

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants