Repository navigation
Conversation
This was referenced Oct 11, 2026
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>
…rmsIdsOfItems() The name was missing an "s". The misspelled one stays on what the package exports (FragmentsModel, SingleThreadedFragmentsModel, IFragmentsModel and EditUtils) as a deprecated method that calls the renamed one. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
ShaMan123
force-pushed
the
refactor/typed-rpc
branch
from
October 11, 2026 20:31
7def125 to
1237f03
Compare
ShaMan123
marked this pull request as ready for review
October 11, 2026 20:39
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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 amodelIdcan 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, addsload({ signal })(#173), and types the messages between the threads, where most of the bugs it fixes were hidden behindany.The stack, bottom to top, in merge order:
refactor(): route model calls through _invoke: mechanical groundwork, no behavior change (merged)fix(): type calls to a model's worker, rebuild what the worker copiesfeat(): load abort signal + harden the message layerfix(): key models by an internal uid, dispose synchronously: the core fixfix(edit): key editor state by the model's uidrefactor(): type the requests workers send to the main threadEvery 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: all of it. #325, the PR below, is merged and this branch is rebased on
main, so nothing in the diff belongs to another PR.Description
PR 2 of 6.
_invoke()took any string and any arguments and resolved toany, so every call site cast the result to what it hoped the worker returned. This PR types the call end to end, fixes the bugs the types exposed, and tests the result through a worker.Typing
_invoke()only takes names of the worker model's methods, with arguments that match their parameters.RemoteMethods: the methods the main thread calls.dispose(), which deleted the worker-side model behind the main thread's back, andgetItemsByConfig(), which takes a function that can't be copied to a worker, are no longer callable through it.Cloned<T>, the structured-clone copy that actually arrives. Methods are dropped from it, so aTHREE.Matrix4is{ elements, isMatrix4 }and no longer passes for the real thing.ItemData,SpatialTreeItem, the raw data maps), a return annotation keeps the name and is checked against the worker's result. The casts that stay are the index methods' andgetMetadata()'s, whose type parameters are the caller's own claim.Bugs the types exposed (all reproducible on
main)getItemsGeometry()andgetGeometries()returned atransformtyped as aTHREE.Matrix4that was a plain object, so calling aMatrix4method on it threw.getHighlight()andgetItemsMaterialDefinition()did the same withcolorandTHREE.Color.resetColors()was meant to rebuild it but testedisColor, which the copy keeps, so it never did.getSequenced()handed outmergedBoxes(THREE.Box3), geometry transforms and highlight colors as copies too. Each kind of result is now rebuilt through a map keyed by kind, so a new kind can't be left out.getBuffer()was cast toArrayBufferbut resolves to aUint8Arraywhen compressed, which is the default:new DataView(buffer)type-checked and threw."IFCLABEL", so a numeric comparison never matched.getMetadata()answerednullfor a model saved without metadata, against its declared type.Bug the worker tests exposed (also reproducible on
main)getSequenced("mergedBoxes", …)always resolved to an empty box. The worker passed the selected items' local ids togetBBoxes(), which takes item ids. The types could not catch it, both being numbers.Public API changes
AttributeData.typeandItemAttributes.setType()take a string.getBuffer()resolves toArrayBuffer | Uint8Array.getSequenced()can resolve tonull, for a result kind the worker doesn't know, as its docs say.getMetadata()resolves to{}instead ofnullfor a model without metadata.getCRS()still resolves tonullwithout a CRS.VirtualFragmentsModelsignatures declare what the methods already handle: the highlight and visibility methods accept no ids, meaning every item, andgetItemsData()accepts GUIDs.MaterialManager.resetColors()is replaced byrestoreColor().getGlobalTranformsIdsOfItems()is renamedgetGlobalTransformsIdsOfItems(). The misspelled name stays onFragmentsModel,SingleThreadedFragmentsModel,IFragmentsModelandEditUtils, marked@deprecated, and calls the renamed one.Tests
The fixes are tested end to end in
fragments-model.worker.test.ts:small_test.fragis loaded and queried through a worker, using theWorkerof@vitest/web-workerthatvitest.setup.tsalready installs. Every message crosses a realMessagePort, so results arrive as structured-clone copies, and the first test asserts exactly that. These replace this PR's earlier tests, which stubbed_invoke()withstructuredClone()and so never asked the worker anything.SingleThreadedFragmentsModelon the same file, and must be strictly equal, classes included.Matrix4fromgetItemsGeometry(),getGeometries()and an item's geometry,ColorfromgetHighlight()andgetItemsMaterialDefinition(),Vector3andBox3from positions and boxes.getSequenced()are compared with the method that returns the same thing.Matrix4and moved.Two limits. The stub runs the worker in the test's own thread, so this tests what copying does, not isolation between threads. The index methods (the fixture has no indexes) and
getItemDrawChunks()(it depends on which tiles are loaded) are not covered.Additional context
Worth a close look:
Cloned<T>inmultithreading/cloned.ts.RemoteMethodslist is complete.restoreColor()keeps the color components as they are. The worker already converted them from sRGB, and the old fallback would have converted them a second time.vitest.worker.ts, the worker the tests give toFragmentsModels. Under the stub a worker runs wherewindowexists, sofragments-thread.tstakes itself for the main thread and doesn't listen; this file listens for it, in one line, andfragments-thread.tsis untouched. The alternative is to havefragments-thread.tsrecognize the stub.14 commits. The first 12 are one change each, and if that is too much for one review they split in two after
refactor(): limit _invoke to the worker methods the main thread calls. The last two were added later:fixesmoves the tests onto a worker and fixesmergedBoxes, and the last one is the rename.Tests: 268 pass, 58 of them new (
fragments-model.worker.test.ts,cloned.test.ts,get-metadata.test.ts).What is the purpose of this pull request?
Before submitting the PR, please make sure you do the following:
feat(examples): add hello-world example).fixes #123).🤖 Generated with Claude Code