[MBL-19666][S] Eliminate redundant calls to GetContextTabs and GetCourse APIs when viewing New Quiz details screen

[suhaibabsi-inst]

refs: MBL-19666
affects: Student
builds: Student
release note: None.

Analysis

While it turned out this issue is resolvable by removing force refreshing for course in StudentAssignmentDetailsPresenter.viewIsReady() method:

https://github.com/instructure/canvas-ios/pull/3858/changes#diff-d0da11be14d5d7b553db6be2ac15259f85632f0bee5e141560abb9f33bc87d3dR272

Tracking the origin of this change, I came across the original PR where force refreshing was introduced. Apparently it came out as a necessity as the original Presenter was contributing to "SubmitAssignment" share extension functions. Currently, it no longer contributes to those, hence we can have it reverted.

Other changes:

In order to make the call for context tabs more efficient, following changes were made:

• Implemented makeRequest() method onGetContextTabs that do api.exhaust(..) rather than api.request(..).
• Simplified call sites to use .refresh() instead of manual .exhaust() calls
• Cleaned up CourseDetailsViewModel by removing custom exhaust logic.
• Updated CourseNavigationPresenter, GroupNavigationViewController, and StudentAssignmentDetailsPresenter to use the simplified API.

Test Plan

• Did a regression test for the UX navigating to a new Quiz,
• With the help of a proxy tool (Proxyman), it was noticed that no API calls is made to those endpoints when opening NewQuiz details screen any more.

For more information, see ticket's description.

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]

Review Summary

This PR refactors how GetContextTabs handles pagination by moving the exhaust logic from call sites into the use case itself via a custom makeRequest override. While this simplifies call sites, there are several concerns about this approach.

Architecture & Pattern Consistency

Positive:

• Simplifies call sites - callers no longer need to know about pagination details
• Reduces code duplication across multiple files
• Makes the exhaustive pagination behavior more centralized

Concerns:

• Breaking Pattern Consistency (GetContextTabs.swift:43-48): The override of makeRequest to use api.exhaust breaks the standard UseCase pattern. All other CollectionUseCase implementations use the default api.makeRequest. This inconsistency makes the codebase harder to reason about.

• Loss of Caller Control: Previously, callers could choose between refresh() (single page) and exhaust() (all pages) based on their needs. Now all callers are forced to exhaust all pages, even when they may only need the first page.

• Async Timing Change (CourseDetailsViewModel.swift:265-269): The continuation now resumes after the first page callback, not after all pages are exhausted. This changes when await refresh() returns and could cause race conditions.

Potential Bugs

• Race Condition (CourseDetailsViewModel.swift:192): The updateTabs() method checks !tabs.hasNextPage to ensure all tabs are loaded. With the new pattern, the first refresh callback fires before pagination completes, so hasNextPage may still be true, preventing the UI from updating properly.

• Unrelated Change (StudentAssignmentDetailsPresenter.swift:272): Removes force: true from courses refresh. This appears unrelated to the tabs pagination changes. Was this intentional?

Performance Considerations

• Always Exhausting on View Appear (CourseDetailsViewModel.swift:106): Every viewDidAppear() now triggers exhausting all tab pages (when cache is expired). While the cache mitigates this, it's less visible that this potentially expensive operation happens on every view appearance.

Testing

The existing tests in CourseDetailsViewModelTests mock single-page responses. Consider adding tests that verify multi-page tab responses are handled correctly with the new pattern.

Recommendations

1. Consider Alternative Approach: Instead of overriding makeRequest, consider adding a property on GetContextTabs like shouldExhaustPages: Bool = true that the Store checks, keeping the behavior explicit.

2. Fix Async Timing: If exhausting is truly required, the async continuation should wait until all pages complete, not just the first page.

3. Document Behavior: Add inline documentation explaining why GetContextTabs always exhausts pages and how this differs from other use cases.

4. Verify Unrelated Change: Confirm whether the courses.refresh() change in StudentAssignmentDetailsPresenter was intentional.

Code Quality

The code follows Swift and SwiftLint conventions. No security concerns identified. The refactoring is well-structured, but the architectural implications need consideration before merging.

[inst-danger]

Release Note:

None.

Affected Apps: Student

Builds: Student

MBL-19666

Coverage	New %	Master %	Delta
Canvas iOS	91.43%	81.06%	10.37%

Generated by 🚫 dangerJS against cf6c682

[inst-danger]

Builds

Commit: Fix unit tests (cf6c682)
Build Number: 1212
Built At: Jan 26 17:48 CET (01/26 09:48 AM MST)

Student

[vargaat]

causing the call to load all the tabs and banner resources every time a user clicks a next question.

The original ticket mentions that these calls are made each time the user goes to the next question. Did you test that scenario as well? How requesting the next question in a webview triggers a native reload?

[suhaibabsi-inst]

causing the call to load all the tabs and banner resources every time a user clicks a next question.

The original ticket mentions that these calls are made each time the user goes to the next question. Did you test that scenario as well? How requesting the next question in a webview triggers a native reload?

Yes, I have tested that, and found no trace of multiple calls when users tap "next question".
The only duplicate calls I found is when user up to view the details of NewQuiz.

Moreover, if go back a bit with the analysis they've given, they said:

She dug into the course and also pointed out that the "one question at a time" setting was enabled for the quiz, which in conjunction with the mobile usage could be causing the call to load all the tabs and banner resources every time a user clicks a next question.

So, there hasn't been an actual proof that the Mobile is doing so. It seemed like a speculation to me.

[suhaibabsi-inst]

@vargaat Here is my full comment about it:
https://instructure.atlassian.net/browse/MBL-19450?focusedCommentId=2758020

[vargaat]

@suhaibabsi-inst Awesome, thanks for the clarification.