fix(tui): provider color overrides across ranks + CLI / sessions / app.rs cleanup#440
Merged
fix(tui): provider color overrides across ranks + CLI / sessions / app.rs cleanup#440
Conversation
…ade map PR #436 introduced cost-ranked shade palettes but shipped four issues: 1. [HIGH] Provider color override silently disabled for rank ≥ 1. build_model_shade_map called get_provider_shade() which never consulted TokscaleConfig::get_provider_color(). A user setting [colors.providers] anthropic = "#FF0000" only saw red on their top-cost Anthropic model; ranks 1-6 kept the hardcoded coral family. Fix: check config override inside get_provider_shade; when present, derive a 7-step lighten-to-white palette from the override base via shade_from_base(). Rank 0 is the override color exactly; later ranks interpolate toward white with factors that match the end-of-ramp lightness (≈0.67) of the hardcoded palettes. 2. [MEDIUM] Shade collisions under GroupBy::WorkspaceModel. Same model appearing in N workspaces was counted N times during rank assignment, inflating ranks and making insert() last-write-win on the shared model name key. Fix: aggregate cost per (provider, model_name) before ranking; NaN costs are coerced to 0 for safety. 3. [MEDIUM] Non-deterministic ordering on cost ties. total_cmp + HashMap iteration produced unstable shade assignments when two models had equal cost (common during early data or zero- cost models). Fix: secondary sort by model name, so ties always resolve identically across refreshes. 4. [LOW] CURSOR_SHADES had 1 entry (vs 7 for others) and the unknown- provider fallback collapsed all models to pure white. Fix: generate 7-step Cursor purple ramp; add UNKNOWN_SHADES neutral gray ramp so unknown-provider models stay visually distinguishable. Also simplifies get_model_color() now that override handling lives in get_provider_shade() — removes the duplicate config lookup. Tests cover: rank-0-to-highest-cost, workspace dedupe, cost-tie determinism, NaN cost handling, cross-provider independence, rebuild on update_data, plus shade_from_base edge cases and unknown/cursor palette regressions. Constraint: hardcoded palettes are hand-picked from colorhexa and must not change for default providers Constraint: TokscaleConfig::load() returns &'static via OnceLock, so calling it per-rank is free Rejected: regenerate all palettes algorithmically from a single base color | would change default visual output for existing users Rejected: parameterize get_provider_shade() with &TokscaleConfig | call sites span UI render paths; OnceLock cache makes it unnecessary Confidence: high Scope-risk: narrow Directive: when adding a new provider palette, also add a case to get_provider_from_model — get_provider_shade defaults to UNKNOWN_SHADES for unmatched providers Not-tested: live TokscaleConfig override round-trip (would require touching ~/.tokscale); relies on unit coverage of shade_from_base + integration via get_provider_shade
main.rs had 19 client-filter bools + 6 date-range options redeclared
across 7 command structs — roughly 145 duplicated clap #[arg(...)] lines
plus 19-field struct-literal rebuilds at every call site. The existing
`ClientFlags` helper already named those fields, so this turns it into
a #[derive(Args)] struct and adds a parallel `DateRangeFlags` for the
date block, then #[command(flatten)]s both into each command that
needs them.
- `Cli` root, `Models`, `Monthly`, `Hourly`, `Graph`, `Tui`, `Submit`
now flatten both structs.
- `Wrapped` flattens only `ClientFlags` as `client_flags` (its own
`--year`/`--clients` flags have different semantics).
- Match-arm destructures collapse from ~25 field names per arm to
`{ clients, date, ... }`, and the 19-field `ClientFlags { ... }`
rebuilds at call sites are gone — `build_client_filter(clients)`
takes the flattened struct directly.
- `build_client_filter` / `build_date_filter` / `normalize_year_filter`
signatures unchanged; no runtime behavior change.
- 19 existing tests that construct `ClientFlags` literally keep working
because the public fields are identical.
UX: Submit's 13 flags previously read "Include only X data" while the
other six read "Show only X usage"; after flatten all 19 read "Show
only X usage" on every command. That's an intentional consistency fix
rather than a regression.
Net: main.rs 5093 → 4445 lines (-648).
Constraint: CLI surface must remain identical — all `--<client>` / `--today` / `--since` etc. still parse on the same subcommands
Rejected: a single `FilterFlags` carrying both clients and dates | Wrapped has no date block, so separate structs avoid forcing an awkward Option<DateRangeFlags>
Rejected: keep `#[arg(help = ...)]` variations per command | serves no user need; unified help matches the actual single code path
Confidence: high
Scope-risk: narrow
Directive: when adding a new client, edit `ClientFlags` + `build_client_filter` in main.rs AND `ClientId` in tokscale-core — the struct mirrors `ClientId` on purpose
Not-tested: every command's --help text rendered by clap (spot-checked `models`, `submit`, root); relies on existing CLI integration suite (83 passing)
11 parsers repeated the same two I/O boilerplate patterns:
- Connection::open_with_flags(path, READ_ONLY | NO_MUTEX)
- match fs::read(path) { Ok(c) => c, Err(_) => return Vec::new() }
Consolidated both into `sessions/utils.rs` as:
- `open_readonly_sqlite(&Path) -> Option<Connection>`
- `read_file_or_none(&Path) -> Option<Vec<u8>>`
Touched: amp, claudecode, crush, droid, gemini, kilo, mux, openclaw,
opencode, roocode, synthetic. No behavior change — helpers wrap the
same underlying calls and return None on the same errors the caller
previously matched into `Vec::new()`/early-return.
An earlier assessment rejected the broader `SessionParser` trait
refactor — parser signatures diverge too much (Codex's incremental
state, Claude's parent-subagent cache, Kilo's fallback timestamp,
Cursor's CSV schema auto-detect) for a trait to be anything but a
shoehorn. This helper extraction is the portion that was actually
shared.
Opencode.rs gets `#[cfg(test)] use rusqlite::Connection;` since
production code no longer needs the type directly but tests still
construct DBs for fixtures.
Constraint: tokscale-core already depends on rusqlite — no new dep added
Rejected: full SessionParser trait | parsers share <10% of their body; dispatch in lib.rs is per-client by design
Rejected: helper that also derives session_id from path stem | amp/openclaw/codex use three subtly different fallbacks; unify later
Confidence: high
Scope-risk: narrow
Directive: when adding a new SQLite-backed parser, call open_readonly_sqlite to preserve the READ_ONLY|NO_MUTEX invariant rather than opening the DB with write flags
Not-tested: failure paths on read-only volumes; the helpers pass through rusqlite/std errors identically so existing parser tests cover the happy path
app.rs was ~2247 lines, half tests half 32-field TUI state machine. An architect review rejected a full App-struct split (handle_key_event touches 17 fields — the coupling is essential to a TUI reducer) but approved moving two genuinely-pure computations out: - `build_model_shade_map(&[ModelUsage]) -> HashMap<String, Color>` moves to `tui/colors.rs`. Doesn't need any App state beyond `data.models`. - `build_export_json(&UsageData) -> Result<String>` moves to `tui/export.rs`. File I/O and the status-message side effect stay on `App` where they belong. `App::build_model_shade_map` and `App::export_to_json` keep the same signatures as thin call-site wrappers so nothing external shifts. The existing shade-map unit tests still go through `App::build_model_shade_map` and are unchanged — though they could now target the pure function directly if a future pass wants to drop the `App` setup. Net: app.rs 2247 -> 2174 (-73). Not huge, but the two functions that moved are the ones that actually wanted to be pure — the state-machine methods that remain are where they belong. Constraint: don't alter CLI/TUI behavior — only relocate pure logic Rejected: full split into event.rs / sort.rs / dialog.rs | handle_key_event touches 17 fields; splitting just moves the tangle through more indirection Rejected: move tests to sibling file via `#[path]` | cosmetic, doesn't improve anything Confidence: high Scope-risk: narrow Directive: when adding new pure computations on UsageData, consider tui/colors.rs or tui/export.rs before expanding App's impl block Not-tested: JSON output byte-equivalence (serde_json::to_string_pretty is used identically); relies on downstream tooling that reads the export to flag regressions
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
junhoyeo
changed the title
Store TUI shade entries by provider and model so duplicate model names stop overwriting each other across providers. Constraint: Existing non-provider-specific views must keep their model-name fallback behavior Rejected: Key the map by model name only | collapses ClientProviderModel rows with the same normalized model Confidence: high Scope-risk: narrow
Carry provider, display name, and color lookup metadata through aggregated and cached hourly data so provider-specific colors survive reloads and derived views. Constraint: Cached data must remain backward-compatible when older entries are missing the new fields Rejected: Recompute provider metadata only at render time | hourly and cached views do not preserve enough source context for a reliable rebuild Confidence: high Scope-risk: moderate
Update the model, overview, and stats screens to use provider-aware color lookups so duplicate model IDs render consistently across TUI surfaces. Constraint: Model and workspace views still need graceful fallback when provider metadata is merged or unavailable Rejected: Leave legacy model-only lookups in place for some screens | would keep the bug visible in overview and stats despite the underlying data fix Confidence: high Scope-risk: narrow
![devin-ai-integration[bot]](https://avatars.github.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
![cubic-dev-ai[bot]](https://avatars.github.com/in/1082092?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Treat empty provider IDs the same way as merged provider lists so shade-map construction stays consistent with lookup normalization. Constraint: Missing provider metadata must still preserve ranked color lookup for inferred providers Rejected: Leave empty providers as-is | creates build-vs-lookup key mismatches and forces rank-0 fallback colors Confidence: high Scope-risk: narrow
Use the hourly model map key as the fallback source for legacy cache entries so display names and color keys survive deserialization when older cache files omit the new metadata. Constraint: Cache loading must remain backward-compatible across legacy schema variants Rejected: Fallback from display_name to color_key only | leaves both fields empty for older hourly cache entries Confidence: high Scope-risk: narrow
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
1 participant
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
Follow-ups to #436 (cost-ranked shade palettes) plus adjacent cleanups surfaced during review.
PR #436 bug fixes (
fix(tui))[colors.providers]override was silently disabled for rank ≥ 1.get_provider_shade()now consultsTokscaleConfig::get_provider_color()at every rank and derives a 7-step lighten-to-white palette (shade_from_base) from the override base when present. A user settinganthropic = \"#FF0000\"now sees red-family shades across all their Anthropic models.GroupBy::WorkspaceModel. Same model appearing in N workspaces used to consume N rank slots and then collide on insert. Now aggregated to(provider, model_name) -> total_costbefore ranking.CURSOR_SHADESexpanded from 1 → 7 entries; unknown-provider fallback replaced by a gray ramp (was collapsing every unknown-provider model to pure white).13 new tests cover: rank-0-to-highest-cost, workspace dedupe, cost-tie determinism, NaN-cost handling, cross-provider independence, rebuild-on-update, plus
shade_from_baseedge cases and unknown/cursor palette regressions.CLI refactor (
refactor(cli))19 client-filter bools + 6 date-range options were redeclared across 7 clap structs — ~145 duplicated
#[arg(...)]lines plus 19-fieldClientFlags { ... }rebuilds at every call site. The existingClientFlagsruntime helper becomes a#[derive(Args)]struct; a parallelDateRangeFlagscaptures the date block. Both are#[command(flatten)]ed intoCli,Models,Monthly,Hourly,Graph,Tui, andSubmit.main.rs: 5093 → 4445 (-648 lines, -12.7%). No CLI surface change — all
--opencode,--claude,--today,--since, etc. still parse identically. One intentional UX win: Submit's inconsistent "Include only X data" help text now matches the uniform "Show only X usage" used elsewhere.Sessions + TUI cleanup (
refactor(sessions),refactor(tui))An architect review rejected broader refactors (
SessionParsertrait, full app.rs split) as over-engineering, but flagged two narrow wins that landed:open_readonly_sqlite+read_file_or_nonehelpers insessions/utils.rs. No behavior change — same read-only SQLite flags, same error-to-empty fallback.build_model_shade_mapandbuild_export_jsonmoved out ofAppintotui/colors.rsandtui/export.rsas pure functions. app.rs 2247 → 2174.Test plan
cargo clippy --workspace --all-features --all-targets -- -D warnings— cleancargo fmt --all -- --check— cleancargo test --workspace --all-features— 950 passing (345 + 83 + 519 + 3), no regressionstokscale models --helpspot-check: all client/date flags preservedtokscale submit --help: unified "Show only X usage" help text~/.tokscaleoverride foranthropic(would confirm HIGH bug fix end-to-end)Summary by cubic
Fixes TUI shade palette bugs, makes model colors provider-aware across all views and reloads, and removes duplicated CLI/session code. No CLI surface changes; TUI colors now respect provider overrides, handle empty providers, and keep labels/colors when loading legacy caches.
Bug Fixes
[colors.providers]overrides now apply at every rank via a 7-step palette (rank 0 = base).provider/displayName/colorKey(backward-compatible) so colors persist across reloads.displayName/colorKey, preserving labels and colors.cursorpalette expanded to 7 shades; unknown providers use a neutral gray ramp. Tests cover ranking, dedupe, ties, NaN handling, provider separation, rebuilds, palette overrides, and legacy cache fallback.Refactors
ClientFlagsandDateRangeFlagsasclapArgs, flattened intoCli,Models,Monthly,Hourly,Graph,Tui,Submit, andWrapped. ~648 lines removed; flags unchanged; help text unified to “Show only X usage”.@tokscale-core): Addedopen_readonly_sqliteandread_file_or_none; applied across 11 parsers. No behavior change.@tokscale-cli): Moved shade-map builder totui/colors.rsand export JSON builder totui/export.rsas pure functions.Written for commit 4b2e2a3. Summary will update on new commits.