Skip to content

Commit ef614f9

Browse files
elirantutiaclaude
andcommitted
fix background re-renders stealing focus and killing terminal selection
The statusLine helper rewrote <sid>.sessionid on every Claude render, and updateSessionCliId had no equality guard — so each identical write fired session-changed, and renderLayout() answered with container.appendChild(pane). appendChild on a node that is already a child is a remove-and-reinsert: it blurs whatever is focused inside (the Cmd+F find bar is appended *into* .terminal-pane) and detaches the node mid-drag, where xterm's selection math reads a zero-size rect. setFocused then yanked focus back, because its closest('.terminal-pane') test counts the find bar's input as "already the terminal". Fixed at four levels: Source. updateSessionCliId early-returns on an unchanged id, and .sessionid joins .name behind one write_if_changed helper in the generated statusLine script. DOM. attachToContainer no-ops when the pane is already in the container, matching the seven sibling attach* helpers that already did; the ordering concern it was smuggling moves up to ensurePaneOrder in the layout owner, which compares *relative* order (hidden panes stay in the container interleaved with the laid-out ones) and so mutates nothing in the steady state. renderSwarmMode reuses its grid wrapper instead of rebuilding it. Focus. focusPane gates the call on shouldFocusPane, so a render that does not change the focused pane never reaches setFocused at all — the find bar cannot be told apart from the terminal down there, so the decision belongs one level up. setFocused goes back to its original guard. Resize. fitTerminal skips pty.resize when the cols x rows memo is unchanged; a redundant resize makes the CLI redraw under a selection, and xterm clears the selection outright on a row-count change. spawnTerminal clears that memo and re-fits *after* pty.create resolves: callers fire it un-awaited and fit immediately, so the first resize can reach main while pty:create is still suspended (Copilot awaits a hook install) and resizePty drops it — the memo would otherwise pin the PTY at the 120x30 spawn default for the session's life. Both decisions live in DOM-free pane-order.ts / pane-focus.ts and are unit-tested. The terminal-pane test fake also had className and classList as separate state and an appendChild that did not move nodes; both are fixed, which is what let the attach and fit tests be written honestly. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012qptmPxqXxKn13WfPzFXXz
1 parent a6c6f9d commit ef614f9

14 files changed

Lines changed: 643 additions & 62 deletions

