fix(editor): open saved drafts at the top instead of scrolling to the bottom - #3337
Conversation
… bottom When a draft had no locally-saved cursor position (legacy drafts, drafts created on another device or on web, or the first open after caret-resume shipped), the restore logic placed the caret at the end of the body. The multiline body input auto-focuses, and focusing with the cursor at the end scrolls a long draft to the bottom. - Extract the restore-caret decision into a pure, unit-tested helper resolveRestoreCaret(savedCaret, bodyLength, isReply): no saved caret falls back to the top (0) for posts/drafts and to the end for replies (so a cached comment is appended to, not prepended); a saved caret resumes there, clamped to the current body length. - When restoring a post/draft with no saved caret, leave the body unfocused (blur + skip the delayed auto-focus) so the view stays at the top and there is no active cursor to prepend into. Replies and empty new-compose keep auto-focus. - Ignore the native echo of a programmatically-set selection so it is not persisted as a user caret move (which would make position 0 sticky).
Greptile SummaryThis PR updates markdown draft caret restore behavior so saved drafts open in the intended position.
Confidence Score: 4/5This is close, but the restore echo path should be fixed before merging.
src/components/markdownEditor/view/markdownEditorView.tsx Important Files Changed
Reviews (4): Last reviewed commit: "fix(editor): clear delayed autofocus tim..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![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: 2772a97ab0
ℹ️ 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".
| if (!hasSavedCaret && !isReply) { | ||
| suppressBodyAutoFocusRef.current = true; | ||
| inputRef.current?.blur(); |
There was a problem hiding this comment.
When a no-caret post draft is loaded after the empty editor has already emitted an onSelectionChange (for example from initial focus before tapping DRAFT), _persistCaret(0) can already be queued. This branch skips the restore echo but never cancels that pending debounced write, so it still stores caret 0; because resolveRestoreCaret now treats 0 as a saved caret, the next open will focus at the top and reintroduce prepend-on-type. Please clear the pending caret debounce before suppressing focus.
Useful? React with 👍 / 👎.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe Markdown editor now uses a shared caret-restoration helper, suppresses autofocus when loading posts or drafts without saved carets, cancels queued persistence after restoration, and persists selection changes only while the input is focused. ChangesCaret management
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Address PR review feedback: - Cancel any caret write queued by the empty input's initial focus when the draft body is restored, so a stale caret 0 can't be persisted under the draft key. On a no-caret draft that would resurface prepend-on-type on the next open; on a saved-caret draft it would clobber the position being resumed. - Assign suppressBodyAutoFocusRef unconditionally so a saved-caret restore in the same mounted view clears a previously-armed suppression instead of leaving it stale and skipping a legitimate autofocus. - Consume the programmatic-selection guard on the next selection event regardless of match, so a dropped or coalesced native echo can't leave it armed and later swallow a genuine user caret at the same offset.
…t flags Replace the two refs raised in PR review with stateless checks that cannot go stale: - Drop suppressBodyAutoFocusRef: the delayed focus effect now re-derives, at fire time, whether we restored a non-reply body that still has no saved caret (an empty compose or a since-edited draft both focus normally). A stale flag can no longer skip a later legitimate autofocus in the same mounted editor. - Drop lastProgrammaticSelectionRef: persist the caret only while the input is focused. A user caret move requires focus, and the no-caret restore blurs, so its async native selection echo is ignored without value-matching an echo (removing the dropped/delayed-echo edge cases). Programmatic sets while focused (inserts, saved-caret resume) echo the position we intend to keep anyway.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/components/markdownEditor/view/markdownEditorView.tsx (1)
210-229: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winConsider clearing the focus timeout on unmount or re-run.
The
setTimeoutat line 213 has no cleanup. If the component unmounts within the 1-second delay, the callback may callinputRef.current?.focus()on a torn-down instance. Similarly, ifautoFocusTextchanges and the effect re-runs, the previous timeout is not cleared.This is pre-existing, but the effect body is modified in this PR. Adding a cleanup is low-cost and prevents a potential post-unmount native command.
♻️ Optional cleanup
useEffect(() => { if (isReply || (autoFocusText && inputRef && inputRef.current && draftBtnTooltipRegistered)) { // added delay to open keyboard, solves the issue of keyboard not opening - setTimeout(() => { + const timer = setTimeout(() => { // Skip focusing when we restored an existing non-reply body that still has no // saved caret: focusing would drop the cursor at the top (prepend-on-type) and // slide the keyboard over the draft. Re-derived here at fire time (not a stored // flag) so it can't go stale across draft/compose changes in the same mounted // editor: an empty compose (no body) or a draft the user has since edited (a // caret now exists) both focus normally; replies always focus. const restoredWithoutCaret = !isReply && bodyTextRef.current !== '' && typeof store.getState().editor.caretMap?.[caretKeyRef.current] !== 'number'; if (!restoredWithoutCaret) { inputRef?.current?.focus(); } }, 1000); + return () => clearTimeout(timer); } }, [autoFocusText]);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/markdownEditor/view/markdownEditorView.tsx` around lines 210 - 229, Update the useEffect handling autoFocusText to retain the setTimeout handle and return cleanup that clears it when the effect re-runs or the component unmounts. Preserve the existing delayed focus and restoredWithoutCaret logic, ensuring cleared timers cannot invoke inputRef.current.focus().
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/components/markdownEditor/view/markdownEditorView.tsx`:
- Around line 210-229: Update the useEffect handling autoFocusText to retain the
setTimeout handle and return cleanup that clears it when the effect re-runs or
the component unmounts. Preserve the existing delayed focus and
restoredWithoutCaret logic, ensuring cleared timers cannot invoke
inputRef.current.focus().
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b30de2f7-8b41-4c12-bb58-f00de0a78b8e
📒 Files selected for processing (1)
src/components/markdownEditor/view/markdownEditorView.tsx
The 1s keyboard-focus timeout had no cleanup, so it could fire focus() on a torn-down input or leak a stale timer when autoFocusText changed. Return a clearTimeout cleanup from the effect.
| if (inputRef.current?.isFocused?.()) { | ||
| _persistCaret(selection.start); | ||
| } |
There was a problem hiding this comment.
The restore path still allows the programmatic selection at position 0 to be saved as a real caret. For a non-reply draft with no saved caret, _setTextAndSelection sets the selection before blur() runs. If the native selection echo reaches _handleOnSelectionChange while the input still reports focused, this branch queues _persistCaret(0) after the earlier cancel(). That makes 0 look like a saved caret on the next open, so the draft can focus at the top and the next typed character prepends into the body.
1 participant
Problem
Opening a saved draft scrolled the editor to the bottom of the body. When a draft had no locally-saved cursor position (legacy drafts, drafts from another device or web, or the first open after caret-resume shipped), the restore logic placed the caret at the end of the body; the multiline input auto-focuses, and focus with the cursor at the end scrolls a long draft to the bottom.
This was a regression: caret-resume originally fell back to the top; a later follow-up flipped the missing-caret fallback to the end (to avoid prepend-on-type), which reintroduced the bottom-scroll.
Fix
resolveRestoreCaret(savedCaret, bodyLength, isReply):Behavior
Tests
New
src/utils/editorCaret.test.tscovers the caret fallback matrix. Full jest suite passes; lint clean.Summary by CodeRabbit
Bug Fixes
Editor Experience
Tests