Skip to content

[web] Bound mobile navigation - #2299

Merged
Asherlc merged 6 commits into
mainfrom
issue-2188-bounded-navigation
Jul 29, 2026
Merged

Asherlc merged 6 commits into
mainfrom
issue-2188-bounded-navigation

Conversation

@Asherlc

@Asherlc Asherlc commented Jul 29, 2026

Copy link
Copy Markdown
Owner

Summary

  • replace the in-flow small-screen navigation links with the existing accessible ModalDialog top sheet
  • bound and scroll the navigation independently so opening it never lengthens or displaces the page
  • close on destination selection, Close, Escape, outside interaction, and crossing the desktop breakpoint
  • preserve focus trapping/restoration and focus the first navigation destination on open
  • add focused unit, Storybook, and Cypress coverage plus a shared jsdom matchMedia stub

Root cause

The small-screen links were rendered inside the header's normal document flow. Expanding the menu therefore inserted every destination above the page content, turning the header and route content into one long mixed page.

Mobile parity

The native app already uses five fixed bottom tabs and has no expandable navigation equivalent, so no native mobile behavior changes are needed.

Validation

  • pnpm exec vitest run --project unit packages/web --reporter=dot — 166 files, 1,204 tests passed
  • pnpm exec vitest run --project unit --project mobile --reporter=dot — 942 files passed, 2 skipped; 14,597 tests passed, 21 skipped
  • pnpm --dir packages/web typecheck
  • pnpm --dir packages/server typecheck
  • pnpm tsc --noEmit
  • pnpm lint — passed with an isolated current-worktree ClickHouse service
  • Storybook runtime audit at 390 px, 768 px, and 1,024 px — bounded sheet, unchanged content position, focus/Escape restoration, and desktop breakpoint close verified

Local Cypress execution was attempted twice through the unchanged pnpm e2e:web:up workflow, but Docker infrastructure prevented the test environment from starting:

  1. The first build completed, then Docker failed creating issue-2188-e2e_default: all predefined address pools have been fully subnetted.
  2. After scoped cleanup freed an address pool, the exact retry failed in Dockerfile line 88's pnpm install --force --frozen-lockfile. The first fatal line was [ERR_PNPM_BROKEN_METADATA_JSON] The operation was aborted due to timeout after prolonged npm registry metadata requests.

The unchanged CI E2E job is therefore required as the authoritative Cypress validation before merge.

Fixes #2188

Summary by Sourcery

Bound the small-screen web navigation inside an accessible modal sheet so opening the menu no longer displaces page content and closes appropriately across viewports.

New Features:

  • Introduce a modal-based mobile navigation sheet with a titled header, explicit close action, and independent scroll area.
  • Add a Storybook scenario that presents the AppHeader with the mobile navigation menu opened by default.

Bug Fixes:

  • Ensure mobile navigation closes on destination selection, Close button, Escape key, outside interaction, and when crossing to the desktop breakpoint.
  • Preserve and restore keyboard focus correctly when opening and closing the mobile navigation sheet, with focus trapped inside while open.

Enhancements:

  • Refine mobile navigation link styling for clearer, full-width tappable targets.
  • Wire the AppHeader to respond to a desktop breakpoint media query to keep mobile and desktop navigation states in sync.
  • Mock window.matchMedia in web tests to support viewport-dependent behavior.

Documentation:

  • Add a TDD plan document describing the bounded mobile navigation change, its behavior, scope, and test strategy.

Tests:

  • Extend AppHeader unit tests to cover modal semantics, focus management, dismissal paths, and breakpoint-driven closing for mobile navigation.
  • Expand Cypress navigation E2E coverage to verify bounded mobile navigation behavior and that page content is not displaced when the menu opens.

Summary by cubic

Bound the small-screen navigation in a scroll-bounded top-sheet ModalDialog so it no longer pushes page content. It focuses the first link, traps/restores focus with preventScroll, and closes on selection, Escape, outside pointer, and when switching to desktop; fixes #2188.

  • Bug Fixes
    • Replaced in-flow mobile links with a top-sheet ModalDialog that has independent scroll, prevents scroll jumps on focus, full-width tappable links, and an explicit Close.
    • Auto-close at the lg breakpoint via matchMedia, then focus the first desktop link; desktop sidebar unchanged.
    • Standardized outside-pointer dismissal in ModalDialog to be opt-in via pointer events; mobile navigation opts in.
    • Added focused unit tests (including preventScroll and outside-pointer cases), a Storybook “MobileNavigationOpen” story, a Cypress spec asserting stable scrollY and unchanged main position, and a shared jsdom matchMedia stub; added a short TDD plan doc.

