diff --git a/src/main/ipc/pty.test.ts b/src/main/ipc/pty.test.ts index dadb41f77..aad4adf43 100644 --- a/src/main/ipc/pty.test.ts +++ b/src/main/ipc/pty.test.ts @@ -3530,5 +3530,45 @@ describe('registerPtyHandlers', () => { vi.useRealTimers() } }) + + it('serializes overlapping headless snapshots for the same PTY', async () => { + let resolveFirst!: (snapshot: { data: string; cols: number; rows: number }) => void + let resolveSecond!: (snapshot: { data: string; cols: number; rows: number }) => void + const runtime = { + setPtyController: vi.fn(), + onPtySpawned: vi.fn(), + onPtyData: vi.fn(), + onPtyExit: vi.fn(), + preAllocateHandleForPty: vi.fn(), + serializeHeadlessTerminalBufferForRenderer: vi + .fn() + .mockImplementationOnce( + () => + new Promise<{ data: string; cols: number; rows: number }>((resolve) => { + resolveFirst = resolve + }) + ) + .mockImplementationOnce( + () => + new Promise<{ data: string; cols: number; rows: number }>((resolve) => { + resolveSecond = resolve + }) + ) + } + handlers.clear() + registerPtyHandlers(mainWindow as never, runtime as never) + + const first = handlers.get('pty:serializeHeadlessBuffer')!(null, { id: 'pty-1' }) + const second = handlers.get('pty:serializeHeadlessBuffer')!(null, { id: 'pty-1' }) + expect(runtime.serializeHeadlessTerminalBufferForRenderer).toHaveBeenCalledTimes(1) + + resolveFirst({ data: 'first', cols: 80, rows: 24 }) + await expect(first).resolves.toEqual({ data: 'first', cols: 80, rows: 24 }) + await Promise.resolve() + expect(runtime.serializeHeadlessTerminalBufferForRenderer).toHaveBeenCalledTimes(2) + + resolveSecond({ data: 'second', cols: 80, rows: 24 }) + await expect(second).resolves.toEqual({ data: 'second', cols: 80, rows: 24 }) + }) }) }) diff --git a/src/main/ipc/pty.ts b/src/main/ipc/pty.ts index d75f3c3ea..3a5f425ee 100644 --- a/src/main/ipc/pty.ts +++ b/src/main/ipc/pty.ts @@ -658,8 +658,11 @@ export function registerPtyHandlers( // reduces IPC round-trips from hundreds/sec to ~120/sec under high // throughput. Keystroke echo/redraws bypass this below because agent TUIs // already spend tens of ms producing their redraw. + type HeadlessRendererSnapshot = { data: string; cols: number; rows: number } | null + const pendingData = new Map() - const headlessSnapshotHeldPtyIds = new Set() + const headlessSnapshotHoldCounts = new Map() + const headlessSnapshotQueues = new Map>() const trustedTerminalHandleEnv = new Set() let flushTimer: ReturnType | null = null const PTY_BATCH_INTERVAL_MS = 8 @@ -670,7 +673,7 @@ export function registerPtyHandlers( const hasFlushablePendingData = (): boolean => { for (const id of pendingData.keys()) { - if (!headlessSnapshotHeldPtyIds.has(id)) { + if (!headlessSnapshotHoldCounts.has(id)) { return true } } @@ -684,7 +687,7 @@ export function registerPtyHandlers( return } for (const [id, data] of pendingData) { - if (headlessSnapshotHeldPtyIds.has(id)) { + if (headlessSnapshotHoldCounts.has(id)) { continue } mainWindow.webContents.send('pty:data', { id, data }) @@ -720,6 +723,62 @@ export function registerPtyHandlers( return data } + const captureHeadlessSnapshotForRenderer = async ( + id: string, + opts: { scrollbackRows?: number } + ): Promise => { + if (!runtime) { + return null + } + // Why: hidden-pane reveal uses the headless snapshot as the authoritative + // paint. Hold any ≤8ms main-process PTY batches while serializing: a + // successful headless snapshot already contains them, while a null + // snapshot must release them so the renderer fallback can replay them. + const pendingBeforeSnapshot = takePendingDataForPty(id) + const holdCount = headlessSnapshotHoldCounts.get(id) ?? 0 + headlessSnapshotHoldCounts.set(id, holdCount + 1) + const releaseSnapshotHold = (): void => { + const current = headlessSnapshotHoldCounts.get(id) ?? 0 + if (current <= 1) { + headlessSnapshotHoldCounts.delete(id) + return + } + headlessSnapshotHoldCounts.set(id, current - 1) + } + let snapshot: HeadlessRendererSnapshot + try { + snapshot = await runtime.serializeHeadlessTerminalBufferForRenderer(id, opts) + } catch (err) { + const pendingDuringSnapshot = takePendingDataForPty(id) + releaseSnapshotHold() + sendPtyDataToRenderer(id, pendingBeforeSnapshot + pendingDuringSnapshot) + throw err + } + const pendingDuringSnapshot = takePendingDataForPty(id) + releaseSnapshotHold() + if (!snapshot) { + sendPtyDataToRenderer(id, pendingBeforeSnapshot + pendingDuringSnapshot) + } + return snapshot + } + + const queueHeadlessSnapshotForRenderer = ( + id: string, + opts: { scrollbackRows?: number } + ): Promise => { + const previous = headlessSnapshotQueues.get(id) + const next = previous + ? previous.catch(() => null).then(() => captureHeadlessSnapshotForRenderer(id, opts)) + : captureHeadlessSnapshotForRenderer(id, opts) + const tracked = next.finally(() => { + if (headlessSnapshotQueues.get(id) === tracked) { + headlessSnapshotQueues.delete(id) + } + }) + headlessSnapshotQueues.set(id, tracked) + return tracked + } + // Why: extracted so the "Restart daemon" flow can rebind against the fresh // adapter after replaceDaemonProvider runs. Both the startup registration // and the post-restart rebind go through the same code path — no risk of @@ -758,7 +817,7 @@ export function registerPtyHandlers( nextData.length <= INTERACTIVE_OUTPUT_MAX_CHARS && lastInputAt !== undefined && performance.now() - lastInputAt <= INTERACTIVE_OUTPUT_WINDOW_MS - if (isInteractiveOutput && !headlessSnapshotHeldPtyIds.has(payload.id)) { + if (isInteractiveOutput && !headlessSnapshotHoldCounts.has(payload.id)) { pendingData.delete(payload.id) clearFlushTimerIfIdle() // Why: agent TUIs redraw small prompt regions after every keystroke. @@ -1868,27 +1927,7 @@ export function registerPtyHandlers( ) { opts.scrollbackRows = Math.floor(args.scrollbackRows) } - // Why: hidden-pane reveal uses the headless snapshot as the authoritative - // paint. Hold any ≤8ms main-process PTY batches while serializing: a - // successful headless snapshot already contains them, while a null - // snapshot must release them so the renderer fallback can replay them. - const pendingBeforeSnapshot = takePendingDataForPty(args.id) - headlessSnapshotHeldPtyIds.add(args.id) - let snapshot: { data: string; cols: number; rows: number } | null - try { - snapshot = await runtime.serializeHeadlessTerminalBufferForRenderer(args.id, opts) - } catch (err) { - const pendingDuringSnapshot = takePendingDataForPty(args.id) - headlessSnapshotHeldPtyIds.delete(args.id) - sendPtyDataToRenderer(args.id, pendingBeforeSnapshot + pendingDuringSnapshot) - throw err - } - const pendingDuringSnapshot = takePendingDataForPty(args.id) - headlessSnapshotHeldPtyIds.delete(args.id) - if (!snapshot) { - sendPtyDataToRenderer(args.id, pendingBeforeSnapshot + pendingDuringSnapshot) - } - return snapshot + return queueHeadlessSnapshotForRenderer(args.id, opts) } ) diff --git a/src/main/ipc/shell.ts b/src/main/ipc/shell.ts index 4712828a5..37048c650 100644 --- a/src/main/ipc/shell.ts +++ b/src/main/ipc/shell.ts @@ -11,8 +11,7 @@ import { getSpawnArgsForWindows } from '../win32-utils' export const EXTERNAL_EDITOR_CLI_COMMAND = 'code' const REPO_ICON_IMAGE_MIME_TYPES: Record = { - '.png': 'image/png', - '.svg': 'image/svg+xml' + '.png': 'image/png' } async function pathExists(pathValue: string): Promise { @@ -248,7 +247,7 @@ export function registerShellHandlers(): void { async (): Promise<{ dataUrl: string; fileName: string } | null> => { const result = await dialog.showOpenDialog({ properties: ['openFile'], - filters: [{ name: 'Repo icon images', extensions: ['png', 'svg'] }] + filters: [{ name: 'Repo icon images', extensions: ['png'] }] }) if (result.canceled || result.filePaths.length === 0) { return null @@ -258,7 +257,7 @@ export function registerShellHandlers(): void { const extension = extname(filePath).toLowerCase() const mimeType = REPO_ICON_IMAGE_MIME_TYPES[extension] if (!mimeType) { - throw new Error('Repo icons must be PNG or SVG files.') + throw new Error('Repo icons must be PNG files.') } const stats = await stat(filePath) diff --git a/src/main/persistence.test.ts b/src/main/persistence.test.ts index f5239344f..f3a1c2fd7 100644 --- a/src/main/persistence.test.ts +++ b/src/main/persistence.test.ts @@ -1376,6 +1376,39 @@ describe('Store', () => { expect(store.getRepo('r1')!.displayName).toBe('renamed') }) + it('updateRepo drops repo icons that fail shared sanitization', async () => { + const store = await createStore() + store.addRepo(makeRepo()) + + const updated = store.updateRepo('r1', { + repoIcon: { + type: 'image', + source: 'upload', + src: 'data:image/svg+xml;base64,PHN2Zz48L3N2Zz4=' + } as never + }) + + expect(updated).not.toBeNull() + expect(updated!.repoIcon).toBeUndefined() + expect(store.getRepo('r1')!.repoIcon).toBeUndefined() + }) + + it('getRepo does not expose invalid persisted repo icons', async () => { + const store = await createStore() + store.addRepo( + makeRepo({ + repoIcon: { + type: 'image', + source: 'upload', + src: 'data:image/svg+xml;base64,PHN2Zz48L3N2Zz4=' + } as never + }) + ) + + expect(store.getRepo('r1')!.repoIcon).toBeUndefined() + expect(store.getRepos()[0]!.repoIcon).toBeUndefined() + }) + it('updateRepo returns null for nonexistent id', async () => { const store = await createStore() expect(store.updateRepo('nope', { displayName: 'x' })).toBeNull() diff --git a/src/main/persistence.ts b/src/main/persistence.ts index 65497208a..5e1be8718 100644 --- a/src/main/persistence.ts +++ b/src/main/persistence.ts @@ -93,6 +93,7 @@ import { normalizeWorkspaceStatuses } from '../shared/workspace-statuses' import { isLegacyRepoForExternalWorktreeVisibility } from '../shared/worktree-ownership' +import { sanitizeRepoIcon } from '../shared/repo-icon' function encrypt(plaintext: string): string { if (!plaintext || !safeStorage.isEncryptionAvailable()) { @@ -415,6 +416,21 @@ function readLegacySidekickFlag(parsed: PersistedState | undefined): boolean | u return (parsed?.settings as { experimentalSidekick?: boolean } | undefined)?.experimentalSidekick } +function sanitizeRepoUpdatesForPersistence>>( + updates: T +): T { + const sanitized = { ...updates } + if ('repoIcon' in sanitized) { + const repoIcon = sanitizeRepoIcon(sanitized.repoIcon) + if (repoIcon === undefined) { + delete sanitized.repoIcon + } else { + sanitized.repoIcon = repoIcon + } + } + return sanitized +} + function expandFloatingWorkspaceHomePath(input: string, home: string): string { if (input === '~') { return home @@ -2057,8 +2073,10 @@ export class Store { if (!repo) { return null } + const sanitizedUpdates = sanitizeRepoUpdatesForPersistence(updates) const externalWorktreeVisibilityLegacy = - 'externalWorktreeVisibility' in updates && repo.externalWorktreeVisibilityLegacy === undefined + 'externalWorktreeVisibility' in sanitizedUpdates && + repo.externalWorktreeVisibilityLegacy === undefined ? isLegacyRepoForExternalWorktreeVisibility(repo) : undefined // Why: `issueSourcePreference === undefined` in the patch means "reset to @@ -2066,15 +2084,18 @@ export class Store { // stale explicit value via Object.assign's skip-on-undefined behavior). // Without this delete branch, toggling explicit → auto would silently // leave the old preference in place on disk. - if ('issueSourcePreference' in updates && updates.issueSourcePreference === undefined) { + if ( + 'issueSourcePreference' in sanitizedUpdates && + sanitizedUpdates.issueSourcePreference === undefined + ) { delete repo.issueSourcePreference - const { issueSourcePreference: _drop, ...rest } = updates + const { issueSourcePreference: _drop, ...rest } = sanitizedUpdates Object.assign(repo, rest) } else { - Object.assign(repo, updates) + Object.assign(repo, sanitizedUpdates) } if ( - 'externalWorktreeVisibility' in updates && + 'externalWorktreeVisibility' in sanitizedUpdates && repo.externalWorktreeVisibilityLegacy === undefined ) { // Why: old persisted repos have no explicit marker. Stamp it the first @@ -2086,6 +2107,8 @@ export class Store { } private hydrateRepo(repo: Repo): Repo { + const { repoIcon: rawRepoIcon, ...repoWithoutIcon } = repo + const repoIcon = sanitizeRepoIcon(rawRepoIcon) const gitUsername = isFolderRepo(repo) ? '' : (this.gitUsernameCache.get(repo.path) ?? @@ -2096,7 +2119,8 @@ export class Store { })()) return { - ...repo, + ...repoWithoutIcon, + ...(repoIcon !== undefined ? { repoIcon } : {}), kind: isFolderRepo(repo) ? 'folder' : 'git', gitUsername, hookSettings: { diff --git a/src/renderer/src/components/settings/RepositoryIconPicker.tsx b/src/renderer/src/components/settings/RepositoryIconPicker.tsx index 5931d3afe..a0f90a20a 100644 --- a/src/renderer/src/components/settings/RepositoryIconPicker.tsx +++ b/src/renderer/src/components/settings/RepositoryIconPicker.tsx @@ -230,7 +230,7 @@ export function RepositoryIconPicker({ onClick={handleUploadImage} > - Upload PNG/SVG + Upload PNG