Skip to content

tui: collapse ocean_treatment into ThemeId::Underwater - #5858

Open
Hmbown wants to merge 11 commits into
mainfrom
feat/shell-ux-collapse-20260902
Open

tui: collapse ocean_treatment into ThemeId::Underwater#5858
Hmbown wants to merge 11 commits into
mainfrom
feat/shell-ux-collapse-20260902

Conversation

@Hmbown

@Hmbown Hmbown commented Sep 2, 2026

Copy link
Copy Markdown
Owner

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

Medium Risk
Settings migration and theme resolution affect every user's appearance config; incorrect migration could surprise users who had ocean_treatment paired with non-default themes, though tests cover deepsea/flat legacy paths.

Overview
Removes the separate ocean_treatment setting and folds the painted ocean background into a first-class underwater theme (ThemeId::Underwater). Aliases like deepsea, deep-sea, and ombre resolve to that theme; OceanRamp only applies when that theme is active, not as a modifier on other themes.

Settings & config: The ocean_treatment field, compound set_theme_selection, and theme-picker treatment UI/strings are dropped. On load, persisted ocean_treatment=deepsea migrates to theme = "underwater" and the old key is omitted on save; flat treatment is ignored. /theme usage docs now mention underwater.

UX copy (all locales): Home dashboard title is codewhale (lowercase); launch worktree prompt notes Esc to go back; new LaunchWorktreeCreated toast string. Ocean/treatment-related config and theme preview strings are removed.

Other: codewhale_lane re-exports ProvisionedWorktree; turn-loop title animation uses lowercase codewhale.

Reviewed by Cursor Bugbot for commit 2b54b22. Bugbot is set up for automated code reviews on this repo. Configure here.

Hmbown and others added 11 commits September 2, 2026 13:57
`/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>
Copilot AI lite review requested due to automatic review settings September 2, 2026 21:27
@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, add credits to your account and enable them for code reviews in your settings.

@cursor

cursor Bot commented Sep 2, 2026

Copy link
Copy Markdown

Bugbot couldn't run - usage limit reached

Bugbot 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

claude Bot commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

Claude finished @Hmbown's task in 3m 30s —— View job


Review: tui: collapse ocean_treatment into ThemeId::Underwater

  • Gather context (PR body, changed files list)
  • Read repo guidance (CLAUDE.md/AGENTS.md)
  • Review diff against origin/main...HEAD
  • Check correctness bugs
  • Check provider/model/route safety invariant (n/a — no route/model code touched)
  • Check reuse/simplification/dead code
  • Check test coverage
  • Check security (n/a — no new I/O, secrets, or exec paths)
  • Post final review summary

The ThemeId::Underwater collapse itself (ocean.rs, palette/themes.rs, settings.rs, theme_picker.rs, commands/groups/config/config.rs) is a clean, well-evidenced refactor: OceanRamp::for_theme now keys off theme.name == UNDERWATER_UI_THEME.name (survives with_background_color, which never touches name — verified in palette/themes.rs:996), the settings migration is correctly one-way (settings.rs:907-925, folds legacy ocean_treatment=deepsea into theme="underwater", never writes the retired key back), and the picker/message-id/locale cleanup deletes real dead code rather than leaving a shim. Good adherence to the "migrate the last consumer" rule here.

Scope concern — this PR bundles several unrelated changes not described in the PR body's 11-commit summary:

  1. Branding regression (highest concern). notifications.rs, notification_payload.rs, core.rs, and the startup goldens change the canonical "Codewhale" window title / notification fallback / dashboard string to lowercase "codewhale" (e.g. notifications.rs:725,749,782,914,949,958, notification_payload.rs:69, golden startup_80x24.txt now literally renders codewhale v0.9.12). This directly contradicts this repo's own AGENTS.md: "Public name is Codewhale... change only through an explicit migration." It's also now inconsistent with the rest of the codebase, which still uses uppercase "Codewhale" in client.rs:1668,4281, prompt_suggestion.rs:442, mcp/oauth.rs:1346, auto_review.rs:1552, and the doc comment directly above the changed code still says - **Title**: "Codewhale" (notifications.rs:1079, now stale). This looks unrelated to the ocean/theme collapse and should probably be its own reviewed change, or reverted here.
  2. New "cache %" info-line segment (frame.rs:145-183, infoline.rs new Cache variant) — unrelated to the theme collapse, and the cache_pct computation block is duplicated verbatim between the if let Some((tokens, rate)) = output_figures(app) and else branches (frame.rs:145-160 vs 162-180); should be hoisted above the branch. No test exercises the new segment (rounding via hit*100 + cache_total/2, unwrap_or(100) fallback path).
  3. Context segment now hidden below 50% context usage (frame.rs:96-113) — a behavior change to an existing, unrelated indicator.
  4. Launch-card "reflexive Enter" fix + worktree-creation receipt (session_state.rs, lane/src/lib.rs, new LaunchWorktreeCreated message) — a legitimate, well-tested fix (good regression test in launch_worktree_tests), but it's a separate bug fix from the theme collapse and inflates this diff's blast radius.

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: frame.rs cache calc uses checked_div after already checking cache_total > 0, so the None branch is unreachable — harmless but redundant.

No correctness bugs, panics, or provider/model-routing concerns found in the core collapse logic itself.

Copilot AI left a comment

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.

🟡 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 underwater theme (including settings/config migration + picker/list updates).
  • Plumb context_percent into 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

  • risen is computed with floating-point + .round(), which can produce JELLY_VISIT_ROWS at 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 keep risen in 0..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;
Comment on lines 2017 to 2024
// 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,
Comment on lines 60 to +62
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 codewhale-agent Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[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| {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[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;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[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,

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[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

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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| {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Remove the redundant "deepsea" alternative from the match arm because normalize_theme_name already canonically produces "underwater".

Suggested change
"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;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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,

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Assign a unique shed_priority to Cache or Ttft so drop precedence is deterministic when both segments are present.

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.

2 participants