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>
This commit is contained in:
Claude Opus 5
@@ -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.)
|
||||
|
||||
@@ -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,6 +88,8 @@ export const Dock = ({ items, className, boundaryRef }: DockProps) => {
|
||||
transition: 'transform 150ms ease-out',
|
||||
}}
|
||||
>
|
||||
{({ 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)' }}
|
||||
@@ -98,7 +100,7 @@ export const Dock = ({ items, className, boundaryRef }: DockProps) => {
|
||||
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',
|
||||
boxShadow: isActive ? `0 0 12px ${item.color}40` : 'none',
|
||||
}}
|
||||
>
|
||||
{item.image ? (
|
||||
@@ -107,10 +109,12 @@ export const Dock = ({ items, className, boundaryRef }: DockProps) => {
|
||||
item.icon && <item.icon className="h-5 w-5 md:h-6 md:w-6 text-white" />
|
||||
)}
|
||||
</div>
|
||||
{active && (
|
||||
{isActive && (
|
||||
<div className="absolute -bottom-1.5 w-1.5 h-1.5 rounded-full" style={{ background: item.color }} />
|
||||
)}
|
||||
</Link>
|
||||
</>
|
||||
)}
|
||||
</NavLink>
|
||||
);
|
||||
})}
|
||||
</div>
|
||||
|
||||
@@ -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,20 +109,25 @@ 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
|
||||
{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={`flex items-center gap-3 px-3 py-2.5 rounded-lg transition-colors ${active ? 'bg-white/20' : 'hover:bg-white/10'}`}
|
||||
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: active ? `0 0 12px ${item.color}40` : 'none',
|
||||
boxShadow: isActive ? `0 0 12px ${item.color}40` : 'none',
|
||||
}}
|
||||
>
|
||||
{item.image ? (
|
||||
@@ -134,12 +136,13 @@ export function Header({ dockItems }: HeaderProps) {
|
||||
item.icon && <item.icon className="h-4 w-4 text-white" />
|
||||
)}
|
||||
</div>
|
||||
<span className={`text-sm ${active ? 'text-white font-medium' : 'text-white/80'}`}>
|
||||
<span className={`text-sm ${isActive ? 'text-white font-medium' : 'text-white/80'}`}>
|
||||
{item.label}
|
||||
</span>
|
||||
</Link>
|
||||
);
|
||||
})}
|
||||
</>
|
||||
)}
|
||||
</NavLink>
|
||||
))}
|
||||
</nav>
|
||||
</SheetContent>
|
||||
</Sheet>
|
||||
|
||||
Reference in New Issue
Block a user