Skip to content

fix(base): improve table shortcut behavior & guidance#1803

Merged
zhouyue-bytedance merged 3 commits into
larksuite:mainfrom
huarenmin13:table-review-fixes-pr
Jul 21, 2026
Merged

fix(base): improve table shortcut behavior & guidance#1803
zhouyue-bytedance merged 3 commits into
larksuite:mainfrom
huarenmin13:table-review-fixes-pr

Conversation

@huarenmin13

@huarenmin13 huarenmin13 commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

Summary

Improve Base shortcut behavior and guidance for field writes and record projection. This revision rebuilds the PR's effective diff on the latest main and includes the follow-ups from the previous review pass.

Changes

  • Return structured field_get_recommended, next_step, and verification_hint guidance after successful field writes. +field-update recommends +field-get because the update response cannot reveal the field's previous type, gives a dedicated hint for returned/submitted type mismatches, and documents how to verify state after a no-op API error.
  • Keep auto_number updates on the v3 field-update path and document the API behavior for applying updated numbering rules to existing values.
  • Use one record-projection contract across list, search, and get: canonical --field-id, compatible hidden --fields / --field-names aliases, shared normalization, duplicate/limit/mutual-exclusion checks, and accurate parameter attribution in structured errors. The legacy --field-names CSV/repeated semantics, quoted commas, and literal @-prefixed names remain supported.
  • Strengthen typed validation and API error-path tests for category, subtype, parameter, and API-code metadata.
  • Consolidate create, batch-create, and update readback metadata decoration while preserving each operation's recommendation rules.
  • Clarify link-field input for parent/child records and extend dry-run E2E coverage for field-update and projection behavior.

Review feedback

  • Removed the redundant “普通 v3 更新,无需额外参数” wording from the auto_number style.rules guidance.
  • Preserved the legacy --field-names CSV contract, including quoted commas and literal @-prefixed field names.
  • Included every active projection flag in structured record-search --json conflict errors.
  • Clarified the Rating max guidance and normalized explicit select_fields: null to omission.

Test Plan

  • make unit-test (full repository unit matrix with race)
  • make vet
  • make fmt-check
  • go mod tidy leaves go.mod / go.sum unchanged
  • go run github.com/golangci/golangci-lint/v2/cmd/golangci-lint@v2.1.6 run --new-from-rev=upstream/main (0 issues)
  • go test -count=1 ./shortcuts/base
  • go test -race -count=1 ./shortcuts/base
  • go build -o /private/tmp/lark-cli-pr1803-sync-v2 .
  • gopls check on all changed production Go files
  • Targeted Base dry-run E2E tests for auto_number, Rating, list/search/get aliases, quoted CSV, literal @ names, JSON conflicts, and flag-like field names
  • git diff --check

Related Issues

  • None

@CLAassistant

CLAassistant commented Jul 8, 2026

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

@github-actions github-actions Bot added domain/base PR touches the base domain size/L Large or sensitive change across domains or core paths labels Jul 8, 2026
@coderabbitai

coderabbitai Bot commented Jul 8, 2026

Copy link
Copy Markdown

Review Change Stack

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

Changes

The PR adds field-write readback guidance and stronger dry-run handling, documents auto-number behavior, and unifies record projection aliases across list, search, and get commands with validation and test coverage.

Base field operations

