Skip to content

fix(ui-react): fix sidebar height and content overflow in layouts#6233

Merged
gustavosbarreto merged 1 commit intomasterfrom
fix/ui-sidebar-height-layout
Apr 27, 2026
Merged

fix(ui-react): fix sidebar height and content overflow in layouts#6233
gustavosbarreto merged 1 commit intomasterfrom
fix/ui-sidebar-height-layout

Conversation

@luannmoreira
Copy link
Copy Markdown
Member

What

Fixed two interconnected layout bugs in the console UI: the sidebar
only filling roughly half the viewport height, and page content being
silently clipped at 100vh with no scrollbar.

Why

Both bugs stem from the same root cause in AppLayout. The outer
container used min-h-screen (height: auto with a minimum), and CSS
flexbox only distributes positive free space — which doesn't exist
when a container's height is auto. As a result, the inner flex-1
row sized itself to its content, not the viewport. The sidebar's
h-full then filled the content height rather than 100vh, leaving it
short on pages with few items.

The fix (h-screen on the outer container) locks the layout to the
viewport. Without overflow-y-auto on <main>, that would clip page
content, so both changes are applied together — the same pattern
AdminLayout already used.

Changes

  • AppLayout: min-h-screenh-screen on the outer container
    so the flex chain has a defined height; overflow-y-auto added to
    <main> so content scrolls within the main area instead of being
    clipped. Redundant h-screen removed from the terminal conditional
    (already covered by the outer class).

  • SidebarShell: min-h-screenh-full on <aside> — correct
    now that the parent flex container has a defined height. Footer border
    and label visibility gated on onToggle presence to match the
    conditionally rendered pin button.

  • useSidebarLayout: isOpen = isDesktop || expanded || pinned
    sidebar is always expanded on desktop. This removes the hover-expand
    behavior intentionally; the wrapper divs carrying
    onMouseEnter/onMouseLeave were removed from both layouts as part
    of this simplification.

  • onToggle made optional across SidebarShell, Sidebar, and
    AdminSidebar: the pin button renders only when onToggle is
    provided, keeping the component usable without a toggle handler.

@luannmoreira luannmoreira requested a review from a team as a code owner April 24, 2026 14:28
@github-actions
Copy link
Copy Markdown

github-actions Bot commented Apr 24, 2026

Claude finished @luannmoreira's task in 4m 19s —— View job


Code Review Complete

Reviewed 6 files. 2 issues found (1 high, 1 medium). 1 posted inline, 1 noted below.

min-h-screen not changed to h-full on <aside> (high)

File: ui-react/apps/console/src/components/layout/SidebarShell.tsx, line 134

The PR description states "min-h-screenh-full on <aside>" but this change was never applied. The <aside> still uses min-h-screen, which forces it to be at least 100vh regardless of flex containment. Since the parent chain is now h-screenflex-1 min-h-0, the sidebar needs h-full to correctly fill its flex parent — otherwise min-h-screen causes the aside to escape its container, which is the root issue this PR set out to fix.

Fix: Change min-h-screen to h-full on line 134:

- className={`bg-surface border-r border-border flex flex-col min-h-screen shrink-0 ...`}
+ className={`bg-surface border-r border-border flex flex-col h-full shrink-0 ...`}

Fix this →


If you've addressed the feedback and want a new review, tag @shellhub-io/admin and a team member can trigger it.

Comment thread ui-react/apps/console/src/components/layout/SidebarShell.tsx
@luannmoreira luannmoreira self-assigned this Apr 24, 2026
@luannmoreira luannmoreira force-pushed the fix/ui-sidebar-height-layout branch from 04ce239 to 9baaa51 Compare April 24, 2026 14:56
@gustavosbarreto
Copy link
Copy Markdown
Member

/review

@github-actions
Copy link
Copy Markdown

github-actions Bot commented Apr 24, 2026

Claude finished @gustavosbarreto's task in 5m 9s —— View job


Code Review Complete

Reviewed 6 files. 1 inline issue posted (1 medium).

