fix(config): propagate Lang across credential boundary; respect CurrentApp in priorLang#1157
Conversation
|
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 (5)
📝 WalkthroughWalkthroughThis PR refines language preference handling in the CLI: fixes the ChangesLanguage Preference Type System
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
…ntApp in priorLang Two issues caught in review of #1132 that the existing tests missed because they constructed RuntimeContext/CliConfig directly, bypassing the credential edge where the bug lives. P1 — Lang dropped at credential boundary credential.Account had no Lang field, so AccountFromCliConfig and ToCliConfig silently dropped cfg.Lang. The production Factory builds CliConfig via acct.ToCliConfig() (factory_default.go Phase 3), which meant RuntimeContext.Lang() always returned "" in production and shortcuts/mail/mail_signature.go always fell back to zh_cn — defeating the whole point of persisting --lang. Fix: add Lang i18n.Lang to Account and copy it in both directions. Regression test: TestFullChain_LangSurvivesProductionPath walks the real path (SaveMultiAppConfig -> DefaultAccountProvider.ResolveAccount -> ToCliConfig) and asserts Lang survives, so any future field added to CliConfig forces the same audit. P2 — priorLang ignored CurrentApp in multi-profile workspaces priorLang scanned all Apps and returned the first non-empty Lang. If a user had multiple profiles and the active one disagreed with Apps[0], a re-bind without --lang would silently inherit the wrong profile's preference. Fix: read multi.CurrentAppConfig("").Lang instead. Regression tests cover CurrentApp wins over Apps[0], single-app fallback, and malformed bytes. Change-Id: If7a276605f84f398cec329c2c942b471b4c32749
21316de to
0df7855
Compare
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@0df7855e11e482c0864b7c7780ea3b230ebb7f9b🧩 Skill updatenpx skills add larksuite/cli#fix/cliconfig-lang-credential-boundary -y -g |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1157 +/- ##
=======================================
Coverage 68.74% 68.74%
=======================================
Files 627 627
Lines 58579 58580 +1
=======================================
+ Hits 40269 40273 +4
+ Misses 15018 15016 -2
+ Partials 3292 3291 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Actionable comments posted: 0 |
Follow-up to #1132 (now merged as
3b77055). Addresses two issues caught in cross-boundary review that the existing tests missed because they constructedRuntimeContext/CliConfigdirectly, bypassing the credential edge where the bugs live.P1 —
Langdropped at credential boundary (blocker)credential.Accounthad noLangfield, soAccountFromCliConfig/ToCliConfigsilently droppedcfg.Lang. The production Factory buildsCliConfigviaacct.ToCliConfig()(factory_default.goPhase 3):The shortcut tests passed because they built
&RuntimeContext{Config: &CliConfig{Lang: ...}}directly — bypassing the edge where the bug sits.Fix: add
Lang i18n.LangtoAccount; copy in bothAccountFromCliConfigandToCliConfig.Regression test:
TestFullChain_LangSurvivesProductionPathwalks the real production pathSaveMultiAppConfig -> DefaultAccountProvider.ResolveAccount -> ToCliConfigand assertsLangsurvives — forces future field additions to undergo the same audit.P2 —
priorLangignoredCurrentAppin multi-profile workspacescmd/config/bind.go:387scannedmulti.Appsand returned the first non-emptyLang. If a workspace had multiple named profiles (fromprofile add) and the active one was notApps[0], a re-bind without--langsilently inherited the wrong profile's preference.Fix: read
multi.CurrentAppConfig("").Langinstead.Regression tests:
TestPriorLang_RespectsCurrentApp—CurrentAppwins overApps[0]TestPriorLang_FallsBackToFirstAppWhenCurrentUnset— bind-written single-app shape still worksTestPriorLang_MalformedReturnsEmpty— unparseable bytes branchTest plan
go build ./...go veton impacted packagesgo testoninternal/credential,internal/cmdutil,internal/i18n,internal/core,cmd/config,cmd/auth,cmd/profile,shortcuts/common,shortcuts/mail— all greenTestFullChain_LangSurvivesProductionPath, 3×TestPriorLang_*) — all greenSummary by CodeRabbit
Bug Fixes
Tests