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 1 commit 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:
|
| fn row_display_value(&self, row: &ConfigRow) -> String { | ||
| if row.key == "cost_currency" | ||
| && row.scope == ConfigScope::Saved | ||
| && row.value != self.effective_cost_currency | ||
| { | ||
| format!("{} (effective {})", row.value, self.effective_cost_currency) | ||
| } else { | ||
| row.value.clone() | ||
| } | ||
| } |
There was a problem hiding this comment.
Alias comparison produces false "effective" annotation
row.value holds the raw setting string (e.g. "yuan", "rmb", "dollar", "$"), while effective_cost_currency is always the canonical "cny" or "usd" string from cost_currency_config_value. CostCurrency::from_setting accepts all those aliases ("yuan", "rmb", "dollar", "dollars", "$"), so a user who saved cost_currency = "yuan" would see yuan (effective cny) even though both values represent the same currency — no locale fallback has occurred. The guard should compare canonicalized forms, e.g. by mapping row.value through CostCurrency::from_setting and comparing the resulting enum variants instead of the raw strings.
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'scost_currencyrow so it shows the saved setting value as the primary label and appends an(effective <currency>)annotation when locale fallback silently changes the display currency at runtime (e.g.,usd (effective cny)forzh-Hans). It also threads the composite display value into config-row filtering and adds a regression test using an env-var guard + temp settings file.effective_cost_currency: StringtoConfigView, captured fromapp.cost_currencyat view-creation time viacost_currency_config_value, androw_display_valuereturns the annotated string when the saved and effective currencies differ.row_matches_filterto userow_display_valueso users can filter on the effective currency string, and adds thecost_currency => \"usd | cny\"entry toconfig_hint_for_key.row_display_valueuses raw string equality, which does not account for accepted aliases (yuan,rmb,dollar,$), resulting in a false "effective" annotation for users who saved the setting using a non-canonical alias.Confidence Score: 3/5
The change is narrow and focused, but the core comparison in row_display_value is logically incorrect for users who stored their setting using an accepted alias, which would show a misleading 'effective' label in the config UI.
The display value comparison uses raw string equality between the saved setting and a canonical form, so any user who saved cost_currency as 'yuan', 'rmb', 'dollar', or '$' will see a spurious '(effective cny/usd)' annotation even though no locale override has taken place. This is a clear logic defect in the newly added code path that affects the correctness of the feature being shipped.
crates/tui/src/tui/views/mod.rs — specifically row_display_value and its string comparison against effective_cost_currency
Important Files Changed
Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A[ConfigView::new_for_app] --> B[settings.cost_currency → row.value] A --> C[app.cost_currency → effective_cost_currency\ncost_currency_config_value] C --> D{Locale fallback?\ne.g. usd + zh-Hans} D -->|Yes| E[CostCurrency::Cny → 'cny'] D -->|No| F[CostCurrency::Usd → 'usd'] B --> G[row_display_value] E --> G F --> G G --> H{row.key == cost_currency\n&& scope == Saved\n&& row.value != effective?} H -->|true| I["'usd (effective cny)'"] H -->|false| J[row.value as-is] I --> K[Rendered in config view\n+ used in filter] J --> K style I fill:#f9f,stroke:#333 style H fill:#ffe,stroke:#333Reviews (1): Last reviewed commit: "fix(tui): show effective cost currency i..." | Re-trigger Greptile