preliminary targeted conditional solution #2528
Conversation
WalkthroughThis pull request introduces several modifications across multiple files, primarily focusing on enhancing component structures, improving prop management, and refining logic for handling conditionals and data flow. Key changes include the addition of new components and contexts, updates to existing function signatures, and the removal of unused elements. The changes aim to streamline the codebase, enhance clarity, and improve maintainability across various functionalities related to team data and UI components. Changes
Possibly related PRs
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 (
|
|
[sr-frontend] [Sun Nov 3 04:52:15 UTC 2024] - Deployed e6e05ae to https://genshin-optimizer-prs.github.io/pr/2528/sr-frontend (Takes 3-5 minutes after this completes to be available) [frontend] [Sun Nov 3 04:52:56 UTC 2024] - Deployed e6e05ae to https://genshin-optimizer-prs.github.io/pr/2528/frontend (Takes 3-5 minutes after this completes to be available) [sr-frontend] [Tue Nov 5 04:55:27 UTC 2024] - Deployed 859a062 to https://genshin-optimizer-prs.github.io/pr/2528/sr-frontend (Takes 3-5 minutes after this completes to be available) [frontend] [Tue Nov 5 04:55:49 UTC 2024] - Deployed 859a062 to https://genshin-optimizer-prs.github.io/pr/2528/frontend (Takes 3-5 minutes after this completes to be available) [sr-frontend] [Tue Nov 5 06:38:50 UTC 2024] - Deployed 3e4cf0a to https://genshin-optimizer-prs.github.io/pr/2528/sr-frontend (Takes 3-5 minutes after this completes to be available) [frontend] [Tue Nov 5 06:55:18 UTC 2024] - Deployed 452d48a to https://genshin-optimizer-prs.github.io/pr/2528/frontend (Takes 3-5 minutes after this completes to be available) [sr-frontend] [Tue Nov 5 06:55:49 UTC 2024] - Deployed 452d48a to https://genshin-optimizer-prs.github.io/pr/2528/sr-frontend (Takes 3-5 minutes after this completes to be available) [sr-frontend] [Tue Nov 5 07:21:26 UTC 2024] - Deployed 28b77f1 to https://genshin-optimizer-prs.github.io/pr/2528/sr-frontend (Takes 3-5 minutes after this completes to be available) [frontend] [Tue Nov 5 07:21:38 UTC 2024] - Deployed 28b77f1 to https://genshin-optimizer-prs.github.io/pr/2528/frontend (Takes 3-5 minutes after this completes to be available) [sr-frontend] [Tue Nov 5 07:28:36 UTC 2024] - Deployed 4c43efb to https://genshin-optimizer-prs.github.io/pr/2528/sr-frontend (Takes 3-5 minutes after this completes to be available) [frontend] [Tue Nov 5 07:28:39 UTC 2024] - Deployed 4c43efb to https://genshin-optimizer-prs.github.io/pr/2528/frontend (Takes 3-5 minutes after this completes to be available) [frontend] [Tue Nov 5 15:51:23 UTC 2024] - Deployed 2dde9aa to https://genshin-optimizer-prs.github.io/pr/2528/frontend (Takes 3-5 minutes after this completes to be available) [frontend] [Tue Nov 5 15:58:07 UTC 2024] - Deployed f696439 to https://genshin-optimizer-prs.github.io/pr/2528/frontend (Takes 3-5 minutes after this completes to be available) [sr-frontend] [Tue Nov 5 15:58:09 UTC 2024] - Deployed f696439 to https://genshin-optimizer-prs.github.io/pr/2528/sr-frontend (Takes 3-5 minutes after this completes to be available) [sr-frontend] [Tue Nov 5 20:28:18 UTC 2024] - Deployed 45cad92 to https://genshin-optimizer-prs.github.io/pr/2528/sr-frontend (Takes 3-5 minutes after this completes to be available) [frontend] [Tue Nov 5 20:28:28 UTC 2024] - Deployed 45cad92 to https://genshin-optimizer-prs.github.io/pr/2528/frontend (Takes 3-5 minutes after this completes to be available) [sr-frontend] [Wed Nov 6 01:15:39 UTC 2024] - Deployed 1a7c540 to https://genshin-optimizer-prs.github.io/pr/2528/sr-frontend (Takes 3-5 minutes after this completes to be available) [frontend] [Wed Nov 6 01:15:51 UTC 2024] - Deployed 1a7c540 to https://genshin-optimizer-prs.github.io/pr/2528/frontend (Takes 3-5 minutes after this completes to be available) [sr-frontend] [Wed Nov 6 01:51:57 UTC 2024] - Deployed 09c1a64 to https://genshin-optimizer-prs.github.io/pr/2528/sr-frontend (Takes 3-5 minutes after this completes to be available) [frontend] [Wed Nov 6 01:52:41 UTC 2024] - Deployed 09c1a64 to https://genshin-optimizer-prs.github.io/pr/2528/frontend (Takes 3-5 minutes after this completes to be available) [Wed Nov 6 03:57:39 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: 10
🧹 Outside diff range and nitpick comments (39)
libs/sr/formula-ui/src/relic/index.ts (2)
1-3: LGTM! Consider grouping imports by source.The imports are clean and use type imports appropriately. Consider grouping them by external (@Genshin-Optimizer) and local (./cavern) imports for better organization.
import type { UISheet } from '@genshin-optimizer/pando/ui-sheet' import type { RelicSetKey } from '@genshin-optimizer/sr/consts' + import WatchmakerMasterOfDreamMachinations from './cavern/WatchmakerMasterOfDreamMachinations'
5-7: Add documentation to explain the purpose and structure.The export is well-typed, but would benefit from documentation to explain its purpose and the meaning of the '2' | '4' type parameter.
+/** + * Maps relic sets to their UI sheets for 2-piece and 4-piece set bonuses. + * Currently includes: + * - WatchmakerMasterOfDreamMachinations + */ export const relicUiSheets: Partial<Record<RelicSetKey, UISheet<'2' | '4'>>> = { WatchmakerMasterOfDreamMachinations, } + +// TODO: Add more relic sets as they become availablelibs/sr/ui/src/Hook/useSrCalcContext.ts (1)
6-9: Consider adding error handling for null calculator.The hook returns undefined when
_calcis null. Consider:
- Adding documentation to clarify this behavior
- Adding runtime validation if null state is unexpected
export function useSrCalcContext() { const _calc = useContext(CalcContext) as Calculator | null const tag = useContext(TagContext) + // Throws if calculator is unexpectedly null + if (process.env.NODE_ENV === 'development' && !_calc) { + console.warn('Calculator context is null in useSrCalcContext') + } + return useMemo(() => _calc?.withTag(tag), [_calc, tag]) }libs/sr/ui/src/Character/CharacterTrans.tsx (1)
6-12: Consider performance and maintainability improvements.While the current implementation is functional, consider these enhancements:
- Memoize the component to optimize re-renders when used in lists
- Add error boundary handling for translation failures
Here's a suggested implementation:
+import { memo } from 'react' + +interface CharacterNameProps { + characterKey: CharacterKey +} + -export function CharacterName({ +export const CharacterName = memo(function CharacterName({ characterKey, -}: { - characterKey: CharacterKey -}) { - return <Translate ns="charNames_gen" key18={characterKey} /> -} +}: CharacterNameProps) { + try { + return <Translate ns="charNames_gen" key18={characterKey} /> + } catch (error) { + console.error(`Failed to translate character name: ${characterKey}`, error) + return <span>{characterKey}</span> + } +}) + +CharacterName.displayName = 'CharacterName'libs/pando/ui-sheet/src/types/conditional.ts (1)
13-13: Please add JSDoc documentation for thetargetedproperty.Since this property is crucial for implementing targeted buff conditionals with distinct src/dst values, adding documentation would help other developers understand its purpose and proper usage.
Example documentation:
export type Conditional = { metadata: IConditionalData label: ReactNode | ((calc: Calculator, value: number) => ReactNode) badge?: ReactNode | ((calc: Calculator, value: number) => ReactNode) header?: Header fields?: Field[] + /** When true, indicates that this conditional supports distinct values based on source and destination parameters */ targeted?: boolean }libs/pando/ui-sheet/src/components/HeaderDisplay.tsx (1)
6-21: Consider performance optimization and prop validation.The component looks good but could benefit from these improvements:
- Add React.memo to prevent unnecessary rerenders if the parent component updates frequently
- Add prop validation for the header object to ensure required fields are present
Here's how you could implement these improvements:
+import { memo } from 'react' + -export function HeaderDisplay({ +export const HeaderDisplay = memo(function HeaderDisplay({ header, hideDivider, }: { header: Header hideDivider?: boolean | ((section: Document) => boolean) }) { + if (!header.text) { + console.warn('HeaderDisplay: header.text is required') + return null + } + const { icon, text: title, additional: action } = header return ( <> <CardHeaderCustom avatar={icon} title={title} action={action} /> {!hideDivider && <Divider />} </> ) -} +})libs/sr/page-team/src/RelicSheetsDisplay.tsx (1)
6-6: Consider making the sets array configurable.The hardcoded array with a single relic set key might limit the component's reusability. Consider accepting this as a prop or fetching it from a configuration source.
-const sets: RelicSetKey[] = ['WatchmakerMasterOfDreamMachinations'] as const +interface RelicSheetsDisplayProps { + sets?: RelicSetKey[]; +} +const DEFAULT_SETS: RelicSetKey[] = ['WatchmakerMasterOfDreamMachinations'] as const;libs/sr/formula/src/data/relic/sheets/WatchmakerMasterOfDreamMachinations.ts (1)
Line range hint
18-18: Address TODO comment regarding directional targeted buffs.The TODO comment raises an important architectural question about implementing directional targeted buffs. This aligns with the PR's focus on targeted buff conditionals with distinct src/dst values.
Would you like me to help design a solution for implementing directional targeted buffs using the new TagContext system mentioned in the PR objectives? This could involve:
- Extending the current buff system to handle src/dst relationships
- Implementing validation for these directional buffs
- Creating appropriate UI components to display these relationships
libs/sr/formula-ui/src/relic/cavern/WatchmakerMasterOfDreamMachinations.tsx (1)
50-51: Address pending localization TODOsThere are multiple TODO comments indicating pending localization work for:
- 'Use Their Ult on an ally'
- '4-Set'
Please ensure these strings are properly localized before finalizing the PR.
Would you like me to help set up the localization entries for these strings?
libs/sr/page-team/src/RelicSheetDisplay.tsx (3)
26-27: Improve image component handling.The ternary operator in the component prop is not a React-recommended pattern. Consider creating a separate ImageComponent.
-component={NextImage ? NextImage : 'img'} +component={NextImage ?? 'img'}
36-39: Add translation for badge text.The TODO comment indicates missing translation. The badge text should be internationalized for better user experience.
-{/* TODO: translate */} -<Typography> - <SqBadge>{isCavernRelic(setKey) ? 'Cavern' : 'Planar'}</SqBadge> -</Typography> +<Typography> + <SqBadge>{t(`relic.type.${isCavernRelic(setKey) ? 'cavern' : 'planar'}`)}</SqBadge> +</Typography>
29-32: Consider using theme-based dimensions.Hard-coded dimensions make it harder to maintain consistent spacing across the application. Consider using theme-based spacing units.
sx={{ - maxHeight: '5em', + maxHeight: theme => theme.spacing(10), width: 'auto', }}libs/sr/ui/src/Character/CharacterCard.tsx (1)
Line range hint
78-107: Consider moving StatLine to a separate fileThe
StatLinecomponent appears to be a reusable component. Consider moving it to a separate file (e.g.,StatLine.tsx) to improve code organization and maintainability.libs/common/util/src/lib/array.ts (2)
93-98: Enhance documentation with more details.The JSDoc comments should include:
- Warning about in-place array modification
- Parameter validation requirements
- Example usage
/** * Move an element in the array to the front, if it exists. + * Modifies the array in-place. * @param arr The input array (must not be null/undefined) * @param key The element to move to front - * @returns + * @returns The modified array + * @example + * moveToFront([1, 2, 3], 2) // returns [2, 1, 3] + * moveToFront([1, 2, 3], 4) // returns [1, 2, 3] */
99-108: Add parameter validation and consider performance optimization.The implementation is correct but could be more robust with input validation and potentially more efficient for specific types.
Consider these improvements:
export function moveToFront<T>(arr: T[], key: T): T[] { + if (!Array.isArray(arr)) { + throw new TypeError('First argument must be an array'); + } + if (arr.length <= 1) return arr; + const index = arr.indexOf(key) if (index > -1) { - // Remove the element from its current position - const [element] = arr.splice(index, 1) - // Add the element to the front of the array - arr.unshift(element) + if (index === 0) return arr; + arr.unshift(...arr.splice(index, 1)); } return arr }Additionally, for better type safety and performance with specific types like numbers or strings, consider adding specialized overloads:
// Add above the existing implementation export function moveToFront(arr: number[], key: number): number[]; export function moveToFront(arr: string[], key: string): string[];libs/sr/page-team/src/ComboEditor.tsx (2)
117-128: Consider enhancing the modal implementation.The modal implementation could benefit from the following improvements:
- Add error boundary for RelicSheetsDisplay
- Consider adding loading state handling
- Add a descriptive title to the modal
function RelicConditionals() { const [show, onShow, onHide] = useBoolState() return ( <> - {/* TODO: translation */} - <Button onClick={onShow}>Relic Conditionals</Button> + <Button onClick={onShow}>{t('relicConditionals.button')}</Button> <ModalWrapper open={show} onClose={onHide}> + <CardHeader title={t('relicConditionals.modalTitle')} /> <RelicSheetsDisplay /> </ModalWrapper> </> ) }
121-122: Address the translation TODO comment.Please add the necessary translation keys for the "Relic Conditionals" button text.
Would you like me to help create the translation entries or open an issue to track this task?
libs/sr/page-team/src/TeamCalcProvider.tsx (1)
94-106: Consider refactoring nested buildType conditions.The nested ternary operations for buildType checks reduce readability. Consider extracting this logic into a separate function.
+ function getEquipmentId(buildType: string | undefined, equippedId?: string, buildId?: string) { + if (buildType === 'equipped') return equippedId; + if (buildType === 'real') return buildId; + return undefined; + } const lightCone = useLightCone( - meta?.buildType === 'equipped' - ? character?.equippedLightCone - : meta?.buildType === 'real' - ? build?.lightConeId - : undefined + getEquipmentId(meta?.buildType, character?.equippedLightCone, build?.lightConeId) )libs/common/database/src/lib/DataManagerBase.ts (2)
78-78: LGTM! Consider adding JSDoc to document the new return type.The addition of
falseas a return type provides a clear way to signal failure conditions. Consider adding JSDoc comments to document this behavior:+ /** + * Updates the value for the given key. + * @param key The key to update + * @param valueOrFunc The new value or a function to transform the existing value + * @param notify Whether to trigger notifications + * @returns false if the operation failed, true if successful + */🧰 Tools
🪛 Biome
[error] 78-78: void is confusing inside a union type.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
88-88: Consider adding a trigger notification for consistency.Other failure paths in this method trigger notifications (e.g., 'invalid'). For consistency, consider notifying listeners when a function explicitly returns
false:- if (value === false) return false + if (value === false) { + this.trigger(key, 'invalid', valueOrFunc) + return false + }libs/sr/util/src/relic.ts (1)
184-186: Add documentation and defensive programming to isCavernRelic function.The function would benefit from proper documentation and input validation.
Consider this enhancement:
+/** + * Checks if the given relic set key represents a cavern relic + * @param setKey - The relic set key to check + * @returns true if the set key represents a cavern relic, false otherwise + * @throws Error if setKey is not provided + */ export function isCavernRelic(setKey: RelicSetKey) { + if (!setKey) { + throw new Error('setKey is required'); + } return allRelicCavernSetKeys.includes(setKey as RelicCavernSetKey) }libs/sr/page-team/src/TeammateDisplay.tsx (2)
44-66: LGTM! Component structure improvements look good.The changes to Stack layout and component organization enhance the UI structure and maintainability.
Consider these minor improvements:
- Add a tooltip to explain why the Edit Character button is disabled
- Use translation keys for the "Edit Character" button text for consistency
<Button fullWidth disabled={!characterKey} + title={!characterKey ? "Select a character first" : undefined} onClick={() => characterKey && setCharacterKey(characterKey)} > - Edit Character + {t("button.editCharacter")} </Button>
117-197: Consider improvements to the debug display component.While the component effectively shows debug information, there are several opportunities for enhancement:
- Extract repeated accordion patterns into a reusable component:
interface DebugAccordionProps { title: string; formulas: Formula[]; calc: CalcType; } function DebugAccordion({ title, formulas, calc }: DebugAccordionProps) { return ( <Accordion> <AccordionSummary expandIcon={<ExpandMoreIcon />}> {title} </AccordionSummary> <AccordionDetails> <Stack> {calc?.listFormulas(formulas).map((read, index) => { const computed = calc.compute(read); const name = read.tag.name || read.tag.q; return ( <DebugItem key={`${name}${index}`} name={name} computed={computed} calc={calc} /> ); })} </Stack> </AccordionDetails> </Accordion> ); }
- Use a proper JSON viewer component instead of raw JSON.stringify for better readability
- Add translations for strings like "All target listings", "All target buffs", "debug for", etc.
- Consider adding copy-to-clipboard functionality for debug data
libs/sr/formula/src/data/util/tag.ts (1)
Line range hint
229-234: Consider enhancing documentation for thesharedparameter.The
sharedparameter's behavior significantly impacts tag generation, but its purpose and effects aren't immediately clear from the code. Consider adding JSDoc comments explaining:
- The purpose of the
sharedparameter- The difference between each possible value ('both', 'src', 'dst', 'none')
- The impact on the generated conditional tags
Add documentation like this:
function allConditionals<T>( sheet: Sheet, + /** + * Controls how source/destination tags are handled: + * - 'both': Removes both src and dst tags + * - 'src': Removes only src tag + * - 'dst': Removes only dst tag + * - 'none': Preserves both tags (default) + */ shared: CondIgnored = 'none', meta: IBaseConditionalData, transform: (r: Read, q: string) => T ): Record<string, T> {libs/sr/formula-ui/src/char/sheets/RuanMei.tsx (5)
5-5: LGTM! Consider adding JSDoc comments.The Tag import and selfTag constant align well with the PR objectives for targeted buff conditionals. Consider adding JSDoc comments to document the purpose and usage of the selfTag constant.
+/** Tag used for self-targeting abilities and buffs */ const selfTag: Tag = { src: key, dst: key }Also applies to: 24-24
Line range hint
83-99: Address TODO comments in skill conditionals.Several TODO comments need attention:
- Missing translation for "skill active"
- Missing translation for "Ally DMG Increase"
- Missing translation for "Duration"
Would you like me to help create a GitHub issue to track these translation tasks?
Line range hint
149-156: Address critical TODO items in ultimate ability.Several important features are missing:
- Action delay percentage on Thanatoplum Rebloom trigger
- Damage multiplier increase from e6
- Zone duration increase from e6
Would you like me to help implement these missing features or create a GitHub issue to track them?
Line range hint
195-214: Improve document organization for talent ability.The comment "Move this to proper document" suggests that the bonus ability fields are temporarily placed here. Consider:
- Identifying the proper location for these fields
- Creating a separate component if needed
- Updating the documentation accordingly
Would you like assistance in reorganizing these sections?
Line range hint
1-400: Create tracking issues for remaining TODO items.Several TODO items need to be addressed throughout the file:
- Translations needed for:
- "defIgn_" field
- Various ability and effect names
- Feature implementations needed for:
- Break Effect translations
- Duration translations
- Various buff and effect calculations
Consider creating separate issues for translations and feature implementations to track them effectively.
Would you like me to help create GitHub issues to track these items systematically?
libs/sr/page-team/src/TalentContent.tsx (1)
Line range hint
334-371: Consider the following improvements to the EidolonDropdown component.
- Clean up commented-out buildTc code to improve readability
- Add prop validation for the eidolon value
- Optimize MenuItem generation using useMemo
Here's a suggested implementation:
-export function EidolonDropdown({ eidolon }: { eidolon: number }) { +type EidolonDropdownProps = { + eidolon: number +} + +export function EidolonDropdown({ eidolon }: EidolonDropdownProps) { + if (eidolon < 0 || eidolon > maxEidolonCount) { + console.warn(`Invalid eidolon value: ${eidolon}`) + } + const { t } = useTranslation('characters_gen') const { teammateDatum: { characterKey }, } = useTeamContext() const { database } = useDatabaseContext() + + const menuItems = useMemo(() => + range(0, maxEidolonCount).map((i) => ( + <MenuItem + key={i} + selected={eidolon === i} + disabled={eidolon === i} + onClick={() => + database.chars.set(characterKey, { + eidolon: i, + }) + } + > + <span> + {t('eidolonLvl')} {i} + </span> + </MenuItem> + )), [eidolon, characterKey, t] + ) + return ( <DropdownButton fullWidth title={<span>{t('eidolonLvl')} {eidolon}</span>} color="primary" - // sx={{ color: buildTc?.character ? 'yellow' : undefined }} > - {range(0, maxEidolonCount).map((i) => ( - <MenuItem - key={i} - selected={eidolon === i} - disabled={eidolon === i} - onClick={() => - // buildTc?.character - // ? setBuildTc((buildTc) => { - // if (buildTc.character) buildTc.character.constellation = i - // }) - // : - database.chars.set(characterKey, { - eidolon: i, - }) - } - > - <span> - {t('eidolonLvl')} {i} - </span> - </MenuItem> - ))} + {menuItems} </DropdownButton> ) }libs/pando/ui-sheet/src/components/DocumentDisplay.tsx (1)
34-34: Remove commented-out props in 'ConditionalsDisplay' for code cleanlinessThe commented-out props
hideDesc,hideHeader, anddisabledare no longer in use. Removing these lines will improve code readability and maintainability.Apply this diff to clean up the code:
<ConditionalsDisplay conditional={document.conditional} - // hideDesc={hideDesc} - // hideHeader={hideHeader} - // disabled={disabled} bgt={bgt} />libs/pando/ui-sheet/src/components/FieldDisplay.tsx (2)
Line range hint
123-128: Use theme palette colors instead of hardcoded 'red' inboxShadowTo maintain consistency and theming support across the application, replace the hardcoded
'red'color with a color from the theme palette.You can modify the
boxShadowstyle as follows:- boxShadow: emphasize ? '0px 0px 0px 2px red inset' : undefined, + boxShadow: emphasize + ? (theme) => `0px 0px 0px 2px ${theme.palette.primary.main} inset` + : undefined,Ensure that you adjust your code to access the theme within the
sxprop. This can be done by using a function syntax for thesxprop to gain access to the theme object.
Line range hint
115-116: Address theTODOcomments forvariantandfixedvariablesThe variables
variantandfixedare currently set withTODOcomments indicating that further implementation is needed. This may lead to runtime errors, especially sincevariantis used in theColorTextcomponent andfixedaffects value formatting.Would you like assistance in determining the appropriate values for
variantandfixedto ensure correct functionality and prevent potential errors?libs/sr/page-team/src/index.tsx (1)
205-216: Remove commented-out code to improve readabilityThe code block from lines 205 to 216 is commented out. If it is no longer needed, consider removing it to keep the codebase clean and improve readability.
Apply this diff to remove the commented-out code:
- // sx={(theme) => { - // const elementKey = characterKey && allStats.char[characterKey] - // if (!elementKey) return {} - // const hex = theme.palette[elementKey].main as string - // const color = hexToColor(hex) - // if (!color) return {} - // const rgba = colorToRgbaString(color, 0.1) - // return { - // background: `linear-gradient(to bottom, ${rgba} 0%, rgba(0,0,0,0)) 25%`, - // } - // }}libs/pando/ui-sheet/src/components/ConditionalDisplay.tsx (1)
233-233: Remove commented-out code for clarityThe commented-out line
// disabled={disabled}is unnecessary since thedisabledprop is already set below. Removing it improves code readability.Apply this diff to clean up:
onClick={() => setValue(+!value)} - // disabled={disabled} startIcon={value ? <CheckBoxIcon /> : <CheckBoxOutlineBlankIcon />}libs/sr/db/src/Database/DataManagers/TeamDataManager.ts (4)
184-184: Nitpick: Address the TODO Comment forsrc/dstHandlingA TODO comment on line 184 indicates that handling for
src/dstbeing'all'is pending. This needs to be implemented to ensure the conditionals function correctly when applied globally.Would you like assistance in implementing the handling for
'all'values insrcanddst? I can help provide a solution or open a GitHub issue to track this task.
414-416: Refactor Suggestion: Removeconsole.logStatementsThe
console.logstatements on lines 414-416 and 418 are likely used for debugging purposes. They should be removed or replaced with appropriate logging to avoid cluttering the console in production environments.Apply this diff to remove the debugging statements:
- console.log({ - condIndex, - }) - console.log('creating new conditional')Also applies to: 418-418
513-515: Refactor Suggestion: Avoid UsinganyingetConditionalFunctionIn the
getConditionalfunction (lines 513-515), castingconditionalsasanyundermines TypeScript's type safety. Consider properly typingconditionalsto enhance code reliability and maintain the benefits of strong typing.You could define the type for
conditionalsto reflect its actual structure, for example:import { conditionals } from '@genshin-optimizer/sr/formula' // Then the function can use the proper type export function getConditional(sheet: Sheet, condKey: string) { return conditionals[sheet]?.[condKey] }
516-535: Refactor Suggestion: Handle Unrecognized Conditional TypesThe
correctConditionalValuefunction currently handles'bool','num', and'list'conditional types. If an unrecognized type is passed, the function silently returns the unmodified value. This could lead to unexpected behavior if new conditional types are introduced in the future.Consider adding an
elseclause to handle unrecognized types:} else if (conditional.type === 'list') { // existing code } + else { + console.warn(`Unhandled conditional type: ${conditional.type}`) + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (32)
apps/gi-frontend/src/app/teams/[teamId]/TalentContent.tsx(0 hunks)libs/common/database/src/lib/DataManagerBase.ts(2 hunks)libs/common/util/src/lib/array.ts(1 hunks)libs/pando/ui-sheet/src/components/ConditionalDisplay.tsx(1 hunks)libs/pando/ui-sheet/src/components/DocumentDisplay.tsx(3 hunks)libs/pando/ui-sheet/src/components/FieldDisplay.tsx(2 hunks)libs/pando/ui-sheet/src/components/HeaderDisplay.tsx(1 hunks)libs/pando/ui-sheet/src/components/index.ts(1 hunks)libs/pando/ui-sheet/src/context/TagContext.ts(1 hunks)libs/pando/ui-sheet/src/context/index.ts(1 hunks)libs/pando/ui-sheet/src/types/conditional.ts(1 hunks)libs/sr/db/src/Database/DataManagers/TeamDataManager.ts(8 hunks)libs/sr/formula-ui/src/char/sheets/RuanMei.tsx(6 hunks)libs/sr/formula-ui/src/index.ts(1 hunks)libs/sr/formula-ui/src/relic/cavern/WatchmakerMasterOfDreamMachinations.tsx(3 hunks)libs/sr/formula-ui/src/relic/cavern/index.ts(0 hunks)libs/sr/formula-ui/src/relic/index.ts(1 hunks)libs/sr/formula/src/data/relic/sheets/WatchmakerMasterOfDreamMachinations.ts(2 hunks)libs/sr/formula/src/data/util/tag.ts(1 hunks)libs/sr/page-team/src/ComboEditor.tsx(2 hunks)libs/sr/page-team/src/RelicSheetDisplay.tsx(1 hunks)libs/sr/page-team/src/RelicSheetsDisplay.tsx(1 hunks)libs/sr/page-team/src/TalentContent.tsx(4 hunks)libs/sr/page-team/src/TeamCalcProvider.tsx(5 hunks)libs/sr/page-team/src/TeammateDisplay.tsx(2 hunks)libs/sr/page-team/src/index.tsx(3 hunks)libs/sr/ui/src/Character/CharacterCard.tsx(4 hunks)libs/sr/ui/src/Character/CharacterTrans.tsx(1 hunks)libs/sr/ui/src/Character/index.tsx(1 hunks)libs/sr/ui/src/Hook/useSrCalcContext.ts(1 hunks)libs/sr/ui/src/Relic/index.tsx(1 hunks)libs/sr/util/src/relic.ts(2 hunks)
💤 Files with no reviewable changes (2)
- apps/gi-frontend/src/app/teams/[teamId]/TalentContent.tsx
- libs/sr/formula-ui/src/relic/cavern/index.ts
✅ Files skipped from review due to trivial changes (2)
- libs/pando/ui-sheet/src/context/TagContext.ts
- libs/sr/formula-ui/src/index.ts
🧰 Additional context used
🪛 Biome
libs/common/database/src/lib/DataManagerBase.ts
[error] 78-78: void is confusing inside a union type.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
libs/pando/ui-sheet/src/components/ConditionalDisplay.tsx
[error] 244-244: Useless rename.
Safe fix: Remove the renaming.
(lint/complexity/noUselessRename)
🔇 Additional comments (48)
libs/pando/ui-sheet/src/context/index.ts (1)
1-2: LGTM! Clean and consistent export structure.
The addition of TagContext export follows the established pattern and aligns with the PR's objective of supporting targeted conditional values through src/dst context.
Let's verify the TagContext implementation:
✅ Verification successful
LGTM! TagContext implementation and usage are properly structured.
The verification confirms:
- TagContext is correctly implemented as a React context with the expected Tag type
- It's properly used in ConditionalDisplay for passing src/dst context to nested components
- The implementation follows React context best practices
- Usage is consistent with the PR's objective of supporting targeted conditional values
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify TagContext implementation and its consumers
# Test 1: Check if TagContext is properly implemented
echo "Checking TagContext implementation..."
fd TagContext.ts$ --exec cat {}
# Test 2: Find components using TagContext
echo "Finding components using TagContext..."
rg -l "useTagContext|TagContext.Provider"
Length of output: 497
Script:
#!/bin/bash
# Let's examine the actual usage of TagContext in the components
echo "Checking TagContext usage in ConditionalDisplay.tsx..."
cat libs/pando/ui-sheet/src/components/ConditionalDisplay.tsx
echo "\nChecking TagContext usage in sr/page-team/index.tsx..."
cat libs/sr/page-team/src/index.tsx
# Also check if there are any other files importing Tag type
echo "\nChecking other files importing Tag type..."
rg "import.*Tag.*from.*pando/engine"
Length of output: 19431
libs/sr/ui/src/Relic/index.tsx (1)
5-5: LGTM! Export addition maintains good code organization.
The new export for RelicTrans is well-placed, following the alphabetical ordering pattern of the barrel file, and aligns with the PR's objective of enhancing conditional value display functionality.
libs/sr/ui/src/Character/index.tsx (1)
6-6: LGTM! Clean addition of the CharacterTrans module export.
The new export maintains proper alphabetical ordering and follows the established pattern of module exports in this barrel file, supporting the modular architecture of the application.
libs/sr/ui/src/Hook/useSrCalcContext.ts (2)
1-1: LGTM! Clean import organization.
The imports are well-organized and the addition of TagContext aligns with the PR's objective of passing src/dst/frame information through TagContext.
Also applies to: 3-3
6-6: Verify if type assertion is necessary.
The type assertion as Calculator | null might be unnecessary if the CalcContext is properly typed. Consider using TypeScript's type inference if possible.
Let's check the context type definition:
✅ Verification successful
Type assertion is necessary and correctly used
The type assertion in useSrCalcContext is necessary because:
CalcContextis explicitly defined ascreateContext<Calculator | null>(null)inlibs/pando/ui-sheet/src/context/CalcContext.ts- The type assertion matches exactly with the context's type definition
- This pattern is consistently used across the codebase (e.g., in
useGiCalcContext)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check CalcContext type definition
# Expected: Find the type definition of CalcContext to verify if type assertion is needed
rg -t typescript "export.*CalcContext" "@genshin-optimizer/pando/ui-sheet"
Length of output: 109
Script:
#!/bin/bash
# Let's try searching for CalcContext in the entire codebase
rg "CalcContext" -A 3
# Also search for the context creation
ast-grep --pattern 'createContext<$_>($_)'
Length of output: 12448
libs/sr/ui/src/Character/CharacterTrans.tsx (2)
1-4: LGTM! Good practice with the 'use client' directive.
The directive is correctly placed with a helpful comment explaining its purpose for handling translation hydration differences.
11-11: Verify translation namespace and keys.
Let's ensure all character keys have corresponding translations in the "charNames_gen" namespace.
libs/pando/ui-sheet/src/types/conditional.ts (1)
13-13: LGTM! Type definition is safe and backward compatible.
The optional boolean property maintains compatibility with existing code while enabling the new targeted conditional functionality.
libs/pando/ui-sheet/src/components/HeaderDisplay.tsx (2)
1-4: LGTM! Well-organized imports and correct Next.js directive placement.
The imports are properly organized and the 'use client' directive is correctly placed at the top of the file.
9-12: Verify the Document type usage in hideDivider prop.
The hideDivider function parameter is typed as Document, but it seems this might be incorrect since the component is rendering a header section. Should this be Header instead?
✅ Verification successful
Let me analyze the component's type definitions and usages to verify the correct type for the hideDivider prop's function parameter.
I need to see the implementation details to understand how the hideDivider function is actually used with the Document type.
The Document type usage in hideDivider prop is correct
Looking at the implementation and type definitions:
- The
Documenttype is correctly used as it represents the document structure that contains the header being displayed - The
hideDividerfunction is used to conditionally hide the divider based on the document's structure - This is evidenced by usage in
DocumentDisplay.tsxwherehideDivideris used with document fields length check
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other usages of HeaderDisplay to verify the correct type
rg -t typescript -t javascript "HeaderDisplay.*hideDivider"
Length of output: 96
Script:
#!/bin/bash
# Search for HeaderDisplay component usages with ripgrep
rg "HeaderDisplay" -A 5
# Search for Document type definition and usage
ast-grep --pattern 'type Document = $_'
# Search for Header type definition
ast-grep --pattern 'type Header = $_'
Length of output: 10739
Script:
#!/bin/bash
# Let's look at the actual implementation to see how hideDivider is used
cat libs/pando/ui-sheet/src/components/HeaderDisplay.tsx
# Also check the Document type definition file for its structure
cat libs/pando/ui-sheet/src/types/document.ts
Length of output: 1317
libs/sr/page-team/src/RelicSheetsDisplay.tsx (2)
1-4: LGTM! Imports are well-organized and necessary.
The imports follow a logical organization pattern and all imported items are utilized in the component.
13-15: Consider adding error handling for RelicSheetDisplay.
The component should handle potential errors from RelicSheetDisplay to prevent the entire UI from breaking.
Consider wrapping the RelicSheetDisplay with an error boundary:
import { ErrorBoundary } from 'react-error-boundary';
function ErrorFallback({ error }: { error: Error }) {
return (
<div role="alert">
<p>Something went wrong:</p>
<pre>{error.message}</pre>
</div>
);
}
// Usage in Grid item
<ErrorBoundary FallbackComponent={ErrorFallback}>
<RelicSheetDisplay setKey={setKey} />
</ErrorBoundary>libs/sr/formula/src/data/relic/sheets/WatchmakerMasterOfDreamMachinations.ts (2)
9-9: LGTM: Successful transition from individual to team-wide buffs.
The change from ownBuff to teamBuff correctly implements the PR's objective of enhancing targeted conditional functionality, allowing buffs to affect the entire team when a character with the 4-piece set triggers an ultimate ability on an ally.
Also applies to: 29-29
28-31: Verify the buff registration logic.
The implementation looks correct, but let's ensure the buff registration behaves as expected across different team compositions.
Let's verify other similar buff implementations:
✅ Verification successful
The buff registration logic is correctly implemented
The implementation follows the established pattern for relic buff registration in the codebase:
- It properly uses
registerBuffwith a unique identifier 'set4_brEffect_' - The buff is correctly applied to the team using
teamBuff.premod - The conditional logic using
useUltimateOnAlly.ifOnandcmpGE(relicCount, 4, ...)matches the expected 4-piece set behavior - The implementation aligns with the UI metadata in
WatchmakerMasterOfDreamMachinations.tsx
The verification confirms that the buff registration behaves consistently with other relic implementations in the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other buff registrations to ensure consistent patterns
rg -A 5 "registerBuff.*teamBuff" --type typescript
# Look for potential edge cases in test files
rg -A 5 "WatchmakerMasterOfDreamMachinations.*test" --type typescript
Length of output: 194
Script:
#!/bin/bash
# Search for other buff registrations without file type restriction
rg -A 5 "registerBuff.*teamBuff"
# Look for test files related to Watchmaker
fd "Watchmaker.*test"
# Search for similar buff effect patterns
rg -A 5 "brEffect_.*teamBuff"
# Look for similar conditional buff applications
rg -A 5 "ifOn.*cmpGE.*relicCount"
Length of output: 21156
Script:
#!/bin/bash
# Search for similar brEffect_ patterns
rg -A 5 "brEffect_.*teamBuff"
# Look for useUltimateOnAlly patterns
rg -A 5 "useUltimateOnAlly"
# Search for similar buff registration patterns in the same file
rg -A 10 "registerBuff" "libs/sr/formula/src/data/relic/sheets/WatchmakerMasterOfDreamMachinations.ts"
Length of output: 2791
libs/sr/formula-ui/src/relic/cavern/WatchmakerMasterOfDreamMachinations.tsx (3)
8-8: LGTM: Import addition aligns with new asset handling approach
The addition of getDefaultRelicSlot import supports the new dynamic relic slot determination.
47-47: LGTM: Targeted property addition enhances conditional behavior
The addition of targeted: true aligns with the PR's objective of supporting distinct values based on source and destination parameters for buff conditionals.
13-13: Verify default slot determination for this relic set
The dynamic slot determination is a good improvement. Let's verify that getDefaultRelicSlot returns the correct slot for this specific relic set.
✅ Verification successful
Based on the search results, I can now provide a definitive response about the default slot verification:
Default slot determination is correctly implemented
The implementation of getDefaultRelicSlot in libs/sr/util/src/relic.ts shows that for cavern relics like WatchmakerMasterOfDreamMachinations, it returns the first slot from allRelicCavernSlotKeys. This is correct because:
WatchmakerMasterOfDreamMachinationsis confirmed to be a cavern relic set (found inRelicSetConfig_keys_cavern_gen.json)- The relic has all standard cavern slots: head, hands, body, feet (verified in
AssetsData_gen.json) - The function correctly identifies cavern relics and returns their default slot
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the default slot configuration for this relic set
# Search for the default slot configuration for WatchmakerMasterOfDreamMachinations
rg -A 5 "WatchmakerMasterOfDreamMachinations.*getDefaultRelicSlot"
Length of output: 68
Script:
#!/bin/bash
# Let's try a broader search to understand the relic slot configuration
# Search for getDefaultRelicSlot implementation
ast-grep --pattern 'function getDefaultRelicSlot($_) {
$$$
}'
# Search for any references to WatchmakerMasterOfDreamMachinations
rg "WatchmakerMasterOfDreamMachinations" -A 5 -B 5
# Search for relic slot configurations or mappings
rg "relicSlot|RELIC_SLOT" -A 3
Length of output: 39817
libs/sr/page-team/src/RelicSheetDisplay.tsx (2)
1-16: LGTM! Imports are well-organized and properly used.
The imports are cleanly organized, properly typed with TypeScript, and all dependencies are utilized within the components.
18-50: Verify component usage and integration.
The component seems well-structured, but let's verify its integration with the system.
✅ Verification successful
Based on the search results, I can now generate the final response as we have sufficient information about the component's integration and usage:
Component integration verified successfully
The RelicSheetDisplay component is properly integrated with the system:
- Correctly imports and uses
relicAsset,getDefaultRelicSlot, andisCavernRelicutilities from their respective packages - Used appropriately in
RelicSheetsDisplaywith the requiredsetKeyprop - Properly integrates with
relicUiSheetsfrom the formula-ui package - Helper functions are used consistently across the codebase (RelicCard, WatchmakerMasterOfDreamMachinations)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for proper component usage and potential prop type mismatches
rg -A 2 "RelicSheetDisplay" --type typescript
# Verify relicUiSheets integration
ast-grep --pattern 'const relicSheet = relicUiSheets[$_]'
Length of output: 230
Script:
#!/bin/bash
# Check for proper component usage with correct file type
rg -A 2 "RelicSheetDisplay" --type tsx
# Check imports and usage of relicUiSheets
rg "relicUiSheets" --type tsx -B 2
# Check type definition of RelicSetKey
rg "RelicSetKey" --type ts --type tsx -B 2
# Check usage of helper functions
rg "isCavernRelic|getDefaultRelicSlot|relicAsset" --type tsx -B 2
Length of output: 296
Script:
#!/bin/bash
# Search for component usage without file type restriction
rg "RelicSheetDisplay" -A 2
# Check imports and usage of relicUiSheets
rg "relicUiSheets" -B 2
# Check type definition of RelicSetKey
rg "RelicSetKey"
# Find files with .tsx extension to confirm file types
fd -e tsx
# Check usage of helper functions
rg "isCavernRelic|getDefaultRelicSlot|relicAsset"
Length of output: 74328
libs/sr/ui/src/Character/CharacterCard.tsx (3)
3-3: LGTM: Clean import additions
The new imports for CharacterKey type and CharacterName component are well-organized and properly typed.
Also applies to: 20-20
52-54: LGTM: Improved internationalization and prop handling
Good improvements:
- Using
CharacterNamecomponent for consistent character name display and better i18n support - Properly passing
characterKeytoStatLinefor targeted conditional handling
Also applies to: 57-59, 66-71
98-103: Verify the src/dst tag implementation
The implementation sets both src and dst tags to the same characterKey. Please verify if this is the intended behavior, as the PR objectives mention distinct values based on source and destination parameters.
✅ Verification successful
Based on the search results, I can see that withTag is used extensively in both the Star Rail (sr) and Genshin Impact (gi) formula systems. Looking at the usage patterns:
- In team-based calculations, when
srcanddstare different, it typically represents buff/debuff calculations between characters:
// From libs/sr/formula/src/util.ts
entry.reread(teamBuff.withTag({ dst, src, name: null }))- When
srcanddstare the same (like in the review case), it's used for self-referential calculations:
// From libs/sr/formula/src/util.ts
reader.withTag({ et: 'own', dst: null, src: dst })The implementation in CharacterCard.tsx setting both src and dst to the same characterKey appears to be correct as it's calculating a character's own stats. This aligns with the codebase patterns where self-referential calculations use the same value for both tags.
Setting same source and destination is correct for self-stats
The current implementation correctly sets both src and dst to the same characterKey as it's calculating the character's own stats, not buffs/debuffs from other characters.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other withTag usages to compare implementation patterns
rg -A 3 "withTag\(" --type ts
Length of output: 19212
libs/sr/page-team/src/ComboEditor.tsx (2)
1-22: LGTM: Import organization and component structure.
The new imports are well-organized and properly aligned with the added functionality.
104-111: Verify context propagation for targeted conditionals.
While the UI structure looks good, please verify that the TagContext for src/dst parameters (mentioned in PR objectives) is properly propagated to the child components.
Let's verify the context implementation:
✅ Verification successful
The previous results show that TagContext is properly provided at the top level in libs/sr/page-team/src/index.tsx, but we need to verify how it's consumed in the new components mentioned in the review comment. Let's gather more information about these components and their context usage.
The previous results show that TagContext is properly defined and used in the codebase, but we need to verify the specific implementation of these new components to ensure they correctly handle the src/dst parameters. Let's check their implementation details.
Based on the gathered information, I can now provide the final response:
Context propagation for targeted conditionals is properly implemented.
The codebase shows that:
- TagContext is correctly provided at the top level in
libs/sr/page-team/src/index.tsx - BonusStats component properly uses src/dst parameters in its implementation
- The src/dst context is managed through
srcDstDisplayContextValueand properly propagated via context providers - The tag structure includes proper handling of src/dst parameters as shown in the
newTagfunction in BonusStats.tsx
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for TagContext implementation and usage
rg -A 5 "TagContext"
# Search for src/dst parameter handling
rg -A 5 "(src|dst).*parameter"
Length of output: 4962
Script:
#!/bin/bash
# Let's check the implementation of BonusStats and RelicConditionals
rg -l "BonusStats|RelicConditionals"
# Check for src/dst usage in these components
ast-grep --pattern 'const $_ = useContext(TagContext)'
# Check for any Tag type definitions
rg -A 5 "type Tag"
Length of output: 9277
Script:
#!/bin/bash
# Let's check the implementation of BonusStats and RelicConditionals components
cat libs/sr/page-team/src/BonusStats.tsx
cat libs/sr/page-team/src/RelicConditionals.tsx
# Check for any src/dst usage in the team page components
rg -A 5 "src|dst" libs/sr/page-team/src/
Length of output: 10609
libs/sr/page-team/src/TeamCalcProvider.tsx (4)
3-4: LGTM! Import cleanup looks good.
The import statements have been properly cleaned up to only include the types that are actually used in the code.
54-57: LGTM! Simplified member data spreading.
The direct spreading of member data makes the code more readable and maintainable compared to the previous conditional approach.
114-128: Verify RelicSubStatKey type assertion safety.
The code assumes the filter on empty keys ensures type safety for the RelicSubStatKey assertion. Let's verify this assumption by checking all possible substat key values in the codebase.
✅ Verification successful
Let me gather more information about the RelicSubStatKey type and its possible values.
Now I can generate the final response based on the verification of RelicSubStatKey type safety.
Type assertion for RelicSubStatKey is safe when filtering empty keys
The type assertion is safe because:
RelicSubStatKeyis defined as a union of literal string types fromallRelicSubStatKeysarray- The array contains all possible substat keys: 'hp', 'atk', 'def', 'hp_', 'atk_', 'def_', 'spd', 'crit_', 'crit_dmg_', 'eff_', 'eff_res_', 'brEffect_'
- The code filters out empty strings (
key !== '') before the type assertion - Empty string is not part of the
RelicSubStatKeytype, so filtering it out ensures the remaining keys are valid
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for RelicSubStatKey type definition and usage
ast-grep --pattern 'type RelicSubStatKey = $_'
Length of output: 5760
Script:
#!/bin/bash
# Get the content of the file containing RelicSubStatKey definition
cat libs/sr/consts/src/relic.ts
Length of output: 4085
67-72: Verify the conditional value handling logic.
The nested flatMap operations with conditional checks might impact performance with large datasets. Additionally, ensure that the preset naming convention using string literals (preset${frameIndex}) is consistently used across the codebase.
✅ Verification successful
The previous command failed due to file type specification. Let's try these alternative approaches to verify both the preset naming convention and analyze the conditional logic implementation.
Preset naming convention is consistent, but consider performance optimizations
The verification shows that the preset naming convention (preset${frameIndex}) is consistent with the codebase's predefined preset values (preset0 through preset9) in both sr and gi formula utilities. The type Preset is properly defined and used across the codebase.
Regarding performance with nested flatMap operations:
- The conditional logic in
TeamCalcProvider.tsxuses multiple nestedflatMapoperations which could impact performance with large datasets - Consider extracting the nested operations into a separate memoized function to prevent unnecessary recalculations
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other preset string literal usages to verify consistency
rg "preset\$\{.*\}" --type typescript
Length of output: 73
Script:
#!/bin/bash
# Search for preset string literal usages with ripgrep
rg "preset\d+" -A 2 -B 2
# Search for template literal usage of preset
rg "preset\$\{" -A 2 -B 2
# Search for withPreset function usage
rg "withPreset\(" -A 2 -B 2
# Look for preset type or enum definitions
ast-grep --pattern 'type Preset = $_'
Length of output: 8827
libs/common/database/src/lib/DataManagerBase.ts (1)
Line range hint 78-88: Verify handling of false returns by callers.
Let's verify that callers properly handle the new failure case where set returns false.
✅ Verification successful
Based on the extensive search results, I can now provide a definitive response about the handling of set() return values:
The false return value from set() is properly handled throughout the codebase
The code analysis reveals:
-
The
set()method'sfalsereturn is consistently used as a validation check, particularly in database operations where:- Invalid data is rejected (
if (!this.set(...)) return false) - Storage cleanup is triggered (
if (!this.set(key, {}) this.database.storage.remove(key)) - Cascading updates are prevented for invalid data
- Invalid data is rejected (
-
The implementation in DataManagerBase.ts shows that
falsereturns are specifically designed to indicate validation failures and are propagated correctly. -
All major callers (UI components, data managers, migrations) properly handle the failure cases by either:
- Checking return values in conditional statements
- Using early returns
- Chaining operations only after successful sets
The code demonstrates robust error handling patterns around set() operations, making the new failure case safe and well-integrated into the existing validation flow.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for calls to set() to ensure proper error handling
rg -A 3 "\.set\(" | grep -v "DataManagerBase" | grep -B 1 -A 2 "if.*set.*\)|set.*\{"
Length of output: 108
Script:
#!/bin/bash
# Search for calls to set() method with better pattern matching
rg -A 3 "\.set\(" --type typescript | grep -v "DataManagerBase"
Length of output: 98
Script:
#!/bin/bash
# Search for specific usage of set() with false handling
ast-grep --pattern 'if ($_ = $_.set($_)) {
$$$
}'
Length of output: 53
Script:
#!/bin/bash
# Search for set() method calls with conditional checks
ast-grep --pattern 'if ($_.set($_)) {
$$$
}'
Length of output: 48
Script:
#!/bin/bash
# Search for direct assignments of set() results
ast-grep --pattern '$_ = $_.set($_)'
Length of output: 38
Script:
#!/bin/bash
# Search for calls to set() method
rg -A 3 "\.set\(" --type ts
Length of output: 128917
Script:
#!/bin/bash
# Find all TypeScript files that might use set()
rg --files-with-matches "\.set\(" --type ts
Length of output: 6544
Script:
#!/bin/bash
# Look for conditional checks around set() calls
rg "if.*\.set\(|\.set\(.*\{" --type ts -A 2
Length of output: 78779
🧰 Tools
🪛 Biome
[error] 78-78: void is confusing inside a union type.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
libs/sr/util/src/relic.ts (2)
8-13: LGTM: Import changes are well-organized.
The new type imports are properly structured and necessary for the added functionality.
174-186: Verify integration with existing relic handling code.
The new functions appear to be part of a larger change involving relic handling. Let's verify their integration:
✅ Verification successful
Based on the shell script results, I can now generate the final response:
Integration verified - functions are properly used in the codebase
The new functions are well-integrated:
getDefaultRelicSlotis used inRelicSheetDisplay.tsxfor image display and inWatchmakerMasterOfDreamMachinations.tsxfor icon assetsisCavernRelicis used inRelicSheetDisplay.tsxfor badge labeling to distinguish between 'Cavern' and 'Planar' relics
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for usages of the new functions
echo "Searching for getDefaultRelicSlot usage:"
rg "getDefaultRelicSlot" -A 2
echo -e "\nSearching for isCavernRelic usage:"
rg "isCavernRelic" -A 2
# Check for any remaining TODOs related to relic handling
echo -e "\nChecking for related TODOs:"
rg "TODO.*relic" -A 2
Length of output: 4893
libs/sr/page-team/src/TeammateDisplay.tsx (1)
128-154:
Improve error handling in debug calculations.
The mapping operations don't handle potential null/undefined cases from calc?.listFormulas.
Let's verify the calc context usage:
Add null checks and error boundaries:
-{calc?.listFormulas(own.listing.formulas).map((read, index) => {
+{calc?.listFormulas(own.listing.formulas)?.map((read, index) => {
const computed = calc.compute(read)
const name = read.tag.name || read.tag.q
return (Also applies to: 164-190
libs/sr/formula/src/data/util/tag.ts (2)
Line range hint 235-245: Implementation looks good!
The function's implementation is well-structured with proper type safety and clear handling of tag generation. The change to the default parameter integrates well with the existing logic.
230-230: Verify the impact of changing the default shared parameter.
The change from 'src' to 'none' means that by default, both source and destination tags will be preserved in conditional tags. This aligns with the PR's objective of supporting targeted buff conditionals but could affect existing code that relies on the previous default behavior.
Let's verify the usage of allConditionals across the codebase:
✅ Verification successful
Change in shared parameter default value is safe and intentional
The codebase analysis reveals that:
- All direct calls to
allConditionalsare within the same file (tag.ts) and its counterpart in thegilibrary. - The change from
'src'to'none'is consistent with the usage pattern in both libraries. - The function is used internally by higher-level utility functions (
allBoolConditionals,allListConditionals, etc.) which explicitly specify their desiredsharedparameter value.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for calls to allConditionals to identify potential breaking changes
ast-grep --pattern 'allConditionals($$$)'
# Also search for any direct string references to these values
rg -A 2 "'src'|'dst'|'none'|'both'" --type ts
Length of output: 15458
libs/sr/formula-ui/src/char/sheets/RuanMei.tsx (4)
43-43: LGTM! Consistent implementation of tagged calculations.
The modification to use withTag(selfTag) ensures proper targeting for basic ability calculations, aligning with the PR's objectives.
75-75: LGTM! Consistent implementation of tagged calculations.
The modification to use withTag(selfTag) ensures proper targeting for skill calculations, aligning with the PR's objectives.
124-124: LGTM! Consistent implementation of tagged calculations.
The modification to use withTag(selfTag) ensures proper targeting for ultimate calculations, aligning with the PR's objectives.
179-179: LGTM! Consistent implementation of tagged calculations.
The modification to use withTag(selfTag) ensures proper targeting for talent calculations, aligning with the PR's objectives.
libs/sr/page-team/src/TalentContent.tsx (2)
34-34: LGTM: Import statements are correctly added and utilized.
The new imports are properly used throughout the codebase:
useCallbackanduseMemohooks are used for performance optimizationuseTeamContextis used to access team-related data
Also applies to: 36-36
129-129: LGTM: EidolonDropdown is properly integrated with consistent prop passing.
The component is correctly implemented in both responsive layouts (large and small screens) with consistent eidolon value handling.
Also applies to: 190-190
libs/pando/ui-sheet/src/components/DocumentDisplay.tsx (5)
3-3: Import 'CardThemed' is appropriate
The import of CardThemed from @genshin-optimizer/common/ui is necessary for the FieldsSectionDisplay component.
6-6: Imports 'Box' and 'Collapse' are correctly added
The addition of Box and Collapse from @mui/material is appropriate for use in the TextSectionDisplayCollapse component.
9-9: Importing types from '../types' is correct
Importing Document, FieldsDocument, and TextDocument types ensures proper type definitions within the component.
12-12: Import 'HeaderDisplay' is appropriate
The import of HeaderDisplay from ./HeaderDisplay is necessary for rendering headers in the FieldsSectionDisplay component.
10-10: Verify the export of 'ConditionalsDisplay' from './ConditionalDisplay'
Ensure that the ConditionalsDisplay component is properly exported from ./ConditionalDisplay. If the component was previously named ConditionalDisplay, confirm that all references have been updated consistently to prevent import errors.
Run the following script to verify the export:
✅ Verification successful
Let me try a different search strategy to locate the file and verify the export.
The import is correct and matches the exported component
The verification shows that ConditionalsDisplay is properly exported as a named export from ./ConditionalDisplay.tsx in the same directory. The component is:
- Exported in
libs/pando/ui-sheet/src/components/ConditionalDisplay.tsx - Correctly imported in
DocumentDisplay.tsx - Used within the
DocumentDisplaycomponent
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that 'ConditionalsDisplay' is exported from './ConditionalDisplay'
# Test: Search for the export of 'ConditionalsDisplay' in the codebase. Expect: An export statement for 'ConditionalsDisplay'.
ast-grep --pattern 'export function ConditionalsDisplay($$) { $$$ }'
# Alternatively, check for named exports
ast-grep --pattern 'export { ConditionalsDisplay }'
# Or for default exports
ast-grep --pattern 'export default ConditionalsDisplay'
Length of output: 2668
Script:
#!/bin/bash
# Find the ConditionalDisplay file
fd ConditionalDisplay
# Search for any mentions of ConditionalsDisplay in the codebase
rg "ConditionalsDisplay" -A 2
# Check if there might be a typo in the component name
rg "Conditional.*Display"
Length of output: 2169
libs/pando/ui-sheet/src/components/ConditionalDisplay.tsx (1)
130-188: Well-structured 'ConditionalDisplay' component
The ConditionalDisplay component efficiently handles different conditional types and manages state correctly. The use of useMemo and context providers optimizes performance and maintains clean code architecture.
libs/sr/db/src/Database/DataManagers/TeamDataManager.ts (2)
406-406: Verify Boundary Condition for frameIndex
On line 406, the condition if (frameIndex > team.frames.length) return false may allow frameIndex to be equal to team.frames.length, which could lead to out-of-bounds errors when accessing arrays. Ensure that frameIndex should not equal team.frames.length if that's unintended.
To verify the potential issue, consider adjusting the condition:
- if (frameIndex > team.frames.length) return false
+ if (frameIndex >= team.frames.length) return false178-203: Verify Logic for Removing Unused Conditionals
In the validation filter for conditionals (lines 178-203), conditionals are removed if all their condValues are falsy (line 198). Ensure that this logic aligns with the desired behavior, especially if a conditional being inactive in all frames should still be retained for future use.
If the intent is to keep all conditionals regardless of their values, consider removing the check on line 198.
- if (condValues.every((v) => !v)) return false
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (8)
libs/sr/page-team/src/index.tsx (3)
2-14: LGTM! Consider grouping related imports.The imports are well-organized but could be grouped better. Consider grouping related imports together:
- External libraries (@mui, react, react-router)
- Common utilities
- Pando UI components
- SR (Star Rail) specific imports
Also applies to: 21-22
203-214: Remove or document commented code.The commented theme-related code should either be:
- Removed if it's no longer needed
- Documented with a TODO comment explaining why it's kept
174-227: Consider simplifying context provider nesting.The current implementation has deep nesting of context providers which could make the code harder to maintain.
Consider:
- Creating a composite provider component
- Using context composition pattern
Example:
const TeamProviders: React.FC<{ children: React.ReactNode }> = ({ children }) => { // ... context value calculations ... return ( <TagContext.Provider value={tag}> <PresetContext.Provider value={presetObj}> {/* ... other providers ... */} </PresetContext.Provider> </TagContext.Provider> ) } // Usage return ( <TeamProviders> <Box> {/* ... content ... */} </Box> </TeamProviders> )libs/pando/ui-sheet/src/components/ConditionalDisplay.tsx (5)
65-78: Consider optimizing memoization dependenciesThe
filteredConditionalsmemo andhasExistingcallback could be optimized:
filteredConditionalscould destructure needed properties from conditionals to avoid unnecessary rerendershasExistingcould be memoized with the filtered list instead of recomputing on every renderConsider applying these optimizations:
const filteredConditionals = useMemo( () => conditionals.filter( - ({ condValue, sheet: s, condKey }) => - condValue && s === sheet && condKey === name + ({ condValue, sheet: s, condKey }) => { + if (!condValue) return false; + return s === sheet && condKey === name; + } ), [conditionals, sheet, name] ) - const hasExisting = useCallback( + const hasExisting = useMemo( + () => (src: string, dst: string) => filteredConditionals.some(({ src: s, dst: d }) => s === src && d === dst), - [filteredConditionals] + [filteredConditionals] )
127-129: Enhance the default SetConditionalContext warning messageThe current warning message could be more descriptive to help developers understand what's missing.
Consider updating the warning message:
export const SetConditionalContext = createContext<SetConditionalFunc>(() => - console.warn('SetConditional NOT IMPLEMENTED') + console.warn('SetConditionalContext: No provider found. Please ensure SetConditionalContext.Provider is present in the component tree.') )
130-150: Add prop validation for required propertiesConsider adding runtime validation for required props to catch potential issues early.
Add validation at the beginning of the component:
const ConditionalDisplay = memo(function ConditionalDisplay({ conditional, src, setSrc, dst, setDst, value, setValue, bgt = 'normal', disabled, }: { conditional: Conditional src: string setSrc?: (src: string) => void dst: string setDst?: (dst: string) => void value: number setValue: (value: number) => void bgt?: CardBackgroundColor disabled?: boolean }) { + if (!conditional) { + throw new Error('ConditionalDisplay: conditional prop is required'); + } + if (typeof src !== 'string') { + throw new Error('ConditionalDisplay: src prop must be a string'); + } + if (typeof dst !== 'string') { + throw new Error('ConditionalDisplay: dst prop must be a string'); + }
333-334: Remove commented out onChange handlerThere's a commented out onChange handler that should be removed to maintain clean code.
Apply this change:
- // onChange={(_e, v) => setInnerValue(v as number)} onChangeCommitted={(_e, v) => setValue(v as number)}
341-350: Add cleanup for CondSlider stateConsider cleaning up the inner state when the component unmounts or when the props value changes.
Add an effect to sync the inner state with props:
function CondSlider(props: Omit<SliderProps, 'onChange'>) { const [innerValue, setInnerValue] = useState(props.value) + + useEffect(() => { + setInnerValue(props.value) + }, [props.value]) + return ( <Slider {...props} onChange={(_e, v) => setInnerValue(v as number)} value={innerValue} /> ) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
libs/pando/ui-sheet/src/components/ConditionalDisplay.tsx(1 hunks)libs/sr/db/src/Types/conditional.ts(0 hunks)libs/sr/db/src/Types/index.ts(0 hunks)libs/sr/formula/src/util.ts(0 hunks)libs/sr/page-team/src/RelicSheetsDisplay.tsx(1 hunks)libs/sr/page-team/src/index.tsx(3 hunks)
💤 Files with no reviewable changes (3)
- libs/sr/db/src/Types/conditional.ts
- libs/sr/db/src/Types/index.ts
- libs/sr/formula/src/util.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- libs/sr/page-team/src/RelicSheetsDisplay.tsx
🔇 Additional comments (2)
libs/sr/page-team/src/index.tsx (1)
165-172: LGTM! Tag implementation follows previous review recommendations.
The implementation correctly uses characterKey for both src and dst in the tag context.
libs/pando/ui-sheet/src/components/ConditionalDisplay.tsx (1)
1-48: LGTM! Well-structured context and type definitions.
The context definitions and types are clear, well-organized, and provide good type safety for the conditional values system.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (4)
libs/sr/page-team/src/TeammateDisplay.tsx (2)
44-66: Consider adding loading states and accessibility improvements.The Stack layout and component organization look good, but there are some potential improvements:
- The Edit Character button could benefit from an aria-label
- Consider adding loading states for async operations
- The disabled state logic could be more robust
<Button fullWidth disabled={!characterKey} + aria-label="Edit Character Details" + disabled={!characterKey || isLoading} onClick={() => characterKey && setCharacterKey(characterKey)} > Edit Character </Button>
128-154: Optimize calculation performance and improve error handling.The mapping functions that compute values could impact performance and need better error handling:
- Consider implementing calculation memoization
- Add error handling for calculations
- Implement pagination or virtualization if the lists become large
Example implementation:
const computeWithErrorHandling = (calc: any, read: any) => { try { return { computed: calc.compute(read), error: null } } catch (error) { return { computed: null, error: error instanceof Error ? error.message : 'Unknown error' } } }Also applies to: 164-190
libs/sr/page-team/src/index.tsx (2)
175-228: Consider reducing context nesting depthThe current implementation has 6 levels of nested context providers, which could make the code harder to maintain and impact performance. Consider extracting some of these providers into a separate component:
// Create a new component function ConditionalProviders({ children }) { // Move conditional-related providers here return ( <SrcDstDisplayContext.Provider value={srcDstDisplayContextValue}> <ConditionalValuesContext.Provider value={conditionals}> <SetConditionalContext.Provider value={setConditional}> {children} </SetConditionalContext.Provider> </ConditionalValuesContext.Provider> </SrcDstDisplayContext.Provider> ) } // Use in main component return ( <TagContext.Provider value={tag}> <PresetContext.Provider value={presetObj}> <TeamCalcProvider teamId={teamId}> <ConditionalProviders> {/* Rest of the JSX */} </ConditionalProviders> </TeamCalcProvider> </PresetContext.Provider> </TagContext.Provider> )
204-214: Remove commented-out styling codeThe commented-out styling code should either be implemented or removed to maintain code cleanliness.
- // sx={(theme) => { - // const elementKey = characterKey && allStats.char[characterKey] - // if (!elementKey) return {} - // const hex = theme.palette[elementKey].main as string - // const color = hexToColor(hex) - // if (!color) return {} - // const rgba = colorToRgbaString(color, 0.1) - // return { - // background: `linear-gradient(to bottom, ${rgba} 0%, rgba(0,0,0,0)) 25%`, - // } - // }}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (11)
libs/sr/formula/src/index.ts (1)
16-16: Consider documenting the new exports.Since this module is introducing new conditional utilities, please ensure that the exports from
conditionalUtilare properly documented to help other developers understand their purpose and usage.Would you like me to help generate JSDoc documentation for the new exports?
libs/sr/formula/src/data/util/listing.ts (1)
120-122: Consider strengthening type safety in the includes check.While the current implementation works, the type assertion in the includes check could be made more type-safe.
Consider this alternative implementation:
-export function isSheet(x: string): x is Sheet { - return sheets.includes(x as Sheet) +export function isSheet(x: string): x is Sheet { + return (sheets as readonly string[]).includes(x)This approach avoids the type assertion on the input parameter and instead safely widens the type of the constant array for the includes check.
libs/sr/formula/src/calculator.ts (2)
106-108: Consider improving object literal formatting.The nested object literal could be more readable with proper indentation.
- meta.conds = { - [dst]: { [src]: { [sheet!]: { [q!]: val } } }, - } + meta.conds = { + [dst]: { + [src]: { + [sheet!]: { + [q!]: val + } + } + } + }
Line range hint
14-108: Consider documenting the architectural changes.The changes to the conditional system (removing 'all' key and making targeting more precise) represent a significant architectural shift. Consider:
- Updating documentation to reflect the new targeted conditional system
- Adding examples of how to migrate from the old 'all' approach to the new targeted approach
- Documenting the structure of the new conditional data model (
dst -> src -> sheet -> value)libs/sr/page-team/src/index.tsx (4)
131-140: Consider adding null check before accessing characterKey propertiesThe
characterKeyis used directly in the display name generation without proper null checking. While this might work due to the filter, it's better to be explicit about the null check.const srcDstDisplayContextValue = useMemo(() => { const charList = team.teamMetadata .filter(notEmpty) .map(({ characterKey }) => characterKey) - if (characterKey) moveToFront(charList, characterKey) + if (characterKey && charList.includes(characterKey)) { + moveToFront(charList, characterKey) + } const charDisplay = objKeyMap(charList, (ck) => ( <CharacterName genderedKey={characterKeyToGenderedKey(ck)} /> )) return { srcDisplay: charDisplay, dstDisplay: charDisplay } }, [team.teamMetadata, characterKey])
164-172: Add error handling for database operationsThe database operation could fail silently. Consider adding error handling to provide feedback to the user.
database.teams.setConditional( teamId, sheet, condKey, src, dst, condValue, presetIndex - ) + ).catch(error => { + console.error('Failed to set conditional:', error) + // Consider showing a user-friendly error notification + })
214-224: Clean up or implement commented styling codeThere's a block of commented styling code that appears to be for element-based background gradients. Either implement this feature or remove the commented code to maintain cleaner codebase.
185-238: Consider breaking down nested context providersThe component has multiple nested context providers which could make it harder to maintain. Consider extracting these into a separate component for better organization.
// Example: Create a separate ContextProviders component function TeamContextProviders({ children, ...props }) { return ( <TagContext.Provider value={props.tag}> <PresetContext.Provider value={props.presetObj}> <TeamCalcProvider teamId={props.teamId}> {/* ... other providers */} </TeamCalcProvider> </PresetContext.Provider> </TagContext.Provider> ) }libs/sr/db/src/Database/DataManagers/TeamDataManager.ts (3)
179-202: Consider optimizing validation logicThe conditional validation logic is robust but could benefit from some optimizations:
- The hash list could be pre-allocated based on the conditionals array length
- The validation logic could be split into smaller, focused functions for better maintainability
Consider refactoring like this:
- const hashList: string[] = [] + const hashList = new Set<string>() + function validateConditionalUniqueness( + sheet: Sheet, + condKey: string, + src: Member, + dst: Member, + hashList: Set<string> + ): boolean { + const hash = `${sheet}:${condKey}:${src}:${dst}` + if (hashList.has(hash)) return false + hashList.add(hash) + return true + } conditionals = conditionals.filter( ({ sheet, condKey, src, dst, condValues }) => { if (!isMember(src) || !isMember(dst)) return false const cond = getConditional(sheet, condKey) if (!cond) return false - const hash = `${sheet}:${condKey}:${src}:${dst}` - if (hashList.includes(hash)) return false - hashList.push(hash) + if (!validateConditionalUniqueness(sheet, condKey, src, dst, hashList)) + return false if (!Array.isArray(condValues)) return false pruneOrPadArray(condValues, framesLength, 0) condValues = condValues.map((v) => correctConditionalValue(cond, v)) return !condValues.every((v) => !v) } )
413-415: Remove debug loggingThe console.log statement should be removed before merging to production.
- console.log({ - condIndex, - })
511-530: Enhance type safety and readabilityThe conditional value correction logic is comprehensive but could benefit from some improvements:
- Add type assertions for better type safety
- Use constants for magic numbers
- Consider adding input validation
+ const MINIMUM_LIST_INDEX = 0; function correctConditionalValue(conditional: IConditionalData, value: number) { + if (typeof value !== 'number') { + console.warn('Invalid value type provided to correctConditionalValue'); + value = 0; + } if (conditional.type === 'bool') { return +!!value } else if (conditional.type === 'num') { if (conditional.int_only && !Number.isInteger(value)) { value = Math.round(value) } if (conditional.min !== undefined && value < conditional.min) value = conditional.min if (conditional.max !== undefined && value > conditional.max) value = conditional.max } else if (conditional.type === 'list') { if (!Number.isInteger(value)) { value = Math.round(value) } - if (value < 0) value = 0 + if (value < MINIMUM_LIST_INDEX) value = MINIMUM_LIST_INDEX if (value > conditional.list.length - 1) value = conditional.list.length - 1 } return value }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
libs/sr/db/src/Database/DataManagers/TeamDataManager.ts(9 hunks)libs/sr/formula/src/calculator.ts(2 hunks)libs/sr/formula/src/conditionalUtil.ts(1 hunks)libs/sr/formula/src/data/util/listing.ts(1 hunks)libs/sr/formula/src/data/util/tag.ts(2 hunks)libs/sr/formula/src/index.ts(1 hunks)libs/sr/page-team/src/index.tsx(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- libs/sr/formula/src/data/util/tag.ts
🔇 Additional comments (10)
libs/sr/formula/src/conditionalUtil.ts (2)
1-3: LGTM!
Clean and well-organized imports with proper type imports.
5-7: Consider adding input validation.
The function silently returns undefined for invalid inputs. Consider whether explicit validation would be beneficial.
Let's check how this function is used in the codebase to determine if additional validation is needed:
Consider adding validation if the usage patterns show a need:
export function getConditional(sheet: Sheet, condKey: string) {
+ if (!sheet || !condKey) {
+ throw new Error('Sheet and condKey must be provided')
+ }
const sheetConditionals = conditionals[sheet as keyof typeof conditionals]
+ if (!sheetConditionals) {
+ throw new Error(`Invalid sheet: ${sheet}`)
+ }
return sheetConditionals?.[condKey as keyof typeof sheetConditionals] as IConditionalData | undefined
}libs/sr/formula/src/index.ts (1)
16-16: LGTM! Verify for naming conflicts.
The new export aligns with the module's organization and the PR's objectives for enhancing conditional value handling.
Let's check for potential naming conflicts with existing exports:
✅ Verification successful
LGTM! The export is safe and properly scoped
The new export of getConditional function from conditionalUtil.ts is safe because:
- It exports a single, well-defined utility function with clear purpose
- The function has proper type safety with TypeScript
- The export is properly re-exported through the module's main entry point (index.ts)
- No naming conflicts are found in the codebase
- The function follows the module's pattern of organizing exports
The change aligns with the PR's goal of enhancing conditional value handling functionality.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for potential naming conflicts between conditionalUtil exports and existing exports
# Get all exports from the current directory
rg -l "^export" .
# Check conditionalUtil exports
rg "^export" "./conditionalUtil.ts"
Length of output: 145939
Script:
#!/bin/bash
# Let's try to find the conditionalUtil.ts file relative to the formula directory
fd conditionalUtil.ts libs/sr/formula/src
# If found, let's check its exports
fd conditionalUtil.ts libs/sr/formula/src --exec rg "^export"
Length of output: 210
Script:
#!/bin/bash
# Let's check if there are any existing exports from conditionalUtil in other files
rg "from.*conditionalUtil" libs/sr/formula/src
# Let's also check the content of index.ts to see what it's re-exporting
cat libs/sr/formula/src/index.ts
Length of output: 1252
Script:
#!/bin/bash
# Let's check if there are any files importing from index.ts that use conditionalUtil
rg "from.*@genshin-optimizer/sr/formula.*conditionalUtil" .
# Also check the content of conditionalUtil.ts to understand its full scope
cat libs/sr/formula/src/conditionalUtil.ts
Length of output: 407
libs/sr/formula/src/data/util/listing.ts (2)
117-119: LGTM: Type guard implementation for member validation.
The isMember function correctly implements a type guard to ensure type safety when working with member strings.
117-122: Verify the usage of these type guards.
These type guards are crucial for type safety. Let's verify their usage in the codebase to ensure they're being used appropriately.
✅ Verification successful
Let me gather more information to check for any unguarded usages of Member or Sheet types.
Type guards are being used appropriately and safely
The verification shows that:
- The type guards are used correctly in conditional checks before accessing type-specific properties (e.g., in
libs/sr/page-team/src/index.tsxandlibs/sr/db/src/Database/DataManagers/TeamDataManager.ts) - Type assertions (
as Member/as Sheet) are only used within the type guard functions themselves - No unsafe type assertions were found in conditional statements
- Direct array usage of
membersandsheetsappears in safe contexts (mapping operations) and doesn't bypass the type system
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check usage of isMember and isSheet type guards
# Expected: Find calls to these functions in conditional checks
echo "Checking usage of isMember type guard:"
rg -A 2 "isMember\("
echo "Checking usage of isSheet type guard:"
rg -A 2 "isSheet\("
# Also check for potential places where these guards should be used
echo "Checking for potential unguarded member/sheet usage:"
ast-grep --pattern 'if ($x as Member)' # Look for unsafe type assertions
Length of output: 1622
Script:
#!/bin/bash
# Check for direct type assertions without guards
echo "Checking for 'as Member' assertions:"
rg "as Member" -B 2
echo "Checking for 'as Sheet' assertions:"
rg "as Sheet" -B 2
# Check for direct usage of members/sheets arrays
echo "Checking direct array usage:"
rg "members\." -B 2
rg "sheets\." -B 2
Length of output: 9946
libs/sr/formula/src/calculator.ts (2)
105-108: LGTM! Improved type safety with defensive checks.
The addition of explicit checks for src, dst, sheet, and q before constructing the meta.conds object improves type safety and prevents potential runtime errors.
14-14: Verify the removal of 'all' key from MemRec type.
The removal of the 'all' key from MemRec<V> type makes member targeting more precise, aligning with the PR's goal. However, this is a breaking change that needs verification.
✅ Verification successful
The removal of 'all' key from MemRec type is intentional and safe
The removal of 'all' from MemRec<V> type in libs/sr/formula/src/calculator.ts is a valid change because:
- The Star Rail (sr) calculator implementation differs from Genshin Impact's (gi) implementation, where 'all' is still used
- The code shows that the Star Rail calculator's
markGatheredmethod explicitly requires bothsrcanddstto be defined, making the 'all' key unnecessary - No usage of 'all' key was found in the Star Rail formula codebase
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for any remaining usage of the 'all' key in member records
# that might be affected by this type change
# Test 1: Search for direct string usage
rg -l "'all'" --type ts
# Test 2: Search for object access patterns
rg -l '\.all\b' --type ts
# Test 3: Search for object literal patterns
rg -l '\ball:' --type ts
Length of output: 796
Script:
#!/bin/bash
# Let's examine the actual usage context of 'all' in the relevant files
# Focusing on formula-related files that might use it in the same context
# Check the specific usage in formula files
rg "'all'" -C 3 libs/gi/formula/src/calculator.ts libs/gi/formula/src/example.test.ts libs/gi/formula/src/util.ts
# Check if there are any imports or type references to calculator.ts
rg -t ts "calculator|MemRec" libs/sr/formula/src/
# Check the specific implementation in the file under review
cat libs/sr/formula/src/calculator.ts
Length of output: 8071
libs/sr/db/src/Database/DataManagers/TeamDataManager.ts (3)
1-1: LGTM: Import changes align with new functionality
The new imports support the enhanced conditional validation system.
Also applies to: 13-19
427-441: LGTM: Efficient update handling
The optimization to prevent unnecessary updates is well implemented. The code checks all relevant fields before updating, which helps prevent unnecessary re-renders.
50-51: Verify compatibility with existing code
The type change from CharacterKey | 'all' to Member is a breaking change. While it aligns with the PR objectives for targeted conditionals, we should verify existing code compatibility.
✅ Verification successful
Let's search for more specific patterns to understand the usage and impact of this type change.
Based on the gathered information, I can now provide a final response:
Type change from CharacterKey | 'all' to Member appears safe
The change from CharacterKey | 'all' to Member type is actually not a breaking change. Here's why:
- The
Membertype is properly defined in the formula package and imported correctly across the codebase - No usage of the literal
'all'was found in the TeamDataManager or related conditional logic - The type is consistently used in both the database layer (TeamDataManager) and UI layer (TeamCalcProvider)
- No migration code was needed for this change, suggesting it's a refinement rather than a breaking change
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any direct usage of 'all' with conditionals
rg -l "'all'.*conditional"
# Search for type usage to verify all callers are updated
ast-grep --pattern 'conditionals: Array<{ src: $_, dst: $_ }>'
Length of output: 99
Script:
#!/bin/bash
# Search for files containing TeamDataManager or related team interfaces
rg -l "TeamDataManager|interface.*Team"
# Search for any usage of 'all' in the codebase
rg -l "'all'" --type ts
# Search for files containing conditionals related to teams or buffs
rg -l "conditional.*team|team.*conditional" --type ts
# Look for Member type definition and usage
rg -l "type.*Member|interface.*Member" --type ts
Length of output: 2058
Script:
#!/bin/bash
# Let's look at the actual Member type definition and its usage in TeamDataManager
rg -A 5 "type.*Member|interface.*Member" libs/sr/db/src/Database/DataManagers/TeamDataManager.ts
# Check how conditionals are used in the team context
rg -A 5 "conditional.*team|team.*conditional" libs/sr/db/src/Database/DataManagers/TeamDataManager.ts
# Look for actual usage of src/dst in team conditionals
rg -A 2 "src:|dst:" libs/sr/db/src/Database/DataManagers/TeamDataManager.ts
# Check if there are any direct usages of 'all' in team-related files
rg "'all'" libs/sr/db/src/Database/DataManagers/TeamDataManager.ts
Length of output: 1468
Script:
#!/bin/bash
# Let's check the Member type definition in the formula package
rg -A 3 "type.*Member" libs/sr/formula/src/util.ts libs/sr/formula/src/data/util/tag.ts
# Check for any usage of CharacterKey | 'all' pattern in the codebase
rg "CharacterKey.*\|.*'all'" --type ts
# Look at the actual usage of conditionals in the team page
rg -A 5 "conditionals.*src.*dst" libs/sr/page-team/src/TeamCalcProvider.tsx
Length of output: 773
Script:
#!/bin/bash
# Let's check the Member type definition in the listing file
rg -A 3 "type.*Member" libs/sr/formula/src/data/util/listing.ts
# Check imports and usage of Member type in TeamCalcProvider
rg -B 2 -A 5 "import.*Member|type.*Member" libs/sr/page-team/src/TeamCalcProvider.tsx
# Look for any migration code or changes related to this type
rg -A 5 "migrate.*team|team.*migrate" libs/sr/db/src/Database/
Length of output: 553
| export function getConditional(sheet: Sheet, condKey: string) { | ||
| return (conditionals as any)[sheet]?.[condKey] as IConditionalData | undefined | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve type safety and add documentation.
The function implementation has the following concerns:
- Using
anytype assertion bypasses TypeScript's type checking - Missing documentation about the function's purpose and parameters
Consider applying these improvements:
+/**
+ * Retrieves conditional data for a given sheet and conditional key.
+ * @param sheet - The sheet identifier
+ * @param condKey - The conditional key to look up
+ * @returns The conditional data if found, undefined otherwise
+ */
export function getConditional(sheet: Sheet, condKey: string) {
- return (conditionals as any)[sheet]?.[condKey] as IConditionalData | undefined
+ const sheetConditionals = conditionals[sheet as keyof typeof conditionals]
+ return sheetConditionals?.[condKey as keyof typeof sheetConditionals] as IConditionalData | undefined
}This refactor:
- Adds JSDoc documentation
- Improves type safety by using more specific type assertions
- Makes the code more maintainable by breaking down the access into steps
📝 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 getConditional(sheet: Sheet, condKey: string) { | |
| return (conditionals as any)[sheet]?.[condKey] as IConditionalData | undefined | |
| } | |
| /** | |
| * Retrieves conditional data for a given sheet and conditional key. | |
| * @param sheet - The sheet identifier | |
| * @param condKey - The conditional key to look up | |
| * @returns The conditional data if found, undefined otherwise | |
| */ | |
| export function getConditional(sheet: Sheet, condKey: string) { | |
| const sheetConditionals = conditionals[sheet as keyof typeof conditionals] | |
| return sheetConditionals?.[condKey as keyof typeof sheetConditionals] as IConditionalData | undefined | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
libs/sr/db/src/Database/DataManagers/TeamDataManager.ts (3)
179-201: Robust validation logic with room for improvementThe new validation system effectively prevents duplicates and ensures data integrity. Consider adding logging for invalid conditionals to help with debugging.
conditionals = conditionals.filter( ({ sheet, condKey, src, dst, condValues }) => { if (!isMember(src) || !isMember(dst)) { + console.warn(`Invalid member in conditional: src=${src}, dst=${dst}`); return false; } const cond = getConditional(sheet, condKey) if (!cond) { + console.warn(`Invalid conditional: sheet=${sheet}, condKey=${condKey}`); return false; }
Line range hint
399-440: Add value validation in setConditional methodThe method lacks validation of the
condValueparameter against the conditional type. This could lead to invalid values being stored.setConditional( teamId: string, sheet: Sheet, condKey: string, src: Member, dst: Member, condValue: number, frameIndex: number ) { this.set(teamId, (team) => { if (frameIndex > team.frames.length) return false + const cond = getConditional(sheet, condKey) + if (!cond) return false + const validatedValue = correctConditionalValue(cond, condValue) const condIndex = team.conditionals.findIndex( (c) => c.condKey === condKey && c.sheet === sheet && c.src === src && c.dst === dst ) if (condIndex === -1) { const condValues = new Array(team.frames.length).fill(0) - condValues[frameIndex] = condValue + condValues[frameIndex] = validatedValue team.conditionals.push({ sheet, src, dst, condKey, condValues, }) } else { // ... rest of the code - cond.condValues[frameIndex] = condValue + cond.condValues[frameIndex] = validatedValue }
508-527: Well-implemented value validation with room for improvementThe function handles all conditional types correctly with proper boundary checking. Consider adding error handling for invalid conditional types.
function correctConditionalValue(conditional: IConditionalData, value: number) { if (conditional.type === 'bool') { return +!!value } else if (conditional.type === 'num') { if (conditional.int_only && !Number.isInteger(value)) { value = Math.round(value) } if (conditional.min !== undefined && value < conditional.min) value = conditional.min if (conditional.max !== undefined && value > conditional.max) value = conditional.max } else if (conditional.type === 'list') { if (!Number.isInteger(value)) { value = Math.round(value) } if (value < 0) value = 0 if (value > conditional.list.length - 1) value = conditional.list.length - 1 - } + } else { + console.error(`Invalid conditional type: ${conditional.type}`); + return 0; // safe default + } return value }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
libs/sr/db/src/Database/DataManagers/TeamDataManager.ts(9 hunks)
🔇 Additional comments (2)
libs/sr/db/src/Database/DataManagers/TeamDataManager.ts (2)
Line range hint 1-19: LGTM: Import changes align with new functionality
The new imports support the enhanced conditional validation system and type checking.
50-51: Verify impact of type change from CharacterKey | 'all' to Member
This is a breaking change in the Team interface that could affect existing code. The type change for src and dst needs careful verification.
Describe your changes
Issue or discord link
Testing/validation
Note: in this screenshot, Arlan has the 4p Watchmaker.
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
RelicConditionalscomponent for enhanced user interaction with relics.RelicSheetDisplayandRelicSheetsDisplaycomponents for displaying relic information.Enhancements
TalentContentandComboEditorcomponents for improved clarity and user experience.TeamDataManagerandCalculatorclasses.Bug Fixes
Documentation