Skip to content

feat(tui): land welcome motion when the ocean is actually visible - #5656

Open
Hmbown wants to merge 2 commits into
mainfrom
grok/v0912-omarchy-ethos-motion-20260827
Open

feat(tui): land welcome motion when the ocean is actually visible#5656
Hmbown wants to merge 2 commits into
mainfrom
grok/v0912-omarchy-ethos-motion-20260827

Conversation

@Hmbown

@Hmbown Hmbown commented Aug 27, 2026

Copy link
Copy Markdown
Owner

Why

The 640ms idle whale / wordmark could finish while the launch menu still covered the ocean, so first visible idle looked still. Settings pickers also lacked a snappy presence settle.

What (Omarchy ethos, Codewhale material)

  • Start the welcome clock only when launch/onboarding are not covering the mark
  • Whale arrives with ease-out from ~87% presence (never from 0)
  • One almost-linear shine pass
  • Picker pop-in ~180ms, faster near-linear exit
  • No workspace-slide, no bounce, no Hyprland chrome
  • Reduced/Still skip decorative motion
  • 70ms receipt stagger and 800ms fish flee unchanged

Does not replace PR #5643 (MCP login copy + ocean_started_at). welcome_visible_since is a separate clock.

Rebased onto current origin/main after #5648.

Tests

Focused TUI motion/underwater/picker tests + fmt + coauthor trailers.

No-Issue: welcome-motion polish; occlusion-aware surface clock, not a tracked GitHub issue.

Do not merge while required GitHub checks are red or still running.

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

@claude

claude Bot commented Aug 27, 2026

Copy link
Copy Markdown
Contributor

Claude finished @Hmbown's task in 7m 10s —— View job


Review complete

  • Gather context (diff, changed files, CLAUDE.md/AGENTS.md rules)
  • Understand the request (code review only)
  • Review correctness (motion/timing state machines, panics, exhaustiveness)
  • Review provider/model/route safety implications (N/A — pure TUI motion PR)
  • Review reuse/simplification/dead code
  • Review test coverage
  • Review security implications
  • Post final review

Note: I was unable to run cargo test/cargo fmt in this sandbox (approval required for cargo invocations was not granted), so this review is source-only. The occlusion/timing logic reads correctly by inspection.

Summary

The welcome_visible_since occlusion clock, ease-out-quint arrival curve, and one-shot shine pass in crates/tui/src/tui/underwater.rs/motion/ethos.rs are well-scoped and cleanly separated from PR #5643's ocean_started_at clock, exactly as MOTION_ETHOS_HANDOFF.md describes. Tests for that lane (startup_surface_tests::*, the two widgets::tests::idle_welcome_* cases) look correct and actually exercise the occlusion/hand-off behavior.

Findings

1. The "picker pop-in" feature described in the PR/CHANGELOG is not actually wired to any renderer.

SettingsPickerController::settle_pop/is_settling (crates/tui/src/tui/settings_picker/controller.rs:312-326) are the only production call site of ethos::surface_pop/ethos::surface_exit. Neither method is ever called outside their own unit test:

$ grep -rn "settle_pop\|is_settling" --include="*.rs" . 
./crates/tui/src/tui/settings_picker/controller.rs  (definitions)
./crates/tui/src/tui/settings_picker/mod.rs         (unit test only)

theme_picker.rs::render (the only consumer of SettingsPickerController) never reads settle_pop/is_settling, and it isn't in this PR's changed-files list. So today a settings/theme picker renders at full opacity from frame one — the "~87% → 1.0 in 180ms" pop never actually paints.

MOTION_ETHOS_HANDOFF.md's own "Remaining" section is honest about this ("Live picker DIM needs a one-shot redraw while is_settling... This lane does not add a ViewAction::Redraw driver"), but the CHANGELOG.md entry states it as shipped fact: "Picker/menu pop uses the same 180 ms snappy settle" — with no such caveat, and docs/MOTION_CONTRACT.md's new Ethos section likewise doesn't flag it as unwired.

docs/MOTION_CONTRACT.md already has a precedent for exactly this situation ("Honesty note: catch-up is staged, not wired" — timing helpers exist and are tested, but no production call site drives them). This PR should either add the render-side wiring (even a no-redraw-driver version that just calls settle_pop once on first paint) or add the same kind of explicit "staged, not wired" note to docs/MOTION_CONTRACT.md and soften the CHANGELOG line so it doesn't read as shipped behavior.

Fix this →

2. Minor: welcome_surface_occluded duplicates part of idle_mark_animation_enabled.

ensure_welcome_surface_started (crates/tui/src/tui/underwater.rs:1537) checks both welcome_surface_occluded(app) (launch/onboarding) and !idle_mark_animation_enabled(app), but idle_mark_animation_enableddecorative_shell_motion_enabled (underwater.rs:1513-1519) already requires !app.launch.visible && app.onboarding == OnboardingState::None. Not a bug (belt-and-suspenders, and the explicit check documents intent well), but the welcome_surface_occluded condition is currently redundant with logic already covered by idle_mark_animation_enabled. Low priority.

Good choices worth calling out

  • Cleanly isolates its clock (welcome_visible_since) from PR fix(tui): recover MCP login and restore welcome motion #5643's in-flight ocean_started_at work, with an explicit non-collision rationale in both the field doc comment and handoff doc — reduces merge risk.
  • ethos.rs centralizes the timing constants/curves with tests that assert relative properties (exit faster than entry, pop starts near 0.87, etc.) rather than pinning exact pixel/frame values — resilient test design.
  • No panics/unwraps introduced; the new arithmetic (saturating_sub, guarded duration_ms == 0, bounded elapsed_ms >= WINDOW checks) is careful about edge cases.
  • Reduced/Still correctly short-circuit to settled state in every new animated path (fade_opacity, surface_pop, surface_exit, ensure_welcome_surface_started).

CodeWhale Bot and others added 2 commits August 27, 2026 06:38
The idle whale's 640ms surface and Codewhale letter-write now start when
that mark is actually on screen. Full-canvas launch and onboarding
occlude the welcome clock so the shine cannot finish off-canvas.

Arrivals ease-out-quint from ~87% presence; exits are faster and closer
to linear; picker pop is a 180ms snappy settle. 70ms receipt stagger and
800ms fish flee stay one-shot. Reduced/Still skip decorative treatments.

Does not fork ocean_started_at into Option; welcome_visible_since stays the occlusion-aware surface clock.

Co-authored-by: Codewhale Agent <codewhale-agent@hmbown.local>
Signed-off-by: CodeWhale Bot <bot@codewhale.net>
Allow the unused chef's-choice ethos/picker DIM API until a redraw driver
exists, use a const assertion for exit-vs-entry timing, and sync the TUI
changelog slice after rebasing onto main.

Signed-off-by: CodeWhale Bot <bot@codewhale.net>
@Hmbown
Hmbown force-pushed the grok/v0912-omarchy-ethos-motion-20260827 branch from 334e779 to 12a7950 Compare August 27, 2026 13:38
@claude

claude Bot commented Aug 27, 2026

Copy link
Copy Markdown
Contributor

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


Review complete

  • Gather context (diff, changed files, CLAUDE.md/AGENTS.md rules)
  • Understand the request (code review, PR synchronize trigger — new commit 12a7950)
  • Review correctness (motion/timing state machines, panics, exhaustiveness)
  • Review provider/model/route safety implications (N/A — pure TUI motion PR)
  • Review reuse/simplification/dead code
  • Review test coverage
  • Review security implications
  • Post final review

Summary

This synchronize adds one follow-up commit, 12a7950 ("keep welcome-motion helpers compiling under -Dwarnings"), on top of the previously-reviewed 26c217e. It's small: it sprinkles #[allow(dead_code)] over the ethos/picker-pop API, switches a runtime assert to a const { assert!(...) } compile-time check, extracts fish_flee_offset's magic 800 into ethos::FISH_FLEE_MS, and syncs crates/tui/CHANGELOG.md. These are mechanical and correct — the const-assertion works fine on this workspace's MSRV (rust-version = "1.88", inline const blocks stabilized in 1.79), and the FISH_FLEE_MS extraction is a good small dedup.

Findings

1. Still open: the "picker pop-in" is dead code with no production call site, and the CHANGELOG/docs still describe it as shipped.

The new commit's own message says it plainly: "Allow the unused chef's-choice ethos/picker DIM API until a redraw driver exists" — and every new #[allow(dead_code)] in crates/tui/src/tui/motion/ethos.rs and crates/tui/src/tui/settings_picker/controller.rs (settle_pop, is_settling, surface_pop, surface_exit, ease_out_exit, FADE_MS, SURFACE_EXIT_MS, fade_opacity) confirms it. I re-checked the current tree and settle_pop/is_settling are still called only from crates/tui/src/tui/settings_picker/mod.rs's unit test (crates/tui/src/tui/settings_picker/mod.rs:362-390) — theme_picker.rs, the only render-side consumer of SettingsPickerController, never calls either method.

Despite that, both CHANGELOG.md:12-19 and crates/tui/CHANGELOG.md:12-19 (unchanged by this commit) still state as fact: "Picker/menu pop uses the same 180 ms snappy settle" — with no caveat — and docs/MOTION_CONTRACT.md's new Ethos section (also unchanged) doesn't flag it as unwired either. This is exactly the situation docs/MOTION_CONTRACT.md already has a documented pattern for one section down ("Honesty note: catch-up is staged, not wired").

This commit had the chance to close that gap — it touches crates/tui/CHANGELOG.md and ethos.rs directly — but instead it codifies the unwired state with #[allow(dead_code)] comments rather than fixing the doc/changelog claim or adding the render-side call. Recommend either:

  • adding a matching "picker pop-in is staged, not wired" honesty note under the Ethos section of docs/MOTION_CONTRACT.md and softening both CHANGELOG entries so "Picker/menu pop uses the same 180 ms snappy settle" doesn't read as shipped, or
  • wiring settle_pop/is_settling into theme_picker.rs::render (even a no-redraw-driver version that blends opacity once on first paint).

Fix this →

2. Minor, unchanged from prior review: welcome_surface_occluded still duplicates part of idle_mark_animation_enabled (crates/tui/src/tui/underwater.rs:1537). Not a bug — belt-and-suspenders, and the redundant check documents intent. Low priority, no action needed.

Good choices worth calling out

  • The new commit's fix is honest about the gap in its own commit message and code comments (// picker DIM host API; live redraw driver is a follow-up) even though the CHANGELOG/docs weren't updated to match — the code-level honesty just needs to propagate outward.
  • fish_flee_offset's 800ethos::FISH_FLEE_MS extraction removes a duplicated magic number and keeps the one-shot timing centralized in ethos.rs, consistent with the module's stated purpose ("hosts should ask here rather than invent").
  • const { assert!(SURFACE_EXIT_MS < SURFACE_POP_MS) } turns a previously-runtime invariant into a compile-time one — strictly stronger, and correctly scoped to this workspace's rust-version = "1.88".

Note: cargo test/cargo fmt were not run in this sandbox (no approval for cargo/network invocations); this review is source-only via git diff and grep.

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.

1 participant