Fix cursor movement logic during Snapshot Event #398
Conversation
WalkthroughThe changes made in the pull request involve modifications to the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CodeMirror
participant Yorkie
User->>CodeMirror: Make changes to document
CodeMirror->>Yorkie: Dispatch changes with selection
Yorkie->>CodeMirror: Update document with new content
Yorkie->>CodeMirror: Send remote changes
CodeMirror->>User: Restore cursor position
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
devleejb
force-pushed
the
sync-cursor-when-snapshot
branch
from
5a08135
to
168e9b8
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
frontend/src/utils/yorkie/yorkieSync.ts (1)
56-71: LGTM! Clean implementation of cursor position preservation.The implementation effectively solves the cursor jumping issue by:
- Capturing the current selection state
- Properly converting position coordinates
- Restoring the selection after content replacement
Consider adding error handling for the
posRangeToIndexRangecall, as it might throw if the selection range is invalid:if (selection) { // Restore the cursor position when the text is replaced. + try { const cursor = text.posRangeToIndexRange(selection); transactionSpec["selection"] = { anchor: cursor[0], head: cursor[1], }; + } catch (error) { + console.warn('Failed to restore cursor position:', error); + // Fallback to default selection behavior + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
frontend/src/utils/yorkie/yorkieSync.ts(1 hunks)
🔇 Additional comments (1)
frontend/src/utils/yorkie/yorkieSync.ts (1)
56-57: Verify presence synchronization with selection updates.
While the implementation correctly retrieves the selection from presence, we should ensure that presence updates are properly synchronized when selection changes occur elsewhere in the codebase.
✅ Verification successful
Let me gather more specific information about the selection synchronization implementation.
Selection synchronization is properly implemented and maintained
The codebase shows a complete and correct implementation of presence synchronization for selections:
- In
remoteSelection.ts, selections are properly synchronized when they change:- Updates presence when selection changes:
presence.set({ selection, cursor }) - Clears presence when focus is lost:
presence.set({ selection: null, cursor: null })
- Updates presence when selection changes:
- In
yorkieSync.ts, selections are correctly restored during text updates:- Retrieves current selection:
this._doc.getMyPresence().selection - Properly converts and applies selection ranges
- Retrieves current selection:
- The presence initialization in
useYorkieDocument.tscorrectly sets up the initial state
The original concern about selection synchronization is addressed by the existing implementation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for presence update logic related to selection changes
echo "Searching for presence update handlers..."
rg -l "presence" --type ts
echo "\nSearching for selection-related presence updates..."
ast-grep --pattern 'selection = $_'
echo "\nSearching for getMyPresence usage..."
rg 'getMyPresence\(\)' -A 3
Length of output: 1572
Script:
#!/bin/bash
# Search for selection-related presence updates in the frontend
echo "Searching for selection update implementations..."
rg "selection" -A 5 frontend/src/utils/yorkie/yorkieSync.ts
echo "\nSearching for presence update handlers in useYorkieDocument..."
rg "presence" -A 5 frontend/src/hooks/useYorkieDocument.ts
echo "\nSearching for selection-related code in remoteSelection..."
rg "selection" -A 5 frontend/src/utils/yorkie/remoteSelection.ts
Length of output: 3916
What this PR does / why we need it:
This PR modifies the cursor movement logic triggered during a Snapshot Event. Previously, when a Snapshot Event occurred, the entire content was replaced, causing the cursor to jump to the top. This update ensures that the cursor remains in its intended position after the Snapshot Event.
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
The previous implementation negatively impacted the user experience by causing disruption in text editing, as users would lose their cursor position unexpectedly. This change aims to improve usability by maintaining the cursor’s position during content updates.
Does this PR introduce a user-facing change?:
Additional documentation:
Checklist:
Summary by CodeRabbit