fix(dashboard): harden terminal paste contract and manual-start button per review

Review follow-up to 907e8d03e (ce-code-review, 8 personas):
- Ctrl/Cmd+V with no clipboard API (or after a denied read) now returns
  false WITHOUT preventDefault: xterm skips key handling and the browser's
  default paste fires xterm's helper-textarea listener once. Returning true
  made non-mac xterm inject \x16 and cancel the native paste (verified
  against xterm 5.5.0 _keyDown).
- A denied clipboard read sets a sticky ref so later pastes use the native
  path instead of being preventDefaulted into zero delivery.
- Custom-path delivery goes through terminal.paste() to restore bracketed
  paste and newline normalization.
- 'Start terminal' surfaces createTab failures in the error banner and
  disables while a create is in flight (no duplicate PTY sessions).
- normalizeActiveTab() extracted so storage-read and server-validation
  share one all-inactive tie-break; failure-path regression test added.
- Rewrote docs/solutions/ui-bugs/xterm-async-font-remeasure-paste-dedupe.md
  to the current paste contract (was prescribing the pre-#1902 behavior).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This commit is contained in:
gsxdsm
2026-07-23 19:46:02 -07:00
parent ca4639bb67
commit faeb491ea4
11 changed files with 220 additions and 41 deletions

View File

@@ -45,11 +45,25 @@ The recurrence path was stricter real-iOS font/text measurement behavior. A long
A second pitfall is custom paste handling. If an `attachCustomKeyEventHandler` Cmd/Ctrl+V branch reads `navigator.clipboard.readText()` and forwards that text to the PTY while the browser also performs the native paste into xterm's helper textarea, the same payload reaches `terminal.onData` and is sent twice.
**Paste history (superseded twice — read this before touching the paste branch):**
1. This doc originally prescribed "prefer native paste, return `true`, never read the clipboard manually."
2. GitHub #1902 (2026-07-04) reversed that: relying only on helper-textarea paste swallowed physical Ctrl/Cmd+V in some environments, so TerminalModal switched to a custom `navigator.clipboard.readText()` path returning `false`.
3. GitHub #2121/#2307 review (2026-07-23) found the custom path double-delivered and hardened it. The CURRENT contract is below; following the original prescription verbatim would reintroduce the #1902 swallow, and following #1902's version verbatim reintroduces the double paste.
Hard-won xterm 5.5.0 facts (verified against `@xterm/xterm` source during the 2026-07-23 fix):
- Returning `false` from `attachCustomKeyEventHandler` skips xterm's key handling but does NOT cancel the browser's default action — the default paste still fires xterm's helper-textarea `paste` listener.
- Returning `true` for Ctrl+V on non-mac lets xterm's `_keyDown` convert it into a `\x16` (SYN) data event sent to the PTY AND `cancel(event)` the browser paste — never return `true` for paste.
- xterm's `handlePasteEvent` calls `stopPropagation()` but NOT `preventDefault()`, and the same paste handler is registered on both the helper textarea and the root element.
## Solution
Keep one canonical paste path and remeasure after font resolution.
Current Cmd/Ctrl+V contract in `TerminalModal.tsx` (single delivery on every path):
- Prefer xterm's native helper-textarea paste for Cmd/Ctrl+V; return `true` from the custom key handler so the browser/xterm path runs, and do not read/send clipboard text manually.
- **Async clipboard available (secure context):** call `event.preventDefault()` (otherwise the browser's default paste double-delivers via the helper textarea), read via `navigator.clipboard.readText()`, and deliver through `terminal.paste(text)` — never raw `sendInput` — so bracketed-paste wrapping and `\n`→`\r` normalization apply. Return `false`.
- **Async clipboard missing (non-HTTPS remote, older Firefox) or a prior read was permission-denied (sticky `clipboardReadBlockedRef`):** return `false` WITHOUT `preventDefault()` — xterm skips its key handling and the un-prevented native paste delivers exactly once through the helper textarea.
- **`readText()` rejection** sets the sticky blocked ref so every subsequent Ctrl/Cmd+V uses the native path; at most one paste (at denial time) is lost.
- Preserve custom copy behavior only for selected text, where suppressing terminal input is intentional.
- After `terminal.open()`, treat FontFaceSet loading as best-effort: try the full stack, fall back to concrete individual families only if the full shorthand rejects, await `document.fonts.ready`, and never let an iOS shorthand rejection skip the later remeasure.
- Guard async remeasure work with the expected session id and current terminal/addon refs so stale font-load promises cannot mutate a disposed or switched terminal.
@@ -62,7 +76,9 @@ Keep one canonical paste path and remeasure after font resolution.
Cover the invariant across terminal surfaces and input paths:
- Keyboard paste on macOS (`metaKey`) and non-mac (`ctrlKey`) returns `true`, does not call `clipboard.readText()`, and sends exactly one PTY input frame via xterm `onData`.
- Keyboard paste on macOS (`metaKey`) and non-mac (`ctrlKey`) with clipboard available: returns `false`, preventDefaults, calls `clipboard.readText()` once, and delivers exactly once via `terminal.paste()` (no direct `sendInput`).
- Clipboard API missing (undefined `navigator.clipboard` or no `readText`): returns `false` with `defaultPrevented === false` so the native helper-textarea paste is the single delivery path.
- After a `readText()` permission denial, the NEXT Ctrl/Cmd+V returns `false` with no `preventDefault` and no further `readText()` call (sticky denial → native path).
- Native helper-textarea paste without the shortcut handler sends exactly once, covering mobile/iOS context-menu paste.
- A controlled `document.fonts.load()` promise resolving after `terminal.open()` triggers a post-font-load fit, resize, and refresh.
- A controlled `document.fonts.load()` rejection (the real-iOS shorthand failure mode) still triggers font option reapply, fit/resize, and refresh for both `TerminalModal` and `SessionTerminal`.

View File

@@ -551,6 +551,8 @@ interface TerminalModalProps {
export function TerminalModal({ isOpen, onClose, initialCommand, initialCommandGeneration = 0, projectId, embedded = false, defaultCwd, scopeId, footerVisible = false }: TerminalModalProps) {
const { t } = useTranslation("app");
const [error, setError] = useState<string | null>(null);
// FNXC:Terminal 2026-07-23-20:10: In-flight guard for the manual "Start terminal" action (GitHub #2121/#2307 review): rapid clicks must not create duplicate PTY sessions, and the Windows bootstrap-failure cohort this button serves must SEE createTab failures instead of a silently dead button.
const [isStartingTerminal, setIsStartingTerminal] = useState(false);
const [exitCode, setExitCode] = useState<number | null>(null);
const [xtermReady, setXtermReady] = useState(false);
const [xtermInitError, setXtermInitError] = useState<string | null>(null);
@@ -619,6 +621,8 @@ export function TerminalModal({ isOpen, onClose, initialCommand, initialCommandG
// (which under StrictMode/Vite Fast Refresh could leak a stale listener
// on the same xterm instance and cause per-character input doubling).
const sendInputRef = useRef<(data: string) => void>(() => {});
// FNXC:Terminal 2026-07-23-20:10: Sticky marker set when navigator.clipboard.readText rejects (permission denied). Once set, Ctrl/Cmd+V routes through the browser's native paste into xterm's helper textarea instead of retrying a read that will keep rejecting — at most one paste is lost, at denial time.
const clipboardReadBlockedRef = useRef(false);
// Window resize listener tied to the live xterm instance — tracked here so
// it can be removed in step with xterm disposal (modal close, tab switch).
const windowResizeListenerRef = useRef<(() => void) | null>(null);
@@ -1767,15 +1771,17 @@ export function TerminalModal({ isOpen, onClose, initialCommand, initialCommandG
if (key === "v") {
/*
FNXC:Terminal 2026-07-04-10:24:
GitHub #1902 showed that relying only on xterm's helper-textarea paste can swallow physical Ctrl/Cmd+V before clipboard text reaches the PTY. Own platform paste here, then return false so xterm's own key handling cannot also emit input.
GitHub #1902 showed that relying only on xterm's helper-textarea paste can swallow physical Ctrl/Cmd+V before clipboard text reaches the PTY. Own platform paste here when the async clipboard API is available.
FNXC:Terminal 2026-07-23-14:30:
Returning false only skips xterm's key handling — it does NOT cancel the browser's default paste, which fires xterm's helper-textarea `paste` listener and delivered every Ctrl/Cmd+V payload to the PTY twice. Call event.preventDefault() so the custom clipboard read is the single delivery path.
When the async clipboard API is unavailable (non-HTTPS remote access, older Firefox), return true instead of swallowing the shortcut: the browser's native paste into xterm's helper textarea is then the only working paste path.
FNXC:Terminal 2026-07-23-20:10:
Paste contract (GitHub #2121/#2307 review), verified against xterm 5.5.0 source:
- returning false from attachCustomKeyEventHandler skips xterm's key handling but does NOT cancel the browser's default paste — that default fires xterm's helper-textarea `paste` listener (single delivery). Never return true for paste: on non-mac, xterm's own _keyDown turns Ctrl+V into a \x16 data event and cancels the browser paste.
- When readText is available: call event.preventDefault() so the custom clipboard read is the SINGLE delivery path (without it the payload reached the PTY twice), and deliver via terminal.paste() so bracketed-paste wrapping and \n→\r normalization apply.
- When readText is missing (non-HTTPS remote, older Firefox) or a prior read was denied: return false with NO preventDefault so the native helper-textarea paste delivers exactly once.
*/
const readText = navigator.clipboard?.readText;
if (!readText) {
return true;
if (!readText || clipboardReadBlockedRef.current) {
return false;
}
event.preventDefault();
readText.call(navigator.clipboard)
@@ -1783,10 +1789,12 @@ export function TerminalModal({ isOpen, onClose, initialCommand, initialCommandG
if (!text || xtermInitializedRef.current !== currentSessionId) {
return;
}
sendInputRef.current(text);
terminal.paste(text);
})
.catch(() => {
// Ignore clipboard permission/errors so terminal input stays responsive.
// Permission denied (or transient failure): stop preventDefaulting future
// Ctrl/Cmd+V so the native paste path stays functional.
clipboardReadBlockedRef.current = true;
});
return false;
}
@@ -2915,7 +2923,20 @@ export function TerminalModal({ isOpen, onClose, initialCommand, initialCommandG
<div className="terminal-error-actions">
<button
className="terminal-retry-btn"
onClick={() => void createTab()}
onClick={() => {
if (isStartingTerminal) return;
setIsStartingTerminal(true);
setError(null);
createTab()
.catch((err) => {
const message = getErrorMessage(err);
setError(t("terminal.manualStartError", "Failed to start terminal: {{message}}", { message }));
})
.finally(() => {
setIsStartingTerminal(false);
});
}}
disabled={isStartingTerminal}
data-testid="terminal-manual-start-btn"
>
<Plus size={14} />

View File

@@ -1294,6 +1294,75 @@ describe("TerminalModal", () => {
expect(createTab).toHaveBeenCalledTimes(1);
});
it("surfaces a manual-start failure and re-enables the button instead of silently no-oping", async () => {
const createTab = vi.fn().mockRejectedValue(new Error("spawn failed"));
mockUseTerminalSessions.mockReturnValue({
...defaultSessionState,
tabs: [],
activeTab: null,
autoCreateDisabled: true,
createTab,
});
render(<TerminalModal isOpen={true} onClose={mockOnClose} />);
await waitFor(() => {
expect(screen.getByTestId("terminal-manual-start")).toBeTruthy();
});
fireEvent.click(screen.getByTestId("terminal-manual-start-btn"));
// The Windows bootstrap-failure cohort is exactly where creates fail — a
// rejected createTab must render the existing terminal-error banner, not
// leave a button that silently does nothing.
await waitFor(() => {
expect(screen.getByTestId("terminal-error").textContent).toContain("spawn failed");
});
const button = screen.getByTestId("terminal-manual-start-btn") as HTMLButtonElement;
await waitFor(() => {
expect(button.disabled).toBe(false);
});
});
it("ignores rapid Start-terminal clicks while a create is already in flight", async () => {
let resolveCreate: (tab: typeof defaultTab) => void = () => {};
const createTab = vi.fn().mockImplementation(
() => new Promise((resolve) => {
resolveCreate = resolve;
}),
);
mockUseTerminalSessions.mockReturnValue({
...defaultSessionState,
tabs: [],
activeTab: null,
autoCreateDisabled: true,
createTab,
});
render(<TerminalModal isOpen={true} onClose={mockOnClose} />);
await waitFor(() => {
expect(screen.getByTestId("terminal-manual-start")).toBeTruthy();
});
const button = screen.getByTestId("terminal-manual-start-btn") as HTMLButtonElement;
fireEvent.click(button);
fireEvent.click(button);
fireEvent.click(button);
// One PTY session per user intent: while the first create is pending the
// button is disabled and the handler short-circuits.
expect(createTab).toHaveBeenCalledTimes(1);
await waitFor(() => {
expect(button.disabled).toBe(true);
});
await act(async () => {
resolveCreate(defaultTab);
await Promise.resolve();
});
});
it("keeps the normal xterm surface when auto-create is disabled but a tab already exists", async () => {
mockUseTerminalSessions.mockReturnValue({
...defaultSessionState,
@@ -7519,8 +7588,12 @@ describe("TerminalModal — xterm focus initialization (FN-1602)", () => {
// and the payload reaches the PTY twice.
expect(pasteEvent.defaultPrevented).toBe(true);
await waitFor(() => expect(readText).toHaveBeenCalledTimes(1));
expect(mockSendInput).toHaveBeenCalledTimes(1);
expect(mockSendInput).toHaveBeenCalledWith("npm test\n");
// Delivery must go through terminal.paste() (bracketed-paste wrapping,
// \n -> \r normalization), which feeds onData -> sendInput in real xterm —
// never through a raw sendInput that bypasses paste semantics.
await waitFor(() => expect(mockTerminalInstance.paste).toHaveBeenCalledTimes(1));
expect(mockTerminalInstance.paste).toHaveBeenCalledWith("npm test\n");
expect(mockSendInput).not.toHaveBeenCalled();
},
);
@@ -7549,14 +7622,55 @@ describe("TerminalModal — xterm focus initialization (FN-1602)", () => {
const handled = terminalKeyEventHandler?.(pasteEvent);
// Without an async clipboard read the browser's native paste into
// xterm's helper textarea is the ONLY working paste path — the handler
// must let it run rather than returning false and killing paste dead.
expect(handled).toBe(true);
// xterm's helper textarea is the ONLY working paste path. The handler
// must return false WITHOUT preventDefault: false skips xterm's key
// handling (whose non-mac Ctrl+V path would inject \x16 into the PTY
// and cancel the browser paste), while the un-prevented default paste
// fires xterm's helper-textarea paste listener exactly once.
expect(handled).toBe(false);
expect(pasteEvent.defaultPrevented).toBe(false);
expect(mockSendInput).not.toHaveBeenCalled();
expect(mockTerminalInstance.paste).not.toHaveBeenCalled();
},
);
it("routes Ctrl+V through the native paste path after a clipboard permission denial (sticky)", async () => {
const readText = vi.fn().mockRejectedValue(new DOMException("denied"));
Object.defineProperty(navigator, "platform", {
value: "Win32",
configurable: true,
});
Object.defineProperty(navigator, "clipboard", {
value: { readText },
configurable: true,
});
render(<TerminalModal isOpen={true} onClose={mockOnClose} />);
await waitFor(() => {
expect(terminalKeyEventHandler).not.toBeNull();
});
// First Ctrl+V: custom path preventDefaults, read rejects (denied) — this
// one paste is lost, and the denial must be remembered.
const firstPaste = new KeyboardEvent("keydown", { key: "v", ctrlKey: true, cancelable: true });
expect(terminalKeyEventHandler?.(firstPaste)).toBe(false);
expect(firstPaste.defaultPrevented).toBe(true);
await waitFor(() => expect(readText).toHaveBeenCalledTimes(1));
await act(async () => {
await Promise.resolve();
});
// Second Ctrl+V: the sticky denial marker must route through the native
// helper-textarea paste (no preventDefault, no further readText attempts)
// instead of preventDefaulting into a read that will reject again —
// otherwise permission-denied users have zero working paste path.
const secondPaste = new KeyboardEvent("keydown", { key: "v", ctrlKey: true, cancelable: true });
expect(terminalKeyEventHandler?.(secondPaste)).toBe(false);
expect(secondPaste.defaultPrevented).toBe(false);
expect(readText).toHaveBeenCalledTimes(1);
});
it.each([
["rejected clipboard", { readText: vi.fn().mockRejectedValue(new DOMException("denied")) }],
["empty clipboard", { readText: vi.fn().mockResolvedValue("") }],

View File

@@ -405,6 +405,31 @@ describe("useTerminalSessions", () => {
expect(result.current.activeTab?.id).toBe("tab-a");
expect(mockCreateTerminalSession).not.toHaveBeenCalled();
});
it("activates the first tab even when server validation FAILS (the motivating wedge)", async () => {
// The validation-failure branch keeps tabs exactly as read from storage —
// without normalization at the storage boundary, an all-inactive payload
// plus an unreachable server left activeTab null forever: the spinner
// stayed up and auto-create was blocked by tabs.length > 0.
const storedTabs = [
{ id: "tab-a", sessionId: "session-a", title: "bash", isActive: false, createdAt: 1 },
{ id: "tab-b", sessionId: "session-b", title: "zsh", isActive: false, createdAt: 2 },
];
localStorageMock.getItem.mockImplementation((key: string) =>
key === TERMINAL_TABS_KEY ? JSON.stringify(storedTabs) : null,
);
mockListTerminalSessions.mockRejectedValue(new Error("server unreachable"));
const { result } = renderHook(() => useTerminalSessions(TEST_PROJECT_ID));
await waitFor(() => {
expect(result.current.isReady).toBe(true);
});
expect(result.current.tabs.length).toBe(2);
expect(result.current.activeTab).not.toBeNull();
expect(result.current.activeTab?.id).toBe("tab-a");
});
});
describe("bootstrap sequencing (FN-7686)", () => {

View File

@@ -98,6 +98,22 @@ function terminalTabsStorageKey(storageScope?: string): string {
return trimmed ? `${STORAGE_KEY}:${trimmed}` : STORAGE_KEY;
}
/*
FNXC:Terminal 2026-07-23-14:30 (helper extracted 2026-07-23-20:10):
A tab list where no tab is active must never survive a restore or validation
pass: TerminalModal derives its whole UI from `activeTab`, and an all-inactive
tab list leaves the "Starting terminal..." spinner up forever while the
auto-create effect is blocked by tabs.length > 0. This single helper owns the
tie-break (activate the first tab) for BOTH the storage-read boundary and the
server-validation success branch so the two paths cannot drift.
*/
function normalizeActiveTab(tabs: TerminalTab[]): TerminalTab[] {
if (tabs.length === 0 || tabs.some((tab) => tab.isActive)) {
return tabs;
}
return tabs.map((tab, i) => ({ ...tab, isActive: i === 0 }));
}
function readTabsFromStorage(projectId?: string, storageScope?: string): TerminalTab[] {
if (typeof window === "undefined") return [];
@@ -106,19 +122,9 @@ function readTabsFromStorage(projectId?: string, storageScope?: string): Termina
if (stored) {
const parsed = JSON.parse(stored) as TerminalTab[];
if (!Array.isArray(parsed)) return [];
/*
FNXC:Terminal 2026-07-23-14:30:
A persisted payload where no tab is active must never survive the restore:
TerminalModal derives its whole UI from `activeTab`, and an all-inactive
tab list leaves the "Starting terminal..." spinner up forever while the
auto-create effect is blocked by tabs.length > 0. The success path of
server validation normalizes this, but the validation-failure path keeps
tabs as-read, so normalize at the storage boundary instead.
*/
if (parsed.length > 0 && !parsed.some((tab) => tab.isActive)) {
return parsed.map((tab, i) => ({ ...tab, isActive: i === 0 }));
}
return parsed;
// Normalize here (not only in server validation) because the
// validation-FAILURE path keeps tabs exactly as read from storage.
return normalizeActiveTab(parsed);
}
} catch {
// Ignore localStorage errors
@@ -288,17 +294,8 @@ export function useTerminalSessions(projectId?: string, options: UseTerminalSess
// Strip internal _verified property and return clean TerminalTab objects
const cleanTabs = remainingTabs.map(({ _verified: _unused, ...tab }) => tab);
// Ensure exactly one tab is active
const activeTab = cleanTabs.find((t) => t.isActive);
if (!activeTab) {
// No active tab, activate the first one
return cleanTabs.map((tab, i) => ({
...tab,
isActive: i === 0,
}));
}
return cleanTabs;
// Ensure at least one tab is active (shared tie-break with the storage-read boundary)
return normalizeActiveTab(cleanTabs);
});
// Mark as ready after validation

View File

@@ -8541,6 +8541,7 @@
"shortcuts": "Shortcuts",
"startingTerminal": "Starting terminal...",
"manualStartHint": "The terminal is ready — start a session to begin.",
"manualStartError": "Failed to start terminal: {{message}}",
"startTerminal": "Start terminal",
"statusConnected": "Connected",
"statusConnecting": "Connecting...",

View File

@@ -8531,6 +8531,7 @@
"shortcuts": "Atajos",
"startingTerminal": "Iniciando terminal...",
"manualStartHint": "La terminal está lista: inicia una sesión para comenzar.",
"manualStartError": "No se pudo iniciar la terminal: {{message}}",
"startTerminal": "Iniciar terminal",
"statusConnected": "Conectado",
"statusConnecting": "Conectando...",

View File

@@ -8531,6 +8531,7 @@
"shortcuts": "Raccourcis",
"startingTerminal": "Démarrage du terminal...",
"manualStartHint": "Le terminal est prêt — démarrez une session pour commencer.",
"manualStartError": "Échec du démarrage du terminal : {{message}}",
"startTerminal": "Démarrer le terminal",
"statusConnected": "Connecté",
"statusConnecting": "Connexion en cours...",

View File

@@ -8531,6 +8531,7 @@
"shortcuts": "단축키",
"startingTerminal": "터미널 시작 중...",
"manualStartHint": "터미널이 준비되었습니다 — 세션을 시작하세요.",
"manualStartError": "터미널 시작 실패: {{message}}",
"startTerminal": "터미널 시작",
"statusConnected": "연결됨",
"statusConnecting": "연결 중...",

View File

@@ -8531,6 +8531,7 @@
"shortcuts": "快捷键",
"startingTerminal": "启动终端中...",
"manualStartHint": "终端已就绪 — 启动会话以开始。",
"manualStartError": "启动终端失败:{{message}}",
"startTerminal": "启动终端",
"statusConnected": "已连接",
"statusConnecting": "连接中...",

View File

@@ -8531,6 +8531,7 @@
"shortcuts": "快捷鍵",
"startingTerminal": "正在啟動終端...",
"manualStartHint": "終端已就緒 — 啟動工作階段以開始。",
"manualStartError": "啟動終端失敗:{{message}}",
"startTerminal": "啟動終端",
"statusConnected": "已連線",
"statusConnecting": "正在連線...",