fix(tui): make topbar route segment interactive

[Hmbown]

Closes #5756

Product change

The painted route/model segment now opens the existing provider picker by click or F3. Both entry points delegate to the same picker/apply path as /provider; the header owns neither catalog data nor route authority.

Verification

• focused topbar interaction target test
• mouse click event test, including Startup shell routing
• F3 routing and keybinding contract tests
• cargo fmt --all -- --check
• git diff --check

Boundary

This makes the visible control truthful. It does not resolve #5755, the wider single-authority provider/runtime/catalog lifecycle problem.

[chatgpt-codex-connector]

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review	Status	Commit	Review trigger
📝 Code Review	✅ Completed 2026-08-30T22:52:08.602475Z	55fb926	PR opened

ℹ️ 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" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

[claude]

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

---

Review: fix(tui): make topbar route segment interactive

Summary: This is a clean, well-scoped fix. It correctly unifies the topbar's route/model segment click (and new F3) handling to delegate to the exact same open_provider_picker/ProviderPickerView path already used by /provider, and it fixes a real pre-existing bug where the header's mouse-target routing was placed after the launch-screen's catch-all mouse consumption, so header clicks silently did nothing on the pre-session launch screen. Verified this by tracing the code (see below) — no correctness issues, no route/provider-safety violations, and the refactor actually reduces duplication rather than adding a new layer.

Correctness

• Confirmed render() in crates/tui/src/tui/ui/frame.rs:966-977 early-returns during onboarding before render_topbar_row/register_topbar_interaction_targets run, so no HEADER_ROUTE target is registered during onboarding — the new mouse/F3 handling can't fire there, which matches existing onboarding-owns-its-own-keys behavior.
• The F3 handler in event_loop.rs:4410-4431 sits after the if app.onboarding != OnboardingState::None { …; continue; } block (event_loop.rs:4289-4408), so it's onboarding-gated the same way the mouse target is. Consistent, not redundant with anything else in that function (the codebase's existing pattern is many localized view_stack.is_empty() guards rather than one central dispatch).
• open_provider_picker (overlays.rs:287-303) correctly reproduces the old inline AppAction::OpenProviderPicker branch from apply.rs, including the "already open" guard (open_launch_provider_picker / open_onboarding_provider_picker both early-return on view_stack.top_kind() == Some(ModalKind::ProviderPicker)), so no risk of double-pushing the picker onto the view stack.
• TopbarInteractionHitboxes::route is derived from the rendered hitboxes (post-layout), not from topbar_segments directly, so the click target only exists where the segment actually painted — correctly handles narrow-width truncation.
• No exhaustiveness/compile risk from the two new enum variants (InteractionAction::OpenProviderPicker, InspectDetail::Route): grepped for all consumers, and mouse_ui.rs is the only match site, already updated.
• register_topbar_interaction_targets is shared between the launch-screen render path (frame.rs:1001-1002) and the session-shell path (frame.rs:1255+), which are mutually exclusive branches of render() — confirmed no double-registration within one frame.

Provider/model/route safety (EPIC #2608 invariant)

No violation. The picker itself remains the sole authority for catalog/readiness/apply; the topbar and F3 only ever emit ViewEvent::TopbarRoutePickerRequested, which routes to the same open_provider_picker helper the /provider command uses. No provider/model inference from a prefix string or freeform text anywhere in this diff.

Reuse/simplification

• Good deduplication: apply.rs's AppAction::OpenProviderPicker arm shrank from a 22-line inline branch to a single delegated call (apply.rs), and the two render_topbar_row call sites' near-duplicated interaction-target/hover-label registration block was extracted into register_topbar_interaction_targets (frame.rs). Both are genuine consolidations, not new bridging layers, matching AGENTS.md guidance.
• render_topbar_row's visibility was correctly narrowed from pub(crate) to private now that both call sites live in the same file.
• Minor, out of scope for this PR: crates/tui/src/tui/underwater.rs:1346 (header_hitboxes/HeaderHitbox) looks like a pre-existing, separate hitbox mechanism that's only referenced from its own tests, not from any production render/mouse path — pre-existing dead code, not introduced here, not worth blocking on.

Tests

The new tests are appropriately targeted: topbar_route_click_emits_provider_picker_request (mouse_ui.rs) specifically pins the launch-screen regression (comment calls out it "protects against its old catch-all mouse route swallowing the topbar affordance"), topbar_route_f3_requires_a_plain_function_key (shell_key_routing.rs) covers the modifier-gating, and topbar_route_segment_registers_interaction_target (frame.rs) verifies the rendered hitbox matches the registered target. One small gap: there's no integration-level test exercising event_loop.rs's F3 branch end-to-end into ViewEvent::TopbarRoutePickerRequested → open_provider_picker (only the key-matching predicate and the ViewEvent handler are tested in isolation) — acceptable given this codebase's existing testing patterns, not a blocker.

Security

No secret handling, shell/exec, or input-validation surface touched.

Note: I was not able to run cargo build/cargo fmt --all -- --check/tests in this environment (Bash tool calls requiring cargo were not auto-approved), so the above is based on static code tracing rather than a green build. The PR description states cargo fmt --all -- --check and git diff --check were already run by the author.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 55fb926277

ℹ️ 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".

[chatgpt-codex-connector]

Derive the route target from the shed layout

When an active Run, Pod, or Whales segment is removed at narrower widths, Topbar::render moves the Model segment left using shed_pass, but topbar_hitboxes() still positions it after every original segment. Taking the route rectangle from that list therefore registers HEADER_ROUTE over the wrong cells: clicking the painted route can do nothing, while clicking cells to its right can unexpectedly open the provider picker. Compute the interaction rectangle from the same kept-segment geometry used for rendering.

Useful? React with 👍 / 👎.

[codewhale-agent]

Codewhale review

PR makes the topbar route/model segment interactive by click or F3 and delegates to the existing provider picker through a new shared helper. The design direction is sound, but the diff appears to call the new helper from sibling modules without importing it, and the new ViewEvent/F3 routing path is only partially tested.

Findings

• [ERROR] open_provider_picker may be unresolved in handlers.rs (crates/tui/src/tui/ui/handlers.rs:1913)
  The new ViewEvent::TopbarRoutePickerRequested arm calls open_provider_picker, but that helper is defined in the sibling overlays module. This diff does not add an import for it. Unless the file already has a glob import such as use super::overlays::*, this will fail to compile with an unresolved-name error.
• [ERROR] open_provider_picker may be unresolved in apply.rs (crates/tui/src/tui/ui/apply.rs:1719)
  AppAction::OpenProviderPicker now calls open_provider_picker, but the helper is defined in the sibling overlays module and no new import is added in the diff. Without an existing glob or re-export import, this will fail to compile.
• [INFO] TopbarRoutePickerRequested handler is not directly tested (crates/tui/src/tui/ui/handlers.rs:1913)
  The PR tests that a mouse click emits TopbarRoutePickerRequested and that F3 is recognized, but there is no test asserting that handle_view_events actually handles this variant by opening the provider picker. A regression in this arm could otherwise go unnoticed.
• [INFO] F3 event-loop routing is not directly tested (crates/tui/src/tui/ui/event_loop.rs)
  The new F3 branch in run_event_loop is central to the keyboard entry point, but the added tests only cover is_provider_route_shortcut. There is no test proving that an F3 key event with an empty view stack dispatches TopbarRoutePickerRequested and continues without leaking into launch-surface key handling.

Suggestions

• crates/tui/src/tui/ui/handlers.rs:1913 — Use the fully qualified path to the new sibling helper; the diff adds this call without a corresponding use, so the unqualified name may not resolve.

                  crate::tui::ui::overlays::open_provider_picker(app, config, engine_handle).await;
  

• crates/tui/src/tui/ui/apply.rs:1719 — Use the fully qualified path to the new sibling helper; the diff adds this call without a corresponding use, so the unqualified name may not resolve.

                  crate::tui::ui::overlays::open_provider_picker(app, config, engine_handle).await;
  

Assessment

The PR cleanly consolidates provider-picker opening and makes the topbar route segment truthful. Before merge, confirm that the new open_provider_picker helper resolves in both call sites and add direct coverage for the ViewEvent and F3 event-loop dispatch paths.

---

Advisory review by Codewhale (codewhale review --pr 5763 --post, head 55fb926277f274ccf801048f3848676ec6a12575). 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.

[codewhale-agent]

[ERROR] open_provider_picker may be unresolved in handlers.rs

The new ViewEvent::TopbarRoutePickerRequested arm calls open_provider_picker, but that helper is defined in the sibling overlays module. This diff does not add an import for it. Unless the file already has a glob import such as use super::overlays::*, this will fail to compile with an unresolved-name error.

[codewhale-agent]

[ERROR] open_provider_picker may be unresolved in apply.rs

AppAction::OpenProviderPicker now calls open_provider_picker, but the helper is defined in the sibling overlays module and no new import is added in the diff. Without an existing glob or re-export import, this will fail to compile.

[codewhale-agent]

[INFO] TopbarRoutePickerRequested handler is not directly tested

The PR tests that a mouse click emits TopbarRoutePickerRequested and that F3 is recognized, but there is no test asserting that handle_view_events actually handles this variant by opening the provider picker. A regression in this arm could otherwise go unnoticed.

[codewhale-agent]

Use the fully qualified path to the new sibling helper; the diff adds this call without a corresponding use, so the unqualified name may not resolve.

Suggested change

	open_provider_picker(app, config, engine_handle).await;
	crate::tui::ui::overlays::open_provider_picker(app, config, engine_handle).await;

[codewhale-agent]

Use the fully qualified path to the new sibling helper; the diff adds this call without a corresponding use, so the unqualified name may not resolve.

Suggested change

	open_provider_picker(app, config, engine_handle).await;
	crate::tui::ui::overlays::open_provider_picker(app, config, engine_handle).await;

[claude]

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

---

Review: fix(tui): make topbar route segment interactive

• Gather context (PR diff, prior review comments from Claude/Codex/Codewhale-agent)
• Verify the open_provider_picker unresolved-symbol claim (handlers.rs, apply.rs)
• Verify the shed-layout hitbox mismatch claim (frame.rs)
• Check remaining diff for correctness, provider/route safety, reuse, tests, security
• Post final review

Summary: The design is sound and matches /provider's existing picker path, but there is one real correctness bug worth fixing before merge (Codex's finding, confirmed by tracing), and two of Codewhale-agent's "ERROR" findings are false positives that should be disregarded.

