Skip to content

feat(testpress): implement user-facing live classes list and calendar views - #259

Merged
syed-tp merged 4 commits into
mainfrom
feat/live-classes
Aug 18, 2026
Merged

feat(testpress): implement user-facing live classes list and calendar views#259
syed-tp merged 4 commits into
mainfrom
feat/live-classes

Conversation

@syed-tp

@syed-tp syed-tp commented Aug 18, 2026

Copy link
Copy Markdown
Collaborator

Why

Students need a centralized screen to track live class sessions (ongoing, upcoming, completed, or cancelled), which were previously only accessible via a limited dashboard preview section.

What

  • Created a tabbed LiveStreamListScreen and an interactive monthly LiveStreamCalendarView using the syncfusion_flutter_calendar package.
  • Implemented LiveClassesRepository to sync remote payloads and cache them locally in the Drift SQLite database.
  • Created @riverpod generated state notifiers for synchronization updates, and registered the /live-classes GoRouter path in GlobalRoutes.
  • Wrapped scrollable lists in AppSemantics.scrollableList(), resized the toggle switch hit target to 48x48dp for WCAG compliance, and localized all text strings across EN, AR, TA, and ML locales.

@claude

claude Bot commented Aug 18, 2026

Copy link
Copy Markdown

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


PR Review Summary

✅ What's good

  • OpenSpec change present and complete (openspec/changes/feat-live-classes/) with proposal, design, and spec docs — satisfies rule 1.
  • SDK boundaries respected: testpress only imports core/courses, no domain-package cross-imports, app/ untouched directly.
  • No Material/Cupertino widgets or static token imports; colors/spacing/radius consistently read via Design.of(context).
  • _CustomSwitch toggle correctly sized to 48×48dp and uses MotionPreferences.duration() + design.motion.easeOut instead of hardcoded curves/durations — good WCAG 2.5.5 + motion-preference compliance.
  • 3-layer data flow followed: DataSource (abstract) → HttpDataSource/MockDataSource (both implement getLiveClasses) → LiveClassesRepository (Drift cache, single source of truth) → @riverpod LiveStreamList.
  • /live-classes registered as a proper GoRoute under rootNavigatorKey, matching existing immersive-route patterns.
  • Generated files (repository_providers.g.dart, live_stream_provider.g.dart, initialization_provider.g.dart) are consistent with their source annotations — no hand-editing detected.
  • Full localization added across EN/AR/TA/ML for all new user-facing strings.

🔴 CRITICAL — Must fix before merge

  1. Live class duration is silently dropped, so the UI can never show it correctly. LiveClassDto (packages/core/lib/data/models/live_class_dto.dart:7-51) has no durationMinutes field — fromJson parses duration from the API only to bake it into the faculty string ('$providerStr • $durationInt mins', line 40) and then discards the numeric value. The Drift table (live_classes_table.dart) also stores no duration column. Downstream, LiveStreamList.build() (packages/testpress/lib/providers/live_stream_provider.dart:56-62) never sets durationMinutes on LiveStreamItem, so it's always null. As a result: live_stream_card.dart:118-120 always renders "0 mins" for every class, and live_stream_calendar_view.dart:151 always falls back to a hardcoded 60-minute appointment span regardless of the actual scheduled duration. The design doc explicitly calls out "duration" as a tracked field (design.md:20), but the feature never actually surfaces it. Fix this →

