perf(courses): fix PDF OOM crashes and migrate from Syncfusion to pdfrx

[syed-tp]

Background & Problem:

When loading and scrolling large or high-definition PDF documents (e.g. 100MB / 100 pages),
the app suffered from critical Out-Of-Memory (OOM) crashes and extreme latency with Syncfusion:

• Fatal OOM Crashes: Anonymous RAM surged from a 120MB baseline to 1.36 GB (1,362 MB),
  triggering fatal E/DartVM: Exhausted heap space crashes around Page 9 as decompressed
  page streams accumulated unbounded in the Dart VM heap.
• 45s Download Lockup: SfPdfViewer.network forced a complete 100MB binary download upfront
  before rendering Page 1, ignoring HTTP 206 Partial Content headers.
• Severe UI Stutter: In-Dart text extraction and page parsing (PdfTextExtractor) ran on the
  main UI isolate, blocking the thread for 17.22s and freezing user navigation.

Solution & Implementation:

• Replaced syncfusion_flutter_pdfviewer with pdfrx, utilizing native C++ Google PDFium
  workers to manage memory off the Dart VM heap.
• Enabled HTTP 206 Byte-Range Streaming (preferRangeAccess: true) to fetch only active
  page chunks on demand, rendering Page 1 in seconds.
• Configured Viewport Texture Bounds (verticalCacheExtent: 3.0, 256MB memory cap) with
  limitRenderingCache: false to retain rendered page bitmaps and prevent white page flashes.
• Enabled Progressive Background Decoding (document.loadPagesProgressively()) to resolve
  trailing pages asynchronously on native threads without starving the Flutter UI isolate.
• Added dynamic Flutter canvas watermark overlays (pageOverlaysBuilder) at zero memory
  overhead, eliminating heavy in-memory PDF binary rewriting.
• Added interactive scroll thumb (PdfViewerScrollThumb) showing dynamic page counters
  ($pageNumber / $pageCount).

Verified Metrics (100MB PDF Perfetto Profile):

• Crash Stability: Resolved OOM crashes completely (scrolled 55+ pages with 0 crashes).
• Peak Memory: Dropped by 72% (from 1,362 MB down to 382 MB).
• Initial Load Time: Dropped from ~45.2s to ~8.5s (<100ms on session revisit).
• Total CPU Run Time: Dropped by 80% (from 40.45s to 8.20s).
• UI Thread Block: Dropped by 88% (from 17.22s to 2.10s).

[claude]

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

---

Claude PR Review

Todo List

• Read AGENTS.md and core docs (ai_context.md, architecture.md)
• Check openspec/changes/ and ADRs (no active change; only archived PDF-related changes found — none referenced by this PR)
• Fetch full PR diff (gh pr diff 289)
• Review SDK boundaries, dependency rules, design system usage
• Review generated files / platform plugin changes
• Review pdf_viewer.dart implementation in detail
• Post structured review summary

---

PR Review Summary

✅ What's good

• Solid, well-documented rationale for the Syncfusion → pdfrx migration, with concrete before/after perf numbers.
• SDK boundaries are respected: pdfrx is only added to packages/courses/pubspec.yaml; app/pubspec.lock only picks it up transitively (no direct import from app/), and no domain package imports another domain package.
• AnimatedOpacity duration correctly goes through MotionPreferences.duration(context, design.motion.normal) rather than a hardcoded duration.
• Progressive page decoding (document.loadPagesProgressively()) and byte-range streaming (preferRangeAccess: true) are reasonable, well-motivated choices for the stated OOM/latency problem.
• Removing syncfusion_flutter_pdfviewer/syncfusion_pdfviewer_* from the Linux/macOS/Windows generated plugin registrants is consistent with dropping the dependency.

🔴 CRITICAL — Must fix before merge

