From 9dfe515edf9f1323a5ea8913af1354337dcf19e1 Mon Sep 17 00:00:00 2001 From: Georgy Butaev <41178744+g-but@users.noreply.github.com> Date: Sat, 29 Aug 2026 10:39:03 +0200 Subject: [PATCH 1/2] fix(nav): the sidebar section label was not a control MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Clicking "Fund" / "Coordinate" / "Finance" did nothing — the label was an

in a
and only the small chevron beside it carried the onClick. Three defects in the same family, all of them "the click did nothing": 1. The whole header row is now one - )} -
- )} + {/* 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(); + }); +}); From 3cb2f6c2824e6ff9568c709e2cac3a8bea7648b2 Mon Sep 17 00:00:00 2001 From: Georgy Butaev <41178744+g-but@users.noreply.github.com> Date: Sat, 29 Aug 2026 11:21:44 +0200 Subject: [PATCH 2/2] chore(ci): gate the dead-label shape instead of fixing it a third time MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A heading sitting BESIDE a chevron-only toggle is a dead click: the visitor aims at the word and nothing happens, while the only real target is a small arrow in a full-width row of apparent affordance. This shape has now been reported twice in this one sidebar — on the ContextSwitcher avatar (two separate visitors, see its test's header) and on the Fund / Coordinate / Finance section headers, fixed in the previous commit. Per the never-twice rule the third occurrence should be impossible, not cheap, so the class gets a check rather than another sweep. check:dead-labels scans src/components for a heading followed within 12 lines by a + *

+ * + * 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 + *