Skip to content

[CLX-3200][Horizon] ModuleItemSequence screen notebook changes #3369

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 11 commits into from
Nov 11, 2025
Merged

[CLX-3200][Horizon] ModuleItemSequence screen notebook changes #3369

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
a59d968
WebView UI changes
domonkosadam Nov 4, 2025
b5a4e0f
Edit webview actionbar options
domonkosadam Nov 4, 2025
b5b47e2
Remove notebook dialog
domonkosadam Nov 4, 2025
39be5d9
Refactor url
domonkosadam Nov 5, 2025
0e9f426
Fix findings
domonkosadam Nov 5, 2025
bbcf04d
Fix tests
domonkosadam Nov 5, 2025
e31b63c
Merge branch 'master' into CLX-3200-ModuleItemSequence-screen-Noteboo…
domonkosadam Nov 10, 2025
0111ede
Fix wideLayout
domonkosadam Nov 10, 2025
c6254fd
Fix flag handling
domonkosadam Nov 10, 2025
1aca263
Fix tests
domonkosadam Nov 10, 2025
fb02ec0
Fix tests
domonkosadam Nov 10, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 10 additions & 22 deletions ...main/java/com/instructure/horizon/features/moduleitemsequence/ModuleItemSequenceScreen.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,6 @@ import androidx.compose.material3.ExperimentalMaterial3Api
import androidx.compose.material3.Scaffold
import androidx.compose.material3.SnackbarHost
import androidx.compose.material3.SnackbarHostState
import androidx.compose.material3.SnackbarResult
import androidx.compose.material3.Surface
import androidx.compose.material3.Text
import androidx.compose.runtime.Composable
Expand Down Expand Up @@ -101,7 +100,7 @@ import com.instructure.horizon.features.moduleitemsequence.content.lti.ExternalT
import com.instructure.horizon.features.moduleitemsequence.content.page.PageDetailsContentScreen
import com.instructure.horizon.features.moduleitemsequence.content.page.PageDetailsViewModel
import com.instructure.horizon.features.moduleitemsequence.progress.ProgressScreen
import com.instructure.horizon.features.notebook.NotebookBottomDialog
import com.instructure.horizon.features.notebook.navigation.NotebookRoute
import com.instructure.horizon.horizonui.foundation.HorizonColors
import com.instructure.horizon.horizonui.foundation.HorizonCornerRadius
import com.instructure.horizon.horizonui.foundation.HorizonElevation
Expand All @@ -122,14 +121,12 @@ import com.instructure.horizon.horizonui.molecules.PillType
import com.instructure.horizon.horizonui.molecules.Spinner
import com.instructure.horizon.horizonui.molecules.SpinnerSize
import com.instructure.horizon.horizonui.platform.LoadingStateWrapper
import com.instructure.horizon.navigation.MainNavigationRoute
import com.instructure.pandautils.compose.modifiers.conditional
import com.instructure.pandautils.utils.Const
import com.instructure.pandautils.utils.ThemePrefs
import com.instructure.pandautils.utils.ViewStyler
import com.instructure.pandautils.utils.getActivityOrNull
import com.instructure.pandautils.utils.orDefault
import kotlinx.coroutines.launch
import kotlin.math.abs

