plan: refine (round 2)

Can you write human-readable report? Like simplex, structured (TOC), withlou LLM/AI patterns + BRIEF, but cover the issue. Structure should be like "## Issue 1: blablabla" then descripttion and then "### Fix"

plans/sh-testing-design.md

@@ -1,229 +1,141 @@
-# Performance audit of the Android/desktop Kotlin code — impact-ranked findings
+# Brief: Performance review report for the multiplatform Kotlin clients
 
 ## Goal
-Deliver a performance audit of the shared multiplatform Kotlin used by the
-Android and desktop apps (`apps/multiplatform/common`). The deliverable is a
-list of concrete findings, **categorized by impact with the most severe at
-the top**, plus a remediation plan for the top findings. Each finding is
-grounded in the repository: it names the `file:line`, the cost mechanism
-(what the code does per invocation), and the hot path (how often that
-invocation runs). This revision investigates further than a loose prior audit
-and organizes the results into ranked severity tiers.
+Produce one human-readable Markdown report that documents the performance
+problems found in the SimpleX Chat Android/Desktop Kotlin code
+(`apps/multiplatform/`) and states a concrete fix for each. The report is the
+deliverable; it changes no application code.
 
-The goal is the audit itself and a plan to act on it. Specific remediation
-techniques belong to the plan, not to this brief; where this brief names a fix
-it is illustrative, not a commitment.
+## Origin
+The request has two parts:
+1. Review the Kotlin client code and determine whether performance issues exist.
+2. Write the result as a structured, human-readable report — styled like the
+   project's own docs (table of contents, plain technical prose), with a BRIEF
+   at the top and one section per issue in the form `## Issue N: <title>`,
+   followed by a description and a `### Fix` subsection.
 
-## Severity model
-Severity is **analytical, not profiled**. Impact is estimated as
-`per-invocation cost × invocation frequency`. Findings on the per-message and
-per-scroll-frame paths, and a crash-risk regression, rank highest; findings
-gated behind rare user actions rank lowest. This ranking is a hypothesis to be
-confirmed by profiling (see success criteria) before large remediation effort
-is committed in the ordered plan.
+The review has already been carried out against the checked-out worktree; the
+findings below are grounded in specific files and line numbers. This brief and
+the plan define how those findings become the report.
 
-## Deliverable: ranked findings
+## Deliverable
+A single Markdown file, `apps/multiplatform/PERFORMANCE_REPORT.md`, containing:
+- A short **Brief** (scope, what was reviewed, method, how severities are used).
+- A **Table of contents** linking to every issue.
+- One `## Issue N: <title>` section per finding, each with a description and a
+  `### Fix`.
+- A closing **Not defects / verified-good patterns** section that records the
+  places already doing the right thing (stable `LazyColumn` keys, off-main JSON
+  decode, existing caches and index maps), so the report is balanced and does
+  not imply the whole codebase is slow.
 
-### Tier A — Critical (crash risk, or dominant per-message / per-frame cost)
+## Audience
+SimpleX maintainers familiar with the codebase. Assume they read Kotlin and
+Compose; do not explain language basics. Every claim must point at a concrete
+`file:line` in `apps/multiplatform/`.
 
-- **F1 — Unguarded index into the reversed item list can crash
-  (regression vs iOS).** `getPrevHiddenMember`
-  (`common/.../model/ChatModel.kt:1163-1177`) loops `for (i in range)` and reads
-  `reversedChatItems[i].chatDir` at `:1167` with no bounds check. It is called
-  from `MemberNameAndRole` at `common/.../views/chat/ChatView.kt:2028`, passing a
-  freshly read `reversedChatItems.value`. `range` comes from a `MutableStateFlow`
-  populated at merge time (`ChatItemsMerger.kt`); `mergedItems` is a
-  `derivedStateOf` (`ChatView.kt:1795`). If `chatItems` shrinks (trim, delete,
-  chat switch) between the merge computation and this read, `range` can exceed
-  `reversedChatItems.lastIndex` → `IndexOutOfBoundsException`. The Swift port
-  guards the identical loop (`apps/ios/Shared/Model/ChatModel.swift:1222-1230`
-  logs and skips when `i >= items.count`); the Kotlin port does not. Hot path:
-  every grouped `GroupRcv` item that shows a member name, on every recomposition.
+## Content and voice
+- Structure exactly as requested: `## Issue N: ...` → description → `### Fix`.
+- Each issue states: what the code does, the file:line evidence (with a short
+  excerpt where it clarifies), why it costs time or memory (name the complexity,
+  e.g. O(n) per add / O(n^2) per batch, or "runs on the UI thread"), when it is
+  triggered (scroll, incoming message batch, recomposition), and its severity.
+- The fix is specific and grounded in this repo — prefer pointing at patterns the
+  codebase already uses (`groupMembersIndexes` id->index map at
+  `ChatModel.kt:139`; the async `Base64AsyncImage` at `Images.kt:36`; the desktop
+  `loadedImageCache` / `base64BitmapCache`; the `mapItemsToIds` Set in
+  `ChatItemsLoader.kt`).
+- Plain and formal. No LLM/AI tells: no "As an AI", no hedging ("it seems",
+  "might possibly"), no marketing adjectives, no emoji, no per-section filler.
+  Write as an engineer filing findings.
+- Severity labels: High (blocks the UI thread or scales O(n^2) on common paths),
+  Medium (per-event or per-recomposition waste on hot paths), Low (bounded or
+  startup-only).
 
-- **F2 — `mergedItems` derived state rebuilds the whole merged list on every
-  message and every reveal.** `ChatView.kt:1795-1799` wraps
-  `MergedItems.create(chatItems.value.asReversed(), unreadCount, revealedItems, chatState)`
-  in `derivedStateOf`. `MergedItems.create` (`ChatItemsMerger.kt:17-108`) is a
-  full O(n) pass over all loaded items building `ArrayList`, an
-  `indexInParentItems` map, split ranges, and per-group `MutableStateFlow`
-  allocations. It recomputes whenever `chatItems`, `unreadCount`, or
-  `revealedItems` change — i.e. on every new/edited/deleted message and every
-  reveal toggle. This is the dominant per-update cost feeding the `LazyColumn`.
-  (`reversedChatItems` at `ChatView.kt:1800` is a cheap O(1) view, not a copy.)
+## Issues to cover (grounded findings)
+Data model / collections (`model/ChatModel.kt`, `views/chat/ChatItemsLoader.kt`):
+- `addToChatItems` allocates a new `SnapshotStateList` and copies the whole list
+  on every single insert — O(n) per add, O(n^2) for the per-item batch loop in
+  `CR.NewChatItems` (`ChatModel.kt:500-506`; `SimpleXAPI.kt:2899-2928`).
+- `addChatItem` dedupes with `none { it.id == ... }` — O(n) scan per add before
+  the O(n) copy (`ChatModel.kt:582-590`).
+- `upsertChatItem` / `updateChatItem` locate items with
+  `indexOfFirst { it.id == ... }` — O(n) per update, O(m*n) per status/reaction
+  batch (`ChatModel.kt:638-639, 661-662`).
+- Chat lookup by id (`getChatIndex` / `getChat` / `hasChat`) is a linear scan
+  called per incoming item (`ChatModel.kt:377-379`, used at 460, 533, 615, 677 …).
+- `getChatItemIndexOrNull` linear scan is called from render-time composables
+  (`ChatModel.kt:1122-1145`; callers `ChatItemView.kt:887`, `ChatView.kt:3333`,
+  `CIChatFeatureView.kt:79`, `MarkedDeletedItemView.kt:46`).
+- `upsertGroupMember` maps the entire `chatItems` list and does an O(n) equality
+  check on every member update (`ChatModel.kt:941-955`).
+- Pagination duplicate-removal uses `List.indexOf`/`contains` on `splits` inside
+  per-item `removeAll` predicates — O(items*splits)
+  (`ChatItemsLoader.kt:196-, 341-364`).
 
