[CLXR-476][Horizon] Offline sync#3674
[CLXR-476][Horizon] Offline sync#3674domonkosadam wants to merge 16 commits intofeature/horizon-offlinefrom
Conversation
🧪 Unit Test Results✅ 📱 Student App
✅ 🌅 Horizon
❌ 📦 Submodules
❌ Failed Tests (2)
📊 Summary
Last updated: Mon, 04 May 2026 11:58:37 GMT |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Summary
This PR delivers the offline sync infrastructure for the Horizon app — a substantial and well-structured addition covering WorkManager-based sync, per-course sync plans, image caching, and offline-aware UI across multiple screens. The overall design is solid. A few issues worth addressing before merge:
Issues Found
- Debug feature flag override (
FeatureFlagProvider.kt) —offlineEnabled()is hardcoded totrue, bypassing the environment flag check entirely. Needs to be reverted before merging to avoid enabling offline for all users/environments. - Missing Room migration scripts (
HorizonDatabase.kt) — Version jumps from 15 → 18 with 5 new tables but no migration objects visible. Existing installs will crash without explicit migrations or a declaredfallbackToDestructiveMigration. -
clearAllTables()clears user settings (DeleteSyncedContentUseCase.kt) — Deleting synced content also wipeshorizon_sync_settings, resetting the user's sync frequency and wifi-only preference to defaults. - Unstructured coroutine scope in Worker (
HorizonOfflineSyncWorker.kt) —CoroutineScope(Dispatchers.Default)inobserveProgress()is not bound to the worker's lifecycle and won't be cancelled by WorkManager cancellation. UsecoroutineContextfrom theCoroutineWorker. - Navigation in composable body (
DeleteContentScreen.kt) —navController.popBackStack/navigatecalls are inside the composition directly (not in aLaunchedEffect), which will re-trigger on every recomposition whileisComplete = true. - Hash collision risk in image cache filenames (
ImageSyncer.kt) —url.hashCode()is 32-bit; two different URLs can produce the same filename, silently serving the wrong cached image. - Direct DAO access in ViewModel (
ManageOfflineContentViewModel.kt) —HorizonCourseSyncPlanDaoandHorizonFileSyncPlanDaoare injected directly; consider wrapping in a repository/use case for consistency and testability. - Null user ID path in file storage (
HorizonFileSyncRepository.downloadFileByUrl) —apiPrefs.user?.id.toString()evaluates to"null"when user is absent, writing files to an untracked directory that cleanup helpers won't find.
Positive Highlights
NET_CAPABILITY_VALIDATED+onCapabilitiesChangedinNetworkStateProvideris a meaningful accuracy improvement overNET_CAPABILITY_INTERNET+onAvailable— correctly handles captive portals.Mutex-guarded file ID accumulation inHorizonCourseSync.syncCourseis the right approach for concurrent coroutine launches modifying shared sets.@Volatile var isStoppedonHorizonCourseSyncis correct for safe cross-thread visibility.OfflineCardStateHelperis a clean abstraction that centralises the "am I offline + is this course synced + resolve image URL" logic rather than duplicating it across three ViewModels.ImageSyncerchunking (6 concurrent downloads) is a sensible rate-limit.- Test coverage is properly updated for all changed ViewModels and repositories.
|
|
||
| suspend fun offlineEnabled(): Boolean { | ||
| return checkEnvironmentFeatureFlag(FEATURE_FLAG_OFFLINE) && !apiPrefs.canvasForElementary | ||
| return true//checkEnvironmentFeatureFlag(FEATURE_FLAG_OFFLINE) && !apiPrefs.canvasForElementary |
There was a problem hiding this comment.
Debug override left in production code. The real feature flag check is commented out — offlineEnabled() now unconditionally returns true for all users and environments. This should be reverted before merging:
return checkEnvironmentFeatureFlag(FEATURE_FLAG_OFFLINE) && !apiPrefs.canvasForElementary| HorizonAssignmentCommentAttachmentEntity::class, | ||
| HorizonSubmissionEntity::class, | ||
| HorizonSubmissionAttachmentEntity::class, | ||
| HorizonSyncSettingsEntity::class, |
There was a problem hiding this comment.
Missing Room migration scripts. The version jumps from 15 → 18 (adding 5 new tables), but no migration objects are visible in this diff. Without addMigrations(...) or fallbackToDestructiveMigration() declared in the dagger/Room builder, existing installs will crash with IllegalStateException: A migration from 15 to 18 was required but not found.
If destructive migration is acceptable here (e.g. this is still pre-release), please make it explicit. If users need their data preserved, the migration scripts need to be added.
| syncHelper.cancelPeriodicSync() | ||
|
|
||
| withContext(Dispatchers.IO) { | ||
| database.clearAllTables() |
There was a problem hiding this comment.
clearAllTables() wipes every table in the database, including horizon_sync_settings and horizon_user. After the user deletes their synced content, they will lose their configured sync frequency and wifi-only preference — those will reset to defaults on next open.
Consider only clearing the tables that hold synced content (course data, files, images, module items, sync plans, sync metadata) while preserving horizon_sync_settings.
|
|
||
| private fun observeProgress() { | ||
| observerJob = CoroutineScope(Dispatchers.Default).launch { | ||
| aggregateProgressObserver.progressData.collect { data -> |
There was a problem hiding this comment.
Creating a new CoroutineScope(Dispatchers.Default) here is unstructured concurrency — this scope is not tied to the worker's lifecycle and will not be cancelled if the worker is cancelled via the normal WorkManager path.
Use the worker's own coroutine context instead:
observerJob = CoroutineScope(coroutineContext + Dispatchers.Default).launch {Or rely on the CoroutineWorker's built-in structured coroutine scope by keeping the observer as part of the doWork suspending call rather than a side-launched job.
| fun DeleteContentScreen( | ||
| navController: NavHostController, | ||
| viewModel: DeleteContentViewModel = hiltViewModel(), | ||
| ) { |
There was a problem hiding this comment.
Navigation calls in the composable body (outside a LaunchedEffect) will execute on every recomposition while isComplete is true, which can trigger multiple redundant popBackStack / navigate calls and cause a crash or an unexpected back-stack state.
Wrap this in a LaunchedEffect:
LaunchedEffect(isComplete) {
if (isComplete) {
navController.popBackStack(AccountRoute.ManageOfflineContent.route, inclusive = true)
navController.navigate(AccountRoute.ManageOfflineContent.route)
}
}| .take(4) | ||
| .ifEmpty { "jpg" } | ||
| val destFile = File(dir, "${url.hashCode()}.$ext") | ||
|
|
There was a problem hiding this comment.
url.hashCode() is a 32-bit signed integer, so different URLs can produce the same hash code. When two URLs collide, the second image will silently re-use the first image's cached file. Consider using a more collision-resistant key, e.g. a URL-safe Base64 of the URL's SHA-256, or simply url.replace(Regex("[^a-zA-Z0-9]"), "_").takeLast(100).
| @ApplicationContext private val context: Context, | ||
| private val getCoursesWithFilesUseCase: GetCoursesWithFilesUseCase, | ||
| private val getDeviceStorageUseCase: GetDeviceStorageUseCase, | ||
| private val courseSyncPlanDao: HorizonCourseSyncPlanDao, |
There was a problem hiding this comment.
The ViewModel is injecting HorizonCourseSyncPlanDao and HorizonFileSyncPlanDao directly. This bypasses the repository layer that the rest of the codebase uses for data access. Consider wrapping these DAO operations in a repository or use case so the ViewModel isn't coupled directly to the database layer — and to make this unit-testable without Room artifacts.
| Row( | ||
| verticalAlignment = Alignment.CenterVertically, | ||
| horizontalArrangement = Arrangement.spacedBy(8.dp), | ||
| modifier = Modifier |
There was a problem hiding this comment.
CourseSyncState.SYNCING and CourseSyncState.PENDING render identically (both show a Spinner). If this is intentional it's fine, but consider whether a distinct visual indicator for "queued/waiting" vs "actively downloading" would help the user understand the overall progress.
| suspend fun downloadFileByUrl(fileId: Long, courseId: Long, url: String, displayName: String) { | ||
| if (localFileDao.findById(fileId) != null) return | ||
| val dir = File(context.filesDir, apiPrefs.user?.id.toString()).also { it.mkdirs() } | ||
| val destFile = File(dir, "${fileId}_$displayName") |
There was a problem hiding this comment.
If apiPrefs.user is null, apiPrefs.user?.id.toString() evaluates to the string "null", and files will be placed in <filesDir>/null/. This directory won't be cleaned up correctly by CourseCleanupHelper or DeleteSyncedContentUseCase (which looks for <filesDir>/<userId>). Guard against this:
val userId = apiPrefs.user?.id ?: return
val dir = File(context.filesDir, userId.toString()).also { it.mkdirs() }
|
Student Install Page |
1 participant
refs: CLXR-476
affects: Student
release note: none