10 changed files with 218 additions and 19 deletions
@@ -7,6 +7,7 @@ import kotlinx.coroutines.withContext
 import org.json.JSONArray
 import org.json.JSONObject
 import org.libremail.domain.model.OutgoingMessage
+import org.libremail.reporting.AppLog
 import java.io.IOException
 import java.net.HttpURLConnection
 import java.net.URL
@@ -34,6 +35,14 @@ class GraphSender @Inject constructor() {
        message: OutgoingMessage,
        attachments: List<SendableAttachment> = emptyList(),
    ) = withContext(Dispatchers.IO) {
+        // Guard before any attachment is read into memory: Graph sendMail carries attachment bytes inline
+        // (base64) in a single ~4 MB request, so an oversized file would blow that request limit and risk
+        // an OOM from readBytes(). Fail with mayHaveSent=false so the outbox falls back to SMTP, which
+        // streams attachments and handles far larger files (#298).
+        attachments.firstOrNull { it.file.length() > MAX_ATTACHMENT_BYTES }?.let {
+            AppLog.w(TAG, "Attachment over Graph sendMail size limit; not sending via Graph")
+            throw GraphSendException("Attachment exceeds the Graph sendMail size limit", mayHaveSent = false)
+        }
        val payload = buildSendMailPayload(message, attachments)
        val connection = (URL(SEND_MAIL_URL).openConnection() as HttpURLConnection).apply {
            requestMethod = "POST"
@@ -76,8 +85,13 @@ class GraphSender @Inject constructor() {
    }

    private companion object {
+        const val TAG = "GraphSender"
        const val SEND_MAIL_URL = "https://graph.microsoft.com/v1.0/me/sendMail"
        const val TIMEOUT_MS = 15_000
+
+        // Per-file ceiling kept below Graph sendMail's ~4 MB whole-request cap, so one attachment can never
+        // exceed the request limit or OOM when read into the base64 payload; larger files fall back to SMTP.
+        const val MAX_ATTACHMENT_BYTES = 3L * 1024 * 1024
        const val HTTP_OK_MIN = 200
        const val HTTP_OK_MAX = 299
        const val ERROR_BODY_LIMIT = 500
@@ -12,23 +12,29 @@ package org.libremail.mail
 */
 object HtmlToText {

+    // Hoisted out of convert() so each pattern is compiled once, not four times per call — convert()
+    // runs once per fetched HTML body during sync, so this is a hot path (#298).
+    private val SCRIPT_STYLE = Regex("(?is)<(script|style)\\b[^>]*>.*?</\\1>")
+    private val LIST_ITEM = Regex("(?i)<li\\b[^>]*>")
    private val BLOCK_BREAK = Regex(
        "(?i)</?(p|div|tr|table|ul|ol|h[1-6]|blockquote)\\b[^>]*>|<br\\s*/?>",
    )
-    private val LIST_ITEM = Regex("(?i)<li\\b[^>]*>")
+    private val TAG = Regex("<[^>]*>")
+    private val SPACES_AND_TABS = Regex("[ \\t]+")
+    private val BLANK_LINES = Regex("\n{3,}")

    fun convert(html: String): String {
        var s = html
        // Drop script/style contents outright so their text never leaks into the output.
-        s = s.replace(Regex("(?is)<(script|style)\\b[^>]*>.*?</\\1>"), "")
+        s = SCRIPT_STYLE.replace(s, "")
        s = LIST_ITEM.replace(s, "\n• ")
        s = BLOCK_BREAK.replace(s, "\n")
-        s = s.replace(Regex("<[^>]*>"), "")
+        s = TAG.replace(s, "")
        s = decodeEntities(s)
        // Collapse runs of spaces/tabs, then trim trailing spaces and cap consecutive blank lines.
-        s = s.replace(Regex("[ \\t]+"), " ")
+        s = SPACES_AND_TABS.replace(s, " ")
        s = s.lineSequence().joinToString("\n") { it.trim() }
-        s = s.replace(Regex("\n{3,}"), "\n\n")
+        s = BLANK_LINES.replace(s, "\n\n")
        return s.trim()
    }

@@ -99,14 +99,24 @@ private fun providerLabel(account: Account): String = when (account.authType) {
    AuthType.PASSWORD_IMAP -> imapProviderLabel(account.imap.host)
 }

+/**
+ * Buckets an IMAP host to a coarse provider by matching brand tokens at DNS-label boundaries rather
+ * than as raw substrings, so a custom domain that merely contains a brand name — e.g.
+ * `mail.notgmail.example` — is no longer mislabeled (here it would have read as Gmail) (#298). The
+ * short, common tokens (`me`/`mac`/`live`) match only as a registrable-domain suffix, never as a bare
+ * label, so an innocent `me.company.example` doesn't read as iCloud either.
+ */
 private fun imapProviderLabel(host: String): String {
    val h = host.lowercase()
+    val labels = h.split('.')
+    fun hasLabel(vararg brands: String) = brands.any { it in labels }
+    fun hasDomain(vararg domains: String) = domains.any { h == it || h.endsWith(".$it") }
    return when {
-        "gmail" in h || "googlemail" in h -> "Gmail"
-        "yahoo" in h -> "Yahoo"
-        "icloud" in h || "me.com" in h || "mac.com" in h -> "iCloud"
-        "outlook" in h || "office365" in h || "hotmail" in h || "live.com" in h -> "Outlook"
-        "aol" in h -> "AOL"
+        hasLabel("gmail", "googlemail") -> "Gmail"
+        hasLabel("yahoo") -> "Yahoo"
+        hasLabel("icloud") || hasDomain("me.com", "mac.com") -> "iCloud"
+        hasLabel("outlook", "office365", "hotmail") || hasDomain("live.com") -> "Outlook"
+        hasLabel("aol") -> "AOL"
        else -> "Other"
    }
 }
@@ -9,6 +9,9 @@ import kotlinx.coroutines.flow.StateFlow
 import kotlinx.coroutines.flow.asStateFlow
 import kotlinx.coroutines.launch
 import java.io.File
+import java.nio.file.AtomicMoveNotSupportedException
+import java.nio.file.Files
+import java.nio.file.StandardCopyOption

 /**
 * File-backed store of pending [DebugReport]s — one JSON file per report under [directory].
@@ -52,7 +55,7 @@ class ReportStore(
        synchronized(lock) {
            val serialized = serializeForDisk(report) ?: return
            directory.mkdirs()
-            File(directory, fileName(report.id)).writeText(serialized)
+            writeAtomically(File(directory, fileName(report.id)), serialized)
            _reports.value = scan()
        }
    }
@@ -69,7 +72,7 @@ class ReportStore(
            val report = _reports.value.firstOrNull { it.id == id } ?: return
            if (report.surfaced) return
            val serialized = serializeForDisk(report.copy(surfaced = true)) ?: return
-            File(directory, fileName(id)).writeText(serialized)
+            writeAtomically(File(directory, fileName(id)), serialized)
            _reports.value = scan()
        }
    }
@@ -135,10 +138,38 @@ class ReportStore(
        return runCatching { DebugReport.fromStorageJson(json) }.getOrNull()
    }

+    /**
+     * Writes [content] to [target] via a temp file + atomic rename, so a process death mid-write — the
+     * crash path that saves a report while the app is dying — can never leave a torn `.json` that [scan]
+     * would fail to parse and silently drop (#298). The temp file uses a non-`.json` suffix so [scan]
+     * ignores it (and any orphan left by an interrupted write), and the rename replaces an existing file
+     * (the [markSurfaced] rewrite) atomically. Falls back to a plain replace on the rare filesystem
+     * without atomic rename — still safer than an in-place truncate-then-write.
+     */
+    private fun writeAtomically(target: File, content: String) {
+        val tmp = File(directory, target.name + TMP_SUFFIX)
+        tmp.writeText(content)
+        try {
+            Files.move(
+                tmp.toPath(),
+                target.toPath(),
+                StandardCopyOption.ATOMIC_MOVE,
+                StandardCopyOption.REPLACE_EXISTING,
+            )
+        } catch (e: AtomicMoveNotSupportedException) {
+            AppLog.w(TAG, "Atomic report write unsupported here; falling back to a non-atomic replace", e)
+            Files.move(tmp.toPath(), target.toPath(), StandardCopyOption.REPLACE_EXISTING)
+        }
+    }
+
    private fun fileName(id: String) = "$id$SUFFIX"

    private companion object {
        const val SUFFIX = ".json"
+
+        // Suffix for the write-and-rename temp file. Deliberately NOT ending in [SUFFIX] so scan() never
+        // treats a half-written or orphaned temp as a report (#298).
+        const val TMP_SUFFIX = ".tmp"
        const val TAG = "ReportStore"

        /**
@@ -109,11 +109,17 @@ internal fun lineMarker(line: String): String? = when {
 */
 internal fun mergeSameValueSpans(spans: List<RichSpan>): List<RichSpan> {
    val merged = ArrayList<RichSpan>()
+    // Index of the right-most merged run for each style value. Spans are processed in ascending start
+    // order, and runs of one value stay non-overlapping with strictly increasing ends, so only that
+    // value's last run can touch the next span — track it directly instead of re-scanning `merged` for
+    // every span (the old O(n^2) indexOfLast). The produced list is byte-for-byte identical (#298).
+    val lastRunByStyle = HashMap<RichStyle, Int>()
    for (span in spans.sortedWith(compareBy({ it.start }, { it.end }))) {
-        val i = merged.indexOfLast { it.style == span.style && span.start <= it.end }
-        if (i >= 0) {
+        val i = lastRunByStyle[span.style]
+        if (i != null && span.start <= merged[i].end) {
            merged[i] = merged[i].copy(end = maxOf(merged[i].end, span.end))
        } else {
+            lastRunByStyle[span.style] = merged.size
            merged.add(span)
        }
    }
@@ -33,10 +33,15 @@ object RichTextEditing {
        return content.copy(spans = (otherKinds + updated).sortedBy { it.start })
    }

-    /** Links [[start], [end]) to [url], replacing any links that overlap the range. */
+    /**
+     * Links [[start], [end]) to [url]. A link that only partially overlaps the range keeps its
+     * non-overlapping remainder — the same split [toggleStyle]/[subtractRange] does for spans — instead
+     * of being dropped whole, so relinking part of a longer link no longer silently un-links the rest
+     * of it (#298). A link fully inside the range is replaced outright.
+     */
    fun applyLink(content: RichTextContent, start: Int, end: Int, url: String): RichTextContent {
        if (start >= end || url.isBlank()) return content
-        val kept = content.links.filter { it.end <= start || it.start >= end }
+        val kept = subtractLinkRange(content.links, start, end)
        return content.copy(links = (kept + RichLink(start, end, url)).sortedBy { it.start })
    }

@@ -221,6 +226,17 @@ private fun subtractRange(spans: List<RichSpan>, start: Int, end: Int): List<Ric
    }
 }

+/** Links analogue of [subtractRange]: drops the [[start], [end]) slice of each link, keeping the rest. */
+private fun subtractLinkRange(links: List<RichLink>, start: Int, end: Int): List<RichLink> = links.flatMap { link ->
+    when {
+        link.end <= start || link.start >= end -> listOf(link)
+        else -> buildList {
+            if (link.start < start) add(link.copy(end = start))
+            if (link.end > end) add(link.copy(start = end))
+        }
+    }
+}
+
 // --- block marker helpers ---

 private val ORDERED = Regex("^\\d+\\. ")
@@ -1,14 +1,20 @@
 // SPDX-License-Identifier: GPL-3.0-or-later
 package org.libremail.mail

+import io.mockk.every
+import io.mockk.mockkStatic
+import io.mockk.unmockkAll
 import kotlinx.coroutines.test.runTest
 import org.junit.After
+import org.junit.Before
 import org.junit.Test
 import org.libremail.domain.model.OutgoingMessage
 import java.io.ByteArrayOutputStream
+import java.io.File
 import java.io.IOException
 import java.io.InputStream
 import java.io.OutputStream
+import java.io.RandomAccessFile
 import java.net.HttpURLConnection
 import java.net.URL
 import java.net.URLConnection
@@ -18,6 +24,7 @@ import java.util.concurrent.atomic.AtomicReference
 import kotlin.test.assertEquals
 import kotlin.test.assertFailsWith
 import kotlin.test.assertFalse
+import kotlin.test.assertNull
 import kotlin.test.assertTrue

 /**
@@ -30,8 +37,19 @@ import kotlin.test.assertTrue
 */
 class GraphSenderSendTest {

+    @Before
+    fun setUp() {
+        // send() now breadcrumbs through AppLog on the oversized-attachment guard; android.util.Log is a
+        // no-op stub under plain JVM tests, so mock it (fully qualified, so this file never imports it).
+        mockkStatic(android.util.Log::class)
+        every { android.util.Log.w(any<String>(), any<String>()) } returns 0
+    }
+
    @After
-    fun tearDown() = armed.set(null)
+    fun tearDown() {
+        armed.set(null)
+        unmockkAll()
+    }

    private val message =
        OutgoingMessage(accountId = "outlook:me@x.com", to = "bob@example.org", subject = "Hi", body = "Body")
@@ -84,6 +102,26 @@ class GraphSenderSendTest {
        assertFalse(ex.mayHaveSent, "the request never reached Graph, so a retry is safe")
    }

+    @Test
+    fun `an oversized attachment fails safe-to-fall-back before opening a connection`() = runTest {
+        val last = arm() // armed, but the guard must trip before any connection is opened
+        val big = File.createTempFile("graph-big", ".bin")
+        try {
+            // 4 MiB, over the 3 MiB per-file cap. setLength allocates the size without writing the bytes,
+            // so the guard (which reads file.length()) trips without the test materializing 4 MiB.
+            RandomAccessFile(big, "rw").use { it.setLength(4L * 1024 * 1024) }
+
+            val ex = assertFailsWith<GraphSendException> {
+                GraphSender().send("token", message, listOf(SendableAttachment(big)))
+            }
+
+            assertFalse(ex.mayHaveSent, "oversized never reached Graph, so SMTP fallback is safe")
+            assertNull(last.get(), "the guard must trip before any connection is opened")
+        } finally {
+            big.delete()
+        }
+    }
+
    @Test
    fun `GraphSendException carries its message, flag and cause`() {
        val cause = IOException("boom")
@@ -169,6 +169,26 @@ class DiagnosticsCollectorTest {
        )
    }

+    @Test
+    fun `provider label matches brand tokens at label boundaries, not as substrings`() = runTest {
+        every { settingsRepository.settings } returns flowOf(AppSettings())
+        // Each custom host merely CONTAINS a brand name inside a longer DNS label; the old substring match
+        // mislabeled them (notgmail→Gmail, yahooligans→Yahoo, me.company→iCloud, kaolin→AOL). They must all
+        // bucket to "Other" now (#298).
+        every { accountRepository.observeAccounts() } returns flowOf(
+            listOf(
+                account("1@x", AuthType.PASSWORD_IMAP, "mail.notgmail.example"),
+                account("2@x", AuthType.PASSWORD_IMAP, "imap.yahooligans.example"),
+                account("3@x", AuthType.PASSWORD_IMAP, "me.company.example"),
+                account("4@x", AuthType.PASSWORD_IMAP, "kaolin.example"),
+            ),
+        )
+
+        val report = collector.collectManual()
+
+        assertEquals(List(4) { "Other (PASSWORD_IMAP)" }, report.accounts)
+    }
+
    private fun account(email: String, authType: AuthType, imapHost: String) = Account(
        id = "id:$email",
        email = email,
@@ -111,6 +111,29 @@ class ReportStoreTest {
        assertEquals(listOf("valid"), store.reports.value.map { it.id })
    }

+    @Test
+    fun `save leaves no temporary file behind (atomic write renames it into place)`() {
+        val store = newStore()
+
+        store.save(report("a"))
+
+        // The write-and-rename temp must not linger: only the final ".json" remains on disk (#298).
+        assertEquals(listOf("a.json"), tempFolder.root.listFiles()?.map { it.name }.orEmpty())
+    }
+
+    @Test
+    fun `a stray temp file from an interrupted write is never scanned as a report`() {
+        // A process death mid-write leaves a ".json.tmp" file, never a torn ".json". scan() filters on
+        // ".json", so the orphan is ignored and a valid report saved alongside still lists cleanly — the
+        // old in-place write could instead leave a truncated ".json" that scan() silently dropped (#298).
+        File(tempFolder.root, "torn.json.tmp").writeText("{ half-written")
+        val store = newStore()
+
+        store.save(report("valid"))
+
+        assertEquals(listOf("valid"), store.reports.value.map { it.id })
+    }
+
    @Test
    fun `purgeOlderThan deletes reports strictly older than the cutoff`() {
        val store = newStore()
@@ -301,18 +301,53 @@ class RichTextEditingTest {
    }

    @Test
-    fun `applyLink keeps links wholly outside the range and replaces overlapping ones`() {
+    fun `applyLink keeps links outside the range and splits partial overlaps, keeping the remainder`() {
        val content = RichTextContent(
            "0123456789",
            links = listOf(RichLink(0, 2, "a"), RichLink(3, 6, "b"), RichLink(7, 9, "c")),
        )
        val result = RichTextEditing.applyLink(content, 4, 7, "http://new")
        assertEquals(
-            listOf(RichLink(0, 2, "a"), RichLink(4, 7, "http://new"), RichLink(7, 9, "c")),
+            // "a" is wholly outside; "b" (3,6) overlaps [4,7) so only its (3,4) remainder survives (it is
+            // no longer dropped whole); the new link takes [4,7); "c" starts at the range end, kept whole.
+            listOf(RichLink(0, 2, "a"), RichLink(3, 4, "b"), RichLink(4, 7, "http://new"), RichLink(7, 9, "c")),
            result.links.sortedBy { it.start },
        )
    }

+    @Test
+    fun `applyLink over the middle of a link relinks the middle and keeps both surrounding remainders`() {
+        val content = RichTextContent("0123456789", links = listOf(RichLink(0, 8, "old")))
+        val result = RichTextEditing.applyLink(content, 3, 5, "new")
+        assertEquals(
+            listOf(RichLink(0, 3, "old"), RichLink(3, 5, "new"), RichLink(5, 8, "old")),
+            result.links.sortedBy { it.start },
+        )
+    }
+
+    // --- mergeSameValueSpans (shared merge used by toggleStyle and the HTML parser) ---
+
+    @Test
+    fun `mergeSameValueSpans coalesces touching and overlapping runs of the same value only`() {
+        val merged = mergeSameValueSpans(
+            listOf(
+                RichSpan(5, 8, RichStyle.Bold), // out of order, and overlaps the (3,6) run below
+                RichSpan(0, 3, RichStyle.Bold),
+                RichSpan(3, 6, RichStyle.Bold), // touches (0,3) and overlaps (5,8) → one 0..8 run
+                RichSpan(0, 4, RichStyle.Italic), // a different value never folds into the Bold run
+                RichSpan(10, 12, RichStyle.Bold), // a gap breaks the run into a fresh one
+            ),
+        )
+        assertEquals(
+            listOf(
+                RichSpan(0, 8, RichStyle.Bold),
+                RichSpan(0, 4, RichStyle.Italic),
+                RichSpan(10, 12, RichStyle.Bold),
+            ),
+            merged,
+        )
+    }
+
    // --- styleAt / isStyled caret edges ---

    @Test