Written for commit f307e45. Summary will update on new commits.

Review in cubic

Copilot AI review requested due to automatic review settings July 29, 2026 18:18
@Asherlc Asherlc linked an issue Jul 29, 2026 that may be closed by this pull request
@cursor

cursor Bot commented Jul 29, 2026

Copy link
Copy Markdown

Bugbot is not enabled for your account, so this pull request was not reviewed.

Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs.

@codereviewbot-ai

Copy link
Copy Markdown

🤖 Review skipped: Repository rate limit exceeded. Free accounts are limited to 2 reviews per 4 hours per repository. Upgrade to a paid plan for unlimited reviews.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@sourcery-ai sourcery-ai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry @Asherlc, you have reached your weekly rate limit of 500000 diff characters.

Please try again later or upgrade to continue using Sourcery

@coderabbitai

coderabbitai Bot commented Jul 29, 2026

Copy link
Copy Markdown

Warning

Review limit reached

@Asherlc, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 24 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 0505684e-dc91-483e-92c9-1e65a597b24f

📥 Commits

Reviewing files that changed from the base of the PR and between c9c3868 and f307e45.

📒 Files selected for processing (8)
  • cypress/e2e/navigation.cy.ts
  • docs/superpowers/plans/2026-07-29-bounded-mobile-navigation.md
  • packages/web/src/components/AppHeader.stories.tsx
  • packages/web/src/components/AppHeader.test.tsx
  • packages/web/src/components/AppHeader.tsx
  • packages/web/src/components/ModalDialog.test.tsx
  • packages/web/src/components/ModalDialog.tsx
  • packages/web/test-setup.ts

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

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

@sourcery-ai

sourcery-ai Bot commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

Reviewer's Guide

Replaces in-flow small-screen navigation in AppHeader with a bounded, accessible modal top sheet, wires it to close on multiple interactions including breakpoint changes, and adds focused unit, Cypress, Storybook, and shared jsdom matchMedia coverage for the new behavior.

Sequence diagram for bounded mobile navigation open/close behavior

sequenceDiagram
  actor User
  participant AppHeader
  participant ModalDialog
  participant Window

  User->>AppHeader: click Toggle navigation menu (button)
  AppHeader->>AppHeader: setMenuOpen(true)
  AppHeader->>ModalDialog: render ModalDialog open=true initialFocusRef=firstMobileLinkRef
  ModalDialog-->>User: move focus to firstMobileLinkRef

  User->>ModalDialog: click link (Link onClick)
  ModalDialog->>AppHeader: Link onClick
  AppHeader->>AppHeader: setMenuOpen(false)
  AppHeader->>ModalDialog: render ModalDialog open=false

  User->>ModalDialog: press Escape
  ModalDialog->>AppHeader: onClose()
  AppHeader->>AppHeader: setMenuOpen(false)
  ModalDialog-->>User: restore focus to Toggle navigation menu

  AppHeader->>Window: matchMedia("(min-width: 64rem)")
  Window-->>AppHeader: MediaQueryList
  AppHeader->>Window: addEventListener(change, closeAtDesktopBreakpoint)

  User->>Window: resize viewport to desktop
  Window->>AppHeader: closeAtDesktopBreakpoint()
  AppHeader->>AppHeader: setMenuOpen(false)
  AppHeader->>ModalDialog: render ModalDialog open=false
Loading

File-Level Changes

Change Details Files
Refactor mobile navigation in AppHeader to use a ModalDialog top sheet with focus management and breakpoint-aware closing.
  • Replace inline mobile nav with a ModalDialog-based portal that renders a top-aligned, scroll-bounded sheet containing the mobile nav
  • Track menu open state and wire it into ModalDialog along with outside-click dismissal and explicit Close button handling
  • Introduce a ref to the first mobile navigation link and pass it to ModalDialog for initial focus on open
  • Adjust mobile link typography/layout classes to block-level, slightly larger touch targets
  • Add an effect that registers a matchMedia listener for the desktop breakpoint and closes the sheet when it becomes true
  • Ensure mobile nav links close the menu on navigation while preserving existing desktop sidebar behavior
