refactor(async): drop fake-async Task.Run wrappers#264
Open
rlorenzo wants to merge 9 commits into
Open
Conversation
- suppress CA1505 for scaffolded EF contexts and S6964 for ApiPagination, which is never model-bound (editorconfig precedent) - create the Data.CMS logger via ILoggerFactory so the logger category matches its consumer (S6672) - move the Directory route prefix to the controller, URLs unchanged (S6931) - return 400 from RAPS view actions when model binding fails instead of proceeding with defaulted params (S6967) - suppress S6967/S6932 on the RAPS and CTS nav filter overrides with justification; mark RAPSController.Nav [NonAction] to remove an unintended endpoint - make GroupAddEdit.GroupId nullable to reflect create semantics (S6964)
Splits the two large NSubstitute fakes out of ServiceLayerIntegrationTest constructor (CA1502); behavior unchanged.
The BadRequest result was discarded, so a PUT whose body groupId did not match the route fell through and updated the group anyway. Add a regression test asserting the 400 and that the group is unchanged.
SingleAsync threw on every RAPS page when the memberId query param matched no user; the nav already handles the null case.
OnActionExecutionAsync awaited both the AreaController base, which already invokes next(), and next() again, so every RAPS action ran its pipeline twice.
The fire-and-forget sync captured the request-scoped RAPSContext, which is disposed once the response returns, faulting the background task. Resolve a fresh context from IServiceScopeFactory and log failures instead of leaving them unobserved.
Unit-locks the new 400-on-binding-failure behavior for all 8 guarded RAPS view actions and covers the permission lookup found/not-found paths plus the GroupSync no-op path (no background sync for unknown groups). Raises the PR patch coverage flagged by Codecov.
- Closes the CA1502 warning on GetUCD; the shared query helper and the VMACS enrichment were duplicated verbatim across both actions - SQLite-backed tests pin the query helper (field matches, name-span match, current-user filter, ordering)
- 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
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 45.39% 45.59% +0.20%
==========================================
Files 916 916
Lines 52784 52805 +21
Branches 5005 5015 +10
==========================================
+ Hits 23960 24076 +116
+ Misses 28196 28096 -100
- Partials 628 633 +5
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}
Comment on lines
+623
to
+628
| catch (Exception ex) | ||
| { | ||
| // Background-job entry point: the task is discarded, so anything not caught | ||
| // here becomes an unobserved exception and the sync fails with no log entry. | ||
| LogManager.GetCurrentClassLogger().Error(ex, "Group sync failed for group {GroupId}", groupId); | ||
| } |
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
Comment on lines
+177
to
+190
| [Fact] | ||
| public async Task GroupSync_RendersWithoutSyncing_WhenGroupMissing() | ||
| { | ||
| using var connection = await OpenConnectionAsync(); | ||
| using var context = await CreateContextAsync(connection); | ||
| var scopeFactory = Substitute.For<IServiceScopeFactory>(); | ||
| var controller = CreateController(context, scopeFactory); | ||
|
|
||
| var result = await controller.GroupSync(9999); | ||
|
|
||
| var view = Assert.IsType<ViewResult>(result); | ||
| Assert.Null(view.ViewData["Group"]); | ||
| scopeFactory.DidNotReceive().CreateScope(); | ||
| } |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Add success-path coverage for GroupSync.
Current coverage only verifies the "group missing" branch (DidNotReceive().CreateScope()). Add a case seeding an OuGroup and asserting scopeFactory.Received(1).CreateScope() (and/or that ViewData["Group"] is populated) to cover the actual dispatch path that was reworked in this PR. CreateScope() runs synchronously before the first await in SyncGroupInBackground, so this doesn't require awaiting the fire-and-forget task to completion.
🤖 Prompt for 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.
In `@test/RAPS/RAPSControllerTests.cs` around lines 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.
Comment on lines
+57
to
75
| [Route("search/{search}")] | ||
| public async Task<ActionResult<IEnumerable<IndividualSearchResult>>> Get(string search) | ||
| { | ||
| var individuals = await _aaud.AaudUsers | ||
| .Where(u => (u.DisplayFirstName + " " + u.DisplayLastName).Contains(search) | ||
| || (u.MailId != null && u.MailId.Contains(search)) | ||
| || (u.LoginId != null && u.LoginId.Contains(search)) | ||
| || (u.SpridenId != null && u.SpridenId.Contains(search)) | ||
| || (u.Pidm != null && u.Pidm.Contains(search)) | ||
| || (u.MothraId != null && u.MothraId.Contains(search)) | ||
| || (u.EmployeeId != null && u.EmployeeId.Contains(search)) | ||
| || (u.IamId != null && u.IamId.Contains(search)) | ||
| ) | ||
| .Where(u => u.Current != 0) | ||
| .OrderBy(u => u.DisplayLastName) | ||
| .ThenBy(u => u.DisplayFirstName) | ||
| .ToListAsync(); | ||
| var individuals = await SearchCurrentAaudUsers(_aaud, search); | ||
| List<IndividualSearchResult> results = new(); | ||
| AaudUser? currentUser = UserHelper.GetCurrentUser(); | ||
| bool hasDetailPermission = UserHelper.HasPermission(_rapsContext, currentUser, "SVMSecure.DirectoryDetail"); | ||
| individuals.ForEach(m => | ||
| foreach (var m in individuals) | ||
| { | ||
| LdapUserContact? l = LdapService.GetUserByID(m.IamId); | ||
| var result = hasDetailPermission | ||
| ? new IndividualSearchResultWithIDs(m, l) | ||
| : new IndividualSearchResult(m, l); | ||
| result.LookupEmailHost(_aaud); | ||
| results.Add(result); | ||
|
|
||
| var vmsearch = VMACSService.Search(result.LoginId); | ||
| var vm = vmsearch.Result; | ||
| if (vm != null && vm.item != null && vm.item.Nextel != null) result.Nextel = vm.item.Nextel[0]; | ||
| if (vm != null && vm.item != null && vm.item.LDPager != null) result.LDPager = vm.item.LDPager[0]; | ||
| if (vm != null && vm.item != null && vm.item.Unit != null) result.Department = vm.item.Unit[0]; | ||
|
|
||
| }); | ||
| await AddVmacsContactInfoAsync(result); | ||
| } | ||
| return results; | ||
| } |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Locate LdapService.GetUserByID implementation to confirm if it's a blocking call
fd -e cs -i ldapservice
ast-grep run --pattern 'public static $RET GetUserByID($$$) { $$$ }' --lang csharp
rg -nP -A 15 'static.*GetUserByID\s*\(' --type=csRepository: ucdavis/VIPER
Length of output: 229
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== LdapService outline =="
ast-grep outline web/Classes/Utilities/LdapService.cs --view expanded || true
echo "== GetUserByID occurrences =="
rg -n -C 3 'GetUserByID|BeginBind|Bind|NewLDAP|LDAP|Task\.Run' web/Classes/Utilities/LdapService.cs test/Utilities/LdapServiceFilterTests.cs web/Areas/Directory/Controllers/DirectoryController.cs || true
echo "== relevant LdapService sections =="
sed -n '1,260p' web/Classes/Utilities/LdapService.cs
echo "== DirectoryController relevant lines =="
sed -n '1,120p' web/Areas/Directory/Controllers/DirectoryController.csRepository: ucdavis/VIPER
Length of output: 16851
Keep LDAP user lookups on a pool-thread or offload per-result work.
LdapService.GetUserByID(m.IamId) still uses blocking LdapConnection.Bind(), SendRequest(), and SearchRequest.Entries in the async /search loop, so each matched user blocks the request pipeline while adding result metadata rather than doing work off the async continuation thread.
🤖 Prompt for 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.
In `@web/Areas/Directory/Controllers/DirectoryController.cs` around lines 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.
Comment on lines
+64
to
+73
| foreach (var m in individuals) | ||
| { | ||
| LdapUserContact? l = LdapService.GetUserByID(m.IamId); | ||
| var result = hasDetailPermission | ||
| ? new IndividualSearchResultWithIDs(m, l) | ||
| : new IndividualSearchResult(m, l); | ||
| result.LookupEmailHost(_aaud); | ||
| results.Add(result); | ||
|
|
||
| var vmsearch = VMACSService.Search(result.LoginId); | ||
| var vm = vmsearch.Result; | ||
| if (vm != null && vm.item != null && vm.item.Nextel != null) result.Nextel = vm.item.Nextel[0]; | ||
| if (vm != null && vm.item != null && vm.item.LDPager != null) result.LDPager = vm.item.LDPager[0]; | ||
| if (vm != null && vm.item != null && vm.item.Unit != null) result.Department = vm.item.Unit[0]; | ||
|
|
||
| }); | ||
| await AddVmacsContactInfoAsync(result); | ||
| } |
Contributor
There was a problem hiding this comment.
🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win
Sequential VMACS lookups in Get and GetUCD add N network round-trips per search.
await AddVmacsContactInfoAsync(result) runs inside the foreach, one network call per matched result. This helper never touches _aaud, so it can be pulled out of the loop and awaited in parallel via Task.WhenAll without violating the DbContext thread-safety rule (only LookupEmailHost(_aaud) needs to stay sequential).
⚡ Proposed refactor for `Get` (same pattern applies to `GetUCD`)
foreach (var m in individuals)
{
LdapUserContact? l = LdapService.GetUserByID(m.IamId);
var result = hasDetailPermission
? new IndividualSearchResultWithIDs(m, l)
: new IndividualSearchResult(m, l);
result.LookupEmailHost(_aaud);
results.Add(result);
- await AddVmacsContactInfoAsync(result);
}
+ await Task.WhenAll(results.Select(AddVmacsContactInfoAsync));
return results;Also applies to: 91-100
🤖 Prompt for 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.
In `@web/Areas/Directory/Controllers/DirectoryController.cs` around lines 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.
Comment on lines
74
to
+99
| /// <summary> | ||
| /// RAPS home page | ||
| /// </summary> | ||
| [Route("/[area]/{instance?}")] | ||
| public async Task<ActionResult> Index(string? instance) | ||
| public ActionResult Index(string? instance) | ||
| { | ||
| ViewData["KeyColumnName"] = "RoleId"; | ||
| instance ??= _securityService.GetDefaultInstanceForUser(); | ||
|
|
||
| return instance.ToUpper() switch | ||
| { | ||
| "VIPER" => await Task.Run(() => Redirect("~/raps/VIPER/rolelist")), | ||
| "VMACS.VMTH" => await Task.Run(() => Redirect("~/raps/VMACS.VMTH/rolelist")), | ||
| "VMACS.VMLF" => await Task.Run(() => Redirect("~/raps/VMACS.VMLF/rolelist")), | ||
| "VMACS.UCVMCSD" => await Task.Run(() => Redirect("~/raps/VMACS.UCVMCSD/rolelist")), | ||
| "VIPERFORMS" => await Task.Run(() => Redirect("~/raps/ViperForms/rolelist")), | ||
| _ => await Task.Run(() => View("~/Views/Home/403.cshtml")), | ||
| "VIPER" => Redirect("~/raps/VIPER/rolelist"), | ||
| "VMACS.VMTH" => Redirect("~/raps/VMACS.VMTH/rolelist"), | ||
| "VMACS.VMLF" => Redirect("~/raps/VMACS.VMLF/rolelist"), | ||
| "VMACS.UCVMCSD" => Redirect("~/raps/VMACS.UCVMCSD/rolelist"), | ||
| "VIPERFORMS" => Redirect("~/raps/ViperForms/rolelist"), | ||
| _ => View("~/Views/Home/403.cshtml"), | ||
| }; | ||
| } | ||
|
|
||
| [NonAction] | ||
| public async Task<NavMenu> Nav(int? roleId, int? permissionId, string? memberId, string instance = "VIPER", string page = "") | ||
| { | ||
| TblRole? selectedRole = (roleId != null) ? await _RAPSContext.TblRoles.FindAsync(roleId) : null; | ||
| TblPermission? selectedPermission = (permissionId != null) ? await _RAPSContext.TblPermissions.FindAsync(permissionId) : null; | ||
| VwAaudUser? selecteduser = (memberId != null) ? await _RAPSContext.VwAaudUser.SingleAsync(r => r.MothraId == memberId) : null; | ||
| VwAaudUser? selecteduser = (memberId != null) ? await _RAPSContext.VwAaudUser.AsNoTracking().SingleOrDefaultAsync(r => r.MothraId == memberId) : null; |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== file exists and size =="
wc -l web/Areas/RAPS/Controllers/RAPSController.cs || true
echo "== relevant controller lines =="
sed -n '1,180p' web/Areas/RAPS/Controllers/RAPSController.cs | cat -n
echo "== search VwAaudUser usages =="
rg -n "VwAaudUser|MothraId|SingleOrDefaultAsync|FirstOrDefaultAsync|FindAsync" web -S
echo "== locate model/context files containing VwAaudUser =="
rg -n "VwAaudUser|VwAaud" . -S | head -100 || trueRepository: ucdavis/VIPER
Length of output: 50370
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== RAPSContext VwAaudUser config =="
sed -n '45,65p' web/Classes/SQLContext/RAPSContext.cs | cat -n
sed -n '200,215p' web/Classes/SQLContext/RAPSContext.cs | cat -n
sed -n '480,525p' web/Classes/SQLContext/RAPSContext.cs | cat -n
echo "== AaudUser key/index/constraint references =="
rg -n "AaudUser|MothraId|HasIndex|IsUnique|HasIndexIsUnique|UniqueIndex|Key|MothraId" web/Models/AAUD web/Classes/SQLContext web/Migrations -S | head -200
echo "== VwAaudUser model files =="
fd -i "VwAaudUser|AaudUser" web -t f -x sh -c 'echo "--- $1"; wc -l "$1"; sed -n "1,220p" "$1" | cat -n' sh {}
echo "== RAPS migrations that mention AaudUser/VwAaudUser =="
rg -n "AaudUser|VwAaudUser|Aaud|VwAaud" web/Migrations -S || trueRepository: ucdavis/VIPER
Length of output: 21451
Use FirstOrDefaultAsync and guard duplicate MothraId usage
OnActionExecutionAsync loads left-nav for every RAPS request, and Nav() uses SingleOrDefaultAsync for VwAaudUser.MothraId. Since VwAaudUser has no constrained/untracked guarantee of unique MothraId, duplicate AAUD rows make the whole RAPS area 500 when a memberId query is present. Use FirstOrDefaultAsync here, or enforce/guard against duplicate MothraId before this filter.
🛡️ Proposed fix
- VwAaudUser? selecteduser = (memberId != null) ? await _RAPSContext.VwAaudUser.AsNoTracking().SingleOrDefaultAsync(r => r.MothraId == memberId) : null;
+ VwAaudUser? selecteduser = (memberId != null) ? await _RAPSContext.VwAaudUser.AsNoTracking().FirstOrDefaultAsync(r => r.MothraId == memberId) : null;📝 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.
Suggested change
| /// <summary> | |
| /// RAPS home page | |
| /// </summary> | |
| [Route("/[area]/{instance?}")] | |
| public async Task<ActionResult> Index(string? instance) | |
| public ActionResult Index(string? instance) | |
| { | |
| ViewData["KeyColumnName"] = "RoleId"; | |
| instance ??= _securityService.GetDefaultInstanceForUser(); | |
| return instance.ToUpper() switch | |
| { | |
| "VIPER" => await Task.Run(() => Redirect("~/raps/VIPER/rolelist")), | |
| "VMACS.VMTH" => await Task.Run(() => Redirect("~/raps/VMACS.VMTH/rolelist")), | |
| "VMACS.VMLF" => await Task.Run(() => Redirect("~/raps/VMACS.VMLF/rolelist")), | |
| "VMACS.UCVMCSD" => await Task.Run(() => Redirect("~/raps/VMACS.UCVMCSD/rolelist")), | |
| "VIPERFORMS" => await Task.Run(() => Redirect("~/raps/ViperForms/rolelist")), | |
| _ => await Task.Run(() => View("~/Views/Home/403.cshtml")), | |
| "VIPER" => Redirect("~/raps/VIPER/rolelist"), | |
| "VMACS.VMTH" => Redirect("~/raps/VMACS.VMTH/rolelist"), | |
| "VMACS.VMLF" => Redirect("~/raps/VMACS.VMLF/rolelist"), | |
| "VMACS.UCVMCSD" => Redirect("~/raps/VMACS.UCVMCSD/rolelist"), | |
| "VIPERFORMS" => Redirect("~/raps/ViperForms/rolelist"), | |
| _ => View("~/Views/Home/403.cshtml"), | |
| }; | |
| } | |
| [NonAction] | |
| public async Task<NavMenu> Nav(int? roleId, int? permissionId, string? memberId, string instance = "VIPER", string page = "") | |
| { | |
| TblRole? selectedRole = (roleId != null) ? await _RAPSContext.TblRoles.FindAsync(roleId) : null; | |
| TblPermission? selectedPermission = (permissionId != null) ? await _RAPSContext.TblPermissions.FindAsync(permissionId) : null; | |
| VwAaudUser? selecteduser = (memberId != null) ? await _RAPSContext.VwAaudUser.SingleAsync(r => r.MothraId == memberId) : null; | |
| VwAaudUser? selecteduser = (memberId != null) ? await _RAPSContext.VwAaudUser.AsNoTracking().SingleOrDefaultAsync(r => r.MothraId == memberId) : null; | |
| VwAaudUser? selecteduser = (memberId != null) ? await _RAPSContext.VwAaudUser.AsNoTracking().FirstOrDefaultAsync(r => r.MothraId == memberId) : null; |
🤖 Prompt for 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.
In `@web/Areas/RAPS/Controllers/RAPSController.cs` around lines 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.
Comment on lines
589
to
629
| [Permission(Allow = "RAPS.Admin,RAPS.OUGroupsView")] | ||
| [Route("/[area]/{Instance}/[action]")] | ||
| [SupportedOSPlatform("windows")] | ||
| public async Task<IActionResult> GroupSync(int groupId) | ||
| { | ||
| if (!ModelState.IsValid) | ||
| { | ||
| return BadRequest(); | ||
| } | ||
| OuGroup? group = await _RAPSContext.OuGroups.FindAsync(groupId); | ||
| if (group != null) | ||
| { | ||
| _ = new OuGroupService(_RAPSContext).Sync(groupId, group.Name); | ||
| _ = SyncGroupInBackground(groupId, group.Name); | ||
| } | ||
|
|
||
| ViewData["Group"] = group; | ||
| return await Task.Run(() => View("~/Areas/RAPS/Views/Groups/Sync.cshtml")); | ||
| return View("~/Areas/RAPS/Views/Groups/Sync.cshtml"); | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Run the AD/OU group sync outside the request scope so it can keep running after the response | ||
| /// is returned (the sync page tells users it may take a few minutes). Resolves its own RAPSContext | ||
| /// from a fresh DI scope, since the request-scoped _RAPSContext is disposed once the request ends. | ||
| /// </summary> | ||
| [SupportedOSPlatform("windows")] | ||
| [NonAction] | ||
| public async Task SyncGroupInBackground(int groupId, string groupName) | ||
| { | ||
| try | ||
| { | ||
| using var scope = _scopeFactory.CreateScope(); | ||
| var context = scope.ServiceProvider.GetRequiredService<RAPSContext>(); | ||
| await new OuGroupService(context).Sync(groupId, groupName); | ||
| } | ||
| catch (Exception ex) | ||
| { | ||
| // Background-job entry point: the task is discarded, so anything not caught | ||
| // here becomes an unobserved exception and the sync fails with no log entry. | ||
| LogManager.GetCurrentClassLogger().Error(ex, "Group sync failed for group {GroupId}", groupId); | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Locate controller and relevant symbols"
fd -a 'RAPSController\.cs$' . | sed 's#^\./##'
controller=$(fd 'RAPSController\.cs$' . | head -n1 || true)
if [ -n "${controller:-}" ]; then
echo "Using $controller"
wc -l "$controller"
echo "--- lines 560-640 ---"
sed -n '560,640p' "$controller" | nl -ba -v560
echo "--- occurrences of SyncGroupInBackground or BackgroundService/HostedService/IHostApplicationLifetime ---"
rg -n "(SyncGroupInBackground|BackgroundService|IHostedService|IHostApplicationLifetime|Task\.Factory|TaskCompletionSource|Task\.Run|AddHostedService|IMemoryCache|BackgroundTaskScheduler|WorkItem)" "$controller" "$(dirname "$controller")" || true
fi
echo "--- repository-wide hints for background/hosted services in RAPS area ---"
rg -n "IHostedService|BackgroundService|AddHostedService|AddHosted|BackgroundTask|Task\.Run|_ = .*Async|IHostApplicationLifetime|ApplicationStopping|ApplicationStopped" web/Areas/RAPS 2>/dev/null || trueRepository: ucdavis/VIPER
Length of output: 421
🏁 Script executed:
#!/bin/bash
set -euo pipefail
controller="$(fd 'RAPSController\.cs$' . | head -n1)"
echo "controller=$controller"
wc -l "$controller"
echo "--- lines 560-650 ---"
awk '{printf "%6d\t%s\n", NR, $0}' "$controller" | sed -n '560,650p'
echo "--- target symbols in controller ---"
rg -n "SyncGroupInBackground|_ = SyncGroupInBackground|BackgroundService|IHostedService|AddHostedService|IHostApplicationLifetime|ApplicationStopping|ApplicationStopped|Task\.Run|TaskCompletionSource" "$controller" || true
echo "--- target symbols in RAPS area ---"
rg -n "SyncGroupInBackground|BackgroundService|IHostedService|AddHostedService|IHostApplicationLifetime|ApplicationStopping|ApplicationStopped|Task\.Run|TaskCompletionSource" web/Areas/RAPS || trueRepository: ucdavis/VIPER
Length of output: 4660
Use a registered background handler instead of discarding SyncGroupInBackground.
GroupSync returns immediately after _ = SyncGroupInBackground(...), so ASP.NET can stop the app/pool before the task runs, and the task is not tracked for retries or durable failure handling. Queue the sync ID/name and process it through an IHostedService/BackgroundService using IServiceScopeFactory.
🤖 Prompt for 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.
In `@web/Areas/RAPS/Controllers/RAPSController.cs` around lines 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.
Comment on lines
+17
to
+22
| public IViewComponentResult Invoke(string? userName) | ||
| { | ||
| IUserHelper UserHelper = new UserHelper(); | ||
| AaudUser? user = UserHelper.GetByLoginId(_AAUDContext, userName); | ||
|
|
||
| return await Task.Run(() => View("Default", user)); | ||
| return View("Default", user); |
Contributor
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
rg -n -C 3 '\bGetByLoginId(?:Async)?\b|\bProfilePicViewComponent\b' webRepository: ucdavis/VIPER
Length of output: 7543
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "## ProfilePic.cs"
cat -n web/Views/Shared/Components/ProfilePic/ProfilePic.cs
echo
echo "## UserHelper.cs relevant sections"
sed -n '225,275p' web/Classes/UserHelper.cs | cat -n
echo
echo "## GetByLoginId declarations"
sed -n '370,380p' web/Classes/UserHelper.cs | cat -n
echo
echo "## Find EF async methods"
rg -n -C 2 '\b(FirstOrDefault\(|LastOrDefault\(|SingleOrDefault\|Async|FirstOrDefaultAsync|FirstOrDefault)\b' webRepository: ucdavis/VIPER
Length of output: 50370
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "## Programmatic check: synchronous EF calls from ProfilePic path"
python3 - <<'PY'
from pathlib import Path
p = Path('web/Views/Shared/Components/ProfilePic/ProfilePic.cs')
s = p.read_text()
print('ProfilePic.Invoke present:', 'public IViewComponentResult Invoke(string? userName)' in s)
print('ProfilePic.InvokeAsync absent:', 'public async Task<IViewComponentResult> InvokeAsync(string? userName)' not in s)
usp = Path('web/Classes/UserHelper.cs').read_text()
gb = usp[usp.index('public AaudUser? GetByLoginId(AAUDContext aaudContext, string? loginId)'):usp.index('public AaudUser??', usp.find(']', usp.index('`#region` public AaudUser? GetByLoginId')))]
print('GetByLoginId sync database AaudUsers query:', 'AaudUsers.FirstOrDefault(' in gb)
print('GetByLoginIdAsync method present:', 'GetByLoginIdAsync' in usp)
PYRepository: ucdavis/VIPER
Length of output: 389
Keep the ProfilePic route non-blocking.
ProfilePic.Invoke now calls UserHelper.GetByLoginId, whose cache miss path executes AaudContext.AaudUsers.FirstOrDefault(...). That blocks the ASP.NET request thread on database I/O; use an async GetByLoginIdAsync/FirstOrDefaultAsync path with InvokeAsync instead of restoring Task.Run.
🤖 Prompt for 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.
In `@web/Views/Shared/Components/ProfilePic/ProfilePic.cs` around lines 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.
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 helperStacked on #261
This branch is based on
chore/warning-quick-wins(#261), so its commits appear here until that PR merges to main. After #261 lands, this branch will be rebased onto main so only the fake-async sweep remains.Testing