From 7bddc2eb586a790806466392f7275d00042b1c79 Mon Sep 17 00:00:00 2001 From: Jason Ross Date: Wed, 1 Jul 2026 22:06:22 -0500 Subject: [PATCH] fix(folders): apply label disambiguation to picker and app bar; fix self-referential and transient labels MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three display bugs from the PR #54 code review, all in the shared label-resolution/presentation path: - #59: resolveDrawerLabels was wired only into the drawer, so the move-to picker and the app-bar title still rendered the bare folderDisplayLabel — two identical "Drafts" rows in the picker could move mail to different folders. Both surfaces now consume the same resolution via a shared resolvedFolderLabels helper; picker rows resolve against the unfiltered target list so a row keeps its disambiguation even when its colliding twin is filtered out. - #60: two top-level folders sharing a role-derived base label (e.g. "Sent" and "Sent Items" both classifying SENT on servers without SPECIAL-USE) fell through to the full-path safety net as a self-referential "Sent [Sent]". Colliding top-level user folders now tie-break on the display name: the folder actually named like the base keeps it, the others show their real server name. Corrected the resolver KDoc's overclaimed uniqueness sketch. - #61: the drawer derived the de-dup provider suffix from drawerAccount, which updates before the lagging folders StateFlow during an account switch, so stale Gmail folders briefly rendered as "Drafts - Outlook". The suffix now derives from the rendered folder list's own accountId (providerLabelFor), keeping a stale list under its own account's brand. Tests: FolderLabelsTest covers the role tie-break and providerLabelFor; MailboxViewModelTest pins the switch gap with Turbine; MailboxScreenTest drives the disambiguated move picker (moving via "Drafts - Gmail" lands in [Gmail]/Drafts) and the app-bar title; FolderDrawerTest renders the transient switch frame. Closes #59, closes #60, closes #61. Co-Authored-By: Claude Fable 5 --- .../libremail/ui/mailbox/FolderDrawerTest.kt | 26 ++++++ .../libremail/ui/mailbox/MailboxScreenTest.kt | 59 ++++++++++++- .../org/libremail/ui/mailbox/FolderDrawer.kt | 16 +++- .../org/libremail/ui/mailbox/FolderLabels.kt | 41 +++++++-- .../org/libremail/ui/mailbox/MailboxScreen.kt | 18 +++- .../libremail/ui/mailbox/FolderLabelsTest.kt | 83 ++++++++++++++++++- .../ui/mailbox/MailboxViewModelTest.kt | 39 +++++++++ 7 files changed, 266 insertions(+), 16 deletions(-) diff --git a/app/src/androidTest/kotlin/org/libremail/ui/mailbox/FolderDrawerTest.kt b/app/src/androidTest/kotlin/org/libremail/ui/mailbox/FolderDrawerTest.kt index fbe7abb..bf52dea 100644 --- a/app/src/androidTest/kotlin/org/libremail/ui/mailbox/FolderDrawerTest.kt +++ b/app/src/androidTest/kotlin/org/libremail/ui/mailbox/FolderDrawerTest.kt @@ -79,6 +79,32 @@ class FolderDrawerTest { composeTestRule.onNodeWithText("Drafts").assertIsDisplayed() } + @Test + fun accountSwitchGap_staleFolderListKeepsItsOwnProviderSuffix() { + val gmail = account("imap:g", "user@gmail.com").copy( + imap = ServerConfig("imap.gmail.com", 993, MailSecurity.SSL_TLS), + ) + val outlook = account("imap:o", "user@outlook.com").copy( + authType = AuthType.OAUTH_OUTLOOK, + imap = ServerConfig("outlook.office365.com", 993, MailSecurity.SSL_TLS), + ) + // The transient frame from issue #61: the drawer account has already switched to Outlook, + // but the folder list still holds the Gmail account's folders until its query emits. + setContent( + accounts = listOf(gmail, outlook), + drawerAccount = outlook, + folders = listOf( + folder("imap:g", "INBOX", "INBOX", FolderRole.INBOX), + folder("imap:g", "[Gmail]/Drafts", "Drafts", FolderRole.DRAFTS, specialUse = true), + folder("imap:g", "Drafts", "Drafts", FolderRole.DRAFTS), + ), + ) + + // The de-dup suffix derives from the folders' own account — never the incoming account. + composeTestRule.onNodeWithText("Drafts - Gmail").assertIsDisplayed() + composeTestRule.onNodeWithText("Drafts - Outlook").assertDoesNotExist() + } + @Test fun tappingAFolder_reportsItsAccountAndFullName() { var picked: Pair? = null diff --git a/app/src/androidTest/kotlin/org/libremail/ui/mailbox/MailboxScreenTest.kt b/app/src/androidTest/kotlin/org/libremail/ui/mailbox/MailboxScreenTest.kt index fafb851..0738389 100644 --- a/app/src/androidTest/kotlin/org/libremail/ui/mailbox/MailboxScreenTest.kt +++ b/app/src/androidTest/kotlin/org/libremail/ui/mailbox/MailboxScreenTest.kt @@ -54,6 +54,18 @@ class MailboxScreenTest { smtp = ServerConfig("smtp.example.org", 465, MailSecurity.SSL_TLS), ) + private val gmailAccount = account.copy( + email = "a@gmail.com", + imap = ServerConfig("imap.gmail.com", 993, MailSecurity.SSL_TLS), + ) + + /** A Gmail-style tree where the built-in Drafts collides with a same-named user folder. */ + private val duplicateDraftsFolders = listOf( + Folder("imap:a", "INBOX", "INBOX", FolderRole.INBOX, selectable = true), + Folder("imap:a", "[Gmail]/Drafts", "Drafts", FolderRole.DRAFTS, selectable = true, specialUse = true), + Folder("imap:a", "Drafts", "Drafts", FolderRole.DRAFTS, selectable = true), + ) + private fun message(uid: String, subject: String, bodyFetched: Boolean = false, folder: String = "INBOX") = Message( id = "imap:a:$folder:$uid", accountId = "imap:a", @@ -71,10 +83,10 @@ class MailboxScreenTest { bodyFetched = bodyFetched, ) - private fun setContent(repo: FakeMailRepository): MailboxViewModel { + private fun setContent(repo: FakeMailRepository, activeAccount: Account = account): MailboxViewModel { val viewModel = MailboxViewModel( repo, - FakeAccountRepository(accounts = listOf(account)), + FakeAccountRepository(accounts = listOf(activeAccount)), FakeMailSyncer(), SavedStateHandle(), ) @@ -222,6 +234,49 @@ class MailboxScreenTest { assertEquals("Receipts", repo.movedToFolder.first().second) } + @Test + fun movePicker_disambiguatesDuplicateNames_andMovesToTheChosenFolder() { + val repo = FakeMailRepository( + messages = listOf(message("1", "First")), + folders = duplicateDraftsFolders, + ) + setContent(repo, activeAccount = gmailAccount) + waitForText("First") + + composeTestRule.onNodeWithText("First").performTouchInput { longClick() } + composeTestRule.onNodeWithContentDescription(string(R.string.action_more)).performClick() + composeTestRule.onNodeWithText(string(R.string.action_move)).performClick() + + // Issue #59: the two same-named Drafts folders are told apart in the picker, and choosing + // the suffixed row files into the provider's built-in folder — not the user folder. + composeTestRule.onNode(hasText("Drafts") and hasAnyAncestor(isDialog())).assertIsDisplayed() + composeTestRule.onNode(hasText("Drafts - Gmail") and hasAnyAncestor(isDialog())).performClick() + + composeTestRule.waitUntil(5_000) { repo.movedToFolder.isNotEmpty() } + assertEquals("[Gmail]/Drafts", repo.movedToFolder.first().second) + } + + @Test + fun appBarTitle_keepsTheDisambiguatedFolderLabel() { + val repo = FakeMailRepository( + messages = listOf(message("1", "First")), + folders = duplicateDraftsFolders, + ) + setContent(repo, activeAccount = gmailAccount) + waitForText("First") + + composeTestRule.onNodeWithContentDescription(string(R.string.drawer_open)).performClick() + composeTestRule.onNodeWithText("Drafts - Gmail").assertIsDisplayed() + composeTestRule.onNodeWithText("Drafts - Gmail").performClick() + + // Issue #59: the app-bar title keeps the drawer's de-duplicated label instead of collapsing + // to an ambiguous "Drafts". Once selected, the label renders twice — the app-bar title plus + // the (composed but closed) drawer's entry. + composeTestRule.waitUntil(5_000) { + composeTestRule.onAllNodesWithText("Drafts - Gmail").fetchSemanticsNodes().size == 2 + } + } + @Test fun offlineIndicator_showsForCachedMessages() { setContent(FakeMailRepository(messages = listOf(message("1", "Cached", bodyFetched = true)))) diff --git a/app/src/main/kotlin/org/libremail/ui/mailbox/FolderDrawer.kt b/app/src/main/kotlin/org/libremail/ui/mailbox/FolderDrawer.kt index edef7eb..de9a918 100644 --- a/app/src/main/kotlin/org/libremail/ui/mailbox/FolderDrawer.kt +++ b/app/src/main/kotlin/org/libremail/ui/mailbox/FolderDrawer.kt @@ -79,8 +79,7 @@ fun FolderDrawer( } // De-duplicate labels: two distinct folders that would render the same name (e.g. a // provider's built-in Drafts and a same-named user folder) get disambiguated. - val baseLabels = sorted.associate { it.fullName to folderDisplayLabel(it) } - val resolvedLabels = resolveDrawerLabels(sorted, baseLabels, drawerAccount?.let(::providerLabel).orEmpty()) + val resolvedLabels = resolvedFolderLabels(sorted, accounts) sorted.forEach { folder -> val isSelected = selectedAccountId != null && selectedAccountId == drawerAccount?.id && @@ -126,6 +125,19 @@ private fun AccountSwitcher(accounts: List, current: Account, onSelect: } } +/** + * De-duplicated display labels for [folders], keyed by [Folder.fullName] — the one resolution used + * by the drawer, the move-to picker, and the app-bar title, so a folder reads the same everywhere. + * The provider suffix derives from the account owning the listed folders (see [providerLabelFor]), + * not from whichever account is currently selected, so a folder list that briefly lags an account + * switch never borrows the incoming account's brand. + */ +@Composable +fun resolvedFolderLabels(folders: List, accounts: List): Map { + val baseLabels = folders.associate { it.fullName to folderDisplayLabel(it) } + return resolveDrawerLabels(folders, baseLabels, providerLabelFor(folders, accounts)) +} + /** The user-facing label for a folder: a friendly name for standard roles, else the server name. */ @Composable fun folderDisplayLabel(folder: Folder): String = when (folder.role) { diff --git a/app/src/main/kotlin/org/libremail/ui/mailbox/FolderLabels.kt b/app/src/main/kotlin/org/libremail/ui/mailbox/FolderLabels.kt index c65b85b..db2d928 100644 --- a/app/src/main/kotlin/org/libremail/ui/mailbox/FolderLabels.kt +++ b/app/src/main/kotlin/org/libremail/ui/mailbox/FolderLabels.kt @@ -20,14 +20,27 @@ fun providerLabel(account: Account): String = when { } /** - * Resolves each folder's drawer label so no two entries collide. [baseLabels] maps a folder's + * The provider suffix used when de-duplicating [folders]' labels, derived from the account that owns + * the listed folders themselves. While switching accounts, the drawer's account updates before its + * folder list catches up, so a still-rendered stale list must keep its own account's suffix instead + * of borrowing the incoming account's brand. Empty when [folders] is empty or its owner is not in + * [accounts] (no suffix is better than a wrong one). + */ +fun providerLabelFor(folders: List, accounts: List): String = + accounts.firstOrNull { it.id == folders.firstOrNull()?.accountId }?.let(::providerLabel).orEmpty() + +/** + * Resolves each folder's user-facing label so folders that would render the same name are told apart + * (used by the drawer, the move-to picker, and the app-bar title). [baseLabels] maps a folder's * [Folder.fullName] to its localized base label (a friendly role name, or the raw display name). * * A base label shared by 2+ folders is disambiguated: the provider's built-in special folder gets * " - [providerLabel]" (e.g. "Archive - Gmail"); a nested user folder gets its parent location in - * parentheses (e.g. "Reports (Work)"); a top-level user folder keeps its base label. Among colliding - * user folders at most one is top-level (paths are unique), so the result is distinct. A final pass - * appends the full path to any labels that still tie, guaranteeing uniqueness. + * parentheses (e.g. "Reports (Work)"); a top-level user folder keeps the base label only when that + * is its own name, otherwise it shows its real name (see [userFolderLabel]). These rules cover the + * common collisions but not every one (e.g. "Work/2024/Reports" and "Home/2024/Reports" both yield + * "Reports (2024)"), so a final pass appends the full path to any labels that still tie, telling + * the tied entries apart. */ fun resolveDrawerLabels( folders: List, @@ -42,18 +55,34 @@ fun resolveDrawerLabels( val label = when { base !in duplicated -> base folder.specialUse -> "$base - $providerLabel" - else -> parentOf(folder.fullName, folder.displayName)?.let { "$base ($it)" } ?: base + else -> userFolderLabel(folder, base) } folder.fullName to label } // Safety net for a residual tie (e.g. two special folders mapping to one role): fall back to the - // unambiguous full path so every drawer entry stays distinct. + // unambiguous full path so the tied entries stay apart. val stillTied = resolved.values.groupingBy { it }.eachCount().filterValues { it > 1 }.keys if (stillTied.isEmpty()) return resolved return resolved.mapValues { (fullName, label) -> if (label in stillTied) "$label [$fullName]" else label } } +/** + * A colliding user folder's label. A nested folder shows its parent location. Top-level folders + * tie-break on the display name: several can share one role-derived base label (on servers without + * SPECIAL-USE, "Sent" and "Sent Items" both classify as the Sent role), so only the folder actually + * named like the base keeps it — the others show their real server name rather than falling through + * to the safety net as a self-referential "Sent [Sent]". + */ +private fun userFolderLabel(folder: Folder, base: String): String { + val parent = parentOf(folder.fullName, folder.displayName) + return when { + parent != null -> "$base ($parent)" + folder.displayName.equals(base, ignoreCase = true) -> base + else -> folder.displayName + } +} + /** * The immediate parent segment of [fullName] (its location), or null when the folder is top-level. * [displayName] is the leaf, so the character just before it in [fullName] is the server's hierarchy diff --git a/app/src/main/kotlin/org/libremail/ui/mailbox/MailboxScreen.kt b/app/src/main/kotlin/org/libremail/ui/mailbox/MailboxScreen.kt index aaebd11..fc4f1aa 100644 --- a/app/src/main/kotlin/org/libremail/ui/mailbox/MailboxScreen.kt +++ b/app/src/main/kotlin/org/libremail/ui/mailbox/MailboxScreen.kt @@ -197,7 +197,10 @@ fun MailboxScreen( val current = folders.firstOrNull { it.fullName == selectedFolder } Text( if (current != null) { - folderDisplayLabel(current) + // The drawer's de-duplicated label, so "Drafts - Gmail" + // doesn't collapse to an ambiguous "Drafts" once opened. + resolvedFolderLabels(folders, accounts)[current.fullName] + ?: folderDisplayLabel(current) } else { stringResource(R.string.title_mailbox) }, @@ -315,6 +318,10 @@ fun MailboxScreen( if (showMovePicker) { MoveFolderDialog( folders = moveTargetFolders.filter { it.selectable && it.fullName != selectedFolder }, + // Labels resolved against the unfiltered list, so rows keep the drawer's + // disambiguation even when a colliding twin (e.g. the current folder) is + // filtered out of the picker itself. + labels = resolvedFolderLabels(moveTargetFolders, accounts), onSelect = { folder -> showMovePicker = false viewModel.moveSelected(folder.fullName) @@ -664,7 +671,12 @@ private fun ConfirmActionDialog(pending: PendingAction, onConfirm: () -> Unit, o } @Composable -private fun MoveFolderDialog(folders: List, onSelect: (Folder) -> Unit, onDismiss: () -> Unit) { +private fun MoveFolderDialog( + folders: List, + labels: Map, + onSelect: (Folder) -> Unit, + onDismiss: () -> Unit, +) { AlertDialog( onDismissRequest = onDismiss, title = { Text(stringResource(R.string.move_picker_title)) }, @@ -672,7 +684,7 @@ private fun MoveFolderDialog(folders: List, onSelect: (Folder) -> Unit, Column(Modifier.verticalScroll(rememberScrollState())) { folders.forEach { folder -> Text( - text = folderDisplayLabel(folder), + text = labels[folder.fullName] ?: folderDisplayLabel(folder), style = MaterialTheme.typography.bodyLarge, modifier = Modifier .fillMaxWidth() diff --git a/app/src/test/kotlin/org/libremail/ui/mailbox/FolderLabelsTest.kt b/app/src/test/kotlin/org/libremail/ui/mailbox/FolderLabelsTest.kt index 65644ed..0b4682f 100644 --- a/app/src/test/kotlin/org/libremail/ui/mailbox/FolderLabelsTest.kt +++ b/app/src/test/kotlin/org/libremail/ui/mailbox/FolderLabelsTest.kt @@ -17,10 +17,16 @@ class FolderLabelsTest { displayName: String = fullName.substringAfterLast('/'), role: FolderRole = FolderRole.NORMAL, specialUse: Boolean = false, - ) = Folder("acct", fullName, displayName, role, selectable = true, specialUse = specialUse) + accountId: String = "acct", + ) = Folder(accountId, fullName, displayName, role, selectable = true, specialUse = specialUse) - private fun account(email: String, imapHost: String, authType: AuthType = AuthType.PASSWORD_IMAP) = Account( - id = "acct", + private fun account( + email: String, + imapHost: String, + authType: AuthType = AuthType.PASSWORD_IMAP, + id: String = "acct", + ) = Account( + id = id, email = email, displayName = email, authType = authType, @@ -117,6 +123,77 @@ class FolderLabelsTest { assertEquals(2, labels.values.toSet().size, "every drawer entry must be unique") } + // Issue #60: on a server without SPECIAL-USE (e.g. GreenMail) top-level "Sent" and "Sent Items" + // both classify as SENT via the name fallback and share the friendly "Sent" base label. The tie + // must break on the display name, not net out as a self-referential "Sent [Sent]". + @Test + fun `top-level folders sharing a role tie-break on their names, not a self-referential path`() { + val folders = listOf( + folder("Sent", "Sent", FolderRole.SENT), + folder("Sent Items", "Sent Items", FolderRole.SENT), + ) + val labels = resolve(folders, "example.org") + assertEquals("Sent", labels["Sent"]) + assertEquals("Sent Items", labels["Sent Items"]) + } + + @Test + fun `same-role top-level folders where none matches the friendly name keep their own names`() { + val folders = listOf( + folder("Sent Mail", "Sent Mail", FolderRole.SENT), + folder("Sent Items", "Sent Items", FolderRole.SENT), + ) + val labels = resolve(folders, "example.org") + assertEquals("Sent Mail", labels["Sent Mail"]) + assertEquals("Sent Items", labels["Sent Items"]) + } + + @Test + fun `provider special, canonical, and synonym folders for one role all stay apart`() { + val folders = listOf( + folder("[Gmail]/Sent Mail", "Sent Mail", FolderRole.SENT, specialUse = true), + folder("Sent", "Sent", FolderRole.SENT), + folder("Sent Items", "Sent Items", FolderRole.SENT), + ) + val labels = resolve(folders, "Gmail") + assertEquals("Sent - Gmail", labels["[Gmail]/Sent Mail"]) + assertEquals("Sent", labels["Sent"]) + assertEquals("Sent Items", labels["Sent Items"]) + } + + // Issue #61: while switching accounts the drawer's account updates before its folder list, so + // the provider suffix must derive from the rendered folders' own account — never from the newer + // drawer selection. + @Test + fun `providerLabelFor derives the suffix from the folder list's owner account`() { + val gmail = account("user@gmail.com", "imap.gmail.com", id = "acct-gmail") + val outlook = + account("user@outlook.com", "outlook.office365.com", AuthType.OAUTH_OUTLOOK, id = "acct-outlook") + // The drawer account has already switched to Outlook, but these stale folders are Gmail's. + val staleGmailFolders = listOf( + folder("[Gmail]/Drafts", "Drafts", FolderRole.DRAFTS, specialUse = true, accountId = "acct-gmail"), + folder("Drafts", "Drafts", FolderRole.DRAFTS, accountId = "acct-gmail"), + ) + + val provider = providerLabelFor(staleGmailFolders, listOf(gmail, outlook)) + + assertEquals("Gmail", provider) + val labels = + resolveDrawerLabels(staleGmailFolders, baseLabelsOf(staleGmailFolders, friendlyNames), provider) + assertEquals("Drafts - Gmail", labels["[Gmail]/Drafts"]) + assertEquals("Drafts", labels["Drafts"]) + } + + @Test + fun `providerLabelFor is empty for an empty or orphaned folder list`() { + val outlook = + account("user@outlook.com", "outlook.office365.com", AuthType.OAUTH_OUTLOOK, id = "acct-outlook") + assertEquals("", providerLabelFor(emptyList(), listOf(outlook))) + + val orphaned = listOf(folder("INBOX", "INBOX", FolderRole.INBOX, accountId = "acct-gone")) + assertEquals("", providerLabelFor(orphaned, listOf(outlook))) + } + @Test fun `providerLabel resolves brands, Outlook, and falls back to the email domain`() { assertEquals("Gmail", providerLabel(account("a@gmail.com", "imap.gmail.com"))) diff --git a/app/src/test/kotlin/org/libremail/ui/mailbox/MailboxViewModelTest.kt b/app/src/test/kotlin/org/libremail/ui/mailbox/MailboxViewModelTest.kt index 25cb649..b73e4dd 100644 --- a/app/src/test/kotlin/org/libremail/ui/mailbox/MailboxViewModelTest.kt +++ b/app/src/test/kotlin/org/libremail/ui/mailbox/MailboxViewModelTest.kt @@ -2,12 +2,14 @@ package org.libremail.ui.mailbox import androidx.lifecycle.SavedStateHandle +import app.cash.turbine.test import io.mockk.coEvery import io.mockk.coVerify import io.mockk.every import io.mockk.mockk import kotlinx.coroutines.Dispatchers import kotlinx.coroutines.ExperimentalCoroutinesApi +import kotlinx.coroutines.flow.MutableSharedFlow import kotlinx.coroutines.flow.MutableStateFlow import kotlinx.coroutines.flow.first import kotlinx.coroutines.flow.flowOf @@ -157,6 +159,43 @@ class MailboxViewModelTest { assertTrue(vm.folders.value.none { it.fullName == "Archive" }, "alice's folders should no longer show") } + // Issue #61: switching the drawer account emits the new account immediately, while the folder + // list keeps the old account's folders until the new account's query emits. The UI must derive + // provider labels from the rendered folders' own accountId (providerLabelFor) so those gap + // frames never show the old folders with the new account's brand. + @Test + fun `folder list lags a drawer-account switch until the new folders arrive`() = runTest(testDispatcher) { + val repo = mockk(relaxed = true) + every { repo.observeMessages() } returns MutableStateFlow(emptyList()) + every { repo.observeDrafts() } returns flowOf(emptyList()) + every { repo.observeOutbox() } returns flowOf(emptyList()) + every { repo.observeFolders("imap:a") } returns + MutableStateFlow(listOf(folder("imap:a", "INBOX", FolderRole.INBOX))) + // Bob's folder query stays in flight (no emission yet) to reproduce the switch gap. + val bobFolders = MutableSharedFlow>() + every { repo.observeFolders("imap:b") } returns bobFolders + val accountRepository = mockk(relaxed = true) + every { accountRepository.observeAccounts() } returns MutableStateFlow(listOf(alice, bob)) + val vm = MailboxViewModel(repo, accountRepository, mockk(relaxed = true), SavedStateHandle()) + + vm.folders.test { + var current = awaitItem() // the StateFlow's initial empty value, or alice's list + while (current.isEmpty()) current = awaitItem() + assertEquals(listOf("imap:a"), current.map { it.accountId }.distinct()) + + vm.setDrawerAccount("imap:b") + + // The drawer account has already switched, but the rendered folder list has not — + // exactly the transient frames issue #61 is about. + assertEquals("imap:b", vm.drawerAccount.value?.id) + assertEquals(listOf("imap:a"), vm.folders.value.map { it.accountId }.distinct()) + expectNoEvents() + + bobFolders.emit(listOf(folder("imap:b", "Work", FolderRole.NORMAL))) + assertEquals(listOf("imap:b"), awaitItem().map { it.accountId }.distinct()) + } + } + @Test fun `toggle adds then removes a message from the selection`() = runTest(testDispatcher) { val vm = createViewModel(accounts = listOf(alice), messages = emptyList())