# FE Rewrite Review — CS-0.37.14

**Reviewer:** Claude (Opus 4.6)
**Date:** 2026-03-23
**Scope:** Full critical review of the Preact+htm frontend rewrite (src/js/sw/) and supporting backend changes (CS-0.37.1 permission enforcement).

**Stats:** ~16,200 lines of new JS across 115 files, ~6,800 lines of CSS across 17 files. Backend: 2 new handler files, 9 route edits, 12 new integration tests.

---

## Severity Legend

- **P0** — Breaks functionality or security in a live deployment. Fix before merging.
- **P1** — Significant regression or correctness issue. Fix before tagging.
- **P2** — Design debt or missing wiring. Track for next changeset.
- **P3** — Cleanup, dead code, cosmetic. Fix opportunistically.

---

## P0 — Fix Before Merge

### 1. Pipe/filter pipeline is wired but never invoked

`sw.pipe` is fully built (pre/stream/render stages, scoping, stats, dedup detection) but no component calls `_runPre`, `_runStream`, or `_runRender`. The chat completion flow in `use-chat.js` calls `sw.api.channels.complete()` directly. `use-stream.js` parses SSE without running stream filters. Markdown rendering skips render filters entirely.

**Impact:** Extensions that register pipe filters are silently ignored. The v0.30.0 compaction-as-filter roadmap item cannot work. This is a hard regression from the old FE where the pipe was invoked during completion.

**Fix:** Wire `sw.pipe._runPre(ctx)` before the completion call in `use-chat.sendMessage()`, `sw.pipe._runStream(ctx)` in the SSE loop in `use-stream.js`, and `sw.pipe._runRender(ctx)` in the markdown rendering path. The calling conventions match the old SDK — the hooks just need to invoke them at the right points.

**Files:** `src/js/sw/components/chat-pane/use-chat.js`, `src/js/sw/components/chat-pane/use-stream.js`, `src/js/sw/components/chat-pane/markdown.js`

### 2. User menu RBAC gating is inverted

```js
if ((!authenticated || sw?.isAdmin) && current !== 'admin') {
    list.push({ label: 'Admin', ... });
}
```

The `!authenticated` branch means unauthenticated users see Admin, Team Admin, and Debug menu items. The same inverted pattern appears on all three privileged items.

**Impact:** Security-facing. Non-admin users see (and can navigate to) admin pages. The backend still enforces `RequireAdmin()` on the route, so it's defense-in-depth, but the menu exposes attack surface.

**Fix:** Change `!authenticated || sw?.isAdmin` to `sw?.isAdmin` (and similarly for Team Admin/Debug). Remove the `!authenticated` fallback — it was clearly a dev-time bypass.

**File:** `src/js/sw/shell/user-menu.js`, lines 86–100

### 3. `_unwrap` misses bare `{data:[...]}` envelopes

The REST client strips the `data` array only when pagination fields (`page`, `total`, `per_page`) are present:

```js
if (json && Array.isArray(json.data) && ('page' in json || 'total' in json || 'per_page' in json)) {
    return json.data;
}
```

Multiple backend endpoints return `{"data": [...]}` without pagination fields: notes backlinks, notes titles, notes graph, notes search results, channel KBs, and others. These pass through as `{data: [...]}` objects. **24 call sites** defensively work around this with `resp.data || resp || []`.

**Impact:** The SDK's stated contract — "List methods always return the unwrapped array" — is silently broken. Every consumer must know which endpoints include pagination and which don't. New code will get this wrong.

**Fix:** Change the condition to `json && Array.isArray(json.data)`. This matches the response envelope convention (`{"data": []}` for all lists). Single-object responses don't have a `data` array, so they pass through correctly.

**File:** `src/js/sw/sdk/rest-client.js`, `_unwrap` function

---

## P1 — Fix Before Tag

### 4. `deleteMessage` calls a phantom API

`use-chat.js` calls `sw.api.channels.deleteMessage(...)` which doesn't exist in `api-domains.js` and has no corresponding backend endpoint in the ICD. The `?.` guard makes it a no-op, but the optimistic UI remove (`setMessages(prev => prev.filter(...))`) means messages disappear from the UI until page refresh, then reappear.

**Impact:** Users think they deleted a message; they didn't. Data inconsistency.

**Fix:** Either add the backend endpoint + SDK method, or remove the optimistic UI remove and surface a "not supported" toast.

**Files:** `src/js/sw/components/chat-pane/use-chat.js` (deleteMessage), `src/js/sw/sdk/api-domains.js` (missing method)

### 5. Menu submenu positioning is wrong

