Full codebase review - #8
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR introduces a complete Inventory Flow application: a .NET 9 backend (Domain/Application/Infrastructure/Api) with JWT authentication, workspace tenancy, EF Core migrations, and inventory/product/supplier/warehouse/purchase/sales/transfer features; a React/Vite frontend consuming these APIs; Docker/compose deployment; and unit/integration test suites. ChangesBackend (.NET API)
Frontend (React/Vite)
Deployment tooling
Estimated code review effort: 5 (Critical) | ~180 minutes Sequence Diagram(s)sequenceDiagram
participant Browser
participant ApiClient as Frontend apiClient
participant AuthController
participant IdentityAuthenticationService
participant Db as ApplicationDbContext
Browser->>ApiClient: login(email, password)
ApiClient->>AuthController: POST /api/auth/login
AuthController->>IdentityAuthenticationService: LoginAsync(command)
IdentityAuthenticationService->>Db: validate user, resolve workspace
Db-->>IdentityAuthenticationService: user + workspace
IdentityAuthenticationService-->>AuthController: AuthenticationSession
AuthController-->>ApiClient: 200 OK + refresh cookie + access token
ApiClient-->>Browser: store session, redirect to dashboard
sequenceDiagram
participant Browser
participant InventoryController
participant RecordReceiptHandler
participant EfInventoryLedger
participant Db as ApplicationDbContext
Browser->>InventoryController: POST /api/inventory/receipts (Idempotency-Key)
InventoryController->>RecordReceiptHandler: RecordReceiptCommand
RecordReceiptHandler->>EfInventoryLedger: RecordAsync(...)
EfInventoryLedger->>Db: begin Serializable transaction
EfInventoryLedger->>Db: check idempotency key, upsert InventoryBalance
Db-->>EfInventoryLedger: InventoryMovement
EfInventoryLedger-->>RecordReceiptHandler: InventoryMovement
RecordReceiptHandler-->>InventoryController: InventoryMovementResponse
InventoryController-->>Browser: 201 Created
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
🟠 Major comments (24)
backend/src/InventoryFlow.Api/Program.cs-23-33 (1)
23-33: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winPartition the
"api"rate limit by caller.This is a single shared bucket, so one caller can consume the entire 100 requests/minute budget and throttle every other request, including
/health. Use a partitioned policy keyed by authenticated subject or client IP instead.🤖 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 `@backend/src/InventoryFlow.Api/Program.cs` around lines 23 - 33, Replace the shared fixed-window limiter registered in AddRateLimiter with a partitioned policy for the "api" limiter. Key each caller’s bucket by authenticated subject when available, otherwise by client IP, while preserving the 100 requests-per-minute window, queue settings, and 429 rejection status.backend/src/InventoryFlow.Api/ExceptionHandling/GlobalExceptionHandler.cs-80-82 (1)
80-82: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win409-mapped conflict exceptions don't expose
Detail.
ProductSkuConflictException,SupplierNameConflictException,WarehouseNameConflictException, andCollaborationConflictExceptionall map to 409 (lines 45-50), but onlyInsufficientInventoryException/InventoryArchiveConflictException(andDomainException) get their message surfaced viaDetail. Clients hitting a duplicate-name/SKU conflict get a generic "conflicting value exists" body with no explanation.🩹 Proposed fix: include all conflict exceptions
- Detail = exception is DomainException or InsufficientInventoryException or InventoryArchiveConflictException - ? exception.Message - : null, + Detail = exception is DomainException or InsufficientInventoryException or InventoryArchiveConflictException + or ProductSkuConflictException or SupplierNameConflictException or WarehouseNameConflictException + or CollaborationConflictException + ? exception.Message + : null,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/src/InventoryFlow.Api/ExceptionHandling/GlobalExceptionHandler.cs` around lines 80 - 82, Update the Detail selection in GlobalExceptionHandler so ProductSkuConflictException, SupplierNameConflictException, WarehouseNameConflictException, and CollaborationConflictException also expose exception.Message, alongside the existing DomainException, InsufficientInventoryException, and InventoryArchiveConflictException handling.backend/src/InventoryFlow.Api/ExceptionHandling/GlobalExceptionHandler.cs-39-58 (1)
39-58: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winAll exceptions logged at Error level, including expected 4xx business errors.
logger.LogError(line 54) fires unconditionally for every exception —ValidationException,DomainException, conflict exceptions, auth failures — not just true 500s. In production this drowns real server errors in noise and undermines error-rate alerting.🩹 Proposed fix: log level by severity
- logger.LogError( - exception, - "Unhandled exception while processing {RequestMethod} {RequestPath}", - httpContext.Request.Method, - httpContext.Request.Path); + if (statusCode >= StatusCodes.Status500InternalServerError) + { + logger.LogError(exception, "Unhandled exception while processing {RequestMethod} {RequestPath}", + httpContext.Request.Method, httpContext.Request.Path); + } + else + { + logger.LogWarning(exception, "Request failed with {StatusCode} while processing {RequestMethod} {RequestPath}", + statusCode, httpContext.Request.Method, httpContext.Request.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 `@backend/src/InventoryFlow.Api/ExceptionHandling/GlobalExceptionHandler.cs` around lines 39 - 58, Update the exception logging in the global handler to select the log level based on the computed statusCode: retain Error for 500-level unhandled exceptions and use a lower appropriate severity for expected 4xx validation, authorization, authentication, domain, and conflict exceptions. Preserve the existing status mapping and request context, and ensure every exception is still logged.frontend/.gitignore-10-13 (1)
10-13: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winIgnore environment files in Git.
.envand.env.productionremain trackable, so deployment credentials or other configuration can be committed. Mirror the.dockerignorerule while retaining the example file.Proposed fix
node_modules dist dist-ssr *.local +.env +.env.* +!.env.example🤖 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 `@frontend/.gitignore` around lines 10 - 13, Update the frontend ignore rules to exclude environment files, including .env and .env.production, while explicitly retaining the example environment file as tracked. Mirror the existing environment-file pattern used by .dockerignore.frontend/src/lib/api-client.ts-46-60 (1)
46-60: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winPrevent pre-logout requests from refreshing a new session.
A request sent before
invalidateSession()can return 401 afterward. Since Line 57 captures the post-logout epoch, it may refresh with a still-valid cookie and callsetSession(), resurrecting the session while logout is in flight. Stamp each request with its originating epoch and reject its 401 when it no longer matchessessionEpoch.Proposed fix
+type AuthRequestConfig = InternalAxiosRequestConfig & { + _authRetried?: boolean + _sessionEpoch?: number +} + apiClient.interceptors.request.use((config) => { + const request = config as AuthRequestConfig + request._sessionEpoch = sessionEpoch const token = useAuthStore.getState().accessToken - if (token) config.headers.Authorization = `Bearer ${token}` - return config + if (token) request.headers.Authorization = `Bearer ${token}` + return request }) apiClient.interceptors.response.use(undefined, async (error: AxiosError) => { - const request = error.config as - (InternalAxiosRequestConfig & { _authRetried?: boolean }) | undefined - if (error.response?.status !== 401 || !request || request._authRetried) + const request = error.config as AuthRequestConfig | undefined + if ( + error.response?.status !== 401 || + !request || + request._authRetried || + request._sessionEpoch !== sessionEpoch + ) return Promise.reject(error)🤖 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 `@frontend/src/lib/api-client.ts` around lines 46 - 60, Update the request and response interceptors around refreshAccessToken to stamp each request with its originating sessionEpoch, then reject 401 responses when that stamp no longer matches the current sessionEpoch. Ensure the epoch is captured before the request is sent and checked before refreshing or calling setSession, preventing invalidateSession from being undone by an in-flight request.backend/src/InventoryFlow.Domain/Entities/PurchaseReceipt.cs-9-21 (1)
9-21: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winReject null idempotency keys at the domain boundary.
idempotencyKeyis passed toNormalizeIdempotencyKeywithout an explicit null check. Runtime callers can bypass nullable annotations and trigger an unhandled exception instead of a domain validation failure.As per path instructions, missing null checks in
**/*.csare HIGH priority.Proposed fix
{ if (id == Guid.Empty || workspaceId == Guid.Empty || supplierId == Guid.Empty || warehouseId == Guid.Empty || productId == Guid.Empty || inventoryMovementId == Guid.Empty) throw new DomainException("Purchase receipt identifiers are required."); if (receivedAtUtc.Offset != TimeSpan.Zero) throw new DomainException("Purchase receipt time must be in UTC."); + if (idempotencyKey is null) throw new DomainException("Purchase receipt idempotency key is required."); WorkspaceId = workspaceId;🤖 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 `@backend/src/InventoryFlow.Domain/Entities/PurchaseReceipt.cs` around lines 9 - 21, Update the PurchaseReceipt constructor to explicitly reject a null idempotencyKey before calling InventoryMovement.NormalizeIdempotencyKey, raising the appropriate DomainException for invalid input while preserving existing normalization for non-null keys.Source: Path instructions
backend/src/InventoryFlow.Application/Features/Authentication/AuthenticationValidators.cs-11-11 (1)
11-11: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winMake the display-name predicate null-safe in
AuthenticationValidators.cs:11.name.Trim()can throw on null input and turn validation into an exception instead of a normal error. Guard the predicate itself or stop the rule chain beforeMust.🤖 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 `@backend/src/InventoryFlow.Application/Features/Authentication/AuthenticationValidators.cs` at line 11, Update the DisplayName rule in AuthenticationValidators so its whitespace predicate is null-safe and never calls Trim on a null value. Guard the Must predicate or stop validation before it executes, while preserving the existing required-field and whitespace validation messages.Source: Path instructions
backend/src/InventoryFlow.Application/Common/Tenancy/CurrentWorkspace.cs-6-6 (1)
6-6: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winGuard
Nameagainst runtime null values.
string Namecan still receivenullfrom another implementation or deserialization vianull!, allowing an invalid workspace context to propagate. Validate it in an explicit constructor before assigning the property. As per path instructions,**/*.cs: “Flag missing null checks as HIGH.”🤖 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 `@backend/src/InventoryFlow.Application/Common/Tenancy/CurrentWorkspace.cs` at line 6, Update the CurrentWorkspace record to use an explicit constructor that validates the Name argument before assigning it, rejecting runtime null values while preserving the existing Id, Name, and Role members and record semantics.Source: Path instructions
frontend/src/components/layout/Topbar.tsx-45-53 (1)
45-53: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winAdd
catchblocks to async handlers.
signOutandhandleWorkspaceSwitchonly use try/finally — iflogout()orswitchWorkspace()reject, the error is unhandled (unhandled promise rejection) and the user gets no feedback (e.g. workspace switch silently fails with no toast/error state).🔧 Proposed fix
const signOut = async () => { invalidateSession() try { await logout() + } catch (error) { + console.error("Failed to log out on the server", error) } finally { navigate("/login", { replace: true }) } }const handleWorkspaceSwitch = async (workspaceId: string) => { if (workspaceId === user?.workspace.id || switchingWorkspaceId) return setSwitchingWorkspaceId(workspaceId) try { const session = await switchWorkspace(workspaceId) setSession(session) + } catch (error) { + console.error("Failed to switch workspace", error) + toast.error("Failed to switch workspace") } finally { setSwitchingWorkspaceId(null) } }As per path instructions, "Flag async/await without try-catch as MEDIUM."
Also applies to: 59-69
🤖 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 `@frontend/src/components/layout/Topbar.tsx` around lines 45 - 53, Update the async handlers signOut and handleWorkspaceSwitch to add catch blocks around logout() and switchWorkspace() failures. Handle the rejected errors through the component’s existing user-feedback mechanism, such as the current toast or error state, while preserving the existing cleanup and navigation behavior in finally.Source: Path instructions
frontend/src/features/warehouses/pages/WarehousesPage.tsx-90-100 (1)
90-100: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDuplicate
namefield — second warehouse attribute is never captured.Both the catalog list item and the "Add warehouse" form duplicate the same
namefield instead of showing/capturing a distinct second attribute:
- Lines 96 & 98 both render
{warehouse.name}in the list item.- Lines 121-136 render two separate
<input name="name">fields (the second label even reads lowercase"name"), soObject.fromEntries(form)collapses them into a singlenamekey — whatever second field (e.g. code/location) this form was meant to submit is silently dropped before it ever reacheswarehouseSchema.safeParse.This looks like a copy-paste leftover from the two-field template used in
ProductsPage.tsx(name + sku) that wasn't updated for warehouses.🐛 Proposed fix (adjust to the actual intended second field)
<div> <p className="font-medium">{warehouse.name}</p> <p className="text-sm text-muted-foreground"> - {warehouse.name} + {warehouse.code} </p> </div><label className="block text-sm"> - name + Code <input className="mt-1 w-full rounded-md border bg-background p-2" - name="name" + name="code" required /> </label>Also applies to: 121-136
🤖 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 `@frontend/src/features/warehouses/pages/WarehousesPage.tsx` around lines 90 - 100, Replace the duplicated warehouse.name rendering in the warehouse list and the second name input in the add-warehouse form with the intended distinct warehouse attribute, using the corresponding property, label, and form field name consistently. Update the related submission/schema handling so Object.fromEntries preserves that second field and warehouseSchema.safeParse receives both warehouse values.frontend/src/features/warehouses/pages/WarehousesPage.tsx-55-63 (1)
55-63: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winUncontrolled inputs reset on failed submit
<form action={submit}>clears these uncontrolled fields as soon as the action returns, even whensafeParsefails orcreate.mutaterejects. UsemutateAsync/await(or switch toonSubmit/controlled inputs) if the entered values should stay visible after an error.🤖 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 `@frontend/src/features/warehouses/pages/WarehousesPage.tsx` around lines 55 - 63, Update the submit function around warehouseSchema.safeParse and create.mutate so the form action does not clear uncontrolled inputs when validation or mutation fails. Use an async mutation flow with mutateAsync and await its result, preserving the existing validation error handling while keeping entered values visible after failures.frontend/src/features/products/pages/ProductsPage.tsx-55-63 (1)
55-63: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winKeep the form pending until the mutation settles.
submitreturns synchronously here, so React 19 clears the uncontrolledname/skuinputs as soon as the action finishes — even on validation failures or async mutation errors. Switch this to an async action (mutateAsync/await) oronSubmitso failed submits keep the typed values. Same root cause infrontend/src/features/warehouses/pages/WarehousesPage.tsx.🤖 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 `@frontend/src/features/products/pages/ProductsPage.tsx` around lines 55 - 63, Update the submit handlers in ProductsPage and WarehousesPage to remain pending until validation and the asynchronous mutation settle. Use an async action with mutateAsync/await or move submission to onSubmit, ensuring validation failures and mutation errors preserve the uncontrolled name/sku inputs while successful submissions retain current behavior.frontend/src/features/purchases/pages/PurchasesPage.tsx-1-1 (1)
1-1: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win"Retry" button can silently resubmit stale form data. In both pages, the retry payload is snapshotted once inside
onErrorand shown via a "Retry" button, but none of the fieldonChangehandlers clear or refresh that snapshot — so if the user edits any select/input after a failed submit, clicking "Retry" resends the old values, not what's currently visible in the form.
frontend/src/features/purchases/pages/PurchasesPage.tsx#L77-80: clearretryReceipt(or recompute it) wheneversupplierId/warehouseId/productId/quantitychange after an error, so the button always reflects current form state (or hide it once inputs diverge from the stored snapshot).frontend/src/features/purchases/pages/PurchasesPage.tsx#L193-202: guard the "Retry receipt" button so it only renders/fires whenretryReceiptstill matches the current form values.frontend/src/features/sales/pages/SalesPage.tsx#L70-73: apply the same fix toretryFulfillment.frontend/src/features/sales/pages/SalesPage.tsx#L165-174: apply the same guard to the "Retry fulfillment" button.🤖 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 `@frontend/src/features/purchases/pages/PurchasesPage.tsx` at line 1, Update the retry state and button logic in PurchasesPage and SalesPage so editing any form field after an error invalidates the stored retry payload, or prevents retry unless it still matches the current values. Cover all listed supplier, warehouse, product, quantity, and sales fulfillment fields in their onChange handlers, and guard the respective retry buttons and callbacks using the current form state.frontend/src/features/suppliers/pages/SuppliersPage.tsx-55-63 (1)
55-63: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winForm clears the
nameinput immediately on submit, even when the create fails.
submitis a plain synchronous function passed to<form action={submit}>; it firescreate.mutate(...)without awaiting it. React 19 automatically resets uncontrolled form fields once the action function returns — which for a sync function happens immediately, not after the async mutation settles. So thenamefield is cleared right away regardless of whethercreateSupplierlater succeeds or fails, leaving the user with an error message but no way to see/correct what they typed.🛠️ Suggested fix — make the action await the mutation so React only resets on actual completion
- const submit = (form: FormData) => { + const submit = async (form: FormData) => { const parsed = supplierSchema.safeParse(Object.fromEntries(form)) if (!parsed.success) return setError( parsed.error.issues[0]?.message ?? "Check the supplier details." ) setError("") - create.mutate(parsed.data) + await create.mutateAsync(parsed.data).catch(() => {}) }Note this still resets on failure per React 19 semantics; if you want the input preserved on error, prefer a controlled input tied to state instead of relying on the native form reset.
Also applies to: 115-123
🤖 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 `@frontend/src/features/suppliers/pages/SuppliersPage.tsx` around lines 55 - 63, Update the submit function to be asynchronous and await the create.mutate operation before returning, so the React 19 form action remains pending until the mutation settles. Preserve the existing validation and error handling, and update the mutation invocation in submit rather than unrelated form fields or components.Source: Path instructions
backend/src/InventoryFlow.Infrastructure/Persistence/Configurations/InventoryBalanceConfiguration.cs-16-18 (1)
16-18: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftEnforce same-workspace foreign keys for inventory balances.
The balance carries
WorkspaceId, but the warehouse and product relationships constrain onlyWarehouseIdandProductId. This permits a row to combine one workspace with a warehouse or product from another workspace, undermining tenant isolation if a writer or query trusts the persisted relationship.Add composite alternate keys/foreign keys on
(WorkspaceId, Id)for workspace-scoped warehouses and products, or enforce the ownership check atomically before saving and reflect that constraint in the migration.🤖 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 `@backend/src/InventoryFlow.Infrastructure/Persistence/Configurations/InventoryBalanceConfiguration.cs` around lines 16 - 18, Update InventoryBalanceConfiguration to enforce workspace ownership atomically: configure composite alternate keys on Warehouse and Product using (WorkspaceId, Id), then map the balance relationships with composite foreign keys (WorkspaceId, WarehouseId) and (WorkspaceId, ProductId), preserving the existing delete behaviors. Ensure the corresponding database migration reflects these composite constraints.backend/src/InventoryFlow.Infrastructure/Persistence/Configurations/WorkspaceInvitationConfiguration.cs-25-27 (1)
25-27: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftDo not let expired invitations occupy the active unique slot.
The filtered index treats an invitation as active whenever
AcceptedAtUtcandRevokedAtUtcare null, even afterExpiresAtUtchas passed. A resend to the same workspace/email can therefore fail with a unique-key violation until the old invitation is explicitly revoked or accepted.Use a persisted active/status state that is updated when invitations expire, or make expiry cleanup and insertion atomic. Avoid relying on a moving
GETUTCDATE()predicate in a SQL Server filtered index.🤖 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 `@backend/src/InventoryFlow.Infrastructure/Persistence/Configurations/WorkspaceInvitationConfiguration.cs` around lines 25 - 27, Update the unique filtered index in WorkspaceInvitationConfiguration so expired invitations no longer occupy the active workspace/email slot. Use a persisted active or status state maintained when invitations expire, or coordinate expiry cleanup atomically with insertion; do not use a moving GETUTCDATE() predicate in the SQL Server filter.backend/src/InventoryFlow.Infrastructure/Persistence/Configurations/InventoryMovementConfiguration.cs-11-12 (1)
11-12: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winAdd required null checks to each public
Configuremethod.
backend/src/InventoryFlow.Infrastructure/Persistence/Configurations/InventoryMovementConfiguration.cs#L11-L12: validatebuilderbefore configuring the entity.backend/src/InventoryFlow.Infrastructure/Persistence/Configurations/ProductConfiguration.cs#L11-L12: validatebuilderbefore configuring the entity.backend/src/InventoryFlow.Infrastructure/Persistence/Configurations/PurchaseReceiptConfiguration.cs#L9-L10: validatebuilderbefore configuring the entity.backend/src/InventoryFlow.Infrastructure/Persistence/Configurations/RefreshTokenConfiguration.cs#L14-L15: validatebuilderbefore configuring the entity.backend/src/InventoryFlow.Infrastructure/Persistence/Configurations/SalesFulfillmentConfiguration.cs#L9-L10: validatebuilderbefore configuring the entity.backend/src/InventoryFlow.Infrastructure/Persistence/Configurations/SupplierConfiguration.cs#L11-L12: validatebuilderbefore configuring the entity.backend/src/InventoryFlow.Infrastructure/Persistence/Configurations/WarehouseConfiguration.cs#L4-L4: validatebbefore configuring the entity.backend/src/InventoryFlow.Infrastructure/Persistence/Configurations/WarehouseTransferConfiguration.cs#L9-L10: validatebuilderbefore configuring the entity.backend/src/InventoryFlow.Infrastructure/Persistence/Configurations/WorkspaceConfiguration.cs#L11-L12: validatebuilderbefore configuring the entity.Use
System.ArgumentNullException.ThrowIfNull(builder);at each method entry. As per path instructions, "**/*.cs: Flag missing null checks as HIGH."🤖 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 `@backend/src/InventoryFlow.Infrastructure/Persistence/Configurations/InventoryMovementConfiguration.cs` around lines 11 - 12, Each public Configure method must validate its entity builder at method entry before applying configuration. Add ArgumentNullException.ThrowIfNull(builder) in InventoryMovementConfiguration.cs, ProductConfiguration.cs, PurchaseReceiptConfiguration.cs, RefreshTokenConfiguration.cs, SalesFulfillmentConfiguration.cs, SupplierConfiguration.cs, WarehouseTransferConfiguration.cs, and WorkspaceConfiguration.cs; in WarehouseConfiguration.cs, validate the parameter named b instead. Apply the same guard to all listed sites without changing the remaining configuration logic.Source: Path instructions
backend/src/InventoryFlow.Infrastructure/Persistence/Migrations/20260718122029_AddInventoryLedger.cs-26-80 (1)
26-80: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftEnforce workspace consistency in ledger foreign keys.
These FKs validate
ProductIdandWarehouseIdindependently, so a ledger row can useWorkspaceIdA with a product or warehouse owned by workspace B. This breaks the tenancy boundary and can corrupt balances/movements. Add composite principal keys and foreign keys for(WorkspaceId, ProductId)and(WorkspaceId, WarehouseId), update the EF configurations, then regenerate the migrations.🤖 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 `@backend/src/InventoryFlow.Infrastructure/Persistence/Migrations/20260718122029_AddInventoryLedger.cs` around lines 26 - 80, Update the ledger entity configurations and migration for InventoryBalances and InventoryMovements so ProductId and WarehouseId are constrained together with WorkspaceId via composite principal keys and composite foreign keys. Add the corresponding composite alternate keys on Products and Warehouses, remove the independent product/warehouse foreign keys, and regenerate the migration so cross-workspace ledger references are rejected.backend/src/InventoryFlow.Infrastructure/Persistence/Migrations/20260719073620_AddCollaborationFoundation.cs-116-121 (1)
116-121: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winAllow invitations to be reissued after expiry.
This index continues to reserve
(WorkspaceId, NormalizedEmail)afterExpiresAtUtcpasses.EfCollaborationService.CreateInvitationAsyncthen returns a duplicate conflict instead of creating a replacement, although expiry makes the invitation non-pending. Atomically revoke/replace expired invitations before inserting, while retaining concurrency-safe uniqueness, then regenerate this migration/model.🤖 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 `@backend/src/InventoryFlow.Infrastructure/Persistence/Migrations/20260719073620_AddCollaborationFoundation.cs` around lines 116 - 121, Update EfCollaborationService.CreateInvitationAsync and the WorkspaceInvitations uniqueness configuration so expired invitations no longer block reissuance: atomically revoke or replace expired records before inserting the new invitation, while preserving concurrency-safe uniqueness for active, non-revoked invitations. Regenerate the corresponding EF migration and model so the filtered index reflects the pending-state rules, including ExpiresAtUtc.backend/tests/InventoryFlow.IntegrationTests/Api/InventoryEndpointsTests.cs-200-213 (1)
200-213: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winMissing null check before deserializing responses.
PostAsync<T>andRegisterSessionAsyncnull-forgive (!) the deserialized body without asserting it's non-null. A malformed/empty response body will throw a rawNullReferenceExceptioninstead of a clear assertion failure, making CI failures harder to diagnose — unlike theAssert.IsType<T>(...)pattern already used elsewhere in this test suite (e.g.,CollaborationEndpointsTests.cs).✅ Suggested fix
- return (await response.Content.ReadFromJsonAsync<T>())!; + var payload = await response.Content.ReadFromJsonAsync<T>(); + return Assert.IsType<T>(payload);Apply the same pattern to
RegisterSessionAsync(line 212).As per path instructions, "Flag missing null checks as HIGH."
🤖 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 `@backend/tests/InventoryFlow.IntegrationTests/Api/InventoryEndpointsTests.cs` around lines 200 - 213, HIGH: Add explicit non-null assertions for deserialized response bodies in PostAsync<T> and RegisterSessionAsync, replacing the null-forgiving operators with the test suite’s established assertion pattern (such as Assert.IsType<T>(...)); preserve the existing status-code checks and return the validated deserialized values.Source: Path instructions
backend/tests/InventoryFlow.IntegrationTests/Api/AuthenticatedApiFixture.cs-39-69 (1)
39-69: 🩺 Stability & Availability | 🟠 Major | ⚖️ Poor tradeoffNo cancellation/timeout on container startup and migration.
InitializeAsyncawaits_sqlServer.StartAsync(), database creation, andMigrateAsync()with noCancellationTokenor timeout. A stuck container pull/start or a hung migration will block the test run indefinitely rather than failing fast.🕒 Suggested fix
- public async Task InitializeAsync() + public async Task InitializeAsync(CancellationToken cancellationToken = default) { - await _sqlServer.StartAsync(); + using var timeoutCts = new CancellationTokenSource(TimeSpan.FromMinutes(2)); + using var linked = CancellationTokenSource.CreateLinkedTokenSource(cancellationToken, timeoutCts.Token); + await _sqlServer.StartAsync(linked.Token);As per path instructions, "Flag async/await without proper cancellation token handling as MEDIUM."
🤖 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 `@backend/tests/InventoryFlow.IntegrationTests/Api/AuthenticatedApiFixture.cs` around lines 39 - 69, Update AuthenticatedApiFixture.InitializeAsync to apply a bounded cancellation timeout across _sqlServer.StartAsync, database creation, and ApplicationDbContext.Database.MigrateAsync. Create and manage a CancellationTokenSource for the initialization scope, pass its token to each supported async operation, and ensure timeout cancellation causes the test setup to fail promptly rather than waiting indefinitely.Source: Path instructions
backend/src/InventoryFlow.Infrastructure/Products/EfProductCatalog.cs-42-65 (1)
42-65: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftMake the archive retry state-safe.
ArchiveAsyncreuses the samedbContextacross retries. BecauseProduct.Archive()is idempotent, a retry can keep the trackedproductmarked archived and returntrueeven if the prior transaction rolled back, leaving the row unarchived. Use a fresh context per attempt or a transaction helper that verifies commit success. Also passcancellationTokenintoExecuteAsync(...).🤖 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 `@backend/src/InventoryFlow.Infrastructure/Products/EfProductCatalog.cs` around lines 42 - 65, Update ArchiveAsync to make each execution-strategy retry state-safe by creating and using a fresh DbContext within every attempt, rather than reusing the tracked context and product across retries. Pass cancellationToken to the strategy’s ExecuteAsync call, and preserve the existing archive, conflict, rollback, and success behavior while ensuring true is returned only after a successful commit.Sources: Path instructions, MCP tools
backend/src/InventoryFlow.Infrastructure/Inventory/InventoryLedgerWriter.cs-17-19 (1)
17-19: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winIdempotency short-circuits match only on the idempotency key, never validating the rest of the request. All four write paths look up an existing record by
WorkspaceId+IdempotencyKeyalone and return it immediately on a hit, without comparing the other request parameters (warehouse/product/type/quantity, supplier, or source/destination). A key reused with different parameters silently returns the stale record instead of surfacing a conflict, which can mask client bugs or hide a real business-logic mismatch behind an apparently-successful response.
backend/src/InventoryFlow.Infrastructure/Inventory/InventoryLedgerWriter.cs#L17-L19: after findingexisting, compare itsWarehouseId/ProductId/Type/Quantityto the request and throw a conflict (e.g., a dedicatedIdempotencyConflictException) on mismatch instead of returningexistingunconditionally.backend/src/InventoryFlow.Infrastructure/Purchases/EfPurchaseReceiptService.cs#L22-L28: same — validateexisting.SupplierId/WarehouseId/ProductId/Quantityagainst the request before returning it.backend/src/InventoryFlow.Infrastructure/Sales/EfSalesFulfillmentService.cs#L22-L28: same — validateexisting.WarehouseId/ProductId/Quantityagainst the request before returning it.backend/src/InventoryFlow.Infrastructure/Transfers/EfWarehouseTransferService.cs#L42-L48: same — validateexisting.SourceWarehouseId/DestinationWarehouseId/ProductId/Quantityagainst the request before returning it.🤖 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 `@backend/src/InventoryFlow.Infrastructure/Inventory/InventoryLedgerWriter.cs` around lines 17 - 19, Idempotency hits must reject reused keys with different request parameters instead of returning stale records. In InventoryLedgerWriter.cs lines 17-19, compare WarehouseId, ProductId, Type, and Quantity before returning existing; in EfPurchaseReceiptService.cs lines 22-28, compare SupplierId, WarehouseId, ProductId, and Quantity; in EfSalesFulfillmentService.cs lines 22-28, compare WarehouseId, ProductId, and Quantity; and in EfWarehouseTransferService.cs lines 42-48, compare SourceWarehouseId, DestinationWarehouseId, ProductId, and Quantity. Throw a dedicated idempotency conflict exception on any mismatch, otherwise return the existing record.backend/src/InventoryFlow.Infrastructure/Inventory/EfInventoryLedger.cs-17-27 (1)
17-27: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winCreate a fresh
ApplicationDbContextperExecuteAsyncattempt
backend/src/InventoryFlow.Infrastructure/Inventory/EfInventoryLedger.cs,backend/src/InventoryFlow.Infrastructure/Purchases/EfPurchaseReceiptService.cs, andbackend/src/InventoryFlow.Infrastructure/Sales/EfSalesFulfillmentService.csall reuse the injected context inside the retry delegate. If a transient failure happens after an entity is tracked, the next attempt replays with staleAddedstate; in the purchase/sales flows the newGuid.NewGuid()movement key also makes the replay capable of posting a second inventory movement and double-applying stock.- Mirror
backend/src/InventoryFlow.Infrastructure/Transfers/EfWarehouseTransferService.csand resolve a freshApplicationDbContextinside the delegate for each attempt.🤖 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 `@backend/src/InventoryFlow.Infrastructure/Inventory/EfInventoryLedger.cs` around lines 17 - 27, Update the retry delegates in backend/src/InventoryFlow.Infrastructure/Inventory/EfInventoryLedger.cs (lines 17-27), backend/src/InventoryFlow.Infrastructure/Purchases/EfPurchaseReceiptService.cs (lines 18-19), and backend/src/InventoryFlow.Infrastructure/Sales/EfSalesFulfillmentService.cs (lines 18-19) to resolve a fresh ApplicationDbContext inside each ExecuteAsync attempt, mirroring EfWarehouseTransferService. Use that per-attempt context for transactions, writers, entity tracking, saves, and commits so retries do not reuse stale state or generate duplicate movements.
🟡 Minor comments (10)
docker-compose.yml-20-22 (1)
20-22: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winExpose the SDK pin to the backend Docker build
docker-compose.yml#L20-L22:build.context: ./backendexcludes the repo-rootglobal.json, so the container build falls back to the floatingmcr.microsoft.com/dotnet/sdk:9.0base inbackend/Dockerfile.global.json#L1-L6:"rollForward": "latestPatch"still allows patch drift; set it to"disable"if the build must stay on9.0.316.🤖 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 `@docker-compose.yml` around lines 20 - 22, Update docker-compose.yml build configuration to use the repository root as the Docker build context so backend/Dockerfile can access global.json. In global.json, change rollForward to disable to pin the SDK to the specified 9.0.316 version; apply these changes at docker-compose.yml lines 20-22 and global.json lines 1-6.backend/src/InventoryFlow.Api/Controllers/InventoryController.cs-27-27 (1)
27-27: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUse a routable
Locationtarget for theseCreated(...)responses.These responses point at detail URLs that the API does not expose:
backend/src/InventoryFlow.Api/Controllers/InventoryController.cs#L27backend/src/InventoryFlow.Api/Controllers/InventoryController.cs#L43backend/src/InventoryFlow.Api/Controllers/ProductsController.cs#L24Add matching detail endpoints or return a
Locationthat resolves to an existing route.🤖 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 `@backend/src/InventoryFlow.Api/Controllers/InventoryController.cs` at line 27, Update the Created responses in InventoryController at backend/src/InventoryFlow.Api/Controllers/InventoryController.cs lines 27-27 and 43-43, and ProductsController at backend/src/InventoryFlow.Api/Controllers/ProductsController.cs line 24, so each Location targets an existing routable detail endpoint; alternatively add matching detail actions for those URLs. Preserve the existing NotFound behavior and response bodies.backend/src/InventoryFlow.Api/ExceptionHandling/GlobalExceptionHandler.cs-73-79 (1)
73-79: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winMissing explicit
TypeURI case for 401.
Status401Unauthorizedisn't listed, so it falls through to the_default, which is the RFC 9110 section for 500 Server Error (#section-15.6.1) rather than the 401 section (#section-15.5.2). Title switch (line 72) already handles 401 correctly — Type switch is inconsistent with it.🩹 Proposed fix
Type = statusCode switch { StatusCodes.Status400BadRequest => "https://tools.ietf.org/html/rfc9110#section-15.5.1", + StatusCodes.Status401Unauthorized => "https://tools.ietf.org/html/rfc9110#section-15.5.2", StatusCodes.Status403Forbidden => "https://tools.ietf.org/html/rfc9110#section-15.5.4", StatusCodes.Status409Conflict => "https://tools.ietf.org/html/rfc9110#section-15.5.10", _ => "https://tools.ietf.org/html/rfc9110#section-15.6.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 `@backend/src/InventoryFlow.Api/ExceptionHandling/GlobalExceptionHandler.cs` around lines 73 - 79, Add an explicit StatusCodes.Status401Unauthorized case to the Type switch in GlobalExceptionHandler, mapping it to the RFC 9110 URI for section 15.5.2. Leave the existing mappings and default case unchanged.backend/src/InventoryFlow.Application/Features/Authentication/AuthenticationValidators.cs-20-24 (1)
20-24: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAdd missing auth-scope validation.
GetCurrentUserQueryValidatorshould also requireWorkspaceId, andSwitchWorkspaceCommandneeds validation forUserId,WorkspaceId, andRefreshToken.🤖 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 `@backend/src/InventoryFlow.Application/Features/Authentication/AuthenticationValidators.cs` around lines 20 - 24, Update GetCurrentUserQueryValidator to require both UserId and WorkspaceId, and add or update the SwitchWorkspaceCommand validator to require UserId, WorkspaceId, and RefreshToken using the existing NotEmpty validation style.frontend/src/features/dashboard/pages/DashboardPage.tsx-102-102 (1)
102-102: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winMake the Export report action functional or disable it.
This button has no click handler or navigation target, so it silently does nothing.
🤖 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 `@frontend/src/features/dashboard/pages/DashboardPage.tsx` at line 102, Update the Export report Button in DashboardPage so it performs the intended report-export action by wiring it to the existing export handler or navigation target; if no implementation is available, render the button disabled instead of leaving it inert.frontend/src/features/dashboard/components/MetricCard.tsx-28-28 (1)
28-28: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winSeparate metric direction from favorable sentiment.
A decrease in “Low stock items” is beneficial, but this component always renders
downas destructive. Add an explicit sentiment/status prop and mark this metric as positive.
frontend/src/features/dashboard/components/MetricCard.tsx#L28-L28: derive visual color and indicator from an explicit favorable/unfavorable value rather than direction.frontend/src/features/dashboard/components/MetricCard.tsx#L46-L49: use that sentiment for the text color.frontend/src/features/dashboard/pages/DashboardPage.tsx#L63-L67: mark the low-stock decrease as favorable.🤖 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 `@frontend/src/features/dashboard/components/MetricCard.tsx` at line 28, Update MetricCard to accept an explicit favorable/unfavorable sentiment prop, and derive the trend indicator and text color from that sentiment instead of assuming trend "up" is always positive. In frontend/src/features/dashboard/components/MetricCard.tsx lines 28-28 and 46-49, use the sentiment for indicator and color selection; in frontend/src/features/dashboard/pages/DashboardPage.tsx lines 63-67, mark the low-stock decrease as favorable.backend/tests/InventoryFlow.IntegrationTests/Api/InventoryFlowApiFactory.cs-19-33 (1)
19-33: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winJwt env vars set in the constructor are never restored on Dispose.
Only
_originalConnectionStringis captured and restored;Jwt__SigningKey,Jwt__Issuer, andJwt__Audienceset in the constructor (lines 30-32) are left in the process environment afterDispose, unlike the more complete save/restore pattern inAuthenticatedApiFixture.cs.♻️ Suggested fix
private readonly string? _originalConnectionString = Environment.GetEnvironmentVariable( ConnectionStringEnvironmentVariable); + private readonly string? _originalSigningKey = Environment.GetEnvironmentVariable("Jwt__SigningKey"); + private readonly string? _originalIssuer = Environment.GetEnvironmentVariable("Jwt__Issuer"); + private readonly string? _originalAudience = Environment.GetEnvironmentVariable("Jwt__Audience"); ... if (disposing) { Environment.SetEnvironmentVariable( ConnectionStringEnvironmentVariable, _originalConnectionString); + Environment.SetEnvironmentVariable("Jwt__SigningKey", _originalSigningKey); + Environment.SetEnvironmentVariable("Jwt__Issuer", _originalIssuer); + Environment.SetEnvironmentVariable("Jwt__Audience", _originalAudience); }Also applies to: 67-77
🤖 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 `@backend/tests/InventoryFlow.IntegrationTests/Api/InventoryFlowApiFactory.cs` around lines 19 - 33, Update InventoryFlowApiFactory to capture the original Jwt__SigningKey, Jwt__Issuer, and Jwt__Audience values alongside _originalConnectionString, then restore each original value in Dispose using the existing environment-variable cleanup pattern from AuthenticatedApiFixture. Preserve restoration of the connection string and ensure unset variables are returned to an unset state.backend/Dockerfile-1-1 (1)
1-1: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winPin the .NET image tags to the repo’s exact SDK version
global.jsonis locked to9.0.316, butbackend/Dockerfileusesmcr.microsoft.com/dotnet/sdk:9.0andmcr.microsoft.com/dotnet/aspnet:9.0, so builds can drift to a different patch than the repo pin. Align both images with the pinned version, or updateglobal.jsonif rolling patches are 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 `@backend/Dockerfile` at line 1, Update the Dockerfile image tags used by the base SDK and ASP.NET runtime stages to the repository’s pinned .NET SDK version 9.0.316, keeping both stages aligned with global.json instead of using floating 9.0 tags.backend/src/InventoryFlow.Infrastructure/Products/EfProductCatalog.cs-42-43 (1)
42-43: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winPropagate
cancellationTokento the execution strategy.
ExecuteAsyncis called without the token, so retry delays can continue after the request is canceled. Use the cancellation-aware overload here, orExecuteInTransactionAsync(..., cancellationToken)if you want EF to manage the transaction.🤖 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 `@backend/src/InventoryFlow.Infrastructure/Products/EfProductCatalog.cs` around lines 42 - 43, Update the execution-strategy call in the product catalog method around CreateExecutionStrategy and ExecuteAsync to use the cancellation-aware overload, passing the existing cancellationToken through retry execution and delays. Alternatively, use ExecuteInTransactionAsync with the same token if EF should manage the transaction.Sources: Path instructions, MCP tools
backend/src/InventoryFlow.Application/Features/Warehouses/WarehouseValidators.cs-3-3 (1)
3-3: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winValidate the normalized warehouse name length.
Warehouse.NormalizeNametrims before enforcingNameMaxLength(backend/src/InventoryFlow.Domain/Entities/Warehouse.cs:4), but this validator measures the raw input. A valid 200-character canonical name with surrounding spaces is rejected before reaching the domain. Validatename.Trim().Lengthor normalize before validation so both layers enforce the same contract.🤖 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 `@backend/src/InventoryFlow.Application/Features/Warehouses/WarehouseValidators.cs` at line 3, Update CreateWarehouseCommandValidator’s Name rule to measure the trimmed/normalized warehouse name before applying Warehouse.NameMaxLength, while retaining the existing non-empty validation so it matches Warehouse.NormalizeName’s contract.
…idempotency validation, session epoch, and cleanup
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
backend/src/InventoryFlow.Infrastructure/Purchases/EfPurchaseReceiptService.cs (1)
14-84: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick winDuplicate/orphaned code block breaks compilation.
Lines 59-83 duplicate the persistence logic already handled inside the
strategy.ExecuteAsync(...)lambda (lines 24-57), but sit after thereturn await strategy.ExecuteAsync(...)statement closes at line 58 — making them unreachable, and they referencetransactionanddbContextdirectly, neither of which exists in that outer scope (transactionwas declared inside the lambda at line 23, scoped toattemptCtx). This is a leftover from the refactor to per-attempt scoped contexts that was never removed, and it will not compile (CS0103: The name 'transaction' does not exist in the current context).🐛 Proposed fix: remove the orphaned tail block
var receipt = new PurchaseReceipt(receiptId, workspaceId, supplierId, warehouseId, productId, quantity, idempotencyKey, movement.Id, receivedAtUtc); attemptCtx.PurchaseReceipts.Add(receipt); await attemptCtx.SaveChangesAsync(cancellationToken); await transaction.CommitAsync(cancellationToken); return receipt; }); - - var supplierExists = await dbContext.Suppliers.AnyAsync(supplier => supplier.Id == supplierId && - supplier.WorkspaceId == workspaceId && supplier.ArchivedAtUtc == null, cancellationToken); - if (!supplierExists) - { - await transaction.RollbackAsync(cancellationToken); - return null; - } - - var receiptId = Guid.NewGuid(); - var movement = await new InventoryLedgerWriter(dbContext).RecordAsync(workspaceId, warehouseId, productId, - InventoryMovementType.Receipt, quantity, receiptId.ToString("N"), receivedAtUtc, Guid.NewGuid(), cancellationToken); - if (movement is null) - { - await transaction.RollbackAsync(cancellationToken); - return null; - } - - var receipt = new PurchaseReceipt(receiptId, workspaceId, supplierId, warehouseId, productId, quantity, idempotencyKey, - movement.Id, receivedAtUtc); - dbContext.PurchaseReceipts.Add(receipt); - await dbContext.SaveChangesAsync(cancellationToken); - await transaction.CommitAsync(cancellationToken); - return receipt; - }); }🤖 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 `@backend/src/InventoryFlow.Infrastructure/Purchases/EfPurchaseReceiptService.cs` around lines 14 - 84, Remove the orphaned persistence block after the return from strategy.ExecuteAsync in RecordAsync, including its duplicate supplier check, ledger write, receipt creation, save, rollback, and commit logic. Keep the complete implementation inside the ExecuteAsync lambda unchanged, using attemptCtx and its transaction.frontend/src/features/warehouses/pages/WarehousesPage.tsx (1)
18-26: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUse operation-specific error fallbacks.
readErroris also used for the warehouse-list failure at Line 82 and archive failure at Line 52, but its fallback always says “Unable to save the warehouse.” List and archive failures therefore display misleading messages.Proposed fix
-const readError = (error: unknown) => { +const readError = (error: unknown, fallback = "Unable to save the warehouse.") => { ... - return "Unable to save the warehouse." + return fallback }Pass
"Unable to load warehouses."and"Unable to archive the warehouse."at the respective call sites.🤖 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 `@frontend/src/features/warehouses/pages/WarehousesPage.tsx` around lines 18 - 26, Update readError to accept an operation-specific fallback message, then pass “Unable to load warehouses.” at the warehouse-list failure call site and “Unable to archive the warehouse.” at the archive failure call site; preserve the existing save fallback for the save operation.
🤖 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
`@backend/src/InventoryFlow.Infrastructure/Purchases/EfPurchaseReceiptService.cs`:
- Around line 19-23: Update the execution-strategy callback in
EfPurchaseReceiptService to capture the IServiceScope returned by
scopeFactory.CreateScope() and dispose it for every attempt, preferably with a
using declaration or statement. Resolve ApplicationDbContext from that disposed
scope and preserve the existing transaction and retry behavior.
In `@frontend/src/features/warehouses/pages/WarehousesPage.tsx`:
- Around line 55-63: Remove the empty catch from the mutateAsync call in submit
so creation failures remain rejected and the form action does not resolve
successfully; preserve the existing validation and setError behavior.
---
Outside diff comments:
In
`@backend/src/InventoryFlow.Infrastructure/Purchases/EfPurchaseReceiptService.cs`:
- Around line 14-84: Remove the orphaned persistence block after the return from
strategy.ExecuteAsync in RecordAsync, including its duplicate supplier check,
ledger write, receipt creation, save, rollback, and commit logic. Keep the
complete implementation inside the ExecuteAsync lambda unchanged, using
attemptCtx and its transaction.
In `@frontend/src/features/warehouses/pages/WarehousesPage.tsx`:
- Around line 18-26: Update readError to accept an operation-specific fallback
message, then pass “Unable to load warehouses.” at the warehouse-list failure
call site and “Unable to archive the warehouse.” at the archive failure call
site; preserve the existing save fallback for the save operation.
🪄 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 Plus
Run ID: 76e068a0-50a8-4230-bd8b-8f33f1bdaa3d
📒 Files selected for processing (37)
backend/Dockerfilebackend/src/InventoryFlow.Api/ExceptionHandling/GlobalExceptionHandler.csbackend/src/InventoryFlow.Application/Common/Tenancy/CurrentWorkspace.csbackend/src/InventoryFlow.Application/Features/Authentication/AuthenticationValidators.csbackend/src/InventoryFlow.Application/Features/Warehouses/WarehouseValidators.csbackend/src/InventoryFlow.Domain/Entities/PurchaseReceipt.csbackend/src/InventoryFlow.Infrastructure/Inventory/EfInventoryLedger.csbackend/src/InventoryFlow.Infrastructure/Inventory/InventoryLedgerWriter.csbackend/src/InventoryFlow.Infrastructure/Persistence/Configurations/InventoryBalanceConfiguration.csbackend/src/InventoryFlow.Infrastructure/Persistence/Configurations/InventoryMovementConfiguration.csbackend/src/InventoryFlow.Infrastructure/Persistence/Configurations/ProductConfiguration.csbackend/src/InventoryFlow.Infrastructure/Persistence/Configurations/PurchaseReceiptConfiguration.csbackend/src/InventoryFlow.Infrastructure/Persistence/Configurations/RefreshTokenConfiguration.csbackend/src/InventoryFlow.Infrastructure/Persistence/Configurations/SalesFulfillmentConfiguration.csbackend/src/InventoryFlow.Infrastructure/Persistence/Configurations/SupplierConfiguration.csbackend/src/InventoryFlow.Infrastructure/Persistence/Configurations/WarehouseConfiguration.csbackend/src/InventoryFlow.Infrastructure/Persistence/Configurations/WarehouseTransferConfiguration.csbackend/src/InventoryFlow.Infrastructure/Persistence/Configurations/WorkspaceConfiguration.csbackend/src/InventoryFlow.Infrastructure/Persistence/Configurations/WorkspaceInvitationConfiguration.csbackend/src/InventoryFlow.Infrastructure/Products/EfProductCatalog.csbackend/src/InventoryFlow.Infrastructure/Purchases/EfPurchaseReceiptService.csbackend/src/InventoryFlow.Infrastructure/Sales/EfSalesFulfillmentService.csbackend/src/InventoryFlow.Infrastructure/Transfers/EfWarehouseTransferService.csbackend/tests/InventoryFlow.IntegrationTests/Api/AuthenticatedApiFixture.csbackend/tests/InventoryFlow.IntegrationTests/Api/InventoryEndpointsTests.csbackend/tests/InventoryFlow.IntegrationTests/Api/InventoryFlowApiFactory.csdocker-compose.ymlfrontend/src/components/layout/Topbar.tsxfrontend/src/features/dashboard/components/MetricCard.tsxfrontend/src/features/dashboard/pages/DashboardPage.tsxfrontend/src/features/products/pages/ProductsPage.tsxfrontend/src/features/purchases/pages/PurchasesPage.tsxfrontend/src/features/sales/pages/SalesPage.tsxfrontend/src/features/suppliers/pages/SuppliersPage.tsxfrontend/src/features/warehouses/pages/WarehousesPage.tsxfrontend/src/lib/api-client.tsglobal.json
🚧 Files skipped from review as they are similar to previous changes (33)
- backend/src/InventoryFlow.Infrastructure/Persistence/Configurations/WorkspaceConfiguration.cs
- backend/src/InventoryFlow.Infrastructure/Persistence/Configurations/SupplierConfiguration.cs
- backend/src/InventoryFlow.Application/Features/Warehouses/WarehouseValidators.cs
- backend/src/InventoryFlow.Infrastructure/Persistence/Configurations/InventoryBalanceConfiguration.cs
- backend/src/InventoryFlow.Infrastructure/Inventory/InventoryLedgerWriter.cs
- frontend/src/features/products/pages/ProductsPage.tsx
- backend/src/InventoryFlow.Infrastructure/Persistence/Configurations/RefreshTokenConfiguration.cs
- backend/src/InventoryFlow.Infrastructure/Persistence/Configurations/ProductConfiguration.cs
- backend/src/InventoryFlow.Infrastructure/Persistence/Configurations/WorkspaceInvitationConfiguration.cs
- backend/src/InventoryFlow.Application/Common/Tenancy/CurrentWorkspace.cs
- backend/src/InventoryFlow.Infrastructure/Persistence/Configurations/SalesFulfillmentConfiguration.cs
- backend/src/InventoryFlow.Infrastructure/Persistence/Configurations/InventoryMovementConfiguration.cs
- backend/src/InventoryFlow.Infrastructure/Persistence/Configurations/WarehouseTransferConfiguration.cs
- backend/src/InventoryFlow.Infrastructure/Inventory/EfInventoryLedger.cs
- backend/src/InventoryFlow.Infrastructure/Persistence/Configurations/PurchaseReceiptConfiguration.cs
- frontend/src/features/dashboard/components/MetricCard.tsx
- backend/src/InventoryFlow.Infrastructure/Persistence/Configurations/WarehouseConfiguration.cs
- backend/tests/InventoryFlow.IntegrationTests/Api/AuthenticatedApiFixture.cs
- backend/src/InventoryFlow.Domain/Entities/PurchaseReceipt.cs
- frontend/src/features/sales/pages/SalesPage.tsx
- backend/tests/InventoryFlow.IntegrationTests/Api/InventoryFlowApiFactory.cs
- frontend/src/components/layout/Topbar.tsx
- docker-compose.yml
- backend/src/InventoryFlow.Infrastructure/Sales/EfSalesFulfillmentService.cs
- frontend/src/features/dashboard/pages/DashboardPage.tsx
- frontend/src/features/purchases/pages/PurchasesPage.tsx
- backend/Dockerfile
- backend/src/InventoryFlow.Infrastructure/Transfers/EfWarehouseTransferService.cs
- backend/src/InventoryFlow.Api/ExceptionHandling/GlobalExceptionHandler.cs
- backend/src/InventoryFlow.Infrastructure/Products/EfProductCatalog.cs
- frontend/src/features/suppliers/pages/SuppliersPage.tsx
- backend/tests/InventoryFlow.IntegrationTests/Api/InventoryEndpointsTests.cs
- frontend/src/lib/api-client.ts
…adError fallbacks
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/src/features/warehouses/pages/WarehousesPage.tsx (1)
55-63: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winSwallow the rejected mutation promise here.
create.mutateAsync(parsed.data)can reject and bubble out of theform actioncallback. The other create flows already usecatch(() => {}); do the same here so a failed save still shows theonErrormessage without surfacing an unhandled rejection.🤖 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 `@frontend/src/features/warehouses/pages/WarehousesPage.tsx` around lines 55 - 63, Update the submit function to handle rejection from create.mutateAsync(parsed.data) by swallowing the promise error, matching the catch behavior used by the other create flows; preserve the existing onError message handling and successful submission behavior.Source: Path instructions
🤖 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
`@backend/src/InventoryFlow.Infrastructure/Purchases/EfPurchaseReceiptService.cs`:
- Around line 20-24: Update the scope creation in the strategy.ExecuteAsync
callback to use the asynchronous scope factory method, CreateAsyncScope(), so
the existing await using declaration in the purchase receipt flow compiles and
disposes the scope correctly on each retry.
In `@frontend/src/features/warehouses/pages/WarehousesPage.tsx`:
- Around line 49-53: Update the WarehousesPage archive mutation and UI to use a
dedicated archiveError state instead of the add-warehouse error state. Have
archive.onError set archiveError, clear archiveError on successful archiving,
and render the archive error near the catalog controls while leaving the
add-warehouse form error handling unchanged.
---
Outside diff comments:
In `@frontend/src/features/warehouses/pages/WarehousesPage.tsx`:
- Around line 55-63: Update the submit function to handle rejection from
create.mutateAsync(parsed.data) by swallowing the promise error, matching the
catch behavior used by the other create flows; preserve the existing onError
message handling and successful submission behavior.
🪄 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 Plus
Run ID: 13e3e602-3491-4dca-a46a-a9a9daae12ce
📒 Files selected for processing (2)
backend/src/InventoryFlow.Infrastructure/Purchases/EfPurchaseReceiptService.csfrontend/src/features/warehouses/pages/WarehousesPage.tsx
Entire InventoryFlow codebase for CodeRabbit review.
Summary by CodeRabbit