Combo only UI redesign #2514
Combo only UI redesign #2514
Conversation
|
[sr-frontend] [Mon Oct 21 18:39:48 UTC 2024] - Deployed 52038bc to https://genshin-optimizer-prs.github.io/pr/2514/sr-frontend (Takes 3-5 minutes after this completes to be available) [frontend] [Mon Oct 21 18:40:26 UTC 2024] - Deployed 52038bc to https://genshin-optimizer-prs.github.io/pr/2514/frontend (Takes 3-5 minutes after this completes to be available) [sr-frontend] [Mon Oct 21 19:08:06 UTC 2024] - Deployed 2ac5af3 to https://genshin-optimizer-prs.github.io/pr/2514/sr-frontend (Takes 3-5 minutes after this completes to be available) [frontend] [Mon Oct 21 19:08:18 UTC 2024] - Deployed 2ac5af3 to https://genshin-optimizer-prs.github.io/pr/2514/frontend (Takes 3-5 minutes after this completes to be available) [sr-frontend] [Tue Oct 22 08:20:08 UTC 2024] - Deployed 8fee319 to https://genshin-optimizer-prs.github.io/pr/2514/sr-frontend (Takes 3-5 minutes after this completes to be available) [frontend] [Tue Oct 22 08:20:22 UTC 2024] - Deployed 8fee319 to https://genshin-optimizer-prs.github.io/pr/2514/frontend (Takes 3-5 minutes after this completes to be available) [sr-frontend] [Wed Oct 23 21:20:47 UTC 2024] - Deployed a5fcafd to https://genshin-optimizer-prs.github.io/pr/2514/sr-frontend (Takes 3-5 minutes after this completes to be available) [frontend] [Wed Oct 23 21:20:58 UTC 2024] - Deployed a5fcafd to https://genshin-optimizer-prs.github.io/pr/2514/frontend (Takes 3-5 minutes after this completes to be available) [sr-frontend] [Wed Oct 23 22:28:08 UTC 2024] - Deployed a6f478c to https://genshin-optimizer-prs.github.io/pr/2514/sr-frontend (Takes 3-5 minutes after this completes to be available) [frontend] [Wed Oct 23 22:28:23 UTC 2024] - Deployed a6f478c to https://genshin-optimizer-prs.github.io/pr/2514/frontend (Takes 3-5 minutes after this completes to be available) [sr-frontend] [Thu Oct 24 03:29:53 UTC 2024] - Deployed 3611895 to https://genshin-optimizer-prs.github.io/pr/2514/sr-frontend (Takes 3-5 minutes after this completes to be available) [frontend] [Thu Oct 24 03:29:59 UTC 2024] - Deployed 3611895 to https://genshin-optimizer-prs.github.io/pr/2514/frontend (Takes 3-5 minutes after this completes to be available) [sr-frontend] [Thu Oct 24 03:48:35 UTC 2024] - Deployed 3704c51 to https://genshin-optimizer-prs.github.io/pr/2514/sr-frontend (Takes 3-5 minutes after this completes to be available) [frontend] [Thu Oct 24 03:48:46 UTC 2024] - Deployed 3704c51 to https://genshin-optimizer-prs.github.io/pr/2514/frontend (Takes 3-5 minutes after this completes to be available) [sr-frontend] [Fri Oct 25 04:24:20 UTC 2024] - Deployed ec3cbc8 to https://genshin-optimizer-prs.github.io/pr/2514/sr-frontend (Takes 3-5 minutes after this completes to be available) [frontend] [Fri Oct 25 04:24:31 UTC 2024] - Deployed ec3cbc8 to https://genshin-optimizer-prs.github.io/pr/2514/frontend (Takes 3-5 minutes after this completes to be available) [sr-frontend] [Fri Oct 25 22:36:05 UTC 2024] - Deployed 4b7adea to https://genshin-optimizer-prs.github.io/pr/2514/sr-frontend (Takes 3-5 minutes after this completes to be available) [frontend] [Fri Oct 25 22:36:47 UTC 2024] - Deployed 4b7adea to https://genshin-optimizer-prs.github.io/pr/2514/frontend (Takes 3-5 minutes after this completes to be available) [sr-frontend] [Fri Oct 25 22:46:32 UTC 2024] - Deployed f4939c2 to https://genshin-optimizer-prs.github.io/pr/2514/sr-frontend (Takes 3-5 minutes after this completes to be available) [frontend] [Fri Oct 25 22:46:56 UTC 2024] - Deployed f4939c2 to https://genshin-optimizer-prs.github.io/pr/2514/frontend (Takes 3-5 minutes after this completes to be available) [sr-frontend] [Sat Oct 26 02:35:38 UTC 2024] - Deployed 87433dd to https://genshin-optimizer-prs.github.io/pr/2514/sr-frontend (Takes 3-5 minutes after this completes to be available) [frontend] [Sat Oct 26 02:35:47 UTC 2024] - Deployed 87433dd to https://genshin-optimizer-prs.github.io/pr/2514/frontend (Takes 3-5 minutes after this completes to be available) [sr-frontend] [Sat Oct 26 02:43:57 UTC 2024] - Deployed 4061645 to https://genshin-optimizer-prs.github.io/pr/2514/sr-frontend (Takes 3-5 minutes after this completes to be available) [frontend] [Sat Oct 26 02:44:08 UTC 2024] - Deployed 4061645 to https://genshin-optimizer-prs.github.io/pr/2514/frontend (Takes 3-5 minutes after this completes to be available) [sr-frontend] [Sat Oct 26 03:27:18 UTC 2024] - Deployed 413bd79 to https://genshin-optimizer-prs.github.io/pr/2514/sr-frontend (Takes 3-5 minutes after this completes to be available) [frontend] [Sat Oct 26 03:28:01 UTC 2024] - Deployed 413bd79 to https://genshin-optimizer-prs.github.io/pr/2514/frontend (Takes 3-5 minutes after this completes to be available) [sr-frontend] [Sat Oct 26 04:27:03 UTC 2024] - Deployed 1637f02 to https://genshin-optimizer-prs.github.io/pr/2514/sr-frontend (Takes 3-5 minutes after this completes to be available) [frontend] [Sat Oct 26 04:27:09 UTC 2024] - Deployed 8edc3c0 to https://genshin-optimizer-prs.github.io/pr/2514/frontend (Takes 3-5 minutes after this completes to be available) [sr-frontend] [Mon Oct 28 04:39:27 UTC 2024] - Deployed 9d9fff6 to https://genshin-optimizer-prs.github.io/pr/2514/sr-frontend (Takes 3-5 minutes after this completes to be available) [frontend] [Mon Oct 28 04:39:42 UTC 2024] - Deployed 9d9fff6 to https://genshin-optimizer-prs.github.io/pr/2514/frontend (Takes 3-5 minutes after this completes to be available) [sr-frontend] [Mon Oct 28 04:49:40 UTC 2024] - Deployed 0b60504 to https://genshin-optimizer-prs.github.io/pr/2514/sr-frontend (Takes 3-5 minutes after this completes to be available) [frontend] [Mon Oct 28 04:50:36 UTC 2024] - Deployed 0b60504 to https://genshin-optimizer-prs.github.io/pr/2514/frontend (Takes 3-5 minutes after this completes to be available) [sr-frontend] [Mon Oct 28 06:46:26 UTC 2024] - Deployed d17c30e to https://genshin-optimizer-prs.github.io/pr/2514/sr-frontend (Takes 3-5 minutes after this completes to be available) [frontend] [Mon Oct 28 06:46:54 UTC 2024] - Deployed d17c30e to https://genshin-optimizer-prs.github.io/pr/2514/frontend (Takes 3-5 minutes after this completes to be available) [sr-frontend] [Wed Oct 30 02:05:48 UTC 2024] - Deployed fe126b2 to https://genshin-optimizer-prs.github.io/pr/2514/sr-frontend (Takes 3-5 minutes after this completes to be available) [frontend] [Wed Oct 30 02:06:04 UTC 2024] - Deployed fe126b2 to https://genshin-optimizer-prs.github.io/pr/2514/frontend (Takes 3-5 minutes after this completes to be available) [sr-frontend] [Wed Oct 30 04:10:04 UTC 2024] - Deployed ae09592 to https://genshin-optimizer-prs.github.io/pr/2514/sr-frontend (Takes 3-5 minutes after this completes to be available) [frontend] [Wed Oct 30 04:10:52 UTC 2024] - Deployed ae09592 to https://genshin-optimizer-prs.github.io/pr/2514/frontend (Takes 3-5 minutes after this completes to be available) [Wed Oct 30 16:01:47 UTC 2024] - Deleted deployment |
WalkthroughThe changes in this pull request involve significant updates across multiple files, primarily focusing on refactoring context management from loadout-based to team-based structures. Key modifications include the introduction of new contexts ( Changes
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 30
🧹 Outside diff range and nitpick comments (27)
libs/sr/page-team/src/hooks/useTeam.ts (1)
Line range hint
1-6: Well-structured hook for the new team-based system.The hook maintains a clean and focused implementation while supporting the transition from loadout-based to team-based management. The composition with
useDataManagerBaseprovides good separation of concerns.Consider documenting the team data structure schema in a comment, as this hook will be a key integration point for the new team-based system.
libs/sr/page-team/src/context/PresetContext.ts (1)
1-11: Add JSDoc documentation for the context and its usage.Since this context is part of the new team-based architecture, adding comprehensive documentation would help other developers understand its role and integration points.
Consider adding documentation:
+/** + * Context for managing preset indices in the team-based UI. + * Each team can have multiple presets, and this context helps track + * which preset is currently active. + * + * @example + * ```tsx + * function TeamComponent() { + * const { presetIndex, setPresetIndex } = useContext(PresetContext); + * // ... use the context + * } + * ``` + */ export const PresetContext = createContext<PresetContextObj>({libs/sr/page-team/src/context/TeamContext.ts (1)
4-8: Consider clarifying theteamMetadatumproperty name.While the type structure is good, the property name
teamMetadatummight be unclear to other developers. Consider using a more descriptive name liketeamMetaorteamMetadatato better convey its purpose.export type TeamContextObj = { teamId: string team: Team - teamMetadatum: TeamMetaDataum + teamMeta: TeamMetaDataum // or teamMetadata }libs/sr/page-team/src/TeammateDisplay/Optimize/OptConfigWrapper.tsx (1)
1-28: Add documentation to explain the component's role in the new team-based architecture.As this component is part of the transition from loadout-based to team-based management, it would be valuable to add documentation explaining:
- Its role in the new architecture
- How it relates to team management
- Example usage in child components
Add JSDoc comments at the component level:
+/** + * Provides optimization configuration context for the new team-based management system. + * This wrapper enables any character to optimize from a team combo, replacing the old + * loadout-based approach. + * + * @example + * ```tsx + * <OptConfigWrapper optConfigId="team1_opt"> + * <OptimizationTools /> + * </OptConfigWrapper> + * ``` + */ export default function OptConfigWrapper({libs/sr/page-team/src/TeammateDisplay/Optimize/OptimizationTargetSelector.tsx (1)
43-43: LGTM! Consider consistent key format for all list items.The addition of the
keyprop toSqBadgeis a good practice for React's reconciliation process. For consistency with the parentMenuItemkey format, you might want to consider using a similar compound key pattern.- <SqBadge key={dmgType}>{dmgType}</SqBadge> + <SqBadge key={`${index}_${read.tag.sheet || read.tag.q}_${read.tag.name}_${dmgType}`}>{dmgType}</SqBadge>libs/sr/page-team/src/ComboEditor.tsx (3)
21-24: Consider making the sticky positioning more maintainable.The hardcoded top value (137) for sticky positioning could break if the team selector's height changes. Consider:
- Using CSS custom properties (variables) for maintaining these values
- Using dynamic calculations based on the actual element height
- top: 137, // height of the team selector + top: 'var(--team-selector-height)',
63-71: Enhance type safety and documentation.Consider adding:
- A proper interface/type alias for the props
- JSDoc documentation for the component
- Prop validation for required fields
+interface TeamProps { + /** The tag configuration for the team member */ + tag: Tag; + /** Zero-based index of the team member */ + index: number; + /** Callback to update the team member's target */ + setTarget(tag: Tag): void; +} + -function Team({ +function Team({ tag, index, setTarget, -}: { - tag: Tag - index: number - setTarget(tag: Tag): void -}) { +}: TeamProps) {
84-93: Improve accessibility for team member selection.The clickable area should have proper ARIA attributes and keyboard navigation support.
<ConditionalWrapper condition={presetIndex !== index} wrapper={(children) => ( <CardActionArea + role="button" + tabIndex={0} + aria-label={`Select team ${index + 1}`} + aria-pressed={presetIndex === index} onClick={() => setPresetIndex(index)}> {children} </CardActionArea> )} >libs/sr/solver/src/childWorker.ts (2)
Line range hint
108-157: Consider performance optimizations for the build calculation logic.The current implementation has several areas where performance could be improved:
- The nested loops (6 levels deep) create a high computational complexity.
- All calculations are performed sequentially in a single worker.
- Memory usage might spike with large relic sets.
Consider these architectural improvements:
- Implement a worker pool to parallelize calculations across multiple chunks of the search space.
- Use a generator pattern to yield results incrementally instead of building large arrays.
- Add early termination conditions based on constraint violations to reduce unnecessary calculations.
Would you like me to provide a detailed implementation proposal for any of these improvements?
Fix typo in interface name
ChildMessaageResultsand its usageThe interface has a typo and is used in two places within
childWorker.ts:
- Interface declaration:
export interface ChildMessaageResults- Union type:
ChildMessaageResultsinChildMessagetypeThe interface is imported in
parentWorker.tsthrough theChildMessagetype, but not directly. Since this is a local type system issue within the worker implementation, fixing both occurrences inchildWorker.tswill resolve the typo.🔗 Analysis chain
Line range hint
29-33: Fix typo in interface nameChildMessaageResults.The interface name contains a typo (extra 'a'). Since this is an exported interface, we should verify all its usages across the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any usage of the misspelled interface rg "ChildMessaageResults" # Search for any usage of the correct spelling to ensure we don't have mixed usage rg "ChildMessageResults"Length of output: 183
Script:
#!/bin/bash # Let's check for any imports or usage of this interface in other files rg -A 2 "from.*childWorker" # Also check for any type unions that might use this interface rg "type.*Child.*Message" # Check the entire file content to understand the interface usage cat libs/sr/solver/src/childWorker.tsLength of output: 5143
libs/sr/page-team/src/TeammateDisplay/index.tsx (2)
46-46: Consider UX flow in component ordering.While adding the ComboEditor aligns with the team combo redesign, consider if its placement before BonusStats creates the most intuitive user flow. The combo editor might be more logically placed after the character editing controls since it's closely related to character configuration.
Line range hint
54-123: Consider extracting debug sections to a separate component.The component contains extensive debug sections with nested accordions. Consider:
- Moving debug functionality to a separate DebugView component
- Conditionally rendering it based on environment/flags
- Using a dedicated debugging utility for formula calculations
This would improve maintainability and reduce the cognitive load when reading the main component code.
Example structure:
// DebugView.tsx export function DebugView({ calc }: { calc: CalcType }) { return ( <> <TargetListingsDebug calc={calc} /> <TargetBuffsDebug calc={calc} /> </> ); } // TeammateDisplay.tsx {process.env.NODE_ENV === 'development' && <DebugView calc={calc} />}libs/sr/page-team/src/TeamCharacterSelector.tsx (2)
Line range hint
49-54: Add input validation to team name/description handlersThe handlers directly update the database without any validation. Consider adding:
- Input length limits
- Special character sanitization
- Error handling for failed database updates
const handleName = (teamName: string): void => { + if (!teamName.trim()) return + const sanitized = teamName.slice(0, 50) // reasonable length limit - database.teams.set(teamId, { name: teamName }) + try { + database.teams.set(teamId, { name: sanitized }) + } catch (error) { + console.error('Failed to update team name:', error) + } }
170-171: Add translation for default character labelThe hardcoded string "Character X" needs to be translated for internationalization.
- `Character ${ind + 1}` // TODO: Translation + t('team.defaultCharacterLabel', { number: ind + 1 })Don't forget to add the translation key to your locale files.
libs/sr/solver/src/parentWorker.ts (1)
65-67: LGTM! Consider improving error serialization.The change to
console.erroris appropriate for error conditions. However, consider enhancing the error serialization to preserve stack traces.- console.error(err) - postMessage({ resultType: 'err', message: JSON.stringify(err) }) + const errorMessage = err instanceof Error ? + { message: err.message, stack: err.stack } : + { message: String(err) } + console.error(err) + postMessage({ resultType: 'err', message: JSON.stringify(errorMessage) })libs/common/util/src/lib/object.ts (2)
Line range hint
19-27: Fix missing await in recursive call.The changes improve async handling by properly awaiting the callback, but there's a missing await in the recursive call.
Apply this diff to ensure proper async chain:
else if (obj && typeof obj === 'object') { for (const [key, val] of Object.entries(obj)) { - crawlObjectAsync(val, [...keys, key], validate, cb) + await crawlObjectAsync(val, [...keys, key], validate, cb) } }This ensures that all nested async operations complete before the function returns.
Line range hint
1-215: Consider adding comprehensive tests for these critical utilities.Given that these utility functions are fundamental to the new team-based management system, consider adding comprehensive test cases that cover:
- Edge cases for null/undefined handling
- Async operation chains
- Deep object structures
- Performance benchmarks for recursive operations
This is particularly important as these utilities will be heavily used in the new UI redesign.
Would you like me to help create a test suite for these utilities?
libs/sr/db/src/Database/DataManagers/OptConfigDataManager.ts (1)
Line range hint
234-236: Address TODO comment regarding planar relics.The TODO comment indicates that 4-set exclusion for planar relics needs to be removed. This could affect the optimization behavior.
Would you like me to help implement the removal of 4-set exclusion for planar relics or create a GitHub issue to track this task?
libs/sr/page-team/src/BonusStats.tsx (2)
Line range hint
95-96: Address the TODO: Improve percentage stat inferenceThere's a TODO comment indicating uncertainty about inferring percentage stats from the tag:
// TODO: best way to infer percentage from tag? const isPercent = (tag.name || tag.q)?.endsWith('_')Relying on the underscore suffix may not be reliable for all stat keys. Consider using a predefined list of percentage-based stat keys to accurately determine if a stat is a percentage.
Would you like assistance in implementing a function that accurately identifies percentage stats?
67-70: Use stable keys for list renderingIn the
bonusStats.mapfunction, thekeyprop is constructed as:key={JSON.stringify(tag) + i}Relying on
JSON.stringify(tag)may not produce stable and unique keys due to potential property order variations in thetagobject. This could lead to rendering issues.Consider generating a stable key using a unique identifier from the
tag, such as a combination of its properties that are guaranteed to be consistent.- key={JSON.stringify(tag) + i} + key={`${tag.q}-${i}`}This change ensures that each key remains stable across renders.
libs/sr/page-team/src/index.tsx (3)
133-144: Remove commented-out code to improve code cleanlinessThe code block from lines 133 to 144 is commented out. If this code is no longer needed, consider removing it to enhance code readability and maintainability.
62-62: Clarify variable names to enhance readabilityThe variables
teamMetadata,teamMetadatumIndex, andteamMetadatumare similarly named and could potentially cause confusion. Consider renaming them to more descriptive names such asteamMembers,activeMemberIndex, andactiveMemberto improve code clarity.Also applies to: 71-81
52-60: Confirm if includingsetPresetIndexinuseMemodependencies is necessaryIn the
useMemoat lines 53-60,setPresetIndexis included in the dependency array. SincesetPresetIndexis a stable function returned byuseState, it's not necessary to include it as a dependency. Removing it could simplify the code.libs/sr/page-team/src/TalentContent.tsx (1)
339-347: Ensure proper rendering of document sections.In the mapping of
sheetElement.documents, verify that all necessary props are passed toDocumentDisplay. Additionally, consider uncommenting or removing thehideHeaderprop if it's no longer needed to keep the code clean.libs/sr/db/src/Database/DataManagers/TeamDataManager.ts (3)
Line range hint
278-293: Handle missing 'tc' case in 'getTeamRelics' methodIn the
getTeamRelicsmethod, theswitchstatement handles 'equipped' and 'real' build types but lacks a case for 'tc' (theory-crafted) builds. As a result, whenbuildTypeis 'tc', the method returns relics withundefinedvalues, which may not be the intended behavior.Add a 'tc' case to handle theory-crafted builds appropriately:
switch (buildType) { case 'equipped': { // existing code return ... } case 'real': { // existing code return ... + } + case 'tc': { + const buildTc = this.database.buildTcs.get(buildTcId) + if (!buildTc) return objKeyMap(allRelicSlotKeys, () => undefined) + return objMap(buildTc.relicIds, (id) => this.database.relics.get(id)) } }This ensures that all build types are adequately handled, and the method returns the correct relics for 'tc' builds.
233-273: Remove commented-out code to improve maintainabilityThe code block from lines 233 to 273 contains several methods (
duplicate,export, andimport) that are entirely commented out. Keeping large sections of commented-out code can clutter the codebase and reduce readability.Consider removing this unused code. If these methods are intended for future implementation, it's better to track them using version control or issue trackers rather than leaving them commented out in the codebase.
175-200: Implement missing validation logic indicated by TODO commentsThere are several TODO comments in the
validatemethod indicating missing validation logic:
- Line 176:
// TODO: validate conditionals src dst condKey- Line 199:
// TODO: validate frames- Line 201:
// TODO: validate bonusStatsTo ensure data integrity and prevent potential bugs, it's important to implement these validation steps. Would you like assistance in developing the validation logic for these sections, or should I open a new GitHub issue to track this task?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (45)
- libs/common/util/src/lib/array.ts (1 hunks)
- libs/common/util/src/lib/object.ts (1 hunks)
- libs/sr/consts/src/relic.ts (1 hunks)
- libs/sr/db/src/Database/DataManagers/LoadoutDataManager.ts (0 hunks)
- libs/sr/db/src/Database/DataManagers/OptConfigDataManager.ts (2 hunks)
- libs/sr/db/src/Database/DataManagers/TeamDataManager.test.ts (0 hunks)
- libs/sr/db/src/Database/DataManagers/TeamDataManager.ts (9 hunks)
- libs/sr/db/src/Database/DataManagers/index.ts (0 hunks)
- libs/sr/db/src/Database/Database.ts (0 hunks)
- libs/sr/page-team/src/BonusStats.tsx (5 hunks)
- libs/sr/page-team/src/ComboEditor.tsx (1 hunks)
- libs/sr/page-team/src/TalentContent.tsx (7 hunks)
- libs/sr/page-team/src/TeamCalcProvider.tsx (5 hunks)
- libs/sr/page-team/src/TeamCharacterSelector.tsx (3 hunks)
- libs/sr/page-team/src/TeamSelectors.tsx (1 hunks)
- libs/sr/page-team/src/TeamSettings/index.tsx (0 hunks)
- libs/sr/page-team/src/TeammateDisplay/Optimize/BuildDisplay.tsx (1 hunks)
- libs/sr/page-team/src/TeammateDisplay/Optimize/OptConfigWrapper.tsx (1 hunks)
- libs/sr/page-team/src/TeammateDisplay/Optimize/OptimizationTargetSelector.tsx (1 hunks)
- libs/sr/page-team/src/TeammateDisplay/Optimize/StatFilterCard.tsx (1 hunks)
- libs/sr/page-team/src/TeammateDisplay/Optimize/index.tsx (6 hunks)
- libs/sr/page-team/src/TeammateDisplay/index.tsx (2 hunks)
- libs/sr/page-team/src/context/PresetContext.ts (1 hunks)
- libs/sr/page-team/src/context/TeamContext.ts (1 hunks)
- libs/sr/page-team/src/context/index.ts (1 hunks)
- libs/sr/page-team/src/hooks/index.ts (1 hunks)
- libs/sr/page-team/src/hooks/useTeam.ts (1 hunks)
- libs/sr/page-team/src/index.tsx (5 hunks)
- libs/sr/page-teams/src/TeamCard.tsx (2 hunks)
- libs/sr/page-teams/src/TeamInventory.tsx (4 hunks)
- libs/sr/page-teams/src/index.tsx (1 hunks)
- libs/sr/solver/src/childWorker.ts (1 hunks)
- libs/sr/solver/src/parentWorker.ts (2 hunks)
- libs/sr/solver/src/solver.ts (6 hunks)
- libs/sr/ui/src/Character/CharacterCard.tsx (0 hunks)
- libs/sr/ui/src/Character/index.tsx (0 hunks)
- libs/sr/ui/src/Context/CharacterContext.tsx (1 hunks)
- libs/sr/ui/src/Context/LoadoutContext.tsx (0 hunks)
- libs/sr/ui/src/Context/index.tsx (0 hunks)
- libs/sr/ui/src/Hook/index.ts (0 hunks)
- libs/sr/ui/src/Hook/useLoadout.ts (0 hunks)
- libs/sr/ui/src/Provider/index.tsx (0 hunks)
- libs/sr/ui/src/Team/index.tsx (0 hunks)
- libs/sr/ui/src/index.tsx (0 hunks)
- tsconfig.base.json (1 hunks)
💤 Files with no reviewable changes (14)
- libs/sr/db/src/Database/DataManagers/LoadoutDataManager.ts
- libs/sr/db/src/Database/DataManagers/TeamDataManager.test.ts
- libs/sr/db/src/Database/DataManagers/index.ts
- libs/sr/db/src/Database/Database.ts
- libs/sr/page-team/src/TeamSettings/index.tsx
- libs/sr/ui/src/Character/CharacterCard.tsx
- libs/sr/ui/src/Character/index.tsx
- libs/sr/ui/src/Context/LoadoutContext.tsx
- libs/sr/ui/src/Context/index.tsx
- libs/sr/ui/src/Hook/index.ts
- libs/sr/ui/src/Hook/useLoadout.ts
- libs/sr/ui/src/Provider/index.tsx
- libs/sr/ui/src/Team/index.tsx
- libs/sr/ui/src/index.tsx
✅ Files skipped from review due to trivial changes (5)
- libs/sr/consts/src/relic.ts
- libs/sr/page-team/src/context/index.ts
- libs/sr/page-team/src/hooks/index.ts
- libs/sr/ui/src/Context/CharacterContext.tsx
- tsconfig.base.json
🧰 Additional context used
🪛 Biome
libs/sr/page-team/src/TeamSelectors.tsx
[error] 41-41: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
libs/sr/page-team/src/TeammateDisplay/Optimize/index.tsx
[error] 119-119: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
🔇 Additional comments (43)
libs/sr/page-teams/src/index.tsx (2)
Line range hint
4-10: Clean and focused component structure.The component follows React best practices with a clear single responsibility and minimal styling.
2-2: Verify local TeamInventory component.The import path change aligns with the PR's goal of restructuring team-related components. Let's verify the local component exists.
✅ Verification successful
Import path change is correct and verified
The local
TeamInventorycomponent exists atlibs/sr/page-teams/src/TeamInventory.tsx, and there are no remaining imports from the old@genshin-optimizer/sr/uipath. The import change is properly implemented.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the existence of TeamInventory component in the local directory # and ensure no other files are still importing it from @genshin-optimizer/sr/ui # Check if TeamInventory exists locally fd --type f "TeamInventory.tsx" --parent-dir "libs/sr/page-teams/src" # Check for any remaining imports from the old path rg "from ['|\"]@genshin-optimizer/sr/ui['|\"].*TeamInventory"Length of output: 404
Script:
#!/bin/bash # Let's try with correct fd syntax and expand our search fd "TeamInventory.tsx" "libs/sr/page-teams/src" # Search for any imports of TeamInventory from the old path rg "from ['\"]@genshin-optimizer/sr/ui['\"].*TeamInventory" # Also check if TeamInventory exists in the old location fd "TeamInventory.tsx" "libs/sr/ui" # Let's also check the content of the directory to understand the structure ls -la libs/sr/page-teams/src/Length of output: 518
libs/sr/page-team/src/context/TeamContext.ts (2)
1-2: LGTM! Clean and focused imports.The imports are well-organized, using type imports appropriately and including only the necessary React context utilities.
1-14: Verify context setup and usage across the codebase.Let's ensure the context is properly integrated and there are no circular dependencies.
✅ Verification successful
Let me gather more information about the usage of TeamContext to ensure it's properly integrated.
Based on the results, I can now provide a final response:
TeamContext implementation is properly integrated and used
The context setup follows React best practices with:
- Proper Provider setup in parent components (found in
libs/sr/page-team/src/index.tsx)- Consistent usage of
useTeamContexthook across multiple components- No circular dependencies detected (only one-way imports from
@genshin-optimizer/sr/db)🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify TeamContext setup and usage # Test 1: Check for TeamContext.Provider usage echo "Checking TeamContext.Provider usage..." rg "TeamContext.Provider" -A 2 # Test 2: Check for potential circular dependencies echo "Checking for potential circular dependencies..." rg -l "TeamContext" | xargs rg "from.*@genshin-optimizer/sr/(db|page-team)" # Test 3: Verify consistent import paths echo "Checking import consistency..." rg "from.*@genshin-optimizer/sr/page-team.*TeamContext"Length of output: 1442
Script:
#!/bin/bash # Check how TeamContext is being used in components echo "Checking useTeamContext usage..." rg "useTeamContext" -A 2 # Check the structure of components using TeamContext.Provider echo "Checking TeamContext.Provider parent components..." rg -B 5 "TeamContext.Provider" # Check if there are any other context-related imports or usage echo "Checking other context-related patterns..." rg "createContext|useContext" --type typescriptLength of output: 4883
libs/sr/page-team/src/TeammateDisplay/Optimize/BuildDisplay.tsx (2)
3-7: LGTM! Good consolidation of UI components.The migration of component imports to
@genshin-optimizer/sr/uialigns well with the PR's objective of streamlining the codebase. This centralization will improve maintainability and ensure consistency across the application.
Line range hint
10-35: LGTM! Clean and well-structured component implementation.The BuildDisplay component follows React best practices with:
- Proper TypeScript typing for props
- Efficient grid-based layout using MUI
- Clear conditional rendering logic
- Consistent key usage in the map function
Let's verify that these UI components are consistently imported from the new location across the codebase:
✅ Verification successful
Components are correctly imported from the UI package
The verification confirms that:
RelicCardandEmptyRelicCardare properly imported from@genshin-optimizer/sr/ui- Both components are correctly exported from the UI package through barrel files
- The imports are consistent with the component locations (
libs/sr/ui/src/Relic/)- No local relative imports are used in the BuildDisplay component
The review comment's approval was correct, and the component's implementation follows the project's architectural patterns.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify consistent import patterns for RelicCard components # Expected: All imports should be from @genshin-optimizer/sr/ui # Search for any remaining local imports of these components rg -l "import.*\{.*(RelicCard|EmptyRelicCard).*\}.*from '\.\./.*'" # Search for the new import pattern to confirm adoption rg -l "import.*\{.*(RelicCard|EmptyRelicCard).*\}.*from '@genshin-optimizer/sr/ui'"Length of output: 215
Script:
#!/bin/bash # Let's check the actual imports in the BuildDisplay component and related files # First, check the imports in BuildDisplay.tsx rg -A 5 "^import.*RelicCard" "libs/sr/page-team/src/TeammateDisplay/Optimize/BuildDisplay.tsx" # Then check where RelicCard and EmptyRelicCard are actually defined fd -e tsx -e ts "RelicCard.tsx|EmptyRelicCard.tsx" # Also check all imports of these components across the codebase rg "import.*\{.*RelicCard.*\}.*from" rg "import.*\{.*EmptyRelicCard.*\}.*from"Length of output: 475
Script:
#!/bin/bash # Let's check the actual imports in BuildDisplay.tsx since previous command didn't show them cat "libs/sr/page-team/src/TeammateDisplay/Optimize/BuildDisplay.tsx" # Also check the exports from the UI module to verify these components are properly exported cat "libs/sr/ui/src/index.ts"Length of output: 964
Script:
#!/bin/bash # Let's check the exports in the UI package fd -e ts -e tsx index "libs/sr/ui/src" -x cat {} # Also check if there are any barrel files in the Relic directory fd -e ts -e tsx index "libs/sr/ui/src/Relic" -x cat {}Length of output: 1463
libs/sr/page-team/src/TeammateDisplay/Optimize/OptConfigWrapper.tsx (1)
1-5: LGTM! Imports are well-organized.The imports are properly structured, using specific imports rather than whole module imports, which is good for tree-shaking.
libs/sr/page-team/src/TeammateDisplay/Optimize/StatFilterCard.tsx (2)
3-3: LGTM! Import changes align with the new architecture.The changes correctly reflect the shift from loadout-based to team-based management by introducing the necessary context and database hooks.
Also applies to: 7-7
Line range hint
1-50: Verify integration with the new team-based system.The component changes look good, but let's verify its integration with other components in the new team-based system.
✅ Verification successful
Let me gather more information about the similar component found in the Genshin Impact (gi) module to ensure proper alignment.
Integration with team-based system is properly implemented
The verification shows that:
- No remaining references to
LoadoutContextexist, confirming complete migration- Both GI and SR implementations follow similar patterns but with appropriate module-specific contexts:
- SR uses
OptConfigContextfor optimization configuration- GI uses
TeamCharacterContextwithuseOptConfighook- Both implementations maintain the same core functionality for stat filtering while properly integrating with their respective team management systems
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the component's integration and usage patterns # Check for any remaining references to LoadoutContext echo "Checking for any remaining LoadoutContext references..." rg "LoadoutContext" -l # Verify consistent usage of OptConfigContext echo "Checking OptConfigContext usage patterns..." ast-grep --pattern 'useContext(OptConfigContext)' # Look for similar components that might need the same updates echo "Finding similar stat filter components..." fd -e tsx -e ts -t f ".*Filter.*Card\.tsx$"Length of output: 800
Script:
#!/bin/bash # Compare the implementation of both stat filter components echo "Comparing StatFilterCard implementations..." echo "=== SR StatFilterCard ===" cat libs/sr/page-team/src/TeammateDisplay/Optimize/StatFilterCard.tsx echo -e "\n=== GI StatFilterCard ===" cat libs/gi/page-team/src/CharacterDisplay/Tabs/TabOptimize/Components/StatFilterCard.tsx # Check the context usage in GI component echo -e "\n=== Checking context usage in GI optimize components ===" rg "useContext" libs/gi/page-team/src/CharacterDisplay/Tabs/TabOptimize/ -A 2Length of output: 19623
libs/sr/page-teams/src/TeamCard.tsx (2)
3-3: LGTM: Import path update aligns with project structure.The change from relative to absolute import path improves maintainability and reflects the new project organization.
56-59: Verify type safety for teamMetadata structure.The code assumes
cmdatahas acharacterKeyproperty but there's no visible type checking.Let's verify the type definitions:
✅ Verification successful
Based on the gathered information, I can now provide a final response:
Type safety for teamMetadata is properly implemented
The code is type-safe as
teamMetadatais defined in theTeaminterface asArray<TeamMetaDataum | undefined>whereTeamMetaDataumis a type that explicitly includes thecharacterKeyproperty of typeCharacterKey. TheCharacterKeytype is derived from a const array of valid character keys, ensuring type safety at compile time.
Teaminterface inlibs/sr/db/src/Database/DataManagers/TeamDataManager.tsproperly types theteamMetadataarrayTeamMetaDataumtype in the same file includescharacterKey: CharacterKeyCharacterKeyis a union type derived fromallCharacterKeysconst array inlibs/sr/consts/src/character.ts🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for TeamMetadata type definition ast-grep --pattern 'type TeamMetadata = $_' # Search for related interfaces ast-grep --pattern 'interface Team { $$$ teamMetadata: $_ $$$ }'Length of output: 7736
Script:
#!/bin/bash # Search for TeamMetaDataum type definition ast-grep --pattern 'type TeamMetaDataum = $_' ast-grep --pattern 'interface TeamMetaDataum { $$$ }' # Search for characterKey usage in TeamMetaDataum rg "characterKey.*TeamMetaDataum" -A 5Length of output: 6777
Script:
#!/bin/bash # Let's verify the CharacterKey type since it's used in TeamMetaDataum ast-grep --pattern 'type CharacterKey = $_' # Also check how TeamMetaDataum is used in TeamCard.tsx rg -A 5 "TeamMetaDataum" "libs/sr/page-teams/src/TeamCard.tsx"Length of output: 5914
Script:
#!/bin/bash # Let's check the allCharacterKeys constant since CharacterKey is derived from it ast-grep --pattern 'const allCharacterKeys = $_' # Also check the Team type import in TeamCard.tsx rg "import.*Team.*from" "libs/sr/page-teams/src/TeamCard.tsx"Length of output: 6212
libs/sr/page-team/src/TeamSelectors.tsx (3)
1-24: LGTM! Clean implementation of the TeamSelectors component.The component follows React best practices with proper typing and efficient list rendering. The key generation strategy using both index and characterKey helps React's reconciliation process.
75-80: LGTM! Clean component rendering.The component's render logic is clean and properly passes the required props to CharacterAutocomplete.
58-61: 🛠️ Refactor suggestionAvoid type assertion and add validation.
The type assertion
as TeamMetaDataumbypasses TypeScript's type checking. Consider creating a proper validation function for the metadata object.Consider implementing proper validation:
function createTeamMetadata(characterKey: CharacterKey): TeamMetaDataum { const metadata: TeamMetaDataum = { characterKey, // Add other required properties with defaults }; return metadata; } // Usage: team.teamMetadata[teamMetadataIndex] = createTeamMetadata(cKey);libs/common/util/src/lib/array.ts (1)
85-91: Verify usage patterns of this mutating function.Since this function modifies arrays in-place and is part of a UI redesign, we should verify its usage to prevent unintended side effects.
✅ Verification successful
Let me gather more context about the usage in TeamDataManager.ts to better understand the data flow and mutation patterns.
Based on the gathered information, I can now provide a final response:
In-place array modification is used safely in validation context
The
pruneOrPadArrayfunction is only used within thevalidatemethod ofTeamDataManagerwhere in-place modification is acceptable since:
- It's used during data validation/normalization of incoming data
- The arrays being modified are already filtered/transformed copies (
conditional.filter(),bonusStats.filter(),statConstraints.filter())- The modifications ensure consistent array lengths for frame-based data structures
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for usages of pruneOrPadArray to verify: # 1. That callers are aware of the in-place modification # 2. That it's not used directly on props or state in React components # Find direct usage in components echo "Checking usage in React components..." rg -t tsx "pruneOrPadArray" -B 2 -A 2 # Find usage in utility functions echo "Checking usage in other utility functions..." rg -t ts "pruneOrPadArray" -B 2 -A 2Length of output: 2664
Script:
#!/bin/bash # Get more context around the TeamDataManager usage echo "Checking TeamDataManager class structure and data flow..." ast-grep --pattern 'class TeamDataManager { $$$ validate($$$) { $$$ } $$$ }' # Check if the arrays being modified are cloned/copied before modification rg -l "\.slice\(\)|\.concat\(\)|\.spread|\.map\(" libs/sr/db/src/Database/DataManagers/TeamDataManager.ts # Look for comments or documentation about data handling rg -B 2 "pruneOrPadArray|validate" libs/sr/db/src/Database/DataManagers/TeamDataManager.tsLength of output: 1799
libs/sr/page-teams/src/TeamInventory.tsx (4)
6-6: LGTM! Good practice using absolute imports.The change from relative to absolute import path for
useDatabaseContextimproves maintainability and aligns with the codebase restructuring.
44-47: LGTM! Consistent variable naming.The rename from
teamsIdsToShowtoteamIdsToShowimproves naming consistency across the codebase.
Line range hint
82-93: LGTM! Proper infinite scroll implementation.The skeleton loader implementation correctly uses the renamed variable and provides good visual feedback during loading.
Line range hint
70-79: Verify error handling for empty team lists.While the mapping logic is correct, consider adding a fallback UI for when there are no teams to display.
libs/sr/page-team/src/ComboEditor.tsx (2)
1-13: LGTM! Well-organized imports.The imports are logically grouped and all dependencies are properly utilized in the code.
15-100: Overall implementation aligns well with the PR objectives.The ComboEditor component successfully implements the new UI design philosophy described in the PR objectives. It provides a clean interface for managing team combos while maintaining good separation of concerns and component structure.
While there are some opportunities for optimization and enhancement, the core functionality is solid and well-implemented.
Let's verify the component's integration with the team context:
✅ Verification successful
Component integration with TeamContext is properly implemented
The verification shows that:
- The
useTeamContexthook is correctly imported from the context module- The hook is properly used to access
teamandteamIdin the ComboEditor component- The TeamContext is well-defined in
libs/sr/page-team/src/context/TeamContext.ts- The context is consistently used across multiple components in the codebase including BonusStats, TalentContent, and TeammateDisplay
- The context provider is properly set up in the parent component (
libs/sr/page-team/src/index.tsx)🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify the TeamContext usage across the codebase rg -A 3 "useTeamContext|TeamContext.Provider" --type tsxLength of output: 86
Script:
#!/bin/bash # Let's try searching without file type restriction since tsx extension might not be recognized rg -A 3 "useTeamContext|TeamContext.Provider" # Also search for the TeamContext definition rg -A 5 "createContext.*Team" # And look for files with Team in their name that might contain the context fd -e ts -e tsx "Team"Length of output: 7275
libs/sr/page-team/src/TeammateDisplay/index.tsx (3)
22-25: LGTM! Import changes align with team-based restructuring.The reorganization of imports reflects the architectural shift from loadout-based to team-based management.
28-28: LGTM! Simplified component signature.The removal of the tab parameter aligns with the new single-page UI design philosophy.
30-31: Verify error handling for undefined characterKey.While the switch to teamContext aligns with the new design, we should ensure proper handling of cases where characterKey might be undefined.
Let's check how characterKey is used throughout the codebase:
libs/sr/page-team/src/TeamCharacterSelector.tsx (2)
8-8: LGTM: Clean transition to team-based managementThe introduction of the
useTeamhook and retention of the database context shows a clean transition from direct database access to a more abstracted team-based management approach.Also applies to: 29-30
146-146: LGTM: Clean integration of TeamSelectorsThe addition of TeamSelectors within the modal provides a cohesive UI for team management, aligning well with the PR's objectives.
libs/sr/solver/src/parentWorker.ts (1)
Line range hint
1-200: Consider architectural changes for team combo system.Given the PR's objective to implement a new team combo system where builds can be referenced from any team, consider the following architectural improvements:
- The
BuildResulttype might need to store team context or combo information to support build sharing across teams.- The
MAX_BUILDSconstant might need to be adjusted to account for team-specific build limits.- The optimization algorithm might benefit from team-aware filtering to prioritize builds that work well in multiple team compositions.
Let's verify the current usage of BuildResult type:
libs/sr/db/src/Database/DataManagers/OptConfigDataManager.ts (2)
8-8: LGTM! Improved type safety for main stat keys.The changes improve type safety by deriving types directly from the
relicSlotToMainStatKeysconstant, ensuring better maintainability and preventing potential type mismatches.Also applies to: 52-55
52-55: Verify the removal of optimizationTarget.The validation logic has been updated to remove optimizationTarget handling. Let's verify there are no remaining references to this property in the codebase.
Also applies to: 89-147
libs/sr/solver/src/solver.ts (9)
1-1: Addition of 'sum' importThe import of
sumfrom'@genshin-optimizer/pando/engine'is appropriate and reflects its usage in the code.
4-4: Importing 'Team' typeIncluding
Teamfrom'@genshin-optimizer/sr/db'aligns with the use ofTeam['frames']in the class.
5-10: Adding 'Preset' and other importsThe addition of
Read,Calculator,Preset, andTagimports from'@genshin-optimizer/sr/formula'is necessary for the updated functionality.
29-29: Adding 'frames' property to 'Solver' classDefining
private frames: Team['frames']correctly updates the class to handle multiple frames.
40-40: Updating constructor to accept 'frames'Replacing
optTargetwithframesin the constructor parameters reflects the change in class properties and is appropriate.
48-48: Assigning 'frames' in constructorThe statement
this.frames = framesproperly initializes the newframesproperty.
72-72: Using 'console.error' for error loggingReplacing
console.logwithconsole.errorenhances error reporting by properly logging errors.
108-108: Consistent error logging in 'terminate' methodUsing
console.errorin theterminatemethod ensures errors are appropriately handled and logged.
123-129: Summing frames in 'detachNodes' methodThe implementation correctly sums over
this.frames, creatingReadinstances with associatedpresetidentifiers. This approach effectively detaches nodes corresponding to team frames.libs/sr/page-team/src/TeamCalcProvider.tsx (2)
87-103: Ensure Correct Handling of Conditionals and Bonus StatsThe mapping over
team.conditionalsandteam.bonusStatsappears correct. Verify that thecondValuesandvaluesarrays correctly align with the expectedpresetIndexvalues to prevent any potential mismatches.
110-117: ConfirmcalcWithTagNull Handling LogicEnsure that
calcWithTagcorrectly handles cases wherecalcmight be null or undefined. The use of?? nullshould handle this scenario, but please confirm that this aligns with the intended behavior.libs/sr/page-team/src/BonusStats.tsx (1)
88-89: Verify the usage of 'src' and 'dst' in 'newTag' functionIn the
newTagfunction, bothsrcanddstare assigned the samemembervalue:return { src: member, dst: member, et: 'own', q, qt: 'premod', sheet: 'agg', }Ensure that this assignment aligns with the intended logic of the application. If
srcanddstare supposed to represent different entities, setting them to the same value might lead to unexpected behavior.Please confirm that using the same
memberfor bothsrcanddstis correct in this context.libs/sr/page-team/src/index.tsx (1)
85-87: Ensure navigation logic doesn't cause unintended redirectsIn the
useEffectat lines 85-87, whencharacterKeyandcharacterKeyRawdo not match, the application navigates to the correctcharacterKey. Please verify that this logic doesn't result in an infinite redirect loop or unexpected navigation behaviors, especially ifcharacterKeyorteamMetadatumupdate frequently.libs/sr/page-team/src/TalentContent.tsx (1)
297-309: Verify the parameters passed todatabase.teams.setConditional.Ensure that the parameters in the
setConditionalfunction are correctly ordered and match the expected types. Specifically, confirm that:
sheetKeyandsrcKeyare correctly assigned and correspond to valid keys.condKeyandcondValueare accurate and will not cause unintended side effects.Run the following script to check all usages of
database.teams.setConditionalfor consistency:✅ Verification successful
Let me gather more information about the
setConditionalfunction definition to verify the parameter types and order.
Let me search specifically for the TeamDataManager's setConditional method definition to verify the parameter types and order.
Parameters in
database.teams.setConditionalare correctly ordered and typedThe implementation in
TalentContent.tsxcorrectly matches the method signature inTeamDataManager.ts:
teamId(string)sheetKey as SheetsrcKey as MembercharacterKey(as dst Member)condKey(string)condValue(number)presetIndex(number as frameIndex)The type casts to
SheetandMemberare appropriate as these are valid type aliases defined in the codebase.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify parameter usage for database.teams.setConditional # Search for all calls to database.teams.setConditional and display their parameters rg "database\.teams\.setConditional\(" -A 3Length of output: 311
Script:
#!/bin/bash # Search for the definition of setConditional in database.teams ast-grep --pattern 'setConditional($$$) { $$$ }' # Also search for any type definitions or interfaces that might define this method rg "setConditional" -B 2 -A 2Length of output: 13150
Script:
#!/bin/bash # Get the full definition of setConditional in TeamDataManager rg "setConditional\(" -A 10 libs/sr/db/src/Database/DataManagers/TeamDataManager.ts # Also search for any type definitions of Sheet and Member rg "type (Sheet|Member) =" -A 3Length of output: 818
| export const PresetContext = createContext<PresetContextObj>({ | ||
| presetIndex: 0, | ||
| setPresetIndex: () => {}, | ||
| }) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance error handling for the default context.
The current default setter is a no-op function, which could make debugging difficult if a component tries to use the context without a provider.
Consider adding a development-only warning:
export const PresetContext = createContext<PresetContextObj>({
presetIndex: 0,
- setPresetIndex: () => {},
+ setPresetIndex: () => {
+ if (process.env.NODE_ENV !== 'production') {
+ console.warn(
+ 'Attempted to call setPresetIndex without a PresetContext.Provider'
+ )
+ }
+ },
})📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export const PresetContext = createContext<PresetContextObj>({ | |
| presetIndex: 0, | |
| setPresetIndex: () => {}, | |
| }) | |
| export const PresetContext = createContext<PresetContextObj>({ | |
| presetIndex: 0, | |
| setPresetIndex: () => { | |
| if (process.env.NODE_ENV !== 'production') { | |
| console.warn( | |
| 'Attempted to call setPresetIndex without a PresetContext.Provider' | |
| ) | |
| } | |
| }, | |
| }) |
| export type PresetContextObj = { | ||
| presetIndex: number | ||
| setPresetIndex: (presetIndex: number) => void | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding parameter validation for presetIndex.
The type definition could benefit from runtime validation to ensure presetIndex remains within valid bounds, especially since this is part of the UI redesign where invalid indices could lead to rendering issues.
Consider wrapping the setter with validation:
export type PresetContextObj = {
presetIndex: number
- setPresetIndex: (presetIndex: number) => void
+ setPresetIndex: (presetIndex: number) => void
+ isValidPresetIndex: (index: number) => boolean
}Committable suggestion was skipped due to low confidence.
| export function useTeamContext() { | ||
| return useContext(TeamContext) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for missing context provider.
The hook should handle cases where it's used outside of a TeamContext.Provider. Consider adding a runtime check to provide a better developer experience.
export function useTeamContext() {
- return useContext(TeamContext)
+ const context = useContext(TeamContext)
+ if (!context) {
+ throw new Error('useTeamContext must be used within a TeamContext.Provider')
+ }
+ return context
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export function useTeamContext() { | |
| return useContext(TeamContext) | |
| } | |
| export function useTeamContext() { | |
| const context = useContext(TeamContext) | |
| if (!context) { | |
| throw new Error('useTeamContext must be used within a TeamContext.Provider') | |
| } | |
| return context | |
| } |
| teamMetadatum: TeamMetaDataum | ||
| } | ||
|
|
||
| export const TeamContext = createContext({} as TeamContextObj) |
There was a problem hiding this comment.
Add a meaningful default value to prevent potential runtime errors.
Creating the context with an empty object could lead to runtime errors if the context is accessed outside of a provider. Consider adding a default value that represents an "empty" or "loading" state.
-export const TeamContext = createContext({} as TeamContextObj)
+export const TeamContext = createContext<TeamContextObj>({
+ teamId: '',
+ team: { /* default team structure */ },
+ teamMetadatum: { /* default metadata structure */ }
+})Committable suggestion was skipped due to low confidence.
| if (!calc) return | ||
| const cancelled = new Promise<void>((r) => (cancelToken.current = r)) |
There was a problem hiding this comment.
Avoid assignment within expressions for better readability
Assigning cancelToken.current = r within the Promise constructor's executor function can be confusing and reduce code clarity. Consider refactoring to separate the assignment from the expression.
Apply this diff to improve code readability:
- const cancelled = new Promise<void>((r) => (cancelToken.current = r))
+ const cancelled = new Promise<void>((resolve) => {
+ cancelToken.current = resolve
+ })📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (!calc) return | |
| const cancelled = new Promise<void>((r) => (cancelToken.current = r)) | |
| if (!calc) return | |
| const cancelled = new Promise<void>((resolve) => { | |
| cancelToken.current = resolve | |
| }) |
🧰 Tools
🪛 Biome
[error] 119-119: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
| teamMetadatum: { characterKey }, | ||
| } = useTeamContext() |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider refactoring duplicated extraction of characterKey.
The extraction of characterKey using useTeamContext() is repeated in multiple components (CharacterTalentPane, SkillDisplayCard, and EidolonDropdown). To improve maintainability and reduce code duplication, consider creating a custom hook or utility function that encapsulates this logic.
| const { teamId } = useContext(TeamContext) | ||
| const { presetIndex } = useContext(PresetContext) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Optimize context usage to prevent unnecessary re-renders.
Both SkillDisplayCard and EidolonDropdown components access teamId and characterKey from context. Ensure that only the necessary parts of the context are consumed to prevent unnecessary re-renders when unrelated context values change.
Also applies to: 357-358
There was a problem hiding this comment.
Actionable comments posted: 13
🧹 Outside diff range and nitpick comments (46)
libs/sr/db-ui/src/hooks/useLightCone.ts (1)
4-7: Add JSDoc documentation and consider type safety improvements.The hook implementation could benefit from:
- JSDoc documentation explaining its purpose, parameters, and return type
- Explicit handling of invalid
lightConeIdvaluesConsider adding documentation and improving type safety:
+/** + * Hook to manage light cone data from the database. + * @param lightConeId - The ID of the light cone to retrieve, or undefined + * @returns The light cone data manager instance + */ -export function useLightCone(lightConeId: string | undefined) { +export function useLightCone(lightConeId: string | '' | undefined) { const { database } = useDatabaseContext() + if (lightConeId && !database.lightCones.get(lightConeId)) { + console.warn(`Light cone with ID ${lightConeId} not found`) + } return useDataManagerBase(database.lightCones, lightConeId ?? '') }libs/sr/db-ui/src/hooks/useCharacter.ts (2)
1-8: Add JSDoc documentation.The hook lacks documentation explaining its purpose, parameters, and return value.
Add documentation to improve usability:
+/** + * Hook to manage character data from the database. + * @param characterKey - The key identifying the character. Can be empty string or undefined. + * @returns The character data manager if a valid key is provided, undefined otherwise. + */ export function useCharacter(characterKey: CharacterKey | '' | undefined) { const { database } = useDatabaseContext() return useDataManagerBase(database.chars, characterKey as CharacterKey) }
1-8: Good architectural decision moving this hook to db-ui layer.Moving this hook from the UI layer to the db-ui layer improves separation of concerns by keeping database-related logic closer to the database layer.
Consider documenting this architectural decision in the project's architecture documentation to help other developers understand the rationale behind the layering.
libs/sr/ui/src/Character/AbilityDropdown.tsx (1)
8-8: LGTM! Good move towards centralization.The consolidation of database-related hooks into
@genshin-optimizer/sr/db-uialigns well with modular architecture principles. This change improves maintainability by centralizing database access patterns.Consider documenting the centralized hooks in the module's README to help other developers understand the available database interactions.
libs/sr/db-ui/src/provider/DatabaseProvider.tsx (1)
Line range hint
10-47: Consider adding JSDoc documentation.The
DatabaseProvidercomponent serves a crucial role in managing database state and context. Consider adding JSDoc documentation to describe:
- The purpose and responsibility of the component
- The structure of the database context
- The significance of the database indexing
Add documentation like this:
+/** + * Provider component that manages multiple database instances and their states. + * Handles both local and sandbox storage types, with support for up to 4 databases. + * Uses localStorage for persistence and provides database context to child components. + * @param {Object} props - Component props + * @param {ReactNode} props.children - Child components that will have access to the database context + */ export function DatabaseProvider({ children }: { children: ReactNode }) {libs/sr/db/src/Database/DataManagers/BuildDataManager.ts (1)
Line range hint
1-65: Architectural Review: Build Reference ManagementGiven the PR's objective to allow builds to be "referenced from any team", the current implementation needs additional architectural considerations:
- Reference Tracking: The class currently has no mechanism to track which teams are using a build
- Validation: No validation exists to ensure referenced builds exist when loading team data
- Migration: No migration strategy for existing build-team relationships
Recommendations:
- Add methods to track build references:
interface BuildReference { teamId: string; characterId: string; timestamp: number; } class BuildDataManager { private buildReferences: Map<string, BuildReference[]>; addReference(buildId: string, reference: BuildReference): void; removeReference(buildId: string, teamId: string): void; getReferences(buildId: string): BuildReference[]; }
- Implement validation in the database layer:
validateTeamBuildReferences(teamData: TeamData): void { // Verify all referenced builds exist // Clean up invalid references }Would you like me to create a detailed design document for this architecture?
libs/sr/ui/src/Character/CharacterAutocomplete.tsx (2)
Line range hint
52-57: Consider implementing the image functionality.The
toImgprop is currently returning an empty fragment. This might affect the user experience as character images are typically helpful in autocomplete components.Consider implementing proper character image rendering:
- toImg={() => <> </>} // TODO + toImg={(key) => key ? ( + <CharacterIcon characterKey={key} size={24} /> + ) : null}
Line range hint
53-53: Track the TODO comment for image implementation.The TODO comment for image implementation should be addressed as part of the UI redesign.
Would you like me to create a GitHub issue to track the implementation of character images in the autocomplete component?
libs/sr/ui/src/Character/LocationAutocomplete.tsx (2)
Line range hint
63-63: Address TODO comment for image implementation.The TODO comment indicates missing image implementation in the
toImgprop. This should be addressed to ensure proper visual feedback in the autocomplete component.Would you like me to help implement the image rendering functionality or create a GitHub issue to track this task?
Line range hint
41-52: Consider memoizing callback dependencies.The
charInDbandcharIsFavoritecallbacks depend on database objects that might trigger unnecessary re-renders if the database reference changes frequently.Consider memoizing the database dependencies:
+ const chars = useMemo(() => database.chars, [database.chars]) + const charMeta = useMemo(() => database.charMeta, [database.charMeta]) const charInDb = useCallback( (characterKey: CharacterKey) => - !!database.chars.get(characterKey), + !!chars.get(characterKey), - [database.chars] + [chars] ) const charIsFavorite = useCallback( (characterKey: CharacterKey) => - database.charMeta.get(characterKey).favorite, + charMeta.get(characterKey).favorite, - [database.charMeta] + [charMeta] )libs/sr/ui/src/Character/CharacterInventory.tsx (4)
Line range hint
52-56: Fix typo in Skeleton component's width prop.There's a typo in the width prop of the Skeleton component.
- sx={{ widht: '100%', height: '100%', minHeight: 300 }} + sx={{ width: '100%', height: '100%', minHeight: 300 }}
Line range hint
22-35: Consider memoizing size-dependent calculations.The character slice operations and size calculations are being performed on every render. Consider memoizing these values to optimize performance, especially for large character lists.
+ const columnsForSize = useMemo(() => columns[size], [size]); + const itemsPerPage = useMemo(() => amtPerSize[size], [size]); const { numShow, setTriggerElement } = useInfScroll( - amtPerSize[size], + itemsPerPage, characters.length )
Line range hint
73-79: Enhance accessibility for character selection.Consider adding ARIA labels and keyboard navigation support for better accessibility. The character cards should be navigable via keyboard.
<CharacterCard character={c} onClick={() => setCharacterKey(c.key)} + role="button" + tabIndex={0} + aria-label={`Select ${c.name}`} + onKeyPress={(e) => e.key === 'Enter' && setCharacterKey(c.key)} />
Line range hint
65-89: Consider adding loading state indicators for better UX.While the infinite scroll implementation is good, consider adding visual feedback during data loading and scroll operations.
<Grid container spacing={1} columns={columns}> + {charactersInView.length === 0 && ( + <Grid item xs={12}> + <Box textAlign="center" py={2}> + <Typography color="textSecondary"> + Loading characters... + </Typography> + </Box> + </Grid> + )} {charactersInView.map((c, i) => ( // ... existing code ))} </Grid>libs/sr/ui/src/Relic/RelicInventory.tsx (3)
Line range hint
33-37: Consider optimizing database subscription cleanupWhile the database subscription is correctly set up, ensure the cleanup function is properly handled by returning the unsubscribe callback from useEffect.
useEffect( - () => database.relics.followAny(setDirtyDatabase), + () => { + const unsubscribe = database.relics.followAny(setDirtyDatabase) + return () => unsubscribe() + }, [database, setDirtyDatabase] )
Line range hint
39-45: Consider adding explicit type definitions for memoized valuesWhile the code functions correctly, adding explicit TypeScript interfaces would improve type safety and documentation.
+ interface RelicState { + relicIds: string[]; + totalRelicsNum: number; + } const { relicIds, totalRelicsNum } = useMemo(() => { const relics = database.relics.values const totalRelicsNum = relics.length const relicIds = relics.map((r) => r.id) - return dirtyDatabase && { relicIds, totalRelicsNum } + return dirtyDatabase ? { relicIds, totalRelicsNum } as RelicState : { relicIds: [], totalRelicsNum: 0 } }, [database, dirtyDatabase])
Line range hint
89-98: Enhance accessibility for the add buttonThe add button could benefit from improved accessibility attributes.
<Button fullWidth onClick={onAdd} color="info" startIcon={<AddIcon />} + aria-label={t('addNew')} + role="button" > {t('addNew')} </Button>libs/sr/ui/src/LightCone/LightConeInventory.tsx (3)
Line range hint
39-52: Consider persisting sort/filter preferences.The code comments indicate that sort/filter options storage is planned but not implemented. Given this PR's focus on UI redesign and improved user experience, consider implementing this feature to enhance usability.
Would you like me to propose an implementation for storing UI preferences in the database?
Line range hint
91-106: Consider adding loading state feedback for inventory actions.Given the UI redesign context, the component could benefit from visual feedback during operations like adding/editing/deleting light cones. Consider wrapping these operations with loading states to improve user experience.
Example implementation:
<Button fullWidth onClick={onAdd} color="info" + disabled={isLoading} startIcon={<AddIcon />} > - {t('addNew')} + {isLoading ? t('adding') : t('addNew')} </Button>
Line range hint
107-147: Consider implementing virtualization for better performance.The current implementation loads all items and uses infinite scroll. For better performance with large inventories, consider implementing virtualization using a library like
react-windoworreact-virtualized.This would be particularly beneficial as part of the UI redesign, especially when users have many light cones in their inventory.
libs/sr/page-team/src/TeamCalcProvider.tsx (1)
129-129: Track TODO comment for TC build handling.The TODO comment indicates missing functionality for handling TC builds.
Would you like me to create a GitHub issue to track the implementation of TC build handling?
libs/sr/page-team/src/TeammateDisplay/index.tsx (2)
46-46: Consider enhancing UX for ComboEditor integrationThe ComboEditor integration aligns with the new team combo design. Consider these UX improvements:
- Add loading states for async operations
- Implement error boundaries
- Include ARIA attributes for accessibility
Example implementation:
- <ComboEditor /> + <ErrorBoundary fallback={<ErrorMessage />}> + <Suspense fallback={<LoadingSpinner />}> + <ComboEditor aria-label="Team Combo Editor" /> + </Suspense> + </ErrorBoundary>
Line range hint
58-124: Consider extracting debug accordion into a reusable componentThe debug sections for "target listings" and "target buffs" share similar code patterns. Consider extracting this into a reusable component to improve maintainability.
Example implementation:
type DebugAccordionProps = { title: string; formulas: Array<any>; // Type should match calc.listFormulas return type calc: any; // Type should match useSrCalcContext return type }; function DebugAccordion({ title, formulas, calc }: DebugAccordionProps) { return ( <Accordion> <AccordionSummary expandIcon={<ExpandMoreIcon />}> All {title} </AccordionSummary> <AccordionDetails> <Stack> {formulas.map((read, index) => { const computed = calc.compute(read); const name = read.tag.name || read.tag.q; return ( <Box key={`${name}${index}`}> {/* ... rest of the debug UI ... */} </Box> ); })} </Stack> </AccordionDetails> </Accordion> ); }Usage:
- <Accordion> - <AccordionSummary expandIcon={<ExpandMoreIcon />}> - All target listings - </AccordionSummary> - {/* ... */} - </Accordion> - <Accordion> - <AccordionSummary expandIcon={<ExpandMoreIcon />}> - All target buffs - </AccordionSummary> - {/* ... */} - </Accordion> + <DebugAccordion + title="target listings" + formulas={calc?.listFormulas(own.listing.formulas)} + calc={calc} + /> + <DebugAccordion + title="target buffs" + formulas={calc?.listFormulas(own.listing.buffs)} + calc={calc} + />libs/sr/page-team/src/TeamCharacterSelector.tsx (2)
145-145: Document the TeamSelectors component's purpose.The newly added TeamSelectors component lacks documentation. Please add JSDoc comments explaining its role in the team management UI redesign.
Line range hint
31-180: Implementation aligns well with UI redesign goals.The changes successfully implement the new team-centric approach, making teams represent singular combos as intended. The UI is more consolidated, with improved team management capabilities through the modal interface.
Consider extracting the team editing modal into a separate component to improve maintainability and reusability.
libs/sr/ui/src/Character/CharacterEditor.tsx (1)
Line range hint
41-144: Consider UX improvements for the character editor.The implementation is solid and well-structured. Consider these UX enhancements:
- Add validation feedback for level/ascension input fields
- Consider grouping related abilities visually
- Add tooltips to explain bonus abilities and stat boosts
Example enhancement for input validation:
<TextField type="number" label="Level" variant="outlined" inputProps={{ min: 1, max: 90 }} value={character?.level || 0} + error={character?.level < 1 || character?.level > 90} + helperText={character?.level < 1 || character?.level > 90 ? "Level must be between 1 and 90" : ""} onChange={(e) => database.chars.set(characterKey, { level: parseInt(e.target.value), }) } disabled={!character} />libs/sr/page-team/src/BonusStats.tsx (2)
46-51: Add error handling for team context.Consider adding error handling for cases where team context values might be undefined, especially since this is part of a significant architectural change.
const { presetIndex } = useContext(PresetContext) const { teamId, team: { bonusStats }, teamMetadatum: { characterKey }, - } = useTeamContext() + } = useTeamContext() + if (!teamId || !bonusStats || !characterKey) { + return <div>Error: Team context not properly initialized</div> + }
Line range hint
86-93: Add documentation for tag creation.Consider adding JSDoc comments to explain the tag structure and its purpose, especially since this is part of a new team-based architecture.
+ /** + * Creates a new bonus stat tag for a team member + * @param q - The qualifier for the bonus stat + * @param member - The team member to apply the bonus to + * @returns A tag object with source and destination set to the same member + */ function newTag(q: Tag['q'], member: Member): Tag { return { src: member, dst: member, et: 'own', q, qt: 'premod', sheet: 'agg', } }libs/sr/page-team/src/index.tsx (4)
52-59: Simplify context object creation.The preset context object creation can be simplified by removing the type assertion since TypeScript can infer the type.
- const presetObj = useMemo( - () => - ({ - presetIndex, - setPresetIndex, - } as PresetContextObj), - [presetIndex, setPresetIndex] - ) + const presetObj = useMemo( + () => ({ + presetIndex, + setPresetIndex, + }), + [presetIndex, setPresetIndex] + )
132-142: Clean up or restore commented styling code.There's a block of commented styling code that should either be removed if it's no longer needed or restored if it's still required. This appears to be related to element-based background styling.
Please either:
- Remove the commented code if it's no longer needed
- Restore and fix the styling if it's still required
- Add a comment explaining why it's temporarily disabled if you plan to restore it later
84-86: Optimize navigation effect dependencies.The effect's dependency array includes
teamMetadatumwhich isn't used in the effect body.useEffect(() => { if (characterKey && characterKey !== characterKeyRaw) navigate(`${characterKey}`, { replace: true }) - }, [characterKey, characterKeyRaw, teamMetadatum, navigate]) + }, [characterKey, characterKeyRaw, navigate])
Line range hint
1-173: Implementation aligns well with PR objectives.The code successfully implements the new team-based UI architecture with the following highlights:
- Proper context management for team and preset data
- Sticky header implementation for quick access
- Clean separation of concerns between team and character components
Consider documenting the new team-based architecture in the codebase to help other developers understand the design decisions and data flow.
libs/sr/page-team/src/TeammateDisplay/Optimize/index.tsx (3)
Line range hint
72-164: Consider splitting component responsibilitiesThe
OptimizeWrappercomponent handles multiple concerns including:
- Worker management
- Optimization state
- Progress tracking
- Result display
Consider splitting these into smaller, focused components for better maintainability.
Would you like me to provide an example of how to break this down into smaller components?
🧰 Tools
🪛 Biome
[error] 120-120: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
Line range hint
147-156: Optimize dependency array for useCallbackThe dependency on the entire
teamobject inonOptimize's dependency array could cause unnecessary re-renders. Consider extracting only the required properties.Apply this diff:
}, [ calc, characterKey, - team, + team.frames, numWorkers, optConfig.statFilters, relicsBySlot, ])
Line range hint
201-227: Consider memoizing ProgressIndicator componentSince this component receives frequently changing progress data during optimization, consider memoizing it to prevent unnecessary re-renders.
Apply this diff:
-function ProgressIndicator({ +const ProgressIndicator = React.memo(function ProgressIndicator({ progress, totalPermutations, }: { progress: ProgressResult totalPermutations: number }) { const { t } = useTranslation('optimize') return ( // ... component implementation ... ) -} +})libs/sr/ui/src/Settings/DatabaseCard.tsx (3)
Line range hint
89-95: Consider enhancing error handling for clipboard operations.While the error is logged to console, consider providing user feedback for both success and failure cases using a more modern notification system instead of
alert().const copyToClipboard = useCallback( () => navigator.clipboard .writeText(JSON.stringify(database.exportSROD())) - .then(() => alert('Copied database to clipboard.')) + .then(() => { + // Consider using a toast/snackbar notification system + notificationSystem.success('Database copied to clipboard'); + }) - .catch(console.error), + .catch((error) => { + console.error(error); + notificationSystem.error('Failed to copy database to clipboard'); + }), [database] )
Line range hint
162-171: Enhance accessibility for interactive elements.Add appropriate ARIA labels to improve accessibility for screen readers.
<StyledInputBase value={tempName} sx={{ borderRadius: 1, px: 1, flexGrow: 1 }} onChange={(e) => setTempName(e.target.value)} onBlur={onBlur} onKeyDown={onKeyDown} + aria-label={t`DatabaseCard.input.name`} />
Line range hint
264-268: Consider using consistent date formatting.The last edit timestamp should use a consistent date format that respects the user's locale settings.
- <strong>{new Date(lastEdit).toLocaleString()}</strong> + <strong>{new Intl.DateTimeFormat(undefined, { + dateStyle: 'medium', + timeStyle: 'medium' + }).format(new Date(lastEdit))}</strong>apps/sr-frontend/src/app/Header.tsx (1)
Line range hint
1-324: Consider these improvements for maintainability.While the current implementation is solid, consider these suggestions to improve maintainability:
- Extract the common tab configuration to reduce duplication between
maincontentandmobileContentarrays- Consider using constants for magic strings like color values and breakpoint names
Here's how you could refactor the tab configuration:
const TAB_CONFIG = { relics: { i18Key: 'tabs.relics', icon: <RelicIcon />, to: '/relics', value: 'relics', textSuffix: <RelicsTab key="relicsAdd" />, }, // ... other tab configs } as const const maincontent = [ TAB_CONFIG.relics, TAB_CONFIG.lightCones, TAB_CONFIG.characters, TAB_CONFIG.teams, TAB_CONFIG.settings, ] as const const mobileContent = [...maincontent] as constlibs/sr/ui/src/LightCone/LightConeEditor/LightConeEditor.tsx (2)
Line range hint
156-161: Enhance error handling and user experience.Consider the following improvements:
- Replace
window.confirmwith Material-UI'sDialogcomponent for consistent styling- Add loading states during database operations
- Implement proper error handling for database operations with user feedback
Here's how you could improve the confirmation dialog:
-if (!window.confirm(t('editor.clearPrompt') as string)) { - e?.preventDefault() - return -} +const [showConfirmDialog, setShowConfirmDialog] = useState(false); + +// Add this component within the JSX +<Dialog + open={showConfirmDialog} + onClose={() => setShowConfirmDialog(false)} +> + <DialogTitle>{t('editor.clearPrompt')}</DialogTitle> + <DialogActions> + <Button onClick={() => setShowConfirmDialog(false)}> + {t('common:cancel')} + </Button> + <Button + onClick={() => { + setShowConfirmDialog(false); + setShowEditor(false); + reset(); + }} + color="primary" + > + {t('common:confirm')} + </Button> + </DialogActions> +</Dialog>Don't forget to add the necessary imports:
import { Dialog, DialogTitle, DialogActions, } from '@mui/material';Also applies to: 229-232
Based on the gathered information, I can now provide a final response:
Technical debt confirmed: Temporary implementations have broader impact than initially assessed
The temporary implementations are more significant than initially suggested:
- The
ILightConeSheetinterface is used across multiple components (libs/sr/page-lightcones,libs/sr/ui/src/LightCone)SuperimposeKeyis a core type used throughout the codebase, including data models (sr/db,sr/srod,sr/dm)- The temporary implementation limits superimposition to basic array
[1, 2, 3, 4, 5]while the actual implementation insr/dmhas more complex requirements (AbilityProperty, ParamList)Priority areas for implementation:
- Replace
tempLightConeSheetwith proper implementation aligned with data models insr/dm- Implement proper error handling and validation for superimposition values
- Add missing image display system (no current implementation found)
🔗 Analysis chain
Line range hint
34-42: Address technical debt: Temporary implementations need completion.Several temporary implementations and TODOs need attention:
ILightConeSheetinterface andtempLightConeSheetare temporary implementations- Light cone sheets, error handling, and i18n support are incomplete
- Image display is not implemented (
toImgprop)These gaps could affect maintainability and user experience.
Let's verify the scope of these temporary implementations:
Would you like help implementing any of these missing features? I can help:
- Design a proper light cone sheet interface
- Implement proper error handling and validation
- Set up the image display system
Also applies to: 256-256
Let me gather more specific information about these temporary implementations.
This will help us understand:
- The actual interface definition and its limitations
- The specific TODO comments and their context
- The temporary implementations and their impact
Let me check if there are any other related components or implementations that might be affected.
This will help us understand:
- Where the LightConeEditor is used
- The scope of SuperimposeKey implementation
- The current state of image handling
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for related light cone sheet implementations echo "Searching for light cone sheet implementations..." rg -l "LightConeSheet" # Search for similar TODO patterns echo "Searching for related TODOs..." rg -l "TODO.*light.*cone"Length of output: 352
Script:
#!/bin/bash # Check the actual implementation details echo "Checking LightConeSheet interface and implementations..." rg -A 5 "interface.*LightConeSheet|type.*LightConeSheet" echo "Checking TODO comments..." rg "TODO.*light.*cone" -A 2 echo "Checking temporary implementations..." rg "temp.*LightCone" -A 2Length of output: 2304
Script:
#!/bin/bash # Check for LightCone related components and their dependencies echo "Checking LightCone component usage..." rg -l "LightConeEditor" # Check for SuperimposeKey usage echo "Checking SuperimposeKey usage..." rg -A 3 "SuperimposeKey" # Check for image-related implementations echo "Checking image-related code..." rg -A 3 "toImg.*prop|ImageProps" libs/sr/ui/src/LightCone/Length of output: 5451
libs/sr/page-team/src/TalentContent.tsx (2)
308-309: Optimize useCallback dependencies.The dependency array for
setConditionalcallback can be optimized by usingdatabase.teams.setConditionaldirectly instead ofdatabase.teams.- [teamId, database.teams, characterKey, presetIndex] + [teamId, database.teams.setConditional, characterKey, presetIndex]
339-347: Consider adding type safety for DocumentDisplay props.The
setConditionalprop passed toDocumentDisplayshould have its type explicitly defined to ensure type safety across component boundaries.+type DocumentDisplayProps = { + document: Document; + collapse?: boolean; + setConditional: (srcKey: string, sheetKey: string, condKey: string, condValue: number) => void; +}libs/sr/ui/src/Relic/RelicEditor/RelicEditor.tsx (3)
Line range hint
88-106: Consider performance optimizations for state managementThe current implementation uses deep cloning and multiple state updates which could impact performance. Consider these optimizations:
- Replace deep cloning with immutable updates
- Batch related state updates
- Memoize expensive computations with useMemo
Example optimization for the useEffect:
- if (dbRelic) { - setShowEditor(true) - dispatchRelic({ - type: 'overwrite', - relic: deepClone(dbRelic), - }) - } + const memoizedRelic = useMemo(() => dbRelic ? {...dbRelic} : null, [dbRelic]) + if (memoizedRelic) { + setShowEditor(true) + dispatchRelic({ + type: 'overwrite', + relic: memoizedRelic, + }) + }
Line range hint
108-121: Enhance error handling and user confirmationsThe current implementation uses browser's native
window.confirmfor user confirmations and could benefit from these improvements:
- Replace
window.confirmwith a custom modal dialog for better UX- Add error boundaries for better error recovery
- Provide more specific error messages
Consider using a custom confirmation dialog:
interface ConfirmDialogProps { open: boolean; onConfirm: () => void; onCancel: () => void; message: string; } const ConfirmDialog: React.FC<ConfirmDialogProps> = ({ open, onConfirm, onCancel, message, }) => ( <Dialog open={open} onClose={onCancel}> <DialogTitle>Confirm Action</DialogTitle> <DialogContent>{message}</DialogContent> <DialogActions> <Button onClick={onCancel}>Cancel</Button> <Button onClick={onConfirm} color="error">Confirm</Button> </DialogActions> </Dialog> );Also applies to: 392-401
Line range hint
271-401: Improve accessibility complianceThe UI components need accessibility improvements to comply with WCAG guidelines:
- Add aria-labels to interactive elements
- Ensure proper keyboard navigation
- Don't rely solely on color to convey information
Apply these accessibility improvements:
<Button onClick={() => update({ lock: !relic?.lock })} color={relic?.lock ? 'success' : 'primary'} disabled={!relic} + aria-label={relic?.lock ? 'Unlock relic' : 'Lock relic'} > {relic?.lock ? <LockIcon /> : <LockOpenIcon />} </Button> <DropdownButton startIcon={relic?.mainStatKey ? <StatIcon statKey={relic.mainStatKey} /> : undefined} title={<b>{relic ? <RelicStatWithUnit statKey={relic.mainStatKey} /> : t('mainStat')}</b>} disabled={!sheet} color={relic ? 'success' : 'primary'} + aria-label="Select main stat" >
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (53)
- apps/sr-frontend/src/app/App.tsx (1 hunks)
- apps/sr-frontend/src/app/Header.tsx (1 hunks)
- libs/sr/db-ui/.babelrc (1 hunks)
- libs/sr/db-ui/.eslintrc.json (1 hunks)
- libs/sr/db-ui/README.md (1 hunks)
- libs/sr/db-ui/project.json (1 hunks)
- libs/sr/db-ui/src/context/CharacterContext.ts (1 hunks)
- libs/sr/db-ui/src/context/index.ts (0 hunks)
- libs/sr/db-ui/src/hooks/index.ts (1 hunks)
- libs/sr/db-ui/src/hooks/useBuild.ts (1 hunks)
- libs/sr/db-ui/src/hooks/useCharacter.ts (1 hunks)
- libs/sr/db-ui/src/hooks/useEquippedRelics.ts (1 hunks)
- libs/sr/db-ui/src/hooks/useLightCone.ts (1 hunks)
- libs/sr/db-ui/src/hooks/useTeam.ts (1 hunks)
- libs/sr/db-ui/src/index.ts (1 hunks)
- libs/sr/db-ui/src/provider/DatabaseProvider.tsx (1 hunks)
- libs/sr/db-ui/src/provider/index.ts (0 hunks)
- libs/sr/db-ui/tsconfig.json (1 hunks)
- libs/sr/db-ui/tsconfig.lib.json (1 hunks)
- libs/sr/db/src/Database/DataManagers/BuildDataManager.ts (1 hunks)
- libs/sr/db/src/Database/DataManagers/BuildTcDataManager.ts (1 hunks)
- libs/sr/db/src/Database/DataManagers/OptConfigDataManager.ts (0 hunks)
- libs/sr/page-team/src/BonusStats.tsx (5 hunks)
- libs/sr/page-team/src/ComboEditor.tsx (1 hunks)
- libs/sr/page-team/src/TalentContent.tsx (8 hunks)
- libs/sr/page-team/src/TeamCalcProvider.tsx (5 hunks)
- libs/sr/page-team/src/TeamCharacterSelector.tsx (3 hunks)
- libs/sr/page-team/src/TeamSelectors.tsx (1 hunks)
- libs/sr/page-team/src/TeammateDisplay/Optimize/BuildDisplay.tsx (1 hunks)
- libs/sr/page-team/src/TeammateDisplay/Optimize/OptConfigWrapper.tsx (1 hunks)
- libs/sr/page-team/src/TeammateDisplay/Optimize/StatFilterCard.tsx (1 hunks)
- libs/sr/page-team/src/TeammateDisplay/Optimize/index.tsx (6 hunks)
- libs/sr/page-team/src/TeammateDisplay/index.tsx (2 hunks)
- libs/sr/page-team/src/index.tsx (4 hunks)
- libs/sr/page-teams/src/TeamCard.tsx (2 hunks)
- libs/sr/page-teams/src/TeamInventory.tsx (4 hunks)
- libs/sr/solver/src/solver.ts (6 hunks)
- libs/sr/ui/src/Character/AbilityDropdown.tsx (1 hunks)
- libs/sr/ui/src/Character/CharacterAutocomplete.tsx (1 hunks)
- libs/sr/ui/src/Character/CharacterEditor.tsx (1 hunks)
- libs/sr/ui/src/Character/CharacterInventory.tsx (1 hunks)
- libs/sr/ui/src/Character/LocationAutocomplete.tsx (1 hunks)
- libs/sr/ui/src/Hook/index.ts (0 hunks)
- libs/sr/ui/src/Hook/useCharacter.ts (0 hunks)
- libs/sr/ui/src/Hook/useLightCone.ts (0 hunks)
- libs/sr/ui/src/LightCone/LightConeEditor/LightConeEditor.tsx (1 hunks)
- libs/sr/ui/src/LightCone/LightConeInventory.tsx (1 hunks)
- libs/sr/ui/src/Relic/RelicEditor/RelicEditor.tsx (1 hunks)
- libs/sr/ui/src/Relic/RelicInventory.tsx (1 hunks)
- libs/sr/ui/src/Settings/DatabaseCard.tsx (1 hunks)
- libs/sr/ui/src/Settings/UploadCard.tsx (1 hunks)
- libs/sr/ui/src/index.tsx (0 hunks)
- tsconfig.base.json (1 hunks)
💤 Files with no reviewable changes (7)
- libs/sr/db-ui/src/context/index.ts
- libs/sr/db-ui/src/provider/index.ts
- libs/sr/db/src/Database/DataManagers/OptConfigDataManager.ts
- libs/sr/ui/src/Hook/index.ts
- libs/sr/ui/src/Hook/useCharacter.ts
- libs/sr/ui/src/Hook/useLightCone.ts
- libs/sr/ui/src/index.tsx
✅ Files skipped from review due to trivial changes (10)
- libs/sr/db-ui/.babelrc
- libs/sr/db-ui/.eslintrc.json
- libs/sr/db-ui/README.md
- libs/sr/db-ui/project.json
- libs/sr/db-ui/src/hooks/index.ts
- libs/sr/db-ui/src/hooks/useBuild.ts
- libs/sr/db-ui/src/hooks/useEquippedRelics.ts
- libs/sr/db-ui/src/hooks/useTeam.ts
- libs/sr/db-ui/src/index.ts
- libs/sr/db-ui/tsconfig.json
🚧 Files skipped from review as they are similar to previous changes (7)
- libs/sr/page-team/src/ComboEditor.tsx
- libs/sr/page-team/src/TeammateDisplay/Optimize/BuildDisplay.tsx
- libs/sr/page-team/src/TeammateDisplay/Optimize/OptConfigWrapper.tsx
- libs/sr/page-team/src/TeammateDisplay/Optimize/StatFilterCard.tsx
- libs/sr/page-teams/src/TeamInventory.tsx
- libs/sr/solver/src/solver.ts
- tsconfig.base.json
🧰 Additional context used
🪛 Biome
libs/sr/page-team/src/TeamSelectors.tsx
[error] 38-38: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
libs/sr/page-team/src/TeammateDisplay/Optimize/index.tsx
[error] 120-120: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
🔇 Additional comments (39)
libs/sr/db-ui/src/context/CharacterContext.ts (2)
4-6: LGTM! Clean and type-safe context initialization.The simplified context structure removes unnecessary nesting while maintaining type safety through explicit type assertion.
4-6: Verify context consumers for breaking changes.Since the context structure has changed from a wrapped object to a direct value, ensure all consumers have been updated accordingly.
✅ Verification successful
Based on the search results, I can see that the CharacterContext is properly used throughout the codebase. The context structure change from a wrapped object to a direct value appears to be handled correctly in all consumers. Here's why:
- The context definition in
libs/sr/db-ui/src/context/CharacterContext.tsshows it's created with the correct type:export const CharacterContext = createContext( undefined as ICachedCharacter | undefined )
- All consumers use the context value directly without assuming a wrapper object, for example in
TeammateDisplay/index.tsx:const character = useCharacterContext()
- The provider in
libs/sr/page-team/src/index.tsxpasses the value directly:<CharacterContext.Provider value={character}>No breaking changes found in context consumers.
The context structure change has been properly implemented and all consumers are correctly handling the direct value pattern.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find all usages of CharacterContext to verify they handle the new structure correctly # Search for direct context usage echo "=== Direct CharacterContext usages ===" rg -l "CharacterContext\." --type ts --type tsx # Search for useCharacterContext hook usage echo -e "\n=== useCharacterContext hook usages ===" rg -l "useCharacterContext\(" --type ts --type tsx # Look for potential old structure references echo -e "\n=== Potential old structure references ===" rg -l "character\s*:\s*ICachedCharacter" --type ts --type tsxLength of output: 533
Script:
#!/bin/bash # Description: Find all usages of CharacterContext to verify they handle the new structure correctly # Note: Using --type typescript instead of ts/tsx # Search for direct context usage echo "=== Direct CharacterContext usages ===" rg -l "CharacterContext\." --type typescript # Search for useCharacterContext hook usage echo -e "\n=== useCharacterContext hook usages ===" rg -l "useCharacterContext\(" --type typescript # Look for potential old structure references echo -e "\n=== Potential old structure references ===" rg -l "character\s*:\s*ICachedCharacter" --type typescript # Get content of files using CharacterContext for detailed analysis echo -e "\n=== Content analysis of CharacterContext usage ===" rg "CharacterContext" --type typescript -C 3Length of output: 742
Script:
#!/bin/bash # Let's try without file type restrictions since ripgrep might not recognize typescript # Search for context usage patterns echo "=== CharacterContext usage analysis ===" rg "CharacterContext" -C 3 # Search for potential consumers echo -e "\n=== Context consumer components ===" rg "useContext\(CharacterContext\)" -C 3 # Look for the provider implementation echo -e "\n=== Context provider implementation ===" rg "CharacterContext.Provider" -C 3Length of output: 150584
libs/sr/db-ui/src/hooks/useLightCone.ts (1)
1-2: LGTM! Well-structured imports.The imports follow a good pattern, importing the base functionality from the common module and the context from the local module.
libs/sr/db-ui/tsconfig.lib.json (2)
12-23: LGTM! File patterns are well-structured.The exclude patterns comprehensively cover all test file variations, and the include patterns appropriately capture all relevant source files.
#!/bin/bash # Verify if there are any test files that might slip through fd -e ts -e tsx -e js -e jsx . src/ | rg -v "spec|test" || echo "No files found"
4-4: Consider using a more maintainable output directory path.The relative path
"../../../dist/out-tsc"could become brittle if the project structure changes. Consider using a workspace-relative path or environment variable.libs/sr/ui/src/Character/AbilityDropdown.tsx (1)
8-8: Verify the hook migration across the codebase.Let's ensure all instances of these hooks have been migrated consistently.
✅ Verification successful
Migration verified successfully
The hooks have been properly migrated across the codebase:
- All imports of
useCharacteranduseDatabaseContextare now correctly sourced from@genshin-optimizer/sr/db-ui- The only remaining relative imports (
../Hook,../Context) are unrelated to these specific hooks- The hooks are properly defined in the
sr/db-uipackage and imported consistently across all consuming components🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any remaining local imports of these hooks # Expected: No results, indicating complete migration echo "Checking for remaining local hook imports..." rg -g '*.{ts,tsx}' "import.*\{.*(useCharacter|useDatabaseContext).*\}.*from.*['\"]\.\./(Context|Hook)['\"]" echo "Verifying new import usage..." rg -g '*.{ts,tsx}' "import.*\{.*(useCharacter|useDatabaseContext).*\}.*from.*['\"]\@genshin-optimizer/sr/db-ui['\"]"Length of output: 625
Script:
#!/bin/bash # Let's fix the regex syntax and try again echo "Checking for remaining local imports..." rg -g '*.{ts,tsx}' 'import.*\{.*(useCharacter|useDatabaseContext).*\}.*from.*["\']\.\./(Context|Hook)["\']' echo -e "\nChecking new import pattern usage..." rg -g '*.{ts,tsx}' 'import.*\{.*(useCharacter|useDatabaseContext).*\}.*from.*["\'']@genshin-optimizer/sr/db-ui["\'']' # Let's also check for any other potential import patterns of these hooks echo -e "\nChecking for any other import patterns of these hooks..." rg -g '*.{ts,tsx}' '(useCharacter|useDatabaseContext)'Length of output: 333
Script:
#!/bin/bash # Simplify the search patterns to avoid syntax errors # Search for old relative imports echo "Checking for relative imports..." rg -g "*.{ts,tsx}" "../Context|../Hook" # Search for new package imports echo -e "\nChecking package imports..." rg -g "*.{ts,tsx}" "@genshin-optimizer/sr/db-ui" # Search for hook usage echo -e "\nChecking hook usage..." rg -g "*.{ts,tsx}" "useCharacter|useDatabaseContext"Length of output: 13064
libs/sr/db-ui/src/provider/DatabaseProvider.tsx (1)
8-8: LGTM! Verify the import path change impact.The import path change from '../Context/DatabaseContext' to '../context' looks good, following a more conventional naming structure.
Let's verify if all related imports have been updated consistently:
✅ Verification successful
Based on the search results, I can now generate the final response:
Import path change is safe and consistent across the codebase
The search results confirm that:
- The new import path
../contextis used consistently across all files in thesr/db-uimodule- No files are still using the old import path
Context/DatabaseContext- The context module is properly structured with
DatabaseContext.tsand an index file that re-exports it🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any remaining imports from the old path # Expect: No results, indicating all imports have been updated rg -l "from.*Context/DatabaseContext" # Also check for any potential circular dependencies with the new path rg -l "from.*@genshin-optimizer/sr/db-ui.*context"Length of output: 92
Script:
#!/bin/bash # Let's try a broader search to find any references to DatabaseContext # and understand the import patterns across the codebase # Search for DatabaseContext usage rg "DatabaseContext" -A 2 # Find all files that import from context directory rg "from.*['\"].*context['\"]" # List all files in the context directory to understand the structure fd . "context$"Length of output: 27270
libs/sr/db/src/Database/DataManagers/BuildDataManager.ts (1)
63-65:⚠️ Potential issueImplementation needed: Build removal from teams is incomplete
The current implementation removes the build but doesn't clean up references to it from teams, which could lead to orphaned references and inconsistent state. This needs to be addressed before the PR can be merged.
Let's verify if there are any direct references to builds in team files:
Consider implementing one of these approaches:
- Add a BuildReferenceManager to track and manage build references across teams
- Implement an event system where teams can subscribe to build deletions
- Store build references in a separate collection that can be efficiently queried and updated
Would you like me to help design and implement one of these solutions?
libs/sr/ui/src/Character/CharacterAutocomplete.tsx (1)
5-5: Verify consistent usage of the new import path across the codebase.The change from relative to absolute import path looks good and aligns with the architectural changes. Let's ensure this pattern is consistently applied.
✅ Verification successful
Import path change is consistent with the codebase architecture
Based on the search results, the change from relative to absolute import path for
useDatabaseContextis consistent with the codebase's architecture. The new import path@genshin-optimizer/sr/db-uiis widely used across multiple files in the Star Rail (sr) codebase, including other UI components and pages. This indicates a standardized approach to importing database-related functionality.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any remaining relative imports of useDatabaseContext # to ensure consistent usage of the new absolute import path. # Test 1: Search for relative imports echo "Checking for any remaining relative imports of useDatabaseContext..." rg -l "from '\.\./Context'" --type tsx # Test 2: Verify the new import pattern is used consistently echo "Verifying usage of new absolute import path..." rg -l "from '@genshin-optimizer/sr/db-ui'" --type tsxLength of output: 414
Script:
#!/bin/bash # Description: Check for any remaining relative imports of useDatabaseContext # and verify the new import pattern using correct file type # Test 1: Search for relative imports echo "Checking for any remaining relative imports of useDatabaseContext..." rg -l "from '\.\./Context'" -t ts -t tsx # Test 2: Verify the new import pattern is used consistently echo "Verifying usage of new absolute import path..." rg -l "from '@genshin-optimizer/sr/db-ui'" -t ts -t tsx # Test 3: Check for any other variations of Context imports echo "Checking for other variations of Context imports..." rg "import.*Context.*from" -t ts -t tsxLength of output: 601
Script:
#!/bin/bash # Let's try without file type restrictions since ripgrep might not recognize tsx # Test 1: Search for relative imports echo "Checking for any remaining relative imports of useDatabaseContext..." rg -l "from '\.\./Context'" # Test 2: Verify the new import pattern is used consistently echo "Verifying usage of new absolute import path..." rg -l "from '@genshin-optimizer/sr/db-ui'" # Test 3: Check for any other variations of Context imports echo "Checking for other variations of Context imports..." rg "import.*Context.*from"Length of output: 31627
libs/sr/page-teams/src/TeamCard.tsx (2)
3-3: LGTM! Import path update improves maintainability.The switch to absolute imports from relative paths is a good practice that makes the codebase more maintainable and resilient to refactoring.
56-59: Previous review feedback about React keys and conditional rendering is still applicable.Consider extracting team member card into a separate component.
Given this PR's focus on UI redesign, consider improving the component architecture by extracting the team member card into a reusable component:
type TeamMemberCardProps = { metadata?: { characterKey: string }; sx?: SxProps; } function TeamMemberCard({ metadata, sx }: TeamMemberCardProps) { return ( <CardThemed sx={{ flexGrow: 1, ...sx }}> <Typography> {metadata?.characterKey ?? 'Empty'} </Typography> </CardThemed> ); }This would:
- Improve reusability across the new single-page design
- Make it easier to maintain consistent styling
- Centralize team member rendering logic
libs/sr/ui/src/Character/LocationAutocomplete.tsx (1)
5-5: LGTM! Import path change aligns with centralization efforts.The update to import
useDatabaseContextfrom@genshin-optimizer/sr/db-uialigns with the codebase restructuring efforts and improves modularity.apps/sr-frontend/src/app/App.tsx (2)
Line range hint
19-26: Verify if PageTeam should be removed.The PR objectives mention removing the old "page-team" UI, but
PageTeamis still imported and used in the routes. Please clarify if this component should be removed as part of this PR or if it will be handled in a future update.Let's check if there are any pending changes or TODOs related to this:
#!/bin/bash # Description: Look for comments or code indicating planned removal of PageTeam # Search for TODO comments or related notes about PageTeam removal rg -i "todo.*page.?team|remove.*page.?team" # Check if PageTeam component has been modified in this PR rg "PageTeam" $(git diff --name-only)
2-2: Verify the new import path exists.The import path has been changed from
@genshin-optimizer/sr/uito@genshin-optimizer/sr/db-ui, which appears to be part of a module restructuring effort to better organize database-related UI components.Let's verify the new import path exists:
✅ Verification successful
Import path change is correct and properly implemented
The verification confirms that:
DatabaseProvideris correctly located inlibs/sr/db-ui/src/provider/- It is properly exported from both the component file and the index
- No references to the old import path (
@genshin-optimizer/sr/ui) forDatabaseProviderremain🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the existence of DatabaseProvider in the new location # and check for any other components that might need similar updates # Check if the new module exists and contains DatabaseProvider fd -e ts -e tsx . | xargs rg -l "export.*DatabaseProvider.*from.*@genshin-optimizer/sr/(ui|db-ui)" # Look for any other components still using the old import path rg "from.*@genshin-optimizer/sr/ui"Length of output: 1298
Script:
#!/bin/bash # Let's try a different approach to verify the DatabaseProvider import paths # Search for DatabaseProvider exports in the codebase rg "export.*DatabaseProvider" # Search for all imports from @genshin-optimizer/sr packages rg "from.*@genshin-optimizer/sr/" # Check the package structure in libs/sr directory fd . libs/sr --type d --max-depth 2Length of output: 106084
Script:
#!/bin/bash # Let's verify the DatabaseProvider component in the new location # Check for DatabaseProvider in db-ui rg -l "export.*DatabaseProvider" libs/sr/db-ui/ # Check if there are any remaining references to the old import path rg "from.*@genshin-optimizer/sr/ui.*DatabaseProvider"Length of output: 189
libs/sr/page-team/src/TeamSelectors.tsx (3)
1-21: LGTM! Well-structured component implementation.The component follows React best practices with proper typing, efficient list rendering, and good key composition for optimal reconciliation.
23-32: LGTM! Clean component setup.The component is well-typed and correctly uses the database context and team data.
72-78: LGTM! Clean render implementation.The component properly handles the null case and correctly passes props to CharacterAutocomplete.
libs/sr/ui/src/Character/CharacterInventory.tsx (1)
7-7: Verify consistent usage of the new import path across the codebase.The change from relative import to module import is good. Let's verify this pattern is consistently applied across other components.
✅ Verification successful
Based on the results, I can generate the final response as we have clear evidence that:
- There are no remaining relative imports of
useDatabaseContextfrom '../Context'- The new module import pattern
@genshin-optimizer/sr/db-uiis consistently used across multiple components in the UI directoryImport path change is consistently applied across the codebase
The migration from relative imports to module imports for
useDatabaseContexthas been thoroughly implemented across all components in thelibs/sr/ui/srcdirectory. No inconsistencies found.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any remaining relative imports of useDatabaseContext # Expected: No relative imports should remain echo "Checking for relative imports of useDatabaseContext..." rg -l "from '\.\./Context'" "libs/sr/ui/src" echo "Checking current import pattern usage..." rg -l "from '@genshin-optimizer/sr/db-ui'" "libs/sr/ui/src"Length of output: 843
libs/sr/ui/src/Relic/RelicInventory.tsx (2)
6-6: LGTM: Import centralization improves maintainabilityThe change to import
useDatabaseContextfrom@genshin-optimizer/sr/db-uialigns with the codebase-wide effort to centralize database context management.
6-6: Verify consistent database context usage across the codebaseLet's ensure all related components have been updated to use the centralized database context.
✅ Verification successful
Database context import pattern is consistently used
The verification shows that all components using
useDatabaseContextare importing it from the centralized@genshin-optimizer/sr/db-uipackage. No relative imports of the context were found, confirming that the codebase consistently uses the new centralized database context pattern.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining relative imports of database context rg -l "import.*useDatabaseContext.*from '\.\.?/Context'" "libs/sr/ui/src/**/*" # Search for the new centralized import pattern rg -l "import.*useDatabaseContext.*from '@genshin-optimizer/sr/db-ui'" "libs/sr/ui/src/**/*"Length of output: 307
Script:
#!/bin/bash # Search for any remaining relative imports of database context without glob expansion rg -l "import.*useDatabaseContext.*from '\.\.?/Context'" libs/sr/ui/src/ # Search for the new centralized import pattern without glob expansion rg -l "import.*useDatabaseContext.*from '@genshin-optimizer/sr/db-ui'" libs/sr/ui/src/ # List all files in the directory to verify path correctness fd . libs/sr/ui/src/Length of output: 2383
libs/sr/ui/src/LightCone/LightConeInventory.tsx (1)
8-8: LGTM! Import path update aligns with the broader restructuring.The change from relative import to module import (
@genshin-optimizer/sr/db-ui) is consistent with the codebase restructuring and improves maintainability.libs/sr/page-team/src/TeamCalcProvider.tsx (2)
Line range hint
150-169: LGTM! Improved type safety in createMember.The function now properly filters and handles undefined relics, making it more robust.
110-117:⚠️ Potential issueAdd error boundary for preset handling.
The current implementation assumes the preset index will always map to a valid preset string. Consider adding validation or error handling.
Let's verify the possible preset values:
Add validation:
+ const presetKey = useMemo(() => { + if (presetIndex < 0) return null; + return `preset${presetIndex}` as Preset; + }, [presetIndex]); const calcWithTag = useMemo( () => (currentChar && calc?.withTag({ src: currentChar, dst: currentChar, - preset: `preset${presetIndex}` as Preset, + preset: presetKey ?? 'preset0', })) ?? null, - [calc, currentChar, presetIndex] + [calc, currentChar, presetKey] )libs/sr/page-team/src/TeammateDisplay/index.tsx (2)
3-3: LGTM: Import changes align with team-based architectureThe updated imports correctly reflect the architectural shift from loadout-based to team-based context, maintaining clean organization and proper module boundaries.
Also applies to: 22-26
28-32: Verify the removal of tab-based navigationThe component signature change aligns with the PR's objective to remove character tabs. However, we should verify that all consumers of this component have been updated accordingly.
✅ Verification successful
Tab prop removal has been properly handled across the codebase
The verification shows that:
- Current usage of
TeammateDisplayinlibs/sr/page-team/src/index.tsxdoesn't pass any tab prop- Git history confirms the tab prop was removed as part of architectural changes
- No other instances of
TeammateDisplaywith tab prop usage were found in the codebase🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining references to the removed tab prop rg -A 2 "TeammateDisplay.*tab[^a-zA-Z]" # Search for any remaining tab-based navigation patterns that might need updating ast-grep --pattern 'tab={$_}'Length of output: 73
Script:
#!/bin/bash # Search for TeammateDisplay usage patterns in the codebase rg -l "TeammateDisplay" # Search for files that import TeammateDisplay rg -l "from.*TeammateDisplay" # Look for any JSX/TSX usage of TeammateDisplay with props ast-grep --pattern '<TeammateDisplay $$$>'Length of output: 385
Script:
#!/bin/bash # Check the actual usage of TeammateDisplay in the identified files rg -A 3 "<TeammateDisplay" libs/sr/page-team/src/ComboEditor.tsx libs/sr/page-team/src/index.tsx # Look at the content of the component file to verify its previous state git log -p libs/sr/page-team/src/TeammateDisplay/index.tsx | grep -A 10 "tab"Length of output: 9732
libs/sr/page-team/src/TeamCharacterSelector.tsx (2)
42-42: Previous comment about error handling is still applicable.
31-37: Verify the removal of the tab prop default value.The removal of the default value for the
tabprop from the component's interface could potentially break existing usages. Please ensure all component consumers are updated accordingly.✅ Verification successful
No breaking changes from tab prop removal
The codebase analysis shows that there are no usages of the
tabprop in any component that importsTeamCharacterSelector. The current usages in bothlibs/gi/page-team/src/index.tsxandlibs/sr/page-team/src/index.tsxonly passteamIdandcharacterKey/charKeyprops. The removal of the tab prop appears to be a safe cleanup rather than a breaking change.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any usage of TeamCharacterSelector with tab prop rg "TeamCharacterSelector.*tab=" -A 2Length of output: 39
Script:
#!/bin/bash # Let's search for any usage of TeamCharacterSelector component more broadly rg "TeamCharacterSelector" -A 2 # Also search for imports of this component rg "from.*TeamCharacterSelector" -A 2Length of output: 1857
Script:
#!/bin/bash # Let's check the history of TeamCharacterSelector.tsx to see the removed tab prop git log -p libs/sr/page-team/src/TeamCharacterSelector.tsx | grep -A 10 "^+.*TeamCharacterSelector" # Also check the GI version since we found it exists there too git log -p libs/gi/page-team/src/TeamCharacterSelector.tsx | grep -A 10 "^+.*TeamCharacterSelector"Length of output: 1874
libs/sr/page-team/src/BonusStats.tsx (2)
11-12: LGTM: Import changes align with team-based architecture.The new imports properly support the transition from loadout-based to team-based management.
Also applies to: 26-26
63-76: 🛠️ Refactor suggestionFix value preservation and consolidate database updates.
- The previous review's concern about value preservation during tag updates is still valid
- The database update logic is repeated with similar patterns
+ const updateBonusStat = (tag: Tag, value: number) => + database.teams.setBonusStat(teamId, tag, value, presetIndex) {bonusStats.map(({ tag, values }, i) => ( <BonusStatDisplay key={JSON.stringify(tag) + i} tag={tag} value={values[presetIndex]} - setValue={(value) => - database.teams.setBonusStat(teamId, tag, value, presetIndex) - } - onDelete={() => - database.teams.setBonusStat(teamId, tag, 0, presetIndex) - } - setTag={(tag) => - database.teams.setBonusStat(teamId, tag, 0, presetIndex) - } + setValue={(value) => updateBonusStat(tag, value)} + onDelete={() => updateBonusStat(tag, 0)} + setTag={(newTag) => updateBonusStat(newTag, values[presetIndex])} /> ))}libs/sr/page-team/src/TeammateDisplay/Optimize/index.tsx (1)
119-120: Avoid assignment within Promise executorThe existing issue with assignment within the Promise executor is still present, as noted in the previous review.
🧰 Tools
🪛 Biome
[error] 120-120: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
libs/sr/db/src/Database/DataManagers/BuildTcDataManager.ts (1)
62-62: Clarify the TODO and handle potential dangling references.The removal of cleanup logic could lead to dangling references in teams. While this aligns with making builds referenceable from any team, we should ensure proper handling of build deletions.
Let's verify the impact:
Consider implementing one of these approaches:
- Implement a reference counting system to track build usage.
- Add a notification system to inform teams when builds are deleted.
- Make teams store copies of builds instead of references.
Would you like me to elaborate on any of these approaches?
libs/sr/ui/src/Settings/DatabaseCard.tsx (2)
8-8: LGTM! Good practice moving context to a centralized location.The change from relative import to a module import improves code organization and maintainability.
Line range hint
97-102: Consider improving error handling for database operations.The database operations (delete, download, swap) should handle potential errors and provide appropriate user feedback.
Let's verify the error handling patterns in other database operations:
Consider wrapping these operations in try-catch blocks and providing user feedback:
const onDelete = useCallback(() => { - if (!window.confirm(`Are you sure you want to delete "${name}"?`)) return - database.clear() - database.toExtraLocalDB() + const confirmDelete = async () => { + try { + if (!window.confirm(`Are you sure you want to delete "${name}"?`)) return + await database.clear() + await database.toExtraLocalDB() + notificationSystem.success('Database deleted successfully'); + } catch (error) { + console.error(error); + notificationSystem.error('Failed to delete database'); + } + } + confirmDelete(); }, [database, name])Also applies to: 126-159
libs/sr/ui/src/Settings/UploadCard.tsx (2)
Line range hint
31-296: LGTM: Component implementation remains robust and well-structured.The component maintains good practices:
- Proper error handling for file uploads and JSON parsing
- Clear separation of concerns between upload, validation, and display
- Comprehensive UI feedback through the import process
- Type-safe database operations
9-9: LGTM: Import change aligns with centralized context management.The change to import
useDatabaseContextfrom '@genshin-optimizer/sr/db-ui' instead of a local context aligns with good architectural practices by centralizing database context management.Let's verify that this change is consistent across the codebase:
✅ Verification successful
Import change is consistently applied across the codebase
The verification shows that:
- No local database context imports remain in the codebase
- The centralized
useDatabaseContextfrom '@genshin-optimizer/sr/db-ui' is consistently used across 21 filesThis confirms that the import change in
UploadCard.tsxis part of a properly executed centralization of database context management.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any remaining local database context imports # and verify consistent usage of the centralized context # Test 1: Check for any remaining local context imports echo "Checking for local context imports..." rg -l "import.*useDatabaseContext.*from.*['\"]\.\.?/Context['\"]" # Test 2: Verify consistent usage of the new import path echo "Checking usage of centralized context..." rg -l "import.*useDatabaseContext.*from.*['\"]@genshin-optimizer/sr/db-ui['\"]"Length of output: 1317
libs/sr/ui/src/LightCone/LightConeEditor/LightConeEditor.tsx (1)
21-21: LGTM: Import path update aligns with codebase reorganization.The change to use
@genshin-optimizer/sr/db-uifor database context follows the standardization pattern seen across other components.libs/sr/page-team/src/TalentContent.tsx (2)
46-48: LGTM: Context migration implemented correctly.The change from loadout context to team context aligns with the PR objectives while maintaining proper type safety.
408-408: Address TODO comment for levelBoost calculation.The levelBoost calculation is currently hardcoded to 0 with a TODO comment. This might affect the displayed talent levels.
Would you like help implementing the talent boost calculation?
libs/sr/ui/src/Relic/RelicEditor/RelicEditor.tsx (1)
19-19: LGTM: Clean database context integrationThe database context integration is well-implemented with proper cleanup and state management using React hooks.
Also applies to: 82-86
| export function useLightCone(lightConeId: string | undefined) { | ||
| const { database } = useDatabaseContext() | ||
| return useDataManagerBase(database.lightCones, lightConeId ?? '') | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
Add test coverage for the database hooks
The verification reveals:
- No direct tests for
useLightConehook - No tests for the underlying
useDataManagerBasehook in the common package - The hook is part of a broader database interaction system with other similar hooks (useTeam, useCharacter, useBuild)
- All these hooks follow the same pattern of using
useDataManagerBase
Action items:
- Add unit tests for
useDataManagerBaseinlibs/common/database-ui/src/hooks/as it's the core functionality - Add integration tests for
useLightConeto verify database interactions - Consider adding a test suite that covers all database hooks consistently
🔗 Analysis chain
Verify test coverage for the hook.
Since this hook handles critical database interactions, ensure it has proper test coverage.
Let's check for existing tests:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Look for test files related to the light cone hook
# Expected: Find test files that cover the useLightCone hook functionality
echo "Searching for test files..."
fd -e test.ts -e spec.ts useLightCone
echo "Searching for light cone related test content..."
rg -l "useLightCone" -g "*.test.ts" -g "*.spec.ts"
Length of output: 262
Script:
#!/bin/bash
# Let's try a broader search to understand the testing structure and coverage
# First, let's check if there are any test files in the same directory
echo "Checking for test files in the same directory..."
fd -e test.ts -e spec.ts . libs/sr/db-ui/src/hooks/
# Let's also check for any files that might be testing this functionality
# even if not directly named after the hook
echo "Checking for test files containing references to light cones..."
rg -l "lightCone" -g "*.test.ts" -g "*.spec.ts"
# Let's also check for tests of the base hook that this component uses
echo "Checking for tests of the base hook..."
rg -l "useDataManagerBase" -g "*.test.ts" -g "*.spec.ts"
# Let's examine the implementation of useDataManagerBase to understand test coverage needs
echo "Checking useDataManagerBase implementation..."
rg -A 10 "useDataManagerBase" libs/sr/db-ui/src/
Length of output: 2986
Script:
#!/bin/bash
# Let's check the implementation of useDataManagerBase since it's the core functionality
echo "Checking useDataManagerBase implementation in common package..."
rg -A 10 "export function useDataManagerBase" libs/common/database-ui/src/
# Let's also check for tests in the common package where the base functionality lives
echo "Checking for tests of useDataManagerBase in common package..."
fd -e test.ts -e spec.ts . libs/common/database-ui/src/
# Let's also look for any test files that might be testing the database context
echo "Checking for database context tests..."
rg -l "useDatabaseContext" -g "*.test.ts" -g "*.spec.ts"
Length of output: 1485
| export function useCharacter(characterKey: CharacterKey | '' | undefined) { | ||
| const { database } = useDatabaseContext() | ||
| return useDataManagerBase(database.chars, characterKey as CharacterKey) | ||
| } |
There was a problem hiding this comment.
Add type safety and error handling.
The current implementation type-casts potentially invalid values ('' or undefined) to CharacterKey without validation, which could lead to runtime errors.
Consider adding validation and proper error handling:
export function useCharacter(characterKey: CharacterKey | '' | undefined) {
const { database } = useDatabaseContext()
- return useDataManagerBase(database.chars, characterKey as CharacterKey)
+ if (!characterKey) return undefined
+ return useDataManagerBase(database.chars, characterKey)
}Committable suggestion was skipped due to low confidence.
| "compilerOptions": { | ||
| "outDir": "../../../dist/out-tsc", | ||
| "types": [ | ||
| "node", | ||
|
|
||
| "@nx/react/typings/cssmodule.d.ts", | ||
| "@nx/react/typings/image.d.ts" | ||
| ] | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding important compiler options for type safety and module compatibility.
The current compiler options are minimal. Consider adding these commonly used options for better type safety and module compatibility:
"compilerOptions": {
"outDir": "../../../dist/out-tsc",
+ "strict": true,
+ "esModuleInterop": true,
+ "sourceMap": true,
+ "declaration": true,
"types": [
"node",
"@nx/react/typings/cssmodule.d.ts",
"@nx/react/typings/image.d.ts"
]
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "compilerOptions": { | |
| "outDir": "../../../dist/out-tsc", | |
| "types": [ | |
| "node", | |
| "@nx/react/typings/cssmodule.d.ts", | |
| "@nx/react/typings/image.d.ts" | |
| ] | |
| }, | |
| "compilerOptions": { | |
| "outDir": "../../../dist/out-tsc", | |
| "strict": true, | |
| "esModuleInterop": true, | |
| "sourceMap": true, | |
| "declaration": true, | |
| "types": [ | |
| "node", | |
| "@nx/react/typings/cssmodule.d.ts", | |
| "@nx/react/typings/image.d.ts" | |
| ] | |
| }, |
| if (existingIndex === -1) { | ||
| // If none, create it | ||
| database.teams.set(teamId, (team) => { | ||
| // Let the validator handle default properties for everything else | ||
| team.teamMetadata[teamMetadataIndex] = { | ||
| characterKey: cKey, | ||
| } as TeamMetaDataum | ||
| }) | ||
| } |
There was a problem hiding this comment.
Improve type safety and error handling for character creation.
The code uses type assertion and lacks error handling for database operations.
Apply these improvements:
if (existingIndex === -1) {
// If none, create it
+ try {
database.teams.set(teamId, (team) => {
// Let the validator handle default properties for everything else
- team.teamMetadata[teamMetadataIndex] = {
- characterKey: cKey,
- } as TeamMetaDataum
+ const newMetadata: TeamMetaDataum = {
+ characterKey: cKey,
+ // Add other required properties here
+ };
+ team.teamMetadata[teamMetadataIndex] = newMetadata;
+ return team;
})
+ } catch (error) {
+ console.error('Failed to create character:', error);
+ // Consider adding user notification here
+ }
}Committable suggestion was skipped due to low confidence.
| function setCharKey(cKey: CharacterKey | '') { | ||
| // Remove teammate | ||
| if (cKey === '') { | ||
| database.teams.set( | ||
| teamId, | ||
| (team) => (team.teamMetadata[teamMetadataIndex] = undefined) | ||
| ) | ||
| return | ||
| } |
There was a problem hiding this comment.
Fix the assignment expression and add error handling.
The current implementation has potential issues:
- Assignment in expression is considered bad practice
- Missing error handling for database operations
Apply this fix:
if (cKey === '') {
- database.teams.set(
- teamId,
- (team) => (team.teamMetadata[teamMetadataIndex] = undefined)
- )
+ try {
+ await database.teams.set(teamId, (team) => {
+ team.teamMetadata[teamMetadataIndex] = undefined;
+ return team;
+ });
+ } catch (error) {
+ console.error('Failed to remove teammate:', error);
+ // Consider adding user notification here
+ }
return;
}Committable suggestion was skipped due to low confidence.
🧰 Tools
🪛 Biome
[error] 38-38: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
| `Character ${ind + 1}` // TODO: Translation | ||
| ) | ||
| } |
There was a problem hiding this comment.
Add translation for character slot label.
The string "Character ${ind + 1}" is hardcoded in English. Use the translation system consistently:
- `Character ${ind + 1}` // TODO: Translation
+ t('team:characterSlot', { number: ind + 1 })Committable suggestion was skipped due to low confidence.
| // conserve the current tab when switching to another character | ||
| loadoutKey && navigate(`/teams/${teamId}/${loadoutKey}/${tab}`) | ||
| characterKey && navigate(`/teams/${teamId}/${characterKey}`) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider improving the navigation UX.
The current implementation only navigates when a character is selected. Consider providing feedback or a different interaction for empty slots to improve user experience.
- characterKey && navigate(`/teams/${teamId}/${characterKey}`)
+ characterKey
+ ? navigate(`/teams/${teamId}/${characterKey}`)
+ : database.teams.showCharacterSelector(teamId, ind)Committable suggestion was skipped due to low confidence.
There was a problem hiding this comment.
Actionable comments posted: 16
🧹 Outside diff range and nitpick comments (53)
libs/sr/db-ui/src/context/CharacterContext.ts (1)
4-6: Consider adding JSDoc comments.Since this context is part of a public API, consider adding JSDoc comments to document its purpose and usage pattern.
+/** + * Context for the currently selected character. + * @example + * function MyComponent() { + * const character = useCharacterContext(); + * if (!character) return null; + * return <div>{character.name}</div>; + * } + */ export const CharacterContext = createContext( undefined as ICachedCharacter | undefined )libs/sr/db-ui/src/hooks/useLightCone.ts (1)
4-7: Consider adding JSDoc documentation.Adding JSDoc documentation would improve maintainability by documenting:
- Purpose of the hook
- Parameter description
- Return value type and description
- Example usage
+/** + * Hook to manage light cone data from the database. + * @param lightConeId - The ID of the light cone to retrieve, or undefined + * @returns The light cone data manager instance + * @example + * const lightCone = useLightCone("123") + */ export function useLightCone(lightConeId: string | undefined) {libs/sr/db-ui/tsconfig.lib.json (2)
12-22: Consider adding more test-related patterns to exclusions.The current exclusions cover most test files, but there might be other test-related files to exclude:
- Storybook stories
- Test utilities
- Mock files
"exclude": [ "jest.config.ts", "src/**/*.spec.ts", "src/**/*.test.ts", "src/**/*.spec.tsx", "src/**/*.test.tsx", "src/**/*.spec.js", "src/**/*.test.js", "src/**/*.spec.jsx", "src/**/*.test.jsx", + "src/**/*.stories.tsx", + "src/**/*.stories.ts", + "src/**/*.mock.ts", + "src/test-utils/**/*" ]
1-24: Add documentation comments for configuration purpose.Consider adding a comment block at the top of the file to document the purpose of this specific tsconfig and its relationship to the broader build system.
+/** + * TypeScript configuration for the Star Rail DB UI library + * This config extends the base tsconfig and adds specific settings for: + * - React component compilation + * - CSS modules and image imports + * - Test file exclusions + */ { "extends": "./tsconfig.json", // ... rest of the configlibs/sr/ui/src/Character/AbilityDropdown.tsx (1)
8-8: LGTM! Good architectural improvement.The consolidation of hooks into
@genshin-optimizer/sr/db-uipromotes better modularity and maintainability.libs/sr/db-ui/src/provider/DatabaseProvider.tsx (1)
Line range hint
10-45: Consider adding documentation for database management logic.While the code is well-structured, adding JSDoc comments would help explain:
- The purpose of multiple database instances
- The significance of the database index
- The local storage persistence strategy
- The sandbox vs. local storage usage criteria
Example documentation:
/** * Provides database context for the Star Rail Optimizer application. * Manages multiple database instances with different storage strategies: * - Primary database uses local storage * - Additional databases use sandbox storage * * @param children - React child components that will have access to the database context */libs/sr/db/src/Database/DataManagers/BuildDataManager.ts (1)
Line range hint
1-64: Consider adding migration strategy for existing data.Given the architectural shift from loadout-based to team-based management, there should be a strategy to handle existing build data and references during this transition.
Consider implementing:
- Data migration utilities to convert loadout-based builds to team-based builds
- Validation methods to ensure data integrity during the transition
- Fallback handling for any legacy data structures
libs/sr/ui/src/Character/CharacterAutocomplete.tsx (2)
Line range hint
44-44: Address TODO comment for character images.The empty image render function should be implemented to enhance the autocomplete UI with character avatars.
Would you like me to help implement the character image rendering functionality or create a GitHub issue to track this task?
Line range hint
29-39: Consider memoizing the base options array.The base options array containing the 'none' option could be memoized separately from the dynamic character options to reduce unnecessary recreations.
+ const baseOptions = useMemo( + () => [{ + key: '', + label: t('character:autocomplete.none'), + }], + [t] + ) const options: GeneralAutocompleteOption<CharacterKey | ''>[] = useMemo( () => [ - { - key: '', - label: t('character:autocomplete.none'), - }, + ...baseOptions, ...allCharacterKeys.map( (key): GeneralAutocompleteOption<CharacterKey | ''> => ({libs/sr/page-teams/src/TeamCard.tsx (1)
Line range hint
11-24: Consider these architectural improvements for better maintainability.
- Add TypeScript types for team metadata:
interface TeamMetadata { characterKey: CharacterKey; // Add other relevant fields } interface TeamCardProps { teamId: string; bgTheme?: 'light' | 'dark'; onClick: (charID?: CharacterKey) => void; }
- Add loading and error states:
if (!team) { return <CardThemed><Typography>Loading team...</Typography></CardThemed>; }
- Add JSDoc comments for better documentation:
/** * Displays a team card with character slots and team information. * @param {string} teamId - Unique identifier for the team * @param {'light' | 'dark'} [bgTheme='light'] - Card background theme * @param {Function} onClick - Callback when a character or empty slot is clicked */Would you like me to create a GitHub issue to track these improvements?
libs/sr/ui/src/Character/LocationAutocomplete.tsx (1)
Line range hint
52-52: Address TODO comment for image implementationThe
toImgprop implementation is currently a placeholder with a TODO comment. This might affect the visual completeness of the location selector.Would you like me to help implement the image rendering functionality or create a GitHub issue to track this task? I notice this is part of the UI redesign effort mentioned in the PR objectives.
libs/sr/ui/src/Character/CharacterInventory.tsx (1)
Line range hint
52-56: Fix typo in Skeleton width prop.There's a small typo in the Skeleton component's width prop (
widht).<Skeleton variant="rectangular" - sx={{ widht: '100%', height: '100%', minHeight: 300 }} + sx={{ width: '100%', height: '100%', minHeight: 300 }} />libs/sr/ui/src/Relic/RelicInventory.tsx (1)
Line range hint
22-146: Consider breaking down the component for better maintainability.The
RelicInventorycomponent currently handles multiple responsibilities including:
- Relic data management
- Infinite scrolling
- UI rendering
- CRUD operations
Consider splitting this into smaller, focused components:
RelicListfor the grid displayRelicHeaderfor the showing/filtering sectionRelicInfiniteScrollfor scroll managementThis would improve:
- Code maintainability
- Component reusability
- Testing isolation
libs/sr/solver/src/solver.ts (2)
72-72: Consider enhancing error handling with structured logging.While changing to
console.erroris an improvement, consider implementing structured logging for better error tracking and debugging in production.-console.error(data) +console.error('Solver optimization failed:', { + error: data, + characterKey: this.characterKey, + frameCount: this.frames.length +})Also applies to: 108-108
123-129: Well-structured implementation of frame-based optimization.The new implementation elegantly handles multiple frames through the
sumaggregation while maintaining proper frame identification via preset tags. This change successfully enables the team-based optimization approach.Consider documenting the frame aggregation logic, as it's a core part of the new architecture:
private detachNodes() { // Step 2: Detach nodes from Calculator const relicSetKeys = new Set(allRelicSetKeys) const detachedNodes = detach( [ - // team + // Aggregate team frames for optimization + // Each frame is tagged with a unique preset identifier to maintain + // frame-specific calculations while allowing combined optimization sum( ...this.frames.map((frame, i) => new Read(frame, 'sum').with('preset', `preset${i}` as Preset) ) ),libs/sr/ui/src/LightCone/LightConeInventory.tsx (2)
Line range hint
46-55: Consider adding error boundary for database operations.While the component handles loading states well with Suspense, it might benefit from explicit error handling for database operations.
const { database } = useDatabaseContext() const [dirtyDatabase, setDirtyDatabase] = useForceUpdate() + const [error, setError] = useState<Error | null>(null) // lives here until DB storage for UI sort/filter options is created const initialSort = () => ({ sortType: lightConeSortKeys[0], ascending: false, rarity: [...allRarityKeys], path: [...allPathKeys], }) + if (error) { + return <ErrorDisplay error={error} onRetry={() => setError(null)} /> + }
Line range hint
63-77: Consider memoizing the sort function for better performance.The sort function is recreated on every render. Consider memoizing it with
useCallbackto optimize performance, especially with large datasets.+ const sortFn = useCallback( + (lightCones: LightCone[]) => + lightCones.sort( + sortFunction( + lightConeSortMap[sortType] ?? [], + ascending, + lightConeSortConfigs() + ) + ), + [sortType, ascending] + ) const { lightConeIds, totalLightConeNum } = useMemo(() => { const lightCones = database.lightCones.values const totalLightConeNum = lightCones.length - const lightConeIds = lightCones - .sort( - sortFunction( - lightConeSortMap[sortType] ?? [], - ascending, - lightConeSortConfigs() - ) - ) + const lightConeIds = sortFn(lightCones) .map((lc) => lc.id) return dirtyDatabase && { lightConeIds, totalLightConeNum } }, [database, dirtyDatabase, sortType, ascending, sortFn])libs/sr/page-team/src/TeammateDisplay/index.tsx (2)
30-31: Consider adding error handling for missing team metadataWhile the transition to team context is good, consider handling edge cases where team metadata or character key might be undefined.
const { teamMetadatum: { characterKey }, - } = useTeamContext() + } = useTeamContext() + if (!characterKey) { + return <Typography>Please select a character for this team slot</Typography> + }
Line range hint
28-132: Consider extracting debug views into a separate componentThe component currently handles multiple responsibilities: team member display, character editing, combo editing, and debug information. Consider splitting this into smaller, focused components to improve maintainability:
TeammateDisplay- Main componentTeammateDebugView- Debug accordions and formula displaysTeammateActions- Character editing buttons and actionsThis would align better with the Single Responsibility Principle and make the codebase more maintainable as the team-based features evolve.
libs/sr/page-team/src/TeamCharacterSelector.tsx (1)
156-174: Improve accessibility and internationalization.A few suggestions for improvement:
- Add ARIA labels to improve screen reader experience when selecting characters.
- The hardcoded string "Character {index}" needs to be moved to translations.
Apply these improvements:
<Tab icon={<PersonIcon />} iconPosition="start" value={characterKey ?? ind} key={ind} disabled={!teamMetadatum} + aria-label={characterKey + ? t(`charNames_gen:${characterKey}`) + : t('team:characterSlot', { index: ind + 1 })} label={ characterKey ? ( <Typography>{t(`charNames_gen:${characterKey}`)}</Typography> ) : ( - `Character ${ind + 1}` // TODO: Translation + t('team:characterSlot', { index: ind + 1 }) ) } onClick={() => characterKey && navigate(`/teams/${teamId}/${characterKey}`) } />Please add this translation key to your locale files:
// en.json { "team": { "characterSlot": "Character {{index}}" } }libs/sr/ui/src/Character/CharacterEditor.tsx (3)
Line range hint
25-37: Consider adding error boundaries for robust error handling.While the Suspense wrapper handles loading states well, adding an error boundary would improve the user experience by gracefully handling potential errors in character data fetching.
Consider wrapping the content with an error boundary:
import { ErrorBoundary } from '@genshin-optimizer/common/ui' export function CharacterEditor({ characterKey, onClose }: Props) { return ( <ModalWrapper open={!!characterKey} onClose={onClose}> <ErrorBoundary fallback={<div>Error loading character data</div>}> <Suspense fallback={<Skeleton variant="rectangular" width="100%" height={1000} />}> {characterKey && <CharacterEditorContent characterKey={characterKey} />} </Suspense> </ErrorBoundary> </ModalWrapper> ) }
Line range hint
52-76: Enhance form handling robustness.The current implementation could benefit from several improvements:
- Add debouncing to prevent excessive database updates
- Implement stronger input validation
- Handle potential race conditions in database updates
Consider implementing these improvements:
import { useMemo } from 'react' import { debounce } from 'lodash' // Inside CharacterEditorContent const debouncedUpdate = useMemo( () => debounce((key: string, value: number) => { database.chars.set(characterKey, { [key]: value }) }, 300), [database, characterKey] ) // Example TextField with enhanced validation <TextField type="number" label="Level" variant="outlined" inputProps={{ min: 1, max: 90 }} value={character?.level || 0} onChange={(e) => { const value = Math.max(1, Math.min(90, parseInt(e.target.value) || 0)) debouncedUpdate('level', value) }} error={!character?.level || character.level < 1 || character.level > 90} helperText={character?.level ? '' : 'Invalid level'} disabled={!character} />
Line range hint
108-157: Improve accessibility for interactive elements.The UI components need enhancement for better accessibility:
- Add aria-labels to interactive elements
- Ensure proper keyboard navigation
- Verify color contrast ratios
Example improvements:
<Button color={enabled ? 'success' : 'primary'} onClick={() => { database.chars.set(characterKey, { bonusAbilities: { ...character?.bonusAbilities, [bonusAbility]: !enabled, }, }) }} startIcon={enabled ? <CheckBoxIcon /> : <CheckBoxOutlineBlankIcon />} aria-label={`Toggle ${bonusAbility} ${enabled ? 'off' : 'on'}`} role="switch" aria-checked={enabled} > {bonusAbility} </Button>Also consider adding keyboard shortcuts for common actions and ensuring all interactive elements are reachable via keyboard navigation.
libs/sr/page-team/src/BonusStats.tsx (3)
46-51: Consider adding type safety to context destructuring.While the context usage is correct, consider adding type assertions or runtime checks to ensure the destructured values are always available.
const { presetIndex } = useContext(PresetContext) const { teamId, team: { bonusStats }, - teamMetadatum: { characterKey }, + teamMetadatum: { characterKey = '' }, // Provide default value } = useTeamContext() + if (!characterKey) { + throw new Error('Character key is required') + }
53-54: Add error handling for database operations.Consider wrapping the database operation in a try-catch block to handle potential failures gracefully.
const newTarget = (q: InitialStats) => { const tag = newTag(q, characterKey) - database.teams.setBonusStat(teamId, tag, 0, presetIndex) + try { + database.teams.setBonusStat(teamId, tag, 0, presetIndex) + } catch (error) { + console.error('Failed to set bonus stat:', error) + // Consider showing user feedback + } }
86-89: Enhance type safety for tag creation.Consider adding more specific types and validation for the tag properties.
+ type TagOptions = { + src: Member + dst: Member + et: 'own' + q: Tag['q'] + qt: 'premod' + sheet: 'agg' + } - function newTag(q: Tag['q'], member: Member): Tag { + function newTag(q: Tag['q'], member: Member): TagOptions { return { src: member, dst: member, et: 'own', q, qt: 'premod', sheet: 'agg', } satisfies TagOptions }libs/sr/page-team/src/index.tsx (2)
51-59: Consider simplifying the preset context creation.The preset context object creation can be simplified by removing the explicit type assertion since TypeScript can infer the correct type.
- const presetObj = useMemo( - () => - ({ - presetIndex, - setPresetIndex, - } as PresetContextObj), - [presetIndex, setPresetIndex] - ) + const presetObj = useMemo( + () => ({ + presetIndex, + setPresetIndex, + }), + [presetIndex, setPresetIndex] + )
132-142: Remove commented styling code.The commented styling code should either be implemented or removed to maintain code cleanliness. If this is temporary, consider adding a TODO comment explaining why it's kept.
libs/sr/page-team/src/TeammateDisplay/Optimize/index.tsx (4)
Line range hint
114-157: Refactor onOptimize for better separation of concernsThe
onOptimizecallback handles multiple responsibilities (state management, optimization setup, and execution). Consider breaking it down into smaller, focused functions.Here's a suggested refactor:
const setupOptimization = useCallback(() => { setProgress(undefined) setOptimizing(true) return new Promise<void>((r) => (cancelToken.current = r)) }, []) const createOptimizer = useCallback(() => { const statFilters = (optConfig.statFilters ?? []) .filter(({ disabled }) => !disabled) .map(({ tag, value, isMax }) => ({ tag, value, isMax, })) return new Solver( characterKey, calc, team.frames, statFilters, relicsBySlot, numWorkers, setProgress ) }, [calc, characterKey, team.frames, optConfig.statFilters, relicsBySlot, numWorkers]) const onOptimize = useCallback(async () => { if (!calc) return const cancelled = setupOptimization() const optimizer = createOptimizer() cancelled.then(async () => await optimizer.terminate()) const results = await optimizer.optimize() await optimizer.terminate() cancelToken.current = () => {} setOptimizing(false) setBuild(results[0]) }, [calc, setupOptimization, createOptimizer])🧰 Tools
🪛 Biome
[error] 120-120: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
72-86: Add cleanup for progress stateThe component cleans up the optimizer but doesn't reset the progress state on unmount.
Consider adding progress cleanup to the unmount effect:
useEffect(() => () => { cancelToken.current() + setProgress(undefined) }, [])
Line range hint
201-228: Optimize ProgressIndicator performanceSince this component receives frequently updating progress data, consider memoizing it to prevent unnecessary re-renders.
Wrap the component with
React.memo:-function ProgressIndicator({ +const ProgressIndicator = React.memo(function ProgressIndicator({ progress, totalPermutations, }: { progress: ProgressResult totalPermutations: number -}) { +}) { const { t } = useTranslation('optimize') return ( // ... existing JSX ) -} +})
36-70: Team-based architecture aligns with PR objectivesThe transition from loadout-based to team-based management is well implemented. The separation of concerns between team metadata and optimization configuration provides good flexibility for future extensions.
Consider documenting these architectural changes in the component's JSDoc comments to help other developers understand the new team-based paradigm.
libs/sr/db/src/Database/DataManagers/BuildTcDataManager.ts (1)
Line range hint
1-248: Consider architectural improvements for the team-based redesign.As part of the transition from loadout-based to team-based management, consider the following architectural improvements:
- Add a method to handle team reference cleanup during build TC removal
- Implement a reverse lookup mechanism to efficiently find all teams referencing a build TC
- Consider adding events/hooks for build TC lifecycle (create/update/delete) to maintain team consistency
- Add validation for team references in the
validatemethodThis will ensure better data consistency and maintainability in the new team-based architecture.
libs/sr/ui/src/Settings/DatabaseCard.tsx (3)
Line range hint
89-94: Enhance error handling for clipboard operationsThe clipboard operation only logs errors to console without user feedback.
Consider implementing proper error handling:
const copyToClipboard = useCallback( () => navigator.clipboard .writeText(JSON.stringify(database.exportSROD())) - .then(() => alert('Copied database to clipboard.')) - .catch(console.error), + .then(() => { + // Consider using a toast/snackbar instead of alert + alert('Copied database to clipboard.'); + }) + .catch((error) => { + console.error('Failed to copy to clipboard:', error); + alert('Failed to copy to clipboard. Please try again.'); + }), [database] )
Line range hint
96-101: Improve delete confirmation UXThe current delete confirmation uses a basic
window.confirmwhich isn't very user-friendly and doesn't match the Material-UI design system.Consider using Material-UI's Dialog component for a more consistent and user-friendly confirmation:
const [deleteDialogOpen, setDeleteDialogOpen] = useState(false); const handleDelete = useCallback(() => { database.clear(); database.toExtraLocalDB(); setDeleteDialogOpen(false); }, [database]); // Add this Dialog component to your JSX <Dialog open={deleteDialogOpen} onClose={() => setDeleteDialogOpen(false)} > <DialogTitle>Confirm Delete</DialogTitle> <DialogContent> Are you sure you want to delete "{name}"? </DialogContent> <DialogActions> <Button onClick={() => setDeleteDialogOpen(false)}>Cancel</Button> <Button onClick={handleDelete} color="error">Delete</Button> </DialogActions> </Dialog>
Line range hint
22-226: Consider implementing database operation hooksThe DatabaseCard component currently handles multiple database operations directly. To improve modularity and reusability across the new UI design, consider extracting these operations into custom hooks.
Example structure:
// hooks/useDatabase.ts export function useDatabaseOperations(database: Database, index: number) { const copyToClipboard = useCallback(...); const handleDelete = useCallback(...); const handleDownload = useCallback(...); const handleSwap = useCallback(...); return { copyToClipboard, handleDelete, handleDownload, handleSwap, }; } // DatabaseCard.tsx function DataCard({ index }: { index: number }) { const { database } = useDatabaseContext(); const operations = useDatabaseOperations(database, index); // ... rest of the component }This would:
- Make database operations reusable across different components
- Simplify testing of database operations
- Make the component more focused on UI rendering
apps/sr-frontend/src/app/Header.tsx (3)
Line range hint
142-190: Consider enhancing keyboard navigation in desktop header.While the desktop header implementation is solid, consider adding keyboard shortcuts for quick navigation between tabs, especially given this is a main navigation component in the new consolidated UI design.
Add keyboard shortcuts by extending the
Tabscomponent:<Tabs value={currentTab} + onKeyDown={(e) => { + const tabs = maincontent.map(tab => tab.value); + const currentIndex = tabs.indexOf(currentTab); + if (e.key === 'ArrowRight' && currentIndex < tabs.length - 1) { + navigate(maincontent[currentIndex + 1].to); + } else if (e.key === 'ArrowLeft' && currentIndex > 0) { + navigate(maincontent[currentIndex - 1].to); + } + }} sx={(theme) => ({
Line range hint
191-268: Enhance mobile drawer accessibility.The mobile drawer implementation could benefit from improved accessibility features, particularly for screen readers.
Add ARIA labels and roles to enhance accessibility:
<Drawer anchor="right" variant="temporary" open={mobileOpen} onClose={handleDrawerToggle} + aria-label="Navigation menu" ModalProps={{ keepMounted: true, + 'aria-hidden': !mobileOpen }}
Line range hint
269-285: Consider adding loading state for mobile toolbar.The mobile toolbar could benefit from a loading state indicator, especially since the desktop version uses
SuspensewithSkeleton.Add loading state to mobile toolbar:
<Toolbar> + <Suspense fallback={ + <Box display="flex" alignItems="center"> + <Skeleton variant="text" width={120} height={32} /> + </Box> + }> <Button variant="text" sx={(theme) => ({ color: `${theme.palette.neutral200.main} !important`, })} component={RouterLink} to="/" > <Typography variant="h6" noWrap component="div" sx={{ px: 1 }}> {t('pageTitle')} </Typography> </Button> + </Suspense>libs/sr/ui/src/Settings/UploadCard.tsx (2)
Line range hint
92-103: Consider improving file handling robustness.The file upload handlers could benefit from additional error handling and type safety:
Consider these improvements:
- const onUpload = async (e: any) => { + const onUpload = async (e: React.ChangeEvent<HTMLInputElement>) => { const file = e.target.files?.[0] + if (!file) return e.target.value = null // Reset the value so the same file can be uploaded again... - if (file) setFilename(file.name) + setFilename(file.name) const reader = new FileReader() + reader.onerror = () => setErrorMsg('uploadCard.error.fileRead') reader.onload = () => setData(reader.result as string) reader.readAsText(file) } - const onDrop = async (e: any) => { + const onDrop = async (e: React.DragEvent<HTMLDivElement>) => { e.preventDefault() const file = e.dataTransfer.files[0] + if (!file) return e.target.value = null // Reset the value so the same file can be uploaded again... - if (file) setFilename(file.name) + setFilename(file.name) const reader = new FileReader() + reader.onerror = () => setErrorMsg('uploadCard.error.fileRead') reader.onload = () => setData(reader.result as string) reader.readAsText(file) }
Line range hint
44-71: Consider enhancing data validation and error handling.The JSON parsing and validation logic could be more robust with type guards and specific error messages:
Consider these improvements:
+ interface SRODFormat { + format: 'SROD' | 'SRO' + // add other expected properties + } + function isSRODFormat(data: unknown): data is SRODFormat { + return ( + typeof data === 'object' && + data !== null && + 'format' in data && + (data.format === 'SROD' || data.format === 'SRO') + ) + } const { importResult, importedDatabase } = useMemo(() => { if (!data) return undefined - let parsed: any + let parsed: unknown try { parsed = JSON.parse(data) - if (typeof parsed !== 'object') { + if (!parsed || typeof parsed !== 'object') { setErrorMsg('uploadCard.error.jsonParse') return undefined } } catch (e) { - setErrorMsg('uploadCard.error.jsonParse') + setErrorMsg(`uploadCard.error.jsonParse: ${(e as Error).message}`) return undefined } // Figure out the file format - if (parsed.format === 'SROD' || parsed.format === 'SRO') { + if (isSRODFormat(parsed)) { // Parse as SROD format const copyStorage = new SandboxStorage(undefined, 'sro') copyStorage.copyFrom(database.storage)libs/sr/ui/src/LightCone/LightConeEditor/LightConeEditor.tsx (3)
Line range hint
34-44: Track temporary implementation of light cone sheetThe current implementation is marked as temporary with a TODO comment. This technical debt should be tracked to ensure it's addressed in future updates.
Would you like me to create a GitHub issue to track the implementation of the proper light cone sheet system?
Line range hint
142-156: Improve confirmation dialog handlingConsider these improvements to the confirmation dialog:
- The type assertion
as stringon the translation could be unsafe if the key doesn't exist- The confirmation logic could be more consistent across different editor states
Consider this safer implementation:
- if (!lightConeIdToEdit && lightCone && !window.confirm(t('editor.clearPrompt') as string)) { + const confirmMessage = t('editor.clearPrompt', { defaultValue: 'Are you sure you want to clear changes?' }); + if (!lightConeIdToEdit && lightCone && !window.confirm(confirmMessage)) {
Light cone images should be implemented in autocomplete component
The codebase has all the necessary light cone images available at
libs/sr/assets/src/gen/lightCones/*/icon.png, but they're not being utilized. The current implementation uses an empty placeholder (<> </>) with a TODO comment, similar to other autocomplete components in the codebase.To implement the image:
- Import light cone icons from the assets directory
- Update the
toImgprop inGeneralAutocompleteto render the corresponding icon- Remove the TODO comment once implemented
🔗 Analysis chain
Line range hint
276-276: Address missing image implementation in autocompleteThe current implementation uses an empty placeholder for images (
<> </>). This could impact accessibility and user experience.Let's check if image assets are available in the codebase:
Would you like help implementing the image component for the light cone autocomplete?
Let me check how these images are being used in the codebase.
Let me check the actual implementation of the image placeholder in the LightConeAutocomplete component.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for image assets or image-related components fd -e png -e jpg -e svg . | grep -i "lightcone"Length of output: 12489
Script:
#!/bin/bash # Search for image component usage patterns in LightCone components ast-grep --pattern 'import { $_ } from "$_/assets"' # Check the LightConeAutocomplete component implementation rg -A 5 "LightConeAutocomplete" .Length of output: 2161
Script:
#!/bin/bash # Search for the empty image placeholder implementation rg -A 10 -B 10 "<> </>" . # Look for any image-related imports in the LightConeEditor ast-grep --pattern 'import { $_ } from "$_"' libs/sr/ui/src/LightCone/LightConeEditor/LightConeEditor.tsxLength of output: 5818
libs/sr/page-team/src/TalentContent.tsx (2)
47-48: Clean up commented-out code and TODO items.There are several sections of commented-out code and a TODO comment for TC overrides. Since this PR represents a significant UI redesign, these should either be implemented or removed to maintain code cleanliness.
Would you like me to help implement the TC override functionality or create an issue to track this task?
Also applies to: 52-56
Line range hint
408-412: Implement levelBoost calculation.The levelBoost calculation is currently hardcoded to 0 with a TODO comment. This should be implemented to ensure proper talent level calculations.
Would you like me to help implement the levelBoost calculation based on the input total boost value?
libs/sr/db/src/Database/DataManagers/TeamDataManager.ts (3)
55-55: Consider implementing enemy base stats functionality.The TODO comment indicates a planned feature for enemy base stats. This could be important for accurate damage calculations and team optimization.
Would you like help designing and implementing the enemy base stats feature? I can help create a proposal that includes:
- Data structure design
- Integration with existing team functionality
- Validation logic
234-274: Remove commented code and track as separate tasks.Large blocks of commented code can lead to maintenance issues and code rot. Consider:
- Removing the commented code
- Creating GitHub issues to track the implementation of duplicate, export, and import functionality
- Adding proper documentation about the planned features
Would you like me to:
- Create GitHub issues for tracking these features?
- Help implement these features now?
279-283: Enhance method documentation.While the current comment indicates that TC builds return undefined relics, it could be more descriptive about:
- Why TC builds return undefined
- The expected return format for other build types
- Potential error cases
Consider updating the documentation:
/** - * Note: this doesnt return any relics (all undefined) when the current teamchar is using a TC Build. + * Returns a record of relics for the given team member based on their build type. + * @param characterKey - The character to get relics for + * @param buildType - The type of build (equipped, real, or tc) + * @param buildId - The ID of the build (required for 'real' buildType) + * @returns Record of relics by slot. Returns all undefined for TC builds as they don't have real relics. + */libs/sr/ui/src/Relic/RelicEditor/RelicEditor.tsx (4)
19-19: LGTM! Good architectural choice using database context.The shift to using
useDatabaseContextcentralizes data management and improves maintainability by reducing prop drilling.
Line range hint
82-87: Consider removing dirtyDatabase from the effect dependencies.The
dirtyDatabasedependency in the useEffect might cause unnecessary re-renders since it's just a force update trigger. The database subscription should be sufficient to handle updates.useEffect( () => database.relics.followAny(setDirtyDatabase), - [database, setDirtyDatabase] + [database] )
Line range hint
196-244: Consider refactoring the update function for better maintainability.Several improvements could make this function more robust and maintainable:
- Extract the substat indices to a constant:
+ const SUBSTAT_INDICES = [0, 1, 2, 3] as const; if (newValue.mainStatKey) { - newValue.substats = [0, 1, 2, 3].map((i) => + newValue.substats = SUBSTAT_INDICES.map((i) =>
- Use type-safe maximum calculation:
- Math.max(...newSheet.rarity) as RelicRarityKey + (Math.max(...newSheet.rarity) as number) as RelicRarityKey
- Consider breaking down the update logic into smaller, focused functions for better maintainability:
const updateRarity = (newValue: Partial<IRelic>, currentRelic: IRelic) => { // Rarity-specific updates } const updateLevel = (newValue: Partial<IRelic>, currentRelic: IRelic) => { // Level-specific updates }
Line range hint
251-254: Replace window.confirm with a custom confirmation dialog.Using
window.confirmbreaks component abstraction and isn't accessible. Consider implementing a custom confirmation dialog component.interface ConfirmationDialogProps { open: boolean; onConfirm: () => void; onCancel: () => void; message: string; } const ConfirmationDialog: React.FC<ConfirmationDialogProps> = ({ open, onConfirm, onCancel, message, }) => ( <Dialog open={open} onClose={onCancel}> <DialogContent>{message}</DialogContent> <DialogActions> <Button onClick={onCancel}>Cancel</Button> <Button onClick={onConfirm} color="primary">Confirm</Button> </DialogActions> </Dialog> );Replace the window.confirm calls with the custom dialog:
- if (!window.confirm(t('editor.clearPrompt'))) return + const [showConfirmDialog, setShowConfirmDialog] = useState(false); + // ... in JSX + <ConfirmationDialog + open={showConfirmDialog} + message={t('editor.clearPrompt')} + onConfirm={() => { + setShowConfirmDialog(false); + // handle confirmation + }} + onCancel={() => setShowConfirmDialog(false)} + />Also applies to: 447-450
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (56)
- apps/sr-frontend/src/app/App.tsx (1 hunks)
- apps/sr-frontend/src/app/Header.tsx (1 hunks)
- libs/common/util/src/lib/object.ts (2 hunks)
- libs/sr/db-ui/.babelrc (1 hunks)
- libs/sr/db-ui/.eslintrc.json (1 hunks)
- libs/sr/db-ui/README.md (1 hunks)
- libs/sr/db-ui/project.json (1 hunks)
- libs/sr/db-ui/src/context/CharacterContext.ts (1 hunks)
- libs/sr/db-ui/src/context/index.ts (0 hunks)
- libs/sr/db-ui/src/hooks/index.ts (1 hunks)
- libs/sr/db-ui/src/hooks/useBuild.ts (1 hunks)
- libs/sr/db-ui/src/hooks/useCharacter.ts (1 hunks)
- libs/sr/db-ui/src/hooks/useEquippedRelics.ts (1 hunks)
- libs/sr/db-ui/src/hooks/useLightCone.ts (1 hunks)
- libs/sr/db-ui/src/hooks/useTeam.ts (1 hunks)
- libs/sr/db-ui/src/index.ts (1 hunks)
- libs/sr/db-ui/src/provider/DatabaseProvider.tsx (1 hunks)
- libs/sr/db-ui/src/provider/index.ts (0 hunks)
- libs/sr/db-ui/tsconfig.json (1 hunks)
- libs/sr/db-ui/tsconfig.lib.json (1 hunks)
- libs/sr/db/src/Database/DataManagers/BuildDataManager.ts (1 hunks)
- libs/sr/db/src/Database/DataManagers/BuildTcDataManager.ts (1 hunks)
- libs/sr/db/src/Database/DataManagers/OptConfigDataManager.ts (0 hunks)
- libs/sr/db/src/Database/DataManagers/TeamDataManager.ts (9 hunks)
- libs/sr/page-team/src/BonusStats.tsx (5 hunks)
- libs/sr/page-team/src/ComboEditor.tsx (1 hunks)
- libs/sr/page-team/src/TalentContent.tsx (8 hunks)
- libs/sr/page-team/src/TeamCalcProvider.tsx (5 hunks)
- libs/sr/page-team/src/TeamCharacterSelector.tsx (3 hunks)
- libs/sr/page-team/src/TeamSelectors.tsx (1 hunks)
- libs/sr/page-team/src/TeammateDisplay/Optimize/BuildDisplay.tsx (1 hunks)
- libs/sr/page-team/src/TeammateDisplay/Optimize/OptConfigWrapper.tsx (1 hunks)
- libs/sr/page-team/src/TeammateDisplay/Optimize/StatFilterCard.tsx (1 hunks)
- libs/sr/page-team/src/TeammateDisplay/Optimize/index.tsx (6 hunks)
- libs/sr/page-team/src/TeammateDisplay/index.tsx (2 hunks)
- libs/sr/page-team/src/context/TeamContext.ts (1 hunks)
- libs/sr/page-team/src/index.tsx (4 hunks)
- libs/sr/page-teams/src/TeamCard.tsx (2 hunks)
- libs/sr/page-teams/src/TeamInventory.tsx (4 hunks)
- libs/sr/solver/src/solver.ts (6 hunks)
- libs/sr/ui/src/Character/AbilityDropdown.tsx (1 hunks)
- libs/sr/ui/src/Character/CharacterAutocomplete.tsx (1 hunks)
- libs/sr/ui/src/Character/CharacterEditor.tsx (1 hunks)
- libs/sr/ui/src/Character/CharacterInventory.tsx (1 hunks)
- libs/sr/ui/src/Character/LocationAutocomplete.tsx (1 hunks)
- libs/sr/ui/src/Hook/index.ts (0 hunks)
- libs/sr/ui/src/Hook/useCharacter.ts (0 hunks)
- libs/sr/ui/src/Hook/useLightCone.ts (0 hunks)
- libs/sr/ui/src/LightCone/LightConeEditor/LightConeEditor.tsx (1 hunks)
- libs/sr/ui/src/LightCone/LightConeInventory.tsx (1 hunks)
- libs/sr/ui/src/Relic/RelicEditor/RelicEditor.tsx (1 hunks)
- libs/sr/ui/src/Relic/RelicInventory.tsx (1 hunks)
- libs/sr/ui/src/Settings/DatabaseCard.tsx (1 hunks)
- libs/sr/ui/src/Settings/UploadCard.tsx (1 hunks)
- libs/sr/ui/src/index.tsx (0 hunks)
- tsconfig.base.json (1 hunks)
💤 Files with no reviewable changes (7)
- libs/sr/db-ui/src/context/index.ts
- libs/sr/db-ui/src/provider/index.ts
- libs/sr/db/src/Database/DataManagers/OptConfigDataManager.ts
- libs/sr/ui/src/Hook/index.ts
- libs/sr/ui/src/Hook/useCharacter.ts
- libs/sr/ui/src/Hook/useLightCone.ts
- libs/sr/ui/src/index.tsx
✅ Files skipped from review due to trivial changes (9)
- libs/sr/db-ui/.babelrc
- libs/sr/db-ui/.eslintrc.json
- libs/sr/db-ui/README.md
- libs/sr/db-ui/project.json
- libs/sr/db-ui/src/hooks/index.ts
- libs/sr/db-ui/src/hooks/useBuild.ts
- libs/sr/db-ui/src/hooks/useEquippedRelics.ts
- libs/sr/db-ui/src/hooks/useTeam.ts
- libs/sr/db-ui/src/index.ts
🚧 Files skipped from review as they are similar to previous changes (8)
- libs/common/util/src/lib/object.ts
- libs/sr/page-team/src/ComboEditor.tsx
- libs/sr/page-team/src/TeammateDisplay/Optimize/BuildDisplay.tsx
- libs/sr/page-team/src/TeammateDisplay/Optimize/OptConfigWrapper.tsx
- libs/sr/page-team/src/TeammateDisplay/Optimize/StatFilterCard.tsx
- libs/sr/page-team/src/context/TeamContext.ts
- libs/sr/page-teams/src/TeamInventory.tsx
- tsconfig.base.json
🧰 Additional context used
🪛 Biome
libs/sr/page-team/src/TeamSelectors.tsx
[error] 38-38: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
libs/sr/page-team/src/TeammateDisplay/Optimize/index.tsx
[error] 120-120: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
🔇 Additional comments (42)
libs/sr/db-ui/src/hooks/useLightCone.ts (1)
4-7: Implementation looks good, verify database integration.The hook implementation follows React patterns and correctly handles undefined input.
Let's verify the database integration:
✅ Verification successful
Implementation is correctly integrated with the database context
The verification shows that:
- The hook follows the same pattern as other database hooks in the codebase
useDataManagerBaseis consistently used across both SR and GI implementations- The hook is properly used in
TeamCalcProvider.tsxwith optional ID handling- The database context integration matches the established pattern across the codebase
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if the hook is properly integrated with the database context # and if there are any potential type mismatches # Look for database context type definitions rg -t typescript "interface.*Database.*lightCones" # Check for other usages of this hook to ensure consistent typing rg -t typescript "useLightCone\(" -A 2Length of output: 163
Script:
#!/bin/bash # Let's try without typescript file type flag since it's not recognized # Search for Database interface definitions rg "interface.*Database.*lightCones" # Search for context provider implementation rg "DatabaseContext|useDatabaseContext" # Look for base manager implementation rg "useDataManagerBase" # Find usages of the hook rg "useLightCone\("Length of output: 11732
libs/sr/db-ui/src/hooks/useCharacter.ts (2)
1-3: LGTM: Imports are well-organized.The imports are properly structured, separating common utilities, types, and local context.
5-8: Verify the usage of empty string in characterKey.The type union includes an empty string (
''), but it's unclear why this is needed. This might indicate inconsistent character key handling across the application.Let's check how character keys are used elsewhere:
✅ Verification successful
The empty string in characterKey is valid and necessary.
The empty string in
characterKeyis intentionally used to represent "unequipped" or "no character selected" states. This is evidenced by:
- The
CharacterAutocompleteandLocationAutocompletecomponents both acceptCharacterKey | ''as valid types for their selection props- The empty string is used consistently across the codebase for:
- Unequipped relics (
RelicCard,RelicEditor)- Unequipped light cones (
LightConeCard,LightConeEditor)- Character selection dropdowns (
TeamSelectors)This pattern ensures type safety while allowing UI components to handle the "no selection" state in a consistent way across the application.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other character key type definitions and usage echo "Searching for CharacterKey type definitions:" rg -t typescript "type CharacterKey" -A 3 echo "\nSearching for useCharacter hook usage:" rg -t typescript "useCharacter\(" -A 2Length of output: 344
Script:
#!/bin/bash # Search for CharacterKey type definitions without file type restriction echo "Searching for CharacterKey type definitions:" rg "type CharacterKey" -A 3 echo -e "\nSearching for useCharacter hook usage:" rg "useCharacter\(" -A 2 echo -e "\nSearching for files containing 'CharacterKey':" fd -e ts -e tsx | xargs rg "CharacterKey"Length of output: 141266
Script:
#!/bin/bash # Search for actual usage of useCharacter hook with empty string echo "Searching for useCharacter hook calls with empty string:" rg "useCharacter\([^)]*''" -A 2 echo -e "\nSearching for CharacterAutocomplete component usage:" rg "CharacterAutocomplete" -A 5 echo -e "\nSearching for LocationAutocomplete component usage:" rg "LocationAutocomplete" -A 5Length of output: 14689
libs/sr/db-ui/tsconfig.json (1)
1-18: LGTM! Configuration follows TypeScript best practices.The TypeScript configuration is well-structured with appropriate compiler options for a React library using Emotion.
Let's verify the empty arrays don't cause issues with module resolution:
✅ Verification successful
Empty files/include arrays are correctly handled by the TypeScript configuration
The empty
filesandincludearrays inlibs/sr/db-ui/tsconfig.jsonare intentional and valid because:
- Module resolution is properly handled by the referenced
tsconfig.lib.jsonwhich includes the source files- The base configuration in
tsconfig.base.jsonprovides the core compiler options and path mappings- This follows the standard TypeScript project reference pattern where the root config delegates to specialized configs
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify module resolution configuration # Check if module resolution is handled by the base or lib config # Test 1: Check base config for include/files echo "Checking base config..." cat ../../../tsconfig.base.json # Test 2: Check lib config echo "Checking lib config..." cat ./tsconfig.lib.jsonLength of output: 283
Script:
#!/bin/bash # Let's use fd to locate the TypeScript config files echo "Finding TypeScript config files:" fd -e json -e jsonc tsconfig # Check contents of any found config files echo -e "\nChecking contents of found config files:" fd -e json -e jsonc tsconfig --exec cat {}Length of output: 108973
libs/sr/db-ui/tsconfig.lib.json (1)
4-4: Verify the output directory path.The output directory is set to
../../../dist/out-tsc. Ensure this aligns with your build system's expectations and other library configurations.✅ Verification successful
Output directory path is correctly aligned with project standards
The verification shows that all library configurations in the project (70+ libraries) consistently use the same
dist/out-tscoutput directory pattern, confirming this is the standard project-wide configuration.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if other libraries follow the same output directory pattern fd tsconfig.lib.json -x grep -l "dist/out-tsc" {} \; # Verify the existence and usage of the output directory in build scripts rg "dist/out-tsc" -g "package.json"Length of output: 3105
libs/sr/ui/src/Character/AbilityDropdown.tsx (1)
8-8: Verify consistent hook import updates across the codebase.Let's ensure all components have been updated to use the centralized hooks from
@genshin-optimizer/sr/db-ui.✅ Verification successful
All hook imports are consistently using the centralized path
Based on the search results, all instances of
useCharacteranduseDatabaseContexthooks in the Star Rail (sr) codebase are properly importing from@genshin-optimizer/sr/db-ui. The only exceptions are within thedb-uipackage itself, whereuseDatabaseContextis correctly imported from the localcontextdirectory as it's the source of the hook.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any remaining old import paths for these hooks # Test 1: Search for old useCharacter hook imports echo "Checking for old useCharacter imports..." rg "import.*useCharacter.*from.*['\"]\.\./(Hook|hooks)['\"]" --type ts --type tsx # Test 2: Search for old useDatabaseContext hook imports echo "Checking for old useDatabaseContext imports..." rg "import.*useDatabaseContext.*from.*['\"]\.\./(Context|contexts)['\"]" --type ts --type tsxLength of output: 437
Script:
#!/bin/bash # Let's try a different approach using ripgrep without file type specifications # Search for any imports of useCharacter not from the new centralized location echo "Checking useCharacter imports..." rg "import.*useCharacter.*from" -g "*.{ts,tsx}" # Search for any imports of useDatabaseContext not from the new centralized location echo "Checking useDatabaseContext imports..." rg "import.*useDatabaseContext.*from" -g "*.{ts,tsx}"Length of output: 3600
libs/sr/db-ui/src/provider/DatabaseProvider.tsx (1)
8-8: LGTM! Verify import path consistency.The import path change follows React conventions for lowercase directory names.
Let's verify that all imports are consistently updated:
✅ Verification successful
The results show that there are no remaining imports from the old path
'../Context/DatabaseContext', and the new import pattern'../context'is being consistently used across multiple files. The change appears to be part of a broader, consistent update across the codebase.Import path change is consistent across the codebase
The verification confirms that:
- No files are using the old import path
'../Context/DatabaseContext'- The new import pattern
'../context'is consistently used in related files within thesr/db-uimodule- Other modules also follow the lowercase directory convention for context imports
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any remaining imports from the old path # and verify consistency of new imports # Test 1: Check for any remaining old imports echo "Checking for old imports..." rg -l "from '\.\./Context/DatabaseContext'" # Test 2: Verify new import pattern usage echo "Checking new import pattern..." rg -l "from '\.\./context'"Length of output: 884
libs/sr/db/src/Database/DataManagers/BuildDataManager.ts (1)
63-63:⚠️ Potential issueImplement team cleanup to prevent orphaned build references.
The current TODO leaves the system in a potentially inconsistent state where removed builds might still be referenced by teams. This could lead to runtime errors or unexpected behavior.
Let's verify if there are any direct references to builds in team configurations:
Would you like me to help implement the team cleanup logic? This should:
- Identify all teams referencing the build
- Remove or update those references
- Trigger appropriate UI updates
libs/sr/ui/src/Character/CharacterAutocomplete.tsx (2)
5-5: LGTM: Import path update aligns with new module structure.The change from relative to absolute import path is consistent with the codebase restructuring and provides better maintainability.
Line range hint
1-53: Verify integration with new team-based UI.The component seems well-structured for the new single-page UI design. However, please ensure it properly integrates with the new team combo functionality mentioned in the PR objectives.
✅ Verification successful
Let me check the integration details by examining the TeamSelectors.tsx file and its usage of CharacterAutocomplete.
Let me check the team-related functionality in TeamSelectors.tsx to understand how the character selection is managed.
CharacterAutocomplete is properly integrated with team functionality
The component is correctly integrated with the team-based UI:
- It's used within TeamSelectors component for character selection in teams
- Handles team-specific logic including:
- Proper character removal from teams
- Character creation if needed
- Duplicate character prevention in teams
- Team state management via database context
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any team-related component interactions # Look for files that import CharacterAutocomplete and verify team integration # Search for imports of CharacterAutocomplete rg -l "CharacterAutocomplete" | xargs rg -l "Team|Combo"Length of output: 96
Script:
#!/bin/bash # Check how CharacterAutocomplete is used in TeamSelectors.tsx rg -A 10 "CharacterAutocomplete" "libs/sr/page-team/src/TeamSelectors.tsx"Length of output: 615
Script:
#!/bin/bash # Check the team-related hooks and state management rg -B 5 -A 15 "useTeam|setCharKey" "libs/sr/page-team/src/TeamSelectors.tsx"Length of output: 1568
libs/sr/page-teams/src/TeamCard.tsx (2)
3-3: LGTM! Import path update looks good.The change from relative to absolute import path improves maintainability and follows better module import practices.
56-59: 🛠️ Refactor suggestionPrevious review feedback is still applicable.
The existing review comment about array index keys and conditional rendering simplification remains valid.
Additionally, consider these UX improvements:
{team.teamMetadata.map((cmdata, i) => ( <CardThemed key={`team-${teamId}-slot-${i}`} sx={{ flexGrow: 1 }} + component={CardActionArea} + onClick={() => onClick(cmdata?.characterKey)} > - <Typography> + <Typography + aria-label={cmdata ? `Team member: ${cmdata.characterKey}` : 'Empty team slot'} + sx={{ textAlign: 'center' }} + > {cmdata?.characterKey ?? 'Empty'} </Typography> </CardThemed> ))}These changes:
- Make each slot clickable to select the character
- Add proper ARIA labels for accessibility
- Center-align the text for better visual appearance
libs/sr/ui/src/Character/LocationAutocomplete.tsx (1)
5-5: LGTM! Import path standardizationThe update to use an absolute import path from
@genshin-optimizer/sr/db-uialigns with the codebase's standardization efforts and improves maintainability.Let's verify this is part of a consistent pattern:
✅ Verification successful
Import standardization verified across the codebase
The import path
@genshin-optimizer/sr/db-uiforuseDatabaseContextis consistently used across 21 different files in the codebase, including components in Character, LightCone, Relic, Settings, and Team-related modules. This confirms that the import path change inLocationAutocomplete.tsxfollows the established pattern.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the standardization of database context imports # Test: Check if other files are using the same import pattern rg "import.*useDatabaseContext.*from.*@genshin-optimizer/sr/db-ui" -A 1Length of output: 4594
apps/sr-frontend/src/app/App.tsx (2)
Line range hint
1-67: LGTM! Component structure aligns with PR objectives.The overall component structure, particularly the routing setup with
/teams/*and/teams/:teamId/*, properly supports the new team combo UI design. The DatabaseProvider's placement ensures database context is available throughout the application, which is essential for the new team-based management approach.
2-2: Verify the DatabaseProvider API compatibility.The import path change from
@genshin-optimizer/sr/uito@genshin-optimizer/sr/db-uisuggests an architectural reorganization. Let's verify that the new DatabaseProvider maintains the same API to ensure compatibility.✅ Verification successful
Let me try a different approach to verify the DatabaseProvider's API compatibility.
Let me try one more time with the correct file type flags.
Import path change for DatabaseProvider is valid and safe
The verification shows that:
- The DatabaseProvider component is properly defined in
libs/sr/db-ui/src/provider/DatabaseProvider.tsx- It maintains a simple API accepting only
childrenprop- The implementation handles database state management consistently
- The new import path
@genshin-optimizer/sr/db-uicorrectly maps to the component's location🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Compare the DatabaseProvider exports between old and new locations # to ensure API compatibility # Search for DatabaseProvider exports in both locations echo "Checking DatabaseProvider exports in both locations:" ast-grep --pattern 'export { DatabaseProvider }' ast-grep --pattern 'export default function DatabaseProvider'Length of output: 229
Script:
#!/bin/bash # Search for DatabaseProvider definition and usage patterns echo "Searching for DatabaseProvider definition:" rg -l "DatabaseProvider" --type tsx --type ts echo -e "\nChecking DatabaseProvider implementation:" rg "DatabaseProvider" -A 5 --type tsx --type ts echo -e "\nChecking import statements for DatabaseProvider:" rg "import.*DatabaseProvider" --type tsx --type tsLength of output: 537
Script:
#!/bin/bash # Search for DatabaseProvider definition and usage patterns echo "Searching for DatabaseProvider definition:" rg -l "DatabaseProvider" echo -e "\nChecking DatabaseProvider implementation:" rg "DatabaseProvider" -A 5 echo -e "\nChecking import statements for DatabaseProvider:" rg "import.*DatabaseProvider"Length of output: 2324
libs/sr/page-team/src/TeamSelectors.tsx (2)
1-21: LGTM! Clean implementation of the main TeamSelectors component.The component is well-structured with appropriate imports and efficient rendering of team members.
23-32: LGTM! Clean component interface.The component's props and hooks usage is well-structured.
libs/sr/ui/src/Character/CharacterInventory.tsx (1)
7-7: LGTM! Good architectural improvement.The change from a relative import to an absolute import from
@genshin-optimizer/sr/db-uialigns well with the PR's goal of consolidating UI components. This centralization of the database context makes the codebase more maintainable and the dependencies more explicit.libs/sr/ui/src/Relic/RelicInventory.tsx (2)
6-6: LGTM! Import path standardization improves maintainability.The change from relative to absolute import path aligns with the codebase's move towards standardized database context access.
Line range hint
22-28: Verify component's integration with new team-based system.Given that the PR introduces a shift from loadout-based to team-based management, please verify:
- Should the relic inventory interact with the new team system?
- Does the current implementation of
RelicInventoryPropsneed to be updated to support the new team-based UI?libs/sr/solver/src/solver.ts (2)
1-1: LGTM! Import changes align with the new team-based architecture.The addition of the
sumutility and Team-related types properly supports the transition from loadout-based to team-based management.Also applies to: 4-10
29-29: Verify the migration from optTarget to frames across the codebase.The property and constructor changes correctly implement the transition to team-based frame optimization. However, we should verify that all callers have been updated.
Also applies to: 40-40, 48-48
✅ Verification successful
Let me gather more information about the Solver usage to verify the migration.
Based on the evidence from the shell scripts, I can now generate the final response:
Migration from optTarget to frames is correctly implemented
The codebase shows a clean migration:
- The only remaining
optTargetreferences are in the Genshin Impact (gi) codebase, which is separate from the Star Rail (sr) solver- The Solver constructor is correctly called with
framesparameter inlibs/sr/page-team/src/TeammateDisplay/Optimize/index.tsx- The implementation in
solver.tsproperly uses the frames for team-based optimization in thedetachNodesmethod🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining references to optTarget that might need updating rg "optTarget" --type ts # Search for new frame-based constructor calls to verify correct usage ast-grep --pattern 'new Solver($$$, $$$, frames, $$$)'Length of output: 2273
Script:
#!/bin/bash # Search for Solver constructor calls and their context rg "new Solver" -A 3 --type ts # Search for Solver class usage to understand the migration ast-grep --pattern 'class Solver { $$$ }'Length of output: 7819
libs/sr/ui/src/LightCone/LightConeInventory.tsx (1)
8-8: LGTM! Import path update aligns with best practices.The change from relative to absolute import path improves maintainability and is consistent with the codebase-wide updates.
libs/sr/page-team/src/TeamCalcProvider.tsx (3)
60-63: Consider using array mapping for team members.The previous review comment about hardcoded member variables is still valid. Consider refactoring to handle team members dynamically.
129-129: Clarify and address the TODO comment.The TODO comment about handling "tc build" needs clarification. What does "tc build" refer to? Please document this clearly or address the TODO if this is blocking functionality.
Would you like me to help implement the tc build handling or create a GitHub issue to track this?
Line range hint
150-169: LGTM! Well-structured member data creation.The implementation correctly handles character, light cone, and relic data. The type safety is properly maintained with appropriate filtering before type assertions.
libs/sr/page-team/src/TeammateDisplay/index.tsx (2)
3-3: LGTM: Import changes align with team-based architectureThe updated imports correctly reflect the architectural shift from loadout-based to team-based management.
Also applies to: 22-25
28-28: Verify component usage after prop removalThe removal of the
tabprop is a breaking change. Let's verify all usages ofTeammateDisplayhave been updated accordingly.✅ Verification successful
The results show usage in both SR and GI packages, and we need to verify if they are the same component or different implementations. This will help determine if the prop removal actually affects other components.
We need to verify if these are separate implementations for different games (Genshin Impact vs Star Rail) since they appear to be in different packages.
No issues with prop removal - separate implementations
The
TeammateDisplaycomponents in the GI (Genshin Impact) and SR (Star Rail) packages are completely separate implementations:
- SR implementation (
libs/sr/page-team/src/TeammateDisplay/index.tsx): A prop-less component that uses context- GI implementation (
libs/gi/page-team/src/TeamSetting/TeamComponents.tsx): A different component with required propsThe removal of the
tabprop from the SR implementation doesn't affect the GI implementation as they are separate components for different games.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining usage of TeammateDisplay with props ast-grep --pattern 'TeammateDisplay tab={$_}' # Search for any direct usage of TeammateDisplay rg -A 2 '<TeammateDisplay'Length of output: 705
Script:
#!/bin/bash # Let's check the TeammateDisplay component in the GI package to understand its interface cat libs/gi/page-team/src/TeamSetting/index.tsx # Also check if there are multiple TeammateDisplay components fd TeammateDisplayLength of output: 12834
Script:
#!/bin/bash # Let's check the TeammateDisplay implementation in TeamComponents cat "libs/gi/page-team/src/TeamSetting/TeamComponents.tsx" # And check the SR implementation cat "libs/sr/page-team/src/TeammateDisplay/index.tsx"Length of output: 13508
libs/sr/page-team/src/TeamCharacterSelector.tsx (2)
8-8: LGTM! Import changes align with team-based architecture.The shift to using
useTeamhook aligns well with the PR's objective of moving to team-based management.
145-145: Verify TeamSelectors integration with modal workflow.Please ensure that the newly added TeamSelectors component properly handles state updates and maintains consistency with the parent modal's state.
✅ Verification successful
Let me gather more information about the TeamSelectors component and its integration.
Let me check the TeammateSelector component since it's being used within TeamSelectors to understand the complete state management flow.
Let me check one final aspect - the TeammateSelector component's implementation and any state updates it performs.
Based on the gathered information, I can now provide the final response:
TeamSelectors component is properly integrated with state management
The implementation shows proper state handling through:
- TeamSelectors uses
useTeamhook to access team data- TeammateSelector component handles state updates via database.teams.set
- Clean state mutations through the
setCharKeyfunction that properly updates team metadataThe component maintains consistency with the parent modal's state through direct database updates rather than local state, ensuring data consistency across the application.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for potential state management issues between TeamSelectors and parent components # Test: Look for state management patterns in TeamSelectors ast-grep --pattern 'function TeamSelectors({ teamId }) { $$$ const [$_state, $_setState] = useState($_) $$$ }' # Test: Check for proper cleanup in useEffect hooks ast-grep --pattern 'useEffect(() => { $$$ return () => { $$$ } }, [$_])'Length of output: 2354
Script:
#!/bin/bash # Search for TeamSelectors component definition and its state management rg -A 10 "export.*TeamSelectors" # Look for any state updates or prop changes related to TeamSelectors ast-grep --pattern 'const TeamSelectors = ({ teamId }) => { $$$ }' # Check for any event handlers or callbacks passed to TeamSelectors rg -A 5 "<TeamSelectors"Length of output: 1382
Script:
#!/bin/bash # Check TeammateSelector implementation and its state management rg -A 15 "export.*TeammateSelector" # Look for any state updates in TeammateSelector ast-grep --pattern 'function TeammateSelector({ teamId, teamMetadataIndex }) { $$$ }' # Check for any event handlers or state updates in the parent TeamCharacterSelector rg -A 10 "export.*TeamCharacterSelector"Length of output: 1711
Script:
#!/bin/bash # Find TeammateSelector implementation rg -A 20 "function TeammateSelector" # Check for any state updates or mutations related to team data ast-grep --pattern 'const team = useTeam($teamId)' # Look for any team update functions or mutations rg -A 5 "updateTeam|setTeam|useTeamMutation"Length of output: 8273
libs/sr/ui/src/Character/CharacterEditor.tsx (2)
8-8: LGTM! Import changes align with the new architecture.The addition of database context hooks aligns well with the PR's objective of centralizing data management.
Line range hint
1-157: Verify integration with team-based architecture.The implementation aligns well with the PR's objectives of moving to a team-based approach. However, let's verify the integration points with the new team system.
✅ Verification successful
Integration with team-based architecture verified successfully
The implementation in
CharacterEditor.tsxcorrectly uses the database context hooks (useDatabaseContext,useCharacter) in a manner consistent with their usage across the codebase. The search results show these hooks are widely used in team-related components (TeamCard,TeamCalcProvider,TeamSelectors, etc.) and other UI components, confirming proper integration with the team-based architecture.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other components using the new database context to ensure consistent implementation rg "useDatabaseContext|useCharacter" --type typescriptLength of output: 91
Script:
#!/bin/bash # Search for other components using the new database context to ensure consistent implementation # Using ripgrep without file type filter since typescript extension is not recognized rg "useDatabaseContext|useCharacter"Length of output: 9446
libs/sr/page-team/src/BonusStats.tsx (2)
11-12: LGTM: Import changes align with team-based architecture.The changes correctly transition from loadout-based to team-based context management.
Also applies to: 26-26
63-76: 🛠️ Refactor suggestionOptimize rendering performance with memoization.
Consider memoizing the callback functions to prevent unnecessary re-renders.
+ const handleSetValue = useCallback( + (tag: Tag) => (value: number) => + database.teams.setBonusStat(teamId, tag, value, presetIndex), + [database, teamId, presetIndex] + ) + const handleDelete = useCallback( + (tag: Tag) => () => + database.teams.setBonusStat(teamId, tag, 0, presetIndex), + [database, teamId, presetIndex] + ) {bonusStats.map(({ tag, values }, i) => ( <BonusStatDisplay key={JSON.stringify(tag) + i} tag={tag} value={values[presetIndex]} - setValue={(value) => - database.teams.setBonusStat(teamId, tag, value, presetIndex) - } + setValue={handleSetValue(tag)} - onDelete={() => - database.teams.setBonusStat(teamId, tag, 0, presetIndex) - } + onDelete={handleDelete(tag)} setTag={(tag) => database.teams.setBonusStat(teamId, tag, 0, presetIndex) } /> ))}The previous review comment about preserving values when updating tags is still valid. Please refer to the existing comment for the solution.
libs/sr/page-team/src/index.tsx (1)
156-159:⚠️ Potential issueAdd null check before destructuring teamMetadatum.
The direct destructuring of characterKey from teamMetadatum could cause runtime errors. This issue was previously identified in a past review.
function TeammateDisplayWrapper() { const { - teamMetadatum: { characterKey }, + teamMetadatum, } = useTeamContext() + const characterKey = teamMetadatum?.characterKeylibs/sr/db/src/Database/DataManagers/BuildTcDataManager.ts (1)
62-62:⚠️ Potential issueImplement team reference cleanup.
The TODO comment indicates missing cleanup logic for build TC references in teams. This should be addressed to prevent orphaned references and potential memory leaks.
Let's verify the current usage of build TCs in teams:
libs/sr/ui/src/Settings/DatabaseCard.tsx (2)
8-8: LGTM: Import path standardizationThe change from relative to module import path aligns with best practices and the broader context management standardization effort.
Line range hint
103-122: Add validation for database operationsThe download operation creates and manipulates DOM elements directly and handles potentially large JSON data without validation.
Consider adding the following safeguards:
- Validate database size before operations:
const MAX_DB_SIZE = 10 * 1024 * 1024; // 10MB example limit const validateDatabaseSize = (data: string): boolean => { const size = new Blob([data]).size; return size <= MAX_DB_SIZE; };
- Use URL.createObjectURL for safer file downloads:
- const a = document.createElement('a'); - a.download = filename; - a.href = `data:${contentType},${encodeURIComponent(JSONStr)}`; + const blob = new Blob([JSONStr], { type: contentType }); + const url = URL.createObjectURL(blob); + const a = document.createElement('a'); + a.href = url; + a.download = filename; a.click(); - document.body.removeChild(a); + URL.revokeObjectURL(url);libs/sr/ui/src/Settings/UploadCard.tsx (2)
Line range hint
1-400: Overall implementation is well-structured and aligns with PR objectives.The component implementation demonstrates:
- Clear separation of concerns
- Effective use of Material-UI components
- Proper internationalization
- Good error handling and user feedback
The changes align well with the PR's objective of improving UI design and modularity.
9-9: LGTM: Import statement update aligns with architectural changes.The change from local context to a package import (
@genshin-optimizer/sr/db-ui) aligns with the PR's objective of improving modularity and consolidating UI components.Let's verify that this change is consistent across the codebase:
✅ Verification successful
Import statement update is consistently applied across the codebase
The verification shows that:
- No files are using the old local context import pattern
- The new import
@genshin-optimizer/sr/db-uiis consistently used across 21 files in the codebaseThis confirms that the import change in
UploadCard.tsxis part of a properly implemented architectural change.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any remaining local database context imports that should be updated # and verify the new import is used consistently. echo "Checking for any remaining local database context imports..." rg -l "import.*useDatabaseContext.*from.*['\"].*Context['\"]" echo "Verifying consistent usage of the new import path..." rg -l "import.*useDatabaseContext.*from.*['\"]@genshin-optimizer/sr/db-ui['\"]"Length of output: 1383
libs/sr/ui/src/LightCone/LightConeEditor/LightConeEditor.tsx (1)
21-21: LGTM: Import path improvement aligns with architecture changesThe change to use an absolute import path from
@genshin-optimizer/sr/db-uiimproves maintainability and aligns with the broader architectural changes moving from loadout-based to team-based management.libs/sr/page-team/src/TalentContent.tsx (1)
339-347: Consider implementing the hideHeader functionality.The commented-out
hideHeaderprop suggests that this functionality might be needed. If this feature is part of the UI redesign requirements, it should be implemented.Let's check if this functionality is used elsewhere in the codebase:
| "compilerOptions": { | ||
| "jsx": "react-jsx", | ||
| "allowJs": false, | ||
| "esModuleInterop": false, | ||
| "allowSyntheticDefaultImports": true, | ||
| "strict": true, | ||
| "jsxImportSource": "@emotion/react" | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding explicit module resolution settings.
While the current configuration works, you might want to consider adding explicit module resolution settings for better clarity:
{
"compilerOptions": {
"jsx": "react-jsx",
"allowJs": false,
"esModuleInterop": false,
"allowSyntheticDefaultImports": true,
"strict": true,
- "jsxImportSource": "@emotion/react"
+ "jsxImportSource": "@emotion/react",
+ "moduleResolution": "node",
+ "resolveJsonModule": true
},📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "compilerOptions": { | |
| "jsx": "react-jsx", | |
| "allowJs": false, | |
| "esModuleInterop": false, | |
| "allowSyntheticDefaultImports": true, | |
| "strict": true, | |
| "jsxImportSource": "@emotion/react" | |
| }, | |
| "compilerOptions": { | |
| "jsx": "react-jsx", | |
| "allowJs": false, | |
| "esModuleInterop": false, | |
| "allowSyntheticDefaultImports": true, | |
| "strict": true, | |
| "jsxImportSource": "@emotion/react", | |
| "moduleResolution": "node", | |
| "resolveJsonModule": true | |
| }, |
| "types": [ | ||
| "node", | ||
|
|
||
| "@nx/react/typings/cssmodule.d.ts", | ||
| "@nx/react/typings/image.d.ts" | ||
| ] |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding more essential type definitions.
While the current type definitions cover Node.js and React-specific typings, consider adding other essential types that might be needed:
@types/reactfor React-specific types@types/react-domfor DOM-related React types
"types": [
"node",
+ "@types/react",
+ "@types/react-dom",
"@nx/react/typings/cssmodule.d.ts",
"@nx/react/typings/image.d.ts"
]📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "types": [ | |
| "node", | |
| "@nx/react/typings/cssmodule.d.ts", | |
| "@nx/react/typings/image.d.ts" | |
| ] | |
| "types": [ | |
| "node", | |
| "@types/react", | |
| "@types/react-dom", | |
| "@nx/react/typings/cssmodule.d.ts", | |
| "@nx/react/typings/image.d.ts" | |
| ] |
| "src/**/*.spec.jsx", | ||
| "src/**/*.test.jsx" | ||
| ], | ||
| "include": ["src/**/*.js", "src/**/*.jsx", "src/**/*.ts", "src/**/*.tsx"] |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider being more specific with file inclusions.
The current include pattern might be too broad. Consider structuring it to match your source code organization:
-"include": ["src/**/*.js", "src/**/*.jsx", "src/**/*.ts", "src/**/*.tsx"]
+"include": [
+ "src/components/**/*",
+ "src/hooks/**/*",
+ "src/utils/**/*",
+ "src/types/**/*"
+]Committable suggestion was skipped due to low confidence.
libs/sr/page-team/src/index.tsx
Outdated
| if (characterKey && characterKey !== characterKeyRaw) | ||
| navigate(`${characterKey}`, { replace: true }) | ||
| }, [characterKey, characterKeyRaw, teamMetadatum, navigate]) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance navigation effect robustness.
The navigation effect should handle the case where characterKey is undefined more explicitly.
useEffect(() => {
- if (characterKey && characterKey !== characterKeyRaw)
+ if (!characterKey) {
+ navigate('', { replace: true })
+ } else if (characterKey !== characterKeyRaw) {
navigate(`${characterKey}`, { replace: true })
+ }
}, [characterKey, characterKeyRaw, teamMetadatum, navigate])📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (characterKey && characterKey !== characterKeyRaw) | |
| navigate(`${characterKey}`, { replace: true }) | |
| }, [characterKey, characterKeyRaw, teamMetadatum, navigate]) | |
| useEffect(() => { | |
| if (!characterKey) { | |
| navigate('', { replace: true }) | |
| } else if (characterKey !== characterKeyRaw) { | |
| navigate(`${characterKey}`, { replace: true }) | |
| } | |
| }, [characterKey, characterKeyRaw, teamMetadatum, navigate]) |
| // TODO: validate conditionals src dst condKey | ||
| if (!Array.isArray(condValues)) return false |
There was a problem hiding this comment.
Implement missing validation logic.
Several critical validation checks are missing:
- Validation of conditionals' sheet, src, and dst properties
- Validation of frames
- Validation of bonusStats
These validations are important for maintaining data integrity and preventing runtime errors.
Would you like me to help implement these validation checks? I can provide a comprehensive implementation that includes:
- Type checking for sheet, src, and dst
- Frame structure validation
- BonusStats validation with proper error handling
Also applies to: 200-202
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (4)
libs/sr/page-team/src/TeammateDisplay/Optimize/OptConfigWrapper.tsx (2)
1-1: Rename file to match exported component name.The file is named
OptConfigWrapper.tsxbut exportsOptConfigProvider. Consider renaming the file toOptConfigProvider.tsxfor better consistency.
10-28: Architecture aligns well with the new UI design.The
OptConfigProvidercomponent effectively supports the new single-page layout and team combo concept by:
- Providing centralized access to optimization configuration
- Supporting the sticky optimization tools mentioned in the PR objectives
libs/common/util/src/lib/array.ts (1)
79-91: Add return type annotation for clarity.The implementation looks good and addresses most of the previous review comments. However, the return type annotation is still missing.
Apply this small change:
-export function pruneOrPadArray<T>(array: T[], length: number, value: T) { +export function pruneOrPadArray<T>(array: T[], length: number, value: T): T[] {libs/sr/page-team/src/TeammateDisplay/Optimize/index.tsx (1)
Line range hint
87-103: Optimize relicsBySlot computationThe
relicsBySlotcomputation could be expensive for large datasets and is recomputed on every database.relics.values change.Consider these optimizations:
- Move the reduction to the database layer
- Use a more efficient data structure
+ // Add this to the database layer + const getRelicsBySlot = useCallback(() => { + const slotMap = new Map<RelicSlotKey, ICachedRelic[]>() + for (const relic of database.relics.values) { + const slotRelics = slotMap.get(relic.slotKey) ?? [] + slotRelics.push(relic) + slotMap.set(relic.slotKey, slotRelics) + } + return Object.fromEntries(slotMap) as Record<RelicSlotKey, ICachedRelic[]> + }, [database.relics.values]) - const relicsBySlot = useMemo(...) + const relicsBySlot = useMemo(() => getRelicsBySlot(), [getRelicsBySlot])
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
- libs/common/util/src/lib/array.ts (1 hunks)
- libs/common/util/src/lib/object.ts (2 hunks)
- libs/sr/page-team/src/ComboEditor.tsx (1 hunks)
- libs/sr/page-team/src/TeammateDisplay/Optimize/OptConfigWrapper.tsx (1 hunks)
- libs/sr/page-team/src/TeammateDisplay/Optimize/index.tsx (6 hunks)
- libs/sr/page-teams/src/TeamCard.tsx (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- libs/common/util/src/lib/object.ts
- libs/sr/page-team/src/ComboEditor.tsx
🧰 Additional context used
🪛 Biome
libs/sr/page-team/src/TeammateDisplay/Optimize/index.tsx
[error] 120-120: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
🔇 Additional comments (5)
libs/sr/page-team/src/TeammateDisplay/Optimize/OptConfigWrapper.tsx (3)
6-9: Consider using a more type-safe default value for OptConfig.The current implementation uses type assertion which could lead to runtime errors.
17-18: Add error handling and loading states.The non-null assertion on the database result is unsafe.
19-22: LGTM! Proper memoization implementation.The
useMemohook is correctly used to optimize performance by memoizing the context value based on its dependencies.libs/sr/page-teams/src/TeamCard.tsx (1)
3-3: LGTM! Import path change aligns with the new architecture.The change from relative to absolute import path is consistent with the architectural changes for team-based management.
libs/sr/page-team/src/TeammateDisplay/Optimize/index.tsx (1)
Line range hint
203-229: LGTM! Well-structured progress indicatorThe ProgressIndicator component is well-implemented with proper internationalization and clear presentation of progress metrics.
| {team.teamMetadata.map((cmdata, i) => ( | ||
| <CardThemed | ||
| key={`${i}-${cmdata?.characterKey}`} | ||
| sx={{ flexGrow: 1 }} | ||
| > | ||
| <Typography>{cmdata?.characterKey ?? 'Empty'} </Typography> | ||
| </CardThemed> | ||
| ))} |
There was a problem hiding this comment.
Improve key generation and add TypeScript types.
While the conditional rendering has been improved, there are still some concerns:
- The current key format
${i}-${cmdata?.characterKey}could result in keys like "0-undefined" whencmdatais null, which isn't ideal for React's reconciliation. - The component would benefit from proper TypeScript types for the team object.
Consider this improved implementation:
+ import type { Team } from '@genshin-optimizer/sr/db' // Add type import
- {team.teamMetadata.map((cmdata, i) => (
- <CardThemed
- key={`${i}-${cmdata?.characterKey}`}
- sx={{ flexGrow: 1 }}
- >
- <Typography>{cmdata?.characterKey ?? 'Empty'} </Typography>
- </CardThemed>
- ))}
+ {team.teamMetadata.map((cmdata, slotIndex) => (
+ <CardThemed
+ key={`team-${teamId}-slot-${slotIndex}`}
+ sx={{ flexGrow: 1 }}
+ >
+ <Typography>{cmdata?.characterKey ?? 'Empty'}</Typography>
+ </CardThemed>
+ ))}This change:
- Uses a more stable key format that includes the team ID and slot index
- Renames the index parameter to be more descriptive
- Suggests adding proper type imports
Committable suggestion was skipped due to low confidence.
libs/common/util/src/lib/array.ts
Outdated
| /** | ||
| * Shorten or pad an array to a certain length, with a default value. | ||
| * Modifies the array in-place. | ||
| * @param array | ||
| * @param length | ||
| * @param value | ||
| * @returns The modified array | ||
| */ | ||
| export function pruneOrPadArray<T>(array: T[], length: number, value: T) { | ||
| if (array.length > length) array.length = length | ||
| else array.push(...new Array(length - array.length).fill(value)) | ||
| return array | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding input validation and examples.
To make the function more robust and user-friendly:
- Add validation for negative length
- Add examples in the documentation showing common use cases
Here's a suggested improvement:
/**
* Shorten or pad an array to a certain length, with a default value.
* Modifies the array in-place.
* @param array The array to modify
* @param length The target length
* @param value The value to use for padding
* @returns The modified array
+ * @throws {Error} If length is negative
+ * @example
+ * // Truncate array
+ * pruneOrPadArray([1, 2, 3], 2, 0) // returns [1, 2]
+ *
+ * // Pad array
+ * pruneOrPadArray([1], 3, 0) // returns [1, 0, 0]
*/
export function pruneOrPadArray<T>(array: T[], length: number, value: T): T[] {
+ if (length < 0) {
+ throw new Error('Length must be non-negative')
+ }
if (array.length > length) array.length = length
else array.push(...new Array(length - array.length).fill(value))
return array
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /** | |
| * Shorten or pad an array to a certain length, with a default value. | |
| * Modifies the array in-place. | |
| * @param array | |
| * @param length | |
| * @param value | |
| * @returns The modified array | |
| */ | |
| export function pruneOrPadArray<T>(array: T[], length: number, value: T) { | |
| if (array.length > length) array.length = length | |
| else array.push(...new Array(length - array.length).fill(value)) | |
| return array | |
| } | |
| /** | |
| * Shorten or pad an array to a certain length, with a default value. | |
| * Modifies the array in-place. | |
| * @param array The array to modify | |
| * @param length The target length | |
| * @param value The value to use for padding | |
| * @returns The modified array | |
| * @throws {Error} If length is negative | |
| * @example | |
| * // Truncate array | |
| * pruneOrPadArray([1, 2, 3], 2, 0) // returns [1, 2] | |
| * | |
| * // Pad array | |
| * pruneOrPadArray([1], 3, 0) // returns [1, 0, 0] | |
| */ | |
| export function pruneOrPadArray<T>(array: T[], length: number, value: T): T[] { | |
| if (length < 0) { | |
| throw new Error('Length must be non-negative') | |
| } | |
| if (array.length > length) array.length = length | |
| else array.push(...new Array(length - array.length).fill(value)) | |
| return array | |
| } |
| const [numWorkers, setNumWorkers] = useState(8) | ||
| const [progress, setProgress] = useState<ProgressResult | undefined>( | ||
| undefined | ||
| ) | ||
| const { loadout } = useContext(LoadoutContext) | ||
| const optConfig = useDataManagerBase(database.optConfigs, loadout.optConfigId) | ||
|
|
||
| const optTarget = optConfig?.optimizationTarget | ||
| const setOptTarget = useCallback( | ||
| (optimizationTarget: Tag) => { | ||
| database.optConfigs.set(loadout.optConfigId, { | ||
| optimizationTarget, | ||
| }) | ||
| }, | ||
| [database.optConfigs, loadout.optConfigId] | ||
| ) | ||
| const { optConfig } = useContext(OptConfigContext) | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consolidate related state management
The component uses multiple separate state variables that are conceptually related. Consider consolidating them for better state management.
Consider applying this improvement:
- const [progress, setProgress] = useState<ProgressResult | undefined>(undefined)
- const [build, setBuild] = useState<BuildResult | undefined>(undefined)
- const [optimizing, setOptimizing] = useState(false)
+ const [optimizationState, setOptimizationState] = useState<{
+ progress?: ProgressResult
+ build?: BuildResult
+ isOptimizing: boolean
+ }>({
+ isOptimizing: false
+ })Committable suggestion was skipped due to low confidence.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (2)
libs/sr/page-team/src/TeamSelectors.tsx (2)
13-19: Improve key prop for better React reconciliation.Using array index in keys can lead to rendering issues when items are reordered. Consider using a more stable unique identifier:
- key={`${index}_${meta?.characterKey}`} + key={meta?.characterKey ?? `empty_${index}`}
1-79: Implementation aligns well with the new team-based architecture.The component successfully implements the new team-based management approach, allowing:
- Characters to be part of multiple teams
- Flexible team composition through metadata
- Character swapping between team positions
This aligns perfectly with the PR's objective of moving away from loadout-based management.
🧰 Tools
🪛 Biome
[error] 39-39: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- libs/common/util/src/lib/object.ts (2 hunks)
- libs/sr/page-team/src/TeamSelectors.tsx (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- libs/common/util/src/lib/object.ts
🧰 Additional context used
🪛 Biome
libs/sr/page-team/src/TeamSelectors.tsx
[error] 39-39: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
| import { memo } from 'react' | ||
|
|
||
| export default function TeamSelectors({ teamId }: { teamId: string }) { | ||
| const team = useTeam(teamId)! |
There was a problem hiding this comment.
Add error handling for non-existent teams.
The non-null assertion (!) assumes the team always exists. Consider adding error handling:
- const team = useTeam(teamId)!
+ const team = useTeam(teamId)
+ if (!team) {
+ return <Box>Team not found</Box>
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const team = useTeam(teamId)! | |
| const team = useTeam(teamId) | |
| if (!team) { | |
| return <Box>Team not found</Box> | |
| } |
| // Create char if needed | ||
| database.chars.getOrCreate(cKey) |
There was a problem hiding this comment.
Add await for asynchronous database operation.
The getOrCreate operation appears to be asynchronous but isn't awaited:
- database.chars.getOrCreate(cKey)
+ await database.chars.getOrCreate(cKey)Make sure to update the parent function signature:
- function setCharKey(cKey: CharacterKey | '') {
+ async function setCharKey(cKey: CharacterKey | '') {Committable suggestion was skipped due to low confidence.
| team.teamMetadata[teamMetadataIndex] = { | ||
| characterKey: cKey, | ||
| } as TeamMetadatum | ||
| }) |
There was a problem hiding this comment.
Replace type assertion with proper type initialization.
Using type assertion (as TeamMetadatum) bypasses TypeScript's type checking. Consider initializing all required properties:
- team.teamMetadata[teamMetadataIndex] = {
- characterKey: cKey,
- } as TeamMetadatum
+ const newMetadata: TeamMetadatum = {
+ characterKey: cKey,
+ // Add other required properties with their default values
+ }
+ team.teamMetadata[teamMetadataIndex] = newMetadata
+ return teamCommittable suggestion was skipped due to low confidence.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
libs/sr/formula/src/formula.test.ts (1)
146-150: Consider adding specific relic buff test cases.The addition of
allRelicSetKeysto the validation set is correct, but consider adding test cases that specifically verify relic-related buff entries to ensure complete coverage of the new validation scope.Consider:
- Adding a descriptive test name that reflects all validated types (e.g., "validate character, light cone, and relic buff entries").
- Adding test cases that specifically exercise relic buff validations.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- libs/sr/formula/src/formula.test.ts (2 hunks)
🔇 Additional comments (1)
libs/sr/formula/src/formula.test.ts (1)
8-8: LGTM!The import is properly placed and follows the existing pattern for similar constant imports.
frzyc
force-pushed
the
combo_only_experiment
branch
from
183920e to
b030d23
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
libs/sr/page-team/src/TalentContent.tsx (1)
408-408: Address TODO comment for levelBoost calculation.The levelBoost calculation is currently hardcoded to 0 with a TODO comment. This should be implemented to properly calculate talent level boosts.
Would you like me to help implement the levelBoost calculation or create a GitHub issue to track this task?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- libs/sr/page-team/src/TalentContent.tsx (8 hunks)
- libs/sr/ui/src/LightCone/LightConeEditor/LightConeEditor.tsx (1 hunks)
- libs/sr/ui/src/Relic/RelicEditor/RelicEditor.tsx (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- libs/sr/ui/src/LightCone/LightConeEditor/LightConeEditor.tsx
- libs/sr/ui/src/Relic/RelicEditor/RelicEditor.tsx
🔇 Additional comments (1)
libs/sr/page-team/src/TalentContent.tsx (1)
47-48: Verify the impact of context migration.The switch from loadout context to team context is aligned with the PR objectives. However, we should ensure that all character-related functionality continues to work as expected.
✅ Verification successful
Based on the search results, I can now verify the context migration from loadout to team context. The search results show that:
- The
teamMetadatumproperty is correctly used in the new team-based structure- The
characterKeyis properly accessed through the team context- No remaining loadout context usage was found in the codebase
- The migration appears to be complete and consistent
Migration to team context is properly implemented
The switch from loadout context to team context has been correctly implemented. The
characterKeyis now accessed throughteamMetadatumin the team context, which aligns with the new team-based management approach. No legacy loadout context usage remains that could cause conflicts.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any remaining loadout context usage that might conflict with the new team context # Test: Search for loadout context usage. Expect: No instances of LoadoutContext or useLoadoutContext rg -i "loadoutcontext|useloadoutcontext"Length of output: 42
Script:
#!/bin/bash # Let's verify the migration more comprehensively by: # 1. Checking for any remaining loadout-related code # 2. Verifying the team context implementation # 3. Checking the usage pattern of characterKey # Check for any loadout-related terms rg -i "loadout" -A 2 -B 2 # Check the team context implementation and usage ast-grep --pattern 'const { teamMetadatum } = useTeamContext()' # Check characterKey usage pattern rg "characterKey" -A 2 -B 2Length of output: 445162
| {sheetElement.documents.map((doc, i) => ( | ||
| <DocumentDisplay | ||
| key={i} | ||
| document={doc} | ||
| collapse | ||
| // hideHeader={hideHeader} | ||
| setConditional={setConditional} | ||
| /> | ||
| ))} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider memoizing document display components.
The document mapping could trigger unnecessary re-renders. Consider using useMemo for the DocumentDisplay components if the documents array is stable.
- {sheetElement.documents.map((doc, i) => (
- <DocumentDisplay
- key={i}
- document={doc}
- collapse
- setConditional={setConditional}
- />
- ))}
+ {useMemo(
+ () =>
+ sheetElement.documents.map((doc, i) => (
+ <DocumentDisplay
+ key={i}
+ document={doc}
+ collapse
+ setConditional={setConditional}
+ />
+ )),
+ [sheetElement.documents, setConditional]
+ )}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| {sheetElement.documents.map((doc, i) => ( | |
| <DocumentDisplay | |
| key={i} | |
| document={doc} | |
| collapse | |
| // hideHeader={hideHeader} | |
| setConditional={setConditional} | |
| /> | |
| ))} | |
| {useMemo( | |
| () => | |
| sheetElement.documents.map((doc, i) => ( | |
| <DocumentDisplay | |
| key={i} | |
| document={doc} | |
| collapse | |
| // hideHeader={hideHeader} | |
| setConditional={setConditional} | |
| /> | |
| )), | |
| [sheetElement.documents, setConditional] | |
| )} |
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Outside diff range and nitpick comments (6)
libs/sr/page-team/src/TeamCalcProvider.tsx (1)
130-130: Address TODO comment about tc build handling.
The comment indicates missing functionality for handling tc builds. This should be addressed to ensure complete feature implementation.
Would you like me to help implement the tc build handling or create a GitHub issue to track this task?
libs/sr/page-team/src/index.tsx (2)
132-142: Remove commented styling code.
The commented-out styling code should be removed if it's no longer needed. If it's intended for future use, consider moving it to a separate file or adding a TODO comment explaining the plan.
Line range hint 156-172: Add error boundary protection.
Consider wrapping the component with an error boundary to gracefully handle potential runtime errors, especially for character loading failures.
+import { ErrorBoundary } from '@genshin-optimizer/common/ui'
function TeammateDisplayWrapper() {
const {
teammateDatum: { characterKey },
} = useTeamContext()
const character = useCharacter(characterKey)
if (!character)
return <Skeleton variant="rectangular" width="100%" height={1000} />
return (
+ <ErrorBoundary fallback={<div>Error loading character data</div>}>
<CharacterContext.Provider value={character}>
<Routes>
<Route path=":characterKey">
<Route path="*" index element={<TeammateDisplay />} />
</Route>
</Routes>
</CharacterContext.Provider>
+ </ErrorBoundary>
)
}libs/sr/page-team/src/TeammateDisplay/Optimize/index.tsx (1)
Line range hint 114-117: Enhance cleanup logic in useEffect
The current cleanup might not handle all resources properly if the component unmounts during optimization.
Consider enhancing the cleanup:
useEffect(() => () => {
+ // Ensure optimization is stopped
+ setOptimizing(false)
cancelToken.current()
+ // Reset states
+ setProgress(undefined)
+ setBuild(undefined)
}, [])🧰 Tools
🪛 Biome
[error] 120-120: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
libs/sr/db/src/Database/DataManagers/TeamDataManager.ts (1)
40-52: Consider restructuring frame data to prevent synchronization issues.
The current "sparse 2d array" approach requires manual synchronization of multiple arrays (frames, conditionals.condValues, bonusStats.values). This could lead to maintenance issues and bugs.
Consider restructuring to use a Frame object as suggested in the previous discussion:
type Frame = {
conditionals: Array<{
sheet: Sheet
src: Member | 'all'
dst: Member | 'all'
condKey: string
condValue: number
}>
bonusStats: Array<{
tag: Tag
value: number
}>
}
interface Team {
// ...
frames: Array<Frame>
// ...
}While this might increase storage size, it would:
- Prevent array synchronization issues
- Make the code more maintainable
- Improve type safety
- Make it easier to add new frame-specific properties
libs/sr/page-team/src/BonusStats.tsx (1)
Line range hint 98-99: Improve the method for inferring percentage-based stats
There's a TODO comment regarding inferring whether a stat is percentage-based:
// TODO: best way to infer percentage from tag?
const isPercent = (tag.name || tag.q)?.endsWith('_')This heuristic might not cover all cases accurately, especially if there are stat keys that don't follow the naming convention strictly. Consider implementing a utility function or maintaining a list of percentage-based stat keys to make this determination more reliable.
Would you like assistance in creating a utility function to accurately determine if a stat is percentage-based? I can help implement this to ensure consistency across your codebase.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (12)
- libs/sr/db/src/Database/DataManagers/TeamDataManager.ts (9 hunks)
- libs/sr/page-team/src/BonusStats.tsx (5 hunks)
- libs/sr/page-team/src/TalentContent.tsx (8 hunks)
- libs/sr/page-team/src/TeamCalcProvider.tsx (5 hunks)
- libs/sr/page-team/src/TeamCharacterSelector.tsx (3 hunks)
- libs/sr/page-team/src/TeamSelectors.tsx (1 hunks)
- libs/sr/page-team/src/TeammateDisplay/Optimize/index.tsx (6 hunks)
- libs/sr/page-team/src/TeammateDisplay/index.tsx (2 hunks)
- libs/sr/page-team/src/context/TeamContext.ts (1 hunks)
- libs/sr/page-team/src/index.tsx (4 hunks)
- libs/sr/ui/src/Character/CharacterInventory.tsx (1 hunks)
- libs/sr/ui/src/Character/index.tsx (0 hunks)
💤 Files with no reviewable changes (1)
- libs/sr/ui/src/Character/index.tsx
🚧 Files skipped from review as they are similar to previous changes (5)
- libs/sr/page-team/src/TalentContent.tsx
- libs/sr/page-team/src/TeamCharacterSelector.tsx
- libs/sr/page-team/src/TeammateDisplay/index.tsx
- libs/sr/page-team/src/context/TeamContext.ts
- libs/sr/ui/src/Character/CharacterInventory.tsx
🧰 Additional context used
🪛 Biome
libs/sr/page-team/src/TeamSelectors.tsx
[error] 39-39: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
libs/sr/page-team/src/TeammateDisplay/Optimize/index.tsx
[error] 120-120: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
🔇 Additional comments (11)
libs/sr/page-team/src/TeamSelectors.tsx (1)
73-79: Handle async character selection.
Since setCharKey is now async, ensure CharacterAutocomplete properly handles the Promise.
Let's verify the implementation of CharacterAutocomplete:
libs/sr/page-team/src/TeamCalcProvider.tsx (4)
12-37: LGTM: Type and import changes align with the new team-based architecture.
The renaming from LoadoutMetadatum to TeammateDatum and the addition of new types properly reflect the transition to a team-based system.
70-74: LGTM: Improved team member filtering logic.
The new implementation properly handles undefined team members and ensures type safety through explicit filtering.
Line range hint 150-169: LGTM: Clean implementation of member data creation.
The createMember function effectively combines character, light cone, and relic data in a clean and maintainable way.
110-117: LGTM: Robust preset-based tag handling.
The implementation properly handles undefined cases and correctly integrates with the preset system.
libs/sr/page-team/src/index.tsx (3)
7-7: LGTM: Clean architectural transition to team-based management.
The import changes effectively support the transition from loadout-based to team-based management, aligning with the PR objectives for the UI redesign.
Also applies to: 9-9, 19-23
84-86: Enhance URL synchronization robustness.
The URL synchronization effect should handle undefined characterKey explicitly, as mentioned in past reviews.
100-107: Verify team context integration.
The team context implementation looks correct, but let's verify the integration with the solver mentioned in the PR objectives.
✅ Verification successful
Let me try to find the team context hook definition and its usage patterns to better understand the integration.
Based on the search results, I can now generate a final response:
Team context implementation is correctly integrated
The team context is properly implemented and used across multiple components:
- Used in
ComboEditorfor team optimization - Integrated with character talents in
TalentContent - Connected to bonus stats management in
BonusStats - Properly scoped to team-specific data with
teamId,team, andteammateDatum
The implementation aligns with the PR objectives of supporting team combos and character optimization, with the context providing necessary team data to all child components.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for solver integration with team context
rg -l "useTeamContext.*solver" --type tsx
Length of output: 71
Script:
#!/bin/bash
# Search for team context usage and solver integration with proper file extensions
rg -l "useTeamContext" --type ts --type tsx --type jsx --type js
# Search for solver-related code near team context
rg "useTeamContext|solver" -C 3 --type ts --type tsx --type jsx --type js
# Find files containing both team context and solver references
rg -l "solver.*team|team.*solver" --type ts --type tsx --type jsx --type js
Length of output: 302
Script:
#!/bin/bash
# Find the team context hook definition
ast-grep --pattern 'export const useTeamContext = $_'
# Look for TeamContext provider component
ast-grep --pattern 'export const TeamContext = createContext<$_>($_)'
Length of output: 5858
Script:
#!/bin/bash
# Let's try searching without file type restrictions
rg "useTeamContext" -C 5
# Search for team-related solver code
rg "solver.*team|team.*solver" -C 5
# Look for team context hook implementation
rg "const useTeamContext" -C 5
Length of output: 9021
libs/sr/page-team/src/TeammateDisplay/Optimize/index.tsx (3)
119-120: Improve promise cancellation token assignment
The assignment within the Promise executor makes the code harder to read and understand.
🧰 Tools
🪛 Biome
[error] 120-120: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
132-138: Add validation for optimization parameters
The Solver instantiation lacks validation for required parameters, which could lead to runtime errors.
Add parameter validation:
+ if (!team.frames?.length) {
+ console.error('No frames available for optimization')
+ return
+ }
+ if (Object.values(relicsBySlot).some(relics => !relics.length)) {
+ console.error('Missing relics in some slots')
+ return
+ }
const optimizer = new Solver(
characterKey,
calc,
team.frames,
statFilters,
relicsBySlot,
numWorkers,
setProgress
)Line range hint 205-234: LGTM! Well-structured progress indicator
The component effectively displays progress with proper internationalization and clear progress calculation.
| const TeammateSelector = memo(function TeammateSelector({ | ||
| teamMetadataIndex, | ||
| teamId, | ||
| }: { | ||
| teamMetadataIndex: number | ||
| teamId: string | ||
| }) { | ||
| const { database } = useDatabaseContext() | ||
| const team = useTeam(teamId)! | ||
|
|
There was a problem hiding this comment.
Add proper error handling for team lookup.
Similar to the main component, replace the non-null assertion with proper error handling.
Apply this improvement:
const TeammateSelector = memo(function TeammateSelector({
teamMetadataIndex,
teamId,
}: {
teamMetadataIndex: number
teamId: string
}) {
const { database } = useDatabaseContext()
const team = useTeam(teamId)
+ if (!team) {
+ return null
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const TeammateSelector = memo(function TeammateSelector({ | |
| teamMetadataIndex, | |
| teamId, | |
| }: { | |
| teamMetadataIndex: number | |
| teamId: string | |
| }) { | |
| const { database } = useDatabaseContext() | |
| const team = useTeam(teamId)! | |
| const TeammateSelector = memo(function TeammateSelector({ | |
| teamMetadataIndex, | |
| teamId, | |
| }: { | |
| teamMetadataIndex: number | |
| teamId: string | |
| }) { | |
| const { database } = useDatabaseContext() | |
| const team = useTeam(teamId) | |
| if (!team) { | |
| return null | |
| } | |
| export default function TeamSelectors({ teamId }: { teamId: string }) { | ||
| const team = useTeam(teamId)! | ||
|
|
||
| return ( | ||
| <Box display="flex" flexDirection="column" gap={1}> | ||
| {team.teamMetadata.map((meta, index) => ( | ||
| <TeammateSelector | ||
| key={`${index}_${meta?.characterKey}`} | ||
| teamMetadataIndex={index} | ||
| teamId={teamId} | ||
| /> | ||
| ))} | ||
| </Box> | ||
| ) | ||
| } |
There was a problem hiding this comment.
Add proper type safety and error handling.
The component has several areas for improvement:
- Replace non-null assertion with proper error handling
- Add TypeScript interface for props
- Simplify key prop usage
Apply these improvements:
+interface TeamSelectorsProps {
+ teamId: string;
+}
-export default function TeamSelectors({ teamId }: { teamId: string }) {
+export default function TeamSelectors({ teamId }: TeamSelectorsProps) {
const team = useTeam(teamId)
+ if (!team) {
+ return <Box>Team not found</Box>
+ }
return (
<Box display="flex" flexDirection="column" gap={1}>
{team.teamMetadata.map((meta, index) => (
<TeammateSelector
- key={`${index}_${meta?.characterKey}`}
+ key={index}
teamMetadataIndex={index}
teamId={teamId}
/>
))}
</Box>
)
}Committable suggestion was skipped due to low confidence.
| function setCharKey(cKey: CharacterKey | '') { | ||
| // Remove teammate | ||
| if (cKey === '') { | ||
| database.teams.set( | ||
| teamId, | ||
| (team) => (team.teamMetadata[teamMetadataIndex] = undefined) | ||
| ) | ||
| return | ||
| } | ||
|
|
||
| // Create char if needed | ||
| database.chars.getOrCreate(cKey) | ||
|
|
||
| // Check if character is already in the team. | ||
| const existingIndex = team.teamMetadata.findIndex( | ||
| (teammateDatum) => teammateDatum?.characterKey === cKey | ||
| ) | ||
| // If not exists, insert it | ||
| if (existingIndex === -1) { | ||
| // If none, create it | ||
| database.teams.set(teamId, (team) => { | ||
| // Let the validator handle default properties for everything else | ||
| team.teamMetadata[teamMetadataIndex] = { | ||
| characterKey: cKey, | ||
| } as TeammateDatum | ||
| }) | ||
| } | ||
| // If exists already, swap positions | ||
| else { | ||
| if (existingIndex === teamMetadataIndex) return | ||
| const existingMeta = team.teamMetadata[existingIndex] | ||
| const destMeta = team.teamMetadata[teamMetadataIndex] | ||
| database.teams.set(teamId, (team) => { | ||
| team.teamMetadata[teamMetadataIndex] = existingMeta | ||
| team.teamMetadata[existingIndex] = destMeta | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
Improve error handling and type safety in database operations.
The function needs several improvements:
- Add error handling for database operations
- Make the function async to properly handle database operations
- Fix the assignment in expression
- Replace type assertion with proper initialization
Apply these improvements:
- function setCharKey(cKey: CharacterKey | '') {
+ async function setCharKey(cKey: CharacterKey | '') {
// Remove teammate
if (cKey === '') {
+ try {
database.teams.set(
teamId,
- (team) => (team.teamMetadata[teamMetadataIndex] = undefined)
+ (team) => {
+ team.teamMetadata[teamMetadataIndex] = undefined;
+ return team;
+ }
)
+ } catch (error) {
+ console.error('Failed to remove teammate:', error);
+ // Consider adding user notification
+ }
return
}
// Create char if needed
+ try {
- database.chars.getOrCreate(cKey)
+ await database.chars.getOrCreate(cKey)
+ } catch (error) {
+ console.error('Failed to create character:', error);
+ return;
+ }
// Check if character is already in the team.
const existingIndex = team.teamMetadata.findIndex(
(teammateDatum) => teammateDatum?.characterKey === cKey
)
// If not exists, insert it
if (existingIndex === -1) {
// If none, create it
+ try {
database.teams.set(teamId, (team) => {
// Let the validator handle default properties for everything else
- team.teamMetadata[teamMetadataIndex] = {
+ const newMetadata: TeammateDatum = {
characterKey: cKey,
- } as TeammateDatum
+ };
+ team.teamMetadata[teamMetadataIndex] = newMetadata;
+ return team;
})
+ } catch (error) {
+ console.error('Failed to update team:', error);
+ }
}
// If exists already, swap positions
else {
if (existingIndex === teamMetadataIndex) return
const existingMeta = team.teamMetadata[existingIndex]
const destMeta = team.teamMetadata[teamMetadataIndex]
+ try {
database.teams.set(teamId, (team) => {
team.teamMetadata[teamMetadataIndex] = existingMeta
team.teamMetadata[existingIndex] = destMeta
+ return team;
})
+ } catch (error) {
+ console.error('Failed to swap characters:', error);
+ }
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function setCharKey(cKey: CharacterKey | '') { | |
| // Remove teammate | |
| if (cKey === '') { | |
| database.teams.set( | |
| teamId, | |
| (team) => (team.teamMetadata[teamMetadataIndex] = undefined) | |
| ) | |
| return | |
| } | |
| // Create char if needed | |
| database.chars.getOrCreate(cKey) | |
| // Check if character is already in the team. | |
| const existingIndex = team.teamMetadata.findIndex( | |
| (teammateDatum) => teammateDatum?.characterKey === cKey | |
| ) | |
| // If not exists, insert it | |
| if (existingIndex === -1) { | |
| // If none, create it | |
| database.teams.set(teamId, (team) => { | |
| // Let the validator handle default properties for everything else | |
| team.teamMetadata[teamMetadataIndex] = { | |
| characterKey: cKey, | |
| } as TeammateDatum | |
| }) | |
| } | |
| // If exists already, swap positions | |
| else { | |
| if (existingIndex === teamMetadataIndex) return | |
| const existingMeta = team.teamMetadata[existingIndex] | |
| const destMeta = team.teamMetadata[teamMetadataIndex] | |
| database.teams.set(teamId, (team) => { | |
| team.teamMetadata[teamMetadataIndex] = existingMeta | |
| team.teamMetadata[existingIndex] = destMeta | |
| }) | |
| } | |
| } | |
| async function setCharKey(cKey: CharacterKey | '') { | |
| // Remove teammate | |
| if (cKey === '') { | |
| try { | |
| database.teams.set( | |
| teamId, | |
| (team) => { | |
| team.teamMetadata[teamMetadataIndex] = undefined; | |
| return team; | |
| } | |
| ) | |
| } catch (error) { | |
| console.error('Failed to remove teammate:', error); | |
| // Consider adding user notification | |
| } | |
| return | |
| } | |
| // Create char if needed | |
| try { | |
| await database.chars.getOrCreate(cKey) | |
| } catch (error) { | |
| console.error('Failed to create character:', error); | |
| return; | |
| } | |
| // Check if character is already in the team. | |
| const existingIndex = team.teamMetadata.findIndex( | |
| (teammateDatum) => teammateDatum?.characterKey === cKey | |
| ) | |
| // If not exists, insert it | |
| if (existingIndex === -1) { | |
| // If none, create it | |
| try { | |
| database.teams.set(teamId, (team) => { | |
| // Let the validator handle default properties for everything else | |
| const newMetadata: TeammateDatum = { | |
| characterKey: cKey, | |
| }; | |
| team.teamMetadata[teamMetadataIndex] = newMetadata; | |
| return team; | |
| }) | |
| } catch (error) { | |
| console.error('Failed to update team:', error); | |
| } | |
| } | |
| // If exists already, swap positions | |
| else { | |
| if (existingIndex === teamMetadataIndex) return | |
| const existingMeta = team.teamMetadata[existingIndex] | |
| const destMeta = team.teamMetadata[teamMetadataIndex] | |
| try { | |
| database.teams.set(teamId, (team) => { | |
| team.teamMetadata[teamMetadataIndex] = existingMeta | |
| team.teamMetadata[existingIndex] = destMeta | |
| return team; | |
| }) | |
| } catch (error) { | |
| console.error('Failed to swap characters:', error); | |
| } | |
| } | |
| } |
🧰 Tools
🪛 Biome
[error] 39-39: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
| <CardThemed | ||
| sx={{ | ||
| overflow: 'visible', | ||
| top: 0, | ||
| position: 'sticky', | ||
| zIndex: 100, | ||
| }} | ||
| > | ||
| <TeamCharacterSelector teamId={teamId} charKey={characterKey} /> | ||
| </CardThemed> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding ARIA attributes for sticky header.
While the sticky header implementation is correct, consider adding ARIA attributes to improve accessibility:
<CardThemed
sx={{
overflow: 'visible',
top: 0,
position: 'sticky',
zIndex: 100,
}}
+ role="banner"
+ aria-label="Team character selector"
>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <CardThemed | |
| sx={{ | |
| overflow: 'visible', | |
| top: 0, | |
| position: 'sticky', | |
| zIndex: 100, | |
| }} | |
| > | |
| <TeamCharacterSelector teamId={teamId} charKey={characterKey} /> | |
| </CardThemed> | |
| <CardThemed | |
| sx={{ | |
| overflow: 'visible', | |
| top: 0, | |
| position: 'sticky', | |
| zIndex: 100, | |
| }} | |
| role="banner" | |
| aria-label="Team character selector" | |
| > | |
| <TeamCharacterSelector teamId={teamId} charKey={characterKey} /> | |
| </CardThemed> |
| const createOptConfig = useCallback(() => { | ||
| if (optConfigId) return | ||
|
|
||
| database.teams.set(teamId, (team) => { | ||
| const meta = team.teamMetadata.find( | ||
| (meta) => meta?.characterKey === teammateDatum.characterKey | ||
| ) | ||
| if (meta) { | ||
| const newOptConfigId = database.optConfigs.new() | ||
| meta.optConfigId = newOptConfigId | ||
| } | ||
| }) | ||
| }, [teamId, teammateDatum.characterKey, database, optConfigId]) |
There was a problem hiding this comment.
Add error handling and loading state to createOptConfig
The configuration creation function needs better error handling and loading state management to prevent potential race conditions and improve user feedback.
Consider applying this improvement:
const createOptConfig = useCallback(() => {
if (optConfigId) return
+ const [isLoading, setIsLoading] = useState(false)
+ if (isLoading) return
try {
+ setIsLoading(true)
database.teams.set(teamId, (team) => {
+ // Prevent race condition by re-checking metadata
+ if (team.teamMetadata.find(
+ meta => meta?.characterKey === teammateDatum.characterKey
+ )?.optConfigId) {
+ return team // Config already created
+ }
const meta = team.teamMetadata.find(
(meta) => meta?.characterKey === teammateDatum.characterKey
)
if (meta) {
const newOptConfigId = database.optConfigs.new()
meta.optConfigId = newOptConfigId
}
+ return team
})
} catch (error) {
+ console.error('Failed to create optimization config:', error)
+ // Consider showing an error notification to the user
} finally {
+ setIsLoading(false)
}
}, [teamId, teammateDatum.characterKey, database, optConfigId])Committable suggestion was skipped due to low confidence.
| setConditional( | ||
| teamId: string, | ||
| sheet: Sheet, | ||
| src: Member | 'all', | ||
| dst: Member | 'all', | ||
| condKey: string, | ||
| condValue: number, | ||
| frameIndex: number | ||
| ) { | ||
| this.set(teamId, (team) => { | ||
| const condIndex = team.conditionals.findIndex( | ||
| (c) => c.src === src && c.dst === dst && c.condKey === condKey | ||
| ) | ||
| if (frameIndex > team.frames.length) return | ||
| if (condIndex === -1) { | ||
| const condValues = new Array(team.frames.length).fill(0) | ||
| condValues[frameIndex] = condValue | ||
| team.conditionals.push({ | ||
| sheet, | ||
| src, | ||
| dst, | ||
| condKey, | ||
| condValues, | ||
| }) | ||
| } else { | ||
| team.conditionals[condIndex].condValues[frameIndex] = condValue | ||
| } | ||
| }) | ||
| } | ||
| /** | ||
| * | ||
| * @param teamId | ||
| * @param tag | ||
| * @param value number or null, null to delete | ||
| * @param frameIndex | ||
| */ | ||
| setBonusStat( | ||
| teamId: string, | ||
| tag: Tag, | ||
| value: number | null, | ||
| frameIndex: number | ||
| ) { | ||
| this.set(teamId, (team) => { | ||
| if (frameIndex > team.frames.length) return | ||
| const statIndex = team.bonusStats.findIndex((s) => | ||
| shallowCompareObj(s.tag, tag) | ||
| ) | ||
| if (statIndex === -1 && value !== null) { | ||
| const values = new Array(team.frames.length).fill(0) | ||
| values[frameIndex] = value | ||
| team.bonusStats.push({ tag, values }) | ||
| } else if (value === null && statIndex > -1) { | ||
| team.bonusStats.splice(statIndex, 1) | ||
| } else if (value !== null && statIndex > -1) { | ||
| team.bonusStats[statIndex].values[frameIndex] = value | ||
| } | ||
| }) | ||
| } | ||
| /** | ||
| * | ||
| * @param teamId | ||
| * @param tag | ||
| * @param value number or null, null to delete | ||
| * @param isMax | ||
| * @param frameIndex | ||
| */ | ||
| setStatConstraint( | ||
| teamId: string, | ||
| tag: Tag, | ||
| value: number | null, | ||
| isMax: boolean, | ||
| frameIndex: number | ||
| ) { | ||
| this.set(teamId, (team) => { | ||
| if (frameIndex > team.frames.length) return | ||
| const statIndex = team.statConstraints.findIndex((s) => | ||
| shallowCompareObj(s.tag, tag) | ||
| ) | ||
| if (statIndex === -1 && value !== null) { | ||
| const values = new Array(team.frames.length).fill(0) | ||
| values[frameIndex] = value | ||
| const isMaxs = new Array(team.frames.length).fill(false) | ||
| isMaxs[frameIndex] = isMax | ||
| team.statConstraints.push({ tag, values, isMaxs }) | ||
| } else if (value === null && statIndex > -1) { | ||
| team.statConstraints.splice(statIndex, 1) | ||
| } else if (value !== null && statIndex > -1) { | ||
| team.statConstraints[statIndex].values[frameIndex] = value | ||
| team.statConstraints[statIndex].isMaxs[frameIndex] = isMax | ||
| } | ||
| }) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Reduce code duplication in state management methods.
The setConditional, setBonusStat, and setStatConstraint methods share similar patterns and validations.
Consider extracting common logic:
private validateFrameIndex(team: Team, frameIndex: number): boolean {
return frameIndex <= team.frames.length
}
private updateArrayValue<T extends { values: number[] }>(
array: T[],
index: number,
value: number,
defaultValues: number[]
): void {
if (index === -1) {
array.push({ ...defaultValues })
} else {
array[index].values[frameIndex] = value
}
}This would simplify the implementation of each method and make the code more maintainable.
| override remove(teamId: string, notify?: boolean): Team | undefined { | ||
| const rem = super.remove(teamId, notify) | ||
| if (!rem) return | ||
| rem.teamMetadata.forEach((teammateDatum) => { | ||
| teammateDatum?.optConfigId && | ||
| this.database.optConfigs.remove(teammateDatum?.optConfigId) | ||
| }) | ||
| return rem | ||
| } |
There was a problem hiding this comment.
Add error handling to the remove method.
The remove method performs database operations that could fail silently.
Add try-catch blocks for proper error handling:
override remove(teamId: string, notify?: boolean): Team | undefined {
+ try {
const rem = super.remove(teamId, notify)
if (!rem) return
rem.teamMetadata.forEach((teammateDatum) => {
teammateDatum?.optConfigId &&
this.database.optConfigs.remove(teammateDatum?.optConfigId)
})
return rem
+ } catch (error) {
+ console.error(`Failed to remove team ${teamId}:`, error)
+ throw new Error(`Failed to remove team: ${error.message}`)
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| override remove(teamId: string, notify?: boolean): Team | undefined { | |
| const rem = super.remove(teamId, notify) | |
| if (!rem) return | |
| rem.teamMetadata.forEach((teammateDatum) => { | |
| teammateDatum?.optConfigId && | |
| this.database.optConfigs.remove(teammateDatum?.optConfigId) | |
| }) | |
| return rem | |
| } | |
| override remove(teamId: string, notify?: boolean): Team | undefined { | |
| try { | |
| const rem = super.remove(teamId, notify) | |
| if (!rem) return | |
| rem.teamMetadata.forEach((teammateDatum) => { | |
| teammateDatum?.optConfigId && | |
| this.database.optConfigs.remove(teammateDatum?.optConfigId) | |
| }) | |
| return rem | |
| } catch (error) { | |
| console.error(`Failed to remove team ${teamId}:`, error) | |
| throw new Error(`Failed to remove team: ${error.message}`) | |
| } | |
| } |
| //TODO: dup + i/o | ||
| // duplicate(teamId: string): string { | ||
| // const teamRaw = this.database.teams.get(teamId) | ||
| // if (!teamRaw) return '' | ||
| // const team = deepClone(teamRaw) | ||
| // team.name = `${team.name} (duplicated)` | ||
| // return this.new(team) | ||
| // } | ||
| // export(teamId: string): object { | ||
| // const team = this.database.teams.get(teamId) | ||
| // if (!team) return {} | ||
| // const { loadoutMetadata, ...rest } = team | ||
| // return { | ||
| // ...rest, | ||
| // loadoutMetadata: loadoutMetadata.map( | ||
| // (loadoutMetadatum) => | ||
| // loadoutMetadatum?.loadoutId && | ||
| // this.database.loadouts.export(loadoutMetadatum?.loadoutId) | ||
| // ), | ||
| // } | ||
| // } | ||
| // import(data: object): string { | ||
| // const { teamMetadata: loadoutMetadata, ...rest } = data as Team & { | ||
| // loadoutMetadata: object[] | ||
| // } | ||
| // const id = this.generateKey() | ||
| // if ( | ||
| // !this.set(id, { | ||
| // ...rest, | ||
| // name: `${rest.name ?? ''} (Imported)`, | ||
| // teamMetadata: loadoutMetadata.map( | ||
| // (obj) => | ||
| // obj && { | ||
| // loadoutId: this.database.loadouts.import(obj), | ||
| // } | ||
| // ), | ||
| // } as Team) | ||
| // ) | ||
| // return '' | ||
| // return id | ||
| // } |
There was a problem hiding this comment.
💡 Codebase verification
Uncomment and implement the duplicate, export, and import operations
The duplicate operation is actively being used in the codebase (specifically in libs/gi/page-team/src/TeamSetting/index.tsx), and the GI implementation provides a working reference. The commented-out code should be uncommented and implemented similarly to the GI version.
- Uncomment and implement the
duplicate()method following the existing pattern inlibs/gi/db/src/Database/DataManagers/TeamDataManager.ts - Consider keeping the
export()andimport()methods as they provide data portability, though they aren't currently used - Update the method implementations to match the current data structure and requirements
🔗 Analysis chain
Address commented-out code for duplicate and I/O operations.
The TODO comment and commented-out code for duplicate, export, and import operations need to be addressed.
Let's verify if these operations are still needed:
Would you like help implementing these methods or should they be removed if no longer needed?
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if there are any references to these operations in the codebase
echo "Searching for references to duplicate/export/import operations..."
rg -l "duplicate\(.*teamId" || rg -l "export\(.*teamId" || rg -l "import\(.*data.*Team"
Length of output: 326
Script:
#!/bin/bash
# Let's check the actual usage of these methods in the files found
echo "Checking TeamSetting/index.tsx usage:"
rg -A 5 "duplicate\(.*teamId" "libs/gi/page-team/src/TeamSetting/index.tsx"
echo -e "\nChecking GI TeamDataManager implementation:"
rg -A 5 "duplicate\(.*teamId" "libs/gi/db/src/Database/DataManagers/TeamDataManager.ts"
echo -e "\nLet's also check for export/import usage:"
rg -A 5 "export\(.*teamId|import\(.*data.*Team" "libs/gi/page-team/src/TeamSetting/index.tsx" "libs/gi/db/src/Database/DataManagers/TeamDataManager.ts"
Length of output: 1066
| onDelete={() => | ||
| database.teams.setBonusStat(teamId, tag, 0, presetIndex) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove bonus stat entry instead of setting value to zero on delete
In the onDelete handler, you're setting the bonus stat value to zero:
onDelete={() =>
database.teams.setBonusStat(teamId, tag, 0, presetIndex)
}Setting the value to zero may leave unnecessary entries in the database, which could clutter data and affect performance over time. It would be more efficient to remove the bonus stat entry entirely when it's deleted.
Apply this diff to remove the bonus stat:
- onDelete={() =>
- database.teams.setBonusStat(teamId, tag, 0, presetIndex)
- }
+ onDelete={() =>
+ database.teams.removeBonusStat(teamId, tag, presetIndex)
+ }Ensure that the removeBonusStat method is implemented in your database.teams API to handle the deletion properly.
| {bonusStats.map(({ tag, values }, i) => ( | ||
| <BonusStatDisplay | ||
| key={JSON.stringify(tag) + i} | ||
| tag={tag} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using a more stable key for BonusStatDisplay components
Using JSON.stringify(tag) + i as a key can lead to performance issues due to unnecessary re-renders. Since tag is an object, the stringification can result in different outputs if property order changes or if there are nested objects, causing React to remount components unnecessarily.
Consider using a stable and unique identifier for the key. If tag.q combined with presetIndex is unique, you can use that to create a more consistent key.
Apply this diff to use a more stable key:
- key={JSON.stringify(tag) + i}
+ key={`${tag.q}_${presetIndex}_${i}`}Committable suggestion was skipped due to low confidence.
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (6)
libs/sr/page-team/src/Optimize/BuildDisplay.tsx (1)
Line range hint 1-28: Component implementation remains robust and type-safe.
The BuildDisplay component maintains its clean implementation with proper TypeScript types and a clear separation of concerns. The grid-based layout provides a consistent display of relic cards.
Consider adding a loading state or error boundary to handle cases where the useEquippedRelics hook fails to fetch data, as this is now dependent on a potentially remote data source.
libs/sr/page-team/src/Optimize/OptConfigWrapper.tsx (1)
1-5: Add JSDoc documentation for the new context system.
Since this is part of a significant UI redesign moving from loadout-based to team-based structures, it would be valuable to add comprehensive documentation explaining:
- The purpose and role of OptConfigContext in the new architecture
- How it relates to the team-based structure
- Example usage in child components
Add documentation like this:
/**
* Context for managing optimization configurations in the new team-based UI structure.
* This context provides access to optimization settings that can be shared across
* different characters within a team combo.
*
* @example
* function ChildComponent() {
* const { optConfig } = useOptConfig();
* // Use optConfig data
* }
*/libs/sr/page-team/src/Optimize/OptimizationTargetSelector.tsx (1)
Line range hint 1-48: Consider architectural improvements for the new team-based UI.
Given the PR's objective to move from loadout-based to team-based structures, consider these architectural improvements:
-
The component could benefit from memoization since it's likely to be re-rendered frequently in the new UI structure:
- Wrap with
React.memo - Use
useCallbackfor thesetOptTargethandler - Consider caching the formula list results
- Wrap with
-
The component might need to integrate with the new team context for better cohesion with the redesigned UI.
Example implementation:
import { memo, useCallback } from 'react'
export const OptimizationTargetSelector = memo(function OptimizationTargetSelector({
optTarget,
setOptTarget,
buttonProps = {},
}: {
optTarget?: Tag
setOptTarget: (o: Tag) => void
buttonProps?: Omit<DropdownButtonProps, 'title' | 'children'>
}) {
const { t } = useTranslation('optimize')
const calc = useSrCalcContext()
const handleTargetSelect = useCallback((tag: Tag) => {
setOptTarget(tag)
}, [setOptTarget])
// Rest of the component implementation...
})libs/sr/page-team/src/Optimize/StatFilterCard.tsx (2)
7-7: Consider adding type annotation for context destructuring.
The context usage looks good and aligns with the team-based management approach. Consider adding a type annotation for better type safety and code readability.
const {
optConfigId,
optConfig: { statFilters },
- } = useContext(OptConfigContext)
+ } = useContext(OptConfigContext) as { optConfigId: string; optConfig: { statFilters: StatFilters } }Also applies to: 14-16
Line range hint 20-24: Consider adding error handling for database operations.
While the implementation is solid, the database operation in setStatFilters could benefit from error handling to gracefully handle potential failures.
const setStatFilters = useCallback(
- (statFilters: StatFilters) =>
- database.optConfigs.set(optConfigId, { statFilters }),
+ (statFilters: StatFilters) => {
+ try {
+ database.optConfigs.set(optConfigId, { statFilters })
+ } catch (error) {
+ console.error('Failed to update stat filters:', error)
+ // Consider adding a user-friendly error notification here
+ }
+ },
[database, optConfigId]
)libs/sr/page-team/src/TeammateDisplay.tsx (1)
54-54: Consider adding ARIA labels for better accessibility
The ComboEditor integration looks good, but consider enhancing accessibility.
Consider wrapping the ComboEditor with appropriate ARIA labels:
-<ComboEditor />
+<div role="region" aria-label="Team Combo Editor">
+ <ComboEditor />
+</div>📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
- libs/sr/page-team/src/ComboEditor.tsx (1 hunks)
- libs/sr/page-team/src/Optimize/BuildDisplay.tsx (1 hunks)
- libs/sr/page-team/src/Optimize/OptConfigWrapper.tsx (1 hunks)
- libs/sr/page-team/src/Optimize/OptimizationTargetSelector.tsx (1 hunks)
- libs/sr/page-team/src/Optimize/StatFilterCard.tsx (1 hunks)
- libs/sr/page-team/src/Optimize/index.tsx (6 hunks)
- libs/sr/page-team/src/TeammateDisplay.tsx (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- libs/sr/page-team/src/ComboEditor.tsx
🧰 Additional context used
🪛 Biome
libs/sr/page-team/src/Optimize/index.tsx
[error] 120-120: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
🔇 Additional comments (9)
libs/sr/page-team/src/Optimize/BuildDisplay.tsx (1)
3-4: LGTM! Import restructuring aligns with centralization goals.
The migration of imports from local paths to centralized packages (@genshin-optimizer/sr/db-ui and @genshin-optimizer/sr/ui) aligns well with the PR's objective of consolidating the UI components.
Let's verify that these components are properly exported from the new locations:
✅ Verification successful
Imports are correctly structured and exported from the new locations
The verification confirms:
useEquippedRelicshook is properly exported from@genshin-optimizer/sr/db-ui- Both
EmptyRelicCardandRelicCardcomponents are correctly exported from@genshin-optimizer/sr/ui - The remaining imports from paths containing "Hook" or "Relic" are unrelated to these components and belong to different contexts (stats, formula, assets)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the availability of the imported components in their new locations
# Check for the exports of the required components
rg -A 5 "export.*useEquippedRelics"
rg -A 5 "export.*EmptyRelicCard"
rg -A 5 "export.*RelicCard"
# Verify no other files still import from the old locations
rg "from ['\"].*/(Hook|Relic)['\"]"
Length of output: 3017
libs/sr/page-team/src/Optimize/OptimizationTargetSelector.tsx (2)
43-43: LGTM! Good addition of the key prop.
Adding the key prop to the SqBadge component follows React best practices and prevents potential reconciliation issues during re-renders.
Line range hint 1-48: Verify integration with the new combo system.
Since this component is part of the optimization UI, we should verify its integration with the new combo system mentioned in the PR objectives.
✅ Verification successful
Based on the code inspection, I can now generate the final response:
The OptimizationTargetSelector is properly integrated with the new combo system
The component is correctly used within the ComboEditor for managing optimization targets in the team frames system:
- It's used to add new frames to the team's combo sequence
- Each frame in the combo can have its own optimization target
- The component maintains proper state management through the team database context
The integration is complete and working as expected with no issues found.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for references to this component and verify its usage in the new team/combo context
echo "Searching for OptimizationTargetSelector usage..."
rg -l "OptimizationTargetSelector"
# Look for any remaining loadout-related code that might need updating
echo "Checking for old loadout-related code..."
rg -l "loadout|LoadoutContext"
# Check if this component is properly integrated with the new team context
echo "Verifying team context integration..."
rg -l "TeamContext.*OptimizationTargetSelector"
Length of output: 4846
Script:
#!/bin/bash
# Let's examine the ComboEditor.tsx since it appears to be the integration point
echo "Checking ComboEditor.tsx for OptimizationTargetSelector usage..."
cat "libs/sr/page-team/src/ComboEditor.tsx"
# Let's also check the OptimizationTargetEditorList.tsx to understand the integration
echo "Checking OptimizationTargetEditorList.tsx..."
cat "libs/sr/page-team/src/Optimize/OptimizationTargetEditorList.tsx"
# Check for any combo-related context or hooks
echo "Searching for combo-related context or hooks..."
rg -l "ComboContext|useCombo"
Length of output: 8125
libs/sr/page-team/src/TeammateDisplay.tsx (3)
3-3: LGTM: Import changes align with new architecture
The updated imports correctly reflect the transition from loadout-based to team-based structure, maintaining good module organization.
Also applies to: 22-26
Line range hint 83-93: Consider removing or protecting debug information
The component includes detailed debug information in accordion sections. This might expose sensitive information in production.
#!/bin/bash
# Check if there's a development flag or environment variable controlling debug output
rg -l "process\.env\.NODE_ENV|isDevelopment|DEBUG" Consider:
- Moving debug information behind a development flag
- Creating a separate debug component that's only included in development builds
- Using proper logging infrastructure instead of UI display
Also applies to: 120-130
28-32: Verify team context usage completeness
The switch to useTeamContext looks good, but let's ensure all necessary team context properties are properly handled.
✅ Verification successful
Based on the search results, I can now generate a final response. The code shows the complete TeamContextObj type and all its usages across the codebase.
Team context usage is complete and correct
The useTeamContext() hook in TeammateDisplay.tsx correctly destructures teammateDatum.characterKey from the team context. The context type definition in TeamContext.ts shows this is a valid property, and examining other usages confirms this pattern is consistent throughout the codebase. The switch from useLoadoutContext maintains all necessary team-related data access.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for all properties accessed from TeamContext to ensure complete coverage
ast-grep --pattern 'const { $$$props } = useTeamContext()'
Length of output: 422
Script:
#!/bin/bash
# Let's try a different pattern to find useTeamContext usage
rg "useTeamContext" -A 3 -B 3
# Also search for TeamContext provider to understand available properties
rg "TeamContext|TeamProvider" -A 3 -B 3
Length of output: 21216
libs/sr/page-team/src/Optimize/index.tsx (3)
36-70: LGTM: The 'Optimize' component is well-structured
The refactored Optimize component correctly utilizes context and manages the rendering logic based on optConfigId. The conditional rendering between the optimization interface and the prompt card is clear and maintains good user experience.
85-85: LGTM: Correct use of OptConfigContext
The optConfig retrieved from OptConfigContext ensures that the optimization configuration is accessible within OptimizeWrapper, aligning with the new context-based architecture.
Line range hint 165-187: LGTM: Optimization UI components are effectively integrated
The UI components for the optimization process, including the WorkerSelector, StatFilterCard, and ProgressIndicator, are well-integrated and enhance user interaction. The use of MUI components maintains consistency with the overall application design.
| export const OptConfigContext = createContext({ | ||
| optConfigId: '', | ||
| optConfig: {} as OptConfig, | ||
| }) |
There was a problem hiding this comment.
Improve type safety and context consumption patterns.
The context creation could be enhanced in several ways:
- The default value using an empty object cast is unsafe
- Missing a custom hook for consuming the context safely
Consider applying these improvements:
-export const OptConfigContext = createContext({
- optConfigId: '',
- optConfig: {} as OptConfig,
-})
+type OptConfigContextType = {
+ optConfigId: string;
+ optConfig: OptConfig;
+}
+
+export const OptConfigContext = createContext<OptConfigContextType | null>(null);
+
+export function useOptConfig(): OptConfigContextType {
+ const context = useContext(OptConfigContext);
+ if (context === null) {
+ throw new Error('useOptConfig must be used within an OptConfigProvider');
+ }
+ return context;
+}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export const OptConfigContext = createContext({ | |
| optConfigId: '', | |
| optConfig: {} as OptConfig, | |
| }) | |
| type OptConfigContextType = { | |
| optConfigId: string; | |
| optConfig: OptConfig; | |
| } | |
| export const OptConfigContext = createContext<OptConfigContextType | null>(null); | |
| export function useOptConfig(): OptConfigContextType { | |
| const context = useContext(OptConfigContext); | |
| if (context === null) { | |
| throw new Error('useOptConfig must be used within an OptConfigProvider'); | |
| } | |
| return context; | |
| } |
| export default function OptConfigProvider({ | ||
| optConfigId, | ||
| children, | ||
| }: { | ||
| optConfigId: string | ||
| children: React.ReactNode | ||
| }) { | ||
| const { database } = useDatabaseContext() | ||
| const optConfig = useDataManagerBase(database.optConfigs, optConfigId)! | ||
| const providerValue = useMemo( | ||
| () => ({ optConfigId, optConfig }), | ||
| [optConfigId, optConfig] | ||
| ) | ||
| return ( | ||
| <OptConfigContext.Provider value={providerValue}> | ||
| {children} | ||
| </OptConfigContext.Provider> | ||
| ) | ||
| } |
There was a problem hiding this comment.
Add error handling and validation.
The provider implementation could be more robust:
- The non-null assertion (
!) on line 18 could hide potential runtime errors - Missing error handling for invalid optConfigId
- No loading state handling
Consider applying these improvements:
export default function OptConfigProvider({
optConfigId,
children,
}: {
optConfigId: string
children: React.ReactNode
}) {
const { database } = useDatabaseContext()
- const optConfig = useDataManagerBase(database.optConfigs, optConfigId)!
+ const optConfig = useDataManagerBase(database.optConfigs, optConfigId)
+
+ if (!optConfig) {
+ throw new Error(`OptConfig with id ${optConfigId} not found`);
+ }
+
const providerValue = useMemo(
() => ({ optConfigId, optConfig }),
[optConfigId, optConfig]
)
+
return (
<OptConfigContext.Provider value={providerValue}>
{children}
</OptConfigContext.Provider>
)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export default function OptConfigProvider({ | |
| optConfigId, | |
| children, | |
| }: { | |
| optConfigId: string | |
| children: React.ReactNode | |
| }) { | |
| const { database } = useDatabaseContext() | |
| const optConfig = useDataManagerBase(database.optConfigs, optConfigId)! | |
| const providerValue = useMemo( | |
| () => ({ optConfigId, optConfig }), | |
| [optConfigId, optConfig] | |
| ) | |
| return ( | |
| <OptConfigContext.Provider value={providerValue}> | |
| {children} | |
| </OptConfigContext.Provider> | |
| ) | |
| } | |
| export default function OptConfigProvider({ | |
| optConfigId, | |
| children, | |
| }: { | |
| optConfigId: string | |
| children: React.ReactNode | |
| }) { | |
| const { database } = useDatabaseContext() | |
| const optConfig = useDataManagerBase(database.optConfigs, optConfigId) | |
| if (!optConfig) { | |
| throw new Error(`OptConfig with id ${optConfigId} not found`); | |
| } | |
| const providerValue = useMemo( | |
| () => ({ optConfigId, optConfig }), | |
| [optConfigId, optConfig] | |
| ) | |
| return ( | |
| <OptConfigContext.Provider value={providerValue}> | |
| {children} | |
| </OptConfigContext.Provider> | |
| ) | |
| } |
| import { CardThemed, InfoTooltip } from '@genshin-optimizer/common/ui' | ||
| import type { StatFilters } from '@genshin-optimizer/sr/db' | ||
| import { LoadoutContext, useDatabaseContext } from '@genshin-optimizer/sr/ui' | ||
| import { useDatabaseContext } from '@genshin-optimizer/sr/db-ui' |
There was a problem hiding this comment.
💡 Codebase verification
Import path change is correct but needs coordinated updates
The useDatabaseContext hook is properly implemented in the new location at libs/sr/db-ui/src/context/DatabaseContext.ts. However, there are multiple files still using the old import path, which need to be updated for consistency:
- Multiple files in
libs/sr/ui/*still use the old import - Several files in
libs/sr/page-teams/*andlibs/sr/page-team/*need updates apps/sr-frontend/src/app/Header.tsxalso needs to be updated
This change should be part of a coordinated update across all files to maintain consistency in the codebase.
🔗 Analysis chain
Verify the new import path structure.
The import change aligns with better separation of concerns, moving database-related UI functionality to a dedicated package.
Let's verify the package structure:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the new package structure and usage
# Test 1: Check if the new package exists
fd -t d "db-ui$" libs/sr
# Test 2: Check for other potential usages of the old import path
rg -l "@genshin-optimizer/sr/ui.*useDatabaseContext"
Length of output: 96
Script:
#!/bin/bash
# Let's verify the new import path is actually used across the codebase
# and the component exists in the new location
# Check if useDatabaseContext exists in the new location
rg -l "export.*useDatabaseContext" libs/sr/db-ui/
# Check all current imports of useDatabaseContext
rg -l "import.*useDatabaseContext.*from.*@genshin-optimizer"
# Check the actual implementation to ensure it's properly exported
ast-grep --pattern 'export function useDatabaseContext() { $$$ }'
Length of output: 1385
| @@ -89,23 +116,23 @@ export default function Optimize() { | |||
| useEffect(() => () => cancelToken.current(), []) | |||
|
|
|||
| const onOptimize = useCallback(async () => { | |||
| if (!optTarget || !calc) return | |||
| if (!calc) return | |||
| const cancelled = new Promise<void>((r) => (cancelToken.current = r)) | |||
There was a problem hiding this comment.
Avoid assignment within an expression for clarity
Assigning cancelToken.current within the Promise constructor can be confusing and may lead to unintended side effects. Consider separating the assignment from the expression for better readability and maintainability.
You can refactor the code as follows:
- const cancelled = new Promise<void>((r) => (cancelToken.current = r))
+ let resolveCancel!: () => void
+ const cancelled = new Promise<void>((resolve) => {
+ resolveCancel = resolve
+ })
+ cancelToken.current = resolveCancel📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const cancelled = new Promise<void>((r) => (cancelToken.current = r)) | |
| let resolveCancel!: () => void | |
| const cancelled = new Promise<void>((resolve) => { | |
| resolveCancel = resolve | |
| }) | |
| cancelToken.current = resolveCancel |
🧰 Tools
🪛 Biome
[error] 120-120: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
Describe your changes
Using the iterated design from the usecase/requirement study document
Create a new philosophy of design:
Some major overhaul for the UI of "team character view"
Updated the MVP/Parity Document with new design considerations
TODO:
page-combo, once UI is mostly done, removepage-team, and renamepage-combo->pageteam@deprecatedThings to do for future PRs
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
OptConfigProviderfor managing optimization configurations.shallowCompareObjfunction for object comparison.ComboEditorcomponent for editing team configurations.TeamSelectorsandTeammateSelectorcomponents for character selection.Bug Fixes
Refactor
TeamContextandPresetContext.Chores