Skip to content

refactor(): type the requests workers send to the main thread - #330

Draft
ShaMan123 wants to merge 33 commits into
ThatOpen:mainfrom
ShaMan123:refactor/typed-worker-requests
Draft

ShaMan123 wants to merge 33 commits into
ThatOpen:mainfrom
ShaMan123:refactor/typed-worker-requests

Conversation

@ShaMan123

@ShaMan123 ShaMan123 commented Oct 11, 2026 •

Copy link
Copy Markdown
Contributor

Part of a stack of six PRs that replaces #305. The branches live in a fork, so GitHub can't chain them: every PR is based on main, and the diff of each one includes the PRs below it. Each PR names the commits that are its own and links a comparison that shows only those. They are meant to merge in the order below, and PRs 2 to 6 stay drafts until the one before them is in.

The work. All model state, on both threads, is keyed by the consumer's modelId, and a modelId can be reused. Late async work of a model that was disposed or replaced (a worker reply, queued tiles, a material transfer, a progress event, an in-flight dispose) therefore lands on the next model loaded under the same ID. The stack gives each model an internal uid that is never reused and routes everything by it, adds load({ signal }) (#173), and types the messages between the threads, where most of the bugs it fixes were hidden behind any.

The stack, bottom to top, in merge order:

  1. refactor(): route model calls through _invoke #325 refactor(): route model calls through _invoke: mechanical groundwork, no behavior change
  2. fix(): type calls to a model's worker, rebuild what the worker copies #326 fix(): type calls to a model's worker, rebuild what the worker copies
  3. feat(): load abort signal + harden the message layer #327 feat(): load abort signal + harden the message layer
  4. fix(): key models by an internal uid, dispose synchronously #328 fix(): key models by an internal uid, dispose synchronously: the core fix
  5. fix(edit): key editor state by the model's uid #329 fix(edit): key editor state by the model's uid
  6. refactor(): type the requests workers send to the main thread #330 refactor(): type the requests workers send to the main thread

Every commit in the stack type-checks and passes the test suite on its own, and each PR builds. The top of the stack matches #305's branch merged with today's main, apart from one fix that merge needs (see PR 4) and two reworded lines.


This PR's own diff: its last 4 commits, shown here on their own. The commits before them belong to the PRs below.

Description

PR 6 of 6. PR 2 typed the calls the main thread makes to a worker. This PR types the other direction: what the workers send the main thread, and how the main thread reads it.

  • WorkerRequest types the requests workers send, and the workers' senders are checked against it with satisfies. Connection<TInput> and ThreadHandler<T> are generic over what they receive.
  • TileRequest types the tile requests inside a RECOMPUTE_MESHES, with one member per TileRequestClass. They were any[], built in four places on the worker and read in six classes on the main thread.
  • Received messages are typed as Cloned, requests and answers alike. RequestsManager, MeshManager, LODManager and MaterialManager read them as any until now. The tile matrix and bounding box they receive as copies are rebuilt instead of being passed on as THREE objects.
  • HighlightDefinition types a highlight as it is. getHighlight() resolved to MaterialDefinition[], but it has an entry for every item it looks up, undefined for an item without a highlight, and a highlight that keeps the item's own material only holds the properties it changes, so it may have no color. Both were cast away.

From the review of #305: the fetchMeshCompute catch. manageRequest is now the connection's handler itself, so the connection awaits it and answers a request it fails to handle with an error, which it also logs. The workers send these requests fire-and-forget (fetchMeshCompute, material transfers, load progress), so they now catch that answer instead of leaving an unhandled rejection.

Public type changes

  • getHighlight() resolves to (HighlightDefinition | undefined)[], and so does getSequenced() for "highlight".
  • highlight() accepts a HighlightDefinition, so a highlight read with getHighlight() can be applied again.
  • WorkerRequest, TileRequest and HighlightDefinition are exported.

Removed as dead code: the THROW_ERROR branch and the templateId reads. No worker sends either.

Additional context

This PR only needs PR 4. It doesn't overlap PR 5: the two apply in either order with the same result, so this one can move ahead of PR 5 if that one takes longer.

23 source files, +463/−253. Tests: 198 pass, 1 of them new.


What is the purpose of this pull request?

  • Bug fix
  • New Feature
  • Documentation update
  • Other

Before submitting the PR, please make sure you do the following:

  • Check that there isn't already a PR that solves the problem the same way to avoid creating a duplicate.
  • Follow the Conventional Commits v1.0.0 standard for PR naming (e.g. feat(examples): add hello-world example).
  • Provide a description in this PR that addresses what the PR is solving, or reference the issue that it solves (e.g. fixes #123).
  • Ideally, include relevant tests that fail without this PR but pass with it.

🤖 Generated with Claude Code

ShaMan123 and others added 30 commits October 11, 2026 06:29
Every call to a model's worker goes through FragmentsModel._invoke()
instead of spelling out threads.invoke(model.modelId, …) at each of the
89 call sites. The model is now the one place that knows how its worker
is addressed, which is what the typing of these calls and the internal
uid build on.

Mechanical, with one behavior detail: the calls to a delta model go
through the delta model itself, looked up with _getDeltaModel(), instead
of invoking its modelId. A deltaModelId whose model is no longer loaded
now reads as "no edits" where it sent a request for a missing model.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
FragmentsModel._invoke() only takes names of VirtualFragmentsModel's
methods, with arguments matching their parameters, and resolves to their
return type. A renamed or retyped worker method no longer fails at
runtime only.

It showed worker methods declaring narrower parameters than they handle:
the highlight and visibility methods take no ids to mean every item, and
getItemsData() takes GUIDs too. Their signatures now say so. Results are
still cast at the call sites: they are copied between threads, so class
instances in them arrive as plain objects.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
FragmentsModel.getItemsGeometry() and getGeometries() returned MeshData
whose `transform`, typed as a THREE.Matrix4, was the plain object the
worker's copy arrives as, so calling a Matrix4 method on it threw. They
now rebuild it, as ItemGeometry.get() already did.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
_invoke is typed now, and every one of these arguments matches its worker
method, so the comments only switched off the argument checking.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
_invoke claimed to resolve to what the worker method returns, but the
worker's answer is a structured-clone copy: class instances in it arrive
without their prototype. And nothing checked the claim, because
FragmentsConnection.fetch/invoke resolved to `any`.

Cloned<T> describes that copy: methods are dropped, so a THREE.Matrix4 is
typed as `{ elements, isMatrix4 }` and no longer passes for one. invoke<R>()
resolves to Cloned<R>, with R (default any) the method's return type, and
FragmentsConnection.fetch keeps the request's type instead of dropping it,
so the execute and box requests now declare the fields the worker answers
with.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
_invoke accepted any public method of VirtualFragmentsModel, including
ones that break over the wire: dispose() deleted the worker-side model
behind the main thread's back, and getItemsByConfig() takes a function,
which can't be copied to the worker. It now takes only the methods listed
in RemoteMethods, which also documents what crosses the thread boundary.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
getHighlight() and getItemsMaterialDefinition() returned definitions whose
`color`, typed as a THREE.Color, was the plain copy the worker's answer
arrives as, so calling a THREE.Color method on it threw. resetColors()
meant to rebuild it but checked `isColor`, which the copy keeps as an own
property, so it never did, and getItemsMaterialDefinition() didn't call it.

MaterialManager.restoreColor() rebuilds the color from a copied definition
and is typed that way, so a definition can't be handed out without it. It
keeps the components as they are: the worker already converted them from
sRGB, and the old fallback would have converted them again. Definitions
the worker transfers for rendering go through it too.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…copy

EditManager.restoreTransforms() takes the geometries as they arrive from
the worker and returns them with their transform rebuilt, so dropping the
rebuild is now a type error rather than a cast that hides it. ItemGeometry
uses it instead of its own copy of the loop.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
getBuffer() is compressed by default, and compressed it resolves to a
Uint8Array, but it was cast to an ArrayBuffer, so `new DataView(buffer)`
type-checked and threw. The cast couldn't simply go: the worker method's
inferred type, ArrayBufferLike, absorbs Uint8Array too. The worker method
now declares both, which is also what IFragmentsModel says.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Item.getAttributes() cast the worker's attributes to `type?: number`, and
AttributeData and ItemAttributes.setType() took a number too, but the
types are the names the importer writes, such as "IFCLABEL", and the
worker declares them as strings. A numeric comparison against them never
matched. They are strings now, and the cast is gone.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
getSpatialStructure(), getMetadata(), getCRS() and getSequenced() on the
worker returned `any`, so the casts on the main thread were the only type
their results had. They now declare what they return.

That showed getSequenced() handing out results as the worker's copies:
"mergedBoxes" typed as a THREE.Box3, "geometry" transforms as THREE.Matrix4
and "highlight" colors as THREE.Color were all plain objects. Each kind of
result is now rebuilt from the copy, by a map keyed by the result kind, so
a kind that needs rebuilding can't be left out. It resolves to null when
the worker doesn't know the result kind, as its documentation says.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Most of them claimed exactly what _invoke already resolves to, each as a
separate unchecked claim, so a worker method changing its result would go
unnoticed. Where a result is handed out under a named type (ItemData,
SpatialTreeItem, the raw data maps, ...), a return annotation keeps that
name in the declarations and is checked against the worker's result.

The casts that stay are the index methods' and getMetadata()'s: their type
parameters are the caller's word for an index's or the metadata's shape,
which the worker can't check either.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
getMetadata() promises an object (IFragmentsModel types it as T), but the
worker answered null for a model whose file has no metadata, which the
main thread's cast passed through. It answers {} now, so the promise
holds without every caller having to check for null. getCRS() still
answers null for a model without a CRS.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The answer was posted outside the try/catch, so a result postMessage()
can't copy, such as one holding a function, threw a DataCloneError as an
unhandled rejection and the request never settled. The request is now
answered with that error instead.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A model is registered on its worker before its generate() loop ends, so a
DELETE_MODEL that landed mid-load, from disposing a model while it loads,
disposed the half-built model from under that loop. The delete now aborts
the load, waits for it to unwind and clean up after itself, and answers
after that.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Both threads key a model's state by its modelId, so a second load under an
ID that was loaded or still loading silently clobbered the first model.
load() now throws for it. abort(modelId) relies on it too: an ID names at
most one load in flight.

Only models in `models.list` count. An ID whose model was disposed can be
loaded again as before, including while its worker still deletes it.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
abort(modelId) kept its own account of the loads in flight and of the
ones already aborted, in two sets of IDs next to the caller's signal. A
load now owns one AbortController that the signal and abort(modelId)
both go through. abort(modelId) only looks up the load running under
that ID, so it needs no bookkeeping of its own, and an abort can't
outlive its load or reach a later load of the same ID.

An abort also ends the load's wait for the worker's step right away
instead of when the worker's answer comes back, and goes on to dispose
the partial model. load() still rejects once that model is disposed.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Disposing a model while it loads, with dispose(), disposeModel() or
FragmentsModels.dispose(), left its load running: load() carried on and,
with autoCoordinate off, fired onModelLoaded for the dead model.
dispose() now aborts the model's load through the same controller as the
signal and abort(modelId), so load() rejects with a LoadAbortedError.

dispose() returns the same promise when it is called again. That lets the
aborted load wait for the disposal that aborted it instead of starting a
second one.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Every model gets a uid from FragmentsModels that is never reused, and all
internal routing uses it instead of the modelId: messages to and from the
workers, thread assignment, progress callbacks, the tile queue, material
definitions and the worker's mesh cache. Late work of a disposed model can
no longer land on a newer model loaded under the same modelId. The public
`models.list` stays keyed by modelId.

dispose() now takes effect before it returns: the model leaves the models
list and the scene, its thread slot is freed and later requests to it
reject. The returned promise resolves once the worker has deleted the
model. The modelId can be loaded again right away.

- A failed or aborted load disposes its partial model without waiting
  for the worker, so load() rejects right away.
- Terminating a worker rejects the requests still waiting on it.
- A FINISH from a disposed model still settles update fences.
- editor.save() no longer reuses the old model's materials; they are
  disposed with it in finalizeDispose().
- The per-model fence of update(true) is keyed by uid too: a FINISH
  carries the uid now, so a fence still keyed by modelId never saw its
  model finish and update(true) never resolved. It is cleared when the
  model is removed.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The main thread answers a worker's request with the request itself.
RequestsManager empties a RECOMPUTE_MESHES request's tile list once it has
taken the tiles, but a disposed model's request was dropped with its list
in place, so every tile batch its worker still flushed was copied back,
buffers and all. It is emptied there too.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A call on a disposed model, e.g. through an Item or Element kept from
before, was rejected by the connection as "model 7 is not loaded": 7 is
the model's internal uid, which the caller has never seen. _invoke now
rejects it first as 'model "arq" is disposed'.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Delta models and queued element requests belonged to a modelId, so a model
loaded under a disposed model's modelId inherited them, including after
editor.save(). They now belong to the model's uid and go away with it.

- Disposing a model disposes its delta models.
- reset() clears deltaModelId along with the delta models.
- An edit or reset whose model is disposed meanwhile disposes the delta
  it built instead of leaving it loaded; concurrent edits no longer
  orphan a delta model.
- save() keeps the old delta models in the scene until the new model
  replaces them, and does nothing if the model is disposed meanwhile.
- relate(), unrelate() and deleteData() resolve their model once, and
  drop their work if it is disposed meanwhile. createElements() returns
  the elements of the model it edited.
- Delta model IDs end in a counter instead of performance.now(), so two
  can't collide.
- Queuing element requests for a model that isn't loaded throws.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
save() disposes the model, keeping its visuals until the reloaded model is
in the scene, and tore them down only after the reload. When the reload
rejected (an abort, the model disposed again, FragmentsModels disposed, a
worker error), the old THREE object, tiles, materials and delta models
stayed in the scene for good: dispose() had already run, so nothing could
clean them up. The teardown now runs either way, and the reloaded model
joins the scene as soon as it is loaded.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
save() replaces the model with a reloaded one under a new uid, and
disposing the old one dropped the element requests queued for it (with
createMaterial(), setItem(), createIndex(), ...) and not applied yet, so a
later applyChanges() lost them without a word. Before requests were keyed
by uid they carried over under the modelId. They now move to the reloaded
model, temp ids included, the way save() already passes undone requests on.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Each edit loads a delta model built from every request the worker had
when the edit reached it, and the edit shows it once it has loaded,
replacing the one shown. Loads run concurrently, so with two edits in
flight the first edit's delta model could finish last and replace the
second's, showing the model without the second edit until the next edit
that affects rendering. A reset raced the same way: a delta model loaded
after it was shown anyway.

Edits and resets are now numbered in the order they reach the worker, and
a delta model of an earlier one than the model shows is disposed instead.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
CONTRIBUTING.md gets a Design patterns section: a model's consumer-facing
modelId against the internal uid everything is keyed and routed by, why
async work needs the latter, and the rules for new code. Also updates the
mental model's stale threads.invoke(modelId, ...) line.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
manageRequest is now the connection's handler itself, so the connection
awaits it and answers a request it fails to handle with an error. The
requests it receives are typed as WorkerRequest, which the workers'
senders are checked against with `satisfies`, and Connection is generic
over the requests it receives.

The workers send these requests fire-and-forget, so they catch that error
answer instead of leaving an unhandled rejection; the main thread already
logs the failure.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
ShaMan123 and others added 3 commits October 11, 2026 07:10
getHighlight() resolved to MaterialDefinition[], but it has an entry for
every item it looks up, undefined for an item without a highlight, and a
highlight that preserves the item's original material (setColor(),
setOpacity(), resetColor(), ...) only holds the properties it changes, so
it can have no color. Both were cast away.

HighlightDefinition types such a highlight, and getHighlight() now
resolves to (HighlightDefinition | undefined)[], as does getSequenced()
for "highlight". highlight() accepts a HighlightDefinition, so a highlight
read with getHighlight() can be applied again, as the MaterialsManagement
example does. The worker's material list and the definitions it sends the
main thread are typed the same way, which drops their casts.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A RECOMPUTE_MESHES request carried its tile requests as any[], built in
four places of VirtualTilesController and read in six classes on the main
thread, none of which could tell what they held. TileRequest types them,
one member per TileRequestClass, and the worker side now builds, batches
and prepares them for transfer against it. Typing the main side's reads
comes next.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Everything a connection receives from the other side, requests and
answers alike, is a structured-clone copy, so ThreadHandler and fetch()
now type it as Cloned. On the main thread that reaches the classes that
take the tile and material requests, which read them as `any` until now:
RequestsManager, MeshManager, LODManager and MaterialManager are typed
against the requests, and the tile matrix and bounding box they got as
copies are rebuilt instead of passed on as THREE objects.

MaterialManager keeps the definitions the worker transfers as
HighlightDefinition[], with one documented narrowing: a tile's own
material is one of the model's, which are complete. The THROW_ERROR
branch and the templateId reads go, since no worker sends either, and a
tile's positions are typed as the Float32Array the worker allocates.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

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