feat: Integrate Microsoft Teams for video conference lessons

[pugal66]

• Add support to identify MS Teams as a live stream provider
• Add dedicated TeamsVideoConferenceScreen with WebView joining flow
• Request camera/mic permissions before WebView loads
• Add localized strings for Teams meeting join flows and errors

[pugal66]

@claude review

[claude]

Claude encountered an error after 0s —— View job

---

I'll analyze this and get back to you.

[pugal66]

@claude review

[claude]

Claude encountered an error after 0s —— View job

---

I'll analyze this and get back to you.

[pugal66]

@claude review

[claude]

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

---

PR Review Summary

✅ What's good

• OpenSpec change (openspec/changes/support-ms-teams-lessons/) is present with proposal/design/spec/tasks — satisfies the spec-driven requirement for this new capability.
• teams_web_view.dart correctly imports package:flutter/widgets.dart (not material.dart) and uses core primitives (AppText, AppLoadingIndicator, AppSemantics.container, AppToast) and Design.of(context) tokens — good response to the earlier review comment.
• teamsPermissionRequired unused localization key was removed as requested in review comments; generated .arb/.dart l10n files are consistent with each other across en/ar/ml/ta.
• webview_flutter and permission_handler are already existing dependencies in packages/courses/pubspec.yaml — no new/unjustified third-party deps added.
• SDK boundaries respected: courses only imports package:core/core.dart plus Flutter SDK/third-party packages, no cross-domain imports.
• Navigation uses Navigator.of(context).push(AppRoute(page: ...)), consistent with the existing pattern in this same file for the Zoom flow and with ai_context.md's documented pattern for nested/modal pushes.
• Native permission strings (NSCameraUsageDescription, NSMicrophoneUsageDescription, Android CAMERA/RECORD_AUDIO) already exist in the app manifests, so permission_handler requests will resolve correctly.

🔴 CRITICAL — Must fix before merge

None found.

🟡 WARNING — Should fix

1. String-interpolation bug in error log — packages/courses/lib/widgets/lesson_detail/teams_web_view.dart:317-318:

   debugPrint('Ignored Teams background resource error: \${error.description}');

   The \$ escapes the interpolation, so this always prints the literal text ${error.description} instead of the actual error message. Should be '...: ${error.description}'. Fix this →
2. No visible way to exit the Teams meeting screen — TeamsVideoConferenceScreen (teams_web_view.dart:225-374) renders only the WebViewWidget + loading overlay in a Stack/SafeArea, with no header/close affordance. It relies entirely on the OS back gesture/button (which isn't guaranteed on iOS with a PageRouteBuilder-based AppRoute, and isn't discoverable for screen-reader users). Per the accessibility rules, screen titles should use AppSemantics.header(), but the title constructor field (teams_web_view.dart:227) is never read/rendered anywhere — it's effectively dead code and the screen has no accessible header or close action.
3. Inconsistent error reporting vs. sibling LessonWebView — _hideScrollbars() (teams_web_view.dart:107-120) and _autoJoinTeamsMeetingWeb() (teams_web_view.dart:193-196) swallow runJavaScript failures with only debugPrint, whereas the existing lesson_web_view.dart reports equivalent JS-injection failures via SentryService().captureException(...). Silent failures here mean auto-join breakage (e.g., after a Teams DOM change, a risk the design doc itself calls out) won't be visible in Sentry.
4. Numerous debugPrint calls left in production code (teams_web_view.dart:77,85,89,92,118,195,203,208,317) — the repo's own guidance flags debug print-style statements in production code; debugPrint is not stripped in release builds. Worth gating behind kDebugMode or removing before merge.
5. Silent screen close on permission denial — _checkPermissionsAndLoad() (teams_web_view.dart:276-291) calls Navigator.of(context).pop() with no toast/feedback when permissions are denied (this replaced the removed teamsPermissionRequired string). Users tapping "Attend Class" will see the screen flash and disappear with no explanation of why they weren't joined.

🔵 SUGGESTION — Nice to have

1. AppText.body(..., style: const TextStyle(fontWeight: FontWeight.w500)) (teams_web_view.dart:~365) hardcodes a TextStyle override rather than sourcing weight from design.typography, slightly bypassing the token system for an otherwise token-driven widget.
2. Consider extracting the large inline JS auto-join script (teams_web_view.dart:130-190) into a separate asset/constants file for readability and easier testing/updates when Teams' DOM changes (a risk the design doc already anticipates).

Verdict

CHANGES REQUESTED

(Mainly the missing exit affordance for the Teams screen and the broken interpolation in the error log — the rest are lower-severity cleanups.)

[pugal66]

@claude review

[claude]

Claude finished @pugal66's task in 2m 4s —— View job

---

Review complete

• Read AGENTS.md and core docs (ai_context.md, architecture.md)
• Check for relevant ADRs (0007-zoom-sdk-integration.md, 0006-sentry-error-tracking.md)
• Fetch full PR diff (updated since last review)
• Review OpenSpec compliance
• Review SDK boundaries, design system, a11y, navigation, l10n, etc.
• Post final review summary

PR Review Summary

This is a re-review of the latest push, which addressed the prior review's findings. All previously flagged CRITICAL/WARNING items appear resolved:

✅ What's good

• OpenSpec change (openspec/changes/support-ms-teams-lessons/) is present with proposal/design/spec/tasks, and the spec's scenarios (permission denial toast, exit via header back button, silent Sentry reporting on auto-join failure) now match the implementation exactly.
• teams_web_view.dart only imports package:flutter/widgets.dart (no material.dart), webview_flutter, permission_handler, and package:core/core.dart — respects SDK boundaries and neutral-UI rules.
• Exit affordance fixed: TeamsVideoConferenceScreen now renders inside LessonDetailShell with onBack: () => Navigator.of(context).pop(), giving an accessible, visible back button (AppSemantics.button inside the shell header) instead of relying on OS gestures.
• Error reporting fixed: TeamsAutoJoinHandler's onError callback now routes JS-injection failures (_hideScrollbars, _autoJoinTeamsMeetingWeb) through SentryService().captureException(...), consistent with lesson_web_view.dart's pattern. The onWebResourceError no-op also mirrors the existing sibling file (lesson_web_view.dart:51), so it's consistent with established convention, not a regression.
• debugPrint removed: no debugPrint/print calls remain in the file.
• Permission-denial UX fixed: _checkPermissionsAndLoad() now shows AppToast with the restored, more descriptive teamsPermissionRequired string before popping, so users get feedback instead of a silent close.
• teamsJoinMeetingLoading / teamsPermissionRequired l10n keys are consistent across .arb sources and generated AppLocalizations/_en/_ar/_ml/_ta files.
• webview_flutter / permission_handler are pre-existing deps in packages/courses/pubspec.yaml — no new third-party packages introduced.
• Navigation uses Navigator.of(context).push(AppRoute(page: ...)), matching the existing Zoom flow in the same file and the documented nested/modal-push pattern.

🔴 CRITICAL — Must fix before merge

None found.

🟡 WARNING — Should fix

None found.

🔵 SUGGESTION — Nice to have

1. Silent fallback when a Teams lesson has no contentUrl — video_conference_viewer.dart:39 only routes to TeamsVideoConferenceScreen when lesson.isTeams && lesson.contentUrl != null. If a Teams lesson somehow has a null contentUrl, execution falls through to the native Zoom meetingService.joinMeeting(...) path with Teams' conferenceId/password values, which will fail and only surface as a generic "Failed to join the meeting" toast + Sentry exception. Consider an explicit early-return/toast for isTeams && contentUrl == null so the failure is diagnosable without cross-referencing Sentry stack traces.
2. Consider extracting the large inline JS auto-join script (teams_web_view.dart:132-188) into a separate asset/constants file for readability and easier updates when Teams' DOM changes (a risk the design doc already calls out).
3. No widget/unit tests were added for TeamsAutoJoinHandler's URL-gate detection (_isTeamsJoinGateUrl) or the permission-denial flow. Not blocking, but worth covering given the auto-join logic's fragility to upstream DOM changes (as the design doc itself flags as a risk).

Verdict

APPROVED

[pugal66]

@claude review

[claude]

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

---

Review complete

• Read AGENTS.md and core docs (ai_context.md, architecture.md)
• Check for relevant ADRs (0006-sentry-error-tracking.md, 0007-zoom-sdk-integration.md)
• Review diff of latest commit (7eb4f00) since the prior approval
• Review OpenSpec compliance, SDK boundaries, design system, a11y, navigation, l10n, tests
• Post final review summary

PR Review Summary

This is a re-review focused on the latest commit (7eb4f00), pushed after the previous review already APPROVED the PR. That commit addresses two of the three outstanding SUGGESTIONs from the last pass:

✅ What's good

• Missing-URL fallback fixed (previous suggestion feat(ui): implement LMS core primitives #1): video_conference_viewer.dart:246-253 now explicitly checks _isLive && lesson.isTeams && lesson.contentUrl == null and renders a warning ConferenceInfoRow (using L10n.of(context).liveStreamJoinFailed / new teamsMissingJoinLink string) instead of silently falling through to the Zoom join path. This follows the exact same pattern already used for the "ended"/"not started" states in the same widget, so it's visually and structurally consistent.
• New tests added (previous suggestion chore(openspec): archive completed lms-navigation-shell change #3): packages/courses/test/widgets/teams_auto_join_handler_test.dart covers TeamsAutoJoinHandler's URL-gate detection (_isTeamsJoinGateUrl, case-insensitive, multiple gate/non-gate URLs) and the JS-channel message flow (onButtonClicked, onLobbyDetected, onAutoJoinTimeout, post-dispose() messages). A minimal @visibleForTesting simulateJsMessage hook was added to teams_web_view.dart:214-216 for this — a reasonable, narrowly-scoped test seam.
• New teamsMissingJoinLink l10n key is present and consistent across app_en/ar/ml/ta.arb and the corresponding generated AppLocalizations* files.
• No SDK boundary, design-token, or Material/Cupertino violations introduced by this commit — video_conference_viewer.dart's pre-existing material.dart import is unchanged from before this PR, not a new issue.
• All CRITICAL/WARNING items from the two earlier review rounds (exit affordance, Sentry error reporting, permission-denial toast, string-interpolation bug, debugPrint cleanup) remain resolved in this version.

🔴 CRITICAL — Must fix before merge

None found.

🟡 WARNING — Should fix

None found.

🔵 SUGGESTION — Nice to have

1. The OpenSpec spec (openspec/changes/support-ms-teams-lessons/specs/ms-teams-webview-join/spec.md) doesn't mention the new "missing join link" scenario added in this commit. Not blocking (it's a small incremental UX fix, not a new capability), but worth a one-line scenario addition for completeness since the rest of the spec is otherwise kept in sync with implementation.
2. Still open from the prior review: consider extracting the large inline JS auto-join script (teams_web_view.dart:132-188) into a separate asset/constants file for readability and easier updates when Teams' DOM changes (a risk the design doc itself calls out).
3. Note: I verified the new test logic by static reading (the assertions correctly match TeamsAutoJoinHandler's state transitions), but couldn't execute flutter test in this sandboxed session (command requires approval this tool isn't granted). Worth confirming green in CI.

Verdict

APPROVED
·