feat: decouple --lang preference from TUI display language#1132
Open
luozhixiong01 wants to merge 21 commits into
Open
feat: decouple --lang preference from TUI display language#1132luozhixiong01 wants to merge 21 commits into
luozhixiong01 wants to merge 21 commits into
Conversation
…non-zh/en message structs
…essage follows opts.Lang preference
Empty string (explicit), wrong case, typos, and removed language codes all exit with ExitValidation (code 2). Empty Lang on bypass-cobra test paths normalizes to default 'zh' to preserve test ergonomics while keeping the user-facing contract strict.
After the --lang refactor, the only callers of NormalizeLang were the picker pre-select (now hardcoded zh) and getBindMsg/getInitMsg default branches (now bilingual collapse). Strict validation at the flag-parse boundary makes silent normalization unnecessary.
The help text still said 'language for interactive prompts', which contradicts the refactor — --lang no longer controls TUI language. Updated to describe it as a downstream API/output preference that does not change this command's TUI language. (acceptance-reviewer finding)
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe PR separates persistent language preference (Lang) from per-invocation TUI display language (UILang), adds an i18n validation module, updates message templates and confirmation behavior, and expands tests for bind and init commands. ChangesLanguage Preference Separation in Config Commands
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
|
|
github-actions
Bot
added
the
size/L
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1132 +/- ##
==========================================
+ Coverage 67.85% 68.32% +0.47%
==========================================
Files 592 620 +28
Lines 55373 57461 +2088
==========================================
+ Hits 37574 39262 +1688
- Misses 14685 14959 +274
- Partials 3114 3240 +126 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@0794bc981ef7b362cf777d864192c0f455f7762e🧩 Skill updatenpx skills add larksuite/cli#feat/cliconfig-lang-multilang -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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cmd/config/init_messages_test.go (1)
50-67:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winInclude
LangPreferenceSetin the non-empty message field assertions.The new
initMsg.LangPreferenceSetfield isn’t validated byassertAllFieldsNonEmpty, so an empty confirmation template would pass tests unnoticed.Suggested patch
fields := map[string]string{ "SelectAction": msg.SelectAction, "CreateNewApp": msg.CreateNewApp, "ConfigExistingApp": msg.ConfigExistingApp, "Platform": msg.Platform, "SelectPlatform": msg.SelectPlatform, "Feishu": msg.Feishu, "ScanQRCode": msg.ScanQRCode, "ScanOrOpenLink": msg.ScanOrOpenLink, "WaitingForScan": msg.WaitingForScan, "OpenLinkNonTTY": msg.OpenLinkNonTTY, "WaitingForScanNonTTY": msg.WaitingForScanNonTTY, "DetectedLarkTenant": msg.DetectedLarkTenant, "AppCreated": msg.AppCreated, "ConfigSaved": msg.ConfigSaved, + "LangPreferenceSet": msg.LangPreferenceSet, }🤖 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 `@cmd/config/init_messages_test.go` around lines 50 - 67, The assertion helper assertAllFieldsNonEmpty is missing the new initMsg field LangPreferenceSet, so add "LangPreferenceSet": msg.LangPreferenceSet to the fields map inside assertAllFieldsNonEmpty to ensure the LangPreferenceSet template is validated as non-empty during tests; locate the fields map in assertAllFieldsNonEmpty and include the LangPreferenceSet entry alongside the other message keys.
🧹 Nitpick comments (2)
internal/i18n/lang_test.go (1)
8-28: ⚡ Quick winCover all declared valid language codes in the positive path.
At Line 13, the table only checks a subset of valid codes. Add an assertion that iterates
ValidLanguagesand verifies each returnstrueto prevent list/function drift.Proposed test hardening
func TestIsValidLang(t *testing.T) { tests := []struct { lang string expected bool }{ {"zh", true}, {"en", true}, {"ja", true}, {"ko", true}, {"invalid", false}, {"", false}, {"ZH", false}, // case sensitive } for _, tt := range tests { t.Run(tt.lang, func(t *testing.T) { if got := IsValidLang(tt.lang); got != tt.expected { t.Errorf("IsValidLang(%q) = %v, want %v", tt.lang, got, tt.expected) } }) } + for _, lang := range ValidLanguages { + if !IsValidLang(lang) { + t.Errorf("IsValidLang(%q) = false, want true", lang) + } + } }🤖 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 `@internal/i18n/lang_test.go` around lines 8 - 28, The test only checks a subset of valid language codes; update TestIsValidLang to also iterate over the ValidLanguages slice and assert IsValidLang(code) returns true for each entry so the test will catch any drift between ValidLanguages and IsValidLang. Locate TestIsValidLang and add a subtest or loop that goes through ValidLanguages (the symbol ValidLanguages) and calls IsValidLang for each value, failing the test if any valid code returns false.internal/core/config.go (1)
162-163: ⚡ Quick winClarify
CliConfig.Langas preference, not display language.At Line 162, “UI language” can be misread as TUI render language. This PR separates preference (
Lang) from display (UILang), so tightening this comment will reduce future misuse.Suggested wording update
- Lang string // UI language (zh, en, ja, ko, etc.) + Lang string // Persistent language preference (e.g. zh, en, ja, ko)🤖 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 `@internal/core/config.go` around lines 162 - 163, Update the comment for the CliConfig.Lang field to state that it is the user's language preference for content/translation (e.g., "preferred language for prompts, responses, and content") and not the terminal/UI render language; reference the separate UILang field (e.g., "use UILang for TUI rendering/localization") so future readers don't confuse CliConfig.Lang with display rendering. Ensure the comment mentions CliConfig.Lang and UILang by name so maintainers can find them easily.
🤖 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 `@cmd/config/config_test.go`:
- Around line 186-188: In TestConfigInitCmd_InvalidLang add isolation for config
state by calling t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir()) at the start
of the test (before clearAgentEnv and any test table iterations) so the test
uses a unique temporary config directory; update any related new table tests in
the same file to do the same to avoid cross-test interference.
In `@cmd/config/init.go`:
- Around line 338-344: The picker error handling in the init command currently
returns raw errors from promptLangSelection (only huh.ErrUserAborted is
wrapped), which can leak huh errors into RunE; replace the bare return err with
a structured output error (e.g., use output.Errorf or output.ErrWithHint) so all
non-abort failures are returned as output.* errors—keep the huh.ErrUserAborted
branch returning output.ErrBare(1) and for other errors wrap err using
output.Errorf("failed to select language: %v", err) or output.ErrWithHint as
appropriate; update the block around promptLangSelection to use these output.*
wrappers so RunE only receives structured output errors.
---
Outside diff comments:
In `@cmd/config/init_messages_test.go`:
- Around line 50-67: The assertion helper assertAllFieldsNonEmpty is missing the
new initMsg field LangPreferenceSet, so add "LangPreferenceSet":
msg.LangPreferenceSet to the fields map inside assertAllFieldsNonEmpty to ensure
the LangPreferenceSet template is validated as non-empty during tests; locate
the fields map in assertAllFieldsNonEmpty and include the LangPreferenceSet
entry alongside the other message keys.
---
Nitpick comments:
In `@internal/core/config.go`:
- Around line 162-163: Update the comment for the CliConfig.Lang field to state
that it is the user's language preference for content/translation (e.g.,
"preferred language for prompts, responses, and content") and not the
terminal/UI render language; reference the separate UILang field (e.g., "use
UILang for TUI rendering/localization") so future readers don't confuse
CliConfig.Lang with display rendering. Ensure the comment mentions
CliConfig.Lang and UILang by name so maintainers can find them easily.
In `@internal/i18n/lang_test.go`:
- Around line 8-28: The test only checks a subset of valid language codes;
update TestIsValidLang to also iterate over the ValidLanguages slice and assert
IsValidLang(code) returns true for each entry so the test will catch any drift
between ValidLanguages and IsValidLang. Locate TestIsValidLang and add a subtest
or loop that goes through ValidLanguages (the symbol ValidLanguages) and calls
IsValidLang for each value, failing the test if any valid code returns false.
🪄 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: e71fe396-b371-4aa4-9366-d7ba958f06aa
📒 Files selected for processing (10)
cmd/config/bind.gocmd/config/bind_messages.gocmd/config/bind_test.gocmd/config/config_test.gocmd/config/init.gocmd/config/init_messages.gocmd/config/init_messages_test.gointernal/core/config.gointernal/i18n/lang.gointernal/i18n/lang_test.go
Bare picker errors leaked through RunE, breaking structured stderr JSON for agent callers. Wrap non-abort huh failures in output.Errorf in both bind and init. Also isolate the InvalidLang test config dir, assert the new LangPreferenceSet field is non-empty, guard ValidLanguages drift, and clarify the CliConfig.Lang comment as preference (not TUI language). Addresses CodeRabbit review on PR #1132.
Add unit tests for the flag-mode language-preference confirmation line (bind + init), the printLangPreferenceConfirmation helper (both explicit and implicit branches), and validateInitLang's empty-implicit normalize path. Raises patch coverage on the new --lang preference code; the remaining uncovered lines are interactive picker/TUI paths.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
cmd/config/config_test.go (1)
460-468:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winIsolate config state in the newly added tests.
Please set
LARKSUITE_CLI_CONFIG_DIRto a temp dir at test start so these tests remain hermetic and cannot leak/read shared config state.Suggested patch
func TestValidateInitLang_NormalizesEmptyImplicitToZh(t *testing.T) { + t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir()) opts := &ConfigInitOptions{Lang: "", langExplicit: false} if err := validateInitLang(opts); err != nil { t.Fatalf("expected nil error, got %v", err) } @@ func TestPrintLangPreferenceConfirmation(t *testing.T) { + t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir()) t.Run("explicit prints confirmation", func(t *testing.T) { f, _, stderr, _ := cmdutil.TestFactory(t, nil) printLangPreferenceConfirmation(&ConfigInitOptions{Factory: f, Lang: "en", UILang: "zh", langExplicit: true})As per coding guidelines,
**/*_test.go:Use t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir()) to isolate config state in tests.Also applies to: 472-488
🤖 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 `@cmd/config/config_test.go` around lines 460 - 468, The tests modify/inspect config and must isolate config state: at the start of TestValidateInitLang_NormalizesEmptyImplicitToZh and the other test covering lines 472-488, call t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir()) so each test uses a fresh temp config directory; add this single line as the first statement in those test functions (referencing the test function names TestValidateInitLang_NormalizesEmptyImplicitToZh and the adjacent test) to ensure hermetic behavior.
🤖 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.
Duplicate comments:
In `@cmd/config/config_test.go`:
- Around line 460-468: The tests modify/inspect config and must isolate config
state: at the start of TestValidateInitLang_NormalizesEmptyImplicitToZh and the
other test covering lines 472-488, call t.Setenv("LARKSUITE_CLI_CONFIG_DIR",
t.TempDir()) so each test uses a fresh temp config directory; add this single
line as the first statement in those test functions (referencing the test
function names TestValidateInitLang_NormalizesEmptyImplicitToZh and the adjacent
test) to ensure hermetic behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 43a5d826-997b-4e01-9eea-2ca3f811b619
📒 Files selected for processing (2)
cmd/config/bind_test.gocmd/config/config_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- cmd/config/bind_test.go
Set LARKSUITE_CLI_CONFIG_DIR to a temp dir in the two newly added tests to follow the repo test-isolation convention (addresses CodeRabbit).
Collaborator
Author
Reviewer summaryThis PR decouples the What changed (behavior)
Key design point
Verification
Ready for review — the only gate left is approval. |
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
The
config init/config bind--langflag was overloaded: it both selected the TUI display language and set a persistent preference, sharing oneopts.Langfield. This produced a contradictory UX (the picker offered languages the TUI then fell back to Chinese for) and let unvalidated values reach disk. This PR splits the two concerns:--langbecomes a strictly-validated persistent preference, while the TUI is bilingual (zh/en) driven by a separate field. The supported preference set is aligned with the Feishu client UI (14 languages).Changes
UILangfield toBindOptions/ConfigInitOptions(cmd/config/bind.go,cmd/config/init.go); TUI rendering readsUILang(default zh, picker-only), preference writes readLang--langagainsti18n.ValidLanguagesinvalidateBindFlagsand a newvalidateInitLang— empty string, wrong case, typos, and removed codes all exit 2 viaoutput.ErrValidationgetBindMsg/getInitMsgto a bilingual switch and delete the 12 non-zh/en message structs (cmd/config/bind_messages.go,cmd/config/init_messages.go)promptLangSelectionto 2 options (中文 / English); the picker now writes bothLangandUILang--langis explicit (LangPreferenceSetmessage field)messagefield followsLang(preference) for AI-agent consumption, while stderr TUI text followsUILangi18n.NormalizeLang; trimValidLanguagesto 14 and update its count assertion--langhelp text to describe preference semantics instead of "interactive prompts"Test Plan
make unit-testpassed (per-package; the aggregate run is resource-constrained in the sandbox, every package passes in isolation —cmd/config,internal/i18n, plus 16/16 sequentialok)config init --lang fr ...→ exit 0,lang:"fr"on disk, zh confirmation语言偏好已设置：fr;--lang frr|ZH|""|ar→ exit 2 with structured validation error;config init --helplists exactly 14 codesRelated Issues
N/A
Summary by CodeRabbit
New Features
Bug Fixes / Validation
Tests