SRO preliminary buildTC and UI #2536
Conversation
WalkthroughThe pull request introduces several enhancements across multiple files, primarily focusing on the addition of new hooks and components for managing builds and relics. Notable changes include the introduction of Changes
Assessment against linked issues
Possibly related PRs
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] [Sat Nov 9 02:34:56 UTC 2024] - Deployed 1284193 to https://genshin-optimizer-prs.github.io/pr/2536/sr-frontend (Takes 3-5 minutes after this completes to be available) [sr-frontend] [Sun Nov 10 05:08:23 UTC 2024] - Deployed 7baf7c2 to https://genshin-optimizer-prs.github.io/pr/2536/sr-frontend (Takes 3-5 minutes after this completes to be available) [sr-frontend] [Sun Nov 10 17:17:55 UTC 2024] - Deployed 328ef8e to https://genshin-optimizer-prs.github.io/pr/2536/sr-frontend (Takes 3-5 minutes after this completes to be available) [frontend] [Sun Nov 10 17:18:10 UTC 2024] - Deployed 328ef8e to https://genshin-optimizer-prs.github.io/pr/2536/frontend (Takes 3-5 minutes after this completes to be available) [sr-frontend] [Sun Nov 10 17:18:56 UTC 2024] - Deployed 25a93f6 to https://genshin-optimizer-prs.github.io/pr/2536/sr-frontend (Takes 3-5 minutes after this completes to be available) [frontend] [Sun Nov 10 17:19:09 UTC 2024] - Deployed 25a93f6 to https://genshin-optimizer-prs.github.io/pr/2536/frontend (Takes 3-5 minutes after this completes to be available) [sr-frontend] [Mon Nov 11 05:35:12 UTC 2024] - Deployed 309a8be to https://genshin-optimizer-prs.github.io/pr/2536/sr-frontend (Takes 3-5 minutes after this completes to be available) [frontend] [Mon Nov 11 05:35:45 UTC 2024] - Deployed 309a8be to https://genshin-optimizer-prs.github.io/pr/2536/frontend (Takes 3-5 minutes after this completes to be available) [sr-frontend] [Mon Nov 11 07:16:06 UTC 2024] - Deployed f68b8f0 to https://genshin-optimizer-prs.github.io/pr/2536/sr-frontend (Takes 3-5 minutes after this completes to be available) [frontend] [Mon Nov 11 07:16:12 UTC 2024] - Deployed f68b8f0 to https://genshin-optimizer-prs.github.io/pr/2536/frontend (Takes 3-5 minutes after this completes to be available) [frontend] [Mon Nov 11 22:07:08 UTC 2024] - Deployed d48e07a to https://genshin-optimizer-prs.github.io/pr/2536/frontend (Takes 3-5 minutes after this completes to be available) [sr-frontend] [Mon Nov 11 22:07:33 UTC 2024] - Deployed d48e07a to https://genshin-optimizer-prs.github.io/pr/2536/sr-frontend (Takes 3-5 minutes after this completes to be available) [sr-frontend] [Tue Nov 12 01:29:08 UTC 2024] - Deployed 8931055 to https://genshin-optimizer-prs.github.io/pr/2536/sr-frontend (Takes 3-5 minutes after this completes to be available) [frontend] [Tue Nov 12 01:29:28 UTC 2024] - Deployed 8931055 to https://genshin-optimizer-prs.github.io/pr/2536/frontend (Takes 3-5 minutes after this completes to be available) [frontend] [Tue Nov 12 02:20:22 UTC 2024] - Deployed 52bccf5 to https://genshin-optimizer-prs.github.io/pr/2536/frontend (Takes 3-5 minutes after this completes to be available) [sr-frontend] [Tue Nov 12 02:20:38 UTC 2024] - Deployed 52bccf5 to https://genshin-optimizer-prs.github.io/pr/2536/sr-frontend (Takes 3-5 minutes after this completes to be available) [frontend] [Tue Nov 12 03:39:03 UTC 2024] - Deployed e972410 to https://genshin-optimizer-prs.github.io/pr/2536/frontend (Takes 3-5 minutes after this completes to be available) [sr-frontend] [Tue Nov 12 03:39:14 UTC 2024] - Deployed e972410 to https://genshin-optimizer-prs.github.io/pr/2536/sr-frontend (Takes 3-5 minutes after this completes to be available) [sr-frontend] [Tue Nov 12 03:42:14 UTC 2024] - Deployed 955b815 to https://genshin-optimizer-prs.github.io/pr/2536/sr-frontend (Takes 3-5 minutes after this completes to be available) [frontend] [Tue Nov 12 03:42:29 UTC 2024] - Deployed 955b815 to https://genshin-optimizer-prs.github.io/pr/2536/frontend (Takes 3-5 minutes after this completes to be available) [Fri Nov 15 19:00:59 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: 20
🧹 Outside diff range and nitpick comments (41)
libs/sr/db-ui/src/hooks/useBuildTc.ts (1)
1-6: Add TypeScript types and documentation for better maintainability.The hook implementation could benefit from:
- Explicit return type definition
- JSDoc documentation explaining the hook's purpose and usage
- Error handling for missing database context
Consider applying these improvements:
+import { BuildTc } from '@genshin-optimizer/sr/database' // Assuming this type exists +import { DataManager } from '@genshin-optimizer/common/database' import { useDataManagerBase } from '@genshin-optimizer/common/database-ui' import { useDatabaseContext } from '../context' +/** + * Hook to manage build test cases (TCs) in the database. + * @param buildTcId - Unique identifier for the build TC, can be undefined + * @returns DataManager instance for the specified build TC + * @throws Error if database context is not available + */ -export function useBuildTc(buildTcId: string | undefined) { +export function useBuildTc(buildTcId: string | undefined): DataManager<BuildTc> { const { database } = useDatabaseContext() + if (!database) { + throw new Error('Database context not found. Ensure useBuildTc is used within a DatabaseProvider') + } return useDataManagerBase(database.buildTcs, buildTcId ?? '') }libs/common/ui/src/hooks/useRefSize.tsx (3)
Line range hint
10-13: Add null check in handleResize function.The resize handler doesn't verify if
ref.currentexists before accessing its properties, which could lead to runtime errors.Apply this diff to add the null check:
const handleResize = () => { - setWidth(ref.current?.clientWidth ?? 0) - setHeight(ref.current?.clientHeight ?? 0) + const element = ref.current + if (element) { + setWidth(element.clientWidth) + setHeight(element.clientHeight) + } else { + setWidth(0) + setHeight(0) + } }
Line range hint
9-19: Add ref to effect dependencies.The effect doesn't re-run if the ref changes, which could lead to stale measurements.
Apply this diff to add the dependency:
- }, []) + }, [ref])
Line range hint
1-21: Consider performance optimizations.The current implementation could benefit from several performance improvements:
- Consider using
useLayoutEffectinstead ofuseEffectto prevent layout flicker- Add debouncing to the resize handler to prevent excessive re-renders
Here's a suggested implementation with these optimizations:
'use client' import { useLayoutEffect, useRef, useState } from 'react' import debounce from 'lodash/debounce' export function useRefSize() { const ref = useRef<HTMLElement>() const [width, setWidth] = useState(0) const [height, setHeight] = useState(0) useLayoutEffect(() => { const element = ref.current if (!element) return const handleResize = debounce(() => { setWidth(element.clientWidth) setHeight(element.clientHeight) }, 250) // Initial measurement handleResize() window.addEventListener('resize', handleResize) return () => { handleResize.cancel() window.removeEventListener('resize', handleResize) } }, [ref]) return { width, height, ref } }libs/sr/db/src/Interfaces/IBuildTc.ts (1)
17-17: LGTM: Well-structured type definition for TC light conesGood use of TypeScript's
Omitutility type to create a specialized light cone type for TC builds. The omission oflocationandlockfields is appropriate as TC builds are theoretical and don't need these runtime-specific properties.Consider documenting why these fields are omitted, either in a comment or in the project's technical documentation, to help future maintainers understand the design decision.
libs/common/ui/src/hooks/useRefOverflow.tsx (1)
24-30: Consider enhancing helper functions for better reusabilityThe helper functions could be improved with additional features:
- Memoization for performance
- Optional threshold parameter for customizable overflow detection
Here's a suggested enhancement:
+const DEFAULT_THRESHOLD = 1; // 1px threshold for overflow + -function isOverflowedX(ref: HTMLElement) { - return ref.scrollWidth > ref.clientWidth +function isOverflowedX(ref: HTMLElement, threshold = DEFAULT_THRESHOLD) { + return ref.scrollWidth > ref.clientWidth + threshold } -function isOverflowedY(ref: HTMLElement) { - return ref.scrollHeight > ref.clientHeight +function isOverflowedY(ref: HTMLElement, threshold = DEFAULT_THRESHOLD) { + return ref.scrollHeight > ref.clientHeight + threshold }libs/sr/db/src/Database/DataManager.ts (1)
Line range hint
1-47: Document the key pattern requirementSince this class is foundational to the database layer and is being modified to support TC builds, consider adding:
- JSDoc comments explaining the required key pattern with underscore
- Examples of valid/invalid keys
- Migration notes for downstream implementations
export class DataManager< CacheKey extends string, DataKey extends string, CacheValue extends StorageValue, StorageValue > extends DataManagerBase< CacheKey, DataKey, CacheValue, StorageValue, SroDatabase > { + /** + * Manages data storage and retrieval with specific key patterns. + * Keys are stored in format: `${prefix}_${key}_${id}` where: + * - prefix: Database prefix + * - key: Data type identifier + * - id: Unique identifier + * + * @example + * Valid keys: "sro_buildTc_123", "sro_character_456" + * Invalid keys: "sro_buildTc", "sro_character" + */ constructor(database: SroDatabase, dataKey: DataKey) {libs/sr/ui/src/Relic/RelicRarityDropdown.tsx (1)
17-23: Consider adding JSDoc documentation for the exported component.The component declaration looks good. Using a named export is better for tree-shaking. Consider adding JSDoc documentation to describe the component's purpose and props, especially since it's a public API.
+/** + * A dropdown component for selecting relic rarity with optional number display. + * @param props.rarity - The currently selected rarity + * @param props.onRarityChange - Callback when rarity changes + * @param props.filter - Optional function to disable certain rarity options + * @param props.showNumber - Whether to show the rarity number alongside stars + */ export function RelicRarityDropdown({libs/sr/ui/src/LightCone/LightConeAutocomplete.tsx (2)
15-22: Consider simplifying the label fallback logic.The label assignment could be more concise using the nullish coalescing operator.
- label = label ? label : t('editor.lightConeName') + label = label ?? t('editor.lightConeName')
1-60: Consider adding unit tests for the component.Given this component's role in TC builds and its reusable nature, it would benefit from comprehensive unit tests covering:
- Option generation
- Selection handling
- Loading states
- Translation integration
Would you like me to help generate unit test cases for this component?
libs/sr/ui/src/Relic/RelicMainStatDropdown.tsx (3)
13-25: Add JSDoc documentation for the component and its props.While the types are well-defined, adding JSDoc documentation would improve maintainability and help other developers understand the component's purpose and usage.
Add documentation like this:
+/** + * Dropdown component for selecting relic main stats. + * @param {Object} props + * @param {RelicMainStatKey} [props.statKey] - Currently selected stat key + * @param {RelicSlotKey} props.slotKey - The relic slot this dropdown is for + * @param {Function} props.setStatKey - Callback to update the selected stat + * @param {ReactNode} [props.defText] - Default text when no stat is selected + * @param {DropdownButtonProps} [props.dropdownButtonProps] - Additional props for the dropdown button + */ export function RelicMainStatDropdown({
26-34: Consider adding safety checks for array access and improve type safety.The current implementation has potential edge cases:
- Direct array access with
[0]assumes the array is non-empty- The condition could be more explicit about statKey existence
Consider this safer implementation:
- if ((slotKey === 'head' || slotKey === 'hands') && statKey) + if ((slotKey === 'head' || slotKey === 'hands')) { + const validStats = relicSlotToMainStatKeys[slotKey] + if (!validStats.length) return null + const mainStat = validStats[0] return ( <CardThemed sx={{ p: 1 }} bgt="light"> <StatDisplay - statKey={relicSlotToMainStatKeys[slotKey][0]} + statKey={mainStat} showPercent /> </CardThemed> ) + }
35-63: Consider performance optimization for menu items.The current implementation recreates MenuItem components on every render. Consider memoizing the menu items if the list of possible stats is static.
Here's a suggested optimization:
+const StatMenuItem = React.memo(({ + mk, + selected, + onClick +}: { + mk: RelicMainStatKey, + selected: boolean, + onClick: () => void +}) => ( + <MenuItem + selected={selected} + disabled={selected} + onClick={onClick} + > + <ListItemIcon> + <StatIcon statKey={mk} /> + </ListItemIcon> + <StatDisplay statKey={mk} showPercent disableIcon /> + </MenuItem> +)) export function RelicMainStatDropdown({ // ... existing props }) { // ... existing code return ( <DropdownButton // ... existing props > {relicSlotToMainStatKeys[slotKey].map((mk) => ( - <MenuItem - key={mk} - selected={statKey === mk} - disabled={statKey === mk} - onClick={() => setStatKey(mk)} - > - <ListItemIcon> - <StatIcon statKey={mk} /> - </ListItemIcon> - <StatDisplay statKey={mk} showPercent disableIcon /> - </MenuItem> + <StatMenuItem + key={mk} + mk={mk} + selected={statKey === mk} + onClick={() => setStatKey(mk)} + /> ))} </DropdownButton> )libs/sr/ui/src/Character/CharacterCard.tsx (1)
56-57: Consider separating TC build logicSince this component is used for both normal and TC builds (as per PR objectives), consider adding a clear separation of concerns:
- Consider adding a prop to distinguish between TC and normal builds
- Add appropriate type guards or conditional logic to handle TC-specific requirements
Example approach:
type CharacterCardProps = { character: ICachedCharacter onClick?: () => void isTCBuild?: boolean // Add this prop } // Then in the component: const calc = useSrCalcContext() ?? ( isTCBuild ? srCalculatorWithEntries(charTagMapNodeEntries(character)) : // original calculation logic for normal builds )libs/sr/formula/src/util.ts (1)
Line range hint
38-67: Consider adding JSDoc documentation.The function is well-structured and handles character data comprehensively. The name change to
charTagMapNodeEntriesbetter reflects its purpose. Consider adding JSDoc documentation to explain the function's purpose and parameter structure.Add documentation like:
/** * Generates tag map node entries for character data processing * @param data Character data containing level, skills, and other attributes * @returns Array of TagMapNodeEntries for character processing */libs/sr/formula/src/formula.test.ts (4)
Line range hint
44-52: Consider adding JSDoc comments for the test parametersWhile the test data construction is well-structured, adding JSDoc comments explaining the significance of the test parameters (level, ascension, atk, def, hp, spd) would improve maintainability.
Example documentation:
/** * Test cases for March7th character stats at different levels and ascensions * @param level Character level * @param ascension Ascension phase (0-6) * @param atk Expected attack value * @param def Expected defense value * @param hp Expected HP value * @param spd Expected speed value */
Line range hint
78-90: Extract character setup into a helper functionThe character setup code is duplicated across test cases and contains minimal values. Consider extracting it into a helper function for better maintainability.
function getBaseCharacterSetup(charKey: CharacterKey = 'March7th') { return charTagMapNodeEntries({ level: 1, ascension: 0, key: charKey, eidolon: 0, basic: 0, skill: 0, ult: 0, talent: 0, bonusAbilities: {}, statBoosts: {}, }); }
90-90: Consider consistent parameter style between functions
lightConeTagMapNodeEntriesuses individual parameters whilecharTagMapNodeEntriesuses an object parameter. Consider using a consistent style for better maintainability.// Suggested signature: function lightConeTagMapNodeEntries(params: { key: LightConeKey, level: number, ascension: AscensionKey, superimposition: number })
Line range hint
114-126: Enhance test coverage for combined statsThe combined character + light cone test only covers a single scenario. Consider adding more test cases with different level/ascension combinations, similar to the individual character and light cone tests.
Example:
it.each([ [1, 0, 81.6], [20, 0, 191.16], [20, 1, 236.28], ])('calculate base stats at different levels', (charLevel, charAscension, expectedAtk) => { // ... test implementation });libs/sr/ui/src/LightCone/LightConeEditor/LightConeEditorCard.tsx (3)
56-59: Consider improving null handling in LightConeAutocomplete.The empty string fallback for
lightCone?.key ?? ''could lead to invalid states. Consider usingundefinedinstead to make the unselected state more explicit.-lcKey={lightCone?.key ?? ''} +lcKey={lightCone?.key}
136-141: Enhance accessibility of the DropdownButton.Add ARIA labels to improve screen reader support.
<DropdownButton title={`${t('superimpose')} ${superimpose}`} color="primary" disabled={disabled} fullWidth={true} + aria-label={t('lightCone:editor.selectSuperimposition')} >
26-38: Consider memoizing callback props for better performance.To prevent unnecessary re-renders, consider memoizing the callback functions passed as props.
+import { useCallback } from 'react' export function LightConeEditorCard({ onClose, lightCone, setLightCone, footer, hideLocation = false, }: { onClose: (e: MouseEvent) => void lightCone: Partial<ILightCone> setLightCone: (lightConeData: Partial<ILightCone>) => void footer?: ReactNode hideLocation?: boolean }) { + const handleLightConeChange = useCallback((lightConeData: Partial<ILightCone>) => { + setLightCone(lightConeData) + }, [setLightCone])libs/sr/ui/src/LightCone/LightConeCard.tsx (1)
Line range hint
31-43: Consider enhancing props for TC build supportSince this PR introduces TC builds, consider adding TC-specific props to
LightConeCardProps:export type LightConeCardProps = { lightCone: ILightCone onClick?: () => void onEdit?: () => void onDelete?: () => void onLockToggle?: () => void canEquip?: boolean setLocation?: (lk: LocationKey) => void extraButtons?: JSX.Element + // Add TC-specific props + isTCBuild?: boolean + tcBuildId?: string + onTCBuildChange?: (changes: TCBuildChanges) => void }This would allow the component to handle TC-specific behaviors and styling differently when used in TC builds.
libs/sr/ui/src/LightCone/LightConeCardCompact.tsx (2)
83-89: LGTM! Consider adding ARIA attributes for better accessibility.The layout structure is well-organized with proper use of flexbox and consistent dimensions. However, consider enhancing accessibility for screen readers.
Add ARIA attributes to improve accessibility:
<CardThemed bgt={bgt} sx={{ height: COMPACT_ELE_HEIGHT, width: COMPACT_ELE_WIDTH, }} + role="article" + aria-label="Light Cone Card" >Also applies to: 90-181, 235-247
136-154: Consider adding a loading state for better UX.The UI elements are well-structured with good visual hierarchy. However, there's no visible loading state while the light cone data is being fetched.
Consider adding a loading skeleton:
export function LightConeCardCompact({ lightConeId, bgt, onClick, }: { lightConeId?: string bgt?: CardBackgroundColor onClick?: () => void }) { const lightCone = useLightCone(lightConeId) + const isLoading = !lightCone && lightConeId + if (isLoading) return <LightConeCardSkeleton /> if (!lightCone) return <LightConeCardCompactEmpty bgt={bgt} /> return ( <LightConeCardCompactObjAlso applies to: 156-166, 175-177
libs/sr/db/src/Database/DataManagers/BuildTcDataManager.ts (2)
85-89: Consider optimizing getBuildTcIds for performanceWhile the current implementation is correct, consider these improvements:
- Use a Map or index for O(1) lookups if this method is frequently called
- Add return type annotation for better type safety
- getBuildTcIds(characterKey: CharacterKey) { + getBuildTcIds(characterKey: CharacterKey): string[] {
101-103: Extract magic number to a named constantThe hard-coded rarity value of 5 should be extracted to a named constant for better maintainability and consistency. Consider using a default from your constants file.
+ const DEFAULT_RELIC_RARITY = 5; substats: { type: 'max', stats: objKeyMap(allRelicSubStatKeys, () => 0), - rarity: 5, + rarity: DEFAULT_RELIC_RARITY, },libs/sr/ui/src/Relic/RelicEditor/SubstatInput.tsx (3)
37-37: Address or remove TODO commentThe TODO comment lists several important features. Consider creating separate issues to track these improvements or remove if no longer relevant.
Would you like me to create GitHub issues to track these improvements?
Line range hint
248-267: Improve Slider implementationThe current Slider implementation has several potential issues:
- Marks are defined but commented out, making the slider less intuitive
- Hardcoded step size (0.1) might not be appropriate for all stat types
- Potential precision issues with floating-point steps
Consider this improvement:
<Slider value={innerValue} - // step={null} - step={0.1} + step={(marks[marks.length - 1]?.value ?? 0) / 100} // Dynamic step based on max value disabled={disabled} - // marks={marks} + marks={marks} min={marks[1]?.value ?? 0} max={marks[marks.length - 1]?.value ?? 0} onChange={(_e, v) => setinnerValue(v as number)} onChangeCommitted={(_e, v) => setValue(v as number)} valueLabelDisplay="auto" + sx={{ mt: 2, mb: 1 }} // Add some spacing for better UX />
Line range hint
1-267: Clean up commented codeThere are several blocks of commented-out code throughout the file. This makes the code harder to maintain and understand.
Consider:
- Removing commented code if it's no longer needed
- Converting useful comments into TODO items with clear rationale
- Moving experimental code to a separate branch
Examples of commented sections to clean up:
- Lines 156-171: Roll calculation buttons
- Lines 190-203: Roll count badge
- Lines 218-227: Efficiency display
libs/sr/ui/src/Relic/RelicEditor/RelicEditor.tsx (1)
360-369: Consider enhancing type safety for RelicMainStatDropdown propsWhile the implementation is solid, consider adding TypeScript interfaces to document the component's API and improve type safety:
interface RelicMainStatDropdownProps { slotKey: RelicSlotKey; statKey?: string; setStatKey: (key: string) => void; defText: string; dropdownButtonProps?: Partial<ButtonProps>; // From MUI }This would:
- Make the component's API self-documenting
- Prevent passing invalid props
- Enable better IDE support
libs/sr/page-team/src/TeamCalcProvider.tsx (1)
171-174: Simplify type assertion forstatsobjectIn the
relicTcDatafunction, the type assertion usingas Record<RelicMainStatKey | RelicSubStatKey, number>may be unnecessary ifsubstatsalready conforms to this type. Removing the explicit type assertion can simplify the code without impacting type safety.Suggested Change:
- const stats = { ...substats } as Record< - RelicMainStatKey | RelicSubStatKey, - number - > + const stats = { ...substats }libs/sr/page-team/src/TeammateDisplay.tsx (3)
83-86: Use 'else if' for mutually exclusive conditionsThe conditions
buildType === 'real'andbuildType === 'tc'are mutually exclusive. Usingelse ifimproves readability and ensures that only one subscription is set at a time.Apply this diff:
if (buildType === 'real') unFollow = database.builds.follow(buildId, setDbDirty) - if (buildType === 'tc') + else if (buildType === 'tc') unFollow = database.buildTcs.follow(buildTcId, setDbDirty)
109-117: Specify return type for 'BuildDisplay' functionAdding an explicit return type enhances type safety and code readability.
Apply this change:
- function BuildDisplay() { + function BuildDisplay(): JSX.Element {
118-122: Specify return type for 'BuildTCDisplay' functionAdding an explicit return type enhances type safety and code readability.
Apply this change:
- function BuildTCDisplay() { + function BuildTCDisplay(): JSX.Element | null {libs/sr/ui/src/Relic/RelicCardCompact.tsx (3)
276-283: Refactor duplicatedactionWrapperFuncto improve maintainabilityThe
actionWrapperFuncis defined identically in multiple components (RelicSubCard,RelicSetCardCompact,RelicMainsCardCompact). To reduce code duplication and enhance maintainability, consider extracting it into a shared utility function or custom hook.Also applies to: 336-343, 385-392
207-208: Ensure unique keys in array renderingUsing
i + stat.keyas a key in themapfunction may lead to non-unique keys ifstat.keyis not unique. It's recommended to use a more reliable unique identifier.Apply this diff to improve the key assignment:
- <SubstatDisplay key={i + stat.key} stat={stat} /> + <SubstatDisplay key={`${stat.key}-${i}`} stat={stat} />
363-363: Translate hardcoded text for internationalizationThe message "No Relic sets" is hardcoded and lacks internationalization support. Consider using a translation function to make it translatable.
libs/sr/page-team/src/BuildsDisplay.tsx (3)
148-159: Simplify nested ternary operators for better readabilityThe use of nested ternary operators in the
useActiveBuildSwapfunction reduces code readability and may cause confusion. Consider refactoring the logic to use if-else statements or simplifying the conditionals.Here's a possible refactor:
function useActiveBuildSwap( newBuildType: TeammateDatum['buildType'], newBuildId = '' ) { const { database } = useDatabaseContext() const { teamId, teammateDatum: { characterKey, buildType, buildId, buildTcId }, } = useTeamContext() return useMemo( () => { let isActive = false if (buildType === 'equipped') { isActive = newBuildType === 'equipped' } else if (buildType === 'real') { isActive = newBuildType === 'real' && buildId === newBuildId } else if (buildType === 'tc') { isActive = newBuildType === 'tc' && buildTcId === newBuildId } return { active: isActive, onActive: () => { database.teams.set(teamId, (team) => { const teammateDatum = team.teamMetadata.find( (teammateDatum) => teammateDatum?.characterKey === characterKey ) if (!teammateDatum) { console.error( `Teammate data not found for character ${characterKey}` ) return } teammateDatum.buildType = newBuildType if (newBuildType === 'real' && newBuildId) teammateDatum.buildId = newBuildId if (newBuildType === 'tc' && newBuildId) teammateDatum.buildTcId = newBuildId }) }, } }, [ buildId, buildTcId, buildType, characterKey, database.teams, newBuildId, newBuildType, teamId, ] ) }
224-232: Maintain consistent naming for event handlersIn the
BuildBasecomponent, you're renaming theonActiveprop toonEquip, which can cause confusion. For clarity, consider using the same prop name throughout.Apply this diff to keep naming consistent:
function BuildBase({ name, description, buildGrid, active, - onActive: onEquip, + onActive, }: { name: string description?: string buildGrid: ReactNode active?: boolean onActive?: () => void }) { const onEdit = useCallback(() => {}, []) // TODO: implement return ( <CardThemed sx={(theme) => ({ outline: active ? `solid ${theme.palette.success.main}` : undefined, })} > <Box display={'flex'}> <CardActionArea onClick={onEdit}> <CardHeader title={name} subheader={description} /> </CardActionArea> <Box display={'flex'} alignContent={'center'} sx={{ padding: 1 }}> <Button size="small" color="success" - onClick={onEquip} + onClick={onActive} disabled={active} > Set Active </Button> </Box> </Box> <Divider /> <CardContent>{buildGrid}</CardContent> </CardThemed> ) }
296-299: Prevent potential division by zero in grid calculationsIn the
EquipRowcomponent, when calculatingnumCols, ensure thateleWidthPx + spacingis not zero to avoid a division by zero error.Add a check to handle this scenario:
const numCols = - Math.floor((width - eleWidthPx) / (eleWidthPx + spacing)) + 1 + eleWidthPx + spacing > 0 + ? Math.floor((width - eleWidthPx) / (eleWidthPx + spacing)) + 1 + : 1 const numRows = Math.ceil(8 / numCols) // 6 relic + set + lc setRows(numRows)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (35)
libs/common/ui/src/hooks/index.ts(1 hunks)libs/common/ui/src/hooks/useRefOverflow.tsx(1 hunks)libs/common/ui/src/hooks/useRefSize.tsx(2 hunks)libs/sr/db-ui/src/hooks/index.ts(1 hunks)libs/sr/db-ui/src/hooks/useBuildTc.ts(1 hunks)libs/sr/db/src/Database/DataManager.ts(1 hunks)libs/sr/db/src/Database/DataManagers/BuildDataManager.ts(0 hunks)libs/sr/db/src/Database/DataManagers/BuildTcDataManager.ts(8 hunks)libs/sr/db/src/Interfaces/IBuildTc.ts(1 hunks)libs/sr/db/src/Interfaces/index.ts(1 hunks)libs/sr/formula/src/data/common/index.ts(1 hunks)libs/sr/formula/src/formula.test.ts(6 hunks)libs/sr/formula/src/util.ts(3 hunks)libs/sr/page-team/src/BuildsDisplay.tsx(8 hunks)libs/sr/page-team/src/TeamCalcProvider.tsx(3 hunks)libs/sr/page-team/src/TeammateDisplay.tsx(5 hunks)libs/sr/ui/src/Character/CharacterCard.tsx(2 hunks)libs/sr/ui/src/LightCone/LightConeAutocomplete.tsx(1 hunks)libs/sr/ui/src/LightCone/LightConeCard.tsx(2 hunks)libs/sr/ui/src/LightCone/LightConeCardCompact.tsx(3 hunks)libs/sr/ui/src/LightCone/LightConeEditor/LightConeEditor.tsx(1 hunks)libs/sr/ui/src/LightCone/LightConeEditor/LightConeEditorCard.tsx(1 hunks)libs/sr/ui/src/LightCone/LightConeEditor/index.tsx(1 hunks)libs/sr/ui/src/LightCone/LightConeEditor/reducer.ts(0 hunks)libs/sr/ui/src/LightCone/index.tsx(1 hunks)libs/sr/ui/src/Relic/RelicCardCompact.tsx(6 hunks)libs/sr/ui/src/Relic/RelicEditor/RelicEditor.tsx(3 hunks)libs/sr/ui/src/Relic/RelicEditor/RelicStatKeyDisplay.tsx(0 hunks)libs/sr/ui/src/Relic/RelicEditor/SubstatInput.tsx(3 hunks)libs/sr/ui/src/Relic/RelicEditor/index.tsx(0 hunks)libs/sr/ui/src/Relic/RelicMainStatDropdown.tsx(1 hunks)libs/sr/ui/src/Relic/RelicRarityDropdown.tsx(1 hunks)libs/sr/ui/src/Relic/index.tsx(1 hunks)libs/sr/ui/src/compactConst.ts(1 hunks)libs/sr/ui/src/index.tsx(1 hunks)
💤 Files with no reviewable changes (4)
- libs/sr/db/src/Database/DataManagers/BuildDataManager.ts
- libs/sr/ui/src/LightCone/LightConeEditor/reducer.ts
- libs/sr/ui/src/Relic/RelicEditor/RelicStatKeyDisplay.tsx
- libs/sr/ui/src/Relic/RelicEditor/index.tsx
✅ Files skipped from review due to trivial changes (3)
- libs/sr/formula/src/data/common/index.ts
- libs/sr/ui/src/LightCone/LightConeEditor/index.tsx
- libs/sr/ui/src/compactConst.ts
🔇 Additional comments (40)
libs/sr/db/src/Interfaces/index.ts (1)
1-1: LGTM! Verify the interface file exists.
The addition of IBuildTc export aligns with the PR objectives for TC builds support and follows the established naming conventions.
Let's verify the interface file exists and contains the expected properties:
✅ Verification successful
Interface file exists and contains expected properties ✅
The IBuildTc interface is properly defined in libs/sr/db/src/Interfaces/IBuildTc.ts with all the necessary properties including teamId, characterKey, lightCone, and relic configurations. The barrel export is valid.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify IBuildTc interface file exists and contains expected properties
# Expected: File exists and contains teamId property as mentioned in the AI summary
# Check if the interface file exists
fd "IBuildTc.ts" --type f
# Check the interface contents
rg -A 10 "interface IBuildTc" libs/sr/db/src/Interfaces/
Length of output: 837
libs/sr/ui/src/index.tsx (2)
2-2: LGTM! Export follows consistent pattern.
The new export maintains the established pattern of wildcard exports and follows alphabetical ordering.
2-2: Verify consistent usage of compact constants.
Let's ensure the new compact constants are used consistently across the codebase, particularly in TC build-related components.
✅ Verification successful
Let me gather more information about the compact constants and their usage.
Based on the analysis of the code, I can now generate the final response:
The compact constants are used consistently across the codebase.
The compact constants (COMPACT_ELE_HEIGHT and COMPACT_ELE_WIDTH) are properly used in:
RelicCardCompactcomponent for relic displayLightConeCardCompactcomponent for light cone displayRelicSetCardCompactcomponent for relic set displayRelicSubCardcomponent for substat displayBuildsDisplaycomponent for grid layout calculations
All components maintain consistent sizing using these constants, particularly in TC build-related components as expected.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check usage of compact constants in components
# Expected: Find usage in LightConeCardCompact, RelicCardCompact, and other TC-related components
# Search for constant usage
echo "Searching for compact constant usage:"
rg -l "COMPACT_ELE_(HEIGHT|WIDTH)"
# Search for compact components that might need these constants
echo -e "\nSearching for compact components:"
ast-grep --pattern 'export const $name = $$$' | rg -i "compact"
Length of output: 431
Script:
#!/bin/bash
# Check the actual constants defined and their usage patterns
echo "Constants defined in compactConst.ts:"
cat libs/sr/ui/src/compactConst.ts
echo -e "\nUsage in RelicCardCompact:"
cat libs/sr/ui/src/Relic/RelicCardCompact.tsx
echo -e "\nUsage in LightConeCardCompact:"
cat libs/sr/ui/src/LightCone/LightConeCardCompact.tsx
echo -e "\nUsage in BuildsDisplay:"
cat libs/sr/page-team/src/BuildsDisplay.tsx
Length of output: 40822
libs/sr/db-ui/src/hooks/index.ts (1)
2-2: LGTM! Verify the hook implementation.
The export follows the established pattern and maintains alphabetical ordering. The addition aligns with the PR's objective of integrating TC builds functionality.
Let's verify the hook implementation:
✅ Verification successful
LGTM! The hook implementation is well-structured and properly used
The verification confirms:
- The hook implementation follows the standard pattern using
useDataManagerBase - It's properly integrated with the database context
- The hook is actively used across multiple components in both GI and SR implementations
- Usage is consistent, primarily for managing TC build data
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the useBuildTc hook implementation and its dependencies
# Check if the hook file exists and examine its implementation
fd -e ts "useBuildTc.ts" -x cat {}
# Look for any direct dependencies on this hook
rg "useBuildTc" --type ts -g '!**/index.ts'
# Verify integration with the database context (mentioned in AI summary)
rg "useDatabaseContext.*buildTc" --type ts
Length of output: 1915
libs/sr/ui/src/LightCone/index.tsx (1)
1-1: LGTM! The new export follows the established pattern.
The addition of LightConeAutocomplete to the barrel exports is consistent with the module's organization and supports the PR's UI enhancement objectives.
Let's verify the module's existence and exports:
✅ Verification successful
The export is correctly implemented and the module exists
The verification confirms that:
- The
LightConeAutocomplete.tsxfile exists in the correct location - It exports a default React component with the expected functionality
- The barrel file correctly re-exports it
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the LightConeAutocomplete module exists and exports correctly
# Check if the module file exists
fd -t f "LightConeAutocomplete.tsx" "libs/sr/ui/src/LightCone"
# Check for named exports in the module
rg -t tsx "export .*LightConeAutocomplete" "libs/sr/ui/src/LightCone"
Length of output: 209
Script:
#!/bin/bash
# Let's try with TypeScript file extension and without file type restriction
# Check if the module exists
fd "LightConeAutocomplete.tsx" "libs/sr/ui/src/LightCone"
# Check exports without file type restriction
rg "export.*LightConeAutocomplete" "libs/sr/ui/src/LightCone"
# Also check the content of the file to understand the exports
cat "libs/sr/ui/src/LightCone/LightConeAutocomplete.tsx"
Length of output: 2078
libs/common/ui/src/hooks/index.ts (1)
5-5: LGTM! The export follows established patterns.
The new export maintains alphabetical ordering and follows the consistent pattern of the barrel file.
Let's verify the implementation of the new hook:
✅ Verification successful
Let me verify the implementation of isOverflowedX and isOverflowedY functions since they are used in the hook but not visible in the current output.
Let me try a different pattern to find these helper functions as they might be defined in a different way.
Implementation verified and follows best practices
The hook is well-implemented with:
- Proper cleanup of event listeners
- Helper functions
isOverflowedXandisOverflowedYusing standard DOM properties - Correct usage of React hooks (useRef, useState, useEffect)
- Type safety with HTMLElement
- Efficient overflow detection logic
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the implementation of useRefOverflow hook
# Expected: Find the hook implementation and its usage in the codebase
# Check the hook implementation
echo "Checking hook implementation:"
ast-grep --pattern 'export function useRefOverflow() {
$$$
}'
# Check for hook usage to ensure it's being utilized
echo -e "\nChecking hook usage:"
rg -l "useRefOverflow"
Length of output: 1884
Script:
#!/bin/bash
# Check for isOverflowedX and isOverflowedY implementations
echo "Checking overflow helper functions:"
ast-grep --pattern 'function isOverflowed$_($$$) {
$$$
}'
# Also check for any imports of these functions
echo -e "\nChecking for imports:"
rg "import.*isOverflow" libs/common/ui/src/hooks/useRefOverflow.tsx
Length of output: 271
Script:
#!/bin/bash
# Check the entire content of useRefOverflow.tsx to see the helper functions
echo "Checking full content of useRefOverflow.tsx:"
cat libs/common/ui/src/hooks/useRefOverflow.tsx
# Also check if these helpers might be imported from another file
echo -e "\nChecking for files containing isOverflow:"
rg -l "isOverflow" --type ts --type tsx
Length of output: 1218
libs/sr/db-ui/src/hooks/useBuildTc.ts (1)
3-6: Verify the integration with team composition feature.
The hook appears to be correctly implemented for managing build TCs, but let's verify its integration with the team composition feature mentioned in the PR objectives.
✅ Verification successful
Based on the shell script results, I can see that the useBuildTc hook is well integrated with the team composition feature. The evidence shows:
-
The hook is used in multiple team-related components:
libs/sr/page-team/src/TeamCalcProvider.tsxlibs/sr/page-team/src/BuildsDisplay.tsxlibs/gi/page-team/src/TeamSetting/TeamComponents.tsxlibs/gi/page-team/src/CharacterDisplay/Build/BuildTc.tsx
-
The database operations show proper integration:
- Creation of new build TCs:
database.buildTcs.new() - Following build TC changes:
database.buildTcs.follow() - Setting build TC data:
database.buildTcs.set() - Getting build TC data:
database.buildTcs.get()
- Creation of new build TCs:
The hook is properly integrated and used throughout the team composition feature.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the integration of useBuildTc with team composition related files
# Check for usages of useBuildTc in team composition related files
echo "Checking useBuildTc usage in team composition files..."
rg -l "useBuildTc" | grep -i "team"
# Check for buildTcs database operations
echo "Checking buildTcs database operations..."
ast-grep --pattern 'database.buildTcs'
Length of output: 62351
libs/sr/ui/src/Relic/index.tsx (1)
6-8: LGTM! Clean and consistent exports.
The new exports follow the established naming conventions and maintain proper organization within the module.
libs/sr/db/src/Interfaces/IBuildTc.ts (3)
4-4: LGTM: Import statements are properly structured
The new imports for RelicRarityKey and ILightCone are correctly added to support the new type definitions for TC builds.
Also applies to: 10-10
12-16: LGTM: BuildTcRelicSlot type properly extended
The addition of the rarity field enhances the type definition to support relic rarity specification in TC builds.
Let's verify the usage of this new field:
#!/bin/bash
# Check if the rarity field is consistently used across the codebase
rg -A 2 "BuildTcRelicSlot" 22-23: Consider adding runtime validation for rarity consistency
The interface changes look good, but consider adding runtime validation to ensure that the substat rarity matches the slot rarity for consistency.
Let's check for existing validation logic:
Also applies to: 29-29
libs/common/ui/src/hooks/useRefOverflow.tsx (1)
1-2: LGTM! Proper setup for client-side React hook
The directive and imports are correctly configured for a client-side React hook.
libs/sr/db/src/Database/DataManager.ts (1)
24-24: Verify data migration for the modified key pattern
The change from this.goKeySingle to ${this.goKeySingle}_ makes the key matching more specific. Ensure that:
- Existing data with keys not ending in underscore are properly migrated
- All code paths creating new keys follow this pattern
libs/sr/ui/src/Relic/RelicRarityDropdown.tsx (2)
13-14: LGTM! Props type definitions are well-structured.
The changes to make filter optional and add showNumber improve component flexibility while maintaining type safety.
Line range hint 1-61: Verify integration with TC builds system.
Since this component is part of the TC builds enhancement, let's verify its usage across the codebase.
libs/sr/ui/src/LightCone/LightConeAutocomplete.tsx (4)
1-14: LGTM! Well-structured imports and type definitions.
The imports are properly organized and the type definitions are clear and concise.
23-32: LGTM! Efficient options generation with proper memoization.
The useMemo implementation is correct and follows React best practices for performance optimization.
49-60: LGTM! Well-implemented rendering with proper loading states.
The component rendering is well-structured with appropriate loading states and proper prop passing.
34-48: Verify the stability of lightConeAsset function.
The toImg callback has an empty dependency array but uses the lightConeAsset function. While this might be intentional if lightConeAsset is stable, it's worth verifying.
✅ Verification successful
Let me check the implementation of lightCones to understand if it's a mutable object or a constant.
Let me check the actual assets file to see how the lightCones object is defined in the assets module.
Let me try to check the content of the generated assets directory.
Based on the evidence gathered, I can now provide a final response:
Empty dependency array in toImg callback is correct
The lightConeAsset function is used in a generated assets module where all the data is marked with as const. The function simply accesses static image paths from an immutable object, making it a stable reference that doesn't need to be included in the dependency array.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if lightConeAsset is a stable function or might change
rg -A 5 "export.*lightConeAsset"
Length of output: 411
Script:
#!/bin/bash
# Check the lightCones object definition
rg -A 5 "const.*lightCones.*="
Length of output: 3165
Script:
#!/bin/bash
# Find and check the content of the file that defines the lightCones object in assets
fd "lightCones" libs/sr/assets/src/ --exec cat {}
Length of output: 106
Script:
#!/bin/bash
# Check the content of the lightCones directory and any JSON/TS files within
fd "\.(json|ts)$" libs/sr/assets/src/gen/lightCones/ --exec cat {}
Length of output: 23963
libs/sr/ui/src/Relic/RelicMainStatDropdown.tsx (1)
1-12: LGTM! Clean and well-organized imports.
The imports are properly organized and all imported entities are utilized in the component.
libs/sr/ui/src/Character/CharacterCard.tsx (2)
16-16: LGTM! Clean import addition
The addition of charTagMapNodeEntries to the existing formula imports is well-organized.
56-57: Verify calculation consistency with the new data transformation
The switch to charTagMapNodeEntries for calculator initialization could affect stat calculations. Please ensure this change maintains consistent results with the previous implementation.
Let's verify the usage and test coverage:
✅ Verification successful
Let me gather more information about the implementation and test coverage.
Let me check the implementation of srCalculatorWithEntries and the data transformation to ensure consistency.
Based on the gathered information, I can now provide the final response:
The calculator initialization change is safe and well-tested
The implementation shows that:
charTagMapNodeEntriesis extensively tested informula.test.tswith various test cases covering base stats calculations- The function is consistently used across multiple components (CharacterCard, TeamCalcProvider)
srCalculatorWithEntriesis a thin wrapper that creates a Calculator instance with compiled values, maintaining calculation consistency
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check test coverage and usage of charTagMapNodeEntries
# Check for test cases covering the new transformation
rg -A 5 "charTagMapNodeEntries" --type ts --type tsx
# Look for test files that verify calculation results
fd -e test.ts -e test.tsx | xargs rg -l "srCalculatorWithEntries|charTagMapNodeEntries"
Length of output: 206
Script:
#!/bin/bash
# Check the implementation of charTagMapNodeEntries and its test coverage
rg -p "charTagMapNodeEntries" --type=ts
# Check the test file content
cat libs/sr/formula/src/formula.test.ts
# Look for calculator-related tests
rg -p "srCalculatorWithEntries" --type=ts
Length of output: 6701
Script:
#!/bin/bash
# Check the implementation of srCalculatorWithEntries and related functions
ast-grep --pattern 'export function srCalculatorWithEntries($_) {
$$$
}'
# Check the implementation of charTagMapNodeEntries
ast-grep --pattern 'export function charTagMapNodeEntries($_) {
$$$
}'
# Look for any related test assertions
rg -p "expect\(.*calculator" --type=ts
Length of output: 582
libs/sr/formula/src/util.ts (3)
2-9: LGTM! Well-organized type imports.
The new type imports are properly structured and provide strong typing for the game-specific keys needed for TC builds.
Line range hint 83-106: Great performance optimization with opt-in artifact buffs!
The implementation shows excellent attention to performance by:
- Using opt-in for artifact buffs to reduce 'read' traffic
- Smart separation of stats and sets processing
- Efficient use of Partial for flexible input handling
68-81: Verify light cone parameter constraints.
The function's new signature with explicit parameters improves type safety. However, we should verify that the parameters have appropriate constraints.
✅ Verification successful
Parameter constraints are properly enforced through validation
The codebase has comprehensive validation for all light cone parameters:
- Level is validated to be between 1 and 80 (lightConeMaxLevel)
- Ascension is validated to be between 0 and 6 (allAscensionKeys)
- Superimpose is validated to be between 1 and 5 (allSuperimposeKeys)
These validations are enforced through the validateLevelAsc function and type constraints, ensuring the parameters passed to lightConeTagMapNodeEntries are always within valid ranges.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for parameter validation in related files
rg -A 5 "type (AscensionKey|SuperimposeKey)" --type typescript
Length of output: 99
Script:
#!/bin/bash
# Search for type definitions and usage patterns
rg -g '*.ts' -g '*.tsx' "type (AscensionKey|SuperimposeKey|LightConeKey)"
# Look for any validation or constraint checks
rg -g '*.ts' -g '*.tsx' -B 2 -A 2 "(ascension|superimpose|level).*(check|valid|constraint)"
# Find light cone related type definitions
ast-grep --pattern 'type $_Key = $$$'
Length of output: 12014
Script:
#!/bin/bash
# Search for validateLevelAsc implementation
rg -g '*.ts' -g '*.tsx' -B 5 -A 5 "function validateLevelAsc"
# Look for light cone specific constants and limits
rg -g '*.ts' -g '*.tsx' "lightConeMaxLevel|allSuperimposeKeys|allAscensionKeys" -B 2 -A 2
Length of output: 4796
libs/sr/formula/src/formula.test.ts (1)
14-18: LGTM: Improved data handling structure
The updated imports provide better granularity and explicit data management through dedicated entry point functions.
libs/sr/ui/src/LightCone/LightConeCard.tsx (2)
11-11: LGTM: Import change aligns with the architectural improvements
The replacement of lightConeData with lightConeTagMapNodeEntries reflects a more structured approach to handling light cone data.
59-63: Verify calculation changes maintain backward compatibility
The refactoring to use individual parameters (key, level, ascension, superimpose) instead of passing the entire lightCone object is a good improvement for clarity and performance. However, let's verify the changes:
Consider adding JSDoc comments to document the expected parameter types and return value of lightConeTagMapNodeEntries to improve maintainability.
✅ Verification successful
Let me gather more information about the lightConeTagMapNodeEntries function to ensure the refactoring maintains backward compatibility.
Let me search for the implementation of lightConeTagMapNodeEntries to complete the verification.
Refactoring maintains backward compatibility
The changes are safe and maintain backward compatibility for the following reasons:
- The
lightConeTagMapNodeEntriesfunction inlibs/sr/formula/src/util.tsis already designed to accept individual parameters (key,level,ascension,superimpose), matching the ILightCone interface exactly - All usages across the codebase (LightConeCard, LightConeCardCompact, TeamCalcProvider) consistently use these individual parameters
- The parameters match the exact properties defined in the ILightCone interface in
libs/sr/srod/src/ILightCone.ts - No instances of passing the full lightCone object to this function were found in the codebase
The refactoring improves code clarity by making parameter dependencies explicit without breaking any existing functionality.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any other usages of lightConeData to ensure consistent refactoring
# Search for any remaining usages of lightConeData
echo "Checking for remaining lightConeData usages:"
rg "lightConeData" -A 3
# Search for new function usage pattern
echo "Checking new function usage pattern:"
rg "lightConeTagMapNodeEntries.*key.*level.*ascension.*superimpose" -A 3
Length of output: 2700
Script:
#!/bin/bash
# Search for the implementation and usage of lightConeTagMapNodeEntries
ast-grep --pattern 'function lightConeTagMapNodeEntries'
# Search for any previous implementation that might have used the full lightCone object
rg "lightConeTagMapNodeEntries.*lightCone" -A 3
# Check the interface definition for ILightCone
rg "interface ILightCone" -A 10
Length of output: 585
Script:
#!/bin/bash
# Search for the implementation of lightConeTagMapNodeEntries with more context
rg "lightConeTagMapNodeEntries" -A 5 -B 5
# Also check for any imports or exports related to this function
rg "export.*lightConeTagMapNodeEntries|import.*lightConeTagMapNodeEntries" -A 2
Length of output: 8090
libs/sr/ui/src/LightCone/LightConeCardCompact.tsx (2)
74-79: LGTM! Good performance optimizations.
The implementation shows good attention to performance:
- Proper memoization of expensive calculations
- Correct use of useCallback for event handlers
- Smart handling of image optimization with NextImage
Also applies to: 219-226
16-16: LGTM! Verify the data handling refactor.
The switch to lightConeTagMapNodeEntries and the memoized calculation setup look good. The implementation correctly handles all necessary parameters (key, level, ascension, superimpose).
Also applies to: 73-79
✅ Verification successful
Based on the gathered information, I can now provide the final response:
The data handling refactor is consistent and complete
The switch from lightConeData to lightConeTagMapNodeEntries is a clean refactor:
- The old
lightConeDatais only used in the stats generator (gen-stats/executor.ts), which is the source of truth - The new
lightConeTagMapNodeEntriesfunction informula/src/util.tsproperly handles all required parameters (key, level, ascension, superimpose) - All component usages (
LightConeCard,LightConeCardCompact,TeamCalcProvider) consistently use the new function
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify the consistency of the data handling refactor
# Look for any remaining uses of the old lightConeData
rg "lightConeData" --type typescript
# Check for consistent parameter usage across the codebase
ast-grep --pattern 'lightConeTagMapNodeEntries($key, $level, $ascension, $superimpose)'
Length of output: 161
Script:
#!/bin/bash
# Search for lightConeData pattern in all files
rg "lightConeData"
# Search for lightConeTagMapNodeEntries pattern in all files
rg "lightConeTagMapNodeEntries"
# Look for any TypeScript/JavaScript files containing lightCone in their name
fd -e ts -e tsx -e js -e jsx . | grep -i "lightcone"
Length of output: 22457
Script:
#!/bin/bash
# Check the implementation of lightConeTagMapNodeEntries function
rg -A 10 "export function lightConeTagMapNodeEntries"
# Look for the usage pattern of lightConeTagMapNodeEntries
ast-grep --pattern 'lightConeTagMapNodeEntries($key, $level, $ascension, $superimpose)'
# Check if there are any remaining references to the old data structure
rg -A 5 "import.*lightConeData"
Length of output: 1457
libs/sr/db/src/Database/DataManagers/BuildTcDataManager.ts (1)
44-45: Consider implementing post-initialization validation for teamId
The current approach of disabling teamId validation during database initialization could allow invalid teamIds to persist. Consider implementing a post-initialization validation mechanism or a database ready event to ensure data integrity.
libs/sr/ui/src/Relic/RelicEditor/SubstatInput.tsx (3)
35-35: LGTM: Clean import replacement
The replacement of RelicStatWithUnit with StatDisplay maintains proper module organization.
97-98: LGTM: Proper StatDisplay configuration
Good use of showPercent and disableIcon props. The icon is correctly handled via the DropdownButton's startIcon prop instead.
126-126: LGTM: Consistent StatDisplay usage
Maintains UI consistency by using the same StatDisplay configuration as in the DropdownButton.
libs/sr/ui/src/Relic/RelicEditor/RelicEditor.tsx (1)
21-21: LGTM! Import restructuring improves modularity
The separation of component imports into individual paths and removal of unused imports enhances code organization and maintainability.
Also applies to: 60-62
libs/sr/page-team/src/TeamCalcProvider.tsx (1)
135-139:
Include buildTc in the dependency array of useMemo
Similarly, in the relicTagEntries useMemo hook, you're referencing buildTc within the function body but only include buildTc, meta?.buildType, and relics in the dependency array. To ensure that changes to buildTc.relic trigger a recalculation, include buildTc in the dependency array.
Suggested Change:
- }, [buildTc, meta?.buildType, relics])
+ }, [meta?.buildType, buildTc, relics])Likely invalid or redundant comment.
libs/sr/page-team/src/TeammateDisplay.tsx (2)
1-4: Imports are correctly updated
The addition of useForceUpdate from @genshin-optimizer/common/react-util is appropriate.
68-68: Conditional rendering of 'Optimize' component is appropriate
Hiding the Optimize component when buildType is 'tc' aligns with the PR objectives and ensures that optimization features are disabled for TC builds as intended.
libs/sr/page-team/src/BuildsDisplay.tsx (2)
245-249: Button should be disabled based on active prop
Ensure the Set Active button correctly reflects the build's activation state by disabling it when active is true.
Please verify that the disabled prop is correctly set on the Button component:
<Button
size="small"
color="success"
onClick={onActive}
disabled={active}
>
Set Active
</Button>582-636: Ensure controlled inputs for relic slot editor
In the SlotEditor component, confirm that the NumberInputLazy component properly controls and validates the level input to prevent invalid values.
Please verify that NumberInputLazy enforces min and max constraints and correctly handles user input for level.
There was a problem hiding this comment.
Actionable comments posted: 19
🧹 Outside diff range and nitpick comments (46)
libs/common/ui/src/hooks/useRefSize.tsx (2)
Line range hint
4-8: Improve type safety with proper initializationThe ref is typed as
HTMLElementbut initialized asundefined. This could lead to type-related issues.Apply this diff to improve type safety:
- const ref = useRef<HTMLElement>() + const ref = useRef<HTMLElement | null>(null)
Line range hint
9-19: Consider using ResizeObserver for better performanceUsing a window resize event listener can impact performance as it triggers for all resize events. ResizeObserver provides a more efficient way to observe size changes specifically for the target element.
Here's how to implement it:
useEffect(() => { const handleResize = () => { setWidth(ref.current?.clientWidth ?? 0) setHeight(ref.current?.clientHeight ?? 0) } - handleResize() // Check on mount and whenever the window resizes - if (ref.current) window.addEventListener('resize', handleResize) - return () => { - window.removeEventListener('resize', handleResize) - } + handleResize() // Initial size check + + if (ref.current) { + const resizeObserver = new ResizeObserver(handleResize) + resizeObserver.observe(ref.current) + return () => resizeObserver.disconnect() + } }, [])This change will:
- Only trigger when the observed element's size changes
- Automatically handle cleanup when the element is removed
- Reduce unnecessary calculations on window resize
libs/sr/db/src/Interfaces/IBuildTc.ts (2)
17-17: LGTM: Well-defined type for TC-specific light conesGood use of TypeScript's
Omitutility type to create a TC-specific light cone type without location tracking.Consider documenting why
locationandlockare omitted for TC builds, perhaps in a comment above the type definition, to help future maintainers understand the design decision.
Line range hint
18-37: LGTM: Well-structured interface with room for future expansionThe interface is well-organized and includes all necessary components for TC builds. The optional
teamIdand optimization settings provide good flexibility.Consider adding:
- A version field to support future interface evolution
- JSDoc comments to document the purpose of each major section (relic, optimization)
- Validation constraints (e.g., min/max values) as TypeScript types for numeric fields
Example:
/** Interface for Team Composition builds */ export interface IBuildTc { /** Version for interface compatibility */ version: number; // ... rest of the interface }libs/common/ui/src/hooks/useRefOverflow.tsx (3)
4-8: Improve type safety for useRefThe current type declaration could be more explicit about the initial undefined state.
Consider updating the ref declaration:
- const ref = useRef<HTMLElement>() + const ref = useRef<HTMLElement | undefined>(undefined)
24-30: Add JSDoc documentation for helper functionsThe helper functions would benefit from documentation explaining their purpose and parameters.
Consider adding JSDoc comments:
+/** + * Checks if the element has horizontal overflow + * @param ref - The HTML element to check + * @returns true if the element's content width exceeds its client width + */ function isOverflowedX(ref: HTMLElement) { return ref.scrollWidth > ref.clientWidth } +/** + * Checks if the element has vertical overflow + * @param ref - The HTML element to check + * @returns true if the element's content height exceeds its client height + */ function isOverflowedY(ref: HTMLElement) { return ref.scrollHeight > ref.clientHeight }
1-30: Consider performance optimizations for resize handlingSince this hook will be used in TC builds UI, frequent resize events could impact performance, especially with multiple instances.
Consider these optimizations:
- Debounce the resize handler to reduce the frequency of updates
- Use ResizeObserver instead of window resize events for more precise element-specific monitoring
- Consider adding a cleanup threshold to remove listeners when the component is not visible
Example implementation of debouncing:
import { debounce } from 'lodash-es' // or implement your own debounce const debouncedHandleResize = debounce(handleResize, 150) window.addEventListener('resize', debouncedHandleResize) return () => { debouncedHandleResize.cancel() window.removeEventListener('resize', debouncedHandleResize) }libs/sr/db/src/Database/DataManager.ts (2)
Line range hint
24-28: Add defensive programming for key removalThe current implementation might silently remove data. Consider adding logging and validation before removal.
if ( key.startsWith(`${this.goKeySingle}_`) && !this.set(this.toCacheKey(key), {}) ) { + console.warn(`Removing orphaned key from storage: ${key}`); this.database.storage.remove(key) }
Line range hint
1-48: Consider documenting TC build data flowGiven this class's central role in managing TC build data (as per PR objectives), consider adding documentation about:
- The relationship between
DataManagerand TC builds- The key structure for TC build data
- Data flow between UI components and storage
This will help future maintainers understand how TC builds are persisted and managed.
libs/sr/ui/src/LightCone/LightConeAutocomplete.tsx (2)
15-21: Simplify label assignment using nullish coalescing.The label assignment can be simplified for better readability.
- label = label ? label : t('editor.lightConeName') + label = label ?? t('editor.lightConeName')
38-48: Consider making the icon size configurable.The icon size is currently hardcoded to 2. Consider making it configurable through props for better reusability.
type LightConeAutocompleteProps = { lcKey: LightConeKey | '' setLCKey: (key: LightConeKey | '') => void label?: string + iconSize?: number } export default function LightConeAutocomplete({ lcKey, setLCKey, label = '', + iconSize = 2, }: LightConeAutocompleteProps) { // ... const toImg = useCallback( (key: LightConeKey | '') => !key ? undefined : ( <ImgIcon - size={2} + size={iconSize} sx={{ width: 'auto' }} src={lightConeAsset(key, 'cover')} /> ), - [] + [iconSize] )libs/sr/ui/src/Relic/RelicMainStatDropdown.tsx (2)
13-25: Add JSDoc documentation for better maintainability.Consider adding JSDoc documentation to describe the component's purpose and its props. This would improve maintainability and make it easier for other developers to understand the component's usage.
+/** + * A dropdown component for selecting relic main stats. + * @param {RelicMainStatKey} [statKey] - The currently selected stat key + * @param {RelicSlotKey} slotKey - The type of relic slot (e.g., 'head', 'hands') + * @param {function} setStatKey - Callback function to update the selected stat + * @param {ReactNode} [defText] - Default text to display when no stat is selected + * @param {DropdownButtonProps} [dropdownButtonProps] - Additional props for the dropdown button + */ export function RelicMainStatDropdown({
35-63: Enhance accessibility and optimize the dropdown implementation.
- Add accessibility attributes for better screen reader support
- Consider removing redundant disabled state since the item is already marked as selected
<DropdownButton + aria-label="Select main stat" startIcon={statKey ? <StatIcon statKey={statKey} /> : undefined} title={ <Box> {statKey ? ( <StatDisplay statKey={statKey} showPercent disableIcon /> ) : ( defText )} </Box> } {...dropdownButtonProps} > {relicSlotToMainStatKeys[slotKey].map((mk) => ( <MenuItem key={mk} selected={statKey === mk} - disabled={statKey === mk} onClick={() => setStatKey(mk)} + aria-selected={statKey === mk} >libs/sr/ui/src/Character/CharacterCard.tsx (2)
56-57: Consider adding error handling for calculator initialization.While the nullish coalescing operator handles the undefined case, consider adding error boundaries or fallback UI for cases where calculator initialization fails.
- const calc = - useSrCalcContext() ?? - srCalculatorWithEntries(charTagMapNodeEntries(character)) + const calc = useMemo(() => { + try { + return useSrCalcContext() ?? srCalculatorWithEntries(charTagMapNodeEntries(character)) + } catch (error) { + console.error('Failed to initialize calculator:', error) + return null // Or a default calculator instance + } + }, [character]) + + if (!calc) { + return <ErrorFallback message="Failed to load character stats" /> + }
Line range hint
31-41: Consider performance optimizations for stat calculations.The current implementation recalculates stats on every render. Consider these optimizations:
- Memoize the stats calculations
- Use a virtual list for large stat collections
+const StatLine = memo(function StatLine({ characterKey, calc, statKey, }: { characterKey: CharacterKey calc: Calculator statKey: (typeof stats)[number] }) { + const value = useMemo(() => { + const computed = calc + .withTag({ src: characterKey, dst: characterKey }) + .compute(own.final[statKey]).val + return toPercent(computed, statKey).toFixed(statToFixed(statKey)) + getUnitStr(statKey) + }, [calc, characterKey, statKey]) return ( <Typography sx={{ display: 'flex', alignItems: 'center', gap: 1, justifyContent: 'space-between', }} > <StatDisplay statKey={statKey} /> - <span> - {toPercent( - calc - .withTag({ src: characterKey, dst: characterKey }) - .compute(own.final[statKey]).val, - statKey - ).toFixed(statToFixed(statKey))} - {getUnitStr(statKey)} - </span> + <span>{value}</span> </Typography> ) -} +})Also applies to: 123-146
libs/sr/formula/src/util.ts (4)
Line range hint
38-67: Consider documenting default character statsThe implementation looks good, but the default character stats (5% crit rate, 50% crit damage) should be documented to explain their significance:
// Default char + // Base stats that all characters have by default: + // - 5% base crit rate + // - 50% base crit damage ownBuff.premod.crit_.add(0.05), ownBuff.premod.crit_dmg_.add(0.5),
68-81: Enhance comment clarity for light cone usage trackingThe implementation looks good, but consider making the comment more descriptive:
- // Mark light cones as used + // Increment the usage counter for this light cone type to track equipped/used light cones own.common.count.sheet(key).add(1),
Line range hint
109-171: Consider enhancing documentation for non-stacking mechanicsWhile the implementation is solid and the CAUTION note about formula limitations is valuable, the non-stacking buff logic could benefit from more detailed documentation. Consider adding a comment block explaining:
- The purpose of using
i + 1for priority- How the stacking mechanism works
- Examples of when this is applied
// Non-stacking + // Handles buffs that don't stack between team members: + // - Uses index + 1 as priority to ensure 0 means no buff + // - stackTmp tracks temporary stack state + // - stackOut determines final buff application based on priority members.flatMap((src, i) => {
Based on the search results, I can generate the final response as there are clear issues that need to be addressed:
Update required: Found old function names still in use
The codebase still contains calls to old function names that need to be migrated:
charData()is still used in:
apps/somnia/src/commands/go/calculator.tsapps/gi-frontend/src/app/teams/[teamId]/CalcWrapper.tsxapps/gi-frontend/src/app/characters/CharacterCard.tsxlibs/gi/formula/src/util.tslibs/gi/formula/src/example.test.tsrelicsData()is still used inlibs/sr/page-team/src/TeamCalcProvider.tsxThese calls need to be updated to use the new function signatures:
charData()→charTagMapNodeEntries()relicsData()→relicTagMapNodeEntries()🔗 Analysis chain
Line range hint
38-107: Verify function signature changes across the codebaseThe significant changes to function signatures (
charTagMapNodeEntries,lightConeTagMapNodeEntries,relicTagMapNodeEntries) require verification of all calling code.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for old function names to ensure complete migration echo "Checking for any remaining old function names..." rg "charData\(" || echo "No occurrences of old charData function" rg "lightConeData\(" || echo "No occurrences of old lightConeData function" rg "relicsData\(" || echo "No occurrences of old relicsData function" # Search for new function usage patterns to verify correct parameter passing echo -e "\nChecking new function usage patterns..." rg -A 2 "charTagMapNodeEntries\(" rg -A 2 "lightConeTagMapNodeEntries\(" rg -A 2 "relicTagMapNodeEntries\("Length of output: 4587
libs/sr/formula/src/formula.test.ts (3)
Line range hint
78-90: Consider parameterizing character level in Light Cone testsThe character level and ascension are hardcoded to 1 and 0 in Light Cone tests. Consider parameterizing these values to ensure Light Cone calculations are correct across different character levels.
- level: 1, - ascension: 0, + level: lvl, // Use the same test parameters + ascension: ascension as AscensionKey,
Line range hint
114-126: Enhance combined character + Light Cone test coverageThe current test only verifies:
- A single level/ascension combination
- Only the attack stat
Consider:
- Using test.each with multiple level combinations
- Verifying all stats (ATK, DEF, HP) like in individual tests
- Testing edge cases (max level, different ascensions)
- it('calculate base stats', () => { + it.each([ + [1, 0, 1, 0, 81.6, 90.0, 182.4], // [charLvl, charAsc, lcLvl, lcAsc, expectedAtk, expectedDef, expectedHp] + [20, 1, 20, 1, 236.28, 243.9, 532.32], + [80, 6, 80, 6, 584.28, 633.9, 1252.32], + ])('calculate combined stats at char lvl %i asc %i, LC lvl %i asc %i', + (charLvl, charAsc, lcLvl, lcAsc, expectedAtk, expectedDef, expectedHp) => { const charKey: CharacterKey = 'March7th' const lcKey: LightConeKey = 'Amber' const data: TagMapNodeEntries = [ ...withMember( 'March7th', ...charTagMapNodeEntries({ - level: 1, - ascension: 0, + level: charLvl, + ascension: charAsc as AscensionKey, key: charKey, eidolon: 0, basic: 0, skill: 0, ult: 0, talent: 0, bonusAbilities: {}, statBoosts: {}, }), - ...lightConeTagMapNodeEntries(lcKey, 1, 0, 1) + ...lightConeTagMapNodeEntries(lcKey, lcLvl, lcAsc as AscensionKey, 1) ), ] const calc = new Calculator(keys, values, compileTagMapValues(keys, data)) const m7 = convert(ownTag, { et: 'own', src: 'March7th' }) - expect(calc.compute(m7.final.atk).val).toBeCloseTo(81.6) + expect(calc.compute(m7.final.atk).val).toBeCloseTo(expectedAtk) + expect(calc.compute(m7.final.def).val).toBeCloseTo(expectedDef) + expect(calc.compute(m7.final.hp).val).toBeCloseTo(expectedHp) })
Additional test coverage needed for character ability parameters
The test cases only use default values (0) for the new parameters (eidolon, basic, skill, ult, talent). Given that these parameters are extensively used in character formulas and calculations as shown in the codebase, test coverage should be expanded to include:
- Non-zero values for ability levels (basic, skill, ult, talent)
- Different eidolon levels that affect ability scaling
- Various combinations of bonusAbilities and statBoosts
🔗 Analysis chain
Line range hint
44-52: Verify test coverage for new character parametersWhile the test cases cover level and ascension variations, they don't verify the impact of the newly added parameters (eidolon, basic, skill, ult, talent, bonusAbilities, statBoosts).
Let's analyze the test coverage for the new parameters. From the search results, I can see:
- The test file
formula.test.tscontains test cases that set all the new parameters (eidolon, basic, skill, ult, talent) to 0- The codebase extensively uses these parameters in character sheets and formulas
- The
util.tsfile shows these parameters are used in core calculationsLet me verify if there are any test cases with non-zero values for these parameters.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other test files that might cover these parameters rg -t ts "eidolon|basic|skill|ult|talent|bonusAbilities|statBoosts" "libs/sr/formula/src/**/*test*"Length of output: 171
Script:
#!/bin/bash # Let's first find the test files in the formula directory fd ".*test.*\.ts$" libs/sr/formula/src/ # Then search for test coverage of these parameters rg -t ts "eidolon|basic|skill|ult|talent|bonusAbilities|statBoosts" libs/sr/formula/src/Length of output: 101168
Script:
#!/bin/bash # Search for test cases with non-zero values for these parameters rg -A 5 -B 5 "eidolon:|basic:|skill:|ult:|talent:" libs/sr/formula/src/formula.test.tsLength of output: 1165
libs/sr/ui/src/LightCone/LightConeCard.tsx (1)
Line range hint
68-76: Consider enhancing error handling in the Suspense boundary.While the Suspense fallback is implemented, consider adding an error boundary to gracefully handle rendering failures.
+ import { ErrorBoundary } from '@genshin-optimizer/common/ui' + <ErrorBoundary fallback={<Typography color="error">Failed to load Light Cone</Typography>}> <Suspense fallback={ <Skeleton variant="rectangular" sx={{ width: '100%', height: '100%', minHeight: 350 }} /> } > + </ErrorBoundary>libs/sr/ui/src/LightCone/LightConeCardCompact.tsx (2)
90-180: Consider simplifying the nested Box structureWhile the layout achieves the desired visual result, the multiple nested Box components could be simplified to improve maintainability and performance.
Consider consolidating the nested Box components at lines 91-98 and 110-114 into a single component with combined styles. For example:
-<Box - sx={(theme) => ({ - display: 'flex', - height: '100%', - width: '100%', - borderLeft: '5px solid', - borderImage: `${theme.palette[`grad${rarity}`].gradient} 1`, - })} -> - <Box - sx={{ - position: 'relative', - flexGrow: 1, - display: 'flex', - flexDirection: 'column', - alignItems: 'center', - justifyContent: 'center', - }} - > +<Box + sx={(theme) => ({ + display: 'flex', + height: '100%', + width: '100%', + borderLeft: '5px solid', + borderImage: `${theme.palette[`grad${rarity}`].gradient} 1`, + position: 'relative', + flexGrow: 1, + flexDirection: 'column', + alignItems: 'center', + justifyContent: 'center', + })} +>
219-247: Consider extracting shared wrapper logicThe
actionWrapperFuncand dimension styles are duplicated betweenLightConeCardCompactObjandLightConeCardCompactEmpty. Consider extracting these into shared utilities.Create a shared hook for the wrapper logic:
function useActionWrapper(onClick?: () => void) { return useCallback( (children: ReactNode) => ( <CardActionArea onClick={onClick} sx={{ height: '100%', width: '100%' }}> {children} </CardActionArea> ), [onClick] ) }And a shared style constant:
const cardBaseStyles = { height: COMPACT_ELE_HEIGHT, width: COMPACT_ELE_WIDTH, }libs/sr/db/src/Database/DataManagers/BuildTcDataManager.ts (2)
102-102: Document the rationale for default rarity valueThe default rarity of 5 is used consistently but lacks documentation explaining why this value was chosen. Consider adding a comment explaining the significance of this default value in the context of the game mechanics.
Also applies to: 120-120
164-164: TODO: Implement set validation logicThe comment indicates missing validation for relic sets. This could lead to invalid set combinations being stored.
Would you like me to help implement the set validation logic or create a GitHub issue to track this task?
libs/sr/ui/src/Relic/RelicEditor/SubstatInput.tsx (2)
Line range hint
164-178: Consider cleaning up commented code blocksThere are several blocks of commented-out code that should be removed if they're no longer needed. If they're being kept for reference during the UI transition, consider:
- Moving the reference code to documentation
- Creating separate reference files
- Using version control history for reference instead
This will improve code maintainability and readability.
Also applies to: 207-220, 241-251
37-37: Address TODO: Implement missing validationThe TODO comment indicates several missing validations:
- Roll table validation
- Roll values validation
- Efficiency calculation
- Display text
- Icons
These validations are important for ensuring data integrity and proper UI feedback.
Would you like me to help create a comprehensive validation implementation or create a GitHub issue to track these requirements?
libs/sr/ui/src/Relic/RelicEditor/RelicEditor.tsx (1)
360-369: Consider enhancing type safety and error handlingThe refactoring to use
RelicMainStatDropdownimproves code organization. However, consider these enhancements:
- Add TypeScript interfaces to enforce TC-specific constraints
- Wrap the component with an error boundary to gracefully handle potential rendering failures
Example type definition:
interface RelicMainStatDropdownProps { slotKey: RelicSlotKey; statKey: StatKey | undefined; setStatKey: (key: StatKey) => void; defText: string; dropdownButtonProps?: Partial<DropdownButtonProps>; tcConstraints?: { allowedStats?: StatKey[]; minValue?: number; maxValue?: number; }; }libs/sr/ui/src/LightCone/LightConeEditor/LightConeEditor.tsx (2)
73-87: Use appropriate icons for action buttonsThe "Save" and "Add" buttons both use the
<Add />icon, which may confuse users. Differentiate the actions by using suitable icons.For example, use the
<Save />icon for the "Save" button:-<Button - startIcon={<Add />} +<Button + startIcon={<Save />} onClick={...} > {t('editor.btnSave')} </Button>
119-120: Redundantdisabledprop in Delete buttonThe
disabledprop in the "Delete" button checks!dbLightCone?.id, but the button is only rendered whendbLightCone?.idis truthy. This check is redundant.Remove the unnecessary
disabledprop:<Button startIcon={<DeleteForever />} onClick={...} - disabled={!dbLightCone?.id} color="error" sx={{ top: '2px' }} >libs/sr/page-team/src/TeamCalcProvider.tsx (3)
126-126: Avoid unnecessary type assertionIn line 126, the type assertion
(buildTc?.lightCone as ILightCone)may be unnecessary and could potentially mask type issues. SincebuildTc?.lightConeshould already be of typeILightCone | undefined, consider removing the type assertion to ensure proper type checking.
150-179: Refactor to reduce code duplication betweenrelicsDataandrelicTcDataThe functions
relicsDataandrelicTcDatahave similar logic for aggregating relic stats and sets. To improve maintainability and reduce duplication, consider creating a generalized function that both can utilize.Here's how you might refactor the code:
function aggregateRelicData( stats: Record<RelicMainStatKey | RelicSubStatKey, number>, sets: Partial<Record<RelicSetKey, number>> ): TagMapNodeEntries { return relicTagMapNodeEntries(stats, sets) }Then modify
relicsDataandrelicTcDatato use this function accordingly.
168-171: Improve variable naming for clarityThe destructuring of
relicassignssubstats: { stats: substats }, which can be confusing due to the repeated use ofsubstats. For better readability, consider renaming the innerstatsvariable:Apply this change to enhance clarity:
const { slots, - substats: { stats: substats }, + substats: { stats: substatValues }, sets, } = relic const stats = { ...substatValues } as Record< RelicMainStatKey | RelicSubStatKey, number >libs/sr/page-team/src/TeammateDisplay.tsx (1)
118-122: Redundant build type check inBuildTCDisplayThe condition
if (teammateDatum.buildType !== 'tc') return nullis unnecessary sinceBuildTCDisplayis only rendered whenbuildType === 'tc'in the parent component.Apply this diff to simplify the function:
function BuildTCDisplay() { const { teammateDatum } = useTeamContext() - if (teammateDatum.buildType !== 'tc') return null return <EquipRowTC buildTcId={teammateDatum.buildTcId} /> }libs/sr/ui/src/Relic/RelicCardCompact.tsx (2)
276-283: Refactor duplicatedactionWrapperFuncto adhere to DRY principleThe
actionWrapperFuncis defined identically inRelicSubCard,RelicSetCardCompact, andRelicMainsCardCompact. To reduce code duplication and improve maintainability, consider extracting this function into a shared utility or higher-order component.Also applies to: 335-342, 384-391
362-363: Internationalization: Replace hard-coded string with translation functionThe hard-coded string
"No Relic sets"should be replaced with a translation function to support localization and internationalization.Apply this diff:
{!Object.keys(sets).length && ( - <Typography>No Relic sets</Typography> + <Typography>{t('noRelicSets')}</Typography> )}I can assist with adding the necessary translation keys if needed.
libs/sr/page-team/src/BuildsDisplay.tsx (8)
82-105: Consider memoizingbuildIdsandbuildTcIdswithout dependency on state updates.The current implementation uses
useMemowithdbDirtyanddbTCDirtyto recomputebuildIdsandbuildTcIds. However, sincedbDirtyanddbTCDirtyare state hooks that trigger re-renders, the memoization may not be effective.You can simplify the code by removing
useMemowrappers and directly assigning the values, as React will re-render when state updates occur.-const buildIds = useMemo( - () => dbDirty && database.builds.getBuildIds(characterKey), - [dbDirty, database, characterKey] -) +const buildIds = database.builds.getBuildIds(characterKey) -const buildTcIds = useMemo( - () => dbTCDirty && database.buildTcs.getBuildTcIds(characterKey), - [dbTCDirty, database, characterKey] -) +const buildTcIds = database.buildTcs.getBuildTcIds(characterKey)
156-159: Simplify theactivecomputation for readability.The nested ternary operators make the
activevalue hard to read and maintain.Consider refactoring the
activecomputation to improve clarity:- active = - buildType === 'equipped' - ? buildType === newBuildType - : buildType === 'real' - ? buildId === newBuildId - : buildType === 'tc' && buildTcId === newBuildId + let active = false + if (buildType === 'equipped') { + active = newBuildType === 'equipped' + } else if (buildType === 'real') { + active = buildId === newBuildId + } else if (buildType === 'tc') { + active = buildTcId === newBuildId + }
226-230: Remove unusedonEditcallback or implement its functionality.The
onEditcallback is currently an empty function, which may cause confusion.Either implement the
onEditfunctionality or remove it if not required.- const onEdit = useCallback(() => {}, []) // TODO: implement + // TODO: Implement onEdit functionality or remove if unnecessary
239-253: Adjust button alignment for better UI presentation.The
Set Activebutton might not be vertically aligned with the card content, affecting the visual layout.Consider adjusting the styling to align the button properly:
<Box display={'flex'} alignContent={'center'} sx={{ padding: 1 }}> + <Box display={'flex'} alignItems={'center'} sx={{ padding: 1 }}>
269-285: Optimize the calculation of relic set bonuses.The current logic for calculating set bonuses can be simplified for efficiency.
Simplify the
setscalculation:const sets = useMemo(() => { const counts: Record<RelicSetKey, number> = {} Object.values(relicIds).forEach((relicId) => { const setKey = database.relics.get(relicId)?.setKey if (setKey) counts[setKey] = (counts[setKey] || 0) + 1 }) const bonuses: Partial<Record<RelicSetKey, 2 | 4>> = {} Object.entries(counts).forEach(([setKey, count]) => { if (count >= 4) bonuses[setKey as RelicSetKey] = 4 else if (count >= 2) bonuses[setKey as RelicSetKey] = 2 }) return bonuses }, [database.relics, relicIds])
350-353: Ensure consistent naming for boolean state hooks.The state hooks
showMainEditor,showSubsEditor, andshowSetsEditorcould be named more consistently.Consider prefixing boolean state variables with
isorshowfor clarity.const [showMainEditor, onShowMainEditor, onHideMainEditor] = useBoolState() const [showSubsEditor, onShowSubsEditor, onHideSubsEditor] = useBoolState() const [showSetsEditor, onShowSetsEditor, onHideSetsEditor] = useBoolState()
362-405: Refactor similar update functions to reduce code duplication.The
setSlot,setSubstat,setType, andsetSetsfunctions share similar logic patterns.Consider creating a generic update function or utilizing a helper to handle updates.
805-825: Improve accessibility by adding labels to buttons.The buttons for adding relic sets lack accessible labels, which may affect users utilizing assistive technologies.
Add
aria-labelattributes to the buttons:<Button disabled={!localSetKey} onClick={() => setRelicSetCount(2)} + aria-label="Add 2-piece set" > 2 </Button> <Button disabled={!localSetKey} onClick={() => setRelicSetCount(4)} + aria-label="Add 4-piece set" > 4 </Button>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (35)
libs/common/ui/src/hooks/index.ts(1 hunks)libs/common/ui/src/hooks/useRefOverflow.tsx(1 hunks)libs/common/ui/src/hooks/useRefSize.tsx(2 hunks)libs/sr/db-ui/src/hooks/index.ts(1 hunks)libs/sr/db-ui/src/hooks/useBuildTc.ts(1 hunks)libs/sr/db/src/Database/DataManager.ts(1 hunks)libs/sr/db/src/Database/DataManagers/BuildDataManager.ts(0 hunks)libs/sr/db/src/Database/DataManagers/BuildTcDataManager.ts(8 hunks)libs/sr/db/src/Interfaces/IBuildTc.ts(1 hunks)libs/sr/db/src/Interfaces/index.ts(1 hunks)libs/sr/formula/src/data/common/index.ts(1 hunks)libs/sr/formula/src/formula.test.ts(6 hunks)libs/sr/formula/src/util.ts(3 hunks)libs/sr/page-team/src/BuildsDisplay.tsx(8 hunks)libs/sr/page-team/src/TeamCalcProvider.tsx(3 hunks)libs/sr/page-team/src/TeammateDisplay.tsx(5 hunks)libs/sr/ui/src/Character/CharacterCard.tsx(2 hunks)libs/sr/ui/src/LightCone/LightConeAutocomplete.tsx(1 hunks)libs/sr/ui/src/LightCone/LightConeCard.tsx(2 hunks)libs/sr/ui/src/LightCone/LightConeCardCompact.tsx(3 hunks)libs/sr/ui/src/LightCone/LightConeEditor/LightConeEditor.tsx(1 hunks)libs/sr/ui/src/LightCone/LightConeEditor/LightConeEditorCard.tsx(1 hunks)libs/sr/ui/src/LightCone/LightConeEditor/index.tsx(1 hunks)libs/sr/ui/src/LightCone/LightConeEditor/reducer.ts(0 hunks)libs/sr/ui/src/LightCone/index.tsx(1 hunks)libs/sr/ui/src/Relic/RelicCardCompact.tsx(6 hunks)libs/sr/ui/src/Relic/RelicEditor/RelicEditor.tsx(3 hunks)libs/sr/ui/src/Relic/RelicEditor/RelicStatKeyDisplay.tsx(0 hunks)libs/sr/ui/src/Relic/RelicEditor/SubstatInput.tsx(3 hunks)libs/sr/ui/src/Relic/RelicEditor/index.tsx(0 hunks)libs/sr/ui/src/Relic/RelicMainStatDropdown.tsx(1 hunks)libs/sr/ui/src/Relic/RelicRarityDropdown.tsx(1 hunks)libs/sr/ui/src/Relic/index.tsx(1 hunks)libs/sr/ui/src/compactConst.ts(1 hunks)libs/sr/ui/src/index.tsx(1 hunks)
💤 Files with no reviewable changes (4)
- libs/sr/db/src/Database/DataManagers/BuildDataManager.ts
- libs/sr/ui/src/LightCone/LightConeEditor/reducer.ts
- libs/sr/ui/src/Relic/RelicEditor/RelicStatKeyDisplay.tsx
- libs/sr/ui/src/Relic/RelicEditor/index.tsx
✅ Files skipped from review due to trivial changes (2)
- libs/sr/formula/src/data/common/index.ts
- libs/sr/ui/src/compactConst.ts
🔇 Additional comments (50)
libs/sr/ui/src/LightCone/LightConeEditor/index.tsx (1)
1-2: LGTM! Verify the new component's integration.
The barrel file export follows good practices and maintains consistent naming conventions.
Let's verify the integration of the new LightConeEditorCard component:
✅ Verification successful
✅ Component is properly integrated and used
The LightConeEditorCard component is correctly integrated and used across the codebase:
- Used in
libs/sr/page-team/src/BuildsDisplay.tsx - Used within
LightConeEditor.tsxas a subcomponent - Properly exported through the barrel file
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check usage of LightConeEditorCard component
# Expected: Should find imports in TC build-related files
# Search for imports of LightConeEditorCard
rg -l "import.*LightConeEditorCard.*from.*LightCone/LightConeEditor"
# Search for usage of LightConeEditorCard component
ast-grep --pattern '<LightConeEditorCard $$$>'
Length of output: 119
Script:
#!/bin/bash
# Let's try a broader search approach
# Search for any mentions of LightConeEditorCard
rg "LightConeEditorCard"
# Find the actual component file
fd "LightConeEditorCard.tsx"
# Check the barrel file's directory structure
ls -R libs/sr/ui/src/LightCone/LightConeEditor/
Length of output: 800
libs/sr/db/src/Interfaces/index.ts (2)
1-1: LGTM! The new export follows the established pattern.
The addition of IBuildTc export maintains consistency with the existing interface export pattern and is appropriately placed in the database module.
1-1: Verify the interface implementation and usage.
Let's ensure the IBuildTc interface is properly defined and used consistently across the codebase.
✅ Verification successful
Based on the shell script results, I can now provide the final response:
Interface IBuildTc is well-defined and consistently used across the codebase
The interface is properly defined in IBuildTc.ts with clear type definitions and is used consistently in several components:
- Database management in
BuildTcDataManager.tswith proper validation - UI components in
RelicCardCompact.tsx - Team calculation in
TeamCalcProvider.tsxandBuildsDisplay.tsx
All usages follow the interface contract correctly, with appropriate type checking and property access. The implementation shows proper type safety through extensive validation in BuildTcDataManager.ts.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the IBuildTc interface definition and its usage
# Check the interface definition
echo "Checking IBuildTc interface definition:"
cat libs/sr/db/src/Interfaces/IBuildTc.ts
# Find all usages of IBuildTc in the codebase
echo -e "\nChecking IBuildTc usage:"
rg "IBuildTc" --type ts
# Check for any type errors in files using IBuildTc
echo -e "\nChecking for potential type-related imports:"
rg "import.*IBuildTc" --type ts
Length of output: 4355
libs/sr/ui/src/index.tsx (2)
2-2: LGTM! Export addition follows proper organization.
The new export for compact UI constants is properly placed in alphabetical order, maintaining good code organization.
2-2: Verify consistent usage of compact constants.
Let's ensure the new UI constants are used consistently across components that require compact layouts.
✅ Verification successful
Let me check the actual usage of these constants to ensure they are being used correctly.
Compact constants are used consistently across components
The constants are properly defined in compactConst.ts and are consistently used across all relevant compact components:
LightConeCardCompact.tsxandRelicCardCompact.tsxuse them for consistent sizing of compact cardsBuildsDisplay.tsxuses them for grid layout calculations and template definitions- No instances of hardcoded values were found that should be using these constants instead
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check usage of compact constants across the codebase
# Expected: Find references to these constants in TC build-related components
# Search for constant usage
echo "Searching for constant usage..."
rg -l "COMPACT_ELE_(HEIGHT|WIDTH)"
# Search for hardcoded values that might need to use these constants instead
echo "Checking for potential hardcoded values..."
ast-grep --pattern 'height: "$_em"'
ast-grep --pattern 'width: "$_em"'
Length of output: 442
Script:
#!/bin/bash
# Check the content of the constant file and its usage
echo "Content of compactConst.ts:"
cat libs/sr/ui/src/compactConst.ts
echo -e "\nUsage in LightConeCardCompact.tsx:"
rg "COMPACT_ELE_" libs/sr/ui/src/LightCone/LightConeCardCompact.tsx -B 1 -A 1
echo -e "\nUsage in RelicCardCompact.tsx:"
rg "COMPACT_ELE_" libs/sr/ui/src/Relic/RelicCardCompact.tsx -B 1 -A 1
echo -e "\nUsage in BuildsDisplay.tsx:"
rg "COMPACT_ELE_" libs/sr/page-team/src/BuildsDisplay.tsx -B 1 -A 1
Length of output: 2378
libs/sr/db-ui/src/hooks/index.ts (1)
2-2: LGTM! Verify the referenced hook file exists.
The new export follows the established pattern and aligns with the PR objectives for adding TC build functionality.
Let's verify the existence and implementation of the referenced hook:
✅ Verification successful
The referenced hook file exists and is properly implemented
The verification confirms that useBuildTc.ts exists at the correct location (libs/sr/db-ui/src/hooks/useBuildTc.ts) and contains the expected hook implementation. The hook follows the same pattern as its counterpart in the gi library, with appropriate database context integration.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the existence and basic structure of useBuildTc hook
# Check if the hook file exists and contains expected exports
fd --type f "useBuildTc.ts" --exec sh -c '
echo "=== Found hook file: {}" &&
echo "=== Content preview:" &&
head -n 10 "{}"
'
# Verify the hook implementation matches the pattern described in the AI summary
ast-grep --pattern 'export function useBuildTc($_) {
$$$
}'
Length of output: 1549
libs/sr/ui/src/LightCone/index.tsx (1)
1-1: LGTM! The new export follows consistent patterns.
The addition of LightConeAutocomplete export aligns with the existing module organization and supports the PR's objective of enhancing TC build management.
Let's verify the exported module exists:
✅ Verification successful
The exported module exists at the expected location
The verification confirms that LightConeAutocomplete.tsx exists at libs/sr/ui/src/LightCone/LightConeAutocomplete.tsx, making this a valid export statement.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the LightConeAutocomplete module exists
# Expected: File should exist at the specified path
fd --type f "LightConeAutocomplete.tsx" libs/sr/ui/src/LightCone
Length of output: 115
libs/common/ui/src/hooks/index.ts (1)
5-5: LGTM! Clean and consistent export.
The new export for useRefOverflow maintains the established patterns:
- Follows the consistent
export *pattern - Maintains alphabetical ordering
- Integrates well with the existing hook exports
libs/sr/db-ui/src/hooks/useBuildTc.ts (2)
1-2: LGTM! Clean and focused imports.
The imports are well-organized, importing only the necessary dependencies. The separation between common utilities and local context is clear.
3-6: Consider adding input validation and error handling.
While the implementation is clean and follows React conventions, consider these improvements:
- Add validation for the
buildTcIdparameter - Consider throwing an error or returning null for invalid IDs instead of defaulting to an empty string
- Add JSDoc documentation to explain the hook's purpose and return type
Here's a suggested implementation:
+/**
+ * Hook to access and manage TC (Team Composition) build data
+ * @param buildTcId - The unique identifier for the TC build
+ * @returns The TC build data manager instance
+ * @throws {Error} When the buildTcId is invalid
+ */
export function useBuildTc(buildTcId: string | undefined) {
const { database } = useDatabaseContext()
+ if (buildTcId !== undefined && !buildTcId) {
+ throw new Error('Invalid buildTcId provided')
+ }
return useDataManagerBase(database.buildTcs, buildTcId ?? '')
}Let's verify how this hook is being used in the codebase:
libs/sr/ui/src/Relic/index.tsx (1)
6-8: LGTM! Well-organized exports supporting TC build functionality.
The new exports are logically organized and provide essential UI components for relic configuration, aligning well with the PR's objective of implementing TC build editing capabilities.
Let's verify that these new components exist and are properly implemented:
✅ Verification successful
All exported components are properly implemented and match the barrel exports
The verification confirms that:
- All three component files exist at the expected locations
- Each component is properly implemented as a named export
- The components accept appropriate props for their functionality:
RelicMainStatDropdown: handles stat and slot selectionRelicRarityDropdown: manages rarity with filter and number display optionsRelicSetAutocomplete: handles relic set selection
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the existence and implementation of the newly exported components
# Check if the component files exist
echo "Checking for component files..."
fd -t f "Relic(MainStatDropdown|RarityDropdown|SetAutocomplete).tsx?" "libs/sr/ui/src"
# Verify the component implementations
echo -e "\nChecking component implementations..."
for pattern in "export function RelicMainStatDropdown" "export function RelicRarityDropdown" "export function RelicSetAutocomplete"; do
echo -e "\nSearching for: $pattern"
rg "$pattern" -A 5 "libs/sr/ui/src"
done
Length of output: 2519
libs/sr/db/src/Interfaces/IBuildTc.ts (2)
4-4: LGTM: Import statements are properly structured
The new imports for RelicRarityKey and ILightCone are correctly added and follow the project's module structure.
Also applies to: 10-10
12-16: Verify rarity initialization in BuildTcRelicSlot usage
The addition of the rarity field is good, but ensure it's properly initialized in all places where BuildTcRelicSlot is created.
✅ Verification successful
Let me gather more information to verify how BuildTcRelicSlot is being used and initialized.
Rarity initialization is properly handled
The rarity field is correctly initialized in all relevant places:
- Default initialization is handled in
initCharTCRelicSlots()with a default value of 5 - Updates are managed through a
setSlotfunction that usesPartial<BuildTcRelicSlot>, allowing incremental updates while preserving existing values
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for BuildTcRelicSlot object creation to verify rarity initialization
ast-grep --pattern 'BuildTcRelicSlot = {
$$$
}'
# Search for any direct assignments to check initialization
rg -A 5 'BuildTcRelicSlot.*='
Length of output: 1343
Script:
#!/bin/bash
# Search for any usage of BuildTcRelicSlot in function parameters or type definitions
rg -A 10 'BuildTcRelicSlot'
# Search for relic slot initialization patterns
ast-grep --pattern '{
level: $_,
statKey: $_,
rarity: $_
}'
# Look for default values or initialization logic
rg -A 5 'setSlot.*BuildTcRelicSlot'
Length of output: 6801
libs/common/ui/src/hooks/useRefOverflow.tsx (1)
1-2: LGTM! Proper setup for client-side React hook
The 'use client' directive and imports are correctly configured for a client-side React hook.
libs/sr/db/src/Database/DataManager.ts (1)
24-24: Verify the impact of the key pattern change
The modification to check for keys with an underscore suffix (${this.goKeySingle}_) is more specific than before. While this prevents accidental deletion of similarly prefixed keys, it could potentially break compatibility with existing data if there are keys stored without the underscore pattern.
Let's verify the existing key patterns in the codebase:
✅ Verification successful
Based on the search results, I can now generate the final response as we have sufficient evidence about the key pattern usage:
Key pattern change is consistent with existing implementation
The underscore suffix in ${this.goKeySingle}_ is already the established pattern across the codebase:
CharacterDataManager.tsexplicitly uses${this.goKeySingle}_${key}for storage keysDataManagerBase.tsuses${this.goKeySingle}_${ind++}for generating unique keys- The key splitting in
toCacheKeyalso relies on this exact pattern
The change in DataManager.ts actually aligns the key checking with the existing key generation pattern, making it more robust rather than introducing compatibility issues.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for key pattern usage to ensure consistency
# Look for direct string assignments or template literals that might use the key pattern
# Search for key pattern usage in TypeScript/JavaScript files
rg -t ts -t js "this\.goKeySingle" -A 2 -B 2
# Search for potential key assignments
ast-grep --pattern 'const $_ = `${$_}_${$_}`'
Length of output: 1648
libs/sr/ui/src/Relic/RelicRarityDropdown.tsx (3)
13-14: Props interface changes look good!
The changes to make filter optional and add showNumber improve component flexibility while maintaining type safety.
17-21: Good use of named exports and default props!
The switch to named exports and the use of parameter destructuring for default props follow React best practices.
Line range hint 1-61: Verify integration with TC builds feature
Since this component is part of the TC builds feature, we should verify its integration with the relic editing interface and build system.
✅ Verification successful
Integration with TC builds feature is properly implemented
The component is correctly integrated within the codebase:
- Used in
RelicEditor/RelicEditor.tsxfor relic editing functionality with proper props for rarity management and sheet-based filtering - Integrated in
BuildsDisplay.tsxunder the team page for TC builds feature with rarity display and selection - Properly exported through the barrel file (
index.tsx) for consistent module access
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for proper imports and usage in related files
echo "Checking RelicEditor.tsx usage:"
rg -A 5 "RelicRarityDropdown" "libs/sr/ui/src/Relic/RelicEditor.tsx"
echo "\nChecking other TC build related usage:"
rg -A 5 "RelicRarityDropdown" "libs/sr/ui/src/**/*Tc*.tsx"
Length of output: 433
Script:
#!/bin/bash
# Let's first find the actual files in the repository
echo "Finding relic editor and TC related files:"
fd -e tsx -e ts "Relic|Tc" libs/sr/ui/src
echo "\nChecking for RelicRarityDropdown usage across the codebase:"
rg -A 5 "RelicRarityDropdown"
Length of output: 3209
libs/sr/ui/src/LightCone/LightConeAutocomplete.tsx (4)
1-14: LGTM! Well-structured imports and type definitions.
The imports are properly organized and the type definitions are clear and properly constrained.
23-32: LGTM! Efficient options generation with proper memoization.
The options generation is well-implemented with proper performance optimization using useMemo and correct dependency array.
49-60: LGTM! Well-implemented loading state and component rendering.
The component properly handles loading states with Suspense and provides a good user experience with the Skeleton fallback.
20-20: Verify translation key existence.
Ensure that the translation key 'editor.lightConeName' exists in the 'lightCone' namespace.
libs/sr/ui/src/Relic/RelicMainStatDropdown.tsx (2)
1-12: LGTM! Well-organized imports with clear separation of concerns.
The imports are properly structured, with clear separation between UI components, type definitions, and internal utilities.
26-34: Verify the assumption about fixed main stats for head and hands slots.
The code assumes that head and hands slots have fixed main stats by always using index 0. Let's verify this assumption is correct and will remain stable.
✅ Verification successful
The assumption about fixed main stats for head and hands slots is correct
Found the definition in libs/sr/consts/src/relic.ts:
export const relicSlotToMainStatKeys: Record<RelicSlotKey, RelicMainStatKey[]> = {
head: ['hp'],
hands: ['atk'],
body: ['hp_', 'atk_', 'def_', 'eff_', 'heal_', 'crit_', 'crit_dmg_'],
feet: ['hp_', 'atk_', 'def_', 'spd'],
}The code shows that head and hands slots indeed have exactly one fixed main stat each:
- Head slot can only have HP
- Hands slot can only have ATK
The implementation in RelicMainStatDropdown.tsx correctly assumes this by accessing index 0, as these arrays will always have exactly one element.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that head and hands slots have exactly one main stat option
# Search for the definition of relicSlotToMainStatKeys
rg -A 10 "relicSlotToMainStatKeys.*=.*{"
# Alternative: Look for any tests that verify this constraint
rg -l "test.*head.*main.*stat|test.*hands.*main.*stat"
Length of output: 221
Script:
#!/bin/bash
# Let's try a different approach to find the definition
# Search for relicSlotToMainStatKeys with proper escaping
rg "relicSlotToMainStatKeys" -A 5
# Also search for any constant/type definitions related to relic slots
rg "type.*RelicSlot|interface.*RelicSlot" -A 5
Length of output: 14370
libs/sr/ui/src/Character/CharacterCard.tsx (1)
16-16: Verify the impact of the new import.
The switch to charTagMapNodeEntries aligns with the PR's TC builds feature. Let's verify its usage across the codebase to ensure consistency.
✅ Verification successful
Based on the results, I can now generate the final response:
The switch to charTagMapNodeEntries is properly implemented and consistent
The codebase scan shows that:
charTagMapNodeEntriesis used consistently in the relevant modules (CharacterCard.tsx,TeamCalcProvider.tsx, and formula-related files)- No legacy
charDatacalculator usages were found - The function is properly exported from
formula/src/util.tsand used appropriately in tests
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check usage of charTagMapNodeEntries and potential conflicts with charData
# Check for all usages of charTagMapNodeEntries
echo "Checking usages of charTagMapNodeEntries:"
rg "charTagMapNodeEntries" -A 2
# Check for any remaining usages of charData that might need updating
echo "Checking for potentially outdated charData usages:"
rg "charData.*srCalculator" -A 2
Length of output: 2166
libs/sr/formula/src/util.ts (2)
2-9: LGTM: Well-structured type imports
The new type imports are properly organized and follow TypeScript best practices, providing strong typing for game-specific mechanics.
Line range hint 83-107: LGTM: Well-structured implementation with good TypeScript practices
The function demonstrates excellent use of TypeScript features:
- Proper use of
Partial<Record>for flexible parameter typing - Clear separation of stats and sets handling
- Informative comments explaining the implementation details
libs/sr/formula/src/formula.test.ts (1)
14-18: LGTM! Import changes improve data handling structure
The switch to using tag map node entries provides a more organized approach to managing character and light cone data in tests.
libs/sr/ui/src/LightCone/LightConeCard.tsx (2)
11-11: LGTM: Import changes align with the new calculation approach.
The replacement of lightConeData with lightConeTagMapNodeEntries is consistent with the refactoring of the calculation logic.
59-63: Verify type safety and consider adding documentation.
The calculation logic has been refactored to use individual parameters, which improves clarity. However, let's ensure type safety and add documentation.
Let's verify the type safety:
Consider adding JSDoc comments to document the calculation:
+ // Memoize the calculator instance with light cone stats
+ // @param key - Light cone identifier
+ // @param level - Current level
+ // @param ascension - Ascension level
+ // @param superimpose - Superimposition level
const calc = useMemo(
() =>
srCalculatorWithEntries(
lightConeTagMapNodeEntries(key, level, ascension, superimpose)
),
[ascension, key, level, superimpose]
)✅ Verification successful
Let me gather more information about the type definitions and function implementation.
Let me search for the actual implementation of lightConeTagMapNodeEntries to verify its type safety.
Based on the gathered information, I can now generate the final response:
Type safety is verified, documentation suggestion remains valid
The implementation in libs/sr/formula/src/util.ts shows proper type safety with correctly typed parameters:
key: LightConeKeylevel: numberascension: AscensionKeysuperimpose: SuperimposeKey
All usages in the codebase match this type signature, including the one in the reviewed file. The suggestion to add JSDoc comments for better documentation remains valid but is not a critical issue.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for the type definition and usage of lightConeTagMapNodeEntries
ast-grep --pattern 'export function lightConeTagMapNodeEntries($$$)'
# Search for any TypeScript errors or type assertions around this usage
rg -A 2 'lightConeTagMapNodeEntries.*as'
Length of output: 843
Script:
#!/bin/bash
# Search for the implementation and type definition
ast-grep --pattern 'function lightConeTagMapNodeEntries($$$)'
# Search for the type imports and definitions related to the parameters
rg -B 2 -A 5 'type.*AscensionKey|interface.*LightCone'
# Look for any TypeScript errors in the files
rg 'error TS\d+|Type.*error'
Length of output: 9238
Script:
#!/bin/bash
# Search for the implementation using ripgrep with context
rg -B 3 -A 10 'export.*function lightConeTagMapNodeEntries'
# Also check for any usages of this function to verify parameter types
rg -B 2 -A 2 'lightConeTagMapNodeEntries\('
Length of output: 2845
libs/sr/ui/src/LightCone/LightConeCardCompact.tsx (1)
73-79: LGTM: Enhanced light cone stat calculation with proper memoization
The calculator initialization now properly handles all light cone parameters (level, ascension, superimpose) with correct dependency tracking in useMemo.
libs/sr/db/src/Database/DataManagers/BuildTcDataManager.ts (2)
85-89: LGTM! Clean implementation of character-specific build filtering.
The method efficiently filters build IDs using appropriate functional programming patterns.
44-45: Consider alternative validation strategy for teamId
The current approach of skipping teamId validation during database initialization could allow invalid team references to persist. Consider:
- Adding a post-initialization validation step
- Implementing a lazy validation mechanism
- Using a temporary validation state during initialization
✅ Verification successful
Let me analyze the code further to understand the teamId validation context better.
Let me try a different approach to understand the database initialization flow.
Let me try one final search to understand the database initialization and team validation.
Based on the code analysis, I can now provide a final response:
Comment about teamId validation is accurate and necessary
The commented code about skipping teamId validation during database initialization is correct and necessary because:
- The BuildTcDataManager is initialized as part of the database setup
- The teams collection is not available during this initialization phase
- The code shows this is a known limitation in the architecture, where circular dependencies are avoided during initialization
This is a valid architectural decision to prevent initialization deadlocks, and the comment serves as important documentation for future maintainers.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for potential invalid teamId references
rg "teamId" --type ts
Length of output: 29561
Script:
#!/bin/bash
# Check the Database class structure and initialization
ast-grep --pattern 'class Database {
$$$
teams: $_
$$$
}'
# Check the initialization order of database components
rg "new \(" -A 5 -B 5 "Database"
Length of output: 147
Script:
#!/bin/bash
# Look for Database class definition and initialization
rg "class Database" --type ts -A 10
# Check for database initialization order in constructor
rg "constructor" -A 10 -B 2 "Database.*Manager"
Length of output: 503
Script:
#!/bin/bash
# Search for BuildTcDataManager class and its validate method
rg "class BuildTcDataManager" --type ts -A 20
# Look for database initialization and teams setup
rg "database\." --type ts -A 3 -B 3 "teams"
Length of output: 4133
libs/sr/ui/src/Relic/RelicEditor/SubstatInput.tsx (3)
35-35: LGTM: Clean import replacement
The replacement of RelicStatWithUnit with StatDisplay aligns with the UI improvements objective.
97-97: LGTM: Consistent StatDisplay implementation
The StatDisplay component is consistently implemented with appropriate props across both dropdown and menu item contexts.
Also applies to: 126-126
Line range hint 38-47: Verify consistent StatDisplay usage across the application
Since this component is part of a larger relic editing system, we should ensure consistent stat display implementation across related components.
✅ Verification successful
StatDisplay implementation is consistent across relic components
Based on the verification results, the StatDisplay component is consistently used across the relic-related components with proper prop handling:
- All relic components import StatDisplay from the Character module
- SubstatInput correctly uses StatDisplay with
showPercentanddisableIconprops - Same pattern is followed in RelicMainStatDropdown, RelicCard, and other related components
- The implementation maintains consistency in displaying stat values across the application
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistent StatDisplay usage across related components
# Find all files that might contain stat display logic
echo "Checking for StatDisplay usage patterns..."
rg -A 2 "StatDisplay" --type typescript --type tsx
# Look for potential inconsistencies in stat display implementations
echo "Checking for other stat display implementations..."
rg -A 2 "statKey.*showPercent|statKey.*disableIcon" --type typescript --type tsx
Length of output: 409
Script:
#!/bin/bash
# Let's try a different approach to find stat display related components
# Search for files containing StatDisplay
fd -e tsx -e ts | xargs rg -l "StatDisplay"
# Search for substat related components and their implementations
fd -e tsx -e ts | xargs rg -l "substat|Substat"
# Look for relic-related components that might use similar stat display logic
fd -e tsx -e ts | xargs rg -l "relic.*stat|stat.*display" -i
Length of output: 10356
Script:
#!/bin/bash
# Let's examine the StatDisplay implementation in the SR UI and its usage
# Check the StatDisplay component implementation
echo "=== StatDisplay Implementation ==="
rg -A 10 "export.*StatDisplay" libs/sr/ui/src/Character/StatDisplay.tsx
# Check how SubstatInput uses StatDisplay
echo "=== SubstatInput Implementation ==="
rg -A 10 "export.*SubstatInput" libs/sr/ui/src/Relic/RelicEditor/SubstatInput.tsx
# Check other relic-related components using StatDisplay
echo "=== Other Relic Components using StatDisplay ==="
rg "StatDisplay" libs/sr/ui/src/Relic/
Length of output: 2457
libs/sr/ui/src/Relic/RelicEditor/RelicEditor.tsx (1)
21-21: LGTM! Import organization improves modularity
The reorganization of imports, particularly moving RelicRarityDropdown to the parent directory and adding new component imports, improves the code's modularity and maintainability.
Also applies to: 60-62
libs/sr/ui/src/LightCone/LightConeEditor/LightConeEditor.tsx (1)
52-58: Handle unsaved changes when closing the editor
The onClose function checks if lightConeState?.key exists before prompting the user, but when adding a new light cone, lightConeState.key may not be set, potentially not warning users about unsaved changes.
Verify whether unsaved changes in a new light cone trigger the confirmation prompt. If not, consider adjusting the condition.
Run the following script to check for the presence of key in lightConeState during new light cone creation:
libs/sr/ui/src/LightCone/LightConeEditor/LightConeEditorCard.tsx (2)
94-94: Confirm the fallback value for undefined ascension in display.
In the ascension display, if lightCone?.ascension is undefined, it defaults to 0. Verify if displaying Ascension 0 is the intended behavior when no ascension level is set.
If this is not the desired outcome, consider handling the undefined case differently or updating the default value.
136-153: Component implementation follows best practices and is well-structured.
The SuperimpositionDropdown component is clean and effectively handles user interaction with proper localization and state management.
libs/sr/page-team/src/TeammateDisplay.tsx (7)
1-4: Added necessary imports for React utilities
The imports useBoolState and useForceUpdate from @genshin-optimizer/common/react-util are appropriate and correctly utilized.
30-31: Imported required React hooks and components
The addition of useEffect, useMemo, useState, and components from './BuildsDisplay' is appropriate for the new functionality.
39-39: Destructured buildType from teammateDatum
Including buildType in the destructuring allows for conditional rendering based on the build type.
68-68: Conditional rendering of Optimize component
The Optimize component is correctly rendered only when buildType is not 'tc', aligning with the intended functionality.
75-79: Efficient use of useMemo for buildName computation
The useMemo hook is appropriately utilized to memoize the buildName, improving performance by avoiding unnecessary recalculations.
104-104: Conditional rendering of build display components
The component correctly renders <BuildTCDisplay /> when buildType is 'tc' and <BuildDisplay /> otherwise.
136-136: Passing onClose prop to BuildsDisplay
The onClose prop is appropriately passed to the BuildsDisplay component, ensuring it can control the modal's visibility.
libs/sr/ui/src/Relic/RelicCardCompact.tsx (1)
202-205:
Potential issue: SubstatDisplay may not render when value is zero
In the SubstatDisplay component, the condition if (!value || !statKey) return null will prevent rendering when value is 0. If 0 is a valid substat value, consider adjusting the condition to correctly handle zero values.
Apply this diff to modify the condition:
function SubstatDisplay({ stat }: { stat: ICachedSubstat }) {
const { value, key: statKey } = stat
- if (!value || !statKey) return null
+ if (value === undefined || value === null || !statKey) return nullLikely invalid or redundant comment.
libs/sr/page-team/src/BuildsDisplay.tsx (2)
287-299:
Fix typo in variable name overflex to overflow.
There is a typo in the CSS property overflex which should be overflow.
Correct the typo to ensure proper styling:
overflex: 'hidden',
+ overflow: 'hidden',Likely invalid or redundant comment.
630-633:
Ensure InputAdornment is properly closed.
There is a potential issue with the InputAdornment component not being correctly closed.
Verify that the InputAdornment component is self-contained:
<InputAdornment position="start">Level</InputAdornment>Ensure there are no unclosed tags or nesting issues.
Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (16)
libs/sr/ui/src/LightCone/LightConeEditor/LightConeEditor.tsx (2)
27-38: Consider combining useEffect hooksThe two
useEffecthooks can be combined to handle both cases in a single effect, reducing complexity and potential race conditions.-useEffect(() => { - if (dbLightCone) setLightConeState(structuredClone(dbLightCone)) -}, [dbLightCone]) -useEffect(() => { - if (lightConeIdToEdit === 'new') - setLightConeState({ - level: 1, - ascension: 0, - superimpose: 1, - }) -}, [lightConeIdToEdit]) +useEffect(() => { + if (dbLightCone) { + setLightConeState(structuredClone(dbLightCone)) + } else if (lightConeIdToEdit === 'new') { + setLightConeState({ + level: 1, + ascension: 0, + superimpose: 1, + }) + } +}, [dbLightCone, lightConeIdToEdit])
117-128: Consider adding a loading indicatorWhile the Suspense wrapper is appropriate, the
fallback={false}might lead to a jarring user experience. Consider adding a loading indicator.-<Suspense fallback={false}> +<Suspense fallback={<CircularProgress />}>Don't forget to add the import:
+import { CircularProgress } from '@mui/material'libs/sr/consts/src/relic.ts (1)
109-114: LGTM! Consider adding JSDoc documentation.The type guard implementation is correct and follows TypeScript best practices. It properly validates both the type and value range of the input.
Consider adding JSDoc documentation to improve maintainability:
+/** + * Type guard to check if a value is a valid relic rarity (2-5 stars) + * @param rarity - The value to check + * @returns True if the value is a valid RelicRarityKey + */ export function isRelicRarityKey(rarity: unknown): rarity is RelicRarityKey { return ( typeof rarity === 'number' && allRelicRarityKeys.includes(rarity as RelicRarityKey) ) }libs/sr/page-team/src/TeamCalcProvider.tsx (2)
124-134: Simplify lcTagEntries logic and ensure proper memoizationThe current implementation can be simplified and made more robust:
- The ternary operation could be moved into a separate variable for better readability
- The dependency array should include all accessed properties
Consider this refactoring:
const lcTagEntries = useMemo(() => { - const lc = - meta?.buildType === 'tc' ? (buildTc?.lightCone as ILightCone) : lightCone + const lc = meta?.buildType === 'tc' + ? buildTc?.lightCone + : lightCone if (!lc) return [] return lightConeTagMapNodeEntries( lc.key, lc.level, lc.ascension, lc.superimpose ) - }, [meta?.buildType, buildTc?.lightCone, lightCone]) + }, [meta?.buildType, buildTc, lightCone])
151-164: Enhance type safety in relicsData functionThe function handles accumulation well, but could benefit from stronger type guarantees.
Consider these type safety improvements:
function relicsData(relics: ICachedRelic[]): TagMapNodeEntries { - const sets: Partial<Record<RelicSetKey, number>> = {}, - stats: Partial<Record<RelicMainStatKey | RelicSubStatKey, number>> = {} + const sets = new Map<RelicSetKey, number>() + const stats = new Map<RelicMainStatKey | RelicSubStatKey, number>() relics.forEach((relic) => { - sets[relic.setKey] = (sets[relic.setKey] ?? 0) + 1 - stats[relic.mainStatKey] = - (stats[relic.mainStatKey] ?? 0) + relic.mainStatVal + sets.set(relic.setKey, (sets.get(relic.setKey) ?? 0) + 1) + stats.set(relic.mainStatKey, + (stats.get(relic.mainStatKey) ?? 0) + relic.mainStatVal) // ... rest of the function }) - return relicTagMapNodeEntries(stats, sets) + return relicTagMapNodeEntries( + Object.fromEntries(stats), + Object.fromEntries(sets) + ) }libs/sr/ui/src/LightCone/LightConeEditor/LightConeEditorCard.tsx (3)
44-44: Address TODO: Add translation for "Light Cone Editor"The title text is hardcoded and needs to be translated.
- title="Light Cone Editor" // TODO: translation + title={t('editor.lightConeEditor')}
56-59: Consider memoizing callback functionThe callback function passed to
setLCKeyis recreated on every render. Consider memoizing it with useCallback.+const handleSetLCKey = useCallback( + (lcKey: string) => setLightCone({ key: lcKey as LightConeKey }), + [setLightCone] +) - setLCKey={(lcKey) => setLightCone({ key: lcKey as LightConeKey })} + setLCKey={handleSetLCKey}
129-154: Consider memoizing SuperimpositionDropdown componentSince this component is used within a Grid layout that might re-render frequently, consider memoizing it with React.memo to prevent unnecessary re-renders.
-function SuperimpositionDropdown({ +const SuperimpositionDropdown = React.memo(function SuperimpositionDropdown({ superimpose, setSuperimposition, disabled = false, -}: SuperimpositionDropdownProps) { +}: SuperimpositionDropdownProps) { // ... component implementation ... -} +})libs/sr/page-team/src/TeammateDisplay.tsx (1)
118-122: Consider removing redundant type checkSince the component is only rendered when
buildType === 'tc'(see line 104), the type guard is redundant and can be simplified.function BuildTCDisplay() { const { teammateDatum } = useTeamContext() - if (teammateDatum.buildType !== 'tc') return null return <EquipRowTC buildTcId={teammateDatum.buildTcId} /> }libs/sr/db/src/Database/DataManagers/BuildTcDataManager.ts (2)
86-90: Consider optimizing getBuildTcIds performanceThe current implementation works but could be optimized for frequently accessed characters by:
- Using type assertion to avoid optional chaining
- Implementing memoization for frequently accessed characters
getBuildTcIds(characterKey: CharacterKey) { - return this.keys.filter( - (key) => this.get(key)?.characterKey === characterKey - ) + return this.keys.filter((key) => { + const build = this.get(key) as IBuildTc; + return build.characterKey === characterKey; + }); }
103-103: Extract magic number to a constantThe default rarity value of 5 is used directly in multiple places. Consider extracting it to a named constant for better maintainability.
+ const DEFAULT_RELIC_RARITY = 5 as const; return { // ... substats: { type: 'max', stats: objKeyMap(allRelicSubStatKeys, () => 0), - rarity: 5, + rarity: DEFAULT_RELIC_RARITY, }, // ... }Also applies to: 121-121
libs/sr/ui/src/Relic/RelicCardCompact.tsx (3)
165-175: Consider improving color contrast for better accessibilityThe level chip uses a semi-transparent background (
rgba(0,0,0,0.3)) which might not provide sufficient contrast for the text, especially on lighter backgrounds. Consider increasing the opacity or adding a text shadow to ensure better readability.<Chip size="small" label={<strong>{` +${level}`}</strong>} sx={{ position: 'absolute', top: 3, left: 3, - backgroundColor: 'rgba(0,0,0,0.3)', + backgroundColor: 'rgba(0,0,0,0.6)', backdropFilter: 'blur(2px)', + '& .MuiChip-label': { + textShadow: '0px 0px 2px rgba(0,0,0,0.8)', + }, }} />
362-363: Address the translation TODO commentThe comment indicates missing translations for the "No Relic sets" text. This should be handled using your translation system for consistency.
Would you like me to help implement the translation for this text?
276-283: Consider extracting the common action wrapper logicThe
actionWrapperFuncis duplicated across multiple components. Consider extracting it into a shared utility function to reduce code duplication and improve maintainability.Create a new utility function:
function createActionWrapper(onClick?: () => void) { return useCallback( (children: ReactNode) => ( <CardActionArea onClick={onClick} sx={{ height: '100%', width: '100%' }}> {children} </CardActionArea> ), [onClick] ) }Then use it in the components:
- const actionWrapperFunc = useCallback( - (children: ReactNode) => ( - <CardActionArea onClick={onClick} sx={{ height: '100%', width: '100%' }}> - {children} - </CardActionArea> - ), - [onClick] - ) + const actionWrapperFunc = createActionWrapper(onClick)Also applies to: 335-342, 384-391
libs/sr/page-team/src/BuildsDisplay.tsx (2)
173-176: Improve build type assignment logic clarityThe conditions for setting buildId and buildTcId should be more explicit with proper else-if chaining to prevent potential issues.
- if (newBuildType === 'real' && newBuildId) - teammateDatum.buildId = newBuildId - else if (newBuildType === 'tc' && newBuildId) - teammateDatum.buildTcId = newBuildId + if (newBuildType === 'real' && newBuildId) { + teammateDatum.buildId = newBuildId + teammateDatum.buildTcId = undefined + } else if (newBuildType === 'tc' && newBuildId) { + teammateDatum.buildTcId = newBuildId + teammateDatum.buildId = undefined + }
546-546: Implement translations for editor titlesThere are multiple TODO comments about translations. The editor titles should be localized using the translation system.
Would you like me to help implement the translation system or create a GitHub issue to track this task?
Also applies to: 661-661, 780-780
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (11)
libs/common/ui/src/hooks/useRefOverflow.tsx(1 hunks)libs/common/ui/src/hooks/useRefSize.tsx(2 hunks)libs/sr/consts/src/relic.ts(1 hunks)libs/sr/db/src/Database/DataManagers/BuildTcDataManager.ts(9 hunks)libs/sr/page-team/src/BuildsDisplay.tsx(8 hunks)libs/sr/page-team/src/TeamCalcProvider.tsx(3 hunks)libs/sr/page-team/src/TeammateDisplay.tsx(5 hunks)libs/sr/ui/src/LightCone/LightConeEditor/LightConeEditor.tsx(1 hunks)libs/sr/ui/src/LightCone/LightConeEditor/LightConeEditorCard.tsx(1 hunks)libs/sr/ui/src/Relic/RelicCardCompact.tsx(6 hunks)libs/sr/ui/src/Relic/RelicRarityDropdown.tsx(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- libs/common/ui/src/hooks/useRefOverflow.tsx
- libs/common/ui/src/hooks/useRefSize.tsx
- libs/sr/ui/src/Relic/RelicRarityDropdown.tsx
🔇 Additional comments (10)
libs/sr/ui/src/LightCone/LightConeEditor/LightConeEditor.tsx (3)
1-14: LGTM! Type definitions are more explicit now
The imports are well-organized, and the type definition for lightConeIdToEdit is now more explicit with string | '' | 'new' union type.
40-42: LGTM! Efficient state update implementation
The update callback correctly handles partial updates while preserving existing state.
18-26: Verify initialization logic consistency
The default value of lightConeIdToEdit is set to '', but the initialization logic in useEffect checks for 'new'. This could lead to inconsistent initialization of new light cones.
libs/sr/page-team/src/TeamCalcProvider.tsx (1)
1-41: LGTM! Well-organized imports
The imports are properly organized by source and include all necessary types and utilities for the component's functionality.
libs/sr/page-team/src/TeammateDisplay.tsx (3)
39-39: LGTM! Clean implementation of TC build type handling
The changes effectively differentiate between regular and TC builds, with appropriate conditional rendering of the Optimize component.
Also applies to: 68-68
81-88: LGTM! Well-implemented database subscription handling
The useEffect hook correctly manages database subscriptions for both regular and TC builds, with proper cleanup and dependency array.
31-31: Verify TC build component dependencies
Let's ensure all TC build related components are properly imported and available.
Also applies to: 104-104
✅ Verification successful
All TC build components are properly integrated
The verification shows that:
BuildsDisplay,EquipRow, andEquipRowTCare all properly exported fromBuildsDisplay.tsxEquipRowTCcomponent correctly handles TC builds with the requiredbuildTcIdpropBuildsDisplaycomponent properly accepts theonCloseprop- The components are correctly imported and used in
TeammateDisplay.tsx
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify TC build component dependencies
# Check for EquipRowTC component definition
echo "Checking for EquipRowTC component..."
rg -l "export.*EquipRowTC" --type typescript
# Check for TC build type definitions
echo "Checking for TC build type definitions..."
rg -l "type.*buildType.*=.*'tc'" --type typescript
# Check for potential breaking changes in team composition feature
echo "Checking for team composition related files..."
rg -l "teammateDatum|buildType|buildTcId" --type typescript
Length of output: 551
Script:
#!/bin/bash
# Let's try without typescript file type restriction
# Check for EquipRowTC component definition
echo "Checking for EquipRowTC component..."
rg -l "export.*EquipRowTC"
# Check for TC build type definitions
echo "Checking for TC build type definitions..."
rg -l "type.*buildType.*=.*'tc'"
# Check for BuildsDisplay component and its usage
echo "Checking BuildsDisplay component and its usage..."
rg -l "BuildsDisplay"
# Check for EquipRow component
echo "Checking EquipRow component..."
rg -l "export.*EquipRow"
# Check for team composition related patterns
echo "Checking team composition patterns..."
rg -l "teammateDatum|buildType|buildTcId"
Length of output: 3158
Script:
#!/bin/bash
# Let's check the actual implementation and usage of these components
# Check BuildsDisplay implementation and props
echo "Checking BuildsDisplay implementation..."
rg "export.*BuildsDisplay" -A 5 libs/sr/page-team/src/BuildsDisplay.tsx
# Check EquipRowTC implementation
echo "Checking EquipRowTC implementation..."
rg "export.*EquipRowTC" -A 5 libs/sr/page-team/src/BuildsDisplay.tsx
# Check how TeammateDisplay uses these components
echo "Checking TeammateDisplay usage..."
rg "BuildsDisplay|EquipRowTC" -A 5 libs/sr/page-team/src/TeammateDisplay.tsx
Length of output: 1395
libs/sr/db/src/Database/DataManagers/BuildTcDataManager.ts (2)
167-169: LGTM! Improved rarity validation and level clamping
The changes properly handle rarity validation and correctly use rarity-specific max levels. The implementation addresses previous review comments about rarity validation.
Also applies to: 183-193
45-46: Document and track the pending teamId validation
The commented validation code needs proper documentation and tracking:
- Add a TODO comment with a tracking issue number
- Document when and how the validation should be implemented
- Consider adding runtime validation after database initialization
libs/sr/ui/src/Relic/RelicCardCompact.tsx (1)
288-289:
Fix incorrect width constant usage
The component's width is set to COMPACT_ELE_HEIGHT instead of COMPACT_ELE_WIDTH, which could cause inconsistent dimensions.
sx={{
height: COMPACT_ELE_HEIGHT,
- width: COMPACT_ELE_HEIGHT,
+ width: COMPACT_ELE_WIDTH,
}}Likely invalid or redundant comment.
Describe your changes
Issue or discord link
Testing/validation
Checklist before requesting a review (leave this PR as draft if any part of this list is not done.)
yarn run mini-cilocally to validate format and lint.Summary by CodeRabbit
New Features
useRefOverflowhook for overflow detection in referenced elements.useBuildTchook for managing build test cases from the database.LightConeAutocompletecomponent for selecting light cones.RelicMainStatDropdownandRelicRarityDropdowncomponents for relic management.RelicSubCard,RelicSetCardCompact, andRelicMainsCardCompactcomponents for enhanced relic display.Improvements
BuildsDisplaycomponent with new UI sections and state management.TeammateDisplayfor conditional rendering based on build types.LightConeEditorfor improved state management.LightConeCardCompactandRelicCardCompact.Bug Fixes
useRefSizehook.Chores