mirror of
https://github.com/paperclipai/paperclip.git
synced 2026-10-28 12:04:41 +01:00
fix(ui): secondary sidebar hard-forces the app rail collapsed, preserving the user's pin (PAP-10694)
A secondary sidebar (company settings, plugin routeSidebar) now ALWAYS collapses the app sidebar to its rail and hides its expand toggle — overriding the user's pin while the route is active — but the force is ephemeral and never mutates the persisted expanded/collapsed preference, so leaving the route restores exactly what the user chose. Previously the collapse was a soft route-request that the user pin won over, so a user who pinned the sidebar expanded saw both sidebars (240+240) on settings/ plugin routes and could not get the intended rail. - SidebarContext: new ephemeral `forceCollapsed` + `collapseLocked`; precedence becomes force > user pin > route request > default. `toggleCollapsed` is a no-op while forced (won't expand or write the pin). - Layout: drives `setForceCollapsed(hasSecondarySidebar)` instead of the soft `setRouteRequestsCollapsed`. - Sidebar: hides the expand/collapse affordance while `collapseLocked`. - Plugin docs (SPEC, authoring guide, SDK README) updated to the hard-force rule. - Tests: context forced-collapse precedence/lock/restore/mobile; Sidebar hidden toggle; Layout mock updated. Co-Authored-By: Paperclip <noreply@paperclip.ing>
This commit is contained in:
1 parent
9ca8cfd15d
commit
2dee4ba32e
9 files changed
+132
-31
No files matched your search
@@ -17,7 +17,7 @@ const mockInstanceSettingsApi = vi.hoisted(() => ({
|
||||
const mockNavigate = vi.hoisted(() => vi.fn());
|
||||
const mockSetSelectedCompanyId = vi.hoisted(() => vi.fn());
|
||||
const mockSetSidebarOpen = vi.hoisted(() => vi.fn());
|
||||
const mockSetRouteRequestsCollapsed = vi.hoisted(() => vi.fn());
|
||||
const mockSetForceCollapsed = vi.hoisted(() => vi.fn());
|
||||
const mockCompanyState = vi.hoisted(() => ({
|
||||
companies: [{ id: "company-1", issuePrefix: "PAP", name: "Paperclip" }],
|
||||
selectedCompany: { id: "company-1", issuePrefix: "PAP", name: "Paperclip" },
|
||||
@@ -173,11 +173,14 @@ vi.mock("../context/SidebarContext", () => ({
|
||||
toggleSidebar: vi.fn(),
|
||||
toggleCollapsed: vi.fn(),
|
||||
collapsed: mockSidebarState.collapsed,
|
||||
collapseLocked: false,
|
||||
peeking: mockSidebarState.peeking,
|
||||
setPeeking: mockSetPeeking,
|
||||
isMobile: mockSidebarState.isMobile,
|
||||
forceCollapsed: false,
|
||||
setForceCollapsed: mockSetForceCollapsed,
|
||||
routeRequestsCollapsed: false,
|
||||
setRouteRequestsCollapsed: mockSetRouteRequestsCollapsed,
|
||||
setRouteRequestsCollapsed: vi.fn(),
|
||||
}),
|
||||
}));
|
||||
|
||||
@@ -410,7 +413,7 @@ describe("Layout", () => {
|
||||
expect(container.textContent).not.toContain("Instance sidebar");
|
||||
expect(container.textContent).not.toContain("Plugin route sidebar");
|
||||
// The route asks the host to collapse the app sidebar to its rail.
|
||||
expect(mockSetRouteRequestsCollapsed).toHaveBeenCalledWith(true);
|
||||
expect(mockSetForceCollapsed).toHaveBeenCalledWith(true);
|
||||
|
||||
await act(async () => {
|
||||
root.unmount();
|
||||
@@ -474,7 +477,7 @@ describe("Layout", () => {
|
||||
expect(container.textContent).toContain("Main company nav");
|
||||
expect(container.textContent).not.toContain("Company rail");
|
||||
expect(container.textContent).not.toContain("Plugin route sidebar");
|
||||
expect(mockSetRouteRequestsCollapsed).toHaveBeenCalledWith(true);
|
||||
expect(mockSetForceCollapsed).toHaveBeenCalledWith(true);
|
||||
|
||||
await act(async () => {
|
||||
root.unmount();
|
||||
@@ -529,7 +532,7 @@ describe("Layout", () => {
|
||||
expect(container.textContent).toContain("Main company nav");
|
||||
expect(container.textContent).not.toContain("Company settings sidebar");
|
||||
expect(container.textContent).not.toContain("Instance sidebar");
|
||||
expect(mockSetRouteRequestsCollapsed).toHaveBeenCalledWith(true);
|
||||
expect(mockSetForceCollapsed).toHaveBeenCalledWith(true);
|
||||
|
||||
await act(async () => {
|
||||
root.unmount();
|
||||
@@ -713,8 +716,8 @@ describe("Layout", () => {
|
||||
expect(container.textContent).toContain("Main company nav");
|
||||
expect(container.textContent).not.toContain("Plugin route sidebar");
|
||||
// No secondary pane, so the route must not force the sidebar collapsed.
|
||||
expect(mockSetRouteRequestsCollapsed).not.toHaveBeenCalledWith(true);
|
||||
expect(mockSetRouteRequestsCollapsed).toHaveBeenCalledWith(false);
|
||||
expect(mockSetForceCollapsed).not.toHaveBeenCalledWith(true);
|
||||
expect(mockSetForceCollapsed).toHaveBeenCalledWith(false);
|
||||
|
||||
await act(async () => {
|
||||
root.unmount();
|
||||
|
||||
@@ -58,7 +58,7 @@ export function Layout() {
|
||||
peeking,
|
||||
setPeeking,
|
||||
isMobile,
|
||||
setRouteRequestsCollapsed,
|
||||
setForceCollapsed,
|
||||
} = useSidebar();
|
||||
const { openNewIssue, openOnboarding } = useDialogActions();
|
||||
const { togglePanelVisible } = usePanel();
|
||||
@@ -143,14 +143,15 @@ export function Layout() {
|
||||
queryFn: () => instanceSettingsApi.getGeneral(),
|
||||
}).data?.keyboardShortcuts === true;
|
||||
|
||||
// While a secondary sidebar is shown, ask the app sidebar to default to its
|
||||
// collapsed rail (still peek-able). An explicit user pin always wins — that
|
||||
// precedence lives in SidebarContext. Clearing on cleanup restores the
|
||||
// default when navigating off the takeover route (PAP-10695).
|
||||
// A secondary sidebar always collapses the app sidebar to its rail (still
|
||||
// peek-able) — a hard invariant that overrides the user pin while the route
|
||||
// is active, but does NOT mutate the persisted preference. Clearing the force
|
||||
// on cleanup restores the user's expanded/collapsed choice when navigating
|
||||
// off the takeover route (PAP-10694).
|
||||
useEffect(() => {
|
||||
setRouteRequestsCollapsed(hasSecondarySidebar);
|
||||
return () => setRouteRequestsCollapsed(false);
|
||||
}, [hasSecondarySidebar, setRouteRequestsCollapsed]);
|
||||
setForceCollapsed(hasSecondarySidebar);
|
||||
return () => setForceCollapsed(false);
|
||||
}, [hasSecondarySidebar, setForceCollapsed]);
|
||||
|
||||
useEffect(() => {
|
||||
if (companiesLoading || onboardingTriggered.current) return;
|
||||
|
||||
@@ -52,6 +52,7 @@ const mockSidebar = vi.hoisted(() => ({
|
||||
isMobile: false,
|
||||
setSidebarOpen: vi.fn(),
|
||||
collapsed: false,
|
||||
collapseLocked: false,
|
||||
peeking: false,
|
||||
toggleCollapsed: vi.fn(),
|
||||
setCollapsed: vi.fn(),
|
||||
@@ -315,6 +316,22 @@ describe("Sidebar", () => {
|
||||
});
|
||||
});
|
||||
|
||||
it("hides the expand/collapse toggle while a secondary sidebar locks the rail", async () => {
|
||||
// A secondary sidebar forces the rail; the user must not be able to expand
|
||||
// the primary while it is shown (PAP-10694).
|
||||
mockInstanceSettingsApi.getExperimental.mockResolvedValue({ enableIsolatedWorkspaces: false });
|
||||
mockSidebar.collapseLocked = true;
|
||||
const root = await renderSidebar();
|
||||
|
||||
expect(container.querySelector('button[aria-label="Collapse sidebar"]')).toBeNull();
|
||||
expect(container.querySelector('button[aria-label="Expand sidebar"]')).toBeNull();
|
||||
|
||||
mockSidebar.collapseLocked = false;
|
||||
flushSync(() => {
|
||||
root.unmount();
|
||||
});
|
||||
});
|
||||
|
||||
it("keeps the collapsed rail top bar to just the company logo (no clipped search/toggle)", async () => {
|
||||
// In the narrow rail the search/toggle controls don't fit beside the logo and
|
||||
// would overflow/clip, shoving the logo out of the icon column (PAP-10676), so
|
||||
|
||||
@@ -41,7 +41,7 @@ import { SidebarCompanyMenu } from "./SidebarCompanyMenu";
|
||||
export function Sidebar() {
|
||||
const { openNewIssue } = useDialogActions();
|
||||
const { selectedCompanyId, selectedCompany } = useCompany();
|
||||
const { isMobile, collapsed, peeking, toggleCollapsed, setCollapsed } = useSidebar();
|
||||
const { isMobile, collapsed, collapseLocked, peeking, toggleCollapsed, setCollapsed } = useSidebar();
|
||||
const rail = collapsed && !peeking;
|
||||
const inboxBadge = useInboxBadge(selectedCompanyId);
|
||||
const { data: experimentalSettings } = useQuery({
|
||||
@@ -94,8 +94,10 @@ export function Sidebar() {
|
||||
{/* Desktop-only collapse/expand affordance. While peeking (hover flyout
|
||||
over the collapsed rail) it becomes a Pin that promotes the peek to a
|
||||
pinned-expanded sidebar; otherwise it toggles the pinned rail. Mobile
|
||||
uses the off-canvas drawer, so this control is hidden there. */}
|
||||
{!isMobile ? (
|
||||
uses the off-canvas drawer, so this control is hidden there. It is
|
||||
also hidden while a secondary sidebar forces the rail (collapseLocked):
|
||||
the user cannot expand the primary while a secondary sidebar is shown. */}
|
||||
{!isMobile && !collapseLocked ? (
|
||||
peeking ? (
|
||||
<Button
|
||||
variant="ghost"
|
||||
|
||||
@@ -131,6 +131,53 @@ describe("SidebarContext", () => {
|
||||
});
|
||||
});
|
||||
|
||||
describe("forced collapse (secondary sidebar): overrides the pin, preserves preference", () => {
|
||||
it("forces collapsed even when the user pinned expanded, without mutating the pin", () => {
|
||||
active = renderProvider();
|
||||
// User prefers expanded site-wide.
|
||||
act(() => capturedValue?.setCollapsed(false));
|
||||
expect(capturedValue?.collapsed).toBe(false);
|
||||
expect(localStorage.getItem(COLLAPSED_STORAGE_KEY)).toBe("0");
|
||||
|
||||
// Entering a secondary-sidebar route forces the rail and locks it.
|
||||
act(() => capturedValue?.setForceCollapsed(true));
|
||||
expect(capturedValue?.collapsed).toBe(true);
|
||||
expect(capturedValue?.collapseLocked).toBe(true);
|
||||
// The persisted preference is untouched.
|
||||
expect(localStorage.getItem(COLLAPSED_STORAGE_KEY)).toBe("0");
|
||||
});
|
||||
|
||||
it("restores the user's preference when the force is cleared (leaving the route)", () => {
|
||||
active = renderProvider();
|
||||
act(() => capturedValue?.setCollapsed(false));
|
||||
act(() => capturedValue?.setForceCollapsed(true));
|
||||
expect(capturedValue?.collapsed).toBe(true);
|
||||
|
||||
// Navigating away clears the force; the expanded preference returns.
|
||||
act(() => capturedValue?.setForceCollapsed(false));
|
||||
expect(capturedValue?.collapsed).toBe(false);
|
||||
expect(capturedValue?.collapseLocked).toBe(false);
|
||||
});
|
||||
|
||||
it("locks the toggle while forced: toggleCollapsed is a no-op and never writes the pin", () => {
|
||||
active = renderProvider();
|
||||
act(() => capturedValue?.setCollapsed(false));
|
||||
act(() => capturedValue?.setForceCollapsed(true));
|
||||
|
||||
act(() => capturedValue?.toggleCollapsed());
|
||||
expect(capturedValue?.collapsed).toBe(true); // still forced
|
||||
expect(localStorage.getItem(COLLAPSED_STORAGE_KEY)).toBe("0"); // pin unchanged
|
||||
});
|
||||
|
||||
it("never forces or locks on mobile", () => {
|
||||
setViewport({ mobile: true });
|
||||
active = renderProvider();
|
||||
act(() => capturedValue?.setForceCollapsed(true));
|
||||
expect(capturedValue?.collapsed).toBe(false);
|
||||
expect(capturedValue?.collapseLocked).toBe(false);
|
||||
});
|
||||
});
|
||||
|
||||
describe("persistence round-trip", () => {
|
||||
it("writes '1'/'0' to localStorage on setCollapsed", () => {
|
||||
active = renderProvider();
|
||||
|
||||
@@ -18,12 +18,23 @@ interface SidebarContextValue {
|
||||
collapsed: boolean;
|
||||
setCollapsed: (next: boolean) => void;
|
||||
toggleCollapsed: () => void;
|
||||
// True while a secondary sidebar forces the rail: the collapse is locked, so
|
||||
// the expand/toggle affordance must be hidden/inert. Desktop-only.
|
||||
collapseLocked: boolean;
|
||||
// Ephemeral peek (hover flyout). Only meaningful on desktop, collapsed,
|
||||
// hover-capable pointer. Never persisted.
|
||||
peeking: boolean;
|
||||
setPeeking: (next: boolean) => void;
|
||||
// Route-requested collapse: a route may default the app sidebar to collapsed.
|
||||
// Lower precedence than an explicit user pin. Wired by Layout/routes (Task 5).
|
||||
// Hard, ephemeral collapse forced by an active secondary sidebar (settings,
|
||||
// plugin `routeSidebar`, …). HIGHER precedence than the user pin — the rule
|
||||
// is "a secondary sidebar always collapses the primary" — but it never
|
||||
// mutates the persisted pin, so leaving the route restores the preference.
|
||||
// Wired by Layout (PAP-10694).
|
||||
forceCollapsed: boolean;
|
||||
setForceCollapsed: (next: boolean) => void;
|
||||
// Route-requested collapse: a route may *default* the app sidebar to
|
||||
// collapsed. LOWER precedence than an explicit user pin. Wired by routes via
|
||||
// RequestCollapsedSidebar.
|
||||
routeRequestsCollapsed: boolean;
|
||||
setRouteRequestsCollapsed: (next: boolean) => void;
|
||||
}
|
||||
@@ -84,6 +95,7 @@ export function SidebarProvider({ children }: { children: ReactNode }) {
|
||||
// `null` = unpinned; an explicit user pin takes precedence over route request.
|
||||
const [userCollapsed, setUserCollapsed] = useState<boolean | null>(() => readStoredCollapsed());
|
||||
const [routeRequestsCollapsed, setRouteRequestsCollapsed] = useState(false);
|
||||
const [forceCollapsed, setForceCollapsed] = useState(false);
|
||||
const [rawPeeking, setRawPeeking] = useState(false);
|
||||
const [pointerCanPeek, setPointerCanPeek] = useState(() => readPointerCanPeek());
|
||||
|
||||
@@ -105,11 +117,16 @@ export function SidebarProvider({ children }: { children: ReactNode }) {
|
||||
return () => mql.removeEventListener("change", onChange);
|
||||
}, []);
|
||||
|
||||
// Precedence (highest wins): explicit user pin > route request > default expanded.
|
||||
const desktopCollapsed = userCollapsed !== null ? userCollapsed : routeRequestsCollapsed;
|
||||
// Precedence (highest wins): forced (active secondary sidebar) > explicit user
|
||||
// pin > route request > default expanded. The force is ephemeral and never
|
||||
// touches the persisted pin, so dropping it restores the user's preference.
|
||||
const pinnedOrRequested = userCollapsed !== null ? userCollapsed : routeRequestsCollapsed;
|
||||
const desktopCollapsed = forceCollapsed || pinnedOrRequested;
|
||||
// Collapsed/peek are desktop-only; mobile always uses the drawer. The user
|
||||
// pin is preserved across the breakpoint and reapplies on the desktop side.
|
||||
const collapsed = isMobile ? false : desktopCollapsed;
|
||||
// While forced, the pin is locked: the expand/toggle affordance is inert.
|
||||
const collapseLocked = !isMobile && forceCollapsed;
|
||||
// Peek only applies when collapsed on a hover-capable pointer.
|
||||
const peeking = rawPeeking && collapsed && pointerCanPeek;
|
||||
|
||||
@@ -119,8 +136,11 @@ export function SidebarProvider({ children }: { children: ReactNode }) {
|
||||
}, []);
|
||||
|
||||
const toggleCollapsed = useCallback(() => {
|
||||
setCollapsed(!desktopCollapsed);
|
||||
}, [desktopCollapsed, setCollapsed]);
|
||||
// While a secondary sidebar forces the rail, the toggle is locked: it must
|
||||
// neither expand the rail nor mutate the persisted preference.
|
||||
if (forceCollapsed) return;
|
||||
setCollapsed(!pinnedOrRequested);
|
||||
}, [forceCollapsed, pinnedOrRequested, setCollapsed]);
|
||||
|
||||
const setPeeking = useCallback((next: boolean) => {
|
||||
setRawPeeking(next);
|
||||
@@ -137,8 +157,11 @@ export function SidebarProvider({ children }: { children: ReactNode }) {
|
||||
collapsed,
|
||||
setCollapsed,
|
||||
toggleCollapsed,
|
||||
collapseLocked,
|
||||
peeking,
|
||||
setPeeking,
|
||||
forceCollapsed,
|
||||
setForceCollapsed,
|
||||
routeRequestsCollapsed,
|
||||
setRouteRequestsCollapsed,
|
||||
}),
|
||||
@@ -149,8 +172,10 @@ export function SidebarProvider({ children }: { children: ReactNode }) {
|
||||
collapsed,
|
||||
setCollapsed,
|
||||
toggleCollapsed,
|
||||
collapseLocked,
|
||||
peeking,
|
||||
setPeeking,
|
||||
forceCollapsed,
|
||||
routeRequestsCollapsed,
|
||||
],
|
||||
);
|
||||
|
||||
Reference in new issue
Block a user