packages/web/src/components/AppHeader.tsx
Expand AppHeader unit tests to cover modal semantics, focus behavior, closing paths, and breakpoint-driven dismissal with media-query stubbing.
  • Enhance the mocked Link to support onClick and ref, preventing default navigation while still triggering click handlers
  • Stub global matchMedia in AppHeader tests, including a special case that exposes a change listener and toggles matches
  • Add tests asserting the dialog renders outside the header, is styled as a fixed, scroll-bounded sheet, and exposes an explicit Close control
  • Add tests for initial focus on the first destination, Tab focus containment within the dialog, Escape-based closing, and trigger-focus restoration
  • Add tests verifying closing via Close button, outside pointer interaction on the overlay, and clicking a navigation destination
  • Add a test that simulates crossing the desktop breakpoint via matchMedia and asserts the dialog closes
packages/web/src/components/AppHeader.test.tsx
Introduce shared jsdom matchMedia stubbing for web tests.
  • Define window.matchMedia on the jsdom window in the web test setup, returning an object with add/remove listener methods and a configurable query
  • Use vi.fn to create reusable mock implementations so individual tests can refine behavior as needed
packages/web/test-setup.ts
Add Storybook coverage for opened mobile navigation state.
  • Import within from storybook/test to query the rendered canvas
  • Add a MobileNavigationOpen story that sets a mobile viewport and programmatically clicks the toggle button in a play function to show the sheet
packages/web/src/components/AppHeader.stories.tsx
Extend Cypress navigation E2E to validate bounded mobile navigation, focus behavior, and desktop breakpoint closing.
  • Add an E2E test that captures the main element’s top position before opening the menu, then asserts it is unchanged when the modal dialog is open
  • Assert the modal has appropriate dialog attributes and that the mobile nav inside it is visible
  • Verify keyboard focus moves to the dashboard link on open, supports Escape to close, and restores focus to the toggle button
  • Simulate viewport changes from phone to tablet to desktop, checking that the dialog disappears and the desktop sidebar appears at the desktop breakpoint
cypress/e2e/navigation.cy.ts
Add internal documentation describing the bounded mobile navigation plan and TDD strategy.
  • Document the goal, behavioral requirements, scope, and references for the bounded mobile navigation feature
  • Outline current evidence, test strategy, file structure, and stepwise TDD tasks for implementing and verifying the change
docs/superpowers/plans/2026-07-29-bounded-mobile-navigation.md

Assessment against linked issues

Issue Objective Addressed Explanation
#2188 Change the small-screen navigation so that expanding it no longer pushes or displaces the main page content, instead using a clearly bounded disclosure/menu.
#2188 Provide proper focus management and predictable close behaviors for the small-screen navigation (e.g., focus moved into the menu, trapped while open, restored on close; close via destination selection, explicit control, Escape, and outside interaction).
#2188 Add or update automated coverage and documentation to validate and describe the new bounded mobile navigation behavior across responsive breakpoints.

Tips and commands

Interacting with Sourcery

  • Trigger a new review: Comment @sourcery-ai review on the pull request.
  • Continue discussions: Reply directly to Sourcery's review comments.
  • Generate a GitHub issue from a review comment: Ask Sourcery to create an
    issue from a review comment by replying to it. You can also reply to a
    review comment with @sourcery-ai issue to create an issue from it.
  • Generate a pull request title: Write @sourcery-ai anywhere in the pull
    request title to generate a title at any time. You can also comment
    @sourcery-ai title on the pull request to (re-)generate the title at any time.
  • Generate a pull request summary: Write @sourcery-ai summary anywhere in
    the pull request body to generate a PR summary at any time exactly where you
    want it. You can also comment @sourcery-ai summary on the pull request to
    (re-)generate the summary at any time.
  • Generate reviewer's guide: Comment @sourcery-ai guide on the pull
    request to (re-)generate the reviewer's guide at any time.
  • Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
    pull request to resolve all Sourcery comments. Useful if you've already
    addressed all the comments and don't want to see them anymore.
  • Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
    request to dismiss all existing Sourcery reviews. Especially useful if you
    want to start fresh with a new review - don't forget to comment
    @sourcery-ai review to trigger a new review!

Customizing Your Experience

Access your dashboard to:

  • Enable or disable review features such as the Sourcery-generated pull request
    summary, the reviewer's guide, and others.
  • Change the review language.
  • Add, remove or edit custom review instructions.
  • Adjust other review settings.

