feat(courses): add LearnLens AI Chat and AI Practice Quiz to video lesson viewer

[syed-tp]

• Add AI Chat and AI Practice Quiz tabs for video lessons using LearnLens integration.
• Support quiz generation, difficulty filters, hint cards with timestamp cues, and multi-language localizations.
• Follow design system tokens and WCAG 2.5.5 touch target accessibility standards.

[claude]

Claude finished @syed-tp's task in 5m 14s —— View job

---

Claude finished the review

• Read AGENTS.md and core docs for repo rules
• Check OpenSpec change / ADRs (openspec/changes/video-ai-chat-mcq/ present, satisfies SDD requirement)
• Fetch full PR diff (gh pr diff 233)
• Review SDK boundaries, design system, accessibility, motion, navigation, providers, data layer, generated files, localization
• Post structured review

PR Review Summary

✅ What's good

• OpenSpec change (video-ai-chat-mcq) with proposal/design/tasks/specs is present — satisfies the SDD requirement for this new feature.
• Data layer correctly follows the 3-layer pattern: DataSource.createAiSession added to the abstract interface and implemented in both HttpDataSource and MockDataSource (packages/core/lib/data/sources/*.dart), with LearnLensRepository sitting on top.
• SDK import boundaries respected — courses only imports package:core, no cross-domain imports found.
• ai_tab.dart actually removed a Material/TextField usage in favor of AppTextField, and the send button now uses AppIconButton (which bakes in AppSemantics.button + 48dp target) instead of the previous bare, unlabeled AppFocusable. Good cleanup.
• video_mcq_filter_sheet.dart's _buildChip correctly wraps AppFocusable with AppSemantics.button(...) — the right pattern (see 🔴 feat(ui): implement LMS core primitives #1 for where this pattern is missing elsewhere).
• Most new strings are properly localized via ARB files across en/ar/ml/ta (see 🟡 feat(ui): implement LMS core primitives #1 for the exceptions).

🔴 CRITICAL — Must fix before merge

1. Quiz answer-correctness logic is broken and can mark wrong answers as correct. _isOptionCorrect() in packages/courses/lib/widgets/lesson_detail/mcq/video_mcq_stepper_card.dart:33-41 (duplicated in video_mcq_summary_card.dart:19-27) does:

   if (cleanOpt.startsWith(cleanAns)) return true;
   if (cleanAns.startsWith(cleanOpt[0])) return true;

   The last line only compares the first character. E.g. options "Prague" / "Paris" with correctAnswer = "Paris": "Paris".startsWith("P") is true, so "Prague" is also flagged correct (green checkmark) even though it's wrong. Any two options sharing a first letter will trigger this. This silently corrupts quiz scoring and the "correct answer" highlight. Recommend matching against a stable option identifier/index from the API instead of fuzzy string prefixes.
   Fix this →

2. Missing AppSemantics on interactive elements in the new MCQ stepper card, contrary to AGENTS.md §4 and the correct pattern already used in video_mcq_filter_sheet.dart's _buildChip. In video_mcq_stepper_card.dart:

   • Option rows (line ~209): AppFocusable(onTap: ... ) with no AppSemantics.button() wrapper — screen readers get no role/label/selected-state for quiz options.
   • Hint toggle (line ~245): same issue.
   • Inline video-timestamp chips inside hints/explanations (_buildTextWithTimestamps, line ~97): tappable via AppFocusable but unlabeled for assistive tech, and the touch target (padding 6h/2v around an 11px icon + caption text) is well under the 48×48dp WCAG 2.5.5 minimum this PR's own description claims to follow.
     Fix this →

3. Chat message list isn't wrapped in AppSemantics.scrollableList(). ai_tab.dart line ~184 introduces a ListView.builder for chat messages with no semantics wrapper, unlike the established pattern elsewhere in the repo (product_list.dart, category_filter_bar.dart, top_learners_section.dart all use AppSemantics.scrollableList(...)).

4. course_list_provider.g.dart changed without its source file changing — courseListHash/courseSearchHash in packages/courses/lib/providers/course_list_provider.g.dart were regenerated with new hashes, but course_list_provider.dart was last modified in an unrelated earlier commit (47d2b18, PR feat(core): graceful 401 session expiry dialog #227) and is not part of this PR's diff. Per the generated-files rule, a .g.dart changing while its source doesn't strongly suggests a stale/mismatched build_runner run (possibly generated against local uncommitted state). Please regenerate cleanly from this branch's actual source or drop this unrelated diff.

🟡 WARNING — Should fix

1. Hardcoded, non-localized user-visible strings, inconsistent with the rest of this PR (which added ~20 new ARB keys correctly):

   • "Easy" / "Medium" / "Hard" chip labels — video_mcq_filter_sheet.dart:95,102,109
   • Difficulty label capitalization — video_mcq_initial_card.dart:77 ('${difficulty[0].toUpperCase()}...')
   • accessibilityLabel: 'Previous Question' / 'Next Question' — video_mcq_stepper_card.dart:336,347
   • accessibilityLabel: 'Send Message' — ai_tab.dart:232
   • Score summary '$correctCount out of ${questions.length} Correct ($percentage%)' — video_mcq_summary_card.dart:76

2. New error paths use debugPrint instead of the established SentryService error-tracking pattern (ADR 0006-sentry-error-tracking.md, already used in lesson_detail_orchestrator.dart via ref.read(sentryServiceProvider)): AI session creation failures (learnlens_provider.dart:41), chat failures (ai_tab.dart:141), and quiz-load failures (video_mcq_tab.dart:89) are only logged locally and won't surface in Sentry.

3. LearnLens base URL is hardcoded in learnlens_network_client.dart:11 (_learnLensBaseUrl = 'https://learnlens.testpress.in') instead of being sourced via AppConfig/--dart-define, unlike apiBaseUrl and the new learnLensOrgUuid. This breaks per-instance/white-label overriding of the LearnLens endpoint.

🔵 SUGGESTION — Nice to have

• Consider deduplicating _isOptionCorrect (once fixed) into a single shared helper instead of copy-pasting it in both video_mcq_stepper_card.dart and video_mcq_summary_card.dart.

Verdict

CHANGES REQUESTED

[syed-tp]

@claude review

[claude]

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

---

Claude finished reviewing

• Read AGENTS.md and core docs for repo rules
• Check OpenSpec change / ADRs (openspec/changes/video-ai-chat-mcq/ present)
• Diff PR commits against base (git diff 8acc464...1263f23, the commit range for this PR)
• Review SDK boundaries, design system, accessibility, motion, providers, data layer, generated files, localization
• Post structured review

Note on prior review

The repo's shallow clone has no common ancestor with origin/main, so I diffed the PR's own commit range (8acc464..1263f23, all 6 commits) instead. This is the same code a prior automated review (posted earlier today) looked at. Since then several of that review's CRITICAL/WARNING items have clearly been fixed in later commits (9cf0b14, 92698fe, a82d035) — I verified each one against the current code rather than assuming. Updated status below.

PR Review Summary

✅ What's good

• _isOptionCorrect bug is fixed. The broken cleanAns.startsWith(cleanOpt[0]) heuristic is gone. Logic now lives in one place (LearnLensQuizQuestionDto.isOptionCorrect in packages/core/lib/data/models/learnlens_dto.dart:105-122) and is used by both the stepper and summary cards — also resolves the earlier "dedupe this" suggestion.
• Accessibility semantics added. Option rows, hint toggle, and inline timestamp chips in video_mcq_stepper_card.dart are now wrapped in AppSemantics.button(...), and the chat list in ai_tab.dart:185 now uses AppSemantics.scrollableList(...), matching the established pattern in product_list.dart/video_mcq_filter_sheet.dart.
• Localization is thorough. Difficulty chips, prev/next labels, send-message label, and the score summary are now all sourced from L10n.of(context) (ARB keys added across en/ar/ml/ta).
• Sentry integration added for AI session, chat, and quiz-load failures via sentryServiceProvider.
• Data layer still correctly follows the 3-layer pattern (DataSource → HttpDataSource/MockDataSource → LearnLensRepository → @riverpod provider), SDK import boundaries are respected, and LearnLensRepository/learnLensNetworkClientProvider are legitimate DI-wiring providers (not app-state, so manual Provider/plain classes are fine here per AGENTS rules).

🔴 CRITICAL — Must fix before merge

1. course_list_provider.g.dart still changes without its source changing. courseListHash/courseSearchHash differ between the PR's base and head (f92f267 → 40cad83), but git diff on course_list_provider.dart across the same range is empty — the source file was not touched by any commit in this PR. This means the checked-in generated file doesn't match what build_runner would produce from this branch's actual source, which will cause CI/codegen-check drift or mask an unrelated stale regen. Please regenerate *.g.dart cleanly from a clean build_runner build on this branch, or drop this hunk if it's leftover from a rebase/local run.
   Fix this →

2. WCAG 2.5.5 touch targets still under 48×48dp, even though the missing AppSemantics wrapping (previously flagged) is now fixed. In video_mcq_stepper_card.dart:

   • Inline timestamp chip (_buildTextWithTimestamps, ~line 93-99): Container padding is EdgeInsets.symmetric(horizontal: 8, vertical: 4) around an 11px icon + caption — total tappable height is roughly 24px.
   • Hint toggle (~line 250-254): padding is EdgeInsets.symmetric(horizontal: design.spacing.xs, vertical: design.spacing.xs) (4px both axes) around a 16px icon + label — also roughly 24px tall.
     Both are well under the 48dp minimum this PR's own description claims to follow. Screen-reader users now get a correct label, but low-motor-control / touch users still get an undersized target.
     Fix this →

🟡 WARNING — Should fix

1. New hardcoded, non-localized string slipped in despite this PR's otherwise thorough localization pass: video_mcq_tab.dart:69 — _errorMessage = 'Unable to start AI session for quiz generation.';. Every other error path in this PR (ai_tab.dart's videoAiSessionError, videoMcqFailedToLoad) correctly uses L10n.of(context); this one path was missed.

2. Hardcoded animation duration, no motion-preference check. ai_tab.dart's _ThreeDotWavingIndicator (~line 353) uses duration: const Duration(milliseconds: 1200) directly on AnimationController, and never checks MotionPreferences.shouldAnimate(context) before repeating. Established pattern elsewhere in courses (transcripts_tab.dart, notes_tab.dart, pdf_viewer.dart) uses MotionPreferences.duration(context, design.motion.*). As written, the "AI thinking" dots animation ignores the user's reduced-motion setting and hardcodes a duration outside the design token set.

3. LearnLens base URL is still hardcoded. learnlens_network_client.dart:11 — _learnLensBaseUrl = 'https://learnlens.testpress.in'. AppConfig.learnLensOrgUuid (added in this same PR) correctly follows the --dart-define pattern used by apiBaseUrl, but the base URL itself doesn't, which breaks per-instance/white-label overriding of the LearnLens endpoint.

4. New Drift columns added without a schema migration. lessons_table.dart adds canEnableLearnlensAi, learnlensAssetId, learnlensAssetStatus, but AppDatabase.schemaVersion (app_database.dart:59) stays at 1, and onUpgrade only creates missing tables (app_database.dart:66-77) — it never adds columns to existing tables. This appears to be a pre-existing gap in the migration strategy (schemaVersion has been 1 since the DB was introduced), so it's not new to this PR, but it means users with an already-created local DB won't get these 3 new columns until a reinstall/data wipe, and any Drift read/write against them will fail on those installs. Worth raising given this PR depends on it for the AI-availability gating (isAiAvailable check in video_lesson_viewer.dart).

🔵 SUGGESTION — Nice to have

• Minor: debugPrint(...) calls remain alongside the new sentryServiceProvider().captureException(...) calls in ai_tab.dart:141, video_mcq_tab.dart:89, and learnlens_provider.dart:41-42. Sentry now covers reporting, so the local debugPrint is redundant in production builds and could be dropped or gated behind kDebugMode.

Verdict

CHANGES REQUESTED

Most of the previous critical accessibility/localization/scoring issues are resolved. What remains: the stale generated-file diff, touch-target sizing on two elements, one missed localized string, a motion-preference gap, and the hardcoded LearnLens URL.

[syed-tp]

@claude review

LearnLens base URL is still hardcoded. learnlens_network_client.dart:11 — _learnLensBaseUrl = 'https://learnlens.testpress.in'. AppConfig.learnLensOrgUuid (added in this same PR) correctly follows the --dart-define pattern used by apiBaseUrl, but the base URL itself doesn't, which breaks per-instance/white-label overriding of the LearnLens endpoint. - its fine

New Drift columns added without a schema migration. lessons_table.dart adds canEnableLearnlensAi, learnlensAssetId, learnlensAssetStatus, but AppDatabase.schemaVersion (app_database.dart:59) stays at 1, and onUpgrade only creates missing tables (app_database.dart:66-77) — it never adds columns to existing tables. This appears to be a pre-existing gap in the migration strategy (schemaVersion has been 1 since the DB was introduced), so it's not new to this PR, but it means users with an already-created local DB won't get these 3 new columns until a reinstall/data wipe, and any Drift read/write against them will fail on those installs. Worth raising given this PR depends on it for the AI-availability gating (isAiAvailable check in video_lesson_viewer.dart). - its in development, so we can ignore it (the app is not released)

[claude]

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

---

Claude finished reviewing

• Read AGENTS.md and core docs for repo rules
• Noted author's dispositions on the two prior WARNING items (LearnLens base URL, Drift migration) — treated as acknowledged/deferred per author, not re-flagged as blocking
• Diffed latest commits (cdf74a6, 9524a9e) against what the previous review flagged as outstanding
• Verified touch-target fix (cdf74a6)
• Verified motion-preference + localization fix (9524a9e)
• Re-checked course_list_provider.g.dart stale-generated-file issue against origin/main
• Posted updated review

PR Review Summary

✅ What's good — confirmed fixed since last review

• Touch targets now meet 48×48dp (cdf74a6). The inline timestamp chip and hint toggle in video_mcq_stepper_card.dart are now wrapped in ConstrainedBox(minWidth: 48, minHeight: 48) around the visual content, satisfying WCAG 2.5.5 without changing the visible chip size.
• Motion-preference gap fixed (9524a9e). _ThreeDotWavingIndicator in ai_tab.dart now sources its duration from MotionPreferences.duration(context, design.motion.slow), checks MotionPreferences.shouldAnimate(context) before repeating, and renders static (non-animating) dots when reduced motion is on — matching the pattern used elsewhere in courses.
• Missed hardcoded string fixed (9524a9e). video_mcq_tab.dart:69 now uses L10n.of(context).videoMcqSessionError instead of a literal string.

Acknowledged, not re-flagged (per author)

• LearnLens base URL hardcoded in learnlens_network_client.dart:11 — author confirmed this is fine for now.
• Missing Drift schema migration for the new lessons_table.dart columns — author confirmed this is acceptable pre-release, app not yet in production.

🔴 CRITICAL — Still outstanding

1. course_list_provider.g.dart still doesn't match its source. Comparing directly against origin/main: courseListHash/courseSearchHash differ (5c4261d... → 6741d1f..., 754041d... → 390cf82...), but course_list_provider.dart has zero diff against origin/main across the entire branch — confirmed no commit in this PR touches that file. The regenerated hash first appears in commit 7ba65aa, alongside the LearnLens feature work, with no corresponding source change. This means the checked-in .g.dart doesn't reflect what build_runner would currently produce from this branch's actual source, which will show up as codegen drift in CI (or mask a stale/local regen). Please run a clean dart run build_runner build --delete-conflicting-outputs on this branch and either commit the correct resulting diff or revert this file to match origin/main.
   Fix this →

🔵 SUGGESTION — Nice to have (unchanged from last review)

• debugPrint(...) calls remain alongside the sentryServiceProvider().captureException(...) calls in ai_tab.dart:141, video_mcq_tab.dart:89, and learnlens_provider.dart:41. Sentry now covers reporting; the local debugPrint is redundant in production builds and could be dropped or gated behind kDebugMode.

Verdict

CHANGES REQUESTED — only the stale course_list_provider.g.dart generated file remains blocking. All accessibility, localization, and motion-preference issues from the prior review are resolved, and the two remaining warnings are acknowledged/deferred by the author.

[syed-tp]

@claude code

course_list_provider.g.dart still doesn't match its source. Comparing directly against origin/main: courseListHash/courseSearchHash differ (5c4261d... → 6741d1f..., 754041d... → 390cf82...), but course_list_provider.dart has zero diff against origin/main across the entire branch — confirmed no commit in this PR touches that file. The regenerated hash first appears in commit 7ba65aa, alongside the LearnLens feature work, with no corresponding source change. This means the checked-in .g.dart doesn't reflect what build_runner would currently produce from this branch's actual source, which will show up as codegen drift in CI (or mask a stale/local regen). Please run a clean dart run build_runner build --delete-conflicting-outputs on this branch and either commit the correct resulting diff or revert this file to match origin/main. - we can skip this i guess, im not sure why but i have tried generating multiple times and it was there all times, so its better to keep that way than removing

[claude]

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

---

Claude's response

• Read the trigger comment — author has decided to keep course_list_provider.g.dart as-is after multiple regen attempts reproduced the same diff
• Sanity-checked the claim against the repo
• Posted final wrap-up

Investigation

Confirmed packages/courses/lib/providers/course_list_provider.dart is byte-identical between origin/main and HEAD (git diff origin/main..HEAD -- .../course_list_provider.dart is empty), and pubspec.lock/pubspec.yaml are unchanged too — so this isn't a dependency bump or a source edit.

Given the source text feeding riverpod_generator's hash is unchanged and the locked package versions are unchanged, a hash flip like this most likely comes from a local toolchain difference (Dart SDK / riverpod_generator / build_runner patch version installed in your environment vs. whatever produced the hash currently on origin/main) rather than any actual code drift. That's consistent with you seeing the same regenerated hash every time you ran it locally — your environment is internally consistent, it just differs from whatever generated the baseline.

Practically, this hash is only used by Riverpod's codegen tooling for provider-identity/hot-reload bookkeping — it has no effect on runtime behavior or test outcomes. Given:

• it's non-functional,
• you've already verified it reproduces consistently across multiple local regens, and
• reverting it by hand would just cause it to drift again the next time anyone runs build_runner in CI or another dev's environment,

I agree it's reasonable to leave it as committed rather than hand-editing it back. Worth a quick check at some point on whether CI's build_runner/Dart SDK version matches what's pinned for local dev (if there's a version mismatch, it'll keep resurfacing on unrelated PRs) — but that's a tooling/CI hygiene item, not something blocking this PR.

Status

With this item acknowledged, there are no remaining blocking items from my earlier reviews — all CRITICAL/WARNING findings (quiz scoring bug, missing semantics, touch targets, localization, motion preference, Sentry integration) were fixed in later commits, and the LearnLens base URL / Drift migration / codegen-hash items are knowingly deferred by you. Nothing further to implement on my end for this request.

[syed-tp]

@claude review

course_list_provider.dart line 4 imports package:core/data/data.dart. This PR modified lesson_detail_provider.dart, which also lives in the same packages/courses build graph.

riverpod_generator doesn't just hash the source of the file itself — it hashes the full transitive closure of every Dart file that participates in the same code generation run. So when any file in that graph changes (even an indirect dependency), all downstream _$xxxHash() values are recomputed.

In short: course_list_provider.dart didn't change, but another file in the same build_runner input set did — and that's enough for the generator to produce new hashes. The diff is correct and expected; it reflects a clean regeneration on this branch's actual source.

Tell me the verdict, approved or not?

[claude]

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

---

Claude finished reviewing

• Read trigger comment — author's theory: riverpod_generator hashes the full transitive closure of the build graph, so course_list_provider.g.dart's hash change is "correct and expected" even though its source file is unchanged
• Verified the technical claim against how riverpod_generator actually computes _$xxxHash()
• Re-confirmed current diff state (git diff origin/main..HEAD) for both the source and generated file
• Posted final verdict

Investigation

Re-confirmed against origin/main (dab9788):

• packages/courses/lib/providers/course_list_provider.dart — zero diff, byte-identical.
• packages/courses/lib/providers/course_list_provider.g.dart — only the two hash literals changed:
  • _$courseListHash(): 5c4261d... → 6741d1f...
  • _$courseSearchHash(): 754041d... → 390cf82...

On the theory itself: riverpod_generator's _$xxxHash() is not a hash of the transitive closure of the build graph — it's a hash of just the annotated provider function/class's own source text (used purely so Riverpod's hot-reload machinery can tell "did this specific provider's implementation change" and selectively reset state). If it hashed the whole dependency graph, changing any file anywhere in a large app would flip every provider's hash on every build — which would defeat the entire point of the mechanism (fine-grained, per-provider hot-reload invalidation). So a change in lesson_detail_provider.dart (or any other file in the package) changing course_list_provider.dart's own hash isn't how this is designed to work — I wasn't able to fetch the upstream source in this sandbox to quote the exact implementation line, so treat this as high-confidence based on documented Riverpod hot-reload behavior rather than a verified source read, but it lines up with course_list_provider.dart being byte-for-byte unchanged: same generator version + identical source text should be a deterministic function, i.e. an identical hash.

The much more mundane explanation — consistent with what you reported earlier ("tried generating multiple times and it was there all times") — is a local toolchain difference: a different riverpod_generator/analyzer/Dart SDK patch version in your environment vs. whatever produced the hash currently checked in on origin/main. That's not a code defect in this PR, but the "transitive closure" explanation isn't the right mechanism for it.

That said, this doesn't change the practical bottom line from the last review: this hash has zero runtime effect (it's only consulted by Riverpod's dev-time hot-reload bookkeeping), it's the only remaining item on this PR, and you've already made the call to keep it as-is rather than risk re-introducing drift by hand-editing it back. That's a reasonable call.

Verdict

APPROVED — no remaining blocking issues. All prior CRITICAL items (quiz scoring, missing semantics, touch targets) and WARNING items (localization, motion preference, Sentry integration) were fixed in later commits. The LearnLens base URL and missing Drift migration are knowingly deferred by you. The course_list_provider.g.dart hash is non-functional codegen noise — worth a follow-up check on whether CI's pinned build_runner/riverpod_generator/Dart SDK versions match your local dev setup so it stops resurfacing on unrelated PRs, but not a blocker for this one.
(branch)