Skip to content

Conversation

@suhaibabsi-inst
Copy link
Contributor

@suhaibabsi-inst suhaibabsi-inst commented Nov 9, 2025

refs: MBL-19480
affects: All
builds: All
release note: Introduced immersive experience for video player.

Screen Recording

Simulator.Screen.Recording.-.iPhone.16.Pro.-.2025-11-12.at.05.30.43.mov

Test Plan

  • Create a page with a couple of media uploads embedded.
  • On appearance, 2 buttons should show up, "Expand" & "Open in Details View".
  • Make sure to turn on flag "RCE Studio Embed improvements" on course level in order to see "Open in Details View" button, you should also see that button be removed when that flag is off.
  • Upon tapping on one of those buttons, a modal for the immersive media player must show up. See ticket's description for the visual requirements of that screen.

Checklist

  • Follow-up e2e test ticket created
  • A11y checked
  • Tested on phone
  • Tested on tablet
  • Tested in dark mode
  • Tested in light mode
  • Approve from product

@suhaibabsi-inst suhaibabsi-inst self-assigned this Nov 9, 2025
@suhaibabsi-inst suhaibabsi-inst changed the title Feature/mbl 19480 immersive video player canvas uploads [MBL-19480][S/T/P] Immersive Video Player for Canvas Media Uploads Nov 9, 2025
@claude
Copy link

claude bot commented Nov 9, 2025

Claude Code Review

Updated: 2025-11-10

Critical Issues Found

  • ParentSubmissionViewController Protocol Extension Mismatch: ParentSubmissionViewController extends UINavigationController (not UIViewController), so the protocol extension CoreWebViewLinkDelegate where Self: UIViewController (CoreWebViewLinkDelegate.swift:45) doesn't apply. This means handleLink(_:) and routeLinksFrom default implementations are unavailable. The manual handleLink override on line 117 returns false, preventing link routing, but the protocol still requires routeLinksFrom which will cause a compile error.

  • Data Loss in FeatureFlag.save(APIFeatureFlagState): Line 66 in FeatureFlag.swift unconditionally sets flag.isEnvironmentFlag = false, overwriting any existing environment flag state. This loses information about whether a flag originated from the environment when updating via APIFeatureFlagState.

  • Race Condition in CoreWebViewController: The linkDelegate is assigned in init (line 41), but featuresContext is assigned after (line 25 as a property). If any delegate callbacks fire during initialization or before featuresContext is set, the feature flag subscription in CoreWebView may fail due to missing context.

Issues found

@inst-danger
Copy link
Contributor

inst-danger commented Nov 9, 2025

Warnings
⚠️ One or more files are below the minimum test coverage 50%
⚠️ The total test coverage is below the minimum 90%

Release Note:

Introduced immersive experience for video player.

Affected Apps: Student, Teacher, Parent

Builds: Student, Teacher, Parent

MBL-19480

Coverage New % Master % Delta
Canvas iOS 81.22% 81.06% 0.15%
Horizon/Horizon/Sources/Features/Dashboard/SkillsHighlightsWidget/Data/ProficiencyLevel.swift 0% 0% 0%
Horizon/Horizon/Sources/Features/Notebook/Common/View/HighlightWebView/HighlightWebFeature.swift 0% 0% 0%
Horizon/Horizon/Sources/Features/Notebook/Common/View/HighlightWebView/EnableZoom.swift 0% 0% 0%
Horizon/Horizon/Sources/Common/Domain/GetCoursesInteractor.swift 38.16% 38.16% 0%
Horizon/Horizon/Sources/Features/LearningObjects/Assignment/SubmissionComment/Data/CommentAttachment.swift 0% 0% 0%
Horizon/Horizon/Sources/Features/Account/Notifications/Domain/NotificationSettingsInteractor.swift 0% 0% 0%
Horizon/Horizon/Sources/Common/Utilities/Double+.swift 0% 0% 0%
Horizon/Horizon/Sources/Features/Session/SessionInteractor.swift 32% 33.33% -1.33%
Horizon/Horizon/Sources/Common/Data/HModuleStatus.swift 0% 0% 0%
Horizon/Horizon/Sources/Features/Assist/AssistFlashCard/View/AssistFlashCardViewModel.swift 0% 0% 0%

