Merged
Conversation
Replace all inline 'hasPageFrame && layout === mobile' compound checks with a named isMobilePageFrame constant in each function scope. Also fix the hamburger button aria-label in MobilePageFrameNavigation and the hasPageFrame early-return path of MobileNavigation to toggle between 'Open main menu' and 'Close main menu' based on open state. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ogic When PrimaryNavigationItems was extracted from Navigation, its local ref was never attached to any DOM element. useActivateNavigationGroupOnHover received a ref with current === null, causing useMouseMovement to skip event listener setup and breaking the smart hover delay for the desktop sidebar. Fix by passing the ref from Navigation (which is attached to PrimaryNavigation.List) down to PrimaryNavigationItems as listRef. Mobile usages fall back to a local null ref, preserving the previous no-op behavior since hover delay is desktop-only. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Remove unreachable if (hasPageFrame) block from MobileNavigation — index.tsx
only renders MobileNavigation when hasPageFrame is false, so this branch
could never execute. The dead block also rendered a non-functional menu
button whose overlay would never appear.
Fix MobilePageFrameNavigation useEffect calling setView('expanded') on
initial mount. Since isOpen initializes as false, the else branch fired
immediately and overwrote any persisted collapsed preference. Now checks
scrollLock.held() before calling setView — if the lock is not held the
panel was never open, so no reset is needed. Expose held() on useScrollLock
to support this check.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…xtProvider The mobile context provider passed useState's setView directly as the context value, but the context interface expects (view: SecondaryNavState) => void. Wrap in useCallback to match the expected signature. Co-Authored-By: Claude Sonnet 4 <noreply@example.com>
Member
natemoo-re
approved these changes
Mar 24, 2026
Member
natemoo-re
left a comment
There was a problem hiding this comment.
I guess all the commits will get squashed anyway. LGTM!
Asynchronite
pushed a commit
to Asynchronite/sentry
that referenced
this pull request
Mar 24, 2026
Mobile navigation changes should not affect user's desktop preferences. This PR mitigates this by wrapping the secondary navigation using in-memory expanded/collapsed state as opposed to reusing the desktops localStorage persisted state --------- Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com> Co-authored-by: Claude Sonnet 4 <noreply@example.com>
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
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.
Mobile navigation changes should not affect user's desktop preferences. This PR mitigates this by wrapping the secondary navigation using in-memory expanded/collapsed state as opposed to reusing the desktops localStorage persisted state