chore: do addItem on inventory document, not accountId #699

Sainan · 2025-01-03T13:21:11-08:00

Sainan commented

2025-01-03 13:21:11 -08:00

purchase 50 arcane packs - before: 2715.070 ms
purchase 50 arcane packs - after: 183.988 ms

purchase 50 arcane packs - before: 2715.070 ms purchase 50 arcane packs - after: 183.988 ms

coderabbitai[bot] commented

2025-01-03 13:21:18 -08:00

(Migrated from github.com)

Warning

Rate limit exceeded

@Sainan has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 21 minutes and 56 seconds before requesting another review.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

📥 Commits

Reviewing files that changed from the base of the PR and between 542a295cfb and 947c7a622f.

📒 Files selected for processing (1)

src/services/inventoryService.ts (11 hunks)

Walkthrough

The pull request introduces a comprehensive refactoring of inventory management across multiple services and controllers in the codebase. The primary focus is on modifying how inventory is retrieved, manipulated, and saved. The changes shift from passing account IDs to directly working with inventory objects, which allows for more precise and efficient inventory state management.

Key modifications include updating function signatures in the inventoryService.ts to accept inventory objects instead of account IDs, refactoring controllers like claimCompletedRecipeController, focusController, and modularWeaponCraftingController to retrieve inventory before performing operations, and ensuring that inventory changes are consistently saved after modifications.

The refactoring aims to centralize inventory state management, reduce redundant database calls, and provide a more consistent approach to handling inventory-related operations across different parts of the application.

Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

❤️ Share

🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

Review comments: Directly reply to a review comment made by CodeRabbit. Example:
- I pushed a fix in commit <commit_id>, please review it.
- Generate unit testing code for this file.
- Open a follow-up GitHub issue for this discussion.
Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
- @coderabbitai generate unit testing code for this file.
- @coderabbitai modularize this function.
PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
- @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
- @coderabbitai read src/utils.ts and generate unit testing code.
- @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
- @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

@coderabbitai pause to pause the reviews on a PR.
@coderabbitai resume to resume the paused reviews.
@coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
@coderabbitai full review to do a full review from scratch and review all the files again.
@coderabbitai summary to regenerate the summary of the PR.
@coderabbitai generate docstrings to generate docstrings for this PR. (Beta)
@coderabbitai resolve resolve all the CodeRabbit review comments.
@coderabbitai configuration to show the current CodeRabbit configuration for the repository.
@coderabbitai help to get help.

Other keywords and placeholders

Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
Add @coderabbitai anywhere in the PR title to generate the title automatically.

Documentation and Community

Visit our Documentation for detailed information on how to use CodeRabbit.
Join our Discord Community to get help, request features, and share feedback.
Follow us on X/Twitter for updates and announcements.

> [!WARNING] > ## Rate limit exceeded > > @Sainan has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait **21 minutes and 56 seconds** before requesting another review. > > <details> > <summary>⌛ How to resolve this issue?</summary> > > After the wait time has elapsed, a review can be triggered using the `@coderabbitai review` command as a PR comment. Alternatively, push new commits to this PR. > > We recommend that you space out your commits to avoid hitting the rate limit. > > </details> > > > <details> > <summary>🚦 How do rate limits work?</summary> > > CodeRabbit enforces hourly rate limits for each developer per organization. > > Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. > > Please see our [FAQ](https://docs.coderabbit.ai/faq) for further information. > > </details> > > <details> > <summary>📥 Commits</summary> > > Reviewing files that changed from the base of the PR and between 542a295cfbf4591b540c3991599c527c0567f81a and 947c7a622f917b4661b5662c3c1748e13ff249c1. > > </details> > > <details> > <summary>📒 Files selected for processing (1)</summary> > > * `src/services/inventoryService.ts` (11 hunks) > > </details>   ## Walkthrough The pull request introduces a comprehensive refactoring of inventory management across multiple services and controllers in the codebase. The primary focus is on modifying how inventory is retrieved, manipulated, and saved. The changes shift from passing account IDs to directly working with inventory objects, which allows for more precise and efficient inventory state management. Key modifications include updating function signatures in the `inventoryService.ts` to accept inventory objects instead of account IDs, refactoring controllers like `claimCompletedRecipeController`, `focusController`, and `modularWeaponCraftingController` to retrieve inventory before performing operations, and ensuring that inventory changes are consistently saved after modifications. The refactoring aims to centralize inventory state management, reduce redundant database calls, and provide a more consistent approach to handling inventory-related operations across different parts of the application.      --- Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? <details> <summary>❤️ Share</summary> - [X](https://twitter.com/intent/tweet?text=I%20just%20used%20%40coderabbitai%20for%20my%20code%20review%2C%20and%20it%27s%20fantastic%21%20It%27s%20free%20for%20OSS%20and%20offers%20a%20free%20trial%20for%20the%20proprietary%20code.%20Check%20it%20out%3A&url=https%3A//coderabbit.ai) - [Mastodon](https://mastodon.social/share?text=I%20just%20used%20%40coderabbitai%20for%20my%20code%20review%2C%20and%20it%27s%20fantastic%21%20It%27s%20free%20for%20OSS%20and%20offers%20a%20free%20trial%20for%20the%20proprietary%20code.%20Check%20it%20out%3A%20https%3A%2F%2Fcoderabbit.ai) - [Reddit](https://www.reddit.com/submit?title=Great%20tool%20for%20code%20review%20-%20CodeRabbit&text=I%20just%20used%20CodeRabbit%20for%20my%20code%20review%2C%20and%20it%27s%20fantastic%21%20It%27s%20free%20for%20OSS%20and%20offers%20a%20free%20trial%20for%20proprietary%20code.%20Check%20it%20out%3A%20https%3A//coderabbit.ai) - [LinkedIn](https://www.linkedin.com/sharing/share-offsite/?url=https%3A%2F%2Fcoderabbit.ai&mini=true&title=Great%20tool%20for%20code%20review%20-%20CodeRabbit&summary=I%20just%20used%20CodeRabbit%20for%20my%20code%20review%2C%20and%20it%27s%20fantastic%21%20It%27s%20free%20for%20OSS%20and%20offers%20a%20free%20trial%20for%20proprietary%20code) </details> <details> <summary>🪧 Tips</summary> ### Chat There are 3 ways to chat with [CodeRabbit](https://coderabbit.ai): - Review comments: Directly reply to a review comment made by CodeRabbit. Example: - `I pushed a fix in commit <commit_id>, please review it.` - `Generate unit testing code for this file.` - `Open a follow-up GitHub issue for this discussion.` - Files and specific lines of code (under the "Files changed" tab): Tag `@coderabbitai` in a new review comment at the desired location with your query. Examples: - `@coderabbitai generate unit testing code for this file.` - `@coderabbitai modularize this function.` - PR comments: Tag `@coderabbitai` in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples: - `@coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.` - `@coderabbitai read src/utils.ts and generate unit testing code.` - `@coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.` - `@coderabbitai help me debug CodeRabbit configuration file.` Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. ### CodeRabbit Commands (Invoked using PR comments) - `@coderabbitai pause` to pause the reviews on a PR. - `@coderabbitai resume` to resume the paused reviews. - `@coderabbitai review` to trigger an incremental review. This is useful when automatic reviews are disabled for the repository. - `@coderabbitai full review` to do a full review from scratch and review all the files again. - `@coderabbitai summary` to regenerate the summary of the PR. - `@coderabbitai generate docstrings` to [generate docstrings](https://docs.coderabbit.ai/finishing-touches/docstrings) for this PR. ([Beta](https://docs.coderabbit.ai/early-access#beta)) - `@coderabbitai resolve` resolve all the CodeRabbit review comments. - `@coderabbitai configuration` to show the current CodeRabbit configuration for the repository. - `@coderabbitai help` to get help. ### Other keywords and placeholders - Add `@coderabbitai ignore` anywhere in the PR description to prevent this PR from being reviewed. - Add `@coderabbitai summary` to generate the high-level summary at a specific location in the PR description. - Add `@coderabbitai` anywhere in the PR title to generate the title automatically. ### Documentation and Community - Visit our [Documentation](https://docs.coderabbit.ai) for detailed information on how to use CodeRabbit. - Join our [Discord Community](http://discord.gg/coderabbit) to get help, request features, and share feedback. - Follow us on [X/Twitter](https://twitter.com/coderabbitai) for updates and announcements. </details>

coderabbitai[bot] (Migrated from github.com) reviewed 2025-01-03 13:25:17 -08:00

coderabbitai[bot] (Migrated from github.com) left a comment

Actionable comments posted: 1

🧹 Nitpick comments (4)

src/controllers/custom/addItemController.ts (1)

20-20: Include error checks for weapon type
You’re getting weaponType = getWeaponType(...), then calling addEquipment. If the returned weaponType is invalid or empty, you may want to handle or raise an error first.

Also applies to: 22-24
src/services/purchaseService.ts (3)
207-212: Move the default clause to the last case in the switch block.
This is a best practice to enhance readability, in line with the static analysis hint. Having the default clause in between other case statements can cause confusion for future maintainers.
         switch (storeCategory) {
-            default: {
-                const inventory = await getInventory(accountId);
-                purchaseResponse = await addItem(inventory, internalName, quantity);
-                await inventory.save();
-                break;
-            }
             case "Types":
                 purchaseResponse = await handleTypesPurchase(internalName, accountId, quantity);
                 break;
             case "Boosters":
                 purchaseResponse = await handleBoostersPurchase(internalName, accountId, durability);
                 break;
+            default: {
+                const inventory = await getInventory(accountId);
+                purchaseResponse = await addItem(inventory, internalName, quantity);
+                await inventory.save();
+                break;
+            }
         }
🧰 Tools

🪛 Biome (1.9.4)

[error] 207-212: The default clause should be the last switch clause.

The following case clause is here:

Regardless its position, the default clause is always executed when there is no match. To avoid confusion, the default clause should be the last switch clause.

(lint/suspicious/useDefaultSwitchClauseLast)

295-295: Watch for potential performance overhead when addItem is invoked many times in a loop.
Although the inventory is only saved once at line 300, repeated calls to addItem might still be costly. Consider a batched approach if performance is critical.

313-318: Place the default clause last in the switch statement.
This mirrors the earlier improvement. Reordering this clause to the bottom improves clarity.
         switch (typeCategory) {
-            default: {
-                const inventory = await getInventory(accountId);
-                const resp = await addItem(inventory, typesName, quantity);
-                await inventory.save();
-                return resp;
-            }
             case "BoosterPacks":
                 return await handleBoosterPackPurchase(typesName, accountId, quantity);
             case "SlotItems":
                 return await handleSlotPurchase(typesName, accountId);
+            default: {
+                const inventory = await getInventory(accountId);
+                const resp = await addItem(inventory, typesName, quantity);
+                await inventory.save();
+                return resp;
+            }
         }
🧰 Tools

🪛 Biome (1.9.4)

[error] 313-318: The default clause should be the last switch clause.

The following case clause is here:

Regardless its position, the default clause is always executed when there is no match. To avoid confusion, the default clause should be the last switch clause.

(lint/suspicious/useDefaultSwitchClauseLast)

📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between e6432b5052 and 542a295cfb.

📒 Files selected for processing (7)

src/controllers/api/claimCompletedRecipeController.ts (1 hunks)
src/controllers/api/focusController.ts (2 hunks)
src/controllers/api/inventorySlotsController.ts (2 hunks)
src/controllers/api/modularWeaponCraftingController.ts (1 hunks)
src/controllers/custom/addItemController.ts (2 hunks)
src/services/inventoryService.ts (10 hunks)
src/services/purchaseService.ts (5 hunks)

🧰 Additional context used

🪛 Biome (1.9.4)

src/services/purchaseService.ts

[error] 207-212: The default clause should be the last switch clause.

The following case clause is here:

Regardless its position, the default clause is always executed when there is no match. To avoid confusion, the default clause should be the last switch clause.

(lint/suspicious/useDefaultSwitchClauseLast)

[error] 313-318: The default clause should be the last switch clause.

The following case clause is here:

Regardless its position, the default clause is always executed when there is no match. To avoid confusion, the default clause should be the last switch clause.

(lint/suspicious/useDefaultSwitchClauseLast)

🔇 Additional comments (33)

src/services/inventoryService.ts (21)

108-108: Ensure no unexpected item types slip through
The addItem function now directly operates on the passed inventory. This is good for performance but ensure proper validations so that any unknown typeName triggers a safe fallback or explicit error.

129-129: Validate Ship creation parameters
The call to createShip(inventory.accountOwnerId, typeName) here might fail if typeName is invalid, but the code doesn’t safeguard that scenario. Consider catching or validating before passing.

142-143: Enhance consistency in naming
addCrewShip is used here, which mirrors the pattern of addPowerSuit, etc. This is consistent with the new approach. No issues found.

160-161: Item path consistency
addSkin is properly called, now referencing the inventory object. The return structure is consistent with the rest of the code.

164-165: Minimal duplication
addCustomization is analogous to addSkin. The function name is good. Looks consistent.

201-202: Slot updates after adding suits
Reserving an extra slot upon suit addition is coherent with the rest of the code. No issues spotted.

215-216: Uniform approach for space suits
The logic to register suits and track slot usage is correct. The object spread merges changes nicely.

Also applies to: 219-219, 224-224

229-230: Suit bin usage
addMechSuit(inventory, typeName) plus updateSlots(inventory, InventorySlot.MECHSUITS, 0, 1) is aligned with other suit additions.

246-247: Weapon slot deduction
Calling both addEquipment and then updateSlots is consistent. The WeaponBin structure is a neat way to summarize the slot-usage effect.

Also applies to: 250-251

273-274: Sentinel slot consistency
addSentinel(inventory, typeName) followed by updateSlots for sentinels ensures parallel structure.

340-344: Better clarity for sentinel creation
Having addSentinel require inventory plus optional inventoryChanges clarifies the code flow, removing the old async calls. Looks good.

Also applies to: 346-346

365-369: Efficient sentinel push
Creating the sentinel in the array and pushing immediately into inventoryChanges.Sentinels looks clean. No concurrency risk is apparent here since everything is synchronous in a single operation.

374-383: Naming alignment
addSentinelWeapon also looks consistent. Reusing inventoryChanges.SentinelWeapons ensures uniform approach for expansions.

386-390: Extending Exalted logic
The check for special items (getExalted) and the loop to addSpecialItem is a good pattern. No obvious issues found.

Also applies to: 393-394, 397-399

403-407: Parallel structure for mechs
addMechSuit follows the same structural pattern as addPowerSuit. Code consistency is high.

Also applies to: 410-411, 414-416

420-424: Potential duplicates
addSpecialItem discards duplicates with a simple find check, which is good. The final push into inventoryChanges.SpecialItems is properly typed.

Also applies to: 428-439

455-460: GA: general approach
updateSlots now uses inventory directly, eliminating repeated lookups by accountId. This improvement is aligned with the overall PR objective.

538-555: Reusable pattern
addEquipment similarly updates inventory in a consistent manner with the other addX functions. The push to inventoryChanges[category] is well-structured.

558-568: Flavour item addition
addCustomization is straightforward, following the same pattern. No concerns here.

571-581: Check missing index assignment
In addSkin, the local index assignment is missing from the snippet (line 576 might be overshadowed in the diff). Ensure const index = inventory.WeaponSkins.push({ ItemType: typeName }) - 1; is included, as the code references inventory.WeaponSkins[index].

584-592: Non-exported function
addCrewShip is introduced as a local function, consistent with other addX patterns. It’s named properly and merges changes into inventoryChanges. Good addition.

src/controllers/custom/addItemController.ts (2)

4-4: Use consistent imports
Importing getInventory alongside addPowerSuit and addEquipment ensures a single source of truth for inventory retrieval. Good approach.

13-16: Positive approach
Fetching inventory before calling addPowerSuit streamlines logic. The subsequent save is also essential to persist changes.

src/controllers/api/inventorySlotsController.ts (2)

2-2: Refactored currency updates
Importing updateCurrency directly indicates the shift away from the old updateCurrencyByAccountId. This is consistent with the inventory-based pattern.

29-32: Slot purchase flow
getInventory(accountId) → updateCurrency(inventory, 20, true) → updateSlots(inventory, ...) → inventory.save() is a clear and coherent flow.

src/controllers/api/modularWeaponCraftingController.ts (2)

37-37: Inventory retrieval
Fetching the inventory first ensures we have a single source of truth for subsequent operations. This matches the refactoring approach seen elsewhere.

40-40: Currency deduction and part consumption
addEquipment followed by updateCurrency(...) and addMiscItems(...) for part removal is logically consistent. Good job.

src/controllers/api/claimCompletedRecipeController.ts (1)

86-92: Smooth final claim flow
Retrieving the inventory again ensures no stale data, especially after removing the pending recipe. Merging the addItem changes into the final InventoryChanges object is a clean approach.

src/services/purchaseService.ts (3)

254-256: Consider adding error handling around inventory.
Although it’s unlikely, if getInventory fails or returns an unexpected value, updateSlots might operate on a non-existent object. Adding a quick check to ensure inventory is valid or throwing a descriptive error can make the code more robust.

285-285: Good approach to load the inventory once before the loop to minimize I/O overhead.
This is a prudent move. Doing all item additions in a loop and saving afterward is efficient.

300-300: Good practice performing a single save after the loop.
This pattern avoids repeated writes, which boosts performance.
src/controllers/api/focusController.ts (2)

7-7: New import is logically consistent.
Importing IEquipmentClient makes sense to maintain consistent typing and clarity for equipment changes.

106-109: Consider validating inventory before and after adding equipment.
If addEquipment fails or the inventory is null, an error might occur at save time. Adding minimal checking ensures robust handling of edge cases.
+ if (!inventory) {
+   throw new Error("Failed to retrieve inventory for the given accountId");
+ }
  const inventoryChanges = addEquipment(inventory, "OperatorAmps", request.StartingWeaponType, parts);
  await inventory.save();
  res.json((inventoryChanges.OperatorAmps as IEquipmentClient[])[0]);

**Actionable comments posted: 1** <details> <summary>🧹 Nitpick comments (4)</summary><blockquote> <details> <summary>src/controllers/custom/addItemController.ts (1)</summary><blockquote> `20-20`: **Include error checks for weapon type** You’re getting `weaponType = getWeaponType(...)`, then calling `addEquipment`. If the returned `weaponType` is invalid or empty, you may want to handle or raise an error first. Also applies to: 22-24 </blockquote></details> <details> <summary>src/services/purchaseService.ts (3)</summary><blockquote> `207-212`: **Move the `default` clause to the last case in the switch block.** This is a best practice to enhance readability, in line with the static analysis hint. Having the `default` clause in between other `case` statements can cause confusion for future maintainers. ```diff switch (storeCategory) { - default: { - const inventory = await getInventory(accountId); - purchaseResponse = await addItem(inventory, internalName, quantity); - await inventory.save(); - break; - } case "Types": purchaseResponse = await handleTypesPurchase(internalName, accountId, quantity); break; case "Boosters": purchaseResponse = await handleBoostersPurchase(internalName, accountId, durability); break; + default: { + const inventory = await getInventory(accountId); + purchaseResponse = await addItem(inventory, internalName, quantity); + await inventory.save(); + break; + } } ``` <details> <summary>🧰 Tools</summary> <details> <summary>🪛 Biome (1.9.4)</summary> [error] 207-212: The default clause should be the last switch clause. The following case clause is here: Regardless its position, the default clause is always executed when there is no match. To avoid confusion, the default clause should be the last switch clause. (lint/suspicious/useDefaultSwitchClauseLast) </details> </details> --- `295-295`: **Watch for potential performance overhead when `addItem` is invoked many times in a loop.** Although the inventory is only saved once at line 300, repeated calls to `addItem` might still be costly. Consider a batched approach if performance is critical. --- `313-318`: **Place the `default` clause last in the switch statement.** This mirrors the earlier improvement. Reordering this clause to the bottom improves clarity. ```diff switch (typeCategory) { - default: { - const inventory = await getInventory(accountId); - const resp = await addItem(inventory, typesName, quantity); - await inventory.save(); - return resp; - } case "BoosterPacks": return await handleBoosterPackPurchase(typesName, accountId, quantity); case "SlotItems": return await handleSlotPurchase(typesName, accountId); + default: { + const inventory = await getInventory(accountId); + const resp = await addItem(inventory, typesName, quantity); + await inventory.save(); + return resp; + } } ``` <details> <summary>🧰 Tools</summary> <details> <summary>🪛 Biome (1.9.4)</summary> [error] 313-318: The default clause should be the last switch clause. The following case clause is here: Regardless its position, the default clause is always executed when there is no match. To avoid confusion, the default clause should be the last switch clause. (lint/suspicious/useDefaultSwitchClauseLast) </details> </details> </blockquote></details> </blockquote></details> <details> <summary>📜 Review details</summary> **Configuration used: .coderabbit.yaml** **Review profile: CHILL** **Plan: Pro** <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between e6432b505244d900e75a8332d35bc9149d88990f and 542a295cfbf4591b540c3991599c527c0567f81a. </details> <details> <summary>📒 Files selected for processing (7)</summary> * `src/controllers/api/claimCompletedRecipeController.ts` (1 hunks) * `src/controllers/api/focusController.ts` (2 hunks) * `src/controllers/api/inventorySlotsController.ts` (2 hunks) * `src/controllers/api/modularWeaponCraftingController.ts` (1 hunks) * `src/controllers/custom/addItemController.ts` (2 hunks) * `src/services/inventoryService.ts` (10 hunks) * `src/services/purchaseService.ts` (5 hunks) </details> <details> <summary>🧰 Additional context used</summary> <details> <summary>🪛 Biome (1.9.4)</summary> <details> <summary>src/services/purchaseService.ts</summary> [error] 207-212: The default clause should be the last switch clause. The following case clause is here: Regardless its position, the default clause is always executed when there is no match. To avoid confusion, the default clause should be the last switch clause. (lint/suspicious/useDefaultSwitchClauseLast) --- [error] 313-318: The default clause should be the last switch clause. The following case clause is here: Regardless its position, the default clause is always executed when there is no match. To avoid confusion, the default clause should be the last switch clause. (lint/suspicious/useDefaultSwitchClauseLast) </details> </details> </details> <details> <summary>🔇 Additional comments (33)</summary><blockquote> <details> <summary>src/services/inventoryService.ts (21)</summary> `108-108`: **Ensure no unexpected item types slip through** The `addItem` function now directly operates on the passed `inventory`. This is good for performance but ensure proper validations so that any unknown `typeName` triggers a safe fallback or explicit error. --- `129-129`: **Validate Ship creation parameters** The call to `createShip(inventory.accountOwnerId, typeName)` here might fail if `typeName` is invalid, but the code doesn’t safeguard that scenario. Consider catching or validating before passing. --- `142-143`: **Enhance consistency in naming** `addCrewShip` is used here, which mirrors the pattern of `addPowerSuit`, etc. This is consistent with the new approach. No issues found. --- `160-161`: **Item path consistency** `addSkin` is properly called, now referencing the `inventory` object. The return structure is consistent with the rest of the code. --- `164-165`: **Minimal duplication** `addCustomization` is analogous to `addSkin`. The function name is good. Looks consistent. --- `201-202`: **Slot updates after adding suits** Reserving an extra slot upon suit addition is coherent with the rest of the code. No issues spotted. --- `215-216`: **Uniform approach for space suits** The logic to register suits and track slot usage is correct. The object spread merges changes nicely. Also applies to: 219-219, 224-224 --- `229-230`: **Suit bin usage** `addMechSuit(inventory, typeName)` plus `updateSlots(inventory, InventorySlot.MECHSUITS, 0, 1)` is aligned with other suit additions. --- `246-247`: **Weapon slot deduction** Calling both `addEquipment` and then `updateSlots` is consistent. The `WeaponBin` structure is a neat way to summarize the slot-usage effect. Also applies to: 250-251 --- `273-274`: **Sentinel slot consistency** `addSentinel(inventory, typeName)` followed by `updateSlots` for sentinels ensures parallel structure. --- `340-344`: **Better clarity for sentinel creation** Having `addSentinel` require `inventory` plus optional `inventoryChanges` clarifies the code flow, removing the old async calls. Looks good. Also applies to: 346-346 --- `365-369`: **Efficient sentinel push** Creating the sentinel in the array and pushing immediately into `inventoryChanges.Sentinels` looks clean. No concurrency risk is apparent here since everything is synchronous in a single operation. --- `374-383`: **Naming alignment** `addSentinelWeapon` also looks consistent. Reusing `inventoryChanges.SentinelWeapons` ensures uniform approach for expansions. --- `386-390`: **Extending Exalted logic** The check for special items (`getExalted`) and the loop to `addSpecialItem` is a good pattern. No obvious issues found. Also applies to: 393-394, 397-399 --- `403-407`: **Parallel structure for mechs** `addMechSuit` follows the same structural pattern as `addPowerSuit`. Code consistency is high. Also applies to: 410-411, 414-416 --- `420-424`: **Potential duplicates** `addSpecialItem` discards duplicates with a simple find check, which is good. The final push into `inventoryChanges.SpecialItems` is properly typed. Also applies to: 428-439 --- `455-460`: **GA: general approach** `updateSlots` now uses `inventory` directly, eliminating repeated lookups by `accountId`. This improvement is aligned with the overall PR objective. --- `538-555`: **Reusable pattern** `addEquipment` similarly updates `inventory` in a consistent manner with the other `addX` functions. The push to `inventoryChanges[category]` is well-structured. --- `558-568`: **Flavour item addition** `addCustomization` is straightforward, following the same pattern. No concerns here. --- `571-581`: **Check missing index assignment** In `addSkin`, the local `index` assignment is missing from the snippet (line 576 might be overshadowed in the diff). Ensure `const index = inventory.WeaponSkins.push({ ItemType: typeName }) - 1;` is included, as the code references `inventory.WeaponSkins[index]`. --- `584-592`: **Non-exported function** `addCrewShip` is introduced as a local function, consistent with other `addX` patterns. It’s named properly and merges changes into `inventoryChanges`. Good addition. </details> <details> <summary>src/controllers/custom/addItemController.ts (2)</summary> `4-4`: **Use consistent imports** Importing `getInventory` alongside `addPowerSuit` and `addEquipment` ensures a single source of truth for inventory retrieval. Good approach. --- `13-16`: **Positive approach** Fetching `inventory` before calling `addPowerSuit` streamlines logic. The subsequent save is also essential to persist changes. </details> <details> <summary>src/controllers/api/inventorySlotsController.ts (2)</summary> `2-2`: **Refactored currency updates** Importing `updateCurrency` directly indicates the shift away from the old `updateCurrencyByAccountId`. This is consistent with the inventory-based pattern. --- `29-32`: **Slot purchase flow** `getInventory(accountId)` → `updateCurrency(inventory, 20, true)` → `updateSlots(inventory, ...)` → `inventory.save()` is a clear and coherent flow. </details> <details> <summary>src/controllers/api/modularWeaponCraftingController.ts (2)</summary> `37-37`: **Inventory retrieval** Fetching the inventory first ensures we have a single source of truth for subsequent operations. This matches the refactoring approach seen elsewhere. --- `40-40`: **Currency deduction and part consumption** `addEquipment` followed by `updateCurrency(...)` and `addMiscItems(...)` for part removal is logically consistent. Good job. </details> <details> <summary>src/controllers/api/claimCompletedRecipeController.ts (1)</summary> `86-92`: **Smooth final claim flow** Retrieving the inventory again ensures no stale data, especially after removing the pending recipe. Merging the `addItem` changes into the final `InventoryChanges` object is a clean approach. </details> <details> <summary>src/services/purchaseService.ts (3)</summary> `254-256`: **Consider adding error handling around `inventory`.** Although it’s unlikely, if `getInventory` fails or returns an unexpected value, `updateSlots` might operate on a non-existent object. Adding a quick check to ensure `inventory` is valid or throwing a descriptive error can make the code more robust. --- `285-285`: **Good approach to load the inventory once before the loop to minimize I/O overhead.** This is a prudent move. Doing all item additions in a loop and saving afterward is efficient. --- `300-300`: **Good practice performing a single save after the loop.** This pattern avoids repeated writes, which boosts performance. </details> <details> <summary>src/controllers/api/focusController.ts (2)</summary> `7-7`: **New import is logically consistent.** Importing `IEquipmentClient` makes sense to maintain consistent typing and clarity for equipment changes. --- `106-109`: **Consider validating `inventory` before and after adding equipment.** If `addEquipment` fails or the inventory is null, an error might occur at save time. Adding minimal checking ensures robust handling of edge cases. ```diff + if (!inventory) { + throw new Error("Failed to retrieve inventory for the given accountId"); + } const inventoryChanges = addEquipment(inventory, "OperatorAmps", request.StartingWeaponType, parts); await inventory.save(); res.json((inventoryChanges.OperatorAmps as IEquipmentClient[])[0]); ``` </details> </blockquote></details> </details>

src/services/inventoryService.ts

coderabbitai[bot] (Migrated from github.com) commented

2025-01-03 13:25:16 -08:00

⚠️ Potential issue

Avoid indexing the newly added item with [suitIndex - 1]
The new suit’s index is suitIndex = SpaceSuits.length - 1 after .push(...). Using [suitIndex - 1] references the prior item, not the newly added one. This is likely a bug causing misreporting or overwriting.

Consider this fix:

-    (inventoryChanges.SpaceSuits as IEquipmentClient[]).push(
-        inventory.SpaceSuits[suitIndex - 1].toJSON<IEquipmentClient>()
-    );
+    (inventoryChanges.SpaceSuits as IEquipmentClient[]).push(
+        inventory.SpaceSuits[suitIndex].toJSON<IEquipmentClient>()
+    );

Also applies to: 447-451

_:warning: Potential issue_ **Avoid indexing the newly added item with `[suitIndex - 1]`** The new suit’s index is `suitIndex = SpaceSuits.length - 1` after `.push(...)`. Using `[suitIndex - 1]` references the prior item, not the newly added one. This is likely a bug causing misreporting or overwriting. Consider this fix: ```diff - (inventoryChanges.SpaceSuits as IEquipmentClient[]).push( - inventory.SpaceSuits[suitIndex - 1].toJSON<IEquipmentClient>() - ); + (inventoryChanges.SpaceSuits as IEquipmentClient[]).push( + inventory.SpaceSuits[suitIndex].toJSON<IEquipmentClient>() + ); ``` Also applies to: 447-451

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: OpenWF/SpaceNinjaServer#699