fix(base): improve dashboard shortcut guidance#1787
Merged
zhouyue-bytedance merged 12 commits intoJul 16, 2026
Merged
Conversation
github-actions
Bot
added
domain/base
|
Note Reviews pausedIt 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 Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthrough本次更新规范化仪表盘排序配置，补充漏斗图与仪表盘创建指导，为 get-data 增加兼容参数，并为记录列表加入 Changes仪表盘排序与创建指导
记录列表参数别名
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
skills/lark-base/references/dashboard-block-data-config.md (1)
285-295: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winFix the current-stage funnel template to use
count_all.The “当前阶段分布” example still shows a
seriesaggregation, but the surrounding text says this path should usecount_all:true. That contradicts the documentedseries/count_allcontract and sends users to the wrong payload shape.🔧 Proposed fix
{ "table_name": "表名", - "series": [{ "field_name": "数值字段", "rollup": "SUM" }], + "count_all": true, "group_by": [{ "field_name": "状态字段", "mode": "integrated" }] }🤖 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 `@skills/lark-base/references/dashboard-block-data-config.md` around lines 285 - 295, The “当前阶段分布” example in the dashboard block data config is using the wrong payload shape: it shows a series aggregation even though this path is supposed to use count_all:true. Update the funnel template in the current-stage section to match the documented contract by removing the series-based example and showing the count_all form, keeping group_by with the状态字段 so users copy the correct low-cost stage-count payload.
🧹 Nitpick comments (1)
tests/cli_e2e/base/base_dashboard_block_get_data_dryrun_test.go (1)
62-142: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winFile name no longer matches contents.
This file is named
base_dashboard_block_get_data_dryrun_test.go, but now also holdsTestBaseDashboardBlockWriteDryRuncovering create/update/delete. Consider moving the write test to a separate file (e.g.,base_dashboard_block_write_dryrun_test.go) for clearer file-to-test-suite mapping.🤖 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 `@tests/cli_e2e/base/base_dashboard_block_get_data_dryrun_test.go` around lines 62 - 142, The file name no longer matches the test content: this file now contains TestBaseDashboardBlockWriteDryRun for create/update/delete rather than only get-data dry-run coverage. Move the write dry-run test group out of the current file into a more appropriate test file such as a dashboard-block-write dry-run test, and keep the existing get-data dry-run tests in the file that matches their behavior so the suite-to-file mapping stays clear.
🤖 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 `@tests/cli_e2e/base/base_dashboard_block_get_data_dryrun_test.go`:
- Around line 86-91: The create subtest for base dashboard block dry-run is
missing assertions for the request-body fields passed via --name and
--data-config, so update that test to verify the generated payload includes both
the Unicode name and the data-config content. Use the existing dry-run output
checks in base_dashboard_block_get_data_dryrun_test.go, and add assertions
alongside the current method, dashboard_id, and type checks so the create path
in the CLI confirms both fields are serialized correctly.
---
Outside diff comments:
In `@skills/lark-base/references/dashboard-block-data-config.md`:
- Around line 285-295: The “当前阶段分布” example in the dashboard block data config
is using the wrong payload shape: it shows a series aggregation even though this
path is supposed to use count_all:true. Update the funnel template in the
current-stage section to match the documented contract by removing the
series-based example and showing the count_all form, keeping group_by with
the状态字段 so users copy the correct low-cost stage-count payload.
---
Nitpick comments:
In `@tests/cli_e2e/base/base_dashboard_block_get_data_dryrun_test.go`:
- Around line 62-142: The file name no longer matches the test content: this
file now contains TestBaseDashboardBlockWriteDryRun for create/update/delete
rather than only get-data dry-run coverage. Move the write dry-run test group
out of the current file into a more appropriate test file such as a
dashboard-block-write dry-run test, and keep the existing get-data dry-run tests
in the file that matches their behavior so the suite-to-file mapping stays
clear.
🪄 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: 8eaf0ffe-e208-418f-adcd-172e7d31fefa
📒 Files selected for processing (3)
skills/lark-base/references/dashboard-block-data-config.mdskills/lark-base/references/lark-base-dashboard.mdtests/cli_e2e/base/base_dashboard_block_get_data_dryrun_test.go
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@3092ea7d3dc7fcae8db365ecc2a15e608cc5cd54🧩 Skill updatenpx skills add zhouyue-bytedance/cli#dashboard-review-fixes-pr -y -g |
zhouyue-bytedance
force-pushed
the
dashboard-review-fixes-pr
branch
from
July 8, 2026 02:48
b56f2c2 to
5657b5f
Compare
zhouyue-bytedance
changed the title
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
tests/cli_e2e/base/base_dashboard_block_get_data_dryrun_test.go (1)
92-117: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winCreate subtest still missing
nameanddata-configassertions.This was flagged in a prior review and remains unresolved. The
updatesubtest correctly asserts"text": "库存说明"(line 142), but thecreatesubtest supplies--name "订单趋势"and--data-config '{"table_name":"订单表","count_all":true}'without asserting either in the output, leaving request-body construction unverified.✅ Suggested additional assertions
assert.Contains(t, output, `"type": "column"`) + assert.Contains(t, output, `"name": "订单趋势"`) + assert.Contains(t, output, `"table_name": "订单表"`) })🤖 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 `@tests/cli_e2e/base/base_dashboard_block_get_data_dryrun_test.go` around lines 92 - 117, The create subtest in base_dashboard_block_get_data_dryrun_test.go still does not verify the request body fields for --name and --data-config. Update the create case in TestBaseDashboardBlockGetDataDryRun to assert the dry-run output contains the expected name value ("订单趋势") and the serialized data-config content from the request, alongside the existing checks on the POST endpoint and dashboard/type fields, so the request construction is fully covered.
🧹 Nitpick comments (2)
shortcuts/base/helpers.go (1)
1141-1143: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueValidation message may be misleading after normalization.
Since
normalizeDataConfigsilently defaultsorderto"asc"forgroup/viewtypes before validation runs, the error message at line 1142 ("sort 存在时必须设置 order") will only ever fire fortype=valuewith a missing order. The message implies order is universally required, but forgroup/viewit's auto-filled. Consider clarifying the message to reflect that this only applies whentype=value, or add a note thatgroup/viewdefault toasc.🤖 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/helpers.go` around lines 1141 - 1143, The validation text in the group_by sort order check is misleading because normalizeDataConfig already defaults order to asc for group/view before validation. Update the error message in the sort validation path in helpers.go (the branch that checks o == "" and o != "asc"/"desc") so it clearly states the requirement only applies to type=value, or explicitly note that group/view default to asc; keep the message aligned with the actual behavior in normalizeDataConfig and the group_by validation logic.shortcuts/base/record_list.go (1)
86-93: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider a hidden-flag test for
--field-names, mirroringTestBaseRecordListJSONAliasIsHidden.The PR adds a dedicated test verifying
--jsonstays hidden from help output, but there's no equivalent assertion for--field-names. Since both are meant to be hidden aliases, a regression hiding one but not the other would go undetected.🤖 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/record_list.go` around lines 86 - 93, Add a hidden-flag test for the `recordListFieldNamesAliasFlag` alias so it is covered the same way as `TestBaseRecordListJSONAliasIsHidden`. Extend the existing shortcut flag tests in the record list area to assert that `--field-names` remains hidden from help output and is treated as a hidden alias for `--field-id`, using the existing `common.Flag`/`recordListFieldNamesAliasFlag` symbol to locate the change.
🤖 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/helpers.go`:
- Around line 1041-1053: Add tests in normalizeDataConfig coverage to verify
sort.order defaults correctly for sort type group and view, and remains unset
for value. Update helpers_test.go with cases that exercise the sort handling
logic in normalizeDataConfig, asserting sub["order"] becomes asc only when
sub["type"] is group or view and no order is provided, and that the value path
does not introduce an order default.
In `@shortcuts/base/record_list.go`:
- Around line 109-123: The mutual-exclusivity check in
normalizeRecordListJSONAlias is too strict because it rejects cases where --json
was explicitly set to false alongside --format. Update the logic so the conflict
is only raised when the json alias is actually enabled, and keep the existing
alias handling for the true case while preserving the
runtime.Cmd.Flags().Set("format", "json") behavior and runtime.Format
assignment.
---
Duplicate comments:
In `@tests/cli_e2e/base/base_dashboard_block_get_data_dryrun_test.go`:
- Around line 92-117: The create subtest in
base_dashboard_block_get_data_dryrun_test.go still does not verify the request
body fields for --name and --data-config. Update the create case in
TestBaseDashboardBlockGetDataDryRun to assert the dry-run output contains the
expected name value ("订单趋势") and the serialized data-config content from the
request, alongside the existing checks on the POST endpoint and dashboard/type
fields, so the request construction is fully covered.
---
Nitpick comments:
In `@shortcuts/base/helpers.go`:
- Around line 1141-1143: The validation text in the group_by sort order check is
misleading because normalizeDataConfig already defaults order to asc for
group/view before validation. Update the error message in the sort validation
path in helpers.go (the branch that checks o == "" and o != "asc"/"desc") so it
clearly states the requirement only applies to type=value, or explicitly note
that group/view default to asc; keep the message aligned with the actual
behavior in normalizeDataConfig and the group_by validation logic.
In `@shortcuts/base/record_list.go`:
- Around line 86-93: Add a hidden-flag test for the
`recordListFieldNamesAliasFlag` alias so it is covered the same way as
`TestBaseRecordListJSONAliasIsHidden`. Extend the existing shortcut flag tests
in the record list area to assert that `--field-names` remains hidden from help
output and is treated as a hidden alias for `--field-id`, using the existing
`common.Flag`/`recordListFieldNamesAliasFlag` symbol to locate the 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: 3d9aef19-d386-42f5-be7f-b1c52afee3de
📒 Files selected for processing (15)
shortcuts/base/base_data_query.goshortcuts/base/base_dryrun_ops_test.goshortcuts/base/base_execute_test.goshortcuts/base/base_shortcuts_test.goshortcuts/base/dashboard_block_create.goshortcuts/base/dashboard_block_get.goshortcuts/base/dashboard_block_get_data.goshortcuts/base/dashboard_create.goshortcuts/base/helpers.goshortcuts/base/record_batch_create.goshortcuts/base/record_list.goshortcuts/base/record_ops.goskills/lark-base/references/dashboard-block-data-config.mdskills/lark-base/references/lark-base-dashboard.mdtests/cli_e2e/base/base_dashboard_block_get_data_dryrun_test.go
✅ Files skipped from review due to trivial changes (5)
- shortcuts/base/dashboard_block_get.go
- shortcuts/base/record_batch_create.go
- shortcuts/base/dashboard_create.go
- shortcuts/base/base_data_query.go
- shortcuts/base/dashboard_block_create.go
🚧 Files skipped from review as they are similar to previous changes (2)
- skills/lark-base/references/lark-base-dashboard.md
- skills/lark-base/references/dashboard-block-data-config.md
zhouyue-bytedance
force-pushed
the
dashboard-review-fixes-pr
branch
2 times, most recently
from
July 8, 2026 06:39
42c7889 to
a627848
Compare
There was a problem hiding this comment.
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 `@skills/lark-base/references/dashboard-block-data-config.md`:
- Around line 93-95: The wording around `sort` auto-fill is too broad and should
be limited to the create flow. Update the guidance in
`dashboard-block-data-config.md` so it only states that block creation via the
create path normalizes `sort.type:"group"` or `"view"` without an `order` to
`order:"asc"` before validation, and avoid implying the same behavior for update
or dry-run unless those paths share the same normalization logic. Keep the
explicit `sort.type:"value"` requirement and the helper-table example tied to
first-time block creation.
🪄 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: b1b339c0-68cc-4611-905a-2740236a18c4
📒 Files selected for processing (13)
shortcuts/base/base_dryrun_ops_test.goshortcuts/base/base_execute_test.goshortcuts/base/base_shortcuts_test.goshortcuts/base/dashboard_block_create.goshortcuts/base/dashboard_block_get.goshortcuts/base/dashboard_block_get_data.goshortcuts/base/helpers.goshortcuts/base/record_batch_create.goshortcuts/base/record_list.goshortcuts/base/record_ops.goskills/lark-base/references/dashboard-block-data-config.mdskills/lark-base/references/lark-base-dashboard.mdtests/cli_e2e/base/base_dashboard_block_get_data_dryrun_test.go
✅ Files skipped from review due to trivial changes (4)
- shortcuts/base/dashboard_block_get.go
- shortcuts/base/record_batch_create.go
- shortcuts/base/dashboard_block_create.go
- shortcuts/base/base_dryrun_ops_test.go
🚧 Files skipped from review as they are similar to previous changes (8)
- shortcuts/base/dashboard_block_get_data.go
- shortcuts/base/record_list.go
- tests/cli_e2e/base/base_dashboard_block_get_data_dryrun_test.go
- skills/lark-base/references/lark-base-dashboard.md
- shortcuts/base/base_execute_test.go
- shortcuts/base/helpers.go
- shortcuts/base/record_ops.go
- shortcuts/base/base_shortcuts_test.go
zhouyue-bytedance
force-pushed
the
dashboard-review-fixes-pr
branch
from
July 8, 2026 06:56
a627848 to
4676feb
Compare
zhouyue-bytedance
force-pushed
the
dashboard-review-fixes-pr
branch
from
July 8, 2026 07:34
4676feb to
833684e
Compare
The 'do not audit every block after creation' hint duplicates the create-then-suppress-get guidance already in lark-base-dashboard.md, so remove it from the +dashboard-block-get tips to keep them focused.
Commit 778da63 removed the 'do not audit every block' tip from the +dashboard-block-get source as redundant but left the matching assertion in TestBaseDashboardHelpGuidesAgents, breaking the unit test. Remove the stale assertion to realign the test with the tips.
… defaulting to asc
… raw pass-through
…d data-query outputs
…view-fixes-pr # Conflicts: # skills/lark-base/references/lark-base-dashboard-block-get-data.md
zgz2048
approved these changes
Jul 16, 2026
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
3 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
Bring the GitHub PR in line with the current branch diff against local main while excluding
harness/anddownloads/. This improves Base dashboard shortcut affordances, dry-run coverage, and agent guidance for dashboard/funnel workflows.Changes
Test Plan
make unit-testnot run)go test ./shortcuts/base -count=1go build -buildvcs=false -trimpath -o lark-cli .go test ./tests/cli_e2e/base -run 'TestBaseDashboardBlockGetDataDryRun' -count=1Related Issues
Summary by CodeRabbit
--field-names（与--field-id互斥）；并支持别名场景下字段回显更一致。+dashboard-block-get-data接受但忽略--dashboard-id（兼容）。data_config，--dry-run用于校验/排错；arrange仅在需要时使用。group_by.sort保序；并强化sort与sort.order的约束与校验（含默认与非法值处理）。