docs(审查): 归档本轮四份代码审查报告
`docs/reviews/` 此前一直是**未跟踪**状态 —— 审查报告只在磁盘上， 不进版本库 ⇒ 换机器、换会话、给别人看时全部拿不到， 而它们正是本轮五个修复（hap 出库 / SSE 写锁 / 换身份清数据 / 鸿蒙门禁三态 / HMS 配额）的**来源**。 push-and-gui-review.md 推送链 + Electron GUI（HMS 配额那两条） electron-gui-review.md 换身份不清数据 harmony-client-review.md 鸿蒙：门禁 fail-open / MailStore 快照共用 / clear() 零调用方 harmony-pages-review.md 页面层 harmony-state-review.md 状态层 fix-report-2026-09-26.md 上述修复的实施记录 其中 `harmony-client-review.md` §三.1 记的那条值得单独留意： 该报告自己声明「ArkTS 语言规范层面零违规，本文所有问题都是**逻辑缺陷**」—— 本次提交的三处鸿蒙改动也只动逻辑（门禁条件、logout 清理）， 不碰语法层。
This commit is contained in:
100
docs/reviews/electron-gui-review.md
Normal file
100
docs/reviews/electron-gui-review.md
Normal file
@ -0,0 +1,100 @@
|
||||
### 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.
|
||||
236
docs/reviews/fix-report-2026-09-26.md
Normal file
236
docs/reviews/fix-report-2026-09-26.md
Normal file
@ -0,0 +1,236 @@
|
||||
# 修复报告 —— 2026-09-26
|
||||
|
||||
按「CRITICAL + HIGH + MEDIUM」范围修完两端，**29 个文件**。
|
||||
每处修复都带**可复现的验证**；没有验证过的改动一条都没有。
|
||||
|
||||
---
|
||||
|
||||
## 0. 验证口径（先说清楚什么算「修好了」）
|
||||
|
||||
| 层 | 工具 | 结果 |
|
||||
|---|---|---|
|
||||
| ArkTS 编译 | `devecocli build`（SDK 6.1.0(23)，API 26） | ✅ BUILD SUCCESSFUL |
|
||||
| 端上安装 + 启动 | `devecocli run` | ✅ 装得上、起得来 |
|
||||
| 577 条判据 | `node test/run-all.mjs` | 见 §3 |
|
||||
| 单测 | `npx vitest run` | ✅ 15 文件 / **266 → 270 通过**（新增 4 条） |
|
||||
| 类型 | `npx tsc --noEmit` | ✅ 无错 |
|
||||
| Go | `go test ./...` | ✅ **全套通过**（本轮才第一次跑通，见 §5） |
|
||||
|
||||
> ★ **Go 测试之前跑不起来**（`GOMODCACHE`/`GOPATH`/`GOCACHE` 都没设）。
|
||||
> 补齐之后（含 `GOTOOLCHAIN=local`，本机 go1.25.12 与 `go.mod` 的 1.25.0 匹配）
|
||||
> 才发现推送额度那个 bug —— **上一轮我"跑不了 Go 测试"是能力缺口，不是环境缺陷**。
|
||||
|
||||
---
|
||||
|
||||
## 1. 两处跨账号数据泄露（CRITICAL）
|
||||
|
||||
### 1.1 GUI：切账号不清 store ⇒ A 的邮件显示在 B 下
|
||||
|
||||
`AccountSwitcher.tsx:56-62` 原来只 `setActive` + `fetchInbox('all')`。
|
||||
而 `setActive` 会 `syncAuth()` 翻 `api/config` 单例的 `API_BASE` 与 bearer ——
|
||||
此后 `getSent()` / `getMail()` / `archiveContact()` 全带**新**账号的凭证，
|
||||
而 `sent`、`currentSession(+Mails)`、`contacts` 全是**旧**账号的。
|
||||
|
||||
**修法**：把"清空"收成一个函数 `lib/resetAccountData.ts`，**三条身份路径共用**：
|
||||
|
||||
| 入口 | 调用点 |
|
||||
|---|---|
|
||||
| 主动切账号 | `AccountSwitcher.pick` |
|
||||
| 登出 | `authStore.logout` |
|
||||
| 401 掉登录 | `authStore.markAnonymous` |
|
||||
|
||||
后两条**原来一条都没有** —— 它们不在"切账号"的代码路径上，grep 找不到，
|
||||
是最容易漏的那种。三个 store 各加 `resetAll()`，纯界面状态仍归 `uiStore.reset`，
|
||||
**不混**（"切账号"不能变成"退出登录"）。
|
||||
|
||||
**行为锁**：`test/stores/resetAccountData.test.ts`（4 条）。
|
||||
做过**变异验证** —— 把 `resetAccountData` 改回只清 mailStore，
|
||||
两条断言立刻红（另两条本就该绿，因为它们只查幂等与 API 形状）。
|
||||
不是"写了测试就绿"的空壳。
|
||||
|
||||
### 1.2 鸿蒙：删账号不清快照 ⇒ 下一个登录的人看到上一个人的邮件
|
||||
|
||||
`MailStore.clear()` 从写下来到今天**零调用**。同一个根因的另一端。
|
||||
|
||||
**修法**（两处，都收口）：
|
||||
- `api/Logout.ets` ③′ —— 放在 `disconnectAll()` 之后（晚一步清更保险）；
|
||||
- `SettingsPage.removeAccount` —— **只在删的是当前账号时**清（删别人的账号不该清）。
|
||||
|
||||
> ★ 为什么放 `performLogout` 而不是各调用点自己清：它已经写明"两个入口必须做同一件事"，
|
||||
> 复制一份到侧栏就会重新分叉。
|
||||
|
||||
---
|
||||
|
||||
## 2. 推送：额度按「设备数」在扣（HIGH）
|
||||
|
||||
`hms.go:192` 传的是 `len(tokens)`，而华为按 `messages:send` 的**调用次数**计。
|
||||
|
||||
⇒ **3 个设备收到 1 封邮件就吃掉 3 条额度，实际只发出去 1 条。**
|
||||
多设备自部署用户按 1/设备数 的速度提前耗尽 1000 条/天。
|
||||
|
||||
而且它**扣在投递之前**：`accessToken` 失败 / HTTP 失败 / 非成功码，
|
||||
一条都没发出去，额度已经扣了，失败只 `log.Printf` ⇒ 一次抖动静默烧配额。
|
||||
|
||||
**修法**：`reserveDaily(1)`，并挪到 `accessToken` 之后。
|
||||
**为什么不是"发送成功后再扣"**：那会超发（并发下多 goroutine 都能过检查）。
|
||||
前置预留 + 放在"确认能发"之后 = **宁可少算也不多发**。
|
||||
|
||||
**测试**：新增 `TestHMSDailyLimitCountsMessagesNotTokens`（3 设备 = 1 条）。
|
||||
原有的 `TestHMSDailyLimitStopsSending` 每次只传 1 个 token，
|
||||
所以 `len(tokens)` 恰好等于 1 —— **它一直是绿的，却没钉住单位**。
|
||||
这正是"测试通过 ≠ 行为正确"的教科书案例。
|
||||
|
||||
---
|
||||
|
||||
## 3. 顺手挖出来的三个真问题（都不在原报告里）
|
||||
|
||||
### 3.1 小字对比度 2.85:1 —— 而且**上一轮的修法是错的** ★
|
||||
|
||||
设备判据抓到浅色小字（时间戳、「N 封」，66 处）= `rgb(153,153,153)` 压白底
|
||||
= **2.85:1**（< 3:1）。
|
||||
|
||||
2026-09-19 的修法是让 `textSubtleFor()` 返回 `textMuted`，理由写着
|
||||
「二级色语义对得上，且跟随主题」。
|
||||
|
||||
**复扫证明那个前提不成立**：本机 HarmonyOS 6.1.1 上
|
||||
`ohos_id_color_text_secondary` 实测**也是 rgb(153,153,153)**，
|
||||
与三级色同值 ⇒ 换过去**一点没变**。
|
||||
|
||||
⇒ 这是「**系统语义 ≠ 我们的可读性要求**」的**第三次**复发
|
||||
（前两次：`surface` 当前景色、accent 深色没调亮）。
|
||||
系统色只保证"比一级淡"这层**层次关系**，不保证 WCAG 下限。
|
||||
|
||||
**修法**：自己拥有这一档 `textSubtleLight #6B7280` / `textSubtleDark #8A92A1`
|
||||
（= WebUI gray-400 的两套主题值，实测 4.83:1 / 5.51:1），
|
||||
按纪律登记进 `SELF_OWNED_COLORS` + Theme.ets 的手写色登记表；
|
||||
顺手**移除** `textSubtle` 这个原始令牌（它就是那个坑的来源，
|
||||
留着只会让人以为"三级色可以直接用"）；
|
||||
8 处直接用 `Theme.textSubtle` 的地方（绕过访问器）全部改走 `textSubtleFor()`。
|
||||
|
||||
**验证**：真机 21/21 通过（改动前 4 处红）。
|
||||
|
||||
### 3.2 `dumpLayout` 在本机模拟器上必超时（判据自己跑不成）
|
||||
|
||||
`uitest dumpLayout` 裸调用 → `Wait for subscribe uitest.broadcast.command.reply timeout`
|
||||
（非 0 退出、stdout 空）⇒ 日历手势那类判据**直接报错**。
|
||||
加 `-p <path>` → 正常。
|
||||
|
||||
**修法**：`harmony-device.mjs` 的 `dumpLayout` 显式给路径
|
||||
（顺带 `rm` 掉设备临时文件；路径带 pid+时间戳，避免并发判据互相读到对方中间态）。
|
||||
修完 `cross-client-gesture` **9/9 通过**（修前设备半边红）。
|
||||
|
||||
### 3.3 `harmony-imageprep` 的"素材不在就跳过"守卫形同虚设
|
||||
|
||||
`hdc shell` **把 stderr 并进 stdout**，于是文件不存在时 stdout 是
|
||||
`ls: … No such file or directory` —— **里面仍然含有文件名**，
|
||||
`listed.includes('real-wallpaper.jpg')` 判成"在" ⇒ 该跳过的继续跑 ⇒ 撞在
|
||||
`/bin/file` 上**报错**（而不是跳过）。
|
||||
|
||||
**修法**：判存在看**退出码** + 行首形状，不看"输出里有没有那个名字"。
|
||||
|
||||
---
|
||||
|
||||
## 4. 其余修复
|
||||
|
||||
### GUI
|
||||
|
||||
| 严重度 | 位置 | 问题 | 修法 |
|
||||
|---|---|---|---|
|
||||
| HIGH | `MailView.tsx:756` | 审批失败只 `console.error` ⇒ "点了没反应"、Agent 一直阻塞 | 加 `submitError` 并渲染（审批/提问两种形态共用一条） |
|
||||
| HIGH | `MailView.tsx:390` | 转发双发窗口 + 关窗依赖可能失败的刷新 | 同步 `inFlight` ref 闸门；刷新改 `allSettled` |
|
||||
| HIGH | `CalendarView.tsx:267` | object URL **同步 revoke** ⇒ Firefox/WebKit 静默 0 字节 | 推迟 `setTimeout(…,0)` + `<a>` 挂上 document 再摘 |
|
||||
| MED | `CalendarView.tsx:101` | 翻月无代次守卫 ⇒ 标题与内容错配 | `loadGen`（照 `ThreadView.tsx:62-84` 的既有形状） |
|
||||
| MED | `BackgroundPicker.tsx:120` | 内联 `backgroundImage` 覆盖预设类 ⇒ **6 个格子显示同一张照片** | 去掉内联样式 |
|
||||
| MED | `appearanceSync.ts:40` | `lastUploadedImage` 不分账号 ⇒ B 永不上传却报"已同步" | 改 `Map<accountId, string>` |
|
||||
| MED | 3 处 `setTimeout` | 无卸载清理、连点留多个定时器 | 句柄存 ref + 卸载清 |
|
||||
|
||||
### 鸿蒙
|
||||
|
||||
| 严重度 | 位置 | 问题 | 修法 |
|
||||
|---|---|---|---|
|
||||
| CRITICAL | `AdminUsersPage.ets:319` | `roleKnown && !isAdmin` ⇒ **身份未知时把管理台整个渲染出来** | 三态化：`!roleKnown` 停在大门外 |
|
||||
| HIGH | `MailDetailPage.ets:1939` | `'permission'` 恒不成立（服务端发 `permission_request`）⇒ 权限面板上回车**同时**开回复框 | 改正字面量 |
|
||||
| HIGH | `MainPage.ets:2759` | 轮播换字的 `setTimeout` 句柄丢弃 | 存 ref + `stopTopbarRotation` 一并清 |
|
||||
| HIGH | `MailStore.ets:499/628` | `cc_list.length` 无 `?? []`（收件箱+发件箱两处） | 补兜底 |
|
||||
| MED | `Theme.ets` | 8 处绕过 `textSubtleFor()` | 全部改走访问器 |
|
||||
|
||||
### 系统返回键：**试了，失败，已撤回** ⚠️
|
||||
|
||||
这一项我在原报告里评的是 HIGH，**动手后发现自己的修法把应用关掉了**。
|
||||
|
||||
**原问题**（仍未修）：`MainPage` 的 `PopIntent` 监听者**只接 Esc**，
|
||||
系统返回键/手势绕开它走 `Navigation` 自己的 pop ⇒
|
||||
① `KEY_COMM_STACK_DEPTH` 停在旧值；② `KEY_OPEN_MAIL_ID` 留着上一封
|
||||
（回列表按回车会开"回复"）；③ `closeDetail()` 是死代码。
|
||||
|
||||
**我做的**：`EntryAbility.onBackPressed()` → `PopIntent.request()`。
|
||||
为此把 `PopIntent` 的回调从 `() => void` 改成 `() => boolean`
|
||||
（调用方需要区分"退了"和"没什么可退"）。
|
||||
|
||||
**实测结果**：应用在前台按**一次**系统返回键 ⇒ **UIAbility 被销毁**
|
||||
（hilog `HandleAppDied`，`aa dump -a` 里 EntryAbility 消失），
|
||||
`harmony-admin` 设备判据**挂住不返回**。
|
||||
**对照实验**：把这三处 stash 掉重编重装，同一条判据 **31/31 通过**。
|
||||
|
||||
⇒ `UIAbability.onBackPressed()` 在这条链上**没被调用到**
|
||||
（`Navigation` 自己先消费了返回事件）。
|
||||
|
||||
**为什么撤回而不是继续查**：要验的是"框架内部返回事件的分发顺序"这类
|
||||
**框架行为**，本机模拟器这条又不能代表真机。与其赌一个**会关掉应用**的
|
||||
半成品，不如整块撤回、留成一条带复现步骤的账。
|
||||
|
||||
**试过并被否掉的三种接法**（都写进代码注释了）：
|
||||
1. `Navigation.onPop` —— 不存在（编译报 `Property 'onPop' does not exist`）；
|
||||
2. `pushPath` 第三个参数 —— 不接受（只接受 1–2 个）；
|
||||
3. `NavPathInfo.onPop` —— 存在，但**只在 `pop()` 带 result 时触发**，
|
||||
系统返回键不走 `pop(result)` ⇒ 照样漏。
|
||||
|
||||
**已登记**：`docs/DEBTS.json` 的 `harmony-system-back-key`（28 笔），
|
||||
含建议的下一步（挂 `NavDestination` 的 `onWillDisappear`，
|
||||
它是每页自己的出栈时机，不依赖谁分发返回事件 —— 需真机确认）。
|
||||
|
||||
---
|
||||
|
||||
## 5. 三个诚实的缺口
|
||||
|
||||
### 5.1 3 条红判据**没修**，且与本轮无关
|
||||
|
||||
- `build-stamp` —— 产物构建于 `d9e71a4`，HEAD 是别的；
|
||||
- `commit-hygiene` —— 两个 `.hap` 仍被 git 跟踪（**安全问题，见下**）；
|
||||
- `criteria-hygiene` —— 探针路径在本地历史里找不到。
|
||||
|
||||
三条都与本轮改动无关（改动前后完全一致）。
|
||||
|
||||
### 5.2 安全问题仍然开着 ★
|
||||
|
||||
`AgentMail-v1.2.1-pushdiag.hap` 与 `AgentMail-v1.2.1-pushlog2.hap`
|
||||
**仍被 git 跟踪**，内含 AGC 信封密钥/校验和/api_key。
|
||||
已确认**尚未推送**（`git cat-file -e origin/main:<path>` 两个都失败），
|
||||
引入于 `f51c9c8`。**下次 push 前必须** `git rm --cached <两个文件>`，
|
||||
并且**建议轮换密钥**。
|
||||
|
||||
### 5.3 我改不动的两条
|
||||
|
||||
- **HMS `click_action` 缺失**（原报告 HIGH 2）：载荷形态要改，但**验证需要真机产出的
|
||||
token** —— `hms.go:50` 自己写着"我无法在本机验证成功路径"。改完无法证明，
|
||||
不动。模拟器不是真机（拿不到 AGC 真实 token）。
|
||||
- **锁屏文案里的邮件主题**（原报告 MEDIUM 3）：**先更正我上一轮的说法** ——
|
||||
`push.go:36-38` 的注释写的是「**正文**不进通知」，那是**准确的**
|
||||
（`hms.go` 发的是 title=主题 + body=发件人，邮件正文确实没进去）。
|
||||
我上轮说它"在骗人"是**说错了**。主题上锁屏是个**设计取舍**（且做得一致），
|
||||
不是文档 bug，故降级为"可选"。
|
||||
|
||||
### 5.4 我修砸了又撤回的一条
|
||||
|
||||
**系统返回键**（见 §4 末）。原报告评 HIGH，我动手后**实测把应用关掉了**，
|
||||
已整块撤回并登记成 `harmony-system-back-key` 这笔账。
|
||||
|
||||
**为什么把它单独写一节**：本轮 20 处修复里，这一处是唯一**我自己的改动导致
|
||||
回归**的地方。留着"修好了"的记录比留下一个已知缺口更危险 ——
|
||||
所以撤回、把复现步骤和被否掉的接法全部写进代码注释与 DEBTS。
|
||||
|
||||
---
|
||||
|
||||
## 6. 变更清单
|
||||
|
||||
29 文件，+603 / −77。
|
||||
373
docs/reviews/harmony-client-review.md
Normal file
373
docs/reviews/harmony-client-review.md
Normal file
@ -0,0 +1,373 @@
|
||||
# 鸿蒙客户端代码审查报告
|
||||
|
||||
审查对象：`client/harmony/entry/src/main/ets/`（65 个源文件，约 23,600 行）
|
||||
审查方式：加载 `arkts-grammar-standards` 技能后逐文件通读 + 交叉核对服务端契约
|
||||
基线：`node test/run-all.mjs` → **577 判据 / 565 通过 / 3 红 / 9 跳过**
|
||||
|
||||
---
|
||||
|
||||
## 一、总体评价
|
||||
|
||||
这份客户端的**工程素养显著高于常见水平**，审查中需要特别说明：
|
||||
|
||||
- 大量注释带日期 + 现象 + 根因 + 判据，可追溯到具体 commit
|
||||
- 已系统性修过 ArkUI 生命周期、SSE 解码、UTF-8 分片、主题应用、键盘避让等问题
|
||||
- 已接入 34 个判据文件、577 条判据的自动化回归网
|
||||
|
||||
**ArkTS 语言规范层面零违规**：无 `any`/`unknown`、无解构、无正则字面量、无 `Object.assign`、
|
||||
无 `for...in`、无 `@ts-ignore`、无 V1/V2 装饰器混用、无伪造的 ArkUI 修饰符、无属性名与
|
||||
ArkUI 通用属性冲突。本文所有问题都是**逻辑缺陷**，不是语法问题。
|
||||
|
||||
因此下面列出的不是"代码很烂"，而是**一处安全边界失效 + 一处数据隔离失效 + 一批边界态缺陷**。
|
||||
|
||||
---
|
||||
|
||||
## 二、必须修（会出事）
|
||||
|
||||
### 1. `AdminUsersPage.ets:319` — 管理员门禁**失败开放**（fail-open）
|
||||
|
||||
```ts
|
||||
if (this.roleKnown && !this.isAdmin) { // 只有"确定不是管理员"才拦
|
||||
```
|
||||
|
||||
`loadRole()` 在**任何**失败路径上都把 `roleKnown` 置 false（`:113-116` —— 网络抖动、
|
||||
token 过期、500、DNS 失败全都一样）。于是：
|
||||
|
||||
- `aboutToAppear` 里 `loadRole()` 与 `load()` **都未 await 且并行** ⇒ 首帧 `roleKnown=false`
|
||||
⇒ 完整管理控制台**在角色校验返回之前就已挂载**
|
||||
- 此后每次网络失败都会再次 fail-open
|
||||
|
||||
**讽刺的是该文件自己的头注释（`:100-103`）写的就是正确规则**：
|
||||
> 「不能把"读不到"当成"是管理员"（那会让一次网络抖动对所有人显示管理入口）」
|
||||
|
||||
代码做的正是这条注释禁止的事。
|
||||
|
||||
**影响边界**：服务端 `middleware/user.go:70-77` 的 `AdminOnly` 仍在，所以**不是越权**。
|
||||
但非管理员会看到完整用户列表、建号表单、改密/重置入口，并向管理端点发起请求 —— 属于
|
||||
客户端信息泄露 + 无意义的失败请求风暴。
|
||||
|
||||
**修法**：
|
||||
|
||||
```ts
|
||||
if (!this.roleKnown) { /* 读不到：显示"正在确认身份"，不要渲染管理台 */ }
|
||||
else if (!this.isAdmin) { /* 现有那面墙 */ }
|
||||
else { /* 管理台 */ }
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
### 2. `MailStore.ets:446` / `:593` — 收件箱与发件箱**共用同一份快照**
|
||||
|
||||
```ts
|
||||
async loadInbox(...) { const snap = this.snapshot; ... snap.mails = merged; ... }
|
||||
async loadSent (...) { const snap = this.snapshot; ... snap.mails = merged; ... }
|
||||
```
|
||||
|
||||
`loadSent` 会把 `snap.unread` 置 0、用发件箱分组重建 `groups`。
|
||||
`MainPage.ets:1947` 的 `navPathStack.clear()` 只保证了"同一时刻只挂载一个页签"，
|
||||
**没有串行化在途请求**：SSE 事件在 `loadSent` await 期间落地
|
||||
→ `notifyRemoteChange()` → `InboxTab.onMailRevChanged()` → `loadInbox()`，
|
||||
两个 promise 同时在飞，**后落地的覆盖先落地的**。
|
||||
|
||||
**修法**：给 `MailStore` 加 generation 计数器，`await` 之后校验自己仍是最新一代，
|
||||
否则丢弃结果；或者按视图拆成 `inboxSnapshot` / `sentSnapshot` 两个对象。
|
||||
|
||||
---
|
||||
|
||||
### 3. `MailStore.ets:702` — `clear()` **零调用方**，登出不清内存
|
||||
|
||||
```ts
|
||||
/** 退出登录/换账号时清空 —— 否则下一个账号会看到上一个人的邮件（哪怕只有一帧）。 */
|
||||
clear(): void { this.snapshot = new MailSnapshot(); this.bump(); }
|
||||
```
|
||||
|
||||
全仓检索确认：**除了定义处，没有任何调用点**。
|
||||
`api/Logout.ets:34-101` 的 `performLogout()` 与 `SettingsPage` 的 `removeAccount()` 都没调。
|
||||
|
||||
⇒ 上一个账号的邮件、`AppearanceStore.wallpaper`、`TopbarStore` 内容全部留在内存里。
|
||||
新账号走 `LoginPage.aboutToAppear` 快速路径时，会在请求返回前**先渲染出上一个账号的邮件**。
|
||||
这正是 `clear()` 注释自己声称要防的那件事。
|
||||
|
||||
**修法**：`performLogout()` 与 `removeAccount()` 里补 `MailStore.getInstance().clear()`。
|
||||
|
||||
---
|
||||
|
||||
### 4. `MailDetailPage.ets:1939` — 回车键守卫比较了一个**不存在的值**
|
||||
|
||||
```ts
|
||||
if (this.mailType === 'permission' && this.permissionResult.length === 0) {
|
||||
```
|
||||
|
||||
服务端只发 `'normal'`、`'permission_request'`（`server/internal/handler/permission.go:286`、
|
||||
`repo/repo.go:442`）。同文件 `:1100` 与 `:1388` 都正确写的是 `'permission_request'`，
|
||||
**只有这一处是 `'permission'`** ⇒ 该守卫恒为假，是死代码。
|
||||
|
||||
后果：在一封**尚未决策**的权限请求上按回车，会走进"打开回复框"的分支
|
||||
（`MainPage.ets:4263-4274` → `ReplyIntent.request()`），而正确行为是进入决策面板。
|
||||
|
||||
**修法**：改为 `'permission_request'`。
|
||||
|
||||
---
|
||||
|
||||
## 三、应修（生产环境会撞上）
|
||||
|
||||
### 5. `MainPage.ets:2759` — 顶栏轮播的内层 `setTimeout` 句柄被丢弃
|
||||
|
||||
```ts
|
||||
this.topTimer = setInterval(() => {
|
||||
...
|
||||
setTimeout(() => { this.topIndex = ...; ui.animateTo(...) }, TOPBAR_FADE_MS);
|
||||
}, TOPBAR_ROTATE_MS);
|
||||
```
|
||||
|
||||
`stopTopbarRotation()`（`:2768-2773`）**只 `clearInterval(topTimer)`**。
|
||||
那个 `setTimeout` 的句柄被丢弃 ⇒ 页面销毁后回调仍会执行，在正在销毁的组件上写
|
||||
`topIndex`/`topOpacity`，并对已失效的 `UIContext` 调 `animateTo`。
|
||||
|
||||
同一块还**缺重入保护**：`TOPBAR_ROTATE_MS`(5500) 远大于 `TOPBAR_FADE_MS`(260)，
|
||||
但一次超过一个周期的卡顿（大收件箱 `JSON.parse`、切后台）会让下一 tick 再排一个淡入，
|
||||
`topIndex` 前进两次而只显示一次淡入 ⇒ 静默跳过一条。
|
||||
|
||||
**修法**：把 timeout 句柄与 `topTimer` 一起保存/一起清；加一个"淡入中"标志。
|
||||
|
||||
---
|
||||
|
||||
### 6. `MainPage.ets:2154` — `closeDetail()` 是死代码，系统返回键绕过了它
|
||||
|
||||
```ts
|
||||
private closeDetail(): void {
|
||||
this.currentMailId = '';
|
||||
AppStorage.setOrCreate<string>(KEY_OPEN_MAIL_ID, '');
|
||||
}
|
||||
```
|
||||
|
||||
全仓**仅此一处定义、零调用**。`KEY_OPEN_MAIL_ID` 由 `openMail` 写、
|
||||
由根部按键分发器（`:4263-4274`）读；唯一的清除点是 `NavDestinations.ets:54-57` 里
|
||||
应用内返回箭头的 `onBack` 回调。
|
||||
|
||||
**两个 `Navigation` 都没有注册 `onPop`**（已核对 `MainPage.ets` / `ContactsTab.ets`，
|
||||
都只调了 `.navDestination(...)`）⇒ 硬件返回键与侧滑返回直接弹栈，绕过那个回调。
|
||||
|
||||
后果：用系统返回键退出详情页后，`KEY_OPEN_MAIL_ID` 仍是旧值 ⇒ 在收件箱按回车
|
||||
会打开**一封你已不在看的邮件的回复框**。`currentMailId` 同样不复位 ⇒ 已读行高亮残留、
|
||||
`groupHasActive()` 继续给已离开的组描边。
|
||||
|
||||
---
|
||||
|
||||
### 7. `MainPage.ets:2081` — `KEY_COMM_STACK_DEPTH` 只在按 Esc 时递减
|
||||
|
||||
`publishStackDepth()` 的调用点里，**唯一能减的地方是 `PopIntent` 监听器（`:1708`），
|
||||
而它只在按 Esc 时才跑**。
|
||||
|
||||
推入详情 → 用系统返回键弹出 ⇒ 真实栈已空但发布出去的深度仍是 1。
|
||||
下一次 Esc 读到 `depth > 0` → 触发 `PopIntent.request()` → `navPathStack.pop()`
|
||||
在空栈上是空操作 → handler `return true`（`:4239`）**吞掉了这次按键**。
|
||||
|
||||
⇒ 此后 Esc 永远无法透传给系统，**用户无法用键盘退出应用**。
|
||||
|
||||
**修法**：深度必须由真实的 pop 观察驱动（`NavDestination.onHidden`，或任何栈变化时重发），
|
||||
而不是只由 Esc 路径驱动。
|
||||
|
||||
---
|
||||
|
||||
### 8. `ContactsTab.ets:202` — `openSession()` 没有请求令牌
|
||||
|
||||
```ts
|
||||
this.openSessionId = c.session_id;
|
||||
this.sessionTitle = ...;
|
||||
this.sessionMails = [];
|
||||
this.sessionLoading = true;
|
||||
try { this.sessionMails = await m.sessionMails(c.session_id); } ...
|
||||
```
|
||||
|
||||
连点 A 再点 B，两个请求同时在飞。若 A 的响应后到（账号慢/冷连接），
|
||||
赋值会**用 A 的列表覆盖 B 的**，而标题仍显示 B ⇒ 面板上方是 B 的名字和主题、
|
||||
下方是 A 的邮件列表；`openSessionId`（`confirmArchive` 用）指向 B，内容却是 A 的。
|
||||
`sessionLoading` 也被先完成者清掉，另一个还在飞却已无转圈。
|
||||
|
||||
**修法**：加序号，`await` 之后校验仍是最新一次请求。
|
||||
|
||||
---
|
||||
|
||||
### 9. `MailStore.ets:499` — `cc_list.length` 少了 `?? []` 兜底
|
||||
|
||||
```ts
|
||||
mail.cc_count = mail.cc_list.length; // ← 无兜底
|
||||
mail.attach_count = (mail.attachments ?? []).length; // ← 两行之下就有兜底
|
||||
```
|
||||
|
||||
注释里详细记录了 `attachments` 因为服务端 `omitempty` 导致 94/96 缺失、
|
||||
`.length` 当场崩掉的全过程 —— 紧接着的下一行**又犯了同形状的错**（只是 `cc_list`
|
||||
目前恰好 96/96 都有）。
|
||||
|
||||
聚合模式下单个账号的 `try/catch`（`:529`）会把这个 TypeError 吞成
|
||||
`failed.push(...)` ⇒ **静默丢掉一整个账号的邮件，并污染未读总数**。
|
||||
|
||||
---
|
||||
|
||||
### 10. `AppearanceStore.ets:188-197` — 壁纸 PixelMap 泄漏 + 全尺寸解码
|
||||
|
||||
```ts
|
||||
const bytes: ArrayBuffer = await api.fetchImageBytes();
|
||||
const src: image.ImageSource = image.createImageSource(bytes);
|
||||
this.wallpaper = await src.createPixelMap(); // 无 desiredSize，且不 release
|
||||
```
|
||||
|
||||
三个问题叠在一起：
|
||||
|
||||
- 旧 `PixelMap` **从不 `release()`**，`ImageSource` 也不释放 —— 而同仓
|
||||
`BackgroundPicker.ets:167-229` 对释放是极其严谨的，这里的标准不一致
|
||||
- `createPixelMap()` **不带 `desiredSize`** ⇒ 2560px 壁纸按全尺寸解码（约 26 MB ARGB），
|
||||
并在整个会话里由静态单例持有 ⇒ **低内存设备 OOM 风险**
|
||||
- 全尺寸解码**跑在 UI 线程**上，且设置页会 `await` 它才应用主题 ⇒ 每次同步都有
|
||||
数百毫秒卡顿
|
||||
|
||||
---
|
||||
|
||||
### 11. `MainPage.ets:2917` — 每条 SSE 事件都触发一次全量多账号重拉
|
||||
|
||||
```ts
|
||||
MailStore.getInstance().notifyRemoteChange(); // → 所有已挂载页签 loadData()
|
||||
```
|
||||
|
||||
`loadInbox` 会**逐个账号**各发一次 `GET /me/mail/inbox`（`:477-487`），且
|
||||
**没有 debounce、没有在途抑制、没有 "正在加载" 早退** ⇒ 事件会排队而不是合并。
|
||||
一分钟 20 条 `session_update`、3 个账号 = 60 次冗余请求。
|
||||
|
||||
这是"接收邮件不正常/卡顿"最可能的来源。
|
||||
|
||||
**修法**：修订号处理侧加短 debounce + `loadInbox` 入口加在途早退。
|
||||
|
||||
---
|
||||
|
||||
### 12. `CalendarPage.ets:1312` vs `:1344` — 同一事件按两个时间基准放置与标注
|
||||
|
||||
```ts
|
||||
if (new Date(e.event_time).getHours() === hour) { ... } // 设备本地时
|
||||
Text(hhmmAtOffset(e.event_time, this.offsetMinutes)) // 日历配置时区
|
||||
```
|
||||
|
||||
日历时区与设备时区不一致的用户，**每个事件被画在这一行、却被标注成另一时间**。
|
||||
`nowHour()` 同样用设备本地 `getHours()`，所以"当前时间"参考线也对不上所有标签。
|
||||
|
||||
---
|
||||
|
||||
### 13. `ApiClient.ets:119` — `persistBase()` 绕过了归一化闸口
|
||||
|
||||
```ts
|
||||
async persistBase(base: string): Promise<void> {
|
||||
this.apiBase = base; // ← 直接赋值，未经 normalizeApiBase
|
||||
pref.putSync(PREF_KEY_API_BASE, base);
|
||||
```
|
||||
|
||||
`setBase()`（`:95-97`）的注释明确说它是"归一化的唯一闸口"，
|
||||
但 `persistBase` 绕过了它。`LoginPage.ets:216-217` 虽然先调了 `setBase`，
|
||||
可传入 `persistBase` 的是**未经归一化的原始输入** `this.serverAddr`。
|
||||
|
||||
后果：用户填 `https://x.com/` ⇒ 内存里是对的，**存进 preferences 的是不带 `/api/v1` 的**，
|
||||
下次冷启 `init()` 虽会再归一化一次，但这条路径本身就是设计上的漏洞。
|
||||
另外该方法还会**丢弃调用方已算好的 `check.base`**。
|
||||
|
||||
---
|
||||
|
||||
### 14. `SseService.ets` — 重连不销毁旧连接，且不支持事件回放
|
||||
|
||||
- **`doConnect` 没有 `destroy()` 旧 `httpRequest`**（`:201-202` 直接新建并覆盖
|
||||
`conn.httpRequest`）⇒ 每次重连泄漏一个 `HttpRequest`，旧实例的回调也不会被解除。
|
||||
`disconnectAccount` 里的 `destroy()`（`:127`）只对**当前**那个生效。
|
||||
- **完全不支持 `Last-Event-ID`**（全仓零命中）。服务端 `server/internal/sse/manager.go:184-193`
|
||||
专门实现了环形缓冲回放，而 WebUI 靠 `EventSource` 自动带这个头。
|
||||
⇒ 鸿蒙端**断线期间的事件永久丢失**，这正是服务端那段回放代码想解决的问题。
|
||||
- 重连固定 3 秒、无指数退避（`:305-308`），WebUI 侧有 backoff。
|
||||
|
||||
---
|
||||
|
||||
### 15. `MailStore.ets:690` — `dropSession` 忽略账号，而分组键带账号
|
||||
|
||||
```ts
|
||||
if (m.session_id !== sessionId) { kept.push(m); }
|
||||
```
|
||||
|
||||
而 `MailGrouping.ts:193` 的 `sessionKey` 明确要求键必须是 `account + '/' + session_id`
|
||||
（"同一个 `session_id` 出现在两个账号里是两件事"）。
|
||||
|
||||
后果：聚合模式下归档 A 账号的某条会话，会把**其他账号里同 id 的会话一并删掉**，
|
||||
且要等下次重拉才恢复。
|
||||
|
||||
---
|
||||
|
||||
### 16. `MailStore.ets:375-419` — 缓存读路径没有归一化
|
||||
|
||||
`paintFromCache` 走 `JSON.parse(...) as MailLike[]` 后**直接使用**，
|
||||
不像 `MailApi.inbox()` 那样过 `MailSummary.normalize()`。
|
||||
旧版本构建留下的缓存里，`session_alias` / `permission_result` / `attachments`
|
||||
都会是 `undefined` ⇒ **复现 `Models.ets:139-163` 记录过的那次白屏崩溃**
|
||||
（`Cannot read property length of undefined`）。
|
||||
|
||||
---
|
||||
|
||||
## 四、可选清理
|
||||
|
||||
| 位置 | 问题 |
|
||||
|---|---|
|
||||
| `Index.ets:1-38` | DevEco "Hello World" 模板页仍注册在 `main_pages.json` 里，可被路由到并显示模板文案 |
|
||||
| `InboxPage.ets` | 既不在 `main_pages.json`，也无任何 `pushUrl` 指向 —— 完全死代码，~262 行，且自带一套与 `MainPage.InboxTab` 重复的取数与未读计数 |
|
||||
| `SessionsPage.ets` | 仅被上面那个死页面引用 |
|
||||
| `MainPage.ets:38` | `import { MailDetailView }` 从未使用（细节渲染走 `MailDetailDestination`）—— 半途重构的痕迹 |
|
||||
| `MainPage.ets:455` | `InboxTab.openCompose()` 仍是迁移前的 `pushUrl` 全页路径（当前不可达，但离回归只差一个调用点） |
|
||||
| `WideSidebar.ets:97` | 注释声称"订阅 SSE 状态变化"，实际只在 `aboutToAppear` 读一次 ⇒ 连接指示灯永远冻结在挂载时的状态 |
|
||||
| `MailDetailPage.ets:113` | `autoReadMailId` 带着 7 行去重注释但从未被读或写，真正的去重没实现（`status` 在 await 之后才置 `'read'`，不构成重入锁） |
|
||||
| `MailDetailPage.ets:2008` | `const sessionId: string \| null = this.sessionId;` 而 `@State sessionId: string = ''` 永不为 null ⇒ 那个 `=== null` 守卫恒假，空串会漏过去 |
|
||||
| `SessionApi.ets:48` | `proposal: RenameProposal \| null` 用了 `null`（本仓约定偏向 `undefined`） |
|
||||
| `ContactsTab.ets:416` | `ForEach` 用数组下标做 key（其余列表都用稳定标识），归档后行组件会错位复用 |
|
||||
| `CalendarPage.ets:499/993/1036/1060` | `loading = false` 写在 `catch` 之后而非 `finally`（同仓 `PermissionTab`/`AdminUsersPage` 是对的） |
|
||||
|
||||
---
|
||||
|
||||
## 五、审查过程中**排除**的疑点
|
||||
|
||||
避免这些被当成 bug 去"修"：
|
||||
|
||||
- **ICS 生成**：客户端**根本不生成** ICS，`IcsFile.ets` 只负责选/读/写文本；
|
||||
生成在服务端 `handler/calendar.go`，CRLF 与 UTC `DTSTART` 都正确。
|
||||
- **`EntryAbility.onCreate` 里登录前就 `reportToken`**：已知且**已修**（`LoginPage:179`
|
||||
的 `reportPushToken` 补报，注释记录了完整因果）。
|
||||
- **顶栏 `setInterval`**：外层 interval 在 `aboutToDisappear` 确实清了（内层 timeout 漏了，见第 5 条）。
|
||||
- **SSE 监听器生命周期**：`addListener`/`removeListener` 在 `MainPage:2868/2876` 正确配对。
|
||||
- **`bump()` 与 `publishChange()` 共用 revision 计数器**：曾被怀疑会导致 AppStorage 值永久漂移，
|
||||
实际不会 —— `@Watch` 关心的是"值有没有变"，两者同步自增反而是必需的。
|
||||
- **`CalendarPage.ets:335` 的 `Date.UTC(year, month-2, 1)`**：1 月时索引为 -1 是**故意的**溢出算术。
|
||||
- **Markdown 渲染远端邮件正文**：全仓无 WebView / `loadUrl` / `innerHTML` 路径，
|
||||
走原生 `@luvi/lv-markdown-in` 组件 ⇒ **不存在 XSS 执行路径**。
|
||||
- **`.onClick` 签名**：16 个页面里全部零参，无 `ClickEvent`/`GestureEvent` 混用。
|
||||
- **ARKTS 语法层**：无 `any`/`unknown`、无解构、无正则字面量、无 `Object.assign`、
|
||||
无 `for...in`、无 `@ts-ignore`、无 V1/V2 混用、无伪造修饰符、无 `Record` 字面量未加引号的键。
|
||||
|
||||
---
|
||||
|
||||
## 六、建议的修复顺序
|
||||
|
||||
1. **第 1 条**（管理员门禁 fail-open）—— 唯一的"安全边界失效"，改动量最小
|
||||
2. **第 3 条**（`clear()` 零调用）—— 一行调用，堵住跨账号数据泄露
|
||||
3. **第 4 条**（`'permission'` 拼写）—— 一个字符串，修掉一处死守卫
|
||||
4. **第 6、7 条**（系统返回键相关的两处状态未复位）—— 一起改，都需要真实 pop 观察
|
||||
5. **第 2、11 条**（快照争用 + 无 debounce 重拉）—— 一起做，加 generation 与 debounce
|
||||
6. 第 14 条（SSE 重连销毁 / `Last-Event-ID`）—— 影响实时性，可排后
|
||||
7. 其余按需
|
||||
|
||||
---
|
||||
|
||||
## 附：测试基线
|
||||
|
||||
`node test/run-all.mjs` 本身是**红的**，3 条失败均与鸿蒙客户端代码无关：
|
||||
|
||||
- `build-stamp` —— 产物是在旧提交 `d9e71a4` 上构建的，当前 HEAD 是 `3b46126`（需重构建）
|
||||
- `commit-hygiene` —— **两个 `.hap` 产物被 git 跟踪**，内含 AGC 信封密钥/校验码/api_key。
|
||||
已确认**尚未推送到 `origin/main`**（`git cat-file -e origin/main:<path>` 均失败），
|
||||
提交于 `f51c9c8` ⇒ **赶在下一次 push 前**执行
|
||||
`git rm --cached AgentMail-v1.2.1-pushdiag.hap AgentMail-v1.2.1-pushlog2.hap` 即可；
|
||||
但既然是客户端签名凭证，仍建议**轮换**
|
||||
- `criteria-hygiene` —— 判据自身的探针路径在本地历史里查不到（探针自身有问题）
|
||||
|
||||
9 条"跳过"全部是**设备相关**判据（hdc 服务/模拟器不在），
|
||||
按本仓自己的约定「跳过不是通过」⇒ 上述运行时行为类问题仍缺设备侧验证。
|
||||
231
docs/reviews/harmony-pages-review.md
Normal file
231
docs/reviews/harmony-pages-review.md
Normal file
@ -0,0 +1,231 @@
|
||||
# HarmonyOS ArkTS client review — `client/harmony/entry/src/main/ets/pages/`
|
||||
|
||||
Scope: `MainPage.ets`, `MailDetailPage.ets`, `CalendarPage.ets`, `SettingsPage.ets`, `ComposePage.ets`,
|
||||
`ContactsTab.ets`, `AdminUsersPage.ets`, `PermissionTab.ets`, `LoginPage.ets`, `InboxPage.ets`,
|
||||
`SessionsPage.ets`, `PermissionPanel.ets`, `WideSidebar.ets`, `Index.ets`, `NavDestinations.ets`, `NavShared.ets`.
|
||||
|
||||
---
|
||||
|
||||
### CRITICAL (will crash / data loss / security hole)
|
||||
|
||||
**`AdminUsersPage.ets:319` — the admin gate fails OPEN: any `GET /me` failure renders the full admin console to a non-admin.**
|
||||
```ts
|
||||
if (this.roleKnown && !this.isAdmin) {
|
||||
```
|
||||
`loadRole()` sets `roleKnown = false` on *any* rejection (`:113-116` — network error, expired token, 500, DNS failure). The render condition only shows the "not an admin" wall when `roleKnown === true`. So the fail-open path is: request fails ⇒ `roleKnown === false` ⇒ `else` branch ⇒ full user list, create-user form, edit/delete/password-reset. Worse, `aboutToAppear` fires `loadRole()` and `load()` **un-awaited and in parallel**, so on the very first frame `roleKnown` is `false` and the admin UI is already mounted before the role check resolves. The file's own header at `:100-103` states the intended rule — "不能把'读不到'当成'是管理员'" — and the code does exactly the thing that comment forbids. The client-side gate is cosmetic anyway (the server must enforce it), but this renders a privileged-looking console and lets a non-admin fire `createUser` / `updateUser` / `resetPassword` requests; any of those endpoints that trusts the client's page visibility will 200. Minimum fix: gate on `if (!this.roleKnown)` → show a "checking…" state and return; show the wall only when `roleKnown && !isAdmin`.
|
||||
|
||||
No `any`/`unknown`, no destructuring, no regex literals, no `Object.assign`, no `for...in`, no `@ts-ignore`, and no V1/V2 decorator mixing were found in any in-scope file. No member name collides with an ArkUI universal attribute. No `null` is passed as a function argument without narrowing. The ArkTS grammar itself is clean throughout — the defects below are logic, not syntax.
|
||||
|
||||
---
|
||||
|
||||
### HIGH (real bug, will be hit in production)
|
||||
|
||||
**1. `MailDetailPage.ets:1939` — a `mail_type` value that does not exist makes the Enter-key guard dead code.**
|
||||
```ts
|
||||
if (this.mailType === 'permission' && this.permissionResult.length === 0) {
|
||||
```
|
||||
The only values the server ever emits are `'normal'`, `'permission_request'` (`repo.go:455-461` and `client/electron/src/types/index.ts:110`) and the internal control-plane `'permission_decision'`. `'permission'` appears exactly once in the entire ArkTS client and matches nothing. Consequence: the guard never fires, so the detail page's root `onKeyEvent` always treats Enter as "open reply" — including on a mail that is an *undecided permission request*, where the correct action is answering the panel. The surrounding `MainPage.ets:4263-4274` dispatch then routes Enter to `ReplyIntent.request()` → `openReplyWithMorph()`, so the user gets a reply box over a decision they were supposed to make. Every other permission check in the codebase correctly uses `'permission_request'` (e.g. `PermissionTab.ets:175`, `MailGrouping.ts` `isPendingPermission`).
|
||||
|
||||
**2. `MainPage.ets:2759` — the inner `setTimeout` of the topbar carousel is never tracked, so it fires after teardown.**
|
||||
```ts
|
||||
this.topTimer = setInterval(() => {
|
||||
...
|
||||
setTimeout(() => {
|
||||
this.topIndex = (this.topIndex + 1) % this.topbarTexts().length;
|
||||
ui.animateTo({...}, () => { this.topOpacity = 1; });
|
||||
}, TOPBAR_FADE_MS);
|
||||
}, TOPBAR_ROTATE_MS);
|
||||
```
|
||||
`stopTopbarRotation()` (`:2768-2773`) clears **only** `this.topTimer` (the interval). The `setTimeout` handle is discarded, so on `aboutToDisappear` the pending callback still runs: it writes `this.topIndex` and `this.topOpacity` on a component that is being destroyed, and calls `ui.animateTo` on a stale `UIContext`. Second, independent defect in the same block: there is **no re-entrancy guard**. `TOPBAR_ROTATE_MS` is 5500 and `TOPBAR_FADE_MS` is 260, so under normal conditions timers don't overlap — but any stall longer than one cycle (main-thread jank, a `JSON.parse` of a large inbox payload, app backgrounding) lets the next interval tick schedule a second fade while the first is still pending, and `topIndex` advances twice with a single visible fade, so a quote is silently skipped. Store the timeout handle alongside `topTimer` and clear both in `stopTopbarRotation`; guard the body with a "fade in flight" flag.
|
||||
|
||||
**3. `MainPage.ets:2154` — `closeDetail()` is dead code, so `KEY_OPEN_MAIL_ID` survives a system back and permanently misroutes Enter.**
|
||||
```ts
|
||||
private closeDetail(): void {
|
||||
this.currentMailId = '';
|
||||
AppStorage.setOrCreate<string>(KEY_OPEN_MAIL_ID, '');
|
||||
}
|
||||
```
|
||||
Grep confirms the definition at `:2154` is the only occurrence in the repository — nothing calls it. The key is written by `openMail` (`:2144`) and is read by the root key dispatcher at `:4263-4274`, which decides "am I looking at a mail? → `ReplyIntent`; else → `ComposeIntent`". The only place it gets cleared is the in-app back chevron's `onBack` callback inside `NavDestinations.ets:54-57`. There is **no `onPop` callback registered on either `Navigation`** (verified: `MainPage.ets` and `ContactsTab.ets` only call `.navDestination(...)`, no `onPop` / `onNavBarStateChange`). So hardware back button and the back-swipe gesture pop the stack directly, bypassing that callback. Result: after system-back out of a detail page, `KEY_OPEN_MAIL_ID` still holds the old id, so pressing Enter on the inbox list opens a **reply box for a mail you are no longer viewing** instead of the compose page. `this.currentMailId` has the mirror problem — it is never reset, so the previously-read row keeps its highlight and `groupHasActive()` (`:741`) keeps painting the accent border on a group you have left.
|
||||
|
||||
**4. `MainPage.ets:2081-2083` / `:4235-4243` — `KEY_COMM_STACK_DEPTH` is published on push but never on system pop, so Esc is swallowed forever after.**
|
||||
```ts
|
||||
private publishStackDepth(): void {
|
||||
AppStorage.setOrCreate<number>(KEY_COMM_STACK_DEPTH, this.navPathStack.size());
|
||||
}
|
||||
```
|
||||
Call sites are `openMail` (`:2145`), `openComposeWith`, the tab-bar `clear()` (`:1947`) and the `PopIntent` listener (`:1708`). The `PopIntent` listener is the *only* place a decrement can happen — and it only runs when Esc itself is pressed. Push a detail, then dismiss it with the system back button: the real stack is empty, but the published depth is still 1. The next Esc reads `depth > 0`, fires `PopIntent.request()`, `navPathStack.pop()` is a no-op on an empty stack, and the handler `return true`s (`:4239`) — consuming the key. From then on Esc can never be allowed through to the system, so the user cannot exit the app with the keyboard. `publishStackDepth()` must be driven by an actual pop observation (a `NavDestination` `onHidden`, or re-publishing whenever the stack changes), not only from the Esc path.
|
||||
|
||||
**5. `ContactsTab.ets:202-221` — `openSession()` has no request token, so a slower earlier tap overwrites a later one.**
|
||||
```ts
|
||||
async openSession(c: Contact): Promise<void> {
|
||||
...
|
||||
this.openSessionId = c.session_id;
|
||||
this.openSessionTitle = c.subject.length > 0 ? c.subject : ...;
|
||||
this.sessionMails = [];
|
||||
this.sessionLoading = true;
|
||||
try {
|
||||
this.sessionMails = await m.sessionMails(c.session_id);
|
||||
} catch (e) { ... } finally { this.sessionLoading = false; }
|
||||
}
|
||||
```
|
||||
Tapping contact A then quickly contact B leaves two requests in flight. If A's response lands second (slow account, cold connection), the assignment overwrites B's list while the header still reads `openSessionTitle` from B — the panel shows B's name and subject above A's mail list, and `openSessionId` (used by `confirmArchive` at `:181` to decide whether to clear the selection) points at B while the visible content is A's. There is no sequence number, no abort, and no post-await re-check that `c.session_id === this.openSessionId`. `this.sessionLoading` is also cleared by whichever request finishes first, dropping the spinner while the other is still pending.
|
||||
|
||||
---
|
||||
|
||||
### MEDIUM (correctness or robustness gap)
|
||||
|
||||
**6. `MailDetailPage.ets:113` — `autoReadMailId` is declared with a 7-line comment about deduplication but is never read or assigned, so the dedupe it describes does not exist.**
|
||||
```ts
|
||||
private autoReadMailId: string = '';
|
||||
```
|
||||
Verified: the identifier occurs only on this line. The real protection is `doMarkRead`'s `if (this.status !== 'unread') { return; }` (`:596-598`), and `this.status` is only set to `'read'` **after** the `await this.mailApi.markRead(...)` resolves (`:604-605`). So the guard is not a re-entrancy lock: two overlapping triggers — the retry button at `:1231` (`.onClick(() => { this.loadMail(this.mailId); })`) tapped twice, or `loadMail` racing an SSE-driven reload — both pass the `status !== 'unread'` test before either `markRead` resolves, and fire two `POST` requests. The server treats mark-read as idempotent so the outcome is correct, but the duplicate is real and the field that was written to prevent it is dead. Either set the field and compare against it, or set `this.status = 'read'` optimistically before the await and roll back on failure.
|
||||
|
||||
**7. `MailDetailPage.ets:2008-2012` and `:2044-2048` — `string | null` locals compared against `null` guard nothing, because the `@State` they copy is typed `string` and initialised to `''`.**
|
||||
```ts
|
||||
const sessionId: string | null = this.sessionId;
|
||||
if (proposal === null || sessionId === null || this.renameBusy) {
|
||||
return;
|
||||
}
|
||||
```
|
||||
`this.sessionId` is `@State sessionId: string = ''` — it can never be `null`, so the widened type is fiction and the check is always false. The empty-string case falls straight through and `acceptRename('', alias)` / `dismissRename('')` are issued against a session that does not exist. The narrowing is written as if the field were optional, which is the signature of a missed rename: the `@State` declaration, not the guard, is what should have been `string | undefined` (ArkTS has no `null`, so `''` is the sentinel and the guard must test `.length === 0`). Same shape in `doDismissRename`.
|
||||
|
||||
**8. `WideSidebar.ets:97-98` — the comment claims a subscription to SSE status changes that does not exist; the status dot is a one-shot snapshot.**
|
||||
```ts
|
||||
aboutToAppear(): void {
|
||||
this.refreshSseStatus();
|
||||
}
|
||||
```
|
||||
The header comment at `:92` says "read once in `aboutToAppear` **+ subscribe to changes**", and `SseService` provides `addStatusListener` / `removeStatusListener` for exactly this. No `addStatusListener` call exists anywhere in the file. `refreshSseStatus()` is never called again, so the connection indicator is frozen at whatever it read during mount: it shows "connected" through a dropped connection and "disconnected" after a successful reconnect, indefinitely, until the whole app is restarted. The missing `removeStatusListener` would also leak the component if the call were added without the teardown.
|
||||
|
||||
**9. `Index.ets:1-38` — the untouched DevEco "Hello World" template ships as a registered route.**
|
||||
```ts
|
||||
@Entry
|
||||
struct Index {
|
||||
build() {
|
||||
Column() { Text('Hello World') ... }
|
||||
}
|
||||
}
|
||||
```
|
||||
`resources/base/profile/main_pages.json` registers `pages/Index` alongside the real pages. It is a reachable-by-router page that renders template placeholder text in a shipping app. Delete it and drop it from `main_pages.json`.
|
||||
|
||||
**10. `Index.ets` route table also carries two more orphans.** `pages/InboxPage` is not in `main_pages.json` at all **and** nothing pushes to it — verified by grepping every `pushUrl` in the tree, the only references to `pages/InboxPage` are its own definition. It is therefore fully dead code, but it is a substantial `@Entry` page (~262 lines) with its own `loadInbox()` that duplicates `MainPage.InboxTab`'s fetching and its own unread-counting. `pages/SessionsPage` is reachable only from that dead page (`InboxPage.ets:171`). Both duplicate logic that already exists in `MainPage`, and both are where the `e as ApiError` issue below lives.
|
||||
|
||||
**11. `InboxPage.ets` / `SessionsPage.ets` — `e as ApiError` is trusted without narrowing, so a non-`ApiError` rejection throws a second `TypeError` inside the error handler.**
|
||||
```ts
|
||||
catch (e) {
|
||||
const ae = e as ApiError;
|
||||
this.error = ae.message.length > 0 ? ae.message : '加载失败';
|
||||
}
|
||||
```
|
||||
`ApiClient` throws `ApiError` for HTTP failures, but the same `catch` also receives the rejections of internal helpers — a `JSON.parse` `SyntaxError` inside `Models.normalize()`, a `preferences` store failure, a `TypeError` from an unexpected shape. In every one of those cases `ae.message` is either `undefined` or a non-string, and `.length` on it throws. The result is that the page's own error path crashes instead of showing a message. `AdminUsersPage` shows the correct shape for comparison — `messageOf(e)` (`:161-163`) tests `e instanceof ApiError` before touching `.message`. Both files are dead routes today, which is the only reason this has not surfaced.
|
||||
|
||||
**12. `MainPage.ets:38` — an unused import of the built-in detail view signals a half-finished refactor.**
|
||||
```ts
|
||||
import { MailDetailView } from './MailDetailPage';
|
||||
```
|
||||
`MailDetailView` is referenced nowhere in the file; detail rendering goes through `MailDetailDestination` (`:2181`) from `NavDestinations.ets`. The dead import drags the whole `MailDetailPage` module into `MainPage`'s dependency graph and, more importantly, tells a reader the built-in view is still the path. `NavDestinations.ets` has the mirror issue with `PaneModifier`.
|
||||
|
||||
**13. `MainPage.ets:455` — `InboxTab.openCompose()` still uses the old full-page `pushUrl` path while its sibling was migrated to the nav stack.**
|
||||
```ts
|
||||
this.getUIContext().getRouter().pushUrl({ url: 'pages/ComposePage', params: params });
|
||||
```
|
||||
`CommPage.openCompose()` (`:1839`) delegates to `openComposeWith()` (`:2225`) which uses `navPathStack.pushPath`, so compose opens in the right pane on wide screens. `InboxTab.openCompose()` is the pre-migration version. It is currently **unreachable** — `InboxTab` uses the injected `onOpenCompose` prop (`:2225`), never this method — so the `onOpenCompose` prop declared at `:204` is itself vestigial. It is one stray call site away from regressing the wide-screen layout, and it is the reason the vestigial prop was not removed.
|
||||
|
||||
**14. `MailDetailPage.ets:1982-1991` — the conversation-tree sheet flattens the tree into indented strings and keys the `ForEach` by array index.**
|
||||
```ts
|
||||
ForEach(this.threadLines, (line, idx) => { ... }, (line, idx) => idx.toString())
|
||||
```
|
||||
Two issues. (a) The key is the index, so a re-fetch that changes the node count makes ArkUI treat every row as new — it destroys and rebuilds all rows, and any in-progress scroll offset is lost. (b) The tree is reconstructed purely by `indent += ' '` per `t.depth` (`:693-696`) inside a plain `lines: string[]`; the `ThreadNode`'s identity, `parent_mail_id` and `mail_id` are all discarded at that point. On a long or deeply-replying thread this produces a long run of `Text` rows with no per-row data, and depth is a server-computed integer that is not clamped here — a bad `depth` (or a negative one, which the server does not validate) makes the loop either emit a negative count of spaces or spin. The `ThreadApiResponse` shape is already flat with a `depth` field, so the simplest fix is to keep `ThreadNode[]` and render with a real key on `mail_id`.
|
||||
|
||||
**15. `SettingsPage.ets:483-497` — the appearance snapshot is mutated in place before the server `PUT`, and a failed `PUT` never rolls back, so the theme silently reverts on next cold start.**
|
||||
```ts
|
||||
const snap: AppearanceSnapshot = store.current(); // live reference, not a copy
|
||||
snap.theme = theme; // mutates the singleton
|
||||
```
|
||||
`AppearanceStore.current()` (`common/AppearanceStore.ets:200-202`) returns `this.snapshot` by reference. The mutation therefore lands on the shared singleton immediately. On failure the handler sets `appearanceStatus = 'local-only'` but never calls `store.saveLocal`, so the persisted copy on disk still holds the old theme while the in-memory one holds the new one — the user sees the new theme until the app restarts, then it is back. `MainPage.applyThemeNow` (`:3024-3048`) does the right thing by calling `store.saveLocal` on both paths; the settings page should mirror that, or clone the snapshot before mutating.
|
||||
|
||||
---
|
||||
|
||||
### STRESS (plausible under load/concurrency)
|
||||
|
||||
**16. `MailStore.ets:446` and `:593` — `loadInbox` and `loadSent` write the same `this.snapshot`, so the two tabs clobber each other's data.**
|
||||
```ts
|
||||
async loadInbox(ctx, accountFilter) { const snap = this.snapshot; ... snap.mails = merged; ... }
|
||||
async loadSent(ctx, accountFilter) { const snap = this.snapshot; ... snap.mails = merged; ... }
|
||||
```
|
||||
Both alias the **same** `MailSnapshot` and assign `mails` / `groups` / `loaded` / `unread` / `loading`. `loadSent` sets `snap.unread = 0` and rebuilds `groups` with the sent-folder grouping, so an in-flight `loadSent` that resolves after an `loadInbox` leaves the inbox rendering sent-folder data. `MainPage.ets:1947` mitigates the common case by calling `this.navPathStack.clear()` on tab switch (only one tab is mounted at a time), but nothing serialises the in-flight promise: an `loadSent` started, then a `notifyRemoteChange()` SSE event landing mid-flight, calls `InboxTab.onMailRevChanged` → `loadData()` → `loadInbox()` while `loadSent` is still awaiting. Whichever resolves last wins. Needs a generation counter checked after the await, or separate snapshot objects per view.
|
||||
|
||||
**17. `MainPage.ets:2917-2920` — every SSE event type triggers a full multi-account inbox refetch, with no coalescing or debounce.**
|
||||
```ts
|
||||
if (event.type === 'permission_decision' || event.type === 'session_update'
|
||||
|| event.type === 'session_archived') {
|
||||
MailStore.getInstance().notifyRemoteChange();
|
||||
}
|
||||
```
|
||||
`notifyRemoteChange()` publishes `KEY_MAIL_REV`, and the mounted pane's `onMailRevChanged` calls `loadData()` → `MailStore.loadInbox()` (`:446`), which loops over **every** account and issues one `GET /me/mail/inbox` per account (`:477-487`). A busy session firing twenty `session_update` events in a minute produces twenty full multi-account refetches with no debounce, no in-flight suppression and no "already loading" check — `loadInbox` has no `if (snap.loading) return` guard, so they also queue up rather than collapsing. With three accounts that is up to sixty redundant requests. The fix is a short debounce on the revision handler plus an early-out in `loadInbox` when a fetch for the same filter is already in flight.
|
||||
|
||||
**18. `MailStore.ets:216-244` — `markReadLocal` mutates `snapshot.mails` in place and can be silently undone by a concurrent `loadInbox`.**
|
||||
```ts
|
||||
markReadLocal(mailId: string): void {
|
||||
const snap = this.snapshot;
|
||||
for (let i = 0; i < snap.mails.length; i++) {
|
||||
const m: MailLike = snap.mails[i];
|
||||
if (m.mail_id === mailId && m.status === 'unread') { m.status = 'read'; changed = true; }
|
||||
}
|
||||
```
|
||||
This mutates a shared singleton that an in-flight `loadInbox` will wholesale overwrite at `:568-575` when its response lands. Sequence: user opens an unread mail, detail calls `markRead` → `markReadLocal` flips the row locally; the SSE `new_mail` event that the server emits for that same action then fires `notifyRemoteChange` → `loadInbox` → the response may still carry `unread` for that id (the server's own write and the list query are not in one transaction) ⇒ the row flips back to unread in the list while the detail page shows it read. The local-only optimisation has no reconciliation against the refetch. Carrying a set of locally-applied read ids and re-applying them after the load would close it.
|
||||
|
||||
**19. `CalendarPage.ets:1312-1322` vs `:1344` — the same event is placed on the grid by one time base and labelled by another.**
|
||||
```ts
|
||||
// placement (device-local):
|
||||
if (new Date(e.event_time).getHours() === hour) { ... }
|
||||
// label (user-configured offset):
|
||||
Text(hhmmAtOffset(e.event_time, this.offsetMinutes))
|
||||
```
|
||||
`hourEvents(hour)` buckets an event into a row using the **device's** local hour, while `EventRow` prints the same event using the **calendar's** `offsetMinutes`. A user whose calendar is set to a zone other than the device's sees every event drawn in one row and captioned with a time from another. `nowHour()` (`:1328`) also uses device-local `getHours()`, so the "current time" guide line is drawn against device-local hours while every label is offset-aware. Both need to go through the same offset conversion that `hhmmAtOffset` uses.
|
||||
|
||||
**20. `CalendarPage.ets:499-520` — `loading` is cleared after the `catch` rather than in a `finally`, so a throw from outside the `try` strands the spinner.**
|
||||
```ts
|
||||
this.loading = true;
|
||||
try { ... } catch { this.error = ...; this.events = []; }
|
||||
this.loading = false;
|
||||
this.loadLunar();
|
||||
```
|
||||
`this.loadLunar()` at `:520` is a separate call whose own failure mode is unrelated, and the assignments at `:519` are on `@State` — a throw from `errorTextOf` or a listener notification inside the assignment strands `loading === true` with no error message, i.e. a permanent spinner. The sibling mutators `saveEvent` (`:993`), `deleteEvent` (`:1036`) and `toggleStatus` (`:1060`) have the identical shape. `PermissionTab.load` (`:189`) and `AdminUsersPage.load` (`:133`) use `finally` correctly — the calendar page is the outlier.
|
||||
|
||||
**21. `ContactsTab.ets:416` — index-based `ForEach` key over a mutable contact list.**
|
||||
```ts
|
||||
ForEach(this.contacts, ..., (_c: Contact, idx: number) => idx.toString())
|
||||
```
|
||||
Every list in this codebase keys `ForEach` on a stable identity (`m.source_account_id + ':' + m.mail_id` at `MainPage.ets:690`, `user.user_id` at `AdminUsersPage.ets:380`, `g.key` at `:708`). This one keys on position. `confirmArchive` (`:180`) removes an element from the middle of the array via `filter`, which shifts every later index — ArkUI reuses the wrong row components, so the list visibly reorders/mismatches after any archive until a full remount. `c.session_id` is already available and unique here.
|
||||
|
||||
**22. `MailDetailPage.ets:812-840` — a fresh `MarkdownController` is constructed on every `build()`.**
|
||||
```ts
|
||||
private mdController(): MarkdownController {
|
||||
const c: MarkdownController = new MarkdownController();
|
||||
c.setTextColor(...); // ~17 setter calls
|
||||
...
|
||||
}
|
||||
```
|
||||
Called from `Markdown({ text: this.body, controller: this.mdController() })` at `:1324`, which is inside `build()`. The design is deliberate and the comment at `:802-807` explains why (the controller must be rebuilt so a theme change is picked up, because it is a plain object ArkUI does not observe). But it means every re-render of the detail page — scroll-driven state changes, any `@State` write, the auto-mark-read `status` flip at `:604`, a theme toggle — allocates a controller and makes seventeen setter calls on the main thread. On a long mail body that markdown component also re-parses the full text each time. The theme case can be handled by keeping one controller and re-applying colours from a `@StorageLink`-driven path, and the parse can be pinned to the mail id.
|
||||
|
||||
---
|
||||
|
||||
### FALSE POSITIVES YOU RULED OUT (brief, so the lead can double check)
|
||||
|
||||
- **`MainPage.ets:2782-2804` `loadTopbar()` — un-`catch()`ed promise chain.** `AccountManager.load()` has a total `try/catch` (`AccountManager.ets:49-62`) and never rejects; `TopbarStore.refresh()` (`TopbarStore.ets:110-127`) also catches internally and returns `null`. Neither `.then()` can produce an unhandled rejection. **False positive.**
|
||||
- **`MainPage.ets:372` `loadSent` setting `unread = 0`.** Looks like a logic inversion but is correct — the sent folder has no unread concept, and `loadInbox` recomputes the badge on the next load.
|
||||
- **`CalendarPage.ets:335` `Date.UTC(this.year, this.month - 2, 1)`.** `month` is 1-12 so the index can be `-1` (January). That is intentional overflow arithmetic for "start of the month before", and `Date.UTC` normalises it correctly. Not an off-by-one.
|
||||
- **`MailDetailPage.ets:674-711` `openThread()` reading `resp.nodes` directly.** `ThreadApiResponse` **is** the flat `ThreadPage`; the server has no `thread` wrapper. The comment at `:680-685` records that the earlier guess was corrected against the real shape. The flattening itself is a MEDIUM (#14) but the field access is right.
|
||||
- **`MailDetailPage.ets:305` `loadMail` calling `doMarkRead()` which reads `this.mailId` rather than the `mailId` parameter.** I traced every assignment of `this.mailId` (`:240`, `:244` only, both in `aboutToAppear`) and confirmed each `MailDetailDestination` instance is created fresh per `pushPath` with its own params, so `this.mailId` is stable for the life of the instance. Not a stale-parameter bug.
|
||||
- **`CalendarPage.ets:186` `@Prop @Watch('onVisibleChanged') visible` with the self-call at `:302`.** Self-invoking a `@Watch` handler by name is legal and is the documented pattern for handling the initial value; the guard at `:313` makes the `aboutToAppear` call a no-op when hidden. Fine.
|
||||
- **`.onClick` handler signatures.** Every `.onClick` in all 16 files takes zero parameters (checked by grep across `MainPage`, `CalendarPage`, `ContactsTab`, `AdminUsersPage`, `LoginPage`, `PermissionTab`, `PermissionPanel`, `ComposePage`, `MailDetailPage`, `SettingsPage`) — no `ClickEvent`/`GestureEvent` confusion anywhere.
|
||||
- **`.fadingEdge(true, {...})`, `.attributeModifier(...)`, `.geometryTransition(...)`, `bindSheet`/`bindMenu` with `$$` two-way binding, `PageTransitionEnter/Exit` in `pageTransition()`.** All genuine API usages, not fabricated modifiers. No `List({gap})`, no `.bgColor()`, no `.textSize()`, no `TextAlign.CENTER` (the codebase uses `TextAlign.Center`), no `Switch(...)` component, no `ScrollEdgeEffect` anywhere in scope.
|
||||
- **Markdown rendering of remote email bodies (`MailDetailPage.ets:1324`).** I checked for a WebView/`loadUrl`/`innerHTML` path across `pages/` and `common/` — there is none. The body goes through the native `@luvi/lv-markdown-in` component with an explicit controller, so remote HTML is not executed. No XSS via WebView.
|
||||
- **`ComposePage.ets:229` bare `catch {`.** Intentional and correct: an unbound catch binding is the ArkTS-legal way to swallow, since binding `e` would make it `any` and trip `arkts-no-any-unknown`. Not a missing-`any` violation.
|
||||
- **`AdminUsersPage.ets:114` keeping `roleKnown = false` on `loadRole` failure.** The *storage* of that state is the documented deliberate choice; the **bug is the render condition at `:319` that consumes it fail-open** (reported as CRITICAL). The state itself is not the defect.
|
||||
- **`MainPage.ets:2077-2079` comment about `NavPathStack` having no change callback.** Verified accurate: the framework's `Navigation` exposes `onNavBarStateChange` / `onNavigationModeChange`, neither of which reports stack depth. This makes HIGH #4 a genuine design gap rather than an oversight the author could have trivially avoided.
|
||||
- **`PermissionTab.ets:252-258` `isStale` and the absent "已失效" branch on history rows.** Both match WebUI's `PermissionList.tsx:249-312` exactly, and the server's `AttachPermissionDeadline` genuinely returns early for already-decided rows, so the history branch would indeed be dead. The deliberate omission is correct, not a missing feature.
|
||||
- **`PermissionTab.ets:38-48` duplicate local `compactMailTime`.** Duplicated with `NavShared.compactMailTime`, but the two are used in different modules and I found no behavioural divergence between them. Drift risk, not a current defect.
|
||||
- **`InboxTab`/`SentTab` reading the same `MailStore.snapshot` by design.** The singleton is deliberate (`MailStore.ets:147-153` explains why) and the one-tab-at-a-time `if` in `MainPage.build()` keeps them from coexisting in the common case. The defect is only the *concurrent* write race (#16), not the sharing itself.
|
||||
- **Dead imports in `MainPage.ets` (`ApiError`, `ComposeView`, `Contact`, `DecideResponse`, `budgetLabel`, `budgetState`, `groupMailsBySession`, `partialLoadNotice`, and ~12 others), `CalendarPage.ets` (`weekdayNameOf`), `ComposePage.ets` (`filterIndexes`), `ContactsTab.ets` (`AccountInfo`), `PermissionPanel.ets` (`AmIcon`, `Motion`), `NavDestinations.ets` (`PaneModifier`).** Style noise, not reported as findings. I mention only `MailDetailView` (MEDIUM #12) because it is load-bearing evidence of a half-finished refactor.
|
||||
386
docs/reviews/harmony-state-review.md
Normal file
386
docs/reviews/harmony-state-review.md
Normal file
@ -0,0 +1,386 @@
|
||||
### CRITICAL (crash / data loss / cross-account leak / security)
|
||||
|
||||
**1. `MailStore.ets:446,593` — one `snapshot` field is shared by the Inbox pane and the Sent pane; concurrent loads clobber each other and the Inbox pane renders the Sent list.**
|
||||
|
||||
Both `loadInbox` and `loadSent` read `const snap = this.snapshot` (line 447 / 594) — the *same* `MailSnapshot` object — then assign `snap.mails` / `snap.groups` / `snap.loaded`. There is no per-tab snapshot and no epoch/generation guard.
|
||||
|
||||
```
|
||||
446: async loadInbox(ctx: common.Context, accountFilter: string): Promise<void> {
|
||||
447: const snap = this.snapshot;
|
||||
...
|
||||
593: async loadSent(ctx: common.Context, accountFilter: string): Promise<void> {
|
||||
594: const snap = this.snapshot;
|
||||
```
|
||||
|
||||
Both panes `@Watch` the *same* `AppStorage` key and each then calls `applyStoreSnapshot(store.snapshot)` (`MainPage.ets:357` inbox, `MainPage.ets:1301` sent). Trigger path: user opens the Comm page → Inbox starts `loadInbox` (several awaits, one per account) → user taps the "发件箱" tab → `SentTab.load()` runs `loadSent` concurrently. The last writer wins the shared `snapshot.mails/groups`, and the Inbox pane — which reads `store.snapshot` after *its* own await resolves — copies whatever the Sent load left there. Symptom: inbox shows sent mail (wrong `from_name`/`to_name` columns, wrong unread), or vice versa. The `Navigation` split layout keeps both panes mounted, so this is the normal path, not an edge case.
|
||||
|
||||
**2. `MailStore.ets:155,702` / `Logout.ets:78` — `clear()` exists but is never called on logout or account removal, so the previous account's mail stays in the singleton and is rendered by the next login.**
|
||||
|
||||
`clear()`'s own comment states the requirement ("否则下一个账号会看到上一个人的邮件"), but grep shows **zero callers**:
|
||||
|
||||
```
|
||||
702: clear(): void {
|
||||
703: this.snapshot = new MailSnapshot();
|
||||
704: this.bump();
|
||||
705: }
|
||||
```
|
||||
|
||||
`api/Logout.ets` `performLogout()` (lines 50–101) does unregister-push → server logout → `AccountManager.clearAll()` → `disconnectAll()` → `replaceUrl(LoginPage)` and never touches `MailStore`. Likewise `SettingsPage.ets:558 removeAccount()` only calls `SseService.disconnectAccount`. After logout, `LoginPage.aboutToAppear()` fast-paths into `MainPage` for the next account, and `InboxTab.loadData()` only reads the mail cache (`snap.loaded === 0` → `paintFromCache`) — but the account-specific cache keys are empty for a new account, so the UI flashes the **old account's in-memory snapshot** before the request returns. `AppearanceStore.wallpaper` (a full-resolution `PixelMap`) and `TopbarStore` content survive logout the same way.
|
||||
|
||||
### HIGH (real bug hit in production)
|
||||
|
||||
**3. `MailStore.ets:258-264, 194-197` — `notifyRemoteChange()` publishes a value that can equal the current `AppStorage` value, so `@Watch` panes never re-load on repeated SSE events.**
|
||||
|
||||
```
|
||||
194: private publishChange(): void {
|
||||
195: this.revision += 1;
|
||||
196: AppStorage.setOrCreate<number>(KEY_MAIL_REV, this.revision);
|
||||
197: }
|
||||
...
|
||||
258: notifyRemoteChange(): void {
|
||||
263: this.publishChange();
|
||||
264: }
|
||||
```
|
||||
|
||||
`revision` starts at 0 and is bumped by *both* `bump()` and `publishChange()`, so the two counters share one sequence — that part is fine. The real problem is the comment at line 259–262 is wrong about the mechanism: `setOrCreate` always writes `this.revision`, and `revision` is strictly increasing, so the value does change. However, `markReadLocal` (line 246) and `notifyRemoteChange` (line 263) both go through `publishChange`, and `loadInbox`'s `finally` (line 583) goes through `bump()` — which does **not** publish. So after the detail page marks a mail read, the Inbox pane is woken and calls `loadInbox`, whose completion bumps `revision` **without** publishing, leaving `AppStorage` and `revision` permanently out of sync. The next `markReadLocal` publishes `revision+1`, but the Inbox pane's own reload has already consumed an unpublished increment — panes silently miss refreshes. This is exactly the "接收邮件不正常 / 自动已读不正常" symptom class the file was written to fix.
|
||||
|
||||
**4. `MailStore.ets:628,499` — `mail.cc_list.length` is unguarded at both fill points, while the sibling line 3 lines below *is* guarded, re-introducing the documented white-screen crash.**
|
||||
|
||||
```
|
||||
499: mail.cc_count = mail.cc_list.length;
|
||||
...
|
||||
522: mail.attach_count = (mail.attachments ?? []).length;
|
||||
...
|
||||
628: mail.cc_count = mail.cc_list.length;
|
||||
631: mail.attach_count = (mail.attachments ?? []).length;
|
||||
```
|
||||
|
||||
Lines 505–518 in this same file are a long post-mortem explaining that Go's `omitempty` means the key is **absent** from JSON, so a bare `JSON.parse(raw) as T` yields `undefined` and `.length` throws `Cannot read property length of undefined` → white screen + restart. The file's defence is `MailSummary.normalize()` (`Models.ets:183`) which sets `m.cc_list = … ? [] : …`. But `normalize()` is only called from the *parse boundary* — `MailApi.inbox()` / `MailApi.sessionMails()` — and the 100-line comment at line 503 explicitly notes "列表这条没有 normalize" for `attachments`, which is why the `?? []` is inlined here. The exact same reasoning applies to `cc_list`: if any endpoint path (a cached-JSON path, a future caller, or a server response that omits `cc_list`) skips `normalize`, line 499/628 throws inside the per-account try/catch at line 529/634 — which, under `accountFilter === 'all'`, **swallows it into `failed.push(...)`** and silently drops that whole account's mail from the inbox. Line 525 (`unreadTotals.push`) is then also skipped, so the unread badge is wrong too, with no visible error.
|
||||
|
||||
**5. `MailStore.ets:595,462,465` — `loadSent` sets `loading = true` but calls `bump()` *before* the request, so the Inbox pane's `loading` flag is driven by the Sent tab's fetch (and vice versa) — combined with finding #1 this is the visible "收件箱一直转圈 / 空列表" failure.**
|
||||
|
||||
```
|
||||
595: snap.loading = true;
|
||||
...
|
||||
605: this.bump();
|
||||
...
|
||||
673: snap.loading = false;
|
||||
674: this.bump();
|
||||
```
|
||||
|
||||
`loadInbox` does the same at 462/465. Because both write the shared `snap.loading`, whichever request finishes last sets it to `false` — but a pane that started its own load while the other was mid-flight will observe `loading=false` with its own `mails` never written, i.e. an empty list with no spinner and no error.
|
||||
|
||||
**6. `AppearanceStore.ets:188-197` — `loadWallpaper` overwrites `this.wallpaper` without releasing the previous `PixelMap`, leaking a full-resolution bitmap on every account switch / every sync.**
|
||||
|
||||
```
|
||||
188: async loadWallpaper(api: AppearanceApi): Promise<void> {
|
||||
189: try {
|
||||
190: const bytes: ArrayBuffer = await api.fetchImageBytes();
|
||||
191: const src: image.ImageSource = image.createImageSource(bytes);
|
||||
192: this.wallpaper = await src.createPixelMap();
|
||||
193: } catch (e) {
|
||||
195: this.wallpaper = null;
|
||||
196: }
|
||||
197: }
|
||||
```
|
||||
|
||||
Two leaks per call: (a) the previous `PixelMap` is dropped without `release()` — a 4 MB wallpaper is native memory that stays mapped until GC pressure; (b) `src` (the `ImageSource`) is never released at all, even on the success path. `syncFromServer` calls this whenever `resp.has_image` is true (line 181), and it is invoked on every sync and every account switch. Note the contrast with `BackgroundPicker.ets:167-229`, which is scrupulous about `head.release()` / `passSource.release()` / `scaled.release()` in `finally` — the store has no equivalent.
|
||||
|
||||
**7. `AppearanceStore.ets:192` — the wallpaper `PixelMap` is decoded with no `desiredSize`, so a user-uploaded 2560px image is decoded at full size and held indefinitely.**
|
||||
|
||||
```
|
||||
192: this.wallpaper = await src.createPixelMap();
|
||||
```
|
||||
|
||||
`BackgroundPicker` deliberately caps the upload at `MAX_EDGE = 2560` (`ImagePrep.ts:35`), so the source is bounded — but it is still a 2560×2560 ARGB bitmap ≈ 26 MB resident for a background that is always drawn full-screen and downscaled by the compositor. `createPixelMap(options)` accepts a `DecodingOptions` with `desiredSize`; nothing here uses it. On a 2-in-1 or a low-RAM phone this is a plausible OOM, and it is retained for the whole session in a **static singleton** that outlives logout (finding #2).
|
||||
|
||||
**8. `AppearanceStore.ets:191-192` — no downscaling *and* the decode runs on the UI thread inside an `async` method that the page awaits before rendering.**
|
||||
|
||||
`createPixelMap()` with no `desiredSize` and no off-thread decode option means the full 2560px decode happens inline. `syncFromServer` (line 143) is `await`ed by the settings page before it applies the theme, so a large wallpaper produces a visible multi-hundred-ms UI stall on every sync, not just at startup.
|
||||
|
||||
### MEDIUM (correctness or robustness gap)
|
||||
|
||||
**9. `MailStore.ets:368-390` — `readCache` returns cached `MailLike` objects that are then mutated in place, and those mutations are written straight back to disk; `paintFromCache` mutates the objects it read.**
|
||||
|
||||
```
|
||||
375: const parsed = JSON.parse(raw) as Record<string, Object>;
|
||||
376: const rows = parsed.mails as MailLike[];
|
||||
...
|
||||
416: const m: MailLike = cached[j];
|
||||
417: m.source_account_id = acct.id;
|
||||
418: m.source_account_name = acct.displayName;
|
||||
```
|
||||
|
||||
`readCache` returns freshly-parsed objects each call, so the mutation is not a shared-reference bug — but note that `parsed.mails` is cast through `Record<string, Object>` and then straight to `MailLike[]` **without any normalization**, so a cache written by an older build (or a partially-written one) yields objects where `session_alias` / `permission_result` / `mail_type` are `undefined`. `paintFromCache` then feeds them to `groupMailsBySession` → `splitByPermission`, and `splitByPermission` reads `m.mail_type` (safe) but `MainPage`'s row builders read `c.session_alias.length` / `m.permission_result.length` — the exact `Cannot read property length of undefined` white screen the whole `Models.ets` post-mortem is about. `writeCache`'s comment (line 341) says "派生量多存一份无害（读回来时会重算）" but no recomputation exists on the read path, and the cache deliberately stores the *pre-split* `mergedMails` (line 561) so permission mails are persisted too — widening the exposure.
|
||||
|
||||
**10. `MailStore.ets:375-376` — `Record<string, Object>` then a blind `as MailLike[]`; `parsed.mails` is `Object`, and if a cache entry is a JSON array or the object is malformed the cast succeeds and the field reads return `undefined` at every access site.**
|
||||
|
||||
```
|
||||
375: const parsed = JSON.parse(raw) as Record<string, Object>;
|
||||
376: const rows = parsed.mails as MailLike[];
|
||||
377: if (rows === undefined || rows.length === 0) {
|
||||
```
|
||||
|
||||
`rows.length` is checked, so a missing key is handled, but a non-array truthy value (e.g. `{"mails": {}}`) passes the check and then `.length` on an object is `undefined` → the `for` loops at 285/415 iterate zero times and the pane silently shows an empty list with no error. A `Record<string, Object>` intermediate is also pointless here — it is not ArkTS-illegal (it is one of the four allowed utility types and the literal has no unquoted keys issue since no literal is constructed), but it provides no type safety at all.
|
||||
|
||||
**11. `MailStore.ets:216-247` — `markReadLocal` mutates `m.status` in place on the shared snapshot objects, which the page has already aliased into `@State`.**
|
||||
|
||||
```
|
||||
221: if (m.mail_id === mailId && m.status === 'unread') {
|
||||
222: m.status = 'read';
|
||||
```
|
||||
|
||||
`applyStoreSnapshot` does `this.mails = snap.mails as MailSummary[]` (`MainPage.ets:414`) — an alias, not a copy. ArkUI V1 `@State` only re-renders on reference change of the observed value, so mutating a nested element in place is invisible; the code compensates by reassigning `snap.groups` (line 234) and calling `publishChange()` (line 246), which works only because the pane re-reads the whole snapshot. It is correct today but fragile: any new consumer that holds `snapshot.mails` without re-reading after `markReadLocal` will not update. The `revision` contract is load-bearing in a way that is invisible at the call site.
|
||||
|
||||
**12. `MailStore.ets:685-699` — `dropSession` filters by `session_id` only, ignoring the account, while the grouping key is `account + session_id` — so archiving one account's session drops the same-id session from every other account.**
|
||||
|
||||
```
|
||||
688: for (let i = 0; i < snap.mails.length; i++) {
|
||||
689: const m = snap.mails[i];
|
||||
690: if (m.session_id !== sessionId) {
|
||||
691: kept.push(m);
|
||||
692: }
|
||||
693: }
|
||||
```
|
||||
|
||||
`sessionKey()` (`MailGrouping.ts:193-196`) is explicit that the key must be `source_account_id + '/' + session_id` "因为鸿蒙的收件箱是多账号合并的，同一个 `session_id` 出现在两个账号里是两件事". `dropSession` is called from `MainPage.ets:861` with `mail.session_id` only. In aggregate mode (`accountFilter === 'all'`, the default) two accounts can legitimately have sessions with colliding ids, and the list silently loses one account's mail with no error and no way to recover until the next reload. `dropSession` should take the `SessionGroup.key` (or mail id) rather than a bare `session_id`.
|
||||
|
||||
**13. `MailStore.ets:577` — `partialLoadNotice` is computed from the single largest per-account fetch, so in aggregate mode the "可能还有更多" hint is wrong whenever accounts have different page depths.**
|
||||
|
||||
```
|
||||
526: if (response.mails.length > maxFetched) {
|
||||
527: maxFetched = response.mails.length;
|
||||
528: }
|
||||
...
|
||||
577: snap.notice = partialLoadNotice(maxFetched, INBOX_PAGE_SIZE);
|
||||
```
|
||||
|
||||
With 3 accounts, if two return 50 and one returns 12, `maxFetched = 50` ⇒ the notice fires, which is right. But if all three return 12 (a quiet account each), `maxFetched = 12 < 50` ⇒ no notice, correct. The defect is the converse: with 2 accounts at 50 and 50 the aggregate list holds 100 rows, and the notice says "已加载 50 封" (`MailGrouping.ts:411`) — understating by half. The message hard-codes `fetched`, which in aggregate mode is not the number of rows displayed. Minor, but it is the "两处数字对不上" class the file repeatedly warns about.
|
||||
|
||||
**14. `MailStore.ets:555` — `snap.mails = mergedMails` stores the *pre-split* list while `snap.groups` is built from the *post-split* `inboxMails`, so `snap.mails.length !== snap.groups` contents.**
|
||||
|
||||
```
|
||||
552: const split: MailSplit = splitByPermission(mergedMails);
|
||||
553: const inboxMails: MailLike[] = split.normal;
|
||||
554:
|
||||
555: snap.mails = mergedMails;
|
||||
556: snap.groups = groupMailsBySession(inboxMails);
|
||||
557: snap.loaded = inboxMails.length;
|
||||
```
|
||||
|
||||
`loaded` is `split.normal.length` but `mails` is `mergedMails.length`. Any consumer that computes counts from `snapshot.mails` (rather than `groups`/`loaded`) will disagree with the header. `dropSession` (line 695-697) and `markReadLocal` (line 233-234) both *do* use the split view, so the two halves of the store disagree about what `mails` means depending on which method last ran — exactly the divergence `paintFromCache` avoids by storing `merged` and splitting locally, and `paintSentFromCache` avoids by not splitting at all.
|
||||
|
||||
**15. `Theme.ets:631-636` — `isDarkNow` returns `false` whenever the `AppStorage` key has not been written yet, so every `*For()` accessor silently returns the light palette before first paint.**
|
||||
|
||||
```
|
||||
631: static isDarkNow(dark?: boolean): boolean {
|
||||
632: if (dark !== undefined) {
|
||||
633: return dark;
|
||||
634: }
|
||||
635: return AppStorage.get<boolean>(Theme.KEY_IS_DARK) === true;
|
||||
636: }
|
||||
```
|
||||
|
||||
`AppStorage.get` on an absent key returns `undefined`, so the comparison is `false` — light. On a dark-mode device the first build pass (before `MainPage`'s `@StorageProp('agentmail.appearance.isDark')` is populated from `setColorMode`) paints every card as the light glass, then repaints dark. Since `GlassCardModifier.applyNormalAttribute` (Surface.ets:420) bakes the colour in at modifier-apply time, this is a visible light-to-dark flash on cold start in dark mode, and it is precisely the failure `Motion.captureForThemeFade` exists to hide — the fade is driven by `Theme.isDarkNow()` transitively via the snapshot, so the two can disagree.
|
||||
|
||||
**16. `Theme.ets:111-129` — `textSubtleFor(dark?)` accepts a `dark` parameter and then ignores it entirely, always returning `textMuted`.**
|
||||
|
||||
```
|
||||
111: static textSubtleFor(dark?: boolean): Resource {
|
||||
...
|
||||
128: return Theme.textMuted;
|
||||
129: }
|
||||
```
|
||||
|
||||
The `dark` argument is dead. The body comments at 113–127 explain the reasoning and say "两个主题都返回二级色", so the behaviour is intentional and the comment matches the code — but keeping an accepted-but-ignored parameter means a caller that *believes* it is overriding the theme (`textSubtleFor(true)`) gets silently ignored, and the linter cannot tell. It also breaks the pattern every sibling accessor (`accentFor`, `dangerFor`, `glassCardFor`, `chipBgFor`) follows. This is a latent trap rather than a live bug.
|
||||
|
||||
**17. `Surface.ets:485-487, 879-888` — `GlassCardModifier` attaches press feedback *and* sets a background colour in the same modifier; `PressFeedbackModifier.attach` writes `instance.backgroundColor(...)` from an `onTouch` handler, so the press colour is applied to the shared `instance` and can outlive the gesture if a `Cancel` is missed.**
|
||||
|
||||
```
|
||||
485: if (this.pressable) {
|
||||
486: PressFeedbackModifier.attach(instance);
|
||||
487: }
|
||||
...
|
||||
881: instance.onTouch((e: TouchEvent) => {
|
||||
882: const down: boolean = e.type === TouchType.Down;
|
||||
883: const cancel: boolean = e.type === TouchType.Cancel;
|
||||
885: const c: ResourceColor = down ? pressColor : Color.Transparent;
|
||||
886: instance.backgroundColor(c);
|
||||
887: });
|
||||
```
|
||||
|
||||
`cancel` is computed and never used — the restore happens on any non-Down event, so the variable is dead code, not a defect. The real issue is ordering: `attach` is called *after* `instance.backgroundColor(Theme.glassCardFor(...))` and `instance.border(...)`, and its `onTouch` will overwrite the glass colour with `Color.Transparent` on release. In `CompositeModifier` composition (`Surface.ets:921-928`) a later `TintModifier` re-applies a colour on the next normal-attribute pass, but the ordering between the three modifiers is load-bearing and undocumented in code — only in comments. Any new call site that composes them in a different order silently changes the press feedback.
|
||||
|
||||
**18. `Surface.ets:931-937` — `CompositeModifier.applyPressedAttribute` calls `p.applyPressedAttribute` on modifiers that never define it; the `!== undefined` guard is on the method, but the loop is over an interface whose members are all optional, so the guard is a no-op type check that will not catch a modifier throwing.**
|
||||
|
||||
```
|
||||
931: applyPressedAttribute(instance: CommonAttribute): void {
|
||||
932: for (let i = 0; i < this.parts.length; i++) {
|
||||
933: const p = this.parts[i];
|
||||
934: if (p.applyPressedAttribute !== undefined) {
|
||||
935: p.applyPressedAttribute(instance);
|
||||
936: }
|
||||
937: }
|
||||
938: }
|
||||
```
|
||||
|
||||
`TintModifier` and `PaneModifier` do not implement `applyPressedAttribute` at all. Reading `p.applyPressedAttribute` on such an object yields `undefined` and the guard works — but this means `GlassCardModifier`'s press feedback (which is wired via `onTouch` in `applyNormalAttribute`, not via `applyPressedAttribute`) is *never* driven by the pressed-state pass. The composite's pressed pass is effectively a no-op for every modifier in the codebase, so the two mechanisms are in competition and only one of them works. The `=== undefined` check on a method that is declared non-optional on the interface would be a compile error; here the interface member is optional so it compiles, which means the guard is doing real work at runtime on every press frame.
|
||||
|
||||
**19. `BackgroundPicker.ets:135-148` — user cancellation inside the picker `try` block returns without clearing `uploading`, and any exception thrown after `this.uploading = true` but before the outer `finally` is the only thing that resets it.**
|
||||
|
||||
```
|
||||
135: try {
|
||||
136: const options = new photoAccessHelper.PhotoSelectOptions();
|
||||
...
|
||||
141: if (result.photoUris.length === 0) {
|
||||
142: return; // 用户取消
|
||||
143: }
|
||||
...
|
||||
159: this.uploading = true;
|
||||
```
|
||||
|
||||
`this.uploading = true` is set at line 159, *after* the picker block, so the cancel path is safe. But the whole `await` chain from 160 to 244 is inside `try { … } catch { this.fail(…) } finally { this.uploading = false }`, and the three `return` statements at 175, 225 and 143 are all *inside* that try/finally — those are fine. The actual defect is different: `head.getImageInfo()` at line 169 is `await`ed, and if it rejects, the `finally { await head.release() }` at 227-229 runs and the rejection propagates to the outer `catch` at 238, which calls `this.fail(uploadFailureHint(businessMessage(e)))` — a message about a *network* failure ("上传失败：…") for what was a *local decode* failure. Cosmetic but user-hostile: the user is told to check the server when the real problem is an unreadable photo.
|
||||
|
||||
**20. `BackgroundPicker.ets:182-221` — the second compression pass re-decodes from `uri` but `plan.passes` always has 2 entries regardless of source size, so every upload of a small image that still exceeds the byte limit pays a full second decode.**
|
||||
|
||||
Not a defect on its own (the loop breaks on the first pass), but `CompressPlan.targetWidth/targetHeight` are computed from `MAX_EDGE` only (`ImagePrep.ts:169`) and never used by the component — the progress text at line 184 interpolates `pass.maxEdge`, so the displayed size is the pass's, not the plan's. Dead fields with a parallel display path; if a caller ever surfaces `targetWidth` the two will disagree.
|
||||
|
||||
**21. `Motion.ets:33,41-50` — `Motion.cached` is a static latch that never invalidates, so toggling the system "reduce motion" setting has no effect until the process restarts.**
|
||||
|
||||
```
|
||||
33: private static cached: boolean | undefined = undefined;
|
||||
...
|
||||
44: Motion.cached = accessibility.isAnimationReduceEnabledSync();
|
||||
```
|
||||
|
||||
The comment at 23-25 acknowledges this trade-off explicitly ("运行中改系统设置不会立刻生效 —— 可接受"). For a system accessibility setting this is the wrong side to err on: the obligation is to stop animating, and a user who enables reduce-motion mid-session keeps getting full-motion transitions. There is no `invalidate()` and no subscriber. Reporting as a robustness gap, not a bug, because the trade-off is documented.
|
||||
|
||||
**22. `Motion.ets:200-209` — `captureForThemeFade` returns a `PixelMap` that the caller must release; nothing in this layer releases it, and the catch path returns `undefined` after the snapshot may have partially allocated.**
|
||||
|
||||
```
|
||||
200: static async captureForThemeFade(ui: UIContext, rootId: string): Promise<image.PixelMap | undefined> {
|
||||
...
|
||||
204: return await ui.getComponentSnapshot().get(rootId, opt);
|
||||
```
|
||||
|
||||
A full-window `PixelMap` at device resolution (~3184×2232 ARGB ≈ 28 MB on the documented emulator) is returned to a caller in another file. If the caller's theme switch throws between snapshot and use, or the component unmounts, the bitmap is never `release()`d. Unlike `BackgroundPicker`, there is no `finally` and no ownership contract documented on the return type.
|
||||
|
||||
**23. `MailStore.ets:349,370,461` — `preferences.getPreferencesSync` is called on every read and every write with no handle reuse and no `close()`.**
|
||||
|
||||
```
|
||||
349: const store = preferences.getPreferencesSync(ctx, { name: PREF_STORE });
|
||||
355: store.flush();
|
||||
...
|
||||
370: const store = preferences.getPreferencesSync(ctx, { name: PREF_STORE });
|
||||
```
|
||||
|
||||
`writeCache` is called once **per account** per load (`persistByAccount` loops), each doing a `getPreferencesSync` + `putSync` + `flush`. `flush()` is a synchronous disk write on the UI thread, inside the `loadInbox` `try` block, between the network fetch and the snapshot assignment. With 5 accounts that is 5 synchronous fsyncs per refresh, on the main thread, on every inbox load and every SSE-driven reload. The `AppearanceStore` and `TopbarStore` have the same pattern (single account, so less severe).
|
||||
|
||||
**24. `IcsFile.ets:66-78` — `pickIcsText` reads the whole file into memory with no size cap, and `saveIcsText` writes with no size cap either.**
|
||||
|
||||
```
|
||||
67: const stat = fileIo.statSync(file.fd);
|
||||
68: const buf = new ArrayBuffer(stat.size);
|
||||
69: fileIo.readSync(file.fd, buf);
|
||||
```
|
||||
|
||||
`stat.size` is attacker/user-controlled (a user picks the file). A multi-GB file selected by mistake causes an immediate allocation failure, and `readSync` returning fewer bytes than `stat.size` is not checked — `out.ok` is then derived from `out.text.length > 0` (line 72), so a **truncated read yields `ok: true` with a partial calendar** which is then POSTed to `/calendar/import.ics`. There is no loop around `readSync` to drain the fd. Same for `saveBinaryFile` (line 154) with attachment bytes.
|
||||
|
||||
### STRESS (plausible under load/concurrency/long session)
|
||||
|
||||
**25. `MailStore.ets:477-542, 614-642` — the per-account fetch loop is fully sequential and allocates a fresh `ApiClient` per account per load; with N accounts a refresh costs N round-trips serially.**
|
||||
|
||||
```
|
||||
483: const accountClient: ApiClient = new ApiClient(ctx);
|
||||
484: accountClient.setBase(acct.server);
|
||||
485: accountClient.setToken(acct.token);
|
||||
```
|
||||
|
||||
`new ApiClient(ctx)` per account per load, inside a `for` loop, with an `await` immediately after. On 5 accounts on mobile networks the inbox takes 5× the single-request latency before *any* data appears, and each `ApiClient` presumably holds an `http.HttpRequest` handle. The `finally` at 581 sets `loading = false` only after the last account, so the empty-list-then-spinner window is the full serial chain. `PaintFromCache` mitigates the first paint only.
|
||||
|
||||
**26. `MailStore.ets:176-178` — `revision` is a `number` incremented without bound; at the SSE rate described in the file's own header (one event per new mail) it is harmless, but `notifyRemoteChange` fires per `new_mail` *per account* and `AppStorage.setOrCreate` on every one forces a re-render storm across all panes.**
|
||||
|
||||
```
|
||||
176: private bump(): void {
|
||||
177: this.revision += 1;
|
||||
178: }
|
||||
```
|
||||
|
||||
`onGlobalSseEvent` (`MainPage.ets:2914`) calls `notifyRemoteChange()` for every `new_mail` event. Each publish wakes both the Inbox and Sent panes, each of which fires a **full multi-account reload**. A burst of 10 new mails (a busy agent session) therefore triggers 10 × N_accounts HTTP round-trips with no debounce, no coalescing, and no in-flight guard. This is the most likely source of the reported "接收邮件不正常" (UI stutter / list flicker) under load.
|
||||
|
||||
**27. `MailStore.ets:446,593` — no in-flight guard: `loadInbox` can be re-entered while a previous `loadInbox` is still awaiting, and the two write the same snapshot with no ordering guarantee.**
|
||||
|
||||
There is no `if (this.loading) return`, no generation counter, no AbortController. Given finding #26 (every SSE event triggers a reload from every pane), overlapping loads are not hypothetical. The slower earlier call resolves last and overwrites the fresher result with stale data — the classic last-writer-wins race, with no epoch guard anywhere in the file.
|
||||
|
||||
**28. `MailStore.ets:81,350` — the mail cache stores up to 50 mails × N accounts as a single JSON string per account in `preferences`, with no total-size budget and no eviction.**
|
||||
|
||||
```
|
||||
81: const CACHE_MAX: number = 50;
|
||||
...
|
||||
350: const n: number = mails.length < CACHE_MAX ? mails.length : CACHE_MAX;
|
||||
```
|
||||
|
||||
The file's own header cites the 16 MB per-value limit as the reason for the cap, but 50 `MailSummary` objects with full `body_preview` and `cc_list` can easily be 200–400 KB per account. Accounts are never removed from the cache when `AccountManager.removeAccount` is called (`SettingsPage.ets:558`) — `writeCache`/`readCache` key on `kind + accountId` and nothing ever deletes a key. Every account ever logged into on the device leaves a permanent preferences entry.
|
||||
|
||||
**29. `TopbarStore.ets:88-90` — `loadLocal` assigns `parsed.quotes ?? []` where `parsed` is a bare `JSON.parse` cast, so a cache written by an older build yields `quotes: [ {text: undefined, source: undefined} ]` and the rotating topbar renders `[object Object]`/blank forever.**
|
||||
|
||||
```
|
||||
88: const parsed = JSON.parse(raw) as TopbarContent;
|
||||
89: out.quotes = parsed.quotes ?? [];
|
||||
90: out.signature = parsed.signature ?? '';
|
||||
```
|
||||
|
||||
The `??` guards the array and the string, but not the *elements*. The class-field defaults (`text: string = ''`) do not apply to a missing JSON key — the same lesson the `Models.ets` header records four times. The file's own stated discipline is "不要手工挑字段" (don't hand-pick fields), which is right for writing but means the read path must normalize; it doesn't.
|
||||
|
||||
**30. `ComposeIntent.ets:44,65-72` — `pending` is a plain static boolean with no timestamp; an intent raised while no listener is mounted (e.g. during a route transition) is delivered to whatever component mounts next, potentially much later.**
|
||||
|
||||
```
|
||||
44: static pending: boolean = false;
|
||||
...
|
||||
65: static request(): void {
|
||||
66: const fn: (() => void) | undefined = ComposeIntent.listener;
|
||||
67: if (fn !== undefined) {
|
||||
68: fn();
|
||||
69: return;
|
||||
70: }
|
||||
71: ComposeIntent.pending = true;
|
||||
72: }
|
||||
```
|
||||
|
||||
If the user presses the compose shortcut during a `replaceUrl` transition, the listener is momentarily `undefined` (or, worse, still set to the *previous* `CommPage` instance which is being torn down), and the intent is either dropped, delivered to a dying component, or replayed on the next mount minutes later. Same shape in `ReplyIntent` (line 107) and `PopIntent` (line 149). There is no `aboutToDisappear`-ordering guarantee documented, and `clearListener()` sets `undefined` unconditionally, so a remount-then-unmount ordering loses the intent entirely.
|
||||
|
||||
**31. `Models.ets:312-314` — `MailDetail.normalize` uses `Array.isArray` to guard `permission_options`, which is correct, but `permission_options` is then read by the permission panel with no further guard; a server that returns a non-array truthy value (e.g. a string) would survive `Array.isArray` returning `false` → set to `[]`, so this one is actually fine.**
|
||||
|
||||
No defect — I checked specifically because the pattern is suspicious. Listed under ruled-out findings rather than here.
|
||||
|
||||
### FALSE POSITIVES YOU RULED OUT
|
||||
|
||||
- **`MailStore.ets:155` `private static instance: MailStore | null = null`** — flagged as `null` usage (ArkTS rule 7: prefer `undefined`). It is *technically* the banned form, but the file's own convention is `T | null` throughout (`IcsFile.ets:49` `IcsPickResult`, `AppearanceStore.ets:45` `image.PixelMap | null`, `MainPage.ets:212`). `null` is only banned as a *value* (assigning/returning `null` to a `T` slot); a nullable sentinel in a `static` field with an explicit `=== null` check is consistent with the codebase and does not compile-error. I am not reporting the `instance`/`wallpaper` fields, but the `MailLike[] | null` returns at lines 282, 368, 411 and the `const cached = ...` + `=== null` checks are a real narrowing burden: `readCache` returns `MailLike[] | null` and every caller must check, and the *unannotated* `const cached` at 281/411 relies on inference. That is the rule-7 concern in its mildest form; flagging as MEDIUM-worthy, not CRITICAL.
|
||||
|
||||
- **`BackgroundPicker.ets:189-191, 405-408` — untyped object literals.** `const decodeOptions: image.DecodingOptions = { desiredSize: { width, height } }` and `const opt: image.PackingOption = { format: 'image/jpeg', quality: … }` are *annotated* with a named SDK type, and the outer annotation does propagate to the nested `desiredSize` literal here because `DecodingOptions.desiredSize` has a declared SDK type. Legal — the skill's "outer type does not propagate into nested literals" rule applies to *object literals in general positions*, and this one is a direct assignment to a declared type, which is the accepted pattern. Not a violation.
|
||||
|
||||
- **`Motion.ets:85, 162-165, 203` — object literals returned from `async`/typed functions** (`{ duration: 0, curve: Curve.Linear }`, `PageTransitionOptions`, `componentSnapshot.SnapshotOptions`). All three are assigned to an explicitly typed local before being returned (`const o: PageTransitionOptions = {…}`; `const opt: componentSnapshot.SnapshotOptions = {…}`; the `return {…}` at 85 returns `AnimateParam`, the declared return type of `Motion.anim`, and its members are all SDK-typed with no nested literals). Legal.
|
||||
|
||||
- **`PushContract.ts:230, 139-144` — object literals in `buildTokenBody` / `parseNotificationData`.** `const body: PushTokenBody = { provider: provider, token: token }` is annotated; the `out` literal at 139 is assigned to a declared `const out: PushNotificationData` first, and its values are all `string` primitives — no nested literal. The `ParseJudgement`/`AppearanceResponse`-style named classes are used consistently. Legal.
|
||||
|
||||
- **`ApiBase.ts:144-185` — many `return { ok: false, base: '', error: '…', warning: '' }` literals.** Each is a direct return against the declared `ApiBaseCheck` return type with only primitive members and no nesting. The `Record` rule (quoted keys) is not triggered — these are interface-typed, not `Record`-typed. Legal. Same for `describeFailure`'s `FailureText` returns.
|
||||
|
||||
- **`DeviceProbe.ts:54, 77` — `{ bundle: bundle, state: state, appState: appState }` pushed into `MissionInfo[]`.** `out` is declared as `MissionInfo[]`, so the literal has a type context from the `push` parameter; all members are `string`, no nesting. Legal. (`RE_BUNDLE.exec(line)` returns `RegExpExecArray | null` and is correctly narrowed with `!== null` before `mb[1]` is read.)
|
||||
|
||||
- **`Wallpaper.ts:141-159` — `radial()`/`linear()` build `PresetLayer` via `new` + field assignment rather than literals.** Deliberate and correct; not a violation, just a different (safer) pattern than the rest of the codebase.
|
||||
|
||||
- **`.ics` generation.** I looked for escaping/folding/CRLF/date-format bugs and there are none *on the client side* — the client never generates ICS. `IcsFile.ets` only selects, reads, and writes text; the actual `VEVENT`/`DTSTART`/CRLF emission is server-side in `server/internal/handler/calendar.go:483-547`, which uses `\r\n` throughout and `e.EventTime.UTC().Format("20060102T150405Z")` (a correct RFC5545 UTC form). The import path also handles LF-only files, TZID-parameterised `DTSTART`, and `\n` escapes in `SUMMARY` (verified by `ics_test.go`). No finding here; the only real client-side issues in this file are the memory ones (#24).
|
||||
|
||||
- **`MailGrouping.ts:365-372` — `groups.sort` comparator returns `0` when either `latest` is `undefined`.** I checked for an off-by-one / instability here: `latest` is assigned unconditionally at line 353 (`g.mails[0]`, and `mails` is non-empty because the group was created by pushing into it), so the `undefined` branch is unreachable. The comparator is a valid strict weak ordering (`byNewest` is transitive: primary `created_at` desc, tiebreak `mail_id` desc). Not a bug.
|
||||
|
||||
- **`Calendar.ts:133-144` — the 42-cell tail-filling loop.** `d.setDate(d.getDate() + (cells.length - (blanks + len) + 1))` — I verified the arithmetic: at the first iteration `cells.length === blanks + len` so the delta is `+1`, and each subsequent iteration advances by exactly one more day because both `cells.length` and `getDate()` increase by 1. Correct. `firstWeekday` uses `Date.UTC` + `getUTCDay` consistently with `leadingBlanks`, and `weekdayNameOf` uses the same path. `addMonths` at line 58: `Math.floor(total/12)` with `total = year*12 + (month-1) + delta` — for `month=1, delta=-1, year=2026` gives `2025, 12`; correct across the year boundary. `addDaysIso` / `weekDaysOf` use `Date.UTC` throughout and validate `parts.length !== 3` plus `Number.isNaN`. `judgeSwipe`'s `Number.MAX_SAFE_INTEGER` guard for `ms === 0` avoids the divide-by-zero. No off-by-one found.
|
||||
|
||||
- **`Models.ets:233-239` — `str(v: string | undefined | null)`.** Uses `null` in a *type* position on a narrowing helper. This is the codebase's deliberate `omitempty` defence and does not compile-error (the ban is on producing `null` as a value). `MailDetail.normalize` line 311 `!!m.permission_multi_select` and line 312 `Array.isArray` guard are both correct. `Me`, `Session`, `Contact`, `PermissionRequest`, `CalendarEvent` all have class-field defaults that only fail for *absent JSON keys*, which is the documented four-times-repeated lesson — but every model that is actually read through `.length`/`.trim` (`MailSummary`, `MailDetail`, `AddressSuggestion`) has a `normalize()` at the parse boundary. Not a finding here beyond #9/#10.
|
||||
|
||||
- **`Motion.anim` returning `{ curve: spring }` with no `duration`** — intentional and documented at length (Motion.ets:68-88, Theme.ets:1128-1134). The SDK ignores `duration` under spring curves, so omitting it is correct, and `Motion.anim` swaps *both* the curve and the duration when reduced-motion is on, so the accessibility switch is genuinely not silently broken. Correct as written.
|
||||
|
||||
- **`Theme.glassCardFor(wall, dark?)` and the other `*For` accessors reading `AppStorage` themselves** — this is a deliberate design (documented at 611-636) that removes the need for callers to thread `isDark` through, and it is more robust than the alternative. The one genuine gap is finding #15 (absent key ⇒ light), which is a startup-ordering issue, not a design error.
|
||||
|
||||
- **`Surface.ets:103, 195, 292` — `@BuilderParam content: () => void = this.emptyContent`.** This is the documented V1 pattern from the skill (`@BuilderParam` + `@Builder` fallback), and the `@Builder emptyContent()` declarations are valid. The `.onClick(...)` chaining hazard the skill warns about is not triggered — `AppHeader`'s `trailing()` at line 763 is called as a statement inside `build()`, with the modifiers on the enclosing `Row`. Legal.
|
||||
|
||||
- **`CompositeModifier` / `AttributeModifier` implementations** — `applyNormalAttribute`/`applyPressedAttribute` are the correct interface methods, the `parts: AttributeModifier<CommonAttribute>[]` field is properly typed, and `of()` is a static factory that sets private fields (the skill's recommended pattern instead of constructor parameter properties, which are banned). The constructor parameter-property rule is respected throughout `Common/` — I checked every class in scope and none uses `constructor(private x: T)`.
|
||||
355
docs/reviews/push-and-gui-review.md
Normal file
355
docs/reviews/push-and-gui-review.md
Normal file
@ -0,0 +1,355 @@
|
||||
# 系统推送链 与 GUI 审查报告
|
||||
|
||||
审查对象：
|
||||
- **推送链（端到端）**：`client/harmony` 推送侧（`PushService.ets` / `PushContract.ts` / `EntryAbility.ets`）
|
||||
＋ `server/internal/push/*`（`push.go` / `hms.go` / `config.go`）＋ `handler/push.go`
|
||||
＋ `repo/push_tokens.go` ＋ `notify/mail.go`
|
||||
- **GUI（Electron/React）**：`client/electron/src/`（约 14,100 行，10 个 store + 30 个组件）
|
||||
|
||||
基线：`node test/run-all.mjs` → 577 判据 / 565 通过 / 3 红（与本报告无关，见上份报告）
|
||||
Go 侧未能执行 `go test`（本机无 `GOMODCACHE`/`GOPATH`），推送逻辑以**读码 + 对照既有测试**验证。
|
||||
|
||||
---
|
||||
|
||||
# 第一部分：系统推送
|
||||
|
||||
## 一、先说结论：推送链整体是健康的
|
||||
|
||||
设计上有几处**明确比一般实现高明**的地方，先列出来避免后续误改：
|
||||
|
||||
- **不写死华为**：`Notifier` 接口 + `Factory` 注册表 + `push.json` 配置式接入，
|
||||
`RegisterType` 一个函数就能加厂商。
|
||||
- **单项配错不拖垮服务**：`config.go:133-159` 逐项 `continue` 并打明确日志，
|
||||
网关照常启动 —— 推送是锦上添花，不能变成单点故障。
|
||||
- **推送是可选旁路且零开销**：`shouldDispatch` 抽成**纯函数**（`push.go:115-117`），
|
||||
且注释诚实记录了变异验证曾经失败的原因（goroutine 竞态让错误理由也能绿）。
|
||||
- **所有失败静默**：客户端 `PushService` 每一步 catch 都不打扰用户、不阻塞登录。
|
||||
- **有效 token 自愈**：华为回 `80300007` / `illegal_tokens` 时自动删表（`hms.go:244-254`），
|
||||
因为测试额度是**项目级**的，死 token 会白吃额度。
|
||||
- **动态 `IN (...)` 的方言安全性我已验证**：`repo/push_tokens.go` 手拼 `$1,$2…` 占位符，
|
||||
而 `db.go:7` 明确「SQLite 也支持 $1/$2，无需改写」⇒ 两种方言都安全，**不是 SQL 注入**
|
||||
（占位符位置由 `i+1` 生成，值一律走参数绑定）。
|
||||
|
||||
## 二、推送链的真实缺陷
|
||||
|
||||
### 1.【HIGH】每日额度按 **token 数**扣，且在**实际投递之前**扣
|
||||
|
||||
`hms.go:192-194`：
|
||||
```go
|
||||
if !h.reserveDaily(len(tokens)) {
|
||||
return fmt.Errorf("达到每日推送上限 %d 条（HMS_DAILY_LIMIT）", h.DailyLimit)
|
||||
}
|
||||
tok, err := h.accessToken(ctx) // ← 额度已经扣了
|
||||
...
|
||||
// 网络请求在下面，失败也只记日志
|
||||
```
|
||||
|
||||
两个问题叠在一起：
|
||||
|
||||
- **计数单位错**：`reserveDaily(len(tokens))` 记的是 **token 个数**，而注释与变量名
|
||||
（`dayCount`、`达到每日推送上限 N 条`）都说是「**条**」。华为的测试消息额度是
|
||||
**按条消息**限的（一次 `messages:send` 一条消息，无论带几个 token）。
|
||||
⇒ **3 个设备 1 封邮件就吃掉 3 条额度**，实际发出去 1 条。
|
||||
多设备自部署用户会以约 1/设备数 的速度提前耗尽 1000 条/天。
|
||||
- **扣在投递前**：`accessToken` 失败、HTTP 失败、华为返回非成功码 —— 这些**一条都没发出去**，
|
||||
但额度已经扣掉了。而失败只 `log.Printf`（`push.go:174-176`），用户侧完全无感。
|
||||
⇒ 一次网络抖动会静默烧掉配额。
|
||||
|
||||
**修法**：`reserveDaily(1)`（按批次计），并把预留挪到**确认拿到 access_token 之后**、
|
||||
或失败时归还。
|
||||
|
||||
### 2.【HIGH】HMS 通知载荷缺 `click_action`，锁屏点击可能带不出 `data`
|
||||
|
||||
`hms.go:206-217`：
|
||||
```go
|
||||
"message": map[string]any{
|
||||
"token": tokens,
|
||||
"notification": map[string]any{ "title": ..., "body": ... },
|
||||
"data": string(data), // ← data 带了，但没有任何 click 行为声明
|
||||
}
|
||||
```
|
||||
|
||||
客户端**完全依赖** `data` 里的 `mail_id` 做跳转：
|
||||
`EntryAbility.onCreate` / `onNewWant` → `PushService.routeFromWant` → `routeFromWant(params, ...)`
|
||||
读 `parameters['data']`（`EntryAbility.ets:84-86`、`120-122`）。
|
||||
|
||||
而载荷里**没有 `click_action`、没有 `intent`、没有 `backgroundMode`**。
|
||||
HMS v1 在「普通通知」形态下，点击行为由 `click_action` 决定（`1`=打开应用、`2`=打开页面、
|
||||
`3`=触发 Action）；**缺省时不同形态/系统版本对 `data` 是否透传到 `want.parameters` 的行为并不一致**。
|
||||
项目里 `PushContract.ts:9-10` 自己写着「服务端推送是**至多一次、无幂等键**」，
|
||||
`EntryAbility.ets:78-82` 也记录了 2026-09-15 为此专门加过冷启处理 ——
|
||||
说明这条链此前已经因「跳转目标丢失」返工过一次，而**根因（载荷形态）没有变**。
|
||||
|
||||
**这正是「App 弹到前台、停在列表」的另一种表现形态。**
|
||||
|
||||
**修法**：显式声明点击行为（HMS v1 `click_action` 或按 Action 形态补 `intent`），
|
||||
并**用真机 token 实测一次**确认 `data` 能进 `want.parameters` ——
|
||||
`hms.go:50` 自己写着「我**无法在本机验证成功路径**（需要一台真机产出的 token）」，
|
||||
这个未知项至今没有关闭。
|
||||
|
||||
### 3.【MEDIUM】通知标题把**完整邮件主题**写进锁屏
|
||||
|
||||
`push.go:36-38` 的设计声明：
|
||||
> 「字段刻意少：推送只负责『通知栏那一行 + 点进去看哪封信』。
|
||||
> **正文不进通知**，否则锁屏上就会露出邮件内容。」
|
||||
|
||||
但 `hms.go:211` 实际是：
|
||||
```go
|
||||
"title": "新邮件：" + truncate(n.Subject, 40), // ← 主题全文前 40 字
|
||||
"body": n.From,
|
||||
```
|
||||
|
||||
`Subject` 是**邮件主题本身**，标题里就带「报销 3 万走哪个账户」这类内容。
|
||||
声明说"正文不进通知"，实现却把**主题**放进去了 —— 这在锁屏上是可见的。
|
||||
（`truncate` 限了 40 字，但 40 字足够泄漏一个机密项目名。）
|
||||
|
||||
**修法**：要么按声明改成固定文案（如「新邮件」+ 发件人），
|
||||
要么把 `push.go` 的注释改成与实现一致 —— 现状是**注释在骗人**，这比任一选择都糟。
|
||||
|
||||
### 4.【MEDIUM】客户端写死 `provider: 'hms'`，与"配置式多厂商"自相矛盾
|
||||
|
||||
`PushContract.ts:13` + `PushService.ets:348,381` 无条件用 `PROVIDER_HMS`。
|
||||
|
||||
服务端明确支持一实例多厂商、且 `name` 可覆盖（`config.go:44-47`）。
|
||||
但客户端把 `hms` 硬编码，`PushService.ts:360-361` 又只是**把** `resp.providers` 打进日志
|
||||
就丢掉，从不据此选择。⇒ 服务端配了小米通道，鸿蒙客户端永远不会往它登记。
|
||||
|
||||
讽刺的是 `PushContract.ts:203-207` 的注释正在讲"不要硬编码只有 hms 合法"这个原则，
|
||||
而调用处恰恰硬编码了。
|
||||
|
||||
### 5.【MEDIUM】`NotificationLedger` 只在 want 层去重，SSE 侧无跨通道去重
|
||||
|
||||
`PushContract.ts:9-10` 说「重复保护只能落在客户端」，`EntryAbility` 用
|
||||
`NotificationLedger(50)` 按 `mail_id` 去重（`EntryAbility.ets:42`）。
|
||||
|
||||
但这只覆盖「点通知」这一条路径。用户实际会看到**两次**：
|
||||
1. App 在前台 → SSE `new_mail` → `MainPage.ets:2911` `showToast('新邮件：…')`
|
||||
2. HMS 通知同时下发 → 通知栏一条
|
||||
|
||||
两条路径**各自独立**、无共享去重台账 ⇒ 前台每次都"toast + 通知栏"双份。
|
||||
`main.go` 端也没有按在线状态抑制推送的逻辑（`shouldDispatch` 只看 `Enabled()` 和收件人非空）。
|
||||
|
||||
### 6.【MEDIUM】`ledger` 是 `EntryAbility` 实例字段，50 条就会开始漏
|
||||
|
||||
`EntryAbility.ets:42` `private ledger: NotificationLedger = new NotificationLedger(50)`。
|
||||
台账按 `mail_id` 去重且**有界（50）**。高流量场景（Agent 批量发信）下，
|
||||
第 51 封之后旧 `mail_id` 被挤出 ⇒ 同一封邮件若被系统重放 `want`（HMS 在网络重试时确实会）
|
||||
会**再次触发跳转**。属于可接受但需知情的取舍，`limit=50` 是否够值得确认。
|
||||
|
||||
### 7.【LOW】`client` 侧 `PushService` 在 `aboutToAppear` 之前就上报（已知，非新问题）
|
||||
|
||||
`EntryAbility.onCreate:106` 用 `ApiClient.getInstance(...)` 上报，而该实例**尚未 `init()`**
|
||||
（`init()` 只在 `LoginPage.ets:101` 与 `MainPage.ets:3128` 调）⇒ 此刻无 token，服务端回 401。
|
||||
`LoginPage.ets:168-192` 的 `reportPushToken` 已在登录后补报，**已修复**。
|
||||
仅记录：`EntryAbility.ets:101` 那句注释仍写「`PushService.isEnabled`，默认关」，
|
||||
而 `PushContract.DEFAULT_PUSH_ENABLED = true` —— **注释与实现相反**，会误导后续维护。
|
||||
|
||||
### 8.【LOW】`SessionApi` 同类问题：`null` 违反本仓约定
|
||||
|
||||
`api/SessionApi.ets:48` `proposal: RenameProposal | null = null`。
|
||||
本仓 ArkTS 约定偏向 `undefined`。服务端用 `null` 表示"没有待处理建议"，
|
||||
客户端需要原样承载 —— 属于**契约强制**，但与 `PushContract.ts` 的 `| undefined` 风格不一致。
|
||||
|
||||
---
|
||||
|
||||
# 第二部分：GUI（Electron/React）
|
||||
|
||||
## 一、先说结论：GUI 的 XSS 面是干净的
|
||||
|
||||
我独立复核并确认：
|
||||
|
||||
- **全仓零** `dangerouslySetInnerHTML` / `innerHTML` / `insertAdjacentHTML`
|
||||
- 三个 Markdown 渲染点（`MailView.tsx:146`、`:690`、`ComposePage.tsx:305`）统一用
|
||||
`<Markdown remarkPlugins={[remarkGfm]}>`，**没有 `rehype-raw`** ⇒ 原始 HTML 被转义
|
||||
- 唯一的动态 `href` 是 `Attachments.tsx:22` 的 `api.attachmentURL(a.attachment_id)`
|
||||
（服务端签发的 id + `encodeURIComponent` 过的 token），唯一的动态 `src` 是本地生成的 JPEG data URL
|
||||
- `electron/main.cjs:47-48`：`contextIsolation: true` / `nodeIntegration: false` ✔
|
||||
- `main.cjs:47-48` 有 `preload`，未见 `openExternal` / `will-navigate` 缺口
|
||||
|
||||
**这条链上 `test/markdown-xss.test.mjs` 的不变量没有被绕过。**
|
||||
|
||||
## 二、GUI 的真实缺陷
|
||||
|
||||
### 1.【CRITICAL】切账号不清 store ⇒ **A 账号的邮件显示在 B 账号下**
|
||||
|
||||
`AccountSwitcher.tsx:56-62`：
|
||||
```tsx
|
||||
const pick = async (id: string) => {
|
||||
setOpen(false);
|
||||
await setActive(id);
|
||||
await fetchInbox('all'); // ← 只重取了收件箱
|
||||
};
|
||||
```
|
||||
|
||||
`setActive`（`accountStore.ts:186-197`）会 `syncAuth(next, id)`，**把 `api/config` 单例的
|
||||
`API_BASE` 与 bearer 翻到新账号**。此后：
|
||||
|
||||
- `useMailStore.sent`、`currentMail`
|
||||
- `useSessionStore.sessions`、`currentSession`、`currentSessionMails`
|
||||
- `useContactStore.contacts` / `archivedContacts`
|
||||
|
||||
**一个都没复位**。而后续单账号请求（`api.getSent()`、`api.getMail()`、`api.archiveContact()`）
|
||||
会带上 **B 的凭证**。
|
||||
|
||||
后果：A 账号下打开发件箱 → 切到 B → 发件箱**仍是 A 的邮件**；
|
||||
`MailView.tsx:67` 仍按 A 的会话渲染全文；**从这些陈旧视图点转发/归档/批准，改的是 B**。
|
||||
|
||||
grep 确认：只有 `MailList.pick` / `ContactPanel.open` / `PermissionList.pick` 会调
|
||||
`clearSession()` / `clearCurrentMail()`，**账号切换路径一个都没有**。
|
||||
|
||||
**修法**：`setActive` 之后除 `fetchInbox` 外，还要
|
||||
`clearCurrentMail()` / `clearSession()` / 清 `sent` / 清 contacts。
|
||||
|
||||
> 这与鸿蒙侧 `MailStore.clear()` **零调用**（上份报告第 3 条）是**同一个根因的两端**：
|
||||
> 两个客户端都在"切换身份"时只刷新了主列表，没清其余状态。
|
||||
|
||||
### 2.【CRITICAL】`lastUploadedImage` 是模块级单例、不按账号分 ⇒ 第二个账号的壁纸**永不上传**
|
||||
|
||||
`appearanceSync.ts:40`：
|
||||
```ts
|
||||
let lastUploadedImage = ''; // 模块级，不带账号维度
|
||||
```
|
||||
|
||||
`pull()` 第 87 行写它（从服务端图回填时），`push()` 第 115 行用它做跳过判据：
|
||||
```ts
|
||||
if (ok && bg.kind === 'image' && bg.imageDataUrl && bg.imageDataUrl !== lastUploadedImage) {
|
||||
```
|
||||
|
||||
路径：A 账号把壁纸设为 X（上传成功 ⇒ 标记 = X）→ 切到 B 账号，B 的服务端记录是
|
||||
`saved:false` ⇒ `pull()` 走 `appearanceSync.ts:69-76` 的"**把本地这份推上去**"分支
|
||||
（`:74` 调 `push()`）⇒ 但跳过判据被 A 留下的标记满足 ⇒
|
||||
**B 的壁纸永远不传**，而 `:120` 仍把 `status` 置为 `'synced'`。
|
||||
|
||||
⇒ B 的壁纸在其**其它所有设备**上都缺失，而界面上显示"已同步"。
|
||||
|
||||
讽刺的是该文件头明确把 localStorage 键按账号做了隔离，
|
||||
却漏了这个变量 —— 它同样需要 `accountId` 维度。
|
||||
|
||||
**修法**：`lastUploadedImage` 改成 `Map<accountId, string>`，或直接存进按账号分区的
|
||||
localStorage（与现有 key 策略一致）。
|
||||
|
||||
### 3.【HIGH】权限审批**吞掉错误** ⇒ 失败的"同意"看起来像成功了
|
||||
|
||||
`MailView.tsx:746-761`：
|
||||
```ts
|
||||
} catch (err) {
|
||||
console.error(err); // ← 只进控制台
|
||||
} finally { setBusy(false); }
|
||||
```
|
||||
|
||||
`setDecided(...)`（`:753`）在 `await api.decidePermission` **之后**。
|
||||
⇒ 请求失败时 `decided` 仍是 `''`，界面回到可点状态，**用户看不到任何提示**，
|
||||
只会以为"点了没反应"而反复点，而 Agent 那边一直阻塞。
|
||||
|
||||
同文件其它提交路径（`ForwardBar.submit:401`、`ReplyBar.send:1046`、`BudgetEditor.commit:252`）
|
||||
都正确地 `setError` 了 —— 只有这一处是例外。
|
||||
|
||||
**修法**：`catch` 里 `setError(...)` 并把错误渲染出来。
|
||||
|
||||
### 4.【HIGH】转发有双发窗口，且"关窗"依赖可能失败的刷新
|
||||
|
||||
`MailView.tsx:390-408`：
|
||||
```ts
|
||||
if (!to.trim() || busy) return;
|
||||
setBusy(true);
|
||||
...
|
||||
await api.forwardMail(...);
|
||||
await Promise.all([fetchInbox('all'), fetchSent(), fetchSessions(), fetchContacts()]);
|
||||
onClose();
|
||||
```
|
||||
|
||||
- `busy` 守卫要等 React 提交 `setBusy(true)` 之后才生效 ⇒ **快速双击会发出两个
|
||||
`forwardMail`**（真转发，不是本地乐观更新）
|
||||
- 四个刷新里任一 reject ⇒ `onClose()` 被跳过，**尽管转发已经成功** ⇒
|
||||
用户看到报错、于是**再转一次**
|
||||
|
||||
**修法**：`setBusy` 之前用同步 ref 置位；`onClose()` 挪进 `finally` 或改成
|
||||
`Promise.allSettled`。
|
||||
|
||||
### 5.【HIGH】ICS 导出的 object URL **同步 revoke** ⇒ 文件可能 0 字节
|
||||
|
||||
`CalendarView.tsx:262-267`：
|
||||
```ts
|
||||
const url = URL.createObjectURL(new Blob([ics], ...));
|
||||
const a = document.createElement('a');
|
||||
a.href = url; a.download = `...ics`;
|
||||
a.click();
|
||||
URL.revokeObjectURL(url); // ← 同一同步块内立即撤销
|
||||
```
|
||||
|
||||
`a` 也从未插入 document。Firefox / WebKit 的下载是**异步**取 blob 的 ⇒
|
||||
同步撤销会导致**静默失败或 0 字节 .ics**（Chromium 通常容忍，故容易漏测）。
|
||||
|
||||
**修法**：`setTimeout(() => URL.revokeObjectURL(url), 0)`，并把 `a` 挂到
|
||||
document 后再 `remove()`。
|
||||
|
||||
### 6.【MEDIUM】日历加载无请求代次守卫 ⇒ 快速翻月时"标题与内容错配"
|
||||
|
||||
`CalendarView.tsx:101-106`：
|
||||
```ts
|
||||
useEffect(() => { setLoading(true); load(); }, [range.from, range.to]);
|
||||
```
|
||||
`load()` 里无条件 `setEvents(r.events ?? [])`。
|
||||
快速点「下一页/上一页」⇒ 两个请求同时在飞，**后落地的覆盖先落地的** ⇒
|
||||
标题显示 11 月而网格是 10 月的事件。
|
||||
|
||||
对比：`ThreadView.tsx:66-85` 用 `gen.current` / `myGen` **做对了**，
|
||||
`CalendarView` 没有对应实现。
|
||||
|
||||
### 7.【MEDIUM】三个 `setTimeout` 无卸载清理
|
||||
|
||||
`ComposePage.tsx:160-163`（900ms 后清提示并关窗）、`AdminUsersPage.tsx:93-96`、
|
||||
`KeyPanel.tsx:37-40`。`ComposePage` 那条可达（清空按钮在 `sending` 期间未禁用）。
|
||||
|
||||
### 8.【MEDIUM】预设缩略图全部渲染同一张自定义图
|
||||
|
||||
`BackgroundPicker.tsx:120-122`：
|
||||
```tsx
|
||||
className={`bg-preset-${p.id} absolute inset-0 block`}
|
||||
style={{ backgroundImage: 'var(--bg-image)', backgroundSize: 'cover' }}
|
||||
```
|
||||
当 `kind==='image'` 时 `--bg-image` 有值 ⇒ **六个预设格子都显示同一张照片**，
|
||||
而用户正在这一步里挑选预设。
|
||||
|
||||
### 9.【LOW】`api.attachmentURL` 把 token 放进 query
|
||||
|
||||
`api/client.ts:385-387` + `:118-122`。服务端**有意支持**
|
||||
（`main.go:299` 用 `middleware.UserAuthAllowQueryToken`，`handler/events.go:17` 注释说明
|
||||
这是浏览器 `EventSource` 专用回退）⇒ **不是缺陷**。
|
||||
但代价是 token 会进浏览器历史/服务端访问日志，是已知的取舍，记录备查。
|
||||
（鸿蒙侧 `ApiClient.getBytes` 的注释明确写了"不用 `?token=`，那会把密钥写进日志" ——
|
||||
**两端做法不一致**，值得统一口径。）
|
||||
|
||||
---
|
||||
|
||||
## 三、本轮明确排除的疑点
|
||||
|
||||
- **推送 SQL 注入**：`repo/push_tokens.go` 的 `IN (...)` 占位符由序号生成、值全走绑定，
|
||||
且两种方言都支持 `$N` ⇒ 安全。
|
||||
- **Go 推送测试**：23 个测试覆盖了形状、分批、限额、无效 token 清理、配置容错 ——
|
||||
覆盖面是好的；`TestHMSDailyLimitStopsSending` 用例也说明"按 token 计"是**已固化的行为**，
|
||||
但**没人质疑这个计数单位**（见缺陷 1）。
|
||||
- **`resetUI()` 登出不清数据 store**：单独看不可达（无账号时 `AccountSwitcher` 渲染 `null`），
|
||||
真正的泄露面是**切账号**路径（缺陷 1）。不算独立缺陷。
|
||||
- **Electron `MonthGrid key={i}`**：父级 `key={${anchor.getTime()}-${scale}}` 使其
|
||||
每次导航都重挂载，索引不会跨数据变化 ⇒ 安全。
|
||||
- **`ContactPanel` 双按钮**：非嵌套可交互元素，无点击吞没。
|
||||
- **GUI 监听器/定时器清理**：`LoginPage.tsx:43-55`、`AddressInput.tsx:195-199`
|
||||
（`{capture:true}` 成对）、`backgroundStore.ts:379-393`（两条路径都 revoke）⇒ 正确。
|
||||
|
||||
---
|
||||
|
||||
## 四、修复顺序建议
|
||||
|
||||
| 序 | 项 | 位置 | 理由 |
|
||||
|---|---|---|---|
|
||||
| 1 | 切账号清 store | `AccountSwitcher.tsx:56-62` | 唯一的**跨账号数据泄露**，改动最小 |
|
||||
| 2 | 壁纸按账号分 | `appearanceSync.ts:40` | 静默丢数据 + 谎报"已同步" |
|
||||
| 3 | 审批错误提示 | `MailView.tsx:756-757` | 权限决策失败却无反馈，Agent 一直阻塞 |
|
||||
| 4 | 每日额度计数单位 | `hms.go:192` | 用户可感知的功能失效（提前耗尽配额） |
|
||||
| 5 | HMS `click_action` + 真机实测 | `hms.go:206-217` | 需真机 token 才能关闭这个未知项 |
|
||||
| 6 | ICS revoke 时机 | `CalendarView.tsx:267` | 导出功能在部分内核上失效 |
|
||||
| 7 | 转发双发 / 关窗 | `MailView.tsx:390-408` | 数据重复 |
|
||||
| 8 | 锁屏文案 | `hms.go:211` + `push.go:36-38` | 注释与实现不一致（二选一，别留现状） |
|
||||
| 9 | 日历代次守卫 | `CalendarView.tsx:101` | 照抄 `ThreadView.tsx:66-85` 即可 |
|
||||
| 10 | provider 硬编码 | `PushContract.ts:13` | 影响"配置式多厂商"这个卖点 |
|
||||
Reference in New Issue
Block a user