[CLX-3438][Horizon] Notebook changes #3427
Conversation
🧪 Unit Test Results✅ 📱 Student App
✅ 🌅 Horizon
✅ 📦 Submodules
📊 Summary
Last updated: Tue, 09 Dec 2025 13:32:27 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 implements notebook navigation improvements, including the ability to scroll to a specific note when navigating from the notebook list to the content page. The changes also include icon updates and string resource renaming.
Positive Aspects
✅ Good architectural improvement - moving note creation logic from navigation to state management
✅ Clean parameter threading through the navigation stack
✅ Proper use of Compose state management with rememberSaveable and LaunchedEffect
✅ Test coverage updated to reflect the new date formatting behavior
✅ Consistent icon changes across multiple screens for better UX
Issues to Address
- Timing issue with scroll animation (ComposeNotesHighlightingCanvasWebView.kt:112) - The hardcoded 500ms delay may not be reliable on slower devices
- No retry mechanism for failed scrolls (ComposeNotesHighlightingCanvasWebView.kt:114) - If the first scroll attempt fails, users can't reach their note
- JavaScript function could be more efficient (JSTextSelectionInterface.kt:691) - Use
querySelectorinstead of iterating through all highlights - Potential XSS concern (JSTextSelectionInterface.kt:692) - Ensure noteId is properly escaped in the JavaScript context
- Incorrect timestamp behavior (AddNoteViewModel.kt:62) - lastModifiedDate is set at ViewModel initialization, not when the note is saved
- Missing verification (PageDetailsContentScreen.kt:120) - Need to confirm that
uiState.addNote()properly handles navigation, errors, and user feedback
Code Quality
- Navigation parameter passing is clean and well-structured
- State management follows Compose best practices
- The JavaScript injection pattern is consistent with existing code
Recommendations
- Consider making the scroll delay configurable or using a WebView ready callback
- Add error handling and retry logic for the scroll-to-note feature
- Move the timestamp creation to the actual save operation
- Verify the
addNotefunction implementation includes proper error handling
Overall, this is a solid feature implementation with good architectural decisions, but the issues listed above should be addressed to ensure reliability and security.
| } | ||
| } | ||
|
|
||
| LaunchedEffect(scrollToNoteId, isPageLoaded) { |
There was a problem hiding this comment.
The hardcoded 500ms delay might not be sufficient in all cases, especially on slower devices or with complex page content. Consider:
- Using a more reliable mechanism like waiting for the WebView's content height to stabilize
- Making this configurable or using a larger safe default (e.g., 1000ms)
- Adding a callback from the WebView when it's truly ready for scrolling operations
|
|
||
| LaunchedEffect(scrollToNoteId, isPageLoaded) { | ||
| delay(500) | ||
| val noteId = scrollToNoteId |
There was a problem hiding this comment.
The isScrolled flag prevents multiple scroll attempts, but if the first scroll fails (e.g., note not found or WebView not ready), there's no retry mechanism. Consider adding error handling or a limited retry mechanism to ensure users can reach their note even if the first attempt fails.
| } | ||
| } | ||
There was a problem hiding this comment.
The getNotePosition function returns -1 when a note is not found, but this could theoretically be a valid y-position (though unlikely). Consider using null or a more explicit sentinel value to indicate "not found" state. Also, this function could be more efficient by using querySelector with the data attribute:
const highlight = document.querySelector(`[data-note-id="${noteId}"]`);
if (highlight) {
const rect = highlight.getBoundingClientRect();
return rect.top + window.scrollY;
}
return -1;| } | ||
| function getNotePosition(noteId) { |
There was a problem hiding this comment.
The noteId passed to getNotePosition should be escaped to prevent potential XSS issues if a malicious noteId contains quotes or special characters. While JSONObject.quote() is used in the Kotlin code, ensure that the noteId in the JavaScript function is also properly escaped. Consider using template literals more carefully or validating the noteId format.
| it.copy( | ||
| title = context.getString(R.string.createNoteTitle), | ||
| hasContentChange = true, | ||
| lastModifiedDate = Date().toNotebookLocalisedDateFormat(), |
There was a problem hiding this comment.
Creating a new Date() here means the lastModifiedDate is set when the ViewModel initializes, not when the note is actually saved. This could lead to inconsistent timestamps if the user takes time to write their note. Consider moving this to the actual save operation in the parent class or wherever the note creation happens.
| textSelectionEnd = textSelectionEnd, | ||
| noteType = noteType | ||
| ) | ||
| uiState.addNote( |
There was a problem hiding this comment.
The onNoteAdded callback now calls uiState.addNote() instead of navigating directly. This is good architectural change, but ensure that:
- The
addNotefunction properly handles all the navigation and state management - Error handling is in place if the note creation fails
- The user receives feedback about the note creation status
Could you verify that uiState.addNote is implemented and handles these cases?
domonkosadam
changed the title
📊 Code Coverage Report✅ Student
✅ Teacher
✅ Pandautils
📈 Overall Average
|
|
Student Install Page |
|
The scroll to item is still not perfect, but can be improved later (if needed) |
refs: CLX-3438
affects: Student
release note: none