From 39125b50281d0cf2b2834c0ebe804fd71ee5ae9f Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Andr=C3=A9=20Padez?= <andre.padez@gmail.com>
Date: Fri, 7 Aug 2026 12:45:59 +0000
Subject: [PATCH] let the router decide which nav item is active

Dock, Header and the mobile sheet were computing active state from
useLocation with two copies of the same startsWith helper. react-router's
NavLink already knows. end is set for Home only: without it NavLink treats
'/' as an ancestor of every route, and with it on the others a detail route
would lose its highlight.

Segment matching is stricter than the string prefix it replaces, which is
what was meant all along.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
---
 docs/navigation-audit.md                      | 12 ++--
 .../Screens/Dashboard/Layout/Dock.tsx         | 60 +++++++++--------
 .../Dashboard/Layout/Header/Header.tsx        | 67 ++++++++++---------
 3 files changed, 75 insertions(+), 64 deletions(-)

diff --git a/docs/navigation-audit.md b/docs/navigation-audit.md
index f68f1b3f..f1e415da 100644
--- a/docs/navigation-audit.md
+++ b/docs/navigation-audit.md
@@ -86,7 +86,7 @@ are good building blocks. The **Workspace/Panel framework** contains **zero** ro
 | ~~M7~~ | ~~`workspaces/components/Combobox.tsx:53`~~ | caller-supplied route | — | **Deleted, not fixed.** "Every caller inherits the opaque click" was the reason this ranked MEDIUM, and it is wrong: `Combobox` has **no callers**. Nothing has imported it since the initial commit, there is no barrel export, and nothing anywhere sets `href` on a `SelectOption` — so the navigate, the separator that only showed for `href` options, and the `href` field on both declarations of the type were all unreachable. Writing anchor semantics into a component that is never rendered is building, not fixing. Its `Command` primitives stay; `AIHarnessesSection` uses them. |
 | ~~M8~~ | `Layout/Header/UserMenu.tsx` | — | — | **Done.** Removed rather than routed: nothing had ever been built behind `/settings/resources`, so the item was a bounce to `/` dressed as navigation. Its `header.userMenu.resources` locale keys went with it. |
 | ~~M9~~ | `Screens/Dashboard/Plans/index.tsx` | a plan document | `/plans/:name` | **Done.** Route pair, no `Navigate` guard — the bare route means "no plan open", which is a real state, so the auto-select-first effect was deleted rather than turned into a redirect. The `<select>` navigates instead of setting state; it stays a `<select>` on purpose (chrome for one document, not a master list) and therefore genuinely has no cmd-click — a native `<option>` cannot be an anchor. A name that no longer exists gets the empty pane, not a rewritten URL. Reading the server route for this also turned up a **path traversal**: hono percent-decodes route params, so `GET /api/plans/..%2F..%2Fsecret` reached `join(plansDir, '../../secret.md')`. Now `basename()`d. |
-| ~~M10~~ | `SystemMonitor/ScopeList.tsx` | monitor scope (btop/pm2/docker) | `/system-monitor/:scope` | **Done.** Route pair + `Navigate` guard; the scope buttons are `NavLink`s and `useMonitorScope` reads `useParams` instead of the channel. The Dock's hand-rolled `isActive` is a `startsWith`, so its highlight survives the redirect. |
+| ~~M10~~ | `SystemMonitor/ScopeList.tsx` | monitor scope (btop/pm2/docker) | `/system-monitor/:scope` | **Done.** Route pair + `Navigate` guard; the scope buttons are `NavLink`s and `useMonitorScope` reads `useParams` instead of the channel. The Dock's highlight survives the redirect — it was a `startsWith` then and is a `NavLink` ancestor match now, so the conclusion is unchanged. |
 
 **Music** (M-music) and **Soulseek** (M-slsk) are whole-workspace channel apps — pulled out below because each
 is **one design decision** that cascades across many files:
@@ -110,8 +110,12 @@ is **one design decision** that cascades across many files:
 
 ### 🟡 LOW / borderline
 
-- **Dock / Header active styling** (`Dock.tsx:43,78` · `Header.tsx:53,112`) — destinations are already real
-  `<Link>`s; only the *active* class is JS-derived from `useLocation`. Switch to react-router `<NavLink>` and drop the hand-rolled `isActive`.
+- ~~**Dock / Header active styling**~~ (`Dock.tsx` · `Header.tsx`) — **done.** Both are react-router
+  `<NavLink>`s now and the two copies of `isActive` are gone, along with the `useLocation` each needed.
+  One behavioural difference, deliberate: the hand-rolled version was a string `startsWith`, so `/task-logs`
+  would also have matched a hypothetical `/task-logsomething`; `NavLink` matches by path *segment*, which
+  is what was meant. `end` is set for Home only — without it `NavLink` treats `/` as an ancestor of every
+  route; with it on the others, a detail route (`/plans/x`, `/system-monitor/btop`) would lose its highlight.
 - ~~**Browser tabs**~~ (`Browser/TabList.tsx:93`) — **done, against this file's own advice.** The objection
   was that a CDP target id is ephemeral, so a durable `/browser/:tabId` is dubious. True of *bookmarking*,
   and irrelevant to everything else the URL buys: the id was in an onClick closure, three components read a
@@ -228,7 +232,7 @@ publishers means changing the chat panel, which is another agent's, so it is wri
 - [x] **M9** Plans → `/plans/:name`; the auto-select-first effect is gone (the bare route is a real state: no plan open), and the `<select>` navigates instead of setting state. It stays a `<select>` — a native `<option>` cannot be an anchor, so this one has no cmd-click and the doc should not pretend otherwise; it is chrome for a single document, not a master list. Reading the route also turned up a path traversal in `GET /api/plans/:name` (hono percent-decodes params, so `..%2F..%2Fx` walked out of `plansDir`) — fixed with `basename()`. **Needs runtime test.**
 
 ### Phase 4 — Polish + borderline decisions
-- [ ] Dock + Header + mobile sheet → react-router `<NavLink>` for active state; drop hand-rolled `isActive` (`Dock.tsx`, `Header.tsx`).
+- [x] Dock + Header + mobile sheet → react-router `<NavLink>`; both `isActive` helpers and their `useLocation`s deleted. `end` on Home only. **Needs runtime test.**
 - [ ] "New Chat" → `<Link to="/chat/new">` (`SessionList.tsx:68`) once H4's channel cleanup lands.
 - [ ] Jobs back button + any `navigate('/jobs')` → shared `BackButton` (`JobDetail.tsx:709`).
 - [ ] Decide/skip: Jobs step deep-link, Preview slug, FileBrowser widget. (Browser tabs: **done** — see the LOW section. Monitor scope: **done** as M10, it was not a view toggle. Music favorites: **decided** — stays a channel, reasoning in the LOW section.)
diff --git a/src/apps/officer-web/Screens/Dashboard/Layout/Dock.tsx b/src/apps/officer-web/Screens/Dashboard/Layout/Dock.tsx
index 72ffacfb..a27e01d2 100644
--- a/src/apps/officer-web/Screens/Dashboard/Layout/Dock.tsx
+++ b/src/apps/officer-web/Screens/Dashboard/Layout/Dock.tsx
@@ -1,5 +1,5 @@
 import { useEffect, useRef, useState } from 'react';
-import { Link, useLocation } from 'react-router';
+import { NavLink } from 'react-router';
 import type { LucideIcon } from 'lucide-react';
 
 export type DockItem = {
@@ -38,9 +38,6 @@ export const Dock = ({ items, className, boundaryRef }: DockProps) => {
   const [mouseX, setMouseX] = useState<number | null>(null);
   const [visible, setVisible] = useState(false);
   const dockRef = useRef<HTMLDivElement | null>(null);
-  const location = useLocation();
-
-  const isActive = (to: string) => (to === '/' ? location.pathname === '/' : location.pathname.startsWith(to));
 
   useEffect(() => {
     const handleMouseMove = (ev: MouseEvent) => {
@@ -75,12 +72,15 @@ export const Dock = ({ items, className, boundaryRef }: DockProps) => {
       {items.map((item, index) => {
         const iconCenter = DOCK_PADDING + index * (ICON_SIZE + ICON_GAP) + ICON_SIZE / 2;
         const scale = getScale(mouseX, iconCenter);
-        const active = isActive(item.to);
 
         return (
-          <Link
+          <NavLink
             key={item.to}
             to={item.to}
+            // Home would otherwise light up on every route: without `end`, NavLink treats "/" as an
+            // ancestor of everything. Every other item wants the ancestor match, so that a detail route
+            // (/plans/x, /system-monitor/btop) keeps its dock tile lit.
+            end={item.to === '/'}
             className="group relative flex flex-col items-center"
             style={{
               transform: `scale(${scale})`,
@@ -88,29 +88,33 @@ export const Dock = ({ items, className, boundaryRef }: DockProps) => {
               transition: 'transform 150ms ease-out',
             }}
           >
-            <span
-              className="absolute -top-9 px-2 py-1 rounded-md text-white text-xs whitespace-nowrap hidden md:block opacity-0 group-hover:opacity-100 transition-opacity duration-150 pointer-events-none"
-              style={{ backgroundColor: 'var(--dock-tooltip-bg)' }}
-            >
-              {item.label}
-            </span>
-            <div
-              className="w-10 h-10 md:w-12 md:h-12 rounded-xl flex items-center justify-center transition-all"
-              style={{
-                background: item.color,
-                boxShadow: active ? `0 0 12px ${item.color}40` : 'none',
-              }}
-            >
-              {item.image ? (
-                <img src={item.image} alt="" className="h-7 w-7 md:h-8 md:w-8 object-contain" />
-              ) : (
-                item.icon && <item.icon className="h-5 w-5 md:h-6 md:w-6 text-white" />
-              )}
-            </div>
-            {active && (
-              <div className="absolute -bottom-1.5 w-1.5 h-1.5 rounded-full" style={{ background: item.color }} />
+            {({ isActive }) => (
+              <>
+                <span
+                  className="absolute -top-9 px-2 py-1 rounded-md text-white text-xs whitespace-nowrap hidden md:block opacity-0 group-hover:opacity-100 transition-opacity duration-150 pointer-events-none"
+                  style={{ backgroundColor: 'var(--dock-tooltip-bg)' }}
+                >
+                  {item.label}
+                </span>
+                <div
+                  className="w-10 h-10 md:w-12 md:h-12 rounded-xl flex items-center justify-center transition-all"
+                  style={{
+                    background: item.color,
+                    boxShadow: isActive ? `0 0 12px ${item.color}40` : 'none',
+                  }}
+                >
+                  {item.image ? (
+                    <img src={item.image} alt="" className="h-7 w-7 md:h-8 md:w-8 object-contain" />
+                  ) : (
+                    item.icon && <item.icon className="h-5 w-5 md:h-6 md:w-6 text-white" />
+                  )}
+                </div>
+                {isActive && (
+                  <div className="absolute -bottom-1.5 w-1.5 h-1.5 rounded-full" style={{ background: item.color }} />
+                )}
+              </>
             )}
-          </Link>
+          </NavLink>
         );
       })}
     </div>
diff --git a/src/apps/officer-web/Screens/Dashboard/Layout/Header/Header.tsx b/src/apps/officer-web/Screens/Dashboard/Layout/Header/Header.tsx
index 5b0394ec..4f999132 100644
--- a/src/apps/officer-web/Screens/Dashboard/Layout/Header/Header.tsx
+++ b/src/apps/officer-web/Screens/Dashboard/Layout/Header/Header.tsx
@@ -1,5 +1,5 @@
 import { useState } from 'react';
-import { Link, useLocation } from 'react-router';
+import { Link, NavLink } from 'react-router';
 import { Menu } from 'lucide-react';
 // import { Menu, Terminal } from 'lucide-react'; // Terminal used by the commented-out web inspector button
 import { Sheet, SheetContent, SheetHeader, SheetTitle } from '@/components/ui/sheet';
@@ -58,9 +58,6 @@ type HeaderProps = {
 export function Header({ dockItems }: HeaderProps) {
   const [open, setOpen] = useState(false);
   const isTouch = useIsTouch();
-  const location = useLocation();
-
-  const isActive = (to: string) => (to === '/' ? location.pathname === '/' : location.pathname.startsWith(to));
 
   return (
     <header className="fixed z-10 w-full">
@@ -112,34 +109,40 @@ export function Header({ dockItems }: HeaderProps) {
               <SheetTitle>Navigation</SheetTitle>
             </SheetHeader>
             <nav className="flex flex-col gap-1 mt-4 px-2">
-              {dockItems.map((item) => {
-                const active = isActive(item.to);
-                return (
-                  <Link
-                    key={item.to}
-                    to={item.to}
-                    onClick={() => setOpen(false)}
-                    className={`flex items-center gap-3 px-3 py-2.5 rounded-lg transition-colors ${active ? 'bg-white/20' : 'hover:bg-white/10'}`}
-                  >
-                    <div
-                      className="w-8 h-8 rounded-lg flex items-center justify-center shrink-0"
-                      style={{
-                        background: item.color,
-                        boxShadow: active ? `0 0 12px ${item.color}40` : 'none',
-                      }}
-                    >
-                      {item.image ? (
-                        <img src={item.image} alt="" className="h-5 w-5 object-contain" />
-                      ) : (
-                        item.icon && <item.icon className="h-4 w-4 text-white" />
-                      )}
-                    </div>
-                    <span className={`text-sm ${active ? 'text-white font-medium' : 'text-white/80'}`}>
-                      {item.label}
-                    </span>
-                  </Link>
-                );
-              })}
+              {dockItems.map((item) => (
+                // `end` only for Home — see the note in Dock.tsx. Everything else wants the ancestor
+                // match so a detail route keeps its entry highlighted.
+                <NavLink
+                  key={item.to}
+                  to={item.to}
+                  end={item.to === '/'}
+                  onClick={() => setOpen(false)}
+                  className={({ isActive }) =>
+                    `flex items-center gap-3 px-3 py-2.5 rounded-lg transition-colors ${isActive ? 'bg-white/20' : 'hover:bg-white/10'}`
+                  }
+                >
+                  {({ isActive }) => (
+                    <>
+                      <div
+                        className="w-8 h-8 rounded-lg flex items-center justify-center shrink-0"
+                        style={{
+                          background: item.color,
+                          boxShadow: isActive ? `0 0 12px ${item.color}40` : 'none',
+                        }}
+                      >
+                        {item.image ? (
+                          <img src={item.image} alt="" className="h-5 w-5 object-contain" />
+                        ) : (
+                          item.icon && <item.icon className="h-4 w-4 text-white" />
+                        )}
+                      </div>
+                      <span className={`text-sm ${isActive ? 'text-white font-medium' : 'text-white/80'}`}>
+                        {item.label}
+                      </span>
+                    </>
+                  )}
+                </NavLink>
+              ))}
             </nav>
           </SheetContent>
         </Sheet>