feat: app shell — sidebar, layout, workspace context (#25)#44
Conversation
- Add (app)/ layout with collapsible sidebar + main content area - Sidebar: workspace switcher (placeholder), page tree (placeholder), user menu with sign-out - Mobile (<768px): sidebar renders as Sheet, hamburger toggle in header - Keyboard shortcut ⌘+\ toggles sidebar visibility - Root / redirects authenticated users to their personal workspace - Install shadcn/ui sheet, dropdown-menu, separator components Co-authored-by: Ona <no-reply@ona.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
zacharias-ona
left a comment
There was a problem hiding this comment.
Review complete — 1 issue to fix before merge:
- Accessibility (blocking):
SheetTitleinapp-sidebar.tsx:44is outsideSheetContent, so it renders in the main DOM instead of inside the portal. Screen readers won't associate it with the dialog. Move it insideSheetContentas the first child. See inline comment for the fix.
Non-blocking nit: w-60 class on the desktop <aside> is dead code since inline style overrides it.
Everything else is clean — conventions followed, design spec alignment correct, schema queries match the migration, auth flow properly guarded, as cast on the Supabase join type is justified with a comment.
zacharias-ona
left a comment
There was a problem hiding this comment.
Review: 1 blocking issue to fix before merge
SheetTitleoutside portal (app-sidebar.tsx:44):SheetTitleis a sibling ofSheetContent, butSheetContentrenders through aPortal. The title ends up in the main DOM tree while the dialog is in the portal — screen readers won't associate them. MoveSheetTitleinsideSheetContentas the first child.
Everything else is clean — conventions followed, design spec alignment verified, auth flow properly guarded, as cast on the Supabase join type is justified. Ready to merge once the SheetTitle fix is pushed.
zacharias-ona
left a comment
There was a problem hiding this comment.
Review: 1 blocking issue to fix before merge
SheetTitleoutside portal (app-sidebar.tsx:44):SheetTitleis a sibling ofSheetContent, butSheetContentrenders through a portal. The title ends up in the main DOM while the dialog is in the portal — screen readers won't associate them. MoveSheetTitleinsideSheetContentas the first child. See inline comment for the fix.
Everything else is clean — conventions followed, design spec alignment verified, auth flow properly guarded, as cast on the Supabase join type is justified. Ready to merge once the SheetTitle fix is pushed.
SheetTitle was a sibling of SheetContent, but SheetContent renders through a portal. The title ended up in the main DOM while the dialog was in the portal — screen readers couldn't associate them. Co-authored-by: Ona <no-reply@ona.com>
1005974 to
6c5c7d9
Compare
|
Review — no blocking issues found. Reviewed against AGENTS.md, conventions.md, and design.md.
Merging. |
|
Review (posted as comment — same author, cannot self-approve) Reviewed against AGENTS.md, conventions.md, and design.md. No blocking issues.
Previous review issue resolved: Non-blocking nit: Merging. |
|
✅ Post-merge verification passed. Routes tested:
Routes skipped: none No console errors detected. |
|
✅ UI verification passed — design spec compliance confirmed. Files reviewed (11 UI files):
Static analysis — all checks pass:
Visual verification (Playwright screenshots):
|
Closes #25
What
Adds the authenticated app shell: a responsive layout with a collapsible sidebar, workspace context from the URL, and the foundational navigation structure. This is the chrome that all product features render inside.
How
Layout:
(app)/layout.tsxfetches the user profile server-side and passes display name + email to the client-sideAppShellcomponent, which wrapsSidebarProvider+ sidebar + main content area.Sidebar components (
src/components/sidebar/):sidebar-context.tsx— React context managing open/close state, mobile detection (<768px), and⌘+\keyboard shortcutapp-sidebar.tsx— Desktop: collapsible<aside>with 200ms transition. Mobile: shadcn/uiSheetsliding from leftworkspace-switcher.tsx— Placeholder button showing workspace name (functional in Pages CRUD and sidebar page tree #27)page-tree.tsx— Placeholder page list with "New Page" button (functional in Lexical editor — core blocks, slash commands, floating toolbar #28)user-menu.tsx— Dropdown menu with user info, settings link, and sign-outRoot redirect:
/now checks auth and redirects authenticated users to their first workspace via amembers→workspacesjoin query.New shadcn/ui components:
sheet,dropdown-menu,separator.Testing
pnpm lint✅pnpm typecheck✅pnpm test— 8/8 tests pass ✅SignOutButtonpattern.Acceptance Criteria Verification
(app)/with shared layout containing sidebar + main content area[workspaceSlug]dynamic segment resolves the current workspace from the URL⌘+\toggles sidebar visibility--radius: 0) (already in globals.css)/redirects authenticated users to their personal workspace