Confirmed bug: route hitbox can misalign at narrow widths

register_topbar_interaction_targets's HEADER_ROUTE target (crates/tui/src/tui/ui/frame.rs:171-178) takes its Rect from topbar_hitboxes(&topbar, area) (crates/tui/src/tui/topbar.rs:435). That function walks topbar.segments.iter() — the full, unshed segment list — and accumulates x assuming every segment painted. But Topbar::render (topbar.rs:326-332) paints only shed_pass's kept list, which drops Run/Pod/Whales/Theme/Workspace (shed priorities 1-5) before Model (priority 0, never shed) once the row is too narrow. Because Run, Pod, and Whales all render before Model in segment order, shedding any of them shifts the Model segment left on screen while topbar_hitboxes() still reserves space for it — so the registered HEADER_ROUTE rect drifts right of the actually-painted text: clicking the visible route can do nothing, and clicking cells to its right (over Theme/Workspace/the pinned meter) can wrongly open the provider picker.

This bug lives in topbar_hitboxes() itself, which predates this PR and previously only backed hover highlighting (harmless if occasionally wrong) and unused stored hitboxes — the code comment this PR replaces literally says "click routing in a follow-up slice." This PR is that follow-up slice, so it's the first change that turns the pre-existing layout mismatch into an actual misrouted click.

The new topbar_route_segment_registers_interaction_target test (frame.rs:1798) only renders at width 160, wide enough that nothing sheds, so it can't catch this. Suggest deriving the route rect from the same kept-segment geometry shed_pass produces (expose it or an equivalent accessor from topbar.rs) and adding a narrow-width regression case.

