feat(testpress): Implement Daily Questions feature by pugal66 · Pull Request #271 · testpress/cortex

pugal66 · 2026-09-07T06:40:53Z

Add institute-gated drawer navigation and QOTD routing.
Add progress overview and interactive multi-question quiz flow.
Support instant solution reviews and daily completion tracking.
Parse polymorphic subject, difficulty, and question-type payloads.
Refine quiz metadata badges, header navigation, and MathJax rendering.

- Add institute-gated drawer navigation and QOTD routing. - Add progress overview and interactive multi-question quiz flow. - Support instant solution reviews and daily completion tracking. - Parse polymorphic subject, difficulty, and question-type payloads. - Refine quiz metadata badges, header navigation, and MathJax rendering.

claude · 2026-09-07T06:41:13Z

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

Todo list

PR Review Summary

✅ What's good

Proper OpenSpec change (openspec/changes/add-question-of-the-day/) with proposal, design, tasks, and spec deltas for both the new daily-questions capability and the modified lms-home-paid-active drawer capability — this is exactly the pattern required for a new feature.
Clean SDK layering: DataSource (abstract) → HttpDataSource + MockDataSource (both implement getQotdQuestions/submitQotdAttempt/getQotdSummary) → QotdRepository → @riverpod providers (qotd_provider.dart, qotd_quiz_controller.dart). No boundary violations found (testpress → core/courses/exams only).
/qotd is properly registered as a GoRoute in global_routes.dart, gated behind qotdEnabled in the drawer — no Navigator.push(MaterialPageRoute(...)) usage.
No Material/Cupertino widgets, no Icons.*/CupertinoIcons.* (LucideIcons used throughout), no static token imports — all colors/spacing/typography/motion read from Design.of(context).
Motion correctly uses MotionPreferences.duration/curve with design.motion.* tokens (no hardcoded Duration/Curves) for both progress-bar animations and option-card transitions.
QotdDto.fromJson's polymorphic subject/difficulty/type extraction is defensive and well-documented, matching the design doc's stated rationale.
The DonutChart dedup (new shared widget in core, renamed local percentage-based variant to SubjectDonutChart in exams) is a nice bit of reuse instead of two similarly-named but incompatible widgets.

🔴 CRITICAL — Must fix before merge

Missing AppSemantics.progressValue() on all three progress/completion indicators: QotdCompletionGauge (packages/testpress/lib/screens/dashboard/qotd/widgets/qotd_completion_gauge.dart:16), the attempted/total bar in qotd_overview_screen.dart:184, and the question-stepper bar in qotd_quiz_screen.dart:138. This pattern is used consistently elsewhere in the repo (hero_donut_card.dart, metrics_grid.dart, chapter_content_item.dart) — screen readers currently get no value/label for any of QOTD's progress UI.
.g.dart files changed with no corresponding source diff: bootstrap_provider.g.dart, initialization_provider.g.dart, and live_stream_provider.g.dart all have new _$...Hash() values, but git diff shows their source .dart files are byte-identical to origin/main. This is the exact signature the repo's "no hand-edited generated files" rule flags — please confirm this reproduces from a clean build_runner build (likely caused by a transitive riverpod_generator/analyzer bump from adding flutter_svg) rather than being stray/manual edits, and consider reverting if not reproducible.

🟡 WARNING — Should fix

Hardcoded English strings, not localized: _formatQuestionType() in qotd_quiz_screen.dart:51-85 returns literal 'Single Correct' / 'Multiple Correct', displayed directly in the metadata pill — no matching ARB key exists. Similarly, the submit-failure toast in qotd_quiz_controller.dart:128 ('Failed to submit answer: $e') is hardcoded and also leaks the raw exception to the user.
QotdRepository skips the caching/Drift layer entirely (packages/core/lib/data/repositories/qotd_repository.dart:7) — it's a pure passthrough to DataSource, unlike LeaderboardRepository/DoubtRepository/DashboardRepository which all sync into the local DB. If QOTD is intentionally online-only (reasonable for daily/time-sensitive content), that's fine — just worth an explicit call-out since it deviates from the documented repository pattern.

🔵 SUGGESTION — Nice to have

No unit/widget tests were added for the new polymorphic QotdDto.fromJson parsing logic or QotdQuizController state transitions, despite the parsing being explicitly flagged in the design doc as the highest-risk part of this change ("Varying API Payload Structures").
_buildActionButtons in qotd_quiz_screen.dart hand-rolls Container + GestureDetector + AppSemantics.button for the Previous/Check/Next buttons instead of reusing AppButton (which already supports backgroundColor/foregroundColor/leading/loading state and wraps semantics internally) — not a violation since no Material/Cupertino widgets are used, but it duplicates logic the design system already provides.
AppSemantics.button(label: l10n.qotdOptionLabel, ...) gives every option card the same generic "Option" accessible label (qotd_quiz_screen.dart:346) — screen reader users can't distinguish options by announcement alone; consider including the option's plain-text content or index in the label.

Verdict

CHANGES REQUESTED — the two CRITICAL items (missing progress semantics, unexplained generated-file hash churn) should be resolved before merge; the WARNING items are worth addressing but not blocking.
· Branch: feat/qotd

pugal66 · 2026-09-07T09:00:17Z

@claude review

claude · 2026-09-07T09:00:32Z

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

Todo list

Fetch latest PR diff (origin/main...HEAD) and check commits since last review
Read AGENTS.md, ai_context.md, architecture.md
Verify fixes claimed in review comments (semantics, localization, unrelated generated files)
Re-check remaining open items
Post updated review summary

PR Review Summary