The recursive `<Menu>` for submenus uses `anchor=${menuRef.current}` — the parent menu's root div. The submenu positions relative to the entire parent menu element, not the individual item that triggered it.

**Impact:** Submenus appear at the parent menu's top-left corner instead of adjacent to the hovered row.

**Fix:** Track a ref per menu item (or pass the item's DOM element as anchor) so the submenu positions relative to its trigger row.

**File:** `src/js/sw/primitives/menu.js`, submenu rendering block

### 6. ChatDashboard recent items are not clickable

The recent channel list in the empty-state dashboard renders items without an `onClick` handler. Users can see their recent chats but cannot click to navigate to them.

**Impact:** UX dead end. The dashboard shows data it doesn't act on.

**Fix:** Add `onClick=${() => onSelect(ch.id, ch.type)}` to the recent item divs.

**File:** `src/js/sw/surfaces/chat/chat-workspace.js`, `ChatDashboard` component

### 7. Version string mismatch

Three different version strings in the same changeset:
- `sw._sdk = '0.37.14'` (sdk/index.js)
- `console.log('[sw] SDK v0.37.9 ready')` (sdk/index.js)
- `VERSION` file: `0.37.14.14` (four segments, breaks `0.<major>.<minor>` convention)

**Fix:** Unify to one version string, probably `0.37.14`. Fix the console log. Decide whether the four-segment VERSION is intentional.

**Files:** `src/js/sw/sdk/index.js`, `VERSION`

### 8. New FE code has zero CI coverage

The CI syntax check globs `src/js/*.js` (top-level only), missing all 115 files under `src/js/sw/`. Additionally, `node --check` doesn't work on ES modules with `import` statements. The existing `__tests__` test against old globals, not the new Preact stack.

**Impact:** Syntax errors, import typos, or broken modules in the new code won't be caught until runtime.

**Fix:** Add a CI step that either validates ES module syntax (e.g., a lightweight bundler dry-run or a glob that covers `src/js/sw/**/*.js`) or adds basic smoke tests for the new SDK boot path.

**File:** `.gitea/workflows/ci.yaml`, `test-frontend` job

---

## P2 — Track for Next Changeset

### 9. `sw.can()` is essentially unused

Only 1 reference in all surface code (`team-admin/index.js` uses `sw.isAdmin`, not `sw.can()`). No surface gates create/edit/delete buttons with `sw.can('kb.write')`, `sw.can('channel.create')`, etc. The backend permission enforcement from CS-0.37.1 has no client-side counterpart.

The settings surface does its own policy resolution from `__PAGE_DATA__` + a separate API fallback, rather than reading `sw.auth.policies` which is already cached from `/profile/permissions`.

**Impact:** The entire client-side RBAC story — the motivating feature for this rewrite — is infrastructure without consumers. Users will see buttons they can't use; the backend returns 403s but the UI doesn't prevent the click.

**Recommendation:** A follow-up pass through all surfaces to add `sw.can()` gates on action buttons, and migrate settings to use `sw.auth.policies`.

### 10. Pipe `_runChain` has no Promise guard

The comment says "Sync-only — filters must not return Promises" but there's no guard. If a filter returns a Promise (which is truthy), it silently replaces `ctx` with the Promise object, corrupting the chain for all downstream filters.

**Fix:** Add `if (result instanceof Promise) { console.error(...); continue; }` after the filter call.

**File:** `src/js/sw/sdk/pipe.js`, `_runChain`

### 11. ChatPane handleRef `useEffect` runs every render

The `useEffect` that sets `handleRef.current = {...}` has no dependency array, rebuilding the imperative handle object on every render cycle.

**Fix:** Add `[]` or `[handleRef]` as dependency array.

**File:** `src/js/sw/components/chat-pane/index.js`, line 66

### 12. `__USER__` / `__PAGE_DATA__` injection is mostly dead

The Go template still injects `window.__USER__` and `window.__PAGE_DATA__` on every page load. The new SDK boots from API calls. Only `settings/index.js` reads `__PAGE_DATA__` (for BYOK/persona policies) and it already has an API fallback. `window.__USER__` is never read by new code.

**Recommendation:** Remove `__USER__` injection. Migrate settings to use `sw.auth.policies` (see #9), then remove `__PAGE_DATA__` injection.

### 13. Debug modal is old-world code

The debug modal in `base.html` (~50 lines of raw HTML with inline `onclick` handlers, `getElementById`, global functions like `switchDebugTab()`) is the largest remaining "no raw DOM" violation and is shared across all surfaces.

**Recommendation:** Rebuild as a Preact component in a follow-up, or at minimum flag it as intentional legacy.

---

## P3 — Cleanup

### 14. Dual toast container

`base.html` still has `<div id="toastContainer" class="toast-container"></div>` from the old world. Every surface also renders `<ToastContainer />` (Preact). The old div is orphaned dead DOM.

**Fix:** Remove the `#toastContainer` div from `base.html`.

### 15. Dead code: `esc()` in admin/users.js

`function esc(s) {...}` is defined but never called. Leftover from a template that got Preact-ified.

**Fix:** Delete it.

**File:** `src/js/sw/surfaces/admin/users.js`, line 7

### 16. `sidebar-chats.js` reaches into `#surfaceInner`

`_getScale()` reads `document.getElementById('surfaceInner')` to compensate for CSS zoom on `position: fixed` menus. Technically correct but creates an implicit coupling between a surface component and the shell's DOM structure.

**Recommendation:** Move scale detection into the SDK (e.g., `sw.shell.getScale()`) so the coupling is explicit.

### 17. Notes markdown rendering uses raw DOM

`markdown.js` uses `innerHTML`, `querySelector`, `querySelectorAll` — inherent to the marked+DOMPurify pipeline. `attachWikilinkHandlers` adds click listeners imperatively on every render without explicit cleanup (Preact's DOM replacement handles GC, but the pattern is fragile).

**Recommendation:** Document this as an intentional exception to the "no raw DOM" principle. Consider a wrapper component that manages the imperative lifecycle.

### 18. Double-unwrap defensive pattern ~~(24 instances)~~ — **RESOLVED v0.37.14.21**

~~Once P0 #3 is fixed (`_unwrap` condition change), all 24 instances of `resp.data || resp || []` become dead fallback code.~~

**Resolved:** Actually 93 instances (not 24). Fixed `_unwrap` to preserve sibling fields (`Object.keys` check), normalized all 18 non-standard Go handler envelopes to `{data: [...]}`, and simplified all frontend call sites. Also fixed 3 latent bugs (roles data lost, provider types empty, audit entries empty). See CHANGELOG v0.37.14.21.

### 19. Keyboard shortcuts in use-notes.js use DOM presence check

`if (document.querySelector('.sw-notes-pane'))` gates Ctrl+N and Ctrl+D shortcuts. This means the shortcuts fire whenever the notes pane exists anywhere in the DOM, not when it's focused. If notes and chat are ever composed on the same page, shortcuts will conflict.

**Recommendation:** Use a focus-based check or a "surface active" flag from the SDK.

---

## Positives

### Architecture

The layer separation is clean and consistently applied. Primitives have zero business logic. The SDK has zero DOM. Shell has zero API calls. Surfaces compose from both. This matches the design doc precisely. The factory pattern for all SDK modules avoids singleton coupling — every module is testable in isolation.

### SDK internals

The auth module handles token persistence with cookie sync for Go SSR, proactive refresh at 80% expiry, coalesced refresh promises (preventing thundering herd on concurrent 401s), and a clean boot sequence with offline tolerance. The event bus correctly classifies local-only events to prevent UI concerns from crossing the WebSocket. The CRUD factory in `api-domains.js` eliminates boilerplate across 18 domain namespaces.

### Performance

The `use-stream.js` hook uses rAF-batched state updates during SSE parsing. Content accumulates in refs and flushes at display frame rate, preventing re-render storms during fast token output. This is the correct pattern for streaming LLM output into a reactive UI.

### Developer experience

The crash catcher in surface templates (`window.addEventListener('error', ...)` with a visible banner) is a smart production debugging aid for a scorched-earth rewrite. The admin surface's lazy section loading with version-busted dynamic imports means sections load on demand and cache-bust on upgrade. The `sw.chatPane()` and `sw.notesPane()` imperative render helpers let extension surfaces mount chat/notes without knowing Preact internals.

### Backend (CS-0.37.1)

The permission handler correctly gives admins `auth.AllPermissions` and resolves the full permission set for non-admins. The 12 new integration tests use a deny→grant→allow pattern that doesn't depend on the Everyone group's seed data. The route audit is thorough — 8 routes fixed, 18 verified correct, and the ICD is updated.

---

## Recommended Merge Order

1. Fix P0 #1–3 (pipe wiring, user menu RBAC, `_unwrap` condition)
2. Fix P1 #4–8 (phantom API, submenu anchor, dashboard clicks, version, CI)
3. Merge and deploy to dev
4. Track P2 items for v0.37.15 or v0.38.0
5. Clean up P3 items opportunistically