Fix this →

False positives — disregard Codewhale-agent's two ERROR findings

Codewhale-agent flagged open_provider_picker as possibly unresolved in handlers.rs:1913 and apply.rs:1719 for lacking an explicit use. This doesn't hold up:

• crates/tui/src/tui/ui.rs:901-906 declares mod overlays; and pub(crate) use overlays::*; at the ui module root.
• Both handlers.rs and apply.rs open with use super::*;, which pulls in that glob re-export, so every pub(crate) item in overlays.rs — including open_provider_picker — already resolves unqualified.
• This is proven by the pre-existing code this PR replaces: the deleted branch in apply.rs already called open_onboarding_provider_picker(...) (also defined in overlays.rs) completely unqualified, and that compiled on main before this diff. The new open_provider_picker call sites follow the identical, already-working pattern.

No import is missing; no fully-qualified path is needed.

Everything else

• Provider/route safety: confirmed — both entry points only emit ViewEvent::TopbarRoutePickerRequested / call open_provider_picker, which delegates to the same picker/apply authority as /provider. No prefix-based or freeform-text provider inference anywhere in the diff.
• Reuse: genuine consolidation — apply.rs's OpenProviderPicker arm and the two render_topbar_row call sites' duplicated registration logic are properly deduplicated, not a new bridging layer.
• Tests: Codewhale-agent's two INFO items (no direct test of the TopbarRoutePickerRequested handler arm, no end-to-end F3→dispatch test) are reasonable but non-blocking given this codebase's existing granularity of unit coverage — the narrow-width hitbox gap above is the one gap worth closing.
• Security: no secret handling, shell/exec, or input-validation surface touched.

Assessment

Fix the shed/hitbox mismatch before merge — it's a real, user-visible correctness bug in the exact control this PR makes interactive. Everything else in the diff (provider/model/route safety, dedup, F3 gating behind onboarding/view-stack state) checks out.

[codewhale-agent]

Codewhale review

The PR makes the topbar route/model segment interactive by mouse and F3, and centralizes provider picker entry through a shared open_provider_picker helper. The structure is sound, but the new key routing path is under-tested and the refactor appears to drop an existing duplicate-modal guard.

Findings

• [WARNING] F3 event-loop path lacks a direct integration test (crates/tui/src/tui/ui/event_loop.rs:4410)
  The PR adds a non-trivial async branch in run_event_loop that converts F3 into ViewEvent::TopbarRoutePickerRequested, but the added tests only cover is_provider_route_shortcut and binding catalog strings. No test asserts that an F3 key event with an empty view stack emits the route-picker event or opens the picker. This is the primary new user-visible control, so it should be covered.
• [WARNING] Reused picker helper may drop the existing duplicate-modal guard (crates/tui/src/tui/ui/overlays.rs)
  The previous AppAction::OpenProviderPicker implementation would only push ProviderPickerView when app.view_stack.top_kind() != Some(ModalKind::ProviderPicker). The new open_provider_picker delegates to open_launch_provider_picker without an explicit equivalent guard. If open_launch_provider_picker does not already perform that check, repeated /provider invocations or a stale topbar route target can stack duplicate picker views.

Assessment

The refactor is logically clean and improves the topbar from decorative text to a typed control with shared entry points. Before merge, add direct event-loop coverage for F3 and confirm or restore the duplicate provider-picker guard.

---

Advisory review by Codewhale (codewhale review --pr 5763 --post, head d9bd021fe6d8339060a619101f9dba663c881d3e). 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.

[codewhale-agent]

[WARNING] F3 event-loop path lacks a direct integration test

The PR adds a non-trivial async branch in run_event_loop that converts F3 into ViewEvent::TopbarRoutePickerRequested, but the added tests only cover is_provider_route_shortcut and binding catalog strings. No test asserts that an F3 key event with an empty view stack emits the route-picker event or opens the picker. This is the primary new user-visible control, so it should be covered.