fix(playground): wait for browser error reports#73
fix(playground): wait for browser error reports#73devallibus wants to merge 2 commits intofix/playground-glsl-error-reportingfrom
Conversation
devallibus
left a comment
There was a problem hiding this comment.
LGTM. The waitForBrowserSyncResult state machine is correct across all scenarios — early break on compilation errors, waiting for both signals on success, and clean timeout handling. The ~96ms vs ~5s improvement is a great UX win.
One actionable item: setErrors is an unused import in $.ts — should be removed. The rest is optional nits (test gaps, clarifying comments).
| @@ -5,11 +5,11 @@ import { | |||
| updateShader, | |||
| setScreenshot, | |||
| setErrors, | |||
There was a problem hiding this comment.
setErrors is still imported (line 7) but no longer used directly — the /errors POST handler now goes through recordErrorReport, and no other code path calls it. Should be removed.
| export async function waitForBrowserSyncResult(sessionId: string, timeoutMs: number): Promise<{ | ||
| screenshotBase64: string | null | ||
| errorReport: ErrorReportPayload | null | ||
| }> { | ||
| const screenshotPromise = waitForScreenshot(sessionId, timeoutMs).then((base64) => ({ | ||
| type: 'screenshot' as const, | ||
| base64, | ||
| })) | ||
| const errorPromise = waitForErrorReport(sessionId, timeoutMs).then((report) => ({ | ||
| type: 'errorReport' as const, | ||
| report, | ||
| })) | ||
|
|
||
| let waitForScreenshotEvent = true | ||
| let waitForErrorEvent = true | ||
| let screenshotBase64: string | null = null | ||
| let errorReport: ErrorReportPayload | null = null | ||
|
|
||
| while (waitForScreenshotEvent || waitForErrorEvent) { | ||
| const pending: Array< | ||
| Promise< | ||
| | { type: 'screenshot'; base64: string | null } | ||
| | { type: 'errorReport'; report: ErrorReportPayload | null } | ||
| > | ||
| > = [] | ||
|
|
||
| if (waitForScreenshotEvent) pending.push(screenshotPromise) | ||
| if (waitForErrorEvent) pending.push(errorPromise) | ||
|
|
||
| const next = await Promise.race(pending) | ||
|
|
||
| if (next.type === 'screenshot') { | ||
| waitForScreenshotEvent = false | ||
| screenshotBase64 = next.base64 | ||
| } else { | ||
| waitForErrorEvent = false | ||
| errorReport = next.report | ||
| } | ||
|
|
||
| const hasCompilationErrors = !!errorReport | ||
| && (errorReport.errors.length > 0 || errorReport.structuredErrors.length > 0) | ||
| const hasSuccessfulSync = screenshotBase64 !== null && errorReport !== null | ||
|
|
||
| if (hasCompilationErrors || hasSuccessfulSync) { | ||
| break | ||
| } | ||
| } | ||
|
|
||
| return { screenshotBase64, errorReport } |
There was a problem hiding this comment.
The state machine here is solid — I traced through all five scenarios:
- Error with messages first →
hasCompilationErrorsbreaks immediately (~96ms path) - Empty error first, then screenshot → waits for screenshot after empty error
- Screenshot first, then empty error → waits for error report after screenshot
- Both timeout → both flags become false, while loop exits naturally
- Screenshot first, then error with messages →
hasCompilationErrorsbreaks with both
One thing to note: when breaking early on compilation errors, the screenshot waiter remains registered until the 5s timeout fires. Functionally harmless (promise resolves to null, nobody reads it, waiter cleaned up on timeout). Not worth adding cancellation complexity.
Also: browserResult.errorReport is unused by the API handler — it re-fetches from DB instead. This is correct since recordErrorReport persists before resolving waiters. A brief comment in the API handler could make that intent clearer for future readers, but optional.
| runTest('waitForBrowserSyncResult resolves early on compilation errors', async () => { | ||
| const { id } = createSession() | ||
| const startedAt = Date.now() | ||
| const wait = waitForBrowserSyncResult(id, 1_000) | ||
|
|
||
| setTimeout(() => { | ||
| recordErrorReport(id, { | ||
| errors: ['compile failed'], | ||
| structuredErrors: [], | ||
| }) | ||
| }, 20) | ||
|
|
||
| const result = await wait | ||
| assert.equal(Date.now() - startedAt < 500, true) | ||
| assert.equal(result.screenshotBase64, null) | ||
| assert.deepEqual(result.errorReport, { | ||
| errors: ['compile failed'], | ||
| structuredErrors: [], | ||
| }) | ||
| }) |
There was a problem hiding this comment.
Good coverage of the key scenarios — error-first, screenshot-first, and the interleaved orderings.
Two optional gaps if you want full coverage:
- Timeout scenario: neither event arrives within the window → returns
{ screenshotBase64: null, errorReport: null } structuredErrorsonly: error report with emptyerrorsbut non-emptystructuredErrors— thehasCompilationErrorscheck handles it correctly but there's no test exercising that branch
nit / optional
Summary
/api/playground/:sessionId/updateresolve on actual browser feedback instead of idling on the screenshot timeoutVerification
bun run test:webbun run build:webCloses #71.
Stacked on #72.