Skip to content

fix(dash): only act on key Press events in overlay dispatch - #89

Merged
michaelroy-amd merged 1 commit into
mainfrom
worktree-fix-model-select
Jul 13, 2026
Merged

fix(dash): only act on key Press events in overlay dispatch#89
michaelroy-amd merged 1 commit into
mainfrom
worktree-fix-model-select

Conversation

@michaelroy-amd

Copy link
Copy Markdown
Member

Problem

In both the bare-`rocm` launcher (focused Serve a model) and the full dash, you could not select a model in the serve wizard's recipe picker. Pressing Enter on a highlighted model re-opened the picker with that model pre-filled as the filter query instead of choosing it — so the model was never actually selected and the serve could not proceed.

Root cause

The dash event loop (crates/rocm-dash-tui/src/app/mod.rs) dispatches each operational overlay (serve wizard, engine manager, onboarding, …) directly to its on_key via if state.<overlay>.is_some() match arms. Those arms run before the general handle_key, which was the only place filtering out non-Press key events.

Terminals that also emit Release/Repeat key events (Windows Terminal / ConPTY under WSL, and any terminal with the kitty keyboard protocol) therefore delivered each keystroke to overlays twice:

  1. Press → picker chooses the model → picker closes, model field filled.
  2. Release/Repeat echo → focus is back on the Model field, so Enter re-opens the picker seeded with the just-chosen model id as the filter query.

The model picker itself was correct; the bug was purely in the event-loop wiring, which is why it hit both front ends (they share the loop).

Fix

  • Extract is_actionable_key(KeyEventKind) -> bool (Press only).
  • Add a top-priority match arm that swallows non-Press key events above every key arm, so the Press-only invariant now covers the overlay dispatch too.
  • Route handle_key through the same predicate (DRY with the existing release_events_are_ignored behavior).

The mouse path was audited and is immune to the same "one action → multiple events" class: resolve_mouse only acts on Down(Left) and the four scroll kinds. The only two terminal event-reading sites are this loop and the launcher loop (which already filtered to Press) — both are now covered.

Test plan

  • only_press_key_events_are_actionable (new) and release_events_are_ignored pass
  • cargo test -p rocm-dash-tui app-module suite (156 tests) green
  • cargo clippy -p rocm-dash-tui --all-targets -- -D warnings clean
  • Manual: in the launcher Serve a model flow and the dash serve wizard, Enter on a highlighted recipe selects it (verify on Windows Terminal / WSL where the double-fire reproduced)

The dash event loop dispatched each operational overlay (serve wizard,
engine manager, onboarding, …) straight to its `on_key` via
`if state.<overlay>.is_some()` arms that ran BEFORE the general
`handle_key` — the only place filtering non-Press key events. Terminals
that also emit Release/Repeat events (Windows Terminal / ConPTY under
WSL, kitty keyboard protocol) thus delivered each keystroke to overlays
twice.

For the serve wizard's model picker this meant Enter chose+closed the
picker on Press, then the Release/Repeat echo re-opened it seeded with
the just-chosen model as a filter — so the model could never be selected.
This hit both the bare-`rocm` launcher (focused Serve) and the full dash,
since both share this loop.

Extract `is_actionable_key(KeyEventKind)` (Press only), add a
top-priority arm that swallows non-Press key events above every key arm
so the Press-only invariant covers overlays too, and route `handle_key`
through the same predicate. Regression test:
`only_press_key_events_are_actionable`.

Signed-off-by: Michael Roy <michael.roy@amd.com>
@michaelroy-amd
michaelroy-amd enabled auto-merge July 8, 2026 22:54

@rominf rominf left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Reviewed the change and verified the behavior manually — the model picker now selects on Enter in both the launcher serve flow and the dash serve wizard. LGTM.

The diagnosis is right: the overlay dispatch arms in event_loop call their on_key directly and never filtered KeyEventKind, while the general handle_key already dropped non-Press — so overlays double-fired on terminals that emit a second event per keystroke. A single Press-only swallow arm above all key arms closes the gap, and routing handle_key through the same is_actionable_key predicate is a clean DRY-up. Nice test coverage with only_press_key_events_are_actionable.

I checked the obvious worry — that swallowing Repeat would break held-key auto-repeat (scrolling a job console, holding Backspace in a field). It doesn't: the app never pushes PushKeyboardEnhancementFlags, so the unix backend never emits Repeat; OS auto-repeat arrives as repeated Press and stays actionable; and the pre-existing handle_key already dropped non-Press, so overlays are just being brought in line with the rest of the UI.

A few optional, non-blocking nits:

  1. app/mod.rs:1671let _ = k; is redundant. k is already used by the guard, so an empty arm body (=> {}) compiles cleanly without an unused-binding warning.
  2. app/mod.rs:1670 — the Press-only invariant for overlays now relies on this arm staying first among the key arms; a future key arm inserted above it would silently re-introduce the double-fire. Not a problem today, just worth a note near the arm (which the comment already partly does).
  3. ui/launcher.rs:279 still hardcodes k.kind != KeyEventKind::Press rather than the new is_actionable_key. Fine as-is (separate module/loop), just means the "shared gate" is shared only within mod.rs.

None of these block. Approving.

@michaelroy-amd
michaelroy-amd added this pull request to the merge queue Jul 13, 2026
Merged via the queue into main with commit 0613cd2 Jul 13, 2026
15 checks passed
@michaelroy-amd
michaelroy-amd deleted the worktree-fix-model-select branch July 13, 2026 11:21
michaelroy-amd added a commit that referenced this pull request Jul 16, 2026
Reconcile PR #97 (configured-endpoint served-model discovery) with main's
PR #100 startup local-first detection (EAI-7347).

Conflicts in crates/rocm-dash-tui/src/app/{chat.rs,mod.rs} resolved as a
union that keeps both behaviors:

- Preserve main's local-engine detection: should_detect_local_chat,
  StartupChatOutcome/startup_chat_outcome, detect_local_chat_with_probe,
  and the #89 Press-only overlay key gate (is_actionable_key).
- Port PR #97's /v1/models discovery for a *configured* chat endpoint into
  a new helper, chat::discover_configured_chat_model, wired only on the
  StartupChatOutcome::Configured path.

Startup matrix (verified by unit tests in app::chat::tests):
- Configured URL + no explicit model + reachable -> adopt served /v1/models id.
- Configured URL + explicit model            -> config precedence, never overridden.
- Configured URL + unreachable               -> never probed, never replaced
  (no fetch timeout; an unreachable explicit CLI/env URL is left untouched).
- Local detected / OAuth paths unchanged from main.

RED before the port: configured_endpoint_adopts_served_model_when_no_model_set
failed against the merged-main stub (got "local-model", expected served id);
GREEN after wiring the helper. Full workspace tests, clippy -D warnings, and
scripts/smoke_local.py all pass.

Signed-off-by: Michael Roy <michael.roy@amd.com>
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