Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork73
Develop#259
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Thanks for opening this, but we'd appreciate a little more information. Could you update it with more details? |
coderabbitaibot commentedOct 23, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
WalkthroughAdds a GitHub Actions workflow to post merged PRs to Changerawr, changes SubscriptionsService to return an empty DepartmentPlanCount instead of null on missing billing data, removes an unused using from MongoRepository, and changes MappingController to fetch personnel via a new users-service method. Changes
Sequence Diagram(s)sequenceDiagram participant GH as GitHub participant Runner as Actions Runner participant Script as github-script participant Ch as Changerawr API rect rgb(235,245,255) Note over GH,Runner: PR closed (master) & merged end GH->>Runner: trigger workflow (pull_request.closed) Runner->>Script: run script (extract PR data) Script->>Script: build payload (notes, metadata) rect rgb(240,255,240) Note over Script,Ch: POST changelog entry end Script->>Ch: POST /api/projects/{id}/changelog (payload, API key header) alt 2xx Success Ch-->>Script: 2xx response Script->>Runner: log success Script->>GH: optionally post comment on PR else Non-OK / Error Ch-->>Script: non-OK / error Script->>Runner: log error & fail job endEstimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
🔇 Additional comments (3)
Thanks for usingCodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Actionable comments posted: 3
🧹 Nitpick comments (1)
.github/workflows/changerawr-sync.yml (1)
41-57:Consider validating required environment variables.The workflow uses
CHANGERAWR_API_KEYandCHANGERAWR_PROJECT_IDfrom secrets but doesn't validate they exist before making the API call. While the API call will fail anyway, early validation would provide clearer error messages.Add validation before the fetch call:
try {+ if (!process.env.CHANGERAWR_API_KEY || !process.env.CHANGERAWR_PROJECT_ID) {+ throw new Error('Required secrets CHANGERAWR_API_KEY and CHANGERAWR_PROJECT_ID must be configured');+ }+ const response = await fetch(
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.github/workflows/changerawr-sync.yml(1 hunks)Core/Resgrid.Services/SubscriptionsService.cs(1 hunks)Repositories/Resgrid.Repositories.NoSqlRepository/MongoRepository.cs(0 hunks)Web/Resgrid.Web.Services/Controllers/v4/MappingController.cs(2 hunks)
💤 Files with no reviewable changes (1)
- Repositories/Resgrid.Repositories.NoSqlRepository/MongoRepository.cs
🧰 Additional context used
📓 Path-based instructions (1)
**/*.cs
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.cs: Prefer functional patterns and immutable data where appropriate in C#
Use modern C# features appropriately
Use meaningful, descriptive names for types, methods, and parameters; avoid unclear abbreviations
Separate state from behavior (e.g., use records for state and place operations in separate static classes)
Prefer pure methods over methods with side effects
Use extension methods appropriately for domain-specific operations
Design for testability; avoid hidden dependencies inside methods and prefer explicit, pure functions
Minimize constructor injection; keep the number of injected dependencies small
Prefer composition with interfaces to extend behavior
Files:
Core/Resgrid.Services/SubscriptionsService.csWeb/Resgrid.Web.Services/Controllers/v4/MappingController.cs
🧬 Code graph analysis (1)
Core/Resgrid.Services/SubscriptionsService.cs (1)
Core/Resgrid.Model/DepartmentPlanCount.cs (1)
DepartmentPlanCount(3-23)
🪛 GitHub Check: CodeQL
.github/workflows/changerawr-sync.yml
[warning] 13-73: Workflow does not contain permissions
Actions job or workflow does not limit the permissions of the GITHUB_TOKEN. Consider setting an explicit permissions block, using the following as a minimal starting point: {{}}
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: build
- GitHub Check: Analyze (csharp)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (csharp)
🔇 Additional comments (2)
Core/Resgrid.Services/SubscriptionsService.cs (1)
101-105:LGTM: Improved null safety.Returning an empty
DepartmentPlanCountobject instead of null is a good defensive programming practice that prevents null reference exceptions in calling code. This aligns with the existing fallback behavior at line 110 and provides consistent semantics (zero counts when plan data is unavailable).Web/Resgrid.Web.Services/Controllers/v4/MappingController.cs (1)
106-106:LGTM: Clean refactoring to users service.The change from
GetAllPersonnelNamesForDepartmentAsynctoGetUserGroupAndRolesByDepartmentIdAsyncappears to consolidate personnel data retrieval into the users service. The newpeoplecollection provides the necessaryUserIdandNameproperties used throughout the mapping logic, and the refactoring maintains the same functionality while improving the API design.Also applies to: 377-379
| jobs: | ||
| post-to-changerawr: | ||
| # Only run if the PR was merged (not just closed) | ||
| if:github.event.pull_request.merged == true | ||
| runs-on:ubuntu-latest | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Add explicit permissions block for security.
The workflow should define explicit permissions for the GITHUB_TOKEN to follow the principle of least privilege. This prevents potential security issues if the workflow is modified in the future.
Apply this diff to add a minimal permissions block:
jobs: post-to-changerawr: # Only run if the PR was merged (not just closed) if: github.event.pull_request.merged == true runs-on: ubuntu-latest+ permissions:+ pull-requests: write+ contents: read steps:
Based on static analysis hints.
🤖 Prompt for AI Agents
.github/workflows/changerawr-sync.yml lines 10-15: the workflow lacks anexplicit permissions block for GITHUB_TOKEN; add a top-level permissions section(placed before jobs:) that explicitly grants only the minimal scopes this jobneeds (e.g., contents: read and any specific scopes like pull-requests: write oractions: read only if required by the post-to-changerawr job); ensure you scopepermissions to the least privilege necessary and do not rely on defaultpermissions.Uh oh!
There was an error while loading.Please reload this page.
| //var personnelNames = await _departmentsService.GetAllPersonnelNamesForDepartmentAsync(DepartmentId); | ||
| varpeople=await_usersService.GetUserGroupAndRolesByDepartmentIdAsync(DepartmentId,false,false,false); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
🛠️ Refactor suggestion |🟠 Major
Remove commented-out code.
The commented-out line should be deleted rather than left in the codebase. Version control preserves the history if needed.
Apply this diff:
-//var personnelNames = await _departmentsService.GetAllPersonnelNamesForDepartmentAsync(DepartmentId); var people = await _usersService.GetUserGroupAndRolesByDepartmentIdAsync(DepartmentId, false, false, false); var personnelLocations = await _usersService.GetLatestLocationsForDepartmentPersonnelAsync(DepartmentId);As per coding guidelines (avoid commented-out code).
📝 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.
| //var personnelNames = await _departmentsService.GetAllPersonnelNamesForDepartmentAsync(DepartmentId); | |
| varpeople=await_usersService.GetUserGroupAndRolesByDepartmentIdAsync(DepartmentId,false,false,false); | |
| varpeople=await_usersService.GetUserGroupAndRolesByDepartmentIdAsync(DepartmentId,false,false,false); |
🤖 Prompt for AI Agents
In Web/Resgrid.Web.Services/Controllers/v4/MappingController.cs around lines 105to 106, remove the commented-out line "//var personnelNames = await_departmentsService.GetAllPersonnelNamesForDepartmentAsync(DepartmentId);" soonly the active call to _usersService remains; commit the deletion (no codechanges beyond removing the commented line) to keep the codebase clean and relyon version control for history.…ain permissionsCo-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Approve |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This PR is approved.
0f606fe intomasterUh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Summary by CodeRabbit
Bug Fixes
Refactor
Chores