-
Notifications
You must be signed in to change notification settings - Fork 108
[MBL-19580][Student][Teacher] File download logic update #3438
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[MBL-19580][Student][Teacher] File download logic update #3438
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Summary
This PR removes the RouteUtils.getMediaUri() method and its associated async media URL resolution logic, simplifying media handling by using URLs directly instead of making HEAD requests to resolve redirects and detect DASH content types.
Positive Changes
✅ Significant complexity reduction - Removes unnecessary async/coroutine wrappers and HEAD requests for every media URL
✅ Better performance - Eliminates network round-trips before playing media
✅ Cleaner architecture - Removes the complex redirect following logic from RouteUtils
✅ Smart fallback handling - Adds .mpd retry logic in ExoPlayerHelper for DASH videos without proper extensions
Issues Found
- Thread safety concerns in
OpenMediaAsyncTaskLoader- Instance variables are modified during async operations (libs/pandautils/src/main/java/com/instructure/pandautils/loaders/OpenMediaAsyncTaskLoader.kt:301) - Null pointer risk - Non-null assertion on response body without null check (libs/pandautils/src/main/java/com/instructure/pandautils/loaders/OpenMediaAsyncTaskLoader.kt:341)
- State management issue in
ExoPlayerHelper-currentUriis permanently modified during retry attempts (libs/pandautils/src/main/java/com/instructure/pandautils/utils/ExoPlayerHelper.kt:191) - Potential regression in URL handling - Direct string comparison may fail with query parameters (apps/student/src/main/java/com/instructure/student/activity/BaseRouterActivity.kt:319)
- Cache key stability - Hash-based filename generation could cause cache misses (libs/pandautils/src/main/java/com/instructure/pandautils/loaders/OpenMediaAsyncTaskLoader.kt:170)
- MimeType state pollution - Instance variable reuse could cause incorrect mimeType associations (libs/pandautils/src/main/java/com/instructure/pandautils/loaders/OpenMediaAsyncTaskLoader.kt:333)
Overall Assessment
The core idea of this refactoring is sound - removing unnecessary network requests and complexity. However, there are several implementation issues around state management, error handling, and edge cases that should be addressed before merging. The inline comments provide specific suggestions for each issue.
Testing Recommendations
Please verify:
- Media playback with query parameters in URLs (e.g.,
video.mp4?token=xyz&expires=123) - DASH videos without
.mpdextensions retry correctly and don't cause issues on subsequent playback attempts - Concurrent media file downloads don't interfere with each other
- Cached files are properly reused across app sessions
libs/pandautils/src/main/java/com/instructure/pandautils/loaders/OpenMediaAsyncTaskLoader.kt
Show resolved
Hide resolved
libs/pandautils/src/main/java/com/instructure/pandautils/loaders/OpenMediaAsyncTaskLoader.kt
Show resolved
Hide resolved
libs/pandautils/src/main/java/com/instructure/pandautils/loaders/OpenMediaAsyncTaskLoader.kt
Show resolved
Hide resolved
libs/pandautils/src/main/java/com/instructure/pandautils/loaders/OpenMediaAsyncTaskLoader.kt
Show resolved
Hide resolved
libs/pandautils/src/main/java/com/instructure/pandautils/utils/ExoPlayerHelper.kt
Show resolved
Hide resolved
apps/student/src/main/java/com/instructure/student/activity/BaseRouterActivity.kt
Show resolved
Hide resolved
🧪 Unit Test Results✅ 📱 Student App
✅ 📱 Teacher App
✅ 🌅 Horizon
✅ 📦 Submodules
📊 Summary
Last updated: Wed, 10 Dec 2025 12:40:07 GMT |
adamNagy56
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
QA +1
Test plan:
refs: MBL-19580
affects: Student, Teacher
release note: none
Checklist