feat(core): implement redesigned AI tab and chat features

[syed-tp]

• Establish a dedicated AI assistant workspace to optimize student concept-clarification.
• Wrap views with semantics headers, buttons, and scrollable lists to meet WCAG standards.
• Replace hardcoded layouts, animations, and copy with design tokens and localized arb strings.

[claude]

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

---

🤖 Claude finished reviewing

Todo List

• Read AGENTS.md and core docs (ai_context.md, architecture.md)
• Check for relevant ADRs
• Fetch full PR diff (git diff vs merge-base d47b649)
• Review OpenSpec compliance
• Review SDK boundary enforcement
• Review design system & widget usage
• Review accessibility semantics
• Review motion & animation
• Review platform neutrality
• Review navigation (GoRouter)
• Review Riverpod provider pattern
• Review data layer architecture
• Review localization
• Review icon library usage
• Review generated files
• Review architecture & file structure
• Post final structured review summary

---

PR Review Summary

✅ What's good

• OpenSpec change (openspec/changes/redesign-ai-tab/) is thorough — proposal, design (with explicit risks/mitigations), tasks, and per-capability specs are all present. Compliant with SDD requirements.
• No Material/Cupertino widgets, no static token imports, no Platform.isIOS/isAndroid visual branching detected in any new file.
• New routes (/ai/chat, /ai/history) correctly registered as nested GoRoutes under /ai using parentNavigatorKey: rootNavigatorKey, consistent with GoRouter conventions elsewhere in the repo.
• AppIconButton correctly used for the composer's attach/camera buttons (48×48dp + semantics baked in), and most interactive elements are wrapped in AppSemantics.button/header/scrollableList.
• Motion tokens (design.motion.normal, design.motion.easeOut, design.motion.fast) are used correctly in _TypingIndicator and _scrollToBottom, gated by MotionPreferences.shouldAnimate.
• No hand-edits to .g.dart files; ARB → generated l10n diffs are consistent across en/ar/ml/ta.

🔴 CRITICAL — Must fix before merge

1. Missing !mounted guard in _scrollToBottom, contradicting the PR's own design doc. packages/core/lib/screens/ai_chat_immersive_screen.dart:35-52 — design.md explicitly calls out this exact risk ("the scroll controller might be disposed before the post-frame callback runs... Mitigation: guard with if (!mounted) return;"), but the implemented _scrollToBottom() has no such guard before touching _scrollController.hasClients / Design.of(context). If the screen is popped (e.g., user taps back right after sending a message, or during the typewriter animation) before the scheduled addPostFrameCallback fires, this throws ("A ScrollController was used after being disposed."/defunct context lookup). Contrast with _onSendMessage's Future.delayed and Timer.periodic callbacks, which correctly check if (!mounted) return; (lines 69, 89-92).
   Fix this →

2. Chat message list is missing AppSemantics.scrollableList. packages/core/lib/screens/ai_chat_immersive_screen.dart:163-213 — the main conversation AppScroll (rendering _messages + typing indicator) is not wrapped in AppSemantics.scrollableList(itemCount: ...), unlike the equivalent lists in ai_screen.dart:190 and ai_chat_history_screen.dart:24. Per repo accessibility rules, all scrollable lists must expose this semantic.
   Fix this →

🟡 WARNING — Should fix

1. Reused openDetailedLesson ARB key for AI chat items produces a misleading screen-reader announcement. packages/core/lib/screens/ai_screen.dart:108 and packages/core/lib/screens/ai_chat_history_screen.dart:83 call l10n.openDetailedLesson(title) as the accessibility label for chat session cards. That string is "Open lesson: {title}" and is already used for actual lesson items in packages/courses/lib/widgets/lesson_list_item.dart / chapter_content_item.dart. A screen-reader user tapping "Quantum Computing Intro" would hear "Open lesson: Quantum Computing Intro," which is incorrect. Add a dedicated key (e.g. openChatSession) instead of reusing a semantically unrelated string.
2. Typewriter/typing-simulation delays are hardcoded and not gated by MotionPreferences. packages/core/lib/screens/ai_chat_immersive_screen.dart:68 (Duration(milliseconds: 1500)) and :86 (Timer.periodic(const Duration(milliseconds: 30), ...)) don't use design.motion.* tokens and aren't checked against MotionPreferences.shouldAnimate(context). Users with reduce-motion enabled will still see the AI reply stream in character-by-character, since only the dot _TypingIndicator (not the message text reveal) currently respects motion preferences per design.md's decision chore(openspec): archive completed lms-navigation-shell change #3.

🔵 SUGGESTION — Nice to have

1. AiScreen, AiChatHistoryScreen, AiChatImmersiveScreen, and AiComposer all extend Consumer(Stateful)Widget and import flutter_riverpod, but none actually read ref anywhere — data comes from the static mockChatSessions list. Could be plain Stateless/StatefulWidget until real Riverpod-backed data is wired in.
2. Initial message loading in _AiChatImmersiveScreenState.build() (lines 117-129) mutates instance state (_messages, _initialized) and triggers a scroll side-effect directly inside build(). Since GoRouterState.of(context) is available in initState() too, moving this one-time setup there would be more idiomatic and avoid coupling state mutation to the build phase.
3. The composer's send button (ai_composer.dart:120-148) hand-rolls a 48×48dp GestureDetector + Semantics.button wrapper that duplicates what AppIconButton already provides — could reuse AppIconButton (with a custom child, if needed) for consistency with the attach/camera buttons right above it.

Verdict

CHANGES REQUESTED
· Branch: ref/redesign-ai-tab

[syed-tp]

@claude review

[claude]

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

---

🤖 Claude finished reviewing

Todo List

• Read AGENTS.md and core docs (ai_context.md, architecture.md)
• Check for relevant ADRs
• Fetch full PR diff (git diff vs merge-base d47b649)
• Review OpenSpec compliance
• Review SDK boundary enforcement
• Review design system & widget usage
• Review accessibility semantics
• Review motion & animation
• Review platform neutrality
• Review navigation (GoRouter)
• Review Riverpod provider pattern
• Review data layer architecture
• Review localization
• Review icon library usage
• Review generated files
• Review architecture & file structure
• Post final structured review summary

---

PR Review Summary

Re-reviewed the branch, including the newest commit (5754368), which specifically addresses the CRITICAL/WARNING items raised in the previous review pass. All three are confirmed fixed:

• _scrollToBottom in packages/core/lib/screens/ai_chat_immersive_screen.dart:37 now guards with if (!mounted) return; inside the addPostFrameCallback, before touching _scrollController/Design.of(context).
• The message list (ai_chat_immersive_screen.dart:183) is now wrapped in AppSemantics.scrollableList(itemCount: _messages.length, ...).
• A dedicated openChatSession ARB key ("Open chat: {title}") was added and is now used in ai_screen.dart:108 and ai_chat_history_screen.dart:83, instead of reusing openDetailedLesson. Present and consistent across en/ar/ml/ta.
• Bonus: the typing/typewriter timing now derives from design.motion.verySlow and design.motion.fast (motion tokens) instead of hardcoded Durations, and the animated typewriter reveal is skipped entirely when MotionPreferences.shouldAnimate is false (ai_chat_immersive_screen.dart:74-87) — resolving the reduce-motion WARNING too.
• The unrelated AITab._scrollToBottom in packages/courses/lib/widgets/lesson_detail/ai_tab.dart:163 also picked up the same !mounted guard and its scrollable-list label was fixed from a hardcoded 'AI Chat Messages' string to l10n.aiSupportTitle.

✅ What's good

• OpenSpec change (openspec/changes/redesign-ai-tab/) remains thorough (proposal, design w/ risk mitigations, tasks, per-capability specs).
• No Material/Cupertino widgets, no static token imports, no Platform.isIOS/isAndroid visual branching in any changed file.
• SDK import boundaries respected: core has no cross-package imports; courses/testpress changes only touch their own layer.
• Routes (/ai/chat, /ai/history) correctly registered as nested GoRoutes under /ai with parentNavigatorKey: rootNavigatorKey.
• Removed legacy ARB keys (aiSupportGreeting, aiSupportQuickActions, aiSupportAskDoubtTitle, etc.) and the old AiScreen callback params (onAskAiPressed/onViewAllDoubtsPressed/onDoubtTapped) have no remaining references anywhere in the repo — clean removal, no dead code.
• ARB additions/removals are identical in shape across en/ar/ml/ta, and generated l10n files match their .arb sources (no hand-edits).
• No print()/commented-out code left behind in the touched files.

🔴 CRITICAL — Must fix before merge

None found.

🟡 WARNING — Should fix

None found. (Both prior warnings — reused openDetailedLesson string and unguarded hardcoded typing durations — are resolved.)

🔵 SUGGESTION — Nice to have

1. AiScreen, AiChatHistoryScreen, AiChatImmersiveScreen, and AiComposer still extend Consumer(Stateful)Widget and import flutter_riverpod, but none read ref anywhere — data still comes from the static mockChatSessions list (packages/core/lib/data/ai_chat_mock_data.dart). Fine for now since this is explicitly mock-backed, but worth switching to plain Stateless/StatefulWidget (or wiring real Riverpod providers) before this ships with live data.
2. The composer's send button (packages/core/lib/widgets/ai_composer.dart:120-148) still hand-rolls a 48×48dp GestureDetector + AppSemantics.button wrapper that duplicates what AppIconButton already provides for the attach/camera buttons right above it — could be consolidated for consistency.

Verdict

APPROVED
· Branch: ref/redesign-ai-tab