fix: correct NAT question parsing and cache custom exam courses

[syed-tp]

• Updated QuestionDto to parse verbose string types like "NAT, Numerical Answer Type".
• This ensures numerical questions properly render as text inputs instead of options.
• Added keepAlive to CustomExamCourses provider to prevent redundant network fetches.

[gemini-code-assist]

Caution

The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased.

[claude]

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

---

Todo List

• Read AGENTS.md and core docs (ai_context.md, architecture.md)
• Fetch PR diff and review changed files
• Check package boundary / import rules (none applicable — no new imports)
• Check generated file consistency (.g.dart changes)
• Post structured review summary

---

PR Review Summary

✅ What's good

• The QuestionDto.fromJson type switch (packages/core/lib/data/models/question_dto.dart:130-145) correctly falls back to the existing single-letter codes (R/C/S/N/E) before trying the verbose-string guards, so no regression for the legacy format.
• custom_exam_courses_provider.g.dart was regenerated consistently with the source change (AutoDisposeAsyncNotifierProvider → AsyncNotifierProvider, AutoDisposeAsyncNotifier → AsyncNotifier) — that's exactly what @Riverpod(keepAlive: true) should produce.
• Both fixes are bug fixes / caching corrections, so no OpenSpec change is required here.
• No SDK boundary, design-system, or accessibility rules are touched by this diff.

🔴 CRITICAL — Must fix before merge

1. Unexplained generated-file change: packages/exams/lib/providers/custom_exam_config_provider.g.dart changes _$customExamSelectionHash() (line 174), but its source file packages/exams/lib/providers/custom_exam_config_provider.dart has zero changes in this PR (verified via git diff origin/main...HEAD). Per the generated-files rule, a .g.dart diff with no corresponding source change usually indicates either a hand-edit or a stray/inconsistent build_runner run (e.g. a different generator/analyzer version than CI uses). Please re-run dart run build_runner build --delete-conflicting-outputs from a clean state and confirm whether this file actually needs to change; if it doesn't, drop it from the PR.

🟡 WARNING — Should fix

1. keepAlive: true on CustomExamCourses (packages/exams/lib/providers/custom_exam_courses_provider.dart:7): this provider holds a remote list of courses, not app-lifetime state like Auth/AppDatabase. There is no invalidation path anywhere in the codebase (ref.invalidate(customExamCoursesProvider) doesn't appear, nor any pull-to-refresh in custom_exam_course_selection_screen.dart), so once fetched the list is cached for the entire app process lifetime and will never reflect course changes (e.g. a course's allow_custom_test flag toggling) until the app is killed and restarted. Consider either a shorter-lived cache (e.g. ref.keepAlive() with a timer/dispose-after-idle in the notifier) or exposing a manual refresh action, rather than a blanket keepAlive: true.
2. No regression test for the new type-parsing branches: the new NAT/MCQ/MSQ verbose-string handling in question_dto.dart:136-143 has no accompanying test in packages/core/test/data/models/question_dto_test.dart, even though this is exactly the kind of parsing bug that regresses silently. Given the PR is explicitly fixing a parsing bug, a couple of cases (e.g. type: 'NAT, Numerical Answer Type' → numerical, 'MCQ, Multiple Choice Type' → singleSelect, 'MSQ, Multiple Select Type' → multipleSelect) would guard against future regressions.

🔵 SUGGESTION — Nice to have

1. The three new guarded cases in the switch (question_dto.dart:136-143) could be simplified — since 'N', 'R', 'C' already short-circuit the single-letter forms above, the verbose matching only needs to check contains(...) (e.g. contains('Numerical')) rather than also checking startsWith('NAT')/startsWith('MCQ')/startsWith('MSQ'), unless there's a known format where the prefix appears without the descriptive word. Not blocking, just a minor readability simplification if the prefix check is redundant.

Verdict

CHANGES REQUESTED — please confirm/resolve the unexplained custom_exam_config_provider.g.dart hash change (CRITICAL) before merge; the keepAlive staleness concern and missing test coverage are worth addressing too but are not hard blockers.

[syed-tp]

@claude review

[claude]

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

---

Todo List

• Read AGENTS.md and core docs (ai_context.md, architecture.md)
• Fetch PR diff and review changed files
• Check package boundary / import rules (none applicable — no cross-package imports)
• Check generated file consistency (.g.dart changes)
• Post structured review summary

---

PR Review Summary

✅ What's good

• QuestionDto.fromJson's type switch (packages/core/lib/data/models/question_dto.dart:130-144) still checks the legacy single-letter codes (R/C/S/N/E) before the new verbose-string guards, so there's no regression for the old format, and it's now backed by a real test (packages/core/test/data/models/question_dto_test.dart:26-44) covering NAT/MCQ/MSQ — this addresses the missing-test-coverage warning from the earlier review.
• The latest commit (6137959) replaces the blanket @Riverpod(keepAlive: true) with a plain @riverpod + manual ref.keepAlive() / 5-minute Timer in custom_exam_courses_provider.dart:11-16 — a good-faith attempt to address the "cached forever, no invalidation path" warning from the prior review.
• Both changes are bug fixes / caching corrections; no OpenSpec change is required.

🔴 CRITICAL — Must fix before merge

1. custom_exam_courses_provider.g.dart is now stale relative to its source, and this silently breaks the intended fix. The source annotation was reverted from @Riverpod(keepAlive: true) to plain @riverpod in commit 6137959 (packages/exams/lib/providers/custom_exam_courses_provider.dart:8), which should regenerate the provider as AutoDisposeAsyncNotifierProvider / AutoDisposeAsyncNotifier. However custom_exam_courses_provider.g.dart:14-24 still declares AsyncNotifierProvider<...> / typedef _$CustomExamCourses = AsyncNotifier<...> — the non-autoDispose variant left over from the previous commit. Since the underlying provider is not actually autoDispose, ref.keepAlive()/link.close() in the build() method (lines 12-15) has no real effect — the provider stays alive for the app's entire lifetime regardless of the 5-minute timer. The caching/staleness bug this PR set out to fix is not actually fixed — please run dart run build_runner build --delete-conflicting-outputs from a clean state and commit the regenerated file.
2. Unexplained, still-unresolved generated-file change: packages/exams/lib/providers/custom_exam_config_provider.g.dart:174 changes _$customExamSelectionHash(), but its source file custom_exam_config_provider.dart has zero diff anywhere in this PR (verified via git log -p origin/main..HEAD -- packages/exams/lib/providers/custom_exam_config_provider.dart, which is empty). This was flagged in the previous automated review and is still present after the latest commit. Please confirm whether this file needs to change at all; if a clean build_runner run doesn't reproduce this diff, drop it from the PR.

🟡 WARNING — Should fix

1. None outstanding beyond the CRITICAL items above — the missing-test-coverage warning from the previous review has been resolved, and the keepAlive: true design (in intent) is now a reasonable, time-bounded cache rather than a permanent one, pending the generated-file fix in CRITICAL feat(ui): implement LMS core primitives #1.

🔵 SUGGESTION — Nice to have

1. In question_dto.dart:136-143, since 'N', 'R', 'C' already short-circuit the single-letter forms above, the verbose matching could just check contains(...) (e.g. contains('Numerical')) rather than also checking contains('NAT')/contains('MCQ')/contains('MSQ'), unless there's a known format where the code appears without the descriptive word. Not blocking.

Verdict

CHANGES REQUESTED — the generated .g.dart files need to be regenerated from a clean build_runner run so custom_exam_courses_provider.g.dart actually reflects the @riverpod (autoDispose) annotation, and the unrelated custom_exam_config_provider.g.dart hash change needs to be explained or dropped.
· branch: fix/course_parsing

[syed-tp]

@claude review

[claude]

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

---

Todo List

• Read AGENTS.md and core docs (ai_context.md, architecture.md)
• Fetch latest PR diff (post fix commits) and review changed files
• Check generated file consistency (.g.dart changes) — verify the previously flagged issues were fixed
• Check package boundary / import rules (none applicable — no new package imports)
• Post structured review summary

---

PR Review Summary

Re-reviewed after the latest commits (6137959, 9dee2a0, 0b27951). Both CRITICAL issues from the previous review are now resolved.

✅ What's good

• custom_exam_courses_provider.g.dart is now consistent with its source. The provider annotation is plain @riverpod (custom_exam_courses_provider.dart:8), and the regenerated file correctly declares AutoDisposeAsyncNotifierProvider<CustomExamCourses, List<CourseDto>> / typedef _$CustomExamCourses = AutoDisposeAsyncNotifier<...> (custom_exam_courses_provider.g.dart:13-24). The manual ref.keepAlive() + 5-minute Timer in build() (custom_exam_courses_provider.dart:12-16) now actually works as intended: autoDispose is real, and the timer just extends the provider's life by 5 minutes past the point it would otherwise dispose, avoiding the "cached forever" problem while still cutting down on redundant fetches.
• The unrelated custom_exam_config_provider.g.dart change has been dropped (commit 0b27951) — confirmed via git diff origin/main...HEAD --stat, only the 4 originally-intended files are touched now.
• QuestionDto.fromJson's type switch (question_dto.dart:130-145) still checks the legacy single-letter codes (R/C/S/N/E) before the new verbose-string guards, so there's no regression for the old format. Traced through the guard ordering for NAT/MCQ/MSQ inputs — no overlap issues (e.g. 'MSQ, Multiple Select Type' doesn't accidentally match the MCQ guard since it contains neither 'Multiple Choice' nor 'MCQ').
• Test coverage added (question_dto_test.dart:26-45) covering all three new verbose-string branches (NAT, MCQ, MSQ) — addresses the missing-test-coverage warning from the first review.
• Both changes are bug fixes / caching corrections; no OpenSpec change required. No package-boundary, design-system, or accessibility rules are touched by this diff (the only new import is dart:async, which is fine).

🔴 CRITICAL — Must fix before merge

None outstanding.

🟡 WARNING — Should fix

None outstanding.

🔵 SUGGESTION — Nice to have

1. In question_dto.dart:136-143, since 'N', 'R', 'C' already short-circuit the single-letter forms above, the verbose matching could just check contains('Numerical') / contains('Multiple Choice') / contains('Multiple Select') rather than also checking contains('NAT')/contains('MCQ')/contains('MSQ'), unless there's a known backend format where the short code appears without the descriptive word. Not blocking — carried over from the earlier review since it's still true, just a minor readability call.

Verdict

APPROVED — both CRITICAL blockers from the earlier review (stale/hand-edited .g.dart, unexplained unrelated generated-file diff) are resolved, and the missing test coverage has been addressed. Only a non-blocking style suggestion remains.

---

Note: I don't have dart/flutter available in this environment, so I verified the build_runner output structurally (annotation → expected generated types) rather than by re-running codegen. The structure lines up correctly with @riverpod (autoDispose) semantics.
· branch: fix/course_parsing