fix(assets): consume ONE_WAY stock on custody release, gate activity notes server-side - #2800
Conversation
…notes server-side Two independent defects: one on quantity-tracked consumables, one on the activity surfaces. ONE_WAY consumables did not consume outside bookings. `consumptionType` was honoured only in booking check-in; `releaseQuantity` always wrote a RETURN log and handed the units back to the available pool, so using up a consumable's units left stock unchanged. `releaseQuantity` now resolves the disposition from the locked asset row: ONE_WAY writes a CONSUME log, decrements `Asset.quantity` and emits ASSET_QUANTITY_CHANGED alongside CUSTODY_RELEASED, all in the same transaction. TWO_WAY and legacy null rows are unchanged. The branch lives in the service rather than in a sibling function so the outcome is server-derived and both the web and the mobile release endpoints are fixed by one change. The custody action reads "Mark as consumed" for a consumable. Activity notes were gated client-side only. The asset and audit activity loaders gated on asset:read / audit:read, fetched the notes, and returned them in the page payload, leaving the note permission to a check in the component. Both loaders now gate on note:read / auditNote:read, matching the bookings and locations activity routes, so notes are never fetched without the permission. The Activity tab is hidden rather than routing an unpermitted user to an error. A sweep for the same pattern found the asset overview loader returning scan data that includes personal information; that fetch now goes through `getLastScanForViewer`, which applies the scan:read gate server-side. Location placements are deliberately not reconciled on the CONSUME decrement, matching the booking check-in path. Reasoning is recorded at the decrement site.
🩺 React Doctor — webappFindings on the files changed by this PR:
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a52fcaca9a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
… assets Addresses the Codex review on #2800. With the server now consuming a ONE_WAY consumable's units when a custodian's hold ends, the companion's "Release Quantity" / "Release" wording described the opposite of what happens. The sheet title, subtitle and confirm label now read "Mark as consumed" for a consumable, and the subtitle states that stock is permanently reduced. The custody row's accessibility label follows. Returnable (TWO_WAY) and legacy null assets keep the existing release wording, matching the server's own default in `resolveReleaseDisposition`. The mobile release endpoint now returns `disposition` alongside the refreshed asset, so the app can confirm what actually happened rather than re-deriving the rule from `consumptionType`. Additive: older clients ignore it, and the companion type marks it optional so a pre-disposition server reads as RETURN. This is wording only. The outcome is decided server-side from the asset row, so no client can pick the wrong disposition.
🩺 React Doctor — companionFindings on the files changed by this PR:
|
|
React Doctor — 0 errors, and all 3 warnings are pre-existing. Checked rather than assumed, since they surface only because this PR touches
Not fixing them here — splitting a ~1,100-line component is its own piece of work and would bury a security fix in an unrelated refactor. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughQuantity custody releases now support ChangesCustody disposition handling
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant CustodyUI
participant ReleaseRoute
participant releaseQuantity
participant AssetRecord
participant ActivityEvents
CustodyUI->>ReleaseRoute: submit released and consumed quantities
ReleaseRoute->>releaseQuantity: pass custody release request
releaseQuantity->>AssetRecord: resolve disposition and update stock
releaseQuantity->>ActivityEvents: write logs and quantity/custody events
releaseQuantity-->>ReleaseRoute: return consumed and returned quantities
ReleaseRoute-->>CustodyUI: show outcome-specific result
Possibly related issues
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches 💡 1📝 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
apps/webapp/app/routes/_layout+/audits.$auditId.activity.test.server.ts (2)
38-41: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the audit-service mock.
Add a
// why:comment forvi.mock("~/modules/audit/service.server", ...). State why the audit-session and assignee services are mocked in this loader test.As per coding guidelines, “Every mock in tests must be accompanied by a
// why:comment explaining the reason for mocking.”Proposed fix
+// why: isolate audit-session lookup and assignee authorization so these tests +// can verify the loader permission boundary and protected-fetch ordering. vi.mock("~/modules/audit/service.server", () => ({ getAuditSessionDetails, requireAuditAssigneeForBaseSelfService, }));🤖 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 `@apps/webapp/app/routes/_layout`+/audits.$auditId.activity.test.server.ts around lines 38 - 41, Add a concise `// why:` comment immediately above the `vi.mock` declaration in the audit activity loader test, explaining why the audit-session and assignee service dependencies are mocked while preserving the existing mock factory.Source: Coding guidelines
109-126: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest the denied assignee outcome.
The current call assertion passes if the loader fetches notes before the assignee restriction rejects. Make
requireAuditAssigneeForBaseSelfServicereject. Then assert that the loader rejects andgetAuditNoteswas not called.As per coding guidelines, “Write behavior-driven tests focusing on observable outcomes rather than implementation details.”
Proposed fix
it("keeps the assignee restriction for BASE/SELF_SERVICE", async () => { - // Switching the gated entity must not drop the orthogonal check that stops - // a BASE/SELF_SERVICE user reading an audit they are not assigned to. requirePermission.mockResolvedValue({ organizationId: "org-1", userOrganizations: [], isSelfServiceOrBase: true, }); + requireAuditAssigneeForBaseSelfService.mockImplementation(() => { + throw Object.assign(new Error("You are not assigned to this audit"), { + status: 403, + }); + }); - await loader(loaderArgs()); - - expect(requireAuditAssigneeForBaseSelfService).toHaveBeenCalledWith( - expect.objectContaining({ - userId: "user-1", - isSelfServiceOrBase: true, - auditId: "audit-1", - }) - ); + await expect(loader(loaderArgs())).rejects.toBeDefined(); + expect(getAuditNotes).not.toHaveBeenCalled(); });🤖 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 `@apps/webapp/app/routes/_layout`+/audits.$auditId.activity.test.server.ts around lines 109 - 126, Update the “keeps the assignee restriction for BASE/SELF_SERVICE” test to mock requireAuditAssigneeForBaseSelfService as rejecting, assert that loader(loaderArgs()) rejects, and verify getAuditNotes is not called. Replace the implementation-only invocation assertion with these observable outcome checks while preserving the existing permission setup.Source: Coding guidelines
apps/webapp/app/modules/scan/last-scan-for-viewer.test.ts (1)
31-58: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse overrideable test-data factories.
The fixed scan, user, organization, asset, and note records cannot vary one field per scenario. Use an existing test factory when available. Otherwise add typed local factories with overrides.
apps/webapp/app/modules/scan/last-scan-for-viewer.test.ts#L31-L58: ReplacescanRowand fixed viewer arguments with overrideable scan and viewer-argument factories.apps/webapp/app/routes/_layout+/assets.$assetId.activity.test.server.ts#L89-L112: Replace fixed loader arguments and mocked service records with overrideable factories.As per coding guidelines, tests must use factories with field overrides and avoid hardcoded test data where practical.
🤖 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 `@apps/webapp/app/modules/scan/last-scan-for-viewer.test.ts` around lines 31 - 58, Replace the fixed scanRow and args fixtures in apps/webapp/app/modules/scan/last-scan-for-viewer.test.ts:31-58 with typed factories that accept field overrides for scan data and viewer arguments, reusing an existing test factory if available. Also replace the fixed loader arguments and mocked service records in apps/webapp/app/routes/_layout+/assets.$assetId.activity.test.server.ts:89-112 with equivalent overrideable factories; update each affected test to provide only scenario-specific fields.Source: Coding guidelines
apps/webapp/app/routes/api+/assets.release-quantity-custody.ts (1)
134-136: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse one server-side audit-note formatter.
Both routes duplicate the disposition branch and Markdoc text. A later wording change can make web and mobile audit trails differ. Put the base-line formatting behind one helper that accepts
ReleaseQuantityDisposition, then append the user note in each route.
apps/webapp/app/routes/api+/assets.release-quantity-custody.ts#L134-L136: replace the local disposition template with the shared helper.apps/webapp/app/routes/api+/mobile+/custody.release-quantity.ts#L195-L197: replace the duplicate template with the same helper.As per coding guidelines, “When duplicated code patterns occur, extract focused reusable helpers; check for existing reusable logic before implementing new functionality.”
🤖 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 `@apps/webapp/app/routes/api`+/assets.release-quantity-custody.ts around lines 134 - 136, The disposition-specific audit-note formatting is duplicated across both custody release routes. Add one shared server-side helper accepting ReleaseQuantityDisposition and the required values, then use it in apps/webapp/app/routes/api+/assets.release-quantity-custody.ts lines 134-136 and apps/webapp/app/routes/api+/mobile+/custody.release-quantity.ts lines 195-197; each route should append its user note after receiving the shared base line.Source: Coding guidelines
🤖 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 `@apps/webapp/app/modules/asset/service.server.ts`:
- Around line 7938-8010: Update the `disposition === "CONSUME"` transaction to
identify the consumed units’ `AssetLocation` placement and decrement its
quantity alongside `Asset.quantity`, preserving placement totals. If placement
cannot be determined, reject the consume or apply an established reconciliation
policy before committing. Add a regression test covering a fully placed
`ONE_WAY` asset and verify asset and location quantities remain consistent.
In `@apps/webapp/app/routes/_layout`+/assets.$assetId.activity.test.server.ts:
- Around line 141-145: Update the loader rejection assertion in the
authorization test to verify the thrown result has HTTP status 403, rather than
merely being defined. Keep the existing assertion that
getPaginatedAndFilterableAssetNotes was not called, so the test checks both the
observable denial response and skipped note fetching.
---
Nitpick comments:
In `@apps/webapp/app/modules/scan/last-scan-for-viewer.test.ts`:
- Around line 31-58: Replace the fixed scanRow and args fixtures in
apps/webapp/app/modules/scan/last-scan-for-viewer.test.ts:31-58 with typed
factories that accept field overrides for scan data and viewer arguments,
reusing an existing test factory if available. Also replace the fixed loader
arguments and mocked service records in
apps/webapp/app/routes/_layout+/assets.$assetId.activity.test.server.ts:89-112
with equivalent overrideable factories; update each affected test to provide
only scenario-specific fields.
In `@apps/webapp/app/routes/_layout`+/audits.$auditId.activity.test.server.ts:
- Around line 38-41: Add a concise `// why:` comment immediately above the
`vi.mock` declaration in the audit activity loader test, explaining why the
audit-session and assignee service dependencies are mocked while preserving the
existing mock factory.
- Around line 109-126: Update the “keeps the assignee restriction for
BASE/SELF_SERVICE” test to mock requireAuditAssigneeForBaseSelfService as
rejecting, assert that loader(loaderArgs()) rejects, and verify getAuditNotes is
not called. Replace the implementation-only invocation assertion with these
observable outcome checks while preserving the existing permission setup.
In `@apps/webapp/app/routes/api`+/assets.release-quantity-custody.ts:
- Around line 134-136: The disposition-specific audit-note formatting is
duplicated across both custody release routes. Add one shared server-side helper
accepting ReleaseQuantityDisposition and the required values, then use it in
apps/webapp/app/routes/api+/assets.release-quantity-custody.ts lines 134-136 and
apps/webapp/app/routes/api+/mobile+/custody.release-quantity.ts lines 195-197;
each route should append its user note after receiving the shared base line.
🪄 Autofix
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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: bd30c9d3-f097-461f-bbbf-ab1d7d7e8ecf
📒 Files selected for processing (14)
apps/webapp/app/components/assets/quantity-custody-list.tsxapps/webapp/app/modules/asset/service.server.test.tsapps/webapp/app/modules/asset/service.server.tsapps/webapp/app/modules/scan/last-scan-for-viewer.test.tsapps/webapp/app/modules/scan/service.server.tsapps/webapp/app/routes/_layout+/assets.$assetId.activity.test.server.tsapps/webapp/app/routes/_layout+/assets.$assetId.activity.tsxapps/webapp/app/routes/_layout+/assets.$assetId.overview.tsxapps/webapp/app/routes/_layout+/assets.$assetId.tsxapps/webapp/app/routes/_layout+/audits.$auditId.activity.test.server.tsapps/webapp/app/routes/_layout+/audits.$auditId.activity.tsxapps/webapp/app/routes/_layout+/audits.$auditId.tsxapps/webapp/app/routes/api+/assets.release-quantity-custody.tsapps/webapp/app/routes/api+/mobile+/custody.release-quantity.ts
Addresses the CodeRabbit review on #2800. The activity-loader authorization tests asserted `rejects.toBeDefined()`, which any exception satisfies. Fixing that surfaced a fidelity bug in the stub: it rejected with a bare `Error`, but `validatePermission` throws a `ShelfError`, and only a `ShelfError` keeps its status through the loader's `makeShelfError` — a bare Error becomes a generic 500. So the tests were passing on the wrong status. The stub now throws the real shape and both tests assert the thrown result carries status 403. Also adds a regression test pinning that a CONSUME does not write to `AssetLocation`. That drift is deliberate and pre-existing: the booking service makes no `assetLocation` write either, and custody carries no location, so there is nothing to identify which placement the used-up units came off. The test documents the deferral in executable form, so the eventual reconciliation work across every path that lowers `Asset.quantity` has a failing assertion to rewrite rather than an invisible assumption to rediscover.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/webapp/app/routes/api`+/mobile+/custody.release-quantity.ts:
- Around line 270-273: Update the mobile route’s header documentation to include
disposition in the successful response envelope and describe both outcomes:
RELEASE returns units to the available pool, while CONSUME consumes them. Keep
the documented behavior aligned with the response returned by the route handler.
🪄 Autofix
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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 2fce8d26-0958-4d40-ad1b-d7b505df7adb
📒 Files selected for processing (3)
apps/companion/app/(tabs)/assets/[id].tsxapps/companion/lib/api/types.tsapps/webapp/app/routes/api+/mobile+/custody.release-quantity.ts
Addresses the CodeRabbit review on #2800. Both route headers still described every release as returning units to the available pool, and the mobile one documented the success envelope as { success: true, asset }. Neither is true for a ONE_WAY consumable since the disposition branch landed. Both headers now state that the outcome is resolved server-side from consumptionType, describe RETURN and CONSUME, and explain why the low-stock notifier runs for both. The mobile envelope documents disposition. CodeRabbit flagged only the mobile route; the web sibling had the same stale wording.
Relationship to #2806, and a live reproduction of the defectTwo things for anyone landing here later: proof the defect is real outside unit tests, and where this PR sits relative to the follow-up. Reproduced end to end on stagingThe PR previously carried unit tests only, so the original defect had never been exercised in a browser. Setup: a
The consumed units returned to the available pool and The confirmation dialog reads "Enter the number of pcs to release back to the available pool" on an asset whose Behaviour field says "Used up (one-way)" — the copy promises the opposite of the configured behaviour. Both the behaviour and every affected string are addressed here. Where this PR ends and #2806 beginsThis PR makes consumption actually happen: Taking the same asset and consuming 10:
So this PR does not create the location-axis gap, but it does make it visible in a panel where both numbers render together. That is a deliberate trade: stock that never decrements is a worse defect than a stale location count. #2806 closes the second column, scoped across every path that lowers |
|
Split the authorization half out into #2807 so it isn't blocked by the consumable work here. #2807 contains only the loader gating: asset + audit activity notes, the That leaves this PR as the ONE_WAY consumable work alone. @DonKoko since you're reworking that half, do whatever suits you with this branch — reduce it to the consumable change, or close it in favour of your own. The authz work no longer depends on it either way. Verified on the split branch before opening: typecheck 4/4, lint 0 errors, 14/14 tests across the three suites. |
Adds releaseCategory(consumptionType) as the single encoding of the ONE_WAY rule and rebuilds defaultDisposition on top of it, so the package ships one predicate instead of two. The webapp service, the webapp custody list and the companion screen can all reach it; a .server module and a client component cannot reach each other, and React Native can reach neither. Also corrects the module note claiming Metro cannot consume raw TypeScript -- @shelf/datetime ships the same raw-src exports map and the companion imports it in five files.
releaseQuantity now takes an optional consumed count and writes one consumption log per non-zero leg, decrementing Asset.quantity by the consumed amount only. The default still comes from the locked asset row via the shared releaseCategory predicate, so a stale client cannot return a consumable's units to the pool, and a returnable asset rejects a non-zero consumed. Without the split, ending a consumable custody could only destroy every unit: 40 gloves handed back with 10 actually used would drop stock by 40. Booking check-in already offers this split for the same case. Drops the local resolveReleaseDisposition in favour of the package predicate.
Adds the optional consumed field and words the audit note and toast from the split the service reports back. Corrects the low-stock rationale: available is quantity minus custody, so it rises by exactly the returned units and is flat when everything was consumed.
A consumable row now asks how many of the released units were used up, so handing back unused stock no longer destroys it. The returnable path is unchanged. The consumable flag comes from the shared releaseCategory predicate rather than a second hardcoded comparison.
Mirrors the web route: optional consumed field, audit note worded from the persisted split, corrected low-stock rationale. Drops the disposition field from the response envelope -- no client reads it.
The asset screen imports releaseCategory from @shelf/quantity-control instead of hardcoding the ONE_WAY comparison, and the quantity sheet gained an optional second field so an operator can record how many released units were used up. Without it, mobile could only destroy every released unit. Drops the unread disposition field from the response type.
Drops the NodeNext .js extensions from @shelf/quantity-control's relative specifiers. Metro resolves by appending its sourceExts to the literal path, so ./types.js was probed as types.js / types.js.ts and never found types.ts -- the companion's new releaseCategory import would have failed to bundle. Verified by driving the installed metro-resolver directly: extensionless resolves, .js does not. A note on the entrypoint records why. Depends the quantity sheet's re-seed effect on primitives instead of the inline-built secondary prop object, so a parent re-render while the sheet is open no longer wipes a split the operator has already typed. Updates the mobile release-route test mock to the service's real return shape and pins all three audit-note wordings, which the stale mock left unasserted. Corrects the package description, which still called the companion a future consumer.
…uite The route's "bulk" bucket allows 10 requests a minute per user and the counter is an in-process Map that survives across tests in a file. Every case here posts as the same user, so the four note-wording tests added alongside took the file to twelve and the last cases started returning 429 instead of 200. Mocks enforceUserRateLimit to a no-op, matching mobile.bookings.partial-checkin. No coverage is lost: nothing in this file asserted the limiter.
The activity-note and last-scan authorization work now lives in #2807, so it is removed here rather than carrying the same nine files across two open PRs. This also clears the two ESLint errors blocking CI. The rule they trip, local-rules/no-test-files-in-routes, landed on main after this branch was cut, so the two route test suites went red on a rebase rather than on any change here. assets.$assetId.overview.tsx was the only file carrying both halves. It returns to its pre-authz state except for the one consumable line, which is kept: consumptionType passed through to QuantityCustodyList.
|
@coderabbitai full review again |
|
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
apps/webapp/test/routes-tests/api+/mobile.custody.release-quantity.test.ts (1)
205-210: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReplace the new
anycasts with typed mocks.These casts bypass the
releaseQuantityandcreateNotecontracts. They can hide an invalid fixture after the release result changed to includeconsumedandreturned. Use typed Vitest mocks and fixtures that satisfy the actual return types.Validate with
pnpm db:generate,pnpm turbo typecheck, andpnpm webapp:test -- --run.Also applies to: 269-272, 291-296, 316-321
🤖 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 `@apps/webapp/test/routes-tests/api`+/mobile.custody.release-quantity.test.ts around lines 205 - 210, Replace the any casts on releaseQuantity and createNote mocks with typed Vitest mocks, and update every affected fixture—including the release result—to satisfy the actual contracts with consumed and returned fields. Preserve the existing test behavior while ensuring mock implementations and resolved values are type-checked. Validate with pnpm db:generate, pnpm turbo typecheck, and pnpm webapp:test -- --run.Source: Coding guidelines
apps/webapp/app/modules/asset/service.server.test.ts (1)
1214-1217: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winStrengthen this assertion so it can fail.
The comment states "No quantity write at all", but the assertion only rejects a
decrement: 0write. The service gates the whole decrement block onconsumedUnits > 0, so it never issuesdecrement: 0. This assertion therefore passes even if the service wrongly decremented by 10. TheASSET_QUANTITY_CHANGEDassertion below does catch that regression, so coverage is not lost, but this line does not test its stated intent.Assert that no
quantitywrite occurred for any amount.♻️ Proposed change
- // No quantity write at all — the returnable path stays byte-identical. - expect(mockAssetUpdate).not.toHaveBeenCalledWith( - expect.objectContaining({ data: { quantity: { decrement: 0 } } }) - ); + // No quantity write at all — the returnable path stays byte-identical. + // Match any `quantity` payload, not just `decrement: 0`; the service + // gates the decrement block on `consumedUnits > 0`, so asserting the + // zero case alone could never fail. + expect(mockAssetUpdate).not.toHaveBeenCalledWith( + expect.objectContaining({ + data: expect.objectContaining({ quantity: expect.anything() }), + }) + );🤖 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 `@apps/webapp/app/modules/asset/service.server.test.ts` around lines 1214 - 1217, Strengthen the assertion in the returnable-path test so mockAssetUpdate is verified not to receive any update containing a quantity field, regardless of the decrement amount. Replace the current decrement: 0-specific matcher while preserving the existing ASSET_QUANTITY_CHANGED assertion.packages/quantity-control/src/dispositions.test.ts (1)
18-18: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the
.jsimports from the package test files.
packages/quantity-control/src/index.tsdocuments extensionless relative imports for this package. The../../types.jsand.jstest imports still violate that convention. Change all of them to extensionless relative specifiers.🤖 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 `@packages/quantity-control/src/dispositions.test.ts` at line 18, Update all relative imports in the quantity-control package test files, including dispositions.test.ts, to remove the .js extension and use extensionless specifiers consistent with src/index.ts; preserve the existing import paths and symbols otherwise.
🤖 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 `@apps/companion/components/quantity-input-sheet.tsx`:
- Around line 125-139: Update both primary-value change handlers in the quantity
input sheet so that when the primary quantity decreases, secondaryValue is
clamped to the new primary value. Preserve existing behavior when the secondary
field is absent or already within range, and ensure this applies to both primary
input change paths.
In `@apps/webapp/app/components/assets/quantity-custody-list.tsx`:
- Around line 498-570: Update the release form around the consumable quantity
inputs and the existing fetcher.data success handling to render any returned
server-side validation or service error in a nearby alert element with
role="alert". Preserve the current success-close behavior, and ensure stale
quantities or invalid consumed splits visibly explain the API rejection instead
of leaving the dialog without feedback.
In `@apps/webapp/test/routes-tests/api`+/mobile.custody.release-quantity.test.ts:
- Around line 58-63: Update the explanatory comment above the releaseQuantity
mock in the test setup to begin with the required “// why:” marker, while
preserving its existing explanation of the mocked persisted counts.
---
Nitpick comments:
In `@apps/webapp/app/modules/asset/service.server.test.ts`:
- Around line 1214-1217: Strengthen the assertion in the returnable-path test so
mockAssetUpdate is verified not to receive any update containing a quantity
field, regardless of the decrement amount. Replace the current decrement:
0-specific matcher while preserving the existing ASSET_QUANTITY_CHANGED
assertion.
In `@apps/webapp/test/routes-tests/api`+/mobile.custody.release-quantity.test.ts:
- Around line 205-210: Replace the any casts on releaseQuantity and createNote
mocks with typed Vitest mocks, and update every affected fixture—including the
release result—to satisfy the actual contracts with consumed and returned
fields. Preserve the existing test behavior while ensuring mock implementations
and resolved values are type-checked. Validate with pnpm db:generate, pnpm turbo
typecheck, and pnpm webapp:test -- --run.
In `@packages/quantity-control/src/dispositions.test.ts`:
- Line 18: Update all relative imports in the quantity-control package test
files, including dispositions.test.ts, to remove the .js extension and use
extensionless specifiers consistent with src/index.ts; preserve the existing
import paths and symbols otherwise.
🪄 Autofix
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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: f16dcf14-24d9-40e9-8479-3b8eda156453
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (18)
apps/companion/app/(tabs)/assets/[id].tsxapps/companion/components/quantity-input-sheet.tsxapps/companion/hooks/use-custody-actions.tsapps/companion/lib/api/custody.tsapps/companion/package.jsonapps/webapp/app/components/assets/quantity-custody-list.tsxapps/webapp/app/modules/asset/service.server.test.tsapps/webapp/app/modules/asset/service.server.tsapps/webapp/app/routes/_layout+/assets.$assetId.overview.tsxapps/webapp/app/routes/api+/assets.release-quantity-custody.tsapps/webapp/app/routes/api+/mobile+/custody.release-quantity.tsapps/webapp/test/routes-tests/api+/mobile.custody.release-quantity.test.tspackages/quantity-control/package.jsonpackages/quantity-control/src/availability.tspackages/quantity-control/src/dispositions.test.tspackages/quantity-control/src/dispositions.tspackages/quantity-control/src/guards.tspackages/quantity-control/src/index.ts
The release dialog read fetcher.data only to detect success, so a rejected submit left it open with no explanation and a re-enabled button on a non-idempotent action. releaseQuantity rejects a stale quantity, a consumed above the released amount, and any consumed on a returnable asset, all with messages written for an operator to read. They now render in a role="alert" block, matching move-units-dialog and quick-adjust-dialog. On mobile, lowering the release quantity left the used-up count above it, which disabled Confirm until the operator edited the second field by hand. Both primary-value paths now clamp it downward, matching the webapp reducer.
…mock The fetcher.Form stub intercepted submit, but no test in the file fires one and the component passes no onSubmit to fetcher.Form, so it was dead code. Removing it also clears the react-doctor no-prevent-default warning this file introduced.
The "no quantity write" check only rejected `decrement: 0`, which the service never issues -- it gates the whole decrement block on consumedUnits > 0. The assertion therefore passed even if the service had wrongly decremented by 10. It now matches any quantity payload; the status-flip write carries no quantity key, so it cannot collide. Also drops the .js extensions from the quantity-control test imports. The package entrypoint has documented extensionless relative specifiers as mandatory since the Metro resolution fix, and the tests were the last holdout.
|
Answering the three 🧹 nitpicks from the full review — they have no inline thread, so they're grouped here. The three actionable inline comments were answered and resolved on their own threads. 1. Fixed in
expect(mockAssetUpdate).not.toHaveBeenCalledWith(
expect.objectContaining({
data: expect.objectContaining({ quantity: expect.anything() }),
})
);The status-flip write in step 6b carries no 2. Fixed in Worth recording why this mattered more than tidiness. The extensionless convention isn't stylistic: Metro's resolver ( 3. Not applying this. CLAUDE.md's "never use |
Asset and audit activity loaders gated on the entity permission rather than the note permission, then fetched and returned every note in the page payload. The note check ran client-side only, so roles without it received the data and only the UI hid it. The same shape existed on the asset overview loader, which returned last-scan data containing personal information with the only scan:read check running in the component. Splits the authorization half out of Shelf-nu#2800 so it is not blocked by the consumable work under review there. No behavioural overlap between the two.
Triggered by: Shelf-nu/shelf.nu#2800 Consumption type used to be honored only at booking check-in, so releasing custody on a ONE_WAY consumable handed every unit back to the pool and the total never moved. shelf.nu#2800 derives the disposition from the asset row, so a consumable's units are consumed instead, with an optional split for whatever came back unused. The KB stated the old behaviour outright ('Releasing custody, fully or partially, returns those units to the available pool'). Corrected here, along with the mobile section: the outcome is decided server-side, so the live Companion build consumes too while still labelling the action 'release'. Also closes a carried-over follow-up: an 'Editing stock in bulk' section on the same article, held back while PR #215 owned the file. Co-authored-by: Carlos Virreira <carlos@shelf.nu>
2 participants
Two independent, source-verified defects. Fix 1 is a correctness bug on
quantity-tracked consumables; Fix 2 is a server-side authorization gap.
Source-verified against
main, and rebased onto86231bc57.Fix 1 — ONE_WAY consumables did not consume outside bookings
The defect
consumptionTypewas honoured on exactly one path: booking check-in. The directcustody path never read it.
releaseQuantity(modules/asset/service.server.ts) unconditionally wroteConsumptionCategory.RETURNand handed the units back to the available pool,whatever the asset's
consumptionType. So for aQUANTITY_TRACKEDasset withconsumptionType: ONE_WAY(gloves, batteries, cable ties), an operator who usedup 10 units and released them saw all 10 return to stock.
Asset.quantitynevermoved, consumption reporting counted the units as back on the shelf, and the
workspace's stock figure drifted further from reality with every use.
The UI matched:
quantity-custody-list.tsxoffered only a "Release" button, so aconsumable had no way at all to be consumed outside a booking.
The shape, and why
The branch lives inside
releaseQuantity, deriving the disposition fromAsset.consumptionTypeon the row it has already locked. A siblingconsumeQuantityfunction was the alternative; three things decided against it.caller applied the wrong outcome to a consumable. A second entry point makes
that a permanent possibility: the old endpoint would still happily return a
consumable's units to the pool, so
releaseQuantitywould need to readconsumptionTypeto reject that case anyway. Once it reads the field, it mayas well act on it. Deriving server-side from the locked row means no client,
stale or otherwise, can get it wrong.
routes/api+/assets.release-quantity-custody.ts(web) androutes/api+/mobile+/custody.release-quantity.ts(companion) call thisfunction. Fixing inside it fixes the companion the moment the webapp deploys.
A new endpoint would leave the mobile path consuming nothing until an App
Store / Play release.
ConsumptionLog.categorydistinguishes
RETURNfromCONSUME— the same discriminator bookingcheck-in writes, so consumption reporting sees both paths identically. A
second function would not make the log clearer; it would duplicate ~120 lines
of lock / org-validation / custody-lookup machinery to change three of them.
Blast radius is one function body, plus wording in the two routes and the one
component.
What changed
resolveReleaseDisposition(consumptionType)— single source of truth for thebranch, exported and shared by the service and the routes so the human-readable
note can never drift from the persisted log.
ONE_WAY → CONSUME;TWO_WAYandlegacy
null→RETURN.releaseQuantitynow, forCONSUME:ConsumptionCategory.CONSUMEinstead ofRETURN;Asset.quantityby the released amount;ASSET_QUANTITY_CHANGEDwith the truefrom/to, alongside theexisting
CUSTODY_RELEASED— one event per field that changed, both insidethe same transaction as the writes.
{ asset, disposition }(was: the asset alone; neither caller usedthe old return value).
TWO_WAYand legacy-nullbehaviour is byte-for-byte unchanged. There is atest pinning that.
decrements the pool without touching custody, so it must check the
Asset.quantity >= SUM(Custody.quantity)invariant explicitly. Here custodydrops by the same
nin the same transaction, soQ >= CimpliesQ - n >= C - n, andn <= custody.quantity <= C <= Qkeeps the resultnon-negative. The reasoning is in a comment at the site rather than a redundant
aggregate query.
ONE_WAYasset,with dialog copy that states the stock reduction is permanent. Presentation
only — it posts to the same endpoint, and the server decides.
disposition,and both now run the low-stock notifier for the right reason: a
RETURNraisesavailable stock (clears a stale debounce marker), a
CONSUMElowersAsset.quantityand can trip the threshold.Tests
modules/asset/service.server.test.ts— newreleaseQuantity — consumptionType dispositionsuite. No existing test touchedconsumptionTypeon this path atall, which is how it shipped.
Proven to fail without the fix: with the branch neutered to the old constant
RETURN, exactly the three consumable tests fail and theTWO_WAY/ legacyguards keep passing.
Fix 2 — activity notes were sent to users who cannot read them
The exposure
routes/_layout+/assets.$assetId.activity.tsxgated its loader onasset:read,fetched every note, and returned them in the page payload. The
note:readcheckran client-side only, in the component, swapping the notes for an empty state.
BASE and SELF_SERVICE both hold
asset: [read]andnote: []. Both thereforereceived every asset note in the response body, hidden only by React and visible
to anyone who opens the network tab. Org scoping held, so this is same-workspace
only — but it is a server-side authorization gap, not a display bug.
routes/_layout+/audits.$auditId.activity.tsxhad the identical shape withauditNote:read. That one is not a live exposure: every role currently holdsauditNote:read, so nothing leaks today. It is the same defect one permissionedit away from mattering, and it is fixed the same way.
The intended rule was already established elsewhere: all four activity CSV exports
require the note permission, and the bookings and locations activity pages gate
their loaders on
bookingNote:read/locationNote:read. Assets and audits werethe outliers.
What changed
note:read. The parent route(
assets.$assetId.tsx) already enforcesasset:readfor the page, so the childrequires only the permission covering the data it returns. Notes are never
fetched without it.
auditNote:read. The orthogonalrequireAuditAssigneeForBaseSelfServicerestriction is untouched, and there isa test pinning that it still runs.
the asset and audit detail pages, rather than routing them into a 403. Mirrors
what the bookings detail page already does.
locations.
The sweep (scope item d) — one more live instance, fixed
Grepping every loader that returns data the component hides behind
userHasPermissionturned up two further cases of this class:Fixed here —
assets.$assetId.overview.tsxlastScan. The loader returnedscan data that includes personal information, with the only
scan:readcheckrunning client-side in the component. Because that is personal data rather than
workspace content, it is fixed in this PR rather than filed.
The gate is extracted into
getLastScanForViewer(modules/scan/service.server.ts)so the data cannot be fetched without the check — a caller can no longer forget.
It returns
nullfor an unauthorized viewer, the same shape as "never scanned",so callers need no special case. Extracting it also makes it directly
unit-testable, which an inline check in that (very large) loader would not be.
One further instance was identified during the sweep. Its correct server-side
gate depends on an org-level visibility setting rather than a flat permission,
so fixing it changes visible behaviour and is being handled separately as a
product decision.
Tests
routes/_layout+/assets.$assetId.activity.test.server.ts— drives the realRole2PermissionMapthrough the loader. Asserts the gate asks fornote:read(not
asset:read), that BASE and SELF_SERVICE are rejected andgetPaginatedAndFilterableAssetNotesis never called, and that a permittedrole still gets its notes. Asserting on the real matrix rather than restating it
means the test speaks up if the matrix changes.
routes/_layout+/audits.$auditId.activity.test.server.ts— same, plus a testpinning that the assignee restriction survives the entity switch, plus one that
documents (and will fail on a change to) every role currently holding
auditNote:read.modules/scan/last-scan-for-viewer.test.ts— runs the realhasPermissionandparseScanDataagainst the real matrix with only the DB read mocked. A viewerwithout
scan:readgetsnulland the scan row is never queried; a permittedviewer still gets the parsed scan.
Verification
No runnable environment was available while building this, so verification is unit
tests plus source reasoning. The tests above are written to be the proof rather
than a formality: each one drives the real permission matrix or the real service,
and the Fix 1 suite was checked to fail with the new branch neutered.
pnpm turbo typecheck— 4/4 successful.pnpm webapp:lint— 0 errors (5 warnings, all pre-existing, none in touched files).pnpm webapp:test -- --run --poolOptions.threads.maxThreads=3— 320/320 files,4118 tests pass.
Each leg of
pnpm webapp:validateis green on its own, but the combined commanddid not complete green on the machine this was built on.
validateisrun-p test:run lint typecheck, and that box was carrying a load average of80–162 on 8 cores from unrelated processes. Under that starvation vitest suites
die during collect with
Hook timed out in 10000ms, and across three runs ofidentical code the failing set was 3, then 2, then 3 suites with different
membership each time — including React component suites with no connection to
this diff. Capping the runner to 3 threads makes the whole suite pass
deterministically. Worth a clean
validaterun in CI (or on an idle machine)before merge.
Not in scope
Location placements are not reconciled when stock drops. The
CONSUMEbranchlowers
Asset.quantityand leavesAssetLocationalone, so the location axis candrift above the total: consume 10 of 100 placed units and the location page reads
100 while the asset reads 90. Nothing aborts at the time —
asset_location_sum_within_totalisAFTER INSERT OR UPDATE OR DELETE ON "AssetLocation"and does not fire on anAssetwrite — but a later placementedit then trips the constraint on a write that is itself legitimate.
This is pre-existing, not introduced here. Booking check-in's
CONSUMEhasbehaved this way since it shipped (the booking service contains no
assetLocationwrite at all), and the manual stock-lowering path drifts the same way:
assertAssetQuantityNotBelowReservationsqueries custody, assetKit, bookingAssetand consumptionLog, not assetLocation. This PR adds a second call site to existing
behaviour rather than a new class of drift, and matching the booking path is the
right call for consistency — reconciling the location axis belongs in one piece of
work covering every path that lowers
Asset.quantity, not in a one-off here. Thereasoning is recorded in a comment at the decrement site.
Worth prioritising: the drift is most visible for workspaces that model stock as
one asset per site with a placement per location.
Asset Inventory report collapses multi-location quantity-tracked stock to the
asset-wide total. A passing repro exists, but it is separate, larger work pending
a product decision.
Summary by CodeRabbit