JsonSyncable class#50
Conversation
📝 WalkthroughWalkthroughAdds a new ChangesJSON Syncable Feature
Sequence Diagram(s)sequenceDiagram
participant Local as Local Syncable
participant Remote as Remote Syncable
participant QueryHelpers
participant FileSystem
Local->>Remote: GetSyncState()
Remote-->>Local: SyncState (ClientHeads)
Local->>QueryHelpers: GetMissingCommits(localState, remoteState)
QueryHelpers-->>Local: missing commits (local & remote)
Local->>FileSystem: read/write commit JSONL files
Local->>Remote: AddRangeFromSync(missingFromRemote)
Remote->>Local: AddRangeFromSync(missingFromLocal)
Local-->>Local: mirror commits to read model
Remote-->>Remote: mirror commits to read model
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~35 minutes Suggested reviewers
🚥 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 |
rmunn
left a comment
rmunn
left a comment
There was a problem hiding this comment.
GitHub won't let me approve a PR that I created, but overall this LGTM. I do think we should use the .jsonl extension rather than .json, and I am a bit concerned about the scenario where one of the JSON files might be empty; I'm not sure that GetMissingCommits would handle that scenario correctly. But maybe it can't come up in practice? If so, then it's not a concern.
Once those two things are addressed then I'm 👍 on this PR.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
src/SIL.Harmony.Tests/JsonSyncableTests.cs (1)
17-50: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winStrengthen assertions beyond file existence/line count.
WritesPerClientFileonly checksFile.Exists, andDoesNotDuplicateFileLinesonly checks line count for a single re-added commit. Neither test verifies that the written line actually round-trips into the original commit's data, nor is there a case with two distinct commits confirming both get appended (positive counterpart to the dedup test).Consider reading the line(s) back, deserializing with
SerializerOptions, and asserting the commit content matches; and adding a case where two different commits for the same client produce two lines.Also applies to: 52-77
🤖 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/SIL.Harmony.Tests/JsonSyncableTests.cs` around lines 17 - 50, The JsonSyncable tests only validate file presence and line count, so strengthen them in WritesPerClientFile and DoesNotDuplicateFileLines by verifying the written commit data round-trips correctly. Read back the per-client file, deserialize the line(s) with SerializerOptions, and assert the resulting Commit matches the original commit fields and ChangeEntities content. Also add a positive case for JsonSyncable.AddRangeFromSync where two distinct commits for the same client produce two appended lines, using the existing JsonSyncable, Commit, and SerializerOptions symbols to locate the test setup.src/SIL.Harmony.Tests/Syncable/SyncableTestHelpers.cs (1)
1-33: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider centralizing the
CreateCommithelper.
SyncableTests.cs(lines 44-59) andCrossSyncableTests.cs(lines 39-55) each define their own privateCreateCommitmethod with overlapping logic for building aCommit. Moving a shared version here (parameterized for single/multiple changes and optional timestamp) would avoid duplication and future drift between the two implementations.🤖 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/SIL.Harmony.Tests/Syncable/SyncableTestHelpers.cs` around lines 1 - 33, The test helpers currently duplicate commit निर्माण logic in separate private CreateCommit methods across SyncableTests and CrossSyncableTests. Add a shared CreateCommit helper to SyncableTestHelpers that can build a Commit for one or many changes and optionally accept a timestamp, then update both test classes to call it instead of maintaining their own copies. Keep the new helper aligned with the existing Commit, CommitMetadata, and CommitChange construction so both test suites use the same implementation.src/SIL.Harmony.Tests/Syncable/JsonSyncableSyncBackend.cs (1)
13-16: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueDispose the transient
ServiceProvider.Building a
ServiceProviderviaAddCrdtDataSample(":memory:")purely to readJsonSerializerOptionsand never disposing it can leak any disposables it registers. SinceBackendscreates a freshJsonSyncableSyncBackendon every access (and[MemberData]is enumerated multiple times by xUnit), this cost/leak can repeat many times per run.♻️ Proposed fix
- private JsonSerializerOptions SerializerOptions { get; } = new ServiceCollection() - .AddCrdtDataSample(":memory:") - .BuildServiceProvider() - .GetRequiredService<JsonSerializerOptions>(); + private static readonly JsonSerializerOptions SerializerOptions = BuildSerializerOptions(); + + private static JsonSerializerOptions BuildSerializerOptions() + { + using var provider = new ServiceCollection() + .AddCrdtDataSample(":memory:") + .BuildServiceProvider(); + return provider.GetRequiredService<JsonSerializerOptions>(); + }🤖 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/SIL.Harmony.Tests/Syncable/JsonSyncableSyncBackend.cs` around lines 13 - 16, Dispose the temporary ServiceProvider created in JsonSyncableSyncBackend when resolving JsonSerializerOptions from AddCrdtDataSample(":memory:"). Replace the inline property initializer with a pattern that creates the provider, reads the JsonSerializerOptions, and disposes the provider immediately (for example via a using scope or equivalent helper), so repeated JsonSyncableSyncBackend instances from Backends do not leak registered disposables.src/SIL.Harmony.Core/QueryHelpers.cs (1)
73-84: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winGroup commits once before iterating client heads.
commits.Where(c => c.ClientId == clientId)re-scans the full sequence once per client; inJsonSyncable.GetChanges, that means repeatedly scanning the bag of all commits read from all files. Materialize by client once, then query the lookup.♻️ Proposed refactor
public static IEnumerable<TCommit> GetMissingCommits<TCommit, TChange>( this IEnumerable<TCommit> commits, SyncState localState, SyncState remoteState) where TCommit : CommitBase<TChange> { + var commitsByClient = commits.ToLookup(c => c.ClientId); foreach (var (clientId, localTimestamp) in localState.ClientHeads) { long? remoteTimestamp = remoteState.ClientHeads.TryGetValue(clientId, out var otherTimestamp) ? otherTimestamp : null; foreach (var commit in GetMissingCommitsForClient( - commits.Where(c => c.ClientId == clientId), localTimestamp, remoteTimestamp)) + commitsByClient[clientId], localTimestamp, remoteTimestamp)) { yield return commit; } } }🤖 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/SIL.Harmony.Core/QueryHelpers.cs` around lines 73 - 84, GetMissingCommits currently re-enumerates the full commits sequence for every client head via commits.Where(c => c.ClientId == clientId), which is especially expensive in JsonSyncable.GetChanges. Update GetMissingCommits<TCommit, TChange> in QueryHelpers to group or lookup commits by ClientId once before looping over localState.ClientHeads, then use that grouped/lookup collection inside GetMissingCommitsForClient instead of rescanning commits each time.
🤖 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.
Inline comments:
In `@src/SIL.Harmony/JsonSyncable.cs`:
- Around line 53-55: `JsonSyncable` has a lock mismatch: `AddRangeFromSync` uses
`ClientLocks` for appends, but `GetSyncState` and `GetChanges` read the same
client files without that per-client lock. Update the read paths in
`GetSyncState` and `GetChanges` to acquire the same `ClientLocks` key used by
the writer so reads are serialized with appends and cannot observe partial JSONL
data or inconsistent commit state.
- Around line 99-113: AllClientFiles()/ClientIdForFile() currently assume every
matching file name contains a valid GUID, so non-canonical temp/conflict files
can crash sync. Update the client-file enumeration/parsing path in JsonSyncable
so GetSyncState/GetChanges ignore files that match the prefix/suffix but do not
contain a parseable client ID. Prefer guarding in ClientIdForFile() or the
caller that iterates AllClientFiles(), using safe GUID parsing and skipping
invalid entries rather than throwing.
---
Nitpick comments:
In `@src/SIL.Harmony.Core/QueryHelpers.cs`:
- Around line 73-84: GetMissingCommits currently re-enumerates the full commits
sequence for every client head via commits.Where(c => c.ClientId == clientId),
which is especially expensive in JsonSyncable.GetChanges. Update
GetMissingCommits<TCommit, TChange> in QueryHelpers to group or lookup commits
by ClientId once before looping over localState.ClientHeads, then use that
grouped/lookup collection inside GetMissingCommitsForClient instead of
rescanning commits each time.
In `@src/SIL.Harmony.Tests/JsonSyncableTests.cs`:
- Around line 17-50: The JsonSyncable tests only validate file presence and line
count, so strengthen them in WritesPerClientFile and DoesNotDuplicateFileLines
by verifying the written commit data round-trips correctly. Read back the
per-client file, deserialize the line(s) with SerializerOptions, and assert the
resulting Commit matches the original commit fields and ChangeEntities content.
Also add a positive case for JsonSyncable.AddRangeFromSync where two distinct
commits for the same client produce two appended lines, using the existing
JsonSyncable, Commit, and SerializerOptions symbols to locate the test setup.
In `@src/SIL.Harmony.Tests/Syncable/JsonSyncableSyncBackend.cs`:
- Around line 13-16: Dispose the temporary ServiceProvider created in
JsonSyncableSyncBackend when resolving JsonSerializerOptions from
AddCrdtDataSample(":memory:"). Replace the inline property initializer with a
pattern that creates the provider, reads the JsonSerializerOptions, and disposes
the provider immediately (for example via a using scope or equivalent helper),
so repeated JsonSyncableSyncBackend instances from Backends do not leak
registered disposables.
In `@src/SIL.Harmony.Tests/Syncable/SyncableTestHelpers.cs`:
- Around line 1-33: The test helpers currently duplicate commit निर्माण logic in
separate private CreateCommit methods across SyncableTests and
CrossSyncableTests. Add a shared CreateCommit helper to SyncableTestHelpers that
can build a Commit for one or many changes and optionally accept a timestamp,
then update both test classes to call it instead of maintaining their own
copies. Keep the new helper aligned with the existing Commit, CommitMetadata,
and CommitChange construction so both test suites use the same implementation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 237a106e-748d-4327-94eb-fbee3a498295
📒 Files selected for processing (9)
src/SIL.Harmony.Core/QueryHelpers.cssrc/SIL.Harmony.Tests/JsonSyncableTests.cssrc/SIL.Harmony.Tests/Syncable/CrossSyncableTests.cssrc/SIL.Harmony.Tests/Syncable/DataModelSyncBackend.cssrc/SIL.Harmony.Tests/Syncable/ISyncableTestBackend.cssrc/SIL.Harmony.Tests/Syncable/JsonSyncableSyncBackend.cssrc/SIL.Harmony.Tests/Syncable/SyncableTestHelpers.cssrc/SIL.Harmony.Tests/Syncable/SyncableTests.cssrc/SIL.Harmony/JsonSyncable.cs
2 participants
closes #45
Got started, but hadn't gotten as far as implementing GetChanges which is likely to be the hardest part of the task.
Summary by CodeRabbit
New Features
.jsonlfiles.Bug Fixes
Tests