diff --git a/__tests__/unit/hooks/useNavigationStorage.test.ts b/__tests__/unit/hooks/useNavigationStorage.test.ts index 197b53d12..29d8c0faf 100644 --- a/__tests__/unit/hooks/useNavigationStorage.test.ts +++ b/__tests__/unit/hooks/useNavigationStorage.test.ts @@ -82,9 +82,20 @@ describe('buildInitialCollapsedSections', () => { expect(collapsed.has('settings')).toBe(false); }); - it('on mobile collapses collapsible sections with priority > 3', () => { + it('gives the same defaults on mobile as on desktop — config is the only input', () => { + // Viewport used to override the config with `priority > 3`, so the same + // account saw different sections open on a phone than on a laptop and the + // declared `defaultExpanded` was a lie on one of them. + Object.defineProperty(window, 'innerWidth', { value: 1280, writable: true }); + const onDesktop = buildInitialCollapsedSections(basicSections); Object.defineProperty(window, 'innerWidth', { value: 375, writable: true }); - const mobileSections: NavSection[] = [ + const onMobile = buildInitialCollapsedSections(basicSections); + expect([...onMobile].sort()).toEqual([...onDesktop].sort()); + }); + + it('honours defaultExpanded regardless of priority', () => { + Object.defineProperty(window, 'innerWidth', { value: 375, writable: true }); + const sections: NavSection[] = [ { id: 'low', title: 'Low', items: [], collapsible: true, defaultExpanded: true, priority: 2 }, { id: 'high', @@ -95,9 +106,7 @@ describe('buildInitialCollapsedSections', () => { priority: 4, }, ]; - const collapsed = buildInitialCollapsedSections(mobileSections); - expect(collapsed.has('low')).toBe(false); - expect(collapsed.has('high')).toBe(true); + expect(buildInitialCollapsedSections(sections).size).toBe(0); }); it('returns empty set when no collapsible sections', () => { @@ -178,6 +187,26 @@ describe('useNavigationStorage', () => { expect(onStateLoaded).toHaveBeenCalledTimes(1); }); + it('keeps an empty saved set — expanding every section survives a reload', () => { + // "[]" means the user opened everything. Reading it as "nothing saved" and + // falling back to the defaults made that click silently revert on reload. + window.localStorage.setItem(STORAGE_KEYS.COLLAPSED_SECTIONS, '[]'); + + renderHook(() => useNavigationStorage(true, basicSections, { onStateLoaded, onLoadFailed })); + + expect(onStateLoaded).toHaveBeenCalledWith( + expect.objectContaining({ collapsedSections: new Set() }) + ); + }); + + it('falls back to config defaults only when nothing was ever saved', () => { + renderHook(() => useNavigationStorage(true, basicSections, { onStateLoaded, onLoadFailed })); + + expect(onStateLoaded).toHaveBeenCalledWith( + expect.objectContaining({ collapsedSections: new Set(['discover']) }) + ); + }); + it('loads saved state from localStorage', () => { window.localStorage.setItem(STORAGE_KEYS.SIDEBAR_OPEN, 'true'); window.localStorage.setItem(STORAGE_KEYS.SIDEBAR_COLLAPSED, 'true'); diff --git a/package.json b/package.json index c0f159403..b21d66dda 100644 --- a/package.json +++ b/package.json @@ -34,6 +34,7 @@ "check:sizes": "node scripts/check-file-sizes.js", "check:duplication": "node scripts/check-duplication.mjs", "check:dead-fields": "node scripts/check-dead-fields.mjs", + "check:dead-labels": "node scripts/check-dead-labels.mjs", "check:schema-columns": "node scripts/check-schema-columns.mjs", "check:currency-units": "node scripts/check-currency-units.mjs", "check:rpc-exists": "node scripts/check-rpc-exists.mjs", @@ -43,7 +44,7 @@ "check:user-scoped-deletes": "node scripts/check-user-scoped-deletes.mjs", "check:ai-models": "node scripts/check-ai-models.mjs", "check:mdx": "node scripts/check-mdx.mjs", - "verify": "npm run ci:docs && npm run check:accent-ink && npm run type-check && npm run type-check:scripts && npm run check:sizes && npm run audit:routes && npm run lint && npm run check:duplication && npm run check:dead-fields && npm run check:migration-versions && npm run check:schema-columns && npm run check:currency-units && npm run check:rpc-exists && npm run check:one-current-user && npm run check:app-locale && npm run check:client-ip && npm run check:user-scoped-deletes && npm run check:mdx && npm run test:unit -- --watchAll=false", + "verify": "npm run ci:docs && npm run check:accent-ink && npm run type-check && npm run type-check:scripts && npm run check:sizes && npm run audit:routes && npm run lint && npm run check:duplication && npm run check:dead-fields && npm run check:dead-labels && npm run check:migration-versions && npm run check:schema-columns && npm run check:currency-units && npm run check:rpc-exists && npm run check:one-current-user && npm run check:app-locale && npm run check:client-ip && npm run check:user-scoped-deletes && npm run check:mdx && npm run test:unit -- --watchAll=false", "audit:schema": "node scripts/db/audit-schema-drift.mjs", "audit:routes": "node scripts/audit-routes.mjs", "gen:types": "bash scripts/db/gen-types.sh", diff --git a/scripts/check-dead-labels.mjs b/scripts/check-dead-labels.mjs new file mode 100644 index 000000000..257895276 --- /dev/null +++ b/scripts/check-dead-labels.mjs @@ -0,0 +1,80 @@ +#!/usr/bin/env node +/** + * A heading that sits BESIDE a chevron-only toggle is a dead click. + * + * The shape: + * + *
+ *

Fund

<- looks like the control, isn't + * + *
+ * + * The visitor aims at the word, nothing happens, and the arrow beside it is a + * small target in a full-width row of apparent affordance. This landed twice + * in the same sidebar — once on the ContextSwitcher avatar ("should be + * clickable", reported by two different visitors), once on the Fund / + * Coordinate / Finance section headers — so the class is closed by a gate + * instead of a third fix. + * + * The correct shape puts the label INSIDE the control, which is what + * HeaderNavigation already does: + * + * + * + * Heuristic, so it is deliberately narrow: a heading, then within 12 lines a + * - )} - - )} + {/* Section Header - Hidden on desktop (icons only), visible on mobile (expanded). + The WHOLE row toggles, not just the chevron: a label that looks + like a control but isn't reads as a dead click, and the arrow + alone is a 44px target inside a full-width row of affordance. */} + {isExpanded && + (section.collapsible ? ( + + ) : ( + section.title && ( +
+

+ {section.title} +

+
+ ) + ))} - {/* Section Items - always show on desktop, respect collapse on mobile */} - {(!isExpanded || !section.collapsible || !isCollapsed || hasActiveItem) && ( -
+ {/* Section Items - always show on desktop, respect collapse on mobile. + An explicit toggle always wins: keeping a section open because it + happens to hold the active page made the chevron lie and the + click look dead. Landing on a page inside a collapsed section is + handled at load time, not by overriding the user here. */} + {(!isExpanded || !section.collapsible || !isCollapsed) && ( +
{section.items.map(item => ( 3 : ...` — + * so on a phone the declared config was ignored in favour of a bare magic + * number, and the same account saw different sections open on a phone than on + * a laptop with nothing recording why. A config field that a second rule can + * silently override is not a source of truth. One field, one behaviour: to + * change what opens by default, edit `defaultExpanded` in config/navigation. + */ export function buildInitialCollapsedSections(sections: NavSection[]): Set { - const isMobile = typeof window !== 'undefined' && window.innerWidth < 1024; const collapsed = new Set(); sections.forEach(section => { - if (section.collapsible) { - if (isMobile ? section.priority > 3 : !section.defaultExpanded) { - collapsed.add(section.id); - } + if (section.collapsible && !section.defaultExpanded) { + collapsed.add(section.id); } }); return collapsed; @@ -49,14 +57,14 @@ export function useNavigationStorage( const savedCollapsedState = localStorage.getItem(STORAGE_KEYS.SIDEBAR_COLLAPSED); const isSidebarCollapsed = savedCollapsedState ? JSON.parse(savedCollapsedState) : false; + // An empty saved set means "the user expanded everything" — it is NOT the + // same as "nothing was ever saved". Keying off size made expand-all + // silently revert to the defaults on the next load. const savedCollapsedSections = localStorage.getItem(STORAGE_KEYS.COLLAPSED_SECTIONS); - const collapsedFromStorage = savedCollapsedSections - ? new Set(JSON.parse(savedCollapsedSections)) - : new Set(); - - const defaultCollapsed = buildInitialCollapsedSections(sections); const collapsedSections = - collapsedFromStorage.size > 0 ? collapsedFromStorage : defaultCollapsed; + savedCollapsedSections === null + ? buildInitialCollapsedSections(sections) + : new Set(JSON.parse(savedCollapsedSections)); onStateLoaded({ isSidebarOpen, isSidebarCollapsed, collapsedSections }); } catch (error) { diff --git a/tests/unit/components/sidebar/SidebarNavigation.test.tsx b/tests/unit/components/sidebar/SidebarNavigation.test.tsx new file mode 100644 index 000000000..2750473e5 --- /dev/null +++ b/tests/unit/components/sidebar/SidebarNavigation.test.tsx @@ -0,0 +1,102 @@ +/** + * SidebarNavigation — section headers must be controls, and a toggle must toggle. + * + * George reported clicking "Fund" / "Coordinate" / "Finance" did nothing: the + * label was an

and only the small chevron beside it carried the onClick. + * This is the second time this class landed in the same sidebar (see + * ContextSwitcher.test.tsx — "the avatar must be a control, not a dead pixel"), + * so it gets a guard rather than a third fix. + * + * Three assertions, one per way the toggle used to lie to the user: + * 1. the whole header row is one control — clicking the WORD toggles + * 2. an explicit collapse wins even when the section holds the active page + * 3. the chevron's state matches what is actually rendered (aria-expanded) + */ + +import React from 'react'; +import { render, screen, fireEvent } from '@testing-library/react'; +import '@testing-library/jest-dom'; +import { Home } from 'lucide-react'; +import { SidebarNavigation } from '@/components/sidebar/SidebarNavigation'; +import type { NavSection } from '@/hooks/useNavigation'; + +jest.mock('next/navigation', () => ({ + useRouter: () => ({ push: jest.fn() }), + usePathname: () => '/projects', +})); + +jest.mock('@/stores/messaging', () => ({ + useUnreadCount: () => 0, +})); + +const SECTIONS: NavSection[] = [ + { + id: 'main', + title: '', + priority: 1, + collapsible: false, + items: [{ name: 'Home', href: '/dashboard', icon: Home }], + }, + { + id: 'fund', + title: 'Fund', + priority: 3, + collapsible: true, + items: [{ name: 'Projects', href: '/projects', icon: Home }], + }, +]; + +// /projects is active — it lives inside the collapsible "Fund" section +const isItemActive = (href: string) => href === '/projects'; + +function renderNav(collapsed: string[], toggleSection = jest.fn()) { + const view = render( + + ); + return { toggleSection, ...view }; +} + +describe('SidebarNavigation section headers', () => { + it('toggles when the section WORD is clicked, not just the chevron', () => { + const { toggleSection } = renderNav([]); + + // The label itself must sit inside a real control. + const header = screen.getByRole('heading', { name: 'Fund' }); + const control = header.closest('button'); + expect(control).not.toBeNull(); + + fireEvent.click(header); + expect(toggleSection).toHaveBeenCalledWith('fund'); + }); + + it('honours an explicit collapse even when the section holds the active page', () => { + renderNav(['fund']); + + // "Projects" is the active route; collapsing "Fund" used to be a no-op + // because an active child forced the list to stay rendered. + expect(screen.queryByText('Projects')).not.toBeInTheDocument(); + }); + + it('reports expanded state that matches what is rendered', () => { + const { unmount } = renderNav(['fund']); + expect(screen.getByRole('heading', { name: 'Fund' }).closest('button')).toHaveAttribute( + 'aria-expanded', + 'false' + ); + unmount(); + + renderNav([]); + expect(screen.getByRole('heading', { name: 'Fund' }).closest('button')).toHaveAttribute( + 'aria-expanded', + 'true' + ); + expect(screen.getByText('Projects')).toBeInTheDocument(); + }); +});