ZO Add option to clear 4/2p set #2621
Conversation
|
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 eslint
/usr/local/bin/yarn: 3: dirname: not found Error: Cannot find module '/yarn.js' Node.js v22.9.0 WalkthroughThe pull request modifies the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
[zzz-frontend] [Sat Jan 25 20:34:30 UTC 2025] - Deployed 4082685 to https://genshin-optimizer-prs.github.io/pr/2621/zzz-frontend (Takes 3-5 minutes after this completes to be available) [zzz-frontend] [Sat Jan 25 20:46:17 UTC 2025] - Deployed 4082685 to https://genshin-optimizer-prs.github.io/pr/2621/zzz-frontend (Takes 3-5 minutes after this completes to be available) [Sat Jan 25 20:46:50 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: 0
🧹 Nitpick comments (2)
libs/zzz/page-optimize/src/Optimize.tsx (2)
336-349: Consider extracting duplicated filtering logic.The filtering logic for removing set constraints is duplicated across multiple places. Consider extracting it into a reusable function to improve maintainability.
+const removeSetConstraint = ( + constraints: Constraints, + setValue: number +) => Object.fromEntries( + Object.entries(constraints).filter( + ([k, { value }]) => !(allDiscSetKeys.includes(k as DiscSetKey) && value === setValue) + ) +) function Set4Selector({ constraints, setConstraints, }: { constraints: Constraints setConstraints: (c: Constraints) => void }) { // ... <MenuItem onClick={() => - setConstraints( - Object.fromEntries( - Object.entries(constraints).filter( - ([k, { value }]) => - !(allDiscSetKeys.includes(k as DiscSetKey) && value === 4) - ) - ) - ) + setConstraints(removeSetConstraint(constraints, 4)) } >
382-395: Implementation looks good but could be simplified.The implementation correctly handles clearing the 2-set constraint. Apply the same refactoring suggestion as the 4-set clearing to maintain consistency and reduce code duplication.
<MenuItem onClick={() => - setConstraints( - Object.fromEntries( - Object.entries(constraints).filter( - ([k, { value }]) => - !(allDiscSetKeys.includes(k as DiscSetKey) && value === 2) - ) - ) - ) + setConstraints(removeSetConstraint(constraints, 2)) } >
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
libs/zzz/page-optimize/src/Optimize.tsx(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: lint
- GitHub Check: test
🔇 Additional comments (1)
libs/zzz/page-optimize/src/Optimize.tsx (1)
Line range hint
336-395: Add test coverage for the new set clearing functionality.Please add unit tests to verify:
- Clearing 4-set constraints works correctly
- Clearing 2-set constraints works correctly
- Edge cases (e.g., clearing when no constraints exist)
- Integration with the optimization process
Would you like me to help generate the test cases?
Describe your changes
Add option to remove the forced 4/2 sets in optimize.
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