Skip to content

fix(core): core stability (plugin duplicate ids, WebGPU disposal, error surfacing) - #1003

Open
ovurrsl wants to merge 5 commits into
pascalorg:mainfrom
ovurrsl:fix/core-stability
Open

ovurrsl wants to merge 5 commits into
pascalorg:mainfrom
ovurrsl:fix/core-stability

Conversation

@ovurrsl

@ovurrsl ovurrsl commented Oct 6, 2026 •

Copy link
Copy Markdown

fix(editor): stop mid-drag GPU disposal in the drag box and surface guide-image errors

Description

What does this PR do?

Two stability fixes in the editor:

  1. DragBoundingBox no longer creates and disposes GPU resources every frame. It previously rebuilt its BoxGeometry/PlaneGeometry on every size change and disposed the old ones in an effect cleanup. During a resize drag that happens once per frame, and WebGPU can still be executing a command buffer that references the destroyed buffers, so it drops the whole frame ("Vertex buffer slot … was not set"). The box now uses module-level unit geometry scaled through the mesh transform, plus materials cached by colour.
  2. Guide-image upload errors show the reason. Both guide-image upload paths (action-menu upload button and site panel) used a bare catch {}. They now log the error and show Could not add that guide image: <reason>, built by one shared guideImageErrorMessage helper in lib/local-guide-image.ts.

The earlier description also mentioned a plugin duplicate-id change and an insecure-context (crypto.randomUUID) fix. Both are out of this PR: the first only affects third-party plugin kinds, and upstream asset ids already come from nanoid.

The branch is merged with the current main.

How to test

  1. bun test packages/editor/src/lib/local-guide-image.test.ts.
  2. bun dev on a WebGPU browser: resize-drag an item or a cabinet with the bounding box showing. No frames blank out, and the box follows the size live.
  3. Make a guide-image upload fail (for example by blocking IndexedDB / storage). The error message now includes the reason.

Screenshots / screen recording

Checklist

  • I've tested this locally with bun dev
  • My code follows the existing code style (run bun check to verify)
  • I've updated relevant documentation (if applicable) (none needed)
  • This PR targets the main branch

Replies to bot review threads

  • Cursor Bugbot — shared module-level geometry/material disposed on unmount (needs dispose={null}): Checked against the versions in use; it does not occur. In @react-three/fiber 9.6.1, unmounting calls dispose() on the removed object only, and three 0.186's Object3D.dispose() only fires its dispose event, which lets the WebGPU renderer drop the per-object RenderObject. The shared UNIT_EDGES/UNIT_PLANE geometry and the cached materials are never disposed. dispose={null} would also skip that per-object cleanup, so it is left out.
  • pascal Bot / any thread that says the described plugin duplicate-id fix is missing: That change was taken out of this PR; it only affects third-party plugin kinds whose id prefix contains an underscore, which Pascal's own nodes never have. Title and description are updated.
  • Any thread on the stale "secure-context TypeError" comment: Removed.
  • Any thread on the duplicated error formatting in the two catch blocks: Moved into guideImageErrorMessage in lib/local-guide-image.ts, with a test.

…facing)

- fix: node id prefixes split at the wrong underscore, breaking plugin duplicate
- fix: surface upload failures, survive insecure contexts, stop mid-drag GPU disposal
@pascal

pascal Bot commented Oct 6, 2026

Copy link
Copy Markdown
Contributor

Summary

Three stability fixes in the editor package, all in packages/editor/src/components. The biggest one is in DragBoundingBox: it used to build a fresh BoxGeometry/PlaneGeometry and new materials whenever the dragged dimensions or colour changed, and dispose the previous ones in an effect cleanup. That is once per frame during a resize drag, so WebGPU could still be executing a command buffer pointing at buffers that had just been freed. The new version creates one unit edge geometry and one unit ground plane at module scope, caches materials in a Map keyed by colour, and moves the per-frame change into the mesh transform via position and scale. The other two changes are the same small fix applied in two places: the guide-image upload catch blocks in LevelReferences and UploadButton discarded the caught error, so a secure-context failure showed up as a generic "Could not add that guide image." They now log the error and append its message to the toast when there is one.

File Change What changed
packages/editor/src/components/tools/shared/drag-bounding-box.tsx modified Shared module-level unit geometry and cached materials replace per-frame geometry/material creation and disposal; sizing moves to scale, plane offset moves from baked translate to position
packages/editor/src/components/ui/action-menu/view-toggles.tsx modified Guide-image upload catch keeps the error: logs it and includes error.message in the toast
packages/editor/src/components/ui/sidebar/panels/site-panel/index.tsx modified Same error surfacing in LevelReferences, through useUploadStore.setError

One thing worth knowing while you read: the PR description lists an extractIdPrefix / plugin duplicate-id fix as item 1, but no such change appears in these three files.

Best place to start is drag-bounding-box.tsx, specifically whether the unit geometry plus scale reproduces the old dimensions exactly (the plane previously baked cx/groundY/cz into the geometry, and now gets them from position), and whether the never-disposed module-level geometry and material cache are the lifetime you want.

ovurrsl and others added 4 commits October 8, 2026 14:22
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FX8vGAP5g43AHR1jixUeXr
…odes

generateId builds ids as `${prefix}_${suffix}` with a suffix drawn from
0-9a-z, but the three copies of extractIdPrefix (core registry/subtree,
core utils/clone-scene-graph, editor scene-clipboard) split at the FIRST
underscore. Built-in prefixes have none, so only plugin kinds noticed: a
duplicate of `pallet_rack_<suffix>` came back as `pallet_<suffix>`, which
the kind's own id template rejects.

Replace the copies with one nodeIdPrefix next to generateId that splits at
the last underscore. Tests cover the helper, cloneNodesInto,
cloneSceneGraph / cloneLevelSubtree and the editor's fresh-subtree
duplicate of a registered plugin kind; all of them fail on the old split.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FX8vGAP5g43AHR1jixUeXr
Both guide-image upload paths formatted the same "Could not add that guide
image: <reason>" message and logged the error; move that into
guideImageErrorMessage next to createLocalGuideImage. This also drops the
stale secure-context comment: upstream asset ids already come from nanoid,
so that TypeError no longer exists.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FX8vGAP5g43AHR1jixUeXr
The underscore split only matters for third-party plugin kinds whose id
prefix contains an underscore; Pascal's own nodes never do, so it does not
belong in an upstream stability fix.

This reverts commit dc98059.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FX8vGAP5g43AHR1jixUeXr

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant