Skip to content
Original file line number Diff line number Diff line change
Expand Up @@ -231,7 +231,7 @@ private fun HomeScreenTopBar(uiState: DashboardUiState, mainNavController: NavCo
)
Spacer(modifier = Modifier.weight(1f))
IconButton(
iconRes = R.drawable.menu_book_notebook,
iconRes = R.drawable.edit_note,
contentDescription = stringResource(R.string.a11y_dashboardNotebookButtonContentDescription),
onClick = {
mainNavController.navigate(MainNavigationRoute.Notebook.route)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ fun DashboardTimeSpentSection(
}
DashboardItemState.ERROR -> {
DashboardWidgetCardError(
stringResource(R.string.dashboardTimeSpentTitle),
stringResource(R.string.dashboardTimeLearningTitle),
R.drawable.schedule,
HorizonColors.PrimitivesHoney.honey12(),
false,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ fun DashboardTimeSpentCardContent(
modifier: Modifier = Modifier,
) {
DashboardWidgetCard(
stringResource(R.string.dashboardTimeSpentTitle),
stringResource(R.string.dashboardTimeLearningTitle),
R.drawable.schedule,
HorizonColors.PrimitivesHoney.honey12(),
modifier,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -305,6 +305,7 @@ private fun ModuleItemSequenceContent(
uiState.assignmentToolsOpened,
updateAiContext = uiState.updateAiAssistContext,
updateNotebookContext = uiState.updateObjectTypeAndId,
scrollToNoteId = uiState.scrollToNoteId
)
}
}
Expand Down Expand Up @@ -403,6 +404,7 @@ private fun ModuleItemContentScreen(
assignmentToolsOpened: () -> Unit,
updateAiContext: (AiAssistContextSource, String) -> Unit,
updateNotebookContext: (Pair<String, String>) -> Unit,
scrollToNoteId: String?,
modifier: Modifier = Modifier
) {
if (moduleItemUiState.isLoading) {
Expand Down Expand Up @@ -461,7 +463,8 @@ private fun ModuleItemContentScreen(
uiState = uiState,
scrollState = scrollState,
updateAiContext = { source, content -> updateAiContext(source, content) },
mainNavController = mainNavController
mainNavController = mainNavController,
scrollToNoteId = scrollToNoteId
)
}
composable(
Expand Down Expand Up @@ -565,7 +568,7 @@ private fun ModuleItemSequenceBottomBar(
onClick = onAiAssistClick,
)
if (showNotebookButton) IconButton(
iconRes = R.drawable.menu_book_notebook,
iconRes = R.drawable.edit_note,
enabled = notebookEnabled,
color = IconButtonColor.Inverse,
elevation = HorizonElevation.level4,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ data class ModuleItemSequenceUiState(
val hasUnreadComments: Boolean = false,
val updateAiAssistContext: (AiAssistContextSource, String) -> Unit = { _, _ -> },
val shouldRefreshPreviousScreen: Boolean = false,
val scrollToNoteId: String? = null
)

data class ModuleItemUiState(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -66,13 +66,15 @@ class ModuleItemSequenceViewModel @Inject constructor(
private val moduleItemId = savedStateHandle.toRoute<MainNavigationRoute.ModuleItemSequence>().moduleItemId
private val moduleItemAssetType = savedStateHandle.toRoute<MainNavigationRoute.ModuleItemSequence>().moduleItemAssetType
private val moduleItemAssetId = savedStateHandle.toRoute<MainNavigationRoute.ModuleItemSequence>().moduleItemAssetId
private val scrollToNoteId = savedStateHandle.toRoute<MainNavigationRoute.ModuleItemSequence>().scrollToNoteId

private var courseProgressChanged = false

private val _uiState =
MutableStateFlow(
ModuleItemSequenceUiState(
courseId = courseId,
scrollToNoteId = scrollToNoteId,
loadingState = LoadingState(onRefresh = ::refresh),
onPreviousClick = ::previousClicked,
onNextClick = ::nextClicked,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,9 @@ 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 All @@ -51,7 +54,8 @@ fun PageDetailsContentScreen(
scrollState: ScrollState,
updateAiContext: (AiAssistContextSource, String) -> Unit,
mainNavController: NavHostController,
modifier: Modifier = Modifier
scrollToNoteId: String?,
modifier: Modifier = Modifier,
) {
val activity = LocalContext.current.getActivityOrNull()
LaunchedEffect(uiState.urlToOpen) {
Expand Down Expand Up @@ -81,6 +85,7 @@ fun PageDetailsContentScreen(
ComposeNotesHighlightingCanvasWebView(
content = "<div id=\"parent-container\"><div>$it</div></div>",
notes = uiState.notes,
scrollToNoteId = scrollToNoteId,
applyOnWebView = {
activity?.let { addVideoClient(it) }
overrideHtmlFormatColors = HorizonColors.htmlFormatColors
Expand Down Expand Up @@ -112,20 +117,21 @@ fun PageDetailsContentScreen(
)
},
onNoteAdded = { selectedText, noteType, startContainer, startOffset, endContainer, endOffset, textSelectionStart, textSelectionEnd ->
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
)
uiState.addNote(
Copy link

Choose a reason for hiding this comment

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

The onNoteAdded callback now calls uiState.addNote() instead of navigating directly. This is good architectural change, but ensure that:

  1. The addNote function properly handles all the navigation and state management
  2. Error handling is in place if the note creation fails
  3. The user receives feedback about the note creation status

Could you verify that uiState.addNote is implemented and handles these cases?

NoteHighlightedData(
selectedText = selectedText,
range = NoteHighlightedDataRange(
startOffset = startOffset,
endOffset = endOffset,
startContainer = startContainer,
endContainer = endContainer
),
textPosition = NoteHighlightedDataTextPosition(
start = textSelectionStart,
end = textSelectionEnd
)
),
noteType
)
}
)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -216,6 +216,7 @@ fun NotebookScreen(
courseId = note.courseId,
moduleItemAssetType = note.objectType.value,
moduleItemAssetId = note.objectId,
scrollToNoteId = note.id
)
)
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,11 +27,13 @@ import com.instructure.canvasapi2.utils.weave.catch
import com.instructure.canvasapi2.utils.weave.tryLaunch
import com.instructure.horizon.R
import com.instructure.horizon.features.notebook.addedit.AddEditViewModel
import com.instructure.horizon.features.notebook.common.composable.toNotebookLocalisedDateFormat
import com.instructure.horizon.features.notebook.common.model.NotebookType
import com.instructure.horizon.features.notebook.navigation.NotebookRoute
import dagger.hilt.android.lifecycle.HiltViewModel
import dagger.hilt.android.qualifiers.ApplicationContext
import kotlinx.coroutines.flow.update
import java.util.Date
import javax.inject.Inject

@HiltViewModel
Expand All @@ -57,6 +59,7 @@ class AddNoteViewModel @Inject constructor(
it.copy(
title = context.getString(R.string.createNoteTitle),
hasContentChange = true,
lastModifiedDate = Date().toNotebookLocalisedDateFormat(),
Copy link

Choose a reason for hiding this comment

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

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.

highlightedData = NoteHighlightedData(
selectedText = highlightedText,
range = NoteHighlightedDataRange(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ import androidx.compose.runtime.getValue
import androidx.compose.runtime.mutableIntStateOf
import androidx.compose.runtime.mutableStateOf
import androidx.compose.runtime.remember
import androidx.compose.runtime.rememberCoroutineScope
import androidx.compose.runtime.rememberUpdatedState
import androidx.compose.runtime.saveable.rememberSaveable
import androidx.compose.runtime.setValue
Expand All @@ -45,6 +46,7 @@ import com.instructure.horizon.features.notebook.common.model.Note
import com.instructure.horizon.features.notebook.common.model.NotebookType
import com.instructure.horizon.features.notebook.common.webview.JSTextSelectionInterface.Companion.addTextSelectionInterface
import com.instructure.horizon.features.notebook.common.webview.JSTextSelectionInterface.Companion.evaluateTextSelectionInterface
import com.instructure.horizon.features.notebook.common.webview.JSTextSelectionInterface.Companion.getNoteYPosition
import com.instructure.horizon.features.notebook.common.webview.JSTextSelectionInterface.Companion.highlightNotes
import com.instructure.pandautils.compose.composables.ComposeEmbeddedWebViewCallbacks
import com.instructure.pandautils.compose.composables.ComposeWebViewCallbacks
Expand All @@ -53,6 +55,7 @@ import com.instructure.pandautils.utils.HtmlContentFormatter
import com.instructure.pandautils.utils.JsExternalToolInterface
import com.instructure.pandautils.utils.JsGoogleDocsInterface
import com.instructure.pandautils.views.CanvasWebView
import kotlinx.coroutines.delay
import kotlinx.coroutines.flow.MutableStateFlow
import kotlinx.coroutines.launch

Expand All @@ -69,7 +72,8 @@ fun ComposeNotesHighlightingCanvasWebView(
applyOnWebView: (CanvasWebView.() -> Unit)? = null,
webViewCallbacks: ComposeWebViewCallbacks = ComposeWebViewCallbacks(),
embeddedWebViewCallbacks: ComposeEmbeddedWebViewCallbacks? = null,
scrollState: ScrollState? = null
scrollState: ScrollState? = null,
scrollToNoteId: String? = null
) {
var pageHeight by remember { mutableIntStateOf(0) }
var scrollValue by rememberSaveable { mutableIntStateOf(0) }
Expand All @@ -78,6 +82,7 @@ fun ComposeNotesHighlightingCanvasWebView(
val webViewState = rememberSaveable { bundleOf() }
val selectionLocation: MutableStateFlow<SelectionLocation> by remember { mutableStateOf(MutableStateFlow(SelectionLocation(0f, 0f, 0f, 0f))) }
val lifecycleOwner = LocalLifecycleOwner.current
val composeScope = rememberCoroutineScope()
val context = LocalContext.current
val clipboardManager: ClipboardManager = LocalClipboardManager.current

Expand All @@ -91,6 +96,10 @@ fun ComposeNotesHighlightingCanvasWebView(
var selectedTextStart by remember { mutableIntStateOf(0) }
var selectedTextEnd by remember { mutableIntStateOf(0) }

var isScrolled by rememberSaveable { mutableStateOf(false) }
var isPageLoaded by remember { mutableStateOf(false) }
var webViewInstance by remember { mutableStateOf<NotesHighlightingCanvasWebViewWrapper?>(null) }

LaunchedEffect(scrollState?.maxValue) {
val maxValue = scrollState?.maxValue ?: 0

Expand All @@ -100,14 +109,30 @@ fun ComposeNotesHighlightingCanvasWebView(
}
}

LaunchedEffect(scrollToNoteId, isPageLoaded) {
Copy link

Choose a reason for hiding this comment

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

The hardcoded 500ms delay might not be sufficient in all cases, especially on slower devices or with complex page content. Consider:

  1. Using a more reliable mechanism like waiting for the WebView's content height to stabilize
  2. Making this configurable or using a larger safe default (e.g., 1000ms)
  3. Adding a callback from the WebView when it's truly ready for scrolling operations

delay(500)
val noteId = scrollToNoteId
Copy link

Choose a reason for hiding this comment

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

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.

if (noteId != null && isPageLoaded && scrollState != null && webViewInstance != null && !isScrolled) {
isScrolled = true
webViewInstance?.webView?.getNoteYPosition(noteId) { yPosition ->
if (yPosition != null) {
composeScope.launch {
val targetScroll = yPosition.toInt().coerceIn(0, scrollState.maxValue)
scrollState.animateScrollTo(targetScroll)
}
}
}
}
}

var previousHeight by remember { mutableIntStateOf(0) }
if (LocalInspectionMode.current) {
Text(text = content)
} else {
AndroidView(
factory = {
scrollValue = scrollState?.value ?: 0
NotesHighlightingCanvasWebViewWrapper(
val wrapper = NotesHighlightingCanvasWebViewWrapper(
it,
callback = AddNoteActionModeCallback(
lifecycleOwner,
Expand Down Expand Up @@ -161,6 +186,7 @@ fun ComposeNotesHighlightingCanvasWebView(

webView.evaluateTextSelectionInterface()
webView.highlightNotes(notesStateValue.value)
isPageLoaded = true
}

override fun onPageStartedCallback(webView: WebView, url: String) = webViewCallbacks.onPageStarted(webView, url)
Expand All @@ -184,6 +210,8 @@ fun ComposeNotesHighlightingCanvasWebView(

applyOnWebView?.let { applyOnWebView -> webView.applyOnWebView() }
}
webViewInstance = wrapper
wrapper
},
update = {
if (webViewState.isEmpty) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -678,6 +678,7 @@ function highlightSelection(paramsJson) {
const highlightElement = document.createElement("span");
highlightElement.classList.add(highlightClassName);
highlightElement.classList.add(cssClass);
highlightElement.setAttribute('data-note-id', noteId);
highlightElement.onclick = function () { ${ JS_INTERFACE_NAME }.onHighlightedTextClicked(noteId, noteReactionString, selectedText, userComment, startOffset, startContainer, endOffset, endContainer, textSelectionStart, textSelectionEnd, updatedAt); };
highlightElement.textContent = textNode.textContent;

Expand All @@ -687,6 +688,17 @@ function highlightSelection(paramsJson) {
}

}

Copy link

Choose a reason for hiding this comment

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

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) {
Copy link

Choose a reason for hiding this comment

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

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.

const highlights = document.getElementsByClassName('notebook-highlight');
for (const highlight of highlights) {
if (highlight.getAttribute('data-note-id') === noteId) {
const rect = highlight.getBoundingClientRect();
return rect.top + window.scrollY;
}
}
return -1;
}
javascript: (function () {
document.addEventListener("selectionchange", () => {
const selection = window.getSelection();
Expand Down Expand Up @@ -771,5 +783,13 @@ javascript: (function () {
this.evaluateJavascript(script, null)
}
}

fun WebView.getNoteYPosition(noteId: String, onResult: (Float?) -> Unit) {
val quotedNoteId = JSONObject.quote(noteId)
this.evaluateJavascript("getNotePosition($quotedNoteId)") { result ->
val position = result?.replace("\"", "")?.toFloatOrNull()
onResult(if (position != null && position >= 0) position else null)
}
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ data class NotesCallback(
) -> Unit,
val onNoteAdded: (
selectedText: String,
noteType: String?,
noteType: String,
startContainer: String,
startOffset: Int,
endContainer: String,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,8 @@ sealed class MainNavigationRoute(val route: String) {
val courseId: Long,
val moduleItemId: Long? = null,
val moduleItemAssetType: String? = null,
val moduleItemAssetId: String? = null
val moduleItemAssetId: String? = null,
val scrollToNoteId: String? = null
) :
MainNavigationRoute("module_item_sequence")

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,16 @@ val animationRules = listOf(
to = NotebookRoute.Notebook.route,
style = NavigationTransitionAnimation.SCALE
),
NavigationTransitionAnimationRule(
from = MainNavigationRoute.ModuleItemSequence.serializableRoute,
to = NotebookRoute.AddNotebook.serializableRoute,
style = NavigationTransitionAnimation.SCALE
),
NavigationTransitionAnimationRule(
from = MainNavigationRoute.ModuleItemSequence.serializableRoute,
to = NotebookRoute.EditNotebook.serializableRoute,
style = NavigationTransitionAnimation.SCALE
),

//Account
NavigationTransitionAnimationRule(
Expand Down
2 changes: 1 addition & 1 deletion libs/horizon/src/main/res/values/strings.xml
Original file line number Diff line number Diff line change
Expand Up @@ -369,7 +369,7 @@
<string name="dashboardCourseCardErrorMessage">Please try again.</string>
<string name="dashboardCourseCardRefreshLabel">Refresh</string>
<string name="dashboardNotStartedProgramDetailsLabel">Program details</string>
<string name="dashboardTimeSpentTitle">Time learning</string>
<string name="dashboardTimeLearningTitle">Time learning</string>
<string name="dashboardTimeSpentEmptyMessage">This widget will update once data becomes available.</string>
<string name="dashboardTimeSpentErrorMessage">We weren\'t able to load this content.\nPlease try again.</string>
<string name="dashboardTimeSpentRetry">Refresh</string>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,12 +17,10 @@
package com.instructure.horizon.features.notebook.addedit.add

import android.content.Context
import android.text.format.DateFormat
import androidx.compose.ui.text.input.TextFieldValue
import androidx.lifecycle.SavedStateHandle
import androidx.navigation.toRoute
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.R
import com.instructure.horizon.features.notebook.common.model.NotebookType
import com.instructure.horizon.features.notebook.navigation.NotebookRoute
Expand Down Expand Up @@ -64,9 +62,11 @@ class AddNoteViewModelTest {
fun setup() {
Dispatchers.setMain(testDispatcher)
mockkStatic("androidx.navigation.SavedStateHandleKt")
mockkStatic("android.text.format.DateFormat")
every { context.getString(R.string.createNoteTitle) } returns "Add note"
every { context.getString(R.string.noteHasBeenSavedMessage) } returns "Note has been saved"
every { context.getString(R.string.failedToSaveNoteMessage) } returns "Failed to save note"
every { DateFormat.getBestDateTimePattern(any(), any()) } returns "yyyy-MM-dd HH:mm"

setupSavedStateHandle(
courseId = testCourseId,
Expand Down
Loading