feat: rebuild app with React + Vite replacing StencilJS#38
Open
adrianschmidt-bot wants to merge 2 commits intopocket-dragon:devfrom
Open
feat: rebuild app with React + Vite replacing StencilJS#38adrianschmidt-bot wants to merge 2 commits intopocket-dragon:devfrom
adrianschmidt-bot wants to merge 2 commits intopocket-dragon:devfrom
Conversation
📝 WalkthroughWalkthroughMigration from a Stencil app to a Vite + React project: adds React entry, App component, game-state modules, React components, Vitest unit tests, Playwright e2e suites, Vite/VitePWA and Playwright configs, CI actions; removes legacy Stencil components, SCSS, several static assets, and old lint/format configs. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant BrowserApp as "React App (UI)"
participant GameState
participant Audio
User->>BrowserApp: Start / Pause / Feed / Use clue / Log success / Change difficulty
BrowserApp->>GameState: createInitialState(difficulty) / startTicker()
loop every 1s
BrowserApp->>GameState: tick()
GameState-->>BrowserApp: updated state (timers, clues, maybe gameResult)
alt sound enabled and threshold reached
BrowserApp->>Audio: play beep
Audio-->>BrowserApp: playback finished
end
BrowserApp-->>User: render updated UI (timers, buttons, dialogs)
end
User->>BrowserApp: user action (feed | clue | success | reset)
BrowserApp->>GameState: applyAction(...)
GameState-->>BrowserApp: updated state (maybe gameResult)
BrowserApp->>User: show result (game over / points / updated UI)
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Contributor
|
Hey @adrianschmidt-bot! Welcome to pocket-dragon! CodeRabbit made a bunch of comments before I had set up the webhook. Can you have a look at them please? |
Collaborator
Author
|
Hey @adrianschmidt! I went through all 11 CodeRabbit comments. Here's the summary: Fixed (⚡
|
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
package.json (1)
28-28: Prefer removing or populating emptyauthor.Leaving
authoras an empty string adds metadata noise; either set a value or drop the field.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package.json` at line 28, The package.json currently has an empty "author" field which should be removed or populated; edit the package.json to either delete the "author" property entirely or set it to a valid value (e.g., a name, organization, or contact) so metadata is meaningful—update the "author" entry that appears as "author": "" accordingly.src/components/RulesModal.tsx (2)
20-35: Tie toggle to panel and defer collapsed content rendering.For better a11y/perf, connect the button to the body via
aria-controls+id, and avoid rendering heavy markdown/image content while collapsed.♻️ Proposed refactor
<header className="collapsible-header"> <h2> <button type="button" className="collapsible-toggle" onClick={() => setIsOpen(!isOpen)} aria-expanded={isOpen} + aria-controls={`${testId}-content`} > Promo Rules: {title} </button> </h2> </header> - <div className="collapsible-body collapsible-content"> - <img src={imageUrl} alt={title} className="promo" /> - {children} - </div> + <div + id={`${testId}-content`} + className="collapsible-body collapsible-content" + hidden={!isOpen} + > + {isOpen ? ( + <> + <img src={imageUrl} alt={title} className="promo" /> + {children} + </> + ) : null} + </div>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/RulesModal.tsx` around lines 20 - 35, The collapsible button and body need ARIA linkage and deferred rendering: add a stable id (e.g., rulesPanelId) used as the body's id and reference it from the button's aria-controls while keeping aria-expanded driven by isOpen (functions/props: setIsOpen, isOpen, children, imageUrl); also avoid rendering the heavy collapsible content when collapsed by conditionally rendering the body (and image) only when isOpen (or when isOpen || some eager flag), and consider adding lazy-loading attributes to the image to reduce initial load.
161-161: Sanitize fallbackdata-testidgeneration.The fallback can include spaces/special characters from titles. Consider slugifying to keep stable selector-friendly test IDs for future promo entries.
💡 Suggested tweak
+const toPromoTestId = (title: string) => + `promo-${title.toLowerCase().replace(/[^a-z0-9]+/g, '-').replace(/^-|-$/g, '')}`; ... - testId={promoTestIds[game.title] || `promo-${game.title.toLowerCase()}`} + testId={promoTestIds[game.title] || toPromoTestId(game.title)}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/RulesModal.tsx` at line 161, The fallback data-testid generation uses raw game.title which may contain spaces/special chars—create a small sanitizer (e.g., slugifyTitle or getPromoTestId) that lowercases the title, replaces whitespace and non-alphanumeric chars with hyphens (and collapses multiple hyphens), and trim leading/trailing hyphens, then use promoTestIds[game.title] || `promo-${slugifyTitle(game.title)}` in the RulesModal component to produce stable selector-friendly test IDs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@package.json`:
- Around line 19-20: The publishApp script assumes the dist worktree is checked
out to master; add an explicit precondition check at the start of publishApp to
verify dist is a git worktree on branch master (or contains the expected commit)
and fail with a clear message if not, or automatically call setupPublishBranch
to create the worktree; specifically, update the publishApp npm script
(referencing "publishApp" and "setupPublishBranch") to run a git verification
such as checking git -C dist rev-parse --is-inside-work-tree and git -C dist
rev-parse --abbrev-ref HEAD (or checking git worktree list) and abort/prompt if
the branch is not master before proceeding to build, add, commit and push.
---
Nitpick comments:
In `@package.json`:
- Line 28: The package.json currently has an empty "author" field which should
be removed or populated; edit the package.json to either delete the "author"
property entirely or set it to a valid value (e.g., a name, organization, or
contact) so metadata is meaningful—update the "author" entry that appears as
"author": "" accordingly.
In `@src/components/RulesModal.tsx`:
- Around line 20-35: The collapsible button and body need ARIA linkage and
deferred rendering: add a stable id (e.g., rulesPanelId) used as the body's id
and reference it from the button's aria-controls while keeping aria-expanded
driven by isOpen (functions/props: setIsOpen, isOpen, children, imageUrl); also
avoid rendering the heavy collapsible content when collapsed by conditionally
rendering the body (and image) only when isOpen (or when isOpen || some eager
flag), and consider adding lazy-loading attributes to the image to reduce
initial load.
- Line 161: The fallback data-testid generation uses raw game.title which may
contain spaces/special chars—create a small sanitizer (e.g., slugifyTitle or
getPromoTestId) that lowercases the title, replaces whitespace and
non-alphanumeric chars with hyphens (and collapses multiple hyphens), and trim
leading/trailing hyphens, then use promoTestIds[game.title] ||
`promo-${slugifyTitle(game.title)}` in the RulesModal component to produce
stable selector-friendly test IDs.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (3)
.github/workflows/ci.ymlpackage.jsonsrc/components/RulesModal.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/ci.yml
Comment on lines
+19
to
+20
| "setupPublishBranch": "mkdir -p dist && git worktree add dist master", | ||
| "publishApp": "npm run build && cd dist && git add --all && (git diff --cached --quiet || git commit -m \"Publishing to gh-pages\") && git push --force-with-lease origin master && cd ..", |
There was a problem hiding this comment.
Add a branch/worktree precondition before publishing.
publishApp assumes dist is already the master worktree. If that precondition is missed, this can publish from the wrong local state.
Suggested hardening
- "publishApp": "npm run build && cd dist && git add --all && (git diff --cached --quiet || git commit -m \"Publishing to gh-pages\") && git push --force-with-lease origin master && cd ..",
+ "publishApp": "npm run build && cd dist && test \"$(git rev-parse --abbrev-ref HEAD)\" = \"master\" && git add --all && (git diff --cached --quiet || git commit -m \"Publishing to gh-pages\") && git push --force-with-lease origin master && cd ..",📝 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
| "setupPublishBranch": "mkdir -p dist && git worktree add dist master", | |
| "publishApp": "npm run build && cd dist && git add --all && (git diff --cached --quiet || git commit -m \"Publishing to gh-pages\") && git push --force-with-lease origin master && cd ..", | |
| "setupPublishBranch": "mkdir -p dist && git worktree add dist master", | |
| "publishApp": "npm run build && cd dist && test \"$(git rev-parse --abbrev-ref HEAD)\" = \"master\" && git add --all && (git diff --cached --quiet || git commit -m \"Publishing to gh-pages\") && git push --force-with-lease origin master && cd ..", |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@package.json` around lines 19 - 20, The publishApp script assumes the dist
worktree is checked out to master; add an explicit precondition check at the
start of publishApp to verify dist is a git worktree on branch master (or
contains the expected commit) and fail with a clear message if not, or
automatically call setupPublishBranch to create the worktree; specifically,
update the publishApp npm script (referencing "publishApp" and
"setupPublishBranch") to run a git verification such as checking git -C dist
rev-parse --is-inside-work-tree and git -C dist rev-parse --abbrev-ref HEAD (or
checking git worktree list) and abort/prompt if the branch is not master before
proceeding to build, add, commit and push.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
package.json (1)
17-18:⚠️ Potential issue | 🟠 MajorAdd a publish precondition check before pushing from
dist.
publishApp(Line 18) still assumesdistis themasterworktree. If that precondition is missed, the push can target unintended state. This was previously flagged and is still relevant.Suggested hardening
- "publishApp": "npm run build && cd dist && git add --all && (git diff --cached --quiet || git commit -m \"Publishing to gh-pages\") && git push --force-with-lease origin master && cd ..", + "publishApp": "npm run build && cd dist && test \"$(git rev-parse --abbrev-ref HEAD)\" = \"master\" && git add --all && (git diff --cached --quiet || git commit -m \"Publishing to gh-pages\") && git push --force-with-lease origin master && cd ..",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package.json` around lines 17 - 18, The publish script publishApp assumes dist is a master worktree; add a precondition check that verifies dist is a git worktree currently on branch "master" (for example by running git -C dist rev-parse --abbrev-ref HEAD and ensuring it equals "master" or by checking git -C dist status --porcelain --branch) and abort with a non-zero exit and clear error message if the check fails; update the "publishApp" npm script to perform this check before running git add/commit/push so pushes cannot target an unintended worktree, referencing the existing scripts "setupPublishBranch" and "publishApp" to locate the change.
🧹 Nitpick comments (1)
playwright.config.ts (1)
15-21: Deduplicate the base URL to avoid config drift.
http://localhost:5173is declared twice (use.baseURLandwebServer.url). A shared constant/env fallback makes future port/url changes safer.♻️ Proposed refactor
import { defineConfig, devices } from '@playwright/test'; +const E2E_BASE_URL = process.env.PLAYWRIGHT_BASE_URL ?? 'http://localhost:5173'; + export default defineConfig({ testDir: './e2e/tests', fullyParallel: true, forbidOnly: !!process.env.CI, retries: process.env.CI ? 2 : 0, workers: process.env.CI ? 1 : undefined, reporter: 'html', @@ use: { ...devices['Desktop Chrome'], - baseURL: 'http://localhost:5173', + baseURL: E2E_BASE_URL, trace: 'on-first-retry', }, webServer: { command: 'npm run dev', - url: 'http://localhost:5173', + url: E2E_BASE_URL, reuseExistingServer: !process.env.CI, }, });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@playwright.config.ts` around lines 15 - 21, The configuration duplicates the literal 'http://localhost:5173' in use.baseURL and webServer.url; define a single constant or read from an env fallback (e.g., const BASE_URL = process.env.PLAYWRIGHT_BASE_URL ?? 'http://localhost:5173') at the top of playwright.config.ts and replace both use.baseURL and webServer.url with that constant to prevent config drift and make future changes or CI overrides easy.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@package.json`:
- Around line 17-18: The publish script publishApp assumes dist is a master
worktree; add a precondition check that verifies dist is a git worktree
currently on branch "master" (for example by running git -C dist rev-parse
--abbrev-ref HEAD and ensuring it equals "master" or by checking git -C dist
status --porcelain --branch) and abort with a non-zero exit and clear error
message if the check fails; update the "publishApp" npm script to perform this
check before running git add/commit/push so pushes cannot target an unintended
worktree, referencing the existing scripts "setupPublishBranch" and "publishApp"
to locate the change.
---
Nitpick comments:
In `@playwright.config.ts`:
- Around line 15-21: The configuration duplicates the literal
'http://localhost:5173' in use.baseURL and webServer.url; define a single
constant or read from an env fallback (e.g., const BASE_URL =
process.env.PLAYWRIGHT_BASE_URL ?? 'http://localhost:5173') at the top of
playwright.config.ts and replace both use.baseURL and webServer.url with that
constant to prevent config drift and make future changes or CI overrides easy.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
adrianschmidt-bot
force-pushed
the
pocket-dragon-v2
branch
from
ae20e9c to
96a0357
Compare
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (2)
package.json (1)
17-18:⚠️ Potential issue | 🟠 MajorAdd a preflight check that
distis themasterworktree before publishing.Line 18 still assumes the worktree is correctly set up. If it isn’t,
publishAppcan push unintended content/branch history.Suggested hardening
- "publishApp": "npm run build && cd dist && git add --all && (git diff --cached --quiet || git commit -m \"Publishing to gh-pages\") && git push --force-with-lease origin master && cd ..", + "publishApp": "git -C dist rev-parse --is-inside-work-tree >/dev/null 2>&1 && test \"$(git -C dist rev-parse --abbrev-ref HEAD)\" = \"master\" || (echo \"dist must be a git worktree on branch master. Run: npm run setupPublishBranch\" >&2; exit 1); npm run build && cd dist && git add --all && (git diff --cached --quiet || git commit -m \"Publishing to gh-pages\") && git push --force-with-lease origin master && cd ..",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package.json` around lines 17 - 18, The publishApp script assumes dist is the master worktree and may push the wrong content; add a preflight that verifies dist is a git worktree on branch "master" and aborts if not. Modify the "publishApp" npm script to run a check (e.g. using git -C dist to show current branch or verify worktree membership) and exit non‑zero with a clear message when dist is not on master, before performing git add/commit/push; keep the remaining steps (build, add/commit, push, cd ..) unchanged so the publish only proceeds after the check passes. Ensure you reference the existing scripts "setupPublishBranch" and "publishApp" and that the check runs prior to any destructive git push.src/components/RulesModal.tsx (1)
22-33:⚠️ Potential issue | 🟡 MinorConnect the collapsible trigger to a controlled region.
Line [26] sets
aria-expanded, but Line [32] has noid/hiddenpairing, so collapsed content isn’t explicitly hidden from assistive technologies.Proposed fix
<button type="button" className="collapsible-toggle" onClick={() => setIsOpen(!isOpen)} aria-expanded={isOpen} + aria-controls={`${testId}-content`} > Promo Rules: {title} </button> @@ - <div className="collapsible-body collapsible-content"> + <div + id={`${testId}-content`} + className="collapsible-body collapsible-content" + hidden={!isOpen} + > <img src={imageUrl} alt={title} className="promo" /> {children} </div>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/RulesModal.tsx` around lines 22 - 33, The collapsible trigger sets aria-expanded but the collapsible content (the element with classes "collapsible-body collapsible-content") is not programmatically connected to the button; update the button (the element using setIsOpen and isOpen) to include aria-controls pointing at a unique id and give the collapsible content an id; also, when isOpen is false ensure the content is removed from AT tree (e.g., add hidden or aria-hidden="true") and when true remove hidden/aria-hidden so assistive tech sees the state change—use the existing setIsOpen/isOpen state and reference the button class "collapsible-toggle" and the content container to implement this.
🧹 Nitpick comments (4)
e2e/tests/sound.spec.ts (1)
82-97: Reduce real-time waiting in the warning-threshold e2e test.Line [93] allows a ~95s wait for one assertion, which can noticeably slow and destabilize CI runs. Consider a test-only shorter timer seed (or equivalent setup hook) so this scenario executes quickly and deterministically.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/tests/sound.spec.ts` around lines 82 - 97, The test currently waits ~95s to reach the 30s threshold; make it deterministic and fast by seeding or faking the timer instead of real-time waiting: modify the test to set the app into a test mode or inject a seed/initial timer value before starting (e.g., via a query param, localStorage flag, or page.evaluate to set the timer state) so selectors.feedTimer reports ≤30 immediately, then click selectors.startPauseButton and assert feedTimerContainer (feedTimer.locator('..')) has the warning class; alternatively, use Playwright's page.evaluate to directly advance or stub the timer function in the app to jump to 30s and assert immediately.e2e/tests/clues.spec.ts (1)
125-130: Derive regen timeout from shared constants.Use
gameConstants.clueRegenMax(+ buffer) instead of hardcoded25000to keep test timing aligned with config changes.Proposed refactor
- // Wait for clue regeneration (15-20 seconds, plus buffer) - // Clue should regenerate within 20 seconds, we'll wait up to 25 to be safe + // Wait for clue regeneration (config max + buffer) + const regenTimeoutMs = (gameConstants.clueRegenMax + 5) * 1000; await expect(async () => { const clues = await remainingClues.textContent(); expect(parseInt(clues!)).toBeGreaterThan(0); - }).toPass({ timeout: 25000 }); + }).toPass({ timeout: regenTimeoutMs });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/tests/clues.spec.ts` around lines 125 - 130, Replace the hardcoded timeout 25000 in the toPass call with a derived value using gameConstants.clueRegenMax plus your buffer; locate the assertion block that awaits remainingClues.textContent() in clues.spec.ts and change the timeout argument to something like (gameConstants.clueRegenMax + BUFFER_MS) so the test uses gameConstants.clueRegenMax (reference symbol) and a small explicit buffer constant, ensuring remainingClues and the toPass call are updated accordingly.e2e/tests/success.spec.ts (1)
30-35: UsegameConfig.easyvalues instead of hardcoded counters.These literals couple tests to today’s easy defaults and will drift if config changes.
Proposed refactor
- await expect(successCounter).toHaveText('3'); + await expect(successCounter).toHaveText(String(gameConfig.easy.goalNumberOfSuccesses)); - await expect(successCounter).toHaveText('2'); + await expect(successCounter).toHaveText(String(gameConfig.easy.goalNumberOfSuccesses - 1)); - await expect(successCounter).toHaveText('1'); + await expect(successCounter).toHaveText(String(gameConfig.easy.goalNumberOfSuccesses - 2)); - await expect(successCounter).toHaveText('3'); + await expect(successCounter).toHaveText(String(gameConfig.easy.goalNumberOfSuccesses));Also applies to: 49-53, 104-104
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/tests/success.spec.ts` around lines 30 - 35, Replace the hardcoded numeric expectations in the test with values derived from the game's easy config: read the initial success count from gameConfig.easy (e.g., gameConfig.easy.successes or the correct property) and assert successCounter shows that value, then click logSuccessButton and assert the counter equals that initial value minus one; apply the same replacement pattern for the other occurrences mentioned (around the checks at 49-53 and 104) to avoid coupling the test to today's defaults.e2e/tests/rules-modal.spec.ts (1)
82-97: Strengthen the collapsible test to verify true toggle behavior.Right now this can still pass if the section is always expanded. Assert both directions (expand and collapse).
Suggested enhancement
const header = anachronyPromo.locator('h2'); await expect(header).toBeVisible(); const promoImage = anachronyPromo.locator('img.promo'); + await expect(promoImage).toBeHidden(); await header.click(); - const promoImage = anachronyPromo.locator('img.promo'); await expect(promoImage).toBeVisible(); + + await header.click(); + await expect(promoImage).toBeHidden();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/tests/rules-modal.spec.ts` around lines 82 - 97, The test currently only clicks the header and checks that promoImage is visible, which won’t fail if the section is always expanded; update the test around the locators anachronyPromo and header to assert a true toggle: first assert the section is initially collapsed (e.g., no expanded attribute/class on anachronyPromo or promoImage not visible), then click header and assert expanded state (presence of expanded attribute/class or promoImage visible), then click header again and assert it returns to collapsed (attribute/class removed or promoImage hidden). Use the existing anachronyPromo, header and promoImage locators to perform these three assertions so the test verifies both expand and collapse behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@e2e/tests/rules-modal.spec.ts`:
- Around line 57-60: The current assertion only checks absence of the CSS class
and can false-pass; change the assertion on the rules dialog locator
(rulesDialog) to assert visibility/hidden state (e.g., use toBeHidden() or
expect(...).not.toBeVisible()) so the test verifies the dialog is actually not
visible to the user rather than just lacking mdc-dialog--open; update the
assertion call on rulesDialog accordingly and keep any needed wait/timeout to
account for animations.
In `@e2e/tests/timers.spec.ts`:
- Line 40: Replace uses of the deprecated fixed sleep page.waitForTimeout (e.g.,
the call at await page.waitForTimeout(1100) and the other occurrences on the
same file) with state-driven waits: either assert the expected UI change with
Playwright's expect(...) auto-waiters (matching the pattern used in the "timers
decrement each second" test, e.g., expect(el).toHaveText / toContainText with a
timeout) or, for deterministic timer tests, use page.clock to mock/fast-forward
time and trigger setTimeout/setInterval behavior; update the affected tests to
remove page.waitForTimeout and use expect assertions or page.clock.advance()
calls instead so tests wait on state changes rather than fixed timeouts.
In `@src/App.tsx`:
- Around line 366-370: The game-over dialog container (<aside
id="game-over-dialog" data-testid="game-over-dialog">) is missing explicit modal
semantics; update that element to include role="dialog" and aria-modal="true"
(mirroring the rules modal) so assistive tech recognizes it as a modal dialog
and properly restricts background interaction.
In `@src/components/RulesModal.tsx`:
- Around line 72-102: The loop currently calls line.trim() into trimmed which
removes leading indentation and flattens nested list items; change the parsing
to preserve leading spaces for list-detection (use a variable like raw =
line.replace(/\s+$/,"") or raw = line.replace(/\r?\n$/,"") to only trim trailing
whitespace) and run startsWith/regex checks against raw (or match against
/^\s*## /, /^\s*### /, /^\s*-\s/, /^\s*\d+\.\s/) while only trimming the content
that gets pushed into currentList (use content = raw.replace(/^\s*[-\d.]+\s/,"")
or formatInline(raw.trimStart().slice(...)) as appropriate). Update references
to trimmed in the header and list branches to use raw for detection and a
trimmed/content variable for the stored/displayed text so flushList, listType,
currentList, and formatInline preserve nested indentation.
---
Duplicate comments:
In `@package.json`:
- Around line 17-18: The publishApp script assumes dist is the master worktree
and may push the wrong content; add a preflight that verifies dist is a git
worktree on branch "master" and aborts if not. Modify the "publishApp" npm
script to run a check (e.g. using git -C dist to show current branch or verify
worktree membership) and exit non‑zero with a clear message when dist is not on
master, before performing git add/commit/push; keep the remaining steps (build,
add/commit, push, cd ..) unchanged so the publish only proceeds after the check
passes. Ensure you reference the existing scripts "setupPublishBranch" and
"publishApp" and that the check runs prior to any destructive git push.
In `@src/components/RulesModal.tsx`:
- Around line 22-33: The collapsible trigger sets aria-expanded but the
collapsible content (the element with classes "collapsible-body
collapsible-content") is not programmatically connected to the button; update
the button (the element using setIsOpen and isOpen) to include aria-controls
pointing at a unique id and give the collapsible content an id; also, when
isOpen is false ensure the content is removed from AT tree (e.g., add hidden or
aria-hidden="true") and when true remove hidden/aria-hidden so assistive tech
sees the state change—use the existing setIsOpen/isOpen state and reference the
button class "collapsible-toggle" and the content container to implement this.
---
Nitpick comments:
In `@e2e/tests/clues.spec.ts`:
- Around line 125-130: Replace the hardcoded timeout 25000 in the toPass call
with a derived value using gameConstants.clueRegenMax plus your buffer; locate
the assertion block that awaits remainingClues.textContent() in clues.spec.ts
and change the timeout argument to something like (gameConstants.clueRegenMax +
BUFFER_MS) so the test uses gameConstants.clueRegenMax (reference symbol) and a
small explicit buffer constant, ensuring remainingClues and the toPass call are
updated accordingly.
In `@e2e/tests/rules-modal.spec.ts`:
- Around line 82-97: The test currently only clicks the header and checks that
promoImage is visible, which won’t fail if the section is always expanded;
update the test around the locators anachronyPromo and header to assert a true
toggle: first assert the section is initially collapsed (e.g., no expanded
attribute/class on anachronyPromo or promoImage not visible), then click header
and assert expanded state (presence of expanded attribute/class or promoImage
visible), then click header again and assert it returns to collapsed
(attribute/class removed or promoImage hidden). Use the existing anachronyPromo,
header and promoImage locators to perform these three assertions so the test
verifies both expand and collapse behavior.
In `@e2e/tests/sound.spec.ts`:
- Around line 82-97: The test currently waits ~95s to reach the 30s threshold;
make it deterministic and fast by seeding or faking the timer instead of
real-time waiting: modify the test to set the app into a test mode or inject a
seed/initial timer value before starting (e.g., via a query param, localStorage
flag, or page.evaluate to set the timer state) so selectors.feedTimer reports
≤30 immediately, then click selectors.startPauseButton and assert
feedTimerContainer (feedTimer.locator('..')) has the warning class;
alternatively, use Playwright's page.evaluate to directly advance or stub the
timer function in the app to jump to 30s and assert immediately.
In `@e2e/tests/success.spec.ts`:
- Around line 30-35: Replace the hardcoded numeric expectations in the test with
values derived from the game's easy config: read the initial success count from
gameConfig.easy (e.g., gameConfig.easy.successes or the correct property) and
assert successCounter shows that value, then click logSuccessButton and assert
the counter equals that initial value minus one; apply the same replacement
pattern for the other occurrences mentioned (around the checks at 49-53 and 104)
to avoid coupling the test to today's defaults.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (58)
package-lock.jsonis excluded by!**/package-lock.jsonpublic/assets/font/cartoons_123-webfont.woffis excluded by!**/*.woffpublic/assets/font/cartoons_123-webfont.woff2is excluded by!**/*.woff2public/assets/icon/android-icon-144x144.pngis excluded by!**/*.pngpublic/assets/icon/android-icon-192x192.pngis excluded by!**/*.pngpublic/assets/icon/android-icon-36x36.pngis excluded by!**/*.pngpublic/assets/icon/android-icon-48x48.pngis excluded by!**/*.pngpublic/assets/icon/android-icon-512x512.pngis excluded by!**/*.pngpublic/assets/icon/android-icon-72x72.pngis excluded by!**/*.pngpublic/assets/icon/android-icon-96x96.pngis excluded by!**/*.pngpublic/assets/icon/apple-icon-114x114.pngis excluded by!**/*.pngpublic/assets/icon/apple-icon-120x120.pngis excluded by!**/*.pngpublic/assets/icon/apple-icon-144x144.pngis excluded by!**/*.pngpublic/assets/icon/apple-icon-152x152.pngis excluded by!**/*.pngpublic/assets/icon/apple-icon-180x180.pngis excluded by!**/*.pngpublic/assets/icon/apple-icon-57x57.pngis excluded by!**/*.pngpublic/assets/icon/apple-icon-60x60.pngis excluded by!**/*.pngpublic/assets/icon/apple-icon-72x72.pngis excluded by!**/*.pngpublic/assets/icon/apple-icon-76x76.pngis excluded by!**/*.pngpublic/assets/icon/apple-icon-precomposed.pngis excluded by!**/*.pngpublic/assets/icon/apple-icon.pngis excluded by!**/*.pngpublic/assets/icon/favicon-16x16.pngis excluded by!**/*.pngpublic/assets/icon/favicon-32x32.pngis excluded by!**/*.pngpublic/assets/icon/favicon-96x96.pngis excluded by!**/*.pngpublic/assets/icon/favicon.icois excluded by!**/*.icopublic/assets/icon/icon-source.pngis excluded by!**/*.pngpublic/assets/icon/ms-icon-144x144.pngis excluded by!**/*.pngpublic/assets/icon/ms-icon-150x150.pngis excluded by!**/*.pngpublic/assets/icon/ms-icon-310x310.pngis excluded by!**/*.pngpublic/assets/icon/ms-icon-70x70.pngis excluded by!**/*.pngpublic/assets/promo-anachrony.jpgis excluded by!**/*.jpgpublic/assets/promo-daysOfIre.jpgis excluded by!**/*.jpgpublic/assets/promo-diceSettlers.jpgis excluded by!**/*.jpgpublic/assets/promo-kitchenRush.jpgis excluded by!**/*.jpgpublic/assets/promo-microfilms.jpgis excluded by!**/*.jpgpublic/assets/promo-nightsOfFire.jpgis excluded by!**/*.jpgpublic/assets/promo-petrichor.jpgis excluded by!**/*.jpgpublic/assets/promo-redacted.jpgis excluded by!**/*.jpgpublic/assets/promo-tashKalar.jpgis excluded by!**/*.jpgpublic/assets/promo-trickerion.jpgis excluded by!**/*.jpgpublic/assets/sound/beep-hightone-2x.mp3is excluded by!**/*.mp3public/assets/sound/beep-hightone-2x.oggis excluded by!**/*.oggpublic/assets/sound/beep-hightone-2x.wavis excluded by!**/*.wavpublic/assets/sound/beep-hightone-3x.mp3is excluded by!**/*.mp3public/assets/sound/beep-hightone-3x.oggis excluded by!**/*.oggpublic/assets/sound/beep-hightone-3x.wavis excluded by!**/*.wavpublic/assets/sound/beep-hightone.mp3is excluded by!**/*.mp3public/assets/sound/beep-hightone.oggis excluded by!**/*.oggpublic/assets/sound/beep-hightone.wavis excluded by!**/*.wavpublic/assets/sound/cling_2-2x.mp3is excluded by!**/*.mp3public/assets/sound/cling_2-2x.oggis excluded by!**/*.oggpublic/assets/sound/cling_2-2x.wavis excluded by!**/*.wavpublic/assets/sound/cling_2-3x.mp3is excluded by!**/*.mp3public/assets/sound/cling_2-3x.oggis excluded by!**/*.oggpublic/assets/sound/cling_2-3x.wavis excluded by!**/*.wavpublic/assets/sound/cling_2.mp3is excluded by!**/*.mp3public/assets/sound/cling_2.oggis excluded by!**/*.oggpublic/assets/sound/cling_2.wavis excluded by!**/*.wav
📒 Files selected for processing (61)
.github/actions/set-up-node/action.yml.github/workflows/ci.yml.github/workflows/nodejs.yml.github/workflows/up-check.yml.gitignore.nvmrc.prettierrcLICENSES.txte2e/fixtures/selectors.tse2e/tests/clues.spec.tse2e/tests/difficulty.spec.tse2e/tests/feeding.spec.tse2e/tests/game-over.spec.tse2e/tests/rules-modal.spec.tse2e/tests/sound.spec.tse2e/tests/success.spec.tse2e/tests/timers.spec.tseslint.jsindex.htmlpackage.jsonplaywright.config.tssrc/App.csssrc/App.tsxsrc/assets/font/generator_config.txtsrc/assets/font/stylesheet.csssrc/browserconfig.xmlsrc/components/PdButton.csssrc/components/PdButton.tsxsrc/components/RulesModal.tsxsrc/components/app-collapsible-section/app-collapsible-section.scsssrc/components/app-collapsible-section/app-collapsible-section.tsxsrc/components/app-home/app-home.spec.tssrc/components/app-home/app-home.tsxsrc/components/app-root/app-root.scsssrc/components/app-root/app-root.spec.tssrc/components/app-root/app-root.tsxsrc/components/app-rules/app-rules.scsssrc/components/app-rules/app-rules.tsxsrc/components/button/button.scsssrc/components/button/button.tsxsrc/components/settings/settings.scsssrc/components/settings/settings.tsxsrc/global/app.csssrc/index.htmlsrc/logic/difficulty.test.tssrc/logic/difficulty.tssrc/logic/gameState.test.tssrc/logic/gameState.tssrc/main.tsxsrc/manifest.jsonsrc/rules/game-rules.tssrc/services/storage.tssrc/styles/global.csssrc/styles/variables.csssrc/variables.scsssrc/vite-env.d.tsstencil.config.tstsconfig.jsontsconfig.node.jsontslint.jsvite.config.ts
💤 Files with no reviewable changes (26)
- src/components/app-root/app-root.scss
- src/components/app-root/app-root.tsx
- src/components/app-home/app-home.spec.ts
- src/index.html
- src/variables.scss
- src/components/app-rules/app-rules.tsx
- src/browserconfig.xml
- src/manifest.json
- src/global/app.css
- src/components/button/button.tsx
- src/services/storage.ts
- src/components/settings/settings.tsx
- src/components/settings/settings.scss
- src/components/app-home/app-home.tsx
- eslint.js
- src/components/button/button.scss
- tslint.js
- .github/workflows/nodejs.yml
- src/components/app-collapsible-section/app-collapsible-section.scss
- stencil.config.ts
- src/components/app-root/app-root.spec.ts
- src/components/app-collapsible-section/app-collapsible-section.tsx
- .prettierrc
- src/assets/font/stylesheet.css
- src/assets/font/generator_config.txt
- src/components/app-rules/app-rules.scss
✅ Files skipped from review due to trivial changes (1)
- src/App.css
🚧 Files skipped from review as they are similar to previous changes (18)
- src/styles/global.css
- .nvmrc
- vite.config.ts
- src/components/PdButton.tsx
- e2e/fixtures/selectors.ts
- src/vite-env.d.ts
- e2e/tests/feeding.spec.ts
- e2e/tests/game-over.spec.ts
- .gitignore
- index.html
- src/logic/difficulty.test.ts
- .github/workflows/up-check.yml
- .github/workflows/ci.yml
- src/logic/gameState.ts
- src/main.tsx
- src/styles/variables.css
- .github/actions/set-up-node/action.yml
- playwright.config.ts
e2e/tests/timers.spec.ts
Outdated
|
|
||
| // Start the game | ||
| await page.locator(selectors.startPauseButton).click(); | ||
| await page.waitForTimeout(1100); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
# First, check if the file exists and view its content
file_path="e2e/tests/timers.spec.ts"
if [ -f "$file_path" ]; then
echo "=== File found: $file_path ==="
wc -l "$file_path"
echo ""
echo "=== Lines 35-65 (around line 40) ==="
sed -n '35,65p' "$file_path"
echo ""
echo "=== Lines 115-125 (around line 122) ==="
sed -n '115,125p' "$file_path"
else
echo "File not found at $file_path"
find . -name "timers.spec.ts" -o -name "*.spec.ts" | head -20
fiRepository: pocket-dragon/pocket-dragon.github.io
Length of output: 1699
🏁 Script executed:
# Search for all waitForTimeout usages in e2e tests
rg -n "waitForTimeout" e2e/tests --color neverRepository: pocket-dragon/pocket-dragon.github.io
Length of output: 341
🌐 Web query:
Playwright waitForTimeout API deprecation modern version
💡 Result:
page.waitForTimeout() is not removed, but in current Playwright docs it’s treated as discouraged (only for debugging) and is listed under the Page “Deprecated” section—because fixed sleeps make tests flaky and slow. Instead, Playwright recommends relying on auto-waiting locator actions and web assertions that wait for the real condition you care about. [1]
Modern replacements (pick the signal you actually need):
- Prefer auto-waits + assertions:
await expect(locator).toBeVisible()/toHaveText()etc. (these wait automatically). [1] - Wait for navigation/URL:
await page.waitForURL(...)(preferred over racy navigation waits). [1] - Wait for page readiness:
await page.waitForLoadState('domcontentloaded' | 'load' | 'networkidle'). [1] - Wait for a custom condition:
await page.waitForFunction(() => ...). [1]
If your goal is just “pause so I can look”:
- Use
await page.pause()and run in debug mode (e.g.npx playwright test --debug) rather than sleeping. [2]
If you still use it: keep it for local debugging only, not production/CI stability. [1]
Sources:
[1] Playwright API docs – Page.waitForTimeout() (discouraged; shown under Page “Deprecated”)
[2] Playwright “Debugging Tests” – recommends page.pause() for breakpoints/debugging
🌐 Web query:
Playwright best practices waiting for timer state changes
💡 Result:
Best practices for “timer/state change” waits in Playwright
1) Prefer web-first assertions on user-visible UI (auto-retries)
Instead of waiting for a timeout/interval to “finish”, assert the UI that changes because of it:
await page.getByRole('button', { name: 'Start' }).click();
await expect(page.getByTestId('status')).toHaveText('Done');Playwright’s locator assertions auto-wait/retry until the condition is met (or times out). (playwright.dev)
2) If the “state” is not directly in the DOM, use expect.poll (or waitForFunction)
For app/JS state that changes asynchronously (often driven by timers), poll until it becomes true:
await expect.poll(async () => {
return await page.evaluate(() => window.appState?.phase);
}).toBe('ready');expect.poll turns a normal expect(...) into a polling assertion with configurable timeout/intervals. (playwright.dev)
3) For timer-driven code, control time with Playwright’s Clock (avoid real waiting)
If your app uses setTimeout/setInterval/Date.now(), fake and advance time deterministically:
await page.clock.install(); // must be before the app sets timers
await page.goto('http://localhost:3000');
await page.clock.runFor(30_000); // or: fastForward(...)
await expect(page.getByTestId('status')).toHaveText('Expired');Playwright Clock can override Date, setTimeout, setInterval, requestAnimationFrame, etc., letting you fast-forward without flaky delays. (playwright.dev)
4) Avoid hard sleeps like waitForTimeout in real tests
They don’t assert readiness and are a common cause of flakiness—use the strategies above instead. (circleci.com)
Citations:
- 1: https://playwright.dev/docs/best-practices
- 2: https://playwright.dev/docs/test-assertions
- 3: https://playwright.dev/docs/clock?utm_source=openai
- 4: https://circleci.com/blog/mastering-waits-and-timeouts-in-playwright/?utm_source=openai
🏁 Script executed:
# Check more context around timer tests to understand what's being tested
sed -n '1,40p' e2e/tests/timers.spec.tsRepository: pocket-dragon/pocket-dragon.github.io
Length of output: 1631
🏁 Script executed:
# Look for imports and setup to understand the test structure better
sed -n '30,80p' e2e/tests/timers.spec.tsRepository: pocket-dragon/pocket-dragon.github.io
Length of output: 1822
Replace waitForTimeout (deprecated API) with state-driven waits to eliminate flakiness.
page.waitForTimeout() is explicitly discouraged in modern Playwright and listed in the deprecated section. Fixed sleeps cause CI flakiness and are unreliable for timer testing.
Two approaches:
- Use
expect()assertions (already in your first test) — auto-wait for the UI state change with configurable timeout. This matches the pattern shown at lines 13–33. - Prefer
page.clock(best for timer tests) — mock/fast-forward time deterministically instead of waiting for real time to pass. This eliminates flakiness entirely forsetTimeout/setInterval-driven behavior.
Apply to lines 40, 49, 60, and 122. Note that your first test (timers decrement each second) already uses the modern assertion pattern—align the remaining tests.
Example: expect() replacement
- await page.waitForTimeout(1100);
+ await expect(gameTimer).not.toHaveText(String(gameConfig.easy.initialGameTimer), { timeout: 3000 });
// Pause the game
await page.locator(selectors.startPauseButton).click();
// Record the timer value
const timerValueWhenPaused = await gameTimer.textContent();
- await page.waitForTimeout(1100);
- await expect(gameTimer).toHaveText(timerValueWhenPaused!);
+ await expect(gameTimer).toHaveText(timerValueWhenPaused!, { timeout: 1500 });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@e2e/tests/timers.spec.ts` at line 40, Replace uses of the deprecated fixed
sleep page.waitForTimeout (e.g., the call at await page.waitForTimeout(1100) and
the other occurrences on the same file) with state-driven waits: either assert
the expected UI change with Playwright's expect(...) auto-waiters (matching the
pattern used in the "timers decrement each second" test, e.g.,
expect(el).toHaveText / toContainText with a timeout) or, for deterministic
timer tests, use page.clock to mock/fast-forward time and trigger
setTimeout/setInterval behavior; update the affected tests to remove
page.waitForTimeout and use expect assertions or page.clock.advance() calls
instead so tests wait on state changes rather than fixed timeouts.
Contributor
There was a problem hiding this comment.
This seems like a good improvement, but I want to get this merged, so let's put this on the todo-list to do after this PR has been merged.
There was a problem hiding this comment.
@adrianschmidt, understood! That's a reasonable approach to get this PR merged first.
Would you like me to create a GitHub issue to track this improvement? I can open one with:
- Title: "Replace
waitForTimeoutwith state-driven waits in e2e timer tests" - Details about the specific lines (40, 49, 60, 122 in
e2e/tests/timers.spec.ts) - Context about using
expect()assertions orpage.clockfor more reliable timer testing - A link back to this PR discussion
Just let me know if you'd like me to create that issue! 🐰
Collaborator
Author
There was a problem hiding this comment.
Got it — added to the post-merge TODO list. Won't hold up the PR.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/components/RulesModal.tsx (1)
209-209: Consider adding keyboard support for closing the modal.The backdrop
onClickallows mouse users to close the modal, but keyboard users have no equivalent Escape key handler. While the close button is keyboard-accessible, adding anonKeyDownlistener for Escape on the modal container would align with WAI-ARIA dialog best practices.♻️ Optional enhancement
export function RulesModal({ isOpen, onClose }: RulesModalProps) { + const handleKeyDown = (e: React.KeyboardEvent) => { + if (e.key === 'Escape') { + onClose(); + } + }; + return ( <aside id="rules-dialog" data-testid="rules-dialog" className={`mdc-dialog ${isOpen ? 'mdc-dialog--open' : ''}`} role="dialog" aria-modal="true" + onKeyDown={handleKeyDown} >🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/RulesModal.tsx` at line 209, Add Escape-key handling to the RulesModal so keyboard users can close it: in the RulesModal component (the element rendering the backdrop with className "mdc-dialog__backdrop" and the surrounding dialog/container), add an onKeyDown handler that listens for the Escape key and calls the existing onClose prop, ensure the dialog container can receive keyboard events by adding a focusable attribute (e.g., tabIndex={-1}) and set focus to it when opening so Escape presses are captured; keep the existing backdrop onClick and close button behavior unchanged.src/App.css (1)
302-308: Chevron direction appears inverted from common convention.Currently, collapsed sections show
▲and expanded show▼. Typical conventions use▶or▼to indicate expandable (pointing toward hidden content) and▲to indicate collapsible. This may confuse users expecting the standard pattern.♻️ Optional: Align with common conventions
.collapsible-section.collapsed .collapsible-toggle::before { - content: '▲'; + content: '▶'; } .collapsible-section.expanded .collapsible-toggle::before { content: '▼'; }Alternatively, if
▲/▼are preferred, swap them so▼appears when collapsed (indicating "expand down") and▲when expanded.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/App.css` around lines 302 - 308, The chevron directions are inverted in the CSS: update the pseudo-elements for .collapsible-section.collapsed .collapsible-toggle::before and .collapsible-section.expanded .collapsible-toggle::before so the collapsed state shows the standard "expand" glyph and the expanded state shows the "collapse" glyph — e.g., swap the current characters so collapsed uses '▼' (or '▶' if preferring a right-pointing indicator) and expanded uses '▲'.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/App.css`:
- Around line 302-308: The chevron directions are inverted in the CSS: update
the pseudo-elements for .collapsible-section.collapsed
.collapsible-toggle::before and .collapsible-section.expanded
.collapsible-toggle::before so the collapsed state shows the standard "expand"
glyph and the expanded state shows the "collapse" glyph — e.g., swap the current
characters so collapsed uses '▼' (or '▶' if preferring a right-pointing
indicator) and expanded uses '▲'.
In `@src/components/RulesModal.tsx`:
- Line 209: Add Escape-key handling to the RulesModal so keyboard users can
close it: in the RulesModal component (the element rendering the backdrop with
className "mdc-dialog__backdrop" and the surrounding dialog/container), add an
onKeyDown handler that listens for the Escape key and calls the existing onClose
prop, ensure the dialog container can receive keyboard events by adding a
focusable attribute (e.g., tabIndex={-1}) and set focus to it when opening so
Escape presses are captured; keep the existing backdrop onClick and close button
behavior unchanged.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/components/RulesModal.tsx (1)
80-90: Add an explicit accessible name for the dialog.
role="dialog"on Line 84 should be paired witharia-labelledby(oraria-label) so assistive tech announces a proper dialog name.✅ Proposed fix
<aside id="rules-dialog" data-testid="rules-dialog" className={`mdc-dialog ${isOpen ? 'mdc-dialog--open' : ''}`} role="dialog" aria-modal="true" + aria-labelledby="rules-dialog-title" > @@ - <h1 className="mdc-dialog__header__title">Rules</h1> + <h1 id="rules-dialog-title" className="mdc-dialog__header__title">Rules</h1>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/RulesModal.tsx` around lines 80 - 90, The dialog element with id "rules-dialog" (in RulesModal.tsx) has role="dialog" but no accessible name; add an explicit accessible name by giving the dialog header title element a stable id (e.g., "rules-dialog-title") and add aria-labelledby="rules-dialog-title" to the aside (or alternatively add aria-label on the aside) so assistive technology will announce the dialog title correctly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/RulesModal.tsx`:
- Around line 78-104: The modal currently mounts and parses all markdown and
promo images regardless of visibility; update RulesModal to avoid rendering
heavy content unless visible by checking isOpen before rendering RulesMarkdown
for pocketDragon and mapping promoGames, and change CollapsibleSection to accept
an "expanded" or "lazy" prop (or a render-prop like children={() =>
<RulesMarkdown ...>}) so each promo's RulesMarkdown and image are only created
when that section is actually expanded; modify usage within RulesModal to pass
the expansion state (or conditionally render the child content) so markdown
parsing and image mounting are deferred until the modal or section opens (refer
to RulesModal, isOpen, RulesMarkdown, CollapsibleSection, promoGames,
promoTestIds).
---
Nitpick comments:
In `@src/components/RulesModal.tsx`:
- Around line 80-90: The dialog element with id "rules-dialog" (in
RulesModal.tsx) has role="dialog" but no accessible name; add an explicit
accessible name by giving the dialog header title element a stable id (e.g.,
"rules-dialog-title") and add aria-labelledby="rules-dialog-title" to the aside
(or alternatively add aria-label on the aside) so assistive technology will
announce the dialog title correctly.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (2)
package.jsonsrc/components/RulesModal.tsx
Comment on lines
78
to
104
| export function RulesModal({ isOpen, onClose }: RulesModalProps) { | ||
| return ( | ||
| <aside | ||
| id="rules-dialog" | ||
| data-testid="rules-dialog" | ||
| className={`mdc-dialog ${isOpen ? 'mdc-dialog--open' : ''}`} | ||
| role="dialog" | ||
| aria-modal="true" | ||
| > | ||
| <div className="mdc-dialog__surface rules-surface"> | ||
| <header className="mdc-dialog__header"> | ||
| <h1 className="mdc-dialog__header__title">Rules</h1> | ||
| </header> | ||
| <section className="mdc-dialog__body dialog-body"> | ||
| <div data-testid="rules-content"> | ||
| <RulesMarkdown text={pocketDragon} /> | ||
|
|
||
| {promoGames.map((game: PromoGame) => ( | ||
| <CollapsibleSection | ||
| key={game.title} | ||
| title={game.title} | ||
| imageUrl={game.imageUrl} | ||
| testId={promoTestIds[game.title] || `promo-${game.title.toLowerCase()}`} | ||
| > | ||
| <RulesMarkdown text={game.content} /> | ||
| </CollapsibleSection> | ||
| ))} |
There was a problem hiding this comment.
Defer heavy rules content rendering until the modal/section is actually open.
On Line 78 and Line 34, the component renders/parses all markdown and mounts promo images even when the modal is closed and sections are collapsed. This adds avoidable startup/render cost.
💡 Proposed fix
export function RulesModal({ isOpen, onClose }: RulesModalProps) {
+ if (!isOpen) return null;
+
return (
<aside function CollapsibleSection({ title, imageUrl, testId, children }: CollapsibleSectionProps) {
const [isOpen, setIsOpen] = useState(false);
return (
@@
- <div className="collapsible-body collapsible-content">
- <img src={imageUrl} alt={title} className="promo" />
- {children}
+ <div className="collapsible-body collapsible-content" hidden={!isOpen}>
+ {isOpen && (
+ <>
+ <img src={imageUrl} alt={title} className="promo" loading="lazy" decoding="async" />
+ {children}
+ </>
+ )}
</div>Also applies to: 34-37
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/RulesModal.tsx` around lines 78 - 104, The modal currently
mounts and parses all markdown and promo images regardless of visibility; update
RulesModal to avoid rendering heavy content unless visible by checking isOpen
before rendering RulesMarkdown for pocketDragon and mapping promoGames, and
change CollapsibleSection to accept an "expanded" or "lazy" prop (or a
render-prop like children={() => <RulesMarkdown ...>}) so each promo's
RulesMarkdown and image are only created when that section is actually expanded;
modify usage within RulesModal to pass the expansion state (or conditionally
render the child content) so markdown parsing and image mounting are deferred
until the modal or section opens (refer to RulesModal, isOpen, RulesMarkdown,
CollapsibleSection, promoGames, promoTestIds).
This comment was marked as outdated.
This comment was marked as outdated.
adrianschmidt-bot
added a commit
to adrianschmidt-bot/adrianschmidt-bot.github.io
that referenced
this pull request
Feb 27, 2026
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
Collaborator
Author
|
Also deployed to the preview site: https://adrianschmidt-bot.github.io/ |
There was a problem hiding this comment.
♻️ Duplicate comments (1)
package.json (1)
17-18:⚠️ Potential issue | 🟠 MajorAdd a hard precondition before publishing from
dist.
publishApp(Line 18) still assumesdistis themasterworktree. If that assumption is false, this can push unintended branch state toorigin/master.Suggested hardening
- "publishApp": "npm run build && cd dist && git add --all && (git diff --cached --quiet || git commit -m \"Publishing to gh-pages\") && git push --force-with-lease origin master && cd ..", + "publishApp": "npm run build && git -C dist rev-parse --is-inside-work-tree >/dev/null 2>&1 && test \"$(git -C dist rev-parse --abbrev-ref HEAD)\" = \"master\" && cd dist && git add --all && (git diff --cached --quiet || git commit -m \"Publishing to gh-pages\") && git push --force-with-lease origin master && cd ..",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package.json` around lines 17 - 18, The publishApp script assumes dist is a worktree checked out to master; add a hard precondition in publishApp to verify that dist exists as a git worktree and that its current branch/HEAD is master (or the expected branch) and exit non‑zero if not. Specifically, in the publishApp flow (referencing the npm scripts "setupPublishBranch" and "publishApp") run a Git check inside dist to ensure it's a git repo/worktree and that git -C dist rev-parse --abbrev-ref HEAD (or equivalent) returns "master" before performing add/commit/push; if the check fails, print a clear error and abort to prevent pushing the wrong branch.
🧹 Nitpick comments (2)
src/App.css (2)
236-236: Consider extracting duplicatedmax-heightvalue to a CSS variable.The
calc(100vh - 12rem)value appears in both.dialog-body(line 236) and.collapsible-content .promo(line 327). Extracting this to a custom property would improve maintainability.♻️ Suggested refactor
Add to your
:rootor equivalent::root { --dialog-content-max-height: calc(100vh - 12rem); }Then update both usages:
.dialog-body { font-family: Arial, sans-serif; - max-height: calc(100vh - 12rem); + max-height: var(--dialog-content-max-height); overflow-y: auto;.collapsible-content .promo { - max-height: calc(100vh - 12rem); + max-height: var(--dialog-content-max-height); max-width: 100%;Also applies to: 327-327
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/App.css` at line 236, Extract the duplicated max-height value into a CSS custom property and use it in both places: declare a variable (e.g., --dialog-content-max-height) in :root, then replace the hardcoded calc(100vh - 12rem) in .dialog-body and .collapsible-content .promo with the new variable; update any other occurrences to reference the same property to keep behavior identical and improve maintainability.
334-342: Optional: Modern alternative to clearfix.The clearfix hack works but is a legacy pattern. If you don't need IE11 support,
display: flow-rooton the parent achieves the same block formatting context with less code.♻️ Modern alternative
-.clearfix::before, -.clearfix::after { - content: " "; - display: table; -} - -.clearfix::after { - clear: both; -} +.clearfix { + display: flow-root; +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/App.css` around lines 334 - 342, The .clearfix ruleset uses legacy pseudo-elements; replace the two-rule clearfix (::before/::after) with a single modern declaration by setting the .clearfix class to create a block formatting context via display: flow-root; remove the .clearfix::before and .clearfix::after selectors and their rules (or keep them as an optional IE fallback) so the parent element itself establishes the new formatting context.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@package.json`:
- Around line 17-18: The publishApp script assumes dist is a worktree checked
out to master; add a hard precondition in publishApp to verify that dist exists
as a git worktree and that its current branch/HEAD is master (or the expected
branch) and exit non‑zero if not. Specifically, in the publishApp flow
(referencing the npm scripts "setupPublishBranch" and "publishApp") run a Git
check inside dist to ensure it's a git repo/worktree and that git -C dist
rev-parse --abbrev-ref HEAD (or equivalent) returns "master" before performing
add/commit/push; if the check fails, print a clear error and abort to prevent
pushing the wrong branch.
---
Nitpick comments:
In `@src/App.css`:
- Line 236: Extract the duplicated max-height value into a CSS custom property
and use it in both places: declare a variable (e.g.,
--dialog-content-max-height) in :root, then replace the hardcoded calc(100vh -
12rem) in .dialog-body and .collapsible-content .promo with the new variable;
update any other occurrences to reference the same property to keep behavior
identical and improve maintainability.
- Around line 334-342: The .clearfix ruleset uses legacy pseudo-elements;
replace the two-rule clearfix (::before/::after) with a single modern
declaration by setting the .clearfix class to create a block formatting context
via display: flow-root; remove the .clearfix::before and .clearfix::after
selectors and their rules (or keep them as an optional IE fallback) so the
parent element itself establishes the new formatting context.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (3)
package.jsonsrc/App.csssrc/components/RulesModal.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- src/components/RulesModal.tsx
This comment was marked as resolved.
This comment was marked as resolved.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/components/RulesModal.tsx (1)
35-38:⚠️ Potential issue | 🟠 MajorDefer markdown/image rendering until the modal/section is actually open.
The component still mounts/parses all rules markdown and promo images even when the dialog is closed and sections are collapsed, which adds unnecessary render cost. This was already raised earlier and is still present.
💡 Proposed fix
export function RulesModal({ isOpen, onClose }: RulesModalProps) { useEffect(() => { if (!isOpen) return; @@ }, [isOpen, onClose]); + if (!isOpen) return null; + return ( <aside @@ function CollapsibleSection({ title, imageUrl, testId, children }: CollapsibleSectionProps) { const [isOpen, setIsOpen] = useState(false); @@ - <div className="collapsible-body collapsible-content"> - <img src={imageUrl} alt={title} className="promo" /> - {children} + <div className="collapsible-body collapsible-content" hidden={!isOpen}> + {isOpen && ( + <> + <img src={imageUrl} alt={title} className="promo" loading="lazy" decoding="async" /> + {children} + </> + )} </div>Also applies to: 93-119
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/RulesModal.tsx` around lines 35 - 38, The RulesModal renders heavy markdown and promo images eagerly; update RulesModal to conditionally render the markdown children and <img src={imageUrl} .../> only when the modal/section is actually open/expanded (use the modal's isOpen prop/state and the collapsible section's expanded flag) so parsing and image mounting are deferred; locate the render block that contains the "collapsible-body collapsible-content" div and the children/image (and the analogous code referenced around lines 93-119) and wrap those expensive pieces in a conditional (or lazy render helper) that returns null until the modal/section is open.
🧹 Nitpick comments (1)
src/components/RulesModal.tsx (1)
5-5: Use a type-only import forPromoGame.
PromoGameis used only as a type annotation in the.map()callback. While this project doesn't haveverbatimModuleSyntaxenabled in TypeScript config, type-only imports are a best practice that clarifies intent and avoids unnecessary runtime dependencies on type definitions.Proposed fix
-import { pocketDragon, promoGames, PromoGame } from '../rules/game-rules'; +import { pocketDragon, promoGames, type PromoGame } from '../rules/game-rules';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/RulesModal.tsx` at line 5, The import currently brings PromoGame in as a value import which is only used as a type in RulesModal.tsx; change the import to a type-only import so the compiler treats PromoGame as a type and it is removed from runtime bundles (replace the import for PromoGame with an `import type { PromoGame } from '../rules/game-rules'` while leaving pocketDragon and promoGames as normal imports).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/components/RulesModal.tsx`:
- Around line 35-38: The RulesModal renders heavy markdown and promo images
eagerly; update RulesModal to conditionally render the markdown children and
<img src={imageUrl} .../> only when the modal/section is actually open/expanded
(use the modal's isOpen prop/state and the collapsible section's expanded flag)
so parsing and image mounting are deferred; locate the render block that
contains the "collapsible-body collapsible-content" div and the children/image
(and the analogous code referenced around lines 93-119) and wrap those expensive
pieces in a conditional (or lazy render helper) that returns null until the
modal/section is open.
---
Nitpick comments:
In `@src/components/RulesModal.tsx`:
- Line 5: The import currently brings PromoGame in as a value import which is
only used as a type in RulesModal.tsx; change the import to a type-only import
so the compiler treats PromoGame as a type and it is removed from runtime
bundles (replace the import for PromoGame with an `import type { PromoGame }
from '../rules/game-rules'` while leaving pocketDragon and promoGames as normal
imports).
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/App.csssrc/components/RulesModal.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- src/App.css
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
Collaborator
Author
|
Fixed the dialog height issue! Switched from |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/App.tsx (1)
379-414: Consider adding focus management for better keyboard accessibility.The dialog has proper ARIA attributes and Escape key handling, but for full WAI-ARIA compliance:
- Move focus to the dialog (or first focusable element) when it opens
- Trap focus within the dialog while open
- Return focus to the triggering element when closed
Example: auto-focus the action button on dialog open
+ const gameOverButtonRef = useRef<HTMLButtonElement>(null); + + // Focus the game-over button when dialog opens + useEffect(() => { + if (state.gameResult && gameOverButtonRef.current) { + gameOverButtonRef.current.focus(); + } + }, [state.gameResult]);Then pass the ref to the button:
<PdButton data-testid="game-over-button" label={state.gameResult.buttonLabel} primary className="mdc-dialog__footer__button--accept" onClick={closeGameOver} + ref={gameOverButtonRef} />Note: This requires
PdButtonto forward refs.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/App.css (1)
309-321: Optional cleanup: remove duplicatedfont-sizein state-specific chevron rules.
font-size: 1remis already defined in the shared::beforeblock, so Line 315 and Line 320 can be dropped to reduce repetition.♻️ Proposed cleanup
.collapsible-section.collapsed .collapsible-toggle::before { - font-size: 1rem; content: '▶'; } .collapsible-section.expanded .collapsible-toggle::before { - font-size: 1rem; content: '▼'; }
There was a problem hiding this comment.
🧹 Nitpick comments (4)
src/components/RulesModal.tsx (1)
80-90: Addaria-labelledbyto associate the dialog with its title.The dialog has
role="dialog"andaria-modal="true", but lacks an association with its visible title. This helps screen readers announce the dialog purpose.💡 Proposed fix
<aside id="rules-dialog" data-testid="rules-dialog" className={`mdc-dialog ${isOpen ? 'mdc-dialog--open' : ''}`} role="dialog" aria-modal="true" + aria-labelledby="rules-dialog-title" > <div className="mdc-dialog__surface rules-surface"> <header className="mdc-dialog__header"> - <h1 className="mdc-dialog__header__title">Rules</h1> + <h1 id="rules-dialog-title" className="mdc-dialog__header__title">Rules</h1> </header>src/App.tsx (3)
32-50: Consider wrapping localStorage access in try-catch.
localStoragecan throw in private browsing mode or when storage quota is exceeded. While rare, this could prevent the app from loading.💡 Optional defensive fix
useEffect(() => { + try { const stored = localStorage.getItem('soundEnabled'); if (stored !== null) { setSoundEnabled(stored === 'true'); } + } catch { + // localStorage unavailable (private browsing, quota exceeded) + } }, []);
90-97: Guard check uses potentially stale state reference.The
isFeedingAllowed(state)check on line 92 uses capturedstate, while the setState usesprev. In fast interactions, the guard could allow/deny based on stale state. Consider moving the guard inside the functional update.💡 Optional improvement
const feed = useCallback(() => { - if (!isFeedingAllowed(state)) return; - setState(prev => ({ - ...prev, - feedTimer: calculateFeedReset(prev.difficulty.initialFeedTimer, prev.feedTimer), - })); - }, [state]); + setState(prev => { + if (!isFeedingAllowed(prev)) return prev; + return { + ...prev, + feedTimer: calculateFeedReset(prev.difficulty.initialFeedTimer, prev.feedTimer), + }; + }); + }, []);This applies similarly to
logSuccess,useGeneralClue, anduseSpecificClue.
380-392: Addaria-labelledbyfor consistency with the rules modal.The game-over dialog has
role="dialog"andaria-modal="true"but lacks association with its title heading.💡 Proposed fix
<aside id="game-over-dialog" data-testid="game-over-dialog" className="mdc-dialog mdc-dialog--open" role="dialog" aria-modal="true" + aria-labelledby="game-over-dialog-title" > <div className="mdc-dialog__surface"> <header className="mdc-dialog__header"> - <h2 className="mdc-dialog__header__title" data-testid="game-over-heading"> + <h2 id="game-over-dialog-title" className="mdc-dialog__header__title" data-testid="game-over-heading"> {state.gameResult.heading} </h2> </header>
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/App.csssrc/App.tsxsrc/components/RulesModal.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- src/App.css
adrianschmidt
previously approved these changes
Feb 28, 2026
Complete rewrite of the Pocket Dragon companion app: Tech stack: - React 19 + TypeScript + Vite 7 (was: StencilJS) - VitePWA with Workbox for service worker (was: Workbox 4 via CDN) - Vitest for unit tests, Playwright for e2e tests App features (full parity with original): - Difficulty selection (Easy/Medium/Hard) - Game and feed timers with countdown - Feed button enabled at ≤30s with audio alerts at 30/20/10s - Sound toggle with localStorage persistence - Success logging with win/loss conditions - General/specific clue system with costs and regeneration - Game over dialog with scoring (base + time points) - Rules modal with collapsible promo sections Tests: - 60 unit tests (difficulty config, game state logic) - 55 e2e tests (clues, difficulty, feeding, game-over, rules, sound, success logging, timers) Changes from original: - Dropped Google Analytics (UA property, obsolete) - Custom CSS dialog replaces MDC Dialog dependency - PWA manifest now includes 512px and maskable icons - Font loaded via CSS @font-face instead of external stylesheet
adrianschmidt-bot
force-pushed
the
pocket-dragon-v2
branch
from
c34b306 to
6c0103e
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.gitignore (1)
3-3:⚠️ Potential issue | 🔴 CriticalRemove the catch-all ignore rule (
*) or scope it narrowly.At Line 3,
*ignores effectively every untracked file, which can silently prevent new app files from being committed. This is risky for ongoing development and CI changes.Suggested fix
-* +# Avoid global catch-all ignores; keep explicit patterns only.
♻️ Duplicate comments (1)
package.json (1)
17-18:⚠️ Potential issue | 🟠 MajorAdd a worktree/branch precondition before publish push.
Line 18 still assumes
distis themasterworktree. If setup is skipped ordistpoints elsewhere, publish can push the wrong state.Suggested hardening
- "publishApp": "npm run build && cd dist && git add --all && (git diff --cached --quiet || git commit -m \"Publishing to gh-pages\") && git push --force-with-lease origin master && cd ..", + "publishApp": "npm run build && test \"$(git -C dist rev-parse --is-inside-work-tree 2>/dev/null)\" = \"true\" && test \"$(git -C dist rev-parse --abbrev-ref HEAD)\" = \"master\" && cd dist && git add --all && (git diff --cached --quiet || git commit -m \"Publishing to gh-pages\") && git push --force-with-lease origin master && cd ..",
🧹 Nitpick comments (12)
src/styles/global.css (1)
4-10: Consider addingfont-display: swapto the@font-facedeclaration.Without
font-display, browsers may hide text until the custom font loads (FOIT). Usingswapensures text remains visible with a fallback font during loading, improving perceived performance—especially important for PWA/offline scenarios.Proposed fix
`@font-face` { font-family: 'cartoons_123regular'; src: url('/assets/font/cartoons_123-webfont.woff2') format('woff2'), url('/assets/font/cartoons_123-webfont.woff') format('woff'); font-weight: normal; font-style: normal; + font-display: swap; }e2e/tests/difficulty.spec.ts (2)
19-23: Outdated comment: StencilJS reference in React codebase.The comment mentions "StencilJS reflects boolean attributes" but this PR rewrites the app to React. The assertion pattern may still work, but the comment is misleading and should be updated to reflect the current implementation.
💡 Suggested fix
test('Easy button is selected by default', async ({ page }) => { const easyButton = page.locator(selectors.easyButton); - // StencilJS reflects boolean attributes as empty strings + // React reflects boolean attributes as empty strings await expect(easyButton).toHaveAttribute('selected', ''); });
49-57: Same outdated StencilJS comment.Line 53 also references StencilJS behavior. Consider updating or removing these comments for consistency with the React rewrite.
e2e/tests/sound.spec.ts (2)
28-31: Outdated comment: MDC reference in React codebase.The comment mentions "MDC removes aria-pressed or changes aria-label" but MDC has been removed in this rewrite. The test logic may still be valid, but the comment should reflect the current React implementation.
💡 Suggested fix
// Click to toggle await soundToggle.click(); - // Should now be disabled - MDC removes aria-pressed or changes aria-label - // Check by aria-label instead (more reliable for MDC toggle) + // Should now be disabled - check via aria-label await expect(soundToggle).toHaveAttribute('aria-label', 'Enable sound');
94-97: Fragile parent selector using.locator('..').Using
feedTimer.locator('..')to get the parent container is fragile and depends on the exact DOM structure. If the component structure changes, this test will break silently or target the wrong element.Consider adding a dedicated
data-testidfor the feed timer container in the component and the selectors fixture.e2e/tests/feeding.spec.ts (2)
45-48: Redundanttest.slow()call in nested describe block.Line 47 calls
test.slow()inside a nested describe block, but line 6 already appliestest.slow()to the parent describe block. The nested call is redundant since slow mode is inherited.💡 Suggested fix
test.describe('Feed timer reset behavior', () => { - // Note: This test is slow as we need to wait for the timer to reach the feed threshold - test.slow(); - test('feeding resets timer using formula: initialTimer - currentTimer', async ({ page }) => {
109-112: Fragile parent selector using.locator('..').Same pattern as noted in
sound.spec.ts- usingfeedTimer.locator('..')depends on exact DOM structure. Consider adding a dedicateddata-testidfor the feed timer container.src/components/RulesModal.tsx (1)
65-105: Consider deferring heavy content rendering until modal/section is open.The component renders all
RulesMarkdowncontent and mounts promo images even when the modal is closed. While<details>visually hides collapsed content, the markdown is still parsed and children are mounted on initial render.For a "Chill" review, this is an optional optimization - consider deferring if performance becomes a concern with more promo content.
💡 Optional lazy rendering approach
export function RulesModal({ isOpen, onClose }: RulesModalProps) { + if (!isOpen) return null; + useEffect(() => {This would prevent mounting the entire modal tree until opened. The trade-off is a brief delay on first open.
e2e/tests/rules-modal.spec.ts (1)
67-74: Consider using selectors fixture for promo test IDs.Lines 68-70 use hardcoded selectors (
[data-testid="promo-anachrony"], etc.) instead of the centralizedselectorsfixture. For consistency with other tests, consider adding these to the fixture.src/App.tsx (2)
163-181: Sound alerts readsoundEnabledfrom closure insidesetState.The
setStatecallback at line 171 readssoundEnabledfrom the outer closure. While this works because the effect re-runs whensoundEnabledchanges, mixing side effects (audio playback) inside a state updater is unconventional. Consider extracting the sound alerts outsidesetStateusing a separate effect or ref to avoid coupling state updates with side effects.
346-357: Consider using<button>instead of<a href="#">for the rules link.Using
<a href="#">withpreventDefaultfor an action (opening the modal) is semantically incorrect. Buttons are the appropriate element for actions that don't navigate. This also avoids the need forpreventDefault.♻️ Proposed fix
<p className="rules-version"> - <a - href="#" + <button + type="button" data-testid="rules-link" - onClick={(e) => { - e.preventDefault(); - setRulesOpen(true); - }} + className="rules-link-button" + onClick={() => setRulesOpen(true)} > Rules version: 1.0.1 - </a> + </button> </p>You'll need to add styling for
.rules-link-buttonto match the current appearance (e.g.,background: none; border: none; color: inherit; text-decoration: underline; cursor: pointer;).e2e/fixtures/selectors.ts (1)
74-82: Same sync concern applies togameConstants.These values mirror constants from
src/logic/gameState.ts. Consider adding a comment noting which source constants these map to, or creating a shared constants file imported by both source and tests to prevent drift.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (58)
package-lock.jsonis excluded by!**/package-lock.jsonpublic/assets/font/cartoons_123-webfont.woffis excluded by!**/*.woffpublic/assets/font/cartoons_123-webfont.woff2is excluded by!**/*.woff2public/assets/icon/android-icon-144x144.pngis excluded by!**/*.pngpublic/assets/icon/android-icon-192x192.pngis excluded by!**/*.pngpublic/assets/icon/android-icon-36x36.pngis excluded by!**/*.pngpublic/assets/icon/android-icon-48x48.pngis excluded by!**/*.pngpublic/assets/icon/android-icon-512x512.pngis excluded by!**/*.pngpublic/assets/icon/android-icon-72x72.pngis excluded by!**/*.pngpublic/assets/icon/android-icon-96x96.pngis excluded by!**/*.pngpublic/assets/icon/apple-icon-114x114.pngis excluded by!**/*.pngpublic/assets/icon/apple-icon-120x120.pngis excluded by!**/*.pngpublic/assets/icon/apple-icon-144x144.pngis excluded by!**/*.pngpublic/assets/icon/apple-icon-152x152.pngis excluded by!**/*.pngpublic/assets/icon/apple-icon-180x180.pngis excluded by!**/*.pngpublic/assets/icon/apple-icon-57x57.pngis excluded by!**/*.pngpublic/assets/icon/apple-icon-60x60.pngis excluded by!**/*.pngpublic/assets/icon/apple-icon-72x72.pngis excluded by!**/*.pngpublic/assets/icon/apple-icon-76x76.pngis excluded by!**/*.pngpublic/assets/icon/apple-icon-precomposed.pngis excluded by!**/*.pngpublic/assets/icon/apple-icon.pngis excluded by!**/*.pngpublic/assets/icon/favicon-16x16.pngis excluded by!**/*.pngpublic/assets/icon/favicon-32x32.pngis excluded by!**/*.pngpublic/assets/icon/favicon-96x96.pngis excluded by!**/*.pngpublic/assets/icon/favicon.icois excluded by!**/*.icopublic/assets/icon/icon-source.pngis excluded by!**/*.pngpublic/assets/icon/ms-icon-144x144.pngis excluded by!**/*.pngpublic/assets/icon/ms-icon-150x150.pngis excluded by!**/*.pngpublic/assets/icon/ms-icon-310x310.pngis excluded by!**/*.pngpublic/assets/icon/ms-icon-70x70.pngis excluded by!**/*.pngpublic/assets/promo-anachrony.jpgis excluded by!**/*.jpgpublic/assets/promo-daysOfIre.jpgis excluded by!**/*.jpgpublic/assets/promo-diceSettlers.jpgis excluded by!**/*.jpgpublic/assets/promo-kitchenRush.jpgis excluded by!**/*.jpgpublic/assets/promo-microfilms.jpgis excluded by!**/*.jpgpublic/assets/promo-nightsOfFire.jpgis excluded by!**/*.jpgpublic/assets/promo-petrichor.jpgis excluded by!**/*.jpgpublic/assets/promo-redacted.jpgis excluded by!**/*.jpgpublic/assets/promo-tashKalar.jpgis excluded by!**/*.jpgpublic/assets/promo-trickerion.jpgis excluded by!**/*.jpgpublic/assets/sound/beep-hightone-2x.mp3is excluded by!**/*.mp3public/assets/sound/beep-hightone-2x.oggis excluded by!**/*.oggpublic/assets/sound/beep-hightone-2x.wavis excluded by!**/*.wavpublic/assets/sound/beep-hightone-3x.mp3is excluded by!**/*.mp3public/assets/sound/beep-hightone-3x.oggis excluded by!**/*.oggpublic/assets/sound/beep-hightone-3x.wavis excluded by!**/*.wavpublic/assets/sound/beep-hightone.mp3is excluded by!**/*.mp3public/assets/sound/beep-hightone.oggis excluded by!**/*.oggpublic/assets/sound/beep-hightone.wavis excluded by!**/*.wavpublic/assets/sound/cling_2-2x.mp3is excluded by!**/*.mp3public/assets/sound/cling_2-2x.oggis excluded by!**/*.oggpublic/assets/sound/cling_2-2x.wavis excluded by!**/*.wavpublic/assets/sound/cling_2-3x.mp3is excluded by!**/*.mp3public/assets/sound/cling_2-3x.oggis excluded by!**/*.oggpublic/assets/sound/cling_2-3x.wavis excluded by!**/*.wavpublic/assets/sound/cling_2.mp3is excluded by!**/*.mp3public/assets/sound/cling_2.oggis excluded by!**/*.oggpublic/assets/sound/cling_2.wavis excluded by!**/*.wav
📒 Files selected for processing (61)
.github/actions/set-up-node/action.yml.github/workflows/ci.yml.github/workflows/nodejs.yml.github/workflows/up-check.yml.gitignore.nvmrc.prettierrcLICENSES.txte2e/fixtures/selectors.tse2e/tests/clues.spec.tse2e/tests/difficulty.spec.tse2e/tests/feeding.spec.tse2e/tests/game-over.spec.tse2e/tests/rules-modal.spec.tse2e/tests/sound.spec.tse2e/tests/success.spec.tse2e/tests/timers.spec.tseslint.jsindex.htmlpackage.jsonplaywright.config.tssrc/App.csssrc/App.tsxsrc/assets/font/generator_config.txtsrc/assets/font/stylesheet.csssrc/browserconfig.xmlsrc/components/PdButton.csssrc/components/PdButton.tsxsrc/components/RulesModal.tsxsrc/components/app-collapsible-section/app-collapsible-section.scsssrc/components/app-collapsible-section/app-collapsible-section.tsxsrc/components/app-home/app-home.spec.tssrc/components/app-home/app-home.tsxsrc/components/app-root/app-root.scsssrc/components/app-root/app-root.spec.tssrc/components/app-root/app-root.tsxsrc/components/app-rules/app-rules.scsssrc/components/app-rules/app-rules.tsxsrc/components/button/button.scsssrc/components/button/button.tsxsrc/components/settings/settings.scsssrc/components/settings/settings.tsxsrc/global/app.csssrc/index.htmlsrc/logic/difficulty.test.tssrc/logic/difficulty.tssrc/logic/gameState.test.tssrc/logic/gameState.tssrc/main.tsxsrc/manifest.jsonsrc/rules/game-rules.tssrc/services/storage.tssrc/styles/global.csssrc/styles/variables.csssrc/variables.scsssrc/vite-env.d.tsstencil.config.tstsconfig.jsontsconfig.node.jsontslint.jsvite.config.ts
💤 Files with no reviewable changes (26)
- src/components/app-collapsible-section/app-collapsible-section.scss
- src/components/settings/settings.scss
- src/assets/font/stylesheet.css
- src/global/app.css
- src/components/app-root/app-root.scss
- eslint.js
- src/components/app-collapsible-section/app-collapsible-section.tsx
- src/browserconfig.xml
- src/components/app-rules/app-rules.scss
- src/components/button/button.tsx
- src/components/app-home/app-home.tsx
- src/components/app-home/app-home.spec.ts
- src/components/app-root/app-root.tsx
- tslint.js
- src/components/settings/settings.tsx
- src/components/app-rules/app-rules.tsx
- src/manifest.json
- .prettierrc
- src/variables.scss
- src/components/app-root/app-root.spec.ts
- .github/workflows/nodejs.yml
- src/services/storage.ts
- src/components/button/button.scss
- stencil.config.ts
- src/assets/font/generator_config.txt
- src/index.html
🚧 Files skipped from review as they are similar to previous changes (16)
- tsconfig.node.json
- src/logic/difficulty.test.ts
- vite.config.ts
- src/logic/difficulty.ts
- src/main.tsx
- src/logic/gameState.test.ts
- src/vite-env.d.ts
- e2e/tests/success.spec.ts
- src/App.css
- .github/workflows/up-check.yml
- index.html
- src/logic/gameState.ts
- .nvmrc
- e2e/tests/clues.spec.ts
- playwright.config.ts
- .github/actions/set-up-node/action.yml
Comment on lines
+38
to
+58
| test('losing game shows 0 time points', async ({ page }) => { | ||
| const logSuccessButton = page.locator(selectors.logSuccessButton); | ||
| const feedTimer = page.locator(selectors.feedTimer); | ||
| const gameOverDialog = page.locator(selectors.gameOverDialog); | ||
| const timePoints = page.locator(selectors.timePoints); | ||
|
|
||
| // Start the game | ||
| await page.locator(selectors.startPauseButton).click(); | ||
|
|
||
| // Wait for feed timer to reach 0 | ||
| await expect(async () => { | ||
| const timer = await feedTimer.textContent(); | ||
| expect(parseInt(timer!)).toBeLessThanOrEqual(0); | ||
| }).toPass({ timeout: 125000 }); | ||
|
|
||
| // Game over dialog should appear | ||
| await expect(gameOverDialog).toBeVisible(); | ||
|
|
||
| // Time points should show 0 for losing | ||
| await expect(timePoints).toContainText('0'); | ||
| }); |
There was a problem hiding this comment.
Unused variable logSuccessButton.
Line 39 declares logSuccessButton but it's never used in this test. This appears to be leftover from a copy-paste.
🔧 Proposed fix
test('losing game shows 0 time points', async ({ page }) => {
- const logSuccessButton = page.locator(selectors.logSuccessButton);
const feedTimer = page.locator(selectors.feedTimer);
const gameOverDialog = page.locator(selectors.gameOverDialog);
const timePoints = page.locator(selectors.timePoints);📝 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
| test('losing game shows 0 time points', async ({ page }) => { | |
| const logSuccessButton = page.locator(selectors.logSuccessButton); | |
| const feedTimer = page.locator(selectors.feedTimer); | |
| const gameOverDialog = page.locator(selectors.gameOverDialog); | |
| const timePoints = page.locator(selectors.timePoints); | |
| // Start the game | |
| await page.locator(selectors.startPauseButton).click(); | |
| // Wait for feed timer to reach 0 | |
| await expect(async () => { | |
| const timer = await feedTimer.textContent(); | |
| expect(parseInt(timer!)).toBeLessThanOrEqual(0); | |
| }).toPass({ timeout: 125000 }); | |
| // Game over dialog should appear | |
| await expect(gameOverDialog).toBeVisible(); | |
| // Time points should show 0 for losing | |
| await expect(timePoints).toContainText('0'); | |
| }); | |
| test('losing game shows 0 time points', async ({ page }) => { | |
| const feedTimer = page.locator(selectors.feedTimer); | |
| const gameOverDialog = page.locator(selectors.gameOverDialog); | |
| const timePoints = page.locator(selectors.timePoints); | |
| // Start the game | |
| await page.locator(selectors.startPauseButton).click(); | |
| // Wait for feed timer to reach 0 | |
| await expect(async () => { | |
| const timer = await feedTimer.textContent(); | |
| expect(parseInt(timer!)).toBeLessThanOrEqual(0); | |
| }).toPass({ timeout: 125000 }); | |
| // Game over dialog should appear | |
| await expect(gameOverDialog).toBeVisible(); | |
| // Time points should show 0 for losing | |
| await expect(timePoints).toContainText('0'); | |
| }); |
Comment on lines
+91
to
+97
| const feed = useCallback(() => { | ||
| if (!isFeedingAllowed(state)) return; | ||
| setState(prev => ({ | ||
| ...prev, | ||
| feedTimer: calculateFeedReset(prev.difficulty.initialFeedTimer, prev.feedTimer), | ||
| })); | ||
| }, [state]); |
There was a problem hiding this comment.
Stale closure risk in feed callback.
The feed callback captures state in its dependency array and uses it for the isFeedingAllowed check. However, if feed is called rapidly or state changes between calls, the check may use outdated state. Consider using the functional updater pattern consistently:
🔧 Proposed fix
const feed = useCallback(() => {
- if (!isFeedingAllowed(state)) return;
setState(prev => {
+ if (!isFeedingAllowed(prev)) return prev;
return {
...prev,
feedTimer: calculateFeedReset(prev.difficulty.initialFeedTimer, prev.feedTimer),
};
});
- }, [state]);
+ }, []);📝 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
| const feed = useCallback(() => { | |
| if (!isFeedingAllowed(state)) return; | |
| setState(prev => ({ | |
| ...prev, | |
| feedTimer: calculateFeedReset(prev.difficulty.initialFeedTimer, prev.feedTimer), | |
| })); | |
| }, [state]); | |
| const feed = useCallback(() => { | |
| setState(prev => { | |
| if (!isFeedingAllowed(prev)) return prev; | |
| return { | |
| ...prev, | |
| feedTimer: calculateFeedReset(prev.difficulty.initialFeedTimer, prev.feedTimer), | |
| }; | |
| }); | |
| }, []); |
Comment on lines
+117
to
+132
| const useGeneralClue = useCallback(() => { | ||
| if (!isGeneralClueAllowed(state)) return; | ||
| setState(prev => ({ | ||
| ...prev, | ||
| remainingClues: prev.remainingClues - GENERAL_CLUE_COST, | ||
| })); | ||
| }, [state]); | ||
|
|
||
| // Use specific clue | ||
| const useSpecificClue = useCallback(() => { | ||
| if (!isSpecificClueAllowed(state)) return; | ||
| setState(prev => ({ | ||
| ...prev, | ||
| remainingClues: prev.remainingClues - SPECIFIC_CLUE_COST, | ||
| })); | ||
| }, [state]); |
There was a problem hiding this comment.
Same stale closure pattern in clue callbacks.
Both useGeneralClue and useSpecificClue have the same stale closure risk as feed. Consider moving the guard inside setState using the functional updater pattern for consistency with logSuccess.
Comment on lines
+42
to
+65
| <defs> | ||
| <filter id="pd-shadow"> | ||
| <feDropShadow | ||
| dx="0" | ||
| dy="3" | ||
| stdDeviation="0.5" | ||
| floodColor="#000000" | ||
| floodOpacity="0.2" | ||
| /> | ||
| <feDropShadow | ||
| dx="0" | ||
| dy="2" | ||
| stdDeviation="1" | ||
| floodColor="#000000" | ||
| floodOpacity="0.14" | ||
| /> | ||
| <feDropShadow | ||
| dx="0" | ||
| dy="1" | ||
| stdDeviation="1.5" | ||
| floodColor="#000000" | ||
| floodOpacity="0.12" | ||
| /> | ||
| </filter> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check how many PdButton instances are rendered in App.tsx
echo "=== Counting PdButton usages in App.tsx ==="
rg "PdButton" src/App.tsx || echo "Pattern not found in App.tsx"
echo -e "\n=== Searching for all filter definitions ==="
rg "id=\"pd-shadow\"" src/ || echo "Filter ID not found"
echo -e "\n=== Examining PdButton.tsx file size and content ==="
wc -l src/components/PdButton.tsx
echo -e "\n=== Reading PdButton.tsx to understand filter implementation ==="
cat -n src/components/PdButton.tsxRepository: pocket-dragon/pocket-dragon.github.io
Length of output: 3052
Use unique SVG filter IDs for each PdButton instance to avoid duplicate IDs in the DOM.
The component renders a <filter id="pd-shadow"> inside every PdButton's SVG. With 9+ button instances on the page, this creates duplicate IDs, which violates HTML standards. While the filter may still render due to browser tolerance, CSS selectors or accessibility tools may not reliably reference url("#pd-shadow"). Generate a unique ID per instance (e.g., using a UUID or index) and update the corresponding CSS filter reference accordingly.
Comment on lines
+129
to
+130
| // Wait for timer to decrement a couple of seconds | ||
| await expect(gameTimer).toHaveText(String(initialGameTime - 2), { timeout: 5000 }); |
There was a problem hiding this comment.
Exact timer value assertion may cause flakiness.
Waiting for the timer to equal exactly initialGameTime - 2 (298) is fragile—if CI timing varies, the timer might tick past 298 before the assertion evaluates, causing test failure.
Since the goal is simply "let some time pass before reset," consider a more robust approach:
Suggested fix using the same pattern as other tests
// Wait for timer to decrement a couple of seconds
- await expect(gameTimer).toHaveText(String(initialGameTime - 2), { timeout: 5000 });
+ await expect(gameTimer).not.toHaveText(String(initialGameTime), { timeout: 3000 });Or if you need to ensure multiple ticks have occurred:
// Wait for at least 2 seconds of decrement
await expect.poll(async () => {
const text = await gameTimer.textContent();
return parseInt(text!);
}, { timeout: 5000 }).toBeLessThanOrEqual(initialGameTime - 2);
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.
Complete rewrite of the Pocket Dragon companion app with a modern tech stack.
Tech Stack
What Changed
@font-faceinstead of external stylesheetTesting
Service Worker Migration
The new SW uses the same
/sw.jspath. Browsers detect the change automatically and the new SW activates viaskipWaiting+clientsClaim. Existing installed apps update on next online visit — worst case users need one reload.Notes
.mp3files excluded from precache (GitHub Pages returns HTTP 206 for audio, which Firefox rejects in Cache.put). The app falls back to.ogg/.wavformats offline.Summary by CodeRabbit
New Features
Refactor
Style
Tests
Chores