Skip to content

Commit b2aed9c

Browse files
committed
fix(desktop): close review edge cases
1 parent 0926849 commit b2aed9c

20 files changed

Lines changed: 300 additions & 65 deletions

File tree

apps/desktop/src/main/browser-agent/panel.test.ts

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -126,6 +126,22 @@ describe('panel chat scope', () => {
126126
expect(view.setBounds).not.toHaveBeenCalled()
127127
})
128128

129+
it('recovers when capturePage throws before returning a promise', async () => {
130+
const { win, view } = showPanel(panel)
131+
const scopeId = panel.getActivePanelScopeId()
132+
const image = await view.webContents.capturePage()
133+
vi.mocked(view.webContents.capturePage).mockImplementationOnce(() => {
134+
throw new Error('WebContents was destroyed')
135+
})
136+
137+
await expect(panel.capturePanelSnapshot(win, scopeId)).resolves.toBeNull()
138+
139+
vi.mocked(view.webContents.capturePage).mockResolvedValue(image)
140+
await expect(panel.capturePanelSnapshot(win, scopeId)).resolves.toMatchObject({
141+
dataUrl: 'data:image/png;base64,c2lt',
142+
})
143+
})
144+
129145
it('shares one native capture across concurrent requests for the same frame', async () => {
130146
const { win, view } = showPanel(panel)
131147
const scopeId = panel.getActivePanelScopeId()

apps/desktop/src/main/browser-agent/panel.ts

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -627,8 +627,16 @@ export async function capturePanelSnapshot(
627627

628628
occludableFrame = null
629629
const generation = ++panelCaptureGeneration
630-
const promise = contents
631-
.capturePage(undefined, { stayHidden: false })
630+
let capture: ReturnType<typeof contents.capturePage>
631+
try {
632+
capture = contents.capturePage(undefined, { stayHidden: false })
633+
} catch (error) {
634+
logger.warn('Could not capture browser panel for a toolbar menu', {
635+
error: getErrorMessage(error, 'unknown'),
636+
})
637+
return null
638+
}
639+
const promise = capture
632640
.then((image): BrowserPanelSnapshot | null => {
633641
const imageSize = image.getSize()
634642
if (

apps/desktop/src/main/browser-agent/session.test.ts

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2171,6 +2171,18 @@ describe('browser-agent session', () => {
21712171
})
21722172
).toBe(false)
21732173

2174+
const staleGestureCallback = vi.fn()
2175+
requestHandler(contents, 'media', staleGestureCallback, {
2176+
isMainFrame: true,
2177+
mediaTypes: ['audio'],
2178+
requestingUrl: 'https://example.com/',
2179+
securityOrigin: 'https://example.com',
2180+
})
2181+
expect(staleGestureCallback).toHaveBeenCalledWith(false)
2182+
expect(
2183+
session.mediaPermissionRequestForContents(contents as unknown as WebContents)
2184+
).toBeUndefined()
2185+
21742186
// Chromium routes navigator.clipboard.writeText through this one; denying
21752187
// it silently broke every copy button that does not use execCommand.
21762188
const writeCallback = vi.fn()

apps/desktop/src/main/browser-agent/session.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -943,6 +943,7 @@ function settleMediaPermission(tab: AgentTab, allowed: boolean): boolean {
943943
function revokeTabMediaPermissions(tab: AgentTab, publish = true): void {
944944
const hadPrompt = settleMediaPermission(tab, false)
945945
tab.mediaPermissionGrant = undefined
946+
tab.lastRealUserGestureAt = undefined
946947
if (hadPrompt && publish) publishPageIssue(tab)
947948
}
948949

apps/desktop/src/main/browser-agent/url-guard.test.ts

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -248,6 +248,22 @@ describe('isBlockedSubresourceUrl', () => {
248248
expect(mockLookup).toHaveBeenCalledTimes(24)
249249
})
250250

251+
it('bounds queued requests by the original DNS deadline', async () => {
252+
vi.useFakeTimers()
253+
try {
254+
mockLookup.mockReturnValue(new Promise(() => {}))
255+
const verdicts = Array.from({ length: 16 }, (_, index) =>
256+
isBlockedSubresourceUrl(`https://slow-${index}.example/app.js`)
257+
)
258+
await vi.advanceTimersByTimeAsync(5_000)
259+
260+
await expect(Promise.all(verdicts)).resolves.toEqual(Array(16).fill(true))
261+
expect(mockLookup).toHaveBeenCalledTimes(8)
262+
} finally {
263+
vi.useRealTimers()
264+
}
265+
})
266+
251267
it('treats a trailing-dot host as the same host', async () => {
252268
await isBlockedSubresourceUrl('https://example.com/a.js')
253269
await isBlockedSubresourceUrl('https://example.com./b.js')

apps/desktop/src/main/browser-agent/url-guard.ts

Lines changed: 22 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
import { createLogger } from '@sim/logger'
2-
import { resolveHostAddresses } from '@sim/security/dns'
2+
import { DEFAULT_DNS_TIMEOUT_MS, DnsTimeoutError, resolveHostAddresses } from '@sim/security/dns'
33
import {
44
isIpLiteral,
55
isLoopbackIp,
@@ -157,15 +157,29 @@ const MAX_QUEUED_DNS_LOOKUPS = 64
157157
let activeDnsLookups = 0
158158
const dnsLookupWaiters: Array<() => void> = []
159159

160-
async function acquireDnsLookupSlot(): Promise<void> {
160+
async function acquireDnsLookupSlot(host: string, deadline: number): Promise<void> {
161161
if (activeDnsLookups < MAX_CONCURRENT_DNS_LOOKUPS) {
162162
activeDnsLookups++
163163
return
164164
}
165165
if (dnsLookupWaiters.length >= MAX_QUEUED_DNS_LOOKUPS) {
166166
throw new Error('DNS lookup queue is full')
167167
}
168-
await new Promise<void>((resolve) => dnsLookupWaiters.push(resolve))
168+
const remainingMs = deadline - Date.now()
169+
if (remainingMs <= 0) throw new DnsTimeoutError(host)
170+
171+
await new Promise<void>((resolve, reject) => {
172+
const grant = () => {
173+
clearTimeout(timer)
174+
resolve()
175+
}
176+
const timer = setTimeout(() => {
177+
const index = dnsLookupWaiters.indexOf(grant)
178+
if (index >= 0) dnsLookupWaiters.splice(index, 1)
179+
reject(new DnsTimeoutError(host))
180+
}, remainingMs)
181+
dnsLookupWaiters.push(grant)
182+
})
169183
}
170184

171185
function releaseDnsLookupSlot(): void {
@@ -178,9 +192,12 @@ function releaseDnsLookupSlot(): void {
178192
}
179193

180194
async function resolveHostAddressesBounded(host: string) {
181-
await acquireDnsLookupSlot()
195+
const deadline = Date.now() + DEFAULT_DNS_TIMEOUT_MS
196+
await acquireDnsLookupSlot(host, deadline)
182197
try {
183-
return await resolveHostAddresses(host)
198+
const remainingMs = deadline - Date.now()
199+
if (remainingMs <= 0) throw new DnsTimeoutError(host)
200+
return await resolveHostAddresses(host, { timeoutMs: remainingMs })
184201
} finally {
185202
releaseDnsLookupSlot()
186203
}

apps/desktop/src/main/browser-sites/directory.test.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -304,7 +304,7 @@ describe('SiteDirectory', () => {
304304
expect(await store.list()).toEqual([{ hostname: 'github.com', name: 'GitHub' }])
305305
})
306306

307-
it('blocks stored site records with fields outside the persistence contract', async () => {
307+
it('blocks stored site records with invalid field values', async () => {
308308
const payload = [{ hostname: 'github.com', visits: -1 }]
309309
const original = JSON.stringify({
310310
version: 2,

apps/desktop/src/main/desktop-chat-session-store.test.ts

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -121,6 +121,23 @@ describe('DesktopChatSessionStore', () => {
121121
expect(statSync(filePath).mode & 0o077).toBe(0)
122122
})
123123

124+
it('does not replace the durable store with an oversized encrypted envelope', () => {
125+
const provider = encryption()
126+
const store = open(provider)
127+
store.setTerminal(ORIGIN, 'chat-existing', TERMINAL)
128+
expect(store.flush()).toBe(true)
129+
const existing = readFileSync(filePath, 'utf8')
130+
131+
vi.mocked(provider.encryptString).mockReturnValueOnce(Buffer.alloc(8 * 1024 * 1024))
132+
store.setTerminal(ORIGIN, 'chat-new', TERMINAL)
133+
134+
expect(store.flush()).toBe(false)
135+
expect(readFileSync(filePath, 'utf8')).toBe(existing)
136+
137+
expect(store.flush()).toBe(true)
138+
expect(readFileSync(filePath, 'utf8')).not.toBe(existing)
139+
})
140+
124141
it('keeps a pending chat in memory until migration promotes it to a durable chat id', () => {
125142
const provider = encryption()
126143
const pending = open(provider)

apps/desktop/src/main/desktop-chat-session-store.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -541,6 +541,7 @@ export class DesktopChatSessionStore {
541541
v: STORE_VERSION,
542542
ciphertext: this.encryption.encryptString(JSON.stringify(payload)).toString('base64'),
543543
}
544+
if (Buffer.byteLength(JSON.stringify(envelope), 'utf8') > MAX_STORE_BYTES) return false
544545
writeJsonFileAtomicallySync(this.filePath, envelope)
545546
this.dirty = false
546547
return true

apps/desktop/src/main/index.ts

Lines changed: 8 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -151,6 +151,7 @@ function main(): void {
151151
let resumingQuitAfterTeardown = false
152152
let tray: TrayHandle | null = null
153153
let updater: UpdaterHandle | null = null
154+
let startupReady: Promise<void> | null = null
154155
const configuredPartitions = new Set<string>()
155156

156157
const allowHttpLocalhost = () => !app.isPackaged || appOrigin().startsWith('http://')
@@ -540,6 +541,7 @@ function main(): void {
540541
return [stores[index].label]
541542
})
542543
},
544+
canCompleteDeploymentScopedStateChange: () => getAccountDataTeardownKind() === 'deployment',
543545
completeDeploymentScopedStateChange: completeDeploymentScopedTeardown,
544546
relaunch: relaunchApp,
545547
})
@@ -557,7 +559,7 @@ function main(): void {
557559
}
558560

559561
app.on('second-instance', () => {
560-
void app.whenReady().then(() => createAndLoadAppWindow())
562+
void (startupReady ?? app.whenReady()).then(() => createAndLoadAppWindow())
561563
})
562564

563565
app.on('window-all-closed', () => {
@@ -606,12 +608,13 @@ function main(): void {
606608
})
607609

608610
app.on('activate', () => {
609-
if (app.isReady() && !getMainWindow()) {
610-
void ensureMainWindow()
611-
}
611+
if (!app.isReady()) return
612+
void (startupReady ?? app.whenReady()).then(() => {
613+
if (!getMainWindow()) return ensureMainWindow()
614+
})
612615
})
613616

614-
void app.whenReady().then(async () => {
617+
startupReady = app.whenReady().then(async () => {
615618
// Packaged apps keep their native bundle icon so the Dock appearance does
616619
// not change when the process starts. Unpackaged runs have no branded
617620
// bundle, so they still need the channel-specific development icon.

0 commit comments

Comments
 (0)