feat(exams): improve offline exam submission flow and pre-screen UX

[syed-tp]

Why

• Offline submissions lacked toast feedback, and synced exams vanished silently from the list.
• Mismatched button heights on the pre-screen made the UI look inconsistent.
• Custom container status badges duplicated the core AppBadge widget.
• Direct connectivity checks in the exams UI duplicated logic from the sync manager, creating a tighter dependency coupling.
• The new sync state machine branching logic required unit test coverage to guarantee submission durability.

What

• Show a submission toast when submitting an offline exam without internet.
• Retain synced downloads with a SYNCED status and reactive badge list indicators.
• Added isOfflineOnly flag to hide online attempts, retakes, and mode switchers when loading from the offline list.
• Refactored pre-screen action buttons to use AppButton core primitives.
• Extended AppBadge to support a custom leading widget (such as a loader) and converted custom badge containers.
• Extracted and reused a shared hasInternetConnection helper in core, removing direct connectivity_plus dependencies from the exams package.
• Added unit tests verifying the success, transient failure, and permanent failure status transition paths of OfflineExamSyncService.

[claude]

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

---

PR Review Summary

✅ What's good

• OpenSpec change (openspec/changes/offline-exam-submit-ux/) is present with proposal.md, design.md, spec.md, and tasks.md — good compliance with the OpenSpec-required workflow for a new feature.
• exam_prescreen_action_button.dart was nicely simplified by replacing a hand-rolled AppSemantics.button + Container with AppButton.primary / AppButton.secondary, which already wraps AppSemantics.button internally — good primitive reuse and less code to maintain (packages/exams/lib/widgets/exam_prescreen_action_button.dart:37-63).
• All new user-facing strings (offlineExamSavedToast, inProgressStatus, syncingStatus, pendingSyncStatus, submittedStatus) are correctly added to all four ARB locales (en/ar/ml/ta) and the generated l10n classes are regenerated consistently.
• Icon usage stays within LucideIcons, and colors/spacing are sourced from Design.of(context) — no static token imports or Material/Cupertino widgets introduced.

🔴 CRITICAL — Must fix before merge

1. DB migration was skipped, contradicting the PR's own spec — will crash for existing installs. offline_exam_tables.dart adds a new syncedAt column to the pre-existing OfflineExamDownloadsTable (this table already ships in production under schemaVersion == 1, see origin/main version of the file). app_database.dart:59 still has schemaVersion => 1, and onUpgrade (app_database.dart:66-78) only creates missing tables — it never adds missing columns to existing tables. Any user who already has a downloaded/synced offline exam on disk will hit a no such column: synced_at SQLite error the first time this code path runs after the update.
   The PR's own spec.md explicitly requires this: "The database schema version SHALL be bumped to accommodate the migration" and describes an "Upgrade from schema version 1" scenario where onUpgrade adds the column without data loss (openspec/changes/offline-exam-submit-ux/specs/offline-exam-submit-ux/spec.md:60-70). tasks.md:4-5 shows this was explicitly skipped ("skipped per user — no migration needed"), so the shipped code diverges from its own written spec.
   → Bump schemaVersion to 2 and add an onUpgrade branch that calls m.addColumn(offlineExamDownloadsTable, offlineExamDownloadsTable.syncedAt) when from < 2.
   Fix this →

2. Custom status badge duplicates the existing AppBadge core primitive. _ExamCardHeader in offline_exams_list_screen.dart:199-291 hand-builds a status badge (Container + BoxDecoration + Icon + AppText.labelSmall) with a switch over 5 statuses, instead of using AppBadge (packages/core/lib/widgets/app_badge.dart), which already accepts label, icon, backgroundColor/foregroundColor and is purpose-built for exactly this. Per the design-system rule, custom-rolled equivalents of existing core primitives should be flagged — this expands a pre-existing single-status container into a full multi-status implementation rather than consolidating onto AppBadge.
   Fix this →

🟡 WARNING — Should fix

1. No test coverage for the new sync state machine. OfflineExamSyncService.syncPendingExams() (packages/core/lib/data/services/offline_exam_sync_service.dart) now has meaningfully more branching (SYNCING → SYNCED / PENDING_SYNC retry / permanent-failure delete), but no test file was added or modified in this PR (git diff origin/main...HEAD --stat -- '*test*' shows zero test changes). Given this directly affects submission durability, it should have unit coverage for at least the three status-transition paths.
2. OpenSpec change doesn't cover the pre-screen changes shipped in this PR. spec.md only documents the toast, badge, retained-row, and l10n behavior. It does not mention isOfflineOnly hiding retake buttons/history tables in exam_prescreen.dart, or the AppButton height-alignment refactor in exam_prescreen_action_button.dart — both called out in the PR description as intentional feature work. Consider adding these as requirements to the spec for traceability.
3. Connectivity-check logic is duplicated instead of reused. test_detail_screen.dart:752-765 and :788-800 inline a Connectivity().checkConnectivity() + mobile/wifi/ethernet check, duplicating the same pattern already implemented in packages/core/lib/data/services/sync_manager.dart:23-27. Consider extracting a shared hasInternetConnection() helper in core so both call sites (and any future ones) stay in sync, and so test_detail_screen.dart doesn't need its own connectivity know-how at the widget layer. This logic is also duplicated verbatim between the pause and submit confirmation handlers within the same file — worth factoring into one local method.

🔵 SUGGESTION — Nice to have

1. connectivity_plus was added to packages/exams/pubspec.yaml — it's already a dependency of packages/core, so this isn't a net-new third-party package, but the PR description's "no new dependencies required" framing (per proposal.md) is only true at the monorepo level; worth a one-line note in the PR body for reviewers scanning pubspec.yaml diffs.

Verdict

CHANGES REQUESTED

[syed-tp]

@claude review, the db migration was skipped intentionally as this app is not released yet

[claude]

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

---

PR Review Summary (updated)

Thanks for the context on the DB migration — since the app hasn't shipped to production yet, there's no installed base with the old schema, so onUpgrade never actually runs for real users today. I'm withdrawing that item as a blocker. Re-verified the rest of the diff against origin/main...HEAD (still just the single commit f12ca8e, unchanged since the last review).

✅ What's good