Getting Help

@qodo-code-review

Copy link
Copy Markdown

PR Summary by Qodo

Fix mobile navigation to use bounded top-sheet modal (no page displacement)

🐞 Bug fix 🧪 Tests 📝 Documentation 🕐 40+ Minutes

Grey Divider

AI Description

• Move small-screen navigation into a portal-based top-sheet ModalDialog instead of in-flow header
 links.
• Ensure the sheet is scroll-bounded, focus-trapped, and closes on all expected dismissal paths.
• Add unit/Storybook/Cypress coverage and a shared jsdom matchMedia stub for responsive tests.
Diagram

graph TD
  U("User") -->|"Toggle nav"| H["AppHeader"] -->|"open/close"| M[["ModalDialog (Radix)"]] -->|"focus trap + scroll"| N["Mobile nav sheet"]
  H -->|"subscribe"| Q{{"matchMedia lg"}} -->|"desktop => close"| H
  T["Vitest/Story/Cypress"] -->|"assert behavior"| H

  subgraph Legend
    direction LR
    _actor("Actor") ~~~ _cmp["Component"] ~~~ _modal[["Modal"]] ~~~ _mq{{"Decision"}}
  end
Loading
High-Level Assessment

The following are alternative approaches to this PR:

1. CSS-only fixed drawer (no modal)
  • ➕ Avoids portal/dialog abstractions; simpler DOM structure
  • ➕ Potentially less JS/state if purely CSS-driven
  • ➖ Hard to meet ARIA modal + focus-trap requirements without recreating dialog behavior
  • ➖ More risk of regressions across dismissal paths (Escape/outside click/focus restore)
2. Use Radix Dialog directly in `AppHeader`
  • ➕ More direct control over Radix props/events
  • ➕ Avoids relying on wrapper defaults like onPointerDownOutside prevention
  • ➖ Duplicates logic the repo already standardized in ModalDialog (focus restore, initial focus)
  • ➖ Higher maintenance burden and more inconsistency across modals

Recommendation: The chosen approach (reuse existing ModalDialog to implement a top-sheet) is the best fit: it fixes the displacement bug by moving mobile nav to a portal, and it leverages a standardized, accessibility-focused primitive for focus trapping and restoration. The added breakpoint-driven close via matchMedia is a pragmatic way to prevent dual-nav overlap when transitioning to desktop.

Files changed (6) +321 / -27

Bug fix (1) +55 / -23
AppHeader.tsxRender mobile navigation in a bounded modal top sheet +55/-23

Render mobile navigation in a bounded modal top sheet

• Replaces in-flow mobile nav links with a 'ModalDialog' top sheet using a portal, bounded height, and internal scrolling. Adds initial focus targeting the first destination, closes on link selection/Close/outside interaction, and automatically closes when '(min-width: 64rem)' matches to avoid overlap with the desktop sidebar.

packages/web/src/components/AppHeader.tsx

Tests (4) +186 / -4
navigation.cy.tsAdd E2E regression for bounded mobile navigation +31/-0

Add E2E regression for bounded mobile navigation

• Introduces a small-screen regression test that asserts opening the navigation does not shift 'main' content position. Verifies focus moves into the dialog, Escape closes and restores focus to the trigger, and crossing the desktop breakpoint closes the sheet and shows the sidebar nav.

cypress/e2e/navigation.cy.ts

AppHeader.stories.tsxAdd Storybook scenario with mobile navigation opened +12/-0

Add Storybook scenario with mobile navigation opened

• Adds a 'MobileNavigationOpen' story that sets a mobile viewport and uses a play function to open the menu. Enables quick visual/a11y verification of the top-sheet behavior in Storybook.

packages/web/src/components/AppHeader.stories.tsx

AppHeader.test.tsxExpand unit coverage for modal, focus, dismissal, and breakpoint close +125/-4

Expand unit coverage for modal, focus, dismissal, and breakpoint close

• Enhances router 'Link' mocking to support 'ref' and 'onClick', enabling focus tests and click-to-close behavior. Adds tests validating portal rendering (not in the header flow), scroll-bounded dialog classes, initial focus, Tab containment, Escape dismissal with focus restoration, outside-overlay click close, link close, and 'matchMedia' breakpoint-driven close.

packages/web/src/components/AppHeader.test.tsx

