fix(playground): report structured TSL errors#75
Conversation
devallibus
left a comment
devallibus
left a comment
There was a problem hiding this comment.
LGTM. The shared PlaygroundErrorReport contract is the right abstraction — it unifies the error channel so TSL failures propagate as machine-readable structured errors alongside plain strings, without breaking the existing GLSL flow.
The tsl-error-reporting module is clean: TslPreviewError for known failures, SyntaxError → tsl-parse heuristic for module parse errors, and a fallback for everything else. All call sites updated consistently.
Four inline comments left — one nit about a redundant constructor in the WebGPU check, one noting the PlaygroundErrorReport/ErrorReportPayload type duplication, one optional missing test, and one future enhancement idea for GLSL structured errors. None block merge.
| if (!('gpu' in navigator)) { | ||
| setPreviewError('WebGPU is not available in this browser.') | ||
| setPreviewError( | ||
| createTslErrorReport( | ||
| new TslPreviewError('tsl-runtime', 'WebGPU is not available in this browser.'), | ||
| 'tsl-runtime', | ||
| 'WebGPU is not available in this browser.', |
There was a problem hiding this comment.
This is a bit roundabout — you construct a TslPreviewError just so createTslErrorReport can unwrap it back into the same kind and message. The message string appears three times.
Since the kind and message are fully known here, you could skip the round-trip:
setPreviewError({
errors: ['WebGPU is not available in this browser.'],
structuredErrors: [{ kind: 'tsl-runtime', message: 'WebGPU is not available in this browser.' }],
})Or if you prefer keeping the helper for consistency, at least the TslPreviewError wrapper carries the info without needing the redundant string args:
const err = new TslPreviewError('tsl-runtime', 'WebGPU is not available in this browser.')
setPreviewError(createTslErrorReport(err, 'tsl-runtime', err.message))nit — works correctly as-is
There was a problem hiding this comment.
Addressed in 19c2091. I added a direct createKnownTslErrorReport() helper and switched the WebGPU gate to use it, so there is no redundant TslPreviewError round-trip and the message only lives in one place.
| export function createTslErrorReport( | ||
| error: unknown, | ||
| fallbackKind: TslErrorKind, | ||
| fallbackMessage: string, | ||
| ): PlaygroundErrorReport { | ||
| const message = getMessage(error, fallbackMessage) | ||
| const kind = error instanceof TslPreviewError | ||
| ? error.kind | ||
| : error instanceof SyntaxError | ||
| ? 'tsl-parse' | ||
| : fallbackKind | ||
|
|
||
| return { | ||
| errors: [message], | ||
| structuredErrors: [{ kind, message }], | ||
| } |
There was a problem hiding this comment.
Clean classification chain. The TslPreviewError → SyntaxError → fallback priority is the right heuristic for catching TSL module errors at different stages (parse, runtime, material build).
One thing to be aware of: PlaygroundErrorReport here and ErrorReportPayload in playground-db.ts are structurally identical types with different names. They're compatible via structural typing so nothing breaks, but if you ever want to consolidate, PlaygroundErrorReport from playground-types.ts is the natural single source since it's already in the shared types file. The server could import it instead of defining ErrorReportPayload separately. Not blocking — just noting the duplication.
There was a problem hiding this comment.
Addressed in 19c2091. playground-db.ts now uses the shared PlaygroundErrorReport type directly instead of keeping a separate ErrorReportPayload alias.
| message: 'Unexpected token', | ||
| }], | ||
| }) | ||
| }) |
There was a problem hiding this comment.
Good coverage of the TslPreviewError and SyntaxError branches. One optional gap: a plain Error (not TslPreviewError, not SyntaxError) should use fallbackKind. Easy to add:
runTest('createTslErrorReport uses fallbackKind for generic errors', () => {
const report = createTslErrorReport(
new Error('something broke'),
'tsl-runtime',
'fallback',
)
assert.deepEqual(report, {
errors: ['something broke'],
structuredErrors: [{ kind: 'tsl-runtime', message: 'something broke' }],
})
})nit / optional
There was a problem hiding this comment.
Addressed in 19c2091. I added the plain Error coverage so the allbackKind path is now tested directly.
| } finally { | ||
| renderer.debug.onShaderError = previousShaderError | ||
| } | ||
|
|
There was a problem hiding this comment.
Worth noting for a future enhancement: GLSL errors go through createPlainErrorReport (no structured errors), even though webgl-shader-errors.ts already produces ShaderDiagnostic objects with kind: 'glsl-compile' | 'glsl-link' that map directly to PlaygroundError. If you wanted full structured coverage for both GLSL and TSL, you could map ShaderDiagnostic[] → PlaygroundError[] here instead of flattening to strings. Not in scope for this PR though.
There was a problem hiding this comment.
Kept out of scope for this PR, but I filed the follow-up here: #76. That issue tracks mapping ShaderDiagnostic[] through PlaygroundErrorReport so GLSL gets the same structured coverage as TSL.
1 participant
Summary
tsl-parse,tsl-runtime, ortsl-material-buildand persist them through the playground/errorsendpointVerification
bun run test:webbun run build:webthree/not-realimport) returned:compilationErrors: ["Unsupported TSL import source: three/not-real"]structuredErrors: [{ kind: "tsl-parse", message: "Unsupported TSL import source: three/not-real" }]Addresses #70.