Repository navigation
Conversation
📝 WalkthroughWalkthroughAdds a complete shifting-mosaic solver with C++ search engines, TypeScript UI integration, WASM variants, cross-origin isolation support, configuration I/O, solution visualization, benchmarks, fixtures, and extensive tests. It also refactors rolling-blocks internals and updates phasic-dial, CI, and repository tooling. ChangesShifting Mosaic Solver
Rolling Blocks Solver
Repository maintenance
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Editor
participant wasmBridge
participant Worker
participant WASM
participant SolutionView
Editor->>wasmBridge: searchShiftingMosaicWasm(puzzle, callbacks)
wasmBridge->>Worker: start selected WASM variant
Worker->>WASM: solve(puzzle, config)
WASM-->>Worker: progress/phase/done/error
Worker-->>wasmBridge: normalized solver messages
wasmBridge-->>Editor: solution or status callbacks
Editor->>SolutionView: render solution steps
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
🧹 Nitpick comments (33)
src/pages/shifting-mosaic-solver/aStar.ts (4)
121-157: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMove the late field declarations above the constructor.
movableBlockIndices,unsolvableAtStart, andgoalAnchorBfsDistare declared at Lines 153-157 but used at Lines 121-146. Initializers still run before the constructor body, so behavior is fine, but declaring them after the constructor makes the initialization order hard to follow.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/pages/shifting-mosaic-solver/aStar.ts` around lines 121 - 157, Move the field declarations for movableBlockIndices, unsolvableAtStart, and goalAnchorBfsDist above the constructor that initializes them, keeping their existing types and initializers unchanged. Leave the constructor’s initialization logic and behavior intact.
199-306: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider extracting the expansion loop to cut cognitive complexity.
SonarCloud fails this function at complexity 39/15. Pulling the successor-generation block (Lines 267-301) and the pop/stale-entry filtering into private helpers (e.g.
expandNode,popFreshEntry) would bring it under the threshold without behavior change.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/pages/shifting-mosaic-solver/aStar.ts` around lines 199 - 306, Reduce the cognitive complexity of search by extracting successor generation into a private expandNode helper and the heap-pop, stale-entry, and missing-node filtering into a private popFreshEntry helper. Update search to delegate these responsibilities while preserving existing goal checks, pruning, score updates, queue behavior, timeout handling, and path reconstruction.Source: Linters/SAST tools
200-205: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
[]conflates "already solved", "unsolvable", and "timed out".
search()returns an empty array for a root that is already at goal (Line 212), for an unsolvable board, for a timeout, and for exhausted search. Callers cannot distinguish these. Returning a discriminated result ({ status, turns }) would make the reference implementation usable for the same reporting the UI does.Also applies to: 236-242
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/pages/shifting-mosaic-solver/aStar.ts` around lines 200 - 205, Update AStar search() and its unsolvableAtStart and timeout/exhaustion return paths to return a discriminated result such as { status, turns } instead of a bare array, with distinct statuses for solved, unsolvable, timed out, and exhausted search. Preserve the computed turn sequence in turns and update all callers to consume the new result shape.
483-512: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valuePer-expansion deadlock check is the dominant cost.
isDeadlockedrebuilds the full occupancy grid and runs the movable-set fixpoint for every expanded node, i.e. O(nodes × blocks² × cells). Since the C++ solver is the production path, this is acceptable for a reference implementation, but consider running it only every N expansions or reusing a preallocated occupancy buffer if the TS path is ever used on larger boards.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/pages/shifting-mosaic-solver/aStar.ts` around lines 483 - 512, Reduce the per-expansion cost of isDeadlocked by avoiding a full occupancy-grid rebuild and movable-set fixpoint on every expanded node, preferably by reusing a preallocated occupancy buffer; if retaining the current implementation, invoke the check only at a defined interval of expansions. Preserve deadlock detection correctness while keeping the C++ solver behavior unchanged.src/pages/shifting-mosaic-solver/board.ts (1)
75-101: 🗄️ Data Integrity & Integration | 🔵 Trivial | 💤 Low valueDocument (or enforce) that
loadConfigrequires a validated config.Line 92 silently drops cells whose
xis outside the new grid, so an unvalidated config yields a block that exists inblocksbut is missing fromblockAssignments. The only current caller validates first viavalidateConfig; making that precondition explicit in the docblock (or asserting it) prevents a future caller from producing this inconsistent state.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/pages/shifting-mosaic-solver/board.ts` around lines 75 - 101, Document or enforce in loadConfig that its ShiftingMosaicTest argument must already pass validateConfig before loading. Preserve the current board initialization logic, but make this precondition explicit so callers cannot create blocks outside blockAssignments through an unvalidated configuration.src/util/benchWasmSimd.ts (1)
17-26: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
pathToFileURLfor the dynamic import.An absolute OS path is not a portable ESM specifier (fails on Windows, where the sibling script
src/util/benchShiftingMosaic.tsexplicitly handleswin32).fileURLToPathis already imported fromnode:url.♻️ Proposed change
-import { fileURLToPath } from "node:url"; +import { fileURLToPath, pathToFileURL } from "node:url"; @@ - const createModule = (await import(wasmPath)).default; + const createModule = (await import(pathToFileURL(wasmPath).href)).default;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/util/benchWasmSimd.ts` around lines 17 - 26, Update loadModule to convert wasmPath to a file URL with pathToFileURL before the dynamic import, using the existing node:url utilities. Keep the wasmBinary loading and createModule configuration unchanged, and preserve portable behavior across Windows and other platforms.e2e/shiftingMosaicHeapLimit.slow.test.ts (1)
65-65: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse the absolute
/sm-wasm/...URL for the worker.The relative
../sm-wasm/...only happens to resolve correctly because the page sits at the root;e2e/shiftingMosaicWasmVariants.test.tsalready uses the absolute form. Switching keeps the two files consistent and immune to a future route nesting change.♻️ Proposed change
- const worker = new Worker("../sm-wasm/astar.worker.js", { type: "module" }); + const worker = new Worker("/sm-wasm/astar.worker.js", { type: "module" });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@e2e/shiftingMosaicHeapLimit.slow.test.ts` at line 65, Update the Worker constructor in shiftingMosaicHeapLimit to use the absolute “/sm-wasm/astar.worker.js” URL, matching the worker path used by shiftingMosaicWasmVariants and preserving module loading.test/shiftingMosaicSolver/aStar.test.ts (1)
179-205: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDelete this placeholder test — it asserts nothing but reports as passing coverage.
The body only builds unused shapes and
voids them, so the named scenario (bbox overlap without cell overlap) is never actually exercised. Either drop it or implement it againstblocksCollidewith normalized shapes.♻️ Proposed removal
- test("two L-shapes whose bboxes overlap but whose cells do not are not colliding", () => { - // Shape A: L pointing top-right - // X X - // X . - const shapeA: Position[] = [ - { x: 0, y: 0 }, - { x: 1, y: 0 }, - { x: 0, y: 1 }, - ]; - // Shape B: just bottom-right cell - // . X - const shapeB: Position[] = [{ x: 1, y: 1 }]; - // We use a 2x2 bbox shape B by also including {x:0,y:0} for bbox - // ... but we want a shape that fits the (1,1) corner exactly. - // Easier: use a real bbox-overlapping case where the cell at (1,1) is empty. - const shapeBReal: Position[] = [ - { x: 1, y: 1 }, - { x: 0, y: 1 }, - ]; - // shapeBReal's bbox is 2x2 (because we have x∈{0,1}, y∈{0,1}... wait y=1 only) - // Actually bbox is 2 wide, 1 tall (since min(y)=1, but bbox calc uses max(y)+1 = 2) - // Hmm bbox calc is just max+1 — let me redo with normalized shape. - // Skip this — use a more direct test below instead. - void shapeA; - void shapeB; - void shapeBReal; - }); -The next test (Lines 207-236) already covers the bbox-overlap-without-cell-overlap case.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/shiftingMosaicSolver/aStar.test.ts` around lines 179 - 205, Delete the placeholder test named “two L-shapes whose bboxes overlap but whose cells do not are not colliding” from the test suite. Its unused shape definitions and void expressions assert nothing, and the following test already covers the intended bbox-overlap-without-cell-overlap behavior.e2e/shiftingMosaicSolver.test.ts (1)
656-656: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueFixture path is CWD-relative.
readFileSync("test/resources/...")only works when Playwright is invoked from the repo root. Resolving against the test file (ortest.info().config.rootDir) makes it independent of the working directory.♻️ Proposed change
- const json = readFileSync("test/resources/shiftingMosaicTest2.json", "utf8"); + const json = readFileSync( + new URL("../test/resources/shiftingMosaicTest2.json", import.meta.url), + "utf8", + );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@e2e/shiftingMosaicSolver.test.ts` at line 656, Update the fixture loading in shiftingMosaicSolver.test.ts around the readFileSync call to resolve shiftingMosaicTest2.json from the test file location or Playwright’s configured rootDir instead of the process CWD, while preserving the existing UTF-8 read behavior.test/shiftingMosaicSolver/mem64.node.test.mjs (2)
17-26: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAlso skip when the mem64 build artifacts are absent.
The skip predicate only covers engine capability; a tree without
astar.mem64.mjs/.wasm(no wasm build run locally) fails with an opaque module-resolution error instead of skipping.♻️ Proposed guard
+import { existsSync, readFileSync } from "node:fs"; @@ const memory64 = typeof WebAssembly !== "undefined" && WebAssembly.validate(MEMORY64_PROBE); +const built = existsSync(mjsPath) && existsSync(wasmPath);and use
{ skip: !memory64 || !built }at the test declaration.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/shiftingMosaicSolver/mem64.node.test.mjs` around lines 17 - 26, Add a built-artifact availability check near the existing memory64 capability probe, verifying both astar.mem64.mjs and astar.mem64.wasm via the existing path utilities. Update the test declaration’s skip predicate to skip when either the runtime lacks memory64 support or the required artifacts are absent.
33-45: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
replaySolveschecks only the landing anchor, not move legality.Out-of-bounds or colliding moves emitted by a MEMORY64 codegen bug would still pass here, weakening the "did not silently corrupt the search" claim in the header comment.
validateSolutionintest/shiftingMosaicSolver/wasm.test.ts(Lines 100-151) already implements the full bounds/collision replay and could be mirrored.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/shiftingMosaicSolver/mem64.node.test.mjs` around lines 33 - 45, The replaySolves function must validate each move’s legality, not just update anchors and check the goal position. Mirror the bounds and collision checks from validateSolution in wasm.test.ts while replaying turns, returning false for invalid or colliding moves before applying them; preserve the existing goal-anchor validation for legal sequences.test/shiftingMosaicSolver/portfolio.worker.ts (1)
15-26: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAttach a rejection handler to
modulePromise.If module instantiation fails before the first message arrives, the rejection is unhandled at that moment and can tear the worker down without ever posting
{type:"error"}; the parent then only seesexit, losing the diagnostic.🛡️ Proposed fix
const modulePromise = (async () => { ... })(); +// Keep an early instantiation failure from surfacing as an unhandled rejection +// before the first message binds it to a try/catch. +modulePromise.catch(() => {});🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/shiftingMosaicSolver/portfolio.worker.ts` around lines 15 - 26, Attach a rejection handler to modulePromise so failures during WASM module import or instantiation are handled immediately and routed through the worker’s existing {type:"error"} reporting path before processing messages. Preserve successful module resolution and avoid leaving the promise rejection unhandled.test/shiftingMosaicSolver/wasm.test.ts (1)
25-28: 📐 Maintainability & Code Quality | 🔵 TrivialWorst-case suite wall time is very large — consider gating this behind an env flag like the slow e2e test.
44 fixtures × 210s timeout plus a 1230s long case means a stuck run can occupy hours of CI, and each fixture spawns one worker per portfolio arm with multi-GB wasm heaps.
e2e/shiftingMosaicHeapLimit.slow.test.tsalready uses anSM_SLOW_E2Eopt-in; the same pattern (or splitting the heavy fixtures into a nightly job) would keep PR feedback fast.Also applies to: 158-162
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/shiftingMosaicSolver/wasm.test.ts` around lines 25 - 28, Gate the heavy shifting-mosaic solver tests, including the per-puzzle and long-running cases using PER_PUZZLE_TIMEOUT_MS and LONG_TIMEOUT_MS, behind the existing SM_SLOW_E2E opt-in pattern used by shiftingMosaicHeapLimit.slow.test.ts. Keep the tests available when the flag is enabled while skipping them by default for normal CI runs.e2e/shiftingMosaicWasmVariants.test.ts (2)
288-305: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winThis test re-implements the selection rule instead of exercising the bridge, so it can't fail meaningfully.
selectedis computed by the ternary right above the assertions, soexpect(selected).toBe("threads-mem64")and thetoContaincheck are tautologies — a regression inwasmBridge's actual variant choice would still pass. Import/evaluate the bridge's selection function (as the other tests deliberately do not do for the probes, but here the point is the bridge behaviour) or assert on the variant the page actually loaded.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@e2e/shiftingMosaicWasmVariants.test.ts` around lines 288 - 305, Update the “bridge selects the largest heap the engine supports” test to exercise the bridge’s actual variant-selection behavior instead of deriving selected from the capability probe. Import or evaluate the bridge selection function, or inspect the variant actually loaded by the page, then assert that result against the expected variants while preserving the isolated shared-Memory64 expectation.
199-216: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse a whitespace-tolerant regex here. The current
maximum:131072n,address:"i64", andshared:truechecks are brittle against harmless Emscripten formatting or minifier changes; match the 8GiB / i64 / shared intent instead.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@e2e/shiftingMosaicWasmVariants.test.ts` around lines 199 - 216, Update the assertions in the “MEMORY64 threads build declares an 8GB shared 64-bit heap” test to use whitespace-tolerant regular expressions instead of exact string matching. Match the intended 8GiB maximum, i64 address mode, and shared memory properties while allowing harmless Emscripten or minifier formatting changes.src/common/coiRegister.ts (1)
9-14: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueConsider clearing
RELOAD_GUARDonce isolation succeeds.The early return at Line 11 leaves the key set for the rest of the session, so if isolation is ever lost mid-session (worker unregistered, shim updated) no further reload is attempted. Resetting it when
window.crossOriginIsolatedis true keeps the one-shot semantics without the sticky state.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/common/coiRegister.ts` around lines 9 - 14, Update registerCoiShim so the window.crossOriginIsolated early-return path clears the RELOAD_GUARD value before returning. Preserve the existing one-shot reload behavior when isolation is unavailable while allowing a future attempt if isolation is later lost.src/util/build.ts (1)
38-53: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winShifting-mosaic WASM artifact names are maintained in three places.
src/util/buildWasm.tsowns the variant table, but the artifact filenames are re-typed independently in the dist copy loop and the dev-server allowlist; adding or renaming a variant requires three synchronized edits, and a miss shows up only as an ENOENT at build time or a 404 at runtime.
src/util/build.ts#L38-L53: import a shared exported list (e.g.SHIFTING_MOSAIC_ASSETSderived fromSHIFTING_MOSAIC_VARIANTS) instead of the inline array.src/util/serve.ts#L46-L68: build the allowlistSetonce at module scope from that same shared list rather than re-creating a hardcoded set on every request.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/util/build.ts` around lines 38 - 53, The Shifting-mosaic WASM artifact filenames are duplicated across the build copy loop and dev-server allowlist. In src/util/build.ts lines 38-53, import and iterate the shared exported asset list derived from SHIFTING_MOSAIC_VARIANTS instead of the inline array; in src/util/serve.ts lines 46-68, create the allowlist Set once at module scope from that same list rather than rebuilding a hardcoded Set per request.src/pages/shifting-mosaic-solver/a-star/ParallelCascade.h (2)
277-287: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueProgress polling can lag the final value by up to 200 ms and never reports the terminal sum.
The loop exits as soon as
finished == ARMS, so the lastonProgresscall may be one sleep interval stale. A single post-join forward would make the UI's final node count exact.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/pages/shifting-mosaic-solver/a-star/ParallelCascade.h` around lines 277 - 287, Update the progress-reporting flow around the `finished` polling loop to invoke `onProgress` once after all worker threads have been joined, recomputing the sum from `progress` so the terminal callback reports the exact final node count.
7-13: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd
<algorithm>here forstd::min.ParallelCascade.husesstd::minbut doesn’t include<algorithm>itself; it only works becauseDragSolver.hpulls it in transitively viaBitGrid.h. Keep the dependency local to avoid a brittle header chain.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/pages/shifting-mosaic-solver/a-star/ParallelCascade.h` around lines 7 - 13, Add the <algorithm> header to ParallelCascade.h so its std::min usage has a direct include dependency, independent of transitive inclusion through DragSolver.h or BitGrid.h.src/util/buildWasm.ts (1)
115-137: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
"memory64" in variantkeys off property presence, not value.Adding an explicit
memory64: false(a natural edit) would silently produce a MEMORY64/8GB build. Typing the table and reading the value removes the trap.♻️ Proposed fix
-const SHIFTING_MOSAIC_VARIANTS = [ +const SHIFTING_MOSAIC_VARIANTS: { + outputJs: string; + threads?: boolean; + memory64?: boolean; +}[] = [ { outputJs: "astar.mjs" }, { outputJs: "astar.threads.mjs", threads: true }, { outputJs: "astar.mem64.mjs", memory64: true }, { outputJs: "astar.threads.mem64.mjs", threads: true, memory64: true }, -] as const; +]; @@ - memory64: "memory64" in variant, - maxMemory: "memory64" in variant ? "8GB" : "4GB", + memory64: variant.memory64 === true, + maxMemory: variant.memory64 === true ? "8GB" : "4GB", extraArgs: - "threads" in variant ? ["-pthread", "-sPTHREAD_POOL_SIZE=10"] : [], + variant.threads === true ? ["-pthread", "-sPTHREAD_POOL_SIZE=10"] : [],🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/util/buildWasm.ts` around lines 115 - 137, Update SHIFTING_MOSAIC_VARIANTS and the build mapping to read variant.memory64 as a boolean value rather than checking property presence. Ensure memory64 defaults to false for variants without memory64, while maxMemory remains 8GB only when the value is true and 4GB otherwise.src/pages/shifting-mosaic-solver/a-star/PARALLEL.md (1)
20-40: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winLayer 2 omits the sequential phase-2 fallback.
wasm_bindings.cpprunssolveArmsSequential(ungated, whole heap, ~4x budget, user-visible "sequential" phase notice) whenever the race returns empty. That is a materially different behaviour from what this section describes; worth a paragraph so the doc stays the source of truth.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/pages/shifting-mosaic-solver/a-star/PARALLEL.md` around lines 20 - 40, Update the Layer 2 section in PARALLEL.md to document that an empty parallel race result triggers the ungated solveArmsSequential phase, including its whole-heap execution, approximately 4x budget, and user-visible “sequential” notice. Clarify that this phase-2 fallback applies after the threaded race and preserve the existing layer behavior description.src/pages/shifting-mosaic-solver/a-star/wasm_bindings.cpp (1)
256-258: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low value
maxMs * 4can wrap for large caller budgets.
maxMsisunsigned; a caller passing anything above ~1.07e9 ms silently produces a tinyseqTotalMs. A saturating clamp keeps the fallback honest.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/pages/shifting-mosaic-solver/a-star/wasm_bindings.cpp` around lines 256 - 258, Update the seqTotalMs fallback in the opt<unsigned> configuration within the WASM binding to prevent maxMs * 4 from overflowing unsigned arithmetic. Saturate the computed fallback at the unsigned maximum while preserving zero when maxMs is zero and the existing caller-provided seqTotalMs value.src/pages/shifting-mosaic-solver/index.html (1)
115-125: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider announcing solver status changes to assistive tech.
#solution-status/#solution-progress-textupdate purely viatextContent, so screen-reader users get no signal that the search started, progressed, or failed. Anaria-live="polite"region would cover all three paths.♿ Suggested markup tweak
- <div id="solution-panel" class="hidden"> + <div id="solution-panel" class="hidden" aria-live="polite">🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/pages/shifting-mosaic-solver/index.html` around lines 115 - 125, Add an aria-live="polite" attribute to the existing solution-status or solution-progress-text announcement region in the solver markup, ensuring dynamic status, progress, and failure text updates are conveyed to assistive technologies without changing the existing visual behavior.src/pages/shifting-mosaic-solver/shiftingMosaicSolver.ts (1)
509-516: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winRevoking the object URL synchronously after
click()can abort the download.Some browsers start fetching the blob asynchronously; revoking in the same tick has historically cancelled the save. Defer the revoke (and the link removal) instead.
♻️ Suggested change
link.click(); - document.body.removeChild(link); - URL.revokeObjectURL(url); + setTimeout(() => { + link.remove(); + URL.revokeObjectURL(url); + }, 0);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/pages/shifting-mosaic-solver/shiftingMosaicSolver.ts` around lines 509 - 516, Update the download flow around the object URL and temporary link so link removal and URL.revokeObjectURL(url) occur asynchronously after link.click(), such as in a deferred callback. Preserve the existing filename, download behavior, and cleanup while avoiding synchronous revocation.src/pages/shifting-mosaic-solver/shiftingMosaicSolver.css (1)
547-574: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueHard-coded hex colors bypass the Material theme tokens.
The magenta/green/amber path and zone colors are literals, so they won't adapt with the rest of the theme (and may contrast poorly in dark mode). Consider lifting them into CSS custom properties defined once.
Also applies to: 605-649
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/pages/shifting-mosaic-solver/shiftingMosaicSolver.css` around lines 547 - 574, Replace the hard-coded path and zone color literals in the .sm-path-casing, .sm-path-line, .sm-path-start, and .sm-path-arrowhead rules, plus the related styles around the additional referenced section, with shared CSS custom properties. Define those properties once using the existing Material theme tokens so all magenta, green, and amber colors adapt consistently across themes.src/pages/shifting-mosaic-solver/solutionView.ts (1)
84-85: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOptional: redraw on layout change rather than only on window resize.
The overlay is pixel-measured but only recomputed on
resize; aResizeObserveron#solution-gridalso covers card-width changes and font/zoom reflows, and naturally coalesces bursts.Also applies to: 105-106
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/pages/shifting-mosaic-solver/solutionView.ts` around lines 84 - 85, Update the resize handling around SolutionView’s onResize and its related listener setup to observe the `#solution-grid` element with ResizeObserver, invoking drawPath when the grid’s layout changes. Replace or supplement window-resize-only handling while preserving cleanup for the observer when the view is disposed.src/pages/shifting-mosaic-solver/wasm/astar.worker.js (1)
11-34: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueNit: a rejected
astar.mjspromise stays cached forever.
loadModuleevicts the failed preferred entry but not the fallback, so a transient failure loadingastar.mjspermanently poisons the cache for every later message in this worker. Evicting on rejection insidegetModulecovers both.Also applies to: 51-61
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/pages/shifting-mosaic-solver/wasm/astar.worker.js` around lines 11 - 34, Update getModule so each cached module promise removes its own name from modulePromises when the dynamic import or module initialization rejects, covering both preferred and fallback modules. Preserve successful promise caching and ensure the original rejection still propagates to callers.src/pages/shifting-mosaic-solver/wasmBridge.ts (2)
137-140: 🚀 Performance & Scalability | 🔵 TrivialConsider capping concurrency by memory, not just core count.
poolSizecan reachPORTFOLIO.length(9) on a many-core machine, and each non-isolated arm owns its own wasm heap (up to 4GB, or 8GB with the mem64 variant). On hard boards where several arms actually search deep, that is tens of GB of committed heap; a lower cap for the mem64 variant (ordeviceMemory-aware sizing) would make the worst case survivable.Also applies to: 287-287
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/pages/shifting-mosaic-solver/wasmBridge.ts` around lines 137 - 140, Update the pool-size calculation near poolSize and its equivalent occurrence around the second reported location to include a memory-aware concurrency cap, using a lower limit for the mem64 variant or navigator.deviceMemory when available. Preserve the existing minimum of one and core-based sizing while ensuring poolSize cannot grow to an unsafe number of simultaneous WASM heaps.
253-259: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueDefensive: a malformed
donemessage throws insideonmessageand the arm never retires.
event.data.path.map(...)assumespathis always present; if it isn't, the exception escapes the handler,retire()is never reached, andpendingnever drains — the portfolio hangs with no callback. A guard (Array.isArray(event.data.path) ? … : []) keeps the failure contained to that arm.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/pages/shifting-mosaic-solver/wasmBridge.ts` around lines 253 - 259, Update the `type === "done"` branch in the message handler to guard `event.data.path` with `Array.isArray` before calling `.map`, using an empty `Turn[]` when it is missing or malformed. Preserve the existing mapping for valid paths so the arm can still reach `retire()` and drain `pending`.src/pages/shifting-mosaic-solver/a-star/AStar.h (1)
32-37: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueStale rationale: the state store is
StateTable, notunordered_map.
StateMapisStateTable<StateInfo>(Line 258); pointer stability comes from its block arena, not fromunordered_map's node guarantees. Worth correcting so a future reader doesn't validate the invariant against the wrong container.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/pages/shifting-mosaic-solver/a-star/AStar.h` around lines 32 - 37, Update the rationale comment above StateInfo::parent to identify StateTable as the state store and attribute pointer stability to its block-arena allocation, rather than unordered_map node guarantees. Preserve the existing explanation that states are never erased and the parent chain is used only for moves.src/pages/shifting-mosaic-solver/a-star/AStar.cpp (2)
329-331: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueImplicit invariant: this early return leaves
blockSafeAnchorDist_empty.
lpDisplacementCostguards only oninitialGoalPathCells_.empty(), which is false here (it was just assigned all-zeros on Line 329), so safety relies on "no corridor cell set ⇒ never indexblockSafeAnchorDist_". That holds today, but any future change that sets a corridor cell before this return turns Line 428 into an out-of-bounds read. GuardinglpDisplacementCostonblockSafeAnchorDist_.empty()too would make the coupling explicit.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/pages/shifting-mosaic-solver/a-star/AStar.cpp` around lines 329 - 331, Update lpDisplacementCost to return its existing fallback when either initialGoalPathCells_ or blockSafeAnchorDist_ is empty. This makes the early return after goalAnchorAt in the initialization flow safe even if corridor cells are populated before it.
795-825: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the unused
countSweepRectangleBlockershelper. It’s declared private inAStar.hand isn’t referenced anywhere; drop it unless you plan to wire it into the heuristic.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/pages/shifting-mosaic-solver/a-star/AStar.cpp` around lines 795 - 825, Remove the unused AStar::countSweepRectangleBlockers method from its definition and remove its corresponding private declaration in AStar.h; do not wire it into the heuristic or add replacement behavior.src/pages/shifting-mosaic-solver/a-star/DragSolver.cpp (1)
2205-2210: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueLatent dangling reference:
fromandstartAnchorsalias intoelites.
frompoints intoelitesandstartAnchorsbinds tofrom->anchors; Line 2281-2285 thenpush_back/resizethat same vector. It is safe today only because neither is dereferenced after the mutation. Copying the elite's anchors (or capturing an index instead of a pointer) would remove the hazard for the next edit in this block.Also applies to: 2243-2246
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/pages/shifting-mosaic-solver/a-star/DragSolver.cpp` around lines 2205 - 2210, Remove the aliasing hazard in the elite-selection logic around from and startAnchors by copying the selected elite’s anchors into independent storage before the later push_back/resize mutations of elites. Apply the same change to the corresponding logic around the additionally referenced block, while preserving the existing anchor values and selection behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.claude/settings.local.json:
- Line 12: Remove .claude/settings.local.json from the shared repository and
ensure it is ignored so local permissions remain untracked. If configuration
must be retained, narrow its command allowlist to repository-scoped operations
instead of broad access to bun, python, /tmp, E:\, process termination, and
package installation.
In @.github/workflows/test.yml:
- Around line 110-113: Update the “Run MEMORY64 wasm test (node)” workflow step
so it cannot succeed when mem64.node.test.mjs skips due to unavailable Memory64
support. Pin the step to a Node runtime that supports Memory64, or add an
assertion that the Memory64 test executed, while preserving the existing
artifact test command.
In `@src/common/coi-serviceworker.js`:
- Around line 11-13: Correct the opening comment in coi-serviceworker.js so it
no longer claims the file registers itself or can be imported from a page for
registration; describe only its service-worker header-injection role, with
registration handled by coiRegister.ts.
In `@src/common/common.css`:
- Line 1: Update the `@import` declaration in common.css to use the bare-string
URL form instead of url(...), while preserving the existing Google Fonts URL and
import parameters.
In `@src/pages/shifting-mosaic-solver/a-star/AStar.cpp`:
- Around line 1376-1377: Update the base metric initialization near computeRuns
in the A* optimization flow to use countSteps(turns), not runs.size(), so it
matches the step metric used for candidate acceptance at the checks around
tryReorderMerge. Preserve the strict lexicographic improvement condition for
both acceptance sites and ensure tryReorderMerge candidates cannot be accepted
unless they reduce turns or, when turns are unchanged, reduce the actual step
count.
In `@src/pages/shifting-mosaic-solver/a-star/DragSolver.cpp`:
- Around line 2170-2176: Prevent unbounded restart loops in both DragSolver.cpp
sites: cap the restart loop in searchJamRestarts (lines 2170-2176) and the
beam-round loop in searchBeamJam (lines 2381-2387) with the same intrinsic round
limit, or return early when maxMs and maxNodes are zero and cancellation is
unavailable. Preserve existing deadline, node-budget, and cancellation checks.
In `@src/pages/shifting-mosaic-solver/a-star/main.cpp`:
- Around line 392-398: Update usage() so its help text matches the documented
command-line interface: include parallel|sequential|twophase|arm engine values
and the --arm, --arm-gated, --jam-* , --max-heap-bytes, --generate, and --verify
options, while removing stale engine names or flags. Keep the existing argv0
formatting and return code unchanged.
- Around line 621-745: Replace the inline cascade arm configuration chain in the
engine "cascade" branch with the shared cascade::runArm implementation and
sequential ORDER used by ParallelCascade.h. Preserve the existing per-stage
statistics accumulation, stage naming, budgets, node limits, and final unit
fallback, while removing duplicated weights, expansion widths, jam settings, and
profile gates.
- Around line 58-82: Update loadFixture to validate that shapes.size() equals
initialAnchors.size() and that the parsed goalIndex is within both collections
before returning Fixture; throw a descriptive runtime error for invalid fixtures
so replaySolves, runVerify, and main never receive inconsistent data.
In `@src/pages/shifting-mosaic-solver/a-star/test/json_test.cpp`:
- Around line 67-91: Update validateSolution to validate every replayed turn,
not just the final goal position: after each move, reject anchors outside the
grid and reject any overlapping blocks, matching the bounds and collision checks
used by replayValid, collisionFree, and AStar::replaySolves. Preserve the
existing invalid-block and final-goal checks.
In `@src/pages/shifting-mosaic-solver/config.ts`:
- Around line 71-90: Validate each shape in the configuration parsing flow
before accepting it, ensuring every block’s minimum row and column coordinates
are zero (or normalize coordinates to be anchor-relative). Apply the same
anchor-relative requirement consistently for all entries in raw.shapes,
preserving the existing non-empty and MAX_BLOCKS checks and returning a clear
validation error for invalid offsets.
In `@src/pages/shifting-mosaic-solver/shiftingMosaicSolver.css`:
- Around line 448-451: Add the required empty line before the `color`
declaration in the `md-icon` CSS rule, or run the project formatter, so the
`declaration-empty-line-before` Stylelint check passes.
In `@src/pages/shifting-mosaic-solver/shiftingMosaicSolver.ts`:
- Around line 592-599: Move event.preventDefault() to the start of the drop
handler registered in the window.addEventListener("drop", ...) block, before
reading or validating event.dataTransfer.files. Preserve the existing overlay
reset, early return for missing files, and loadConfigFromFile(file) behavior.
- Around line 623-629: Replace the runtime process.env.NODE_ENV check in the
ShiftingMosaicSolver initialization block with a browser-safe compile-time or
page-specific environment mechanism, ensuring registerCoiShim() and
ShiftingMosaicSolverEditor() still initialize outside test builds without
referencing process at runtime.
In `@src/util/fuzzShiftingMosaic.ts`:
- Around line 99-111: The child-process handling in runCli and runOne must drain
or ignore stderr and tolerate malformed trailing JSON. In
src/util/fuzzShiftingMosaic.ts lines 99-111, update runCli to use stderr
"ignore" or await stderr consumption, and catch JSON.parse failures by returning
json: null. In src/util/benchShiftingMosaic.ts lines 113-136, apply the same
stderr and parse protections in runOne, returning its existing error result for
unparsable output; extract a shared helper only if it preserves both callers’
result contracts.
---
Nitpick comments:
In `@e2e/shiftingMosaicHeapLimit.slow.test.ts`:
- Line 65: Update the Worker constructor in shiftingMosaicHeapLimit to use the
absolute “/sm-wasm/astar.worker.js” URL, matching the worker path used by
shiftingMosaicWasmVariants and preserving module loading.
In `@e2e/shiftingMosaicSolver.test.ts`:
- Line 656: Update the fixture loading in shiftingMosaicSolver.test.ts around
the readFileSync call to resolve shiftingMosaicTest2.json from the test file
location or Playwright’s configured rootDir instead of the process CWD, while
preserving the existing UTF-8 read behavior.
In `@e2e/shiftingMosaicWasmVariants.test.ts`:
- Around line 288-305: Update the “bridge selects the largest heap the engine
supports” test to exercise the bridge’s actual variant-selection behavior
instead of deriving selected from the capability probe. Import or evaluate the
bridge selection function, or inspect the variant actually loaded by the page,
then assert that result against the expected variants while preserving the
isolated shared-Memory64 expectation.
- Around line 199-216: Update the assertions in the “MEMORY64 threads build
declares an 8GB shared 64-bit heap” test to use whitespace-tolerant regular
expressions instead of exact string matching. Match the intended 8GiB maximum,
i64 address mode, and shared memory properties while allowing harmless
Emscripten or minifier formatting changes.
In `@src/common/coiRegister.ts`:
- Around line 9-14: Update registerCoiShim so the window.crossOriginIsolated
early-return path clears the RELOAD_GUARD value before returning. Preserve the
existing one-shot reload behavior when isolation is unavailable while allowing a
future attempt if isolation is later lost.
In `@src/pages/shifting-mosaic-solver/a-star/AStar.cpp`:
- Around line 329-331: Update lpDisplacementCost to return its existing fallback
when either initialGoalPathCells_ or blockSafeAnchorDist_ is empty. This makes
the early return after goalAnchorAt in the initialization flow safe even if
corridor cells are populated before it.
- Around line 795-825: Remove the unused AStar::countSweepRectangleBlockers
method from its definition and remove its corresponding private declaration in
AStar.h; do not wire it into the heuristic or add replacement behavior.
In `@src/pages/shifting-mosaic-solver/a-star/AStar.h`:
- Around line 32-37: Update the rationale comment above StateInfo::parent to
identify StateTable as the state store and attribute pointer stability to its
block-arena allocation, rather than unordered_map node guarantees. Preserve the
existing explanation that states are never erased and the parent chain is used
only for moves.
In `@src/pages/shifting-mosaic-solver/a-star/DragSolver.cpp`:
- Around line 2205-2210: Remove the aliasing hazard in the elite-selection logic
around from and startAnchors by copying the selected elite’s anchors into
independent storage before the later push_back/resize mutations of elites. Apply
the same change to the corresponding logic around the additionally referenced
block, while preserving the existing anchor values and selection behavior.
In `@src/pages/shifting-mosaic-solver/a-star/PARALLEL.md`:
- Around line 20-40: Update the Layer 2 section in PARALLEL.md to document that
an empty parallel race result triggers the ungated solveArmsSequential phase,
including its whole-heap execution, approximately 4x budget, and user-visible
“sequential” notice. Clarify that this phase-2 fallback applies after the
threaded race and preserve the existing layer behavior description.
In `@src/pages/shifting-mosaic-solver/a-star/ParallelCascade.h`:
- Around line 277-287: Update the progress-reporting flow around the `finished`
polling loop to invoke `onProgress` once after all worker threads have been
joined, recomputing the sum from `progress` so the terminal callback reports the
exact final node count.
- Around line 7-13: Add the <algorithm> header to ParallelCascade.h so its
std::min usage has a direct include dependency, independent of transitive
inclusion through DragSolver.h or BitGrid.h.
In `@src/pages/shifting-mosaic-solver/a-star/wasm_bindings.cpp`:
- Around line 256-258: Update the seqTotalMs fallback in the opt<unsigned>
configuration within the WASM binding to prevent maxMs * 4 from overflowing
unsigned arithmetic. Saturate the computed fallback at the unsigned maximum
while preserving zero when maxMs is zero and the existing caller-provided
seqTotalMs value.
In `@src/pages/shifting-mosaic-solver/aStar.ts`:
- Around line 121-157: Move the field declarations for movableBlockIndices,
unsolvableAtStart, and goalAnchorBfsDist above the constructor that initializes
them, keeping their existing types and initializers unchanged. Leave the
constructor’s initialization logic and behavior intact.
- Around line 199-306: Reduce the cognitive complexity of search by extracting
successor generation into a private expandNode helper and the heap-pop,
stale-entry, and missing-node filtering into a private popFreshEntry helper.
Update search to delegate these responsibilities while preserving existing goal
checks, pruning, score updates, queue behavior, timeout handling, and path
reconstruction.
- Around line 200-205: Update AStar search() and its unsolvableAtStart and
timeout/exhaustion return paths to return a discriminated result such as {
status, turns } instead of a bare array, with distinct statuses for solved,
unsolvable, timed out, and exhausted search. Preserve the computed turn sequence
in turns and update all callers to consume the new result shape.
- Around line 483-512: Reduce the per-expansion cost of isDeadlocked by avoiding
a full occupancy-grid rebuild and movable-set fixpoint on every expanded node,
preferably by reusing a preallocated occupancy buffer; if retaining the current
implementation, invoke the check only at a defined interval of expansions.
Preserve deadlock detection correctness while keeping the C++ solver behavior
unchanged.
In `@src/pages/shifting-mosaic-solver/board.ts`:
- Around line 75-101: Document or enforce in loadConfig that its
ShiftingMosaicTest argument must already pass validateConfig before loading.
Preserve the current board initialization logic, but make this precondition
explicit so callers cannot create blocks outside blockAssignments through an
unvalidated configuration.
In `@src/pages/shifting-mosaic-solver/index.html`:
- Around line 115-125: Add an aria-live="polite" attribute to the existing
solution-status or solution-progress-text announcement region in the solver
markup, ensuring dynamic status, progress, and failure text updates are conveyed
to assistive technologies without changing the existing visual behavior.
In `@src/pages/shifting-mosaic-solver/shiftingMosaicSolver.css`:
- Around line 547-574: Replace the hard-coded path and zone color literals in
the .sm-path-casing, .sm-path-line, .sm-path-start, and .sm-path-arrowhead
rules, plus the related styles around the additional referenced section, with
shared CSS custom properties. Define those properties once using the existing
Material theme tokens so all magenta, green, and amber colors adapt consistently
across themes.
In `@src/pages/shifting-mosaic-solver/shiftingMosaicSolver.ts`:
- Around line 509-516: Update the download flow around the object URL and
temporary link so link removal and URL.revokeObjectURL(url) occur asynchronously
after link.click(), such as in a deferred callback. Preserve the existing
filename, download behavior, and cleanup while avoiding synchronous revocation.
In `@src/pages/shifting-mosaic-solver/solutionView.ts`:
- Around line 84-85: Update the resize handling around SolutionView’s onResize
and its related listener setup to observe the `#solution-grid` element with
ResizeObserver, invoking drawPath when the grid’s layout changes. Replace or
supplement window-resize-only handling while preserving cleanup for the observer
when the view is disposed.
In `@src/pages/shifting-mosaic-solver/wasm/astar.worker.js`:
- Around line 11-34: Update getModule so each cached module promise removes its
own name from modulePromises when the dynamic import or module initialization
rejects, covering both preferred and fallback modules. Preserve successful
promise caching and ensure the original rejection still propagates to callers.
In `@src/pages/shifting-mosaic-solver/wasmBridge.ts`:
- Around line 137-140: Update the pool-size calculation near poolSize and its
equivalent occurrence around the second reported location to include a
memory-aware concurrency cap, using a lower limit for the mem64 variant or
navigator.deviceMemory when available. Preserve the existing minimum of one and
core-based sizing while ensuring poolSize cannot grow to an unsafe number of
simultaneous WASM heaps.
- Around line 253-259: Update the `type === "done"` branch in the message
handler to guard `event.data.path` with `Array.isArray` before calling `.map`,
using an empty `Turn[]` when it is missing or malformed. Preserve the existing
mapping for valid paths so the arm can still reach `retire()` and drain
`pending`.
In `@src/util/benchWasmSimd.ts`:
- Around line 17-26: Update loadModule to convert wasmPath to a file URL with
pathToFileURL before the dynamic import, using the existing node:url utilities.
Keep the wasmBinary loading and createModule configuration unchanged, and
preserve portable behavior across Windows and other platforms.
In `@src/util/build.ts`:
- Around line 38-53: The Shifting-mosaic WASM artifact filenames are duplicated
across the build copy loop and dev-server allowlist. In src/util/build.ts lines
38-53, import and iterate the shared exported asset list derived from
SHIFTING_MOSAIC_VARIANTS instead of the inline array; in src/util/serve.ts lines
46-68, create the allowlist Set once at module scope from that same list rather
than rebuilding a hardcoded Set per request.
In `@src/util/buildWasm.ts`:
- Around line 115-137: Update SHIFTING_MOSAIC_VARIANTS and the build mapping to
read variant.memory64 as a boolean value rather than checking property presence.
Ensure memory64 defaults to false for variants without memory64, while maxMemory
remains 8GB only when the value is true and 4GB otherwise.
In `@test/shiftingMosaicSolver/aStar.test.ts`:
- Around line 179-205: Delete the placeholder test named “two L-shapes whose
bboxes overlap but whose cells do not are not colliding” from the test suite.
Its unused shape definitions and void expressions assert nothing, and the
following test already covers the intended bbox-overlap-without-cell-overlap
behavior.
In `@test/shiftingMosaicSolver/mem64.node.test.mjs`:
- Around line 17-26: Add a built-artifact availability check near the existing
memory64 capability probe, verifying both astar.mem64.mjs and astar.mem64.wasm
via the existing path utilities. Update the test declaration’s skip predicate to
skip when either the runtime lacks memory64 support or the required artifacts
are absent.
- Around line 33-45: The replaySolves function must validate each move’s
legality, not just update anchors and check the goal position. Mirror the bounds
and collision checks from validateSolution in wasm.test.ts while replaying
turns, returning false for invalid or colliding moves before applying them;
preserve the existing goal-anchor validation for legal sequences.
In `@test/shiftingMosaicSolver/portfolio.worker.ts`:
- Around line 15-26: Attach a rejection handler to modulePromise so failures
during WASM module import or instantiation are handled immediately and routed
through the worker’s existing {type:"error"} reporting path before processing
messages. Preserve successful module resolution and avoid leaving the promise
rejection unhandled.
In `@test/shiftingMosaicSolver/wasm.test.ts`:
- Around line 25-28: Gate the heavy shifting-mosaic solver tests, including the
per-puzzle and long-running cases using PER_PUZZLE_TIMEOUT_MS and
LONG_TIMEOUT_MS, behind the existing SM_SLOW_E2E opt-in pattern used by
shiftingMosaicHeapLimit.slow.test.ts. Keep the tests available when the flag is
enabled while skipping them by default for normal CI runs.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
| "PowerShell(Get-Process bun -ErrorAction SilentlyContinue | Select-Object Id, StartTime, CPU; if \\(Test-Path \"/tmp/junit.xml\"\\) { \\(Get-Item /tmp/junit.xml\\).Length } else { \"no junit\" })", | ||
| "Bash(ls -la /tmp/junit.xml 2>&1; echo ---; ps -ef | grep -i bun | grep -v grep)", | ||
| "Read(//tmp/**)", | ||
| "PowerShell(Stop-Process -Force)", |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== tracked files ==\n'
git ls-files -- .claude/settings.local.json .gitignore .claude 2>/dev/null || true
printf '\n== file size ==\n'
wc -l .claude/settings.local.json 2>/dev/null || true
printf '\n== settings file with line numbers ==\n'
cat -n .claude/settings.local.json 2>/dev/null || true
printf '\n== ignore rules mentioning claude ==\n'
rg -n --hidden --glob '.gitignore' --glob '.git/info/exclude' --glob '.gitignore' '(^|/)\.claude(/|$)|settings\.local\.json|claude' .gitignore .git/info/exclude 2>/dev/null || trueRepository: Valle12/islands-of-insight-tools
Length of output: 21401
Keep .claude/settings.local.json out of the shared repo. It’s tracked here and grants broad wildcard access to bun, python, /tmp, E:\, process termination, and package installs. Narrow this to repo-scoped commands or move it to an untracked local file.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.claude/settings.local.json at line 12, Remove .claude/settings.local.json
from the shared repository and ensure it is ignored so local permissions remain
untracked. If configuration must be retained, narrow its command allowlist to
repository-scoped operations instead of broad access to bun, python, /tmp, E:\,
process termination, and package installation.
| # bun cannot instantiate a Memory64 module, so the mem64 build is tested | ||
| # under node (preinstalled on the runner) instead of in `bun test`. | ||
| - name: Run MEMORY64 wasm test (node) | ||
| run: node --test test/shiftingMosaicSolver/mem64.node.test.mjs |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
set -o pipefail
tmp="$(mktemp)"
trap 'rm -f "$tmp"' EXIT
node --test --test-reporter=tap test/shiftingMosaicSolver/mem64.node.test.mjs | tee "$tmp"
! grep -q '# SKIP' "$tmp"Repository: Valle12/islands-of-insight-tools
Length of output: 1266
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== workflow excerpt =="
nl -ba .github/workflows/test.yml | sed -n '100,125p'
echo
echo "== test file excerpt =="
nl -ba test/shiftingMosaicSolver/mem64.node.test.mjs | sed -n '1,220p'
echo
echo "== search for memory64 detection =="
rg -n "memory64|skip: !memory64|# SKIP|test\.skip|wasm test" test/shiftingMosaicSolver .github/workflows/test.ymlRepository: Valle12/islands-of-insight-tools
Length of output: 233
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== workflow excerpt =="
sed -n '100,125p' .github/workflows/test.yml | cat -n
echo
echo "== test file excerpt =="
sed -n '1,220p' test/shiftingMosaicSolver/mem64.node.test.mjs | cat -n
echo
echo "== search for memory64 detection =="
rg -n "memory64|skip: !memory64|# SKIP|test\\.skip|wasm test" test/shiftingMosaicSolver .github/workflows/test.ymlRepository: Valle12/islands-of-insight-tools
Length of output: 5276
Fail the MEMORY64 step when the test is skipped.
mem64.node.test.mjs can pass with { skip: !memory64 }, so this job may go green without exercising the generated artifact. Pin a Node runtime with Memory64 support or assert that the test actually ran.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/test.yml around lines 110 - 113, Update the “Run MEMORY64
wasm test (node)” workflow step so it cannot succeed when mem64.node.test.mjs
skips due to unavailable Memory64 support. Pin the step to a Node runtime that
supports Memory64, or add an assertion that the Memory64 test executed, while
preserving the existing artifact test command.
| // This file doubles as its own registration script: import it from a page | ||
| // (see coiRegister.ts) and it registers itself; the service-worker context | ||
| // runs the header-injection half. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Comment claims a registration half that this file does not contain.
There is no window-side branch here — registration lives entirely in coiRegister.ts, and importing this file from a page would do nothing. Drop or correct the claim.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/common/coi-serviceworker.js` around lines 11 - 13, Correct the opening
comment in coi-serviceworker.js so it no longer claims the file registers itself
or can be imported from a page for registration; describe only its
service-worker header-injection role, with registration handled by
coiRegister.ts.
| @@ -1,4 +1,4 @@ | |||
| @import url("https://fonts.googleapis.com/css2?family=Material+Symbols+Outlined:opsz,wght,FILL,GRAD@20..48,100..700,0..1,-50..200&icon_names=add,cancel,delete,edit_location_alt,home,question_mark"); | |||
| @import url("https://fonts.googleapis.com/css2?family=Material+Symbols+Outlined:opsz,wght,FILL,GRAD@20..48,100..700,0..1,-50..200&icon_names=add,cancel,delete,download,edit_location_alt,home,question_mark,upload,upload_file"); | |||
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Stylelint import-notation fails on this line.
Use the bare-string form of @import.
🔧 Proposed fix
-@import url("https://fonts.googleapis.com/css2?family=Material+Symbols+Outlined:opsz,wght,FILL,GRAD@20..48,100..700,0..1,-50..200&icon_names=add,cancel,delete,download,edit_location_alt,home,question_mark,upload,upload_file");
+@import "https://fonts.googleapis.com/css2?family=Material+Symbols+Outlined:opsz,wght,FILL,GRAD@20..48,100..700,0..1,-50..200&icon_names=add,cancel,delete,download,edit_location_alt,home,question_mark,upload,upload_file";📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @import url("https://fonts.googleapis.com/css2?family=Material+Symbols+Outlined:opsz,wght,FILL,GRAD@20..48,100..700,0..1,-50..200&icon_names=add,cancel,delete,download,edit_location_alt,home,question_mark,upload,upload_file"); | |
| `@import` "https://fonts.googleapis.com/css2?family=Material+Symbols+Outlined:opsz,wght,FILL,GRAD@20..48,100..700,0..1,-50..200&icon_names=add,cancel,delete,download,edit_location_alt,home,question_mark,upload,upload_file"; |
🧰 Tools
🪛 Stylelint (17.14.0)
[error] 1-1: Expected "url("https://fonts.googleapis.com/css2?family=Material+Symbols+Outlined:opsz,wght,FILL,GRAD@20..48,100..700,0..1,-50..200&icon_names=add,cancel,delete,download,edit_location_alt,home,question_mark,upload,upload_file")" to be ""https://fonts.googleapis.com/css2?family=Material+Symbols+Outlined:opsz,wght,FILL,GRAD@20..48,100..700,0..1,-50..200&icon_names=add,cancel,delete,download,edit_location_alt,home,question_mark,upload,upload_file"" (import-notation)
(import-notation)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/common/common.css` at line 1, Update the `@import` declaration in
common.css to use the bare-string URL form instead of url(...), while preserving
the existing Google Fonts URL and import parameters.
Source: Linters/SAST tools
| const auto runs = computeRuns(turns); | ||
| const size_t baseSteps = runs.size(); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
baseSteps is a run count, not a step count — the reorder/merge acceptance test is too weak.
Line 1377 sets baseSteps = runs.size(), but computeRuns splits on direction changes while countSteps (used for cand at Line 1398 and Line 1411) folds a block's consecutive turns across direction changes. So for any plan where one block has a direction change inside a step, runs.size() > countSteps(turns) and a candidate with an unchanged step count passes the countSteps(cand) < baseSteps test.
tryReorderMerge never changes the turn count, so accepting a no-improvement candidate breaks the "every accepted rewrite strictly lowers (turns, then steps)" invariant asserted on Line 1425-1427: the fixpoint loop can churn until kMaxIterations / optimizeMaxMs, each iteration replaying the whole plan.
🐛 Proposed fix
const auto runs = computeRuns(turns);
- const size_t baseSteps = runs.size();
+ const size_t baseSteps = countSteps(turns);Also applies to: 1398-1398, 1411-1411
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/pages/shifting-mosaic-solver/a-star/AStar.cpp` around lines 1376 - 1377,
Update the base metric initialization near computeRuns in the A* optimization
flow to use countSteps(turns), not runs.size(), so it matches the step metric
used for candidate acceptance at the checks around tryReorderMerge. Preserve the
strict lexicographic improvement condition for both acceptance sites and ensure
tryReorderMerge candidates cannot be accepted unless they reduce turns or, when
turns are unchanged, reduce the actual step count.
| if ( | ||
| !Array.isArray(raw.shapes) || | ||
| raw.shapes.length === 0 || | ||
| !raw.shapes.every(isPositionArray) | ||
| ) { | ||
| return { | ||
| ok: false, | ||
| error: "shapes must be a non-empty array of cell arrays.", | ||
| }; | ||
| } | ||
| if (raw.shapes.length > MAX_BLOCKS) { | ||
| return { | ||
| ok: false, | ||
| error: `A config may define at most ${MAX_BLOCKS} blocks.`, | ||
| }; | ||
| } | ||
| const shapes = raw.shapes as Position[][]; | ||
| if (shapes.some(shape => shape.length === 0)) { | ||
| return { ok: false, error: "Every shape must have at least one cell." }; | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -nP -C3 'shapeBoxWidth|boxWidth|maxX\s*\+\s*1' src/pages/shifting-mosaic-solver/a-starRepository: Valle12/islands-of-insight-tools
Length of output: 17704
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- config.ts ---'
sed -n '1,220p' src/pages/shifting-mosaic-solver/config.ts
echo
echo '--- search for normalization helpers/usages ---'
rg -n "normalize|normalized|anchor-relative|smallest x|smallest y|shapeBoxWidth|shapeBoxHeight|boxWidth|boxHeight" src/pages/shifting-mosaic-solver -g '!**/*.map'Repository: Valle12/islands-of-insight-tools
Length of output: 12156
Normalize or reject non-anchor-relative shapes on upload Both the TypeScript and C++ solvers treat each block’s footprint as max + 1 on each axis, so configs with leading offsets other than 0 produce the wrong bounding box. That makes valid edge placements look invalid, and negative offsets can let blocks move out of bounds.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/pages/shifting-mosaic-solver/config.ts` around lines 71 - 90, Validate
each shape in the configuration parsing flow before accepting it, ensuring every
block’s minimum row and column coordinates are zero (or normalize coordinates to
be anchor-relative). Apply the same anchor-relative requirement consistently for
all entries in raw.shapes, preserving the existing non-empty and MAX_BLOCKS
checks and returning a clear validation error for invalid offsets.
| md-icon { | ||
| --md-icon-size: 48px; | ||
| color: var(--md-sys-color-primary); | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Stylelint failure: declaration-empty-line-before at line 450.
Add the blank line (or run the formatter) so the lint job passes.
🎨 Proposed fix
md-icon {
--md-icon-size: 48px;
+
color: var(--md-sys-color-primary);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| md-icon { | |
| --md-icon-size: 48px; | |
| color: var(--md-sys-color-primary); | |
| } | |
| md-icon { | |
| --md-icon-size: 48px; | |
| color: var(--md-sys-color-primary); | |
| } |
🧰 Tools
🪛 Stylelint (17.14.0)
[error] 450-450: Expected empty line before declaration (declaration-empty-line-before)
(declaration-empty-line-before)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/pages/shifting-mosaic-solver/shiftingMosaicSolver.css` around lines 448 -
451, Add the required empty line before the `color` declaration in the `md-icon`
CSS rule, or run the project formatter, so the `declaration-empty-line-before`
Stylelint check passes.
Source: Linters/SAST tools
| window.addEventListener("drop", event => { | ||
| dragDepth = 0; | ||
| this.dropOverlay.classList.add("hidden"); | ||
| const file = event.dataTransfer?.files?.[0]; | ||
| if (!file) return; | ||
| event.preventDefault(); | ||
| void this.loadConfigFromFile(file); | ||
| }); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
preventDefault() runs too late in the drop handler.
If the drop carries no accessible file (empty files, a dropped folder in some browsers, or a non-file payload after dragover was already prevented), the handler returns before event.preventDefault(), letting the browser perform its default action — navigating away from the page and discarding the editor state. Prevent first, then inspect.
🐛 Proposed fix
window.addEventListener("drop", event => {
+ if (!hasFiles(event)) return;
+ event.preventDefault();
dragDepth = 0;
this.dropOverlay.classList.add("hidden");
const file = event.dataTransfer?.files?.[0];
if (!file) return;
- event.preventDefault();
void this.loadConfigFromFile(file);
});🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/pages/shifting-mosaic-solver/shiftingMosaicSolver.ts` around lines 592 -
599, Move event.preventDefault() to the start of the drop handler registered in
the window.addEventListener("drop", ...) block, before reading or validating
event.dataTransfer.files. Preserve the existing overlay reset, early return for
missing files, and loadConfigFromFile(file) behavior.
| if (process.env.NODE_ENV !== "test") { | ||
| new ShiftingMosaicSolverEditor(); | ||
| // Opt this page into cross-origin isolation (wasm threads) where the | ||
| // browser supports the shim; everything degrades to the worker portfolio | ||
| // otherwise. | ||
| registerCoiShim(); | ||
| editor = new ShiftingMosaicSolverEditor(); | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check bundler define/env handling and existing usages
fd -e ts . src/util --exec rg -n "NODE_ENV|define|env" {}
rg -n "process\.env" src --type=ts -g '!**/node_modules/**'Repository: Valle12/islands-of-insight-tools
Length of output: 1114
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect package/build tooling and the solver page file
git ls-files | rg '^(package\.json|bun\.fig|vite\.config|tsconfig|src/pages/shifting-mosaic-solver/shiftingMosaicSolver\.ts|src/pages/rolling-blocks-solver/rollingBlocksSolver\.ts|src/pages/phasic-dial-solver/phasicDialSolver\.ts|src/util/buildWasm\.ts)$' || true
echo '--- package.json ---'
cat -n package.json
echo '--- bun.fig / vite config candidates ---'
for f in bun.fig vite.config.ts vite.config.js vite.config.mts vite.config.mjs; do
if [ -f "$f" ]; then
echo "### $f"
cat -n "$f"
fi
done
echo '--- shiftingMosaicSolver.ts excerpt ---'
sed -n '600,650p' src/pages/shifting-mosaic-solver/shiftingMosaicSolver.ts
echo '--- rollingBlocksSolver.ts excerpt ---'
sed -n '320,350p' src/pages/rolling-blocks-solver/rollingBlocksSolver.ts
echo '--- phasicDialSolver.ts excerpt ---'
sed -n '160,190p' src/pages/phasic-dial-solver/phasicDialSolver.ts
echo '--- process.env usages ---'
rg -n "process\.env|NODE_ENV|define:" src package.json bun.fig vite.config.* -g '!**/node_modules/**'Repository: Valle12/islands-of-insight-tools
Length of output: 4091
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- src/util/build.ts ---'
wc -l src/util/build.ts
cat -n src/util/build.ts
echo '--- src/util/serve.ts ---'
wc -l src/util/serve.ts
cat -n src/util/serve.ts
echo '--- src/util/preload.ts ---'
wc -l src/util/preload.ts
cat -n src/util/preload.tsRepository: Valle12/islands-of-insight-tools
Length of output: 6229
🌐 Web query:
Bun.build browser bundle process.env.NODE_ENV replaced at build time
💡 Result:
Yes, you can replace process.env.NODE_ENV at build time in Bun using two primary methods: the --define flag or the env configuration option [1][2][3]. Method 1: Using --define The --define flag allows you to statically replace any identifier or property reference with a constant value at build time [1][4]. This is the recommended approach for forcing specific environment variables like NODE_ENV into your bundle [4][5]. Usage: bun build src/index.ts --define 'process.env.NODE_ENV="production"' Because this replaces the code before execution, it also enables optimizations like dead code elimination, where Bun automatically removes code blocks that become unreachable (e.g., if (false) {... }) [1][4][5]. Method 2: Using the env configuration The env option in the Bun.build API (or the --env flag in the CLI) controls how environment variables are handled during bundling [2][3]. - Setting --env inline or env: "inline" instructs Bun to automatically find process.env references and replace them with the current values from your shell environment at build time [2][3]. - If you only want to inline specific variables, you can use a prefix pattern, such as --env MY_APP_* [2][3][6]. Important Considerations: - Static Replacement: When you use --define or --env, the values are baked into the generated JavaScript bundle [1][3]. This means if you change the environment variable on the machine running the final bundle, the value inside the bundle will not change because it was already replaced with a literal string [1][3]. - --compile vs. --build: Be aware that when using --compile, Bun historically had behaviors where it might bake in default environment values [7]. Always explicitly define your target environment variables using --define for predictable results in compiled binaries [4][5].
Citations:
- 1: https://bun.com/docs/guides/runtime/define-constant
- 2: https://bun.com/reference/bun/build
- 3: https://bun.com/docs/bundler
- 4: https://bun.com/guides/runtime/build-time-constants
- 5: https://bun.sh/docs/guides/runtime/build-time-constants
- 6: https://bun.sh/reference/bun/BuildConfig/env
- 7:
NODE_ENVenvironment variable (developmentif not provided) is somehow getting baked-in inbun build --compileoven-sh/bun#26994
🌐 Web query:
Bun.build default process.env.NODE_ENV browser bundle inline default
💡 Result:
In Bun, the handling of process.env.NODE_ENV during bundling depends on the target and configuration. For browser targets, process.env does not exist at runtime, so Bun typically handles NODE_ENV through build-time inlining to facilitate optimizations like dead code elimination [1]. Key behaviors include: 1. Automatic Inlining: Bun's bundler historically auto-inlines process.env.NODE_ENV [1]. For browser-targeted builds, this is generally intended behavior, as it allows tools like React to strip out development-only code [1]. 2. Controlling Inlining: - You can explicitly control environment variable inlining using the env option in Bun.build or the --env flag in the CLI [2][3][4]. - Setting env: "disable" is intended to stop environment variable inlining [3][4]. Note that there have been historical issues where NODE_ENV continued to inline despite this setting [5], but modern versions (1.2+) have implemented fixes to gate these defaults based on the target [1][6]. - If you need to force a specific value or prevent inlining, you can use the define option [7][3]. To prevent inlining by keeping the expression at runtime, you can pass: define: { 'process.env.NODE_ENV': 'process.env.NODE_ENV' } [5]. 3. Build-time Constants: The --define flag is the primary way to replace identifiers like process.env.NODE_ENV with static strings (e.g., "production") across your bundle [8][7][9]. This is distinct from automatic environment variable injection and is the recommended approach for setting fixed environment configurations [7]. If you are experiencing unexpected inlining, ensure you are using the latest version of Bun, as recent updates have specifically improved how NODE_ENV is resolved for non-browser targets [1][6].
Citations:
- 1: bun build --compile: read NODE_ENV at runtime, set argv[0] to the executable path oven-sh/bun#32851
- 2: https://bun.com/reference/bun/build
- 3: https://bun.com/docs/bundler
- 4: https://bun.sh/reference/bun/BuildConfig/env
- 5:
Bun.build({ env: "disable" })still inlinesprocess.env.NODE_ENVoven-sh/bun#19508 - 6:
bun buildinlinesprocess.envwhen--targetis set tobunis this intentional? oven-sh/bun#12695 - 7: https://bun.com/docs/guides/runtime/define-constant
- 8: https://bun.sh/docs/guides/runtime/build-time-constants
- 9: https://bun.com/guides/runtime/build-time-constants
🌐 Web query:
site:bun.com Bun bundler process.env.NODE_ENV default inline
💡 Result:
In Bun, the default behavior for bundling is to disable environment variable inlining, which means process.env.NODE_ENV is not automatically replaced in your bundled code [1]. To inline process.env.NODE_ENV, you have two primary approaches depending on your goals: 1. Using the --define flag: This allows you to explicitly define the value of process.env.NODE_ENV at build time [2][3][4]. This is highly recommended for enabling optimizations like dead code elimination [3]. Example (CLI): bun build --define 'process.env.NODE_ENV="production"' src/index.ts Example (Bun.build API): await Bun.build({ entrypoints: ['./index.ts'], define: { 'process.env.NODE_ENV': '"production"' } }); 2. Using the env option: If you want to automatically inline environment variables from your host system into your bundle, you can set the env property to "inline" or use a prefix [5][1][6]. Example (CLI): bun build src/index.ts --env inline Example (Bun.build API): await Bun.build({ entrypoints: ['./index.ts'], env: "inline", // Inlines all found env vars }); Note: Inlining environment variables works only with literal process.env.FOO references [1]. If you use indirect access (e.g., const env = process.env; env.NODE_ENV), the bundler cannot perform the replacement [1]. For production builds where dead code elimination is critical, using the --define approach is the most reliable method [3].
Citations:
- 1: https://bun.com/docs/bundler/fullstack
- 2: https://bun.com/docs/guides/runtime/build-time-constants
- 3: https://bun.com/docs/guides/runtime/define-constant
- 4: https://bun.com/guides/runtime/build-time-constants
- 5: https://bun.com/docs/bundler
- 6: https://bun.com/reference/bun/BuildConfig/env
🌐 Web query:
site:bun.com/docs/bundler process.env.NODE_ENV default inlined disabled
💡 Result:
In the context of the Bun runtime and bundler, environment variable inlining refers to the process of replacing references like process.env.NODE_ENV with their actual values (as string literals) at build time [1][2]. The default behavior for environment variable inlining in Bun's bundler is disable [1][3]. This means that by default, Bun does not automatically replace process.env references with their values during the build process [2]. To enable this behavior, users must explicitly configure it—for example, by setting the env option in their bunfig.toml or passing the --env flag to the Bun CLI, typically specifying either inline (to inline all) or a prefix (such as PUBLIC_*) to limit which variables are inlined [1][2]. In standard Node.js, process.env.NODE_ENV is not inlined by default by the runtime itself; it is simply a property accessed at runtime that reflects the value of the NODE_ENV environment variable set in the host system. Inlining is a feature specific to build tools and bundlers (like Bun, esbuild, Webpack, or Vite) used to optimize client-side or bundled code. If you are using Bun's bundler and want to ensure process.env.NODE_ENV is explicitly set to a production value in your final output, you can use the define option in the build configuration [4][5]: await Bun.build({ //... define: { "process.env.NODE_ENV": JSON.stringify("production"), }, });
Citations:
- 1: https://bun.com/docs/bundler/html-static
- 2: https://bun.com/docs/bundler
- 3: https://bun.com/docs/bundler/fullstack
- 4: https://bun.com/docs/bundler/executables
- 5: https://bun.com/docs/bundler/executables.md
🌐 Web query:
site:bun.com/docs/bundler/fullstack process.env.NODE_ENV browser bundle inline Bun
💡 Result:
Bun can inline process.env.NODE_ENV and other environment variables in browser bundles during the build process [1]. To enable this, you must configure the env option within your bunfig.toml file [1]. By default, this feature is disabled [1]. You can configure it to inline specific environment variables or all of them [1]: [serve.static] env = "PUBLIC_*" # Inlines only variables starting with PUBLIC_ # env = "inline" # Inlines all environment variables # env = "disable" # Disables replacement (default) Note that this replacement only works for literal process.env.FOO references [1]. It does not support indirect access or alternatives like import.meta.env [1]. If a referenced variable is not set during the build, it may cause a ReferenceError in the browser at runtime [1].
Citations:
Inline process.env.NODE_ENV for this page bundle. process.env isn’t available in the browser bundle unless Bun rewrites it at build time, and this page doesn’t set that up. If it reaches runtime, initialization fails with ReferenceError.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/pages/shifting-mosaic-solver/shiftingMosaicSolver.ts` around lines 623 -
629, Replace the runtime process.env.NODE_ENV check in the ShiftingMosaicSolver
initialization block with a browser-safe compile-time or page-specific
environment mechanism, ensuring registerCoiShim() and
ShiftingMosaicSolverEditor() still initialize outside test builds without
referencing process at runtime.
| const proc = Bun.spawn([exe, ...args], { stdout: "pipe", stderr: "pipe" }); | ||
| const killer = setTimeout(() => proc.kill(), timeoutMs); | ||
| const [stdout, exitCode] = await Promise.all([ | ||
| new Response(proc.stdout).text(), | ||
| proc.exited, | ||
| ]); | ||
| clearTimeout(killer); | ||
| const lastJson = stdout | ||
| .split("\n") | ||
| .map(l => l.trim()) | ||
| .filter(l => l.startsWith("{")) | ||
| .pop(); | ||
| return { json: lastJson ? JSON.parse(lastJson) : null, exitCode }; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Both harnesses duplicate the same unsafe child-process pattern: stderr: "pipe" is never drained and the trailing line is parsed without a guard. A chatty child can fill the stderr pipe and hang until the kill timer; a malformed line throws out of the sweep.
src/util/fuzzShiftingMosaic.ts#L99-L111: inrunCli, switchstderrto"ignore"(or awaitnew Response(proc.stderr).text()alongside stdout) and wrapJSON.parse(lastJson)in try/catch returningnull.src/util/benchShiftingMosaic.ts#L113-L136: apply the same change inrunOne, returning anerrorresult instead of throwing on unparsable output — ideally by extracting the shared spawn/parse helper into one module used by both scripts.
📍 Affects 2 files
src/util/fuzzShiftingMosaic.ts#L99-L111(this comment)src/util/benchShiftingMosaic.ts#L113-L136
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/util/fuzzShiftingMosaic.ts` around lines 99 - 111, The child-process
handling in runCli and runOne must drain or ignore stderr and tolerate malformed
trailing JSON. In src/util/fuzzShiftingMosaic.ts lines 99-111, update runCli to
use stderr "ignore" or await stderr consumption, and catch JSON.parse failures
by returning json: null. In src/util/benchShiftingMosaic.ts lines 113-136, apply
the same stderr and parse protections in runOne, returning its existing error
result for unparsable output; extract a shared helper only if it preserves both
callers’ result contracts.
|
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/pages/shifting-mosaic-solver/a-star/main.cpp (1)
128-164: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winReject values that overflow each option’s destination type.
nextU()only bounds-checksunsigned long long; the subsequent casts silently truncate. For example,--max-nodes 4294967296becomes0, disabling the node cap. Validate against each destination type’s maximum before casting.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/pages/shifting-mosaic-solver/a-star/main.cpp` around lines 128 - 164, Update the argument parsing branches in main to validate each nextU() result against the maximum representable value of its destination type before every static_cast, including uint8_t, uint16_t, uint32_t, and int destinations. Reject out-of-range values through the existing argument-error path, while preserving valid assignments and the special maxStates/maxHeapBytes configuration updates.
🧹 Nitpick comments (13)
src/pages/shifting-mosaic-solver/a-star/test/DragSolverTestOracle.h (1)
204-206: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
aloneis dead code. It's built and assigned but never read — the goal-anchor check below only tests the bbox. Drop it to keep the oracle's intent clear.♻️ Proposed cleanup
- std::vector<Position> alone(p.anchors.size(), {127, 127}); - alone[0] = t; // Target just needs the goal's bbox in-grid.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/pages/shifting-mosaic-solver/a-star/test/DragSolverTestOracle.h` around lines 204 - 206, Remove the unused alone vector initialization and assignment near the goal-anchor check in the test oracle. Keep the existing bbox validation and surrounding target setup unchanged.test/shiftingMosaicSolver/aStar.collision.test.ts (1)
12-51: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
makeAStar/applyTurnsare copy-pasted verbatim across three test files.aStar.collision.test.ts,aStar.primitives.test.ts, andaStar.search.test.tseach carry the same helpers (and in this fileapplyTurns/Nodeare never used). Extracting them into a sharedtest/shiftingMosaicSolver/aStarTestHelpers.tswould kill the duplication and the unused copies.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/shiftingMosaicSolver/aStar.collision.test.ts` around lines 12 - 51, Extract the shared makeAStar and applyTurns helpers into test/shiftingMosaicSolver/aStarTestHelpers.ts, then import and reuse them from aStar.collision.test.ts, aStar.primitives.test.ts, and aStar.search.test.ts. Remove the unused applyTurns and Node references from aStar.collision.test.ts while preserving all existing test behavior.test/shiftingMosaicSolver/board.state.test.ts (1)
6-31: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueShared
Boardtest fixture is copy-pasted across all four board suites. TheeditorMockshape, thebeforeEachDOM setup, and theboard/internalwiring are byte-identical in every file; extracting one helper (e.g.makeBoardFixture()in a shared test module) removes four copies that must otherwise be updated together whenever the editor interface changes.
test/shiftingMosaicSolver/board.state.test.ts#L6-L31: replace the inline mock/beforeEachwith the shared fixture helper.test/shiftingMosaicSolver/board.geometry.test.ts#L11-L36: same replacement, keeping the localinternalalias.test/shiftingMosaicSolver/board.pointer.test.ts#L6-L31: same replacement; also move the repeated localdispatchhelper into the shared module.test/shiftingMosaicSolver/board.render.test.ts#L6-L31: same replacement.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/shiftingMosaicSolver/board.state.test.ts` around lines 6 - 31, Extract the duplicated Board test setup into a shared makeBoardFixture helper, including the editorMock shape, DOM initialization, Board construction, and internal alias wiring. Replace the inline fixture in test/shiftingMosaicSolver/board.state.test.ts lines 6-31, board.geometry.test.ts lines 11-36 while preserving its local internal alias, and board.render.test.ts lines 6-31. In test/shiftingMosaicSolver/board.pointer.test.ts lines 6-31, use the shared fixture and move its repeated dispatch helper into the shared test module.test/shiftingMosaicSolver/aStar.search.test.ts (1)
216-216: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
toHaveLengthfor clearer failure output.♻️ Proposed tweak
- expect(turns.length).toBe(1); + expect(turns).toHaveLength(1);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/shiftingMosaicSolver/aStar.search.test.ts` at line 216, Update the assertion in the aStar search test to use the turns collection’s toHaveLength matcher instead of comparing turns.length with toBe(1), preserving the expected length of one.Source: Linters/SAST tools
e2e/rollingBlocksSolver.placement.test.ts (2)
56-207: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThree full-board aria snapshots assert the same state.
The snapshots after each
dragTo(lines 56-101, 109-154, 162-207) are byte-identical, so ~140 lines restate one invariant: no block was created. Asserting the blocks list is empty (plus the relevant cell states) once per drag would give the same signal at a fraction of the maintenance cost.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@e2e/rollingBlocksSolver.placement.test.ts` around lines 56 - 207, The test repeats identical full-board aria snapshots after each drag instead of checking the relevant invariant. In the rolling blocks placement test, replace the three redundant snapshots with concise assertions that the Blocks section remains empty and verify only the affected cell states needed for each drag; retain one initial full-board snapshot if useful.
3-31: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the unused
setupSolvedPuzzlehelper — this file never calls it. Delete it here, or move the shared setup into a common helper since the same block is duplicated in the other rolling-blocks solver tests.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@e2e/rollingBlocksSolver.placement.test.ts` around lines 3 - 31, Remove the unused setupSolvedPuzzle helper from this test file. Do not alter the surrounding tests; only consider moving its setup logic to an existing shared helper if required to eliminate the duplicated setup across rolling-blocks solver tests.e2e/phasicDialSolver.solvable.test.ts (1)
11-30: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffConsider stable hooks instead of Material Web internal DOM chains.
Selectors like
md-outlined-text-field > .text-field > .field > .input-wrapper > .inputcouple these tests to@material/webshadow/internal class names, so a library bump can break them silently. Adata-testidor role query per matrix cell would be more durable. Same pattern repeats at lines 64-143.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@e2e/phasicDialSolver.solvable.test.ts` around lines 11 - 30, Replace the Material Web internal DOM-chain selectors in the matrix-cell interactions throughout the test, including the repeated cases later in the file, with stable per-cell hooks such as data-testid attributes or accessible role/name queries. Update the selectors used by the phasicDialSolver test without changing the entered values or matrix-cell targets.e2e/rollingBlocksSolver.solution.test.ts (1)
157-181: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse the setup helper instead of a third verbatim copy.
Lines 157-181 repeat
setupSolvedPuzzle's body exactly up to the "Calculate Moves" click (also inlined at lines 37-64). Splitting the helper into abuildPuzzle(page)step plus the solve/assert step would remove both copies.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@e2e/rollingBlocksSolver.solution.test.ts` around lines 157 - 181, Refactor the duplicated puzzle setup in the test by extracting the shared actions from setupSolvedPuzzle and this inline block into a buildPuzzle(page) helper. Update setupSolvedPuzzle to reuse buildPuzzle before calculating moves, and replace the inline setup in the current test with the helper while preserving the existing solve and assertion steps.e2e/shiftingMosaicSolver.solving.test.ts (1)
1-58: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the duplicated shifting-mosaic test preamble into a shared helper module. All four files carry a byte-identical 58-line block (
SHIFTING_MOSAIC_URL,SAMPLE_CONFIG,Cell,cellAt,cellCenter,paintShape,paintWithGap, plus thefsimport), so each file also ships declarations it never uses and any selector change must be applied four times.
e2e/shiftingMosaicSolver.solving.test.ts#L1-L58: move the shared constants/helpers into e.g.e2e/helpers/shiftingMosaic.tsand import onlycellAt,paintShape,paintWithGap; drop the unusedSAMPLE_CONFIGandreadFileSyncimport.e2e/shiftingMosaicSolver.blocks.test.ts#L1-L58: importcellAt/paintShapefrom the new helper; dropSAMPLE_CONFIG,paintWithGap, and thereadFileSyncimport.e2e/shiftingMosaicSolver.config.test.ts#L1-L58: importcellAtandSAMPLE_CONFIGfrom the new helper; drop the unusedpaintShape/paintWithGap/cellCentercopies.e2e/shiftingMosaicSolver.editing.test.ts#L1-L58: importcellAt/paintShapefrom the new helper; dropSAMPLE_CONFIG,paintWithGap, and thereadFileSyncimport.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@e2e/shiftingMosaicSolver.solving.test.ts` around lines 1 - 58, Extract the shared SHIFTING_MOSAIC_URL, SAMPLE_CONFIG, Cell, cellAt, cellCenter, paintShape, and paintWithGap declarations into e2e/helpers/shiftingMosaic.ts. In e2e/shiftingMosaicSolver.solving.test.ts#L1-L58, import cellAt, paintShape, and paintWithGap and remove SAMPLE_CONFIG/readFileSync; in e2e/shiftingMosaicSolver.blocks.test.ts#L1-L58, import cellAt/paintShape and remove SAMPLE_CONFIG, paintWithGap, and readFileSync; in e2e/shiftingMosaicSolver.config.test.ts#L1-L58, import cellAt/SAMPLE_CONFIG and remove paintShape, paintWithGap, and cellCenter; in e2e/shiftingMosaicSolver.editing.test.ts#L1-L58, import cellAt/paintShape and remove SAMPLE_CONFIG, paintWithGap, and readFileSync.src/pages/shifting-mosaic-solver/a-star/AStarOptimizer.cpp (1)
238-239: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
deadlineFrom()here.
SolverClock.his already included and exists precisely to name this ternary; keeping a local copy re-introduces the inlining/dead-code problem its header comment describes.♻️ Proposed refactor
- const uint64_t deadline = - cfg_.optimizeMaxMs == 0 ? 0 : nowMs() + cfg_.optimizeMaxMs; + const uint64_t deadline = deadlineFrom(cfg_.optimizeMaxMs);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/pages/shifting-mosaic-solver/a-star/AStarOptimizer.cpp` around lines 238 - 239, Replace the inline deadline ternary in the AStarOptimizer flow with the existing deadlineFrom() helper from SolverClock.h, preserving the current optimizeMaxMs behavior and using the helper as the sole deadline calculation.src/pages/shifting-mosaic-solver/a-star/DragSolverAssembly.cpp (1)
18-19: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftSonarCloud gate is failing on
searchAssembly(cognitive complexity 149 vs 25).The frame/candidate selection, joint-endgame relaxation, packing-variant rotation, and the final goal leg are four independent stages already separated by comments; extracting them into private helpers (or a small
AssemblyRunstruct holdingplan/anchors/stack) would bring this under the gate without behavior change. The nesting findings at Lines 216 and 263 fall out of the same extraction.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/pages/shifting-mosaic-solver/a-star/DragSolverAssembly.cpp` around lines 18 - 19, Reduce cognitive complexity in DragSolver::searchAssembly by extracting the four commented stages—frame/candidate selection, joint-endgame relaxation, packing-variant rotation, and final goal-leg processing—into private helpers or an AssemblyRun context carrying plan, anchors, and stack. Preserve the existing stage order, state transitions, and behavior while ensuring searchAssembly and the extracted helpers satisfy the complexity gate.Source: Linters/SAST tools
src/pages/shifting-mosaic-solver/a-star/AStarHeuristics.cpp (1)
233-244: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueComment describes an occupancy test
anchorPassabledoes not do.The comment says passability means "no overlap with a non-goal block at any shape cell", but
anchorPassableonly bounds-checks — the BFS ignores every block, movable or locked. Line 271 hints that was intentional for movable blocks; either way the comment should state that the path is computed on the empty grid, otherwise readers will assume the returned blocker count is path-relative to real obstacles.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/pages/shifting-mosaic-solver/a-star/AStarHeuristics.cpp` around lines 233 - 244, Update the comment above anchorPassable to accurately state that BFS passability checks only grid bounds on an otherwise empty grid and ignores all block occupancy, including movable and locked blocks. Keep anchorPassable’s existing bounds-only behavior unchanged, and clarify that the resulting path or blocker count is not relative to real obstacles.src/pages/shifting-mosaic-solver/a-star/DragSolverHeuristics.cpp (1)
265-272: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe symmetry-group canonicalization block is copy-pasted four times.
Identical
std::array<uint16_t, 64> vals/ sort / write-back appears here, inchildSignature(Lines 301-310), insignatureFromAnchors(Lines 327-334), and again inAStarHeuristics.cpp(Lines 459-466). A small private helper takinguint16_t *dwould keep them > 64skip rule in one place — right now a change to that rule has to be replicated four times.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/pages/shifting-mosaic-solver/a-star/DragSolverHeuristics.cpp` around lines 265 - 272, Extract the repeated symmetry-group canonicalization logic into a small private helper accepting a uint16_t* buffer, including the m > 64 guard, value collection, sorting, and write-back. Replace the duplicated blocks in the current function, childSignature, signatureFromAnchors, and the corresponding AStarHeuristics.cpp code with calls to that helper, preserving existing behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@e2e/shiftingMosaicSolver.editing.test.ts`:
- Around line 90-94: Blur the Grid Height field in the test flow before
asserting the grid-cell count. Update the interaction around the Grid Height
spinbutton to match the existing Grid Width and later field pattern by pressing
Tab after filling it, then keep the 32-cell assertion unchanged.
In `@src/pages/rolling-blocks-solver/a-star/AStarGoals.cpp`:
- Around line 161-183: Remove the broken bitmask deduplication from
AStar::blockCompatibleWithCluster so every orientation in dims is checked
against cluster.width and cluster.depth. Preserve the existing orientation list
and return behavior, allowing redundant comparisons rather than skipping
orientations due to hash collisions.
- Around line 188-267: Replace the fixed-size taken array in assignBlocksToGoals
and the related assignUniqueGoals/assignGreedyGoals signatures with a
dynamically sized container initialized to goalClusters_.size(). Keep indexing
behavior unchanged while ensuring all taken accesses remain within the number of
goal clusters.
In `@src/pages/shifting-mosaic-solver/a-star/BenchFixture.cpp`:
- Around line 17-41: Validate goalIndex in loadFixture immediately after reading
it from JSON, ensuring it is within both data.shapes and data.initialAnchors
bounds, including rejecting empty arrays. Throw a descriptive exception for
invalid fixtures before returning data or allowing replaySolves and runVerify to
access the vectors unchecked.
- Around line 26-36: Replace the required JSON field lookups using operator[]
with checked at() access in the fixture parsing loops for c["x"], c["y"],
a["x"], and a["y"] in src/pages/shifting-mosaic-solver/a-star/BenchFixture.cpp
lines 26-36. Apply the same change to t["blockId"] and t["direction"] in
src/pages/shifting-mosaic-solver/a-star/BenchCommands.cpp lines 28-33; no other
parsing behavior needs to change.
In `@src/pages/shifting-mosaic-solver/a-star/DragSolver.cpp`:
- Around line 67-69: Validate gridWidth_ and goalAnchor_ inside
DragSolver::DragSolver before constructing goalFinalRows_, rejecting wide grids
and off-board goal anchors and setting unsolvableAtStart_. Ensure the
goalFinalRows_ indexing and bit shifts only execute after these validations; do
not rely on search() to perform checks after construction.
In `@src/pages/shifting-mosaic-solver/a-star/DragSolverBeam.cpp`:
- Around line 147-153: Update the layer-processing loop around anchorsOf and
stats_.nodesExpanded so it stops expanding nodes once maxNodes is reached,
checking the limit before each iteration and immediately after incrementing the
counter as needed. Ensure the enclosing round also exits when the cap is hit,
preventing later layers from expanding beyond maxNodes while preserving normal
beam processing below the limit.
In `@src/pages/shifting-mosaic-solver/a-star/MemoryProbe.h`:
- Around line 107-109: Add the Windows-only Psapi.lib link dependency to the
shifting_mosaic_a_star target, alongside its existing Threads::Threads linkage,
so GetProcessMemoryInfo used by MemoryProbe is resolved without affecting
non-Windows builds.
In `@src/pages/shifting-mosaic-solver/a-star/test/json_test.cpp`:
- Around line 180-190: Clamp every environment override to its destination
type’s maximum before narrowing, including the uint8_t assignments for weight,
pathBlockerWeight, boundaryDistanceWeight, axisAwareWeight,
lpDisplacementWeight, and strideOverride, plus the uint16_t override below.
Reuse the existing envU32 values and ensure oversized inputs saturate rather
than wrap.
In `@test/shiftingMosaicSolver/aStar.collision.test.ts`:
- Around line 55-81: Remove the assertion-free placeholder test and its unused
shape declarations/comments from the test suite; retain the existing concrete
bbox-overlap/cells-disjoint coverage in the following test.
---
Outside diff comments:
In `@src/pages/shifting-mosaic-solver/a-star/main.cpp`:
- Around line 128-164: Update the argument parsing branches in main to validate
each nextU() result against the maximum representable value of its destination
type before every static_cast, including uint8_t, uint16_t, uint32_t, and int
destinations. Reject out-of-range values through the existing argument-error
path, while preserving valid assignments and the special maxStates/maxHeapBytes
configuration updates.
---
Nitpick comments:
In `@e2e/phasicDialSolver.solvable.test.ts`:
- Around line 11-30: Replace the Material Web internal DOM-chain selectors in
the matrix-cell interactions throughout the test, including the repeated cases
later in the file, with stable per-cell hooks such as data-testid attributes or
accessible role/name queries. Update the selectors used by the phasicDialSolver
test without changing the entered values or matrix-cell targets.
In `@e2e/rollingBlocksSolver.placement.test.ts`:
- Around line 56-207: The test repeats identical full-board aria snapshots after
each drag instead of checking the relevant invariant. In the rolling blocks
placement test, replace the three redundant snapshots with concise assertions
that the Blocks section remains empty and verify only the affected cell states
needed for each drag; retain one initial full-board snapshot if useful.
- Around line 3-31: Remove the unused setupSolvedPuzzle helper from this test
file. Do not alter the surrounding tests; only consider moving its setup logic
to an existing shared helper if required to eliminate the duplicated setup
across rolling-blocks solver tests.
In `@e2e/rollingBlocksSolver.solution.test.ts`:
- Around line 157-181: Refactor the duplicated puzzle setup in the test by
extracting the shared actions from setupSolvedPuzzle and this inline block into
a buildPuzzle(page) helper. Update setupSolvedPuzzle to reuse buildPuzzle before
calculating moves, and replace the inline setup in the current test with the
helper while preserving the existing solve and assertion steps.
In `@e2e/shiftingMosaicSolver.solving.test.ts`:
- Around line 1-58: Extract the shared SHIFTING_MOSAIC_URL, SAMPLE_CONFIG, Cell,
cellAt, cellCenter, paintShape, and paintWithGap declarations into
e2e/helpers/shiftingMosaic.ts. In
e2e/shiftingMosaicSolver.solving.test.ts#L1-L58, import cellAt, paintShape, and
paintWithGap and remove SAMPLE_CONFIG/readFileSync; in
e2e/shiftingMosaicSolver.blocks.test.ts#L1-L58, import cellAt/paintShape and
remove SAMPLE_CONFIG, paintWithGap, and readFileSync; in
e2e/shiftingMosaicSolver.config.test.ts#L1-L58, import cellAt/SAMPLE_CONFIG and
remove paintShape, paintWithGap, and cellCenter; in
e2e/shiftingMosaicSolver.editing.test.ts#L1-L58, import cellAt/paintShape and
remove SAMPLE_CONFIG, paintWithGap, and readFileSync.
In `@src/pages/shifting-mosaic-solver/a-star/AStarHeuristics.cpp`:
- Around line 233-244: Update the comment above anchorPassable to accurately
state that BFS passability checks only grid bounds on an otherwise empty grid
and ignores all block occupancy, including movable and locked blocks. Keep
anchorPassable’s existing bounds-only behavior unchanged, and clarify that the
resulting path or blocker count is not relative to real obstacles.
In `@src/pages/shifting-mosaic-solver/a-star/AStarOptimizer.cpp`:
- Around line 238-239: Replace the inline deadline ternary in the AStarOptimizer
flow with the existing deadlineFrom() helper from SolverClock.h, preserving the
current optimizeMaxMs behavior and using the helper as the sole deadline
calculation.
In `@src/pages/shifting-mosaic-solver/a-star/DragSolverAssembly.cpp`:
- Around line 18-19: Reduce cognitive complexity in DragSolver::searchAssembly
by extracting the four commented stages—frame/candidate selection, joint-endgame
relaxation, packing-variant rotation, and final goal-leg processing—into private
helpers or an AssemblyRun context carrying plan, anchors, and stack. Preserve
the existing stage order, state transitions, and behavior while ensuring
searchAssembly and the extracted helpers satisfy the complexity gate.
In `@src/pages/shifting-mosaic-solver/a-star/DragSolverHeuristics.cpp`:
- Around line 265-272: Extract the repeated symmetry-group canonicalization
logic into a small private helper accepting a uint16_t* buffer, including the m
> 64 guard, value collection, sorting, and write-back. Replace the duplicated
blocks in the current function, childSignature, signatureFromAnchors, and the
corresponding AStarHeuristics.cpp code with calls to that helper, preserving
existing behavior.
In `@src/pages/shifting-mosaic-solver/a-star/test/DragSolverTestOracle.h`:
- Around line 204-206: Remove the unused alone vector initialization and
assignment near the goal-anchor check in the test oracle. Keep the existing bbox
validation and surrounding target setup unchanged.
In `@test/shiftingMosaicSolver/aStar.collision.test.ts`:
- Around line 12-51: Extract the shared makeAStar and applyTurns helpers into
test/shiftingMosaicSolver/aStarTestHelpers.ts, then import and reuse them from
aStar.collision.test.ts, aStar.primitives.test.ts, and aStar.search.test.ts.
Remove the unused applyTurns and Node references from aStar.collision.test.ts
while preserving all existing test behavior.
In `@test/shiftingMosaicSolver/aStar.search.test.ts`:
- Line 216: Update the assertion in the aStar search test to use the turns
collection’s toHaveLength matcher instead of comparing turns.length with
toBe(1), preserving the expected length of one.
In `@test/shiftingMosaicSolver/board.state.test.ts`:
- Around line 6-31: Extract the duplicated Board test setup into a shared
makeBoardFixture helper, including the editorMock shape, DOM initialization,
Board construction, and internal alias wiring. Replace the inline fixture in
test/shiftingMosaicSolver/board.state.test.ts lines 6-31, board.geometry.test.ts
lines 11-36 while preserving its local internal alias, and board.render.test.ts
lines 6-31. In test/shiftingMosaicSolver/board.pointer.test.ts lines 6-31, use
the shared fixture and move its repeated dispatch helper into the shared test
module.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 0d413de1-4e63-487b-9adf-3d9855553490
📒 Files selected for processing (70)
e2e/phasicDialSolver.solvable.test.tse2e/phasicDialSolver.states.test.tse2e/rollingBlocksSolver.placement.test.tse2e/rollingBlocksSolver.solution.test.tse2e/rollingBlocksSolver.tools.test.tse2e/shiftingMosaicSolver.blocks.test.tse2e/shiftingMosaicSolver.config.test.tse2e/shiftingMosaicSolver.editing.test.tse2e/shiftingMosaicSolver.solving.test.tse2e/shiftingMosaicSolver.test.tssrc/pages/rolling-blocks-solver/a-star/AStar.cppsrc/pages/rolling-blocks-solver/a-star/AStarGoals.cppsrc/pages/rolling-blocks-solver/a-star/CMakeLists.txtsrc/pages/rolling-blocks-solver/a-star/test/CMakeLists.txtsrc/pages/rolling-blocks-solver/aStar.tssrc/pages/rolling-blocks-solver/goalClusters.tssrc/pages/shifting-mosaic-solver/a-star/.clang-tidysrc/pages/shifting-mosaic-solver/a-star/AStar.cppsrc/pages/shifting-mosaic-solver/a-star/AStar.hsrc/pages/shifting-mosaic-solver/a-star/AStarHeuristics.cppsrc/pages/shifting-mosaic-solver/a-star/AStarOptimizer.cppsrc/pages/shifting-mosaic-solver/a-star/AStarSearch.cppsrc/pages/shifting-mosaic-solver/a-star/BenchCommands.cppsrc/pages/shifting-mosaic-solver/a-star/BenchCommands.hsrc/pages/shifting-mosaic-solver/a-star/BenchFixture.cppsrc/pages/shifting-mosaic-solver/a-star/BenchFixture.hsrc/pages/shifting-mosaic-solver/a-star/BitGrid.hsrc/pages/shifting-mosaic-solver/a-star/CMakeLists.txtsrc/pages/shifting-mosaic-solver/a-star/ClangdCompat.hsrc/pages/shifting-mosaic-solver/a-star/DragSolver.cppsrc/pages/shifting-mosaic-solver/a-star/DragSolver.hsrc/pages/shifting-mosaic-solver/a-star/DragSolverArms.cppsrc/pages/shifting-mosaic-solver/a-star/DragSolverAssembly.cppsrc/pages/shifting-mosaic-solver/a-star/DragSolverBeam.cppsrc/pages/shifting-mosaic-solver/a-star/DragSolverConfig.hsrc/pages/shifting-mosaic-solver/a-star/DragSolverExpand.cppsrc/pages/shifting-mosaic-solver/a-star/DragSolverHeuristics.cppsrc/pages/shifting-mosaic-solver/a-star/DragSolverSearch.cppsrc/pages/shifting-mosaic-solver/a-star/MemoryProbe.hsrc/pages/shifting-mosaic-solver/a-star/NodeKey.hsrc/pages/shifting-mosaic-solver/a-star/ParallelCascade.hsrc/pages/shifting-mosaic-solver/a-star/SolverClock.cppsrc/pages/shifting-mosaic-solver/a-star/SolverClock.hsrc/pages/shifting-mosaic-solver/a-star/StateTable.hsrc/pages/shifting-mosaic-solver/a-star/main.cppsrc/pages/shifting-mosaic-solver/a-star/test/CMakeLists.txtsrc/pages/shifting-mosaic-solver/a-star/test/DragSolverTestOracle.hsrc/pages/shifting-mosaic-solver/a-star/test/astar_unit_test.cppsrc/pages/shifting-mosaic-solver/a-star/test/dragsolver_search_test.cppsrc/pages/shifting-mosaic-solver/a-star/test/dragsolver_unit_test.cppsrc/pages/shifting-mosaic-solver/a-star/test/json_test.cppsrc/pages/shifting-mosaic-solver/a-star/test/optimizer_test.cppsrc/pages/shifting-mosaic-solver/aStar.tssrc/pages/shifting-mosaic-solver/board.tssrc/pages/shifting-mosaic-solver/boardGeometry.tssrc/pages/shifting-mosaic-solver/editorHelpers.tssrc/pages/shifting-mosaic-solver/shiftingMosaicSolver.tssrc/util/buildWasm.tssrc/util/minHeap.tstest/rollingBlocksSolver/block.musttouch.test.tstest/rollingBlocksSolver/block.roll.test.tstest/rollingBlocksSolver/block.validity.test.tstest/shiftingMosaicSolver/aStar.collision.test.tstest/shiftingMosaicSolver/aStar.primitives.test.tstest/shiftingMosaicSolver/aStar.search.test.tstest/shiftingMosaicSolver/board.geometry.test.tstest/shiftingMosaicSolver/board.pointer.test.tstest/shiftingMosaicSolver/board.render.test.tstest/shiftingMosaicSolver/board.state.test.tstest/shiftingMosaicSolver/board.test.ts
💤 Files with no reviewable changes (1)
- e2e/shiftingMosaicSolver.test.ts
🚧 Files skipped from review as they are similar to previous changes (10)
- src/pages/shifting-mosaic-solver/a-star/test/CMakeLists.txt
- src/pages/shifting-mosaic-solver/a-star/CMakeLists.txt
- src/pages/shifting-mosaic-solver/aStar.ts
- src/pages/shifting-mosaic-solver/board.ts
- src/pages/shifting-mosaic-solver/a-star/AStar.h
- src/pages/shifting-mosaic-solver/a-star/NodeKey.h
- src/pages/shifting-mosaic-solver/a-star/StateTable.h
- src/pages/shifting-mosaic-solver/shiftingMosaicSolver.ts
- src/util/buildWasm.ts
- src/pages/shifting-mosaic-solver/a-star/BitGrid.h
| await page.getByRole("spinbutton", { name: "Grid Width" }).click(); | ||
| await page.getByRole("spinbutton", { name: "Grid Width" }).fill("8"); | ||
| await page.getByRole("spinbutton", { name: "Grid Width" }).press("Tab"); | ||
| await page.getByRole("spinbutton", { name: "Grid Height" }).fill("4"); | ||
| await expect(page.locator(".grid-cell")).toHaveCount(32); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Blur the Grid Height field before asserting the new cell count.
Line 93 fills Grid Height without a press("Tab"), unlike Grid Width above and the pattern used at lines 100-103. If the resize commits on blur/change rather than on input, this assertion becomes order-dependent and flaky.
🔧 Proposed change
await page.getByRole("spinbutton", { name: "Grid Height" }).fill("4");
+ await page.getByRole("spinbutton", { name: "Grid Height" }).press("Tab");
await expect(page.locator(".grid-cell")).toHaveCount(32);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| await page.getByRole("spinbutton", { name: "Grid Width" }).click(); | |
| await page.getByRole("spinbutton", { name: "Grid Width" }).fill("8"); | |
| await page.getByRole("spinbutton", { name: "Grid Width" }).press("Tab"); | |
| await page.getByRole("spinbutton", { name: "Grid Height" }).fill("4"); | |
| await expect(page.locator(".grid-cell")).toHaveCount(32); | |
| await page.getByRole("spinbutton", { name: "Grid Width" }).click(); | |
| await page.getByRole("spinbutton", { name: "Grid Width" }).fill("8"); | |
| await page.getByRole("spinbutton", { name: "Grid Width" }).press("Tab"); | |
| await page.getByRole("spinbutton", { name: "Grid Height" }).fill("4"); | |
| await page.getByRole("spinbutton", { name: "Grid Height" }).press("Tab"); | |
| await expect(page.locator(".grid-cell")).toHaveCount(32); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@e2e/shiftingMosaicSolver.editing.test.ts` around lines 90 - 94, Blur the Grid
Height field in the test flow before asserting the grid-cell count. Update the
interaction around the Grid Height spinbutton to match the existing Grid Width
and later field pattern by pressing Tab after filling it, then keep the 32-cell
assertion unchanged.
| bool AStar::blockCompatibleWithCluster(const Block &block, | ||
| const GoalCluster &cluster) { | ||
| const std::array<std::array<uint8_t, 2>, 6> dims = { | ||
| {{block.width, block.depth}, | ||
| {block.height, block.depth}, | ||
| {block.width, block.height}, | ||
| {block.depth, block.width}, | ||
| {block.depth, block.height}, | ||
| {block.height, block.width}}}; | ||
| uint32_t seen = 0; | ||
| for (const auto &[w, d] : dims) { | ||
| const auto k = static_cast<uint32_t>(static_cast<uint16_t>(w) << 8 | | ||
| static_cast<uint16_t>(d)); | ||
| if (seen & 1u << (k & 31u)) { | ||
| continue; | ||
| } | ||
| seen |= 1u << (k & 31u); | ||
| if (w == cluster.width && d == cluster.depth) { | ||
| return true; | ||
| } | ||
| } | ||
| return false; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Dedup hash collapses to d&31, permanently skipping 3 of 6 block orientations.
k = (w<<8)|d then k & 31 only ever reflects d's low 5 bits — w<<8 never lands inside the low-5-bit mask, so seen is effectively keyed on d alone. Since the dims list pairs {w,d}/{h,d}, {w,h}/{d,h}, {d,w}/{h,w} share the same d per pair, the second entry of each pair is always treated as "already seen" and its w == cluster.width check never runs — even when its w differs.
Concrete repro: block = {width:2, height:3, depth:4}, cluster = {width:3, depth:4}. The (height, depth) = (3,4) orientation matches the cluster, but its hash collides with (width, depth) = (2,4) (both have d=4), so it's skipped and the function wrongly returns false.
For comparison, the TS port (src/pages/rolling-blocks-solver/goalClusters.ts blockPossibleFootprints) dedups via a proper ${w}x${d} string key and doesn't have this problem — confirming this is a defect introduced in the C++ port, not an intentional simplification.
Since this only feeds the goal-distance heuristic (never move validity), it stays admissible but silently weakens goal-cluster assignment/heuristic guidance.
🐛 Proposed fix — drop the broken dedup (redundant comparisons are harmless)
bool AStar::blockCompatibleWithCluster(const Block &block,
const GoalCluster &cluster) {
const std::array<std::array<uint8_t, 2>, 6> dims = {
{{block.width, block.depth},
{block.height, block.depth},
{block.width, block.height},
{block.depth, block.width},
{block.depth, block.height},
{block.height, block.width}}};
- uint32_t seen = 0;
for (const auto &[w, d] : dims) {
- const auto k = static_cast<uint32_t>(static_cast<uint16_t>(w) << 8 |
- static_cast<uint16_t>(d));
- if (seen & 1u << (k & 31u)) {
- continue;
- }
- seen |= 1u << (k & 31u);
if (w == cluster.width && d == cluster.depth) {
return true;
}
}
return false;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| bool AStar::blockCompatibleWithCluster(const Block &block, | |
| const GoalCluster &cluster) { | |
| const std::array<std::array<uint8_t, 2>, 6> dims = { | |
| {{block.width, block.depth}, | |
| {block.height, block.depth}, | |
| {block.width, block.height}, | |
| {block.depth, block.width}, | |
| {block.depth, block.height}, | |
| {block.height, block.width}}}; | |
| uint32_t seen = 0; | |
| for (const auto &[w, d] : dims) { | |
| const auto k = static_cast<uint32_t>(static_cast<uint16_t>(w) << 8 | | |
| static_cast<uint16_t>(d)); | |
| if (seen & 1u << (k & 31u)) { | |
| continue; | |
| } | |
| seen |= 1u << (k & 31u); | |
| if (w == cluster.width && d == cluster.depth) { | |
| return true; | |
| } | |
| } | |
| return false; | |
| } | |
| bool AStar::blockCompatibleWithCluster(const Block &block, | |
| const GoalCluster &cluster) { | |
| const std::array<std::array<uint8_t, 2>, 6> dims = { | |
| {{block.width, block.depth}, | |
| {block.height, block.depth}, | |
| {block.width, block.height}, | |
| {block.depth, block.width}, | |
| {block.depth, block.height}, | |
| {block.height, block.width}}}; | |
| for (const auto &[w, d] : dims) { | |
| if (w == cluster.width && d == cluster.depth) { | |
| return true; | |
| } | |
| } | |
| return false; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/pages/rolling-blocks-solver/a-star/AStarGoals.cpp` around lines 161 -
183, Remove the broken bitmask deduplication from
AStar::blockCompatibleWithCluster so every orientation in dims is checked
against cluster.width and cluster.depth. Preserve the existing orientation list
and return behavior, allowing redundant comparisons rather than skipping
orientations due to hash collisions.
| int AStar::countCompatibleClusters(const Block &block, | ||
| const std::array<bool, 64> &taken, | ||
| size_t &outIdx) const { | ||
| int count = 0; | ||
| for (size_t i = 0; i < goalClusters_.size(); i++) { | ||
| if (!taken[i] && blockCompatibleWithCluster(block, goalClusters_[i])) { | ||
| outIdx = i; | ||
| if (++count > 1) { | ||
| break; | ||
| } | ||
| } | ||
| } | ||
| return count; | ||
| } | ||
|
|
||
| // --------------------------------------------------------------------------- | ||
| // Helper: assign blocks that have exactly one compatible cluster | ||
| // --------------------------------------------------------------------------- | ||
| void AStar::assignUniqueGoals(const std::vector<Block> &blocks, | ||
| std::array<bool, 64> &taken) { | ||
| bool changed = true; | ||
| while (changed) { | ||
| changed = false; | ||
| for (const auto &block : blocks) { | ||
| if (blockGoalAssignment_[block.id].valid) { | ||
| continue; | ||
| } | ||
| size_t compatIdx = std::numeric_limits<size_t>::max(); | ||
| if (const int compatCount = | ||
| countCompatibleClusters(block, taken, compatIdx); | ||
| compatCount == 1) { | ||
| blockGoalAssignment_[block.id] = {goalClusters_[compatIdx], true}; | ||
| taken[compatIdx] = true; | ||
| changed = true; | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // --------------------------------------------------------------------------- | ||
| // Helper: greedily assign remaining blocks to nearest compatible cluster | ||
| // --------------------------------------------------------------------------- | ||
| void AStar::assignGreedyGoals(const std::vector<Block> &blocks, | ||
| std::array<bool, 64> &taken) { | ||
| for (const auto &block : blocks) { | ||
| if (blockGoalAssignment_[block.id].valid) { | ||
| continue; | ||
| } | ||
| int bestDist = std::numeric_limits<int>::max(); | ||
| size_t bestIdx = std::numeric_limits<size_t>::max(); | ||
| for (size_t i = 0; i < goalClusters_.size(); i++) { | ||
| if (taken[i] || !blockCompatibleWithCluster(block, goalClusters_[i])) { | ||
| continue; | ||
| } | ||
| const int d = | ||
| std::abs(static_cast<int>(block.x) - goalClusters_[i].minX) + | ||
| std::abs(static_cast<int>(block.y) - goalClusters_[i].minY); | ||
| if (d < bestDist) { | ||
| bestDist = d; | ||
| bestIdx = i; | ||
| } | ||
| } | ||
| if (bestIdx != std::numeric_limits<size_t>::max()) { | ||
| blockGoalAssignment_[block.id] = {goalClusters_[bestIdx], true}; | ||
| taken[bestIdx] = true; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| void AStar::assignBlocksToGoals(const std::vector<Block> &blocks) { | ||
| for (auto &[cluster, valid] : blockGoalAssignment_) { | ||
| valid = false; | ||
| } | ||
| if (goalClusters_.empty()) { | ||
| return; | ||
| } | ||
| std::array<bool, 64> taken{}; | ||
| assignUniqueGoals(blocks, taken); | ||
| assignGreedyGoals(blocks, taken); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Look for any invariant capping grid size / goal-cluster count.
fd -e h -p 'rolling-blocks-solver/a-star' | xargs rg -n 'gridWidth_|gridHeight_|MAX_.*GOAL|goalClusters_' -C2Repository: Valle12/islands-of-insight-tools
Length of output: 791
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the relevant files, then inspect the goal-cluster generation and any board-size limits.
ast-grep outline src/pages/rolling-blocks-solver/a-star/AStar.h --view expanded || true
ast-grep outline src/pages/rolling-blocks-solver/a-star/AStar.cpp --view expanded || true
echo '--- AStar.h ---'
cat -n src/pages/rolling-blocks-solver/a-star/AStar.h | sed -n '1,220p'
echo '--- AStar.cpp (goal cluster related slices) ---'
rg -n "precomputeGoalClusters|goalClusters_|assignBlocksToGoals|countCompatibleClusters|assignUniqueGoals|assignGreedyGoals|gridWidth_|gridHeight_" src/pages/rolling-blocks-solver/a-star/AStar.cpp -n -C 4Repository: Valle12/islands-of-insight-tools
Length of output: 12291
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the definition of GoalCluster and the preprocessing that creates clusters.
fd -p 'src/pages/rolling-blocks-solver' -e h -e cpp | rg 'GoalCluster|AStar|Goal'
echo '--- GoalCluster.h ---'
cat -n src/pages/rolling-blocks-solver/GoalCluster.h | sed -n '1,220p'
echo '--- AStar.cpp relevant lines ---'
sed -n '1,260p' src/pages/rolling-blocks-solver/a-star/AStar.cppRepository: Valle12/islands-of-insight-tools
Length of output: 473
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find any explicit limits on board dimensions or number of goal clusters anywhere in the solver.
rg -n "64|gridWidth_|gridHeight_|goal cluster|GoalCluster|connected Goal|connected component|precomputeGoalClusters" src/pages/rolling-blocks-solver -C 2Repository: Valle12/islands-of-insight-tools
Length of output: 22627
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the full AStar implementation around cluster generation and assignment.
sed -n '1,360p' src/pages/rolling-blocks-solver/a-star/AStar.cppRepository: Valle12/islands-of-insight-tools
Length of output: 9854
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read the cluster type and any size-related helpers.
if [ -f src/pages/rolling-blocks-solver/GoalCluster.h ]; then
cat -n src/pages/rolling-blocks-solver/GoalCluster.h | sed -n '1,240p'
else
fd -a -p 'src/pages/rolling-blocks-solver' -e h | rg 'GoalCluster'
fiRepository: Valle12/islands-of-insight-tools
Length of output: 242
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Search for any explicit cap on board dimensions or solver state size.
rg -n "8x8|64 cells|64|gridWidth_|gridHeight_|connected component|GoalCluster|precomputeGoalClusters" src/pages/rolling-blocks-solver -C 3Repository: Valle12/islands-of-insight-tools
Length of output: 28473
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Look for board-size validation / puzzle generation limits that could bound goal clusters.
rg -n --hidden --glob '!**/node_modules/**' \
"gridWidth|gridHeight|width.*height|8x8|9x9|10x10|max.*width|max.*height|validate.*size|size limit|goalClusters|precomputeGoalClusters|AStar\(" \
src/pages/rolling-blocks-solver -C 2
echo '--- files mentioning AStar construction ---'
rg -n --hidden --glob '!**/node_modules/**' "new AStar\(|AStar\(" src/pages/rolling-blocks-solver -C 2Repository: Valle12/islands-of-insight-tools
Length of output: 50388
taken needs to be sized from goalClusters_, not hard-coded to 64. goalClusters_ is built from all connected goal components, and the solver accepts user-chosen grid sizes here, so a board with more than 64 goal regions will make taken[i] go out of bounds. Switch this to a dynamically sized container keyed to goalClusters_.size().
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/pages/rolling-blocks-solver/a-star/AStarGoals.cpp` around lines 188 -
267, Replace the fixed-size taken array in assignBlocksToGoals and the related
assignUniqueGoals/assignGreedyGoals signatures with a dynamically sized
container initialized to goalClusters_.size(). Keep indexing behavior unchanged
while ensuring all taken accesses remain within the number of goal clusters.
| Fixture loadFixture(const std::string &path) { | ||
| std::ifstream f(path); | ||
| if (!f.is_open()) | ||
| throw std::runtime_error("Cannot open " + path); | ||
| json j = json::parse(f); | ||
|
|
||
| Fixture data; | ||
| data.gridWidth = j["gridWidth"].get<uint8_t>(); | ||
| data.gridHeight = j["gridHeight"].get<uint8_t>(); | ||
| for (const auto &shape : j["shapes"]) { | ||
| std::vector<Position> cells; | ||
| cells.reserve(shape.size()); | ||
| for (const auto &c : shape) | ||
| cells.push_back({static_cast<int8_t>(c["x"].get<int>()), | ||
| static_cast<int8_t>(c["y"].get<int>())}); | ||
| data.shapes.push_back(std::move(cells)); | ||
| } | ||
| for (const auto &a : j["initialAnchors"]) | ||
| data.initialAnchors.push_back({static_cast<int8_t>(a["x"].get<int>()), | ||
| static_cast<int8_t>(a["y"].get<int>())}); | ||
| data.goalIndex = j["goalIndex"].get<uint8_t>(); | ||
| data.goalAnchor = {static_cast<int8_t>(j["goalAnchor"]["x"].get<int>()), | ||
| static_cast<int8_t>(j["goalAnchor"]["y"].get<int>())}; | ||
| return data; | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Missing goalIndex bounds validation on fixture load.
data.goalIndex is read from JSON with no check against data.shapes.size()/data.initialAnchors.size(). It is later used unchecked via anchors[data.goalIndex] in replaySolves (Line 83, this file) and in runVerify (BenchCommands.cpp, Line 38). A fixture with an out-of-range goalIndex (e.g. from a hand-edited or fuzzer-mutated file, or an empty shapes array) causes an out-of-bounds std::vector::operator[] read — undefined behavior — in what is meant to be the safety net that catches malformed inputs.
🛡️ Proposed fix
data.goalIndex = j["goalIndex"].get<uint8_t>();
data.goalAnchor = {static_cast<int8_t>(j["goalAnchor"]["x"].get<int>()),
static_cast<int8_t>(j["goalAnchor"]["y"].get<int>())};
+ if (data.goalIndex >= data.shapes.size())
+ throw std::runtime_error("loadFixture: goalIndex out of range in " + path);
return data;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Fixture loadFixture(const std::string &path) { | |
| std::ifstream f(path); | |
| if (!f.is_open()) | |
| throw std::runtime_error("Cannot open " + path); | |
| json j = json::parse(f); | |
| Fixture data; | |
| data.gridWidth = j["gridWidth"].get<uint8_t>(); | |
| data.gridHeight = j["gridHeight"].get<uint8_t>(); | |
| for (const auto &shape : j["shapes"]) { | |
| std::vector<Position> cells; | |
| cells.reserve(shape.size()); | |
| for (const auto &c : shape) | |
| cells.push_back({static_cast<int8_t>(c["x"].get<int>()), | |
| static_cast<int8_t>(c["y"].get<int>())}); | |
| data.shapes.push_back(std::move(cells)); | |
| } | |
| for (const auto &a : j["initialAnchors"]) | |
| data.initialAnchors.push_back({static_cast<int8_t>(a["x"].get<int>()), | |
| static_cast<int8_t>(a["y"].get<int>())}); | |
| data.goalIndex = j["goalIndex"].get<uint8_t>(); | |
| data.goalAnchor = {static_cast<int8_t>(j["goalAnchor"]["x"].get<int>()), | |
| static_cast<int8_t>(j["goalAnchor"]["y"].get<int>())}; | |
| return data; | |
| } | |
| Fixture loadFixture(const std::string &path) { | |
| std::ifstream f(path); | |
| if (!f.is_open()) | |
| throw std::runtime_error("Cannot open " + path); | |
| json j = json::parse(f); | |
| Fixture data; | |
| data.gridWidth = j["gridWidth"].get<uint8_t>(); | |
| data.gridHeight = j["gridHeight"].get<uint8_t>(); | |
| for (const auto &shape : j["shapes"]) { | |
| std::vector<Position> cells; | |
| cells.reserve(shape.size()); | |
| for (const auto &c : shape) | |
| cells.push_back({static_cast<int8_t>(c["x"].get<int>()), | |
| static_cast<int8_t>(c["y"].get<int>())}); | |
| data.shapes.push_back(std::move(cells)); | |
| } | |
| for (const auto &a : j["initialAnchors"]) | |
| data.initialAnchors.push_back({static_cast<int8_t>(a["x"].get<int>()), | |
| static_cast<int8_t>(a["y"].get<int>())}); | |
| data.goalIndex = j["goalIndex"].get<uint8_t>(); | |
| data.goalAnchor = {static_cast<int8_t>(j["goalAnchor"]["x"].get<int>()), | |
| static_cast<int8_t>(j["goalAnchor"]["y"].get<int>())}; | |
| if (data.goalIndex >= data.shapes.size()) | |
| throw std::runtime_error("loadFixture: goalIndex out of range in " + path); | |
| return data; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/pages/shifting-mosaic-solver/a-star/BenchFixture.cpp` around lines 17 -
41, Validate goalIndex in loadFixture immediately after reading it from JSON,
ensuring it is within both data.shapes and data.initialAnchors bounds, including
rejecting empty arrays. Throw a descriptive exception for invalid fixtures
before returning data or allowing replaySolves and runVerify to access the
vectors unchecked.
| for (const auto &shape : j["shapes"]) { | ||
| std::vector<Position> cells; | ||
| cells.reserve(shape.size()); | ||
| for (const auto &c : shape) | ||
| cells.push_back({static_cast<int8_t>(c["x"].get<int>()), | ||
| static_cast<int8_t>(c["y"].get<int>())}); | ||
| data.shapes.push_back(std::move(cells)); | ||
| } | ||
| for (const auto &a : j["initialAnchors"]) | ||
| data.initialAnchors.push_back({static_cast<int8_t>(a["x"].get<int>()), | ||
| static_cast<int8_t>(a["y"].get<int>())}); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== relevant files =="
git ls-files 'src/pages/shifting-mosaic-solver/a-star/*'
echo
echo "== BenchFixture.cpp outline =="
ast-grep outline src/pages/shifting-mosaic-solver/a-star/BenchFixture.cpp --view expanded || true
echo
echo "== BenchCommands.cpp outline =="
ast-grep outline src/pages/shifting-mosaic-solver/a-star/BenchCommands.cpp --view expanded || true
echo
echo "== main/loadFixture usage =="
rg -n "loadFixture\(" src/pages/shifting-mosaic-solver/a-star -S || true
echo
echo "== nlohmann/json access patterns =="
rg -n '\["(x|y|blockId|direction)"\]|\.at\("(\w+)"\)' src/pages/shifting-mosaic-solver/a-star -S || true
echo
echo "== vendored or referenced nlohmann/json docs/files =="
git ls-files | rg 'json|nlohmann|third_party' || trueRepository: Valle12/islands-of-insight-tools
Length of output: 12324
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
import json, urllib.request
url='https://raw.githubusercontent.com/nlohmann/json/v3.11.3/README.md'
print('fetching', url)
try:
with urllib.request.urlopen(url, timeout=20) as r:
txt = r.read().decode('utf-8', 'replace')
for needle in [
'operator[]',
'const object',
'undefined behavior',
'at()',
]:
idx = txt.lower().find(needle.lower())
print('\nNEEDLE', needle, 'IDX', idx)
if idx != -1:
start=max(0, idx-300); end=min(len(txt), idx+900)
print(txt[start:end])
except Exception as e:
print('ERR', e)
PYRepository: Valle12/islands-of-insight-tools
Length of output: 384
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== BenchFixture.cpp == "
cat -n src/pages/shifting-mosaic-solver/a-star/BenchFixture.cpp | sed -n '1,120p'
echo
echo "== BenchCommands.cpp =="
cat -n src/pages/shifting-mosaic-solver/a-star/BenchCommands.cpp | sed -n '1,120p'
echo
echo "== main.cpp relevant section =="
cat -n src/pages/shifting-mosaic-solver/a-star/main.cpp | sed -n '160,210p'
echo
echo "== json headers / version clues =="
rg -n 'nlohmann|json\.hpp|NLOHMANN_JSON' src/pages/shifting-mosaic-solver/a-star package.json test -S || true
echo
echo "== local test JSON usage patterns =="
cat -n src/pages/shifting-mosaic-solver/a-star/test/json_test.cpp | sed -n '1,120p'Repository: Valle12/islands-of-insight-tools
Length of output: 18705
🌐 Web query:
nlohmann/json const operator[] missing key undefined behavior documentation at() throws
💡 Result:
In the nlohmann/json library, the operator[] and at member functions behave differently when accessing keys in a JSON object [1][2]. Operator[] (Unchecked Access) For non-const JSON objects, operator[] will insert a new element with a null value if the specified key is missing [3][4][5]. However, for const JSON objects, accessing a missing key is undefined behavior [4][6][7]. The library guards against this in debug mode by triggering a runtime assertion (often resulting in an program abort) [4][6][8]. If you disable these assertions (e.g., by defining the NDEBUG preprocessor macro), the access remains undefined behavior, which can lead to unpredictable results or crashes [4][6][7][8]. at (Checked Access) The at member function provides checked access for both const and non-const JSON objects [1][9]. If the specified key does not exist, at throws a basic_json::out_of_range exception [1][9]. Recommendation The library documentation explicitly recommends using at instead of operator[] when working with const JSON objects or when you cannot guarantee that a key exists [6][2]. Alternatively, you can verify the existence of a key using the contains member function before using operator[] [6][8].
Citations:
- 1: https://nlohmann.github.io/json/features/element_access/checked_access/
- 2: https://github.com/nlohmann/json/blob/master/README.md
- 3: https://json.nlohmann.me/api/basic_json/operator%5B%5D/
- 4: https://json.nlohmann.me/features/element_access/unchecked_access/
- 5: https://nlohmann.github.io/json/api/basic_json/operator%5B%5D/
- 6: https://json.nlohmann.me/features/assertions/
- 7: https://nlohmann.github.io/json/features/element_access/unchecked_access/
- 8: https://nlohmann.github.io/json/features/assertions/
- 9: https://json.nlohmann.me/api/basic_json/at/
Switch required JSON lookups to .at()
Missing keys on const JSON elements make operator[] undefined behavior here; use checked access for required fields.
src/pages/shifting-mosaic-solver/a-star/BenchFixture.cpp#L26-L36:c["x"]/c["y"]anda["x"]/a["y"]src/pages/shifting-mosaic-solver/a-star/BenchCommands.cpp#L28-L33:t["blockId"]/t["direction"]
📍 Affects 2 files
src/pages/shifting-mosaic-solver/a-star/BenchFixture.cpp#L26-L36(this comment)src/pages/shifting-mosaic-solver/a-star/BenchCommands.cpp#L28-L33
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/pages/shifting-mosaic-solver/a-star/BenchFixture.cpp` around lines 26 -
36, Replace the required JSON field lookups using operator[] with checked at()
access in the fixture parsing loops for c["x"], c["y"], a["x"], and a["y"] in
src/pages/shifting-mosaic-solver/a-star/BenchFixture.cpp lines 26-36. Apply the
same change to t["blockId"] and t["direction"] in
src/pages/shifting-mosaic-solver/a-star/BenchCommands.cpp lines 28-33; no other
parsing behavior needs to change.
| goalFinalRows_.assign(gridHeight_, 0); | ||
| for (const auto &[cx, cy] : shapes_[goalIndex_]) | ||
| goalFinalRows_[goalAnchor_.y + cy] |= uint64_t{1} << (goalAnchor_.x + cx); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -nP -C4 'DragSolver\s*\(' --type cpp --type cc -g '!**/test/**' src | head -80
rg -nP -C3 'gridWidth|goalAnchor' src/pages/shifting-mosaic-solver/a-star/wasm_bindings.cppRepository: Valle12/islands-of-insight-tools
Length of output: 4899
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the relevant files first, then inspect the constructor and search guard.
ast-grep outline src/pages/shifting-mosaic-solver/a-star/DragSolver.cpp --view expanded || true
ast-grep outline src/pages/shifting-mosaic-solver/a-star/DragSolverArms.cpp --view expanded || true
ast-grep outline src/pages/shifting-mosaic-solver/a-star/wasm_bindings.cpp --view expanded || true
echo '--- DragSolver.cpp (constructor area) ---'
sed -n '1,140p' src/pages/shifting-mosaic-solver/a-star/DragSolver.cpp
echo '--- DragSolverArms.cpp (search guard area) ---'
sed -n '1,80p' src/pages/shifting-mosaic-solver/a-star/DragSolverArms.cpp
echo '--- wasm_bindings.cpp (solver construction paths) ---'
sed -n '80,230p' src/pages/shifting-mosaic-solver/a-star/wasm_bindings.cppRepository: Valle12/islands-of-insight-tools
Length of output: 17573
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- DragSolver constructor definition ---'
sed -n '1,110p' src/pages/shifting-mosaic-solver/a-star/DragSolver.cpp
echo '--- All DragSolver construction sites ---'
rg -n --no-heading 'DragSolver\s+(solver|aStar)\s*\(' src/pages/shifting-mosaic-solver/a-star src | sed -n '1,120p'
echo '--- Any constructor-time validation for gridWidth / goalAnchor ---'
rg -n --no-heading 'gridWidth_ > 64|goalAnchor|unsolvableAtStart_|goalFinalRows_' src/pages/shifting-mosaic-solver/a-star/DragSolver.cpp src/pages/shifting-mosaic-solver/a-star/*.cppRepository: Valle12/islands-of-insight-tools
Length of output: 23092
Validate DragSolver inputs before building goalFinalRows_
DragSolver::DragSolver constructs goalFinalRows_ before any gridWidth_/goalAnchor_ validation, so a wide grid or off-board goal anchor can still hit UB/OOB during construction; the search() guard comes too late. Move those checks into the constructor or reject them at the call sites and set unsolvableAtStart_.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/pages/shifting-mosaic-solver/a-star/DragSolver.cpp` around lines 67 - 69,
Validate gridWidth_ and goalAnchor_ inside DragSolver::DragSolver before
constructing goalFinalRows_, rejecting wide grids and off-board goal anchors and
setting unsolvableAtStart_. Ensure the goalFinalRows_ indexing and bit shifts
only execute after these validations; do not rely on search() to perform checks
after construction.
| for (const int32_t ai : layer) { | ||
| // Copy: arena may reallocate while children are appended later. | ||
| const NodeKey parentKey = arena[ai].key; | ||
| anchorsOf(parentKey, anchors); | ||
| stats_.nodesExpanded++; | ||
| if (onProgress && stats_.nodesExpanded % 4096 == 0) | ||
| onProgress(stats_.nodesExpanded); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Enforce maxNodes inside each beam layer.
The limit is checked only before a round (Lines 91-92). A later layer can expand up to W additional states after the cap is reached, so small limits are materially violated. Stop the layer/round as soon as stats_.nodesExpanded reaches maxNodes.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/pages/shifting-mosaic-solver/a-star/DragSolverBeam.cpp` around lines 147
- 153, Update the layer-processing loop around anchorsOf and
stats_.nodesExpanded so it stops expanding nodes once maxNodes is reached,
checking the limit before each iteration and immediately after incrementing the
counter as needed. Ensure the enclosing round also exits when the cap is hit,
preventing later layers from expanding beyond maxNodes while preserving normal
beam processing below the limit.
| #include <Windows.h> | ||
| // Psapi.h must follow Windows.h. | ||
| #include <Psapi.h> |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
fd -a '^CMakeLists\.txt$' src/pages/shifting-mosaic-solver/a-star \
-x rg -n -i -C2 'psapi|GetProcessMemoryInfo|target_link_libraries' {}Repository: Valle12/islands-of-insight-tools
Length of output: 471
Link Psapi.lib for the Windows target GetProcessMemoryInfo here needs the PSAPI import library, but shifting_mosaic_a_star only links Threads::Threads; add Psapi.lib on Windows or the build will fail at link time.
🧰 Tools
🪛 GitHub Check: SonarCloud Code Analysis
[warning] 107-107: Move these 2 #include directives to the top of the file.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/pages/shifting-mosaic-solver/a-star/MemoryProbe.h` around lines 107 -
109, Add the Windows-only Psapi.lib link dependency to the
shifting_mosaic_a_star target, alongside its existing Threads::Threads linkage,
so GetProcessMemoryInfo used by MemoryProbe is resolved without affecting
non-Windows builds.
| cfg.weight = static_cast<uint8_t>(envU32("SHIFTING_MOSAIC_WEIGHT", cfg.weight)); | ||
| cfg.pathBlockerWeight = static_cast<uint8_t>( | ||
| envU32("SHIFTING_MOSAIC_PATH_BLOCKER_WEIGHT", cfg.pathBlockerWeight)); | ||
| cfg.boundaryDistanceWeight = static_cast<uint8_t>( | ||
| envU32("SHIFTING_MOSAIC_BOUNDARY_WEIGHT", cfg.boundaryDistanceWeight)); | ||
| cfg.axisAwareWeight = static_cast<uint8_t>( | ||
| envU32("SHIFTING_MOSAIC_AXIS_WEIGHT", cfg.axisAwareWeight)); | ||
| cfg.lpDisplacementWeight = static_cast<uint8_t>( | ||
| envU32("SHIFTING_MOSAIC_LP_WEIGHT", cfg.lpDisplacementWeight)); | ||
| cfg.strideOverride = static_cast<uint8_t>( | ||
| envU32("SHIFTING_MOSAIC_STRIDE", cfg.strideOverride)); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== json_test.cpp around target lines ==\n'
nl -ba src/pages/shifting-mosaic-solver/a-star/test/json_test.cpp | sed -n '150,230p'
printf '\n== search envU32 and envU8/envU16 helpers ==\n'
rg -n "envU32|envU8|envU16|UINT8_MAX|UINT16_MAX|static_cast<uint8_t>|static_cast<uint16_t>" src/pages/shifting-mosaic-solver/a-star -S
printf '\n== locate config type definitions ==\n'
rg -n "struct .*Config|class .*Config|weight|pathBlockerWeight|boundaryDistanceWeight|axisAwareWeight|lpDisplacementWeight|strideOverride" src/pages/shifting-mosaic-solver/a-star -SRepository: Valle12/islands-of-insight-tools
Length of output: 251
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== json_test.cpp around target lines ==\n'
awk 'NR>=150 && NR<=230 { printf "%6d %s\n", NR, $0 }' src/pages/shifting-mosaic-solver/a-star/test/json_test.cpp
printf '\n== search envU32 and casting helpers ==\n'
rg -n "envU32|envU8|envU16|UINT8_MAX|UINT16_MAX|static_cast<uint8_t>|static_cast<uint16_t>" src/pages/shifting-mosaic-solver/a-star -S
printf '\n== locate config declarations ==\n'
rg -n "weight|pathBlockerWeight|boundaryDistanceWeight|axisAwareWeight|lpDisplacementWeight|strideOverride" src/pages/shifting-mosaic-solver/a-star -SRepository: Valle12/islands-of-insight-tools
Length of output: 50388
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== envU32 implementation ==\n'
awk 'NR>=100 && NR<=140 { printf "%6d %s\n", NR, $0 }' src/pages/shifting-mosaic-solver/a-star/test/json_test.cpp
printf '\n== exact config field types ==\n'
awk 'NR>=55 && NR<=90 { printf "%6d %s\n", NR, $0 }' src/pages/shifting-mosaic-solver/a-star/AStar.h
printf '\n== nearby uint16 override in json_test.cpp ==\n'
awk 'NR>=204 && NR<=214 { printf "%6d %s\n", NR, $0 }' src/pages/shifting-mosaic-solver/a-star/test/json_test.cppRepository: Valle12/islands-of-insight-tools
Length of output: 5056
Clamp the env overrides before narrowing. envU32 can return values that exceed uint8_t, so these casts can silently wrap (for example, 256 becomes 0). Apply the same bound check to the uint16_t override below as well.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/pages/shifting-mosaic-solver/a-star/test/json_test.cpp` around lines 180
- 190, Clamp every environment override to its destination type’s maximum before
narrowing, including the uint8_t assignments for weight, pathBlockerWeight,
boundaryDistanceWeight, axisAwareWeight, lpDisplacementWeight, and
strideOverride, plus the uint16_t override below. Reuse the existing envU32
values and ensure oversized inputs saturate rather than wrap.
| test("two L-shapes whose bboxes overlap but whose cells do not are not colliding", () => { | ||
| // Shape A: L pointing top-right | ||
| // X X | ||
| // X . | ||
| const shapeA: Position[] = [ | ||
| { x: 0, y: 0 }, | ||
| { x: 1, y: 0 }, | ||
| { x: 0, y: 1 }, | ||
| ]; | ||
| // Shape B: just bottom-right cell | ||
| // . X | ||
| const shapeB: Position[] = [{ x: 1, y: 1 }]; | ||
| // We use a 2x2 bbox shape B by also including {x:0,y:0} for bbox | ||
| // ... but we want a shape that fits the (1,1) corner exactly. | ||
| // Easier: use a real bbox-overlapping case where the cell at (1,1) is empty. | ||
| const shapeBReal: Position[] = [ | ||
| { x: 1, y: 1 }, | ||
| { x: 0, y: 1 }, | ||
| ]; | ||
| // shapeBReal's bbox is 2x2 (because we have x∈{0,1}, y∈{0,1}... wait y=1 only) | ||
| // Actually bbox is 2 wide, 1 tall (since min(y)=1, but bbox calc uses max(y)+1 = 2) | ||
| // Hmm bbox calc is just max+1 — let me redo with normalized shape. | ||
| // Skip this — use a more direct test below instead. | ||
| void shapeA; | ||
| void shapeB; | ||
| void shapeBReal; | ||
| }); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Delete this assertion-free placeholder test. The body is abandoned scratch work (void shapeA; etc.) with a comment saying to skip it; it always passes and SonarCloud flags it as a failure. Either write the intended bbox-overlap/cells-disjoint case or drop the test — the "L-shape and single cell" test below already covers it.
♻️ Proposed removal
- test("two L-shapes whose bboxes overlap but whose cells do not are not colliding", () => {
- // Shape A: L pointing top-right
- // X X
- // X .
- const shapeA: Position[] = [
- { x: 0, y: 0 },
- { x: 1, y: 0 },
- { x: 0, y: 1 },
- ];
- // Shape B: just bottom-right cell
- // . X
- const shapeB: Position[] = [{ x: 1, y: 1 }];
- // We use a 2x2 bbox shape B by also including {x:0,y:0} for bbox
- // ... but we want a shape that fits the (1,1) corner exactly.
- // Easier: use a real bbox-overlapping case where the cell at (1,1) is empty.
- const shapeBReal: Position[] = [
- { x: 1, y: 1 },
- { x: 0, y: 1 },
- ];
- // shapeBReal's bbox is 2x2 (because we have x∈{0,1}, y∈{0,1}... wait y=1 only)
- // Actually bbox is 2 wide, 1 tall (since min(y)=1, but bbox calc uses max(y)+1 = 2)
- // Hmm bbox calc is just max+1 — let me redo with normalized shape.
- // Skip this — use a more direct test below instead.
- void shapeA;
- void shapeB;
- void shapeBReal;
- });
-📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| test("two L-shapes whose bboxes overlap but whose cells do not are not colliding", () => { | |
| // Shape A: L pointing top-right | |
| // X X | |
| // X . | |
| const shapeA: Position[] = [ | |
| { x: 0, y: 0 }, | |
| { x: 1, y: 0 }, | |
| { x: 0, y: 1 }, | |
| ]; | |
| // Shape B: just bottom-right cell | |
| // . X | |
| const shapeB: Position[] = [{ x: 1, y: 1 }]; | |
| // We use a 2x2 bbox shape B by also including {x:0,y:0} for bbox | |
| // ... but we want a shape that fits the (1,1) corner exactly. | |
| // Easier: use a real bbox-overlapping case where the cell at (1,1) is empty. | |
| const shapeBReal: Position[] = [ | |
| { x: 1, y: 1 }, | |
| { x: 0, y: 1 }, | |
| ]; | |
| // shapeBReal's bbox is 2x2 (because we have x∈{0,1}, y∈{0,1}... wait y=1 only) | |
| // Actually bbox is 2 wide, 1 tall (since min(y)=1, but bbox calc uses max(y)+1 = 2) | |
| // Hmm bbox calc is just max+1 — let me redo with normalized shape. | |
| // Skip this — use a more direct test below instead. | |
| void shapeA; | |
| void shapeB; | |
| void shapeBReal; | |
| }); |
🧰 Tools
🪛 GitHub Check: SonarCloud Code Analysis
[failure] 55-55: Add at least one assertion to this test case.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/shiftingMosaicSolver/aStar.collision.test.ts` around lines 55 - 81,
Remove the assertion-free placeholder test and its unused shape
declarations/comments from the test suite; retain the existing concrete
bbox-overlap/cells-disjoint coverage in the following test.
Source: Linters/SAST tools
Summary by CodeRabbit
New Features
Bug Fixes
Style