feat(base): support ranking dashboard blocks - #2306
Conversation
github-actions
Bot
added
domain/base
|
|
📝 WalkthroughWalkthroughThe CLI now supports ranking dashboard blocks with normalized defaults, strict configuration validation, partial ChangesRanking dashboard blocks
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant CLI
participant DataConfigNormalizer
participant RankingValidator
participant DashboardBlockAPI
CLI->>DataConfigNormalizer: normalize ranking data-config
DataConfigNormalizer->>RankingValidator: validate ranking fields and limits
RankingValidator-->>CLI: return normalized configuration
CLI->>DashboardBlockAPI: create or patch ranking block
DashboardBlockAPI-->>CLI: return ranking block request result
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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: 3
🤖 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_dashboard_execute_test.go`:
- Around line 733-735: Update the error assertion around runShortcut to use
errors.As and inspect the resulting *errs.ValidationError. Assert that the typed
validation error has Param equal to "--data-config" and the expected
invalid-argument subtype, while preserving the existing failure handling; do not
rely on matching err.Error() as the sole validation.
In `@shortcuts/base/dashboard_block_create.go`:
- Around line 51-55: Add a nearby regression test for the ranking creation path
without --data-config, asserting it returns the typed validation error metadata
and includes the --data-config parameter. Use the existing ranking creation test
setup and ensure the test fails if the ranking case is removed from the switch
in the validation flow.
In `@skills/lark-base/references/dashboard-block-data-config.md`:
- Line 108: Unify the documented ranking filter-patch semantics across
skills/lark-base/references/dashboard-block-data-config.md:108-108 and
skills/lark-base/references/lark-base-dashboard.md:145-150. Align the
limit_size-only update with the implementation and partial-update contract: if
omitting filter resets to all records, update the dashboard example accordingly;
otherwise document and preserve the existing filter consistently. No direct code
change is required.
🪄 Autofix
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 Plus
Run ID: 14fbe1be-658c-4be1-8d77-576ecc6e6ef5
📒 Files selected for processing (9)
shortcuts/base/base_dashboard_execute_test.goshortcuts/base/base_shortcuts_test.goshortcuts/base/dashboard_block_create.goshortcuts/base/dashboard_block_update.goshortcuts/base/helpers.goskills/lark-base/references/dashboard-block-data-config.mdskills/lark-base/references/lark-base-dashboard-block-get-data.mdskills/lark-base/references/lark-base-dashboard.mdtests/cli_e2e/base/base_dashboard_block_ranking_dryrun_test.go
| err := runShortcut(t, BaseDashboardBlockCreate, args, factory, stdout) | ||
| if err == nil || !strings.Contains(err.Error(), tc.want) { | ||
| t.Fatalf("err=%v, want %q", err, tc.want) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Assert the typed validation error contract.
Do not use message matching as the only assertion. Assert the validation category and invalid-argument subtype. Use errors.As to inspect *errs.ValidationError and assert Param == "--data-config".
Based on learnings, errs.ProblemOf(err) does not expose Param. As per coding guidelines, “Error tests must assert typed metadata and cause preservation rather than message text 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_dashboard_execute_test.go` around lines 733 - 735, Update
the error assertion around runShortcut to use errors.As and inspect the
resulting *errs.ValidationError. Assert that the typed validation error has
Param equal to "--data-config" and the expected invalid-argument subtype, while
preserving the existing failure handling; do not rely on matching err.Error() as
the sole validation.
Sources: Coding guidelines, Learnings
| switch strings.ToLower(runtime.Str("type")) { | ||
| case "text": | ||
| return errs.NewValidationError(errs.SubtypeInvalidArgument, "text 类型组件必须提供 data-config，包含必填字段 text").WithParam("--data-config") | ||
| case "ranking": | ||
| return errs.NewValidationError(errs.SubtypeInvalidArgument, "ranking 类型组件必须提供 data-config").WithParam("--data-config") |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add a regression test for the missing ranking configuration path.
No supplied test runs ranking creation without --data-config. Removing case "ranking" would preserve the current ranking tests. Add a test that asserts the typed validation metadata and --data-config parameter.
As per coding guidelines, “Every behavior change requires a nearby regression test that fails when the implementation is reverted.”
🤖 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/dashboard_block_create.go` around lines 51 - 55, Add a nearby
regression test for the ranking creation path without --data-config, asserting
it returns the typed validation error metadata and includes the --data-config
parameter. Use the existing ranking creation test setup and ensure the test
fails if the ranking case is removed from the switch in the validation flow.
Source: Coding guidelines
| - `limit_size` 是 Top N，取值为 `1..500` 的整数，创建时省略默认 `10`。 | ||
| - 不支持顶层 `sort`、公开 `ranking` 对象或头像开关。 | ||
|
|
||
| 更新 `ranking` 时，`data_config` 是顶层 patch：只传 `limit_size` 只改 Top N；只传 `group_by` 只替换唯一分组和排序；只传 `series` 或 `count_all:true` 只切换指标；只传 `filter` 只替换筛选。切换 `table_name` 时必须在同一 patch 提供新的 `group_by` 以及 `series` 或 `count_all:true`；未传 `filter` 会重置为全部记录，未传 `limit_size` 保留原 Top N。 |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Unify ranking filter patch semantics.
The two references document different behavior for a ranking update that only changes limit_size.
skills/lark-base/references/dashboard-block-data-config.md#L108-L108: align the documentedfilterbehavior with the implementation and the partial-update contract.skills/lark-base/references/lark-base-dashboard.md#L145-L150: update the example if omittedfilterintentionally resets to all records; otherwise preserve the existing filter consistently.
📍 Affects 2 files
skills/lark-base/references/dashboard-block-data-config.md#L108-L108(this comment)skills/lark-base/references/lark-base-dashboard.md#L145-L150
🤖 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` at line 108,
Unify the documented ranking filter-patch semantics across
skills/lark-base/references/dashboard-block-data-config.md:108-108 and
skills/lark-base/references/lark-base-dashboard.md:145-150. Align the
limit_size-only update with the implementation and partial-update contract: if
omitting filter resets to all records, update the dashboard example accordingly;
otherwise document and preserve the existing filter consistently. No direct code
change is required.
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 `@tests/cli_e2e/base/base_dashboard_block_ranking_dryrun_test.go`:
- Around line 74-75: Update the table cases in the dry-run validation tests to
include expected error.type, error.subtype, error.param, and error.message
values, and import encoding/json. Parse result.Stderr as the validation JSON
envelope and assert those typed fields for each case, replacing the
message-substring-only check while preserving the existing exit-code and
empty-stdout assertions.
🪄 Autofix
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 Plus
Run ID: 7e3450a2-db75-41b0-8ef2-f73cd1252cf2
📒 Files selected for processing (3)
shortcuts/base/base_dashboard_execute_test.goshortcuts/base/helpers.gotests/cli_e2e/base/base_dashboard_block_ranking_dryrun_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- shortcuts/base/helpers.go
- shortcuts/base/base_dashboard_execute_test.go
| require.Contains(t, result.Stderr, tc.want) | ||
| require.Empty(t, result.Stdout) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert the typed validation envelope.
The message substring check can pass when error.type, error.subtype, or error.param is wrong or missing. Parse result.Stderr as the validation JSON envelope and assert error.type, error.subtype, error.param, and error.message for each table case. Keep the existing exit-code and empty-stdout checks.
As per coding guidelines, error tests must assert typed metadata and cause preservation rather than message text alone. Based on learnings, validation-stage E2E failures must write the typed envelope to stderr and assert these fields.
Suggested assertion shape
- require.Contains(t, result.Stderr, tc.want)
+ var envelope struct {
+ Error struct {
+ Type string `json:"type"`
+ Subtype string `json:"subtype"`
+ Param string `json:"param"`
+ Message string `json:"message"`
+ } `json:"error"`
+ }
+ require.NoError(t, json.Unmarshal([]byte(result.Stderr), &envelope))
+ require.Equal(t, tc.wantType, envelope.Error.Type)
+ require.Equal(t, tc.wantSubtype, envelope.Error.Subtype)
+ require.Equal(t, tc.wantParam, envelope.Error.Param)
+ require.Equal(t, tc.want, envelope.Error.Message)Add the expected typed fields to each table case and import encoding/json.
🤖 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_ranking_dryrun_test.go` around lines
74 - 75, Update the table cases in the dry-run validation tests to include
expected error.type, error.subtype, error.param, and error.message values, and
import encoding/json. Parse result.Stderr as the validation JSON envelope and
assert those typed fields for each case, replacing the message-substring-only
check while preserving the existing exit-code and empty-stdout assertions.
Sources: Coding guidelines, Learnings
2 participants
Summary
Tests
Summary by CodeRabbit
New Features
Bug Fixes
Documentation