-
Notifications
You must be signed in to change notification settings - Fork 2.4k
fix: calculation memory on hardware and system monitor #5922
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Important
Looks good to me! 👍
Reviewed everything up to c018bf8 in 2 minutes and 27 seconds. Click for details.
- Reviewed
43lines of code in2files - Skipped
0files when reviewing. - Skipped posting
2draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. web-app/src/routes/settings/hardware.tsx:296
- Draft comment:
The progress bar now uses (total_memory - used_memory)/total_memory, which represents the available memory percentage. The PR description suggests switching to a used memory representation. If the intent is to show used memory %, this should be systemUsage.used_memory/hardwareData.total_memory. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50% The comment correctly identifies that the calculation was changed to show available memory instead of used memory. However, this appears to be an intentional change to match the "Available RAM" display above it. The code is consistent in showing available memory rather than used memory throughout this section. The comment assumes this is incorrect without strong evidence. I could be wrong about the intention - maybe the UI should consistently show used memory instead of available memory. The comment raises a valid point about consistency with the PR description. Without seeing the PR description or requirements, I cannot assume this change is incorrect. The code is internally consistent in showing available memory. The comment makes assumptions about correctness without strong evidence. The code appears internally consistent in its choice to display available rather than used memory.
2. web-app/src/routes/system-monitor.tsx:96
- Draft comment:
The RAM card computes 'ramUsagePercentage' as available memory percentage while the 'usedRam' row shows used memory. This mix may confuse users; if the goal is to display used memory usage consistently, update the percentage calculation to systemUsage.used_memory/hardwareData.total_memory. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
Workflow ID: wflow_MBYcYohTB55wFymn
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
Barecheck - Code coverage reportTotal: 38.65%Your code coverage diff: 1.29% ▴ Uncovered files and lines
|
louis-jan
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be great to have test added haha
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Caution
Changes requested ❌
Reviewed 1b96362 in 1 minute and 51 seconds. Click for details.
- Reviewed
181lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. web-app/src/routes/settings/__tests__/hardware.test.tsx:106
- Draft comment:
Directly setting global.IS_MACOS can lead to side effects; consider isolating platform behavior via dependency injection or mocks. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% This is a test file where global state manipulation is common and accepted. The IS_MACOS flag is properly managed with beforeEach() cleanup. The tests are explicitly testing platform-specific behavior, so directly toggling this flag is reasonable. Using dependency injection here would add complexity without clear benefits. The comment raises a valid general principle about avoiding global state. Global state can make tests harder to maintain and debug. While global state can be problematic, in test files it's common and acceptable to manipulate globals, especially when testing platform-specific behavior. The code follows testing best practices by cleaning up in beforeEach(). The comment should be deleted. The current implementation is a standard testing pattern and the suggested changes would add unnecessary complexity.
Workflow ID: wflow_88J8qrcq7bpCkBAz
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
Describe Your Changes
This pull request updates the memory usage calculations in the
HardwareandSystemMonitorcomponents of the web application to ensure consistency and correctness. The changes primarily involve switching the representation of memory usage from "available memory" to "used memory" in the relevant components.Updates to memory usage calculations:
web-app/src/routes/settings/hardware.tsx: Modified the progress bar and percentage display to calculate memory usage as(used_memory / total_memory)instead of(available_memory / total_memory)for better alignment with standard conventions.web-app/src/routes/system-monitor.tsx: Updated the memory usage display to show "used memory" directly instead of "available memory" by changing the calculation to usesystemUsage.used_memory.Fixes Issues
Self Checklist
Important
Update memory usage calculations in
HardwareandSystemMonitorto use 'used memory' instead of 'available memory', and add tests.hardware.tsxto use(used_memory / total_memory)instead of(available_memory / total_memory).system-monitor.tsxto displayused_memorydirectly.hardware.test.tsxto test memory usage display and other hardware settings.This description was created by
for 1b96362. You can customize this summary. It will automatically update as commits are pushed.