fix(courses): optimize chapter status filter tab switching

[syed-tp]

• Prevent skeleton shimmer flicker when filtering chapters with local content during background status sync.
• Auto-dispose the chapter status filter provider to reset active tab selection across chapter visits.
• Add widget and unit tests for tab filtering and provider lifecycle.

[claude]

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

---

Review complete

• Read repo guidelines (AGENTS.md, core docs)
• Fetch PR diff and changed files
• Review OpenSpec compliance
• Review SDK boundaries / design system / a11y / motion / navigation / providers / data layer
• Post final review summary

PR Review Summary

✅ What's good

• OpenSpec change is properly included (openspec/changes/optimize-chapter-tab-filtering/) with proposal, design, spec, and tasks — good compliance for a behavior change like this.
• The core fix (chapter.lessons.isEmpty instead of filteredLessons.isEmpty in packages/courses/lib/screens/chapter_detail_page.dart:131) is a small, correct, well-targeted change that matches the described bug (skeleton flicker when a filtered tab is legitimately empty while other local data exists).
• chapterStatusFilterProvider → StateProvider.autoDispose (packages/courses/lib/widgets/chapter_status_filter_bar.dart:9-12) is a reasonable, minimal way to reset filter selection when the widget watching it is unmounted.
• No SDK boundary, Material/Cupertino, design-token, or platform-branching violations — courses still only imports package:core, and the change stays within existing widget/provider usage patterns.
• Widget test wraps with ProviderScope + DesignProvider + LocalizationProvider, consistent with the testing conventions in ai_context.md.

🔴 CRITICAL — Must fix before merge

None found.

🟡 WARNING — Should fix

1. Provider-lifecycle test doesn't actually verify autoDispose behavior — packages/courses/test/screens/chapter_detail_page_test.dart:125-146. The test creates a ProviderContainer, sets state, closes the listener, then disposes the whole container and asserts a brand-new ProviderContainer reads the default value. A brand-new container would return the default state regardless of whether chapterStatusFilterProvider were autoDispose or a plain StateProvider — separate containers never share provider state in the first place. This test would pass even if the .autoDispose change were reverted, so it doesn't cover the claimed behavior ("Auto-dispose the chapter status filter provider to reset active tab selection across chapter visits"). Consider instead asserting on the same container after removing all listeners (e.g. read the state again post-sub.close() — Riverpod disposes autoDispose providers once listener count hits zero — using container.pump()/await Future(() {}) to flush the disposal microtask, or use container.read(chapterStatusFilterProvider.notifier).state = ... then verify via a fresh container.listen call on the same container that it re-initializes to all).
2. Related to (1): the OpenSpec scenario "Filter State Scope" (specs/chapter-tab-filtering/spec.md:12-15) requires the filter to reset "when leaving or entering a chapter detail screen," but there's no widget-level test that mounts ChapterDetailPage, changes the tab, unmounts it, remounts (simulating a chapter revisit), and confirms the filter is back to All. The current unit test operates on ProviderContainer directly, not through the actual widget lifecycle this feature is meant to fix.

🔵 SUGGESTION — Nice to have

1. chapterStatusFilterProvider remains a manually-declared StateProvider rather than @riverpod codegen (chapter_status_filter_bar.dart:9). This predates this PR (only .autoDispose was added here) so it's not blocking, but since this file is being touched, it could be a good opportunity to migrate to codegen for consistency with chapter_detail_provider.dart's pattern, if a follow-up is planned.

Verdict

CHANGES REQUESTED (non-blocking — the WARNING is about test coverage for the auto-dispose claim, not the production code, which looks correct)

[syed-tp]

@claude review

[claude]

Claude finished @syed-tp's task in 2m 18s —— View job

---

PR Review Summary

✅ What's good

• The production fix is minimal and correct: packages/courses/lib/screens/chapter_detail_page.dart:131 now gates the skeleton on chapter.lessons.isEmpty instead of filteredLessons.isEmpty, exactly matching the bug described (empty filtered tabs no longer trigger a shimmer flicker while the chapter already has local data).
• chapterStatusFilterProvider → StateProvider.autoDispose (packages/courses/lib/widgets/chapter_status_filter_bar.dart:9-10) is a small, well-scoped change to reset the tab selection once nothing is watching it.
• OpenSpec change (openspec/changes/optimize-chapter-tab-filtering/) is fully fleshed out with proposal, design, spec, and tasks — this is a bug fix/perf change so OpenSpec isn't strictly required, but including it anyway is good practice and the docs are accurate to the diff.
• The test suite (packages/courses/test/screens/chapter_detail_page_test.dart) now properly closes the gap flagged in the earlier automated review on this PR:
  • The unit test (lines 191-213) listens on a ProviderContainer, mutates state, closes the listener, awaits the autoDispose microtask (await Future(() {})), and re-reads the same container to confirm it reset to all — this actually exercises autoDispose, unlike testing via a brand-new container.
  • The new widget test "leaving and re-entering ChapterDetailPage resets filter to All" (lines 126-189) mounts ChapterDetailPage with a ValueKey per chapter, changes the tab, unmounts, remounts with a different chapter, and asserts the filter is back to All — this covers the OpenSpec "Filter State Scope" scenario at the actual widget-lifecycle level.
  • No SDK boundary, Material/Cupertino, design-token, or platform-branching violations; courses still only imports package:core (test files' internal imports like courses/providers/... are fine since they're within-package).

🔴 CRITICAL — Must fix before merge

None found.

🟡 WARNING — Should fix

None found. (Both WARNINGs from the earlier automated pass on this PR — inadequate autoDispose unit test, and missing widget-level coverage for the reset-on-navigation scenario — are resolved by the latest commit, 0ac2fc2.)

🔵 SUGGESTION — Nice to have

1. chapterStatusFilterProvider is a global (non-family) provider. The reset-on-navigation behavior relies on the old ChapterDetailPage fully unmounting before the new one mounts (true for the current GoRouter setup in study_routes.dart/exams_routes.dart/global_routes.dart, where navigating between chapters pops back through the chapters list). If a future "next chapter" flow ever pushes a new ChapterDetailPage directly on top of the current one without popping, the old page's still-mounted filter bar would keep the provider alive and the selection could leak into the next chapter. Not an issue today, but worth a comment or family-scoping (.autoDispose.family<..., chapterId>) if such navigation is ever added.
2. chapterStatusFilterProvider remains a manually-declared StateProvider rather than @riverpod codegen (chapter_status_filter_bar.dart:9), and the "All" tab label is still a hardcoded English string rather than going through l10n (unlike the other three tabs). Both predate this PR and weren't touched here, so non-blocking — but since this file is being edited again, could be a good opportunity to fold in if a follow-up cleanup is planned.

Verdict

APPROVED