Skip to content

Conversation

@Ahmed-Naguib93
Copy link
Contributor

@Ahmed-Naguib93 Ahmed-Naguib93 commented Jan 4, 2026

refs: CLX-3721
builds: Student
affects: Student
release note: none

What is new?

  1. Removed unnecessary code, including properties and functions.
  2. Removed SwiftUI and HorizonUI imports from ViewModels.
  3. Fixed pull-to-refresh to call all APIs.
  4. Enhanced API calls when loading the view.
  5. Fixed issues with uploading and downloading files.
  6. Called the notifications API to show announcements and set indicators for read and unread. This was added because announcements were previously hidden from notifications.
  7. Removed the uploaded files if the user discarded the message the safe the quota.
  8. Called GetAllAnnouncementsRequest in dashboard widget in place of activity_stream
Before After Comment
https://github.com/user-attachments/assets/0dcddf58-e0c1-4bc1-a740-122c12de3935 https://github.com/user-attachments/assets/b4c1de3e-9b7f-47b6-a1c5-f2194ebc3066 Selecting multiple recipients isn't working.
Before After Comment
https://github.com/user-attachments/assets/f7814cd5-5b32-4d70-b1a8-5b646d7ff28c https://github.com/user-attachments/assets/576a241f-ecfb-47c9-9d2b-e9f6ec43b012 When selecting different files, uploading files with the same name still shows the loader until you take any action.
Before After Comment
https://github.com/user-attachments/assets/a1544665-2072-4f22-8c19-83f54dba38b1 https://github.com/user-attachments/assets/955b0ee3-5370-4f63-8f5a-b46ab4d35119 When you exceed your quota, an error message is shown.
Before After Comment
https://github.com/user-attachments/assets/d8e79b49-0a32-4949-aea4-f9907066ecf7 https://github.com/user-attachments/assets/60f8c250-a4e2-4fe4-a5a6-0d84d49d4882 Enhanced launch inbox view
Before After Comment
https://github.com/user-attachments/assets/caf8abed-0e7e-4853-82cd-46d33d817b0c https://github.com/user-attachments/assets/f6264171-c60e-4d76-8632-8c4d41031b35 Added an indicator for unread announcements and applied it to the unread filter as well.

refs: CLX-3701
builds: Student
affects: Student
release note: none

test plan: none
refs: CLX-3721
builds: Student
affects: Student
release note: none

test plan: none
@Ahmed-Naguib93 Ahmed-Naguib93 self-assigned this Jan 4, 2026
@Ahmed-Naguib93 Ahmed-Naguib93 added the career Canvas Career Experience label Jan 4, 2026
@claude
Copy link

claude bot commented Jan 4, 2026

Claude Code Review

Updated: 2026-01-04

Approved - No critical issues found.

Summary:

  • Refactored Inbox feature with proper separation of concerns (announcements vs messages)
  • Added UI helper modifiers (loader overlay, keyboard dismiss, rounded corners)
  • Improved attachment handling with upload state management
  • Enhanced notification integration with unread indicators
  • Properly refactored HMessageDetailsView into separate components

Key observations:

  • Old view models properly replaced with new architecture
  • Subscription management correctly implemented
  • API integration logic properly refactored to use Combine publishers
  • All deleted files have appropriate replacements
  • No breaking changes detected

✅ Approved

@inst-danger
Copy link
Contributor

inst-danger commented Jan 4, 2026

Warnings
⚠️ This pull request will not generate a release note.
⚠️ One or more files are below the minimum test coverage 50%

Affected Apps: Student

Builds: Student

Coverage New % Master % Delta
Canvas iOS 91.23% 81.06% 10.17%
Core/Core/Common/Extensions/PDFKit/PDFDocumentExtensions.swift 0% -- --

Generated by 🚫 dangerJS against 5b7a454

@inst-danger
Copy link
Contributor

inst-danger commented Jan 4, 2026

Builds

Commit: Merge branch 'master' into feature/career-Enhancement-inbox-CLX-3721 (5b7a454)
Build Number: 1113
Built At: Jan 08 13:24 CET (01/08 05:24 AM MST)

Student

@claude
Copy link

claude bot commented Jan 5, 2026

Claude Code Review

Updated: 2026-01-05

Critical Issues Found:

  1. Debug print statement in RecipientSelectionViewModel.swift:75 - print(value) should be removed
  2. Logic issue in HInboxViewModel.swift:183 - announcement filter may exclude announcements prematurely
  3. isSending state not reset on sendMessage failure in HMessageDetailsViewModel.swift:136
  4. Conversation object may be nil when accessed in HMessageDetailsViewModel.swift:82-85

🚫 Issues found

@claude
Copy link

claude bot commented Jan 5, 2026

Claude Code Review

Updated: 2026-01-05

Critical Issues Found

  1. Missing self prefix in HMessageDetailsViewModel - Code filters recipients with .filter { $0 != userID } but userID is not in scope - should be self.userID. Will crash on reply message send.

  2. Broken announcement route fallback - Changed from /announcements/:announcementID to /announcements requiring announcement in userInfo. Guard returns nil on missing data, causing silent navigation failure with no error handling.

  3. Incomplete AttachmentFileModel refactor - Deleted AttachmentItemViewModel with download logic but AttachmentFileModel lacks performAction() implementation. UI expects download functionality that no longer exists.

Positive Changes

  • Added didUploadFiles subject for proper file upload state tracking
  • Improved pull-to-refresh to call all APIs
  • Fixed keyboard dismissal handling

🔴 Issues found

@claude
Copy link

claude bot commented Jan 5, 2026

Claude Code Review

Updated: 2026-01-05

✅ Approved

No critical issues found. Large-scale inbox refactoring with proper subscription management, weak self captures, and sound filtering logic.

@claude
Copy link

claude bot commented Jan 6, 2026

Claude Code Review

Updated: 2026-01-06

Critical Issues Found

  • Type Safety: ComposeMessageInteractorPreview.swift:28 - Uses 'any Error' instead of 'Error', breaks protocol conformance
  • Data Model Migration: DiscussionTopic.swift:64 - New 'readState' property has no default value; will crash on old CoreData objects when accessing computed 'isRead' property
  • Silent Error Swallowing: ComposeMessageInteractorLive.swift:168 - Attachment binding ignores all errors from fileStore, no error communication to UI
  • Retain Cycle Risk: ComposeMessageInteractorLive.swift:199 - File parameter captured by value in async closure chain; verify lifecycle

Action Items

  1. Fix type mismatch: 'any Error' to 'Error' in Preview implementation
  2. Provide CoreData migration strategy or default value for 'readState'
  3. Implement proper error handling in setupAttachmentListBinding()
  4. Review file capture in duplicateFileToUploadFolder()

❌ Issues found

Base automatically changed from feature/career-Add-a11y-for-inbox-messages-CLX-3701 to master January 8, 2026 11:44
@claude
Copy link

claude bot commented Jan 8, 2026

Claude Code Review

Updated: 2026-01-08 12:10:00 UTC

Code Quality Assessment

APPROVED - No critical issues found.

Key Points:

  • Database schema updated correctly with readState field for DiscussionTopic
  • Message details UI cleanly separated into Message and Announcement views with dedicated ViewModels
  • File upload failure handling properly implemented with subscription cleanup in AttachmentViewModel
  • New AnnouncementInteractor correctly handles both course-based and global announcements
  • Reactive Store patterns used consistently for API calls with error suppression
  • Weak self captures properly managed in closures
  • LoaderOverlayModifier and RoundedTopCorners are well-designed reusable view modifiers

Minor Notes:

  • File attachment security-scoped resource handling follows best practices
  • New didUploadFiles publisher correctly bridged from upload manager to attachment UI

✅ Approved

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

career Canvas Career Experience

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants