Conversation
|
🌸 Diff Haiku Rift's pulse now steady, 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 focuses on refactoring the rift lifecycle, adding extension/test hooks, and stabilizing trigger logic within the Shandoku game. The changes aim to improve the manageability and robustness of the rift feature, reduce solver pressure, provide extension points for external code, and enhance accessibility. 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 and well-executed refactoring of the rift lifecycle management. The introduction of serializeRiftState/deserializeRiftState and applyPostMovePipeline greatly improves code organization and reduces duplication. The new extension hooks and test hooks are valuable additions for future development and testing. I've identified a couple of minor areas for improvement regarding error handling in hooks and code duplication, but overall, this is a solid set of changes.
script.js
Outdated
| function notifyRiftHook(name, payload){ | ||
| const handlers = riftExtensionHooks[name] || []; | ||
| handlers.forEach(handler => { | ||
| try { handler(payload); } catch { /* no-op */ } |
There was a problem hiding this comment.
Swallowing errors from extension handlers silently with a /* no-op */ can make debugging extensions very difficult. It's better to at least log the error to the console so that extension developers are aware of issues in their code.
| try { handler(payload); } catch { /* no-op */ } | |
| try { handler(payload); } catch (e) { console.error('Rift extension hook failed:', e); } |
script.js
Outdated
| if(typeof extension.onTrigger === 'function'){ | ||
| riftExtensionHooks.onTrigger.push(extension.onTrigger); | ||
| dispose.push(()=>{ riftExtensionHooks.onTrigger = riftExtensionHooks.onTrigger.filter(fn=>fn!==extension.onTrigger); }); | ||
| } | ||
| if(typeof extension.onResolve === 'function'){ | ||
| riftExtensionHooks.onResolve.push(extension.onResolve); | ||
| dispose.push(()=>{ riftExtensionHooks.onResolve = riftExtensionHooks.onResolve.filter(fn=>fn!==extension.onResolve); }); | ||
| } |
There was a problem hiding this comment.
The logic for registering and creating disposal functions for onTrigger and onResolve hooks is duplicated. This can be refactored to be more DRY by iterating over the hook names, which would make it easier to maintain and add new hooks in the future.
| if(typeof extension.onTrigger === 'function'){ | |
| riftExtensionHooks.onTrigger.push(extension.onTrigger); | |
| dispose.push(()=>{ riftExtensionHooks.onTrigger = riftExtensionHooks.onTrigger.filter(fn=>fn!==extension.onTrigger); }); | |
| } | |
| if(typeof extension.onResolve === 'function'){ | |
| riftExtensionHooks.onResolve.push(extension.onResolve); | |
| dispose.push(()=>{ riftExtensionHooks.onResolve = riftExtensionHooks.onResolve.filter(fn=>fn!==extension.onResolve); }); | |
| } | |
| ['onTrigger', 'onResolve'].forEach(hookName => { | |
| const handler = extension[hookName]; | |
| if (typeof handler === 'function') { | |
| riftExtensionHooks[hookName].push(handler); | |
| dispose.push(() => { | |
| riftExtensionHooks[hookName] = riftExtensionHooks[hookName].filter(fn => fn !== handler); | |
| }); | |
| } | |
| }); |
|
@codex review |
![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: 7cb63a4f06
ℹ️ 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".
script.js
Outdated
| const solvable=hasAnySolution(grid); | ||
| if(shouldTriggerRift(origin, options, solvable)){ |
There was a problem hiding this comment.
evaluateRiftTrigger computes hasAnySolution(grid) before applying the cooldown/active/debounce policy in shouldTriggerRift, so the expensive backtracking solver now runs on every scheduled player move (including non-conflicting moves that should be skipped). This is a regression from the prior flow where these gates short-circuited first, and it can cause noticeable lag/battery drain on harder boards and slower devices.
Useful? React with 👍 / 👎.
script.js
Outdated
|
|
||
| applyTheme(localStorage.getItem(THEME_KEY)||'dark'); | ||
| buildDigitPad(); | ||
| installTestHooks(); |
There was a problem hiding this comment.
@gemini do we even need these testHooks() now that we've integrated playwright via the main branch?
There was a problem hiding this comment.
That's a great question, ssfinney! While Playwright provides excellent end-to-end testing capabilities by simulating user interactions, the installTestHooks() function serves a different, complementary purpose that remains valuable.
These test hooks expose internal game state and control functions (like setBoardState, getStateSummary, forceEvaluateRift, and registerRiftExtension) directly to the window object under a __shandokuTest namespace. This allows Playwright tests (or any other in-browser testing framework) to:
- Efficiently set up complex test scenarios: Instead of simulating many clicks to reach a specific board state, tests can directly call
setBoardState. - Assert internal application state: It's often easier and more reliable to check the actual game state (e.g.,
countErrors(),boardWasSolvable) directly viagetStateSummaryrather than inferring it solely from UI elements. - Trigger specific events: Functions like
forceEvaluateRiftallow testing specific logic paths that might be hard to reliably trigger through UI interactions alone. - Observe internal events:
registerRiftExtensionallows tests to subscribe to and verify rift lifecycle events.
In essence, these hooks provide a more direct, less brittle, and often faster way to interact with the application's logic for testing purposes, complementing the UI-driven tests provided by Playwright. They are crucial for writing robust and maintainable tests that cover both UI and underlying game mechanics.
1 participant
Motivation
Description
riftSequenceRunningand centralized rift state serialization/deserialization withserializeRiftState/deserializeRiftState, plusresetRiftState.applyPostMovePipelinetorender,saveGame, optionallycaptureLastSolvableSnapshot, and schedule rift evaluation; replaced directrender/saveGamecalls in move flows with this pipeline.shouldTriggerRiftthat enforces policy (debouncing non-conflicting moves, honoring cooldown/force flags, and checking prior solvability) and leftevaluateRiftTriggerto call into it;triggerRiftEventnow honorsreduced-motionand togglesriftSequenceRunning.riftExtensionHooks) withnotifyRiftHookandregisterRiftExtensionto allow external handlers ononTrigger/onResolve.saveGameusingserializeRiftState, and restored it viadeserializeRiftStateinrestoreRiftStateFromSave.openRiftModalsaveslastFocusedBeforeRiftModaland focuses the return button, whilecloseRiftrestores focus and notifiesonResolve.installTestHooks, exposingwindow.__shandokuTest(state inspection,setBoardState,forceEvaluateRift,getRiftState,restoreLastSolvable,clearSave) andwindow.__shandokuInternal.registerRiftExtension.rift-glitchonly when not in reduced-motion mode and ensure board shell showsrift-activeappropriately.Testing
Codex Task