ZO disc scanner #2598
ZO disc scanner #2598
Conversation
WalkthroughThis pull request introduces a comprehensive set of changes across multiple libraries, focusing on enhancing the disc scanning, processing, and management functionality. The modifications primarily involve restructuring how substats are handled, updating interfaces, introducing new utility functions for image processing, and creating a new disc scanner library. Key changes include transitioning from a Changes
Sequence DiagramsequenceDiagram
participant User
participant DiscEditor
participant ScanningQueue
participant ImageProcessor
participant OCR
User->>DiscEditor: Upload Image
DiscEditor->>ScanningQueue: Add Image File
ScanningQueue->>ImageProcessor: Process Image
ImageProcessor->>OCR: Extract Text
OCR-->>ImageProcessor: Return Text Data
ImageProcessor-->>ScanningQueue: Return Processed Result
ScanningQueue-->>DiscEditor: Update Scanning Status
DiscEditor->>User: Display Scanned Disc Information
Possibly related PRs
Suggested labels
Poem
Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
[zzz-frontend] [Fri Jan 17 05:58:06 UTC 2025] - Deployed 5d7e6d1 to https://genshin-optimizer-prs.github.io/pr/2598/zzz-frontend (Takes 3-5 minutes after this completes to be available) [sr-frontend] [Fri Jan 17 05:58:30 UTC 2025] - Deployed 5d7e6d1 to https://genshin-optimizer-prs.github.io/pr/2598/sr-frontend (Takes 3-5 minutes after this completes to be available) [frontend] [Fri Jan 17 05:58:47 UTC 2025] - Deployed 5d7e6d1 to https://genshin-optimizer-prs.github.io/pr/2598/frontend (Takes 3-5 minutes after this completes to be available) [sr-frontend] [Fri Jan 17 06:16:58 UTC 2025] - Deployed 755a9da to https://genshin-optimizer-prs.github.io/pr/2598/sr-frontend (Takes 3-5 minutes after this completes to be available) [zzz-frontend] [Fri Jan 17 06:17:07 UTC 2025] - Deployed 755a9da to https://genshin-optimizer-prs.github.io/pr/2598/zzz-frontend (Takes 3-5 minutes after this completes to be available) [frontend] [Fri Jan 17 06:17:15 UTC 2025] - Deployed 755a9da to https://genshin-optimizer-prs.github.io/pr/2598/frontend (Takes 3-5 minutes after this completes to be available) [zzz-frontend] [Sat Jan 18 05:12:30 UTC 2025] - Deployed 1cba715 to https://genshin-optimizer-prs.github.io/pr/2598/zzz-frontend (Takes 3-5 minutes after this completes to be available) [frontend] [Sat Jan 18 05:13:30 UTC 2025] - Deployed 1cba715 to https://genshin-optimizer-prs.github.io/pr/2598/frontend (Takes 3-5 minutes after this completes to be available) [sr-frontend] [Sat Jan 18 05:13:47 UTC 2025] - Deployed 1cba715 to https://genshin-optimizer-prs.github.io/pr/2598/sr-frontend (Takes 3-5 minutes after this completes to be available) [Sat Jan 18 14:51:38 UTC 2025] - Deleted deployment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🔭 Outside diff range comments (2)
libs/zzz/ui/src/Disc/DiscEditor/DiscEditor.tsx (1)
Line range hint
166-170: Fix substat removal logic insetSubstatfunctionThe
filtermethod doesn't modify thesubstatsarray in place. Without assigning the filtered array back, the substat at the specified index won't be removed as intended.Apply this diff to correct the issue:
const setSubstat = useCallback( (index: number, substat?: ISubstat) => { const substats = [...(disc.substats || [])] if (substat) substats[index] = substat - else substats.filter((_, i) => i !== index) + else substats = substats.filter((_, i) => i !== index) setDisc({ substats }) }, [disc, setDisc] )libs/zzz/db/src/Interfaces/IDisc.ts (1)
Make location, lock, and trash properties optional in IDisc interface
The verification confirms that these properties are not supported by the scanner:
- The scanner sets them to default values (
location: '',lock: false,trash: false)- No validation logic exists for these properties
- No documentation about these limitations
Since these appear to be metadata properties managed outside the scanner, they should be marked as optional in the interface.
🔗 Analysis chain
Line range hint
19-21: Address unsupported scanner features.The interface includes
location,lock, andtrashproperties, but according to the PR description, these features are not yet supported by the scanner. Consider:
- Making these properties optional
- Adding validation in the scanner
- Documenting the scanner limitations
Let's check if there's any validation for these unsupported features:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for validation of unsupported features rg -t typescript "location|lock|trash" ./libs/zzz/disc-scanner/Length of output: 100
Script:
#!/bin/bash # Search for property usage in TypeScript files rg "location|lock|trash" --glob "*.{ts,tsx}" ./libs/zzz/disc-scanner/ # Look for validation-related files fd -e ts -e tsx . ./libs/zzz/disc-scanner/ | grep -i "valid\|scanner" # Check for documentation fd "README|DOCS" -e md ./libs/zzz/disc-scanner/Length of output: 681
Script:
#!/bin/bash # Check processImg.ts implementation cat ./libs/zzz/disc-scanner/src/lib/processImg.ts # Check parse.ts for validation cat ./libs/zzz/disc-scanner/src/lib/parse.ts # Check documentation cat ./libs/zzz/disc-scanner/README.mdLength of output: 10331
🧹 Nitpick comments (18)
libs/zzz/disc-scanner/tsconfig.lib.json (1)
23-23: Consider restricting to TypeScript files only.Since this is a new TypeScript library, you might want to consider restricting the includes to just TypeScript files (*.ts, *.tsx) unless there's a specific need for JavaScript files.
- "include": ["src/**/*.js", "src/**/*.jsx", "src/**/*.ts", "src/**/*.tsx"] + "include": ["src/**/*.ts", "src/**/*.tsx"]libs/zzz/ui/src/Disc/DiscEditor/ScanningUtil.tsx (1)
34-36: Simplify nestedawaitstatements for readabilityThe nested
awaitstatements can be flattened to improve code clarity.Apply this diff to simplify the code:
async (scheduler) => - (await ( - await scheduler - ).addJob('recognize', canvas, options)) as RecognizeResult + async (schedulerPromise) => { + const scheduler = await schedulerPromise + const result = await scheduler.addJob('recognize', canvas, options) + return result as RecognizeResult + }libs/zzz/disc-scanner/src/lib/processImg.ts (1)
125-129: Avoid defaulting to a specific set key without detectionDefaulting to
'AstralVoice'when the set key is not detected may lead to incorrect data entries. Consider prompting the user for input or marking the disc as incomplete.You might modify the code to handle this case more appropriately, such as:
- setKey = 'AstralVoice' - retProcessed.texts.push( - 'Could not detect set key. Assuming Astral Voice.' - ) + retProcessed.texts.push( + 'Could not detect set key. Please verify and enter the correct set.' + )libs/zzz/ui/src/Disc/DiscEditor/DiscEditor.tsx (2)
254-256: Ensure consistent event listener managementIf
allowUploadchanges over time, the event listener for thepasteevent might not be correctly added or removed, leading to unexpected behavior. Consider using a ref to storeallowUploador restructuring the effect to handle updates.You might revise the effect to ensure it handles
allowUploadchanges:-useEffect(() => { +useEffect(() => { + if (!allowUpload) return const pasteFunc = (e: Event) => { // Existing code... } - allowUpload && window.addEventListener('paste', pasteFunc) + window.addEventListener('paste', pasteFunc) return () => { - if (allowUpload) window.removeEventListener('paste', pasteFunc) + window.removeEventListener('paste', pasteFunc) } -}, [uploadFiles, allowUpload]) +}, [uploadFiles, allowUpload])
221-226: Reset input value after file selection to allow re-uploadDirectly assigning an empty string to
e.target.valuemay not consistently reset the file input in all browsers. To ensure the input is reset properly, consider using a ref or creating a new input element.Alternatively, you can reset the input by setting the
valuetonull:(e: ChangeEvent<HTMLInputElement>) => { if (!e.target) return uploadFiles(e.target.files) - e.target.value = '' // reset the value so the same file can be uploaded again... + e.target.value = null // Reset the input },libs/zzz/ui/src/util/isDev.ts (1)
1-7: Add type safety for environment variables.While the implementation is correct, consider adding type safety for environment variables to prevent runtime issues.
+declare global { + namespace NodeJS { + interface ProcessEnv { + NODE_ENV: 'development' | 'production' | 'test' + NX_SHOW_DEV_COMPONENTS?: string + } + } +} export const isDev = process.env['NODE_ENV'] === 'development'libs/zzz/disc-scanner/src/lib/consts.ts (2)
3-8: Consider expanding character mapping for OCR corrections.Given the PR objective mentions OCR issues with text wrapping, consider expanding the
misreadCharactersInSubstatMapto handle more common OCR misreadings.export const misreadCharactersInSubstatMap = [ { pattern: /#/, replacement: '+', }, + { + pattern: /[Oo0]/, + replacement: '0', + }, + { + pattern: /[Il1]/, + replacement: '1', + }, ]
10-14: Add type literal for Color values.Consider using a more specific type for color values to ensure they stay within valid RGB ranges.
+type RGBValue = 0 | 1 | 2 | /* ... */ | 254 | 255 export const blackColor: Color = { - r: 0, - g: 0, - b: 0, + r: 0 as RGBValue, + g: 0 as RGBValue, + b: 0 as RGBValue, }libs/zzz/db/src/Interfaces/IDisc.ts (2)
11-11: Consider using a more specific type for upgrades count.Since upgrades represent discrete improvements, consider using a union type to enforce valid values.
- upgrades: number // This is the number of upgrades this sub receives. + upgrades: 0 | 1 | 2 | 3 | 4 | 5 // Maximum upgrades possible
16-16: Enforce level range with type.Instead of relying on a comment, consider enforcing the level range through types.
- level: number // 0-15 + type DiscLevel = 0 | 1 | 2 | 3 | 4 | 5 | 6 | 7 | 8 | 9 | 10 | 11 | 12 | 13 | 14 | 15 + level: DiscLevellibs/zzz/disc-scanner/src/lib/enStringMap.ts (2)
12-27: Standardize naming patterns in statMapEngMap.There are inconsistencies in the naming patterns:
- Some keys end with underscore (e.g.,
hp_,atk_)- Some don't (e.g.,
anomProf)- Some use camelCase (e.g.,
enerRegen_)Consider standardizing the naming pattern across all keys for better maintainability.
29-31: Consider pre-computing elemental damage bonus mappings.The current implementation dynamically generates elemental damage bonus entries on every import. Consider pre-computing these values to avoid unnecessary iterations.
-Object.entries(elementalData).forEach(([e, name]) => { - statMapEngMap[`${e}_dmg_`] = `${name} DMG Bonus` -}) +const elementalDmgMap = Object.fromEntries( + Object.entries(elementalData).map(([e, name]) => + [`${e}_dmg_`, `${name} DMG Bonus`] + ) +) +Object.assign(statMapEngMap, elementalDmgMap)libs/zzz/ui/src/Disc/DiscMainStatDropdown.tsx (1)
46-59: Consider memoizing MenuItem components.The MenuItems are recreated on every render. Consider memoizing them to improve performance, especially if the list is long.
+const MainStatMenuItem = memo(function MainStatMenuItem({ + mk, + statKey, + setStatKey +}: { + mk: DiscMainStatKey + statKey?: DiscMainStatKey + setStatKey: (statKey: DiscMainStatKey) => void +}) { + return ( + <MenuItem + key={mk} + selected={statKey === mk} + disabled={statKey === mk} + onClick={() => setStatKey(mk)} + > + <ListItemIcon> + <StatIcon statKey={mk} /> + </ListItemIcon> + <StatDisplay statKey={mk} showPercent disableIcon /> + </MenuItem> + ) +})libs/zzz/disc-scanner/src/lib/ScanningQueue.tsx (2)
11-12: Consider making queue limits configurable.The maximum processing and processed counts are hardcoded. Consider making these configurable through constructor parameters to allow for different use cases and environments.
-const maxProcessingCount = 3, - maxProcessedCount = 16 +export type ScanningQueueConfig = { + maxProcessingCount?: number + maxProcessedCount?: number +}
62-66: Fix typo in variable name.The variable name
procesesdhas a typo and should beprocessed.-const procesesd = this.processed.shift() -if (procesesd) this.processQueue() -return procesesd +const processed = this.processed.shift() +if (processed) this.processQueue() +return processedlibs/zzz/ui/src/Disc/DiscEditor/SubstatInput.tsx (3)
38-52: Remove commented-out validation code.There's a significant block of commented-out validation code. Either implement the validation or remove the comments to maintain code cleanliness.
- // let error = '', - // allowedRolls = 0 - - // if (disc?.rarity) { - // // Account for the rolls it will need to fill all 4 substates, +1 for its base roll - // const rarity = disc.rarity - // const { numUpgrades, high } = discSubstatRollData[rarity] - // const maxRollNum = numUpgrades + high - 3 - // allowedRolls = maxRollNum - value - // } - - // if (allowedRolls < 0) - // error = - // error || - // t('editor.substat.error.noOverRoll', { value: allowedRolls + value })
90-92: Remove commented-out ListItemIcon code.There's commented-out ListItemIcon code. Either implement the icon or remove the comments.
- {/* <ListItemIcon> - <StatIcon statKey={k} /> - </ListItemIcon> */}
167-175: Add aria-label to Slider component.The Slider component is missing accessibility attributes.
<Slider value={innerValue} step={null} disabled={disabled} marks={marks} min={marks[0]?.value ?? 0} max={marks[marks.length - 1]?.value ?? 0} onChange={(_e, v) => setinnerValue(v as number)} onChangeCommitted={(_e, v) => setValue(v as number)} valueLabelDisplay="auto" valueLabelFormat={valueLabelFormat} + aria-label={t('editor.substat.upgradeSlider')} />
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (27)
libs/common/img-util/src/processing.ts(2 hunks)libs/zzz/consts/src/disc.ts(2 hunks)libs/zzz/db/src/Database/DataManagers/DiscDataManager.ts(6 hunks)libs/zzz/db/src/Interfaces/IDbDisc.ts(1 hunks)libs/zzz/db/src/Interfaces/IDisc.ts(1 hunks)libs/zzz/disc-scanner/.babelrc(1 hunks)libs/zzz/disc-scanner/.eslintrc.json(1 hunks)libs/zzz/disc-scanner/README.md(1 hunks)libs/zzz/disc-scanner/project.json(1 hunks)libs/zzz/disc-scanner/src/index.ts(1 hunks)libs/zzz/disc-scanner/src/lib/ScanningQueue.tsx(1 hunks)libs/zzz/disc-scanner/src/lib/consts.ts(1 hunks)libs/zzz/disc-scanner/src/lib/enStringMap.ts(1 hunks)libs/zzz/disc-scanner/src/lib/parse.ts(1 hunks)libs/zzz/disc-scanner/src/lib/processImg.ts(1 hunks)libs/zzz/disc-scanner/tsconfig.eslint.json(1 hunks)libs/zzz/disc-scanner/tsconfig.json(1 hunks)libs/zzz/disc-scanner/tsconfig.lib.json(1 hunks)libs/zzz/page-discs/src/index.tsx(1 hunks)libs/zzz/solver/src/common.ts(2 hunks)libs/zzz/ui/src/Disc/DiscCard.tsx(3 hunks)libs/zzz/ui/src/Disc/DiscEditor/DiscEditor.tsx(8 hunks)libs/zzz/ui/src/Disc/DiscEditor/ScanningUtil.tsx(1 hunks)libs/zzz/ui/src/Disc/DiscEditor/SubstatInput.tsx(1 hunks)libs/zzz/ui/src/Disc/DiscMainStatDropdown.tsx(2 hunks)libs/zzz/ui/src/util/isDev.ts(1 hunks)tsconfig.base.json(1 hunks)
✅ Files skipped from review due to trivial changes (7)
- libs/zzz/disc-scanner/README.md
- libs/zzz/disc-scanner/src/index.ts
- libs/zzz/disc-scanner/.babelrc
- libs/zzz/disc-scanner/tsconfig.eslint.json
- libs/zzz/disc-scanner/tsconfig.json
- libs/zzz/disc-scanner/.eslintrc.json
- libs/zzz/disc-scanner/project.json
🧰 Additional context used
🪛 Biome (1.9.4)
libs/zzz/db/src/Database/DataManagers/DiscDataManager.ts
[error] 360-360: isFinite is unsafe. It attempts a type coercion. Use Number.isFinite instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isFinite instead.
(lint/suspicious/noGlobalIsFinite)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: lint
🔇 Additional comments (16)
libs/zzz/disc-scanner/tsconfig.lib.json (3)
1-2: LGTM! Base configuration extension is properly set up.The configuration correctly extends the base
tsconfig.json, maintaining consistency with the project's TypeScript settings.
12-22: LGTM! Test file exclusions are comprehensive.The configuration properly excludes all test files across different extensions (ts, tsx, js, jsx) and naming patterns (.spec., .test.).
3-11: Verify the output directory path and consider adding scanner-specific types.The compiler options look good, but since this is a scanner library, you might want to consider:
- Adding types for image processing libraries you'll be using
- Including OCR-related type definitions if you're planning to use any OCR libraries
Let's verify the output directory structure:
libs/zzz/disc-scanner/src/lib/processImg.ts (2)
141-143: EnsureslotKeyis correctly identified when missingDefaulting the
mainStatKeywithout a detectedslotKeymay cause inconsistencies. Ensure that theslotKeyis identified based on themainStatKey, or prompt the user for verification.You might add a check to handle this scenario more robustly.
134-138: Handle undetected rarity and level more cautiouslyAssuming a default rarity of
'S'and level15when detection fails might lead to inaccurate data. It's better to notify the user and request manual input.Consider updating the code to reflect this:
- retProcessed.texts.push( - 'Could not detect rarity + level, assuming S Lv.15/15' - ) - rarity = 'S' - level = 15 + retProcessed.texts.push( + 'Could not detect rarity and level. Please verify and enter the correct values.' + )libs/zzz/db/src/Interfaces/IDbDisc.ts (1)
Line range hint
1-6: Verify the impact of interface simplification.The significant reduction in the
ICachedDiscinterface (removal ofmainStatValandsubstats) is a breaking change. While this aligns with the move to an upgrades-based system, we should ensure all consumers of this interface are updated.Let's verify the usage of this interface across the codebase:
✅ Verification successful
Interface changes appear safe to proceed.
The
ICachedDiscinterface is not currently used in the codebase, and its simplified structure (only adding anidfield toIDisc) aligns well with the upgrades-based system already implemented in the baseIDiscinterface. The changes can proceed safely.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for ICachedDisc usage rg "ICachedDisc" --type typescript -B 2 -A 2Length of output: 79
Script:
#!/bin/bash # First, verify the TypeScript file extensions in the codebase fd "\.(ts|tsx)$" # Search for ICachedDisc usage in TypeScript files rg "ICachedDisc" -g "*.{ts,tsx}" -B 2 -A 2 # Use AST-based search to find interface usage ast-grep --pattern 'interface $_ extends ICachedDisc' ast-grep --pattern 'class $_ implements ICachedDisc' ast-grep --pattern ': ICachedDisc'Length of output: 73698
Script:
#!/bin/bash # Check the file history fd "IDbDisc.ts$" --exec git log -p {} | grep -A 5 -B 5 "ICachedDisc" # Look for any references in commit messages git log --grep="ICachedDisc" # Search for any files that might have been renamed git log --follow --patch libs/zzz/db/src/Interfaces/IDbDisc.tsLength of output: 0
Script:
#!/bin/bash # Check the base IDisc interface cat libs/zzz/db/src/Interfaces/IDisc.ts # Look for related files in the Database implementation directory fd "Disc.*\.(ts|tsx)$" libs/zzz/db/src/Database/ # Check for any implementations in DataManagers fd "DataManager.*\.(ts|tsx)$" libs/zzz/db/src/Database/DataManagers/Length of output: 842
libs/zzz/disc-scanner/src/lib/enStringMap.ts (1)
3-10: Consider adding missing elemental types.The
elementalDatamapping appears incomplete. Based on common game mechanics, consider adding other elemental types like 'wind', 'quantum', 'imaginary' if they exist in the game.libs/zzz/page-discs/src/index.tsx (1)
30-37: Consider separating upload handling logic.The
allowUploadprop suggests image upload functionality, but the handling logic isn't immediately clear. Consider extracting the upload handling into a separate component or hook for better maintainability.Also, verify that
onShowandonOpenserve different purposes, as they're both being passed the same function.✅ Verification successful
The
onShowandonOpenprops serve different purposesLooking at the implementation in
DiscEditor,onShowis used for queue processing side effects whileonOpenhandles UI visibility. The current implementation is correct and follows the codebase patterns.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for usages of onShow and onOpen props rg "onShow|onOpen" --type tsLength of output: 13968
libs/zzz/ui/src/Disc/DiscCard.tsx (2)
207-211: LGTM!The
SubstatDisplaycomponent is now correctly invoked with the requiredrarityprop, which is essential for calculating the substat's base value.
Line range hint
285-307: LGTM! Clean implementation of the substat display logic.The component correctly:
- Uses the
rarityparameter to get the base value- Multiplies it by the number of upgrades
- Formats the result appropriately
libs/zzz/db/src/Database/DataManagers/DiscDataManager.ts (3)
96-98: LGTM!Clean implementation of the substat mapping that correctly preserves only the necessary fields (
keyandupgrades).
241-241: LGTM!The duplicate detection logic has been properly updated to use
upgradesinstead ofvaluewhile maintaining the same functionality.Also applies to: 257-257, 265-265, 283-283
300-310: LGTM!Clean destructuring of disc properties with proper type annotation.
libs/common/img-util/src/processing.ts (2)
145-145: LGTM!The
thresholdFilterfunction is now properly exported for use in other modules.
165-165: LGTM!The
invertColorsfunction is now properly exported for use in other modules.tsconfig.base.json (1)
158-160: LGTM!The path mapping for the new disc scanner module is correctly configured and maintains the established format.
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (5)
libs/zzz/db/src/Database/DataManagers/DiscDataManager.ts (2)
355-366: Consider adding range validation for upgrades.The validation for the
upgradesproperty is good, but consider adding a range check to ensure the value is non-negative and within expected bounds.if ( !allDiscSubStatKeys.includes(key as DiscSubStatKey) || typeof upgrades !== 'number' || - !Number.isFinite(upgrades) + !Number.isFinite(upgrades) || + upgrades < 0 )
Line range hint
1-380: Consider adding JSDoc comments for public methods.While the implementation is solid, adding JSDoc comments for public methods would improve maintainability and help document the transition from
valuetoupgradesbased substats.Example for the
parseSubstatsfunction:/** * Parses and validates an array of substats. * @param obj - The input object containing substat data * @param _rarity - The rarity of the disc * @param _allowZeroSub - Whether to allow zero value substats * @param _sortSubs - Whether to sort the substats * @returns An array of validated substats with upgrade values */libs/zzz/disc-scanner/src/lib/processImg.ts (3)
42-47: Add error handling for invalidImageDatainzzzPreprocessImage.Currently, the function assumes that
pixelDatais always valid. Consider adding checks to handle cases wherepixelDatamight benullorundefinedto prevent potential runtime errors.
89-97: Consolidate duplicate error messages for clarity.The error message
'Could not detect main stat, substats or set effect.'is repeated multiple times. Consider defining it once as a constant or using a helper function to avoid duplication and make future changes easier.Apply this diff to define a constant:
+const ERROR_DETECT_STATS = 'Could not detect main stat, substats or set effect.' if ( mainStatTextIndex === -1 || substatTextIndex === -1 || setEffectTextIndex === -1 ) { retProcessed.texts.push( - 'Could not detect main stat, substats or set effect.' + ERROR_DETECT_STATS ) return retProcessed }
173-233: ExtractcropDiscCardfunction into a separate module for clarity.The
cropDiscCardfunction is sizable and focused on a specific task. Moving it to its own module can improve code organization and reusability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
libs/common/img-util/src/processing.ts(4 hunks)libs/zzz/db/src/Database/DataManagers/DiscDataManager.ts(6 hunks)libs/zzz/disc-scanner/src/lib/processImg.ts(1 hunks)libs/zzz/ui/src/Disc/DiscMainStatDropdown.tsx(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- libs/common/img-util/src/processing.ts
- libs/zzz/ui/src/Disc/DiscMainStatDropdown.tsx
🔇 Additional comments (3)
libs/zzz/db/src/Database/DataManagers/DiscDataManager.ts (3)
96-98: LGTM! Clean transition to upgrades-based substats.The change from
valuetoupgradesin the substat mapping is implemented correctly and maintains type safety.
241-241: Verify the duplicate detection logic with test cases.The transition from
valuetoupgradescomparison in duplicate detection looks correct. However, since this is a critical feature that affects data integrity, comprehensive testing is recommended.Would you like me to help generate test cases covering various duplicate and upgrade scenarios?
Also applies to: 257-257, 265-265, 283-283
300-310: LGTM! Clean property destructuring.The destructuring pattern has been improved to be more concise while maintaining completeness.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
libs/zzz/disc-scanner/src/lib/parse.ts (3)
15-32: Add input validation to utility functions.While the implementation is correct, consider adding defensive programming checks:
function getBestKeyDist<T extends string>(hams: Array<KeyDist<T>>) { + if (!hams.length) return [] const minHam = Math.min(...hams.map(([, ham]) => ham)) const keys = hams.filter(([, ham]) => ham === minHam).map(([key]) => key) return keys } function findBestKeyDist<T extends string>( str: string, keys: readonly T[] ): T | undefined { + if (!str?.trim()) return undefined if (!keys.length) return undefined if (keys.length === 1) return keys[0]
53-64: Optimize parseSet function performance.The current implementation checks all combinations of texts and keys. Consider adding optimizations:
export function parseSet(texts: string[]) { + if (!texts?.length) return undefined const kdist: Array<KeyDist<DiscSetKey>> = [] for (const text of texts) { + const setKeyStrTrimmed = text.replace(/\W/g, '') + // Early exit on exact match + const exactMatch = allDiscSetKeys.find(key => key === setKeyStrTrimmed) + if (exactMatch) return exactMatch for (const key of allDiscSetKeys) { - const setKeyStrTrimmed = text.replace(/\W/g, '') kdist.push([key, levenshteinDistance(setKeyStrTrimmed, key)]) } }
84-95: Enhance main stat key matching robustness.Consider improving the text matching logic:
export function parseMainStatKeys(texts: string[], slotKey?: DiscSlotKey) { + if (!texts?.length) return undefined const keys = slotKey ? discSlotToMainStatKeys[slotKey] : allDiscMainStatKeys if (keys.length === 1) return keys[0] for (const text of texts) { + const normalizedText = text.toLowerCase().trim() const isPercent = text.includes('%') const filteredKeys = keys.filter((key) => isPercent === key.endsWith('_')) for (const key of filteredKeys) { - if (text.includes(statMapEngMap[key])) return key + const statName = statMapEngMap[key].toLowerCase() + if (normalizedText.includes(statName)) return key } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
libs/zzz/disc-scanner/src/lib/parse.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: lint
🔇 Additional comments (2)
libs/zzz/disc-scanner/src/lib/parse.ts (2)
1-14: LGTM! Well-organized imports with clear separation of concerns.The imports are properly structured, utilizing TypeScript's type system effectively with clear separation between types, constants, and utility functions.
97-120: Previous review comments about substat parsing remain valid.The issues mentioned in the previous review about duplicate substats, upgrade validation, and maximum limits are still applicable.
Additionally, consider normalizing the stat string before matching:
export function parseSubstats(texts: string[]): ISubstat[] { + if (!texts?.length) return [] + const seenKeys = new Set<string>() const substats: ISubstat[] = [] for (let text of texts) { // Apply OCR character corrections for (const { pattern, replacement } of misreadCharactersInSubstatMap) { text = text.replace(pattern, replacement) } const isPercent = text.includes('%') const match = /([a-zA-Z\s]+)\s*(\+(\d))?/.exec(text) if (match) { - const statStr = match[1].trim() + const statStr = match[1].toLowerCase().trim() const key = findBestKeyDist( statStr, allDiscSubStatKeys.filter((key) => isPercent === key.endsWith('_')) ) - if (!key) continue + if (!key || seenKeys.has(key)) continue + seenKeys.add(key) + const upgrade = match[3] ? parseInt(match[3]) + 1 : 1 + if (upgrade > 5) continue // Max upgrades is 5 substats.push({ key, - upgrades: match[3] ? parseInt(match[3]) + 1 : 1, + upgrades: upgrade, }) } }
| @@ -0,0 +1,7 @@ | |||
| export const isDev = process.env['NODE_ENV'] === 'development' | |||
There was a problem hiding this comment.
Prob should refactor but shrug
Describe your changes
Pretty bare bones ZZZ scanner inspired by GO implementation.
Does not scan location/lock/trash.
Seems that "Shockstar Disco" has an issue due to a unique text wrap-> OCR issue where OCR does not read "[2]" on the second line properly. Can be mitigated via inferring from mainstat(not 100%)
Issue or discord link
Testing/validation
Checklist before requesting a review (leave this PR as draft if any part of this list is not done.)
yarn run mini-cilocally to validate format and lint.Summary by CodeRabbit
Based on the comprehensive changes, here are the updated release notes:
New Features
Improvements
Changes
Development