Fix loadFile bugs, add CI, lint, tests on Vitest, split ImageTool - #9
Merged
Merged
Conversation
Two bugs in the image loader: - The "too large" message read width and height after decoded.close(), and a closed ImageBitmap reports 0, so the message said "0×0". Size is now read before closing. - If worker.load (or anything after the decode) threw, the decoded bitmap was never closed, leaving up to 100 megapixels in memory. The bitmap is now owned by loadFile until it is handed to state, and the catch closes it. Verified with a 10100x10100 PNG: message shows the real size. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Cloudflare only ran the build, so a failing npm run check could be merged. The workflow installs with npm ci, runs the Node checks, then the production build (which includes tsc). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
🚀 Deploying Preview to Cloudflare 🚀Preview URL: https://fix-load-file-and-ci-hdr-glow.beregovyyk.workers.dev (commit 2a6abe9)This URL reflects your latest Preview deploymentPreview Deployments by commit
|
Versions in package.json and package-lock.json no longer use ^ ranges, so installs are reproducible. index.html was reformatted, and its viewport meta is now "width=device-width" (no initial-scale=1). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
protocol.ts now has Requests (payload per request type) and Results (result per request type), and WorkerRequest is derived from Requests. ImageWorker.ask is overloaded on them, so a call site gets its exact result type, and the newest-wins requests (Skippable) are typed "| null". The six "as Promise<...>" casts are gone; one cast remains, at the postMessage boundary. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
preview, build and buildPq each recomputed the mask for the same colors, tolerance and softness, so one slider position cost three passes over up to four million pixels. The mask is now keyed by its parameters and dropped when the picture or its background changes. Output is unchanged (Lighthouse sample: 181 KB HDR, 127 KB PQ, 5% mask). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Six catch blocks showed the visitor a generic message and threw the error away, so a real failure left nothing to debug from. logError() now writes the cause to the console next to each message. The two catches left in copyText are intended fallbacks and are commented as such. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
oxlint (with the react, react-hooks and jsx-a11y rules) is used instead of ESLint because typescript-eslint does not support TypeScript 7 yet. Findings fixed or explained: - The status lines are <output> elements instead of role="status". - useBuiltFile no longer sets state in an effect to clear its result; it returns null when no colors are chosen. - The deliberate exhaustive-deps omission, the focusable code block, the drop zone, the preview-mode group and the swatch object URL now carry a targeted disable comment with the reason. Adds npm run lint, format and format:check, and runs lint and format:check in CI (the formatting itself lands in the next commit). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Formatting only, no behavior change: tsc, npm run check and the linter pass before and after. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
ImageTool.tsx was 647 lines with 12 useState and 7 useEffect calls. It is now a 280-line composition of: - useImageSession: the worker, the loaded bitmap, the background, the pixel version and the error, plus loadFile and loadSample. loadFile takes an onLoaded callback that runs in the same tick as its state updates, so a new picture still renders with its suggested colors. - useMaskPreview: the debounced mask canvas and its coverage. - useBuiltFile and ResultCard: moved out unchanged. - DropZone, GlowColors (chips, hex form, found-in-image chips) and GlowSliders: the markup, each owning only its own local state. - config.ts: the limits and delays; toHex moved into tool/colors.ts and gained a test. The data flow is the same as before: React state -> ImageWorker -> Pipeline. Verified in the browser end to end: samples, chip add and remove, hex add and error, the 12-color cap, sliders, background, preview toggle, empty state, and the oversized-image message. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
- vite.config.ts sets the dev server port to 5180 (5173 is Vite's default and often taken by another project). - package.json engines and .nvmrc state Node 22: npm run check runs .ts files directly, which needs 22.18 or newer. - displaySize, stripMetadata, REFERENCE_WHITE_NITS and GainMapJpegInput were exported but only used in their own file. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Two quick sample clicks could apply the slower, older picture last. Each load takes a number, and a load that is no longer the latest closes its bitmap and stops after the decode and after the worker call. Verified by clicking Arctic then Lighthouse, Arctic then KB Tech, and three in a row: the last click always ends up shown. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
tools/check/check.mjs had its own mini runner and could not import src/tool/swatch.ts (Node's loader needs file extensions that the Vite-style imports omit), so buildSwatch was copied into the test. Vitest runs the same tests through Vite's resolver: the test now imports buildSwatch directly and the runner code is gone. npm run check is "vitest run" (30 tests). Comments that explained the old limitation are updated, engines is set to Vite's own minimum (Node 22.12), and the README lists the scripts. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Sting29
changed the title
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
One branch, one commit per item from the code review.
Bugs
loadFile: the "too large" message read width and height afterclose()and said "0×0". Read before closing.loadFile: the decoded bitmap (up to 100 MP) was never closed if the worker call failed. It is now owned byloadFileuntil handed to state.loadFile: two quick loads could apply the older one last. Each load has a number; a stale one closes its bitmap and stops.Infrastructure
checkjob is required in branch protection.buildSwatchis imported directly; the copy in the test is gone.vite.config.ts,engines+.nvmrcfor Node 22, four exports that were only used in their own file are no longer exported.index.htmlreformatted (commit by the author, included as is).Code quality
as Promise<...>casts are gone.ImageTool.tsx(647 lines, 12useState, 7useEffect) is now a 280-line composition ofuseImageSession,useMaskPreview,useBuiltFile,DropZone,GlowColors,GlowSliders,ResultCard. Same data flow: React state -> ImageWorker -> Pipeline.Test plan
npm ci, then lint, format:check, 30 tests,tsc, production build all passcheckjob and Cloudflare Workers Build pass on this PR🤖 Generated with Claude Code