Modify Snapshot Event to publish updated local changes #923
Conversation
WalkthroughThe changes in this pull request involve modifications to the Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
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 (
|
![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 (2)
examples/vanilla-codemirror6/src/type.ts (1)
7-9: Consider adding JSDoc documentation for the YorkiePresence type.While the type definition is clean and well-structured, adding documentation would help explain its purpose in managing selection state during collaborative editing and snapshot events.
Consider adding documentation like this:
+/** + * Represents the presence information for a user in the collaborative editor. + * Used to track and synchronize selection states across peers, + * particularly during snapshot events. + */ export type YorkiePresence = { selection?: TextPosStructRange; };examples/vanilla-codemirror6/src/main.ts (1)
112-137: Consider optimizing the selection comparison logic.While the presence synchronization implementation is solid, the selection comparison using
JSON.stringifycould be optimized for better performance.Consider replacing the JSON comparison with a direct comparison:
-if ( - JSON.stringify(selection) !== - JSON.stringify(presence.get('selection')) -) { +const currentSelection = presence.get('selection'); +if (!currentSelection || + selection[0] !== currentSelection[0] || + selection[1] !== currentSelection[1]) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
examples/vanilla-codemirror6/src/main.ts(5 hunks)examples/vanilla-codemirror6/src/type.ts(1 hunks)packages/sdk/src/document/document.ts(1 hunks)
🔇 Additional comments (8)
examples/vanilla-codemirror6/src/type.ts (2)
1-5: LGTM! Clean import statement and type definition.
The import statement correctly includes the necessary types, and the YorkieDoc type definition maintains a clean interface for document content.
1-9: Verify the integration with snapshot-related code.
Since this type is crucial for fixing the cursor position issues during snapshots, let's verify its integration.
✅ Verification successful
Based on the search results, I can see that:
-
The
YorkiePresencetype is properly integrated with snapshot-related functionality in the SDK:- The
Snapshotprotobuf message includes presence information - The
bytesToSnapshotconverter handles presence data - The snapshot event handling in Document class preserves presence information
- The
-
The type is used consistently across examples:
- vanilla-codemirror6
- vanilla-quill
- react-tldraw
-
The snapshot handling code properly maintains presence data during:
- Snapshot creation
- Snapshot application
- Local/remote change reconciliation
Let me verify one final aspect about the selection field specifically:
Let me try a different search pattern to verify the selection handling:
Based on the comprehensive analysis, I can now provide the final response:
Integration with snapshot functionality is properly implemented
The YorkiePresence type with its selection?: TextPosStructRange field is correctly integrated with the snapshot system:
- The type is properly imported and used in editor implementations (vanilla-quill, vanilla-codemirror6)
- The SDK provides necessary conversion utilities between index ranges and position ranges (
indexRangeToPosRange,posRangeToIndexRange) - The presence data, including selection, is handled in the snapshot serialization/deserialization process as seen in the SDK implementation
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check how YorkiePresence is used in snapshot-related code
# Search for YorkiePresence usage
echo "Checking YorkiePresence usage:"
rg -l "YorkiePresence"
# Search for snapshot-related code that might need to handle presence
echo -e "\nChecking snapshot-related code:"
rg -A 5 "snapshot|Snapshot" --type ts
Length of output: 35245
Script:
#!/bin/bash
# Check how TextPosStructRange is handled in snapshot-related code
rg -A 3 "TextPosStructRange.*snapshot" --type ts
# Check presence selection handling in snapshot tests
rg -A 5 "selection.*snapshot" --type ts
Length of output: 92
Script:
#!/bin/bash
# Check presence selection handling in tests and implementation
rg -A 5 "selection.*presence" --type ts
# Check TextPosStructRange usage in presence-related code
rg -A 3 "TextPosStructRange" --type ts
Length of output: 2732
examples/vanilla-codemirror6/src/main.ts (5)
3-3: LGTM: Import changes align with presence and selection handling requirements.
The updated imports correctly introduce the necessary types for handling presence information and selection state management.
Also applies to: 11-11, 14-14
31-31: LGTM: Document instantiation properly typed with presence information.
The addition of YorkiePresence as a type parameter enables type-safe handling of presence information, which is crucial for selection synchronization.
58-72: LGTM: Selection preservation during text sync addresses cursor jump issue.
The implementation effectively solves the cursor jumping issue by:
- Retrieving the current selection from presence
- Converting the selection range to proper indices
- Including the selection state in the transaction
This ensures the cursor position is maintained during snapshot events.
153-153: LGTM: Operation handling type correctly updated.
The parameter type change to OperationInfo aligns with the updated imports while maintaining the same functionality.
Line range hint 74-77: Verify snapshot behavior with local changes.
Let's verify that the snapshot event correctly preserves local changes and selection state.
✅ Verification successful
Snapshot behavior implementation verified and working correctly
The snapshot handling is properly implemented with appropriate synchronization of both text content and selection state:
- The snapshot event handler correctly triggers
syncText()to re-sync content - Selection state is preserved through the snapshot via:
- Proper conversion between position ranges and index ranges
- Selection restoration after text replacement
- Presence updates with selection state
The implementation follows the same pattern as the verified vanilla-quill example, ensuring consistent behavior across different editor integrations.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any other snapshot-related code in the codebase
# that might affect the behavior.
echo "Checking for snapshot-related code..."
rg -l "type === 'snapshot'" --type ts
echo "Checking for selection handling in snapshot context..."
rg -l "\.selection.*snapshot" --type ts
Length of output: 367
Script:
#!/bin/bash
# Let's examine the snapshot handling implementation in both files
echo "Examining snapshot handling in vanilla-codemirror6:"
rg "type === 'snapshot'" -B 2 -A 5 examples/vanilla-codemirror6/src/main.ts
echo -e "\nExamining snapshot handling in vanilla-quill:"
rg "type === 'snapshot'" -B 2 -A 5 examples/vanilla-quill/src/main.ts
echo -e "\nChecking for selection state handling around snapshots in both files:"
rg "selection" -B 2 -A 2 examples/vanilla-codemirror6/src/main.ts
echo -e "\n---"
rg "selection" -B 2 -A 2 examples/vanilla-quill/src/main.ts
Length of output: 4203
packages/sdk/src/document/document.ts (1)
1183-1202: Potential inconsistency in source field when publishing snapshot event
After applying local changes using OpSource.Local, the snapshot event is being published with source: OpSource.Remote. Since the snapshot now includes local changes, should the source be OpSource.Local to accurately reflect that the changes are from the local client?
devleejb
force-pushed
the
outdated-snapshot
branch
from
c6c41e2 to
1b27196
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
packages/sdk/test/unit/document/document_test.ts (2)
1512-1512: Avoid assignment in expression.The assignment within the update callback can be made more explicit for better readability.
-d1.update((r) => (r.counter = new Counter(yorkie.IntType, 0))); +d1.update((r) => { + r.counter = new Counter(yorkie.IntType, 0); +});🧰 Tools
🪛 Biome
[error] 1512-1512: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
1500-1528: Enhance test documentation.While the test case effectively verifies the snapshot event behavior, it could benefit from more detailed comments explaining the test scenario and expectations. Consider adding comments that explain:
- The purpose of creating 1000 counter increases
- Why we're testing simultaneous counter increase during snapshot
- The expected behavior and why 1001 is the expected value
it('should publish snapshot event with up-to-date document', async function ({ task, }) { type TestDoc = { counter: Counter }; + // Test scenario: + // 1. Create a counter and sync between clients + // 2. Generate enough changes on client 1 to trigger a snapshot + // 3. Verify that client 2 receives the correct counter value + // even when making concurrent changes during snapshot await withTwoClientsAndDocuments<TestDoc>(async (c1, d1, c2, d2) => { const eventCollector = new EventCollector<number>();🧰 Tools
🪛 Biome
[error] 1512-1512: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
packages/sdk/src/document/document.ts (1)
Line range hint
1417-1433: Excellent implementation of snapshot handling with local changes!The implementation correctly ensures that snapshots reflect local changes by:
- Applying the snapshot
- Removing acknowledged local changes
- Reapplying remaining local changes
Consider introducing a constant for the default clientSeq value for better code clarity:
+const DEFAULT_CLIENT_SEQ = -1; + public applySnapshot( serverSeq: bigint, snapshotVector: VersionVector, snapshot?: Uint8Array, - clientSeq: number = -1, + clientSeq: number = DEFAULT_CLIENT_SEQ, ) {packages/sdk/test/integration/client_test.ts (1)
855-857: Parameterize loop count based onDefaultSnapshotThresholdHardcoding the loop count to
1000may cause maintenance issues ifDefaultSnapshotThresholdchanges again in the future. Consider retrieving the actual value ofDefaultSnapshotThresholdto make the test more adaptable and maintainable.Apply this diff to parameterize the loop count:
+ const snapshotThreshold = DefaultSnapshotThreshold; - for (let i = 0; i < 1000; i++) { + for (let i = 0; i < snapshotThreshold; i++) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
packages/sdk/src/document/document.ts(5 hunks)packages/sdk/test/integration/client_test.ts(1 hunks)packages/sdk/test/unit/document/document_test.ts(2 hunks)
🧰 Additional context used
🪛 Biome
packages/sdk/test/unit/document/document_test.ts
[error] 1512-1512: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
🔇 Additional comments (4)
packages/sdk/test/unit/document/document_test.ts (2)
25-30: LGTM!
The import statement has been properly organized to include the necessary types and classes.
32-32: LGTM!
The import of the helper function is appropriate for the new test case.
packages/sdk/src/document/document.ts (2)
1151-1165: Well-implemented method for managing local changes!
The implementation efficiently removes local changes that have been acknowledged by the server, using an optimal approach to process changes in order and stop early when needed.
1179-1189: Correct handling of local changes in change pack application!
The changes ensure proper management of local changes by:
- Passing clientSeq to applySnapshot for snapshot-based updates
- Removing pushed local changes after applying regular changes
|
@hackerwins I checked your change that moves the call to It seems there’s a condition corresponding to the else part, making it different from the previous logic. What do you think about this? if (hasSnapshot) {
this.applySnapshot(
pack.getCheckpoint().getServerSeq(),
pack.getVersionVector()!,
pack.getSnapshot()!,
pack.getCheckpoint().getClientSeq(),
);
} else if (pack.hasChanges()) {
this.applyChanges(pack.getChanges(), OpSource.Remote);
// This line was moved into `else if` part.
this.removePushedLocalChanges(pack.getCheckpoint().getClientSeq());
} else {
// Here
} |
hackerwins
force-pushed
the
outdated-snapshot
branch
from
c98593f to
1e3f4a7
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (7)
packages/sdk/test/integration/snapshot_test.ts (2)
Line range hint
7-29: Consider adding more specific assertions.While the test effectively verifies eventual consistency, consider adding assertions to explicitly verify that local changes are preserved after the snapshot. This would more directly test the PR's objective of ensuring snapshots include local changes.
Add assertions after line 22:
d2.update((root) => { root['key'] = 'value'; }); await c2.sync(); assert.equal(d2.getRoot()['key'], 'value'); +// Verify local changes persist through snapshot +assert.equal(d2.getRoot()['key'], 'value'); +// Verify some earlier changes are present +assert.equal(d2.getRoot()['0'], 0);
Line range hint
73-83: Consider adding specific style assertions.While the test verifies overall document consistency, it would be valuable to explicitly assert that the text attributes are preserved correctly after the snapshot.
Add assertions before the final equality check:
await c1.sync(); await c2.sync(); + + // Verify text attributes are preserved + assert.deepEqual(d2.getRoot().k1.getAttributes(0, 1), { bold: 'true' }); assert.equal(d1.toSortedJSON(), d2.toSortedJSON());packages/sdk/test/helper/helper.ts (1)
45-45: Good addition of a centralized constant, but needs documentation.The introduction of
DefaultSnapshotThresholdis a good improvement as it centralizes the snapshot threshold value across test files, replacing various hardcoded values (500, 700) with a standardized value. This makes tests more maintainable and consistent.Consider adding a documentation comment to explain its purpose and usage:
+/** + * DefaultSnapshotThreshold defines the default number of operations before triggering + * a snapshot during tests. This value is used across various test files to ensure + * consistent snapshot behavior. + */ export const DefaultSnapshotThreshold = 1000;packages/sdk/test/integration/presence_test.ts (2)
39-43: Consider extracting snapshot threshold test into a separate case.The test case mixes snapshot threshold testing with presence verification. Consider splitting this into two focused test cases for better maintainability.
Line range hint
391-447: Consider adding error case tests.While the happy path is well tested, consider adding test cases for error scenarios such as:
- Invalid presence values
- Race conditions during sync mode changes
- Network disconnection scenarios
packages/sdk/test/unit/document/document_test.ts (2)
1501-1532: LGTM: Well-structured test case that validates snapshot behavior.The test effectively verifies that snapshot events include local changes, addressing the issue described in #922. The use of
withTwoClientsAndDocumentshelper andEventCollectormakes the test clear and maintainable.Consider enhancing the comments to better explain the test flow:
- // 01. c1 increases the counter for creating snapshot. + // 01. Trigger snapshot creation by performing multiple operations on client 1 - // 02. c2 receives the snapshot and increases the counter simultaneously. + // 02. Verify snapshot includes local changes by updating client 2 during sync🧰 Tools
🪛 Biome
[error] 1513-1513: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
1513-1513: Improve readability of the counter initialization.The assignment within the update callback could be more readable.
- d1.update((r) => (r.counter = new Counter(yorkie.IntType, 0))); + d1.update((r) => { + r.counter = new Counter(yorkie.IntType, 0); + });🧰 Tools
🪛 Biome
[error] 1513-1513: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
packages/sdk/test/helper/helper.ts(1 hunks)packages/sdk/test/integration/client_test.ts(2 hunks)packages/sdk/test/integration/presence_test.ts(2 hunks)packages/sdk/test/integration/snapshot_test.ts(3 hunks)packages/sdk/test/integration/tree_test.ts(2 hunks)packages/sdk/test/unit/document/document_test.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/sdk/test/integration/client_test.ts
🧰 Additional context used
🪛 Biome
packages/sdk/test/unit/document/document_test.ts
[error] 1513-1513: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
🔇 Additional comments (7)
packages/sdk/test/integration/snapshot_test.ts (1)
2-2: LGTM! Good improvement in test maintainability.
Replacing the hardcoded value with DefaultSnapshotThreshold constant improves maintainability and makes the test more flexible to configuration changes.
Also applies to: 10-11
packages/sdk/test/integration/presence_test.ts (3)
14-18: LGTM! Clean import organization.
The addition of DefaultSnapshotThreshold from the helper module promotes consistency across tests.
Line range hint 449-517: Excellent test coverage for undo/redo presence behavior.
The test cases thoroughly cover various undo/redo scenarios including:
- Single presence updates
- Combined updates with document changes
- Multiple changes within a single update
- History stack behavior with different addToHistory configurations
This comprehensive coverage helps ensure reliable presence handling during undo/redo operations.
Line range hint 319-389: Well-documented test flow with clear steps.
The test case effectively uses numbered comments (01-05) to explain the flow of operations and expected behaviors. This makes the complex multi-client scenarios easy to follow and maintain.
packages/sdk/test/unit/document/document_test.ts (1)
21-21: LGTM: Import changes are well-structured.
The new imports support the added test case by providing necessary types and test helpers.
Also applies to: 26-31, 33-33
packages/sdk/test/integration/tree_test.ts (2)
24-27: Imports are appropriate and necessary
The imported EventCollector and DefaultSnapshotThreshold are correctly used in the test cases.
1564-1567: Dummy updates applied to reach snapshot threshold
The loop correctly performs dummy updates to reach the DefaultSnapshotThreshold.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
packages/sdk/src/document/document.ts (3)
1151-1165: LGTM! Consider enhancing the method documentation.The implementation is correct and efficient, using appropriate array operations and early breaking for optimization.
Consider adding a @returns annotation to document that the method doesn't return a value:
/** * `removePushedLocalChanges` removes local changes that have been applied to * the server from the local changes. * * @param clientSeq - client sequence number to remove local changes before it + * @returns void */
Line range hint
1417-1433: Consider improving parameter documentation.The implementation correctly addresses the snapshot synchronization issue by properly handling local changes. However, the documentation could be enhanced for better maintainability.
Consider adding more detailed parameter documentation:
/** * `applySnapshot` applies the given snapshot into this document. + * @param serverSeq - The server sequence number for this snapshot + * @param snapshotVector - The version vector of this snapshot + * @param snapshot - The snapshot data as a byte array + * @param clientSeq - The client sequence number to determine which local changes + * to remove. A value of -1 indicates that no local changes + * should be removed. */
Line range hint
1702-1707: Address TODO comment about clientSeq handling.The current implementation doesn't pass clientSeq to applySnapshot, which could lead to inconsistent handling of local changes when applying doc events.
Consider implementing the clientSeq check and passing it to applySnapshot:
// TODO(hackerwins): We need to check the clientSeq of the snapshot. this.applySnapshot( BigInt(serverSeq), converter.hexToVersionVector(snapshotVector), converter.hexToBytes(snapshot), + -1, // Or determine appropriate clientSeq from the event context );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
packages/sdk/src/document/document.ts(5 hunks)
🔇 Additional comments (1)
packages/sdk/src/document/document.ts (1)
1179-1189: LGTM! Improved handling of local changes during snapshot application.
The changes correctly ensure that local changes are handled appropriately in both snapshot and non-snapshot cases. Moving removePushedLocalChanges to the else block prevents duplicate removal since applySnapshot now handles it.
What this PR does / why we need it?
This PR updates the behavior of the Snapshot Event to ensure that it publishes the most recent data reflecting local changes. Additionally, it modifies the CodeMirror example to update the cursor position when a snapshot occurs, which addresses an issue where the cursor momentarily jumps to the top.
Before: Cursor is moved to unintended position when the snapshot event is published (
--backend-snapshot-threshold=1)2024-11-03.1.49.27.mov
After: Cursor is not moving when the snapshot event is published (
--backend-snapshot-threshold=1)2024-11-03.1.50.19.mov
Any background context you want to provide?
The changes made in this PR aim to resolve this issue by ensuring the local changes are reflected in the snapshot and that the cursor position is updated accordingly during the snapshot process.
Problem: When dragging text with mouse, the
anchoris moved to 0. (This issue will be handled in Issues with cursor positioning and scroll behavior during Snapshot Events in CodeMirror codepair#402)2024-11-03.1.56.04.mov
I suspect that the issue occurs because the cursor briefly moves to the top when the code below runs.
What are the relevant tickets?
Fixes #922
Checklist
Summary by CodeRabbit
Summary by CodeRabbit
New Features
YorkiePresence, to better handle user presence and selection states.Bug Fixes
Tests