Conversation
|
Thanks for opening this, but we'd appreciate a little more information. Could you update it with more details? |
📝 WalkthroughWalkthroughTwo files modified: DepartmentNotification.cs adds null or whitespace checks to translation methods with type-specific return values, while NotificationsController.cs converts synchronous method calls to asynchronous awaits in the user notification aggregation path. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes 🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
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}
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)
Web/Resgrid.Web/Areas/User/Controllers/NotificationsController.cs (1)
97-104:⚠️ Potential issue | 🟠 MajorHandle stale user IDs before dereferencing
user.UserId.Line 99 can return
null, and Line 102 / Line 104 dereference it immediately. That means older notifications with deleted or stale recipients can still break the page here.UserHelper.GetFullNameForUseralready accepts the storeduserId, so either pass that directly or skip missing users.Suggested fix
if (!String.IsNullOrWhiteSpace(notification.UsersToNotify)) { var users = notification.UsersToNotify.Split(char.Parse(",")); foreach (var userId in users) { - var user = _usersService.GetUserById(userId); - - if (sb.Length > 0) - sb.Append("," + await UserHelper.GetFullNameForUser(user.UserId)); - else - sb.Append(await UserHelper.GetFullNameForUser(user.UserId)); + var fullName = await UserHelper.GetFullNameForUser(userId); + if (string.IsNullOrWhiteSpace(fullName)) + continue; + + if (sb.Length > 0) + sb.Append(','); + + sb.Append(fullName); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Web/Resgrid.Web/Areas/User/Controllers/NotificationsController.cs` around lines 97 - 104, NotificationsController is dereferencing user.UserId after calling _usersService.GetUserById(userId), which can return null for stale/deleted users; update the loop over users to handle nulls by either skipping when user == null or by calling UserHelper.GetFullNameForUser with the original userId instead of user.UserId. Specifically, in the foreach over users, check if _usersService.GetUserById(userId) returned null and continue (or call UserHelper.GetFullNameForUser(userId) directly), and keep the existing StringBuilder logic when appending names so null lookups no longer throw.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Core/Resgrid.Model/DepartmentNotification.cs`:
- Around line 91-92: The TranslateBefore and TranslateCurrent methods in
DepartmentNotification use int.Parse in multiple places (e.g., the branches
around the BeforeData/CurrentData checks and the parse calls referenced at lines
noted in your comment) which will throw on malformed data; replace every
int.Parse usage in these methods (including redundant double-parses) with
int.TryParse and fall back to the existing sentinel strings ("Any" for empty
BeforeData, "Unknown" where currently used) so invalid or legacy values do not
throw—specifically update all parse points in TranslateBefore and
TranslateCurrent to attempt TryParse into a local int and use the fallback when
TryParse fails, and remove duplicate parse calls by reusing the parsed result.
---
Outside diff comments:
In `@Web/Resgrid.Web/Areas/User/Controllers/NotificationsController.cs`:
- Around line 97-104: NotificationsController is dereferencing user.UserId after
calling _usersService.GetUserById(userId), which can return null for
stale/deleted users; update the loop over users to handle nulls by either
skipping when user == null or by calling UserHelper.GetFullNameForUser with the
original userId instead of user.UserId. Specifically, in the foreach over users,
check if _usersService.GetUserById(userId) returned null and continue (or call
UserHelper.GetFullNameForUser(userId) directly), and keep the existing
StringBuilder logic when appending names so null lookups no longer throw.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f17fc74f-2321-4a9d-9ead-7c017be08146
📒 Files selected for processing (2)
Core/Resgrid.Model/DepartmentNotification.csWeb/Resgrid.Web/Areas/User/Controllers/NotificationsController.cs
| if (string.IsNullOrWhiteSpace(BeforeData)) | ||
| return "Any"; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n Core/Resgrid.Model/DepartmentNotification.cs | head -300Repository: Resgrid/Core
Length of output: 9712
Use non-throwing parsing in these translation branches.
These guards fix the blank-data case, but lines 97, 126, 155, 217, 246, and 275 still use int.Parse on any other non-empty value. A legacy or partially corrupted notification row will still throw while rendering the page. Additionally, lines 157, 248, and 277 contain redundant parse calls that should also be made non-throwing.
Switch all these branches to int.TryParse with the current "Unknown"/"Any" sentinel fallback.
Suggested pattern
- int id = int.Parse(BeforeData);
+ if (!int.TryParse(BeforeData, out var id))
+ return "Unknown";Apply the same pattern to all instances in both TranslateBefore and TranslateCurrent methods.
Also applies to: 120-121, 149-150, 157, 211-212, 240-241, 248, 269-270, 277
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Core/Resgrid.Model/DepartmentNotification.cs` around lines 91 - 92, The
TranslateBefore and TranslateCurrent methods in DepartmentNotification use
int.Parse in multiple places (e.g., the branches around the
BeforeData/CurrentData checks and the parse calls referenced at lines noted in
your comment) which will throw on malformed data; replace every int.Parse usage
in these methods (including redundant double-parses) with int.TryParse and fall
back to the existing sentinel strings ("Any" for empty BeforeData, "Unknown"
where currently used) so invalid or legacy values do not throw—specifically
update all parse points in TranslateBefore and TranslateCurrent to attempt
TryParse into a local int and use the fallback when TryParse fails, and remove
duplicate parse calls by reusing the parsed result.
|
Approve |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
1 participant
Summary by CodeRabbit
Bug Fixes
Refactor