Conversation
WalkthroughThis PR standardizes unavailable per-process metrics using sentinel -1 and propagates that across frontend and backend types and formatting. It changes ProcInfo VmRSS to int64 and initializes CPU/memory/thread fields to -1 on Darwin, Linux, and Windows. The RPC layer now uses WidgetId, LastPidOrder, and KeepAlive, returns Gone markers for missing PIDs, and maintains per-widget PID ordering with idle expiration. The frontend view updates formatting (CPU/memory/threads display and tooltips), adds configurable fetchInterval and fetchEpoch-based stale-response protection, implements keep-alive and connection-change handling, updates polling/paused behavior, and exposes a Refresh Interval UI. Mock previews and scrollbar CSS were also adjusted. Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes 🚥 Pre-merge checks | ❌ 3❌ Failed checks (1 warning, 2 inconclusive)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 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 |
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| {showThreads && ( | ||
| <div className="px-2 flex items-center truncate justify-end text-secondary font-mono text-[11px]"> | ||
| {proc.numthreads >= 1 ? proc.numthreads : ""} | ||
| {proc.numthreads === -1 ? "-" : proc.numthreads >= 1 ? proc.numthreads : ""} |
There was a problem hiding this comment.
WARNING: Processes with gone: true (dead processes returned by backend) are not handled. When gone is true, fields like command, user, status, etc. are all empty, but this component still renders them as empty rows instead of filtering them out or showing them differently.
| sort.Slice(processes, func(i, j int) bool { | ||
| if sortDesc { | ||
| return processes[i].Mem > processes[j].Mem | ||
| if processes[i].Mem != processes[j].Mem { |
There was a problem hiding this comment.
WARNING: Memory sort doesn't handle -1 (unavailable) values. Unlike CPU sort which converts -1 to 0 for comparison (line 399-404), Mem sort directly compares Mem values. If both processes have -1, they would be equal, but if one has a real value and one has -1, it uses the raw values which may cause inconsistent sorting behavior.
| sort.Slice(processes, func(i, j int) bool { | ||
| if sortDesc { | ||
| return processes[i].NumThreads > processes[j].NumThreads | ||
| if processes[i].NumThreads != processes[j].NumThreads { |
There was a problem hiding this comment.
WARNING: Threads sort doesn't handle -1 (unavailable) values. Similar to Mem sort, this compares NumThreads directly without checking for -1, which could cause inconsistent sorting when some processes have unavailable thread counts.
Code Review SummaryStatus: No Issues Found | Recommendation: Merge OverviewAll previous issues have been resolved in the latest commits:
Changes Since Last ReviewThe following issues from the previous review have been addressed:
Files Reviewed (4 files)
Summary: This PR is now ready for merge. All previous issues have been addressed:
Reviewed by minimax-m2.5-20260211 · 1,079,372 tokens |
Deploying waveterm with
|
| Latest commit: |
7a6fb89
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://d487e283.waveterm.pages.dev |
| Branch Preview URL: | https://sawka-processviewer-2.waveterm.pages.dev |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (1)
pkg/wshrpc/wshremote/processviewer.go (1)
119-132: Consider copying the slice to prevent aliasing.The
pidsslice is stored directly without copying. If the caller later modifies the slice, the cached ordering could be corrupted. However, reviewing line 509-512, the caller creates a new slice specifically for storage, so this is safe in practice.Optional: Defensive copy
func (s *procCacheState) storeWidgetPidOrder(widgetId string, pids []int32) { if widgetId == "" { return } s.lock.Lock() defer s.lock.Unlock() if s.widgetPidOrders == nil { s.widgetPidOrders = make(map[string]*widgetPidOrder) } + pidsCopy := make([]int32, len(pids)) + copy(pidsCopy, pids) s.widgetPidOrders[widgetId] = &widgetPidOrder{ - pids: pids, + pids: pidsCopy, lastRequest: time.Now(), } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/wshrpc/wshremote/processviewer.go` around lines 119 - 132, storeWidgetPidOrder stores the incoming pids slice directly which can lead to aliasing if the caller mutates the slice later; to fix, make a defensive copy of the pids slice before assigning it into s.widgetPidOrders (in function storeWidgetPidOrder) so the stored widgetPidOrder.pids is independent of the caller's slice, then proceed to set lastRequest and insert into the map as currently done.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@frontend/app/view/processviewer/processviewer.tsx`:
- Around line 858-865: The effect uses model.forceRefreshOnConnectionChange()
but model is missing from the dependency array; update the React.useEffect
dependency array to include model (keep isFirstRender ref logic unchanged) so
the effect depends on both connection and model, ensuring exhaustive-deps
compliance and stable behavior for the isFirstRender check and the call to
model.forceRefreshOnConnectionChange().
In `@frontend/preview/previews/processviewer.preview.tsx`:
- Around line 13-27: The mock ProcessInfo literals are missing the required
numthreads field so they fail type-checking; update every mocked row in the
preview data (the array of ProcessInfo objects) to include numthreads: -1 (use
the -1 sentinel consistently) so each object conforms to the ProcessInfo type;
ensure every object (e.g., entries with pid 1, 123, 456, etc.) gets the
numthreads property added.
In `@pkg/util/procinfo/procinfo_darwin.go`:
- Around line 90-99: The Darwin path sets info.NumThreads only on successful
getDarwinProcTaskInfo but leaves the default zero when the call fails; change
the fallback to use the documented sentinel by setting info.NumThreads = -1 in
the unhappy path (where getDarwinProcTaskInfo returns an error) so that
NumThreads is -1 when unavailable, mirroring the behavior described in
pkg/util/procinfo/procinfo.go.
In `@pkg/util/procinfo/procinfo_linux.go`:
- Around line 121-123: The code currently skips setting info.VmRSS when the
parsed rssPages is zero; change the guard so a parsed value of zero is treated
as valid. In the procinfo_linux.go parsing block that reads rest[21] into
rssPages, update the conditional that sets info.VmRSS (referencing rssPages,
info.VmRSS, pageSize and rest[21]) to accept zero values (e.g., use err == nil
&& rssPages >= 0 or simply err == nil and validate non-negative) so processes
with 0 RSS are recorded as 0 bytes rather than "unknown".
In `@pkg/wshrpc/wshremote/processviewer.go`:
- Around line 490-516: When using a cached pid order
(procCache.getWidgetPidOrder) we never set filteredCount so it stays 0; fix by
assigning filteredCount = len(pidOrder) immediately after retrieving pidOrder
from the cache (i.e., after the procCache.getWidgetPidOrder call inside the
data.LastPidOrder branch) so the response uses the correct filtered count when
serving cached orders.
In `@pkg/wshrpc/wshrpctypes.go`:
- Around line 914-927: The ProcessInfo struct now requires sentinel values for
unavailable metrics, but the code still creates dead-process entries with
ProcessInfo{Pid: pid, Gone: true}, which leaves Mem/MemPct/Cpu/NumThreads as
zeros; update that construction in the process viewer (the code that builds
ProcessInfo for gone processes in processviewer.go) to explicitly set Mem: -1,
MemPct: -1, Cpu: -1, NumThreads: -1 along with Gone: true (and keep Pid/other
fields as before) so dead processes serialize with the intended -1 sentinels
rather than zeros.
---
Nitpick comments:
In `@pkg/wshrpc/wshremote/processviewer.go`:
- Around line 119-132: storeWidgetPidOrder stores the incoming pids slice
directly which can lead to aliasing if the caller mutates the slice later; to
fix, make a defensive copy of the pids slice before assigning it into
s.widgetPidOrders (in function storeWidgetPidOrder) so the stored
widgetPidOrder.pids is independent of the caller's slice, then proceed to set
lastRequest and insert into the map as currently done.
🪄 Autofix (Beta)
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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 775e57c2-0827-47b4-9fdb-433fe2eeed5b
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (10)
frontend/app/view/processviewer/processviewer.tsxfrontend/preview/previews/processviewer.preview.tsxfrontend/tailwindsetup.cssfrontend/types/gotypes.d.tspkg/util/procinfo/procinfo.gopkg/util/procinfo/procinfo_darwin.gopkg/util/procinfo/procinfo_linux.gopkg/util/procinfo/procinfo_windows.gopkg/wshrpc/wshremote/processviewer.gopkg/wshrpc/wshrpctypes.go
| const isFirstRender = React.useRef(true); | ||
| React.useEffect(() => { | ||
| if (isFirstRender.current) { | ||
| isFirstRender.current = false; | ||
| return; | ||
| } | ||
| model.forceRefreshOnConnectionChange(); | ||
| }, [connection]); |
There was a problem hiding this comment.
Minor: model missing from dependency array.
The model reference is used inside the effect but isn't in the dependency array. While it's likely stable, adding it would satisfy React's exhaustive-deps rule.
Suggested fix
React.useEffect(() => {
if (isFirstRender.current) {
isFirstRender.current = false;
return;
}
model.forceRefreshOnConnectionChange();
- }, [connection]);
+ }, [connection, model]);📝 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.
| const isFirstRender = React.useRef(true); | |
| React.useEffect(() => { | |
| if (isFirstRender.current) { | |
| isFirstRender.current = false; | |
| return; | |
| } | |
| model.forceRefreshOnConnectionChange(); | |
| }, [connection]); | |
| const isFirstRender = React.useRef(true); | |
| React.useEffect(() => { | |
| if (isFirstRender.current) { | |
| isFirstRender.current = false; | |
| return; | |
| } | |
| model.forceRefreshOnConnectionChange(); | |
| }, [connection, model]); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/app/view/processviewer/processviewer.tsx` around lines 858 - 865,
The effect uses model.forceRefreshOnConnectionChange() but model is missing from
the dependency array; update the React.useEffect dependency array to include
model (keep isFirstRender ref logic unchanged) so the effect depends on both
connection and model, ensuring exhaustive-deps compliance and stable behavior
for the isFirstRender check and the call to
model.forceRefreshOnConnectionChange().
| { pid: 1, ppid: 0, command: "launchd", user: "root", cpu: 0.0, mem: 4096 * 1024, mempct: 0.01 }, | ||
| { pid: 123, ppid: 1, command: "kernel_task", user: "root", cpu: 12.3, mem: 2048 * 1024 * 1024, mempct: 6.25 }, | ||
| { pid: 456, ppid: 1, command: "WindowServer", user: "_windowserver", cpu: 5.1, mem: 512 * 1024 * 1024, mempct: 1.56 }, | ||
| { pid: 789, ppid: 1, command: "node", user: "mike", cpu: 8.7, mem: 256 * 1024 * 1024, mempct: 0.78 }, | ||
| { pid: 1001, ppid: 1, command: "Electron", user: "mike", cpu: 3.2, mem: 400 * 1024 * 1024, mempct: 1.22 }, | ||
| { pid: 1234, ppid: 1001, command: "waveterm-helper", user: "mike", cpu: 0.5, mem: 64 * 1024 * 1024, mempct: 0.20 }, | ||
| { pid: 2001, ppid: 1, command: "sshd", user: "root", cpu: 0.0, mem: 8 * 1024 * 1024, mempct: 0.02 }, | ||
| { pid: 2345, ppid: 1, command: "postgres", user: "postgres", cpu: 1.2, mem: 128 * 1024 * 1024, mempct: 0.39 }, | ||
| { pid: 3001, ppid: 1, command: "nginx", user: "_www", cpu: 0.3, mem: 32 * 1024 * 1024, mempct: 0.10 }, | ||
| { pid: 3456, ppid: 1, command: "python3", user: "mike", cpu: 2.8, mem: 96 * 1024 * 1024, mempct: 0.29 }, | ||
| { pid: 4001, ppid: 1, command: "docker", user: "root", cpu: 0.1, mem: 48 * 1024 * 1024, mempct: 0.15 }, | ||
| { pid: 4567, ppid: 4001, command: "containerd", user: "root", cpu: 0.2, mem: 80 * 1024 * 1024, mempct: 0.24 }, | ||
| { pid: 5001, ppid: 1, command: "zsh", user: "mike", cpu: 0.0, mem: 6 * 1024 * 1024, mempct: 0.02 }, | ||
| { pid: 5678, ppid: 5001, command: "vim", user: "mike", cpu: 0.0, mem: 20 * 1024 * 1024, mempct: 0.06 }, | ||
| { pid: 6001, ppid: 1, command: "coreaudiod", user: "_coreaudiod", cpu: 0.4, mem: 16 * 1024 * 1024, mempct: 0.05 }, |
There was a problem hiding this comment.
Populate numthreads in every mock ProcessInfo.
ProcessInfo now requires numthreads (frontend/types/gotypes.d.ts:1265-1277), so these literals no longer type-check. If the preview does not care about thread counts, use the new -1 sentinel consistently.
🧪 Minimal fix
- { pid: 1, ppid: 0, command: "launchd", user: "root", cpu: 0.0, mem: 4096 * 1024, mempct: 0.01 },
+ { pid: 1, ppid: 0, command: "launchd", user: "root", cpu: 0.0, mem: 4096 * 1024, mempct: 0.01, numthreads: -1 },Apply the same numthreads field to the remaining mock rows.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/preview/previews/processviewer.preview.tsx` around lines 13 - 27,
The mock ProcessInfo literals are missing the required numthreads field so they
fail type-checking; update every mocked row in the preview data (the array of
ProcessInfo objects) to include numthreads: -1 (use the -1 sentinel
consistently) so each object conforms to the ProcessInfo type; ensure every
object (e.g., entries with pid 1, 123, 456, etc.) gets the numthreads property
added.
| // ProcessInfo holds per-process information for the process viewer. | ||
| // Mem, MemPct, Cpu, and NumThreads are set to -1 when the data is unavailable | ||
| // (e.g. permission denied reading another user's process on macOS). | ||
| type ProcessInfo struct { | ||
| Pid int32 `json:"pid"` | ||
| Ppid int32 `json:"ppid,omitempty"` | ||
| Command string `json:"command,omitempty"` | ||
| Status string `json:"status,omitempty"` | ||
| User string `json:"user,omitempty"` | ||
| Mem uint64 `json:"mem,omitempty"` | ||
| MemPct float64 `json:"mempct,omitempty"` | ||
| Cpu *float64 `json:"cpu,omitempty"` | ||
| NumThreads int32 `json:"numthreads,omitempty"` | ||
| Pid int32 `json:"pid"` | ||
| Ppid int32 `json:"ppid,omitempty"` | ||
| Command string `json:"command,omitempty"` | ||
| Status string `json:"status,omitempty"` | ||
| User string `json:"user,omitempty"` | ||
| Mem int64 `json:"mem"` // resident set size in bytes; -1 if unavailable | ||
| MemPct float64 `json:"mempct"` // memory percent; -1 if unavailable | ||
| Cpu float64 `json:"cpu"` // cpu percent; -1 if unavailable | ||
| NumThreads int32 `json:"numthreads"` // -1 if unavailable | ||
| Gone bool `json:"gone,omitempty"` |
There was a problem hiding this comment.
Gone placeholders still need sentinel metrics.
This contract now says mem/mempct/cpu/numthreads are required and use -1 when unavailable, but pkg/wshrpc/wshremote/processviewer.go:535-545 still constructs ProcessInfo{Pid: pid, Gone: true}. Those rows will now serialize as zeros, making dead processes look like live zero-usage entries.
🧩 Follow-up needed in pkg/wshrpc/wshremote/processviewer.go
- processes = append(processes, wshrpc.ProcessInfo{Pid: pid, Gone: true})
+ processes = append(processes, wshrpc.ProcessInfo{
+ Pid: pid,
+ Mem: -1,
+ MemPct: -1,
+ Cpu: -1,
+ NumThreads: -1,
+ Gone: true,
+ })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/wshrpc/wshrpctypes.go` around lines 914 - 927, The ProcessInfo struct now
requires sentinel values for unavailable metrics, but the code still creates
dead-process entries with ProcessInfo{Pid: pid, Gone: true}, which leaves
Mem/MemPct/Cpu/NumThreads as zeros; update that construction in the process
viewer (the code that builds ProcessInfo for gone processes in processviewer.go)
to explicitly set Mem: -1, MemPct: -1, Cpu: -1, NumThreads: -1 along with Gone:
true (and keep Pid/other fields as before) so dead processes serialize with the
intended -1 sentinels rather than zeros.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
frontend/app/view/processviewer/processviewer.tsx (1)
239-269: Consider making keep-alive interval configurable or a constant.The 10-second keep-alive interval is hardcoded on line 252. While functional, extracting this to a named constant would improve readability and make future adjustments easier.
🧹 Optional: Extract constant
+const KeepAliveInterval = 10000; + // In startKeepAlive: - const timer = setTimeout(resolve, 10000); + const timer = setTimeout(resolve, KeepAliveInterval);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/app/view/processviewer/processviewer.tsx` around lines 239 - 269, Extract the hardcoded 10000ms sleep in startKeepAlive into a named constant (e.g., KEEP_ALIVE_INTERVAL_MS) or a configurable option so the interval is easy to find and change; update the startKeepAlive function (and the inner keepAliveLoop) to use that constant instead of the literal, and ensure any references to cancelPoll and timer logic remain unchanged so behavior is preserved while improving readability and configurability.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@frontend/app/view/processviewer/processviewer.tsx`:
- Around line 239-269: Extract the hardcoded 10000ms sleep in startKeepAlive
into a named constant (e.g., KEEP_ALIVE_INTERVAL_MS) or a configurable option so
the interval is easy to find and change; update the startKeepAlive function (and
the inner keepAliveLoop) to use that constant instead of the literal, and ensure
any references to cancelPoll and timer logic remain unchanged so behavior is
preserved while improving readability and configurability.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 343b991d-4062-48ff-9603-53fb1dc3c8b7
📒 Files selected for processing (4)
frontend/app/view/processviewer/processviewer.tsxpkg/util/procinfo/procinfo_darwin.gopkg/util/procinfo/procinfo_linux.gopkg/wshrpc/wshremote/processviewer.go
✅ Files skipped from review due to trivial changes (1)
- pkg/wshrpc/wshremote/processviewer.go
1 participant
No description provided.