[MBL-19552] Add Studio embed immersive view support #3406
Conversation
Implements support for Studio embed videos to open in immersive view with "Open in Detail View" buttons. This provides better video viewing experience for Studio content embedded in pages and assignments. Changes: - Detect Studio embed URLs (thumbnail_embed and learn_embed types) - Detect Studio media URLs (media_attachments_iframe) - Transform embed URLs to immersive view format with required parameters - Add "Open in Detail View" buttons below Studio iframes - Route immersive view URLs to InternalWebviewFragment without LTI auth - Extract and pass video titles to immersive view Known limitation: - Buttons inside iframes cannot be intercepted due to WebView cross-origin isolation. Users must use the "Open in Detail View" button below the iframe. - Working with web team on postMessage solution for better UX. Test plan: - Verify Studio embed iframes show "Open in Detail View" button - Verify button navigates to immersive view in InternalWebviewFragment - Verify video title displays correctly in immersive view - Verify standard media uploads continue to work with Expand button 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
📊 Code Coverage Report
|
|
Student Install Page |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Review - Update
I've reviewed the latest changes to this PR. Here's my assessment:
What Changed
This update includes improvements to handle Studio embed URLs in both the RouteMatcher and HtmlContentFormatter:
- RouteMatcher.kt: Added early detection logic for Studio embed immersive view URLs to prevent them from being caught by the generic LTI route matcher
- HtmlContentFormatter.kt:
- Restructured iframe handling logic to check for Studio embeds before generic external tools
- Added new
convertStudioEmbedToImmersiveView()function for URL transformation - Fixed HTML entity escaping in generated buttons (security improvement)
- Added
hasStudioEmbedUrl()helper function
Issues Found
🔴 Must Fix
- Debug logging in production code (RouteMatcher.kt:564-581): Multiple
Logger.e()calls should be removed or changed to debug level - Null-safety concern (RouteMatcher.kt:571): Force-unwrap operator
!!used after insufficient null checks - Missing error handling (HtmlContentFormatter.kt:219): URL decoding operations could fail and should be wrapped in try-catch
🟡 Should Improve
- Performance (RouteMatcher.kt:564-567): Repeated
toString()calls - extract to variable - Code complexity (HtmlContentFormatter.kt:219-276): Complex URL manipulation logic could benefit from simplification and better error handling
❓ Questions
- Test coverage: Are there unit tests for the new
hasStudioEmbedUrl()pattern matching with various encoding scenarios?
Positive Changes
✅ HTML entity escaping fix prevents potential XSS issues
✅ Improved control flow logic prevents routing conflicts
✅ Good separation of concerns between Studio embed types
Please address the must-fix issues before merging.
| val title = uri.getQueryParameter("title") ?: activity.getString(R.string.immersiveView) | ||
| route.arguments.putString(Const.ACTION_BAR_TITLE, title) | ||
|
|
||
| Logger.e("RouteMatcher - Routing to InternalWebviewFragment with URL: $urlString") |
There was a problem hiding this comment.
Issue: Debug logging left in production code
Multiple Logger.e() calls are present which should be removed or changed to debug level:
- Line 564:
Logger.e("RouteMatcher - Checking route: ${route?.uri?.toString()}") - Line 568:
Logger.e("RouteMatcher - Detected Studio embed immersive view URL in route()") - Line 581:
Logger.e("RouteMatcher - Routing to InternalWebviewFragment with URL: $urlString")
These appear to be debugging statements that should either be removed or changed to Logger.d() (debug level) before merging to production. Error-level logging should be reserved for actual errors.
| Logger.e("RouteMatcher - Checking route: ${route?.uri?.toString()}") | ||
| if (route?.uri?.toString()?.contains("external_tools/retrieve") == true && | ||
| route.uri?.toString()?.contains("custom_arc_launch_type") == true && | ||
| route.uri?.toString()?.contains("immersive_view") == true) { |
There was a problem hiding this comment.
Issue: Inefficient repeated string conversions
The code calls route?.uri?.toString() multiple times (lines 564, 566, 567) which is inefficient. Consider extracting this to a variable:
val uriString = route?.uri?.toString()
if (uriString?.contains("external_tools/retrieve") == true &&
uriString.contains("custom_arc_launch_type") &&
uriString.contains("immersive_view")) {This improves both performance and readability.
| Logger.e("RouteMatcher - Detected Studio embed immersive view URL in route()") | ||
| // Handle Studio embed immersive view - pass the full URL and title to InternalWebviewFragment | ||
| val uri = route.uri!! | ||
| val urlString = uri.toString() |
There was a problem hiding this comment.
Issue: Potential null pointer exception
On line 571, route.uri is force-unwrapped with !! after only checking that the string representation contains certain values. However, the null-safety checks on lines 564-567 use ?.toString() which could theoretically be non-null string "null" if the URI itself is null.
While unlikely, the safer approach would be:
val uri = route.uri ?: return
val urlString = uri.toString()This ensures uri is non-null before proceeding and avoids the force-unwrap operator.
|
|
||
| private fun convertStudioEmbedToImmersiveView(srcUrl: String, title: String?): String { | ||
| // Normalize HTML entities before processing | ||
| val normalizedUrl = srcUrl.replace("&", "&") |
There was a problem hiding this comment.
Issue: Missing null-safety check in URL normalization
On line 221, the code normalizes the URL by replacing & with &, but there's no null check before the replace operation. While srcUrl is checked earlier, it's better to be explicit:
val normalizedUrl = srcUrl.replace("&", "&")This is actually safe since srcUrl is guaranteed non-null by the regex match, but the pattern should be consistent with line 283 where you do check for null.
| private fun iframeWithStudioButton(immersiveUrl: String, iframe: String, context: Context): String { | ||
| val buttonText = context.getString(R.string.openInDetailView) | ||
| val htmlButton = "</br><p><div class=\"lti_button\" onClick=\"location.href='$immersiveUrl'\">$buttonText</div></p>" | ||
| val escapedUrl = immersiveUrl.replace("&", "&") |
There was a problem hiding this comment.
Good: HTML entity escaping fix
This is an important security fix! Properly escaping the & characters in the URL prevents potential XSS issues when the HTML is rendered. The change from location.href='$immersiveUrl' to using $escapedUrl with proper entity encoding is the correct approach.
| return newHTML | ||
| } | ||
|
|
||
| private fun convertStudioEmbedToImmersiveView(srcUrl: String, title: String?): String { |
There was a problem hiding this comment.
Issue: Complex URL manipulation could be simplified
The convertStudioEmbedToImmersiveView() function performs multiple string replacements and encoding/decoding operations. Consider:
- Error handling: What happens if URL decoding fails? Should wrap in try-catch
- URL encoding consistency: Using both
java.net.URLDecoderandURLEncoder- ensure charset is consistent - String building: Consider using
StringBuilderorbuildString {}for constructing the final URL
Example improvement:
private fun convertStudioEmbedToImmersiveView(srcUrl: String, title: String?): String {
try {
val normalizedUrl = srcUrl.replace("&", "&")
// ... rest of logic
} catch (e: Exception) {
Logger.e("Failed to convert Studio embed URL: ${e.message}")
return srcUrl // Return original on failure
}
}| // Handle the LTI case | ||
| // Check Studio embed URLs first (before generic external_tools check) | ||
| // because Studio embeds also contain "external_tools" in their URL | ||
| if (hasStudioEmbedUrl(srcUrl)) { |
There was a problem hiding this comment.
Good: Improved control flow logic
The restructured if-else chain is much better! Checking for Studio embed URLs before the generic hasExternalTools() check prevents the wrong handler from being applied. This fix addresses the routing issue mentioned in the PR description where Studio embeds were being incorrectly caught by the LTI route matcher.
| fun hasKalturaUrl(text: String?) = text?.contains("kaltura.com").orDefault() | ||
| fun hasExternalTools(text: String?) = text?.contains("external_tools").orDefault() | ||
| fun hasStudioMediaUrl(text: String?) = text?.contains("media_attachments_iframe").orDefault() | ||
| fun hasStudioEmbedUrl(text: String?): Boolean { |
There was a problem hiding this comment.
Question: Test coverage for new URL patterns
The new hasStudioEmbedUrl() function checks for encoded URL parameters (custom_arc_launch_type%3Dthumbnail_embed). Are there unit tests covering:
- URLs with
&entities vs plain& - Different encoding scenarios (single vs double encoded)
- Edge cases where these parameters might appear in different positions
This is complex string matching logic that would benefit from comprehensive test coverage.
🧪 Unit Test Results✅ 📱 Student App
✅ 🌅 Horizon
✅ 📦 Submodules
📊 Summary
Last updated: Mon, 12 Jan 2026 15:28:53 GMT |
Implements support for Studio embed videos to open in immersive view with "Open in Detail View" buttons. This provides better video viewing experience for Studio content embedded in pages and assignments.
Changes:
Known limitation:
Test plan:
refs: MBL-19552
affects: Student
release note: none
🤖 Generated with Claude Code