Guarantee solvable boards (+ order-strategy seam, explainer & difficulty review)#45
Merged
Merged
Conversation
Randomly filling the turtle layout frequently produced unsolvable boards, so whether a game could be won at all was partly luck. Build boards by recording a valid solution instead of dealing and hoping. - board-rules.ts: extract the geometric freeness rule (isSelectable) into a shared pure function, reused by the store's allowedforSelection so the game and generator can never disagree about what is reachable. - init-gameboard.ts: peel the layout into a valid removal order, then lay matched token pairs onto it. Replaying the order wins, so every board is solvable by construction for any shuffle. Verified by re-solving deals. - order-strategies.ts: ports & adapters seam for the peel-order policy (topDownRandom, scatter [default], original, bottomUpRandom; deterministic canonical fallback). scatter pairs the highest free tile with the lowest, spreading matches across layers to avoid pre-stacked matching pairs at the top of the pyramid. Selectable via env / localStorage / runtime override. - game-store: allowedforSelection delegates to the shared rule; add redealCurrentLevel to re-deal the current level on demand.
Dev-only badge on /play to compare peel strategies live: s / Shift+S to cycle, r to re-deal the same strategy, hover for descriptions. Hidden in production builds; the selection persists via localStorage.
- solvable-boards.html: animated explainer contrasting random fill (often unsolvable) with solvability-by-construction; references docs/gifs. - gifs/: short looping clips rendered from the real algorithms. - strategy-difficulty-report.html: measured difficulty proxies (branching factor, forced-move rate, copy-spread) across strategies, for game-design review of the difficulty/look trade-off.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
|
Warning Review limit reached
Your plan includes 1 review of capacity. Refill in 43 minutes and 17 seconds. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more review capacity refills, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than trial, open-source, and free plans. In all cases, review capacity refills continuously over time. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughSolvable board generation added via pluggable order strategies, geometric selectability rules, deterministic shuffling, store persistence of strategy, a dev-only StrategySwitcher UI with keyboard controls, tests for strategies and generation, UI/type/DB changes to persist/display strategy, and supporting documentation. ChangesSolvability-by-Construction with Order Strategies
Sequence DiagramsequenceDiagram
participant Dev as Developer (keyboard/console)
participant UI as StrategySwitcher
participant API as setOrderStrategy/getOrderStrategy
participant Store as GameStore (redealCurrentLevel)
participant Generator as dealSolvableBoard
participant Strategy as OrderStrategy.peel()
participant Fallback as canonical.peel()
participant Shuffle as shuffleInPlace
Dev->>UI: press 's' / 'Shift+S' / 'r' or call window.eurekaOrder
UI->>API: setOrderStrategy(name)
API->>API: persist runtime override/localStorage (dev)
UI->>Store: redealCurrentLevel()
Store->>Generator: initializeGameBoard(level, strategy)
Generator->>Strategy: strategy.peel(random) attempts
alt strategy returns null (strand)
Generator->>Fallback: canonical.peel()
Fallback-->>Generator: RemovalOrder
else success
Strategy-->>Generator: RemovalOrder
end
Generator->>Shuffle: shuffleInPlace(tokens, random)
Shuffle-->>Generator: shuffled tokens
Generator-->>Store: new GameBoard + solution
Store-->>UI: updated board (animations)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 2
🤖 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 `@src/zustand/game-store.ts`:
- Around line 334-336: Remove the assignment to shouldAnimateOnMount from
redealCurrentLevel(): currently redealCurrentLevel() sets both boardGeneration
and shouldAnimateOnMount; change it to only update boardGeneration (and any
other necessary state) and do not touch shouldAnimateOnMount there so the flag
remains controlled exclusively by start() and levelCleared() flows; locate
redealCurrentLevel in the zustand store and remove the shouldAnimateOnMount
update while leaving boardGeneration: Date.now() unchanged.
- Around line 331-337: The redealCurrentLevel function replaces gameBoard but
doesn’t prevent previously scheduled animation timeouts (from functions like
clicked) from mutating the new board; modify redealCurrentLevel to either
increment/update the shared boardGeneration and ensure all animation callbacks
check that captured boardGeneration matches the current one before mutating
state, or track and clear outstanding timeout IDs before re-dealing; update the
animation callback in clicked (or wherever setTimeout is used) to capture the
boardGeneration value and no-op if it mismatches to prevent stale timeouts from
altering the new board.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e448c107-c253-4ea4-a8d7-676ca0ac0c54
⛔ Files ignored due to path filters (3)
docs/gifs/construction.gifis excluded by!**/*.gifdocs/gifs/freeness.gifis excluded by!**/*.gifdocs/gifs/random.gifis excluded by!**/*.gif
📒 Files selected for processing (12)
CLAUDE.mddocs/solvable-boards.htmldocs/strategy-difficulty-report.htmlsrc/app/play/page.tsxsrc/components/strategy-switcher/index.tsxsrc/utils/board-rules.tssrc/utils/init-gameboard.tssrc/utils/order-strategies.test.tssrc/utils/order-strategies.tssrc/utils/shuffle.tssrc/utils/solvable-board.test.tssrc/zustand/game-store.ts
Comment on lines
+331
to
+337
| redealCurrentLevel: () => { | ||
| set((state) => ({ | ||
| gameBoard: initializeGameBoard(state.level), | ||
| boardGeneration: Date.now(), | ||
| shouldAnimateOnMount: true, | ||
| })); | ||
| }, |
There was a problem hiding this comment.
Prevent stale animation timeouts from mutating a re-dealt board.
Line 331 enables board replacement while older clicked() animation timers may still fire. Those callbacks can delete indices from the new board and corrupt token parity/solvability. Guard timeout callbacks with a captured boardGeneration (or clear tracked timeout IDs before re-deal/restart).
💡 Proposed guard pattern
clicked: async (index: string) => {
+ const generationAtClick = get().boardGeneration;
set((state) => {
const gameBoard = { ...state.gameBoard };
const tile = gameBoard[index];
@@
setTimeout(() => {
set((state) => {
+ if (state.boardGeneration !== generationAtClick) return state;
const board = { ...state.gameBoard };
delete board[index];
delete board[otherActiveId];
@@
setTimeout(() => {
set((state) => {
+ if (state.boardGeneration !== generationAtClick) return state;
const board = { ...state.gameBoard };
if (board[otherActiveId]) {
board[otherActiveId] = { ...board[otherActiveId], animating: null };
@@
setTimeout(() => {
set((state) => {
+ if (state.boardGeneration !== generationAtClick) return state;
const board = { ...state.gameBoard };
if (board[index]?.animating === "sparkle") {
board[index] = { ...board[index], animating: null };🤖 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/zustand/game-store.ts` around lines 331 - 337, The redealCurrentLevel
function replaces gameBoard but doesn’t prevent previously scheduled animation
timeouts (from functions like clicked) from mutating the new board; modify
redealCurrentLevel to either increment/update the shared boardGeneration and
ensure all animation callbacks check that captured boardGeneration matches the
current one before mutating state, or track and clear outstanding timeout IDs
before re-dealing; update the animation callback in clicked (or wherever
setTimeout is used) to capture the boardGeneration value and no-op if it
mismatches to prevent stale timeouts from altering the new board.
Comment on lines
+334
to
+336
| boardGeneration: Date.now(), | ||
| shouldAnimateOnMount: true, | ||
| })); |
There was a problem hiding this comment.
Remove shouldAnimateOnMount assignment from redealCurrentLevel().
Line 335 sets shouldAnimateOnMount during re-deal. This flag should stay constrained to start()/levelCleared() flows.
✂️ Minimal fix
redealCurrentLevel: () => {
set((state) => ({
gameBoard: initializeGameBoard(state.level),
boardGeneration: Date.now(),
- shouldAnimateOnMount: true,
}));
},As per coding guidelines: Set shouldAnimateOnMount: true only in start() and levelCleared() actions; do NOT set it during pause/resume operations.
📝 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
| boardGeneration: Date.now(), | |
| shouldAnimateOnMount: true, | |
| })); | |
| redealCurrentLevel: () => { | |
| set((state) => ({ | |
| gameBoard: initializeGameBoard(state.level), | |
| boardGeneration: Date.now(), | |
| })); | |
| }, |
🤖 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/zustand/game-store.ts` around lines 334 - 336, Remove the assignment to
shouldAnimateOnMount from redealCurrentLevel(): currently redealCurrentLevel()
sets both boardGeneration and shouldAnimateOnMount; change it to only update
boardGeneration (and any other necessary state) and do not touch
shouldAnimateOnMount there so the flag remains controlled exclusively by start()
and levelCleared() flows; locate redealCurrentLevel in the zustand store and
remove the shouldAnimateOnMount update while leaving boardGeneration: Date.now()
unchanged.
Strategy switching is a dev-only affordance. In production getOrderStrategy now ignores player-accessible overrides (runtime + localStorage) and the window.eurekaOrder console hook is not exposed; production resolves env (NEXT_PUBLIC_ORDER_STRATEGY) -> default only. This keeps highscores comparable and prevents players from changing the board distribution they are scored on. The dev switcher badge was already NODE_ENV-gated.
Persist which order strategy produced a board so highscores can be compared within a strategy. The store captures getOrderStrategy().name whenever a board is dealt (start / next level / re-deal) and threads it through saveGameState -> /api/game -> the games row; the highscores query returns it and the leaderboard shows it under each name. Migration adds games.strategy (text, default 'random'): rows created before solvability-by-construction used a plain random fill, so they backfill to 'random'; the default also keeps inserts from the previously-deployed client valid. Applied to the project and verified (835 historical rows -> 'random').
Owner
Author
Update: strategy locked in prod + recorded on highscoresTwo follow-up commits:
DB migration — already applied to the project (additive + idempotent):
|
…ration # Conflicts: # src/app/highscores/page.tsx
There was a problem hiding this comment.
♻️ Duplicate comments (2)
src/zustand/game-store.ts (2)
338-344:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftGuard delayed tile callbacks against board-generation changes.
Re-deal swaps in a new board, but pending
setTimeoutcallbacks inclicked()can still delete/reset tiles on that new board. This can corrupt the re-dealt state.Suggested guard pattern
clicked: async (index: string) => { + const generationAtClick = get().boardGeneration; set((state) => { @@ setTimeout(() => { set((state) => { + if (state.boardGeneration !== generationAtClick) return state; const board = { ...state.gameBoard }; delete board[index]; delete board[otherActiveId]; @@ setTimeout(() => { set((state) => { + if (state.boardGeneration !== generationAtClick) return state; const board = { ...state.gameBoard }; if (board[otherActiveId]) { board[otherActiveId] = { ...board[otherActiveId], animating: null }; @@ setTimeout(() => { set((state) => { + if (state.boardGeneration !== generationAtClick) return state; const board = { ...state.gameBoard }; if (board[index]?.animating === "sparkle") { board[index] = { ...board[index], animating: null };🤖 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/zustand/game-store.ts` around lines 338 - 344, When redealCurrentLevel() replaces the board it sets boardGeneration; update clicked() so any delayed setTimeout callbacks capture the current boardGeneration (e.g., const token = get().boardGeneration or pass it into the closure) and early-return if get().boardGeneration !== token before performing any tile deletion/reset operations. Guard every delayed callback in clicked() that mutates gameBoard or tiles using this boardGeneration token to prevent callbacks from affecting a newly generated board.
338-344:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRemove
shouldAnimateOnMountfromredealCurrentLevel().Keeping this flag true in re-deal broadens intro-animation triggers beyond the intended lifecycle.
As per coding guidelines: `Set shouldAnimateOnMount: true only in start() and levelCleared() actions; do NOT set it during pause/resume operations`.Minimal fix
redealCurrentLevel: () => { set((state) => ({ gameBoard: initializeGameBoard(state.level), strategy: getOrderStrategy().name, boardGeneration: Date.now(), - shouldAnimateOnMount: true, })); },🤖 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/zustand/game-store.ts` around lines 338 - 344, redealCurrentLevel currently sets shouldAnimateOnMount to true which improperly retriggers intro animations; remove the shouldAnimateOnMount assignment from the redealCurrentLevel action in the game-store (function redealCurrentLevel) so it no longer mutates that flag. Verify that only start() and levelCleared() actions set shouldAnimateOnMount: true (adjust those methods if needed) and ensure pause/resume/redeal paths do not touch shouldAnimateOnMount.
🤖 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.
Duplicate comments:
In `@src/zustand/game-store.ts`:
- Around line 338-344: When redealCurrentLevel() replaces the board it sets
boardGeneration; update clicked() so any delayed setTimeout callbacks capture
the current boardGeneration (e.g., const token = get().boardGeneration or pass
it into the closure) and early-return if get().boardGeneration !== token before
performing any tile deletion/reset operations. Guard every delayed callback in
clicked() that mutates gameBoard or tiles using this boardGeneration token to
prevent callbacks from affecting a newly generated board.
- Around line 338-344: redealCurrentLevel currently sets shouldAnimateOnMount to
true which improperly retriggers intro animations; remove the
shouldAnimateOnMount assignment from the redealCurrentLevel action in the
game-store (function redealCurrentLevel) so it no longer mutates that flag.
Verify that only start() and levelCleared() actions set shouldAnimateOnMount:
true (adjust those methods if needed) and ensure pause/resume/redeal paths do
not touch shouldAnimateOnMount.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c5605015-8584-4af0-8523-b4bc1665a455
📒 Files selected for processing (7)
src/app/highscores/page.tsxsrc/db/select-game.tssrc/types/game.tssrc/utils/order-strategies.tssrc/utils/post-game-state.tssrc/zustand/game-store.tssupabase/migrations/20260525_add_strategy_to_games.sql
✅ Files skipped from review due to trivial changes (1)
- supabase/migrations/20260525_add_strategy_to_games.sql
Master refactored /highscores to render via the Leaderboard client component,
so the per-row strategy label moves there. `strategy` is optional on
HighscoreRow so the optimistic "you" row, historical rows ('random'), and
master's existing test fixtures all type-check.
order-strategies test: fewer iterations + an explicit timeout so the
solvable-deal sweep doesn't exceed the default 5s on slower CI — which also
stops it from starving the real-timer autosave test in a parallel worker.
"autosaves every 60 seconds" could flake on slow CI: start() spins a live 1s interval, so if the 59 manual step() calls took >1s the interval fired an extra step and tripped the autosave at 60 early. Scope fake timers to the test so only manual steps advance time. (Pre-existing flake; surfaced by CI running the workflow twice and splitting pass/fail on identical code.)
The autosave flake's real cause was a cross-test leak, not the test's own interval: start() spins a real 1s interval on the singleton store, and beforeEach reset state without clearing live intervals, so intervals from earlier tests kept firing step() on the shared store throughout the file — intermittently pushing timePassed to 60 and tripping the autosave early. Add a file-scope afterEach that clears any live timer after every test. With that plus the autosave test's own fake timers, no real interval fires during that test, so it's deterministic regardless of runner speed.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What & why
Randomly filling the turtle layout frequently dealt unsolvable boards, so whether a game could be won at all was partly luck — which undercuts a skill game. This makes every board solvable by construction, adds a strategy seam to tune board character/difficulty, and includes review docs.
Changes
src/utils/init-gameboard.ts): peel the fixed layout into a valid removal order, then lay matched token pairs onto it → every board is winnable for any shuffle.src/utils/board-rules.ts,isSelectable): single source of truth; the store'sallowedforSelectionnow delegates to it so game and generator can't diverge.src/utils/order-strategies.ts): swappable peel policies —topDownRandom,scatter(default),original,bottomUpRandom, plus a deterministiccanonicalfallback. Default isscatterbecause the layer-greedy policies pre-stack matching pairs at the top of the pyramid (100% of boards);scatterspreads matches across layers (0%)./play):s/Shift+Scycle,rre-deal, hover for descriptions; hidden in production.docs/solvable-boards.html+docs/gifs/) and a measured difficulty review (docs/strategy-difficulty-report.html).Difficulty findings (for the team)
Branching factor = legal moves available per step (higher = easier):
scatteris ~28% more forgiving than the old default. The deliberate difficulty lever (symbol labeling / copy-spread) is currently unused — see the report for the recommendation (add a difficulty dial) and open questions for design.Testing
yarn test— 101 pass (incl. re-solving generated boards across levels and every strategy).yarn type-checkandyarn lintclean (one pre-existing unrelated warning).Notes for reviewers
docs/gifs/adds ~4 MB of binaries (clips rendered from the real algorithms). Happy to move to Git LFS or drop from the repo if preferred.DEFAULT_STRATEGYchange (or an env / localStorage override) —topDownRandomis preserved verbatim.Summary by CodeRabbit
New Features
Bug Fixes / Improvements
Persistence / Backend
Tests
Documentation
UI