Use Tag as db data instead of Read
#2517
Conversation
WalkthroughThis pull request implements significant changes across multiple files, primarily focusing on the transition from the Changes
Possibly related PRs
Suggested reviewers
Poem
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 (
|
frzyc
changed the title
Tag as db data instead of Read
|
[sr-frontend] [Wed Oct 23 23:07:41 UTC 2024] - Deployed 5483b04 to https://genshin-optimizer-prs.github.io/pr/2517/sr-frontend (Takes 3-5 minutes after this completes to be available) [sr-frontend] [Thu Oct 24 02:50:56 UTC 2024] - Deployed fe24393 to https://genshin-optimizer-prs.github.io/pr/2517/sr-frontend (Takes 3-5 minutes after this completes to be available) [Thu Oct 24 03:19:43 UTC 2024] - Deleted deployment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (3)
libs/sr/page-team/src/TeammateDisplay/Tabs/Optimize/OptimizationTargetEditorList.tsx (1)
82-85: Consider optimizing the key prop implementation.While the tag transition is implemented correctly, using
JSON.stringify(statFilter)in the key prop might impact performance for large objects. Consider using a more specific unique identifier.-key={i + JSON.stringify(statFilter)} +key={`${i}-${statFilter.tag.name || statFilter.tag.q || ''}`}libs/sr/db/src/Database/DataManagers/OptConfigDataManager.ts (2)
116-118: Consider adding descriptive error messages to validation.While the validation logic is correct, adding descriptive error messages would help debugging when validation fails. Consider wrapping the validation in a try-catch block with meaningful error messages.
Here's a suggested improvement:
statFilters = statFilters.filter((statFilter) => { const { tag, value, isMax, disabled } = statFilter as UnArray<StatFilters> - if (!validateTag(tag)) return false + try { + if (!validateTag(tag)) { + console.warn(`Invalid tag in statFilter: ${JSON.stringify(tag)}`) + return false + } + } catch (e) { + console.warn(`Error validating tag: ${e}`) + return false + } if (typeof value !== 'number') return false if (typeof isMax !== 'boolean') return false if (typeof disabled !== 'boolean') return false return true }) -if (optimizationTarget && !validateTag(optimizationTarget)) +if (optimizationTarget && !validateTag(optimizationTarget)) { + console.warn(`Invalid optimizationTarget: ${JSON.stringify(optimizationTarget)}`) optimizationTarget = undefined +}Also applies to: 142-142
Line range hint
276-276: Address TODO comment about planar relics.The TODO comment about removing 4-set exclusion for planar relics should be addressed or tracked in an issue.
Would you like me to create a GitHub issue to track this TODO?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
- libs/sr/db/src/Database/DataManagers/OptConfigDataManager.ts (5 hunks)
- libs/sr/page-team/src/TeammateDisplay/Tabs/Optimize/OptimizationTargetEditorList.tsx (6 hunks)
- libs/sr/page-team/src/TeammateDisplay/Tabs/Optimize/OptimizationTargetSelector.tsx (3 hunks)
- libs/sr/page-team/src/TeammateDisplay/Tabs/Optimize/index.tsx (3 hunks)
- libs/sr/solver/src/solver.ts (4 hunks)
🧰 Additional context used
🔇 Additional comments (14)
libs/sr/page-team/src/TeammateDisplay/Tabs/Optimize/OptimizationTargetSelector.tsx (2)
27-27: LGTM! Implementation changes are consistent.The component implementation correctly handles the type changes:
- Direct usage of
optTargetin OptimizationTargetDisplay- Event handler properly passes
read.tagto setOptTargetLet's verify the OptimizationTargetDisplay component's prop requirements:
#!/bin/bash # Description: Verify OptimizationTargetDisplay component's prop types # Search for OptimizationTargetDisplay component definition ast-grep --pattern 'export function OptimizationTargetDisplay({ $$ }: { tag: $_ }) {'Also applies to: 35-35
16-17: LGTM! Verify type compatibility across components.The props interface changes from
ReadtoTagalign with the PR objective. The type definitions are clean and well-structured.Let's verify that this type change is consistent across all components:
✅ Verification successful
Let me try a different approach to verify the type changes across components.
This should help us:
- Find all files that reference OptimizationTarget
- See the type definitions and usages in these files to verify consistency
Based on the search results, I can now generate a final response:
Type changes from
ReadtoTagare consistently implementedThe search results confirm that the type changes in
OptimizationTargetSelector.tsxare consistent with related components:
OptimizationTargetDisplay.tsxusesTagtype:export function OptimizationTargetDisplay({ tag }: { tag: Tag })OptimizationTargetEditorList.tsxalso usesTagtype in its callback:(tag: Tag, oldIndex?: number)- The components correctly pass and handle the
Tagtype in their JSX, as seen inOptimizationTargetSelector.tsx:<OptimizationTargetDisplay tag={optTarget} />🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any remaining Read type usage in optimization-related components # and ensure Tag type is used consistently # Search for any remaining Read type usage in optimization components rg -l "Read.*OptimizationTarget|OptimizationTarget.*Read" --type ts --type tsx # Search for Tag usage in optimization components to verify consistency rg -l "Tag.*OptimizationTarget|OptimizationTarget.*Tag" --type ts --type tsxLength of output: 181
Script:
#!/bin/bash # Search for files containing OptimizationTarget fd -e tsx -e ts | xargs rg -l "OptimizationTarget" # Then check the content of these files fd -e tsx -e ts | xargs rg "OptimizationTarget.*\{|type.*OptimizationTarget|interface.*OptimizationTarget" -A 2Length of output: 8380
libs/sr/solver/src/solver.ts (4)
5-5: LGTM: Import statement maintains functionality.The reordering of imports accommodates the new
Tagtype requirement without affecting functionality.
35-35: Verify constructor calls are updated to use Tag type.The constructor parameter type change is consistent with the property type change. Let's verify all constructor calls have been updated accordingly.
#!/bin/bash # Search for Solver constructor calls to verify they pass Tag type ast-grep --pattern 'new Solver($$$)'
117-120: Verify the Read transformation logic for Tag types.The changes look correct:
optTargetis now wrapped in aReadinstance with 'sum' argumentstatFiltersmapping uses thetagproperty consistentlyThe transformation logic maintains the same functionality while adapting to the new Tag type system.
#!/bin/bash # Search for similar Read transformations to verify consistency ast-grep --pattern 'new Read($_, "sum")'
24-24: Verify type compatibility of optTarget throughout the class.The type change from
ReadtoTaglooks correct, but let's verify all usages ofoptTargetare compatible with the new type.✅ Verification successful
Let me try a different approach to verify the usage of
optTargetin the codebase.This should help us see the class definition and all occurrences of
optTargetin the codebase, including its usage context.
Based on the search results, I can now provide a final response about the type change verification:
Type change from
ReadtoTagis compatible with all usagesThe code shows that
optTargetis:
- Received as
Tagtype in the constructor- Only used once in the
detachNodesmethod where it's wrapped innew Read(this.optTarget, 'sum')- This usage pattern shows the type change is intentional and correct, as
Tagis now the base type that gets wrapped in aReadwhen needed🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for all usages of optTarget to verify type compatibility ast-grep --pattern 'this.optTarget'Length of output: 42
Script:
#!/bin/bash # Search for the Solver class definition and its content to analyze optTarget usage ast-grep --pattern 'class Solver { $$$ }' # Also search for any direct string matches of optTarget rg "optTarget" -A 2 -B 2Length of output: 20103
libs/sr/page-team/src/TeammateDisplay/Tabs/Optimize/OptimizationTargetEditorList.tsx (3)
4-4: LGTM! Necessary imports added.The addition of the
Tagtype import supports the transition fromReadtoTag, and theuseCallbackimport follows React best practices for performance optimization.Also applies to: 17-17
141-143: LGTM! Consistent prop updates.The OptimizationTargetSelector props have been correctly updated to use the new tag type.
32-41: Verify Tag structure validation.The implementation correctly handles state updates using immutable patterns. However, we should verify that the Tag structure is properly validated before being used.
✅ Verification successful
Let me gather more information about the Tag type definition and its usage.
Let me check the Tag type definition from the imported module and its validation usage.
Let me check the Tag type definition and its usage in the component file.
Let me try a different approach to find the Tag type definition and its validation.
Based on the code analysis, I can now generate the final response:
Tag validation is properly handled through the component structure
The Tag type is safely used in the component through:
- Type-safe imports from '@genshin-optimizer/sr/formula'
- Proper state management with TypeScript type checking
- Structured component props validation
- Safe Tag manipulation using structuredClone for immutability
The implementation correctly handles Tag objects in a type-safe manner through TypeScript's static type checking, and the component's structure ensures Tags are properly validated at compile time. No additional runtime validation is necessary in this context since:
- Tags are received through the type system
- Tag mutations maintain type safety
- Tag usage is consistent with the formula module's expectations
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for Tag type definition and validation ast-grep --pattern 'type Tag = { $$$ }' # Search for any validation functions for Tag rg -A 5 'function.*validateTag|isValidTag'Length of output: 561
Script:
#!/bin/bash # Search for Tag type definition ast-grep --pattern 'type Tag = { $$$ }' # Search for Tag type imports and usage in the component file rg -l "import.*Tag" libs/sr/page-team/src/TeammateDisplay/Tabs/Optimize/OptimizationTargetEditorList.tsx # Search for Tag usage patterns in the component rg -A 5 "Tag" libs/sr/page-team/src/TeammateDisplay/Tabs/Optimize/OptimizationTargetEditorList.tsxLength of output: 1212
Script:
#!/bin/bash # Search for Tag type definition in the formula module rg -A 10 "type Tag = {" libs/sr/formula/ # Check if validateTag is imported and used in the component rg "import.*validateTag|validateTag" libs/sr/page-team/src/TeammateDisplay/Tabs/Optimize/OptimizationTargetEditorList.tsx # Check Tag usage in the formula module rg -A 5 "Tag" libs/sr/formula/src/Length of output: 137793
Script:
#!/bin/bash # Search for Tag type definition in formula module rg -A 10 "export type Tag = {" libs/sr/formula/src/ # Check Tag imports and usage in the component rg -A 10 "import.*Tag" libs/sr/page-team/src/TeammateDisplay/Tabs/Optimize/OptimizationTargetEditorList.tsx # Check Tag validation usage in the component rg "validateTag" libs/sr/page-team/src/TeammateDisplay/Tabs/Optimize/OptimizationTargetEditorList.tsxLength of output: 570
Script:
#!/bin/bash # Search for Tag type definition with ast-grep ast-grep --pattern 'export type Tag = { $$$ }' # Search for Tag type definition with rg using literal pattern rg -p "type Tag = \{" --type ts # Check the component's full content to understand Tag usage rg -l "Tag" libs/sr/page-team/src/TeammateDisplay/Tabs/Optimize/OptimizationTargetEditorList.tsx | xargs catLength of output: 5403
libs/sr/page-team/src/TeammateDisplay/Tabs/Optimize/index.tsx (2)
Line range hint
52-57: LGTM! Type change is properly implemented.The parameter type change in
setOptTargetis consistent with the transition toTagtype, and the database operation remains unchanged.
100-101: Verify Solver compatibility with the new tag property.The mapping has been updated to use
taginstead ofread. Let's verify that the Solver class has been updated to handle this change.✅ Verification successful
Based on the provided output, I can see that the Solver class is already using the
tagproperty in its implementation. Specifically, in thedetachNodesmethod at line 119:...this.statFilters.map(({ tag }) => new Read(tag, 'sum')),This shows that the Solver class is correctly destructuring and using the
tagproperty from the statFilters array, which matches the change in the TeammateDisplay component. Additionally, the constructor acceptsstatFiltersas a parameter and stores it directly without any transformation, indicating it expects the same structure as what's being passed.The Solver class is already compatible with the tag property.
The code change from
readtotagin TeammateDisplay is safe as the Solver class is already using thetagproperty in its implementation. No further changes are needed.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if Solver class has been updated to handle 'tag' property # Test: Look for the Solver class implementation and its filter handling ast-grep --pattern 'class Solver { $$$ constructor($$$) { $$$ } $$$ }'Length of output: 7143
libs/sr/db/src/Database/DataManagers/OptConfigDataManager.ts (3)
19-19: LGTM: Import of Tag type is correctly added.The import aligns with the PR objective of transitioning from Read to Tag type.
Line range hint
40-44: LGTM: StatFilter type properly updated to use Tag.The type definition maintains a clean structure while transitioning from Read to Tag type.
67-68: Verify if optimizationTarget should be optional.While the type change from Read to Tag is correct, consider if making
optimizationTargetrequired would improve type safety and prevent potential runtime issues.
libs/sr/page-team/src/TeammateDisplay/Tabs/Optimize/OptimizationTargetEditorList.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
libs/sr/db/src/Database/tagUtil.ts (2)
Line range hint
4-14: Consider enhancing the tag validation.The current validation could be improved in several ways:
- Replace type assertion with proper type guards
- Implement comprehensive validation as noted in the TODO
- Consider returning validation errors instead of just boolean for better error handling
Here's a suggested implementation:
- export function validateTag(tag: Tag) { - // TODO: more tag validation - if (typeof tag !== 'object') return false - - // TODO: REMOVE. this is a stopgap to invalid data since Read were stored where tags are now stored. - if ((tag as any).tag) return false - - if (tag.src && !allCharacterKeys.includes(tag.src)) return false - if (tag.dst && !allCharacterKeys.includes(tag.dst)) return false - return true - } + interface ValidationResult { + isValid: boolean; + errors: string[]; + } + + export function validateTag(tag: unknown): ValidationResult { + const errors: string[] = []; + + if (!tag || typeof tag !== 'object') { + return { isValid: false, errors: ['Tag must be an object'] }; + } + + const tagObj = tag as Record<string, unknown>; + + // Validate structure + if ('src' in tagObj && typeof tagObj.src === 'string') { + if (!allCharacterKeys.includes(tagObj.src)) { + errors.push(`Invalid source character: ${tagObj.src}`); + } + } + + if ('dst' in tagObj && typeof tagObj.dst === 'string') { + if (!allCharacterKeys.includes(tagObj.dst)) { + errors.push(`Invalid destination character: ${tagObj.dst}`); + } + } + + return { + isValid: errors.length === 0, + errors + }; + }
6-6: Consider adding JSDoc documentation.Adding documentation would help clarify the expected structure of valid
Tagobjects and the validation rules being applied.+ /** + * Validates a Tag object ensuring it meets the required structure and constraints. + * @param tag - The tag object to validate + * @returns True if the tag is valid, false otherwise + */ export function validateTag(tag: Tag) {
Describe your changes
Summary by CodeRabbit
New Features
Tagtype instead ofReadacross various components, enhancing data structure alignment.Bug Fixes
Documentation
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.