[CLXR-575][Horizon] Download notes to database#3682
Conversation
🧪 Unit Test Results✅ 📱 Student App
✅ 🌅 Horizon
✅ 📦 Submodules
📊 Summary
Last updated: Tue, 12 May 2026 07:13:07 GMT |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Review Progress Update
This is a follow-up review on the synchronize event (latest commit: "Fix findings"). I was unable to fetch my prior review threads through the GitHub MCP tools available in this session, so I cannot mechanically tick off which earlier findings were resolved — the "Fix findings" commit message suggests prior feedback has been addressed, and the diff is consistent with that.
What I can confirm from the current state of the PR
-
FeatureFlagProvider.offlineEnabled()is now actually checking the env feature flag andcanvasForElementaryinstead of being hard-coded totrue— good fix. -
DeleteContentScreennow wraps the post-delete navigation inLaunchedEffect(isComplete)instead of callingpopBackStack/navigateinline during composition — the prior pattern was an anti-pattern and this is correct. -
NotebookViewModel,AddNoteViewModel,EditNoteViewModelnow extendHorizonOfflineViewModeland gate write operations (addNote,editNote,deleteNote) withisOffline()checks, surfacing a user-facing snackbar via the newnotebookOfflineActionUnavailablestring. - Direct
RedwoodApiManager/repository wiring in ViewModels has been replaced by use cases (AddNoteUseCase,EditNoteUseCase,GetNotesUseCase, etc.), aligning with the existing architecture. - Test coverage has been substantially refreshed (
NotebookRepositoryTestindata/repository/,NotebookSyncerTest,NotebookNetworkDataSourceTest, plus ViewModel offline-path tests).
Remaining concerns / new feedback (inline comments added)
-
NotebookLocalDataSource.toEntitysilently coerces an unparseablecourseIdto0L. This will cross-pollute course-scoped queries. -
HorizonCourseSyncswallowsnotebookSyncer.syncNotesfailures withcatch (_: Exception)and no log — matches the convention used by sibling syncers but makes field debugging hard. -
DashboardScreenremoved theenabled = !uiState.isOffline/offlineDisabledmodifier on the top-bar icon button while the rest of the PR consistently adds offline-disable semantics — please confirm this is intentional. -
PageDetailsViewModel.fetchNotescatches genericExceptionand returns an empty list, indistinguishable from "no notes" — logging would help.
Nits / observations (not posted inline)
NotebookLocalDataSource.parseHighlightedDatasimilarly swallowsGsondeserialization errors. Same general suggestion: at minimum log so corrupted local rows surface in telemetry.PageDetailsViewModel.NOTES_PAGE_LIMIT = 100fetches up to 100 notes on every page view; if a single page accumulates more than that, the user will silently miss notes. A "load more" affordance or paging on this screen could be a follow-up.SaveOfflineSyncPlanUseCase.Params.Coursedoes not surface asyncNotesflag and relies on the entity default (true). That's fine today but couples the use-case API to the entity defaults — consider making it explicit ifsyncNotesever becomes user-selectable.
Overall this is a well-structured offline-sync slice — clear separation of Local/Network data sources, repository orchestration, dedicated syncer, and a HorizonOfflineViewModel base that the notebook ViewModels can extend cleanly. Posting this as a review rather than a plain PR comment because the workflow's allowed-tool list does not include the issue-comment tool.
| objectId = node.objectId, | ||
| objectType = node.objectType, | ||
| userText = node.userText.orEmpty(), | ||
| reaction = when (node.reaction?.firstOrNull()) { |
There was a problem hiding this comment.
node.courseId.toLongOrNull() ?: 0L silently buckets notes with malformed courseId under 0L. If the network ever returns an unexpected value, those rows would (a) be persisted and queried as if they belonged to a fictitious course 0, and (b) be invisible to the per-course offline reads. Consider either skipping the edge (filter it out of the entities mapped into replaceNotesForCourse) or surfacing the parse failure rather than coercing.
| try { | ||
| notebookSyncer.syncNotes(plan.courseId) | ||
| courseSyncPlanDao.updateNotesState(plan.courseId, HorizonProgressState.COMPLETED) | ||
| } catch (_: Exception) { |
There was a problem hiding this comment.
catch (_: Exception) here silently discards the underlying failure — the same pattern is used for the other syncers, so this matches existing convention, but it makes diagnosing partial sync failures impossible. If logging is available elsewhere in the syncers, mirroring that here would help future debugging (e.g., a low-level breadcrumb tagged with courseId and the syncer name). Not blocking, just flagging since this is new code being added.
| @@ -271,8 +271,6 @@ private fun DashboardTopBar(uiState: DashboardUiState, navController: NavControl | |||
| }, | |||
| color = IconButtonColor.Inverse, | |||
| elevation = HorizonElevation.level4, | |||
There was a problem hiding this comment.
The enabled = !uiState.isOffline and modifier = Modifier.offlineDisabled(uiState.isOffline) props were removed from this IconButton. Given the rest of this PR adds offline-disable semantics to many other surfaces (delete buttons, edit, etc.), removing it here looks asymmetric — was it intentional that this top-bar action should remain enabled while offline? If yes, a brief comment justifying it would help. If no, this is a regression of the prior offline-gating.
|
|
||
| companion object { | ||
| private const val NOTES_PAGE_LIMIT = 100 | ||
| } |
There was a problem hiding this comment.
fetchNotes swallows all exceptions and returns emptyList(). With the new repository falling back to local data when offline, the only paths that should reach the catch here are real (parsing, mapping) failures — which today look identical to "no notes." Even just logging the exception before swallowing would make this much easier to diagnose if it ever misbehaves in the field. Same pattern exists in refreshNotes/fetchNotes — consider centralizing.
|
Student Install Page |
| ) | ||
| page.edges?.let { all.addAll(it) } | ||
| cursor = if (page.pageInfo.hasNextPage) page.pageInfo.endCursor else null | ||
| } while (cursor != null) |
There was a problem hiding this comment.
Potential infinite loop in pagination. If the API ever returns hasNextPage = true together with endCursor = null (server bug, partial response, etc.), cursor stays null and the same first page is fetched forever — blocking the sync coroutine and any worker calling it.
Consider gating the loop on a cursor that actually advanced, e.g.:
val next = if (page.pageInfo.hasNextPage) page.pageInfo.endCursor else null
if (next == cursor) break
cursor = next| reaction = when (node.reaction?.firstOrNull()) { | ||
| NoteReaction.Confusing.value -> NotebookType.Confusing.name | ||
| else -> NotebookType.Important.name | ||
| }, |
There was a problem hiding this comment.
Unknown reactions are silently coerced to Important. Any reaction value that isn't exactly NoteReaction.Confusing.value — including null/empty list, an unexpected casing, or a future reaction the server might add — gets persisted as NotebookType.Important. After a sync the user's locally cached notes can look different from the server's truth (e.g. a "Helpful" note would render as "Important" offline).
If Important truly is the only safe default, please log/Crashlytics-record the unexpected value so we'll notice when new reaction types ship. Otherwise consider storing the raw reaction string and resolving it at read time.
| val node = edge.node | ||
| return HorizonNoteEntity( | ||
| id = node.id, | ||
| courseId = node.courseId.toLongOrNull() ?: 0L, |
There was a problem hiding this comment.
Silent fallback to courseId = 0L orphans the note. If node.courseId can't be parsed as Long, the entity is persisted with courseId = 0L. That row will never be returned by a courseId = realId filter and will never be cleaned up by deleteByCourseId(realId) or CourseCleanupHelper.cleanupCourseContent(realId) — it lingers in horizon_notes indefinitely.
Course IDs are server-controlled Longs, so a parse failure here indicates a malformed payload. Preferable to skip the edge (return null from toEntity, filter at the call site) and record the malformed input rather than persist a ghost row.
| if (shouldSync()) { | ||
| val entities = response.edges.orEmpty().map { NotebookLocalDataSource.toEntity(it) } | ||
| localDataSource.upsertNotes(entities) | ||
| } |
There was a problem hiding this comment.
Server-side deletions are not reconciled into the local cache during online browsing. Each page fetch upsertAlls the response — but a note deleted on the server simply doesn't appear in subsequent responses, so its stale copy stays in horizon_notes until the next full NotebookSyncer.syncNotes cycle for that course (which only runs from HorizonCourseSync, not from regular browsing).
If the user then drops offline, they see zombie notes that no longer exist. Two options:
- Treat the network-while-online path as read-through only and let
NotebookSyncerbe the sole writer (drop the writethrough here). - When fetching the first page for a
(courseId, filter)combination, do a course-scoped replace instead ofupsertAll.
| viewModelScope.tryLaunch { | ||
| _uiState.update { | ||
| it.copy(loadingState = it.loadingState.copy(isLoading = true)) | ||
| it.copy(loadingState = it.loadingState.copy(isLoading = true), isOnline = networkStateProvider.isOnline()) |
There was a problem hiding this comment.
isOnline is snapshotted, not observed. It's read once at construction and again on loadData(), but the VM doesn't subscribe to networkStateProvider.isOnlineLiveData. If the user loses connectivity while a page is open, uiState.isOnline stays true and canMarkNotes in ComposeNotesHighlightingCanvasWebView continues to surface the "Mark Important / Confusing" action — invoking it goes to addNote, which silently fails in the catch {} on line 144 with no UI feedback.
Either observe isOnlineLiveData here (like HorizonOfflineViewModel does), or guard addNote with an isOffline() check that surfaces the standard notebookOfflineActionUnavailable snackbar.
1 participant
refs: CLXR-575
affects: Student
release note: none