Skip to content

fix(mobile): sidebar overlaps content on medium-sized screens#9870

Open
jmelahman wants to merge 2 commits intomainfrom
jamison/medium-sidebar
Open

fix(mobile): sidebar overlaps content on medium-sized screens#9870
jmelahman wants to merge 2 commits intomainfrom
jamison/medium-sidebar

Conversation

@jmelahman
Copy link
Copy Markdown
Contributor

@jmelahman jmelahman commented Apr 2, 2026

Description

Closes https://linear.app/onyx-app/issue/ENG-3890/sidebar-breakpoints

How Has This Been Tested?

Expecting no/minimal change to large screen cases.

20260402_14h09m58s_grim

Additional Options

  • [Optional] Override Linear Check

Summary by cubic

Fixes sidebar overlap on medium screens by switching to a foldable rail that stays in flow and opens as a fixed overlay with a blur backdrop on expand. Addresses Linear ENG-3890 by aligning breakpoints and centralizing fold/hide logic.

  • Bug Fixes

    • Medium screens: folded rail remains visible; expanded sidebar overlays content with blur-only backdrop and closes on backdrop click.
    • Mobile/medium use a fixed overlay; page scroll remains intact.
  • Refactors

    • web/src/layouts/sidebar-layouts.tsx: added medium-screen branch with a spacer; Body now auto-hides when folded.
    • Breakpoints unified: Tailwind sm 724px, md 912px, lg 1232px; mobile max 724; MOBILE_SIDEBAR_BREAKPOINT_PX set to 724; updated admin document-processing page to sm:*.
    • Sidebars: AppSidebar relies on layout-managed folding; AdminSidebar uses shared fold state, adds a folded “Search” action that expands and auto-focuses the input, and hides the footer when folded.

Written for commit 396f7a7. Summary will update on new commits.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Apr 2, 2026

Preview Deployment

Status Preview Commit Updated
https://onyx-preview-4uz0hi9op-danswer.vercel.app 396f7a7 2026-04-02 21:14:37 UTC

@jmelahman jmelahman force-pushed the jamison/medium-sidebar branch 2 times, most recently from 7a9361f to 9dcee62 Compare April 2, 2026 20:55
@jmelahman jmelahman force-pushed the jamison/medium-sidebar branch from 9dcee62 to 396f7a7 Compare April 2, 2026 21:11
@jmelahman jmelahman marked this pull request as ready for review April 2, 2026 21:13
@jmelahman jmelahman requested a review from a team as a code owner April 2, 2026 21:13
@jmelahman jmelahman changed the base branch from jamison/consolidate-sidebars to main April 2, 2026 21:14
@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps bot commented Apr 2, 2026

Greptile Summary

This PR fixes sidebar overlap on medium-sized screens (725–1232 px) by introducing a third layout mode: a folded icon-strip that stays in-flow via a spacer, while expanding to a fixed overlay with a blur-only backdrop. It also consolidates JS and Tailwind breakpoints (new sm/md/lg at 724/912/1232 px) and refactors AdminSidebar to read fold state from context.

Key changes:

  • sidebar-layouts.tsx: New isMediumScreen branch in SidebarRoot; SidebarBody auto-hides via CSS hidden when folded (preserving scroll state vs. unmounting).
  • tailwind.config.js: Adds sm: 724px, md: 912px, lg: 1232px in extend.screens, overriding Tailwind's defaults globally — the desktop breakpoint is cleanly removed with no remaining usages.
  • constants.ts: MOBILE_SIDEBAR_BREAKPOINT_PX updated from 640 → 724 to stay in sync.
  • AdminSidebar.tsx: Split into AdminSidebarInner (context-aware) + thin AdminSidebar wrapper; adds folded search-icon that expands the sidebar and auto-focuses the input.
  • AppSidebar.tsx: Removes now-redundant folded ? null : guard in SidebarBody.

Confidence Score: 4/5

  • Mostly safe — the sidebar logic is sound, but the global Tailwind breakpoint overrides should be verified against all existing sm:/md:/lg: usages before merging.
  • One P1 concern: overriding sm, md, lg in extend.screens shifts Tailwind's default responsive breakpoints for every component in the codebase that already uses those prefixes (~15+ files). The sidebar changes themselves are well-structured and the spacer width matches SidebarWrapper's folded width exactly. All desktop: usages were cleanly removed. The score stays at 4 pending a visual audit of the components affected by the breakpoint shift.
  • web/tailwind-themes/tailwind.config.js — the sm/md/lg overrides and their impact on all pre-existing responsive classes across Modal.tsx, dialog.tsx, Popover.tsx, input.tsx, settings-layouts.tsx, and ~10 others.

Important Files Changed

Filename Overview
web/tailwind-themes/tailwind.config.js Removes desktop breakpoint, adds sm/md/lg overrides — globally changes when responsive styles apply for all existing usages of those prefixes across ~15+ files.
web/src/layouts/sidebar-layouts.tsx Adds isMediumScreen branch that renders a fixed overlay with a spacer (w-[3.25rem] matching SidebarWrapper's folded width); SidebarBody now auto-hides via CSS hidden when folded, preserving scroll state.
web/src/sections/sidebar/AdminSidebar.tsx Refactored into AdminSidebarInner + AdminSidebar; reads fold state from context, adds folded search-icon that expands and auto-focuses the input via a one-shot focusSearch state + useEffect.
web/src/sections/sidebar/AppSidebar.tsx Removes redundant folded ? null : guard from SidebarBody since the layout layer now handles hiding automatically.
web/src/lib/constants.ts Updates MOBILE_SIDEBAR_BREAKPOINT_PX from 640 to 724, aligned with the new sm Tailwind breakpoint.
web/src/app/admin/configuration/document-processing/page.tsx Migrates desktop:flex-row / desktop:mt-0 to sm:flex-row / sm:mt-0 following the breakpoint rename.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[SidebarRoot renders] --> B{isMobile?\nwidth ≤ 724px}
    B -- Yes --> C[Fixed overlay\n-translate-x-full when folded\nBackdrop with bg-mask-03 + blur]
    B -- No --> D{isMediumScreen?\nwidth ≤ 1232px}
    D -- Yes --> E[Spacer div w-3.25rem in flow\n+ Fixed sidebar overlays when expanded\nBackdrop blur-only no tint]
    D -- No --> F[Large screen\nSidebarWrapper inline\nFoldable if foldable=true]
    E --> G{folded?}
    G -- No --> H[Backdrop opacity-100\npointer-events-auto\nclick → close]
    G -- Yes --> I[Backdrop opacity-0\npointer-events-none]
Loading
Prompt To Fix All With AI
This is a comment left during a code review.
Path: web/tailwind-themes/tailwind.config.js
Line: 67-71

Comment:
**Global breakpoint override affects existing `sm:*`, `md:*`, `lg:*` usages**

Adding `sm: "724px"`, `md: "912px"`, and `lg: "1232px"` inside `extend.screens` **overrides** Tailwind's default breakpoints (640 px, 768 px, 1024 px) globally. Any file in the codebase that already uses `sm:`, `md:`, or `lg:` responsive prefixes — `Modal.tsx`, `dialog.tsx`, `Popover.tsx`, `input.tsx`, `Badge.tsx`, `settings-layouts.tsx`, and ~15 others found — will silently shift to these new pixel values.

The affected screen ranges are:
- 640–724 px: `sm:` styles no longer apply (was 640 px, now 724 px)
- 768–912 px: `md:` styles no longer apply (was 768 px, now 912 px)
- 1024–1232 px: `lg:` styles no longer apply (was 1024 px, now 1232 px)

Those components were authored against Tailwind's standard defaults. If this consolidation is intentional, it's worth auditing all existing `sm:`, `md:`, and `lg:` usages to confirm none of them regress visually at the affected screen-width ranges.

How can I resolve this? If you propose a fix, please make it concise.

Reviews (1): Last reviewed commit: "fix(mobile): sidebar overlaps content on..." | Re-trigger Greptile

Comment on lines 67 to 71
screens: {
sm: "724px",
md: "912px",
lg: "1232px",
"2xl": "1420px",
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.

P1 Global breakpoint override affects existing sm:*, md:*, lg:* usages

Adding sm: "724px", md: "912px", and lg: "1232px" inside extend.screens overrides Tailwind's default breakpoints (640 px, 768 px, 1024 px) globally. Any file in the codebase that already uses sm:, md:, or lg: responsive prefixes — Modal.tsx, dialog.tsx, Popover.tsx, input.tsx, Badge.tsx, settings-layouts.tsx, and ~15 others found — will silently shift to these new pixel values.

The affected screen ranges are:

  • 640–724 px: sm: styles no longer apply (was 640 px, now 724 px)
  • 768–912 px: md: styles no longer apply (was 768 px, now 912 px)
  • 1024–1232 px: lg: styles no longer apply (was 1024 px, now 1232 px)

Those components were authored against Tailwind's standard defaults. If this consolidation is intentional, it's worth auditing all existing sm:, md:, and lg: usages to confirm none of them regress visually at the affected screen-width ranges.

Prompt To Fix With AI
This is a comment left during a code review.
Path: web/tailwind-themes/tailwind.config.js
Line: 67-71

Comment:
**Global breakpoint override affects existing `sm:*`, `md:*`, `lg:*` usages**

Adding `sm: "724px"`, `md: "912px"`, and `lg: "1232px"` inside `extend.screens` **overrides** Tailwind's default breakpoints (640 px, 768 px, 1024 px) globally. Any file in the codebase that already uses `sm:`, `md:`, or `lg:` responsive prefixes — `Modal.tsx`, `dialog.tsx`, `Popover.tsx`, `input.tsx`, `Badge.tsx`, `settings-layouts.tsx`, and ~15 others found — will silently shift to these new pixel values.

The affected screen ranges are:
- 640–724 px: `sm:` styles no longer apply (was 640 px, now 724 px)
- 768–912 px: `md:` styles no longer apply (was 768 px, now 912 px)
- 1024–1232 px: `lg:` styles no longer apply (was 1024 px, now 1232 px)

Those components were authored against Tailwind's standard defaults. If this consolidation is intentional, it's worth auditing all existing `sm:`, `md:`, and `lg:` usages to confirm none of them regress visually at the affected screen-width ranges.

How can I resolve this? If you propose a fix, please make it concise.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This was intentional

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.

1 participant