improve performance of add changes #49
Conversation
…created snapshots
…ig for using linq2db or not
…mpiled query for better performance
WalkthroughCentralizes package upgrades, adds a benchmarks project, introduces ICrdtRepository/ICrdtRepositoryFactory abstractions, implements a Linq2Db-backed repository wrapper/factory, updates DI and consumers to use interfaces, changes Commit.SetParentHash to return a bool and persist hash updates conditionally, expands tests (including benchmarks), and adjusts sample/README/solution. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor App
participant DI as DI Container
participant RepoFactory as ICrdtRepositoryFactory
participant DbCtxFactory as ICrdtDbContextFactory
participant Repo as ICrdtRepository
App->>DI: Resolve ICrdtRepositoryFactory
App->>RepoFactory: CreateRepository()
RepoFactory->>DbCtxFactory: CreateDbContextAsync()
DbCtxFactory-->>RepoFactory: ICrdtDbContext
RepoFactory-->>App: ICrdtRepository (CrdtRepository or Linq2Db wrapper)
note over App,Repo: Callers (DataModel, ResourceService, SnapshotWorker, tests) now use interfaces
sequenceDiagram
autonumber
participant SnapshotWorker as Worker
participant Repo as ICrdtRepository
participant Commit as Commit
rect rgb(240,248,255)
note right of SnapshotWorker: history-rewrite path in ApplyCommitChanges
SnapshotWorker->>Commit: SetParentHash(previousCommitHash)
alt parent hash changed
SnapshotWorker->>Repo: UpdateCommitHash(commit.Id, commit.Hash, commit.ParentHash)
else no change
note right of SnapshotWorker: skip DB update
end
end
sequenceDiagram
autonumber
participant Linq2DbCrdtRepo as Wrapper
participant OrigRepo as ICrdtRepository (original)
participant L2D as LinqToDB
Wrapper->>OrigRepo: Flush pending changes
Wrapper->>L2D: Delete existing snapshots for IDs
Wrapper->>L2D: Bulk copy new snapshots
loop project per snapshot
Wrapper->>L2D: InsertOrReplace / Delete via runtime-generic reflection
end
Wrapper-->>OrigRepo: Return/complete
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type 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: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
src/SIL.Harmony/ResourceService.cs (1)
152-159: Guard RemoteId against empty/whitespace, not just null.ThrowIfNull won’t catch empty strings. Prefer a stronger validation.
- ArgumentNullException.ThrowIfNull(remoteResource.RemoteId); + if (string.IsNullOrWhiteSpace(remoteResource.RemoteId)) + throw new ArgumentException("RemoteId must be non-empty.", nameof(remoteResource));src/SIL.Harmony.Tests/DataModelTestBase.cs (1)
62-63: ForkDatabase drops the useLinq2DbRepo flag.This can lead to mismatched repo implementations after forking.
- var newTestBase = new DataModelTestBase(connection, alwaysValidate, performanceTest: _performanceTest); + var newTestBase = new DataModelTestBase(connection, alwaysValidate, performanceTest: _performanceTest, useLinq2DbRepo: _useLinq2DbRepo);Add a backing field and set it in ctors:
// new field private readonly bool _useLinq2DbRepo; // in both constructors that accept `useLinq2DbRepo` _useLinq2DbRepo = useLinq2DbRepo;src/SIL.Harmony/DataModel.cs (2)
197-216: Reduce duplicate IDs in the IN-clause to lower query size and CPU.Distinct the entity IDs before querying current snapshots.
- if (newCommits.Length > 10) + if (newCommits.Length > 10) { - var entityIds = newCommits.SelectMany(c => c.ChangeEntities.Select(ce => ce.EntityId)); + var entityIds = newCommits + .SelectMany(c => c.ChangeEntities.Select(ce => ce.EntityId)) + .Distinct() + .ToArray(); snapshotLookup = await repo.CurrentSnapshots() - .Where(s => entityIds.Contains(s.EntityId)) + .Where(s => entityIds.Contains(s.EntityId)) .Select(s => new KeyValuePair<Guid, Guid?>(s.EntityId, s.Id)) .ToDictionaryAsync(s => s.Key, s => s.Value); }
282-286: RenameCurrenSimpleSnapshotstoCurrentSimpleSnapshotseverywhere.
Update the interface (src/SIL.Harmony/Db/ICrdtRepository.cs:19), its implementations (src/SIL.Harmony/Db/CrdtRepository.cs:161; src/SIL.Harmony.Linq2db/Linq2DbCrdtRepo.cs:187) and the call site in src/SIL.Harmony/DataModel.cs:284.src/SIL.Harmony/Db/CrdtRepository.cs (1)
47-55: In-memory DBs don’t share the lock due to random key.Using Guid.NewGuid() for ":memory:" yields a different lock per repository, defeating cross-repo synchronization and risking races. Use a stable key based on the actual DbConnection instance.
- var connection = _dbContext.Database.GetDbConnection(); - if (connection.ConnectionString is ":memory:") return Guid.NewGuid().ToString(); - return connection.ConnectionString; + var connection = _dbContext.Database.GetDbConnection(); + if (connection.ConnectionString is ":memory:") + // stable per-connection key; avoids leaking the raw connection string + return System.Runtime.CompilerServices.RuntimeHelpers.GetHashCode(connection).ToString(); + return connection.ConnectionString;Alternatively, use a ConditionalWeakTable<DbConnection, AsyncLock> keyed by connection to avoid retaining disposed connections.
src/SIL.Harmony.Tests/RepositoryTests.cs (1)
446-469: Scoped repository should preserve the Linq2Db wrapper.Returning the concrete CrdtRepository from ICrdtRepository.GetScopedRepository drops the wrapper (and its bulk/projection path) within scopes.
Once ICrdtRepository.GetScopedRepository returns ICrdtRepository, the wrapper can wrap the scoped repo, keeping consistent behavior. See proposed interface and wrapper changes in respective files.
Also applies to: 471-493
🧹 Nitpick comments (29)
src/SIL.Harmony.Linq2db/SIL.Harmony.Linq2db.csproj (1)
13-15: Confirm intentional use of prerelease linq2db.Extensions (align with EFCore + linq2db EFCore).Good to add, and version is centrally managed. Since both linq2db.Extensions and linq2db.EntityFrameworkCore are RCs, please confirm this is intentional and validated against EF Core 9.0.7 to avoid breaking changes on RC bumps. Consider planning an upgrade to stable once available.
src/SIL.Harmony.Tests/DbContextTests.VerifyModel.verified.txt (1)
240-240: Snapshot bump matches EF Core upgrade.ProductVersion updated to 9.0.7 is expected. If version churn causes noisy diffs, consider scrubbing this annotation in Verify settings.
src/SIL.Harmony.Tests/SIL.Harmony.Tests.csproj (1)
17-17: Add PrivateAssets to keep test-only logging fully local.Not required (test project isn’t packable), but adding PrivateAssets avoids accidental transitive flow if project settings change.
Apply this diff:
- <PackageReference Include="Microsoft.Extensions.Logging.Debug" /> + <PackageReference Include="Microsoft.Extensions.Logging.Debug"> + <PrivateAssets>all</PrivateAssets> + </PackageReference>src/SIL.Harmony.Sample/Models/Definition.cs (1)
43-46: Harden ToString against locale variance and include key fields.Use invariant formatting (for DateTime/Order) and include PartOfSpeech/Order/OneWordDefinition to aid debugging.
Apply this diff:
- public override string ToString() - { - return $"{nameof(Text)}: {Text}, {nameof(Id)}: {Id}, {nameof(WordId)}: {WordId}, {nameof(DeletedAt)}: {DeletedAt}"; - } + public override string ToString() + { + return + $"{nameof(Text)}: {Text}, {nameof(Id)}: {Id}, {nameof(WordId)}: {WordId}, " + + $"{nameof(Order)}: {Order.ToString(System.Globalization.CultureInfo.InvariantCulture)}, " + + $"{nameof(PartOfSpeech)}: {PartOfSpeech}, " + + $"{nameof(OneWordDefinition)}: {OneWordDefinition}, " + + $"{nameof(DeletedAt)}: {DeletedAt?.ToString("O", System.Globalization.CultureInfo.InvariantCulture)}"; + }If you prefer, add a
using System.Globalization;at top and drop the fully qualified names.Directory.Packages.props (1)
6-13: Audit prerelease packages and scope non-shipping dependencies
- Directory.Packages.props contains prerelease versions—linq2db.Extensions 6.0.0-rc.1, linq2db.EntityFrameworkCore 9.1.0-rc.1, FluentAssertions 7.0.0-alpha.6. Confirm these RC/alpha versions are required and plan migration to stable.
- In src/SIL.Harmony.Benchmarks/SIL.Harmony.Benchmarks.csproj and src/SIL.Harmony.Tests/SIL.Harmony.Tests.csproj, add
<PrivateAssets>all</PrivateAssets>to all BenchmarkDotNet*, Enterprize1.BenchmarkDotNet.GitCompare and FluentAssertions<PackageReference>entries to prevent transitive flow.src/SIL.Harmony/Db/ObjectSnapshot.cs (1)
81-84: Make ToString null-safe and avoid empty string for null Entity.Interpolating a null Entity prints nothing, which is ambiguous; prefer explicit "". Also avoids accidental heavy ToString chains when null.
- return $"{Id} [{CommitId}] {TypeName} {EntityId} Deleted:{EntityIsDeleted}, Entity: {Entity}"; + return $"{Id} [{CommitId}] {TypeName} {EntityId} Deleted:{EntityIsDeleted}, Entity: {(Entity is null ? "<null>" : Entity.ToString())}";src/SIL.Harmony.Benchmarks/SIL.Harmony.Benchmarks.csproj (2)
3-8: Don’t pack the benchmarks project.Bench projects are tooling; avoid producing NuGet artifacts.
<PropertyGroup> <OutputType>Exe</OutputType> - <TargetFramework>net9.0</TargetFramework> + <TargetFramework>net8.0</TargetFramework> <ImplicitUsings>enable</ImplicitUsings> <Nullable>enable</Nullable> + <IsPackable>false</IsPackable> </PropertyGroup>
1-21: Optional: Multi-target only after CI supports .NET 9.When CI has .NET 9 SDK, you can multi-target. Note: dotnet test will build all TFMs, so ensure CI has both before adding.
- <TargetFramework>net8.0</TargetFramework> + <TargetFrameworks>net8.0;net9.0</TargetFrameworks>src/SIL.Harmony.Benchmarks/Program.cs (1)
1-4: Prefer classic array initializer for broader compiler compatibilityThe collection expression [] requires newer language version. Use new[] {} to avoid build failures on stricter LangVersion settings.
-BenchmarkSwitcher.FromAssemblies([typeof(Program).Assembly, typeof(ChangeThroughput).Assembly]).Run(args); +BenchmarkSwitcher.FromAssemblies(new[] { typeof(Program).Assembly, typeof(ChangeThroughput).Assembly }).Run(args);Re-evaluate dependency direction: benchmarks referencing test assembly
Running a benchmark type from the Tests assembly couples Benchmarks -> Tests. Consider moving ChangeThroughput to the Benchmarks project (or a shared project) and consuming test helpers via a shared library to avoid circular or brittle references.
src/SIL.Harmony.Tests/DataModelSimpleChanges.cs (1)
216-216: Use AsNoTracking for read-only assertion to avoid EF change tracking overheadThis is a hot path in tests; AsNoTracking reduces statefulness and surprises.
- var objectSnapshot = await DbContext.Snapshots.SingleAsync(s => s.CommitId == commit.Id); + var objectSnapshot = await DbContext.Snapshots + .AsNoTracking() + .SingleAsync(s => s.CommitId == commit.Id);src/SIL.Harmony.Sample/CrdtSampleKernel.cs (2)
19-20: Boolean-flag soup: prefer options object or named argsAdding another bool (useLinq2DbRepo) makes call sites error-prone. Consider an options record (e.g., CrdtSampleOptions { PerformanceTest, UseLinq2DbRepo }) to improve clarity and extensibility.
79-81: Verify service replacement semantics when enabling Linq2Db repositoryEnsure AddLinq2DbRepository replaces any default ICrdtRepository registrations set by AddCrdtData, not just adds another implementation. If not, prefer ServiceCollection.Replace or explicit TryAdd/Remove to avoid ambiguous resolution.
If replacement is needed, update AddLinq2DbRepository to use Microsoft.Extensions.DependencyInjection.Extensions.ServiceCollectionDescriptorExtensions.Replace.
src/SIL.Harmony.Benchmarks/AddSnapshotsBenchmarks.cs (3)
1-10: Remove unused usings to reduce noise.
BenchmarkDotNet_GitCompareandSIL.Harmony.Linq2dbaren't used.-using BenchmarkDotNet_GitCompare; +// using BenchmarkDotNet_GitCompare; // enable if [GitJob] is used @@ -using SIL.Harmony.Linq2db;
22-26: Keep OperationsPerInvoke aligned with SnapshotCount.Use a single const for both to avoid drift if the param changes.
public class AddSnapshotsBenchmarks { private DataModelTestBase _emptyDataModel = null!; private ICrdtRepository _repository = null!; private Commit _commit = null!; + private const int DefaultSnapshotCount = 1000; @@ - [Params(1000)] + [Params(DefaultSnapshotCount)] public int SnapshotCount { get; set; } @@ - [Benchmark(OperationsPerInvoke = 1000)] + [Benchmark(OperationsPerInvoke = DefaultSnapshotCount)] public void AddSnapshotsOneAtATime() @@ - [Benchmark(OperationsPerInvoke = 1000)] + [Benchmark(OperationsPerInvoke = DefaultSnapshotCount)] public void AddSnapshotsAllAtOnce()Also applies to: 42-43, 57-58
43-55: Avoid per-iteration array allocation in the hot loop.Reuse a 1-element buffer to cut GC pressure in the one-at-a-time benchmark.
public void AddSnapshotsOneAtATime() { - for (var i = 0; i < SnapshotCount; i++) + var single = new ObjectSnapshot[1]; + for (var i = 0; i < SnapshotCount; i++) { - _repository.AddSnapshots([ - new ObjectSnapshot(new Word() - { - Id = Guid.NewGuid(), - Text = "test", - }, _commit, true) - ]).GetAwaiter().GetResult(); + single[0] = new ObjectSnapshot( + new Word { Id = Guid.NewGuid(), Text = "test" }, + _commit, + true); + _repository.AddSnapshots(single).GetAwaiter().GetResult(); } }src/SIL.Harmony.Benchmarks/AddChangeBenchmarks.cs (2)
31-41: Pre-size the list to avoid growth re-allocations.Initialize with capacity to reduce allocations and GC during tight loops.
- public List<Commit> AddChanges() + public List<Commit> AddChanges() { - var commits = new List<Commit>(); + var commits = new List<Commit>(ActualChangeCount); for (var i = 0; i < ActualChangeCount; i++) { commits.Add(_emptyDataModel.WriteNextChange(_emptyDataModel.SetWord(Guid.NewGuid(), "entity1")).Result); } return commits; }
46-49: Use the benchmark’s mock time for determinism.Replace DateTimeOffset.Now with the model’s NextDate() to keep timestamps monotonic and iteration-independent.
- return _emptyDataModel.WriteChange(_clientId, DateTimeOffset.Now, Enumerable.Range(0, ActualChangeCount) + return _emptyDataModel.WriteChange(_clientId, _emptyDataModel.NextDate(), Enumerable.Range(0, ActualChangeCount) .Select(i => _emptyDataModel.SetWord(Guid.NewGuid(), "entity1"))).Result;src/SIL.Harmony/Db/CrdtRepositoryFactory.cs (1)
35-38: Narrow the surface of CreateInstance.CreateInstance exposing the concrete CrdtRepository invites misuse and breaks abstraction. Make it private.
- public CrdtRepository CreateInstance(ICrdtDbContext dbContext) + private CrdtRepository CreateInstance(ICrdtDbContext dbContext) { return ActivatorUtilities.CreateInstance<CrdtRepository>(serviceProvider, dbContext); }src/SIL.Harmony/Db/CrdtRepository.cs (4)
235-243: Apply the same optimization to the non-tracking path.- return await Snapshots - .AsTracking(tracking) - .Include(s => s.Commit) - .DefaultOrder() - .LastOrDefaultAsync(s => s.EntityId == objectId); + return await Snapshots + .AsTracking(tracking) + .Where(s => s.EntityId == objectId) + .Include(s => s.Commit) + .DefaultOrderDescending() + .FirstOrDefaultAsync();
161-177: Typo: CurrenSimpleSnapshots.Public method name is misspelled. Provide a correctly named wrapper and mark the old one [Obsolete] to avoid a breaking change.
Additional code (outside diff scope):
[Obsolete("Use CurrentSimpleSnapshots")] public IAsyncEnumerable<SimpleSnapshot> CurrenSimpleSnapshots(bool includeDeleted = false) => CurrentSimpleSnapshots(includeDeleted); public IAsyncEnumerable<SimpleSnapshot> CurrentSimpleSnapshots(bool includeDeleted = false) { var queryable = CurrentSnapshots(); if (!includeDeleted) queryable = queryable.Where(s => !s.EntityIsDeleted); return queryable.Select(s => new SimpleSnapshot(/* ... */)) .AsNoTracking() .AsAsyncEnumerable(); }
345-348: Interface leak: returns concrete CrdtRepository.GetScopedRepository returning the concrete type breaks substitutability (e.g., Linq2Db wrapper). Prefer ICrdtRepository.
This requires updating ICrdtRepository signature and call sites to return ICrdtRepository.
362-370: Consider checking the update count.Optionally validate that exactly one row was updated to catch missing/removed commits during history rewrite.
- await _dbContext.Commits + var updated = await _dbContext.Commits .Where(c => c.Id == commitId) .ExecuteUpdateAsync(s => s .SetProperty(c => c.Hash, hash) .SetProperty(c => c.ParentHash, parentHash) ); + if (updated != 1) _logger.LogWarning("UpdateCommitHash affected {Updated} rows for {CommitId}", updated, commitId);src/SIL.Harmony.Tests/Benchmarks/ChangeThroughput.cs (2)
35-39: Add MemoryDiagnoser to capture allocations.Helps compare memory impact across StartingSnapshots.
-[SimpleJob(RunStrategy.Throughput, warmupCount: 2)] +[SimpleJob(RunStrategy.Throughput, warmupCount: 2)] +[MemoryDiagnoser] public class ChangeThroughput
44-47: Avoid synchronous Count() on DbSet during the measured phase.Precompute in IterationSetup (or use AsNoTracking().LongCount()) to keep WriteChange-focused timing clean.
- var count = _dataModelTestBase.DbContext.Snapshots.Count(); + var count = _dataModelTestBase.DbContext.Snapshots.AsNoTracking().LongCount();Or cache the value during IterationSetup.
src/SIL.Harmony.Tests/RepositoryTests.cs (3)
21-29: DI setup looks good; consider adding Console logger for CI visibility.AddDebug is silent in many CI runners; Console logging improves failure triage.
.AddCrdtDataSample(builder => builder.UseSqlite("Data Source=:memory:"), useLinq2DbRepo: useLinq2DbRepo) -.AddLogging(c => c.AddDebug()) +.AddLogging(c => { c.AddDebug(); c.AddConsole(); })
66-69: Remove empty object initializer.No-op initializer adds noise.
- return new(new Word { Text = text ?? "test", Id = entityId }, Commit(commitId, time), false) { }; + return new(new Word { Text = text ?? "test", Id = entityId }, Commit(commitId, time), false);
71-75: Optional: clear tracker after seeding via DbContext helper.Reduces accidental state bleed between asserts.
_crdtDbContext.AddRange(snapshots); - await _crdtDbContext.SaveChangesAsync(); + await _crdtDbContext.SaveChangesAsync(); + _crdtDbContext.ChangeTracker.Clear();src/SIL.Harmony.Linq2db/Linq2DbCrdtRepo.cs (2)
81-83: Avoid lambda shadowing for readability.Shadowing
stwice is confusing.- .Where(s => objectSnapshots.Select(s => s.Id).Contains(s.Id)) + .Where(db => objectSnapshots.Select(os => os.Id).Contains(db.Id))
103-106: Preserve exception type and add context without generic Exception.Use InvalidOperationException (or a domain exception) with snapshot id/type; do not downcast all errors to Exception.
See the AddSnapshots diff above where InvalidOperationException is used with contextual message.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (28)
Directory.Packages.props(2 hunks)README.md(1 hunks)harmony.sln(2 hunks)src/SIL.Harmony.Benchmarks/AddChangeBenchmarks.cs(1 hunks)src/SIL.Harmony.Benchmarks/AddSnapshotsBenchmarks.cs(1 hunks)src/SIL.Harmony.Benchmarks/Program.cs(1 hunks)src/SIL.Harmony.Benchmarks/SIL.Harmony.Benchmarks.csproj(1 hunks)src/SIL.Harmony.Linq2db/Linq2DbCrdtRepo.cs(1 hunks)src/SIL.Harmony.Linq2db/Linq2dbKernel.cs(2 hunks)src/SIL.Harmony.Linq2db/SIL.Harmony.Linq2db.csproj(1 hunks)src/SIL.Harmony.Sample/CrdtSampleKernel.cs(2 hunks)src/SIL.Harmony.Sample/Models/Definition.cs(1 hunks)src/SIL.Harmony.Tests/Benchmarks/ChangeThroughput.cs(1 hunks)src/SIL.Harmony.Tests/DataModelPerformanceTests.cs(2 hunks)src/SIL.Harmony.Tests/DataModelSimpleChanges.cs(1 hunks)src/SIL.Harmony.Tests/DataModelTestBase.cs(3 hunks)src/SIL.Harmony.Tests/DbContextTests.VerifyModel.verified.txt(1 hunks)src/SIL.Harmony.Tests/RepositoryTests.cs(11 hunks)src/SIL.Harmony.Tests/SIL.Harmony.Tests.csproj(1 hunks)src/SIL.Harmony/Commit.cs(1 hunks)src/SIL.Harmony/CrdtKernel.cs(3 hunks)src/SIL.Harmony/DataModel.cs(3 hunks)src/SIL.Harmony/Db/CrdtRepository.cs(4 hunks)src/SIL.Harmony/Db/CrdtRepositoryFactory.cs(1 hunks)src/SIL.Harmony/Db/ICrdtRepository.cs(1 hunks)src/SIL.Harmony/Db/ObjectSnapshot.cs(1 hunks)src/SIL.Harmony/ResourceService.cs(2 hunks)src/SIL.Harmony/SnapshotWorker.cs(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (17)
src/SIL.Harmony.Sample/Models/Definition.cs (2)
src/SIL.Harmony/Db/ObjectSnapshot.cs (1)
ToString(81-84)src/SIL.Harmony.Sample/Models/Word.cs (1)
ToString(46-51)
src/SIL.Harmony/Db/ObjectSnapshot.cs (2)
src/SIL.Harmony.Sample/Models/Definition.cs (1)
ToString(43-46)src/SIL.Harmony.Sample/Models/Word.cs (1)
ToString(46-51)
src/SIL.Harmony/Db/ICrdtRepository.cs (7)
src/SIL.Harmony.Linq2db/Linq2DbCrdtRepo.cs (2)
ICrdtRepository(25-28)ClearChangeTracker(64-67)src/SIL.Harmony/Db/CrdtRepositoryFactory.cs (2)
ICrdtRepository(8-8)ICrdtRepository(30-33)src/SIL.Harmony/Db/CrdtRepository.cs (1)
ClearChangeTracker(59-62)src/SIL.Harmony/Commit.cs (3)
Commit(7-43)Commit(17-21)Commit(30-33)src/SIL.Harmony/DataModel.cs (1)
Commit(102-112)src/SIL.Harmony/Db/ObjectSnapshot.cs (1)
SimpleSnapshot(20-29)src/SIL.Harmony/Resource/LocalResource.cs (1)
LocalResource(6-16)
src/SIL.Harmony.Benchmarks/AddChangeBenchmarks.cs (2)
src/SIL.Harmony.Tests/DataModelTestBase.cs (6)
DataModelTestBase(15-189)DataModelTestBase(26-31)DataModelTestBase(33-35)DataModelTestBase(37-53)DataModelTestBase(55-65)DateTimeOffset(74-74)src/SIL.Harmony/Commit.cs (3)
Commit(7-43)Commit(17-21)Commit(30-33)
src/SIL.Harmony.Sample/CrdtSampleKernel.cs (1)
src/SIL.Harmony.Linq2db/Linq2dbKernel.cs (1)
DbContextOptionsBuilder(16-49)
src/SIL.Harmony.Benchmarks/AddSnapshotsBenchmarks.cs (6)
src/SIL.Harmony.Benchmarks/AddChangeBenchmarks.cs (5)
SimpleJob(11-56)IterationSetup(24-29)Benchmark(31-41)Benchmark(43-49)IterationCleanup(51-55)src/SIL.Harmony.Tests/DataModelTestBase.cs (6)
DataModelTestBase(15-189)DataModelTestBase(26-31)DataModelTestBase(33-35)DataModelTestBase(37-53)DataModelTestBase(55-65)DateTimeOffset(74-74)src/SIL.Harmony.Linq2db/Linq2DbCrdtRepo.cs (3)
ICrdtRepository(25-28)HybridDateTime(253-256)Dispose(54-57)src/SIL.Harmony/Db/CrdtRepositoryFactory.cs (2)
ICrdtRepository(8-8)ICrdtRepository(30-33)src/SIL.Harmony/Commit.cs (3)
Commit(7-43)Commit(17-21)Commit(30-33)src/SIL.Harmony.Sample/Models/Word.cs (1)
Word(5-52)
src/SIL.Harmony/ResourceService.cs (2)
src/SIL.Harmony/DataModel.cs (2)
DataModel(14-371)DataModel(28-39)src/SIL.Harmony/Db/CrdtRepositoryFactory.cs (2)
ICrdtRepository(8-8)ICrdtRepository(30-33)
src/SIL.Harmony/DataModel.cs (2)
src/SIL.Harmony/Db/CrdtRepository.cs (16)
Task(68-71)Task(76-79)Task(81-94)Task(96-105)Task(107-117)Task(121-124)Task(179-191)Task(193-196)Task(198-205)Task(207-215)Task(217-223)Task(235-243)Task(245-253)Task(255-259)Task(270-273)Task(275-278)src/SIL.Harmony/Db/CrdtRepositoryFactory.cs (2)
ICrdtRepository(8-8)ICrdtRepository(30-33)
src/SIL.Harmony/Db/CrdtRepositoryFactory.cs (3)
src/SIL.Harmony.Linq2db/Linq2DbCrdtRepo.cs (3)
ICrdtRepository(25-28)ValueTask(49-52)CrdtRepository(248-251)src/SIL.Harmony/Db/CrdtRepository.cs (21)
Task(68-71)Task(76-79)Task(81-94)Task(96-105)Task(107-117)Task(121-124)Task(179-191)Task(193-196)Task(198-205)Task(207-215)Task(217-223)Task(235-243)Task(245-253)ValueTask(308-336)ValueTask(338-343)ValueTask(415-418)ValueTask(432-435)ValueTask(475-478)CrdtRepository(15-419)CrdtRepository(24-35)CrdtRepository(345-348)src/SIL.Harmony/Db/ICrdtRepository.cs (1)
CrdtRepository(32-32)
src/SIL.Harmony.Linq2db/Linq2dbKernel.cs (4)
src/SIL.Harmony.Sample/CrdtSampleKernel.cs (2)
IServiceCollection(13-16)IServiceCollection(18-82)src/SIL.Harmony/CrdtKernel.cs (3)
IServiceCollection(13-20)IServiceCollection(22-28)IServiceCollection(29-54)src/SIL.Harmony/Db/CrdtRepositoryFactory.cs (1)
CrdtRepositoryFactory(23-39)src/SIL.Harmony.Linq2db/Linq2DbCrdtRepo.cs (1)
Linq2DbCrdtRepoFactory(15-36)
src/SIL.Harmony/Db/CrdtRepository.cs (3)
src/SIL.Harmony.Linq2db/Linq2DbCrdtRepo.cs (4)
CrdtRepository(248-251)ICrdtRepository(25-28)ClearChangeTracker(64-67)HybridDateTime(253-256)src/SIL.Harmony/Db/ICrdtRepository.cs (3)
CrdtRepository(32-32)ClearChangeTracker(10-10)HybridDateTime(36-36)src/SIL.Harmony/Db/CrdtRepositoryFactory.cs (3)
CrdtRepository(35-38)ICrdtRepository(8-8)ICrdtRepository(30-33)
src/SIL.Harmony.Linq2db/Linq2DbCrdtRepo.cs (2)
src/SIL.Harmony/Db/CrdtRepositoryFactory.cs (4)
CrdtRepositoryFactory(23-39)ICrdtRepository(8-8)ICrdtRepository(30-33)CrdtRepository(35-38)src/SIL.Harmony/Db/CrdtRepository.cs (16)
Task(68-71)Task(76-79)Task(81-94)Task(96-105)Task(107-117)Task(121-124)Task(179-191)Task(193-196)Task(198-205)Task(207-215)Task(217-223)Task(235-243)Task(245-253)CrdtRepository(15-419)CrdtRepository(24-35)CrdtRepository(345-348)
src/SIL.Harmony.Tests/DataModelTestBase.cs (3)
src/SIL.Harmony.Linq2db/Linq2dbKernel.cs (1)
IServiceCollection(51-57)src/SIL.Harmony.Sample/CrdtSampleKernel.cs (2)
IServiceCollection(13-16)IServiceCollection(18-82)src/SIL.Harmony/CrdtKernel.cs (3)
IServiceCollection(13-20)IServiceCollection(22-28)IServiceCollection(29-54)
src/SIL.Harmony/SnapshotWorker.cs (2)
src/SIL.Harmony.Linq2db/Linq2DbCrdtRepo.cs (1)
ICrdtRepository(25-28)src/SIL.Harmony/Commit.cs (1)
SetParentHash(23-29)
src/SIL.Harmony.Tests/RepositoryTests.cs (7)
src/SIL.Harmony.Linq2db/Linq2DbCrdtRepo.cs (2)
ICrdtRepository(25-28)HybridDateTime(253-256)src/SIL.Harmony/Db/CrdtRepositoryFactory.cs (2)
ICrdtRepository(8-8)ICrdtRepository(30-33)src/SIL.Harmony.Sample/SampleDbContext.cs (1)
SampleDbContext(8-18)src/SIL.Harmony/Db/ObjectSnapshot.cs (2)
ObjectSnapshot(32-85)ObjectSnapshot(34-48)src/SIL.Harmony/Db/CrdtRepository.cs (18)
HybridDateTime(372-379)Task(68-71)Task(76-79)Task(81-94)Task(96-105)Task(107-117)Task(121-124)Task(179-191)Task(193-196)Task(198-205)Task(207-215)Task(217-223)Task(235-243)Task(245-253)Task(255-259)Task(270-273)Task(275-278)AddRange(460-463)src/SIL.Harmony/Db/CrdtDbContextFactory.cs (1)
AddRange(87-90)src/SIL.Harmony/Db/ICrdtDbContext.cs (1)
AddRange(19-19)
src/SIL.Harmony/Commit.cs (3)
src/SIL.Harmony.Tests/RepositoryTests.cs (2)
Commit(48-64)Guid(77-80)src/SIL.Harmony.Tests/Db/QueryHelperTests.cs (2)
Commit(14-15)Guid(23-26)src/SIL.Harmony.Core/CommitBase.cs (1)
GenerateHash(32-40)
src/SIL.Harmony/CrdtKernel.cs (3)
src/SIL.Harmony/Db/CrdtRepositoryFactory.cs (1)
CrdtRepositoryFactory(23-39)src/SIL.Harmony/DataModel.cs (2)
DataModel(14-371)DataModel(28-39)src/SIL.Harmony/ResourceService.cs (2)
ResourceService(11-199)ResourceService(18-24)
🪛 markdownlint-cli2 (0.17.2)
README.md
192-192: Bare URL used
(MD034, no-bare-urls)
🪛 GitHub Actions: Build NuGet packages
src/SIL.Harmony.Benchmarks/SIL.Harmony.Benchmarks.csproj
[error] 1-1: dotnet test --configuration Release --logger GitHubActions failed. NETSDK1045: The current .NET SDK does not support targeting .NET 9.0. Either target .NET 8.0 or lower, or use a version of the .NET SDK that supports .NET 9.0. Download the .NET SDK from https://aka.ms/dotnet/download
🪛 GitHub Actions: Tests
src/SIL.Harmony.Benchmarks/SIL.Harmony.Benchmarks.csproj
[error] 1-1: Command 'dotnet test --configuration Release --logger GitHubActions' failed. NETSDK1045: The current .NET SDK does not support targeting .NET 9.0. Either target .NET 8.0 or lower, or use a version of the .NET SDK that supports .NET 9.0. Download the .NET SDK from https://aka.ms/dotnet/download.
🪛 GitHub Actions: PR #49
src/SIL.Harmony.Benchmarks/SIL.Harmony.Benchmarks.csproj
[error] 1-1: NETSDK1045: The current .NET SDK does not support targeting .NET 9.0. Either target .NET 8.0 or lower, or use a version of the .NET SDK that supports .NET 9.0.
🔇 Additional comments (25)
harmony.sln (1)
24-25: Benchmarks project added to solution — LGTM.Solution mappings look correct for Debug/Release.
Also applies to: 59-62
src/SIL.Harmony/ResourceService.cs (1)
13-13: Good switch to ICrdtRepository abstractions.Constructor and field type updates improve testability and decouple from concrete repo.
Also applies to: 18-18
src/SIL.Harmony.Benchmarks/SIL.Harmony.Benchmarks.csproj (1)
10-15: All referenced packages are centrally versioned. BenchmarkDotNet, BenchmarkDotNet.Diagnostics.dotMemory, BenchmarkDotNet.Diagnostics.dotTrace, and Enterprize1.BenchmarkDotNet.GitCompare all have entries in Directory.Packages.props, so no further action is needed.src/SIL.Harmony.Tests/DataModelPerformanceTests.cs (2)
14-14: Import OKNeeded for ChangeThroughput; no issues.
29-34: Incorrect review comment - ChangeThroughput already has proper job configurationThe review comment is incorrect. The
ChangeThroughputclass already has a[SimpleJob(RunStrategy.Throughput, warmupCount: 2)]attribute on line 9, which means it declares its own job configuration. When usingManualConfig.CreateEmpty(), this existing job attribute will still be respected, so no additional job configuration is needed in the manual config.Additionally, the class has a proper baseline configuration with
[Benchmark(Baseline = true)]on theAddSingleChangePerformancemethod, so the BaselineRatioColumn concern is also unfounded.The suggested fix to add
.AddJob(BenchmarkDotNet.Jobs.Job.ShortRun)would actually conflict with the existing[SimpleJob]attribute and is unnecessary.Likely an incorrect or invalid review comment.
src/SIL.Harmony/Commit.cs (1)
17-21: Public Commit(Guid) usage is intentional; EF and JSON continue using their designated constructors
EF Core will materialize via the internal parameterless ctor, and System.Text.Json will invoke the protected[JsonConstructor]overload. All calls tonew Commit(Guid)are confined to benchmarks/tests and factory methods.src/SIL.Harmony.Sample/CrdtSampleKernel.cs (1)
27-27: Good: toggle Linq2DB logging based on performanceTestPassing !performanceTest into UseLinqToDbCrdt is sensible for CI/bench contexts.
src/SIL.Harmony.Linq2db/Linq2dbKernel.cs (2)
42-47: Conditional logging wiring looks good.
51-56: DI registration for Linq2Db repo factory is correct.
RemoveAll<ICrdtRepositoryFactory>()ensures the override is effective; scoping aligns with the rest of the graph.src/SIL.Harmony.Tests/DataModelTestBase.cs (3)
24-25: Exposing IServiceProvider is useful for advanced test scenarios.
37-47: Wiring of useLinq2DbRepo into AddCrdtDataSample and conditional logging looks good.
104-121: Making WriteChange(...) public improves reuse in benchmarks and tests.src/SIL.Harmony/CrdtKernel.cs (2)
37-41: Switch to ICrdtRepositoryFactory throughout DI is correct and future-proof.Also applies to: 47-49
61-62: No change needed: ICrdtRepository implements IDisposable
Confirmed thatICrdtRepositoryextendsIDisposable(andIAsyncDisposable) and thatLinq2DbCrdtRepoprovides a synchronousDispose(), sousing varis appropriate.src/SIL.Harmony/DataModel.cs (2)
21-39: Constructor and field type changes to interface-based factory look good.
218-236: Hash-chain validation logic is solid and scoped to AlwaysValidate.src/SIL.Harmony.Benchmarks/AddChangeBenchmarks.cs (1)
24-30: Consider isolating construction overhead from measurement.IterationSetup seeds once, but per-iteration construction cost (ServiceProvider, DbContext) dominates small payloads. If you want pure AddChanges cost, move setup to GlobalSetup and fork per-iteration contexts if needed.
src/SIL.Harmony/Db/CrdtRepositoryFactory.cs (1)
10-21: Ensure ICrdtRepository implements IAsyncDisposable.The default Execute methods use “await using”; the interface type must guarantee IAsyncDisposable to compile and dispose correctly.
If ICrdtRepository doesn’t extend IAsyncDisposable, add it to the interface or change the default methods to dispose conditionally.
src/SIL.Harmony/Db/CrdtRepository.cs (1)
31-35: Prefer a connection-based lock for “:memory:”.Follow-up to the above: initialize _lock using the stable DatabaseIdentifier so concurrent repos share the same guard.
Verify multi-repo scenarios under in-memory mode (tests/benchmarks) no longer interleave writes when run in parallel.
src/SIL.Harmony/SnapshotWorker.cs (3)
14-29: Good shift to ICrdtRepository for testability and alt backends.
71-76: Conditional hash persistence avoids unnecessary writes.The SetParentHash() boolean combined with UpdateCommitHash is the right trade-off for history rewrite.
190-198: Potential translation risk in predicate over IQueryable.GetSnapshotsWhere compiles the predicate for in-memory dictionaries but also applies predicateExpression to CurrentSnapshots() (IQueryable). Ensure all expressions passed are EF-translatable; otherwise, enumerate first or split paths.
If needed, materialize IDs from CurrentSnapshots() before applying non-translatable predicates.
src/SIL.Harmony.Tests/RepositoryTests.cs (2)
13-14: Nice: dual-path test entry to exercise Linq2Db.This cleanly reuses the suite for both repos.
380-405: Verify provider translation of array Contains on References.SQLite/EFCore and Linq2Db may not both translate Guid[] Contains(...) over JSON/converted columns. If provider falls back to client eval, query will throw or underperform.
You can assert server-side translation by inspecting the generated SQL/logs or, minimally, run a focused test with logging set to Information for Command and fail on warnings.
src/SIL.Harmony.Linq2db/Linq2DbCrdtRepo.cs (1)
15-36: Factory wiring is fine.Using the base CrdtRepositoryFactory here prevents recursive wrapping; good.
| ### Run benchmarks | ||
|
|
||
| docs: https://benchmarkdotnet.org/articles/guides/console-args.html | ||
| ```bash | ||
| dotnet run --project ./src/SIL.Harmony.Benchmarks/SIL.Harmony.Benchmarks.csproj -c Release -- --filter * | ||
| ``` |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix markdownlint MD034 and align run command with CI SDK.
- Replace bare URL with a markdown link.
- CI fails on net9.0; document running benchmarks with net8.0 to match the pipeline until SDK is upgraded.
-### Run benchmarks
-
-docs: https://benchmarkdotnet.org/articles/guides/console-args.html
-```bash
-dotnet run --project ./src/SIL.Harmony.Benchmarks/SIL.Harmony.Benchmarks.csproj -c Release -- --filter *
-```
+### Run benchmarks
+
+[Docs: BenchmarkDotNet console args](https://benchmarkdotnet.org/articles/guides/console-args.html)
+```bash
+dotnet run --project ./src/SIL.Harmony.Benchmarks/SIL.Harmony.Benchmarks.csproj -c Release -f net8.0 -- --filter *
+```🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
192-192: Bare URL used
(MD034, no-bare-urls)
🤖 Prompt for AI Agents
In README.md around lines 190 to 195, replace the bare URL with a proper
markdown link and update the example run command to target net8.0 to match CI;
specifically, change the docs line to a linked text ("Docs: BenchmarkDotNet
console args" linking to the provided URL) and modify the dotnet run command to
include the framework option (e.g., -f net8.0) so the command matches the CI SDK
until the pipeline is upgraded.
| public async Task AddSnapshots(IEnumerable<ObjectSnapshot> snapshots) | ||
| { | ||
| //save any pending commit changes | ||
| await _dbContext.SaveChangesAsync(); | ||
| var linqToDbTable = _dbContext.Set<ObjectSnapshot>().ToLinqToDBTable(); | ||
| var dataContext = linqToDbTable.DataContext; | ||
| foreach (var grouping in snapshots.GroupBy(s => s.EntityIsDeleted) | ||
| .OrderByDescending(g => g.Key)) //execute deletes first | ||
| { | ||
| var objectSnapshots = grouping.ToArray(); | ||
|
|
||
| //delete existing snapshots before we bulk recreate them | ||
| await _dbContext.Set<ObjectSnapshot>() | ||
| .Where(s => objectSnapshots.Select(s => s.Id).Contains(s.Id)) | ||
| .ExecuteDeleteAsync(); | ||
|
|
||
| await linqToDbTable.BulkCopyAsync(objectSnapshots); | ||
|
|
||
| //descending to insert the most recent snapshots first, only keep the last objects by ordering by descending | ||
| //don't want to change the objectSnapshot order to preserve the order of the changes | ||
| var snapshotsToProject = objectSnapshots.DefaultOrderDescending().DistinctBy(s => s.EntityId).Select(s => s.Id).ToHashSet(); | ||
| foreach (var objectSnapshot in objectSnapshots.IntersectBy(snapshotsToProject, s => s.Id)) | ||
| { | ||
| try | ||
| { | ||
| if (objectSnapshot.EntityIsDeleted) | ||
| { | ||
| await DeleteAsync(dataContext, objectSnapshot.Entity); | ||
| } | ||
| else | ||
| { | ||
| await InsertOrReplaceAsync(dataContext, objectSnapshot.Entity); | ||
| } | ||
| } | ||
| catch (Exception e) | ||
| { | ||
| throw new Exception("error when projecting snapshot " + objectSnapshot, e); | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
AddSnapshots lacks transaction/lock and diverges from base semantics.
- Not atomic: EF ExecuteDelete + Linq2Db BulkCopy + projections run outside a transaction; failures leave inconsistent state.
- No repository lock; concurrent calls can interleave.
- Projection selection differs from CrdtRepository (global “projected per entity” vs per-group), risking behavior drift.
- Minor: shadowed lambda variable in delete query harms readability.
Apply this refactor to align semantics and ensure atomicity:
- public async Task AddSnapshots(IEnumerable<ObjectSnapshot> snapshots)
+ public async Task AddSnapshots(IEnumerable<ObjectSnapshot> snapshots)
{
- //save any pending commit changes
- await _dbContext.SaveChangesAsync();
- var linqToDbTable = _dbContext.Set<ObjectSnapshot>().ToLinqToDBTable();
- var dataContext = linqToDbTable.DataContext;
- foreach (var grouping in snapshots.GroupBy(s => s.EntityIsDeleted)
- .OrderByDescending(g => g.Key)) //execute deletes first
- {
- var objectSnapshots = grouping.ToArray();
-
- //delete existing snapshots before we bulk recreate them
- await _dbContext.Set<ObjectSnapshot>()
- .Where(s => objectSnapshots.Select(s => s.Id).Contains(s.Id))
- .ExecuteDeleteAsync();
-
- await linqToDbTable.BulkCopyAsync(objectSnapshots);
-
- //descending to insert the most recent snapshots first, only keep the last objects by ordering by descending
- //don't want to change the objectSnapshot order to preserve the order of the changes
- var snapshotsToProject = objectSnapshots.DefaultOrderDescending().DistinctBy(s => s.EntityId).Select(s => s.Id).ToHashSet();
- foreach (var objectSnapshot in objectSnapshots.IntersectBy(snapshotsToProject, s => s.Id))
- {
- try
- {
- if (objectSnapshot.EntityIsDeleted)
- {
- await DeleteAsync(dataContext, objectSnapshot.Entity);
- }
- else
- {
- await InsertOrReplaceAsync(dataContext, objectSnapshot.Entity);
- }
- }
- catch (Exception e)
- {
- throw new Exception("error when projecting snapshot " + objectSnapshot, e);
- }
- }
- }
+ await using (await _original.Lock())
+ {
+ var beganTx = !_original.IsInTransaction;
+ IDbContextTransaction? tx = null;
+ if (beganTx) tx = await _original.BeginTransactionAsync();
+ try
+ {
+ // ensure commits are persisted before bulk ops
+ await _dbContext.SaveChangesAsync();
+
+ var table = _dbContext.Set<ObjectSnapshot>().ToLinqToDBTable();
+ var dc = table.DataContext;
+
+ // mirror base: process deletes first and project only once per EntityId across both groups
+ var projectedEntityIds = new HashSet<Guid>();
+ foreach (var grouping in snapshots.GroupBy(s => s.EntityIsDeleted).OrderByDescending(g => g.Key))
+ {
+ var objectSnapshots = grouping.ToArray();
+ var ids = objectSnapshots.Select(os => os.Id).ToArray();
+
+ // remove duplicates before reinsert
+ await _dbContext.Set<ObjectSnapshot>()
+ .Where(db => ids.Contains(db.Id))
+ .ExecuteDeleteAsync();
+
+ await table.BulkCopyAsync(objectSnapshots);
+
+ // project most recent per entity within this group, skipping entities already projected in prior groups
+ foreach (var snap in objectSnapshots.DefaultOrderDescending())
+ {
+ if (!projectedEntityIds.Add(snap.EntityId)) continue;
+ try
+ {
+ if (snap.EntityIsDeleted)
+ await DeleteAsync(dc, snap.Entity);
+ else
+ await InsertOrReplaceAsync(dc, snap.Entity);
+ }
+ catch (Exception e)
+ {
+ throw new InvalidOperationException($"Error projecting snapshot {snap.Id} ({snap.TypeName})", e);
+ }
+ }
+ }
+
+ if (beganTx && tx is not null) await tx.CommitAsync();
+ }
+ catch
+ {
+ if (beganTx && tx is not null) await tx.RollbackAsync();
+ throw;
+ }
+ finally
+ {
+ if (beganTx && tx is not null) await tx.DisposeAsync();
+ }
+ }
}Committable suggestion skipped: line range outside the PR's diff.
| public IAsyncEnumerable<SimpleSnapshot> CurrenSimpleSnapshots(bool includeDeleted = false) | ||
| { | ||
| return _original.CurrenSimpleSnapshots(includeDeleted); | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Align name with interface: CurrentSimpleSnapshots.
Fix method name to match the corrected interface.
- public IAsyncEnumerable<SimpleSnapshot> CurrenSimpleSnapshots(bool includeDeleted = false)
+ public IAsyncEnumerable<SimpleSnapshot> CurrentSimpleSnapshots(bool includeDeleted = false)
{
- return _original.CurrenSimpleSnapshots(includeDeleted);
+ return _original.CurrentSimpleSnapshots(includeDeleted);
}📝 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.
| public IAsyncEnumerable<SimpleSnapshot> CurrenSimpleSnapshots(bool includeDeleted = false) | |
| { | |
| return _original.CurrenSimpleSnapshots(includeDeleted); | |
| } | |
| public IAsyncEnumerable<SimpleSnapshot> CurrentSimpleSnapshots(bool includeDeleted = false) | |
| { | |
| return _original.CurrentSimpleSnapshots(includeDeleted); | |
| } |
🤖 Prompt for AI Agents
In src/SIL.Harmony.Linq2db/Linq2DbCrdtRepo.cs around lines 187 to 191, the
method is misspelled as CurrenSimpleSnapshots; rename it to
CurrentSimpleSnapshots to match the interface, update the method declaration and
any references (including the call to _original) to the corrected name, and
ensure the class still implements the interface signature (build and run tests
to catch remaining references).
| public bool SetParentHash(string parentHash) | ||
| { | ||
| if (ParentHash == parentHash) return false; | ||
| Hash = GenerateHash(parentHash); | ||
| ParentHash = parentHash; | ||
| return true; | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Normalize parentHash and guard inputs to avoid redundant updates and casing drift
Equality is currently case-sensitive and stores parentHash as-is. Hex casing differences will cause unnecessary hash recomputations and state churn. Add null/empty guards and normalize to uppercase.
- public bool SetParentHash(string parentHash)
+ public bool SetParentHash(string parentHash)
{
- if (ParentHash == parentHash) return false;
- Hash = GenerateHash(parentHash);
- ParentHash = parentHash;
- return true;
+ if (parentHash is null) throw new ArgumentNullException(nameof(parentHash));
+ if (parentHash.Length == 0) throw new ArgumentException("Parent hash cannot be empty.", nameof(parentHash));
+ var normalized = parentHash.ToUpperInvariant();
+ if (string.Equals(ParentHash, normalized, StringComparison.Ordinal)) return false;
+ Hash = GenerateHash(normalized);
+ ParentHash = normalized;
+ return true;
}Confirm no callers rely on case-variant parent hashes being treated as distinct:
🏁 Script executed:
#!/bin/bash
# Look for string comparisons or storage of ParentHash/Hash that may depend on casing.
rg -nP --type=cs -C2 '\b(ParentHash|Hash)\b'Length of output: 11587
Normalize and guard parentHash in SetParentHash
In src/SIL.Harmony/Commit.cs’s SetParentHash, throw on null/empty, convert input to upper-case (ToUpperInvariant), then compare against ParentHash to skip redundant updates and prevent casing drift—no callers rely on case-variant ParentHash values.
🤖 Prompt for AI Agents
In src/SIL.Harmony/Commit.cs around lines 23 to 29, SetParentHash must validate
and normalize the input: throw an ArgumentException when parentHash is null or
empty, convert parentHash to upper-case using ToUpperInvariant(), then compare
that normalized value against the existing ParentHash to skip redundant updates;
if different, call GenerateHash with the normalized parentHash, assign Hash and
set ParentHash to the normalized value, and return true (otherwise return
false).
| private readonly Func<DbContext, Guid, Task<ObjectSnapshot?>> GetCurrentSnapshotByObjectIdQuery = | ||
| EF.CompileAsyncQuery((DbContext dbContext, Guid objectId) => | ||
| dbContext.Set<ObjectSnapshot>() | ||
| .AsTracking(QueryTrackingBehavior.TrackAll) | ||
| .Include(s => s.Commit) | ||
| .OrderBy(c => c.Commit.HybridDateTime.DateTime) | ||
| .ThenBy(c => c.Commit.HybridDateTime.Counter) | ||
| .ThenBy(c => c.Commit.Id) | ||
| .LastOrDefault(s => s.EntityId == objectId)); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Compiled query can filter first and use FirstOrDefault with descending order.
Filtering before ordering reduces work; using FirstOrDefault on a descending order avoids LastOrDefault translation pitfalls.
- private readonly Func<DbContext, Guid, Task<ObjectSnapshot?>> GetCurrentSnapshotByObjectIdQuery =
- EF.CompileAsyncQuery((DbContext dbContext, Guid objectId) =>
- dbContext.Set<ObjectSnapshot>()
- .AsTracking(QueryTrackingBehavior.TrackAll)
- .Include(s => s.Commit)
- .OrderBy(c => c.Commit.HybridDateTime.DateTime)
- .ThenBy(c => c.Commit.HybridDateTime.Counter)
- .ThenBy(c => c.Commit.Id)
- .LastOrDefault(s => s.EntityId == objectId));
+ private readonly Func<DbContext, Guid, Task<ObjectSnapshot?>> GetCurrentSnapshotByObjectIdQuery =
+ EF.CompileAsyncQuery((DbContext dbContext, Guid objectId) =>
+ dbContext.Set<ObjectSnapshot>()
+ .AsTracking(QueryTrackingBehavior.TrackAll)
+ .Where(s => s.EntityId == objectId)
+ .Include(s => s.Commit)
+ .OrderByDescending(c => c.Commit.HybridDateTime.DateTime)
+ .ThenByDescending(c => c.Commit.HybridDateTime.Counter)
+ .ThenByDescending(c => c.Commit.Id)
+ .FirstOrDefault());📝 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.
| private readonly Func<DbContext, Guid, Task<ObjectSnapshot?>> GetCurrentSnapshotByObjectIdQuery = | |
| EF.CompileAsyncQuery((DbContext dbContext, Guid objectId) => | |
| dbContext.Set<ObjectSnapshot>() | |
| .AsTracking(QueryTrackingBehavior.TrackAll) | |
| .Include(s => s.Commit) | |
| .OrderBy(c => c.Commit.HybridDateTime.DateTime) | |
| .ThenBy(c => c.Commit.HybridDateTime.Counter) | |
| .ThenBy(c => c.Commit.Id) | |
| .LastOrDefault(s => s.EntityId == objectId)); | |
| private readonly Func<DbContext, Guid, Task<ObjectSnapshot?>> GetCurrentSnapshotByObjectIdQuery = | |
| EF.CompileAsyncQuery((DbContext dbContext, Guid objectId) => | |
| dbContext.Set<ObjectSnapshot>() | |
| - .AsTracking(QueryTrackingBehavior.TrackAll) | |
| - .Include(s => s.Commit) | |
| - .OrderBy(c => c.Commit.HybridDateTime.DateTime) | |
| - .ThenBy(c => c.Commit.HybridDateTime.Counter) | |
| - .ThenBy(c => c.Commit.Id) | |
| .AsTracking(QueryTrackingBehavior.TrackAll) | |
| .Where(s => s.EntityId == objectId) | |
| .Include(s => s.Commit) | |
| .OrderByDescending(c => c.Commit.HybridDateTime.DateTime) | |
| .ThenByDescending(c => c.Commit.HybridDateTime.Counter) | |
| .ThenByDescending(c => c.Commit.Id) | |
| .FirstOrDefault()); |
🤖 Prompt for AI Agents
In src/SIL.Harmony/Db/CrdtRepository.cs around lines 225 to 233, the compiled
query currently orders the whole set then applies LastOrDefault with the filter,
which is inefficient and can cause translation issues; change it to apply the
filter first (i.e., call .Where(s => s.EntityId == objectId) or move the
predicate into the initial query) then order by commit descending (swap the
OrderBy/ThenBy sequence to OrderByDescending on HybridDateTime.DateTime, then by
Counter descending, then by Commit.Id descending as needed) and use
FirstOrDefault instead of LastOrDefault so the database can execute the
filter+limit efficiently and avoid LastOrDefault translation pitfalls.
| Task DeleteSnapshotsAndProjectedTables(); | ||
| IQueryable<Commit> CurrentCommits(); | ||
| IQueryable<ObjectSnapshot> CurrentSnapshots(); | ||
| IAsyncEnumerable<SimpleSnapshot> CurrenSimpleSnapshots(bool includeDeleted = false); |
There was a problem hiding this comment.
Rename CurrenSimpleSnapshots → CurrentSimpleSnapshots.
Public API typo; fix before it ships.
- IAsyncEnumerable<SimpleSnapshot> CurrenSimpleSnapshots(bool includeDeleted = false);
+ IAsyncEnumerable<SimpleSnapshot> CurrentSimpleSnapshots(bool includeDeleted = false);📝 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.
| IAsyncEnumerable<SimpleSnapshot> CurrenSimpleSnapshots(bool includeDeleted = false); | |
| IAsyncEnumerable<SimpleSnapshot> CurrentSimpleSnapshots(bool includeDeleted = false); |
🤖 Prompt for AI Agents
In src/SIL.Harmony/Db/ICrdtRepository.cs around line 19, the interface method
name has a typo: rename CurrenSimpleSnapshots to CurrentSimpleSnapshots (keep
the signature IAsyncEnumerable<SimpleSnapshot> CurrentSimpleSnapshots(bool
includeDeleted = false)); then update all implementations, references/usages,
and unit tests to call the new name, update any XML docs or comments, and run a
build to ensure no remaining references to the old identifier remain.
| Task<SyncState> GetCurrentSyncState(); | ||
| Task<ChangesResult<Commit>> GetChanges(SyncState remoteState); | ||
| Task AddSnapshots(IEnumerable<ObjectSnapshot> snapshots); | ||
| CrdtRepository GetScopedRepository(Commit excludeChangesAfterCommit); |
There was a problem hiding this comment.
Return ICrdtRepository from GetScopedRepository to keep abstraction.
Returning a concrete type leaks implementation and breaks wrappers.
- CrdtRepository GetScopedRepository(Commit excludeChangesAfterCommit);
+ ICrdtRepository GetScopedRepository(Commit excludeChangesAfterCommit);🤖 Prompt for AI Agents
In src/SIL.Harmony/Db/ICrdtRepository.cs around line 32, the interface exposes a
concrete CrdtRepository return type which leaks implementation; change the
method signature to return ICrdtRepository instead, then update all implementing
classes and call sites to return/accept ICrdtRepository (adjust using/imports
and constructors as needed) so the abstraction is preserved and wrappers remain
compatible.
# Conflicts: # src/SIL.Harmony/Db/CrdtRepository.cs
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
src/SIL.Harmony/Db/ICrdtRepository.cs (1)
19-19: Rename CurrenSimpleSnapshots → CurrentSimpleSnapshots (public API typo).Same issue noted previously; fix before release and update all implementations/usages.
Apply:
- IAsyncEnumerable<SimpleSnapshot> CurrenSimpleSnapshots(bool includeDeleted = false); + IAsyncEnumerable<SimpleSnapshot> CurrentSimpleSnapshots(bool includeDeleted = false);src/SIL.Harmony/Db/CrdtRepository.cs (1)
225-234: Refactor compiled query: filter first, order descending, use FirstOrDefault.Repeating prior suggestion to avoid LastOrDefault translation pitfalls and extra work.
- private readonly Func<DbContext, Guid, Task<ObjectSnapshot?>> GetCurrentSnapshotByObjectIdQuery = - EF.CompileAsyncQuery((DbContext dbContext, Guid objectId) => - dbContext.Set<ObjectSnapshot>() - .AsTracking(QueryTrackingBehavior.TrackAll) - .Include(s => s.Commit) - .OrderBy(c => c.Commit.HybridDateTime.DateTime) - .ThenBy(c => c.Commit.HybridDateTime.Counter) - .ThenBy(c => c.Commit.Id) - .LastOrDefault(s => s.EntityId == objectId)); + private readonly Func<DbContext, Guid, Task<ObjectSnapshot?>> GetCurrentSnapshotByObjectIdQuery = + EF.CompileAsyncQuery((DbContext dbContext, Guid objectId) => + dbContext.Set<ObjectSnapshot>() + .AsTracking(QueryTrackingBehavior.TrackAll) + .Where(s => s.EntityId == objectId) + .Include(s => s.Commit) + .OrderByDescending(c => c.Commit.HybridDateTime.DateTime) + .ThenByDescending(c => c.Commit.HybridDateTime.Counter) + .ThenByDescending(c => c.Commit.Id) + .FirstOrDefault());src/SIL.Harmony.Linq2db/Linq2DbCrdtRepo.cs (2)
187-191: Align name with interface: CurrentSimpleSnapshots.Repeat of prior note; update here and in _original call.
- public IAsyncEnumerable<SimpleSnapshot> CurrenSimpleSnapshots(bool includeDeleted = false) + public IAsyncEnumerable<SimpleSnapshot> CurrentSimpleSnapshots(bool includeDeleted = false) { - return _original.CurrenSimpleSnapshots(includeDeleted); + return _original.CurrentSimpleSnapshots(includeDeleted); }
69-109: AddSnapshots lacks lock/transaction; risks inconsistent state under concurrency/failures.Wrap in _original.Lock() and a transaction (BeginTransactionAsync), mirroring base semantics.
- public async Task AddSnapshots(IEnumerable<ObjectSnapshot> snapshots) - { - //save any pending commit changes - await _dbContext.SaveChangesAsync(); - var linqToDbTable = _dbContext.Set<ObjectSnapshot>().ToLinqToDBTable(); - var dataContext = linqToDbTable.DataContext; - foreach (var grouping in snapshots.GroupBy(s => s.EntityIsDeleted) - .OrderByDescending(g => g.Key)) //execute deletes first - { - var objectSnapshots = grouping.ToArray(); - - //delete existing snapshots before we bulk recreate them - await _dbContext.Set<ObjectSnapshot>() - .Where(s => objectSnapshots.Select(s => s.Id).Contains(s.Id)) - .ExecuteDeleteAsync(); - - await linqToDbTable.BulkCopyAsync(objectSnapshots); - - //descending to insert the most recent snapshots first, only keep the last objects by ordering by descending - //don't want to change the objectSnapshot order to preserve the order of the changes - var snapshotsToProject = objectSnapshots.DefaultOrderDescending().DistinctBy(s => s.EntityId).Select(s => s.Id).ToHashSet(); - foreach (var objectSnapshot in objectSnapshots.IntersectBy(snapshotsToProject, s => s.Id)) - { - try - { - if (objectSnapshot.EntityIsDeleted) - { - await DeleteAsync(dataContext, objectSnapshot.Entity); - } - else - { - await InsertOrReplaceAsync(dataContext, objectSnapshot.Entity); - } - } - catch (Exception e) - { - throw new Exception("error when projecting snapshot " + objectSnapshot, e); - } - } - } - } + public async Task AddSnapshots(IEnumerable<ObjectSnapshot> snapshots) + { + await using (await _original.Lock()) + { + var beganTx = !_original.IsInTransaction; + IDbContextTransaction? tx = null; + if (beganTx) tx = await _original.BeginTransactionAsync(); + try + { + await _dbContext.SaveChangesAsync(); + var table = _dbContext.Set<ObjectSnapshot>().ToLinqToDBTable(); + var dc = table.DataContext; + + var projectedEntityIds = new HashSet<Guid>(); + foreach (var grouping in snapshots.GroupBy(s => s.EntityIsDeleted).OrderByDescending(g => g.Key)) + { + var objectSnapshots = grouping.ToArray(); + var ids = objectSnapshots.Select(os => os.Id).ToArray(); + await _dbContext.Set<ObjectSnapshot>().Where(db => ids.Contains(db.Id)).ExecuteDeleteAsync(); + await table.BulkCopyAsync(objectSnapshots); + + foreach (var snap in objectSnapshots.DefaultOrderDescending()) + { + if (!projectedEntityIds.Add(snap.EntityId)) continue; + try + { + if (snap.EntityIsDeleted) await DeleteAsync(dc, snap.Entity); + else await InsertOrReplaceAsync(dc, snap.Entity); + } + catch (Exception e) + { + throw new InvalidOperationException($"Error projecting snapshot {snap.Id} ({snap.TypeName})", e); + } + } + } + + if (beganTx && tx is not null) await tx.CommitAsync(); + } + catch + { + if (beganTx && tx is not null) await tx.RollbackAsync(); + throw; + } + finally + { + if (beganTx && tx is not null) await tx.DisposeAsync(); + } + } + }
🧹 Nitpick comments (6)
src/SIL.Harmony/Db/CrdtRepositoryFactory.cs (2)
10-27: Consider reducing Execute overload ambiguity.Having both Task and ValueTask overloads named Execute can confuse type inference; consider dropping the ValueTask overload or renaming it (e.g., ExecuteValueTask).
41-44: Public CreateInstance exposes construction details across assemblies.If this must remain public for Linq2Db, add XML docs clarifying intended use. Otherwise, consider making it internal and using InternalsVisibleTo for SIL.Harmony.Linq2db.
src/SIL.Harmony.Linq2db/Linq2DbCrdtRepo.cs (4)
15-19: Concrete CrdtRepositoryFactory dependency leaks implementation.Prefer depending on an abstraction or extend ICrdtRepositoryFactory with CreateInstance(ICrdtDbContext). If keeping concrete, document why (shared DbContext/transaction).
81-83: Avoid lambda shadowing and re-selection; precompute IDs for clarity.- await _dbContext.Set<ObjectSnapshot>() - .Where(s => objectSnapshots.Select(s => s.Id).Contains(s.Id)) - .ExecuteDeleteAsync(); + var ids = objectSnapshots.Select(os => os.Id).ToArray(); + await _dbContext.Set<ObjectSnapshot>() + .Where(db => ids.Contains(db.Id)) + .ExecuteDeleteAsync();
103-106: Throw a specific exception type and include identifiers.- throw new Exception("error when projecting snapshot " + objectSnapshot, e); + throw new InvalidOperationException($"Error projecting snapshot {objectSnapshot.Id} ({objectSnapshot.TypeName})", e);
111-133: Cache constructed generic methods per entity type to cut reflection overhead.Use a ConcurrentDictionary<Type, MethodInfo> for InsertOrReplaceAsync/DeleteAsync generics.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (6)
src/SIL.Harmony.Benchmarks/SIL.Harmony.Benchmarks.csproj(1 hunks)src/SIL.Harmony.Linq2db/Linq2DbCrdtRepo.cs(1 hunks)src/SIL.Harmony/Db/CrdtRepository.cs(5 hunks)src/SIL.Harmony/Db/CrdtRepositoryFactory.cs(1 hunks)src/SIL.Harmony/Db/ICrdtRepository.cs(1 hunks)src/SIL.Harmony/ResourceService.cs(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/SIL.Harmony.Benchmarks/SIL.Harmony.Benchmarks.csproj
🧰 Additional context used
🧬 Code graph analysis (5)
src/SIL.Harmony/ResourceService.cs (3)
src/SIL.Harmony/DataModel.cs (2)
DataModel(14-371)DataModel(28-39)src/SIL.Harmony/Db/CrdtRepositoryFactory.cs (3)
ICrdtRepository(8-8)ICrdtRepository(36-39)ICrdtRepository(41-44)src/SIL.Harmony/Db/ICrdtRepository.cs (1)
ICrdtRepository(32-32)
src/SIL.Harmony/Db/CrdtRepositoryFactory.cs (2)
src/SIL.Harmony/Db/CrdtRepository.cs (24)
ICrdtRepository(345-348)Task(68-71)Task(76-79)Task(81-94)Task(96-105)Task(107-117)Task(121-124)Task(179-191)Task(193-196)Task(198-205)Task(207-215)Task(217-223)Task(235-243)Task(245-253)Task(255-259)Task(270-273)Task(275-278)ValueTask(308-336)ValueTask(338-343)ValueTask(420-423)ValueTask(437-440)ValueTask(480-483)CrdtRepository(15-424)CrdtRepository(24-35)src/SIL.Harmony/Db/ICrdtRepository.cs (1)
ICrdtRepository(32-32)
src/SIL.Harmony/Db/CrdtRepository.cs (3)
src/SIL.Harmony.Linq2db/Linq2DbCrdtRepo.cs (4)
ICrdtRepository(25-28)ICrdtRepository(248-252)ClearChangeTracker(64-67)HybridDateTime(254-257)src/SIL.Harmony/Db/CrdtRepositoryFactory.cs (3)
ICrdtRepository(8-8)ICrdtRepository(36-39)ICrdtRepository(41-44)src/SIL.Harmony/Db/ICrdtRepository.cs (3)
ICrdtRepository(32-32)ClearChangeTracker(10-10)HybridDateTime(36-36)
src/SIL.Harmony/Db/ICrdtRepository.cs (6)
src/SIL.Harmony.Linq2db/Linq2DbCrdtRepo.cs (3)
ICrdtRepository(25-28)ICrdtRepository(248-252)ClearChangeTracker(64-67)src/SIL.Harmony/Db/CrdtRepository.cs (2)
ICrdtRepository(345-348)ClearChangeTracker(59-62)src/SIL.Harmony/Db/CrdtRepositoryFactory.cs (3)
ICrdtRepository(8-8)ICrdtRepository(36-39)ICrdtRepository(41-44)src/SIL.Harmony/Commit.cs (3)
Commit(7-43)Commit(17-21)Commit(30-33)src/SIL.Harmony/Db/ObjectSnapshot.cs (1)
SimpleSnapshot(20-29)src/SIL.Harmony/Resource/LocalResource.cs (1)
LocalResource(6-16)
src/SIL.Harmony.Linq2db/Linq2DbCrdtRepo.cs (3)
src/SIL.Harmony/Db/CrdtRepositoryFactory.cs (5)
CrdtRepositoryFactory(29-45)ICrdtRepository(8-8)ICrdtRepository(36-39)ICrdtRepository(41-44)ValueTask(22-26)src/SIL.Harmony/Db/CrdtRepository.cs (33)
Task(68-71)Task(76-79)Task(81-94)Task(96-105)Task(107-117)Task(121-124)Task(179-191)Task(193-196)Task(198-205)Task(207-215)Task(217-223)Task(235-243)Task(245-253)Task(255-259)Task(270-273)Task(275-278)ICrdtRepository(345-348)ValueTask(308-336)ValueTask(338-343)ValueTask(420-423)ValueTask(437-440)ValueTask(480-483)AwaitableDisposable(37-40)ClearChangeTracker(59-62)IQueryable(126-129)IQueryable(131-153)IQueryable(156-159)IQueryable(261-268)IQueryable(405-408)IAsyncEnumerable(161-177)IAsyncEnumerable(393-396)IAsyncEnumerable(397-400)HybridDateTime(372-379)src/SIL.Harmony/Db/ICrdtRepository.cs (11)
ICrdtRepository(32-32)AwaitableDisposable(9-9)ClearChangeTracker(10-10)IQueryable(17-17)IQueryable(18-18)IQueryable(28-28)IQueryable(44-44)IAsyncEnumerable(19-19)IAsyncEnumerable(38-38)IAsyncEnumerable(39-39)HybridDateTime(36-36)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: build
- GitHub Check: build
- GitHub Check: Analyze (csharp)
🔇 Additional comments (13)
src/SIL.Harmony/ResourceService.cs (3)
13-13: Good switch to interface-based factory.Decouples from concrete type and aligns with the new DI pattern.
18-18: Constructor update to ICrdtRepositoryFactory looks correct.No public API change; internal wiring only.
167-170: Accepting ICrdtRepository in DownloadResourceInternal is the right abstraction.Keeps the method independent of EF-specific repo.
src/SIL.Harmony/Db/ICrdtRepository.cs (3)
1-4: Required usings for async locking and EF transactions are appropriate.
7-13: Interface shape and scoped repo return type look good.Returning ICrdtRepository from GetScopedRepository preserves abstraction.
Also applies to: 31-33
47-47: DeleteLocalResource addition is sensible.Matches new ResourceService flows.
src/SIL.Harmony/Db/CrdtRepositoryFactory.cs (1)
5-9: Factory interface is clear and minimal.src/SIL.Harmony/Db/CrdtRepository.cs (5)
15-15: Implementing ICrdtRepository is correct and matches the DI changes.
59-62: ClearChangeTracker exposure is fine.Public surface matches interface.
237-237: Using compiled query only for tracking path is fine.Keeps non-tracking simpler.
345-348: Scoped repository now returns ICrdtRepository — good.
362-370: UpdateCommitHash via ExecuteUpdateAsync is efficient.Atomic, server-side update of both fields.
src/SIL.Harmony.Linq2db/Linq2DbCrdtRepo.cs (1)
248-252: Good: return wrapped scoped repository to preserve Linq2Db behavior.
hahn-kev
changed the title
Results from running locally, the baseline is
UsingLinq2DbRepo falseNote
forcing all the tests to run against the new linq2db repo they still pass, however I've left it off by default.
Summary by CodeRabbit