test-setup.tsStub 'window.matchMedia' for jsdom-based tests +18/-0

Stub 'window.matchMedia' for jsdom-based tests

• Adds a shared Vitest jsdom 'matchMedia' stub (including legacy listener methods) so components using media queries can run reliably in unit tests. Reduces per-test boilerplate and supports responsive behavior validation.

packages/web/test-setup.ts

Documentation (1) +80 / -0
2026-07-29-bounded-mobile-navigation.mdDocument TDD plan for bounded mobile navigation +80/-0

Document TDD plan for bounded mobile navigation

• Adds a detailed plan describing the root cause, desired accessibility behavior, and a test strategy spanning unit, Storybook, and Cypress. Captures scope and non-goals (native app parity) for issue #2188.

docs/superpowers/plans/2026-07-29-bounded-mobile-navigation.md

@github-actions

github-actions Bot commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

Storybook previews for 845d6f02 are ready:

This comment updates automatically on each PR push.

@qodo-code-review

qodo-code-review Bot commented Jul 29, 2026

Copy link
Copy Markdown

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📜 Skill insights (0)

Context used
✅ Compliance rules (platform): 178 rules

Grey Divider


Action required

1. Focus lost on breakpoint ✓ Resolved 🐞 Bug ≡ Correctness
Description
When the mobile sheet closes because the desktop breakpoint is crossed, ModalDialog restores focus
to the menu toggle that was active on open; at desktop widths that toggle is inside a lg:hidden
header, so focus is restored to a hidden element and can effectively disappear for keyboard users.
Code

packages/web/src/components/AppHeader.tsx[R48-57]

+  useEffect(() => {
+    const desktopMediaQuery = window.matchMedia("(min-width: 64rem)");
+    const closeAtDesktopBreakpoint = () => {
+      if (desktopMediaQuery.matches) setMenuOpen(false);
+    };
+
+    desktopMediaQuery.addEventListener("change", closeAtDesktopBreakpoint);
+    return () => {
+      desktopMediaQuery.removeEventListener("change", closeAtDesktopBreakpoint);
+    };
Relevance

●●● Strong

Accessibility/focus-management issues in AppHeader components have been accepted previously;
hidden-focus restoration will likely be fixed.

PR-#1197

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The PR introduces breakpoint-driven closing, and the existing ModalDialog focus restoration will
attempt to focus the element that opened the dialog even if it is now hidden by responsive CSS
(lg:hidden).

packages/web/src/components/AppHeader.tsx[48-72]
packages/web/src/components/ModalDialog.tsx[25-77]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
When the mobile navigation sheet closes due to crossing the desktop breakpoint, focus restoration targets the mobile toggle button captured on open. Because that button becomes `display: none` at `lg`, focusing it after close leaves keyboard focus in a hidden/unusable state.

### Issue Context
- The PR adds a `matchMedia(...).addEventListener('change', ...)` handler that closes the sheet when the desktop breakpoint matches.
- `ModalDialog` restores focus to the previously focused element as long as it is still connected, without checking visibility.

### Fix Focus Areas
- packages/web/src/components/AppHeader.tsx[48-58]
- packages/web/src/components/ModalDialog.tsx[25-77]

### Suggested fix
Implement one of these (or both for robustness):
1) **Harden ModalDialog focus restoration:** before focusing `returnFocusRef.current`, ensure it’s actually visible/focusable (e.g., `el.getClientRects().length > 0` and not `aria-hidden`, etc.). If not visible, skip restoring focus.
2) **Provide a desktop-visible fallback on breakpoint close:** in `AppHeader`, keep a ref to a stable desktop focus target (e.g., the first sidebar nav link) and, when closing due to the desktop media query, schedule focus to that element after close (e.g., `requestAnimationFrame(() => firstDesktopLinkRef.current?.focus())`).

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools



Remediation recommended

2. Flaky main-top assertion ✓ Resolved 🐞 Bug ☼ Reliability
Description
The new Cypress regression asserts getBoundingClientRect().top equality with a strict to.equal,
which is prone to subpixel/scrollbar/font/layout timing differences and can cause flaky CI failures.
Code

cypress/e2e/navigation.cy.ts[R34-44]

