fix(editor): bound the settings read on note open, and test the two rules that had none (#204) - #278
Conversation
…ules 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 <noreply@anthropic.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 24e31ba2dd
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| withTimeoutOrNull(SETTINGS_READ_TIMEOUT_MS) { settingsRepository.settings.first() } | ||
| ?: AppSettings() |
There was a problem hiding this comment.
When settingsRepository.settings.first() takes longer than this timeout but then DataStore eventually emits the user's settings, the note is loaded with AppSettings() at caret 0 and isLoaded becomes true. As soon as appSettings.openNotesAt later updates to LAST_POSITION, the position recorder in LaunchedEffect(noteId, isLoaded, appSettings.openNotesAt) starts from that fallback top-of-note state and, after its debounce, upserts caretOffset = 0, overwriting the saved last position without user movement. Please gate that recorder or avoid writing fallback positions until the real settings snapshot/user interaction is available.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in 529e97d — good catch, and it is a hole the timeout in this PR opened rather than a pre-existing one.
Traced it through: appSettings.openNotesAt is a key of the recording effect, so a late DataStore emission restarts it against the fallback state, and the first debounced write stores caret 0 over a position the user never moved from. Before the timeout the read simply hung, so there was no fallback state to record — the guard belongs to this change.
recordsPosition(openNotesAt, openedOnFallbackSettings) now requires both the setting and a real settings read. Skipping the write is the conservative half of your suggestion: we could not read what the user asked for, so we do not overwrite what we already knew, and reopening the note takes the normal path. Two cases in OpenModeTest pin it.
…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 <noreply@anthropic.com>
1 participant
Three items from the v2.28.0 hardening tracker (#204) — the oldest open one, with 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 or fallback. 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.Now bounded at two seconds, falling back to
AppSettings(): edit mode, top of the note. Generous rather than tight — 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. This 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 brand-new note lands in preview with no editor and no keyboard, which reads as "the button did nothing". Extracted as
opensInPreviewwith four cases, including the one the guard exists for and the fact that whitespace counts as content.The lock gesture had exactly one case
EditorTopAppBarLockTestcovered tap-then-long-press. The order at risk is the other one: the lock runs in the Initial pass and consumes the release so the underlyingIconButtondoes not also see a tap. If that consumption leaked past its own gesture, the next tap would be swallowed and the control would look dead.Four cases added — tap after long-press, both gestures while already locked, repeated long-presses each firing, and the toggle reached by its label in preview mode (which is what a screen reader announces before either gesture). The shared setup moved into a helper as part of adding them.
Verification
./gradlew test(debug + release) and:app:lintReleasepass. The five gesture cases run under Robolectric insrc/testDebug, so they are in the requiredbuildcheck rather than the instrumented job.Still open on #204:
ViewModeLockedTintis a hardcoded amber with no contrast assertion, the locked-state and RTL goldens do not exist (both need a Linux record run), RTL editing edge cases are unverified, and RTL system locales still fall back to English UI strings — the last is the separate i18n effort the issue already calls out.🤖 Generated with Claude Code