diff --git a/desktop/src-tauri/src/terminal_runtime.rs b/desktop/src-tauri/src/terminal_runtime.rs index 0e0123aa9..87f969592 100644 --- a/desktop/src-tauri/src/terminal_runtime.rs +++ b/desktop/src-tauri/src/terminal_runtime.rs @@ -277,12 +277,11 @@ impl Session { } fn shutdown(mut self) { - if let Ok(mut channel) = self.channel.lock() { - *channel = None; - } // Publication is detached before the reader enters close mode; the // lifecycle helper then abandons parser work and keeps raw-draining - // through child termination and reap. + // through child termination and reap. Keep the renderer channel alive + // until the reader has reported Exit; close() removes the session from + // the runtime before shutdown begins, so this is its final message. if let Ok(mut publisher) = self.publisher.lock() { publisher.close(); } @@ -300,6 +299,9 @@ impl Session { reader.join(); } } + if let Ok(mut channel) = self.channel.lock() { + *channel = None; + } } } diff --git a/desktop/src/features/terminal/TerminalBootstrap.test.mjs b/desktop/src/features/terminal/TerminalBootstrap.test.mjs index df73cd89e..7eb5ffcb9 100644 --- a/desktop/src/features/terminal/TerminalBootstrap.test.mjs +++ b/desktop/src/features/terminal/TerminalBootstrap.test.mjs @@ -319,6 +319,43 @@ test("opening a tab keeps terminal ownership while its attachment is pending", a view.unmount(); }); +test("a successful close removes the tab even if the exit event is lost", async () => { + const { createElement } = await import("react"); + const { fireEvent, render, waitFor } = await import("@testing-library/react"); + const { ThemeProvider } = await import("@/shared/theme/ThemeProvider"); + const { TerminalBootstrap } = await import("./TerminalBootstrap.tsx"); + + const view = render( + createElement( + ThemeProvider, + null, + createElement("div", { + className: "buzz-huddle-app-surface", + tabIndex: -1, + }), + createElement(TerminalBootstrap, { + channelId: "channel-1", + channelName: "general", + npub: "npub1owner", + relayUrl: "wss://relay.example", + threadId: null, + }), + ), + ); + await waitFor(() => + assert.ok(calls.some(({ command }) => command === "terminal_attach")), + ); + await waitFor(() => assert.ok(view.queryByRole("tab", { name: /SHELL/ }))); + + fireEvent.click(view.getByLabelText("Close SHELL")); + + await waitFor(() => + assert.ok(calls.some(({ command }) => command === "terminal_close")), + ); + await waitFor(() => assert.equal(view.queryByRole("tab"), null)); + view.unmount(); +}); + // The wheel-to-IPC path end to end. `TerminalSubstrate` already proves it // accumulates pixels into whole cells and `buzz-terminal` already proves which // way the engine goes; the seam between them is this file's business, and the diff --git a/desktop/src/features/terminal/TerminalBootstrap.tsx b/desktop/src/features/terminal/TerminalBootstrap.tsx index acd822dcf..20905e9fb 100644 --- a/desktop/src/features/terminal/TerminalBootstrap.tsx +++ b/desktop/src/features/terminal/TerminalBootstrap.tsx @@ -228,7 +228,10 @@ export function TerminalBootstrap({ removeSession(key); return; } - void connection.close().catch(fail); + void connection + .close() + .then(() => removeSession(key)) + .catch(fail); }} onFrameConsumed={(frame) => { const delivery = sessionsRef.current.find( diff --git a/desktop/src/features/terminal/TerminalSubstrate.test.mjs b/desktop/src/features/terminal/TerminalSubstrate.test.mjs index 6b4fb28b9..2f9e24eff 100644 --- a/desktop/src/features/terminal/TerminalSubstrate.test.mjs +++ b/desktop/src/features/terminal/TerminalSubstrate.test.mjs @@ -65,6 +65,7 @@ after(() => dom.window.close()); beforeEach(() => { cleanup?.(); reducedMotion = false; + paintLog.length = 0; dom.window.localStorage.clear(); dom.window.HTMLCanvasElement.prototype.getBoundingClientRect = () => ({ bottom: 782, @@ -77,18 +78,50 @@ beforeEach(() => { y: 0, toJSON() {}, }); - dom.window.HTMLCanvasElement.prototype.getContext = () => ({ + dom.window.HTMLCanvasElement.prototype.getContext = function () { + return paintRecorder(this); + }; +}); + +// The default stub discards everything it is handed, so an assertion made +// against it cannot distinguish "repainted" from "drew nothing". This records +// every draw call against the canvas that received it, so the tests below are +// about commands actually issued and can exclude the banner canvas. +const paintLog = []; + +function paintRecorder(canvas) { + return { clearRect() {}, - fillRect() {}, + fillRect(x, y, width, height) { + paintLog.push({ + canvas, + height, + kind: "fill", + style: this.fillStyle, + width, + x, + y, + }); + }, fillStyle: "", - fillText() {}, + fillText(text, x, y) { + paintLog.push({ canvas, kind: "text", text, x, y }); + }, font: "", restore() {}, save() {}, setTransform() {}, textBaseline: "", - }); -}); + }; +} + +/** Draws issued to the grid canvas only, newest paint pass included. */ +function gridDraws(view) { + const grid = view.container.querySelector( + ".buzz-terminal-viewport > canvas:not(.buzz-terminal-welcome)", + ); + return paintLog.filter((entry) => entry.canvas === grid); +} function fixture(overrides = {}) { const calls = { input: [], scroll: [] }; @@ -171,6 +204,30 @@ test("mounted IME paths neither toggle nor emit preedit text", async () => { assert.deepEqual(calls.input, ["か"]); }); +test("tab actions restore terminal input focus", async () => { + const { view } = fixture(); + await ready(view); + toggleChord(); + const input = view.getByLabelText("Terminal input"); + await waitFor(() => assert.equal(document.activeElement, input)); + + const actions = [ + ["select", view.getByRole("tab")], + ["close", view.getByLabelText("Close SHELL")], + ["new", view.getByLabelText("New Buzz Term tab")], + ]; + for (const [label, target] of actions) { + target.focus(); + assert.equal(document.activeElement, target, `${label} takes focus`); + fireEvent.click(target); + assert.equal( + document.activeElement, + input, + `${label} restores terminal focus`, + ); + } +}); + const EMPTY_FRAME = { cursor: { column: 0, line: 0, visible: false }, full: false, @@ -392,6 +449,177 @@ test("reduced motion keeps the terminal cursor solid", async () => { } }); +const TWO_SESSIONS = [ + { active: true, closing: false, id: "one", title: "SHELL" }, + { active: false, closing: false, id: "two", title: "LOG" }, +]; + +function frameWith(text, generation = 1) { + return { + cursor: { column: 0, line: 0, visible: false }, + full: false, + rows: [ + { + line: 0, + spans: [ + { + style: { fg: 0, bg: 0, flags: 0 }, + clusters: [{ column: 0, text, width: text.length }], + }, + ], + }, + ], + viewport: { columns: 112, generation, screenLines: 46 }, + }; +} + +const SWAPPED_SESSIONS = [ + { active: false, closing: false, id: "one", title: "SHELL" }, + { active: true, closing: false, id: "two", title: "LOG" }, +]; + +test("switching back to an idle session repaints its retained rows", async () => { + const subject = fixture({ + sessionFrames: [ + { frame: frameWith("one"), sessionId: "one" }, + { frame: frameWith("two"), sessionId: "two" }, + ], + sessions: TWO_SESSIONS, + }); + await ready(subject.view); + await waitFor(() => + assert.ok(gridDraws(subject.view).some((entry) => entry.text === "one")), + ); + + // A round trip is required, not a single switch: `paint()` is what drains the + // dirty set, so a grid that has never been painted still holds every line and + // repaints on first switch by luck. Only a session that was painted and then + // deactivated has an empty dirty set while holding rows. + subject.rerender({ sessions: SWAPPED_SESSIONS }); + await waitFor(() => + assert.ok(gridDraws(subject.view).some((entry) => entry.text === "two")), + ); + + paintLog.length = 0; + subject.rerender({ sessions: TWO_SESSIONS }); + await waitFor(() => + assert.ok( + gridDraws(subject.view).some((entry) => entry.text === "one"), + "returning to an idle session must repaint its retained rows", + ), + ); + assert.equal( + gridDraws(subject.view).some((entry) => entry.text === "two"), + false, + "the outgoing session's rows must not be repainted", + ); +}); + +test("switching to a session with no frame yet clears the outgoing pixels", async () => { + const subject = fixture({ + sessionFrames: [{ frame: frameWith("one"), sessionId: "one" }], + sessions: TWO_SESSIONS, + }); + await ready(subject.view); + await waitFor(() => + assert.ok(gridDraws(subject.view).some((entry) => entry.text === "one")), + ); + + // Session "two" has delivered no frame, so it has no grid: `markAllDirty()` + // and `paint()` are both no-ops and only a background fill can erase "one". + paintLog.length = 0; + subject.rerender({ sessions: SWAPPED_SESSIONS }); + await waitFor(() => + assert.ok( + gridDraws(subject.view).some( + (entry) => + entry.kind === "fill" && + entry.x === 0 && + entry.y === 0 && + entry.width === 940.8 && + entry.height === 782, + ), + "a full-viewport background fill must erase the outgoing session", + ), + ); + assert.equal( + gridDraws(subject.view).some((entry) => entry.text === "one"), + false, + ); +}); + +test("a frame on an unchanged session does not force a full repaint", async () => { + const subject = fixture({ + sessionFrames: [{ frame: frameWith("one"), sessionId: "one" }], + sessions: TWO_SESSIONS, + }); + await ready(subject.view); + await waitFor(() => + assert.ok(gridDraws(subject.view).some((entry) => entry.text === "one")), + ); + + // Guards the damage model, not the bug: dropping the `paintedSessionRef` + // write leaves `sessionChanged` permanently true, which repaints the whole + // viewport on every frame. That looks correct on screen and passes every + // assertion above, so only a negative test can see it. + paintLog.length = 0; + subject.rerender({ + sessionFrames: [{ frame: frameWith("more", 1), sessionId: "one" }], + }); + await waitFor(() => + assert.ok(gridDraws(subject.view).some((entry) => entry.text === "more")), + ); + assert.equal( + gridDraws(subject.view).some( + (entry) => + entry.kind === "fill" && entry.width === 940.8 && entry.height === 782, + ), + false, + "an incremental frame must not fill the whole viewport", + ); +}); + +test("a switch during a bailed paint pass is honoured once painting resumes", async () => { + const subject = fixture({ + sessionFrames: [{ frame: frameWith("one"), sessionId: "one" }], + sessions: TWO_SESSIONS, + }); + await ready(subject.view); + await waitFor(() => + assert.ok(gridDraws(subject.view).some((entry) => entry.text === "one")), + ); + + // The paint effect returns early when there is no 2d context. Writing + // `paintedSessionRef` before those returns would mark this switch as already + // painted, and the fill that erases "one" would never run. + dom.window.HTMLCanvasElement.prototype.getContext = () => null; + subject.rerender({ sessions: SWAPPED_SESSIONS }); + paintLog.length = 0; + + // Restoring the context is not enough to re-run the effect: its deps are the + // session, cursor, frames and palette. A frame for the now-inactive session + // re-triggers it without giving "two" a grid and without touching the palette, + // either of which would force the fill for an unrelated reason. + dom.window.HTMLCanvasElement.prototype.getContext = function () { + return paintRecorder(this); + }; + subject.rerender({ + sessionFrames: [{ frame: frameWith("one", 1), sessionId: "one" }], + sessions: SWAPPED_SESSIONS, + }); + await waitFor(() => + assert.ok( + gridDraws(subject.view).some( + (entry) => + entry.kind === "fill" && + entry.width === 940.8 && + entry.height === 782, + ), + "the pending switch must still repaint after the bail", + ), + ); +}); + function tabFixture(overrides = {}) { const calls = { close: [], input: [], select: [], spawn: 0 }; const subject = fixture({ diff --git a/desktop/src/features/terminal/TerminalSubstrate.tsx b/desktop/src/features/terminal/TerminalSubstrate.tsx index 46177e463..83dd1e04b 100644 --- a/desktop/src/features/terminal/TerminalSubstrate.tsx +++ b/desktop/src/features/terminal/TerminalSubstrate.tsx @@ -102,6 +102,7 @@ export function TerminalSubstrate({ const appliedFramesRef = React.useRef(new WeakSet()); const gridRef = React.useRef(null); const paintedPaletteRef = React.useRef(terminalPalette); + const paintedSessionRef = React.useRef(null); const previousFocusRef = React.useRef(null); const reportedFocusRef = React.useRef(null); const scrollBySessionRef = React.useRef(new Map()); @@ -408,15 +409,25 @@ export function TerminalSubstrate({ canvas.width !== pixelWidth || canvas.height !== pixelHeight; const paletteChanged = paintedPaletteRef.current !== terminalPalette; paintedPaletteRef.current = terminalPalette; + // A grid drains its dirty set in paint(), so a session that was painted and + // then deactivated comes back holding rows with nothing marked dirty. Both + // refs are written after the early returns above, so a pass that bails + // keeps the switch pending instead of swallowing it. + const sessionChanged = paintedSessionRef.current !== activeSessionId; + paintedSessionRef.current = activeSessionId; + const repaintAll = resized || paletteChanged || sessionChanged; if (resized) { canvas.width = pixelWidth; canvas.height = pixelHeight; } - if (resized || paletteChanged) { + if (repaintAll) { gridRef.current?.markAllDirty(); } context.setTransform(dpr, 0, 0, dpr, 0, 0); - if (resized || paletteChanged) { + if (repaintAll) { + // Not grid-guarded: switching to a session that has delivered no frame + // yet has no grid to mark or paint, so this fill is the only thing that + // erases the outgoing session's pixels. context.fillStyle = terminalPalette.background; context.fillRect(0, 0, bounds.width, bounds.height); } @@ -424,6 +435,13 @@ export function TerminalSubstrate({ gridRef.current?.paint(context, TERMINAL_CELL_METRICS, terminalPalette); }, [activeSessionId, cursorPainted, frames, terminalPalette]); + const runTabAction = (action: () => void) => { + action(); + if (owner === "terminal") { + textareaRef.current?.focus({ preventScroll: true }); + } + }; + return (
onSelectSession(session.id)} + onClick={() => runTabAction(() => onSelectSession(session.id))} role="tab" type="button" > @@ -477,7 +495,7 @@ export function TerminalSubstrate({ aria-label={`Close ${session.title}`} className="buzz-terminal-close" disabled={session.closing} - onClick={() => onCloseSession(session.id)} + onClick={() => runTabAction(() => onCloseSession(session.id))} type="button" > × @@ -487,7 +505,7 @@ export function TerminalSubstrate({