[CLXR-455][Horizon] Implement offline POC by domonkosadam · Pull Request #3620 · instructure/canvas-android

domonkosadam · 2026-04-02T11:56:50Z

refs: CLXR-455
affects: Student
release note: none

github-actions · 2026-04-02T11:57:03Z

🧪 Unit Test Results

📊 Summary

Total Tests: 0
Failed: 0
Skipped: 0
Status: ⚠️ No test results found

Last updated: Thu, 02 Apr 2026 11:57:12 GMT

claude

Review: Offline-First Dashboard Architecture

This PR introduces a well-structured offline-first layer for the Horizon dashboard — Room database persistence, repository/datasource pattern, use cases, and an offline banner UI. The separation of concerns is clear and the MVVM pattern is consistently followed. A few things worth addressing before this ships:

Issues

sync() not implemented in all three repositories (CourseEnrollmentRepository, ModuleItemRepository, ProgramRepository) — TODO("Not yet implemented") throws NotImplementedError at runtime if called. See inline comments.
Silent -1 user ID fallback in CourseEnrollmentNetworkDataSource (and duplicated in HorizonOfflineModule) — when apiPrefs.user is null the GraphQL query receives -1, which will produce a silent bad request rather than a clear error. See inline comment.
LearningObjectType.valueOf() can crash in ModuleItemLocalDataSource if the database contains a string that doesn't match an enum constant. ProgramLocalDataSource uses safeValueOf for the same pattern — this should match. See inline comment.
fallbackToDestructiveMigration() in HorizonDatabaseProvider — this silently deletes all cached data on any schema version bump. There is already a version 2 defined in HorizonDatabase. For an offline-first feature this is high-impact data loss. See inline comment.
Typo in OfflineBanner.kt — preview function named OfflineBannerNoDDatePreview should be OfflineBannerNoDatePreview. See inline comment.
SyncPolicy enum (SyncPolicy.kt) is defined but never referenced anywhere in this PR. If it's placeholder for future work, a tracking reference would help; otherwise it's dead code.
Missing unit tests for the new offline logic: OfflineSyncRepository.shouldFetchFromNetwork() decision branches, CourseEnrollmentRepository returning cached data when offline, GetDashboardCoursesUseCase orchestration (especially the invitation acceptance flow), and DashboardMapper changes for DashboardEnrollment.

Minor

The auto-invite-acceptance behaviour is carried over from the old DashboardCourseViewModel — noted in an inline comment purely for documentary purposes, no change required.
HorizonOfflineModule also uses apiPrefs.user?.id ?: -1L when providing the HorizonDatabase instance — same concern as the datasource fallback.
No newline at end of file in HorizonTypeConverters.kt and HorizonSyncMetadataEntity.kt.

Overall the architecture is solid and the refactor from ViewModel-level data fetching to proper use cases is a good step forward. The main blockers are the unimplemented sync() methods and the enum crash risk in ModuleItemLocalDataSource.

claude · 2026-04-02T12:12:45Z

+    }
+
+    override suspend fun sync() {
+        TODO("Not yet implemented")


This TODO will throw a NotImplementedError at runtime if sync() is ever called. The base class OfflineSyncRepository defines this as an abstract method that is part of the offline sync contract. Either implement it or, if intentionally deferred, throw a more informative exception or leave a tracked issue reference here so it doesn't silently ship as dead code.

override suspend fun sync() { // TODO: Implement full enrollment sync - tracked in ISSUE-XXXX throw UnsupportedOperationException("Enrollment sync not yet implemented") }

claude · 2026-04-02T12:12:50Z

+    }
+
+    override suspend fun sync() {
+        TODO("Not yet implemented")


Same concern as in CourseEnrollmentRepository — TODO("Not yet implemented") will crash at runtime with a NotImplementedError if sync() is invoked. Please either implement or explicitly mark as deferred with a tracking reference.

claude · 2026-04-02T12:12:58Z

+    }
+
+    override suspend fun sync() {
+        TODO("Not yet implemented")


Same concern as other repositories — TODO("Not yet implemented") will crash at runtime. Please implement or add a tracked issue reference.

claude · 2026-04-02T12:13:04Z

+
+    suspend fun getEnrollments(): List<DashboardEnrollment> {
+        return horizonGetCoursesManager.getDashboardEnrollments(
+            userId = apiPrefs.user?.id ?: -1,


apiPrefs.user?.id ?: -1 will silently pass -1 as a user ID to the GraphQL query when the user is not logged in, likely producing an empty or error response that is hard to debug. Consider throwing instead so callers can handle the unauthenticated case explicitly:

userId = apiPrefs.user?.id ?: error("User must be logged in to fetch enrollments"),

claude · 2026-04-02T12:13:12Z

+            moduleItemId = entity.moduleItemId,
+            courseId = entity.courseId,
+            title = entity.moduleItemTitle,
+            type = LearningObjectType.valueOf(entity.moduleItemType),


LearningObjectType.valueOf(entity.moduleItemType) will throw IllegalArgumentException if the stored string doesn't match an enum constant — for example after a future enum rename or if the database already contains a value from a schema that predates this one. ProgramLocalDataSource uses safeValueOf for the same pattern; please apply the same defensive approach here:

type = LearningObjectType.safeValueOf(entity.moduleItemType),

(or filter out rows with unknown types before mapping them)

claude · 2026-04-02T12:13:20Z

+    fun getDatabase(userId: Long): HorizonDatabase {
+        return dbMap.getOrPut(userId) {
+            Room.databaseBuilder(context, HorizonDatabase::class.java, "horizon-db-$userId")
+                .fallbackToDestructiveMigration()


fallbackToDestructiveMigration() will silently wipe all cached offline data when the schema version is bumped (e.g. version 1 → 2 already exists in HorizonDatabase). For an offline-first feature this means users lose their cached content on every schema change without warning. If full migration scripts aren't ready yet, at a minimum document this decision and track the migration work. A lightweight migration that just drops and recreates tables can also be written to make the data-loss intent explicit rather than accidental.

claude · 2026-04-02T12:13:26Z

+
+@Preview
+@Composable
+private fun OfflineBannerNoDDatePreview() {


Typo in preview function name: NoDDate should be NoDate.

private fun OfflineBannerNoDatePreview() {

claude · 2026-04-02T12:13:36Z

+
+        val invitations = enrollments.filter { it.enrollmentState == DashboardEnrollment.STATE_INVITED }
+        if (invitations.isNotEmpty()) {
+            invitations.forEach { enrollment ->


Invitations are being accepted automatically without user confirmation. This was already the behaviour in DashboardCourseViewModel before this refactor, so the behaviour isn't changing — but it's worth flagging since this is now more prominently a domain-layer concern. If silent auto-acceptance is intentional per product spec, a brief comment here would help future readers understand why (e.g. "Canvas Career automatically accepts all pending invitations on dashboard load").

tamaskozmer · 2026-04-16T13:48:27Z

-): ViewModel() {
+    private val getDashboardCoursesUseCase: GetDashboardCoursesUseCase,
+    private val dashboardEventHandler: DashboardEventHandler,
+    networkStateProvider: NetworkStateProvider,


You can use composition instead. Creating a class that encapsulates the behavior of the HorizonOfflineViewModel, in that case you won't need to pass all the parameters to the ViewModels instead just pass that new class.

tamaskozmer · 2026-04-16T13:48:57Z

+    private val networkStateProvider: NetworkStateProvider,
+    private val featureFlagProvider: FeatureFlagProvider,
+    private val getLastSyncedAtUseCase: GetLastSyncedAtUseCase,
 ) : ViewModel() {


Shouldn't we use HorizonOfflineViewModel here as well?

tamaskozmer · 2026-04-16T13:53:39Z

+    private val syncMetadataDao: HorizonSyncMetadataDao,
+) : BaseUseCase<SyncDataType, Long?>() {
+
+    override suspend fun execute(params: SyncDataType): Long? = syncMetadataDao.getLastSyncedAt(params)


If I remember correctly we agreed with Ákos to have the params as an inner class of the Use Case as a convention. I am not sure if this is applicable where we have a single parameter only, but I see some occurrences that suggest this, for example GetAuthenticatedSessionUseCase. What do you think @hermannakos ?

Yeah, I would go with the inner class for consistency at least. We didn't follow this convention at first since we didn't have that, so there might be some where it is not followed

tamaskozmer · 2026-04-16T13:55:08Z

+
+class ProgramRepository @Inject constructor(
+    private val networkDataSource: ProgramNetworkDataSource,
+    private val localDataSource: ProgramLocalDataSource,


Why don't we have a data source interface like in the current offline implementation?

domonkosadam added 10 commits March 30, 2026 15:04

Implement course widget basic architecture

5375c00

Migrate to usecase pattern

fb2b3f9

Refactor architecture

1951eea

Improve architecture

1072fe3

Add sync functionality

df03097

Refactor daos

544244c

Improve entity handling

bdce3e9

Refactor feature keys

2144299

Implement sync date usecase

63c70e2

Update FeatureFlagProvider.kt

9d6bf2d

domonkosadam requested a review from andrasmaczak April 2, 2026 11:56

domonkosadam added the horizon label Apr 2, 2026

claude bot reviewed Apr 2, 2026

View reviewed changes

tamaskozmer approved these changes Apr 16, 2026

View reviewed changes

Conversation

domonkosadam commented Apr 2, 2026

Uh oh!

github-actions bot commented Apr 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🧪 Unit Test Results

📊 Summary

Uh oh!

claude bot left a comment

Choose a reason for hiding this comment

Review: Offline-First Dashboard Architecture

Issues

Minor

Uh oh!

claude bot Apr 2, 2026

Choose a reason for hiding this comment

Uh oh!

claude bot Apr 2, 2026

Choose a reason for hiding this comment

Uh oh!

claude bot Apr 2, 2026

Choose a reason for hiding this comment

Uh oh!

claude bot Apr 2, 2026

Choose a reason for hiding this comment

Uh oh!

claude bot Apr 2, 2026

Choose a reason for hiding this comment

Uh oh!

claude bot Apr 2, 2026

Choose a reason for hiding this comment

Uh oh!

claude bot Apr 2, 2026

Choose a reason for hiding this comment

Uh oh!

claude bot Apr 2, 2026

Choose a reason for hiding this comment

Uh oh!

tamaskozmer Apr 16, 2026

Choose a reason for hiding this comment

Uh oh!

tamaskozmer Apr 16, 2026

Choose a reason for hiding this comment

Uh oh!

tamaskozmer Apr 16, 2026

Choose a reason for hiding this comment

Uh oh!

hermannakos Apr 16, 2026

Choose a reason for hiding this comment

Uh oh!

tamaskozmer Apr 16, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

github-actions bot commented Apr 2, 2026 •

edited

Loading