🟡 WARNING — Should fix

  1. Global design tokens changed as a side effect of this feature, with app-wide blast radius. design_config.dart changes DesignStatusColors.light()/.dark() for live, completed, and upcoming (e.g. completed green #065F46#2E7D32, upcoming blue #1E40AF#01579B, live red→orange). These tokens are also consumed outside live classes: packages/courses/lib/widgets/lesson_status_badge.dart:26-32, packages/core/lib/screens/ai_screen.dart:218-226, and packages/core/lib/widgets/typography_gallery_screen.dart:105. This silently recolors lesson status badges and the AI screen for an unrelated feature, and isn't mentioned in the PR description or design doc. Consider adding live-class-specific status colors instead of repurposing the shared semantic tokens.
  2. Cancelled classes are mislabeled as "Completed" on the dashboard. today_schedule_section_widget.dart:34 maps dto.LiveClassStatus.cancelled => ClassStatus.completed because packages/courses/lib/models/today_schedule.dart:6 (enum ClassStatus { upcoming, live, completed }) has no cancelled case. Since this PR is what introduces LiveClassStatus.cancelled end-to-end, a cancelled class will now visually appear as "Completed" in the Today Schedule dashboard widget — misleading to students.
  3. Provider imports a domain model from a UI widget file. packages/testpress/lib/providers/live_stream_provider.dart:4 imports ../screens/live_streams/widgets/live_stream_card.dart to obtain LiveStreamItem/LiveStreamStatus. Defining state-layer models inside a screens/…/widgets/ file inverts the intended dependency direction (provider → UI) and makes the model harder to reuse/test independently of the widget tree. Consider moving LiveStreamItem/LiveStreamStatus into the provider file or a dedicated model file.
  4. No tests added for new logic. No unit tests were added for LiveClassDto.fromJson/fromListResponse (particularly the course-map merging and status parsing), LiveClassesRepository, or the LiveStreamList sync/pagination notifier — the only test-related diffs are regenerated mocks for unrelated pre-existing tests.
  5. Accessibility labels hardcoded in English. AppSemantics.scrollableList(label: 'Live classes list', …) (live_stream_list_screen.dart:116) and label: 'Live classes agenda' (live_stream_calendar_view.dart:90) are screen-reader-facing strings that bypass L10n.of(context), unlike every other string in this PR.

🔵 SUGGESTION — Nice to have

  1. Dead-end tap affordance. LiveStreamCard always wraps itself in AppSemantics.button with a chevron icon implying navigation (live_stream_card.dart:45-47, 139-144), but it's rendered with no onTap in both the list (live_stream_list_screen.dart:126) and calendar agenda (live_stream_calendar_view.dart:99-100) — tapping does nothing, and screen readers announce a "button" that performs no action. If a details screen isn't in scope for this PR, consider a follow-up or removing the button semantics/chevron until it's wired up.
  2. Calendar header/day text style is hardcoded rather than sourced from design.typography. live_stream_calendar_view.dart:49-62 builds TextStyle(fontSize: 16, fontWeight: FontWeight.w600, …) manually instead of using a typography token, unlike the rest of the design-token-driven codebase.
  3. New third-party dependency syncfusion_flutter_calendar (and the timezone: any override) is justified inline via comments in pubspec.yaml/design.md, but isn't called out in the PR description itself — worth a one-line mention there for future reviewers.

Verdict

CHANGES REQUESTED

@syed-tp
syed-tp force-pushed the feat/live-classes branch from 0fcea5a to a946a17 Compare August 18, 2026 05:18
@syed-tp

syed-tp commented Aug 18, 2026

Copy link
Copy Markdown
Collaborator Author

@claude review

NOTE: The schema migration is skipped intentionally, as the app is not released to the real users

@claude

claude Bot commented Aug 18, 2026

Copy link
Copy Markdown

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


Claude finished re-reviewing this PR

  • Load repo review rules (AGENTS.md, ai_context.md, architecture.md)
  • Fetch full PR diff vs origin/main
  • Re-check prior CRITICAL/WARNING findings against current code
  • Post updated structured review

Note acknowledged: schema migration intentionally skipped since the app isn't released to real users yet (schemaVersion stays at 1, durationMinutes column just added directly) — reasonable for a pre-release app, not flagged.


PR Review Summary

Since the last review (3 commits ago), the PR has grown to also add durationMinutes persistence, dedicated live-class status colors, cancelled-status support across the dashboard, and a standalone live-class detail flow (/live-classes/:id route + liveClassDetailProvider). I re-verified all previously reported issues against the current diff.

✅ Previously reported issues that are now resolved

  1. Duration data flow (was CRITICAL)LiveClassDto now has a real durationMinutes field (packages/core/lib/data/models/live_class_dto.dart:14,52), LiveClassesTable persists it (live_classes_table.dart:14), and LiveStreamList.build() populates it (live_stream_provider.dart:81). live_stream_card.dart:98-105 now renders the real value via L10n.of(context).liveClassesDurationMins(...).
  2. Repurposed shared design tokens (was WARNING) — dedicated liveClassLive/Completed/Upcoming/Cancelled tokens were added to DesignStatusColors (design_config.dart:913-916, 954-968) instead of mutating the shared live/completed/upcoming tokens, so lesson_status_badge.dart and ai_screen.dart are no longer affected.
  3. Cancelled mislabeled as Completed (was WARNING)ClassStatus now has a cancelled member (today_schedule.dart:6), today_schedule_section_widget.dart:34 maps it correctly, and today_snapshot/shared.dart:39-43 renders a distinct icon for it.
  4. Provider importing UI widget file (was WARNING)LiveStreamItem/LiveStreamStatus now live directly in live_stream_provider.dart:7-25; the widget imports from the provider instead of the reverse.
  5. Hardcoded English a11y labels (was WARNING)live_stream_list_screen.dart:116 and live_stream_calendar_view.dart:91 now use L10n.of(context)... strings.
  6. Dead-end tap affordance (was SUGGESTION)LiveStreamCard now accepts onTap, and both the list (live_stream_list_screen.dart:129) and calendar agenda (live_stream_calendar_view.dart:102-103) wire it to context.push('/live-classes/${item.id}'), which is now a registered GoRoute (global_routes.dart:156-186) backed by a new liveClassDetailProvider (packages/courses/lib/providers/live_class_detail_provider.dart).

The new detail flow itself follows the architecture correctly: DataSource.getLiveClassDetail is added and implemented in both HttpDataSource and MockDataSource, CourseRepository.refreshLiveClass() owns the DB write, and the provider only talks to the repository. LessonDto's flat-JSON parsing fallbacks (lesson_dto.dart:481-495 and the provider/duration/contentUrl fallbacks in _parseLiveStreamLesson/_parseVideoConferenceLesson) are a reasonable way to support the /api/v3/live-classes/contents/{id}/ flat payload alongside the existing nested live_stream/video_conference shapes. design.md was updated with new Decision entries (#4, #5) covering this, so OpenSpec stays in sync — good.

🔴 CRITICAL

None outstanding.

🟡 WARNING — Should fix

  1. Still no unit tests for the repository/provider layer. live_class_dto_test.dart covers LiveClassDto.fromJson/fromListResponse well (including the new cancelled status and course-map merging), but LiveClassesRepository (cache/sync logic) and LiveStreamList's pagination/sync notifier remain untested, as does the new refreshLiveClass/liveClassDetailProvider flat-JSON parsing path in lesson_dto.dart, which is the trickiest logic added in this round. Given the flat-vs-nested JSON detection is heuristic (json.containsKey('detail_url') || ... || json.containsKey('meeting_id')), a regression test guarding against the flat payload being misidentified would be valuable.

🔵 SUGGESTION — Nice to have

  1. Calendar header/day text style still hardcoded. live_stream_calendar_view.dart:49-62 builds TextStyle(fontSize: 16, fontWeight: FontWeight.w600, …) manually rather than sourcing from design.typography — minor, and constrained by Syncfusion's CalendarHeaderStyle API, but worth a token-driven wrapper if reused elsewhere.

Verdict

APPROVED — all CRITICAL and WARNING items from the previous review pass have been addressed; the one remaining WARNING (test coverage for repository/provider/flat-JSON-parsing logic) is worth a follow-up but isn't a merge blocker given this targets a pre-release app.

@syed-tp
syed-tp merged commit 526786d into main Aug 18, 2026
6 checks passed
@syed-tp
syed-tp deleted the feat/live-classes branch August 18, 2026 07:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant