feat(downloads): add pause and resume support for attachment downloads

[syed-tp]

• Integrate background_downloader for pausing, resuming, and persisting attachments and PDF lessons.
• Add system progress notifications, background Range request handling, and SQLite taskId tracking.
• Update lesson detail and downloads screens with interactive pause, resume, and downloaded state indicators.

[claude]

Claude finished @syed-tp's task in 7m 11s —— View job

---

Todo

• Read AGENTS.md and core docs (ai_context.md, architecture.md)
• Check ADRs (no package-boundary/architecture ADR relevant to this change)
• Fetch full PR diff
• Review OpenSpec compliance
• Review SDK boundary enforcement
• Review design system & widget usage
• Review accessibility semantics
• Review motion & animation
• Review platform neutrality
• Review navigation pattern
• Review Riverpod provider pattern
• Review data layer architecture
• Review generated files (.g.dart)
• Review dependency management
• Review general code quality
• Post final review

PR Review Summary

✅ What's good

• OpenSpec change (openspec/changes/attachment-pause-resume/) is thorough — proposal, design (with alternatives considered), and a scoped spec delta. Satisfies the spec-driven-development requirement in AGENTS.md.
• Layering is respected: DownloadsService stays a pure SDK-call worker with no DB knowledge, DownloadsRepository owns all Drift reads/writes — consistent with the data-layer architecture rules.
• No SDK boundary or app/ import violations; no Material/Cupertino widgets or hardcoded design tokens introduced.
• app_database.g.dart diff cleanly matches the new taskId column in downloads_table.dart — no evidence of hand-editing a generated file.
• New Pause/Resume controls in attachment_viewer.dart use AppButton, which already wraps AppSemantics.button internally, so the main new interactive elements stay accessible.

🟡 WARNING — Should fix

