Jobs step deep-link: skipped, and measured first — selectedKey is plain useState, not a channel, so it breaks none of this document's rules. The only thing anchor semantics would buy is a deep link nobody asked for. Preview slug: void, there is no Preview app. FileBrowser widget: stays local, and M4 turned that shrug into a rule — only a workspace guaranteed to host one browser may own the address bar. Phases 1-4 are now closed except H4 and the New Chat button, both of which live in the chat nucleus. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
286 lines
32 KiB
Markdown
286 lines
32 KiB
Markdown
# Frontend Navigation Audit — the "opaque click" anti-pattern
|
||
|
||
> **Status, 2026-08-07. Phases 1–4 are closed except the two chat items.** Every finding H1–H5 and M1–M10
|
||
> is done, void, or decided-and-recorded, and so is every Phase 4 line. What remains, and only this:
|
||
> **H4** (chat detail reading `sessionId` from `useParams` instead of the `chat:selected-session` channel)
|
||
> and the **"New Chat"** button that depends on it. Both are inside the chat nucleus, which has its own
|
||
> owner — they are not stalled here, they are somebody else's to land.
|
||
>
|
||
> Three findings turned out to be wrong on inspection and are marked as such rather than quietly dropped:
|
||
> **H3** is void (Projects was deleted, so every `Projects/`, `SELECTED_PROJECT` and `/projects/:id`
|
||
> reference below is historical), **M7**'s "every caller inherits the opaque click" was false (it had no
|
||
> callers, and the component is deleted), and the **Preview slug** item names an app that does not exist.
|
||
> Two components — `Combobox` and `BackButton` — were deleted rather than fixed, having never been used.
|
||
>
|
||
> The rules in this file are current and authoritative; the findings table is a snapshot.
|
||
> **Nothing below has had a runtime click-through.** Every "Needs runtime test" note is still true.
|
||
|
||
|
||
**Date:** 2026-07-30 · **Origin:** written as exploration before any of the routing work was done.
|
||
|
||
Prep work for the upcoming **full navigation refactor**. This catalogues every place the frontend
|
||
navigates to / selects an **addressable entity** using a `<button onClick>` / `<div onClick>` /
|
||
imperative `navigate()` / global-channel setter **instead of a real `<Link to>` / `<a href>`**.
|
||
|
||
> Line numbers were captured on `sidecars-claude` at audit time and may drift as other work lands —
|
||
> treat them as anchors, re-grep the symbol if a line looks off.
|
||
|
||
---
|
||
|
||
## The anti-pattern (definition)
|
||
|
||
A clickable element selects/opens something that has (or should have) a URL, but:
|
||
- **(a)** the entity id/slug is **not in the DOM** (no `href`, no `data-*`) — it lives only in an onClick closure;
|
||
- **(b)** clicking **doesn't change the URL** (or does so only via an indirect state→URL effect);
|
||
- **(c)** selection is held in **JS state / a global channel** (`usePanelChannel`, `useGlobal`), not the URL;
|
||
- **(d)** **no anchor semantics** — can't cmd/ctrl-click to a new tab, no middle-click, not link-focusable.
|
||
|
||
**Exemplar (already fixed):** the `/chat` session list. Rows were `<button onClick={() => selectById(id)}>`
|
||
(id only in the closure) → converted to `<Link to={`/chat/${session.id}`}>` (committed to master `f35c145`).
|
||
That fix is the template for the HIGH items below. **Caveat:** the fix only did the *rows* — the chat
|
||
**detail panel** still selects via channel, not the URL (finding **C1**), so `/chat` is the model for both
|
||
"done right" (rows) and "still to do" (detail).
|
||
|
||
## Route map (from `App.tsx`)
|
||
|
||
**Existing entity routes:** `/chat/:sessionId`, `/jobs/:id`, `/dashboards/:id`, `/email/:emailId`,
|
||
`/browser/:tabId`, `/tasks/:dirName`, `/skills/:dirName`, `/processes/:dirName`, `/task-logs/:id`,
|
||
`/activity/:id`, `/plans/:name`. (`/projects/:id`
|
||
was in this list; the Projects feature was removed end to end on 2026-07-30, so the route is gone with it. H3
|
||
and navigate-site 22 below still name it — they are the record of work that happened, not of code that exists.)
|
||
**Section routes** (a param that names a view rather than an entity): `/settings/:page/:section`,
|
||
`/soulseek/:section`, `/headscale/:section`, `/wallet/:section`, `/system-monitor/:scope`.
|
||
**Query-param screens:** `/music?path=`, `/files?path=`, `/code-editor?file=`.
|
||
**Genuinely flat, and correctly so:** `/` `/terminal` `/desktop` `/qr-transfer` — none of them selects
|
||
anything, so there is nothing to put in an address.
|
||
The five settings pages are route **pairs** now, not flat screens — `/settings/{profile,ai,system,integrations,user-management}`
|
||
plus a `:section` each (M6). There has never been a `/settings/apps`; that entry was wrong when this list
|
||
was written.
|
||
|
||
**What's already correct** (lean on these in the refactor): the **Dock**, **Header** (logo + mobile sheet),
|
||
**UserMenu**, **JobsIndicator** are all real `<Link>`s. Shared `NavLink.tsx` (query-string-appending `<Link>`
|
||
wrapper — note: *not* react-router's NavLink, gives no active state) and `BackButton.tsx` (`<Link>` back arrow)
|
||
are good building blocks. The **Workspace/Panel framework** contains **zero** route navigation — it's orthogonal.
|
||
|
||
---
|
||
|
||
## Consolidated findings — severity ranked
|
||
|
||
### 🔴 HIGH — addressable route already exists; just needs a `<Link>` / URL-as-source-of-truth
|
||
|
||
| ID | file:line | Entity | Current impl | Fix |
|
||
|----|-----------|--------|--------------|-----|
|
||
| H1 | `Screens/Dashboard/Jobs/JobsPage.tsx:108` | a job | `<button onClick={() => navigate(`/jobs/${job.id}`)}>` — id in closure | → `<Link to={`/jobs/${job.id}`}>`. Active-row already keys off `useParams().id`; keep the stop/delete button. **The exact twin of the /chat fix.** |
|
||
| H2 | `workspaces/…/apps/Dashboards/DashboardListApp.tsx:133` | a dashboard | `<div onClick={handleClick}>` → `useGlobal(SELECTED_DASHBOARD_KEY)` on-page (**no URL change**), `navigate()` off-page | rows → `<Link to={`/dashboards/${ws.id}`}>`; drop the global as selection source (derive from `useParams`). Header is already a `<Link>` — app is internally inconsistent. |
|
||
| H3 | `workspaces/…/apps/Projects/ProjectListApp.tsx:161` | a project | `<div onClick={handleClick}>` → `useGlobal(SELECTED_PROJECT)` on-page (**no URL change**), `navigate()` off-page | identical to H2 → `<Link to={`/projects/${p.id}`}>`; retire `SELECTED_PROJECT` as source of truth. |
|
||
| H4 | `workspaces/…/apps/ChatHistory/ChatDetailPanel.tsx:136` | which chat to render | reads `usePanelChannel('chat:selected-session')`, **not** `useParams` | make the route the source of truth: read `sessionId` from `useParams`, fetch by id, retire `chat:selected-session` (or make it a derived cache). **Finishes the /chat fix.** |
|
||
| ~~H5~~ | ~~`Screens/Dashboard/Email/EmailList.tsx:336`~~ | ~~an email~~ | ~~`<button data-email-id onClick={setSelectedId}>` + `EMAIL_SELECTED` channel + a state↔URL sync effect (`EmailScreen.tsx:27-33`)~~ | **Done.** Rows are `<Link to={emailPath(msg.id)}>`, list + reader + mobile-panel all read `useSelectedEmailId()` (`Email/shared.ts`), and `EMAIL_SELECTED` plus both sync effects are deleted. Arrow-key browsing navigates with `replace` so a sweep doesn't stack history. |
|
||
|
||
> ~~**Verify:**~~ **Verified.** Of the four `navigate('/…/:id')` calls flagged here, the two in
|
||
> `ProjectPreview` are void (Projects was deleted on 2026-07-30) and one of the two in `DashboardPreview`
|
||
> is a genuine post-create redirect. The apps auditor was right and the nav auditor was wrong: exactly one
|
||
> — the "Open Dashboard" button in the edit form — was an opaque click, and it is a `<Link>` now.
|
||
|
||
### 🟠 MEDIUM — navigable entity with **no route yet** (add a route, then link)
|
||
|
||
| ID | file:line | Entity | Proposed route | Note |
|
||
|----|-----------|--------|----------------|------|
|
||
| ~~M1~~ | ~~`Screens/Dashboard/CapabilityPage.tsx:431`~~ | task / skill / process | `/tasks/:dirName`, `/skills/:dirName`, `/processes/:dirName` | **Done.** One component backed three screens, so one change covered all of them. The auto-select-`items[0]` effect is gone — the bare route is now the list with an empty detail pane. `editing`/`isNew` moved to `?edit=1` / `?new=1` because a `<Link>` row cannot imperatively reset them. |
|
||
| ~~M2~~ | ~~`Screens/Dashboard/TaskLogs/index.tsx:104`~~ | a task-log run | `/task-logs/:id` | **Done.** As predicted — the detail fetch already keyed off the id, so only its source changed. `showDetail` is gone; the mobile swap and both back arrows derive from the param. |
|
||
| ~~M3~~ | ~~`Screens/Dashboard/Activity/ActivityScreen.tsx:63,73`~~ | background task / detached job | `/activity/:id` | **Done.** One param for both row kinds; the screen looks the id up in the polled registry and derives `task=`/`path=` from the row. The SSE effect now depends on that derived *string*, so the 3s poll no longer risks re-opening the stream. An id that has left the registry says so instead of hanging on "waiting for output". |
|
||
| ~~M4~~ | ~~`FileBrowser/.../useFileBrowserApp.ts:269`~~, `FileItem.tsx:516`, ~~`Breadcrumb.tsx:16`~~ | a folder | `/files?path=<dir>` | **Partly done — the rest is an owner decision, not a defect.** `currentPath` is `?path=` on `/files`, so back/forward and linking a folder work, and the crumbs are `<Link>`s. Two things the audit line did not know: `?view=` is *ephemeral* (wiped on mount by `useFileViewerPanels`), so `path` is the screen's first durable param, and four `setSearchParams({…})` calls replaced the whole query string — opening any file would have silently reset the folder. They go through a `setViewerParams` helper now that keeps `path`. Opt-in via the parsed `WorkspaceIdentity` (`screens/files`), because a dashboard can hold two browsers and one shared param would move both. **Folder *items* stay buttons:** ⌘/Ctrl/Shift-click is already bound to multi-select in `FileItem.tsx` and open is double-click, so anchor semantics collide with an existing gesture. |
|
||
| ~~M5~~ | ~~`CodeEditor/FileTree.tsx:59`, `EditorTabs.tsx:33`~~ | open source file / active tab | `/code-editor?file=<path>` | **Done, minus `open=`.** The active file is `?file=`; tree *file* rows and tabs are `<Link>`s. **The tab set stays local** — it is a working session, not an address: it grows without bound, each entry costs a read on load, and nobody links someone else to a tab bar. A `?file=` naming a file that is not open now *opens* it, which is what makes a pasted link work; a path that fails to read is remembered so a bad link errors once instead of once per render, and the address is left alone rather than rewritten. **Tree folder rows stay buttons** — unlike the M4 case this needs no owner call, because expanding a directory is disclosure, not navigation. Two things fixed in passing: the tab close control was a `role="button"` span *nested inside* the tab (invalid then, a nested interactive inside an anchor now) and is a sibling `<button>` with an `aria-label`; and `closeFile` computed the next-active file *inside* a `setFiles` updater, which is exactly the impurity React double-invokes to catch. |
|
||
| ~~M6~~ | `Settings/SettingsPanel.tsx` | a settings sub-section | `/settings/:page/:section` | **Done.** `<NavLink>` + `useParams`, five `*_SELECTED` globals gone, one `SettingsRoute` guard per page. The "one change covers all settings pages" claim was *almost* right: Integrations builds its own sidebar and did not go through `createSettingsPanelComponents`, and it also held the Enterprise/Personal tab in a second global — derived from the section key now, which is what fixes deep-linking a Personal section. |
|
||
| ~~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 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:
|
||
|
||
- ~~**Music**~~ — **done.** The library location is `/music?path=<rel>` (`rel` relative to the `Music` root);
|
||
`music:cwd` is deleted. Each panel calls `useMusicCwd()` and reads the param itself, so `MusicBrowser`,
|
||
`MusicDetail` and `FavoritesView` no longer tell each other anything. Every drill-in is a `<Link>` — library
|
||
rows, folder rows, album/artist cards, both "up" affordances, the favorites rows, and the dock's now-playing
|
||
tile. Track rows stay `<button>`s: they play, which is a mutation. Two things that were channel-shaped and
|
||
stayed channels: `music:favorites` (a view toggle over this one panel) and `music:resync` (a refresh signal).
|
||
A query param rather than `/music/*` because the location is only one of the things this screen holds — the
|
||
lyrics split and the favorites view are the others — and because a splat has to be a route's last segment.
|
||
**Needs runtime test.**
|
||
- ~~**Soulseek**~~ — **mostly done.** The section is `/soulseek/:section` (nav entries are `NavLink`s, the
|
||
view panel reads the same URL), the peer is `?user=<name>`, and the past-search was already `?search=<id>`.
|
||
The `soulseek:section` and `soulseek:user` channels are deleted; `soulseek:refresh` stays, which is a
|
||
signal and not selection. The peer went in the query string rather than `/soulseek/users/:name` as
|
||
sketched here, because the section nav has to stay a one-segment `NavLink` — a second path segment would
|
||
need a nested route just to keep the highlight, and the query string is what `?search=` already uses.
|
||
Still on `useState`: rooms (`SoulseekRooms`) and conversations (`SoulseekChat`).
|
||
|
||
### 🟡 LOW / borderline
|
||
|
||
- ~~**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
|
||
`BROWSER_SELECTED_TAB` global, and the row could not be cmd-clicked. Staleness is handled where it
|
||
actually shows up — the preview now distinguishes "no tab open" from "that tab is no longer attached"
|
||
by checking the polled target list, which it gets from the same React Query key the list uses, so it
|
||
costs no extra request. En route: the row's Focus and Close buttons were nested *inside* the row
|
||
`<button>`, which is invalid HTML and only worked because of two `stopPropagation` calls; they are
|
||
siblings of the anchor now. And its "Set up in Integrations" was a raw `<a href>` that reloaded the SPA.
|
||
- **Jobs step/iteration** (`Jobs/JobDetail.tsx`) — **decided: skipped, and it is not an anti-pattern.**
|
||
Measured rather than assumed: `selectedKey` is plain `useState` behind a context, not a channel and not a
|
||
global, so it breaks none of the rules this document sets. What it lacks is anchor semantics, and the only
|
||
thing that buys is a `/jobs/:id/:stepKey` deep link — a feature nobody has asked for, on keys that are
|
||
array indexes and iteration labels. Building it is a product call, not a fix. **Owner's if he wants it.**
|
||
- ~~**Preview slug**~~ (`Preview/PreviewApp.tsx:23`) — **void.** There is no Preview app. Nothing in the tree
|
||
matches `PreviewApp`, and it is in no registry. It went with the Projects removal on 2026-07-30, or earlier.
|
||
- **FileBrowser widget** (`FileBrowserWidget/.../BrowseTab.tsx`) — **decided: stays local**, and M4 turned
|
||
the shrug into a rule. A widget sits on dashboards next to other browsers, and one shared `?path=` would
|
||
drive all of them; only a workspace guaranteed to host exactly one browser may own the address bar, which
|
||
is why the widget passes no `searchForPath` and its breadcrumb stays buttons. Same test, same answer.
|
||
- **Music favorites view** — the channel is `music:favorites`, not `music:fav`. **Decided: it stays a channel.**
|
||
It is a view of the detail panel rather than a location, it survives no reload worth surviving, and the one
|
||
case where its being off-URL shows is handled explicitly: going Home or opening a favorite closes it by hand,
|
||
because navigating to where you already are fires no route change. (**SystemMonitor scope** was listed here
|
||
as the same kind of thing and was not — it is M10, and it is in the URL.)
|
||
- **"New Chat" button** (`ChatHistory/SessionList.tsx:68`) — a create-action that also `navigate('/chat/new')`; could be `<Link to="/chat/new">` if the state-set moves into the route. Its `chat:selected-session` write is part of the H4 cleanup.
|
||
|
||
---
|
||
|
||
## Channel-based selection map (the core of the refactor)
|
||
|
||
Every place an **addressable entity** is selected through a global channel / global state instead of the URL.
|
||
This is the primary surface to convert to URL-driven selection.
|
||
|
||
| Channel / global key | Entity held | Should map to | Files |
|
||
|----------------------|-------------|---------------|-------|
|
||
| `chat:selected-session` | open chat session | `/chat/:sessionId` | `ChatDetailPanel.tsx:136`, `SessionList.tsx:16` (H4) |
|
||
| `chat:active-cwd` | chat working dir | query param on `/chat` | `ChatDetailPanel.tsx:102`, `SessionList.tsx:14` |
|
||
| `SELECTED_DASHBOARD_KEY` (`useGlobal`) | selected dashboard | `/dashboards/:id` | `DashboardListApp.tsx:36`, `DashboardPreview.tsx:287` (H2) |
|
||
| `SELECTED_PROJECT` (`useGlobal`) | selected project | `/projects/:id` | `ProjectListApp.tsx:45`, `ProjectPreview.tsx:386` (H3) |
|
||
| local `useState` selections | search / room / conversation / peer / open file / preview slug | respective nested routes | `SearchView`, `SoulseekRooms`, `SoulseekChat`, `SoulseekUsers`, `CodeEditor/useEditorState`, `PreviewProvider` |
|
||
|
||
**Excluded — event-bus / refresh signals, NOT selection:** `files:refresh-signal`,
|
||
`SLSKD_REFRESH_CHANNEL`, `MUSIC_RESYNC_CHANNEL`.
|
||
(`FILE_VIEWER_CHANNEL` was listed here too; it had no publisher and has been deleted — the file viewer
|
||
reads `?view=` from the URL. `preview:refresh` and `chat:active-session` were also listed, and
|
||
`preview:refresh` was cited above as the exemplar of a *legitimate* channel — but both have a publisher
|
||
in `ChatPanelWrapper` and **no subscriber at all**, and `preview:refresh`'s reader, `PreviewProvider`, is
|
||
no longer in the repo. They are declared in `officerdev/src/channels.ts` with that stated; deleting the
|
||
publishers means changing the chat panel, which is another agent's, so it is written up in
|
||
`COMMS/chat-agent-handoff-2026-08-07.md` instead. Use `files:refresh-signal` as the exemplar.)
|
||
|
||
---
|
||
|
||
## Appendix A — all `navigate()` call sites (whole frontend, 25)
|
||
|
||
"Link?" = a `<Link>`/`<NavLink>` is the right refactor.
|
||
|
||
| # | file:line | what | target | Link? | note |
|
||
|---|-----------|------|--------|-------|------|
|
||
| 1 | `Jobs/JobsPage.tsx:109` | job list row | `/jobs/:id` | **YES** | H1 |
|
||
| 2 | `Dashboards/DashboardListApp.tsx:90` | dashboard row (off-page) | `/dashboards/:id` | YES | H2 |
|
||
| 3 | `Dashboards/DashboardListApp.tsx:114` | inside "New Dashboard" | `/dashboards` | ~ | create action |
|
||
| ~~4~~ | `Dashboards/DashboardPreview.tsx` (create) | open dashboard | `/dashboards/:id` | no | **stays.** post-create redirect — a link cannot write the row first |
|
||
| ~~5~~ | `Dashboards/DashboardPreview.tsx` (edit form) | open dashboard | `/dashboards/:id` | YES | **done** — `<Button asChild><Link>` |
|
||
| 6 | `Projects/ProjectListApp.tsx:116` | project row (off-page) | `/projects/:id` | YES | H3 |
|
||
| 7 | `Projects/ProjectListApp.tsx:142` | inside "New Project" | `/projects` | ~ | create action |
|
||
| ~~8~~ | ~~`Projects/ProjectPreview.tsx:430`~~ | — | — | — | void: Projects deleted 2026-07-30 |
|
||
| ~~9~~ | ~~`Projects/ProjectPreview.tsx:457`~~ | — | — | — | void: Projects deleted 2026-07-30 |
|
||
| ~~10~~ | `Jobs/JobDetail.tsx` | back button | `/jobs` | YES | **done** — `<Link>`; `BackButton` was dead and is deleted |
|
||
| 11 | `ChatHistory/SessionList.tsx:70` | "New Chat" | `/chat/new` | ~ | borderline (LOW) |
|
||
| ~~12~~ | ~~`components/Combobox.tsx:54`~~ | nav combobox option | `option.href` | — | M7 — file deleted, it had no callers |
|
||
| ~~13~~ | `MusicPlayer/MusicPlayerHost.tsx:251` | open current album (setCwd+nav) | `/music` | ~ | **Done** — the side-effect was the `setCwd`; with the location in the URL the tile is a plain `<Link>` to `musicPath(albumRel)`. |
|
||
| 14 | `FileBrowserWidget/useFileBrowserWidget.ts:53` | open folder in Files | `/files?view=…` | ~ | query nav |
|
||
| 15 | `FileBrowserApp/useFileBrowserApp.ts:401` | create dashboard from folder | `/dashboards/new?…` | ~ | action-nav |
|
||
| 16 | `FileBrowserApp/useFileBrowserApp.ts:427` | create dashboard from files | `/dashboards/new?…` | ~ | action-nav |
|
||
| 17 | `FileBrowserApp/.../TaskRunnerModal.tsx:1168` | after starting job | `/jobs` `/jobs/:id` | NO | post-submit |
|
||
| 18 | `FileBrowserApp/.../TaskRunnerModal.tsx:1649` | after starting pipeline | `/jobs/:id` | NO | post-submit |
|
||
| ~~19~~ | ~~`Email/EmailScreen.tsx:29`~~ | ~~sync selection → URL (effect)~~ | — | — | deleted (H5) |
|
||
| ~~20~~ | ~~`Email/EmailScreen.tsx:31`~~ | ~~clear selection → URL (effect)~~ | — | — | deleted (H5); the mobile Back button navigates to `/email` instead |
|
||
| 21 | `ChatHistory/index.tsx:107` | redirect when no id | `/chat` | NO | guard |
|
||
| 22 | `Projects/ProjectListScreen.tsx:66` | clear/redirect | `/projects` | NO | guard |
|
||
| 23 | `Authentication/ForgotPassword/useResetPassword.ts:48` | post-reset redirect | `/` | NO | action result |
|
||
| 24 | `Authentication/LandingPage/Bootstrap.tsx:34` | post-bootstrap redirect | `/` | NO | action result |
|
||
| 25 | `hooks/useQueryState/useQueryState.ts:39` | generic query-state writer | dynamic | NO | infra hook |
|
||
|
||
---
|
||
|
||
## ✅ TO-DO — ordered for tomorrow
|
||
|
||
> **Progress — 2026-07-30, branch `navigation-refactor` (off master; NOT yet runtime-tested):**
|
||
> H1, H2, H3 implemented and tsgo-clean. **Design correction for H2/H3:** the naive "row → `<Link to="/dashboards/:id">`"
|
||
> would destroy the *preview-on-list* feature (that route is the full page). The faithful fix — which is what
|
||
> was implemented — moves selection out of the `SELECTED_*` global into a **`?selected=<id>` URL param** read by
|
||
> the list, the screen (mobile panel), and the preview; rows are real `<Link>`s (`/…?selected=id` on-page,
|
||
> `/…/:id` off-page) with the action buttons kept as **siblings** of the anchor, not nested inside it. Same
|
||
> approach will suit any future "master list + live preview" screen. **Must verify on next restart:** preview,
|
||
> create/edit/delete(/publish), and mobile-panel flows on `/dashboards` and `/projects`.
|
||
|
||
### Phase 1 — Quick wins (routes already exist; mechanical, high value)
|
||
- [x] **H1** Jobs rows → `<Link to={`/jobs/${job.id}`}>` (`JobsPage.tsx`). Done — `46482f3`. (active-row highlight already keyed off `useParams().id`.)
|
||
- [x] **H2** Dashboards rows → `<Link>`; `SELECTED_DASHBOARD_KEY` global replaced by `?selected=` URL param across `DashboardListApp`/`DashboardsScreen`/`DashboardPreview`. Done — `01365cb`. **Needs runtime test.** (The constant itself outlived its last reader by four months and has now been deleted; its siblings in `Dashboards/constants.ts` are dialog form state, not selection, and stay.)
|
||
- [x] **H3** Projects rows → `<Link>`; `SELECTED_PROJECT` global replaced by `?selected=` URL param across `ProjectListApp`/`ProjectListScreen`/`ProjectPreview`. Done — `2aaacc8`. **Needs runtime test.**
|
||
- [ ] **H4** Chat detail: read `sessionId` from `useParams`, retire `chat:selected-session` as source of truth (`ChatDetailPanel.tsx:136`) — **finishes the /chat fix**. *Deferred: overlaps the in-flight `sidecars-*` chat-comms work; do after that lands.*
|
||
- [x] **H5** Email rows → `<Link>` driven by `useParams().emailId`; the `EMAIL_SELECTED` global and both state↔URL sync effects are gone. **Needs runtime test.** (`EMAIL_FOLDER` stays a `useGlobal` for now — it is read in one component and is view state, not selection; putting the folder in `?folder=` is a separate, smaller item.)
|
||
- [x] **M8** Dead `/settings/resources` menu item removed from `UserMenu.tsx`, along with its now-orphaned `en`/`pt` locale keys. **Needs runtime test.**
|
||
- [x] Verified + converted the preview "open" navigates. Four were listed; **one** was real. The two
|
||
`ProjectPreview` lines are void — Projects was deleted end to end on 2026-07-30. Of the two in
|
||
`DashboardPreview`, the create path (`navigate` after writing the dashboard) is a genuine
|
||
post-mutation redirect and stays; only the "Open Dashboard" button in the edit form was pure
|
||
navigation, and it is now `<Button asChild><Link …>`. The big click-through overlay on the preview
|
||
was already a `<Link>`. `DashboardListApp`'s "New Dashboard" also stays a button: it sets six pieces
|
||
of form state and only *then* conditionally navigates.
|
||
|
||
### Phase 2 — Add a route, then link (per-entity, medium effort)
|
||
- [x] **M6** Settings sub-sections → `/settings/:page/:section`; `SectionButton` is now a `SectionLink` (`<NavLink>`), the five `*_SELECTED` globals and `INTEGRATIONS_SETTINGS_TAB` are gone, and each page renders one `SettingsRoute` guard that canonicalises the bare route and a bogus section. **Needs runtime test.**
|
||
- [x] **M1** Capabilities → `/tasks|skills|processes/:dirName`, rows → `<Link>`; `CapabilityPage` takes an explicit `basePath` (not reused from `endpoint`, which only happens to match). Selection is `useParams`, the mobile pane swap and back arrow are derived from it, delete navigates to the bare route, and the two per-item modes are `?edit=1` / `?new=1`. No `<Navigate>` guard: an unknown `dirName` gets the empty detail pane. **Needs runtime test.**
|
||
- [x] **M2** TaskLogs → `/task-logs/:id`; rows are `<Link>`s, `showDetail` deleted. **Needs runtime test.**
|
||
- [x] **M3** Activity → `/activity/:id`; the `{label, query}` selection object is gone — the id is the URL and the stream query is derived from the registry row. `/activity` also had no `usePageTitle` rule (it read "Officer"); added. **Needs runtime test.**
|
||
- [x] **M4** FileBrowser folders → `/files?path=`; breadcrumbs are `<Link>`s. Folder *rows* deliberately still buttons — ⌘-click is multi-select, open is double-click; converting them needs an owner call on the gesture.
|
||
- [x] **M5** CodeEditor active file → `/code-editor?file=`; tree file rows and tabs are `<Link>`s. The open-tab *set* stays local state, on purpose — see the findings row.
|
||
- [x] **M7** Combobox — **deleted instead**. Zero callers since the initial commit; `href` on `SelectOption` was never set by anything, so the whole branch was unreachable.
|
||
|
||
### Phase 3 — Whole-workspace routing decisions (needs a design call first)
|
||
- [x] **Music** — `/music?path=<rel>`; `music:cwd` deleted; every drill-in (including the dock's now-playing tile, navigate-site 13) is a `<Link>`. `music:favorites` and `music:resync` stay — a view toggle and a refresh signal. **Needs runtime test.**
|
||
- [x] **Soulseek** — `/soulseek/:section` with the peer in `?user=` and the search already in `?search=`; the two selection channels are deleted. Rooms and conversations are still `useState`. **Needs runtime test.**
|
||
- [x] **M10** SystemMonitor scope → `/system-monitor/:scope`; `monitor:scope` channel deleted. **Needs runtime test.**
|
||
- [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
|
||
- [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.
|
||
- [x] Jobs back button → `<Link to="/jobs">`, and `useNavigate` dropped from `PipelineJobDetail` (it had no
|
||
other caller). **Not** the shared `BackButton`, which turned out to be the second dead component this
|
||
audit has found: zero importers since the initial commit, no barrel entry, and a label-plus-underline
|
||
shape that fits none of the icon-only back controls in the app. Adopting it would have been a visual
|
||
redesign of the Jobs header dressed up as a navigation fix, so it is deleted instead. The two sibling
|
||
panes (`ScriptJobDetail`, `DownloadJobDetail`) were already `<Link to="/jobs">`; this was the odd one out.
|
||
- [x] Decided/skipped: Jobs step deep-link (a feature, not a fix — owner's call), Preview slug (**void**: no such app), FileBrowser widget (stays local, on M4's rule). Reasoning for each in the LOW section. (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.)
|
||
|
||
### Cross-cutting for the refactor itself
|
||
- [ ] Standardise a URL-as-source-of-truth pattern for panel selection (replace the `usePanelChannel`/`useGlobal`
|
||
selection channels in the map above with `useParams`/`useSearchParams`, keeping channels only for
|
||
genuine signals/refresh buses).
|
||
- [ ] Adopt `<NavLink>` (real react-router) for all nav chrome so active state stops being JS-derived.
|
||
- [ ] Keep `NavLink.tsx` (query-string wrapper) and `BackButton.tsx` as the standard building blocks.
|
||
|
||
---
|
||
|
||
## Coverage
|
||
|
||
- **Area 1 — Dashboard screens** (`Screens/Dashboard/**`, excl. Settings/Layout): 9 findings (2 HIGH, 4 MEDIUM, 3 LOW) + spillover to `officerdev` apps.
|
||
- **Area 2 — Workspace panel apps** (`workspaces/officerdev/src/apps/**`): 22 findings (4 HIGH, ~15 MEDIUM, 3 LOW); 11 distinct channel-selection sites.
|
||
- **Area 3 — Global nav / Layout / Settings / shared components**: 6 findings (0 HIGH — chrome is already links; 3 MEDIUM, 3 LOW) + the 25-site `navigate()` map.
|
||
|
||
Raw per-area findings (with full impl / id-in-DOM / URL-change / coverage notes) were produced in the
|
||
audit run; this document is the consolidated, de-duplicated synthesis. The three HIGH clusters
|
||
(Jobs · Dashboards · Projects) plus the two chat halves (rows done, detail = H4) are the direct siblings of
|
||
the `/chat` fix and the highest-value starting point.
|