refactor(async): drop fake-async Task.Run wrappers - #264
Open
rlorenzo wants to merge 1 commit into
Open
Conversation
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
Bundle ReportBundle size has no change ✅ |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #264 +/- ##
=======================================
Coverage 50.07% 50.07%
=======================================
Files 998 998
Lines 58354 58353 -1
Branches 5853 5854 +1
=======================================
Hits 29221 29221
+ Misses 28228 28227 -1
Partials 905 905
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. |
There was a problem hiding this comment.
Pull request overview
Refactors ASP.NET MVC actions and shared view components to remove “fake async” await Task.Run(...) wrappers around synchronous work, keeping true async where there are real awaits and documenting the remaining legitimate offloads.
Changes:
- Converted trivial async controller actions and view components to synchronous signatures and removed
Task.Runwrappers forView(...)/Redirect(...). - Kept and documented the two intentional
Task.Runusages (blocking LDAP bind; potentially-blocking network shareFile.Exists). - Updated/added tests to cover new sync action behavior and some related correctness fixes (e.g., background OU group sync scope, bad-request guards).
Reviewed changes
Copilot reviewed 23 out of 23 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| web/Views/Shared/Components/VueTableDefault/VueTableDefault.cs | Make view component synchronous; remove Task.Run around View. |
| web/Views/Shared/Components/VueCdn/VueCdnInit.cs | Make view component synchronous; remove Task.Run. |
| web/Views/Shared/Components/VueCdn/VueCdnCreate.cs | Make view component synchronous; remove Task.Run. |
| web/Views/Shared/Components/SessionTimeout/SessionTimeout.cs | Make view component synchronous; remove Task.Run. |
| web/Views/Shared/Components/ProfilePic/ProfilePic.cs | Make view component synchronous; remove Task.Run. |
| web/Views/Shared/Components/MiniNav/MiniNav.cs | Make view component synchronous; remove Task.Run. |
| web/Views/Shared/Components/MainNav/MainNav.cs | Make view component synchronous; remove Task.Run. |
| web/Views/Shared/Components/LeftNav/LeftNav.cs | Make view component synchronous; remove Task.Run. |
| web/Views/Shared/Components/EmulationBanner/EmulationBanner.cs | Make view component synchronous; remove Task.Run/use direct Content/View. |
| web/Views/Shared/Components/CMSBlocks/CMSBlocks.cs | Make view component synchronous; remove Task.Run. |
| web/Classes/HealthChecks/LdapHealthCheck.cs | Add rationale comment for intentional Task.Run offload. |
| web/Areas/Students/Services/PhotoService.cs | Add rationale comment for intentional Task.Run offload. |
| web/Areas/RAPS/Models/GroupAddEdit.cs | Make GroupId nullable for “new group” semantics. |
| web/Areas/RAPS/Controllers/RAPSController.cs | Remove fake-async wrappers; add ModelState guards; fix background group sync scoping/logging; minor query improvements. |
| web/Areas/RAPS/Controllers/AdGroupsController.cs | Fix missing return on mismatched ID bad-request path. |
| web/Areas/Directory/Controllers/DirectoryController.cs | Remove fake-async wrappers; add shared AAUD search helper; make VMACS enrichment properly async. |
| web/Areas/CTS/Controllers/CTSController.cs | Add targeted analyzer suppression for filter override. |
| web/Areas/CMS/Controllers/CMSController.cs | Switch to creating the Data.CMS logger from ILoggerFactory. |
| test/RAPS/RAPSControllerTests.cs | Add coverage for ModelState bad-request guards + sync RolePermissions behavior. |
| test/RAPS/AdGroupsControllerTests.cs | Add regression test for mismatched group ID not updating entity. |
| test/Directory/DirectoryControllerTests.cs | Add SQLite-translated query tests for shared directory search helper. |
| test/ClinicalScheduler/Integration/ServiceLayerIntegrationTest.cs | Refactor test constructor to helper factories (maintainability). |
| .editorconfig | Add/extend analyzer suppressions for generated contexts and ApiPagination. |
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
Contributor
📝 WalkthroughWalkthroughChangesDirectory search
RAPS controller behavior
Synchronous execution cleanup
Estimated code review effort: 4 (Complex) | ~45 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 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 `@test/RAPS/RAPSControllerTests.cs`:
- Around line 177-190: Add a success-path test alongside
GroupSync_RendersWithoutSyncing_WhenGroupMissing that seeds an OuGroup, invokes
GroupSync with its ID, and asserts scopeFactory.Received(1).CreateScope() and/or
populated ViewData["Group"]. Keep the assertion focused on synchronous dispatch;
do not await the fire-and-forget background task.
In `@web/Areas/Directory/Controllers/DirectoryController.cs`:
- Around line 64-73: Refactor the result-building loops in Get and GetUCD so
LookupEmailHost(_aaud) remains sequential, while
AddVmacsContactInfoAsync(result) calls are collected and awaited together with
Task.WhenAll after each loop. Preserve result ordering and ensure every
constructed result receives VMACS enrichment before the action returns.
- Around line 57-75: Update the Get method’s per-result LDAP lookup so the
blocking LdapService.GetUserByID call runs on a pool thread, or offload the
complete per-result metadata work without blocking the async continuation.
Preserve result construction, LookupEmailHost, AddVmacsContactInfoAsync, and
ordering while ensuring each LdapConnection operation does not execute directly
on the request thread.
In `@web/Areas/RAPS/Controllers/RAPSController.cs`:
- Around line 589-629: Replace the discarded SyncGroupInBackground call in
GroupSync with enqueueing the groupId and group name through a registered
background queue/handler. Implement processing in an IHostedService or
BackgroundService using IServiceScopeFactory to resolve RAPSContext and run
OuGroupService.Sync, with appropriate failure handling and lifecycle tracking;
remove the controller-owned fire-and-forget execution path.
- Around line 74-99: In the Nav method, replace the SingleOrDefaultAsync lookup
of VwAaudUser by MothraId with FirstOrDefaultAsync, preserving the existing null
guard and AsNoTracking behavior so duplicate MothraId rows do not fail RAPS
requests.
In `@web/Views/Shared/Components/ProfilePic/ProfilePic.cs`:
- Around line 17-22: Update ProfilePic.Invoke to InvokeAsync and await an
asynchronous GetByLoginIdAsync lookup; implement or reuse the corresponding
IUserHelper/UserHelper path with FirstOrDefaultAsync for cache misses. Preserve
the existing “Default” view and user result, and do not use Task.Run.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 5a7266cc-41db-41e7-89dd-c94498fea40c
📒 Files selected for processing (23)
.editorconfigtest/ClinicalScheduler/Integration/ServiceLayerIntegrationTest.cstest/Directory/DirectoryControllerTests.cstest/RAPS/AdGroupsControllerTests.cstest/RAPS/RAPSControllerTests.csweb/Areas/CMS/Controllers/CMSController.csweb/Areas/CTS/Controllers/CTSController.csweb/Areas/Directory/Controllers/DirectoryController.csweb/Areas/RAPS/Controllers/AdGroupsController.csweb/Areas/RAPS/Controllers/RAPSController.csweb/Areas/RAPS/Models/GroupAddEdit.csweb/Areas/Students/Services/PhotoService.csweb/Classes/HealthChecks/LdapHealthCheck.csweb/Views/Shared/Components/CMSBlocks/CMSBlocks.csweb/Views/Shared/Components/EmulationBanner/EmulationBanner.csweb/Views/Shared/Components/LeftNav/LeftNav.csweb/Views/Shared/Components/MainNav/MainNav.csweb/Views/Shared/Components/MiniNav/MiniNav.csweb/Views/Shared/Components/ProfilePic/ProfilePic.csweb/Views/Shared/Components/SessionTimeout/SessionTimeout.csweb/Views/Shared/Components/VueCdn/VueCdnCreate.csweb/Views/Shared/Components/VueCdn/VueCdnInit.csweb/Views/Shared/Components/VueTableDefault/VueTableDefault.cs
- convert controller actions and view components with no real async work to synchronous signatures - keep the two legitimate Task.Run offloads (blocking LDAP bind, network-share File.Exists) with comments explaining why
rlorenzo
force-pushed
the
refactor/remove-fake-async-task-run
branch
from
July 31, 2026 22:41
0b639b1 to
e15a349
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 15 out of 15 changed files in this pull request and generated no new comments.
Suppressed comments (1)
web/Areas/Directory/Controllers/DirectoryController.cs:112
- The inline comment still refers to "uid", but this action now routes/binds by
mothraID(and the XML doc was updated accordingly). Update the comment to avoid confusion when maintaining this action.
// pull in the user based on uid
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
await Task.Run(() => ...)wrappers that scheduled trivial synchronous work (mostlyView(...)allocations) onto the thread poolInvokeAsynctoInvoke) to synchronous signatures; actions with real awaits stay asyncTask.Runoffloads (LdapHealthCheck's blocking LDAP bind, PhotoService's network-shareFile.Exists) with comments explaining whyRAPSControllerTeststo the syncRolePermissionssignature via a sync overload of the bad-request helper<param>tags whose names never matched their parameter (RoleList,DirectoryResult), flagged by the ReSharper gate once the signature lines entered the diffTesting
RAPSController.Nav, is pre-existing and on untouched lines)