[MBL-19393][S/T] RCE - Fixed issue where media text entry submitted via RCE are not playable when viewed later

[suhaibabsi-inst]

refs: MBL-19393
affects: Student, Teacher
builds: Student, Teacher
release note: Fixed issue where media text entry submissions using RCE are not being playable when viewed later.

What changed?

The fix was basically at pass uploaded media attachmentID to RCE, so we can use that with updated URL of media iframe:
/media_attachments_iframe/${attachmentID}

Rather than,
/media_objects_iframe/${mediaID}

Test Plan

See ticket's description for test accounts.
Make sure to follow Steps to reproduce shown in the ticket.

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

[claude]

Pull Request Review: Media Text Entry Submission Fix

Summary

This PR fixes an issue where media uploaded via RCE in text entry submissions were not playable when viewed later. The solution switches from using media_objects_iframe to media_attachments_iframe by passing the attachmentID alongside the mediaID.

---

Code Quality ✅

Strengths

1. Clean Architecture: The introduction of MediaEntry struct provides a clear, type-safe container for both IDs
2. Consistent Naming: Uses Swift naming conventions appropriately (camelCase for properties)
3. Proper Optional Handling: attachmentID is correctly marked as optional throughout the chain
4. Minimal Scope: Changes are focused and do not introduce unnecessary modifications

Swift Best Practices ✅

• Proper use of flatMap for optional transformation in UploadMedia.swift:128-133
• Correct Core Data attribute addition with optional marking
• Good use of struct for value types (MediaEntry)

---

Potential Issues & Concerns

1. Critical: Missing Nil Check in JavaScript ⚠️

Location: RichContentEditor.js:182

Issue: When rce_enhancements feature flag is enabled, attachmentID could be undefined for:

• Old media entries uploaded before this change
• Media uploaded without a context (line 124 in UploadMedia.swift)
• API response where attachment_id is null

Impact: This would generate invalid URLs like /media_attachments_iframe/undefined

Recommendation: Add a fallback check to handle cases where attachmentID is undefined or null, falling back to the old media_objects_iframe URL pattern.

2. Core Data Migration Consideration 📊

Location: Database.xcdatamodel/contents

Assessment: ✅ This is safe since the attribute is optional and Core Data will auto-migrate by adding the field with null values.

However, consider:

• Are there any existing media entries that need backfilling?
• Should there be a data migration strategy for old content?

3. Test Coverage Gap ⚠️

Location: Core/CoreTests/Features/AudioVideo/UploadMediaTests.swift

Issue: The test file still uses the old callback signature expecting a String instead of MediaEntry:

• Line 33: upload.callback = { [weak self] _, error in (only expecting String)
• Lines 89-102: Tests reference mediaID as a String instead of MediaEntry
• Lines 94-95: var mediaID: String? should be var entry: MediaEntry?

Required Test Updates:

1. testCompleteUploadSuccess - Update to verify both mediaID and attachmentID
2. testCompleteUploadError - Update callback signature
3. testCompleteUploadNoContext - Update to verify MediaEntry with nil attachmentID

4. API Response Consistency 🔍

Location: APIMediaComment.swift:210-211

The API response structure marks attachment_id as optional. Question: Is this always present in the actual API response when using RCE enhancements?

---

Security Considerations ✅

No security concerns identified:

• No SQL injection risks (Core Data handles parameterization)
• No XSS concerns (data attributes are properly set, not injected as HTML)
• URLs are server-relative paths, not user-controlled

---

Performance Considerations ✅

• No performance regressions expected
• Adding one optional field to Core Data has negligible impact
• JavaScript changes are minimal and do not affect rendering performance

---

Recommendations

Must Fix 🔴

1. Add nil check for attachmentID in JavaScript (RichContentEditor.js:182)
2. Update test files to work with new MediaEntry type

Should Consider 🟡

1. Add explicit test case for attachmentID being nil
2. Test RCE enhancement feature flag behavior with old vs new media entries
3. Consider logging/analytics to track if attachmentID is ever null when expected

Nice to Have 🟢

1. Add code comment explaining why we switched from media_objects_iframe to media_attachments_iframe
2. Document the difference between the two iframe endpoints in code comments

---

Test Plan Validation ✅

The test plan references the ticket for test accounts and reproduction steps, which is appropriate. However:

• Ensure testing includes viewing old media submissions (uploaded before this change)
• Test with feature flag enabled and disabled
• Test media upload without context

---

Overall Assessment

Verdict: ⚠️ Needs Minor Revisions

The core approach is sound and solves the stated problem. The code quality is good and follows project conventions. However, the critical nil check for attachmentID in JavaScript must be addressed, and tests need updating to compile with the new type signature.

Estimated Risk: Low (once nil check is added)
Estimated Effort to Fix: ~30 minutes

Great work on identifying the root cause and implementing a clean solution with the MediaEntry struct! 🎉

[claude]

Code Review for MBL-19393

Thanks for the fix! This addresses a real issue where media uploads in RCE were using the wrong iframe URL path. I've reviewed the changes and have the following feedback:

✅ Positive Aspects

1. Good Architectural Approach: Introducing the MediaEntry struct (UploadMedia.swift:139-142) properly encapsulates the media ID and attachment ID together, making the data flow clearer.

2. Consistent Implementation: The changes properly thread the attachmentID through all layers:

   • API response parsing (APIMediaComment.swift:211)
   • Upload flow (UploadMedia.swift)
   • File entity (File.swift:64)
   • JavaScript RCE (RichContentEditor.js:109, 180, 182)

3. Backwards Compatibility: The code handles the case where attachmentID might be nil (it's optional), which is important for backwards compatibility.

🐛 Potential Issues

1. Critical: Missing nil handling in JavaScript (RichContentEditor.js:182)

When rce_enhancements feature flag is enabled, the code uses attachmentID directly in the iframe URL without checking if it's undefined/null. If attachmentID is undefined/null, this will generate /media_attachments_iframe/undefined or /media_attachments_iframe/null, breaking media playback.

Recommendation: Add a fallback to the old media_objects_iframe URL when attachmentID is missing.

2. Test Coverage Gaps

The existing tests (UploadMediaTests.swift:86-140) need updates:

• Lines 89 and 126: Mock responses don't include attachment_id, which means tests don't cover the new functionality
• No tests verify that MediaEntry properly carries both IDs through the callback
• No tests verify the JavaScript handles missing attachmentID

Recommendation:

• Update test mocks to include attachment_id: "some_attachment_id"
• Add assertions to verify the MediaEntry contains both mediaID and attachmentID
• Consider adding a test case with nil attachment_id to verify graceful degradation

3. Core Data Migration

The PR adds a new mediaAttachmentID attribute to the File entity (Database.xcdatamodel/contents:874).

Question: Is this a lightweight migration? Since the attribute is optional, it should be fine, but please confirm that no migration code is needed for adding optional attributes.

🔒 Security Considerations

Low Risk: The new attachmentID appears to come from the API response and is properly handled. However, verify that the attachment ID is validated server-side to prevent unauthorized access.

⚡ Performance Considerations

No concerns: The changes don't introduce any performance issues.

📝 Style & Best Practices

1. Good: The new MediaEntry struct follows Swift best practices with immutable properties
2. Good: The callback signature change from String? to MediaEntry? is more type-safe
3. Minor: Consider adding documentation comments to MediaEntry explaining when attachmentID might be nil

🎯 Summary

The fix is architecturally sound and addresses the root cause. The main concern is the missing nil check in JavaScript which could cause regressions for older content or edge cases where the API doesn't return an attachment ID.

Recommended Actions Before Merge:

1. ✅ Add nil/undefined check in RichContentEditor.js:182 with fallback to old URL
2. ✅ Update unit tests to verify the new MediaEntry behavior
3. ✅ Add test case for nil attachmentID scenario
4. ℹ️ Confirm Core Data migration strategy is intentional

Testing Recommendations:

• Test with old content that only has mediaID but no attachmentID
• Test with new uploads to verify attachmentID is populated
• Test the RCE both with and without rce_enhancements feature flag

Let me know if you'd like help implementing any of these suggestions!

[inst-danger]

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

Release Note:

Fixed issue where media text entry submissions using RCE are not being playable when viewed later.

Affected Apps: Student, Teacher

Builds: Student, Teacher

MBL-19393

Coverage	New %	Master %	Delta
Canvas iOS	80.84%	80.41%	0.44%
Horizon/Horizon/Sources/Features/Dashboard/SkillsHighlightsWidget/Data/ProficiencyLevel.swift	0%	--	--
Horizon/Horizon/Sources/Features/Notebook/Common/View/HighlightWebView/EnableZoom.swift	0%	0%	0%
Horizon/Horizon/Sources/Features/Notebook/Common/View/HighlightWebView/HighlightWebFeature.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/Session/SessionInteractor.swift	33.33%	33.33%	0%
Horizon/Horizon/Sources/Common/Data/HModuleStatus.swift	0%	0%	0%
Horizon/Horizon/Sources/Features/Account/Notifications/Domain/NotificationSettingsInteractor.swift	0%	0%	0%
Horizon/Horizon/Sources/Features/Assist/AssistFlashCard/View/AssistFlashCardViewModel.swift	0%	0%	0%
Horizon/Horizon/Sources/Features/LearningObjects/Assignment/AssignmentDetails/View/AssignmentSubmissionType.swift	0%	0%	0%

Generated by 🚫 dangerJS against f0a6a21

[claude]

PR Review - MBL-19393: RCE Media Attachment Fix

Overview

This PR fixes an issue where media uploaded through the Rich Content Editor (RCE) were not playable when viewed later. The fix changes the media iframe URL from using media_objects_iframe to media_attachments_iframe, which requires passing the attachment ID alongside the media ID.

Code Quality & Best Practices

Positive aspects:

• Clean separation of concerns with the new MediaEntry struct encapsulating both IDs
• Consistent API changes across the entire media upload flow
• Good test coverage updates matching the implementation changes
• Follows Swift naming conventions and coding style

Observations:

1. JavaScript null handling (RichContentEditor.js:109, 182) - The code sets img.dataset.media_attachment_id without checking if mediaAttachmentID is defined. If undefined, this will set the attribute to the string "undefined".

2. Missing API documentation - The MediaEntry struct (UploadMedia.swift:139-142) lacks documentation comments.

Potential Bugs & Issues

Critical: Backward compatibility concern (RichContentEditor.js:182) - The code now uses media_attachments_iframe but attachmentID could be null/undefined in some scenarios:

• When context is not provided to UploadMedia (line 124 in UploadMedia.swift)
• For existing media uploads that do not have the new mediaAttachmentID field

This would result in an invalid URL like /media_attachments_iframe/undefined. The code should fall back to the old URL format when attachmentID is missing.

Core Data migration - Adding mediaAttachmentID to the File entity requires a lightweight migration. Verified the new attribute is marked as optional in Database.xcdatamodel.

Performance Considerations

No significant performance concerns identified:

• The additional attachmentID field adds minimal overhead
• API response parsing is straightforward with optional handling
• Database schema change is lightweight (single optional string attribute)

Security Concerns

Minor concern: URL injection risk (RichContentEditor.js:182) - While the IDs come from trusted backend responses, attachmentID is inserted directly into an iframe src without sanitization. Canvas backend should validate these IDs.

Test Coverage

Test updates look good:

• UploadMediaTests.swift properly updated to test both mediaID and attachmentID
• Tests cover success case, error case, and no-context case
• The test assertions correctly validate the MediaEntry struct

Recommendation: Consider adding a test case for the backward compatibility scenario where attachment_id is nil in the API response.

Additional Recommendations

1. Database migration verification - Before merging, verify on a device with existing data that the Core Data migration succeeds without issues
2. Feature flag consideration - This change is already gated behind rce_enhancements feature flag. Ensure the backend supports the media_attachments_iframe endpoint
3. Error handling - Consider logging when attachmentID is unexpectedly nil for debugging purposes

Summary

This is a well-structured fix that properly threads the attachment ID through the media upload flow. The main concern is backward compatibility - the JavaScript code needs to handle cases where attachmentID is missing and fall back to the original URL format. Otherwise, the implementation is solid with good test coverage.

Recommendation: Address the backward compatibility issue before merging, then approve.

[inst-danger]

Builds

Commit: Fix unit tests (f0a6a21)
Build Number: 675
Built At: Oct 30 00:19 CET (10/29 05:19 PM MDT)

Student

Teacher

[rh12]

Code + 1

[rh12]

Unfortunately this is not fixed for me.

I've tried both apps, and I get the exact results which are in the ticket's videos:

1. Login with respective test account
2. Assignments > AssignmentDetails > Submissions (or SpeedGrader)
3. I saw the videos thumbnail, there is a play button
4. Tap play, and it's black box with crossed play button

iPhone 14 @ iOS 18.2
iPad 10 @ iOS 17.2

[suhaibabsi-inst]

Unfortunately this is not fixed for me.

I've tried both apps, and I get the exact results which are in the ticket's videos:

1. Login with respective test account
2. Assignments > AssignmentDetails > Submissions (or SpeedGrader)
3. I saw the videos thumbnail, there is a play button
4. Tap play, and it's black box with crossed play button

iPhone 14 @ iOS 18.2 iPad 10 @ iOS 17.2

@rh12

The testing should not be about viewing the old submission, it should be about submitting a new text entry with media embedded using RCE, then after that you check both submission details and SpeedGrader to view them.

Make sure before you do that, to turn on "Assignment Enhancements - Student" feature flag on the course you are submitting to. See "Steps to reproduce" on the ticket.

Old submissions using that way, would still be in-viewable for using malformed html content.

[rh12]

@suhaibabsi-inst

The testing should not be about viewing the old submission, it should be about submitting a new text entry with media embedded using RCE, then after that you check both submission details and SpeedGrader to view them.

I tried that now, but still the same, the video will not play.

Make sure before you do that, to turn on "Assignment Enhancements - Student" feature flag on the course you are submitting to. See "Steps to reproduce" on the ticket.

it is enabled for the course

ScreenRecording_10-31-2025.13-03-02_1.MP4

[petkybenedek]

Uploaded a video to the test course from the PR app on iOS 26, not working, same as Richard said
:(