SPA + API Review: Status¶

Original review date: 2026-07-10. Last updated: 2026-07-11. This doc tracked the findings that survived the Tier 1-3 remediation; a follow-up fix pass on 2026-07-11 then cleared most of the remainder. Every fix below was validated (typecheck + lint + tests + drift check, and a browser check for UI changes) but is not yet committed - the changes sit in the working tree pending a commit.

Fix-pass summary (2026-07-11)¶

Done (10): FE-4, FE-5, FE-7, FE-8c, FE-8d, FE-8e, FE-8f, FE-8g, FE-8h, CQ-7, FE-9, API-8 (mostly), plus the CQ-6 doc and the stale-docstring cleanup.
Partial (2): API-8 (403/404/$defs fixed; 409 deferred - needs per-endpoint declaration), CQ-8 (multipart math pinned; ChatRoute/settings component suites deferred).
Skipped, needs a deliberate decision (4): API-5 (pagination - contract-breaking + UX call), API-6 (REST dialect - contract-breaking + convention call), CQ-5 (service extraction across ~15 modules - large refactor for reviewed PRs), API-10 (helper dedup on the access-control path - IDOR risk), FE-8i (upload constants from a contract - cross-cutting backend+contract change).
False alarm removed (1): FE-1 (see note below).

What has been resolved (for traceability)¶

Closed by the Tier 1-3 remediation commits (30c2208, 04e946d, b1bccc4) and the upload-limits commit (3f0f0f8):

Security - all of it: SEC-1 (admin stored XSS -> sanitize_agent_html/nh3), SEC-2 (rate limiting via declarative rate_limit=), SEC-3 (presigned PUT now signs ContentLength + bounded declaration), SEC-4 (multipart-complete access check), SEC-5 (safeHref allowlist), SEC-6 (legacy endpoints deleted).
API: API-1 (OpenAPI drift gate in CI + widened pre-commit filter), API-2 (legacy layer removed), API-3 (streaming pre-frame ApiError message surfaces), API-4 (HttpResponseBase escape hatch; document_original back in-framework), API-7 (account PATCH + rename integrity), API-9 (error taxonomy: 429/Retry-After, Allow header, HEAD, no OS-path leak).
Code quality: CQ-1 (CLAUDE.md), CQ-2 (CI drift gate), CQ-3 (connector enable-gate fails closed + logs), CQ-4 (dead layer).
Frontend: FE-2 (stream no longer follows across conversations), FE-3 (stream without done frame reported as error), FE-6 (unmount aborts the request), and FE-8(a) controllerRef race + FE-8(b) SSE malformed-frame/CRLF handling.

Partially closed items keep only their remaining piece below (API-6, CQ-5, CQ-8, FE-8).

Status table¶

#	Sev	Finding	Status
FE-4	Medium	Copy button copies raw HTML markup	✅ Done
FE-5	Medium	Deleting a filtered conversation leaves the row visible	✅ Done
FE-7	Medium	FeatureDrawer modal has no dialog semantics / focus mgmt	✅ Done
CQ-6	Medium	No `docs/project/` doc for the SPA / API framework	✅ Done
API-8	Low	`schema.py` fidelity gaps (403/404/`$defs`)	🟡 Mostly (409 deferred)
CQ-7	Low	Stray `.bak`, inert eslint rule, stale docstrings	✅ Done
FE-9	Low	Dead `zod` dep; self-contradicting API docstrings	✅ Done
FE-8c/d/e/f/g/h	Low	Robustness sub-items (retry, concurrency cap, poll cap, menu keys, retry button, drag cleanup)	✅ Done
CQ-8	Low	Frontend test coverage gaps	🟡 Partial (multipart math pinned; ChatRoute/settings deferred)
API-5	Medium	Pagination module dead; list endpoints unbounded	⏭ Skipped (needs decision)
API-6	Medium	REST delete/update dialects still split	⏭ Skipped (needs decision)
CQ-5	Medium	Domain logic in `apis/`, not services	⏭ Skipped (large refactor)
API-10	Low	Duplicated per-endpoint boilerplate	⏭ Skipped (IDOR-adjacent; review needed)
FE-8i	Low	Upload constants not from a contract	⏭ Skipped (cross-cutting)

Details for each finding (with current file:line and what was done or why deferred) are in the sections below.

FE-1 was a false alarm - removed. The original review (and two validators) claimed the Sources/Context panel never renders because ConversationPanel reads chatId via useParams() at the AppShell layout depth. Verified empirically in the browser on 2026-07-11 (localhost:8000, conversation d28a8cc4-...): the panel renders with "SOURCES 4". React Router shares one mutated params object across every match in a branch (matchRouteBranch in react-router utils: Object.assign(matchedParams, match.params) then matches.push({ params: matchedParams })), so a layout component's useParams() does see the leaf route's :chatId. The static model was wrong; no fix needed.

Medium¶

FE-4 - Copy button copies raw HTML markup - ✅ DONE¶

Fixed: added web/src/lib/render/htmlToText.ts (safe DOMParser-based HTML->plain-text with block/list breaks, unit-tested) and MessageActions.copy now copies htmlToPlainText(text) instead of the raw markup.

web/src/components/chat/MessageBubble.tsx:196 passes text={message.text} (bot HTML, rendered via <RichText html={message.text}/> at :185) to MessageActions; web/src/components/chat/MessageActions.tsx:57 writes it to the clipboard verbatim (fallback :62 too). The user pastes <p>...</p><ul><li>... instead of readable text. Affects every saved answer.

FE-5 - Deleting a filtered conversation leaves the row visible - ✅ DONE¶

Fixed: useDeleteConversation now invalidates conversationKeys.all instead of only list(""), so a delete from a filtered sidebar drops the row.

web/src/lib/api/conversations.ts:90: useDeleteConversation invalidates only conversationKeys.list(""). Keys are ["conversations","list", q]; prefix matching from list("") can't reach q !== "" keys, so deleting from a filtered sidebar leaves the dead row until an unrelated refetch. useRenameConversation (:64) and useMessageFeedback (:101) already use conversationKeys.all - delete should too.

Fixed: the drawer is now a native <dialog> opened with showModal() (focus trap + inert page + Escape via onCancel), styled as the same right slide-over, backdrop-click closes via the target check. Verified in-browser: renders correctly, Escape closes.

web/src/components/settings/FeatureSettings.tsx:406-416 is a plain backdrop div + <aside> with only aria-label. No role="dialog", aria-modal, focus trap, initial focus, or focus restore - keyboard/AT users tab into the obscured page behind it. Inconsistent with the project's own native-<dialog> Modal/ConfirmDialog pattern.

API-5 - Pagination module dead; list endpoints unbounded - ⏭ SKIPPED (needs decision)¶

Skipped in this pass: adopting Page[T] changes list response envelopes (breaking the generated frontend types and the SPA consumers) and needs a pagination UX decision (cursor vs offset, infinite-scroll UI); merely capping the unbounded lists would silently drop data. This is a product/design call, not a mechanical fix - left for a deliberate decision.

campus_core/api/pagination.py Page[T]/Cursor have zero consumers; only workspace_users_api.py:35 imports encode_cursor/decode_cursor and hand-rolls its own envelope.
Unbounded lists: conversations_list truncates silently at [:200] (conversations_api.py:201); conversation_messages (conversations_api.py:240-244), files_list (files_api.py), and workspace_folder_files_list (workspace_folders_api.py:84-100) have no limit - a 10k-file folder serializes 10k rows per request.
Fix: adopt Page[T] across list endpoints, or delete the module and standardize a cursor envelope.

API-6 (remaining) - REST delete/update dialects still split - ⏭ SKIPPED (needs decision)¶

Skipped in this pass: standardizing on one delete/update dialect renames/re-methods many endpoints, which breaks the SPA and the generated types, and there is no single mandated best practice (both DELETE /x/{id} and POST /x/delete are legitimate). This is a team convention decision plus a coordinated frontend change - left for a deliberate call.

Idempotent toggles were fixed (connector_toggle/sso_toggle now take a desired state). Still inconsistent: path-param DELETE (conversations/faqs/folders/files, e.g. conversations_api.py:338) coexists with POST .../delete|remove|revoke (connections/delete, workspace/sso/delete, workspace/domains/remove, workspace/connectors/config/delete, roles/{id}/revoke). Updates split PATCH .../update vs POST /save + /set-primary. All creates return 200; responses.created() (responses.py:31) still has zero callers.

CQ-6 - No project doc for the SPA / API framework - ✅ DONE¶

Fixed: added docs/project/spa-and-api-framework.md (SPA stack + state/routing, the @api_endpoint framework, the OpenAPI->types contract pipeline + CI drift gate, and the SPA serving model) and linked it from the docs/project/README.md index. Includes the route-param gotcha so the FE-1 confusion is documented for future readers.

CQ-5 (remaining) - Domain logic lives in `apis/` view modules, not services - ⏭ SKIPPED (large refactor)¶

Skipped in this pass: extracting domain logic into services spans ~15 workspace_* modules and is a substantial architectural refactor. CLAUDE.md itself calls for changes to be proportional and independently reviewed; doing this as a bulk autonomous sweep (without the per-change review step) is the wrong way to land it. Best done incrementally, one module per PR, with review. Left for that.

The SSO silent-drop bug was fixed. The architectural drift remains: workspace_sso_api.py:331-429 (_save, ~90 lines of domain logic), plus workspace_users_api (assign/revoke) and workspace_connectors_api (config save/toggle) still hold domain logic in apis/; there is no SSO/workspace service module. The pattern is replicated across ~15 workspace_* modules.

CQ-8 (remaining) - Frontend test coverage gaps - 🟡 PARTIAL¶