@Composable
Expand All @@ -153,7 +150,15 @@ fun ModuleItemSequenceScreen(mainNavController: NavHostController, uiState: Modu
onPreviousClick = uiState.onPreviousClick,
onAssignmentToolsClick = uiState.onAssignmentToolsClick,
onAiAssistClick = { uiState.updateShowAiAssist(true) },
onNotebookClick = { uiState.updateShowNotebook(true) },
onNotebookClick = {
Copy link

@claude claude bot Nov 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

mainNavController.navigate(
NotebookRoute.Notebook.route(
uiState.courseId.toString(),
uiState.objectTypeAndId.first,
uiState.objectTypeAndId.second
)
)
Comment on lines 153 to 163
Copy link

@claude claude bot Nov 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
    }
},

},
notebookEnabled = uiState.notebookButtonEnabled,
aiAssistEnabled = uiState.aiAssistButtonEnabled,
hasUnreadComments = uiState.hasUnreadComments
Expand All @@ -166,22 +171,6 @@ fun ModuleItemSequenceScreen(mainNavController: NavHostController, uiState: Modu
onDismiss = { uiState.updateShowAiAssist(false) },
)
}
if (uiState.showNotebook) {
NotebookBottomDialog(
uiState.courseId,
uiState.objectTypeAndId,
{ snackbarMessage, onDismiss ->
scope.launch {
if (snackbarMessage != null) {
val result = snackbarHostState.showSnackbar(snackbarMessage)
if (result == SnackbarResult.Dismissed) {
onDismiss()
}
}
} },
{ uiState.updateShowNotebook(false) }
)
}
ModuleItemSequenceContent(uiState = uiState, mainNavController = mainNavController, onBackPressed = {
mainNavController.popBackStack()
})
Expand Down Expand Up @@ -646,7 +635,6 @@ private fun ModuleItemSequenceScreenPreview() {
moduleItemContent = ModuleItemContent.Assignment(courseId = 1, assignmentId = 1L)
),
updateShowAiAssist = {},
updateShowNotebook = {},
)
)
}
2 changes: 0 additions & 2 deletions ...ain/java/com/instructure/horizon/features/moduleitemsequence/ModuleItemSequenceUiState.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,10 +36,8 @@ data class ModuleItemSequenceUiState(
val assignmentToolsOpened: () -> Unit = {},
val showAiAssist: Boolean = false,
val aiAssistButtonEnabled: Boolean = false,
val showNotebook: Boolean = false,
val notebookButtonEnabled: Boolean = false,
Copy link

@claude claude bot Nov 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good cleanup removing unused state properties. The removal of showNotebook and updateShowNotebook is consistent with the architectural change from bottom sheet to full navigation.

val updateShowAiAssist: (Boolean) -> Unit,
val updateShowNotebook: (Boolean) -> Unit,
val objectTypeAndId: Pair<String, String> = Pair("", ""),
val updateObjectTypeAndId: (Pair<String, String>) -> Unit = {},
val hasUnreadComments: Boolean = false,
Expand Down
5 changes: 0 additions & 5 deletions ...n/java/com/instructure/horizon/features/moduleitemsequence/ModuleItemSequenceViewModel.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,6 @@ class ModuleItemSequenceViewModel @Inject constructor(
onAssignmentToolsClick = ::onAssignmentToolsClicked,
assignmentToolsOpened = ::assignmentToolsOpened,
updateShowAiAssist = ::updateShowAiAssist,
updateShowNotebook = ::updateShowNotebook,
updateObjectTypeAndId = ::updateNotebookObjectTypeAndId,
updateAiAssistContext = ::updateAiAssistContext,
)
Expand Down Expand Up @@ -550,10 +549,6 @@ class ModuleItemSequenceViewModel @Inject constructor(
_uiState.update { it.copy(showAiAssist = show) }
}

private fun updateShowNotebook(show: Boolean) {
_uiState.update { it.copy(showNotebook = show) }
}

private fun updateNotebookObjectTypeAndId(objectTypeAndId: Pair<String, String>) {
_uiState.update {
it.copy(
Expand Down
50 changes: 14 additions & 36 deletions .../instructure/horizon/features/moduleitemsequence/content/page/PageDetailsContentScreen.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,9 +30,6 @@ import androidx.compose.ui.draw.clip
import androidx.compose.ui.platform.LocalContext
import androidx.compose.ui.unit.dp
import androidx.navigation.NavHostController
import com.instructure.canvasapi2.managers.graphql.horizon.redwood.NoteHighlightedData
import com.instructure.canvasapi2.managers.graphql.horizon.redwood.NoteHighlightedDataRange
import com.instructure.canvasapi2.managers.graphql.horizon.redwood.NoteHighlightedDataTextPosition
import com.instructure.horizon.features.aiassistant.common.model.AiAssistContextSource
import com.instructure.horizon.features.notebook.common.webview.ComposeNotesHighlightingCanvasWebView
import com.instructure.horizon.features.notebook.common.webview.NotesCallback
Expand Down Expand Up @@ -114,40 +111,21 @@ fun PageDetailsContentScreen(
)
},
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(
Copy link

@claude claude bot Nov 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

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
)
Comment on lines 113 to 127
Copy link

@claude claude bot Nov 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 call uiState.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:

  1. Documenting this UX change in the PR description
  2. Updating the AddNotebook screen to detect when noteType is pre-set and skip unnecessary input fields
  3. Consider if this aligns with user expectations and usage patterns

}
)
}
)
)
Expand Down
44 changes: 0 additions & 44 deletions libs/horizon/src/main/java/com/instructure/horizon/features/notebook/NotebookDialog.kt
View file
Open in desktop

