Add Cypress test and fix for win dialog null crash (render.js fadeIn)#187
Open
Coteh wants to merge 1 commit into
Open
Add Cypress test and fix for win dialog null crash (render.js fadeIn)#187Coteh wants to merge 1 commit into
Coteh wants to merge 1 commit into
Conversation
The 10ms fadeIn setTimeout in renderDialog re-queried the DOM with
document.querySelector(".dialog") instead of closing over the dialog
variable already in scope. If the dialog was removed before that timer
fired (e.g. the overlay became visible while the dialog was still at
opacity:0, and a click landed on it), the query returned null and threw
"can't access property 'style', dialog is null".
Fix: drop the re-declaration inside the setTimeout so it closes over
the existing dialog reference. A detached element reference is safe to
write styles to; a null one is not.
The new Cypress describe freezes all timers with cy.clock() so the
fadeIn callback can be triggered at an exact moment via cy.tick(10),
making the race condition reliably reproducible.
https://claude.ai/code/session_01DYsKXcjAhF95UkPuT3v4Wk
📝 WalkthroughWalkthroughThis pull request adds comprehensive test coverage for win dialog fade-in animation timing, including race condition validation, and removes a redundant DOM query in the render function to use an outer-scoped reference instead. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
cypress/e2e/game/dialog.cy.js (3)
24-29: Consider reducing repeated key-click boilerplate.This sequence is clear, but a tiny helper/loop would keep test setup shorter and easier to update.
♻️ Small refactor
- cy.keyboardItem("l").click(); - cy.keyboardItem("e").click(); - cy.keyboardItem("a").click(); - cy.keyboardItem("f").click(); - cy.keyboardItem("y").click(); + "leafy".split("").forEach((ch) => { + cy.keyboardItem(ch).click(); + }); cy.keyboardItem("enter").click();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cypress/e2e/game/dialog.cy.js` around lines 24 - 29, The repeated cy.keyboardItem("x").click() calls are boilerplate; replace them with a small helper or loop to iterate over the characters of the input string (e.g., "leafy" plus "enter") and call cy.keyboardItem(char).click() for each, or add a custom Cypress command like cy.typeKeyboardSequence(sequence) that maps each char to cy.keyboardItem and clicks it; update the test to use that helper and remove the six explicit cy.keyboardItem calls to make the setup concise and easier to change.
39-47: Add an explicit invisibility precondition before closing.This test intent is “close while invisible”; asserting that state before the click will make the race repro more robust against UI behavior drift.
✅ Make the race precondition explicit
cy.get(".dialog").should("exist"); + cy.get(".dialog").should("not.be.visible"); // Close the dialog before the timer fires (simulates overlay click on the invisible dialog) cy.get(".overlay-back").click();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cypress/e2e/game/dialog.cy.js` around lines 39 - 47, The test should assert the dialog is actually invisible before simulating the overlay click: after the existence check for ".dialog" add an explicit invisibility assertion (e.g., assert CSS opacity is "0" or use Cypress's visibility check) so the precondition for "close while invisible" is enforced prior to clicking ".overlay-back"; update the block around the cy.get(".dialog") and cy.get(".overlay-back").click() to include this invisibility assertion.
32-36: Assert pre-tick hidden state to validate the transition path.Right now this can pass even if the dialog is already visible before the timer callback. Add a precondition check so the test explicitly verifies the fade-in behavior.
✅ Tighten transition verification
it("should display win dialog after player wins", () => { + cy.get(".dialog").should("exist").and("not.be.visible"); // Tick to fire the frozen fadeIn timer - dialog transitions from opacity:0 to visible cy.tick(10); cy.contains("You win!").should("be.visible"); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cypress/e2e/game/dialog.cy.js` around lines 32 - 36, The test "should display win dialog after player wins" currently only checks visibility after cy.tick(10), so add an explicit precondition that the dialog is hidden before the tick to validate the fade-in transition path: before cy.tick(10) use cy.contains("You win!").should("not.be.visible") (or assert CSS opacity: 0 with cy.contains(...).should("have.css", "opacity", "0")) then call cy.tick(10) and assert it becomes visible with cy.contains("You win!").should("be.visible"); keep the existing assertions and use the same selector text "You win!" to locate the dialog.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@cypress/e2e/game/dialog.cy.js`:
- Around line 24-29: The repeated cy.keyboardItem("x").click() calls are
boilerplate; replace them with a small helper or loop to iterate over the
characters of the input string (e.g., "leafy" plus "enter") and call
cy.keyboardItem(char).click() for each, or add a custom Cypress command like
cy.typeKeyboardSequence(sequence) that maps each char to cy.keyboardItem and
clicks it; update the test to use that helper and remove the six explicit
cy.keyboardItem calls to make the setup concise and easier to change.
- Around line 39-47: The test should assert the dialog is actually invisible
before simulating the overlay click: after the existence check for ".dialog" add
an explicit invisibility assertion (e.g., assert CSS opacity is "0" or use
Cypress's visibility check) so the precondition for "close while invisible" is
enforced prior to clicking ".overlay-back"; update the block around the
cy.get(".dialog") and cy.get(".overlay-back").click() to include this
invisibility assertion.
- Around line 32-36: The test "should display win dialog after player wins"
currently only checks visibility after cy.tick(10), so add an explicit
precondition that the dialog is hidden before the tick to validate the fade-in
transition path: before cy.tick(10) use cy.contains("You
win!").should("not.be.visible") (or assert CSS opacity: 0 with
cy.contains(...).should("have.css", "opacity", "0")) then call cy.tick(10) and
assert it becomes visible with cy.contains("You win!").should("be.visible");
keep the existing assertions and use the same selector text "You win!" to locate
the dialog.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7d739dd3-ba42-4f25-8afd-77f975cdf776
📒 Files selected for processing (2)
cypress/e2e/game/dialog.cy.jssrc/render.js
💤 Files with no reviewable changes (1)
- src/render.js
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
2 participants
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.
The 10ms fadeIn setTimeout in renderDialog re-queried the DOM with
document.querySelector(".dialog") instead of closing over the dialog
variable already in scope. If the dialog was removed before that timer
fired (e.g. the overlay became visible while the dialog was still at
opacity:0, and a click landed on it), the query returned null and threw
"can't access property 'style', dialog is null".
Fix: drop the re-declaration inside the setTimeout so it closes over
the existing dialog reference. A detached element reference is safe to
write styles to; a null one is not.
The new Cypress describe freezes all timers with cy.clock() so the
fadeIn callback can be triggered at an exact moment via cy.tick(10),
making the race condition reliably reproducible.
https://claude.ai/code/session_01DYsKXcjAhF95UkPuT3v4Wk
Summary by CodeRabbit
Bug Fixes
Tests