Generated by 🚫 dangerJS against b045bb9

refs: MBL-19480
affects: Student, Teacher, Parent
builds: Student, Teacher, Parent
release note: Introduced immersive experience for video player.
@claude
Copy link

claude bot commented Nov 10, 2025

Claude Code Review

Updated: 2025-11-10

APPROVED

Reviewed changes for immersive video player feature (MBL-19480). No critical issues found.

Summary:

  • New feature flag infrastructure properly implemented (GetFeatureFlagState, GetFeatureFlagStateRequest)
  • InsertStudioDetailView feature correctly injects studio detail links into media
  • Protocol implementations verified - all conform properly
  • Feature subscription pattern follows existing patterns
  • New files added (will require make sync)
  • No compilation errors detected

@claude
Copy link

claude bot commented Nov 10, 2025

Claude Code Review

Updated: 2025-11-10

Approved - No critical issues found

Summary of changes:

  • Adds immersive video player feature with feature flag support
  • Introduces new GetFeatureFlagState use case for per-context feature flags
  • Adds InsertStudioDetailView web feature to inject Studio detail view links
  • Updates CoreWebView to dynamically apply Studio features based on flag state
  • Properly handles JavaScript injection and feature cleanup on reload

Key observations:

  • Feature flag state is correctly fetched and cached
  • Web content controller scripts are properly removed/reapplied during feature toggles
  • URL query parameters are safely encoded
  • New files follow existing project patterns and SwiftLint compliance
  • Minimal API surface changes with backward compatibility maintained

@claude
Copy link

claude bot commented Nov 11, 2025

Claude Code Review

Updated: 2025-11-11

Critical Issues Found:

  • DEBUG CODE IN PRODUCTION: Print statements left in CoreWebView.swift that logs all loaded content. Must be removed before merge.

  • METHOD NAME MISMATCH: In Parent/CourseDetailsViewController.swift, calls setStudioInteractor() but the actual method is setStudioFeatures(). This will cause a crash at runtime.

  • INCONSISTENT API: Feature flag removal in removeFeatures() method clears all user scripts then re-adds them for all features - could cause order-dependent issues if features have dependencies.

❌ (issues found)

@claude
Copy link

claude bot commented Nov 11, 2025

Claude Code Review

Updated: 2025-11-11

Approved - No critical issues found

Summary of changes:

  • Adds immersive video player feature with feature flag support
  • Introduces GetFeatureFlagState use case for per-context feature flags
  • Adds InsertStudioOpenInDetailButtons web feature to inject Studio detail view links
  • Updates CoreWebView with dynamic feature flag subscription
  • Properly handles JavaScript injection and feature cleanup

Verified:

  • No print/debug statements in CoreWebView
  • Method names consistent (setStudioFeatures correctly named)
  • FeatureFlag.save() properly initializes isEnvironmentFlag for APIFeatureFlagState
  • Feature removal with reload() works correctly
  • New files follow project patterns

@claude
Copy link

claude bot commented Nov 11, 2025

Claude Code Review

Updated: 2025-11-11

Critical Issues Found:

  • Null Pointer Dereference in JavaScript (InsertStudioOpenDetailViewButton.swift:29-30): Line 29 accesses elm.nextElementSibling.nextElementSibling without null checks. If the iframe lacks two following siblings, this throws an error and fails button injection. Add null guard: check next !== null before getAttribute.

  • FeatureFlag Data Loss (FeatureFlag.swift:66): save(:APIFeatureFlagState) unconditionally sets isEnvironmentFlag = false, losing whether flags originated from environment. Inconsistent with save(:APIFeatureFlag) which preserves this value.

  • Potential Race Condition (StudioFeaturesInteractor.swift:52): studioImprovementsFlag initialized nil, set during setupFeatureFlagStore(). If updateStudioFeatures() fires via subscription before setup completes, nil flag store is used. Add guard or delay subscription.

❌ (issues found)

@inst-danger
Copy link
Contributor

inst-danger commented Nov 11, 2025

Builds

Commit: Add better name. (b045bb9)
Build Number: 893
Built At: Nov 27 18:59 CET (11/27 10:59 AM MST)

Student
Teacher
Parent

@claude
Copy link

claude bot commented Nov 11, 2025

Claude Code Review