-- **F3 — Chats have no id→index map; every mutation does an O(n) scan.** Chats
-  are a flat `SnapshotStateList`; lookups are linear `firstOrNull`/`indexOfFirst`:
-  `getChatIndex` (`ChatModel.kt:379`, the hot one, keyed on `id + remoteHostId`),
-  `hasChat` (`:377`), `getChat` (`:344`, `:378`), `getContactChat` (`:345`),
-  `getGroupChat` (`:346`). `getChatIndex` runs on essentially every mutation:
-  `addChatItem` (`:533`), `upsertChatItem` (`:615`), `removeChatItem` (`:677`),
-  `markChatItemsRead` (`:836`) — once per received/sent/updated message. Group
-  members already have an O(1) index map (`getGroupMember`, `:357-364`); chats do
-  not. Note: item scans are keyed by `id` only (`:584`, `:639`, `:662`, `:1123`);
-  the remote-host dimension is carried at the chat level, not the item level.
+Compose recomposition (`views/chatlist/*`, `views/chat/*`):
+- `ChatList` re-runs `filteredChats(... allChats.value.toList() ...)` inline on
+  every recomposition — full copy + full filter, not memoized
+  (`ChatListView.kt:936-943`, `1443-1471`).
+- The per-row `remember(chat.id, chats)` is keyed on the freshly allocated
+  filtered list, so it is invalidated for every visible row every pass
+  (`ChatListView.kt:1001-1006`).
+- Whole-list reassignment: `replaceAll` and `reorderChat` throw away and rebuild
+  the `SnapshotStateList`, invalidating all readers (`ChatModel.kt:3526-3528`,
+  `406-413`).
+- Unstable `selectChatItem` lambda allocated per item makes `ChatItemView`
+  non-skippable (`ChatView.kt:1950`).
+- `EventItemView` reads `chatsCtx.chatItems.value` and reverses/scans the list in
+  composition, recomputing text unmemoized (`ChatItemView.kt:605-643`).
+- Chat-list rows read model-wide `deletedChats`/`chatRunning` in the row body, so
+  one deletion recomposes every row (`ChatListNavLinkView.kt:42`).
+- Toolbar rebuilds a mutable list of `@Composable` lambdas each recomposition
+  (`ChatListView.kt:483-568`).
 
-### Tier B — High (per-message list copies, per-row composition cost)
+Main-thread blocking (`views/chat/ChatView.kt`, helpers, platform):
+- `runBlocking { getLoadedImage(...) }` decrypts + decodes a full image on the
+  composition thread in the fullscreen gallery — High
+  (`ChatView.kt:3599`; `getMedia` at `ImageFullScreenView.kt:49, 97`).
+- `Thread.sleep(10)` busy-wait in `ModalManager.runAtomically` on the UI thread
+  (`ModalView.kt:241-247`).
+- `runBlocking { progressJob?.cancelAndJoin() }` in audio recorder `stop()`
+  (`RecAndPlay.android.kt:82-84`, `RecAndPlay.desktop.kt:80`).
+- `Thread.sleep(50)` file-existence polling at desktop startup — Low
+  (`SingleInstance.kt:50-53`).
 