+    cy.get("main").then(($main) => {
+      const initialMainTop = $main[0].getBoundingClientRect().top;
+
+      cy.get('button[aria-label="Toggle navigation menu"]').click();
+      cy.get('[role="dialog"][aria-modal="true"]')
+        .should("be.visible")
+        .and("have.attr", "aria-labelledby");
+      cy.get('nav[aria-label="Mobile"]').should("be.visible");
+      cy.get("main").should(($openedMain) => {
+        expect($openedMain[0].getBoundingClientRect().top).to.equal(initialMainTop);
+      });
Relevance

●●● Strong

Team has accepted Cypress anti-flake changes: add stabilization/quiet windows for layout assertions.

PR-#2039

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The new test uses an exact pixel equality assertion on getBoundingClientRect().top, which is
sensitive to minor layout differences over time.

cypress/e2e/navigation.cy.ts[30-45]
PR-#2039

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
The test compares `main.getBoundingClientRect().top` before/after opening the sheet using strict equality. Minor subpixel changes or late layout adjustments can make this flaky while still preserving the intended invariant (no meaningful displacement).

### Issue Context
This is a layout-stability regression, similar to other recent Cypress stability improvements.

### Fix Focus Areas
- cypress/e2e/navigation.cy.ts[34-44]

### Suggested fix
- Replace strict equality with a tolerance:
 - e.g., `expect(newTop).to.be.closeTo(initialMainTop, 1)` (or an agreed epsilon).
- Optionally add a short stabilization step after opening the dialog (e.g., wait for a quiet window / next animation frame) before measuring, consistent with other layout-stability hardening patterns in the repo.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


3. Outside close is click-only ✓ Resolved 🐞 Bug ☼ Reliability
Description
closeOnInteractOutside is enabled for the mobile navigation, but ModalDialog always
preventDefault()s Radix’s onPointerDownOutside, so outside dismissal only happens via the
overlay click handler and not via Radix’s normal outside-interaction semantics.
Code

packages/web/src/components/AppHeader.tsx[R107-114]

+      <ModalDialog
+        open={menuOpen}
+        onClose={() => setMenuOpen(false)}
+        closeOnInteractOutside
+        initialFocusRef={firstMobileLinkRef}
+        overlayClassName="bg-black/45 backdrop-blur-sm lg:hidden"
+        contentClassName="!left-0 !right-0 !top-0 !translate-x-0 !translate-y-0 w-full max-h-dvh overflow-y-auto overscroll-contain border-b border-border-strong bg-surface-solid p-3 shadow-2xl lg:hidden"
+      >
Relevance

●● Moderate

No close precedent on Radix onPointerDownOutside preventDefault semantics; could be intentional
overlay-only dismissal.

PR-#2045

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
AppHeader turns on closeOnInteractOutside, but ModalDialog’s content unconditionally prevents
pointer-down-outside dismissal and instead closes only via overlay onClick.

packages/web/src/components/AppHeader.tsx[107-114]
packages/web/src/components/ModalDialog.tsx[51-81]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`ModalDialog` currently disables Radix’s pointer-down-outside dismissal unconditionally, even when `closeOnInteractOutside` is true. This makes outside dismissal depend on an overlay click path only, which is narrower than the prop name/intent.

### Issue Context
AppHeader now relies on `closeOnInteractOutside` for the mobile navigation sheet.

### Fix Focus Areas
- packages/web/src/components/AppHeader.tsx[107-114]
- packages/web/src/components/ModalDialog.tsx[51-81]

### Suggested fix
Update `ModalDialog` so that when `closeOnInteractOutside` is true it does **not** prevent Radix’s outside interaction:
- Make `onPointerDownOutside` conditional:
 - if `closeOnInteractOutside` is `false`, keep `event.preventDefault()` to block outside dismissal.
 - if `true`, allow default behavior (or explicitly call `closeDialog()` there), and remove/avoid relying on the overlay `onClick` close path as the only mechanism.
This will make `closeOnInteractOutside` behave consistently across pointer interactions.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

To customize comments, go to the Qodo configuration screen, or learn more in the docs.

Qodo Logo

Comment thread packages/web/src/components/AppHeader.tsx
Comment thread packages/web/src/components/AppHeader.tsx
Comment thread cypress/e2e/navigation.cy.ts
@codereviewbot-ai

ghost commented Jul 29, 2026

Copy link
Copy Markdown

🤖 Review skipped: Repository rate limit exceeded. Free accounts are limited to 2 reviews per 4 hours per repository. Upgrade to a paid plan for unlimited reviews.

1 similar comment
@codereviewbot-ai

ghost commented Jul 29, 2026

Copy link
Copy Markdown

🤖 Review skipped: Repository rate limit exceeded. Free accounts are limited to 2 reviews per 4 hours per repository. Upgrade to a paid plan for unlimited reviews.

@Asherlc

ghost commented Jul 29, 2026

Copy link
Copy Markdown
Owner Author

CI E2E failure diagnosed and fixed in da66b30.

  • Exact failing step: Run pnpm e2e:web:run
  • First fatal assertion: navigation.cy.ts:43 timed out with expected 37 to be close to 65 +/- 1
  • Root cause: custom dialog initial focus used element.focus() without preventScroll. Focusing the portaled Overview link moved the window to scrollY=28, so the main element's viewport-relative top changed from 65 to 37 even though the fixed sheet remained outside document flow.
  • Direct fix: shared initial and return focus now use focus({ preventScroll: true }).
  • Regression coverage: the focused unit test requires the custom initial-focus call to use preventScroll; Cypress now explicitly requires scrollY === 0 before checking the main-position invariant.

Validation after the fix:

  • focused ModalDialog + AppHeader: 15/15 tests passed
  • full web unit project: 166 files, 1,204 tests passed
  • web typecheck passed
  • focused Biome and diff checks passed

The new CI run is the authoritative E2E confirmation.

@codereviewbot-ai

ghost commented Jul 29, 2026

Copy link
Copy Markdown

🤖 Review skipped: Repository rate limit exceeded. Free accounts are limited to 2 reviews per 4 hours per repository. Upgrade to a paid plan for unlimited reviews.

@Asherlc

ghost commented Jul 29, 2026

Copy link
Copy Markdown
Owner Author

Follow-up root-cause evidence for the residual 20 px CI scroll:\n\n- On 70d16f9, the E2E measured a 28 px main-content shift.\n- On da66b30, ModalDialog used focus({ preventScroll: true }); CI then measured window.scrollY === 20. This isolates 8 px to the modal focus transition and independently justifies retaining the shared focus contract.\n- The remaining operation before the assertion is Cypress .click(). Cypress documents that .click() scrolls the element before firing and exposes scrollBehavior to control that action: https://docs.cypress.io/api/commands/click\n- 16bd0f0 disables only Cypress driver scrolling for this already-visible toggle and compares the browser scroll offset before and after the app opens the dialog. This preserves the exact product invariant; it adds no tolerance, wait, or scroll restoration.\n\nA scoped local Compose build no longer hit subnet exhaustion, but dependency materialization remained non-diagnostic after ~18 minutes (799/802 packages downloaded, 102/802 installed), so it was stopped and the issue-scoped E2E project was cleanly torn down. The new CI run is the authoritative browser validation.

@Asherlc
Asherlc force-pushed the issue-2188-bounded-navigation branch from 16bd0f0 to f307e45 Compare July 29, 2026 19:52
@codereviewbot-ai

ghost commented Jul 29, 2026

Copy link
Copy Markdown

🤖 Review skipped: Repository rate limit exceeded. Free accounts are limited to 2 reviews per 4 hours per repository. Upgrade to a paid plan for unlimited reviews.

@Asherlc

ghost commented Jul 29, 2026

Copy link
Copy Markdown
Owner Author

Rebased onto current origin/main (c9c3868e7) after the unit job exposed #2300 drift. The exact failure was AppHeader.test.tsx:238 expected ... length 2 but got 1.\n\nThe /more destination was present in both the desktop sidebar and bounded mobile dialog; Radix correctly makes the background sidebar inaccessible while the modal is open, so the stale current-main assertion could see only the dialog link. The rebased test now verifies the desktop More link within Sections before opening and the mobile More link within Mobile after opening. This preserves #2300 parity and the modal accessibility contract.\n\nValidation on rebased head f307e4513:\n- focused AppHeader + ModalDialog: 15/15\n- full web unit: 166 files, 1,208 tests\n- web typecheck: passed\n- Biome (changed web/Cypress files): passed\n- git diff --check: passed\n\nPushed with --force-with-lease; the refreshed CI run is authoritative for E2E and the complete matrix.

@Asherlc
Asherlc merged commit adb18b3 into main Jul 29, 2026
@Asherlc
Asherlc deleted the issue-2188-bounded-navigation branch July 29, 2026 20:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[WEB-04] Expanded nav pushes content into a long mixed page

2 participants