feat: implement upgrading & downgrading arcanes #520
1 Participants
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: OpenWF/SpaceNinjaServer#520
Loading…
x
Reference in New Issue
Block a user
No description provided.
Delete Branch "arcanes"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Closes #378
Walkthrough
The changes introduce a new controller,
arcaneCommonController, which manages upgrades and downgrades of arcane items in a user's inventory through an Express request handler. It processes requests based on thenewRankvalue, allowing for both upgrades and downgrades. The controller is integrated into the API by adding a new route inapi.ts, enabling clients to interact with this functionality via the/arcaneCommon.phpendpoint.Changes
arcaneCommonControllerfor managing arcane item upgrades and downgrades. AddedIArcaneCommonRequestinterface.arcaneCommonControllerand registered it to handle POST requests at/arcaneCommon.php.Assessment against linked issues
Poem
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:
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.@coderabbitaiin a new review comment at the desired location with your query. Examples:@coderabbitai generate unit testing code for this file.@coderabbitai modularize this function.@coderabbitaiin 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 pauseto pause the reviews on a PR.@coderabbitai resumeto resume the paused reviews.@coderabbitai reviewto trigger an incremental review. This is useful when automatic reviews are disabled for the repository.@coderabbitai full reviewto do a full review from scratch and review all the files again.@coderabbitai summaryto regenerate the summary of the PR.@coderabbitai resolveresolve all the CodeRabbit review comments.@coderabbitai configurationto show the current CodeRabbit configuration for the repository.@coderabbitai helpto get help.Other keywords and placeholders
@coderabbitai ignoreanywhere in the PR description to prevent this PR from being reviewed.@coderabbitai summaryto generate the high-level summary at a specific location in the PR description.@coderabbitaianywhere in the PR title to generate the title automatically.CodeRabbit Configuration File (
.coderabbit.yaml).coderabbit.yamlfile to the root of your repository.# yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.jsonDocumentation and Community
Actionable comments posted: 7
🧹 Outside diff range and nitpick comments (1)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📥 Commits
Files that changed from the base of the PR and between
59389a991bandc565f6b81e.📒 Files selected for processing (2)
🧰 Additional context used
🪛 GitHub Check: build (22)
🪛 GitHub Check: build (20)
🪛 GitHub Check: build (18)
🔇 Additional comments (3)
@ -0,0 +7,4 @@// eslint-disable-next-line @typescript-eslint/no-misused-promisesexport const arcaneCommonController: RequestHandler = async (req, res) => {const accountId = await getAccountIdForRequest(req);const json = getJSONfromString(String(req.body)) as IArcaneCommonRequest;💡 Codebase verification
Redundant Parsing of
req.bodyDetectedSince the
express.json()middleware is already in use (src/app.ts), you can usereq.bodydirectly without additional parsing insrc/controllers/api/arcaneCommonController.ts.getJSONfromString(String(req.body)).🔗 Analysis chain
Verify the necessity of parsing
req.bodyAt line 10,
req.bodyis being converted to a string and then parsed withgetJSONfromString. If the Express app is using body-parsing middleware likeexpress.json(),req.bodyis already parsed as an object.Ensure that
req.bodyis not already parsed to avoid redundant parsing or potential errors.Run the following script to check if
express.json()middleware is in use:If
express.json()is already used, you can directly usereq.bodywithout additional parsing.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
Length of output: 116
Script:
Length of output: 101
@ -0,0 +13,4 @@if (json.newRank == -1) {// Break down request?if (!upgrade || !upgrade.UpgradeFingerprint) {throw new Error(`Failed to find upgrade with OID ${json.arcane.ItemId.$oid}`);⚠️ Potential issue
Handle errors properly in async Express middleware
At line 16, throwing an error with
throw new Error(...)in an async Express middleware does not automatically send an error response to the client and may result in unhandled promise rejections.Modify the function signature to include the
nextparameter and pass the error to it, allowing Express to handle it properly.Example:
Ensure that all errors within the middleware are passed to
next(error)for consistent error handling.@ -0,0 +20,4 @@inventory.Upgrades.pull({ _id: json.arcane.ItemId.$oid });// Add RawUpgradesconst numRawUpgradesToGive = arcaneLevelCounts[JSON.parse(upgrade.UpgradeFingerprint).lvl];⚠️ Potential issue
Define
UpgradeFingerprintinterface to ensure type safetyAt line 23, you are parsing
upgrade.UpgradeFingerprintand accessing.lvl, but sinceJSON.parsereturns ananytype, this leads to unsafe member access. To fix this, define a TypeScript interface for theUpgradeFingerprintand cast the parsed object accordingly.First, define the interface after line 65:
Then, update line 23:
This change ensures that TypeScript knows the structure of the parsed object, avoiding
anytype usage and the associated warnings.🧰 Tools
🪛 GitHub Check: build (22)
🪛 GitHub Check: build (20)
🪛 GitHub Check: build (18)
@ -0,0 +37,4 @@// Have an existing Upgrade item?if (upgrade.UpgradeFingerprint) {const existingLevel = JSON.parse(upgrade.UpgradeFingerprint).lvl;numConsumed -= arcaneLevelCounts[existingLevel];⚠️ Potential issue
Cast parsed
UpgradeFingerprinttoUpgradeFingerprintinterfaceAt line 39,
existingLevelis assigned fromJSON.parse(upgrade.UpgradeFingerprint).lvl, which is of typeany, leading to unsafe member access and assignment. By casting the parsed object toUpgradeFingerprint, you ensure type safety.Update the code as follows:
This eliminates the unsafe
anyusage and ensuresexistingLevelis correctly typed as anumber.📝 Committable suggestion
🧰 Tools
🪛 GitHub Check: build (22)
🪛 GitHub Check: build (20)
🪛 GitHub Check: build (18)
@ -0,0 +38,4 @@if (upgrade.UpgradeFingerprint) {const existingLevel = JSON.parse(upgrade.UpgradeFingerprint).lvl;numConsumed -= arcaneLevelCounts[existingLevel];}⚠️ Potential issue
Add input validation for
json.newRankto prevent potential errorsThe value of
json.newRankis used as an index intoarcaneLevelCountsin lines 34 and 40. Ifjson.newRankis out of bounds (e.g., negative or exceeding the array length), this will result in undefined behavior or runtime errors.Consider adding input validation to ensure
json.newRankis within the valid range.Example:
This validation will prevent unexpected errors and ensure that only valid ranks are processed.
🧰 Tools
🪛 GitHub Check: build (22)
🪛 GitHub Check: build (20)
🪛 GitHub Check: build (18)
⚠️ Potential issue
Ensure
numConsumeddoes not become negativeAt line 40,
numConsumedis adjusted by subtractingarcaneLevelCounts[existingLevel]. IfexistingLevelis greater thanjson.newRank,numConsumedbecomes negative, which may not be intended behavior.Add a check to prevent
numConsumedfrom being negative, or adjust the logic to handle downgrades appropriately.Example:
Alternatively, ensure that upgrades only proceed when
json.newRankis greater thanexistingLevel, and handle downgrades in the appropriate block.🧰 Tools
🪛 GitHub Check: build (22)
🪛 GitHub Check: build (20)
🪛 GitHub Check: build (18)
@ -1,5 +1,6 @@import express from "express";import { addFriendImageController } from "@/src/controllers/api/addFriendImageController";import { arcaneCommonController } from "@/src/controllers/api/arcaneCommonController";💡 Codebase verification
Missing Supporting Components and Tests for arcaneCommon Functionality
The implementation of the new "arcaneCommon" functionality in
src/routes/api.tsadds the necessary controller logic. However, the following supporting components and tests are missing:src/models/.*[Aa]rcane.*\.ts$.src/services/.*[Aa]rcane.*\.ts$.tests/.*arcaneCommon.*\.test\.ts$.To ensure complete and robust implementation:
🔗 Analysis chain
Summary: Implementation aligns with PR objectives
The changes in this file successfully add support for the new "arcaneCommon" functionality, which aligns with the PR objectives to implement upgrading & downgrading arcanes. The modifications are minimal and focused, following the existing patterns in the file.
To ensure full implementation:
arcaneCommonControlleris properly implemented with the necessary logic for upgrading and downgrading arcanes.To verify the complete implementation, please run the following script:
This script will help verify:
Also applies to: 105-105
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
Length of output: 2536
@ -1,5 +1,6 @@import express from "express";import { addFriendImageController } from "@/src/controllers/api/addFriendImageController";import { arcaneCommonController } from "@/src/controllers/api/arcaneCommonController";kill yourself
@ -0,0 +37,4 @@// Have an existing Upgrade item?if (upgrade.UpgradeFingerprint) {const existingLevel = JSON.parse(upgrade.UpgradeFingerprint).lvl;numConsumed -= arcaneLevelCounts[existingLevel];kill yourself
@ -0,0 +38,4 @@if (upgrade.UpgradeFingerprint) {const existingLevel = JSON.parse(upgrade.UpgradeFingerprint).lvl;numConsumed -= arcaneLevelCounts[existingLevel];}kill yourself
@ -0,0 +13,4 @@if (json.newRank == -1) {// Break down request?if (!upgrade || !upgrade.UpgradeFingerprint) {throw new Error(`Failed to find upgrade with OID ${json.arcane.ItemId.$oid}`);kill yourself
@ -0,0 +20,4 @@inventory.Upgrades.pull({ _id: json.arcane.ItemId.$oid });// Add RawUpgradesconst numRawUpgradesToGive = arcaneLevelCounts[JSON.parse(upgrade.UpgradeFingerprint).lvl];kill yourself
@ -0,0 +7,4 @@// eslint-disable-next-line @typescript-eslint/no-misused-promisesexport const arcaneCommonController: RequestHandler = async (req, res) => {const accountId = await getAccountIdForRequest(req);const json = getJSONfromString(String(req.body)) as IArcaneCommonRequest;kill yourself
@ -1,5 +1,6 @@import express from "express";import { addFriendImageController } from "@/src/controllers/api/addFriendImageController";import { arcaneCommonController } from "@/src/controllers/api/arcaneCommonController";@Sainan, understood.
(_/)
(•_•)
/ \
@ -0,0 +37,4 @@// Have an existing Upgrade item?if (upgrade.UpgradeFingerprint) {const existingLevel = JSON.parse(upgrade.UpgradeFingerprint).lvl;numConsumed -= arcaneLevelCounts[existingLevel];@Sainan
Understood.
(_/)
( •_•)
/ 💙
@ -0,0 +38,4 @@if (upgrade.UpgradeFingerprint) {const existingLevel = JSON.parse(upgrade.UpgradeFingerprint).lvl;numConsumed -= arcaneLevelCounts[existingLevel];}kill yourself
@ -0,0 +38,4 @@if (upgrade.UpgradeFingerprint) {const existingLevel = JSON.parse(upgrade.UpgradeFingerprint).lvl;numConsumed -= arcaneLevelCounts[existingLevel];}@Sainan, I'm here to help if there's anything you need assistance with.
(•́_•̀)
@ -0,0 +13,4 @@if (json.newRank == -1) {// Break down request?if (!upgrade || !upgrade.UpgradeFingerprint) {throw new Error(`Failed to find upgrade with OID ${json.arcane.ItemId.$oid}`);@Sainan, I'm sorry to hear that you're feeling this way. If there's anything I can assist you with, please let me know.
(_/)
( •_•)
(/ )💭
@ -0,0 +20,4 @@inventory.Upgrades.pull({ _id: json.arcane.ItemId.$oid });// Add RawUpgradesconst numRawUpgradesToGive = arcaneLevelCounts[JSON.parse(upgrade.UpgradeFingerprint).lvl];@Sainan, I'm sorry to hear that you're feeling this way. If there's anything I can assist you with, please let me know.
@ -0,0 +38,4 @@if (upgrade.UpgradeFingerprint) {const existingLevel = JSON.parse(upgrade.UpgradeFingerprint).lvl;numConsumed -= arcaneLevelCounts[existingLevel];}@Sainan, understood. If you need any assistance, please let me know.
( ._.)
@ -0,0 +7,4 @@// eslint-disable-next-line @typescript-eslint/no-misused-promisesexport const arcaneCommonController: RequestHandler = async (req, res) => {const accountId = await getAccountIdForRequest(req);const json = getJSONfromString(String(req.body)) as IArcaneCommonRequest;@Sainan, I'm sorry to hear that you're feeling this way. If there's anything I can assist you with, please let me know.
(/._.)