From 24e31ba2dd391d8992056ed13c16bbb907123543 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=EB=B0=95=EC=9A=A9=EC=9D=80?= Date: Fri, 24 Jul 2026 17:51:09 +0900 Subject: [PATCH 1/2] fix(editor): bound the settings read on note open, and test the two rules that had none (#204) Three items from the v2.28.0 hardening tracker, which was the oldest open one and had none of its eight ticked. ## The note-open path could hang forever `settingsRepository.settings.first()` suspends before the note is shown at all, with no timeout. A DataStore read that never returns would hang note-open with nothing on screen and no way out -- and what it is being read for is which *mode* the note opens in. Bounded at two seconds, falling back to `AppSettings()`: edit mode, top of the note. Generous rather than tight, because the read normally completes in milliseconds and a device slow enough to need a second is one where opening in the wrong mode is worse than waiting. It is a stuck-forever guard, not a latency budget. ## The open-mode rule now has a test `isPreviewMode = openInPreview && content.isNotEmpty()` lived inline in the load effect, so the only thing exercising it was someone opening a note on a device. The content check is not a nicety: the FAB creates the note first and opens it with a real id, so the id alone cannot tell a new note from an existing one -- drop the check and a new note lands in preview with no editor and no keyboard. Extracted as `opensInPreview` with four cases, including the one the guard exists for and the fact that whitespace counts as content. ## The lock gesture had one case `EditorTopAppBarLockTest` covered tap-then-long-press only. The order at risk is the other one: the lock runs in the Initial pass and consumes the release so the IconButton does not also see a tap, and if that leaked past its own gesture the *next* tap would be swallowed and the button would look dead. Four cases added -- tap after long-press, both gestures while already locked, repeated long-presses, and the toggle reached by its label in preview mode (which is what a screen reader announces). The shared setup moved into a helper as part of adding them. ./gradlew test (debug + release) and :app:lintRelease pass. Co-Authored-By: Claude Opus 4.8 --- .../notes/feature/editor/EditorScreen.kt | 45 ++++++++-- .../notes/feature/editor/OpenModeTest.kt | 49 ++++++++++ .../feature/editor/EditorTopAppBarLockTest.kt | 89 +++++++++++++++++-- 3 files changed, 168 insertions(+), 15 deletions(-) create mode 100644 app/src/test/java/com/markleaf/notes/feature/editor/OpenModeTest.kt diff --git a/app/src/main/java/com/markleaf/notes/feature/editor/EditorScreen.kt b/app/src/main/java/com/markleaf/notes/feature/editor/EditorScreen.kt index a128316..80f6909 100644 --- a/app/src/main/java/com/markleaf/notes/feature/editor/EditorScreen.kt +++ b/app/src/main/java/com/markleaf/notes/feature/editor/EditorScreen.kt @@ -110,6 +110,7 @@ import kotlinx.coroutines.flow.collectLatest import kotlinx.coroutines.flow.distinctUntilChanged import kotlinx.coroutines.flow.first import kotlinx.coroutines.withContext +import kotlinx.coroutines.withTimeoutOrNull import kotlinx.coroutines.delay import kotlinx.coroutines.launch import kotlin.math.ceil @@ -316,7 +317,16 @@ fun EditorScreen( // snapshot, which starts on the default before DataStore emits) so // an existing note honours "open notes in preview" on its very first // frame instead of flashing edit (#200). New notes stay in edit. - val persistedSettings = settingsRepository.settings.first() + // + // Bounded, because this suspends before the note is shown at all: a + // DataStore read that never returns would hang note-open with no + // way out, and the whole point of reading it here is a detail of + // which mode the note opens in (#204). Falling back to the defaults + // opens in edit at the top, which is the safe answer — it shows the + // note and puts the caret somewhere harmless. + val persistedSettings = + withTimeoutOrNull(SETTINGS_READ_TIMEOUT_MS) { settingsRepository.settings.first() } + ?: AppSettings() val openInPreview = persistedSettings.openNotesInPreview val loadedNote = repo.getNote(noteId) val content = loadedNote?.contentMarkdown.orEmpty() @@ -346,12 +356,7 @@ fun EditorScreen( OpenNotesAt.LAST_POSITION -> lastPosition?.let { PreviewScrollRequest(it.previewIndex, animate = false) } } - // Only land in preview when there is something to preview. A - // brand-new note is created first and then opened with a real id - // (the FAB path in MarkleafNavHost), so `noteId != null` alone would - // drop an empty note into preview with no editor/IME — honour "new - // notes still open for editing" by gating on content (#200). - isPreviewMode = openInPreview && content.isNotEmpty() + isPreviewMode = opensInPreview(openInPreview, content) shouldRequestEditorFocus = content.isEmpty() isLoaded = true // Remember this note as the launch target for the opt-in @@ -1132,6 +1137,32 @@ private const val MAX_TAG_SUGGESTIONS = 8 */ private const val POSITION_WRITE_DEBOUNCE_MS = 1_000L +/** + * How long the note-open path waits for DataStore before opening on defaults. + * + * Generous rather than tight: the read normally completes in milliseconds, and + * a device slow enough to need a second is a device where opening on the wrong + * mode is worse than waiting. This is a stuck-forever guard, not a latency + * budget (#204). + */ +private const val SETTINGS_READ_TIMEOUT_MS = 2_000L + +/** + * Whether a note opens in preview: only when the setting is on *and* the note + * has something to preview. + * + * The content check is not a nicety. A brand-new note is created first and then + * opened with a real id (the FAB path in `MarkleafNavHost`), so the id alone + * cannot tell a new note from an existing one — without this an empty note + * would land in preview with no editor and no keyboard, and "new notes still + * open for editing" would be broken (#200). + * + * Extracted so that rule has a test rather than living only in a load effect + * that needs a device to exercise (#204). + */ +internal fun opensInPreview(openNotesInPreview: Boolean, content: String): Boolean = + openNotesInPreview && content.isNotEmpty() + /** * A preview scroll waiting for the preview to be built. [animate] separates the * two callers: restoring where a note was left should already be there when the diff --git a/app/src/test/java/com/markleaf/notes/feature/editor/OpenModeTest.kt b/app/src/test/java/com/markleaf/notes/feature/editor/OpenModeTest.kt new file mode 100644 index 0000000..dbba601 --- /dev/null +++ b/app/src/test/java/com/markleaf/notes/feature/editor/OpenModeTest.kt @@ -0,0 +1,49 @@ +package com.markleaf.notes.feature.editor + +import org.junit.Assert.assertFalse +import org.junit.Assert.assertTrue +import org.junit.Test + +/** + * Which mode a note opens in when "open notes in preview" is on (#200). + * + * The rule lived inline in the editor's load effect, where the only thing + * exercising it was someone opening a note on a device (#204). It is one + * boolean and one emptiness check, and getting it wrong is not subtle — a new + * note that opens in preview has no editor and no keyboard, so the FAB appears + * to do nothing. + */ +class OpenModeTest { + + @Test + fun `setting off always opens in edit`() { + assertFalse(opensInPreview(openNotesInPreview = false, content = "# A note")) + assertFalse(opensInPreview(openNotesInPreview = false, content = "")) + } + + @Test + fun `an existing note opens in preview when the setting is on`() { + assertTrue(opensInPreview(openNotesInPreview = true, content = "# A note")) + } + + /** + * The case the guard exists for. The FAB creates the note first and opens + * it with a real id, so the id cannot distinguish a new note from an + * existing one — only its content can. + */ + @Test + fun `a just-created note opens in edit even with the setting on`() { + assertFalse(opensInPreview(openNotesInPreview = true, content = "")) + } + + /** + * Whitespace is content: the user typed it, and a note holding a newline + * has something the preview can render. Emptiness is the "brand new" + * signal, not blankness. + */ + @Test + fun `whitespace counts as content`() { + assertTrue(opensInPreview(openNotesInPreview = true, content = "\n")) + assertTrue(opensInPreview(openNotesInPreview = true, content = " ")) + } +} diff --git a/app/src/testDebug/java/com/markleaf/notes/feature/editor/EditorTopAppBarLockTest.kt b/app/src/testDebug/java/com/markleaf/notes/feature/editor/EditorTopAppBarLockTest.kt index 18ecf19..3a7934b 100644 --- a/app/src/testDebug/java/com/markleaf/notes/feature/editor/EditorTopAppBarLockTest.kt +++ b/app/src/testDebug/java/com/markleaf/notes/feature/editor/EditorTopAppBarLockTest.kt @@ -26,15 +26,20 @@ class EditorTopAppBarLockTest { @get:Rule val composeRule = createComposeRule() - @Test - fun tapFlipsPreviewAndLongPressFlipsLockIndependently() { - var previewToggles = 0 - var lockToggles = 0 + private var previewToggles = 0 + private var lockToggles = 0 + + /** + * Renders the bar and returns the view-toggle node. In edit mode the toggle + * offers "Preview"; in preview mode it offers "Edit" — the label names the + * destination, not the current state. + */ + private fun toggle(isPreviewMode: Boolean = false, isViewModeLocked: Boolean = false) = run { composeRule.setContent { MarkleafTheme(dynamicColor = false) { EditorTopAppBar( title = "New Note", - isPreviewMode = false, + isPreviewMode = isPreviewMode, isFocusMode = false, showMore = true, moreExpanded = false, @@ -46,14 +51,17 @@ class EditorTopAppBarLockTest { onOpenMore = {}, onDismissMore = {}, moreMenuContent = {}, - isViewModeLocked = false, + isViewModeLocked = isViewModeLocked, onToggleLock = { lockToggles++ } ) } } + composeRule.onNodeWithContentDescription(if (isPreviewMode) "Edit" else "Preview") + } - // While in edit mode the toggle offers "Preview". - val toggle = composeRule.onNodeWithContentDescription("Preview") + @Test + fun tapFlipsPreviewAndLongPressFlipsLockIndependently() { + val toggle = toggle() toggle.performClick() assertEquals(1, previewToggles) @@ -63,4 +71,69 @@ class EditorTopAppBarLockTest { assertEquals(1, lockToggles) assertEquals(1, previewToggles) } + + /** + * The other order, which is the one at risk. The lock gesture runs in the + * Initial pass and consumes the release so the underlying IconButton does + * not also see a tap — if that consumption leaked past its own gesture, the + * *next* tap would be swallowed and the button would look dead. + */ + @Test + fun aTapAfterALongPressStillFlipsPreview() { + val toggle = toggle() + + toggle.performTouchInput { longClick() } + assertEquals(1, lockToggles) + assertEquals(0, previewToggles) + + toggle.performClick() + assertEquals(1, previewToggles) + assertEquals(1, lockToggles) + } + + /** Locking pins the mode; it does not disable the control that flips it. */ + @Test + fun tapAndLongPressStillWorkWhileLocked() { + val toggle = toggle(isViewModeLocked = true) + + toggle.performClick() + assertEquals(1, previewToggles) + + // Long-pressing a locked toggle unlocks it — same gesture, same handler. + toggle.performTouchInput { longClick() } + assertEquals(1, lockToggles) + assertEquals(1, previewToggles) + } + + /** + * Repeated long-presses each register. The lock is a toggle, so a second + * press has to reach the handler rather than being eaten as a repeat. + */ + @Test + fun repeatedLongPressesEachFire() { + val toggle = toggle() + + toggle.performTouchInput { longClick() } + toggle.performTouchInput { longClick() } + toggle.performTouchInput { longClick() } + + assertEquals(3, lockToggles) + assertEquals(0, previewToggles) + } + + /** + * The control keeps its label in preview mode, which is what a screen + * reader announces before either gesture. Losing it would leave the lock + * reachable only by sighted trial and error. + */ + @Test + fun theToggleIsReachableByItsLabelInPreviewMode() { + val toggle = toggle(isPreviewMode = true) + + toggle.performTouchInput { longClick() } + assertEquals(1, lockToggles) + + toggle.performClick() + assertEquals(1, previewToggles) + } } From 529e97d23d48886c2b157dd74ff5e6c508d6c379 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=EB=B0=95=EC=9A=A9=EC=9D=80?= Date: Fri, 24 Jul 2026 18:34:23 +0900 Subject: [PATCH 2/2] fix(editor): do not record a position from a note opened on fallback settings (#204) Codex review on #278, and it found a hole the timeout itself opened. When the settings read times out, the note opens at the top on defaults. DataStore may then emit the real settings a moment later, flipping `appSettings.openNotesAt` to LAST_POSITION -- which is a key of the position-recording effect, so the recorder starts, sees the fallback state, and after its debounce writes caret 0 over the position the user actually left. All without them touching anything. Before the timeout this could not happen: the read hung instead, so there was no fallback state to record. The guard is therefore part of the same change, not a pre-existing bug. `recordsPosition` now requires both the setting and a real settings read. Skipping the write is the conservative half: we could not read what the user asked for, so we do not overwrite what we already knew. Reopening the note takes the normal path. Two more cases in OpenModeTest cover it. ./gradlew test (debug + release) and :app:lintRelease pass. Co-Authored-By: Claude Opus 4.8 --- .../notes/feature/editor/EditorScreen.kt | 32 ++++++++++++++++--- .../notes/feature/editor/OpenModeTest.kt | 23 +++++++++++++ 2 files changed, 51 insertions(+), 4 deletions(-) diff --git a/app/src/main/java/com/markleaf/notes/feature/editor/EditorScreen.kt b/app/src/main/java/com/markleaf/notes/feature/editor/EditorScreen.kt index 80f6909..7919b03 100644 --- a/app/src/main/java/com/markleaf/notes/feature/editor/EditorScreen.kt +++ b/app/src/main/java/com/markleaf/notes/feature/editor/EditorScreen.kt @@ -139,6 +139,9 @@ fun EditorScreen( var shouldRequestEditorFocus by remember(noteId) { mutableStateOf(noteId == null) } val editorFocusRequester = remember(noteId) { FocusRequester() } var isPreviewMode by remember(noteId) { mutableStateOf(false) } + // True when the settings read timed out and the note opened on defaults. + // The position recorder consults it — see [recordsPosition] (#204). + var openedOnFallbackSettings by remember(noteId) { mutableStateOf(false) } var isFocusMode by remember(noteId) { mutableStateOf(false) } var isFormattingExpanded by remember(noteId) { mutableStateOf(false) } var showDeleteConfirm by remember(noteId) { mutableStateOf(false) } @@ -324,9 +327,10 @@ fun EditorScreen( // which mode the note opens in (#204). Falling back to the defaults // opens in edit at the top, which is the safe answer — it shows the // note and puts the caret somewhere harmless. - val persistedSettings = + val readSettings = withTimeoutOrNull(SETTINGS_READ_TIMEOUT_MS) { settingsRepository.settings.first() } - ?: AppSettings() + openedOnFallbackSettings = readSettings == null + val persistedSettings = readSettings ?: AppSettings() val openInPreview = persistedSettings.openNotesInPreview val loadedNote = repo.getNote(noteId) val content = loadedNote?.contentMarkdown.orEmpty() @@ -449,9 +453,9 @@ fun EditorScreen( // write at the end of it rather than one per keystroke, and written while // the screen is still composed so it survives the process being killed // rather than depending on a tidy exit. - LaunchedEffect(noteId, isLoaded, appSettings.openNotesAt) { + LaunchedEffect(noteId, isLoaded, appSettings.openNotesAt, openedOnFallbackSettings) { if (noteId == null || !isLoaded) return@LaunchedEffect - if (appSettings.openNotesAt != OpenNotesAt.LAST_POSITION) return@LaunchedEffect + if (!recordsPosition(appSettings.openNotesAt, openedOnFallbackSettings)) return@LaunchedEffect snapshotFlow { Triple( isPreviewMode, @@ -1163,6 +1167,26 @@ private const val SETTINGS_READ_TIMEOUT_MS = 2_000L internal fun opensInPreview(openNotesInPreview: Boolean, content: String): Boolean = openNotesInPreview && content.isNotEmpty() +/** + * Whether this screen may record where the note was left. + * + * The setting has to be on, and — the part that is easy to miss — the note must + * not have opened on fallback settings. When the settings read times out the + * note opens at the top on defaults; DataStore may then emit the real settings + * a moment later, flipping `openNotesAt` to `LAST_POSITION` and starting the + * recorder from that fallback state. Its first debounced write would store + * caret 0 and overwrite the position the user actually left, without them + * having touched anything (#204). + * + * Skipping the write is the conservative answer: we could not read what the + * user asked for, so we do not overwrite what we already knew. Reopening the + * note takes the normal path. + */ +internal fun recordsPosition( + openNotesAt: OpenNotesAt, + openedOnFallbackSettings: Boolean +): Boolean = openNotesAt == OpenNotesAt.LAST_POSITION && !openedOnFallbackSettings + /** * A preview scroll waiting for the preview to be built. [animate] separates the * two callers: restoring where a note was left should already be there when the diff --git a/app/src/test/java/com/markleaf/notes/feature/editor/OpenModeTest.kt b/app/src/test/java/com/markleaf/notes/feature/editor/OpenModeTest.kt index dbba601..3d30900 100644 --- a/app/src/test/java/com/markleaf/notes/feature/editor/OpenModeTest.kt +++ b/app/src/test/java/com/markleaf/notes/feature/editor/OpenModeTest.kt @@ -1,5 +1,6 @@ package com.markleaf.notes.feature.editor +import com.markleaf.notes.data.settings.OpenNotesAt import org.junit.Assert.assertFalse import org.junit.Assert.assertTrue import org.junit.Test @@ -46,4 +47,26 @@ class OpenModeTest { assertTrue(opensInPreview(openNotesInPreview = true, content = "\n")) assertTrue(opensInPreview(openNotesInPreview = true, content = " ")) } + + // ---- position recording ---- + + @Test + fun `position is recorded only when the setting selects it`() { + assertTrue(recordsPosition(OpenNotesAt.LAST_POSITION, openedOnFallbackSettings = false)) + assertFalse(recordsPosition(OpenNotesAt.TOP, openedOnFallbackSettings = false)) + assertFalse(recordsPosition(OpenNotesAt.BOTTOM, openedOnFallbackSettings = false)) + } + + /** + * The trap the timeout introduced. A note that opened on fallback settings + * sits at caret 0; if DataStore then emits the real settings and flips + * `openNotesAt` to LAST_POSITION, the recorder would start from that + * fallback state and its first debounced write would store 0 over the + * position the user actually left — without them touching anything. + */ + @Test + fun `a note opened on fallback settings never records its position`() { + assertFalse(recordsPosition(OpenNotesAt.LAST_POSITION, openedOnFallbackSettings = true)) + assertFalse(recordsPosition(OpenNotesAt.TOP, openedOnFallbackSettings = true)) + } }