Updated: 2025-11-11

Approved - No critical issues found

Summary:

  • Immersive video player feature properly implemented with feature flag support
  • GetFeatureFlagState use case correctly handles per-context feature flags
  • InsertStudioOpenInDetailButtons JavaScript properly includes null checks for sibling navigation
  • FeatureFlag.save(APIFeatureFlagState) correctly initializes isEnvironmentFlag (appropriate for context-specific flags)
  • StudioFeaturesInteractor properly guards against nil flag store before usage
  • Feature flag subscription and cleanup lifecycle correctly implemented
  • No debug statements or critical issues detected

Note: Danger reports below minimum test coverage for new files (expected for new feature)

@suhaibabsi-inst suhaibabsi-inst marked this pull request as ready for review November 12, 2025 02:34
@claude
Copy link

claude bot commented Nov 16, 2025

Claude Code Review

Updated: 2025-11-16

Critical Issues:

  • StudioViewController:26 - load() called BEFORE setStudioFeatures() configured, preventing feature flag store setup
  • Memory leak in CoreWebStudioFeaturesInteractor - setupFeatureFlagStore() doesn't cancel previous subscriptions before assigning new ones
  • removeScript() uses string comparison that may fail with special chars, risking dangling script references
  • SwiftUI WebView doesn't validate featuresContext, silently disables features when nil

Other Issues:

  • JavaScript injection in button labels lacks HTML escaping
  • Consistent setup pattern across 10+ controllers (good coverage)
  • Feature flag state conversion documented
  • Tests have adequate coverage

Status: Issues found

@claude
Copy link

claude bot commented Nov 17, 2025

Claude Code Review

CRITICAL COMPILATION ERROR & ISSUES FOUND

Undefined method URL.append(queryItems:) - Called in CoreWebViewStudioFeaturesInteractor lines 86, 90. Method not defined in URLExtensions. Will not compile.

String format injection vulnerability - String(format: script, iconSVG, title) can crash if localized 'Open in Detail View' text contains format specifiers (%s, %d, %@).

Race condition in state management - Feature flag subscription updates state while pending JavaScript evaluations modify shared map. No async cancellation on context reset.

Memory safety issue - Unowned WebView reference + weak self closures create crash risk if WebView deallocates during pending JS callback.

URL matching bug - hasPrefix lookup can match wrong video entries with overlapping path prefixes (e.g., /media_attachments/1/ matches /media_attachments/12/).

❌ Cannot merge - compilation error and correctness issues

@claude
Copy link

claude bot commented Nov 18, 2025

Claude Code Review

Updated: 2025-11-18

🔴 CRITICAL ISSUES FOUND

  • Compilation Error: url.append(queryItems:) method does not exist (CoreWebViewStudioFeaturesInteractor.swift:86, 90). Only appendingQueryItems() (non-mutating) exists. Build will fail.
  • Memory Leak Risk: unowned reference to webView with stored AnyCancellable subscription could create cycles. Use weak with nil checks instead.
  • Silent JavaScript Errors: Error parameter ignored in scanVideoFrames() callback. Should log errors for debugging.

BUILD STATUS

🔴 Will not compile - missing URL.append(queryItems:) method

@claude
Copy link

claude bot commented Nov 18, 2025

Claude Code Review

Updated: 2025-11-23

❌ Critical Issues Found

  1. Potential Infinite Reload Loop: removeFeatures() calls reload() (line 135), which calls studioFeaturesInteractor.refresh(), which can trigger updateStudioImprovementFeature() → removeFeatures() again, creating a recursion cycle.

  2. Missing deinit Cleanup: studioFeaturesInteractor subscription (storeSubscription) is never cancelled in CoreWebView's deinit. This can cause memory leaks and subscription callbacks after deallocation.

  3. Weak Reference Without Cleanup: CoreWebViewStudioFeaturesInteractor holds a weak reference to CoreWebView but doesn't clean up the storeSubscription when the webView is deallocated—subscription may fire on a nil webView pointer.

Recommendations

  • Guard against recursive reload calls in removeFeatures() or prevent the refresh from triggering feature updates
  • Add subscription cleanup in CoreWebView deinit: studioFeaturesInteractor.resetFeatureFlagStore(context: nil, env: .shared)
  • Verify subscription properly handles weak self capture in all callbacks

