Repository navigation
Conversation
🦋 Changeset detectedLatest commit: c3815ee 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 |
|
@ericmason is attempting to deploy a commit to the HyperDX Team on Vercel. A member of the Team first needs to authorize it. |
|
Hi @ericmason, thanks for the pull request! Before we review code from a first-time contributor, a maintainer vouches for them, and you're not on our list yet. There's nothing else to file and this PR stays open — a maintainer will look at it, usually within a day or two, and vouch for you right here if it's a genuine attempt at the problem. To make that quick, check the PR description has:
More detail in our contributing guide. |
| onClickCapture={ | ||
| isOverlay | ||
| ? e => { | ||
| if (e.target instanceof Element && e.target.closest('a')) { |
There was a problem hiding this comment.
🟠 major — Tapping the Saved Searches or Dashboards chevron in the open compact nav closes the nav instead of toggling favorites
The favorites chevron <button> is rendered inside the <Link> in AppNavLink (AppNav.components.tsx:376-416). The capture handler runs before the chevron's own handler, finds closest('a'), and calls closeOverlay(). The chevron then calls preventDefault/stopPropagation, so the page doesn't navigate either: the nav just closes. Fix: skip clicks inside a button, e.g. const el = e.target.closest('a, button'); if (el?.tagName === 'A') closeOverlay();. Alternatively, close the nav on a route change (router.events routeChangeStart, or an effect on pathname) rather than on anchor clicks.
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
PR Review
3 finding(s): 🔴 0 critical · 🟠 1 major · 🔵 2 minor 2 posted as inline comment(s) on the changed lines. 1 listed below. Findings outside the changed lines1 minor
Severity is the reviewer's own estimate and is used for ordering, not filtering. No finding blocks merge automatically; a maintainer decides. |
[Medium impact] The PR appears safe to merge, with a small remaining issue when users select the current page.
|
Deep ReviewCompact-viewport (sub-900px) nav now opens as a modal overlay via the new 🔴 P0/P1 — must fix
🟡 P2 — recommended
🔵 P3 nitpicks (7)
Pre-existing (not counted toward verdict)
Reviewers (8): correctness, testing, maintainability, project-standards, adversarial, julik-frontend-races, kieran-typescript, previous-comments. Testing gaps:
Prior comments: The earlier Greptile P2 ("favorites chevrons close the nav" via 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 |
Below 900px the nav was forced collapsed, and the expand button only toggled the stored desktop preference, so it did nothing on a phone in portrait or landscape. On a compact viewport the nav now stays a rail, and the expand button opens the full nav over the page as a modal dialog with a backdrop. The backdrop, Escape, the button, or any route change closes it, and the favorites chevrons leave it open. Focus moves into the nav when it opens and returns to the button when it closes. The open state is not stored, so a phone session does not change the desktop preference. The log detail panel opens full width on a compact viewport, without the resize handle or the width toggle, and without the slide-in. The View Trace callout was placed while the panel was still sliding in, and the phone browser then widened the page to fit it and zoomed out. Wide viewports render as before. Fixes hyperdxio#1430 Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
2c6761b to
c3815ee
Compare
| if (openedOn != null && (!isCompact || openedOn !== route)) { | ||
| setOpenedOn(null); |
There was a problem hiding this comment.
Current-page links leave nav open
On a compact screen, tapping Search while already at /search leaves the nav over the page. useAppNavCollapse only closes it when the URL changes, and AppNavLink does not close it on selection. Users must close the nav separately to return to the page.
Close the overlay when a navigation link is followed, including links to the current URL, while leaving favorites chevrons alone.
| style={{ width: navWidth }} | ||
| style={ | ||
| isOverlay | ||
| ? { width: navWidth, zIndex: getDefaultZIndex('overlay') } |
There was a problem hiding this comment.
🟠 major — Menus and modals opened from the open compact nav appear behind it
The nav and backdrop are both at getDefaultZIndex('overlay') (400). Mantine Menu dropdowns sit at popover (300) and Modals at modal (200). So the user menu and Help menu (AppNav.components.tsx:101 and :245) open behind the nav, and so does the install-instructions modal (the onboarding 'Add data' button calls openInstallInstructions). The user preferences modal is opened from the user menu, so it can't be reached at all. Fix: give the nav and backdrop a z-index below popover/modal. Better: render the open nav with Mantine Drawer position="left", which also gives you the backdrop, Escape handling, focus trap and focus return that are hand-rolled here.
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
| {/* Holds the rail's space so the page does not move under the open nav. */} | ||
| <div | ||
| style={{ | ||
| width: COLLAPSED_NAV_WIDTH, |
There was a problem hiding this comment.
🔵 minor — The spacer is 1px narrower than the ghost it replaces, so the page shifts when the nav opens
With fixed, the navGhost it replaces is navWidth + 1 wide (51px, which allows for the nav's 1px right border). The new spacer is COLLAPSED_NAV_WIDTH (50px), so the page moves 1px left every time the nav opens and back when it closes. That contradicts the comment above the spacer. Fix: use COLLAPSED_NAV_WIDTH + 1 when fixed, or keep navGhost at the rail width while the overlay is open.
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
Summary
Below 900px the nav is forced collapsed, and the expand button only toggles the stored desktop preference, so on a phone it does nothing in portrait or landscape (#1430).
aria-controls, focus moves into the nav when it opens, and focus returns to the button when it closes. The open state isn't stored, so using HyperDX on a phone doesn't change the collapse preference on a desktop. The logic is in the newuseAppNavCollapsehook, with unit tests for both sides of the 900px cutoff.main).Tested on iPhone-size viewports (390x844 and 393x659) in headless Chromium with mobile emulation and in headless WebKit, and at 1440x900. Not tested on a physical phone.
Screenshots or video
All screenshots use the public demo server's data.
How to reproduce
Starting point:
yarn app:dev:local, then onboarding, then Connect to Demo Server. Set the browser to a phone-size viewport (any width under 900px).On
main:/searchand tap the expand button under the HyperDX logo. The nav stays collapsed.On this branch:
/searchand tap the expand button. The full nav opens over the page with a backdrop.main.How to test on Vercel preview
Preview routes: /search
Steps:
title="Collapse/Expand Navigation".data-testid="app-nav-overlay"is visible.data-testidstarts withtable-row-and verify the element withdata-testid="row-side-panel"starts at x=0 and is 390px wide.data-testid="app-nav-overlay"exists.References
initial-scale=1) and fix(app): keep saved search usable on a phone #3376 (phone search layout), which this builds on; Fix mobile app nav expansion button #1431, an earlier closed attempt at Mobile app nav expansion button does not work #1430.🤖 Generated with Claude Code