-
Couldn't load subscription status.
- Fork 1
Expose GetLatestSnapshots on DataModel #53
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
WalkthroughA new public async method Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~5 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/SIL.Harmony/DataModel.cs (1)
254-261: Consider adding.AsNoTracking()for performance, and evaluate whether this should be public.Since this method is exposed solely for testing (per PR description), consider making it
internaland using[assembly: InternalsVisibleTo("TestAssemblyName")]to avoid polluting the public API surface. Additionally, the method should likely use.AsNoTracking()to prevent EF from tracking potentially large numbers of entities, which improves performance and reduces memory overhead. Other read-only query methods in this class use this pattern (e.g., line 221).Apply this diff to add
.AsNoTracking():public async IAsyncEnumerable<ObjectSnapshot> GetLatestSnapshots() { await using var repo = await _crdtRepositoryFactory.CreateRepository(); - await foreach (var snapshot in repo.CurrentSnapshots().AsAsyncEnumerable()) + await foreach (var snapshot in repo.CurrentSnapshots().AsNoTracking().AsAsyncEnumerable()) { yield return snapshot; } }Optionally, consider adding an
includeDeletedparameter for consistency withGetProjectSnapshot(line 291) if your test scenarios require it.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/SIL.Harmony/DataModel.cs(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/SIL.Harmony/DataModel.cs (3)
src/SIL.Harmony/Db/CrdtRepository.cs (3)
IAsyncEnumerable(192-208)IAsyncEnumerable(403-406)IAsyncEnumerable(407-410)src/SIL.Harmony/SnapshotWorker.cs (2)
IAsyncEnumerable(166-169)IAsyncEnumerable(171-197)src/SIL.Harmony/Db/ObjectSnapshot.cs (2)
ObjectSnapshot(32-80)ObjectSnapshot(34-48)
⏰ 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)
I exposed this for a test:
https://github.com/sillsdev/languageforge-lexbox/blob/19cc6898cdaab426d9ac11947706bbff4ce9bc2a/backend/FwLite/LcmCrdt.Tests/Data/MigrationTests.cs#L128
Summary by CodeRabbit