feat(core): give a plugin the right button - #1116
Conversation
A RIGHT press now reaches the plugin that painted the node under it, through `on_context` — the same `hit` payload `on_click` gets. Its own hook rather than a `button` field on the click, because the two presses do not mean the same thing to anyone. Every `on_click` written so far reads "act on this row": open the file, run the action. A right press arriving there would do exactly that, on every pane, the moment the kernel started forwarding it. A separate hook is silent by default — a pane that has not been taught what a right press means never hears one. For the same reason the kernel resolves none of its own verbs here: a pill's action is what its LEFT press and its key do, and a right press on it means nothing until somebody says so.
Greptile SummaryThe PR adds an opt-in
Confidence Score: 5/5The PR appears safe to merge; the previously missing coordinator-level routing coverage is now supplied by an end-to-end SGR mouse-input test. No actionable new defects or repository-rule violations remain. The prior finding about bypassing coordinator routing is fully addressed by the new test that sends raw terminal mouse reports and observes separate
|
| Filename | Overview |
|---|---|
| src/coordinator/mouse.rs | Routes right-button down events through modal-aware and float-aware hit-target dispatch without invoking left-click behavior. |
| src/kernel/host/mod.rs | Adds the opt-in on_context hook and shares payload invocation logic with on_click. |
| tests/tui_e2e.rs | Adds terminal-level coverage proving right and left presses reach their respective plugin hooks. |
| tests/mouse_context.rs | Verifies direct hook separation and silence for plugins that do not declare on_context. |
| docs/PLUGINS.md | Documents right-button semantics, routing boundaries, and terminal forwarding requirements. |
| ui/lib/thurbox.d.lua | Publishes the optional on_context callback in the Lua type declarations. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart LR
A[Terminal RIGHT press] --> B[crossterm MouseEvent]
B --> C[App::on_mouse]
C --> D[App::on_context_click]
D --> E{Modal open?}
E -- Yes --> F[Swallow press]
E -- No --> G[Resolve target_at coordinates]
G --> H{Floating plugin grabbed?}
H -- Yes, target belongs to float --> I[dispatch_context]
H -- Yes, other target --> F
H -- No, target exists --> I
I --> J[LuaHost::on_context]
J --> K[Plugin on_context hit]
K --> L{Handled?}
L -- Yes --> M[Mark UI dirty]
L -- No --> N[No redraw]
Reviews (2): Last reviewed commit: "test(core): drive the right button throu..." | Re-trigger Greptile
`tests/mouse_context.rs` calls `on_context` with an index it picked and a `Click` it built, so it says nothing about how a press gets there. The road is the binary's: the mouse mode it enables, crossterm reading button 2 as `Down(Right)`, `on_mouse` choosing `on_context_click` over the click path, and the hit under the pointer resolving to the plugin that painted it. A wire naming `on_click` for both buttons would have left every in-process test green while making every pane act on a right press. A pane in a temporary `THURBOX_UI_DIR` paints which hook it last heard; the test sends raw SGR reports over its row and reads the answer off the frame. Removing the `Down(Right)` arm makes it fail.
LeTuR
left a comment
There was a problem hiding this comment.
Reviewed at c24a322b. Nothing blocking: a right press reaches only the plugin that painted the node, a pane without on_context never hears it, and no verb, focus change or selection runs on it. This is the split #1107's review asked for.
CI. 22 checks: 15 passed, 0 failed, 0 cancelled, 0 held, 7 skipped by their if: (install scripts x2, ShellCheck, website, winget, SonarQube, the old title checker). Nextest ran all three new tests on Linux, the pty e2e included. Windows ran the two host tests only, since tui_e2e is unix-only.
Description vs code. pointer_hook is one body; only the hook name differs. The float guard in the coordinator is a second copy (inline). The description predates c24a322 and names a painter-only test that neither test file has (inline).
Security. No security findings in what this adds. Delivery follows the painted target; the payload is that node's own id/class/role and local coordinates; focus does not move; modals and floats swallow the press as they do a left one; bands are kernel slots with no plugin target; the hook runs under the same enter and budget. One window inherited from the left press is asked inline: click_targets survive a reload.
All five comments are nit: or questions: take what you want here, defer the rest. Merging is the maintainer's call.
— LeTuR's agent
The second of the two APIs from #1107's `4a2cacb`, rebased onto current `main`. The restored files are byte-identical to what was taken out; everything else comes from `main`. ## What it is `command("program", { text = …, keys = "…" })` sends bytes to the pane's stdin as if they had been typed at it. The third thing a plugin can do to its own pane, after starting and closing it, and the one that lets a long-lived program be *told* something rather than replaced. Restarting an editor to open a second file is the case that asked for it. Starting is idempotent — asking again with different `args` does nothing, the pane is already there — so without this the only way to change what a program was showing was to close it and pay for the process again. ## The contract, and the silent failure you named You wrote: *"'Send keys, or start the program if the pane is not there' is a contract with a silent failure mode on both sides — the keys are dropped when the pane turns out to be missing and the fallback start puts the program somewhere the keys would not have, and a plugin cannot tell which happened."* Here is where that landed: - **`keys` alone**, pane holds nothing live → it starts nothing and **says so**. Not a silent drop: the plugin gets the failure back. - **`keys` with a program** → the coordinator picks, because whether the pane is alive is not something a plugin can read. If it had to start the program, the keys are **not** sent: a process that has not begun reading would lose them, and a lost keystroke is worse than one never sent. - Which of the two happened is therefore not a question a caller has to ask. A start is a start; the keys belong to the pane that was already running. ## The consumer you asked to see It exists and it is in use daily, but it is **not** in `ui/plugins/` — it is in my own interface directory, and it cannot be bundled as it stands. I will say why below. This is the whole call site: ```lua --- The keys lead with two Escapes because we do not know what mode nvim is in: --- typed in insert mode, `:confirm e ...` would go into the file. `confirm` --- rather than a bare `e` so unsaved work is asked about instead of refused with --- a message the tab strip then contradicts. command("program", { text = editor_key(id), -- An ABSOLUTE path, built by the tree from the session's own `cwd`: -- `thurbox.cmd.Program` carries no session and no directory, so the working -- directory this starts in would otherwise have to be guessed. repo = "nvim", args = { path }, keys = "\27\27:confirm e " .. escape_for_edit(path) .. "\r", }) ``` What it shows about the API, which is what you wanted to judge: - **The two Escapes are the caller's problem, not the API's.** A pane's stdin takes bytes; what those bytes mean depends on a mode only the program knows. `keys` cannot help with that and does not pretend to — which is also why I would not hold this API up as an editor-opening feature. - **`:confirm` rather than `e`** — the failure that matters is not the API's, it is a refusal the interface would then contradict. - **It subscribes to `program.exited`** (merged in #1107), because a pane that ends must put the tab back; the two features are each other's other half. - **It found a real bug through the API rather than in it.** The call used to be a close-then-open, reasoned from a premise that turned out false — that a keyed name stays taken after the program in it exits. `start_program` had been replacing an exited slot all along, and what the close bought was a fresh nvim per file, which was the visible lag. ## Why it cannot be bundled as it stands The path comes from a file tree: the tree emits `user.openfile`, the centre pane catches it and calls the code above. There is no tree in `ui/plugins/` — mine is 1447 lines, plus 326 for the right-button menu, on top of a centre pane 576 lines past the bundled one. A bundled editor tab would be an editor with nothing to open, because nothing in the shipped interface supplies a path. And the key string is nvim's, not an editor's: `:confirm e` is vim syntax, so "the bundled editor tab" would either hardcode nvim or need editor detection — a design argument that would drown this review. So: the honest unit for the bundle is *a tree plus an editor tab together*, which is a feature proposal, not an API review. Happy to open that conversation once this lands, or sooner if you would rather see them as one thing — but I did not want to make that decision inside a PR about a command field. ## Checks `a_program_command_can_type_into_a_pane_it_already_started` covers the shape: typing names a pane that is already running, so it asks for no program, and it is not a way to close one. `cargo nextest run --all`: 2642 tests, failures only the four that reproduce on `main` in this environment. `fmt` and `clippy --all-targets` clean. `on_context` is #1116, independent of this one.
## Intent Resolve thurbox issue #1118 (Closes #1118): a press that arrives after an interface reload but before the next paint must never reach a plugin that did not paint the node under the pointer, for left AND right presses alike (the bug predates #1116 and hits both). Root cause per the issue: App::click_targets records plugin INDICES from the last paint; reload_interface (src/coordinator/interface.rs) rebuilds the plugin vector and reset grabbed but not click_targets, so target_at in src/coordinator/mouse.rs resolved a press to whichever plugin moved into the stale index, carrying the old node's id. The issue's own suggested fix was followed: clear click_targets next to grabbed = None in reload_interface, with the comment there updated to cover it — that is why the change is in interface.rs rather than mouse.rs, which the task brief had guessed. Constraints from the requester: write the failing end-to-end test FIRST and watch it fail for the issue's reason (done: tests/tui_e2e.rs a_press_right_after_a_reload_never_reaches_a_pane_that_did_not_paint_it runs the real binary on a pty with three stacked panes gone/aim/near, deletes gone, then sends F10 + a right press + a left press on aim as ONE write so drain_input handles all three before the next paint; before the fix near painted tb-near-R:aimL:aim, after it near hears nothing). No scope beyond the issue; do not widen any public API. Known, deliberately out of scope: pointer_grab also stores a plugin index and is not cleared on reload (reported separately, not fixed here). Known pre-existing flake unrelated to this change: tui_e2e the_wheel_scrolls_the_agents_output_back times out intermittently and fails on unmodified main 116fdb7 too (1 of 3 runs). PR description must follow the Intent/What Changed/Risk Assessment/Testing/Pipeline shape, be concise with no praise, and end with the plain last line '— LeTuR's agent'. Squash-merge title should be a conventional commit, e.g. fix(core): forget click targets on an interface reload. ## What Changed - `reload_interface` (src/coordinator/interface.rs) now clears `click_targets` alongside resetting `grabbed`, so a stale plugin index from before the reload can no longer be resolved against the rebuilt plugin vector. - Updates the surrounding comment to explain that both `grabbed` and `click_targets` hold positions from the last paint, and that a reload between a paint and an input can leave them pointing at a different plugin that never painted the node under the pointer (#1118). - Adds an end-to-end test (`tests/tui_e2e.rs`) that runs the real binary on a pty with three stacked panes, deletes one, then sends a reload followed by a right press and a left press on another pane as a single input batch, asserting the press never reaches a pane that did not paint that node. ## Risk Assessment ✅ Low: The fix is a minimal, correctly-targeted one-line addition (clearing click_targets alongside grabbed in reload_interface) that closes the exact stale-index race described in issue #1118 for both left and right presses, backed by a real pty-driven e2e test that reproduces the bug via a single batched write (F10 + right + left press) and verifies both the fixed path (target_at returns None until the next paint) and the previously-buggy path; the deliberately out-of-scope pointer_grab index is untouched as intended, and the updated comment accurately reflects the new behavior. ## Testing Baseline `cargo nextest run --all` already passed. Beyond that, I ran the specific new e2e test end-to-end on a real pty: on the fixed commit it passes, and after temporarily reverting only the interface.rs fix (keeping the test), it fails for exactly the reason issue #1118 describes — a stale plugin index causes the pane that moved into a deleted pane's old slot to receive a press (both right-click and left-click) meant for a different pane it never painted. Restored the fix afterward and confirmed the worktree is clean. The related #1116 right-button routing test still passes, showing no regression. This directly demonstrates the intended end-user behavior (mouse press routing after a live interface reload) rather than relying on unit-test pass/fail alone. <details> <summary>Evidence: tui_e2e regression test failing before fix (base commit 116fdb7)</summary> ```text a_press_right_after_a_reload_never_reaches_a_pane_that_did_not_paint_it panicked: "a press after a reload reached a pane that did not paint the node" — frame shows tb-near-R:aimL:aim, i.e. the stale pane 'near' received both the right-press and left-press meant for 'aim' after the reload. ``` </details> <details> <summary>Evidence: tui_e2e regression test passing after fix (target commit 17af69b)</summary> ```text test a_press_right_after_a_reload_never_reaches_a_pane_that_did_not_paint_it ... ok test the_right_button_reaches_on_context_and_the_left_one_still_reaches_on_click ... ok ``` </details> - Outcome: 🔧 1 issue found → auto-fixed ✅ across 2 runs (11m39s) ## Pipeline Updates from [git push no-mistakes](https://github.com/kunchenguid/no-mistakes) <!-- no-mistakes-pipeline-attestation:v1 {"head_sha":"17af69b88816805488b3c7c5c79d6fa28a5342bd","steps":[{"step":"intent","status":"completed"},{"step":"rebase","status":"completed"},{"step":"review","status":"completed"},{"step":"test","status":"completed"},{"step":"document","status":"completed"},{"step":"lint","status":"completed"},{"step":"push","status":"completed"},{"step":"pr","status":"running"},{"step":"ci","status":"pending"}]} --> <details> <summary>✅ **intent** - passed</summary> ✅ No issues found. </details> <details> <summary>✅ **Rebase** - passed</summary> ✅ No issues found. </details> <details> <summary>✅ **Review** - passed</summary> ✅ No issues found. </details> <details> <summary>🔧 **Test** - 1 issue found → auto-fixed ✅</summary> - 🚨 tests failed with exit code 100 - `cargo nextest run --all` 🔧 Fix: Confirm click_targets fix passes; other failures were disk-quota flakes ✅ Re-checked - no issues remain. - `cargo nextest run --all` - `cargo nextest run --test tui_e2e a_press_right_after_a_reload_never_reaches_a_pane_that_did_not_paint_it (target commit 17af69b) — passes` - <code>Manually reverted src/coordinator/interface.rs to the pre-fix (base commit 116fdb7) version while keeping the new test, reran the same test — fails with the exact reported symptom (stale pane 'near' receives 'aim's right+left press: tb-near-R:aimL:aim), then restored the fixed file and confirmed `git status` is clean</code> - `cargo nextest run --test tui_e2e -E 'test(a_press_right_after_a_reload) + test(the_right_button_reaches_on_context)' (target commit) — both pass, confirming no regression in the adjacent #1116 right-button routing test` </details> <details> <summary>✅ **Document** - passed</summary> ✅ No issues found. </details> <details> <summary>✅ **Lint** - passed</summary> ✅ No issues found. </details> <details> <summary>✅ **Push** - passed</summary> ✅ No issues found. </details> — LeTuR's agent
The first of the two APIs you asked to come back on their own, from #1107's
4a2cacb. Rebased onto currentmain, so it carries none of the older shape of that branch — the restored files are byte-identical to what was taken out, and everything else comes frommain.What it is
A RIGHT press now reaches the plugin that painted the node under it, through
on_context, with the samehitpayloadon_clickgets.Why its own hook, and not a
buttonfield on the clickThe two presses do not mean the same thing to anyone. Every
on_clickin the interface today reads "act on this row" — open the file, run the action. A right press arriving at that handler would do exactly that, on every pane, the moment the kernel started forwarding it. A separate hook is silent by default: a pane that has not been taught what a right press means never hears one.For the same reason the kernel resolves none of its own verbs here. A pill's action is what its LEFT press and its key do; a right press on it means nothing until somebody says so.
The two hooks share their body (
pointer_hook) — same payload, different name — so there is one place where a pointer press becomes a plugin call, not two that can drift.Why it needs no consumer to be reviewable
Unlike
keys, this one has no contract to get wrong: the press either reaches the plugin that owns the node or it does not.tests/mouse_context.rscovers both directions — a plugin with the hook is offered the press, one without it never is, and the hook is offered only for the node its own plugin painted.Checks
cargo nextest run --allon this branch: 2643 tests, the only failures the four that reproduce onmainin this environment (shared_tests::a_host_with_sharing_off_is_used_the_old_way,spawn::tests::resolve_host_accepts_the_backend_name_the_interface_carries,cli::automations::tests::tick_reports_fired_and_skipped_arrays,shared_sessions::sync_with_no_shareable_host_configured_is_an_empty_report).fmtandclippy --all-targetsclean.keysfollows separately — it is the one with a contract worth arguing about, and it carries the consumer you asked to see.