Layer / File(s) Summary
Field request and result handling
shortcuts/base/field_ops.go
Dry-run parsing failures are surfaced, and field create/update results include type-based readback guidance.
Field guidance and validation coverage
shortcuts/base/field_update.go, shortcuts/base/record_upsert.go, shortcuts/base/base_*_test.go, tests/cli_e2e/base/*, skills/lark-base/references/*
Auto-number payloads, rating limits, no-op behavior, readback signals, and related command guidance are documented and tested.

Record projection aliases

Layer / File(s) Summary
Shared record projection flow
shortcuts/base/record_{get,list,ops,query,search}.go
--field-id, --fields, and --field-names use shared parsing, normalization, mutual-exclusion checks, and error propagation.
Projection validation and end-to-end coverage
shortcuts/base/base_*_test.go, tests/cli_e2e/base/base_record_list_dryrun_test.go
Tests cover hidden aliases, JSON-array projections, repeated aliases, duplicates, invalid combinations, and generated request parameters.

Estimated code review effort: 4 (Complex) | ~45 minutes

Sequence Diagram(s)

Field update flow

sequenceDiagram
  participant CLI as base +field-update
  participant DryRun as dryRunFieldUpdate
  participant API as Base field API
  participant Result as fieldUpdateResult
  CLI->>DryRun: parse field JSON
  DryRun-->>CLI: validation error or PUT request
  CLI->>API: submit update
  API-->>Result: field response or no-op error
  Result-->>CLI: readback recommendation
Loading

Record projection flow

sequenceDiagram
  participant CLI as Base record command
  participant Projection as recordProjectionFields
  participant Request as record request builder
  participant API as Base records API
  CLI->>Projection: read projection flags
  Projection-->>CLI: normalized fields or validation error
  CLI->>Request: build select_fields or field_id parameters
  Request->>API: send record query
  API-->>CLI: return records
Loading

Possibly related PRs

Suggested labels: feature

Suggested reviewers: kongenpei

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Title check ✅ Passed The title is concise and accurately reflects the main changes to Base shortcuts and guidance.
Description check ✅ Passed The description follows the required template and covers summary, changes, test plan, and related issues.
✨ 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.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@shortcuts/base/base_execute_test.go`:
- Around line 1785-1791: The new “list projection aliases reject ambiguous
inputs” test is only checking an error message substring, but it should assert
typed problem metadata instead. Update the test around
runShortcut/BaseRecordList to use errs.ProblemOf and verify the expected
category, subtype, and param for the mutually exclusive input error, following
the same pattern used by other tests in this file. Preserve any wrapped cause
assertions as needed rather than relying on strings.Contains(err.Error(),
"mutually exclusive").
🪄 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: b62b7211-e3e2-435a-94db-ee1459af23e1

📥 Commits

Reviewing files that changed from the base of the PR and between 9413e7c and 63dde95.

📒 Files selected for processing (14)
  • shortcuts/base/base_dryrun_ops_test.go
  • shortcuts/base/base_execute_test.go
  • shortcuts/base/base_shortcuts_test.go
  • shortcuts/base/field_ops.go
  • shortcuts/base/field_update.go
  • shortcuts/base/helpers.go
  • shortcuts/base/record_list.go
  • shortcuts/base/record_ops.go
  • shortcuts/base/record_query.go
  • shortcuts/base/record_upsert.go
  • skills/lark-base/references/lark-base-field-json.md
  • skills/lark-base/references/lark-base-field-update.md
  • tests/cli_e2e/base/base_field_update_dryrun_test.go
  • tests/cli_e2e/base/base_record_list_dryrun_test.go

Comment thread shortcuts/base/base_execute_test.go Outdated
@huarenmin13
huarenmin13 force-pushed the table-review-fixes-pr branch from 63dde95 to 6b254de Compare July 9, 2026 12:16
@github-actions github-actions Bot added size/M Single-domain feat or fix with limited business impact and removed size/L Large or sensitive change across domains or core paths labels Jul 9, 2026

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (2)
shortcuts/base/field_ops.go (1)

214-238: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Consolidate fieldCreateResult and fieldUpdateResult into a single helper.

These two functions are identical except for the operation string passed to fieldWriteReadbackRecommendation. Extracting a single fieldWriteResult(result, submitted, operation) eliminates the duplication and ensures future metadata fields are added in one place.

♻️ Proposed consolidation
-func fieldCreateResult(result map[string]interface{}, submitted map[string]interface{}) map[string]interface{} {
-	readbackRecommended, reason := fieldWriteReadbackRecommendation(submitted, "create")
-	result["field_get_recommended"] = readbackRecommended
-	if readbackRecommended {
-		result["next_step"] = "field_get"
-		result["verification_hint"] = reason
-	} else {
-		result["next_step"] = "done"
-		result["verification_hint"] = reason
-	}
-	return result
-}
-
-func fieldUpdateResult(result map[string]interface{}, submitted map[string]interface{}) map[string]interface{} {
-	readbackRecommended, reason := fieldWriteReadbackRecommendation(submitted, "update")
-	result["field_get_recommended"] = readbackRecommended
-	if readbackRecommended {
-		result["next_step"] = "field_get"
-		result["verification_hint"] = reason
-	} else {
-		result["next_step"] = "done"
-		result["verification_hint"] = reason
-	}
-	return result
-}
+func fieldWriteResult(result map[string]interface{}, submitted map[string]interface{}, operation string) map[string]interface{} {
+	readbackRecommended, reason := fieldWriteReadbackRecommendation(submitted, operation)
+	result["field_get_recommended"] = readbackRecommended
+	if readbackRecommended {
+		result["next_step"] = "field_get"
+		result["verification_hint"] = reason
+	} else {
+		result["next_step"] = "done"
+		result["verification_hint"] = reason
+	}
+	return result
+}

Then update call sites:

-	runtime.Out(fieldCreateResult(map[string]interface{}{"field": fields[0], "created": true}, bodies[0]), nil)
+	runtime.Out(fieldWriteResult(map[string]interface{}{"field": fields[0], "created": true}, bodies[0], "create"), nil)
-	runtime.Out(fieldUpdateResult(map[string]interface{}{"field": data, "updated": true}, body), nil)
+	runtime.Out(fieldWriteResult(map[string]interface{}{"field": data, "updated": true}, body, "update"), nil)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@shortcuts/base/field_ops.go` around lines 214 - 238, `fieldCreateResult` and
`fieldUpdateResult` are duplicate implementations that only differ by the
operation string passed to `fieldWriteReadbackRecommendation`. Extract a shared
helper such as `fieldWriteResult(result, submitted, operation)` in
`field_ops.go` that sets `field_get_recommended`, `next_step`, and
`verification_hint`, then have both existing functions delegate to it with their
respective operation values. Update any call sites to use the consolidated
helper path where appropriate so future metadata changes only need to be made in
one place.
shortcuts/base/base_execute_test.go (1)

833-853: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Noop test doesn't assert typed category/subtype.

errs.ProblemOf(err) here only confirms the error is typed, but doesn't check Category/Subtype as the sibling test TestBaseFieldExecuteUpdateRejectsReformatKeyInJSON does. Per path guideline, error-path tests should assert typed metadata (category/subtype/param), not just "is typed."

✅ Proposed strengthening
-	if _, ok := errs.ProblemOf(err); !ok {
+	p, ok := errs.ProblemOf(err)
+	if !ok {
 		t.Fatalf("expected a typed API error, got %T %v", err, err)
 	}
+	if p.Category == "" || p.Subtype == "" {
+		t.Fatalf("expected category/subtype set on typed error, got %+v", p)
+	}

As per path instructions: "Error-path tests must assert typed metadata via errs.ProblemOf (category / subtype / param) and cause preservation, not message substrings alone."

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@shortcuts/base/base_execute_test.go` around lines 833 - 853, The no-op API
error test only checks that errs.ProblemOf(err) returns a typed error, but it
does not verify the typed metadata required by this path. Strengthen
TestBaseFieldExecuteUpdateNoopReturnsAPIError by asserting the returned
problem’s Category, Subtype, and Param fields via errs.ProblemOf, matching the
pattern used in TestBaseFieldExecuteUpdateRejectsReformatKeyInJSON, and keep the
existing cause-preservation check if applicable.

Source: Path instructions

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@shortcuts/base/base_execute_test.go`:
- Around line 833-853: The no-op API error test only checks that
errs.ProblemOf(err) returns a typed error, but it does not verify the typed
metadata required by this path. Strengthen
TestBaseFieldExecuteUpdateNoopReturnsAPIError by asserting the returned
problem’s Category, Subtype, and Param fields via errs.ProblemOf, matching the
pattern used in TestBaseFieldExecuteUpdateRejectsReformatKeyInJSON, and keep the
existing cause-preservation check if applicable.

In `@shortcuts/base/field_ops.go`:
- Around line 214-238: `fieldCreateResult` and `fieldUpdateResult` are duplicate
implementations that only differ by the operation string passed to
`fieldWriteReadbackRecommendation`. Extract a shared helper such as
`fieldWriteResult(result, submitted, operation)` in `field_ops.go` that sets
`field_get_recommended`, `next_step`, and `verification_hint`, then have both
existing functions delegate to it with their respective operation values. Update
any call sites to use the consolidated helper path where appropriate so future
metadata changes only need to be made in one place.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 695356ce-a34f-472f-bc41-ce1e7f24a33e

📥 Commits

Reviewing files that changed from the base of the PR and between 63dde95 and 6b254de.

📒 Files selected for processing (15)
  • shortcuts/base/base_dryrun_ops_test.go
  • shortcuts/base/base_execute_test.go
  • shortcuts/base/base_shortcuts_test.go
  • shortcuts/base/field_ops.go
  • shortcuts/base/field_update.go
  • shortcuts/base/helpers.go
  • shortcuts/base/record_list.go
  • shortcuts/base/record_ops.go
  • shortcuts/base/record_query.go
  • shortcuts/base/record_upsert.go
  • skills/lark-base/references/lark-base-field-create.md
  • skills/lark-base/references/lark-base-field-json.md
  • skills/lark-base/references/lark-base-field-update.md
  • tests/cli_e2e/base/base_field_update_dryrun_test.go
  • tests/cli_e2e/base/base_record_list_dryrun_test.go
✅ Files skipped from review due to trivial changes (2)
  • shortcuts/base/record_upsert.go
  • skills/lark-base/references/lark-base-field-json.md
🚧 Files skipped from review as they are similar to previous changes (4)
  • tests/cli_e2e/base/base_record_list_dryrun_test.go
  • shortcuts/base/record_list.go
  • shortcuts/base/record_query.go
  • shortcuts/base/record_ops.go

@huarenmin13
huarenmin13 force-pushed the table-review-fixes-pr branch from 6b254de to 5b407f7 Compare July 9, 2026 14:58

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (1)
shortcuts/base/field_ops.go (1)

211-259: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Consolidate fieldCreateResult and fieldUpdateResult; drop redundant if/else.

Both functions are identical apart from the operation string passed to fieldWriteReadbackRecommendation, and in each, verification_hint is set to reason in both branches of the if/else — only next_step actually differs. Consider one shared helper.

♻️ Proposed consolidation
-func fieldCreateResult(result map[string]interface{}, submitted map[string]interface{}) map[string]interface{} {
-	readbackRecommended, reason := fieldWriteReadbackRecommendation(submitted, "create")
-	result["field_get_recommended"] = readbackRecommended
-	if readbackRecommended {
-		result["next_step"] = "field_get"
-		result["verification_hint"] = reason
-	} else {
-		result["next_step"] = "done"
-		result["verification_hint"] = reason
-	}
-	return result
-}
+func fieldCreateResult(result map[string]interface{}, submitted map[string]interface{}) map[string]interface{} {
+	return fieldWriteResult(result, submitted, "create")
+}
 
 // fieldCreateBatchResult attaches the same top-level readback contract to a
 // multi-field create. ...
 func fieldCreateBatchResult(result map[string]interface{}, submitted []map[string]interface{}) map[string]interface{} {
 	...
 }
 
-func fieldUpdateResult(result map[string]interface{}, submitted map[string]interface{}) map[string]interface{} {
-	readbackRecommended, reason := fieldWriteReadbackRecommendation(submitted, "update")
-	result["field_get_recommended"] = readbackRecommended
-	if readbackRecommended {
-		result["next_step"] = "field_get"
-		result["verification_hint"] = reason
-	} else {
-		result["next_step"] = "done"
-		result["verification_hint"] = reason
-	}
-	return result
-}
+func fieldUpdateResult(result map[string]interface{}, submitted map[string]interface{}) map[string]interface{} {
+	return fieldWriteResult(result, submitted, "update")
+}
+
+func fieldWriteResult(result map[string]interface{}, submitted map[string]interface{}, operation string) map[string]interface{} {
+	readbackRecommended, reason := fieldWriteReadbackRecommendation(submitted, operation)
+	result["field_get_recommended"] = readbackRecommended
+	result["verification_hint"] = reason
+	if readbackRecommended {
+		result["next_step"] = "field_get"
+	} else {
+		result["next_step"] = "done"
+	}
+	return result
+}
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@shortcuts/base/field_ops.go` around lines 211 - 259, `fieldCreateResult` and
`fieldUpdateResult` duplicate the same branching logic, with only the operation
passed to `fieldWriteReadbackRecommendation` differing. Refactor them to share a
small helper that sets `field_get_recommended`, `next_step`, and
`verification_hint` once, and have both functions call it with their respective
operation strings; keep `fieldCreateBatchResult` unchanged unless it can use the
same helper too.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@shortcuts/base/base_shortcuts_test.go`:
- Around line 1237-1244: The new mutual-exclusivity test for
BaseRecordSearch.Validate should assert the validation error using typed
metadata instead of only matching the message text. Update the test around
BaseRecordSearch.Validate/newBaseTestRuntimeWithArrays to use errs.ProblemOf and
verify the expected category, subtype, and param for the --field-id/--fields
conflict, while also preserving the underlying cause. Keep the substring check
out of the assertion path and follow the same typed-error pattern used by the
other tests in this file/package.

---

Nitpick comments:
In `@shortcuts/base/field_ops.go`:
- Around line 211-259: `fieldCreateResult` and `fieldUpdateResult` duplicate the
same branching logic, with only the operation passed to
`fieldWriteReadbackRecommendation` differing. Refactor them to share a small
helper that sets `field_get_recommended`, `next_step`, and `verification_hint`
once, and have both functions call it with their respective operation strings;
keep `fieldCreateBatchResult` unchanged unless it can use the same helper too.
🪄 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: 7bba421d-3020-4054-acd9-e105a81858e4

📥 Commits

Reviewing files that changed from the base of the PR and between 6b254de and 5b407f7.

📒 Files selected for processing (15)
  • shortcuts/base/base_dryrun_ops_test.go
  • shortcuts/base/base_execute_test.go
  • shortcuts/base/base_shortcuts_test.go
  • shortcuts/base/field_ops.go
  • shortcuts/base/field_update.go
  • shortcuts/base/helpers.go
  • shortcuts/base/record_list.go
  • shortcuts/base/record_ops.go
  • shortcuts/base/record_query.go
  • shortcuts/base/record_upsert.go
  • skills/lark-base/references/lark-base-field-create.md
  • skills/lark-base/references/lark-base-field-json.md
  • skills/lark-base/references/lark-base-field-update.md
  • tests/cli_e2e/base/base_field_update_dryrun_test.go
  • tests/cli_e2e/base/base_record_list_dryrun_test.go
✅ Files skipped from review due to trivial changes (6)
  • shortcuts/base/record_upsert.go
  • shortcuts/base/field_update.go
  • shortcuts/base/record_list.go
  • skills/lark-base/references/lark-base-field-json.md
  • skills/lark-base/references/lark-base-field-create.md
  • skills/lark-base/references/lark-base-field-update.md
🚧 Files skipped from review as they are similar to previous changes (6)
  • tests/cli_e2e/base/base_field_update_dryrun_test.go
  • shortcuts/base/helpers.go
  • shortcuts/base/record_query.go
  • tests/cli_e2e/base/base_record_list_dryrun_test.go
  • shortcuts/base/record_ops.go
  • shortcuts/base/base_dryrun_ops_test.go

Comment thread shortcuts/base/base_shortcuts_test.go Outdated
@huarenmin13

Copy link
Copy Markdown
Contributor Author

Addressed all current CodeRabbit findings in aee1ec3:

  • Both projection-conflict tests now assert validation category, subtype, and --field-id param metadata.
  • The no-op API test now asserts CategoryAPI, SubtypeUnknown, code 800070003, and the concrete APIError type. An API-side error has no flag param to fabricate.
  • Field create, batch-create, and update now share one metadata decorator while field update still prefers the type returned by the same PUT response.
  • The PR description now follows the repository template with Changes, Test Plan, and Related Issues sections.

The generic docstring-coverage warning was reviewed; this change adds no exported API, so adding repetitive comments to internal helpers and tests would not improve the public contract.

@huarenmin13

Copy link
Copy Markdown
Contributor Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Jul 10, 2026

Copy link
Copy Markdown
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@huarenmin13

Copy link
Copy Markdown
Contributor Author

@coderabbitai resume

@coderabbitai

coderabbitai Bot commented Jul 10, 2026

Copy link
Copy Markdown
✅ Action performed

Reviews resumed.

@huarenmin13 huarenmin13 changed the title fix(base): keep attributable token compression changes fix(base): improve table shortcut behavior & guidance Jul 10, 2026
@github-actions

github-actions Bot commented Jul 10, 2026

Copy link
Copy Markdown

🚀 PR Preview Install Guide

🧰 CLI update

npm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@729883ca4af0173dfc5637e71f68cd3ff884f999

🧩 Skill update

npx skills add huarenmin13/cli#table-review-fixes-pr -y -g

Comment thread skills/lark-base/references/lark-base-field-update.md Outdated

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@shortcuts/base/base_execute_test.go`:
- Around line 1959-1961: Update both error-path assertions in
shortcuts/base/base_execute_test.go at lines 1959-1961 and 2317-2319 to validate
errs.ProblemOf(err) has CategoryValidation and SubtypeInvalidArgument, while
retaining the existing “duplicate field id” message-substring check. Follow the
typed error assertion pattern already used elsewhere in the file.
🪄 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: 5a19f822-e7c7-4abc-8a5f-8c29a5bda81c

📥 Commits

Reviewing files that changed from the base of the PR and between 7462992 and ddd5e03.

📒 Files selected for processing (10)
  • shortcuts/base/base_dryrun_ops_test.go
  • shortcuts/base/base_execute_test.go
  • shortcuts/base/base_shortcuts_test.go
  • shortcuts/base/field_ops.go
  • shortcuts/base/record_get.go
  • shortcuts/base/record_list.go
  • shortcuts/base/record_ops.go
  • shortcuts/base/record_query.go
  • shortcuts/base/record_search.go
  • skills/lark-base/references/lark-base-field-update.md
🚧 Files skipped from review as they are similar to previous changes (4)
  • shortcuts/base/base_dryrun_ops_test.go
  • skills/lark-base/references/lark-base-field-update.md
  • shortcuts/base/record_query.go
  • shortcuts/base/field_ops.go

Comment thread shortcuts/base/base_execute_test.go Outdated
@github-actions github-actions Bot added size/L Large or sensitive change across domains or core paths and removed size/M Single-domain feat or fix with limited business impact labels Jul 17, 2026

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
shortcuts/base/base_execute_test.go (1)

2324-2368: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Remove superseded duplicate tests and align get duplicate projection coverage.

This new parameterized test fully covers the validation previously tested in t.Run("get rejects mixed field-id and json select_fields") (lines 2396-2402), making the older test redundant.

Additionally, consider adding a similar parameterized test for get to verify that duplicate canonical and alias projections are rejected consistently, matching the new coverage you added for list at line 1946. This would replace the older un-parameterized t.Run("get rejects duplicate field ids") test (lines 2380-2386).

♻️ Proposed test removal and addition

Delete the redundant get rejects mixed field-id and json select_fields and get rejects duplicate field ids tests, and add the following parameterized test:

	t.Run("get canonical and alias projections reject duplicates consistently", func(t *testing.T) {
		tests := []struct {
			name  string
			args  []string
			param string
		}{
			{name: "canonical", args: []string{"--field-id", "Name", "--field-id", "Name"}, param: "--field-id"},
			{name: "fields alias", args: []string{"--fields", `["Name","Name"]`}, param: "--fields"},
			{name: "field names alias", args: []string{"--field-names", "Name", "--field-names", "Name"}, param: "--field-names"},
		}
		for _, tc := range tests {
			t.Run(tc.name, func(t *testing.T) {
				factory, stdout, _ := newExecuteFactory(t)
				args := append([]string{"+record-get", "--base-token", "app_x", "--table-id", "tbl_x", "--record-id", "rec_1"}, tc.args...)
				err := runShortcut(t, BaseRecordGet, args, factory, stdout)
				if err == nil {
					t.Fatal("expected duplicate projection error, got nil")
				}
				p, ok := errs.ProblemOf(err)
				if !ok || p.Category != errs.CategoryValidation || p.Subtype != errs.SubtypeInvalidArgument {
					t.Fatalf("expected invalid-argument validation problem, got %T %v", err, err)
				}
				var validationErr *errs.ValidationError
				if !errors.As(err, &validationErr) {
					t.Fatalf("expected ValidationError, got %T %v", err, err)
				}
				if validationErr.Param != tc.param || len(validationErr.Params) != 1 || validationErr.Params[0].Name != tc.param {
					t.Fatalf("param/params=%q/%#v, want %q", validationErr.Param, validationErr.Params, tc.param)
				}
				if !strings.Contains(err.Error(), "duplicate field id") {
					t.Fatalf("err=%v", err)
				}
			})
		}
	})
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@shortcuts/base/base_execute_test.go` around lines 2324 - 2368, In the get
test suite, remove the superseded “get rejects mixed field-id and json
select_fields” and “get rejects duplicate field ids” cases. Add a parameterized
test alongside the existing get projection tests covering duplicate canonical,
fields-alias, and field-names-alias inputs, asserting each returns an
invalid-argument ValidationError with the corresponding parameter metadata and
duplicate-field error.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@shortcuts/base/base_execute_test.go`:
- Around line 2324-2368: In the get test suite, remove the superseded “get
rejects mixed field-id and json select_fields” and “get rejects duplicate field
ids” cases. Add a parameterized test alongside the existing get projection tests
covering duplicate canonical, fields-alias, and field-names-alias inputs,
asserting each returns an invalid-argument ValidationError with the
corresponding parameter metadata and duplicate-field error.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 1e3f98c9-4b6a-4bc4-9354-b9f15ac77634

📥 Commits

Reviewing files that changed from the base of the PR and between ddd5e03 and 340ca05.

📒 Files selected for processing (3)
  • shortcuts/base/base_execute_test.go
  • shortcuts/base/helpers_test.go
  • shortcuts/base/record_ops.go
🚧 Files skipped from review as they are similar to previous changes (1)
  • shortcuts/base/record_ops.go

@huarenmin13
huarenmin13 force-pushed the table-review-fixes-pr branch from 0d80e70 to 7462992 Compare July 17, 2026 12:01
@github-actions github-actions Bot added size/M Single-domain feat or fix with limited business impact and removed size/L Large or sensitive change across domains or core paths labels Jul 17, 2026
@huarenmin13
huarenmin13 force-pushed the table-review-fixes-pr branch from 7462992 to 2af257b Compare July 17, 2026 12:30
@github-actions github-actions Bot added size/L Large or sensitive change across domains or core paths and removed size/M Single-domain feat or fix with limited business impact labels Jul 17, 2026
@huarenmin13
huarenmin13 force-pushed the table-review-fixes-pr branch from 2af257b to 65c28c1 Compare July 17, 2026 13:12
huarenmin13 and others added 2 commits July 21, 2026 17:47
1. Treat select_fields:null as omitted before record-get projection conflict checks.
2. Add dry-run E2E coverage for omitted and flag-projection cases.

```ai-signature
改动范围: shortcuts/base/record_ops.go 与 tests/cli_e2e/base/base_record_list_dryrun_test.go,仅调整 record-get 对 JSON null projection 的处理和回归验证
思考过程: 保持现有 projection normalizer 与互斥规则不变,只在读取 select_fields 后把 null 与缺失键等价,避免扩大到字段上限或 auto_number 行为
改动原因: PR 1803 声明 list search get 使用统一 projection contract,但 record-get 对 select_fields:null 仍返回 invalid_argument,与 record-search 不一致
Break Change: 否;仅将此前失败的 select_fields:null 输入规范化为省略,并保留 flag projection
```

Co-authored-by: BASE Infra Harness <ai@base-infra-harness.noreply.local>
AI-SHA256: b3d37c6c026f0215d994bc7c9bad4c65caee1b3bc2e9584ff20403a4d06969c3
1. Centralize Base dry-run environment setup, timeout handling, command execution,
    and exit-code assertions in runBaseDryRun.
2. Migrate record projection and field update dry-run tests without changing their contract assertio
    ns or covered scenarios.
3. Verify all 11 affected top-level tests and four projection subtests with the current-HEAD binary
    under race mode.

```ai-signature
改动范围: tests/cli_e2e/base/helpers_test.go、base_record_list_dryrun_test.go 与 base_field_update_dryrun_test.go,仅收敛 dry-run 测试执行脚手架
思考过程: 复用现有测试基础设施,把环境隔离、超时、dry-run 参数、命令执行和退出码断言集中到一个 helper,同时保留每个用例的业务断言
改动原因: PR 1803 的新增测试占主要改动量,其中 11 处重复执行模板可安全去重,降低评审体量而不削减 P1 或 P2 场景覆盖
Break Change: 否
```

Co-authored-by: BASE Infra Harness <ai@base-infra-harness.noreply.local>
AI-SHA256: ee39fef8497de65ecea1a0f22d9d87f1622c3f69daa5743ba7fd4c874dbb2ed3
Comment thread shortcuts/base/base_dryrun_ops_test.go
@zhouyue-bytedance
zhouyue-bytedance merged commit 483aade into larksuite:main Jul 21, 2026
22 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

domain/base PR touches the base 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.

4 participants