Fix issue #336: Mirror #340
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
WalkthroughA new combat mechanism called "mirror" has been introduced to the game's battle system. This feature allows a character to reflect all their current negative effects onto another target. The implementation involves adding a new function in the combat tags library and defining a corresponding type in the types module. The mirror functionality checks for existing negative effects and transfers them to the specified target, potentially changing the dynamics of battle interactions. Changes
Sequence DiagramsequenceDiagram
participant Caster
participant Target
participant CombatSystem
Caster->>CombatSystem: Trigger mirror effect
CombatSystem->>Caster: Identify negative effects
CombatSystem->>Target: Apply mirrored effects
CombatSystem-->>Caster: Return mirror result message
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
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? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
app/src/libs/combat/types.ts (1)
332-338: Consider adding power and calculation attributes for consistency.The
MirrorTagdefinition is missingpowerandcalculationattributes that are present in most other effect tags. These attributes could be useful for:
- Controlling the success rate of mirroring effects (via power)
- Determining how the mirroring calculation is performed (via calculation)
export const MirrorTag = z.object({ ...BaseAttributes, type: z.literal("mirror").default("mirror"), description: msg("Applies all negative effects from the caster to the target"), target: z.enum(BaseTagTargets).optional().default("INHERIT"), direction: type("defence"), + power: z.coerce.number().min(1).max(100).default(100), + calculation: z.enum(["static"]).default("static"), });app/src/libs/combat/tags.ts (1)
1594-1598: Improve feedback message clarity.The current message could be clearer about what happened during the mirroring process.
return getInfo( target, effect, - `all negative effects on ${target.username} are mirrored to their opponent`, + `all negative effects from ${target.username} are copied to their opponent`, );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/src/libs/combat/tags.ts(1 hunks)app/src/libs/combat/types.ts(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: analyze
- GitHub Check: Analyze (javascript)
🔇 Additional comments (2)
app/src/libs/combat/types.ts (1)
686-686: LGTM!The
MirrorTagis correctly added to theAllTagsunion type, maintaining consistency with other tag definitions.app/src/libs/combat/tags.ts (1)
1549-1599: Verify the impact on battle balance.The mirror functionality is a powerful mechanic that could significantly impact battle balance. Consider:
- Testing with various combinations of negative effects
- Verifying interaction with other mechanics like cleanse/clear
- Checking for potential infinite loops with multiple mirror effects
✅ Verification successful
Mirror implementation has appropriate safety checks
The mirror functionality is well-balanced with built-in safeguards:
- Prevents infinite loops via isNew/castThisRound flags
- Each mirrored effect gets unique ID
- Subject to normal cleanse/clear mechanics
- One-time copy without cascading effects
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for potential interactions with mirror effects echo "Searching for cleanse/clear interactions with mirror..." rg -A 5 "cleanse|clear" --type ts echo "Searching for other effects that might create loops..." rg -A 5 "isNegativeUserEffect" --type tsLength of output: 37167
| const newEffect = structuredClone(mirroredEffect) as unknown as UserEffect; | ||
| // eslint-disable-next-line @typescript-eslint/no-unsafe-argument, @typescript-eslint/no-unsafe-assignment, @typescript-eslint/no-unsafe-member-access, @typescript-eslint/no-unsafe-call | ||
| usersEffects.push(newEffect); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Optimize memory usage and improve type safety.
The code uses structuredClone for deep cloning, which can be memory intensive for large objects. Additionally, the type assertions can be removed with proper typing.
- const newEffect = structuredClone(mirroredEffect) as unknown as UserEffect;
- usersEffects.push(newEffect);
+ const newEffect = { ...mirroredEffect } as UserEffect;
+ try {
+ usersEffects.push(newEffect);
+ } catch (error) {
+ console.error('Failed to add mirrored effect:', error);
+ return {
+ txt: `Failed to mirror effects: ${error.message}`,
+ color: "red",
+ };
+ }Committable suggestion skipped: line range outside the PR's diff.
| usersEffects: UserEffect[], | ||
| target: BattleUserState, | ||
| ) => { | ||
| if (!effect.isNew && !effect.castThisRound) { |
There was a problem hiding this comment.
Add validation for effect.userId.
The function assumes effect.userId exists when filtering negative effects, but it's not validated. This could lead to runtime errors if the effect doesn't have a userId.
if (!effect.isNew && !effect.castThisRound) {
+ if (!effect.userId) {
+ return {
+ txt: `Failed to mirror effects: Invalid effect userId`,
+ color: "red",
+ };
+ }
// Find all negative effects on the casterCommittable suggestion skipped: line range outside the PR's diff.
app/src/libs/combat/tags.ts
Outdated
| const mirroredEffect = { | ||
| type: negEffect.type, | ||
| id: nanoid(), | ||
| targetId: effect.targetId, | ||
| userId: effect.userId, | ||
| isNew: true, | ||
| castThisRound: true, | ||
| createdRound: effect.createdRound, | ||
| power: negEffect.power ?? 0, | ||
| powerPerLevel: negEffect.powerPerLevel ?? 0, | ||
| rounds: negEffect.rounds ?? 0, | ||
| direction: negEffect.direction ?? "offence", | ||
| calculation: negEffect.calculation ?? "static", | ||
| description: negEffect.description ?? "", | ||
| target: negEffect.target ?? "INHERIT", | ||
| friendlyFire: negEffect.friendlyFire, | ||
| staticAssetPath: negEffect.staticAssetPath ?? "", | ||
| staticAnimation: negEffect.staticAnimation ?? "", | ||
| appearAnimation: negEffect.appearAnimation ?? "", | ||
| disappearAnimation: negEffect.disappearAnimation ?? "", | ||
| timeTracker: negEffect.timeTracker, | ||
| } as const; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve type safety by defining a proper interface.
The code uses unsafe type assertions and eslint-disable comments. This can be improved by defining a proper interface for the mirrored effect.
+ interface MirroredEffect {
+ type: string;
+ id: string;
+ targetId: string;
+ userId: string;
+ isNew: boolean;
+ castThisRound: boolean;
+ createdRound: number;
+ power: number;
+ powerPerLevel: number;
+ rounds: number;
+ direction: string;
+ calculation: string;
+ description: string;
+ target: string;
+ friendlyFire?: string;
+ staticAssetPath: string;
+ staticAnimation: string;
+ appearAnimation: string;
+ disappearAnimation: string;
+ timeTracker?: Record<string, number>;
+ }
- const mirroredEffect = {
+ const mirroredEffect: MirroredEffect = {Committable suggestion skipped: line range outside the PR's diff.
|
./src/libs/combat/tags.ts:1558:36 1556 | // Find all negative effects on the caster
|
|
./src/libs/combat/tags.ts:1558:36 1556 | // Find all negative effects on the caster
|
|
Overview of Changes: Status: ✅ Successfully Resolved
The modifications appear to be complete and properly resolve the original type error while maintaining the intended behavior of the mirror tag system. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
app/src/libs/combat/tags.ts (1)
1550-1554: Add explicit return type annotation.The function signature should include the return type for better type safety and documentation.
export const mirror = ( effect: UserEffect, usersEffects: UserEffect[], target: BattleUserState, -) => { +): ActionEffect | undefined => {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/src/libs/combat/tags.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: analyze
- GitHub Check: Analyze (javascript)
🔇 Additional comments (2)
app/src/libs/combat/tags.ts (2)
1555-1559: LGTM! Core logic is well implemented.The implementation correctly filters negative effects on the caster using appropriate type-safe conditions.
1564-1590: Previous review comment about type safety and memory usage is still applicable.The code still uses unsafe type assertions and memory-intensive deep cloning.
| return getInfo( | ||
| target, | ||
| effect, | ||
| `all negative effects on ${target.username} are mirrored to their opponent`, | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for edge cases.
The function should handle potential edge cases:
- Empty usersEffects array
- Invalid effect properties
- Failed effect application
return getInfo(
target,
effect,
- `all negative effects on ${target.username} are mirrored to their opponent`,
+ negativeEffects.length > 0
+ ? `all negative effects on ${target.username} are mirrored to their opponent`
+ : `no negative effects found to mirror`,
);📝 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.
| return getInfo( | |
| target, | |
| effect, | |
| `all negative effects on ${target.username} are mirrored to their opponent`, | |
| ); | |
| return getInfo( | |
| target, | |
| effect, | |
| negativeEffects.length > 0 | |
| ? `all negative effects on ${target.username} are mirrored to their opponent` | |
| : `no negative effects found to mirror`, | |
| ); |
📦 Next.js Bundle Analysis for tnrThis analysis was generated by the Next.js Bundle Analysis action. 🤖 🎉 Global Bundle Size Decreased
DetailsThe global bundle is the javascript bundle that loads alongside every page. It is in its own category because its impact is much higher - an increase to its size means that every page on your website loads slower, and a decrease means every page loads faster. Any third party scripts you have added directly to your app using the If you want further insight into what is behind the changes, give @next/bundle-analyzer a try! |
|
@MathiasGruber Similar to the Copy tag, the logic looks nice but it's just not working. |
theeneon
added
Medium Priority
Low Priority
and removed
Low Priority
Medium Priority
labels
Pull Request
Please fill: what was implemented, why, possibly include screenshots, are any of the changes breaking, etc.
License
By making this pull request, I confirm that I have the right to waive copyright and related rights to my contribution, and agree that all copyright and related rights in my contributions are waived, and I acknowledge that the Studie-Tech ApS organization has the copyright to use and modify my contribution for perpetuity.
Summary by CodeRabbit
New Features
Improvements