‎CLAUDE.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -65,6 +65,7 @@ CLI-specific behavior is encapsulated behind a `CliProvider` interface (`src/mai
6565
- `terminal-pane.ts` — xterm.js wrapper per session, handles PTY data streaming and WebGL rendering with software fallback
6666
- `state.ts` — Reactive AppState singleton; debounced persistence (300ms) to `~/.vibeyard/state.json`
6767
- `split-layout.ts` — Manages tab mode (single terminal) vs split mode (side-by-side)
68+
- **Renders must not touch pane DOM they don't need to.** `renderLayout()` runs on every `session-changed`/`layout-changed`, and `appendChild` on a node that is *already* a child is a remove-and-reinsert: it blurs whatever is focused inside (the Cmd+F find bar is appended *into* `.terminal-pane`, see `search-bar.ts`) and collapses an in-progress selection. So every `attach*ToContainer` no-ops when `element.parentElement === container`; DOM order among the panes laid out together is corrected separately by `ensurePaneOrder` (split-layout) via the DOM-free `isInRelativeOrder` in `components/pane-order.ts` — *relative* order, because hidden panes stay in the container interleaved with the visible ones — and `renderSwarmMode` reuses its `.swarm-grid-wrapper` instead of rebuilding it. Focus is guarded one level up, not by sniffing the DOM: `setFocused` cannot tell the find bar from the terminal (it is inside the pane), so `focusPane` gates the call on the DOM-free `shouldFocusPane` (`components/pane-focus.ts`) — a render that does not change the focused pane never calls `setFocused` at all, unless focus is sitting on nothing. `fitTerminal` skips `pty.resize` when the `cols`x`rows` memo is unchanged (a redundant resize makes the CLI redraw under a selection, and xterm does `clearSelection()` on a row-count change); `spawnTerminal` clears that memo and re-fits **after** `pty.create` resolves, because a fit racing the un-awaited spawn is dropped by `resizePty` for a session it has not registered yet (Copilot awaits a hook install first) and the PTY would stay at the 120×30 spawn default. Feeding this loop, `updateSessionCliId` (`state.ts`) early-returns on an unchanged id and the statusLine's `write_if_changed` writes `.sessionid`/`.name` only on change — the statusLine fires on every render, and each write used to cost a persist plus a full re-render.
6869
- `session-activity.ts` — Tracks working/waiting/idle status with debounced transitions
6970
- **Hook → session-state contract** — see `HOOKS.md` for the full map; it is verified against a specific Claude Code version and must be re-checked against https://code.claude.com/docs/en/hooks whenever hook handling changes. Three points are load-bearing and easy to get wrong. (1) **`PostToolUse` fires only on success**; a tool that ran and failed fires `PostToolUseFailure` (`error`, `is_interrupt`, `duration_ms`), and a call rejected before execution fires neither. Treating any non-empty `tool_response` as a failure — which this repo did until the events were re-verified — feeds every successful tool call into `missing-tool-detector.ts`. (2) **A `Stop` is not always a completion**: the main agent fires one every time it pauses on parallel subagents. `stop_status_writer.py` resolves it from the payload's `background_tasks` array, holding `working` only for `subagent`/`teammate`/`workflow` entries; only a *non-empty* array is authoritative — an empty one is not proof of an idle session, because the CLI filters that array on an `isBackgrounded` flag freshly-dispatched subagents don't carry yet — so empty *and* absent both fall through to the legacy `<sid>.subagents` counter. `session_crons` is never consulted or a `/loop` session would never complete. (3) **Every field name in `INSPECTOR_FIELDS` (`claude-cli.ts`) must exist in a documented per-event schema, and be read by something.** A compile-time assertion against `keyof InspectorEvent` catches the internal half of that drift; the external half — whether Claude Code actually sends the field — no type system can check, and an invented name silently renders a blank timeline row forever (how `config_key`/`question`/`answer` survived for months). Beware nested keys when reading the docs: the Elicitation example's `requested_schema.properties.username.type` scans like top-level `type`/`username` fields, and both were added on a previous pass and were dead on arrival. Generated Python embeds values via `pyLiteral` (`shared/python.ts`), never a raw `r'…'` literal, which breaks on an apostrophe in the path.
7071
- `session-cost.ts` — Structured cost tracking via Claude CLI status line (`statusLine` setting), with regex fallback for older CLI versions. Provides per-session and aggregate cost data (USD, tokens, cache, duration)

‎src/main/hook-status.test.ts‎

Lines changed: 58 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -101,9 +101,14 @@ describe('hook-status', () => {
101101
const opts = { input, env: { ...process.env, PYTHONIOENCODING: 'utf-8' } };
102102
return spawnSync('python3', ['-c', code], opts);
103103
};
104+
// Memoised: three tests probe for python3, and each probe is a process launch.
105+
let pythonAvailable: boolean | undefined;
104106
const hasPython = () => {
105-
const { spawnSync } = require('child_process') as typeof import('child_process');
106-
return !spawnSync('python3', ['-c', 'pass']).error;
107+
if (pythonAvailable === undefined) {
108+
const { spawnSync } = require('child_process') as typeof import('child_process');
109+
pythonAvailable = !spawnSync('python3', ['-c', 'pass']).error;
110+
}
111+
return pythonAvailable;
107112
};
108113

109114
it('extracts cost, context_window, session_id and session_name', () => {
@@ -154,6 +159,57 @@ describe('hook-status', () => {
154159
expect(body()).not.toContain("encoding=");
155160
});
156161

162+
it('writes .sessionid only when the id changed', () => {
163+
// The statusLine fires on every render. An unconditional write reaches the
164+
// renderer as an IPC that ends in a persist plus a full renderLayout(),
165+
// which used to re-append the terminal pane many times a second — blurring
166+
// the in-pane find bar and collapsing an in-progress selection.
167+
if (!hasPython()) return; // no python3 on this runner
168+
169+
// Real fs, deliberately: `fs` is mocked in this suite, so the script's own
170+
// writes are observed from inside Python instead.
171+
const tmp = path.join(
172+
process.env.TMPDIR || process.env.TEMP || '/tmp',
173+
`vibeyard-statusline-${process.pid}`,
174+
);
175+
const driver = `
176+
import sys,os,io,json,builtins,shutil
177+
src=sys.stdin.read()
178+
d=${JSON.stringify(tmp)}
179+
shutil.rmtree(d,ignore_errors=True)
180+
os.makedirs(d)
181+
writes=[]
182+
real=builtins.open
183+
def spy(file,mode='r',*a,**k):
184+
if str(file).endswith('.sessionid') and 'w' in mode:
185+
writes.append(str(file))
186+
return real(file,mode,*a,**k)
187+
builtins.open=spy
188+
os.environ['CLAUDE_IDE_SESSION_ID']='sess1'
189+
def run(payload):
190+
sys.stdin=io.StringIO(payload)
191+
try:
192+
exec(compile(src,'statusline','exec'),{})
193+
except SystemExit:
194+
pass
195+
run('{"session_id":"cli-a"}')
196+
run('{"session_id":"cli-a"}')
197+
after_repeat=len(writes)
198+
run('{"session_id":"cli-b"}')
199+
final=real(os.path.join(d,'sess1.sessionid')).read()
200+
shutil.rmtree(d,ignore_errors=True)
201+
sys.stdout.write(json.dumps({'afterRepeat':after_repeat,'total':len(writes),'final':final}))
202+
`;
203+
// Built against the temp dir so the script's .cost/.name writes land there too.
204+
const result = runPython(driver, buildStatusLinePython(tmp));
205+
expect(result.status).toBe(0);
206+
207+
const seen = JSON.parse(result.stdout.toString());
208+
expect(seen.afterRepeat).toBe(1); // the repeat wrote nothing
209+
expect(seen.total).toBe(2); // the changed id did
210+
expect(seen.final).toBe('cli-b');
211+
});
212+
157213
it('is syntactically valid Python', () => {
158214
// A syntax error here is silent in production: it kills cost, context,
159215
// sessionid and name at once, with the traceback going only to

‎src/main/hook-status.ts‎

Lines changed: 25 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -49,6 +49,15 @@ export function getStatusLineScriptPath(): string {
4949
* The `.name` payload carries the CLI session id alongside the title so a
5050
* stale title can be told apart from the current conversation's.
5151
*
52+
* `.sessionid` and `.name` both go through `write_if_changed`, which writes only
53+
* when the content differs (and removes the file for an empty blob — how a
54+
* cleared `.name` is retracted; `.sessionid` never passes one). The statusLine
55+
* fires on every render, and each write reaches the renderer as an IPC that ends
56+
* in a `persist()` plus a full `renderLayout()` — which used to detach and
57+
* re-insert the terminal pane many times a second, blurring the in-pane find bar
58+
* and breaking mouse selection. `.cost` is exempt: its payload genuinely changes
59+
* on nearly every render, and it lands on a persist-only path with no re-render.
60+
*
5261
* Installed as a real `.py` file on every platform and invoked by path, never
5362
* inlined into the shell command — see the module docstring in
5463
* `hook-commands.ts` for why inlining Python is fragile here.
@@ -77,28 +86,28 @@ if cost or ctx or model:
7786
payload['model']=model
7887
with open(os.path.join(status_dir,sid+'.cost'),'w') as f:
7988
json.dump(payload,f)
80-
claude_sid=d.get('session_id','')
81-
if claude_sid:
82-
with open(os.path.join(status_dir,sid+'.sessionid'),'w') as f:
83-
f.write(claude_sid)
84-
name_path=os.path.join(status_dir,sid+'.name')
85-
name=str(d.get('session_name') or '').strip()
86-
blob=json.dumps({'name':name,'session_id':claude_sid}) if name else ''
87-
prev=''
88-
try:
89-
with open(name_path) as f:
90-
prev=f.read()
91-
except:
92-
pass
93-
if prev!=blob:
89+
def write_if_changed(path,blob):
90+
prev=''
91+
try:
92+
with open(path) as f:
93+
prev=f.read()
94+
except:
95+
pass
96+
if prev==blob:
97+
return
9498
try:
9599
if blob:
96-
with open(name_path,'w') as f:
100+
with open(path,'w') as f:
97101
f.write(blob)
98102
else:
99-
os.remove(name_path)
103+
os.remove(path)
100104
except:
101105
pass
106+
claude_sid=d.get('session_id','')
107+
if claude_sid:
108+
write_if_changed(os.path.join(status_dir,sid+'.sessionid'),claude_sid)
109+
name=str(d.get('session_name') or '').strip()
110+
write_if_changed(os.path.join(status_dir,sid+'.name'),json.dumps({'name':name,'session_id':claude_sid}) if name else '')
102111
`;
103112
}
104113

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,25 @@
1+
import { describe, it, expect } from 'vitest';
2+
import { shouldFocusPane, type PaneFocusMemo } from './pane-focus';
3+
4+
describe('shouldFocusPane', () => {
5+
const prev: PaneFocusMemo = { projectId: 'p1', sessionId: 's1' };
6+
7+
it('focuses on the first render', () => {
8+
expect(shouldFocusPane(null, prev, false)).toBe(true);
9+
});
10+
11+
it('focuses when the session or the project changed', () => {
12+
expect(shouldFocusPane(prev, { projectId: 'p1', sessionId: 's2' }, false)).toBe(true);
13+
expect(shouldFocusPane(prev, { projectId: 'p2', sessionId: 's1' }, false)).toBe(true);
14+
});
15+
16+
it('leaves focus alone on a repeat render', () => {
17+
// The find bar the user is typing into lives inside the pane, so setFocused
18+
// cannot tell it from the terminal — this is where it is protected.
19+
expect(shouldFocusPane(prev, { ...prev }, false)).toBe(false);
20+
});
21+
22+
it('reclaims focus on a repeat render when nothing holds it', () => {
23+
expect(shouldFocusPane(prev, { ...prev }, true)).toBe(true);
24+
});
25+
});
Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,29 @@
1+
// Focus decisions for the layout, kept DOM-free so they can be unit-tested.
2+
3+
/** Which pane the layout last moved DOM focus to. */
4+
export interface PaneFocusMemo {
5+
projectId: string;
6+
sessionId: string;
7+
}
8+
9+
/**
10+
* Whether a layout render may move DOM focus into `next`'s pane.
11+
*
12+
* `renderLayout()` runs on every `session-changed` — a background statusLine tick
13+
* included — so only an actual change of the focused pane justifies taking focus.
14+
* The Cmd+F find bar lives inside the pane it searches, which makes it invisible
15+
* to `setFocused`'s "is focus already on a terminal" test; not calling `setFocused`
16+
* at all is what keeps the user's keystrokes in it.
17+
*
18+
* `focusIsIdle` (nothing focused, or the body) re-opens the door on a repeat
19+
* render, so focus still lands in the terminal when it is sitting nowhere.
20+
*/
21+
export function shouldFocusPane(
22+
prev: PaneFocusMemo | null,
23+
next: PaneFocusMemo,
24+
focusIsIdle: boolean,
25+
): boolean {
26+
if (!prev) return true;
27+
if (prev.projectId !== next.projectId || prev.sessionId !== next.sessionId) return true;
28+
return focusIsIdle;
29+
}
Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,30 @@
1+
import { describe, it, expect } from 'vitest';
2+
import { isInRelativeOrder } from './pane-order';
3+
4+
describe('isInRelativeOrder', () => {
5+
it('accepts an empty wanted list', () => {
6+
expect(isInRelativeOrder(['a', 'b'], [])).toBe(true);
7+
expect(isInRelativeOrder([], [])).toBe(true);
8+
});
9+
10+
it('accepts a contiguous run in order', () => {
11+
expect(isInRelativeOrder(['a', 'b', 'c'], ['a', 'b', 'c'])).toBe(true);
12+
});
13+
14+
it('ignores unrelated nodes interleaved between the wanted ones', () => {
15+
// Hidden panes stay in the container between the laid-out ones.
16+
expect(isInRelativeOrder(['a', 'hidden', 'b'], ['a', 'b'])).toBe(true);
17+
});
18+
19+
it('rejects a swapped pair', () => {
20+
expect(isInRelativeOrder(['b', 'a'], ['a', 'b'])).toBe(false);
21+
});
22+
23+
it('rejects a wanted node that is not a child yet', () => {
24+
expect(isInRelativeOrder(['a'], ['a', 'b'])).toBe(false);
25+
});
26+
27+
it('rejects a node moved to the front of the container', () => {
28+
expect(isInRelativeOrder(['c', 'a', 'b'], ['a', 'b', 'c'])).toBe(false);
29+
});
30+
});
Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,19 @@
1+
// DOM-order decisions for the layout, kept DOM-free so they can be unit-tested.
2+
3+
/**
4+
* Whether `wanted` already sits inside `children` in that relative order — i.e.
5+
* whether a layout pass needs to move any pane at all.
6+
*
7+
* Only the *relative* order matters: hidden panes stay in the container and sit
8+
* interleaved with the laid-out ones, so a `previousSibling` comparison would
9+
* report a false mismatch and re-append every pane on every render.
10+
*/
11+
export function isInRelativeOrder<T>(children: readonly T[], wanted: readonly T[]): boolean {
12+
let last = -1;
13+
for (const item of wanted) {
14+
const index = children.indexOf(item);
15+
if (index === -1 || index < last) return false;
16+
last = index;
17+
}
18+
return true;
19+
}

