Add Rift Mode: detect unsolvable boards, show modal, persist state and styles - #25
Conversation
|
🌸 Diff Haiku Puzzle breaks, a rift, Generated by spit-the-diff |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a 'Rift Mode' to the Shandoku game, which activates when a player makes a move that results in an unsolvable puzzle state. It provides a user interface for recovery options, including restoring to the last solvable state or starting a new game. The changes include adding new game logic, UI elements, persistence mechanisms, and styles to support this new feature. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a significant new 'Rift Mode' feature, which activates when the puzzle becomes unsolvable. The implementation is comprehensive, covering state detection, UI changes, animations, and persistence. The code is generally well-structured. My review includes a few suggestions to refactor some of the new logic for better clarity and maintainability, and to improve consistency in how DOM elements are handled.
| if(hasConflict(r,c)) conflicted.push({r,c}); | ||
| } | ||
| } | ||
| if(conflicted.length) return conflicted.slice(0,Math.min(3,conflicted.length)); |
There was a problem hiding this comment.
The expression Math.min(3, conflicted.length) is not necessary here. The slice method handles an end index that is larger than the array's length gracefully by simply slicing to the end of the array. You can simplify this to conflicted.slice(0, 3) for better readability.
| if(conflicted.length) return conflicted.slice(0,Math.min(3,conflicted.length)); | |
| if(conflicted.length) return conflicted.slice(0, 3); |
| function openRiftModal(){ | ||
| const copy=riftCopySets[riftState.copyKey]||riftCopySets.pattern; | ||
| riftBodyEl.textContent=copy.fragment; | ||
| document.getElementById('riftTitle').textContent=copy.title; |
There was a problem hiding this comment.
For consistency with how other DOM elements are cached at the top of the script, and for minor performance/maintainability benefits, it's better to query for riftTitle and other rift-modal-related elements (like the buttons) once and store them in constants. This avoids repeated calls to document.getElementById within functions where the elements are used.
| if(data.riftState&&data.riftState.active){ | ||
| riftState.active=true; | ||
| riftState.sequenceRunning=false; | ||
| riftState.nodes=(data.riftState.nodes||[]).slice(0,3); | ||
| riftState.hasTriggered=!!data.riftState.hasTriggered; | ||
| riftState.cooldownUntil=Number(data.riftState.cooldownUntil)||0; | ||
| riftState.copyKey=data.riftState.copyKey||'pattern'; | ||
| boardShellEl.classList.add('rift-active'); | ||
| statusEl.classList.add('rift-status'); | ||
| } else { | ||
| riftState={ | ||
| active:false, | ||
| sequenceRunning:false, | ||
| nodes:[], | ||
| hasTriggered:!!(data.riftState&&data.riftState.hasTriggered), | ||
| cooldownUntil:Number(data.riftState&&data.riftState.cooldownUntil)||0, | ||
| copyKey:(data.riftState&&data.riftState.copyKey)||'pattern' | ||
| }; | ||
| } |
There was a problem hiding this comment.
The logic to restore riftState from saved data is a bit complex and contains some repetition. It can be refactored to be more concise and easier to read, which will improve maintainability. By using a single object for the saved state and providing defaults, you can handle all cases within a single block of code.
const savedRiftState = data.riftState || {};
const isActive = !!savedRiftState.active;
riftState = {
active: isActive,
sequenceRunning: false,
nodes: isActive ? (savedRiftState.nodes || []).slice(0, 3) : [],
hasTriggered: !!savedRiftState.hasTriggered,
cooldownUntil: Number(savedRiftState.cooldownUntil) || 0,
copyKey: savedRiftState.copyKey || 'pattern'
};
if (isActive) {
boardShellEl.classList.add('rift-active');
statusEl.classList.add('rift-status');
}
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0c042b6e13
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| riftState.active=false; | ||
| riftState.sequenceRunning=false; | ||
| riftState.nodes=[]; |
There was a problem hiding this comment.
clearRiftVisualState() resets flags/classes but never invalidates the in-flight runStatusSequence() token, so if a user starts a new game or restores state while the Rift sequence is mid-animation, the original triggerRiftEvent() continues and can re-activate Rift on the new board a few seconds later. This produces stale status text and incorrect riftState after the user has already moved to a different game state.
Useful? React with 👍 / 👎.
| render(); startTimer(); saveGame(); hideSplash(); | ||
| captureLastSolvableSnapshot(); |
There was a problem hiding this comment.
In newGame(), saveGame() runs before captureLastSolvableSnapshot(), so localStorage can briefly persist the previous puzzle’s lastSolvableSnapshot with the new puzzle’s grid/startingGrid. If the app is reloaded before the next autosave tick, a later “Restore last solvable” can restore an out-of-sync board from the old puzzle into the new game context.
Useful? React with 👍 / 👎.
|
@gemini review last commit |
There was a problem hiding this comment.
Code Review
This pull request introduces a significant new feature, 'Rift Mode', which activates when the Sudoku board becomes unsolvable. The changes are extensive, touching HTML to add a new modal, CSS for styling the rift state, and a large amount of JavaScript for the core logic. This includes a new solver to detect unsolvable states, state management for the rift, and persistence to local storage.
Overall, the implementation of the feature is comprehensive. However, I have a few points of feedback primarily concerning performance and maintainability in the JavaScript code.
- There is a significant performance concern with running a full Sudoku solver on every player move, which could lead to UI lag.
- There are opportunities to reduce code duplication and refactor complex functions to improve maintainability.
- The pull request description notes that no automated tests were added. For a feature with complex logic like the board solver, adding unit tests would be highly beneficial to ensure correctness and prevent future regressions.
My detailed comments are below.
| function evaluateRiftTrigger(origin='system'){ | ||
| if(origin!=='player-move') return; | ||
| if(riftState.active||riftState.sequenceRunning) return; | ||
| if(Date.now()<riftState.cooldownUntil) return; | ||
| const solvable=hasAnySolution(grid); | ||
| if(solvable){ | ||
| boardWasSolvable=true; | ||
| captureLastSolvableSnapshot(); | ||
| return; | ||
| } | ||
| if(boardWasSolvable){ | ||
| boardWasSolvable=false; | ||
| triggerRiftEvent(); | ||
| } | ||
| } |
There was a problem hiding this comment.
The evaluateRiftTrigger function calls hasAnySolution on every player move. This function performs a full backtracking search to determine if the board is solvable, which can be computationally expensive and may lead to UI freezes, especially on more complex boards or slower devices.
Consider these potential mitigations:
- Web Worker: Offload the solver to a Web Worker to prevent blocking the main UI thread.
- Debouncing: Debounce the call to
hasAnySolutionso it doesn't run on every single rapid input. - Trigger Condition: Only trigger this expensive check under more specific conditions, rather than on every move. For example, you could run it only when a move introduces a direct conflict.
| for(const line of lines){ | ||
| if(id!==statusSequenceId) return false; | ||
| setStatus(line); | ||
| await new Promise(resolve=>setTimeout(resolve,fast?500:950)); |
There was a problem hiding this comment.
The setTimeout delay uses magic numbers (500 and 950). To improve readability and maintainability, it's best to extract these values into named constants defined at the top of the script, for example:
const STATUS_SEQUENCE_DELAY_MS = 950;
const STATUS_SEQUENCE_DELAY_REDUCED_MOTION_MS = 500;Then you can use these constants here.
| if(data.lastSolvableSnapshot && data.lastSolvableSnapshot.grid && data.lastSolvableSnapshot.notes){ | ||
| lastSolvableSnapshot={ | ||
| ...data.lastSolvableSnapshot, | ||
| grid:cloneGrid(data.lastSolvableSnapshot.grid), | ||
| notes:data.lastSolvableSnapshot.notes.map(row=>row.map(arr=>new Set(arr))) | ||
| }; | ||
| } else { | ||
| lastSolvableSnapshot=null; | ||
| } | ||
| const savedRiftState=data.riftState||{}; | ||
| const isRiftActive=!!savedRiftState.active; | ||
| riftState={ | ||
| active:isRiftActive, | ||
| sequenceRunning:false, | ||
| nodes:isRiftActive?(savedRiftState.nodes||[]).slice(0,3):[], | ||
| hasTriggered:!!savedRiftState.hasTriggered, | ||
| cooldownUntil:Number(savedRiftState.cooldownUntil)||0, | ||
| copyKey:savedRiftState.copyKey||'pattern' | ||
| }; | ||
| if(isRiftActive){ | ||
| boardShellEl.classList.add('rift-active'); | ||
| statusEl.classList.add('rift-status'); | ||
| } |
There was a problem hiding this comment.
The logic for restoring the rift state within applyLoadedData adds significant complexity to an already large function. To improve maintainability and readability, consider extracting this block of code into its own dedicated helper function, for example restoreRiftState(data). This would make applyLoadedData easier to follow and the rift-specific restoration logic would be neatly encapsulated.
| if(interactionLocked) return; | ||
| if(riftState.active){ setStatus('Resolve the rift first.'); return; } |
There was a problem hiding this comment.
These two lines for checking interactionLocked and riftState.active are repeated in many of the user action functions (clearSelected, giveHint, checkBoard, solveBoard, etc.). This creates code duplication which can be a maintenance issue.
Consider refactoring this logic into a shared helper function to check if an interaction is allowed. This would centralize the guard logic and make the code cleaner and easier to update.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 15ce83ea76
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| cell.addEventListener('click',()=>{ selected={r,c}; render(); saveGame(); }); | ||
| cell.addEventListener('click',()=>{ | ||
| if(interactionLocked) return; | ||
| if(isRiftNode){ openRiftModal(); return; } |
There was a problem hiding this comment.
isRiftNode is captured when the cell is rendered, but this handler opens the modal without checking riftState.active. After closeRift() is used from Return/backdrop/Escape (which does not re-render), tapping a previously marked node still reopens the Rift modal even though the rift was closed, so users can get pulled back into Rift mode until another action forces a re-render.
Useful? React with 👍 / 👎.
| const solvable=hasAnySolution(grid); | ||
| if(solvable){ | ||
| boardWasSolvable=true; | ||
| captureLastSolvableSnapshot(); |
There was a problem hiding this comment.
This path computes solvable = hasAnySolution(grid) and then immediately calls captureLastSolvableSnapshot(), which runs hasAnySolution(grid) again internally. Because that solver is recursive backtracking, doing it twice on every solvable player move adds avoidable latency and can degrade responsiveness on harder boards or slower devices.
Useful? React with 👍 / 👎.
| if(movesSinceSolvabilityCheck<RIFT_NON_CONFLICT_CHECK_INTERVAL) return; | ||
| movesSinceSolvabilityCheck=0; | ||
| } | ||
| } |
There was a problem hiding this comment.
Summary
- Refactored the Rift solvability-check gating logic into a dedicated helper,
shouldRunSolvabilityCheck(options={}), so the intent is clearer and the nested branching is easier to read. This keeps the same behavior forforce,conflictIntroduced, and interval-based checks while improving maintainability. script.jsL368-L378 - Simplified
evaluateRiftTrigger(...)by replacing the prior nested gating block with a single early-return guard:if(!shouldRunSolvabilityCheck(options)) return;, which directly addresses the readability concern around this section. script.jsL380-L385
Testing
- ✅
node --check script.js
1 participant
Motivation
Description
index.htmlincluding a modal (#riftModal) with return/restore/start-fresh buttons and a backdrop.script.js: new constants and state (RIFT_COOLDOWN_MS,riftState,lastSolvableSnapshot, etc.), solver helpers (hasImmediateContradiction,hasAnySolution), rift lifecycle functions (triggerRiftEvent,evaluateRiftTrigger,openRiftModal,clearRiftVisualState,restoreLastSolvableState,captureLastSolvableSnapshot) and interaction locks to prevent regular actions while the rift is processing.lastSolvableSnapshotinto the save payload insaveGameand restore them inapplyLoadedData, and ensure new games / restores capture the solvable snapshot.style.cssfor rift visuals (rift-active,rift-glitch,rift-node,rift-modal), and bump the service worker cache name insw.jstoshandoku-wife-edition-v5.Testing
Codex Task