1. Attachments may no longer save to the public Downloads folder. FileDownloader.getDirectory(StorageType.publicDownload) (packages/core/lib/network/file_downloader.dart) was changed from resolving /storage/emulated/0/Download (with a getDownloadsDirectory() fallback) to unconditionally returning getApplicationDocumentsDirectory(). That's app-private storage. The enum's own doc comment still says "Public user-accessible storage... visible in the system Downloads folder", and scanMediaIfAndroid() is still called on completion expecting the file to show up in the Android Files app. This looks like it was done so getLocalPath() agrees with where background_downloader actually saves files (bg.BaseDirectory.applicationDocuments in downloads_service.dart), but it's an undocumented, user-facing regression (downloaded files disappear from the public Downloads folder) — not mentioned in design.md. Please confirm this is intentional, and if so update the stale doc comment/MediaScanner expectations.
2. Schema change without a version bump — confirm the "pre-production" assumption still holds. taskId was added to DownloadsTable, but schemaVersion stays at 1 (packages/core/lib/data/db/app_database.dart), so onUpgrade never runs for anyone already on v1. design.md/tasks.md explicitly justify this as safe "because the app has not been deployed to production" and flag a fallback (bump to 2 + addColumn) if that's no longer true. Worth a final sanity check at merge time — if any build has already shipped to real users, this will crash existing installs with no such column: task_id.
3. Silent error swallowing on download completion — in downloads_repository.dart's bg.TaskStatusUpdate handler, the bg.TaskStatus.complete branch wraps filePath()/file-stat/scanMediaIfAndroid in try { ... } catch (_) {} with no Sentry logging, unlike the rest of this file/service which consistently reports via _sentryService.captureException. If it throws, the row is marked completed with filePath left null and no diagnostic trail — silently breaking _openFile()/resume flows for that item.
4. No explicit reconciliation of stuck in-flight downloads on cold start. synchronize() (downloads_repository.dart) only prunes attachment rows whose files are missing; it never queries background_downloader for the real status of rows still marked downloading/paused. State recovery relies entirely on the passive attachmentUpdates stream re-firing after relaunch. tasks.md (4.5) claims this reconnection was implemented — worth double-checking a paused/interrupted download actually resyncs correctly after a full process kill + relaunch (task 8.3's manual test), since a missed update would leave a row stuck indefinitely.
5. No automated tests added for a fairly large behavioral surface (status-update branching, pause/resume service calls, PDF background fetch). Verification in tasks.md is manual-only (8.1–8.5).

🔵 SUGGESTION — Nice to have

1. New strings in attachment_viewer.dart ('Pause', 'Resume Download', 'Paused', 'Paused • $progress%') are hardcoded rather than going through AppLocalizations — consistent with this file's pre-existing pattern, but worth tracking for localization along with the rest of the file.
2. The new openspec/.../offline-persistence/spec.md scenario "Task ID cleared on completion or deletion" only holds for deletion — on completion, taskId is left populated in the row (harmless today since callers branch on status, but a small drift from the written spec).
3. The new isDownloaded/isDownloading _HeaderButton states in lesson_detail_shell.dart don't wrap with AppSemantics — but this mirrors the sibling "Bookmark"/"Download content" buttons in the same widget, which have the same gap, so it's pre-existing debt rather than a regression.

Verdict

CHANGES REQUESTED — mainly to get an explicit answer on the public-storage location change (#1) and to confirm the schema-version assumption (#2) is still valid before merge; #3–5 are good hardening but not necessarily blocking.

[syed-tp]

@claude review

[claude]

Claude finished @syed-tp's task in 3m 41s —— View job

---

Todo

• Read AGENTS.md and core docs (ai_context.md, architecture.md)
• Check ADRs (no package-boundary/architecture ADR relevant to this change)
• Fetch full PR diff
• Review OpenSpec compliance
• Review SDK boundary enforcement
• Review design system & widget usage
• Review accessibility semantics
• Review motion & animation
• Review platform neutrality
• Review navigation pattern
• Review Riverpod provider pattern
• Review data layer architecture
• Review generated files (.g.dart)
• Review dependency management
• Review general code quality
• Post final review

PR Review Summary

This is a re-review after the previous pass (see prior comment). Diffed origin/main...HEAD.

✅ What's good

• All four blocking items from the previous review round are now addressed:
  • StorageType.publicDownload's doc comment now correctly describes app-private storage (file_downloader.dart), and scanMediaIfAndroid()/MediaScanner calls have been fully removed — no more stale public-Downloads expectations.
  • The bg.TaskStatus.complete branch in downloads_repository.dart (lines ~125-140) now reports failures via _sentryService.captureException instead of a silent catch (_) {}.
  • synchronize() now calls _service.getActiveAttachmentTaskIds() and reconciles rows stuck in downloading back to paused when the OS-level task is no longer active — addressing the cold-start reconciliation gap.
  • A new test file (downloads_repository_test.dart, 187 lines) covers pause/resume and the stuck-download reconciliation logic with a FakeDownloadsService.
• design.md Decision 6 now explicitly documents and justifies the app-private storage migration (previously undocumented).
• SDK boundaries, design-system/widget usage, and generated file (app_database.g.dart) consistency all remain clean — no new violations.
• Android WorkManagerInitializer provider and iOS UIBackgroundModes (fetch, processing) are correctly added to support background_downloader's background execution.

🔴 CRITICAL — Must fix before merge

None found.

🟡 WARNING — Should fix

1. PDF resume after process death likely skips watermarking. DownloadsRepository.resumeDownload() calls _service.resumeAttachmentDownload(taskId, contentUrl) (downloads_service.dart:126-149) directly on the raw background_downloader task — it never re-invokes PdfDownloader.downloadAndWatermark(). Two paths, both problematic if the original app process that started the download is gone (the exact scenario taskId persistence exists to support):
   • If background_downloader still has the paused task, resuming it eventually fires a TaskStatus.complete update. With the original downloadAndWatermark() coroutine dead, only the repository's generic handler (downloads_repository.dart:125-140) observes it — marking the row completed with filePath pointing at the raw, un-watermarked file inside BaseDirectory.temporary, which PdfDownloader normally deletes after watermarking.
   • If the task was lost, the fallback reconstructs a new task with baseDirectory: bg.BaseDirectory.applicationDocuments (the plain-attachment directory, not .temporary) — so the re-downloaded PDF is saved straight into permanent storage, permanently bypassing the watermark step.
     design.md's "PDF partial download on pause" risk note describes the happy path within a single process lifetime, not resume-after-relaunch. Worth a manual test: kill the app mid-PDF-download, relaunch, tap Resume, and confirm the final file is watermarked and lands in the expected location. Fix this →
2. packages/core/pubspec.yaml and packages/courses/pubspec.yaml still declare media_scanner: ^2.2.0, but no code references MediaScanner anymore now that scanMediaIfAndroid() is removed — the dependency is dead weight and can be dropped.
3. New strings 'Downloaded' / 'Downloading' in lesson_detail_shell.dart (~lines 387, 394) and 'Pause' / 'Resume Download' / 'Paused' / 'Paused • $progress%' in attachment_viewer.dart are hardcoded rather than via AppLocalizations — consistent with the pre-existing pattern in the surrounding header-button block ('Bookmark lesson', 'Download content'), so not a new regression, but worth tracking for localization.

🔵 SUGGESTION — Nice to have

1. DownloadsService.deleteDownloadItem()'s attachment branch calls bg.FileDownloader().allTasks() and filters by taskId just to check it exists before calling cancelTaskWithId(item.taskId!) — that lookup is redundant since cancelTaskWithId already no-ops if the task doesn't exist.
2. The offline-persistence spec's "Task ID cleared on completion or deletion" scenario still only holds for deletion — the bg.TaskStatus.complete branch leaves taskId populated on the row (harmless today since callers branch on status, carried over from the previous review round).
3. requestNotificationPermission() swallows all exceptions with an empty catch (_) {}, unlike the rest of the file's consistent _sentryService.captureException pattern.

Verdict

CHANGES REQUESTED — the previous round's blockers are resolved and this is close; mainly need confirmation that PDF pause/resume across an app restart doesn't silently skip watermarking (#1). #2–3 are good hardening but not blocking.

[syed-tp]

@claude review

[claude]

Claude finished @syed-tp's task in 3m 11s —— View job

---

Todo

• Read AGENTS.md and core docs (ai_context.md, architecture.md)
• Check ADRs (no package-boundary/architecture ADR relevant to this change)
• Fetch full PR diff (origin/main...HEAD)
• Review changes since last review round (esp. PDF resume watermark fix)
• Review OpenSpec compliance
• Review SDK boundary enforcement
• Review design system & widget usage
• Review accessibility semantics
• Review data layer architecture
• Review generated files (.g.dart)
• Review dependency management
• Post final review

PR Review Summary

Re-review after the second round. Diffed origin/main...HEAD. New commit since the last review (f47461c) attempted to fix the previously-flagged "PDF resume after app restart skips watermarking" issue — see finding #1, it's not actually fixed for the primary case.

✅ What's good

• The three other WARNING items from the previous round are genuinely resolved:
  • media_scanner dependency fully removed from both packages/core/pubspec.yaml and packages/courses/pubspec.yaml (dead weight cleaned up).
  • requestNotificationPermission() (downloads_service.dart:57-75) now reports failures via _sentryService.captureException(..., level: AppErrorLevel.warning) instead of swallowing silently.
  • taskId is now cleared on completion for both attachments and PDFs (downloads_repository.dart TaskStatus.complete branch, and the final upsertDownload(..., taskId: null, ...) in startWatermarkedPdfDownload), so the OpenSpec scenario drift noted previously is gone.
• synchronize() correctly reconciles stuck downloading rows to paused using getActiveAttachmentTaskIds(), and this is now covered by a passing test.
• No new SDK boundary, design-system, or generated-file violations. attachment_viewer.dart's new Pause/Resume buttons use AppButton (accessible by default), consistent with the rest of the file.

🔴 CRITICAL — Must fix before merge

None found.

🟡 WARNING — Should fix

1. PDF resume-after-pause still skips watermarking — the "fix" only covers an unreachable case. DownloadsRepository.resumeDownload() (downloads_repository.dart:553-566) now branches on item.isWatermarked to decide whether to re-run startWatermarkedPdfDownload():

   } else if (item.isWatermarked && item.contentUrl != null) {
     await startWatermarkedPdfDownload(item, item.contentUrl!, applyWatermark: true);
     return;
   } else if (item.taskId != null && item.contentUrl != null) {
     await _service.resumeAttachmentDownload(item.taskId!, item.contentUrl!);
   }

   But isWatermarked is only ever persisted as true after a PDF download completes successfully — see startWatermarkedPdfDownload() (downloads_repository.dart:386-397), where isWatermarked: applyWatermark is set only in the final upsertDownload after Future.wait([downloadFuture, thumbnailFuture]) succeeds. The initial upsert at the start of the download (itemWithTaskId = item.copyWith(taskId: taskId), line 346-347) never sets isWatermarked, so it stays at the model default of false for the entire in-progress/paused lifetime of the download.
   Net effect: for a PDF that is paused before it finishes (the normal case pause/resume exists for), item.isWatermarked is false when resumeDownload() runs, so it always falls into the raw _service.resumeAttachmentDownload() path — reconstructing a plain background_downloader task pointed at BaseDirectory.applicationDocuments — which is exactly the bug flagged in the previous round (un-watermarked file, wrong directory, PdfDownloader.downloadAndWatermark() never re-invoked).
   The new test resumeDownload for watermarked PDF re-triggers watermark pipeline (downloads_repository_test.dart:207-231) doesn't catch this because it manually constructs the paused row with isWatermarked: true, which doesn't reflect how a real in-progress PDF row is ever persisted — so it's testing a state the app can't actually reach.
   A reliable signal that survives pause is item.fileType == 'PDF' (set on the very first upsert and persisted through pause), not isWatermarked. Separately, even after fixing detection, resumeDownload() hardcodes applyWatermark: true — this needs to reflect the original watermark intent (e.g. persist it explicitly), since a paused PDF where the course had watermarkBeforeDownload == false would incorrectly gain a watermark on resume.
   Fix this →

🔵 SUGGESTION — Nice to have

1. DownloadsService.deleteDownloadItem()'s attachment branch still calls bg.FileDownloader().allTasks() and filters by taskId just to check existence before cancelTaskWithId(item.taskId!) — redundant since cancelTaskWithId already no-ops if the task doesn't exist (carried over from last round, not blocking).
2. Hardcoded strings ('Downloaded', 'Downloading' in lesson_detail_shell.dart; 'Pause', 'Resume Download', 'Paused', 'Paused • $progress%' in attachment_viewer.dart) still bypass AppLocalizations — consistent with the surrounding pre-existing pattern, worth batching into a future localization pass.

Verdict

CHANGES REQUESTED — the storage-location, schema-version, silent-error-swallowing, and cold-start-reconciliation items from earlier rounds are all resolved. The remaining blocker is #1: the PDF watermark-on-resume fix doesn't actually trigger for the realistic pause-mid-download case, so a paused PDF download will currently resume as an un-watermarked file in the wrong location.
· branch ref/attachment-download

[syed-tp]

@claude review

[claude]

Claude finished @syed-tp's task in 3m 30s —— View job

---

Todo

• Read AGENTS.md and core docs (ai_context.md, architecture.md)
• Fetch full PR diff since last review round (origin/main...HEAD)
• Trace PDF completion/resume logic end-to-end (downloads_repository.dart, downloads_service.dart, pdf_downloader.dart, attachment_viewer.dart, downloads_provider.dart)
• Verify previously-flagged items are resolved
• Post final review

PR Review Summary

Re-review after the third round. Diffed origin/main...HEAD. The previously-flagged "PDF resume-after-pause skips watermarking" bug (round 3, item #1) is now genuinely fixed — but tracing the same fileType == 'PDF' detection heuristic further surfaced a new, more serious bug in the completion path.

✅ What's good

• Watermark-on-resume is now correctly fixed. startWatermarkedPdfDownload() (downloads_repository.dart:347-353) now sets isWatermarked/fileType on the initial upsert (not just on completion), and resumeDownload() (downloads_repository.dart:561) uses the persisted item.fileType == 'PDF' signal (which survives pause) instead of the always-false-until-completion isWatermarked. This is exactly the fix suggested last round.
• All prior WARNING items are resolved: media_scanner dependency removed, requestNotificationPermission() reports via Sentry, taskId is cleared on completion for both attachment and PDF rows, and the redundant allTasks() lookup before cancelTaskWithId() in deleteDownloadItem() is gone.
• New commit 9c2aee4 localizes the previously-hardcoded strings (Pause, Resume Download, Paused, Downloaded, Downloading, etc.) via AppLocalizations/ARB files across all 4 locales — addresses the standing SUGGESTION.
• synchronize()'s stuck-download reconciliation and the passing test suite (downloads_repository_test.dart) remain solid.

🔴 CRITICAL — Must fix before merge

1. Plain (non-lesson) PDF attachments can never reach "completed" status. The bg.TaskStatus.complete handler in downloads_repository.dart (lines 125-129) unconditionally skips completion for any row where fileType == 'PDF':

   case bg.TaskStatus.complete:
     final isPdf = row.fileType?.toUpperCase() == 'PDF' || row.isWatermarked;
     if (isPdf) break;   // <-- skips ALL PDF-fileType rows, not just watermark-pipeline ones

   This check exists to prevent a race for the watermark pipeline (startWatermarkedPdfDownload / LessonType.pdf): both PdfDownloader's internal listener and this repository listener observe the same taskId's raw-fetch completion on the shared broadcast stream, so skipping here avoids marking the row "completed" with the temporary, un-watermarked file before watermarking finishes.
   However, AttachmentViewer._startDownload() (attachment_viewer.dart:43-67) — used for generic LessonType.attachment lessons — calls startAttachmentDownload(), which sets fileType from the URL extension (so a PDF file attachment gets fileType: 'PDF' too, attachment_viewer.dart:53-60) and relies entirely on this same TaskStatus.complete handler to mark the row completed — it has no other completion path (unlike startWatermarkedPdfDownload, which has its own explicit upsertDownload(status: completed, ...) after watermarking). Since isPdf is true purely from the file extension, the row for a plain PDF attachment download will sit in downloading forever — and on the next synchronize() call gets reconciled to paused (since the background task is no longer active), leaving the user stuck in a Pause/Resume loop for a file that has actually finished downloading to disk.
   The detection needs to distinguish "watermark-pipeline task" from "any PDF file," e.g. by checking the taskId prefix ('pdf_' vs 'att_', both already established in startWatermarkedPdfDownload/startAttachmentDownload) rather than fileType.
   Fix this →

🟡 WARNING — Should fix

1. Same root-cause heuristic misroutes resumeDownload() for plain PDF attachments. resumeDownload() (downloads_repository.dart:561-569) uses the identical item.fileType?.toUpperCase() == 'PDF' check to decide whether to re-run startWatermarkedPdfDownload(). For a paused plain attachment PDF (task ID att_<id>, saved under BaseDirectory.applicationDocuments via the deterministic getLocalPath() path), this incorrectly routes resume into the watermark pipeline: it re-downloads the entire file from scratch (discarding the paused progress — no Range/resume), saves it to public storage under a title-based filename (PdfDownloader.downloadAndWatermark's pubDir/safeTitle() scheme, pdf_downloader.dart:108-127) instead of the original deterministic path, and leaves the original partially-downloaded file in applicationDocuments orphaned. This would also break _openFile()'s existing-file fallback in attachment_viewer.dart:83-87, which looks up getLocalPath(url, StorageType.publicDownload) — a path that no longer matches where the resumed file actually landed.
   Fixing the taskId-prefix detection suggested in the CRITICAL item above would also resolve this.

🔵 SUGGESTION — Nice to have

1. No automated test currently covers a plain (non-lesson) PDF-typed attachment going through startAttachmentDownload → pause → resume, or its completion — the existing downloads_repository_test.dart coverage for PDFs only exercises the startWatermarkedPdfDownload/LessonType.pdf path. Worth adding once the detection fix above lands, so this class of regression is caught by CI going forward.

Verdict

CHANGES REQUESTED — the watermark-on-resume fix from the previous round is solid, but tracing its fileType == 'PDF' detection heuristic further surfaced that it also silently breaks completion (and, on resume, storage location) for ordinary file attachments that happen to be PDFs, since the same signal can't distinguish "watermark-pipeline task" from "any PDF file."
· branch ref/attachment-download

[syed-tp]

@claude review

[claude]

Claude finished @syed-tp's task in 1m 4s —— View job

---

Todo

• Read AGENTS.md and core docs (ai_context.md, architecture.md)
• Fetch full PR diff since last review round (origin/main...HEAD)
• Verify the taskId-prefix fix (commit 0fd0f94) resolves the previously-flagged CRITICAL issue
• Re-check related WARNING item (resumeDownload misrouting for plain PDF attachments)
• Post final review

PR Review Summary

Re-review after the fourth round. Only one new commit since the last review: 0fd0f94 ("refactor: identify watermark pipeline tasks by taskId prefix instead of file metadata"). Note: I wasn't able to run flutter test in this session (sandboxed shell required approval that isn't available here), so the assessment below is based on static review of the diff, not an executed test run — worth confirming CI is green.

✅ What's good

• The CRITICAL bug from the previous round is fixed correctly. Both the completion handler (downloads_repository.dart:125-127) and resumeDownload() (downloads_repository.dart:560) now key off taskId.startsWith('pdf_') instead of fileType/isWatermarked. This correctly distinguishes "watermark-pipeline task" from "any PDF file," since startAttachmentDownload() always assigns 'att_${item.id}' (downloads_repository.dart:282) and startWatermarkedPdfDownload() always assigns 'pdf_${item.id}' (downloads_repository.dart:346), and taskId is persisted on the very first upsert in both paths, so it survives pause/resume and process restarts.
• A plain PDF attachment (att_*) now correctly reaches DownloadStatus.completed via the generic handler, and correctly resumes via _service.resumeAttachmentDownload() (preserving the paused progress/Range-resume and the deterministic storage path), while a watermark-pipeline PDF (pdf_*) still skips the generic completion path and re-triggers startWatermarkedPdfDownload() on resume with the original applyWatermark intent preserved (item.isWatermarked).
• New tests added directly targeting both branches: resumeDownload ... plain PDF attachment routes to resumeAttachmentDownload, and both TaskStatus.complete cases (att_ completes, pdf_ is skipped) — this is exactly the regression class flagged as missing in the previous round.
• All prior rounds' CRITICAL/WARNING items (public storage doc, schema version, silent error swallowing, cold-start reconciliation, watermark-on-resume detection, media_scanner dependency, notification-permission error reporting, redundant allTasks() lookup, localization) remain resolved — no regressions introduced by this commit.

🔴 CRITICAL — Must fix before merge

None found.

🟡 WARNING — Should fix

None found in this round.

🔵 SUGGESTION — Nice to have

1. The offline-persistence spec's "Task ID cleared on completion or deletion" scenario is now fully accurate for both attachment and PDF completion paths (carried-over item from earlier rounds is resolved — no action needed, noting for the record).
2. Since the 'att_'/'pdf_' prefix is now a load-bearing contract used in two places for branching logic, consider extracting it into a small shared helper/constant (e.g. TaskIdKind.isWatermarkPipeline(taskId)) so future call sites can't drift by hardcoding the string literal independently — minor hardening, not blocking.

Verdict

APPROVED — all previously-flagged CRITICAL and WARNING issues across all review rounds are resolved. The taskId-prefix fix is targeted, correct, and covered by new tests for both the plain-attachment and watermark-pipeline branches.
· branch ref/attachment-download