fix(mail): below-cut mail/richtext perf & correctness nits (#298) #435

Merged
JMR-dev merged 1 commits from perf-298-mail-richtext-nits into main 2026-07-08 18:42:55 +00:00
JMR-dev commented 2026-07-08 12:25:16 +00:00 (Migrated from github.com)

Closes #298.

Phase-3 review, below-the-cut findings. Each nit fixed focused and true to surrounding style; every behavioural change ships a unit test.

Per-nit

Nit File Fix Test
Regex recompilation (hot path) HtmlToText.kt Hoist the 4 per-convert() Regex(...) literals to private val (compiled once, not once per fetched body) Covered by existing HtmlToTextTest (all 4 patterns exercised)
Non-atomic report writes ReportStore.kt save/markSurfaced now write a temp file (non-.json suffix) + atomic rename, so a crash mid-write can't leave a torn .json that scan() silently drops ReportStoreTest: no temp lingers after save; a stray .json.tmp is never scanned
Link-overlap un-links remainder RichTextEditing.kt applyLink splits partially-overlapping links (keeps the non-overlapping remainder) via a subtractLinkRange mirroring subtractRange, instead of dropping them whole RichTextEditingTest: updated overlap test + new both-side-split test
Attachment read wholly into memory GraphSender.kt Size guard before readBytes() (per-file cap < Graph's ~4 MB request limit); throws GraphSendException(mayHaveSent=false) so the outbox falls back to SMTP, which streams GraphSenderSendTest: oversized attachment fails safe-to-fall-back, opens no connection
mergeSameValueSpans O(n²) RichText.kt Track the last merged run per style value in a map (O(n) after the sort) instead of indexOfLast; output identical RichTextEditingTest: direct merge-equivalence test
Provider-label substring mislabel DiagnosticsCollector.kt Match brand tokens at DNS-label boundaries (short tokens me/mac/live only as a domain suffix), so mail.notgmail.example no longer reads as Gmail DiagnosticsCollectorTest: label-boundary regression test (all buckets keep working)

Logging

GraphSender (oversized-attachment fallback) and ReportStore (atomic-move-unsupported fallback) log PII-free breadcrumbs via AppLog — no file names, hosts, or content.

Gate

Local fast gate green: assembleDebug, testDebugUnitTest, jacocoTestCoverageVerification (0.84 floor), compileDebugAndroidTestKotlin, lintDebug, ktlintCheck, detekt. No new Compose UI controls or instrumented tests; E2E via CI's matrix.

Not arming auto-merge — Mergify handles the merge.

Closes #298. Phase-3 review, below-the-cut findings. Each nit fixed focused and true to surrounding style; every behavioural change ships a unit test. ## Per-nit | Nit | File | Fix | Test | | --- | --- | --- | --- | | Regex recompilation (hot path) | `HtmlToText.kt` | Hoist the 4 per-`convert()` `Regex(...)` literals to `private val` (compiled once, not once per fetched body) | Covered by existing `HtmlToTextTest` (all 4 patterns exercised) | | Non-atomic report writes | `ReportStore.kt` | `save`/`markSurfaced` now write a temp file (non-`.json` suffix) + atomic rename, so a crash mid-write can't leave a torn `.json` that `scan()` silently drops | `ReportStoreTest`: no temp lingers after save; a stray `.json.tmp` is never scanned | | Link-overlap un-links remainder | `RichTextEditing.kt` | `applyLink` splits partially-overlapping links (keeps the non-overlapping remainder) via a `subtractLinkRange` mirroring `subtractRange`, instead of dropping them whole | `RichTextEditingTest`: updated overlap test + new both-side-split test | | Attachment read wholly into memory | `GraphSender.kt` | Size guard before `readBytes()` (per-file cap < Graph's ~4 MB request limit); throws `GraphSendException(mayHaveSent=false)` so the outbox falls back to SMTP, which streams | `GraphSenderSendTest`: oversized attachment fails safe-to-fall-back, opens no connection | | `mergeSameValueSpans` O(n²) | `RichText.kt` | Track the last merged run per style value in a map (O(n) after the sort) instead of `indexOfLast`; output identical | `RichTextEditingTest`: direct merge-equivalence test | | Provider-label substring mislabel | `DiagnosticsCollector.kt` | Match brand tokens at DNS-label boundaries (short tokens `me`/`mac`/`live` only as a domain suffix), so `mail.notgmail.example` no longer reads as Gmail | `DiagnosticsCollectorTest`: label-boundary regression test (all buckets keep working) | ## Logging `GraphSender` (oversized-attachment fallback) and `ReportStore` (atomic-move-unsupported fallback) log PII-free breadcrumbs via `AppLog` — no file names, hosts, or content. ## Gate Local fast gate green: `assembleDebug`, `testDebugUnitTest`, `jacocoTestCoverageVerification` (0.84 floor), `compileDebugAndroidTestKotlin`, `lintDebug`, `ktlintCheck`, `detekt`. No new Compose UI controls or instrumented tests; E2E via CI's matrix. _Not arming auto-merge — Mergify handles the merge._
mergify[bot] commented 2026-07-08 12:52:53 +00:00 (Migrated from github.com)

Merge Queue Status

  • ✅ Entered queue — 2026-07-08 12:52 UTC · Rule: default · triggered by merge protections
  • ❌ Checks failed — 2026-07-08 13:58 UTC · on draft WIP: merge queue: checking main (6802b60) and [#439 + #435 + #438] together (#441)
  • ✂️ Bisecting to identify the failing PR (round 1/1)
  • ❌ Checks failed · on draft #444 · identified as the failing PR
  • 🚫 Left the queue — 2026-07-08 16:10 UTC · at 49594da10e9f3d2b6a8d031e086802e2e10f0981

This pull request spent 3 hours 17 minutes 35 seconds in the queue, including 1 hour 42 minutes 14 seconds running CI.

Waiting for
  • check-success = CI passed
  • any of: [🛡 GitHub branch protection]
    • check-neutral = CI passed
    • check-skipped = CI passed
    • check-success = CI passed
  • any of: [🛡 GitHub repository ruleset rule main]
    • check-neutral = @github-actions/CI passed
    • check-skipped = @github-actions/CI passed
    • check-success = @github-actions/CI passed
All conditions

Reason

The merge conditions cannot be satisfied due to failing checks

  • CI passed
  • Debug build
  • Unit tests
  • @github-actions/CI passed

Failing checks:

Hint

You may have to fix your CI before adding the pull request to the queue again.
If you update this pull request, to fix the CI, it will automatically be requeued once the queue conditions match again.
If you think this was a flaky issue instead, you can requeue the pull request, without updating it, by posting a @mergifyio queue comment.

Tick the box to put this pull request back in the merge queue (same as @mergifyio queue).

  • Requeue this pull request
<!--- DO NOT EDIT -*- Mergify Payload -*- {"version": 1, "state": "dequeued", "queue_rule_name": "default", "queued_at": "2026-07-08T12:52:50.990949+00:00", "estimated_time_of_merge": null, "speculative_check_pr": null, "required_conditions": []} -*- Mergify Payload End -*- --> # Merge Queue Status - ✅ **Entered queue** — `2026-07-08 12:52 UTC` · Rule: `default` · triggered by merge protections - ❌ **Checks failed** — `2026-07-08 13:58 UTC` · on draft #441 - ✂️ Bisecting to identify the failing PR (round 1/1) - ❌ **Checks failed** · on draft #444 · identified as the failing PR - 🚫 **Left the queue** — `2026-07-08 16:10 UTC` · at `49594da10e9f3d2b6a8d031e086802e2e10f0981` This pull request spent **3 hours 17 minutes 35 seconds** in the queue, including **1 hour 42 minutes 14 seconds** running CI. <details> <summary><strong>Waiting for</strong></summary> - [ ] `check-success = CI passed` - [ ] any of: [🛡 GitHub branch protection] - [ ] `check-neutral = CI passed` - [ ] `check-skipped = CI passed` - [ ] `check-success = CI passed` - [ ] any of: [🛡 GitHub repository ruleset rule `main`] - [ ] `check-neutral = @github-actions/CI passed` - [ ] `check-skipped = @github-actions/CI passed` - [ ] `check-success = @github-actions/CI passed` </details> <details> <summary>All conditions</summary> - [ ] `check-success = CI passed` - [ ] any of [🛡 GitHub branch protection]: - [ ] `check-neutral = CI passed` - [ ] `check-skipped = CI passed` - [ ] `check-success = CI passed` - [ ] any of [🛡 GitHub repository ruleset rule `main`]: - [ ] `check-neutral = @github-actions/CI passed` - [ ] `check-skipped = @github-actions/CI passed` - [ ] `check-success = @github-actions/CI passed` - `-conflict` - [X] #435 - `-draft` - [X] #435 - [X] `base = main` - `github-review-approved` [🛡 GitHub repository ruleset rule `main`] - [X] #435 - `label != broken` - [X] #435 - [X] any of [🛡 GitHub branch protection]: - [X] `check-success = Debug build` - [ ] `check-neutral = Debug build` - [ ] `check-skipped = Debug build` - [X] any of [🛡 GitHub branch protection]: - [X] `check-success = Unit tests` - [ ] `check-neutral = Unit tests` - [ ] `check-skipped = Unit tests` </details> ## Reason The merge conditions cannot be satisfied due to failing checks - `CI passed` - `Debug build` - `Unit tests` - `@github-actions/CI passed` Failing checks: - ❌ [CI passed](https://github.com/JMR-dev/LibreMail/actions/runs/28954030050/job/85920226538) ([job log](https://github.com/JMR-dev/LibreMail/actions/runs/28954030050/job/85920226538)) ## Hint You may have to fix your CI before adding the pull request to the queue again. If you update this pull request, to fix the CI, it will automatically be requeued once the queue conditions match again. If you think this was a flaky issue instead, you can requeue the pull request, without updating it, by posting a `@mergifyio queue` comment. Tick the box to put this pull request back in the merge queue (same as `@mergifyio queue`). - [ ] Requeue this pull request <!-- mergify:queue-control:requeue -->
Sign in to join this conversation.