Improve menubar: process-based detection, auto-refresh#3
Merged
Conversation
📝 WalkthroughWalkthroughServer availability detection moved from HTTP localhost probes to per-port PID files with liveness checks via Changes
Sequence Diagram(s)sequenceDiagram
participant Timer as Timer (`@rumps.timer` 5s)
participant Menu as Menubar (_update_status)
participant PID as PID File (dashboard-{port}.pid)
participant OS as OS / Process
Timer->>Menu: trigger periodic status update
Menu->>PID: _read_pid()
alt PID exists
PID-->>Menu: pid
Menu->>OS: os.kill(pid, 0) (liveness check)
alt Process alive
OS-->>Menu: alive
Menu->>Menu: set "Status: Running"
else Not alive
OS-->>Menu: ESRCH / no response
Menu->>PID: _clear_pid()
Menu->>Menu: set "Status: Stopped"
end
else PID missing
PID-->>Menu: missing
Menu->>Menu: set "Status: Stopped"
end
sequenceDiagram
participant User as User (click Start/Stop)
participant Menubar as Menubar (start_server / stop_server)
participant Proc as OS / Process (server)
participant PID as PID File
User->>Menubar: start_server()
Menubar->>Proc: spawn server subprocess
Proc-->>Menubar: child_pid
Menubar->>PID: write child_pid to PID file
User->>Menubar: stop_server()
Menubar->>PID: _read_pid()
alt pid found
Menubar->>Proc: send SIGTERM
Proc-->>Menubar: terminates (or not)
alt still alive
Menubar->>Proc: send SIGKILL
end
Menubar->>PID: _clear_pid()
else no pid / no process handle
Menubar->>PID: attempt read & signal external pid (same flow)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsTimed out fetching pipeline failures after 30000ms 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: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/claude_dashboard/menubar.py`:
- Around line 90-99: The code prematurely sets stopped=True right after sending
SIGTERM and swallows errors; update the fallback in menubar.py that uses
subprocess.run([... "claude_dashboard.server"]) so that after calling
os.kill(pid, signal.SIGTERM) you wait/poll for the PID to actually exit (e.g.,
loop with short sleep and a timeout) before setting stopped=True, and replace
the blanket except with targeted exception handling that logs/propagates kill
failures (e.g., OSError/ProcessLookupError) instead of silently passing so
failures to stop the process are visible.
- Around line 33-37: The pgrep -f check in the subprocess.run call for
"claude_dashboard.server" is too broad and can match unrelated processes;
replace this substring match with a reliable single-instance mechanism: create
and check a pid file (write the child PID when launching the dashboard) and
change the is-running check to read that PID and verify the process exists and
matches the expected executable before returning True. Update the stop/fallback
logic to read the pid file and send the proper signal (and remove the pid file)
rather than using the current pgrep-based stop; key symbols to modify are the
subprocess.run(...) invocation that uses "pgrep -f claude_dashboard.server", the
launch code that should write the pid file, and the stop/fallback handler that
currently relies on pgrep.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: e476e8b1-5a4e-4633-aee8-c2771d3489c6
📒 Files selected for processing (1)
src/claude_dashboard/menubar.py
… status - Detect running server via pgrep instead of HTTP request (avoids auth issues) - Stop server by process name instead of port (won't kill unrelated processes) - Add 5-second timer to auto-refresh menu status - Fix status menu item lookup to handle both states Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Replace broad pgrep matching with PID file (~/.claude/dashboard.pid) - Server writes PID on startup, cleans up on exit - Menubar detects server via PID file with os.kill(pid, 0) liveness check - Stop verifies process actually exits before reporting success - Falls back to SIGKILL after 5s timeout Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
jameswnl
force-pushed
the
improve-menubar
branch
from
1746019 to
68a24df
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/claude_dashboard/menubar.py (1)
27-35:⚠️ Potential issue | 🟠 MajorValidate that the PID still belongs to Claude Dashboard.
_read_pid()currently trusts any live PID in the file. Ifsrc/claude_dashboard/server.pyis terminated without running its cleanup and that PID gets reused,_is_running()can flip toRunningand the fallback stop path can signal an unrelated process. Re-check process identity before returning the PID, not just liveness.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/claude_dashboard/server.py`:
- Around line 277-284: The PID file currently written in server.main() as
pid_file = Path.home() / ".claude" / "dashboard.pid" is global and conflicts
between instances; change the logic in the server startup (the main() function
or wherever pid_file is created/written) to scope the PID file to the bound port
(e.g., include the port number in the filename like dashboard.{port}.pid) or
only write the PID when using the default port, and ensure you also update
removal logic to remove the matching port-scoped file; additionally update
menubar.py to only read the default-port filename (or ignore files not matching
port 8420) so it won’t pick up other-instance PID files.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: f33feefc-f3b6-46d9-b1a5-166a5b676c8d
📒 Files selected for processing (2)
src/claude_dashboard/menubar.pysrc/claude_dashboard/server.py
PID file is now dashboard-{port}.pid so instances on different
ports don't conflict. Menubar only checks the default port file.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/claude_dashboard/menubar.py (1)
128-140:⚠️ Potential issue | 🟠 MajorOnly
ProcessLookupErrorshould mean “stopped.”Both the liveness loop and the outer
except OSErrorturn permission or other signal failures intostopped = True, then_clear_pid()hides the failure. Ifos.kill()cannot actually signal the process, the menu will still flip toStoppedwhile the server keeps running.Minimal fix
for _ in range(50): # up to 5 seconds import time time.sleep(0.1) try: os.kill(pid_to_kill, 0) - except OSError: + except ProcessLookupError: stopped = True break if not stopped: os.kill(pid_to_kill, signal.SIGKILL) stopped = True - except OSError: + except ProcessLookupError: # Process already gone stopped = True - _clear_pid() + if stopped: + _clear_pid()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/claude_dashboard/menubar.py` around lines 128 - 140, The code currently treats any OSError from os.kill as the process being stopped; change both exception handlers around the liveness loop and the outer handler to only treat ProcessLookupError as indicating "stopped" (i.e., set stopped = True when except ProcessLookupError), and for other OSError types do not clear the PID or flip state—either re-raise the exception or log and return an error so the server state isn't hidden; edit the handlers around pid_to_kill and the outer except to reference ProcessLookupError instead of OSError and ensure _clear_pid() is only called when stopped is true due to ProcessLookupError.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/claude_dashboard/menubar.py`:
- Around line 18-36: Remove the menubar-side PID file creation and rely on the
server process to own its PID file: stop calling _write_pid(...) from the
menubar startup flow and instead use the in-memory SERVER_PROCESS reference to
manage stop/status actions; keep _read_pid() and _clear_pid() to handle cases
where the server itself created/cleared the file, but do not pre-write PID files
from the menubar (remove or disable uses of _write_pid in the menubar start
logic such as where the child PID is currently written) so Stop Server will
signal SERVER_PROCESS rather than a potentially stale PID file.
---
Duplicate comments:
In `@src/claude_dashboard/menubar.py`:
- Around line 128-140: The code currently treats any OSError from os.kill as the
process being stopped; change both exception handlers around the liveness loop
and the outer handler to only treat ProcessLookupError as indicating "stopped"
(i.e., set stopped = True when except ProcessLookupError), and for other OSError
types do not clear the PID or flip state—either re-raise the exception or log
and return an error so the server state isn't hidden; edit the handlers around
pid_to_kill and the outer except to reference ProcessLookupError instead of
OSError and ensure _clear_pid() is only called when stopped is true due to
ProcessLookupError.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 259ca974-683d-424b-9c8d-be57d1067a9d
📒 Files selected for processing (2)
src/claude_dashboard/menubar.pysrc/claude_dashboard/server.py
The server already writes dashboard-{port}.pid from server.main().
Writing it from the menubar on subprocess.Popen created a stale-file
risk if the server dies before reaching its own PID write/cleanup.
Now the menubar relies on SERVER_PROCESS for its own child and
_read_pid() for externally started servers.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
♻️ Duplicate comments (2)
src/claude_dashboard/menubar.py (2)
112-130:⚠️ Potential issue | 🟠 MajorOnly clear the PID file after a confirmed external stop.
The fallback
except OSError:also catchesPermissionError, but it still marks the stop as successful, and Line 130 deletes the PID file even if the process is still alive. It also flipsstopped = Trueimmediately afterSIGKILLwithout rechecking liveness. Keep the PID file untilos.kill(pid_to_kill, 0)fails, and reserve the success path forProcessLookupErroror a confirmed exit.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/claude_dashboard/menubar.py` around lines 112 - 130, The PID cleanup currently treats any OSError as a successful stop and clears the PID file immediately; instead, only consider ProcessLookupError (or errno=ESRCH) as confirmation the process is gone, and preserve the PID file on PermissionError or other errors. Update the stop logic around pid_to_kill so that: catch PermissionError separately and do not set stopped=True; when sending SIGTERM/SIGKILL use a recheck loop that calls os.kill(pid_to_kill, 0) after each signal and only set stopped=True (and call _clear_pid()) when that check raises ProcessLookupError/OS error indicating no such process; ensure you reference and update the existing pid handling code paths and the _clear_pid() call so it only runs after a confirmed liveness check failure.
18-24:⚠️ Potential issue | 🔴 CriticalValidate and verify PID-file contents before returning them.
int()accepts0and negative values, andos.kill(0, …)/os.kill(-1, …)do not target a single process._read_pid()also treats any live PID as the dashboard, so a staledashboard-{PORT}.pidcan still resolve to an unrelated reused process. Rejectpid <= 0and confirm the command for that PID is stillclaude_dashboard.serverbefore using it.🛡️ Minimal hardening
def _read_pid(): """Read server PID from file. Returns int or None.""" try: pid = int(PID_FILE.read_text().strip()) - # Check if process is still alive + if pid <= 0: + raise ValueError(f"Invalid PID: {pid}") + command = subprocess.run( + ["ps", "-ww", "-p", str(pid), "-o", "command="], + capture_output=True, + text=True, + check=False, + ).stdout.strip() + if "claude_dashboard.server" not in command: + raise ValueError(f"Unexpected process for PID {pid}") os.kill(pid, 0) return pid except (OSError, ValueError): _clear_pid() return None🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/claude_dashboard/menubar.py` around lines 18 - 24, The _read_pid() helper must validate PID-file contents and ensure the PID actually belongs to our dashboard: when reading PID_FILE in _read_pid(), catch ValueError and return None for non-integer content, reject pid <= 0, and avoid relying solely on os.kill(pid, 0). After os.kill succeeds, verify the process command matches "claude_dashboard.server" (e.g., via /proc/<pid>/cmdline or a ps lookup) and return the pid only if that check passes; otherwise treat it as stale and return None. Ensure any OS errors are handled gracefully and do not treat other live processes as our dashboard.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/claude_dashboard/menubar.py`:
- Around line 112-130: The PID cleanup currently treats any OSError as a
successful stop and clears the PID file immediately; instead, only consider
ProcessLookupError (or errno=ESRCH) as confirmation the process is gone, and
preserve the PID file on PermissionError or other errors. Update the stop logic
around pid_to_kill so that: catch PermissionError separately and do not set
stopped=True; when sending SIGTERM/SIGKILL use a recheck loop that calls
os.kill(pid_to_kill, 0) after each signal and only set stopped=True (and call
_clear_pid()) when that check raises ProcessLookupError/OS error indicating no
such process; ensure you reference and update the existing pid handling code
paths and the _clear_pid() call so it only runs after a confirmed liveness check
failure.
- Around line 18-24: The _read_pid() helper must validate PID-file contents and
ensure the PID actually belongs to our dashboard: when reading PID_FILE in
_read_pid(), catch ValueError and return None for non-integer content, reject
pid <= 0, and avoid relying solely on os.kill(pid, 0). After os.kill succeeds,
verify the process command matches "claude_dashboard.server" (e.g., via
/proc/<pid>/cmdline or a ps lookup) and return the pid only if that check
passes; otherwise treat it as stale and return None. Ensure any OS errors are
handled gracefully and do not treat other live processes as our dashboard.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5a015354-c4f8-4d4e-8b97-a28f2bd696a8
📒 Files selected for processing (1)
src/claude_dashboard/menubar.py
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
pgrepinstead of HTTP request (avoids auth token issues)Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit