minor SRO UI updates #2529
minor SRO UI updates #2529
Conversation
WalkthroughThe changes in this pull request involve multiple enhancements across various files, primarily focusing on character and relic management functionalities. Key modifications include the addition of new functions for character stat boosts and gendered key handling, updates to UI components for better character representation, and new types and functions for relic management. The overall structure and logic of existing components have been preserved while extending their capabilities. Changes
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] [Mon Nov 4 04:33:35 UTC 2024] - Deployed bf9c1c7 to https://genshin-optimizer-prs.github.io/pr/2529/sr-frontend (Takes 3-5 minutes after this completes to be available) [sr-frontend] [Mon Nov 4 04:33:55 UTC 2024] - Deployed bf9c1c7 to https://genshin-optimizer-prs.github.io/pr/2529/sr-frontend (Takes 3-5 minutes after this completes to be available) [sr-frontend] [Mon Nov 4 04:39:02 UTC 2024] - Deployed 9820ceb to https://genshin-optimizer-prs.github.io/pr/2529/sr-frontend (Takes 3-5 minutes after this completes to be available) [sr-frontend] [Mon Nov 4 05:20:44 UTC 2024] - Deployed 6e23de3 to https://genshin-optimizer-prs.github.io/pr/2529/sr-frontend (Takes 3-5 minutes after this completes to be available) [sr-frontend] [Mon Nov 4 19:29:11 UTC 2024] - Deployed caf3667 to https://genshin-optimizer-prs.github.io/pr/2529/sr-frontend (Takes 3-5 minutes after this completes to be available) [sr-frontend] [Mon Nov 4 19:34:15 UTC 2024] - Deployed 9a0c436 to https://genshin-optimizer-prs.github.io/pr/2529/sr-frontend (Takes 3-5 minutes after this completes to be available) [sr-frontend] [Mon Nov 4 20:10:37 UTC 2024] - Deployed 3cd5fe8 to https://genshin-optimizer-prs.github.io/pr/2529/sr-frontend (Takes 3-5 minutes after this completes to be available) [sr-frontend] [Tue Nov 5 00:27:30 UTC 2024] - Deployed a17604d to https://genshin-optimizer-prs.github.io/pr/2529/sr-frontend (Takes 3-5 minutes after this completes to be available) [Tue Nov 5 19:58:16 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: 4
🧹 Outside diff range and nitpick comments (8)
libs/sr/ui/src/Character/CharacterTrans.tsx (1)
6-12: Consider performance and type safety improvements.While the implementation is correct, consider these enhancements:
- Memoize the component to prevent unnecessary re-renders in lists
- Extract the prop type to a named interface
- Consider making the translation namespace configurable
Here's a suggested implementation:
+interface CharacterNameProps { + characterKey: CharacterKey; + /** Optional override for translation namespace */ + translationNs?: string; +} -export function CharacterName({ - characterKey, -}: { - characterKey: CharacterKey -}) { - return <Translate ns="charNames_gen" key18={characterKey} /> -} +export const CharacterName = React.memo(function CharacterName({ + characterKey, + translationNs = "charNames_gen" +}: CharacterNameProps) { + return <Translate ns={translationNs} key18={characterKey} /> +});libs/sr/assets/src/index.ts (1)
37-46: Add JSDoc documentation for the function.The function's purpose and return value types should be documented for better maintainability.
+/** + * Converts a CharacterKey to its corresponding gendered or non-gendered form. + * For Trailblazer characters, returns a gendered key (currently hardcoded to female). + * For non-Trailblazer characters, returns the original key. + * @param ck The character key to convert + * @returns Either a NonTrailblazerCharacterKey or TrailblazerGenderedKey + */ export function characterKeyToGenderedKey( ck: CharacterKey ): NonTrailblazerCharacterKey | TrailblazerGenderedKey {libs/sr/stats/src/index.ts (2)
59-88: Add JSDoc documentation for the new functionsThese utility functions lack documentation explaining their purpose, parameters, return values, and potential error cases.
Consider adding JSDoc comments:
+/** + * Gets the stat key from a character's stat boost + * @param ck - Character key + * @param bonusStats - The type of stat boost to look up + * @returns The stat key for the boost + * @throws {Error} If no stat boost data is found + */ export function getCharStatBoostStatKey(...) +/** + * Gets the value from a character's stat boost + * @param ck - Character key + * @param bonusStats - The type of stat boost to look up + * @returns The numeric value of the boost + * @throws {Error} If no stat boost data is found + */ export function getCharStatBoostStatValue(...) // Add similar docs for other functions
59-88: Consider reordering functions for better code organizationThe functions are currently organized from specific to general, which makes the code harder to follow since they depend on each other. Consider reordering them from general to specific:
getCharStatBoost(most general)getCharStatBoostStatgetCharStatBoostStatKeyandgetCharStatBoostStatValue(most specific)libs/sr/util/src/relic.ts (2)
175-182: Improve documentation and clarityThe function logic is sound, but consider these improvements:
- Replace the "noopt" comment with a clear explanation of the fallback case
- Add JSDoc to document the function's behavior, especially for edge cases
+/** + * Returns the default relic slot for a given set key. + * @param setKey The relic set key to check + * @returns The first cavern slot for cavern sets, first planar slot for planar sets, + * or first cavern slot as fallback + */ export function getDefaultRelicSlot(setKey: RelicSetKey) { if (allRelicCavernSetKeys.includes(setKey as RelicCavernSetKey)) return allRelicCavernSlotKeys[0] if (allRelicPlanarSetKeys.includes(setKey as RelicPlanarSetKey)) return allRelicPlanarSlotKeys[0] - // noopt + // Fallback to first cavern slot for unknown set types return allRelicCavernSlotKeys[0] }
184-186: Consider adding complementary utility functionThe function is well-implemented. For API consistency, consider adding a complementary
isPlanarRelicfunction.export function isCavernRelic(setKey: RelicSetKey) { return allRelicCavernSetKeys.includes(setKey as RelicCavernSetKey) } + +export function isPlanarRelic(setKey: RelicSetKey) { + return allRelicPlanarSetKeys.includes(setKey as RelicPlanarSetKey) +}libs/sr/ui/src/Character/CharacterEditor.tsx (2)
114-170: Consider extracting common ability dropdown props to reduce duplication.While the implementation is functionally correct, there's significant repetition in the icon configuration across ability dropdowns.
Consider extracting the common props into a reusable configuration:
+ const getAbilityIconProps = (genderedKey: string, abilityType: string) => ({ + startIcon: ( + <ImgIcon + size={1.5} + sideMargin + src={characterAsset(genderedKey, `${abilityType}_0`)} + /> + ), + }); <AbilityDropdown characterKey={characterKey} abilityKey="basic" - dropDownButtonProps={{ - startIcon: ( - <ImgIcon - size={1.5} - sideMargin - src={characterAsset(genderedKey, 'basic_0')} - /> - ), - }} + dropDownButtonProps={getAbilityIconProps(genderedKey, 'basic')} />
211-247: Consider extracting the stat boost button logic for better maintainability.While the implementation is functional, the nested structure could be simplified for better readability.
Consider these improvements:
+ const StatBoostButton = ({ + statBoost, + enabled, + genderedKey, + onToggle + }) => { + const { statKey, value } = getCharStatBoostStat(genderedKey, statBoost); + return ( + <Button + color={enabled ? 'success' : 'primary'} + onClick={onToggle} + startIcon={enabled ? <CheckBoxIcon /> : <CheckBoxOutlineBlankIcon />} + > + <StatIcon statKey={statKey} iconProps={{ sx: { ml: -1, mr: 0.5 } }} /> + <Box> + {toPercent(value, statKey)} + {getUnitStr(statKey)} + </Box> + </Button> + ); + }; {Object.entries(character?.statBoosts ?? {}).map( ([statBoost, enabled]) => ( <Grid item key={statBoost}> + <StatBoostButton + statBoost={statBoost} + enabled={enabled} + genderedKey={genderedKey} + onToggle={() => { + database.chars.set(characterKey, { + statBoosts: { + ...character?.statBoosts, + [statBoost]: !enabled, + }, + }); + }} + /> </Grid> ) )}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (9)
libs/sr/assets/src/index.ts(2 hunks)libs/sr/stats/src/index.ts(2 hunks)libs/sr/ui/src/Character/AbilityDropdown.tsx(3 hunks)libs/sr/ui/src/Character/CharacterCard.tsx(3 hunks)libs/sr/ui/src/Character/CharacterEditor.tsx(6 hunks)libs/sr/ui/src/Character/CharacterTrans.tsx(1 hunks)libs/sr/ui/src/Character/index.tsx(1 hunks)libs/sr/ui/src/Relic/index.tsx(1 hunks)libs/sr/util/src/relic.ts(2 hunks)
🔇 Additional comments (16)
libs/sr/ui/src/Relic/index.tsx (1)
5-5: LGTM! Consider verifying RelicTrans exports.
The addition follows the established barrel file pattern.
Let's verify the exports from RelicTrans to ensure only intended exports are exposed:
✅ Verification successful
Exports from RelicTrans are minimal and well-defined ✅
The RelicTrans module only exports a single React component RelicSetName that handles translations. The other exports are commented out. This is a clean and focused export that aligns with the UI translation responsibilities suggested by the module name.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check the exports from RelicTrans
# Expected: Only UI/translation-related exports should be present
# Look for export statements in RelicTrans
rg --type typescript "export" "libs/sr/ui/src/Relic/RelicTrans"
Length of output: 96
Script:
#!/bin/bash
# Let's try different approaches to find and analyze the RelicTrans file
# First, let's find the exact file path
fd RelicTrans
# Then, let's look for exports using ripgrep without type filter
rg "export" --context 2 $(fd RelicTrans)
Length of output: 586
libs/sr/ui/src/Character/index.tsx (1)
6-6: LGTM! Verify usage of the new export.
The new export follows the established pattern and maintains alphabetical ordering.
Let's verify the usage of the newly exported CharacterTrans components:
✅ Verification successful
Export is correctly used and follows best practices
The verification confirms:
- The
CharacterNamecomponent is properly exported fromCharacterTrans.tsx - It's correctly imported by SR UI components (
CharacterCard.tsxandCharacterEditor.tsx) using relative imports - The barrel file export aligns with the actual usage in the codebase
- The component follows React best practices with proper TypeScript typing and 'use client' directive
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for imports of CharacterTrans or its components
# This helps ensure the new export is being utilized as intended
# Search for direct imports of CharacterTrans
echo "=== Direct CharacterTrans imports ==="
rg "from ['\"].*Character.*CharacterTrans['\"]"
# Search for CharacterName component usage (mentioned in AI summary)
echo -e "\n=== CharacterName component usage ==="
rg "<CharacterName"
Length of output: 2792
Script:
#!/bin/bash
# Let's verify the imports to understand if CharacterName is being imported correctly through the barrel file
# Check imports in sr/ui files that use CharacterName
echo "=== CharacterName imports in SR UI ==="
rg "import.*CharacterName" libs/sr/ui/src/Character/
# Check the actual CharacterTrans file content
echo -e "\n=== CharacterTrans exports ==="
cat libs/sr/ui/src/Character/CharacterTrans.tsx || echo "File not found"
Length of output: 833
libs/sr/ui/src/Character/CharacterTrans.tsx (2)
1-4: LGTM! Well-structured imports and proper Next.js directive usage.
The 'use client' directive is correctly placed with a clear explanation, and imports are properly typed.
11-11: Verify translation namespace consistency.
Let's ensure the "charNames_gen" namespace is consistently used across the codebase and properly configured in the i18n system.
libs/sr/assets/src/index.ts (1)
1-8: LGTM! Import statements are well-organized.
The new imports are properly structured and necessary for the added functionality.
libs/sr/ui/src/Character/AbilityDropdown.tsx (3)
14-16: LGTM! Well-structured type definition.
The new dropDownButtonProps type is well-defined using TypeScript's Omit utility to prevent accidental override of internally managed props (title and children).
18-22: LGTM! Safe parameter addition with default value.
The new parameter is added with a default empty object, maintaining backward compatibility and preventing undefined errors.
Line range hint 28-33: Verify safe prop spreading to DropdownButton.
The prop spreading implementation is correct, but let's ensure we're not allowing override of any critical props that could break the component's functionality.
Let's check the DropdownButton component's props:
✅ Verification successful
Let me gather more information about the DropdownButton component implementation to understand how the props are used.
Let me try one more search to find the component implementation with a different pattern:
Based on the information we've gathered so far, I can make a determination. We found the DropdownButtonProps type definition which shows:
- It extends
ButtonPropswithOmit<ButtonProps, 'title'> - It has its own required props:
title(ReactNode),children(ReactNode), and optionalid - The spread operator is used after explicitly setting
titleanddisabledprops
The prop spreading is safe in this implementation. The component's type definition ensures type safety, and the spread operator's placement after explicit props prevents unintended overwrites of critical functionality.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find DropdownButton component definition to verify allowed props
# Expected: Find the component's prop interface/type definition
# Search for DropdownButton definition
ast-grep --pattern 'export $_ DropdownButton = $$$'
# Search for DropdownButton type/interface
ast-grep --pattern 'export type DropdownButtonProps = $$$'
Length of output: 6122
Script:
#!/bin/bash
# Get the DropdownButton component implementation
ast-grep --pattern 'const DropdownButton = $$$'
# Also search for any ButtonProps type definition since DropdownButtonProps extends it
ast-grep --pattern 'type ButtonProps = $$$'
Length of output: 11323
Script:
#!/bin/bash
# Search for the DropdownButton component implementation in the specific file
rg -A 10 "export const DropdownButton" libs/common/ui/src/components/DropdownButton.tsx
Length of output: 89
libs/sr/stats/src/index.ts (2)
4-4: LGTM: Import addition is appropriate
The StatBoostKey import is properly grouped with related types and is necessary for the new stat boost functionality.
82-88: Verify the template literal type usage
The function accepts StatBoostKey | \${StatBoostKey}`` but uses string concatenation. This might not provide the type safety intended.
Let's check how StatBoostKey is used across the codebase:
✅ Verification successful
Based on the gathered information, I can now provide a final response:
Template literal type usage is correct and type-safe
The implementation is actually providing proper type safety because:
StatBoostKeyis defined as a numeric literal type fromallStatBoostKeysarray (1-10)- The template literal type
${StatBoostKey}allows string conversion while maintaining type checking - The property access
skillTree[statBoost${bonusStats}]matches the actual data structure inallStat_gen.jsonwhere properties are namedstatBoost1throughstatBoost10
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Find StatBoostKey type definition and usage
echo "Searching for StatBoostKey definition..."
rg --type typescript "type StatBoostKey"
echo "Searching for statBoost template usage..."
rg --type typescript "statBoost[A-Z]"
Length of output: 336
Script:
#!/bin/bash
# Find StatBoostKey type definition and usage with correct ripgrep syntax
echo "Searching for StatBoostKey definition..."
rg "type\s+StatBoostKey"
echo "\nSearching for statBoost template usage..."
rg "statBoost[A-Z]"
echo "\nSearching for files importing StatBoostKey..."
rg "import.*StatBoostKey"
Length of output: 521
Script:
#!/bin/bash
# Let's check the definition and usage in more detail
echo "Content of character.ts for StatBoostKey definition:"
cat libs/sr/consts/src/character.ts | grep -A 5 "StatBoostKey"
echo "\nChecking for statBoost property access patterns:"
rg "statBoost.*['\"]"
echo "\nChecking for allStatBoostKeys usage:"
rg "allStatBoostKeys"
Length of output: 131809
Script:
#!/bin/bash
# Let's check how the skillTree property is accessed in the codebase
echo "Checking skillTree property access:"
rg "skillTree\[.*\]" --type ts
echo "\nChecking getCharStat implementation:"
rg "function getCharStat" -A 5
Length of output: 2972
libs/sr/ui/src/Character/CharacterCard.tsx (3)
1-32: LGTM: Import organization is clean and well-structured
The new imports are properly organized by package and all are utilized in the component.
57-83: LGTM: Clean component structure with proper conditional rendering
The component maintains a clean structure with proper conditional rendering of the CardActionArea based on onClick presence.
Line range hint 118-146: LGTM: Improved stat calculation with proper scoping
The addition of characterKey prop and use of tags in calculations provides better scoping and accuracy for character-specific stats.
libs/sr/util/src/relic.ts (1)
8-13: LGTM: Clean import organization
The new type imports are well-organized and properly support the added functionality.
Also applies to: 15-15
libs/sr/ui/src/Character/CharacterEditor.tsx (2)
4-4: LGTM: Import additions are well-organized and purposeful.
The new imports enhance the UI capabilities with icons, formatting utilities, and character asset handling.
Also applies to: 7-11, 15-16
64-66: LGTM: Improved heading hierarchy and internationalization.
The switch to CharacterName component enhances translation support while maintaining proper heading hierarchy.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
libs/sr/assets/src/index.ts (1)
41-42: Track the TODO comment for gender implementation.The TODO comment indicates incomplete gender implementation. This should be addressed to support proper gender selection for Trailblazer characters.
Would you like me to create a GitHub issue to track this TODO and propose a proper implementation?
libs/sr/stats/src/index.ts (1)
59-71: Consider simplifying the type signature.The union type
StatBoostKey | \${StatBoostKey}`appears redundant sinceStatBoostKeyis likely already a string-based type. Consider simplifying to justStatBoostKey`.- bonusStats: StatBoostKey | `${StatBoostKey}` + bonusStats: StatBoostKeylibs/sr/consts/src/character.ts (1)
134-134: Remove unnecessary empty line.This empty line doesn't serve any purpose and can be removed for better code organization.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
libs/sr/assets/src/index.ts(2 hunks)libs/sr/consts/src/character.ts(1 hunks)libs/sr/stats/src/index.ts(2 hunks)
🔇 Additional comments (5)
libs/sr/assets/src/index.ts (2)
1-8: LGTM! Import statements are well-organized.
The imports are properly structured and include all necessary types and utilities.
37-46: Verify the function's integration with CharacterEditor.tsx.
The AI summary mentions this function is used in CharacterEditor.tsx. Let's verify its usage to ensure proper integration.
✅ Verification successful
Let me check the usage in CharacterCard.tsx as well to get a complete picture.
Let me check the implementation of isTrailblazerKey and related type definitions to complete the verification.
Function is properly integrated and working as expected
The verification shows that characterKeyToGenderedKey is correctly used in both CharacterEditor.tsx and CharacterCard.tsx:
- In CharacterEditor.tsx, it's used to convert character keys for display
- In CharacterCard.tsx, it's used to get gendered keys for retrieving character stats
- The function correctly handles both Trailblazer and non-Trailblazer characters through proper type checking with
isTrailblazerKey
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for usage of characterKeyToGenderedKey in CharacterEditor.tsx
rg -g '*CharacterEditor.tsx' -A 3 'characterKeyToGenderedKey'
# Also check for any other components that might use this function
rg -g '*.{tsx,ts}' -l 'characterKeyToGenderedKey'
Length of output: 932
Script:
#!/bin/bash
# Check usage in CharacterCard.tsx
rg -g '*CharacterCard.tsx' -A 3 'characterKeyToGenderedKey'
# Also check the type definitions and interfaces
ast-grep --pattern 'type TrailblazerGenderedKey = $_'
Length of output: 6527
Script:
#!/bin/bash
# Check implementation of isTrailblazerKey and related constants
rg -g '*.ts' -A 3 'isTrailblazerKey'
# Check TrailblazerKey related constants
rg -g '*.ts' 'allTrailblazerKeys'
Length of output: 1913
libs/sr/stats/src/index.ts (2)
4-4: LGTM!
The StatBoostKey import is properly added and used consistently throughout the new functions.
73-88: Verify single stat assumption.
The function only returns the first stat entry from levels[0].stats. Please confirm if this is the intended behavior or if we should handle multiple stats.
✅ Verification successful
The previous pattern might have been too specific. Let's try a broader search to understand the stat boost structure and verify if there are cases with multiple stats.
The data shows that each character has only one stat per level in their stat boosts, but let's verify the implementation and any tests to confirm this is the intended behavior.
Based on the code inspection and data from the character files, I can now generate a final response:
Single stat per boost level is correct implementation
The current implementation correctly handles the data structure. Looking at the character data files, each stat boost level consistently contains exactly one stat entry. This is a deliberate design where each boost path provides a single focused stat improvement, not multiple stats simultaneously.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if there are any cases where levels[0].stats has multiple entries
# that might be ignored by the current implementation
# Search for stat boost definitions in the codebase
ast-grep --pattern 'skillTree: {
$$$
statBoost$_: {
levels: [{
stats: {
$$$
}
}]
}
$$$
}'
Length of output: 127
Script:
#!/bin/bash
# Let's try a different approach to find stat boost definitions
# First, let's look for files that might contain these definitions
fd -e json -e ts "stat.*boost" --type f
# Then, let's search for actual usage patterns of stats object
rg -A 5 "stats: {"
Length of output: 142
Script:
#!/bin/bash
# Let's try a different approach
# First, find relevant files
fd -e json -e ts "stat.*boost" --type f
# Then search for stats patterns with proper escaping
rg -A 5 '"stats": \{' || rg -A 5 'stats: \{'
# Also search for the usage of getCharStatBoostStat to understand the context
rg -A 3 "getCharStatBoostStat"
Length of output: 411824
Script:
#!/bin/bash
# Let's look at how getCharStatBoost is implemented and used
rg -A 10 "function getCharStatBoost" libs/sr/stats/src/index.ts
# Also check for any tests that might verify this behavior
rg -l "getCharStatBoost" libs/sr/stats/__tests__/
Length of output: 1237
libs/sr/consts/src/character.ts (1)
135-137: LGTM! Well-implemented type guard function.
The function is well-implemented with proper type predicate usage, ensuring type safety when checking for TrailblazerKey types. The implementation is simple, focused, and follows TypeScript best practices.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (32)
libs/sr/db-ui/src/hooks/useRelic.ts (1)
3-6: Consider enhancing robustness and documentation.While the implementation is functional, consider the following improvements:
- Add error handling for undefined database context
- Add JSDoc documentation explaining the hook's purpose, parameters, and return value
- Consider using
nullinstead of empty string as the default value for better semanticsHere's a suggested implementation:
+/** + * Hook to manage a single relic's data. + * @param relicId - The unique identifier of the relic to manage + * @returns Data manager instance for the specified relic + * @throws {Error} When database context is not available + */ export function useRelic(relicId: string | undefined) { const { database } = useDatabaseContext() + if (!database) { + throw new Error('Database context not available') + } - return useDataManagerBase(database.relics, relicId ?? '') + return useDataManagerBase(database.relics, relicId ?? null) }libs/sr/ui/src/Character/CharacterTrans.tsx (1)
6-12: Consider these enhancements for better maintainability.While the implementation is correct, consider these improvements:
- Add JSDoc documentation to explain the component's purpose and props
- Consider extracting the "charNames_gen" namespace to a constant
- Consider using React.memo if this component is used in lists for better performance
Here's how you could enhance it:
+/** + * Renders a character's name with proper gender-specific translation + * @param genderedKey - The character's gendered key for translation lookup + */ +const CHAR_NAMES_NAMESPACE = 'charNames_gen' as const; + -export function CharacterName({ +export const CharacterName = React.memo(function CharacterName({ genderedKey, }: { genderedKey: CharacterGenderedKey }) { - return <Translate ns="charNames_gen" key18={genderedKey} /> -} + return <Translate ns={CHAR_NAMES_NAMESPACE} key18={genderedKey} /> +})libs/sr/ui/src/Components/LocationName.tsx (2)
13-16: Consider adding JSDoc documentation and explicit prop types.While the typing is correct, adding documentation would improve maintainability and developer experience.
Add JSDoc documentation:
+/** + * Renders a location name with an icon, either showing a character's location or inventory + * @param {LocationKey} location - The location key for the character + * @param {TypographyProps} props - Additional typography props from MUI + * @returns {JSX.Element} A typography component with location information + */ export function LocationName({ location, ...props }: { location: LocationKey } & TypographyProps) {
22-26: Consider memoizing the character asset source.The character asset source calculation could be memoized to prevent unnecessary recalculations.
Consider using useMemo:
+const iconSrc = useMemo( + () => characterAsset(characterKeyToGenderedKey(location), 'icon'), + [location] +); <ImgIcon size={2} sx={{ borderRadius: '50%' }} - src={characterAsset(characterKeyToGenderedKey(location), 'icon')} + src={iconSrc} />libs/sr/formula-ui/src/util.tsx (1)
Line range hint
23-38: Add JSDoc documentation for better maintainabilityConsider adding JSDoc comments to document:
- Function purpose
- Parameter requirements
- Return value format
- Usage examples
Example documentation:
+/** + * Generates translation functions for different game entities + * @param typeKey - The type of entity ('char', 'lightcone', or 'relic') + * @param key - The specific entity key + * @returns A tuple of translation functions [generic, specific] + * @example + * const [trg, tr] = trans('char', 'SomeCharacter') + * trg('skill.name') // Translates from generic namespace + * tr('profile.details') // Translates from specific namespace + */ export function trans(typeKey: 'char', key: CharacterGenderedKey): Translatedlibs/sr/dm-localization/src/executors/gen-locale/lib/hashData.ts (1)
17-17: Consider adding gender-specific validation checks.While the basic key verification is maintained, consider adding specific validation for gender variants to ensure all required combinations are present in
charNames.-verifyObjKeys(charNames, allCharacterGenderedKeys) +// Verify both base character names and their gender variants +const verifyCharacterKeys = (names: Record<string, string>, keys: string[]) => { + verifyObjKeys(names, keys); + // Add additional gender variant verification logic here + keys.forEach(key => { + if (key.includes('F_') || key.includes('M_')) { + const baseKey = key.replace(/(F|M)_/, ''); + if (!names[baseKey]) { + console.warn(`Missing base character key ${baseKey} for gender variant ${key}`); + } + } + }); +}; +verifyCharacterKeys(charNames, allCharacterGenderedKeys)libs/sr/dm-localization/src/executors/gen-locale/lib/charHashData/allCharacters.ts (1)
Line range hint
52-66: Consider adding documentation for complex logic.The Trailblazer special case handling could benefit from a comment explaining why the name is overridden with a specific hash value.
Add a comment before the condition:
const charKey = characterIdMap[charId] + // Override Trailblazer's name with a fixed hash since it's handled differently from other characters const name = ([...allTrailblazerGenderedKeys] as string[]).includes(charKey) ? '-2090701432' // Trailblazer : AvatarName.Hash.toString()apps/somnia/src/commands/databank/char.ts (3)
Line range hint
43-57: Address TODO comments and improve type safetyThe
baseEmbedfunction contains TODO comments that should be addressed:// TODO: const icon = getAssets(id).icon // TODO: const element = allStats.char[id].damageTypeWould you like help implementing these TODOs?
Line range hint
58-77: Enhance type safety in skillEmbed functionThe function has several areas for improvement:
- Replace
Record<string, any>with a proper type for skills- Add null checks for thumbnail access
- Consider using an enum or const object for ability types
Here's a suggested improvement:
type Skill = { name: string; shortDesc?: string; fullDesc: string; }; type AbilitySkills = Record<string, Skill>; function skillEmbed( ability: AbilityKey, id: CharacterGenderedKey, name: string, data: { abilities: Record<AbilityKey, AbilitySkills> } ) { // ... rest of the implementation }
Line range hint
78-103: Add type safety to eidolonsEmbed functionThe function uses
anytype for data parameter and lacks proper type checking for constellation data access.Consider creating proper types:
type Constellation = { name: string; desc: string; }; type CharacterData = { ranks: Record<string, Constellation>; constellationName?: string; };libs/sr/assets-data/src/executors/gen-assets-data/executor.ts (1)
64-64: Consider documenting the gender-specific asset handling strategy.This type change suggests a broader architectural shift in how character assets are handled. Consider:
- Adding comments explaining the gender-specific asset strategy
- Updating the README to document this asset organization pattern
- Creating migration guides if this affects existing data structures
libs/sr/ui/src/Character/CharacterCard.tsx (2)
84-117: Consider adding prop types validation.The Header component could benefit from using PropTypes or TypeScript interfaces to validate the shape of the character prop, especially since it's accessing nested properties.
Consider adding prop validation:
interface HeaderProps { character: { key: CharacterKey; eidolon: number; level: number; ascension: number; }; } function Header({ character }: HeaderProps) { // ... existing implementation }
138-143: Document the stat calculation logic.The stat calculation using
withTagand the purpose of settingsrcanddsttocharacterKeyneeds documentation. This would help maintainers understand the reasoning behind this implementation.Consider adding a comment explaining the calculation:
+ // Calculate the final stat value using character-specific tags + // to ensure proper stat attribution and modification tracking {toPercent( calc .withTag({ src: characterKey, dst: characterKey }) .compute(own.final[statKey]).val, statKey ).toFixed(statToFixed(statKey))}libs/sr/theme/src/Theme.tsx (1)
16-30: Consider using more descriptive gradient names.The gradient property implementation is correct and follows TypeScript best practices. However, consider using more semantic names that reflect their purpose (e.g.,
gradientPrimary,gradientSecondary) instead of numbered gradients for better maintainability.libs/sr/page-team/src/BuildsDisplay.tsx (1)
Line range hint
53-106: Consider adding JSDoc documentation for the useActiveBuildSwap hook.This hook handles complex build switching logic. Adding documentation would help other developers understand its purpose, parameters, and return value.
Example documentation:
/** * Hook to handle build type switching between equipped and custom builds * @param newBuildType - The target build type to switch to * @param newBuildId - Optional build ID for custom builds * @returns Object containing active state and click handler for build switching */libs/sr/page-team/src/Optimize/GeneratedBuildsDisplay.tsx (1)
Dialog implementation issue is a known bug, other concerns need attention
- The Dialog implementation issue is a known bug that also affects other components (see similar TODO in
libs/gi/ui/src/components/build/EquipBuildModal.tsx). This should be tracked and fixed at the framework level.- The "Create a new TC Build" text needs translation. There are existing TC build related translations in the localization files that can be referenced.
- The TC build functionality is incomplete and needs implementation.
- Add translation key for "Create a new TC Build" in
libs/sr/localization/assets/locales/en/following existing TC build translation patterns- Implement the TC build creation logic in the
toTCBuildbranch of the conditional- Consider using
ModalWrapperas a temporary solution for Dialog issues until the framework-level bug is resolved🔗 Analysis chain
Line range hint
89-143: Address TODOs and missing translations in NewBuildButton.Several items need attention:
- TC build functionality is incomplete (line 95)
- Dialog implementation has reported issues (line 112)
- Missing translation for "Create a new TC Build" text (line 134)
Would you like help with:
- Implementing the TC build functionality?
- Resolving the Dialog implementation issues?
- Adding the missing translations?
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for other instances of untranslated TC build related strings rg -i "tc build" --type ts --type tsxLength of output: 67
Script:
#!/bin/bash # Search for TC build related strings with correct file extension rg -i "tc build" -t ts -t typescript # Search for translation keys related to TC build rg -i "tc.*build" -g "*.json" # Look for other TODO comments in the same file rg "TODO:" "libs/sr/page-team/src/Optimize/GeneratedBuildsDisplay.tsx" # Check if Dialog component is used elsewhere successfully ast-grep --pattern 'import { $$ Dialog $$ } from "@mui/material"' rg "Dialog" -t typescriptLength of output: 3157
Script:
#!/bin/bash # Search for Dialog usage with correct file type rg "Dialog" -t ts # Check for existing translation keys for TC build in the specific locale file rg "createBuildReal" libs/sr/localization/assets/locales/en/ # Look for existing implementations of TC build functionality ast-grep --pattern 'database.builds.new'Length of output: 5589
libs/sr/stats/src/executors/gen-stats/src/characterData.ts (2)
204-204: Consider adding value validation.While the key validation is good, consider adding validation for the actual values to ensure data integrity.
Add validation for critical fields:
+ // Validate critical character data fields + Object.values(data).forEach((char) => { + if (char.ascension.length === 0) throw new Error('Missing ascension data') + if (Object.keys(char.skillTree).length === 0) throw new Error('Missing skill tree data') + if (Object.keys(char.rankMap).length === 0) throw new Error('Missing rank data') + }) verifyObjKeys(data, allCharacterGenderedKeys)
Line range hint
1-204: Add documentation for complex data transformations.The file contains complex data transformations, particularly in the skill tree and rank map generation. Consider adding:
- JSDoc comments explaining the data structure transformations
- Examples of input/output data shapes
- Documentation for game-specific concepts (e.g., avatarSkillTreeConfig, rankConfig)
Example documentation:
/** * Generates character data by transforming raw game data into a structured format. * * @example * // Input avatarConfig shape: * { * "1001": { Rarity: 5, DamageType: "Fire", ... } * } * * // Output CharacterData shape: * { * "Himeko": { rarity: "5", damageType: "Fire", ... } * } */libs/sr/page-team/src/TeammateDisplay.tsx (1)
Line range hint
171-173: Address the translation TODO comment.The "Change Build" button text needs to be added to the translation system.
Would you like me to help implement the translation for this button text?
libs/sr/ui/src/LightCone/LightConeCardCompact.tsx (3)
98-99: Simplify NextImage component usage.The conditional component usage can be simplified using the nullish coalescing operator.
-component={NextImage ? NextImage : 'img'} +component={NextImage ?? 'img'}
171-173: Remove commented code.There are commented lines related to roll calculations that should be either implemented or removed to maintain code cleanliness.
- // const numRolls = stat.rolls?.length ?? 0 - // const rollColor = `roll${clamp(numRolls, 1, 6)}` - // color={(numRolls ? `${rollColor}.main` : 'error.main') as any}Also applies to: 177-177
164-191: Consider memoizing SubstatDisplay component.Since this component is used within a map function and receives stable props, it could benefit from memoization to prevent unnecessary re-renders.
-function SubstatDisplay({ +const SubstatDisplay = React.memo(function SubstatDisplay({ calc, stat, }: { calc: Calculator stat: 'hp' | 'atk' | 'def' -}) { +}) { return ( <Box display="flex" gap={1} alignContent="center"> {/* ... */} </Box> ) -} +})libs/sr/ui/src/LightCone/LightConeCard.tsx (2)
58-61: Great performance optimization with useMemo!The memoization of
calcis a good optimization since it prevents unnecessary recalculations of potentially expensive operations when the lightCone prop hasn't changed.Consider maintaining consistent indentation with the rest of the codebase:
const calc = useMemo( - () => srCalculatorWithEntries(lightConeData(lightCone)), - [lightCone] + () => srCalculatorWithEntries(lightConeData(lightCone)), + [lightCone] )
Line range hint
34-61: Consider further component decomposition for better maintainability.While the current changes improve performance and modularity, consider breaking down this component further:
- Extract the header section (path icon, name, level) into a separate component
- Create a dedicated StatsSection component for the stats display
- Move the action buttons into a separate ActionButtons component
This would make the component more maintainable and easier to test.
Also applies to: 172-172
libs/sr/ui/src/Relic/RelicCardCompact.tsx (5)
26-27: Consider using responsive dimensionsUsing fixed dimensions (
ELE_HEIGHT = '7em',ELE_WIDTH = '12em') might limit the component's adaptability across different screen sizes. Consider using responsive units or CSS Grid/Flexbox for better adaptability.-const ELE_HEIGHT = '7em' as const -const ELE_WIDTH = '12em' as const +const ELE_HEIGHT = 'clamp(7em, 10vw, 9em)' as const +const ELE_WIDTH = 'clamp(12em, 15vw, 14em)' as const
64-71: Simplify callback memoizationThe memoized callback could be simplified using arrow function syntax.
- const actionWrapperFunc = useCallback( - (children: ReactNode) => ( - <CardActionArea onClick={onClick} sx={{ height: '100%' }}> - {children} - </CardActionArea> - ), - [onClick] - ) + const actionWrapperFunc = useCallback((children: ReactNode) => ( + <CardActionArea onClick={onClick} sx={{ height: '100%' }}> + {children} + </CardActionArea> + ), [onClick])
78-108: Enhance accessibility and maintainability
- Add aria-label for better accessibility
- Consider extracting inline styles to a separate styles object or theme
<CardThemed bgt={bgt} + aria-label={`${setKey} ${slotKey} relic card`} sx={(theme) => ({ display: 'flex', height: ELE_HEIGHT, width: ELE_WIDTH, borderLeft: '5px solid', borderImage: `${theme.palette[`grad${rarity}`].gradient} 1`, })} >
199-222: Clean up technical debt
- Remove or implement the commented-out roll color functionality
- Consider adding proper type validation for stat values
- // const numRolls = stat.rolls?.length ?? 0 - // const rollColor = `roll${clamp(numRolls, 1, 6)}` const statUnit = getUnitStr(stat.key) return ( <Box display="flex" gap={1} alignContent="center"> <Typography sx={{ flexGrow: 1, display: 'flex', gap: 0.5, alignItems: 'center' }} - // color={(numRolls ? `${rollColor}.main` : 'error.main') as any} component="span" >
223-247: Add accessibility label to empty stateConsider adding an aria-label to improve accessibility for screen readers.
<CardThemed bgt={bgt} + aria-label={`Empty ${slotKey} relic slot`} sx={{ display: 'flex', height: ELE_HEIGHT, width: ELE_WIDTH, alignItems: 'center', justifyContent: 'center', }} >libs/sr/ui/src/Character/CharacterEditor.tsx (2)
114-170: LGTM: Consistent implementation of ability dropdowns with icons.The addition of icons improves visual clarity and user experience. The implementation is consistent across all ability types.
Consider extracting the common dropdown configuration into a shared component to reduce code duplication. Example:
type AbilityType = 'basic' | 'skill' | 'ult' | 'talent'; function CharacterAbilityDropdown({ characterKey, abilityType }: { characterKey: CharacterKey, abilityType: AbilityType }) { const genderedKey = characterKeyToGenderedKey(characterKey); return ( <AbilityDropdown characterKey={characterKey} abilityKey={abilityType} dropDownButtonProps={{ startIcon: ( <ImgIcon size={1.5} sideMargin src={characterAsset(genderedKey, `${abilityType}_0`)} /> ), }} /> ); }
211-247: LGTM: Well-structured stat boosts implementation with proper formatting.The refactor improves both code organization and UI presentation. Good use of utility functions for stat calculations and proper unit display.
Consider memoizing the stat calculations if performance profiling shows frequent re-renders:
const statBoostData = useMemo( () => Object.entries(character?.statBoosts ?? {}).map(([statBoost, enabled]) => ({ statBoost, enabled, ...getCharStatBoostStat(genderedKey, statBoost) })), [character?.statBoosts, genderedKey] );libs/sr/ui/src/Relic/RelicCard.tsx (1)
Line range hint
46-350: Consider component decomposition for better maintainabilityThe
RelicCardcomponent handles multiple responsibilities and contains complex nested JSX. Consider breaking it down into smaller, focused components:
RelicCardHeaderfor the top section with level, set name, and starsRelicCardContentfor the main stats and substatsRelicCardActionsfor the bottom section with location and buttonsThis would improve:
- Code maintainability
- Testing capabilities
- Performance through targeted memoization
Example structure:
export function RelicCard(props: RelicCardProps) { return ( <Suspense fallback={<RelicCardSkeleton />}> <CardThemed> <RelicCardHeader relic={props.relic} onLockToggle={props.onLockToggle} /> <RelicCardContent relic={props.relic} mainStatAssumptionLevel={props.mainStatAssumptionLevel} /> <RelicCardActions location={props.relic.location} setLocation={props.setLocation} onEdit={props.onEdit} onDelete={props.onDelete} extraButtons={props.extraButtons} /> </CardThemed> </Suspense> ); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (30)
apps/somnia/src/commands/databank.ts(2 hunks)apps/somnia/src/commands/databank/char.ts(6 hunks)libs/sr/assets-data/src/executors/gen-assets-data/executor.ts(2 hunks)libs/sr/assets/src/index.ts(2 hunks)libs/sr/consts/src/character.ts(2 hunks)libs/sr/db-ui/src/hooks/index.ts(1 hunks)libs/sr/db-ui/src/hooks/useRelic.ts(1 hunks)libs/sr/dm-localization/src/executors/gen-locale/lib/charHashData/allCharacters.ts(3 hunks)libs/sr/dm-localization/src/executors/gen-locale/lib/hashData.ts(2 hunks)libs/sr/dm/src/mapping/character.ts(1 hunks)libs/sr/formula-ui/src/util.tsx(2 hunks)libs/sr/page-team/src/BuildsDisplay.tsx(4 hunks)libs/sr/page-team/src/Optimize/GeneratedBuildDisplay.tsx(0 hunks)libs/sr/page-team/src/Optimize/GeneratedBuildsDisplay.tsx(2 hunks)libs/sr/page-team/src/TeammateDisplay.tsx(3 hunks)libs/sr/stats/src/executors/gen-stats/src/characterData.ts(3 hunks)libs/sr/stats/src/index.ts(4 hunks)libs/sr/theme/src/Theme.tsx(3 hunks)libs/sr/ui/src/Character/CharacterCard.tsx(3 hunks)libs/sr/ui/src/Character/CharacterEditor.tsx(6 hunks)libs/sr/ui/src/Character/CharacterTrans.tsx(1 hunks)libs/sr/ui/src/Components/LocationName.tsx(1 hunks)libs/sr/ui/src/Components/index.ts(1 hunks)libs/sr/ui/src/LightCone/LightConeCard.tsx(3 hunks)libs/sr/ui/src/LightCone/LightConeCardCompact.tsx(1 hunks)libs/sr/ui/src/LightCone/index.tsx(1 hunks)libs/sr/ui/src/Relic/RelicCard.tsx(2 hunks)libs/sr/ui/src/Relic/RelicCardCompact.tsx(1 hunks)libs/sr/ui/src/Relic/index.tsx(1 hunks)libs/sr/ui/src/index.tsx(1 hunks)
💤 Files with no reviewable changes (1)
- libs/sr/page-team/src/Optimize/GeneratedBuildDisplay.tsx
✅ Files skipped from review due to trivial changes (1)
- libs/sr/ui/src/Components/index.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- libs/sr/assets/src/index.ts
- libs/sr/consts/src/character.ts
- libs/sr/ui/src/Relic/index.tsx
🧰 Additional context used
📓 Learnings (1)
libs/sr/ui/src/Character/CharacterCard.tsx (1)
Learnt from: tiffanynwyeung
PR: frzyc/genshin-optimizer#2521
File: libs/sr/ui/src/Character/CharacterCard.tsx:21-31
Timestamp: 2024-10-29T18:28:48.475Z
Learning: Character stat displays should match the in-game order: hp > atk > def > spd > crit > crit dmg > br effect > heal > energy regen > effect hit rate > effect res.
🪛 Biome
libs/sr/page-team/src/BuildsDisplay.tsx
[error] 166-171: 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 (56)
libs/sr/ui/src/index.tsx (1)
2-2: LGTM! Clean addition of Components module export.
The new export statement follows the established pattern and maintains alphabetical ordering. This change appropriately exposes the Components module, which contains the LocationName component needed for the UI updates.
libs/sr/db-ui/src/hooks/index.ts (2)
4-4: LGTM! Clean addition following established patterns.
The new export follows the existing pattern and maintains a logical order among related hooks.
4-4: Verify the useRelic hook implementation.
Let's ensure the hook implementation exists and follows consistent patterns with other hooks.
✅ Verification successful
The useRelic hook implementation is verified and follows consistent patterns
The implementation is correct and follows the established patterns:
- Uses
useDataManagerBasefrom common database-ui package - Properly integrates with the database context
- Has consistent usage in the codebase (RelicCardCompact, TeamCalcProvider)
- Follows the same pattern as related hook
useRelics
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the useRelic hook implementation and its usage patterns
# Check if the useRelic hook file exists and examine its implementation
echo "Checking useRelic hook implementation:"
fd --type f "useRelic.ts" --exec cat {}
# Look for usage patterns to ensure consistency
echo -e "\nChecking usage patterns across the codebase:"
rg "useRelic" --type ts
Length of output: 1184
libs/sr/ui/src/LightCone/index.tsx (1)
2-2: LGTM! Clean barrel file update.
The new export follows the established pattern and maintains alphabetical ordering. This change appropriately exposes the new compact light cone card component, aligning with the PR's UI enhancement objectives.
libs/sr/db-ui/src/hooks/useRelic.ts (2)
1-2: LGTM! Clean and well-organized imports.
The imports follow a clear separation between common and local functionality, with appropriate naming conventions.
3-6: Verify hook usage patterns.
Let's verify how this hook is being used across the codebase to ensure proper integration.
✅ Verification successful
Based on the search results, I can see that the useRelic hook is used in several components and is properly integrated with the database system. The key findings are:
- The hook is used in
RelicCardCompactcomponent for displaying relic information - The hook is properly implemented in
db-uilibrary usinguseDataManagerBasewith the database's relic manager - The database has proper relic management functionality with validation and state management
- The hook follows the same pattern as other similar hooks in the codebase
Let me verify one more aspect to ensure there are no breaking changes:
Let me try a different search pattern to verify the hook implementations:
Based on the search results, I can now provide a definitive response about the useRelic hook:
Hook implementation follows established patterns
The useRelic hook follows the same consistent pattern as other database hooks in the codebase:
- Uses
useDatabaseContext()to access the database - Returns
useDataManagerBase(database.relics, relicId) - Matches implementation pattern of other hooks like
useLightCone,useCharacter, etc. - Is properly integrated with the database system and state management
The hook is being used correctly in components and there are no breaking changes from previous implementations.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check usage patterns of useRelic hook
# Find direct imports and usages
echo "=== Direct useRelic hook usages ==="
rg -l "useRelic" --type ts --type tsx
# Find existing relic-related hooks for comparison
echo -e "\n=== Other relic-related hooks ==="
rg -l "useRelic\w+" --type ts --type tsx
# Check for potential breaking changes by finding old relic management patterns
echo -e "\n=== Existing relic management patterns ==="
rg "database\.relics" --type ts --type tsx -A 2
Length of output: 479
Script:
#!/bin/bash
# Let's try a different approach without specifying file types
# First, find all TypeScript/React files
echo "=== Finding TypeScript/React files ==="
fd "\.(ts|tsx)$"
# Then search for useRelic usages
echo -e "\n=== Direct useRelic hook usages ==="
rg "useRelic" -A 2
# Look for database.relics patterns
echo -e "\n=== Existing relic management patterns ==="
rg "database\.relics" -A 2
# Look for any relic-related imports or hooks
echo -e "\n=== Relic-related patterns ==="
rg "relic" -i
Length of output: 418025
Script:
#!/bin/bash
# Check the implementation of useDataManagerBase to verify hook pattern
ast-grep --pattern 'function useDataManagerBase($_) {
$$$
}'
# Check for any other similar hook patterns
ast-grep --pattern 'function use$_($_) {
const { database } = useDatabaseContext()
return useDataManagerBase(database.$_, $_)
}'
Length of output: 199
Script:
#!/bin/bash
# Check the database hook implementations
echo "=== Database Hook Implementations ==="
rg "export function use.*\(" libs/sr/db-ui/src/hooks/ -A 3
# Check for any breaking changes in database context usage
echo -e "\n=== Database Context Usage ==="
rg "useDatabaseContext" -A 2
Length of output: 21114
libs/sr/ui/src/Character/CharacterTrans.tsx (3)
1-2: LGTM! Well-documented client directive.
The 'use client' directive is correctly placed and well-documented with an explanation of its necessity for handling translation hydration differences.
3-4: LGTM! Clean and well-structured imports.
The imports are properly typed and well-scoped to the component's needs.
11-11: Verify translation namespace existence.
Let's ensure the "charNames_gen" namespace is properly defined in the translation files.
✅ Verification successful
Let me verify the actual content of the translation files to ensure the namespace is properly defined.
Let me try one more search strategy to locate the translation files and their content.
Based on the evidence from our searches, I can generate the final response:
Translation namespace integration verified
The charNames_gen namespace is well-integrated across the codebase:
- It's properly registered in both SR and GI i18n configurations (
libs/sr/i18n-node/src/i18n.tsandlibs/gi/i18n-node/src/i18n.ts) - It's consistently used across multiple components for character name translations
- The namespace is part of the core translation namespaces loaded by default
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if the charNames_gen namespace exists in translation files
# Look for translation files and check for the namespace
rg -g "*.json" -g "*.ts" -g "*.tsx" "charNames_gen"
Length of output: 3438
Script:
#!/bin/bash
# Search for translation files containing charNames_gen definitions
fd -e json -x cat {} \; | jq 'select(."charNames_gen" != null) | ."charNames_gen"' 2>/dev/null || true
# Also check TypeScript/JavaScript files for namespace registration
rg -g '*.ts' -g '*.tsx' 'registerLocale.*charNames_gen'
Length of output: 145
Script:
#!/bin/bash
# Look for translation files in common i18n locations
fd -g "*locale*.json" -g "*i18n*.json" -g "*translations*.json" -g "*en.json" -g "*zh.json"
# Also check specific paths where i18n files might be located based on the imports
fd -p "libs/sr/i18n-node/src" -p "libs/gi/i18n-node/src" -t f -e json
Length of output: 436
libs/sr/ui/src/Components/LocationName.tsx (2)
1-11: LGTM! Imports are well-organized and properly typed.
All imports are being utilized and appropriately structured.
1-39: Verify consistent usage across the codebase.
Let's ensure this component is being used consistently across the codebase where location display is needed.
✅ Verification successful
LocationName component is consistently used across the codebase
The component is being used appropriately in all relevant places where location display is needed:
- Used in Relic cards (
RelicCard.tsx,RelicCardCompact.tsx) - Used in LightCone cards (
LightConeCard.tsx,LightConeCardCompact.tsx) - Used in Artifact components (
ArtifactCard.tsx,ArtifactTooltip.tsx) - Used in Weapon card (
WeaponCard.tsx)
There are no instances of inconsistent location display patterns that should be using this component instead.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for potential locations where this component should be used
echo "Searching for direct Typography usage with location props..."
rg -t tsx -t jsx "Typography.*location[^a-zA-Z]" --glob '!LocationName.tsx'
echo -e "\nSearching for potential character location rendering patterns..."
rg -t tsx -t jsx "characterKeyToGenderedKey.*location[^a-zA-Z]" --glob '!LocationName.tsx'
Length of output: 502
Script:
#!/bin/bash
# Search for potential locations where this component should be used
echo "Searching for direct Typography usage with location props..."
rg "Typography.*location[^a-zA-Z]" --type-add 'tsx:*.tsx' --type-add 'jsx:*.jsx' --type tsx --type jsx --glob '!LocationName.tsx'
echo -e "\nSearching for potential character location rendering patterns..."
rg "characterKeyToGenderedKey.*location[^a-zA-Z]" --type-add 'tsx:*.tsx' --type-add 'jsx:*.jsx' --type tsx --type jsx --glob '!LocationName.tsx'
echo -e "\nSearching for LocationName component usage..."
rg "<LocationName" --type-add 'tsx:*.tsx' --type-add 'jsx:*.jsx' --type tsx --type jsx
Length of output: 1467
libs/sr/formula-ui/src/util.tsx (2)
Line range hint 1-7: LGTM: Clean type import addition
The addition of CharacterGenderedKey type import is well-structured and maintains consistency with existing imports.
23-25: Verify type consistency between overloads and implementation
The overload for 'char' type now uses CharacterGenderedKey, but the implementation still accepts NonTrailblazerCharacterKey | TrailblazerGenderedKey. This might lead to runtime type mismatches.
✅ Verification successful
Based on the search results, I can now generate the final response:
Type change appears to be correct and consistent
The type CharacterGenderedKey is defined as a union of all character keys including both non-Trailblazer and Trailblazer gendered keys (from allCharacterGenderedKeys). This is actually more type-safe than the previous implementation as it properly represents all possible character keys. The usage across the codebase (found in various character sheet files) shows consistent usage with character key constants, which would be compatible with the new type.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for usage of trans function with 'char' type to ensure compatibility
rg -t typescript -t tsx "trans\(['\"]char['\"]" --context 3
Length of output: 100
Script:
#!/bin/bash
# Search for usage of trans function with 'char' type using ripgrep
# Note: Using just 'ts' and 'tsx' extensions instead of file types
rg "trans\(['\"]char['\"]" -g "*.{ts,tsx}" --context 3
# Also search for the implementation of trans function
ast-grep --pattern 'function trans($_) {
$$$
}'
Length of output: 4770
Script:
#!/bin/bash
# Let's find the actual implementation of trans function
ast-grep --pattern 'export function trans(typeKey: $_, key: $_) {
$$$
}'
# Also search for the type definitions
rg "type (CharacterGenderedKey|NonTrailblazerCharacterKey|TrailblazerGenderedKey)" -g "*.{ts,tsx}" --context 2
Length of output: 1551
libs/sr/dm/src/mapping/character.ts (2)
2-2: LGTM! Import changes are appropriate.
The addition of CharacterGenderedKey import aligns with the type signature changes and follows the project's import conventions.
10-10: Verify type compatibility in dependent code.
The type change from CharacterDataKey to CharacterGenderedKey is appropriate given the presence of gendered character variants. However, we should ensure this change doesn't break any consuming code.
Let's verify the usage of this type in dependent code:
✅ Verification successful
Type change is safe and correctly implemented
The change from CharacterDataKey to CharacterGenderedKey is verified to be safe. The type CharacterGenderedKey is properly defined in libs/sr/consts/src/character.ts and all usages of characterIdMap across the codebase are consistent with this type. The consuming code uses it correctly for:
- Character asset mapping
- Stats generation
- Localization handling
- Character data processing
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for direct usage of characterIdMap to ensure compatibility
rg -l "characterIdMap" --type ts
# Search for any remaining usage of CharacterDataKey type
rg -l "CharacterDataKey" --type ts
Length of output: 800
Script:
#!/bin/bash
# Let's examine the actual usage of characterIdMap in SR-related files
rg "characterIdMap" libs/sr/dm/src/mapping/character.ts -A 2 -B 2
rg "characterIdMap" libs/sr/assets-data/src/executors/gen-assets-data/executor.ts -A 2 -B 2
rg "characterIdMap" libs/sr/stats/src/executors/gen-stats/src/characterData.ts -A 2 -B 2
rg "characterIdMap" libs/sr/dm-localization/src/executors/gen-locale/lib/charHashData/allCharacters.ts -A 2 -B 2
# Let's also check the definition of CharacterGenderedKey
rg "CharacterGenderedKey" libs/sr/dm/src/ -A 2 -B 2
Length of output: 2071
Script:
#!/bin/bash
# Let's check the definition and usage of CharacterGenderedKey type
ast-grep --pattern 'type CharacterGenderedKey = $_'
# Let's also check if there are any type errors or incompatible usages
rg "characterIdMap\[.*\]" -A 2 -B 2 --type ts
Length of output: 12516
libs/sr/dm-localization/src/executors/gen-locale/lib/hashData.ts (1)
3-3: Verify the impact of switching to gendered character keys.
The change from allCharacterDataKeys to allCharacterGenderedKeys suggests an enhancement in character gender handling. Let's verify that this change is consistently applied across the codebase.
✅ Verification successful
Based on the shell script results, I can now generate the final response:
The change to allCharacterGenderedKeys is consistently implemented
The codebase shows a clean transition to the new gendered keys system:
- No remaining references to the old
allCharacterDataKeys - Consistent usage of
allCharacterGenderedKeysacross multiple files - No lingering
CharacterDataKeytype references found
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining references to the old key type and verify consistent usage of the new type
# Test 1: Check for any remaining references to allCharacterDataKeys
echo "Checking for remaining references to allCharacterDataKeys..."
rg "allCharacterDataKeys"
# Test 2: Verify the new type is properly imported and used
echo "Verifying usage of allCharacterGenderedKeys..."
rg -A 2 "allCharacterGenderedKeys"
# Test 3: Look for potential type-related issues in character handling
echo "Checking for potential character key type issues..."
ast-grep --pattern 'CharacterDataKey'
Length of output: 2517
libs/sr/stats/src/index.ts (4)
2-4: LGTM! Type imports are properly aligned with the changes.
The addition of CharacterGenderedKey and StatBoostKey types supports the updated function signatures and new utility functions.
18-18: LGTM! Improved type consistency.
The consolidation of character key types to CharacterGenderedKey enhances type safety and maintains consistent typing across the codebase.
Also applies to: 31-31
56-85: LGTM! Robust error handling implementation.
The implementation includes proper null checks and clear error messages, making it easier to diagnose issues when they occur. The separation into specific utility functions improves code organization and reusability.
86-92: Consider implementing type-safe property access.
The string template literal access to skillTree properties could benefit from additional type safety.
See the previous review comment about adding type safety for skillTree property access:
#2529 (comment)
libs/sr/dm-localization/src/executors/gen-locale/lib/charHashData/allCharacters.ts (3)
2-5: LGTM! Import changes align with the type system updates.
The transition from CharacterDataKey to CharacterGenderedKey is cleanly implemented in the imports.
Also applies to: 8-8
Line range hint 121-128: LGTM! Type annotation correctly updated.
The tuple type annotation has been properly updated to use CharacterGenderedKey while maintaining the existing data structure.
133-133: Verify consistent key validation across the codebase.
The verification step has been correctly updated to use allCharacterGenderedKeys.
Let's verify that this validation pattern is consistently applied across the codebase:
✅ Verification successful
Key validation is consistently implemented across the codebase
The verification using allCharacterGenderedKeys is consistently applied in all relevant places:
- Character data validation in stats generation (
libs/sr/stats/src/executors/gen-stats/src/characterData.ts) - Character hash data validation (
libs/sr/dm-localization/src/executors/gen-locale/lib/charHashData/allCharacters.ts) - Character names validation in hash data (
libs/sr/dm-localization/src/executors/gen-locale/lib/hashData.ts)
All instances properly verify their data structures against the canonical allCharacterGenderedKeys defined in libs/sr/consts/src/character.ts.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other instances of verifyObjKeys to ensure consistent validation
rg "verifyObjKeys.*allCharacter" --type ts
Length of output: 396
Script:
#!/bin/bash
# Let's check the implementation context of these verifications
rg "verifyObjKeys.*allCharacter" -B 3 -A 3 --type ts
Length of output: 1829
Script:
#!/bin/bash
# Let's check the definition and usage of allCharacterGenderedKeys
rg "allCharacterGenderedKeys" -B 2 -A 2 --type ts
Length of output: 3420
apps/somnia/src/commands/databank/char.ts (2)
2-5: LGTM: Type import changes enhance type safety
The addition of CharacterGenderedKey type import aligns with the codebase-wide transition to better handle character gender variations.
16-16: Verify function call sites for type compatibility
The parameter type changes from CharacterDataKey to CharacterGenderedKey are consistent across all functions. However, we should verify that all callers of these functions have been updated to pass the correct type.
Also applies to: 39-39, 43-43, 60-60, 79-79, 105-105
✅ Verification successful
Type changes are correctly implemented and used
The verification shows that:
- The caller in
databank.tscorrectly casts the parameter toCharacterGenderedKeywhen callingcharBank - No remaining usages of
CharacterDataKeyexist in the codebase
The parameter type changes are consistent and properly implemented across all call sites.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for function calls to ensure they're using the new CharacterGenderedKey type
echo "Searching for function calls..."
rg -A 2 "getEmbed|getAssets|baseEmbed|skillEmbed|eidolonsEmbed|charBank" --type typescript
# Search for any remaining uses of CharacterDataKey
echo "Checking for remaining CharacterDataKey usage..."
rg "CharacterDataKey" --type typescript
Length of output: 382
Script:
#!/bin/bash
# Search for function calls without file type restriction
echo "Searching for function calls..."
rg -A 2 "getEmbed|getAssets|baseEmbed|skillEmbed|eidolonsEmbed|charBank"
# Search for any remaining uses of CharacterDataKey
echo "Checking for remaining CharacterDataKey usage..."
rg "CharacterDataKey"
# Search for files with .ts extension to verify TypeScript files
echo "Listing TypeScript files..."
fd "\.ts$"
Length of output: 106216
Script:
#!/bin/bash
# Search for usages of the functions with CharacterGenderedKey parameter
echo "Searching for function calls in databank.ts..."
rg -A 2 "charBank\(" apps/somnia/src/commands/databank.ts
# Search for any remaining uses of CharacterDataKey
echo "Checking for CharacterDataKey usage..."
rg "CharacterDataKey" apps/somnia/src/commands/databank/char.ts
Length of output: 402
libs/sr/assets-data/src/executors/gen-assets-data/executor.ts (2)
3-3: LGTM! Import addition is well-organized.
The new CharacterGenderedKey type import is properly grouped with related type imports.
64-64: Verify compatibility with character ID mapping system.
The type change from CharacterDataKey to CharacterGenderedKey looks good, but requires verification:
- Ensure that
characterIdMapcorrectly maps avatar IDs to gendered keys - Consider updating documentation for consumers of
AssetsData_gen.jsonabout this structural change
Let's verify the character ID mapping implementation:
✅ Verification successful
Let me gather more information about the character keys to verify the mapping compatibility.
Let me check one more aspect to verify the character key mapping completeness.
Based on the gathered information, I can now provide the final response:
The character ID mapping system is correctly implemented for the type change
The verification shows that:
- The
characterIdMapinlibs/sr/dm/src/mapping/character.tscorrectly maps IDs toCharacterGenderedKey - The
CharacterGenderedKeytype is properly defined as a union of non-Trailblazer characters and gendered Trailblazer variants - The mapping is used consistently across the codebase in character-related functionality
The type change is safe and properly integrated with the character ID mapping system.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check characterIdMap implementation and usage
# Expected: Find the implementation to confirm it generates valid CharacterGenderedKey values
# Search for characterIdMap definition and usage
rg -A 5 "characterIdMap\s*=|characterIdMap\s*:"
# Search for any CharacterGenderedKey type definitions or validations
ast-grep --pattern 'type CharacterGenderedKey = $_'
Length of output: 6570
Script:
#!/bin/bash
# Check the definition of allCharacterGenderedKeys and related types
rg -A 10 "allCharacterGenderedKeys\s*=" libs/sr/consts/src/character.ts
# Check if there are any direct usages of CharacterGenderedKey type
rg "CharacterGenderedKey" -A 2 -B 2
# Look for any character-related type definitions
ast-grep --pattern 'type CharacterDataKey = $_'
Length of output: 19263
Script:
#!/bin/bash
# Check the definition of nonTrailblazerCharacterKeys and allTrailblazerGenderedKeys
rg -B 5 "nonTrailblazerCharacterKeys\s*=|allTrailblazerGenderedKeys\s*=" libs/sr/consts/src/character.ts
# Check for any character key validation in the codebase
rg "verifyObjKeys.*character" -A 2 -B 2
Length of output: 492
libs/sr/ui/src/Character/CharacterCard.tsx (3)
Line range hint 35-46: LGTM: Stats array follows the correct in-game order.
The stats array implementation correctly follows the in-game order: hp > atk > def > spd > crit > crit dmg > br effect > heal > energy regen > effect hit rate > effect res, as previously established.
61-67: LGTM: Clean conditional rendering pattern.
The conditional rendering of CardActionArea is well-implemented, avoiding unnecessary wrapper components when onClick is not provided.
Line range hint 119-143: Verify consistent stat calculation across components.
Let's verify that other components using character stats follow the same calculation pattern with proper tagging.
✅ Verification successful
Based on the search results, I can now provide a final response:
Stat calculation pattern is consistently implemented across components
The code review shows consistent stat calculation patterns:
- All stat displays use the same
StatDisplaycomponent withstatKeyprop - Stat calculations consistently use
calc.compute()with proper tagging - Value formatting follows a standard pattern using
toPercent()andstatToFixed()utilities - The implementation in
CharacterCard.tsxfollows the same pattern used in other components likeRelicCardandLightConeCard
The changes in the review are consistent with the established patterns in the codebase for handling stat calculations and displays.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other components using stat calculations
rg -l "withTag.*src:.*dst:" --type typescript
# Search for direct usage of calc.compute without proper tagging
rg "calc\.compute\([^)]*\)" --type typescript
Length of output: 165
Script:
#!/bin/bash
# Search for other components using stat calculations with proper file extension
rg -l "withTag.*src:.*dst:" --type ts --type tsx
# Search for direct usage of calc.compute without proper tagging
rg "calc\.compute\([^)]*\)" -A 2 --type ts --type tsx
# Search for files that might be using character stats
rg -l "Calculator|CharacterKey|statKey" --type ts --type tsx
Length of output: 253
Script:
#!/bin/bash
# Search for other components using stat calculations without file type flags
rg -l "withTag.*src:.*dst:"
# Search for direct usage of calc.compute with context
rg "calc\.compute\([^)]*\)" -A 2
# Search for files that might be using character stats
rg -l "Calculator|CharacterKey|statKey"
# List TypeScript/React files to understand the file patterns
fd "\.(ts|tsx)$"
Length of output: 175937
Script:
#!/bin/bash
# Search for stat calculation patterns in the codebase
rg "calc\.compute.*withTag" -A 2
# Search for stat display components to verify consistent usage
rg "StatDisplay.*statKey" -A 2
# Search for files that might contain stat calculation logic
rg "toPercent.*statKey|statToFixed.*statKey"
Length of output: 4666
libs/sr/theme/src/Theme.tsx (2)
43-57: LGTM! Correct implementation of optional theme properties.
The PaletteOptions interface extension properly mirrors the Palette interface with optional properties, following Material-UI's type system patterns.
Line range hint 16-166: Verify consistent gradient usage across the application.
Since these gradients are part of the theme system and likely used for character cards (based on PR description), we should verify their consistent usage.
libs/sr/page-team/src/BuildsDisplay.tsx (3)
Line range hint 23-52: LGTM! Well-structured component with proper hook usage.
The BuildsDisplay component follows React best practices with appropriate use of hooks and clean separation of concerns.
107-110: LGTM! Clean implementation of EquippedBuild.
The component effectively uses the new EquipRow component for a more streamlined display.
122-123: LGTM! Consistent implementation with EquippedBuild.
The Build component maintains consistency with EquippedBuild in using the new EquipRow component.
libs/sr/page-team/src/Optimize/GeneratedBuildsDisplay.tsx (2)
24-24: LGTM: Import changes align with component replacement.
The import change correctly reflects the architectural shift from GeneratedBuildDisplay to EquipRow component.
61-64: LGTM: Component replacement maintains data flow.
The EquipRow integration correctly handles the build data props and aligns with the PR's objective of implementing a more compact UI for builds display.
libs/sr/stats/src/executors/gen-stats/src/characterData.ts (2)
75-75: LGTM! Type definition is properly updated.
The CharacterData type definition correctly uses CharacterGenderedKey as the record key type, maintaining type safety while supporting the new gendered key system.
7-7: Verify complete migration to gendered keys.
The change from CharacterDataKey to CharacterGenderedKey looks good, but we should ensure all consumers have been updated.
Also applies to: 9-9
✅ Verification successful
Migration to CharacterGenderedKey is complete
No references to the old CharacterDataKey or allCharacterDataKeys were found in the TypeScript codebase. The SQL files found in the search results are unrelated database migrations that don't reference these types. The migration appears to be complete and consistent.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining references to the old CharacterDataKey type
# to ensure complete migration
# Test 1: Search for any remaining references to CharacterDataKey
echo "Checking for remaining CharacterDataKey references..."
rg "CharacterDataKey"
# Test 2: Search for any remaining references to allCharacterDataKeys
echo "Checking for remaining allCharacterDataKeys references..."
rg "allCharacterDataKeys"
Length of output: 289
Script:
#!/bin/bash
# Let's try a more comprehensive search with case-insensitive and file type filters
# to ensure we don't miss any references
# Search for CharacterDataKey in TypeScript files
echo "Searching for CharacterDataKey in TypeScript files..."
rg -t ts "CharacterDataKey" -i
# Search for allCharacterDataKeys in TypeScript files
echo "Searching for allCharacterDataKeys in TypeScript files..."
rg -t ts "allCharacterDataKeys" -i
# Let's also check for any potential type imports related to CharacterDataKey
echo "Checking type imports..."
ast-grep --pattern 'import { $$$CharacterDataKey$$$ } from $_'
# And check for type aliases or interfaces extending/using CharacterDataKey
echo "Checking type definitions..."
ast-grep --pattern 'type $_ = $$$CharacterDataKey$$$'
ast-grep --pattern 'interface $_ extends $$$CharacterDataKey$$$'
Length of output: 6235
libs/sr/page-team/src/TeammateDisplay.tsx (3)
29-29: LGTM: Import statement updated correctly.
The import change aligns with the component replacement from BuildGridBase to EquipRow.
193-197: LGTM: Modal width constraint added.
The addition of maxWidth constraint improves the modal's layout and prevents it from becoming too wide on large screens.
180-180: Verify the compact layout implementation.
The replacement of BuildGridBase with EquipRow aligns with the PR's objective for a compact version of the relic and lightcone card. However, please confirm that:
- The removal of the
columns={2}prop doesn't affect the desired layout - The new EquipRow component provides the compact view as intended
Let's check the EquipRow implementation:
✅ Verification successful
Let me try a different search pattern to find the EquipRow component:
Let me try searching for the file containing EquipRow since it might be in a different location:
Let me try one more search with ripgrep to find any references to EquipRow in the codebase:
Let's check the implementation of EquipRow in BuildsDisplay.tsx since we found its definition there:
Layout implementation is correctly handled in EquipRow
The removal of columns={2} prop is appropriate as EquipRow implements its own layout using flexbox:
- Uses
display: 'flex'withflexWrap: 'wrap'for responsive layout - Includes
gap: 1for consistent spacing - Components use compact variants (
LightConeCardCompactandRelicCardCompact)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for EquipRow implementation to verify layout handling
ast-grep --pattern 'export const EquipRow = $$$'
Length of output: 5664
Script:
#!/bin/bash
# Search for EquipRow component with rg to get more context
rg -A 10 "export const EquipRow"
Length of output: 34
Script:
#!/bin/bash
# Find files containing EquipRow
fd EquipRow
Length of output: 13
Script:
#!/bin/bash
# Search for any mentions of EquipRow in the codebase
rg "EquipRow"
Length of output: 707
Script:
#!/bin/bash
# Get the EquipRow implementation from BuildsDisplay.tsx
rg -A 20 "export function EquipRow" libs/sr/page-team/src/BuildsDisplay.tsx
Length of output: 535
libs/sr/ui/src/LightCone/LightConeCardCompact.tsx (3)
1-29: LGTM! Well-organized imports and constants.
The imports are logically grouped and the constants are properly typed for consistent sizing.
30-48: LGTM! Clean wrapper component with proper null handling.
The component follows good practices:
- Proper TypeScript typing for props
- Handles the null case gracefully
- Single responsibility principle
192-211: LGTM! Clean empty state implementation.
The empty state component is well-implemented with consistent styling and proper visual feedback.
apps/somnia/src/commands/databank.ts (3)
16-16: LGTM: Type import addition aligns with system changes.
The addition of CharacterGenderedKey import is part of a broader system change to better handle gendered character variations.
Line range hint 93-102: Consider updating Trailblazer handling for future compatibility.
The hardcoded Trailblazer entries and TODO comment suggest this might need updates when new Trailblazer paths are added.
Let's check for other Trailblazer-related code that might need updates:
Consider creating a more maintainable system for handling Trailblazer variations, perhaps through configuration rather than hardcoding.
182-182: Verify the impact of type casting change.
The type casting to CharacterGenderedKey looks correct, but we should verify that all character IDs passed to this function are compatible with the new type.
Let's verify the character ID handling:
✅ Verification successful
Based on the gathered information, I can now provide a final response. The type casting is safe because:
- The
CharacterGenderedKeytype is properly defined as a union of all valid character keys - The character IDs used in the codebase (
TrailblazerPhysicalF,TrailblazerFireF, etc.) are all included in theallCharacterGenderedKeysarray - The databank entries in
databank.tsuse the same character ID keys that match the type definition
The type casting to CharacterGenderedKey is safe and correctly implemented. The character IDs used throughout the codebase are consistent with the type definition, and there's no risk of incompatible IDs being passed to the function.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for potential incompatible character ID usage
# Search for direct calls to databankMessage with character IDs
rg -A 2 "databankMessage\('char'"
# Check character ID handling in related files
rg -A 2 "TrailblazerPhysicalF|TrailblazerFireF"
Length of output: 1378
Script:
#!/bin/bash
# Let's check for character ID usage patterns and type definitions
# Find the CharacterGenderedKey type definition
ast-grep --pattern 'type CharacterGenderedKey = $_'
# Check for character ID usage in databank.ts
rg -A 3 "charBank\(" apps/somnia/src/commands/databank.ts
# Look for any character ID constants or enums
ast-grep --pattern 'const $_ = {
$$$
Trailblazer: $_,
$$$
}'
# Check for direct character ID string literals
rg "TrailblazerPhysicalF|TrailblazerPhysicalM|TrailblazerFireF|TrailblazerFireM|TrailblazerImaginaryF|TrailblazerImaginaryM"
Length of output: 18463
Script:
#!/bin/bash
# Let's check the actual implementation of charBank and its usage
# Find charBank function definition
ast-grep --pattern 'function charBank($_) {
$$$
}'
# Check for the type definition and usage of allCharacterGenderedKeys
ast-grep --pattern 'const allCharacterGenderedKeys = $_'
# Look for any character ID validation or type checks
rg -A 3 "CharacterGenderedKey" libs/sr/consts/src/character.ts
Length of output: 6295
libs/sr/ui/src/LightCone/LightConeCard.tsx (2)
28-28: LGTM: Import additions are well-organized.
The new imports for useMemo and LocationName are properly organized with related imports and follow React best practices.
Also applies to: 31-31
172-172: Verify LocationName component's empty state handling.
The switch to LocationName component improves modularity, but we should verify its behavior with empty location values.
Let's check how LocationName handles empty values:
✅ Verification successful
LocationName component properly handles empty location values
The component implementation shows proper handling of empty location values:
- When location is truthy: Displays character icon and name
- When location is falsy: Shows a fallback with BusinessCenterIcon and "inventory" text
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check LocationName component's empty state handling
# Search for LocationName component implementation
ast-grep --pattern 'export function LocationName({ location }: { location: string }) {
$$$
}'
# Search for tests related to LocationName
rg -l "LocationName.*test"
Length of output: 126
Script:
#!/bin/bash
# Let's try a broader search for LocationName component
ast-grep --pattern 'LocationName'
# Also search for the component definition with ripgrep
rg -A 5 "export.*LocationName"
# Search for any file containing LocationName
fd "LocationName"
Length of output: 2668
Script:
#!/bin/bash
# Let's examine the LocationName component implementation in SR
cat libs/sr/ui/src/Components/LocationName.tsx
# Also check for any type definitions of LocationKey
rg "type LocationKey"
Length of output: 1519
libs/sr/ui/src/Relic/RelicCardCompact.tsx (1)
29-52: LGTM! Well-structured wrapper component
The component follows good practices:
- Clear prop types with TypeScript
- Proper handling of empty states
- Good separation of concerns
libs/sr/ui/src/Character/CharacterEditor.tsx (3)
4-4: LGTM: Import additions are well-organized and purposeful.
The new imports are properly categorized and all are utilized within the component.
Also applies to: 7-11, 15-16
194-201: LGTM: Consistent icon implementation for bonus abilities.
The addition of icons maintains visual consistency with the ability dropdowns while improving the UI.
59-59: Verify the impact of typography variant change.
The heading hierarchy has been changed from h2 to h3. While this works for this component, we should verify it doesn't disrupt the overall page hierarchy.
Run this script to check the heading hierarchy across the application:
Also applies to: 64-66
✅ Verification successful
Typography variant change from h2 to h3 is appropriate
The change from h2 to h3 in CharacterEditor is correct as it appears to be a sub-section within the application. The search results show that h2 variants are primarily used in top-level components like ResinCounter, TeyvatTime, and ResinCard which represent main sections, while our CharacterEditor component likely sits at a lower hierarchy level.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check Typography variant usage across the codebase
# Look for potential hierarchy issues with h2/h3 usage
echo "Checking Typography variant hierarchy..."
rg -A 1 "Typography.*variant=\"h[23]\"" --type tsx
Length of output: 169
Script:
#!/bin/bash
# Check Typography variant usage across the codebase with correct file extension
# Look for potential hierarchy issues with h2/h3 usage
echo "Checking Typography variant hierarchy..."
rg -A 1 "Typography.*variant=\"h[23]\"" --type ts --type tsx --type jsx --type js
# Also check for MUI Typography imports to ensure we're searching relevant files
echo -e "\nChecking Typography imports..."
rg "@mui.*Typography" --type ts --type tsx --type jsx --type js
Length of output: 371
Script:
#!/bin/bash
# Search for Typography usage without specifying file types
# ripgrep will automatically detect text files
echo "Checking Typography variant hierarchy..."
rg -A 1 "Typography.*variant=['\"]h[23]['\"]"
echo -e "\nChecking Typography imports..."
rg "@mui.*Typography"
Length of output: 1075
libs/sr/ui/src/Relic/RelicCard.tsx (2)
40-40: LGTM: Clean import addition
The import statement is properly organized with other component imports.
254-254: LGTM: Clean component replacement
The LocationName component is appropriately used to replace the previous implementation, maintaining the conditional rendering logic while potentially improving the location display functionality.
| <span> | ||
| <BusinessCenterIcon sx={{ verticalAlign: 'text-bottom' }} />{' '} | ||
| {t('inventory')} | ||
| </span> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add accessibility attributes to inventory fallback.
The inventory fallback should have proper accessibility attributes.
Apply these changes:
-<span>
+<span role="group" aria-label={t('inventory')}>
- <BusinessCenterIcon sx={{ verticalAlign: 'text-bottom' }} />
+ <BusinessCenterIcon sx={{ verticalAlign: 'text-bottom' }} aria-hidden="true" />
{' '}
{t('inventory')}
</span>📝 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.
| <span> | |
| <BusinessCenterIcon sx={{ verticalAlign: 'text-bottom' }} />{' '} | |
| {t('inventory')} | |
| </span> | |
| <span role="group" aria-label={t('inventory')}> | |
| <BusinessCenterIcon sx={{ verticalAlign: 'text-bottom' }} aria-hidden="true" />{' '} | |
| {t('inventory')} | |
| </span> |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
libs/sr/page-team/src/BuildsDisplay.tsx (2)
Line range hint
74-84: Consider enhancing error handling in useActiveBuildSwap.The current error handling only logs to console. Consider throwing an error or providing a fallback behavior when teammate data is not found.
if (!teammateDatum) { - console.error( - `Teammate data not found for character ${characterKey}` - ) - return + throw new Error( + `Teammate data not found for character ${characterKey}` + ) }
155-174: Consider making EquipRow more configurable.The component looks good, but consider making the gap spacing configurable via props for better reusability across different contexts.
export function EquipRow({ relicIds, lightConeId, + gap = 1, }: { relicIds: RelicIds lightConeId?: string + gap?: number }) { return ( - <Box sx={{ display: 'flex', gap: 1, flexWrap: 'wrap' }}> + <Box sx={{ display: 'flex', gap, flexWrap: 'wrap' }}>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
libs/sr/page-team/src/BuildsDisplay.tsx(4 hunks)
🔇 Additional comments (2)
libs/sr/page-team/src/BuildsDisplay.tsx (2)
4-4: LGTM! Import changes are well-organized.
The imports have been cleaned up and properly aligned with the new compact card components being used.
Also applies to: 11-12, 15-15
107-107: LGTM! Build components are well structured.
The refactoring to use EquipRow maintains clean component composition while improving UI consistency.
Also applies to: 122-122
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (4)
libs/sr/consts/src/relic.ts (2)
155-159: LGTM! Consider adding JSDoc documentation.The type guard implementation is correct and follows TypeScript best practices. To improve maintainability, consider adding JSDoc documentation to describe the function's purpose and return value.
+/** + * Type guard to check if a RelicSetKey is specifically a cavern relic set + * @param key The relic set key to check + * @returns True if the key is a cavern relic set key + */ export function isCavernRelicSetKey( key: RelicSetKey ): key is RelicCavernSetKey { return allRelicCavernSetKeys.includes(key as RelicCavernSetKey) }
161-165: Consider refactoring to reduce code duplication.While the implementation is correct, there's a pattern duplication between this and the previous function. Consider creating a higher-order function to generate these type guards.
+/** + * Creates a type guard function for relic set keys + */ +function createRelicSetKeyTypeGuard<T extends RelicSetKey>( + validKeys: readonly T[] +) { + return (key: RelicSetKey): key is T => validKeys.includes(key as T); +} + +export const isCavernRelicSetKey = createRelicSetKeyTypeGuard(allRelicCavernSetKeys); +export const isPlanarRelicSetKey = createRelicSetKeyTypeGuard(allRelicPlanarSetKeys);libs/sr/util/src/relic.ts (2)
175-180: Add documentation and clarify the fallback commentThe function logic is sound, but could benefit from:
- JSDoc documentation explaining the purpose and return values
- Clarification of the "noopt" comment
Consider adding documentation and improving the comment:
+/** + * Returns the default relic slot for a given set key + * @param setKey - The relic set key to get the default slot for + * @returns The default slot key based on the relic type (cavern or planar) + */ export function getDefaultRelicSlot(setKey: RelicSetKey) { if (isCavernRelic(setKey)) return allRelicCavernSlotKeys[0] if (isPlanarRelicSetKey(setKey)) return allRelicPlanarSlotKeys[0] - // noopt + // fallback to cavern slot if type cannot be determined return allRelicCavernSlotKeys[0] }
182-184: Consider performance optimization and add documentationThe function works correctly but could be optimized for larger datasets.
Consider these improvements:
+/** + * Checks if a given relic set key is a cavern relic + * @param setKey - The relic set key to check + * @returns True if the set key is a cavern relic, false otherwise + */ export function isCavernRelic(setKey: RelicSetKey) { - return allRelicCavernSetKeys.includes(setKey as RelicCavernSetKey) + // Use Set for O(1) lookup instead of Array.includes() O(n) + return new Set(allRelicCavernSetKeys).has(setKey as RelicCavernSetKey) }Note: This optimization is only beneficial if
allRelicCavernSetKeysis large or if this function is called frequently. If the array is small or rarely accessed, the current implementation is fine.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
libs/sr/consts/src/relic.ts(1 hunks)libs/sr/util/src/relic.ts(2 hunks)
🔇 Additional comments (2)
libs/sr/consts/src/relic.ts (1)
155-165: Verify usage patterns of the new type guards.
Let's check how these new type guards are being used in the codebase and if there are similar patterns that could benefit from this approach.
✅ Verification successful
Based on the results, I can now generate the final response:
Type guards are being used appropriately and follow established patterns
The verification shows:
- The new type guards are already being used in
libs/sr/util/src/relic.ts - They follow the same pattern as other type guards in the codebase (e.g.,
isTrailblazerKey,isCharacterKey) - The implementation using
includeswith type assertion is consistent with numerous similar checks throughout the codebase
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check usage patterns of the new type guard functions and similar type checking patterns
# Test 1: Find direct usage of the new type guards
echo "=== Direct usage of new type guards ==="
rg "isCavernRelicSetKey|isPlanarRelicSetKey"
# Test 2: Look for similar type checking patterns that might benefit from these guards
echo -e "\n=== Similar type checking patterns ==="
rg "includes\(.*as.*\)" --type ts
# Test 3: Look for potential conditional checks on relic types
echo -e "\n=== Potential relic type conditions ==="
rg "RelicSetKey.*=== '|RelicSetKey.*instanceof" --type ts
Length of output: 8684
libs/sr/util/src/relic.ts (1)
8-12: LGTM: Import changes are well-organized
The new type imports and constants are properly organized and align with the new functionality being added.
Also applies to: 14-21
Describe your changes
Issue or discord link
Testing/validation
Checklist before requesting a review (leave this PR as draft if any part of this list is not done.)
yarn run mini-cilocally to validate format and lint.Summary by CodeRabbit
Release Notes
New Features
AbilityDropdowncomponent to allow for additional customization.CharacterCardcomponent with a newHeaderfor better organization.CharacterEditorfor a more visually informative interface.CharacterNamecomponent for translated character names.CharacterTransandRelicTrans.LocationNamecomponent for displaying location information.LightConeCardCompactandRelicCardCompactcomponents for compact representations of their respective cards.BuildDisplayandTeammateDisplaycomponents for improved build presentation.Bug Fixes
Documentation