[evanpelle]

Regression fix for #5009 (7ae09256b). No approved issue — reporting and fixing in one go.

Description:

Since #5009, opening an inline page modal (Store, Settings, Leaderboard, Help, ...) scrolls the whole document instead of scrolling inside the modal.

Cause. Those modals are the .page-content elements in index.html, sized w-full h-full. They used to get a bounded height from the locked chain #5009 removed: body h-full overflow-hidden → content div h-full overflow-hidden → main-layout's overflow-y-auto. Unbounding that chain is correct for the homepage (document-level scrolling, needed for ad viewability), but it also let the page modals grow to their full content height, so o-modal's [data-modal-scroll] container had nothing left to scroll. Measured at main with Help open: document scrollHeight 7353px, modal container clientHeight == scrollHeight == 7034.

Fix. Re-lock the layout only while an inline page is open, keyed off a page-open body class — the same pattern as the existing in-game class:

• src/client/Navigation.ts — showPage() toggles body.page-open. It's the single place page visibility changes; BaseModal.open()/close() for inline modals route through it.
• src/client/styles.css — body.page-open { height: 100%; overflow: hidden; flex-direction: column; flex-wrap: nowrap }. The column matters: Make homepage scrollable and shift UI below the header ad #5009 made body a wrapping flex row so the Playwire header-ad slot (#pw-oop-flex_container) gets its own full-width line. A column lets the content area take the height remaining under that slot rather than overflowing by its height.
• index.html — content area gets in-[.page-open]:min-h-0 in-[.page-open]:overflow-hidden; the ad slot gets flex-basis: auto; flex-shrink: 0 under body.page-open, since its usual flex-basis: 100% would mean 100% height in a column.
• src/client/components/MainLayout.ts — min-h-0 on main and the inner column so they can shrink to the bounded height. This is what the removed overflow-hidden / overflow-y-auto implicitly gave them; inert on the homepage, whose chain is auto-height.

The homepage keeps scrolling at the document level exactly as #5009 intends — the class is only present while a page modal is open.

Verification

Headless Chromium against the dev server, at 1400×900 and 390×844:

• Repro at main: Help open → document scrollHeight 7353 / 10470, window.scrollTo(0, 5000) moves the page, modal container doesn't scroll.
• With this PR: Help open → scrollHeight == innerHeight (900 / 844), window.scrollTo doesn't move, modal container scrolls (7034px of content in a 581px box; 10284 in 658 on mobile), footer pinned at the viewport bottom.
• Back to Play: class removed, document scrolls again.
• Header ad interaction: simulated a 250px #pw-oop-flex unit in its slot with a page open → slot occupies 0–250, nav sits at 250, the modal's scroll box shrinks to 331px and still scrolls, footer still at the viewport bottom, no document scroll.
• 10 navigation-related client test files (72 tests) pass; Prettier + ESLint clean.

