Defer audio example resolution out of practice selection (#7702)#7708
Merged
Conversation
📝 WalkthroughWalkthroughPractice documentation and audio practice generation now use local target selection followed by deferred content generation. Audio examples are resolved during generation, with lemma-meaning fallback when resolution fails, while selection tests cover metadata eligibility and unresolved audio targets. ChangesAudio Practice Flow
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant PracticeSessionController
participant VocabAudioTargetGenerator
participant VocabAudioPracticeExerciseGenerator
participant MatrixState
participant ExampleMessageUtil
PracticeSessionController->>VocabAudioTargetGenerator: select local audio targets
VocabAudioTargetGenerator-->>PracticeSessionController: return lemma targets
PracticeSessionController->>VocabAudioPracticeExerciseGenerator: generate exercise
VocabAudioPracticeExerciseGenerator->>MatrixState: fetch construct uses
MatrixState-->>VocabAudioPracticeExerciseGenerator: return analytics uses
VocabAudioPracticeExerciseGenerator->>ExampleMessageUtil: resolve audio example
ExampleMessageUtil-->>VocabAudioPracticeExerciseGenerator: return audio example or failure
alt audio example unavailable
VocabAudioPracticeExerciseGenerator-->>PracticeSessionController: generate lemma meaning exercise
else audio example available
VocabAudioPracticeExerciseGenerator-->>PracticeSessionController: generate audio exercise
end
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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: 2
🧹 Nitpick comments (1)
test/pangea/analytics_practice/vocab_audio_target_selection_test.dart (1)
62-70: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover uses with only one metadata identifier.
Add cases for
eventId-only androomId-only uses. The current all-null case would still pass if eligibility accidentally changed from&&to||.As per path instructions, tests should “verify the explicit selection/loading contract.”
Proposed coverage
test('skips a lemma whose uses point at no message (no eventId/roomId), so ' 'an audio example could never resolve', () async { - final constructs = [ - _construct('perro', [_use(lemma: 'perro')]), - ]; - - final targets = await VocabAudioTargetGenerator.get(constructs); - expect(targets, isEmpty); + for (final use in [ + _use(lemma: 'perro'), + _use(lemma: 'perro', eventId: r'$evt1'), + _use(lemma: 'perro', roomId: '!room1'), + ]) { + final targets = await VocabAudioTargetGenerator.get([ + _construct('perro', [use]), + ]); + expect(targets, isEmpty); + } });🤖 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 `@test/pangea/analytics_practice/vocab_audio_target_selection_test.dart` around lines 62 - 70, Add test cases alongside the existing no-metadata test for uses containing only an eventId and only a roomId, and assert each produces no audio targets. Use the existing construct/use helpers and keep the assertions focused on the explicit selection/loading eligibility contract, ensuring both metadata identifiers are required.Source: Path instructions
🤖 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 @.github/instructions/practice-exercises.instructions.md:
- Around line 138-139: Update the Activity Generation section around the
remaining old references to use the new lib/routes paths and the
MessagePracticeExerciseRequest and PracticeExerciseModel contract, matching the
flow documented in the numbered steps. Remove all references to the obsolete
lib/pangea paths, MessageActivityRequest, and PracticeActivityModel so the
section is consistent.
In
`@lib/routes/analytics/construct_analytics/practice/vocab_audio_practice_exercise_generator.dart`:
- Around line 23-26: Update the audio-example resolution flow in get so expected
failures from getConstructUses or getAudioExampleMessage are caught, logged, and
converted to null before the existing meaning fallback runs. Ensure
_asMeaningRequest is invoked for both null results and resolver exceptions,
while preserving successful audio generation; add coverage for a throwing
resolver.
---
Nitpick comments:
In `@test/pangea/analytics_practice/vocab_audio_target_selection_test.dart`:
- Around line 62-70: Add test cases alongside the existing no-metadata test for
uses containing only an eventId and only a roomId, and assert each produces no
audio targets. Use the existing construct/use helpers and keep the assertions
focused on the explicit selection/loading eligibility contract, ensuring both
metadata identifiers are required.
🪄 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: 1d8f62e8-c7e8-4979-98d4-b83a2d7a8d1f
📒 Files selected for processing (4)
.github/instructions/practice-exercises.instructions.mdlib/routes/analytics/construct_analytics/practice/vocab_audio_practice_exercise_generator.dartlib/routes/analytics/construct_analytics/practice/vocab_audio_target_generator.darttest/pangea/analytics_practice/vocab_audio_target_selection_test.dart
Comment on lines
+138
to
+139
| 3. For each target, a [`MessagePracticeExerciseRequest`](../../lib/routes/chat/toolbar/practice_exercises/message_practice_exercise_request.dart) is routed by [`PracticeRepo`](../../lib/routes/chat/toolbar/practice_exercises/practice_generation_repo.dart) to the appropriate generator. | ||
| 4. The generator returns a [`PracticeExerciseModel`](../../lib/routes/chat/toolbar/practice_exercises/practice_exercise_model.dart) subclass with choices and answers. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Finish updating the Activity Generation section to the new contract.
These lines document the new routes and MessagePracticeExerciseRequest/PracticeExerciseModel, but Lines 197-210 still reference the old lib/pangea/ paths, MessageActivityRequest, and PracticeActivityModel. Update that section to avoid conflicting flow documentation.
🤖 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 @.github/instructions/practice-exercises.instructions.md around lines 138 -
139, Update the Activity Generation section around the remaining old references
to use the new lib/routes paths and the MessagePracticeExerciseRequest and
PracticeExerciseModel contract, matching the flow documented in the numbered
steps. Remove all references to the obsolete lib/pangea paths,
MessageActivityRequest, and PracticeActivityModel so the section is consistent.
Comment on lines
+23
to
+26
| final audioExample = | ||
| req.audioExampleMessage ?? await _resolveAudioExample(req); | ||
| if (audioExample == null) { | ||
| return VocabMeaningPracticeExerciseGenerator.get(_asMeaningRequest(req)); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Make resolver errors trigger the meaning fallback.
The fallback handles only null. An exception from getConstructUses or getAudioExampleMessage escapes get, aborting generation despite the documented “never leaves a gap” contract. Convert expected lookup/event-resolution failures to null with logging, then invoke _asMeaningRequest; cover a throwing resolver in tests.
Also applies to: 102-115
🤖 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/analytics/construct_analytics/practice/vocab_audio_practice_exercise_generator.dart`
around lines 23 - 26, Update the audio-example resolution flow in get so
expected failures from getConstructUses or getAudioExampleMessage are caught,
logged, and converted to null before the existing meaning fallback runs. Ensure
_asMeaningRequest is invoked for both null results and resolver exceptions,
while preserving successful audio generation; add coverage for a throwing
resolver.
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.
Closes #7702.
Problem
Standalone practice loaded slowly and inconsistently, even on fresh accounts. Root cause:
VocabAudioTargetGeneratorresolved an example message per candidate lemma — a Matrix event fetch that can hit the server — serially, during target selection, before the first exercise could render. How many candidates it walked before finding enough audio-capable examples varied, hence the inconsistency. Exercise content was already eager-prefetched; only selection blocked.Fix
VocabAudioPracticeExerciseGenerator), inside the existing eager background queue — every exercise is still proactively loaded, it just no longer gates first paint.lemmaMeaningexercise for the same lemma, so a deferred failure never leaves a gap.Docs
practice-exercises.instructions.mdwas stale (alllib/pangea/...paths dead from a move) and silent on the load sequence. Fixed the paths, added a Loading & Generation Sequencing section, and split the "never blocks on network" contract into message vs standalone. The standalone contract now states: selection stays cheap and resolves no example messages/audio/events; the first exercise gates the UI, the rest are prefetched eagerly in the background.Testing
vocab_audio_target_selection_test.dart) proves selection needs noMatrixState(the front-load is gone) and skips lemmas with no message-bearing use.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Documentation