[CLX-3200][Horizon] ModuleItemSequence screen notebook changes #3369
Conversation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Summary
I've reviewed PR #3369 which refactors the Notebook feature from a bottom sheet dialog to full-screen navigation. Overall, this is a good architectural improvement that simplifies state management and improves navigation flow.
Key Findings
🐛 Bug Found:
- Line 40 in
NotebookRoute.kthas a typo in URL parameter construction that needs fixing
- Missing validation before navigation in
ModuleItemSequenceScreen.kt - Complex initialization logic in
NotebookViewModel.ktcould be simplified
📝 Significant UX Changes:
- Removal of quick-action workflow: Users can no longer instantly mark text as Important/Confusing without going through an additional screen
- Removal of "Add note" context menu option: The ability to add custom notes directly from text selection has been removed
- Visual design changes: Removed icon badges (flag/question mark) in favor of underline styles only
Recommendations
- Fix the bug in
NotebookRoute.ktline 40 (critical) - Consider UX impact: The removal of quick-actions may impact user efficiency. Consider whether the AddNotebook screen can detect pre-set note types and streamline the flow
- Document changes: These UX changes should be clearly documented in the PR description for stakeholder review
- Design review: Ensure the visual indicator changes (removal of icon badges) have been validated by design/UX team
Positive Aspects
✅ Clean removal of unused state (showNotebook, updateShowNotebook)
✅ Proper use of navigation instead of dialog state management
✅ Good use of SavedStateHandle for ViewModel initialization
✅ String resource updates are appropriately named
✅ Proper cleanup of unused imports
The architectural direction is solid, but please address the bug and consider the UX implications before merging.
| objectType: String? = null, | ||
| objectId: String? = null | ||
| ): String { | ||
| return "$ROUTE?COURSE_ID=${courseId}&$OBJECT_TYPE=${objectType}&$OBJECT_ID=${objectId}" |
There was a problem hiding this comment.
Bug: Incorrect URL parameter name
There's a typo in the route construction on line 40. It should use the $COURSE_ID constant instead of hardcoding "COURSE_ID":
return "$ROUTE?$COURSE_ID=${courseId}&$OBJECT_TYPE=${objectType}&$OBJECT_ID=${objectId}"This will ensure the URL parameter name matches the constant definition and the route pattern.
|
|
||
| private var courseId: Long? = null | ||
| private var objectTypeAndId: Pair<String, String>? = null | ||
| private var courseId: Long? = savedStateHandle.get<String>(NotebookRoute.Notebook.COURSE_ID)?.toLongOrNull() | ||
| private var objectTypeAndId: Pair<String, String>? = if ( | ||
| savedStateHandle.get<String>(NotebookRoute.Notebook.OBJECT_TYPE) != null && | ||
| savedStateHandle.get<String>(NotebookRoute.Notebook.OBJECT_ID) != null | ||
| ) { | ||
| Pair( | ||
| savedStateHandle.get<String>(NotebookRoute.Notebook.OBJECT_TYPE)!!, | ||
| savedStateHandle.get<String>(NotebookRoute.Notebook.OBJECT_ID)!! | ||
| ) | ||
| } else { | ||
| null |
There was a problem hiding this comment.
Code quality: Complex initialization could be simplified
The initialization logic for objectTypeAndId is verbose and could be more readable. Consider extracting to a helper function:
private fun getObjectTypeAndId(savedStateHandle: SavedStateHandle): Pair<String, String>? {
val objectType = savedStateHandle.get<String>(NotebookRoute.Notebook.OBJECT_TYPE) ?: return null
val objectId = savedStateHandle.get<String>(NotebookRoute.Notebook.OBJECT_ID) ?: return null
return Pair(objectType, objectId)
}Then use: private var objectTypeAndId: Pair<String, String>? = getObjectTypeAndId(savedStateHandle)
This makes the code more testable and easier to maintain.
| onNotebookClick = { | ||
| mainNavController.navigate( | ||
| NotebookRoute.Notebook.route( | ||
| uiState.courseId.toString(), | ||
| uiState.objectTypeAndId.first, | ||
| uiState.objectTypeAndId.second | ||
| ) | ||
| ) |
There was a problem hiding this comment.
Potential issue: Missing validation before navigation
Good refactoring to use navigation instead of a dialog! However, consider adding validation before navigating. The objectTypeAndId might not always be properly initialized, which could lead to navigation with invalid parameters.
Consider adding a guard:
onNotebookClick = {
if (uiState.courseId > 0 &&
uiState.objectTypeAndId.first.isNotEmpty() &&
uiState.objectTypeAndId.second.isNotEmpty()) {
mainNavController.navigate(
NotebookRoute.Notebook.route(
uiState.courseId.toString(),
uiState.objectTypeAndId.first,
uiState.objectTypeAndId.second
)
)
} else {
// Log error or show user feedback
}
},| text-decoration-color: rgba(14, 104, 179, 1); | ||
| position: relative; | ||
| } | ||
| .highlighted-important::before { | ||
| content: ""; | ||
| display: block; | ||
| position: absolute; | ||
| top: -6px; | ||
| left: -6px; | ||
| width: 12px; | ||
| height: 12px; | ||
| border-radius: 50%; | ||
| z-index: 10; | ||
| background-color: rgba(14, 104, 179, 1); | ||
| background-image: ${flagBase64Source}; | ||
| background-size: 60%; | ||
| background-repeat: no-repeat; | ||
| background-position: center; | ||
| } | ||
| .highlighted-confusing { | ||
| background-color: rgba(199, 31, 35, 0.2); | ||
| text-decoration: underline; | ||
| text-decoration-color: rgba(199, 31, 35, 1); | ||
| text-decoration-style: dashed; | ||
| position: relative; | ||
| } |
There was a problem hiding this comment.
Design change: Removal of visual indicator icons
The CSS changes remove the flag and question mark badge icons that previously appeared on highlighted text. This is a significant visual design change.
Before:
- Important notes: Blue highlight + flag icon badge
- Confusing notes: Red highlight + question icon badge
After:
- Important notes: Blue highlight + solid underline
- Confusing notes: Red highlight + dashed underline
Trade-offs:
✅ Cleaner, less intrusive appearance
✅ Still visually distinguishable via underline style
❌ Less discoverable - icons were more immediately recognizable
❌ Potentially less accessible - badges were stronger visual indicators
Recommendation: Ensure this change has been reviewed by your design team and consider whether any user research/testing supports this direction. The dashed underline is subtle and users might not immediately understand the difference between note types.
| onNoteAdded = { selectedText, noteType, startContainer, startOffset, endContainer, endOffset, textSelectionStart, textSelectionEnd -> | ||
| if (noteType == null) { | ||
| mainNavController.navigate( | ||
| NotebookRoute.AddNotebook( | ||
| courseId = uiState.courseId.toString(), | ||
| objectType = "Page", | ||
| objectId = uiState.pageId.toString(), | ||
| highlightedTextStartOffset = startOffset, | ||
| highlightedTextEndOffset = endOffset, | ||
| highlightedTextStartContainer = startContainer, | ||
| highlightedTextEndContainer = endContainer, | ||
| highlightedText = selectedText, | ||
| textSelectionStart = textSelectionStart, | ||
| textSelectionEnd = textSelectionEnd, | ||
| noteType = noteType | ||
| ) | ||
| ) | ||
| } else { | ||
| uiState.addNote( | ||
| NoteHighlightedData( | ||
| selectedText = selectedText, | ||
| range = NoteHighlightedDataRange( | ||
| startOffset = startOffset, | ||
| endOffset = endOffset, | ||
| startContainer = startContainer, | ||
| endContainer = endContainer | ||
| ), | ||
| textPosition = NoteHighlightedDataTextPosition( | ||
| start = textSelectionStart, | ||
| end = textSelectionEnd | ||
| ) | ||
| ), | ||
| noteType | ||
| mainNavController.navigate( | ||
| NotebookRoute.AddNotebook( | ||
| courseId = uiState.courseId.toString(), | ||
| objectType = "Page", | ||
| objectId = uiState.pageId.toString(), | ||
| highlightedTextStartOffset = startOffset, | ||
| highlightedTextEndOffset = endOffset, | ||
| highlightedTextStartContainer = startContainer, | ||
| highlightedTextEndContainer = endContainer, | ||
| highlightedText = selectedText, | ||
| textSelectionStart = textSelectionStart, | ||
| textSelectionEnd = textSelectionEnd, | ||
| noteType = noteType | ||
| ) |
There was a problem hiding this comment.
Workflow change: Removal of quick-action note creation
This refactoring removes the conditional logic that previously enabled two different note creation paths:
Before:
- If
noteType == null: Navigate to AddNotebook screen (user adds custom note) - If
noteType != null: Directly calluiState.addNote()(quick mark as Important/Confusing)
After:
- Always navigate to AddNotebook screen regardless of
noteType
Impact:
This removes the quick-action workflow where users could immediately mark text as Important or Confusing with a single tap. Now users must always go through the AddNotebook screen, even if they just want to mark text without adding comments.
Question: Was this intentional? The quick-action workflow provided a streamlined experience for simple marking. If removed intentionally, consider:
- Documenting this UX change in the PR description
- Updating the AddNotebook screen to detect when
noteTypeis pre-set and skip unnecessary input fields - Consider if this aligns with user expectations and usage patterns
There was a problem hiding this comment.
Summary
This PR refactors the Notebook feature from a bottom sheet dialog to full-screen navigation. Overall, the changes improve the architecture and code organization, but there are some concerns about functionality changes and user experience.
Positive Changes ✅
- Better Architecture: Moving from bottom sheet to proper navigation improves the navigation flow and makes the feature more maintainable
- SavedStateHandle Usage: Using
SavedStateHandleinNotebookViewModelfollows Android best practices and improves testability - Code Cleanup: Removed unused state properties and simplified the codebase
- Clearer String Resources: Updated action labels ("Mark important" vs "Add important note") better describe the user action
Concerns & Questions ⚠️
-
Feature Removal: The "Add note" option has been removed from the text selection menu. Users can now only mark text as "Important" or "Confusing" but cannot add a custom note with highlighted text. Is this intentional?
-
User Flow Change: Previously, when users marked text as Important/Confusing, the note was created immediately without navigation. Now all note additions require navigating to a new screen, which adds friction to the user experience.
-
Visual Design Changes: The decorative flag/question icons have been removed in favor of simple underlines (solid for important, dashed for confusing). While cleaner, this may reduce visual clarity.
-
Null Handling: The route construction could result in "null" strings in URLs. Consider using optional parameters more carefully.
-
Error Handling: No error handling for navigation failures when constructing routes with potentially invalid data.
Test Coverage
The PR doesn't include test updates. Consider adding or updating:
- Unit tests for
NotebookViewModelwithSavedStateHandle - Navigation tests for the new routing logic
- Tests verifying the note creation flow
Recommendation
The architectural improvements are solid, but please clarify whether the functionality changes (removal of "Add note" option and the change to always navigate) are intentional or regressions. If intentional, ensure this aligns with product requirements and UX design.
libs/horizon/src/main/java/com/instructure/horizon/features/notebook/NotebookViewModel.kt
Show resolved
Hide resolved
| objectType: String? = null, | ||
| objectId: String? = null | ||
| ): String { | ||
| return "$ROUTE?$COURSE_ID=${courseId}&$OBJECT_TYPE=${objectType}&$OBJECT_ID=${objectId}" |
There was a problem hiding this comment.
The route construction with nullable parameters could lead to "null" strings in the URL if parameters are not provided. Consider using optional parameters differently:
fun route(
courseId: String? = null,
objectType: String? = null,
objectId: String? = null
): String {
val params = buildList {
courseId?.let { add("$COURSE_ID=$it") }
objectType?.let { add("$OBJECT_TYPE=$it") }
objectId?.let { add("$OBJECT_ID=$it") }
}
return if (params.isNotEmpty()) "$ROUTE?${params.joinToString("&")}" else ROUTE
}This prevents "null" strings appearing in the navigation route.
| onAssignmentToolsClick = uiState.onAssignmentToolsClick, | ||
| onAiAssistClick = { uiState.updateShowAiAssist(true) }, | ||
| onNotebookClick = { uiState.updateShowNotebook(true) }, | ||
| onNotebookClick = { |
There was a problem hiding this comment.
This navigation change is cleaner than the previous bottom sheet approach. However, consider error handling if any of these values are invalid:
onNotebookClick = {
try {
mainNavController.navigate(
NotebookRoute.Notebook.route(
uiState.courseId.toString(),
uiState.objectTypeAndId.first,
uiState.objectTypeAndId.second
)
)
} catch (e: Exception) {
// Handle navigation error
}
}This prevents crashes if the route construction or navigation fails.
| selectedTextEnd | ||
| ) | ||
| } | ||
| ) |
There was a problem hiding this comment.
Removing the "Add note" option (ActionMenuItem with id=4) changes the user experience. Users can now only mark text as "Important" or "Confusing" without the ability to add a custom note with highlighted text.
Question: Is this intentional? This seems like a significant feature removal. If users want to add a note with context but without marking it as important/confusing, they no longer have that option.
| text-decoration: underline; | ||
| text-decoration-color: rgba(14, 104, 179, 1); | ||
| position: relative; | ||
| } |
There was a problem hiding this comment.
Removing the decorative indicators (flag and question mark icons) for highlighted text changes the visual design significantly. The new approach uses only underlines:
- Important: solid underline (blue)
- Confusing: dashed underline (red)
While this simplifies the CSS and removes the base64-encoded SVG images, it may make it harder for users to quickly distinguish between note types at a glance. The previous icons provided immediate visual recognition.
Consider: User testing to ensure the new visual design is clear enough for users to distinguish note types.
| ) | ||
| ), | ||
| noteType | ||
| mainNavController.navigate( |
There was a problem hiding this comment.
The logic has been simplified by removing the conditional check for noteType == null. Now all note additions navigate to AddNotebook route.
Question: What happened to the previous logic that handled quick note creation when noteType was provided? The old code would call uiState.addNote() directly for typed notes, which seems more efficient than always navigating to a new screen.
This change forces users through an additional navigation step even when they've already selected a note type (Important/Confusing). Was this intentional?
| val showAiAssist: Boolean = false, | ||
| val aiAssistButtonEnabled: Boolean = false, | ||
| val showNotebook: Boolean = false, | ||
| val notebookButtonEnabled: Boolean = false, |
There was a problem hiding this comment.
Good cleanup removing unused state properties. The removal of showNotebook and updateShowNotebook is consistent with the architectural change from bottom sheet to full navigation.
| <string name="notesActionMenuAddNote">Add note</string> | ||
| <string name="notesActionMenuAddImportantNote">Important</string> | ||
| <string name="notesActionMenuAddConfusingNote">Confusing</string> | ||
| <string name="notesActionMenuMarkImportantNote">Mark important</string> |
There was a problem hiding this comment.
String changes look good and more accurately reflect the user action:
- "Add important note" → "Mark important"
- "Add confusing note" → "Mark confusing"
These are clearer about what the action does to the selected text.
📊 Code Coverage Report✅ Student
✅ Teacher
✅ Pandautils
📈 Overall Average
|
There was a problem hiding this comment.
Review Summary
This PR refactors the Notebook feature from a modal bottom sheet approach to a full navigation-based implementation. Overall, the changes are well-structured and improve the architecture. Here are the key observations:
✅ Strengths
- Better architecture: Moving from modal bottom sheet to proper navigation improves state management and back stack handling
- SavedStateHandle usage: Proper use of
SavedStateHandlefor navigation parameters ensures process death survival - Test improvements: Good fix using
useUnmergedTree = trueto prevent flaky tests - UX improvements: String changes ("Mark important" vs "Important") are clearer to users
- Code cleanup: Removed unused imports and simplified conditional logic
🤔 Areas to Consider
-
UX Flow Change: The removal of quick-add functionality means users can no longer immediately mark text as Important/Confusing without navigation. All note actions now require navigating to a new screen. Is this the intended UX change?
-
Visual Indicators: The CSS changes remove icon indicators (flag/question mark) for note types, relying only on color and underline styles. This may reduce visual distinction between note types.
-
Error Handling: Consider adding validation or error handling for navigation parameters, especially when
courseIdor other required parameters might be null. -
Test Coverage: The existing tests were updated, but consider adding tests for the new SavedStateHandle parameter extraction logic and edge cases.
📝 Recommendation
The code quality is good and the refactoring makes sense architecturally. The main questions are around intentional UX changes (removal of quick-add, simplified visual indicators). If these changes are part of the design direction, this PR looks ready to merge after addressing the inline suggestions.
|
|
||
| // Verify zero hours is not displayed | ||
| composeTestRule.onNodeWithText("0").assertDoesNotExist() | ||
| composeTestRule.onNodeWithText("0", useUnmergedTree = true).assertDoesNotExist() |
There was a problem hiding this comment.
Good fix! Using useUnmergedTree = true is the correct approach when dealing with nested composables where text nodes might be in different semantic tree levels. This prevents flaky tests.
|
|
||
| private fun getViewModel(): NotebookViewModel { | ||
| return NotebookViewModel(repository) | ||
| return NotebookViewModel(repository, savedStateHandle) |
There was a problem hiding this comment.
Test update: Good that you updated the test to match the new constructor signature with SavedStateHandle.
Suggestion: Consider adding tests for the new navigation parameter extraction logic:
- Test when
courseId/objectType/objectIdare provided via SavedStateHandle - Test when they are null
- Test edge cases (empty strings, invalid formats, etc.)
|
Student Install Page |
libs/horizon/src/main/java/com/instructure/horizon/features/notebook/NotebookViewModel.kt
Show resolved
Hide resolved
|
Student Install Page |
|
Student Install Page |
domonkosadam
deleted the
CLX-3200-ModuleItemSequence-screen-Notebook-changes
branch
refs: CLX-3200
affects: Horizon
release note: none