fix(tui): show effective cost currency context in config view#1902
fix(tui): show effective cost currency context in config view#1902LING71671 wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
Code Review
This pull request modifies the ConfigView to display the effective runtime cost_currency from the application state rather than the saved configuration value. It also adds a ConfigSettingsEnvGuard test utility and a test case to verify this behavior. The review feedback indicates that displaying runtime values in a ConfigScope::Saved row is misleading and breaks UI conventions, as it hides the actual persisted setting and may lead users to believe their changes weren't saved. It is suggested to revert this change to maintain consistency.
| section: ConfigSection::Display, | ||
| key: "cost_currency".to_string(), | ||
| value: settings.cost_currency.clone(), | ||
| value: cost_currency_config_value(app), |
There was a problem hiding this comment.
This change breaks the convention in ConfigView where rows with ConfigScope::Saved display values directly from settings, while ConfigScope::Session rows display runtime values from app.
By showing the effective runtime value (app.cost_currency) in a Saved row, the UI incorrectly implies that the displayed value is what is persisted in the configuration file. This is particularly confusing when locale-based overrides are active (e.g., zh-Hans forcing cny even if usd is saved). If a user attempts to edit this value to usd, the UI will continue to show cny after saving, making it appear as if the change failed or was not persisted.
Consider showing the actual saved value from settings here to maintain UI consistency and predictability for the 'Saved' scope.
value: settings.cost_currency.clone(),f9c744d to
d394b1e
Compare
|
Updated after Gemini's review: the cost_currency row now keeps the saved setting value as the editable value and only shows the effective runtime currency as display context when it differs, for example |
|
Independent review (Devin): Solid fix — the original gemini feedback was addressed well. The Currency scope: USD/CNY only. Rates are hardcoded in Bug: alias normalization mismatch. Saved values Test gap: Only the v0.8.48 conflict (#2256 in flight): One conflict in the test imports — HEAD adds Filter behavior: The core UX fix is right and worth landing once the alias comparison is tightened. |
d394b1e to
df63a18
Compare
|
Rebased this PR onto current Verification:
|
Closes #1901.\n\n## Summary\n- keep the native /config cost_currency row anchored to the saved setting value\n- show the effective runtime currency alongside the saved value when locale fallback changes the display currency, e.g. usd (effective cny) for zh-Hans\n- include the effective display value in config filtering and add a regression test for zh-Hans + saved usd\n\n## Verification\n- cargo fmt --all\n- cargo test -p deepseek-tui --bin deepseek-tui config_view_cost_currency_shows_saved_and_effective_runtime_currency\n- cargo test -p deepseek-tui --bin deepseek-tui config_view_filter_matches_group_and_rows\n- git diff --check\n- cargo build
Greptile Summary
This PR fixes the
/configview to correctly show the effective runtime currency alongside the saved setting value when a locale-based fallback causes a mismatch (e.g., savedusdbutzh-Hanslocale activatescny).effective_cost_currencysnapshot toConfigView(populated fromapp.cost_currencyat construction, always refreshed when the view is rebuilt after a save), and arow_display_valuehelper that compares canonicalizedCostCurrencyvariants viafrom_settingon both sides before showing any annotation — correctly treating all aliases (yuan,rmb,¥,$) as equivalent to their canonical currency.row_matches_filterto search the display value (so filtering on\"cny\"matches a saved\"usd\"row when zh-Hans locale is active), and adds acost_currencyhint string to the hints map.ConfigSettingsEnvGuardthat safely isolates env-var state across parallel test runs.Confidence Score: 5/5
Safe to merge — the change is narrowly scoped to display logic in the config view and does not touch any data persistence or runtime state paths.
The previous reviewer concern about alias comparison has been fully addressed: both the saved value and the effective value are now parsed through
CostCurrency::from_settingbefore comparison, so aliases likeyuanorrmbcorrectly match their canonical variant and produce no false annotation. Theeffective_cost_currencysnapshot is always fresh becauseConfigViewis rebuilt vianew_for_appimmediately after everyConfigUpdatedevent.No files require special attention.
Important Files Changed
Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A[ConfigView::new_for_app] --> B[snapshot effective_cost_currency from app.cost_currency] B --> C[ConfigView ready] C --> D{row_display_value called} D -->|key != cost_currency or scope != Saved| E[return row.value] D -->|key == cost_currency AND scope == Saved| F[from_setting on saved value returns Option] F --> G[from_setting on effective returns Option] G --> H{variants equal?} H -->|yes| E H -->|no| I[return annotated string] C --> J{ConfigUpdated persisted} J --> K[set_config_value updates app.cost_currency] K --> L[ConfigView rebuilt with fresh snapshot]Reviews (2): Last reviewed commit: "fix(config): normalize cost currency dis..." | Re-trigger Greptile