Skip to content

feat(base): add --position and statistics number_format to dashboard-block create/update - #2118

Open
Neseria wants to merge 2 commits into
larksuite:mainfrom
Neseria:feat/dashboard-layout-precision-202607
Open

feat(base): add --position and statistics number_format to dashboard-block create/update#2118
Neseria wants to merge 2 commits into
larksuite:mainfrom
Neseria:feat/dashboard-layout-precision-202607

Conversation

@Neseria

@Neseria Neseria commented Jul 30, 2026

Copy link
Copy Markdown
Contributor

Summary

Add optional dashboard-block layout and metric-card precision to lark-cli base +dashboard-block-create / +dashboard-block-update:

  • --position JSON {x,y,w,h} — parsed as JSON and passed through in the request body (advisory bounds; coordinates are not validated client- or server-side beyond shape, aligning with dws).
  • data_config.number_format {formatName, precision} for statistics blocks, with local validation (formatName ∈ 5 enums; precision integer 0–9 via toIntStrict/UseNumber). Validation runs symmetrically on create and update, and is skipped under --no-validate.

Both fields are optional and backward compatible.

Changes

  • shortcuts/base/dashboard_block_create.go / dashboard_block_update.go: add --position flag + Tips; update path now validates number_format symmetrically.
  • shortcuts/base/dashboard_ops.go: unified body assembly via buildDashboardBlockBody (DryRun/Execute isomorphic) carrying position.
  • shortcuts/base/helpers.go: number_format validation + toIntStrict.
  • skills/lark-base/references/lark-base-dashboard.md, dashboard-block-data-config.md, SKILL.md: SSOT sync (--position vs +dashboard-arrange; number_format field table/mapping/templates).
  • Tests: dry-run E2E (create+update body isomorphism), validation boundaries, --no-validate bypass, combined update.

Test Plan

  • go build ./shortcuts/base + go build -o lark-cli .
  • go test ./shortcuts/base (full package, no regressions)
  • go test ./tests/cli_e2e/base -run 'LayoutPrecision|DashboardBlock(Create|Update)DryRun' (E2E)
  • CI: ci.yml / semantic-review.yml / skill-format-check.yml / arch-audit.yml
  • Note: make build/make unit-test route through scripts/fetch_meta.py which failed locally with SSL cert verify (sandbox); verified via direct go build/go test.

Related Issues

Known limitations

  • Coordinate readback (get/list-block returning x/y/w/h) is out of scope this iteration (separate requirement).

改动范围: lark-cli base dashboard-block create/update 的 --position flag、number_format 校验、body 透传、lark-base skill SSOT 文档与 dry-run/单测。
思考过程: 依据已 tech-review 的技术方案,新增两个可选入参,复用 parseJSONObject/UseNumber,统一 body 组装保证 DryRun/Execute 同构,create/update 校验对称。
改动原因: 补齐 lark-cli 缺失的仪表盘组件布局与指标卡精度能力,对齐钉钉 dws 口径。
Break Change: 无(全部可选入参,向后兼容)

Co-authored-by: BASE Infra Harness base-infra-harness@bytedance.com

Summary by CodeRabbit

  • New Features

    • Added optional --position support for dashboard block creation and updates using 12-column grid coordinates.
    • Added statistics card number formatting with configurable formatName and precision.
    • Dry-run previews now reflect the same request payloads as execution.
  • Bug Fixes

    • Improved handling of malformed position and configuration JSON.
    • Added validation for unsupported formats and invalid precision values, with --no-validate bypass support.
  • Documentation

    • Updated dashboard guidance and examples for positioning, number formatting, and layout behavior.

wanglei75 and others added 2 commits July 30, 2026 17:02
…block create/update

Add an optional top-level --position flag ({x,y,w,h} JSON, parsed but not
coordinate-validated, passed through as a sibling of name/type/data_config) and
optional statistics data_config.number_format ({formatName,precision}) with
light enum + 0-9 integer validation. Both are backward compatible. Body
assembly is unified in a shared buildDashboardBlockBody helper so DryRun and
Execute stay isomorphic. Adds toIntStrict for strict precision parsing, focused
helper/execute/dry-run tests, an E2E dry-run test, and syncs the lark-base
dashboard + data-config skill references.

Co-authored-by: TRAE CLI <noreply@bytedance.com>
The dashboard-block-update command parsed data_config but never ran the
statistics number_format check, so an illegal formatName/precision slipped
through locally while create rejected it — violating the SSOT + backend-design
§4.5 promise of CLI-side interception on BOTH paths. Update has no --type flag
(block type is immutable) and intentionally skips strong type validation, so it
now reuses the shared validateNumberFormat sub-validator that
validateBlockDataConfig delegates to, keeping create/update symmetric without
demanding table_name/series on a number_format-only update.

Also: add tests for the --no-validate bypass on create+update, a combined
update carrying position + number_format + name, and extend the DryRun/Execute
body isomorphism assertion to the update path. Clarify the --position flag Desc
that coordinate bounds are advisory (not validated locally or server-side) and
sync the lark-base SKILL.md routing table for --position / number_format.

Co-authored-by: TRAE CLI <noreply@bytedance.com>
@CLAassistant

Copy link
Copy Markdown

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@github-actions github-actions Bot added domain/base PR touches the base domain size/M Single-domain feat or fix with limited business impact labels Jul 30, 2026
@coderabbitai

coderabbitai Bot commented Jul 30, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

Dashboard block create and update now support optional position payloads and statistics data_config.number_format validation. Shared request builders align dry-run and execution bodies, with tests and documentation covering validation, passthrough, bypass behavior, and E2E dry-run output.

Changes

Dashboard block precision controls

Layer / File(s) Summary
Number-format validation contract
shortcuts/base/helpers.go, shortcuts/base/base_dashboard_layout_precision_test.go
Adds strict integer parsing and validates statistics number_format.formatName and precision.
Shared dashboard payload construction
shortcuts/base/dashboard_ops.go, shortcuts/base/base_dryrun_ops_test.go
Centralizes create/update payload construction and aligns strict execution with best-effort dry-run behavior.
Create and update command wiring
shortcuts/base/dashboard_block_*.go, shortcuts/base/base_dashboard_layout_precision_test.go, tests/cli_e2e/base/*
Adds --position, validates JSON and number formats, forwards fields, and tests create/update execution and dry-run behavior.
Dashboard block documentation and coverage
skills/lark-base/SKILL.md, skills/lark-base/references/*, tests/cli_e2e/base/coverage.md
Documents position semantics, number-format rules, bypass behavior, and expanded coverage.

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

Sequence Diagram(s)

sequenceDiagram
  participant CLICommand
  participant PositionValidator
  participant NumberFormatValidator
  participant buildDashboardBlockBody
  participant DashboardBlockAPI
  CLICommand->>PositionValidator: parse and canonicalize --position
  CLICommand->>NumberFormatValidator: validate statistics number_format
  CLICommand->>buildDashboardBlockBody: build create or update body
  buildDashboardBlockBody->>DashboardBlockAPI: send dashboard block request
Loading

Possibly related PRs

  • larksuite/cli#341: Modifies the shared validateBlockDataConfig logic used by this PR.
  • larksuite/cli#386: Also changes shared dashboard block data-config validation.
  • larksuite/cli#388: Modifies the same validation entrypoint for dashboard block data-config.

Suggested labels: feature

Suggested reviewers: liangshuo-1

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 70.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
Title check ✅ Passed The title clearly and concisely summarizes the main change: adding position and statistics number_format support to dashboard-block create/update.
Description check ✅ Passed The description matches the template with Summary, Changes, Test Plan, and Related Issues, and it includes sufficient detail on scope and verification.
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.
✨ 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: 8

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

1251-1256: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low value

formatName is validated after trimming but forwarded untrimmed.

" digital" passes local validation via strings.TrimSpace(fn) yet the original padded string is what gets sent in data_config, so the server sees a value the local check normalized away. Either compare without trimming, or write the trimmed value back into the config during normalization.

🤖 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 1251 - 1256, The formatName
normalization in the number-format validation block must match the value
forwarded in data_config. Update the handling near validNumberFormatNames so
validation and downstream configuration use the same representation: either
validate the original string without trimming or write the trimmed string back
into nf before forwarding it.
🤖 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_layout_precision_test.go`:
- Around line 263-270: Update both error-path assertions around
BaseDashboardBlockCreate to validate typed metadata instead of relying only on
message substrings: use errs.ProblemOf(err) to assert the expected Category and
Subtype, and errors.As(err, &ve) to assert *errs.ValidationError with ve.Param
equal to "--data-config". Also verify the preserved underlying cause as required
by the existing error-test conventions while retaining the current rejection
checks.

In `@shortcuts/base/dashboard_block_update.go`:
- Around line 60-67: The update path around validateNumberFormat applies
validation to all block types, unlike validateBlockDataConfig during create,
which only validates statistics blocks. Document this intentional create/update
behavior difference in the relevant tips or reference documentation, including
the --no-validate escape hatch for update requests.

In `@shortcuts/base/dashboard_ops.go`:
- Around line 45-64: Update the data-config handling in the surrounding
execution method to trim the raw value before checking for emptiness, matching
the existing position handling and Validate behavior. Use the trimmed value for
parseJSONObject so whitespace-only data-config inputs are treated as absent and
non-empty values are parsed consistently.

In `@skills/lark-base/references/dashboard-block-data-config.md`:
- Around line 345-362: 修正文档中关于非 statistics 组件 number_format
的行为描述,明确区分创建与更新路径:更新时即使组件非 statistics,dashboard_block_update.go 中的
validateNumberFormat 仍会校验并拒绝格式非法的
number_format。若要保留“静默忽略且不报错”的文档表述,则需调整对应更新实现以传入组件类型并跳过非 statistics 校验。

In `@skills/lark-base/references/lark-base-dashboard.md`:
- Around line 29-30: Update the dashboard creation example’s --dashboard-id
value to use a dashboard-scoped placeholder such as dsh_xxx instead of the
block-like blk_xxx value, while leaving the other command arguments unchanged.

In `@tests/cli_e2e/base/base_dashboard_block_layout_precision_dryrun_test.go`:
- Around line 20-52: Extend
TestBaseDashboardBlockCreateDryRun_PositionAndNumberFormat with a live E2E
workflow, guarded by the repository’s standard credential-based skip: create the
dashboard block using position and number_format, update its position, then
delete it during cleanup. Assert each CLI operation succeeds and reuse the
created block identifier so the server validates the full create/update/delete
flow.
- Around line 105-107: Update the assertion in the dry-run test around the
Validate-stage failure to search the combined result.Stdout and result.Stderr
output for “formatName”, while preserving the non-zero ExitCode assertion.

In `@tests/cli_e2e/base/coverage.md`:
- Around line 5-6: Update the coverage headline in coverage.md to match the 26
checked command rows and the 78-command denominator: use Covered: 26 and
Coverage: 33.3%. If the denominator intentionally excludes two covered commands,
document those exclusions instead.

---

Nitpick comments:
In `@shortcuts/base/helpers.go`:
- Around line 1251-1256: The formatName normalization in the number-format
validation block must match the value forwarded in data_config. Update the
handling near validNumberFormatNames so validation and downstream configuration
use the same representation: either validate the original string without
trimming or write the trimmed string back into nf before forwarding it.
🪄 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 Plus

Run ID: 12b9a2f2-78db-45c2-a551-820e6fcaa864

📥 Commits

Reviewing files that changed from the base of the PR and between 87be09e and bb7d8fb.

📒 Files selected for processing (11)
  • shortcuts/base/base_dashboard_layout_precision_test.go
  • shortcuts/base/base_dryrun_ops_test.go
  • shortcuts/base/dashboard_block_create.go
  • shortcuts/base/dashboard_block_update.go
  • shortcuts/base/dashboard_ops.go
  • shortcuts/base/helpers.go
  • skills/lark-base/SKILL.md
  • skills/lark-base/references/dashboard-block-data-config.md
  • skills/lark-base/references/lark-base-dashboard.md
  • tests/cli_e2e/base/base_dashboard_block_layout_precision_dryrun_test.go
  • tests/cli_e2e/base/coverage.md

Comment on lines +263 to +270
err := runShortcut(t, BaseDashboardBlockCreate, args, factory, stdout)
if err == nil {
t.Fatalf("expected validation error for bad formatName")
}
if !strings.Contains(err.Error(), "formatName") || !strings.Contains(err.Error(), "data_config 校验失败") {
t.Fatalf("unexpected error: %v", err)
}
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win

Error-path assertions rely only on message substrings.

Both rejections should assert typed metadata: errs.ProblemOf(err) for Category/Subtype, and errors.As(err, &ve) on *errs.ValidationError to check ve.Param == "--data-config". Message-only checks won't catch a regression that changes the error classification.

As per coding guidelines: "Error-path tests must assert typed metadata through errs.ProblemOf (category, subtype, and param) and verify cause preservation rather than relying only on message substrings."

Note: per prior learnings in this repo, errs.ProblemOf does not expose Param; read it from *errs.ValidationError via errors.As.

Also applies to: 282-289

🤖 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_layout_precision_test.go` around lines 263 -
270, Update both error-path assertions around BaseDashboardBlockCreate to
validate typed metadata instead of relying only on message substrings: use
errs.ProblemOf(err) to assert the expected Category and Subtype, and
errors.As(err, &ve) to assert *errs.ValidationError with ve.Param equal to
"--data-config". Also verify the preserved underlying cause as required by the
existing error-test conventions while retaining the current rejection checks.

Sources: Coding guidelines, Learnings

Comment on lines +60 to +67
// update 时不做强类型校验(不传 type),让后端验证具体字段。
// 但 number_format 必须与 create 对称地本地拦截(backend-design §4.5):
// update 无 type flag,无法调用 validateBlockDataConfig 的完整分支
// (那会误报 table_name/series 缺失,破坏仅改 number_format 的用法),
// 因此直接复用其委托的 number_format 子校验,保持双路径一致。
if problems := validateNumberFormat(norm["number_format"]); len(problems) > 0 {
return formatDataConfigErrors(problems)
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Update validates number_format for every block type; create only for statistics.

validateBlockDataConfig gates the check on blockType == "statistics" (helpers.go Line 1231), so a non-statistics block accepts any number_format on create but is rejected on update. Since the block type isn't known here, this is a defensible tradeoff, but the divergence is user-visible — worth documenting in the tips/reference alongside the --no-validate escape hatch.

🤖 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_update.go` around lines 60 - 67, The update
path around validateNumberFormat applies validation to all block types, unlike
validateBlockDataConfig during create, which only validates statistics blocks.
Document this intentional create/update behavior difference in the relevant tips
or reference documentation, including the --no-validate escape hatch for update
requests.

Comment on lines +45 to +64
if raw := runtime.Str("data-config"); raw != "" {
parsed, err := parseJSONObject(pc, raw, "data-config")
if err != nil {
if strict {
return nil, err
}
} else {
body["data_config"] = parsed
}
}
if raw := strings.TrimSpace(runtime.Str("position")); raw != "" {
parsed, err := parseJSONObject(pc, raw, "position")
if err != nil {
if strict {
return nil, err
}
} else {
body["position"] = parsed
}
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Whitespace handling is asymmetric between data-config and position.

position is trimmed before the empty check, data-config is not. Validate treats a whitespace-only --data-config as absent (strings.TrimSpace(raw) == "" → return nil), but here the same value reaches parseJSONObject and fails the strict Execute path. Trim both for consistency.

♻️ Proposed fix
-	if raw := runtime.Str("data-config"); raw != "" {
+	if raw := strings.TrimSpace(runtime.Str("data-config")); raw != "" {
 		parsed, err := parseJSONObject(pc, raw, "data-config")
📝 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
if raw := runtime.Str("data-config"); raw != "" {
parsed, err := parseJSONObject(pc, raw, "data-config")
if err != nil {
if strict {
return nil, err
}
} else {
body["data_config"] = parsed
}
}
if raw := strings.TrimSpace(runtime.Str("position")); raw != "" {
parsed, err := parseJSONObject(pc, raw, "position")
if err != nil {
if strict {
return nil, err
}
} else {
body["position"] = parsed
}
}
if raw := strings.TrimSpace(runtime.Str("data-config")); raw != "" {
parsed, err := parseJSONObject(pc, raw, "data-config")
if err != nil {
if strict {
return nil, err
}
} else {
body["data_config"] = parsed
}
}
if raw := strings.TrimSpace(runtime.Str("position")); raw != "" {
parsed, err := parseJSONObject(pc, raw, "position")
if err != nil {
if strict {
return nil, err
}
} else {
body["position"] = parsed
}
}
🤖 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_ops.go` around lines 45 - 64, Update the data-config
handling in the surrounding execution method to trim the raw value before
checking for emptiness, matching the existing position handling and Validate
behavior. Use the trimmed value for parseJSONObject so whitespace-only
data-config inputs are treated as absent and non-empty values are parsed
consistently.

Comment on lines +345 to +362
仅 `type: statistics` 的指标卡支持在 `data_config` 里加可选 `number_format`,控制数值展示精度与格式;不传时后端回退默认千分位整数(`digital`)。其它组件类型携带 `number_format` 会被静默忽略、不生效、不报错。

| 字段 | 类型 | 必填 | 说明 |
|------|------|------|------|
| `number_format.formatName` | string | 否 | 数值格式名,取值见下表 |
| `number_format.precision` | integer | 否 | 小数位数,取值 `0` 到 `9` 的整数(本地严格校验,`2.5` 之类非整数会被拒) |

`formatName` 枚举与含义:

| formatName | 含义 | 示例(precision=2) |
|------------|------|--------------------|
| `digital` | 千分位数字(默认) | `1,234.56` |
| `digital_without_separator` | 无千分位数字 | `1234.56` |
| `percentage_rounded` | 百分比 | `1,234.56%` |
| `cyn_rounded` | 人民币金额 | `¥1,234.56` |
| `dollar_rounded` | 美元金额 | `$1,234.56` |

> 本地会校验 `formatName` 属于上述 5 个枚举、`precision` 为 0..9 整数;`--no-validate` 跳过本地校验,由后端裁决。

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Document update-path behavior for non-statistics blocks.

The statement that other component types’ number_format is silently ignored and never errors is not true for update: dashboard_block_update.go calls validateNumberFormat without a block type, so malformed number_format is rejected even on non-statistics updates. Clarify the create/update distinction or change the implementation to match the documented behavior.

🤖 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 345
- 362, 修正文档中关于非 statistics 组件 number_format 的行为描述,明确区分创建与更新路径:更新时即使组件非
statistics,dashboard_block_update.go 中的 validateNumberFormat 仍会校验并拒绝格式非法的
number_format。若要保留“静默忽略且不报错”的文档表述,则需调整对应更新实现以传入组件类型并跳过非 statistics 校验。

Comment on lines +29 to +30
```bash
lark-cli base +dashboard-block-create --base-token xxx --dashboard-id blk_xxx --name "总销售额" --type statistics --data-config '{"table_name":"订单表","count_all":true}' --position '{"x":0,"y":0,"w":6,"h":4}'

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Use a dashboard-shaped ID in the example.

--dashboard-id blk_xxx uses a block-like identifier and can cause copy-paste failures. Use dsh_xxx or another clearly dashboard-scoped placeholder.

🤖 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/lark-base-dashboard.md` around lines 29 - 30,
Update the dashboard creation example’s --dashboard-id value to use a
dashboard-scoped placeholder such as dsh_xxx instead of the block-like blk_xxx
value, while leaving the other command arguments unchanged.

Comment on lines +20 to +52
func TestBaseDashboardBlockCreateDryRun_PositionAndNumberFormat(t *testing.T) {
setBaseDryRunConfigEnv(t)

ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second)
t.Cleanup(cancel)

result, err := clie2e.RunCmd(ctx, clie2e.Request{
Args: []string{
"base", "+dashboard-block-create",
"--base-token", "app_x",
"--dashboard-id", "dsh_1",
"--name", "Revenue",
"--type", "statistics",
"--data-config", `{"table_name":"Orders","series":[{"field_name":"Amount","rollup":"SUM"}],"number_format":{"formatName":"dollar_rounded","precision":2}}`,
"--position", `{"x":0,"y":0,"w":6,"h":4}`,
"--dry-run",
},
BinaryPath: "../../../lark-cli",
DefaultAs: "bot",
})
require.NoError(t, err)
result.AssertExitCode(t, 0)

output := strings.TrimSpace(result.Stdout)
assert.Contains(t, output, "/open-apis/base/v3/bases/app_x/dashboards/dsh_1/blocks")
assert.Contains(t, output, `"method": "POST"`)
// top-level position sibling of name/type/data_config
assert.Contains(t, output, `"position"`)
assert.Contains(t, output, `"w": 6`)
// number_format nested inside data_config
assert.Contains(t, output, `"number_format"`)
assert.Contains(t, output, `"dollar_rounded"`)
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win

Only dry-run E2E coverage for the new --position / number_format flow.

As per path instructions for tests/cli_e2e/**/*.go: "New flows or behavior changes require live E2E coverage with a self-contained create/use/cleanup workflow and bot credentials where applicable." Consider adding a live create → update-position → delete workflow (guarded by the usual credential skip) so the server actually accepts the position and number_format payloads. Want me to draft it?

Also applies to: 56-82

🤖 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_layout_precision_dryrun_test.go`
around lines 20 - 52, Extend
TestBaseDashboardBlockCreateDryRun_PositionAndNumberFormat with a live E2E
workflow, guarded by the repository’s standard credential-based skip: create the
dashboard block using position and number_format, update its position, then
delete it during cleanup. Assert each CLI operation succeeds and reuse the
created block identifier so the server validates the full create/update/delete
flow.

Source: Coding guidelines

Comment on lines +105 to +107
require.NoError(t, err)
assert.NotEqual(t, 0, result.ExitCode)
assert.Contains(t, result.Stderr, "formatName")

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Validation rejections print the JSON error envelope to stdout, not stderr.

A Validate-stage failure exits non-zero and emits the structured error envelope on stdout, so asserting only result.Stderr will likely never match. Assert against the combined output.

💚 Proposed fix
-	assert.Contains(t, result.Stderr, "formatName")
+	assert.Contains(t, result.Stdout+result.Stderr, "formatName")

Based on learnings: in tests/cli_e2e dry-run tests, Validate-stage rejections exit non-zero and print a structured JSON error envelope to stdout; tests should verify the message via result.Stdout + result.Stderr.

📝 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
require.NoError(t, err)
assert.NotEqual(t, 0, result.ExitCode)
assert.Contains(t, result.Stderr, "formatName")
require.NoError(t, err)
assert.NotEqual(t, 0, result.ExitCode)
assert.Contains(t, result.Stdout+result.Stderr, "formatName")
🤖 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_layout_precision_dryrun_test.go`
around lines 105 - 107, Update the assertion in the dry-run test around the
Validate-stage failure to search the combined result.Stdout and result.Stderr
output for “formatName”, while preserving the non-zero ExitCode assertion.

Sources: Coding guidelines, Learnings

Comment on lines +5 to +6
- Covered: 24
- Coverage: 30.8%

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Reconcile the headline coverage metrics.

The command table contains 26 rows, not 24. If all listed commands count toward the 78-command denominator, the metrics should be Covered: 26 and Coverage: 33.3%; otherwise document which two covered commands are excluded.

🤖 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/coverage.md` around lines 5 - 6, Update the coverage
headline in coverage.md to match the 26 checked command rows and the 78-command
denominator: use Covered: 26 and Coverage: 33.3%. If the denominator
intentionally excludes two covered commands, document those exclusions instead.

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/M Single-domain feat or fix with limited business impact

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants