GO: Builds indicator for artifacts #2620
Conversation
WalkthroughThis pull request enhances the artifact management system by adding localization support for build count and artifact usage. It introduces new localization keys for pluralizing "Build" and displaying artifact usage context. In the Changes
Sequence DiagramsequenceDiagram
participant User
participant ArtifactCard
participant ArtifactBuildUsageModal
participant Database
User->>ArtifactCard: View Artifact
ArtifactCard->>Database: Fetch Build Data
Database-->>ArtifactCard: Return Build Names
ArtifactCard->>ArtifactCard: Display Build Count Badge
User->>ArtifactCard: Click Build Count Badge
ArtifactCard->>ArtifactBuildUsageModal: Show Modal
ArtifactBuildUsageModal->>User: Display Build Names
Possibly related PRs
Suggested labels
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
libs/gi/ui/src/components/artifact/ArtifactCard.tsx (3)
161-170: Consider optimizing build counting performance.The current implementation could be optimized to prevent unnecessary calculations on each render:
- Memoize the buildNames array and numberOfBuilds calculation
- Consider using Object.entries() for cleaner iteration
const database = useDatabase() -const builds = database.builds.data -const buildNames = [] -for (const buildId in builds) { - if (builds[buildId]?.artifactIds[artifact.slotKey] === artifact.id) { - buildNames.push(builds[buildId].name) - } -} -const numberOfBuilds = buildNames.length +const { buildNames, numberOfBuilds } = useMemo(() => { + const builds = database.builds.data + const names = Object.entries(builds) + .filter(([_, build]) => build?.artifactIds[artifact.slotKey] === artifact.id) + .map(([_, build]) => build.name) + return { buildNames: names, numberOfBuilds: names.length } +}, [database.builds.data, artifact.id, artifact.slotKey])
531-564: Enhance modal accessibility and UX.Consider the following improvements:
- Add proper accessibility attributes
- Handle long lists of builds
- Pass build names as an array for more flexible formatting
function ArtifactBuildUsageModal({ show, onHide, - buildNames, + buildNames: string[], }: { show: boolean onHide: () => void - buildNames: string + buildNames: string[] }) { return ( - <ModalWrapper open={show} onClose={onHide}> + <ModalWrapper + open={show} + onClose={onHide} + aria-labelledby="build-usage-title" + aria-describedby="build-usage-description" + > <CardThemed> <CardHeader + id="build-usage-title" title={ <Typography variant="h6" flexGrow={1} display="flex" alignItems="center" > - {buildNames} + {t('artifact:usage')} </Typography> } action={ - <IconButton onClick={onHide}> + <IconButton onClick={onHide} aria-label="close"> <CloseIcon /> </IconButton> } /> <Divider /> + <Box + id="build-usage-description" + sx={{ + maxHeight: '60vh', + overflowY: 'auto', + p: 2 + }} + > + {buildNames.map((name, index) => ( + <Typography key={index}> + {name} + </Typography> + ))} + </Box> </CardThemed> </ModalWrapper> ) }
161-170: Consider optimizing build counting logic for better performance.The current implementation iterates through all builds using a for...in loop. Consider using array methods for better readability and potentially better performance.
Here's a suggested optimization:
- const builds = database.builds.data - const buildNames = [] - for (const buildId in builds) { - if (builds[buildId]?.artifactIds[artifact.slotKey] === artifact.id) { - buildNames.push(builds[buildId].name) - } - } - const numberOfBuilds = buildNames.length + const builds = database.builds.data + const buildNames = Object.values(builds) + .filter(build => build?.artifactIds[artifact.slotKey] === artifact.id) + .map(build => build.name) + const numberOfBuilds = buildNames.length
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
libs/gi/localization/assets/locales/en/artifact.json(1 hunks)libs/gi/ui/src/components/artifact/ArtifactCard.tsx(10 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: typecheck
- GitHub Check: test
- GitHub Check: lint
- GitHub Check: gen-file
- GitHub Check: call-build / build
- GitHub Check: call-build / build
🔇 Additional comments (5)
libs/gi/ui/src/components/artifact/ArtifactCard.tsx (4)
3-3: LGTM: Import changes align with new feature requirements.The added imports support the new build indicator functionality.
Also applies to: 12-12, 14-14, 29-29
390-399: LGTM: Well-implemented build count badge.The implementation:
- Provides clear visual feedback through cursor and color changes
- Uses proper localization for pluralization
- Has appropriate click handling
3-3: LGTM! Clean import organization and hook usage.The new imports are well-organized, and the hook implementation follows React best practices with clear, descriptive naming.
Also applies to: 12-12, 14-14, 29-29, 104-104
390-399: LGTM! Well-implemented build count badge.The badge implementation is clean with:
- Proper accessibility considerations (cursor styling)
- Correct translation usage for pluralization
- Appropriate conditional styling
libs/gi/localization/assets/locales/en/artifact.json (1)
125-127: LGTM: Proper localization support for build counts.The added translations:
- Support pluralization for build counts
- Include context for build usage
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
libs/gi/ui/src/components/artifact/ArtifactCard.tsx (2)
393-402: Enhance badge accessibility.The badge acts as a button but lacks proper accessibility attributes.
<SqBadge sx={{ ml: 'auto', cursor: numberOfBuilds ? 'pointer' : 'default', }} color={numberOfBuilds ? 'success' : 'secondary'} onClick={numberOfBuilds ? onShowUsage : undefined} + role="button" + tabIndex={numberOfBuilds ? 0 : undefined} + aria-label={t('builds', { count: numberOfBuilds })} > {t('builds', { count: numberOfBuilds })} </SqBadge>
534-574: Enhance modal robustness.The modal implementation could benefit from handling edge cases and improving user experience for long lists.
function ArtifactBuildUsageModal({ show, onHide, buildNames, usageText, }: { show: boolean onHide: () => void buildNames: string[] usageText: string }) { + if (!buildNames.length) { + return null + } return ( <ModalWrapper open={show} onClose={onHide}> <CardThemed> <CardHeader title={ <Typography variant="h6" flexGrow={1} display="flex" alignItems="center" > {usageText} </Typography> } action={ <IconButton onClick={onHide}> <CloseIcon /> </IconButton> } /> - <List> + <List sx={{ maxHeight: '60vh', overflow: 'auto' }}> {buildNames.map((name, index) => ( <ListItem key={index}> <ListItemText primary={name} /> </ListItem> ))} </List> </CardThemed> </ModalWrapper> ) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
libs/gi/localization/assets/locales/en/artifact.json(1 hunks)libs/gi/ui/src/components/artifact/ArtifactCard.tsx(9 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- libs/gi/localization/assets/locales/en/artifact.json
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: call-build / build
- GitHub Check: lint
🔇 Additional comments (1)
libs/gi/ui/src/components/artifact/ArtifactCard.tsx (1)
3-3: LGTM! Clean import organization.The new imports are properly organized and necessary for implementing the build usage feature.
Also applies to: 12-12, 14-14, 29-29
failchon
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
libs/gi/ui/src/components/artifact/ArtifactCard.tsx (1)
394-403: Consider adding a tooltip to the badge.While the badge implementation is good, adding a tooltip would improve user experience by clarifying that clicking shows the list of builds.
<SqBadge sx={{ ml: 'auto', cursor: numberOfBuilds ? 'pointer' : 'default', }} color={numberOfBuilds ? 'success' : 'secondary'} onClick={numberOfBuilds ? onShowUsage : undefined} + title={numberOfBuilds ? t('artifact:clickToShowBuilds') : undefined} > {t('builds', { count: numberOfBuilds })} </SqBadge>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
libs/gi/ui/src/components/artifact/ArtifactCard.tsx(9 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: call-build / build
- GitHub Check: lint
🔇 Additional comments (4)
libs/gi/ui/src/components/artifact/ArtifactCard.tsx (4)
3-3: LGTM! Clean import additions.The new imports and hooks are well-chosen for implementing the build indicator feature.
Also applies to: 12-12, 14-14, 29-29
163-173: Well-implemented build counting logic with memoization!The code efficiently uses
useMemoto prevent unnecessary recalculations, and properly tracks both build names and count.
198-205: LGTM! Good use of Suspense for modal loading.The modal implementation properly handles loading states and receives the necessary props.
535-576: Well-structured modal component!The implementation follows the suggested improvements from previous reviews, with proper typing and a clean UI layout using Material-UI components.
|
[frontend] [Sat Jan 25 23:24:03 UTC 2025] - Deployed fad2bb3 to https://genshin-optimizer-prs.github.io/pr/2620/frontend (Takes 3-5 minutes after this completes to be available) [frontend] [Sun Jan 26 19:10:18 UTC 2025] - Deployed 9e9c529 to https://genshin-optimizer-prs.github.io/pr/2620/frontend (Takes 3-5 minutes after this completes to be available) [Sun Jan 26 19:24:37 UTC 2025] - Deleted deployment |
…ailchon/go/artifacts-builds-usage
…ailchon/go/artifacts-builds-usage
…hon/genshin-optimizer into failchon/go/artifacts-builds-usage
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
libs/gi/db/src/Database/DataManagers/BuildDataManager.ts (1)
Line range hint
32-69: Add type validation for the id property.While the id is correctly extracted and included in the validated object, consider adding type validation to ensure it's a non-empty string.
override validate(obj: unknown): Build | undefined { let { name, description, weaponId, artifactIds } = obj as Build const { id } = obj as Build + if (typeof id !== 'string' || !id) { + return undefined + } if (typeof name !== 'string') name = 'Build Name'libs/gi/ui/src/components/artifact/ArtifactCard.tsx (2)
549-602: Enhance modal list items with hover state.Consider adding hover state styling to list items for better user experience.
<ListItem key={index} + sx={{ + '&:hover': { + backgroundColor: 'action.hover', + cursor: 'pointer', + }, + }}
212-219: Optimize Suspense implementation.The nested Suspense with
fallback={false}can be simplified since it doesn't provide meaningful loading state.- <Suspense fallback={false}> <ArtifactBuildUsageModal show={showUsage} onHide={onHideUsage} usageText={t('artifact:artifactUsage')} builds={builds} /> - </Suspense>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
libs/gi/db/src/Database/DataManagers/BuildDataManager.ts(3 hunks)libs/gi/localization/assets/locales/en/artifact.json(1 hunks)libs/gi/ui/src/components/artifact/ArtifactCard.tsx(9 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- libs/gi/localization/assets/locales/en/artifact.json
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: lint
🔇 Additional comments (3)
libs/gi/db/src/Database/DataManagers/BuildDataManager.ts (1)
11-13: LGTM! The Build interface update is well-structured.The addition of the
idproperty enables unique identification of builds, which is essential for the new builds indicator feature.libs/gi/ui/src/components/artifact/ArtifactCard.tsx (2)
165-187: LGTM! Efficient implementation of build counting.The build counting logic is well-optimized:
- Uses
useMemoto prevent unnecessary recalculations- Correctly filters builds based on artifact usage
- Effectively maps the relationship between builds and team characters
408-417: LGTM! Well-implemented build indicator UI.The build indicator implementation:
- Follows accessibility best practices with conditional interactivity
- Uses proper translation for build count
- Has clear visual feedback with color changes
Describe your changes
Added a Builds indicator for artifacts that are used in a specific build.
Issue or discord link
Testing/validation
Checklist before requesting a review (leave this PR as draft if any part of this list is not done.)
yarn run mini-cilocally to validate format and lint.Summary by CodeRabbit
New Features
Improvements