Fix dotnet warnings and suggestions #1951
Conversation
📝 WalkthroughWalkthroughPublic create methods in FwDataMiniLcmApi were made non-async and refactored to use Task.FromResult; collections use spread syntax; translation/multistring nulls map to empty arrays. Minor refactors in progress utilities, MAUI warning suppression, import API signature default removal, project services constructor/logging cleanup, controller catch tweak, and test async write. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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 (
|
github-actions
bot
added
💻 FW Lite
|
The latest updates on your projects. Learn more about Argos notifications ↗︎
|
C# Unit Tests130 tests 130 ✅ 19s ⏱️ Results for commit 726d8ff. ♻️ This comment has been updated with latest results. |
UI unit Tests 1 files 40 suites 21s ⏱️ Results for commit 726d8ff. ♻️ This comment has been updated with latest results. |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/FwLite/FwLiteShared/Services/ProjectServicesProvider.cs (1)
153-164: Dispose underlying JS-invokables before disposing their DotNetObjectReferencesYou now null-guard the disposals, which is good. However, disposing DotNetObjectReference alone doesn't dispose the wrapped object. MiniLcm is correctly disposed via
.Value.Dispose()before.Dispose(). Do the same for HistoryService and SyncService to avoid leaks.logger.LogInformation("Disposing project scope {ProjectName}", projectName); projectServicesProvider._projectScopes.TryRemove(this, out _); - HistoryService?.Dispose(); - SyncService?.Dispose(); + if (HistoryService?.Value is IAsyncDisposable hsAsync) await hsAsync.DisposeAsync(); + else (HistoryService?.Value as IDisposable)?.Dispose(); + HistoryService?.Dispose(); + + if (SyncService?.Value is IAsyncDisposable ssAsync) await ssAsync.DisposeAsync(); + else (SyncService?.Value as IDisposable)?.Dispose(); + SyncService?.Dispose(); MiniLcm.Value.Dispose(); MiniLcm.Dispose();
🧹 Nitpick comments (7)
backend/Testing/FwHeadless/MediaFileServiceTests.cs (1)
235-243: Avoid size-check flakiness from StreamWriter buffering; write bytes directly (or flush) while filling the MemoryStreamUsing StreamWriter to grow MemoryStream means memoryStream.Length may not reflect recent writes until the writer flushes, which can make the loop run longer than intended. Your switch to WriteAsync is fine, but the buffering caveat remains. Prefer writing bytes directly to the MemoryStream (or force flushing) to make the size check precise and the test faster/deterministic.
Option A (minimal): enable AutoFlush so Length is updated on each write.
- using (var stream = new StreamWriter(memoryStream, leaveOpen: true)) + using (var stream = new StreamWriter(memoryStream, leaveOpen: true)) { + stream.AutoFlush = true; while (memoryStream.Length < _fwHeadlessConfig.MaxUploadFileSizeBytes) { await stream.WriteAsync(Guid.NewGuid().ToString("N")); } }Option B (preferred): remove StreamWriter and write random bytes directly.
- var memoryStream = new MemoryStream(); - using (var stream = new StreamWriter(memoryStream, leaveOpen: true)) - { - while (memoryStream.Length < _fwHeadlessConfig.MaxUploadFileSizeBytes) - { - await stream.WriteAsync(Guid.NewGuid().ToString("N")); - } - } + var memoryStream = new MemoryStream(); + var buffer = new byte[8192]; + var rng = System.Security.Cryptography.RandomNumberGenerator.Create(); + while (memoryStream.Length <= _fwHeadlessConfig.MaxUploadFileSizeBytes) + { + rng.GetBytes(buffer); + var remaining = (int)Math.Min(buffer.Length, + _fwHeadlessConfig.MaxUploadFileSizeBytes - memoryStream.Length + 1); + await memoryStream.WriteAsync(buffer.AsMemory(0, remaining)); + }backend/FwLite/FwLiteMaui/MainPage.xaml.Windows.cs (1)
30-41: LGTM: suppression expanded to CS4014 to document intentional fire-and-forgetThis matches the established pattern for SetPermissionStateAsync being intentionally not awaited. Consider optionally logging faults to avoid completely swallowing exceptions in release:
foreach (var permission in permissions) { - e.WebView.CoreWebView2.Profile.SetPermissionStateAsync(permission, + _ = e.WebView.CoreWebView2.Profile.SetPermissionStateAsync(permission, "https://0.0.0.1", - CoreWebView2PermissionState.Allow); - e.WebView.CoreWebView2.Profile.SetPermissionStateAsync(permission, + CoreWebView2PermissionState.Allow).ContinueWith(t => System.Diagnostics.Debug.WriteLine(t.Exception), TaskContinuationOptions.OnlyOnFaulted); + _ = e.WebView.CoreWebView2.Profile.SetPermissionStateAsync(permission, "https://0.0.0.0", - CoreWebView2PermissionState.Allow); + CoreWebView2PermissionState.Allow).ContinueWith(t => System.Diagnostics.Debug.WriteLine(t.Exception), TaskContinuationOptions.OnlyOnFaulted); }backend/FwLite/FwLiteProjectSync/Import/ResumableImportApi.cs (1)
69-72: Explicit CreateWritingSystem default-parameter removal is safeI verified that all callers invoke this method via the interface (which still declares
between = null), and there are no direct references to the explicit implementation omitting the second argument.Optional: to avoid any chance of cache-key collision, you could include a delimiter between the type and code. For example, in
backend/FwLite/FwLiteProjectSync/Import/ResumableImportApi.cs:71change:- ws => ws.Type + ws.WsId.Code + ws => $"{ws.Type}:{ws.WsId.Code}"backend/FwLite/FwDataMiniLcmBridge/LcmUtils/LcmThreadedProgress.cs (1)
42-45: Canceled/IsCanceling hardcoded to false — consider documenting or wiring a tokenIf cancellation is intentionally unsupported in this implementation, consider an XML doc comment stating that this progress source never cancels. If limited cancellation becomes desirable later, you could back these by a CancellationTokenSource.
backend/FwLite/FwDataMiniLcmBridge/Api/FwDataMiniLcmApi.cs (3)
297-312: CreatePartOfSpeech made sync (Task.FromResult) — LGTMThis removes unnecessary async state machines. Implementation correctly ensures an ID and applies the name.
If you expect these synchronous wrappers to be hot, consider returning
ValueTask<T>to avoid Task allocation; otherwise currentTask.FromResultis fine for consistency.
359-368: FromLcmPossibility can be expression-bodied (minor)Tiny readability win; no behavior change.
-private Publication FromLcmPossibility(ICmPossibility lcmPossibility) -{ - var possibility = new Publication - { - Id = lcmPossibility.Guid, - Name = FromLcmMultiString(lcmPossibility.Name) - }; - - return possibility; -} +private Publication FromLcmPossibility(ICmPossibility lcmPossibility) => + new Publication + { + Id = lcmPossibility.Guid, + Name = FromLcmMultiString(lcmPossibility.Name) + };
346-357: Remove redundant async/await in both CreatePublication methodsBoth implementations of
CreatePublicationuse anasyncmodifier only toawait Task.FromResult(...). You can simplify these by droppingasync/awaitand returning theTaskdirectly.Locations:
backend/FwLite/FwDataMiniLcmBridge/Api/FwDataMiniLcmApi.cs(lines 346–357)backend/FwLite/FwLiteProjectSync/DryRunMiniLcmApi.cs(around line 304)Suggested changes:
--- a/backend/FwLite/FwDataMiniLcmBridge/Api/FwDataMiniLcmApi.cs +++ b/backend/FwLite/FwDataMiniLcmBridge/Api/FwDataMiniLcmApi.cs @@ - public async Task<Publication> CreatePublication(Publication pub) + public Task<Publication> CreatePublication(Publication pub) { ICmPossibility? lcmPublication = null; NonUndoableUnitOfWorkHelper.DoUsingNewOrCurrentUOW(Cache.ServiceLocator.ActionHandler, () => { lcmPublication = Cache.ServiceLocator.GetInstance<ICmPossibilityFactory>() .Create(pub.Id, Cache.LangProject.LexDbOA.PublicationTypesOA); UpdateLcmMultiString(lcmPublication.Name, pub.Name); }); - return await Task.FromResult(FromLcmPossibility( - lcmPublication ?? throw new InvalidOperationException("Failed to create publication"))); + return Task.FromResult(FromLcmPossibility( + lcmPublication ?? throw new InvalidOperationException("Failed to create publication"))); }--- a/backend/FwLite/FwLiteProjectSync/DryRunMiniLcmApi.cs +++ b/backend/FwLite/FwLiteProjectSync/DryRunMiniLcmApi.cs @@ - public async Task<Publication> CreatePublication(Publication pub) + public Task<Publication> CreatePublication(Publication pub) { DryRunRecords.Add(new DryRunRecord(nameof(CreatePublication), $"Create publication {pub.Id}")); - return await Task.FromResult(pub); + return Task.FromResult(pub); }By applying these changes, you eliminate unnecessary state-machine overhead and align with the pattern used elsewhere.
📜 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 (7)
backend/FwLite/FwDataMiniLcmBridge/Api/FwDataMiniLcmApi.cs(17 hunks)backend/FwLite/FwDataMiniLcmBridge/LcmUtils/LcmThreadedProgress.cs(3 hunks)backend/FwLite/FwLiteMaui/MainPage.xaml.Windows.cs(1 hunks)backend/FwLite/FwLiteProjectSync/Import/ResumableImportApi.cs(1 hunks)backend/FwLite/FwLiteShared/Services/ProjectServicesProvider.cs(2 hunks)backend/LexBoxApi/Controllers/SyncController.cs(1 hunks)backend/Testing/FwHeadless/MediaFileServiceTests.cs(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: hahn-kev
PR: sillsdev/languageforge-lexbox#1760
File: backend/FwLite/LcmCrdt/CrdtMiniLcmApi.cs:274-277
Timestamp: 2025-06-27T09:24:39.507Z
Learning: In the CrdtMiniLcmApi class, the user prefers to keep the current AddChanges method signature (IEnumerable<IChange>) rather than modifying it to support IAsyncEnumerable for streaming, even when it means materializing collections into memory for bulk operations.
📚 Learning: 2025-08-07T08:35:47.803Z
Learnt from: hahn-kev
PR: sillsdev/languageforge-lexbox#1885
File: backend/FwLite/FwLiteMaui/MainPage.xaml.Windows.cs:30-41
Timestamp: 2025-08-07T08:35:47.803Z
Learning: In backend/FwLite/FwLiteMaui/MainPage.xaml.Windows.cs, the SetPermissionStateAsync calls in BlazorWebViewInitialized method are intentionally used in a fire-and-forget pattern with pragma warning suppression for VSTHRD110, as awaiting them is not necessary and async void should be avoided.
Applied to files:
backend/FwLite/FwLiteMaui/MainPage.xaml.Windows.cs
📚 Learning: 2025-07-22T09:19:27.281Z
Learnt from: rmunn
PR: sillsdev/languageforge-lexbox#1836
File: backend/FwLite/LcmCrdt/MediaServer/LcmMediaService.cs:120-140
Timestamp: 2025-07-22T09:19:27.281Z
Learning: In the LcmMediaService class (backend/FwLite/LcmCrdt/MediaServer/LcmMediaService.cs), File.OpenWrite should not be used for saving files because it doesn't truncate existing files, which causes data corruption when re-uploading files smaller than the original. Use File.Create instead to ensure proper file truncation.
Applied to files:
backend/Testing/FwHeadless/MediaFileServiceTests.cs
🧬 Code graph analysis (2)
backend/FwLite/FwLiteProjectSync/Import/ResumableImportApi.cs (2)
backend/FwLite/FwLiteProjectSync.Tests/Import/ResumableTests.cs (11)
Task(139-142)Task(144-147)Task(155-159)Task(161-165)Task(167-174)Task(176-180)Task(181-185)Task(186-190)Task(192-199)Task(201-205)Task(206-210)backend/FwLite/FwLiteProjectSync/DryRunMiniLcmApi.cs (5)
Task(21-26)Task(28-41)Task(43-47)Task(49-53)Task(55-59)
backend/FwLite/FwLiteShared/Services/ProjectServicesProvider.cs (2)
backend/FwLite/MiniLcm/Project/IProjectProvider.cs (1)
IEnumerable(8-8)backend/FwLite/FwLiteShared/Services/MiniLcmJsInvokable.cs (1)
Dispose(374-378)
🔇 Additional comments (13)
backend/LexBoxApi/Controllers/SyncController.cs (1)
49-52: LGTM: remove unused exception variable to silence warning without changing behaviorCatching OperationCanceledException without a named variable cleans up the compiler warning and preserves the existing 200 OK Timeout response.
backend/FwLite/FwLiteShared/Services/ProjectServicesProvider.cs (1)
20-21: Constructor signature change verified – no manual instantiations or custom factories detectedAll references to
ProjectServicesProviderwere inspected:
- DI registration exists in
• backend/FwLite/FwLiteShared/FwLiteSharedKernel.cs (line 31):
services.AddScoped<ProjectServicesProvider>();- Type mapping in
• backend/FwLite/FwLiteShared/Services/FwLiteProvider.cs (lines 46, 103):
DotnetService.ProjectServicesProvider => typeof(ProjectServicesProvider)- No occurrences of
new ProjectServicesProvider(...)orActivatorUtilities.CreateInstance/GetService<…ProjectServicesProvider>were found.Since the logger parameter was dropped from the constructor and DI only ever resolves the concrete type, everything will bind to the updated signature without further changes.
backend/FwLite/FwDataMiniLcmBridge/LcmUtils/LcmThreadedProgress.cs (3)
8-8: Readonly synchronize-invoke field — LGTMMarking
_synchronizeInvokeasreadonlyis correct and prevents accidental reassignment.
25-25: Expression-bodied SynchronizeInvoke property — LGTMThe expression-bodied member improves clarity with no functional change.
10-12: Confirmed:Cancelingevent is unused, CS0067 suppression is safeI searched the entire repository for any subscriptions (
.Canceling +=) or invocations (Canceling() of theCancelingevent and found no references. Since no code subscribes to or raises this event, the warning suppression can remain without risk of hiding behavioral bugs.backend/FwLite/FwDataMiniLcmBridge/Api/FwDataMiniLcmApi.cs (8)
143-145: Property pattern on LexemeForm — LGTMUsing
entry.LexemeFormOA is { Form: not null }is a concise, null-safe guard.
501-516: CreateComplexFormType moved to sync Task.FromResult — LGTMImplementation correctly adds the created type and returns the projected DTO.
773-783: Sense mapping and collection expressions — LGTMCollection expressions for
SemanticDomainsandExampleSentencesare clear and allocation-friendly.Also applies to: 781-783
1176-1178: Ensure a ComplexForm entryRef exists before adding types — LGTMThis fixes a latent null/empty case by creating a
LexEntryRefwhen none exist. Matches LCM behavior comment.
1427-1437: CreateSense converted to sync Task.FromResult — LGTMThe flow creates and inserts the sense inside a UOW and returns the mapped DTO. Consistent with other Create* refactors.
1572-1583: CreateExampleSentence converted to sync Task.FromResult — LGTMSame pattern as
CreateSense; looks good.
1672-1674: I’ve requested inspection of thePathFromMediaUriimplementations and associated tests to confirm whether they ever return absolute paths outside the expected root. Once we have that, we can determine if the suggested hardening is needed.
662-665: Verified: Collection expressions align with List properties in API proxiesI’ve confirmed that in
UpdateEntryProxy.cs, the overridden properties are all declared asList<T>:
public override List<Sense> Sensespublic override List<ComplexFormComponent> Componentspublic override List<ComplexFormEntry> ComplexFormspublic override List<ComplexFormType> ComplexFormTypesSince collection expressions (
[.. entry.AllSenses.Select(...)], etc.) targetList<T>, they will compile intonew List<T> { … }and match the intended API shape. No further changes needed here.
hahn-kev
removed
📦 Lexbox
2 participants
Inspired by:
