Repository navigation
IIT-26-UI-refactorings - #36
Conversation
Phasic dial: a dial is now an SVG polygon with `max + 1` corners and a pointer you drag or arrow-key round it, so the shape and the starting position are what the game shows instead of two raw number fields. The page keeps a real model and renders from it rather than scraping the DOM, which is what makes dragging possible and what lets one `invalidateResult()` drop a stale answer on any edit. Buttons became cards that stack a dial icon per turn — the way the game's own button panels read — and carry their own press count once solved. Reset and help moved out of the header into one action row. The config format is untouched: `maxValues[i]` is still positions minus one. Rolling blocks: solving opens a step view over the editor showing the board before each roll, with the moving block ringed and its landing footprint outlined, next to the full move list. That needed a TypeScript replay, which did not exist — `replay.ts` mirrors `a-star/Replay.cpp`, and `aStar.slow.test.ts` now validates all 48 wasm answers through it instead of carrying its own copy. Two traps worth naming. Only `body` is border-box on the dial page, so a 148px `.dial-card` is really 170px — `layoutDials()` measures rather than repeating stylesheet numbers, after a hardcoded track width made the cards overlap. And both grids balance their rows through a computed column count, because `flex-wrap` fills each row to capacity and dumps the remainder on the last one. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe PR replaces the phasic-dial table UI with state-driven dial and button cards. It adds rolling-blocks replay and step-by-step solution views. It also extracts shared grid-outline helpers and updates unit, integration, and E2E coverage. ChangesPhasic dial interface
Rolling-blocks solution replay
Shared grid outlines
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant RollingBlocksSolver
participant SolutionView
participant replayTurns
participant SolutionBoard
User->>RollingBlocksSolver: Calculate solution
RollingBlocksSolver->>replayTurns: Replay solution turns
RollingBlocksSolver->>SolutionView: Open stored solution path
SolutionView->>SolutionBoard: Render replay state
User->>SolutionView: Select or navigate to a move
SolutionView->>SolutionBoard: Render selected step and highlights
User->>RollingBlocksSolver: Return to editor
RollingBlocksSolver->>SolutionView: Dispose active view
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 (10)
src/pages/rolling-blocks-solver/solutionView.ts (1)
72-73: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
Array.prototype.at(-1)for the final step.SonarCloud flags the index arithmetic.
at(-1)states the intent directly.♻️ Proposed change
- const last = steps[steps.length - 1]!; + const last = steps.at(-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 `@src/pages/rolling-blocks-solver/solutionView.ts` around lines 72 - 73, Update the final-step lookup in the solution view initialization to use steps.at(-1) instead of steps[steps.length - 1], while preserving the existing non-null handling and solvedAtEnd calculation.Source: Linters/SAST tools
test/rolling-blocks-solver/replay.test.ts (1)
13-32: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider sharing the
gridhelper.
test/rolling-blocks-solver/solutionView.test.tslines 18-32 define an identicalgridhelper. Move it to a shared test fixture module and import it in both files.🤖 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/replay.test.ts` around lines 13 - 32, Move the duplicated grid helper into a shared test fixture module, preserving its column-major initialization and paint overlay behavior. Remove the local grid definitions from replay.test.ts and solutionView.test.ts, then import the shared grid helper in both files.src/pages/rolling-blocks-solver/index.html (1)
187-187: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider making the move rows keyboard reachable.
The
lirows in#solution-movesrespond to click only. Keyboard users can still navigate with Previous/Next and arrow keys, so this is not a blocker. If you want row selection by keyboard, render each row as abuttoninside theli, or addtabindex="0"plus an Enter/Space handler.🤖 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/index.html` at line 187, Update the solution-move rendering associated with `#solution-moves` so each clickable move row is keyboard reachable and selectable, preferably by rendering a button inside each li. Ensure Enter and Space activate the same selection behavior as clicking while preserving the existing Previous/Next and arrow-key controls.src/pages/phasic-dial-solver/phasicDialSolver.css (2)
267-269: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider scoping the unused-card dimming.
opacity: 0.6dims the whole.button-card, including the.turn-inputfield and the stepper controls. The card stays fully editable after a solve, so the user edits a control that renders at reduced contrast. The.button-pressesbadge already signals the unused state through its own colour change at lines 291-294, so the card-wide opacity is a second signal on top of it.Applying the opacity to
.button-card-headeronly, or reducing it to a subtler value, keeps the signal and leaves the editable controls at full contrast.🤖 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.css` around lines 267 - 269, Scope the unused-state opacity in the .button-card[data-unused="true"] rule to the non-interactive .button-card-header instead of the entire card, preserving full contrast for .turn-input and stepper controls while retaining the unused-state visual signal.
127-144: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the dial palette into custom properties.
The six accent hex values appear twice: once for
.dial-card[data-color]and once for.turn-slot[data-color]. A palette change requires twelve edits, and the two lists can drift apart.♻️ Proposed refactor
Define the palette once next to
--dial-solved::root { /* The blue the game lights a dial up in once its pointer aims at the hub. */ --dial-solved: `#1f97cf`; + + --dial-blue: `#4f6bd9`; + --dial-red: `#e0567c`; + --dial-green: `#35b784`; + --dial-yellow: `#d9a72e`; + --dial-cyan: `#3fbdd8`; + --dial-purple: `#9b6be0`; }Then reference it from both blocks, for example:
.dial-card[data-color="blue"] { - --dial-accent: `#4f6bd9`; + --dial-accent: var(--dial-blue); }.turn-slot[data-color="blue"] { - --dial-accent: `#4f6bd9`; + --dial-accent: var(--dial-blue); }A single selector list covering both, such as
.dial-card[data-color="blue"], .turn-slot[data-color="blue"], is another option.Also applies to: 316-333
🤖 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.css` around lines 127 - 144, Extract the six dial accent colors into shared custom properties near --dial-solved, then update both the .dial-card[data-color] and .turn-slot[data-color] selector blocks to reference those palette variables instead of duplicating hex values. Preserve the existing color-to-accent mappings and ensure both components use the same definitions.src/pages/phasic-dial-solver/buttonsView.ts (1)
149-160: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider making the view read-only over
buttons.
renderstores the caller'sbuttonsarray by reference.commitwritesrow[dial] = turns, and thenonTurnsChangewrites the same cell again inphasicDialSolver.tsat line 64. The two writes agree today, so there is no defect. Two owners of one array is the risk: a later change to either write path can make the view and the solver disagree.One option is to drop the write in
commitand let the solver remain the single owner of the state.♻️ Proposed refactor
const row = this.buttons[button]; if (!row || dial >= row.length) return; - row[dial] = turns; this.paintIcons(slot, dial, turns);
onClickreadsthis.buttons[button]?.[dial]to compute the next step value, so the solver callback must write the cell before the next click. It already does.🤖 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/buttonsView.ts` around lines 149 - 160, Make the view read-only over the caller-owned buttons state by removing the row[dial] = turns mutation from commit, while preserving the existing onTurnsChange(button, dial, turns) callback so the solver remains the single state owner and subsequent onClick reads receive updated values.test/phasic-dial-solver/phasicDialSolver.test.ts (2)
435-435: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRename the test to match the file convention.
Every other test in this suite starts with "should". Use the same form here.
♻️ Proposed rename
- test("test help dialog", () => { + test("should open and close the help dialog", () => {🤖 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` at line 435, Rename the test case currently titled "test help dialog" to a description beginning with "should", matching the naming convention used by the other tests in the suite. Keep the test implementation unchanged.
563-566: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winReplace the fixed microtask count with the existing
flushhelper.
pickawaits exactly three microtasks. The number is tied to the current internalawaitchain ofloadConfigFromFile. If that chain gains one step, or if the file read resolves on a macrotask, the assertions afterpickrun against stale DOM state. The rest of this suite already settles async work withflush().♻️ Proposed change
input.dispatchEvent(new Event("change")); - // loadConfigFromFile is async; let its promise chain settle. - await Promise.resolve(); - await Promise.resolve(); - await Promise.resolve(); + // loadConfigFromFile is async; let its promise chain 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 563 - 566, Replace the three explicit Promise.resolve awaits after pick in the phasic dial solver test with the suite’s existing flush helper, ensuring all asynchronous work and DOM updates settle before assertions run.e2e/phasic-dial-solver/config.test.ts (1)
172-175: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAssert the badge shape on every card, not only the first.
The count check confirms 8 badges exist. The text check covers 1 of them. Playwright can assert an array of texts against the whole locator, which keeps the distribution unpinned and still verifies each badge.
♻️ Proposed change
await expect(page.locator(".button-presses")).toHaveCount(8); - await expect(page.locator(".button-presses").first()).toHaveText( - /press ×\d+|not needed/, - ); + await expect(page.locator(".button-presses")).toHaveText( + Array.from({ length: 8 }, () => /press ×\d+|not needed/), + );🤖 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 - 175, Update the assertions in the phasic dial solver test to validate the badge text pattern for every element matched by the ".button-presses" locator, rather than only its first element. Keep the existing count assertion and preserve the allowed “press ×N” or “not needed” formats without constraining badge distribution.e2e/phasic-dial-solver/states.test.ts (1)
12-16: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueShare
setTurnsbetween the phasic-dial e2e specs.
setTurnshere is identical tosetTurnsine2e/phasic-dial-solver/solvable.test.tslines 30-34. Move it to a shared helper module undere2e/phasic-dial-solver/and import it in both specs. The dial-aiming helpers overlap 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 `@e2e/phasic-dial-solver/states.test.ts` around lines 12 - 16, Move the duplicate setTurns helper into a shared module under the phasic-dial-solver test directory, then import and use it in both states.test.ts and solvable.test.ts. Also identify the overlapping dial-aiming helpers in both specs and move them into the same shared module, updating both specs to import the shared implementations.
🤖 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/solvable.test.ts`:
- Around line 47-48: Replace every Material Web shadow-DOM `#button` selector with
accessible role-based locators or application-specific hooks. In
e2e/phasic-dial-solver/solvable.test.ts at lines 47-48, 71-73, and 80-82, update
the add-button and add-dial clicks; in e2e/phasic-dial-solver/states.test.ts at
lines 43-54 and 97-101, update all listed add-dial and add-button clicks. Use
the controls’ accessible names, such as getByRole("button", { name: ... }), and
remove all `#button` selector usage.
In `@e2e/rolling-blocks-solver/solution.test.ts`:
- Around line 4-5: Update the doc comment for buildPuzzle to describe only that
it builds the 9x9 board with one goal and one 2-high block; remove the claim
that it solves the puzzle, since setupSolvedPuzzle performs the solve.
In `@src/pages/phasic-dial-solver/dialView.ts`:
- Around line 255-263: Introduce a shared upper-bound constant for dial maxima,
enforce it in config validation and the changeMax method so values cannot exceed
the bound, and preserve clamping of the current value. Update the render logic
for the .dial-sides-more control to disable it when the current max reaches the
bound, while keeping the existing lower-bound behavior unchanged.
In `@src/pages/phasic-dial-solver/index.html`:
- Line 42: Update src/pages/phasic-dial-solver/index.html lines 42-42 by adding
role="alert" to `#warning-banner`, and lines 123-123 by adding aria-live="polite"
to `#result` so dynamically rendered warning and solve-result messages are
announced by screen readers.
- Line 123: Add an appropriate aria-live attribute to the `#result` element so
screen readers announce content written by renderResult after
calculateTurnsAsync completes, while preserving its existing hidden behavior.
In `@src/pages/phasic-dial-solver/phasicDialSolver.css`:
- Around line 5-8: Update the light-mode --dial-solved CSS custom property in
:root to a darker blue that meets WCAG AA contrast against `#ece6f0` for the 12px
text, while leaving the dark-mode value unchanged.
In `@test/phasic-dial-solver/layout.test.ts`:
- Around line 55-63: Replace the tautological assertion in the “leaves the last
row no fuller than the ones above it” test with an assertion that
balancedColumns(count, capacity) produces the minimum required number of rows,
calculated as ceil(count / capacity). Keep the existing count and capacity
coverage and verify the returned column count yields that row count.
---
Nitpick comments:
In `@e2e/phasic-dial-solver/config.test.ts`:
- Around line 172-175: Update the assertions in the phasic dial solver test to
validate the badge text pattern for every element matched by the
".button-presses" locator, rather than only its first element. Keep the existing
count assertion and preserve the allowed “press ×N” or “not needed” formats
without constraining badge distribution.
In `@e2e/phasic-dial-solver/states.test.ts`:
- Around line 12-16: Move the duplicate setTurns helper into a shared module
under the phasic-dial-solver test directory, then import and use it in both
states.test.ts and solvable.test.ts. Also identify the overlapping dial-aiming
helpers in both specs and move them into the same shared module, updating both
specs to import the shared implementations.
In `@src/pages/phasic-dial-solver/buttonsView.ts`:
- Around line 149-160: Make the view read-only over the caller-owned buttons
state by removing the row[dial] = turns mutation from commit, while preserving
the existing onTurnsChange(button, dial, turns) callback so the solver remains
the single state owner and subsequent onClick reads receive updated values.
In `@src/pages/phasic-dial-solver/phasicDialSolver.css`:
- Around line 267-269: Scope the unused-state opacity in the
.button-card[data-unused="true"] rule to the non-interactive .button-card-header
instead of the entire card, preserving full contrast for .turn-input and stepper
controls while retaining the unused-state visual signal.
- Around line 127-144: Extract the six dial accent colors into shared custom
properties near --dial-solved, then update both the .dial-card[data-color] and
.turn-slot[data-color] selector blocks to reference those palette variables
instead of duplicating hex values. Preserve the existing color-to-accent
mappings and ensure both components use the same definitions.
In `@src/pages/rolling-blocks-solver/index.html`:
- Line 187: Update the solution-move rendering associated with `#solution-moves`
so each clickable move row is keyboard reachable and selectable, preferably by
rendering a button inside each li. Ensure Enter and Space activate the same
selection behavior as clicking while preserving the existing Previous/Next and
arrow-key controls.
In `@src/pages/rolling-blocks-solver/solutionView.ts`:
- Around line 72-73: Update the final-step lookup in the solution view
initialization to use steps.at(-1) instead of steps[steps.length - 1], while
preserving the existing non-null handling and solvedAtEnd calculation.
In `@test/phasic-dial-solver/phasicDialSolver.test.ts`:
- Line 435: Rename the test case currently titled "test help dialog" to a
description beginning with "should", matching the naming convention used by the
other tests in the suite. Keep the test implementation unchanged.
- Around line 563-566: Replace the three explicit Promise.resolve awaits after
pick in the phasic dial solver test with the suite’s existing flush helper,
ensuring all asynchronous work and DOM updates settle before assertions run.
In `@test/rolling-blocks-solver/replay.test.ts`:
- Around line 13-32: Move the duplicated grid helper into a shared test fixture
module, preserving its column-major initialization and paint overlay behavior.
Remove the local grid definitions from replay.test.ts and solutionView.test.ts,
then import the shared grid helper in both files.
🪄 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: 4e076772-f05b-4c22-b975-c9ce23b433e9
📒 Files selected for processing (25)
CLAUDE.mde2e/phasic-dial-solver/config.test.tse2e/phasic-dial-solver/solvable.test.tse2e/phasic-dial-solver/states.test.tse2e/rolling-blocks-solver/solution.test.tssrc/common/common.csssrc/pages/phasic-dial-solver/buttonsView.tssrc/pages/phasic-dial-solver/dialView.tssrc/pages/phasic-dial-solver/index.htmlsrc/pages/phasic-dial-solver/layout.tssrc/pages/phasic-dial-solver/phasicDialSolver.csssrc/pages/phasic-dial-solver/phasicDialSolver.tssrc/pages/rolling-blocks-solver/index.htmlsrc/pages/rolling-blocks-solver/replay.tssrc/pages/rolling-blocks-solver/rollingBlocksSolver.csssrc/pages/rolling-blocks-solver/rollingBlocksSolver.tssrc/pages/rolling-blocks-solver/solutionView.tssrc/pages/shifting-mosaic-solver/solutionView.tssrc/util/gridOutline.tstest/phasic-dial-solver/dialView.test.tstest/phasic-dial-solver/layout.test.tstest/phasic-dial-solver/phasicDialSolver.test.tstest/rolling-blocks-solver/aStar.slow.test.tstest/rolling-blocks-solver/replay.test.tstest/rolling-blocks-solver/solutionView.test.ts
| await page.locator("#add-button #button").click(); | ||
| await page.locator("#add-button #button").click(); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Replace every #button shadow-DOM selector in the phasic-dial e2e specs. The specs click the internal #button node of Material Web icon buttons. The coding guidelines forbid this selector form. Both specs already use getByRole("button", { name: ... }) elsewhere, so switch each site to an accessible name or an application-specific hook.
e2e/phasic-dial-solver/solvable.test.ts#L47-L48: replace the two#add-button#button`` clicks with the accessible "add button" control.e2e/phasic-dial-solver/solvable.test.ts#L71-L73: replace the#add-dial#button`` click inside the loop.e2e/phasic-dial-solver/solvable.test.ts#L80-L82: replace the#add-button#button`` click inside the loop.e2e/phasic-dial-solver/states.test.ts#L43-L54: replace the#add-dial#buttonand `#add-button `#buttonclicks.e2e/phasic-dial-solver/states.test.ts#L97-L101: replace the two#add-dial#buttonclicks and the `#add-button `#buttonclick inside the loop.
As per coding guidelines: "Do not target Material Web buttons by shadow-DOM #button index; use application-specific hooks such as data-block-delete-id."
📍 Affects 2 files
e2e/phasic-dial-solver/solvable.test.ts#L47-L48(this comment)e2e/phasic-dial-solver/solvable.test.ts#L71-L73e2e/phasic-dial-solver/solvable.test.ts#L80-L82e2e/phasic-dial-solver/states.test.ts#L43-L54e2e/phasic-dial-solver/states.test.ts#L97-L101
🤖 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 47 - 48, Replace every
Material Web shadow-DOM `#button` selector with accessible role-based locators or
application-specific hooks. In e2e/phasic-dial-solver/solvable.test.ts at lines
47-48, 71-73, and 80-82, update the add-button and add-dial clicks; in
e2e/phasic-dial-solver/states.test.ts at lines 43-54 and 97-101, update all
listed add-dial and add-button clicks. Use the controls’ accessible names, such
as getByRole("button", { name: ... }), and remove all `#button` selector usage.
Source: Coding guidelines
| /** Builds a 9x9 board with one goal and one 2-high block, and solves it. */ | ||
| async function buildPuzzle(page: Page) { |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Fix the stale doc comment on buildPuzzle.
buildPuzzle now stops after the board setup. setupSolvedPuzzle performs the solve. Remove the "and solves it" clause.
📝 Proposed fix
-/** Builds a 9x9 board with one goal and one 2-high block, and solves it. */
+/** Builds a 9x9 board with one goal and one 2-high block. */
async function buildPuzzle(page: Page) {📝 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.
| /** Builds a 9x9 board with one goal and one 2-high block, and solves it. */ | |
| async function buildPuzzle(page: Page) { | |
| /** Builds a 9x9 board with one goal and one 2-high block. */ | |
| async function buildPuzzle(page: Page) { |
🤖 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 4 - 5, Update the
doc comment for buildPuzzle to describe only that it builds the 9x9 board with
one goal and one 2-high block; remove the claim that it solves the puzzle, since
setupSolvedPuzzle performs the solve.
| private changeMax(max: number) { | ||
| // Max is "positions minus one", so 0 would be an inert one-position dial — | ||
| // the same floor the config validator enforces. | ||
| if (max < 1 || max === this.options.max) return; | ||
| this.options.max = max; | ||
| if (this.options.value > max) this.options.value = max; | ||
| this.render(); | ||
| this.options.onMaxChange(max, this.options.value); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect the phasic-dial config validator for value bounds.
fd -t f 'config.ts' -p 'phasic-dial-solver' --exec cat -n {}Repository: Valle12/islands-of-insight-tools
Length of output: 276
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- candidate files ---'
git ls-files | grep -E '(^|/)(config\.ts|dialView\.ts|phasicDialSolver\.ts)$' || true
printf '%s\n' '--- config references ---'
rg -n --glob '*.{ts,tsx,js,jsx}' 'maxValues|MAX_DIALS|MIN_DIALS|changeMax|dial-sides-more|dial-sides-fewer' src
printf '%s\n' '--- dialView outline ---'
ast-grep outline src/pages/phasic-dial-solver/dialView.ts --view compact || true
printf '%s\n' '--- relevant dialView code ---'
sed -n '220,285p' src/pages/phasic-dial-solver/dialView.tsRepository: Valle12/islands-of-insight-tools
Length of output: 7179
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- candidate files ---'
git ls-files | grep -E '(^|/)(config\.ts|dialView\.ts|phasicDialSolver\.ts)$' || true
printf '%s\n' '--- config references ---'
rg -n --glob '*.{ts,tsx,js,jsx}' 'maxValues|MAX_DIALS|MIN_DIALS|changeMax|dial-sides-more|dial-sides-fewer' src
printf '%s\n' '--- dialView code ---'
sed -n '220,285p' src/pages/phasic-dial-solver/dialView.tsRepository: Valle12/islands-of-insight-tools
Length of output: 6994
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- phasic-dial config ---'
cat -n src/pages/phasic-dial-solver/config.ts | sed -n '1,80p'
printf '%s\n' '--- dialView controls and render ---'
cat -n src/pages/phasic-dial-solver/dialView.ts | sed -n '100,190p'
cat -n src/pages/phasic-dial-solver/dialView.ts | sed -n '285,345p'
printf '%s\n' '--- solver update and solve paths ---'
cat -n src/pages/phasic-dial-solver/phasicDialSolver.ts | sed -n '135,180p'
cat -n src/pages/phasic-dial-solver/phasicDialSolver.ts | sed -n '240,345p'
printf '%s\n' '--- TurnSolver implementation ---'
cat -n src/pages/phasic-dial-solver/turnSolver.ts | sed -n '1,120p'Repository: Valle12/islands-of-insight-tools
Length of output: 18149
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- cartesianProduct implementation ---'
rg -n -A45 -B5 'function cartesianProduct|export .*cartesianProduct|cartesianProduct' src/util src/pages/phasic-dial-solver
printf '%s\n' '--- button validation ---'
cat -n src/pages/phasic-dial-solver/config.ts | sed -n '74,125p'
printf '%s\n' '--- standalone growth check ---'
python3 - <<'PY'
from math import gcd, lcm, prod
def cycle_length(max_value, turn):
modulus = max_value + 1
step = turn % modulus
return modulus // gcd(modulus, step or modulus)
for max_value in (1, 9, 99, 999):
limits = [cycle_length(max_value, 1)] * 4
print(f"max={max_value}: dots={max_value + 1}, four-button combination upper bound={prod(limits)}")
PYRepository: Valle12/islands-of-insight-tools
Length of output: 11195
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- cartesianProduct implementation ---'
rg -n -A45 -B5 'function cartesianProduct|export .*cartesianProduct|cartesianProduct' src/util src/pages/phasic-dial-solver
printf '%s\n' '--- button validation ---'
cat -n src/pages/phasic-dial-solver/config.ts | sed -n '74,125p'
printf '%s\n' '--- standalone growth check ---'
python3 - <<'PY'
from math import gcd, prod
def cycle_length(max_value, turn):
modulus = max_value + 1
step = turn % modulus
return modulus // gcd(modulus, step or modulus)
for max_value in (1, 9, 99, 999):
limits = [cycle_length(max_value, 1)] * 4
print(f"max={max_value}: dots={max_value + 1}, four-button combination upper bound={prod(limits)}")
PYRepository: Valle12/islands-of-insight-tools
Length of output: 11195
Bound max and disable the “more” control at the bound. config.ts enforces only maxValues[i] >= 1. changeMax can therefore increase sides without a practical limit. Each redraw creates max + 1 dots, and TurnSolver enumerates a Cartesian product whose cycle lengths grow with max + 1. Define a shared upper bound, enforce it in changeMax, and disable .dial-sides-more at that bound.
🤖 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/dialView.ts` around lines 255 - 263, Introduce a
shared upper-bound constant for dial maxima, enforce it in config validation and
the changeMax method so values cannot exceed the bound, and preserve clamping of
the current value. Update the render logic for the .dial-sides-more control to
disable it when the current max reaches the bound, while keeping the existing
lower-bound behavior unchanged.
| turns, then calculate the presses that aim them all at the hub. | ||
| </p> | ||
|
|
||
| <div id="warning-banner" class="hidden"></div> |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Script-populated regions in the editor card carry no live-region semantics. showWarning and renderResult both write text into a hidden container and then reveal it. Neither container declares role="alert" or aria-live, so a screen reader announces no dynamic update. The warning case is the more serious one, because showWarning hides the banner again after 3500 ms and the message cannot be found afterwards.
src/pages/phasic-dial-solver/index.html#L42-L42: addrole="alert"to#warning-bannerso a failed config load is announced.src/pages/phasic-dial-solver/index.html#L123-L123: addaria-live="polite"to#resultso the asynchronous solve outcome is announced.
📍 Affects 1 file
src/pages/phasic-dial-solver/index.html#L42-L42(this comment)src/pages/phasic-dial-solver/index.html#L123-L123
🤖 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 42, Update
src/pages/phasic-dial-solver/index.html lines 42-42 by adding role="alert" to
`#warning-banner`, and lines 123-123 by adding aria-live="polite" to `#result` so
dynamically rendered warning and solve-result messages are announced by screen
readers.
| </thead> | ||
| <tbody id="table-body"></tbody> | ||
| </table> | ||
| <div id="result" hidden></div> |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Announce the result when the search finishes.
renderResult writes the answer into #result and clears hidden after calculateTurnsAsync resolves. The element has no aria-live, so a screen reader announces nothing when the asynchronous result arrives. The user pressed Calculate and receives no confirmation that a result exists.
🛡️ Proposed fix
- <div id="result" hidden></div>
+ <div id="result" hidden aria-live="polite"></div>📝 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.
| <div id="result" hidden></div> | |
| <div id="result" hidden aria-live="polite"></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 123, Add an appropriate
aria-live attribute to the `#result` element so screen readers announce content
written by renderResult after calculateTurnsAsync completes, while preserving
its existing hidden behavior.
| :root { | ||
| /* The blue the game lights a dial up in once its pointer aims at the hub. */ | ||
| --dial-solved: #1f97cf; | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Resolve the Material surface-container-high token values used behind .dial-state.
fd -t f -e css . src --exec rg -n 'surface-container-high|--md-sys-color-surface-container' {} \; | head -50Repository: Valle12/islands-of-insight-tools
Length of output: 2921
🏁 Script executed:
#!/bin/bash
set -eu
python3 - <<'PY'
def srgb_to_linear(channel):
channel /= 255
return channel / 12.92 if channel <= 0.04045 else ((channel + 0.055) / 1.055) ** 2.4
def luminance(hex_color):
rgb = [int(hex_color[i:i+2], 16) for i in (1, 3, 5)]
r, g, b = (srgb_to_linear(value) for value in rgb)
return 0.2126 * r + 0.7152 * g + 0.0722 * b
def contrast(foreground, background):
foreground_l = luminance(foreground)
background_l = luminance(background)
return (max(foreground_l, background_l) + 0.05) / (min(foreground_l, background_l) + 0.05)
cases = [
("light", "`#1f97cf`", "`#ece6f0`"),
("dark", "`#7fd7ff`", "`#2b2930`"),
]
for mode, foreground, background in cases:
print(f"{mode}: {foreground} on {background}: {contrast(foreground, background):.3f}:1")
PY
printf '\nToken and selector locations:\n'
rg -n -C 3 --glob '*.css' -- '--md-sys-color-surface-container-high|\.dial-state|--dial-solved' srcRepository: Valle12/islands-of-insight-tools
Length of output: 13262
Darken the light-mode --dial-solved color.
#1f97cf on #ece6f0 has a 2.685:1 contrast ratio and fails WCAG AA for 12px text. The dark-mode value passes at 8.934: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 `@src/pages/phasic-dial-solver/phasicDialSolver.css` around lines 5 - 8, Update
the light-mode --dial-solved CSS custom property in :root to a darker blue that
meets WCAG AA contrast against `#ece6f0` for the 12px text, while leaving the
dark-mode value unchanged.
| test("leaves the last row no fuller than the ones above it", () => { | ||
| for (let count = 1; count <= 12; count++) { | ||
| for (let capacity = 1; capacity <= 6; capacity++) { | ||
| const cols = balancedColumns(count, capacity); | ||
| const last = count % cols === 0 ? cols : count % cols; | ||
| expect(last).toBeLessThanOrEqual(cols); | ||
| } | ||
| } | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
This assertion cannot fail.
last is assigned either cols or count % cols. A remainder is always strictly less than its divisor, so count % cols < cols always holds, and the other branch is cols itself. expect(last).toBeLessThanOrEqual(cols) is therefore true for every value balancedColumns could return, including a wrong one. The test gives no coverage of the property named in its title.
The invariant the title describes is that balancing does not add a row: the number of rows used with cols columns must equal the minimum number of rows, ceil(count / capacity).
💚 Proposed fix
test("leaves the last row no fuller than the ones above it", () => {
for (let count = 1; count <= 12; count++) {
for (let capacity = 1; capacity <= 6; capacity++) {
const cols = balancedColumns(count, capacity);
- const last = count % cols === 0 ? cols : count % cols;
- expect(last).toBeLessThanOrEqual(cols);
+ // Balancing must not cost an extra row: the row count with `cols`
+ // columns has to match the fewest rows `capacity` allows.
+ const rows = Math.ceil(count / cols);
+ expect(rows).toBe(Math.ceil(count / capacity));
}
}
});📝 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("leaves the last row no fuller than the ones above it", () => { | |
| for (let count = 1; count <= 12; count++) { | |
| for (let capacity = 1; capacity <= 6; capacity++) { | |
| const cols = balancedColumns(count, capacity); | |
| const last = count % cols === 0 ? cols : count % cols; | |
| expect(last).toBeLessThanOrEqual(cols); | |
| } | |
| } | |
| }); | |
| test("leaves the last row no fuller than the ones above it", () => { | |
| for (let count = 1; count <= 12; count++) { | |
| for (let capacity = 1; capacity <= 6; capacity++) { | |
| const cols = balancedColumns(count, capacity); | |
| // Balancing must not cost an extra row: the row count with `cols` | |
| // columns has to match the fewest rows `capacity` allows. | |
| const rows = Math.ceil(count / cols); | |
| expect(rows).toBe(Math.ceil(count / capacity)); | |
| } | |
| } | |
| }); |
🤖 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/layout.test.ts` around lines 55 - 63, Replace the
tautological assertion in the “leaves the last row no fuller than the ones above
it” test with an assertion that balancedColumns(count, capacity) produces the
minimum required number of rows, calculated as ceil(count / capacity). Keep the
existing count and capacity coverage and verify the returned column count yields
that row count.
Six of the seven are taken as written. - The phasic e2e specs no longer reach into Material's shadow DOM for `#button`. Every control they clicked now has an accessible name, so `getByRole` reaches it directly. - `buildPuzzle` no longer claims to solve the puzzle; `setupSolvedPuzzle` does that. - `#warning-banner` is a `role="alert"` and `#result` is `aria-live="polite"`. The banner is the worse of the two: it hides itself after 3500 ms, so an unannounced message is simply lost. - Light-mode `--dial-solved` goes from #1f97cf to #0d6e99. It is text as well as a border — `.dial-state` reads "Aimed at the hub" in it at 12px — and 2.7:1 on the card behind it failed WCAG AA. Measured in the browser at 4.62:1. - `layout.test.ts` asserted `count % cols <= cols`, which no return value could fail. It now asserts the property its title claims: balancing moves items into an earlier row, it never opens another one. - The `+` under a dial stops at 12 positions, twice the biggest shape the game has. Every position added widens the cartesian product TurnSolver enumerates, and nothing bounded the click path. The seventh, enforcing that same ceiling in `config.ts`, is deliberately not taken: the validator has no upper bound BY DESIGN, so a hand-written puzzle with a bigger dial still loads and draws rather than being refused. The bound belongs to the control, and a test pins that a loaded 21-position dial renders with `+` disabled and `−` still live. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Summary by CodeRabbit
New Features
Documentation
Bug Fixes