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
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 100 out of 100 changed files in this pull request and generated 11 comments.
Comments suppressed due to low confidence (1)
apps/webapp/app/modules/kit/service.server.ts:2462
- In bulk kit removal,
tx.custody.deleteMany({ where: { id: { in: custodyIdsToDelete }}})will delete every custody record collected for each asset. SincecustodyIdsToDeleteis built from(a.custody ?? []), this can wipe unrelated quantity custody allocations. Restrict the deletion to only the custody record(s) that were created due to the kit being in custody (e.g., match the kit custodian/teamMemberId), and leave other custody rows intact.
- Pass real availableToBook to StatusColumn in advanced index instead of hardcoded true - Fix low-stock tooltip text: "will be notified" instead of "has been sent" (we can't confirm delivery) - Remove `as any` cast in inline entity creation dialog for asset model — construct typed payload explicitly - Add JSDoc to tx params in consumption-log service and quantity-lock explaining why `any` is used (Prisma doesn't export a clean type for interactive transaction clients on extended PrismaClient instances)
remove as-any cast - Block quantity-tracked assets from the generic release-custody route (loader redirects, action throws) — they must use the quantity-aware release flow - Replace inline db.custody.aggregate in overview loader with shared computeAvailableQuantity() helper - Remove (data as any).assetModels cast in advanced filters — use "assetModels" in data type narrowing
pivot table (Phase 3a) Database migration: - Drop empty Phase 1 BookingAsset table shell - Rename _AssetToBooking → BookingAsset with all data - Rename columns A/B → assetId/bookingId - Add id (text PK) and quantity (default 1) columns - Recreate indexes, unique constraint, FK constraints - RLS carries over from the renamed table Schema change: - Remove implicit M2M: Booking.assets and Asset.bookings - BookingAsset explicit model is now the sole relation Code rewiring (~60 Prisma + 18 raw SQL): - All booking.assets → booking.bookingAssets with nested asset includes - All asset.bookings → asset.bookingAssets with nested booking includes - connect/disconnect → create/deleteMany on BookingAsset - _count.assets → _count.bookingAssets across all email templates, workers, dashboard components - Raw SQL: _AssetToBooking/A/B → BookingAsset/assetId/ bookingId in 18 query locations - Conflict detection updated for new relation path - Added normalizeBookingAssets() adapter helper - Updated all tests to match new data shapes Resolved open questions in PRD: - OQ #2: Show warning at checkout if availability changed - OQ #5: BookingModelRequest for book-by-model intent
Availability service: - computeBookingAvailableQuantity() computes Available = Total - InCustody - Reserved - Reserved = sum of BookingAsset.quantity for bookings with status RESERVED, ONGOING, or OVERDUE Booking quantity support: - updateBookingAssets() accepts per-asset quantities map - Raw SQL INSERT uses parallel quantity array via unnest - ON CONFLICT updates quantity instead of skipping Manage-assets UI: - Quantity picker for QUANTITY_TRACKED assets (inline number input clamped to available quantity) - Blue badge showing "Qty tracked · N available" - Fully-allocated qty assets filtered from list - Existing booked quantities pre-populated on edit - Unsaved-changes detection for quantity edits Conflict detection: - QUANTITY_TRACKED assets skip binary conflict checks - Real validation at service layer via availability Checkout validation: - Validates quantity availability at checkout time - Throws clear error listing insufficient assets Check-in: - QUANTITY_TRACKED assets skip status reset to AVAILABLE (other bookings/custody may reference same asset) Booking overview: - bookedQuantity attached to enriched items - Sidebar and list show "x N" for qty-tracked assets Email/PDF: - Email shows "x N" for quantity > 1 - PDF has new "Qty" column Other: - Added .claude/rules/ for self-improving rule files - Updated CLAUDE-CONTEXT.md with Phase 3 progress - Fixed AvailabilityLabel custody check (hasCustody) - Updated test expectations for AssetType.INDIVIDUAL filter in check-in updateMany calls
Second pass on quantity-tracked bookings — adds missing entry points, activity logging across all quantity flows, and fixes several issues surfaced during Phase 3b testing. New UX: - Adjust-quantity dialog on each booking asset row; backed by a new /api/bookings/:bookingId/adjust-asset-quantity endpoint - Quantity picker in "Add to existing booking" modal for qty-tracked assets, validated against booking-aware availability - Auto-open the adjust-quantity dialog after "Create new booking" for qty-tracked assets (via ?adjustQty=<assetId>) so users can size the reservation immediately instead of landing with quantity = 1 - Asset status badge now surfaces per-booking reservation breakdown alongside custody Activity notes: - Adjust-quantity writes matching asset-side and booking-side notes showing the "from N to M" delta - Manage-assets writes quantity-aware add/adjust notes on both sides, using the same wrapAssetsWithDataForNote helper as the remove flow - All note writes are best-effort: failures log via Logger.error server-side but don't roll back the user's mutation Fixes: - Booking duplicate now preserves per-asset BookingAsset.quantity instead of falling back to the schema default of 1 - Booking CSV export renders "Title × N" for qty-tracked assets; narrow direct-by-ID query widened to include quantity and asset.type - Asset overview separates booking-aware availability (used for the display "Available" row) from custody-available = total − inCustody (used to cap custody-assign and quick-adjust dialogs). Future reservations no longer incorrectly block current custody assignment - Fixed infinite revalidation loop in the asset-row auto-open effect — the project's custom setSearchParams is an unstable reference, so the effect needs a ref-guard to run exactly once per mount. Previous behavior hammered the root _layout loader and tripped Stripe's rate limit Refactor: - Migrate booking asset-row actions from deprecated DropdownMenu to Radix Popover, matching the design/sizing/interaction of other popover menus in the app (location/actions-dropdown pattern) - RemoveAssetFromBooking now accepts an optional trigger prop so menu consumers can style the menu item consistently - New "adjust-quantity" icon (lucide SlidersHorizontal) in icons-map Plan: - Add sub-phase 3c "Quantity-aware Check-in" to CLAUDE-CONTEXT.md (covers consumption-type branching, ConsumptionLog writes, pool decrements for ONE_WAY consume / TWO_WAY partial return). Renumber existing 3c/3d → 3d/3e. Check-in work must land before book-by-model so both paths share the same check-in plumbing.
Makes partial and quick check-in fully quantity-aware for qty-tracked
assets. Per-asset dispositions (returned / consumed / lost / damaged)
are captured in the scanner drawer, written as `ConsumptionLog` rows,
and reflected across every surface that shows booking asset state.
Schema:
- New `CONSUME` + `DAMAGE` values on the `ConsumptionCategory` enum
(enum-only migration, no data touched). CONSUME covers ONE_WAY
intended use (AA batteries, latex gloves). DAMAGE is kept distinct
from LOSS so future reporting can tell "never came back" apart from
"came back unusable"
Check-in service (`modules/booking/service.server.ts`):
- New helpers `computeBookingAssetRemaining` and `isBookingFullyCheckedIn`
so partial and quick check-in share one completion signal
- `partialCheckinBooking` accepts a `checkins: CheckinDisposition[]`
payload. Per qty-tracked asset, inside one db.$transaction:
lockAssetForQuantityUpdate → recompute remaining → reject over-return
→ pool-drain guard (projected pool >= inCustody) → write one
ConsumptionLog per non-zero category → decrement Asset.quantity for
consumed+lost+damaged (RETURN leaves pool alone)
- `checkinBooking` (quick path) auto-defaults qty-tracked dispositions
to "return all" (TWO_WAY) or "consume all" (ONE_WAY) when no
explicit checkins payload is provided — the big-button happy path
- PartialBookingCheckin.assetIds lists only session-fully-reconciled
assets; qty-tracked partials stay tracked via ConsumptionLog sums
Server-side hardenings surfaced by testing:
- `checkOutQuantity` now subtracts units checked out via active
bookings (ONGOING/OVERDUE) from the custody-available gate, not just
in-custody. Prevents over-allocating the same physical unit to a
direct custody assignment AND an active booking
- `manage-assets` guardrail rejects reducing `BookingAsset.quantity`
below the sum of already-dispositioned units for that booking
- `getBookingContextAssetStatus` no longer inherits global
`Asset.status = CHECKED_OUT` into DRAFT/RESERVED rows for qty-tracked
assets — pool-wide state doesn't reflect this booking's row
UI:
- Scanner drawer gets per-row quantity disposition block to the right
of the asset title. Primary input is Returned (TWO_WAY) or Consumed
(ONE_WAY), auto-fills to remaining. Shortfall auto-expands with
Lost/Damaged inputs and a pending indicator when primary < remaining.
New blockers: zero disposition + over-return
- Removing a scan now clears that asset's disposition state — re-scan
starts fresh instead of restoring stale edits
- New extended status `PARTIALLY_CHECKED_IN_QTY` (amber badge, label
"Partially checked in") — distinct from the individual-asset
`PARTIALLY_CHECKED_IN` ("Already checked in") because the semantics
differ ("some units are in, work remaining" vs "whole row is in")
- Booking overview + booking assets sidebar:
Qty column reads as `checked-in / booked` with a Radix tooltip
(Booked / Checked in / Remaining). Happy path shows just `N`, done
state shows `N / N` in emerald-700
- Loader enriches qty-tracked assets with `dispositionedQuantity`
(single ConsumptionLog groupBy across the page's bookings)
- New shared `ConsumptionTypeBadge` component — compact "Returnable" /
"Consumable" pill with a Radix tooltip explaining the semantics.
Rendered in booking overview, sidebar, and manage-assets
- `QuantityOverviewCard` + custody list use `custodyAvailable`
(total − inCustody − checkedOut) so custody/adjust dialog caps match
the server-side gate
Activity notes:
- Per-asset note per check-in event: "<user> via partial check-in on
<booking-link>: returned **N**, lost **K**, damaged **J**, **P**
still pending" — only non-zero fields rendered
- Booking-side system note summarizes disposition totals for the
session
- Individual-asset check-in note also links the booking
- Note writes wrapped in best-effort try/catch with Logger.error so a
markdoc failure never blocks a valid check-in
Fixes uncovered while testing:
- Dashboard "Top custodians" widget: `_count.assets` → `.bookingAssets`
(Phase 3a migration miss; was producing "NaN Assets")
- `BOOKING_WITH_ASSETS_INCLUDE` picks up `consumptionType` +
`unitOfMeasure` so downstream consumers stay typesafe
Plan doc:
- CLAUDE-CONTEXT.md Sub-phase 3c marked NOT STARTED → implemented
- Three Phase-3c follow-ups captured before Sub-phase 3d kicks off:
(1) refactor partial-checkin UX to match the audit UI, (2) ship a
consumption-log view on the asset page, (3) write the unit-test
matrix documented at the end of TESTING-PHASE-3C.md
…flows Six issues surfaced by an ultrareview pass on the Phase 3c surface area. Each is independently reachable and has its own root cause; grouping them because they share the same qty-tracked subsystem. - bug_007: `adjust-asset-quantity` read availability outside a transaction, letting two concurrent bumps on the same asset both pass the guard and oversubscribe the pool. Now wraps compute+check+update in `$transaction` with `lockAssetForQuantityUpdate` as the first call. - bug_013: `computeBookingAvailableQuantity` was double-counting already- dispositioned units (RETURN/CONSUME/LOSS/DAMAGE) as still reserved, under-reporting true availability. Now subtracts logged dispositions per booking via a single `consumptionLog.groupBy`. - bug_014: `assets.adjust-quantity` accepted any category+direction combo, so a RESTOCK could subtract and a LOSS could add. Added a `.refine` enforcing RESTOCK=>add, LOSS=>subtract; ADJUSTMENT stays either. - bug_015: `manage-assets` did a blind `JSON.parse(...) as Record<string, number>` on the `quantities` field, trusting client input. Now Zod- validates to positive ints with an upper bound, surfacing structured 400s instead of downstream crashes. - bug_017: `checkoutBooking` validated availability *before* the transaction, same TOCTOU shape as bug_007 but across every qty-tracked asset on the booking. Moved the per-asset lock+compute+guard inside the tx callback; kept the original user-facing error messages. - merged_bug_001: `adjustQuantity` in `consumption-log/service.server` locked the asset but never checked it belonged to the caller's organization, enabling cross-tenant IDOR. Added an org-scoped check immediately after the lock, returning 403 on mismatch. All call sites unchanged; return shapes preserved. Typecheck + lint clean.
…coverage
Phase 3c shipped with broken test scaffolding on two fronts:
1. `.test.server.ts` route tests were silently excluded by Vitest's
default include pattern since the Phase 3a rename (the `.server`
suffix was added to avoid React Router typegen collisions). Added
an explicit pattern to `vitest.config.ts` so they're discoverable
again.
2. Once discovered, the route tests revealed pre-existing breakage
from Phase 3a/3c:
- `mockBooking.assets` → `mockBooking.bookingAssets` (the implicit
M2M was replaced by an explicit pivot model in Phase 3a; the
mocks never got updated).
- `isAssetPartiallyCheckedIn` / `isKitPartiallyCheckedIn` now take a
4th `bookingStatus` arg so COMPLETE/ARCHIVED can be distinguished
from active bookings.
- `updateBookingAssets` now accepts `quantities` + `userId`, and
`removeAssets` now accepts `assets` (the loaded asset rows for
note rendering).
- `manage-assets` writes system booking notes via
`createSystemBookingNote`, which hits `db.bookingNote` — the
route test had to mock the wrapper to avoid touching the real
client.
- `booking/service.server.test.ts`: the `partialCheckinBooking`
happy-path test was hitting the new `isBookingFullyCheckedIn`
short-circuit (empty `bookingAsset.findMany` → "complete" branch
→ `tx.booking.update` default `{}` → `wrapLinkForNote` throws).
Fixed by seeding the mock so the test stays in the partial
branch it's actually asserting.
Then added Phase 3c unit coverage per the planned test matrix:
- `booking/service.server.test.ts` (+18 tests):
- `computeBookingAssetRemaining` (4)
- `isBookingFullyCheckedIn` (4)
- `partialCheckinBooking — qty-tracked dispositions` (7)
- `checkinBooking — qty-tracked auto-default` (3)
- `asset/service.server.test.ts` (+3 tests):
- `checkOutQuantity — availability accounting`: regression guard
for the `total − inCustody − checkedOutViaBooking` fix, plus a
within-bounds case and a RESERVED-ignored case.
- `utils/booking-assets.test.ts` (NEW, +5 tests):
- `getBookingContextAssetStatus`: individual partial/non-partial
on ONGOING, individual on COMPLETE fallback, qty-tracked DRAFT
override, and the ONGOING boundary that proves the override
does not fire outside DRAFT/RESERVED.
Mocks follow the existing file conventions: `// why:` comments on
every new mock, partial-module mocks for `consumption-log/service`
so real helpers stay live, and no assertions on exact activity-note
strings (only that notes were or weren't created, per the plan).
Suite: 1680 → 1715 passing, 9 → 0 failing. Full webapp:validate
(typecheck + lint + test) green.
The partial check-in drawer used to show only what the operator had
already scanned, leaving every outstanding asset invisible until they
scanned it. Qty-tracked assets (batteries, pens) don't even have
physical QRs, so scanning them was impossible — the operator had to
hold the full list of outstanding items in their head.
Ports the "expected scan list" pattern from audits: every asset on
the booking renders up-front, flipping from Pending to Checked-in
as it's scanned. Adds a "Check in without scanning" button for
qty-tracked rows so operators can enter dispositions inline without
needing a physical QR.
Drawer
- New jotai atoms in `atoms/qr-scanner.ts`: `bookingExpectedAssetsAtom`,
`bookingCheckinSessionAtom`, start/end/set actions, plus
`quickCheckinQtyAssetAtom` that inserts a synthetic
`qty-checkin:<assetId>` entry into `scannedItemsAtom` so the rest
of the drawer logic (removal, disposition seeding, blockers)
treats it identically to a real scan.
- New hook `use-booking-checkin-session-initialization.ts` seeds
the atoms on mount and clears them on unmount. Re-syncs when
loader data refreshes.
- Loader at `bookings.$bookingId.overview.checkin-assets.tsx` ships
`expectedAssets` + `expectedKits` computed from the existing
`booking.bookingAssets`, `qtyRemainingByAssetId`, and
`partialCheckinDetails` — no new DB calls.
- `partial-checkin-drawer.tsx` rewrite: bucket-sort renderer with
explicit section headers ("Checked in this session (N)" /
"Pending (N)"), per-bucket rendering, foldable kit group in
pending (chevron + kit image, collapsed by default — pending kit
assets are one summary row instead of N), positive status badges
on active rows ("Scanned" for QR scans, "Checked in without scan"
for synthetic rows), unit-weighted progress indicator
(`N/M units checked in` instead of asset-row count).
- ONE_WAY consumables can now be partly Returned to the pool — the
shortfall panel on a ONE_WAY row adds a Returned field alongside
Lost / Damaged, so "20 batteries booked, 5 consumed, 15 unused
going back to stock" is a one-click workflow. TWO_WAY shortfall
panel unchanged.
- Mobile: title + disposition block stack vertically below `sm:`
breakpoint; button drops below the row; inputs full-width. Above
`sm:` the desktop two-column layout is byte-identical.
Server
- `partialCheckinBooking` now merges `assetIds` (INDIVIDUALs) and
`checkins` (qty dispositions) instead of treating them as
mutually exclusive — fixes a bug where an INDIVIDUAL scanned
alongside a qty-tracked check-in silently dropped out.
- Booking-side activity notes name each qty-tracked asset with a
clickable link + per-category parts instead of aggregating to
"10 returned" with no asset names.
- `getBookingFlags` exempts qty-tracked assets from
`hasCheckedOutAssets` / `hasAlreadyBookedAssets` — a qty-tracked
asset with units out via another booking no longer disables
Reserve for new bookings that fit in the remaining pool.
- `bookings.$bookingId.overview.tsx` + `bookings._index.tsx` loaders
include a per-asset `dispositionBreakdown` (RETURN / CONSUME / LOSS
/ DAMAGE split). Widened `BOOKING_WITH_ASSETS_INCLUDE` to select
`mainImage`, `thumbnailImage`, and `kit.{id, image}` so the
drawer has what it needs for the expected-kits payload.
UI polish
- Qty tooltip in the asset list + booking sidebar shows the
per-category breakdown (Returned emerald, Lost rose, Damaged
amber, Consumed gray) instead of a single "Checked in: N"
aggregate. `asset-status-badge` tooltip contrast fixed
(`text-gray-300` on white was invisible).
- `list-asset-content` no longer crashes when a qty-tracked asset
is partially checked in — the "Checked in on" / "by" cells
fall back to empty (qty partials span multiple sessions, no
single timestamp).
- Scanner-mode text-input submit: bucket sort no longer drops
items whose `type` hasn't hydrated yet, so the API fetch fires
and the row appears.
Tests
- 10 new contract-level tests: atom behavior
(`quickCheckinQtyAssetAtom` + round-trip), hook
(seed/cleanup/refresh), drawer (button visibility per kind,
quick-checkin insertion, progress numerator, blocker trip,
submit payload shape).
- Vitest config now discovers `*.test.server.{ts,tsx}` files — a
pre-existing Phase 3a config bug meant route tests weren't running.
- Full `pnpm webapp:validate` green: 1725+ tests, typecheck + lint
clean (only the pre-existing `invariant` warning in
`kit/service.server.ts`).
Server contract (`partialCheckinBooking`, submit payload shape) is
unchanged modulo the merge-fix above — non-qty bookings submit the
exact same `assetIds[]` form data as before.
…n (Phase 3d)
Phase 3d lets operators reserve units at the model level (e.g.
"3 × Dell Latitude 5550") without picking specific assets upfront.
Concrete `BookingAsset` rows are only created at scan-to-assign
time via the existing scanner drawer, so every downstream code
path (check-in, conflict detection, PDF, email) keeps seeing the
same `BookingAsset.assetId → concrete asset` shape it always has.
Schema
- New `BookingModelRequest` table: `{ bookingId, assetModelId,
quantity }` with `@@unique([bookingId, assetModelId])`. One
intent row per (booking, model); `quantity` decrements on each
matching scan and the row is deleted when it hits zero.
- Relations added on `Booking.modelRequests` and
`AssetModel.modelRequests`. `Cascade` on delete from either side.
Service layer (`modules/booking-model-request/service.server.ts`)
- `getAssetModelAvailability({ assetModelId, bookingId, from, to })`
returns `{ total, inCustody, reservedConcrete, reservedViaRequest,
reserved, available }`. Counts INDIVIDUAL assets of the model;
subtracts custody, concrete BookingAsset reservations, AND other
bookings' model-level requests in the same date window. Excludes
the current booking.
- `upsertBookingModelRequest(...)` validates `quantity ≤ available`
inside a transaction, writes a system booking note. DRAFT /
RESERVED only — editing intent on an active booking would race
with scan-to-assign.
- `removeBookingModelRequest(...)` — same status guard, idempotent
when the row is already gone.
- `materializeModelRequestForAsset({ asset, tx })` — called from
the scan-to-assign flow. Decrements the matching request by 1
(deletes on last unit), writes an activity note naming the
assigned asset + remaining count. Must run in the caller's tx.
Checkout guard (`modules/booking/service.server.ts`)
- `checkoutBooking` refuses the RESERVED → ONGOING transition when
any `BookingModelRequest.quantity > 0`. Error payload lists
`outstanding: [{assetModelName, remaining}]` so the UI can
surface the shortfall. Hard block — operator must drain via
scan or edit the request via manage-assets. No `forcePartial`
flag; security over flexibility.
- `BOOKING_WITH_ASSETS_INCLUDE` now pulls `modelRequests` with
`assetModel` so every loader reusing the include gets the
intent rows alongside `bookingAssets`.
Scan-to-assign (`addScannedAssetsToBooking`)
- Before the bulk `BookingAsset.create`, loops over scanned
assets and calls `materializeModelRequestForAsset` inside the
tx for each. Matched assets decrement the matching request;
unmatched assets fall through to the existing "direct
BookingAsset create" path. A failure anywhere in the scan
pipeline rolls the request decrement back.
HTTP surface
- New `api+/bookings.$bookingId.model-requests.ts` — `POST`
upsert, `DELETE` remove. Shape mirrors the Phase 3b
adjust-asset-quantity route. Delegates availability + status
guards to the service module so the route stays a thin HTTP
adapter.
UI
- Manage-assets route gains a "Models" tab next to "Assets", only
visible when the org has ≥ 1 `AssetModel`. New
`manage-model-requests.tsx` component shows per-request rows
with a Remove button and an Add form (plain `<select>` + qty
input + AvailabilityBadge). The loader prefetches
`getAssetModelAvailability` for every model (capped at 50 with
a TODO for pagination).
- Booking-assets sidebar renders an "Unassigned model
reservations (N)" section above the asset list when
`booking.modelRequests` has outstanding rows. Amber remaining
chip, "Scan to assign" CTA on ONGOING / OVERDUE bookings only.
- Booking-overview PDF + reservation email template both get a
"Requested models" section. `BOOKING_INCLUDE_FOR_RESERVATION_EMAIL`
widened to ship `modelRequests` alongside assets. Renders as
`N × Model name` per row; omitted entirely when the list is
empty. Plain-text email body mirrored.
Tests
- +15 `booking-model-request/service.server.test.ts` —
availability math across empty/partial/over-reserved windows,
upsert happy path / over-reserve rejection / status-guard
rejection / zero-quantity rejection, remove idempotency + active
booking rejection, materialize decrement / delete-on-last /
no-model / no-matching-request.
- +2 `booking/service.server.test.ts` — checkout refuses with
outstanding model requests, allows when all zero. Extends the
module `db` mock with `bookingModelRequest.findMany`.
- Full `pnpm webapp:validate`: 130 files, 1742 tests, typecheck +
lint clean (pre-existing `invariant` warning in
`kit/service.server.ts` unchanged).
Manual verification deferred (task 3d-T9's 9-step browser script)
— user to walk the create-model → reserve → scan-to-assign →
checkout-guard → regression flow before merging.
Manual test checklists that were kept local until now. Committing so
the next device (and the rest of the team) can pull them as
copy-and-check runbooks for each shipped sub-phase:
- TESTING-PHASE-3B.md — quantity-aware booking UX polish
- TESTING-PHASE-3C.md — quantity-aware check-in (the 21-section
checklist the 3c bugs were caught against)
- TESTING-CHECKIN-DRAWER-REFACTOR.md — expected-list drawer port
from audits, ONE_WAY return-unused flow, drawer follow-up fixes
- TESTING-PHASE-3D.md — book-by-model: Models tab, scan-to-assign,
checkout hard-block, cross-tenant + concurrency guards, SQL
spot-checks for each DB-visible invariant
Each file follows the same shape: scope, prerequisites, numbered
sections with checkboxes, SQL spot-checks, explicit out-of-scope
list, sign-off. Section numbering is stable across files so
operators can cite e.g. "3d §11" in a bug report.
Also refreshes CLAUDE-CONTEXT.md:
- Sub-phase 3d marked COMPLETED with commit hash + plan path
- New "Sub-phase 3d follow-ups" section tracking three gaps surfaced
during manual testing:
1. Bulk-create N assets from an AssetModel (quantity + title
template)
2. Asset index grouped by model (collapsible headers + per-model
availability summaries)
3. Import round-trip for AssetModel — export already emits the
`assetModel` column (csv.server.ts:505) but
`createAssetsFromContentImport` has no matching
`createAssetModelsIfNotExists` helper, so the round-trip
silently drops the model link. Covers both content import and
backup import paths plus the downloadable template CSV.
The Models tab shipped with a plain native `<select>` — unusable at 50+ models because there's no typeahead, and visually broken because the per-column helper text pushed one column taller than its siblings so the Add button sat below the Quantity input. Swap in `DynamicSelect` (backed by the shared `api+/model-filters.ts` endpoint, which already supported the `assetModel` discriminator in its Zod schema — no server changes needed): - Picker shows debounced typeahead search, scales to any model count - Each option renders `N / M available` right-aligned so operators can see capacity without leaving the dropdown; amber when zero - `excludeItems` hides models already reserved on this booking - Client-side qty clamp uses the seed-list availability; models found via typeahead beyond the seed fall through with a permissive cap, relying on the server's in-tx availability guard as the authoritative check - Loader exports `initialAssetModels` (seed with availability on `metadata`) and `totalAssetModels` (org-wide count) so the DynamicSelect's `useModelFilters` hook picks them up Alignment fix in the same pass: - Picker + Quantity input + Add button are now three equal-height columns (`h-[38px]`) with aligned tops (`sm:items-start`) - Add button has a label-height spacer so it sits level with the two input bottoms - Helper text (availability hint / validation errors / "no availability" warning) moved to a single full-width row beneath the controls — no column is taller than the others, so the row stays level regardless of which hint is shown Typecheck + lint clean (only the pre-existing `invariant` warning in `kit/service.server.ts` unchanged).
…st editing
Manual testing of Phase 3d surfaced a set of UX problems and small bugs across
the book-by-model flow. Consolidated into one pass:
- manage-model-requests: replace ad-hoc validation with useZorm + a refined
client schema (superRefine on availability). Stop silently clamping the
quantity input — let the user type freely and show a zorm-backed error
instead. Reset picker + quantity on successful add. Add inline per-row
quantity editor on existing reservations so edits go through the upsert
endpoint without requiring remove-and-re-add.
- booking-model-request service: differentiate create / increase / decrease
notes ("reserved N × Model" vs "increased from M to N"), and skip the note
entirely on no-op upserts.
- asset-model form: fix disabled state on the full-page Save button (was
reading a fetcher that the page form doesn't use — swap to navigation-
based useDisabled).
- Reserve button: allow bookings that only hold model-level reservations
to transition to RESERVED. New hasModelRequests flag on getBookingFlags
plugs into edit-booking-form's disable guard.
- Booking overview: render a "Reserved models (N)" section above the
assets list so operators see outstanding model reservations without
opening manage-assets. Manage button deep-links to the Models tab via a
new ?tab=models query param.
- Bookings index sidebar: trigger now opens for book-by-model-only
bookings (0 concrete assets, N reserved models) — previously hasItems
gated on bookingAssets alone. Loader now includes modelRequests so the
"Unassigned model reservations" block has data.
TESTING-PHASE-3D.md check-offs for sections 1–4 included.
…equests Phase 3d-Polish: checkout on a booking with outstanding `BookingModelRequest` rows now routes through a dedicated scanner drawer that pre-renders expected units, tracks per-model progress, and submits materialisation + checkout in one atomic transaction. Model-request rows are kept post-fulfilment as an audit trail. Fulfil-and-checkout flow - New route `bookings.$bookingId.overview.fulfil-and-checkout` — dedicated scanner shell with loader that ships expected outstanding units + already- included concrete assets (with `bookedQuantity` + `type` so qty-tracked rows show `× N`). - New drawer component: per-model progress strips using `prefulfilled + matched / booked`, buckets for matched (green "Ready"), duplicate (red "Already on this booking"), unmatched (yellow warning), and already- included collapser; inline `CheckoutDialog` for early-checkout alert. - New atoms (`fulfilSessionAtom`, `expectedModelRequestsAtom`, `setFulfilSessionAtom`, `endFulfilSessionAtom`) and init hook parallel to the existing partial-checkin session pattern. - Check Out button on booking overview routes to the new flow when `outstandingModelRequestCount > 0`; otherwise keeps the existing CheckoutDialog path. Sidebar + Reserved Models "Scan to assign" CTAs keep routing to the generic scan-assets drawer for "add extras". - `fulfilModelRequestsAndCheckout` service composes scan materialisation + checkout writes in a single `\$transaction` via extracted shared helpers (`addScannedAssetsToBookingWithinTx`, `checkoutBookingWritesWithinTx`, `runCheckoutSideEffects`) so `addScannedAssetsToBooking` and `checkoutBooking` can't drift from the composed path. Audit-trail schema refactor - `BookingModelRequest` gains `fulfilledQuantity Int @default(0)` and `fulfilledAt DateTime?` (migration `20260423120457_track_fulfilled_quantity_on_booking_model_request`). - `quantity` preserves reservation intent (never decrements on scan). `fulfilledQuantity` counts materialisations. `fulfilledAt` is stamped when they match. "Outstanding" filter becomes `fulfilledAt IS NULL`. - `materializeModelRequestForAsset` now increments `fulfilledQuantity` + stamps `fulfilledAt` instead of decrement-and-delete. - `upsertBookingModelRequest` refuses to shrink below `fulfilledQuantity`; re-opens a fulfilled request by clearing `fulfilledAt` when quantity rises. - `removeBookingModelRequest` refuses delete when `fulfilledQuantity > 0` — operator must remove the concrete assets first. - `getAssetModelAvailability` sums `quantity - fulfilledQuantity` for rows where `fulfilledAt IS NULL` to avoid double-counting fulfilled units against concrete BookingAssets. - All filter sites (sidebar, Reserved Models section, PDF, email, fulfil loader, overview loader) switched from `quantity > 0` to `fulfilledAt === null` + `quantity - fulfilledQuantity` for display. - Models tab in manage-assets splits into "Active reservations" (editable) + "Fulfilled reservations" (read-only green "Fulfilled" chip with `N of M fulfilled` subtext) so ONGOING bookings show the original reservation as history instead of an empty state. Bug fixes - `removeAssets` now decrements `fulfilledQuantity` and clears `fulfilledAt` on any matching `BookingModelRequest` when removing an asset — scan/remove/rescan flow self-heals instead of inflating fulfilment and hiding the Reserved Models card. - `reserveBooking` refuses non-DRAFT source status, preventing a stale-tab double-click from writing spurious `Reserved → Reserved` transition notes and re-sending reservation emails. - Fulfil drawer rejects scans of assets already on the booking as duplicates (red badge) instead of accepting them as fresh matches and tripping `BookingAsset @@unique([bookingId, assetId])` on submit. - `CheckoutDialog` accepts a `triggerClassName` override so embedded contexts (fulfil drawer) can avoid the hardcoded `grow` that made the disabled primary button span the whole drawer width as a peach block. - Small cleanups: two `return await` lint regressions in `service.server.ts`; unused `invariant` import in `kit/service.server.ts`. Tests - 5 new service tests for `fulfilModelRequestsAndCheckout` (happy path, partial-fulfil rollback, early-date with/without adjust, off-model scan guard). - `materializeModelRequestForAsset` tests updated for audit-trail semantics (increment + stamp instead of delete). - `reserveBooking` status-guard test replaces the prior test that was asserting the buggy "works on any status" behaviour. Testing docs - TESTING-PHASE-3D.md §6/§7/§8/§11/§12/§12b-d/§22 rewritten for the new flow + audit-trail + rewritten concurrency test (two bookings racing for one pool, not two tabs on one booking). All 1747 tests pass; `pnpm webapp:validate` clean.
Moves outstanding `BookingModelRequest` rows out of the standalone
"Reserved models" Card and into the main Assets & Kits table so model
reservations sit alongside concrete assets and kits with a consistent
row language. The Card is removed.
Row anatomy
- New `ModelRequestRow` (in `booking-assets-column.tsx`) renders a
model reservation using the same column structure as `KitRow` and
`ListAssetContent`: bulk spacer + name + qty + availability +
category + tags + optional checkin + actions.
- Name cell: Package placeholder icon, model name, amber
"Reserved model" badge, and (when partially scanned) a
`N of M fulfilled` caption.
- Qty cell: `× N` where N = `quantity - fulfilledQuantity`, centered
to match the qty-tracked asset cell alignment.
- Bulk cell: empty but uses the same `md:pl-4 md:pr-3` padding as
`BulkListItemCheckbox` so column widths stay consistent across
row types.
Actions dropdown (new `ModelRequestRowActionsDropdown`)
- Built on Radix `Popover` (the shared `DropdownMenu` is deprecated
per CLAUDE.md) — mirrors `AssetRowActionsDropdown` byte-for-byte
for the trigger, mobile bottom-sheet overlay, item row styling,
and SSR fallback.
- Menu items:
- **Scan to assign** — routes to the generic scan-assets drawer.
- **Remove reservation** — fetcher DELETE to
`/api/bookings/:id/model-requests`, gated to
DRAFT/RESERVED + `fulfilledQuantity === 0` (server
enforces the same guard).
Column alignment fix across all row types
- Action-cell kebabs now right-align consistently. Previously
`AssetRowActionsDropdown` used default `justify-start` while
`KitRow` used `justify-end gap-5`, so kit kebabs sat at the right
edge of a 68px-wide cell while asset kebabs sat at its left edge —
visibly misaligned. Both dropdowns now use `flex justify-end`, so
every row's trigger sits flush right.
Loader
- `totalItems` bumped by the outstanding-model-request count so
the `ListTitle` header reads the right total when a booking has
pure book-by-model reservations. `totalPaginationItems` stays as
`paginationItems.length` — pagination arithmetic over concrete
asset/kit rows is unaffected.
Typecheck clean.
Brings in main's audit-delete + bulk-actions, working-hours overrides,
sentry hardening, asset query input validation, and the booking-note
cross-org safety harden (`createSystemBookingNote` /
`createStatusTransitionNote` now require `organizationId`).
Conflict resolution
- `apps/webapp/app/modules/booking/service.server.ts` — three regions:
- `checkoutBooking` body: kept HEAD's call to `runCheckoutSideEffects`
(the helper extracted in Phase 3d-Polish so
`fulfilModelRequestsAndCheckout` can share post-commit work).
Forwarded `organizationId` into the helper's
`createStatusTransitionNote` call to satisfy the new signature.
- `partialCheckinBooking` × 2 system notes: kept the Phase 3c
ledger-style content (precomputed `actor` + `qtyTail` for
per-disposition counts) and added the new `organizationId`
field to each `createSystemBookingNote` call.
Cleanup (non-conflicting Phase 3 callsites that the type system
caught after the merge — main hardened the signature on lines we
hadn't touched):
- `runCheckoutSideEffects` → `createStatusTransitionNote`
- `booking-model-request/service.server.ts`: upsert + remove notes
- `booking/service.server.ts`: qty-tracked check-in summary note
- `bookings.$bookingId.overview.manage-assets.tsx`: add-assets +
adjust-quantity notes
- `api+/bookings.$bookingId.adjust-asset-quantity.ts`: adjust note
Verification
- `pnpm webapp:validate` exits 0
- 1865 tests across 133 files pass (was 1747/130 pre-merge — main
added ~118 tests for audit-delete, working-hours overrides,
sentry hardening, and asset query coverage)
- Typecheck + lint clean
Resolves 7 conflicts from merging origin/main (commit aef2938, the Activity Events / Reports system from PR #2495) into feat-quantities. Conflict resolutions: - utils/error.ts — kept both new ErrorLabel entries ("Activity" from main + "Consumption Log" from ours). - components/assets/form.tsx — kept both imports (useAutoFocus from main + isQuantityTracked from ours). - components/booking/availability-label.tsx — kept ours (asset.bookingAssets.map(ba => ba.booking)) since main's asset.bookings no longer exists post-Phase-3a. - modules/booking/service.server.test.ts — kept both vitest.mock blocks (quantity-lock + consumption-log/service from ours, activity-event from main). - modules/custody/service.server.ts — wrapped releaseCustody in tx (main's pattern) and kept deleteMany: {} (Phase 2 made Custody[]). Recorded CUSTODY_RELEASED activity event inside the tx. Dropped main's unused assignCustody function (no callers anywhere; its custody: { create } pattern would have appended a duplicate Custody row in the Phase 2 array schema — the actual assign-custody route does inline custody assignment with deleteMany first). - modules/booking/service.server.ts — kept ours' Phase 3d-Polish extracted helpers (checkoutBookingWritesWithinTx, runCheckoutSideEffects) and added main's recordEvents calls inside the tx for BOOKING_CHECKED_OUT (per booking asset, mapped via bookingAssets.map(ba => ba.asset.id) for Phase 3a) and BOOKING_CHECKED_IN. For partialCheckin, recordEvents writes BOOKING_PARTIAL_CHECKIN INSIDE the tx (filtered to individualAssetIds + qtySummaries[].assetId — only assets actually touched by this session — rather than main's unfiltered assetIds input array, since Phase 3c filters out qty assets with 0/0/0/0 dispositions). - components/scanner/drawer/uses/partial-checkin-drawer.tsx — kept ours' Phase 3c qty-disposition refactor + audit-style expected list + customRenderAllItems. Kept main's BookingHeader hoisted to module scope (replacing our nested version) and threaded the booking prop through. Known follow-up: main's reports/helpers.server.ts and scripts/seed-reporting-demo/* came in unconflicted but reference pre-Phase-3a asset.bookings / booking.assets and pre-Phase-2 custody.custodian (singular). ~60 typecheck errors. Reports won't run until ported. Tracked separately, not blocking the merge.
…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:
|
… 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).
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