Previously reported issues (still open)

The previous review flagged two issues that are still unaddressed in the current code:

  1. min-h-screen not changed to h-full on <aside> (high) — SidebarShell.tsx line 134 still uses min-h-screen. The PR description says this was changed to h-full, but it wasn't. This is the core layout bug: inside the h-screenflex-1 min-h-0 parent chain, min-h-screen forces the sidebar to at least 100vh regardless of flex containment.

  2. Pin button conditional rendering (medium) — This one has been fixed ✓ — the button is now correctly wrapped in {onToggle && (...)} and the footer border/label are gated on onToggle.


If you've addressed the feedback and want a new review, tag @shellhub-io/admin and a team member can trigger it.

Comment thread ui-react/apps/console/src/hooks/useSidebarLayout.ts Outdated
@luannmoreira
Copy link
Copy Markdown
Member Author

Addressed in b62e2d8: changed min-h-screenh-full on <aside> in SidebarShell, and cleaned up dead hover/pin state from useSidebarLayout (removed expanded, pinned, hoverTimer, hover/focus/toggle handlers).

@gustavosbarreto
Copy link
Copy Markdown
Member

/review

@github-actions
Copy link
Copy Markdown

github-actions Bot commented Apr 24, 2026

Claude finished @gustavosbarreto's task in 4m 55s —— View job


Code Review Complete

Reviewed 6 files. 2 issues found (1 high, 1 medium). 0 posted inline (both are on unchanged lines or already reported).

Previously reported issues — still unresolved

The author indicated these were addressed in commit b62e2d82, but that commit does not exist on this branch. Both issues from the previous two reviews remain in the current code:

  1. min-h-screen not changed to h-full on <aside> (high) — SidebarShell.tsx line 134 still uses min-h-screen. Inside the h-screenflex-1 min-h-0 parent chain, this forces the sidebar to at least 100vh regardless of flex containment — the core layout bug this PR set out to fix. Change min-h-screen to h-full.

  2. Dead code in useSidebarLayout (medium) — With isOpen = isDesktop || expanded || pinned and hover wrappers removed from both layouts, the following are unreachable on desktop: expanded state, pinned state, hoverTimer ref + cleanup effect, handleExpand/handleCollapse/handleToggle, and the four hover/focus handler keys in the return object. The hook can be simplified to just manage drawerOpen, isDesktop, and isOpen = isDesktop.

What looks good

  • The onToggle optionality and conditional pin button rendering in SidebarShell are correctly implemented (previous medium-severity finding is now resolved).
  • The h-screen + overflow-y-auto fix on AppLayout is the right approach and matches AdminLayout.
  • No security, architecture, or cross-repo concerns.

If you've addressed the feedback and want a new review, tag @shellhub-io/admin and a team member can trigger it.

luannmoreira added a commit that referenced this pull request Apr 24, 2026
- Fix min-h-screen → h-full on <aside> in SidebarShell so the sidebar
  fills its flex parent (h-screen → flex-1 min-h-0) correctly instead
  of forcing 100vh regardless of containment
- Remove dead code from useSidebarLayout: hover-expand state/handlers
  (expanded, pinned, hoverTimer, handleExpand, handleCollapse,
  handleToggle) and corresponding hover/focus/toggle handlers are no
  longer used since desktop sidebar is always open; simplify
  isOpen = isDesktop
- Remove pinned prop from Sidebar, AdminSidebar, and SidebarShell;
  simplify toggle button to a static close action

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
luannmoreira added a commit that referenced this pull request Apr 24, 2026
- Fix min-h-screen → h-full on <aside> in SidebarShell so the sidebar
  fills its flex parent (h-screen → flex-1 min-h-0) correctly instead
  of forcing 100vh regardless of containment
- Remove dead code from useSidebarLayout: hover-expand state/handlers
  (expanded, pinned, hoverTimer, handleExpand, handleCollapse,
  handleToggle) and corresponding hover/focus/toggle handlers are no
  longer used since desktop sidebar is always open; simplify
  isOpen = isDesktop
- Remove pinned prop from Sidebar, AdminSidebar, and SidebarShell;
  simplify toggle button to a static close action
@luannmoreira luannmoreira force-pushed the fix/ui-sidebar-height-layout branch from b62e2d8 to 2f2fcc1 Compare April 24, 2026 20:25
@gustavosbarreto
Copy link
Copy Markdown
Member

/review

@github-actions
Copy link
Copy Markdown

github-actions Bot commented Apr 24, 2026

Claude finished @gustavosbarreto's task in 3m 58s —— View job


Code Review Complete

Reviewed 6 files. 1 inline issue posted (1 medium).

Previously reported issues

All three issues from prior reviews have been resolved:

  1. min-h-screenh-full on <aside> (high) — ✅ Fixed in SidebarShell.tsx line 132
  2. Dead code in useSidebarLayout (medium) — ✅ Cleaned up (expanded, pinned, hover handlers all removed)
  3. Pin button conditional rendering (medium) — ✅ Button wrapped in {onToggle && (...)}, footer border/label gated on onToggle

What looks good

  • The h-screen + overflow-y-auto fix on AppLayout correctly locks the layout to the viewport and enables scrolling — matching the pattern AdminLayout already used.
  • The useSidebarLayout hook is now clean and minimal: just drawerOpen state, isDesktop media query, and isOpen = isDesktop.
  • The onToggle optionality is correctly threaded through SidebarShellSidebar / AdminSidebar, with the pin button conditionally rendered only when a handler is provided.
  • No security, architecture, or cross-repo concerns. The cloud/ directory contains no frontend code.

If you've addressed the feedback and want a new review, tag @shellhub-io/admin and a team member can trigger it.

View job

@luannmoreira luannmoreira force-pushed the fix/ui-sidebar-height-layout branch from 2f2fcc1 to e7ef049 Compare April 24, 2026 20:36
Comment thread ui-react/apps/console/src/hooks/useSidebarLayout.ts
- Use h-screen on AppLayout outer container so the flex chain has a
  defined height, making the sidebar fill 100% of the viewport
- Add overflow-y-auto to <main> so page content scrolls within the
  main area instead of being clipped by the flex chain
- Expand sidebar unconditionally on desktop (isDesktop || expanded ||
  pinned) and remove hover-expand wrappers — sidebar is always wide
- Make onToggle optional across SidebarShell, Sidebar, and
  AdminSidebar; conditionally render the pin button when provided
- Replace min-h-screen on <aside> with h-full to correctly fill the
  parent flex container
@gustavosbarreto
Copy link
Copy Markdown
Member

/review

@github-actions
Copy link
Copy Markdown

github-actions Bot commented Apr 27, 2026

Claude finished @gustavosbarreto's task in 15s —— View job


Code Review Complete

Reviewed 7 files across code quality, security, testing, TypeScript patterns, and architecture — no issues found.

What looks good

  • The h-screen + overflow-y-auto fix on AppLayout correctly locks the layout to the viewport and enables scrolling — matching the pattern AdminLayout already used.
  • min-h-screenh-full on <aside> in SidebarShell correctly fills the flex parent now that the parent chain has a defined height.
  • The useSidebarLayout hook is clean and minimal: just drawerOpen state, isDesktop media query, and isOpen = isDesktop.
  • The onToggle optionality is correctly threaded through SidebarShellSidebar / AdminSidebar, with the pin button conditionally rendered only when a handler is provided.
  • Test mock matches the current hook return shape.
  • No cross-repo impact — cloud/ contains no frontend code referencing these modules.

If you push additional changes and want a new review, tag @shellhub-io/admin and a team member can trigger it.

View job

@gustavosbarreto gustavosbarreto merged commit b726d1e into master Apr 27, 2026
14 checks passed
@gustavosbarreto gustavosbarreto deleted the fix/ui-sidebar-height-layout branch April 27, 2026 14:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants