Conversation
PR Review —
|
Re-review after update —
|
| # | Original nit | Resolution |
|---|---|---|
| 1 | Redundant variable aliasing | Removed — only const wgpuTag = tag remains (needed because TS can't narrow a type guard result across closure boundaries). |
| 2 | Inconsistent hidden attribute setting |
Now uses panel.hidden = true consistently with proper if (panel) guard. |
| 3 | hidden redeclared on HybridWgpuTagElement |
Removed from the interface. |
| 4 | Custom element upgrade race condition | Addressed with new setupElectrobunHybridWgpuWhenReady() that awaits customElements.whenDefined("electrobun-wgpu") before retrying. |
| 5 | No tests | hybrid-wgpu.test.ts added, covering the null-return guard path for both sync and async variants. CI step added. |
New code assessment
setupElectrobunHybridWgpuWhenReady() (hybrid-wgpu.ts:134–150): Clean two-phase approach — try sync first, if the tag isn't upgraded yet, await whenDefined then retry. The guard chain (typeof customElements === "undefined" → !document.querySelector(...) → typeof whenDefined !== "function") is defensive in the right places.
Non-null assertions in closures (hybrid-wgpu.ts:68–70): transparentButton!, passthroughButton!, maskButton!, mask! — these are safe since the closures only execute after the null guard at lines 37–48 succeeds. TS can't narrow const bindings into closures, so ! is the correct workaround here.
Test (hybrid-wgpu.test.ts): The globalThis.document/globalThis.customElements mocking with afterEach restore is solid for a Bun test environment without a real DOM. The mock correctly returns null for all elements except hybrid-panel, triggering the early-return path. The async variant test also validates that whenDefined resolving doesn't cause a false positive when the element still isn't present.
CI (ci.yml): bun run test added to the Electrobun smoke job — tests now run in CI before the build steps.
One minor observation (non-blocking)
setupElectrobunHybridWgpuWhenReady calls setupElectrobunHybridWgpu() twice (once eagerly, once after whenDefined). The first call sets panel.hidden = true if the tag isn't ready. The second call, on success, sets panel.hidden = false. This means there's a brief state where the panel is hidden then shown. For a showcase example this is perfectly fine — just noting it in case a future consumer needs instant visibility.
Verdict
Approve. The update is thorough and directly responsive. No remaining concerns.
Re-reviewed by claude-opus-4-6 (Claude Opus 4.6, 1M context)
Summary
<electrobun-wgpu>surfaceGpuWindowworkWhat changed
dev:electrobun:hybridandbuild:electrobun:hybridwhile keeping browser aliasesValidation
bunx tsc --noEmit -p examples/electrobun-counter/tsconfig.jsonbun run build:webinexamples/electrobun-counterbun run build:electrobun:hybridinexamples/electrobun-counterbun run build:electrobun:gpuinexamples/electrobun-counterTracking