Repository navigation
fix(tui): system stats v2 — accurate hw metrics with per-core heatmap - #990
Conversation
… metrics - Use `systeminformation` for proper CPU per-core load and memory stats - RAM now shows active memory (excludes buffers/cache), not total-free - CPU shows combined % + per-core heatmap with color-coded blocks (green < 20%, emerald < 50%, amber < 80%, red > 80%) - Load humanized as % of cores (e.g. "36% (30/84 cores)") - Version shown on first render (before stats load) - Swap hidden when none configured
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8e7c4b8f10
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| async function refresh() { | ||
| try { | ||
| const [cpu, mem] = await Promise.all([si.currentLoad(), si.mem()]); |
There was a problem hiding this comment.
si.currentLoad() and si.mem() are awaited together via Promise.all, and the surrounding catch swallows any error. In environments where one probe intermittently or permanently fails (e.g., restricted containers/unsupported platform paths), this prevents all stats from updating and can leave the widget stuck showing only version text after startup. Collecting metrics independently (or using Promise.allSettled with partial fallbacks) avoids dropping the entire panel when a single source fails.
Useful? React with 👍 / 👎.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request integrates the systeminformation library into the SystemStats component to provide more detailed system metrics, including a new per-core CPU heatmap and humanized load averages. The implementation replaces manual file reading and basic os module calls with asynchronous data fetching. Feedback focuses on improving the robustness of the polling mechanism by using recursive setTimeout to avoid overlapping executions, ensuring cross-platform compatibility for load averages on Windows, and addressing potential inaccuracies in the initial CPU load calculation.
| // Short delay for initial CPU delta to be meaningful | ||
| const init = setTimeout(refresh, 500); | ||
| refresh(); | ||
| const timer = setInterval(refresh, 3000); |
There was a problem hiding this comment.
Using setInterval with an asynchronous function can lead to overlapping executions if the refresh call takes longer than the interval duration. It is generally safer to use a recursive setTimeout pattern to ensure that the next poll only starts after the previous one has completed.
const timer = setTimeout(function run() {
refresh().finally(() => {
if (mountedRef.current) {
setTimeout(run, 3000);
}
});
}, 3000);
return () => {
mountedRef.current = false;
clearTimeout(timer);
};
| if (!mountedRef.current) return; | ||
|
|
||
| const coreCount = os.cpus().length; | ||
| const avg1 = os.loadavg()[0]; |
There was a problem hiding this comment.
The use of os.loadavg() is not portable to Windows systems, where it returns [0, 0, 0]. Since you are already using systeminformation, you can obtain the load average in a cross-platform way from the si.currentLoad() result (via avgLoad) or by using si.fullLoad().
|
|
||
| async function refresh() { | ||
| try { | ||
| const [cpu, mem] = await Promise.all([si.currentLoad(), si.mem()]); |
There was a problem hiding this comment.
The first call to si.currentLoad() often returns 0 or requires a small internal delay to calculate a meaningful delta. By calling refresh() immediately on mount, the first render with stats will likely show 0% CPU usage for the first 3 seconds. Consider restoring a small initial delay (e.g., 500ms) for the first poll, as seen in the previous implementation, to ensure the first non-null state contains accurate data.
Summary
systeminformationfor accurate hardware metrics36% (30/84 cores)instead of raw load averageWhat changed
os.totalmem() - os.freemem()= inflatedsi.mem().active= real usage29.9 27.0 25.136% (30/84 cores)Test plan
bun -eimport check passesgenie tui— verify heatmap, RAM, load display