fix: clickable glyphs replace unreachable right-click, agent-level counts, no false running expiry - #7
Conversation
marktoda
left a comment
There was a problem hiding this comment.
Thanks Daniel — the hotspot work here is genuinely excellent. I traced the Line-borne metadata through every render transformation (paint, overflow fold, height clamp, badge/ledger paths) and the lockstep claim holds structurally at each one; the coordinate e2e is exactly the layer we were missing. The agent-level presence counts also check out against every consumer, and the rail-reference edits are properly wired through the executable spec. Two of the three fixes are ready as-is.
The third one has a confirmed problem that I think blocks merge in its current form, plus a handful of smaller items.
Blocking
is_shell_prompt's strong-evidence condition is unsatisfiable under real Zellij — the exit-clear becomes dead code
crates/core/src/command.rs — the new condition requires a shell name and is_foreground == true. But in Zellij 0.44.3, is_foreground doesn't mean "this process is in the foreground"; it means "the pane's root process currently has a child." From zellij-server/src/pty.rs#L2138-L2160 at v0.44.3:
let foreground_cmd: Vec<String> = process_id
.and_then(|pid| ppids_to_cmds.get(&pid.to_string())) // children of the pane's root
...
let (command, is_foreground) = if foreground_cmd.is_empty() {
let shell_cmd = self.terminal_cmds.get(terminal_id)...;
(shell_cmd, false) // shell at prompt → ALWAYS false
} else {
(foreground_cmd.clone(), true) // true only while a child exists
};A shell sitting at its prompt (childless root) is always reported as (shell_cmd, is_foreground=false). (shell-name, true) would require a shell running as a child of the pane's root shell — which never happens on a normal prompt return. Consequences:
clear_on_prompt_returnnever runs, so the stale-Running grace clock never arms andexpire_stale_runningis unreachable. A SIGKILLed claude spins as Running forever — the ghost is unbounded, not "until the pane's next real prompt signal," because once the pane is at the prompt no furtherCommandChangedfires for it.- The same gate guarded the immediate exit-clear of terminal statuses (Pending/Done/Error → idle + ledger recede), which the PR text and tests don't mention. Codex's adapter has no session-end event, so quitting Codex at a Pending "needs you" row now leaves it climbing
· Nmindefinitely;notify genericDone cards never recede to the ledger. own_presence(this PR) counts exactly these status-origin panes, so the ghost counts republish to every peer session's badge on each heartbeat, and a ghost Running row keeps the Fast render cadence armed permanently.
One factual correction to the comment/PR text: the weak-evidence prompt signal isn't "hypothesized, not live telemetry" — commit 47e2fbd in this repo recorded it from a live session ("Zellij reports a pane whose root has no child process as back at the shell prompt — CommandChanged with is_foreground=false"), and that signal is precisely what the production exit-clear has been firing on all along.
The unit suite can't see this because it synthesizes (["zsh"], true) events that real Zellij never emits for a pane's own shell.
Suggested direction: don't collapse the evidence into one bool. The event gives you a usable tri-state: (shell-name, true) = strong (keep); (shell-or-anything, false) with the reported command being the pane's root/shell = the genuine at-prompt signal (this must keep clearing — it's the only prompt-return Zellij ever sends); (non-shell argv, false) = the weak wrapper/child transition you're rightly trying to exclude from arming the clock. If the field mechanism was a mid-turn is_foreground=false with a wrapper argv, gating only that case removes the false-expiry class without killing the exit-clear. An e2e that kills an agent and waits for the clear would pin whichever policy we land on.
Worth addressing
render.rs(append_hotspot_glyph+ 3 call sites): the reserve-2-cells-or-drop dance is copy-pasted at three sites (>= 6/>= 8/>= 4) and the contract is enforced only by adebug_assertthat compiles out of the release wasm. If a future site pads to full width without reserving, the release line exceedswidthand can wrap — a lockstep break of exactly the class the repo requires to be structural (Line::newenforces its sibling invariant in release; this one doesn't). One helper owning reservation + attachment, parameterized on the per-site minimum, would close it.runtime.rsmouse_clickDismissPresence arm: verbatim copy ofmouse_right_click's session branch (same stale re-check, samesessions.dismiss, same effect). Since right-click is deliberately kept for the zellij#5350 follow-up, extractdismiss_stale_session(&mut self, name) -> Outcomeso the "never race away a healthy peer" rule lives once.rollup.rsPrimaryDetail.origin/.acknowledged: dead in production — every consumer goes throughPaneDisplay::Tracked(has_unacknowledged_status_pending,is_status_origin); the only reader is the render-testdisplay()helper copying constants. Keeping them onPaneDisplay::Trackedonly is a bit-identical shrink.render.rs:696bell suppression: the ✓ now unconditionally drops the ⚑, buthas_bellis tab-level — a BEL from a different pane in the tab is hidden for the whole unacknowledged-Pending window. Geometrically both fit (content is already clamped towidth - 2;…⚑ ✓is legal at any width ≥ prefix+4), so consider rendering both rather than making the glyph eat an independent signal.
Minor
- A click on a glyph cell whose action raced away (converged in the same event batch, before repaint) returns zero effects instead of falling through to row navigation — a one-batch-wide dead click. Probably fine (silently navigating when the user aimed at an action is arguably worse), but worth a comment stating the choice.
- The new e2e derives the click column from
chars().position('✓')— a char index used as a screen column. Correct today (all preceding chars are width-1), but any width-2 char (CJK/emoji inmsg) left of the glyph breaks it, since vt100 emits one char for two cells. Accumulating display width would make it robust. - The badge ✕ is always painted in the extra-dim stale blend, so on a
selectedstale entry (fresh selection lapsing mid-pending, ~1–2s window) the label is bold accent while its action glyph is near-invisible. Cosmetic.
Verification run: 8-angle finder pass + independent adversarial verification per finding; 4 confirmed, 4 plausible, everything else (hotspot lockstep, presence counts, conventions, rail-reference wiring) came back clean.
c9731e5 to
f66ac6e
Compare
|
Thanks for the detailed review. I addressed the requested changes. I also: centralized hotspot admission, width reservation, attachment, and release-build validation in HotspotSlot All current checks pass on Linux, macOS, MSRV, the hermetic environment, and the live E2E suite. Would you mind taking another look? |
Three fixes from daily use of v0.2.0, one of them upstream-blocked.
The right-click actions shipped in v0.2.0 turn out to be unreachable: zellij 0.44.3 never delivers Mouse::RightClick to plugins. The forwarder exists in zellij's plugin pane but nothing calls it; I filed zellij-org/zellij#5350 with the trace. This PR replaces the trigger with click hotspots that work today: a dimmed (stale) session row in the badge gets an x glyph at its right edge that dismisses it, and a row with an unacknowledged pending pane gets a checkmark that acknowledges it. The checkmark is deliberately not an x, so nobody reads it as closing the pane. Left-click anywhere else keeps navigating, and the right-click handlers stay in place. I've sent zellij a fix wiring right-click delivery to plugin panes (zellij-org/zellij#5353); once that lands in a release, a follow-up PR restoring right-click as a second trigger for these actions can be considered. The hotspot metadata lives inside Line itself and rides through every render transformation structurally, so the click map and the glyph cells cannot drift apart. There is a new e2e that clicks the glyph's exact coordinates against a real zellij, which is the test layer that would have caught the right-click gap before release.
Second, presence counts now count agents instead of tabs. Three working agents in one tab previously reported running: 1. The counts are live status-origin panes only, command-origin activity is excluded, and the local rail summaries deliberately stay tab-level. Docs state the semantics.
Third, actively working agents no longer time out of the rail mid-turn. The stale-Running grace clock could be started by weak evidence: is_foreground == false was treated as a prompt return regardless of what the foreground actually was, so an ordinary wrapper or child transition in a live agent's pane could start the 15s expiry while the agent thought for minutes without emitting a hook. The clock now starts only on strong evidence, a genuine shell in the foreground with is_foreground true. To be plain about certainty: the weak-evidence sequence is pinned by a regression test but was reconstructed, not captured from live telemetry, so it is the probable rather than proven field mechanism. The policy change removes the whole class either way, at the documented cost that a killed agent can ghost as Running until the pane's next real signal. If timeouts still show up in the field after this, the remaining suspects are written up in the fix's comments.
While debugging this line of issues I also hit a zellij server crash worth knowing about as a zj-radar user: a failed start-or-reload-plugin against a client-less session leaves pending-plugin state that starves the rail and leaks server FDs until the server dies of EMFILE. Filed as zellij-org/zellij#5349; nothing in this PR depends on it, but the CliPipe timeout floods it causes are easy to misattribute to the plugin.
500 host tests, clippy -D warnings, wasm build, just ci, and 13/13 e2e all pass.