Compose Tideline startup into the shared composer shell - #5770
Conversation
(cherry picked from commit 2001b8f)
(cherry picked from commit 370a7fc)
(cherry picked from commit 07ab498)
(cherry picked from commit d9b9d6d)
(cherry picked from commit 59417c9)
(cherry picked from commit a2dba22)
(cherry picked from commit 888957e)
(cherry picked from commit 5512159)
(cherry picked from commit a936482)
(cherry picked from commit c04a3cc)
(cherry picked from commit e2c7225)
(cherry picked from commit 55fb926)
(cherry picked from commit d9bd021)
(cherry picked from commit ca18b92)
(cherry picked from commit 4b60408)
(cherry picked from commit d073ae1)
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Claude finished @Hmbown's task in 1m 56s —— View job Reviewing PR #5770
This is a large PR (17 commits, 58 files, ~2.4k/1k lines). Digging into the core TUI logic now ( |
|
Claude finished @Hmbown's task in 1m 22s —— View job Reviewing PR #5770
Digging into the core TUI logic now ( |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 84a988dc0e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // provider/config notice, or a previous session record: only an | ||
| // intentional resume or explicit initial input enters the live session | ||
| // path directly. | ||
| let launch_visible = resume_session_id.is_none() && initial_input.is_none(); |
There was a problem hiding this comment.
Every clean interactive launch now sets launch.visible to true, but the event loop's only call to open_setup_checkpoint_if_due is guarded by !app.launch.visible; later, begin_launch_session merely clears the flag without invoking that gate. Consequently, a returning user with a due versioned Constitution checkpoint can choose New Session, Chat, or submit from the Startup composer and work for the entire process without seeing the checkpoint; --skip-onboarding also no longer records its deferral. Run the post-launch setup gate when Startup transitions into a session, or otherwise preserve the gate before exposing Startup.
Useful? React with 👍 / 👎.
| &[work_line.as_str()], | ||
| crate::tui::phase_strip::context_percent_from_app(app), |
There was a problem hiding this comment.
At widths of 100 columns or more, this newly wired active-session rail renders both the work summary and context_percent, even though the same frame already renders the context meter in the topbar and the footer, creating multiple owners for a fact that the TUI contract assigns to the header; the contract likewise assigns work to the top strip. This makes the passive rail compete with established surfaces and allows their projections to diverge, so the live rail should omit these facts or replace their existing owners rather than duplicating them.
AGENTS.md reference: crates/tui/AGENTS.md:L8-L10
Useful? React with 👍 / 👎.
| after = AFTER | ||
| print("ARCHIVED: former Codewhale empty-state portrait — main vs Signal Cut\n") |
There was a problem hiding this comment.
When the documented default invocation python3 scripts/preview-whale-art.py runs, evaluating sys.stdout.isatty() raises NameError because this rewrite removed import sys; --plain only avoids the error through short-circuiting, and the missing-Pillow path also still uses sys.stderr. Restore the import so the normal preview and PNG fallback execute.
Useful? React with 👍 / 👎.
| if let Some(input_row) = hitboxes | ||
| .composer | ||
| .input | ||
| .map(|area| area.y.saturating_sub(stage_area.y)) |
There was a problem hiding this comment.
With the full four-row Startup composer, the new input rect begins at dock.y + 1, but render_launch_completion_popup paints its first result at input_y - 1; typing / or @ therefore writes the selected completion directly over the rounded shell's top border instead of above it. Pass an anchor that accounts for the enclosure's border, or reserve the border row before stacking popup entries.
Useful? React with 👍 / 👎.
| route: hitboxes | ||
| .iter() | ||
| .find(|hitbox| hitbox.id == TopbarSegmentId::Model) | ||
| .map(|hitbox| hitbox.area), |
There was a problem hiding this comment.
When an active run or pod adds segments before Model and a narrow terminal causes those segments to shed, the renderer moves Model next to the brand, but topbar_hitboxes still positions it after every original segment. Registering that stale rectangle as the new route target means clicking the visibly painted route does nothing, while an unrelated or off-screen rectangle owns the action; compute the target from the same kept-segment layout used for painting.
AGENTS.md reference: crates/tui/AGENTS.md:L19-L21
Useful? React with 👍 / 👎.
The clippy collapsible_if repair was still two lines; rustfmt --check wants the let-chain on one line. Refs #5768 Signed-off-by: Hunter Bown <hmbown@gmail.com>
|
Claude finished @Hmbown's task in 1m 58s —— View job Reviewing PR #5770
The latest push ( Confirmed correctness issues
Contract issue
Other notes
None of these were introduced or fixed by the two most recent commits ( |
![codewhale-agent[bot]](https://avatars.githubusercontent.com/u/101357273?s=60&v=4){kind=small}
There was a problem hiding this comment.
Codewhale review
The PR composes the Tideline startup surface into the shared rounded composer shell, removes the retired launch_screen setting, centralizes provider-picker entry, adds a passive active-session Tideline rail, and migrates MCP/plugin boot output into the compact footer. The diff includes extensive tests and golden updates. Main concerns are a narrow-terminal startup composer edge case, the removal of a user-facing launch preference, and a few integration gaps around the centralized provider-picker path and Top-placement rail.
Findings
- [WARNING] Enclosed startup composer can render an empty shell at narrow dock widths (
crates/tui/src/tui/underwater.rs)
Theencloseddecision inrender_tideline_startupuseslayout.dock.width >= 6, buttideline_composer_geometrypositions the[↑]submit target before the content rect until roughly width 9. For dock widths 6–8,render_launch_composerhitscontent_width == 0and returns after painting only the rounded shell, leaving no placeholder or input. Compact terminals should fall back to the one-line projection instead of painting an empty shell. - [WARNING] Unified provider-picker path may lose the duplicate-picker guard (
crates/tui/src/tui/ui/overlays.rs)
The previousAppAction::OpenProviderPickercode guarded the non-onboarding case withapp.view_stack.top_kind() != Some(ModalKind::ProviderPicker)before pushing a new picker. The newopen_provider_pickerhelper delegates toopen_launch_provider_picker, and it is not clear from the diff whether that helper preserves the guard. If it does not, repeated F3 or topbar clicks while the picker is already open could stack duplicate picker views. A regression test for the already-open case is missing. - [WARNING] Removing launch_screen is a breaking startup behavior change without an opt-out (
crates/tui/src/tui/app/init.rs)
Settings::launch_screenis deleted andApp::newnow always setslaunch_visiblefor fresh interactive launches, except resume or initial input. Returning users who previously setlaunch_screen = falsewill now land on the Tideline startup hero. The new test only verifies that the legacy key is dropped when saving settings; there is no migration warning or runtime opt-out described. This should be confirmed as intentional and accepted by product behavior. - [INFO] Missing test coverage for the explicit compact composer_border=false startup path (
crates/tui/src/tui/underwater/tideline_tests.rs)
LaunchComposerDisplay.enclosedandtideline_startup_hitboxes_with_composer(stage, false)are the explicit compact fallback, but the new tests mostly cover the defaultcomposer_border = truerounded shell. A test pinning the one-line fallback rendering and hitboxes, plus the fallback when a full shell cannot fit, would lock in the truthful compact-terminal behavior. - [INFO] No integration test for F3 opening the actual provider picker (
crates/tui/src/tui/ui/event_loop.rs)
The keyboard-shortcut unit test and the mouse-event unit test only prove thatTopbarRoutePickerRequestedis emitted. There is no event-loop or handler test showing that F3 on an empty view stack ultimately opens the picker exactly once and respects onboarding. This is a new shell-wide route binding, so an integration test would prevent regressions in the centralized open path. - [INFO] Active-session rail has no test for Top placement (
crates/tui/src/tui/work_surface/tideline.rs)
active_session_tideline_rail_widthclaims Top receives the compact summary, but the new tests cover only default Left, occupied Left, Right, and Off. Top placement with an empty legacy work surface is untested and could regress the rail width ladder or its interaction with the top work strip.
Assessment
The PR is substantial and generally well-tested, with clear separation between the shared composer geometry and caller-owned content. The main correctness risk is the narrow-terminal startup composer shell, and there are a few integration gaps around the centralized provider-picker entry and rail placements. The launch_screen removal also needs explicit product confirmation because it changes existing users' startup behavior. I would not merge until the narrow-shell edge case and provider-picker guard are verified.
Advisory review by Codewhale (codewhale review --pr 5770 --post, head 5aa6052e6d2ac04d7efe4b93d9f8b5324cb0f248). 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 blocker-size startup goldens still pinned the retired flat composer strip, so startup_matches_goldens_at_blocker_sizes failed on ubuntu and windows CI. The 40x10 floor golden is unchanged: the composer sheds at that size by design. Refs #5768 Signed-off-by: Hunter Bown <hmbown@gmail.com>
|
Claude finished @Hmbown's task in 2m 15s —— View job Reviewing PR #5770 (synchronize)
The new commit since the last review, Because this commit changes no logic, I re-verified each of the five previously-confirmed findings directly against current Confirmed correctness/contract issues (unchanged by this push)
None of these are touched by Other notes (unchanged from prior pass)
Recommendation: items 1–3 (checkpoint/compliance gate skipped, completion popup painting over the border, dead route-click hitbox) are the ones I'd block merge on. Item 5 is worth fixing before merge as a contract violation. Item 4 is a trivial one-liner. |
There was a problem hiding this comment.
Codewhale review
PR #5770 composes the Tideline startup shell into the shared rounded composer geometry, removes the retired launch_screen setting, adds a live session Tideline rail, and refactors boot diagnostics into a compact footer notice. The patch is broad but generally coherent, with several new tests covering the main paths. The highest review risk is the provider-picker apply refactor possibly losing a duplicate-view guard, and the compact composer fallback lacking direct regression coverage.
Findings
- [WARNING] Provider-picker duplicate guard may be lost in apply refactor (
crates/tui/src/tui/ui/apply.rs)
The previous AppAction::OpenProviderPicker branch only pushed ProviderPickerView when view_stack.top_kind() != Some(ModalKind::ProviderPicker). The refactor delegates to open_provider_picker, which calls open_launch_provider_picker in the non-onboarding path. If open_launch_provider_picker does not itself preserve that top_kind guard, repeated /provider commands or topbar route clicks can push duplicate picker views. Verify the guard exists in open_launch_provider_picker; otherwise restore it in open_provider_picker. - [WARNING] Compact composer fallback has no regression coverage (
crates/tui/src/tui/underwater/tideline_tests.rs)
The launch composer was split into a rounded-shell path and a compact one-line fallback, but the new tests and updated goldens exercise only the default enclosed shell (composer_border=true). The composer_border=false path, including hitbox behavior and the removed divider rule, has no direct unit or golden coverage. A silent regression there would not be caught by the listed test commands. - [INFO] launch_screen setting removal is a visible behavior change (
crates/tui/src/tui/app/init.rs)
Removing Settings::launch_screen and making launch_visible depend only on resume_session_id and initial_input means existing users who explicitly set launch_screen = false will now see the startup hero on fresh launches. The legacy setting is accepted and dropped on save, but the PR should ensure release notes and user-facing docs explicitly call out that this opt-out no longer exists.
Assessment
The change is well-structured and test-conscious, with focused tests for startup visibility, provider route events, boot diagnostics, and the live Tideline rail. Before merge, the provider-picker guard should be confirmed, and explicit coverage for the compact composer_border=false startup fallback is recommended. The core shell geometry sharing looks sound.
Advisory review by Codewhale (codewhale review --pr 5770 --post, head 57db4575c73558b2db59b93f448a66bcb92546f6). 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.
1 participant
Closes #5768 (source composition only; no merge or release action).
Scope
[↑]paint/hitbox geometry source for the Startup shell and preserves the normal launch keyboard/mouse submit dispatcher.Local evidence
RUST_MIN_STACK=33554432 cargo test -p codewhale-tui launch_composer_tests:: --locked— 13 passed.RUST_MIN_STACK=33554432 cargo test -p codewhale-tui composer_chrome::tideline_tests:: --locked— 9 passed.RUST_MIN_STACK=33554432 cargo test -p codewhale-tui mouse_ui::tests::composer_click_focuses_and_send_click_matches_the_keyboard_submit --locked— 1 passed.cargo fmt --all -- --checkandgit diff --checkpassed.╭…╮│…│╰…╯chrome and a visible[↑]; a pointer click and Enter reached the same resulting frame aside from the live clock.Still explicitly open
This does not prove active-work composer parity, rail/receipt/ledger runtime behavior, clean onboarding completion, provider safety/selection, CI, release artifacts, deployment, auth, or launch readiness. The current generated diving/current whale mark remains the only brand mark; no retired fluke asset is reintroduced.