Repository navigation
fix(app): open the full nav over the page on compact viewports - #3574
Conversation
Below 900px the sidebar is forced into the 50px icon rail, so the toggle button changed a saved preference that the compact layout ignores. It now opens the full nav as an overlay. The rail width stays reserved so the page does not shift, and the saved desktop collapse preference is left alone. Co-authored-by: Cursor <cursoragent@cursor.com>
🦋 Changeset detectedLatest commit: 33c980e The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
github-actions
Bot
added
the
review/tier-2
🟡 Tier 3 — StandardIntroduces new logic, modifies core functionality, or touches areas with non-trivial risk. Why this tier:
Review process: Full human review — logic, architecture, edge cases. Stats
|
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Duplicate code analysisNo new duplicate code introduced by this PR. jscpd compared this branch against |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| style={{ | ||
| width: navWidth + 1, | ||
| minWidth: navWidth + 1, | ||
| width: reservedWidth + 1, |
There was a problem hiding this comment.
🔵 minor — Opening the overlay moves page content 1px to the right, though the PR says the content stays put
The in-flow rail takes exactly 50px, because globals.css:12 sets * { box-sizing: border-box } and the 1px border is drawn inside that width. The ghost that replaces it is reservedWidth + 1 = 51px, so the content shifts by 1px every time the nav opens or closes. When the ghost is only there for the overlay (isMobileNavOpen && !fixed), size it to reservedWidth with no +1.
Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond
| }} | ||
| ></div> | ||
| )} | ||
| {isMobileNavOpen && ( |
There was a problem hiding this comment.
🔵 minor — The overlay acts like a modal drawer but does not manage focus
Opening the nav leaves focus on the toggle, and Tab moves past the nav into page controls hidden behind the scrim. Closing it with Escape or the scrim does not return focus anywhere. Trap focus inside the nav while it is open, for example with Mantine FocusTrap / useFocusTrap on the nav container, and return focus to the toggle on close. Mantine Drawer already provides the overlay, Escape handling and focus return.
Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond
| position: fixed; | ||
| } | ||
|
|
||
| // Top-level drawers use z-index 10 (see PageHeader.module.scss). |
There was a problem hiding this comment.
🔵 minor — The new comment points to PageHeader.module.scss for drawers at z-index 10, but that file sets z-index 2
PageHeader.module.scss:22 sets z-index 2 and only describes drawers as contextZIndex + 10 from ZIndexContext, e.g. DBRowSidePanel. Point the comment at ZIndexContext/DBRowSidePanel instead, or reference a shared constant rather than hardcoding 10 and 9 here.
Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond
PR Review
3 finding(s): 🔴 0 critical · 🟠 1 major · 🔵 2 minor 1 posted as inline comment(s) on the changed lines. 2 listed below. Findings outside the changed lines2 minor
Severity is the reviewer's own estimate and is used for ordering, not filtering. No finding blocks merge automatically; a maintainer decides. |
Deep Review✅ No critical issues found. No P0/P1: the change introduces no data loss, auth, injection, or guaranteed happy-path crash. The prior human concern (body shift on close, @brandon-pereira) was verified addressed in code. The items below are recommended hardening and nits. 🟡 P2 — recommended
🔵 P3 nitpicks (5)
Pre-existing (not introduced by this PR, excluded from verdict): Brand-mark note (latent, renders correctly today): the bolt uses Reviewers (10): correctness, testing, maintainability, project-standards, agent-native, learnings, kieran-typescript, julik-frontend-races, adversarial, previous-comments. Testing gaps:
No finding blocks merge automatically. A maintainer will expect P0/P1 findings in code this PR changes to be fixed; P2/P3 are your call -- fix or reply. Never fix findings about surrounding code here; reply instead. Do not widen the PR. How to respond |
E2E Test Results✅ All tests passed • 463 passed • 1 skipped • 1652s
Tests ran across 4 shards in parallel. |
|
@elizabetdev LGTM except one weird thing I see is when you close the flyout the whole body moves a bit like its animating the nav expansion state |
Keep the rail width reserved without the extra pixel when the overlay opens, so the page does not shift. Lock body scroll while the overlay is open. Ignore Escape from text fields and menus opened from the nav. Trap focus in the open overlay and return it to the toggle on close. Ignore a toggle while the viewport is wide so it cannot latch open. Name the rail and full-nav widths and correct the z-index comment. Co-authored-by: Cursor <cursoragent@cursor.com>
Replace the bar mark in the logomark and wordmark with the hexagon and bolt symbol from the ClickStack Figma icon. The bolt uses the page background so it reads on both color modes. Regenerate the favicon set from the same symbol. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Addressed in aa670cd (review fixes) and 33c980e (mark):
Not done: a component-level AppNav test for the toggle wiring. It needs a lot of mocks for the nav's data hooks. The PR description lists it as a gap. |
github-actions
Bot
added
review/tier-3
| if (!isOpen) return; | ||
| const previousOverflow = document.body.style.overflow; | ||
| // The scrim covers the page but does not catch wheel or touch scrolling. | ||
| document.body.style.overflow = 'hidden'; |
There was a problem hiding this comment.
🔵 minor — The scroll lock sets overflow: hidden on document.body, but the page doesn't scroll on the body
Pages inside the nav scroll in #app-content-scroll-container (packages/app/src/layout.tsx:86-97, APP_CONTENT_SCROLL_CONTAINER_ID), and its parent is already overflow: hidden. Setting document.body.style.overflow therefore has no effect. The comment on line 37 and the test 'locks body scroll while open' only check a style on an element that never scrolls. Either remove the lock and the test, or lock the real scroll container (or set overscroll-behavior: contain on the scrim) and test that element.
Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond
Closing the overlay drops the nav back into the flow. Its width transition started from the overlay width, so the content jumped to 250px and slid back to 50px. Skip the width transition while the compact rail is closed. Opening keeps its slide-in. Co-authored-by: Cursor <cursoragent@cursor.com>
|
@brandon-pereira thanks for catching this. The body moved on close because the nav drops back into the flow at the rail width, and its width transition started from the overlay width. The content jumped to 250px and slid back to 50px. Fixed in b0b11bb. The width transition is now skipped while the compact rail is closed, so closing snaps. Opening still animates. I checked this in headless Chromium against the compiled nav CSS: the content stays at 50px through open and close, and the nav still animates from 50px to 250px on open. I have not checked it in a running app. |
| &Overlay { | ||
| top: 0; | ||
| left: 0; | ||
| z-index: 10; |
There was a problem hiding this comment.
🟠 major — The nav overlay shares z-index 10 with the row side panel, so the open side panel covers most of the nav
DBRowSidePanel (packages/app/src/components/DBRowSidePanel.tsx:1405, 1461, 1476) renders a portaled Drawer at contextZIndex + 10 = 10, with withOverlay={false} and a width of 80% (DrawerUtils.tsx:16). The portal comes later in the DOM, so at the same z-index it paints over the in-tree nav. On a 390px viewport with a row open, the toggle at x<50 is still visible and clickable. Clicking it opens a 250px nav whose area from x≈78 to x=250 is hidden under the drawer. The z-9 scrim also sits under the drawer, so the drawer stays undimmed and interactive. Fix: put the overlay above top-level drawers, for example by portaling the nav and scrim with a z-index above drawerZIndex, or close open side panels when the nav opens.
Nothing blocks merge automatically, but a maintainer will expect this fixed if it is a real defect in code this PR changes. If it is about surrounding code, reply and say so instead of patching. Do not widen the PR. How to respond
) ## Summary The brand name was the outlined SVG inside `brand/Wordmark.tsx`. This PR rewrites that component as live text next to the hexagon, so the name is selectable and matches the ClickStack lockup. `useWordmark()` is the only caller path, so every header that uses it changes together: - The sidebar, and the header on the login and register pages (`LandingHeader`). - The name is Inter 18px, weight 500, at normal letter spacing. `useWordmark()` passes the name in, so a downstream build still changes it in one place. - The symbol is 23px tall and hugs the hexagon. The gap is 8px from the symbol. - The collapsed rail is unchanged. `Logomark` stays a square box everywhere except inside the wordmark, so Spotlights and the Brand stories keep their old width. The branch also replaces the favicon set with the new artwork (black rounded square, yellow hexagon, dark bolt): - `favicon.ico`, `favicon-16x16.png`, `favicon-32x32.png`, and `apple-touch-icon.png` are replaced. - `favicon.svg` is the same artwork, linked as `image/svg+xml` so browsers that prefer SVG use it. The PNG and ICO files stay as fallbacks. The favicon change is a separate commit from the wordmark. Each has its own changeset. Either can be split into its own PR if reviewers prefer. ## Screenshots or video Not captured. The Before/After table is blank because I did not run the app. Please add pairs, on `main` and this branch, for the expanded sidebar, the collapsed rail, the login or register header, and the browser tab icon. | Before | After | | :----- | :---- | | | | ## How to reproduce **Starting point:** `yarn app:dev:local` connected to the demo server (onboarding → "Connect to Demo Server"), or play.hyperdx.io. **On `main`:** 1. Open the sidebar at a desktop width. 2. The brand name in the sidebar is the outlined logo text, not selectable text. 3. Open `/login`. The header shows the same outlined logo. 4. The browser tab shows the old favicon. **On this branch:** 1. Open the sidebar at a desktop width. 2. The sidebar shows the hexagon symbol followed by "ClickStack" as text, in medium weight. 3. Click "Collapse/expand navigation". The rail shows only the hexagon symbol, at the same size as before. 4. Open `/login`. The header shows the same text lockup as the sidebar. 5. The browser tab shows the new favicon. ## How to test on Vercel preview **Preview routes:** /search **Steps:** 1. Set the viewport width to 1280 pixels. 2. Verify the sidebar header shows the text "ClickStack" next to the hexagon symbol. 3. Click the button labeled "Collapse/expand navigation". 4. Verify the collapsed sidebar shows only the hexagon symbol and no "ClickStack" text. ## Tests - `npx tsc --noEmit`, ESLint, and stylelint pass on the changed files. - `Wordmark.test.tsx` renders the lockup and checks that the name is "ClickStack" and that the mark is narrower than it is tall. A second case checks that `useLogomark` still returns a square box. - Not covered by tests: the favicon in a browser tab, and the login header in a running app. The lockup was checked by rendering the Figma artwork next to the new text in a headless browser. That is not the running app. ## References - Linear Issue: - Related PRs: [#3574](#3574) (compact nav overlay and the hexagon mark, already merged)
Summary
Below 900px the sidebar is forced into its 50px icon rail (
useCompactViewport() || isPreferCollapsed). The header toggle only flipped the savedisNavCollapsedpreference, which the compact layout ignores. On a phone the button did nothing, and the rail has no labels, so there was no way to reach the full nav.This PR makes the toggle open the full nav (250px) as an overlay over the page:
aria-expanded.The overlay sits at the top-level drawer z-index (10), with the scrim one layer below.
The collapsed rail and the wordmark also use the ClickStack hexagon-and-bolt symbol from Figma (node
93:6056), and the favicon set is regenerated from the same symbol. This is a separate commit, so it can move to its own PR if reviewers prefer.Commits
29cd716the nav overlayaa670cdaddress compact nav overlay review feedback: focus trap and focus return, scroll lock, Escape scoping, no-latch toggle, named width constants, a 1px ghost fix, a corrected z-index comment, and three hook tests33c980euse the ClickStack hexagon and bolt mark in the logo, collapsed rail, and faviconScreenshots or video
Not captured. The change was verified with unit tests,
tsc, ESLint, and stylelint only. Please fill in the table from a compact-width viewport (under 900px) with the sidebar collapsed and then open. For the mark, add one before/after pair onmainand this branch in both light and dark mode.How to reproduce
Starting point:
yarn app:dev:localconnected to the demo server (onboarding → "Connect to Demo Server"), or play.hyperdx.io at a narrow window width.On
main:On this branch:
How to test on Vercel preview
Preview routes: /search
Steps:
Tests
useMobileNavOverlay: 8 tests covering toggle, compact gating, Escape, Escape from a text field, route change, scroll lock, no reopen after the window shrinks, and a toggle made while wide.tsc, ESLint, and stylelint are clean.toggleMobileNavon compact viewports, and the scrim close). The review flagged this. It is not in this PR yet.Changesets
compact-nav-overlay: patch for@hyperdx/appclickstack-hexagon-mark: patch for@hyperdx/appReferences