plan: refine (round 1)

Deeper research, plan is short and not detailed.

plans/sh-testing-design.md

@@ -1,36 +1,66 @@
-# Performance audit — SimpleX Chat Kotlin (Android / desktop, `apps/multiplatform`)
+# Product brief: Kotlin (Android/Desktop) performance audit and remediation
 
 ## Goal
-Answer the request "check the Kotlin code (android/desktop), determine if there are performance issues" and turn the answer into an actionable remediation plan. The audit covered the Compose Multiplatform module under `apps/multiplatform/common` across five areas: message-list rendering, chat-list rendering, coroutine/main-thread usage, image/video/file media, and the in-memory state model. The PR head diff itself only touches a build script (`scripts/simplex-chat-reproduce-builds-android.sh`); this work is a codebase-wide performance review, not a review of that diff.
+Review the Compose Multiplatform Kotlin code under `apps/multiplatform` (shared `commonMain`, plus `androidMain`/`desktopMain`) and remove the concrete performance problems it contains. The two costs that matter to users are (1) UI jank — dropped frames while scrolling the chat list and an open chat, and (2) CPU/battery/memory spent per received or sent message. This brief records what the audit found and defines the target state; the plan lists the ordered fixes.
 
-## Verdict
-Yes — there are real performance issues. The codebase is broadly well-engineered (extensive, correct use of `remember` / `derivedStateOf`; JSON decoding of the high-volume receive loop is correctly off-main on `Dispatchers.IO`; the merge algorithm `MergedItems.create` is a single O(n) pass). But concrete, verified problems exist in three classes, roughly in order of user impact:
+## Who is affected and when
+- Users in large groups or long chats (hundreds–thousands of loaded items), where per-message work is O(n) and repeats.
+- Users scrolling the chat list with many chats, or a chat with mixed content (text, images, video, voice, calls, group events).
+- Android users specifically for image previews (Android lacks caches that Desktop already has).
+- All users on every sent/received message (JSON encode/decode and terminal-log overhead).
 
-1. **Per-frame recomposition cost on the hot scroll paths** (message list and chat list) — un-memoized work that re-runs on every recomposition of every visible row.
-2. **Per-event O(n) work in the state model** that scales with the number of loaded items and with group activity, executed on the main thread and often duplicated across primary/secondary chat contexts.
-3. **Media memory and lifecycle** — full-resolution bytes retained per visible image, inline video (ExoPlayer) instances that are stopped but not released while a chat stays open, and an Android base64→bitmap path with no cache and no downsampling.
+## What the audit found (grounded in the code)
 
-A smaller fourth class is **synchronous main-thread blocking** (`runBlocking` in the image-gallery provider and the recorder-stop path; a `Thread.sleep` spin-lock in modal navigation).
+### A. Message-list state and merging — highest impact
+- `ChatModel.chatItems` and `ChatModel.chats` are `mutableStateOf(SnapshotStateList<…>)`; nearly every mutator (`addToChatItems`, `add/addAll/replaceAll/removeAt/removeAll`, `ChatModel.kt:500-506,3475-3540`) allocates a **new** `SnapshotStateList`, copies every element, and reassigns `.value`. Each message is O(n) copy; a burst of n messages is O(n²), and each reassignment invalidates every reader of the state, not just the changed row.
+- No id→index map for `chats` or `chatItems`; inserts/updates do linear `indexOfFirst`/`none` scans on every event (`ChatModel.kt:379,584,639,662`). Group members already have such a map (`groupMembersIndexes`), proving the pattern is available.
+- `MergedItems.create` (`ChatItemsMerger.kt:17-108`, driven by the `derivedStateOf` at `ChatView.kt:1795`) rebuilds the entire grouped/split model with three fresh allocations on **every** new message, reveal/collapse, or unread-count change. `itemSplits.contains(item.id)` (`ChatItemsMerger.kt:39`) is a linear `List` scan per item.
+- Pagination replaces the whole backing list on `Dispatchers.Main` (`ChatItemsLoader.kt:97,116,138,161`), forcing a full merge rebuild per "load more", and uses a `SnapshotStateList` as a scratch buffer (snapshot-record overhead for throwaway work, `ChatItemsLoader.kt:54`).
+- `upsertGroupMember` maps + element-wise `!=` compares + `replaceAll`s the whole item list on any member/connection-stat change, on Main (`ChatModel.kt:941-955`).
 
-## User-visible behaviour to improve
-- **Scrolling the message list** in long or media-heavy chats: dropped frames from re-building each text bubble's `AnnotatedString`, un-memoized per-item geometry, and lack of LazyColumn item recycling by content type.
-- **Scrolling / updating the chat list** with many chats: each recomposition copies and re-filters the entire chat list on the UI thread, and per-row memoization is defeated.
-- **Opening the image gallery** and swiping between images: the provider decrypts/decodes on the calling thread via `runBlocking`.
-- **Active groups and busy chats**: each incoming message, member update, or file-progress tick rebuilds whole lists and does linear scans, causing rising CPU and jank as more items load.
-- **Memory pressure** while scrolling image/video-heavy chats, risking `OutOfMemory` and background eviction.
+### B. Compose recomposition in list rows
+- Neither LazyColumn sets `contentType`: chat list (`ChatListView.kt:1001`) and message list (`ChatView.kt:2381`). Keys are stable, but heterogeneous row layouts share one reuse pool, defeating slot reuse while scrolling.
+- Timestamp/date strings are recomputed per item per recomposition and each call allocates a `DateTimeFormatter` (`getTimestampText`/`getTimestampDateText`, `ChatModel.kt:3671-3711`; read via `get()` at `ChatModel.kt:3602,3113`; used at `CIMetaView.kt:125,192`, `ChatView.kt:3750-3752`, `ChatPreviewView.kt:412`). `getItemSeparation` formats dates ~4×/item.
+- `MarkdownText` rebuilds its `AnnotatedString` on every recomposition with no `remember` (`TextItemView.kt:200-351`); `reserveSpaceForMeta` concatenates with `+=` and re-reads prefs each recomposition (`CIMetaView.kt:130-195`).
+- `ChatItemView` receives ~35 freshly-allocated lambdas per pass (`ChatView.kt:1950`), so it is unstable and cannot be skipped.
+- Chat list filtering runs on every recomposition and copies the whole list first (`allChats.value.toList()` + `filteredChats`, `ChatListView.kt:943,1443-1471`); a per-item `derivedStateOf` is remembered on a new list instance so it is recreated every recomposition (`ChatListView.kt:1002-1004`); several `.filter{}` passes are unremembered (`ChatListView.kt:582-585,1186-1197`). `EventItemView` scans the full item list per event item (`ChatItemView.kt:605-641`).
+
+### C. Media/images (Android-specific gaps vs Desktop)
+- Android `base64ToBitmap` has no cache; Desktop has a 200-entry cache (`Images.android.kt:26-42` vs `Images.desktop.kt:24-31`) — Android re-decodes previews on every scroll-in.
+- Chat-list previews decode base64 on every recomposition with no `remember` on Android's uncached path (`ChatPreviewView.kt:320,357`).
+- Android `getLoadedImage` has no cache and re-reads (and re-decrypts) the file from disk on every scroll-back (`Utils.android.kt:172-194` vs Desktop's `loadedImageCache`).
+- Android decodes previews at full resolution — measures bounds then discards them instead of using `inSampleSize` (`Images.android.kt:32-37`); the correct downsampling helper already exists (`Utils.android.kt:197-210`).
+- First base64 decode of each image/video item runs synchronously on the composition thread (`CIImageView.kt:45`, `CIVideoView.kt:42`) although an off-thread `Base64AsyncImage` helper exists but is unused. `getMedia()` uses `runBlocking { getLoadedImage(...) }` (`ChatView.kt:3599`).
+
+### D. JSON and threading
+- Every response and event is parsed twice: `APISerializer.deserialize` decodes to a `JsonElement` DOM then re-decodes that tree to `CR` (`SimpleXAPI.kt:6426-6441`); the fallback re-encodes the element to a string (a third pass).
+- The shared `json` has `prettyPrint = true` (`SimpleXAPI.kt:6355`) and is used to **encode outgoing commands** (`SimpleXAPI.kt:3998,4008,…`), so every sent message is pretty-printed before the core re-parses it; a `prettyPrint = false` `jsonShort` already exists but is not used there.
+- `terminalItems.value += item` copies the whole list on every message even when the terminal view is not open (`ChatModel.kt:1228-1232`, driven from `SimpleXAPI.kt:2792,842,854`).
+- The receive loop acquires a wake-lock and launches a new release coroutine per received message (`SimpleXAPI.kt:705-710`); `getUserChatData` runs the potentially large `updateChats` merge on `Dispatchers.Main` (`SimpleXAPI.kt:676-679`). Receive/decode itself is correctly on `Dispatchers.IO` (good).
+
+### E. Tooling gap
+- The Compose compiler plugin is applied but no stability configuration or metrics/reports output is enabled (`common/build.gradle.kts`), so recomposition regressions and unstable model classes are invisible.
+
+## User-visible behaviour after remediation
+- Smooth scrolling in the chat list and in open chats, including chats with mixed media and large groups; no perceptible per-frame stutter attributable to re-decoding images, reformatting timestamps, or rebuilding annotated text.
+- Lower CPU/battery use per received and sent message; incoming message bursts in large groups do not degrade quadratically.
+- Android image-preview scrolling matches Desktop responsiveness (cached decode, downsampled previews, no repeated disk reads).
+- Identical rendered output and behaviour — this is an internal optimization, not a feature or UX change.
 
 ## Success criteria
-- Hot-path composables (`MarkdownText`, per-item geometry, chat-list preview thumbnails) do no unbounded or repeated allocation/parsing per recomposition; expensive results are keyed with `remember` on their real inputs.
-- The message `LazyColumn` provides a `contentType` so heterogeneous item types are recycled.
-- Chat-list filtering is computed once per relevant input change, off the recomposition critical path, without reintroducing the `IndexOutOfBoundsException` that caused `derivedStateOf` to be removed there.
-- Inline list video players are released (not merely stopped) when their item leaves composition; full undecoded image bytes are not retained alongside the decoded bitmap on the scroll path; the Android base64→bitmap path downsamples and/or caches like the desktop path already does.
-- No disk read / decryption / decode runs under `runBlocking` on a UI-driving thread; the modal spin-lock does not block the main thread.
-- Per-event state updates avoid whole-list rebuilds and repeated linear scans where a single pass or index/map lookup suffices; behaviour (recomposition correctness, unread counters, ordering) is unchanged.
-- No regression in message ordering, unread counts, merge/reveal behaviour, search results, or media correctness. Changes are verified with the existing test module (`commonTest`) plus manual profiling of scroll and receive paths on Android and desktop.
+- No behavioural or visual change: existing rendering, ordering, reveal/collapse, read-marking, pagination, and message send/receive all behave exactly as before; existing tests still pass.
+- Per-incoming-message work in an open chat is no longer O(n) in the number of loaded items for the common append case (id lookups O(1); merge updated incrementally or measurably cheaper).
+- Both message-list and chat-list LazyColumns declare `contentType`; scroll recomposition counts drop (verified with Compose compiler metrics / layout inspector).
+- Timestamp/date strings and annotated message text are computed once per item (cached/`remember`ed), not per recomposition.
+- On Android, base64 previews and loaded images are cached and downsampled; scrolling a chat back and forth does not re-decode or re-read from disk.
+- Outgoing command JSON is not pretty-printed; responses are parsed in a single pass; terminal-log growth does not copy the whole list per message when the terminal is closed.
+- Compose compiler metrics/report generation is available for regression tracking.
 
-## Constraints, edge cases, and risks
-- **Intentional patterns must not be broken blindly.** `filteredChats` is deliberately *not* wrapped in `derivedStateOf` (comment at `ChatListView.kt:937-939` records an `IndexOutOfBoundsException`); any caching must use a safe mechanism. The `chatItems` mutation helpers deliberately allocate a new `SnapshotStateList` and reassign the `MutableState` (`ChatModel.kt:500-506`, `3490-3540`); this likely exists to force reliable recomposition, so any change to in-place mutation must preserve observer notification and be validated.
-- **Dual-context amplification.** Most receive handlers mutate both `chatModel.chatsContext` and `secondaryChatsContext`; optimizations must apply to both and preserve the secondary (support/reports) views.
-- **Correctness-sensitive counters.** `ActiveChatState` unread/split bookkeeping is subtle; the per-event scans there are bounded by deletion size and are lower priority — touch only with tests.
-- **Platform split.** Android and desktop have separate `Images`/`VideoPlayer`/`Utils` implementations; the desktop base64 cache already exists, so parity work is Android-side.
-- Changes should be incremental and independently verifiable, prioritized by user-visible impact (rendering first), so each can be profiled and reverted in isolation.
+## Edge cases and risks to preserve
+- `derivedStateOf` was previously removed from chat-list filtering due to an `IndexOutOfBoundsException` (comment at `ChatListView.kt:937-939`); any re-introduction of memoization must not reintroduce index desync between `chats` and per-item `index`.
+- Snapshot semantics: switching a `SnapshotStateList`-reassignment pattern to in-place mutation must keep Compose observing the right granularity — under-invalidation (stale UI) is as harmful as over-invalidation. Reveal/collapse, unread markers, splits, and "scroll to item" rely on current invalidation behaviour.
+- `contentType` values must be coarse enough to enable reuse yet not collapse incompatible layouts (e.g. banner vs message vs date separator).
+- Serialization changes must not alter the exact command strings/whitespace the core expects, nor drop `ignoreUnknownKeys`/coercion behaviour; the double-parse exists to inspect the response shape before decoding — the single-pass replacement must preserve error/unknown handling (`CR.Response`/`CR.Invalid`).
+- Image caches must be memory-bounded (LRU) and invalidated on the existing `clearImageCaches()` hook and on file change/deletion.
+- Off-loading merge/model work off `Dispatchers.Main` must preserve ordering guarantees relative to concurrent events.
+- Platform parity: Android and Desktop paths differ; fixes must not regress the platform that is already correct.