Add ratatui and fix implementation - #10
Conversation
📝 WalkthroughWalkthroughChangesProcess monitoring feature
Release automation
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant CLI
participant Monitor
participant ProcessSampler
participant Output
CLI->>Monitor: resolve target and collect samples
Monitor->>ProcessSampler: sample process metrics
ProcessSampler-->>Monitor: ProcessInfo
Monitor-->>CLI: collected samples
CLI->>Output: render dashboard or write CSV/JSON
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (4)
src/core/monitor.rs (1)
64-76: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winSampling period drifts, and
collectnever returns whendurationisNone.
thread::sleep(self.interval)runs after each sample. The sampling work adds to the period, so the effective interval is larger than the configured value and the drift accumulates over a long run. Sleeping until the next scheduled tick keeps the period stable.
collectalso loops forever whendurationisNone, becauseis_expiredalways returnsfalse.src/main.rscurrently requires--durationfor exports, so this path is not reachable from the CLI. A guard incollectprotects the public API against future callers.♻️ Proposed drift-free loop with a duration guard
pub fn collect(mut self) -> Result<Vec<ProcessInfo>, String> { + if self.duration.is_none() { + return Err("collect requires a duration".to_owned()); + } let mut samples = Vec::new(); + let mut next_tick = Instant::now(); loop { if self.is_expired() { break; } let sample = self.sample().ok_or("the monitored process exited")?; samples.push(sample); - thread::sleep(self.interval); + next_tick += self.interval; + let now = Instant::now(); + if next_tick > now { + thread::sleep(next_tick - now); + } else { + next_tick = now; + } } Ok(samples) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core/monitor.rs` around lines 64 - 76, Update Monitor::collect to reject or terminate immediately when duration is None, preserving the existing Result error contract. Replace the unconditional thread::sleep(self.interval) with scheduling based on the next intended sampling tick so sampling work does not accumulate drift; retain the current expiration check, sample collection, and process-exit error behavior.src/output/tui.rs (2)
85-87: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCollapse the nested
ifblocks to satisfy Clippy.Clippy reports
collapsible_ifat Lines 85 and 86. If the build runs with-D warnings, this fails the pipeline. Let-chains or a combined condition remove the warning.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/output/tui.rs` around lines 85 - 87, Collapse the nested event checks in the terminal event loop by combining the poll, key-event pattern match, and KeyEventKind::Press condition using a let-chain or equivalent. Preserve the existing error propagation and execute the press-event handling only when all conditions match.Source: Linters/SAST tools
24-47: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winRestore the terminal when the dashboard panics.
restore_terminalruns only on the normal return path. Ifrun_dashboard,draw, or a widget panics, raw mode stays enabled and the alternate screen stays active. The user then gets an unusable shell. A guard type that restores the terminal inDropcovers the panic path as well.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/output/tui.rs` around lines 24 - 47, The run function currently restores terminal state only after run_dashboard returns normally. Add a guard type for the initialized terminal that invokes restore_terminal in Drop, ensuring raw mode and the alternate screen are restored during panics from run_dashboard, draw, or widgets; retain the existing result/error propagation and avoid duplicate cleanup on normal completion.src/datasources/cpu_mem.rs (1)
95-106: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueUse a minimal
sysinforefresh for the name lookup.
System::new_all()enables all system data categories before this function only reads process names. A freshSystem::new()followed byrefresh_processes_specifics(..., ProcessRefreshKind::nothing())avoids loading CPU, memory, disk, and network metrics for this lookup.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/datasources/cpu_mem.rs` around lines 95 - 106, Update find_processes_by_name to construct System with System::new() instead of System::new_all(), then perform a targeted process refresh using refresh_processes_specifics with ProcessRefreshKind::nothing() before processes_by_exact_name. Import or reference the required sysinfo refresh symbols while preserving the existing result mapping.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/release.yml:
- Around line 10-11: Set the workflow-level contents permission to read, then
add a job-level contents: write permission only to the publish job. Keep the
other build jobs restricted to the read-only default.
- Around line 37-39: Update the action references in the release workflow for
actions/checkout, dtolnay/rust-toolchain, actions/upload-artifact, and
actions/download-artifact to verified 40-character commit SHAs, retaining each
current release tag in a trailing comment for update tooling.
In `@src/datasources/cpu_mem.rs`:
- Around line 37-54: Update ProcessSampler::new so the process is warmed up
before tui::run publishes its first sample: perform the initial process refresh,
wait at least sysinfo’s MINIMUM_CPU_UPDATE_INTERVAL, then refresh the process
again before returning. Preserve the existing missing-PID validation and sampler
initialization.
In `@src/output/json.rs`:
- Around line 6-11: Update write to bind the BufWriter created around the file,
pass it to serde_json::to_writer_pretty, then explicitly flush it and map any
flush failure to the existing JSON output error format. Ensure both
serialization and final buffered-write errors are propagated instead of relying
on Drop.
In `@src/output/tui.rs`:
- Around line 80-84: Update the poll-timeout calculation near last_tick and
monitor.interval() to clamp the resulting timeout to a non-zero minimum,
including while paused. Preserve the existing upper bound and ensure event::poll
never receives a zero duration.
---
Nitpick comments:
In `@src/core/monitor.rs`:
- Around line 64-76: Update Monitor::collect to reject or terminate immediately
when duration is None, preserving the existing Result error contract. Replace
the unconditional thread::sleep(self.interval) with scheduling based on the next
intended sampling tick so sampling work does not accumulate drift; retain the
current expiration check, sample collection, and process-exit error behavior.
In `@src/datasources/cpu_mem.rs`:
- Around line 95-106: Update find_processes_by_name to construct System with
System::new() instead of System::new_all(), then perform a targeted process
refresh using refresh_processes_specifics with ProcessRefreshKind::nothing()
before processes_by_exact_name. Import or reference the required sysinfo refresh
symbols while preserving the existing result mapping.
In `@src/output/tui.rs`:
- Around line 85-87: Collapse the nested event checks in the terminal event loop
by combining the poll, key-event pattern match, and KeyEventKind::Press
condition using a let-chain or equivalent. Preserve the existing error
propagation and execute the press-event handling only when all conditions match.
- Around line 24-47: The run function currently restores terminal state only
after run_dashboard returns normally. Add a guard type for the initialized
terminal that invokes restore_terminal in Drop, ensuring raw mode and the
alternate screen are restored during panics from run_dashboard, draw, or
widgets; retain the existing result/error propagation and avoid duplicate
cleanup on normal completion.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: eed592dc-e224-41a6-873a-ce7822150d6e
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (13)
.github/workflows/release.ymlCargo.tomlreadme.mdsrc/core/mod.rssrc/core/monitor.rssrc/datasources/cpu_mem.rssrc/lib.rssrc/main.rssrc/output/csv.rssrc/output/json.rssrc/output/mod.rssrc/output/tui.rstests/cpu_mem_tests.rs
| permissions: | ||
| contents: write |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Limit contents: write to publish.
Line 11 grants repository write access to all four build jobs. A compromised build step can modify repository contents or release state. Set the workflow default to contents: read, then grant contents: write only to publish. GitHub recommends read-only default permissions and job-specific escalation. (docs.github.com)
Proposed fix
permissions:
- contents: write
+ contents: read
...
publish:
name: Publish GitHub release
+ permissions:
+ contents: write
needs: build🧰 Tools
🪛 zizmor (1.29.0)
[error] 11-11: overly broad permissions (excessive-permissions): contents: write is overly broad at the workflow level
(excessive-permissions)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/release.yml around lines 10 - 11, Set the workflow-level
contents permission to read, then add a job-level contents: write permission
only to the publish job. Keep the other build jobs restricted to the read-only
default.
Source: Linters/SAST tools
| - uses: actions/checkout@v4 | ||
|
|
||
| - uses: dtolnay/rust-toolchain@stable |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== workflow excerpt =="
sed -n '1,110p' .github/workflows/release.yml | cat -n
echo
echo "== action uses in release workflow =="
rg -n "uses:" .github/workflows/release.ymlRepository: imsudiproy/uniproc
Length of output: 3869
Pin each action to a verified full commit SHA.
Lines 37, 39, 58, 69, and 72 use mutable action tags for actions/checkout, dtolnay/rust-toolchain, actions/upload-artifact, and actions/download-artifact. Pin these actions to verified 40-character commit SHAs; keep the release tag in a trailing comment for update tooling. Use a full commit SHA, which is an immutable GitHub Actions reference.
Also applies to: 58, 69, 72
🧰 Tools
🪛 zizmor (1.29.0)
[warning] 37-37: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/release.yml around lines 37 - 39, Update the action
references in the release workflow for actions/checkout, dtolnay/rust-toolchain,
actions/upload-artifact, and actions/download-artifact to verified 40-character
commit SHAs, retaining each current release tag in a trailing comment for update
tooling.
| pub fn new(pid: u32) -> Result<Self, String> { | ||
| let pid = Pid::from_u32(pid); | ||
| let mut system = System::new(); | ||
| system.refresh_memory(); | ||
| system.refresh_processes_specifics( | ||
| ProcessesToUpdate::Some(&[pid]), | ||
| true, | ||
| ProcessRefreshKind::everything(), | ||
| ); | ||
| if system.process(pid).is_none() { | ||
| return Err(format!("process with PID {pid} was not found")); | ||
| } | ||
| Ok(Self { | ||
| pid, | ||
| system, | ||
| networks: Networks::new_with_refreshed_list(), | ||
| }) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
sysinfo 0.37 MINIMUM_CPU_UPDATE_INTERVAL process cpu_usage two refreshes required
💡 Result:
In the sysinfo crate (version 0.37), CPU usage calculation is based on the difference between two measurements taken over a period of time [1][2][3]. Because of this, a single call to a refresh method is insufficient to calculate a non-zero, accurate percentage; the system requires at least two refreshes with an interval between them to compute the delta [1][2][3]. The constant MINIMUM_CPU_UPDATE_INTERVAL is provided by sysinfo as a recommended minimum wait time between these two calls [4][5]. Currently, this value is set to 200 milliseconds [6]. If refreshes occur too frequently—below this interval—the calculated CPU usage may incorrectly return 0% or, on some platforms like Linux, report maximum values [4]. To obtain accurate CPU usage, follow this pattern: 1. Initialize your System object [1][2]. 2. Perform an initial refresh [7]. 3. Wait for at least the duration specified by MINIMUM_CPU_UPDATE_INTERVAL using std::thread::sleep [1][2][7]. 4. Perform a second refresh [1][2][7]. 5. Access the cpu_usage value [1][3]. This requirement applies to both global CPU usage and individual process CPU usage [1][2]. Using a sleep duration shorter than the minimum interval can lead to inaccurate data, as the internal time-diff calculation will not have sufficient time elapsed to produce a valid measurement [4][2][3].
Citations:
- 1: https://docs.rs/sysinfo/latest/sysinfo/struct.Process.html
- 2: https://docs.rs/sysinfo/latest/sysinfo/struct.System.html
- 3: https://docs.rs/sysinfo/latest/sysinfo/struct.Cpu.html
- 4: https://docs.rs/sysinfo/latest/sysinfo/constant.MINIMUM_CPU_UPDATE_INTERVAL.html
- 5: https://docs.rs/sysinfo/latest/sysinfo/index.html
- 6: https://github.com/GuillaumeGomez/sysinfo/blob/master/src/unix/linux/system.rs
- 7: https://eliteai.tools/agent-skills/sysinfo-2
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== rust manifest =="
git ls-files | rg '(^|/)Cargo\.toml$|(^|/)Cargo\.lock$' || true
for f in $(git ls-files | rg '(^|/)Cargo\.toml$'); do
echo "--- $f ---"
rg -n 'sysinfo|name = "cpu_mem"|process_info|ProcessSampler|Monitor|sample' "$f" -C 2 || true
done
echo "== source files matching candidate names =="
git ls-files | rg '(^|/)cpu_mem\.rs$|(^|/).*monitor.*\.rs$|(^|/)tui.*\.rs$' || true
for f in $(git ls-files | rg '(^|/)cpu_mem\.rs$'); do
echo "--- $f ---"
wc -l "$f"
cat -n "$f" | sed -n '1,140p'
done
echo "== sample/call-site references =="
rg -n 'ProcessSampler::new|process_sample|sample\(|\.cpu_usage\(\)|MINIMUM_CPU_UPDATE_INTERVAL' $(git ls-files '*.rs') -C 3Repository: imsudiproy/uniproc
Length of output: 9244
🌐 Web query:
docs.rs sysinfo 0.37.0 Process cpu_usage documentation MINIMUM_CPU_UPDATE_INTERVAL
💡 Result:
In the sysinfo crate (including version 0.37.0), MINIMUM_CPU_UPDATE_INTERVAL is a constant defined as the minimum interval time used internally by the library to refresh CPU-related information [1][2]. Its primary purpose is to ensure that CPU usage calculations—which rely on measuring the difference in process or system time over a specific period—remain accurate [3][4][2]. Because CPU usage is computed by dividing a process's time usage by the total system time over a given interval, attempting to refresh this data too frequently can lead to inaccurate results (e.g., returning 0% or, on some systems like Linux, an artificially high maximum value) [3][2]. Key usage guidelines for MINIMUM_CPU_UPDATE_INTERVAL: 1. Recommended Delay: Developers are advised to use this constant with std::thread::sleep or similar timing mechanisms when looping to refresh CPU usage [5][3][4]. 2. Accuracy Requirement: Because computation is based on time differentials, a process or system must be refreshed at least twice with sufficient time between calls to obtain meaningful usage data [3][4]. 3. Platform Variation: The specific duration of this constant is platform-dependent, as noted in the official documentation [2]. In practice, you should incorporate this interval when implementing refresh loops for both system-wide CPU usage (via System::refresh_cpu_usage) and specific process CPU usage (via Process::cpu_usage) [5][3][4].
Citations:
- 1: https://docs.rs/sysinfo/latest/sysinfo/index.html
- 2: https://docs.rs/sysinfo/latest/sysinfo/constant.MINIMUM_CPU_UPDATE_INTERVAL.html
- 3: https://docs.rs/sysinfo/latest/sysinfo/struct.Process.html
- 4: https://docs.rs/sysinfo/latest/sysinfo/struct.System.html
- 5: https://docs.rs/crate/sysinfo/0.37.0
Warm up ProcessSampler before returning the first sample.
sysinfo computes process CPU usage from two refreshes with at least MINIMUM_CPU_UPDATE_INTERVAL between them. ProcessSampler::new performs only the first refresh, but tui::run publishes the first sample() immediately. That sample can return cpu_percent: 0.0 or another unreliable value. Add a wait before the first sample return, or skip/discard the first sample.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/datasources/cpu_mem.rs` around lines 37 - 54, Update ProcessSampler::new
so the process is warmed up before tui::run publishes its first sample: perform
the initial process refresh, wait at least sysinfo’s
MINIMUM_CPU_UPDATE_INTERVAL, then refresh the process again before returning.
Preserve the existing missing-PID validation and sampler initialization.
| pub fn write(path: impl AsRef<Path>, samples: &[ProcessInfo]) -> Result<(), String> { | ||
| let file = | ||
| File::create(path.as_ref()).map_err(|e| format!("cannot create JSON output: {e}"))?; | ||
| serde_json::to_writer_pretty(BufWriter::new(file), samples) | ||
| .map_err(|e| format!("cannot write JSON output: {e}")) | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Flush the buffered writer explicitly to detect write failures.
serde_json::to_writer_pretty does not flush the BufWriter. The BufWriter is a temporary here, so it is dropped at the end of the expression. Drop flushes the remaining buffer but discards any error. If the final write fails, for example when the disk is full, write returns Ok(()) and src/main.rs prints a success message for a truncated file. Bind the writer and flush it explicitly.
🐛 Proposed fix
+use std::io::Write;
+
pub fn write(path: impl AsRef<Path>, samples: &[ProcessInfo]) -> Result<(), String> {
let file =
File::create(path.as_ref()).map_err(|e| format!("cannot create JSON output: {e}"))?;
- serde_json::to_writer_pretty(BufWriter::new(file), samples)
- .map_err(|e| format!("cannot write JSON output: {e}"))
+ let mut writer = BufWriter::new(file);
+ serde_json::to_writer_pretty(&mut writer, samples)
+ .map_err(|e| format!("cannot write JSON output: {e}"))?;
+ writer
+ .flush()
+ .map_err(|e| format!("cannot write JSON output: {e}"))
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| pub fn write(path: impl AsRef<Path>, samples: &[ProcessInfo]) -> Result<(), String> { | |
| let file = | |
| File::create(path.as_ref()).map_err(|e| format!("cannot create JSON output: {e}"))?; | |
| serde_json::to_writer_pretty(BufWriter::new(file), samples) | |
| .map_err(|e| format!("cannot write JSON output: {e}")) | |
| } | |
| use std::io::Write; | |
| pub fn write(path: impl AsRef<Path>, samples: &[ProcessInfo]) -> Result<(), String> { | |
| let file = | |
| File::create(path.as_ref()).map_err(|e| format!("cannot create JSON output: {e}"))?; | |
| let mut writer = BufWriter::new(file); | |
| serde_json::to_writer_pretty(&mut writer, samples) | |
| .map_err(|e| format!("cannot write JSON output: {e}"))?; | |
| writer | |
| .flush() | |
| .map_err(|e| format!("cannot write JSON output: {e}")) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/output/json.rs` around lines 6 - 11, Update write to bind the BufWriter
created around the file, pass it to serde_json::to_writer_pretty, then
explicitly flush it and map any flush failure to the existing JSON output error
format. Ensure both serialization and final buffered-write errors are propagated
instead of relying on Drop.
| let elapsed = last_tick.elapsed(); | ||
| let timeout = monitor | ||
| .interval() | ||
| .saturating_sub(elapsed) | ||
| .min(Duration::from_millis(100)); |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
Pausing the dashboard causes a busy loop.
last_tick is updated only when a new sample is taken (Line 117). While paused is true, no sample is taken, so elapsed keeps growing. interval().saturating_sub(elapsed) then returns zero, event::poll returns immediately, and the loop redraws continuously. The process consumes a full CPU core for as long as the dashboard stays paused. This also inflates the CPU reading of the monitored process when the user monitors uniproc itself.
Clamp the poll timeout to a non-zero minimum.
🐛 Proposed fix for the poll timeout
let elapsed = last_tick.elapsed();
let timeout = monitor
.interval()
.saturating_sub(elapsed)
- .min(Duration::from_millis(100));
+ .min(Duration::from_millis(100))
+ .max(Duration::from_millis(10));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let elapsed = last_tick.elapsed(); | |
| let timeout = monitor | |
| .interval() | |
| .saturating_sub(elapsed) | |
| .min(Duration::from_millis(100)); | |
| let elapsed = last_tick.elapsed(); | |
| let timeout = monitor | |
| .interval() | |
| .saturating_sub(elapsed) | |
| .min(Duration::from_millis(100)) | |
| .max(Duration::from_millis(10)); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/output/tui.rs` around lines 80 - 84, Update the poll-timeout calculation
near last_tick and monitor.interval() to clamp the resulting timeout to a
non-zero minimum, including while paused. Preserve the existing upper bound and
ensure event::poll never receives a zero duration.
1 participant
Summary by CodeRabbit
New Features
Documentation
Bug Fixes