MailUI4Agents/docs/reviews/electron-gui-review.md

### CRITICAL (security / data leak / data loss)

- **`stores/mailStore.ts` / `stores/sessionStore.ts` / `stores/contactStore.ts` are never cleared on account switch — cross-account mail disclosure.**
  `AccountSwitcher.tsx:64-67` — the entire account-switch side effect is:
  ```tsx
  const pick = async (id: string) => {
    setOpen(false);
    await setActive(id);
    await fetchInbox('all');   // ← only the inbox is refetched
  };
  ```
  `setActive` (`stores/accountStore.ts:190-201`) calls `syncAuth()` which flips the `api/config` singleton (`API_BASE` + bearer) to the new account. But `useMailStore.sent`, `useMailStore.currentMail`, `useSessionStore.sessions`, `useSessionStore.currentSession`, `useSessionStore.currentSessionMails`, `useContactStore.contacts` and `.archivedContacts` are **never reset**, and every subsequent single-account request (`api.getSent()`, `api.getMail()`, `api.getSessionDetail()`, `api.archiveContact()`) now carries the *new* account's credentials.
  Concretely: open Sent while on account A → switch to B → the Sent list still shows A's mail, and `MailView`/`ThreadView` render and forward A's bodies while the user believes they are in B. Archiving or approving from that stale view mutates B. `MailView.tsx:67` (`if (currentSession && currentSessionMails.length > 0)`) will *still* show A's whole session under account B. Nothing in `AccountSwitcher`, `accountStore`, or `App.tsx` calls `clearSession()`/`clearCurrentMail()` on switch (verified by grep: only `MailList.pick` / `ContactPanel.open` / `PermissionList.pick` do).

- **`stores/appearanceSync.ts:40` — `lastUploadedImage` is a module-level singleton never keyed by account; an account's wallpaper is treated as already-uploaded under another account.**
  ```ts
  let lastUploadedImage = '';
  ```
  It is assigned in `pull()` (`:86`) and read in `push()` (`:110`). The wallpaper upload is guarded by `if (ok && bg.kind === 'image' && bg.imageDataUrl && bg.imageDataUrl !== lastUploadedImage)`. Sequence: account A sets wallpaper X (uploaded, `lastUploadedImage = X`) → switch to account B whose server record is `saved:false` → `pull()` takes the "server has no record" branch and calls `push()` with B's *local* background cache → the guard is satisfied by A's marker, so **B's wallpaper is never uploaded to B's server record** and `status` is set to `'synced'`. The wallpaper also survives on any device that syncs B. The file's own header comment enumerates three invariants and checks the account-scoped localStorage key, but this one account-blind module variable defeats invariant 2 for image uploads specifically.

### HIGH (real bug hit in production)

- **`components/MailView.tsx:746-758` — `PermissionPanel.submit` swallows the error entirely; an approval/rejection that failed leaves the UI claiming it was processed.**
  ```ts
  } catch (err) {
    console.error(err);
  } finally {
    setBusy(false);
  }
  ```
  `setDecided(...)` runs *before* `await fetchInbox`, but a network/401/409 thrown by `api.decidePermission` skips it and lands in the `console.error` branch — no `setStaleWarning`, no error UI, and `decided` stays `''`, so the panel looks untouched and the user can re-click forever while the Agent waits. Every sibling call site in the same file (`ForwardBar.submit` :401-404, `ReplyBar.send` :1046-1051, `BudgetEditor.commit` :252) surfaces the message; this one is the odd branch and it is the only one that gates a security decision.

- **`components/MailView.tsx:390-404` and `:404-408` — `ForwardBar.submit` has a double-submit hole combined with a failed refresh leaving the panel open.**
  ```ts
  const submit = async () => {
    if (!to.trim() || busy) return;
    setBusy(true);
    ...
    await api.forwardMail(...);
    await Promise.all([fetchInbox('all'), fetchSent(), fetchSessions(), fetchContacts()]);
    onClose();
  ```
  The `busy` guard only closes the window *after* React commits `setBusy(true)`. A fast double-click (or Enter-style rapid activation) issues two `forwardMail` POSTs — the second one is a duplicate forward. Worse, if any of the four follow-up `fetch*` calls rejects, the `Promise.all` rejects into the `catch`, `onClose()` never runs, the forward *did* succeed, and the user sees an error and will press Forward again. The refreshes are the wrong thing to gate the close on.

