Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| [Authorize(Policy = ResgridResources.CustomStates_Update)] | ||
| public async Task<IActionResult> EditDetail(EditDetailView model, CancellationToken cancellationToken) | ||
| { | ||
| if (model.Detail == null) |
|
Thanks for opening this, but we'd appreciate a little more information. Could you update it with more details? |
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughAdds many .opencode skill docs. Adds NWS area-filter regex validation and controller ModelState errors, adds MaxLength to Name fields, adds null guards and repopulation for views, introduces upcoming-shifts DB query/repo/service and fixes SQL placeholders, regenerates XML docs, and ignores opencode.json. ChangesSkill documentation additions
App code validations and DB/query updates
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
✨ Finishing Touches🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
.opencode/skills/api-versioning/SKILL.md (1)
159-159:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix malformed Decision Guide table row
Line 159 is missing the trailing
|, so the Markdown table can render incorrectly in some viewers.Suggested fix
-| Deprecating a version | Mark deprecated, set sunset date, document migration path | +| Deprecating a version | Mark deprecated, set sunset date, document migration path |🤖 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 @.opencode/skills/api-versioning/SKILL.md at line 159, The Decision Guide table in SKILL.md has a malformed row (missing the trailing pipe on the row at/around line 159); fix it by adding the missing trailing '|' to that table row so the Markdown table renders correctly—locate the Decision Guide table header/rows and ensure every table row (including the one referenced) ends with a trailing pipe character.Web/Resgrid.Web/Areas/User/Views/Dispatch/UpdateCall.cshtml (1)
241-247:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd a null guard for
TargetCallinside the loop.Even with
Model.Call.References != null, Line 243 still dereferencescallReference.TargetCalland can fail if a linked call is missing/deleted.Suggested fix
foreach (var callReference in Model.Call.References) { + if (callReference.TargetCall == null) + { + continue; + } var linkedCallStateText = DispatchDisplayHelper.GetLocalizedCallState(callReference.TargetCall.State, localizer, commonLocalizer);🤖 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/Resgrid.Web/Areas/User/Views/Dispatch/UpdateCall.cshtml` around lines 241 - 247, The loop over Model.Call.References dereferences callReference.TargetCall (used in DispatchDisplayHelper.GetLocalizedCallState, linkedCallStateText and the output/hidden input) without a null check; update the foreach block to skip or continue when callReference.TargetCall is null (e.g., if (callReference.TargetCall == null) continue;) so you never call DispatchDisplayHelper.GetLocalizedCallState or access TargetCall.Number/LoggedOn/Name/Id on a null object.
🟡 Minor comments (25)
.opencode/skills/model-selection/SKILL.md-33-33 (1)
33-33:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd language identifiers to fenced code blocks
Several fenced blocks omit a language tag, which triggers markdownlint (MD040) and reduces editor rendering consistency.
Suggested patch
-``` +```text ROUTINE TASKS → Sonnet 4.6 ... -``` +``` -``` +```text PHASE 1: PLAN (Opus 4.6) ... -``` +``` -``` +```text Claude Code CLI: ... -``` +``` -``` +```text SUBAGENT: "Find all authentication middleware in the project" ... -``` +``` -``` +```text // BAD — Opus for a routine CRUD endpoint ... -``` +``` -``` +```text // BAD — Sonnet for "should we use VSA or Clean Architecture?" ... -``` +``` -``` +```text // BAD — all subagents use Opus ... -``` +```Also applies to: 66-66, 91-91, 105-105, 131-131, 144-144, 158-158
🤖 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 @.opencode/skills/model-selection/SKILL.md at line 33, Multiple fenced code blocks in SKILL.md are missing language identifiers which triggers markdownlint MD040; update each triple-backtick fence enclosing blocks like "ROUTINE TASKS → Sonnet 4.6", "PHASE 1: PLAN (Opus 4.6)", "Claude Code CLI:", 'SUBAGENT: "Find all authentication middleware in the project"', and the "// BAD — ..." example blocks to include a language tag (e.g., ```text) at the opening fence so every fenced block has a language identifier and linter errors are resolved..opencode/skills/aspire/SKILL.md-116-127 (1)
116-127:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd language specifier to fenced code block.
The fenced code block displaying the solution structure is missing a language specifier. Add
textorplaintextto comply with markdown linting rules.📝 Proposed fix
-``` +```text MyApp.slnx ├── MyApp.AppHost/ # Aspire orchestratorAs per coding guidelines, markdownlint-cli2 flagged: "Fenced code blocks should have a language specified (MD040)".
🤖 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 @.opencode/skills/aspire/SKILL.md around lines 116 - 127, The fenced code block in SKILL.md showing the solution tree lacks a language specifier; change the opening fence from ``` to ```text (or ```plaintext) so the block becomes a labeled plain-text block (the block containing "MyApp.slnx" and the tree lines) to satisfy MD040 and markdownlint rules.Web/Resgrid.Web.Services/Resgrid.Web.Services.xml-7699-7707 (1)
7699-7707:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winCorrect
SavePersonnelStaffingInputfield descriptions copied from unit status docs
UserIdandTypesummaries currently reference unit/apparatus semantics, but this DTO is for personnel staffing updates.Suggested doc fix
<member name="P:Resgrid.Web.Services.Models.v4.PersonnelStaffing.SavePersonnelStaffingInput.UserId"> <summary> - UnitId of the apparatus that the state is being set for + UserId of the person whose staffing is being set </summary> </member> <member name="P:Resgrid.Web.Services.Models.v4.PersonnelStaffing.SavePersonnelStaffingInput.Type"> <summary> - The UnitStateType of the Unit + The staffing type to set for the user </summary> </member>🤖 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/Resgrid.Web.Services/Resgrid.Web.Services.xml` around lines 7699 - 7707, The XML doc summaries for SavePersonnelStaffingInput are incorrect: update the <member> summaries for Resgrid.Web.Services.Models.v4.PersonnelStaffing.SavePersonnelStaffingInput.UserId to describe that this is the identifier of the personnel/user being staffed (e.g., "Identifier of the user/personnel for whom the staffing state is being set") and for Resgrid.Web.Services.Models.v4.PersonnelStaffing.SavePersonnelStaffingInput.Type to describe the personnel staffing state/type (e.g., "The PersonnelStaffingType indicating the staffing state for the user"), replacing the current unit/apparatus language so the DTO accurately reflects personnel staffing semantics.Web/Resgrid.Web.Services/Resgrid.Web.Services.xml-9522-9535 (1)
9522-9535:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
UnitStatusInputsummary references user-status behavior and is misleadingThis model is unit-specific, but the summary describes user status behavior and “current user” semantics.
Suggested doc fix
<member name="T:Resgrid.Web.Services.Models.v4.UnitStatus.UnitStatusInput"> <summary> - Object inputs for setting a users Status/Action. If this object is used in an operation that sets - a status for the current user the UserId value in this object will be ignored. + Input object for setting a unit status/action event. </summary> </member>🤖 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/Resgrid.Web.Services/Resgrid.Web.Services.xml` around lines 9522 - 9535, The XML summary for the model Resgrid.Web.Services.Models.v4.UnitStatus.UnitStatusInput is incorrect: it mentions user/status and “current user” semantics though this DTO is unit-specific; update the <summary> for UnitStatusInput (and, if present, the property summaries for UnitStatusInput.Id and UnitStatusInput.Type) to describe that this object represents an input for setting a unit's status/action (e.g., "Input object for setting a unit's status/action; Id is the UnitId and Type is the UnitStateType"), remove any mention of "user" or "current user", and ensure the language matches the property names UnitStatusInput.Id and UnitStatusInput.Type.Web/Resgrid.Web.Services/Resgrid.Web.Services.xml-7590-7602 (1)
7590-7602:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix personnel-location summaries that still describe unit locations
These summaries describe “unit” objects while the type is
PersonnelLocation*, which can mislead API consumers.Suggested doc fix
- <member name="T:Resgrid.Web.Services.Models.v4.PersonnelLocation.PersonnelLocationResult"> + <member name="T:Resgrid.Web.Services.Models.v4.PersonnelLocation.PersonnelLocationResult"> <summary> - A unit location in the Resgrid system + A personnel location in the Resgrid system </summary> </member> @@ - <member name="T:Resgrid.Web.Services.Models.v4.PersonnelLocation.PersonnelLocationResultData"> + <member name="T:Resgrid.Web.Services.Models.v4.PersonnelLocation.PersonnelLocationResultData"> <summary> - The information about a specific unit's location + The information about a specific person's location </summary> </member>🤖 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/Resgrid.Web.Services/Resgrid.Web.Services.xml` around lines 7590 - 7602, The XML doc summaries still refer to "unit" locations; update the summaries for the members/types named PersonnelLocationResult.Data (member name="P:Resgrid.Web.Services.Models.v4.PersonnelLocation.PersonnelLocationResult.Data") and PersonnelLocationResultData (member name="T:Resgrid.Web.Services.Models.v4.PersonnelLocation.PersonnelLocationResultData") to reference "personnel" instead—e.g., change "A unit location in the Resgrid system" to "A personnel location in the Resgrid system", change "Response Data" to something like "Personnel response data" if more clarity is needed, and change "The information about a specific unit's location" to "The information about a specific personnel's location" (or "personnel's location") so the XML accurately describes PersonnelLocation* types..opencode/skills/configuration/SKILL.md-49-57 (1)
49-57:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winChange JSON fence labels to
jsoncor remove comments from copied examples.JSON does not allow comments per RFC 8259, so the
//comment in this example makes it invalid JSON. Users who copy-paste this directly will encounter parse errors. Switch the fence label tojsonc(JSON with Comments) to signal that the example includes comments, or remove the comments from the snippet.This applies to lines 49-57 and 158-171.
🤖 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 @.opencode/skills/configuration/SKILL.md around lines 49 - 57, The fenced JSON examples that include line comments (the snippet starting with "// appsettings.json" showing the "Database" object with ConnectionString/MaxRetryCount/CommandTimeoutSeconds) are invalid JSON; update those code fences to use the "jsonc" label or remove the leading "//" comment and any inline comments so the snippets are valid JSON, and apply the same fix to the second similar snippet further down (the other appsettings.json example) to avoid parse errors when users copy-paste..opencode/skills/configuration/SKILL.md-20-20 (1)
20-20:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winCorrect configuration provider precedence order.
The documented order places user secrets last (highest override priority), but in ASP.NET Core's default configuration chain, environment variables override user secrets, not the reverse. Per Microsoft's official documentation, the precedence is: appsettings.json → appsettings.{Environment}.json → user secrets → environment variables → command-line arguments (highest priority). Update the order or explicitly document if this is custom configuration.
🤖 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 @.opencode/skills/configuration/SKILL.md at line 20, The documented configuration precedence in SKILL.md under the "Configuration layering" section is incorrect; update the line to either show the correct ASP.NET Core default order (appsettings.json → appsettings.{Environment}.json → user secrets → environment variables → command-line arguments) or explicitly state this project uses a custom ordering; edit the "Configuration layering" sentence in SKILL.md to reflect the correct precedence (or call out that environment variables override user secrets if this repo deviates)..opencode/skills/de-sloppify/SKILL.md-52-54 (1)
52-54:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd code-fence languages to pass markdown lint.
The fenced blocks starting at Lines 52, 66, 71, 102, 143, 197, 210, 226, 239, and 253 need explicit language tags to comply with MD040.
Also applies to: 66-68, 71-83, 102-105, 143-145, 197-206, 210-222, 226-235, 239-249, 253-262
🤖 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 @.opencode/skills/de-sloppify/SKILL.md around lines 52 - 54, Several fenced code blocks (for example the block beginning with "MCP: get_diagnostics(scope: "solution", severityFilter: "warning")") are missing explicit language tags and fail MD040; update each triple-backtick fence in SKILL.md for the shown examples (the blocks starting at the occurrences in the review such as the MCP examples and the other fenced snippets) to include an appropriate language identifier (e.g., "mcp", "bash", or "json" as applicable) right after the opening ```, ensuring every fenced block has a language tag so markdown lint passes..opencode/skills/vertical-slice/SKILL.md-25-47 (1)
25-47:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd language identifiers to fenced code blocks.
The blocks starting at Line 25 and Line 187 are missing fence languages, which triggers MD040.
Also applies to: 187-205
🤖 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 @.opencode/skills/vertical-slice/SKILL.md around lines 25 - 47, The markdown has fenced code blocks that lack language identifiers (the project tree block starting with "src/" and the second similar block later that lists MyApp.Api/ Features/ Orders/ etc.), which triggers MD040; fix by adding a fence language (e.g., ```text or ```bash) to each of those code fences so the blocks are explicitly labeled (update the two code fences that contain the file tree listing)..opencode/skills/project-structure/SKILL.md-26-46 (1)
26-46:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winSpecify languages for the fenced code blocks.
The code fences starting at Line 26 and Line 181 are missing language identifiers (MD040).
Also applies to: 181-192
🤖 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 @.opencode/skills/project-structure/SKILL.md around lines 26 - 46, The fenced code blocks containing the ASCII project tree and the later sample block are missing language identifiers; update each opening fence from ``` to a specific language (e.g., ```text for the directory tree and ```bash or ```text for the later command/sample block) so the Markdown linter MD040 is satisfied and syntax highlighting is applied..opencode/skills/security-scan/SKILL.md-61-79 (1)
61-79:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winSeveral fenced blocks are missing language specifiers.
Please add a language (for example
text,bash,csharp,markdown) to the fences starting at Lines 61, 97, 132, 235, 310, 328, 342, and 354 to satisfy MD040.Also applies to: 97-127, 132-154, 235-253, 310-324, 328-338, 342-350, 354-365
🤖 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 @.opencode/skills/security-scan/SKILL.md around lines 61 - 79, The fenced code blocks in .opencode/skills/security-scan/SKILL.md that show the patterns (the triple-backtick blocks containing the "HIGH-CONFIDENCE PATTERNS..." and the other pattern lists) are missing language specifiers, triggering MD040; update each opening fence to include an appropriate language tag (for example `text`, `bash`, `csharp`, or `markdown`) for the blocks starting at the indicated locations so they validate as fenced code blocks (e.g., replace ``` with ```text or ```markdown for those pattern lists) and ensure the same change is applied to the other listed ranges.Providers/Resgrid.Providers.Weather/NwsWeatherAlertProvider.cs-418-423 (1)
418-423:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix duplicate condition logic in GetCodeFormatHint.
Lines 418-419 and 422-423 both check
upper.Length == 5 && int.TryParse(upper, out _), making the FIPS code branch unreachable. ZIP codes and FIPS codes both have 5 numeric digits, so they cannot be distinguished by this logic alone.🐛 Proposed fix to deduplicate or combine conditions
Option 1: Combine ZIP and FIPS into a single hint (simpler):
// US ZIP code (5 digits) if (upper.Length == 5 && int.TryParse(upper, out _)) - return $"'{code}' looks like a ZIP code. NWS uses zone codes, not ZIP codes. Find your zone at https://www.weather.gov/gis/."; - - // FIPS county code (state FIPS + county FIPS, typically 5 digits) - if (upper.Length == 5 && int.TryParse(upper, out _)) - return $"'{code}' looks like a FIPS code. NWS zone codes use the format MIC081 (state + C/Z + 3 digits)."; + return $"'{code}' looks like a ZIP or FIPS code. NWS uses zone codes (e.g. MIC081). Find your zone at https://www.weather.gov/gis/.";Option 2: Remove the unreachable FIPS check entirely if ZIP hint is sufficient.
🤖 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 `@Providers/Resgrid.Providers.Weather/NwsWeatherAlertProvider.cs` around lines 418 - 423, GetCodeFormatHint currently has two identical checks on upper.Length == 5 && int.TryParse(upper, out _) so the second (FIPS) branch is unreachable; update GetCodeFormatHint (referencing the variables upper and code) to handle 5-digit numeric codes with a single branch that returns a combined hint mentioning both ZIP and FIPS ambiguity and guiding the user to NWS zone codes (or delete the redundant FIPS branch and keep a single ZIP/FIPS hint), ensuring only one check for a 5-digit numeric input exists..opencode/skills/docker/SKILL.md-55-56 (1)
55-56:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix the HEALTHCHECK command syntax.
The current
HEALTHCHECKinstruction attempts to run the application with--urls, which would start the entire app rather than perform a quick health probe. The standard pattern is to usecurl,wget, or a dedicated health-check tool.🏥 Proposed fix for HEALTHCHECK
-HEALTHCHECK --interval=30s --timeout=3s --start-period=10s --retries=3 \ - CMD ["dotnet", "MyApp.Api.dll", "--urls", "http://localhost:8080/health/live"] +HEALTHCHECK --interval=30s --timeout=3s --start-period=10s --retries=3 \ + CMD curl --fail http://localhost:8080/health/live || exit 1Or if you want to avoid adding curl to the image:
-HEALTHCHECK --interval=30s --timeout=3s --start-period=10s --retries=3 \ - CMD ["dotnet", "MyApp.Api.dll", "--urls", "http://localhost:8080/health/live"] +HEALTHCHECK --interval=30s --timeout=3s --start-period=10s --retries=3 \ + CMD wget --no-verbose --tries=1 --spider http://localhost:8080/health/live || exit 1🤖 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 @.opencode/skills/docker/SKILL.md around lines 55 - 56, The HEALTHCHECK currently runs the app binary via CMD ["dotnet", "MyApp.Api.dll", "--urls", ...] which launches the server instead of probing it; change the HEALTHCHECK instruction to invoke a lightweight probe (e.g., curl or wget against http://localhost:8080/health/live) rather than starting the process. Update the HEALTHCHECK line that references CMD and "dotnet MyApp.Api.dll" to call a network probe (or use the shell form to run curl/wget) and ensure the base image includes the chosen tool (or use a no-extra-deps approach described in the comment) so the healthcheck performs a quick HTTP GET and returns proper exit codes..opencode/skills/wrap-up-ritual/SKILL.md-72-72 (1)
72-72:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd language identifiers to fenced code blocks to satisfy MD040.
Line 72, Line 98, Line 121, Line 136, Line 156, and Line 172 use unlabeled triple-backtick blocks. Please annotate each fence (e.g.,
text,markdown,bash) so markdownlint passes consistently.Also applies to: 98-98, 121-121, 136-136, 156-156, 172-172
🤖 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 @.opencode/skills/wrap-up-ritual/SKILL.md at line 72, Add explicit language identifiers to the unlabeled fenced code blocks in SKILL.md so markdownlint rule MD040 is satisfied: update the triple-backtick fences at the blocks starting on the locations referenced (the unlabeled fences at lines 72, 98, 121, 136, 156, and 172) by replacing each opening ``` with a suitable language tag such as ```text, ```markdown, or ```bash according to the block contents; ensure the closing ``` remains unchanged so each fenced block is properly labeled and linting will pass..opencode/skills/architecture-advisor/SKILL.md-100-100 (1)
100-100:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winNormalize fenced code block languages across architecture examples.
Line 100, Line 117, Line 132, Line 147, Line 175, Line 190, Line 208, and Line 226 are unlabeled fenced blocks. Please add explicit language tags (
text,csharp, etc.) for markdownlint MD040 compliance.Also applies to: 117-117, 132-132, 147-147, 175-175, 190-190, 208-208, 226-226
🤖 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 @.opencode/skills/architecture-advisor/SKILL.md at line 100, Several fenced code blocks in the architecture examples are missing language tags; locate the unlabeled triple-backtick blocks in SKILL.md's architecture examples (the examples with multiple consecutive fenced blocks) and add explicit language identifiers (e.g., text, csharp, javascript) to each opening ``` so they comply with markdownlint MD040 while keeping the code semantics unchanged; update every unlabeled fenced block noted in the review (the examples around the architecture sections) to use the appropriate language tag..opencode/skills/opentelemetry/SKILL.md-57-57 (1)
57-57:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winSpecify fence languages for env-var code blocks.
Line 57 and Line 171 start fenced blocks without a language tag. Add a language (likely
bashortext) to avoid MD040 lint failures.Also applies to: 171-171
🤖 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 @.opencode/skills/opentelemetry/SKILL.md at line 57, Two fenced code blocks in SKILL.md that show env-var examples are missing a language tag (they start with just ```); update those blocks to use a language specifier such as ```bash or ```text so the markdown linter (MD040) won't flag them. Locate the two env-var example fenced blocks (the ones beginning with ``` and containing environment variable snippets) and change the opening fence to include the chosen language for both occurrences..opencode/skills/clean-architecture/SKILL.md-26-26 (1)
26-26:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winLabel the project-layout fence with a language.
Line 26 starts an unlabeled fenced block; add a language identifier (
textis sufficient) to clear MD040.🤖 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 @.opencode/skills/clean-architecture/SKILL.md at line 26, The fenced code block that begins the "project-layout" example in SKILL.md is missing a language label, triggering MD040; update the opening fence (the ``` line) to include a language identifier (e.g., ```text) so the block is labeled and the linter warning is resolved..opencode/skills/convention-learner/SKILL.md-28-28 (1)
28-28:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd language tags to all unlabeled fenced blocks in this skill doc.
Line 28, 38, 58, 124, 146, 155, 173, 179, 187, 193, 201, 207, 215, and 220 should include a fence language (
text,markdown,ini, etc.) to avoid repeated MD040 lint warnings.Also applies to: 38-38, 58-58, 124-124, 146-146, 155-155, 173-173, 179-179, 187-187, 193-193, 201-201, 207-207, 215-215, 220-220
🤖 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 @.opencode/skills/convention-learner/SKILL.md at line 28, Several fenced code blocks in SKILL.md are missing language tags causing MD040 lint warnings; update each unlabeled triple-backtick block (the ones mentioned around the noted locations) to include an appropriate language identifier (e.g., ```text, ```markdown, ```ini) based on the block content so the markdown linter can parse them. Locate the unlabeled fences in SKILL.md and replace the opening ``` with a language-tagged fence for each occurrence. Ensure the chosen tag matches the block content (plain text vs markdown vs ini) and that all previously unlabeled blocks are updated consistently..opencode/skills/self-correction-loop/SKILL.md-32-32 (1)
32-32:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winSpecify fenced code block languages
At Line 32 and the other flagged fences, please add language identifiers on opening backticks to resolve MD040 and keep docs lint-clean.
Also applies to: 86-86, 105-105, 122-122, 137-137, 147-147, 159-159
🤖 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 @.opencode/skills/self-correction-loop/SKILL.md at line 32, Add language identifiers to the fenced code blocks in SKILL.md so the Markdown linter (MD040) stops flagging them: for each opening triple-backtick at the flagged locations (the code fences around the examples near the self-correction loop documentation), replace ``` with ```bash, ```json, ```js, or the appropriate language for that block's content so each fenced block has a language specifier..opencode/skills/instinct-system/SKILL.md-33-33 (1)
33-33:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix MD040 by annotating code fences with languages
At Line 33 and the other listed lines, add explicit fence languages (
text/markdown/csharpas appropriate) to remove markdownlint warnings.Also applies to: 82-82, 107-107, 121-121, 145-145, 180-180, 193-193, 210-210, 222-222, 235-235
🤖 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 @.opencode/skills/instinct-system/SKILL.md at line 33, The markdown file contains unlabeled code fences causing MD040 warnings; edit the SKILL.md code fences at the specified locations (including the fence at line 33 and also 82, 107, 121, 145, 180, 193, 210, 222, 235) and add explicit language identifiers (e.g., ```text, ```markdown, ```csharp as appropriate for the block content) so each triple-backtick fence is annotated with the correct language..opencode/skills/project-setup/SKILL.md-86-86 (1)
86-86:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd missing fenced-code language specifiers
At Line 86 and the other flagged fences, annotate each opening fence with a language to satisfy MD040.
Also applies to: 92-92, 98-98, 104-104, 110-110, 119-119, 154-154, 159-159, 186-186, 191-191
🤖 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 @.opencode/skills/project-setup/SKILL.md at line 86, Several fenced code blocks in SKILL.md are missing language specifiers (they currently use only "```"); update each opening fence (the "```" markers at the flagged locations) to include an appropriate language identifier (for example ```bash, ```yaml, ```json, ```text, etc.) that matches the block contents to satisfy MD040. Locate the unlabeled fences (the opening "```" tokens) and replace them with the correct fenced-code language specifier for each block so the linter can detect the language..opencode/skills/learning-log/SKILL.md-53-53 (1)
53-53:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winResolve MD040 warnings by labeling code fences
At Line 53 and the other listed fences, include a language token on each opening backtick fence to clear lint warnings.
Also applies to: 83-83, 96-96, 114-114, 138-138, 154-154, 168-168, 183-183
🤖 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 @.opencode/skills/learning-log/SKILL.md at line 53, Several fenced code blocks in SKILL.md (notably the opening ``` fences at the positions referenced) lack a language token, causing MD040 lint warnings; update each opening triple-backtick fence (the ``` lines at the reported locations such as the one at Line 53 and the others) to include an appropriate language identifier (e.g., ```bash, ```js, ```md, or ```yaml) matching the block content so each code fence is labeled and the MD040 warnings are resolved..opencode/skills/code-review-workflow/SKILL.md-35-35 (1)
35-35:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd language tags to fenced code blocks to satisfy MD040
At Line 35 and the other listed fences, add an explicit language (for example
text,markdown,bash,csharp) after each opening triple backtick so markdownlint warnings are cleared.Suggested pattern
-``` +```text ... -``` +```Also applies to: 48-48, 54-54, 61-61, 142-142, 149-149, 161-161, 167-167, 176-176, 183-183, 192-192, 198-198, 209-209, 214-214
🤖 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 @.opencode/skills/code-review-workflow/SKILL.md at line 35, The fenced code blocks in SKILL.md are missing explicit language tags (triggering MD040); update each opening triple-backtick fence (the backtick blocks surrounding example snippets such as the ones at the listed locations) to include an appropriate language identifier (e.g., text, markdown, bash, csharp) immediately after the ``` delimiter so the linter recognizes the block language; ensure each changed fence matches its content (use ```text for plain text, ```markdown for markup, ```bash for shell snippets, etc.) and apply consistently across all affected fences mentioned in the review..opencode/skills/session-management/SKILL.md-33-65 (1)
33-65:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd fence languages to resolve markdownlint MD040 warnings.
Multiple fenced blocks are missing language identifiers. This is easy to fix and removes current lint warnings.
Example fix pattern
-``` +```text STEP 1: Load Handoff → Check for .claude/handoff.md ... -``` +```Apply the same pattern to each warning location (use
textfor procedural blocks, keepmarkdownwhere already appropriate like the handoff template).Also applies to: 71-101, 107-123, 129-140, 180-191, 197-208, 214-231, 237-250, 254-265, 269-282, 286-297, 301-311, 315-331
🤖 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 @.opencode/skills/session-management/SKILL.md around lines 33 - 65, The fenced code blocks in the SKILL.md procedural sections (e.g., the blocks under "STEP 1: Load Handoff", "STEP 2: Load Memory", "STEP 3: Load Instincts", "STEP 4: Detect .NET Solution", "STEP 5: Present Summary" and the other listed sections) are missing fence language tags and causing markdownlint MD040 warnings; update each triple-backtick fence by adding an explicit language identifier—use "text" for procedural/plain blocks and preserve "markdown" where the block contains markdown-specific content (such as the handoff template). Apply this pattern consistently to the other warned ranges (the additional sections you listed) so every fenced block has a language label..opencode/skills/dependency-injection/SKILL.md-18-18 (1)
18-18:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRelax "interfaces only" guidance to avoid invalidating legitimate concrete registrations.
"Register interfaces, resolve interfaces ... not the concrete type" is too absolute. Microsoft's official ASP.NET Core DI documentation presents interface-based registration as the recommended pattern but does not require it exclusively—concrete-type registrations are explicitly supported and valid in legitimate scenarios (app services, framework integrations, decorators/factories). Rephrase as a preference, not a rule.
Suggested doc tweak
-3. **Register interfaces, resolve interfaces** — Register `services.AddScoped<IOrderService, OrderService>()`, not the concrete type. +3. **Prefer interface-based registrations for app boundaries** — `services.AddScoped<IOrderService, OrderService>()` is usually best for substitution/testing, while concrete registrations are still valid where appropriate.🤖 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 @.opencode/skills/dependency-injection/SKILL.md at line 18, The line "Register interfaces, resolve interfaces — Register `services.AddScoped<IOrderService, OrderService>()`, not the concrete type." is too absolute; change it to recommend preferring interface-based registrations (e.g., using `services.AddScoped<IOrderService, OrderService>()`) while acknowledging that registering concrete types is supported and sometimes appropriate (application services, framework integrations, decorators/factories, factories that require concrete types). Rephrase the sentence to present interface registration as the preferred pattern rather than a strict rule and add a short parenthetical note listing those legitimate concrete-registration scenarios.
🧹 Nitpick comments (4)
.opencode/skills/ci-cd/SKILL.md (1)
29-33: ⚡ Quick winMake branch triggers copy-safe across repos.
On Line 31/33 and Line 138, the examples hardcode
main. Since teams use different defaults (master,develop, etc.), these snippets can silently not trigger when copied as-is. Consider using placeholders (e.g.,<default-branch>) or adding an inline note to replace branch names.Suggested doc tweak
on: push: - branches: [main] + branches: [<default-branch>] pull_request: - branches: [main] + branches: [<default-branch>]trigger: branches: - include: [main] + include: [<default-branch>]Also applies to: 136-139
🤖 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 @.opencode/skills/ci-cd/SKILL.md around lines 29 - 33, The CI trigger examples currently hardcode the branch name "main" in the YAML keys (the on: push: branches: [main] and pull_request: branches: [main] examples and the similar snippet at Line 138); change those hardcoded values to a clear placeholder such as "<default-branch>" (or show both "<default-branch> / replace-me" style) and add a one-line inline note beside the examples instructing readers to replace the placeholder with their repository's default branch (e.g., master/develop) so the examples remain copy-safe across repos.Web/Resgrid.Web.Services/Controllers/v4/WeatherAlertsController.cs (1)
199-199: ⚡ Quick winConsider using a named constant instead of magic number
0for NWS source type.The condition
input.SourceType == 0uses a magic number. IfWeatherAlertSourceTypeenum exists (referenced in the provider asWeatherAlertSourceType.NationalWeatherService), consider using it here for clarity.♻️ Proposed refactor using enum
// Validate NWS zone codes at save time so users get immediate feedback // instead of a cryptic 400 from the NWS API during polling. - if (input.SourceType == 0 && !string.IsNullOrWhiteSpace(source.AreaFilter)) + if (input.SourceType == (int)WeatherAlertSourceType.NationalWeatherService + && !string.IsNullOrWhiteSpace(source.AreaFilter)) {This assumes
WeatherAlertSourceTypeis accessible in this controller's namespace. If not, a named constant would still be clearer than0.🤖 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/Resgrid.Web.Services/Controllers/v4/WeatherAlertsController.cs` at line 199, Replace the magic number check on input.SourceType with the named enum value to improve clarity: change the condition in WeatherAlertsController that currently reads "input.SourceType == 0" to use WeatherAlertSourceType.NationalWeatherService (or the appropriate enum/constant) so it becomes "input.SourceType == WeatherAlertSourceType.NationalWeatherService" and keep the existing AreaFilter check (source.AreaFilter) intact; ensure the WeatherAlertSourceType symbol is imported or fully-qualified if needed.Providers/Resgrid.Providers.Weather/NwsWeatherAlertProvider.cs (1)
58-63: ⚡ Quick winConsider logging validation failures per coding guidelines.
The coding guidelines specify using
Resgrid.Framework.Loggingfor all logging throughout the codebase. When validation fails and an exception is thrown, consider logging the validation error with department context before throwing, as this would help diagnose configuration issues.📝 Proposed addition of validation logging
// Validate zone codes before calling the NWS API to produce a clear error // instead of a cryptic 400 Bad Request from the upstream API. var validationError = GetZoneValidationError(source.AreaFilter); if (validationError != null) + { + Resgrid.Framework.Logging.LogError( + $"NWS zone validation failed for department {source.DepartmentId}: {validationError}"); throw new HttpRequestException( $"Invalid NWS zone code in area filter for department {source.DepartmentId}: {validationError}"); + }As per coding guidelines, use
Resgrid.Framework.Loggingstatic methods (LogException,LogError,LogInfo,LogDebug) for all logging throughout the codebase.🤖 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 `@Providers/Resgrid.Providers.Weather/NwsWeatherAlertProvider.cs` around lines 58 - 63, Before throwing the HttpRequestException when GetZoneValidationError(source.AreaFilter) returns a non-null validationError, log the failure using Resgrid.Framework.Logging (e.g., LogError) including the department context and the validationError; locate the validation check around GetZoneValidationError, source.AreaFilter and source.DepartmentId and add a LogError call that records the department id and validation message immediately before throwing the exception..opencode/skills/scalar/SKILL.md (1)
57-59: 💤 Low valueConsider normalizing "prefill" vs "pre-fill" usage.
The heading uses "Prefill" (line 57) while the paragraph uses "Pre-fill" (line 59). For consistency, consider using one variant throughout the document.
🤖 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 @.opencode/skills/scalar/SKILL.md around lines 57 - 59, The document uses both "Prefill" and "Pre-fill"; pick one spelling and make it consistent (e.g., change the heading "Authentication Prefill (Development Only)" and the sentence "Pre-fill credentials so developers..." to the chosen variant). Update all occurrences in this file (search for "Prefill" and "Pre-fill") so the heading and paragraph wording match exactly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b271ed99-e239-4fcb-b3e8-d75b050fc969
📒 Files selected for processing (58)
.gitignore.opencode/skills/80-20-review/SKILL.md.opencode/skills/api-versioning/SKILL.md.opencode/skills/architecture-advisor/SKILL.md.opencode/skills/aspire/SKILL.md.opencode/skills/authentication/SKILL.md.opencode/skills/autonomous-loops/SKILL.md.opencode/skills/caching/SKILL.md.opencode/skills/ci-cd/SKILL.md.opencode/skills/clean-architecture/SKILL.md.opencode/skills/code-review-workflow/SKILL.md.opencode/skills/configuration/SKILL.md.opencode/skills/container-publish/SKILL.md.opencode/skills/context-discipline/SKILL.md.opencode/skills/convention-learner/SKILL.md.opencode/skills/ddd/SKILL.md.opencode/skills/de-sloppify/SKILL.md.opencode/skills/dependency-injection/SKILL.md.opencode/skills/docker/SKILL.md.opencode/skills/ef-core/SKILL.md.opencode/skills/error-handling/SKILL.md.opencode/skills/health-check/SKILL.md.opencode/skills/httpclient-factory/SKILL.md.opencode/skills/instinct-system/SKILL.md.opencode/skills/learning-log/SKILL.md.opencode/skills/logging/SKILL.md.opencode/skills/messaging/SKILL.md.opencode/skills/migration-workflow/SKILL.md.opencode/skills/minimal-api/SKILL.md.opencode/skills/model-selection/SKILL.md.opencode/skills/modern-csharp/SKILL.md.opencode/skills/openapi/SKILL.md.opencode/skills/opentelemetry/SKILL.md.opencode/skills/project-setup/SKILL.md.opencode/skills/project-structure/SKILL.md.opencode/skills/resilience/SKILL.md.opencode/skills/scaffolding/SKILL.md.opencode/skills/scalar/SKILL.md.opencode/skills/security-scan/SKILL.md.opencode/skills/self-correction-loop/SKILL.md.opencode/skills/serilog/SKILL.md.opencode/skills/session-management/SKILL.md.opencode/skills/split-memory/SKILL.md.opencode/skills/testing/SKILL.md.opencode/skills/verification-loop/SKILL.md.opencode/skills/vertical-slice/SKILL.md.opencode/skills/workflow-mastery/SKILL.md.opencode/skills/wrap-up-ritual/SKILL.mdCore/Resgrid.Model/Autofill.csProviders/Resgrid.Providers.Weather/NwsWeatherAlertProvider.csRepositories/Resgrid.Repositories.DataRepository/Servers/PostgreSql/PostgreSqlConfiguration.csRepositories/Resgrid.Repositories.DataRepository/Servers/SqlServer/SqlServerConfiguration.csWeb/Resgrid.Web.Services/Controllers/v4/WeatherAlertsController.csWeb/Resgrid.Web.Services/Resgrid.Web.Services.xmlWeb/Resgrid.Web/Areas/User/Controllers/CustomStatusesController.csWeb/Resgrid.Web/Areas/User/Controllers/DispatchController.csWeb/Resgrid.Web/Areas/User/Models/Templates/NewCallNoteModel.csWeb/Resgrid.Web/Areas/User/Views/Dispatch/UpdateCall.cshtml
| ## Core Principles | ||
|
|
||
| 1. **Constructor injection is the default** — Inject dependencies through the constructor (primary constructors make this clean). No service locator, no property injection. | ||
| 2. **Match lifetimes carefully** — A singleton must never depend on a scoped or transient service. This is the most common DI bug. |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Microsoft.Extensions.DependencyInjection guidance, is injecting transient services into singleton services always invalid, or only risky in specific cases? Please include official Microsoft docs links.
💡 Result:
Injecting transient services into singleton services is not always invalid in Microsoft.Extensions.DependencyInjection; it is allowed and only risky in specific cases, primarily when the transient service is stateful, not thread-safe, or implements IDisposable in ways that could lead to issues.[1][2] Official Microsoft documentation explicitly acknowledges this pattern without prohibition: "If a singleton service has a dependency on a transient service, the transient service might also require thread safety depending on how it's used by the singleton."[1][3] Singletons must be thread-safe overall,[2] and transients are lightweight/stateless by design, so stateless transients work fine as the single instance captured by the singleton lives for the app lifetime without issues.[4][5] Risks arise if the transient holds mutable state (accessed concurrently via the singleton), lacks thread-safety, or if resolved from root scope causing delayed disposal.[1] Microsoft recommends factories or IServiceScopeFactory for fresh instances when needed, but direct injection is valid if safe.[1][6]
Citations:
- 1: https://learn.microsoft.com/en-us/dotnet/core/extensions/dependency-injection-guidelines
- 2: https://learn.microsoft.com/en-us/dotnet/core/extensions/dependency-injection/service-lifetimes
- 3: https://learn.microsoft.com/en-us/dotnet/core/extensions/dependency-injection/guidelines
- 4: https://stackoverflow.com/questions/56289997/what-is-wrong-when-transient-injected-to-singleton
- 5: https://dapiq.com/insights/dependency-injection-anti-patterns-aspnet-core
- 6: https://stackoverflow.com/questions/68122109/using-transient-service-within-a-singleton-service
Correct lifetime rule: singleton→scoped is invalid, singleton→transient is not universally invalid.
The current wording is overly strict and technically inaccurate for .NET DI. Singleton services may depend on transient services if the transient is thread-safe and stateless; the risk only materializes when the transient holds mutable state or lacks thread-safety. Narrow this to "singleton must not capture scoped services" and add a caution for transient services that are stateful or not thread-safe.
Suggested fix
-2. **Match lifetimes carefully** — A singleton must never depend on a scoped or transient service. This is the most common DI bug.
+2. **Match lifetimes carefully** — A singleton must never depend on a scoped service. Transient dependencies in singletons are allowed if they are thread-safe and stateless; use factories or IServiceScopeFactory for fresh instances when transient state isolation is required.📝 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.
| 2. **Match lifetimes carefully** — A singleton must never depend on a scoped or transient service. This is the most common DI bug. | |
| 2. **Match lifetimes carefully** — A singleton must never depend on a scoped service. Transient dependencies in singletons are allowed if they are thread-safe and stateless; use factories or IServiceScopeFactory for fresh instances when transient state isolation is required. |
🤖 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 @.opencode/skills/dependency-injection/SKILL.md at line 17, Update the
lifetime guidance in the "Match lifetimes carefully" sentence so it no longer
forbids singleton→transient universally: change the rule to state that a
singleton must never depend on a scoped service and add a caution that
singletons may depend on transient services only if those transients are
stateless and thread-safe; adjust the sentence in SKILL.md that currently reads
"A singleton must never depend on a scoped or transient service" to the new
phrasing and add a short note warning about stateful or non‑thread‑safe
transients.
| var groups = typeof(Program).Assembly | ||
| .GetTypes() | ||
| .Where(t => t.IsAssignableTo(typeof(IEndpointGroup)) && !t.IsInterface && !t.IsAbstract) | ||
| .Select(Activator.CreateInstance) | ||
| .Cast<IEndpointGroup>(); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In ASP.NET Core minimal APIs, does Activator.CreateInstance support constructor injection for endpoint group classes, or should ActivatorUtilities.CreateInstance(app.Services, type) be used for DI-aware activation?
💡 Result:
In ASP.NET Core minimal APIs, endpoint group classes (custom classes used to organize endpoints, like those implementing a registration method) that have constructor dependencies require DI-aware activation. Activator.CreateInstance does not support constructor injection from the DI container, as it only uses parameterless constructors or provided arguments without resolving services [1][2]. Instead, use ActivatorUtilities.CreateInstance(app.Services, type) to resolve constructor dependencies from the IServiceProvider [3][4][5][1].
Citations:
- 1: https://stackoverflow.com/questions/52644507/using-activatorutilities-createinstance-to-create-instance-from-type
- 2: https://www.reddit.com/r/dotnet/comments/6r4ad6/is_it_good_to_use/
- 3: https://learn.microsoft.com/en-us/dotnet/core/compatibility/extensions/8.0/activatorutilities-createinstance-behavior
- 4: https://github.com/dotnet/aspnetcore/blob/main/src/Shared/ActivatorUtilities/ActivatorUtilities.cs
- 5: https://www.webdevtutor.net/blog/using-activation-utility-class-aspnet-core
Use DI-aware activation for endpoint-group discovery.
Activator.CreateInstance does not support constructor injection from the DI container. If any IEndpointGroup implementation requires constructor-injected services, endpoint registration will fail at startup. Use ActivatorUtilities.CreateInstance(app.Services, t) instead for proper service resolution.
Suggested doc snippet update
var groups = typeof(Program).Assembly
.GetTypes()
.Where(t => t.IsAssignableTo(typeof(IEndpointGroup)) && !t.IsInterface && !t.IsAbstract)
- .Select(Activator.CreateInstance)
+ .Select(t => ActivatorUtilities.CreateInstance(app.Services, t))
.Cast<IEndpointGroup>();📝 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 groups = typeof(Program).Assembly | |
| .GetTypes() | |
| .Where(t => t.IsAssignableTo(typeof(IEndpointGroup)) && !t.IsInterface && !t.IsAbstract) | |
| .Select(Activator.CreateInstance) | |
| .Cast<IEndpointGroup>(); | |
| var groups = typeof(Program).Assembly | |
| .GetTypes() | |
| .Where(t => t.IsAssignableTo(typeof(IEndpointGroup)) && !t.IsInterface && !t.IsAbstract) | |
| .Select(t => ActivatorUtilities.CreateInstance(app.Services, t)) | |
| .Cast<IEndpointGroup>(); |
🤖 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 @.opencode/skills/minimal-api/SKILL.md around lines 42 - 46, The discovery
code uses Activator.CreateInstance which bypasses the DI container; update the
activation to use DI-aware ActivatorUtilities.CreateInstance by replacing the
Activator.CreateInstance call in the var groups LINQ (the code that reflects
Program.Assembly and filters for IEndpointGroup) so that each type t is
instantiated via ActivatorUtilities.CreateInstance(app.Services, t) to allow
constructor injection for IEndpointGroup implementations.
| ### Extension Members (C# 14) | ||
|
|
||
| Extension members replace static extension method classes with a cleaner syntax. | ||
|
|
||
| ```csharp | ||
| // GOOD — extension members (C# 14) | ||
| public extension OrderExtensions for Order | ||
| { | ||
| public decimal TotalWithTax => Total * 1.2m; | ||
|
|
||
| public bool IsHighValue => Total > 1000m; | ||
|
|
||
| public string ToSummary() => $"Order #{Id}: {Total:C} ({Items.Count} items)"; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
What is the official, finalized C# 14 syntax for declaring extension members (including extension properties and methods), according to Microsoft Learn / .NET 10 documentation?
💡 Result:
Official finalized C# 14 (used with.NET 10+) syntax for declaring extension members is the “extension block” inside a non-generic, non-nested static class: you write extension followed by the receiver parameter in parentheses; inside the extension block you can declare extension methods and extension properties as normal members. 1) Extension members for an instance (extension properties + instance extension methods) - Extension block syntax (receiver has a name): extension(ReceiverType receiverName) { // instance extension property, method, etc. public PropertyType Name => /* use receiverName /; public ReturnType Method(/ optional params /) { / use receiverName / } } In the Microsoft Learn reference, the receiver type/name declared by the extension block is in scope in every instance member declared in that extension, and the extension members can be methods or properties. [1] 2) Static extension members (extension properties + static extension methods) - Extension block syntax (receiver type only; receiver parameter is unnamed): extension(ReceiverType) { // static extension property public static PropertyType Name => /... /; // static extension method public static ReturnType Method(/ params /) { /... */ } } Microsoft Learn shows a second extension block form “with a receiver type only” for static extension members, and provides examples of both a static extension method and a static extension property declared inside it. [2] 3) Where this syntax is allowed / constraints (finalized behavior) - Extension declarations are allowed only inside non-generic, non-nested static classes. [3] - The extension keyword introduces an extension block that specifies the receiver for the extension members. [1] Minimal end-to-end example (combines instance and static members) public static class EnumerableExtensions { // Instance extension members for IEnumerable extension(IEnumerable source) { // extension property public bool IsEmpty =>!source.Any; // extension method public IEnumerable Where(Func<TSource, bool> predicate) => System.Linq.Enumerable.Where(source, predicate); } // Static extension members for IEnumerable extension(IEnumerable) { public static IEnumerable Identity => System.Linq.Enumerable.Empty; public static IEnumerable Combine(IEnumerable first, IEnumerable second) => System.Linq.Enumerable.Concat(first, second); } } This matches the official Microsoft Learn examples: the first extension block declares an instance extension property and instance extension method, and the second declares a static extension method and a static extension property. [2] Notes on “finalized C# 14” basis - The Microsoft Learn “Extension member declarations” reference describes the extension block syntax for C# 14. [1] - The C# 14 extension members feature specifications document states the syntax is finalized. [3]
Citations:
- 1: https://learn.microsoft.com/en-us/dotnet/csharp/language-reference/keywords/extension
- 2: https://learn.microsoft.com/en-us/dotnet/csharp/whats-new/csharp-14
- 3: https://learn.microsoft.com/en-us/dotnet/csharp/language-reference/proposals/csharp-14.0/extensions
Fix extension members syntax to match C# 14 specification
The extension members syntax at lines 114–121 does not match the finalized C# 14 specification. The code public extension OrderExtensions for Order { ... } will not compile.
The correct syntax for C# 14 extension members places the extension keyword inside a static class with the receiver type in parentheses:
public static class OrderExtensions
{
extension(Order order)
{
public decimal TotalWithTax => order.Total * 1.2m;
public bool IsHighValue => order.Total > 1000m;
public string ToSummary() => $"Order #{order.Id}: {order.Total:C} ({order.Items.Count} items)";
}
}Align the code snippet with the official Microsoft Learn documentation for C# 14 extension members.
🤖 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 @.opencode/skills/modern-csharp/SKILL.md around lines 108 - 121, The
extension members example uses the invalid `public extension OrderExtensions for
Order` form; update the snippet to a static class named OrderExtensions
containing an extension block `extension(Order order)` and move the members
inside that block, changing references from implicit members (Total, Id, Items)
to use the receiver variable `order` (e.g., order.Total, order.Id, order.Items)
and keep the class public static OrderExtensions and member signatures as shown
in the Microsoft docs for C# 14.
| FOR JSON PATH) AS 'Signups' | ||
|
|
||
| FROM [dbo].[Shifts] sh | ||
| WHERE EXISTS (SELECT 1 FROM [dbo].[ShiftDays] sd WHERE sd.[ShiftId] = sh.[ShiftId] AND sd.[Day] >= CAST(GETDATE() AS DATE) AND sd.[Day] < DATEADD(DAY, 2, CAST(GETDATE() AS DATE))) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for usages of SelectShiftAndDaysJSONQuery to assess impact
rg -nC5 'SelectShiftAndDaysJSONQuery' --type=cs -g '!*Configuration.cs'Repository: Resgrid/Core
Length of output: 1449
🏁 Script executed:
# Find all usages of SelectShiftAndDaysQuery class
rg -nC3 'SelectShiftAndDaysQuery' --type=cs -g '!*Configuration.cs' | head -100Repository: Resgrid/Core
Length of output: 2122
🏁 Script executed:
# Check the SelectShiftAndDaysQuery.cs file to understand how it's used
cat -n Repositories/Resgrid.Repositories.DataRepository/Queries/Shifts/SelectShiftAndDaysQuery.csRepository: Resgrid/Core
Length of output: 1708
🏁 Script executed:
# Search for any configuration or settings related to shift filtering or the 2-day window
rg -n '2.*[Dd]ay|[Dd]ay.*2|shift.*filter|SelectShiftAndDays' --type=cs | grep -i 'config\|setting\|constant\|magic'Repository: Resgrid/Core
Length of output: 3365
🏁 Script executed:
# Look for recent comments or documentation about this query
rg -nB3 -A3 'SelectShiftAndDaysJSONQuery' Repositories/Resgrid.Repositories.DataRepository/Servers/SqlServer/SqlServerConfiguration.cs | head -50Repository: Resgrid/Core
Length of output: 428
🏁 Script executed:
# Find the method in ShiftsRepository that uses SelectShiftAndDaysQuery
rg -nB10 -A15 'GetQuery<SelectShiftAndDaysQuery>' Repositories/Resgrid.Repositories.DataRepository/ShiftsRepository.cs | head -80Repository: Resgrid/Core
Length of output: 937
🏁 Script executed:
# Check if there's any documentation or comments around this method
rg -nB25 'SelectShiftAndDaysQuery' Repositories/Resgrid.Repositories.DataRepository/ShiftsRepository.cs | head -100Repository: Resgrid/Core
Length of output: 834
Document the 2-day shift filter rationale and reconsider method naming.
The GetAllShiftAndDaysAsync() method in ShiftsRepository.cs uses SelectShiftAndDaysJSONQuery, which filters shifts to only those with ShiftDays within a 2-day window (>= CAST(GETDATE() AS DATE) and < DATEADD(DAY, 2, CAST(GETDATE() AS DATE))). This constraint is hardcoded identically in both SQL Server and PostgreSQL configurations.
Issues:
- The method name
GetAllShiftAndDaysAsync()suggests returning all shifts, but it returns only near-term shifts (next 2 days) - The 2-day window lacks documentation explaining the business or performance rationale
- No comments in the SQL configuration explain why this specific window was chosen
- The hardcoded value is inflexible and makes the filter intent unclear
Recommendations:
- Add a comment in the SQL configuration file explaining why the 2-day window is applied
- Consider renaming the method to
GetUpcomingShiftAndDaysAsync()or similar to clarify the filtering behavior - Evaluate whether the 2-day window should be configurable or if the current behavior is intentional
🤖 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
`@Repositories/Resgrid.Repositories.DataRepository/Servers/SqlServer/SqlServerConfiguration.cs`
at line 823, The SQL query SelectShiftAndDaysJSONQuery currently filters
ShiftDays to a hardcoded 2-day window (WHERE sd.[Day] >= CAST(GETDATE() AS DATE)
AND sd.[Day] < DATEADD(DAY, 2, CAST(GETDATE() AS DATE))) but the repository
method GetAllShiftAndDaysAsync() name implies “all” shifts; add a clear comment
above the SQL in SqlServerConfiguration.cs explaining the business/performance
rationale for the 2-day window, then rename GetAllShiftAndDaysAsync() to
GetUpcomingShiftAndDaysAsync() (and update all callers/tests) to reflect the
limited scope, and refactor the hardcoded window into a single configurable
value (constant or parameter) used in both SQL Server and PostgreSQL
configurations so the window can be adjusted centrally.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@Core/Resgrid.Model/Repositories/IShiftsRepository.cs`:
- Around line 26-31: The repository method GetUpcomingShiftAndDaysAsync
currently derives “upcoming” by using the DB/server clock; change its signature
to accept a reference time (e.g., DateTime referenceTime or DateTime
currentTime) and propagate that parameter through the query path (including
callers that call GetShiftsStartingNextDayAsync) so the repository filters use
the provided reference instead of DateTime.UtcNow/Now; update all
implementations of IShiftsRepository.GetUpcomingShiftAndDaysAsync and any
callers (notably places calling GetShiftsStartingNextDayAsync) to pass the
caller-computed currentTime/tomorrow window through to the repository query.
🪄 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: CHILL
Plan: Pro
Run ID: 63302995-31b7-445a-a62e-c943ab1653fd
📒 Files selected for processing (7)
Core/Resgrid.Model/Repositories/IShiftsRepository.csCore/Resgrid.Services/ShiftsService.csRepositories/Resgrid.Repositories.DataRepository/Configs/SqlConfiguration.csRepositories/Resgrid.Repositories.DataRepository/Queries/Shifts/SelectUpcomingShiftAndDaysQuery.csRepositories/Resgrid.Repositories.DataRepository/Servers/PostgreSql/PostgreSqlConfiguration.csRepositories/Resgrid.Repositories.DataRepository/Servers/SqlServer/SqlServerConfiguration.csRepositories/Resgrid.Repositories.DataRepository/ShiftsRepository.cs
| /// <summary> | ||
| /// Gets upcoming shifts (with days in the next 2 days) and their associated data asynchronous. | ||
| /// Filters at the database level to avoid loading the entire Shifts table. | ||
| /// </summary> | ||
| /// <returns>Task<IEnumerable<Shift>>.</returns> | ||
| Task<IEnumerable<Shift>> GetUpcomingShiftAndDaysAsync(); |
There was a problem hiding this comment.
Pass the reference time into this repository method.
GetShiftsStartingNextDayAsync(DateTime currentTime) still computes “tomorrow” from the caller’s currentTime, but this new contract has no way to carry that value through. The result set now depends on the database/server clock instead, so departments near time-zone/day boundaries can have valid next-day shifts filtered out before the service logic runs.
Consider taking a reference time or explicit date window here and pushing that through the query path instead of deriving “upcoming” inside the repository.
🤖 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 `@Core/Resgrid.Model/Repositories/IShiftsRepository.cs` around lines 26 - 31,
The repository method GetUpcomingShiftAndDaysAsync currently derives “upcoming”
by using the DB/server clock; change its signature to accept a reference time
(e.g., DateTime referenceTime or DateTime currentTime) and propagate that
parameter through the query path (including callers that call
GetShiftsStartingNextDayAsync) so the repository filters use the provided
reference instead of DateTime.UtcNow/Now; update all implementations of
IShiftsRepository.GetUpcomingShiftAndDaysAsync and any callers (notably places
calling GetShiftsStartingNextDayAsync) to pass the caller-computed
currentTime/tomorrow window through to the repository query.
|
Approve |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
2 participants
Summary by CodeRabbit
Bug Fixes
New Features
Chores / Documentation