[Horizon] Learn screen redesign #3518
Conversation
refs: CLX-3742 affects: Student release note: none
refs: CLX-3743 affects: Student release note: none
refs: CLXR-57 affects: Student release note: none --------- Co-authored-by: Claude Sonnet 4.5 <[email protected]>
🧪 Unit Test Results📊 Summary
Last updated: Mon, 09 Feb 2026 09:39:51 GMT |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Review Summary
This PR implements a major refactoring of the Horizon module's "Learn" feature, reorganizing the navigation architecture and file structure. I've analyzed all 121 changed files (5,743 additions, 3,141 deletions).
Overview of Changes
- Navigation simplification: Consolidates from dual navigation controllers (main + home) to a single unified controller
- File reorganization: Restructures course/program features into
details/andlist/subdirectories - New API capability: Adds GraphQL query for fetching individual courses (
getCourseWithProgressById()) - Data model enhancement: Adds
courseImageUrlfield toCourseWithProgress - Learn tab refactoring: Implements tabbed navigation (Courses vs Programs)
- Comprehensive test updates: 40+ test files modified to reflect new architecture
Issues Found
- Critical: Null handling in
HorizonGetCoursesManager.kt:61-71- Returns success with invalid data (-1L, empty strings) when course not found. Should returnDataResult.Fail()instead. - Medium: App restart behavior in
DashboardViewModel.kt:106-localeUtils.restartApp()immediately restarts the app, which could be disruptive. Consider deferring or adding user confirmation. - Low: Route handling in
InterwebsToApplication.kt:214-221- Ensure thorough testing of Horizon routing with career view flag enabled/disabled. - Style: Missing newline at EOF in
HorizonGetCourseById.graphql:19
Positive Aspects
Architecture improvements:
- The navigation simplification significantly reduces complexity and potential for navigation bugs
- Better file organization follows clean architecture principles and improves code discoverability
- Type-safe navigation with sealed classes and proper route definitions
Code quality:
- Comprehensive test coverage with 40+ test files updated
- Removal of force unwrap operator (
!!) in favor of safer alternatives - Consistent patterns across new repository and ViewModel classes (Hilt injection, StateFlow)
- New GraphQL API enables more efficient single-course fetching
Developer experience:
- Clear separation of concerns with distinct components for course list and program list
- Feature modularization following single responsibility principle
- Good error handling patterns (with room for improvement noted above)
Recommendations
- Address the critical null handling issue in
getCourseWithProgressById()before merging - Review the app restart behavior and consider UX improvements
- Given the scope of changes (121 files), ensure thorough integration and manual testing, especially for navigation flows
- For future similar refactorings, consider breaking into smaller PRs (e.g., navigation changes separate from file reorganization)
Overall, this is a well-structured refactoring that modernizes the Horizon Learn feature. The navigation simplification is a positive change that should improve long-term maintainability.
| return DataResult.Success(course) | ||
| } catch (e: Exception) { | ||
| DataResult.Fail(Failure.Exception(e)) | ||
| } |
There was a problem hiding this comment.
Potential bug: Returning success with invalid data when course not found
The current implementation uses -1L as a fallback for courseId and empty strings for other fields, then returns DataResult.Success even when the course doesn't exist. This could lead to confusing behavior for callers.
Consider checking if the course data is valid and returning a failure result when not found:
val course = result.legacyNode?.onCourse
if (course?.id == null) {
return DataResult.Fail()
}
val courseId = course.id.toLongOrNull() ?: return DataResult.Fail()
val courseName = course.name ?: ""
val courseImageUrl = course.courseImage?.url ?: ""
CourseWithProgress(
courseId = courseId,
courseName = courseName,
courseCode = course.courseCode ?: "",
courseImageUrl = courseImageUrl,
courseProgress = result.progress?.let {
CourseProgress(
requirementCount = it.requirementCount ?: 0,
requirementCompletedCount = it.requirementCompletedCount ?: 0,
nextRequirementUrl = it.nextRequirement?.htmlUrl ?: "",
completedAt = it.completedAt
)
}
)| } | ||
| } | ||
| } | ||
| } No newline at end of file |
There was a problem hiding this comment.
Style: Missing newline at end of file
This file should end with a newline character per standard conventions.
| intent.addFlags(Intent.FLAG_ACTIVITY_CLEAR_TOP or Intent.FLAG_ACTIVITY_NEW_TASK) | ||
| startActivity(intent) | ||
| } else { | ||
| RouteMatcher.routeUrl(this@InterwebsToApplication, url, domain) |
There was a problem hiding this comment.
Question: Route handling for Horizon feature
This adds routing logic to direct users to HorizonActivity when the career view flag is enabled.
Please ensure:
- This behavior is thoroughly tested with the flag both enabled and disabled
- The route params (
ROUTE_HORIZON_CAREER_COURSE_ID, etc.) are properly validated - Consider what happens if
ApiPrefs.canvasCareerViewchanges state while the app is running
| if (apiPrefs.effectiveLocale != oldLocale) { | ||
| localeUtils.restartApp(context) | ||
| } | ||
| } |
There was a problem hiding this comment.
Potential UX concern: App restart behavior
The localeUtils.restartApp(context) call will immediately restart the entire app when the user's locale is saved. This could be disruptive if the user is in the middle of a task.
Consider:
- Is an immediate restart necessary, or could this be deferred until next app launch?
- Should there be user confirmation before restarting?
- Ensure this behavior is documented and expected
📊 Code Coverage Report
|
2 participants
refs:
affects: Student
release note: Career - Redesigned Learn screen