plan: refresh plan for issue #539

ISSUE-533.md

@@ -1,101 +0,0 @@
-# Plan: Consolidate Email-List UI
-
-## Goal
-
-Three list surfaces — folder view (`EmailListScreen`), combined inbox (`CombinedInboxScreen`), and search results (in `SearchScreen` plus the in-screen search in `EmailListScreen`, plus `AddressEmailsScreen`) — currently duplicate selection state, swipe-dismiss handling, batch actions (archive/delete/spam/move/snooze), and tile rendering. Unify into one widget so behaviour and UI are identical everywhere. Thread detail view is intentionally out of scope.
-
-## Current duplication
-
-| Concern | `EmailListScreen` | `CombinedInboxScreen` | `SearchScreen` / `AddressEmailsScreen` |
-| --- | --- | --- | --- |
-| Tile widget | `EmailThreadTile` + `ThreadTile` (search) | `EmailThreadTile` | `ThreadTile` / inline `ListTile` |
-| Selection set | thread + per-email | thread only | none |
-| Selection AppBar/BottomBar | full set of 5 actions | archive + delete only | n/a |
-| Swipe dismiss | archive/delete + undo | archive/delete + undo (copy) | none |
-| Batch actions | archive, delete, spam, move, snooze | archive, delete (re-implemented) | none |
-
-Tile widgets `lib/ui/widgets/email_thread_tile.dart` and `lib/ui/widgets/thread_tile.dart` render the same fields in slightly different layouts.
-
-## Target architecture
-
-### 1. New widget `lib/ui/widgets/email_thread_list.dart`
-
-A self-contained `ConsumerStatefulWidget` that owns selection state and renders the list. API:
-
-```dart
-EmailThreadList({
-  required Stream<List<EmailThread>> threads,        // folder + combined inbox
-  // or:
-  required List<EmailThread> staticThreads,          // search / address results
-  required EmailListContext listContext,             // see below
-  bool showAccountLabel = false,                     // combined inbox
-  bool showLocationLabel = false,                    // search / cross-mailbox
-  bool enableSwipe = true,
-  bool enablePagination = true,
-  List<EmailBatchAction> actions = EmailBatchAction.standard,
-  ValueChanged<EmailThread>? onTap,                  // null → default navigation
-})
-```
-
-`EmailListContext` carries `accountId?` (nullable for combined/global views) and `mailboxPath?` (nullable for combined/global views). Batch actions read these to scope role lookups and undo-action source paths; when null they fall back to per-thread `t.accountId` / `t.mailboxPath` (this is how `CombinedInboxScreen._batchArchive` already groups by account).
-
-Encapsulated:
-- `_selectedThreadIds` plus toggle/clear/select-all helpers.
-- `_currentThreads` (last stream emission).
-- `_limit` pagination with `Load more`.
-- Selection-mode `AppBar` and `BottomAppBar` rendering — driven by the host scaffold via two builders the widget exposes (`buildSelectionAppBar`, `buildSelectionBottomBar`) so the host keeps Scaffold ownership but doesn't reimplement them.
-- Swipe-to-archive / swipe-to-delete + undo push.
-
-### 2. Shared action layer `lib/ui/screens/email_action_helpers.dart`
-
-Extend the existing file with the batch ops currently duplicated:
-
-```dart
-enum EmailBatchAction { archive, delete, markSpam, move, snooze }
-
-Future<void> batchMoveToRole(WidgetRef ref, BuildContext ctx, {
-  required List<EmailThread> threads,
-  required String role,
-  required String dialogTitle,
-  required String createFolderName,
-});
-
-Future<void> batchDelete(WidgetRef ref, {required List<EmailThread> threads});
-Future<void> batchMove(WidgetRef ref, BuildContext ctx, {required List<EmailThread> threads});
-Future<void> batchSnooze(WidgetRef ref, BuildContext ctx, {required List<EmailThread> threads});
-Future<void> swipeDismiss(WidgetRef ref, EmailThread thread, DismissDirection dir);
-```
-
-Each function fetches `originalEmails`, runs the repo calls, and pushes a single `UndoAction`. Grouping by `accountId` lives here so combined-inbox-style multi-account selections work for every action (not only archive/delete). `_batchMoveToRole` from `EmailListScreen` and `_batchArchive` from `CombinedInboxScreen` collapse into one function.
-
-### 3. Unify the tile widgets
-
-Keep `ThreadTile` (`lib/ui/widgets/thread_tile.dart`) as the single tile. Move the `Dismissible` wrapper out — `EmailThreadList` owns swipe — and add the optional `showAccount` subtitle currently in `EmailThreadTile`. Delete `lib/ui/widgets/email_thread_tile.dart`.
-
-## Screen refactors
-
-- `combined_inbox_screen.dart` — drop selection state, swipe handler, batch methods, `_buildThreadList`, `_selectionBottomBar`. Replace body with `EmailThreadList(stream: emailRepo.observeAllInboxThreads(...), listContext: const EmailListContext.allAccounts(), showAccountLabel: true)`. AppBar/drawer/FAB stay.
-- `email_list_screen.dart` — keep search-bar, sync banner, folder drawer, `Mark all as read`. Replace `_buildStreamBody` and `_buildEmailList` with `EmailThreadList`. Drop selection state, `_toggleThreadSelection`, `_selectionBottomBar`, `_batch*` methods, `_onSwipeDismissed`. The search path inside this screen (results from `searchEmails`) becomes `EmailThreadList(staticThreads: results.map(EmailThread.fromEmail).toList(), enableSwipe: false)` — the per-email vs per-thread split goes away once everything is treated as a thread of one.
-- `search_screen.dart` — Messages section uses `EmailThreadList(staticThreads: ..., enableSwipe: false, showLocationLabel: true, actions: EmailBatchAction.standard)`, so global search results now support the same selection + batch actions. Folders and Addresses sections unchanged.
-- `address_emails_screen.dart` — replace inline `ListView.builder` with `EmailThreadList`, gaining selection/swipe/batch parity.
-
-## Migration steps
-
-1. Add `EmailThreadList` widget with selection, swipe, pagination, and AppBar/BottomBar builders. Lift the existing logic verbatim from `EmailListScreen` so behaviour is unchanged.
-2. Promote the five batch ops + swipe handler into `email_action_helpers.dart`; switch `EmailListScreen` to call them. Keep tile tests passing.
-3. Fold `showAccount` and `Dismissible`-out into `ThreadTile`; delete `EmailThreadTile`; update imports.
-4. Migrate `CombinedInboxScreen` to `EmailThreadList`. Combined inbox now supports spam/move/snooze (was missing). Verify multi-account batches still group correctly.
-5. Migrate the search-result branch inside `EmailListScreen`, then the Messages section of `SearchScreen`, then `AddressEmailsScreen`.
-6. Run `flutter analyze` and the integration tests under `integration_test/` (folder, combined inbox, and search exercise the same code path now, so a single regression test set covers all surfaces).
-
-## Out of scope
-
-- `ThreadDetailScreen` — single-thread message list, intentionally different.
-- Repository / DB code in `lib/core/repositories/` — no changes; the unification is purely on the UI layer.
-- Folder drawer, sync banner, search bar — remain owned by their hosting screens.
-
-## Risks / open questions
-
-- Combined inbox currently has no `mailboxPath` per selection — confirmed handled by grouping selected threads by `accountId` then looking up archive/junk/etc. per group. The same grouping must work for the move/snooze sheet (the destination picker needs an account; when multiple accounts are selected, prompt per-account or block — recommend block with a SnackBar to mirror existing per-folder constraints; flag for user feedback).
-- Snooze for cross-account selection: same constraint as above — implementation should iterate accounts.
-- Swipe-dismiss in search results: currently disabled in `SearchScreen` and `AddressEmailsScreen`. Plan keeps `enableSwipe: false` for those to avoid disorienting users when a swiped item disappears from a filtered list; revisit if user wants parity.

ISSUE-539.md

@@ -0,0 +1,72 @@
+## Background
+
+The issue is real but the proposed mechanism (predicted-mail table + Message-ID match at next sync) is more complex than needed. Two IMAP facilities cover this directly:
+
+- **RFC 4315 / 6851 — `UIDPLUS` + `MOVE` response code `COPYUID`**: the server returns the destination UID(s) inline with the `MOVE` (or `COPY`+`EXPUNGE`) response. `enough_mail`'s `GenericImapResult.responseCodeCopyUid` already parses this (`/data/home/.pub-cache/hosted/pub.dev/enough_mail-2.1.7/lib/src/imap/response.dart:53`). Stalwart and every modern IMAP server advertise UIDPLUS. We currently throw this away — `client.uidMove(...)` is called for its side effect and the result is ignored (`lib/data/repositories/email_repository_impl.dart:2320,2335,2354`).
+- **Fallback — `UID SEARCH HEADER Message-ID …`**: bounded, deterministic, works on every IMAP server. Needed only when `responseCodeCopyUid` is missing.
+
+The undo path already attempts a Message-ID recovery (`lib/core/services/undo_service.dart:78-84`), and `findEmailByMessageId` exists (`lib/data/repositories/email_repository_impl.dart:1899`), so the building blocks are in place; we just need to remap on the write path instead of guessing on the read path.
+
+There is also a latent race: the row is optimistically moved (`mailbox_path = dest`) before the server flush, but the row still carries the source UID. If `_reconcileDeletedImap` runs against the **destination** mailbox between the optimistic write and the flush, it will not find that UID on the server and will hard-delete the row (`lib/data/repositories/email_repository_impl.dart:758-761`).
+
+## Goal
+
+After an IMAP move, the local email row keeps tracking the same physical message under its new UID, without orphaning, without re-fetching, and without breaking UndoLog references.
+
+## Concrete changes
+
+### 1. Capture the new UID inside the flush
+`lib/data/repositories/email_repository_impl.dart` — `_applyPendingChangeImap`, cases `move`, `snooze`, `unsnooze`:
+
+- Capture the `GenericImapResult` returned by `client.uidMove(...)`.
+- Read `result.responseCodeCopyUid`. If non-null, zip `sourceSequence` ↔ `targetSequence` to map `oldUid → newUid`.
+- If null (no UIDPLUS), look up the row's `messageId`. If present: `client.selectMailboxByPath(dest)` then `client.uidSearchMessages(searchCriteria: 'HEADER Message-ID "<id>"')` and take the highest UID. If `messageId` is also missing, log a warning and leave the row to be reconciled out on the next pass (worst case: cache miss, body refetch).
+- For each old/new pair, call a new helper `_remapEmailAfterMove(row, newUid, destPath)`.
+
+### 2. New helper `_remapEmailAfterMove` (same file)
+
+Composes `newId = "accountId:destPath:newUid"`, then in one Drift transaction:
+- `UPDATE email_bodies SET email_id = newId WHERE email_id = oldId`
+- `UPDATE emails SET id = newId, uid = newUid, mailbox_path = destPath WHERE id = oldId`
+- Patch any `threads.email_ids_json` containing `oldId` (reuse the JSON helper from the v41 migration path, or just rebuild affected threads via `_updateThread`).
+- `UPDATE pending_changes SET resource_id = newId WHERE resource_id = oldId` (in case multiple mutations are queued).
+- Call `UndoRepository.remapEmailId(oldId, newId)` (new method, see §3).
+
+`PRAGMA defer_foreign_keys = ON` for the transaction, mirroring the v41 migration pattern.
+
+### 3. UndoLog remap
+
+- `lib/data/repositories/undo_repository_impl.dart`: add `Future<void> remapEmailId(String oldId, String newId)` that loads each `undo_actions` row, rewrites `emailIds[]` entries equal to `oldId` and `originalEmails[].id` entries equal to `oldId`, and re-saves.
+- `lib/core/repositories/undo_repository.dart`: add the interface method.
+- Inject `UndoRepository` into `EmailRepositoryImpl` via DI (already a singleton in `lib/di.dart`) so `_remapEmailAfterMove` can call it.
+- The fallback path in `UndoService.undo` (the Message-ID lookup) becomes a defensive backstop and can stay as-is.
+
+### 4. Reconciliation guard
+
+`_reconcileDeletedImap` (`lib/data/repositories/email_repository_impl.dart:731`): before deleting a row whose UID is absent from the server, query `pending_changes` for a `move`/`snooze`/`unsnooze` row with `resource_id = row.id`. If one exists, skip the delete — the row is mid-flight. Since flush always runs before sync (`SYNC.md:199-200`) the race is narrow but real on retries when flush failed mid-cycle.
+
+### 5. Tests
+
+- `test/unit/fake_imap.dart`: make `uidMove` return a `GenericImapResult` whose `responseCode` is a configurable `"COPYUID <validity> <src> <dest>"` string; add a UIDPLUS-off mode that returns no response code and serves `uidSearchMessages` with a HEADER predicate.
+- New `test/unit/repositories/email_repository_move_test.dart`:
+  - Enqueue a move → flush → assert `emails`, `email_bodies`, `threads`, `pending_changes`, `undo_actions` all carry the new id.
+  - Same with UIDPLUS-off + HEADER-search fallback.
+  - Move with no Message-ID and no COPYUID → row is unchanged, warning logged, next reconciliation does not delete it because pending change is still queued.
+- New test in the same file: `_reconcileDeletedImap` skips a row that has a queued `move`.
+- Extend `test/integration/concurrent_sync_test.dart` (or sibling file) with a real Stalwart move-then-sync-then-undo cycle: assert the row keeps its body, the new UID matches what `UID SEARCH` returns, and undo restores it to the source mailbox.
+
+### 6. Docs
+
+- `SYNC.md` §3: note that IMAP moves remap the local id from the `COPYUID` response (UIDPLUS) or `UID SEARCH HEADER Message-ID` fallback, so `email_bodies` and `undo_actions` stay valid across moves.
+- `DB-SYNC.md` IMAP bullets: add "IMAP move remap: local row keeps its body/undo references via `COPYUID` (RFC 4315) or Message-ID header search."
+
+## Non-goals
+
+- No new "predicted mail" table — `COPYUID` + header search is reliable and stateless.
+- No change to JMAP moves (JMAP IDs are mailbox-independent).
+- No change to `uidValidity` handling — orthogonal.
+- No CONDSTORE/QRESYNC — already listed as future work in `DB-SYNC.md`.
+
+## Open question for review
+
+Should the fallback header-search be best-effort-quiet (current plan) or should it surface a `FailedMutation` when both `COPYUID` and `Message-ID` are missing? I lean quiet because the next full sync will still pull the message under its new UID — we'd just lose the cached body and any queued mutations on top of it. Happy to escalate to `FailedMutation` if you'd rather make the data loss visible.