feat: Quantitative Assets#2337
Open
DonKoko wants to merge 67 commits into
Open
Conversation
Add proposal for quantity-based asset management covering consumables (quantity-tracked fungible items) and asset models (template-grouped individual assets). Includes user stories, competitor analysis, feature overview, data model changes, and migration strategy.
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
Contributor
PR Feedback — Quantitative Assets (Ultra Clarity)
1) Mental model: Tracking method first (identity vs interchangeable)Core principle (should be obvious in UI/copy): This maps directly to the RFC:
Copy suggestion (UI, not schema): even if we keep the enum name 2) Behavior: what happens when units leave inventory?The RFC’s Key clarity note: “Consumable” (word) implies ONE_WAY, but this RFC also supports TWO_WAY pooled tracking (“8 returned, 2 lost”). That’s fine—just make behavior explicit. 3) Two user intents we must support (both already implied by the RFC)The RFC covers both circulation (bookings/custody) and stock decrement (manual restock/adjust + audit log). Let’s explicitly acknowledge both so UX doesn’t accidentally become booking-only. A) Circulation intent (Cheqroom-like)
B) Stock intent (Sortly-like)This is the gift-bags / fittings / box-on-shelf workflow. Suggestion: Add/confirm a primary “Quick adjust” path on consumable QR scan (± quantity + note) that writes to 4) Guardrail: Identity boundary must stay hardThis is the key rule that prevents future confusion: Why this matters: avoids the classic “Laptop quantity 100 — where are my 100 labels?” confusion. If we ever add “print multiple labels for a quantity asset” (Cheqroom bulk dots style), we must label it clearly: 5) Asset creation UX copy (suggestion)Current RFC: type selection “Individual vs Consumable” is good. I’d ensure the UI text makes implications unmistakable:
Then, only for “Tracked by quantity”, ask behavior:
6) Optional: add an explicit Open Question about scan behaviorThe RFC has strong open questions already. One more that directly impacts clarity:
My vote: B for stock-intent users; with a visible path to booking/custody flows when relevant. 6) Booking Integration Guardrail (Core to Shelf)Since bookings are central to Shelf, quantity-tracked assets must feel fully native inside reservation and checkout flows — not like an extension. Key principles: This calculation should be predictable and visible wherever reservations occur. For quantity assets in bookings:
with both reflected in quantity and logged via ConsumptionLog. Book-by-model + scan-to-assign is a strong differentiator. It should feel seamless: This is where Shelf becomes stronger than stock-only systems. 7) Label Expectations (Very Important Edge Case)We must explicitly handle this common scenario:
There are two fundamentally different reasons a user might ask for this: If we support multiple identical labels for quantity assets, we must enforce this guardrail in UI copy:
And provide a visible escape hatch:
This preserves architectural integrity while remaining pragmatic. Summary chart (one-screen clarity)Net: This RFC already contains the right primitives ( |
Replace "Consumable" terminology with "Quantity-tracked" throughout the quantitative assets proposal. The old term conflated returnable mid-value items (cables, adapters) with actual consumables (gloves, batteries). "Quantity-tracked" is neutral and already matches the tracking method label in the UI. Changes: - Rename AssetType enum value: CONSUMABLE → QUANTITY_TRACKED - Update all prose references in Problem Statement, User Stories, Feature Overview, Transition, and Decisions sections - Update Technical Design: schema comments, design principles, custody/booking notes, ER diagram, implementation phases - Preserve ConsumptionLog/ConsumptionType naming (these describe audit events and behavior modes, not the asset type) - Leave competitor analysis descriptions unchanged (they refer to other products' actual terminology)
Contributor
Author
|
@carlosvirreira ty for the feedback. Everything is agreed and documented inside the PR description. Feel free to review it again. |
|
@DonKoko, this is a very well thought-out approach to quantitative assets! I only have a few comments, and questions: In response to the Part 1 - Open Questions sections:
Reserved status question Thanks again for pushing this forward! |
Contributor
Author
|
hey @DroneProf, thanks for your feedback. All 3 points you mentioned will be considered when defining the final version. Concerning your question, I am not certain, but you are raising a very valid question, as "reserved" will have a different value at different points in time. I will think about it and make sure this is included in the final plan. |
Contributor
|
@DonKoko do you think have enough input? Excited about next steps! |
Contributor
|
@DonKoko do you think have enough input? Do you want more opinions? |
Contributor
Author
|
@carlosvirreira nope |
Schema changes: - Add AssetType, ConsumptionType, ConsumptionCategory enums - Add quantity tracking fields to Asset model (type, quantity, minQuantity, consumptionType, unitOfMeasure) - Create AssetModel entity for template/grouping - Create ConsumptionLog model for audit trail - Create BookingAsset explicit pivot table (empty, Phase 3) - Migration with RLS on all new tables AssetModel CRUD: - Service layer with full CRUD + bulk delete - Listing, create, edit routes (Category pattern) - Form, quick actions, delete dialog components - Sidebar nav entry + permission entity for all roles Asset form + detail updates: - Tracking method selector (Individual / Quantity) - Conditional quantity fields section - Server-side validation for quantity-tracked assets - Detail page shows quantity info + asset model link - QTY badge in asset list for quantity-tracked assets Tests (35 new): - AssetModel service (17), form schema (6) - Asset form schema (9), quantity validation (3) - AssetModel test factory PRD updates: - Decision #10: BookingAsset rename migration strategy - Phase scoping clarified for BookingAsset
|
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:
WalkthroughAdds Asset Models and quantity-tracked assets end-to-end: RFC, DB enums/models/migrations, server services (consumption logs, quantity custody), API routes, UI components/forms/dialogs/index pages, query/export/filter support, permissions, tests, and migration helpers. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Browser
participant Route as Remix API Route
participant Service as Asset / Consumption Service
participant DB as Postgres / Prisma
participant Notif as Notification System
Client->>Route: POST /api/assets/assign-quantity-custody (assetId, teamMemberId, qty)
Route->>Service: checkOutQuantity(assetId, teamMemberId, qty, userId, orgId, note)
Service->>DB: BEGIN TRANSACTION
Service->>DB: SELECT * FROM "Asset" WHERE id = $1 FOR UPDATE
DB-->>Service: locked asset row
Service->>DB: upsert custody row (increment or create)
DB-->>Service: custody updated
Service->>DB: INSERT ConsumptionLog(...)
DB-->>Service: consumption log created
Service->>DB: COMMIT
Service->>Route: success
Route->>DB: createNote(...)
Route->>Notif: sendNotification(success)
Route-->>Client: 200 OK
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
✨ Finishing Touches🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/proposals/quantitative-assets.md`:
- Around line 331-334: The document currently contains conflicting statements
about the QR scan default: the paragraph that declares "Quick adjust" as the
primary/default action must be the single source of truth; update the later
passage that treats the default scan action as "open" (the text around the
phrase "default scan action" and lines referring to opening the full
booking/custody flows) to instead state that scans default to the "Quick adjust"
flow and that full booking/custody paths are secondary/visible from that view,
or remove the contradictory "open" wording entirely so only "Quick adjust"
remains as the default behavior.
- Around line 253-255: The fenced formula block containing "Available = Total
quantity − In custody − Reserved in bookings" is missing a language tag (MD040);
update the opening fence from ``` to ```text so the block becomes a labeled
code/text block (i.e., change the fence for the formula to use the "text"
language identifier).
- Around line 566-580: The fenced ER overview block in
docs/proposals/quantitative-assets.md is missing a language tag; update the
opening fence for the ER diagram (the multi-line block starting with
"Organization" and the ASCII tree showing AssetModel, Category, Location, Kit,
Booking) to include a language identifier (e.g., ```text) so the Markdown linter
rule MD040 is satisfied; just change the opening triple backticks to include the
tag and leave the block contents unchanged.
- Around line 483-497: The Custody model in the proposal is missing audit,
relation cascade, and index details: add an updatedAt DateTime `@updatedAt` field
to the Custody model, add explicit relation cascade actions (onDelete: Cascade,
onUpdate: Cascade) on the asset relation (the `@relation` on Asset), and restore
the performance indexes by adding @@index([assetId, teamMemberId]) and
@@index([teamMemberId]) while keeping the intentional removal of assetId `@unique`
and retaining @@unique([assetId, teamMemberId]) for one record per
asset-custodian pair.
- Around line 508-521: The BookingAsset model currently uses a bare
assignedAssetId String? with no foreign key or mutual-exclusivity enforcement;
change the schema to replace assignedAssetId with two nullable relation fields
(e.g., assetId -> relation to Asset for a specific physical assignment and
assetModelId -> relation to AssetModel for model-level reservations), add a
discriminator enum (e.g., BookingAssetType { ASSET, ASSET_MODEL }) on
BookingAsset to indicate which variant is in use, add a DB-level CHECK
constraint enforcing mutual exclusivity ((assetId IS NOT NULL AND assetModelId
IS NULL) OR (assetId IS NULL AND assetModelId IS NOT NULL)), and implement
application-level validation in create/update paths to validate the enum matches
which relation is set and to surface clear errors before hitting the DB; locate
these changes around the BookingAsset model, assignedAssetId, assetId and new
assetModelId symbols.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 855c7e1e-b0ab-46c4-b789-1b8dcee56313
📒 Files selected for processing (1)
docs/proposals/quantitative-assets.md
There was a problem hiding this comment.
Actionable comments posted: 19
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/webapp/app/components/assets/form.tsx (1)
56-87:⚠️ Potential issue | 🟠 MajorServer validation is stricter than the client schema, and error feedback is incomplete for quantity fields.
The schema allows
QUANTITY_TRACKEDassets withoutconsumptionType, but the server rejects this (per service.server.ts). Additionally,minQuantityandconsumptionTypefields don't display server-side validation errors when submission fails—onlyquantitydoes. Empty numeric inputs are coerced rather than treated as unset, which can result in unintended zero values.The
quantityfield correctly displayszo.errors.quantity()?.message, butconsumptionType(Select) andminQuantity(Input) have no error display at all. Other fields liketitlefollow the patternactionData?.errors?.fieldName?.message || zo.errors.fieldName()?.message. Apply this pattern to these fields, and tighten the schema with conditional validation to enforcequantity,minQuantity, andconsumptionTypepresence whentype === QUANTITY_TRACKED.🛡️ Suggested schema and field fixes
export const NewAssetFormSchema = z.object({ @@ - quantity: z.coerce.number().int().positive().optional(), - minQuantity: z.coerce.number().int().nonnegative().optional(), - consumptionType: z.nativeEnum(ConsumptionType).optional(), + quantity: z.preprocess( + (value) => (value === "" ? undefined : value), + z.coerce.number().int().positive().optional() + ), + minQuantity: z.preprocess( + (value) => (value === "" ? undefined : value), + z.coerce.number().int().nonnegative().optional() + ), + consumptionType: z.preprocess( + (value) => (value === "" ? undefined : value), + z.nativeEnum(ConsumptionType).optional() + ), unitOfMeasure: z.string().optional(), -}); +}).superRefine((data, ctx) => { + if (data.type !== AssetType.QUANTITY_TRACKED) return; + + if (data.quantity == null) { + ctx.addIssue({ + code: z.ZodIssueCode.custom, + path: ["quantity"], + message: "Quantity is required for quantity-tracked assets", + }); + } + + if (!data.consumptionType) { + ctx.addIssue({ + code: z.ZodIssueCode.custom, + path: ["consumptionType"], + message: "Behavior mode is required for quantity-tracked assets", + }); + } +});Then add error display to the form fields (lines ~720 and ~705):
<Select name="consumptionType" defaultValue={consumptionType ?? ""} disabled={disabled} + error={ + actionData?.errors?.consumptionType?.message || + zo.errors.consumptionType()?.message + } > <Input type="number" label="Min quantity" name="minQuantity" disabled={disabled} min={0} step={1} className="w-full" defaultValue={minQuantity ?? ""} + error={ + actionData?.errors?.minQuantity?.message || + zo.errors.minQuantity()?.message + } />🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/webapp/app/components/assets/form.tsx` around lines 56 - 87, The client schema NewAssetFormSchema and the form UI need two fixes: (1) tighten schema: preprocess numeric fields (quantity, minQuantity, valuation) to treat "" as undefined (use z.preprocess to convert empty string -> undefined) and add conditional validation (via .superRefine or a .refine using the top-level object) so when type === AssetType.QUANTITY_TRACKED you require quantity (positive int), minQuantity (non-negative int) and consumptionType (nativeEnum(ConsumptionType)); (2) surface server-side errors in the form: update the error rendering for the quantity, minQuantity and consumptionType inputs to use actionData?.errors?.quantity?.message || zo.errors.quantity()?.message (and analogous for minQuantity and consumptionType) so server validation messages are shown alongside client zod errors. Ensure you reference the NewAssetFormSchema, AssetType, ConsumptionType, and the form error sources actionData and zo.errors when making changes.
♻️ Duplicate comments (1)
docs/proposals/quantitative-assets.md (1)
331-333:⚠️ Potential issue | 🟠 MajorKeep one source of truth for the quantity QR default.
Line 331 says quick adjust is the primary scan action, but Open Question
#1still treats the default as undecided. The RFC is still giving implementation two different answers.Also applies to: 381-382
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/proposals/quantitative-assets.md` around lines 331 - 333, The document currently contradicts itself about the default action for scanning a quantity-tracked asset QR: the "primary action" paragraph names "Quick adjust" as the default while "Open Question `#1`" leaves it undecided; update the RFC to keep a single source of truth by making "Quick adjust" the default everywhere—specifically, edit the "Open Question `#1`" section to state that Quick adjust is the default scan action for quantity assets (and remove/replace any language that treats the default as undecided), and make the same consistent change where the duplicate wording appears near the "Primary action" / "Core Concepts" discussion so both references (Quick adjust, Open Question `#1`) match.
🧹 Nitpick comments (4)
apps/webapp/test/factories/assetModel.ts (1)
11-21: Use collision-safe defaults in the factory.Hardcoded identifiers/timestamps increase accidental uniqueness conflicts in tests that create multiple asset models. Prefer generated defaults (while still allowing overrides).
As per coding guidelines: "Use factories to generate consistent and realistic test data with field override capabilities."Suggested refactor
+import { randomUUID } from "node:crypto"; import type { AssetModel } from "@prisma/client"; export function createAssetModel( overrides: Partial<AssetModel> = {} ): AssetModel { + const now = new Date(); return { - id: "asset-model-123", + id: randomUUID(), name: "Dell Latitude 5550", description: "Standard issue laptop", image: null, imageExpiration: null, defaultCategoryId: null, defaultValuation: null, - organizationId: "org-123", - userId: "user-123", - createdAt: new Date("2023-01-01"), - updatedAt: new Date("2023-01-01"), + organizationId: randomUUID(), + userId: randomUUID(), + createdAt: now, + updatedAt: now, ...overrides, }; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/webapp/test/factories/assetModel.ts` around lines 11 - 21, Replace hardcoded fields in the asset model test factory with collision-safe generated defaults: generate id (instead of "asset-model-123"), organizationId and userId, and use dynamic timestamps for createdAt/updatedAt so multiple tests don't collide; keep the factory override capability so callers can still pass explicit id/organizationId/userId/createdAt/updatedAt when needed (update the factory that returns the object with keys id, name, description, image, imageExpiration, defaultCategoryId, defaultValuation, organizationId, userId, createdAt, updatedAt to derive defaults from generators/Date.now()).apps/webapp/app/hooks/use-sidebar-nav-items.tsx (1)
134-140: Prefer permission-based visibility over role shortcut forAsset Models.
hidden: isBaseOrSelfServicecan diverge fromRole2PermissionMap(e.g., BASE hasassetModel:read). Consider deriving visibility from permission checks to keep nav behavior aligned with server authorization.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/webapp/app/hooks/use-sidebar-nav-items.tsx` around lines 134 - 140, The nav entry for "Asset Models" uses the role shortcut hidden: isBaseOrSelfService which can drift from actual permissions; replace that boolean with a permission-derived visibility check (e.g., consult Role2PermissionMap and the current role or use the existing permission hook) so the item is hidden only when the current user lacks the assetModel:read permission; update the "Asset Models" nav item where isBaseOrSelfService is referenced and use the permission check (reference symbols: the "Asset Models" nav object, isBaseOrSelfService, Role2PermissionMap, and whichever currentRole/permission hook function exists in the file) to decide visibility.apps/webapp/app/routes/_layout+/assets.$assetId.overview.tsx (1)
423-466: Render both tracking paradigms explicitly.Only quantity-tracked assets get identity copy here. Adding the matching
Individualstate in the opposite branch would keep the INDIVIDUAL/QUANTITY boundary visible on the overview page instead of only labeling one side.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/webapp/app/routes/_layout`+/assets.$assetId.overview.tsx around lines 423 - 466, The overview only renders a "Tracked by quantity" badge when asset?.type === AssetType.QUANTITY_TRACKED, so add a matching tracking-method list item for the opposite case (identity/individual) inside the same JSX block: update the conditional around asset?.type === AssetType.QUANTITY_TRACKED to render the existing "Tracking method" <li> with Badge "Tracked by quantity" in the true branch and an else branch that renders an equivalent <li> with Badge (e.g., "Individual" or "Tracked by identity") so the INDIVIDUAL/QUANTITY boundary is explicit; use the same surrounding structure/classes and keep the other fields (Quantity/Min quantity/Behavior mode) unchanged.packages/database/prisma/schema.prisma (1)
387-388: ConsideronDelete: SetNullforcreatedByto align with schema patterns.Deleting a User with
onDelete: Cascadewill delete allAssetModelrecords they created. This differs from the pattern used inNoteandAuditNotemodels, which useonDelete: SetNullwith optional creator fields. To preserve asset models when their creator is deleted, consider aligning with that pattern:Suggested change
- createdBy User `@relation`(fields: [userId], references: [id], onDelete: Cascade, onUpdate: Cascade) - userId String + createdBy User? `@relation`(fields: [userId], references: [id], onDelete: SetNull, onUpdate: Cascade) + userId String?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/database/prisma/schema.prisma` around lines 387 - 388, The createdBy relation on the AssetModel currently uses onDelete: Cascade which will remove AssetModel rows when a User is deleted; change it to onDelete: SetNull and make the userId field optional (e.g., userId String? and createdBy User? relation) to match the Note/AuditNote pattern so assets are preserved when their creator is removed; update the relation definition for createdBy and the userId column accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/webapp/app/components/asset-model/bulk-actions-dropdown.tsx`:
- Line 7: The dropdown is importing and mounting the category-specific
BulkDeleteDialog (BulkDeleteDialog from ../category/bulk-delete-dialog) which
posts categoryIds to /api/categories/bulk-actions; replace those mounts/imports
in BulkActionsDropdown with the asset-model-specific bulk delete component
(e.g., AssetModelBulkDeleteDialog) that sends assetModelIds to
/api/asset-models/bulk-actions, or create an AssetModelBulkDeleteDialog
component that mirrors the category flow but targets the correct
payload/endpoint, and update the import(s) and usage in BulkActionsDropdown
accordingly.
- Around line 73-80: The onOpenChange handler in DropdownMenu only calls setOpen
when defaultApplied is true, so normal open/close events don't sync to the open
state; update the onOpenChange callback to call setOpen(open) unconditionally
(or at least remove the defaultApplied guard) so built-in opens/closes are
reflected in the open state managed by useControlledDropdownMenu; keep the
window.innerWidth conditional behavior if you still need mobile-only behavior
but ensure setOpen(open) is executed from onOpenChange (referencing
onOpenChange, setOpen, open, defaultApplied, useControlledDropdownMenu,
DropdownMenu).
- Around line 10-15: The component
apps/webapp/app/components/asset-model/bulk-actions-dropdown.tsx currently
imports and uses the deprecated DropdownMenu symbols (DropdownMenu,
DropdownMenuTrigger, DropdownMenuContent, DropdownMenuItem); replace that
implementation with the Popover pattern using `@radix-ui/react-popover`: remove
the DropdownMenu imports, import Popover, PopoverTrigger, PopoverContent from
`@radix-ui/react-popover`, replicate the dropdown’s trigger button as
PopoverTrigger, render the list of actions inside PopoverContent, and manage
open/close state (and keyboard/select behavior) the same way as in
currency-selector.tsx or qr-id-display-preference-selector.tsx—ensure ARIA
attributes, focus management, and custom selection callbacks for items are
preserved when mapping old DropdownMenuItem handlers to interactive elements
inside PopoverContent.
In `@apps/webapp/app/components/asset-model/delete-asset-model.tsx`:
- Around line 26-27: The delete flow currently reads fetcher.state into disabled
via isFormProcessing but the form uses <Form> so the fetcher state isn't tied to
the submission; change the form to use fetcher.Form (i.e. replace <Form> with
<fetcher.Form>), derive the disabled state via the useDisabled hook
(useDisabled(fetcher) or equivalent instead of directly using isFormProcessing),
and add disabled={disabled} to the delete button so the button is disabled
during the fetcher submission; update any references to isFormProcessing to use
the new useDisabled result and ensure the submit uses the fetcher.Form
submission path.
In `@apps/webapp/app/components/asset-model/form.tsx`:
- Around line 70-77: The form currently only falls back to server-side errors
for the name field; ensure every editable field uses the same
"validationErrors-first" pattern so server-side errors are always shown. For
each field rendered in this component (use AssetModelFormSchema and the
getValidationErrors<typeof AssetModelFormSchema>(fetcher.data?.error) result),
change error lookup from zo.errors.<field>()?.message ?? (hasOnSuccessFunc ?
validationErrors?.<field>?.message : undefined) to
validationErrors?.<field>?.message ?? zo.errors.<field>()?.message (remove the
hasOnSuccessFunc gate). Apply this to all fields referenced in the form (the
same pattern used for name should be replicated for fields between lines
89-120).
- Around line 15-24: The schema (AssetModelFormSchema) includes
defaultCategoryId but the form component never renders an input for it; update
the form UI to add a controlled field (e.g., a select or autocomplete) bound to
the form state that reads/writes defaultCategoryId so users can edit it, ensure
the field name matches "defaultCategoryId" used by AssetModelFormSchema and that
its initial value/validation integrates with the transform/optional behavior
currently defined, and persist the value through the submit handler the same way
name/description/defaultValuation are handled.
In `@apps/webapp/app/components/assets/form.tsx`:
- Around line 651-668: The Quantity Input currently only shows client-side
errors via zo.errors.quantity(), so add a server-error fallback by reading
validationErrors for the same field first; update the Input error prop for the
"quantity" field (and the analogous Behavior-mode fields referenced around the
687-709 block) to use validationErrors.quantity?.message ||
zo.errors.quantity()?.message (using the existing getValidationErrors output
stored in validationErrors) so server-side Zod errors display when present
before falling back to client-side errors; ensure you reference the Input with
name="quantity" and the zo.errors.quantity() call when making the change.
In `@apps/webapp/app/modules/asset-model/service.server.ts`:
- Around line 199-211: bulkDeleteAssetModels currently treats ALL_SELECTED_KEY
as "all in org" and drops active filters; change its signature to accept
currentSearchParams (or a similar scoped filter object) and when
assetModelIds.includes(ALL_SELECTED_KEY) build the where clause using the
getAssetsWhereInput helper with takeAll: true (e.g., getAssetsWhereInput({
...currentSearchParams, takeAll: true })) then combine that result with
organizationId before calling db.assetModel.deleteMany; otherwise keep the
existing id in-list behavior. Ensure you reference bulkDeleteAssetModels,
ALL_SELECTED_KEY, getAssetsWhereInput, and pass currentSearchParams from the
route/api layer into this service.
- Around line 176-183: The current deleteAssetModel function returns the result
of db.assetModel.deleteMany which resolves successfully even when count === 0,
causing the UI (_layout+/asset-models.tsx) to show a false success; update
deleteAssetModel to check the deleteMany result and throw an Error when
result.count === 0 (or alternatively use a checked flow:
db.assetModel.findFirst(...org+id) and then db.assetModel.delete(...) if found),
so that stale or cross-organization ids produce a rejected promise and the UI
won't report a successful deletion.
In `@apps/webapp/app/modules/asset/service.server.test.ts`:
- Around line 419-444: The test currently only asserts that quantity/consumption
validation errors are not thrown, which allows unrelated failures to pass;
update the test for createAsset (test "does not throw quantity validation for
INDIVIDUAL assets") to assert a deterministic post-validation outcome: either
stub/mock the downstream dependency that runs after validation (e.g., the
sequential ID generator or persistence call used inside createAsset such as
generateSequentialAssetId or the DB create function) so createAsset successfully
resolves and assert the returned asset shape/ID, or explicitly expect a specific
downstream error by using
expect(createAsset(...)).rejects.toThrow(/expected-post-validation-error/); this
ensures the test verifies behavior-driven observable outcomes rather than merely
the absence of quantity validation messages.
In `@apps/webapp/app/modules/asset/service.server.ts`:
- Around line 1262-1265: The updateAsset handler currently writes quantity,
minQuantity, consumptionType, and unitOfMeasure without the same validation used
elsewhere; update the updateAsset function to first load the persisted asset and
mirror the quantity-tracked guards: if persistedAsset.type === QUANTITY_TRACKED
then require a non-empty consumptionType (do not allow clearing it), enforce
quantity > 0, and enforce minQuantity >= 0 (and optionally minQuantity <=
quantity if your create logic enforces that); for non-QUANTITY_TRACKED assets
strip or ignore quantity-related fields or reject updates that attempt to set
them; apply the same validations to the other update code path referenced (the
block around the second set of writes) so both update locations validate against
the persisted asset and allowed ranges before saving.
In `@apps/webapp/app/routes/_layout`+/asset-models.$assetModelId_.edit.tsx:
- Around line 31-38: The edit route currently redefines
UpdateAssetModelFormSchema and the form, omits defaultCategoryId, and only
surfaces actionData.error.message; instead import and reuse the shared
AssetModelForm and AssetModelFormSchema (or mirror identical fields including
defaultCategoryId) so client/server validation aligns, use
getValidationErrors(AssetModelFormSchema, actionData) to extract
validationErrors and render those field-level errors before client-side errors,
and replace any useNavigation-based disabling with the useDisabled hook to
disable submit controls during submission.
In `@apps/webapp/app/routes/_layout`+/assets.$assetId_.edit.tsx:
- Around line 208-211: The form handler is parsing NewAssetFormSchema's "type"
but never forwards it to updateAsset, causing type changes to be ignored; update
the call sites where the payload currently includes { quantity, minQuantity,
consumptionType, unitOfMeasure } (and the similar second payload) to also
include type, and ensure the destructured variable "type" from the form is used
in those objects passed into updateAsset so updates to the asset type are
persisted.
In `@docs/proposals/quantitative-assets.md`:
- Line 385: The document contains a broken anchor
"#design-note-bookingasset-and-book-by-model" referenced from the BookingAsset
proposal; either add a corresponding section with the exact heading "Design
Note: BookingAsset and Book-by-Model" that explains the BookingAsset vs.
Book-by-Model options (mentioning assetId, assetModelId, ModelBooking and the
three options a/b/c), or remove/replace the link so it points to an existing
heading; update the link text or target so the reference resolves and readers
can find the detailed design note.
- Around line 368-369: The proposed partial index referencing asset.type is
invalid because PostgreSQL partial index predicates can only use columns from
the indexed table; remove the suggested `WHERE asset.type = 'INDIVIDUAL'` usage
and instead implement one of the valid database-level options: add a
discriminator column on the Custody table (e.g., custody.isIndividual) and
create a partial unique index on "Custody"("assetId") WHERE isIndividual = true,
or implement a trigger that checks the related Asset.type and enforces
uniqueness for individual assets, or redesign the schema by separating
IndividualAsset vs QuantityAsset (and index the appropriate table). Update the
text and the SQL examples (including the other occurrence mentioned) to use one
of these correct approaches rather than referencing asset.type from the Custody
partial index.
In
`@packages/database/prisma/migrations/20260331101357_add_asset_model_and_quantity_based_fields/migration.sql`:
- Around line 11-16: The migration currently adds Asset fields but lacks
DB-level CHECKs to prevent invalid quantities; add NOT NULL/constraints and
CHECKs: on "Asset" ensure "quantity" >= 0, "minQuantity" >= 0 and optionally
"quantity" >= "minQuantity"; on the BookingAsset table add CHECK that "quantity"
> 0; on the ConsumptionLog table add CHECK that "quantity" > 0; also restrict
any enum/text fields as needed (references: table "Asset" and columns
"assetModelId", "consumptionType", "minQuantity", "quantity", "type",
"unitOfMeasure", plus "BookingAsset.quantity" and "ConsumptionLog.quantity") so
the migration fails on invalid data rather than allowing negative/zero values.
- Around line 60-64: The current non-unique index
"AssetModel_organizationId_name_idx" allows duplicate model names per
organization; change it to a unique index so DB enforces uniqueness and triggers
the P2002 conflict handling in AssetModel-related logic: replace the CREATE
INDEX for "AssetModel_organizationId_name_idx" with a CREATE UNIQUE INDEX on
"AssetModel" over organizationId and a functional LOWER(name) (to make
uniqueness case-insensitive if desired), ensuring the index name remains
"AssetModel_organizationId_name_idx" so existing code/refs still match.
In `@packages/database/prisma/schema.prisma`:
- Around line 1242-1264: The ConsumptionLog model is missing indexes on the
optional foreign keys bookingId and custodianId; add @@index([bookingId]) and
@@index([custodianId]) to the ConsumptionLog model so queries filtering by
bookingId or custodianId use indexes; update the model block that contains id,
assetId, userId, bookingId, custodianId and the existing @@index entries to
include these two new @@index directives referencing bookingId and custodianId.
- Around line 1271-1280: The BookingAsset model lacks an index on assetId which
hurts queries filtering by asset alone; add a dedicated index for assetId in the
BookingAsset model (e.g., add an @@index([assetId]) or mark the assetId field
with `@index`) to complement the existing @@unique([bookingId, assetId]), then
regenerate the Prisma migration so the DB receives the new index.
---
Outside diff comments:
In `@apps/webapp/app/components/assets/form.tsx`:
- Around line 56-87: The client schema NewAssetFormSchema and the form UI need
two fixes: (1) tighten schema: preprocess numeric fields (quantity, minQuantity,
valuation) to treat "" as undefined (use z.preprocess to convert empty string ->
undefined) and add conditional validation (via .superRefine or a .refine using
the top-level object) so when type === AssetType.QUANTITY_TRACKED you require
quantity (positive int), minQuantity (non-negative int) and consumptionType
(nativeEnum(ConsumptionType)); (2) surface server-side errors in the form:
update the error rendering for the quantity, minQuantity and consumptionType
inputs to use actionData?.errors?.quantity?.message ||
zo.errors.quantity()?.message (and analogous for minQuantity and
consumptionType) so server validation messages are shown alongside client zod
errors. Ensure you reference the NewAssetFormSchema, AssetType, ConsumptionType,
and the form error sources actionData and zo.errors when making changes.
---
Duplicate comments:
In `@docs/proposals/quantitative-assets.md`:
- Around line 331-333: The document currently contradicts itself about the
default action for scanning a quantity-tracked asset QR: the "primary action"
paragraph names "Quick adjust" as the default while "Open Question `#1`" leaves it
undecided; update the RFC to keep a single source of truth by making "Quick
adjust" the default everywhere—specifically, edit the "Open Question `#1`" section
to state that Quick adjust is the default scan action for quantity assets (and
remove/replace any language that treats the default as undecided), and make the
same consistent change where the duplicate wording appears near the "Primary
action" / "Core Concepts" discussion so both references (Quick adjust, Open
Question `#1`) match.
---
Nitpick comments:
In `@apps/webapp/app/hooks/use-sidebar-nav-items.tsx`:
- Around line 134-140: The nav entry for "Asset Models" uses the role shortcut
hidden: isBaseOrSelfService which can drift from actual permissions; replace
that boolean with a permission-derived visibility check (e.g., consult
Role2PermissionMap and the current role or use the existing permission hook) so
the item is hidden only when the current user lacks the assetModel:read
permission; update the "Asset Models" nav item where isBaseOrSelfService is
referenced and use the permission check (reference symbols: the "Asset Models"
nav object, isBaseOrSelfService, Role2PermissionMap, and whichever
currentRole/permission hook function exists in the file) to decide visibility.
In `@apps/webapp/app/routes/_layout`+/assets.$assetId.overview.tsx:
- Around line 423-466: The overview only renders a "Tracked by quantity" badge
when asset?.type === AssetType.QUANTITY_TRACKED, so add a matching
tracking-method list item for the opposite case (identity/individual) inside the
same JSX block: update the conditional around asset?.type ===
AssetType.QUANTITY_TRACKED to render the existing "Tracking method" <li> with
Badge "Tracked by quantity" in the true branch and an else branch that renders
an equivalent <li> with Badge (e.g., "Individual" or "Tracked by identity") so
the INDIVIDUAL/QUANTITY boundary is explicit; use the same surrounding
structure/classes and keep the other fields (Quantity/Min quantity/Behavior
mode) unchanged.
In `@apps/webapp/test/factories/assetModel.ts`:
- Around line 11-21: Replace hardcoded fields in the asset model test factory
with collision-safe generated defaults: generate id (instead of
"asset-model-123"), organizationId and userId, and use dynamic timestamps for
createdAt/updatedAt so multiple tests don't collide; keep the factory override
capability so callers can still pass explicit
id/organizationId/userId/createdAt/updatedAt when needed (update the factory
that returns the object with keys id, name, description, image, imageExpiration,
defaultCategoryId, defaultValuation, organizationId, userId, createdAt,
updatedAt to derive defaults from generators/Date.now()).
In `@packages/database/prisma/schema.prisma`:
- Around line 387-388: The createdBy relation on the AssetModel currently uses
onDelete: Cascade which will remove AssetModel rows when a User is deleted;
change it to onDelete: SetNull and make the userId field optional (e.g., userId
String? and createdBy User? relation) to match the Note/AuditNote pattern so
assets are preserved when their creator is removed; update the relation
definition for createdBy and the userId column accordingly.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8417d0f2-87ed-4c12-9e4e-e1fa4e2562ee
📒 Files selected for processing (29)
apps/webapp/app/components/asset-model/asset-model-quick-actions.tsxapps/webapp/app/components/asset-model/bulk-actions-dropdown.tsxapps/webapp/app/components/asset-model/delete-asset-model.tsxapps/webapp/app/components/asset-model/form.test.tsapps/webapp/app/components/asset-model/form.tsxapps/webapp/app/components/assets/assets-index/advanced-asset-columns.tsxapps/webapp/app/components/assets/form.test.tsapps/webapp/app/components/assets/form.tsxapps/webapp/app/hooks/use-sidebar-nav-items.tsxapps/webapp/app/modules/asset-model/service.server.test.tsapps/webapp/app/modules/asset-model/service.server.tsapps/webapp/app/modules/asset/fields.tsapps/webapp/app/modules/asset/query.server.tsapps/webapp/app/modules/asset/service.server.test.tsapps/webapp/app/modules/asset/service.server.tsapps/webapp/app/modules/asset/types.tsapps/webapp/app/routes/_layout+/asset-models.$assetModelId_.edit.tsxapps/webapp/app/routes/_layout+/asset-models.new.tsxapps/webapp/app/routes/_layout+/asset-models.tsxapps/webapp/app/routes/_layout+/assets.$assetId.overview.tsxapps/webapp/app/routes/_layout+/assets.$assetId_.edit.tsxapps/webapp/app/routes/_layout+/assets.new.tsxapps/webapp/app/utils/error.tsapps/webapp/app/utils/permissions/permission.data.tsapps/webapp/test/factories/assetModel.tsapps/webapp/test/factories/index.tsdocs/proposals/quantitative-assets.mdpackages/database/prisma/migrations/20260331101357_add_asset_model_and_quantity_based_fields/migration.sqlpackages/database/prisma/schema.prisma
✅ Files skipped from review due to trivial changes (4)
- apps/webapp/app/utils/error.ts
- apps/webapp/test/factories/index.ts
- apps/webapp/app/modules/asset/fields.ts
- apps/webapp/app/components/asset-model/form.test.ts
Asset Models: - move to Settings (Workspace Settings > Asset Models) - split route into parent layout + index (fix double breadcrumbs and new/edit stacking on list) - full-page Card-based create/edit forms with FormRow, DynamicSelect for category, currency-prefixed valuation - delete dialog shows warning box with assigned asset count - search includes description (OR filter) - disabled states on all form buttons Asset form: - tracking method redesign: radio cards with descriptions - quantity fields positioned below Name, above Asset ID - consumption type uses Popover-based dropdown with error display and clear-on-select - better validation messages for quantity fields - Asset Model DynamicSelect with placeholder text fix - model selection auto-sets default category via params - model clear removes category param from URL Asset detail page: - Quantity Overview sidebar card with Low Stock badge - removed inline quantity fields from main info column Asset list: - quantity column added to both simple and advanced views - added quantity/unitOfMeasure to raw SQL query and types
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Pull request overview
Introduces the “Quantitative Assets” foundation by extending the database schema and webapp UI to support Asset Models (template/grouping) and quantity-tracked assets (quantity/unit/min threshold/consumption type), including initial settings CRUD for asset models and surfacing quantity info in asset views and exports.
Changes:
- Add Prisma schema + migration for
AssetModel,ConsumptionLog,BookingAsset, and quantity-tracking fields onAsset(with supporting enums). - Add Asset Models settings section (list/create/edit/delete) with permissions and supporting server services/tests.
- Extend asset create/edit/overview/index/CSV/export/query layers to display and persist quantity-tracking + asset model associations (with tests).
Reviewed changes
Copilot reviewed 36 out of 36 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/database/prisma/schema.prisma | Adds AssetModel/ConsumptionLog/BookingAsset models, enums, and quantity/model fields on Asset. |
| packages/database/prisma/migrations/20260331101357_add_asset_model_and_quantity_based_fields/migration.sql | Creates new enums/tables and adds Asset columns + FKs; enables RLS. |
| docs/proposals/quantitative-assets.md | Adds RFC detailing product + technical design for quantitative assets. |
| apps/webapp/test/factories/index.ts | Re-exports new assetModel factory. |
| apps/webapp/test/factories/assetModel.ts | Adds AssetModel test factory. |
| apps/webapp/app/utils/permissions/permission.data.ts | Introduces assetModel permission entity and role mappings. |
| apps/webapp/app/utils/error.ts | Adds “Asset Model” as a failure reason label. |
| apps/webapp/app/utils/csv.server.ts | Adds CSV export support for quantity column formatting. |
| apps/webapp/app/routes/api+/model-filters.ts | Enables DynamicSelect model filtering for assetModel. |
| apps/webapp/app/routes/_layout+/settings.tsx | Adds “Asset models” to settings navigation and role-based filtering. |
| apps/webapp/app/routes/_layout+/settings.asset-models.tsx | Adds parent layout route for asset models + delete action + permission guard. |
| apps/webapp/app/routes/_layout+/settings.asset-models.new.tsx | Adds “create asset model” page route (loader/action). |
| apps/webapp/app/routes/_layout+/settings.asset-models.index.tsx | Adds paginated asset model listing page with bulk/row actions. |
| apps/webapp/app/routes/layout+/settings.asset-models.$assetModelId.edit.tsx | Adds “edit asset model” page route (loader/action). |
| apps/webapp/app/routes/_layout+/assets.new.tsx | Loads asset models into asset create flow; persists assetModelId + quantity fields. |
| apps/webapp/app/routes/_layout+/assets.$assetId.overview.tsx | Shows asset model link and quantity overview card for quantity-tracked assets. |
| apps/webapp/app/routes/layout+/assets.$assetId.edit.tsx | Loads asset models into edit flow; persists assetModelId + quantity fields. |
| apps/webapp/app/modules/asset/types.ts | Extends asset payload types with assetModelId and quantity-related fields. |
| apps/webapp/app/modules/asset/service.server.ts | Adds server-side quantity validation on create; supports assetModel connect/disconnect and quantity field updates. |
| apps/webapp/app/modules/asset/service.server.test.ts | Adds unit tests for quantity validation in createAsset. |
| apps/webapp/app/modules/asset/query.server.ts | Extends raw SQL fragments to return type/quantity/unitOfMeasure. |
| apps/webapp/app/modules/asset/fields.ts | Includes assetModel in asset overview select. |
| apps/webapp/app/modules/asset-model/service.server.ts | Adds CRUD + bulk delete service functions for AssetModel. |
| apps/webapp/app/modules/asset-model/service.server.test.ts | Adds unit tests for asset-model service layer. |
| apps/webapp/app/modules/asset-index-settings/helpers.ts | Adds quantity as a configurable column (labels/defaults). |
| apps/webapp/app/hooks/use-sidebar-nav-items.tsx | Adds “Asset models” under workspace settings navigation. |
| apps/webapp/app/components/assets/quantity-overview-card.tsx | Adds sidebar card to display quantity-tracking details on asset overview. |
| apps/webapp/app/components/assets/form.tsx | Adds tracking method selector, quantity fields, consumption type selector, and asset model selection with default category behavior. |
| apps/webapp/app/components/assets/form.test.ts | Adds schema tests for new asset form quantity fields. |
| apps/webapp/app/components/assets/assets-index/assets-list.tsx | Adds Quantity column in simple mode list view. |
| apps/webapp/app/components/assets/assets-index/advanced-asset-columns.tsx | Adds quantity column rendering + QTY badge for quantity-tracked assets. |
| apps/webapp/app/components/asset-model/form.tsx | Adds AssetModel create/edit form (page + inline/fetcher modes). |
| apps/webapp/app/components/asset-model/form.test.ts | Adds schema tests for asset model form. |
| apps/webapp/app/components/asset-model/delete-asset-model.tsx | Adds delete confirmation dialog and delete form. |
| apps/webapp/app/components/asset-model/bulk-actions-dropdown.tsx | Adds bulk delete actions dropdown for asset models list. |
| apps/webapp/app/components/asset-model/asset-model-quick-actions.tsx | Adds per-row edit/delete quick actions with permission checks. |
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/webapp/app/modules/asset/service.server.ts (1)
1265-1279:⚠️ Potential issue | 🟠 MajorThe new quantity/model edits bypass the asset audit trail.
updateAssetnow acceptsassetModelId,quantity,minQuantity,consumptionType, andunitOfMeasure, but the note pipeline still only snapshots title/description/category/valuation. Updates to these new fields will be silent, which misses the detailed audit trail this feature requires.Also applies to: 1342-1346, 1369-1387
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/webapp/app/modules/asset/service.server.ts` around lines 1265 - 1279, updateAsset now accepts assetModelId, quantity, minQuantity, consumptionType, and unitOfMeasure but the note/audit snapshot still only captures title/description/category/valuation; update the note pipeline’s snapshot logic so any audit note created for updateAsset also records the new fields (assetModelId, quantity, minQuantity, consumptionType, unitOfMeasure) and their previous values, and ensure the same change is applied to the other update paths referenced (the other updateAsset-related note creation spots). Locate the code that builds the asset snapshot for audit notes (the note pipeline / snapshot function used by updateAsset) and add these fields to both the "before" and "after" snapshots so updates to those properties produce proper audit entries.
♻️ Duplicate comments (1)
apps/webapp/app/components/assets/form.tsx (1)
379-391:⚠️ Potential issue | 🟠 MajorSurface validation errors on the quantity-only fields.
quantitystill skips the server fallback,minQuantitynever renders an error prop at all, andConsumptionTypeSelectonly shows the local submit-time message. If the server rejects any of these fields, the submit fails without inline guidance.Possible fix
<Input type="number" label="Quantity" hideLabel name="quantity" disabled={disabled} min={1} step={1} className="w-full" defaultValue={quantity ?? ""} required={true} - error={zo.errors.quantity()?.message} + error={ + actionData?.errors?.quantity?.message ?? + zo.errors.quantity()?.message + } /> ... <Input type="number" label="Min quantity" hideLabel name="minQuantity" disabled={disabled} min={0} step={1} className="w-full" defaultValue={minQuantity ?? ""} + error={ + actionData?.errors?.minQuantity?.message ?? + zo.errors.minQuantity()?.message + } /> ... <ConsumptionTypeSelect defaultValue={consumptionType ?? undefined} disabled={disabled} - error={consumptionTypeError} + error={ + actionData?.errors?.consumptionType?.message ?? + consumptionTypeError + } onSelect={() => setConsumptionTypeError(undefined)} />As per coding guidelines, "All forms must display server-side validation errors as a fallback. Client-side validation can fail or be bypassed, so server-side errors must always be shown. Use
getValidationErrors()utility with Zod schemas and display errors fromvalidationErrorsfirst, then client-side errors".Also applies to: 415-425, 436-441
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/webapp/app/components/assets/form.tsx` around lines 379 - 391, The quantity, minQuantity and ConsumptionTypeSelect fields currently only use client-side errors (e.g., zo.errors.quantity()) and don't render server-side fallbacks; update these components to first display server validation errors from the form-level validationErrors (use the getValidationErrors() helper with your Zod schema) and then fall back to the client errors (zo.errors.*().message). Specifically, change the Input for "quantity" and the analogous Input for "minQuantity" to pass error={validationErrors.quantity ?? zo.errors.quantity()?.message} (or the appropriate key returned by getValidationErrors()), and update ConsumptionTypeSelect to prefer validationErrors.consumptionType (or its schema key) before showing client submit-time messages so server-side rejections surface inline.
🧹 Nitpick comments (2)
apps/webapp/app/routes/_layout+/settings.asset-models.new.tsx (1)
28-63: Document the exported route members.The module-level block is good, but
loader,meta,action, andNewAssetModelare still undocumented. Please add per-export JSDoc here as well.As per coding guidelines, "Every exported function, component, and type must have a JSDoc comment describing parameters, return values, and thrown errors."
Also applies to: 65-110
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/webapp/app/routes/_layout`+/settings.asset-models.new.tsx around lines 28 - 63, Add JSDoc comments for each exported route member: loader, meta, action, and the NewAssetModel component; for each JSDoc include a short description, parameter descriptions (e.g., loader's {context, request} / meta's {data}), return type/shape (what payload or MetaFunction returns), and any errors thrown (e.g., makeShelfError/data error responses or permission failures). Place the JSDoc immediately above each export (referencing the exact exported symbols loader, meta, action, NewAssetModel) and keep them concise but covering params, returns, and thrown errors per the project guideline.apps/webapp/app/routes/_layout+/settings.asset-models.index.tsx (1)
125-128: Clarify whether this column is records or units.The backing query uses
_count.assets, so this is counting related asset rows, not summed quantity. Once a model can back quantity-tracked stock, a single 500-unit row still renders as1here. Either rename this column toRecords/Asset records, or change the query to surface total quantity instead.Also applies to: 170-170
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/webapp/app/routes/_layout`+/settings.asset-models.index.tsx around lines 125 - 128, The table header "<Th>Assets</Th>" is ambiguous because the backing query uses `_count.assets` (count of related rows) rather than summed quantities; update the UI and/or query: either rename the header to "Asset records" or "Records" and keep rendering `_count.assets`, or change the loader/query to return a summed quantity (e.g., sum(asset.quantity) AS totalQuantity) and update the rendering to use that field instead of `_count.assets`; update the header text and the place where `_count.assets` is read to match the chosen approach.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/webapp/app/components/asset-model/form.tsx`:
- Around line 37-41: The server schema for defaultValuation
(z.string().optional().transform(...)) allows invalid numbers and negatives;
update the zod schema for defaultValuation (in the asset model form schema) to
parse/transform to a number and validate numeric-ness and min >= 0 (e.g., change
to z.union([z.string(), z.number()]) or z.string().refine/transform to Number
and add .refine(!Number.isNaN(...)) and .min(0) or use z.number().optional()
with coercion) so NaN and negatives are rejected, and in the FullPageForm
component where the defaultValuation input is rendered add the same pattern used
for the name field: pass the server validation error into the input via the
error prop (e.g., error={fieldErrors?.defaultValuation?.message}) so server-side
validation messages are displayed to the user.
In `@apps/webapp/app/components/assets/form.tsx`:
- Around line 250-266: handleAssetModelChange retains a stale "category" query
param when switching to a model that has no defaultCategoryId; update the logic
in handleAssetModelChange so that after finding the selected model
(assetModelsData.find(...)) you explicitly remove the "category" param when
model exists but model.defaultCategoryId is falsy (i.e., call
params.delete("category") in that branch) so the query only contains category
when the selected model provides a default.
In `@apps/webapp/app/components/assets/quantity-overview-card.tsx`:
- Around line 92-100: The component currently exposes placeholder inventory math
(const available = qty; const inCustody = 0;) and derives behaviorLabel from
consumptionType which treats null as "ONE_WAY" fallback; update the component to
stop presenting fake availability by either: 1) wiring real values via props
like availableQuantity and inCustodyQuantity and using those instead of
available/inCustody (look for symbols available, inCustody, qty,
availableQuantity, inCustodyQuantity), or 2) if real aggregates are not
provided, hide the availability rows and the Low Stock badge entirely (remove
render of those rows driven by available/inCustody) and ensure behaviorLabel
only computes when consumptionType is explicitly set (do not default null to
TWO_WAY). Ensure the change is applied wherever
available/inCustody/behaviorLabel are used (also in the later block around lines
110-132).
In `@apps/webapp/app/modules/asset-index-settings/helpers.ts`:
- Around line 28-29: parseSortingOptions currently doesn't handle sortBy values
for "quantity" (and similarly "upcomingBookings"), so clicking the new fixed
"quantity" column becomes a no-op; update the parseSortingOptions function in
apps/webapp/app/modules/asset/query.server.ts to map sortBy="quantity" (and
sortBy="upcomingBookings" where applicable) to the correct backend/DB field(s)
and return the appropriate order clause/direction based on the requested sort
direction. Ensure the mapping covers all branches referenced in the file (the
existing switch/case or lookup used by parseSortingOptions) so server-side
sorting works when the UI sends sortBy=quantity or sortBy=upcomingBookings.
In `@apps/webapp/app/modules/asset/service.server.ts`:
- Around line 1092-1101: The current connect uses the raw assetModelId from the
client (Object.assign(data, { assetModel: { connect: { id: assetModelId } } })),
which allows cross-organization linking; instead, first resolve/verify the model
row for the current organization (query the AssetModel by id and organizationId)
and only if found set data.assetModel.connect to that verified id (or attach via
the resolved object); apply the same change in both the create and update flows
(the code blocks handling assetModelId at lines around the shown snippet and the
similar block at 1378-1387) so you reject or ignore client-supplied IDs that
don’t belong to the current organization.
- Around line 981-999: When handling asset creation, enforce the INDIVIDUAL vs
QUANTITY_TRACKED boundary by stripping quantity, minQuantity, consumptionType,
and unitOfMeasure from the payload unless type === "QUANTITY_TRACKED";
additionally validate that when type === "QUANTITY_TRACKED" minQuantity is
present and minQuantity >= 0 (throw a ShelfError with status 400 and a clear
message if negative or missing), and if type !== "QUANTITY_TRACKED" ensure any
provided quantity/minQuantity/consumptionType/unitOfMeasure are not persisted
(remove them from the object before saving); apply the same corrections to the
other server-side validation branch that currently checks quantity-tracked
assets so both create/update paths enforce these rules.
In `@apps/webapp/app/routes/_layout`+/settings.asset-models.index.tsx:
- Around line 109-116: The "New asset model" Button (the Button with to="new"
and data-test-id="createNewAssetModel") is visible for users with only
PermissionAction.read but the /new route requires assetModel.create; wrap or
conditionally render this Button so it only shows when the user has the
assetModel.create (PermissionAction.create) permission — e.g., use the project's
permission check helper (hasPermission/can/Authorized component) to test for
assetModel.create before rendering the Button so UI and route permissions stay
in sync.
In `@apps/webapp/app/routes/_layout`+/settings.asset-models.tsx:
- Around line 72-81: The delete flow currently ignores deleteAssetModel’s return
value; change the action to capture its result (e.g., const result = await
deleteAssetModel({ id, organizationId })) and check result.count === 0; if zero,
do NOT call sendNotification and instead return an error payload (or
throw/return { success: false, error: "Asset model not found" }) so callers see
the failure; only call sendNotification and return payload({ success: true })
when result.count > 0. Ensure you reference the same symbols: deleteAssetModel,
sendNotification, and payload.
---
Outside diff comments:
In `@apps/webapp/app/modules/asset/service.server.ts`:
- Around line 1265-1279: updateAsset now accepts assetModelId, quantity,
minQuantity, consumptionType, and unitOfMeasure but the note/audit snapshot
still only captures title/description/category/valuation; update the note
pipeline’s snapshot logic so any audit note created for updateAsset also records
the new fields (assetModelId, quantity, minQuantity, consumptionType,
unitOfMeasure) and their previous values, and ensure the same change is applied
to the other update paths referenced (the other updateAsset-related note
creation spots). Locate the code that builds the asset snapshot for audit notes
(the note pipeline / snapshot function used by updateAsset) and add these fields
to both the "before" and "after" snapshots so updates to those properties
produce proper audit entries.
---
Duplicate comments:
In `@apps/webapp/app/components/assets/form.tsx`:
- Around line 379-391: The quantity, minQuantity and ConsumptionTypeSelect
fields currently only use client-side errors (e.g., zo.errors.quantity()) and
don't render server-side fallbacks; update these components to first display
server validation errors from the form-level validationErrors (use the
getValidationErrors() helper with your Zod schema) and then fall back to the
client errors (zo.errors.*().message). Specifically, change the Input for
"quantity" and the analogous Input for "minQuantity" to pass
error={validationErrors.quantity ?? zo.errors.quantity()?.message} (or the
appropriate key returned by getValidationErrors()), and update
ConsumptionTypeSelect to prefer validationErrors.consumptionType (or its schema
key) before showing client submit-time messages so server-side rejections
surface inline.
---
Nitpick comments:
In `@apps/webapp/app/routes/_layout`+/settings.asset-models.index.tsx:
- Around line 125-128: The table header "<Th>Assets</Th>" is ambiguous because
the backing query uses `_count.assets` (count of related rows) rather than
summed quantities; update the UI and/or query: either rename the header to
"Asset records" or "Records" and keep rendering `_count.assets`, or change the
loader/query to return a summed quantity (e.g., sum(asset.quantity) AS
totalQuantity) and update the rendering to use that field instead of
`_count.assets`; update the header text and the place where `_count.assets` is
read to match the chosen approach.
In `@apps/webapp/app/routes/_layout`+/settings.asset-models.new.tsx:
- Around line 28-63: Add JSDoc comments for each exported route member: loader,
meta, action, and the NewAssetModel component; for each JSDoc include a short
description, parameter descriptions (e.g., loader's {context, request} / meta's
{data}), return type/shape (what payload or MetaFunction returns), and any
errors thrown (e.g., makeShelfError/data error responses or permission
failures). Place the JSDoc immediately above each export (referencing the exact
exported symbols loader, meta, action, NewAssetModel) and keep them concise but
covering params, returns, and thrown errors per the project guideline.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 069a86a9-2e97-4d80-91f4-dd64a53c52d6
📒 Files selected for processing (23)
apps/webapp/app/components/asset-model/delete-asset-model.tsxapps/webapp/app/components/asset-model/form.tsxapps/webapp/app/components/assets/assets-index/advanced-asset-columns.tsxapps/webapp/app/components/assets/assets-index/assets-list.tsxapps/webapp/app/components/assets/form.tsxapps/webapp/app/components/assets/quantity-overview-card.tsxapps/webapp/app/hooks/use-sidebar-nav-items.tsxapps/webapp/app/modules/asset-index-settings/helpers.tsapps/webapp/app/modules/asset-model/service.server.test.tsapps/webapp/app/modules/asset-model/service.server.tsapps/webapp/app/modules/asset/query.server.tsapps/webapp/app/modules/asset/service.server.tsapps/webapp/app/modules/asset/types.tsapps/webapp/app/routes/_layout+/assets.$assetId.overview.tsxapps/webapp/app/routes/_layout+/assets.$assetId_.edit.tsxapps/webapp/app/routes/_layout+/assets.new.tsxapps/webapp/app/routes/_layout+/settings.asset-models.$assetModelId_.edit.tsxapps/webapp/app/routes/_layout+/settings.asset-models.index.tsxapps/webapp/app/routes/_layout+/settings.asset-models.new.tsxapps/webapp/app/routes/_layout+/settings.asset-models.tsxapps/webapp/app/routes/_layout+/settings.tsxapps/webapp/app/routes/api+/model-filters.tsapps/webapp/app/utils/csv.server.ts
✅ Files skipped from review due to trivial changes (2)
- apps/webapp/app/modules/asset-model/service.server.test.ts
- apps/webapp/app/modules/asset-model/service.server.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- apps/webapp/app/routes/_layout+/assets.new.tsx
- apps/webapp/app/routes/layout+/assets.$assetId.edit.tsx
- apps/webapp/app/components/asset-model/delete-asset-model.tsx
- apps/webapp/app/modules/asset/types.ts
- apps/webapp/app/routes/_layout+/assets.$assetId.overview.tsx
Bugs fixed: - bulk actions dropdown now uses asset-model-specific bulk delete dialog instead of category one - delete dialog uses fetcher.Form so disabled state reflects real submission; trigger button is type=button - QuantityOverviewCard shows "—" for null consumptionType instead of incorrectly defaulting to "Returnable" - asset model link on detail page checks permission; shows plain text for users without assetModel:read Service layer: - updateAsset assetModelId uses proper if/else-if pattern for connect/disconnect (removed redundant null check) - deleteAssetModel checks count:0 and throws 404 - quantity fields use getValidationErrors server-error fallback per form validation pattern Schema: - add indexes on ConsumptionLog (bookingId, custodianId) - add index on BookingAsset (assetId) - keep AssetModel name as index not unique (allow dupes) Tests: - improved createAsset INDIVIDUAL test to use explicit assertion pattern instead of try/catch
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)
packages/database/prisma/schema.prisma (1)
1273-1309:⚠️ Potential issue | 🔴 Critical
BookingAssetwill drift until booking writes stop usingBooking.assets.The new pivot is defined here, but the current create/remove/duplicate paths still connect or disconnect through the implicit M2M in
apps/webapp/app/modules/booking/service.server.ts:437-441,3203-3210, and4701-4703. That makesBookingAsset.quantitynon-authoritative immediately, so quantity reservations and later assignment metadata will diverge from actual bookings.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/database/prisma/schema.prisma` around lines 1273 - 1309, The Booking flow still mutates the implicit M2M relation Booking.assets instead of using the new pivot model BookingAsset, causing BookingAsset.quantity to become out-of-sync; update the create/remove/duplicate routines in the booking service code that currently call connect/disconnect on Booking.assets to instead create, update, and delete BookingAsset records (use the bookingAssets relation) so quantity is the authoritative value: on create, insert BookingAsset rows with bookingId, assetId, and quantity; on remove, delete the BookingAsset row rather than disconnecting the M2M; on duplicate, copy BookingAsset rows (including quantity) for the new booking; ensure you honor the @@unique([bookingId, assetId]) constraint when inserting.
♻️ Duplicate comments (6)
apps/webapp/app/components/asset-model/bulk-actions-dropdown.tsx (1)
63-77:⚠️ Potential issue | 🟠 MajorThe controlled menu state won’t close on normal dismiss paths.
Because
onOpenChangeonly callssetOpenon the mobile/defaultApplied branch, outside-click and Escape do not reliably sync the controlledopenstate. The custom backdrop also never callscloseMenu, so the mobile sheet can get stuck open.♻️ Suggested fix
{open && ( <div className={tw( "fixed right-0 top-0 z-10 h-screen w-screen cursor-pointer bg-gray-700/50 transition duration-300 ease-in-out md:hidden" )} + onClick={closeMenu} + aria-hidden="true" /> )} ... - onOpenChange={(open) => { - if (defaultApplied && window.innerWidth <= 640) setOpen(open); - }} + onOpenChange={setOpen}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/webapp/app/components/asset-model/bulk-actions-dropdown.tsx` around lines 63 - 77, The menu's controlled state isn't reliably updated because onOpenChange only calls setOpen when defaultApplied && window.innerWidth <= 640 and the backdrop never triggers a close; update DropdownMenu to receive the controlled open prop (open={open}) and change the onOpenChange handler in DropdownMenu to always call setOpen(openValue) (replace the conditional so it always syncs), and add an onClick handler to the backdrop div that calls setOpen(false) (or the existing closeMenu function) so outside-clicks/Escape and the custom backdrop reliably close the mobile sheet.apps/webapp/app/modules/asset/service.server.ts (2)
1092-1101:⚠️ Potential issue | 🟠 MajorVerify
assetModelIdagainstorganizationIdbefore connecting it.Both branches trust the client-supplied ID and call Prisma
connectby raw primary key. If an ID from another workspace leaks, this links the asset across org boundaries instead of rejecting it. Resolve the model with{ id: assetModelId, organizationId }first and only connect the verified row.🔒 Suggested direction
+const scopedAssetModel = assetModelId + ? await db.assetModel.findFirst({ + where: { id: assetModelId, organizationId }, + select: { id: true }, + }) + : null; + +if (assetModelId && !scopedAssetModel) { + throw new ShelfError({ + cause: null, + message: "Asset model not found", + label, + status: 404, + }); +} ... - assetModel: { - connect: { - id: assetModelId, - }, - }, + assetModel: { + connect: { + id: scopedAssetModel.id, + }, + },Also applies to: 1369-1385
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/webapp/app/modules/asset/service.server.ts` around lines 1092 - 1101, Verify the provided assetModelId belongs to the same organization before using it in a Prisma connect: in the code path that currently uses assetModelId (the block that assigns assetModel via Object.assign and any similar block around lines 1369-1385), first query the AssetModel table (e.g., via prisma.assetModel.findFirst/findUnique) using both id: assetModelId and organizationId to ensure it exists and matches the organization, throw or return an error if not found, and only then add the connect payload (or connect by the verified id) to data; update both occurrences (the assetModel assignment and the other referenced block) to follow this verification flow.
981-999:⚠️ Potential issue | 🟠 MajorCreate/update still let quantity fields leak across the asset-type boundary.
createAsset()validates the happy path forQUANTITY_TRACKED, but both create and update still writequantity,minQuantity,consumptionType, andunitOfMeasurewithout enforcing the persisted asset type. A direct server call can therefore attach quantity metadata toINDIVIDUALassets, clear required fields onQUANTITY_TRACKEDassets, or store negativeminQuantity.Also applies to: 1063-1067, 1342-1346
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/webapp/app/modules/asset/service.server.ts` around lines 981 - 999, The create/update logic currently validates QUANTITY_TRACKED but still persists quantity-related fields across types; update createAsset and updateAsset to only accept and persist quantity, minQuantity, consumptionType, and unitOfMeasure when the asset's resolved type is "QUANTITY_TRACKED" (reject or strip them otherwise), enforce minQuantity >= 0, and on updates prevent clearing required QUANTITY_TRACKED fields (throw ShelfError if an update would remove quantity or consumptionType for a QUANTITY_TRACKED asset); reference and change the validation/assignment around the existing QUANTITY_TRACKED checks in createAsset() and updateAsset() so writes cannot leak across the asset-type boundary.apps/webapp/app/modules/asset/service.server.test.ts (1)
419-439:⚠️ Potential issue | 🟡 MinorAssert a deterministic post-validation outcome in this INDIVIDUAL test.
This still passes on any later failure once the quantity checks are skipped, so it does not prove the INDIVIDUAL path is correct. Stub one known downstream step and assert that exact success/error instead of only checking that the message is “not quantity”.
As per coding guidelines, "Write behavior-driven tests focusing on observable outcomes rather than implementation details".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/webapp/app/modules/asset/service.server.test.ts` around lines 419 - 439, The test currently only asserts that createAsset({..., type: "INDIVIDUAL"}) does not throw a "Quantity is required" message but remains nondeterministic; update the test to stub the known downstream step that fails after validation (e.g., the sequential ID generation or persistence call invoked by createAsset) to return a deterministic value or error, then assert that createAsset either resolves successfully or rejects with that exact stubbed error; specifically, mock the dependency used inside createAsset (the sequential ID generator/persistence method) to a fixed result and replace the expect(...).rejects.toThrow(...) with an assertion that matches the deterministic outcome from the stub.apps/webapp/app/components/assets/form.tsx (2)
421-452:⚠️ Potential issue | 🟠 MajorThe quantity-only fields still lose validation feedback.
minQuantityhas no error prop, andConsumptionTypeSelectonly receives local state. If either field fails validation, the form comes back without inline guidance.As per coding guidelines, "All forms must display server-side validation errors as a fallback. Client-side validation can fail or be bypassed, so server-side errors must always be shown."Possible fix
<FormRow rowLabel="Min quantity" className="border-b-0 pb-[10px]" subHeading="Low-stock alert threshold. You will be notified when available quantity falls to or below this number." > <Input type="number" label="Min quantity" hideLabel name="minQuantity" disabled={disabled} min={0} step={1} className="w-full" defaultValue={minQuantity ?? ""} + error={ + validationErrors?.minQuantity?.message || + zo.errors.minQuantity()?.message + } /> </FormRow> @@ <ConsumptionTypeSelect defaultValue={consumptionType ?? undefined} disabled={disabled} - error={consumptionTypeError} + error={ + validationErrors?.consumptionType?.message || + zo.errors.consumptionType()?.message || + consumptionTypeError + } onSelect={() => setConsumptionTypeError(undefined)} />🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/webapp/app/components/assets/form.tsx` around lines 421 - 452, The minQuantity Input and ConsumptionTypeSelect are not receiving server-side validation errors; add an error prop to the Input for "minQuantity" (e.g., pass minQuantityError as error to the Input) and ensure ConsumptionTypeSelect gets the authoritative consumptionTypeError from the server-side errors (not just local state) via its error prop; keep the existing onSelect={() => setConsumptionTypeError(undefined)} behavior to clear errors on user change and ensure the form error mapping populates minQuantityError and consumptionTypeError that are passed into Input and ConsumptionTypeSelect respectively.
258-269:⚠️ Potential issue | 🟠 MajorDelete the old
categoryparam when the next model has no default.Switching from a model that sets
categoryto one that doesn't leaves the previous query param behind. The edit loader then prefers that stale search param overasset.categoryId(apps/webapp/app/routes/_layout+/assets.$assetId_.edit.tsx:110-115), so custom fields and the eventual submission can stay pinned to the wrong category.Possible fix
} else if (assetModelsData) { const model = assetModelsData.find((m) => m.id === modelId); if (model?.defaultCategoryId) { params.set("category", model.defaultCategoryId); + } else { + params.delete("category"); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/webapp/app/components/assets/form.tsx` around lines 258 - 269, The handler handleAssetModelChange currently only deletes the "category" search param when modelId is undefined, leaving a stale category when switching to a model that exists but lacks defaultCategoryId; update handleAssetModelChange (and use assetModelsData to find the selected model) to explicitly delete params.delete("category") when the found model has no defaultCategoryId, otherwise set the param to model.defaultCategoryId as now, ensuring the query param is removed whenever the new model does not supply a default.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/webapp/app/components/asset-model/bulk-delete-dialog.tsx`:
- Around line 26-37: The select-all branch using ALL_SELECTED_KEY is not guarded
by the active filters, so when isSelectingAllItems(selectedAssetModels) is true
the dialog can trigger deletes outside the current filtered result set; fix by
passing the currentSearchParams into the BulkUpdateDialogContent when select-all
is active (add a prop like currentSearchParams={currentSearchParams} or similar
alongside arrayFieldId/actionUrl/title), ensure the route/API handler extracts
and forwards currentSearchParams when it sees ALL_SELECTED_KEY, and ensure the
service uses getAssetsWhereInput({ currentSearchParams, takeAll: true }) to
scope deletion to the filtered set; reference symbols: ALL_SELECTED_KEY,
isSelectingAllItems, selectedAssetModels, BulkUpdateDialogContent,
currentSearchParams, and getAssetsWhereInput/takeAll.
In `@packages/database/prisma/schema.prisma`:
- Around line 197-206: Asset quantity semantics are incomplete: add a
quantity-aware custody model and DB checks so QUANTITY_TRACKED assets can be
split across custodians and INDIVIDUAL assets keep quantity fields null. Remove
the unique constraint on Custody.assetId, add a quantity:Int (or Decimal) field
to model Custody (e.g. in class/Model Custody add quantity:Int), and add
schema-level check constraints (using Prisma @@check / migration SQL) that
enforce when Asset.assetType = QUANTITY_TRACKED then asset.quantity,
minQuantity, consumptionType, unitOfMeasure are NOT NULL and Custody.quantity is
NOT NULL and sum(Custody.quantity) <= Asset.quantity, and when Asset.assetType =
INDIVIDUAL those quantity fields (on Asset and Custody) must be NULL and
Custody.assetId may remain unique; update relations referencing
assetModelId/assetModel as needed to keep referential integrity.
---
Outside diff comments:
In `@packages/database/prisma/schema.prisma`:
- Around line 1273-1309: The Booking flow still mutates the implicit M2M
relation Booking.assets instead of using the new pivot model BookingAsset,
causing BookingAsset.quantity to become out-of-sync; update the
create/remove/duplicate routines in the booking service code that currently call
connect/disconnect on Booking.assets to instead create, update, and delete
BookingAsset records (use the bookingAssets relation) so quantity is the
authoritative value: on create, insert BookingAsset rows with bookingId,
assetId, and quantity; on remove, delete the BookingAsset row rather than
disconnecting the M2M; on duplicate, copy BookingAsset rows (including quantity)
for the new booking; ensure you honor the @@unique([bookingId, assetId])
constraint when inserting.
---
Duplicate comments:
In `@apps/webapp/app/components/asset-model/bulk-actions-dropdown.tsx`:
- Around line 63-77: The menu's controlled state isn't reliably updated because
onOpenChange only calls setOpen when defaultApplied && window.innerWidth <= 640
and the backdrop never triggers a close; update DropdownMenu to receive the
controlled open prop (open={open}) and change the onOpenChange handler in
DropdownMenu to always call setOpen(openValue) (replace the conditional so it
always syncs), and add an onClick handler to the backdrop div that calls
setOpen(false) (or the existing closeMenu function) so outside-clicks/Escape and
the custom backdrop reliably close the mobile sheet.
In `@apps/webapp/app/components/assets/form.tsx`:
- Around line 421-452: The minQuantity Input and ConsumptionTypeSelect are not
receiving server-side validation errors; add an error prop to the Input for
"minQuantity" (e.g., pass minQuantityError as error to the Input) and ensure
ConsumptionTypeSelect gets the authoritative consumptionTypeError from the
server-side errors (not just local state) via its error prop; keep the existing
onSelect={() => setConsumptionTypeError(undefined)} behavior to clear errors on
user change and ensure the form error mapping populates minQuantityError and
consumptionTypeError that are passed into Input and ConsumptionTypeSelect
respectively.
- Around line 258-269: The handler handleAssetModelChange currently only deletes
the "category" search param when modelId is undefined, leaving a stale category
when switching to a model that exists but lacks defaultCategoryId; update
handleAssetModelChange (and use assetModelsData to find the selected model) to
explicitly delete params.delete("category") when the found model has no
defaultCategoryId, otherwise set the param to model.defaultCategoryId as now,
ensuring the query param is removed whenever the new model does not supply a
default.
In `@apps/webapp/app/modules/asset/service.server.test.ts`:
- Around line 419-439: The test currently only asserts that createAsset({...,
type: "INDIVIDUAL"}) does not throw a "Quantity is required" message but remains
nondeterministic; update the test to stub the known downstream step that fails
after validation (e.g., the sequential ID generation or persistence call invoked
by createAsset) to return a deterministic value or error, then assert that
createAsset either resolves successfully or rejects with that exact stubbed
error; specifically, mock the dependency used inside createAsset (the sequential
ID generator/persistence method) to a fixed result and replace the
expect(...).rejects.toThrow(...) with an assertion that matches the
deterministic outcome from the stub.
In `@apps/webapp/app/modules/asset/service.server.ts`:
- Around line 1092-1101: Verify the provided assetModelId belongs to the same
organization before using it in a Prisma connect: in the code path that
currently uses assetModelId (the block that assigns assetModel via Object.assign
and any similar block around lines 1369-1385), first query the AssetModel table
(e.g., via prisma.assetModel.findFirst/findUnique) using both id: assetModelId
and organizationId to ensure it exists and matches the organization, throw or
return an error if not found, and only then add the connect payload (or connect
by the verified id) to data; update both occurrences (the assetModel assignment
and the other referenced block) to follow this verification flow.
- Around line 981-999: The create/update logic currently validates
QUANTITY_TRACKED but still persists quantity-related fields across types; update
createAsset and updateAsset to only accept and persist quantity, minQuantity,
consumptionType, and unitOfMeasure when the asset's resolved type is
"QUANTITY_TRACKED" (reject or strip them otherwise), enforce minQuantity >= 0,
and on updates prevent clearing required QUANTITY_TRACKED fields (throw
ShelfError if an update would remove quantity or consumptionType for a
QUANTITY_TRACKED asset); reference and change the validation/assignment around
the existing QUANTITY_TRACKED checks in createAsset() and updateAsset() so
writes cannot leak across the asset-type boundary.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: fcf72193-4e4d-4533-8a02-c5d4c063c105
📒 Files selected for processing (10)
apps/webapp/app/components/asset-model/bulk-actions-dropdown.tsxapps/webapp/app/components/asset-model/bulk-delete-dialog.tsxapps/webapp/app/components/asset-model/delete-asset-model.tsxapps/webapp/app/components/assets/form.tsxapps/webapp/app/components/assets/quantity-overview-card.tsxapps/webapp/app/modules/asset-model/service.server.tsapps/webapp/app/modules/asset/service.server.test.tsapps/webapp/app/modules/asset/service.server.tsapps/webapp/app/routes/_layout+/assets.$assetId.overview.tsxpackages/database/prisma/schema.prisma
✅ Files skipped from review due to trivial changes (1)
- apps/webapp/app/modules/asset-model/service.server.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/webapp/app/components/assets/quantity-overview-card.tsx
…ain sync - Reclassify Sub-phase 3b + 3c as COMPLETED (3b shipped 13eed84 + TOCTOU hardening 9f7642b; 3c shipped 60bfebe / 1fd4ad5 / e92dfd5). - New section: Sub-phase 3d-Polish: Fulfil-and-Checkout Flow (commits c1596f2 + 1a5a12f). Documents the /bookings/:id/overview/fulfil-and-checkout route, fulfil-reservations drawer, fulfilModelRequestsAndCheckout service, and the audit-trail schema change (fulfilledQuantity + fulfilledAt) that splits the Models tab into Active + Fulfilled (read-only) sections. - Migrations applied: add entries 6, 7, 8 covering Phase 3d (BookingModelRequest), Phase 3d-Polish (fulfilled tracking), and main's PR #2495 ActivityEvent table. - Files-created list: extend with booking-model-request module, manage-model-requests + model-request-row-actions-dropdown components, fulfil-reservations-drawer, the two new hooks (use-booking-checkin-session-initialization and use-booking-fulfil-session-initialization), and the two new routes. - New section: Last sync with main — table of the two merges from 2026-04-29 (a613f42 then a64d8c2 for PR #2495 / Activity Events / Reports) with a clear note that main's reports + seed scripts are pending a Phase-3a/Phase-2 port (~60 typecheck errors), but the ActivityEvent table and recordEvent integration are wired and clean. - Update testing status: pre-merge baseline 1865/1865 tests at a613f42 (was 1747); post-merge typecheck status flagged with pointer to the reports port.
main's PR #2495 (Activity Events / Reports) was written against the pre-Phase-3a schema where Asset and Booking had an implicit M2M via the `bookings` / `assets` fields, and against the pre-Phase-2 schema where `Asset.custody` was `Custody?` (one-to-one). After the merge our schema has `BookingAsset` (explicit pivot) and `Custody[]` (array). This commit makes main's queries compile and run against the merged schema: - `reports/helpers.server.ts` (8 query sites): walk `bookingAssets` pivot for both reads (`booking.bookingAssets.map(ba => ba.asset)`, `asset.bookingAssets.map(ba => ba.booking)`) and filters (`{ bookingAssets: { some: { booking: { ... } } } }`). Sub-queries that ordered/took on Booking now order on `booking.to` through the pivot. `_count.assets` → `_count.bookingAssets`. Inventory report reads `a.custody[0]?.custodian?.name` for the Phase-2 array. - `scripts/seed-reporting-demo/phases/bookings.ts` (1 site): `assets: { connect: ... }` → `bookingAssets: { create: assetIds.map( (id) => ({ asset: { connect: { id } } })) }` for the explicit pivot. - `asset/service.server.ts:4100` (bulk release-custody activity event): use `getPrimaryCustody` to pick the representative custodian for the CUSTODY_RELEASED event payload instead of `asset.custody!.custodian`. - `partial-checkin-drawer.tsx`: removed the nested `BookingHeader` inside `PartialCheckinDrawer` that shadowed the module-level one hoisted from main; the call site at `headerContent={<BookingHeader booking={booking}/>}` now resolves to the module-level component. Widened `BookingHeaderBooking.from / .to` to `Date | string` so the loader's serialized booking can be passed without a Pick<Booking> mismatch. `pnpm webapp:validate` green (typecheck + lint + 136 test files / 1892 tests). Reports + seed scripts compile; runtime verification on the report endpoints is the next step (task #68).
All 9 lint warnings from `pnpm webapp:lint` are now cleared.
reports/helpers.server.ts:
- bookingComplianceReport: implement the previously-TODO `locationId`
filter via the BookingAsset pivot — `where.bookingAssets = { some: {
asset: { locationId } } }`.
- 5 internal helpers (`computeOverdueKpis`, `fetchIdleAssetRows`,
`countIdleAssets`, `computeIdleAssetsKpis`, `computeCustodyKpis`,
`computeInventoryKpis`) accepted `organizationId` but never used it.
Apply it explicitly into each query's `where` as a defense-in-depth
guard alongside the caller's filter — cheap insurance against an
accidental cross-org leak if the where-builder ever regresses.
- assetDistributionReport: drop the `db.asset.count({ where: {
organizationId } })` whose result was destructured into `totalAssets`
and never read — saves a DB round-trip.
reports.export.$fileName[.csv].tsx: drop unused `Params` type import.
reports.tsx: add `meta` export with "Reports" title (enforced by the
local-rules/require-meta-export-in-routes ESLint rule).
editor-v2.test.tsx: switch `act` import from the deprecated
`react-dom/test-utils` package to the modern `react` re-export, per
the React 18+ migration guidance.
`pnpm webapp:validate` green: 0 lint warnings, 0 errors,
136 test files / 1892 tests passing.
The 153 "An update to TestComponent was not wrapped in act(…)" warnings
remaining in `pnpm webapp:test` stderr come from `use-api-query.test.ts`
— the hook fires effect-driven setState that resolves between
`renderHook` and the next `await waitFor`. These are React 18
strictness on a pre-existing test file (added in 8ada9c0,
`use-api-query.test.ts` predates the merge). Silencing them properly
requires restructuring each test to wrap the hook setup in
`act(async () => {...})`, which loses the ability to assert on
intermediate `isLoading=true` state. Tracked as a separate cleanup; the
tests still all pass.
🩺 React DoctorFindings on the files changed by this PR:
❌ Errors
|
… actions Integrates the quantity-tracked + Phase-3d service paths with main's Activity Events / Reports system (PR #2495). Strategy: emit existing main ActivityAction values from feat-quantities code paths so the reports queries main brought in keep working for qty-tracked custody flows + booking-model-request fulfilment + bulk-archive/cancel. No new ActivityAction enum values, no schema changes — the quantity-specific reports (restock totals, loss totals, etc.) will read from ConsumptionLog directly in a follow-up PR rather than duplicating that data into ActivityEvent meta payloads. Asset / quantity gaps (apps/webapp/app/modules/asset/service.server.ts): - checkOutQuantity (Phase 3b) — emit CUSTODY_ASSIGNED inside the tx with `meta: { quantity, viaQuantity: true }` so qty-tracked custody participates in main's Custody-Changes KPI + custody-window pairing. - releaseQuantity — same pattern, CUSTODY_RELEASED. - bulkDeleteAssets — pre-fetch ids+titles, emit ASSET_DELETED per id via recordEvents with `meta: { title }`. - bulkUpdateAssetCategory — wrap in a tx, pre-fetch previous categoryIds, emit ASSET_CATEGORY_CHANGED only for actually-changed assets (with field/fromValue/toValue). - bulkAssignAssetTags — emit ASSET_TAGS_CHANGED post-update only when the sorted tag-id arrays differ. - deleteAsset (single) was already covered by main's source. Booking gaps (apps/webapp/app/modules/booking/service.server.ts): - extendBooking — emit BOOKING_DATES_CHANGED post-tx; conditional BOOKING_STATUS_CHANGED when extension flips OVERDUE → ONGOING. - bulkArchiveBookings / bulkCancelBookings — emit BOOKING_ARCHIVED / BOOKING_CANCELLED per booking inside the existing tx via recordEvents. Side-effect: bulkArchiveBookings now threads `userId` through to createStatusTransitionNote so the per-booking status events finally have actor attribution (was a pre-existing gap). Bulk-actions route at api+/bookings.bulk-actions.ts:79 forwards the authenticated userId. - addScannedAssetsToBookingWithinTx — emit BOOKING_ASSETS_ADDED per scanned asset inside the tx (parity with updateBookingAssets:3948). - updateBasicBooking — emit BOOKING_DATES_CHANGED for from/to changes (one event per field). - duplicateBooking — wrap creation in a tx, emit BOOKING_CREATED + per copied-asset BOOKING_ASSETS_ADDED. - fulfilModelRequestsAndCheckout — emit per-asset BOOKING_CHECKED_OUT for the post-scan asset set (parity with checkoutBooking:1768); pairs with the new in-tx BOOKING_ASSETS_ADDED above so combined fulfil-and-checkout events match the standalone paths. - deleteBooking + bulkDeleteBookings: skipped — main's enum has no BOOKING_DELETED action. Test fixes: - asset/service.server.test.ts: extend the db.server mock with the surfaces the new in-tx code reads (asset.findMany/updateMany/ deleteMany, custody.findUnique/delete/update, teamMember.findUnique); add module-level mocks for ~/modules/activity-event/service.server and ./bulk-operations-helper.server. +7 contract tests covering every new emission's payload + no-op filtering. - booking/service.server.test.ts: add booking.updateMany, note.createMany mocks. Update existing extendBooking, duplicateBooking, fulfilModelRequestsAndCheckout tests to assert events. +4 new tests for updateBasicBooking, bulkArchiveBookings, bulkCancelBookings, addScannedAssetsToBooking. `pnpm webapp:validate` green: 0 lint warnings/errors, 0 typecheck errors, 136 test files / 1903 tests passing (+11 new). Plan reference: PLAN-ACTIVITY-EVENTS-INTEGRATION.md (worktree root). Out of scope, tracked separately: extending reports/helpers.server.ts to query ConsumptionLog for quantity-specific reports (restock / loss / consumption KPIs); model-request lifecycle reporting (already covered by BookingModelRequest scalar columns, no events needed).
Resolves the three correctness bugs that had been outstanding since
Phase 2/3a, plus a cluster of folded-in fixes caught during manual
testing of the kit-custody flow. Tracking doc:
TESTING-KIT-CUSTODY-CORRECTNESS.md.
Schema:
- Custody.kitCustodyId TEXT NULL, FK → KitCustody.id with
ON DELETE CASCADE / ON UPDATE CASCADE. Inverse relation
KitCustody.inheritedCustody[]. Index on kitCustodyId.
- Migration 20260430100759_add_kit_custody_id_to_custody includes a
one-shot backfill UPDATE that walks KitCustody → Kit → Asset and
matches Custody.teamMemberId = KitCustody.custodianId to tag
pre-existing kit-allocated rows. Local dev DB has 0 KitCustody
parents (vacuous backfill); production has 628 — needs
staging/snapshot validation per testing doc Path B.
Issue A — Asset-index duplicate rows for multi-custodian qty assets:
- Replaced per-custody LEFT JOIN with LEFT JOIN LATERAL +
jsonb_agg(...) AS custody in asset/query.server.ts.
- Dropped cu.id, tm.name, u.* from GROUP BY in service.server.ts.
- Custody column on advanced + simple index renders primary
custodian inline + +N more chip with tooltip listing every
custodian. Outer wrapper uses flex-wrap so the chip drops to a
second line on narrow columns instead of cropping.
Issue B — Kit-custody quantity = 1 regardless of asset qty:
- New buildKitCustodyInheritData helper reads existing custody for
each asset inside the tx and writes
quantity = asset.quantity − Σ(existing custody) for QUANTITY_TRACKED,
quantity = 1 for INDIVIDUAL. Fully-allocated assets are skipped
(remaining ≤ 0 branch — no kit row created).
- Three call sites refactored to use the helper inside their tx:
updateKitAssets, bulkAssignKitCustody, kits.\$kitId.assets.assign-custody.
Issue C — Kit removal wipes ALL custody:
- Filtered every kit→asset custody deleteMany by { kitCustodyId } so
operator-assigned rows on the same asset survive.
- bulkReleaseKitCustody / releaseCustody (kit) / deleteKit use
emit-first-then-cascade: query rows, emit CUSTODY_RELEASED events,
delete the parent KitCustody (FK cascade removes children).
- Asset.status flip is conditional — only flips to AVAILABLE when
zero remaining Custody rows exist after the removal.
Folded-in fixes (correctness + UX):
Picker filter:
- ?status=AVAILABLE on manage-assets pickers now includes qty-tracked
rows even when their row.status flipped to IN_CUSTODY (because any
unit was allocated). Fix in asset/service.server.ts and
asset/utils.server.ts. IN_CUSTODY / CHECKED_OUT semantics unchanged.
Custody-filter SQL:
- Replaced 7 cu.id IS NULL / IS NOT NULL clauses in addCustodyFilter
with jsonb_array_length(custody_agg.custody) > 0 / = 0 to match the
new lateral. The EXISTS subqueries are unaffected.
Kit-aware qty display on kit detail page:
- When the kit is in custody, qty-tracked rows show
· N / M units in kit where N = kit-allocated and M = asset.quantity.
When kit not in custody, falls back to · M units.
releaseQuantity status flip:
- Was deleting the Custody row but never updating Asset.status. Now
counts remaining custody rows after delete; flips to AVAILABLE when
zero remain. Mirrors the kit-flow conditional-flip pattern.
Custody Breakdown — Via kit badge:
- Kit-allocated rows in the asset overview's Custody Breakdown card
now render a blue Via kit badge with a tooltip linking to the
parent kit, instead of a Release button. Releasing a kit-allocated
row directly would corrupt state.
- Server-side guard in releaseQuantity: throws 400 if
custody.kitCustodyId is set (defense in depth).
QuantityCustodyDialog informational note:
- When the asset is in any kit, a soft blue note tells the user
operator custody is tracked separately from the kit's allocation.
- Phase 4 follow-up documented in PRD: revise this copy once the
rebalance feature ships.
Filter-UI infinite-loop fix:
- value-field.tsx was calling setFilter('') on enum fields with no
default, retriggering its own effect on every parent re-render.
Fixed by removing the no-op call and gating the cf_ branch on a
non-empty default. Caused 'Maximum update depth exceeded' on
Add Filter for the assetModel column.
Other display fixes:
- Location detail page asset list shows · N units for qty-tracked.
- Add-to-kit scanner drawer shows the same suffix when scanning
qty-tracked items.
Tests:
- 4 new tests in kit/service.server.test.ts covering Option B
subtraction, fully-allocated → skip branch, kit-custody threading,
dual-custody preservation on removal.
- 2 new releaseQuantity tests in asset/service.server.test.ts.
- 1 new test in kits.\$kitId.assets.assign-custody.test.tsx for
Option B at the route level.
- query.server.test.ts assertion strings updated.
- New custody-column.test.tsx covering multi-custodian rendering.
- New kits.\$kitId.test.tsx route smoke.
Validate: pnpm webapp:validate green — 138 / 1930 tests passing.
Docs:
- TESTING-KIT-CUSTODY-CORRECTNESS.md: full manual checklist.
- docs/proposals/quantitative-assets.md: Phase 4 bullets added for
rebalance feature + reviewing the dialog informational note.
- CLAUDE-CONTEXT.md: new Sub-phase 3d-Polish-2 section, migrations
list updated, Known Issues cleared.
![hex-security-app[bot]](https://avatars.githubusercontent.com/in/2665233?s=60&v=4){kind=small}
The two phase 3 booking-mutation endpoints only called requirePermission(booking:update). booking:update is granted to SELF_SERVICE and BASE in Role2PermissionMap, so any user in those roles could hit the endpoints with another user's bookingId in the same org and (a) upsert/delete model-level reservations or (b) inflate or shrink the booked quantity of any QUANTITY_TRACKED reservation — burning pool capacity on bookings they don't own. Reported by hex-security-app[bot] on PR #2337 (medium severity, both findings): - r3199039007 — model-requests endpoint - r3199039448 — adjust-asset-quantity endpoint Both routes now match the guard pattern used elsewhere in the booking surface (overview page, calendar export, generate-pdf): after requirePermission, branch on isSelfServiceOrBase and call validateBookingOwnership with the booking's creatorId/custodianUserId. Routes: - api+/bookings.$bookingId.model-requests.ts: extra db.booking.findFirst scoped to organizationId so non-existent / cross-org bookings return 404 (no existence leak), then validateBookingOwnership for SS/BASE. - api+/bookings.$bookingId.adjust-asset-quantity.ts: reuses the existing bookingAsset.findFirst — adds creatorId + custodianUserId to the booking select so the guard runs without an extra query. Service layer is unchanged: a single chokepoint at the route layer is easier to reason about than dual enforcement, and matches how the other booking routes already handle this. Tests (15 new): - routes-tests/api+/bookings.$bookingId.model-requests.test.ts (8) - routes-tests/api+/bookings.$bookingId.adjust-asset-quantity.test.ts (7) Each suite covers SELF_SERVICE/BASE rejection on someone else's booking, allow-when-creator, allow-when-custodian, ADMIN/OWNER bypass (no ownership lookup performed), and 404 leakage protection. pnpm webapp:validate green: 140 files, 1945 tests (+15).
Three small UX tweaks on the /kits/:id sub-routes plus a testing-doc
clarification.
Title:
The /kits/:id/assets and /kits/:id/bookings pages hardcoded their
header to "Kit assets" / "Kit Bookings" regardless of which kit you
were on. Both now follow the sibling overview route's pattern and
render `${kit.name}'s assets` / `${kit.name}'s bookings`. Each loader
runs the existing data fetch in parallel with a tiny org-scoped
db.kit.findFirst lookup for the name to avoid a round-trip; falls
back to the old literal if the kit isn't found (e.g. just deleted).
Qty display on qty-tracked rows in /kits/:id/assets:
Pre-fix the row only showed the kit-allocated/total fraction when the
kit was in custody; otherwise it fell back to "· {total} units",
which was misleading once the asset had operator-allocated units. The
kit won't actually receive all units when later assigned — Option B
will only flow asset.quantity − sum(operator custody) into the kit
row.
Both branches now always render `· N / M units in kit`:
- Kit IS in custody → N = sum of Custody rows tagged with this
KitCustody.id (kit-allocated count after Option B subtraction).
- Kit IS NOT in custody → N = asset.quantity − sum(all custody) = the
units that *would* flow into the kit on assign. An asset belongs to
at most one kit, so when the kit has no KitCustody row every Custody
row is operator-allocated, making the math safe.
In the no-operator-custody case both branches naturally produce
`M / M units in kit`, which is accurate (every unit is part of the
kit).
Testing doc:
TESTING-KIT-CUSTODY-CORRECTNESS.md section 4d was misleading — it
told testers to bulk-select the kit on /kits, but
bulkRemoveAssetsFromKits is wired only to the assets index. Updated
the steps to point at the right surface (assets index → multi-select
→ "Remove from kit") so testers exercise the function as it actually
ships.
Three correctness bugs and one UX polish, all surfaced during manual
verification of the Phase 3d-Polish-2 testing checklist.
1. checkOutQuantity missing Asset.status flip
-----------------------------------------------
`releaseQuantity` (Phase 3d-Polish-2) conditionally flips
Asset.status to AVAILABLE when the last Custody row is released, but
the symmetric path on assignment was never added. Operator-side
qty-custody assignments wrote the Custody row + emitted
CUSTODY_ASSIGNED but left Asset.status = AVAILABLE, drifting away
from the Custody table state.
Concrete failure: a qty-tracked asset with 100/100 units assigned
operator-side still read as `AVAILABLE` to every consumer that gates
on `Asset.status === "AVAILABLE"` (kit-assign route guard, picker
filters, list badges). Most importantly, the kit-assign route's
"all assets must be AVAILABLE" check passed for a fully-allocated
asset, then `buildKitCustodyInheritData` would skip it via Option B
— silently producing a kit-in-custody whose presumed-allocated
asset was actually fully held elsewhere.
Fix: `apps/webapp/app/modules/asset/service.server.ts:checkOutQuantity`
now writes `Asset.status = IN_CUSTODY` after upserting the Custody
row (Step 6b). Constant write — no-op when already IN_CUSTODY, but
needed to cover the AVAILABLE → IN_CUSTODY transition. Existing
test mock for `db.asset.update.mockRejectedValue(P2025)` from the
sibling `refreshExpiredAssetImages` suite leaks across
`clearAllMocks` (which only clears history, not implementations);
two test setups now restore the resolve.
2. deleteKit / bulkDeleteKits skipped application logic
--------------------------------------------------------
Both functions were single-line `db.kit.delete()` / `deleteMany()`
calls relying entirely on FK cascade for cleanup. The cascade
correctly removes KitCustody → child Custody rows, but bypassed:
- `Asset.status` conditional flip → assets stuck at IN_CUSTODY with
zero remaining custody rows (orphaned status).
- `CUSTODY_RELEASED` activity events → kit-deletion was invisible
in the asset activity feed.
- Asset notes → no audit trail of "kit X was deleted".
Fix: extracted a shared `performKitDeletion` helper that mirrors
`releaseCustody` (kit)'s pattern for both single-kit and bulk
paths. Pre-reads inherited Custody rows before the cascade fires,
emits `CUSTODY_RELEASED` events with `meta: { viaKit: true,
viaKitDelete: true }` (the new `viaKitDelete` flag distinguishes
delete-flow from release-flow events), runs the kit deletion,
conditionally flips `Asset.status` to AVAILABLE for assets with
zero remaining custody (preserving operator custody), then writes
asset notes outside the tx. `viaKitDelete` is also covered in the
new tests.
`deleteKit` callsite in `kits.$kitId.tsx` updated to pass `userId`.
Tests for both functions rewritten + new in-custody-delete tests
added covering Option B preservation (operator custody survives,
status only flips on assets that lose their last row).
3. Add-assets-to-location scan drawer missing qty suffix
---------------------------------------------------------
The kit scan drawer renders `· 100 units` on qty-tracked rows; the
location scan drawer rendered just the title. Same `AssetFromQr`
shape feeds both — fix is a 6-line render addition + import.
4. Testing doc clarifications
------------------------------
Three sections in TESTING-KIT-CUSTODY-CORRECTNESS.md updated to
reflect what's actually testable post-fix:
- **4d (`bulkRemoveAssetsFromKits`)** — wording corrected: action is
on the assets index, not the kits listing.
- **6b (Option B skip branch)** — marked as "covered by unit test,
not manually testable". With `checkOutQuantity` now correctly
flipping status, every UI path into
`buildKitCustodyInheritData`'s skip-when-remaining-≤-0 branch is
fenced by a route or picker guard. The branch remains in the
helper for race-condition defense in depth.
- **11a (`@@unique([assetId, teamMemberId])`)** — marked as
"verified at DB + UI layer". UI guards prevent reaching the
constraint; raw-SQL test confirms the constraint fires with the
expected `Custody_assetId_teamMemberId_key` violation that
Prisma surfaces as P2002.
Validation
-----------
- `pnpm webapp:validate` green: 164 files / 2098 tests
(+1 over the pre-fix baseline of 2097, from the new bulk
in-custody-delete test; the existing two `deleteKit` tests were
rewritten so net is +1 not +3).
- Schema-level cascades manually verified via supabase-local MCP
on dev DB: `KitCustody` delete cascades only to children with
matching `kitCustodyId` (operator-tagged rows untouched);
`Kit` delete cascades the full chain Kit → KitCustody → Custody
but leaves operator-tagged Custody and Asset rows intact.
… layer Closes hex-security r3202161632 + r3202162994 (both medium-severity) on PR #2337. Two mobile bulk-custody endpoints introduced by main's mobile companion merge shipped without the SELF_SERVICE checks the web routes carry — letting SELF_SERVICE users bulk-release any org custody and bulk-assign to any custodian. Root cause: the web `bulk-assign-custody` route enforced "self-service can only assign to themselves" via an *inline* guard, and the web `bulk-release-custody` passed `role` to a service-internal guard. When main's mobile merge introduced the parallel mobile routes (`mobile+/bulk-assign-custody.ts`, `mobile+/bulk-release-custody.ts`), the inline web pattern wasn't ported and `role` wasn't forwarded — so neither guard fired. Fix — centralise the logic in the service so both web + mobile callers are protected by simply passing `role`: - `bulkCheckOutAssets` (was: assign): now accepts `role`. After resolving `custodianTeamMember`, throws 403 if `role === SELF_SERVICE && custodianTeamMember.user.id !== userId`. Mirrors the existing `bulkCheckInAssets` (release) self-service guard. - `assets.bulk-assign-custody.ts`: passes `role`, drops the redundant inline guard. Drops the `userId` field from the `getTeamMember` select since the lookup is now purely an org-scope existence check (the userId comparison happens inside the service). - `mobile+/bulk-assign-custody.ts`: passes `role` (was already resolved via `getMobileUserContext` but never plumbed through). - `mobile+/bulk-release-custody.ts`: passes `role` (same fix shape, calls `bulkCheckInAssets`). Tests: - New service-level suite "bulkCheckOutAssets — SELF_SERVICE guard" covers reject (cross-user), allow (self), bypass when ADMIN, and bypass when role omitted (back-compat for any unmigrated caller). - Web `bulk-assign-custody` route tests refactored from the old pattern (assert 403 on inline guard) to the new pattern (assert `role` is forwarded). Behaviour assertions moved to the service layer where the actual logic now lives. - Mobile `bulk-assign-custody` + `bulk-release-custody` route tests gain a "forwards SELF_SERVICE role through" assertion for hex-security regression coverage; existing happy-path tests updated to expect `role: "ADMIN"` in the call shape. Also folded in: TESTING-KIT-CUSTODY-CORRECTNESS.md final-checks ticked off (manual testing of the kit-custody PR is complete). pnpm webapp:validate green: 164 files / 2103 tests (+5 over the pre-fix baseline of 2098).
Documents the session work from 2026-05-07: - Marks Phase 3d-Polish-2 as shipped (commit 4f0d9d6) — was previously flagged "NOT YET COMMITTED" in the doc. - Adds Phase 3d-Polish-3 section covering today's six commits: - 4c34006 — IDOR fix on phase-3 booking endpoints (hex r3199039007 + r3199039448) - 197b51c — main merge bringing in mobile companion app + reports review fixes - d66b6cd — small follow-up merge for ddb104b - 7226f8a — kit detail sub-page title + qty-display polish - 116e4c6 — checkOutQuantity status-sync, deleteKit / bulkDeleteKits app-logic restoration via the new performKitDeletion helper, location scan drawer qty suffix - d5b2808 — centralised SELF_SERVICE bulk-custody guards in bulkCheckOutAssets / bulkCheckInAssets (hex r3202161632 + r3202162994) - Updates the "Last sync with main" table with both 2026-05-07 merges, plus a notes block on the conflicted-file resolutions and the highest-risk merged region (overdue-items KPI math). - Bumps the testing-status baseline from 1930 (Polish-2) to 2103 tests, with a breakdown of where the +173 came from. - Updates Known Issues to note the hex findings + Polish-2 manual-testing bugs are all closed. - Marks the kit-custody manual checklist as complete (4d, 6b, 11a all clarified to "covered by unit test or fence guards, not manually testable").
Two bugs in the reporting-demo seeder that made several reports meaningless on freshly-seeded data: 1. `completedAt = to` exactly for every COMPLETE / ARCHIVED outcome. The seed wrote the `BOOKING_STATUS_CHANGED → COMPLETE` activity event at `occurredAt = booking.to`, so the compliance helper — which compares check-in to `to + 15 min` grace — classified all 1184 bookings on-time. Booking Compliance always rendered 100% regardless of timeframe. Fix: `checkInTimeRelativeTo(to)` produces a realistic spread: ~65% on-time (±15 min jitter around `to`), ~20% early (1-48 h before `to`), ~15% late (30 min – 5 days after `to`). Used by both COMPLETE and ARCHIVED outcomes. 2. `FINAL_STATUS["ONGOING_OVERDUE"]` was `"ONGOING"`, not `"OVERDUE"`. In production a scheduler flips overdue ONGOING bookings to OVERDUE; the seed doesn't run that scheduler. So bookings that *should* be OVERDUE shipped as ONGOING and the Overdue Items report (which filters on `status = 'OVERDUE'`) returned zero rows. Fix: map `ONGOING_OVERDUE` outcome → `OVERDUE` status directly. The transitions array still ends at `RESERVED → ONGOING` since no actual ONGOING → OVERDUE transition event is needed for the reports — they look at `Booking.status` directly. Surfaced during the post-Phase-3d-Polish-3 reports-verification exercise. Reports verification itself is being deferred to post-Phase-4 (kit + location qty changes will reshape the data flow again), but these seed fixes are independent — anyone running `pnpm webapp:seed:reporting-demo` benefits regardless. No webapp code changes; no tests touched. Re-running the seed script against a clean org now produces: - Booking Compliance: ~85% on-time, ~15% late (calibrated to the distribution above; +/- a couple percent of run-to-run variance) - Overdue Items: ~3% of total bookings present as overdue
Two related updates plus a small loose-end:
1. **Defer reports end-to-end verification to post-Phase-4.** Reports
were never walked through against live data on this branch; the
port from main was compile-clean only. Phase 4 (qty in kits +
location split/merge) will reshape the data flow again, so doing
the walkthrough now means redoing it. Documents this deferral in:
- `CLAUDE-CONTEXT.md` — replaces the old "Reports port" follow-up
with a clearer DEFERRED note + links to the scaffold.
- `docs/proposals/quantitative-assets.md` — adds a Phase 4 bullet
under "Kit, Location, and Auxiliary Features" so it's tracked
alongside the rebalance + custody-dialog-note items that also
gate on the same Phase 4 schema work.
2. **Add `TESTING-REPORTS.md`** — full 13-section manual checklist
for the eventual walkthrough. Pre-marked risk indicators
(🔴 Overdue Items, 🟡 Booking Compliance / Asset Utilization /
Top Booked) flag the merge-resolution-sensitive areas to scrutinise
first. Sections cover all 10 reports + CSV export + cross-org
leak guard + final gates. Ready to run when Phase 4 schema lands.
3. **Tick `TESTING-KIT-CUSTODY-CORRECTNESS.md` section 12** — the
UI smoke for the advanced asset index column changes was the only
remaining unchecked block from yesterday's manual walkthrough.
Sequencing decision (2026-05-08): Phase 4 reshapes kit + location qty flows (split/merge per PRD Open Question #6, kit-aware utilisation, group-by-model design). Doing Sub-phase 3e (calendar polish + multi-booking edge cases) and the Sub-phase 3d follow-ups (bulk-create per AssetModel, AssetModel CSV import round-trip, group-by-model index view) now means redoing them after Phase 4 touches the same surface. Updates: - `CLAUDE-CONTEXT.md` - Sub-phase 3d follow-ups: marked DEFERRED with reasoning. - Sub-phase 3e: marked DEFERRED with reasoning. - `docs/proposals/quantitative-assets.md` - Phase 3 deliverables list: struck through "Calendar view updates" with a deferral pointer; added a callout box noting both 3e + 3d follow-ups are deferred and detailed bullets live in the context file. - Phase 4 section: added a "post-Phase-4 cleanup backlog" callout enumerating what gets picked back up once Phase 4 schema is stable — 3e + 3d follow-ups + reports verification (already flagged separately as gated on Phase 4).
Phase 4 design decision (2026-05-11): reject the original "split a quantity-tracked Asset into two rows when it lives in two locations" approach. Adopt 1:N pivot tables for both Asset → Location and Asset → Kit, mirroring the Phase 2 Custody (1:1 → 1:N) and Phase 3a BookingAsset patterns. Why pivot over split: - One Asset row = one logical thing across the whole system. Reports, search, history, custom fields, model membership, valuation all operate on a single record. Splitting fragments identity and forces every cross-cutting concern to reassemble. - Symmetric with what we already shipped (Phase 2 multi-custody, Phase 3a explicit BookingAsset). Same migration shape, same enforcement primitives (composite unique + DB trigger for the INDIVIDUAL-single-row rule). - `AssetModel` returns to being a true template instead of a workaround for grouping split children. The inventory equation (corrected from earlier sketch): `Asset.quantity` stays canonical (total stock the org owns). Placement claims are orthogonal axes describing different facets of the same units — they don't subtract from each other: - AssetLocation: where physically? sum ≤ Asset.quantity - AssetKit: in which kit grouping? sum ≤ Asset.quantity - Custody: who is responsible? sum ≤ Asset.quantity (Phase 2) - BookingAsset (ongoing): reserved? feeds availability formula Critical correction: Custody and Location coexist on the same units. Johnny holding 30 of 100 pens at Office 1 Floor 2 means location quantity stays 100 AND custody quantity is 30 — not 100 and 70. This matches today's INDIVIDUAL behaviour (taking custody doesn't move the asset's location). Restock stays asset-level (bumps Asset.quantity); new units land in the unplaced pool and the user can optionally place them afterward. Updates: - `docs/proposals/quantitative-assets.md`: - Open Question #6 row marked ✅ Resolved with the pivot rationale. - Design Principle #3 rewritten from "split into separate records" to "One Asset, many placements (pivot model)" with the orthogonal-axes explanation. - Phase 4 section replaced with full schema (AssetLocation + AssetKit Prisma models, DB triggers, the inventory equation, deliverables across schema / service / loader / mobile API / user-facing split-merge UX). - `CLAUDE-CONTEXT.md`: - Phase 4 section expanded with the design-decision block, why pivot over split, the inventory equation, the corrected coexistence rule, the single-production-release shipping plan with dev-order sub-phases, and the carried-over auxiliary items. Shipping plan: single production release. Intermediate states where one relation is pivoted and the other isn't would break the inventory equation and the kit-custody Option B math, so schema migration + service / loader / route / UI all move together. Internal dev order laid out in the context file. Next step: detailed migration plan (file-by-file rewrite catalog, migration SQL, trigger specs, risk/rollback). Not part of this commit.
Earlier draft of Phase 4 proposed a single all-of-Phase-4 production
release on the reasoning that "intermediate states would break the
inventory equation." Retracted: the placement axes are independent
— each axis (Location, Kit, Custody, Booking) enforces its own
`sum ≤ Asset.quantity` invariant without referencing the others, so
shipping with Kit pivoted and Location still on the FK column (or
vice versa) is correctness-safe. Sequential shipping wins for
review scope + smaller pattern-validation surface.
Sub-phase ordering:
- **Phase 4a — Kit pivot first.** Smaller surface area (most assets
aren't in a kit); Phase 3d-Polish-2 + 3d-Polish-3 context is fresh;
the Option B math simplifies naturally to a `AssetKit.quantity`
read; lets us validate the pivot pattern on a smaller blast
radius before tackling Location.
- **Phase 4b — Location pivot.** Structurally identical to 4a; larger
surface (every asset has a location); mobile API contract change
lands here.
- **Phase 4c — Split / merge UX.** "Move N units from X to Y" flows
for both pivots; pure user-facing feature work on top of 4a + 4b.
- **Phase 4d — Auxiliary items.** Model grouping tool, group-by-model
view, import/export with qty columns, bulk-op type awareness,
rebalance kit allocation + QuantityCustodyDialog copy update.
Some items may slip into Phase 5 if scope grows.
Post-Phase-4 backlog (sub-phase 3e calendar polish, sub-phase 3d
follow-ups, reports verification) waits until all of 4a-4c are
stable, since reports verification specifically cares about both
pivots being in place.
Updates:
- `docs/proposals/quantitative-assets.md`:
- Phase 4 prerequisites + shipping plan rewritten with the
sequential phasing rationale + a sub-phase table.
- Schema heading reworded from "single migration" to "split across
4a + 4b".
- `CLAUDE-CONTEXT.md`:
- Shipping plan section rewritten with the four sub-phases as
distinct ships, each with its own scope + rationale.
- Phase 4a section calls out the Option B simplification as the
specific natural win.
Next step: detailed Plan for Phase 4a — Kit pivot only.
Structural-only rewrite of kit membership. The 1:1 `Asset.kitId` FK is replaced by an explicit `AssetKit(assetId, kitId, organizationId, quantity)` pivot. `@@unique([assetId])` enforces the same "at most one kit per asset" invariant the FK provided. `quantity` defaults to 1 and stays at 1 for every row in this phase — the quantity-aware enhancements (multi-kit allocation, type-aware single-row trigger, sum-within-total CONSTRAINT TRIGGER) are deferred to a follow-up phase so the pivot itself can be reviewed in isolation. Migration 20260511120000_add_asset_kit_pivot is a single transaction: CREATE TABLE + indexes + FKs (all ON DELETE CASCADE) + backfill one row per Asset.kitId IS NOT NULL + DROP COLUMN Asset.kitId + ENABLE ROW LEVEL SECURITY. No half-pivoted observable state. Net behaviour for end users: zero change. The mobile API keeps the singular `kit` / `kitId` JSON shape by synthesising a primary kit from the pivot's first row in `api+/mobile+/bookings.$bookingId.ts`. Kit-delete cascade semantics changed (SET NULL → CASCADE). End state is equivalent ("asset no longer in a kit"); flagged in the PR. Refactor patterns A–E applied across services, routes, components, scanner drawers, tests. `getPrimaryKit<TKit>` helper centralises the pivot read with the MergeInclude cast workaround. `getKit<const T>` and `satisfies Prisma.KitInclude` annotations preserve literal types through to consumers — most `as unknown as { ... }` casts gone. Two real runtime bugs surfaced and fixed during cleanup: the `assets.$assetId.tsx` loader still passed `kit: true` (would have thrown "Unknown field" on every asset page) and the `kits.$kitId.tsx` loader still passed `assets: { ... }` (would have thrown on every kit page). Comment hygiene pass: 170 narration comments removed; a small number of substantive WHY comments retained without task-reference language. Validation: pnpm webapp:validate green. 2103/2103 tests pass. RELEASE-CHECKLIST.md drafted as a reusable runbook for this and future structural releases.
Each release now owns its own TESTING-PHASE-<phase>-<slug>.md (matching the pre-existing pattern for Phase 3B/3C/3D, kit-custody, etc). RELEASE-CHECKLIST.md keeps the reusable phased deploy runbook (T-24h → T+24h, decision gates, rollback, sign-off) and points at the current release's testing file by name; the verification SQL + UI walkthroughs live in the per-phase file. This restores the prior per-phase pattern that was inadvertently collapsed into the release runbook in an earlier consolidation pass.
Phase 4a follow-up fixes surfaced during manual §0–§3 testing on the
worktree dev DB.
- asset/query.server.ts: project `k.id AS "assetKitId"` instead of
`ak."kitId"` so the alias is covered by `GROUP BY k.id` and the
advanced asset index loader stops failing with PG 42803 ("must
appear in the GROUP BY clause"). Semantically identical because
`ak."kitId" = k.id` via the AssetKit→Kit join.
- routes/_layout+/kits.\$kitId.assets.manage-assets.tsx: memoise the
flattened `kitAssetsList` and `kitAssetIds` arrays so the
`setSelectedBulkItems` / `setDisabledBulkItems` effects stop
re-firing on every render. Pre-pivot the loader returned
`kit.assets` directly (stable identity); after the pivot we have to
`.map` over `assetKits`, which produced a fresh array each render →
infinite setState loop.
- modules/kit/service.server.ts (updateKitAssets):
- Cross-kit move bug: pre-pivot `Asset.kitId` was a single-column
update, so moving an asset from Kit A to Kit B was free. Under
`@@unique([assetId])` the createMany hits P2002. Now delete any
existing pivot row for the moved assets first, inside the same
`$transaction` as the new createMany.
- Removed-asset `ASSET_KIT_CHANGED` events now set `kitId: kit.id`
so report queries keyed on the kit ID see both the add and the
removal (was flagged on PR #2495 as a minor follow-up; picked up
now while in the area).
- TESTING-PHASE-4A-KIT-PIVOT.md: record §0 schema/backfill checks
verified on the dev DB, §2 add-to-kit + cross-kit-move results,
and §3 removal results with the actual asset and event rows.
Phase 4a polish surfaced across §2–§9 of the manual testing pass. A single class of bug — guards treating any non-AVAILABLE row-level status as a blocker — was duplicated across the kit-custody and booking-checkout paths. For QUANTITY_TRACKED assets, row-level `Asset.status` flips to IN_CUSTODY the moment *any* unit is operator-allocated; the remaining pool is still bookable and kit-assignable. Option B math (`buildKitCustodyInheritData`) already handles partial allocation on assign, and the booking system's own availability formula validates pools at checkout. The guards just need to skip qty-tracked. Six guards made qty-aware: - `components/kits/actions-dropdown.tsx` — Assign-Custody button on the kit page no longer disabled by partial qty-tracked custody. - `routes/_layout+/kits.$kitId.assets.assign-custody.tsx` — loader guard mirrors the UI button; needed because the route is also reachable directly. - `modules/kit/service.server.ts` `bulkAssignKitCustody` — service guard rejecting "unavailable" assets in batched kit-custody assign. - `modules/kit/service.server.ts` `updateKitAssets` — "asset already in custody can't be added to a kit" guard now skips qty-tracked. - Kit picker in `routes/_layout+/kits.$kitId.assets.manage-assets.tsx` — three client-side guards (`setDisabledBulkItems`, `<List navigate>` handler, and `RowComponent`'s `allowCursor` / status badge) made qty-aware. The Available badge is forced for qty-tracked rows so the picker doesn't render no-badge-at-all. - `components/booking/availability-label.tsx` `getKitAvailabilityStatus` — kit-in-custody flag no longer fires when only qty-tracked custody exists. - `routes/_layout+/bookings.$bookingId.overview.manage-kits.tsx` — loader's server-side filter (`assetKits.every.asset.custody.none`) rewritten to allow qty-tracked custody. KitForBooking type + loader select extended with `asset.type`. - `modules/booking/service.server.ts` — both `checkoutBooking` paths and the `getBookingFlags` UI flag skip qty-tracked in the IN_CUSTODY guard. Cross-kit move bug (separate, also folded in): pre-pivot `Asset.kitId` was a single-column FK so moving an asset between kits was free. With the `AssetKit` pivot + `@@unique([assetId])`, `updateKitAssets` now `deleteMany`s any existing pivot row for the moved assets first, then `createMany`s for the destination kit — both inside the same `$transaction`. React-doctor diff-vs-main cleanup (33 → 26 warnings, score 93 → 95): - New shared `useAutoFocus` hook adoption across 5 dialog inputs (`adjust-booking-asset-quantity-dialog`, `quantity-custody-list`, `quick-adjust-dialog`, `asset-model/form` ×2). Hook handles the rAF-defer needed for Radix portal mounts. Adds a `.claude/rules/use-auto-focus-hook.md` so future contributors reach for the hook instead of hand-rolling `useRef + useEffect`. - `availability-label.tsx` — `.map().filter(Boolean).flat()` → `.flatMap(... ?? [])`. - `fulfil-reservations-drawer.tsx:817` — array-index-as-key fix using the stable `assetId`. - `kits.$kitId.assets.manage-assets.tsx` — replaced a `setSelectedBulkItems` in `useEffect` (flagged as derived-state- in-effect error) with the render-time `useRef` init pattern already used by the sibling booking picker. Docs: - `docs/proposals/quantitative-assets.md` — adds **Phase 4e — Quantity-aware notes + activity-feed audit** as a new sub-phase for the cross-cutting "notes show units" sweep flagged during §5 testing. Phase 4 shipping plan now lists 4a–4e. - `CLAUDE-CONTEXT.md` — mirrors 4e + adds a Phase 4d follow-up noting that kit-included qty-tracked `BookingAsset.quantity` defaults to 1 today and needs to read `AssetKit.quantity` once multi-kit allocation ships post-4a. - `TESTING-PHASE-4A-KIT-PIVOT.md` — checked off §0 / §2 / §5a / §5c / §7 / §9 with the actual recorded results from the dev DB + mobile API curl. Added "qty-tracked partial-custody is selectable" edge case under §2 and the kit-availability edge under §7.
Turn the structural-only AssetKit pivot into a real allocation pivot. QUANTITY_TRACKED assets can now sit in multiple kits at distinct slices; INDIVIDUAL stays capped at one kit per asset via a DB trigger. Schema - Drop @@unique([assetId]) on AssetKit (replaced by type-aware triggers). - enforce_individual_asset_single_kit BEFORE trigger guards INDIVIDUAL. - enforce_asset_kit_sum_within_total CONSTRAINT trigger, DEFERRABLE INITIALLY DEFERRED so the picker can swap slices mid-tx. - Backfill QUANTITY_TRACKED pivot rows: quantity = Asset.quantity. Service (kit/service.server.ts) - buildKitCustodyInheritData reads AssetKit.quantity as source of truth, with a safety ceiling against pre-existing operator custody. - updateKitAssets accepts per-row assetQuantities, threads them into the createMany / per-row update path. - New in-custody qty-edit cascade: AssetKit.quantity changes ripple to the kit-allocated Custody.quantity in the same tx, with paired CUSTODY_ASSIGNED / CUSTODY_RELEASED events (meta.viaKit + delta). - Server-side strict-available re-check returns a clean 400 instead of letting the DEFERRED constraint surface as a generic 500. Picker UI (kits.$kitId.assets.manage-assets) - Per-row qty input on qty-tracked rows, bounded by the strict-available pool: max(currentInThisKit, asset.quantity - other kits - operator-only custody - ongoing bookings). - "Also in Kit X (N)" subtitle for multi-kit assets. - "N available" hint when the pool is below the asset's total. - "was N" delta indicator when the user edits an existing slice. - In-custody warning box explains the operator allocation cascade. - Loader factored into a reusable getKitPickerMeta() helper; named KitParamsSchema / AssetQuantitiesSchema / ManageAssetsActionSchema replace the inline Zod shape. Asset overview page - "Included in kit" -> "Included in kits": lists every kit membership with its per-kit slice on qty-tracked assets. - Quantity Overview gains a new "In kits" row when > 0; "Available" now subtracts kit slices and operator-only custody so it reflects the truly free pool. - "In custody" now means operator-only (filters kitCustodyId IS NULL) so in-custody kits don't double-count. Kit detail page - Asset list reads inKit from AssetKit.quantity directly; "N units in kit" replaces "N / M units in kit" so users see this kit's slice instead of a misleading kit-vs-total ratio. Tests - 6 new contract tests on updateKitAssets covering the picker + the server-side strict-available re-check. - Update existing assign-custody fixtures to supply the assetKits relation now read by the refactored helper. Docs - TESTING-PHASE-4A-POLISH-2.md: 15-section manual verification plan. - CLAUDE-CONTEXT.md + TESTING-PHASE-4A-KIT-PIVOT.md status updates.
…t notes - TESTING-PHASE-4A-POLISH-2.md: tick §3-§13 as verified (picker happy path, multi-kit, in-custody cascade incl. assign/release/deselect, strict-available cap, INDIVIDUAL single-kit) — all DB-verified via supabase-local MCP during the walkthrough. - CLAUDE-CONTEXT.md: mark the Phase 4e kit-membership-notes bullet UNBLOCKED — multi-kit allocation shipped in bebaf4e so AssetKit.quantity is now the meaningful source; sweep createBulkKitChangeNotes + cross-kit-move / removal note writers.
Replace the Asset.locationId 1:1 FK with an AssetLocation pivot, with the qty-allocation triggers shipping from day one (no structural-only intermediate). QUANTITY_TRACKED assets can now sit at multiple locations at distinct slices; INDIVIDUAL stays capped at one location per asset via a DB trigger. Folds in the Phase 4a-Polish-2 kit fan-out fix in the asset-index raw SQL. Schema + migration (20260519143054_add_asset_location_pivot) - CREATE TABLE AssetLocation (assetId, locationId, organizationId, quantity), composite @@unique([assetId, locationId]) only (no intermediate single-row unique like 4a had). - Backfill one row per Asset.locationId IS NOT NULL: quantity = Asset.quantity for QUANTITY_TRACKED, 1 for INDIVIDUAL. - enforce_individual_asset_single_location BEFORE trigger guards INDIVIDUAL. - enforce_asset_location_sum_within_total CONSTRAINT trigger, DEFERRABLE INITIALLY DEFERRED so per-asset move-N tx is valid mid-flight. - Drop Asset.locationId column + FK + index, ENABLE RLS. Raw SQL (asset/query.server.ts) - addLocationFilter rewritten to EXISTS / NOT EXISTS against the pivot (mirrors the proven addKitFilter pattern). - assetQueryJoins: replaced both `LEFT JOIN AssetKit ak / Kit k` and `LEFT JOIN Location l` with LATERAL primary-pick (oldest pivot row). Yields exactly one row per asset under the existing GROUP BY. The kit half fixes a latent Phase 4a-Polish-2 fan-out regression that duplicated multi-kit qty-tracked assets in the asset index — the same defect class flagged in the 4b plan's "key divergences". - GROUP BY at service.server.ts:996 gains k.status (now from a LATERAL derived table, no PK functional-dependency shortcut). - assetReturnFragment: removed redundant 'locationId' projection — the same id was already inside 'location'. Helpers (asset/utils.ts) - getPrimaryLocation: new helper, mirrors getPrimaryKit. - Both helpers retyped to take a typed asset argument (no more `unknown` cast). TLoc / TKit are inferred from the asset's projected shape, so call sites are bare `getPrimaryLocation(asset)` — no explicit generic needed. JSDoc documents the `as unknown as` escape hatch for the one legitimate case where a service widens its include arg. Service rewrite (~37 files) - asset/service.server.ts: createAsset/updateAsset/bulkUpdateLocation rewritten to do pivot delete-then-create in a tx. Type-aware quantity (Asset.quantity for QUANTITY_TRACKED, 1 for INDIVIDUAL). ASSET_LOCATION_CHANGED events + notes preserved per-asset. Where-builder maps location/locationId filters onto assetLocations.{some,none}. - kit/service.server.ts: updateKitLocations + bulkUpdateKitLocations + the newly-added-asset cascade all rewritten as per-asset pivot replace with type-aware quantity, inside the existing tx. Kit.locationId itself stays a FK (only the asset side pivots). - location/service.server.ts: getLocation now returns { location, totalAssetsWithinLocation, assets } (synthesized from a separate pivot-filtered findMany since Location.assets is gone). updateLocationAssets rewritten as assetLocation.createMany / deleteMany inside the existing tx. Note builders read primary via getPrimaryLocation. IDOR guards preserved. - 8 mobile endpoints: pivot-shaped reads, response shape preserved by synthesizing singular `location` via getPrimaryLocation (mirrors the 4a kit synthesis in api+/mobile+/bookings.$bookingId.ts). - ~26 leaf routes / components / scanner drawers / reports / audit / pdf helpers / seed / test factory: read paths swapped to getPrimaryLocation; include shapes swapped to assetLocations. Loader-shape bugs caught by the inference refactor (7, all fixed) - kit/types.ts KIT_SELECT_FIELDS_FOR_LIST_ITEMS: stale location include on Asset → kit-page rows would render no location. - utils/scanner-includes.server.ts ASSET_INCLUDE: stale location include → scanner overlay would show no location. - asset/service.server.ts duplicateAsset arg type missing assetLocations → duplicates would lose their location. - assets.$assetId.overview.update-location.tsx getAsset had no include → pre-fill always null. - assets.$assetId_.edit.tsx getAsset had no include → pre-fill always null. - locations.$locationId.assets.manage-assets.tsx RowComponent hardcoded the gone Asset.location relation → badge never rendered. - api+/command-palette.search.ts getAssets call didn't pull assetLocations → results showed no location. Tests - 14 existing tests updated to the pivot fixture shape (kit/asset/ location service, query.server, fields, mobile route). - 2177 / 2177 pass. Docs + dedup - TESTING-PHASE-4B.md: 15-section manual verification plan. - CLAUDE-CONTEXT.md: Phase 4b status, migration #13, testing baseline, Phase 4e Location-notes bullet flipped to UNBLOCKED. - AdvancedIndexAsset: removed the duplicate `locationId: string | null` scalar field (and the matching SQL projection + the consumer's redundant ?? fallback). Single canonical path: item.location.id. - Removed 3 `primaryLocOf` local wrappers + their per-function type aliases in kit/asset service; ~26 call sites + ~30 inline explicit generics on getPrimaryLocation collapsed to the bare form now that inference works. - ~80 phase-prefix narration comments cleaned up — 22 deleted, ~58 rewritten to describe permanent design intent. Cascade-semantics flag for PR description Old Asset.locationId was nullable, SET NULL on Location delete. New AssetLocation.locationId is ON DELETE CASCADE — deleting a location removes the pivot rows (assets stay, become "unplaced"). Observably equivalent end-state. Out of scope (carried forward) - Quantity-aware location-change notes → Phase 4e (now unblocked). - Split/merge "move N units A→B" UX → Phase 4c. - Reports location-filter re-verification → still deferred. - Kit.locationId stays a FK. - Multi-placement mobile UI → post-4c.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Quantitative Assets
How to Read This Document
This proposal is split into two parts:
Table of Contents
Part 1 — Product Proposal
Part 2 — Technical Design
Part 1 — Product Proposal
Problem Statement
Shelf currently treats every asset as a single, individually-tracked item. Each asset has its own status, custody record, QR code, and booking history. This works well for laptops, cameras, vehicles, and other uniquely-identifiable equipment.
However, many organizations also manage items that don't fit this model:
Quantity-tracked assets (fungible items managed by count)
Items where individual identity doesn't matter — only the count. Examples:
Creating a separate asset record for each pen in a box of 500 is impractical. Users need a single record that says "500 pens in Room A" and tracks usage over time.
Asset Models (template-grouped individual assets)
Items that are individually tracked but share a common model or template. Examples:
Today, users create these one by one with no formal grouping. They can't say "book any available Dell Latitude" or see "8 of 30 Latitude 5550s are currently available." Categories provide loose grouping, but aren't specific enough — a "Laptop" category contains many different models.
Why this matters
Without quantity support:
Core Concepts
Before diving into user stories and features, here are the foundational concepts that shape this design.
Tracking Method
Every asset in Shelf has a tracking method chosen at creation time. This is a permanent choice that determines how the asset behaves throughout the system.
Identity boundary rule
This is a hard guardrail. An asset's tracking method cannot be changed after creation because the data models diverge (individual assets have per-unit history; quantity assets have aggregate logs). Choosing the wrong method means re-creating the asset.
Behavior modes (quantity-tracked assets only)
Quantity-tracked assets have a behavior mode that controls what happens after checkout:
Two user intents for quantity assets
Quantity-tracked assets support two distinct workflows:
Both create audit log entries with full attribution.
User Stories
Quantity-tracked assets
Asset Models
Cross-cutting
Competitor Analysis
Feature Comparison
Key Takeaways
Cheqroom keeps things simple — a "Bulk Item" toggle converts an asset into a quantity-tracked record. Location-bound (splitting across locations creates separate records). No custody for bulk items. Kits can mix individual and bulk. Limitations: no generic booking, no low-stock alerts, no return tracking.
Sortly takes the most unified approach — every item has a quantity field (defaulting to 1 for individual assets). Variants act as lightweight templates. Folder hierarchy doubles as location structure. Min levels with alerts are built-in. Custom units of measure. Limitations: no formal booking system, no generic booking, variant system is limited.
Snipe-IT has the most mature model with 5 entity types, but it's also the most rigid. Asset Models serve as templates with custom field sets. Consumables are strictly one-way (no return/two-way tracking). Two-tier alert system is powerful. Limitations: no kit concept, no generic booking, consumables live in a completely separate area (different UI, different workflows).
Our approach borrows the best ideas:
Feature Overview
1. Asset Creation
When creating a new asset, users choose a tracking method:
When Tracked by quantity is selected, the form shows additional fields:
Both tracking methods can optionally be assigned to an Asset Model (e.g., "Dell Latitude 5550"). When creating an asset from an existing model, fields like category and valuation are pre-filled from the model's defaults.
2. Asset Listing
For quantity-tracked assets, the asset list shows:
The asset list also gains a "Group by Model" view option:
New filters: type (Individual / Quantity-tracked), model, and "low stock only."
3. Custody with Quantities
Individual assets — unchanged. One custodian per asset.
Quantity-tracked assets — custody becomes quantity-aware:
4. Booking with Quantities
Availability formula
The system calculates available stock using:
When a user reserves N units in a booking, the available count drops by N immediately. This ensures two bookings can't double-reserve the same stock.
Booking quantity-tracked assets
Users can reserve a specific quantity:
Book-by-Model (generic booking)
Users can book from a model without choosing specific units upfront:
5. Kit Integration
Kits can include quantity-tracked items alongside individual assets:
6. Location Handling
Quantity-tracked assets are tied to a single location per record. To track the same item across multiple locations, the user splits the record:
Both records can share the same Asset Model for aggregate reporting. A merge operation does the reverse — combines two same-model records into one.
7. Consumption Tracking
Each quantity-tracked asset can be configured for one of two behavior modes:
One-way consumption — Items are used up and not expected back.
Two-way consumption — Items are checked out and returned, with a consumption report.
Restocking — For both modes, admins can manually increase quantity when new stock arrives.
All quantity changes are logged for audit purposes.
8. Low Stock Alerts
Quantity-tracked assets with a minimum quantity threshold trigger alerts when available stock drops to or below that threshold:
Alerts clear automatically when stock rises above the threshold (via restock or returns).
9. QR Codes
Quick adjust from QR scan
When scanning a quantity-tracked asset's QR code, the primary action is a quick adjust: the user can immediately increase or decrease quantity with a note (e.g., "+50 — new shipment arrived" or "−12 — handed out at event"). This is the stock management intent described in Core Concepts.
Paths to the full booking/custody flows are visible but secondary — most QR scans on quantity assets are for quick stock checks or adjustments, not for starting a formal booking.
Label expectations guardrail
Transition for Existing Users
This is gradual and opt-in. Organizations don't need to group everything at once.
Decisions
These items were discussed during the team call and in Carlos's PR review. They are now resolved.
ConsumptionLogmodel in Part 2.Remaining Open Questions
New questions that emerged from the review and team discussion.
Part 2 — Technical Design
Design Principles
AssetTypeenum to the existingAssetmodel rather than creating separate entity types. This preserves existing queries, filters, bulk operations, and UI patterns.Asset → Locationrelationship singular.Proposed Data Model
New Enums
Modified Model: Asset
Notes:
typedefaults toINDIVIDUAL, so existing assets require no data migration.availableQuantityis maintained by the system:quantity - (in_custody + booked).unitOfMeasureis a freeform string to support any unit without a fixed enum.New Model: AssetModel
Notes:
categoryId.Modified Model: Custody (quantity-aware)
Key change: The current
assetId @uniqueconstraint enforces one custodian per asset. For quantity-tracked assets, we need to allow multiple custodians to hold different quantities. The new constraint is@@unique([assetId, teamMemberId])— one record per asset+custodian pair.For
INDIVIDUALassets, application logic continues to enforce single-custodian behavior (only one Custody row allowed). ForQUANTITY_TRACKEDassets, multiple Custody rows are permitted, each with a quantity.New Model: BookingAsset (explicit pivot)
Currently, bookings and assets use a Prisma implicit many-to-many join. We need an explicit join table to support quantities:
Notes:
INDIVIDUALassets,quantityis always 1.QUANTITY_TRACKEDassets,quantityis the number reserved/checked-out.assetIdinitially points to a placeholder or the model's representative asset, andassignedAssetIdis populated at checkout when specific units are scanned.New Model: ConsumptionLog
Purpose: Full-attribution audit trail for all quantity changes. Every event records who performed it, who received/returned items (custodian), which booking it relates to, and what kind of event it was. This satisfies Decision #6 (full per-custodian, per-booking, per-location attribution).
Entity Relationship Overview
Migration Strategy
Existing Assets
All existing assets default to
type: INDIVIDUAL. No data changes required. The migration adds the new columns with default/null values:Custody Table Changes
The
Custodytable migration removes the@uniqueconstraint onassetIdand adds:quantitycolumn with default value of 1@@unique([assetId, teamMemberId])Existing custody records remain valid (they have
quantity: 1and the new unique constraint holds since each asset currently has at most one custodian).Booking Pivot Migration
The implicit
_AssetToBookingjoin table is replaced with the explicitBookingAssetmodel. Migration must:BookingAssettable_AssetToBookingwithquantity: 1_AssetToBookingAPI Backwards Compatibility
New fields are additive and optional. Existing API consumers that don't send
type,quantity, etc. will continue to createINDIVIDUALassets with default behavior. This is a non-breaking change for external integrations.Implementation Phases
All phases ship together as one release. This ordering reflects build dependencies, not separate releases.
Phase 1: Foundation
Goal: Core data model changes and basic CRUD.
AssetTypeenum,ConsumptionTypeenum,ConsumptionCategoryenumAssetmodelAssetModelmodel (no custom field defaults — deferred per Decision Login register error handling #5)ConsumptionLogmodel (with full attribution fields)BookingAssetexplicit pivot tablePhase 2: Quantity-tracked Operations
Goal: Quantity-aware checkout, custody, and consumption.
Phase 3: Booking Integration
Goal: Quantity-aware bookings and book-by-model.
BookingAssetpivotAvailable = Total − In custody − Reserved)Phase 4: Kit, Location, and Auxiliary Features
Goal: Remaining integrations and polish.
Summary by CodeRabbit
New Features
UI
Tests
Chores