• OpenSpec change (openspec/changes/offline-exam-submit-ux/) present with proposal.md, design.md, spec.md, tasks.md — compliant with the required SDD workflow for new feature work.
• exam_prescreen_action_button.dart simplified by replacing hand-rolled AppSemantics.button + Container with AppButton.primary / AppButton.secondary (packages/exams/lib/widgets/exam_prescreen_action_button.dart:37-63) — good primitive reuse.
• All new strings (offlineExamSavedToast, inProgressStatus, syncingStatus, pendingSyncStatus, submittedStatus) are present in all four ARB locales (en/ar/ml/ta) with consistently regenerated l10n classes.
• Only LucideIcons used, colors/spacing sourced via Design.of(context) — no static token imports or Material/Cupertino widgets introduced.
• Connectivity helper extraction reduces coupling between exams and connectivity_plus in principle (see warning below on where it's still inlined).

🔴 CRITICAL — Must fix before merge

None outstanding. (DB migration item withdrawn per your note — app is pre-release, no installed base on schema v1 to migrate. Worth revisiting once the app ships, since onUpgrade today only creates missing tables and won't add missing columns to existing ones — see packages/core/lib/data/db/app_database.dart:66-78.)

🟡 WARNING — Should fix

1. Custom status badge duplicates the existing AppBadge core primitive. _ExamCardHeader in offline_exams_list_screen.dart:194-291 hand-builds a status badge (Container + BoxDecoration + Icon + AppText.labelSmall) with a switch over 5 statuses, instead of using AppBadge (packages/core/lib/widgets/app_badge.dart), which was extended in this very PR to support a custom leading widget for exactly this use case. Worth consolidating onto AppBadge rather than keeping a parallel bespoke implementation.
   Fix this →
2. PR description says unit tests were added for the sync state machine, but no test files are in the diff. git diff origin/main...HEAD --stat shows zero changes under any test/ path — OfflineExamSyncService.syncPendingExams() (packages/core/lib/data/services/offline_exam_sync_service.dart) picked up meaningfully more branching (SYNCING → SYNCED / PENDING_SYNC retry / permanent-failure delete) with no accompanying coverage. Given this directly affects submission durability, and the PR body explicitly claims this was done, please add the tests (or update the description if they'll follow in a separate PR).
   Fix this →
3. OpenSpec change doesn't cover the pre-screen changes shipped in this PR. spec.md documents the toast, badge, retained-row, and l10n behavior, but not isOfflineOnly hiding retake buttons/history in exam_prescreen.dart, or the AppButton height-alignment refactor in exam_prescreen_action_button.dart — both are called out as intentional in the PR description. Consider adding these as requirements for traceability.
4. Connectivity-check logic still duplicated at two call sites within the same file. test_detail_screen.dart:752-765 and :788-800 each inline a Connectivity().checkConnectivity() + mobile/wifi/ethernet check — identical logic duplicated verbatim between the pause and submit confirmation handlers. Since a shared hasInternetConnection() helper was added to core in this PR specifically to remove this duplication, consider having both call sites use it (or factor one local method) rather than leaving the inline duplicate.

🔵 SUGGESTION — Nice to have

1. connectivity_plus was added to packages/exams/pubspec.yaml. It's already a transitive dependency via packages/core, so this isn't a new third-party package at the monorepo level — just worth a one-line note in the PR body since proposal.md frames it as "no new dependencies required."

Verdict

CHANGES REQUESTED (non-blocking: the two WARNING items above — badge consolidation and the missing/undelivered test coverage — are the most worth addressing before merge; the DB migration concern is resolved by the pre-release context).
· Branch: ref/offline-exam-ux

[syed-tp]

@claude review

[claude]

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

---

PR Review Summary (re-reviewed after latest commits)

Re-verified the full diff (origin/main...HEAD, now 3 commits: f12ca8e, 9a74563, b1008b1). The two newest commits address every item raised in the previous review round — see below — but the very last commit (b1008b1) introduces a new regression.

✅ What's good

• All previously-flagged WARNINGs are now resolved:
  • _ExamCardHeader (packages/exams/lib/screens/offline_exams_list_screen.dart:206-253) now builds every status badge via the core AppBadge primitive (including a leading spinner for SYNCING), replacing the old hand-rolled Container/BoxDecoration switch.
  • Unit tests were added for OfflineExamSyncService.syncPendingExams() (packages/core/test/data/services/offline_exam_sync_service_test.dart), covering all three transition paths: success (PENDING_SYNC → SYNCING → SYNCED, syncedAt populated), transient failure (reverts to PENDING_SYNC), and permanent failure (row deleted on 4xx).
  • The duplicated connectivity-check logic in test_detail_screen.dart is now consolidated into a single _showOfflineToastIfNeeded() helper that calls the new shared hasInternetConnection() (packages/core/lib/network/network_utils.dart), and connectivity_plus was removed from packages/exams/pubspec.yaml entirely — good boundary cleanup.
• OpenSpec design.md was updated with an explicit migration plan (D5) and risk log, and spec.md/tasks.md were extended to match the shipped behavior.
• Localization: openExamAction was added correctly across all 4 locales (en/ar/ml/ta).

🔴 CRITICAL — Must fix before merge

1. "Open" now lets users re-enter and resubmit an already-synced (server-confirmed) exam, risking duplicate submissions and silent answer edits after submission. Commit b1008b1 removed the if (exam.status != 'SYNCED') guard around the action button in _ExamCardActions (packages/exams/lib/screens/offline_exams_list_screen.dart:394-444), so "Open" is now shown — and enabled — for SYNCED rows too. Tapping it routes to ExamPrescreen(isOfflineOnly: true), which renders OfflineExamActionButton unconditionally. That widget (packages/exams/lib/widgets/offline_exam_action_button.dart:79-137) has no branch for status == 'SYNCED' — it falls through the same path as a fresh download and renders "Start Offline Exam", wired to onStartOfflineAttempt → OfflineExamRepository._startOfflineExam(). That method (packages/exams/lib/repositories/offline_exam_repository.dart:194-208) sets the row back to IN_PROGRESS, reloads the previously-saved (already-submitted) answers, and restarts the countdown — letting the user edit answers and call endExam() again, which flips status back to PENDING_SYNC and causes OfflineExamSyncService to POST submitOfflineExamAnswers a second time for an exam the server already has a recorded submission for.
   This directly contradicts the PR's own design.md (D2 / Risks section): "the 'Submitted ✓' badge makes them clearly inert... For now the user can still manually delete them" — the explicit design intent was that SYNCED rows are inert, not re-enterable.
   → Either restore the status != 'SYNCED' guard so no start action is offered once synced, or add an explicit SYNCED branch in OfflineExamActionButton that renders a disabled/read-only state (or navigates to a results view) instead of falling through to "Start Offline Exam".
   Fix this →

🟡 WARNING — Should fix

1. Dead l10n string left behind. attendExamAction (present in all 4 ARB files and generated l10n classes) is no longer referenced anywhere in source after b1008b1 replaced its call site with openExamAction (packages/exams/lib/screens/offline_exams_list_screen.dart). Not harmful, but worth removing from the ARB files to avoid confusion, or confirm there's another call site I'm missing.
2. OpenSpec doesn't cover the new "Open for all statuses" behavior. spec.md's "Offline-only pre-screen navigation" requirement only describes hiding online actions/history; it doesn't document what "Open" should do for an already-SYNCED exam (which is exactly where the CRITICAL bug above originates from a spec-gap perspective). Worth adding an explicit scenario once the fix above lands.

🔵 SUGGESTION — Nice to have

1. connectivity_plus in packages/exams/pubspec.yaml was correctly removed in this round — nice cleanup, no action needed.

Verdict

CHANGES REQUESTED — the badge/test/connectivity-dedup fixes from the previous round are solid, but the new b1008b1 commit introduces a genuine correctness/data-integrity regression (re-submittable synced exams) that should block merge.
· Branch: ref/offline-exam-ux

[syed-tp]

@claude review

[claude]

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

---

PR Review Summary (re-reviewed after latest commit)

Re-verified against origin/main...HEAD (now 4 commits: f12ca8e, 9a74563, b1008b1, d23590f). The new commit d23590f directly addresses the data-integrity regression flagged in the previous round — but the fix introduces its own gap.

✅ What's good

• All prior WARNING items remain resolved (badge consolidation via AppBadge, sync-service unit tests, connectivity dedup via hasInternetConnection()).
• The previous CRITICAL ("Open" silently resubmitting a SYNCED exam) is no longer an accidental fallthrough — d23590f turns it into a deliberate retake flow (OfflineExamRepository._startOfflineExam, packages/exams/lib/repositories/offline_exam_repository.dart:200-224): it clears prior answers via the new AppDatabase.clearAnswersForDownload (packages/core/lib/data/db/app_database.dart:587-592) and resets startedAt/completedAt/elapsedSeconds/syncedAt before restarting the attempt.
• OpenSpec updated accordingly: design.md D2 now documents the retake rationale, and spec.md adds an explicit "Retaking a SYNCED exam" scenario — good traceability for the new behavior.
• Minor unrelated dead-import cleanups (bookmark_folders_sheet.dart, product_discount_sheet.dart, review_dialog_components.dart) are harmless.

🔴 CRITICAL — Must fix before merge

1. Offline retake bypasses the exam's own allowRetake/maxRetakes policy. Every other retake entry point in the app gates on this: test_detail_screen.dart:212-216 and :790-792, exam_prescreen.dart:199, quiz_result_view.dart:105, assessment_detail_screen.dart:570. But _ExamCardActions in offline_exams_list_screen.dart:394-444 operates on OfflineExamDownloadsTableData — a Drift row that has no retake-permission column at all — and unconditionally shows/enables "Open" for SYNCED rows, which routes straight into _startOfflineExam's new retake branch with no check against ExamDto.allowRetake or lesson.maxRetakes. Since offline mode has no connectivity to validate this server-side until the next sync, a student can retake an exam offline whose settings explicitly disallow retakes (or exceed the configured maxRetakes), and by the time it syncs, the original submitted answers are already gone (see feat(navigation-shell): implement unified routing architecture #2).
   Fix this →
2. Destructive action with no confirmation, mislabeled as "Open." The button that triggers the retake (packages/exams/lib/screens/offline_exams_list_screen.dart:428-441) is labeled l10n.openExamAction with a LucideIcons.externalLink icon — the same "view" affordance used for every other status — yet for a SYNCED row it silently deletes the previously submitted answers (clearAnswersForDownload) and resets syncedAt/completedAt before the user sees any exam content. There's no confirmation dialog warning that this discards the prior submission, and no read-only way to review what was actually submitted. This also contradicts the PR's own design.md D2 rationale that SYNCED rows exist to give users "a permanent audit trail."
   Fix this →

🟡 WARNING — Should fix

1. No test coverage for the new retake branch. _startOfflineExam's SYNCED branch (packages/exams/lib/repositories/offline_exam_repository.dart:200-224) mutates state (deletes answer rows, resets 5 metadata fields) but has no accompanying test, unlike the sync-service transitions covered this round. Given the PR's stated goal is durability/correctness of submission state transitions, this branch deserves the same treatment.
2. spec.md's new "Retaking a SYNCED exam" scenario doesn't mention allowRetake/maxRetakes — worth updating alongside the fix for CRITICAL feat(ui): implement LMS core primitives #1 so the spec and code stay in sync.

Verdict

CHANGES REQUESTED — the destructive-resubmission bug from the last round is fixed, but the fix ships an unguarded retake capability (no allowRetake/maxRetakes check, no confirmation before discarding a submitted attempt) that should be addressed before merge.
· Branch: ref/offline-exam-ux

[syed-tp]

@claude review,

the open nvigates to the exam detail screen won't start the exam, and what youre saying is not valid, if the retake is won't allowed the sync will fail, there is no issue with that

[claude]

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

---

Todo

• Read trigger comment and gather context
• Re-examine _ExamCardActions "Open" navigation flow in offline_exams_list_screen.dart
• Re-examine ExamPrescreen / OfflineExamActionButton to confirm whether it starts the exam directly
• Verify the retake/allowRetake claim against OfflineExamSyncService.syncPendingExams()
• Post updated review response

Re-checked both points against the current code on ref/offline-exam-ux.

On "Open won't start the exam"

Confirmed, and you're right that my earlier wording was imprecise. _ExamCardActions (packages/exams/lib/screens/offline_exams_list_screen.dart:441-443) navigates to /exams/test/${exam.contentId}?isOffline=true, which resolves to ExamPrescreen (packages/testpress/lib/navigation/routes/exams_routes.dart:200-227) — the detail/pre-screen page, not the player. Starting an attempt requires an explicit second tap on the OfflineExamActionButton rendered inside that screen (packages/exams/lib/screens/exam_prescreen.dart:213-227), which is clearly labeled "Start Offline Exam" / "Resume Offline Exam" (packages/exams/lib/widgets/offline_exam_action_button.dart:123-137). So there's no silent/accidental start — two deliberate taps are required. I'm dropping that framing.

On "if retake isn't allowed, sync will fail — no issue"

You're correct that the server is the source of truth and will reject a disallowed retake at sync time — the app doesn't need to duplicate allowRetake/maxRetakes policy checks client-side for correctness purposes. Withdrawing that part of the earlier CRITICAL.

There is one residual concern worth flagging (downgrading to WARNING, not blocking): trace through what actually happens when that rejection occurs.

1. Tapping "Start Offline Exam" on a SYNCED row immediately calls clearAnswersForDownload and wipes the previously-submitted answers before the user has done anything in the retake (packages/exams/lib/repositories/offline_exam_repository.dart:200-215) — no confirmation dialog first.
2. If the retake later fails to sync with a 4xx (e.g. the server rejects it because retakes aren't allowed), OfflineExamSyncService.syncPendingExams() treats this as a permanent failure and calls _db.deleteDownload(download.id) (packages/core/lib/data/services/offline_exam_sync_service.dart:113-117), which cascades and deletes the download row entirely (packages/core/lib/data/db/app_database.dart:577-586).
3. Net effect: the original submission's local audit trail was already cleared in step 1, and the retake's row (including its answers) is silently removed in step 2, with no toast or UI indication — the exam just disappears from the offline list. The user is left with no local record that they ever submitted, even though the server presumably still has the original synced submission.

This is a background-sync edge case, not a correctness bug in the retake-permission sense — the server enforcement itself is fine. It's a UX/audit-trail gap: worth either (a) a confirmation dialog before discarding the synced answers to start a retake, or (b) not cascading-deleting on permanent failure so the user gets some visible signal that a retake didn't take. Neither is a merge blocker given the server-side guard already prevents any actual policy violation.

Updated Verdict

No remaining CRITICAL items from my side. The two WARNING items from the previous round (dead attendExamAction l10n string, spec gap for the "Open for all statuses" behavior) still stand as minor cleanup. NEEDS DISCUSSION on whether the silent-delete-on-permanent-failure UX above is worth addressing now or acceptable to defer — otherwise this looks mergeable.
· Branch: ref/offline-exam-ux