1. No failure path for a failed PDF load → infinite loading skeleton. In packages/courses/lib/widgets/lesson_detail/pdf_viewer.dart _setupViewer() (lines 159–236), the PdfViewerParams passed to PdfViewer.uri/PdfViewer.file only wires onViewerReady. There is no error callback (pdfrx exposes an error-banner/error-builder hook for this). Previously, SfPdfViewer's onDocumentLoadFailed called _handleError(id, ...), which set _error and showed AppErrorView with a retry button. Now, if the document fails to load (broken/expired signed URL, mid-load network drop, corrupt file), onViewerReady simply never fires: _isVisible stays false, _error stays null, and build() keeps rendering LessonDetailSkeleton forever with no way to recover except leaving the screen. This is a regression against the exact class of failures this PR is meant to make more robust.
2. PdfViewerController is no longer disposed. dispose() (lines 93–96) and _resetViewer() (lines 335–338) both dropped the _controller.dispose() call that existed pre-migration; _initController() now just creates a fresh PdfViewerController() without releasing the old one. If pdfrx's controller (as with most Flutter controllers you own) needs explicit disposal to release its native PDFium/document reference, every lesson navigation or retry (_resetViewer) leaks a controller — directly undercutting this PR's memory goals. Please confirm against pdfrx's PdfViewerController docs/changelog whether disposal is required and restore it if so.
3. New scroll-thumb/page-counter control violates the design-system and accessibility rules for new code (lines 169–206):
   • Touch target is Size(64, 28) — the 28dp height is below the mandatory 48×48dp minimum (WCAG 2.5.5).
   • Page counter uses a raw Text(...) instead of AppText.caption(...) (Design System & Widget Usage rule).
   • The whole draggable/interactive thumb has no AppSemantics wrapper, so screen readers get no label for the current page or that it's a draggable control (Accessibility Semantics rule).

🟡 WARNING — Should fix

4. Dead dependency left in packages/courses/pubspec.yaml. syncfusion_flutter_pdf: ^33.1.45 is still declared, but nothing under packages/courses imports it anymore (only packages/core/lib/data/services/pdf_downloader.dart, a different package, uses it). Since this PR already touched the pubspec to remove the viewer packages, the now-unused syncfusion_flutter_pdf line should go too.
5. Removed connectivity pre-check compounds Critical feat(ui): implement LMS core primitives #1. The hasInternetConnection() check before loading widget.url was deleted rather than replaced. Combined with the missing error callback, an offline user now gets an indefinite loading skeleton instead of the previous fast-fail "No Internet Connection" state.
6. Desktop (Linux/Windows) plugin registrants look asymmetric vs. macOS. macOS's GeneratedPluginRegistrant.swift gained pdfium_flutter, but the Linux (generated_plugin_registrant.cc/generated_plugins.cmake) and Windows equivalents only had the Syncfusion entries removed — no pdfrx/pdfium plugin was added. This may be expected if pdfrx fetches its native pdfium binary via a build-time script on those platforms rather than plugin registration, but given these are generated files (must reflect flutter pub get for each platform), please confirm PDF rendering still works on Linux/Windows desktop builds and that these files weren't partially hand-edited.
7. New feature vs. OpenSpec. Beyond the library swap, this PR adds new user-facing behavior (interactive scroll thumb with a live page counter). No active openspec/changes/ entry accompanies it (only archived PDF-related changes exist, none referenced here). Flagging as WARNING rather than CRITICAL since it's arguably an incidental UI affordance rather than a standalone feature — worth a quick check on whether it needs a lightweight OpenSpec entry.

🔵 SUGGESTION — Nice to have

8. Dead no-op initState() override added in packages/courses/lib/screens/lesson_detail_orchestrator.dart (lines 57–60):

   @override
   void initState() {
     super.initState();
   }

   This is unrelated to the PDF changes and does nothing beyond the default — looks like leftover scaffolding and can be removed.
9. The scroll-thumb's BoxShadow uses a hardcoded Colors.black.withValues(alpha: 0.15) instead of a design token/shadow color from Design.of(context). Minor, but worth aligning with the "read tokens from context" rule.

Verdict

CHANGES REQUESTED

