fix(dashboard): prevent banner skeleton flash on app reopen when banners are null

[syed-tp]

On app reopen, cached dashboard feeds (like Resume Learning and What's New) render immediately from local storage. However, for institutes with no banner ads, the carousel section displayed a placeholder skeleton during background sync before abruptly disappearing.

• Evaluate cached dashboard sections to skip the banner skeleton on reopen.
• Display loading skeletons only during cold start when no cached data exists.

[claude]

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

---

Claude finished reviewing this PR

• Read AGENTS.md and core docs for repo rules
• Gather PR diff and changed files
• Review OpenSpec compliance
• Review SDK boundary / import rules
• Review design system, a11y, motion, navigation, riverpod, data layer rules
• Review generated file changes (.g.dart)
• Review test coverage
• Post final structured review

PR Review Summary

✅ What's good

• Correct root-cause fix: the old skeleton signal was bootstrapState.isLoading, which is true on every reopen while the background sync runs, even when there's nothing to show — that's what caused the abrupt skeleton→disappear flash for institutes with no banners. Switching to !bootstrapState.hasValue && !hasCachedDashboard (in the carousel and lesson cards widgets) correctly distinguishes "no data yet" from "resyncing existing data."
• dashboardBootstrap was correctly switched to @Riverpod(keepAlive: true), and dashboard_providers.g.dart was regenerated consistently (AutoDisposeFutureProvider → FutureProvider), matching the PR's "keep bootstrap sync alive across the session" goal.
• Good new widget tests covering initial bootstrap loading, completed-empty state, and the cached-reopen scenario for the carousel section.
• No SDK boundary, design-system, Material/Cupertino, or platform-neutrality violations found in the touched files.

🔴 CRITICAL — Must fix before merge

1. top_learners_section_widget.dart:21 reintroduces the exact bug this PR fixes. final isInitialLoading = !bootstrapState.hasValue; dropped the cached-data check that the sibling widgets use. learnersProvider (packages/core/lib/data/repositories/leaderboard_repository.dart → watchLeaderboard) reads from the local Drift DB just like the lesson/hero feeds, so cached leaderboard data is available immediately on reopen — but with this logic the Top Learners skeleton will still flash on every reopen until the background bootstrap resolves, regardless of cached data. It should mirror the pattern used in top_carousel_section_widget.dart/lesson_cards_section_wrapper.dart, e.g.:

   final learners = learnersState.valueOrNull ?? const <LearnerDto>[];
   final isInitialLoading = !bootstrapState.hasValue && learners.isEmpty;

   Fix this →

2. Unrelated hand-edited/stray generated file: packages/courses/lib/providers/course_list_provider.g.dart. The _$courseSearchHash() value changed, but the corresponding source file packages/courses/lib/providers/course_list_provider.dart has no changes in this PR (git diff origin/main...HEAD is empty for it). A .g.dart hash changing with no source diff is the signature of a hand-edit or a stray/unrelated build_runner output and shouldn't be part of this PR's diff.
   Fix this →

🟡 WARNING — Should fix

1. Test coverage gap. New tests only cover TopCarouselSectionWidget. TopLearnersSectionWidget and LessonCardsSectionWrapper got the same kind of isLoading logic change but no equivalent "cached data on reopen" test — a test like the one added for the carousel would have caught CRITICAL feat(ui): implement LMS core primitives #1 above.
2. Possible stuck-skeleton edge case in top_carousel_section_widget.dart:25. showHeroSkeleton = !bootstrapState.hasValue && !hasCachedDashboard means that on a cold start with no cache where dashboardBootstrapProvider resolves to an AsyncError (e.g. persistent network failure), hasValue never becomes true, so the skeleton stays visible indefinitely. Previously, isLoading would turn false on error and the section could collapse to SizedBox.shrink(). Worth confirming this is intentional, or handle the error case explicitly.

🔵 SUGGESTION — Nice to have

1. The !bootstrapState.hasValue && !hasX pattern is now duplicated across three widgets with slightly different data-source combinations. A small shared helper/extension (e.g. bootstrapState.isInitialLoad(hasCachedData)) would reduce duplication and prevent the kind of drift seen in CRITICAL feat(ui): implement LMS core primitives #1.

Verdict

CHANGES REQUESTED
· Branch: fix/dashboard-skeleton-flash

[syed-tp]

@claude review

[claude]

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

---

Claude finished reviewing this PR

• Read AGENTS.md and core docs for repo rules
• Gather PR diff and changed files
• Review OpenSpec compliance
• Review SDK boundary / import rules
• Review design system, a11y, motion, navigation, riverpod, data layer rules
• Review generated file changes (.g.dart)
• Review test coverage
• Post final structured review

Note: the branch has moved since the last review — a new commit (2c449b1) refactored the per-widget cache checks into two shared providers, hasCachedDashboardProvider and isDashboardInitialLoadingProvider. That fixes the previously-flagged Top Learners bug and the stuck-skeleton-on-error case, but introduces a new gap described below.

PR Review Summary

✅ What's good

• Good consolidation: isDashboardInitialLoadingProvider (packages/courses/lib/providers/dashboard_providers.dart:96-101) replaces three near-duplicate bootstrapState.isLoading && !hasX checks with one shared provider, addressing the duplication called out in the prior review pass.
• TopLearnersSectionWidget no longer uses the bare !bootstrapState.hasValue check that previously ignored cached leaderboard data — it now goes through the shared provider like its siblings.
• The stuck-skeleton-on-error edge case from the previous review is now fixed: isDashboardInitialLoading keys off bootstrapState.isLoading (not !hasValue), so an AsyncError correctly collapses the skeleton instead of leaving it stuck forever. Covered by the new 'collapses skeletons and does not stay stuck when bootstrap fails on cold start' test.
• dashboardBootstrap correctly switched to @Riverpod(keepAlive: true) with dashboard_providers.g.dart regenerated consistently (AutoDisposeFutureProvider → FutureProvider).
• Solid new widget tests for cold start, completed-empty, cached-reopen, and error scenarios.

🔴 CRITICAL — Must fix before merge

1. hasCachedDashboardProvider omits the leaderboard feed, so Top Learners can pop in/disappear based on unrelated sections' cache state. hasCachedDashboardProvider (packages/courses/lib/providers/dashboard_providers.dart:84-90) only inspects whatsNewFeedProvider, resumeLearningFeedProvider, recentlyCompletedFeedProvider, and heroBannersProvider — it never looks at learnersProvider. But TopLearnersSectionWidget (packages/testpress/lib/screens/dashboard/widgets/top_learners_section_widget.dart:19-24) now derives its loading state from that same global flag:

   final isInitialLoading = ref.watch(isDashboardInitialLoadingProvider);
   ...
   if (learners.isEmpty && !isInitialLoading) {
     return const SizedBox.shrink();
   }

   If, say, resumeLearningFeedProvider has cached data but the leaderboard hasn't synced yet, hasCachedDashboard is true → isDashboardInitialLoading is false globally → Top Learners immediately renders SizedBox.shrink() (no skeleton) even though its own data is still mid-sync, then pops in once the leaderboard resolves. This is the same class of bug the PR sets out to fix (skeleton/visibility driven by the wrong signal), just shifted onto the leaderboard section instead of eliminated. None of the new tests cover "leaderboard uncached while a sibling feed is cached," so this regression isn't caught.
   Fix: gate on the section's own async state too, e.g. final isInitialLoading = ref.watch(isDashboardInitialLoadingProvider) || (learnersState.isLoading && learners.isEmpty);, or include learnersProvider in the cache aggregation (being mindful that leaderboard can be disabled per institute, so watching it unconditionally may trigger an unwanted fetch).
   Fix this →

2. Unrelated hand-edited/stray generated file: packages/courses/lib/providers/course_list_provider.g.dart — still present on the branch. _$courseSearchHash() changed, but packages/courses/lib/providers/course_list_provider.dart has zero diff against origin/main. A .g.dart hash changing with no corresponding source change is the signature of a stray/unrelated build_runner run and shouldn't be part of this PR's diff.
   Fix this →

🟡 WARNING — Should fix

1. The shared isDashboardInitialLoadingProvider treats the whole dashboard as one atomic warm/cold signal rather than being section-aware. Beyond the leaderboard case above, TopCarouselSectionWidget and LessonCardsSectionWrapper have the same structural property: if any of the four feeds in hasCachedDashboardProvider has cache, isInitialLoading goes false for every section, even ones whose own feed hasn't loaded yet — trading "skeleton flash" for "silent pop-in" for that section. This may be an acceptable/intended trade-off (a pop-in is arguably less jarring than a flash), but worth confirming it's deliberate rather than incidental, since it wasn't called out in the PR description.
2. Test coverage gap. All new tests set up either "everything uncached" or "the-section-under-test's-own-data cached" scenarios. None test the mixed case (one feed cached, another still syncing) that would have caught CRITICAL feat(ui): implement LMS core primitives #1.

🔵 SUGGESTION — Nice to have

1. Since hasCachedDashboardProvider/isDashboardInitialLoadingProvider are now shared app-wide signals, consider documenting (short comment or ADR note) that they intentionally represent dashboard-wide warm/cold state rather than per-section cache, so future contributors don't assume the granularity implied by section-specific usage.

Verdict

CHANGES REQUESTED

[syed-tp]

@claude review

Atomic Sync & Single Endpoint

• The dashboard is not fetched via separate per-section endpoints.
• DashboardRepository.refreshDashboard() calls a single getDashboard() API.
• All dashboard sections — Hero Banners, Lesson Feeds, and Leaderboard — are persisted to the local Drift SQLite database within a single atomic transaction.
• Since all feeds are populated together in SQLite, a partial-sync state (e.g., lessons cached while the leaderboard is uncached) does not occur during normal lifecycle flows.

Unified Warm/Cold State

Cold Start — First Login / Empty SQLite

• hasCachedDashboard evaluates to false.
• isDashboardInitialLoading is set to true.
• Skeletons are displayed uniformly until the initial dashboard sync completes.

App Reopen — Warm SQLite Cache

• All feeds read from the cached SQLite records immediately when the screen mounts.
• hasCachedDashboard evaluates to true.
• isDashboardInitialLoading remains false.
• Cached content is rendered instantly without skeleton flashes while the background sync runs.

Leaderboard Gating

• TopLearnersSectionWidget checks isLeaderboardEnabled before evaluating loading states or rendering data.
• If the institute has disabled the leaderboard, the widget immediately returns SizedBox.shrink().
• This prevents unnecessary loading states and UI work when the leaderboard is disabled.

Stray Generated File Reverted

• packages/courses/lib/providers/course_list_provider.g.dart has been reverted to match origin/main (390cf8...).
• This ensures there are no unrelated changes in the PR.

Test Coverage & Error Recovery

Added comprehensive unit test coverage in paid_active_home_screen_test.dart covering:

• Initial cold-start skeleton display.
• App reopen with cached banners and top learners, verifying zero skeleton flashes during background refresh.
• Empty banner collapse after a completed sync.
• Cold-start network failure recovery, ensuring AsyncError properly collapses skeletons instead of leaving the UI in a frozen loading state.

Validation

• 23 tests in packages/testpress — passed
• 73 tests in packages/courses — passed
• flutter analyze — passed with 0 issues

[claude]

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

---

Claude finished reviewing this PR

• Read AGENTS.md and core docs for repo rules
• Gather latest PR diff and changed files (branch has moved since last review)
• Verify fixes for previously flagged CRITICAL issues (Top Learners cache gap, stray generated file)
• Review new leaderboard gating logic
• Review new test coverage
• Post final structured review

Note: the branch has moved again since the last review pass — two new commits (4e8e0c7, 0667c3e, af295f3) landed. I verified refreshDashboard() (packages/core/lib/data/repositories/dashboard_repository.dart:37-250) writes banners, lessons, and the leaderboard's AllTimeLeaderboardTable inside the same Drift _db.transaction, which confirms the "atomic sync" claim in the comment above and resolves the previously-flagged Top Learners cache gap. However, a new regression was introduced that reverses one of this PR's own stated goals.

PR Review Summary

✅ What's good

• Stray generated file resolved. git diff origin/main...HEAD -- packages/courses/lib/providers/course_list_provider.g.dart is now empty — the unrelated hash change is gone.
• Top Learners cache gap resolved. Verified against the repository code: since DashboardRepository.refreshDashboard() persists banners, What's New/Resume/Completed feeds, and the leaderboard's all-time table inside one atomic transaction, they can never be partially cached relative to each other in normal lifecycle flows — so gating TopLearnersSectionWidget off the shared isDashboardInitialLoadingProvider (top_learners_section_widget.dart:19) is sound, and the leaderboard-uncached-while-sibling-cached race from the previous review pass cannot actually occur.
• Stuck-skeleton-on-error case remains fixed — isDashboardInitialLoadingProvider (dashboard_providers.dart:96-101) keys off bootstrapState.isLoading, so an AsyncError correctly collapses skeletons, covered by the new 'collapses skeletons...on cold start' test.
• Good added test coverage for cold start, cached reopen (carousel + top learners), completed-empty collapse, and error recovery in paid_active_home_screen_test.dart.

🔴 CRITICAL — Must fix before merge

1. dashboardBootstrap was reverted from keepAlive: true back to plain @riverpod (autoDispose), directly contradicting this PR's own stated goal ("Keep dashboard bootstrap sync state alive across the session") and undoing the change praised in the previous review pass.
   packages/courses/lib/providers/dashboard_providers.dart:11-12:

   @riverpod
   Future<void> dashboardBootstrap(Ref ref) async { ... }

   This was introduced by commit 0667c3e ("refactor: update dashboardBootstrap to use autoDispose instead of keepAlive") and is still in place after the latest "regenerate providers" commit (dashboard_providers.g.dart:14 now generates AutoDisposeFutureProvider<void> again). Per repo rules (keepAlive: true reserved for "truly app-lifetime state"), and given the PR description explicitly calls this out as a goal, this looks like an accidental revert rather than an intentional change. Practically, this means every time the dashboard screen's widget subtree is fully disposed and remounted within a session (e.g. navigating away and back, depending on how the shell branch caches state), refreshDashboard() re-fires from scratch instead of the bootstrap staying resolved — reintroducing the redundant-refetch problem this PR set out to fix, even though the hasCachedDashboardProvider fallback happens to mask it from a pure skeleton-flash perspective.
   Fix this →

🟡 WARNING — Should fix

1. Environment limitation: I was unable to run flutter test / flutter analyze in this review environment (no Flutter SDK available), so the "23/73 tests passed, 0 analyze issues" validation claimed in the PR comment could not be independently re-verified — please confirm CI is green.
2. The shared isDashboardInitialLoadingProvider still ties all three sections to one dashboard-wide warm/cold signal rather than being section-aware. This is now justified by the atomic-transaction design (see above), but that invariant is implicit — if a future change ever adds a section that syncs independently of refreshDashboard() (e.g. a standalone refreshLeaderboard() call path via LeaderboardRepository, which already exists alongside the dashboard-bundled leaderboard write), this same class of bug could resurface silently. Worth a short comment on hasCachedDashboardProvider/isDashboardInitialLoadingProvider documenting that they rely on all dashboard sections being written atomically together.

🔵 SUGGESTION — Nice to have

1. None beyond the above — the consolidation from the previous pass (hasCachedDashboardProvider / isDashboardInitialLoadingProvider) is clean and removed the earlier duplication concern.

Verdict

CHANGES REQUESTED — please restore keepAlive: true on dashboardBootstrap (CRITICAL #1) before merge; everything else looks solid.
· Branch: fix/dashboard-skeleton-flash

[syed-tp]

@claude review

Why not keepAlive: true: With keepAlive: true, dashboardBootstrap permanently cached AsyncData(null) on first run and never executed again when returning to the dashboard, preventing background data from ever refreshing during the session.

Decoupling sync from caching: In our offline-first architecture, Drift SQLite is the cache layer, while dashboardBootstrap is an on-mount sync trigger. The goal was to prevent UI skeleton flashing, not suppress background synchronization.

Cache-aware loading fix: By gating UI skeletons on isDashboardInitialLoadingProvider (which inspects SQLite cache), cached content displays instantly with zero skeleton flash while dashboardBootstrap runs silently in the background.

PR Description: We'll update the PR description to clarify that flash elimination is achieved via SQLite cache inspection rather than keepAlive: true.

[claude]

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

---

Review update — response to @syed-tp's keepAlive explanation

• Re-read repo rules (AGENTS.md, core docs)
• Pull latest diff (branch unchanged since last review — still at af295f3)
• Verify the keepAlive/autoDispose reasoning against actual provider lifecycle behavior
• Verify isDashboardInitialLoadingProvider truly decouples skeleton-flash fix from bootstrap re-execution
• Check whether previously flagged CRITICAL issues remain resolved
• Post final structured review

No new commits landed since the last review pass — this is a re-assessment of CRITICAL #1 (the keepAlive revert) in light of your explanation.

Verifying the explanation

I traced how dashboardBootstrapProvider is actually consumed: it's only watched transitively, via isDashboardInitialLoadingProvider (dashboard_providers.dart:96-101), from the three section widgets. The Home tab sits inside a StatefulShellRoute.indexedStack (app_router.dart:31,46), which keeps offstage branches mounted — so a plain tab switch away from and back to Home does not tear down these widgets or their provider subscriptions, regardless of keepAlive/autoDispose. There's also no WidgetsBindingObserver/didChangeAppLifecycleState hook in the dashboard screen, so simply backgrounding/foregrounding the OS app (without the process being killed) doesn't remount anything either — in both of those cases autoDispose vs keepAlive would behave identically.

Where the distinction does matter is a full teardown-and-remount of the Home widget subtree within the same app process — e.g. the session-expiry → re-login flow from #284, or any navigation path that fully unmounts the authenticated shell and later rebuilds it. In that case:

• keepAlive: true → the provider is never disposed, so it keeps returning the stale cached AsyncData(null) forever and refreshDashboard() never fires again.
• plain @riverpod (autoDispose) → losing all listeners disposes it, so remounting creates a fresh instance and refreshDashboard() runs again.

Separately, the skeleton-flash fix itself doesn't depend on this at all: hasCachedDashboardProvider reads directly from the Drift-backed stream providers (whatsNewFeedProvider, resumeLearningFeedProvider, recentlyCompletedFeedProvider, heroBannersProvider), which emit their cached rows immediately on subscribe, independent of whether dashboardBootstrap itself is mid-flight or freshly re-triggered.

So the explanation checks out: autoDispose here is the correct choice for keeping "sync trigger" and "skeleton flash" decoupled as intended, and it doesn't regress the original bug. I'm withdrawing CRITICAL #1 from the previous review.

Remaining items from the last pass (still valid, branch unchanged)

🟡 WARNING — Should fix

1. Please do follow through on updating the PR description as promised — the current description still frames the fix in a way that doesn't mention the atomic-sync/cache-inspection design, which was the source of three rounds of back-and-forth review here.
2. Consider adding a short comment above dashboardBootstrapProvider and isDashboardInitialLoadingProvider (dashboard_providers.dart:11-15, 96-101) capturing this reasoning (bootstrap is an autoDispose on-mount sync trigger, intentionally decoupled from the Drift-cache-driven skeleton signal) so a future contributor doesn't "fix" it back to keepAlive again.
3. Test coverage still doesn't cover the specific scenario that motivated the autoDispose choice (Home subtree fully torn down and remounted mid-session, verifying refreshDashboard() fires again). Given how much discussion this point generated, a regression test would be valuable.

Verdict

APPROVED (pending the PR description update and, ideally, the doc-comment/test items above — none of which are blocking).