fix: pin nail count for the lifetime of a generation run - #6
Merged
Conversation
The greedy winding loop in main.ts read the live `settings.nails` value every frame for both `chooseNextNail`'s edge-key hashing and the `used.add(edgeKey(...))` call, while the actual nail-position array (`nails`) is built once at the start of the run from whatever `settings.nails` was at that moment. Nothing disables the "Nails" slider while a generation is running, so dragging it mid-run (a normal UI interaction, not an edge case) changes `settings.nails` out from under the in-flight run. Because `edgeKey(a, b, count) = a < b ? a*count+b : b*count+a`, a count change mid-run desyncs the hash space the `used` Set was built with from the one later lookups use — collisions can silently block a legitimate new chord, or let an already-drawn chord slip past the "never redraw the same edge" check the code explicitly documents as the anti-oscillation guarantee (see `ChooseOpts.used` / `uniqueEdges` in stringart.ts). Verified live in a browser: dragging Nails 240 → 360 → 120 mid-run no longer has any effect on the in-flight run's edge-key space. Fix: snapshot `settings.nails` into a local `nailCount` once at the top of `startGeneration`, and use that everywhere the loop currently reads `settings.nails` for edge encoding. The slider stays live for the *next* run, which is the only behavior the UI actually promises. Pure-logic tests in tests/core.test.ts (stringart.ts, image.ts, svg.ts) are unaffected — main.ts is the UI wiring layer and isn't covered by vitest, so this was verified manually: npm run typecheck, npm test (27/27 still pass), npm run smoke, and a live browser session (non-square photo upload, garbage-file upload, full generation runs, mid-run slider drag) with a clean console throughout. `docs/` rebuilt via `npm run smoke` per repo convention (content-hashed bundle name changed accordingly). Also audited: no network calls anywhere in src/ or the built bundle (confirmed the one `fetch()` in the bundle is Vite's standard modulepreload polyfill for local same-origin chunks) — the "100% client-side, nothing leaves your device" privacy claim in README.md holds. Malicious/garbage file uploads are already handled gracefully (toast + no crash) via the existing createImageBitmap/Image fallback try/catch. No XSS surface (no innerHTML/eval in the codebase). No autosave is claimed anywhere, so refresh data loss is expected, undocumented-otherwise behavior, not a bug. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
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.
Context
First-pass TRL audit of
string-portrait(no priortrl_assessed_atin the registry). Confirmed the concept by reading source, not assuming: this is a genuine Petros-Vrellis-style computational string-art generator — a greedy solver that repeatedly picks the chord (nail-to-nail line) covering the most remaining "darkness" in a residual buffer, draws it, and subtracts its contribution, producing one continuous thread.src/stringart.ts/src/image.ts/src/svg.tsare pure logic with 27 passing vitest cases;src/main.tsis the DOM/canvas wiring layer (untested by vitest, verified live in a browser for this PR).What was checked (per the audit's priority order)
src/*.ts. Confirmed it's a real greedy string-art solver, not a stub:nailPositions→linePixels(Bresenham) →lineScore/subtractLine(residual-darkness bookkeeping) →chooseNextNail(greedy pick,-1sentinel when nothing left worth drawing) →generateStringArt. The residual only ever decreases and is clamped at 0, so the run is provably monotonic — no divergence/oscillation risk by construction.grep -rniE "fetch\(|XMLHttpRequest|WebSocket|analytics|gtag|sentry"acrosssrc/andindex.html: nothing. The onefetch(n.href,i)in the built bundle (docs/assets/*.js) is Vite's standardmodulepreloadpolyfill fetching the app's own local same-origin chunk — not telemetry. Watched the Network tab live through a full generation run (sample image + two uploaded test images): zero requests beyond the local dev server. The README's "100% client-side, nothing leaves your device" claim holds.localStorage/indexedDB/sessionStorage/autosave anywhere in the codebase or docs, and nothing in the UI or README claims persistence. Refresh-loses-work is undocumented-as-otherwise, expected behavior, not a bug.innerHTML/eval/document.writeanywhere (grep confirmed). Fed the upload input a 2 KB file of random bytes with a spoofedimage/pngMIME type via a syntheticFile/DataTransfer(bypassing theaccept="image/*"picker hint, which is client-side-only anyway): handled cleanly —createImageBitmaprejects, theImage()fallback'sonerrorfires, user gets a "Could not read that image." toast, no exception reaches the console, no crash. SVG uploads can't inject script (loaded via<img>/createImageBitmap, not<object>/direct navigation, so embedded<script>in an SVG is inert per spec).imageToGraycentered and scaled it correctly (verified visually — no stretch, no misalignment). Internal solver resolution is a fixed 500×500 regardless of source photo size, so an oversized upload can't blow up processing cost.The bug (item 3 — greedy algorithm correctness)
startGeneration()insrc/main.tsbuilds the nail-position array once, fromsettings.nailsat that instant:...but the frame loop (
step()) re-reads the livesettings.nailsevery frame for the edge-key hash, both for the lookup and for recording:edgeKey(a, b, count) = a < b ? a*count+b : b*count+a— the hash space depends oncount. Nothing disables the "Nails" range slider while a generation is running, so a completely ordinary UI action (drag the slider mid-run) changessettings.nailsout from under the in-flight run. That desyncs theusedSet's hash space from the one later lookups use: hash collisions can either falsely block a legitimate new chord, or let an already-drawn chord slip back past the "never redraw the same edge" check — whichstringart.tsexplicitly documents as the anti-oscillation guarantee (ChooseOpts.used,GenerateOpts.uniqueEdges).Fix: snapshot
settings.nailsinto a localconst nailCountonce at the top ofstartGeneration, and use that constant everywhere the loop needs the count for edge encoding, instead of re-reading the live, mutable setting. The slider stays live for shaping the next run — the only behavior the UI actually promises.Verified live: clicked Generate, then scripted the exact repro (drag Nails 240 → 360 → 120 mid-run) via the DOM. Before the fix this silently corrupted edge-key hashing for the rest of the run; after the fix the in-flight run is unaffected (as intended — the slider only takes effect on the next Generate), confirmed with a clean console throughout.
Verification
npm run typecheck— cleannpm test— 27/27 pass (unaffected; main.ts isn't vitest-covered, this fix was verified manually)npm run smoke(vitest +vite build+ output sanity checks) — passes;docs/rebuilt, bundle hash changed accordingly (index-DfywjaKV.js→index-B21gUkMX.js)Scope note
npm run fmt:checkcurrently fails againstCLAUDE.mdandpackage.json(pre-existing, unrelated to this change, present before this PR). Left untouched — reformattingCLAUDE.mdwholesale is out of scope for this fix and would fork the fleet-propagated file against its own header instructions ("if you find a stale copy that differs from this one... don't fork it"). Flagging here rather than silently bundling an unrelated 150+ line reformat into a one-file bugfix PR.Left PR #5 (
postcssDependabot bump) and #4 (esbuild/viteDependabot bump) untouched, as instructed.Suggested registry entry (for a follow-up, not applied here — did not touch services-registry)
🤖 Generated with Claude Code