Update combo editor, creating sections of quick scrolling tabs #2553
Conversation
WalkthroughThe changes primarily involve enhancements to the Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
[sr-frontend] [Wed Nov 27 02:59:32 UTC 2024] - Deployed 005ac20 to https://genshin-optimizer-prs.github.io/pr/2553/sr-frontend (Takes 3-5 minutes after this completes to be available) [frontend] [Wed Nov 27 09:28:24 UTC 2024] - Deployed 2b580d9 to https://genshin-optimizer-prs.github.io/pr/2553/frontend (Takes 3-5 minutes after this completes to be available) [sr-frontend] [Wed Nov 27 09:28:47 UTC 2024] - Deployed 2b580d9 to https://genshin-optimizer-prs.github.io/pr/2553/sr-frontend (Takes 3-5 minutes after this completes to be available) [sr-frontend] [Wed Nov 27 09:39:13 UTC 2024] - Deployed ab8ab9b to https://genshin-optimizer-prs.github.io/pr/2553/sr-frontend (Takes 3-5 minutes after this completes to be available) [frontend] [Wed Nov 27 09:39:14 UTC 2024] - Deployed ab8ab9b to https://genshin-optimizer-prs.github.io/pr/2553/frontend (Takes 3-5 minutes after this completes to be available) [sr-frontend] [Wed Nov 27 18:36:16 UTC 2024] - Deployed 0ef1ea0 to https://genshin-optimizer-prs.github.io/pr/2553/sr-frontend (Takes 3-5 minutes after this completes to be available) [frontend] [Wed Nov 27 18:36:52 UTC 2024] - Deployed 0ef1ea0 to https://genshin-optimizer-prs.github.io/pr/2553/frontend (Takes 3-5 minutes after this completes to be available) [sr-frontend] [Wed Nov 27 18:38:32 UTC 2024] - Deployed d2c283d to https://genshin-optimizer-prs.github.io/pr/2553/sr-frontend (Takes 3-5 minutes after this completes to be available) [frontend] [Wed Nov 27 18:38:37 UTC 2024] - Deployed d2c283d to https://genshin-optimizer-prs.github.io/pr/2553/frontend (Takes 3-5 minutes after this completes to be available) [sr-frontend] [Wed Nov 27 18:54:33 UTC 2024] - Deployed 94530e6 to https://genshin-optimizer-prs.github.io/pr/2553/sr-frontend (Takes 3-5 minutes after this completes to be available) [frontend] [Wed Nov 27 18:54:37 UTC 2024] - Deployed 94530e6 to https://genshin-optimizer-prs.github.io/pr/2553/frontend (Takes 3-5 minutes after this completes to be available) [sr-frontend] [Wed Nov 27 19:05:46 UTC 2024] - Deployed 867baef to https://genshin-optimizer-prs.github.io/pr/2553/sr-frontend (Takes 3-5 minutes after this completes to be available) [frontend] [Wed Nov 27 19:06:11 UTC 2024] - Deployed 867baef to https://genshin-optimizer-prs.github.io/pr/2553/frontend (Takes 3-5 minutes after this completes to be available) [sr-frontend] [Wed Nov 27 19:40:18 UTC 2024] - Deployed 9052c88 to https://genshin-optimizer-prs.github.io/pr/2553/sr-frontend (Takes 3-5 minutes after this completes to be available) [frontend] [Wed Nov 27 19:41:18 UTC 2024] - Deployed 9052c88 to https://genshin-optimizer-prs.github.io/pr/2553/frontend (Takes 3-5 minutes after this completes to be available) [frontend] [Sat Nov 30 02:54:59 UTC 2024] - Deployed 79144f3 to https://genshin-optimizer-prs.github.io/pr/2553/frontend (Takes 3-5 minutes after this completes to be available) [sr-frontend] [Sat Nov 30 02:56:16 UTC 2024] - Deployed 79144f3 to https://genshin-optimizer-prs.github.io/pr/2553/sr-frontend (Takes 3-5 minutes after this completes to be available) [Sun Dec 1 20:56:13 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: 10
🧹 Outside diff range and nitpick comments (20)
libs/sr/page-team/src/context/TeamContext.ts (1)
Line range hint
1-16: Add documentation and improve type safetyThe context's purpose and usage patterns should be documented, and type safety could be improved.
Consider these improvements:
+/** + * Context for managing team-level state in the Star Rail team builder. + * Provides access to the current team ID and team data. + * + * @example + * function TeamComponent() { + * const { team, teamId } = useTeamContext(); + * return <div>{team.name}</div>; + * } + */ export type TeamContextObj = { teamId: string team: Team } +// Ensure type safety by creating a separate type for the context +type TeamContextType = TeamContextObj | null + -export const TeamContext = createContext({ +export const TeamContext = createContext<TeamContextType>(null) - teamId: '', - team: {}, -} as TeamContextObj) export function useTeamContext() { - return useContext(TeamContext) + const context = useContext(TeamContext) + if (context === null) { + throw new Error('useTeamContext must be used within a TeamProvider') + } + return context }libs/sr/page-team/src/context/TeammateContext.ts (1)
1-11: Consider adding documentation and usage examples.Since this context is part of a larger refactor from useTeamContext and relates to the combo editor relocation:
- Add JSDoc comments explaining the context's purpose and relationship with the combo editor
- Consider creating a README.md in the context directory with usage examples
- Document how this fits into the new fixed view team menu UI architecture
libs/sr/page-team/src/RelicSheetsDisplay.tsx (1)
4-6: Address the hardcoded implementation noted in TODO comment.The TODO comment indicates this is a temporary implementation. Consider expanding the relic sets coverage as mentioned in the comment to improve the component's reusability.
Would you like me to help create a GitHub issue to track the expansion of relic sets coverage? I can also assist in implementing a more dynamic solution that supports all relic sets with sheets.
libs/sr/page-team/src/LightConeSheetsDisplay.tsx (2)
5-6: Address the hardcoded light cone keys limitationThe TODO comment indicates this is a temporary solution. Consider implementing a more dynamic approach to handle light cone sheets.
Would you like me to help create a more flexible implementation that:
- Dynamically loads available light cone sheets
- Implements proper filtering/selection mechanisms
- Adds proper type safety for the expanded functionality
9-15: Consider enhancing grid responsivenessWhile the grid implementation is clean, it could benefit from better responsive behavior.
Consider this enhancement:
- <Grid container columns={3} spacing={1}> - {cones.map((lcKey) => ( - <Grid item xs={1} key={lcKey}> + <Grid container spacing={1}> + {cones.map((lcKey) => ( + <Grid item xs={12} sm={6} md={4} key={lcKey}>This would provide better adaptation to different screen sizes:
- Full width on mobile (xs)
- Two columns on tablets (sm)
- Three columns on desktop (md)
libs/sr/page-team/src/Optimize/GeneratedBuildsDisplay.tsx (1)
Line range hint
89-91: Implement or remove the TC build feature.The UI for TC build creation is present (checkbox and condition check) but the functionality is not implemented. This could lead to user confusion.
Consider either:
- Implementing the TC build creation functionality, or
- Removing the TC build UI elements until the feature is ready
- const [toTCBuild, setToTCBuild] = useState(false) const toNewBuild: FormEventHandler<HTMLFormElement> = (e) => { e.preventDefault() - if (toTCBuild) { - // TODO: to TC build - } else - database.builds.new({ + database.builds.new({ characterKey, name, teamId, relicIds, lightConeId, }) setName('') OnHidePrompt() } // Remove the checkbox UI - <Box display="flex" alignItems="center" sx={{ ml: -1 }}> - <Checkbox - size="small" - checked={toTCBuild} - onChange={() => setToTCBuild(!toTCBuild)} - /> - <Typography>Create a new TC Build</Typography> - </Box>Also applies to: 132-136
libs/sr/page-team/src/Optimize/index.tsx (1)
83-84: LGTM: Clean context usage.The context usage is well-structured and follows the single responsibility principle. Consider memoizing the context values if they're used in dependency arrays of hooks or memoized components.
- const { team } = useContext(TeamContext) - const { characterKey } = useTeammateContext() + const { team } = useMemo(() => useContext(TeamContext), []) + const { characterKey } = useMemo(() => useTeammateContext(), [])libs/sr/page-team/src/index.tsx (1)
187-232: Consider extracting nested providers into a separate component.While the context providers are correctly implemented, the deep nesting makes the code harder to read and maintain.
Consider extracting the providers into a separate component:
+const TeamProviders = ({ children, ...props }) => ( + <TeamContext.Provider value={props.teamContextObj}> + <TagContext.Provider value={props.tag}> + <PresetContext.Provider value={props.presetObj}> + <TeamCalcProvider teamId={props.teamId}> + <SrcDstDisplayContext.Provider value={props.srcDstDisplayContextValue}> + <ConditionalValuesContext.Provider value={props.conditionals}> + <SetConditionalContext.Provider value={props.setConditional}> + {children} + </SetConditionalContext.Provider> + </ConditionalValuesContext.Provider> + </SrcDstDisplayContext.Provider> + </TeamCalcProvider> + </PresetContext.Provider> + </TagContext.Provider> + </TeamContext.Provider> +); -return ( - <TeamContext.Provider value={teamContextObj}> - {/* ... nested providers ... */} - </TeamContext.Provider> +return ( + <TeamProviders + teamContextObj={teamContextObj} + tag={tag} + presetObj={presetObj} + teamId={teamId} + srcDstDisplayContextValue={srcDstDisplayContextValue} + conditionals={conditionals} + setConditional={setConditional} + > + <Box sx={{ display: 'flex', gap: 1, flexDirection: 'column', mx: 1, mt: 2 }}> + {/* ... rest of the JSX ... */} + </Box> + </TeamProviders> +);libs/sr/page-team/src/TalentContent.tsx (2)
242-242: Consider cleaning up or documenting buildTc commentsThere are multiple instances of commented
buildTc-related code. This pattern appears throughout the component and should be either:
- Removed if no longer needed
- Documented with a TODO if planned for future implementation
- Implemented if required for the current changes
Also applies to: 271-279
Line range hint
1-424: Consider documenting the context migration strategyThe codebase is undergoing a significant context management refactor from
useTeamContexttouseTeammateContext. To maintain consistency and aid future maintenance:
Consider adding documentation about:
- The reason for this context migration
- The differences between team and teammate contexts
- Migration guidelines for other components
Create a tracking issue for:
- Cleaning up commented
buildTccode- Implementing or removing TODO items
- Completing any pending features
libs/sr/page-team/src/BuildsDisplay.tsx (5)
Line range hint
87-95: Consider adding error handling for database operationsWhile the database interactions are implemented correctly, consider adding error handling for database operations to improve reliability:
const buildIds = useMemo( () => dbDirty && database.builds.getBuildIds(characterKey), + try { return dbDirty && database.builds.getBuildIds(characterKey) + } catch (error) { + console.error('Failed to fetch build IDs:', error) + return [] + } [dbDirty, database, characterKey] )
Line range hint
165-171: Enhance error handling in teammate data validationThe current error handling only logs the error but continues execution, which could lead to undefined behavior.
if (!teammateDatum) { - console.error( - `Teammate data not found for character ${characterKey}` - ) - return + throw new Error( + `Teammate data not found for character ${characterKey}` + ) }
Line range hint
234-234: TODO: Implement edit functionalityThe
onEditcallback is currently empty with a TODO comment.Would you like me to help implement the edit functionality or create a GitHub issue to track this task? The implementation could include:
- A modal dialog for editing build details
- Form fields for name and description
- Save/cancel actions
Line range hint
584-584: TODO: Implement substat type and rarity handlingThe comment indicates missing functionality for handling substat types and rarity.
Would you like me to help implement this feature or create a GitHub issue? The implementation could include:
- Dropdown for selecting substat types
- Rarity selector with appropriate validation
- Integration with the existing stat management system
Line range hint
442-446: Consider creating a shared modal componentMultiple modal implementations share similar patterns. Consider creating a reusable modal component to reduce code duplication and improve maintainability.
Create a shared
TCEditorModalcomponent that accepts:
- Title
- Content component
- State management props
This would reduce duplication acrossTCLightconeEditor,TCMainsEditor,TCSubsEditor, andTCRelicSetEditor.Also applies to: 513-519, 557-563, 632-638
libs/sr/page-team/src/ComboEditor.tsx (3)
47-47: Avoid using array index as key in list renderingUsing the array index
ias thekeyin list rendering can lead to issues if theteam.framesarray is modified (e.g., items are added, removed, or reordered). This can cause React to misidentify components, leading to unexpected behavior. Consider using a unique identifier for each frame if available.
175-175: Localize the modal title for internationalizationThe modal title
Edit Combo ${index + 1}is currently hardcoded and lacks localization support. Integrate a translation function to allow this string to be translated, enhancing the application's accessibility to users of different languages.Would you like assistance in implementing localization for this string?
98-102: Optimize dependency array inuseMemohookThe
useMemohook depends onframe.tag, which may be an object. Ifframe.tagreferences a new object on every render, it can cause unnecessary recomputations. To optimize performance:
- Ensure
frame.tagis a stable reference.- Consider using a serialization method like
JSON.stringify(frame.tag)in the dependency array if appropriate.- Alternatively, memoize
frame.tagto maintain reference equality across renders.libs/sr/db/src/Database/DataManagers/TeamDataManager.ts (1)
Line range hint
199-200: Complete the TODO: ValidateframesandbonusStatsThere are TODO comments indicating that validation for
framesandbonusStatsis incomplete. Implementing these validations is essential to maintain data integrity and prevent potential issues arising from invalid data.Would you like assistance in implementing the validation logic for
framesandbonusStats, or should I open a new GitHub issue to track this task?libs/sr/page-team/src/TeamCharacterSelector.tsx (1)
58-58: Translate the placeholder label for unnamed charactersThe label
Character ${ind + 1}is currently hardcoded and not localized. To support internationalization, consider using the translation functiont()with a translation key and dynamic parameter.Apply this diff to implement the translation:
- `Character ${ind + 1}` // TODO: Translation + t('characterWithIndex', { index: ind + 1 })Ensure to add the corresponding translation in your translation files:
In your translation JSON file (e.g.,
en.json):{ "characterWithIndex": "Character {{index}}" }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (17)
libs/sr/db/src/Database/DataManagers/TeamDataManager.ts(4 hunks)libs/sr/page-team/src/BonusStats.tsx(2 hunks)libs/sr/page-team/src/BuildsDisplay.tsx(2 hunks)libs/sr/page-team/src/ComboEditor.tsx(2 hunks)libs/sr/page-team/src/LightConeSheetsDisplay.tsx(1 hunks)libs/sr/page-team/src/Optimize/GeneratedBuildsDisplay.tsx(2 hunks)libs/sr/page-team/src/Optimize/index.tsx(2 hunks)libs/sr/page-team/src/RelicSheetsDisplay.tsx(1 hunks)libs/sr/page-team/src/TalentContent.tsx(4 hunks)libs/sr/page-team/src/TeamCharacterSelector.tsx(3 hunks)libs/sr/page-team/src/TeamNameCardHeader.tsx(1 hunks)libs/sr/page-team/src/TeammateDisplay.tsx(4 hunks)libs/sr/page-team/src/context/TeamContext.ts(1 hunks)libs/sr/page-team/src/context/TeammateContext.ts(1 hunks)libs/sr/page-team/src/context/index.ts(1 hunks)libs/sr/page-team/src/index.tsx(4 hunks)libs/sr/solver/src/solver.ts(2 hunks)
🔇 Additional comments (28)
libs/sr/page-team/src/context/index.ts (1)
3-3: LGTM! Verify module existence and usage.
The addition of TeammateContext export aligns with the architectural changes to separate teammate-specific data handling from team data.
Let's verify the module existence and its usage:
✅ Verification successful
Export and usage of TeammateContext verified successfully
The verification confirms:
TeammateContext.tsexists at the correct locationuseTeammateContextis properly imported and used across multiple components:- Used in main components like
TeammateDisplay,TalentContent,BonusStats - Used in optimization-related components (
Optimize/index.tsx,GeneratedBuildsDisplay) - Used in build management (
BuildsDisplay)
- Used in main components like
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify TeammateContext module exists and is properly used
# Check if TeammateContext.ts/tsx exists
echo "Checking for TeammateContext module..."
fd "TeammateContext\.(ts|tsx)$" libs/sr/page-team/src/context
# Check for usage of useTeammateContext
echo "Checking usage of useTeammateContext..."
rg "useTeammateContext" -A 2 libs/sr/page-team/src
Length of output: 6149
libs/sr/page-team/src/context/TeamContext.ts (1)
1-1: Verify the Team type import usage
Let's verify how the Team type is used across the codebase to ensure the removal of TeammateDatum hasn't created any inconsistencies.
libs/sr/page-team/src/context/TeammateContext.ts (1)
1-2: LGTM! Clean and focused imports.
The imports are well-organized and include only the necessary dependencies.
libs/sr/page-team/src/RelicSheetsDisplay.tsx (1)
9-15: Grid layout implementation looks good.
The Grid implementation with responsive columns and consistent spacing provides a clean layout structure. The removal of CardThemed wrapper aligns well with the broader UI streamlining efforts mentioned in the summary.
Let's verify if this grid layout is consistent with other similar components:
✅ Verification successful
Grid layout implementation is consistent across components
The verification confirms that the grid layout implementation in RelicSheetsDisplay perfectly matches the pattern used in LightConeSheetsDisplay, both using identical Grid container configurations with 3 columns and spacing of 1. This consistency in layout structure across similar components validates the implementation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for similar grid implementations in related components
# Look for Grid components with similar column/spacing configurations
rg -A 5 '<Grid container columns=' libs/sr/page-team/src/
Length of output: 1005
libs/sr/page-team/src/LightConeSheetsDisplay.tsx (1)
1-17: LGTM for the component structure
The overall component structure is clean and follows React best practices. The removal of unnecessary card wrappers aligns well with the UI streamlining effort mentioned in the summary.
libs/sr/page-team/src/TeamNameCardHeader.tsx (3)
1-23: LGTM! Well-organized imports.
The imports are properly structured and follow best practices by importing specific components rather than entire libraries.
24-28: LGTM! Clean component setup with proper hooks usage.
The component follows React best practices with appropriate context and state management.
60-62: Verify color contrast for hover state.
Ensure that the info.light color provides sufficient contrast ratio for accessibility.
libs/sr/page-team/src/Optimize/GeneratedBuildsDisplay.tsx (2)
25-25: LGTM! Clean import addition.
The addition of useTeammateContext aligns with the architectural changes described in the PR objectives.
84-85: LGTM! Clean context management.
The transition to useTeammateContext improves the component's data access pattern by making it more focused and explicit.
libs/sr/page-team/src/BonusStats.tsx (2)
26-26: LGTM! Import follows proper organization.
The addition of useTeammateContext is properly grouped with related context hooks, maintaining good code organization.
51-51: Verify the context migration impact.
The switch to useTeammateContext aligns with the architectural improvements. However, let's verify that all related components have been updated consistently.
✅ Verification successful
Migration to useTeammateContext is consistent across the codebase
The verification shows that the context migration has been implemented consistently:
- All components in
libs/sr/page-team/src/are usinguseTeammateContextfor accessingcharacterKey - No instances of
characterKeybeing accessed throughuseTeamContextwere found - The pattern is consistently applied across multiple components including BuildsDisplay, TalentContent, TeammateDisplay, and Optimize components
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify consistent context usage across components
# Check for any remaining useTeamContext usage for characterKey
rg -A 3 "useTeamContext.*characterKey" "libs/sr/page-team/src/"
# Check adoption of useTeammateContext
rg "useTeammateContext" "libs/sr/page-team/src/"
# Look for potential missed migrations
ast-grep --pattern 'const { characterKey } = useTeamContext()' "libs/sr/page-team/src/"
Length of output: 2295
libs/sr/page-team/src/TeammateDisplay.tsx (4)
32-32: LGTM: Context management refactoring
The addition of useTeammateContext import aligns with the broader context management improvements mentioned in the PR objectives.
69-69: LGTM: Consistent context usage
The change to useTeammateContext maintains component functionality while aligning with the new context management pattern.
106-106: LGTM: Consistent context refactoring
The changes in both BuildDisplay and BuildTCDisplay components maintain consistent context usage patterns while preserving functionality.
Also applies to: 115-115
Line range hint 1-190: Verify multiplier functionality implementation
While the context management changes are well-implemented, the PR objectives mention adding a multiplier alongside the combo editor functionality. This change is not visible in the current file.
libs/sr/page-team/src/Optimize/index.tsx (2)
33-33: LGTM: Import statement is correctly structured.
The addition of useTeammateContext import aligns with the broader context management improvements.
41-42: LGTM: Improved context separation.
The separation of team and teammate context access improves code organization and maintainability.
Let's verify this pattern is consistently applied across the codebase:
✅ Verification successful
Pattern consistently applied across the codebase
The separation of team and teammate context is consistently implemented throughout the codebase. All files are using the dedicated useTeammateContext hook, and there are no instances of directly extracting teammateDatum from TeamContext. The change in the Optimize component aligns with the established pattern across the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistent usage of useTeammateContext
# Expected: All files should use useTeammateContext instead of destructuring teammateDatum from TeamContext
# Search for potential inconsistencies in context usage
rg -l "const.*teammateDatum.*=.*useContext.*TeamContext"
# Search for correct usage pattern
rg -l "useTeammateContext"
Length of output: 424
Script:
#!/bin/bash
# Let's check the actual implementation of useTeammateContext and look for any files still using the old pattern
# First, let's see the TeammateContext implementation
cat libs/sr/page-team/src/context/TeammateContext.ts
# Then, let's check the actual usage in files to ensure they're using the new pattern
rg "teammateDatum.*=.*useContext.*TeamContext" -A 2
Length of output: 407
Script:
#!/bin/bash
# Let's check the actual usage patterns in the files that use useTeammateContext
for file in $(rg -l "useTeammateContext"); do
echo "=== $file ==="
rg -A 2 "useTeammateContext|TeamContext" "$file"
done
Length of output: 5384
libs/sr/page-team/src/index.tsx (4)
30-30: LGTM: Import changes align with PR objectives.
The new imports support the relocation of the combo editor to the fixed view team menu UI, as specified in the PR objectives.
Also applies to: 41-44, 48-48
126-132: LGTM: Good separation of concerns in context management.
The simplification of teamContextObj and removal of teammateDatum is a good architectural decision, as it properly separates team-level and teammate-level concerns into distinct contexts.
237-237: LGTM: Proper usage of TeammateContext.
The switch to useTeammateContext hook is consistent with the new context structure and improves the component's maintainability.
203-219: Verify sticky card behavior with overflow: 'visible'.
While the sticky positioning implementation looks good, the overflow: 'visible' property might cause UI issues if child components extend beyond the card boundaries.
Let's check for potential overflow issues in the codebase:
✅ Verification successful
Based on the search results, I can now generate the final response:
The sticky card implementation with overflow: 'visible' is safe to use
The code inspection shows that:
- ComboEditor has
overflowX: 'auto'for its content, preventing horizontal overflow issues - TeamNameCardHeader uses standard CardContent without any complex popover/dropdown components
- The OptimizationTargetSelector components are rendered within the scrollable content area
- No Menu/Popover components that could cause problematic overflow were found in the sticky card's direct children
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for potential overflow issues with ComboEditor and TeamNameCardHeader
# Search for height/width definitions in ComboEditor
echo "Checking ComboEditor dimensions:"
ast-grep --pattern 'sx={{ $$$height$$$: $_, $$$width$$$: $_ }}'
# Search for potential popover/dropdown components that might overflow
echo "Checking for potential overflow components:"
rg -l "Menu|Popover|Select|Autocomplete" --type tsx
Length of output: 319
Script:
#!/bin/bash
# Let's try a different approach to check for potential overflow issues
# Search for ComboEditor component implementation
echo "Checking ComboEditor implementation:"
rg -A 5 "export.*ComboEditor" --type ts --type tsx
# Search for any style-related props or components that might cause overflow
echo "Checking for style-related content in ComboEditor:"
rg -A 5 "sx=|style=|Menu|Popover|Select|Autocomplete" --type ts --type tsx
# Check TeamNameCardHeader implementation
echo "Checking TeamNameCardHeader implementation:"
rg -A 5 "export.*TeamNameCardHeader" --type ts --type tsx
Length of output: 564
Script:
#!/bin/bash
# Let's try without specifying file types since ripgrep isn't recognizing tsx
# Search for ComboEditor component implementation
echo "Checking ComboEditor implementation:"
rg -A 5 "export.*ComboEditor"
# Search for any style-related props or components that might cause overflow
echo "Checking for style-related content in ComboEditor:"
rg -A 5 "sx=|style=|Menu|Popover|Select|Autocomplete"
# Check TeamNameCardHeader implementation
echo "Checking TeamNameCardHeader implementation:"
rg -A 5 "export.*TeamNameCardHeader"
# Additional check for any files containing these components
echo "Checking files containing these components:"
fd -e tsx -e ts ComboEditor TeamNameCardHeader
Length of output: 101038
Script:
#!/bin/bash
# Let's check the implementation of these components and their potential overflow behavior
# Search for ComboEditor implementation
echo "Checking ComboEditor implementation:"
rg -A 10 "ComboEditor" libs/sr/page-team/src/ComboEditor.tsx
# Search for TeamNameCardHeader implementation
echo "Checking TeamNameCardHeader implementation:"
rg -A 10 "TeamNameCardHeader" libs/sr/page-team/src/
# Check for any Menu/Popover/Select components in these files
echo "Checking for potential overflow components in the sticky card:"
rg -A 5 "Menu|Popover|Select|Autocomplete" libs/sr/page-team/src/ComboEditor.tsx libs/sr/page-team/src/TeamNameCardHeader.tsx
Length of output: 6115
libs/sr/page-team/src/TalentContent.tsx (3)
36-36: LGTM: Context import aligns with refactor
The addition of useTeammateContext import aligns with the PR's objective of improving teammate data management.
45-45: LGTM: Context usage is consistent
The switch to useTeammateContext for accessing characterKey aligns with the broader context management improvements.
332-332: LGTM: Context usage is consistent
The switch to useTeammateContext maintains consistency with other components. However, note that this component follows the same pattern of commented buildTc code seen elsewhere.
libs/sr/page-team/src/BuildsDisplay.tsx (1)
81-84: LGTM: Improved context separation
The refactoring to use useTeammateContext for teammate-specific data improves the separation of concerns and makes the code more maintainable.
Also applies to: 146-147
libs/sr/solver/src/solver.ts (2)
1-1: Importing prod function for calculation updates
The addition of prod to the imports is appropriate for the updated logic in the detachNodes method.
137-140: Correct application of frame multipliers in detachNodes
The use of prod to multiply frame.multiplier with the frame's read value ensures that each frame's contribution is accurately scaled according to its multiplier.
frzyc
changed the title
frzyc
changed the title
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (15)
libs/sr/page-team/src/TeamHeader.tsx (3)
11-19: Consider documenting headerRef usage purpose.The
headerRefprop usingMutableRefObjectsuggests DOM manipulation. Adding JSDoc comments would help explain its intended use and any side effects.+/** + * @param props.headerRef - Used for [explain purpose, e.g., "measuring header height", "scrolling behavior"] + */ export function TeamHeader({
23-28: Document the zIndex choice.The zIndex of 101 seems arbitrary. Consider adding a comment explaining why this specific value was chosen and its relationship to other UI layers.
sx={{ overflow: 'visible', top: HEADER_TOP_PX, position: 'sticky', + // zIndex: 101 ensures header stays above [explain other UI elements] zIndex: 101, }}
20-41: Consider component organization for detailed information display.The PR objectives mention displaying various pieces of information (character sheet, source sheet, translated names, damage types, etc.). Consider:
- Creating separate components for different information categories
- Using tabs or expandable sections for better organization
- Implementing a modal view for detailed editing as mentioned in the requirements
This would help maintain clean separation of concerns and improve maintainability.
libs/sr/page-team/src/TeamCharacterSelector.tsx (2)
75-75: Add translation for "Character X" stringThe string
Character ${ind + 1}should be translated using the i18n system.Would you like me to help set up the translation key for this string?
78-81: Consider memoizing the onClick handlerThe onClick handler is recreated on every render. Consider using useCallback to memoize it:
- onClick={() => - // conserve the current tab when switching to another character - characterKey && navigate(`/teams/${teamId}/${characterKey}`) - } + onClick={useCallback(() => + characterKey && navigate(`/teams/${teamId}/${characterKey}`), + [characterKey, navigate, teamId] + )}libs/sr/page-team/src/ComboEditor.tsx (4)
18-26: Consider accessibility improvements for horizontal scrollingThe horizontal scrollable layout might pose accessibility challenges. Consider:
- Adding keyboard navigation support
- Ensuring the scrollable area is obvious to users
- Adding visual indicators for overflow content
<Box sx={{ display: 'flex', gap: 1, overflowX: 'auto', overflowY: 'visible', p: 1, + '&::-webkit-scrollbar': { + height: '8px', + }, + '&::-webkit-scrollbar-track': { + background: '#f1f1f1', + }, + '&::-webkit-scrollbar-thumb': { + background: '#888', + }, }} + role="list" + aria-label="Combo frames" >
27-29: Add empty state handlingConsider adding a placeholder or message when there are no frames to display.
- {team.frames.map((frame, i) => ( - <Combo key={i} frame={frame} index={i} /> - ))} + {team.frames.length > 0 ? ( + team.frames.map((frame, i) => ( + <Combo key={i} frame={frame} index={i} /> + )) + ) : ( + <Box sx={{ p: 2, textAlign: 'center' }}> + <Typography color="text.secondary"> + No frames added. Use the selector to add your first frame. + </Typography> + </Box> + )}
84-86: Enhance multiplier value displayThe multiplier and computed value display could be improved for better readability:
- Format large numbers with appropriate separators
- Consider adding tooltips for precise values
<Typography> - {frame.multiplier} x {valueString(value, unit)} + <span title={`Multiplier: ${frame.multiplier}`}> + {new Intl.NumberFormat().format(frame.multiplier)} + </span> + {' × '} + <span title={`Value: ${value}${unit}`}> + {valueString(value, unit)} + </span> </Typography>
Line range hint
48-89: Consider memoizing the Combo componentThe Combo component could benefit from memoization to prevent unnecessary re-renders when other frames change.
-function Combo({ frame, index }: { frame: Frame; index: number }) +const Combo = React.memo(function Combo( + { frame, index }: { frame: Frame; index: number } +) { // ... component implementation ... -} +})libs/gi/page-team/src/CharacterDisplay/Tabs/TabOverview/index.tsx (1)
20-20: Consider adding explicit TypeScript types.For better type safety and code documentation, consider adding explicit types:
- const [scrollRef, onScroll] = useScrollRef() + const [scrollRef, onScroll] = useScrollRef<HTMLDivElement>()libs/sr/page-team/src/TeamNameCardHeader.tsx (2)
24-28: Add TypeScript interface for team object structure.Consider adding type definitions for the team object to improve type safety and code documentation:
interface Team { name: string; description?: string; // Add other team properties }
96-110: Add form validation and loading state.Consider the following improvements:
- Add validation for required fields and maximum length
- Show loading state during database updates
<Box display="flex" flexDirection="column" gap={2} sx={{ mt: 2 }}> <TextFieldLazy label={t('team.name')} value={team.name} onChange={(teamName) => handleName(teamName)} + required + inputProps={{ maxLength: 50 }} + error={!team.name} + helperText={!team.name ? t('common:required') : ''} autoFocus /> <TextFieldLazy label={t('team.desc')} value={team.description} onChange={(teamDesc) => handleDesc(teamDesc)} + inputProps={{ maxLength: 200 }} multiline minRows={4} />libs/sr/page-team/src/BonusStats.tsx (2)
26-32: Consider memoizing database operations.The component uses multiple contexts and database operations. While the implementation is correct, consider memoizing the
newTargetcallback to prevent unnecessary re-renders.- const newTarget = (q: InitialStats) => { + const newTarget = useCallback((q: InitialStats) => { const tag = newTag(q, characterKey) database.teams.setBonusStat(teamId, tag, 0, presetIndex) - } + }, [characterKey, database.teams, teamId, presetIndex])
39-63: Enhance accessibility for interactive elements.The card section contains interactive elements that could benefit from improved accessibility:
Consider adding the following improvements:
- Add aria-label to the DropdownButton for adding new stats
- Add aria-label to the NumberInputLazy for bonus stat values
- Consider adding tooltips to explain the purpose of each input
<DropdownButton + aria-label="Select bonus stat type" title={(tag && tagFieldMap.subset(tag)[0]?.title) ?? 'Add Bonus Stat'} >libs/sr/page-team/src/TeammateDisplay.tsx (1)
Line range hint
280-334: Consider refactoring duplicated debug sectionsThe CalcDebug component has similar code structure repeated for formulas and buffs sections.
Consider extracting the common structure into a reusable component:
function DebugSection({ title, formulas }) { return ( <Accordion> <AccordionSummary expandIcon={<ExpandMoreIcon />}> {title} </AccordionSummary> <AccordionDetails> <Stack> {calc?.listFormulas(formulas).map((read, index) => { const computed = calc.compute(read) const name = read.tag.name || read.tag.q return ( <Box key={`${name}${index}`}> {/* ... rest of the implementation ... */} </Box> ) })} </Stack> </AccordionDetails> </Accordion> ) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (12)
libs/common/ui/src/hooks/index.ts(1 hunks)libs/common/ui/src/hooks/useScrollRef.tsx(1 hunks)libs/gi/page-team/src/CharacterDisplay/Tabs/TabOverview/index.tsx(2 hunks)libs/sr/formula-ui/src/lightCone/util.tsx(1 hunks)libs/sr/page-team/src/BonusStats.tsx(1 hunks)libs/sr/page-team/src/ComboEditor.tsx(2 hunks)libs/sr/page-team/src/LightConeSheetDisplay.tsx(1 hunks)libs/sr/page-team/src/TeamCharacterSelector.tsx(3 hunks)libs/sr/page-team/src/TeamHeader.tsx(1 hunks)libs/sr/page-team/src/TeamNameCardHeader.tsx(1 hunks)libs/sr/page-team/src/TeammateDisplay.tsx(5 hunks)libs/sr/page-team/src/index.tsx(4 hunks)
✅ Files skipped from review due to trivial changes (1)
- libs/sr/page-team/src/LightConeSheetDisplay.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- libs/sr/page-team/src/index.tsx
🧰 Additional context used
🪛 Biome (1.9.4)
libs/sr/page-team/src/TeammateDisplay.tsx
[error] 60-60: 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)
[error] 62-62: 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)
[error] 63-63: 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)
[error] 64-64: 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)
[error] 65-65: 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)
[error] 69-69: 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)
[error] 71-71: 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 (20)
libs/common/ui/src/hooks/index.ts (1)
7-7: LGTM! The new export follows established patterns.
The addition of useScrollRef follows the consistent export pattern and is appropriately placed among other UI-related hooks.
Let's verify there's no functionality overlap with existing scroll-related hooks:
✅ Verification successful
No functionality overlap found - each scroll hook serves a distinct purpose
The codebase search reveals three scroll-related hooks with different responsibilities:
useScrollRef: Provides a ref and scroll-into-view functionality for a specific elementuseInfScroll: Handles infinite scrolling with incremental loadinguseWindowScrollPos: Tracks window scroll position coordinates
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Compare implementations of scroll-related hooks
# Expected: Different scroll management approaches
# Find all scroll-related hook implementations
echo "Checking scroll-related hook implementations:"
rg -A 10 "export function use.*Scroll" libs/common/ui/src/hooks/
Length of output: 2596
libs/common/ui/src/hooks/useScrollRef.tsx (1)
1-1: LGTM!
The imports are correctly specified and all imported hooks are used in the implementation.
libs/sr/formula-ui/src/lightCone/util.tsx (1)
10-10:
Address the TODO/FIXME comment regarding characters without lightcones
The comment indicates a potential runtime error when handling characters without lightcones. This needs to be properly handled to prevent crashes.
Let's verify the impact and current handling of characters without lightcones:
✅ Verification successful
Let me gather more context about the specific file and its usage:
Let me check for the usage context and type definitions:
Add null check and provide default value for characters without lightcones
The SuperImposeWrapper component needs defensive coding to handle cases where a character has no lightcone equipped. Looking at the implementation and usage:
- The component is currently hardcoded to return
1as superimpose value - The TODO comment can be addressed by keeping this default behavior
- When the commented-out calculation logic is restored, it should use optional chaining and nullish coalescing:
const superimpose = calc?.compute(own?.lightCone?.superimpose)?.val ?? 1🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for lightcone-related error handling patterns
# and verify if there are other places that need similar fixes
# Check for similar error handling patterns
rg -A 5 "without.*lightcone|!lightCone|lightCone\s*==\s*null|lightCone\s*===\s*undefined"
# Look for lightcone-related type definitions
ast-grep --pattern 'type $_ = {
$$$
lightCone: $_
$$$
}'
Length of output: 6229
Script:
#!/bin/bash
# Check the specific file content and its imports
cat libs/sr/formula-ui/src/lightCone/util.tsx
# Look for usages of this util file
rg -l "from.*lightCone/util"
Length of output: 442
Script:
#!/bin/bash
# Look for the type definition of the character object that might use lightCone
ast-grep --pattern 'interface $_ {
$$$
lightCone$_
$$$
}'
# Also check for optional lightCone properties
ast-grep --pattern 'type $_ = {
$$$
lightCone?: $_
$$$
}'
# Look for any error handling patterns around this component
rg -A 5 "SuperImposeWrapper"
Length of output: 2559
libs/sr/page-team/src/TeamHeader.tsx (3)
1-9: LGTM! Clean imports and well-defined constants.
The imports are organized and the constants are appropriately named and used for styling purposes.
10-10: Verify TeamHeaderHeightContext provider implementation.
The context is created but there's no visible provider component. This could lead to components always using the default value.
✅ Verification successful
Context provider is properly implemented
The TeamHeaderHeightContext provider is correctly implemented in libs/sr/page-team/src/index.tsx. It provides a dynamic height value that falls back to the default height when no explicit height is given, and adds a top padding value.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for TeamHeaderHeightContext.Provider usage
rg "TeamHeaderHeightContext.Provider" -A 3
Length of output: 668
35-35: Verify ComboEditor props requirements.
The PR objectives mention adding a multiplier alongside the combo editor, but the ComboEditor component is rendered without any props. Please verify if it needs access to teamId or characterKey.
libs/sr/page-team/src/TeamCharacterSelector.tsx (4)
1-11: LGTM: Clean import declarations
The imports are well-organized and properly scoped to the component's needs. The removal of unused imports and addition of necessary ones improves code maintainability.
Line range hint 13-25: LGTM: Well-structured component with proper null checks
The component follows React best practices:
- Proper hook usage
- Early return pattern for null case
- Clear type definitions
Line range hint 28-49: LGTM: Well-implemented responsive design
The Tabs component is properly configured with responsive behavior and clear visual feedback through styling.
50-85: Verify integration with combo editor changes
While the character selector implementation looks good, let's ensure it properly integrates with the relocated combo editor mentioned in the PR objectives.
✅ Verification successful
Let's verify how the ComboEditor component is structured and how it's connected to the character selection flow.
Let's verify how the TeamHeader component orchestrates the character selection and how it's connected to the routing system that handles navigation between characters.
Let's examine the main routing setup and navigation handling to understand how character selection integrates with the overall page structure.
Based on the code examination, I can now generate the final response:
Character selector properly integrates with combo editor changes
The implementation is correct and well-integrated:
- Character selection is properly handled through the routing system (
/teams/:teamId/:characterKey) - The TeammateDisplay component (which contains the combo editor) is correctly wrapped in the necessary contexts and rendered based on the selected character
- Navigation between characters maintains the proper state through context providers
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for combo editor integration points
rg -l "ComboEditor|useTeammateContext" libs/sr/page-team/src/
Length of output: 477
Script:
#!/bin/bash
# Check the integration between TeamCharacterSelector and ComboEditor
ast-grep --pattern 'const ComboEditor = $_' libs/sr/page-team/src/ComboEditor.tsx
# Check how TeamCharacterSelector is used
rg -A 5 "TeamCharacterSelector" libs/sr/page-team/src/
Length of output: 1370
Script:
#!/bin/bash
# Check how the character selection flow integrates with the combo editor
rg -A 10 "export function TeamHeader" libs/sr/page-team/src/TeamHeader.tsx
# Check the routing setup to understand the navigation flow
rg -A 5 "Route.*path.*teams" libs/sr/page-team/src/
Length of output: 330
Script:
#!/bin/bash
# Check the index file for routing and main component structure
cat libs/sr/page-team/src/index.tsx
# Look for character-specific navigation handling
rg -A 10 "navigate.*teams.*character" libs/sr/page-team/src/
Length of output: 8302
libs/sr/page-team/src/ComboEditor.tsx (1)
32-42: Consider making the default multiplier configurable
The multiplier is hardcoded to 1 when adding new frames. Consider:
- Making this value configurable
- Adding validation to ensure it's a positive number
setOptTarget={(tag) =>
database.teams.set(teamId, (team) => {
+ const defaultMultiplier = 1; // Consider making this configurable
team.frames = [
...team.frames,
{
- multiplier: 1,
+ multiplier: defaultMultiplier,
tag,
},
]
})
}libs/gi/page-team/src/CharacterDisplay/Tabs/TabOverview/index.tsx (3)
1-1: LGTM! Good use of custom hook for scroll management.
The change to use useScrollRef from the common UI package improves code maintainability by encapsulating scroll logic in a reusable hook.
Also applies to: 13-13
Line range hint 1-100: Verify alignment with PR objectives.
While the code changes in this file improve scroll management, I don't see the mentioned combo editor relocation or multiplier functionality from the PR objectives. Please confirm if these changes are implemented in other files or if additional changes are needed here.
20-21: Verify scroll behavior across different viewports.
Since this component uses responsive Grid layouts and the scroll functionality is crucial for navigation, please ensure the scroll behavior works correctly across different screen sizes and device orientations.
✅ Verification successful
Let me gather more information about the scroll implementation and Grid layout.
Let me try one more search to find the Grid and responsive layout implementations with a different approach.
Based on the code analysis, I can now provide a final response:
The scroll behavior implementation is properly integrated with responsive layouts.
The component uses a well-structured responsive Grid system with defined breakpoints for different screen sizes:
- The main layout uses a responsive Grid container with proper spacing and column definitions
- The equipment row uses responsive columns configuration:
columns={{ xs: 2, sm: 2, md: 3, lg: 6, xl: 6 }} - The stats display component adapts columns responsively:
columns={{ xs: 1, sm: 1, md: 2, lg: 2, xl: 3 }} - The scroll functionality is implemented using
useScrollRefhook which uses smooth scrolling behavior that works across all modern browsers
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all responsive Grid configurations in the codebase to ensure consistent scroll behavior
# Search for Grid container configurations that might affect scroll behavior
rg --type tsx "Grid container.*spacing.*columns" -A 3
# Search for related scroll implementations
ast-grep --pattern 'useScrollRef()'
Length of output: 333
Script:
#!/bin/bash
# Search for Grid layout implementations in the component
rg -t tsx "Grid" libs/gi/page-team/src/CharacterDisplay/Tabs/TabOverview/
# Find the useScrollRef hook implementation
rg -t ts "useScrollRef" -A 5
# Look for responsive styles or media queries
rg -t tsx "@media|useMediaQuery|breakpoints" libs/gi/page-team/src/CharacterDisplay/Tabs/TabOverview/
Length of output: 2994
Script:
#!/bin/bash
# Search for the component's implementation to understand the layout structure
cat libs/gi/page-team/src/CharacterDisplay/Tabs/TabOverview/index.tsx
# Search for any theme or breakpoint configurations
rg "createTheme|ThemeProvider|breakpoints" -A 3
Length of output: 20274
libs/sr/page-team/src/TeamNameCardHeader.tsx (3)
30-36: Add error handling for database operations.
The database update operations should include error handling to manage potential failures and provide user feedback.
68-74: Consider using Paper component or elevation for better contrast
The text shadow implementation could be improved using MUI's Paper component with elevation.
89-91: Add aria-label to close button for accessibility.
The close button IconButton should have an aria-label for screen readers.
libs/sr/page-team/src/BonusStats.tsx (1)
23-23: LGTM! Verify component name updates across the codebase.
The component rename from BonusStats to BonusStatsSection better reflects its purpose. The new useTeammateContext import aligns with the PR's objective of improving team data management.
Also applies to: 25-25
✅ Verification successful
No issues found with the component rename
The search results show that the only reference to the old component name in the Star Rail (sr) codebase is actually importing the new renamed component BonusStatsSection. All other matches are from the Genshin Impact (gi) codebase, which are unrelated components with similar but different names (like BonusStatsCard, BonusStatsEditor) and are part of a different module. The rename from BonusStats to BonusStatsSection has been properly implemented.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that all references to BonusStats have been updated
# Test: Search for any remaining references to the old component name
rg -l 'BonusStats[^S]'
Length of output: 515
Script:
#!/bin/bash
# Let's check the actual usage context of 'BonusStats' in these files
rg 'BonusStats[^S]' -C 2
Length of output: 6127
libs/sr/page-team/src/TeammateDisplay.tsx (2)
87-124: LGTM! Well-implemented section layout
The Section component effectively implements:
- Sticky positioning for section headers
- Proper scroll behavior
- Responsive spacing based on section index
Line range hint 213-279: LGTM! Clean implementation of build display components
The components effectively:
- Use the new useTeammateContext hook
- Handle both regular and TC build types
- Properly manage database subscriptions
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (6)
libs/sr/page-team/src/ComboEditor.tsx (3)
18-26: Consider adding scroll indicatorsThe horizontal scrolling implementation might hide content from users. Consider adding visual indicators for overflow content.
<Box sx={{ display: 'flex', gap: 1, overflowX: 'auto', overflowY: 'visible', p: 1, + position: 'relative', + '&::after': { + content: '""', + position: 'absolute', + right: 0, + top: 0, + bottom: 0, + width: '24px', + background: 'linear-gradient(to right, transparent, rgba(0,0,0,0.1))', + pointerEvents: 'none', + opacity: 0.8, + } }} >
31-43: Consider making the default multiplier configurableThe hardcoded multiplier value of 1 might not be suitable for all cases. Consider making it configurable or derived from context.
<OptimizationTargetSelector setOptTarget={(tag) => database.teams.set(teamId, (team) => { team.frames = [ ...team.frames, { - multiplier: 1, + multiplier: team.defaultMultiplier ?? 1, tag, }, ] }) } />
90-92: Improve value formatting for readabilityThe current format might be hard to read for large numbers. Consider adding thousand separators and rounding for better readability.
<Typography> - {frame.multiplier} x {valueString(value, unit)} + {frame.multiplier.toLocaleString()} × {valueString(Number(value.toFixed(2)), unit)} </Typography>libs/sr/page-team/src/TeammateDisplay.tsx (3)
147-156: Add validation to multiplier inputConsider adding min/max bounds and validation to the multiplier input to prevent invalid values.
<NumberInputLazy value={frame.multiplier} onChange={(v) => setFrame({ multiplier: v })} size="small" + min={0} + max={100} InputProps={{ startAdornment: ( <InputAdornment position="start">Multi x </InputAdornment> ), }} />
Line range hint
176-205: Add error handling for undefined characterConsider adding error handling or a fallback UI when the character is undefined.
function CharacterSection() { const { characterKey } = useTeammateContext() const character = useCharacterContext() + if (!character) { + return <Typography color="error">No character selected</Typography> + } const [editorKey, setCharacterKey] = useState<CharacterKey | undefined>( undefined ) return ( // ... ) }🧰 Tools
🪛 Biome (1.9.4)
[error] 60-60: 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)
Line range hint
238-242: Address translation TODO commentThe "Change Build" button text needs translation support.
Would you like me to help implement the translation or create an issue to track this task?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
libs/sr/page-team/src/ComboEditor.tsx(2 hunks)libs/sr/page-team/src/TeammateDisplay.tsx(5 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
libs/sr/page-team/src/TeammateDisplay.tsx
[error] 60-60: 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 (5)
libs/sr/page-team/src/ComboEditor.tsx (2)
1-11: LGTM! Dependencies are well organized.
The imports are appropriate for the component's functionality and properly organized between external and internal dependencies.
48-48: Add input validation for multiplier
While the previous review already mentioned this, it's still important to validate the multiplier to prevent invalid values.
Also applies to: 91-91
libs/sr/page-team/src/TeammateDisplay.tsx (3)
57-73: Add key props to section content elements
The section content elements should have key props to help React efficiently update the list.
🧰 Tools
🪛 Biome (1.9.4)
[error] 60-60: 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)
87-124: LGTM! Well-implemented section layout
The Section component effectively implements sticky positioning with proper scroll behavior and layout calculations.
207-212: LGTM! Clean conditional rendering
The OptimizeSection component correctly handles the conditional rendering based on buildType.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (7)
libs/sr/page-team/src/ComboEditor.tsx (2)
78-94: Add ARIA label for better accessibilityThe CardActionArea should have an aria-label to improve accessibility for screen readers.
- <CardActionArea onClick={handleClick}> + <CardActionArea + onClick={handleClick} + aria-label={`Select combo ${index + 1}`} + >
52-61: Optimize value computation with useMemo dependenciesThe current useMemo dependency array includes the entire
calcobject, which might cause unnecessary recalculations. Consider extracting only the required calc methods.const value = useMemo(() => { try { return ( calc?.withTag(tagcontext).compute(new Read(frame.tag, 'sum')).val ?? 0 ) } catch (error) { console.error('Error computing value:', error) return 0 } - }, [calc, frame.tag, tagcontext]) + }, [calc?.withTag, frame.tag, tagcontext])libs/sr/page-team/src/TeammateDisplay.tsx (5)
51-52: Consider extracting magic numbers into named constants with documentationThe spacing values (
BOT_PXandSECTION_SPACING_PX) would benefit from documentation explaining their purpose and how they affect the layout.-const BOT_PX = 0 -const SECTION_SPACING_PX = 33 +/** Bottom spacing in pixels for sticky sections */ +const BOTTOM_SPACING_PX = 0 +/** Vertical spacing between sections in pixels for sticky header calculations */ +const SECTION_VERTICAL_SPACING_PX = 33
95-132: Consider memoizing style objects for better performanceThe style objects in the CardThemed component are recreated on every render. Consider memoizing them for better performance.
+const useSectionStyles = (headerHeight: number, index: number, numSections: number) => + useMemo( + () => ({ + outline: (theme: any) => `solid ${theme.palette.secondary.main}`, + position: 'sticky' as const, + top: headerHeight + index * SECTION_SPACING_PX, + bottom: BOT_PX + (numSections - 1 - index) * SECTION_SPACING_PX, + zIndex: 100, + }), + [headerHeight, index, numSections] + ) function Section({ index, title, children }) { const numSections = useContext(SectionNumContext) const headerHeight = useContext(TeamHeaderHeightContext) + const styles = useSectionStyles(headerHeight, index, numSections) return ( <> - <CardThemed sx={(theme) => ({ - outline: `solid ${theme.palette.secondary.main}`, - position: 'sticky', - top: headerHeight + index * SECTION_SPACING_PX, - bottom: BOT_PX + (numSections - 1 - index) * SECTION_SPACING_PX, - zIndex: 100, - })}> + <CardThemed sx={styles}>
215-220: Add documentation for build type conditionsThe component would benefit from documentation explaining why 'tc' build types don't support optimization.
+/** + * Renders the Optimize component for non-TC builds. + * TC (Theory Crafting) builds don't support optimization as they represent + * theoretical configurations rather than actual builds. + */ function OptimizeSection() { const { buildType } = useTeammateContext() if (buildType === 'tc') return null return <Optimize /> }
Line range hint
221-290: Add error boundaries for database operationsThe database operations could fail, but there's no error handling in place. Consider adding error boundaries or try-catch blocks.
+class BuildErrorBoundary extends React.Component { + state = { hasError: false, error: null } + + static getDerivedStateFromError(error) { + return { hasError: true, error } + } + + render() { + if (this.state.hasError) { + return ( + <Alert severity="error"> + Failed to load build data. Please try again. + </Alert> + ) + } + return this.props.children + } +} function CurrentBuildDisplay() { return ( + <BuildErrorBoundary> <CardThemed> {/* existing code */} </CardThemed> + </BuildErrorBoundary> ) }
Line range hint
291-391: Consider lazy loading debug componentThe debug component contains complex calculations and renders that might impact performance. Consider lazy loading it only when needed.
+const LazyCalcDebug = lazy(() => import('./CalcDebug')) function CharacterSection() { return ( <Stack spacing={1}> {/* existing code */} - <CalcDebug /> + <Suspense fallback={<CircularProgress />}> + <LazyCalcDebug /> + </Suspense> </Stack> ) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
libs/sr/page-team/src/ComboEditor.tsx(2 hunks)libs/sr/page-team/src/TeammateDisplay.tsx(5 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
libs/sr/page-team/src/TeammateDisplay.tsx
[error] 60-60: 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 (5)
libs/sr/page-team/src/ComboEditor.tsx (3)
27-29: Add null check for team.frames
The mapping operation assumes team.frames is always an array. Add a null check to prevent runtime errors.
- {team.frames.map((frame, i) => (
+ {team.frames?.map((frame, i) => (
<Combo key={i} frame={frame} index={i} />
))}53-60: Improve error handling UX
The current error handling only logs to console. Consider showing a user-friendly error message in the UI.
try {
return (
calc?.withTag(tagcontext).compute(new Read(frame.tag, 'sum')).val ?? 0
)
} catch (error) {
console.error('Error computing value:', error)
+ // Show error in UI
+ const errorMessage = error instanceof Error ? error.message : 'Unknown error'
+ database.notifications.add({
+ type: 'error',
+ title: 'Computation Error',
+ message: `Failed to compute value: ${errorMessage}`,
+ })
return 0
}14-46: Implementation aligns well with PR objectives
The ComboEditor component successfully implements the required changes:
- Proper layout structure for fixed view team menu UI
- Integration of multiplier functionality
- Clear visual hierarchy with index, optimization target, and computed values
The implementation meets the objectives outlined in PR #2553 and issue #2540.
libs/sr/page-team/src/TeammateDisplay.tsx (2)
54-94: LGTM! Well-structured section management
The component effectively manages sections with proper memoization and key props. The structure aligns with the PR objective of relocating the combo editor.
🧰 Tools
🪛 Biome (1.9.4)
[error] 60-60: 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)
Line range hint 184-213: LGTM! Well-implemented character management
The component effectively manages character data and editing functionality with proper state management and UI feedback.
🧰 Tools
🪛 Biome (1.9.4)
[error] 60-60: 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)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
libs/sr/formula-ui/src/lightCone/util.tsx (1)
17-18: Consider additional error handlingWhile the code checks for light cone existence, there's still a potential edge case where
hasLightConeis true but the superimpose computation fails (returns undefined).Consider making the error handling more explicit:
- if (hasLightCone) - superimpose = calc.compute(own.lightCone.superimpose).val ?? 1 + if (hasLightCone) { + const computed = calc.compute(own.lightCone.superimpose).val + if (computed === undefined) { + console.warn(`Failed to compute superimpose value for light cone: ${lcKey}`) + } + superimpose = computed ?? 1 + }libs/sr/formula-ui/src/lightCone/sheets/PastSelfInMirror.tsx (2)
42-43: Improve string translations and formatting.Several strings need attention:
- "Duration" needs translation (marked with TODO)
- "Energy regen at start of each wave" should be properly formatted
- "Skill POint" has a typo and inconsistent casing
Consider these improvements:
- title: 'Duration', + title: chg('duration'), // Add translation key - title: 'Energy regen at start of each wave', + title: chg('energyRegenWaveStart'), // Add translation key with proper casing - title: 'Skill POint recovered', + title: chg('skillPointRecovered'), // Fix typo and add translation keyAlso applies to: 63-63
Line range hint
8-9: Address TODO comments.There are several TODO comments that need attention:
- getInterpolateObject for lightcone
- translate "Duration"
- conditional display based on brEffect_ threshold
Would you like me to help implement any of these TODOs or create GitHub issues to track them?
Also applies to: 42-42, 51-52
libs/sr/formula/src/data/util/tag.ts (1)
117-117: Consider documenting the accumulation behavior.Since the accumulation behavior for
superimposehas been changed from unique to sum, it would be helpful to document why this accumulation method was chosen and its implications.Add a comment above the
lightConeproperty to explain the accumulation behavior:+ // Superimpose values are accumulated (summed) rather than taking unique values + // to account for multiple contributions in calculations lightCone: { lvl: iso, ascension: iso, superimpose: isoSum },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
libs/sr/formula-ui/src/lightCone/sheets/PastSelfInMirror.tsx(2 hunks)libs/sr/formula-ui/src/lightCone/util.tsx(1 hunks)libs/sr/formula/src/data/util/tag.ts(1 hunks)
🔇 Additional comments (4)
libs/sr/formula-ui/src/lightCone/util.tsx (2)
6-12: LGTM! Type-safe component signature
The addition of the lcKey parameter with proper typing enhances type safety and makes the component's dependencies explicit.
14-19: Improved validation logic
The new implementation properly validates the existence of both the calculation context and the light cone before computing the superimpose value, making it more robust than the previous version.
libs/sr/formula-ui/src/lightCone/sheets/PastSelfInMirror.tsx (1)
45-47: LGTM! Verify SuperImposeWrapper usage consistency.
The addition of lcKey prop to SuperImposeWrapper components is correct and aligns with the component's updated interface.
Let's verify consistent usage across other light cone sheets:
Also applies to: 64-66
✅ Verification successful
SuperImposeWrapper usage is correctly implemented
The verification shows that:
- PastSelfInMirror.tsx is currently the only light cone sheet using SuperImposeWrapper
- The SuperImposeWrapper component requires
lcKeyprop in its interface - Both instances in PastSelfInMirror.tsx correctly provide the required
lcKeyprop - No other light cone sheets currently need this wrapper
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any SuperImposeWrapper usage without lcKey prop
# Expected: All SuperImposeWrapper components should have lcKey prop
# Search for SuperImposeWrapper usage without lcKey prop
ast-grep --pattern 'SuperImposeWrapper>$$$</SuperImposeWrapper' libs/sr/formula-ui/src/lightCone/sheets/
# Compare with total usage to ensure we haven't missed any
ast-grep --pattern 'SuperImposeWrapper lcKey={$_}>$$$</SuperImposeWrapper' libs/sr/formula-ui/src/lightCone/sheets/
Length of output: 224
Script:
#!/bin/bash
# Let's try a different approach using ripgrep to find SuperImposeWrapper usage
rg -U "SuperImposeWrapper.*?>" libs/sr/formula-ui/src/lightCone/sheets/ -A 1
# Also search for any imports of SuperImposeWrapper to ensure we're looking at the right component
rg "import.*SuperImposeWrapper" libs/sr/formula-ui/src/lightCone/sheets/
Length of output: 1058
Script:
#!/bin/bash
# Let's check other light cone sheets to ensure we haven't missed any SuperImposeWrapper usage
rg -l "SuperImposeWrapper" libs/sr/formula-ui/src/lightCone/sheets/
# Let's also check the SuperImposeWrapper component definition to verify the lcKey prop requirement
rg -U "SuperImposeWrapper.*?{" libs/sr/formula-ui/src/lightCone/util.ts -A 5
Length of output: 315
Script:
#!/bin/bash
# Let's fix the regex and check the SuperImposeWrapper component definition
rg "export.*SuperImposeWrapper" -A 10 libs/sr/formula-ui/src/lightCone/util.ts
# Also check if there are any other files that might use SuperImposeWrapper
fd -e tsx -e ts . libs/sr/formula-ui/src/lightCone/ -x rg -l "SuperImposeWrapper" {}
Length of output: 351
Script:
#!/bin/bash
# Check the SuperImposeWrapper component definition in util.tsx
rg "export.*SuperImposeWrapper" -A 10 libs/sr/formula-ui/src/lightCone/util.tsx
# Also check if there are any other light cone sheets that should be using SuperImposeWrapper
fd -e tsx ".*\.tsx$" libs/sr/formula-ui/src/lightCone/sheets/
Length of output: 488
libs/sr/formula/src/data/util/tag.ts (1)
117-117: Verify the impact of changing superimpose accumulation.
The change from iso to isoSum modifies how superimpose values are accumulated, switching from unique values to summed values. While this aligns with the combo editor updates, we should verify the impact on existing calculations.
✅ Verification successful
Let me try another search to find more relevant usage of lightCone.superimpose in the codebase:
Based on the search results, I can now generate the final response:
The change to use isoSum for lightCone superimpose is safe and correct.
The change from iso to isoSum for lightCone.superimpose is verified to be safe based on its usage in the codebase:
- The value is used in UI components (
LightConeCard,LightConeCardCompact,LightConeEditorCard) for display purposes - In calculations, it's accessed through
calc.compute(own.lightCone.superimpose)which handles both accumulation types - The change to sum accumulation aligns with how superimposition values should be aggregated in calculations
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for usage of lightCone.superimpose to verify impact
# Look for direct references to lightCone.superimpose in calculations
# Search for lightCone.superimpose usage in formula calculations
ast-grep --pattern 'lightCone.superimpose'
# Search for any references to superimpose in test files
rg -g '*.test.ts' -g '*.test.tsx' 'superimpose'
Length of output: 57463
Script:
#!/bin/bash
# Search for files that use lightCone.superimpose in calculations
rg -g '*.ts' -g '*.tsx' -A 3 'lightCone.*superimpose'
# Search for files that define or use the lightCone type/interface
ast-grep --pattern 'interface $_ { $$$superimpose$$$}'
Length of output: 2226
There was a problem hiding this comment.
This shiz is laggy as hell
There was a problem hiding this comment.
If you are referring to updating the UI with generated builds, you are right, it can be laggy. I think in the future I'll make the generated builds be skeleton placeholders if they are out of view, so they are not being updated when parts of the teams are changed.
There was a problem hiding this comment.
Shouldn't the conditionals here have the DST already set based on the char set?
And maybe SRC too, but i guess this accomodates for changing equipment on buffer side
There was a problem hiding this comment.
I have idea of setting src to currently selected character, and dst to the current frame target sheet character, but that is ergonomics that I'll figure out later once this UI is more mature.
There was a problem hiding this comment.
I was more thinking, you should never be able to change dst in this view, as it is encased within the "current character view". So changing buffs for other src/dst seems a bit strange. Unless we split the UI some more, which I could also see working
There was a problem hiding this comment.
I agree, if we finalize the UI in this state, this "hardcode" may make sense. I will, however, keep it as is for now to allow flexibility to iterate UI. currently, all conds will show here, in the "hardcoded" case, it will have to be filtered to specific src/dst
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
libs/sr/page-team/src/TeammateDisplay.tsx (2)
51-53: Consider improving constant naming and documentationThe magic numbers used for spacing and positioning could benefit from more descriptive names and documentation explaining their purpose and constraints.
-const BOT_PX = 0 -const SECTION_SPACING_PX = 33 +// Minimum bottom spacing for sections in pixels +const SECTION_BOTTOM_SPACING_PX = 0 +// Vertical spacing between sticky section headers in pixels +const SECTION_HEADER_SPACING_PX = 33
95-132: Consider memoizing style calculationsThe style calculations for positioning could be memoized to prevent unnecessary recalculations during renders.
+const useSectionStyles = (index: number, numSections: number, headerHeight: number) => + useMemo(() => ({ + sticky: (theme: any) => ({ + outline: `solid ${theme.palette.secondary.main}`, + position: 'sticky', + top: headerHeight + index * SECTION_SPACING_PX, + bottom: BOT_PX + (numSections - 1 - index) * SECTION_SPACING_PX, + zIndex: 100, + }), + scroll: { + scrollMarginTop: headerHeight + (index + 1) * SECTION_SPACING_PX, + } + }), [index, numSections, headerHeight]) function Section({ index, title, children }) { const [charScrollRef, onScroll] = useScrollRef() const numSections = useContext(SectionNumContext) const headerHeight = useContext(TeamHeaderHeightContext) + const styles = useSectionStyles(index, numSections, headerHeight) return ( <> - <CardThemed sx={(theme) => ({...})}/> + <CardThemed sx={styles.sticky}/> <Box ref={charScrollRef} - sx={{scrollMarginTop: ...}} + sx={styles.scroll} > {children} </Box> </> ) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
libs/sr/page-team/src/TeammateDisplay.tsx(5 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
libs/sr/page-team/src/TeammateDisplay.tsx
[error] 60-60: 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 (3)
libs/sr/page-team/src/TeammateDisplay.tsx (3)
Line range hint 187-223: LGTM! Components are well-structured
The BuildDisplay components properly handle different build types and use appropriate context hooks. The implementation is clean and maintainable.
Also applies to: 224-275
54-81:
Handle frame selection after deletion
When the current frame is deleted, the component should automatically select another valid frame to maintain a good user experience.
const sections: Array<[key: string, title: ReactNode, content: ReactNode]> =
useMemo(() => {
const frame = team.frames[presetIndex]
- if (!frame) return [['char', 'Character', <CharacterSection />]]
+ if (!frame) {
+ // If no frame exists at current index, select the last available frame
+ if (team.frames.length > 0) {
+ setPresetIndex(team.frames.length - 1)
+ }
+ return [['char', 'Character', <CharacterSection key="char" />]]
+ }
return [🧰 Tools
🪛 Biome (1.9.4)
[error] 60-60: 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)
133-186: 🛠️ Refactor suggestion
Add error handling for frame operations
The frame operations (set/remove) should include error handling to gracefully handle edge cases and provide user feedback.
const setFrame = (frame: Partial<Frame>) => {
+ try {
database.teams.set(teamId, (team) => {
team.frames = [...team.frames]
team.frames[presetIndex] = {
...team.frames[presetIndex],
...frame,
}
if (!team.frames.length) setPresetIndex(0)
else if (team.frames.length <= presetIndex)
setPresetIndex(team.frames.length - 1)
})
+ } catch (error) {
+ console.error('Failed to update frame:', error)
+ // TODO: Add user notification of failure
+ }
}Previous review already suggested adding user confirmation for frame deletion.
Describe your changes
Considerations for the future
Issue or discord link
Testing/validation
Team.Name.-.Character.-.Star.Rail.Optimizer.-.Google.Chrome.2024-11-27.13-52-05.mp4
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
TeammateContextfor improved management of teammate data.TeamHeadercomponent for better team interface organization.useScrollRefhook for enhanced scrolling behavior.Improvements
TeamDataManager.Bug Fixes
BonusStatsSection.Refactor
ComboEditor,TeammateDisplay, andTeamCharacterSelector.