Skip to content

Conversation

@tamaskozmer
Copy link
Contributor

@tamaskozmer tamaskozmer commented Dec 9, 2025

Test plan:

  • Test all the failing scenarios for file downloads (we have the list on Slack).
  • Smoke test all the media playback features

refs: MBL-19580
affects: Student, Teacher
release note: none

Checklist

  • Follow-up e2e test ticket created or not needed
  • Run E2E test suite
  • Tested in light mode

Copy link

@claude claude bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR Review Summary

This PR simplifies media URI handling by removing the RouteUtils.getMediaUri() function and its associated network HEAD request logic. The changes move responsibility for following redirects and determining media types into the download flow rather than pre-checking URLs.

Positive Changes

  • Code simplification: Removes async complexity from multiple video player initialization flows, making the code easier to follow
  • Reduced network calls: Eliminates unnecessary HEAD requests before video playback
  • Better error handling: Moves DASH format detection into ExoPlayer's error handler with automatic .mpd retry
  • Cleaner architecture: Consolidates download logic into OpenMediaAsyncTaskLoader.downloadWithHeaders()

Issues Found

  • Resource leak risk in OpenMediaAsyncTaskLoader.kt:329 - Error handling during file download could be more robust
  • State inconsistency in OpenMediaAsyncTaskLoader.kt:197 - Instance variables may be left inconsistent if download fails
  • Cache validation in OpenMediaAsyncTaskLoader.kt:316 - Cached files are only checked for existence/size, not corruption
  • Retry logic in ExoPlayerHelper.kt:180 - Player state may be inconsistent if second retry also fails
  • Missing logging in ExoPlayerHelper.kt:175 - New retry mechanism should have logging for debugging
  • URL detection limitation in BaseRouterActivity.kt:319 - Simple extension check won't handle redirects or obfuscated URLs
  • Missing newline at end of OpenMediaAsyncTaskLoader.kt:469

Architecture & Performance Considerations

Performance: This change should improve performance by eliminating HEAD requests. However, it shifts complexity to the download phase where errors are discovered later.

Backwards compatibility: The removal of redirect-following in shouldOpenInternally() may cause some videos to not open internally that previously would have. URLs that redirect to .mpd/.m3u8/.mp4 files will no longer be detected.

Testing needs: The ExoPlayer retry logic and new download flow would benefit from additional testing, especially around edge cases like network failures, corrupted caches, and DASH format detection.

Overall Assessment

This is a good refactoring that simplifies the codebase and removes unnecessary network overhead. The inline comments highlight areas where additional error handling or validation could make the implementation more robust. The changes follow Kotlin best practices and maintain consistency with the existing codebase architecture.

@github-actions
Copy link

github-actions bot commented Dec 9, 2025

🧪 Unit Test Results

✅ 📱 Student App

  • Tests: 1226 total, 0 failed, 0 skipped
  • Duration: 0.000s
  • Success Rate: 100%

✅ 📱 Teacher App

  • Tests: 364 total, 0 failed, 0 skipped
  • Duration: 33.491s
  • Success Rate: 100%

✅ 🌅 Horizon

  • Tests: 449 total, 0 failed, 0 skipped
  • Duration: 33.373s
  • Success Rate: 100%

✅ 📦 Submodules

  • Tests: 2414 total, 0 failed, 0 skipped
  • Duration: 52.970s
  • Success Rate: 100%

📊 Summary

  • Total Tests: 4453
  • Failed: 0
  • Skipped: 0
  • Status: ✅ All tests passed!

Last updated: Wed, 10 Dec 2025 09:01:50 GMT

@github-actions
Copy link

github-actions bot commented Dec 9, 2025

Teacher Install Page

@github-actions
Copy link

github-actions bot commented Dec 9, 2025

Student Install Page

Copy link
Contributor

@adamNagy56 adamNagy56 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Student/Teacher: The mp4 files among the inbox attachments still don’t play, I’m getting the same error as before.
  • Student/Teacher: The mp4 files in announcement and discussion attachments still aren’t working, I’m getting the same error as before.
  • Teacher: The newly submitted File-upload submissions still can’t play mp3 or mp4 files, I get a “This media format is not supported” error. This also applies to comment attachments as well. (See attached screenshot.)

Screenshot_20251209_191804_Canvas Teacher

Copy link
Contributor

@adamNagy56 adamNagy56 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

QA +1

@tamaskozmer tamaskozmer merged commit e04fa9a into release/student-8.4.1-284 Dec 10, 2025
42 of 47 checks passed
@tamaskozmer tamaskozmer deleted the MBL-19580-file-download-logic-update branch December 10, 2025 12:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants