fix(apps): make db --environment optional, auto-select branch server-side#1735
Merged
Conversation
github-actions
Bot
added
the
size/M
|
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:
📝 WalkthroughWalkthroughDB shortcut commands now omit empty environment parameters by default, and recovery commands add env-aware preview/apply wiring plus revised diff aggregation. ChangesDB environment auto-selection
DB recovery preview and diff output
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
…side All db shortcuts defaulted --environment to "dev", which forced single-env apps (whose DB lives on the online branch, with no dev branch) to fail with "Invalid DB Branch: dev" unless the user explicitly passed --environment online. Change the default to empty: when --environment is omitted the CLI sends no env, letting the server pick the branch by the app's multi-env state (multi-env → dev, single-env → online), matching miaoda-cli's behavior of not carrying dbBranch when unset. Explicit --environment dev|online is unchanged; explicit dev on a single-env app still errors as expected. - 10 db shortcuts: dbEnvFlags default "dev" → "" (+db-execute, +db-table-list, +db-table-get, +db-quota-get, +db-data-export, +db-data-import, +db-changelog-list, +db-audit-list/-set/-status) - dry-run e2e assertions updated: default env is now unset, not "dev" - skill docs (lark-apps-db, lark-apps-db-execute) describe the auto-select
chenxingyang1019
force-pushed
the
fix/apps-db-env-optional
branch
from
9b1145c to
d752ab9
Compare
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@ee8d8fd941a36860639ef84c7dcb80dbc9884bd6🧩 Skill updatenpx skills add larksuite/cli#fix/apps-db-env-optional -y -g |
![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
🤖 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/apps/apps_db_audit_list.go`:
- Line 43: The DB audit/list command setup uses an empty default env via
dbEnvFlags, but the tests do not cover the no-`--environment` path. Update the
relevant tests in apps_db_audit_test and apps_db_changelog_list_test to add a
dry-run assertion for the default empty env case, alongside the existing env=dev
coverage, so the command behavior is verified when env is unset.
🪄 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: a345fb27-f13b-48c8-88da-31173d1768ce
📒 Files selected for processing (14)
shortcuts/apps/apps_db_audit_list.goshortcuts/apps/apps_db_audit_set.goshortcuts/apps/apps_db_audit_status.goshortcuts/apps/apps_db_changelog_list.goshortcuts/apps/apps_db_data_export.goshortcuts/apps/apps_db_data_import.goshortcuts/apps/apps_db_execute.goshortcuts/apps/apps_db_quota_get.goshortcuts/apps/apps_db_table_get.goshortcuts/apps/apps_db_table_list.goskills/lark-apps/references/lark-apps-db-execute.mdskills/lark-apps/references/lark-apps-db.mdtests/cli_e2e/apps/apps_db_execute_dryrun_test.gotests/cli_e2e/apps/apps_db_table_list_dryrun_test.go
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1735 +/- ##
==========================================
- Coverage 74.40% 74.39% -0.01%
==========================================
Files 853 857 +4
Lines 88311 89001 +690
==========================================
+ Hits 65704 66215 +511
- Misses 17544 17673 +129
- Partials 5063 5113 +50 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
…kill doc Address PR #1735 review: - omit-empty: when --environment is unset, drop the env query key entirely instead of sending env="" — matches the family's omit-empty convention (cf. page_token) and miaoda-cli's "no dbBranch when unset". Add dbEnvParams helper; apply across all db shortcuts (execute, table-list/-get, quota-get, changelog-list, audit-list/-set/-status, data-export/-import) plus the export/import query params, queryExportTotal and audit-list table/status probes. - e2e dry-run assertions pin env is omitted via .Exists() (was Equal ""). - skill doc (lark-apps-db): rewrite the --environment guidance from an agent's decision POV — read vs write, single-env writes hit online prod, explicit dev on single-env as a probe; drop redundant/changelog phrasing.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
shortcuts/apps/apps_db_changelog_list.go (1)
79-82: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winConfirm dry-run test coverage for the unset-env default.
Prior review noted
shortcuts/apps/apps_db_changelog_list_test.goonly exercisesenv=dev, not the no---environmentpath that this refactor now defaults to.🤖 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/apps/apps_db_changelog_list.go` around lines 79 - 82, The changelog parameter builder in buildChangelogParams now defaults unset environment handling, but the tests only cover the env=dev path. Add or update coverage in shortcuts/apps/apps_db_changelog_list_test.go for the no--environment case to verify the dry-run/default behavior matches the new dbEnvParams logic and the fallback environment is applied correctly.Source: Coding guidelines
shortcuts/apps/apps_db_audit_list.go (1)
217-231: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winConfirm dry-run test coverage for the unset-env default.
A prior review noted
shortcuts/apps/apps_db_audit_test.godoesn't assertenvat all for the no---environmentpath, and the same coding guideline ("Every behavior change needs a test alongside the change") applies here sincebuildAuditListParams/fetchExistingTables/fetchAuditEnabledTablesnow build params withoutenvby default.🤖 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/apps/apps_db_audit_list.go` around lines 217 - 231, Confirm the no-`--environment` dry-run path is covered by a test, since `buildAuditListParams`, `fetchExistingTables`, and `fetchAuditEnabledTables` now omit `env` by default. Update `shortcuts/apps/apps_db_audit_test.go` to assert the generated params for the unset-environment case, including that `env` is absent when the flag is not provided. Keep the test aligned with the existing dry-run coverage and verify the behavior through the same audit-list parameter builder flow.Source: Coding guidelines
🧹 Nitpick comments (1)
shortcuts/apps/db_common.go (1)
37-45: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueComment language inconsistency.
Logic is correct, but the doc comment (lines 37-39) is written in Chinese while the rest of the codebase (e.g., copyright headers, other shortcut files) uses English. Consider translating for consistency and to keep the codebase accessible to all contributors.
✏️ Suggested English comment
-// dbEnvParams 把 env 并入 params：仅当显式指定了环境（非空）才带 env 键；未指定（空）时 -// 省略该键，由服务端按应用多环境状态自动选分支（多环境→dev，单环境→online）。与家族对 -// 空可选参数的 omit-empty 约定一致——不发空串，wire 上真正不带 env。原样返回同一个 map 便于链式。 +// dbEnvParams merges env into params: only sets the "env" key when an +// environment is explicitly specified (non-empty). When unset, the key is +// omitted so the server can auto-select the branch based on the app's +// multi-environment state (multi-env -> dev, single-env -> online). This +// follows the family's omit-empty convention for optional params—no empty +// string is sent on the wire. Returns the same map for chaining. func dbEnvParams(rctx *common.RuntimeContext, params map[string]interface{}) map[string]interface{} {🤖 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/apps/db_common.go` around lines 37 - 45, The logic in dbEnvParams is fine, but its doc comment is written in Chinese while the surrounding codebase uses English. Translate the comment above dbEnvParams into concise English, keeping the same meaning about only setting env when dbEnv(rctx) is non-empty and otherwise omitting it for server-side default selection.
🤖 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/apps/apps_db_data_import.go`:
- Line 79: The dry-run test for the omitted --environment path only verifies the
derived table name and should also assert the default env behavior. Update
TestAppsDBDataImport_TableDefaultsToFileBasename to check the request shape
produced by dbEnvParams and Params, specifically that env is unset/defaulted in
the dry-run case when no environment is provided, so the default handling stays
covered.
In `@shortcuts/apps/apps_db_quota_get.go`:
- Around line 44-51: The +db-quota-get command currently builds its request
without an explicit dry-run assertion for the omitted env case. Update the
quota-get flow in Execute and the related tests so that when --environment is
not provided, dbEnvParams receives an empty map and the API request is verified
to omit env entirely. Add this coverage alongside the existing quota-get tests
in apps_db_env_recovery_quota_test.go, using appDbQuotaPath, dbEnvParams, and
rctx.CallAPITyped as the key symbols to locate the change.
---
Duplicate comments:
In `@shortcuts/apps/apps_db_audit_list.go`:
- Around line 217-231: Confirm the no-`--environment` dry-run path is covered by
a test, since `buildAuditListParams`, `fetchExistingTables`, and
`fetchAuditEnabledTables` now omit `env` by default. Update
`shortcuts/apps/apps_db_audit_test.go` to assert the generated params for the
unset-environment case, including that `env` is absent when the flag is not
provided. Keep the test aligned with the existing dry-run coverage and verify
the behavior through the same audit-list parameter builder flow.
In `@shortcuts/apps/apps_db_changelog_list.go`:
- Around line 79-82: The changelog parameter builder in buildChangelogParams now
defaults unset environment handling, but the tests only cover the env=dev path.
Add or update coverage in shortcuts/apps/apps_db_changelog_list_test.go for the
no--environment case to verify the dry-run/default behavior matches the new
dbEnvParams logic and the fallback environment is applied correctly.
---
Nitpick comments:
In `@shortcuts/apps/db_common.go`:
- Around line 37-45: The logic in dbEnvParams is fine, but its doc comment is
written in Chinese while the surrounding codebase uses English. Translate the
comment above dbEnvParams into concise English, keeping the same meaning about
only setting env when dbEnv(rctx) is non-empty and otherwise omitting it for
server-side default selection.
🪄 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: 6796977b-3223-4f0d-b3ea-2ea893b9245c
📒 Files selected for processing (14)
shortcuts/apps/apps_db_audit_list.goshortcuts/apps/apps_db_audit_set.goshortcuts/apps/apps_db_audit_status.goshortcuts/apps/apps_db_changelog_list.goshortcuts/apps/apps_db_data_export.goshortcuts/apps/apps_db_data_import.goshortcuts/apps/apps_db_execute.goshortcuts/apps/apps_db_quota_get.goshortcuts/apps/apps_db_table_get.goshortcuts/apps/apps_db_table_list.goshortcuts/apps/db_common.goskills/lark-apps/references/lark-apps-db.mdtests/cli_e2e/apps/apps_db_execute_dryrun_test.gotests/cli_e2e/apps/apps_db_table_list_dryrun_test.go
✅ Files skipped from review due to trivial changes (1)
- skills/lark-apps/references/lark-apps-db.md
🚧 Files skipped from review as they are similar to previous changes (3)
- tests/cli_e2e/apps/apps_db_table_list_dryrun_test.go
- shortcuts/apps/apps_db_audit_status.go
- shortcuts/apps/apps_db_audit_set.go
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
shortcuts/apps/apps_db_table_list.go (1)
289-299: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winGuard against non-finite strings ("NaN"/"Inf") in the new string branch.
strconv.ParseFloataccepts"NaN","Inf", and"Infinity"(case-insensitive) as valid floats with no error. If the server ever emits such a value for a row count,numericAsFloatreturns(NaN/Inf, true), and downstreamint64(n)conversions inhumanBytes/intStringare implementation-defined for values a float64→int64 cast can't represent, so display could show garbage instead of falling back to"—".🛡️ Proposed guard
case string: // 服务端有些数值字段（如 recovery diff 的 inserted/deleted 行数）以字符串下发。 s := strings.TrimSpace(v) if s == "" { return 0, false } f, err := strconv.ParseFloat(s, 64) if err != nil { return 0, false } + if math.IsNaN(f) || math.IsInf(f, 0) { + return 0, false + } return f, true🤖 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/apps/apps_db_table_list.go` around lines 289 - 299, The new string-handling branch in numericAsFloat should reject non-finite values before returning success. After trimming the input and before returning from the strconv.ParseFloat path, add a finiteness check for values like NaN, Inf, and Infinity so these cases return (0, false) instead of propagating an unusable float. Keep the fix localized to numericAsFloat and ensure downstream callers like humanBytes and intString only receive finite numbers.
🤖 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/apps/apps_db_table_list.go`:
- Around line 289-299: Add a test case in apps_db_table_list_test.go to cover
the new string-to-float conversion branch in the value parsing helper used by
apps_db_table_list.go. Extend the existing test around the string handling path
so it includes a numeric string such as "13.5" or " 13.5 " and asserts it is
parsed successfully via strconv.ParseFloat, while keeping the existing
unsupported string case like "x" intact.
---
Nitpick comments:
In `@shortcuts/apps/apps_db_table_list.go`:
- Around line 289-299: The new string-handling branch in numericAsFloat should
reject non-finite values before returning success. After trimming the input and
before returning from the strconv.ParseFloat path, add a finiteness check for
values like NaN, Inf, and Infinity so these cases return (0, false) instead of
propagating an unusable float. Keep the fix localized to numericAsFloat and
ensure downstream callers like humanBytes and intString only receive finite
numbers.
🪄 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: a56cd64a-19ce-4e60-8a52-c81e7fc480a7
📒 Files selected for processing (5)
shortcuts/apps/apps_db_env_migrate.goshortcuts/apps/apps_db_env_recovery_quota_test.goshortcuts/apps/apps_db_recovery.goshortcuts/apps/apps_db_table_list.goskills/lark-apps/references/lark-apps-db.md
✅ Files skipped from review due to trivial changes (1)
- skills/lark-apps/references/lark-apps-db.md
…ixes - +db-recovery-diff/-apply: add --environment (env → query param on submit and both status polls), aligned with the recovery env IDL - recovery diff: parse string row counts (inserted/deleted arrive as strings) so they render as "-N rows" instead of "no changes"; drop the redundant per-table data-row line when a schema action (drop/restore/alter) exists for the same table; count tables_affected by distinct tables - +db-env-migrate: run a dry_run preview before apply to backfill the change count when the server reports changes_applied=0 on a cold apply (matches miaoda-cli's diff-then-apply) - lark-apps-db.md: drop the redundant recovery clause (recovery follows the standard --environment rule)
chenxingyang1019
force-pushed
the
fix/apps-db-env-optional
branch
from
3d4f7ca to
50fb419
Compare
Address CodeRabbit review threads on PR #1735: - numericAsFloat: add numeric-string cases ("13.5", " 13.5 ", int, empty) - db-data-import: assert dry-run omits env when --environment unset (table still defaults to file basename) - db-quota-get: assert dry-run omits env when --environment unset
liangshuo-1
approved these changes
Jul 8, 2026
This was referenced Jul 8, 2026
Merged
Closed
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
2 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
All db shortcuts defaulted --environment to "dev", which forced single-env apps (whose DB lives on the online branch, with no dev branch) to fail with "Invalid DB Branch: dev" unless the user explicitly passed --environment online.
Change the default to empty: when --environment is omitted the CLI sends no env, letting the server pick the branch by the app's multi-env state (multi-env → dev, single-env → online), matching miaoda-cli's behavior of not carrying dbBranch when unset. Explicit --environment dev|online is unchanged; explicit dev on a single-env app still errors as expected.
Changes
Test Plan
go test ./shortcuts/apps/ ./tests/cli_e2e/apps/all green)lark-cli apps +db-*flow works as expectedcli_e2e.sh: 91 passed, 0 failed (incl. new "no --environment" dry-run + live cases)app_179b0qed55k(BOE): no env /online→ok=true; explicitdev→500002511as expectedRelated Issues
Summary by CodeRabbit
--env/--environmentis left unset, and omit theenvparameter entirely when not provided.--env.envis omitted when the flag is not provided, and refreshed migration polling mocks for preview+apply calls.