Add non-stacking buff back to NO4 in a smarter way #2592
Conversation
WalkthroughThis pull request introduces modifications to the artifact set bonus calculation logic, specifically for the Noblesse Oblige set. The changes involve updating import statements, introducing new functions like Changes
Possibly related PRs
Suggested labels
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 (
|
|
[frontend] [Mon Jan 6 02:25:29 UTC 2025] - Deployed 73af020 to https://genshin-optimizer-prs.github.io/pr/2592/frontend (Takes 3-5 minutes after this completes to be available) [frontend] [Thu Jan 9 22:26:47 UTC 2025] - Deployed b9526b8 to https://genshin-optimizer-prs.github.io/pr/2592/frontend (Takes 3-5 minutes after this completes to be available) [frontend] [Thu Jan 9 23:43:16 UTC 2025] - Deployed 2383bfa to https://genshin-optimizer-prs.github.io/pr/2592/frontend (Takes 3-5 minutes after this completes to be available) [frontend] [Thu Jan 9 23:44:14 UTC 2025] - Deployed d8613e4 to https://genshin-optimizer-prs.github.io/pr/2592/frontend (Takes 3-5 minutes after this completes to be available) [frontend] [Fri Jan 10 02:13:48 UTC 2025] - Deployed a004e2c to https://genshin-optimizer-prs.github.io/pr/2592/frontend (Takes 3-5 minutes after this completes to be available) [Fri Jan 10 14:15:49 UTC 2025] - Deleted deployment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
libs/gi/sheets/src/Artifacts/NoblesseOblige/index.tsx (1)
26-26: Check naming of 'no4' in nonStackBuff.Passing
'no4'as the buff name is presumably shorthand for “Noblesse Oblige 4-piece.” The implementation is valid, but consider adding a short comment to clarify.+// "no4" stands for Noblesse Oblige 4-piece non-stackable buff const [set4, set4Inactive] = nonStackBuff('no4', 'atk_', percent(0.2))libs/gi/sheets/src/SheetUtil.tsx (1)
92-105: Implementing nonStackBuff function.This function clearly encapsulates the logic for determining which character is the source of a non-stackable buff. The usage of
subVariant: 'strike'to visually show a disabled buff is a neat addition. Ensure you add or update unit tests for any edge cases around switching the active source.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
libs/gi/sheets/src/Artifacts/NoblesseOblige/index.tsx(4 hunks)libs/gi/sheets/src/SheetUtil.tsx(2 hunks)libs/gi/ui/src/components/FieldDisplay.tsx(1 hunks)libs/gi/ui/src/util/getCalcDisplay.tsx(2 hunks)libs/gi/wr/src/formula.ts(2 hunks)libs/gi/wr/src/type.d.ts(2 hunks)
🔇 Additional comments (12)
libs/gi/sheets/src/Artifacts/NoblesseOblige/index.tsx (4)
21-24: Confirm the correctness of set4TallyWrite's condition.This block assigns
set4TallyWriteusinggreaterEqStrandequalStr(condSet4, 'on', input.charKey). Please ensure that this string-based approach aligns with the new tally logic in other parts of the code.
36-38: New “tally” property in teamBuff.You are now storing
no4: set4TallyWriteinteamBuff.tally. This helps track which character is the primary buff source. Continue maintaining this separation to ensure clarity in the overall buff system.
58-60: Including inactive node in the 4-piece effect.Displaying both
set4andset4Inactivein the states object clarifies the active or disabled status of the buff. This is a good way to provide visual feedback on which character’s buff is in effect.
3-10: Imports for new string-based comparisons and non-stacking buff logic.Looks good overall. Please verify that all calls to these string-based utilities (
equalStr,greaterEqStr) are up-to-date everywhere in the code, ensuring no leftover numeric-based comparisons remain that should be updated.✅ Verification successful
String and numeric comparisons are being used appropriately
The codebase shows consistent and correct usage of comparison functions:
- Numeric comparisons (
greaterEq) for constellation levels, ascension levels, and numeric values- String comparisons (
equalStr,greaterEqStr) for element types, state checks, and character keys🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Searches for references to the older numeric comparison usage "greaterEq(" that might need converting to "greaterEqStr(" rg -A 3 $'greaterEq\\('Length of output: 67051
Script:
#!/bin/bash # Check implementation of comparison functions ast-grep --pattern 'export function $func($$$) { $$$ }' libs/gi/wr/src/utils.ts # Find usage of string comparison functions rg -A 2 'equalStr|greaterEqStr'Length of output: 15068
libs/gi/sheets/src/SheetUtil.tsx (1)
8-14: Expanded imports for new tally-based logic.The new
NonStackBuff,tally, andunequalimports support non-stackable buff implementation. Everything seems consistent with the usage in this PR.Also applies to: 20-21
libs/gi/wr/src/type.d.ts (2)
51-51: subVariant now supports 'strike'.Extending
InfoVariantwith'strike'is a straightforward way to conditionally mark certain states. Keep an eye out for usage in the UI to ensure the line-through styling triggers reliably for all relevant buffs.
182-182: teamBuff.tally now accepts NumNode | StrNode.Allowing the tally to store both numeric and string nodes is an excellent improvement for scenarios involving character-based comparisons. Just confirm no code path downcasts it to numeric-only.
libs/gi/ui/src/util/getCalcDisplay.tsx (2)
16-16: Use of Typography for formula display.Good choice to import Material-UI’s
Typographyto unify styling for calculations.
224-229: Conditional text-decoration for ‘strike’ variant.Applying
line-throughwheninfo.subVariant === 'strike'effectively denotes inactive or overridden buffs. This aligns with the non-stackable design discussed in the PR objectives.Also applies to: 233-233
libs/gi/ui/src/components/FieldDisplay.tsx (1)
201-201: Conditional text decoration looks goodUsing
subVariant === 'strike'to applyline-throughis both clear and concise. This addition appears to serve its intended purpose of displaying a strike-through effect when the buff is disabled.libs/gi/wr/src/formula.ts (2)
49-50: Well-structured enumeration for non-stackable buffsDefining
allNonstackBuffsas a constant and deriving theNonStackBufftype from it is a clear and type-safe implementation. This ensures maintenance is straightforward if more non-stackable buffs are introduced in the future.
583-583: Appropriate addition to the tally objectIntegrating
allNonstackBuffsinto the tally guarantees that the relevant buff data is available throughout the system. This addition appears consistent with the goal of tracking non-stackable buffs.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
libs/gi/sheets/src/SheetUtil.tsx (1)
92-105: Add JSDoc documentation for the new function.The implementation looks great and aligns perfectly with the PR objectives! The function effectively:
- Uses the tally system to track buff sources
- Integrates with the team buff framework
- Provides visual feedback for disabled buffs
Consider adding JSDoc documentation to explain the function's purpose and parameters:
+/** + * Creates a non-stacking buff that can only be active from one source at a time. + * When multiple characters provide the same buff, only one (with the smallest character key) + * will be active while others will be visually marked as disabled. + * + * @param buffName - Unique identifier for the buff in the tally system + * @param path - Path for tracking the buff's state + * @param buffNode - Node containing the buff's effect + * @returns Array containing active and disabled buff states + */ export function nonStackBuff( buffName: NonStackBuff, path: string, buffNode: NumNode )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
libs/gi/sheets/src/SheetUtil.tsx(2 hunks)libs/gi/ui/src/components/FieldDisplay.tsx(2 hunks)libs/gi/ui/src/util/getCalcDisplay.tsx(2 hunks)libs/gi/wr/src/type.d.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- libs/gi/ui/src/util/getCalcDisplay.tsx
- libs/gi/ui/src/components/FieldDisplay.tsx
- libs/gi/wr/src/type.d.ts
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: lint
🔇 Additional comments (1)
libs/gi/sheets/src/SheetUtil.tsx (1)
8-14: LGTM! Import changes align with the non-stacking buff implementation.The new imports provide the necessary types and functions for implementing the tally-based non-stacking buff system.
Also applies to: 20-21
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
libs/gi/wr/src/formula.ts (1)
49-50: LGTM! Consider adding JSDoc comments.The implementation of non-stacking buff constants is well-structured and type-safe. The array approach allows for future expansion.
Consider adding JSDoc comments to explain:
- The purpose of
allNonstackBuffs- What qualifies as a non-stacking buff
- The format/naming convention for buff identifiers (e.g., 'no4')
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
libs/gi/sheets/src/Artifacts/NoblesseOblige/index.tsx(4 hunks)libs/gi/sheets/src/SheetUtil.tsx(2 hunks)libs/gi/ui/src/util/getCalcDisplay.tsx(2 hunks)libs/gi/uidata/src/uiData.ts(2 hunks)libs/gi/wr/src/api.ts(3 hunks)libs/gi/wr/src/formula.ts(3 hunks)libs/gi/wr/src/type.d.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- libs/gi/ui/src/util/getCalcDisplay.tsx
- libs/gi/wr/src/type.d.ts
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: lint
🔇 Additional comments (14)
libs/gi/wr/src/api.ts (3)
24-24: LGTM!The import statement is correctly placed and aligns with the PR's objective of implementing non-stacking buffs.
44-47: LGTM!The error handling logic is correctly extended to include non-stacking paths, maintaining consistency with the existing tally path handling.
249-255: LGTM!The base operation logic in
mergeDatais well-structured and maintains consistency with the existing tally path handling. The use of ternary operators improves readability.libs/gi/uidata/src/uiData.ts (2)
32-32: LGTM!The import statement is correctly placed and aligns with the PR's objective.
541-542: LGTM!The non-stacking path handling in
getReadNodeis well-implemented and maintains consistency with the existing tally path handling.libs/gi/sheets/src/Artifacts/NoblesseOblige/index.tsx (4)
3-10: LGTM!The imports are well-organized and include all necessary functions for the non-stacking functionality.
21-26: LGTM!The implementation correctly switches from numeric to string comparison and properly implements the non-stacking buff mechanism.
36-38: LGTM!The team buff data structure is correctly extended to include the non-stacking property.
58-60: LGTM!The UI changes correctly implement visual feedback for inactive buffs through the set4Inactive node.
libs/gi/sheets/src/SheetUtil.tsx (2)
8-14: LGTM!The imports are well-organized and include all necessary types and functions for the non-stacking functionality.
Also applies to: 20-21
92-105: LGTM!The
nonStackBufffunction is well-implemented with:
- Clear parameter types
- Proper handling of active and inactive states
- Visual feedback through strikethrough formatting
- Correct team buff context handling
libs/gi/wr/src/formula.ts (3)
592-595: LGTM! Implementation aligns with requirements.The non-stacking buff tracking implementation correctly:
- Uses small string comparison for source selection
- Maintains proper namespace separation
- Follows established patterns in the codebase
614-614: LGTM! Export maintains consistency.The addition of
nonStackingto exports is necessary and maintains alphabetical ordering.
49-50: Verify visual feedback implementation.While the non-stacking buff implementation looks correct, please verify:
- How the strikethrough formatting for disabled buffs is implemented
- The handling of multiple buff sources (e.g., Mavuika and Citlali scenario mentioned in PR objectives)
Also applies to: 592-595
✅ Verification successful
Implementation verified successfully
The codebase correctly implements:
- Visual feedback through strikethrough styling in UI components
- Multiple buff source handling with proper source detection
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for UI components handling buff display rg -l "no4.*disabled|strikethrough" --type ts --type tsx # Search for test cases covering multiple buff sources rg -l "test.*no4.*buff.*source" --type ts --type tsxLength of output: 169
Script:
#!/bin/bash # Search for UI components handling buff display and strikethrough rg -l "strikethrough|disabled.*buff" --type ts # Search for buff source handling implementation rg -A 5 "no4.*buff" --type ts # Search for test files related to buffs rg -l "describe.*buff|test.*buff" --type tsLength of output: 336
Script:
#!/bin/bash # Check UI components for buff display implementation rg -A 5 "strikethrough" libs/gi/ui/src/components/FieldDisplay.tsx rg -A 5 "disabled.*buff|buff.*disabled" libs/gi/ui/src/components/FieldDisplay.tsx # Check utility implementation rg -A 5 "strikethrough" libs/gi/ui/src/util/getCalcDisplay.tsx # Check type definitions rg -A 5 "strikethrough|disabled.*buff" libs/gi/type.d.tsLength of output: 929
Script:
#!/bin/bash # Check buff calculation implementation rg -A 10 "export (function|const) .*buff.*calc" --type ts # Check for buff source handling rg -A 10 "source.*buff|buff.*source" --type tsLength of output: 1443
Describe your changes
Update NO4 to use tally to create a non-stacking buff. Unlike other attempts, this still places the buff in the
teamBuffnamespace, so it renders as a teambuff in all ways that are expected.Additionally, it has a proper source defined as one of the characters who is actually providing the buff. When there are multiple sources, the one with the smallest string (I assume sorted alphabetically?) is chosen instead.
It also renders a strikethrough on the buffs that were disabled to prevent stacking, for better feedback for the user
Issue or discord link
Testing/validation
Optimized many scenarios around buff being enabled, disabled, enabled many times, etc on different characters. No errors or crashes were seen.
Mavuika is the only one with the buff enabled, 20% atk is given to everyone as expected
Citlali activates the buff, now she is the primary source and Mavuika's is rendered with a strikethrough (to show it is disabled/not stacked). Everyone still receives only a 20% buff
Checklist before requesting a review (leave this PR as draft if any part of this list is not done.)
yarn run mini-cilocally to validate format and lint.Summary by CodeRabbit
Release Notes
New Features
Improvements
Technical Updates