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

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 awaited 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 awaited, 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).