plan: refresh plan for issue #474

.pre-commit-config.yaml

@@ -53,9 +53,3 @@ repos:
         entry: bash -c 'cd "$(git rev-parse --show-toplevel)" && nix develop --command task check-ci-images'
         pass_filenames: false
         files: ^(ci/main\.go|\.fvmrc)$
-      - id: dagger-versions-aligned
-        name: verify Dagger version is consistent across dagger.json, flake.nix, Dockerfile and DAGGER.md
-        language: system
-        entry: bash -c 'cd "$(git rev-parse --show-toplevel)" && scripts/check_dagger_versions.sh'
-        pass_filenames: false
-        files: ^(ci/dagger\.json|flake\.nix|\.forgejo/Dockerfile|DAGGER\.md)$

ISSUE-474.md

@@ -0,0 +1,42 @@
+## Goal
+
+Make each email row in **Undo Log Detail** (`lib/ui/screens/undo_log_detail_screen.dart`) tappable, navigating to that email's current location in the app. The issue gates this on "when structured search is implemented" — structured search now exists (`lib/core/filter/filter_expression.dart`, `lib/ui/screens/search_screen.dart`), and the repository already exposes `findEmailByMessageId(accountId, messageId)` (`lib/data/repositories/email_repository_impl.dart:1899`), so we have the building block needed to locate an email regardless of moves.
+
+## Why direct lookup, not a search-screen handoff
+
+After a Move, the email lives at `destinationMailboxPath` with a *new* UID, so the `Email.id` stored in `UndoAction.originalEmails` is stale. The stable identifier across moves is `messageId`. `findEmailByMessageId` returns the current row (with current `mailboxPath` and `id`), giving us a one-tap deep link to the email detail screen — better UX than dumping the user into search.
+
+## Implementation
+
+### 1. `lib/ui/screens/undo_log_detail_screen.dart`
+
+- Convert `_EmailTile` from `StatelessWidget` to `ConsumerWidget` so it can read `emailRepositoryProvider`.
+- Take `accountId` as an additional ctor arg (passed in from the `originalEmails.map` call site so we don't depend on the email's own field, matching how the action scopes lookups).
+- Add an `onTap` handler:
+  1. If `email.messageId == null` → no-op tap, show `SnackBar('Cannot locate this email — no Message-ID.')`.
+  2. Otherwise call `ref.read(emailRepositoryProvider).findEmailByMessageId(action.accountId, email.messageId!)`.
+  3. On hit, `context.go('/accounts/${accountId}/mailboxes/${Uri.encodeComponent(found.mailboxPath)}/emails/${Uri.encodeComponent(found.id)}')` — matches the encoding pattern used in `combined_inbox_screen.dart:280` and `email_list_screen.dart:540`.
+  4. On miss, show `SnackBar('Email no longer exists at its previous location. Use Undo to restore it.')` — covers the hard-deleted case and the not-yet-resynced case.
+- Add `trailing: const Icon(Icons.chevron_right)` to give the row a "navigates" affordance consistent with other tappable `ListTile`s in the app.
+- Leave styling otherwise unchanged; the existing `Icons.email_outlined` leading + subject/sender layout stays.
+
+### 2. No router changes
+
+The existing email-detail route (`router.dart:153`) is reused as-is.
+
+### 3. No model / repository changes
+
+`findEmailByMessageId` is already on `EmailRepository` and scoped per account, which is what we want.
+
+### 4. Tests — `test/widget/`
+
+Add a new widget test `test/widget/undo_log_detail_screen_test.dart` covering:
+- Tapping a row whose `messageId` resolves via a fake `EmailRepository` navigates to `/accounts/<acc>/mailboxes/<encoded-path>/emails/<encoded-id>` (assert via a `GoRouter` test harness similar to `test/widget/email_detail_screen_test.dart`).
+- Tapping a row when `findEmailByMessageId` returns `null` shows the "no longer exists" SnackBar and does not navigate.
+- Tapping a row with `messageId == null` shows the "no Message-ID" SnackBar.
+
+## Out of scope
+
+- Adding Message-ID as a structured `FilterField` — not needed for direct navigation; can be revisited if a UI for "search for this email" is ever wanted.
+- Changing the Undo Log list screen (`undo_log_screen.dart`) — the issue is specifically about the *detail* screen.
+- Persisting/refreshing a stale `originalEmails` list — Move/Snooze update the row in place, so subsequent re-lookups by Message-ID will find them; nothing to maintain.

ISSUE-484.md

@@ -1,100 +0,0 @@
-## Root cause analysis
-
-The "Load remote images" button is rendered in two places: `lib/ui/screens/email_detail_screen.dart:228-262` (single mail view) and `lib/ui/screens/thread_detail_screen.dart:203-237` (thread view). Both call the same pattern:
-
-```dart
-onPressed: () {
-  setState(() => _loadRemoteImages = true);   // 1. schedule rebuild
-  if (senderEmail != null) {
-    unawaited(...addTrustedImageSender(senderEmail));  // 2. fire-and-forget DB write
-    ScaffoldMessenger.of(ctx).showSnackBar(SnackBar(   // 3. queue snack bar
-      duration: const Duration(seconds: 3),
-      ...
-    ));
-  }
-}
-```
-
-Although `duration: 3s` is already set, the snack bar fails to auto-dismiss. This mirrors the bug fixed in PR #401 (issue #399): there, a snack bar fired during a navigation transition and the duration timer "didn't start correctly" because the snack bar was queued on an unstable scaffold.
-
-Here, the analogous instability comes from three rebuilds that all land between `showSnackBar` and the moment the SnackBar's enter-animation would normally complete and start its dismiss timer:
-
-1. The synchronous `setState` flips `_loadRemoteImages` → `true`, which immediately removes the "Load remote images" button (the very widget whose `onPressed` was running) and swaps the `SecureEmailWebView` into the rebuilt subtree with `loadRemoteImages: true`. The WebView's `didUpdateWidget` then triggers an async `loadHtmlString` reload (see `lib/ui/widgets/secure_email_webview.dart:100-106`), which subsequently calls `setState(() => _height = h)` inside `_measureHeight`.
-2. The fire-and-forget `addTrustedImageSender` write resolves a moment later, the `trustedImageSendersProvider` stream emits, and `ref.watch(trustedImageSendersProvider)` in `email_detail_screen.dart:197` causes another rebuild of the whole screen body — including the `Scaffold`'s body subtree that hosts the snack bar overlay's host context.
-3. These rebuilds happen during the SnackBar's enter animation, so the `_SnackBarState` ends up holding stale animation state and the per-snack-bar timer that schedules `hideCurrentSnackBar` after `duration` never fires.
-
-## Plan
-
-### Fix
-
-Queue the snack bar **before** mutating state, so it reaches `ScaffoldMessenger` while the Scaffold subtree is still stable, and defer the state change to a post-frame callback so the snack bar's enter-animation can finish before the WebView reload and the provider-driven rebuild run.
-
-In `lib/ui/screens/email_detail_screen.dart`, replace the body of `OutlinedButton.icon.onPressed` at lines 231-261 with:
-
-```dart
-onPressed: () {
-  if (senderEmail != null) {
-    unawaited(
-      ref
-          .read(userPreferencesRepositoryProvider)
-          .addTrustedImageSender(senderEmail),
-    );
-    ScaffoldMessenger.of(ctx).showSnackBar(
-      SnackBar(
-        duration: const Duration(seconds: 3),
-        content: const Text(
-          'Images will be loaded automatically for this sender.',
-        ),
-        action: SnackBarAction(
-          label: 'View',
-          onPressed: () {
-            if (mounted) {
-              unawaited(
-                context.push(
-                  '/accounts/trusted-senders',
-                  extra: senderEmail,
-                ),
-              );
-            }
-          },
-        ),
-      ),
-    );
-  }
-  WidgetsBinding.instance.addPostFrameCallback((_) {
-    if (mounted) setState(() => _loadRemoteImages = true);
-  });
-},
-```
-
-Apply the same reordering to `lib/ui/screens/thread_detail_screen.dart:206-236`.
-
-The key changes:
-- `showSnackBar` runs first, on the still-stable scaffold subtree.
-- `setState` (which triggers WebView swap-in and subsequent rebuilds) is deferred to a post-frame callback.
-- When `senderEmail == null` (no trusted-sender to register, so no snack bar), the post-frame callback still flips `_loadRemoteImages` to true — preserving existing behavior of the button working even for unknown senders.
-
-### Tests
-
-Add a widget test in `test/widget/email_detail_screen_test.dart` that:
-
-1. Pumps an `EmailDetailScreen` with an HTML body and a non-empty `From` header.
-2. Taps the "Load remote images" button.
-3. Verifies the snack bar with text "Images will be loaded automatically for this sender." appears.
-4. Calls `tester.pump(const Duration(seconds: 4))` (or uses `tester.pumpAndSettle` after a 3.5s pump).
-5. Verifies the snack bar is gone (`expect(find.byType(SnackBar), findsNothing)`).
-6. Verifies `_loadRemoteImages` did flip, by checking that the "Load remote images" button is no longer present.
-
-Add an analogous test in `test/widget/thread_detail_screen_test.dart` (or wherever thread tests live; create the file if it does not exist yet — use the email_detail test as a template).
-
-### Out of scope
-
-- The "First update agent loop, fix search bug" line in the issue body is two unrelated todo notes the reporter jotted down (the search bug is tracked separately). This plan does not address them.
-- Other `showSnackBar` call sites in `email_detail_screen.dart` (download success/failure, copy-to-clipboard, raw-email errors, etc.) are not affected by the same rebuild pattern and stay unchanged.
-
-### Verification checklist
-
-- [ ] `dart test` (or the project's `task test` equivalent) passes, including the two new widget tests.
-- [ ] Manual: open a single mail in `EmailDetailScreen` with HTML body from a sender not yet trusted; tap "Load remote images"; verify snack bar appears, images load, and snack bar disappears after ~3 seconds.
-- [ ] Manual: tap "View" on the snack bar before it dismisses; verify it navigates to `/accounts/trusted-senders` and that the snack bar is dismissed by the navigation as expected.
-- [ ] Manual: repeat in `ThreadDetailScreen`.

Taskfile.yml

@@ -712,11 +712,6 @@ tasks:
     cmds:
       - scripts/check_ci_images.sh
 
-  check-dagger-versions:
-    desc: Verify ci/dagger.json, flake.nix, .forgejo/Dockerfile and DAGGER.md pin the same Dagger version
-    cmds:
-      - scripts/check_dagger_versions.sh
-
   _integrations:
     internal: true
     run: once

flake.nix

@@ -49,16 +49,14 @@
           '';
         };
 
-        # The dagger/nix flake's Nix wrapper is a broken self-exec loop, so we
-        # fetch the CLI binary directly.  Keep this version in lockstep with
-        # ci/dagger.json (engineVersion) and .forgejo/Dockerfile (DAGGER_VERSION) —
-        # scripts/check_dagger_versions.sh enforces this.
-        daggerCli = pkgs.stdenv.mkDerivation {
+        # The dagger/nix flake pins 0.20.8, whose Nix wrapper is a broken self-exec
+        # loop.  Fetch 0.21.4 directly so the pre-commit dart-check hook can run.
+        dagger021 = pkgs.stdenv.mkDerivation {
           pname = "dagger";
-          version = "0.20.8";
+          version = "0.21.4";
           src = pkgs.fetchurl {
-            url = "https://dl.dagger.io/dagger/releases/0.20.8/dagger_v0.20.8_linux_amd64.tar.gz";
-            sha256 = "1ns6wq2z1skd2fq9lbrcali0s8kn24p3haamnjjgchg6zlv6b960";
+            url = "https://dl.dagger.io/dagger/releases/0.21.4/dagger_v0.21.4_linux_amd64.tar.gz";
+            sha256 = "0wlnbr4g5069755131yjp2a6alacn64f1c8b27xn0cbynq3zicjd";
           };
           sourceRoot = ".";
           installPhase = ''
@@ -71,7 +69,7 @@
         devShells.default = pkgs.mkShell {
           buildInputs = with pkgs; [
             # Dagger CLI
-            daggerCli
+            dagger021
 
             # Go compiler — for Dagger development
             go

scripts/check_dagger_versions.sh

@@ -1,49 +0,0 @@
-#!/usr/bin/env bash
-# Verify that the Dagger version is consistent across the project.
-#
-# The Dagger CLI must speak the same protocol as the engine it talks to.  We
-# pin the version in four places (engine image in DAGGER.md, the CLI in
-# flake.nix, the CLI in the Forgejo runner Dockerfile, and the module
-# engineVersion in ci/dagger.json).  This script fails if any of them drift.
-set -euo pipefail
-
-ROOT=$(git rev-parse --show-toplevel)
-
-# ci/dagger.json — strip leading "v" for comparison.
-dagger_json=$(grep -oE '"engineVersion"[[:space:]]*:[[:space:]]*"[^"]+"' "$ROOT/ci/dagger.json" \
-  | sed -E 's/.*"v?([^"]+)"$/\1/')
-
-# flake.nix — the dagger021 derivation's CLI download URL.
-flake_nix=$(grep -oE 'dagger_v[0-9]+\.[0-9]+\.[0-9]+_linux' "$ROOT/flake.nix" \
-  | head -n1 \
-  | sed -E 's/dagger_v([0-9.]+)_linux/\1/')
-
-# .forgejo/Dockerfile — DAGGER_VERSION env on the install line.
-dockerfile=$(grep -oE 'DAGGER_VERSION=[0-9]+\.[0-9]+\.[0-9]+' "$ROOT/.forgejo/Dockerfile" \
-  | head -n1 \
-  | cut -d= -f2)
-
-# DAGGER.md — engine image tag in the example systemd unit.
-dagger_md=$(grep -oE 'dagger/nix/v[0-9]+\.[0-9]+\.[0-9]+' "$ROOT/DAGGER.md" \
-  | head -n1 \
-  | sed -E 's@.*/v@@')
-
-printf 'ci/dagger.json    engineVersion = v%s\n' "$dagger_json"
-printf 'flake.nix         dagger021     = %s\n'  "$flake_nix"
-printf '.forgejo/Dockerf. DAGGER_VERSION= %s\n'  "$dockerfile"
-printf 'DAGGER.md         engine tag    = v%s\n' "$dagger_md"
-
-for v in "$flake_nix" "$dockerfile" "$dagger_md"; do
-  if [ -z "$v" ]; then
-    echo "ERROR: failed to parse a Dagger version reference." >&2
-    exit 1
-  fi
-  if [ "$v" != "$dagger_json" ]; then
-    echo "" >&2
-    echo "ERROR: Dagger versions are out of sync." >&2
-    echo "  Align ci/dagger.json, flake.nix, .forgejo/Dockerfile and DAGGER.md to the same version." >&2
-    exit 1
-  fi
-done
-
-echo "Dagger versions aligned (v$dagger_json)."