❌ Issues found

@suhaibabsi-inst suhaibabsi-inst requested a review from rh12 November 18, 2025 13:20
@claude
Copy link

claude bot commented Nov 24, 2025

Claude Code Review

Updated: 2025-11-24

Analysis Summary

Reviewed core immersive video player feature with feature flag integration (40 files changed, ~600 LOC additions).

No Critical Issues Found

Strengths:

  • Weak reference properly used in lazy init to prevent circular references
  • Feature flag state management correctly implements optional context (gracefully handles nil context)
  • Comprehensive test coverage for feature flag states (on, off, allowed_on) and URL construction
  • HTML injection properly escapes user content via escapeHTML() function
  • Memory cleanup in deinit with subscription cancellation

Safe patterns:

  • Weak self captures in closures prevent retain cycles
  • Subscription properly cancelled on context reset
  • Feature removal correctly triggers reload and cleans up scripts

@claude
Copy link

claude bot commented Nov 25, 2025

Claude Code Review

Updated: 2025-11-25

🔴 Critical Issue Found

XSS/URL Injection Vulnerability in InsertStudioOpenDetailViewButton.swift:107

  • Using escapeHTML() on href attribute values is incorrect. HTML escaping converts characters like & to &, which breaks URLs
  • Line 107: detailButton.href = escapeHTML(frameLink + linkSuffix);
  • Fix: Remove escapeHTML() from href assignment - use direct URL assignment. escapeHTML() should only be used for textContent (line 109 is correct)
  • This will cause immersive video links to fail/become unusable

⚠️ Additional Concerns

  • Memory leak risk (CoreWebViewStudioFeaturesInteractor.swift:143-151): storeSubscription AnyCancellable may not properly cancel if studioImprovementsFlagStore becomes nil while subscription is active
  • JavaScript function shadowing (InsertStudioOpenDetailViewButton.swift line 75): Using let in JavaScript instead of const could cause issues in strict mode contexts

✋ (issues found)

Copy link
Collaborator

@vargaat vargaat left a comment

Choose a reason for hiding this comment

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

I tested on Prod and encountered a few issues that we could fix before merging, see them below.

