feat(assets): bulk assign and remove asset model from the asset index - #2782
Merged
Conversation
Adds "Update asset model" and "Remove from asset model" to the asset index Actions menu, so an existing library can be grouped into asset models without editing assets one at a time or re-importing a CSV. Requested via in-app feedback. - New `bulkUpdateAssetModel` links or unlinks the resolved selection. Quantity-tracked assets are skipped rather than failing the batch, because asset models are individually-tracked only. Only an all-quantity-tracked link errors; the equivalent unlink is a legitimate no-op. - Toasts report real counts: grouped, moved off another model, skipped, plus a distinct grey "No assets updated" with a different sentence for each zero-row cause. - Dialog titles use the loader total, so a cross-page "select all" states the true batch size instead of the page size. - Assign and remove are separate menu items and dialogs, matching the Assign/Remove tags and Add/Remove from kit pairs, so the model picker lists only real asset models. - Forwards `assetModels`/`totalAssetModels` from the simple mode loader. These were already queried by `getEntitiesWithSelectedValues` and discarded, so the picker no longer opens blank in simple mode at no extra query cost. - No activity events or notes: the singular `updateAsset` path emits neither and `ActivityAction` has no ASSET_MODEL action, per .claude/rules/bulk-event-parity.md.
This was referenced Jul 31, 2026
🩺 React Doctor — webappFindings on the files changed by this PR:
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3fd61d93bb
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
WalkthroughThe PR adds bulk asset-model assignment and removal. It adds dialogs, dropdown actions, picker data, service operations, an API route, icons, dialog state, accessibility feedback, and validation tests. ChangesBulk asset model management
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant BulkActionsDropdown
participant BulkAssetModelUpdateDialog
participant assetsBulkUpdateAssetModel
participant bulkUpdateAssetModel
participant Prisma
User->>BulkActionsDropdown: choose assign or remove asset model
BulkActionsDropdown->>BulkAssetModelUpdateDialog: open dialog
BulkAssetModelUpdateDialog->>assetsBulkUpdateAssetModel: submit selected assets and model ID
assetsBulkUpdateAssetModel->>bulkUpdateAssetModel: validate and process request
bulkUpdateAssetModel->>Prisma: resolve assets and update model links
Prisma-->>bulkUpdateAssetModel: return update counts
bulkUpdateAssetModel-->>assetsBulkUpdateAssetModel: return operation result
assetsBulkUpdateAssetModel-->>BulkAssetModelUpdateDialog: return notification or error
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
apps/webapp/app/modules/asset/service.server.ts (1)
6728-6739: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winCollapse the asset model read into a single query.
assertAssetModelBelongsToOrgonly checks that the scoped row exists and throws a 404. The laterfindFirstwith the sameidandorganizationIdcan both prove ownership and fetchname; replace the guard with the missing-row 404 from that query, preserving the existing 404 contract.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/webapp/app/modules/asset/service.server.ts` around lines 6728 - 6739, Replace the separate assertAssetModelBelongsToOrg and findFirst calls in the newAssetModelId branch with one scoped assetModel query that selects name; if no row is returned, throw the same 404 contract used by assertAssetModelBelongsToOrg, otherwise assign the returned name to modelName.apps/webapp/app/components/assets/bulk-asset-model-update-dialog.tsx (1)
148-155: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winLink asset-model errors to the picker.
The error paragraphs below the picker are not referenced by
DynamicSelect. Give the model error anidand thread it into the picker viaaria-describedby, either by adding this as aDynamicSelectprop or by rendering the error on a trigger element that accepts the attribute.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/webapp/app/components/assets/bulk-asset-model-update-dialog.tsx` around lines 148 - 155, Update the model picker and validation message in the bulk asset model update dialog so the asset-model error paragraph has a stable id and DynamicSelect references that id through aria-describedby. Extend the DynamicSelect API if needed, while preserving the existing fetcher error rendering separately.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/webapp/app/modules/asset/service.server.ts`:
- Around line 6800-6812: Update the returned resolved count in the asset update
flow to use the organization-verified assets from assetsBeforeUpdate rather than
resolvedIds, while preserving the existing behavior for valid ids. Ensure the
bulk-update route’s “already in this asset model” message is based only on
assets that matched the organization.
---
Nitpick comments:
In `@apps/webapp/app/components/assets/bulk-asset-model-update-dialog.tsx`:
- Around line 148-155: Update the model picker and validation message in the
bulk asset model update dialog so the asset-model error paragraph has a stable
id and DynamicSelect references that id through aria-describedby. Extend the
DynamicSelect API if needed, while preserving the existing fetcher error
rendering separately.
In `@apps/webapp/app/modules/asset/service.server.ts`:
- Around line 6728-6739: Replace the separate assertAssetModelBelongsToOrg and
findFirst calls in the newAssetModelId branch with one scoped assetModel query
that selects name; if no row is returned, throw the same 404 contract used by
assertAssetModelBelongsToOrg, otherwise assign the returned name to modelName.
🪄 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 Plus
Run ID: 26875a5a-9aef-46b4-8ac7-cb3ff84a666b
📒 Files selected for processing (11)
apps/webapp/app/atoms/bulk-update-dialog.tsapps/webapp/app/components/assets/bulk-actions-dropdown.tsxapps/webapp/app/components/assets/bulk-asset-model-remove-dialog.tsxapps/webapp/app/components/assets/bulk-asset-model-update-dialog.test.tsapps/webapp/app/components/assets/bulk-asset-model-update-dialog.tsxapps/webapp/app/components/bulk-update-dialog/bulk-update-dialog.tsxapps/webapp/app/components/shared/icons-map.tsxapps/webapp/app/modules/asset/data.server.tsapps/webapp/app/modules/asset/service.server.test.tsapps/webapp/app/modules/asset/service.server.tsapps/webapp/app/routes/api+/assets.bulk-update-asset-model.ts
`resolveAssetIdsForBulkOperation` returns a caller-supplied id list verbatim, so `resolvedIds.length` counted ids that may not belong to the organization. The zero-row toast then claimed "The selected assets are already in this asset model" when nothing had matched at all. Count the org-scoped read instead.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/webapp/app/modules/asset/service.server.ts (1)
6727-6739: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueCombine the model-ownership check and name lookup into one query.
assertAssetModelBelongsToOrgand the subsequentdb.assetModel.findFirstboth queryassetModelwith the same{ id: newAssetModelId, organizationId }predicate. Fetch the name as part of the ownership check instead of issuing two round trips.♻️ Proposed refactor (illustrative, depends on `assertAssetModelBelongsToOrg` signature)
- await assertAssetModelBelongsToOrg({ - assetModelId: newAssetModelId, - organizationId, - }); - - const model = await db.assetModel.findFirst({ - where: { id: newAssetModelId, organizationId }, - select: { name: true }, - }); - modelName = model?.name ?? null; + const model = await db.assetModel.findFirst({ + where: { id: newAssetModelId, organizationId }, + select: { name: true }, + }); + if (!model) { + throw new ShelfError({ + cause: null, + title: "Asset model not found", + message: "The selected asset model does not belong to this organization.", + additionalData: { organizationId, userId, assetModelId }, + label, + status: 404, + shouldBeCaptured: false, + }); + } + modelName = model.name;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/webapp/app/modules/asset/service.server.ts` around lines 6727 - 6739, Combine the ownership validation and name retrieval in the newAssetModelId branch by using a single assetModel query with the existing id and organizationId predicate, selecting the model name and preserving the current failure behavior of assertAssetModelBelongsToOrg. Set modelName from that query result and remove the separate db.assetModel.findFirst call.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@apps/webapp/app/modules/asset/service.server.ts`:
- Around line 6727-6739: Combine the ownership validation and name retrieval in
the newAssetModelId branch by using a single assetModel query with the existing
id and organizationId predicate, selecting the model name and preserving the
current failure behavior of assertAssetModelBelongsToOrg. Set modelName from
that query result and remove the separate db.assetModel.findFirst call.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 4b9f78c7-959c-4cb8-9b1a-5b28463e847f
📒 Files selected for processing (2)
apps/webapp/app/modules/asset/service.server.test.tsapps/webapp/app/modules/asset/service.server.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/webapp/app/modules/asset/service.server.test.ts
…model read
Follow-ups from the review of the bulk asset model actions.
Accessibility: DynamicSelect gains an optional `error` prop that renders the
message with role="alert" and links it to the trigger via aria-describedby.
Callers previously hand-rolled a plain paragraph, so a screen-reader user who
submitted the picker empty got nothing at all - the form simply refused to
submit. Adopted in the two bulk asset model dialogs; the remaining consumers
are left for a separate sweep. Deliberately no aria-invalid: ARIA permits it
only on input widgets and the trigger's implicit role is button.
Performance: assertAssetModelBelongsToOrg now selects and returns { id, name },
so bulkUpdateAssetModel drops the second findFirst it issued purely for the
toast label. Keeping the shared guard preserves the org-scope rule that
inlining the lookup would have broken.
Tests: new route tests for /api/assets/bulk-update-asset-model covering every
branch of its notification composition - the three zero-row reasons and the
moved/skipped suffixes - plus permission and service-forwarding assertions.
They post a URLSearchParams body because happy-dom drops empty-valued FormData
fields on the Request round trip, which would silently delete the field this
endpoint reads as "unlink".
Contributor
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/webapp/test/routes-tests/api.assets.bulk-update-asset-model.test.ts`:
- Around line 145-153: Update the requirePermissionMock fixture in beforeEach to
use the known typed return shape of requirePermission instead of an any cast,
while preserving the existing organization, role, and barcode values.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: d808e7aa-a2b4-4003-9953-837fbf51274a
📒 Files selected for processing (16)
apps/webapp/app/atoms/bulk-update-dialog.tsapps/webapp/app/components/assets/bulk-actions-dropdown.tsxapps/webapp/app/components/assets/bulk-asset-model-remove-dialog.tsxapps/webapp/app/components/assets/bulk-asset-model-update-dialog.test.tsapps/webapp/app/components/assets/bulk-asset-model-update-dialog.tsxapps/webapp/app/components/bulk-update-dialog/bulk-update-dialog.tsxapps/webapp/app/components/dynamic-select/dynamic-select.test.tsxapps/webapp/app/components/dynamic-select/dynamic-select.tsxapps/webapp/app/components/shared/icons-map.tsxapps/webapp/app/modules/asset/data.server.tsapps/webapp/app/modules/asset/service.server.test.tsapps/webapp/app/modules/asset/service.server.tsapps/webapp/app/routes/api+/assets.bulk-update-asset-model.tsapps/webapp/app/utils/org-validation.server.test.tsapps/webapp/app/utils/org-validation.server.tsapps/webapp/test/routes-tests/api.assets.bulk-update-asset-model.test.ts
🚧 Files skipped from review as they are similar to previous changes (10)
- apps/webapp/app/modules/asset/data.server.ts
- apps/webapp/app/atoms/bulk-update-dialog.ts
- apps/webapp/app/components/assets/bulk-asset-model-remove-dialog.tsx
- apps/webapp/app/components/assets/bulk-actions-dropdown.tsx
- apps/webapp/app/components/bulk-update-dialog/bulk-update-dialog.tsx
- apps/webapp/app/components/assets/bulk-asset-model-update-dialog.test.ts
- apps/webapp/app/components/assets/bulk-asset-model-update-dialog.tsx
- apps/webapp/app/modules/asset/service.server.ts
- apps/webapp/app/components/shared/icons-map.tsx
- apps/webapp/app/modules/asset/service.server.test.ts
…o any CLAUDE.md forbids `any` as a shortcut, and requirePermission is first-party code so the third-party-shape exception does not apply. Asserts the three-key literal to `Awaited<ReturnType<typeof requirePermission>>` rather than reconstructing the full return: the real result also carries currentOrganization / organizations / userOrganizations, which are Prisma payloads running to ~30 fields this route never reads. The assertion still fails the build if one of the three fields is renamed or retyped, which `as any` did not. Same approach as test/routes-tests/api+/user.entity-counts.test.ts. The identical casts in sibling route tests are untouched by this PR; sweeping them belongs in its own change.
DonKoko
approved these changes
Aug 6, 2026
carlosvirreira
added a commit
to Shelf-nu/website-v2
that referenced
this pull request
Aug 7, 2026
…f.nu#2782) (#230) * content: group a fleet into an asset model from the asset index (shelf.nu#2782) Triggered by: Shelf-nu/shelf.nu#2782 Two new asset index Actions items, Update asset model and Remove from asset model, replace the per-asset edit form and the CSV re-import as the way to group a fleet you already own. Completes the batch-actions list, which was missing seven of the sixteen menu entries, and updates every page that presented CSV as the only bulk route. * content: assert the quantity-tracked warning before capturing the dialog CodeRabbit review on #230. The dialog shot exists to show the skip warning, which renders only when the selection holds pooled stock. The script now waits on that string instead of relying on the demo workspace's first page happening to contain a quantity-tracked asset. Re-ran the pipeline with the assertion in place; both shots re-captured and inspected. --------- Co-authored-by: Carlos Virreira <macwhale@Carlos-MacBook-Pro.local>
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
2 participants
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.
What
Two new items in the asset index Actions menu, directly under "Update category":
Until now the only way to attach an existing asset to a model was the asset edit form, one
asset at a time, or a CSV re-import. Requested via in-app feedback from a user grouping an
existing library.
Both post to
/api/assets/bulk-update-asset-modeland are told apart by whetherassetModelIdis empty. That split is pinned by unit tests, because breaking it silentlybreaks removal.
Why two dialogs instead of a "remove" option inside the picker
The first version put a "Remove from asset model" row inside the model picker. In a workspace
with no models yet it reads as an asset model named "Remove from asset model". Splitting it
matches the existing Assign tags / Remove tags and Add to kit / Remove from kit pairs,
and leaves the picker listing only real asset models.
Behaviour worth reviewing
both
createAssetandupdateAssetalready 400 on that link. A mixed selection is normal ina real library, so the batch applies to the individually-tracked assets and reports the skip.
Only an all-quantity-tracked link errors; the equivalent unlink is a no-op.
that a different model's book-by-model pool just shrank), how many were skipped, and a
distinct grey "No assets updated" with its own sentence for each of the three zero-row causes.
resolveAssetIdsForBulkOperation, and the dialogtitle uses the loader total so it states the real batch size rather than the page size.
updateAssetmodel branch emits neither andActivityActionhas no ASSET_MODEL action, so emitting nothing is what.claude/rules/bulk-event-parity.mdrequires. Adding it properly needs an additive enummigration plus the singular call site plus the model-delete cascade — filed separately.
conveniences; applying them here would overwrite curated data on existing assets.
getEntitiesWithSelectedValuesalready queried asset models onevery simple-mode index load and discarded them at the destructure. Forwarding them adds no
query and is what stops the picker opening blank in simple mode.
Verified
Driven end to end in the browser against a real database: mixed selection with a
quantity-tracked asset; select-all across pages (title read 25 while the page showed 20,
23 grouped and 2 skipped, and the "No model" filtered list shrank 25 → 2); all-quantity-tracked
link rendering its 400 inline; inline model creation from the picker; empty assign showing the
required-field error; removal across three assets on two different models plus one with no
model; simple mode on a 375px viewport.
292 test files / 3852 tests, typecheck clean, lint 0 errors, react-doctor 0 errors.
Note for reviewers
The "Asset model" advanced filter only appears once that column is made visible
(
getAvailableColumnsfilters oncolumn.visible, andassetModelshipsvisible: false),so the filter-driven sweep needs one setup step today. Worth a line in the release note.
Summary by CodeRabbit