fix: Correct merge logic for state updates in sync utility #6277
Conversation
|
@alexwithdev is attempting to deploy a commit to the NextChat Team on Vercel. A member of the Team first needs to authorize it. |
WalkthroughThe pull request updates the Changes
Sequence Diagram(s)sequenceDiagram
participant L as Local State
participant R as Remote State
participant F as mergeWithUpdate()
L->>F: Call mergeWithUpdate(localState, remoteState)
R->>F: Provide remoteState.lastUpdateTime
alt localState.lastUpdateTime > remoteState.lastUpdateTime
F->>L: Merge remote state into local and return remote state
else
F->>R: Merge local state into remote and return local state
end
Poem
β¨ Finishing Touches
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: 1
π Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
π Files selected for processing (1)
app/utils/sync.ts(1 hunks)
π Additional comments (1)
app/utils/sync.ts (1)
158-164: LGTM! The sync logic now correctly preserves the most recent changes.The condition change ensures that:
- When local state is newer: merges remote into local, preserving local changes
- When remote state is newer or equal: merges local into remote, preserving remote changes
This aligns with the PR objective of fixing incorrect data overwriting during sync.
| @@ -153,9 +153,9 @@ export function mergeWithUpdate<T extends { lastUpdateTime?: number }>( | |||
| remoteState: T, | |||
| ) { | |||
| const localUpdateTime = localState.lastUpdateTime ?? 0; | |||
| const remoteUpdateTime = localState.lastUpdateTime ?? 1; | |||
| const remoteUpdateTime = remoteState.lastUpdateTime ?? 1; | |||
There was a problem hiding this comment.
Fix incorrect default value for remoteUpdateTime.
Setting a default value of 1 for remoteUpdateTime while keeping 0 for localUpdateTime artificially makes the remote state newer when both states lack lastUpdateTime. This could lead to unintended data loss if the remote state is actually older.
Apply this diff to use consistent default values:
- const remoteUpdateTime = remoteState.lastUpdateTime ?? 1;
+ const remoteUpdateTime = remoteState.lastUpdateTime ?? 0;π Committable suggestion
βΌοΈ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const remoteUpdateTime = remoteState.lastUpdateTime ?? 1; | |
| const remoteUpdateTime = remoteState.lastUpdateTime ?? 0; |
π» εζ΄η±»ε | Change Type
π εζ΄θ―΄ζ | Description of Change
Fix the issue that local data will be overwritten by the remote one even if it is the latest one when synchronizing.
π θ‘₯ε δΏ‘ζ― | Additional Information
Summary by CodeRabbit