fix(maestro): make the TUI produce the input it already parses #48

Merged
naps62-yolo merged 1 commits from lithuanians into main 2026-08-22 15:03:38 +01:00
Collaborator

The keybinding parser validated strings, not whether any producer could emit the event. So config.rs shipped ("mouse:dblclick(sidebar-row)", "AttachHighlighted") as a default binding that could never fire — and nothing failed, because InputKey::parse was happy with it.

MouseKind::DblClick appeared only in the enum and in parse/format; canonicalise emits Down/Drag/ScrollUp/ScrollDown and nothing else. Adds a ClickTracker that promotes a second click at the same cell within 400ms.

canonicalise stays a pure function of one event — it is well tested and making it stateful for this would have been a shame. The tracker is separate and takes now: Instant as a parameter, so the timing rule is tested without sleeping: fast-second-click promotes, slow one doesn't, different cell doesn't, and a promoted click resets the tracker so three rapid clicks are click / double / click rather than two doubles.

Region::Sidebar and Region::SplitDivider are deleted. region_at can only return Modal, Tab, SidebarRow, Pane or Unknown, so the parser was accepting region names the renderer cannot produce. It now rejects them, with a test that pins exactly that.

Shift-Tab is wired to overlays::prev_field, which existed with no production caller — the spawn modal could only be cycled forwards. crossterm reports Shift-Tab as KeyCode::BackTab, not a modified Tab (my first attempt got this wrong), so InputKey gains a BackTab variant which also forwards \x1b[Z to the pane in Insert mode.

Left alone deliberately: Action::FocusPane(Direction) parses, displays and tests a direction the dispatcher discards. The review flagged it, but that is v1 having one pane per tab — a documented limitation, not a defect. Rejecting directions would break configs for no gain.

Verification
cargo fmt --all -- --check                   ok
cargo clippy --all-targets -- -D warnings    0
cargo test --all                             317 passed, 0 failed
pnpm -C web test                             36 passed

Ninth and last of the architecture review series (#40–#47 merged).

The keybinding parser validated *strings*, not whether any producer could emit the event. So `config.rs` shipped `("mouse:dblclick(sidebar-row)", "AttachHighlighted")` as a **default binding that could never fire** — and nothing failed, because `InputKey::parse` was happy with it. **`MouseKind::DblClick`** appeared only in the enum and in parse/format; `canonicalise` emits `Down`/`Drag`/`ScrollUp`/`ScrollDown` and nothing else. Adds a `ClickTracker` that promotes a second click at the same cell within 400ms. `canonicalise` stays a pure function of one event — it is well tested and making it stateful for this would have been a shame. The tracker is separate and takes `now: Instant` as a parameter, so the timing rule is tested without sleeping: fast-second-click promotes, slow one doesn't, different cell doesn't, and a promoted click resets the tracker so three rapid clicks are click / double / click rather than two doubles. **`Region::Sidebar` and `Region::SplitDivider`** are deleted. `region_at` can only return `Modal`, `Tab`, `SidebarRow`, `Pane` or `Unknown`, so the parser was accepting region names the renderer cannot produce. It now rejects them, with a test that pins exactly that. **Shift-Tab** is wired to `overlays::prev_field`, which existed with no production caller — the spawn modal could only be cycled forwards. crossterm reports Shift-Tab as `KeyCode::BackTab`, not a modified `Tab` (my first attempt got this wrong), so `InputKey` gains a `BackTab` variant which also forwards `\x1b[Z` to the pane in Insert mode. **Left alone deliberately:** `Action::FocusPane(Direction)` parses, displays and tests a direction the dispatcher discards. The review flagged it, but that is v1 having one pane per tab — a documented limitation, not a defect. Rejecting directions would break configs for no gain. <details> <summary>Verification</summary> ``` cargo fmt --all -- --check ok cargo clippy --all-targets -- -D warnings 0 cargo test --all 317 passed, 0 failed pnpm -C web test 36 passed ``` </details> **Ninth and last** of the architecture review series (#40–#47 merged).
naps62-yolo added 1 commit 2026-08-22 14:59:40 +01:00
fix(maestro): make the TUI produce the input it already parses
ci / web (pull_request) Successful in 26s
ci / rust (pull_request) Successful in 2m30s
ci / web (push) Successful in 27s
ci / rust (push) Successful in 2m10s
745e9fde8b
The keybinding parser validated strings, not whether any producer could
emit the event — so the shipped default `mouse:dblclick(sidebar-row)`
could never fire, and nothing failed.

`MouseKind::DblClick` appeared only in the enum and in parse/format;
`canonicalise` emits Down/Drag/ScrollUp/ScrollDown. Adds a `ClickTracker`
that promotes a second click at the same cell within 400ms, keeping
`canonicalise` a pure function of one event — the tracker takes `now` as a
parameter, so the timing rule is tested without sleeping.

`Region::Sidebar` and `Region::SplitDivider` are deleted: `region_at` can
only return Modal, Tab, SidebarRow, Pane or Unknown, so the parser was
accepting names the renderer cannot produce. It now rejects them.

Shift-Tab is wired to `overlays::prev_field`, which existed with no
caller, leaving the spawn modal cycleable in one direction only. crossterm
reports it as `KeyCode::BackTab` rather than a modified Tab, so `InputKey`
gains a `BackTab` variant that also forwards `\x1b[Z` to the pane.

Left alone: `Action::FocusPane(Direction)` discards its direction, but
that is v1 having one pane per tab, not a defect.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@@ -291,0 +318,4 @@
row,
} = key
else {
return key;
Author
Collaborator

A drag leaves last intact, so a click, drag, and another click at the original cell within 400 ms becomes dblclick. Clear the pending click when a drag (or any intervening mouse event) arrives.

A drag leaves `last` intact, so a click, drag, and another click at the original cell within 400 ms becomes `dblclick`. Clear the pending click when a drag (or any intervening mouse event) arrives. <!-- agent-meta: {"model":"gpt-5.6","session":"01a029c5"} -->
naps62-yolo merged commit 745e9fde8b into main 2026-08-22 15:03:38 +01:00
naps62-yolo deleted branch lithuanians 2026-08-22 15:03:38 +01:00
Author
Collaborator

Good catch — that's a real bug and it was mine.

promote() early-returned for any non-click and left last intact, so click -> drag -> click at the same cell inside the window came back as dblclick. A scroll or a keystroke between the two did the same thing.

Fixed in #49: any non-click input clears the pending click. Three regression tests cover drag, keystroke and scroll.

One trade-off I'd rather state than hide: a double-click where the mouse jitters enough for the terminal to emit a Drag between the two downs now registers as two clicks. That is the right way round — a spurious dblclick fires AttachHighlighted and opens a session, whereas a missed one costs a second click.

Good catch — that's a real bug and it was mine. `promote()` early-returned for any non-click and left `last` intact, so click -> drag -> click at the same cell inside the window came back as `dblclick`. A scroll or a keystroke between the two did the same thing. Fixed in #49: any non-click input clears the pending click. Three regression tests cover drag, keystroke and scroll. One trade-off I'd rather state than hide: a double-click where the mouse jitters enough for the terminal to emit a `Drag` between the two downs now registers as two clicks. That is the right way round — a spurious `dblclick` fires `AttachHighlighted` and opens a session, whereas a missed one costs a second click.
This repo is archived. You cannot comment on pull requests.