[MBL-18172] [S/T] Split View selection highlights

[petkybenedek]

What's new?

Selections stay highlighted in Split View on the following screens:

• Assignments
• Grades
• Quizzes
• To-Do
• Notifications
• Inbox

This PR introduces a SwiftUI-idiomatic approach to indicate selection with Environment Values. This eliminates the need of passing down parameters in the View Hierarchy.

refs: MBL-18172
builds: Student, Teacher
affects: Student, Teacher
release note: Selections on iPad now stay highlighted.

Screenshots

Before	After

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

[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:

Split View selections now stay highlighted.

Affected Apps: Student, Teacher

Builds: Student, Teacher

MBL-18172

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/Account/Notifications/Domain/NotificationSettingsInteractor.swift	0%	0%	0%
Horizon/Horizon/Sources/Common/Data/HModuleStatus.swift	0%	0%	0%
Horizon/Horizon/Sources/Features/Session/SessionInteractor.swift	33.33%	33.33%	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 3b8c0af

[inst-danger]

Builds

Commit: remove whitespace (3b8c0af)
Build Number: 686
Built At: Oct 30 14:14 CET (10/30 07:14 AM MDT)

Student

Teacher

[Copilot]

Pull Request Overview

This PR implements selection highlighting in Split View for multiple screens in the Student and Teacher apps. The solution introduces a SwiftUI-based environment value approach to track and display selected items when the split view is not collapsed.

Key changes:

• Added environment value-based selection tracking system using isItemSelected
• Updated existing button styles to support selection highlighting through environment values
• Modified view models to track selected item IDs and pass them to views

Reviewed Changes

Copilot reviewed 15 out of 15 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
Selection.swift	New file introducing environment value system for selection tracking
TintedContextButton.swift	Refactored to use environment values instead of parameter-based highlighting
InboxMessageView.swift	Updated to use new button style and selection environment
InboxViewModel.swift	Added selectedMessageID tracking
InboxView.swift	Added selection highlighting to message cells
GradeListViewModel.swift	Added selectedAssignmentID tracking and updated assignment selection handling
GradeListScreen.swift	Updated to pass selectedAssignmentID to AssignmentListView
AssignmentListScreenViewModel.swift	Added selectedAssignmentID tracking
AssignmentListView.swift	Added selection highlighting to all assignment cells
AssignmentListScreen.swift	Updated to pass selectedAssignmentID and contains commented debug code
CollapsibleListRow.swift	Added selection highlighting support
ActivityStreamViewController.swift	Added UIKit-based selection restoration after table view reload
TodoListViewController.swift	Added UIKit-based selection restoration after table view reload
QuizListViewController.swift	Added UIKit-based selection restoration after table view reload
GradeListViewModelTests.swift	Updated test to match new assignment selection signature

[Copilot]

Pull Request Overview

Copilot reviewed 15 out of 15 changed files in this pull request and generated 37 comments.

[Copilot]

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

[suhaibabsi-inst]

How about replacing the whole navigateToDetailsAction & selectedAssignmentID with a @Published property selectedAssignmentID on screen viewModel. and you can pass that property to this view as @Binding. Having an action and a property referring to the same thing seems difficult to maintain. And you get even more SwiftUI-friendly way of handling state.

[suhaibabsi-inst]

We can say the same about other list views as well: InboxView and GradeListScreen

[petkybenedek]

On the AssignmentListScreen, the navigation action is still required, that's how the ViewModel receives the viewController. Without it, your approach would work, but now I have to go with my solution.

[rh12]

Thanks for adding this!

[Copilot]

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

[claude]

Code Review for PR #3686

Overview

This PR introduces a clean, SwiftUI-idiomatic approach to handling split view selection highlights using Environment Values. The implementation is well-architected and follows SwiftUI best practices.

---

✅ Strengths

1. Excellent Architecture

• The new ItemSelection.swift (Core/Core/Common/CommonUI/ViewModifiers/ItemSelection.swift:21-48) introduces a reusable, declarative solution using SwiftUI Environment Values
• This eliminates prop drilling and makes selection state management much cleaner
• The .selected(when:) modifier is intuitive and follows SwiftUI conventions

2. Smart Split View Detection

• The ItemSelectionModifier correctly checks if the split view is collapsed (line 43), automatically disabling selection highlights on iPhone
• Default value of true for isCollapsed is a safe fallback when split view controller isn't available

3. Refactoring of TintedContextButton

• Removing the isHighlighted parameter simplifies the API
• Migration to Environment Values is the right approach
• Documentation is clear and helpful (line 24)

4. Consistent Pattern Application

• Selection tracking is consistently implemented across multiple screens:
  • Assignments (Core/Core/Features/Assignments/AssignmentList/View/AssignmentListView.swift:62-67)
  • Grades (Core/Core/Features/Grades/View/GradeListScreen.swift:155)
  • Inbox (Core/Core/Features/Inbox/View/InboxView.swift:86)
• .selectionIndicatorDisabled() is properly used on sub-items to prevent nested highlights

5. Good Test Coverage

• Updated test in GradeListViewModelTests (Core/CoreTests/Features/Grades/ViewModel/GradeListViewModelTests.swift:155-157) validates both navigation and selection state
• Test verifies selectedAssignmentId is set correctly

---

⚠️ Issues & Concerns

1. Missing Tests for New Components (Medium Priority)

The new ItemSelection.swift module and its core functionality lack dedicated unit tests:

• No tests for ItemSelectionModifier behavior
• No tests for split view collapsed/expanded states
• No tests for .selectionIndicatorDisabled() behavior
• No tests for the environment value propagation

Recommendation: Add tests covering:

• Selection highlighting when split view is expanded
• No highlighting when split view is collapsed
• Proper environment value inheritance
• .selectionIndicatorDisabled() overrides

2. Potential Race Condition (Low Priority)

In multiple ViewModels, selectedAssignmentId/selectedMessageId is set on the scheduler before navigation:

// Core/Core/Features/Assignments/AssignmentList/ViewModel/AssignmentListScreenViewModel.swift:127-129
.sink { url, id, controller in
    guard let url else { return }
    self.selectedAssignmentId = id  // Published property update
    env.router.route(to: url, from: controller, options: .detail)
}

If the user rapidly taps multiple items, there's a theoretical race where the published value updates before the view redraws.

Recommendation: Consider verifying this behavior during manual testing, especially on slower devices.

3. Inbox Button Style Change (Low-Medium Priority)

The InboxMessageView changes button style from .plain to .tintedContextButton (Core/Core/Features/Inbox/View/InboxMessageView.swift:38):

- .buttonStyle(PlainButtonStyle())
+ .buttonStyle(.tintedContextButton)

This introduces the vertical accent line indicator on press, which may not have been tested.

Recommendation: Verify this visual change is intentional and looks correct in the Inbox screen.

4. Missing AssignmentListViewModel Tests (Medium Priority)

While GradeListViewModel has a test for selection, AssignmentListScreenViewModel doesn't have a corresponding test for the new selectedAssignmentId behavior.

Recommendation: Add a test similar to the one in GradeListViewModelTests to verify assignment selection tracking.

5. No InboxViewModel Tests (Medium Priority)

The InboxViewModel changes lack test coverage for selectedMessageId tracking.

Recommendation: Add tests to verify message selection state is correctly maintained.

---

🔍 Minor Observations

1. Whitespace-Only Changes

Several files have only indentation fixes:

• Core/Core/Features/ObservedStudents/APIAlertThreshold.swift:32-36
• Core/Core/Features/Pages/APIPage.swift:45
• Core/CoreTests/Features/Planner/Model/API/APIPlannableTests.swift
• Core/CoreTests/Features/Modules/ModuleList/ViewModel/ModulePublishMenuTests.swift

These are good cleanup but could potentially be in a separate formatting-only commit to keep the functional changes clearer.

2. Documentation Quality

The inline documentation is good:

• Clear doc comment on .selected(when:) (line 26-27)
• Clear doc comment on .selectionIndicatorDisabled() (line 32)

---

🔒 Security & Performance

• ✅ No security concerns identified
• ✅ Performance impact is minimal - environment values are efficient
• ✅ No unnecessary redraws or state updates
• ✅ Proper use of @Published properties

---

📝 Recommendations Summary

Must Address Before Merge:

• Add unit tests for ItemSelection.swift module
• Verify Inbox button style visual change is intentional
• Add tests for AssignmentListScreenViewModel.selectedAssignmentId
• Add tests for InboxViewModel.selectedMessageId

Nice to Have:

• Manual testing of rapid selection changes
• Consider separating whitespace-only changes

---

🎯 Overall Assessment

This is a high-quality PR with excellent architectural decisions. The use of Environment Values is the correct SwiftUI pattern for this problem. The main concern is test coverage for the new functionality. Once tests are added, this will be ready to merge.

Recommendation: Request changes for additional test coverage before merging.

[rh12]

QA + 1

As discussed offline, we scoped the changes to SwiftUI screens only, so not all screens in the PR description are updated.

One minor issue:
On iPhone, Inbox rows are not showing highlight on tap