Improve optimization progress accuracy #2538
Conversation
WalkthroughThe pull request modifies the calculation of the Changes
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
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 (
|
![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
🧹 Outside diff range and nitpick comments (1)
libs/gi/ui/src/components/character/BuildAlert.tsx (1)
72-72: LGTM! Consider adding a clarifying comment.The new progress calculation
(tested + skipped) / total * 100correctly implements the proportional progress tracking, assuming that skipped builds are distributed throughout the optimization process rather than front-loaded.Consider adding a comment to explain this assumption:
+ // Assume skipped builds are proportionally distributed throughout the process progress = (tested + skipped) / total * 100
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
libs/gi/page-team/src/CharacterDisplay/Tabs/TabOptimize/Components/BuildAlert.tsx(1 hunks)libs/gi/ui/src/components/character/BuildAlert.tsx(1 hunks)
🔇 Additional comments (2)
libs/gi/page-team/src/CharacterDisplay/Tabs/TabOptimize/Components/BuildAlert.tsx (2)
Line range hint 58-65: LGTM: Progress display logic aligns with PR objectives.
The updated progress calculation and display logic correctly implements the new approach where:
- Skipped builds are considered part of progress
- Progress more accurately reflects the actual state of optimization
- The UI updates maintain consistency with the numerical changes
58-58: Verify edge cases in progress calculation.
The new formula (tested + skipped) / total * 100 better reflects actual progress by including skipped builds proportionally. However, we should ensure it handles edge cases gracefully.
Consider adding a guard clause:
- progress = (tested + skipped) / total * 100
+ progress = total > 0 ? Math.min((tested + skipped) / total * 100, 100) : 0✅ Verification successful
Progress calculation is safe and correctly implemented
The progress calculation is already protected by sufficient guards:
- The calculation only runs within the
if (generatingBuilds)block - The
BuildStatustype ensures all required values (tested,skipped,total) are non-optional numbers - The
totalis initialized to 0 in the parent component when inactive, preventing division by zero scenarios
The new formula (tested + skipped) / total * 100 is mathematically sound and correctly reflects the proportional progress including skipped builds.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for potential edge cases in progress calculation
# Look for places where total, tested, or skipped might be 0 or undefined
# Test 1: Check if total can be 0 or undefined
rg -A 5 "total\s*=\s*[0]|total\s*=\s*undefined"
# Test 2: Look for error handling around the division
rg -A 5 "total.*===.*0|total.*==.*0|!total"
Length of output: 1073
Script:
#!/bin/bash
# Let's examine the BuildAlert component more thoroughly to understand the progress calculation context
# Look for the component definition and props
ast-grep --pattern 'interface BuildAlertProps {
$$$
}'
# Check how tested, skipped, and total are derived
rg -B 5 -A 5 "progress\s*=.*tested.*skipped.*total"
# Look for any existing guards or error handling
rg -B 3 -A 3 "const.*{.*tested.*skipped.*total"
Length of output: 2548
Script:
#!/bin/bash
# Let's check how the variables are destructured and if there are any guards
rg -B 3 -A 3 "const.*\{.*tested.*skipped.*\}"
# Check if generatingBuilds has any guards
rg -B 3 -A 3 "if.*generatingBuilds.*\{"
# Look for the type definition of the build status
ast-grep --pattern 'type BuildStatus = {
$$$
}'
Length of output: 3915
|
[frontend] [Sun Nov 10 16:51:45 UTC 2024] - Deployed 8a576bd to https://genshin-optimizer-prs.github.io/pr/2538/frontend (Takes 3-5 minutes after this completes to be available) [frontend] [Mon Nov 11 21:59:55 UTC 2024] - Deployed b11e0b6 to https://genshin-optimizer-prs.github.io/pr/2538/frontend (Takes 3-5 minutes after this completes to be available) [Wed Nov 13 02:05:35 UTC 2024] - Deleted deployment |
TornOne
force-pushed
the
improve-optimization-progress-accuracy
branch
from
64a5339 to
430afc0
Compare
|
What's the behavior for this progress bar if |
|
Indeed, if the amount of skipped builds makes up for a large majority of the total builds at the beginning, then the bar will spend more time seemingly near completion, as you said. I am curious if you could provide examples of the behavior you describe happening in real-world use, because in my experience, the opposite is somewhat common currently - the bar gets "stuck" at 0%, spending 10-100 times (this depends on how many more builds are skipped than unskipped) more time going from 0% to 1% than from 99% to 100%. I would hope the change fixes this dramatic "speedup" I regularly see. |
|
I couldn't find the old benchmark data, and we don't have a perf test either :D A more recent test has around 3x pruning instead, and this PR should work fine enough. |
Describe your changes
Updated the progress bar when optimizing artifacts to better reflect the actual percentage of progress done.
The new formula is based on the assumption that the amount of skipped builds remains proportional to the amount of tested builds, but also improves accuracy in cases where more builds are skipped closer to the end than the beginning, and cases where not too many more builds are skipped closer to the beginning than the end.
In contrast, the previous formula essentially assumed all builds that were going to be skipped were skipped as soon as the generation started.
Issue or discord link
https://discord.com/channels/785153694478893126/1296108919608971334
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