Skip to content

Commit c00a963

Browse files
authored
Let a person collapse the sidebar, and reach the roster on a phone (#308)
* Let a person collapse the sidebar, and reach the roster on a phone The sidebar could always collapse. The primitive has held the state, the width transition and a Cmd/Ctrl+B listener since it was vendored in, and nothing ever rendered a trigger for any of it. The only affordance was SidebarRail, a 16px transparent strip carrying tabIndex={-1}, so the eye could not find it and the keyboard could not reach it. Under 768px the same sidebar is a Sheet that starts closed, and with no trigger nothing could open it. The roster is this app's navigation, so on a phone every channel sat behind a control that did not exist. The toggle is drawn in the chrome each screen already has, and in both states: one that lived inside the sidebar could not bring back what it hid. PageShell's back-button bar becomes unconditional so the control sits at the pane's left edge rather than 400px into a centred prose column, and the three screens that draw no header of their own get the same 48px band. The preference now survives a reload. The primitive wrote a sidebar_state cookie for a server-rendered shell to read on its first byte; nothing renders this app on a server and nothing ever read it back, so that write is gone and lib/sidebar.ts holds the answer instead, in the shape of the theme preference beside it. Mobile is deliberately excluded: an overlay that covers the screen it overlays has no business being open before anybody asked. * Draw no toggle where there is no sidebar, and cover the playground Two gaps found by auditing every screen under a shell rather than trusting the list of files the feature commit happened to touch. The admin playground drew no toggle at all, so collapsing the sidebar there left no way back. It is the one admin page that keeps its own geometry — an editor beside a live preview, as its comment says — so the toggle goes inline in its header rather than in a band of its own, which would take 48px from the thing being previewed. `PageShell` is not guaranteed to be inside a shell. Every one of the twenty screens that draws it is inside one today, so a toggle reading `useSidebar` unconditionally works here; a screen that renders `PageShell` outside a provider is a reasonable layout choice, and it would have met a thrown error during render rather than a missing button. `useSidebar` throwing is correct for a part OF a sidebar, where its absence is a wiring bug. A control that merely offers to toggle one reads the context optionally and draws nothing when there is none. * Draw the bar only when it holds something, and name the action a phone will take Two things review found. The bar was unconditional, which on `/assist` and `/link/slack` meant a 56px band holding nothing: those two draw PageShell directly under `_authed`, so there is no sidebar, the toggle correctly draws nothing, and neither passes a Back link. An empty band of chrome reads as a layout bug rather than as chrome. It is now drawn when it has at least one of the two to hold, and a screen without it keeps the full original space above its heading rather than the reduced space the bar was paying for. The label was wrong on a phone. `open` describes the desktop pane, so a mobile toggle always read "Show sidebar" even with the Sheet open. The Sheet is modal, so nothing could be clicked while it was wrong, but the context already carries `openMobile` and there is no reason to name the wrong action.
1 parent 5dab165 commit c00a963

15 files changed

Lines changed: 411 additions & 142 deletions

File tree

app/src/components/layout/page-shell.tsx

Lines changed: 33 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,8 @@ import { Link, type LinkProps } from "@tanstack/react-router";
33
import type * as React from "react";
44
import { cn } from "@/lib/utils";
55
import { Button } from "../ui/button";
6+
import { useOptionalSidebar } from "../ui/sidebar";
7+
import { SidebarToggle } from "./sidebar-toggle";
68

79
/**
810
* The frame every configuration screen sits in.
@@ -56,23 +58,42 @@ export function PageShell({
5658
label: string;
5759
};
5860
}) {
61+
/*
62+
* Whether this screen has a sidebar at all. `/assist` and `/link/slack` draw PageShell directly
63+
* under `_authed`, which mounts no provider, so there is nothing for a toggle to act on there.
64+
*/
65+
const hasSidebar = useOptionalSidebar() !== null;
66+
/*
67+
* The bar carries the toggle and the Back link, and is drawn when it has at least one of them.
68+
* The screens with a Back link already drew exactly this bar, so for them nothing changes; what
69+
* changed is that a sidebar is now reason enough on its own, because the toggle has to sit at the
70+
* pane's left edge in both states and the prose column is centred — a control inside it would be
71+
* 400px from the edge it belongs to on a wide screen. Drawing it with neither would be a 56px
72+
* band holding nothing, which reads as a layout bug rather than as chrome.
73+
*/
74+
const bar = hasSidebar || !!backButton;
75+
5976
return (
6077
<>
61-
{!!backButton && (
62-
<div className="max-w-7xl w-full h-14 flex items-center px-3">
63-
<Button
64-
variant="ghost"
65-
render={(props) => <Link {...backButton.linkProps} {...props} />}
66-
>
67-
<IconChevronLeft />
68-
{backButton.label}
69-
</Button>
78+
{bar ? (
79+
<div className="max-w-7xl w-full h-14 flex items-center gap-1 px-3">
80+
{hasSidebar ? <SidebarToggle /> : null}
81+
{!!backButton && (
82+
<Button
83+
variant="ghost"
84+
render={(props) => <Link {...backButton.linkProps} {...props} />}
85+
>
86+
<IconChevronLeft />
87+
{backButton.label}
88+
</Button>
89+
)}
7090
</div>
71-
)}
91+
) : null}
7292
<div
7393
className={cn(
74-
"mx-auto flex w-full flex-col px-4 py-12",
75-
{ "pt-8": !!backButton },
94+
"mx-auto flex w-full flex-col px-4 pb-12",
95+
// Without a bar above it the heading keeps the full original space.
96+
bar ? "pt-8" : "pt-12",
7697
WIDTHS[width],
7798
className,
7899
)}
Lines changed: 62 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,62 @@
1+
import { type CSSProperties, type ReactNode, useEffect, useState } from "react";
2+
3+
import { SidebarProvider } from "@/components/ui/sidebar";
4+
import {
5+
applySidebarOpen,
6+
parseStoredSidebarOpen,
7+
SIDEBAR_STORAGE_KEY,
8+
} from "@/lib/sidebar";
9+
10+
/**
11+
* The frame the three shells — app, admin, settings — hang their sidebar in.
12+
*
13+
* It exists for one reason: the open/closed state has to outlive a reload, and the primitive's
14+
* `SidebarProvider` cannot do that on its own (it writes a cookie nothing reads and hardcodes
15+
* `defaultOpen` to true). Driving it as controlled state is a dozen lines, and three copies of a
16+
* dozen lines is three chances for one shell to forget the preference the other two remember.
17+
*
18+
* `width` is what actually differs between the shells. The app's roster earns 340px because its
19+
* rows are two-line message previews; admin and settings hold short nav labels and earn 300px.
20+
*/
21+
export function SidebarShell({
22+
children,
23+
className,
24+
width,
25+
}: {
26+
children: ReactNode;
27+
className?: string;
28+
width: string;
29+
}) {
30+
// Read before the first paint, so a shell somebody left collapsed never flashes open.
31+
const [open, setOpen] = useState(() =>
32+
parseStoredSidebarOpen(window.localStorage.getItem(SIDEBAR_STORAGE_KEY)),
33+
);
34+
35+
useEffect(() => {
36+
applySidebarOpen(open, {
37+
setStoredValue: (key, value) => window.localStorage.setItem(key, value),
38+
});
39+
}, [open]);
40+
41+
return (
42+
<SidebarProvider
43+
className={className}
44+
onOpenChange={setOpen}
45+
open={open}
46+
/*
47+
* `--sidebar-width-mobile` is set for the shape's sake and currently goes unread: the
48+
* primitive's mobile branch styles its Sheet from a hardcoded 18rem. Below 768px the sidebar
49+
* is that Sheet, which is also why the stored preference never reaches it — a panel that
50+
* covers the screen it overlays has no business being open before anybody asked for it.
51+
*/
52+
style={
53+
{
54+
"--sidebar-width": width,
55+
"--sidebar-width-mobile": "20rem",
56+
} as CSSProperties
57+
}
58+
>
59+
{children}
60+
</SidebarProvider>
61+
);
62+
}
Lines changed: 89 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,89 @@
1+
import { IconLayoutSidebar } from "@tabler/icons-react";
2+
3+
import { Button } from "@/components/ui/button";
4+
import { useOptionalSidebar } from "@/components/ui/sidebar";
5+
import {
6+
Tooltip,
7+
TooltipContent,
8+
TooltipTrigger,
9+
} from "@/components/ui/tooltip";
10+
import { cn } from "@/lib/utils";
11+
12+
/**
13+
* ⌘ on Apple platforms, Ctrl everywhere else. The primitive's listener accepts either modifier, so
14+
* this only decides which of the two to name.
15+
*/
16+
const SHORTCUT_LABEL = /Mac|iPhone|iPad|iPod/.test(navigator.userAgent)
17+
? "⌘B"
18+
: "Ctrl+B";
19+
20+
/**
21+
* The control that opens and closes the shell's sidebar.
22+
*
23+
* WHAT THIS IS FIXING. The sidebar could always collapse — the primitive has had the state, the
24+
* width transition and a ⌘B shortcut since it was vendored in. Nothing ever rendered a trigger for
25+
* it. The only affordance was `SidebarRail`, a 16px transparent strip carrying `tabIndex={-1}`, so
26+
* the eye could not find it and the keyboard could not reach it; and under 768px, where the sidebar
27+
* becomes a Sheet that starts closed, there was no way to open the roster at all.
28+
*
29+
* It is therefore drawn in the chrome each screen already has, and in BOTH states, rather than
30+
* inside the sidebar it hides — a trigger that disappears along with the sidebar cannot bring it
31+
* back.
32+
*
33+
* Built from `Button` rather than the primitive's `SidebarTrigger` because that component hardcodes
34+
* its own children after the prop spread, including an `sr-only` "Toggle Sidebar" that would
35+
* contradict the label below. The state still belongs to the primitive: `toggleSidebar` is its hook.
36+
*/
37+
export function SidebarToggle({ className }: { className?: string }) {
38+
const sidebar = useOptionalSidebar();
39+
// No sidebar in scope, so nothing to toggle and nothing to draw.
40+
if (!sidebar) return null;
41+
const { isMobile, open, openMobile, toggleSidebar } = sidebar;
42+
/*
43+
* The label says what the click will do, not what is on screen. Below 768px the sidebar is an
44+
* overlay Sheet with its own open state, and `open` describes the desktop pane — reading it there
45+
* would name the wrong action.
46+
*/
47+
const label = (isMobile ? openMobile : open)
48+
? "Hide sidebar"
49+
: "Show sidebar";
50+
51+
return (
52+
<Tooltip>
53+
<TooltipTrigger
54+
render={
55+
<Button
56+
aria-label={label}
57+
className={cn("text-muted-foreground", className)}
58+
onClick={toggleSidebar}
59+
size="icon"
60+
variant="ghost"
61+
>
62+
<IconLayoutSidebar className="size-4.5" />
63+
</Button>
64+
}
65+
/>
66+
{/* An accelerator nobody is told about is not a feature. */}
67+
<TooltipContent side="bottom">
68+
{label}
69+
<span className="text-background/60">{SHORTCUT_LABEL}</span>
70+
</TooltipContent>
71+
</Tooltip>
72+
);
73+
}
74+
75+
/**
76+
* The toggle on a screen that draws no header of its own.
77+
*
78+
* Three `_app` screens open straight into their content, and the toggle still has to land in the
79+
* same 48px band it occupies everywhere else — a control that moves between screens is a control
80+
* somebody has to look for each time. No bottom border: a divider under an otherwise empty bar is a
81+
* line with nothing to divide.
82+
*/
83+
export function SidebarToggleBar() {
84+
return (
85+
<div className="h-12 shrink-0 flex items-center px-3">
86+
<SidebarToggle />
87+
</div>
88+
);
89+
}

app/src/components/ui/sidebar.tsx

Lines changed: 22 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -25,8 +25,6 @@ import {
2525
} from "@/components/ui/tooltip";
2626
import { IconLayoutSidebar } from "@tabler/icons-react";
2727

28-
const SIDEBAR_COOKIE_NAME = "sidebar_state";
29-
const SIDEBAR_COOKIE_MAX_AGE = 60 * 60 * 24 * 7;
3028
const SIDEBAR_WIDTH = "16rem";
3129
const SIDEBAR_WIDTH_MOBILE = "18rem";
3230
const SIDEBAR_WIDTH_ICON = "3rem";
@@ -53,6 +51,27 @@ function useSidebar() {
5351
return context;
5452
}
5553

54+
/**
55+
* The sidebar's state, or null where there is no sidebar.
56+
*
57+
* `useSidebar` throwing is right for a part OF a sidebar, where its absence is a wiring bug. A
58+
* control that merely offers to toggle one is the other case: `PageShell` sits inside the three
59+
* shells today, and a screen that renders it outside one is a layout choice rather than a defect.
60+
* Throwing there would take a whole screen down over a button that should simply not be drawn.
61+
*/
62+
function useOptionalSidebar() {
63+
return React.useContext(SidebarContext);
64+
}
65+
66+
/**
67+
* Sidebar state, uncontrolled by default.
68+
*
69+
* This vendored file used to write a `sidebar_state` cookie on every toggle, so a server-rendered
70+
* shell could paint the right width on its first byte. Nothing renders this app on a server and
71+
* nothing ever read the cookie back, so the preference lives in `lib/sidebar.ts` and the shells
72+
* drive `open`/`onOpenChange` through `layout/sidebar-shell.tsx` instead. Re-adding the cookie would
73+
* stand a second, staler answer beside that one.
74+
*/
5675
function SidebarProvider({
5776
defaultOpen = true,
5877
open: openProp,
@@ -81,9 +100,6 @@ function SidebarProvider({
81100
} else {
82101
_setOpen(openState);
83102
}
84-
85-
// This sets the cookie to keep the sidebar state.
86-
document.cookie = `${SIDEBAR_COOKIE_NAME}=${openState}; path=/; max-age=${SIDEBAR_COOKIE_MAX_AGE}`;
87103
},
88104
[setOpenProp, open],
89105
);
@@ -719,5 +735,6 @@ export {
719735
SidebarRail,
720736
SidebarSeparator,
721737
SidebarTrigger,
738+
useOptionalSidebar,
722739
useSidebar,
723740
};

app/src/lib/sidebar.ts

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,33 @@
1+
/**
2+
* Whether the shell's sidebar is open, remembered across reloads.
3+
*
4+
* The primitive under `components/ui/sidebar.tsx` writes a `sidebar_state` cookie of its own, which
5+
* exists so a server-rendered shell can paint the right width on the first byte. Nothing renders
6+
* this app on a server, and nothing ever read that cookie back — so the preference lives here
7+
* instead, in the same shape and the same storage as the theme preference next door.
8+
*/
9+
export const SIDEBAR_STORAGE_KEY = "openbot-sidebar";
10+
11+
/**
12+
* Only the exact stored `collapsed` starts the sidebar closed.
13+
*
14+
* Everything else opens it: a key never written, a value from an older build, a value somebody
15+
* else's script left behind. The roster is this app's navigation, and the cost of the two mistakes
16+
* is not symmetric — opening a sidebar somebody wanted shut costs them one click, while shutting one
17+
* on a guess hides every channel they have behind an affordance they have not found yet.
18+
*/
19+
export function parseStoredSidebarOpen(value: string | null) {
20+
return value !== "collapsed";
21+
}
22+
23+
type SidebarEffects = {
24+
setStoredValue: (key: string, value: string) => void;
25+
};
26+
27+
/**
28+
* Records the state in the vocabulary the primitive already uses for it — `expanded` and
29+
* `collapsed`, the two values of its `data-state` attribute — rather than inventing a second pair.
30+
*/
31+
export function applySidebarOpen(open: boolean, effects: SidebarEffects) {
32+
effects.setStoredValue(SIDEBAR_STORAGE_KEY, open ? "expanded" : "collapsed");
33+
}

app/src/routes/_authed/_app.tsx

Lines changed: 3 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
import { createFileRoute, Outlet } from "@tanstack/react-router";
22
import { AppSidebar } from "@/components/app-sidebar/app-sidebar";
3-
import { SidebarProvider } from "@/components/ui/sidebar";
3+
import { SidebarShell } from "@/components/layout/sidebar-shell";
44

55
export const Route = createFileRoute("/_authed/_app")({
66
component: RouteComponent,
@@ -10,19 +10,11 @@ function RouteComponent() {
1010
return (
1111
// One viewport, never scrolls: panes scroll inside it. A growable shell lets the transcript's
1212
// scroller size against the page, grow it, and grow again.
13-
<SidebarProvider
14-
className="h-svh overflow-hidden"
15-
style={
16-
{
17-
"--sidebar-width": "340px",
18-
"--sidebar-width-mobile": "20rem",
19-
} as React.CSSProperties
20-
}
21-
>
13+
<SidebarShell className="h-svh overflow-hidden" width="340px">
2214
<AppSidebar />
2315
<main className="flex-1 flex flex-col min-h-0 overflow-hidden">
2416
<Outlet />
2517
</main>
26-
</SidebarProvider>
18+
</SidebarShell>
2719
);
2820
}

app/src/routes/_authed/_app/agents/index.tsx

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ import { AgentCard } from "@/components/agents/agent-card";
66
import { AgentProfile as AgentProfileDetail } from "@/components/agents/agent-profile";
77
import { NewAgent } from "@/components/agents/new-agent";
88
import { DetailPanel } from "@/components/layout/detail-panel";
9+
import { SidebarToggleBar } from "@/components/layout/sidebar-toggle";
910
import { StaggerItem } from "@/components/layout/stagger";
1011
import { Button } from "@/components/ui/button";
1112
import { Empty, EmptyHeader, EmptyTitle } from "@/components/ui/empty";
@@ -58,6 +59,7 @@ function AgentsScreen() {
5859
) : null
5960
}
6061
>
62+
<SidebarToggleBar />
6163
<div className="max-w-2xl px-4 w-full mx-auto">
6264
<div className="mt-12 w-full max-w-2xl">
6365
<div className="flex flex-row w-full items-center justify-between">

app/src/routes/_authed/_app/bot.tsx

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@ import { CopilotChat } from "@copilotkit/react-core/v2";
22
import { IconPlus } from "@tabler/icons-react";
33
import { useQuery } from "@tanstack/react-query";
44
import { createFileRoute } from "@tanstack/react-router";
5+
import { SidebarToggleBar } from "@/components/layout/sidebar-toggle";
56
import { Button } from "@/components/ui/button";
67
import { agentListQueryOptions } from "@/lib/agents/queries";
78
import { useActiveBot } from "@/lib/copilot/active-bot";
@@ -76,6 +77,7 @@ function BotChat({ agentId, name }: { agentId: string; name: string }) {
7677

7778
return (
7879
<div className="flex h-screen flex-col">
80+
<SidebarToggleBar />
7981
<header className="border-b px-6 py-3">
8082
<div className="flex items-baseline justify-between">
8183
{/*

app/src/routes/_authed/_app/channel/$channelId.tsx

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@ import { ActivityLog } from "@/components/computer/activity-log";
1717
import { ComputerView } from "@/components/computer/computer-view";
1818
import { useNeedsYou } from "@/components/computer/needs-you";
1919
import { DetailPanel } from "@/components/layout/detail-panel";
20+
import { SidebarToggle } from "@/components/layout/sidebar-toggle";
2021
import { Button } from "@/components/ui/button";
2122
import { markChannelReadMutationOptions } from "@/lib/channels/mutations";
2223
import {
@@ -177,6 +178,7 @@ function RouteComponent() {
177178
<div className="h-12 border-b border-border sticky top-0 flex flex-row items-center justify-between px-3 gap-2">
178179
{/* Keyed on the displayed name so cold channel loads animate the resolved name, not the id. */}
179180
<div className="flex min-w-0 items-center gap-1.5">
181+
<SidebarToggle />
180182
<motion.div
181183
animate={{ opacity: 1 }}
182184
className="shrink-0"

app/src/routes/_authed/_app/channel/new.tsx

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ import { ChannelAvatar } from "@/components/channels/avatar";
66
import { canSend, type Recipient } from "@/components/channels/compose-state";
77
import { ConversationView } from "@/components/channels/conversation-view";
88
import { seedMessage } from "@/components/channels/transcript-messages";
9+
import { SidebarToggle } from "@/components/layout/sidebar-toggle";
910
import {
1011
Combobox,
1112
ComboboxContent,
@@ -64,6 +65,7 @@ function RouteComponent() {
6465
return (
6566
<div className="flex h-full flex-col">
6667
<div className="h-12 border-b border-border sticky top-0 flex flex-row px-2 items-center">
68+
<SidebarToggle className="mr-1" />
6769
<span className="text-sm text-muted-foreground">To:</span>
6870
<Combobox
6971
// Do not auto-open when the recipient came from the URL; the field is already answered.

0 commit comments

Comments
 (0)