feat(courses): Add PDF download functionality with watermarking - #216
Conversation
|
Claude finished @pugal66's task in 5m 17s —— View job Review complete
PR Review Summary✅ What's good
🔴 CRITICAL — Must fix before merge
🟡 WARNING — Should fix
🔵 SUGGESTION — Nice to have
VerdictCHANGES REQUESTED — the accessibility gaps and the spec/implementation mismatch on watermarking should be resolved before merge; the DB migration and Downloads-registry issues are functional risks worth addressing too. |
There was a problem hiding this comment.
Code Review
This pull request introduces a conditional PDF download flow based on an allow_download flag from the backend, applying a permanent watermark to downloaded files while ensuring in-app watermarks are always visible. Key changes include updating domain models, integrating the syncfusion_flutter_pdf package, implementing a background isolate watermarking pipeline in PdfDownloadService, and adding UI components like DownloadProgressBanner and AlreadyDownloadedSheet. The review feedback highlights several critical improvements: ensuring filename safety for non-Latin scripts, correcting Android storage permission handling for newer API levels, aligning the download UX in PdfLessonDetailScreen with the orchestrator, preventing potential crashes and resource leaks in the download service, and localizing hardcoded English strings in the new UI components.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| borderRadius: BorderRadius.circular(16), | ||
| boxShadow: [ | ||
| BoxShadow( | ||
| color: Colors.black.withValues(alpha: 0.1), |
There was a problem hiding this comment.
WARNING — Hardcoded color instead of design token: Colors.black.withValues(alpha: 0.1) should be design.colors.shadow, which exists and is even used elsewhere in this very PR (pdf_lesson_detail_screen.dart:183).
|
@claude review |
|
Claude finished @pugal66's task in 3m 18s —— View job Claude finished @pugal66's task in 6m 12s —
PR Review SummaryRe-reviewed after the ✅ What's good / confirmed fixed since last review
🟡 WARNING — Should fix
🔵 SUGGESTION — Nice to have
VerdictCHANGES REQUESTED (minor) — the core CRITICAL issues from the previous round (spec mismatch, missing a11y semantics, dead-code duplicate flow, Downloads-registry bypass) are all resolved. What's left is a small, low-risk localization/error-handling cleanup — no functional or architectural blockers remain. |
4ffd529 to
a1f11b7
Compare
|
@claude review |
|
Claude finished @pugal66's task in 4m 43s —— View job Claude finished @pugal66's task in 5m 6s
PR Review SummaryRe-reviewed the current state of the branch (all prior review rounds are now squashed into a single commit ✅ What's good / confirmed fixed from earlier rounds
🔴 CRITICAL — Must fix before merge
🟡 WARNING — Carried forward, still open
🔵 SUGGESTION — Nice to have (low priority, unchanged from prior rounds)
VerdictCHANGES REQUESTED — the localization/a11y/error-handling items from earlier rounds are all genuinely resolved. What's blocking now is new: the Downloads registry |
| duration: Value(item.duration), | ||
| fileType: Value(item.fileType), | ||
| contentUrl: Value(item.contentUrl), | ||
| filePath: Value(item.filePath), |
There was a problem hiding this comment.
CRITICAL — synchronize() (lines 161-213, unchanged by this PR but broken by it) deletes every downloaded PDF's registry row. activeIds is built only from activeVideoIds and activeAttachmentIds — the query at line 169 filters typeIndex.equals(DownloadType.attachment.index), so the new DownloadType.pdf rows introduced in this PR are never included. Then batch.deleteWhere(_db.downloadsTable, (tbl) => tbl.id.isNotIn(activeIds)) at line 186 deletes any row not in that set — which includes every PDF download, completed or in-progress.
DownloadsScreen.initState() calls synchronize() on every visit (downloads_screen.dart:43-45), so simply opening the Downloads tab wipes the DB record for any downloaded PDF. Concretely:
- Downloaded PDFs vanish from the Downloads screen the moment it's opened (compounding the tab-display issue flagged separately).
LessonDetailOrchestrator._handleDownloadrelies onwatchDownloadItemProvider(lesson.id)reportingDownloadStatus.completedto show the "already downloaded" sheet. Once the row is deleted, re-opening the lesson silently falls through to_startDownload, re-downloading and re-watermarking a PDF that's already on disk — and since the save-path logic appends(1),(2), ... on conflict, duplicate files accumulate in public storage over time.
synchronize() predates DownloadType.pdf; it needs a matching branch now that this PR introduces a third DownloadType.
a1f11b7 to
c4f5e4a
Compare
|
@claude review |
|
Claude finished @pugal66's task in 3m 47s —— View job Claude finished @pugal66's task in 8m 4s
PR Review SummaryAll prior-round history is squashed into one commit now. Verified the two CRITICAL bugs from the last review round are genuinely fixed — but found a new, closely-related bug in the tap-to-open path, plus a new architectural regression. ✅ What's good / confirmed fixed from earlier rounds
🔴 CRITICAL — Must fix before merge
🟡 WARNING — Should fix
🔵 SUGGESTION — Nice to have (carried forward, unaddressed, low priority)
VerdictCHANGES REQUESTED — great progress on the previous round's |
| String watermarkText = fallbackWatermarkText; | ||
| try { | ||
| final db = await _ref.read(appDatabaseProvider.future); | ||
| final currentUser = await db.select(db.usersTable).getSingleOrNull(); |
There was a problem hiding this comment.
WARNING — Bypasses UserRepository/userProvider, reads the core package's Drift table directly. This queries db.usersTable straight from the courses package instead of going through userProvider (which was specifically praised in earlier review rounds for correctly following ADR 0005 — "single source of truth for the authenticated user's profile"). The same pattern also appears in packages/courses/lib/widgets/lesson_detail/pdf_viewer.dart:110-111.
This is a Data Layer Architecture violation (never access AppDatabase/Drift directly — always go through a Repository/Provider) and re-couples courses to core's internal table schema (UsersTableData), which ADR 0005 was written specifically to avoid. Please switch both call sites back to ref.read(userProvider.future) (or the equivalent UserRepository call).
| final attachments = [ | ||
| ...downloads.byType(DownloadType.attachment), | ||
| ...downloads.byType(DownloadType.pdf), | ||
| ]; |
There was a problem hiding this comment.
CRITICAL — DownloadType.pdf is now shown in this tab, but tapping a completed PDF still opens the video player, not the file. _handleAction further down (around line 146-158, not touched by this PR's diff) only special-cases DownloadType.attachment in the completed branch:
case DownloadStatus.completed:
if (item.type == DownloadType.attachment && item.contentUrl != null) {
await _openAttachment(item);
} else {
// DownloadType.pdf falls through here
Navigator.of(context).push(MaterialPageRoute(builder: (_) => OfflineVideoPlayerScreen(item: item)));
}PdfDownloadService sets both contentUrl and filePath on PDF DownloadItems exactly like attachments do (pdf_download_service.dart:106,180), so a completed PDF download should also route through _openAttachment. As written, tapping a downloaded PDF card here pushes OfflineVideoPlayerScreen with a PDF item instead of opening the file — this file wasn't touched by the diff at that line, but it's directly broken by the new DownloadType.pdf value this PR introduces, the same way the tab-count/synchronize() bugs were in the previous round.
| String watermarkText = fallbackWatermarkText; | ||
| try { | ||
| final db = await _ref.read(appDatabaseProvider.future); | ||
| final currentUser = await db.select(db.usersTable).getSingleOrNull(); |
There was a problem hiding this comment.
WARNING — Bypasses UserRepository/userProvider, reads the core package's Drift table directly. This queries db.usersTable straight from the courses package instead of going through userProvider (which was specifically praised in earlier review rounds for correctly following ADR 0005 — "single source of truth for the authenticated user's profile"). The same pattern also appears in packages/courses/lib/widgets/lesson_detail/pdf_viewer.dart:110-111.
This is a Data Layer Architecture violation (rule: never access AppDatabase/Drift directly — always go through a Repository/Provider) and re-couples courses to core's internal table schema (UsersTableData), which ADR 0005 was written specifically to avoid. Please switch both call sites back to ref.read(userProvider.future) (or the equivalent UserRepository call).
388a466 to
a851fc0
Compare
|
@claude review |
|
Claude finished @pugal66's task in 6m 18s —— View job Claude finished @pugal66's task
PR Review SummarySignificant rework since the last round: PDF download/watermark logic moved from ✅ What's good / confirmed fixed from earlier rounds
🔴 CRITICAL — Must fix before merge
🟡 WARNING — Should fix
VerdictCHANGES REQUESTED — the conditional-watermark and spec-alignment work from Branch: |
910dcf5 to
ba5cb9e
Compare
|
@claude review |
|
Claude finished @pugal66's task in 3m 42s —— View job Claude finished @pugal66's task
PR Review SummaryRe-reviewed after the latest "Address review comments" commit ( ✅ What's good / confirmed fixed from earlier rounds
🟡 WARNING — Should fix
🔵 SUGGESTION — Nice to have (low priority)
VerdictCHANGES REQUESTED (minor) — all CRITICAL issues from every prior round are resolved; what's left is a small, self-inflicted filename-sanitization duplication in the newly-added Branch: |
| final fileDownloader = ref.read(fileDownloaderProvider); | ||
| final pubDir = | ||
| await fileDownloader.getDirectory(StorageType.publicDownload); | ||
| final canonicalPath = '${pubDir.path}/${lesson.title}.pdf'; |
There was a problem hiding this comment.
WARNING — Filename sanitization mismatch causes false "not downloaded" state. canonicalPath is built from the raw lesson.title with no sanitization, but the actual save path (computed in PdfDownloader.downloadAndWatermark, pdf_downloader.dart:66 and mirrored in DownloadsService.getExistingPdfSize, downloads_service.dart:152) strips [\\/:*?"<>|] first:
var safeTitle = title.replaceAll(RegExp(r'[\\/:*?"<>|]'), '_').trim();
if (safeTitle.isEmpty) safeTitle = 'lesson';
final path = '${dir.path}/$safeTitle.pdf';For any lesson title containing one of those characters, this "already downloaded" probe checks a path that was never actually written to, so canonicalFileExists is always false even when the file exists — the app will silently re-download and re-watermark a PDF that's already on disk (the exact duplicate-download scenario flagged in earlier review rounds, now reintroduced via a third, unsynchronized copy of the path-construction logic). Consider extracting the sanitization into a shared helper (e.g. on PdfDownloader) and reusing it here instead of re-deriving the path independently.
| onClose: () => | ||
| setState(() => _isAlreadyDownloadedSheetOpen = false), | ||
| onOpenFile: () { | ||
| if (downloadedFilePath != null) { |
There was a problem hiding this comment.
WARNING — "Open File" can silently no-op when the file exists but the DB record doesn't. _handleDownload (line 79) opens this sheet whenever canonicalFileExists is true, regardless of whether there's a matching DownloadItem in the DB (e.g. the registry row was purged/cleared while the physical file survived). But downloadedFilePath here comes only from downloadItem?.filePath (line 220), which is null when no DB record exists. In that case tapping "Open File" does nothing — no error, no fallback. Consider passing canonicalPath through as a fallback when downloadItem is absent, or re-registering the orphaned file in the DB when it's detected in _handleDownload.
ba5cb9e to
dbb22e3
Compare
dbb22e3 to
d3f6b50
Compare
|
@claude review |
|
Claude finished @pugal66's task in 3m 50s —— View job Claude finished @pugal66's task
PR Review SummarySubstantial rework since the last round: the ✅ What's good / confirmed fixed from earlier rounds
🔴 CRITICAL — Must fix before merge
🟡 WARNING — Should fix
🔵 SUGGESTION — Nice to have
VerdictCHANGES REQUESTED — the architectural simplification (folding PDFs into |
|
@claude review |
|
Claude finished @pugal66's task in 1m 40s —— View job Claude finished @pugal66's task
PR Review SummaryReviewed the latest commit ( ✅ What's good / confirmed resolved this round
🟡 WARNING — Should fix
🔵 SUGGESTION — Nice to have (low priority, carried forward, unaddressed)
VerdictCHANGES REQUESTED (minor) — the core architectural question from last round (PDFs excluded from Downloads screen) is now properly resolved via an explicit, spec-documented product decision rather than an accidental filter. What's left is a small, self-inflicted localization gap in the exact toast just added this round, plus long-acknowledged low-risk carryovers (schema migration, two stale doc/dead-code nits). No blocking architectural, spec-compliance, or data-integrity issues remain. Branch: |
bf0183b to
dcea9d1
Compare
|
@claude review |
|
Claude finished @pugal66's task in 1m 34s —— View job Claude finished @pugal66's task
PR Review SummaryReviewed the latest commit ( ✅ What's good / confirmed resolved this round
🟡 WARNING — Carried forward, unchanged
🔵 SUGGESTION — Nice to have (low priority, carried forward, unaddressed)
VerdictAPPROVED — the last remaining actionable item (hardcoded download toast strings) is fixed cleanly and correctly across all locales. What's left is the long-acknowledged, deliberately-deferred schema migration and two purely cosmetic nits carried over from earlier rounds. No blocking architectural, spec-compliance, accessibility, or data-integrity issues remain. |
allow_download: true.watermark_before_download: true.