feat(courses): Add pull-to-refresh to curriculum subfolder screens

[pugal66]

• Add manual refresh to Chapters and Chapter Detail pages
• Preserve existing offline cache during refresh to prevent skeleton flashes

[claude]

Claude finished @pugal66's task in 3m 27s —— View job

---

PR Review: Add pull-to-refresh to curriculum subfolder screens

• Read repo guidelines (AGENTS.md, core docs, ADRs)
• 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 offline data consistency
• Review generated files
• Review localization, icons, dependencies, feature flags
• Post final structured review

---

PR Review Summary

✅ What's good

• Proper OpenSpec change included (proposal.md, design.md, tasks.md, capability specs) — this correctly qualifies as new-feature/architectural since it introduces a new shared core widget (AppRefreshIndicator) and a new provider (ChapterDetailController).
• Good architectural call: onRefresh awaits network calls directly instead of ref.invalidate, preserving the offline cache and avoiding skeleton flicker — matches the stated design decision and the lms-study-chapters-list / chapter-detail spec scenarios.
• AlwaysScrollableScrollPhysics + SliverFillRemaining fix for empty-state scrollability is correct and matches existing precedent (StudyScreen/ExamsScreen).
• Import boundaries respected — courses only imports package:core/core.dart / package:core/data/data.dart and relative files; no cross-domain-package imports.
• Localization done properly: new pullToRefresh / refreshFailed keys added to all 4 ARB files (en/ar/ml/ta) with real translations, and generated app_localizations*.dart files correctly reflect the source ARB changes.
• .g.dart changes (chapter_detail_provider.g.dart, filtered_lessons_provider.g.dart) correspond to real source changes (new ChapterDetailController provider, new hash from added refresh() method) — no evidence of hand-editing.
• Design tokens used correctly (design.spacing.md replacing old hardcoded 16 padding in chapter_detail_page.dart).

🔴 CRITICAL — Must fix before merge

None found.

🟡 WARNING — Should fix

1. Inconsistent/missing error handling in ChaptersListPage.onRefresh — packages/courses/lib/screens/chapters_list_page.dart:200-224. The chapter-list branch silently swallows refresh errors (catch (_) { /* Swallow */ } at line 209) with no user feedback, and the filtered-lessons branch (else, lines 213-223) has no try/catch at all, so a thrown exception propagates unhandled out of RefreshIndicator.onRefresh. This is inconsistent with ChapterDetailPage.onRefresh (chapter_detail_page.dart:108-117), which correctly catches and shows an AppToast with the new refreshFailed string. Recommend catching errors in both branches here and showing the same toast for parity and to satisfy the "don't swallow exceptions" code-quality rule.
   Fix this →

2. throw e; loses original stack trace — packages/courses/lib/providers/chapter_detail_provider.dart:132 (inside ChapterDetailController.refresh). Should be rethrow; so the stack trace isn't reset before propagating to the caller. (Note: this mirrors a pre-existing pattern in lesson_detail_provider.dart:111, so it's not a new anti-pattern being introduced, but worth fixing here since it's new code.)
   Fix this →

3. No test coverage for the new refresh behavior. None of ChapterDetailController.refresh/initialSync, FilteredLessons.refresh(), the reworked CourseRepository.getFilteredLessonsController/startApiSync cancel-and-restart logic, or the new onRefresh UI wiring in either screen have accompanying tests. The existing chapters_list_page_test.dart wasn't updated either. Given this touches cache-preservation behavior (the core goal of the PR), a regression test asserting existing data stays visible during refresh (no skeleton) would be valuable.

🔵 SUGGESTION — Nice to have

1. design.md's Context section states the top-level tab screens "already implement pull-to-refresh via AppRefreshIndicator" — but AppRefreshIndicator is newly introduced in this PR (packages/core/lib/widgets/app_refresh_indicator.dart, git history shows no prior version). StudyScreen, ExamsScreen, and info_page.dart currently instantiate Flutter's RefreshIndicator directly rather than this new shared primitive. Consider a follow-up to migrate those three screens onto AppRefreshIndicator for consistency (and to correct the doc wording).
2. AppRefreshIndicator wraps Material's RefreshIndicator directly (import 'package:flutter/material.dart' show RefreshIndicator;). This matches the pre-existing pattern in study_screen.dart/exams_screen.dart/info_page.dart and is a net improvement (centralizes the Material dependency into one core widget instead of three domain-package files), but it's still at odds with the "first-principles UI, no Material widgets" philosophy in architecture.md. Not a new violation — just worth tracking as future cleanup since RefreshIndicator isn't easily reproducible from primitives.
3. In course_repository.dart's startApiSync() (~line 555), when a refresh is triggered while a previous sync is still in flight, apiSub?.cancel() is called but not awaited before a new subscription is created. Unlikely to cause a real bug since cancel() on the old subscription still runs, but await apiSub?.cancel(); would be more deterministic.

