feat(inference): add browser point suggestions to the editor - #875
Merged
Merged
Conversation
… target A stored suggest-target preference naming a browser target that listTargets() no longer reports is the ordinary shape of every reload for someone who picked "This device" last time, since acquired model bytes are never persisted across page loads. It must revert to Server without a visible error. An explicit in-session choice that later drops out is different and must keep surfacing through blocker/refusal, never auto-switch back.
…hing the rejection
…ith discriminating fixtures
… state Also adds the missing @visionset/browser-inference workspace dependency to frontend/app/package.json (and the resulting pnpm-lock.yaml update), without which BrowserInferenceRuntime.ts's import could never resolve.
annotate.spec.ts's stubbed API route table was module-private, so a second Playwright spec had no way to reuse it without duplicating ~440 lines. Moved verbatim into frontend/app/e2e/_wireApiStub.ts, exporting the pieces other specs and annotate.spec.ts's own remaining tests need (serveApi, openJob, PROJECT/BATCH/JOB, progressStore, openedWorld); annotate.spec.ts now imports them instead of defining its own copy.
…ver tab its own tone/icon
- Test 7 asserted aria-selected on suggest-target-browser while the refused status has TargetChooser unmounted; press Escape first to drop back to idle, which remounts the chooser without touching activeTarget. - Split acquireAndSelectBrowserTarget into an arming step and a non-arming acquire/select step, since toggleSuggest() clears the whole session on a second press with the tool already armed (test 3 was double-arming it). - Registered the model-request listener before navigating in the "never fetched before Download" test, and added a mid-test assertion that merely switching to the device tab is also zero requests. - SHA-256 mismatch test now uses a correctly-sized but wrong-content buffer, so the size check passes and the SHA-256 check is what actually fails. - Added a catch-all abort route for the model CDN host ahead of the specific fixture routes, so anything unmatched hard-fails instead of reaching the real network. - Removed the routing-paragraph duplicated between annotate.spec.ts and _wireApiStub.ts, keeping it only where the route table now lives.
…gression test discriminate The executor guard re-derived "is this target in browserTargets" a second time, independently of computeSuggestBlocker's own answer to the same question - the same shape of bug this file just fixed once already. Derive it from blocker instead, so the two can no longer silently drift apart. The regression test's "no fallthrough to the server" assertion also could not actually fail: it checked synchronously right after the click, before an async mutateAsync dispatch would have reached the sent log, and nothing in the test setup guaranteed a genuinely usable server connection existed to fall through to in the first place. Wait for the server tab to resolve ready, and flush a tick before asserting.
…evision Mutation testing found this gap: hardcoding MODEL_REF to drop EFFICIENT_SAM_TI_REVISION left all existing tests green, silently corrupting the provenance carried into accepted annotations. The new test imports EFFICIENT_SAM_TI_REVISION independently from manifest.js and checks the exact expected string, so it fails if either the constant or the implementation drifts.
… shape A real-CDN smoke test found the live manifest nests artifacts under "artifacts" with bare relative filenames, not the flat shape this suite's mockCdn assumed (manifest.ts/acquireEfficientSam.ts already fixed upstream). Updates the manifest stub and the encoder/decoder route patterns, including the SHA-mismatch test's own re-route, to match.
…rd artifact paths
- Extract EFFICIENT_SAM_TI_BASE_URL so the manifest and artifact URLs share one
source for the /models/efficient-sam-ti/<revision>/ prefix instead of restating it.
- Strip a trailing slash from MODEL_CDN_BASE_URL to avoid a double slash.
- Validate the parsed manifest's artifacts.{encoder,decoder}.path before use, so a
future CDN schema drift throws a diagnosable error instead of a bare TypeError.
- Reject a manifest artifact path containing a slash before building its URL.
- Add tests proving verification never reads the manifest's own bytes/sha256, the
path-traversal guard fails closed, and the manifest-shape validation works.
…URL rewriting
Vite's `import.meta.url`-relative asset transform rewrites the literal argument of
`new URL("./ort/", import.meta.url)` and drops the trailing slash, so `wasmPaths`
arrived as `.../dist/browser/ort`. ORT then resolved its glue module as a *sibling*
of `ort` rather than a file inside it; a dev server answers that 404 with its SPA
fallback, so the dynamic import received HTML, both execution providers failed to
initialize and every session load reported `graph-load-failed`.
The trailing slash is now re-applied to the resolved string instead of being
trusted to the literal, which holds for any bundler and any host.
The suite resolves `@visionset/browser-inference` through its `dist/`, exactly as it does the three packages already built here, so an unbuilt change there was invisible in the browser and the run silently depended on that `dist/` happening to be current.
_wireApiStub.ts's /content route always serves a real-but-1x1 PNG, which is fine for annotate.spec.ts's ~150 tests (AnnotatorCanvas lays the picture out at the asset's declared width/height, never its own naturalWidth) but breaks this suite: the browser executor validates click coordinates against the image's actual decoded dimensions, so every click landed "outside" a 1x1 image and was correctly refused. Adds a small dependency-free PNG encoder and overrides just this file's /content route with a real 640x480 image, matching the stub's asset metadata and the coordinate frame the existing click math already assumes. Scoped to this file rather than changing _wireApiStub.ts's shared default. First real green run against the model fixture: 7/7 passed.
…function it describes It had drifted above `withTrailingSlash`, the helper it only mentions in its last paragraph, and away from `assetsAt`, which is what actually decides where the ORT artifacts are fetched from.
A production `vite build` emitted no `ort/` directory at all. The worker's default resolution — `./ort/` against its own module URL — is right for the package as installed, but rolldown hashes the worker into `assets/`, so the built bundle asked for `/app/assets/ort/ort-wasm-simd-threaded.asyncify.mjs`, which no build writes. The SPA fallback answers that with HTML, the dynamic import of the glue fails, and every execution provider fails to initialize — a feature that works under the dev server and 404s in every real deployment. A vite plugin now copies `@visionset/browser-inference`'s own `dist/browser/ort/` into the build output at `ort/`, and serves the same URL from a dev middleware, so both modes answer the one address the app states through `assetBaseUrl`: `<base>ort/`, absolute against the document, since `BASE_URL` is the only thing that knows the wheel mounts the bundle at `/app/`. The bytes are read from the dependency's build output at build time, so an ONNX Runtime bump needs nothing here.
The production fix shipped with nothing that would notice its removal: delete the `ortAssets()` plugin and tsc, lint, 96 app tests, 129 browser-inference tests and the dev e2e suite all stay green while `/app/` serves an app whose first "this device" suggestion fails inside a worker. The wheel suite already builds the frontend, so the guard is one assertion against output that exists. Counting the wheel's size had to change with it. `_static/ort/` is ~6.4 MB deflated, against a 2 MB ceiling whose stated job is catching the day a `node_modules/` or a fixture video gets swept in. Raising that ceiling past 8 MB would have retired the guard to admit one file, so the ORT runtime is excluded from it and bounded separately instead: the rest of the wheel is still held to 2 MB, and a version bump that doubles ONNX Runtime now reads as a sentence about ONNX Runtime. Also in the plugin, two ways it could fail unhelpfully: an unhandled `error` on the dev middleware's read stream took down the whole dev server, which `tsup`'s `clean: true` makes reachable by rebuilding browser-inference while the server is up; and a subdirectory under `dist/browser/ort/` would have surfaced as an opaque EISDIR from `readFile` rather than as the missing-assets message written for it.
The comment still described a ~570 KB wheel with a ~640 KB bundle and "room for the UI to roughly triple", which was stale twice over: the constant now counts everything but the ORT runtime, and the app bundle has since grown to ~1.19 MB uncompressed on its own. The counted part is ~1.45 MB compressed, so the real headroom is about 1.45x — worth saying plainly, because a reader who trusts "roughly triple" will misjudge how close the next UI addition puts them to a ceiling they would then raise without noticing they were the reason. No threshold or behaviour change.
…ming ready Three things the composition root was getting wrong about its own lifecycle. **One encode per asset.** `createBrowserSuggestionExecutor`'s one-embedding slot is the only encode cache there is, and it lives in that closure. `executorFor` built a fresh executor per call, `AnnotationPage` calls it during render, and it re-renders on every click — so every refinement click re-ran the full encoder pass, silently defeating the design's "one encode per asset, N decodes per N refinements". The executor is now memoized beside the runtime it wraps, which keeps the invariant true whatever ui-core's render behaviour does. **Ready means the session exists.** `createRuntime` returns before the worker has loaded a graph; `ready()` is what waits for that. Claiming ready on the constructor alone listed a target whose every click then refused — and a listed target hides the Download control, so the retry path went with it. `ready()` is now awaited, a rejection leaves the state unacquired and retryable, and the failed runtime is disposed rather than leaked. **No download that can only fail.** `listAcquisitions()` consults the package's own capability check first: a browser with no Worker or no WebAssembly is offered nothing, which is the port's own rule one level down.
…not be reached" Every refusal this executor raised besides the negative-point one was a bare `Error`, and `refusalProse` routes a non-`ApiError` through `asApiError`, which stamps it `NETWORK_ERROR` — whose prose blames a server that was never asked. Three codes, each with prose that says what actually happened on this device: `BROWSER_ASSET_CHANGED` for the missing/mismatched source and both staleness checks, `BROWSER_INFERENCE_UNAVAILABLE` for a runtime that could not start, and `BROWSER_INFERENCE_FAILED` for a run that did not answer. The runtime's own errors are mapped through an exhaustive table over `InferenceRuntimeErrorCode`, so a code added to that package stops compiling here until somebody decides what a person should be told about it. `refusals.test.ts` holds the "never says the server" assertion for all three, which is the regression that would otherwise reopen silently.
`browser-models` named only `browserSuggestion.spec.ts`, not the code that suite guards. A change to the composition root, the vite or playwright config, or the ui-core inference seam and panels woke only the `frontend` job — where that suite self-skips, having neither the ONNX artifacts nor `VISIONSET_REQUIRE_BROWSER_MODELS`. So the PRs most in need of real-model coverage were the ones least likely to get it. `ci_path_filters.test.mjs` now asserts each of those paths wakes both groups.
… a dead click `computeSuggestBlocker` was exported from the package root and consumed by nobody outside it — `AnnotationPage` imports it relatively. An internal helper on the public surface is a contract nobody asked for. And with "This device" selected before any download, the idle card still read "Click the thing you want" over a click that is a silent no-op, since there is no executor for an unready browser target. That one state now points at the Download button instead. Every other state's copy is untouched.
Four passages in `architecture/frontend/ui-core.md` had gone false. No host supplied a runtime (`frontend/app` now does, unconditionally); the port was two members wide (it is four — two required, two optional and additive); no capability let a host ask for local inference (one does); and no consumer of the pixel lease had needed to reconcile browser-canvas-vs-Pillow decode divergence. That last one is the important one, and it is now stated as a live limitation rather than a future concern: the browser suggestion executor feeds `readRgb` straight to a model, so Server and "This device" can legitimately return different masks for the same asset and the same click — and for an asset carrying EXIF orientation or an AdobeRGB profile the difference is not marginal. `ui.md`'s suggest section described only the server path. It now covers the two tabs, the explicit ~41 MB download, that the download does not survive a reload, and that Alt-click needs Server.
frontend/app now imports @visionset/browser-inference and its /browser subpath, both resolved through its generated dist/. The cycle suite's webServer build sequence built every other workspace dependency first but never this one, so app's build failed with TS2307 on a clean clone (GitHub Actions run 1261) even though it passed locally wherever an earlier, unrelated command had already built browser-inference.
…he decode's `onImageReady` built the `BrowserSuggestionAssetSource` from the decoded `<img>`'s `naturalWidth`/`naturalHeight`. Everything that source meets is in the *descriptor's* pixels instead: the click points `suggestAt` sends, the geometry `shapesFromMask` returns, and the annotations already on the frame. `AnnotatorCanvas` has always laid the picture out that way — this is the same rule one layer on. A decode that disagrees — EXIF orientation transposing the axes, a preview served in place of the original — therefore had the executor bound-check clicks and extract pixels against a second, private frame, and answer with suggestions that are individually plausible and uniformly wrong. Nothing crashes and nothing is logged, which is what makes it worth a test. The regression test makes the two frames disagree by a transposition, because a fixture where they coincide cannot tell them apart — which is exactly how this survived the suite that shipped it. It claims the extent the executor bound-checks against *and* what `readRgb` asks the decoder for; mutating either one alone fails it. The e2e header's account of why it serves a real, correctly-sized asset image was written against the old behaviour and is corrected here: the bounds now agree either way, and what a 1x1 `/content` still gets wrong is the pixels.
JArmandoAnaya
force-pushed
the
feat/browser-suggestion-integration
branch
from
September 17, 2026 18:06
b1b8f0f to
c49ea85
Compare
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
1 participant
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.
Summary
@visionset/browser-inference) now runs end to end in the OSS editor, alongside the unchanged server suggestion path. A person chooses "Server" or "This device" per project.frontend/appis the only package that knows aboutmodels.robomous.ai, ONNX Runtime, or SHA-256 verification;ui-coreand@visionset/annotatorstay vendor- and runtime-free, machine-enforced by two boundary-test files (ui_core_boundary.test.mjs,cdn_vendor_boundary.test.mjs).model_refprovenance is immutable and survives acceptance unchanged, regardless of execution provider.@robomous/ui-coredesign system (Tabs,Badge) — no ad hoc controls, and no component the design system doesn't actually ship (it has no radio-group/checkbox, so Server/This-device is a real segmentedTabs, not radio buttons).Real-world verification (not just mocked tests)
This branch required an unusually long real-world validation pass, because several defects only manifested outside of mocks:
models.robomous.airelease: the actual deployed manifest schema didn't match this code's first assumption (nestedartifacts.{encoder,decoder}.path, not a flat shape) — found via a live smoke test, fixed, and re-verified against the real CDN twice more through follow-up hardening (manifest shape validation, path-traversal guard, a decisive test proving verification never trusts the manifest's own hashes).frontend/app/e2e/browserSuggestion.spec.ts) was run for the first time against a real, exported EfficientSAM-Ti model. This surfaced a pre-existing bug in@visionset/browser-inference's worker (a bundler asset-path issue only visible when served through Vite's dev server, as the real app does) and a test-fixture bug (a 1×1 placeholder image, invisible until the model started validating real pixel bounds). Both fixed; suite is now genuinely 7/7 against a real model, independently reproduced twice.vite buildoffrontend/appdidn't ship the ONNX WASM assets at all — the feature would have 404'd in any real deployment despite passing every dev-mode test. Fixed with a small Vite plugin that derives the assets from@visionset/browser-inference's own build output at build time; proven by actually building the app and serving the output over HTTP.tests/packaging/test_wheel.py, proven by an actual delete/rebuild/fail, restore/rebuild/pass cycle.Known trade-offs for human review
pip install visionsetwheel, for a feature most users won't invoke. The model weights themselves are already fetched on demand from the CDN, so bundling the runtime is a choice, not a network-independence requirement — serving it from the same CDN is a viable alternative if wheel size becomes a concern. Not blocking this PR.docs/content/architecture/frontend/ui-core.md.MAX_WHEEL_BYTES's headroom is down to ~1.45× after this branch (from the app bundle's own growth, not this feature) and will likely need raising within a release or two.Test plan
pnpm test:scripts, all four frontend package suites (ui-core1424,annotator1960,browser-inference129,app100), and all typechecks green.bash scripts/check.shandbash scripts/check.sh docsgreen.frontend/browser-inference's existing real-model Playwright suite: 17/17, real ONNX Runtime, mask outputs SHA-256-identical to the Python reference.browserSuggestion.spec.tshermetic suite: 7/7 against a real, exported EfficientSAM-Ti model (independently reproduced twice).models.robomous.airelease (manifest, encoder, decoder — byte counts and SHA-256 hashes all confirmed).vite build+ real wheel build verified to actually ship and correctly serve the ONNX assets.