Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughAdds a cross-platform process viewer feature across frontend and backend. Introduces new process data types (TS and Go), OS-specific procinfo implementations (Linux, Darwin, Windows), a proc cache and RPC handlers for listing/signal operations, client RPC wrappers, and a React ProcessViewer view with its ViewModel, preview mocks, and block registry updates. Also adds utilities for sending signals by name, Go process-info helpers, and updates go.mod and documentation snippets. No existing public APIs were removed. Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes 🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ 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 |
Deploying waveterm with
|
| Latest commit: |
da8c36b
|
| Status: | ⚡️ Build in progress... |
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
|
|
||
| const route = makeConnRoute(conn); | ||
| try { | ||
| console.log("RemoteProcessList", sortBy, sortDesc, start, limit, textSearch); |
There was a problem hiding this comment.
CRITICAL: Debug console.log statement left in production code - should be removed before merging
Code Review SummaryStatus: No Issues Found | Recommendation: Merge OverviewThe console.log debug statement at line 150 has been removed. All other changes are improvements.
Changes Reviewed (2 files)
Resolved Issues (from previous review)
Reviewed by minimax-m2.5-20260211 · 230,061 tokens |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (2)
pkg/util/unixutil/unixutil_unix.go (1)
83-94: Minor: Error message may be misleading on Unix.On Unix systems,
os.FindProcess(pid)always succeeds and returns a Process struct—it doesn't actually verify the process exists. The real check happens when callingp.Signal(sig). The error message "process %d not found" at line 91 is unlikely to trigger on Unix (the error would come from line 93 instead).Consider simplifying since
os.FindProcesson Unix only fails forpid <= 0:♻️ Suggested simplification
func SendSignalByName(pid int, sigName string) error { sig := ParseSignal(sigName) if sig == nil { return fmt.Errorf("unsupported or invalid signal %q", sigName) } p, err := os.FindProcess(pid) if err != nil { - return fmt.Errorf("process %d not found: %w", pid, err) + return fmt.Errorf("invalid pid %d: %w", pid, err) } - return p.Signal(sig) + if err := p.Signal(sig); err != nil { + return fmt.Errorf("failed to send signal %s to process %d: %w", sigName, pid, err) + } + return nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/util/unixutil/unixutil_unix.go` around lines 83 - 94, The SendSignalByName function currently treats os.FindProcess errors as "process not found", but on Unix FindProcess rarely fails (only for pid <= 0) and the real errors come from p.Signal; update SendSignalByName to validate pid > 0 up front (returning a clear error for invalid pid), call ParseSignal as before, call os.FindProcess without assuming non-existence, and rely on p.Signal(sig) to surface real runtime errors (or wrap the p.Signal error with context). Reference: SendSignalByName, ParseSignal, os.FindProcess, p.Signal.pkg/wshrpc/wshremote/processviewer.go (1)
184-201: Avoid spawning one goroutine per process on every poll.This launches one goroutine per PID every second. On busy hosts that can mean thousands of concurrent
GetProcInfocalls and a noticeable load spike from the viewer itself. A bounded worker pool keyed offnumCPU/GOMAXPROCSwould keep sampling cost predictable.🤖 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 184 - 201, The current loop spawns one goroutine per process which can create thousands of concurrent procinfo.GetProcInfo calls; replace it with a bounded worker pool (size based on runtime.GOMAXPROCS(0) or runtime.NumCPU) that consumes jobs from a channel of tasks containing (index i, p.Pid) and writes results back into rawInfos[index] as pidInfo; ensure workers use the same panic handling (panichandler.PanicHandler("collectSnapshot:GetProcInfo", ...)), use a WaitGroup to wait for all jobs to finish, propagate nil on error from procinfo.GetProcInfo, and close the jobs channel to signal workers to exit so sampling cost stays predictable.
🤖 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 239-241: The keyDownHandler currently only checks for "Cmd:f" so
the UI hint for non-macOS (Alt-F) never triggers on Windows/Linux; update the
handler(s) (keyDownHandler and the similar handler around the second occurrence)
to check the appropriate modifier(s) for non-macOS — e.g., detect platform or
test both "Cmd:f" and "Alt:f" (or "Ctrl:f" if your keyutil expects Ctrl on
Windows) and call this.openSearch() when either variant matches, ensuring both
shortcut hints and behavior stay in sync.
- Around line 224-226: The virtualization height is being computed from the
unfiltered totalcount causing the scroller to remain sized to the full dataset
and allowing a deep scrollTop to request empty pages; in setTextSearch (which
writes textSearchAtom and calls triggerRefresh) update the code that computes
totalHeight/virtualization size to use filteredcount instead of totalcount and
also reset the scroller position (clear scrollTop / set to 0) when the search
text changes so the view jumps to the top of the newly filtered results; locate
uses of totalHeight, totalcount and any scrollTop manipulation in the
virtualized list rendering and update them accordingly (also apply same change
around the other affected block referenced at lines ~769-797).
- Around line 202-209: triggerRefresh currently returns early when paused
(this.pausedAtom) which prevents user-driven one-shot updates
(scrollTop/containerHeight/textSearch) from fetching the correct page; change
the logic in triggerRefresh (and the similar blocks around the other occurrences
noted) so that pausing only disables the recurring timer
(cancelPoll/startPolling) but does not prevent immediate fetches for user
actions—i.e., if the call was triggered by a user-driven event
(scroll/search/resize) or if parameters affecting server pagination changed,
still call the routine that performs a single fetch/update (the same code
startPolling would use for one-shot fetches or the page-fetch method used in the
polling loop) while keeping cancelPoll cleared and the periodic poll disabled
when paused. Ensure references: triggerRefresh, startPolling, cancelPoll,
pausedAtom, and the fetch/update routine are updated consistently in the other
blocks around the noted occurrences.
In `@frontend/preview/previews/processviewer.preview.tsx`:
- Around line 40-67: makeMockProcessListResponse currently ignores
data.textsearch so previews never exercise filtering and filteredcount is wrong;
update makeMockProcessListResponse to apply the text search (data.textsearch) to
MockProcesses first (produce a filtered list), keep totalcount as the original
MockProcesses.length, then sort and slice the filtered list (use the same sort
logic and variables like sortBy, sortDesc, start, limit) and set filteredcount
to the filtered list length before paging; reference
makeMockProcessListResponse, MockProcesses, data.textsearch, filteredcount and
totalcount when making the change.
- Around line 82-84: The preview currently overrides only
RemoteProcessListCommand via useRpcOverride and thus ignores kill/stop/resume
actions; add a corresponding useRpcOverride for RemoteProcessSignalCommand (or
update the existing override set) to either simulate the signal effects on the
mock process state (e.g., update the mock returned by
makeMockProcessListResponse) or explicitly reject the RPC so the UI doesn't
report a spurious success; reference the existing useRpcOverride call and
makeMockProcessListResponse to locate where to add the
RemoteProcessSignalCommand handler and ensure it returns a realistic mocked
response or a rejected Promise.
In `@pkg/util/procinfo/procinfo_darwin.go`:
- Around line 84-89: The ProcInfo construction uses k.Proc.P_comm which is
truncated at 16 chars; update the logic that sets ProcInfo.Command (in
pkg/util/procinfo/procinfo_darwin.go) to attempt retrieving a full command name
via libproc (e.g. proc_name or proc_pidpath) using the process PID
(k.Proc.P_pid) through the existing purego binding mechanism, and only fall back
to unix.ByteSliceToString(k.Proc.P_comm[:]) if those libproc calls fail or
return empty; ensure the new code populates ProcInfo.Command with the full
path/name when available and preserves the existing behavior on error.
In `@pkg/util/procinfo/procinfo_linux.go`:
- Around line 15-17: Replace the hard-coded const userHz = 100.0 with a runtime
lookup of CLK_TCK and use that value where CPU ticks are converted (references:
userHz and the code that divides utime/stime at lines ~110-111); implement a
helper like getUserHz() that calls unix.Sysconf(unix._SC_CLK_TCK) (from
golang.org/x/sys/unix), converts the returned tick count to float64, caches it
for subsequent calls, and falls back to 100.0 only if Sysconf returns an error
or non-positive value; update usages to call this helper (or read the cached
value) instead of the constant.
- Around line 71-73: The current parsing sets ProcInfo.Command from the
truncated comm value parsed as comm := s[lp+1 : rp]; instead, read
/proc/[pid]/cmdline for the full command (split NUL bytes and join with spaces)
and set ProcInfo.Command from that; if /proc/[pid]/cmdline is empty or
unavailable, fall back to the existing comm value (trim parentheses) as a
secondary source. Update the parsing logic in procinfo_linux.go (the code that
produces pidStr, comm and rest) to attempt reading cmdline first, handle
NUL-separated args, and only use comm when cmdline yields no usable command.
Ensure ProcInfo.Command uses the cmdline output so long process names aren’t
silently truncated.
In `@pkg/wshrpc/wshremote/processviewer.go`:
- Around line 427-430: The current logic resets limit to 50 whenever data.Limit
is <=0 or >500; instead keep the default for non-positive inputs but clamp
oversized inputs to 500: validate data.Limit into the local variable limit so
that if data.Limit <= 0 you set limit to 50, otherwise if data.Limit > 500 set
limit to 500, else set limit = data.Limit; update the code around the variable
limit (the block using data.Limit and limit) to implement this three-way check.
---
Nitpick comments:
In `@pkg/util/unixutil/unixutil_unix.go`:
- Around line 83-94: The SendSignalByName function currently treats
os.FindProcess errors as "process not found", but on Unix FindProcess rarely
fails (only for pid <= 0) and the real errors come from p.Signal; update
SendSignalByName to validate pid > 0 up front (returning a clear error for
invalid pid), call ParseSignal as before, call os.FindProcess without assuming
non-existence, and rely on p.Signal(sig) to surface real runtime errors (or wrap
the p.Signal error with context). Reference: SendSignalByName, ParseSignal,
os.FindProcess, p.Signal.
In `@pkg/wshrpc/wshremote/processviewer.go`:
- Around line 184-201: The current loop spawns one goroutine per process which
can create thousands of concurrent procinfo.GetProcInfo calls; replace it with a
bounded worker pool (size based on runtime.GOMAXPROCS(0) or runtime.NumCPU) that
consumes jobs from a channel of tasks containing (index i, p.Pid) and writes
results back into rawInfos[index] as pidInfo; ensure workers use the same panic
handling (panichandler.PanicHandler("collectSnapshot:GetProcInfo", ...)), use a
WaitGroup to wait for all jobs to finish, propagate nil on error from
procinfo.GetProcInfo, and close the jobs channel to signal workers to exit so
sampling cost stays predictable.
🪄 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: d44c3819-f3b9-4081-97ae-016c8322295c
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (19)
.kilocode/skills/create-view/SKILL.mdfrontend/app/block/block.tsxfrontend/app/block/blockregistry.tsfrontend/app/block/blockutil.tsxfrontend/app/store/wshclientapi.tsfrontend/app/view/processviewer/processviewer.tsxfrontend/preview/mock/mockwaveenv.tsfrontend/preview/previews/processviewer.preview.tsxfrontend/types/gotypes.d.tsgo.modpkg/util/procinfo/procinfo.gopkg/util/procinfo/procinfo_darwin.gopkg/util/procinfo/procinfo_linux.gopkg/util/procinfo/procinfo_windows.gopkg/util/unixutil/unixutil_unix.gopkg/util/unixutil/unixutil_windows.gopkg/wshrpc/wshclient/wshclient.gopkg/wshrpc/wshremote/processviewer.gopkg/wshrpc/wshrpctypes.go
| triggerRefresh() { | ||
| if (this.cancelPoll) { | ||
| this.cancelPoll(); | ||
| } | ||
| this.cancelPoll = null; | ||
| if (!globalStore.get(this.pausedAtom)) { | ||
| this.startPolling(); | ||
| } |
There was a problem hiding this comment.
Paused mode currently breaks scroll/search/resize refreshes.
triggerRefresh() becomes a no-op while paused, but scrollTop, containerHeight, and textSearch still drive server-side pagination. After pausing, any viewport/search change keeps rendering the old slice, which can leave stale rows or blank gaps. Let paused stop the timer, not user-driven one-shot fetches.
💡 Proposed fix
triggerRefresh() {
if (this.cancelPoll) {
this.cancelPoll();
}
this.cancelPoll = null;
- if (!globalStore.get(this.pausedAtom)) {
- this.startPolling();
- }
+ if (globalStore.get(this.pausedAtom)) {
+ void this.doOneFetch();
+ return;
+ }
+ this.startPolling();
}Also applies to: 224-226, 268-280
🤖 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 202 - 209,
triggerRefresh currently returns early when paused (this.pausedAtom) which
prevents user-driven one-shot updates (scrollTop/containerHeight/textSearch)
from fetching the correct page; change the logic in triggerRefresh (and the
similar blocks around the other occurrences noted) so that pausing only disables
the recurring timer (cancelPoll/startPolling) but does not prevent immediate
fetches for user actions—i.e., if the call was triggered by a user-driven event
(scroll/search/resize) or if parameters affecting server pagination changed,
still call the routine that performs a single fetch/update (the same code
startPolling would use for one-shot fetches or the page-fetch method used in the
polling loop) while keeping cancelPoll cleared and the periodic poll disabled
when paused. Ensure references: triggerRefresh, startPolling, cancelPoll,
pausedAtom, and the fetch/update routine are updated consistently in the other
blocks around the noted occurrences.
| setTextSearch(text: string) { | ||
| globalStore.set(this.textSearchAtom, text); | ||
| this.triggerRefresh(); |
There was a problem hiding this comment.
Virtualization is sized against totalcount instead of the filtered result set.
The RPC applies textsearch before pagination, but totalHeight still uses totalcount. After narrowing the list, the scroller stays as tall as the unfiltered table, and an existing deep scrollTop can request an empty page. Drive the virtual height from filteredcount and reset the scroll position when the search text changes.
💡 Proposed fix
- const totalCount = data?.totalcount ?? 0;
+ const totalCount = data?.totalcount ?? 0;
+ const virtualCount = data?.filteredcount ?? totalCount;
const processes = data?.processes ?? [];
const hasCpu = data?.hascpu ?? false;
@@
- const totalHeight = totalCount * RowHeight;
+ const totalHeight = virtualCount * RowHeight;Also applies to: 769-797
🤖 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 224 - 226,
The virtualization height is being computed from the unfiltered totalcount
causing the scroller to remain sized to the full dataset and allowing a deep
scrollTop to request empty pages; in setTextSearch (which writes textSearchAtom
and calls triggerRefresh) update the code that computes
totalHeight/virtualization size to use filteredcount instead of totalcount and
also reset the scroller position (clear scrollTop / set to 0) when the search
text changes so the view jumps to the top of the newly filtered results; locate
uses of totalHeight, totalcount and any scrollTop manipulation in the
virtualized list rendering and update them accordingly (also apply same change
around the other affected block referenced at lines ~769-797).
| keyDownHandler(waveEvent: WaveKeyboardEvent): boolean { | ||
| if (keyutil.checkKeyPressed(waveEvent, "Cmd:f")) { | ||
| this.openSearch(); |
There was a problem hiding this comment.
Search shortcut hint is out of sync with the handler.
The non-macOS tooltip says Alt-F, but the key handler only checks Cmd:f. Either the hint is wrong or the documented shortcut never fires on Linux/Windows.
Also applies to: 558-559
🤖 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 - 241,
The keyDownHandler currently only checks for "Cmd:f" so the UI hint for
non-macOS (Alt-F) never triggers on Windows/Linux; update the handler(s)
(keyDownHandler and the similar handler around the second occurrence) to check
the appropriate modifier(s) for non-macOS — e.g., detect platform or test both
"Cmd:f" and "Alt:f" (or "Ctrl:f" if your keyutil expects Ctrl on Windows) and
call this.openSearch() when either variant matches, ensuring both shortcut hints
and behavior stay in sync.
| function makeMockProcessListResponse(data: CommandRemoteProcessListData): ProcessListResponse { | ||
| let procs = [...MockProcesses]; | ||
|
|
||
| const sortBy = (data.sortby as "pid" | "command" | "user" | "cpu" | "mem") ?? "cpu"; | ||
| const sortDesc = data.sortdesc ?? false; | ||
|
|
||
| procs.sort((a, b) => { | ||
| let cmp = 0; | ||
| if (sortBy === "pid") cmp = a.pid - b.pid; | ||
| else if (sortBy === "command") cmp = (a.command ?? "").localeCompare(b.command ?? ""); | ||
| else if (sortBy === "user") cmp = (a.user ?? "").localeCompare(b.user ?? ""); | ||
| else if (sortBy === "cpu") cmp = (a.cpu ?? 0) - (b.cpu ?? 0); | ||
| else if (sortBy === "mem") cmp = (a.mem ?? 0) - (b.mem ?? 0); | ||
| return sortDesc ? -cmp : cmp; | ||
| }); | ||
|
|
||
| const start = data.start ?? 0; | ||
| const limit = data.limit ?? procs.length; | ||
| const sliced = procs.slice(start, start + limit); | ||
|
|
||
| return { | ||
| processes: sliced, | ||
| summary: MockSummary, | ||
| ts: Date.now(), | ||
| hascpu: true, | ||
| totalcount: procs.length, | ||
| filteredcount: procs.length, | ||
| }; |
There was a problem hiding this comment.
Apply the preview's search term before paging.
makeMockProcessListResponse() never uses data.textsearch, so the standalone preview can't exercise the widget's filtering path and filteredcount becomes misleading as soon as a query is entered. Filter the mock rows before sorting/slicing and keep totalcount as the unfiltered size.
🤖 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 40 - 67,
makeMockProcessListResponse currently ignores data.textsearch so previews never
exercise filtering and filteredcount is wrong; update
makeMockProcessListResponse to apply the text search (data.textsearch) to
MockProcesses first (produce a filtered list), keep totalcount as the original
MockProcesses.length, then sort and slice the filtered list (use the same sort
logic and variables like sortBy, sortDesc, start, limit) and set filteredcount
to the filtered list length before paging; reference
makeMockProcessListResponse, MockProcesses, data.textsearch, filteredcount and
totalcount when making the change.
| useRpcOverride("RemoteProcessListCommand", async (_client, data) => { | ||
| return makeMockProcessListResponse(data); | ||
| }); |
There was a problem hiding this comment.
Mock process actions explicitly in preview.
The preview only overrides RemoteProcessListCommand. Any kill/stop/resume action will fall through to the mock environment's default Promise.resolve(null) path, so the UI can look successful without changing any mock state. Add a RemoteProcessSignalCommand override, or at least reject until the preview supports it.
Minimal fix
useRpcOverride("RemoteProcessListCommand", async (_client, data) => {
return makeMockProcessListResponse(data);
});
+ useRpcOverride("RemoteProcessSignalCommand", async () => {
+ throw new Error("RemoteProcessSignalCommand is not implemented in the preview");
+ });📝 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.
| useRpcOverride("RemoteProcessListCommand", async (_client, data) => { | |
| return makeMockProcessListResponse(data); | |
| }); | |
| useRpcOverride("RemoteProcessListCommand", async (_client, data) => { | |
| return makeMockProcessListResponse(data); | |
| }); | |
| useRpcOverride("RemoteProcessSignalCommand", async () => { | |
| throw new Error("RemoteProcessSignalCommand is not implemented in the preview"); | |
| }); |
🤖 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 82 - 84,
The preview currently overrides only RemoteProcessListCommand via useRpcOverride
and thus ignores kill/stop/resume actions; add a corresponding useRpcOverride
for RemoteProcessSignalCommand (or update the existing override set) to either
simulate the signal effects on the mock process state (e.g., update the mock
returned by makeMockProcessListResponse) or explicitly reject the RPC so the UI
doesn't report a spurious success; reference the existing useRpcOverride call
and makeMockProcessListResponse to locate where to add the
RemoteProcessSignalCommand handler and ensure it returns a realistic mocked
response or a rejected Promise.
| info := &ProcInfo{ | ||
| Pid: int32(k.Proc.P_pid), | ||
| Ppid: k.Eproc.Ppid, | ||
| Command: unix.ByteSliceToString(k.Proc.P_comm[:]), | ||
| Uid: k.Eproc.Ucred.Uid, | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, locate and examine the file in question
find . -name "procinfo_darwin.go" -type fRepository: wavetermdev/waveterm
Length of output: 102
🏁 Script executed:
# Get context around lines 84-89 of procinfo_darwin.go
cat -n pkg/util/procinfo/procinfo_darwin.go | head -100Repository: wavetermdev/waveterm
Length of output: 3315
🏁 Script executed:
# Check how ProcInfo struct is used throughout the codebase
rg "type ProcInfo struct" -A 10Repository: wavetermdev/waveterm
Length of output: 683
🏁 Script executed:
# Look for any usage of Command field in ProcInfo to understand the impact
rg "\.Command" pkg/util/procinfo/ -B 2 -A 2Repository: wavetermdev/waveterm
Length of output: 46
🏁 Script executed:
# Search for how the Command field is used (better pattern)
rg "Command" pkg/util/procinfo/ -B 1 -A 1Repository: wavetermdev/waveterm
Length of output: 1391
🏁 Script executed:
# Search the entire codebase for how ProcInfo is displayed or used
rg "ProcInfo" --type go | head -30Repository: wavetermdev/waveterm
Length of output: 1590
🏁 Script executed:
# Check the unix package documentation for P_comm size
# Look for kinfo_proc type definition
rg "P_comm|Proc struct" /usr/local/go/src/ 2>/dev/null | head -20 || echo "Checking local codebase for unix imports"Repository: wavetermdev/waveterm
Length of output: 953
🏁 Script executed:
# Look for any existing process name handling or Apple-specific APIs
rg "proc_name|proc_pidpath|libproc" -iRepository: wavetermdev/waveterm
Length of output: 132
🏁 Script executed:
# Check if there are any test files that might show expected behavior
find . -name "*procinfo*test*.go" -o -name "*test*procinfo*.go"Repository: wavetermdev/waveterm
Length of output: 46
🏁 Script executed:
# Examine how Command is displayed in processviewer.go
cat -n pkg/wshrpc/wshremote/processviewer.go | grep -A 10 -B 10 "Command\|pi\.Command"Repository: wavetermdev/waveterm
Length of output: 4247
🏁 Script executed:
# Check if there are any existing bindings to Apple APIs like proc_name
rg "proc_name|proc_pidpath|libproc" --type goRepository: wavetermdev/waveterm
Length of output: 132
🏁 Script executed:
# Check if unix package has proc_name or similar functions
rg "func.*proc" /usr/local/go/src/cmd/vendor/golang.org/x/sys/unix/zsyscall_darwin*.go 2>/dev/null | head -20Repository: wavetermdev/waveterm
Length of output: 46
🏁 Script executed:
# Look at what Apple's proc_info.h exposes in the go environment
grep -r "proc_" /usr/local/go/src/cmd/vendor/golang.org/x/sys/unix/ | grep -i name | head -10Repository: wavetermdev/waveterm
Length of output: 1313
🏁 Script executed:
# Check real-world process name lengths on macOS to understand impact
# Look at how long typical process names are (e.g., in launch daemons, user apps)
cat -n pkg/wshrpc/wshremote/processviewer.go | head -150Repository: wavetermdev/waveterm
Length of output: 4761
🏁 Script executed:
# Check for any TODOs or FIXMEs related to process names or truncation
rg "TODO|FIXME|XXX" pkg/util/procinfo/ -iRepository: wavetermdev/waveterm
Length of output: 46
🏁 Script executed:
# Check how Linux implementation handles process names to understand if truncation is platform-specific
cat -n pkg/util/procinfo/procinfo_linux.go | head -80Repository: wavetermdev/waveterm
Length of output: 2751
🏁 Script executed:
# Look for any comments about MAXCOMLEN or p_comm limitations
rg "MAXCOMLEN|p_comm|truncat" -i pkg/util/procinfo/Repository: wavetermdev/waveterm
Length of output: 153
Process names longer than 16 characters will be truncated.
On Darwin, k.Proc.P_comm is limited to 16 characters (stored in a 17-byte array including null terminator). This affects the process list display, text search filtering, and sorting in the process viewer. Consider using proc_name, proc_pidpath, or similar libproc APIs to provide fuller command names when available. The codebase already uses purego for dynamic library binding, making this approach feasible.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/util/procinfo/procinfo_darwin.go` around lines 84 - 89, The ProcInfo
construction uses k.Proc.P_comm which is truncated at 16 chars; update the logic
that sets ProcInfo.Command (in pkg/util/procinfo/procinfo_darwin.go) to attempt
retrieving a full command name via libproc (e.g. proc_name or proc_pidpath)
using the process PID (k.Proc.P_pid) through the existing purego binding
mechanism, and only fall back to unix.ByteSliceToString(k.Proc.P_comm[:]) if
those libproc calls fail or return empty; ensure the new code populates
ProcInfo.Command with the full path/name when available and preserves the
existing behavior on error.
| // userHz is USER_HZ, the kernel's timer frequency used in /proc/[pid]/stat CPU fields. | ||
| // On Linux this is always 100. | ||
| const userHz = 100.0 |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's examine the file and the specific lines in question
cat -n pkg/util/procinfo/procinfo_linux.go | head -130Repository: wavetermdev/waveterm
Length of output: 4565
🏁 Script executed:
# Search for all uses of userHz in the file
rg "userHz" pkg/util/procinfo/procinfo_linux.go -n -B 2 -A 2Repository: wavetermdev/waveterm
Length of output: 517
🏁 Script executed:
# Check if there are any other related files or tests
fd -e go procinfo | head -20Repository: wavetermdev/waveterm
Length of output: 204
🌐 Web query:
Linux sysconf _SC_CLK_TCK vary by system
💡 Result:
Yes, sysconf(_SC_CLK_TCK) can vary by Linux system. It returns the number of clock ticks per second (USER_HZ), which is fixed at 100 on x86 and most common architectures for ABI compatibility, even if the kernel's internal CONFIG_HZ (timer interrupt frequency) is higher like 250 or 1000. This normalization ensures consistent user-space behavior across kernels with different HZ configurations. On some architectures or custom/embedded kernels, it may differ (e.g., other values possible per kernel config), but standard desktop/server distros (Ubuntu, Fedora, RHEL) report 100. Use getconf CLK_TCK or sysconf(_SC_CLK_TCK) to query runtime value; do not hardcode.
Citations:
- 1: https://linux.die.net/man/3/sysconf
- 2: https://man7.org/linux/man-pages/man3/sysconf.3.html
- 3: https://stackoverflow.com/questions/19919881/sysconf-sc-clk-tck-what-does-it-return
- 4: Ensure correct calculation of process CPU usage percentage on Linux elastic/apm#102
- 5: https://github.com/torvalds/linux/blob/master/kernel/Kconfig.hz
- 6: https://kernelnewbies.kernelnewbies.narkive.com/7qlZrujb/getconf-clk-tck-and-config-hz
Read CLK_TCK dynamically instead of hard-coding 100.
/proc/[pid]/stat reports utime and stime in clock ticks that must be divided by sysconf(_SC_CLK_TCK). Hard-coding 100 breaks on systems where the clock tick frequency differs, causing CPU metrics to be scaled incorrectly. While 100 is standard on most architectures, portable code must query the runtime value. (man7.org)
Affects lines 110-111.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/util/procinfo/procinfo_linux.go` around lines 15 - 17, Replace the
hard-coded const userHz = 100.0 with a runtime lookup of CLK_TCK and use that
value where CPU ticks are converted (references: userHz and the code that
divides utime/stime at lines ~110-111); implement a helper like getUserHz() that
calls unix.Sysconf(unix._SC_CLK_TCK) (from golang.org/x/sys/unix), converts the
returned tick count to float64, caches it for subsequent calls, and falls back
to 100.0 only if Sysconf returns an error or non-positive value; update usages
to call this helper (or read the cached value) instead of the constant.
| pidStr := strings.TrimSpace(s[:lp]) | ||
| comm := s[lp+1 : rp] | ||
| rest := strings.Fields(s[rp+1:]) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's find and examine the file in question
find . -name "procinfo_linux.go" -type fRepository: wavetermdev/waveterm
Length of output: 101
🏁 Script executed:
# Get the file size and read the relevant sections
wc -l pkg/util/procinfo/procinfo_linux.goRepository: wavetermdev/waveterm
Length of output: 103
🏁 Script executed:
# Read the file to see lines 71-73 and 105-108
cat -n pkg/util/procinfo/procinfo_linux.go | head -120 | tail -60Repository: wavetermdev/waveterm
Length of output: 2246
🏁 Script executed:
# Check for other functions that might read process names (cmdline, exe)
cat -n pkg/util/procinfo/procinfo_linux.go | head -60Repository: wavetermdev/waveterm
Length of output: 1991
🏁 Script executed:
# Search for how ProcInfo is defined and used
rg "type ProcInfo" -A 10Repository: wavetermdev/waveterm
Length of output: 683
🏁 Script executed:
# Check if cmdline or exe are used anywhere in the procinfo package
rg "cmdline|/proc.*cmdline" pkg/util/procinfo/Repository: wavetermdev/waveterm
Length of output: 46
🏁 Script executed:
# Check if there's any mention of TASK_COMM_LEN or 15-char limit
rg "TASK_COMM|comm.*len|15" pkg/util/procinfo/Repository: wavetermdev/waveterm
Length of output: 132
🏁 Script executed:
# Search for any references to cmdline in the entire codebase to see if it's already used elsewhere
rg "cmdline" --type goRepository: wavetermdev/waveterm
Length of output: 46
🏁 Script executed:
# Check if there's a Windows procinfo file to compare structure
fd procinfo_.*\.go pkg/util/procinfo/Repository: wavetermdev/waveterm
Length of output: 174
🏁 Script executed:
# Look at the full procinfo.go to understand the data structure
cat pkg/util/procinfo/procinfo.goRepository: wavetermdev/waveterm
Length of output: 1156
🏁 Script executed:
# Let's check if /proc/[pid]/cmdline would be a viable alternative
# and look for any existing pattern of reading multiple /proc files
rg "/proc.*status|readUid" -A 5 pkg/util/procinfo/procinfo_linux.goRepository: wavetermdev/waveterm
Length of output: 748
🏁 Script executed:
# Check the Darwin and Windows implementations to see how they handle Command
cat pkg/util/procinfo/procinfo_darwin.go | head -60Repository: wavetermdev/waveterm
Length of output: 1220
🏁 Script executed:
# Let's verify TASK_COMM_LEN by checking Linux kernel documentation or seeing if there's any note about it
web_search "TASK_COMM_LEN Linux kernel value"Repository: wavetermdev/waveterm
Length of output: 114
🏁 Script executed:
# Let's also check if there are any comments in the code about why cmdline wasn't used
rg "comm.*field|stat.*field|Command" -B 2 -A 2 pkg/util/procinfo/procinfo_linux.go | head -40Repository: wavetermdev/waveterm
Length of output: 1164
🏁 Script executed:
# Check if there's any issue or TODO about process name truncation
rg "TODO|FIXME|truncat|limit|15|TASK_COMM" pkg/util/procinfo/Repository: wavetermdev/waveterm
Length of output: 132
🌐 Web query:
TASK_COMM_LEN Linux kernel /proc/pid/stat comm field truncation
💡 Result:
In the Linux kernel, the comm field in /proc/pid/stat displays the process's command name (basename of the executable), enclosed in parentheses. This field is limited to TASK_COMM_LEN characters, defined as 16 (including the terminating null byte), so effectively 15 visible characters. Strings longer than this are silently truncated without warning. This truncation occurs because task_struct::comm is a fixed-size char array of length TASK_COMM_LEN, and functions like get_task_comm copy at most TASK_COMM_LEN-1 characters plus null terminator into it. In fs/proc/array.c, proc_task_name outputs up to 64 characters for kernel threads (via get_kthread_comm), but for user processes, it uses get_task_comm, respecting the 16-byte limit. Official documentation confirms this: man proc_pid_stat(5) states "Strings longer than TASK_COMM_LEN (16) characters (including the terminating null byte) are silently truncated." The same applies to /proc/pid/comm writes and prctl(PR_SET_NAME). This limit has remained stable across recent kernels (e.g., v6.9+), as shown in kernel source cross-references. Parsing /proc/pid/stat requires finding the matching closing parenthesis after the comm field, not assuming fixed length, due to potential special characters in comm.
Citations:
- 1: https://man7.org/linux/man-pages/man5/proc_pid_comm.5.html
- 2: https://man7.org/linux/man-pages/man5/proc_pid_stat.5.html
- 3: https://elixir.free-electrons.com/linux/latest/ident/TASK_COMM_LEN
- 4: https://github.com/torvalds/linux/blob/master/fs/proc/array.c
Process names longer than 15 characters are truncated from comm field.
The comm field in /proc/[pid]/stat is limited to 15 visible characters (TASK_COMM_LEN = 16 including null terminator). Strings longer than this are silently truncated by the kernel. Using comm directly for ProcInfo.Command will break command-based filtering and sorting for processes with longer names (e.g., google-chrome, firefox, node). Consider using /proc/[pid]/cmdline as the primary source with comm as a fallback.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/util/procinfo/procinfo_linux.go` around lines 71 - 73, The current
parsing sets ProcInfo.Command from the truncated comm value parsed as comm :=
s[lp+1 : rp]; instead, read /proc/[pid]/cmdline for the full command (split NUL
bytes and join with spaces) and set ProcInfo.Command from that; if
/proc/[pid]/cmdline is empty or unavailable, fall back to the existing comm
value (trim parentheses) as a secondary source. Update the parsing logic in
procinfo_linux.go (the code that produces pidStr, comm and rest) to attempt
reading cmdline first, handle NUL-separated args, and only use comm when cmdline
yields no usable command. Ensure ProcInfo.Command uses the cmdline output so
long process names aren’t silently truncated.
| limit := data.Limit | ||
| if limit <= 0 || limit > 500 { | ||
| limit = 50 | ||
| } |
There was a problem hiding this comment.
Clamp oversized limits to the max, not back to the default page size.
When data.Limit is 501+, the code falls back to 50 instead of capping at 500. That makes large viewports under-fetch and is surprising for API callers.
💡 Proposed fix
limit := data.Limit
- if limit <= 0 || limit > 500 {
- limit = 50
- }
+ if limit <= 0 {
+ limit = 50
+ } else if limit > 500 {
+ limit = 500
+ }📝 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.
| limit := data.Limit | |
| if limit <= 0 || limit > 500 { | |
| limit = 50 | |
| } | |
| limit := data.Limit | |
| if limit <= 0 { | |
| limit = 50 | |
| } else if limit > 500 { | |
| limit = 500 | |
| } |
🤖 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 427 - 430, The current
logic resets limit to 50 whenever data.Limit is <=0 or >500; instead keep the
default for non-positive inputs but clamp oversized inputs to 500: validate
data.Limit into the local variable limit so that if data.Limit <= 0 you set
limit to 50, otherwise if data.Limit > 500 set limit to 500, else set limit =
data.Limit; update the code around the variable limit (the block using
data.Limit and limit) to implement this three-way check.
1 participant
No description provided.