MailUI4Agents/docs/reviews/harmony-pages-review.md

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.

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.

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.

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.

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.

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 trues (: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.

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.

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 ''.

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.

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.

@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.

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.

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.

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.

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.

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.

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.

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.

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.

// 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.

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.

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().

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.