-- **F4 — Un-remembered O(n) scans inside composable bodies, per item, per
-  recomposition.** Three item renderers each do
-  `chatItems.value.asReversed()` + `indexOfFirst` + a `while` loop with no
-  `remember`: `EventItemView` (`item/ChatItemView.kt:640-643` → `getConnectedMemberNames`,
-  `ChatModel.kt:1122-1123`), `mergedFeatures` (`item/CIChatFeatureView.kt:74-91`),
-  and `MergedMarkedDeletedText` (`item/MarkedDeletedItemView.kt:43-67`). The
-  `asReversed()` view is cheap; the O(n) `getChatItemIndexOrNull` scan and loop
-  are not, and they rerun on every recomposition of each matching item.
-
-- **F5 — `filteredChats` copies and filters the whole chat list on every
-  ChatList recomposition.** `views/chatlist/ChatListView.kt:943` calls
-  `filteredChats(..., allChats.value.toList(), ...)`; the impl
-  (`ChatListView.kt:1443-1471`) copies the entire list via `.toList()`, then
-  `.filter`s it, and when searching does per-chat `anyNameContains`/`lowercase().contains`.
-  A comment at `:937-939` records that a `derivedStateOf` memoization was removed
-  because it threw `IndexOutOfBoundsException`, so this now recomputes
-  unconditionally, including during scroll (ChatList recomposes on scroll-direction
-  and keyboard state).
-
-- **F6 — List-mutation helpers rebuild a full copy of the backing list on every
-  mutation.** These helpers allocate a fresh `SnapshotStateList` and copy every
-  element on each call: `add`/`add(index)` (`ChatModel.kt:3475-3481`), `addAll`
-  variants (`:3486-3492`), `removeAll` (`:3494-3496`), `removeAllAndNotify`
-  (`:3500-3516`, copy **and** full scan), `removeAt` (`:3518-3524`), `replaceAll`
-  (`:3526-3528`), plus `addToChatItems` (`:500-506`), `removeLastChatItems`
-  (`:508-520`), and `reorderChat` (`:406-413`, rebuilds `newChats` on every sent
-  item on desktop). Each runs on the per-message path. **Correction from prior
-  revision:** `clear` (`:3530-3532`) and `clearAndNotify` (`:3536-3540`) are **not**
-  in this set — both assign a fresh empty `SnapshotStateList()`, which is O(1), not
-  a copy; they must not be remediated as copy-on-write.
-
-- **F7 — Chat-list avatars and previews decode base64 images during composition
-  without `remember`.** `ProfileImage` decodes at
-  `views/helpers/ChatInfoImage.kt:111` (`base64ToBitmap(image)`, synchronous
-  `async=false` branch, no `remember`), reached from every row via
-  `views/chatlist/ChatPreviewView.kt:400`. Link/chat previews decode inline at
-  `ChatPreviewView.kt:320` and `:357`. These re-decode on every recomposition of
-  the row (scroll, unread-count change, selection, draft). By contrast the chat
-  view memoizes the same call (`item/CIImageView.kt:45`
-  `remember(image){ base64ToBitmap(image) }`); the list call sites do not.
-  `ProfileImage` also has an `async=true`/IO path (`ChatInfoImage.kt:103-109`)
-  that the list does not use. (Per-decode cost is F13.)
-
-### Tier C — Medium (per-row composition cost, throttled full rebuilds, per-frame flows)
-
-- **F8 — `MarkdownText` rebuilds its `AnnotatedString` on every recomposition.**
-  `views/chat/item/TextItemView.kt:200-211` (plain branch) and `:224-351`
-  (formatted branch, a full loop over `formattedText.withIndex()` with per-segment
-  `withStyle`/`withAnnotation`) are not memoized. Called once per visible chat-list
-  row for the last message and draft (`ChatPreviewView.kt:233`, `:277`). Markdown
-  is not re-parsed (the parse is a serialized core field, `ChatModel.kt:3106`), but
-  the annotated-string build is redone every pass.
-
-- **F9 — Per-item date formatting and object allocation inside the
-  `itemsIndexed` body.** `ChatView.kt:2416`/`:2420` call `getItemSeparation`
-  (`:3737-3754`), which calls `getTimestampDateText` twice (`:3752`) and allocates
-  a fresh `ItemSeparation` per item per pass; the body also allocates an
-  `ItemContext` per item at `:2423`. Runs for every non-banner item on every list
-  recomposition.
-
-- **F10 — `getTimestampText` recomputed per chat-list row, per recomposition.**
-  `ChatPreviewView.kt:412` calls it un-remembered; the impl
-  (`ChatModel.kt:3682-3711`) allocates `TimeZone.currentSystemDefault()`,
-  `Clock.System.now()`, `Locale.getDefault().country`, and a `DateTimeFormatter`
-  on each call. A precomputed `meta.timestampText` exists (`ChatModel.kt:3113`) but
-  the list uses the live recompute instead.
-
-- **F11 — The throttled `popCollectedChats` rebuilds the entire chat list.**
-  `PopChatCollector` (`ChatModel.kt:785-831`) throttles at 2 s
-  (`throttleLatest(2000)`), then on the main thread rebuilds the list. The
-  `withContext(Dispatchers.Main)` wrapper opens at `ChatModel.kt:795` and the
-  `chats.replaceAll(popCollectedChats())` call is at `:796`. `popCollectedChats`
-  (`:815-830`) does an O(n) `getChat` per collected entry (`:819`), a sort (`:826`),
-  and an O(n) `filter` (`:827`), then `replaceAll` copies the whole list. Fed by
-  `throttlePopChat` from `addChat` (`:403`), `reorderChat` (`:412`), and
-  `addChatItem` (`:571`) — i.e. per message, coalesced by the throttle.
-
-- **F12 — Scroll `snapshotFlow` collectors fire per frame and do date
-  formatting / visible-item loops.** `ChatView.kt:2905-2921` collects
-  `layoutInfo.visibleItemsInfo` (emits per scroll frame, loops over visible items);
-  `ChatView.kt:2937-2945` collects `firstVisibleItemScrollOffset` (emits per scroll
-  pixel) and each emission runs `setDateVisibility` with a double `getTimestampDateText`.
-
-### Tier D — Low (narrow, rare, or platform-specific minor cost)
-
-- **F13 — base64 image decode cost, split by platform.**
-  - **Android (`platform/Images.android.kt:26-42`)** decodes **twice**: a bounds
-    pass with `inJustDecodeBounds=true` (`:32-33`) followed by a full
-    `BitmapFactory.decodeByteArray` (`:37`), and there is **no cache**. This is the
-    material cost, made worse by F7's un-memoized call sites.
-  - **Desktop (`platform/Images.desktop.kt:30-55`)** is materially cheaper: it
-    reads dimensions from metadata via `reader.getWidth(0)`/`getHeight(0)`
-    (`:40-41`) and then performs a **single** full `reader.read(0)` (`:46`) — there
-    is no second full decode. It also already has an LRU `base64BitmapCache`
-    (`:24`, checked `:31`, populated `:49`) that Android lacks. **Correction from
-    prior revision:** the "decode twice" premise applies to Android only.
-
-- **F14 — Group-member index maintenance does redundant work and full item
-  copies.** `populateGroupMembersIndexes` (`ChatModel.kt:348-355`) writes
-  `groupMembersIndexes.value = emptyMap()` (`:349`), then immediately `.toMutableMap()`
-  of that empty map (`:350`), rebuilds via `forEachIndexed` (`:351-353`), and
-  reassigns (`:354`) — the `:349` write is wasted. `upsertGroupMember` copies the
-  entire `chatItems` list via `.map` on every matching member update
-  (`ChatModel.kt:941-954`) plus a `groupMembers.toMutableList()` copy (`:956`).
-
-- **F15 — Minor chat-list and settings allocations.** `nextChatSelected`
-  (`ChatListView.kt:1002`) keys a `remember { derivedStateOf { … } }` on the whole
-  freshly built `chats` list, so it invalidates per row on every recomposition;
-  a per-visible-item `snapshotFlow { highlighted.value }` collector is allocated in
-  `ChatItemViewShortHand` (`ChatView.kt:1941-1948`, has `distinctUntilChanged`);
-  and two settings screens construct `Regex` inside functions
-  (`NetworkAndServers.kt:659`, `OperatorView.kt:871`) rather than at module level
-  (contrast the correctly hoisted `ChatListView.kt:1047`).
+Image / media decoding and memory:
+- Chat-list previews call `base64ToBitmap` in the composable body with no
+  `remember`, re-decoding on every recomposition (`ChatPreviewView.kt:320, 357`).
+- Media items decode the base64 preview synchronously during composition
+  (`CIImageView.kt:45`, `CIVideoView.kt:42`, `ImageFullScreenView.kt:157`) while
+  the async `Base64AsyncImage` (`Images.kt:36`) exists and is unused.
+- Android `base64ToBitmap` has no cache and decodes the byte array twice at full
+  resolution, never setting `inSampleSize` (`Images.android.kt:26-42`), unlike the
+  desktop cache (`Images.desktop.kt:24-50`).
+- Android `getLoadedImage` has no cache and re-reads the whole file with
+  `readBytes()` on every call (`Utils.android.kt:172-194`), unlike the desktop
+  `loadedImageCache` (`Utils.desktop.kt:128-146`).
+- Full undecoded file bytes are retained per visible media item and passed to
+  Coil with `Size.ORIGINAL` (`CIImageView.kt:167-213`, `CIImageView.android.kt:30-34`).
+- Desktop `resizeImageToStrSize` recomputes `hasAlpha()` (full pixel scan) on each
+  shrink iteration (`Images.desktop.kt:57-68, 105-115, 143-155`).
 
 ## Success criteria
-Qualitative (deliverable quality):
-- Every finding names an exact `file:line`, its cost mechanism, and its hot path,
-  and is verifiable against the checked-out source.
-- Findings are grouped into severity tiers, most severe first, with the ranking
-  rationale (`cost × frequency`) stated.
-- The F6 scope excludes `clear`/`clearAndNotify`; F13 is split per platform with
-  the desktop description corrected.
-
-Quantitative (to confirm the ranking and any remediation, via profiling, not
-assumed):
-- Capture a baseline with Android Studio / Compose layout-inspection recomposition
-  counts and a method trace before changing anything, on named scenarios:
-  (a) receiving messages into an open large chat (~5k items), (b) scrolling that
-  chat, (c) scrolling a chat list of ~500 chats, (d) a group with ~500 members.
-- For any finding acted on, show a measurable reduction on its scenario
-  (recomposition count and/or frame time / allocation) versus the baseline, with
-  no behavioural regression.
+- The report file exists at the path above and follows the requested structure
+  (Brief, TOC, `## Issue N` / description / `### Fix`).
+- Every issue above is covered, each with a real `file:line`, a stated cost, a
+  trigger, a severity, and a concrete fix.
+- Issues are ordered by severity/impact and grouped by theme in the TOC.
+- The "verified-good patterns" section is present.
+- The prose is plain and free of LLM/AI tells; no application code is modified.
 
 ## Edge cases and constraints
-- **List-identity contract (`ChatModel.kt:369-372`).** The `chatItems` list must
-  not be mutated in place except by index; add/remove must go through the notify
-  helpers so `ChatView`'s `LazyColumn` and `chatState` stay correct. Any F2/F3/F6
-  remediation must preserve this contract and the `chatState.itemsRemoved`/`clear`
-  notifications.
-- **IOOBE regression (F1).** The fix must guard the index like iOS
-  (`ChatModel.swift:1222-1230`) and cover the case where the list shrinks between
-  merge and access; do not merely reduce the window.
-- **Stale-data / shrinking-list guard.** F5's removed memoization was dropped for
-  an `IOOBE`; any re-introduction must handle concurrent list mutation.
-- **Reversed message list.** Items are rendered newest-first via an `asReversed()`
-  view; indexing and range math operate in reversed coordinates — remediation must
-  keep that convention.
-- **Remote-host keying.** Chats are keyed by `id + remoteHostId`; any chat index
-  map must key on both, or multi-remote-host state will collide.
-- **Dispatcher precision.** Model mutations run on `Dispatchers.Main`
-  (`ChatModel.kt:795`); heavy work must move to `Default`/`IO` without moving state
-  writes off Main.
-- **Platform divergence.** Android and desktop have separate `Images` actuals with
-  different cost and caching; each fix must be applied to the correct platform
-  (F13).
-
-## Out of scope
-- The iOS/Swift app and the Haskell core (referenced only as the correctness
-  baseline for F1 and for where parsing already happens).
-- Functional changes to chat behaviour, protocol, or persistence.
-- Rewriting the storage model beyond what a finding requires.
-- Committing a fixed remediation order before profiling confirms the analytical
-  ranking.
+- Do not assert unverified claims. Findings the review could not confirm at the
+  call site (WebRTC signaling decode in `CallView.*:786/290`, the `rememberSaveable`
+  Saver decode in `ComposeView.kt:235`) are excluded, not stated as defects.
+- Distinguish Android-only from desktop-only from shared issues; several problems
+  are platform-specific (the caches exist on desktop but not Android).
+- Keep the report descriptive: it recommends fixes but must not edit code, and
+  must not overstate — note where a hot path is already throttled (e.g.
+  `PopChatCollector`) or already optimized (Set-based dedupe, stable keys).
+- Length follows coverage; do not pad. One tight section per issue.