‎src/renderer/components/remote-terminal-pane.test.ts‎

Lines changed: 57 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ class FakeTerminal {
99

1010
loadAddon(): void {}
1111
onData(): void {}
12+
onSelectionChange(): void {}
1213
open(): void {}
1314
write(): void {}
1415
dispose(): void {}
@@ -54,6 +55,8 @@ class FakeElement {
5455
innerHTML = '';
5556

5657
appendChild(child: FakeElement): FakeElement {
58+
// Mirror the DOM: appending a node that already has a parent moves it.
59+
child.remove();
5760
child.parentElement = this;
5861
this.children.push(child);
5962
return child;
@@ -125,3 +128,57 @@ describe('applyThemeToAllRemoteTerminals()', () => {
125128
expect((instance.terminal as unknown as FakeTerminal).options.theme).toBe(lightTerminalTheme);
126129
});
127130
});
131+
132+
describe('attachRemoteToContainer()', () => {
133+
beforeEach(() => {
134+
vi.resetModules();
135+
vi.clearAllMocks();
136+
vi.stubGlobal('document', new FakeDocument());
137+
});
138+
139+
/** Stand in for the .xterm node terminal.open() would have created. */
140+
function markOpened(element: FakeElement): void {
141+
const screen = new FakeElement();
142+
screen.className = 'xterm';
143+
element.querySelector('.xterm-wrap')!.appendChild(screen);
144+
}
145+
146+
it('does not re-append a pane that is already in the container', async () => {
147+
// appendChild on an existing child removes and re-inserts it, blurring
148+
// whatever is focused inside and collapsing any selection.
149+
const { createRemoteTerminalPane, attachRemoteToContainer, getRemoteTerminalInstance, _resetForTesting } =
150+
await import('./remote-terminal-pane.js');
151+
const container = new FakeElement();
152+
153+
_resetForTesting();
154+
createRemoteTerminalPane('remote-attach', 'readonly', 80, 24, () => {});
155+
attachRemoteToContainer('remote-attach', container as unknown as HTMLElement);
156+
157+
const element = getRemoteTerminalInstance('remote-attach')!.element as unknown as FakeElement;
158+
markOpened(element);
159+
160+
attachRemoteToContainer('remote-attach', container as unknown as HTMLElement);
161+
attachRemoteToContainer('remote-attach', container as unknown as HTMLElement);
162+
163+
expect(container.children).toEqual([element]);
164+
});
165+
166+
it('moves a pane attached to a different container', async () => {
167+
const { createRemoteTerminalPane, attachRemoteToContainer, getRemoteTerminalInstance, _resetForTesting } =
168+
await import('./remote-terminal-pane.js');
169+
const first = new FakeElement();
170+
const second = new FakeElement();
171+
172+
_resetForTesting();
173+
createRemoteTerminalPane('remote-move', 'readonly', 80, 24, () => {});
174+
attachRemoteToContainer('remote-move', first as unknown as HTMLElement);
175+
176+
const element = getRemoteTerminalInstance('remote-move')!.element as unknown as FakeElement;
177+
markOpened(element);
178+
179+
attachRemoteToContainer('remote-move', second as unknown as HTMLElement);
180+
181+
expect(first.children).toEqual([]);
182+
expect(second.children).toEqual([element]);
183+
});
184+
});

‎src/renderer/components/remote-terminal-pane.ts‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -93,6 +93,7 @@ export function getRemoteTerminalInstance(sessionId: string): RemoteTerminalInst
9393
return instances.get(sessionId);
9494
}
9595

96+
/** Idempotent — see `attachToContainer` in `terminal-pane.ts` for why that matters. */
9697
export function attachRemoteToContainer(sessionId: string, container: HTMLElement): void {
9798
const instance = instances.get(sessionId);
9899
if (!instance) return;
@@ -104,7 +105,7 @@ export function attachRemoteToContainer(sessionId: string, container: HTMLElemen
104105
attachCopyOnSelect(instance.terminal);
105106

106107
loadWebglWithFallback(instance.terminal);
107-
} else {
108+
} else if (instance.element.parentElement !== container) {
108109
container.appendChild(instance.element);
109110
}
110111
}

0 commit comments

Comments
 (0)