Skip to content

bug(tui): clicking a path:line in tool output spawns $EDITOR detached, with the TUI still owning the terminal #6235

Description

@Hmbown

Split out of #6165, which fixed the /hooks edit half and explicitly flagged this one as "Related path with the same defect". Filing it so closing #6165 does not bury it.

What happens

try_open_file_at_line (crates/tui/src/tui/history.rs:2937, reached from a mouse click on a path:line in tool output via crates/tui/src/tui/mouse_ui.rs:1680) runs:

Command::new(&editor)
    .arg(format!("+{line_num}"))
    .arg(&abs_path)
    .spawn()

spawn(), not status(). The child inherits the tty and is never waited on. The TUI does not suspend anything — not raw mode, not the alternate screen, not mouse capture, not the input pump — and goes right on rendering. Its fallback editor is vim, so the default configuration is the broken one, same as #6165.

Why #6165's fix does not cover it

#6165 moved the input-pump pause inside external_editor::with_suspended_tui, so every caller of that function is now correct by construction. This path does not call it. It does not call anything in external_editor at all — it builds its own Command and its own $VISUAL/$EDITOR/vim resolution, duplicating resolve_editor().

Why it needs a different shape, not just a pause

A pause has to end somewhere, and there is nothing here to end it: the function returns as soon as the child is spawned, so there is no point at which the TUI knows the editor exited. The loop can also spawn several editors, one per matching line in the clicked text. Whatever lands has to decide between:

  • suspend and wait, one editor at a time, through with_suspended_tui (and reuse resolve_editor), or
  • keep it fire-and-forget but refuse for terminal editors, i.e. only launch when the resolved editor detaches from the tty (code, subl, a GUI $VISUAL), and say so when it does not.

The second is probably what a click wants — a click should not swallow the session — but it is a product decision, not a mechanical one.

Also worth deciding in the same slice: spawning N editors from one click is unlikely to be intended.

Not verified on a live terminal

Read from source while fixing #6165; I did not reproduce it against a real tty. The reporter of #6165 reproduced the sibling defect on a PTY, and this path is strictly worse (it suspends nothing at all rather than suspending crossterm state only), so the failure should be at least as bad.

Related: #6165 (fixed), and its "companion defect" note about SIGTSTP/SIGTTIN — all three are the same missing invariant: terminal ownership is decided per call site.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    bugSomething isn't working

    Projects

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions