Phase D.5 Facility Resource Readiness Center — Core Assets - #132
Merged
Conversation
Co-authored-by: Cursor <cursoragent@cursor.com>
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
|
🧙 Sourcery has finished reviewing your pull request! Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
📝 WalkthroughWalkthroughAdds a facility-scoped resource readiness center spanning domain entities, APIs, authorization, persistence, workspace widgets, a dedicated frontend page, imports, maintenance, readiness calculations, seed data, documentation, and automated validation. ChangesPhase D.5 resource contracts and persistence
Resource application and workspace flow
Frontend and validation
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant FacilityResourcesPage
participant ResourceApi
participant ResourceReadinessService
participant ResourceDatabase
User->>FacilityResourcesPage: Open facility resources route
FacilityResourcesPage->>ResourceApi: Request summary and scoped resource data
ResourceApi->>ResourceReadinessService: Execute authorized facility queries
ResourceReadinessService->>ResourceDatabase: Read assets, requirements, events, and maintenance
ResourceDatabase-->>ResourceReadinessService: Return scoped resource records
ResourceReadinessService-->>ResourceApi: Return readiness payload
ResourceApi-->>FacilityResourcesPage: Render summary, exceptions, units, and assets
User->>FacilityResourcesPage: Submit asset or import action
FacilityResourcesPage->>ResourceApi: Send create or preview/confirm request
ResourceApi->>ResourceReadinessService: Validate, persist, and audit operation
ResourceReadinessService->>ResourceDatabase: Save resource state and history
Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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 |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- In
ObservationWorkspacePagetest, consider replacingawait waitFor(() => expect(workspace).toHaveBeenCalled())with an assertion on the rendered UI state (e.g., presence of the card) to reduce coupling to the implementation detail of theworkspacemock. - The explicit
timeout: 5_000passed tofindByRolemay be better extracted into a shared test utility or constant so timeout behavior is consistent and easier to adjust across tests.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `ObservationWorkspacePage` test, consider replacing `await waitFor(() => expect(workspace).toHaveBeenCalled())` with an assertion on the rendered UI state (e.g., presence of the card) to reduce coupling to the implementation detail of the `workspace` mock.
- The explicit `timeout: 5_000` passed to `findByRole` may be better extracted into a shared test utility or constant so timeout behavior is consistent and easier to adjust across tests.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
henter36
changed the title
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
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)
src/backend/Baseera.Application/Workspaces/FacilityWorkspaceReadService.cs (1)
273-284: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDomain-quality insert order breaks when Occupancy is absent but Resources is present.
domains.Insert(6, ...)for Resources assumes the priorInsert(5, OccupancyDomain(...))always ran. If a user lacksOccupancyViewSummary(sometrics.Occupancyis null and the index-5 insert is skipped) but hasResourcesViewSummary, the Resources domain lands after the original "risks" slot instead of right after "forms"/before "incidents" — producing an inconsistent domain order for that permission combination.🐛 Proposed fix using a running insert index
- if (metrics.Occupancy is not null) - { - domains.Insert(5, OccupancyDomain(metrics.Occupancy)); - } - if (metrics.Resources is not null) - { - domains.Insert(6, ResourcesDomain(metrics.Resources)); - } - else - { - domains.Insert(6, MissingDomain("resources", "الموارد والجاهزية", "لا يملك المستخدم صلاحية عرض الموارد أو لم تُحمّل بيانات المجال.", "`#15`")); - } + var insertIndex = 5; + if (metrics.Occupancy is not null) + { + domains.Insert(insertIndex++, OccupancyDomain(metrics.Occupancy)); + } + domains.Insert( + insertIndex, + metrics.Resources is not null + ? ResourcesDomain(metrics.Resources) + : MissingDomain("resources", "الموارد والجاهزية", "لا يملك المستخدم صلاحية عرض الموارد أو لم تُحمّل بيانات المجال.", "`#15`"));🤖 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 `@src/backend/Baseera.Application/Workspaces/FacilityWorkspaceReadService.cs` around lines 273 - 284, Update the domain insertion logic in FacilityWorkspaceReadService so the Resources and missing-Resources branches use a running insertion index rather than hard-coded index 6, incrementing it only when the Occupancy domain is inserted. Preserve the intended order of forms, occupancy when available, resources, and incidents for all permission combinations, including when Occupancy is absent.src/frontend/src/pages/workspaces/FacilityWorkspacePage.tsx (1)
640-680: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftResource panels fall through to the generic "domain not implemented" gap panel.
PanelDetailhas no branch for the newvehicle/weapon/communication-device/equipmentpanel types produced byresourcePanelType,panelForPriorityItem, andpanelForActivityItem. They all fall through toDomainGapPanel, whose copy explicitly states no independent domain model exists — but Resources now has a full domain/API. Clicking a resource category rail, exception, priority item, or activity item opens this misleading "not implemented" panel instead of real resource details, even though a fullFacilityResourcesPageexists behind the "فتح الصفحة الكاملة" link.Separately, category-rail/exception fallback entityIds use
domain-resources-${resourceTypeCode}, butfindPanelSummary's domain lookup only matches on the exact domain key"resources", sosummaryresolves toundefinedfor these clicks — reinforcing the generic/empty panel.Consider adding a dedicated resource panel (fetching asset/exception detail similarly to
NotePanel/CorrectiveActionPanel) rather than reusingDomainGapPanelfor an implemented domain.Also applies to: 1108-1174, 1536-1566, 1649-1654
🤖 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 `@src/frontend/src/pages/workspaces/FacilityWorkspacePage.tsx` around lines 640 - 680, Update PanelDetail to handle vehicle, weapon, communication-device, and equipment panel types with a dedicated resource-detail panel instead of DomainGapPanel, reusing the established detail-fetching patterns from NotePanel and CorrectiveActionPanel. Update findPanelSummary and the resource panel creators resourcePanelType, panelForPriorityItem, and panelForActivityItem so domain-resources-${resourceTypeCode} entity IDs resolve against the resources domain key. Preserve the existing fallback only for genuinely unsupported panel types.
🟡 Minor comments (6)
src/backend/Baseera.Application/Resources/ResourceServices.cs-689-723 (1)
689-723: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winCategory readiness passes
StaleRecordswhere the policy expectsMissingDataRecords. Line 705 feeds stale counts intoResourceReadinessInputs.MissingDataRecords, conflating "not recently verified" with "incomplete data". The resultingDataCompletenessRatehappens to be discarded for categories today, so this is latent rather than user-visible — but it will silently produce wrong completeness the moment the field is surfaced.🤖 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 `@src/backend/Baseera.Application/Resources/ResourceServices.cs` around lines 689 - 723, The BuildCategory method incorrectly passes row.StaleRecords into the ResourceReadinessInputs.MissingDataRecords field. Update this argument to use the corresponding MissingDataRecords value from ResourceCategoryCounts, preserving StaleRecords for freshness status and confidence calculations.src/backend/Baseera.Application/Resources/ResourceServices.cs-264-279 (1)
264-279: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Gapis hard-coded to0for every unit. The DTO advertises a per-unit gap and the frontend renders it, butResourceRequirement.FacilityUnitIdis never consulted here, so the UI will always show zero unit-level shortfall — indistinguishable from "fully covered". Either compute it from unit-scoped requirements or drop the field from the payload until it's implemented.🤖 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 `@src/backend/Baseera.Application/Resources/ResourceServices.cs` around lines 264 - 279, The ResourceUnitDistributionDto mapping currently hard-codes Gap to zero and ignores ResourceRequirement.FacilityUnitId. Update the surrounding resource distribution method to calculate each unit’s gap from requirements scoped to row.OperationalFacilityUnitId, or remove Gap from the DTO payload until that calculation is implemented; do not continue sending a misleading zero value.src/backend/Baseera.Application/Resources/ResourceServices.cs-528-559 (1)
528-559: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winBackdated placements can produce inverted intervals. Active placements are closed with
EffectiveToUtc = request.EffectiveFromUtcwithout checking that the newEffectiveFromUtcis at or after the existingEffectiveFromUtc, and the new row'sEffectiveToUtcis never validated against its ownEffectiveFromUtc. Both cases persist history whereEffectiveToUtc < EffectiveFromUtc, which breaks any "active placement at time T" query.🤖 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 `@src/backend/Baseera.Application/Resources/ResourceServices.cs` around lines 528 - 559, The PlaceAssetAsync method must reject or otherwise prevent inverted placement intervals. Before closing active placements, validate that request.EffectiveFromUtc is not earlier than each existing placement’s EffectiveFromUtc, and validate the new placement’s EffectiveToUtc is null or at least request.EffectiveFromUtc; preserve the existing placement update and creation flow only for valid intervals.src/backend/Baseera.Infrastructure/Persistence/DatabaseInitializer.cs-930-938 (1)
930-938: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winDemo resource seeding assumes
FacilityA1units exist. The guard only checks the facility row and the absence of resource assets, yet the seeds hard-referenceSeedIds.FacilityA1UnitNorth/South/Medical.EnsureDemoOccupancyAsynconly inserts those units when no units exist for the facility, so a database with the facility but different units will hit an FK violation onSaveChangesAsyncand fail startup seeding. Consider verifying unit presence (or resolving unit ids from the DB) before seeding placements/profiles.Also applies to: 944-979
🤖 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 `@src/backend/Baseera.Infrastructure/Persistence/DatabaseInitializer.cs` around lines 930 - 938, Update the guard in the demo resource seeding flow around EnsureDemoOccupancyAsync to verify that the required FacilityA1 units—SeedIds.FacilityA1UnitNorth, SeedIds.FacilityA1UnitSouth, and SeedIds.FacilityA1UnitMedical—exist before inserting placements or profiles. Preserve the existing early returns for a missing facility or existing resource assets, and ensure seeding skips or safely resolves unit IDs when those units are unavailable.src/backend/Baseera.Application/Resources/ResourceServices.cs-591-619 (1)
591-619: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winNo overlap guard lets requirement baselines double-count.
GetSummaryAsyncsumsRequiredQuantityover every active requirement (Lines 121-122), so recording a second requirement for the same(ResourceType, ResourceCategory, FacilityUnitId)with an overlapping effective window inflatesRequiredandGaprather than superseding the previous baseline. Issue#15explicitly calls for duplicate prevention here — consider closing the prior row'sEffectiveToUtcor rejecting overlaps.🤖 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 `@src/backend/Baseera.Application/Resources/ResourceServices.cs` around lines 591 - 619, Add overlap protection to RecordRequirementAsync for the same ResourceType, ResourceCategory, and FacilityUnitId before creating the new ResourceRequirement. Query existing requirements with intersecting effective windows and either reject the new request or close the prior row’s EffectiveToUtc, ensuring GetSummaryAsync cannot double-count active baselines.src/backend/Baseera.Application/Resources/ResourceReadinessPolicy.cs-104-122 (1)
104-122: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winRestrict transitions out of terminal resource statuses.
CanTransitiononly blocksRetired→ non-Unknown, so retired assets can be moved toUnknownand then elsewhere, andLostexits are always allowed. This conflicts with the state machine’s terminal statuses;Lost,Retired,Transferred, andUnknownshould either explicitly transition only to approved statuses or be treated as terminal.🤖 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 `@src/backend/Baseera.Application/Resources/ResourceReadinessPolicy.cs` around lines 104 - 122, Update ResourceReadinessPolicy.CanTransition to enforce terminal-state rules for Lost, Retired, Transferred, and Unknown: prevent unapproved transitions out of each terminal status, including the Retired-to-Unknown escape path, while preserving same-status transitions and any explicitly approved destinations defined by the state machine.
🧹 Nitpick comments (10)
docs/phase-d5-resource-test-matrix.md (1)
12-18: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valueUpdate planned coverage entries to point at existing integration coverage.
ResourceReadinessIntegrationTestsalready covers facility scope, unauthorizedForbidden, facility out-of-scopeNotFound, workspace widget visibility, authorized asset creation/auditing, and EF migration model verification. Replacing the stale “Planned CI/integration coverage” bullets with these existing tests or moving them to “Implemented automated coverage” keeps this matrix actionable.🤖 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 `@docs/phase-d5-resource-test-matrix.md` around lines 12 - 18, Replace the stale “Planned CI/integration coverage” entries in the resource test matrix with an “Implemented automated coverage” section that references ResourceReadinessIntegrationTests, including its facility-scope authorization responses, workspace widget visibility, authorized asset creation/auditing, and EF migration model verification coverage.src/backend/Baseera.Application/Resources/ResourceServices.cs (4)
626-638: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueRedundant scope check per import call.
PreviewAsync/ConfirmAsynccallEnsureFacilityVisibleAsyncand thenValidateImportAsyncimmediately repeats it (Line 799), doubling the facility lookup. Drop it from the public entry points since the shared helper already returns the scope info.Also applies to: 792-799
🤖 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 `@src/backend/Baseera.Application/Resources/ResourceServices.cs` around lines 626 - 638, Remove the redundant EnsureFacilityVisibleAsync calls from PreviewAsync and ConfirmAsync, leaving their permission checks and ValidateImportAsync invocations intact. Preserve the shared facility-scope validation performed inside ValidateImportAsync.
65-90: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRepeated permission lookups obscure the gating rule.
HasPermission(ResourcesViewAssets)is evaluated three times; hoisting it into a local (var canViewAssets = ...) makes the payload-shaping policy readable in one glance.🤖 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 `@src/backend/Baseera.Application/Resources/ResourceServices.cs` around lines 65 - 90, In GetWorkspacePayloadAsync, evaluate currentUser.HasPermission(PermissionCodes.ResourcesViewAssets) once into a local canViewAssets variable, then reuse it for the categories, exceptions, and distribution gates while preserving the existing permission behavior.
165-171: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winAwaiting a second query inside the object initializer hides an extra round trip.
DataEffectiveAtUtcre-scans the facility's assets after the aggregate query. FoldMAX(LastVerifiedAtUtc)into the existingResourceStatusCountsprojection, or at least hoist the await above the initializer for readability.🤖 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 `@src/backend/Baseera.Application/Resources/ResourceServices.cs` around lines 165 - 171, Update the resource status construction around the ResourceStatusCounts query and object initializer so DataEffectiveAtUtc does not await a separate AssetsInFacility query inside the initializer. Prefer including MAX(LastVerifiedAtUtc) in the existing ResourceStatusCounts projection and reuse that result; otherwise execute the asset query before the initializer and assign the captured value.
43-49: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
ResourceReadinessOptionsis instantiated inline, so none of it is configurable. The class is shaped like an options type (initsetters, defaults) butnew()bypasses configuration entirely. Either register it withIOptions<ResourceReadinessOptions>and inject, or make the valuesconstto signal they aren't tunable.Also applies to: 63-63
🤖 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 `@src/backend/Baseera.Application/Resources/ResourceServices.cs` around lines 43 - 49, Make ResourceReadinessOptions genuinely configurable by registering it through the application configuration and injecting IOptions<ResourceReadinessOptions> wherever it is consumed, replacing inline new ResourceReadinessOptions() construction with the injected values. Preserve the existing defaults as configuration fallbacks and update all affected usages, including the additional location noted in the comment.src/backend/Baseera.Infrastructure/Persistence/DatabaseInitializer.cs (1)
255-272: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDRY:
resourceManagerrestates every code inresourceSummary. Compose it instead so the two can't drift.♻️ Suggested composition
- string[] resourceManager = - [ - PermissionCodes.ResourcesViewSummary, - PermissionCodes.ResourcesViewAssets, - PermissionCodes.ResourcesViewVehicles, - PermissionCodes.ResourcesViewCommunicationDevices, - PermissionCodes.ResourcesViewEquipment, - PermissionCodes.ResourcesViewFacilityAssets, - PermissionCodes.ResourcesManageAssets, + string[] resourceManager = + [ + ..resourceSummary, + PermissionCodes.ResourcesManageAssets, PermissionCodes.ResourcesManagePlacements, PermissionCodes.ResourcesManageStatus, - PermissionCodes.ResourcesViewMaintenance, PermissionCodes.ResourcesManageMaintenance, - PermissionCodes.ResourcesViewRequirements, PermissionCodes.ResourcesManageRequirements, PermissionCodes.ResourcesImport, PermissionCodes.ResourcesReconcile ];🤖 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 `@src/backend/Baseera.Infrastructure/Persistence/DatabaseInitializer.cs` around lines 255 - 272, Update the resourceManager initialization in DatabaseInitializer to compose its values from the existing resourceSummary collection instead of duplicating each PermissionCodes entry. Preserve the intended resource manager contents while ensuring future changes to resourceSummary are reflected automatically.docs/phase-d5-resource-readiness-calculation.md (1)
7-13: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueClarify the self-referential
Availabledefinition. Line 8 reads asAvailable = Available + Standby, which is circular. It describes the availability-rate numerator, and the denominator (TotalRegistered - Retired - Transferred) plus the null case when the denominator is zero are worth spelling out, perResourceReadinessPolicy.Calculate(src/backend/Baseera.Application/Resources/ResourceReadinessPolicy.cs:45-52).📝 Suggested wording
- Operational = Available + InUse + Standby + Reserved. -- Available = Available + Standby. -- Retired and Transferred are excluded from the operational availability denominator. +- Availability rate = (Available + Standby) / (TotalRegistered - Retired - Transferred), and is null when that denominator is zero. +- Retired and Transferred are excluded from the availability denominator.🤖 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 `@docs/phase-d5-resource-readiness-calculation.md` around lines 7 - 13, Clarify the readiness documentation’s self-referential Available definition by describing it as the availability-rate numerator: Available plus Standby. Add the denominator as TotalRegistered minus Retired and Transferred, and state that the readiness rate is null when this denominator is zero, matching ResourceReadinessPolicy.Calculate.src/backend/Baseera.Domain/Resources/ResourceEntities.cs (1)
349-349: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefer injected time over
DateTimeOffset.UtcNowdefaults. BothResourceStatusEvent.RecordedAtUtcandResourceImportBatch.SubmittedAtUtcdefault to ambient wall-clock time, while the service layer consistently usesTimeProvider(ResourceServices.cs:446,858). Any write path that forgets to set these gets non-deterministic, untestable timestamps.Also applies to: 475-475
🤖 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 `@src/backend/Baseera.Domain/Resources/ResourceEntities.cs` at line 349, Remove the ambient DateTimeOffset.UtcNow property initializers from ResourceStatusEvent.RecordedAtUtc and ResourceImportBatch.SubmittedAtUtc. Ensure each write path assigns timestamps through the existing injected TimeProvider in the relevant ResourceServices methods, including the paths around the status-event and import-batch creation flows, while preserving explicit caller-provided values where applicable.src/backend/Baseera.Application/Resources/ResourceReadinessPolicy.cs (1)
30-61: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
IsOperational/IsAvailable/IsInScopeDenominatorduplicate the arithmetic inCalculate. The predicates define the operational and in-scope sets, butCalculatere-derives them by summing individual fields (Lines 44-45). Adding a newResourceStatusmeans updating both places plus the SQL-side counts inResourceServices.GetSummaryAsync. Worth a comment tying them together, or derivingResourceReadinessInputsfrom the predicates.🤖 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 `@src/backend/Baseera.Application/Resources/ResourceReadinessPolicy.cs` around lines 30 - 61, The status predicates and Calculate use separate definitions of operational and in-scope resources, so they can diverge when statuses change. Update the ResourceReadinessPolicy implementation to derive or validate Calculate’s counts through IsOperational, IsAvailable, and IsInScopeDenominator, and add a concise comment documenting that ResourceReadinessInputs and ResourceServices.GetSummaryAsync must use the same status sets.src/frontend/src/pages/resources/FacilityResourcesPage.tsx (1)
38-51: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winRedundant refetch of unfiltered data on every category filter click.
summary,categories, andexceptionsdon't depend onresourceType, but including it in the sharedqueryKeyforces all four calls to re-run whenever the user clicks a category rail button (which only changes theassetsfilter).♻️ Suggested split
- const query = useQuery({ - queryKey: ['resources-admin', facilityId, resourceType], - queryFn: async () => { - const filters = resourceType ? { resourceType } : {} - const [summary, categories, exceptions, assets] = await Promise.all([ - api.resources.summary(facilityId!), - canViewAssets ? api.resources.categories(facilityId!) : Promise.resolve([]), - canViewAssets ? api.resources.exceptions(facilityId!, 20) : Promise.resolve([]), - canViewAssets ? api.resources.assets(facilityId!, filters) : Promise.resolve([]), - ]) - return { summary, categories, exceptions, assets } - }, - enabled: canView && Boolean(facilityId), - }) + const baseQuery = useQuery({ + queryKey: ['resources-admin', facilityId], + queryFn: async () => { + const [summary, categories, exceptions] = await Promise.all([ + api.resources.summary(facilityId!), + canViewAssets ? api.resources.categories(facilityId!) : Promise.resolve([]), + canViewAssets ? api.resources.exceptions(facilityId!, 20) : Promise.resolve([]), + ]) + return { summary, categories, exceptions } + }, + enabled: canView && Boolean(facilityId), + }) + const assetsQuery = useQuery({ + queryKey: ['resources-admin-assets', facilityId, resourceType], + queryFn: () => api.resources.assets(facilityId!, resourceType ? { resourceType } : {}), + enabled: canView && canViewAssets && Boolean(facilityId), + })🤖 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 `@src/frontend/src/pages/resources/FacilityResourcesPage.tsx` around lines 38 - 51, Split the shared useQuery in FacilityResourcesPage so resourceType only keys and refetches the assets request, while summary, categories, and exceptions use a stable facility-scoped query independent of the category filter. Preserve the existing canViewAssets gating and return the same data needed by the page, avoiding refetches of unfiltered resources when the category rail changes.
🤖 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 `@docs/phase-d5-resource-api-contract.md`:
- Around line 10-11: Update the documented authorization contract for the
`/assets` and `/assets/{assetId}` routes so access is checked against the
resolved resource type’s specific permission (`Resources.ViewVehicles`,
`Resources.ViewCommunications`, `Resources.ViewEquipment`, or
`Resources.ViewFacilityAssets`) rather than only `Resources.ViewAssets`.
Alternatively, explicitly define `Resources.ViewAssets` as the sole all-category
permission and remove the unused category-specific permission contract.
In `@src/backend/Baseera.Api/Endpoints/ApiEndpoints.cs`:
- Around line 283-289: Update the asset lookup handler in the resources.MapGet
endpoint to inspect the result of service.GetAssetAsync and return
Results.NotFound() when it is null; otherwise preserve the existing Results.Ok
response for found assets, matching the neighboring get-by-id endpoints.
In `@src/backend/Baseera.Application/Abstractions/Abstractions.cs`:
- Around line 91-100: The nine resource DbSet properties in
IBaseeraDbContext—ResourceAssets, VehicleProfiles, CommunicationDeviceProfiles,
EquipmentProfiles, FacilityAssetProfiles, ResourceStatusEvents,
ResourcePlacements, MaintenanceWorkOrders, and ResourceRequirements—must be
abstract rather than defaulting to empty queryables, so implementations are
required to provide them. Update all empty test doubles implementing
IBaseeraDbContext to explicitly define these properties with the appropriate
empty queryable values; leave ResourceImportBatches unchanged unless it is also
part of the intended nine.
In `@src/backend/Baseera.Application/Resources/ResourceServices.cs`:
- Around line 564-589: The CreateWorkOrderAsync method must stop deriving
WorkOrderNumber from the global MaintenanceWorkOrders count. Add a
maintenance-work-order sequence allocator alongside the existing
IBaseeraDbContext sequence methods, use it when constructing WorkOrderNumber,
and add a unique database index for the generated number to prevent collisions
under concurrent creation.
- Around line 849-869: Update the import confirmation flow containing the
`ResourceImportBatch` insertion to first look up an existing confirmed batch by
`(FacilityId, SourceSystem, SourceReference, FileHash)`. If found, return the
prior confirmed result before inserting assets or creating a new batch;
otherwise preserve the existing validation, insertion, save, and audit flow.
- Around line 307-321: Update ListAssetsAsync and its /assets endpoint to
enforce the resource-specific permissions before querying or projecting assets.
Require the appropriate vehicle, communication-device, equipment, or
facility-asset permission based on resourceType, while preserving
ResourcesViewAssets as the base permission and preventing unauthorized sensitive
asset fields from being returned.
- Around line 436-444: Align the resource import flow’s duplicate validation
with the organization-scoped unique constraint by reusing the same creation
validation for preview and insert, rather than facility-scoped OrdinalIgnoreCase
matching. Update the ResourceAssets AnyAsync check accordingly, and catch
DataError from SaveChangesAsync to translate organization-level duplicates and
race conditions into the existing duplicate-violation response.
In `@src/backend/Baseera.Application/Workspaces/FacilityWorkspaceReadService.cs`:
- Around line 160-163: Update the resource item construction in the relevant
branches of the workspace read service, including
BuildResourcePriorityItemsAsync and the locations around the other referenced
blocks, so ResourceTarget uses the permission that granted the item’s caller
context (ResourcesViewSummary or ResourcesViewMaintenance) rather than always
emitting ResourcesViewAssets. Mirror OccupancyTarget’s context-aware permission
handling and preserve the existing resource list/detail permission checks.
In `@src/backend/Baseera.Infrastructure/Persistence/BaseeraDbContext.cs`:
- Around line 295-297: Update the model configuration in BaseeraDbContext to add
query filters for resource profiles, status events, placements, and maintenance
work orders that exclude records whose parent ResourceAsset is soft-deleted.
Preserve existing soft-delete filters and ensure administrative or audit queries
can still access these records via IgnoreQueryFilters().
In
`@src/backend/Baseera.Infrastructure/Persistence/Configurations/ResourceConfigurations.cs`:
- Around line 105-117: The ResourceStatusEvent configuration lacks append-only
enforcement. Add a ResourceStatusEventAppendOnlyGuard that rejects Modified and
Deleted entries, then invoke it from BaseeraDbContext.EnforceAppendOnlyGuards()
while preserving allowed Added and Unchanged states.
In
`@src/backend/Baseera.Infrastructure/Persistence/Migrations/20260725111037_PhaseD5ResourceReadinessCore.cs`:
- Around line 86-114: Add the required HasOne<Facility>() and HasOne<User>()
relationships to ResourceImportBatchConfiguration, including their foreign-key
properties and delete behavior, then regenerate the migration and snapshot.
Update 20260725111037_PhaseD5ResourceReadinessCore.cs lines 86-114 and
BaseeraDbContextModelSnapshot.cs lines 5031-5111 so ResourceImportBatches
includes matching foreign-key constraints for FacilityId and SubmittedByUserId.
In
`@src/backend/Baseera.Infrastructure/Persistence/Migrations/20260725111037_PhaseD5ResourceReadinessCore.Designer.cs`:
- Around line 5110-5113: Update the CK_ResourceImportBatches_Counts check
constraint in the ResourceImportBatches table mapping to enforce the full import
contract: require component counts to equal TotalRows and ensure AppliedRows
does not exceed ValidRows, while retaining all non-negative checks.
- Around line 5273-5279: Update the ResourceRequirements model configuration
represented by the migration designer so the facility/unit/resource
type/category identity cannot have duplicate active requirements under the
intended effective-period semantics. Replace or supplement the non-unique index
involving FacilityId, FacilityUnitId, ResourceType, ResourceCategory, and
EffectiveFromUtc with the appropriate unique index or equivalent database
constraint, and ensure the migration applies this enforcement.
- Around line 5034-5056: Update the ResourceImportBatch model configuration to
define a required relationship from ResourceImportBatch.FacilityId to the
Facility entity, using the existing facility key and appropriate delete
behavior. Ensure the generated migration and model snapshot include the
corresponding foreign key constraint.
- Around line 4976-4980: Update the EF relationship mappings containing
OperationalFacilityId and OperationalFacilityUnitId so the database enforces
that the selected unit belongs to the selected facility. Define a composite
principal key or equivalent constraint on the facility-unit relationship and use
composite foreign keys for every affected entity configuration, including all
repeated mappings in this migration; do not leave the two IDs enforced
independently.
---
Outside diff comments:
In `@src/backend/Baseera.Application/Workspaces/FacilityWorkspaceReadService.cs`:
- Around line 273-284: Update the domain insertion logic in
FacilityWorkspaceReadService so the Resources and missing-Resources branches use
a running insertion index rather than hard-coded index 6, incrementing it only
when the Occupancy domain is inserted. Preserve the intended order of forms,
occupancy when available, resources, and incidents for all permission
combinations, including when Occupancy is absent.
In `@src/frontend/src/pages/workspaces/FacilityWorkspacePage.tsx`:
- Around line 640-680: Update PanelDetail to handle vehicle, weapon,
communication-device, and equipment panel types with a dedicated resource-detail
panel instead of DomainGapPanel, reusing the established detail-fetching
patterns from NotePanel and CorrectiveActionPanel. Update findPanelSummary and
the resource panel creators resourcePanelType, panelForPriorityItem, and
panelForActivityItem so domain-resources-${resourceTypeCode} entity IDs resolve
against the resources domain key. Preserve the existing fallback only for
genuinely unsupported panel types.
---
Minor comments:
In `@src/backend/Baseera.Application/Resources/ResourceReadinessPolicy.cs`:
- Around line 104-122: Update ResourceReadinessPolicy.CanTransition to enforce
terminal-state rules for Lost, Retired, Transferred, and Unknown: prevent
unapproved transitions out of each terminal status, including the
Retired-to-Unknown escape path, while preserving same-status transitions and any
explicitly approved destinations defined by the state machine.
In `@src/backend/Baseera.Application/Resources/ResourceServices.cs`:
- Around line 689-723: The BuildCategory method incorrectly passes
row.StaleRecords into the ResourceReadinessInputs.MissingDataRecords field.
Update this argument to use the corresponding MissingDataRecords value from
ResourceCategoryCounts, preserving StaleRecords for freshness status and
confidence calculations.
- Around line 264-279: The ResourceUnitDistributionDto mapping currently
hard-codes Gap to zero and ignores ResourceRequirement.FacilityUnitId. Update
the surrounding resource distribution method to calculate each unit’s gap from
requirements scoped to row.OperationalFacilityUnitId, or remove Gap from the DTO
payload until that calculation is implemented; do not continue sending a
misleading zero value.
- Around line 528-559: The PlaceAssetAsync method must reject or otherwise
prevent inverted placement intervals. Before closing active placements, validate
that request.EffectiveFromUtc is not earlier than each existing placement’s
EffectiveFromUtc, and validate the new placement’s EffectiveToUtc is null or at
least request.EffectiveFromUtc; preserve the existing placement update and
creation flow only for valid intervals.
- Around line 591-619: Add overlap protection to RecordRequirementAsync for the
same ResourceType, ResourceCategory, and FacilityUnitId before creating the new
ResourceRequirement. Query existing requirements with intersecting effective
windows and either reject the new request or close the prior row’s
EffectiveToUtc, ensuring GetSummaryAsync cannot double-count active baselines.
In `@src/backend/Baseera.Infrastructure/Persistence/DatabaseInitializer.cs`:
- Around line 930-938: Update the guard in the demo resource seeding flow around
EnsureDemoOccupancyAsync to verify that the required FacilityA1
units—SeedIds.FacilityA1UnitNorth, SeedIds.FacilityA1UnitSouth, and
SeedIds.FacilityA1UnitMedical—exist before inserting placements or profiles.
Preserve the existing early returns for a missing facility or existing resource
assets, and ensure seeding skips or safely resolves unit IDs when those units
are unavailable.
---
Nitpick comments:
In `@docs/phase-d5-resource-readiness-calculation.md`:
- Around line 7-13: Clarify the readiness documentation’s self-referential
Available definition by describing it as the availability-rate numerator:
Available plus Standby. Add the denominator as TotalRegistered minus Retired and
Transferred, and state that the readiness rate is null when this denominator is
zero, matching ResourceReadinessPolicy.Calculate.
In `@docs/phase-d5-resource-test-matrix.md`:
- Around line 12-18: Replace the stale “Planned CI/integration coverage” entries
in the resource test matrix with an “Implemented automated coverage” section
that references ResourceReadinessIntegrationTests, including its facility-scope
authorization responses, workspace widget visibility, authorized asset
creation/auditing, and EF migration model verification coverage.
In `@src/backend/Baseera.Application/Resources/ResourceReadinessPolicy.cs`:
- Around line 30-61: The status predicates and Calculate use separate
definitions of operational and in-scope resources, so they can diverge when
statuses change. Update the ResourceReadinessPolicy implementation to derive or
validate Calculate’s counts through IsOperational, IsAvailable, and
IsInScopeDenominator, and add a concise comment documenting that
ResourceReadinessInputs and ResourceServices.GetSummaryAsync must use the same
status sets.
In `@src/backend/Baseera.Application/Resources/ResourceServices.cs`:
- Around line 626-638: Remove the redundant EnsureFacilityVisibleAsync calls
from PreviewAsync and ConfirmAsync, leaving their permission checks and
ValidateImportAsync invocations intact. Preserve the shared facility-scope
validation performed inside ValidateImportAsync.
- Around line 65-90: In GetWorkspacePayloadAsync, evaluate
currentUser.HasPermission(PermissionCodes.ResourcesViewAssets) once into a local
canViewAssets variable, then reuse it for the categories, exceptions, and
distribution gates while preserving the existing permission behavior.
- Around line 165-171: Update the resource status construction around the
ResourceStatusCounts query and object initializer so DataEffectiveAtUtc does not
await a separate AssetsInFacility query inside the initializer. Prefer including
MAX(LastVerifiedAtUtc) in the existing ResourceStatusCounts projection and reuse
that result; otherwise execute the asset query before the initializer and assign
the captured value.
- Around line 43-49: Make ResourceReadinessOptions genuinely configurable by
registering it through the application configuration and injecting
IOptions<ResourceReadinessOptions> wherever it is consumed, replacing inline new
ResourceReadinessOptions() construction with the injected values. Preserve the
existing defaults as configuration fallbacks and update all affected usages,
including the additional location noted in the comment.
In `@src/backend/Baseera.Domain/Resources/ResourceEntities.cs`:
- Line 349: Remove the ambient DateTimeOffset.UtcNow property initializers from
ResourceStatusEvent.RecordedAtUtc and ResourceImportBatch.SubmittedAtUtc. Ensure
each write path assigns timestamps through the existing injected TimeProvider in
the relevant ResourceServices methods, including the paths around the
status-event and import-batch creation flows, while preserving explicit
caller-provided values where applicable.
In `@src/backend/Baseera.Infrastructure/Persistence/DatabaseInitializer.cs`:
- Around line 255-272: Update the resourceManager initialization in
DatabaseInitializer to compose its values from the existing resourceSummary
collection instead of duplicating each PermissionCodes entry. Preserve the
intended resource manager contents while ensuring future changes to
resourceSummary are reflected automatically.
In `@src/frontend/src/pages/resources/FacilityResourcesPage.tsx`:
- Around line 38-51: Split the shared useQuery in FacilityResourcesPage so
resourceType only keys and refetches the assets request, while summary,
categories, and exceptions use a stable facility-scoped query independent of the
category filter. Preserve the existing canViewAssets gating and return the same
data needed by the page, avoiding refetches of unfiltered resources when the
category rail changes.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 7b9c35e1-cf46-43d1-bf6b-155cc62ddcfb
⛔ Files ignored due to path filters (17)
docs/screenshots/phase-d5/desktop-communications.pngis excluded by!**/*.pngdocs/screenshots/phase-d5/desktop-context-panel.pngis excluded by!**/*.pngdocs/screenshots/phase-d5/desktop-critical-resource.pngis excluded by!**/*.pngdocs/screenshots/phase-d5/desktop-equipment.pngis excluded by!**/*.pngdocs/screenshots/phase-d5/desktop-facility-assets.pngis excluded by!**/*.pngdocs/screenshots/phase-d5/desktop-maintenance.pngis excluded by!**/*.pngdocs/screenshots/phase-d5/desktop-requirement-gaps.pngis excluded by!**/*.pngdocs/screenshots/phase-d5/desktop-resources-overview.pngis excluded by!**/*.pngdocs/screenshots/phase-d5/desktop-stale-data.pngis excluded by!**/*.pngdocs/screenshots/phase-d5/desktop-vehicles.pngis excluded by!**/*.pngdocs/screenshots/phase-d5/empty-state.pngis excluded by!**/*.pngdocs/screenshots/phase-d5/import-preview.pngis excluded by!**/*.pngdocs/screenshots/phase-d5/mobile-asset-detail.pngis excluded by!**/*.pngdocs/screenshots/phase-d5/mobile-overview.pngis excluded by!**/*.pngdocs/screenshots/phase-d5/partial-state.pngis excluded by!**/*.pngdocs/screenshots/phase-d5/tablet-overview.pngis excluded by!**/*.pngdocs/screenshots/phase-d5/validation-errors.pngis excluded by!**/*.png
📒 Files selected for processing (52)
README.mddocs/implementation-plan.mddocs/permissions-matrix.mddocs/phase-d5-resource-api-contract.mddocs/phase-d5-resource-completion-report.mddocs/phase-d5-resource-domain-model.mddocs/phase-d5-resource-import-contract.mddocs/phase-d5-resource-maintenance-workflow.mddocs/phase-d5-resource-migration.mddocs/phase-d5-resource-performance.mddocs/phase-d5-resource-permissions.mddocs/phase-d5-resource-placement-and-ownership.mddocs/phase-d5-resource-readiness-calculation.mddocs/phase-d5-resource-readiness-current-state-analysis.mddocs/phase-d5-resource-readiness-scope.mddocs/phase-d5-resource-rtl-walkthrough.mddocs/phase-d5-resource-security.mddocs/phase-d5-resource-source-of-truth.mddocs/phase-d5-resource-status-state-machine.mddocs/phase-d5-resource-test-matrix.mdsrc/backend/Baseera.Api/Authorization/AuthorizationExtensions.cssrc/backend/Baseera.Api/Endpoints/ApiEndpoints.cssrc/backend/Baseera.Application/Abstractions/Abstractions.cssrc/backend/Baseera.Application/DependencyInjection/ApplicationServiceCollectionExtensions.cssrc/backend/Baseera.Application/Resources/ResourceDtos.cssrc/backend/Baseera.Application/Resources/ResourceReadinessPolicy.cssrc/backend/Baseera.Application/Resources/ResourceServices.cssrc/backend/Baseera.Application/Workspaces/FacilityWorkspaceDefinitions.cssrc/backend/Baseera.Application/Workspaces/FacilityWorkspaceDtos.cssrc/backend/Baseera.Application/Workspaces/FacilityWorkspaceReadService.cssrc/backend/Baseera.Application/Workspaces/FacilityWorkspaceWidgetProviders.cssrc/backend/Baseera.Domain/Identity/IdentityEntities.cssrc/backend/Baseera.Domain/Resources/ResourceEntities.cssrc/backend/Baseera.Infrastructure/Persistence/BaseeraDbContext.cssrc/backend/Baseera.Infrastructure/Persistence/Configurations/ResourceConfigurations.cssrc/backend/Baseera.Infrastructure/Persistence/DatabaseInitializer.cssrc/backend/Baseera.Infrastructure/Persistence/Migrations/20260725111037_PhaseD5ResourceReadinessCore.Designer.cssrc/backend/Baseera.Infrastructure/Persistence/Migrations/20260725111037_PhaseD5ResourceReadinessCore.cssrc/backend/Baseera.Infrastructure/Persistence/Migrations/BaseeraDbContextModelSnapshot.cssrc/backend/tests/Baseera.IntegrationTests/ResourceReadinessIntegrationTests.cssrc/backend/tests/Baseera.UnitTests/Resources/ResourceReadinessPolicyTests.cssrc/backend/tests/Baseera.UnitTests/Workspaces/WorkspaceFrameworkTests.cssrc/frontend/src/App.notifications.test.tsxsrc/frontend/src/App.tsxsrc/frontend/src/api/client.tssrc/frontend/src/index.csssrc/frontend/src/pages/notes/ObservationWorkspacePage.test.tsxsrc/frontend/src/pages/resources/FacilityResourcesPage.test.tsxsrc/frontend/src/pages/resources/FacilityResourcesPage.tsxsrc/frontend/src/pages/workspaces/FacilityWorkspacePage.test.tsxsrc/frontend/src/pages/workspaces/FacilityWorkspacePage.tsxsrc/frontend/src/workspaces/WorkspaceShell.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- src/frontend/src/pages/notes/ObservationWorkspacePage.test.tsx
- src/frontend/src/App.notifications.test.tsx
Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
src/backend/tests/Baseera.UnitTests/Resources/ResourceAccessPolicyTests.cs (1)
10-17: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover the
nullmapping branch.
ViewPermissionForhas a_ => nullfallback that no case exercises. Adding a case for a non-resource enum member (or assertingnullfor an unmapped value) locks in that newResourceTypemembers are denied by default rather than silently falling through.🤖 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 `@src/backend/tests/Baseera.UnitTests/Resources/ResourceAccessPolicyTests.cs` around lines 10 - 17, Add a theory case to ResourceAccessPolicyTests.ViewPermissionFor_maps_resource_types using a non-resource or otherwise unmapped ResourceType value with a null expected permission. This must exercise the _ => null fallback in ResourceAccessPolicy.ViewPermissionFor and verify unmapped members are denied by default.src/backend/tests/Baseera.IntegrationTests/ResourceReadinessIntegrationTests.cs (2)
420-462: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAssert the valid creation immediately after issuing it.
createis awaited at Line 420 but only validated at Line 462, after the foreign-unit setup andbadCreateassertions. Checking it up front makes a failure of the happy path unambiguous.🤖 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 `@src/backend/tests/Baseera.IntegrationTests/ResourceReadinessIntegrationTests.cs` around lines 420 - 462, Move the create.EnsureSuccessStatusCode() call immediately after the initial valid PostAsJsonAsync request, before creating the foreign facility unit and issuing badCreate. Leave the foreign-unit setup and badCreate status assertion unchanged.
327-335: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winMaterialize the returned IDs before querying EF Core.
ids.Select(x => x!.Id).Contains(o.Id)embeds an in-memory projection inside theWherepredicate. Use a pre-materialized ID array so the repository filter stays stable, translation is deterministic, and null response handling fails clearly.♻️ Proposed refactor
- var ids = await Task.WhenAll(responses.Select(r => r.Content.ReadFromJsonAsync<CreateResponse>(JsonOptions))); + var created = await Task.WhenAll(responses.Select(r => r.Content.ReadFromJsonAsync<CreateResponse>(JsonOptions))); + Assert.All(created, item => Assert.NotNull(item)); + var ids = created.Select(item => item!.Id).ToArray(); using var verifyScope = factory.Services.CreateScope(); var verifyDb = verifyScope.ServiceProvider.GetRequiredService<BaseeraDbContext>(); var numbers = await verifyDb.MaintenanceWorkOrders - .Where(o => ids.Select(x => x!.Id).Contains(o.Id)) + .Where(o => ids.Contains(o.Id)) .Select(o => o.WorkOrderNumber) .ToListAsync();🤖 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 `@src/backend/tests/Baseera.IntegrationTests/ResourceReadinessIntegrationTests.cs` around lines 327 - 335, Materialize the response IDs before the EF Core query in ResourceReadinessIntegrationTests, validating that each CreateResponse is non-null so failures are explicit. Use the resulting ID array in the MaintenanceWorkOrders.Where predicate instead of projecting ids inline, while preserving the existing distinct work-order number assertion.
🤖 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 `@src/backend/Baseera.Application/Resources/ResourceAccessPolicy.cs`:
- Around line 76-86: Reject unsupported import-batch statuses consistently
across validation and database constraints. In ResourceAccessPolicy.cs, update
the status validation to return false after the explicit Confirmed and Previewed
cases. In
src/backend/Baseera.Infrastructure/Persistence/Migrations/20260725151507_PhaseD5ResourceReadinessCore.cs
lines 122-124, update the migration check constraint to allow only Previewed and
Confirmed, then regenerate the corresponding model metadata in
src/backend/Baseera.Infrastructure/Persistence/Migrations/20260725151507_PhaseD5ResourceReadinessCore.Designer.cs
lines 5120-5124.
In
`@src/backend/tests/Baseera.IntegrationTests/ResourceReadinessIntegrationTests.cs`:
- Around line 140-144: Add an explicit non-empty assertion for the assets
collection in the resource-readiness test before Assert.All, ensuring the
facility resource type filtering is exercised while preserving the existing
communication-device type assertion.
---
Nitpick comments:
In
`@src/backend/tests/Baseera.IntegrationTests/ResourceReadinessIntegrationTests.cs`:
- Around line 420-462: Move the create.EnsureSuccessStatusCode() call
immediately after the initial valid PostAsJsonAsync request, before creating the
foreign facility unit and issuing badCreate. Leave the foreign-unit setup and
badCreate status assertion unchanged.
- Around line 327-335: Materialize the response IDs before the EF Core query in
ResourceReadinessIntegrationTests, validating that each CreateResponse is
non-null so failures are explicit. Use the resulting ID array in the
MaintenanceWorkOrders.Where predicate instead of projecting ids inline, while
preserving the existing distinct work-order number assertion.
In `@src/backend/tests/Baseera.UnitTests/Resources/ResourceAccessPolicyTests.cs`:
- Around line 10-17: Add a theory case to
ResourceAccessPolicyTests.ViewPermissionFor_maps_resource_types using a
non-resource or otherwise unmapped ResourceType value with a null expected
permission. This must exercise the _ => null fallback in
ResourceAccessPolicy.ViewPermissionFor and verify unmapped members are denied by
default.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 5a406325-4b25-4df6-9503-35699d3b11b0
📒 Files selected for processing (26)
docs/permissions-matrix.mddocs/phase-d5-resource-api-contract.mddocs/phase-d5-resource-completion-report.mddocs/phase-d5-resource-domain-model.mddocs/phase-d5-resource-import-contract.mddocs/phase-d5-resource-migration.mddocs/phase-d5-resource-permissions.mddocs/phase-d5-resource-test-matrix.mdsrc/backend/Baseera.Api/Endpoints/ApiEndpoints.cssrc/backend/Baseera.Application/Abstractions/Abstractions.cssrc/backend/Baseera.Application/Resources/ResourceAccessPolicy.cssrc/backend/Baseera.Application/Resources/ResourceDtos.cssrc/backend/Baseera.Application/Resources/ResourceServices.cssrc/backend/Baseera.Application/Workspaces/FacilityWorkspaceReadService.cssrc/backend/Baseera.Domain/Resources/ResourceEntities.cssrc/backend/Baseera.Infrastructure/Persistence/BaseeraDbContext.cssrc/backend/Baseera.Infrastructure/Persistence/Configurations/EntityConfigurations.cssrc/backend/Baseera.Infrastructure/Persistence/Configurations/ResourceConfigurations.cssrc/backend/Baseera.Infrastructure/Persistence/Migrations/20260725151507_PhaseD5ResourceReadinessCore.Designer.cssrc/backend/Baseera.Infrastructure/Persistence/Migrations/20260725151507_PhaseD5ResourceReadinessCore.cssrc/backend/Baseera.Infrastructure/Persistence/Migrations/BaseeraDbContextModelSnapshot.cssrc/backend/tests/Baseera.IntegrationTests/ResourceReadinessIntegrationTests.cssrc/backend/tests/Baseera.UnitTests/Forms/Campaigns/FormCampaignCoreTests.cssrc/backend/tests/Baseera.UnitTests/NoteWorkflowTests.cssrc/backend/tests/Baseera.UnitTests/Resources/ResourceAccessPolicyTests.cssrc/backend/tests/Baseera.UnitTests/Workspaces/WorkspaceFrameworkTests.cs
🚧 Files skipped from review as they are similar to previous changes (14)
- docs/phase-d5-resource-migration.md
- docs/phase-d5-resource-import-contract.md
- docs/phase-d5-resource-permissions.md
- docs/phase-d5-resource-api-contract.md
- docs/phase-d5-resource-test-matrix.md
- docs/phase-d5-resource-completion-report.md
- src/backend/tests/Baseera.UnitTests/Workspaces/WorkspaceFrameworkTests.cs
- src/backend/Baseera.Api/Endpoints/ApiEndpoints.cs
- src/backend/Baseera.Infrastructure/Persistence/Configurations/ResourceConfigurations.cs
- src/backend/Baseera.Application/Resources/ResourceDtos.cs
- src/backend/Baseera.Infrastructure/Persistence/Migrations/BaseeraDbContextModelSnapshot.cs
- src/backend/Baseera.Domain/Resources/ResourceEntities.cs
- src/backend/Baseera.Application/Workspaces/FacilityWorkspaceReadService.cs
- src/backend/Baseera.Application/Resources/ResourceServices.cs
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
|
This was referenced Jul 26, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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
Implements the Phase D.5 Facility Resource Readiness Center core for Baseera: operational resource inventory, readiness state, placement/ownership, maintenance, requirements/gaps, and Facility Workspace integration.
Scope delivered
Issue links
Does not close #15 or #11.
Notes
Test plan
dotnet build src/backend/Baseera.slnx -c ReleaseBASEERA_TEST_CONNECTIONandSkipped = 0npm audit --audit-level=highmaindocs/screenshots/phase-d5/presentSummary by CodeRabbit
New Features
Documentation
Bug Fixes