Review fixes: type-check gate, data-loss guards, ML worker hardening, input-handling bugs#108
Merged
Merged
Conversation
The build script ran plain `tsc` against a solution-style tsconfig
("files": [] + references), which type-checks nothing and exits 0, so
real type errors shipped unchecked. Switch to `tsc -b`, fix everything
it surfaces, and add a CI workflow so it stays green:
- App.tsx: "Replace pattern" passed a File to updatePatternImage(url,
w, h), corrupting the project (and persisting the corruption via
autosave); route both upload and drag-drop through a shared
loadPatternImageFile helper
- App.tsx: lamp profile dialog referenced a nonexistent updateProject —
ReferenceError on pattern-scale unit change; use updatePatternScale
(same path as the measure tool)
- App.tsx: drop 11 props passed to SheetPanel that it never declared
or read
- SheetPanel: `forceTool` was an undefined name — ReferenceError when
switching to an uncalibrated sheet with the measure tool active;
align with the handleToolChange path which checks !sheet.scale only
- SheetPanel: CANVAS.fg doesn't exist; use var(--text-soft) for the
pack-popover label
- Lamp3DPreview: convert hoisted `function resize()` to a const arrow
so the container null-narrowing applies
- Tutorial: type STEPS as Record<AnchoredStepId, StepConfig> instead of
a Record keyed by the full StepId union it never covered
- Remove unused locals/imports; add vite-env.d.ts so the ?worker import
in packing.ts resolves; gitignore *.tsbuildinfo
- Add GitHub Actions workflow running pnpm build (tsc -b + vite) on
pushes to main and PRs
Closes #85
https://claude.ai/code/session_018ug1kMLE8aSdT8k4prFPYV
- saveToOPFS no longer swallows write errors; failures now propagate to persist() so the existing "Couldn't save / Retry" status UI actually fires (it was unreachable before) - loadProjectFromOPFS returns null only for genuine NotFoundError and throws on anything else (corrupt JSON, transient OPFS errors), so startup no longer mistakes a broken-but-present project for a first visit and overwrites it with an empty one; on load failure the app starts under "<name> (recovered)" so autosave can never clobber the original file - Rename now writes the project under the new name and deletes the old file only after the write succeeds, instead of deleting immediately and leaving a 500 ms window with no copy on disk - Flush the pending debounced save on pagehide so edits made within the debounce window survive closing the tab - Request navigator.storage.persist() once on startup (OPFS is best-effort storage and silently evictable otherwise) - Importing a project now clears the undo/redo stacks; undoing after an import could previously restore the prior project's state and persist it over that project's file - Export uses a Blob object URL instead of a data: URI, which hit browser URL limits on projects with embedded images - switchProject/deleteProject/createNewProject handle the now-throwing save/load calls explicitly (stay on current project on a failed switch; surface save failures via saveStatus) Closes #86 https://claude.ai/code/session_018ug1kMLE8aSdT8k4prFPYV
- SheetPanel: Konva synthesizes a click after every pointerup with no movement threshold, so committing a marquee selection immediately ran handleStageClick's background-deselect and wiped the selection in the same gesture. Suppress the synthesized click right after a marquee commit. The handler itself stays: on touch devices (where pointerdown pans instead of marqueeing) it is the only tap-to-deselect path. - Both canvas panels' single-letter tool shortcuts now ignore Cmd/Ctrl/Alt chords: Cmd+C no longer switches to crop, Cmd+V to select, and Cmd+S no longer toggles refine-remove. The pen tool's Cmd+Z vertex-pop is hoisted above the guard and unchanged. - App's Delete/Backspace handler now ignores events from <select> elements, matching the panels' own guards; pressing Delete with the piece popover's sheet dropdown focused deleted the selected piece. Fixes #95, partially addresses #94 (panel scoping and the listener- ordering race still need the shared shortcut hook). https://claude.ai/code/session_018ug1kMLE8aSdT8k4prFPYV
… model cache retry - Serve onnxruntime-web's wasm binary as a same-origin Vite asset instead of fetching it from jsdelivr at runtime. The WebGPU EP runs on top of the wasm runtime, so every user needed the CDN to be reachable for segmentation to initialize at all, and the hardcoded version string in the CDN URL could silently drift from the installed package. (#88) - Remove @huggingface/transformers: zero imports anywhere in src, and it bundles its own copy of onnxruntime-web that would conflict with ours if ever actually imported. (#88) - SAM backend: reject all in-flight requests when the worker crashes (GPU device lost, wasm OOM) instead of leaving their promises — and the pieces marked pending — hanging forever, and reset the singleton so the next call spawns a fresh worker. Handle messageerror too. (#90) - Nesting worker: post ERROR instead of logging the exception and reporting COMPLETE as if packing succeeded; packing promise now rejects on worker error/load failure and always terminates the worker, so the Pack spinner can't spin forever. (#90) - Model cache: a failed OPFS cache write no longer triggers a full re-download of the ~305 MB encoder that is already in memory, and the network fallback now checks res.ok instead of handing an HTML error page to InferenceSession.create as model bytes. (#89) https://claude.ai/code/session_018ug1kMLE8aSdT8k4prFPYV
…ions - The welcome-tutorial decision now waits for the OPFS project load (gated on isLoaded) instead of inspecting the empty placeholder project at mount; returning users with saved work but cleared localStorage were dumped into the tutorial on top of their project (#105) - handleAddPiece/handleUpdatePrompt clip segmentation results against the pieces as they are when the worker resolves (via an always-current ref) rather than the render-time snapshot; two boxes drawn in quick succession were each clipped without the other's final polygon, producing overlapping pieces (#97) - debugMask ImageBitmaps are closed when replaced instead of waiting for GC; repeated refine clicks accumulated tens of MB of pinned bitmap memory (#97) - Sheet IDs use crypto.randomUUID() like piece IDs instead of Date.now(), which could collide within a millisecond and break piece->sheet references (#98) https://claude.ai/code/session_018ug1kMLE8aSdT8k4prFPYV
Deploying vitrai with
|
| Latest commit: |
9b6fbf4
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://18c57876.vitrai.pages.dev |
| Branch Preview URL: | https://claude-focused-rubin-pmcknj.vitrai.pages.dev |
For a tier whose radius decreases downward (Rt > Rb), the unrolled sector has L_top > L_bot, but three code paths assumed the opposite: - patternToSurface rejected every point in the tier because the d-range check used [L_top, L_bot] as [lo, hi]; pieces in contracting tiers vanished from the 3D preview entirely - The v parameter clamped its denominator positive with Math.max(1e-6, L_bot - L_top); for contracting tiers numerator and denominator are both legitimately negative, so v collapsed to 0 — and since reflowLampPoints round-trips every drawn polygon through patternToSurfaceRobust on profile edits, committing a profile change destructively flattened those polygons onto the tier's top arc - Same clamp in Lamp3DPreview's per-vertex mapping The inverse mapping (surfaceToPatternRobust) already handled both signs correctly, so only the forward direction needed fixing. Also guard the cylinder branches against 0/0 NaN from zero-size tiers, which passes straight through Math.min/max into the mesh position buffer. Verified numerically (esbuild-bundled module, node): contracting profile 800->400 now maps v=0.3 exactly where the inverse places it, round-trip reflow error < 1e-12 across a v/theta sweep, degenerate zero-height tier produces no NaN. The cross-tier horizontal mirroring of contracting tiers in the 3D preview (theta01 direction) is intentionally left untouched — it needs visual verification. See #92. https://claude.ai/code/session_018ug1kMLE8aSdT8k4prFPYV
findBestPlacement only applied gapPx against the bin walls; the inter-piece test was a raw intersection check, so pieces were packed flush against each other — leaving no clearance for the glass cutter, which is the entire purpose of the gap (defaultCuttingGapPx computes a 2 mm physical allowance). Pieces now conflict when they intersect, one contains the other, or any edge pair comes closer than gapPx (point-to-segment distance over edge pairs; both directions covers the min distance between non-crossing segments). The bounding-box prefilter is inflated by gapPx so the exact check sees near-misses, which also fixes exactly-touching edges slipping through segmentsIntersect's strict inequalities. Verified numerically: packing 30x30 squares into a 100x100 bin with gapPx=8 yields >= 8px separation between all pairs (10px after grid quantization); gapPx=0 still packs flush with no overlap. Addresses the headline bug of #91 (silently-skipped oversized pieces and the rotation reset are still open there). https://claude.ai/code/session_018ug1kMLE8aSdT8k4prFPYV
…urprise phases
First-run flow was: init downloads the decoder (~25 MB), progress hits
100% ("downloaded!"), bar hides — then the first encode kicks off the
~309 MB encoder download and the bar restarts near zero, which reads as
"it's downloading the same thing again" (observed during the tutorial).
- init() now registers the encoder files in the progress accounting up
front (first run only — skipped when they're already in the OPFS
cache), so the reported fraction covers the full first-run download
and never claims completion after just the decoder
- After the decoder is ready, the encoder download is warmed in the
background instead of waiting for the first encode; buffers are
released to the OPFS cache when no encode has claimed them, so blank
projects don't pin ~300 MB of worker memory
- Encoder fetches are deduplicated behind a shared promise so the
warm-up and a concurrent first encode can't download the 309 MB
twice; failed warm-ups remove their progress entries (no stalled bar)
and retry transparently on the first segment
https://claude.ai/code/session_018ug1kMLE8aSdT8k4prFPYV
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
2 participants
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.
Batch of fixes from the codebase review, prioritized for impact and safety to land without manual app testing.
What's in
#85 — build type-check was a no-op
"build": "tsc && vite build"ran plaintscagainst the solution-style tsconfig ("files": []), which checks nothing and exits 0. Nowtsc -b, with all 16 surfaced errors fixed — three of them were live bugs:FiletoupdatePatternImage(url, w, h), corrupting the project (and autosaving the corruption); both upload and drag-drop now share one loaderupdateProject→ReferenceErroron scale-unit change; now usesupdatePatternScaleSheetPanel'sforceToolwas an undefined name →ReferenceErrorwhen switching to an uncalibrated sheet with the measure tool activePlus: 11 props App passed to
SheetPanelthat it never declared,CANVAS.fg, the hoisted-function null-narrowing issue inLamp3DPreview, theSTEPSrecord type, unused locals,vite-env.d.ts, and a CI workflow running the build on PRs/pushes to main.#86 — silent data-loss paths in OPFS persistence
<name> (recovered)so the original file is preservedpagehide;navigator.storage.persist()requested oncedata:URI that hit URL length limits on image-heavy projects#88 / #89 / #90 — ML worker hardening
@huggingface/transformersdependencyERRORinstead of swallowing exceptions and reportingCOMPLETE, and the packing promise rejects + terminates the worker on load failure (Pack spinner can't stick)res.okinstead of feeding an HTML error page toInferenceSession.create#91 — Smart Pack now enforces the cutting gap between pieces
Pieces conflict when they intersect, contain each other, or any edge pair comes closer than
gapPx(point-to-segment distance over edge pairs). Numerically verified: 30×30 squares atgapPx=8land ≥ 8 px apart;gapPx=0still packs flush. (Oversized-piece skipping and the rotation reset remain open in #91.)#92 — smooth lamp mode no longer breaks on contracting tiers
For tiers whose radius decreases (e.g. the tulip profile), the unrolled sector has
L_top > L_bot, but the forward mapping assumed the opposite: the d-range check rejected the whole tier (pieces vanished from 3D) and thevdenominator was clamped positive (profile edits destructively flattened drawn polygons onto an arc viareflowLampPoints). The inverse mapping was already sign-correct, so only the forward direction changed. Verified numerically: round-trip reflow error < 1e-12 across a v/theta sweep on an 800→400 contracting profile; zero-size tiers no longer emit NaN into the mesh. (The cross-tier theta mirroring in the 3D preview is left for visual verification.)#94 (partial) / #95 — input handling
<select>no longer deletes the selected piece#97 / #98 / #105 — smaller fixes
close()dcrypto.randomUUID()instead ofDate.now()Verification
tsc -bclean andpnpm buildgreen after every commit. The packing-gap and lamp-geometry math were verified numerically against the bundled modules (details in the commit messages). Not manually tested in-app — flagging the riskiest spots for a quick once-over: the rename flow, marquee select on the sheet panel, a cold-cache segmentation run (wasm now served same-origin), and lamp smooth mode with the tulip profile.Closes #85, closes #86, closes #88, closes #90, closes #95, closes #105
https://claude.ai/code/session_018ug1kMLE8aSdT8k4prFPYV