Repository navigation
IIT-24-Add-additional-phasic-dial - #24
Merged
Merged
Conversation
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (149)
💤 Files with no reviewable changes (4)
📝 WalkthroughWalkthroughThis PR adds phasic-dial configuration persistence and asynchronous solving, centralizes JSON drag-and-drop handling, migrates rolling-blocks tests from BFS to A*, consolidates clang-tidy configuration, reorganizes solver fixtures, and expands unit and end-to-end coverage. ChangesPhasic dial solver
Rolling-blocks solver
Shifting-mosaic solver and repository tooling
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3✅ 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: 7
🧹 Nitpick comments (8)
src/pages/phasic-dial-solver/index.html (1)
67-67: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winWarning banner isn't announced to assistive tech.
#warning-bannerhas norole="alert"/aria-live, so whenshowWarning()sets its text and un-hides it (on invalid file loads), screen-reader users get no notification of the error.♿ Proposed fix
- <div id="warning-banner" class="hidden"></div> + <div id="warning-banner" class="hidden" role="alert" aria-live="assertive"></div>🤖 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/phasic-dial-solver/index.html` at line 67, Add an appropriate live-region mechanism to the `#warning-banner` element, such as role="alert" or an assertive aria-live setting, so text updates and visibility changes made by showWarning() are announced to assistive technology.src/pages/phasic-dial-solver/config.ts (1)
33-72: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winNo upper bound on dial-size or button-count values from uploaded configs.
validateDialsonly requiresmaxValues[i] >= 1andvalidateButtonsonly checks arity, with no ceiling on either the dial-size values or the number of buttons. Since this validator is the sole gate for the file-upload/drag-and-drop path (loadConfigFromFile→validateConfig→applyLoadedConfig), it bypasses any implicit UI limits. A malformed or malicious config with, say,maxValues: [1e9, ...]or hundreds of buttons will makeTurnSolver's cartesian-product search space astronomically large — the async chunking (YIELD_INTERVAL) keeps the UI thread from freezing, but the search itself will never realistically complete, silently pinning a CPU core indefinitely.Consider adding a sane upper bound (e.g. same order of magnitude as legitimate in-game dials) on
maxValues[i]and onbuttons.length.Also applies to: 77-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/phasic-dial-solver/config.ts` around lines 33 - 72, Bound uploaded configuration sizes in validateDials and validateButtons: reject each maxValues entry above the chosen sane dial-size limit and reject buttons arrays exceeding the chosen button-count limit. Return clear validation errors identifying the offending value or count, while preserving existing validation for valid configurations and the loadConfigFromFile → validateConfig flow.e2e/phasic-dial-solver/solvable.test.ts (1)
64-143: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffCodegen selector chains will break on any Material Web internal change.
Every field is addressed through
md-outlined-text-field > .text-field > .field > .input-wrapper > .input. Those class names are private to@material/webshadow markup and carry no semantics. A small helper based on cell coordinates plusinput(orgetByRole("spinbutton")) would keep these two tests readable and upgrade-safe.♻️ Sketch of a coordinate helper
const buttonField = (page: Page, row: number, dial: number) => page.locator(`#table-body tr:nth-child(${row}) td:nth-child(${dial + 1}) input`);🤖 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/phasic-dial-solver/solvable.test.ts` around lines 64 - 143, Replace the Material Web internal selector chains in the solvable test with a coordinate-based helper for table fields, such as buttonField, targeting the row and dial cell plus its input (or semantic spinbutton role). Use the helper for every fill and Tab operation while preserving the existing row, dial, and value sequence.src/pages/phasic-dial-solver/phasicDialSolver.ts (1)
306-321: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low value
calculate()has no rejection path.
void this.calculate()from the click handler means any throw insidecalculateTurnsAsync()(orreadConfig) surfaces as an unhandled promise rejection with no user feedback — the spinner clears viafinally, but the page silently shows nothing. Wrapping the await and routing toshowWarning()keeps the failure visible.♻️ Surface search failures
let result: number[] | null; try { result = await solver.calculateTurnsAsync(); + } catch { + if (generation === this.solveGeneration) { + this.showWarning("The search failed unexpectedly."); + } + return; } finally { if (generation === this.solveGeneration) this.setSolving(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/phasic-dial-solver/phasicDialSolver.ts` around lines 306 - 321, Update PhasicDialSolver.calculate() to catch failures from readConfig() and solver.calculateTurnsAsync(), route the error to showWarning(), and preserve the existing finally block so solving state is cleared. Ensure the rejection is handled when calculate() is invoked via void from the click handler.src/util/configFile.ts (1)
15-17: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winDefer the object URL revoke
URL.revokeObjectURL(url)in the same task can cancel the download in Safari/WebKit. Move it to a zero-delay timeout so the browser can start reading the blob first.🤖 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/configFile.ts` around lines 15 - 17, Defer URL cleanup in the download flow by moving URL.revokeObjectURL(url) into a zero-delay timeout after link.click() and link.remove(), allowing Safari/WebKit to begin reading the blob before revocation.test/rolling-blocks-solver/block.validity.test.ts (1)
365-385: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winFragile shared-state coupling between
test.eachiterations.
indexis mutated inside the test body (index = 13) to affect the next parameterized run rather than being tied to each specific case. This creates hidden order-dependency: if Bun ever runs these cases out of order, filters one out, or a fixture is added/reordered, the wrong block index gets asserted against instead of failing clearly.Pair each block-array fixture with the target index explicitly instead of relying on shared mutable state.
♻️ Proposed fix: pair fixtures with explicit indices
- const nonOverlappingBlocksCase = [ + const nonOverlappingBlocksCase: [Block[], number][] = [ [ - [ + [ new Block(1, 0, 0, 4, 1, 1), new Block(2, 4, 0, 1, 4, 1), new Block(3, 1, 4, 4, 1, 1), new Block(4, 0, 1, 1, 4, 1), - ], + ], + 0, ], [ - [ + [ new Block(1, 0, 0, 1, 1, 1), ... - ], + ], + 13, ], ]; - - let index = 0; test.each(nonOverlappingBlocksCase)( "there should be no overlaps between blocks (%#)", - blocks => { + (blocks, index) => { checkOutOfBoundsSpy.mockReturnValue(true); checkBlockingCellsSpy.mockReturnValue(true); const result = blocks[index]!.checkValidity( 5, 5, cells, blocks, mustTouchCellsSatisfied, ); expect(result).toBeTrue(); expect((blocks[index]! as any).checkBlockingCells).toHaveBeenCalled(); - index = 13; }, );🤖 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/rolling-blocks-solver/block.validity.test.ts` around lines 365 - 385, Remove the shared mutable index from the test.each flow and pair each nonOverlappingBlocksCase fixture with its intended block index explicitly. Update the parameterized callback to receive that index and use it for checkValidity and checkBlockingCells assertions, eliminating the index = 13 mutation and preserving independent behavior for each case.e2e/rolling-blocks-solver/placement.test.ts (1)
3-31: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the unused
setupSolvedPuzzlehelper, or add the missing test that uses it. It’s defined here but never called in this file, so it reads as dead code.🤖 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/rolling-blocks-solver/placement.test.ts` around lines 3 - 31, Remove the unused setupSolvedPuzzle helper from the test file, unless adding a test that invokes it is required; do not leave the helper defined without a call site.e2e/rolling-blocks-solver/solution.test.ts (1)
3-31: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winBoard-configuration steps (grid/goal/block/height) are copy-pasted four times across two files. The shared root cause is the lack of a small, reusable "configure puzzle" helper separate from the full "configure + calculate + wait" helper.
e2e/rolling-blocks-solver/solution.test.ts#L3-L31: extract lines 4-27 (grid size, goal, block, height) into a standalone helper (e.g.configureSimplePuzzle(page)) and havesetupSolvedPuzzlecall it plus the calculate/wait steps.e2e/rolling-blocks-solver/solution.test.ts#L33-L93: replace the inlined setup (lines 40-64) with a call to the newconfigureSimplePuzzle(page)helper.e2e/rolling-blocks-solver/solution.test.ts#L154-L195: replace the inlined setup (lines 160-181) with a call to the newconfigureSimplePuzzle(page)helper before performing the double-click-calculate assertions.e2e/rolling-blocks-solver/tools.test.ts#L3-L31: this entiresetupSolvedPuzzlecopy is unused in this file; remove it, or import the shared helper from a common test-utils module if it's needed by a future test here.🤖 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/rolling-blocks-solver/solution.test.ts` around lines 3 - 31, Extract the shared board-configuration steps from setupSolvedPuzzle into a reusable configureSimplePuzzle helper, then have setupSolvedPuzzle call it before calculating and waiting for the solution. In e2e/rolling-blocks-solver/solution.test.ts ranges 33-93 and 154-195, replace the duplicated setup with configureSimplePuzzle(page) while preserving each test’s existing assertions and calculation behavior. In e2e/rolling-blocks-solver/tools.test.ts range 3-31, remove the unused setupSolvedPuzzle copy, or move the shared helper to common test utilities only if it is needed there.
🤖 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/phasic-dial-solver/config.test.ts`:
- Around line 172-173: Update the solve flow after clicking the “Calculate
Turns” button so the test waits for the first rendered solution before reading
`#result` via `textContent`. Use the existing result locator and an appropriate
visibility or non-empty-text assertion, then preserve the subsequent
`toHaveText(solved!)` validation.
In `@e2e/rolling-blocks-solver/tools.test.ts`:
- Around line 800-868: Replace the brittle page.locator("`#button`").nth(4) lookup
with a stable locator for the intended control, using its accessible role and
name or a dedicated test ID. Keep the existing click and subsequent aria
snapshot unchanged.
In `@test/phasic-dial-solver/phasicDialSolver.test.ts`:
- Around line 601-603: Replace the paired Promise.resolve waits in the test
setup and the corresponding locations around the later assertions with the
existing flush() helper, awaiting one macrotask so loadConfigFromFile and its
file-reading chain fully settle before DOM assertions.
In `@test/resources/rolling-blocks-solver/rollingBlocksTest32.json`:
- Around line 146-161: Correct the rolling-block test fixture so blocks 6 and 7
do not start on cells marked "unplayable" in the cell map, updating their
coordinates or the corresponding cell definitions consistently. If the fixture
is intentionally invalid, replace this setup with an explicit test asserting
that such a board is rejected.
In `@test/rolling-blocks-solver/aStar.test.ts`:
- Around line 37-69: Extend the solvableCases fixture list in the A* test to
include rollingBlocksTest31.json through rollingBlocksTest39.json, preserving
the existing tuple structure and ordering so the Bun path covers all
native-suite cases.
In `@test/shifting-mosaic-solver/aStar.collision.test.ts`:
- Line 58: Correct the test title in the L-shape and single-cell collision test
to state that the bounding boxes overlap without the individual cells
overlapping; leave the test assertions and behavior unchanged.
In `@test/shifting-mosaic-solver/aStar.search.test.ts`:
- Around line 236-247: Strengthen the replay validation in the aStar.search test
around applyTurns so every turn is checked for legal bounds and collision-free
transitions according to the board rules, rather than relying only on the final
anchor assertion. Preserve the existing final-anchor verification after
validating the complete sequence returned by aStar.search().
---
Nitpick comments:
In `@e2e/phasic-dial-solver/solvable.test.ts`:
- Around line 64-143: Replace the Material Web internal selector chains in the
solvable test with a coordinate-based helper for table fields, such as
buttonField, targeting the row and dial cell plus its input (or semantic
spinbutton role). Use the helper for every fill and Tab operation while
preserving the existing row, dial, and value sequence.
In `@e2e/rolling-blocks-solver/placement.test.ts`:
- Around line 3-31: Remove the unused setupSolvedPuzzle helper from the test
file, unless adding a test that invokes it is required; do not leave the helper
defined without a call site.
In `@e2e/rolling-blocks-solver/solution.test.ts`:
- Around line 3-31: Extract the shared board-configuration steps from
setupSolvedPuzzle into a reusable configureSimplePuzzle helper, then have
setupSolvedPuzzle call it before calculating and waiting for the solution. In
e2e/rolling-blocks-solver/solution.test.ts ranges 33-93 and 154-195, replace the
duplicated setup with configureSimplePuzzle(page) while preserving each test’s
existing assertions and calculation behavior. In
e2e/rolling-blocks-solver/tools.test.ts range 3-31, remove the unused
setupSolvedPuzzle copy, or move the shared helper to common test utilities only
if it is needed there.
In `@src/pages/phasic-dial-solver/config.ts`:
- Around line 33-72: Bound uploaded configuration sizes in validateDials and
validateButtons: reject each maxValues entry above the chosen sane dial-size
limit and reject buttons arrays exceeding the chosen button-count limit. Return
clear validation errors identifying the offending value or count, while
preserving existing validation for valid configurations and the
loadConfigFromFile → validateConfig flow.
In `@src/pages/phasic-dial-solver/index.html`:
- Line 67: Add an appropriate live-region mechanism to the `#warning-banner`
element, such as role="alert" or an assertive aria-live setting, so text updates
and visibility changes made by showWarning() are announced to assistive
technology.
In `@src/pages/phasic-dial-solver/phasicDialSolver.ts`:
- Around line 306-321: Update PhasicDialSolver.calculate() to catch failures
from readConfig() and solver.calculateTurnsAsync(), route the error to
showWarning(), and preserve the existing finally block so solving state is
cleared. Ensure the rejection is handled when calculate() is invoked via void
from the click handler.
In `@src/util/configFile.ts`:
- Around line 15-17: Defer URL cleanup in the download flow by moving
URL.revokeObjectURL(url) into a zero-delay timeout after link.click() and
link.remove(), allowing Safari/WebKit to begin reading the blob before
revocation.
In `@test/rolling-blocks-solver/block.validity.test.ts`:
- Around line 365-385: Remove the shared mutable index from the test.each flow
and pair each nonOverlappingBlocksCase fixture with its intended block index
explicitly. Update the parameterized callback to receive that index and use it
for checkValidity and checkBlockingCells assertions, eliminating the index = 13
mutation and preserving independent behavior for each case.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: c93b89ff-37ce-452f-b2a9-2875513ec779
⛔ Files ignored due to path filters (1)
images/purple-dial.pngis excluded by!**/*.png
📒 Files selected for processing (149)
.github/workflows/test.ymlCLAUDE.mdcmake/ClangTidyGate.cmakee2e/phasic-dial-solver/config.test.tse2e/phasic-dial-solver/solvable.test.tse2e/phasic-dial-solver/states.test.tse2e/phasicDialSolver.states.test.tse2e/rolling-blocks-solver/placement.test.tse2e/rolling-blocks-solver/solution.test.tse2e/rolling-blocks-solver/tools.test.tse2e/shifting-mosaic-solver/blocks.test.tse2e/shifting-mosaic-solver/config.test.tse2e/shifting-mosaic-solver/editing.test.tse2e/shifting-mosaic-solver/heapLimit.slow.test.tse2e/shifting-mosaic-solver/solving.test.tse2e/shifting-mosaic-solver/wasmVariants.test.tspackage.jsonsrc/common/coiRegister.tssrc/common/common.csssrc/pages/phasic-dial-solver/config.tssrc/pages/phasic-dial-solver/index.htmlsrc/pages/phasic-dial-solver/phasicDialSolver.csssrc/pages/phasic-dial-solver/phasicDialSolver.tssrc/pages/phasic-dial-solver/turnSolver.tssrc/pages/rolling-blocks-solver/a-star/.clang-tidysrc/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/a-star/test/rolling_blocks_test.cppsrc/pages/rolling-blocks-solver/aStar.tssrc/pages/rolling-blocks-solver/bfs.tssrc/pages/rolling-blocks-solver/rollingBlocksSolver.csssrc/pages/shifting-mosaic-solver/a-star/CMakeLists.txtsrc/pages/shifting-mosaic-solver/a-star/test/CMakeLists.txtsrc/pages/shifting-mosaic-solver/editorHelpers.tssrc/pages/shifting-mosaic-solver/shiftingMosaicSolver.csssrc/pages/shifting-mosaic-solver/shiftingMosaicSolver.tssrc/util/benchShiftingMosaic.tssrc/util/benchWasmSimd.tssrc/util/build.tssrc/util/configFile.tssrc/util/serve.tssrc/util/types.tstest/configFile.test.tstest/phasic-dial-solver/config.test.tstest/phasic-dial-solver/phasicDialSolver.test.tstest/phasic-dial-solver/turnSolver.test.tstest/resources/phasic-dial-solver/phasicDialTest.jsontest/resources/phasic-dial-solver/phasicDialTest1.jsontest/resources/rolling-blocks-solver/rollingBlocksTest.jsontest/resources/rolling-blocks-solver/rollingBlocksTest1.jsontest/resources/rolling-blocks-solver/rollingBlocksTest10.jsontest/resources/rolling-blocks-solver/rollingBlocksTest11.jsontest/resources/rolling-blocks-solver/rollingBlocksTest12.jsontest/resources/rolling-blocks-solver/rollingBlocksTest13.jsontest/resources/rolling-blocks-solver/rollingBlocksTest14.jsontest/resources/rolling-blocks-solver/rollingBlocksTest15.jsontest/resources/rolling-blocks-solver/rollingBlocksTest16.jsontest/resources/rolling-blocks-solver/rollingBlocksTest17.jsontest/resources/rolling-blocks-solver/rollingBlocksTest18.jsontest/resources/rolling-blocks-solver/rollingBlocksTest19.jsontest/resources/rolling-blocks-solver/rollingBlocksTest2.jsontest/resources/rolling-blocks-solver/rollingBlocksTest20.jsontest/resources/rolling-blocks-solver/rollingBlocksTest21.jsontest/resources/rolling-blocks-solver/rollingBlocksTest22.jsontest/resources/rolling-blocks-solver/rollingBlocksTest23.jsontest/resources/rolling-blocks-solver/rollingBlocksTest24.jsontest/resources/rolling-blocks-solver/rollingBlocksTest25.jsontest/resources/rolling-blocks-solver/rollingBlocksTest26.jsontest/resources/rolling-blocks-solver/rollingBlocksTest27.jsontest/resources/rolling-blocks-solver/rollingBlocksTest28.jsontest/resources/rolling-blocks-solver/rollingBlocksTest29.jsontest/resources/rolling-blocks-solver/rollingBlocksTest3.jsontest/resources/rolling-blocks-solver/rollingBlocksTest30.jsontest/resources/rolling-blocks-solver/rollingBlocksTest31.jsontest/resources/rolling-blocks-solver/rollingBlocksTest32.jsontest/resources/rolling-blocks-solver/rollingBlocksTest33.jsontest/resources/rolling-blocks-solver/rollingBlocksTest34.jsontest/resources/rolling-blocks-solver/rollingBlocksTest35.jsontest/resources/rolling-blocks-solver/rollingBlocksTest36.jsontest/resources/rolling-blocks-solver/rollingBlocksTest37.jsontest/resources/rolling-blocks-solver/rollingBlocksTest38.jsontest/resources/rolling-blocks-solver/rollingBlocksTest39.jsontest/resources/rolling-blocks-solver/rollingBlocksTest4.jsontest/resources/rolling-blocks-solver/rollingBlocksTest5.jsontest/resources/rolling-blocks-solver/rollingBlocksTest6.jsontest/resources/rolling-blocks-solver/rollingBlocksTest7.jsontest/resources/rolling-blocks-solver/rollingBlocksTest8.jsontest/resources/rolling-blocks-solver/rollingBlocksTest9.jsontest/resources/shifting-mosaic-solver/shiftingMosaicTest.jsontest/resources/shifting-mosaic-solver/shiftingMosaicTest1.jsontest/resources/shifting-mosaic-solver/shiftingMosaicTest10.jsontest/resources/shifting-mosaic-solver/shiftingMosaicTest11.jsontest/resources/shifting-mosaic-solver/shiftingMosaicTest12.jsontest/resources/shifting-mosaic-solver/shiftingMosaicTest13.jsontest/resources/shifting-mosaic-solver/shiftingMosaicTest14.jsontest/resources/shifting-mosaic-solver/shiftingMosaicTest15.jsontest/resources/shifting-mosaic-solver/shiftingMosaicTest16.jsontest/resources/shifting-mosaic-solver/shiftingMosaicTest17.jsontest/resources/shifting-mosaic-solver/shiftingMosaicTest18.jsontest/resources/shifting-mosaic-solver/shiftingMosaicTest19.jsontest/resources/shifting-mosaic-solver/shiftingMosaicTest2.jsontest/resources/shifting-mosaic-solver/shiftingMosaicTest20.jsontest/resources/shifting-mosaic-solver/shiftingMosaicTest21.jsontest/resources/shifting-mosaic-solver/shiftingMosaicTest22.jsontest/resources/shifting-mosaic-solver/shiftingMosaicTest23.jsontest/resources/shifting-mosaic-solver/shiftingMosaicTest24.jsontest/resources/shifting-mosaic-solver/shiftingMosaicTest25.jsontest/resources/shifting-mosaic-solver/shiftingMosaicTest26.jsontest/resources/shifting-mosaic-solver/shiftingMosaicTest27.jsontest/resources/shifting-mosaic-solver/shiftingMosaicTest28.jsontest/resources/shifting-mosaic-solver/shiftingMosaicTest29.jsontest/resources/shifting-mosaic-solver/shiftingMosaicTest3.jsontest/resources/shifting-mosaic-solver/shiftingMosaicTest30.jsontest/resources/shifting-mosaic-solver/shiftingMosaicTest31.jsontest/resources/shifting-mosaic-solver/shiftingMosaicTest32.jsontest/resources/shifting-mosaic-solver/shiftingMosaicTest33.jsontest/resources/shifting-mosaic-solver/shiftingMosaicTest34.jsontest/resources/shifting-mosaic-solver/shiftingMosaicTest35.jsontest/resources/shifting-mosaic-solver/shiftingMosaicTest36.jsontest/resources/shifting-mosaic-solver/shiftingMosaicTest37.jsontest/resources/shifting-mosaic-solver/shiftingMosaicTest38.jsontest/resources/shifting-mosaic-solver/shiftingMosaicTest39.jsontest/resources/shifting-mosaic-solver/shiftingMosaicTest4.jsontest/resources/shifting-mosaic-solver/shiftingMosaicTest40.jsontest/resources/shifting-mosaic-solver/shiftingMosaicTest41.jsontest/resources/shifting-mosaic-solver/shiftingMosaicTest42.jsontest/resources/shifting-mosaic-solver/shiftingMosaicTest43.jsontest/resources/shifting-mosaic-solver/shiftingMosaicTest5.jsontest/resources/shifting-mosaic-solver/shiftingMosaicTest6.jsontest/resources/shifting-mosaic-solver/shiftingMosaicTest7.jsontest/resources/shifting-mosaic-solver/shiftingMosaicTest8.jsontest/resources/shifting-mosaic-solver/shiftingMosaicTest9.jsontest/rolling-blocks-solver/aStar.test.tstest/rolling-blocks-solver/block.musttouch.test.tstest/rolling-blocks-solver/block.roll.test.tstest/rolling-blocks-solver/block.validity.test.tstest/shifting-mosaic-solver/aStar.collision.test.tstest/shifting-mosaic-solver/aStar.primitives.test.tstest/shifting-mosaic-solver/aStar.search.test.tstest/shifting-mosaic-solver/block.test.tstest/shifting-mosaic-solver/board.geometry.test.tstest/shifting-mosaic-solver/board.pointer.test.tstest/shifting-mosaic-solver/board.render.test.tstest/shifting-mosaic-solver/board.state.test.tstest/shifting-mosaic-solver/config.test.tstest/shifting-mosaic-solver/mem64.node.test.mjstest/shifting-mosaic-solver/portfolio.worker.tstest/shifting-mosaic-solver/wasm.test.ts
💤 Files with no reviewable changes (4)
- e2e/phasicDialSolver.states.test.ts
- src/pages/rolling-blocks-solver/rollingBlocksSolver.css
- src/pages/rolling-blocks-solver/bfs.ts
- src/pages/shifting-mosaic-solver/shiftingMosaicSolver.css
Comment on lines
+172
to
+173
| await page.getByRole("button", { name: "Calculate Turns" }).click(); | ||
| const solved = await page.locator("#result").textContent(); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Read #result only after the solve has rendered.
textContent is read in the same tick as the click, so solved will usually be "" (the element is hidden and empty until calculate() resolves). The later toHaveText(solved!) then asserts the wrong thing or fails intermittently.
💚 Wait for the first solution
await page.getByRole("button", { name: "Calculate Turns" }).click();
- const solved = await page.locator("`#result`").textContent();
+ await expect(page.locator("`#result`")).toBeVisible();
+ await expect(page.locator("`#result`")).toContainText("Button");
+ const solved = await page.locator("`#result`").textContent();📝 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.
Suggested change
| await page.getByRole("button", { name: "Calculate Turns" }).click(); | |
| const solved = await page.locator("#result").textContent(); | |
| await page.getByRole("button", { name: "Calculate Turns" }).click(); | |
| await expect(page.locator("`#result`")).toBeVisible(); | |
| await expect(page.locator("`#result`")).toContainText("Button"); | |
| const solved = await page.locator("`#result`").textContent(); |
🤖 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/phasic-dial-solver/config.test.ts` around lines 172 - 173, Update the
solve flow after clicking the “Calculate Turns” button so the test waits for the
first rendered solution before reading `#result` via `textContent`. Use the
existing result locator and an appropriate visibility or non-empty-text
assertion, then preserve the subsequent `toHaveText(solved!)` validation.
Comment on lines
+601
to
+603
| // loadConfigFromFile is async; let its promise chain settle. | ||
| await Promise.resolve(); | ||
| await Promise.resolve(); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Waiting a fixed number of microtasks is fragile; use flush().
loadConfigFromFile awaits readJsonFile, which awaits a real File.text(). The number of microtask ticks that takes is an implementation detail of Bun's Blob reader, so two await Promise.resolve() can silently under-wait and the assertions below would then check an unchanged DOM. The existing flush() (a macrotask) covers the whole chain. Same pattern at lines 639-640 and 658-659.
💚 Await a macrotask instead
- // loadConfigFromFile is async; let its promise chain settle.
- await Promise.resolve();
- await Promise.resolve();
+ // loadConfigFromFile reads the file asynchronously; let it settle.
+ await flush();📝 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.
Suggested change
| // loadConfigFromFile is async; let its promise chain settle. | |
| await Promise.resolve(); | |
| await Promise.resolve(); | |
| // loadConfigFromFile reads the file asynchronously; let it settle. | |
| await flush(); |
🤖 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/phasic-dial-solver/phasicDialSolver.test.ts` around lines 601 - 603,
Replace the paired Promise.resolve waits in the test setup and the corresponding
locations around the later assertions with the existing flush() helper, awaiting
one macrotask so loadConfigFromFile and its file-reading chain fully settle
before DOM assertions.
Comment on lines
37
to
69
| const solvableCases = [ | ||
| ["bfsTest.json"], | ||
| ["bfsTest1.json"], | ||
| ["bfsTest2.json"], | ||
| ["bfsTest3.json"], | ||
| ["bfsTest4.json"], | ||
| ["bfsTest5.json"], | ||
| ["bfsTest6.json"], | ||
| ["bfsTest7.json"], | ||
| ["bfsTest8.json"], | ||
| ["bfsTest9.json"], | ||
| ["bfsTest10.json"], | ||
| ["bfsTest11.json"], | ||
| ["bfsTest12.json"], | ||
| ["bfsTest13.json"], | ||
| ["bfsTest14.json"], | ||
| ["bfsTest15.json"], | ||
| ["bfsTest16.json"], | ||
| ["bfsTest17.json"], | ||
| ["bfsTest18.json"], | ||
| ["bfsTest19.json"], | ||
| ["bfsTest20.json"], | ||
| ["bfsTest21.json"], | ||
| ["bfsTest22.json"], | ||
| ["bfsTest23.json"], | ||
| ["bfsTest24.json"], | ||
| ["bfsTest25.json"], | ||
| ["bfsTest26.json"], | ||
| ["bfsTest27.json"], | ||
| ["bfsTest28.json"], | ||
| ["bfsTest29.json"], | ||
| ["bfsTest30.json"], | ||
| ["rollingBlocksTest.json"], | ||
| ["rollingBlocksTest1.json"], | ||
| ["rollingBlocksTest2.json"], | ||
| ["rollingBlocksTest3.json"], | ||
| ["rollingBlocksTest4.json"], | ||
| ["rollingBlocksTest5.json"], | ||
| ["rollingBlocksTest6.json"], | ||
| ["rollingBlocksTest7.json"], | ||
| ["rollingBlocksTest8.json"], | ||
| ["rollingBlocksTest9.json"], | ||
| ["rollingBlocksTest10.json"], | ||
| ["rollingBlocksTest11.json"], | ||
| ["rollingBlocksTest12.json"], | ||
| ["rollingBlocksTest13.json"], | ||
| ["rollingBlocksTest14.json"], | ||
| ["rollingBlocksTest15.json"], | ||
| ["rollingBlocksTest16.json"], | ||
| ["rollingBlocksTest17.json"], | ||
| ["rollingBlocksTest18.json"], | ||
| ["rollingBlocksTest19.json"], | ||
| ["rollingBlocksTest20.json"], | ||
| ["rollingBlocksTest21.json"], | ||
| ["rollingBlocksTest22.json"], | ||
| ["rollingBlocksTest23.json"], | ||
| ["rollingBlocksTest24.json"], | ||
| ["rollingBlocksTest25.json"], | ||
| ["rollingBlocksTest26.json"], | ||
| ["rollingBlocksTest27.json"], | ||
| ["rollingBlocksTest28.json"], | ||
| ["rollingBlocksTest29.json"], | ||
| ["rollingBlocksTest30.json"], | ||
| ]; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if rollingBlocksTest31-39.json are referenced anywhere else in TS/e2e tests
rg -n 'rollingBlocksTest3[1-9]' --type=tsRepository: Valle12/islands-of-insight-tools
Length of output: 170
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n## aStar.test.ts\n'
wc -l test/rolling-blocks-solver/aStar.test.ts
sed -n '1,140p' test/rolling-blocks-solver/aStar.test.ts
printf '\n## fixture references across the repo\n'
rg -n 'rollingBlocksTest([0-9]+)?\.json' test src . --glob '!**/node_modules/**' || true
printf '\n## files that look like rolling blocks fixtures\n'
fd -a 'rollingBlocksTest*.json' . || trueRepository: Valle12/islands-of-insight-tools
Length of output: 9921
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n## native suite references\n'
rg -n 'rollingBlocksTest(3[1-9]|[1-9][0-9]?)\.json|rollingBlocksTest\.json' . --glob '!**/node_modules/**' || true
printf '\n## relevant test file context if present\n'
fd -a 'rolling_blocks_test.cpp|aStar.test.ts|rollingBlocksTest*.json' . || trueRepository: Valle12/islands-of-insight-tools
Length of output: 3057
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n## numbered rolling-blocks fixtures anywhere in repo\n'
rg -n 'rollingBlocksTest[0-9]+\.json' . --glob '!**/node_modules/**' || true
printf '\n## resources directory contents\n'
fd -a . test/resources/rolling-blocks-solver || true
printf '\n## native test file around the file list\n'
sed -n '180,230p' src/pages/rolling-blocks-solver/a-star/test/rolling_blocks_test.cppRepository: Valle12/islands-of-insight-tools
Length of output: 7592
Add rollingBlocksTest31.json–rollingBlocksTest39.json to solvableCases. The native suite already covers those fixtures, but the Bun A* path stops at 30, leaving 9 cases uncovered.
🤖 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/rolling-blocks-solver/aStar.test.ts` around lines 37 - 69, Extend the
solvableCases fixture list in the A* test to include rollingBlocksTest31.json
through rollingBlocksTest39.json, preserving the existing tuple structure and
ordering so the Bun path covers all native-suite cases.
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 7
🧹 Nitpick comments (8)
src/pages/phasic-dial-solver/index.html (1)
67-67: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winWarning banner isn't announced to assistive tech.
#warning-bannerhas norole="alert"/aria-live, so whenshowWarning()sets its text and un-hides it (on invalid file loads), screen-reader users get no notification of the error.♿ Proposed fix
- <div id="warning-banner" class="hidden"></div> + <div id="warning-banner" class="hidden" role="alert" aria-live="assertive"></div>🤖 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/phasic-dial-solver/index.html` at line 67, Add an appropriate live-region mechanism to the `#warning-banner` element, such as role="alert" or an assertive aria-live setting, so text updates and visibility changes made by showWarning() are announced to assistive technology.src/pages/phasic-dial-solver/config.ts (1)
33-72: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winNo upper bound on dial-size or button-count values from uploaded configs.
validateDialsonly requiresmaxValues[i] >= 1andvalidateButtonsonly checks arity, with no ceiling on either the dial-size values or the number of buttons. Since this validator is the sole gate for the file-upload/drag-and-drop path (loadConfigFromFile→validateConfig→applyLoadedConfig), it bypasses any implicit UI limits. A malformed or malicious config with, say,maxValues: [1e9, ...]or hundreds of buttons will makeTurnSolver's cartesian-product search space astronomically large — the async chunking (YIELD_INTERVAL) keeps the UI thread from freezing, but the search itself will never realistically complete, silently pinning a CPU core indefinitely.Consider adding a sane upper bound (e.g. same order of magnitude as legitimate in-game dials) on
maxValues[i]and onbuttons.length.Also applies to: 77-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/phasic-dial-solver/config.ts` around lines 33 - 72, Bound uploaded configuration sizes in validateDials and validateButtons: reject each maxValues entry above the chosen sane dial-size limit and reject buttons arrays exceeding the chosen button-count limit. Return clear validation errors identifying the offending value or count, while preserving existing validation for valid configurations and the loadConfigFromFile → validateConfig flow.e2e/phasic-dial-solver/solvable.test.ts (1)
64-143: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffCodegen selector chains will break on any Material Web internal change.
Every field is addressed through
md-outlined-text-field > .text-field > .field > .input-wrapper > .input. Those class names are private to@material/webshadow markup and carry no semantics. A small helper based on cell coordinates plusinput(orgetByRole("spinbutton")) would keep these two tests readable and upgrade-safe.♻️ Sketch of a coordinate helper
const buttonField = (page: Page, row: number, dial: number) => page.locator(`#table-body tr:nth-child(${row}) td:nth-child(${dial + 1}) input`);🤖 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/phasic-dial-solver/solvable.test.ts` around lines 64 - 143, Replace the Material Web internal selector chains in the solvable test with a coordinate-based helper for table fields, such as buttonField, targeting the row and dial cell plus its input (or semantic spinbutton role). Use the helper for every fill and Tab operation while preserving the existing row, dial, and value sequence.src/pages/phasic-dial-solver/phasicDialSolver.ts (1)
306-321: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low value
calculate()has no rejection path.
void this.calculate()from the click handler means any throw insidecalculateTurnsAsync()(orreadConfig) surfaces as an unhandled promise rejection with no user feedback — the spinner clears viafinally, but the page silently shows nothing. Wrapping the await and routing toshowWarning()keeps the failure visible.♻️ Surface search failures
let result: number[] | null; try { result = await solver.calculateTurnsAsync(); + } catch { + if (generation === this.solveGeneration) { + this.showWarning("The search failed unexpectedly."); + } + return; } finally { if (generation === this.solveGeneration) this.setSolving(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/phasic-dial-solver/phasicDialSolver.ts` around lines 306 - 321, Update PhasicDialSolver.calculate() to catch failures from readConfig() and solver.calculateTurnsAsync(), route the error to showWarning(), and preserve the existing finally block so solving state is cleared. Ensure the rejection is handled when calculate() is invoked via void from the click handler.src/util/configFile.ts (1)
15-17: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winDefer the object URL revoke
URL.revokeObjectURL(url)in the same task can cancel the download in Safari/WebKit. Move it to a zero-delay timeout so the browser can start reading the blob first.🤖 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/configFile.ts` around lines 15 - 17, Defer URL cleanup in the download flow by moving URL.revokeObjectURL(url) into a zero-delay timeout after link.click() and link.remove(), allowing Safari/WebKit to begin reading the blob before revocation.test/rolling-blocks-solver/block.validity.test.ts (1)
365-385: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winFragile shared-state coupling between
test.eachiterations.
indexis mutated inside the test body (index = 13) to affect the next parameterized run rather than being tied to each specific case. This creates hidden order-dependency: if Bun ever runs these cases out of order, filters one out, or a fixture is added/reordered, the wrong block index gets asserted against instead of failing clearly.Pair each block-array fixture with the target index explicitly instead of relying on shared mutable state.
♻️ Proposed fix: pair fixtures with explicit indices
- const nonOverlappingBlocksCase = [ + const nonOverlappingBlocksCase: [Block[], number][] = [ [ - [ + [ new Block(1, 0, 0, 4, 1, 1), new Block(2, 4, 0, 1, 4, 1), new Block(3, 1, 4, 4, 1, 1), new Block(4, 0, 1, 1, 4, 1), - ], + ], + 0, ], [ - [ + [ new Block(1, 0, 0, 1, 1, 1), ... - ], + ], + 13, ], ]; - - let index = 0; test.each(nonOverlappingBlocksCase)( "there should be no overlaps between blocks (%#)", - blocks => { + (blocks, index) => { checkOutOfBoundsSpy.mockReturnValue(true); checkBlockingCellsSpy.mockReturnValue(true); const result = blocks[index]!.checkValidity( 5, 5, cells, blocks, mustTouchCellsSatisfied, ); expect(result).toBeTrue(); expect((blocks[index]! as any).checkBlockingCells).toHaveBeenCalled(); - index = 13; }, );🤖 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/rolling-blocks-solver/block.validity.test.ts` around lines 365 - 385, Remove the shared mutable index from the test.each flow and pair each nonOverlappingBlocksCase fixture with its intended block index explicitly. Update the parameterized callback to receive that index and use it for checkValidity and checkBlockingCells assertions, eliminating the index = 13 mutation and preserving independent behavior for each case.e2e/rolling-blocks-solver/placement.test.ts (1)
3-31: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the unused
setupSolvedPuzzlehelper, or add the missing test that uses it. It’s defined here but never called in this file, so it reads as dead code.🤖 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/rolling-blocks-solver/placement.test.ts` around lines 3 - 31, Remove the unused setupSolvedPuzzle helper from the test file, unless adding a test that invokes it is required; do not leave the helper defined without a call site.e2e/rolling-blocks-solver/solution.test.ts (1)
3-31: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winBoard-configuration steps (grid/goal/block/height) are copy-pasted four times across two files. The shared root cause is the lack of a small, reusable "configure puzzle" helper separate from the full "configure + calculate + wait" helper.
e2e/rolling-blocks-solver/solution.test.ts#L3-L31: extract lines 4-27 (grid size, goal, block, height) into a standalone helper (e.g.configureSimplePuzzle(page)) and havesetupSolvedPuzzlecall it plus the calculate/wait steps.e2e/rolling-blocks-solver/solution.test.ts#L33-L93: replace the inlined setup (lines 40-64) with a call to the newconfigureSimplePuzzle(page)helper.e2e/rolling-blocks-solver/solution.test.ts#L154-L195: replace the inlined setup (lines 160-181) with a call to the newconfigureSimplePuzzle(page)helper before performing the double-click-calculate assertions.e2e/rolling-blocks-solver/tools.test.ts#L3-L31: this entiresetupSolvedPuzzlecopy is unused in this file; remove it, or import the shared helper from a common test-utils module if it's needed by a future test here.🤖 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/rolling-blocks-solver/solution.test.ts` around lines 3 - 31, Extract the shared board-configuration steps from setupSolvedPuzzle into a reusable configureSimplePuzzle helper, then have setupSolvedPuzzle call it before calculating and waiting for the solution. In e2e/rolling-blocks-solver/solution.test.ts ranges 33-93 and 154-195, replace the duplicated setup with configureSimplePuzzle(page) while preserving each test’s existing assertions and calculation behavior. In e2e/rolling-blocks-solver/tools.test.ts range 3-31, remove the unused setupSolvedPuzzle copy, or move the shared helper to common test utilities only if it is needed there.
🤖 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/phasic-dial-solver/config.test.ts`:
- Around line 172-173: Update the solve flow after clicking the “Calculate
Turns” button so the test waits for the first rendered solution before reading
`#result` via `textContent`. Use the existing result locator and an appropriate
visibility or non-empty-text assertion, then preserve the subsequent
`toHaveText(solved!)` validation.
In `@e2e/rolling-blocks-solver/tools.test.ts`:
- Around line 800-868: Replace the brittle page.locator("`#button`").nth(4) lookup
with a stable locator for the intended control, using its accessible role and
name or a dedicated test ID. Keep the existing click and subsequent aria
snapshot unchanged.
In `@test/phasic-dial-solver/phasicDialSolver.test.ts`:
- Around line 601-603: Replace the paired Promise.resolve waits in the test
setup and the corresponding locations around the later assertions with the
existing flush() helper, awaiting one macrotask so loadConfigFromFile and its
file-reading chain fully settle before DOM assertions.
In `@test/resources/rolling-blocks-solver/rollingBlocksTest32.json`:
- Around line 146-161: Correct the rolling-block test fixture so blocks 6 and 7
do not start on cells marked "unplayable" in the cell map, updating their
coordinates or the corresponding cell definitions consistently. If the fixture
is intentionally invalid, replace this setup with an explicit test asserting
that such a board is rejected.
In `@test/rolling-blocks-solver/aStar.test.ts`:
- Around line 37-69: Extend the solvableCases fixture list in the A* test to
include rollingBlocksTest31.json through rollingBlocksTest39.json, preserving
the existing tuple structure and ordering so the Bun path covers all
native-suite cases.
In `@test/shifting-mosaic-solver/aStar.collision.test.ts`:
- Line 58: Correct the test title in the L-shape and single-cell collision test
to state that the bounding boxes overlap without the individual cells
overlapping; leave the test assertions and behavior unchanged.
In `@test/shifting-mosaic-solver/aStar.search.test.ts`:
- Around line 236-247: Strengthen the replay validation in the aStar.search test
around applyTurns so every turn is checked for legal bounds and collision-free
transitions according to the board rules, rather than relying only on the final
anchor assertion. Preserve the existing final-anchor verification after
validating the complete sequence returned by aStar.search().
---
Nitpick comments:
In `@e2e/phasic-dial-solver/solvable.test.ts`:
- Around line 64-143: Replace the Material Web internal selector chains in the
solvable test with a coordinate-based helper for table fields, such as
buttonField, targeting the row and dial cell plus its input (or semantic
spinbutton role). Use the helper for every fill and Tab operation while
preserving the existing row, dial, and value sequence.
In `@e2e/rolling-blocks-solver/placement.test.ts`:
- Around line 3-31: Remove the unused setupSolvedPuzzle helper from the test
file, unless adding a test that invokes it is required; do not leave the helper
defined without a call site.
In `@e2e/rolling-blocks-solver/solution.test.ts`:
- Around line 3-31: Extract the shared board-configuration steps from
setupSolvedPuzzle into a reusable configureSimplePuzzle helper, then have
setupSolvedPuzzle call it before calculating and waiting for the solution. In
e2e/rolling-blocks-solver/solution.test.ts ranges 33-93 and 154-195, replace the
duplicated setup with configureSimplePuzzle(page) while preserving each test’s
existing assertions and calculation behavior. In
e2e/rolling-blocks-solver/tools.test.ts range 3-31, remove the unused
setupSolvedPuzzle copy, or move the shared helper to common test utilities only
if it is needed there.
In `@src/pages/phasic-dial-solver/config.ts`:
- Around line 33-72: Bound uploaded configuration sizes in validateDials and
validateButtons: reject each maxValues entry above the chosen sane dial-size
limit and reject buttons arrays exceeding the chosen button-count limit. Return
clear validation errors identifying the offending value or count, while
preserving existing validation for valid configurations and the
loadConfigFromFile → validateConfig flow.
In `@src/pages/phasic-dial-solver/index.html`:
- Line 67: Add an appropriate live-region mechanism to the `#warning-banner`
element, such as role="alert" or an assertive aria-live setting, so text updates
and visibility changes made by showWarning() are announced to assistive
technology.
In `@src/pages/phasic-dial-solver/phasicDialSolver.ts`:
- Around line 306-321: Update PhasicDialSolver.calculate() to catch failures
from readConfig() and solver.calculateTurnsAsync(), route the error to
showWarning(), and preserve the existing finally block so solving state is
cleared. Ensure the rejection is handled when calculate() is invoked via void
from the click handler.
In `@src/util/configFile.ts`:
- Around line 15-17: Defer URL cleanup in the download flow by moving
URL.revokeObjectURL(url) into a zero-delay timeout after link.click() and
link.remove(), allowing Safari/WebKit to begin reading the blob before
revocation.
In `@test/rolling-blocks-solver/block.validity.test.ts`:
- Around line 365-385: Remove the shared mutable index from the test.each flow
and pair each nonOverlappingBlocksCase fixture with its intended block index
explicitly. Update the parameterized callback to receive that index and use it
for checkValidity and checkBlockingCells assertions, eliminating the index = 13
mutation and preserving independent behavior for each case.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: c93b89ff-37ce-452f-b2a9-2875513ec779
⛔ Files ignored due to path filters (1)
images/purple-dial.pngis excluded by!**/*.png
📒 Files selected for processing (149)
.github/workflows/test.ymlCLAUDE.mdcmake/ClangTidyGate.cmakee2e/phasic-dial-solver/config.test.tse2e/phasic-dial-solver/solvable.test.tse2e/phasic-dial-solver/states.test.tse2e/phasicDialSolver.states.test.tse2e/rolling-blocks-solver/placement.test.tse2e/rolling-blocks-solver/solution.test.tse2e/rolling-blocks-solver/tools.test.tse2e/shifting-mosaic-solver/blocks.test.tse2e/shifting-mosaic-solver/config.test.tse2e/shifting-mosaic-solver/editing.test.tse2e/shifting-mosaic-solver/heapLimit.slow.test.tse2e/shifting-mosaic-solver/solving.test.tse2e/shifting-mosaic-solver/wasmVariants.test.tspackage.jsonsrc/common/coiRegister.tssrc/common/common.csssrc/pages/phasic-dial-solver/config.tssrc/pages/phasic-dial-solver/index.htmlsrc/pages/phasic-dial-solver/phasicDialSolver.csssrc/pages/phasic-dial-solver/phasicDialSolver.tssrc/pages/phasic-dial-solver/turnSolver.tssrc/pages/rolling-blocks-solver/a-star/.clang-tidysrc/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/a-star/test/rolling_blocks_test.cppsrc/pages/rolling-blocks-solver/aStar.tssrc/pages/rolling-blocks-solver/bfs.tssrc/pages/rolling-blocks-solver/rollingBlocksSolver.csssrc/pages/shifting-mosaic-solver/a-star/CMakeLists.txtsrc/pages/shifting-mosaic-solver/a-star/test/CMakeLists.txtsrc/pages/shifting-mosaic-solver/editorHelpers.tssrc/pages/shifting-mosaic-solver/shiftingMosaicSolver.csssrc/pages/shifting-mosaic-solver/shiftingMosaicSolver.tssrc/util/benchShiftingMosaic.tssrc/util/benchWasmSimd.tssrc/util/build.tssrc/util/configFile.tssrc/util/serve.tssrc/util/types.tstest/configFile.test.tstest/phasic-dial-solver/config.test.tstest/phasic-dial-solver/phasicDialSolver.test.tstest/phasic-dial-solver/turnSolver.test.tstest/resources/phasic-dial-solver/phasicDialTest.jsontest/resources/phasic-dial-solver/phasicDialTest1.jsontest/resources/rolling-blocks-solver/rollingBlocksTest.jsontest/resources/rolling-blocks-solver/rollingBlocksTest1.jsontest/resources/rolling-blocks-solver/rollingBlocksTest10.jsontest/resources/rolling-blocks-solver/rollingBlocksTest11.jsontest/resources/rolling-blocks-solver/rollingBlocksTest12.jsontest/resources/rolling-blocks-solver/rollingBlocksTest13.jsontest/resources/rolling-blocks-solver/rollingBlocksTest14.jsontest/resources/rolling-blocks-solver/rollingBlocksTest15.jsontest/resources/rolling-blocks-solver/rollingBlocksTest16.jsontest/resources/rolling-blocks-solver/rollingBlocksTest17.jsontest/resources/rolling-blocks-solver/rollingBlocksTest18.jsontest/resources/rolling-blocks-solver/rollingBlocksTest19.jsontest/resources/rolling-blocks-solver/rollingBlocksTest2.jsontest/resources/rolling-blocks-solver/rollingBlocksTest20.jsontest/resources/rolling-blocks-solver/rollingBlocksTest21.jsontest/resources/rolling-blocks-solver/rollingBlocksTest22.jsontest/resources/rolling-blocks-solver/rollingBlocksTest23.jsontest/resources/rolling-blocks-solver/rollingBlocksTest24.jsontest/resources/rolling-blocks-solver/rollingBlocksTest25.jsontest/resources/rolling-blocks-solver/rollingBlocksTest26.jsontest/resources/rolling-blocks-solver/rollingBlocksTest27.jsontest/resources/rolling-blocks-solver/rollingBlocksTest28.jsontest/resources/rolling-blocks-solver/rollingBlocksTest29.jsontest/resources/rolling-blocks-solver/rollingBlocksTest3.jsontest/resources/rolling-blocks-solver/rollingBlocksTest30.jsontest/resources/rolling-blocks-solver/rollingBlocksTest31.jsontest/resources/rolling-blocks-solver/rollingBlocksTest32.jsontest/resources/rolling-blocks-solver/rollingBlocksTest33.jsontest/resources/rolling-blocks-solver/rollingBlocksTest34.jsontest/resources/rolling-blocks-solver/rollingBlocksTest35.jsontest/resources/rolling-blocks-solver/rollingBlocksTest36.jsontest/resources/rolling-blocks-solver/rollingBlocksTest37.jsontest/resources/rolling-blocks-solver/rollingBlocksTest38.jsontest/resources/rolling-blocks-solver/rollingBlocksTest39.jsontest/resources/rolling-blocks-solver/rollingBlocksTest4.jsontest/resources/rolling-blocks-solver/rollingBlocksTest5.jsontest/resources/rolling-blocks-solver/rollingBlocksTest6.jsontest/resources/rolling-blocks-solver/rollingBlocksTest7.jsontest/resources/rolling-blocks-solver/rollingBlocksTest8.jsontest/resources/rolling-blocks-solver/rollingBlocksTest9.jsontest/resources/shifting-mosaic-solver/shiftingMosaicTest.jsontest/resources/shifting-mosaic-solver/shiftingMosaicTest1.jsontest/resources/shifting-mosaic-solver/shiftingMosaicTest10.jsontest/resources/shifting-mosaic-solver/shiftingMosaicTest11.jsontest/resources/shifting-mosaic-solver/shiftingMosaicTest12.jsontest/resources/shifting-mosaic-solver/shiftingMosaicTest13.jsontest/resources/shifting-mosaic-solver/shiftingMosaicTest14.jsontest/resources/shifting-mosaic-solver/shiftingMosaicTest15.jsontest/resources/shifting-mosaic-solver/shiftingMosaicTest16.jsontest/resources/shifting-mosaic-solver/shiftingMosaicTest17.jsontest/resources/shifting-mosaic-solver/shiftingMosaicTest18.jsontest/resources/shifting-mosaic-solver/shiftingMosaicTest19.jsontest/resources/shifting-mosaic-solver/shiftingMosaicTest2.jsontest/resources/shifting-mosaic-solver/shiftingMosaicTest20.jsontest/resources/shifting-mosaic-solver/shiftingMosaicTest21.jsontest/resources/shifting-mosaic-solver/shiftingMosaicTest22.jsontest/resources/shifting-mosaic-solver/shiftingMosaicTest23.jsontest/resources/shifting-mosaic-solver/shiftingMosaicTest24.jsontest/resources/shifting-mosaic-solver/shiftingMosaicTest25.jsontest/resources/shifting-mosaic-solver/shiftingMosaicTest26.jsontest/resources/shifting-mosaic-solver/shiftingMosaicTest27.jsontest/resources/shifting-mosaic-solver/shiftingMosaicTest28.jsontest/resources/shifting-mosaic-solver/shiftingMosaicTest29.jsontest/resources/shifting-mosaic-solver/shiftingMosaicTest3.jsontest/resources/shifting-mosaic-solver/shiftingMosaicTest30.jsontest/resources/shifting-mosaic-solver/shiftingMosaicTest31.jsontest/resources/shifting-mosaic-solver/shiftingMosaicTest32.jsontest/resources/shifting-mosaic-solver/shiftingMosaicTest33.jsontest/resources/shifting-mosaic-solver/shiftingMosaicTest34.jsontest/resources/shifting-mosaic-solver/shiftingMosaicTest35.jsontest/resources/shifting-mosaic-solver/shiftingMosaicTest36.jsontest/resources/shifting-mosaic-solver/shiftingMosaicTest37.jsontest/resources/shifting-mosaic-solver/shiftingMosaicTest38.jsontest/resources/shifting-mosaic-solver/shiftingMosaicTest39.jsontest/resources/shifting-mosaic-solver/shiftingMosaicTest4.jsontest/resources/shifting-mosaic-solver/shiftingMosaicTest40.jsontest/resources/shifting-mosaic-solver/shiftingMosaicTest41.jsontest/resources/shifting-mosaic-solver/shiftingMosaicTest42.jsontest/resources/shifting-mosaic-solver/shiftingMosaicTest43.jsontest/resources/shifting-mosaic-solver/shiftingMosaicTest5.jsontest/resources/shifting-mosaic-solver/shiftingMosaicTest6.jsontest/resources/shifting-mosaic-solver/shiftingMosaicTest7.jsontest/resources/shifting-mosaic-solver/shiftingMosaicTest8.jsontest/resources/shifting-mosaic-solver/shiftingMosaicTest9.jsontest/rolling-blocks-solver/aStar.test.tstest/rolling-blocks-solver/block.musttouch.test.tstest/rolling-blocks-solver/block.roll.test.tstest/rolling-blocks-solver/block.validity.test.tstest/shifting-mosaic-solver/aStar.collision.test.tstest/shifting-mosaic-solver/aStar.primitives.test.tstest/shifting-mosaic-solver/aStar.search.test.tstest/shifting-mosaic-solver/block.test.tstest/shifting-mosaic-solver/board.geometry.test.tstest/shifting-mosaic-solver/board.pointer.test.tstest/shifting-mosaic-solver/board.render.test.tstest/shifting-mosaic-solver/board.state.test.tstest/shifting-mosaic-solver/config.test.tstest/shifting-mosaic-solver/mem64.node.test.mjstest/shifting-mosaic-solver/portfolio.worker.tstest/shifting-mosaic-solver/wasm.test.ts
💤 Files with no reviewable changes (4)
- e2e/phasicDialSolver.states.test.ts
- src/pages/rolling-blocks-solver/rollingBlocksSolver.css
- src/pages/rolling-blocks-solver/bfs.ts
- src/pages/shifting-mosaic-solver/shiftingMosaicSolver.css
🛑 Comments failed to post (4)
e2e/rolling-blocks-solver/tools.test.ts (1)
800-868: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash # Check whether the rolling-blocks-solver page markup has duplicate id="button" nodes # or whether this locator is likely a typo. fd -a index.html src/pages/rolling-blocks-solver | xargs -I{} sh -c 'echo "--- {} ---"; grep -n "id=\"button\"" {}' rg -n 'id="button"' src/pages/rolling-blocks-solver rg -n 'data-block-delete-id' src/pages/rolling-blocks-solverRepository: Valle12/islands-of-insight-tools
Length of output: 384
🏁 Script executed:
#!/bin/bash set -euo pipefail echo '--- test snippet around the locator ---' sed -n '790,875p' src/e2e/rolling-blocks-solver/tools.test.ts echo '--- rollingBlocksSolver.ts outline ---' ast-grep outline src/pages/rolling-blocks-solver/rollingBlocksSolver.ts --view expanded echo '--- any id="button" in rolling-blocks-solver ---' rg -n 'id="button"|id=\x27button\x27|id=\`button\`' src/pages/rolling-blocks-solver echo '--- delete-button markup ---' rg -n 'data-block-delete-id|button' src/pages/rolling-blocks-solver/rollingBlocksSolver.tsRepository: Valle12/islands-of-insight-tools
Length of output: 297
🏁 Script executed:
#!/bin/bash set -euo pipefail echo '--- matching test files ---' git ls-files | rg 'rolling-blocks-solver|tools\.test\.ts|rollingBlocksSolver\.ts|index\.html$' echo '--- file tree around e2e and src ---' fd -a 'tools.test.ts|rollingBlocksSolver.ts|index.html' . echo '--- test file snippet if present ---' test_file=$(git ls-files | rg '(^|/)tools\.test\.ts$' | head -n 1 || true) if [ -n "${test_file:-}" ]; then echo "--- ${test_file} ---" sed -n '790,875p' "$test_file" fi echo '--- rollingBlocksSolver.ts outline if present ---' solver_file=$(git ls-files | rg 'rollingBlocksSolver\.ts$' | head -n 1 || true) if [ -n "${solver_file:-}" ]; then echo "--- ${solver_file} ---" ast-grep outline "$solver_file" --view expanded fi echo '--- locator-related markup ---' if [ -n "${solver_file:-}" ]; then rg -n 'id="button"|data-block-delete-id|<md-icon-button|<button' "$solver_file" fiRepository: Valle12/islands-of-insight-tools
Length of output: 10370
🏁 Script executed:
#!/bin/bash set -euo pipefail solver_file=src/pages/rolling-blocks-solver/rollingBlocksSolver.ts echo '--- renderToolButtons ---' sed -n '216,279p' "$solver_file" echo '--- renderBlocksList ---' sed -n '279,340p' "$solver_file" echo '--- related template ids/selectors in index.html ---' rg -n 'id="[^"]+"|data-block-delete-id|button|md-icon-button' src/pages/rolling-blocks-solver/index.html src/pages/rolling-blocks-solver/rollingBlocksSolver.tsRepository: Valle12/islands-of-insight-tools
Length of output: 9804
Replace
#buttonwith a stable locator.page.locator("#button").nth(4)is brittle and hard to read; target the actual control by role/name or a dedicated test id 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/rolling-blocks-solver/tools.test.ts` around lines 800 - 868, Replace the brittle page.locator("`#button`").nth(4) lookup with a stable locator for the intended control, using its accessible role and name or a dedicated test ID. Keep the existing click and subsequent aria snapshot unchanged.test/resources/rolling-blocks-solver/rollingBlocksTest32.json (1)
146-161: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Do not initialize blocks on
unplayablecells.Blocks 6 and 7 start at
(0,4)and(0,2), butcells[0][4]andcells[0][2]are both"unplayable". Correct the coordinates or cell map; if invalid boards are intentional, add an explicit rejection test 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 `@test/resources/rolling-blocks-solver/rollingBlocksTest32.json` around lines 146 - 161, Correct the rolling-block test fixture so blocks 6 and 7 do not start on cells marked "unplayable" in the cell map, updating their coordinates or the corresponding cell definitions consistently. If the fixture is intentionally invalid, replace this setup with an explicit test asserting that such a board is rejected.test/shifting-mosaic-solver/aStar.collision.test.ts (1)
58-58: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Correct the test title.
The assertion verifies bounding-box overlap without cell overlap, but the title says the cells overlap.
Proposed fix
- test("L-shape and single cell — bbox overlaps, cells overlap", () => { + test("L-shape and single cell — bbox overlaps, cells do not overlap", () => {📝 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("L-shape and single cell — bbox overlaps, cells do not overlap", () => {🤖 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/shifting-mosaic-solver/aStar.collision.test.ts` at line 58, Correct the test title in the L-shape and single-cell collision test to state that the bounding boxes overlap without the individual cells overlapping; leave the test assertions and behavior unchanged.test/shifting-mosaic-solver/aStar.search.test.ts (1)
236-247: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Validate replayed turns, not only the final anchor.
Line 236 can return an illegal sequence and still pass:
applyTurnsignores collisions and bounds, so the goal block could move through the obstacle and still end at{ x: 0, y: 0 }. Validate every replayed transition against board rules before asserting the final 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 `@test/shifting-mosaic-solver/aStar.search.test.ts` around lines 236 - 247, Strengthen the replay validation in the aStar.search test around applyTurns so every turn is checked for legal bounds and collision-free transitions according to the board rules, rather than relying only on the final anchor assertion. Preserve the existing final-anchor verification after validating the complete sequence returned by aStar.search().
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Bug Fixes
Tests