feat(quests): enforce per-Mission pinned-activity restriction from course-space state#7750
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughPer-course activity pins are stored in teacher-mode state, serialized to JSON, and applied as fail-open restrictions to quest outlines and world-map activity cards. Chat, course, and joined-course flows pass the relevant pins into loaders and caches. ChangesPer-course activity pinning
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant CourseRoom
participant OutlineLoader
participant QuestOutline
participant WorldMap
participant PinsManager
CourseRoom->>OutlineLoader: Provide pinnedActivitiesByObjective
OutlineLoader->>QuestOutline: Apply restrictedTo
QuestOutline-->>OutlineLoader: Filtered outline
WorldMap->>CourseRoom: Find joined course room
WorldMap->>PinsManager: Load course-scoped pins with pin map
PinsManager-->>WorldMap: Filtered activity cards
Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/routes/chat/chat_details/chat_details.dart (1)
85-105: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftInvalidate every pin-dependent projection when teacher-mode state changes. These consumers load pins once based on stable IDs, so state arriving or changing through Matrix sync leaves outlines, progression, and map markers stale.
lib/routes/chat/chat_details/chat_details.dart#L85-L105: reload when the current room's pin mapping changes.lib/routes/courses/own/selected_course_page.dart#L65-L78: observe the matched course room's teacher-mode state and reload its outline.lib/routes/world/joined_objective_cache.dart#L108-L115: include pins and threshold values in the objective-cache fingerprint.lib/routes/world/world_map.dart#L405-L409: reload course-scoped pins when the active room's pin mapping changes.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/routes/chat/chat_details/chat_details.dart` around lines 85 - 105, Invalidate pin-dependent projections when teacher-mode state changes: in lib/routes/chat/chat_details/chat_details.dart lines 85-105, reload via the ChatDetails lifecycle flow when the current room’s pin mapping changes; in lib/routes/courses/own/selected_course_page.dart lines 65-78, observe the matched course room’s teacher-mode state and reload its outline; in lib/routes/world/joined_objective_cache.dart lines 108-115, include pins and threshold values in the objective-cache fingerprint; and in lib/routes/world/world_map.dart lines 405-409, reload course-scoped pins when the active room’s pin mapping changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@lib/routes/chat/chat_details/teacher_mode_model.dart`:
- Around line 26-40: Update TeacherModeModel.copyWith so callers can explicitly
clear pinnedActivitiesByObjective while preserving the existing value when the
argument is omitted. Add an explicit clear flag or equivalent sentinel, and
apply it only to the pinned mapping without changing the other copyWith fields.
In `@lib/routes/world/joined_objective_cache.dart`:
- Around line 102-107: Keep course-space and quest identities distinct: in
lib/routes/world/joined_objective_cache.dart lines 102-107, key cached teacher
modes by the joined room/course-space ID while retaining the quest UUID as
outline input; in lib/routes/courses/own/selected_course_page.dart lines 53-57,
select the joined room by course-space identity instead of matching the first
room by quest UUID; and in lib/routes/world/world_map.dart lines 401-404,
propagate the joined room identity through the course map context so marker
restrictions use the correct course.
In `@lib/routes/world/world_map_pins_manager.dart`:
- Around line 418-450: Restrict _restrictCards evaluation to the current quest’s
learning-objective IDs by passing quest.learningObjectiveIds from the caller.
While building availableByLo and evaluating each card’s references, ignore any
LO not in that quest-scoped set so unrelated fail-open references cannot retain
a card excluded by the quest’s Mission pins.
---
Outside diff comments:
In `@lib/routes/chat/chat_details/chat_details.dart`:
- Around line 85-105: Invalidate pin-dependent projections when teacher-mode
state changes: in lib/routes/chat/chat_details/chat_details.dart lines 85-105,
reload via the ChatDetails lifecycle flow when the current room’s pin mapping
changes; in lib/routes/courses/own/selected_course_page.dart lines 65-78,
observe the matched course room’s teacher-mode state and reload its outline; in
lib/routes/world/joined_objective_cache.dart lines 108-115, include pins and
threshold values in the objective-cache fingerprint; and in
lib/routes/world/world_map.dart lines 405-409, reload course-scoped pins when
the active room’s pin mapping changes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: b66e49e4-28f0-41ca-9f57-c04834ade7bd
📒 Files selected for processing (10)
.github/instructions/quests.instructions.mdlib/features/quests/quest_objectives_loader.dartlib/features/quests/repo/quest_repo.dartlib/routes/chat/chat_details/chat_details.dartlib/routes/chat/chat_details/teacher_mode_model.dartlib/routes/courses/own/selected_course_page.dartlib/routes/world/joined_objective_cache.dartlib/routes/world/world_map.dartlib/routes/world/world_map_pins_manager.darttest/pangea/quests/quest_outline_restriction_test.dart
Comment on lines
26
to
40
| TeacherModeModel copyWith({ | ||
| bool? enabled, | ||
| int? activitiesToUnlockTopic, | ||
| int? starsToUnlockObjective, | ||
| Map<String, List<String>>? pinnedActivitiesByObjective, | ||
| }) { | ||
| return TeacherModeModel( | ||
| enabled: enabled ?? this.enabled, | ||
| activitiesToUnlockTopic: | ||
| activitiesToUnlockTopic ?? this.activitiesToUnlockTopic, | ||
| starsToUnlockObjective: | ||
| starsToUnlockObjective ?? this.starsToUnlockObjective, | ||
| pinnedActivitiesByObjective: | ||
| pinnedActivitiesByObjective ?? this.pinnedActivitiesByObjective, | ||
| ); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Allow copyWith to clear the pin mapping.
copyWith(pinnedActivitiesByObjective: null) currently preserves the old pins, although null is the contract for removing restrictions. Add an explicit clear flag or sentinel.
Proposed fix
TeacherModeModel copyWith({
bool? enabled,
int? activitiesToUnlockTopic,
int? starsToUnlockObjective,
Map<String, List<String>>? pinnedActivitiesByObjective,
+ bool clearPinnedActivitiesByObjective = false,
}) {
return TeacherModeModel(
enabled: enabled ?? this.enabled,
activitiesToUnlockTopic:
activitiesToUnlockTopic ?? this.activitiesToUnlockTopic,
starsToUnlockObjective:
starsToUnlockObjective ?? this.starsToUnlockObjective,
- pinnedActivitiesByObjective:
- pinnedActivitiesByObjective ?? this.pinnedActivitiesByObjective,
+ pinnedActivitiesByObjective: clearPinnedActivitiesByObjective
+ ? null
+ : pinnedActivitiesByObjective ?? this.pinnedActivitiesByObjective,
);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| TeacherModeModel copyWith({ | |
| bool? enabled, | |
| int? activitiesToUnlockTopic, | |
| int? starsToUnlockObjective, | |
| Map<String, List<String>>? pinnedActivitiesByObjective, | |
| }) { | |
| return TeacherModeModel( | |
| enabled: enabled ?? this.enabled, | |
| activitiesToUnlockTopic: | |
| activitiesToUnlockTopic ?? this.activitiesToUnlockTopic, | |
| starsToUnlockObjective: | |
| starsToUnlockObjective ?? this.starsToUnlockObjective, | |
| pinnedActivitiesByObjective: | |
| pinnedActivitiesByObjective ?? this.pinnedActivitiesByObjective, | |
| ); | |
| TeacherModeModel copyWith({ | |
| bool? enabled, | |
| int? activitiesToUnlockTopic, | |
| int? starsToUnlockObjective, | |
| Map<String, List<String>>? pinnedActivitiesByObjective, | |
| bool clearPinnedActivitiesByObjective = false, | |
| }) { | |
| return TeacherModeModel( | |
| enabled: enabled ?? this.enabled, | |
| activitiesToUnlockTopic: | |
| activitiesToUnlockTopic ?? this.activitiesToUnlockTopic, | |
| starsToUnlockObjective: | |
| starsToUnlockObjective ?? this.starsToUnlockObjective, | |
| pinnedActivitiesByObjective: clearPinnedActivitiesByObjective | |
| ? null | |
| : pinnedActivitiesByObjective ?? this.pinnedActivitiesByObjective, | |
| ); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@lib/routes/chat/chat_details/teacher_mode_model.dart` around lines 26 - 40,
Update TeacherModeModel.copyWith so callers can explicitly clear
pinnedActivitiesByObjective while preserving the existing value when the
argument is omitted. Add an explicit clear flag or equivalent sentinel, and
apply it only to the pinned mapping without changing the other copyWith fields.
Comment on lines
+102
to
+107
| final modes = <String, TeacherModeModel>{ | ||
| for (final room in client.joinedCourseRooms) | ||
| room.coursePlan!.uuid: | ||
| room.teacherMode.starsToUnlockObjective ?? | ||
| kDefaultStarsToUnlockObjective, | ||
| room.coursePlan!.uuid: room.teacherMode, | ||
| }; | ||
| return rebuild( | ||
| thresholds.keys.toList(), | ||
| modes.keys.toList(), |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Keep course-space identity separate from quest identity. Using coursePlan.uuid as the course key collapses or ambiguously selects rooms even though separate courses sharing a quest may have different pins.
lib/routes/world/joined_objective_cache.dart#L102-L107: key cached configurations by room/course-space ID and retain the quest UUID as outline input.lib/routes/courses/own/selected_course_page.dart#L53-L57: select the intended joined room by course-space identity rather thanfirstWhereOrNullon quest UUID.lib/routes/world/world_map.dart#L401-L404: carry the joined room identity in the course map context so marker restrictions cannot come from another course.
📍 Affects 3 files
lib/routes/world/joined_objective_cache.dart#L102-L107(this comment)lib/routes/courses/own/selected_course_page.dart#L53-L57lib/routes/world/world_map.dart#L401-L404
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@lib/routes/world/joined_objective_cache.dart` around lines 102 - 107, Keep
course-space and quest identities distinct: in
lib/routes/world/joined_objective_cache.dart lines 102-107, key cached teacher
modes by the joined room/course-space ID while retaining the quest UUID as
outline input; in lib/routes/courses/own/selected_course_page.dart lines 53-57,
select the joined room by course-space identity instead of matching the first
room by quest UUID; and in lib/routes/world/world_map.dart lines 401-404,
propagate the joined room identity through the course map context so marker
restrictions use the correct course.
Comment on lines
+418
to
+450
| _pins = _restrictCards(activityCards, pinnedActivitiesByObjective); | ||
| } | ||
|
|
||
| /// Course-scoped marker filter for per-course activity pinning (org quests | ||
| /// doc, client#7748): a card stays when at least one of its Mission refs | ||
| /// allows it — the same fail-open rule as the outline restriction, via the | ||
| /// shared [effectivePinnedActivityIds]. Null pins (not joined / unset) show | ||
| /// everything; the WORLD-scoped map is deliberately never filtered. | ||
| static List<QuestActivityCard> _restrictCards( | ||
| List<QuestActivityCard> cards, | ||
| Map<String, List<String>>? pinnedByObjective, | ||
| ) { | ||
| if (pinnedByObjective == null || pinnedByObjective.isEmpty) return cards; | ||
| final availableByLo = <String, Set<String>>{}; | ||
| for (final card in cards) { | ||
| for (final loId in card.learningObjectiveRefs) { | ||
| (availableByLo[loId] ??= <String>{}).add(card.activityId); | ||
| } | ||
| } | ||
| final allowedByLo = { | ||
| for (final entry in availableByLo.entries) | ||
| entry.key: effectivePinnedActivityIds( | ||
| entry.value, | ||
| pinnedByObjective[entry.key], | ||
| ), | ||
| }; | ||
| return cards | ||
| .where( | ||
| (card) => card.learningObjectiveRefs.any( | ||
| (loId) => allowedByLo[loId]?.contains(card.activityId) ?? true, | ||
| ), | ||
| ) | ||
| .toList(); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Restrict marker evaluation to this quest's Mission IDs.
A fetched card may reference both a quest Mission and an unrelated LO. Because availableByLo includes every reference, the unrelated LO has no pin and fail-opens; the final any then keeps a card that was pinned out of this course. Pass quest.learningObjectiveIds into _restrictCards and ignore other references.
Proposed fix
- _pins = _restrictCards(activityCards, pinnedActivitiesByObjective);
+ _pins = _restrictCards(
+ activityCards,
+ quest.learningObjectiveIds.toSet(),
+ pinnedActivitiesByObjective,
+ );
}
static List<QuestActivityCard> _restrictCards(
List<QuestActivityCard> cards,
+ Set<String> questObjectiveIds,
Map<String, List<String>>? pinnedByObjective,
) {
if (pinnedByObjective == null || pinnedByObjective.isEmpty) return cards;
final availableByLo = <String, Set<String>>{};
for (final card in cards) {
for (final loId in card.learningObjectiveRefs) {
+ if (!questObjectiveIds.contains(loId)) continue;
(availableByLo[loId] ??= <String>{}).add(card.activityId);
}
}
...
(card) => card.learningObjectiveRefs.any(
- (loId) => allowedByLo[loId]?.contains(card.activityId) ?? true,
+ (loId) =>
+ questObjectiveIds.contains(loId) &&
+ (allowedByLo[loId]?.contains(card.activityId) ?? false),
),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| _pins = _restrictCards(activityCards, pinnedActivitiesByObjective); | |
| } | |
| /// Course-scoped marker filter for per-course activity pinning (org quests | |
| /// doc, client#7748): a card stays when at least one of its Mission refs | |
| /// allows it — the same fail-open rule as the outline restriction, via the | |
| /// shared [effectivePinnedActivityIds]. Null pins (not joined / unset) show | |
| /// everything; the WORLD-scoped map is deliberately never filtered. | |
| static List<QuestActivityCard> _restrictCards( | |
| List<QuestActivityCard> cards, | |
| Map<String, List<String>>? pinnedByObjective, | |
| ) { | |
| if (pinnedByObjective == null || pinnedByObjective.isEmpty) return cards; | |
| final availableByLo = <String, Set<String>>{}; | |
| for (final card in cards) { | |
| for (final loId in card.learningObjectiveRefs) { | |
| (availableByLo[loId] ??= <String>{}).add(card.activityId); | |
| } | |
| } | |
| final allowedByLo = { | |
| for (final entry in availableByLo.entries) | |
| entry.key: effectivePinnedActivityIds( | |
| entry.value, | |
| pinnedByObjective[entry.key], | |
| ), | |
| }; | |
| return cards | |
| .where( | |
| (card) => card.learningObjectiveRefs.any( | |
| (loId) => allowedByLo[loId]?.contains(card.activityId) ?? true, | |
| ), | |
| ) | |
| .toList(); | |
| _pins = _restrictCards( | |
| activityCards, | |
| quest.learningObjectiveIds.toSet(), | |
| pinnedActivitiesByObjective, | |
| ); | |
| } | |
| /// Course-scoped marker filter for per-course activity pinning (org quests | |
| /// doc, client#7748): a card stays when at least one of its Mission refs | |
| /// allows it — the same fail-open rule as the outline restriction, via the | |
| /// shared [effectivePinnedActivityIds]. Null pins (not joined / unset) show | |
| /// everything; the WORLD-scoped map is deliberately never filtered. | |
| static List<QuestActivityCard> _restrictCards( | |
| List<QuestActivityCard> cards, | |
| Set<String> questObjectiveIds, | |
| Map<String, List<String>>? pinnedByObjective, | |
| ) { | |
| if (pinnedByObjective == null || pinnedByObjective.isEmpty) return cards; | |
| final availableByLo = <String, Set<String>>{}; | |
| for (final card in cards) { | |
| for (final loId in card.learningObjectiveRefs) { | |
| if (!questObjectiveIds.contains(loId)) continue; | |
| (availableByLo[loId] ??= <String>{}).add(card.activityId); | |
| } | |
| } | |
| final allowedByLo = { | |
| for (final entry in availableByLo.entries) | |
| entry.key: effectivePinnedActivityIds( | |
| entry.value, | |
| pinnedByObjective[entry.key], | |
| ), | |
| }; | |
| return cards | |
| .where( | |
| (card) => card.learningObjectiveRefs.any( | |
| (loId) => | |
| questObjectiveIds.contains(loId) && | |
| (allowedByLo[loId]?.contains(card.activityId) ?? false), | |
| ), | |
| ) | |
| .toList(); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@lib/routes/world/world_map_pins_manager.dart` around lines 418 - 450,
Restrict _restrictCards evaluation to the current quest’s learning-objective IDs
by passing quest.learningObjectiveIds from the caller. While building
availableByLo and evaluating each card’s references, ignore any LO not in that
quest-scoped set so unrelated fail-open references cannot retain a card excluded
by the quest’s Mission pins.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Per-course activity pinning: a per-Mission pinned-activity map on the course space's teacher-mode state now scopes, in that course's context, which activities are surfaced per Mission, which stars count toward satisfying it, and the effective-threshold clamp — plus the course-scoped map's markers. Design in the org quests doc (pangeachat/.github#263); client mechanics recorded in
quests.instructions.md.TeacherModeModel.pinnedActivitiesByObjective(new state field; absent = unrestricted, so existing courses and old clients are untouched)QuestOutline.restrictedTo— pure copy at the outline boundary; the shared quest-outline cache is never mutated, so two courses can restrict the same quest differentlyeffectivePinnedActivityIds— the single home of the fail-open rule (no pin / empty pin / all-stale pin → unrestricted; a pin can never make a Mission unsatisfiable)Why
Closes #7748. Enables the locked course configuration a fall classroom study may need by early September; teacher editing UI is deferred (pangeachat/admin-dash#30) — pins are written to room state by operators until then.
Testing
dart analyzeclean on all touched files.flutter test(fvm-pinned SDK): full suite passes — 1199 tests, including 9 new intest/pangea/quests/quest_outline_restriction_test.dart(filter/intersection/fail-open rules, cache-copy safety, filtered projection, attribution exclusion, clamp-against-pinned-set throughresolveProgression).Deploy Notes
None — client-only, backward compatible; the new state field is opt-in per course.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation