[CLXR-475][Horizon] Learn offline mode

[domonkosadam]

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

[github-actions]

🧪 Unit Test Results

✅ 📱 Student App

• Tests: 1252 total, 0 failed, 0 skipped
• Duration: 0.000s
• Success Rate: 100%

✅ 🌅 Horizon

• Tests: 788 total, 0 failed, 0 skipped
• Duration: 41.803s
• Success Rate: 100%

✅ 📦 Submodules

• Tests: 3345 total, 0 failed, 0 skipped
• Duration: 56.578s
• Success Rate: 100%

---

📊 Summary

• Total Tests: 5385
• Failed: 0
• Skipped: 0
• Status: ✅ All tests passed!

Last updated: Wed, 22 Apr 2026 15:22:19 GMT

[claude]

Review: Horizon Data Layer Refactor

Good architectural direction overall — splitting local and network datasources is a clean separation of concerns, and the addition of a @Synchronized database provider and a safeValueOf enum helper are welcome improvements.

Issues to address

• Index-based course/enrollment pairing (FakeGetHorizonCourseManager.kt:101) — positional coupling between courses and enrollments lists is fragile; a map keyed by courseId would be safer (inline comment added).
• Progress unit mismatch (FakeGetHorizonCourseManager.kt:143) — values changed from 0–1 fractions to 0–100 percentages; confirm the expected scale matches the API contract and display logic.
• Cache-miss crash (CourseDetailsLocalDataSource.kt:42) — throwing IllegalStateException on an empty cache will crash on first launch or after reinstall; return null and let the caller handle the missing data.
• Asymmetric enum fallback (CourseDetailsLocalDataSource.kt:148) — unknown variant defaults to LINEAR (potentially wrong), while unknown enrollmentStatus becomes null; at minimum, log unrecognised values so they surface during QA.
• Silent item drops in CollectionItemType.safeValueOf (CollectionItemType.kt:29) — when combined with mapNotNull, unknown server-sent types are silently discarded; a Crashlytics non-fatal or an UNKNOWN sentinel entry would make this visible.
• allowMainThreadQueries() in test modules (HorizonOfflineTestModule.kt:50, TestModule.kt:106) — suppresses Room's threading checks, hiding potential main-thread DB access bugs; consider running at least some local datasource tests without it.
• Missing unit tests for new local datasources — CourseDetailsLocalDataSource, CourseProgressLocalDataSource, CourseScoreLocalDataSource, and LearnLearningLibraryLocalDataSource have no corresponding test files; offline sync correctness is hard to verify without them.
• fallbackToDestructiveMigration() with a large version jump (HorizonDatabaseProvider.kt) — jumping from version 2 to 7 with destructive migration will wipe all cached offline data for existing users on update. Define explicit migrations or document that data loss is acceptable here.

Positive aspects

• Clean local/network datasource split following the project's repository pattern.
• @Synchronized on getDatabase prevents race conditions during concurrent access.
• safeValueOf is a good defensive pattern for enum deserialization.
• In-memory DB in test modules is a solid improvement over NotImplementedError stubs.

[claude]

This index-based pairing of courses and enrollments is brittle. If the two lists ever differ in length (or ordering changes), unrelated courses/enrollments get silently paired — or enrollments are dropped with no error.

Consider mapping enrollments by a stable key (e.g. enrollmentId tied to courseId) rather than relying on positional alignment:

val enrollmentByCourseId = enrollments.associateBy { it.courseId }
val dashboardEnrollments = courses.mapNotNull { course ->
    val enrollment = enrollmentByCourseId[course.id] ?: return@mapNotNull null
    ...
}

[claude]

The progress values changed from fractions (0.25, 1.0) to percentages (25.0, 100.0). If the real API contract uses a 0–1 scale this mismatch will cause test assertions to diverge from production behaviour (and vice-versa). Please confirm the expected unit and ensure both the mock data and the display logic use the same scale consistently.

[claude]

Throwing IllegalStateException on a cache miss will crash the app for first-time users or after a fresh install where the local DB is empty. Consider returning null (or a Result) and letting the caller decide how to handle the missing data — e.g. by triggering a network fetch rather than crashing:

suspend fun getCourse(courseId: Long): CourseWithProgress? {
    return courseDao.getByCourseId(courseId)?.toModel()
}

[claude]

The error handling here is asymmetric: an unknown variant silently defaults to LINEAR, while an unknown enrollmentStatus silently becomes null. Defaulting to LINEAR is a potentially wrong assumption — if the server adds a new variant type, all affected courses will appear as linear.

Consider logging the unrecognised value (or recording it to Crashlytics) so it surfaces during development rather than silently corrupting displayed data.

[claude]

safeValueOf returning null for unknown types is good defensive practice, but callers that use mapNotNull { it.toModel() } will silently drop collection items the server sends with a type the client doesn't recognise yet. This can make future API additions invisible to users without any indication something was skipped.

Consider adding a Crashlytics log (non-fatal) or an UNKNOWN fallback entry so unknown items surface during QA rather than disappearing silently in production.

[claude]

.allowMainThreadQueries() disables Room's main-thread safety guard. This is fine for keeping tests simple, but it means any accidental main-thread DB access in production code won't be caught here. If threading correctness is important for offline sync, consider running at least a subset of the new local datasource tests without this flag so regressions are caught earlier.

[github-actions]

Student Install Page