plan: refresh plan for issue #535

ISSUE-535.md

@@ -0,0 +1,69 @@
+## Background — what already exists in this repo
+
+The Android AAB build-and-upload pipeline is already fully automated. The user does **not** need to drop a file into Play Console at all — the same machinery that publishes to `internal` can publish to the closed-testing track.
+
+- `ci/main.go:952` — `PublishAndroid()` builds a release AAB, stamps the versionCode (`time.Now().Unix()`), re-signs with the upload key, and uploads to Play Store.
+- `ci/main.go:899` — `UploadToPlayStore()` invokes `scripts/deploy_playstore.py` inside a container.
+- `scripts/deploy_playstore.py:14` — pinned to `TRACK = "internal"`. This is the only thing limiting the existing pipeline to internal testing.
+- `Taskfile.yml:230` — `task publish-android` wraps the Dagger call.
+- `.forgejo/workflows/deploy.yml` — `deploy-playstore` job runs every hour (`0 * * * *`) and calls `task publish-android` whenever android-relevant files changed since the last successful deploy.
+- `Taskfile.yml:214` — `task build-android-bundle` builds an **unsigned** AAB locally to `build/app/outputs/bundle/release/app-release.aab`. The signed AAB built by `PublishAndroid` lives only inside Dagger and is **not** currently exported to disk on the runner.
+
+## Recommended path (A): publish to the closed-testing track from CI — no manual upload
+
+This piggybacks on what's already running every hour.
+
+1. **In Play Console**, decide which track maps to "closed testing":
+   - The built-in `alpha` track is treated as closed testing by the API.
+   - Or create a custom closed-testing track (e.g. `closed-testers`) under Testing → Closed testing → Create track. Configure the testers list there. Note the track ID exactly as it appears in the URL/console.
+
+2. **Edit `scripts/deploy_playstore.py`** so the upload goes to both internal and the closed track in the same Play edit (one commit, atomic). Concretely:
+   - Replace `TRACK = "internal"` (line 14) with `TRACKS = ["internal", "alpha"]` (or `["internal", "<custom-track-id>"]`).
+   - Replace the single `track_resp = session.put(... /tracks/{TRACK} ...)` block (lines 97-101) with a loop that PUTs the same `versionCodes: [version_code]` payload to each track in `TRACKS` before the commit at line 104.
+   - Internal stays as the smoke-test target; the closed track is what gets the testers.
+
+3. **Update the test fixture** in `scripts/test_deploy_playstore.py` (and any test for `verify_playstore_deploy.py`) to expect the new track list. Run `python3 scripts/test_deploy_playstore.py` locally.
+
+4. **Trigger a deploy**: push the change to `main` and either wait for the next hourly run or `workflow_dispatch` `.forgejo/workflows/deploy.yml` from the Forgejo UI. Note that `deploy.yml:114` already includes `scripts/deploy_playstore\.py` in the change-detection regex, so any normal hourly tick after the merge will pick it up.
+
+5. **Result**: closed-testing release appears in Play Console with the AAB pre-attached. Reviewers only need to edit the release notes (already generated by `task generate-changelog` — see `Taskfile.yml:232`) and click "Roll out". No "Drop app bundles here" step.
+
+Caveat — first time a track is used, Google requires that internal testing has been previously published; that is already true here, so no blocker.
+
+## Alternative path (B): one-shot manual drop — export the signed AAB as a CI artifact
+
+Use this if you want to do this single closed-testing release by hand and decide on full automation later.
+
+1. **Modify `ci/main.go`** to expose the signed AAB as a file return: add a thin wrapper exporting the result of `SignAndroidBundle(StampAndroidVersionCode(BuildAndroidRelease(commitHash), versionCode), ...)` as `*dagger.File` (something like `SignedAndroidBundle(...) *dagger.File`). The existing `PublishAndroid` discards the signed bundle after upload, so a separate entry point is cleaner than reshaping it.
+
+2. **Add a `Taskfile.yml` target** `build-android-bundle-signed` that calls the new Dagger function with `-o build/app/outputs/bundle/release/app-release.aab` (mirroring `build-android-bundle` at `Taskfile.yml:218`).
+
+3. **Add to `.forgejo/workflows/deploy.yml` `deploy-playstore` job** (after `task publish-android`) — or to a new dedicated job — these two steps:
+   ```yaml
+   - name: Export signed AAB locally
+     env: { DAGGER_NO_NAG: "1" }
+     run: task build-android-bundle-signed
+   - name: Upload AAB artifact
+     uses: actions/upload-artifact@v4
+     with:
+       name: app-release-aab
+       path: build/app/outputs/bundle/release/app-release.aab
+       if-no-files-found: error
+   ```
+
+4. **Trigger the workflow** (`workflow_dispatch`), then download `app-release-aab` from the run's artifacts page on Forgejo. Drop it into Play Console.
+
+Risk to flag: the version code stamped via the artifact path must be strictly greater than every version code already on internal. Since the same Unix-timestamp scheme is used everywhere this naturally holds, but if the artifact run and a regular publish run race, the closed release may fail with "Version code N has already been used." Easy fix: only run one or the other.
+
+## Alternative path (C): build locally — fastest one-off
+
+```bash
+export ANDROID_KEYSTORE_BASE64=...     # same secret used by CI
+bash scripts/build_android_bundle_local.sh
+```
+
+AAB lands at `build/app/outputs/bundle/release/app-release.aab`. Note this script uses `fvm flutter build appbundle` directly and does **not** match the Dagger pipeline's stamp/sign step exactly — the keystore path is wired via `ANDROID_KEYSTORE_PATH` and the signing config in `android/app/build.gradle.kts` reads from there. Verify the bundle is correctly signed (`jarsigner -verify -verbose -certs`) before uploading.
+
+## Recommendation
+
+Go with path A. The diff is small (~10 lines in `deploy_playstore.py` + test update), reuses the build-and-sign pipeline that has already been proven in `deploy.yml` for months, and removes the "Drop app bundles here" step entirely instead of just satisfying it once.

ISSUE-539.md

@@ -1,72 +0,0 @@
-## 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.