tui: collapse ocean_treatment into ThemeId::Underwater - #5858
Conversation
`/theme underwater` validated through normalize_theme_name, which has no underwater entry — the alias only existed on the ocean_treatment field, so the command the founder reached for said "invalid theme". Underwater is a compound choice (Dark palette + deepsea treatment); route the aliases underwater|deepsea|deep-sea|ombre through set_theme_selection, the same setter the picker's Deepsea row uses. Test: theme_command_underwater_alias_applies_the_deepsea_pair. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
… has Esc back
Enter on the empty launch composer ran the pre-highlighted "New worktree"
entry, which the founder hit by reflex ("toxic"). The menu now starts
with nothing selected (menu_selected: Option<usize>); ↑/↓ highlight, Enter
runs only a highlighted entry, Esc unhighlights.
Every launch flow now walks back to the card: LaunchState::restore_card()
is applied when the resume picker or changelog pager closes over the
launch screen, on Esc from the empty composer once the card has dissolved,
and Resume with no saved sessions no longer dissolves the card just to
show a status line. The worktree prompt copy says "Esc back".
Suspecting the New worktree entry of doing nothing, prove it end to end:
new_worktree_creates_a_checkout_and_the_session_starts_inside_it drives
provision_launch_worktree against a scratch repo and checks the checkout,
the branch, duplicate refusal, and that begin_launch_session points the
session at the worktree. It did work — silently. It now leaves a receipt
in the transcript and status line (LaunchWorktreeCreated, 15 packs).
Startup goldens re-blessed: the only symbol change is the removed ▸ on
the first menu row; the ink goldens relabel because one palette entry
(SELECTION_TEXT BOLD) is no longer on screen.
Tests: launch/tideline slice 157 passed; event_loop/session_state/
localization/theme_picker slice 91 passed.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Signed-off-by: CodeWhale Bot <bot@codewhale.net>
Signed-off-by: CodeWhale Bot <bot@codewhale.net>
Alias deepsea; picker single list; config read-only migration folds deepsea into theme underwater and drops the key; OceanRamp keys on theme. Signed-off-by: CodeWhale Bot <bot@codewhale.net>
Signed-off-by: CodeWhale Bot <bot@codewhale.net>
Signed-off-by: CodeWhale Bot <bot@codewhale.net>
…p orphans) Signed-off-by: CodeWhale Bot <bot@codewhale.net>
Signed-off-by: CodeWhale Bot <bot@codewhale.net>
Signed-off-by: CodeWhale Bot <bot@codewhale.net>
Signed-off-by: CodeWhale Bot <bot@codewhale.net>
|
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_56c857ee-4083-4006-b34e-b602a062627b) |
|
Claude finished @Hmbown's task in 3m 30s —— View job Review: tui: collapse ocean_treatment into ThemeId::Underwater
The Scope concern — this PR bundles several unrelated changes not described in the PR body's 11-commit summary:
None of items 1–4 are individually broken, but mixing them into a PR titled/scoped as a theme collapse makes the diff harder to review and, for item 1, ships an undocumented brand-name change that the repo's own contract says needs an explicit migration. Recommend splitting those out or explicitly calling them out/reverting the casing change. Minor: No correctness bugs, panics, or provider/model-routing concerns found in the core collapse logic itself. |
There was a problem hiding this comment.
🟡 Changes recommended
The theme picker still captures its “original” state from ThemeId (not the normalized theme setting string), which can overwrite custom:<name> theme selections on Esc/Enter.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR removes the separate ocean_treatment setting and makes the painted ocean background a first-class theme (ThemeId::Underwater), while also aligning launch UX behavior/copy and adding “abyss depth” context-percent plumbing into the underwater field rendering.
Changes:
- Collapse ocean treatment into the
underwatertheme (including settings/config migration + picker/list updates). - Plumb
context_percentinto ocean field rendering and adjust related UI chrome/tests/goldens. - Refine launch card interaction (no pre-selected entry; Esc step-back behavior) and add a worktree-created receipt.
File summaries
| File | Description |
|---|---|
| crates/tui/src/tui/widgets/mod.rs | Switch underwater atmosphere gating to ThemeId::Underwater; pass context_percent into OceanColumn. |
| crates/tui/src/tui/widgets/header.rs | Normalize header wordmark to lowercase codewhale + test updates. |
| crates/tui/src/tui/whales.rs | Lowercase “Plain” species name to codewhale. |
| crates/tui/src/tui/views/tideline_tests.rs | Update theme list expectations for added Underwater row. |
| crates/tui/src/tui/views/mod.rs | Remove ocean_treatment from ThemeSelectionUpdated event + config rows. |
| crates/tui/src/tui/underwater/tideline_tests.rs | Update startup goldens/assertions for lowercase branding. |
| crates/tui/src/tui/underwater.rs | Launch menu selection becomes optional; Esc/Enter behavior refined; branding string lowercase. |
| crates/tui/src/tui/ui/tests.rs | Update fixtures to use Underwater theme instead of Deepsea treatment. |
| crates/tui/src/tui/ui/session_state.rs | Return ProvisionedWorktree; add “created worktree” receipt + tests. |
| crates/tui/src/tui/ui/overlays.rs | Open theme picker without treatment; add restore-launch-card helper. |
| crates/tui/src/tui/ui/handlers.rs | Theme picker updates route through set_config_value(theme, …) only. |
| crates/tui/src/tui/ui/frame/one_owner_tests.rs | Adjust context reading assertions to match new “silent <50%” rule. |
| crates/tui/src/tui/ui/frame.rs | Hide context percent below 50%; add cache hit% info segment. |
| crates/tui/src/tui/ui/event_loop.rs | Underwater redraw gating uses theme; launch-card restoration + new worktree begin flow. |
| crates/tui/src/tui/ui/apply.rs | Open theme picker without treatment. |
| crates/tui/src/tui/theme_picker/tideline_tests.rs | Update selectable theme count to 14. |
| crates/tui/src/tui/theme_picker.rs | Remove treatment rows; make Underwater an ordinary theme row; preserve “unrepresentable” selectors on Enter-without-nav. |
| crates/tui/src/tui/ocean/tests.rs | Update ramp/column tests for Underwater-only field + context-depth effect. |
| crates/tui/src/tui/ocean.rs | Remove OceanTreatment; make field Underwater-only; add context-depth gradient support. |
| crates/tui/src/tui/notifications.rs | Lowercase title animation base/resting title. |
| crates/tui/src/tui/notification_payload.rs | Lowercase fallback headline. |
| crates/tui/src/tui/infoline.rs | Add InfoSegmentId::Cache and priority. |
| crates/tui/src/tui/goldens/startup_surfacing_80x24.txt | Golden update for lowercase wordmark + no preselected menu marker. |
| crates/tui/src/tui/goldens/startup_ink_80x24.txt | Golden ink remap changes reflecting new launch/menu visuals. |
| crates/tui/src/tui/goldens/startup_ink_160x40.txt | Golden ink remap changes reflecting new launch/menu visuals. |
| crates/tui/src/tui/goldens/startup_ink_120x32.txt | Golden ink remap changes reflecting new launch/menu visuals. |
| crates/tui/src/tui/goldens/startup_ink_100x30.txt | Golden ink remap changes reflecting new launch/menu visuals. |
| crates/tui/src/tui/goldens/startup_first_run_80x24.txt | Golden update for lowercase wordmark + no preselected menu marker. |
| crates/tui/src/tui/goldens/startup_80x24.txt | Golden update for lowercase wordmark + no preselected menu marker. |
| crates/tui/src/tui/goldens/startup_40x10.txt | Golden update for lowercase wordmark + no preselected menu marker. |
| crates/tui/src/tui/goldens/startup_160x40.txt | Golden update for lowercase wordmark + no preselected menu marker. |
| crates/tui/src/tui/goldens/startup_120x32.txt | Golden update for lowercase wordmark + no preselected menu marker. |
| crates/tui/src/tui/goldens/startup_100x30.txt | Golden update for lowercase wordmark + no preselected menu marker. |
| crates/tui/src/tui/goldens/settings_80x24.txt | Golden update for Underwater row and renamed “Blue Stage” selection. |
| crates/tui/src/tui/goldens/settings_160x40.txt | Golden update for Underwater row and theme list reshuffle. |
| crates/tui/src/tui/goldens/settings_120x32.txt | Golden update for Underwater row and theme list reshuffle. |
| crates/tui/src/tui/goldens/settings_100x30.txt | Golden update for Underwater row and theme list reshuffle. |
| crates/tui/src/tui/goldens/config_panel_80x24.txt | Golden update removing ocean_treatment row and adjusting counts. |
| crates/tui/src/tui/goldens/config_panel_120x32.txt | Golden update removing ocean_treatment row and adjusting counts. |
| crates/tui/src/tui/app/init.rs | Stop parsing/storing ocean_treatment in App init. |
| crates/tui/src/tui/app.rs | Make launch menu selection optional; remove ocean_treatment from App. |
| crates/tui/src/tui/ambient_life.rs | Convert several animations to continuous-time calculations + drift/glow tweaks. |
| crates/tui/src/settings.rs | Remove ocean_treatment field; migrate legacy deepsea → theme="underwater" on load. |
| crates/tui/src/palette/themes.rs | Add ThemeId::Underwater + UNDERWATER_UI_THEME; normalize aliases to underwater. |
| crates/tui/src/localization.rs | Remove ocean-treatment message IDs; add LaunchWorktreeCreated message ID. |
| crates/tui/src/core/engine/turn_loop.rs | Start title animation with lowercase base. |
| crates/tui/src/config_ui.rs | Remove ocean treatment from config UI document/schema application. |
| crates/tui/src/commands/mod.rs | Remove set_theme_selection command entry point. |
| crates/tui/src/commands/groups/core/core.rs | Update dashboard title assertions to lowercase codewhale. |
| crates/tui/src/commands/groups/config/mod.rs | Update /theme usage string to include underwater. |
| crates/tui/src/commands/groups/config/config.rs | Remove compound theme+treatment setter; add underwater alias coverage tests; ensure Underwater keeps field under background override. |
| crates/tui/locales/en.json | Remove ocean-treatment strings; lowercase dashboard title; add worktree-created copy and Esc hint. |
| crates/tui/locales/de.json | Same: remove treatment strings; lowercase dashboard title; add worktree-created copy and Esc hint. |
| crates/tui/locales/fr.json | Same: remove treatment strings; lowercase dashboard title; add worktree-created copy and Esc hint. |
| crates/tui/locales/es-419.json | Same: remove treatment strings; lowercase dashboard title; add worktree-created copy and Esc hint. |
| crates/tui/locales/ca.json | Same: remove treatment strings; lowercase dashboard title; add worktree-created copy and Esc hint. |
| crates/tui/locales/pt-BR.json | Same: remove treatment strings; lowercase dashboard title; add worktree-created copy and Esc hint. |
| crates/tui/locales/ru.json | Same: remove treatment strings; lowercase dashboard title; add worktree-created copy and Esc hint. |
| crates/tui/locales/uk.json | Same: remove treatment strings; lowercase dashboard title; add worktree-created copy and Esc hint. |
| crates/tui/locales/vi.json | Same: remove treatment strings; lowercase dashboard title; add worktree-created copy and Esc hint. |
| crates/tui/locales/zh-Hans.json | Same: remove treatment strings; lowercase dashboard title; add worktree-created copy and Esc hint. |
| crates/tui/locales/zh-Hant.json | Same: remove treatment strings; lowercase dashboard title; add worktree-created copy and Esc hint. |
| crates/tui/locales/ja.json | Same: remove treatment strings; lowercase dashboard title; add worktree-created copy and Esc hint. |
| crates/tui/locales/ko.json | Same: remove treatment strings; lowercase dashboard title; add worktree-created copy and Esc hint. |
| crates/tui/locales/id.json | Same: remove treatment strings; lowercase dashboard title; add worktree-created copy and Esc hint. |
| crates/tui/locales/hi.json | Same: remove treatment strings; lowercase dashboard title; add worktree-created copy and Esc hint. |
| crates/lane/src/lib.rs | Re-export ProvisionedWorktree. |
| crates/config/src/settings_schema.rs | Remove ocean_treatment from settings schema. |
Review details
Suppressed comments (1)
crates/tui/src/tui/ambient_life.rs:482
risenis computed with floating-point +.round(), which can produceJELLY_VISIT_ROWSat the end of the visit window (and also makes row dwell timing non-uniform). Since movement is ultimately in discrete rows, compute the row index with integer division (and clamp) to keeprisenin0..JELLY_VISIT_ROWS-1.
let rise_period = JELLY_RISE_ROW_MS.saturating_add((j as u128) * JELLY_RISE_ROW_STAGGER_MS);
let cycle_duration = rise_period.saturating_mul(JELLY_VISIT_CYCLE_SLOTS);
let cycle_pos = t.saturating_add(phase) % cycle_duration;
let visit_duration = rise_period.saturating_mul(u128::from(JELLY_VISIT_ROWS));
if cycle_pos >= visit_duration {
continue; // still down in the dark between visits
}
let visit_progress = cycle_pos as f64 / visit_duration as f64;
let risen = (visit_progress * f64::from(JELLY_VISIT_ROWS)).round() as u16;
- Files reviewed: 68/70 changed files
- Comments generated: 3
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| ); | ||
| let cycle_index = school_clock / cycle_ms; | ||
| let cycle_frac = (school_clock % cycle_ms) as f64 / cycle_ms as f64; | ||
| let cycle_step = (cycle_frac * travel as f64).round() as i32; |
| // Capture the active theme name straight from `app` so | ||
| // Esc can revert through the same ConfigUpdated channel. | ||
| // Avoids re-reading settings.toml from disk on every | ||
| // `/theme` invocation. | ||
| let original = app.theme_id.name().to_string(); | ||
| app.view_stack.push_boxed( | ||
| crate::tui::theme_picker::ThemePickerView::boxed_with_treatment( | ||
| app.view_stack | ||
| .push_boxed(crate::tui::theme_picker::ThemePickerView::boxed( | ||
| original, |
| let original = app.theme_id.name().to_string(); | ||
| app.view_stack.push_boxed( | ||
| crate::tui::theme_picker::ThemePickerView::boxed_with_treatment( | ||
| app.view_stack | ||
| .push_boxed(crate::tui::theme_picker::ThemePickerView::boxed( |
![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 folds the separate ocean_treatment setting into a first-class underwater theme, updates locales and goldens, reworks ocean ramp animation, and adds context_percent plumbing. The change is broad but cohesive; primary risks are the config migration overwriting custom theme choices and the new ocean ramp ignoring background_color overrides.
Findings
- [WARNING] background_color override is ignored by the underwater ocean ramp (
crates/tui/src/tui/ocean.rs:359)
OceanRamp::for_theme now returns hardcoded original underwater colors whenever theme.name == "underwater", without deriving them from theme.surface_bg. The new test underwater_custom_background_keeps_the_field only asserts that a ramp exists, not that it reflects the custom background. This means users who set background_color while using the underwater theme will still see the default deep-blue ocean, not their chosen color, contradicting the comment in for_theme and the expectation set by underwater_theme_keeps_its_field_under_a_background_override. - [WARNING] Legacy ocean_treatment migration clobbers any existing theme unconditionally (
crates/tui/src/settings.rs:917)
In Settings migration, if a persisted document contains a deepsea-flavored ocean_treatment, s.theme is set to "underwater" without checking whether theme was a custom (custom:) or otherwise user-selected value. Users who had paired deepsea with a non-default theme (as called out in the PR risk note) will silently lose that theme on first load. - [INFO] Redundant "deepsea" literal in ThemeId::from match arm is unreachable (
crates/tui/src/palette/themes.rs:839)
normalize_theme_name already maps "deepsea" to "underwater" before ThemeId::from matches, so the "deepsea" pattern in the arm "underwater" | "deepsea" can never match. The alias is harmless but adds a dead branch and may confuse future readers. - [INFO] .round() in jellyfish/bubble rise can produce one extra row beyond previous bounds (
crates/tui/src/tui/ambient_life.rs:482)
In ambient_life.rs, risen = (visit_progress * f64::from(JELLY_VISIT_ROWS)).round() as u16 can evaluate to JELLY_VISIT_ROWS when visit_progress is close to 1, whereas the previous integer slot logic clamped to JELLY_VISIT_ROWS - 1. The same rounding pattern is used for bubble rise. This may cause ambient elements to appear one row higher than intended. - [INFO] Cache and Ttft info segments share the same shed_priority (
crates/tui/src/tui/infoline.rs:88)
The new Cache segment is given shed_priority 8, identical to Ttft. If both segments are present and space is constrained, the order in which they are dropped may depend on insertion order or sort stability. Consider assigning distinct priorities or documenting the intended precedence.
Suggestions
-
crates/tui/src/tui/ocean.rs:359— Derive the ramp colors from the effective theme surface (theme.surface_bg) so background_color overrides are respected, or explicitly disable background_color for the underwater theme. This requires reworking the hardcoded colors into a derived ramp similar to the pre-collapse generic path. -
crates/tui/src/settings.rs:917— Preserve the existing theme when migrating if it is a custom or non-default choice, e.g. only set s.theme to "underwater" when the persisted theme is one of the built-in themes that previously supported deepsea, or migrate to a custom theme that carries the underwater field. -
crates/tui/src/palette/themes.rs:839— Remove the redundant "deepsea" alternative from the match arm because normalize_theme_name already canonically produces "underwater"."underwater" => Some(Self::Underwater), -
crates/tui/src/tui/ambient_life.rs:482— Use .floor() or clamp the result to JELLY_VISIT_ROWS - 1 to keep the rise animation within its historical bounds. Similarly for bubble rise. -
crates/tui/src/tui/infoline.rs:88— Assign a unique shed_priority to Cache or Ttft so drop precedence is deterministic when both segments are present.
Assessment
The PR is well-structured with comprehensive tests and golden alignment, but the background_color override regression and the migration's unconditional theme overwrite should be addressed before merging because they directly affect user-visible appearance and configuration persistence.
Advisory review by Codewhale (codewhale review --pr 5858 --post, head 2b54b22609d185eacd6308c4cc181db8559a11d4). 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.
| { | ||
| // The painted field exists only under the underwater theme; every | ||
| // other theme leaves the terminal's ground alone. A user-supplied | ||
| // `background_color` rewrites the underwater surfaces through |
There was a problem hiding this comment.
[WARNING] background_color override is ignored by the underwater ocean ramp
OceanRamp::for_theme now returns hardcoded original underwater colors whenever theme.name == "underwater", without deriving them from theme.surface_bg. The new test underwater_custom_background_keeps_the_field only asserts that a ramp exists, not that it reflects the custom background. This means users who set background_color while using the underwater theme will still see the default deep-blue ocean, not their chosen color, contradicting the comment in for_theme and the expectation set by underwater_theme_keeps_its_field_under_a_background_override.
| .and_then(toml::Value::as_table) | ||
| .and_then(|table| table.get("ocean_treatment")) | ||
| .and_then(toml::Value::as_str) | ||
| .filter(|treatment| { |
There was a problem hiding this comment.
[WARNING] Legacy ocean_treatment migration clobbers any existing theme unconditionally
In Settings migration, if a persisted document contains a deepsea-flavored ocean_treatment, s.theme is set to "underwater" without checking whether theme was a custom (custom:) or otherwise user-selected value. Users who had paired deepsea with a non-default theme (as called out in the PR risk note) will silently lose that theme on first load.
| match normalize_theme_name(value)? { | ||
| "system" => Some(Self::System), | ||
| "terminal" => Some(Self::Terminal), | ||
| "underwater" | "deepsea" => Some(Self::Underwater), |
There was a problem hiding this comment.
[INFO] Redundant "deepsea" literal in ThemeId::from match arm is unreachable
normalize_theme_name already maps "deepsea" to "underwater" before ThemeId::from matches, so the "deepsea" pattern in the arm "underwater" | "deepsea" can never match. The alias is harmless but adds a dead branch and may confuse future readers.
| } | ||
| let risen = slot as u16; | ||
| let visit_progress = cycle_pos as f64 / visit_duration as f64; | ||
| let risen = (visit_progress * f64::from(JELLY_VISIT_ROWS)).round() as u16; |
There was a problem hiding this comment.
[INFO] .round() in jellyfish/bubble rise can produce one extra row beyond previous bounds
In ambient_life.rs, risen = (visit_progress * f64::from(JELLY_VISIT_ROWS)).round() as u16 can evaluate to JELLY_VISIT_ROWS when visit_progress is close to 1, whereas the previous integer slot logic clamped to JELLY_VISIT_ROWS - 1. The same rounding pattern is used for bubble rise. This may cause ambient elements to appear one row higher than intended.
| pub fn shed_priority(self) -> u8 { | ||
| match self { | ||
| Self::Rate => 9, | ||
| Self::Cache => 8, |
There was a problem hiding this comment.
[INFO] Cache and Ttft info segments share the same shed_priority
The new Cache segment is given shed_priority 8, identical to Ttft. If both segments are present and space is constrained, the order in which they are dropped may depend on insertion order or sort stability. Consider assigning distinct priorities or documenting the intended precedence.
| { | ||
| // The painted field exists only under the underwater theme; every | ||
| // other theme leaves the terminal's ground alone. A user-supplied | ||
| // `background_color` rewrites the underwater surfaces through |
There was a problem hiding this comment.
Derive the ramp colors from the effective theme surface (theme.surface_bg) so background_color overrides are respected, or explicitly disable background_color for the underwater theme. This requires reworking the hardcoded colors into a derived ramp similar to the pre-collapse generic path.
| .and_then(toml::Value::as_table) | ||
| .and_then(|table| table.get("ocean_treatment")) | ||
| .and_then(toml::Value::as_str) | ||
| .filter(|treatment| { |
There was a problem hiding this comment.
Preserve the existing theme when migrating if it is a custom or non-default choice, e.g. only set s.theme to "underwater" when the persisted theme is one of the built-in themes that previously supported deepsea, or migrate to a custom theme that carries the underwater field.
| match normalize_theme_name(value)? { | ||
| "system" => Some(Self::System), | ||
| "terminal" => Some(Self::Terminal), | ||
| "underwater" | "deepsea" => Some(Self::Underwater), |
There was a problem hiding this comment.
Remove the redundant "deepsea" alternative from the match arm because normalize_theme_name already canonically produces "underwater".
| "underwater" | "deepsea" => Some(Self::Underwater), | |
| "underwater" => Some(Self::Underwater), |
| } | ||
| let risen = slot as u16; | ||
| let visit_progress = cycle_pos as f64 / visit_duration as f64; | ||
| let risen = (visit_progress * f64::from(JELLY_VISIT_ROWS)).round() as u16; |
There was a problem hiding this comment.
Use .floor() or clamp the result to JELLY_VISIT_ROWS - 1 to keep the rise animation within its historical bounds. Similarly for bubble rise.
| pub fn shed_priority(self) -> u8 { | ||
| match self { | ||
| Self::Rate => 9, | ||
| Self::Cache => 8, |
There was a problem hiding this comment.
Assign a unique shed_priority to Cache or Ttft so drop precedence is deterministic when both segments are present.
The cucumber PTY suite was not in the local gate (--lib only), so three expectations shipped stale against the collapse rules: - the startup/live-shell wordmark is the lowercase codewhale mark; - the ctx reading stays silent below 50% fullness, so the inline screen-mode proof keys on the composer placeholder, which paints in both screen modes, and the pointer proof on the plain-workspace caption; - the new launch-worktree test pins core.autocrlf=false in its fixture repo so Windows CI (global autocrlf=true) checks out HEAD's files verbatim for the byte-fidelity assert. Receipts: cucumber active_composer_pointer + screen_mode_inline 2/2 passed with --features long-running-tests; launch_worktree 1/1 with RUST_MIN_STACK=16MiB; fmt clean; workspace clippy under CI flags clean. Signed-off-by: CodeWhale Bot <bot@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_9e820da0-02f4-4ddc-a4f0-b7f3c6cb9ad1) |
|
Claude finished @Hmbown's task in 3m 11s —— View job Review: tui: collapse ocean_treatment into ThemeId::Underwater
The newest push ( It does not address the two substantive issues raised by the earlier automated reviews on this PR. I independently re-derived both against the pre-collapse code (via Correctness bugs (confirmed)
Also still open (lower severity, flagged by earlier bot passes, still unaddressed)
AssessmentScope, locale, golden, and picker/list mechanics of the collapse itself are clean and well-tested (14,000+ passing tests cited in the PR body). The two confirmed correctness issues above are both user-visible appearance/config-persistence regressions the PR's own risk note anticipated but didn't fully guard against — worth fixing before merge given the "Medium Risk" label already on this PR. |
There was a problem hiding this comment.
Codewhale review
The PR collapses the separate ocean_treatment setting into a dedicated ThemeId::Underwater theme with deepsea/ombre aliases, read-compat migration for persisted deepsea, and substantial updates to ocean rendering, locale strings, golden files, and tests. The change is broadly coherent and well covered by tests; only minor maintainability concerns remain.
Findings
- [INFO] Redundant match arm in ThemeId::parse (
crates/tui/src/palette/themes.rs:839)
normalize_theme_name now maps deepsea, deep-sea, ocean, and ombre to "underwater", so the"deepsea"pattern in the match on normalized value is unreachable dead code. It can be removed for clarity. - [INFO] Cache infoline segment shares shed priority with Ttft (
crates/tui/src/tui/infoline.rs:88)
The new Cache segment is assigned the same shed_priority (8) as Ttft. If shedding is triggered, the order between these two segments is ambiguous. This may be intentional, but a distinct priority would make the intended fallback order explicit.
Assessment
The PR looks correct and well-tested. The migration path, theme normalization, ocean-ramp ownership, and golden updates align with the stated goal. Only minor cleanup items are noted.
Advisory review by Codewhale (codewhale review --pr 5858 --post, head 6f2e4c97f03d75257b008ad8289c3f5cae124561). 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.
| match normalize_theme_name(value)? { | ||
| "system" => Some(Self::System), | ||
| "terminal" => Some(Self::Terminal), | ||
| "underwater" | "deepsea" => Some(Self::Underwater), |
There was a problem hiding this comment.
[INFO] Redundant match arm in ThemeId::parse
normalize_theme_name now maps deepsea, deep-sea, ocean, and ombre to "underwater", so the "deepsea" pattern in the match on normalized value is unreachable dead code. It can be removed for clarity.
| pub fn shed_priority(self) -> u8 { | ||
| match self { | ||
| Self::Rate => 9, | ||
| Self::Cache => 8, |
There was a problem hiding this comment.
[INFO] Cache infoline segment shares shed priority with Ttft
The new Cache segment is assigned the same shed_priority (8) as Ttft. If shedding is triggered, the order between these two segments is ambiguous. This may be intentional, but a distinct priority would make the intended fallback order explicit.
Closes the ocean-collapse slice of the shell UX work. 11 commits: locale strings, mark assets, collapse core (deepsea alias, single picker list, read-only config migration, OceanRamp keys on theme), command/engine routing, ocean+picker+widget repaint, context_percent plumbing with abyss test, test/golden alignment, orphan-shim removal.
Gates run locally on the worktree: cargo fmt clean; workspace clippy (-D warnings + brief allows) clean; nextest -p codewhale-tui --lib: 11910 passed, 0 failed, 13 skipped; check-versions OK (0.9.11 in sync); dead-code PASS at 425. Note: replaces stale remote hmbown-shell-ux-fixes (diverged, left untouched, no force-push).
No-Issue: shell-UX milestone work tracked in #5573.
Note
Low Risk
Mostly appearance settings and read-compat migration; behavior change is users who relied on
ocean_treatment=deepseaon a non-underwater theme now need/theme underwater(handled on load for persisted deepsea).Overview
Removes the separate
ocean_treatmentsetting and makes the painted ocean field part of a dedicatedunderwatertheme (ThemeId::Underwater), with aliases likedeepsea,deep-sea, andombrerouting through the same normalizer./themeand config now set a singlethemevalue; the compoundset_theme_selection(theme, ocean_treatment)path and liveocean_treatmentapp state are gone.OceanRamp::for_themedecides whether the water column renders—only the underwater theme owns it.Migration: persisted
ocean_treatment = deepsea(and legacy spellings) folds intotheme = "underwater"on load;flatleaves the theme unchanged. The retired key is dropped on the next save. Config schema, settings UI DTOs, and locale strings for ocean-treatment labels and theme-picker treatment badges are removed.Smaller UX/copy changes: launch worktree prompt mentions Esc to go back, plus
LaunchWorktreeCreatedtoast; home dashboard title iscodewhale(lowercase); title animation matches.ProvisionedWorktreeis re-exported fromcodewhale-lane.Reviewed by Cursor Bugbot for commit 6f2e4c9. Bugbot is set up for automated code reviews on this repo. Configure here.