tui: align typed config and schema with the live value spaces - #5843
Conversation
UiThemeValue::Custom now holds its full custom:<name> selector, the same single string /theme and the persisted theme setting use, so the typed /config document round-trips a custom theme without the sibling custom_theme_name field. No disk migration: that key was typed-UI only and was never persisted.
ConfigLabelLaunchScreen belonged to the retired launch_screen setting (load already accepts and drops it) and ConfigLabelSidebarWidth / ConfigLabelSidebarFocus belonged to sidebar load-only shims that were never schema keys and had no hints. Remove the MessageIds and every pack entry; all 15 packs stay in parity. No behavior change.
- work_surface_placement: the live default is bottom and Settings::set accepts top|bottom|left|right|off, but the schema offered top|left|right|off defaulting to left, and WorkSurfacePlacementValue only had Top|Left|Right — a persisted bottom round-tripped as top through the typed /config document and corrupted the setting on save. Schema gains bottom (default bottom); the typed enum gains Bottom and Off so every live value round-trips. - rail_panel: the schema offered tasks|agents|context|pinned while the dock cycles eight panels and Settings::set rejected five of them. Schema and set() now accept tasks, agents, background, files, notepad, context, git, price; pinned stays an accepted alias that folds into tasks like the load-time migration. - status_indicator: drop the retired whale choice from the schema; the whale|🐳|🐋 → cw load migration stays. - UiThemeValue gains claude and solarized-light (SELECTABLE_THEMES entries the typed document could not round-trip) and a test pins the typed value space to every selectable theme. - packs: widen the two value-enumeration hints, add the bottom placement copy, and drop the retired choice keys from all 15 packs. Test updates: rail_panel_persists_tasks_agents_context_and_pinned encoded the old four-panel set() and its pinned-verbatim persistence; it now covers all eight panels and the pinned→tasks fold.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_fbce5cf6-959e-4242-956e-14a863b484e2) |
|
Claude finished @Hmbown's task in 2m 9s —— View job Review: tui: align typed config and schema with the live value spaces
SummaryThis is a well-scoped rung-2 fix: it edits the existing
No correctness bugs found. No dead code introduced (the removed Minor, non-blocking observation: there's no test asserting that every Verification note: |
There was a problem hiding this comment.
🟡 Changes recommended
A few correctness/contract issues remain (notably an inaccurate rail_panel “Expected” error list and missing validation that UiThemeValue::Custom actually contains a custom:<name> selector).
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR aligns the TUI’s settings schema, typed /config document, runtime Settings::set validation/normalization, and shipped locale packs so they describe the same (live) value spaces—avoiding silent rewrites when saving typed config (notably rail panel values, work surface placement, and theme selectors).
Changes:
- Expands/aligns
rail_panelandwork_surface_placementaccepted values (incl.bottom/offplacement) and updates defaults/migrations (e.g.,pinnedfolding intotasks, retiringwhalestatus indicator in the editor). - Updates typed config UI schema/logic to represent themes as either a named built-in or a custom selector carried inside
UiThemeValue. - Removes orphaned localization keys and updates all shipped locale packs to reflect the updated settings/value lists.
File summaries
| File | Description |
|---|---|
| crates/tui/src/settings.rs | Updates Settings::set accepted rail panels, folds pinned into tasks, and adjusts related tests. |
| crates/tui/src/localization.rs | Removes orphaned message IDs and adds new placement-related IDs used by schema/UI. |
| crates/tui/src/config_ui.rs | Aligns typed /config theme + placement value spaces with runtime behavior; updates schema-related tests. |
| crates/config/src/settings_schema.rs | Updates schema enums/options and defaults (e.g., placement default to bottom, rail panel list, retire whale). |
| crates/tui/locales/zh-Hant.json | Removes orphaned keys; adds/updates strings for new placement/panel lists. |
| crates/tui/locales/zh-Hans.json | Removes orphaned keys; adds/updates strings for new placement/panel lists. |
| crates/tui/locales/vi.json | Removes orphaned keys; adds/updates strings for new placement/panel lists. |
| crates/tui/locales/uk.json | Removes orphaned keys; adds/updates strings for new placement/panel lists. |
| crates/tui/locales/ru.json | Removes orphaned keys; adds/updates strings for new placement/panel lists. |
| crates/tui/locales/pt-BR.json | Removes orphaned keys; adds/updates strings for new placement/panel lists. |
| crates/tui/locales/ko.json | Removes orphaned keys; adds/updates strings for new placement/panel lists. |
| crates/tui/locales/ja.json | Removes orphaned keys; adds/updates strings for new placement/panel lists. |
| crates/tui/locales/id.json | Removes orphaned keys; adds/updates strings for new placement/panel lists. |
| crates/tui/locales/hi.json | Removes orphaned keys; adds/updates strings for new placement/panel lists. |
| crates/tui/locales/fr.json | Removes orphaned keys; adds/updates strings for new placement/panel lists. |
| crates/tui/locales/es-419.json | Removes orphaned keys; adds/updates strings for new placement/panel lists. |
| crates/tui/locales/en.json | Removes orphaned keys; adds/updates strings for new placement/panel lists. |
| crates/tui/locales/de.json | Removes orphaned keys; adds/updates strings for new placement/panel lists. |
| crates/tui/locales/ca.json | Removes orphaned keys; adds/updates strings for new placement/panel lists. |
Review details
Suppressed comments (1)
crates/tui/src/settings.rs:3253
- This test asserts an "Expected" list that omits
pinned, butpinnedremains a valid input (alias oftasks). If the user-facing error message is corrected to includepinned, this assertion should be updated to match the accurate list.
assert!(
err.to_string()
.contains("tasks, agents, background, files, notepad, context, git, or price")
);
- Files reviewed: 19/19 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| fn theme_setting_for_document(doc: &ConfigUiDocument) -> Result<String> { | ||
| let setting = if doc.settings.theme == UiThemeValue::Custom { | ||
| let name = doc | ||
| .settings | ||
| .custom_theme_name | ||
| .as_deref() | ||
| .map(str::trim) | ||
| .filter(|name| !name.is_empty()) | ||
| .ok_or_else(|| anyhow::anyhow!("custom theme requires custom_theme_name"))?; | ||
| format!("{}{}", crate::palette::USER_THEME_PREFIX, name) | ||
| } else { | ||
| doc.settings.theme.as_setting().to_string() | ||
| }; | ||
| crate::palette::resolve_theme_setting(&setting, None) | ||
| crate::palette::resolve_theme_setting(&doc.settings.theme.as_setting(), None) | ||
| .map(|(normalized, _, _)| normalized) | ||
| .map_err(anyhow::Error::msg) | ||
| } |
| anyhow::bail!( | ||
| "Failed to update setting: invalid rail panel '{value}'. Expected: tasks, agents, context, or pinned." | ||
| "Failed to update setting: invalid rail panel '{value}'. Expected: tasks, agents, background, files, notepad, context, git, or price." | ||
| ); |
![codewhale-agent[bot]](https://avatars.githubusercontent.com/u/101357273?s=60&v=4){kind=small}
There was a problem hiding this comment.
Codewhale review
PR aligns the typed /config document, hand-written settings schema, Settings::set, and locale packs around the live placement, rail-panel, and theme value spaces. Theme and placement round-trip coverage is significantly improved, and rail-panel validation now matches the dock. Remaining concerns are mostly around missing regression coverage for status-indicator retirement and empty schema labels for the newly exposed rail panels.
Findings
- [WARNING] Status indicator alignment may still be incomplete and lacks a regression test (
crates/tui/src/config_ui.rs)
The diff removeswhalefromSTATUS_INDICATORand from locale/MessageId packs, but it does not modify the typedStatusIndicatorValue(which is not shown in this diff). If that type still exposes aWhalevariant and a persisted/set value can reachbuild_documentbefore migration, the typed document will still accept a value the schema no longer offers. The PR adds thorough tests for theme and placement but no equivalent test for status-indicator migration or schema/type equality. - [INFO] New rail panel schema entries use empty label and hint strings (
crates/config/src/settings_schema.rs:328)
background,files,notepad,git, andpriceare added asSettingOption::new(..., "", ""). If any schema consumer does not special-case empty label/hint keys, these options will render blank. The comment indicates rawRailPanel::titlefallback is intentional, but that fallback is not tested here. Consider adding explicit locale entries or an assertion that empty labels fall back to the raw panel title. - [INFO] Theme alignment test covers typed schema but not the hand-written settings schema (
crates/config/src/settings_schema.rs)
The newui_theme_value_covers_every_selectable_themetest verifiesUiThemeValueround-trips and serializes correctly, but there is no analogous check that the hand-writtenSETTINGS_SCHEMAtheme choices (if any are separate) includeclaude,solarized-light, and thecustom:<name>representation. If that list is generated dynamically this is fine, but a cheap parity test would prevent future theme additions from drifting between the two schemas.
Suggestions
crates/config/src/settings_schema.rs:328— Avoid empty label/hint strings for the newly exposed rail panels. Either add explicit locale/MessageId entries for background/files/notepad/git/price, or add a test that proves the empty-label fallback displays the rawRailPanel::titlevalue instead of blank text.
Assessment
The PR is well-structured and directly fixes the silent theme/placement corruption it targets. The new tests are valuable and no blocking correctness bug is apparent in the diff, but a few alignment edge cases—especially status-indicator migration and the empty-label schema entries—should be explicitly covered before merge.
Advisory review by Codewhale (codewhale review --pr 5843 --post, head 01ec884f45a699e9f97e510b6af44019339412a9). Line-specific findings are also posted as inline review comments; mechanical fixes arrive as committable suggestions you can apply from the Files tab. CODEOWNERS approval still governs merge.
| "ConfigChoiceDetailRailPinned", | ||
| ), | ||
| SettingOption::new("git", "", ""), | ||
| SettingOption::new("price", "", ""), |
There was a problem hiding this comment.
[INFO] New rail panel schema entries use empty label and hint strings
background, files, notepad, git, and price are added as SettingOption::new(..., "", ""). If any schema consumer does not special-case empty label/hint keys, these options will render blank. The comment indicates raw RailPanel::title fallback is intentional, but that fallback is not tested here. Consider adding explicit locale entries or an assertion that empty labels fall back to the raw panel title.
| SettingOption::new("git", "", ""), | ||
| SettingOption::new("price", "", ""), | ||
| ]; | ||
|
|
There was a problem hiding this comment.
Avoid empty label/hint strings for the newly exposed rail panels. Either add explicit locale/MessageId entries for background/files/notepad/git/price, or add a test that proves the empty-label fallback displays the raw RailPanel::title value instead of blank text.
…rip test Signed-off-by: Hunter Bown <hmbown@codewhale.net>
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_09b16edd-8ab0-4f1f-a1dc-6d39dcbc3f29) |
|
Claude finished @Hmbown's task in 2m 53s —— View job Claude finished the review
|
There was a problem hiding this comment.
Codewhale review
Aligns config schema, typed /config document, and locale packs with runtime settings; adds theme and work-surface round-trip coverage and rail panel normalization.
Findings
- [WARNING] New rail panel schema entries have empty display strings (
crates/config/src/settings_schema.rs:326)
RAIL_PANEL introduces background/files/notepad/git/price as SettingOption::new with empty title and detail IDs. If the settings UI renders the stored title/detail IDs directly, these five entries will show blank labels or descriptions rather than RailPanel::title. Confirm the UI has an empty-ID fallback or add MessageId/localized strings for parity with the other rail options. - [WARNING] Custom theme selector can bypass the old non-empty validation (
crates/tui/src/config_ui.rs:860)
theme_setting_for_document now passes UiThemeValue::Custom's inner String directly to resolve_theme_setting. The previous code trimmed custom_theme_name and returned a clear 'custom theme requires custom_theme_name' error. Because parse_document deserializes Custom(String) without calling UiThemeValue::from_setting, malformed values such as "" or whitespace can reach apply_document with a low-level palette error instead of a validation message. Consider normalizing/validating the selector in theme_setting_for_document or in a deserialization guard. - [INFO] rail_panel error text omits the still-supported
pinnedalias (crates/tui/src/settings.rs:1382)
Settings::set still acceptspinnedand folds it into tasks, but the invalid-value error only lists tasks, agents, background, files, notepad, context, git, and price. Users who try a nearby alias may not realizepinnedremains valid. - [INFO] Work-surface test may not actually load the persisted settings file (
crates/tui/src/config_ui.rs:1938)
work_surface_placement_round_trips_bottom_and_off_through_typed_document writes settings.toml and sets env guards, then constructs Config::default(). If Config::default() does not load from CODEWHALE_HOME, the test only exercises the default value and the temp file is dead. Confirm Config::default() performs a load, or explicitly load the settings path before build_document.
Assessment
The change is coherent and well-tested for theme and work-surface round-tripping. Remaining concerns are UX/schema consistency details and one test-clarity issue; none block merge if the rail label fallback and Config::default() load behavior are confirmed.
Advisory review by Codewhale (codewhale review --pr 5843 --post, head 9a29726a8576693e0a023297d2d6e226f7bb3551). Line-specific findings are also posted as inline review comments; mechanical fixes arrive as committable suggestions you can apply from the Files tab. CODEOWNERS approval still governs merge.
| "context", | ||
| "ConfigChoiceRailContext", | ||
| "ConfigChoiceDetailRailContext", | ||
| ), |
There was a problem hiding this comment.
[WARNING] New rail panel schema entries have empty display strings
RAIL_PANEL introduces background/files/notepad/git/price as SettingOption::new with empty title and detail IDs. If the settings UI renders the stored title/detail IDs directly, these five entries will show blank labels or descriptions rather than RailPanel::title. Confirm the UI has an empty-ID fallback or add MessageId/localized strings for parity with the other rail options.
| doc.settings.theme.as_setting().to_string() | ||
| }; | ||
| crate::palette::resolve_theme_setting(&setting, None) | ||
| crate::palette::resolve_theme_setting(&doc.settings.theme.as_setting(), None) |
There was a problem hiding this comment.
[WARNING] Custom theme selector can bypass the old non-empty validation
theme_setting_for_document now passes UiThemeValue::Custom's inner String directly to resolve_theme_setting. The previous code trimmed custom_theme_name and returned a clear 'custom theme requires custom_theme_name' error. Because parse_document deserializes Custom(String) without calling UiThemeValue::from_setting, malformed values such as "" or whitespace can reach apply_document with a low-level palette error instead of a validation message. Consider normalizing/validating the selector in theme_setting_for_document or in a deserialization guard.
| | "files" | ||
| | "notepad" | ||
| | "context" | ||
| | "git" |
There was a problem hiding this comment.
[INFO] rail_panel error text omits the still-supported pinned alias
Settings::set still accepts pinned and folds it into tasks, but the invalid-value error only lists tasks, agents, background, files, notepad, context, git, and price. Users who try a nearby alias may not realize pinned remains valid.
| let settings_path = codewhale_home.join("settings.toml"); | ||
| fs::write(&settings_path, "work_surface_placement = \"bottom\"\n").expect("settings"); | ||
| let _home = EnvVarGuard::set("CODEWHALE_HOME", &codewhale_home); | ||
| let _codewhale_config = EnvVarGuard::remove("CODEWHALE_CONFIG_PATH"); |
There was a problem hiding this comment.
[INFO] Work-surface test may not actually load the persisted settings file
work_surface_placement_round_trips_bottom_and_off_through_typed_document writes settings.toml and sets env guards, then constructs Config::default(). If Config::default() does not load from CODEWHALE_HOME, the test only exercises the default value and the temp file is dead. Confirm Config::default() performs a load, or explicitly load the settings path before build_document.
3 commits: typed theme carries custom themes, orphaned locale keys dropped, typed config/schema aligned. Gates: fmt clean, dead-code PASS at 425. Full matrix via CI.
No-Issue: config-truth cleanup with no single tracking issue.
Note
Low Risk
Mostly config schema, localization, and typed-document parity; changes align defaults and enum values with existing runtime normalization, not new security or data paths.
Overview
Aligns the settings schema, typed
/configdocument, and locale packs with what the TUI already accepts at runtime so saves and the settings UI do not rewrite user choices.Work surface & rail: Default
work_surface_placementis now bottom (schema and typed enum). The typedWorkSurfacePlacementValueadds bottom and off so persistedbottomno longer degrades to top on round-trip.rail_panelin the schema andSettings::setcovers the full dock set (background, files, notepad, git, price); pinned is removed from the editor but still accepted and normalized to tasks. Retired whale status indicator is dropped from schema/locales (load migration only).Themes:
UiThemeValue::Customcarries the fullcustom:<name>selector insidetheme, removing the separatecustom_theme_namefield. claude and solarized-light are added to the typed theme enum with tests that everySELECTABLE_THEMESentry round-trips.Locales / MessageIds: Hints and choices updated for bottom placement and expanded rail panels; stale keys removed (launch screen, legacy sidebar width/focus, whale, pinned rail labels).
Reviewed by Cursor Bugbot for commit 9a29726. Bugbot is set up for automated code reviews on this repo. Configure here.