Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR implements server-side support for syncing puzzle resets between clients. The changes enable the system to handle reset events by creating special reset entries in the database and ensuring proper synchronization of reset state across connected clients.
- Adds support for puzzle reset synchronization with new reset entry types
- Implements filtering logic to prevent stale data from appearing after resets
- Updates JavaScript communication to handle reset events between client and server
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| PuzzleItemProperty.cs | Adds IsReset property and CreateReset factory method for reset entries |
| ClientSyncComponent.razor.cs | Implements reset handling logic and filters stale data during sync |
| ClientSyncComponent.razor | Adds JavaScript functions for reset event communication |
| JsPuzzleChange.cs | Defines JsPuzzleReset class for reset data transfer |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
ClientSyncComponent/ClientSyncComponent.Client/Pages/ClientSyncComponent.razor.cs
Show resolved
Hide resolved
ClientSyncComponent/ClientSyncComponent.Client/Pages/ClientSyncComponent.razor.cs
Show resolved
Hide resolved
ClientSyncComponent/ClientSyncComponent.Client/Pages/ClientSyncComponent.razor.cs
Show resolved
Hide resolved
ClientSyncComponent/ClientSyncComponent.Client/Pages/ClientSyncComponent.razor.cs
Show resolved
Hide resolved
| { | ||
| // Remove all data entries for this sub-puzzle | ||
| int removedForReset = newChanges.RemoveAll(e => e.SubPuzzleId == entry.SubPuzzleId && | ||
| e.Timestamp < entry.Timestamp && !e.IsReset); |
There was a problem hiding this comment.
I think you do not need to do the IsReset check here - if there are two resets for the same puzzle you only need one and your timestamp check will keep you from removing yourself
| e.Timestamp < entry.Timestamp && !e.IsReset); | ||
| if (removedForReset > 0) | ||
| { | ||
| removedEntries = true; |
There was a problem hiding this comment.
Why not just say i -= removedForReset; and remove the removedEntries bool and the do loop
There was a problem hiding this comment.
Discussion: Do one loop to identify reset timestamps and then one to build the collection of post-reset data to populate onto the page to guarantee O(N)
| // If this is a reset, we need to send a reset message to the JS side | ||
| await JSRuntime.InvokeVoidAsync("onPuzzleResetSynced", new JsPuzzleReset() | ||
| { | ||
| puzzleIds = new[] { Encoding.UTF8.GetString(Base64UrlTextEncoder.Decode(entry.SubPuzzleId)) }, |
There was a problem hiding this comment.
Why not build a list of IDs and send them all at once
2 participants
This is the server side of syncing resets. It also requires changes in puzzleSync.js. Fixes #1245