Skip to content

fix(tui): show effective cost currency context in config view#1902

Open
LING71671 wants to merge 1 commit into
Hmbown:mainfrom
LING71671:fix/config-effective-cost-currency
Open

fix(tui): show effective cost currency context in config view#1902
LING71671 wants to merge 1 commit into
Hmbown:mainfrom
LING71671:fix/config-effective-cost-currency

Conversation

@LING71671
Copy link
Copy Markdown
Contributor

@LING71671 LING71671 commented May 21, 2026

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 /config view's cost_currency row 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) for zh-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.

  • Adds effective_cost_currency: String to ConfigView, captured from app.cost_currency at view-creation time via cost_currency_config_value, and row_display_value returns the annotated string when the saved and effective currencies differ.
  • Extends row_matches_filter to use row_display_value so users can filter on the effective currency string, and adds the cost_currency => \"usd | cny\" entry to config_hint_for_key.
  • The comparison logic in row_display_value uses 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

Filename Overview
crates/tui/src/tui/views/mod.rs Adds effective_cost_currency field to ConfigView and row_display_value helper to show locale-overridden currency; the string comparison in row_display_value is case-sensitive and does not handle accepted aliases (yuan, rmb, dollar, $), producing a false "effective" annotation for any saved alias value

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:#333
Loading

Fix All in Codex Fix All in Claude Code Fix All in Cursor

Reviews (1): Last reviewed commit: "fix(tui): show effective cost currency i..." | Re-trigger Greptile

Greptile also left 1 inline comment on this PR.

Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread crates/tui/src/tui/views/mod.rs Outdated
section: ConfigSection::Display,
key: "cost_currency".to_string(),
value: settings.cost_currency.clone(),
value: cost_currency_config_value(app),
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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(),

@LING71671 LING71671 force-pushed the fix/config-effective-cost-currency branch from f9c744d to d394b1e Compare May 21, 2026 11:44
@LING71671 LING71671 changed the title fix(tui): show effective cost currency in config view fix(tui): show effective cost currency context in config view May 21, 2026
@LING71671
Copy link
Copy Markdown
Contributor Author

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 usd (effective cny) under zh-Hans. This preserves the SAVED-row convention while still making the locale-driven CNY display visible in native /config.

@Hmbown
Copy link
Copy Markdown
Owner

Hmbown commented May 27, 2026

Independent review (Devin):

Solid fix — the original gemini feedback was addressed well. The row_display_value approach correctly separates the saved value (used for edits) from the display string, so round-trip edits still persist what the user typed.

Currency scope: USD/CNY only. Rates are hardcoded in pricing.rs; no live exchange-rate fetch. The effective-currency override is purely locale-driven ("usd" + zh-Hans → Cny in app.rs:1696).

Bug: alias normalization mismatch. Saved values rmb, yuan, or ¥ are all valid and accepted by CostCurrency::from_setting, but row_display_value compares row.value != self.effective_cost_currency as raw strings. A user with cost_currency = "rmb" on zh-Hans would see rmb (effective cny) even though they already mean CNY — false alarm. Suggest normalizing row.value through CostCurrency::from_setting before the comparison.

Test gap: Only the ("usd", zh-Hans) → Cny path is exercised. No test for: aliases matching effective currency (should be silent), saved=cny matching effective=cny (should be silent), or non-zh-Hans locale.

v0.8.48 conflict (#2256 in flight): One conflict in the test imports — HEAD adds use std::fs at the same line where this PR adds use std::ffi::OsString. Trivial two-line merge; no logic conflict.

Filter behavior: row_display_value now feeds the filter path too (line 843), so typing cny in /config search will match the effective-currency annotation. That seems useful, just calling it out.

The core UX fix is right and worth landing once the alias comparison is tightened.

@LING71671 LING71671 force-pushed the fix/config-effective-cost-currency branch from d394b1e to df63a18 Compare May 27, 2026 13:43
@LING71671
Copy link
Copy Markdown
Contributor Author

Rebased this PR onto current upstream/main (head df63a18a) after the CodeWhale rename/mainline drift. The behavior is still scoped to native /config: the editable cost_currency row keeps the saved value, and only the displayed context/filter text includes the effective runtime currency when it differs, e.g. usd (effective cny) for zh-Hans.

Verification:

  • cargo fmt --all --check
  • git diff --check upstream/main...HEAD
  • cargo test -p codewhale-tui --bin codewhale-tui config_view_cost_currency_shows_saved_and_effective_runtime_currency --all-features
  • cargo test -p codewhale-tui --bin codewhale-tui config_view --all-features
  • cargo clippy -p codewhale-tui --all-targets --all-features (passes with existing schema_migration dead_code warnings plus current main warnings in commands/config.rs and runtime_log.rs)

Comment on lines +1126 to +1135
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()
}
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 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.

Fix in Codex Fix in Claude Code Fix in Cursor

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Native /config shows saved USD while zh-Hans UI displays CNY costs

2 participants