feat: leveling up intrinsics #725

Sainan · 2025-01-05T04:18:21-08:00

Sainan commented

2025-01-05 04:18:21 -08:00

Closes #708

coderabbitai[bot] commented

2025-01-05 04:18:26 -08:00

(Migrated from github.com)

Walkthrough

The pull request introduces a new playerSkillsController in the API to handle player skill upgrades. The changes span multiple files, including creating a new controller, modifying the inventory model, updating API routes, and adjusting inventory type definitions. The controller processes skill upgrade requests by calculating the cost of upgrading a specific skill based on its current rank, deducting the cost from the specified pool in the player's inventory, and incrementing the skill's rank. The changes ensure that player skills are properly initialized with default values and provide a mechanism for upgrading skills through a new API endpoint.

Assessment against linked issues

Objective	Addressed	Explanation
Handle skill upgrade endpoint	✅
Calculate skill upgrade cost	✅
Support skill leveling for intrinsics	✅
Validate skill upgrade request	❓	No explicit validation logic visible in the provided changes.

The implementation directly addresses the primary concerns raised in issue #708 by:

Creating a new endpoint for /playerSkills.php
Implementing a controller that can process skill upgrade requests
Providing logic to calculate skill upgrade costs
Supporting the specific example of upgrading LPS_DRIFT_RIDING using the LPP_DRIFTER pool

The changes appear to provide a comprehensive solution for leveling up intrinsic skills, with the controller handling the core logic of skill upgrades.

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.

## Walkthrough The pull request introduces a new `playerSkillsController` in the API to handle player skill upgrades. The changes span multiple files, including creating a new controller, modifying the inventory model, updating API routes, and adjusting inventory type definitions. The controller processes skill upgrade requests by calculating the cost of upgrading a specific skill based on its current rank, deducting the cost from the specified pool in the player's inventory, and incrementing the skill's rank. The changes ensure that player skills are properly initialized with default values and provide a mechanism for upgrading skills through a new API endpoint. ## Assessment against linked issues | Objective | Addressed | Explanation | |-----------------------------------------|-----------|-------------| | Handle skill upgrade endpoint | ✅ | | | Calculate skill upgrade cost | ✅ | | | Support skill leveling for intrinsics | ✅ | | | Validate skill upgrade request | ❓ | No explicit validation logic visible in the provided changes. | The implementation directly addresses the primary concerns raised in issue #708 by: 1. Creating a new endpoint for `/playerSkills.php` 2. Implementing a controller that can process skill upgrade requests 3. Providing logic to calculate skill upgrade costs 4. Supporting the specific example of upgrading `LPS_DRIFT_RIDING` using the `LPP_DRIFTER` pool The changes appear to provide a comprehensive solution for leveling up intrinsic skills, with the controller handling the core logic of skill upgrades.      --- 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-05 04:21:26 -08:00

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

Actionable comments posted: 2

🧹 Nitpick comments (4)

src/controllers/api/playerSkillsController.ts (3)
1-6: Consider adding error handling for imports and environment readiness.

While these imports are straightforward, any failure in reading or missing environment variables (if the services rely on them) might cause unhandled exceptions during initialization. If your getAccountIdForRequest, getInventory, or other dependencies can throw errors when, for instance, environment variables are misconfigured, consider wrapping those calls in try/catch or dedicated error handling blocks to provide clearer debugging messages.

16-23: Return robust response details and implement partial success checks.

The current response object is minimal, which is fine for simple client usage. However, you could include additional metadata (like the updated skill pool total) or perform a final confirmation read from the saved inventory to ensure the changes persist. Additionally, if save() or other operations fail, ensure the client is notified instead of sending success JSON.

26-29: Use stricter typing for the request interface.

Pool and Skill are typed as string, which is flexible but can lead to runtime errors if an invalid key is passed. Consider using a union type or enum for expected values to enforce compile-time checks.
interface IPlayerSkillsRequest {
  Pool: "LPP_SPACE" | "LPS_PILOTING" | "RegularCredits" | ...; // Example
  Skill: "LPP_SPACE" | "LPS_PILOTING" | "LPS_GUNNERY" | ...;
}
src/models/inventoryModels/inventoryModel.ts (1)

1005-1005: Initialize PlayerSkills with an empty object by default.

Initializing PlayerSkills with {} ensures newly created inventories have the correct structure. Verify that references to subfields in PlayerSkills handle the case where the entire object might be empty, if older documents are missing these fields.

📜 Review details

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

📥 Commits

Reviewing files that changed from the base of the PR and between 1bab76f58b and 101d6435f6.

📒 Files selected for processing (4)

src/controllers/api/playerSkillsController.ts (1 hunks)
src/models/inventoryModels/inventoryModel.ts (2 hunks)
src/routes/api.ts (2 hunks)
src/types/inventoryTypes/inventoryTypes.ts (1 hunks)

🔇 Additional comments (4)

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

7-9: 🛠️ Refactor suggestion

Validate Inventory Existence and Account ID.

After obtaining accountId and fetching the inventory, consider verifying that the inventory is neither null nor undefined. If the inventory is not found, an error response should be returned to avoid null pointer operations upon subsequent usage of inventory.
+ if (!inventory) {
+   return res.status(404).json({ error: "Inventory not found" });
+ }
src/routes/api.ts (1)

47-47: Ensure route naming consistency and consider a separate GET endpoint if necessary.

You added a POST route at /playerSkills.php. Usually, RESTful routes avoid .php suffixes. If possible, consider renaming to /playerSkills for clarity.

The AI-generated summary mentions a GET route for /playerSkills.php, but the code only shows a POST route. Confirm whether you actually need a GET route and adjust accordingly.

Also applies to: 134-134
src/types/inventoryTypes/inventoryTypes.ts (1)

822-822: Verify skill naming consistency and usage.

LPP_DRIFTER implies parity with similarly named constants (LPP_SPACE, etc.). Ensure that all references, especially in front-end or other services, use the correct name. Typos or mismatches in naming can cause skill references to break.

✅ Verification successful

Let me gather more information about related skill naming patterns to ensure consistency.

LPP_DRIFTER follows consistent naming pattern and usage

The naming convention is consistent across the codebase:

LPP_ prefix is used consistently for this category of constants (e.g., LPP_SPACE, LPP_DRIFTER)

Related skills follow the LPS_ prefix pattern (e.g., LPS_DRIFT_COMBAT, LPS_DRIFT_RIDING)

The term "DRIFTER" is used consistently throughout the codebase in various related contexts (inventory types, loadouts, equipment)
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:
#!/bin/bash
# Searching for references of 'LPP_DRIFTER' and verifying consistency:
rg -A 3 "LPP_DRIFTER"
Length of output: 720

Script:
#!/bin/bash
# Search for similar LPP_ patterns to verify naming convention
rg "LPP_" -A 2

# Also search for any potential typos or variations
rg -i "drifter" -A 2
Length of output: 5708
src/models/inventoryModels/inventoryModel.ts (1)

441-451: Good practice setting default values for skill fields.

These default values help avoid null or undefined fields for new documents. However, if negative (or extremely high) skill values must be prevented, consider adding validation logic either at schema level or inside your controller logic to clamp or reject invalid updates.

**Actionable comments posted: 2** <details> <summary>🧹 Nitpick comments (4)</summary><blockquote> <details> <summary>src/controllers/api/playerSkillsController.ts (3)</summary><blockquote> `1-6`: **Consider adding error handling for imports and environment readiness.** While these imports are straightforward, any failure in reading or missing environment variables (if the services rely on them) might cause unhandled exceptions during initialization. If your `getAccountIdForRequest`, `getInventory`, or other dependencies can throw errors when, for instance, environment variables are misconfigured, consider wrapping those calls in try/catch or dedicated error handling blocks to provide clearer debugging messages. --- `16-23`: **Return robust response details and implement partial success checks.** The current response object is minimal, which is fine for simple client usage. However, you could include additional metadata (like the updated skill pool total) or perform a final confirmation read from the saved inventory to ensure the changes persist. Additionally, if `save()` or other operations fail, ensure the client is notified instead of sending success JSON. --- `26-29`: **Use stricter typing for the request interface.** `Pool` and `Skill` are typed as string, which is flexible but can lead to runtime errors if an invalid key is passed. Consider using a union type or enum for expected values to enforce compile-time checks. ```ts interface IPlayerSkillsRequest { Pool: "LPP_SPACE" | "LPS_PILOTING" | "RegularCredits" | ...; // Example Skill: "LPP_SPACE" | "LPS_PILOTING" | "LPS_GUNNERY" | ...; } ``` </blockquote></details> <details> <summary>src/models/inventoryModels/inventoryModel.ts (1)</summary><blockquote> `1005-1005`: **Initialize `PlayerSkills` with an empty object by default.** Initializing `PlayerSkills` with `{}` ensures newly created inventories have the correct structure. Verify that references to subfields in `PlayerSkills` handle the case where the entire object might be empty, if older documents are missing these fields. </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 1bab76f58b9a0cf8cd37e36ac37c24bccb33eb8a and 101d6435f62a0dad1e63e5cbb963e46c5b10cc19. </details> <details> <summary>📒 Files selected for processing (4)</summary> * `src/controllers/api/playerSkillsController.ts` (1 hunks) * `src/models/inventoryModels/inventoryModel.ts` (2 hunks) * `src/routes/api.ts` (2 hunks) * `src/types/inventoryTypes/inventoryTypes.ts` (1 hunks) </details> <details> <summary>🔇 Additional comments (4)</summary><blockquote> <details> <summary>src/controllers/api/playerSkillsController.ts (1)</summary> `7-9`: _:hammer_and_wrench: Refactor suggestion_ **Validate Inventory Existence and Account ID.** After obtaining `accountId` and fetching the inventory, consider verifying that the inventory is neither `null` nor `undefined`. If the inventory is not found, an error response should be returned to avoid null pointer operations upon subsequent usage of `inventory`. ```diff + if (!inventory) { + return res.status(404).json({ error: "Inventory not found" }); + } ``` </details> <details> <summary>src/routes/api.ts (1)</summary> `47-47`: **Ensure route naming consistency and consider a separate GET endpoint if necessary.** 1) You added a `POST` route at `/playerSkills.php`. Usually, RESTful routes avoid `.php` suffixes. If possible, consider renaming to `/playerSkills` for clarity. 2) The AI-generated summary mentions a GET route for `/playerSkills.php`, but the code only shows a POST route. Confirm whether you actually need a GET route and adjust accordingly. Also applies to: 134-134 </details> <details> <summary>src/types/inventoryTypes/inventoryTypes.ts (1)</summary> `822-822`: **Verify skill naming consistency and usage.** `LPP_DRIFTER` implies parity with similarly named constants (`LPP_SPACE`, etc.). Ensure that all references, especially in front-end or other services, use the correct name. Typos or mismatches in naming can cause skill references to break. <details> <summary>✅ Verification successful</summary> Let me gather more information about related skill naming patterns to ensure consistency. --- **`LPP_DRIFTER` follows consistent naming pattern and usage** The naming convention is consistent across the codebase: - `LPP_` prefix is used consistently for this category of constants (e.g., `LPP_SPACE`, `LPP_DRIFTER`) - Related skills follow the `LPS_` prefix pattern (e.g., `LPS_DRIFT_COMBAT`, `LPS_DRIFT_RIDING`) - The term "DRIFTER" is used consistently throughout the codebase in various related contexts (inventory types, loadouts, equipment) </details> <details> <summary>🏁 Scripts executed</summary> The following scripts were executed for the analysis: Script: ```shell #!/bin/bash # Searching for references of 'LPP_DRIFTER' and verifying consistency: rg -A 3 "LPP_DRIFTER" ``` Length of output: 720 --- Script: ```shell #!/bin/bash # Search for similar LPP_ patterns to verify naming convention rg "LPP_" -A 2 # Also search for any potential typos or variations rg -i "drifter" -A 2 ``` Length of output: 5708 </details> </details> <details> <summary>src/models/inventoryModels/inventoryModel.ts (1)</summary> `441-451`: **Good practice setting default values for skill fields.** These default values help avoid null or undefined fields for new documents. However, if negative (or extremely high) skill values must be prevented, consider adding validation logic either at schema level or inside your controller logic to clamp or reject invalid updates. </details> </blockquote></details> </details>

src/controllers/api/playerSkillsController.ts

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

2025-01-05 04:21:25 -08:00

⚠️ Potential issue

Check sufficient pool balance and skill validity.

There's no validation to confirm the chosen pool (e.g., RegularCredits vs. FusionPoints) has enough funds to cover the cost. If cost exceeds the balance, this logic would set the pool to a negative value.
Also ensure that request.Skill is valid and actually exists in inventory.PlayerSkills, to avoid writing to an undefined key or misusing the cast as keyof IPlayerSkills.

 const oldRank: number = inventory.PlayerSkills[request.Skill as keyof IPlayerSkills];
 const cost = (1 << oldRank) * 1000;
- inventory.PlayerSkills[request.Pool as keyof IPlayerSkills] -= cost;
- inventory.PlayerSkills[request.Skill as keyof IPlayerSkills]++;
+ // 1) Validate pool balance
+ const poolKey = request.Pool as keyof IPlayerSkills;
+ if (inventory.PlayerSkills[poolKey] < cost) {
+   return res.status(400).json({ error: "Insufficient balance in pool" });
+ }
+ // 2) Validate skill
+ const skillKey = request.Skill as keyof IPlayerSkills;
+ if (typeof inventory.PlayerSkills[skillKey] !== "number") {
+   return res.status(400).json({ error: "Invalid skill name" });
+ }
+ 
+ // Safe to update
+ inventory.PlayerSkills[poolKey] -= cost;
+ inventory.PlayerSkills[skillKey]++;

Committable suggestion skipped: line range outside the PR's diff.

_:warning: Potential issue_ **Check sufficient pool balance and skill validity.** 1) There's no validation to confirm the chosen pool (e.g., `RegularCredits` vs. `FusionPoints`) has enough funds to cover the `cost`. If `cost` exceeds the balance, this logic would set the pool to a negative value. 2) Also ensure that `request.Skill` is valid and actually exists in `inventory.PlayerSkills`, to avoid writing to an undefined key or misusing the cast `as keyof IPlayerSkills`. ```diff const oldRank: number = inventory.PlayerSkills[request.Skill as keyof IPlayerSkills]; const cost = (1 << oldRank) * 1000; - inventory.PlayerSkills[request.Pool as keyof IPlayerSkills] -= cost; - inventory.PlayerSkills[request.Skill as keyof IPlayerSkills]++; + // 1) Validate pool balance + const poolKey = request.Pool as keyof IPlayerSkills; + if (inventory.PlayerSkills[poolKey] < cost) { + return res.status(400).json({ error: "Insufficient balance in pool" }); + } + // 2) Validate skill + const skillKey = request.Skill as keyof IPlayerSkills; + if (typeof inventory.PlayerSkills[skillKey] !== "number") { + return res.status(400).json({ error: "Invalid skill name" }); + } + + // Safe to update + inventory.PlayerSkills[poolKey] -= cost; + inventory.PlayerSkills[skillKey]++; ``` > Committable suggestion skipped: line range outside the PR's diff.

src/controllers/api/playerSkillsController.ts

						
				@ -0,0 +8,4 @@

				    const accountId = await getAccountIdForRequest(req);

				    const inventory = await getInventory(accountId);

				    const request = getJSONfromString(String(req.body)) as IPlayerSkillsRequest;

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

2025-01-05 04:21:25 -08:00

⚠️ Potential issue

Gracefully handle malformed JSON requests.

Calling getJSONfromString directly on req.body might throw runtime errors if the request body is malformed or non-JSON. Consider guarding against null or invalid input, returning an error response if parsing fails or if required fields are missing.

- const request = getJSONfromString(String(req.body)) as IPlayerSkillsRequest;
+ let request: IPlayerSkillsRequest;
+ try {
+   request = getJSONfromString(String(req.body)) as IPlayerSkillsRequest;
+ } catch (err) {
+   return res.status(400).json({ error: "Invalid JSON format." });
+ }

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

    let request: IPlayerSkillsRequest;
    try {
      request = getJSONfromString(String(req.body)) as IPlayerSkillsRequest;
    } catch (err) {
      return res.status(400).json({ error: "Invalid JSON format." });
    }

_:warning: Potential issue_ **Gracefully handle malformed JSON requests.** Calling `getJSONfromString` directly on `req.body` might throw runtime errors if the request body is malformed or non-JSON. Consider guarding against `null` or invalid input, returning an error response if parsing fails or if required fields are missing. ```diff - const request = getJSONfromString(String(req.body)) as IPlayerSkillsRequest; + let request: IPlayerSkillsRequest; + try { + request = getJSONfromString(String(req.body)) as IPlayerSkillsRequest; + } catch (err) { + return res.status(400).json({ error: "Invalid JSON format." }); + } ```  <details> <summary>📝 Committable suggestion</summary> > ‼️ **IMPORTANT** > Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements. `````suggestion let request: IPlayerSkillsRequest; try { request = getJSONfromString(String(req.body)) as IPlayerSkillsRequest; } catch (err) { return res.status(400).json({ error: "Invalid JSON format." }); } ````` </details>

VampireKitten commented

2025-01-05 08:34:42 -08:00

(Migrated from github.com)

Is the cost not matching up with Live for Duviri intentional? While const cost = (1 << oldRank) * 1000; is correct for Railjack, going from 1 to 512, Drifter Intrinsics scale instead from 20 to 255, with... seemingly no apparent formula?

Is the cost not matching up with Live for Duviri intentional? While `const cost = (1 << oldRank) * 1000;` is correct for Railjack, going from 1 to 512, Drifter Intrinsics scale instead from 20 to 255, with... seemingly no apparent formula? ![image](https://github.com/user-attachments/assets/4cc82f09-9b1e-42ae-bff7-75b1c0919c3f)

Sainan commented

2025-01-05 14:14:04 -08:00

Oh, didn't realise it was a different formula.

coderabbitai[bot] (Migrated from github.com) reviewed 2025-01-05 14:37:51 -08:00

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

Actionable comments posted: 2

🧹 Nitpick comments (3)

src/controllers/api/playerSkillsController.ts (3)

18-23: Consider adding more details to the response.

The response could be more informative by including:

The cost calculation method used (Drifter vs Railjack)
The remaining pool balance

     res.json({
         Pool: request.Pool,
         PoolInc: -cost,
         Skill: request.Skill,
-        Rank: oldRank + 1
+        Rank: oldRank + 1,
+        RemainingBalance: inventory.PlayerSkills[request.Pool as keyof IPlayerSkills],
+        CostType: request.Pool === "LPP_DRIFTER" ? "Drifter" : "Railjack"
     });

26-29: Add documentation and type constraints to the interface.

The interface could be more self-documenting and type-safe.

+/**
+ * Request structure for upgrading player skills
+ * @property {("LPP_DRIFTER"|"REGULAR_CREDITS"|string)} Pool - The resource pool to deduct costs from
+ * @property {string} Skill - The skill to upgrade
+ */
 interface IPlayerSkillsRequest {
-    Pool: string;
+    Pool: "LPP_DRIFTER" | "REGULAR_CREDITS" | string;
     Skill: string;
 }

31-31: Document and protect the drifterCosts array.

Add documentation explaining the cost progression and make the array immutable.

-const drifterCosts = [20, 25, 30, 45, 65, 90, 125, 160, 205, 255];
+/**
+ * Cost progression for Drifter Intrinsics upgrades
+ * Each index represents the cost multiplier (in thousands) for upgrading from that rank
+ * e.g., upgrading from rank 0 costs 20,000, rank 1 costs 25,000, etc.
+ */
+const drifterCosts = Object.freeze([20, 25, 30, 45, 65, 90, 125, 160, 205, 255]);

📜 Review details

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

📥 Commits

Reviewing files that changed from the base of the PR and between 101d6435f6 and b1a9b3fa43.

📒 Files selected for processing (1)

src/controllers/api/playerSkillsController.ts (1 hunks)

🔇 Additional comments (1)

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

1-6: LGTM! Imports are clean and well-organized.

All necessary dependencies are properly imported using consistent path aliasing.

**Actionable comments posted: 2** <details> <summary>🧹 Nitpick comments (3)</summary><blockquote> <details> <summary>src/controllers/api/playerSkillsController.ts (3)</summary><blockquote> `18-23`: **Consider adding more details to the response.** The response could be more informative by including: 1. The cost calculation method used (Drifter vs Railjack) 2. The remaining pool balance ```diff res.json({ Pool: request.Pool, PoolInc: -cost, Skill: request.Skill, - Rank: oldRank + 1 + Rank: oldRank + 1, + RemainingBalance: inventory.PlayerSkills[request.Pool as keyof IPlayerSkills], + CostType: request.Pool === "LPP_DRIFTER" ? "Drifter" : "Railjack" }); ``` --- `26-29`: **Add documentation and type constraints to the interface.** The interface could be more self-documenting and type-safe. ```diff +/** + * Request structure for upgrading player skills + * @property {("LPP_DRIFTER"|"REGULAR_CREDITS"|string)} Pool - The resource pool to deduct costs from + * @property {string} Skill - The skill to upgrade + */ interface IPlayerSkillsRequest { - Pool: string; + Pool: "LPP_DRIFTER" | "REGULAR_CREDITS" | string; Skill: string; } ``` --- `31-31`: **Document and protect the drifterCosts array.** Add documentation explaining the cost progression and make the array immutable. ```diff -const drifterCosts = [20, 25, 30, 45, 65, 90, 125, 160, 205, 255]; +/** + * Cost progression for Drifter Intrinsics upgrades + * Each index represents the cost multiplier (in thousands) for upgrading from that rank + * e.g., upgrading from rank 0 costs 20,000, rank 1 costs 25,000, etc. + */ +const drifterCosts = Object.freeze([20, 25, 30, 45, 65, 90, 125, 160, 205, 255]); ``` </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 101d6435f62a0dad1e63e5cbb963e46c5b10cc19 and b1a9b3fa431b6e39138610140e88992fd6247ddd. </details> <details> <summary>📒 Files selected for processing (1)</summary> * `src/controllers/api/playerSkillsController.ts` (1 hunks) </details> <details> <summary>🔇 Additional comments (1)</summary><blockquote> <details> <summary>src/controllers/api/playerSkillsController.ts (1)</summary> `1-6`: **LGTM! Imports are clean and well-organized.** All necessary dependencies are properly imported using consistent path aliasing. </details> </blockquote></details> </details>

src/controllers/api/playerSkillsController.ts

						
				@ -0,0 +10,4 @@

				    const request = getJSONfromString(String(req.body)) as IPlayerSkillsRequest;

				    const oldRank: number = inventory.PlayerSkills[request.Skill as keyof IPlayerSkills];

				    const cost = (request.Pool == "LPP_DRIFTER" ? drifterCosts[oldRank] : 1 << oldRank) * 1000;

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

2025-01-05 14:37:50 -08:00

⚠️ Potential issue

Add bounds checking for drifterCosts array access.

When accessing drifterCosts[oldRank], there's no validation to ensure oldRank is within array bounds (0-9). This could lead to undefined behavior if the rank exceeds the maximum level.

-    const cost = (request.Pool == "LPP_DRIFTER" ? drifterCosts[oldRank] : 1 << oldRank) * 1000;
+    if (request.Pool === "LPP_DRIFTER" && (oldRank < 0 || oldRank >= drifterCosts.length)) {
+        return res.status(400).json({ error: `Invalid rank ${oldRank}. Maximum rank is ${drifterCosts.length - 1}` });
+    }
+    const cost = (request.Pool === "LPP_DRIFTER" ? drifterCosts[oldRank] : 1 << oldRank) * 1000;

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

    if (request.Pool === "LPP_DRIFTER" && (oldRank < 0 || oldRank >= drifterCosts.length)) {
        return res.status(400).json({ error: `Invalid rank ${oldRank}. Maximum rank is ${drifterCosts.length - 1}` });
    }
    const oldRank: number = inventory.PlayerSkills[request.Skill as keyof IPlayerSkills];
    const cost = (request.Pool === "LPP_DRIFTER" ? drifterCosts[oldRank] : 1 << oldRank) * 1000;

_:warning: Potential issue_ **Add bounds checking for drifterCosts array access.** When accessing `drifterCosts[oldRank]`, there's no validation to ensure `oldRank` is within array bounds (0-9). This could lead to undefined behavior if the rank exceeds the maximum level. ```diff - const cost = (request.Pool == "LPP_DRIFTER" ? drifterCosts[oldRank] : 1 << oldRank) * 1000; + if (request.Pool === "LPP_DRIFTER" && (oldRank < 0 || oldRank >= drifterCosts.length)) { + return res.status(400).json({ error: `Invalid rank ${oldRank}. Maximum rank is ${drifterCosts.length - 1}` }); + } + const cost = (request.Pool === "LPP_DRIFTER" ? drifterCosts[oldRank] : 1 << oldRank) * 1000; ```  <details> <summary>📝 Committable suggestion</summary> > ‼️ **IMPORTANT** > Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements. `````suggestion if (request.Pool === "LPP_DRIFTER" && (oldRank < 0 || oldRank >= drifterCosts.length)) { return res.status(400).json({ error: `Invalid rank ${oldRank}. Maximum rank is ${drifterCosts.length - 1}` }); } const oldRank: number = inventory.PlayerSkills[request.Skill as keyof IPlayerSkills]; const cost = (request.Pool === "LPP_DRIFTER" ? drifterCosts[oldRank] : 1 << oldRank) * 1000; ````` </details>

src/controllers/api/playerSkillsController.ts

						
				@ -0,0 +12,4 @@

				    const oldRank: number = inventory.PlayerSkills[request.Skill as keyof IPlayerSkills];

				    const cost = (request.Pool == "LPP_DRIFTER" ? drifterCosts[oldRank] : 1 << oldRank) * 1000;

				    inventory.PlayerSkills[request.Pool as keyof IPlayerSkills] -= cost;

				    inventory.PlayerSkills[request.Skill as keyof IPlayerSkills]++;

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

2025-01-05 14:37:50 -08:00

⚠️ Potential issue

Prevent negative costs and add maximum rank validation.

The current implementation lacks validation for:

Maximum rank for non-Drifter skills (Railjack goes up to rank 9)
Negative costs that could occur from integer overflow

-    const cost = (request.Pool === "LPP_DRIFTER" ? drifterCosts[oldRank] : 1 << oldRank) * 1000;
-    inventory.PlayerSkills[request.Pool as keyof IPlayerSkills] -= cost;
-    inventory.PlayerSkills[request.Skill as keyof IPlayerSkills]++;
+    // Maximum rank for non-Drifter skills
+    if (request.Pool !== "LPP_DRIFTER" && oldRank >= 9) {
+        return res.status(400).json({ error: "Maximum rank reached" });
+    }
+    
+    const baseCost = request.Pool === "LPP_DRIFTER" ? drifterCosts[oldRank] : 1 << oldRank;
+    const cost = baseCost * 1000;
+    
+    // Prevent integer overflow leading to negative costs
+    if (cost <= 0) {
+        return res.status(400).json({ error: "Invalid cost calculation" });
+    }
+    
+    const poolKey = request.Pool as keyof IPlayerSkills;
+    const skillKey = request.Skill as keyof IPlayerSkills;
+    
+    inventory.PlayerSkills[poolKey] -= cost;
+    inventory.PlayerSkills[skillKey]++;

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

    // Maximum rank for non-Drifter skills
    if (request.Pool !== "LPP_DRIFTER" && oldRank >= 9) {
        return res.status(400).json({ error: "Maximum rank reached" });
    }
    
    const baseCost = request.Pool === "LPP_DRIFTER" ? drifterCosts[oldRank] : 1 << oldRank;
    const cost = baseCost * 1000;
    
    // Prevent integer overflow leading to negative costs
    if (cost <= 0) {
        return res.status(400).json({ error: "Invalid cost calculation" });
    }
    
    const poolKey = request.Pool as keyof IPlayerSkills;
    const skillKey = request.Skill as keyof IPlayerSkills;
    
    inventory.PlayerSkills[poolKey] -= cost;
    inventory.PlayerSkills[skillKey]++;

_:warning: Potential issue_ **Prevent negative costs and add maximum rank validation.** The current implementation lacks validation for: 1. Maximum rank for non-Drifter skills (Railjack goes up to rank 9) 2. Negative costs that could occur from integer overflow ```diff - const cost = (request.Pool === "LPP_DRIFTER" ? drifterCosts[oldRank] : 1 << oldRank) * 1000; - inventory.PlayerSkills[request.Pool as keyof IPlayerSkills] -= cost; - inventory.PlayerSkills[request.Skill as keyof IPlayerSkills]++; + // Maximum rank for non-Drifter skills + if (request.Pool !== "LPP_DRIFTER" && oldRank >= 9) { + return res.status(400).json({ error: "Maximum rank reached" }); + } + + const baseCost = request.Pool === "LPP_DRIFTER" ? drifterCosts[oldRank] : 1 << oldRank; + const cost = baseCost * 1000; + + // Prevent integer overflow leading to negative costs + if (cost <= 0) { + return res.status(400).json({ error: "Invalid cost calculation" }); + } + + const poolKey = request.Pool as keyof IPlayerSkills; + const skillKey = request.Skill as keyof IPlayerSkills; + + inventory.PlayerSkills[poolKey] -= cost; + inventory.PlayerSkills[skillKey]++; ```  <details> <summary>📝 Committable suggestion</summary> > ‼️ **IMPORTANT** > Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements. `````suggestion // Maximum rank for non-Drifter skills if (request.Pool !== "LPP_DRIFTER" && oldRank >= 9) { return res.status(400).json({ error: "Maximum rank reached" }); } const baseCost = request.Pool === "LPP_DRIFTER" ? drifterCosts[oldRank] : 1 << oldRank; const cost = baseCost * 1000; // Prevent integer overflow leading to negative costs if (cost <= 0) { return res.status(400).json({ error: "Invalid cost calculation" }); } const poolKey = request.Pool as keyof IPlayerSkills; const skillKey = request.Skill as keyof IPlayerSkills; inventory.PlayerSkills[poolKey] -= cost; inventory.PlayerSkills[skillKey]++; ````` </details>

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: OpenWF/SpaceNinjaServer#725