Comment on lines 54 to 55
var onScanFinished: (() -> Void)?
var onFeatureUpdate: (() -> Void)?
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is the public interface, so these should go before private property declarations.

) {
reloadObserver?.cancel()
reloadObserver = trigger?.sink {
webView.reload()
Copy link
Collaborator

Choose a reason for hiding this comment

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

There's a new warning here: Result of call to 'reload()' is unused.

public class CoreWebViewStudioFeaturesInteractor {
private static let scanFramesScript = """
function scanVideoFramesForTitles() {
const frameElements = document.querySelectorAll('iframe[data-media-id]');
Copy link
Collaborator

Choose a reason for hiding this comment

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

We are incorrectly adding the detail view for videos that don't have the Expand button. Tapping on the Open in detail view button redirect to external Safari.

Image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@vargaat

Fixed the link for detail view button for such cases.

After asking around about it, seems like we can safely assume that expand button will always show up whenrce_studio_embed_improvements is turned on. Studio team will always show the Expand button when that flag is on. Since it would be inefficient to wait for iframe content to load in order to decide whether to insert those buttons or not.

.open_details_button {
font-weight: 400;
font-size: 15px;
Copy link
Collaborator

Choose a reason for hiding this comment

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

The button doesn't grow with dynamic font sizes.

Image

@claude
Copy link

claude bot commented Nov 26, 2025

Claude Code Review

Updated: Wed Nov 26 07:37:47 UTC 2025

Critical Issues Found

  • URL Object Return Bug (InsertStudioOpenDetailViewButton:104): Function returns URL object instead of string. When assigned to href at line 139, browser will call .toString() which may not properly serialize URLSearchParams—immersive video links will be malformed or non-functional.

  • JavaScript Syntax Errors (Lines 88, 91, 96, 100, 114, 115): Uses let declarations in JavaScript string context. Should be var or const. Non-standard syntax that fails in strict mode.

  • DOMContentLoaded Race Condition (Lines 152-153): Executes function immediately then adds listener. If script runs after DOMContentLoaded already fired, listener never executes—buttons will not render on repeat page loads.

  • Incomplete API State Workaround (GetFeatureFlagState:62): Only handles allowed_on state, not allowed. May persist incorrect feature flag values for course-level contexts when API returns account fallback.

  • Silent SVG Asset Failure (Line 71): Missing/invalid UTF-8 asset silently becomes empty string—broken UI with no error logging.

✋ (issues found)

@claude
Copy link

claude bot commented Nov 26, 2025

Claude Code Review

Updated: 2025-11-26

Approved - No critical issues found.

Summary:

  • Immersive video player feature properly scoped with feature flag
  • Memory management correct: weak references to webView prevent cycles
  • StudioViewController modal presentation uses safe routing patterns
  • Feature removal properly cleans up scripts via new remove(from:) method
  • Navigation policy correctly identifies immersive view links before other handlers
  • Comprehensive unit tests verify feature flag states and URL construction
  • Proper error handling in JavaScript evaluation with RemoteLogger logging

All changes are backward compatible and follow project patterns.

mockRequest = URLRequest(url: URL(string: url)!)
mockType = type
mockSourceFrame = sourceFrame
mockSourceFrame = sourceFrame ?? MockFrameInfo(isMainFrame: true)
Copy link
Collaborator

Choose a reason for hiding this comment

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

It would be better to make the init parameter non-optional and add a default value.

@claude
Copy link

claude bot commented Nov 26, 2025

Claude Code Review

Updated: 2025-11-26

✅ Code Quality: Approved

Summary: Feature flag system refactor and Studio video player integration implemented with solid architecture and comprehensive tests.

Areas Reviewed:

  • New feature flag state management (APIFeatureFlagState, GetFeatureFlagState)
  • Studio video player UI integration and JavaScript interop
  • Weak reference patterns and memory management
  • Test coverage for new functionality

No critical bugs, crashes, or breaking changes identified. Implementation follows project patterns and maintains backward compatibility.

@claude
Copy link

claude bot commented Nov 26, 2025

Claude Code Review

Updated: 2025-11-26

Approved - No critical issues found.

Summary:

  • Immersive video player feature properly scoped with feature flag support
  • Memory management: weak references prevent circular reference leaks
  • JavaScript injection safely escapes user content via jsString() and escapeHTML()
  • Feature flag state management correctly handles optional context (nil safety)
  • StudioViewController modal presentation uses safe routing patterns
  • Comprehensive unit tests verify feature flag states, URL construction, and button injection
  • Feature removal properly cleans up scripts via remove(from:) lifecycle
  • Navigation policy correctly identifies immersive view links before other handlers
  • No debug/print statements or backwards-compatibility hacks present

All changes follow project patterns and maintain backward compatibility.

@claude
Copy link

claude bot commented Nov 27, 2025

Claude Code Review

Updated: 2025-11-27

Approved ✅

Key Findings:

  • Proper memory management with weak self in Combine subscriptions
  • HTML escaping implemented correctly to prevent XSS
  • Feature flag API integration follows existing patterns
  • Tests verify insertion and CSS font sizing

Minor Notes:

  • JavaScript uses let (lines 88, 96, 137, 145, 147, 152) instead of var - works in modern WKWebView contexts but consider var for maximum compatibility
  • Feature flag state handling correctly manages account-level fallback behavior
  • No breaking changes detected

No critical issues found.

@claude
Copy link

claude bot commented Nov 27, 2025

Claude Code Review

Updated: $(date)

❌ Critical Issues Found

  • JavaScript Syntax Errors (lines 88, 91, 96, 100, 114-115): Uses let keyword which fails in strict mode. Change to var or const.

  • URL Type Mismatch (line 139): frameLink is URL object but assigned to href attribute. Explicitly convert to string.

  • Null Pointer in DOM Navigation (lines 114-116): nextSibling?.nextElementSibling accesses getAttribute on potentially null object. Add null guard.

  • Infinite Reload Loop Risk: removeFeatures() calls reload → refresh → updateStudioImprovementFeature → removeFeatures. Add recursion guard.

  • Weak Reference Crash Risk: Weak self subscription may fire after webView deallocation. Need proper lifecycle management.

✋ Issues found

Copy link
Contributor

@rh12 rh12 left a comment

Choose a reason for hiding this comment

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

Code + 1

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