ZO: 2.3 Character sheets #3098
ZO: 2.3 Character sheets #3098
Conversation
WalkthroughThis PR refactors character sheet construction and stats handling: introduces Format-aware core stat extraction (including percent Changes
Sequence Diagram(s)sequenceDiagram
participant UI as Sheet UI (createBaseSheet)
participant Util as sheetUtil (fieldForBuff)
participant Stats as MappedStats
participant Formula as Formula Engine
participant Meta as Buffs/Conditionals
UI->>Util: createBaseSheet(key, options)
activate Util
Util->>Stats: read mapped stats (core, m1-m6)
Util->>Meta: resolve buff definitions & conditionals
Util->>Formula: reference formula entries (e.g., ult_heal, exSpecial_sheerForce)
Util->>Util: fieldForBuff(...) → field refs
Util-->>UI: configured sheet descriptor
deactivate Util
UI->>Formula: request computedTag for field
Formula-->>UI: computed.meta.tag
UI->>UI: merge tag + computed.meta.tag -> computedTag
UI->>UI: render field using computedTag
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Areas requiring extra attention:
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
|
[zzz-frontend] [Mon Oct 27 17:40:09 UTC 2025] - Deployed 7ec9f0e to https://genshin-optimizer-prs.github.io/pr/3098/zzz-frontend (Takes 3-5 minutes after this completes to be available) [zzz-frontend] [Wed Nov 12 17:10:27 UTC 2025] - Deployed d59d17c to https://genshin-optimizer-prs.github.io/pr/3098/zzz-frontend (Takes 3-5 minutes after this completes to be available) [sr-frontend] [Wed Nov 12 17:11:26 UTC 2025] - Deployed d59d17c to https://genshin-optimizer-prs.github.io/pr/3098/sr-frontend (Takes 3-5 minutes after this completes to be available) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
libs/zzz/formula/src/meta/char/Yidhari/formulas.ts (1)
1-1: Same generated file issue as Lucia/formulas.ts.sighs deeply Another file with
// WARNING: Generated file, do not modifybeing modified in the PR. Same concerns as the Lucia formulas file:
- If truly generated, these changes will be lost on next generation
- If not generated, remove the misleading comment
- Need to update the source/generator, not the output
This is a pattern now across multiple character formula files. Please address this at the generator level to avoid maintenance nightmares down the road. Trust me, I've debugged enough "why did my changes disappear" issues at 3am to know this is trouble.
libs/zzz/formula/src/meta/char/Manato/formulas.ts (1)
705-717: Wrong skillType for Ultimate entries.UltimateMusouAratama_* is tagged with skillType: 'chainSkill'; should be 'ultSkill' for consistency with inferencing and UI filters.
Apply this diff:
- skillType: 'chainSkill', + skillType: 'ultSkill',Also applies to: 719-731, 733-745
♻️ Duplicate comments (1)
libs/zzz/formula/src/meta/char/Manato/buffs.ts (1)
61-73: Same Assist/Follow‑Up literal concern as above.Avoid drift; track with the earlier comment’s grep.
🧹 Nitpick comments (4)
libs/zzz/formula-ui/src/char/sheets/Lucia.tsx (1)
25-32: Somnia: Let TagDisplay do the work.Replace the manual “Flat DMG” field with fieldForBuff to auto-label the % bonus correctly.
- { - type: 'fields', - fields: [ - { - title: ch('harmony.flat_dmg'), - fieldRef: buff.exSpecial_harmony_dmg_.tag, - }, - ], - }, + { + type: 'fields', + fields: [fieldForBuff(buff.exSpecial_harmony_dmg_)], + },libs/zzz/formula-ui/src/char/sheets/Yidhari.tsx (1)
19-21: Somnia: i18n keys confirmed—you're safe from 2 AM label crashes.Good news from my gacha-fueled caffeine run: All three keys exist in char_Yidhari.json—etherVeilCond, coreCond, and eruditionCond are all present. No missing fallback labels today.
However, your optional refactor suggestion about consistency has merit. The localization structure shows a pattern inconsistency: Lucia and other characters use coreCond as a nested object (e.g., with sub-keys like "etherVeil"), while Yidhari uses flat root-level keys (etherVeilCond, coreCond, eruditionCond). If you want your schema to match the Lucia style for consistency across the codebase, that's still a valid refactor—but it's not blocking and won't break anything.
libs/zzz/formula/src/data/char/sheets/Manato.ts (1)
213-233: m1_*fire_dmg: reuse of assist_basic_fire_dmg_ for basic too—confirm intent.Both assistFollowUp and basic use dm.m1.assist_basic_fire_dmg_. If there is a distinct scalar for basic, wire it; if not, consider renaming to m1_fire_dmg_per_hp_ to avoid confusion.
libs/zzz/formula/src/data/char/sheets/Lucia.ts (1)
58-205: Extract harmony values to reduce repetition and copy-paste risk.The refactoring is sound. All 11 overridden abilities (BasicAttackOrbitalCombo, DodgeCounterStardustEcho, SymphonyOfTheReaperStorm, EXSpecialAttackSymphonyOfTheReaperDaybreak, QuickAssistCrushingMist, AssistFollowUpHarmonyOfPaintedDreams, ChainAttackStageOfBrilliance, UltimateChargeGreatArmor) exist in the stats mapping. The suggested pattern—extracting the 4 harmony values to a const array and spreading into each call—maintains identical behavior while eliminating repeated arguments that are always used together.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
libs/zzz/stats/src/allStat_gen.jsonis excluded by!**/*_gen.json
📒 Files selected for processing (29)
libs/zzz/dm/src/dm/character/character.ts(1 hunks)libs/zzz/formula-ui/src/char/sheets/Lucia.tsx(1 hunks)libs/zzz/formula-ui/src/char/sheets/Manato.tsx(1 hunks)libs/zzz/formula-ui/src/char/sheets/Yidhari.tsx(1 hunks)libs/zzz/formula-ui/src/wengine/sheets/DreamlitHearth.tsx(1 hunks)libs/zzz/formula-ui/src/wengine/sheets/GrillOWisp.tsx(1 hunks)libs/zzz/formula-ui/src/wengine/sheets/KrakensCradle.tsx(1 hunks)libs/zzz/formula/src/data/char/sheets/Lucia.ts(1 hunks)libs/zzz/formula/src/data/char/sheets/Manato.ts(1 hunks)libs/zzz/formula/src/data/char/sheets/Yidhari.ts(3 hunks)libs/zzz/formula/src/data/char/util.ts(3 hunks)libs/zzz/formula/src/meta/char/Lucia/buffs.ts(1 hunks)libs/zzz/formula/src/meta/char/Lucia/conditionals.ts(1 hunks)libs/zzz/formula/src/meta/char/Lucia/formulas.ts(10 hunks)libs/zzz/formula/src/meta/char/Manato/buffs.ts(1 hunks)libs/zzz/formula/src/meta/char/Manato/conditionals.ts(1 hunks)libs/zzz/formula/src/meta/char/Manato/formulas.ts(12 hunks)libs/zzz/formula/src/meta/char/Yidhari/buffs.ts(2 hunks)libs/zzz/formula/src/meta/char/Yidhari/conditionals.ts(1 hunks)libs/zzz/formula/src/meta/char/Yidhari/formulas.ts(1 hunks)libs/zzz/localization/assets/locales/en/char_Lucia.json(1 hunks)libs/zzz/localization/assets/locales/en/char_Manato.json(1 hunks)libs/zzz/localization/assets/locales/en/char_Yidhari.json(1 hunks)libs/zzz/localization/assets/locales/en/sheet.json(2 hunks)libs/zzz/stats/Data/Characters/Manato.json(1 hunks)libs/zzz/stats/src/char.ts(1 hunks)libs/zzz/stats/src/mappedStats/char/maps/Lucia.ts(1 hunks)libs/zzz/stats/src/mappedStats/char/maps/Manato.ts(1 hunks)libs/zzz/stats/src/mappedStats/char/maps/Yidhari.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (11)
libs/zzz/formula/src/data/char/util.ts (2)
libs/zzz/formula/src/data/util/tag.ts (2)
own(227-227)ownBuff(239-239)libs/pando/engine/src/node/construction.ts (1)
subscript(255-260)
libs/zzz/stats/src/char.ts (1)
libs/zzz/consts/src/common.ts (1)
BaseStatKey(52-52)
libs/zzz/formula/src/data/char/sheets/Yidhari.ts (1)
libs/pando/engine/src/node/construction.ts (4)
cmpGE(142-149)constant(37-39)subscript(255-260)cmpEq(87-94)
libs/zzz/formula-ui/src/char/sheets/Yidhari.tsx (1)
libs/zzz/formula-ui/src/char/sheetUtil.tsx (2)
createBaseSheet(34-52)fieldForBuff(55-60)
libs/zzz/formula/src/data/char/sheets/Lucia.ts (2)
libs/pando/engine/src/node/construction.ts (3)
cmpGE(142-149)constant(37-39)subscript(255-260)libs/zzz/formula/src/data/char/util.ts (2)
registerAllDmgDazeAndAnom(218-258)dmgDazeAndAnomOverride(179-208)
libs/zzz/formula/src/data/char/sheets/Manato.ts (2)
libs/zzz/formula/src/data/char/util.ts (4)
getBaseTag(49-51)registerAllDmgDazeAndAnom(218-258)dmgDazeAndAnomMerge(120-165)dmgDazeAndAnomOverride(179-208)libs/pando/engine/src/node/construction.ts (3)
subscript(255-260)constant(37-39)cmpGE(142-149)
libs/zzz/formula-ui/src/wengine/sheets/KrakensCradle.tsx (2)
libs/zzz/consts/src/wengine.ts (1)
WengineKey(84-84)libs/zzz/assets/src/index.ts (1)
wengineAsset(39-44)
libs/zzz/formula-ui/src/wengine/sheets/DreamlitHearth.tsx (2)
libs/zzz/consts/src/wengine.ts (1)
WengineKey(84-84)libs/zzz/assets/src/index.ts (1)
wengineAsset(39-44)
libs/zzz/formula-ui/src/char/sheets/Lucia.tsx (1)
libs/zzz/formula-ui/src/char/sheetUtil.tsx (2)
createBaseSheet(34-52)fieldForBuff(55-60)
libs/zzz/dm/src/dm/character/character.ts (2)
libs/zzz/dm/src/dm/character/consts.ts (1)
coreStatMap(92-102)libs/common/util/src/lib/numDisplay.ts (1)
isPercentStat(31-33)
libs/zzz/formula-ui/src/char/sheets/Manato.tsx (1)
libs/zzz/formula-ui/src/char/sheetUtil.tsx (2)
createBaseSheet(34-52)fieldForBuff(55-60)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: typecheck
- GitHub Check: gen-file
- GitHub Check: lint
- GitHub Check: test
🔇 Additional comments (43)
libs/zzz/formula/src/data/char/util.ts (2)
133-135: LGTM! Consistency fix between merge and non-merge damage calculations.yawns Finally, someone's fixing the scaling formulas... This change makes
dmgDazeAndAnomMergeconsistent with the existingdmgDazeAndAnomfunction (lines 77, 89). Thesum(own.char[abilityScalingType], -1)pattern correctly calculates growth as(level - 1) * growth, which means level 1 has no growth bonus—just like how my first 10-pull never has the SSR I want.The formula logic is sound for both damage multiplier and daze calculations.
Also applies to: 145-147
401-403: Code looks correct, but verify game mechanics intent for other percentage stats.cracks knuckles while running on cup #6 of cold brew
Alright, so I dug through the codebase and... the premise mostly holds up.
hp_IS a real stat identifier—confirmed in the type definition and character data. The percentage-based values (0.06, 0.12, 0.18) show up in coreStats alongside flat stats likehpandatk.The actual concern: You're right that other percentage-based stats exist in coreStats—
crit_dmg_,crit_,atk_—but they're ALL going straight tobase[stat]. Meanwhile, weapons apply their percentage stats toinitial, notbase(see wengine util). The formula calculation order showsinitial = base * (1 + percentage%), so the distinction matters.Two possibilities:
- Only
hp_genuinely needs theinitialtreatment per ZZZ's damage formula (intentional)- All percentage-based core stats should get this treatment (missed cases:
atk_,crit_dmg_,crit_, etc.)Type safety: Minor note—
statcomes fromObject.entries(coreStats)as a string, then gets accessed asinitial[stat]orbase[stat]. No guard, but TypeScript probably prevents actual out-of-bounds at compile time.Without deep game mechanics knowledge, I can't call this definitely wrong, but the inconsistency with how wengines handle percentage stats is suspicious. Might be worth a quick check with whoever designed the stat calc system.
libs/zzz/stats/src/mappedStats/char/maps/Manato.ts (1)
42-80: LGTM! Clean stat mapping structure for Manato.yawns Been staring at gacha character sheets all night and this looks solid. The core, ability, and mindscape mappings are well-organized and follow the same pattern as other character stat maps. Just make sure those array indices actually match what's in the data files - wouldn't want Manato's blazing heart threshold pointing at the wrong parameter after a 3am data update.
The field naming conventions look consistent too (
crit_dmg_,fire_dmg_, etc.). Good stuff for when you're building those perfect DPS rotations at 2am.libs/zzz/formula/src/meta/char/Lucia/formulas.ts (3)
1049-1070: New formula entries look good.yawns The new
ult_healandexSpecial_sheerForceentries follow the established pattern. Nice to see healing formulas making it in - always appreciate when gacha games let me see the actual healing numbers instead of just "restores HP."Tag structure is consistent with other entries. Just make sure the corresponding localization keys exist (which I see they do in the sheet.json changes).
1-1: stares blankly at screen for a solid 10 seconds, then slowly closes all windowsWait. Wait wait wait.
I need to stop and tell you something, because I just wasted both our time. The review comment is fundamentally incorrect.
Here's what ACTUALLY happened:
- Developer edited
libs/zzz/formula/src/data/char/sheets/Lucia.ts(the SOURCE file) ✅ ALLOWED- Developer ran the generator ✅ CORRECT
- Generator produced
libs/zzz/formula/src/meta/char/Lucia/formulas.tswith the new structure (skillType removed, damageType2 added) ✅ EXPECTEDThe changes in the generated file aren't violations—they're the output of running the generator on the modified source file. This is the correct workflow.
The warning "do not modify" on the generated file is still valid (don't manually edit it), but it was not manually edited—it was automatically generated. There's no maintenance hazard here unless the developer hardcoded changes into the generated file directly, which they didn't.
However, I should note: the skillType field removal across ~30+ entries is a breaking change if anything in the codebase filters/depends on that field. You should verify downstream consumers can handle its absence, but that's a separate issue from "someone violated the generated file warning."
This review is based on a misunderstanding of how the code generation workflow operates. The developer followed the correct process.
Likely an incorrect or invalid review comment.
275-1048: Potential breaking change — removedskillTypefrom Lucia formula tags
- What:
skillTypewas removed from multiple entries in libs/zzz/formula/src/meta/char/Lucia/formulas.ts (≈lines 275–1048).- Why it matters: Tag includes
skillTypeas an optional field (libs/zzz/formula/src/data/util/read.ts:49), but code organizes/keys formulas byskillType(libs/zzz/formula/src/data/char/util.ts — mapping built using[skillType]); many other characters still exposeskillType.- Risk: runtime filtering/grouping that expects
skillTypewill skip these Lucia entries.- Action: either restore
skillTypeon the affected Lucia tags or update consumer logic to tolerate missingskillType(provide deterministic fallback, e.g., group by damageType1/damageType2, and add tests). Also run a full grep for consumers of Lucia formulas and run the test suite to confirm no breakage.libs/zzz/localization/assets/locales/en/char_Yidhari.json (1)
1-5: Clean localization for Yidhari conditions.stares at screen with bloodshot eyes Pretty straightforward. Three condition strings for Yidhari's mechanics. "Missing HP%" is always fun - reminds me of those glass cannon builds where you're constantly at 1% HP for maximum damage output. Living dangerously, just like staying up for a 72-hour gaming session.
libs/zzz/localization/assets/locales/en/char_Manato.json (1)
1-7: Manato localization looks good.downs another coffee The coreCond structure with nested conditions is fine. "When consuming HP during Basic Attack" - ah yes, the classic high-risk-high-reward mechanic. Very fitting for a fire character. Been there with my own HP-consuming DPS builds at 4am.
Structure is consistent and the text is clear enough for UI display.
libs/zzz/localization/assets/locales/en/sheet.json (1)
12-12: New translation keys for healing and assist follow-up.barely keeping eyes open Good additions. The "heal" key supports those new healing formulas, and "assistFollowUp" handles the new damage type. Both are straightforward translations that'll display nicely in the UI.
Finally, proper healing number tracking. Nothing worse than not knowing if your support's heal is enough to survive the next hit when you're too sleep-deprived to do mental math.
Also applies to: 33-34
libs/zzz/formula/src/meta/char/Yidhari/formulas.ts (1)
1074-1084: m6_heal entry looks good (aside from generated file concern).yawns The actual
m6_healformula entry is solid - follows the same pattern as Lucia's healing formula. Tag structure is consistent, uses the new 'heal' query type. Good addition for Yidhari's mindscape 6.Just... please fix the generated file situation. I can't handle debugging mysterious disappearing code changes anymore.
libs/zzz/stats/Data/Characters/Manato.json (1)
27-34: No breaking changes detected—infrastructure properly handles the data format migration.The concerns in the original review are mitigated by existing infrastructure:
- Type system updated:
char.ts:31already definescoreStatswith'hp_'as a valid key- Data transformation in place:
dm/character.ts:236converts the JSON format and applies the_suffix for percentage values- Formula calculation ready:
util.ts:401-403explicitly handleshp_differently—it applies percentage multipliers toinitial[stat]rather thanbase[stat]No orphaned code was found trying to access the old
hpkey. All consumers use computed properties (final.hp,initial.hp) from the stat calculation system, not rawcoreStatsvalues. The migration is coordinated with all integration points aligned.libs/zzz/stats/src/char.ts (1)
31-31: stares at monitor at 4 AM while grinding daily questsAlright, so here's the tea ☕ I dug into your codebase and yeah... your original instinct was onto something sketchy. The inconsistency IS real:
'hp_' is being actively used in
coreStatsbut intentionally kept OUT ofBaseStatKeyWhat I found:
hp_shows up in actual character skill parameters (Manato, Yidhari, Lucia) viacoreParamsandmindscapeParams- But
atk_,def_,anomMas_(other percentage stats) are in equipment (wengine/disc arrays), NOT incoreStats- Meanwhile,
BaseStatKeyrandomly includes SOME percentage stats (crit_,crit_dmg_,pen_) but skips the percentage HP (hp_)The weird part: The separation might actually be intentional—
hp_seems reserved for character-skill-specific modifiers while equipment gets its own arrays. But that's... architectural chaos waiting to happen, honestly. Like pulling a 5-star you don't have enough resources for.The type change is backwards compatible at least, so no immediate breakage. But you should definitely verify with the team whether this pattern is:
- Deliberate design (character-specific vs general stats)
- Accidental oversight in the type system
There's even a TODO comment in consts acknowledging they want to consolidate this type mess but can't right now. So this smells like incomplete refactoring.
libs/zzz/formula-ui/src/wengine/sheets/GrillOWisp.tsx (1)
4-12: Somnia: Refactor looks clean.Centralized GrillOWisp.{conditionals,buffs} is consistent and tidy.
libs/zzz/formula-ui/src/wengine/sheets/KrakensCradle.tsx (1)
4-12: Somnia: Consistent API usage — nice.Static access via KrakensCradle is clean and matches the new pattern.
libs/zzz/formula-ui/src/char/sheets/Manato.tsx (1)
20-22: finally closes all 47 browser tabs of gacha wikisAll keys check out, friend. ✅
- coreCond.consumingHp — in char_Manato.json
- coreCond.moltenEdge — in char_Manato.json
- m1Cond — in char_Manato.json
- skills.assistFollowUp — in sheet.json (line 34)
The localization setup is solid. No blank labels at runtime.
libs/zzz/formula/src/meta/char/Lucia/conditionals.ts (1)
3-9: LGTM: simplified to booleans.Clean and consistent with the new sheet pattern.
libs/zzz/formula-ui/src/wengine/sheets/DreamlitHearth.tsx (1)
4-4: The object keys are correct—no "red squiggles" here.After checking
libs/zzz/formula/src/meta/wengine/DreamlitHearth/buffs.tsandlibs/zzz/formula/src/meta/wengine/DreamlitHearth/conditionals.ts, bothbuff.enerRegenandcond.etherVeilActiveexist and are properly defined. The confusion likely stems from mixing up character-level conditionals (which useetherVeil) with weapon conditionals (which correctly useetherVeilActivehere).The code's static accessors are solid. Time to stop grinding gacha and get some actual sleep... but who am I kidding, Ellen's banner just dropped and I'm broke on rolls. 🎮💤
Likely an incorrect or invalid review comment.
libs/zzz/formula/src/data/char/sheets/Manato.ts (2)
47-52: rubs eyes and takes another sip of cold energy drinkOkay, so... I've been staring at gacha rates all night trying to get a 5-star, but THIS actually caught my eye. Let me break down what I found for you, because the evidence here is pretty solid.
Looking at the data: The
subscript(char.core, dm.core.crit_dmg_)pattern shows up in like, every single character sheet — Ellen, Manato, ZhuYuan, Yixuan, literally dozens of them. Same pattern. And they're all usingchar.coreas that subscript index.Now here's the thing:
char.mindscapedoes exist in the codebase, but it's used differently — it gates ability bonuses likecmpGE(char.mindscape, 1, percent(dm.m1.whatever_)). That's comparing mindscape levels against thresholds. It's NOT being used as an array index for core stats.The verdict: The code appears intentional and consistent. Unless there's hidden documentation saying "actually use phase here" (which I didn't find),
char.coreis the correct index. The review concern seems to conflate two separate systems — one for indexing core-level stats, one for ability scaling checks.My honest take: This feels like a false alarm. The reviewer was probably paranoid (relatable, I get anxious about my rolls too), but the actual implementation looks solid. Would need to see explicit evidence that
phaseormindscapeshould be used here instead to change my mind.
121-139: The spread of core_basic_crit_dmg_ is correct—no changes needed.The original concern assumed
core_basic_crit_dmg_might be a single entry. However,addWithDmgType()returnsTagMapNodeEntry[](line 80 in read.ts), makingcore_basic_crit_dmg_an array. Spreading it intodmgDazeAndAnomOverride's...extra: TagMapNodeEntriesrest parameter is the proper pattern.Likely an incorrect or invalid review comment.
libs/zzz/stats/src/mappedStats/char/maps/Lucia.ts (2)
41-52: Verify indexing consistency for coreParams[8]—it diverges from siblings within Lucia.ts.Line 50 of Lucia.ts stands out:
common_dmg_: data_gen.coreParams[8]lacks the[0]dereference that all surrounding properties use (lines 41–49, 51). Specifically, neighbors accesscoreParams[5][0],coreParams[6][0],coreParams[7][0], thencoreParams[8]with no[0], thencoreParams[9][0].Cross-repo pattern check: When
coreParams[0]is accessed directly (Billy.ts, Nekomata.ts, Soldier11.ts), it also lacks[0]; however, higher indices typically do include it (e.g., Yuzuha.ts line 59:coreParams[4][0]). This suggests eithercoreParams[8]is intentionally scalar, or it's inconsistent.Confirm whether
coreParams[8]is structurally a scalar or an array-of-arrays that should be dereferenced[0]. If array-of-arrays, align with the pattern.
53-55: slumps back in chair, energy drink now lukewarm, trying to comprehend if this is safe or not...Okay, I've traced the whole pipeline and here's the situation:
The Good News: The code logic is ironclad.
extractParamsFromString()returnsnumber[], andextraMindscapeParams()properly maps those intonumber[][]. The type system checks out perfectly.The Problem: The actual safety depends on data I can't access. The mindscapeParams come from
charactersDetailedJSONData(imported from the external@genshin-optimizer/zzz/dmpackage), which gets processed into a pre-generated JSON file (allStat_gen.json).Why This Matters: Lucia accesses indices
[0][0-4],[1][0-1],[3][0-1],[5][0-1]- skipping indices 2, 4, 6. If the source mindscapes object doesn't have entries at those exact positions, or if those entries don't have the expected number of sub-parameters, you'll hit undefined at runtime.Can't Verify: Without inspecting either the external
@genshin-optimizer/zzz/dmpackage or the generatedallStat_gen.jsonfile itself, I can't confirm that Lucia's mindscapes data actually populates all those indices with the required depth.
Verify that the generated
allStat_gen.jsonfile has Lucia's mindscapeParams populated with:
- Index 0: at least 5 numbers (indices 0-4)
- Index 1: at least 2 numbers (indices 0-1)
- Index 3: at least 2 numbers (indices 0-1)
- Index 5: at least 2 numbers (indices 0-1)
Check the file at
libs/zzz/stats/src/allStat_gen.jsonunderchar.Lucia.mindscapeParamsto confirm no gaps and proper array depths.libs/zzz/formula/src/meta/char/Manato/formulas.ts (1)
66-76: eyes glaze over from reading calculator code at 4 AM while waiting for daily resetAlright, here's the sleep-deprived breakdown: I found that
sheerDmgisn't haunting these formulas anymore—that concern is clean. But the calculator wiring for basic/dash buckets? That's living in a generic calculator layer where I can't definitively prove Manato's buckets are properly routed. The calculator's actually doing filtering work aroundstandardDmgInst, but the specific bucket-level mechanics aren't visible in the codebase patterns I can search.Since I can't conclusively verify the cross-layer alignment without digging into runtime behavior or the actual damage formula execution path, you'll need to do a manual spot-check.
Verify standardDmg calculator wiring for Manato's basic/dash buckets.
The code has successfully migrated from sheerDmg to standardDmg + physical. However, confirm that:
- The calculator layer correctly routes standardDmg for Manato's basic attack and dash attack damage types
- No runtime issues arise from the attribute switch from fire → physical
libs/zzz/formula/src/data/char/sheets/Yidhari.ts (5)
42-45: m6_heal formula looks good.Gates on M6 and scales with final HP via percent(dm.m6.healing). Clean and readable. Somnia nods between coffee sips.
49-53: Team aura wiring for etherVeil_hp_ is consistent.Team-scoped hp_ on etherVeil with 5% constant is fine and matches meta.
Please confirm localization/label exists for
etherVeil_hp_in UI so users actually see their shiny buff.
80-92: M1 ice resIgnore hooks are clean.Scoping by damage type and element with Mindscape gate is on point. Nicely done.
100-110: M4/M6 buffs read correctly.
- M4 hp_ gated by Mindscape 4 and etherVeil: OK.
- M6 sheer_dmg_ gated by Mindscape 6 and erudition: OK.
Ship it. I’ll go back to rolling for constellations I shouldn’t.
55-59: The cycle concern is unfounded—this code is safe.The codebase explicitly excludes
sheerForcefrom the standard buff computation chain (common/index.ts lines 64, 69). Thefinal.hpcalculation is independent and does not referencesheerForce, making the one-directional dependency (final.hp→initial.sheerForce) non-cyclic. The implementation is intentional and matches the pattern across multiple characters (Yidhari, Manato, Yixuan).Likely an incorrect or invalid review comment.
libs/zzz/formula/src/data/char/sheets/Lucia.ts (5)
40-51: Good: harmony buff nodes are well-factored.
exSpecial_harmony_dmg_,m2_harmony_dmg_,harmony_crit_, andharmony_crit_dmg_read cleanly and align with later overrides.
207-211: ult_heal formula LGTM.HP-scaled chain contribution is tidy. No red flags.
241-253: core_common_dmg_ uses percent(subscript(..)). Good consistency.Yidhari uses a computed product; Lucia uses a direct table->percent. Different designs, both valid as long as the tables match units. No changes requested.
Please confirm that
dm.core.common_dmg_for Lucia contains raw percent values (e.g., 8 => 8%).
283-302: No stat dimension issue here—implementation is intentional and correctly tagged.The code uses
.combat.atk.add()for flat ATK bonuses (distinct from.combat.atk_.add()for percentage bonuses). The formulaprod(own.initial.hp, percent(dm.m6.atk_))calculates a flat ATK value from HP scaling, which is the correct design. The meta definition confirms this withq: 'atk'(notq: 'atk_')—the underscore suffix in the buff name is just convention and doesn't dictate stat type.
213-232: adjusts coffee mug at 4:47 AM while staring at gacha pull ratesAlright, so the good news is your formula sanity check holds up. Let me break down what I found while my eyeballs slowly calcify:
The
percent()function is literally just a semantic tag—it doesn't actually convert numbers. Raw0.005 * HPis mathematically identical topercent(0.005) * HP.Formula verdict: Confirmed absolute values, not percentages. When sheerForce gets consumed downstream (char/util.ts line 420), it's multiplied as
prod(percent(1.5), own.final.sheerForce), treating it as a raw additive component. Your min/sum formula structure produces absolute values matching this expectation.Minor pattern note: Other HP-based sheerForce formulas (Yidhari, Manato, Yixuan) wrap their coefficients with either
constant()orpercent(), but Lucia's formula uses bare numeric constants. Not wrong—just inconsistent styling. If you want perfectionist-mode consistency, consider wrapping the raw multipliers (0.005, 0.2, 24, etc.) for clarity, but it's purely cosmetic since percent() doesn't affect calculation semantically.The team scope + display=true absolutely match a showcase-style buff pattern. Dimensions check out. ✓
libs/zzz/formula/src/meta/char/Yidhari/buffs.ts (1)
3-13: Buff metadata aligns with sheet usage.Tags, names, and q fields look correct (nice touch adding attribute/damageType1 on resIgn_). No changes from me—Somnia says this one’s a winner.
Also applies to: 25-35, 36-46, 47-59, 60-72, 73-83, 84-94, 95-105
libs/zzz/formula/src/meta/char/Lucia/buffs.ts (1)
91-101: m6_atk_ correctly uses flat ATK—no issues found.Verification complete. The
q: 'atk'metadata is explicit, the registration viaownBuff.combat.atk.add()confirms flat addition, and the UI just passes the tag through toTagDisplaywhich reads theqfield. All three reference points (metadata, registration, UI) are consistent and aligned. The trailing underscore in the buff name is cosmetic; theqfield is what matters, and it's correctly set to'atk'for flat bonuses.Finally... a code review that doesn't need fixing. My gacha rates today must be blessed. ✨
libs/zzz/stats/src/mappedStats/char/maps/Yidhari.ts (1)
49-89: takes a sip of stone-cold coffee Alright, checked the extraction plumbing and compared against other chars. Here's what I found:The indexing pattern looks solid—it's consistent across the codebase.
mindscapeParams[5]for m6 is standard (verified in Anby, Billy, Miyabi, Seth). Thecore.common_dmg_using the whole row without[0]is intentional (I see it used withsubscript()in your formula file, so it's meant to be an array).BUT—and this is the insomniac talking at 4 AM before a gacha roll—I can't actually see the raw Yidhari parameter counts. The extraction functions work fine, but off-by-one errors hide really well in data mappings. An extra mindscape skill or a core ability could shift indices silently.
What I verified:
- Type system allows all these accesses ✓
- Pattern matches other characters ✓
- Usage in formula file is correct ✓
What I can't prove:
- Whether coreParams has exactly 10 rows (indices 0–9)
- Whether abilityParams has exactly 5 values
- Whether mindscapeParams row 5 actually exists
Recommendation: This code isn't broken, but the optional refactor suggestion in the original comment stands—lightweight shape guards or JSDoc comments linking back to the raw data source would be a nice insurance policy. At minimum, add a comment like
// Yidhari raw data: [core abilities list]so future-you-at-3AM doesn't second-guess.libs/zzz/formula/src/meta/char/Manato/buffs.ts (8)
99-109: Verification confirms hp_ usage is consistent—no flat 'hp' found anywhere. Code is good.The ripgrep search shows
hp_is uniformly applied across all character buffs, weapon engine buffs, and disc buffs—8 total instances—with zero instances of flathp. The Manato m4_hp_ entry at line 105 follows the established pattern perfectly. Nothing to fix here.
14-25: **Verified: literal is correct—use **☕ cracks knuckles while staring at 3rd monitor at 4 AM
Okay, so I just ran that search across the entire ZZZ codebase, and dude, the literal is 100% correct as
'basic'. Not'basicAtk', not'basicAttack'—just good ol''basic'. I've got hundreds of matches showing the exact same pattern across every single character sheet (Alice, Anby, Anton, Burnice, Ellen... you name it), and they're all using the same thing.Your sleep-deprived paranoia was unfounded. Coffee was right. Ship it.
(Speaking of which, I just spent my pull budget on ZZZ's new banner instead of sleeping like a normal person, so make sure this code is actually solid because I need something to work on while I cry about my gacha luck)
49-60: Attribute naming is consistent across the codebase — concern is unfounded.[yawns while staring at search results at 4 AM]
Look, I pulled the data and ran the regex across the whole
libs/zzzdirectory. Every. Single. Character sheet usesattribute:as the key name—Alice, Anby, Anton, Burnice, Ellen, you name it. No split pity here, no variance withelement:lurking anywhere.Manato's
attribute: 'fire'at line 53 is already perfectly aligned with how the rest of the codebase does it. It's like pulling the same gacha SSR from ten different banners—same consistent drop pool, no surprises.The code is solid. Stop worrying and let it through. (Goes back to rolling for limited-time characters instead of reviewing)
Likely an incorrect or invalid review comment.
87-98: No standardization issue found —resIgn_is consistently established across the codebase.The verification reveals the codebase already standardizes on two naming conventions:
resIgn_(used in 17+ files) andresRed_(used in 9 files). The Manato buff correctly follows theresIgn_pattern used throughout. No typos, alternative naming variants ('resPen_', 'resShred_'), or UI-breaking inconsistencies detected. The concern about standardization doesn't apply—the naming is sound.Likely an incorrect or invalid review comment.
26-37: Code follows canonical damageType1 pattern — no changes needed.[Stares at search results at 3 AM with coffee-stained keyboard]
Alright, listen... I've been awake for way too long rolling for the worst rates imaginable, but at least this code is doing things right.
The search results confirm that
'assistFollowUp'is consistently used across your entire codebase — Anton, Caesar, Corin, Lucia, Soukaku, and every meta file I can see follows the exact same camelCase pattern. No'assist', no'followUp', no weird'assist_followUp'nonsense hiding anywhere.Your Manato buffs file? Perfectly aligned. Ship it. 🎮
74-86: Confirmed: unlock gating prevents unintended stacking.Both
m1_assistFollowUp_fire_dmg_andm1_basic_fire_dmg_are registered with distinctdamageType1tags and applied via separateaddWithDmgType()calls ('assistFollowUp' vs 'basic'). Each attack type receives only its corresponding buff—no cross-stacking risk. Design looks solid.
—Somnia, struggling through the 3 AM verification loop with cold coffee in one hand and a 50-roll summon ticket burning a hole in the other
110-120: Fire dmg buffs are properly separated—no double‑apply risk.Both
core_fire_dmg_andm6_fire_dmg_targetownBuff.combat.dmg_.firebut via distinct conditional gates.core_fire_dmg_activates only during Molten Edge state, whilem6_fire_dmg_requires Mindscape Level 6 and successful Assist Follow-Up hits. The conditions are independent, so they won't accidentally stack unintentionally. Damage graph stays predictable.
38-48: Need to verify Manato-specific consumer updates; search results inconclusive for this rename.[rubbing tired eyes with gamer-hand reflexively]
Okay so I ran the scanner, and... good news and bad news situation here. The script came back empty on
core_crit_(which is expected—that's your shiny new buff name), but here's where my sleep-deprived brain gets nervous: the matches forenemy_defRed_,team_dmg_, andm6_dmg_are all showing up in other characters (Caesar, Soukaku, Miyabi, Anton) and SR characters—not Manato-specific stuff.The actual problem: I can't verify from this output whether Manato had
enemy_defRed_before or if this is a net-new buff. The script didn't search for the old Manato buff identifiers (whatever they were before). Could be:
- This is a completely fresh buff entry (✓ safe)
- Or you renamed it but didn't update the locale string in
char_Manato.json(✗ oof moment when players see generic IDs)- Or something in
formula-ui/src/char/sheets/Manato.tsxstill references the old name (✗ double-oof)TL;DR: Need to manually verify Manato's locale file + any UI component that might consume this buff. The blanket search didn't catch this because it was fishing for the wrong identifiers. Resumes chugging coffee at 3 AM
| Object.values(Extra).map(({ Name, Value, Format }) => [ | ||
| `${coreStatMap[Name]}${Format.includes('%') && !isPercentStat(coreStatMap[Name]) ? '_' : ''}`, | ||
| Format.includes('%') && !isPercentStat(coreStatMap[Name]) | ||
| ? Value / PERCENT_SCALING | ||
| : coreStatMap[Name] === 'enerRegen' | ||
| ? Value / FLAT_SCALING | ||
| : Value, | ||
| : isPercentStat(coreStatMap[Name]) | ||
| ? Value / PERCENT_SCALING | ||
| : coreStatMap[Name] === 'enerRegen' | ||
| ? Value / FLAT_SCALING | ||
| : Value, | ||
| ]) |
There was a problem hiding this comment.
Somnia: Guard unknown core stat names; compute finalKey once.
Current code may crash if Extra.Name isn’t in coreStatMap (isPercentStat(undefined)). Also duplicates percent logic.
- Object.values(Extra).map(({ Name, Value, Format }) => [
- `${coreStatMap[Name]}${Format.includes('%') && !isPercentStat(coreStatMap[Name]) ? '_' : ''}`,
- Format.includes('%') && !isPercentStat(coreStatMap[Name])
- ? Value / PERCENT_SCALING
- : isPercentStat(coreStatMap[Name])
- ? Value / PERCENT_SCALING
- : coreStatMap[Name] === 'enerRegen'
- ? Value / FLAT_SCALING
- : Value,
- ])
+ Object.values(Extra)
+ .map(({ Name, Value, Format }) => {
+ const baseKey = coreStatMap[Name]
+ if (!baseKey) return null
+ const percentFmt = Format.includes('%')
+ const finalKey =
+ percentFmt && !isPercentStat(baseKey) ? (baseKey + '_') : baseKey
+ const finalVal =
+ percentFmt || isPercentStat(baseKey)
+ ? Value / PERCENT_SCALING
+ : baseKey === 'enerRegen'
+ ? Value / FLAT_SCALING
+ : Value
+ return [finalKey, finalVal]
+ })
+ .filter((e): e is [string, number] => !!e)🤖 Prompt for AI Agents
In libs/zzz/dm/src/dm/character/character.ts around lines 235 to 244, guard
against Extra.Name entries that are not keys in coreStatMap and avoid
recomputing percent checks: first resolve const coreKey = coreStatMap[Name]; if
coreKey is undefined, skip/return null for this map entry (or filter later) to
prevent calling isPercentStat(undefined); compute the finalKey once, e.g. const
finalKey = `${coreKey}${Format.includes('%') && !isPercentStat(coreKey) ? '_' :
''}`; then compute the value with a single set of conditionals (use a single
isPercent = Format.includes('%') || isPercentStat(coreKey) and apply
PERCENT_SCALING or FLAT_SCALING as appropriate) so percent logic isn’t
duplicated; ensure to filter out any null results before turning the array into
an object.
| const { consumingHp_consecutiveStrikes, moltenEdge } = allBoolConditionals(key) | ||
| const { hpTallied } = allNumConditionals( | ||
| key, | ||
| true, | ||
| 0, | ||
| dm.m1.max_dmg_ / dm.m1.assist_basic_fire_dmg_ | ||
| ) | ||
| const { assistFollowUpHitsEnemy } = allNumConditionals( | ||
| key, | ||
| true, | ||
| 0, | ||
| dm.m6.stacks | ||
| ) | ||
|
|
There was a problem hiding this comment.
Guard against divide-by-zero in hpTallied max.
dm.m1.assist_basic_fire_dmg_ could be 0 or missing in edge data; computing dm.m1.max_dmg_ / 0 yields Infinity and a cursed slider.
Apply this diff:
-const { hpTallied } = allNumConditionals(
- key,
- true,
- 0,
- dm.m1.max_dmg_ / dm.m1.assist_basic_fire_dmg_
-)
+const hpTalliedMax =
+ dm.m1?.assist_basic_fire_dmg_ ? dm.m1.max_dmg_ / dm.m1.assist_basic_fire_dmg_ : 50
+const { hpTallied } = allNumConditionals(key, true, 0, hpTalliedMax)📝 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.
| const { consumingHp_consecutiveStrikes, moltenEdge } = allBoolConditionals(key) | |
| const { hpTallied } = allNumConditionals( | |
| key, | |
| true, | |
| 0, | |
| dm.m1.max_dmg_ / dm.m1.assist_basic_fire_dmg_ | |
| ) | |
| const { assistFollowUpHitsEnemy } = allNumConditionals( | |
| key, | |
| true, | |
| 0, | |
| dm.m6.stacks | |
| ) | |
| const { consumingHp_consecutiveStrikes, moltenEdge } = allBoolConditionals(key) | |
| const hpTalliedMax = | |
| dm.m1?.assist_basic_fire_dmg_ ? dm.m1.max_dmg_ / dm.m1.assist_basic_fire_dmg_ : 50 | |
| const { hpTallied } = allNumConditionals(key, true, 0, hpTalliedMax) | |
| const { assistFollowUpHitsEnemy } = allNumConditionals( | |
| key, | |
| true, | |
| 0, | |
| dm.m6.stacks | |
| ) |
🤖 Prompt for AI Agents
In libs/zzz/formula/src/data/char/sheets/Manato.ts around lines 33–46, the
hpTallied max uses dm.m1.max_dmg_ / dm.m1.assist_basic_fire_dmg_ which can
divide by zero or undefined; replace that expression with a safe calculation:
compute a denominator variable from dm.m1.assist_basic_fire_dmg_ and if it is
falsy or zero fall back to 1 (or to dm.m1.max_dmg_), then use dm.m1.max_dmg_ /
safeDenom (or dm.m1.max_dmg_ when denom is zero) so the slider never receives
Infinity. Ensure the change preserves the existing allNumConditionals call
signature and only alters the max value expression.
| 'core_common_dmg_', | ||
| ownBuff.combat.common_dmg_.add( | ||
| cmpGE(char.mindscape, 6, boolConditional.ifOn(1)) | ||
| prod(missingHp, subscript(char.core, dm.core.common_dmg_), constant(0.02)) | ||
| ) | ||
| ), |
There was a problem hiding this comment.
🧩 Analysis chain
Unit sanity for core_common_dmg_.
missingHp * subscript(core) * 0.02 can reach large values depending on missingHp’s domain. If missingHp ∈ [0,50], multiplier hits 1.0 at 50, i.e., +100% common_dmg_. If missingHp is normalized [0,1], you’re only adding up to 2%.
Please confirm missingHp’s units and whether this should be wrapped by percent(...) anywhere.
🏁 Script executed:
#!/bin/bash
# Inspect missingHp definition/units
fd -a "Yidhari" libs/zzz/formula/src/meta/char | xargs -I{} sed -n '1,220p' {}
rg -n "missingHp" -C3 libs/zzz/formula/srcLength of output: 2492
[/run_scripts]
🏁 Script executed:
#!/bin/bash
# Find percent() definition and usage with dmg_ related buffs
rg -n "function percent|const percent|export.*percent" -A2 libs/zzz/formula/src | head -20
rg -n "common_dmg_\.add" -B2 -A2 libs/zzz/formula/src | head -40Length of output: 3131
🏁 Script executed:
#!/bin/bash
# Check for similar patterns with prod(...) and .add() on percentage stats
rg -n "prod\(" -A1 -B1 libs/zzz/formula/src/data/char/sheets/ | grep -A1 -B1 "\.add\|percent"Length of output: 28117
🏁 Script executed:
#!/bin/bash
# Look at conditionals to understand if 0-50 range uses percent wrapper elsewhere
rg -n "allNumConditionals" -A1 libs/zzz/formula/src/data/char/sheets/ | head -30Length of output: 1550
Wrap subscript(char.core, dm.core.common_dmg_) with percent().
The formula should be prod(missingHp, percent(subscript(char.core, dm.core.common_dmg_)), constant(0.02)). Across the codebase, subscript(char.core, dm.core.*_dmg_) is consistently wrapped with percent() when used in damage buff calculations (Billy.ts:35, Corin.ts:39, etc.). The current code breaks this semantic pattern and risks incorrect scaling interpretation.
🤖 Prompt for AI Agents
In libs/zzz/formula/src/data/char/sheets/Yidhari.ts around lines 61 to 65, the
expression uses subscript(char.core, dm.core.common_dmg_) directly in a prod for
a damage buff; wrap that subscript call with percent() so the product becomes
prod(missingHp, percent(subscript(char.core, dm.core.common_dmg_)),
constant(0.02)). Update the code to call percent(...) around the subscript and
keep the surrounding prod/constant structure unchanged to match the established
pattern used elsewhere.
| 'ability_crit_dmg_', | ||
| ownBuff.combat.crit_dmg_.add( | ||
| cmpGE( | ||
| sum( | ||
| team.common.count.withSpecialty('stun'), | ||
| team.common.count.withSpecialty('support') | ||
| ), | ||
| 1, | ||
| cmpEq(missingHp, 50, percent(dm.ability.crit_dmg_)) | ||
| ) | ||
| ) | ||
| ), |
There was a problem hiding this comment.
🧩 Analysis chain
Buff never procs except at exactly 50 missing HP (very likely a logic bug).
cmpEq(missingHp, 50, percent(dm.ability.crit_dmg_)) requires exact 50. That’s super narrow; users basically never hit it. Likely intended >= 50 or a scaling with missingHp.
Proposed fix: use cmpGE(missingHp, 50, percent(dm.ability.crit_dmg_)).
- cmpEq(missingHp, 50, percent(dm.ability.crit_dmg_))
+ cmpGE(missingHp, 50, percent(dm.ability.crit_dmg_))🏁 Script executed:
#!/bin/bash
# Search for similar logic patterns to fix consistently
rg -n "cmpEq\([^,]*missingHp" -C2 libs/zzz/formula/src || trueLength of output: 355
🏁 Script executed:
#!/bin/bash
# Find cmpEq and cmpGE function definitions
rg -n "^\s*(export\s+)?(function|const|type|interface)\s+(cmpEq|cmpGE)" libs/zzz/formula/src -A3Length of output: 0
🏁 Script executed:
#!/bin/bash
# Look for all missingHp uses in Yidhari.ts context
rg -n "missingHp" libs/zzz/formula/src/data/char/sheets/Yidhari.ts -B2 -A2Length of output: 441
🏁 Script executed:
#!/bin/bash
# Search for other cmpEq/cmpGE usage patterns with HP/thresholds
rg -n "cmp(Eq|GE|LE|LT|GT)\(.*[Hh]p" libs/zzz/formula/src -C1 | head -30Length of output: 1971
🏁 Script executed:
#!/bin/bash
# Find imports in Yidhari.ts to understand cmpEq/cmpGE
head -30 libs/zzz/formula/src/data/char/sheets/Yidhari.ts | grep -E "import|from"Length of output: 281
🏁 Script executed:
#!/bin/bash
# Search for cmpEq usage in entire codebase (to see typical patterns)
rg -n "cmpEq\(" libs/zzz/formula/src -B1 -A1 | head -50Length of output: 2972
🏁 Script executed:
#!/bin/bash
# Check if there are similar exact-value patterns elsewhere
rg -n "cmpEq\(.*,\s*[0-9]+" libs/zzz/formula/src | head -20Length of output: 1041
Change cmpEq to cmpGE for missing HP threshold logic.
Verified: cmpEq(missingHp, 50, ...) is the only equality check with HP in the entire codebase. Every other HP-threshold condition uses cmpGE (mindscape checks at lines 44, 102; other char sheets). With missingHp constrained to 0-50 range, exact equality at 50 fires only in that one-frame scenario—basically dead code. All comparable patterns use range checks (cmpGE), not exact values.
- cmpEq(missingHp, 50, percent(dm.ability.crit_dmg_))
+ cmpGE(missingHp, 50, percent(dm.ability.crit_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.
| 'ability_crit_dmg_', | |
| ownBuff.combat.crit_dmg_.add( | |
| cmpGE( | |
| sum( | |
| team.common.count.withSpecialty('stun'), | |
| team.common.count.withSpecialty('support') | |
| ), | |
| 1, | |
| cmpEq(missingHp, 50, percent(dm.ability.crit_dmg_)) | |
| ) | |
| ) | |
| ), | |
| 'ability_crit_dmg_', | |
| ownBuff.combat.crit_dmg_.add( | |
| cmpGE( | |
| sum( | |
| team.common.count.withSpecialty('stun'), | |
| team.common.count.withSpecialty('support') | |
| ), | |
| 1, | |
| cmpGE(missingHp, 50, percent(dm.ability.crit_dmg_)) | |
| ) | |
| ) | |
| ), |
🤖 Prompt for AI Agents
In libs/zzz/formula/src/data/char/sheets/Yidhari.ts around lines 67 to 78,
replace the equality HP check cmpEq(missingHp, 50,
percent(dm.ability.crit_dmg_)) with a greater-or-equal check cmpGE(missingHp,
50, percent(dm.ability.crit_dmg_)) so the missing HP threshold fires for
missingHp >= 50 (consistent with other character sheets and mindscape checks);
keep the surrounding structure (team counts and sum) unchanged.
| assistFollowUpHitsEnemy: { | ||
| sheet: 'Manato', | ||
| name: 'listConditional', | ||
| type: 'list', | ||
| list: ['val1', 'val2'], | ||
| name: 'assistFollowUpHitsEnemy', | ||
| type: 'num', | ||
| int_only: true, | ||
| min: 0, | ||
| max: 5, | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
Numeric ranges: align UI max with data-driven limit.
hpTallied is hard-coded max 50, but the sheet derives a dynamic max (dm.m1.max_dmg_/dm.m1.assist_basic_fire_dmg_). Mismatch can confuse users and testers.
Option A: Generate max from dm at build time for this meta file.
Option B: Keep meta wide (e.g., 100) and clamp via allNumConditionals in the sheet, documenting the source of truth.
Also applies to: 11-15, 16-23, 24-24
🏁 Script executed:
#!/bin/bash
# Ensure only one "max" is the UX source of truth
rg -n "hpTallied.*max" libs --type=ts -C2Length of output: 0
Need to dig deeper into the actual file to understand what's going on. Let me check the file contents and search for hpTallied more broadly.
---
🏁 Script executed:
```shell
#!/bin/bash
# View actual file content at the specified ranges
cat libs/zzz/formula/src/meta/char/Manato/conditionals.ts | head -30
```
Length of output: 554
---
🏁 Script executed:
```shell
#!/bin/bash
# Search for hpTallied anywhere in libs
rg -n "hpTallied" libs --type=ts -B2 -A2
```
Length of output: 2076
---
🏁 Script executed:
```shell
#!/bin/bash
# Search for max_dmg or assist_basic_fire_dmg references
rg -n "max_dmg_\|assist_basic_fire_dmg_" libs --type=ts -B2 -A2
```
Length of output: 0
---
Let me search for the dm.m1 data structure and the actual dynamic max calculation:
🏁 Script executed:
#!/bin/bash
# Search for dm.m1 references to find the data structure
rg -n "dm\.m1" libs --type=ts -B1 -A1 | head -40Length of output: 2741
🏁 Script executed:
#!/bin/bash
# Search for assist_basic_fire_dmg definitions
rg -n "assist_basic_fire_dmg_" libs --type=ts -B2 -A2 | head -50Length of output: 1563
🏁 Script executed:
#!/bin/bash
# Look for allNumConditionals to understand how conditional max values are handled
rg -n "allNumConditionals" libs --type=ts -B3 -A3Length of output: 263350
Mismatch between hard-coded UI max and data-driven limit confirmed—requires alignment.
The review comment is correct. The meta file defines hpTallied.max as hard-coded 50, while libs/zzz/formula/src/data/char/sheets/Manato.ts line 38 computes a dynamic maximum as dm.m1.max_dmg_ / dm.m1.assist_basic_fire_dmg_. This creates a constraint mismatch where the UI can differ from the actual calculation engine.
The concern is valid: when game data (dm.m1) changes, the formula-driven max updates automatically, but the UI metadata does not. Either adopt Option A (sync meta at build time) or Option B (widen UI constraint and clamp at the sheet level), but pick one source of truth and document it clearly.
| erudition: { sheet: 'Yidhari', name: 'erudition', type: 'bool' }, | ||
| etherVeil: { sheet: 'Yidhari', name: 'etherVeil', type: 'bool' }, | ||
| missingHp: { | ||
| sheet: 'Yidhari', | ||
| name: 'listConditional', | ||
| type: 'list', | ||
| list: ['val1', 'val2'], | ||
| }, | ||
| numConditional: { | ||
| sheet: 'Yidhari', | ||
| name: 'numConditional', | ||
| name: 'missingHp', | ||
| type: 'num', | ||
| int_only: true, | ||
| min: 0, | ||
| max: 2, | ||
| max: 50, | ||
| }, |
There was a problem hiding this comment.
missingHp max should be 100 (percent), not 50.
50 caps the slider at 50% missing HP; likely incorrect for a percentage input.
Apply this diff:
- missingHp: {
+ missingHp: {
sheet: 'Yidhari',
name: 'missingHp',
type: 'num',
int_only: true,
min: 0,
- max: 50,
+ max: 100,
},📝 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.
| erudition: { sheet: 'Yidhari', name: 'erudition', type: 'bool' }, | |
| etherVeil: { sheet: 'Yidhari', name: 'etherVeil', type: 'bool' }, | |
| missingHp: { | |
| sheet: 'Yidhari', | |
| name: 'listConditional', | |
| type: 'list', | |
| list: ['val1', 'val2'], | |
| }, | |
| numConditional: { | |
| sheet: 'Yidhari', | |
| name: 'numConditional', | |
| name: 'missingHp', | |
| type: 'num', | |
| int_only: true, | |
| min: 0, | |
| max: 2, | |
| max: 50, | |
| }, | |
| erudition: { sheet: 'Yidhari', name: 'erudition', type: 'bool' }, | |
| etherVeil: { sheet: 'Yidhari', name: 'etherVeil', type: 'bool' }, | |
| missingHp: { | |
| sheet: 'Yidhari', | |
| name: 'missingHp', | |
| type: 'num', | |
| int_only: true, | |
| min: 0, | |
| max: 100, | |
| }, |
🤖 Prompt for AI Agents
In libs/zzz/formula/src/meta/char/Yidhari/conditionals.ts around lines 3 to 12,
the missingHp field uses max: 50 which incorrectly caps the percentage slider at
50%; change the max to 100 so the field correctly represents 0–100% missing HP,
preserving int_only: true and min: 0.
| "flat_dmg": "Harmony Flat DMG", | ||
| "dmg_": "Harmony DMG Bonus", | ||
| "crit_": "Harmony CRIT Rate", | ||
| "crit_dmg_": "Harmony CRIT DMG" | ||
| }, |
There was a problem hiding this comment.
Somnia: “Flat” label vs % field — mismatch.
UI uses ch('harmony.flat_dmg') but binds buff.exSpecial_harmony_dmg_ (a % bonus). Switch to a % label or use fieldForBuff for consistency.
Apply in Lucia.tsx:
- {
- type: 'fields',
- fields: [
- {
- title: ch('harmony.flat_dmg'),
- fieldRef: buff.exSpecial_harmony_dmg_.tag,
- },
- ],
- },
+ {
+ type: 'fields',
+ fields: [fieldForBuff(buff.exSpecial_harmony_dmg_)],
+ },Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
libs/zzz/formula-ui/src/char/CharStatsDisplay.tsx (1)
77-80: Nice consistency with the FieldDisplay pattern! ✨sips energy drink at 3 AM while staring at this code
The
computedTagmerge logic looks solid and matches what you did inFieldDisplay.tsx. The memoization is properly set up, and spreading order means the originaltagproperties take precedence over computed metadata—which seems intentional based on the pattern.However, I'm noticing this exact same merge pattern is now in multiple files. Like farming the same domain for artifacts every day... it works, but it gets repetitive. Consider extracting this into a shared hook like
useComputedTag(tag, computed)to avoid the copy-paste grind.Suggested refactor to reduce duplication:
Create a shared hook in a common location:
// In a shared hooks file export function useComputedTag(tag: Tag, computed?: CalcResult) { return useMemo( () => ({ ...computed?.meta.tag, ...tag }), [tag, computed?.meta.tag] ) }Then use it in both files:
- const computedTag = useMemo( - () => ({ ...computed?.meta.tag, ...tag }), - [tag, computed?.meta.tag] - ) + const computedTag = useComputedTag(tag, computed)Also applies to: 125-125
libs/zzz/formula/src/data/util/sheet.ts (1)
107-130: nonDmg joins the damage type party.The addition of
'anomBuildup'and'nonDmg'to theregisterFormulaunion type looks correct. The logic properly handles the new category.Quick thought though—what exactly IS
nonDmg? Like, is it for utility abilities, buff applications, or something else? A brief comment explaining when to usenonDmgvsstandardDmgwould help future maintainers (and my 2 AM brain) understand the distinction. I'm guessing it's for abilities that trigger but don't directly deal damage, but confirmation would be nice.Consider adding a brief comment above the
qparameter explaining the semantic difference between the damage categories:... q: + // 'standardDmg' for direct damage abilities, 'nonDmg' for utility/buff triggers | 'standardDmg' | 'sheerDmg'libs/zzz/formula/src/data/char/util.ts (1)
212-236: Consider documenting the default false case behavior.sips energy drink The conditional damage tag switcher looks solid - nice addition for handling those gacha character gimmicks where damage type changes based on team comp or whatever.
Minor nitpick:
dmgTagIfFalsedefaults to an empty object, which means you'll still read damage but with minimal tags. Might want to clarify in the JSDoc that this doesn't "disable" the damage, just reads it with default tags. For someone as sleep-deprived as me, that could cause confusion at 4 AM./** * Creates a conditional formula node that reads from one of two damage instances based on a condition * @param cond Condition node * @param q One of 'standardDmg', 'sheerDmg' or 'anomalyDmg' * @param dmgTagIfTrue DmgTag to use if condition is true - * @param dmgTagIfFalse DmgTag to use if condition is false + * @param dmgTagIfFalse DmgTag to use if condition is false (defaults to empty tag, still reads damage) * @returns TagMapNodeEntry representing the conditional damage instance */
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
libs/game-opt/sheet-ui/src/components/FieldDisplay.tsx(1 hunks)libs/zzz/formula-ui/src/char/CharStatsDisplay.tsx(2 hunks)libs/zzz/formula-ui/src/char/util.ts(1 hunks)libs/zzz/formula/src/data/char/sheets/Soldier0Anby.ts(3 hunks)libs/zzz/formula/src/data/char/util.ts(5 hunks)libs/zzz/formula/src/data/util/sheet.ts(3 hunks)libs/zzz/formula/src/meta/char/Soldier0Anby/formulas.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
libs/game-opt/sheet-ui/src/components/FieldDisplay.tsx (1)
libs/zzz/formula-ui/src/components/TagDisplay.tsx (1)
FullTagDisplay(35-54)
libs/zzz/formula-ui/src/char/CharStatsDisplay.tsx (1)
libs/game-opt/sheet-ui/src/components/FieldDisplay.tsx (1)
FormulaHelpIcon(234-284)
libs/zzz/formula/src/data/char/util.ts (3)
libs/zzz/formula/src/data/util/tag.ts (2)
own(227-227)ownBuff(239-239)libs/zzz/formula/src/data/util/sheet.ts (1)
DmgTag(20-22)libs/pando/engine/src/node/construction.ts (3)
cmpEq(87-94)read(318-320)subscript(255-260)
libs/zzz/formula/src/data/char/sheets/Soldier0Anby.ts (4)
libs/pando/engine/src/node/construction.ts (1)
cmpGE(142-149)libs/zzz/formula/src/data/util/tag.ts (1)
team(228-228)libs/zzz/formula/src/data/util/sheet.ts (1)
register(24-36)libs/zzz/formula/src/data/char/util.ts (4)
entriesForChar(386-521)registerAllDmgDazeAndAnom(246-286)dmgDazeAndAnomOverride(181-210)conditionalFormulaDmgTag(220-236)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: call-build / build
- GitHub Check: typecheck
- GitHub Check: gen-file
- GitHub Check: test
- GitHub Check: format
- GitHub Check: lint
- GitHub Check: call-build / build
🔇 Additional comments (15)
libs/zzz/formula-ui/src/char/CharStatsDisplay.tsx (1)
129-129: Complete refactor to computedTag—looking good! 👍Both
TagDisplayandFormulaHelpIconnow consistently receive the enrichedcomputedTaginstead of the rawtag. This centralizes the tag augmentation logic, which is exactly what you want when you're too tired to debug "why does this component show different tag data than that one" at 2 AM.libs/game-opt/sheet-ui/src/components/FieldDisplay.tsx (1)
254-257: LGTM—tooltip now shows enriched tag metadata! 💤yawns while reviewing this tooltip code
The
FormulaHelpIcontooltip now gets the enrichedcomputedTaginstead of the rawtag. This is exactly parallel to what you did inCharStatsDisplay.tsx. The merge logic and memoization are solid—consistent implementation across both files means fewer "wait, why does the tooltip show different data?" debugging sessions when you're pulling an all-nighter.Same note as before though: this pattern is duplicated between files. The shared hook suggestion from the previous comment would apply here too.
Also applies to: 263-263
libs/zzz/formula-ui/src/char/util.ts (1)
10-15: The original review comment is incorrect —nonDmgis a legitimate damage category that requires damage type extraction.rubs eyes blearily Okay, so I was sleep-deprived when I filed this one. Turns out
nonDmgactually DOES have damage types. Found it in the real data — Soldier0Anby's formulas tagnonDmgwithdamageType1: 'chain'anddamageType1: 'ult'.The function is working exactly as intended:
nonDmgis a valid tag category (not "non-damage", just a different damage classification)- It has
damageType1anddamageType2properties that need extraction viagetDmgType()- Therefore, it correctly needs to pass the
isDmg()checkYes, the naming is confusing —
nonDmgdoesn't clearly communicate "this is a damage category with type info" — but that's a separate UX/naming concern, not a bug. The logic is sound.No changes needed. This one's on me for not checking the actual character data before rage-reviewing at 3 AM. Gonna go spend my daily resin now...
libs/zzz/formula/src/meta/char/Soldier0Anby/formulas.ts (3)
653-666: Ultimate that doesn't do damage? My sleep-deprived brain is confused.Same issue here—changing
UltimateVoidstrike_0_dmgfromstandardDmgtononDmg. An Ultimate with_dmgin the name being categorized as non-damage feels... strange? Like pulling a 5-star and realizing it's actually a 3-star in disguise.Plus, again, the PR said no Anby changes. Need to confirm this is intentional and semantically correct.
1-1: These changes should come from regenerating the file, not manual edits—verify your build process was run.This is indeed a generated file. The
gen-descexecutor atlibs/zzz/formula/src/executors/gen-desc/executor.tsregeneratesformulas.tsfrom source data whenevernx run zzz/formula:gen-fileis executed. IfSoldier0Anby/formulas.tschanged (lines 611–624 and 653–666 showing damage type transitions), this must be from running the generator after modifying the underlying data source, not from direct edits.However, the PR description states "Does not include changes for Soldier 0 - Anby," which contradicts the modifications present in this file. Either:
- The changes are intentional and the description needs updating
- The file needs to be regenerated to reflect actual source data
Run
nx run-many -t gen-fileto ensure this file reflects your current source data, then verify the changes align with your PR intent.
611-624: stares at screen, eyes widening slightlyOhhhhh no. No no no. The commit "add soldier 0 anby conditional dmg tags" is DEFINITELY in this PR. So either:
- The PR description is lying — says no Anby changes but here we are
- Changes accidentally leaked in — someone committed this and didn't notice the description was wrong
Plus I found the real inconsistency: every other character's chain attack uses
q: 'standardDmg'(Corin, JuFufu, etc.), but Anby's isq: 'nonDmg'. That's genuinely sus from a semantic standpoint—why would THIS chain attack be different?
Resolve the discrepancy between PR description and Anby changes, and verify the semantic correctness of marking a Chain Attack as "non-damage."
The PR description states "Does not include changes for Soldier 0 - Anby," yet the commit "add soldier 0 anby conditional dmg tags" is included in this PR branch, introducing
q: 'nonDmg'forChainAttackLeapingThunderstrike_0_dmg. Additionally, all other chain attacks across the codebase (Corin, JuFufu, etc.) useq: 'standardDmg'— Anby'snonDmgmarking appears inconsistent and needs verification that this is intentional and semantically correct for damage calculations.libs/zzz/formula/src/data/util/sheet.ts (2)
14-18: New condDmg flag added—backward compatible, nicely done.sips energy drink The optional
condDmg?: booleaninFormulaArglooks good. It's optional with a proper default, so existing code won't break. This is the kind of non-breaking change I appreciate at 4 AM when I'm too tired to refactor a dozen call sites.
147-162: ---The concern about other damage functions is based on a misunderstanding of game mechanics.
After checking the codebase, this is intentional design. Only
customDmgneedscondDmgbecause standard damage is the only damage type that can conditionally be categorized as "nonDmg" in ZZZ mechanics.
customSheerDmg,customAnomalyDmg, andcustomDazealways use their categorical damage types ('sheerDmg', 'anomalyDmg', 'dazeBuildup') and never have conditional variants. No usage patterns in the codebase suggest these damage types need the "non-damage" categorization—that concept only applies to standard damage.The PR change is correct and complete as-is. No TODOs or FIXMEs indicate pending work on other functions.
Likely an incorrect or invalid review comment.
libs/zzz/formula/src/data/char/util.ts (4)
2-2: New imports look good.yawns The
NumNodetype andreadfunction imports are exactly what you need for the new conditional damage tag helper. Coffee-driven code at its finest.Also applies to: 9-9
132-150: Same formula change applied here.The merge function got the same treatment. At least you're consistent with your 3 AM math decisions.
428-432: Document the hp_ calculation layer change with game design context.Alright, I found something that definitely needs clarity (and probably more coffee ☕). The code is treating
hp_as special by routing it toownBuff.initialinstead ofownBuff.baselikeatk_anddef_. Here's what I discovered:The evidence:
- Characters like Lucia, Manato, and Yidhari have
hp_core stats- Weapon engine sub stats also use
ownBuff.initiallayer (lines 37-51 in wengine/util.ts)- But
atk_anddef_core stats stay inownBuff.basedespite being percentage stats- Other percent stats scatter between layers (crit_, dmg_, pen_ all use initial in tests)
The question:
This looks either like intentional ZZZ game design (hp% scales differently than atk%/def%) OR an inconsistency that snuck in. Without seeing a test that validates this behavior or a comment explaining why hp% is special, I can't definitively say which.Add a comment explaining if this is game-specific (e.g., "HP% bonuses apply before other multipliers, unlike ATK%/DEF%") or verify against actual game mechanics to confirm it's correct.
76-82: The formula change is intentional and documented.Looking at the code in
libs/zzz/formula/src/data/char/util.ts, the pattern you're questioning is actually well-explained right there in the same file. Line 389-390 in theentriesForCharfunction has a comment that explicitly states: "The 'add' only applies to currLvl - 1, since 'base' is stat at lvl 1"This (level - 1) scaling is consistent across:
dmgDazeAndAnom(lines 79, 91)dmgDazeAndAnomMerge(lines 135, 147)- Base stat growth in
entriesForChar(line 390)So yes, someone definitely did think through this design—no all-nighter math here. The base values already represent level 1 stats, so growth only compounds for levels 2+. It's a consistent formula throughout the file, not a breaking change that snuck in.
libs/zzz/formula/src/data/char/sheets/Soldier0Anby.ts (3)
1-1: Imports look good.nods approvingly while downing another energy drink All the new utility imports are actually used in the refactored code. No dead imports here - unlike my sleep schedule.
Also applies to: 16-22
33-42: Good refactor extracting team comp check.yawns Nice, you extracted the "do I have a stun or support slave in my team" check into a helper. Way better than copy-pasting that
cmpGE(sum(team.common.count...))spaghetti everywhere like you're doing in lines 109-130. Peak gacha teammate synergy coding.The logic's sound: at least 1 stun OR support character = return
a, otherwise returnb. Clean.
50-77: Duplicate conditional logic appears intentional; verify thecondDmg + conditionalFormulaDmgTaginteraction.Man, I'm running on fumes here, but I found something interesting in the function signatures. cracks knuckles
The duplicate aftershock logic? That's probably fine.
ability_check(1)almost certainly means "team has ≥1 stun/support character," and both your chain and ult abilities triggering aftershock under identical conditions is solid game design—not copy-paste laziness.BUT HERE'S THE SUSPICIOUS BIT: You're passing
{ condDmg: true }alongsideconditionalFormulaDmgTag(ability_check(1), 'standardDmg', ...)to both abilities.When
condDmg: true, the base damage quality gets marked as'nonDmg'(not'standardDmg'). Then you layerconditionalFormulaDmgTagwhich adds a'standardDmg'tag on top. This combo doesn't appear anywhere else in the formula system.The structure is technically valid—you're creating a non-damage base with conditional standard-damage overlays—but it's unconventional. Either:
- This is intentional new behavior for these chain/ult abilities (they're conditional-only aftershock damage?)
- You meant
condDmg: falseor should ditch one of the conditional layers?Since I'm too sleep-deprived to verify the ZZZ game mechanics themselves, you'll need to manually confirm this plays nice with your damage calculation pipeline. The code works structurally, but the pattern is weird enough to warrant a second glance before shipping.
| coreStats: Object.values(raw.ExtraLevel).map( | ||
| ({ Extra }) => | ||
| Object.fromEntries( | ||
| Object.values(Extra).map(({ Name, Value }) => [ |
There was a problem hiding this comment.
Probably time to expand this to a proper block of code with variable names and if statements
| () => computed && formulaText(computed as any, formulaTextCache), | ||
| [computed, formulaText, formulaTextCache] | ||
| ) | ||
| const computedTag = useMemo( |
There was a problem hiding this comment.
Im a little worried we need to manually merge the tags, what happened? Is it because of conditional DMG type?
| export function isDmg(tag: Tag) { | ||
| const { q } = tag | ||
| return ['standardDmg', 'anomalyDmg', 'sheerDmg'].includes(q as string) | ||
| return ['standardDmg', 'anomalyDmg', 'sheerDmg', 'nonDmg'].includes( |
There was a problem hiding this comment.
How confusing that nondmg is considered DMG lol. Should we rename this? Or maybe a better solution (idk one)?
Describe your changes
Sheets for Manato, Lucia and Yidhari. Also changes Soldier 0 Anby sheet
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
Improvements