Conversation
…pandable sidebar tree
nhopeatall
left a comment
There was a problem hiding this comment.
Summary
Clean refactoring that correctly decomposes a monolithic tabbed page into URL-backed child routes. Architecture is sound, CI passes, and tests cover the shared helpers well. A few issues worth addressing.
Code Issues
Should Fix
-
Prefix-matching bug in sidebar (sidebar.tsx:82, 108) —
isActiveProjectusescurrentPath.startsWith(projectBasePath)without a trailing slash delimiter. Since project IDs are user-defined text strings (theprojectstable usestextprimary key), a project ID likeprojwould falsely match against/projects/project-2/general. TheisProjectActivehelper inproject-sections.tshandles this correctly by checkingstartsWith(\/projects/${projectId}/`)(note the trailing slash). The sidebar should import and use that helper — or at minimum add a trailing slash to thestartsWithcheck. The same issue applies to the inlineisSectionActive` computation at line 108. -
Missed back-link updates (prs/$projectId.$prNumber.tsx:36, work-items/$projectId.$workItemId.tsx:35) — These back-links still point to
to="/projects/$projectId"without/general. ThebeforeLoadredirect catches this so navigation won't break, but it causes an unnecessary client-side redirect on every click. These should be updated to/projects/$projectId/generalfor consistency with theprojects-table.tsxchange in this PR. -
Dead helper exports (project-sections.ts) —
isProjectActive,isSectionActive, andresolveDefaultProjectPathare exported and thoroughly tested (19 tests), but not consumed by any component or route. The sidebar duplicates the logic inline (with the bug above). Either the sidebar should import these helpers (fixing #1 at the same time), or the dead exports should be removed.
Nitpick
-
The re-exports in
$projectId.tsx(lines 12-13) for backward compatibility have zero consumers and can be removed. -
The old code had
overflow-y-auto max-h-48on the projects container to prevent the sidebar from being pushed down by many projects. The new code removed this constraint. With expandable tree items (each showing 5 subsection links), even a few projects could push settings/global nav off-screen. Worth considering if a scrollable container is still needed.
🕵️ claude-code · claude-opus-4-6 · run details
…links - Fix prefix-matching bug in ProjectNavItem by importing and using isProjectActive/isSectionActive helpers from project-sections.ts instead of inline startsWith checks (prevents false matches like /projects/proj matching /projects/project-2/general) - Update back-links in prs/$projectId.$prNumber.tsx and work-items/$projectId.$workItemId.tsx to point directly to /projects/$projectId/general, avoiding an unnecessary client-side redirect on every click - Remove dead re-exports from $projectId.tsx (ProjectSection, DEFAULT_PROJECT_SECTION, PROJECT_SECTIONS) which had zero consumers - Restore overflow-y-auto max-h-48 on the projects container to prevent expandable tree items from pushing settings/global nav off-screen Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add `route` property to PROJECT_SECTIONS with typed TanStack Router paths
(`/projects/$projectId/<section>`) and update the sidebar Link component to
use `to={section.route}` with `params={{ projectId: project.id }}` instead
of a template literal that violated TanStack Router's type-safe route
requirements, fixing the frontend build failure.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
CI Failures ResolvedRoot CauseThe frontend build was failing with a TypeScript error in TanStack Router v1 enforces type-safe Fixes Applied
Verification
🕵️ claude-code · claude-sonnet-4-6 · run details |
nhopeatall
left a comment
There was a problem hiding this comment.
Summary
Clean, well-structured refactoring that correctly decomposes a monolithic tab-based page into URL-backed routes with shared section definitions. CI passes, tests are thorough, and the architecture is sound. Two UX issues worth addressing.
Code Issues
Should Fix
-
web/src/components/layout/sidebar.tsx:80 —
useState(activeProject)only captures the initial value. If the user navigates to a different project via the projects table or a direct URL (not by clicking the sidebar toggle), the newly active project won't auto-expand and the previously active one won't collapse. Consider usinguseEffectto sync expansion state whenactiveProjectchanges, or derive expansion from bothisExpandedandactiveProjectso that the active project is always shown expanded. -
web/src/routes/prs/$projectId.$prNumber.tsx:36 & web/src/routes/work-items/$projectId.$workItemId.tsx:35 — The back-navigation links now point to /general, but users reach these pages from the Work section. The back-link should point to /work to return the user to where they came from.
🕵️ claude-code · claude-opus-4-6 · run details
…ink destinations - Use useEffect to keep sidebar project expansion in sync when activeProject changes due to URL navigation (not just initial mount) - Update back-navigation links in PR runs and work item runs pages to point to /work section instead of /general, since users reach these pages from Work Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
nhopeatall
left a comment
There was a problem hiding this comment.
Summary
Clean, well-structured refactoring that converts monolithic tab-based project detail into URL-backed routes with an expandable sidebar tree. The shared project-sections.ts module centralizes section definitions and provides well-tested helper functions. All CI checks pass.
Should Fix
-
Duplicate project data fetching (
$projectId.general.tsx:9,$projectId.harness.tsx:9): The shell route ($projectId.tsxline 10) and thegeneral/harnesschild routes both calluseQuery(trpc.projects.getById). TanStack Query deduplicates at the network level (same query key), so this isn't a bug — but it means the child routes carry their own loading/error states redundantly. Consider passing the project data down from the shell via TanStack Router outlet context, which would eliminate the duplicateuseQuerycalls and the duplicate loading/error UI in child routes. -
resolveDefaultProjectPathis unused (project-sections.ts:53): The helper is defined and tested but never imported anywhere outside tests. ThebeforeLoadredirect in$projectId.tsx:50hardcodes'/projects/$projectId/general'instead of using it (or at leastDEFAULT_PROJECT_SECTION). If the default section ever changes, only the helper would update — the redirect would be left behind.
Observations (non-blocking)
-
The sidebar projects container keeps
max-h-48which may feel tight when a project is expanded (5 child links + the parent header). With 2+ projects this could create nested scroll areas. Worth monitoring. -
Clicking a project name in the sidebar only toggles expand/collapse — it doesn't navigate. This is a UX change from the previous behavior where clicking navigated to the project. The tradeoff seems intentional (expand → pick section), just noting it.
🕵️ claude-code · claude-opus-4-6 · run details
Summary
/projects/$projectId/<section>project-sections.tslib module with canonical section definitions and active-path helpers/generalsubsectionChanges
New files
web/src/lib/project-sections.ts— canonical section definitions, helper functions (isProjectActive,isSectionActive,resolveDefaultProjectPath)web/src/routes/projects/$projectId.general.tsx— General section routeweb/src/routes/projects/$projectId.harness.tsx— Harness section routeweb/src/routes/projects/$projectId.work.tsx— Work section route (moves work queries here)web/src/routes/projects/$projectId.integrations.tsx— Integrations section routeweb/src/routes/projects/$projectId.agent-configs.tsx— Agent Configs section routetests/unit/web/project-navigation.test.ts— 19 unit tests for navigation logicModified files
web/src/routes/projects/$projectId.tsx— Converted from monolithic stateful page to a shell route with<Outlet>andbeforeLoadredirect; imports shared section definitionsweb/src/routes/route-tree.ts— Registers subsection routes as children of the project shell routeweb/src/components/layout/sidebar.tsx— Replaced flat project links with expandableProjectNavItemcomponents showing subsection links when activeweb/src/components/projects/projects-table.tsx— Updated click navigation to/generalsubsectionTest plan
npm test)npm run typecheck)npm run build:web)npm run lint)/projects/$projectIdredirects to/projects/$projectId/general<Sidebar>component)🔗 Card: https://trello.com/c/K6bkimb8/360-lets-refactor-sidebar-in-dashboard-ui-and-have-what-currently-is-project-tabs-general-work-integrations-etc-become-sidebar-eleme
🤖 Generated with Claude Code
🕵️ claude-code · claude-sonnet-4-6 · run details