Note on verification

I wasn't able to run flutter analyze / flutter test in this environment (no Flutter SDK available), so the above is based on static review of the diff only — please confirm CI is green before merging.

Verdict

CHANGES REQUESTED — no CRITICAL issues, but please address the error-handling inconsistency (#1) and the throw e → rethrow fix (#2) before merge; test coverage (#3) is strongly recommended but negotiable.
· branch: subfolder-pull-to-refresh

[pugal66]

@claude review

[claude]

Claude finished @pugal66's task in 2m 2s —— View job

---

Claude finished @pugal66's task in 4m 12s —— View job

---

PR Review: Add pull-to-refresh to curriculum subfolder screens

• Read repo guidelines (AGENTS.md, core docs, ADRs)
• Fetch full PR diff (origin/main...HEAD)
• Check whether prior review feedback was addressed
• 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 offline data consistency
• Review generated files
• Review localization, icons, dependencies, feature flags
• Post final structured review

---

PR Review Summary

This is a re-review after the Address review comments commit (2248084). Both issues raised in my previous pass have been fixed, plus one of the suggestions was proactively addressed.

✅ What's good

• Previous WARNING feat(ui): implement LMS core primitives #1 fixed — ChaptersListPage's onRefresh now wraps both the showChapters and filtered-lessons branches in a single try/catch, showing AppToast with l10n.refreshFailed on failure (packages/courses/lib/screens/chapters_list_page.dart:199-227), matching the pattern in ChapterDetailPage.
• Previous WARNING feat(navigation-shell): implement unified routing architecture #2 fixed — throw e; was changed to rethrow; in ChapterDetailController.refresh (packages/courses/lib/providers/chapter_detail_provider.dart:132), preserving the original stack trace.
• Previous SUGGESTION feat(ui): implement LMS core primitives #1 addressed as a bonus — StudyScreen, ExamsScreen, and InfoPage were migrated from Flutter's raw RefreshIndicator onto the new shared AppRefreshIndicator primitive, so the Material dependency is now fully centralized in packages/core/lib/widgets/app_refresh_indicator.dart and the design.md doc's claim about existing screens is now accurate.
• startApiSync() in course_repository.dart was reworked to return a Future<void> via a Completer, so FilteredLessons.refresh() can properly await page-1 completion, and hasMore is correctly reset to true on refresh so pagination restarts cleanly. streamFilteredContents always starts from page 1 on each call, so cancel-and-restart semantics are correct.
• SDK import boundaries still respected — no new violations introduced by this commit (chapters_list_page.dart only added course_list_provider as a relative/local import).
• core.dart correctly exports the new app_refresh_indicator.dart.
• AppToast.show(context, message: ..., isError: true) usage matches the actual widget API.

🔴 CRITICAL — Must fix before merge

None found.

🟡 WARNING — Should fix

1. Still no test coverage for the refresh behavior (carried over from previous review, not addressed in this commit). ChapterDetailController.refresh/initialSync, FilteredLessons.refresh(), and the reworked CourseRepository.getFilteredLessonsController/startApiSync cancel-and-restart logic, plus the new onRefresh UI wiring in both screens, remain untested. chapters_list_page_test.dart wasn't updated. Since cache-preservation during refresh is the core goal of this PR, a regression test asserting existing data stays visible (no skeleton flash) during refresh would be valuable before merge.

🔵 SUGGESTION — Nice to have

1. In course_repository.dart's startApiSync() (~line 559), apiSub?.cancel() is still called without await before the new subscription is created (previous suggestion chore(openspec): archive completed lms-navigation-shell change #3, not addressed). Still unlikely to cause a real bug since cancel() detaches the old subscription synchronously and a fresh stream/subscription is created immediately after, but await apiSub?.cancel(); would be more deterministic and easier to reason about.

Note on verification

I wasn't able to run flutter analyze / flutter test in this environment (no Flutter SDK available), so this is based on static review of the diff only — please confirm CI is green before merging.

Verdict

CHANGES REQUESTED → close to APPROVED. Both previously-blocking WARNINGs are resolved; the only remaining item is test coverage, which is strongly recommended given this PR's focus on cache-preservation correctness, but is negotiable if the team is comfortable merging without it and following up separately.

branch: subfolder-pull-to-refresh

[pugal66]

@claude review

[claude]

Claude finished @pugal66's task in 3m 19s —— View job

---

PR Review: Add pull-to-refresh to curriculum subfolder screens

• Read repo guidelines (AGENTS.md, core docs, ADRs)
• Fetch full PR diff and check what changed since last review (commits 44c4684, 77ab3e2, 9599b14)
• Review new test coverage commit
• Review CupertinoSliverRefreshControl → AppRefreshIndicator migration commit
• Review await fix for api subscription cancellation
• Review OpenSpec / SDK boundaries / design system / a11y / data layer for new files
• Post final structured review

---

PR Review Summary

Re-review after the latest 3 commits, which added test coverage, fixed the apiSub await, and — beyond the originally scoped work — migrated CupertinoSliverRefreshControl to the new AppRefreshIndicator across 8 more screens (discussions, exams analytics, profile, testpress).

✅ What's good

• Previous WARNING (no test coverage) is now resolved. chapter_detail_provider_test.dart, filtered_lessons_provider_test.dart, and course_repository_test.dart cover ChapterDetailController.initialSync/refresh (success + error/rethrow paths), FilteredLessons.refresh()/fetchNextPage() delegation, and the repository's cancel-and-restart startApiSync logic. chapters_list_page_test.dart adds a widget test that drags to refresh and asserts existing data (Test Course, Chapter 1 Title) stays on screen — directly exercising the "no skeleton flash" goal.
• Previous SUGGESTION (await apiSub?.cancel()) is now fixed — course_repository.dart:555-559 awaits cancellation before starting a new sync.
• The CupertinoSliverRefreshControl → AppRefreshIndicator migration is mechanical and correct everywhere it's applied: AlwaysScrollableScrollPhysics is preserved, onRefresh callbacks are unchanged, and two files (individual_reports_view.dart, overall_reports_view.dart) now import package:flutter/widgets.dart instead of the full package:flutter/cupertino.dart, which is a net improvement.
• No .g.dart files changed in this batch of commits, so no new generated-file concerns.

🟡 WARNING — Should fix

1. OpenSpec docs no longer match the actual diff scope. proposal.md's "Impact" section and tasks.md only describe changes to chapters_list_page.dart and chapter_detail_page.dart, but commit 44c4684 also migrated downloads_screen.dart, and screens in discussions, exams (subject analytics), profile, and testpress (announcements) to AppRefreshIndicator. This is a low-risk widget swap, not new behavior, so it likely doesn't need a full new OpenSpec change — but the proposal/tasks docs should be updated to reflect the true impact so the spec stays trustworthy as a record of what shipped.
   Fix this →
2. Likely-unintended loading-state regression in forum_posts_list_screen.dart. In the migration commit, the category filter chip row's loading branch changed from loading: () => const Center(child: AppLoadingIndicator()) to loading: () => const SizedBox() (packages/discussions/lib/screens/forum_posts_list_screen.dart:208). AppLoadingIndicator is no longer referenced anywhere else in the file. This isn't related to the RefreshIndicator swap and silently drops the loading spinner for the category filter row — worth confirming this was intentional.
   Fix this →

🔵 SUGGESTION — Nice to have

1. In individual_reports_view.dart and overall_reports_view.dart, onRefresh still calls ref.refresh(subjectAnalyticsPaginationProvider(...).future) (pre-existing, unchanged by this PR), which invalidates the provider and can cause a skeleton flash — the same anti-pattern this PR's design.md explicitly avoided for the chapters screens. Not a regression from this PR, but since AppRefreshIndicator is now standardized across the app, a follow-up to align these two screens with the "await directly, don't invalidate" pattern would make the cache-preservation behavior consistent everywhere the new widget is used.
2. The new test chapters_list_page_test.dart (refresh preserves cache and does not flash skeleton) drags and pumps once, then asserts the existing text is still present — a good directional check, but it doesn't stub a slow/delayed refresh future to explicitly prove a skeleton never renders mid-flight. Consider adding a delayed-future variant for a stronger regression guard.

Note on verification

No Flutter SDK is available in this environment, so flutter analyze / flutter test could not be run — this review is based on static diff analysis only. Please confirm CI is green before merging.

Verdict

APPROVED (with minor follow-ups). All previously-blocking items (error handling, rethrow, test coverage, await on cancel) are resolved. The two WARNINGs above are non-blocking — the OpenSpec doc drift is a documentation nit, and the loading-indicator change is likely low-impact — but worth a quick look before merge.
· branch: subfolder-pull-to-refresh