ZO adding basic opt targets to ZO, wrapping up MVPish #2603
Conversation
WalkthroughThis pull request introduces comprehensive enhancements to the statistical representation and optimization system across multiple libraries. The changes focus on expanding statistical keys, introducing new constants and types for elemental and anomaly damage, and modifying various components to support more detailed stat tracking and optimization. The modifications span several files, improving the flexibility and expressiveness of the statistical modeling system for the application. Changes
Suggested Labels
Possibly Related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🔇 Additional comments (2)
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
[zzz-frontend] [Tue Jan 21 01:20:33 UTC 2025] - Deployed 5d0eddf to https://genshin-optimizer-prs.github.io/pr/2603/zzz-frontend (Takes 3-5 minutes after this completes to be available) [zzz-frontend] [Tue Jan 21 02:06:14 UTC 2025] - Deployed 91ca9c9 to https://genshin-optimizer-prs.github.io/pr/2603/zzz-frontend (Takes 3-5 minutes after this completes to be available) [zzz-frontend] [Tue Jan 21 02:58:21 UTC 2025] - Deployed 91ca9c9 to https://genshin-optimizer-prs.github.io/pr/2603/zzz-frontend (Takes 3-5 minutes after this completes to be available) [zzz-frontend] [Tue Jan 21 06:47:25 UTC 2025] - Deployed 732e350 to https://genshin-optimizer-prs.github.io/pr/2603/zzz-frontend (Takes 3-5 minutes after this completes to be available) [zzz-frontend] [Tue Jan 21 06:49:15 UTC 2025] - Deployed 191b84b to https://genshin-optimizer-prs.github.io/pr/2603/zzz-frontend (Takes 3-5 minutes after this completes to be available) [zzz-frontend] [Tue Jan 21 07:03:28 UTC 2025] - Deployed 0a4c0b7 to https://genshin-optimizer-prs.github.io/pr/2603/zzz-frontend (Takes 3-5 minutes after this completes to be available) [zzz-frontend] [Wed Jan 22 04:35:06 UTC 2025] - Deployed 14a7841 to https://genshin-optimizer-prs.github.io/pr/2603/zzz-frontend (Takes 3-5 minutes after this completes to be available) [zzz-frontend] [Wed Jan 22 05:34:30 UTC 2025] - Deployed d630b78 to https://genshin-optimizer-prs.github.io/pr/2603/zzz-frontend (Takes 3-5 minutes after this completes to be available) [zzz-frontend] [Wed Jan 22 05:50:57 UTC 2025] - Deployed 1624f03 to https://genshin-optimizer-prs.github.io/pr/2603/zzz-frontend (Takes 3-5 minutes after this completes to be available) [zzz-frontend] [Wed Jan 22 05:52:22 UTC 2025] - Deployed 6ffe815 to https://genshin-optimizer-prs.github.io/pr/2603/zzz-frontend (Takes 3-5 minutes after this completes to be available) [zzz-frontend] [Fri Jan 24 02:38:31 UTC 2025] - Deployed 1c555b1 to https://genshin-optimizer-prs.github.io/pr/2603/zzz-frontend (Takes 3-5 minutes after this completes to be available) [zzz-frontend] [Fri Jan 24 02:54:41 UTC 2025] - Deployed c5b0ce3 to https://genshin-optimizer-prs.github.io/pr/2603/zzz-frontend (Takes 3-5 minutes after this completes to be available) [zzz-frontend] [Fri Jan 24 03:09:28 UTC 2025] - Deployed 990a643 to https://genshin-optimizer-prs.github.io/pr/2603/zzz-frontend (Takes 3-5 minutes after this completes to be available) [zzz-frontend] [Fri Jan 24 04:17:14 UTC 2025] - Deployed ea5ed42 to https://genshin-optimizer-prs.github.io/pr/2603/zzz-frontend (Takes 3-5 minutes after this completes to be available) [sr-frontend] [Fri Jan 24 04:17:50 UTC 2025] - Deployed ea5ed42 to https://genshin-optimizer-prs.github.io/pr/2603/sr-frontend (Takes 3-5 minutes after this completes to be available) [sr-frontend] [Fri Jan 24 04:25:10 UTC 2025] - Deployed 9e36cc1 to https://genshin-optimizer-prs.github.io/pr/2603/sr-frontend (Takes 3-5 minutes after this completes to be available) [zzz-frontend] [Fri Jan 24 04:25:17 UTC 2025] - Deployed 9e36cc1 to https://genshin-optimizer-prs.github.io/pr/2603/zzz-frontend (Takes 3-5 minutes after this completes to be available) [Fri Jan 24 06:04:54 UTC 2025] - Deleted deployment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (6)
libs/zzz/page-optimize/src/Optimize.tsx (1)
281-281: Fix typo in 'Electrical Damage' labelThere's a typo in the label for
electric_dmg_informulaKeyTextMap. It should be "Electrical Damage" instead of "Eletrical Damage".Apply this diff:
-electric_dmg_: 'Eletrical Damage', +electric_dmg_: 'Electrical Damage',libs/zzz/consts/src/common.ts (2)
19-26: Consider adding documentation for unconditional stats.While the naming is self-explanatory, it would be helpful to add comments explaining when these unconditional stats are used versus their conditional counterparts.
53-60: Consider grouping related anomaly types.The anomaly types could be organized better by grouping related effects (e.g., grouping 'burn' and 'corruption' if they are DoT effects).
export const allAnomalyDmgKeys = [ + // Damage over Time 'burn', - 'shock', 'corruption', + // Direct Damage + 'shock', 'shatter', 'assault', ] as constlibs/zzz/page-optimize/src/BaseStatCard.tsx (1)
37-54: Consider extracting input component.The
inputhelper function could be moved to a separate component for reusability.Consider creating a new component:
interface StatInputProps { statKey: string; value: number; onChange: (value: number) => void; } function StatInput({ statKey, value, onChange }: StatInputProps) { return ( <NumberInputLazy sx={{ flexGrow: 1 }} value={value} onChange={onChange} inputProps={{ sx: { textAlign: 'right', minWidth: '5em' } }} InputProps={{ startAdornment: statKeyTextMap[statKey] ?? statKey, endAdornment: getUnitStr(statKey), }} /> ); }libs/zzz/page-optimize/src/StatFilterCard.tsx (1)
70-73: Consider adding a comment explaining the 4p set filter.The filter logic for excluding 4-piece set constraints is correct, but adding a comment would help future maintainers understand why these constraints are excluded.
{Object.entries(constraints) - // filter out 4p set constraints + // Exclude 4-piece set constraints as they are handled separately in the set filter .filter(([key]) => !allDiscSetKeys.includes(key as DiscSetKey))libs/zzz/solver/src/childWorker.ts (1)
11-11: Consider making formulaKey readonly.Since formulaKey is only set during initialization and shouldn't change during worker execution, consider making it readonly for better type safety.
-let formulaKey: FormulaKey +readonly formulaKey: FormulaKeyAlso applies to: 18-18
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
libs/zzz/consts/src/common.ts(2 hunks)libs/zzz/page-optimize/src/BaseStatCard.tsx(3 hunks)libs/zzz/page-optimize/src/BuildsDisplay.tsx(2 hunks)libs/zzz/page-optimize/src/Optimize.tsx(7 hunks)libs/zzz/page-optimize/src/StatFilterCard.tsx(2 hunks)libs/zzz/page-optimize/src/index.tsx(1 hunks)libs/zzz/solver/src/calc.ts(1 hunks)libs/zzz/solver/src/childWorker.ts(3 hunks)libs/zzz/solver/src/parentWorker.ts(4 hunks)libs/zzz/solver/src/solver.ts(3 hunks)
🔇 Additional comments (16)
libs/zzz/page-optimize/src/Optimize.tsx (2)
Line range hint
70-88: EnsurediscsBySlotupdates when discs changeThe dependency on
database.discs.valuesin theuseMemofordiscsBySlotmight not trigger updates when the contents ofdatabase.discs.valueschange, as the reference may remain the same. Consider using a method to observe changes or ensure that the reference updates when discs change.
283-283: Verify 'frost_dmg_' key validityThe key
'frost_dmg_'informulaKeyTextMapmay not correspond to a defined damage type. Please verify if this key is valid. If not, it should be corrected or removed to prevent potential errors.libs/zzz/page-optimize/src/index.tsx (1)
12-13: LGTM!Adding default values for
charLvlandenemyDefimproves the initial state and enhances the user experience.libs/zzz/consts/src/common.ts (3)
5-12: LGTM! Well-documented enemy-related stats.The enemy-related stats are well-documented with clear comments explaining their purpose.
62-91: LGTM! Comprehensive text mappings.The text mappings are well-organized and provide clear, user-friendly labels for all stats.
33-41: Verify completeness of elemental types.The elemental types and their mappings look good. However, let's verify that all possible elements are covered.
Also applies to: 93-104
✅ Verification successful
Elemental types are complete and correctly defined
The six elemental types (electric, fire, ice, frost, physical, ether) are consistently used throughout the codebase for damage calculations, disc stats, and set bonuses. No additional elements are referenced or required.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any other elemental types mentioned in the codebase rg -i "type.*element" rg "(?i)element.*(?:type|kind|category)"Length of output: 66532
Script:
#!/bin/bash # Search for element-related files and types in zzz/ directory fd . libs/zzz/ -t f -x rg -i "element|type|damage" # Search for game data or config files that might contain element info fd . libs/zzz/ -t f -e json -e yaml -e toml -x rg -l "element|type|damage" # Search for usage of the specific elemental types rg -i "electric|fire|ice|frost|physical|ether" libs/zzz/Length of output: 17798
libs/zzz/solver/src/solver.ts (1)
20-20: Verify worker compatibility with formulaKey.The addition of
formulaKeylooks good, but we should verify that the worker is prepared to handle this new parameter.Also applies to: 26-26, 33-33, 73-73
✅ Verification successful
Worker implementation fully supports formulaKey parameter
The worker implementation properly handles the formulaKey parameter through all communication channels and calculation logic. The parameter is correctly passed from the solver to parent worker, and then to child workers where it's used in calculations.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if parentWorker.ts properly handles formulaKey rg -A 5 "type.*ParentCommandStart" rg -A 10 "case 'start'"Length of output: 3052
Script:
#!/bin/bash # Find ParentCommandStart interface and related code ast-grep --pattern 'interface ParentCommandStart { $$$ }' # Get complete start function implementation ast-grep --pattern 'async function start($_) { $$$ }' # Look for formulaKey usage in worker files rg -A 5 "formulaKey" libs/zzz/solver/src/Length of output: 21498
libs/zzz/page-optimize/src/BaseStatCard.tsx (2)
11-11: LGTM! Well-organized stat categories.The separation of stats into base keys and enemy keys improves code organization.
Also applies to: 24-28
58-73: LGTM! Clear section organization.The UI is well-organized with clear sections for different types of stats.
libs/zzz/page-optimize/src/BuildsDisplay.tsx (1)
50-50: LGTM! Improved stat display.The use of
statKeyTextMapimproves readability of stat names in the UI.libs/zzz/page-optimize/src/StatFilterCard.tsx (2)
8-12: LGTM! Clean import organization.The new imports for
DiscSetKeyandallDiscSetKeysare properly organized and typed.
74-100: LGTM! Well-structured constraint UI.The constraint UI implementation is clean with:
- Proper type safety with TypeScript
- Clear error handling
- Intuitive user controls for value input and constraint type toggle
libs/zzz/solver/src/parentWorker.ts (2)
7-7: LGTM! Clean type integration.The FormulaKey type is properly imported and integrated into the ParentCommandStart interface.
Also applies to: 22-22
85-85: Consider adding error handling for invalid formula keys.While the formulaKey integration is correct, consider adding validation to ensure the provided formulaKey is valid before initializing workers.
Also applies to: 171-172
libs/zzz/solver/src/childWorker.ts (2)
2-3: LGTM! Clean import organization.The imports for FormulaKey type and calcFormula function are properly organized.
145-145: Verify formula calculation performance.The change from direct sum to calcFormula might impact performance. Consider adding performance monitoring to ensure the calculation remains efficient for large datasets.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
apps/zzz-frontend/src/app/PageHome.tsx (2)
22-22: Fix typo in text content."optimial" should be "optimal"
- <Typography sx={{ my: 1 }}>Have an optimial day,</Typography> + <Typography sx={{ my: 1 }}>Have an optimal day,</Typography>
6-6: Consider using theme spacing utilities.Instead of hardcoding margin values, consider using theme spacing utilities for consistent spacing across the application.
- <CardThemed sx={{ my: 1 }}> + <CardThemed sx={{ my: theme.spacing(2) }}>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/zzz-frontend/src/app/PageHome.tsx(1 hunks)
🔇 Additional comments (1)
apps/zzz-frontend/src/app/PageHome.tsx (1)
1-2: LGTM! Clean and well-organized imports.The imports are properly organized, separating internal and external dependencies, and only importing the necessary components.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
libs/zzz/consts/src/common.ts (2)
60-89: Inconsistent naming convention in statKeyTextMap.The naming convention for stat keys is inconsistent. Some keys use underscores (e.g.,
crit_,crit_dmg_), while others don't (e.g.,anomProf). Consider standardizing the naming convention across all keys.
99-101: Add type safety to elementalData mapping.The mapping of elementalData to statKeyTextMap lacks type safety. Consider using a type-safe approach to ensure all elemental keys are properly mapped.
-Object.entries(elementalData).forEach(([e, name]) => { - statKeyTextMap[`${e}_dmg_`] = `${name} DMG Bonus` -}) +Object.entries(elementalData).forEach(([e, name]: [ElementalKey, string]) => { + const key = `${e}_dmg_` as const + statKeyTextMap[key] = `${name} DMG Bonus` +})libs/zzz/page-optimize/src/Optimize.tsx (2)
70-75: Optimize disc filtering logic.The current implementation filters discs in the reduce callback, which can be inefficient for large datasets. Consider filtering the discs before the reduce operation.
const discsBySlot = useMemo( () => - database.discs.values.reduce( + database.discs.values + .filter( + (disc) => + !( + (disc.slotKey === '4' && !slot4.includes(disc.mainStatKey)) || + (disc.slotKey === '5' && !slot5.includes(disc.mainStatKey)) || + (disc.slotKey === '6' && !slot6.includes(disc.mainStatKey)) + ) + ) + .reduce( (discsBySlot, disc) => { - if ( - (disc.slotKey === '4' && !slot4.includes(disc.mainStatKey)) || - (disc.slotKey === '5' && !slot5.includes(disc.mainStatKey)) || - (disc.slotKey === '6' && !slot6.includes(disc.mainStatKey)) - ) - return discsBySlot discsBySlot[disc.slotKey].push(disc) return discsBySlot },
153-164: Add accessibility attributes to UI components.The buttons in the disc slot UI lack proper accessibility attributes. Add
aria-labelandaria-pressedattributes to improve accessibility.<Button key={key} variant={keysMap[slotKey].includes(key) ? 'contained' : 'outlined'} onClick={() => funcMap[slotKey]((s) => toggleInArr([...s], key))} + aria-label={`Toggle ${key} for slot ${slotKey}`} + aria-pressed={keysMap[slotKey].includes(key)} > {key} </Button>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
apps/zzz-frontend/src/app/PageHome.tsx(1 hunks)libs/zzz/consts/src/common.ts(1 hunks)libs/zzz/consts/src/disc.ts(0 hunks)libs/zzz/disc-scanner/src/lib/enStringMap.ts(0 hunks)libs/zzz/page-optimize/src/Optimize.tsx(7 hunks)libs/zzz/solver/src/parentWorker.ts(5 hunks)
💤 Files with no reviewable changes (2)
- libs/zzz/consts/src/disc.ts
- libs/zzz/disc-scanner/src/lib/enStringMap.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- libs/zzz/solver/src/parentWorker.ts
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: lint
🔇 Additional comments (1)
apps/zzz-frontend/src/app/PageHome.tsx (1)
6-27: Consider production readiness and accessibility improvements.The implementation has several areas that need improvement:
- The informal content and humorous quotes might not be suitable for a production environment.
- The component lacks proper semantic structure and accessibility attributes.
- Text content should be externalized for internationalization support.
Apply this diff to improve the implementation:
+ import { useTranslation } from 'react-i18next' export default function PageHome() { + const { t } = useTranslation() return ( - <CardThemed sx={{ my: 1 }}> + <CardThemed sx={{ my: 1 }} role="main" aria-labelledby="page-title"> <CardContent> - <Typography variant="h5">I know what this looks like...</Typography> + <Typography variant="h1" id="page-title">{t('home.title')}</Typography> - <Typography> - This is an <i>Alpha</i>(desperately unfinished) version of{' '} - <strong>Zenless Optimizer</strong>. You have to understand, this is - the result of asking, - </Typography> + <Typography>{t('home.description')}</Typography> - <Typography variant="caption"> - "What is the most Optimizer I can, if I was cutting corners like it's - crunch time at a circle factory?" — frzyc, probably - </Typography> + <Typography variant="caption">{t('home.status')}</Typography> - <Typography> - I know a lot of stuff isn't implemented, and the stuff that are, are - super ugly. This is only like 10 hours of coding, cut me some slack. - </Typography> + <Typography>{t('home.disclaimer')}</Typography> - <Typography sx={{ my: 1 }}>Have an optimal day,</Typography> - <Typography> - <strong>frzyc</strong> - </Typography> + <Typography sx={{ my: 1 }}>{t('home.signature')}</Typography> </CardContent> </CardThemed> ) }
frzyc
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
libs/zzz/db/src/Database/DataManagers/DiscDataManager.ts (1)
Line range hint
31-64: Implement equipment management in toCache method.The TODO section contains critical equipment management logic that needs to be implemented:
- Character equipment updates
- Disc location changes
- Equipment swapping logic
Would you like me to help implement the equipment management logic?
♻️ Duplicate comments (4)
libs/zzz/solver/src/calc.ts (3)
31-36:⚠️ Potential issueFix base stat calculations for percentage values.
The base stat calculations need to account for percentage values.
61-63:⚠️ Potential issueFix damage multiplier calculations.
The damage bonus and crit multiplier calculations need to account for percentage values.
102-103:⚠️ Potential issueReview resistance multiplier calculation.
The resistance multiplier calculation needs to handle different resistance cases correctly.
libs/zzz/page-optimize/src/Optimize.tsx (1)
296-296:⚠️ Potential issueFix typo in formulaKeyTextMap.
There's a typo in "Eletrical" which should be "Electrical".
🧹 Nitpick comments (13)
libs/zzz/consts/src/character.ts (2)
1-30: Consider improving character list organization and documentation.The character list could benefit from:
- Alphabetical sorting for easier maintenance
- Grouping or categorization if applicable (e.g., by element or region)
- Documentation comments explaining the purpose and maintenance strategy
+/** + * List of all available characters in the game. + * @note Keep this list alphabetically sorted for easier maintenance + */ export const allCharacterKeys = [ 'Anby', 'Anton', // ... (rest of the sorted list) ] as const
33-34: Document the purpose of empty string in location keys.The empty string in
allLocationKeysneeds documentation explaining its purpose and usage scenarios.+/** + * Extends character keys with an empty string to represent unequipped/available state. + * @note Empty string represents items that are not currently equipped to any character + */ export const allLocationKeys = [...allCharacterKeys, ''] as constlibs/zzz/db/src/Database/DataManagers/DiscDataManager.ts (1)
332-332: Consider preserving invalid locations for debugging.The current implementation silently clears invalid locations. Consider logging these cases for debugging purposes.
- if (location && !allCharacterKeys.includes(location)) location = '' + if (location && !allCharacterKeys.includes(location)) { + console.warn(`Invalid location ${location} found in disc data`); + location = '' + }libs/zzz/consts/src/common.ts (3)
5-17: Consider adding JSDoc comments for the combat-related stats.The new combat-related stats like
charLvl,enemyDef, etc., would benefit from detailed documentation explaining their purpose, valid ranges, and usage in calculations.export const otherStatKeys = [ + /** Character level used in damage calculations. Valid range: 1-90 */ 'charLvl', + /** Base enemy defense value. Used in damage reduction calculations */ 'enemyDef', // ... add docs for other keys ] as const
33-58: Consider using an enum for ElementalKey and AnomalyDamageKey.Since these represent fixed sets of values that are unlikely to change frequently, using TypeScript enums would provide better type safety and autocompletion support.
-const allElementalKeys = [ - 'electric', - 'fire', - 'ice', - 'physical', - 'ether', -] as const -export type ElementalKey = (typeof allElementalKeys)[number] +export enum ElementalKey { + Electric = 'electric', + Fire = 'fire', + Ice = 'ice', + Physical = 'physical', + Ether = 'ether' +} +export const allElementalKeys = Object.values(ElementalKey)
94-104: Consider using a more type-safe approach for elementalData.The current implementation could potentially allow mismatches between
ElementalKeyand the string mappings.-const elementalData: Record<ElementalKey, string> = { +const elementalData: { [K in ElementalKey]: string } = { electric: 'Electric', fire: 'Fire', ice: 'Ice', physical: 'Physical', ether: 'Ether', } as constlibs/zzz/page-optimize/src/index.tsx (1)
11-18: Consider extracting default values to constants.The default values for
charLvlandenemyDefshould be defined as named constants to improve maintainability and reusability.+const DEFAULT_CHAR_LEVEL = 60 +const DEFAULT_ENEMY_DEF = 953 + const [baseStats, setBaseStats] = useState<BaseStats>({ - charLvl: 60, - enemyDef: 953, // default enemy DEF + charLvl: DEFAULT_CHAR_LEVEL, + enemyDef: DEFAULT_ENEMY_DEF, crit_: 5, crit_dmg_: 50, })libs/zzz/page-optimize/src/BaseStatCard.tsx (2)
Line range hint
12-34: Consider grouping related keys into separate files.The stat keys are growing in number and complexity. Consider moving them to separate files based on their categories (base stats, enemy stats, etc.) to improve maintainability.
Create new files like
baseStats.ts,enemyStats.ts, etc., and import the keys from there.
47-64: Consider memoizing the input rendering function.The
inputfunction is recreated on every render. Since it's used in multiple map operations, memoizing it could improve performance.-const input = (key: string) => ( +const input = useCallback((key: string) => ( <NumberInputLazy key={key} // ... rest of the implementation /> - ) +), [baseStats, setBaseStats])libs/zzz/page-optimize/src/BuildsDisplay.tsx (2)
64-92: Consider adding error handling for database operations.The onEquip callback looks good, but consider adding error handling for the database operations to handle potential failures gracefully.
const onEquip = useCallback(() => { + try { Object.values(build.discIds).forEach((dId) => { database.discs.set(dId, { location: locationKey }) }) + } catch (error) { + console.error('Failed to equip discs:', error) + // Consider showing a user-friendly error message + } }, [database.discs, locationKey, build.discIds])
103-111: Consider adding error handling for setLocation.The DiscCardWrapper component looks good, but consider adding error handling for the setLocation callback.
<DiscCard disc={disc} - setLocation={(location) => database.discs.set(discId, { location })} + setLocation={(location) => { + try { + database.discs.set(discId, { location }) + } catch (error) { + console.error('Failed to set disc location:', error) + // Consider showing a user-friendly error message + } + }} />libs/zzz/page-optimize/src/Optimize.tsx (1)
229-229: Consider adding a tooltip for the disabled state.When the optimize button is disabled, consider adding a tooltip to explain why (no permutations or no location selected).
disabled={!totalPermutations || !location} +title={!totalPermutations ? "No valid disc combinations found" : !location ? "Please select a location" : ""}libs/zzz/page-optimize/src/StatFilterCard.tsx (1)
71-102: Consider extracting constraint item into a separate component.The mapping logic contains complex UI rendering that could be extracted into a reusable
ConstraintItemcomponent for better maintainability and readability.Example structure:
interface ConstraintItemProps { statKey: string value: number isMax: boolean onChange: (value: number) => void onDelete: () => void } function ConstraintItem({ statKey, value, isMax, onChange, onDelete }: ConstraintItemProps) { return ( <Box display={'flex'} gap={1}> <NumberInputLazy value={value} onChange={onChange} inputProps={{ sx: { textAlign: 'right' } }} InputProps={{ startAdornment: statKey, endAdornment: getUnitStr(statKey), }} /> <ButtonGroup size="small"> <Button>{isMax ? 'MAX' : 'MIN'}</Button> <Button onClick={onDelete}> <DeleteForeverIcon /> </Button> </ButtonGroup> </Box> ) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
libs/zzz/consts/src/character.ts(1 hunks)libs/zzz/consts/src/common.ts(1 hunks)libs/zzz/consts/src/index.ts(1 hunks)libs/zzz/db/src/Database/DataManagers/DiscDataManager.ts(2 hunks)libs/zzz/db/src/Interfaces/IDisc.ts(2 hunks)libs/zzz/page-optimize/src/BaseStatCard.tsx(2 hunks)libs/zzz/page-optimize/src/BuildsDisplay.tsx(3 hunks)libs/zzz/page-optimize/src/Optimize.tsx(7 hunks)libs/zzz/page-optimize/src/StatFilterCard.tsx(3 hunks)libs/zzz/page-optimize/src/index.tsx(2 hunks)libs/zzz/solver/src/calc.ts(1 hunks)libs/zzz/ui/src/Character/LocationAutocomplete.tsx(1 hunks)libs/zzz/ui/src/Character/LocationName.tsx(1 hunks)libs/zzz/ui/src/Character/StatDisplay.tsx(2 hunks)libs/zzz/ui/src/Character/index.tsx(1 hunks)libs/zzz/ui/src/Disc/DiscCard.tsx(6 hunks)libs/zzz/ui/src/Disc/DiscEditor/DiscEditor.tsx(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- libs/zzz/ui/src/Character/index.tsx
🔇 Additional comments (17)
libs/zzz/consts/src/index.ts (1)
1-1: LGTM! Clean module organization.The addition of the character module export follows the established barrel pattern, maintaining consistent module organization.
libs/zzz/db/src/Interfaces/IDisc.ts (1)
7-7: LGTM! Improved type safety for location handling.The change from
stringtoLocationKeyenhances type safety by ensuring only valid location values can be assigned.Also applies to: 20-20
libs/zzz/ui/src/Character/StatDisplay.tsx (1)
14-14: Add translation support back to StatDisplay component.The current implementation uses a direct mapping lookup which might not support all localization requirements. Consider implementing the TODO to add proper translation support.
Also applies to: 17-18
✅ Verification successful
Translation support should be restored in StatDisplay component
The codebase shows consistent use of the 'statKey_gen' translation namespace for stat-related text across multiple components. Removing translation support from StatDisplay deviates from this established pattern and could impact localization consistency.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if translations exist for statKey_gen namespace rg -g '*.json' -l "statKey_gen"Length of output: 31
Script:
#!/bin/bash # Check for translation usage patterns echo "=== Translation Hook Usage ===" rg "useTranslation\(" -A 2 echo -e "\n=== Translation Files ===" fd -e json -e yaml -e yml . | grep -i "translation\|locale\|lang" echo -e "\n=== StatDisplay Implementation ===" rg -t typescript -t tsx "StatDisplay" -A 10Length of output: 66308
libs/zzz/ui/src/Character/LocationAutocomplete.tsx (2)
29-33: Implement database integration for character validation.The current implementation uses placeholder functions that always return true/false. This should be properly integrated with the database.
Would you like me to help implement the database integration for character validation?
Also applies to: 35-40
65-65: Add character image support to LocationAutocomplete.The
toImgprop currently renders an empty fragment. Consider implementing character image support for better user experience.✅ Verification successful
Empty
toImgimplementation needs to be addressed as marked by TODO commentThe review comment is valid. The codebase shows a consistent pattern where similar components in other game modules (GI, SR) implement character image support, while this component explicitly marks it as a TODO item.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for character image implementations in the codebase rg "toImg.*character" -A 5Length of output: 28
Script:
#!/bin/bash # Search for toImg implementations echo "=== toImg implementations ===" rg "toImg" -A 3 # Get the full LocationAutocomplete component echo -e "\n=== LocationAutocomplete implementation ===" cat libs/zzz/ui/src/Character/LocationAutocomplete.tsx # Search for character image related patterns echo -e "\n=== Character image patterns ===" rg "character.*image|image.*character" -iLength of output: 15368
libs/zzz/ui/src/Disc/DiscCard.tsx (2)
Line range hint
74-89: Complete the TODO items for disc set information.Multiple features are commented out and marked as TODO:
- Individual disc set piece names/descriptions need to be added to sheets
- Slot name and description tooltips are disabled
✅ Verification successful
Commented code is intentionally disabled awaiting data implementation
The commented-out code and TODO are legitimate. The feature is blocked pending the addition of disc set piece names and descriptions to sheets, and the code is maintained in a consistent state by fully commenting out both the component definitions and their usage.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for existing sheet implementations rg "DiscSetSlotName|DiscSetSlotDesc" -A 5Length of output: 1262
Script:
#!/bin/bash # Check for sheet-related implementations and TODOs for disc sets rg "disc.*set.*sheet" -i rg "TODO.*sheet.*disc" -iLength of output: 161
168-171: Re-enable StatIcon component for main stat display.The StatIcon component is commented out but would provide better visual feedback for the main stat.
✅ Verification successful
Re-enabling StatIcon is appropriate for consistent UI feedback
The StatIcon component is actively used in related components within the same module (DiscMainStatDropdown, SubstatInput) for visual stat representation. Re-enabling it would maintain consistency with the established UI patterns in the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check StatIcon implementation and usage ast-grep --pattern 'StatIcon statKey={$_}'Length of output: 44
Script:
#!/bin/bash # Search for StatIcon component file fd StatIcon # Search for StatIcon usage in imports and JSX rg "StatIcon" -l # Get context around StatIcon mentions rg "StatIcon" -A 3 -B 3Length of output: 52138
libs/zzz/consts/src/common.ts (1)
19-26: LGTM! Clear naming convention for unconditional stats.The
unCondKeysarray follows a consistent naming pattern with the 'uncond_' prefix, making it easy to identify and maintain.libs/zzz/page-optimize/src/index.tsx (1)
21-36: LGTM! Clean prop passing pattern.The component follows a consistent pattern in passing location-related props to child components.
libs/zzz/page-optimize/src/BaseStatCard.tsx (1)
68-85: LGTM! Well-organized UI structure.The UI is logically organized into distinct sections with clear typography headers, making it easy for users to understand and navigate different stat categories.
libs/zzz/ui/src/Disc/DiscEditor/DiscEditor.tsx (1)
414-417: LGTM! Clean integration of LocationAutocomplete.The LocationAutocomplete component is properly integrated with the disc editor, maintaining consistency with the location management pattern used throughout the application.
libs/zzz/page-optimize/src/BuildsDisplay.tsx (2)
1-15: LGTM!The imports are well-organized and properly scoped for the new UI components and location handling functionality.
19-39: LGTM!The BuildsDisplay component now properly handles location-based build management with a clean layout using Stack.
libs/zzz/solver/src/calc.ts (1)
170-183: Consider adding validation for anomaly damage values.The anomaly damage values are hardcoded. Consider adding validation or making them configurable.
libs/zzz/page-optimize/src/StatFilterCard.tsx (3)
8-13: LGTM! Well-organized imports.The new imports are properly organized and grouped, maintaining good code structure.
76-84: Add validation to NumberInputLazy.The number input should validate the entered values to ensure they are within acceptable ranges for the specific stat type.
Please verify:
- Are there any min/max bounds for different stat types?
- Should negative values be allowed?
- Should decimal places be limited for certain stats?
116-116: LGTM! Good use of fallback pattern.The use of
statKeyTextMapwith a fallback to the raw key improves UI readability while maintaining robustness.
| // Res Multiplier | ||
| resMulti(s) | ||
| // DMG Taken Multipler skipped | ||
| // Stunned Multiplier skipped |
There was a problem hiding this comment.
is it hard to add this? it helps compare the various stunners that are available atm
There was a problem hiding this comment.
I think it varies by enemy, could not find a good source/list for them.
The Stunned Multiplier varies by enemy, and the value can be seen under the enemy's Daze bar when they are Stunned.
There was a problem hiding this comment.
Ye I just meant put it in as something to be modified by user. Same could be done for mv%
There was a problem hiding this comment.
might add it later, right now, only concern are stats that directly correlates with the target.
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (1)
libs/zzz/page-optimize/src/Optimize.tsx (1)
Line range hint
115-140: Add error handling to optimization process.The optimization process should handle potential errors from the worker and provide user feedback.
const onOptimize = useCallback(async () => { const cancelled = new Promise<void>((r) => (cancelToken.current = r)) setProgress(undefined) setOptimizing(true) + try { const optimizer = new Solver( formulaKey, objMap(baseStats, (v, k) => toDecimal(v, k)), objMap(constraints, (c, k) => ({ ...c, value: toDecimal(c.value, k) })), discsBySlot, numWorkers, setProgress ) cancelled.then(async () => await optimizer.terminate()) const results = await optimizer.optimize() await optimizer.terminate() cancelToken.current = () => {} setOptimizing(false) setResults(results) + } catch (error) { + setOptimizing(false) + // Handle error appropriately (e.g., show error message to user) + console.error('Optimization failed:', error) + } }, [baseStats, constraints, discsBySlot, formulaKey, numWorkers, setResults])
🧹 Nitpick comments (6)
libs/zzz/page-optimize/src/Optimize.tsx (3)
70-72: Consider memoizing initial slot states.The initial states for slots 4, 5, and 6 are created on every render. Consider memoizing these values to optimize performance.
-const [slot4, setSlot4] = useState([...discSlotToMainStatKeys['4']]) -const [slot5, setSlot5] = useState([...discSlotToMainStatKeys['5']]) -const [slot6, setSlot6] = useState([...discSlotToMainStatKeys['6']]) +const [slot4, setSlot4] = useState(() => [...discSlotToMainStatKeys['4']]) +const [slot5, setSlot5] = useState(() => [...discSlotToMainStatKeys['5']]) +const [slot6, setSlot6] = useState(() => [...discSlotToMainStatKeys['6']])
78-85: Improve readability of disc filtering logic.The nested conditions can be flattened for better readability using early returns or combining conditions.
-if (disc.location && !useEquipped && disc.location !== location) - return discsBySlot -if ( - (disc.slotKey === '4' && !slot4.includes(disc.mainStatKey)) || - (disc.slotKey === '5' && !slot5.includes(disc.mainStatKey)) || - (disc.slotKey === '6' && !slot6.includes(disc.mainStatKey)) -) - return discsBySlot +const isWrongLocation = disc.location && !useEquipped && disc.location !== location +const isInvalidMainStat = ( + (disc.slotKey === '4' && !slot4.includes(disc.mainStatKey)) || + (disc.slotKey === '5' && !slot5.includes(disc.mainStatKey)) || + (disc.slotKey === '6' && !slot6.includes(disc.mainStatKey)) +) +if (isWrongLocation || isInvalidMainStat) return discsBySlot
309-388: Extract common set selection logic.The Set4Selector component has duplicate logic for 2-set and 4-set selection. Consider extracting the common functionality into a reusable helper function.
+const updateSetConstraint = ( + constraints: Constraints, + setKey: DiscSetKey, + setValue: number +) => ({ + ...Object.fromEntries( + Object.entries(constraints).filter( + ([k, { value }]) => + !(allDiscSetKeys.includes(k as DiscSetKey) && value === setValue) + ) + ), + [setKey]: { value: setValue, isMax: false }, +}) function Set4Selector({ constraints, setConstraints }) { // ... existing code ... return ( <> <DropdownButton> {allDiscSetKeys.map((d) => ( <MenuItem key={d} - onClick={() => - setConstraints({ - ...Object.fromEntries( - Object.entries(constraints).filter( - ([k, { value }]) => - !(allDiscSetKeys.includes(k as DiscSetKey) && value === 4) - ) - ), - [d]: { value: 4, isMax: false }, - }) - } + onClick={() => setConstraints(updateSetConstraint(constraints, d, 4))} > {d} </MenuItem> ))} </DropdownButton> <DropdownButton> {allDiscSetKeys.map((d) => ( <MenuItem key={d} - onClick={() => - setConstraints({ - ...Object.fromEntries( - Object.entries(constraints).filter( - ([k, { value }]) => - !(allDiscSetKeys.includes(k as DiscSetKey) && value === 2) - ) - ), - [d]: { value: 2, isMax: false }, - }) - } + onClick={() => setConstraints(updateSetConstraint(constraints, d, 2))} > {d} </MenuItem> ))} </DropdownButton> </> ) }libs/zzz/consts/src/common.ts (3)
33-49: Consider adding JSDoc comments for damage types.While the implementation is solid, adding JSDoc comments would help clarify:
- The difference between
ElementalKeyandAttributeDamageKey- The significance of 'physical' and 'ether' as damage types
- Any game-specific mechanics these types represent
Example documentation:
+/** + * Represents the core elemental types in the game. + * Includes physical and ether as special damage types. + */ const allElementalKeys = [ 'electric', 'fire', 'ice', 'physical', 'ether', ] as const
51-58: Consider grouping related damage type constants.The anomaly damage types are well-defined but consider grouping them with other damage-related constants for better organization.
Example refactor:
+/** Group all damage-related constants together */ +export const DamageTypes = { + elemental: allElementalKeys, + attribute: allAttributeDamageKeys, + anomaly: allAnomalyDmgKeys, +} as const
60-98: Clarify the difference between duplicate stat key mappings.There are several stat keys that map to the same display text:
- 'hp' and 'hp_' → 'HP'
- 'atk' and 'atk_' → 'ATK'
- 'def' and 'def_' → 'DEF'
Consider adding suffixes to differentiate between flat and percentage values in the display text.
Example refactor:
export const statKeyTextMap: Partial<Record<string, string>> = { hp_base: 'Base HP', atk_base: 'Base ATK', def_base: 'Base DEF', hp: 'HP', - hp_: 'HP', + hp_: 'HP%', atk: 'ATK', - atk_: 'ATK', + atk_: 'ATK%', def: 'DEF', - def_: 'DEF', + def_: 'DEF%',
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
libs/zzz/consts/src/common.ts(1 hunks)libs/zzz/db/src/Database/DataManagers/DiscDataManager.ts(3 hunks)libs/zzz/page-optimize/src/Optimize.tsx(7 hunks)libs/zzz/solver/src/calc.ts(1 hunks)libs/zzz/ui/src/Disc/DiscCard.tsx(8 hunks)libs/zzz/ui/src/Disc/DiscEditor/DiscEditor.tsx(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- libs/zzz/db/src/Database/DataManagers/DiscDataManager.ts
- libs/zzz/ui/src/Disc/DiscCard.tsx
- libs/zzz/ui/src/Disc/DiscEditor/DiscEditor.tsx
🔇 Additional comments (9)
libs/zzz/solver/src/calc.ts (6)
1-13: LGTM! Well-organized imports and type definitions.The imports are properly structured and use specific types, promoting type safety.
41-51: LGTM! Well-structured formula calculation setup.The type definitions and formula calculation function are well-implemented with proper typing.
63-65: Correct the Crit Multiplier and Damage Bonus calculations.The calculations for the Damage Bonus Multiplier and Crit Multiplier need to account for percentage values by converting them to decimal form.
96-102: Prevent potential division by zero in DEF Multiplier.The defMulti function needs to handle potential division by zero and adjust percentage values appropriately.
103-105: Review and correct the Resistance Multiplier calculation.The resMulti function needs to handle different cases for enemy resistance, especially for negative resistance values or resistances above certain thresholds.
172-185: LGTM! Well-defined anomaly damage mappings.The anomaly damage mappings and base damage values are well-structured and properly typed.
libs/zzz/page-optimize/src/Optimize.tsx (1)
295-307: LGTM! The formula key mapping is comprehensive and accurate.The previous typo has been fixed, and the mapping covers all necessary damage types and anomalies.
libs/zzz/consts/src/common.ts (2)
19-26: LGTM! Well-structured unconditional stats.The implementation follows consistent naming patterns and covers both flat and percentage values for core stats.
100-110: LGTM! Clean implementation of elemental data mapping.The implementation effectively maps elemental keys to their display names and populates the stat key text map.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
libs/zzz/solver/src/calc.ts (3)
63-65:⚠️ Potential issueCorrect the Crit Multiplier and Damage Bonus calculations.
The calculations for the Damage Bonus Multiplier and Crit Multiplier need to account for percentage values by converting them to decimal form.
96-102:⚠️ Potential issuePrevent potential division by zero in DEF Multiplier.
The defMulti function needs safeguards against division by zero and proper handling of percentage values.
103-105:⚠️ Potential issueReview and correct the Resistance Multiplier calculation.
The current implementation doesn't handle different resistance cases correctly (negative resistance, resistance above thresholds).
🧹 Nitpick comments (9)
libs/zzz/solver/src/calc.ts (3)
71-71: Consider implementing the Stunned Multiplier.The comment indicates that the Stunned Multiplier calculation is skipped. This could be valuable for comparing various stunners available in the game.
Would you like me to help implement the Stunned Multiplier calculation?
82-84: Extract magic numbers into named constants.The code uses magic numbers (0.01, 1/59) which reduce readability and maintainability.
Consider extracting these into named constants:
+const ANOMALY_PROFICIENCY_FACTOR = 0.01 +const ANOMALY_LEVEL_SCALING = 1/59 // Anomaly Proficiency Multiplier - (s('anomProf') * 0.01) * + (s('anomProf') * ANOMALY_PROFICIENCY_FACTOR) * // Anomaly Level Multiplier - (1 + (1 / 59) * (s('charLvl') - 1)) * + (1 + ANOMALY_LEVEL_SCALING * (s('charLvl') - 1)) *
109-170: Improve type safety of level factor map.The level factor map uses string keys instead of numbers, which could lead to type-related issues.
Consider using a number-indexed map:
-const lvlFactorMap = { - '1': 50, - '2': 54, +const lvlFactorMap: Record<number, number> = { + 1: 50, + 2: 54, // ... rest of the entries -} as const +}apps/zzz-frontend/index.html (1)
13-22: Consider making Open Graph URLs configurable.The Open Graph URL is hardcoded to GitHub Pages (
frzyc.github.io). Consider making it configurable through environment variables to support different deployment environments.libs/zzz/page-optimize/src/BaseStatCard.tsx (2)
12-12: Consider grouping related stats together.The
statKeysarray mixes different types of stats. Consider grouping them into separate arrays by category (e.g., base stats, damage stats, anomaly stats) for better maintainability.-const statKeys: StatKey[] = [ +const baseStatKeys: StatKey[] = [ 'hp', 'atk', 'def', 'hp_', 'atk_', 'def_', +] as const + +const damageStatKeys: StatKey[] = [ 'crit_', 'crit_dmg_', ...allAttributeDamageKeys, 'dmg_', +] as const + +const anomalyStatKeys: StatKey[] = [ 'impact', 'anomMas', 'anomProf', 'pen_', 'pen', ] as const + +const statKeys: StatKey[] = [ + ...baseStatKeys, + ...damageStatKeys, + ...anomalyStatKeys, +] as constAlso applies to: 23-29
47-64: Consider extracting input component.The
inputfunction is a reusable component. Consider extracting it to a separate file for better modularity and reusability.// StatInput.tsx export function StatInput({ statKey, value, onChange, }: { statKey: string value: number onChange: (value: number) => void }) { return ( <NumberInputLazy key={statKey} sx={{ flexGrow: 1 }} value={value} onChange={onChange} inputProps={{ sx: { textAlign: 'right', minWidth: '5em' } }} InputProps={{ startAdornment: statKeyTextMap[statKey] ?? statKey, endAdornment: getUnitStr(statKey), }} /> ) }libs/zzz/consts/src/common.ts (3)
33-40: Consider using an enum for ElementalKey.Since
allElementalKeysrepresents a fixed set of values, consider using an enum for better type safety and IDE support.-const allElementalKeys = [ - 'electric', - 'fire', - 'ice', - 'physical', - 'ether', -] as const -export type ElementalKey = (typeof allElementalKeys)[number] +export enum ElementalKey { + Electric = 'electric', + Fire = 'fire', + Ice = 'ice', + Physical = 'physical', + Ether = 'ether', +} +export const allElementalKeys = Object.values(ElementalKey)
51-58: Consider using an enum for AnomalyDamageKey.Similar to ElementalKey, AnomalyDamageKey would benefit from being an enum.
-export const allAnomalyDmgKeys = [ - 'burn', - 'shock', - 'corruption', - 'shatter', - 'assault', -] as const -export type AnomalyDamageKey = (typeof allAnomalyDmgKeys)[number] +export enum AnomalyDamageKey { + Burn = 'burn', + Shock = 'shock', + Corruption = 'corruption', + Shatter = 'shatter', + Assault = 'assault', +} +export const allAnomalyDmgKeys = Object.values(AnomalyDamageKey)
60-98: Consider using a type-safe approach for statKeyTextMap.The current implementation uses
Partial<Record<string, string>>which loses type safety. Consider using a more type-safe approach.-export const statKeyTextMap: Partial<Record<string, string>> = { +type StatTextKey = StatKey | AttributeDamageKey | `${ElementalKey}_dmg_` +export const statKeyTextMap: Record<StatTextKey, string> = {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
apps/zzz-frontend/index.html(1 hunks)libs/zzz/consts/src/common.ts(1 hunks)libs/zzz/page-optimize/src/BaseStatCard.tsx(2 hunks)libs/zzz/solver/src/calc.ts(1 hunks)libs/zzz/ui/src/Disc/DiscCard.tsx(8 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- libs/zzz/ui/src/Disc/DiscCard.tsx
🔇 Additional comments (1)
libs/zzz/consts/src/common.ts (1)
5-6: Remove or update the speculative bundling comment.The comment "Used by calc, likely will be bundled into pando" is speculative and not actionable.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
libs/zzz/ui/src/Settings/UploadCard.tsx (3)
59-61: Consider using more descriptive error keys and verifying format versions.
- The error key
'uploadCard.error.sroInvalid'might be outdated or misleading now that you are working with'ZOD'. Consider using'zodInvalid'for clarity.- If multiple future variants of
'ZOD'are expected (e.g.,'ZODv2'), you might want to implement version checks alongside'parsed.format', and handle them more gracefully.- setErrorMsg('uploadCard.error.sroInvalid') + setErrorMsg('uploadCard.error.zodInvalid')
221-221: Unify the naming of ZOD components.You're calling the new component
<ZOUploadAction>but referencing a'ZOD'import format elsewhere. For consistency and clarity, consider changingZOUploadActiontoZODUploadAction.- <ZOUploadAction + <ZODUploadAction index={index} importedDatabase={importedDatabase} reset={reset} />
356-356: Confirm naming consistency for ZOUploadAction.Similarly, consider updating the function name to align with the
'ZOD'naming scheme to avoid confusion in your codebase.-function ZOUploadAction({ +function ZODUploadAction({ index, importedDatabase, reset, }: { ...libs/zzz/db/src/Database/DataEntry.ts (2)
13-13: Guard against missing database fields.Assigning
this.datatozoDb[this.dataKey]is straightforward. Ifthis.datacan be partially undefined, consider merging rather than overwriting to preserve existing fields inzoDb[this.dataKey].
23-25: Customize key generation with caution.Overriding
toStorageKey()to return${this.database.keyPrefix}_${this.key}is succinct. Keep an eye on potential collisions if other storage classes use similar naming patterns or ifkeycan contain underscores.libs/sr/db/src/Database/DataEntry.ts (1)
24-26: Maintain unique storage naming.Overriding
toStorageKey()with\${this.database.keyPrefix}_${this.key}`ensures a standard naming format. If there’s a chance of reusing the samekey` across different database instances, consider including an additional unique segment to avoid collisions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
libs/common/database/src/lib/DataEntryBase.ts(5 hunks)libs/gi/db/src/Database/DataEntries/DBMetaEntry.ts(1 hunks)libs/gi/db/src/Database/DataEntry.ts(1 hunks)libs/sr/db/src/Database/DataEntry.ts(1 hunks)libs/zzz/db-ui/src/context/CharacterContext.ts(0 hunks)libs/zzz/db-ui/src/context/index.ts(0 hunks)libs/zzz/db/src/Database/DataEntry.test.ts(1 hunks)libs/zzz/db/src/Database/DataEntry.ts(1 hunks)libs/zzz/db/src/Database/Database.ts(2 hunks)libs/zzz/db/src/Database/migrate.ts(1 hunks)libs/zzz/ui/src/Settings/UploadCard.tsx(3 hunks)
💤 Files with no reviewable changes (2)
- libs/zzz/db-ui/src/context/index.ts
- libs/zzz/db-ui/src/context/CharacterContext.ts
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: lint
🔇 Additional comments (21)
libs/zzz/db/src/Database/migrate.ts (1)
Line range hint
13-28: Consider adding actual migration steps or confirming no migrations are needed.While renaming
migrateSrtomigrateZODand settingzo.dbVersion = currentDBVersionmight be sufficient for now, the commented-out calls tomigrateVersionsuggest a more elaborate migration process was initially planned. If older database versions require transformations, you may need to implement them within this function. Otherwise, confirm that no additional migrations are required between legacy versions andcurrentDBVersion.libs/zzz/db/src/Database/Database.ts (2)
9-9: Import rename looks good.The reference to
migrateZODaligns well with the rename inmigrate.ts. Ensure that any remaining references tomigrateSrhave been removed or updated accordingly to avoid confusion.
67-67: Confirm migration coverage for imported data.Replacing
migrateSROD(zod)withmigrateZOD(zod)sets thezod.dbVersiontocurrentDBVersionwithout additional transformations. Verify if older versions require intermediate migration steps, especially ifdbVersionwas previously belowcurrentDBVersion. Otherwise, this approach may inadvertently skip needed updates for older data.libs/gi/db/src/Database/DataEntry.ts (2)
12-12: Ensure null-safe data assignment.Assigning
this.datatogo[this.dataKey]looks correct for maintaining parity between the stored and exported data. However, ifthis.datamight beundefinedornull, consider adding a guard or fallback value to prevent potential null references in downstream code.
18-18: Confirm data availability before assignment.Retrieving
go[this.dataKey]is consistent and aligns with the updated logic. Verify that all relevant calling sites handle cases wherego[this.dataKey]is missing or partially undefined to avoid runtime errors.libs/zzz/db/src/Database/DataEntry.ts (3)
8-11: Check naming consistency for type parameters.Renaming
SROKeytoZOKeyis clear and consistent with your new naming scheme. Ensure that references in other files have been updated accordingly, and confirm that no overlapping type parameter names remain in related data classes.
17-17: Validate partial import definitions.The type
{ [k in ZOKey]?: Partial<StorageValue> | never }correctly allows partial storage values. Double-check that all partial fields are optional in your usage and that invariants are enforced later if needed.
20-20: Check for data existence.This line properly retrieves
zoDb[this.dataKey]before callingthis.set(data). Ensure thethis.setmethod can handle unexpected data shapes or missing fields ifzoDb[this.dataKey]is incomplete.libs/sr/db/src/Database/DataEntry.ts (2)
14-14: Ensure consistent usage ofthis.dataKey.Replacing the old property reference with
this.dataKeyis consistent with the project’s new approach. Validate that no other references to the removed property remain, especially if multiple keys are managed.
21-21: Double-check partial data assignments.Reading
sroDb[this.dataKey]follows the new scheme. Confirm that this data can be safely assigned to the internal state without losing essential fields or introducing inconsistent data shapes.libs/zzz/db/src/Database/DataEntry.test.ts (1)
4-4: Confirm test isolation after changing the storage prefix.Changing the second parameter to
'zzz'updates how data is keyed in local storage. Ensure old data in'sro'is properly cleaned or migrated if needed and that changing the prefix does not inadvertently break test isolation or conflict with other tests.libs/gi/db/src/Database/DataEntries/DBMetaEntry.ts (1)
44-44: Adoption ofdataKey
This change fromgo[this.goKey]togo[this.dataKey]aligns with the new naming convention in the data entry base. Ensure all references to the old key have been updated consistently across the codebase.libs/common/database/src/lib/DataEntryBase.ts (9)
6-9: Refined generic parameters
SeparatingKeyandDatakeyclarifies the distinction between the internal storage key and the external data key for import/export. Consider verifying that references to the old generic parameters have been removed or updated accordingly.
18-18: NewdataKeyproperty
AddingdataKeysupports a dedicated key reference for external data operations, promoting better code clarity.
23-28: Constructor signature
IncorporatingdataKeyinto the constructor clarifies how the external data key is initialized and used, reducing confusion between internal and external keys.
36-36: Minor file revision
This single-line change appears to be a minor edit and should have no functional impact.
37-42: NewtoStorageKey()method
Centralizing the derivation of the storage key intoStorageKey()enhances consistency for storage operations. Good improvement.
57-57: Refactor ingetStorage()
Replacingthis.keywiththis.toStorageKey()unifies how the storage key is derived, which helps maintain consistent data retrieval across the codebase.
88-88: Consistent usage insetCached()
InvokingtoStorageKey()again avoids possible future mismatches, ensuring data storage remains uniform across methods.
99-99: Streamlined removal logic
Usingthis.toStorageKey()inclearStorage()aligns well with the unified storage key approach.
102-102: Uniform saving logic
Applyingthis.toStorageKey()insaveStorage()completes the move to a centralized key resolution strategy.
As long as it doesnt affect GO data it should be fine. rest can be CI/CD later. |
frzyc
changed the title
Describe your changes
Issue or discord link
Testing/validation
Checklist before requesting a review (leave this PR as draft if any part of this list is not done.)
yarn run mini-cilocally to validate format and lint.Summary by CodeRabbit
Release Notes
New Features
LocationNamecomponent for displaying location names.LocationAutocompletecomponent for character location selection.Improvements
DiscCardcomponent.Technical Updates