Skip to content

fix(items): don't clobber a same-commit settled flag (cached-model race) #485

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wass08 wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix(items): don't clobber a same-commit settled flag (cached-model race) #485

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
56 changes: 44 additions & 12 deletions packages/nodes/src/item/renderer.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -217,10 +217,10 @@ const MODEL_RETRY_DELAYS_MS = [1_000, 3_000]
*/
const ModelWithRetry = ({
node,
setSettled,
settleControl,
}: {
node: ItemNode
setSettled: (value: boolean) => void
settleControl: SettleControl
}) => {
// `failures` counts boundary catches; `epoch` bumps after each cache clear
// to reset the boundary and re-mount the loader. The retry timer is owned by
Expand All @@ -236,9 +236,14 @@ const ModelWithRetry = ({

const handleError = useCallback(() => setFailures((current) => current + 1), [])

// Un-settle ONLY when the URL actually changed. A cached model mounts in the
// SAME commit as this wrapper, and React runs child effects first — a blind
// `settle(false)` here would clobber the child's just-written settled flag
// and the item would stay "pending" forever (observed on the bake worker,
// whose shared warm HTTP cache makes most models mount synchronously).
useEffect(() => {
setSettled(false)
}, [setSettled])
settleControl.unsettleForUrl(url)
}, [settleControl, url])

useEffect(() => {
if (failures === 0 || gaveUp) return
Expand All @@ -251,7 +256,7 @@ const ModelWithRetry = ({
return () => clearTimeout(timer)
}, [failures, gaveUp, url])

const markSettled = useCallback(() => setSettled(true), [setSettled])
const markSettled = useCallback(() => settleControl.settle(url), [settleControl, url])

useEffect(() => {
if (!gaveUp) return
Expand All @@ -271,6 +276,15 @@ const ModelWithRetry = ({
)
}

type SettleControl = {
/** The model for `url` is resolved (or terminally skipped) — build work done. */
settle: (url: string) => void
/** A load for `url` is starting; drop the settled flag unless it was already
* settled for this same URL (a cached model settles in the same commit,
* child-effect-first — that write must win). */
unsettleForUrl: (url: string) => void
}

export const ItemRenderer = ({ node: storeNode }: { node: ItemNode }) => {
const ref = useRef<Group>(null!)

Expand All @@ -281,10 +295,24 @@ export const ItemRenderer = ({ node: storeNode }: { node: ItemNode }) => {
// (and headless bakes) wait for real item content instead of exporting the
// loading placeholder. A model swap un-settles (ModelWithRetry's mount
// effect via its URL key) so the replacement load is awaited too.
const setSettled = useCallback((value: boolean) => {
const group = ref.current as (Group & { userData: Record<string, unknown> }) | null
if (group) group.userData.itemModelSettled = value
}, [])
const settleControl = useMemo<SettleControl>(
() => ({
settle: (url) => {
const group = ref.current as (Group & { userData: Record<string, unknown> }) | null
if (!group) return
group.userData.itemModelSettled = true
group.userData.itemModelSettledUrl = url
},
unsettleForUrl: (url) => {
const group = ref.current as (Group & { userData: Record<string, unknown> }) | null
if (!group) return
if (group.userData.itemModelSettled && group.userData.itemModelSettledUrl === url) return
group.userData.itemModelSettled = false

@cursor cursor Bot Jul 10, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remount preserves stale settled flag

High Severity

When ModelWithRetry remounts (e.g. node.asset.src key change) but resolveCdnUrl yields the same URL, unsettleForUrl returns early if itemModelSettled is already true for that URL. Stale userData on the parent group can survive from a prior terminal skip or load, so ItemSystem may clear the dirty mark while the GLB is still loading and scene-ready can fire too early for bakes.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit ab3db67. Configure here.

group.userData.itemModelSettledUrl = url
},
}),
[],
)

// Merge live drag overrides so the mesh transforms in real time during a
// drag (e.g. the in-world rotate gizmo). The handle writes the in-flight
Expand All @@ -300,16 +328,20 @@ export const ItemRenderer = ({ node: storeNode }: { node: ItemNode }) => {
(node as ItemNode & { roomClearPreview?: unknown }).roomClearPreview === true

useEffect(() => {
if (roomClearPreview) setSettled(true)
}, [roomClearPreview, setSettled])
if (roomClearPreview) settleControl.settle('room-clear-preview')
}, [roomClearPreview, settleControl])

const content = (
<group position={node.position} ref={ref} rotation={node.rotation} visible={node.visible}>
{roomClearPreview ? (
<ClearPreviewModel node={node} />
) : (
<>
<ModelWithRetry key={node.asset.src ?? 'no-src'} node={node} setSettled={setSettled} />
<ModelWithRetry
key={node.asset.src ?? 'no-src'}
node={node}
settleControl={settleControl}
/>
{node.children?.map((childId) => (
<NodeRenderer key={childId} nodeId={childId} />
))}
Expand Down
Loading