SRO Add conditional search/filter for relic sets/lightcones #2604
Conversation
WalkthroughThis pull request introduces type safety and search functionality for light cones and relic sets. New type guard functions Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
[sr-frontend] [Wed Jan 22 06:43:02 UTC 2025] - Deployed 07732b8 to https://genshin-optimizer-prs.github.io/pr/2604/sr-frontend (Takes 3-5 minutes after this completes to be available) [Wed Jan 29 19:26:23 UTC 2025] - 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: 0
🧹 Nitpick comments (6)
libs/sr/ui/src/LightCone/LightConeAutocomplete.tsx (1)
Line range hint
9-14: Consider enhancing type safety and error handling.While the current implementation is solid, consider these improvements:
- Add runtime validation for the empty string case
- Add explicit error handling for failed image loads
Here's a suggested implementation:
type LightConeAutocompleteProps = { lcKey: LightConeKey | '' setLCKey: (key: LightConeKey | '') => void label?: string + onError?: (error: Error) => void }And for the image component:
<ImgIcon size={2} sx={{ width: 'auto' }} src={lightConeAsset(key, 'cover')} + onError={(e) => { + console.error(`Failed to load light cone image for ${key}`); + onError?.(new Error(`Failed to load light cone image for ${key}`)); + }} />libs/sr/page-team/src/LightConeSheetsDisplay.tsx (2)
21-25: Add translation for the search label.The TODO comment indicates missing translation for "Search Light Cone".
Would you like me to help set up the translation key in the localization system?
27-32: Consider adding loading states and empty states.The grid implementation could benefit from handling edge cases:
- Loading state while fetching data
- Empty state when no light cones match the filter
<Grid container columns={3} spacing={1}> + {lcList.length === 0 ? ( + <Grid item xs={3}> + <Box sx={{ textAlign: 'center', py: 2 }}> + No light cones found + </Box> + </Grid> + ) : ( {lcList.map((setKey) => ( <Grid item xs={1} key={setKey}> <LightConeSheetDisplay lcKey={setKey} /> </Grid> ))} + )} </Grid>libs/sr/page-team/src/RelicSheetsDisplay.tsx (2)
9-11: Consider memoizing team context value.While the context usage is correct, consider memoizing the destructured value to prevent unnecessary re-renders if the team context changes frequently.
- const { team } = useTeamContext() - const conditionals = team.conditionals + const { conditionals } = useMemo(() => { + const { team } = useTeamContext() + return { conditionals: team.conditionals } + }, [])
12-19: Optimize list computation and consider edge cases.The list computation logic is sound, but there are a few potential improvements:
- The sort callback could be simplified
- Consider handling empty conditionals array
const relicList = useMemo(() => { const sets = conditionals .map((c) => c.sheet) .filter(isRelicSetKey) as RelicSetKey[] if (relicSetKey) sets.push(relicSetKey) - // Make sure the currently selected set is at the front - return [...new Set(sets)].sort((set) => (set === relicSetKey ? -1 : 1)) + const uniqueSets = [...new Set(sets)] + return uniqueSets.length > 0 + ? uniqueSets.sort((a, b) => Number(b === relicSetKey) - Number(a === relicSetKey)) + : uniqueSets }, [conditionals, relicSetKey])libs/sr/consts/src/relic.ts (1)
174-180: Consider optimizing type validation and adding tests.The implementation is correct but could be more efficient and safer:
- Consider caching the result for frequently used keys
- Add unit tests to verify edge cases
+const relicSetKeyCache = new Set<string>() + export function isRelicSetKey(key: unknown): key is RelicSetKey { + if (typeof key !== 'string') return false + if (relicSetKeyCache.has(key)) return true + + const isValid = isCavernRelicSetKey(key as RelicSetKey) || + isPlanarRelicSetKey(key as RelicSetKey) + + if (isValid) relicSetKeyCache.add(key) + return isValid - return ( - typeof key === 'string' && - (isCavernRelicSetKey(key as RelicSetKey) || - isPlanarRelicSetKey(key as RelicSetKey)) - ) }Would you like me to generate unit tests for this function to ensure proper validation of edge cases?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
libs/sr/consts/src/lightCone.ts(1 hunks)libs/sr/consts/src/relic.ts(1 hunks)libs/sr/page-team/src/LightConeSheetsDisplay.tsx(1 hunks)libs/sr/page-team/src/RelicSheetsDisplay.tsx(1 hunks)libs/sr/ui/src/LightCone/LightConeAutocomplete.tsx(1 hunks)libs/sr/ui/src/LightCone/LightConeEditor/LightConeEditorCard.tsx(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- libs/sr/ui/src/LightCone/LightConeEditor/LightConeEditorCard.tsx
🔇 Additional comments (7)
libs/sr/ui/src/LightCone/LightConeAutocomplete.tsx (2)
Line range hint
16-52: Implementation follows React best practices!The component demonstrates excellent use of:
- Proper memoization with
useMemoanduseCallback- Clear TypeScript typing
- Internationalization support
- Loading state handling with Suspense
15-15: Verify all imports are updated after changing to named export.The switch from default export to named export is a good practice for better tree-shaking and explicit imports. However, this is a breaking change.
Let's verify that all imports are updated correctly:
✅ Verification successful
All imports are already using named import syntax
The change to named export is consistent with the existing codebase, as all imports are already using the correct named import syntax. No updates needed.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining default imports of LightConeAutocomplete rg "import\s+LightConeAutocomplete\s+from" -g '*.{ts,tsx}' # Search for the new named imports to ensure they exist rg "import\s+{\s*LightConeAutocomplete\s*}" -g '*.{ts,tsx}'Length of output: 253
Script:
#!/bin/bash # Search for any imports of LightConeAutocomplete rg "import.*LightConeAutocomplete.*from.*['\"]" -g '*.{ts,tsx}' # Also check the actual file to see its location and original export rg "export.*LightConeAutocomplete" -g '*.{ts,tsx}'Length of output: 530
libs/sr/consts/src/lightCone.ts (1)
115-119: Well-implemented type guard function!The implementation follows TypeScript best practices for runtime type validation. The type guard pattern is correctly used to ensure type safety at runtime.
Consider adding unit tests to verify the behavior with:
- Valid light cone keys
- Invalid strings
- Non-string inputs
libs/sr/page-team/src/LightConeSheetsDisplay.tsx (2)
13-18: Efficient implementation of filtered list computation!The
useMemohook is appropriately used to optimize performance by preventing unnecessary recomputations. The implementation correctly:
- Filters valid light cone keys
- Handles duplicates using Set
- Ensures selected item appears first
1-7: Verify the impact of removing allLightConeKeys import.The changes remove the direct usage of
allLightConeKeys. Ensure this doesn't affect any functionality that might need the complete list of light cones.✅ Verification successful
Removal of
allLightConeKeysimport is safeThe complete list of light cones is still available through the
LightConeAutocompletecomponent, which internally usesallLightConeKeys. The type safety is maintained viaLightConeKeytype andisLightConeKeyfunction.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other usages of allLightConeKeys in the codebase rg "allLightConeKeys" --type tsLength of output: 3880
libs/sr/page-team/src/RelicSheetsDisplay.tsx (2)
1-5: LGTM! Import changes align with new functionality.The imports are appropriate for the new filtering functionality, bringing in necessary type validation and UI components.
21-36: Address TODO comment and consider responsive design.The UI structure looks good, but there are two points to address:
- There's a TODO comment for translation
- The grid columns might need adjustment for different screen sizes
<RelicSetAutocomplete relicSetKey={relicSetKey} setRelicSetKey={setRelicSetKey} - label="Search Relic Set" // TODO: translation + label={t('relicSet.search')} // Add translation key /> <Box> - <Grid container columns={3} spacing={1}> + <Grid container columns={{ xs: 1, sm: 2, md: 3 }} spacing={1}>Let me verify if the translation system is set up:
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
libs/sr/ui/src/LightCone/LightConeAutocomplete.tsx (1)
Line range hint
36-51: Consider adding error handling for image loading.While the implementation is solid, consider adding error handling for the image loading to improve user experience when assets fail to load.
Here's a suggested improvement:
const toImg = useCallback( (key: LightConeKey | '') => !key ? undefined : ( <ImgIcon size={2} sx={{ width: 'auto' }} src={lightConeAsset(key, 'cover')} + onError={(e) => { + e.currentTarget.src = 'fallback-image-path'; + console.warn(`Failed to load image for light cone: ${key}`); + }} /> ), [] )libs/sr/page-team/src/RelicSheetsDisplay.tsx (1)
12-19: Consider optimizing the set deduplication logic.While the current implementation works, the deduplication and sorting could be more efficient.
Consider this optimization:
- if (relicSetKey) sets.push(relicSetKey) - // Make sure the currently selected set is at the front - return [...new Set(sets)].sort((set) => (set === relicSetKey ? -1 : 1)) + const uniqueSets = new Set(sets) + if (relicSetKey) uniqueSets.add(relicSetKey) + return relicSetKey + ? [relicSetKey, ...Array.from(uniqueSets).filter(set => set !== relicSetKey)] + : Array.from(uniqueSets)This approach:
- Avoids unnecessary array operations
- Eliminates the need for sorting
- More clearly expresses the intent
libs/sr/consts/src/relic.ts (1)
174-180: Consider adding descriptive error logging for invalid keys.The type guard implementation is correct, but debugging invalid keys could be improved.
Consider adding debug logging:
export function isRelicSetKey(key: unknown): key is RelicSetKey { + if (typeof key !== 'string') { + console.debug(`Invalid RelicSetKey: expected string, got ${typeof key}`); + return false; + } + const isValid = isCavernRelicSetKey(key as RelicSetKey) || + isPlanarRelicSetKey(key as RelicSetKey); + if (!isValid) { + console.debug(`Invalid RelicSetKey: "${key}" is neither a cavern nor planar set key`); + } + return isValid; - return ( - typeof key === 'string' && - (isCavernRelicSetKey(key as RelicSetKey) || - isPlanarRelicSetKey(key as RelicSetKey)) - ) }libs/sr/page-team/src/LightConeSheetsDisplay.tsx (2)
12-18: Consider optimizing the filtering logic.The current implementation rebuilds the set on every render where either
conditionalsorlcKeychanges. Consider these improvements:
- Move the selected key check before creating the Set to avoid unnecessary Set operations
- Use a more explicit variable name than
setssince it's an arrayconst lcList = useMemo(() => { - const sets = conditionals.map((c) => c.sheet).filter(isLightConeKey) - if (lcKey) sets.push(lcKey) - // Make sure the currently selected lc is at the front - return [...new Set(sets)].sort((set) => (set === lcKey ? -1 : 1)) + const lightConeKeys = conditionals.map((c) => c.sheet).filter(isLightConeKey) + if (lcKey && !lightConeKeys.includes(lcKey)) { + lightConeKeys.unshift(lcKey) + } + return Array.from(new Set(lightConeKeys)) }, [conditionals, lcKey])
27-32: Consider adding loading states and error boundaries.The grid layout looks good, but there's no handling for loading states or errors when displaying light cones.
Consider wrapping the
LightConeSheetDisplaywith error boundaries and adding loading states for better user experience.libs/sr/consts/src/lightCone.ts (1)
115-119: Consider caching the light cone keys set for better performance.The current implementation performs an array inclusion check on every call. For better performance, consider using a Set for O(1) lookups.
+ const lightConeKeysSet = new Set(allLightConeKeys) export function isLightConeKey(key: unknown): key is LightConeKey { return ( - typeof key === 'string' && allLightConeKeys.includes(key as LightConeKey) + typeof key === 'string' && lightConeKeysSet.has(key as LightConeKey) ) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
libs/sr/consts/src/lightCone.ts(1 hunks)libs/sr/consts/src/relic.ts(1 hunks)libs/sr/page-team/src/LightConeSheetsDisplay.tsx(1 hunks)libs/sr/page-team/src/RelicSheetsDisplay.tsx(1 hunks)libs/sr/ui/src/LightCone/LightConeAutocomplete.tsx(1 hunks)libs/sr/ui/src/LightCone/LightConeEditor/LightConeEditorCard.tsx(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- libs/sr/ui/src/LightCone/LightConeEditor/LightConeEditorCard.tsx
🔇 Additional comments (7)
libs/sr/ui/src/LightCone/LightConeAutocomplete.tsx (3)
Line range hint
9-14: LGTM! Well-structured type definitions.The props interface is well-defined with proper TypeScript types, making the component type-safe and maintainable.
Line range hint
18-35: LGTM! Excellent use of React hooks and memoization.The implementation follows React best practices with proper use of:
- Memoized options array to prevent unnecessary recalculations
- Memoized callbacks to maintain referential equality
- Internationalization with proper translation keys
15-17: Verify the impact of changing to named export.The component has been changed from default export to named export. This change requires updates to all import statements in other files.
Let's verify all imports are updated correctly:
✅ Verification successful
All imports are correctly using named export syntax
All files importing
LightConeAutocompleteare already using the correct named import syntax:
libs/sr/ui/src/LightCone/LightConeEditor/LightConeEditorCard.tsxlibs/sr/page-team/src/LightConeSheetsDisplay.tsx🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining default imports of LightConeAutocomplete rg "import LightConeAutocomplete from.*LightConeAutocomplete" -l # Search for new named imports to ensure they're correct rg "import \{.*LightConeAutocomplete.*\} from.*LightConeAutocomplete" -lLength of output: 206
Script:
#!/bin/bash # Show the actual import line rg "import.*LightConeAutocomplete.*from" libs/sr/ui/src/LightCone/LightConeEditor/LightConeEditorCard.tsx # Search for any other import patterns containing LightConeAutocomplete rg "import.*LightConeAutocomplete" -g '!LightConeAutocomplete.tsx'Length of output: 485
libs/sr/page-team/src/RelicSheetsDisplay.tsx (3)
1-5: LGTM! Improved type safety and UI component organization.The imports are well-organized and appropriately chosen for implementing the search functionality and type safety.
9-11: LGTM! Clean state management setup.Good use of context for team data and local state for search functionality.
21-36: Address the translation TODO comment.The UI structure looks good, but there's a pending translation task.
Would you like me to help implement the translation for "Search Relic Set" label? I can:
- Check if similar translations exist in the codebase
- Generate the necessary translation key and entry
libs/sr/page-team/src/LightConeSheetsDisplay.tsx (1)
1-7: LGTM! Clean import structure with proper type safety.The imports are well-organized, bringing in necessary types and components while maintaining type safety through the
isLightConeKeytype guard.
frzyc
changed the title
Describe your changes
Currently, all the relic sets/lc sheets are displayed in the UI. this is very laggy.
Limit view to to only the ones with conditionals + searched.
only lc with conditionals set are shown.
search a relic set, searched set comes first, then the set conditionals
Issue or discord link
Testing/validation
Checklist before requesting a review (leave this PR as draft if any part of this list is not done.)
yarn run mini-cilocally to validate format and lint.Summary by CodeRabbit
Release Notes
New Features
Improvements
Technical Updates