fix(core): Handle empty sortedAssets in updateEntityAssets - #4397
Conversation
When assetIds contains IDs not present in the current channel, findByIdsInChannel filters them out leaving sortedAssets empty. createOrderableAssets then crashes accessing index [0] on the empty array. Guard against this by checking sortedAssets.length.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThis pull request adds a test case and modifies asset handling logic. The test file includes a new test case verifying that updating a product with asset IDs not present in the current channel clears the product's assets without crashing. The asset service is updated to explicitly set 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/core/src/service/services/asset.service.ts (1)
279-283: Fix is correct — minor inconsistency with theelse ifbranch worth aligning.The guard correctly prevents calling
createOrderableAssetson an empty array, fixing theTypeError. However, there's now a behavioural asymmetry inupdateEntityAssets:
Scenario DB state entity.assetsin memoryassetIdsnon-empty, all filtered out (new path)orderable assets removed ✅ set to []✅assetIdsis[](existingelse if)orderable assets removed ✅ not updated — stale value remains Any caller that passes
assetIds: []and uses the returned entity without reloading will see stale assets in memory. Consider aligning theelse ifbranch:♻️ Suggested fix for consistency
} else if (assetIds && assetIds.length === 0) { await this.removeExistingOrderableAssets(ctx, entity); + entity.assets = []; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/service/services/asset.service.ts` around lines 279 - 283, In updateEntityAssets, the branch handling the case where assetIds is an empty array leaves entity.assets unchanged causing stale in-memory assets; align it with the other branch by setting entity.assets = [] when you removed orderable assets (the same place you use createOrderableAssets/sortedAssets logic) so callers receive the updated empty assets list; locate the updateEntityAssets method and after the DB removal path for assetIds === [] explicitly assign entity.assets = [].
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/core/e2e/asset-channel.e2e-spec.ts`:
- Around line 291-305: The new test "Updating product with asset IDs not in
channel does not crash" causes global deletion by removeExistingOrderableAssets
(called from updateEntityAssets), which clears all orderable assets for product
T_2 and makes the subsequent "Channel2 does not have asset A" test vacuously
pass; either move this new test to run after the channel-isolation test, or add
a follow-up assertion using adminClient in the default channel to confirm T_2
still has asset T_3 (verifying deletion was channel-scoped), and if global
deletion is intended update the test comment to state that explicitly; reference
removeExistingOrderableAssets, updateEntityAssets, the test updating product
with id 'T_2' and asset 'T_3', and the "Channel2 does not have asset A" test
when applying the change.
---
Nitpick comments:
In `@packages/core/src/service/services/asset.service.ts`:
- Around line 279-283: In updateEntityAssets, the branch handling the case where
assetIds is an empty array leaves entity.assets unchanged causing stale
in-memory assets; align it with the other branch by setting entity.assets = []
when you removed orderable assets (the same place you use
createOrderableAssets/sortedAssets logic) so callers receive the updated empty
assets list; locate the updateEntityAssets method and after the DB removal path
for assetIds === [] explicitly assign entity.assets = [].
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/core/e2e/asset-channel.e2e-spec.tspackages/core/src/service/services/asset.service.ts
|
Thank you! |
Description
AssetService.updateEntityAssets() crashes when assetIds contains IDs that don't exist in the current channel. After findByIdsInChannel filters them out, sortedAssets becomes empty and createOrderableAssets crashes accessing [0] on the empty array:
The guard at line 273 only checks the input assetIds array, not the filtered result. This adds an empty check on sortedAssets before calling createOrderableAssets, matching the behavior of the else if branch which already handles explicitly empty assetIds.
Fixes #4398
Breaking changes
No
Checklist
📌 Always:
👍 Most of the time: