From 75a237b90c886443dc7c3e64c38d848425cc5c94 Mon Sep 17 00:00:00 2001 From: wcjord <32568597+wcjord@users.noreply.github.com> Date: Sat, 14 Feb 2026 14:47:05 -0500 Subject: [PATCH] fix: prioritize bot DM in updateBotOptions, handle DM outside _targetBotChats (#5698) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes #5697 - Extract buildUpdatedBotOptions (pure) and applyBotOptionUpdatesInOrder (async orchestration) as top-level @visibleForTesting functions - Handle bot DM independently of _targetBotChats filter — the DM may lack a botOptions state event or have a stale activityPlan, but it's the most important room to keep current - Update remaining rooms sequentially (not Future.wait) to avoid rate-limiting - DM errors propagate; other room errors are logged and isolated - Add 17 unit tests covering both extracted functions - Add profile.instructions.md design doc for profile settings architecture --- .github/instructions/profile.instructions.md | 110 +++++++ .../utils/bot_client_extension.dart | 137 +++++--- test/pangea/update_bot_options_test.dart | 301 ++++++++++++++++++ 3 files changed, 509 insertions(+), 39 deletions(-) create mode 100644 .github/instructions/profile.instructions.md create mode 100644 test/pangea/update_bot_options_test.dart diff --git a/.github/instructions/profile.instructions.md b/.github/instructions/profile.instructions.md new file mode 100644 index 000000000..71f904e62 --- /dev/null +++ b/.github/instructions/profile.instructions.md @@ -0,0 +1,110 @@ +--- +applyTo: "lib/pangea/user/user_model.dart, lib/pangea/user/user_controller.dart, lib/pangea/user/public_profile_model.dart, lib/pangea/user/analytics_profile_model.dart, lib/pangea/chat_settings/utils/bot_client_extension.dart, lib/pangea/chat_settings/models/bot_options_model.dart, lib/pangea/bot/utils/bot_room_extension.dart, lib/pangea/bot/widgets/bot_chat_settings_dialog.dart, lib/pangea/learning_settings/**, lib/pangea/common/controllers/pangea_controller.dart" +--- + +# Profile Settings — Architecture & Contracts + +How profile settings are structured, stored, propagated, and surfaced to other users. + +## Data Model + +`Profile` (in [user_model.dart](lib/pangea/user/user_model.dart)) is the top-level container. It wraps three sub-models: + +- **`UserSettings`** — learning prefs: target/source language, CEFR level, gender, voice, country, about, etc. +- **`UserToolSettings`** — per-tool on/off toggles (interactive translator, grammar, immersion mode, definitions, auto-IGC, autocorrect, TTS). +- **`InstructionSettings`** — which instructional tooltips the user has dismissed. + +A separate **`PublicProfileModel`** (in [public_profile_model.dart](lib/pangea/user/public_profile_model.dart)) holds data visible to other users: analytics level/XP per language, country, and about. It lives on the Matrix user profile (public), not in account data. + +> **Open question**: `country` and `about` are the only fields that cross the private → public boundary (they live in `UserSettings` but get synced to `PublicProfileModel`). It might be cleaner to keep them solely in `PublicProfileModel` and edit them in a "public profile" editor, making the privacy boundary explicit. + +## Storage & Sync + +| Concern | Design Decision | Why | +|---|---|---| +| **Format** | Single JSON blob in Matrix account data under key `profile` | Atomic writes; no partial-update races between fields | +| **Cross-device sync** | Rides on standard Matrix sync | No extra infrastructure; every logged-in device gets updates automatically | +| **Caching** | `UserController` reads account data on first sync, caches in memory, and refreshes on subsequent sync events | Avoids repeated deserialization; single source of truth in-process | +| **Change detection** | Two separate streams: **`languageStream`** (source/target language changed) and **`settingsUpdateStream`** (everything else) | Language changes have heavyweight side-effects (cache clearing, bot option updates, public profile sync) that other setting changes don't need | +| **Side-effect orchestration** | `PangeaController` subscribes to both streams and triggers bot option propagation + public profile sync | Keeps `UserController` focused on data; orchestration lives in the central controller | +| **Migration** | Legacy users stored individual account data keys; a migration path reads those keys and re-saves in the unified format | One-time upgrade, no ongoing cost | + +## Entry Points for Changing Settings + +### Full settings UI (opens `SettingsLearning`) + +1. **Settings page → Learning** — full-page at `/settings/learning` +2. **IGC button long-press** — modal dialog from the writing-assistance button in the chat input +3. **IT bar gear icon** — modal dialog from the interactive translation bar +4. **Analytics language indicator** — modal dialog from the language-pair chip (e.g. "EN → ES") on the learning progress widget + +All four use the same `SettingsLearning` widget and the same save flow: write to account data → wait for sync round-trip → stream dispatch → side-effects. + +### Per-room bot settings + +5. **Bot member menu** — `BotChatSettingsDialog`, opened from the bot's member profile in a room. Updates the profile *and* immediately calls bot option propagation rather than waiting for the stream (avoids perceived lag in the room the user is looking at). + +### Inline language-switch prompts + +These bypass the full settings UI and only change `targetLanguage`: + +6. **Activity session mismatch** — When the user tries to send a message in an activity room whose target language differs from their current L2, a popup offers to switch. Rate-limited to once per 30 minutes per room (via `LanguageMismatchRepo`) to avoid nagging. On confirm, updates the profile and auto-sends the pending message. + +7. **Reading toolbar mismatch** — When the user taps a message in a language that doesn't match their L2 and the selected toolbar mode is unavailable, a snackbar offers a "Learn" button to switch their target language. + +### Contract all paths must satisfy + +Every path that changes settings **must** write to account data via `UserController.updateProfile`. The sync-driven stream is the canonical trigger for propagating changes to the bot and public profile. The only exception is the bot-settings dialog (path 5), which additionally calls bot propagation eagerly for responsiveness. + +## Bot Option Propagation + +The bot reads the user's settings from a `pangea.bot_options` room state event. The client is responsible for keeping this event current. + +### Priority ordering + +1. **Bot DM first** — The user's 1:1 chat with the bot is updated first, synchronously, with errors propagating to the caller. This is the room the user is most likely actively using. +2. **Other eligible rooms sequentially** — Updated one-by-one (not in parallel) to avoid Matrix rate-limiting. Individual failures are logged but don't block other rooms. + +### Eligible room criteria + +A room receives bot option updates if: +- It has a `pangea.bot_options` state event +- It has **no** `pangea.activity_plan` state event (activity rooms manage their own options) +- It has exactly 2 joined members, one of which is the bot + +### Retry policy + +Each room state write retries up to 3× with exponential backoff (5 s → 10 s → 20 s). + +### Known limitation + +The activity-plan filter uses state event presence, but Matrix state events persist after an activity ends. Rooms with stale activity plans won't get their options updated. The DM-first strategy mitigates this since the most important room is always covered. + +## Public vs. Private Boundary + +| Data | Where it lives | Who can see it | +|---|---|---| +| `UserSettings` (language, gender, CEFR, voice, tool toggles) | Matrix account data | Only the owning user | +| `PublicProfileModel` (analytics level/XP, country, about) | Matrix user profile | Anyone in shared rooms | + +`country` and `about` are synced from `UserSettings` → `PublicProfileModel` on every settings update. All other settings remain private. Other users see *derived* analytics levels (computed from chat activity), not the self-reported CEFR level. + +## Key Files + +| Concern | Location | +|---|---| +| Profile / UserSettings models | [lib/pangea/user/user_model.dart](lib/pangea/user/user_model.dart) | +| UserController (cache, streams, updateProfile) | [lib/pangea/user/user_controller.dart](lib/pangea/user/user_controller.dart) | +| Side-effect subscriptions | [lib/pangea/common/controllers/pangea_controller.dart](lib/pangea/common/controllers/pangea_controller.dart) | +| Bot option propagation | [lib/pangea/chat_settings/utils/bot_client_extension.dart](lib/pangea/chat_settings/utils/bot_client_extension.dart) | +| BotOptionsModel | [lib/pangea/chat_settings/models/bot_options_model.dart](lib/pangea/chat_settings/models/bot_options_model.dart) | +| Language mismatch popup + rate limiter | [lib/pangea/learning_settings/](lib/pangea/learning_settings/) | +| Public profile model | [lib/pangea/user/public_profile_model.dart](lib/pangea/user/public_profile_model.dart) | +| Settings UI | [lib/pangea/learning_settings/settings_learning.dart](lib/pangea/learning_settings/settings_learning.dart) | +| Bot chat settings dialog | [lib/pangea/bot/widgets/bot_chat_settings_dialog.dart](lib/pangea/bot/widgets/bot_chat_settings_dialog.dart) | + +## Future Work + +- **Bio / about editing** — Users currently have no UI to set or edit their `about` field. Add an input to either the learning settings page or a dedicated public-profile editor. +- **Bio / about display** — Decide where other users see the bio. Candidates: user profile sheet in a room, member list hover card, space member directory. Also resolve whether `about` should stay in `UserSettings` (private, synced to public) or move entirely to `PublicProfileModel`. +- **Public learning stats** — Surface vocab count, grammar construct progress, and completed activities on a user's public profile so classmates and teachers can see learning outcomes, not just XP/level. \ No newline at end of file diff --git a/lib/pangea/chat_settings/utils/bot_client_extension.dart b/lib/pangea/chat_settings/utils/bot_client_extension.dart index 307dc1400..134246ba5 100644 --- a/lib/pangea/chat_settings/utils/bot_client_extension.dart +++ b/lib/pangea/chat_settings/utils/bot_client_extension.dart @@ -1,5 +1,6 @@ import 'package:collection/collection.dart'; import 'package:matrix/matrix.dart'; +import 'package:meta/meta.dart'; import 'package:fluffychat/pangea/activity_sessions/activity_room_extension.dart'; import 'package:fluffychat/pangea/bot/utils/bot_name.dart'; @@ -13,6 +14,65 @@ import 'package:fluffychat/pangea/learning_settings/gender_enum.dart'; import 'package:fluffychat/pangea/user/user_model.dart'; import 'package:fluffychat/widgets/matrix.dart'; +/// Builds updated [BotOptionsModel] if any bot-relevant user setting differs +/// from [currentOptions]. Returns null when no changes are needed. +@visibleForTesting +BotOptionsModel? buildUpdatedBotOptions({ + required BotOptionsModel? currentOptions, + required UserSettings userSettings, + required String? userId, +}) { + final botOptions = currentOptions ?? const BotOptionsModel(); + final targetLanguage = userSettings.targetLanguage; + final languageLevel = userSettings.cefrLevel; + final voice = userSettings.voice; + final gender = userSettings.gender; + + if (botOptions.targetLanguage == targetLanguage && + botOptions.languageLevel == languageLevel && + botOptions.targetVoice == voice && + botOptions.userGenders[userId] == gender) { + return null; + } + + final updatedGenders = Map.from(botOptions.userGenders); + if (userId != null && updatedGenders[userId] != gender) { + updatedGenders[userId] = gender; + } + + return botOptions.copyWith( + targetLanguage: targetLanguage, + languageLevel: languageLevel, + targetVoice: voice, + userGenders: updatedGenders, + ); +} + +/// Executes async update functions in priority order: +/// 1. [priorityUpdate] runs first — errors propagate to the caller. +/// 2. [remainingUpdates] run sequentially — errors go to [onError]. +/// +/// This ensures the bot DM (most important room) is updated first and +/// rate-limiting from parallel requests doesn't block it. +@visibleForTesting +Future applyBotOptionUpdatesInOrder({ + required Future Function()? priorityUpdate, + required List Function()> remainingUpdates, + void Function(Object error, StackTrace stack)? onError, +}) async { + if (priorityUpdate != null) { + await priorityUpdate(); + } + + for (final update in remainingUpdates) { + try { + await update(); + } catch (e, s) { + onError?.call(e, s); + } + } +} + extension BotClientExtension on Client { bool get hasBotDM => rooms.any((r) => r.isBotDM); Room? get botDM => rooms.firstWhereOrNull((r) => r.isBotDM); @@ -52,52 +112,51 @@ extension BotClientExtension on Client { ); Future updateBotOptions(UserSettings userSettings) async { - final targetBotRooms = [..._targetBotChats]; - if (targetBotRooms.isEmpty) return; + final dm = botDM; + Future Function()? dmUpdate; - try { - final futures = []; - for (final targetBotRoom in targetBotRooms) { - final botOptions = targetBotRoom.botOptions ?? const BotOptionsModel(); - final targetLanguage = userSettings.targetLanguage; - final languageLevel = userSettings.cefrLevel; - final voice = userSettings.voice; - final gender = userSettings.gender; - - if (botOptions.targetLanguage == targetLanguage && - botOptions.languageLevel == languageLevel && - botOptions.targetVoice == voice && - botOptions.userGenders[userID] == gender) { - continue; - } - - final updatedGenders = Map.from( - botOptions.userGenders, - ); - - if (updatedGenders[userID] != gender) { - updatedGenders[userID!] = gender; - } - - final updated = botOptions.copyWith( - targetLanguage: targetLanguage, - languageLevel: languageLevel, - targetVoice: voice, - userGenders: updatedGenders, - ); - futures.add(targetBotRoom.setBotOptions(updated)); + // Handle the bot DM independently of _targetBotChats. + // The DM may not pass _targetBotChats filters (e.g., botOptions is null, + // or a stale activityPlan state event exists), but it's the most important + // room to keep current. + if (dm != null) { + final updated = buildUpdatedBotOptions( + currentOptions: dm.botOptions, + userSettings: userSettings, + userId: userID, + ); + if (updated != null) { + dmUpdate = () => dm.setBotOptions(updated); } + } - await Future.wait(futures); - } catch (e, s) { - ErrorHandler.logError( + // Remaining eligible rooms, excluding the DM (already handled above). + final otherUpdates = Function()>[]; + for (final room in _targetBotChats) { + if (room == dm) continue; + + final updated = buildUpdatedBotOptions( + currentOptions: room.botOptions, + userSettings: userSettings, + userId: userID, + ); + if (updated == null) continue; + + otherUpdates.add(() => room.setBotOptions(updated)); + } + + if (dmUpdate == null && otherUpdates.isEmpty) return; + + await applyBotOptionUpdatesInOrder( + priorityUpdate: dmUpdate, + remainingUpdates: otherUpdates, + onError: (e, s) => ErrorHandler.logError( e: e, s: s, data: { 'userSettings': userSettings.toJson(), - 'targetBotRooms': targetBotRooms.map((r) => r.id).toList(), }, - ); - } + ), + ); } } diff --git a/test/pangea/update_bot_options_test.dart b/test/pangea/update_bot_options_test.dart new file mode 100644 index 000000000..c143b84b7 --- /dev/null +++ b/test/pangea/update_bot_options_test.dart @@ -0,0 +1,301 @@ +import 'package:fluffychat/pangea/chat_settings/models/bot_options_model.dart'; +import 'package:fluffychat/pangea/chat_settings/utils/bot_client_extension.dart'; +import 'package:fluffychat/pangea/learning_settings/gender_enum.dart'; +import 'package:fluffychat/pangea/learning_settings/language_level_type_enum.dart'; +import 'package:fluffychat/pangea/user/user_model.dart'; +import 'package:flutter_test/flutter_test.dart'; + +void main() { + // --------------------------------------------------------------------------- + // buildUpdatedBotOptions — pure logic tests + // --------------------------------------------------------------------------- + group('buildUpdatedBotOptions', () { + final baseSettings = UserSettings( + targetLanguage: 'es', + cefrLevel: LanguageLevelTypeEnum.b1, + voice: 'voice_1', + gender: GenderEnum.woman, + ); + + const userId = '@user:server'; + + test('returns null when all relevant fields already match', () { + final currentOptions = BotOptionsModel( + targetLanguage: 'es', + languageLevel: LanguageLevelTypeEnum.b1, + targetVoice: 'voice_1', + userGenders: const {userId: GenderEnum.woman}, + ); + + final result = buildUpdatedBotOptions(currentOptions: currentOptions, userSettings: baseSettings, userId: userId); + + expect(result, isNull); + }); + + test('returns updated model when targetLanguage differs', () { + final currentOptions = BotOptionsModel( + targetLanguage: 'fr', + languageLevel: LanguageLevelTypeEnum.b1, + targetVoice: 'voice_1', + userGenders: const {userId: GenderEnum.woman}, + ); + + final result = buildUpdatedBotOptions(currentOptions: currentOptions, userSettings: baseSettings, userId: userId); + + expect(result, isNotNull); + expect(result!.targetLanguage, 'es'); + // Other fields carried over + expect(result.languageLevel, LanguageLevelTypeEnum.b1); + expect(result.targetVoice, 'voice_1'); + }); + + test('returns updated model when languageLevel differs', () { + final currentOptions = BotOptionsModel( + targetLanguage: 'es', + languageLevel: LanguageLevelTypeEnum.a1, + targetVoice: 'voice_1', + userGenders: const {userId: GenderEnum.woman}, + ); + + final result = buildUpdatedBotOptions(currentOptions: currentOptions, userSettings: baseSettings, userId: userId); + + expect(result, isNotNull); + expect(result!.languageLevel, LanguageLevelTypeEnum.b1); + }); + + test('returns updated model when voice differs', () { + final currentOptions = BotOptionsModel( + targetLanguage: 'es', + languageLevel: LanguageLevelTypeEnum.b1, + targetVoice: 'voice_2', + userGenders: const {userId: GenderEnum.woman}, + ); + + final result = buildUpdatedBotOptions(currentOptions: currentOptions, userSettings: baseSettings, userId: userId); + + expect(result, isNotNull); + expect(result!.targetVoice, 'voice_1'); + }); + + test('returns updated model when gender differs', () { + final currentOptions = BotOptionsModel( + targetLanguage: 'es', + languageLevel: LanguageLevelTypeEnum.b1, + targetVoice: 'voice_1', + userGenders: const {userId: GenderEnum.man}, + ); + + final result = buildUpdatedBotOptions(currentOptions: currentOptions, userSettings: baseSettings, userId: userId); + + expect(result, isNotNull); + expect(result!.userGenders[userId], GenderEnum.woman); + }); + + test('defaults to empty BotOptionsModel when currentOptions is null', () { + final result = buildUpdatedBotOptions(currentOptions: null, userSettings: baseSettings, userId: userId); + + expect(result, isNotNull); + expect(result!.targetLanguage, 'es'); + expect(result.languageLevel, LanguageLevelTypeEnum.b1); + expect(result.targetVoice, 'voice_1'); + expect(result.userGenders[userId], GenderEnum.woman); + }); + + test('preserves gender entries for other users', () { + final currentOptions = BotOptionsModel( + targetLanguage: 'fr', // different → triggers update + languageLevel: LanguageLevelTypeEnum.b1, + targetVoice: 'voice_1', + userGenders: const {'@other:server': GenderEnum.man, userId: GenderEnum.woman}, + ); + + final result = buildUpdatedBotOptions(currentOptions: currentOptions, userSettings: baseSettings, userId: userId); + + expect(result, isNotNull); + expect(result!.userGenders['@other:server'], GenderEnum.man); + expect(result.userGenders[userId], GenderEnum.woman); + }); + + test('handles null userId gracefully', () { + const currentOptions = BotOptionsModel(targetLanguage: 'fr'); + + final result = buildUpdatedBotOptions(currentOptions: currentOptions, userSettings: baseSettings, userId: null); + + expect(result, isNotNull); + expect(result!.targetLanguage, 'es'); + // Gender not set because userId is null + expect(result.userGenders, isEmpty); + }); + }); + + // --------------------------------------------------------------------------- + // applyBotOptionUpdatesInOrder — async orchestration tests + // --------------------------------------------------------------------------- + group('applyBotOptionUpdatesInOrder', () { + test('executes priority update before remaining updates', () async { + final callLog = []; + + await applyBotOptionUpdatesInOrder( + priorityUpdate: () async { + callLog.add('dm'); + }, + remainingUpdates: [ + () async { + callLog.add('room_a'); + }, + () async { + callLog.add('room_b'); + }, + ], + ); + + expect(callLog, ['dm', 'room_a', 'room_b']); + }); + + test('executes remaining updates sequentially, not in parallel', () async { + final timestamps = {}; + var counter = 0; + + await applyBotOptionUpdatesInOrder( + priorityUpdate: () async { + timestamps['dm_start'] = counter++; + await Future.delayed(const Duration(milliseconds: 30)); + timestamps['dm_end'] = counter++; + }, + remainingUpdates: [ + () async { + timestamps['a_start'] = counter++; + await Future.delayed(const Duration(milliseconds: 30)); + timestamps['a_end'] = counter++; + }, + () async { + timestamps['b_start'] = counter++; + await Future.delayed(const Duration(milliseconds: 30)); + timestamps['b_end'] = counter++; + }, + ], + ); + + // Sequential order: dm completes before a starts, a before b + expect(timestamps['dm_end']!, lessThan(timestamps['a_start']!)); + expect(timestamps['a_end']!, lessThan(timestamps['b_start']!)); + }); + + test('propagates priority update errors to caller', () async { + expect( + () => applyBotOptionUpdatesInOrder( + priorityUpdate: () async { + throw Exception('DM update failed'); + }, + remainingUpdates: [], + ), + throwsA(isA()), + ); + }); + + test('remaining updates do NOT execute when priority update fails', () async { + final callLog = []; + + try { + await applyBotOptionUpdatesInOrder( + priorityUpdate: () async { + throw Exception('DM failed'); + }, + remainingUpdates: [ + () async { + callLog.add('room_a'); + }, + ], + ); + } catch (_) {} + + expect(callLog, isEmpty); + }); + + test('isolates errors in remaining updates and continues to next room', () async { + final callLog = []; + final errors = []; + + await applyBotOptionUpdatesInOrder( + priorityUpdate: () async { + callLog.add('dm'); + }, + remainingUpdates: [ + () async { + callLog.add('room_a'); + }, + () async { + throw Exception('room_b failed'); + }, + () async { + callLog.add('room_c'); + }, + ], + onError: (e, _) => errors.add(e), + ); + + // room_b's error didn't prevent room_c from running + expect(callLog, ['dm', 'room_a', 'room_c']); + expect(errors, hasLength(1)); + expect(errors.first, isA()); + }); + + test('works correctly when priority update is null', () async { + final callLog = []; + + await applyBotOptionUpdatesInOrder( + priorityUpdate: null, + remainingUpdates: [ + () async { + callLog.add('room_a'); + }, + () async { + callLog.add('room_b'); + }, + ], + ); + + expect(callLog, ['room_a', 'room_b']); + }); + + test('handles empty remaining updates list', () async { + final callLog = []; + + await applyBotOptionUpdatesInOrder( + priorityUpdate: () async { + callLog.add('dm'); + }, + remainingUpdates: [], + ); + + expect(callLog, ['dm']); + }); + + test('handles all null / empty gracefully', () async { + // Should complete without error + await applyBotOptionUpdatesInOrder(priorityUpdate: null, remainingUpdates: []); + }); + + test('multiple remaining errors are all reported', () async { + final errors = []; + + await applyBotOptionUpdatesInOrder( + priorityUpdate: null, + remainingUpdates: [ + () async { + throw Exception('fail_1'); + }, + () async { + throw Exception('fail_2'); + }, + () async { + throw Exception('fail_3'); + }, + ], + onError: (e, _) => errors.add(e), + ); + + expect(errors, hasLength(3)); + }); + }); +}