/plans/:name, no redirect guard: the bare route is 'no plan open', which is a real state, so the auto-select-first effect is deleted rather than turned into a Navigate. The picker stays a native select — chrome for one document, not a master list — but it navigates instead of setting state. Reading the server route for this turned up a path traversal: hono percent-decodes params, so GET /api/plans/..%2F..%2Fsecret reached join(plansDir, '../../secret.md'). basename() the param. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
255 lines
29 KiB
Markdown
255 lines
29 KiB
Markdown
# Frontend Navigation Audit — the "opaque click" anti-pattern
|
|
|
|
> **Status, 2026-07-31.** H1 (jobs), H2 (dashboards) and H4's groundwork are **done** — rows are real
|
|
> `<Link>`s and selection lives in `?selected=` / `useParams`. **H3 is void: the Projects feature was
|
|
> deleted**, so every reference to `Projects/`, `SELECTED_PROJECT` and `/projects/:id` below is historical
|
|
> and should be read as an example of the pattern, not as work to do. **H4 (chat detail reading
|
|
> `sessionId` from `useParams` rather than the `chat:selected-session` channel) is the one item still
|
|
> open.** The rules in this file are current and authoritative; the findings table is a snapshot.
|
|
|
|
|
|
**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:** `Dashboards/DashboardPreview.tsx:355,377` and `Projects/ProjectPreview.tsx:430,457` also
|
|
> `navigate('/…/:id')`. The nav auditor read these as HIGH "open entity"; the apps auditor read the Project
|
|
> ones as *correct* post-create/edit flows. Eyeball them during H2/H3 — convert the ones that are "open an
|
|
> existing entity from a preview", leave genuine post-mutation redirects.
|
|
|
|
### 🟠 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 hand-rolled `isActive` is a `startsWith`, so its highlight survives the redirect. |
|
|
|
|
**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: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`.
|
|
- ~~**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:234,255`) — intra-detail sub-selection; only if we want `/jobs/:id/:stepKey` deep links.
|
|
- **Preview slug** (`Preview/PreviewApp.tsx:23`) — Preview usually derives its target from the surrounding dashboard/project `cwd`; the manual picker is a fallback. Probably leave URL-less.
|
|
- **FileBrowser widget** (`FileBrowserWidget/.../BrowseTab.tsx:36`) — compact sidebar widget; keeping its browse cursor local is defensible.
|
|
- **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:355` | open dashboard | `/dashboards/:id` | YES | verify vs H2 |
|
|
| 5 | `Dashboards/DashboardPreview.tsx:377` | open dashboard | `/dashboards/:id` | YES | verify vs H2 |
|
|
| 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` | open project | `/projects/:id` | YES | verify vs H3 |
|
|
| 9 | `Projects/ProjectPreview.tsx:457` | open project | `/projects/:id` | YES | verify vs H3 |
|
|
| 10 | `Jobs/JobDetail.tsx:709` | back button | `/jobs` | YES | use `BackButton`/`<Link>` |
|
|
| 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.**
|
|
- [ ] Verify + convert the 4 preview "open" navigates (`DashboardPreview:355,377`, `ProjectPreview:430,457`); leave genuine post-mutation redirects.
|
|
|
|
### 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
|
|
- [ ] Dock + Header + mobile sheet → react-router `<NavLink>` for active state; drop hand-rolled `isActive` (`Dock.tsx`, `Header.tsx`).
|
|
- [ ] "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.)
|
|
|
|
### 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.
|