ZO: 2.5 sheets#3224
Open
priolette wants to merge 16 commits into
Open
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughThis PR introduces dual skill-type slots (skillType1/skillType2), moves anomaly move-multiplier stats into the combat namespace, adds veilVulnerabilityCap_/stunned_mult_ logic, and propagates these schema and wiring changes across formulas, metadata, character sheets, UI, executors, and tests. ChangesDual-Slot Skill Types and Anomaly Stat Refactor
Character Sheet Logic and UI Implementation Updates
Estimated code review effort 🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install failed. For unrecoverable errors, disable the tool in CodeRabbit configuration. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
libs/zzz/formula/src/data/char/sheets/Vivian.ts (1)
217-235:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep the M1 anomaly/disorder bonus on the same unit scale.
Classic 3 a.m. gacha math bug: both buffs read
dm.m1.anomaly_disorder_dmg_, but only the anomaly branch wraps it inpercent(...). That makes one side scale differently from the other, so one of these two buffs is wrong.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@libs/zzz/formula/src/data/char/sheets/Vivian.ts` around lines 217 - 235, The two M1 buffs use the same source dm.m1.anomaly_disorder_dmg_ but scale differently because the 'anomaly' branch wraps it in percent(...) while the 'disorder' branch does not; make their units consistent by wrapping dm.m1.anomaly_disorder_dmg_ with percent(...) in the 'disorder' registration (the registerBuff call that uses teamBuff.combat.buff_.addWithDmgType('disorder', cmpGE(char.mindscape, 1, prophecy.ifOn(...)))) so both branches use percent(dm.m1.anomaly_disorder_dmg_).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@libs/zzz/dm-localization/src/executors/gen-locale/lib/dumpChars.ts`:
- Around line 114-118: In dumpChars.ts the desc array is being shifted
incorrectly by desc.unshift('') causing an extra placeholder because name
already skips the first entry; remove the desc.unshift('') line (or stop
inserting the extra empty slot) so that desc = Object.values(data).map(pot =>
processText(pot.Desc)) aligns with the name selection (which uses
Object.values(data).filter((_, i) => i > 0)[0].Name); ensure
processText(pot.Desc) remains used and that no other code expects a leading
empty element.
In `@libs/zzz/formula-ui/src/char/sheets/YeShunguang.tsx`:
- Around line 39-43: The m4 block is currently showing the core veil-cap buff by
referencing buff.core_veilVulnerabilityCap_ (via fieldForBuff) instead of the
Mindscape-4 buff; update the m4 array to call fieldForBuff with the M4-specific
buff identifier (replace buff.core_veilVulnerabilityCap_ with the YeShunguang M4
buff, e.g., buff.yeShunguang_m4 or the actual M4 buff constant used elsewhere)
so the sheet displays the correct M4 effect.
In `@libs/zzz/formula/src/data/char/sheets/YeShunguang.ts`:
- Around line 231-235: The customDmg call for 'm6_dmg' is being passed directly
which nests its result instead of registering its entries; change the call site
(the customDmg invocation that uses 'm6_dmg', baseTag, damageType1: 'elemental',
cmpGE(char.mindscape, 6, prod(own.final.atk, percent(dm.m6.dmg)))) to spread the
returned array into the surrounding entries (i.e. use the spread operator on the
value returned by customDmg) so the m6_dmg entries are inserted rather than
nested.
In `@libs/zzz/formula/src/data/common/dmg.ts`:
- Around line 75-79: The cmpGT call currently applies the veil cap before the
isStunned.ifOn branch, causing non-stunned targets to use enemy.common.stun_
when own.final.veilVulnerabilityCap_ > 0; change the fourth argument so the stun
cap is applied only inside the stunned branch: compute the stunned branch as
min(sum(percent(1), own.final.veilVulnerabilityCap_), enemy.common.stun_) and
pass isStunned.ifOn(that_min_expression, enemy.common.unstun_) as the fourth
parameter to cmpGT so unstunned targets keep enemy.common.unstun_ intact while
stunned targets get clamped.
---
Outside diff comments:
In `@libs/zzz/formula/src/data/char/sheets/Vivian.ts`:
- Around line 217-235: The two M1 buffs use the same source
dm.m1.anomaly_disorder_dmg_ but scale differently because the 'anomaly' branch
wraps it in percent(...) while the 'disorder' branch does not; make their units
consistent by wrapping dm.m1.anomaly_disorder_dmg_ with percent(...) in the
'disorder' registration (the registerBuff call that uses
teamBuff.combat.buff_.addWithDmgType('disorder', cmpGE(char.mindscape, 1,
prophecy.ifOn(...)))) so both branches use percent(dm.m1.anomaly_disorder_dmg_).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: bf4ca6de-0d4a-4d76-aee9-01921ea22332
⛔ Files ignored due to path filters (8)
libs/zzz/dm-localization/assets/locales/en/char_Burnice_gen.jsonis excluded by!**/*_gen.jsonlibs/zzz/dm-localization/assets/locales/en/char_Ellen_gen.jsonis excluded by!**/*_gen.jsonlibs/zzz/dm-localization/assets/locales/en/char_Grace_gen.jsonis excluded by!**/*_gen.jsonlibs/zzz/dm-localization/assets/locales/en/char_Harumasa_gen.jsonis excluded by!**/*_gen.jsonlibs/zzz/dm-localization/assets/locales/en/char_Lycaon_gen.jsonis excluded by!**/*_gen.jsonlibs/zzz/dm-localization/assets/locales/en/char_Soldier0Anby_gen.jsonis excluded by!**/*_gen.jsonlibs/zzz/dm-localization/assets/locales/en/char_Soldier11_gen.jsonis excluded by!**/*_gen.jsonlibs/zzz/stats/src/allStat_gen.jsonis excluded by!**/*_gen.json
📒 Files selected for processing (118)
libs/zzz/consts/src/common.tslibs/zzz/dm-localization/src/executors/gen-locale/lib/dumpChars.tslibs/zzz/formula-ui/src/char/sheets/Burnice.tsxlibs/zzz/formula-ui/src/char/sheets/Ellen.tsxlibs/zzz/formula-ui/src/char/sheets/Grace.tsxlibs/zzz/formula-ui/src/char/sheets/Soldier0Anby.tsxlibs/zzz/formula-ui/src/char/sheets/Soldier11.tsxlibs/zzz/formula-ui/src/char/sheets/Vivian.tsxlibs/zzz/formula-ui/src/char/sheets/YeShunguang.tsxlibs/zzz/formula-ui/src/char/sheets/Zhao.tsxlibs/zzz/formula-ui/src/disc/sheets/ChaosJazz.tsxlibs/zzz/formula/src/data/char/sheets/Alice.tslibs/zzz/formula/src/data/char/sheets/Anton.tslibs/zzz/formula/src/data/char/sheets/Burnice.tslibs/zzz/formula/src/data/char/sheets/Ellen.tslibs/zzz/formula/src/data/char/sheets/Evelyn.tslibs/zzz/formula/src/data/char/sheets/Grace.tslibs/zzz/formula/src/data/char/sheets/Soldier0Anby.tslibs/zzz/formula/src/data/char/sheets/Soldier11.tslibs/zzz/formula/src/data/char/sheets/Vivian.tslibs/zzz/formula/src/data/char/sheets/YeShunguang.tslibs/zzz/formula/src/data/char/sheets/Zhao.tslibs/zzz/formula/src/data/char/util.tslibs/zzz/formula/src/data/common/dmg.tslibs/zzz/formula/src/data/disc/sheets/ChaosJazz.tslibs/zzz/formula/src/data/util/listing.tslibs/zzz/formula/src/data/util/read.tslibs/zzz/formula/src/data/util/sheet.tslibs/zzz/formula/src/data/util/tag.tslibs/zzz/formula/src/executors/gen-desc/executor.tslibs/zzz/formula/src/meta/char/Alice/buffs.tslibs/zzz/formula/src/meta/char/Alice/formulas.tslibs/zzz/formula/src/meta/char/Anby/formulas.tslibs/zzz/formula/src/meta/char/Anton/buffs.tslibs/zzz/formula/src/meta/char/Anton/formulas.tslibs/zzz/formula/src/meta/char/Aria/formulas.tslibs/zzz/formula/src/meta/char/AstraYao/formulas.tslibs/zzz/formula/src/meta/char/Banyue/formulas.tslibs/zzz/formula/src/meta/char/Ben/formulas.tslibs/zzz/formula/src/meta/char/Billy/formulas.tslibs/zzz/formula/src/meta/char/Burnice/buffs.tslibs/zzz/formula/src/meta/char/Burnice/conditionals.tslibs/zzz/formula/src/meta/char/Burnice/formulas.tslibs/zzz/formula/src/meta/char/Caesar/formulas.tslibs/zzz/formula/src/meta/char/Cissia/formulas.tslibs/zzz/formula/src/meta/char/Corin/formulas.tslibs/zzz/formula/src/meta/char/Dialyn/formulas.tslibs/zzz/formula/src/meta/char/Ellen/buffs.tslibs/zzz/formula/src/meta/char/Ellen/formulas.tslibs/zzz/formula/src/meta/char/Evelyn/buffs.tslibs/zzz/formula/src/meta/char/Evelyn/formulas.tslibs/zzz/formula/src/meta/char/Grace/buffs.tslibs/zzz/formula/src/meta/char/Grace/conditionals.tslibs/zzz/formula/src/meta/char/Grace/formulas.tslibs/zzz/formula/src/meta/char/Harumasa/formulas.tslibs/zzz/formula/src/meta/char/Hugo/formulas.tslibs/zzz/formula/src/meta/char/Jane/formulas.tslibs/zzz/formula/src/meta/char/JuFufu/formulas.tslibs/zzz/formula/src/meta/char/Koleda/formulas.tslibs/zzz/formula/src/meta/char/Lighter/formulas.tslibs/zzz/formula/src/meta/char/Lucia/formulas.tslibs/zzz/formula/src/meta/char/Lucy/formulas.tslibs/zzz/formula/src/meta/char/Lycaon/formulas.tslibs/zzz/formula/src/meta/char/Manato/formulas.tslibs/zzz/formula/src/meta/char/Miyabi/formulas.tslibs/zzz/formula/src/meta/char/NangongYu/formulas.tslibs/zzz/formula/src/meta/char/Nekomata/formulas.tslibs/zzz/formula/src/meta/char/Nicole/formulas.tslibs/zzz/formula/src/meta/char/OrphieMagus/formulas.tslibs/zzz/formula/src/meta/char/PanYinhu/formulas.tslibs/zzz/formula/src/meta/char/Piper/formulas.tslibs/zzz/formula/src/meta/char/Promeia/formulas.tslibs/zzz/formula/src/meta/char/Pulchra/formulas.tslibs/zzz/formula/src/meta/char/Qingyi/formulas.tslibs/zzz/formula/src/meta/char/Rina/formulas.tslibs/zzz/formula/src/meta/char/Seed/formulas.tslibs/zzz/formula/src/meta/char/Seth/formulas.tslibs/zzz/formula/src/meta/char/Soldier0Anby/formulas.tslibs/zzz/formula/src/meta/char/Soldier11/buffs.tslibs/zzz/formula/src/meta/char/Soldier11/formulas.tslibs/zzz/formula/src/meta/char/Soukaku/formulas.tslibs/zzz/formula/src/meta/char/StarlightBilly/formulas.tslibs/zzz/formula/src/meta/char/Sunna/formulas.tslibs/zzz/formula/src/meta/char/Trigger/formulas.tslibs/zzz/formula/src/meta/char/Vivian/buffs.tslibs/zzz/formula/src/meta/char/Vivian/formulas.tslibs/zzz/formula/src/meta/char/Yanagi/formulas.tslibs/zzz/formula/src/meta/char/YeShunguang/buffs.tslibs/zzz/formula/src/meta/char/YeShunguang/conditionals.tslibs/zzz/formula/src/meta/char/YeShunguang/formulas.tslibs/zzz/formula/src/meta/char/Yidhari/formulas.tslibs/zzz/formula/src/meta/char/Yixuan/formulas.tslibs/zzz/formula/src/meta/char/Yuzuha/formulas.tslibs/zzz/formula/src/meta/char/Zhao/buffs.tslibs/zzz/formula/src/meta/char/Zhao/conditionals.tslibs/zzz/formula/src/meta/char/Zhao/formulas.tslibs/zzz/formula/src/meta/char/ZhuYuan/formulas.tslibs/zzz/formula/src/meta/disc/ChaosJazz/buffs.tslibs/zzz/localization/assets/locales/en/char_Burnice.jsonlibs/zzz/localization/assets/locales/en/char_Grace.jsonlibs/zzz/localization/assets/locales/en/char_Zhao.jsonlibs/zzz/page-optimize/src/Teammates.tsxlibs/zzz/stats/Data/Characters/Burnice.jsonlibs/zzz/stats/Data/Characters/Ellen.jsonlibs/zzz/stats/Data/Characters/Grace.jsonlibs/zzz/stats/Data/Characters/Harumasa.jsonlibs/zzz/stats/Data/Characters/Lycaon.jsonlibs/zzz/stats/Data/Characters/Soldier0Anby.jsonlibs/zzz/stats/Data/Characters/Soldier11.jsonlibs/zzz/stats/src/executors/gen-stats/src/characterData.tslibs/zzz/stats/src/mappedStats/char/maps/Burnice.tslibs/zzz/stats/src/mappedStats/char/maps/Ellen.tslibs/zzz/stats/src/mappedStats/char/maps/Grace.tslibs/zzz/stats/src/mappedStats/char/maps/Soldier0Anby.tslibs/zzz/stats/src/mappedStats/char/maps/Soldier11.tslibs/zzz/stats/src/mappedStats/char/maps/YeShunguang.tslibs/zzz/stats/src/mappedStats/char/maps/Zhao.tslibs/zzz/ui/src/Character/CharacterSelectionModal.tsx
Comment on lines
+114
to
+118
| const desc = Object.values(data).map((pot) => processText(pot.Desc)) | ||
| desc.unshift('') | ||
| return { | ||
| name: Object.values(data).filter((_, i) => i > 0)[0].Name, | ||
| desc: Object.values(data).map((pot) => processText(pot.Desc)), | ||
| desc, |
Contributor
There was a problem hiding this comment.
Don’t prepend a second placeholder potential entry.
This looks like a sleepy indexing trap: name already skips Object.values(data)[0], so desc is already aligned around that first special entry. desc.unshift('') adds a second empty slot and shifts every real potential description by one index.
Suggested fix
- const desc = Object.values(data).map((pot) => processText(pot.Desc))
- desc.unshift('')
+ const desc = Object.values(data).map((pot) => processText(pot.Desc))📝 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.
Suggested change
| const desc = Object.values(data).map((pot) => processText(pot.Desc)) | |
| desc.unshift('') | |
| return { | |
| name: Object.values(data).filter((_, i) => i > 0)[0].Name, | |
| desc: Object.values(data).map((pot) => processText(pot.Desc)), | |
| desc, | |
| const desc = Object.values(data).map((pot) => processText(pot.Desc)) | |
| return { | |
| name: Object.values(data).filter((_, i) => i > 0)[0].Name, | |
| desc, |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@libs/zzz/dm-localization/src/executors/gen-locale/lib/dumpChars.ts` around
lines 114 - 118, In dumpChars.ts the desc array is being shifted incorrectly by
desc.unshift('') causing an extra placeholder because name already skips the
first entry; remove the desc.unshift('') line (or stop inserting the extra empty
slot) so that desc = Object.values(data).map(pot => processText(pot.Desc))
aligns with the name selection (which uses Object.values(data).filter((_, i) =>
i > 0)[0].Name); ensure processText(pot.Desc) remains used and that no other
code expects a leading empty element.
Comment on lines
+39
to
+43
| m4: [ | ||
| { | ||
| type: 'fields', | ||
| fields: [fieldForBuff(buff.core_veilVulnerabilityCap_)], | ||
| }, |
Contributor
There was a problem hiding this comment.
M4 is still wired to the core veil-cap buff.
My sleep-deprived gacha brain tripped on this: Line 42 points the m4 section at buff.core_veilVulnerabilityCap_, which is already shown in the core block. That makes the sheet display the core stat twice and hides the actual Mindscape 4 effect. Please bind this section to the M4-specific buff instead.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@libs/zzz/formula-ui/src/char/sheets/YeShunguang.tsx` around lines 39 - 43,
The m4 block is currently showing the core veil-cap buff by referencing
buff.core_veilVulnerabilityCap_ (via fieldForBuff) instead of the Mindscape-4
buff; update the m4 array to call fieldForBuff with the M4-specific buff
identifier (replace buff.core_veilVulnerabilityCap_ with the YeShunguang M4
buff, e.g., buff.yeShunguang_m4 or the actual M4 buff constant used elsewhere)
so the sheet displays the correct M4 effect.
Comment on lines
+231
to
+235
| customDmg( | ||
| 'm6_dmg', | ||
| { ...baseTag, damageType1: 'elemental' }, | ||
| cmpGE(char.mindscape, 6, prod(own.final.atk, percent(dm.m6.dmg))) | ||
| ), |
Contributor
There was a problem hiding this comment.
Spread customDmg() here.
Sleep-deprived gacha note: this helper is used as an entry array everywhere else in the sheet pipeline, so passing it bare here nests the result instead of registering the m6_dmg entries.
Suggested fix
- customDmg(
+ ...customDmg(
'm6_dmg',
{ ...baseTag, damageType1: 'elemental' },
cmpGE(char.mindscape, 6, prod(own.final.atk, percent(dm.m6.dmg)))
),📝 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.
Suggested change
| customDmg( | |
| 'm6_dmg', | |
| { ...baseTag, damageType1: 'elemental' }, | |
| cmpGE(char.mindscape, 6, prod(own.final.atk, percent(dm.m6.dmg))) | |
| ), | |
| ...customDmg( | |
| 'm6_dmg', | |
| { ...baseTag, damageType1: 'elemental' }, | |
| cmpGE(char.mindscape, 6, prod(own.final.atk, percent(dm.m6.dmg))) | |
| ), |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@libs/zzz/formula/src/data/char/sheets/YeShunguang.ts` around lines 231 - 235,
The customDmg call for 'm6_dmg' is being passed directly which nests its result
instead of registering its entries; change the call site (the customDmg
invocation that uses 'm6_dmg', baseTag, damageType1: 'elemental',
cmpGE(char.mindscape, 6, prod(own.final.atk, percent(dm.m6.dmg)))) to spread the
returned array into the surrounding entries (i.e. use the spread operator on the
value returned by customDmg) so the m6_dmg entries are inserted rather than
nested.
Comment on lines
+75
to
+79
| cmpGT( | ||
| own.final.veilVulnerabilityCap_, | ||
| 0, | ||
| min(sum(percent(1), own.final.veilVulnerabilityCap_), enemy.common.stun_), | ||
| isStunned.ifOn(enemy.common.stun_, enemy.common.unstun_) |
Contributor
There was a problem hiding this comment.
Preserve the unstunned branch when applying the veil cap.
My 3 a.m. read: once veilVulnerabilityCap_ > 0, this bypasses isStunned.ifOn(...) entirely, so non-stunned targets still read from enemy.common.stun_. That overstates damage outside stun windows; the cap should only clamp the stunned branch.
🐇 Suggested fix
ownBuff.dmg.stunned_mult_.add(
- cmpGT(
- own.final.veilVulnerabilityCap_,
- 0,
- min(sum(percent(1), own.final.veilVulnerabilityCap_), enemy.common.stun_),
- isStunned.ifOn(enemy.common.stun_, enemy.common.unstun_)
- )
+ isStunned.ifOn(
+ cmpGT(
+ own.final.veilVulnerabilityCap_,
+ 0,
+ min(sum(percent(1), own.final.veilVulnerabilityCap_), enemy.common.stun_),
+ enemy.common.stun_
+ ),
+ enemy.common.unstun_
+ )
),🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@libs/zzz/formula/src/data/common/dmg.ts` around lines 75 - 79, The cmpGT call
currently applies the veil cap before the isStunned.ifOn branch, causing
non-stunned targets to use enemy.common.stun_ when
own.final.veilVulnerabilityCap_ > 0; change the fourth argument so the stun cap
is applied only inside the stunned branch: compute the stunned branch as
min(sum(percent(1), own.final.veilVulnerabilityCap_), enemy.common.stun_) and
pass isStunned.ifOn(that_min_expression, enemy.common.unstun_) as the fourth
parameter to cmpGT so unstunned targets keep enemy.common.unstun_ intact while
stunned targets get clamped.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Describe your changes
Issue or discord link
Testing/validation
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
New Features
Bug Fixes
Refactor