Re-reviewed after commit 3928ab5 ("Address PR review feedback"). All items from my previous pass have been verified fixed in the code (not just claimed in comments) — checked line-by-line diffs, not just the PR description.

✅ What's good

All three progress indicators now correctly wrapped with AppSemantics.progressValue(): qotd_completion_gauge.dart:15, qotd_overview_screen.dart:184, qotd_quiz_screen.dart:104.
The three unrelated .g.dart hash changes (bootstrap_provider.g.dart, initialization_provider.g.dart, live_stream_provider.g.dart) are gone — git diff origin/main...HEAD now shows zero diff for these files. Confirmed clean.
Hardcoded question-type strings replaced with a proper QotdQuestionType enum (packages/core/lib/data/models/qotd_dto.dart) parsed once in the DTO layer, exposed via l10n.qotdSingleCorrect / l10n.qotdMultipleCorrect — a nicer fix than just swapping literals for l10n keys, since the parsing logic now lives with the rest of the polymorphic-payload handling and is unit tested (qotd_dto_test.dart).
Submit-failure toast no longer leaks the raw exception ($e) to the user; now uses l10n.qotdErrorSubmitAnswer (qotd_quiz_controller.dart:128).
The "online-only repository" design deviation is now explicitly documented instead of just being an inline PR comment: added as Decision feat(core): implement semantic-first typography foundation and gallery #5 + a Risk/Trade-off entry in design.md, and a new Online-Only Repository Operations requirement/scenario in specs/daily-questions/spec.md. This is the right way to close out an architecture-deviation WARNING — spec-first, not just a comment.
Option-card semantic labels now include the option letter ('${l10n.qotdOptionLabel} A', etc. — qotd_quiz_screen.dart:320), addressing the earlier ambiguous "Option" announcement.
New unit tests added: qotd_dto_test.dart (222 lines, covers the polymorphic parsing) and qotd_quiz_controller_test.dart (223 lines, covers controller state transitions) — closes the test-coverage suggestion from the first pass.

🔴 CRITICAL — Must fix before merge

None remaining. Both CRITICAL items from the previous review (missing progress semantics, stray generated-file churn) are resolved.

🟡 WARNING — Should fix

None remaining. Both WARNING items (hardcoded strings/exception leak, undocumented offline-cache deviation) are resolved.

🔵 SUGGESTION — Nice to have

_buildActionButtons in qotd_quiz_screen.dart:546 still hand-rolls Container + GestureDetector + AppSemantics.button for Previous/Check/Next instead of reusing AppButton. Not a violation (no Material/Cupertino widgets involved), just duplicated logic — still valid as a nice-to-have.
Note for reviewers: I was unable to run flutter analyze / flutter test in this environment (no flutter/dart tooling available), so this review is based on static code reading only — worth confirming CI is green before merge.

Verdict

APPROVED — all CRITICAL and WARNING items from the prior review are verified fixed in code. Only a minor, non-blocking suggestion remains.
· Branch: feat/qotd

Add skeleton loader on quiz exit to avoid stale metrics flash. Add option letter index to accessible button labels and subject pill fallback. Add unit tests for polymorphic DTO parsing and quiz controller state transitions.

- Avoid redundant network calls on quiz exit when reviewing solutions. - Track submission state to return to overview instantly without reloading.

pugal66 · 2026-09-07T09:25:22Z

@claude review

claude · 2026-09-07T09:25:39Z

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

Todo list

Fetch latest PR diff and check commits since last review
Read AGENTS.md, ai_context.md, architecture.md
Review new commit 35ad1ca (quiz exit transitions)
Post updated review summary

PR Review Summary

Re-reviewed after the new commit 35ad1ca ("Optimize quiz exit transitions"), which is the only change since my previous APPROVED pass on 1bf36f4. All prior CRITICAL/WARNING items remain resolved; this commit is a self-contained perf optimization.

✅ What's good

Correctly scoped optimization: QotdQuizState.hasSubmittedNewAnswer (qotd_quiz_controller.dart:15) is only flipped true inside the success branch of submitCurrentAnswer (qotd_quiz_controller.dart:127), so purely-navigational exits (reviewing past solutions, browsing without answering) correctly skip the ref.invalidate(qotdProvider) / ref.invalidate(qotdSummaryProvider) calls in qotd_screen.dart:117-121, while any real submission still triggers a refresh.
Both exit paths in qotd_quiz_screen.dart (the header back button at line 99 and the "Finish" onNext at line 263) were updated consistently to pass state.hasSubmittedNewAnswer — no stale call site left using the old VoidCallback signature.
Design/spec docs were updated in lockstep with the code: design.md gets a new Decision feat(drawer): implement overlay navigation drawer #6 and specs/daily-questions/spec.md gets a matching "Solution Review Navigation" scenario — keeps OpenSpec in sync rather than leaving the deviation undocumented.
Test coverage extended (qotd_quiz_controller_test.dart) to assert hasSubmittedNewAnswer starts false and flips to true after a successful submit.
No SDK boundary, design-system, accessibility, or generated-file issues introduced — the diff is confined to controller/screen wiring plus docs/tests.

🔴 CRITICAL — Must fix before merge

None.

🟡 WARNING — Should fix

None.

🔵 SUGGESTION — Nice to have

hasSubmittedNewAnswer is a single flag for the whole session rather than per-question — this matches the stated intent ("any new submission should refresh overview counts on exit"), just noting it so a future per-question-granularity requirement doesn't get confused with a bug.
As before, I could not run flutter analyze / flutter test in this environment (no flutter/dart tooling available) — this review is based on static code reading; worth confirming CI is green before merge.

Verdict

APPROVED — the new commit is a clean, well-documented optimization with no regressions to previously-fixed items.
· Branch: feat/qotd