Move Sign in button from Header to Side nav when on small devices. #2231
Conversation
Summary by CodeRabbit
WalkthroughAdds a new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Flag potential breaking changes that are not documented:
1. Identify changes to public APIs/exports, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints (including removed/renamed items and changes to types, required params, return values, defaults, or behavior).
2. Ignore purely internal/private changes (e.g., code not exported from package entry points or marked internal).
3. Verify documentation exists: a "Breaking Change" section in the PR description and updates to CHANGELOG.md.Please share your feedback with us on this Discord post. 📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Pre-merge checks (4 passed, 1 warning)❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches
🧪 Generate unit tests
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
frontend/src/hooks/useIsMobile.ts (1)
3-4: Consider generalizing to a reusable useMediaQuery.Optional: export
useMediaQuery(query: string)and deriveuseIsMobilefrom it for other breakpoints later.frontend/src/components/Header.tsx (3)
23-23: Ensure breakpoint alignment with useIsMobile.With the hook fix using
desktopViewMinWidth - 1, this call is fine. If you prefer not to couple the hook toutils/constants, change the hook to accept a param and call it as below.-const isMobile = useIsMobile() +const isMobile = useIsMobile(desktopViewMinWidth)
133-133: Avoid initial mobile flicker by also hiding via CSS at small widths.Before the hook updates on mount, the header UserMenu can briefly render on mobile. Wrap with a responsive utility to keep it hidden on <md regardless of JS timing.
- {!isMobile && <UserMenu isGitHubAuthEnabled={isGitHubAuthEnabled} />} + {!isMobile && ( + <div className="hidden md:block"> + <UserMenu isGitHubAuthEnabled={isGitHubAuthEnabled} /> + </div> + )}
229-229: Verify styling parity for “Sign in” with side-nav buttons.
UserMenu’s unauthenticated button styling differs fromNavButton. If strict visual parity is required per #2226, consider a prop/variant onUserMenuthat renders aNavButton-styled “Sign in” (icon + label), or a smallSignInNavButtoncomponent used here.If helpful, I can draft a
SignInNavButtonthat delegates tosignIn('github', ...)while matchingNavButtonstyles.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
frontend/src/components/Header.tsx(4 hunks)frontend/src/hooks/useIsMobile.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
frontend/src/components/Header.tsx (2)
frontend/src/hooks/useIsMobile.ts (1)
useIsMobile(3-18)frontend/src/components/UserMenu.tsx (1)
UserMenu(13-119)
🔇 Additional comments (1)
frontend/src/components/Header.tsx (1)
9-9: Import looks good.No issues with the alias usage.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
frontend/src/hooks/useIsMobile.ts (1)
7-10: Use a shared breakpoint constant instead of hard-coding 767px (prevents drift).Tie this to the app’s desktop min width so Tailwind/config changes don’t silently break the switch.
-import { useEffect, useState } from 'react' +import { useEffect, useState } from 'react' +import { desktopViewMinWidth } from 'utils/constants' @@ - const mediaQuery = window.matchMedia('(max-width: 767px)') + const mediaQuery = window.matchMedia(`(max-width:${desktopViewMinWidth - 1}px)`)
🧹 Nitpick comments (3)
frontend/src/hooks/useIsMobile.ts (3)
9-16: Guard for environments without matchMedia (JSDOM/old browsers).Avoid runtime errors in tests or non-browser contexts.
- const mediaQuery = window.matchMedia('(max-width: 767px)') + if (typeof window.matchMedia !== 'function') { + // No-op: keep default false and exit effect + return + } + const mediaQuery = window.matchMedia('(max-width: 767px)') @@ - updateMatch(mediaQuery) + updateMatch(mediaQuery)
17-24: Prefer explicit typeof checks for listener APIs and handle “neither” case.Slightly safer narrowing and avoids truthy checks on non-functions.
- if (mediaQuery.addEventListener) { + if (typeof mediaQuery.addEventListener === 'function') { mediaQuery.addEventListener('change', updateMatch) - return () => mediaQuery.removeEventListener('change', updateMatch) - } else { + return () => mediaQuery.removeEventListener('change', updateMatch) + } else if (typeof mediaQuery.addListener === 'function') { // Safari browser < 14 fallback - mediaQuery.addListener(updateMatch) - return () => mediaQuery.removeListener(updateMatch) + mediaQuery.addListener(updateMatch) + return () => mediaQuery.removeListener(updateMatch) + } else { + // Nothing to clean up + return }
3-5: Optional: reduce layout shift with useSyncExternalStore.Not required, but a
useSyncExternalStore-based hook can minimize initial flicker on mobile by reading the current snapshot before paint.If you want, I can provide a
useMediaQuery(query)helper built onuseSyncExternalStoreand refactor this hook to use it.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/src/hooks/useIsMobile.ts(1 hunks)
🔇 Additional comments (1)
frontend/src/hooks/useIsMobile.ts (1)
11-16: Handler design is solid (works for both modern and legacy MQL).Good call unifying the event and list cases via
e.matchesand syncing initial state immediately.
|
kasya
left a comment
kasya
left a comment
There was a problem hiding this comment.
Thanks for working on this!
I can see some things we'd want to improve further, but that will be a separate issue 👌🏼
…2231) * moved the signin button to nav bar on small screens and added useIsMobile custom hook * moved the signin button to nav bar on small screens and added useIsMobile custom hook * fixed useIsMobile hook to support all browsers * made the useIsMobile hook compatible with old browsers * fixed error --------- Co-authored-by: Kate Golovanova <kate@kgthreads.com>
2 participants
Proposed change
Move the Sign in button from the Header to the side navigation on small devices.
It should have the same styling as the other buttons and appear at the top.
Resolves #2226
Added a new custom hook
useIsMobileto detect window sizeUpdated
Headerto conditionally render theUserMenucomponent based on theuseIsMobilehook.UserMenuappears in the header bar.UserMenuis moved into the side navigation.Working:
Screencast.from.08-09-25.11.35.57.AM.IST.webm
Checklist
make check-testlocally; all checks and tests passed.