This file was deleted.

13 changes: 11 additions & 2 deletions libs/horizon/src/main/java/com/instructure/horizon/features/notebook/NotebookViewModel.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,12 +16,14 @@
*/
package com.instructure.horizon.features.notebook

import androidx.lifecycle.SavedStateHandle
import androidx.lifecycle.ViewModel
import androidx.lifecycle.viewModelScope
import com.instructure.canvasapi2.utils.weave.catch
import com.instructure.canvasapi2.utils.weave.tryLaunch
import com.instructure.horizon.features.notebook.common.model.NotebookType
import com.instructure.horizon.features.notebook.common.model.mapToNotes
import com.instructure.horizon.features.notebook.navigation.NotebookRoute
import com.instructure.redwood.QueryNotesQuery
import com.instructure.redwood.type.OrderDirection
import dagger.hilt.android.lifecycle.HiltViewModel
Expand All @@ -33,12 +35,13 @@ import javax.inject.Inject
@HiltViewModel
class NotebookViewModel @Inject constructor(
private val repository: NotebookRepository,
savedStateHandle: SavedStateHandle,
): ViewModel() {
private var cursorId: String? = null
private var pageInfo: QueryNotesQuery.PageInfo? = null

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>? = getObjectTypeAndId(savedStateHandle)

private val _uiState = MutableStateFlow(NotebookUiState(
loadPreviousPage = ::getPreviousPage,
Expand Down Expand Up @@ -140,4 +143,10 @@ class NotebookViewModel @Inject constructor(
_uiState.update { it.copy(showTopBar = true, showFilters = true) }
}
}

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)
}
}
18 changes: 2 additions & 16 deletions ...ructure/horizon/features/notebook/common/webview/ComposeNotesHighlightingCanvasWebView.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,7 +121,7 @@ fun ComposeNotesHighlightingCanvasWebView(
)
if (notes.none { intersects(it.highlightedText.textPosition.start to it.highlightedText.textPosition.end, selectedTextStart to selectedTextEnd) }){
add(
ActionMenuItem(2, context.getString(R.string.notesActionMenuAddImportantNote)) {
ActionMenuItem(2, context.getString(R.string.notesActionMenuMarkImportantNote)) {
notesCallback.onNoteAdded(
selectedText,
NotebookType.Important.name,
Expand All @@ -135,7 +135,7 @@ fun ComposeNotesHighlightingCanvasWebView(
}
)
add(
ActionMenuItem(3, context.getString(R.string.notesActionMenuAddConfusingNote)) {
ActionMenuItem(3, context.getString(R.string.notesActionMenuMarkConfusingNote)) {
notesCallback.onNoteAdded(
selectedText,
NotebookType.Confusing.name,
Expand All @@ -148,20 +148,6 @@ fun ComposeNotesHighlightingCanvasWebView(
)
}
)
Copy link

@claude claude bot Nov 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

add(
ActionMenuItem(4, context.getString(R.string.notesActionMenuAddNote)) {
notesCallback.onNoteAdded(
selectedText,
null,
selectedTextRangeStartContainer,
selectedTextRangeStartOffset,
selectedTextRangeEndContainer,
selectedTextRangeEndOffset,
selectedTextStart,
selectedTextEnd
)
}
)
}
}
}
Expand Down
Loading
Loading