refactor(async): drop fake-async wrappers - #264
Merged
Conversation
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
Bundle ReportBundle size has no change ✅ |
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
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
rlorenzo
force-pushed
the
refactor/remove-fake-async-task-run
branch
from
August 6, 2026 08:10
e15a349 to
016530e
Compare
rlorenzo
force-pushed
the
refactor/remove-fake-async-task-run
branch
from
August 7, 2026 04:14
016530e to
edf7bf3
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
- This inline comment still refers to "uid", but the route and parameter were corrected to
mothraID. Updating the comment avoids confusion about which identifier this action uses.
// pull in the user based on uid
rlorenzo
force-pushed
the
refactor/remove-fake-async-task-run
branch
from
August 7, 2026 18:08
edf7bf3 to
6157660
Compare
- convert controller actions and view components with no real async work to synchronous signatures - drop the async state machine from GetEighthsGroupsAsync, which awaited Task.FromResult over a hardcoded list - keep the 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
August 7, 2026 18:38
6157660 to
6ddf817
Compare
rlorenzo
changed the title
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 16 out of 16 changed files in this pull request and generated no new comments.
Suppressed comments (1)
web/Areas/Directory/Controllers/DirectoryController.cs:112
- The route parameter and action parameter are named
mothraID, which is inconsistent with the repo’s prevailing*Idcasing (e.g.,MothraId/mothraIdelsewhere) and makes the inline comment misleading. Rename tomothraIdfor consistency and update the inline comment to match what the stub will eventually do.
/// <param name="mothraID">Mothra ID</param>
[Route("userInfo/{mothraID}")]
public IActionResult DirectoryResult(string mothraID)
{
// pull in the user based on uid
JasonRobertFrancis
approved these changes
Aug 7, 2026
bniedzie
approved these changes
Aug 7, 2026
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 asyncStudentGroupService.GetEighthsGroupsAsync, which awaitedTask.FromResultover a hardcoded list. The interface signature staysTask<List<string>>because its three sibling group getters are genuinely DB-backed, so no call sites changeTask.Runcalls that offload genuinely blocking sync work (LdapHealthCheck's LDAP bind, PhotoService's network-shareFile.Exists) with comments explaining why. The repo's otherTask.Runsites are untouched and are not fake async: the Effort harvest/import/rollover controllers use it for fire-and-forget background jobs, andBiorenderStudentLookupuses it for a throttled fan-outRAPSControllerTeststo the syncRolePermissionssignature via a sync overload of the bad-request helper<param>tags that did not resolve to their parameter:RoleListdocumentedInstanceagainst a parameter namedinstance(XML doc param names are case-sensitive), andDirectoryResultdocumenteduidagainst a parameter namedmothraID. Both were flagged by the ReSharper gate once the signature lines entered the diffTesting
npm run verify:buildclean, with no new analyzer warningsRAPSController.Nav(complexity 35), is pre-existing and on untouched lines. It is one of 17 pre-existing CA1502 warnings in the repo and belongs in a separate refactor