Done: the multipart part-count math (the "tricky logic worth pinning" CLAUDE.md names) is now covered - uploader.test.ts asserts one part per presigned URL with ordered/unquoted ETags and that a presign-count mismatch throws before any PUT. Remaining: routes/ChatRoute.tsx and components/settings/* component tests, which need a heavier router + QueryClient + mocked-bootstrap harness - a larger effort left as follow-up, not rushed here.

Upload hooks and a single-part uploader test were added, but the specific gaps remain untested: the multipart part-count math at web/src/lib/upload/uploader.ts:124-127, web/src/routes/ChatRoute.tsx (no test), and all of web/src/components/settings/ (0 test files, incl. SsoSettings.tsx, FeatureSettings.tsx).

Low¶

API-8 - `schema.py` fidelity gaps - 🟡 MOSTLY DONE¶

Done: 403 is now documented for staff/workspace_admin/superuser guards (not just staff); 404 is documented for path-param (object-lookup) endpoints; _query_params now uses _register for $defs, so a same-name collision raises instead of being silently dropped. Regenerated openapi.json + api.ts (no collision surfaced), 81 schema tests pass, drift check green. Deferred: documenting 409 accurately needs a per-endpoint error declaration (the generator can't infer which endpoints raise ApiConflictError) - a small decorator/registry addition left as follow-up.

403 documented only for meta.auth == "staff" (campus_core/api/schema.py:182-183); workspace_admin/superuser guards get none. 404/409 never documented despite routine ApiNotFoundError/ApiConflictError (:170-174). _query_params uses components.setdefault for $defs (:146-147), bypassing the _register collision guard. Latent shared-dict GET+POST operation issue unchanged.

API-10 - Duplicated per-endpoint boilerplate - ⏭ SKIPPED (risk vs reward)¶

Skipped in this pass: the code is already correct (pure copy-paste cleanup), and the highest-value part - consolidating the five fetch-owned-or-404 helpers - sits on the object-ownership/access-control path, where a consolidation that flattens a subtle per-case difference could open an IDOR. That belongs in a deliberate, reviewed refactor, not a bulk autonomous sweep. The safe sub-parts (one user-resolution idiom, one domain_errors) can ride along with it.

Two user re-resolution idioms coexist: await aresolve_user(request) (files, attachments, workspace_folders, conversations, account) vs await sync_to_async(lambda: request.user)() (knowledge, chat, cloud_import, connectors). Twin domain_errors() (files_api.py:67) / _domain_errors() (workspace_folders_api.py:53). Five fetch-owned-or-404 variants: _get (workspace_connectors_api.py:237), _owned (conversations_api.py:179), _folder (cloud_import_api.py:106), _connection (cloud_import_api.py:174), _owned_connection (connectors_api.py:274). All correct; all copy-paste - candidates for a shared home.

CQ-7 - Stray file, inert lint rule, stale docstrings - ✅ DONE (docstrings -> DOCS task)¶

Done: deleted apps/main_app/apis/bootstrap_api.py.bak; removed the misleading inert no-floating-promises: "off" override and replaced it with a note explaining the rule is type-aware/inert here and that enabling it means adopting the whole type-checked tier (a deliberate decision). Lint green. The stale docstrings sub-item is handled in the DOCS task at the bottom (kept there so docs land last).

FE-9 - Dead dependency and self-contradicting docstrings - ✅ DONE (docstrings -> DOCS task)¶

Done: removed the unused zod dependency (pnpm remove zod; zero imports in src/); typecheck + build green. The self-contradicting docstrings sub-item is handled in the DOCS task at the bottom.

web/package.json:19 declares zod ^4.4.3 with zero imports in src/ - dead weight.
web/src/lib/api/folders.ts:14-23, users.ts:1-13, workspace.ts:1-13 docstrings claim the shapes are hand-written and "NOT yet in schema.ts", immediately above imports of those exact models from ./schema. The migration happened; the docs contradict themselves.

FE-8 (remaining sub-items)¶

Fixed: (a) controllerRef clobber race, (b) SSE malformed-frame/CRLF handling. Remaining:

(c) ✅ DONE - uploader.ts now retries each multipart part up to PART_UPLOAD_ATTEMPTS (3) via putPartWithRetry (a part PUT is idempotent by part number; aborts never retried), and the docstring was corrected (dropped the unimplemented "resumability" claim). Tested: a transient part failure recovers and the upload completes.
(d) ✅ DONE - folder uploads now run through a runWithConcurrency pool (UPLOAD_CONCURRENCY = 4); pending rows are still created synchronously so all chips appear at once, but only 4 PUTs are in flight at a time.
(e) ✅ DONE - the status poll now gives up after MAX_POLL_TICKS (80 ticks, ~2 min): it stops the interval and marks any still-processing rows failed ("taking too long"), so a wedged attachment no longer polls forever. (Fixed-interval give-up; exponential backoff not added - low value once bounded.)
(f) ✅ DONE - OverflowMenu now moves focus into the menu on open, supports ArrowUp/Down/Home/End roving focus, Tab-closes, and Escape closes + restores focus to the trigger. (Click behavior unchanged; typecheck/lint green.)
(g) ✅ DONE - the "Cannot reach the server" screen now has a Retry button that calls refetch() (disabled/"Retrying…" while in flight), so a transient load failure is recoverable without a manual browser refresh.
(h) ✅ DONE - useResize now stashes a DOM-only teardown in a ref and runs it from an unmount effect, so a mid-drag unmount removes the mousemove/mouseup listeners and restores user-select.
(i) ⏭ SKIPPED - sourcing these from a contract needs a backend bootstrap-schema addition + OpenAPI regen + rewiring uploader.ts's pure (non-hook) functions across call sites. The single-source direction is right, but it is a cross-cutting backend+contract+frontend change for its own reviewed PR, not this sweep. (MULTIPART_THRESHOLD is fixed by S3's 5 MB part floor, so it does not need a contract.)

Validation method¶

Four parallel read-only agents (one per dimension) re-checked every original finding (SEC-1..6, API-1..10, CQ-1..8, FE-1..9) against current code on 2026-07-11, cross-referenced with the Tier 1-3 commit messages. Only findings confirmed still-open (or the still-open portion of a partial) are retained above; all fixed findings were removed.