mobile-chat audit: 9 findings (3 HIGH shipped in fix/mobile-inbox-3-high-audit-findings) #2908

New Issue

2026-06-15T05:04:41Z

agent-dev-b commented

2026-06-15 05:04:41 +00:00

Mobile-chat read-only audit findings (Kimi delegation 2ff1cdee)

Driver-authorized follow-up: the 3 HIGH findings are being shipped in the
companion PR fix/mobile-inbox-3-high-audit-findings (branch + commits
in molecule-core). This issue is the SSOT for ALL 9 findings from the
mobile-chat audit so nothing is lost; the 3 MEDIUM/LOW are deferred for
follow-up PRs.

Source audit: read-only sweep of canvas/src/components/mobile/ and
canvas/src/components/tabs/chat/hooks/, scoped to the recurring
incident area (#2765, #2766 MobileInbox stale rows; #2759 desktop echo).

Shipped in the companion PR (3 HIGH)

F1 — HIGH — backend fetch failure silently renders as "No pending approvals"

File: canvas/src/components/mobile/MobileInbox.tsx:155-162 (pre-fix)
Failure mode: silent blank state on render or stream errors.
Symptom: the load() catch block sets items=[], and the render
shows the "No pending approvals" empty copy. A backend outage is
indistinguishable from a clean inbox. Critical for destructive-approvals
review — a clean-looking inbox during a backend outage would let
destructive actions go unreviewed.
Fix: track loadError state; render a distinct role="alert"
error+retry affordance (data-testid="inbox-load-error",
inbox-retry) when the load fails.
Test: it("F1: backend fetch failure renders error/retry, not silent 'No pending approvals'", ...) — asserts the alert + retry render AND asserts the silent "No pending approvals" copy does NOT render.

F2 — HIGH — `respond` prev-closure wipes a WS-arrived row on POST failure

File: canvas/src/components/mobile/MobileInbox.tsx:81-101 (pre-fix)
Failure mode: race between optimistic update and server state.
Symptom: const prev = items; ... setItems(prev); (on catch) restores
the CAPTURED snapshot, wiping any rows that arrived via WS during the
in-flight POST. If a REQUEST_CREATED WS event fires between optimistic
removal and POST failure, the new row is silently dropped.
Fix: on POST failure, re-fetch from server truth (server is the
source of truth) instead of restoring a stale snapshot.
Test: it("F2: POST failure re-loads from server (preserves a row that arrived via WS mid-POST)", ...) — sets up an initial [A], a failing POST, and a second GET returning [A, B] (B = WS-arrived mid-POST); asserts BOTH A and B are visible after the failure re-load.

F3 — HIGH — WS REQUEST_RESPONDED race with optimistic removal flickers the row

File: canvas/src/components/mobile/MobileInbox.tsx:77-79 (pre-fix)
Failure mode: WS-triggered load() racing with optimistic removal.
Symptom: user clicks Approve → optimistic removal → POST in flight.
A REQUEST_RESPONDED WS event in this window fires load(), which
re-fetches; if the server still has the row (POST hasn't landed), the
row briefly re-renders before vanishing. The "flicker" report.
Fix: justActedRef: useRef<Set<string>> tracks rows the user has
optimistically acted on. load() filters justActedRef out of the
response. The set is cleared on POST completion (success: server no
longer returns the row; failure: catch re-loads from server truth and
re-surfaces it).
Test: it("F3: WS REQUEST_RESPONDED during optimistic removal does not flicker the row", ...) — captures the useSocketEvent callback, fires a synthetic WS event during an in-flight POST, asserts the row stays gone.

Deferred for follow-up PRs (3 MEDIUM + 3 LOW)

F4 — MEDIUM — `useChatHistory.ts:117` unbounded message buffer

Code setMessages((prev) => [...older, ...prev]) has no max cap.
Long-lived chats accumulate without bound, hurting re-render perf and
scroll jank on low-end mobile. Server-side pagination works; client just
keeps accumulating.
Suggested fix (small): MAX_MESSAGES = 500 constant + .slice(-MAX_MESSAGES) after each load. Virtualized list is the proper large fix.

F5 — MEDIUM — `MobileChat.tsx:524` history-load error banner hidden when partial history loaded

Code !historyLoading && historyError && messages.length === 0 only shows
the error when ZERO messages are present. A history reload failure is
silently swallowed if any messages exist (optimistic sends, or messages
from a prior load).
Suggested fix (small): show a non-blocking warning chip at the top
of the list when historyError && messages.length > 0, with a "Retry"
button. Don't gate on messages.length === 0.

F6 — MEDIUM — `useChatSocket.ts:214-216` silent swallow in socket handler

Code } catch { /* ignore */ } silently drops all errors in the socket
event handler. No log, no fallback, no UI signal.
Suggested fix (small): console.warn with event/payload excerpt,
and emit a one-time "stream error" toast if errors persist.

F7 — LOW — `MobileChat.tsx:367-371` composer state lost on send error

Code clears draft and pendingFiles optimistically. If sendMessage
errors (file too large, network), the draft and files vanish.
Suggested fix (small): only clear on successful dispatch; restore
both on error.

F8 — LOW — `useChatSend.ts:191-203` O(N) token cleanup scan per send completion

Linear scan of messageIdToTokenRef on every send finish. Negligible
in practice.
Suggested fix (small): maintain a reverse tokenToMessageIdRef for
O(1) lookup.

F9 — LOW — `useChatHistory.ts:97` `loadOlder` silently no-ops without containerRef

if (!container) return; — the shared hook's "load older" capability
is silently unavailable to MobileChat (which doesn't pass containerRef).
Not a mobile UI issue (no "load older" button on mobile).
Suggested fix (small): warn in dev when called without containerRef,
or skip the loadOlder field in the returned API.

Verified-clean (no findings)

Stream end/error resetting "streaming"/"sending" status —
useChatSend.ts:443-450 correctly handles isClientTimeout +
isCloudflareHeldRequest (524) by pendSendTokenForWS, not by setting
error. Real failures (non-524/timeout) correctly call finishSendToken
and set error.
"Agent not found" empty state in MobileChat.tsx:313-330 ✅
Loading/empty states in MobileChat.tsx:519-560 ✅
MobileChat.tsx:660-676 — error banner correctly suppressed during thinking ✅
MobileInbox.tsx:51-73 — loadSeqRef correctly guards stale fetch responses for kind-tab switches ✅

Linked PR

Branch: fix/mobile-inbox-3-high-audit-findings (to be opened by operator)
Head SHA: 06c3ec66adaf621489e2646edefdfbf94293af8f
Files touched: MobileInbox.tsx (+85/-5) + MobileInbox.test.tsx (+126)
Tests: 3 new + 3 existing in MobileInbox suite; 265 mobile-suite tests
- 40 chat-hooks tests all pass; lint clean.

# Mobile-chat read-only audit findings (Kimi delegation 2ff1cdee) Driver-authorized follow-up: the 3 HIGH findings are being shipped in the companion PR **`fix/mobile-inbox-3-high-audit-findings`** (branch + commits in `molecule-core`). This issue is the SSOT for ALL 9 findings from the mobile-chat audit so nothing is lost; the 3 MEDIUM/LOW are deferred for follow-up PRs. Source audit: read-only sweep of `canvas/src/components/mobile/` and `canvas/src/components/tabs/chat/hooks/`, scoped to the recurring incident area (#2765, #2766 MobileInbox stale rows; #2759 desktop echo). --- ## Shipped in the companion PR (3 HIGH) ### F1 — HIGH — backend fetch failure silently renders as "No pending approvals" - **File**: `canvas/src/components/mobile/MobileInbox.tsx:155-162` (pre-fix) - **Failure mode**: silent blank state on render or stream errors. - **Symptom**: the `load()` catch block sets `items=[]`, and the render shows the "No pending approvals" empty copy. A backend outage is indistinguishable from a clean inbox. Critical for destructive-approvals review — a clean-looking inbox during a backend outage would let destructive actions go unreviewed. - **Fix**: track `loadError` state; render a distinct `role="alert"` error+retry affordance (`data-testid="inbox-load-error"`, `inbox-retry`) when the load fails. - **Test**: `it("F1: backend fetch failure renders error/retry, not silent 'No pending approvals'", ...)` — asserts the alert + retry render AND asserts the silent "No pending approvals" copy does NOT render. ### F2 — HIGH — `respond` prev-closure wipes a WS-arrived row on POST failure - **File**: `canvas/src/components/mobile/MobileInbox.tsx:81-101` (pre-fix) - **Failure mode**: race between optimistic update and server state. - **Symptom**: `const prev = items; ... setItems(prev);` (on catch) restores the CAPTURED snapshot, wiping any rows that arrived via WS during the in-flight POST. If a `REQUEST_CREATED` WS event fires between optimistic removal and POST failure, the new row is silently dropped. - **Fix**: on POST failure, re-fetch from server truth (server is the source of truth) instead of restoring a stale snapshot. - **Test**: `it("F2: POST failure re-loads from server (preserves a row that arrived via WS mid-POST)", ...)` — sets up an initial `[A]`, a failing POST, and a second GET returning `[A, B]` (B = WS-arrived mid-POST); asserts BOTH A and B are visible after the failure re-load. ### F3 — HIGH — WS REQUEST_RESPONDED race with optimistic removal flickers the row - **File**: `canvas/src/components/mobile/MobileInbox.tsx:77-79` (pre-fix) - **Failure mode**: WS-triggered `load()` racing with optimistic removal. - **Symptom**: user clicks Approve → optimistic removal → POST in flight. A `REQUEST_RESPONDED` WS event in this window fires `load()`, which re-fetches; if the server still has the row (POST hasn't landed), the row briefly re-renders before vanishing. The "flicker" report. - **Fix**: `justActedRef: useRef<Set<string>>` tracks rows the user has optimistically acted on. `load()` filters `justActedRef` out of the response. The set is cleared on POST completion (success: server no longer returns the row; failure: catch re-loads from server truth and re-surfaces it). - **Test**: `it("F3: WS REQUEST_RESPONDED during optimistic removal does not flicker the row", ...)` — captures the `useSocketEvent` callback, fires a synthetic WS event during an in-flight POST, asserts the row stays gone. --- ## Deferred for follow-up PRs (3 MEDIUM + 3 LOW) ### F4 — MEDIUM — `useChatHistory.ts:117` unbounded message buffer - Code `setMessages((prev) => [...older, ...prev])` has no max cap. - Long-lived chats accumulate without bound, hurting re-render perf and scroll jank on low-end mobile. Server-side pagination works; client just keeps accumulating. - **Suggested fix (small)**: `MAX_MESSAGES = 500` constant + `.slice(-MAX_MESSAGES)` after each load. Virtualized list is the proper large fix. ### F5 — MEDIUM — `MobileChat.tsx:524` history-load error banner hidden when partial history loaded - Code `!historyLoading && historyError && messages.length === 0` only shows the error when ZERO messages are present. A history reload failure is silently swallowed if any messages exist (optimistic sends, or messages from a prior load). - **Suggested fix (small)**: show a non-blocking warning chip at the top of the list when `historyError && messages.length > 0`, with a "Retry" button. Don't gate on `messages.length === 0`. ### F6 — MEDIUM — `useChatSocket.ts:214-216` silent swallow in socket handler - Code `} catch { /* ignore */ }` silently drops all errors in the socket event handler. No log, no fallback, no UI signal. - **Suggested fix (small)**: `console.warn` with event/payload excerpt, and emit a one-time "stream error" toast if errors persist. ### F7 — LOW — `MobileChat.tsx:367-371` composer state lost on send error - Code clears `draft` and `pendingFiles` optimistically. If `sendMessage` errors (file too large, network), the draft and files vanish. - **Suggested fix (small)**: only clear on successful dispatch; restore both on error. ### F8 — LOW — `useChatSend.ts:191-203` O(N) token cleanup scan per send completion - Linear scan of `messageIdToTokenRef` on every send finish. Negligible in practice. - **Suggested fix (small)**: maintain a reverse `tokenToMessageIdRef` for O(1) lookup. ### F9 — LOW — `useChatHistory.ts:97` `loadOlder` silently no-ops without containerRef - `if (!container) return;` — the shared hook's "load older" capability is silently unavailable to MobileChat (which doesn't pass containerRef). Not a mobile UI issue (no "load older" button on mobile). - **Suggested fix (small)**: warn in dev when called without containerRef, or skip the loadOlder field in the returned API. --- ## Verified-clean (no findings) - Stream end/error resetting "streaming"/"sending" status — `useChatSend.ts:443-450` correctly handles `isClientTimeout` + `isCloudflareHeldRequest` (524) by `pendSendTokenForWS`, not by setting error. Real failures (non-524/timeout) correctly call `finishSendToken` and set error. - "Agent not found" empty state in `MobileChat.tsx:313-330` ✅ - Loading/empty states in `MobileChat.tsx:519-560` ✅ - `MobileChat.tsx:660-676` — error banner correctly suppressed during `thinking` ✅ - `MobileInbox.tsx:51-73` — `loadSeqRef` correctly guards stale fetch responses for kind-tab switches ✅ ## Linked PR - Branch: `fix/mobile-inbox-3-high-audit-findings` (to be opened by operator) - Head SHA: `06c3ec66adaf621489e2646edefdfbf94293af8f` - Files touched: `MobileInbox.tsx` (+85/-5) + `MobileInbox.test.tsx` (+126) - Tests: 3 new + 3 existing in MobileInbox suite; 265 mobile-suite tests + 40 chat-hooks tests all pass; lint clean.

agent-dev-b self-assigned this 2026-06-15 05:04:41 +00:00

devops-engineer referenced this issue

2026-06-15 05:08:18 +00:00

fix(mobile): 3 HIGH MobileInbox audit findings (F1/F2/F3) + tests #2909

agent-reviewer-cr2 referenced this issue

2026-06-15 05:48:47 +00:00

fix(mobile): 3 HIGH MobileInbox audit findings (F1/F2/F3) + tests #2909