[CLCR-58][Horizon] Learning library list#3530
[CLCR-58][Horizon] Learning library list#3530domonkosadam merged 35 commits intofeature/learning-libraryfrom
Conversation
🧪 Unit Test Results✅ 📱 Student App
✅ 🌅 Horizon
✅ 📦 Submodules
📊 Summary
Last updated: Mon, 02 Mar 2026 18:17:17 GMT |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Review: Learning Library Feature Addition
Overview
This PR adds Learning Library functionality to the Canvas Android app, introducing a "Browse" tab in the Learn screen that displays enrolled learning library collections. This is a substantial feature addition with comprehensive API integration, UI components, and test coverage.
Summary
- Files Changed: ~30+ files across canvas-api-2 and Horizon modules
- New Features: GraphQL queries, data models, managers, UI screens, navigation, and tests
- Scope: New browse functionality with collection/item displays, bookmarking, and enrollment
Issues to Address
Critical Issues (Must Fix Before Merge)
- LearnViewModel.kt:40 - Empty catch block silently ignores errors when loading enrolled libraries. The BROWSE tab won't appear on failure with no user indication why.
- GetLearningLibraryManagerImpl.kt:88 - Unsafe
valueOf()enum conversion will crash if backend returns unknownCollectionItemTypevalues.
High Priority Issues
- LearnLearningLibraryBookmarkScreen.kt:16 - Screen is incomplete (placeholder only). Users can't view bookmarked items. Consider implementing or hiding this feature until ready.
- LearnLearningLibraryCompletedScreen.kt:16 - Screen is incomplete (placeholder only). Same concern as bookmark screen.
- LearnLearningLibraryListViewModel.kt:122 - Missing user feedback when bookmark/enroll operations fail. Items appear unresponsive with no error indication.
Code Quality Issues
- LearnLearningLibraryListViewModel.kt:61 - Hard-coded error message "Failed to load Learning Library" instead of using string resources (breaks i18n).
- LearnLearningLibraryListViewModel.kt:25 - Page size hard-coded to 3 (seems very small for production). Verify if intentional or increase to reasonable value (20-50).
Test Coverage Gaps
- No tests for detail/bookmark/completed screens
- Integration tests have mock setup but no actual test cases
- Limited testing of composable UI components
Positive Aspects ✅
Well-Structured Architecture:
- Clear separation of concerns with proper data models and managers
- Good Hilt DI setup and dependency injection patterns
- Proper package restructuring (JourneyAssist models →
assistsubpackage)
Comprehensive Test Coverage:
- 25+ unit tests for ViewModel covering happy path, errors, and edge cases
- Repository tests with proper mocking
- UI tests for Learning Library List screen
- Good test data factories
Quality UI Implementation:
- Reusable composable components with proper state management
- Preview composables for UI testing
- Proper internationalization with string resources (~20 new entries)
- Good TabRow state management fix (tabs as key in rememberSaveable)
Solid API Integration:
- 5 new GraphQL operations with proper schema updates
- Cursor-based pagination support
- Type-safe enum-based
CollectionItemTypewith conversion functions - Appropriate use of
forceNetworkparameter
Design System Enhancements:
- New
StatusChipColorvariants (Institution, Orange) - Added bookmark fill icon drawable
Recommendations
Before Merging:
- Fix the two critical issues (error handling and enum safety)
- Decide on incomplete screens: implement fully or hide the navigation to them
- Add user-facing error messages for failed operations
- Use string resources consistently for all user-facing text
Nice to Have:
- Increase page size to production-ready value
- Add test cases for integration tests
- Separate GraphQL schema formatting changes from functional changes
Post-Merge Improvements:
- Add screen-specific tests for detail/bookmark/completed screens
- Implement pagination UI when needed
- Add retry logic for failed API calls
- Monitor performance with large collection datasets
Conclusion
This is a well-structured feature addition with good architecture and solid test coverage. The core implementation is sound, but there are 2 critical issues and several high-priority items that should be addressed before merging to ensure a stable user experience. The incomplete bookmark/completed screens need a decision: either complete them or hide them until ready.
Overall, great work on the comprehensive implementation! 🎉
|
Student Install Page |
Update and extend test coverage to reflect the refactored list screen:
- Fix all ViewModel tests to use the new nested state structure
(collectionState/itemState) after the split into two loading states
- Replace old client-side search filter tests with tests for the new
server-side item loading flow (debounce, status/type filter triggers)
- Add ViewModel tests for: isEmptyFilter logic, type/status filter state
updates and item loading, cursor-based pagination (show more), item
refresh, dual-state bookmark/enroll updates across both item and
collection state, and snackbar dismiss clearing both states
- Add Repository tests for the new getLearningLibraryItems method
covering all parameters: cursor, searchTerm, types, bookmarkedOnly,
completedOnly, limit, forceNetwork, and pagination info
- Rewrite UI tests to use the new nested state constructors, replace
the removed history icon button test with filter-related tests, and
add coverage for: show more button visibility, item chips, enroll
button, completed icon, bookmark content descriptions, item count
display logic, and item/collection view switching
- Add testTag("CollapsableBody") to both LazyColumns to support scroll
assertions in UI tests
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
andrasmaczak
left a comment
andrasmaczak
left a comment
There was a problem hiding this comment.
Code LGTM, some found issues already fixed, only keyboard e2e needs fixing
There was a problem hiding this comment.
PR Review Progress Update
This is an updated review for the Learning Library feature PR (synchronize event).
Progress on Previously Identified Issues
(No previous review data was retrievable — all issues below are newly identified in this pass)
Summary of Changes
This PR introduces a Learning Library feature (~97 files) for the Horizon/Journey platform:
- New GraphQL queries/mutations for collections, items, bookmarks, and enrollment
- New domain models under
learninglibrary/ - New Compose UI screens (list, detail, enroll, bookmark, completed views)
JourneyAssist*package reorganization (models.journey→models.journey.assist)- GraphQL schema additions and breaking changes
Issues Requiring Changes
[ ] Bug: FakeGetLearningLibraryManager.getLearningLibraryItem — wrong map key (line 163)
The collectionItems map is keyed by collection ID, but the method does collectionItems[itemId] — always returns empty, always throws NoSuchElementException. See inline comment.
[ ] Bug: CollectionItemType.valueOf() unsafe across 4 call sites (lines 94, 147, 192, 223)
Any unexpected enum value from the backend (e.g., UNKNOWN__, new type) will throw IllegalArgumentException at runtime. The safe toModel() extension already exists and should be used. See inline comment.
[ ] Code Quality: Apollo→domain mapping duplicated 4 times
The full LearningLibraryCollectionItem(...) construction is copy-pasted four times. Extract a private mapper to avoid future divergence. See inline comment.
Minor Issues (non-blocking)
- Test reliability:
onShowMoreClicked sets isMoreButtonLoading during load— withUnconfinedTestDispatcherthe coroutine completes synchronously, so this test doesn't actually observe the mid-load state. - Breaking schema change:
accountIdis nowString!(required) in several mutations — ensure all Android call sites supply this argument. - Missing trailing newlines in multiple new files (domain models, GraphQL files).
Positive Notes
- Solid test coverage: ViewModel, Repository, and UI tests added for all new screens
- 300ms debounced search is a nice UX touch
- Clean sealed-class navigation routing
- Good use of fake managers for Espresso test isolation
There was a problem hiding this comment.
PR Review Progress Update
Note: I was unable to retrieve my previous review data due to permission constraints on this run, so this is a fresh review pass on the current state of the PR.
Summary
This PR adds a substantial Learning Library feature to the Horizon app — GraphQL schema expansion, Apollo query/mutation integration, data models, full Compose UI (list, enroll, bookmark screens), ViewModel logic with debounced search and pagination, navigation fixes, espresso fakes, and Compose UI tests. The overall architecture is solid and the implementation of the list/filter/pagination logic is well-done.
There are a few issues that need attention before merging:
Issues Requiring Changes
-
CollectionItemType.UNKNOWN__throwsIllegalArgumentException— potential crash when server returns a new type the client doesn't know about yet. Inline comment added. -
Item
nameis always empty for non-course items —LearnLearningLibraryExtensions.kt:48usescanvasCourse?.courseName.orEmpty()as the card display name, which will always be blank for PAGE, FILE, EXTERNAL_URL, and other non-course item types. Inline comment added. -
Silent error swallowing in
LearnViewModel.init— the emptycatch { }block silently discards all errors with no logging or user feedback. Inline comment added. -
Code duplication in
GetLearningLibraryManagerImpl—LearningLibraryCollectionItemfield mapping is copy-pasted four times. Inline comment added. -
LearnRepositoryTestandLearnViewModelTestdeleted with no replacement — the refactoredLearnRepositoryandLearnViewModelhave zero unit test coverage. Inline comment added onLearnViewModel.
Minor Issues (No Inline Comment)
LearnLearningLibraryBookmarkScreenis a placeholder stub — navigation is wired up but the screen only rendersText("Learning Library Bookmark Screen"). If this is intentional for the current sprint, a// TODOcomment referencing a tracking ticket would help.- Missing trailing newlines on all new
.ktand.graphqlfiles — cosmetic but violates POSIX file standards.
Positive Observations
- Debounced search (300ms) via
searchQueryFlow+collectLatestis well-implemented and avoids redundant API calls. - Both
onBookmarkItemandonCollectionBookmarkItemupdateitemStateandcollectionStatein a single_uiState.updatecall, correctly avoiding state desync. FakeGetLearningLibraryManagerfor espresso tests is comprehensive, covering bookmarks, search, type, and completion filters.- The navigation fix using
currentDestination?.hierarchy?.drop(1)?.any { it.route in subPageRoutes }properly handles nested nav graph visibility for the bottom bar. isEmptyFilter()cleanly determines whether to show the collections view or the filtered items view.
| ApolloCollectionItemType.EXTERNAL_TOOL -> CollectionItemType.EXTERNAL_TOOL | ||
| ApolloCollectionItemType.FILE -> CollectionItemType.FILE | ||
| ApolloCollectionItemType.PROGRAM -> CollectionItemType.PROGRAM | ||
| ApolloCollectionItemType.UNKNOWN__ -> throw IllegalArgumentException("Unknown CollectionItemType: $this") |
There was a problem hiding this comment.
Potential crash: UNKNOWN__ Apollo type throws IllegalArgumentException
If the server introduces a new CollectionItemType that the client doesn't know about yet, toModel() will throw an uncaught IllegalArgumentException, which will propagate up through the Apollo response mapping and crash the feature (or the app, depending on surrounding try-catch coverage).
Consider returning a nullable type or a designated UNKNOWN fallback value, then filtering out null results upstream, so the app degrades gracefully when the server adds new content types:
ApolloCollectionItemType.UNKNOWN__ -> null // or CollectionItemType.UNKNOWN| id = this.id, | ||
| courseId = courseId, | ||
| imageUrl = this.canvasCourse?.courseImageUrl, | ||
| name = this.canvasCourse?.courseName.orEmpty(), |
There was a problem hiding this comment.
Item name is always empty for non-course items
canvasCourse?.courseName.orEmpty() is the sole source of the display name for every item type. For PAGE, FILE, EXTERNAL_URL, EXTERNAL_TOOL, and PROGRAM items, canvasCourse is null, so name will always be an empty string. These cards will display a blank title in the UI.
The LearningLibraryCollectionItem model likely has a top-level name or title field that should be used here, falling back to canvasCourse?.courseName only for COURSE items. Please verify the domain model and use the appropriate field.
| LearnTab.fromStringValue(selectedTabValue)?.let { selectedTab -> | ||
| _uiState.update { it.copy(selectedTab = selectedTab) } | ||
| } | ||
| } catch { } |
There was a problem hiding this comment.
Silent error swallowing hides failures from the user
The empty catch { } block silently discards every exception (network errors, auth failures, serialization errors, etc.). When this block is hit, the BROWSE tab simply never appears and the user gets no feedback — no error state, no retry option, no log entry.
At minimum, log the error so it's visible in crash reporting:
} catch { e ->
Logger.e("Failed to load enrolled learning libraries", e)
}If the BROWSE tab's absence is a meaningful UX outcome (e.g., the user truly has no enrolled collections), that's fine — but errors should still be distinguishable from the empty-collection case, ideally with a visible error state or retry.
| val result = journeyClient.enqueueQuery(query, forceNetwork = forceNetwork).dataOrThrow().learningLibraryCollectionItems | ||
|
|
||
| return LearningLibraryCollectionItemsResponse( | ||
| items = result.items.map { item -> |
There was a problem hiding this comment.
Code duplication: LearningLibraryCollectionItem is constructed identically in four places
The same field-by-field construction of LearningLibraryCollectionItem (and CanvasCourseInfo) from an Apollo result object is repeated in getLearningLibraryCollectionItems (line ~92), getEnrolledLearningLibraryCollections (line ~145), enrollLearningLibraryItem (line ~190), and getLearningLibraryItem (line ~221). If a field is added or renamed, all four sites must be updated in sync.
Extract a private extension function, for example:
private fun <T : ItemFields> T.toCollectionItem() = LearningLibraryCollectionItem(
id = id,
libraryId = libraryId,
itemType = itemType.toModel(),
// ... remaining fields
)This also makes each query method much easier to read.
| LearnTab.fromStringValue(selectedTabValue)?.let { selectedTab -> | ||
| _uiState.update { it.copy(selectedTab = selectedTab) } | ||
| } | ||
| viewModelScope.tryLaunch { |
There was a problem hiding this comment.
LearnRepositoryTest and LearnViewModelTest were deleted with no replacement
Both test files were removed because LearnRepository and LearnViewModel were significantly refactored — but no new tests were added for the updated implementations. The refactored LearnViewModel.init (which fetches enrolled libraries and conditionally adds the BROWSE tab) has no unit test coverage at all.
Please add unit tests for at least the critical paths:
- BROWSE tab is added when enrolled libraries are non-empty
- BROWSE tab is absent when enrolled libraries are empty
- BROWSE tab is absent when the network request fails (the current silent-catch path)
|
|
||
| val collectionItem = repository.loadLearningLibraryItem(learningLibraryItemId) | ||
| learningLibraryItem = collectionItem | ||
| val courseDetails = repository.loadCourseDetails(collectionItem.canvasCourse!!.courseId.toLong()) |
There was a problem hiding this comment.
Potential crash: unsafe !! on canvasCourse
collectionItem.canvasCourse can be null for non-COURSE type items or if the API response omits the field. This will throw a NullPointerException at runtime.
Additionally, courseId.toLong() can throw NumberFormatException if the ID string is not a valid long. Consider using toLongOrNull() with a fallback, consistent with LearnLearningLibraryExtensions.kt which already uses toLongOrNull() ?: -1L.
// Safer approach
val courseId = collectionItem.canvasCourse?.courseId?.toLongOrNull() ?: return
val courseDetails = repository.loadCourseDetails(courseId)| } catch { | ||
| // Silent error | ||
| _state.update { it.copy(loadingState = it.loadingState.copy(isLoading = false)) } |
There was a problem hiding this comment.
Silent error creates a crash trap
The silent catch here leaves learningLibraryItem = null. If loadData fails and the user then taps "Enroll", lines 84–85 will call learningLibraryItem!!.id and learningLibraryItem!!.canvasCourse!!.courseId.toLong(), crashing the app with a NullPointerException.
The error should be surfaced to the user via the LoadingState error mechanism, consistent with the pattern used throughout this feature:
} catch (e: Exception) {
_state.update { it.copy(loadingState = it.loadingState.copy(isLoading = false, isError = true)) }
}| repository.enrollLearningLibraryItem(learningLibraryItem!!.id) | ||
| _state.update { it.copy(isEnrollLoading = false, navigateToCourseId = learningLibraryItem!!.canvasCourse!!.courseId.toLong()) } |
There was a problem hiding this comment.
Two more unsafe !! operators that will crash if loadData failed silently
learningLibraryItem!!on line 84 crashes if loading failed (per the silent catch above)learningLibraryItem!!.canvasCourse!!on line 85 crashes if the item has no associatedcanvasCourse
Also: the return value of enrollLearningLibraryItem is discarded. If the repository returns the post-enrollment state, consider using it to confirm enrollment. If unused, the repository method's return type should be Unit.
// Guard the entire enroll block:
val item = learningLibraryItem ?: run {
_state.update { it.copy(isEnrollLoading = false, loadingState = it.loadingState.copy(isError = true)) }
return@tryLaunch
}
val courseId = item.canvasCourse?.courseId?.toLongOrNull() ?: return@tryLaunch
repository.enrollLearningLibraryItem(item.id)
learnEventHandler.onEvent(LearnEvent.RefreshLearningLibraryList)
_state.update { it.copy(isEnrollLoading = false, navigateToCourseId = courseId) }| onClick = { | ||
| state.updateTypeFilter(LearnLearningLibraryTypeFilter.All) | ||
| state.updateStatusFilter(LearnLearningLibraryStatusFilter.All) | ||
| } |
There was a problem hiding this comment.
Double API call when clearing both filters
Each call to updateTypeFilter and updateStatusFilter independently triggers loadItems(cursor = null) in the ViewModel. This fires two sequential network requests — the first with typeFilter=All, statusFilter=<old>, the second with typeFilter=All, statusFilter=All. Depending on response timing, the first result may briefly appear in the UI.
Consider adding a single onClearFilters() callback that atomically resets both filters before calling loadItems once:
// In ViewModel:
fun clearFilters() {
_uiState.update { it.copy(typeFilter = LearnLearningLibraryTypeFilter.All, statusFilter = LearnLearningLibraryStatusFilter.All) }
loadItems(cursor = null)
}
// In Screen:
onClick = { state.clearFilters() }| CollectionItemType.EXTERNAL_URL -> LearnLearningLibraryCollectionItemChipState( | ||
| label = resources.getString(R.string.learnLearningLibraryExternalLinkLabel), | ||
| color = StatusChipColor.Orange, | ||
| iconRes = R.drawable.text_snippet |
There was a problem hiding this comment.
Wrong icon for EXTERNAL_URL — copy-paste from PAGE
R.drawable.text_snippet is the same icon used for CollectionItemType.PAGE. An external URL should use a link or open-in-browser icon to visually distinguish it from a page. The same issue exists in iconRes() at line 145. Please use a distinct drawable for this type.
| modifier: Modifier = Modifier, | ||
| placeholder: (@Composable () -> Unit)? = { ImagePlaceholder() } | ||
| ) { | ||
| var isImageLoading by rememberSaveable { mutableStateOf(true) } |
There was a problem hiding this comment.
rememberSaveable is wrong for transient image-loading state
rememberSaveable persists state across recompositions and across the composable leaving and re-entering the composition (e.g., navigating away and back). Restoring isImageLoading = true on re-entry would prevent the shimmer from showing the already-loaded image, requiring a fresh Glide request each time.
Use remember instead — this state is inherently transient:
var isImageLoading by remember { mutableStateOf(true) }| if (!imageUrl.isNullOrEmpty()) { | ||
| GlideImage( | ||
| imageUrl, | ||
| contentDescription = null, |
There was a problem hiding this comment.
Missing accessibility content description
contentDescription = null makes all images loaded by this composable invisible to TalkBack/accessibility services. Since this molecule will be used throughout the learning library UI, it should expose a contentDescription parameter:
@Composable
fun LoadingImage(
url: String?,
modifier: Modifier = Modifier,
contentDescription: String? = null,
placeholder: @Composable () -> Unit = { ImagePlaceholder() },
) {
...
GlideImage(
model = url,
contentDescription = contentDescription,
...
)
}| } | ||
|
|
||
| data object LearnLearningLibraryDetailsScreen: LearnRoute() { | ||
| const val collectionIdIdAttr = "collectionId" |
There was a problem hiding this comment.
Typo: duplicated Id in constant name
collectionIdIdAttr has Id repeated. The string value "collectionId" is correct, but the constant name will cause confusion when searching the codebase.
const val collectionIdAttr = "collectionId"| private val eventHandler: LearnEventHandler | ||
| ): ViewModel() { | ||
|
|
||
| private var allCollections: List<LearnLearningLibraryCollectionState> = emptyList() |
There was a problem hiding this comment.
Mutable ViewModel-level field duplicates StateFlow as source of truth
allCollections is a ViewModel-level var that is written by both loadCollections and refreshCollections. Reading it from multiple call sites while the StateFlow already holds the canonical UI state creates two parallel representations of the same data. If a concurrent update occurs, the last write wins silently.
Consider deriving this value solely from _uiState.value.collectionState.collections when needed, or if filtering is required, keep the unfiltered list in UiState and derive the filtered view reactively.
| import androidx.compose.foundation.layout.Row | ||
| import androidx.compose.foundation.layout.fillMaxSize | ||
| import androidx.compose.foundation.layout.padding | ||
| import androidx.compose.material.Text |
There was a problem hiding this comment.
Mixed Material 2 and Material 3 imports
androidx.compose.material.Text (M2) is imported here while the rest of the project uses Material 3. Mixing M2 and M3 components can cause theming inconsistencies — M2 Text will use M2 typography defaults rather than the M3 theme tokens.
import androidx.compose.material3.Text
There was a problem hiding this comment.
PR Review Progress Update
Note: This is a synchronize event review. I was unable to retrieve previous review history in this session, so this covers a fresh analysis of the current PR state.
Summary
This PR implements the Learning Library feature for Horizon (Canvas Career) — a substantial addition including list, enroll, bookmark, details, and completed screens, along with a FakeGetLearningLibraryManager for E2E testing. The architecture is clean and test coverage is good, but there are a few issues that need addressing before merging.
Issues Requiring Changes
-
License headers (all new files) — All new files use GPL v3.0, but
libs/horizonandautomation/espressoshould use Apache License 2.0 per project conventions. See inline comments onLearnLearningLibraryListViewModel.ktandFakeGetLearningLibraryManager.ktas representative examples — applies to all ~30+ new files. -
Crash risk in
LearnLearningLibraryEnrollViewModel(lines 69 and 85) — Force non-null assertions (!!) oncanvasCourse(nullable in the model) andlearningLibraryItem(can be null ifloadDatafailed silently). See inline comments. -
Unused field
learningLibraryCollectionsinFakeGetLearningLibraryManager(line 33) — populated but never read. Remove or use it. -
Inconsistent error handling in
FakeGetLearningLibraryManager.getLearningLibraryItem(line 163) — use.firstOrNull() ?: throw IllegalArgumentException(...)for consistency withenrollLearningLibraryItem. -
Typo:
itemsToDisplays→itemsToDisplayinLearnLearningLibraryListUiState(line 46).
Positive Observations
- Thorough unit test coverage for ViewModels and Repositories
- Consistent MVVM pattern with clean separation of concerns
- Debounced search (300ms) avoids excessive API calls
- Bookmark state is correctly synchronized between both item-list and collection views
- Cursor-based pagination is handled cleanly
- Error states are properly surfaced via snackbar on refresh failures
| * | ||
| * You should have received a copy of the GNU General Public License | ||
| * along with this program. If not, see <http://www.gnu.org/licenses/>. | ||
| * |
There was a problem hiding this comment.
Wrong License Header
This file uses GNU GPL v3.0, but per project guidelines (CLAUDE.md), only the top-level apps (student, teacher, parent) use GPL. The automation/espresso module should use Apache License 2.0, consistent with all other non-app modules.
Existing files in this module (and libs/horizon) use Apache 2.0. Please update the license header to match.
|
|
||
| class FakeGetLearningLibraryManager : GetLearningLibraryManager { | ||
|
|
||
| private val learningLibraryCollections = mutableListOf<LearningLibraryCollection>() |
There was a problem hiding this comment.
Unused field
learningLibraryCollections is populated in initializeMockData() but is never read by any of the override methods — only enrolledCollections and collectionItems are actually used to serve responses. This is dead code and adds confusion. Either remove it or use it where it's needed (e.g., for a potential getLearningLibraryCollections method).
| itemId: String, | ||
| forceNetwork: Boolean | ||
| ): LearningLibraryCollectionItem { | ||
| return collectionItems.values.flatten().first { it.id == itemId } |
There was a problem hiding this comment.
Inconsistent error handling
.first { } throws a generic NoSuchElementException with no helpful message if the item is not found. The enrollLearningLibraryItem method below uses .find {} ?: throw IllegalArgumentException("Item not found: $itemId") which provides a descriptive message. These should be consistent for easier debugging in tests.
// Suggested:
return collectionItems.values.flatten().firstOrNull { it.id == itemId }
?: throw IllegalArgumentException("Item not found: $itemId")|
|
||
| val collectionItem = repository.loadLearningLibraryItem(learningLibraryItemId) | ||
| learningLibraryItem = collectionItem | ||
| val courseDetails = repository.loadCourseDetails(collectionItem.canvasCourse!!.courseId.toLong()) |
There was a problem hiding this comment.
Crash risk: force non-null assertion on nullable canvasCourse
canvasCourse is declared as CanvasCourseInfo? (nullable) in LearningLibraryCollectionItem. Using !! here will throw a NullPointerException if the loaded item doesn't have course details (e.g., for a non-COURSE type item, or if the API returns null).
Also note the // Silent error catch block below (line 76) — if loadLearningLibraryItem fails, learningLibraryItem is never set, yet onEnroll at line 84 will use learningLibraryItem!! which will crash.
Consider using safe call with error surfacing:
val courseId = collectionItem.canvasCourse?.courseId?.toLongOrNull()
?: run {
_state.update { it.copy(loadingState = it.loadingState.copy(isLoading = false, isError = true)) }
return@tryLaunch
}| viewModelScope.tryLaunch { | ||
| _state.update { it.copy(isEnrollLoading = true) } | ||
| repository.enrollLearningLibraryItem(learningLibraryItem!!.id) | ||
| _state.update { it.copy(isEnrollLoading = false, navigateToCourseId = learningLibraryItem!!.canvasCourse!!.courseId.toLong()) } |
There was a problem hiding this comment.
Crash risk: chained force non-null assertions
Two issues here:
learningLibraryItem!!— ifloadDatafailed silently (the catch at line 76 only clears the loading state without settinglearningLibraryItem), tapping "Enroll" will crash withNullPointerException.learningLibraryItem!!.canvasCourse!!—canvasCourseis nullable, so this is a second crash vector even iflearningLibraryItemis set.
Consider guarding with a proper null check and showing an error state instead of crashing:
val item = learningLibraryItem ?: return@tryLaunch
val courseId = item.canvasCourse?.courseId?.toLong() ?: return@tryLaunch| data class LearnLearningLibraryListCollectionUiState( | ||
| val loadingState: LoadingState = LoadingState(), | ||
| val collections: List<LearnLearningLibraryCollectionState> = emptyList(), | ||
| val itemsToDisplays: Int = 1, |
There was a problem hiding this comment.
Typo in field name
itemsToDisplays has an erroneous trailing s — it should be itemsToDisplay. This name propagates to the ViewModel (it.collectionState.itemsToDisplays + collectionPageSize) and the Screen composable (state.collectionState.itemsToDisplays). Worth fixing before this API surface spreads further.
| * | ||
| * You should have received a copy of the GNU General Public License | ||
| * along with this program. If not, see <http://www.gnu.org/licenses/>. | ||
| * |
There was a problem hiding this comment.
Wrong License Header
This file (and all other new files in libs/horizon/) uses GNU GPL v3.0. Per project guidelines (CLAUDE.md), libs/horizon is not a top-level shippable app and should use Apache License 2.0. Existing horizon files like DashboardViewModel.kt already use Apache 2.0 — please update all new files to match.
2 participants
refs: CLXR-58
affects: Student
release note: none