core/FE-REWRITE-REVIEW-0.37.14.md

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

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:

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