- **`components/CalendarView.tsx:263-265` — ICS object URL revoked before the click download has consumed it.**
  ```ts
  a.click();
  URL.revokeObjectURL(url);
  ```
  `revokeObjectURL` is called synchronously immediately after `a.click()` and with no anchor appended to the document. Firefox/WebKit start the download asynchronously; revoking in the same task invalidates the blob and the export silently produces a 0-byte or failed download. Needs the `setTimeout(revoke, 0)` (or a tick later) pattern used elsewhere in the repo.

### MEDIUM (correctness or robustness gap)

- **`components/CalendarView.tsx:101-106` — stale-while-revalidate race in `load()`: a slower earlier month can overwrite a newer one.**
  ```tsx
  useEffect(() => { setLoading(true); load(); }, [range.from, range.to]);
  ```
  `load()` closes over `range` and unconditionally does `setEvents(r.events ?? [])` with no request-generation guard. Rapidly tapping 下一页/上一页 (or a swipe) issues overlapping `listCalendarEvents`; the responses can settle out of order and the grid shows a different month's events than the title says, with no error. `ThreadView.tsx:66-85` gets this right (`gen.current` + `myGen`); `CalendarView` has no equivalent.

- **`components/ComposePage.tsx:160-163` — `setTimeout` after unmount and after `cancelCompose()`.**
  ```tsx
  setTimeout(() => { setOkMsg(null); cancelCompose(); }, 900);
  ```
  No ref, no cleanup on unmount. Within the 900 ms the user can hit 取消/返回/清空/切换视图, unmounting the component; React logs an update-after-unmount on the setState and the pending timer keeps a closure over the whole compose page alive. Combined with the "清空" button (`:206-217`) which is not disabled while `sending`, this is reachable.

- **`components/AdminUsersPage.tsx:93-96` and `components/KeyPanel.tsx:37-40` — same uncleared-timeout pattern** (`flash()` and the clipboard "copied" reset), both set state on an unmounted `AdminUsersPage` / `NewKeyBanner` after navigation or account switch.

- **`components/MailView.tsx:820-846` — approval chips do not visually block while the request is in flight.**
  ```tsx
  disabled={busy}
  className={... disabled:opacity-40 ...}
  ```
  `disabled` on `<button>` does block clicks, so this is not a real double-submit — but `setBusy(true)` is only committed on the next render, and the panel keeps rendering the full chip set plus (below) the answer field, so a fast double activation within one frame can still fire two `decidePermission` calls. `PermissionPanel` has no synchronous in-flight ref; `ThreadView.loadMore` uses exactly that pattern (`busy.current`) for the same class of bug.

- **`components/BackgroundPicker.tsx:120-122` — preset thumbnails always paint `var(--bg-image)` on top of the preset gradient.**
  ```tsx
  className={`bg-preset-${p.id} absolute inset-0 block`}
  style={{ backgroundImage: 'var(--bg-image)', backgroundSize: 'cover' }}
  ```
  When a custom image background is active, `--bg-image` is set (`backgroundStore.ts:262`), so all six preset thumbnails render the *image* and none of them show their own gradient — the picker then shows six identical tiles while the user is choosing a preset. Should be omitted when `kind === 'image'`, or the thumbnail span should be omitted.

- **`components/Attachments.tsx:96-100` — attachment list built with a stale `items` closure when uploads overlap.** `handleFiles` reads `items` from the render closure and calls `onChange([...items, ...added])`; the picker is only blocked by `uploading !== null` on the *button* (`:112`), not on the `<input type="file">` (`:105-112`), and the `disabled` prop passed by `ComposePage` (`:320`) is `sending` only. Rapid re-picks can drop an earlier selection. Same shape in `ReplyBar`.

### FALSE POSITIVES YOU RULED OUT

- **No `dangerouslySetInnerHTML` / `innerHTML` / `insertAdjacentHTML` anywhere in `client/electron/src/`** (grep returned zero hits). All three Markdown render sites (`MailView.tsx:146`, `:690`, `ComposePage.tsx:305`) use `<Markdown remarkPlugins={[remarkGfm]}>` with no `rehype-raw`, so raw HTML stays escaped and `defaultUrlTransform` neutralises `javascript:`/`data:` — exactly the invariant `test/markdown-xss.test.mjs` guards. Not a finding.
- **No attacker-controlled `href`/`src` scheme.** The only dynamic `href` is `Attachments.tsx:22` `api.attachmentURL(a.attachment_id)`, built from `base()` + a server-issued `attachment_id` + `encodeURIComponent`'d token; the only dynamic `src` is `BackgroundPicker.tsx:155` with a locally produced `image:`/JPEG data URL. `withToken` (`api/config.ts:118-122`) percent-encodes the token.
- **`api/sse.ts` EventSource URL** is `withToken(\`${API_BASE}/events/stream\`)`; `API_BASE` comes from the user's own account gateway (`normalizeGateway` in `lib/accounts.ts:55-67` only prepends `http(s)://`). Self-inflicted, not remote-supplied — I deliberately did not report the plaintext-`http://` gateway default.
- **`MonthGrid`/`WeekGrid` use `key={i}`** (`CalendarView.tsx:534`, `:609`) — I checked and this is safe: the parent is `key={`${anchor.getTime()}-${scale}`}` (`:390`), so the entire grid remounts on every navigation and index identity never crosses a data change. Not a stale-row bug.
- **`ContactPanel` `ContactRow` nests two buttons** (the row `<button onClick={onOpen}>` at `:230` and the `.reveal` action buttons at `:243-258`) — valid HTML, not nested-interactive, and the actions are not descendants of the row button, so no click swallowing. Not reported.
- **`MailList` / `PermissionList` use `onClick` on `<button>` throughout** (interactive elements, correct); `useAccountStore(activeAccount)` in `AccountList.tsx:28` is a stable module-level selector, not an inline closure, so it is *not* a zustand v5 infinite-render trap.
- **`LoginPage.tsx:43-55` `setInterval` is cleared both in the effect cleanup and inside the `setCountdown` updater** — no leak.
- **`AddressInput.tsx:195-199`** removes `scroll` with matching `{ capture: true }` — listener removal is correct.
- **`backgroundStore.loadImage` (`backgroundStore.ts:379-393`)** revokes the object URL in both `onload` and `onerror` — no blob leak.
- **The `LEGACY_BACKUP_KEY` permanent residue** and the legacy→per-account migration ordering in `readStored` are explicitly documented decisions (the file says "别再问这键谁写的") — not a finding.
- **`PermissionList.tsx:57-71` `expanded ?? autoOpen` freeze** — the `pendingKey` effect deliberately re-opens newly arrived pending sessions; the comment explains why. Deliberate, not reported.
- **`App.tsx:105-109` `resetUI()` on logout** clears the *view* state only. The underlying data stores are not cleared — I am **not** reporting that as a separate leak, because with no accounts configured the `AccountSwitcher` renders `null` and the store contents are unreachable without re-selecting the same gateway, so it is a cleanliness issue rather than a fresh disclosure on its own. (The real disclosure is the account-*switch* path above.)

## Areas not reached
`components/icons.tsx`, `ModelScopePanel.tsx`, `KeyPanel.tsx` (beyond the snippet above), `QuotaPanel.tsx` (beyond line 120), `ModelScopeTab` tail, `AccountPage.tsx` lines 90-200, `Sidebar.tsx`, `NarrowOnly.tsx`, `BackButton.tsx`, `PermissionChip.tsx`, `ConnectionIndicator.tsx`, `lib/lunar.ts` (327 lines), `lib/addressSuggest.ts` body, `types/index.ts`. `client/electron/test/CRITERIA.md` was read only as far as the sections on structural criteria; I did not audit the test suite itself.