The migration direction and measured perf wins are compelling, but the missing PDF-load error handling (#1) combined with the removed controller disposal (#2) are worth resolving before merge — the first is a concrete UX regression (infinite spinner on any load failure), and the second potentially reintroduces the kind of native-memory leak this PR set out to fix.

[syed-tp]

@claude review

[claude]

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

---

Claude PR Review

Todo List

• Fetch full PR diff (gh pr diff 289)
• Read AGENTS.md and core docs (ai_context.md, architecture.md)
• Check openspec/changes/ (no active change referencing this PR; only archived PDF-related changes)
• Review updated pdf_viewer.dart (error handling, connectivity check, scroll-thumb accessibility fixes)
• Review lesson_detail_orchestrator.dart and pubspec.yaml cleanup
• Post structured review summary

---

PR Review Summary

This is a re-review of the current branch state, which now includes a second commit (21e63ce) specifically addressing the CRITICAL findings from my earlier review. Most of them are resolved.

✅ What's good

• Critical feat(ui): implement LMS core primitives #1 (infinite loading skeleton) is fixed. _setupViewer() (packages/courses/lib/widgets/lesson_detail/pdf_viewer.dart:165-236) now wires both onDocumentLoadFinished and errorBannerBuilder, routing failures into _handleError → AppErrorView with retry, matching the old Syncfusion onDocumentLoadFailed behavior.
• Connectivity pre-check restored (lines 108-113) — offline loads now fail fast with a "No Internet Connection" state instead of hanging, resolving the old Warning feat(core): implement semantic-first typography foundation and gallery #5 too.
• Scroll-thumb accessibility fixed: touch target is now Size(68, 48) (meets WCAG 2.5.5's 48×48dp minimum), the page counter uses AppText.caption(...) instead of a raw Text, and it's wrapped in Semantics(label:, value:, slider: true) — reasonable given the design system has no AppSemantics.slider() helper yet.
• Design-token compliance improved: the thumb's shadow now reads design.shadows.floating instead of a hardcoded Colors.black.
• Dead code cleanup: the unused syncfusion_flutter_pdf dependency and the no-op initState() override in lesson_detail_orchestrator.dart were both removed.

🟡 WARNING — Should fix

1. PdfViewerController still isn't disposed. dispose() (lines 93-96) only calls _controller.removeListener(...), and _resetViewer() (lines 353-356) creates a fresh PdfViewerController() via _initController() without disposing the old one. I'm downgrading this from CRITICAL since the actual native PDFium/document resources appear to be scoped to the PdfViewer.uri/PdfViewer.file widget's own lifecycle — they get released whenever _pdfViewerWidget is nulled out in _prepareState() (called on every reload/retry), which is the part that matters most for the OOM goal. But PdfViewerController is still a caller-owned object with a dispose() method, and every lesson navigation/retry currently leaks one. Please restore _controller.dispose() in both places unless pdfrx's docs explicitly say otherwise.
2. Possible redundant error handling path. Both onDocumentLoadFinished (line 175) and errorBannerBuilder (line 180) can call _handleError for the same failure — not a correctness bug (idempotent), but worth confirming with pdfrx 2.4.8 that both callbacks are actually needed, since errorBannerBuilder alone may already cover the failure case onDocumentLoadFinished handles.
3. Desktop plugin registrant asymmetry (unchanged from last review): macOS gained pdfium_flutter, but Linux/Windows generated registrants only had the Syncfusion entries removed with no pdfrx/pdfium plugin added. Likely fine if pdfrx pulls its native binary at build time on those platforms, but worth confirming Linux/Windows desktop builds still render PDFs.
4. New scroll-thumb/page-counter UI still has no OpenSpec entry. It's a small, incidental affordance bundled with a perf-motivated migration, so flagging as WARNING rather than CRITICAL — but confirm whether it needs a lightweight OpenSpec change per this repo's SDD rules.

🔵 SUGGESTION — Nice to have

• Consider adding an AppSemantics.slider() helper to packages/core so future range/position controls (like this scroll thumb) don't need to fall back to raw Semantics.
• I wasn't able to run flutter analyze/flutter pub get in this sandbox (no SDK/network access), so the exact pdfrx 2.4.8 callback signatures (onDocumentLoadFinished, errorBannerBuilder) and whether PdfViewerScrollThumb.thumbSize truly defines the hit-testable drag region are unverified — worth confirming CI is green before merge.

Verdict

CHANGES REQUESTED (minor) — the two previous CRITICAL blockers are resolved; only the controller-disposal leak (#1) is worth fixing before merge, and the rest are confirmations/nice-to-haves.

[syed-tp]

@claude review

1. PdfViewerController Disposal

PdfViewerController in pdfrx extends ValueListenable<Matrix4> and does not have a dispose() method (calling it causes a compile error). Listeners are properly removed via removeListener(_trackProgress) in both dispose() and _resetViewer(). Native PDFium resources are owned and freed by PdfDocument.

2. Error Handling Redundancy

errorBannerBuilder handles widget-level render failures, while onDocumentLoadFinished handles document-level load failures. Both route to _handleError(), which is idempotent and safely prevents any infinite loading skeleton.

3. Desktop Plugin Registrants

On macOS, PDFium links via CocoaPods and appears in GeneratedPluginRegistrant.swift. On Linux and Windows, pdfrx_engine loads the native binary directly via Dart FFI without platform channels. The registrant files are purely auto-generated by Flutter tooling.

4. OpenSpec Alignment

The scroll thumb restores the visual page indicator that Syncfusion provided out-of-the-box. OpenSpec was bypassed for this engine replacement and bugfix per team alignment.

[claude]

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

---

Claude PR Review

Todo List

• Read trigger comment rebuttal (controller disposal, error handling, desktop plugins, OpenSpec)
• Verify PdfViewerController API claim against available sources
• Verify error handling paths in pdf_viewer.dart
• Verify desktop plugin registrant claim / cross-check CI
• Post updated review summary

---

PR Review Summary

Thanks for the detailed rebuttal — I re-checked each point against packages/courses/lib/widgets/lesson_detail/pdf_viewer.dart (current state, commit 21e63ce). Note: I don't have network/pub.dev access in this sandbox, so I can't directly inspect the pdfrx 2.4.8 source to independently confirm the API-shape claims below — but CI (Build & Test, run 34583576880) is green on this commit, which is consistent with the code compiling as written.

✅ What's good

• All items from the previous round remain fixed: error callback wiring (onDocumentLoadFinished + errorBannerBuilder → _handleError), connectivity pre-check, scroll-thumb accessibility (Size(68, 48), AppText.caption, Semantics(...)), design-token shadow usage, and the syncfusion_flutter_pdf / dead initState() cleanup.

Responses to the 4 points

1. Controller disposal — Accepted. PdfViewerController in this code is only ever used via addListener/removeListener (pdf_viewer.dart:79,94,354), and both dispose() and _resetViewer() correctly remove the listener. If PdfViewerController genuinely has no dispose() (as a ValueListenable-only type) and native PDFium resources are scoped to PdfDocument/the PdfViewer.uri/.file widget's own lifecycle (released when _pdfViewerWidget is swapped out in _prepareState, pdf_viewer.dart:161), there's no leak here. Downgrading — no longer a blocker. Worth a one-line code comment noting why there's no .dispose() call, so the next reader doesn't reintroduce it as a "fix."
2. Error handling redundancy — Accepted. Widget-level render failures (errorBannerBuilder) vs. document-level load failures (onDocumentLoadFinished) are a legitimate distinction, and both funnel into the idempotent _handleError (pdf_viewer.dart:256-267), guarded by _isValidRequest. No infinite-skeleton risk. Resolved.
3. Desktop plugin registrants — Plausible and consistent with what's in the diff (macOS via CocoaPods/plugin registrant, Linux/Windows untouched beyond Syncfusion removal). Since this is Dart FFI + native-binary loading rather than a platform channel, the generated registrant files correctly have nothing new to add. Resolved as far as static review can tell — recommend a quick manual smoke-test on a Linux/Windows build before or shortly after merge, since that path is the one this review genuinely can't verify from source alone.
4. OpenSpec bypass — Acknowledged as a team-level process decision (engine replacement + bugfix, scroll thumb as restored parity behavior rather than net-new feature). Not something to keep blocking on.

🔵 SUGGESTION — Nice to have (unchanged, non-blocking)

• Consider adding an AppSemantics.slider() helper to packages/core so future range/position controls don't need to fall back to raw Semantics.

Verdict

APPROVED — all previously raised CRITICAL/WARNING items are resolved or satisfactorily explained. Only non-blocking suggestions remain.
· Branch: perf/pdf_scroll