SRO: Add traces to sheets #2562
Conversation
WalkthroughThe pull request introduces several changes across multiple files, primarily enhancing localization support for character sheets and abilities in various languages. Key modifications include the addition of new JSON files for localization in multiple languages (e.g., Chinese, German, Spanish, etc.), updates to existing localization files to correct formatting issues, and improvements in the handling of character statistics and abilities. Additionally, new utility functions and constants are introduced to streamline the management of character data and enhance the UI components related to talents and abilities. Changes
Assessment against linked issues
Possibly related PRs
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 (
|
|
[sr-frontend] [Wed Dec 4 22:06:33 UTC 2024] - Deployed 772029b to https://genshin-optimizer-prs.github.io/pr/2562/sr-frontend (Takes 3-5 minutes after this completes to be available) [sr-frontend] [Wed Dec 4 22:10:54 UTC 2024] - Deployed 772029b to https://genshin-optimizer-prs.github.io/pr/2562/sr-frontend (Takes 3-5 minutes after this completes to be available) [sr-frontend] [Thu Dec 5 03:02:33 UTC 2024] - Deployed 6100d74 to https://genshin-optimizer-prs.github.io/pr/2562/sr-frontend (Takes 3-5 minutes after this completes to be available) [sr-frontend] [Thu Dec 5 09:51:18 UTC 2024] - Deployed 7cc69f6 to https://genshin-optimizer-prs.github.io/pr/2562/sr-frontend (Takes 3-5 minutes after this completes to be available) [sr-frontend] [Fri Dec 6 02:24:01 UTC 2024] - Deployed 1a7fe38 to https://genshin-optimizer-prs.github.io/pr/2562/sr-frontend (Takes 3-5 minutes after this completes to be available) [sr-frontend] [Fri Dec 6 03:35:59 UTC 2024] - Deployed e6debe1 to https://genshin-optimizer-prs.github.io/pr/2562/sr-frontend (Takes 3-5 minutes after this completes to be available) [sr-frontend] [Fri Dec 6 04:29:21 UTC 2024] - Deployed 3239815 to https://genshin-optimizer-prs.github.io/pr/2562/sr-frontend (Takes 3-5 minutes after this completes to be available) [Sat Dec 7 03:17:52 UTC 2024] - 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: 7
🧹 Outside diff range and nitpick comments (10)
libs/pando/ui-sheet/src/components/DocumentDisplay.tsx (1)
71-75: Consider adding Typography variant and color props.While wrapping the text in Typography is good, consider adding props to support the PR's objective of making traces fade out when not enabled.
Consider updating the component like this:
- <Typography> + <Typography + variant="body1" + color={textDocument.disabled ? 'text.disabled' : 'text.primary'} + >This would:
- Explicitly set the text style for consistency
- Support the fade-out requirement for disabled traces
libs/sr/formula-ui/src/char/sheets/RuanMei.tsx (1)
40-42: Address pending translations marked with TODO commentsSeveral sections of the code contain
// TODO: translatecomments, indicating that translation strings are missing for titles and labels. Completing these translations will enhance the user experience by providing localized content.Would you like assistance in generating the required translations or creating placeholders until translations are available?
Also applies to: 80-85, 89-91, 105-110, 138-143, 162-171, 174-193
libs/sr/dm-localization/src/executors/gen-locale/executor.ts (1)
Line range hint
1-45: Consider adding type safety for localization namespacesThe array of valid localization types could be defined as a constant or enum to prevent typos and improve maintainability.
+const LOCALIZATION_NAMESPACES = [ + 'characters', + 'charNames', + 'charSheet', + 'common', + 'lightCones', + 'lightConeNames', + 'relics', + 'relicNames', + 'paths', + 'statKey', + 'teams', + 'ui', +] as const; + if ( - [ - 'characters', - 'charNames', - 'charSheet', - // ... - ].includes(type) + LOCALIZATION_NAMESPACES.includes(type) )libs/sr/stats/src/char.ts (1)
22-31: Improve type definition readability and documentationWhile the addition of bonus ability types is correct, consider:
- The type definition could be more readable using a type union
- The JSDoc comment above needs updating to include the new bonus ability types
Consider refactoring to:
- skType: - | 'basic' - | 'skill' - | 'ult' - | 'talent' - | 'technique' - | 'eidolon' - | 'bonusAbility1' - | 'bonusAbility2' - | 'bonusAbility3', + type SkillType = + 'basic' | 'skill' | 'ult' | 'talent' | 'technique' | 'eidolon' | + 'bonusAbility1' | 'bonusAbility2' | 'bonusAbility3' + + skType: SkillType,Also, update the JSDoc to include documentation for the new bonus ability types.
libs/sr/formula-ui/src/char/StatBoostBonusAbility.tsx (3)
10-14: Consider enhancing type safety for ReqMapEntryThe
ReqMapEntrytype could benefit from stricter typing for the calculator functions.Consider:
export type ReqMapEntry = { subtitle: React.ReactNode - onClickable: (calc: Calculator) => boolean - disabled: (calc: Calculator) => boolean + onClickable: (calc: Calculator<CharacterKey>) => boolean + disabled: (calc: Calculator<CharacterKey>) => boolean }
15-29: Reduce code duplication in requirement functionsThe
reqMapEntryAscandreqMapEntryLvlfunctions share similar structure and could be refactored to reduce duplication.Consider creating a common factory function:
function createReqMapEntry( talentKey: TalentSheetElementStatBoostKey | TalentSheetElementBonusAbilityKey, requirement: { type: 'ascension', value: AscensionKey } | { type: 'level', value: number } ): ReqMapEntry { const { type, value } = requirement; return { subtitle: ( <Translate ns="charSheet_gen" key18={`req_${type}`} values={{ 1: value }} /> ), onClickable: (calc: Calculator) => calc.compute(own.char[type]).val >= value, disabled: (calc: Calculator) => calc.compute(own.char[type]).val < value || !calc.compute(own.char[talentKey]).val, } }Also applies to: 30-43
69-71: Extract magic numbers into named constantsLevel requirements like 75 and 80 should be defined as named constants for better maintainability.
Consider:
+ const CHARACTER_LEVEL_REQUIREMENTS = { + STAT_BOOST_9: 75, + STAT_BOOST_10: 80, + } as const; - statBoost9: reqMapEntryLvl('statBoost9', 75), - statBoost10: reqMapEntryLvl('statBoost10', 80), + statBoost9: reqMapEntryLvl('statBoost9', CHARACTER_LEVEL_REQUIREMENTS.STAT_BOOST_9), + statBoost10: reqMapEntryLvl('statBoost10', CHARACTER_LEVEL_REQUIREMENTS.STAT_BOOST_10),libs/sr/formula-ui/src/char/sheetUtil.tsx (1)
33-59: LGTM! Consider adding JSDoc comments.The function is well-structured and type-safe. Consider adding JSDoc comments to document the parameters and return type for better maintainability.
+/** + * Generates a UI sheet element for character talents + * @param ckey - Character gender-specific key + * @param talentKey - Type of talent ('basic' | 'skill' | 'ult' | 'talent' | 'technique') + * @param docs - Optional additional documents + * @returns UISheetElement containing the talent information + */ export function talentSheet(libs/sr/dm-localization/src/executors/gen-locale/lib/stringData.ts (2)
45-46: Document the limitation in README or API docs.The FIXME comment identifies a limitation with nested color tags. This should be documented in the project's README or API documentation to inform other developers.
68-70: Consider consolidating regex stages as suggested.The TODO suggests consolidating the simple placeholder regex into previous stages. This would improve maintainability and potentially performance.
Consider combining this with the earlier regex stages by extending the
match2pattern to also capture simple#npatterns:- const match2 = new RegExp(/(?:#(.*?)\[(.*?)\](.*?)|(.*?))/) + const match2 = new RegExp(/(?:#(\d{1,2})(?:\[(.*?)\](.*?))?|(.*?))/)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (81)
libs/pando/ui-sheet/src/components/DocumentDisplay.tsx(2 hunks)libs/pando/ui-sheet/src/components/FieldDisplay.tsx(2 hunks)libs/pando/ui-sheet/src/types/sheet.ts(1 hunks)libs/sr/dm-localization/assets/locales/chs/charSheet_gen.json(1 hunks)libs/sr/dm-localization/assets/locales/chs/char_Acheron_gen.json(1 hunks)libs/sr/dm-localization/assets/locales/chs/char_March7thTheHunt_gen.json(1 hunks)libs/sr/dm-localization/assets/locales/chs/sheet_gen.json(0 hunks)libs/sr/dm-localization/assets/locales/chs/slotKey_gen.json(0 hunks)libs/sr/dm-localization/assets/locales/cht/charSheet_gen.json(1 hunks)libs/sr/dm-localization/assets/locales/cht/char_Acheron_gen.json(1 hunks)libs/sr/dm-localization/assets/locales/cht/char_March7thTheHunt_gen.json(1 hunks)libs/sr/dm-localization/assets/locales/cht/sheet_gen.json(0 hunks)libs/sr/dm-localization/assets/locales/cht/slotKey_gen.json(0 hunks)libs/sr/dm-localization/assets/locales/de/charSheet_gen.json(1 hunks)libs/sr/dm-localization/assets/locales/de/char_Acheron_gen.json(1 hunks)libs/sr/dm-localization/assets/locales/de/char_March7thTheHunt_gen.json(1 hunks)libs/sr/dm-localization/assets/locales/de/sheet_gen.json(0 hunks)libs/sr/dm-localization/assets/locales/de/slotKey_gen.json(0 hunks)libs/sr/dm-localization/assets/locales/en/charSheet_gen.json(1 hunks)libs/sr/dm-localization/assets/locales/en/char_March7thTheHunt_gen.json(1 hunks)libs/sr/dm-localization/assets/locales/en/sheet_gen.json(0 hunks)libs/sr/dm-localization/assets/locales/en/slotKey_gen.json(0 hunks)libs/sr/dm-localization/assets/locales/es/charSheet_gen.json(1 hunks)libs/sr/dm-localization/assets/locales/es/char_Acheron_gen.json(1 hunks)libs/sr/dm-localization/assets/locales/es/char_March7thTheHunt_gen.json(1 hunks)libs/sr/dm-localization/assets/locales/es/sheet_gen.json(0 hunks)libs/sr/dm-localization/assets/locales/es/slotKey_gen.json(0 hunks)libs/sr/dm-localization/assets/locales/fr/charSheet_gen.json(1 hunks)libs/sr/dm-localization/assets/locales/fr/char_March7thTheHunt_gen.json(1 hunks)libs/sr/dm-localization/assets/locales/fr/sheet_gen.json(0 hunks)libs/sr/dm-localization/assets/locales/fr/slotKey_gen.json(0 hunks)libs/sr/dm-localization/assets/locales/id/charSheet_gen.json(1 hunks)libs/sr/dm-localization/assets/locales/id/char_Acheron_gen.json(1 hunks)libs/sr/dm-localization/assets/locales/id/char_March7thTheHunt_gen.json(1 hunks)libs/sr/dm-localization/assets/locales/id/sheet_gen.json(0 hunks)libs/sr/dm-localization/assets/locales/id/slotKey_gen.json(0 hunks)libs/sr/dm-localization/assets/locales/ja/charSheet_gen.json(1 hunks)libs/sr/dm-localization/assets/locales/ja/char_March7thTheHunt_gen.json(1 hunks)libs/sr/dm-localization/assets/locales/ja/char_RuanMei_gen.json(1 hunks)libs/sr/dm-localization/assets/locales/ja/lightCone_SailingTowardsASecondLife_gen.json(1 hunks)libs/sr/dm-localization/assets/locales/ja/sheet_gen.json(0 hunks)libs/sr/dm-localization/assets/locales/ja/slotKey_gen.json(0 hunks)libs/sr/dm-localization/assets/locales/ko/charSheet_gen.json(1 hunks)libs/sr/dm-localization/assets/locales/ko/char_Acheron_gen.json(1 hunks)libs/sr/dm-localization/assets/locales/ko/char_March7thTheHunt_gen.json(1 hunks)libs/sr/dm-localization/assets/locales/ko/sheet_gen.json(0 hunks)libs/sr/dm-localization/assets/locales/ko/slotKey_gen.json(0 hunks)libs/sr/dm-localization/assets/locales/pt/charSheet_gen.json(1 hunks)libs/sr/dm-localization/assets/locales/pt/char_Gepard_gen.json(1 hunks)libs/sr/dm-localization/assets/locales/pt/char_March7thTheHunt_gen.json(1 hunks)libs/sr/dm-localization/assets/locales/pt/char_Yanqing_gen.json(1 hunks)libs/sr/dm-localization/assets/locales/pt/sheet_gen.json(0 hunks)libs/sr/dm-localization/assets/locales/pt/slotKey_gen.json(0 hunks)libs/sr/dm-localization/assets/locales/ru/charSheet_gen.json(1 hunks)libs/sr/dm-localization/assets/locales/ru/char_Acheron_gen.json(1 hunks)libs/sr/dm-localization/assets/locales/ru/char_March7thTheHunt_gen.json(1 hunks)libs/sr/dm-localization/assets/locales/ru/sheet_gen.json(0 hunks)libs/sr/dm-localization/assets/locales/ru/slotKey_gen.json(0 hunks)libs/sr/dm-localization/assets/locales/th/charSheet_gen.json(1 hunks)libs/sr/dm-localization/assets/locales/th/char_Acheron_gen.json(1 hunks)libs/sr/dm-localization/assets/locales/th/char_March7thTheHunt_gen.json(1 hunks)libs/sr/dm-localization/assets/locales/th/sheet_gen.json(0 hunks)libs/sr/dm-localization/assets/locales/th/slotKey_gen.json(0 hunks)libs/sr/dm-localization/assets/locales/vi/charSheet_gen.json(1 hunks)libs/sr/dm-localization/assets/locales/vi/char_Acheron_gen.json(1 hunks)libs/sr/dm-localization/assets/locales/vi/char_March7thTheHunt_gen.json(1 hunks)libs/sr/dm-localization/assets/locales/vi/sheet_gen.json(0 hunks)libs/sr/dm-localization/assets/locales/vi/slotKey_gen.json(0 hunks)libs/sr/dm-localization/src/executors/gen-locale/executor.ts(1 hunks)libs/sr/dm-localization/src/executors/gen-locale/lib/charHashData/charSheet.ts(1 hunks)libs/sr/dm-localization/src/executors/gen-locale/lib/charHashData/index.ts(1 hunks)libs/sr/dm-localization/src/executors/gen-locale/lib/hashData.ts(2 hunks)libs/sr/dm-localization/src/executors/gen-locale/lib/stringData.ts(2 hunks)libs/sr/formula-ui/src/char/SkillSubtitle.tsx(1 hunks)libs/sr/formula-ui/src/char/StatBoostBonusAbility.tsx(1 hunks)libs/sr/formula-ui/src/char/consts.ts(2 hunks)libs/sr/formula-ui/src/char/index.ts(1 hunks)libs/sr/formula-ui/src/char/sheetUtil.tsx(1 hunks)libs/sr/formula-ui/src/char/sheets/RuanMei.tsx(1 hunks)libs/sr/page-team/src/TalentContent.tsx(5 hunks)libs/sr/stats/src/char.ts(1 hunks)
💤 Files with no reviewable changes (26)
- libs/sr/dm-localization/assets/locales/th/slotKey_gen.json
- libs/sr/dm-localization/assets/locales/cht/slotKey_gen.json
- libs/sr/dm-localization/assets/locales/ko/slotKey_gen.json
- libs/sr/dm-localization/assets/locales/ja/slotKey_gen.json
- libs/sr/dm-localization/assets/locales/es/slotKey_gen.json
- libs/sr/dm-localization/assets/locales/vi/slotKey_gen.json
- libs/sr/dm-localization/assets/locales/chs/slotKey_gen.json
- libs/sr/dm-localization/assets/locales/id/slotKey_gen.json
- libs/sr/dm-localization/assets/locales/cht/sheet_gen.json
- libs/sr/dm-localization/assets/locales/th/sheet_gen.json
- libs/sr/dm-localization/assets/locales/ru/sheet_gen.json
- libs/sr/dm-localization/assets/locales/en/slotKey_gen.json
- libs/sr/dm-localization/assets/locales/ko/sheet_gen.json
- libs/sr/dm-localization/assets/locales/en/sheet_gen.json
- libs/sr/dm-localization/assets/locales/es/sheet_gen.json
- libs/sr/dm-localization/assets/locales/pt/sheet_gen.json
- libs/sr/dm-localization/assets/locales/de/sheet_gen.json
- libs/sr/dm-localization/assets/locales/ja/sheet_gen.json
- libs/sr/dm-localization/assets/locales/chs/sheet_gen.json
- libs/sr/dm-localization/assets/locales/fr/slotKey_gen.json
- libs/sr/dm-localization/assets/locales/de/slotKey_gen.json
- libs/sr/dm-localization/assets/locales/pt/slotKey_gen.json
- libs/sr/dm-localization/assets/locales/ru/slotKey_gen.json
- libs/sr/dm-localization/assets/locales/fr/sheet_gen.json
- libs/sr/dm-localization/assets/locales/id/sheet_gen.json
- libs/sr/dm-localization/assets/locales/vi/sheet_gen.json
✅ Files skipped from review due to trivial changes (27)
- libs/sr/dm-localization/assets/locales/pt/char_Gepard_gen.json
- libs/sr/dm-localization/assets/locales/chs/char_Acheron_gen.json
- libs/sr/dm-localization/src/executors/gen-locale/lib/charHashData/index.ts
- libs/sr/dm-localization/assets/locales/pt/char_March7thTheHunt_gen.json
- libs/sr/dm-localization/assets/locales/cht/char_Acheron_gen.json
- libs/sr/dm-localization/assets/locales/es/char_Acheron_gen.json
- libs/sr/dm-localization/assets/locales/en/char_March7thTheHunt_gen.json
- libs/sr/dm-localization/assets/locales/vi/charSheet_gen.json
- libs/sr/dm-localization/assets/locales/cht/charSheet_gen.json
- libs/sr/dm-localization/assets/locales/ko/char_Acheron_gen.json
- libs/sr/dm-localization/assets/locales/th/charSheet_gen.json
- libs/sr/dm-localization/assets/locales/fr/char_March7thTheHunt_gen.json
- libs/sr/dm-localization/assets/locales/en/charSheet_gen.json
- libs/sr/dm-localization/assets/locales/de/char_Acheron_gen.json
- libs/sr/dm-localization/assets/locales/ru/char_March7thTheHunt_gen.json
- libs/sr/dm-localization/assets/locales/chs/charSheet_gen.json
- libs/sr/dm-localization/assets/locales/vi/char_Acheron_gen.json
- libs/sr/dm-localization/assets/locales/ja/char_March7thTheHunt_gen.json
- libs/sr/dm-localization/assets/locales/vi/char_March7thTheHunt_gen.json
- libs/sr/dm-localization/assets/locales/de/charSheet_gen.json
- libs/sr/dm-localization/assets/locales/ko/charSheet_gen.json
- libs/sr/dm-localization/assets/locales/id/charSheet_gen.json
- libs/sr/dm-localization/assets/locales/ru/charSheet_gen.json
- libs/sr/dm-localization/assets/locales/es/charSheet_gen.json
- libs/sr/dm-localization/assets/locales/ko/char_March7thTheHunt_gen.json
- libs/sr/dm-localization/assets/locales/id/char_March7thTheHunt_gen.json
- libs/sr/dm-localization/assets/locales/pt/charSheet_gen.json
🧰 Additional context used
🪛 Biome (1.9.4)
libs/sr/page-team/src/TalentContent.tsx
[error] 92-92: Missing key property for this element in iterable.
The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.
(lint/correctness/useJsxKeyInIterable)
🔇 Additional comments (26)
libs/sr/dm-localization/assets/locales/ru/char_Acheron_gen.json (1)
16-16: LGTM! Template syntax standardization looks good.
The change from #3[i] to {{3}}[i] standardizes the template syntax for dynamic values, which is consistent with modern localization practices.
Let's verify the template syntax consistency across other language files:
libs/sr/dm-localization/assets/locales/th/char_Acheron_gen.json (1)
16-16: Verify placeholder format consistency across all abilities
The placeholder format change from #3[i] to {{3}}[i] in the shortDesc improves consistency with mustache syntax. However, we should ensure this format is used consistently across all ability descriptions.
✅ Verification successful
Placeholder format is consistent across all localizations
The new {{3}}[i] format is consistently used across all language versions of the Acheron character file, including Thai (th), Chinese (chs/cht), English (en), and other localizations. No instances of the old #number[i] format were found, confirming that the change maintains the standardized mustache syntax throughout the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining old-style placeholders (#number) in localization files
# Test: Search for old placeholder format. Expect: No matches for #number[i] pattern
echo "Checking for old placeholder format in Thai localization..."
rg '#\d+\[i\]' 'libs/sr/dm-localization/assets/locales/th/*.json'
echo "Checking for old placeholder format in all localizations..."
rg '#\d+\[i\]' 'libs/sr/dm-localization/assets/locales/**/*.json'
Length of output: 571
Script:
#!/bin/bash
# Let's first check if the directory structure is different
echo "Checking directory structure..."
fd -t d locales
echo "Searching for json files containing 'char_Acheron'..."
fd -e json . | grep -i acheron
echo "Checking for old placeholder format in found files..."
fd -e json . | grep -i acheron | xargs rg '#\d+\[i\]'
echo "Checking for new placeholder format in found files..."
fd -e json . | grep -i acheron | xargs rg '\{\{\d+\}\}\[i\]'
Length of output: 4650
libs/sr/dm-localization/assets/locales/id/char_Acheron_gen.json (1)
16-16: LGTM! Format change is consistent with other localizations
The placeholder format change matches the Thai localization, maintaining consistency across languages while preserving the color formatting.
libs/pando/ui-sheet/src/types/sheet.ts (1)
6-6: LGTM! Verify usage of UISheetElement across the codebase.
Making the img property optional is a good change that provides more flexibility for UI elements that don't require images, such as traces.
Let's verify that existing code handles the optional img property correctly:
✅ Verification successful
LGTM! All existing code properly handles the optional img property.
The codebase already uses optional chaining (?.) when accessing the img property in all locations, indicating that the code is prepared for cases where the image might be undefined. This is evident in files like:
libs/gi/ui/src/components/character/CharacterProfilePieces.tsxlibs/gi/page-team/src/CharacterDisplay/Tabs/TabTalent.tsxapps/gi-frontend/src/app/teams/[teamId]/TalentContent.tsx
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for direct usage of UISheetElement's img property
# to ensure proper null checking is in place
ast-grep --pattern 'const $var: UISheetElement = { $$$, img: $_, $$$}'
# Also check for direct img access that might need null checks
rg -A 2 '\.img\b'
Length of output: 2925
libs/pando/ui-sheet/src/components/DocumentDisplay.tsx (1)
6-6: LGTM! Import change aligns with Material-UI best practices.
Adding Typography from @mui/material is appropriate for consistent text styling.
libs/sr/formula-ui/src/char/SkillSubtitle.tsx (2)
8-8: LGTM! Verify usage of this component
The addition of 'technique' to the union type is a good enhancement that aligns with the PR objectives for adding traces to sheets.
✅ Verification successful
Let me check the actual usage context by examining the sheetUtil.tsx file to see how the talentKey is being passed.
The change is safe and properly used
The talentKey type in SkillSubtitle is used in a single location within sheetUtil.tsx, where it's passed through the talentSheet function. The function's talentKey parameter already includes 'technique' in its type definition: talentKey: 'basic' | 'skill' | 'ult' | 'talent' | 'technique', making it fully compatible with the updated type in SkillSubtitle.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all usages of SkillSubtitle to ensure compatibility
# Expected: Identify any places that need updates for the new type
# Search for imports and usages of SkillSubtitle
rg "SkillSubtitle"
# Search for specific usage patterns
ast-grep --pattern 'SkillSubtitle talentKey={$_}'
Length of output: 389
Script:
#!/bin/bash
# Check the content of sheetUtil.tsx to understand how talentKey is used
cat libs/sr/formula-ui/src/char/sheetUtil.tsx
Length of output: 3850
8-8: Verify translations for the new 'technique' key
The addition of 'technique' to the union type will require corresponding translations in the i18n files.
✅ Verification successful
Based on the search results, I can see that this is part of the Star Rail (SR) codebase, and 'technique' is a well-established ability type throughout the codebase with corresponding translations in multiple languages (vi, es, chs, cht, ko, etc.) under the libs/sr/dm-localization/assets/locales/ directory. The translations exist for all characters and are consistently structured.
Translations are properly set up for the 'technique' key
The addition of 'technique' to the union type is consistent with the codebase's existing structure and has proper i18n support across all supported languages.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if translations exist for the new 'technique' key
# Expected: Find translations for 'technique' in characters_gen namespace
# Search for technique translations in i18n files
rg -g "*.json" -g "!*node_modules*" "technique" ./public/locales
Length of output: 121
Script:
#!/bin/bash
# Let's try to find the i18n files in the repository first
fd -g "*.json" -E "*node_modules*" --type f
# Then let's check for any references to translations or i18n configuration
rg -g "!*node_modules*" "characters_gen"
Length of output: 73590
Script:
#!/bin/bash
# Let's search for i18n configuration and translation files in the codebase
rg -g "!*node_modules*" -l "i18n"
# Also search for any usage of 'technique' in translation-related files
rg -g "!*node_modules*" -g "*.ts*" -g "*.json" "technique"
# Look for the actual component usage to understand the translation setup
ast-grep --pattern 'const $_ = useTranslation($$$)'
Length of output: 65782
libs/pando/ui-sheet/src/components/FieldDisplay.tsx (2)
60-65: LGTM: Clean styling implementation
The added styling provides proper spacing and alignment using MUI's flex layout system.
Line range hint 1-250: Review TODOs and commented code
The file contains several TODOs and commented-out features that might be relevant to the trace functionality:
- TODO: compare calcs
- TODO: variant from tag
- TODO: what do here? (for
fixedvariable) - TODO: Formula display
- TODO: teambuff
Please clarify if these are:
- Planned for a future PR
- Required for the trace functionality mentioned in the PR objectives
- Can be safely ignored for now
libs/sr/dm-localization/assets/locales/ja/lightCone_SailingTowardsASecondLife_gen.json (1)
5-5: LGTM! Verify placeholder consistency across other light cone files.
The placeholder format update from #4[i] to {{4}}[i] is correct and follows modern interpolation practices.
✅ Verification successful
Placeholder format is consistent with modern standards
The file correctly uses the modern {{n}} format for all placeholders ({{1}}, {{2}}, {{3}}, {{4}}). No instances of the old #n format were found. The placeholder format update is properly implemented.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any inconsistent placeholder formats in light cone files
# Expected: All placeholders should use {{n}} format instead of #n
# Search for old placeholder format in light cone files
echo "Checking for old placeholder format (#[0-9])..."
rg "#[0-9]" "libs/sr/dm-localization/assets/locales/*/lightCone_*.json"
Length of output: 276
Script:
#!/bin/bash
# Let's first find the correct path pattern for light cone files
echo "Finding light cone localization files..."
fd "lightCone.*\.json" libs/sr/dm-localization/assets/locales
# Then search for old placeholder format
echo -e "\nChecking for old placeholder format (#[0-9])..."
fd "lightCone.*\.json" libs/sr/dm-localization/assets/locales --exec rg --no-line-number "#[0-9]" {} || true
# Also check current placeholder format for comparison
echo -e "\nChecking current placeholder format ({{[0-9]}})..."
fd "lightCone.*\.json" libs/sr/dm-localization/assets/locales --exec rg --no-line-number "{{[0-9]}}" {} || true
Length of output: 131069
Script:
#!/bin/bash
# Let's fix the regex pattern and try again
echo "Checking for old placeholder format (#[0-9])..."
fd "lightCone.*\.json" libs/sr/dm-localization/assets/locales --exec rg --no-line-number "#[0-9]" {} || true
echo -e "\nChecking current placeholder format..."
fd "lightCone.*\.json" libs/sr/dm-localization/assets/locales --exec rg --no-line-number "\{\{[0-9]\}\}" {} || true
# Also check the specific file mentioned in the review
echo -e "\nChecking the specific file content..."
cat libs/sr/dm-localization/assets/locales/ja/lightCone_SailingTowardsASecondLife_gen.json || true
Length of output: 66308
Script:
#!/bin/bash
# Let's try to read the specific file content again and search for placeholders
echo "Checking the specific file content..."
cat "libs/sr/dm-localization/assets/locales/ja/lightCone_SailingTowardsASecondLife_gen.json"
echo -e "\nChecking for old placeholder format in this specific file..."
cat "libs/sr/dm-localization/assets/locales/ja/lightCone_SailingTowardsASecondLife_gen.json" | grep -E "#[0-9]" || true
echo -e "\nChecking for current placeholder format in this specific file..."
cat "libs/sr/dm-localization/assets/locales/ja/lightCone_SailingTowardsASecondLife_gen.json" | grep -E "\{\{[0-9]" || true
Length of output: 1343
libs/sr/dm-localization/assets/locales/chs/char_March7thTheHunt_gen.json (1)
14-14: LGTM! Color formatting and placeholder update are consistent.
The changes maintain proper color tag structure while updating the placeholder format.
✅ Verification successful
Color tag formatting is consistent across all language files
The verification shows that the color tag <color=#f29e38ff>...</color> is properly formatted and consistently used across all language versions of the character files, including the Simplified Chinese (chs) file under review. The color tag structure and nesting with <orangeStrong> tags is maintained correctly.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify color tag consistency across character files
# Expected: Color tags should be properly closed and formatted
# Check for proper color tag structure
echo "Checking color tag formatting..."
rg -U "<color=#f29e38ff>.*?</color>" "libs/sr/dm-localization/assets/locales/*/char_*.json"
Length of output: 261
Script:
#!/bin/bash
# Let's first find the correct path pattern for character files
fd "char_.*\.json" libs/sr/dm-localization/assets/locales/
# Then check color tag formatting in found files
echo "Checking color tag formatting..."
fd "char_.*\.json" libs/sr/dm-localization/assets/locales/ --exec rg -U "<color=#f29e38ff>.*?</color>" {}
Length of output: 53899
libs/sr/dm-localization/assets/locales/cht/char_March7thTheHunt_gen.json (1)
14-14: LGTM! Changes are consistent across language versions.
The placeholder format and color tag structure match the changes in other language files while maintaining proper Traditional Chinese characters.
✅ Verification successful
Verified: Localization changes are consistent across language versions
The Traditional Chinese (cht) version correctly matches the Simplified Chinese (chs) version in terms of:
- Placeholder format (
{{3}}[i],{{4}}) - Color tag structure (
<color=#f29e38ff>) - Text formatting tags (
<orangeStrong>) - Character conversion accuracy (e.g., 概率→機率, 单体→單體)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify consistency of changes across all language versions
# Expected: Same placeholder format and structure in all versions
# Compare placeholder format in all language versions
echo "Checking placeholder consistency across languages..."
rg -U "{{3}}\[i\].*?</color>" "libs/sr/dm-localization/assets/locales/*/char_March7thTheHunt_gen.json"
Length of output: 316
Script:
#!/bin/bash
# Description: Verify consistency of changes across all language versions
# Using a simpler pattern to match the placeholder structure
echo "Checking placeholder format across language versions..."
rg -U "{{3}}" "libs/sr/dm-localization/assets/locales/*/char_March7thTheHunt_gen.json"
echo -e "\nChecking color tag structure..."
rg -U "color=#f29e38ff" "libs/sr/dm-localization/assets/locales/*/char_March7thTheHunt_gen.json"
echo -e "\nListing all available language versions of the file..."
fd "char_March7thTheHunt_gen.json" "libs/sr/dm-localization/assets/locales" --exec ls -l
Length of output: 2366
Script:
#!/bin/bash
# Description: Check content of both Traditional (cht) and Simplified (chs) Chinese files
# to verify consistency of placeholders and formatting
echo "Checking Traditional Chinese (cht) version..."
cat "libs/sr/dm-localization/assets/locales/cht/char_March7thTheHunt_gen.json" | grep -A 1 "shortDesc"
echo -e "\nChecking Simplified Chinese (chs) version..."
cat "libs/sr/dm-localization/assets/locales/chs/char_March7thTheHunt_gen.json" | grep -A 1 "shortDesc"
# Also check English version as reference
echo -e "\nChecking English (en) version..."
cat "libs/sr/dm-localization/assets/locales/en/char_March7thTheHunt_gen.json" | grep -A 1 "shortDesc"
Length of output: 4704
libs/sr/dm-localization/assets/locales/ja/char_RuanMei_gen.json (1)
31-31: LGTM: Placeholder format standardization
The update from static to dynamic placeholder format (#1[f1]% → {{1}}[f1]%) aligns with the standardization effort across localization files while maintaining the correct translation and formatting.
libs/sr/dm-localization/assets/locales/pt/char_Yanqing_gen.json (1)
31-31: LGTM: Placeholder format standardization
The update from static to dynamic placeholder format (#4[i]% → {{4}}[i]%) aligns with the standardization effort across localization files while maintaining the correct translation and formatting.
libs/sr/dm-localization/assets/locales/th/char_March7thTheHunt_gen.json (1)
14-14: LGTM: Placeholder format standardization
The update from static to dynamic placeholder format (#3[i] → {{3}}[i]) aligns with the standardization effort across localization files while maintaining the correct translation, color tags, and formatting.
libs/sr/dm-localization/assets/locales/es/char_March7thTheHunt_gen.json (1)
14-14: LGTM! Consistent placeholder formatting.
The placeholder format update from #3[i] to {{3}}[i] aligns with standard templating conventions and improves consistency.
Let's verify this formatting change across other language files:
✅ Verification successful
Placeholder formatting is consistent across all language files
The verification confirms that the new {{3}}[i] format is consistently used across all language files, including es, en, de, fr, ja, ko, ru, th, vi, pt, id, chs, and cht. No instances of the old #3[i] format were found.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistent placeholder formatting across language files
# Expected: All language files should use {{3}}[i] format
# Search for any remaining #3[i] format
echo "Checking for old format (#3[i]):"
rg "#3\[i\]" "libs/sr/dm-localization/assets/locales/"
# Verify new format ({{3}}[i]) is used consistently
echo "Verifying new format ({{3}}[i]):"
rg "\{\{3\}}\[i\]" "libs/sr/dm-localization/assets/locales/"
Length of output: 7310
libs/sr/dm-localization/assets/locales/de/char_March7thTheHunt_gen.json (2)
14-14: LGTM! Consistent localization update.
The placeholder format change matches the Spanish version and maintains proper formatting while preserving the German translation's accuracy.
14-14: Verify trace-related functionality.
While the localization changes look good, the PR objectives mention adding traces and making them fade out when not enabled. However, the implementation of this functionality is not visible in the provided files.
Let's check for related changes:
libs/sr/formula-ui/src/char/index.ts (1)
3-3: LGTM!
The new export statement correctly includes StatBoostBonusAbility, enhancing module accessibility.
libs/sr/dm-localization/assets/locales/ja/charSheet_gen.json (1)
1-21: LGTM!
The Japanese localization entries are accurately added and formatted correctly.
libs/sr/dm-localization/assets/locales/fr/charSheet_gen.json (2)
4-20: Verify completeness of stat boost translations
The stat boost translations look good, but let's verify we have all the same keys across language files.
✅ Verification successful
All stat boost keys are consistent across language files
The verification shows that all language files (en, es, pt, ko, ja, ru, th, vi, fr, id, de, chs, cht) contain exactly the same set of 15 stat boost keys in the same order. The French translations are complete and aligned with all other locales.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Compare stat boost keys across all language files
for locale in $(fd -t d . libs/sr/dm-localization/assets/locales -d 1 -x basename); do
echo "=== Keys in $locale ==="
jq -r '.statBoost | keys[]' "libs/sr/dm-localization/assets/locales/$locale/charSheet_gen.json" 2>/dev/null | sort || true
done
Length of output: 4871
2-3: Verify template parameter usage across languages
The requirement strings use {{1}} for template parameters. Let's verify this is consistent across all language files.
✅ Verification successful
Template parameter usage is consistent across all language files
The verification shows that all language files (en, th, vi, pt, de, ko, es, ru, cht, ja, chs, fr, id) consistently use the {{1}} template parameter syntax in both req_asc and req_lvl strings. The French translations align perfectly with this pattern.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check template parameter usage in requirement strings across all languages
for locale in $(fd -t d . libs/sr/dm-localization/assets/locales -d 1 -x basename); do
echo "=== Checking $locale ==="
jq -r '.req_asc + "\n" + .req_lvl' "libs/sr/dm-localization/assets/locales/$locale/charSheet_gen.json" 2>/dev/null || true
done
Length of output: 3821
libs/sr/dm-localization/src/executors/gen-locale/lib/charHashData/charSheet.ts (1)
1-21: Verify hash ID uniqueness
Let's ensure there are no duplicate hash IDs that could cause localization conflicts.
The comments providing English translations are helpful for maintenance. Consider adding JSDoc comments for better IDE integration.
export const charSheet = {
+ /** Requires character ascension {{1}} */
req_asc: '-2096262979',
+ /** Requires character level {{1}} */
req_lvl: '825636042',
statBoost: {
+ /** HP Boost */
hp_: '2132031605', // "HP Boost"
// ... similar for other properties
},
}✅ Verification successful
Hash IDs are unique and well-documented
After examining the file content, all hash IDs are unique with no duplicates. The existing inline comments provide clear English translations for each stat boost, making the code maintainable. While JSDoc comments could improve IDE integration, the current documentation approach is sufficient for understanding the localization mappings.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Extract all hash IDs and check for duplicates
ast-grep --pattern '$_: "$_"' | grep -o '"[0-9-]*"' | sort | uniq -d
Length of output: 70
Script:
#!/bin/bash
# Let's try a different approach to extract and check hash values
rg -o '"[-\d]+"' libs/sr/dm-localization/src/executors/gen-locale/lib/charHashData/charSheet.ts | sort | uniq -d
Length of output: 114
Script:
#!/bin/bash
# Let's try one more approach - extract all lines with hash values and examine them
rg "': '[0-9-]+'" libs/sr/dm-localization/src/executors/gen-locale/lib/charHashData/charSheet.ts
Length of output: 107
Script:
#!/bin/bash
# Let's try a simpler approach - just display the file content
cat libs/sr/dm-localization/src/executors/gen-locale/lib/charHashData/charSheet.ts
Length of output: 935
libs/sr/dm-localization/src/executors/gen-locale/executor.ts (1)
21-21: Verify charSheet localization file generation
The addition of 'charSheet' to the general localization namespaces looks good. Let's verify the file generation works as expected.
✅ Verification successful
charSheet localization files are properly generated
The verification shows that charSheet_gen.json files are correctly generated for all supported locales (chs, cht, de, en, es, fr, id, ja, ko, pt, ru, th, vi) in the expected location under libs/sr/dm-localization/assets/locales/.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if charSheet_gen.json files are generated for all locales
fd -g "*charSheet_gen.json" libs/sr/dm-localization/assets/locales
Length of output: 863
libs/sr/dm-localization/src/executors/gen-locale/lib/hashData.ts (1)
7-7: LGTM: Clean integration of character sheet localization data
The addition of charSheet to both imports and exports is well-structured and maintains consistency with the existing pattern of data organization.
Also applies to: 33-33
libs/sr/formula-ui/src/char/StatBoostBonusAbility.tsx (1)
49-53:
Handle undefined subtitle case explicitly
The statBoost1 entry uses undefined for subtitle, which could lead to rendering issues.
Let's verify how undefined subtitles are handled in the UI:
Consider providing a default subtitle or documenting why it's undefined:
statBoost1: {
- subtitle: undefined,
+ subtitle: null, // Base stat boost doesn't require a subtitle
onClickable: () => true,
disabled: (calc: Calculator) => !calc.compute(own.char.statBoost1).val,
},✅ Verification successful
Undefined subtitle is acceptable in this context
Looking at the code structure in StatBoostBonusAbility.tsx, the undefined subtitle is intentional for the base statBoost1 entry since it represents a fundamental stat boost that doesn't require additional descriptive text, unlike other entries that may need explanatory subtitles for requirements or conditions.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for subtitle rendering logic in UI components
rg -A 5 "subtitle.*ReactNode" --type tsx
Length of output: 70
Script:
#!/bin/bash
# Search for subtitle rendering logic in UI components without file type restriction
rg -A 5 "subtitle.*:"
# Also search for the component that might be using this subtitle
rg -A 5 "statBoost1"
Length of output: 75131
Script:
#!/bin/bash
# Search for UI components that render the subtitle
rg -A 5 "subtitle.*:" --type typescript --type tsx
# Search for any UI components that might be using this specific statBoost1 entry
rg -A 5 "statBoost1.*" --type typescript --type tsx
Length of output: 176
Script:
#!/bin/bash
# Search for UI components that render the subtitle without file type restriction
rg -A 5 "interface.*ReqMap"
# Also look for where this statBoost1 is used in UI rendering
rg -A 5 "render.*statBoost"
Length of output: 59
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (3)
libs/sr/page-team/src/TalentContent.tsx (2)
64-69: Consider removing redundant type assertionThe
as constassertion is unnecessary sinceTalentSheetElementKeyalready defines the allowed values.- ] as const + ]
248-255: Consider extracting isDisabled logic to a custom hookThe disability check logic could be moved to a custom hook for better reusability and testing:
function useIsDisabled(talentKey: TalentSheetElementKey, calculator: Calculator | undefined) { return useMemo(() => { if (!calculator) return false if (isTalentSheetElementStatBoostKey(talentKey)) return bonusStatsReqMap[talentKey].disabled(calculator) if (isTalentSheetElementBonusAbilityKey(talentKey)) return bonusAbilityReqMap[talentKey].disabled(calculator) return false }, [calculator, talentKey]) }libs/sr/formula-ui/src/char/consts.ts (1)
61-73: Consider UI implications of key orderingThe order of keys in
allTalentSheetElementKeymight affect how items are rendered in the UI. Consider documenting the intended order or grouping related keys together with comments.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
libs/sr/formula-ui/src/char/consts.ts(2 hunks)libs/sr/formula-ui/src/char/sheetUtil.tsx(1 hunks)libs/sr/page-team/src/TalentContent.tsx(5 hunks)
🔇 Additional comments (11)
libs/sr/page-team/src/TalentContent.tsx (3)
18-28: LGTM: Well-structured imports for new functionality
The new imports are logically organized and provide comprehensive support for handling bonus abilities and stat boosts.
88-99: LGTM: Improved type safety and React key handling
The refactor from range-based to key-based mapping enhances type safety, and the addition of the key prop addresses the previous review feedback.
343-346: LGTM: Well-implemented fade-out effect
The opacity styling correctly implements the requirement to fade out disabled talents.
libs/sr/formula-ui/src/char/consts.ts (4)
1-5: LGTM! Well-organized imports
The imports are specific and properly typed, following TypeScript best practices.
7-27: Fix inconsistent capitalization in variable names
The naming convention issue from the previous review still exists.
Additionally, consider adding JSDoc comments to document the purpose of these constants and functions, especially explaining what each stat boost represents.
29-45: LGTM! Correctly models game mechanics
The implementation properly represents the six eidolon levels in the game.
46-59: Verify the number of bonus abilities
Please confirm that three bonus abilities is the correct number according to the game mechanics.
✅ Verification successful
Based on the search results, I can see that the three bonus abilities pattern is consistently used across all character implementations in the codebase. The evidence shows:
- All character stat mappings in
libs/sr/stats/src/mappedStats/char/maps/use exactly three bonus abilities (bonusAbility1, bonusAbility2, bonusAbility3) - The type definitions and constants in multiple places consistently define exactly three bonus abilities
- The implementation is used uniformly across all characters' assets and stats calculations
Three bonus abilities is the correct implementation
The codebase shows this is a fundamental game mechanic that is consistently implemented across all characters, with each bonus ability serving specific purposes (typically break, energy, and break threshold stats).
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any references to bonus abilities in the codebase to verify the count
rg -i "bonusability[0-9]" --type ts
Length of output: 56735
libs/sr/formula-ui/src/char/sheetUtil.tsx (4)
33-59: Function talentSheet is well-implemented.
The function correctly constructs the UISheetElement for talents, and the implementation follows best practices.
111-133: Function eidolonSheet is correctly implemented.
The function effectively constructs the UISheetElement for eidolons, and the code is clean and maintainable.
76-76:
Address the FIXME regarding interpolation for bonus stats.
The comment indicates that getCharInterpolateObject may not work correctly for bonus stats due to the wrong array format. This could lead to incorrect descriptions or runtime errors.
Would you like assistance in investigating and fixing the array format issue for bonus stats interpolation?
88-90:
Add null checks to handle empty stats entries in bonusStatsSheets.
If stats.skillTree[key]?.levels?.[0].stats is empty, Object.entries(...)[0] could be undefined, leading to runtime errors. Consider adding a check to ensure entries are available before destructuring.
Apply this diff to add null checks:
const entries = Object.entries(
stats.skillTree[key]?.levels?.[0].stats ?? {}
)
+if (entries.length === 0) {
+ return {
+ title: <Translate ns={'charSheet_gen'} key18={`statBoost.unknown`} />,
+ subtitle: bonusStatsReqMap[key].subtitle,
+ documents: []
+ }
+}
const [statKey, value] = entries[0]
| } | ||
| } | ||
|
|
||
| export const bonusStatsReqMap: Record< |
There was a problem hiding this comment.
Don't know if you wanna add a TODO for other dependencies (e.g. bonus stat X might rely on bonus stat X-3 being unlocked, and it changes per-path)
There was a problem hiding this comment.
It is datamineable (#2568)
| | 'talent' | ||
| | 'technique' | ||
| | 'eidolon' | ||
| | 'bonusAbility1' |
There was a problem hiding this comment.
Seems possibly broken for bonus ability
There was a problem hiding this comment.
Yeah, I noticed this. I added a // FIXME
There was a problem hiding this comment.
I did not touch datamine stuff so I did not dig too deeply.
Describe your changes
Add bonus ability and stat boost traces to sheets.
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
Improvements
Bug Fixes
Chores