Merge Release teacher to master #3449
Conversation
Release Teacher 2.3.0 (85)
refs: MBL-19580 affects: Student, Teacher release note: none
# Conflicts: # apps/teacher/src/androidTest/java/com/instructure/teacher/ui/e2e/compose/SpeedGraderE2ETest.kt
🧪 Unit Test Results📊 Summary
Last updated: Tue, 16 Dec 2025 15:57:55 GMT |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Review Summary
This PR removes the RouteUtils.getMediaUri() async function and simplifies media URL handling across the codebase. The changes aim to eliminate unnecessary network calls for URL resolution and move DASH format detection to ExoPlayer error handling.
Positive Changes
✅ Simplified architecture - Removes async complexity from URL handling in fragments/activities
✅ Better separation of concerns - DASH detection moved to ExoPlayerHelper where it belongs
✅ Clever retry mechanism - ExoPlayerHelper now automatically retries with .mpd extension on UnrecognizedInputFormatException
✅ Consistent refactoring - Changes applied consistently across Student, Teacher, and Parent apps
✅ Reduced dependencies - Removes kotlinx.coroutines imports where no longer needed
✅ Improved file caching - OpenMediaAsyncTaskLoader now checks cache before downloading
Issues Found
- BaseRouterActivity.kt:319 - URL validation may miss edge cases (query parameters, redirects)
- OpenMediaAsyncTaskLoader.kt:159 - Intent may be set with null MIME type for remote URLs
- OpenMediaAsyncTaskLoader.kt:212 - Missing validation after download (disk full scenarios)
- OpenMediaAsyncTaskLoader.kt:469 - File missing trailing newline
Security & Performance
🔒 Security: No new vulnerabilities introduced. Cookie handling and authentication remain unchanged.
⚡ Performance: Should improve performance by eliminating unnecessary HEAD requests, though may add latency on DASH video failures (requires retry).
Testing Recommendations
Given the scope of changes to media playback, I recommend testing:
- Video playback with
.mp4,.m3u8, and.mpdURLs - DASH videos without
.mpdextension (retry mechanism) - URLs with query parameters and redirects
- File downloads with various MIME types
- Cached file handling
- Network error scenarios
Code Quality
The refactoring follows Kotlin best practices and project conventions. The code is more maintainable with the consolidated downloadWithHeaders() function. Consider addressing the inline comments to ensure robust error handling.
| private suspend fun shouldOpenInternally(url: String): Boolean { | ||
| val mediaUrl = RouteUtils.getMediaUri(Uri.parse(url)).toString() | ||
| return (mediaUrl.endsWith(".mpd") || mediaUrl.endsWith(".m3u8") || mediaUrl.endsWith(".mp4")) | ||
| return (url.endsWith(".mpd") || url.endsWith(".m3u8") || url.endsWith(".mp4")) |
There was a problem hiding this comment.
Potential Bug: Incomplete URL validation
The previous implementation used RouteUtils.getMediaUri() which would follow redirects and handle URL transformations. Now you're checking file extensions directly on the raw URL string, which may miss cases where:
- URLs have query parameters (e.g.,
video.mp4?token=abc) - URLs redirect to different formats
- URLs don't have extensions but serve video content
Consider using Uri.parse(url).lastPathSegment to extract the filename before checking extensions, or better yet, keep the media type detection logic but optimize the implementation.
| if (url.isNotBlank()) { | ||
| // For remote URLs, we'll get the URI and metadata during or after download | ||
| val uri = Uri.parse(url) | ||
| intent.setDataAndType(uri, mimeType) |
There was a problem hiding this comment.
Logic Error: Early return on remote URLs
This logic has changed significantly. Previously, attemptConnection() would validate the connection and get metadata (mimeType, filename) from headers before downloading. Now you're setting the intent with potentially null/empty mimeType and then calling attemptDownloadFile().
This could cause issues if mimeType is null when setting the intent data type. The Android system may not know how to handle the intent properly without a valid MIME type.
Suggestion: You should call attemptDownloadFile() first to get the metadata, then set up the intent with the correct MIME type.
| mimeType = result.mimeType | ||
| } | ||
| if (!filename.isValid() && result.filename != null) { | ||
| filename = result.filename |
There was a problem hiding this comment.
Missing error handling: Download could fail silently
In the new downloadWithHeaders() implementation, if the download completes but the file write fails (disk full, permissions, etc.), you're not handling the error properly. The original writeAttachmentsDirectoryFromURL() returned a boolean to indicate success/failure.
Consider adding validation after the download:
if (!toWriteTo.exists() || toWriteTo.length() == 0) {
throw IOException("Download failed or file is empty")
}| @Throws(IOException::class) | ||
| @Suppress("BooleanLiteralArgument") | ||
| private fun writeAttachmentsDirectoryFromURL(url: String, toWriteTo: File): Boolean { | ||
| private fun downloadWithHeaders(context: Context, url: String, targetFile: File?): DownloadResult { |
There was a problem hiding this comment.
Good approach: Automatic retry for DASH videos
The automatic retry mechanism with .mpd extension in ExoPlayerHelper is a clever solution. However, this comment is for the OpenMediaAsyncTaskLoader refactoring.
The new downloadWithHeaders() function consolidates the download logic nicely, but the flow for remote URLs (line 156-162) needs attention - see my other comment about setting intent data type with potentially null mimeType.
📊 Code Coverage Report✅ Student
✅ Teacher
|
No description provided.