Not covered: the real Playwire creative (RAMP won't initialize headless). The simulation above reproduces the geometry #5009 describes, but a headed pass with ads live is worth a glance.

Please complete the following:

• I have added screenshots for all UI updates — no visual change; the fix is scroll behavior, which the measurements above capture
• I process any text displayed to the user through translateText() and I've added it to the en.json file — N/A, no user-visible text
• I have added relevant tests to the test directory — N/A, pure CSS/layout behavior; verified e2e as above

Please put your Discord username so you can be contacted if a bug or regression is found:

evanpelle

🤖 Generated with Claude Code

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 4bc97091-2a77-40e9-b37d-d35585b87d1a

📥 Commits

Reviewing files that changed from the base of the PR and between 9f2d4c2 and fe0617c.

📒 Files selected for processing (1)

• src/client/Navigation.ts

Included review availability: Your plan provides up to 8 included reviews per hour; 6 remain after this review.

---

Walkthrough

Inline page navigation now applies page-open to the body. CSS rules lock document scrolling, stack body content, allow layout containers to shrink, clip inline content, and size the Playwire ad container to its content.

Changes

Inline page layout

Layer / File(s)	Summary
Page state and body layout src/client/Navigation.ts, src/client/styles.css	showPage toggles page-open after modal cleanup for non-play pages. The body rule locks document scrolling and uses a non-wrapping column layout.
Content and ad sizing src/client/components/MainLayout.ts, index.html	Main layout containers can shrink. Inline content clips overflow. The Playwire flex ad container sizes to its content when page-open is active.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Merge Risk: ⚪ Minimal · up to fe061

This change restores scrolling inside inline page modals while preserving document-level scrolling on the homepage. No actionable merge-blocking risk remains beyond normal checks and review.

Suggested reviewers: ryanbarlow97

Poem

A page opens, the body aligns,
The scroll stays in its proper lines.
Ads fit close, containers bend,
Overflow meets a tidy end.
The play page clears the state again.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 2 files.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly and concisely describes the primary fix: restoring scrolling inside inline page modals instead of the document.
Description check	✅ Passed	The description directly explains the regression, root cause, implementation, and verification for the changeset.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

• Fix all pre-merge checks with AI

✨ Finishing Touches 💡 1

🛠️ Fix failing CI checks 💡

• Create stacked PR
• Commit on current branch

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[github-actions]

🤖 Claude Code Review

Verdict: The core mechanism is sound, but there's a real re-entrancy bug that undoes the fix on the most common navigation path. Findings: 1 High, 0 Medium, 0 Low.

High

src/client/Navigation.ts:19 — page-open gets cleared again when navigating directly from one inline page modal to another, so the exact regression this PR fixes (document scrolling instead of the modal) still occurs on that path.

The new toggle runs early in showPage():

window.currentPageId = pageId;                                             // Navigation.ts:17
document.body.classList.toggle("page-open", pageId !== "page-play");       // Navigation.ts:19 (new)
...
const visibleModal = document.querySelector(".page-content:not(.hidden)"); // Navigation.ts:24ish
if (visibleModal) {
  if (... visibleModal.isOpen() ...) {
    visibleModal.close();                                                  // closes the modal that was open
  }
  ...
}

For inline modals, close() unconditionally re-enters showPage("page-play"):

// src/client/components/BaseModal.ts:238-242
if (this.inline) {
  this.style.pointerEvents = "none";
  if (window.showPage) {
    window.showPage?.("page-play");
  }
}

Trace for "page A (inline modal) is open, user navigates to page B (a different inline modal)" — e.g. clicking a nav-bar item while Store/Settings/Leaderboard/etc. is open, or ModalRouter.openRegistered() (src/client/ModalRouter.ts:87) routing to a modal while another is open:

1. Outer showPage("page-B") → page-open toggled on.
2. showPage finds visible modal A, calls A.close().
3. BaseModal.close() for inline A → window.showPage("page-play") (nested call).
4. Nested call re-runs the toggle with pageId === "page-play" → page-open toggled off. It hides A and returns.
5. Outer call resumes: reveals B, calls B.open(). In BaseModal.open(), the inline branch's needsShow guard (BaseModal.ts:212-213, this.classList.contains("hidden") || this.style.display === "none") is false, because the outer call already removed hidden from B before calling open() — so open() does not call showPage again, and page-open is never restored.

End state: page B is visible, but body.page-open is absent, so body loses height: 100%; overflow: hidden; flex-direction: column and the whole document scrolls again instead of the modal — the bug this PR is meant to fix.

This isn't a rare edge case — it fires whenever a user switches between page modals without returning to the play page first (very common via the nav bar), and whenever ModalRouter routes to a new registered modal while one is already open.

Suggested fix: move the page-open toggle to run after the "close the currently visible modal" block (e.g. right before or after the reveal/open() logic near the end of showPage()), so the nested showPage("page-play") call can't clobber it. Note window.currentPageId (Navigation.ts:17) has the identical pre-existing reentrancy problem and would benefit from the same reordering, though that's outside this PR's stated scope.

[github-actions]

🤖 Claude Code Review

Verdict: No issues found — approve. Findings: 0 critical, 0 high, 0 medium, 0 low.

Reviewed src/client/Navigation.ts, src/client/styles.css, index.html, and src/client/components/MainLayout.ts for CLAUDE.md compliance and for bugs/security issues, including specifically tracing the re-entrancy concern raised by CodeRabbit (nested showPage("page-play") from a modal's close() potentially undoing the new page-open class toggle). That concern does not hold up: the toggle in Navigation.ts runs after the modal-closing block that triggers any nested showPage call, so the outer call's toggle always has the final say. No CLAUDE.md violations (no user-visible strings were added, so the i18n rule doesn't apply; no src/core files were touched).

No issues found. Checked for bugs and CLAUDE.md compliance.