fix zo build sorting #2611
fix zo build sorting #2611
Conversation
WalkthroughThe pull request introduces modifications to the Changes
Suggested Reviewers
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] [Sat Jan 25 04:20:19 UTC 2025] - Deployed 7039ad8 to https://genshin-optimizer-prs.github.io/pr/2611/zzz-frontend (Takes 3-5 minutes after this completes to be available) [sr-frontend] [Sat Jan 25 04:20:39 UTC 2025] - Deployed 7039ad8 to https://genshin-optimizer-prs.github.io/pr/2611/sr-frontend (Takes 3-5 minutes after this completes to be available) [Sat Jan 25 04:30: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: 1
🧹 Nitpick comments (1)
libs/sr/solver/src/parentWorker.ts (1)
Line range hint
182-182: Track UI configurability as a separate enhancement.Both files contain TODO comments about taking
numBuildsfrom the optimization UI. Consider creating a separate issue to track this enhancement for configurable build limits.Would you like me to help create an issue to track the implementation of UI-configurable build limits?
Also applies to: 194-194
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
libs/sr/solver/src/parentWorker.ts(1 hunks)libs/zzz/solver/src/parentWorker.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: lint
🔇 Additional comments (1)
libs/zzz/solver/src/parentWorker.ts (1)
182-182: 🛠️ Refactor suggestionConsider memory usage implications and avoid hard-coding the slice limit.
The changes have two potential issues:
- Removing intermediate sorting/slicing could lead to higher memory usage as all results are kept until the end
- Hard-coding
slice(0, 10)reduces flexibility and ignores the existingMAX_BUILDSconstantConsider this implementation:
- buildResults: results.sort((a, b) => b.value - a.value).slice(0, 10), // TODO: take numBuilds from opt UI + buildResults: results.sort((a, b) => b.value - a.value).slice(0, MAX_BUILDS),Let's verify the potential memory impact:
Describe your changes
Fix situations where ZO builds are not sorted properly.
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
The changes focus on streamlining how computational results are processed and returned, ensuring a more uniform and efficient approach to managing solver outputs.