Skip to content

Conversation

@Ahmed-Naguib93
Copy link
Contributor

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

test plan: none

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

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

claude bot commented Jan 8, 2026

Claude Code Review

Updated: 2026-01-08

✅ Approved. Changes are correct.

Summary:

  • InboxMessageModel refactored from computed to stored properties - all initializations properly updated via convenience initializers (lines 56-68, 292-305)
  • AnnouncementInteractor guard statement prevents redundant work when announcement already marked read
  • ViewModel refactoring: business logic extracted to testable helper methods (shouldIncludeAnnouncements, filterAnnouncementsByReadStatus, messageMatchesPeopleFilter)
  • AnnouncementsPublisher correctly uses CurrentValueSubject with proper replay semantics
  • Class declarations updated to final for performance optimization
  • Comprehensive test coverage added (672 lines in HInboxViewModelTests)
  • All publisher subscriptions properly composed and stored

No critical issues found.

@inst-danger
Copy link
Contributor

inst-danger commented Jan 8, 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.33% 81.06% 10.27%
Core/Core/Common/Extensions/PDFKit/PDFDocumentExtensions.swift 0% -- --

Generated by 🚫 dangerJS against f232651

@inst-danger
Copy link
Contributor

inst-danger commented Jan 8, 2026

Builds

Commit: fix: code review comments (f232651)
Build Number: 1116
Built At: Jan 08 15:27 CET (01/08 07:27 AM MST)

Student

@claude
Copy link

claude bot commented Jan 8, 2026

Claude Code Review

Critical Issue Found

Race Condition in Announcements Publisher (HInboxViewModel.swift:54,235-254)

  • announcementsPublisher is a PassthroughSubject without replay capability
  • In listenForMessagesAndAnnouncements(), CombineLatest4 won't emit until all four publishers emit
  • Initial announcements from init may be lost if other streams haven't emitted yet
  • Symptom: Announcements missing on initial load, especially under slow network
  • Fix: Use CurrentValueSubject instead or restructure initialization order

✅ Remaining changes acceptable (model refactoring, test additions, class->final)

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