Skip to content

#708 Expand Sidebar Nav Items Into In-Page Section Links - #716

Merged
b-at-neu merged 5 commits into
devfrom
708-expand-sidebar-nav-items-into-in-page-section-links
Sep 12, 2026
Merged

b-at-neu merged 5 commits into
devfrom
708-expand-sidebar-nav-items-into-in-page-section-links

Conversation

@b-at-neu

Copy link
Copy Markdown
Collaborator

Closes #708

Summary

  • Sidebar nav items can now expand into a nested list of in-page section links, discovered from the DOM rather than declared statically — a section opts in by putting id + data-section-nav="<Label>" on its outermost <section>, so the nav can never point at a group the page didn't actually render.
  • A client hook (useSectionNav) enumerates opted-in sections under <main>, scroll-spies with an IntersectionObserver rooted on <main> (not the window), and keeps the URL hash in sync via history.replaceState (never router.replace).
  • /manage/positions is the only page opted in for this ticket; ManagedPositionsSection's Open/Closed/Draft groups now carry the opt-in attributes.

Changes

  • lib/types.tsSectionNavItem, SectionVisibility.
  • lib/section-nav.ts (new) — pure helpers (buildSectionNavItems, selectActiveSectionId) plus the selector/root-margin constants; no React/DOM import, so tests/unit can exercise them directly.
  • components/layouts/use-section-nav.ts (new) — client hook: DOM discovery, IntersectionObserver scroll-spy, MutationObserver for streamed/late-appearing sections, hash sync, initial-hash scroll-on-load.
  • components/layouts/section-nav-list.tsx (new) — the nested <ul> of anchors; handles the Sheet's close-then-scroll sequencing so the scroll never fights the exit animation.
  • components/layouts/nav-list.tsx — calls the hook once, renders the nested list inside the active item's <li> only, adds the missing aria-current="page" on the nav <Link>.
  • components/layouts/app-shell.tsxid="main-content" and scroll-smooth motion-reduce:scroll-auto on <main> (the actual scroll container).
  • components/features/managed-positions-section.tsxPositionGroup grows sectionId and renders the opt-in attributes + scroll-mt-6.
  • docs/DESIGN.md — the opt-in pattern documented under §5.
  • tests/unit/section-nav.test.ts (new).

Testing plan

  • As an admin with positions in all three groups, open /manage/positions — Open, Closed and Draft appear indented under the active Manage Positions item.
  • Click Draft — the Draft heading scrolls to the top of the content area with a gap (not flush against the edge); the URL hash updates and the row highlights.
  • Scroll slowly top to bottom — the highlighted row tracks the section at the top; scrolling past the last section leaves Draft highlighted.
  • Sign in as a manager whose positions are all open (or draft only, i.e. fewer than two groups) — no sub-nav renders.
  • Navigate to Applications — the nested list disappears.
  • At 375px, open the hamburger, tap Closed — the Sheet closes and the page lands on the Closed group; the row is a comfortable touch target.
  • Keyboard: Tab from Manage Positions through the section rows — visible focus ring, Enter jumps; middle-click a row still opens the hash link in a new tab on desktop.
  • Enable prefers-reduced-motion in DevTools → Rendering — clicks jump instantly instead of smooth-scrolling.
  • Reload with #managed-positions-draft in the URL — the page lands on Draft.
  • Watch the page body on load in both themes — no content in <main> shifts when the sub-nav appears (hydration-only render, no layout shift).
  • Sign out / view as an anonymous visitor — no sidebar sub-nav appears anywhere it shouldn't (no active manage route).

Automated checks

  • npm run prettier:check
  • npm run eslint:check
  • npm run tsc:check
  • npm run test:unit (401 passed; tests/unit/email-delivery-events.test.ts fails in this sandbox only because DATABASE_URL isn't set — pre-existing, reproduced identically on a clean checkout before this change, unrelated to this PR)

Notes

  • Scope is limited to this ticket's acceptance criteria: only /manage/positions opts in. /manage/applications/[id] (needs SectionCard to grow an id) and the edit page (after Redesign The Edit Position Page #666 collapses its tabs) are follow-ups; the discovery helper already handles streamed/conditional sections for when those land.
  • Both NavList instances (desktop sidebar + mobile Sheet) run the hook simultaneously while the Sheet is open, so there can briefly be two IntersectionObservers over the same elements — cheap, and not worth a client context provider under a server-rendered shell (per plan).
  • scroll-smooth on <main> applies to every anchor/scrollIntoView inside the app shell, not just this feature.

b-at-neu and others added 3 commits September 11, 2026 22:19
DOM-discovered (option C): a section opts in with id +
data-section-nav, and a client hook enumerates them under the active
nav item, scroll-spies with an IntersectionObserver rooted on <main>,
and syncs the URL hash via replaceState. Manage Positions is the only
page opted in.

Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
@b-at-neu b-at-neu added the claude Will be worked on by Claude label Sep 12, 2026
@b-at-neu b-at-neu self-assigned this Sep 12, 2026
@vercel

vercel Bot commented Sep 12, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated
aplio Ready Ready Preview Sep 12, 2026 3:58am UTC

@b-at-neu b-at-neu added ready for review PR ready for review agent reviewing Review agent working (in-flight) and removed ready for review PR ready for review agent labels Sep 12, 2026

@b-at-neu b-at-neu left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review — Cycle 1 · needs revision

4 open — 4 🟡 Low (see inline)

Comment thread components/layouts/use-section-nav.ts Outdated
Comment thread components/layouts/section-nav-list.tsx Outdated
Comment thread lib/section-nav.ts Outdated
Comment thread components/layouts/section-nav-list.tsx Outdated
@b-at-neu b-at-neu added needs revision Review found issues that need fixing revising Revise agent working (in-flight) and removed reviewing Review agent working (in-flight) needs revision Review found issues that need fixing labels Sep 12, 2026
Compress oversized comments to fit the one-line bar, drop the ticket
provenance reference, and unify desktop/mobile section-link history
behavior so back only needs one press per navigation.

Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
@b-at-neu

Copy link
Copy Markdown
Collaborator Author

Revision — Cycle 1

fixed R1-L1, R1-L2, R1-L3, R1-L4 · b5d4e12

@b-at-neu b-at-neu added ready for review PR ready for review agent reviewing Review agent working (in-flight) and removed revising Revise agent working (in-flight) ready for review PR ready for review agent labels Sep 12, 2026

@b-at-neu b-at-neu left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review — Cycle 2 · approved

0 open — all 4 Cycle 1 Low findings (R1-L1..L4) resolved in b5d4e12, no regressions found

@b-at-neu b-at-neu added approved Review passed, ready to merge needs revision Review found issues that need fixing and removed reviewing Review agent working (in-flight) approved Review passed, ready to merge labels Sep 12, 2026
@b-at-neu

Copy link
Copy Markdown
Collaborator Author

Human feedback — post-approval revision

Simplify the section-nav hash ids. Currently managed-positions-section.tsx names its three opted-in sections managed-positions-open / managed-positions-closed / managed-positions-draft (plus -heading suffixed ids for aria-labelledby), which makes the URL hash #managed-positions-open instead of the cleaner #open.

Rename the three section ids to the bare open / closed / draft (keep the -heading suffix on the heading ids, e.g. open-heading, for the aria-labelledby wiring). Verified there is no existing id="open" / id="closed" / id="draft" anywhere in the /manage/positions render tree (page, PageHeader, PositionCreateDialog, ManagedPositionsSection/PositionGroup/PositionCard, ArchivedPositionsCollapsible, or the always-mounted AppShell/Sidebar/NavList/SectionNavList/AppFooter shell) — the shared form/select/radio primitives all use opaque React.useId() ids, not value-derived ones — so this is safe today with no collision.

Update tests/unit/section-nav.test.ts and any other test fixtures that assert the old managed-positions-* id strings.

@b-at-neu b-at-neu added revising Revise agent working (in-flight) and removed needs revision Review found issues that need fixing labels Sep 12, 2026
@b-at-neu

Copy link
Copy Markdown
Collaborator Author

Revision — human feedback (post-approval)

Addressed #716 (comment): renamed the three ManagedPositionsSection section ids in components/features/managed-positions-section.tsx from managed-positions-open / managed-positions-closed / managed-positions-draft to bare open / closed / draft, keeping the -heading suffix on the aria-labelledby targets (open-heading, closed-heading, draft-heading). Confirmed via grep that no other source file or test fixture (including tests/unit/section-nav.test.ts, which only exercises generic a/b/c fixture ids) referenced the old strings, so no other files needed updates. abdeb62

Verification: prettier, eslint, tsc all clean; npm run test:unit — 401 passed (the one unrelated failing suite needs DATABASE_URL, which isn't available in this sandbox and is pre-existing/environmental, not related to this change).

@b-at-neu b-at-neu added ready for review PR ready for review agent reviewing Review agent working (in-flight) and removed revising Revise agent working (in-flight) ready for review PR ready for review agent labels Sep 12, 2026

@b-at-neu b-at-neu left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review — Cycle 3 · approved

1 open — 1 🟡 Low (see inline)

{draft.length > 0 && (
<PositionGroup
headingId="managed-positions-draft-heading"
sectionId="draft"

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

R3-L1 🟡 Low — this commit renamed the hash id from managed-positions-draft to draft, but the PR body's testing-plan step still says "Reload with #managed-positions-draft in the URL". Fix: update that step to #draft (and the sibling #open/#closed mentions if any).

@b-at-neu b-at-neu added approved Review passed, ready to merge and removed reviewing Review agent working (in-flight) labels Sep 12, 2026
@b-at-neu
b-at-neu merged commit 2aba4ae into dev Sep 12, 2026
14 of 19 checks passed
@b-at-neu
b-at-neu deleted the 708-expand-sidebar-nav-items-into-in-page-section-links branch September 12, 2026 04:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Review passed, ready to merge claude Will be worked on by Claude

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Expand Sidebar Nav Items Into In-Page Section Links

1 participant