feat(web): select diagnostics environment#4486
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
github-actions
Bot
added
vouch:unvouched
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
ApprovabilityVerdict: Needs human review This PR introduces a new feature (environment selection for diagnostics) with significant UI and state management changes. An unresolved medium-severity bug regarding refresh button behavior while disconnected has been identified, warranting human review. You can customize Macroscope's approvability policy. Learn more. |
| <DiagnosticsLastChecked checkedAt={processData?.readAt ?? null} /> | ||
| <DiagnosticsRefreshButton | ||
| isPending={isProcessPending} | ||
| isPending={isProcessPending && isConnected} |
There was a problem hiding this comment.
Refresh enabled while disconnected
Medium Severity
Passing isPending && isConnected into DiagnosticsRefreshButton stops the spinner when the backend is down, but also clears disabled because that prop alone gates both states. Disconnected diagnostics queries stay on Effect.never, so refresh stays clickable with no progress. Open logs in the same change correctly uses an explicit !isConnected disable.
Additional Locations (2)
Reviewed by Cursor Bugbot for commit 60f3a62. Configure here.
There was a problem hiding this comment.
Correct, and fixed — the spinner and the disabled state were the same prop, so suppressing the spinner while disconnected also re-enabled the button.
DiagnosticsRefreshButton now takes a separate isDisabled alongside isPending, and renders disabled={isPending || isDisabled}. All three refresh buttons pass isDisabled={!isConnected}, so while an environment is disconnected they are static and disabled rather than static but clickable. "Open logs folder" is disabled on the same condition, so it can't hang on Effect.never either.
| current !== null && current.environmentId === environmentId | ||
| ? { environmentId, isOpening: false, error } | ||
| : current, | ||
| ); |
There was a problem hiding this comment.
Logs open completion race
Low Severity
The open-logs completion updater only checks that current.environmentId matches the request’s environment. A single shared logsDirectoryState slot can be overwritten by another environment, so returning to the original environment and starting a new open while the first request is still in flight lets the older completion clear the newer pending isOpening state.
Reviewed by Cursor Bugbot for commit 60f3a62. Configure here.
There was a problem hiding this comment.
Agreed, and fixed. Matching the completion by environment id was the wrong key — a second request for the same environment could still clobber an earlier one.
The logs-directory state is now a single LogsDirectoryState { environmentId, isOpening, error } record, and it only renders when state.environmentId === environmentId, so nothing leaks across an environment switch. The async completion matches its own request by object identity rather than by environment id, so neither a later request in another environment nor a later request in the same environment can retire the wrong one.
The diagnostics environment picker made three pieces of panel-local state lie about which machine they belong to: - `openLogsDirectoryError` / `isOpeningLogsDirectory` kept showing an environment A failure (or spinner) after switching to environment B. They are now stored together with the environment they belong to, so they only render for that environment and a late completion can only update its own request. - `signalingPid` was a single shared value, so completing a signal on environment A re-enabled the signal controls for an in-flight signal on environment B and allowed a duplicate SIGKILL. Pending signals are now identified by environment id plus pid. - Diagnostics queries only run while the selected environment's supervisor is connected, so a disconnected machine showed permanent "Loading..." placeholders with the refresh buttons disabled. The panel now derives a connection notice and renders it instead of the loading state. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Follow-up to the review feedback on the previous commit: - `DiagnosticsRefreshButton` used a single `isPending` prop for both the spinner and `disabled`, so suppressing the spinner while the selected environment is disconnected also made the button clickable even though the query cannot run. It now takes an explicit `isDisabled` prop. - The open-logs completion matched on environment id alone, so returning to an environment and starting a second open while the first was still in flight let the older completion clear the newer pending state. The completion now matches the request by identity. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
eb272e4 to
0db411e
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 2 potential issues.
There are 4 total unresolved issues (including 2 from previous reviews).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 0db411e. Configure here.
| (isProcessInitialLoading | ||
| ? "Loading live processes..." | ||
| : "No live descendant processes found." | ||
| : "No live descendant processes found.") |
There was a problem hiding this comment.
Stale diagnostics while disconnected
High Severity
The useEnvironmentQuery hook retains cached data after an environment disconnects. This causes the diagnostics UI to display stale metrics and process lists, keeps actions like process signaling enabled, and prevents the intended disconnected state indicators (like — or connection notices) from appearing.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 0db411e. Configure here.
| }: { | ||
| processes: ReadonlyArray<ServerProcessDiagnosticsEntry>; | ||
| signalingPid: number | null; | ||
| signalingPids: ReadonlySet<number>; |
There was a problem hiding this comment.
Collapse state leaks across environments
Low Severity
ProcessDiagnosticsTable keeps collapsedPids in component state and is rendered without a key tied to the selected environment. Switching the diagnostics source therefore reuses prior collapse choices, so a PID collapsed on one machine can hide the same PID on another.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 0db411e. Configure here.
1 participant
What Changed
Adds an explicit diagnostics-source picker and routes every diagnostics request through the selected backend.
Why
Client-only mode has no primary local environment, so the diagnostics page previously had no backend to query. Diagnostics are backend-specific, not client-specific.
Behavior
UI evidence
All shots come from the same build, light theme, and one 1800px-wide window. Two environments are configured:
dev-workstation(this machine, connected) anddemo-box(a saved remote backend that was deliberately shut down)."Before" is
acccd0e9f(this branch's pre-review-fix head); "after" iseb272e4f6. The branch has since been rebased onto upstream main as0db411ea1815; that rebase changed no file content, so these captures still reflect the current head exactly.1. Selecting an environment that is not connected
Diagnostics RPCs only resolve while the environment's supervisor is connected, so before this fix the page waited forever.
Before —
acccd0e9f: every stat reads..., every table sits on "Loading live processes…" / "Collecting process resource samples…" / "Loading failures…" / "Loading span names…" indefinitely, and all three refresh icons spin forever. "Open logs folder" is still enabled even though it cannot work.After —
eb272e4f6(content-identical to current head0db411ea1815): every stat reads—, and every table explains the actual state ("Reconnecting to demo-box… Reason: …") instead of pretending to load. The refresh icons are static and disabled, and "Open logs folder" is disabled too.The notice wording follows the connection phase, so a backend that is
offline,available,connecting, orerrorgets its own message rather than the reconnecting one shown here.2. The new "Diagnostics source" picker
This is what the PR adds; there is no equivalent on
main, where the page always queried the primary backend.Diagnostics collected from the selected backend, with its connection status and URL under the heading:
Picker open — machine choice is explicit, and local vs remote backends are distinguished by icon:
Verification
vp test run apps/web/src/components/settings/DiagnosticsSettings.logic.test.tsvp check(passes; existing repo warnings only)/settings/diagnostics, confirmed diagnostics are collected from whichever machine is selected, then killed the remote backend and re-checked the page on both the pre-fix and current commits to produce the before/after images above.vp run typecheckis blocked by an incomplete local dependency tree (apps/marketingcannot findastro); focused web typecheck also reports 10 pre-existing errors outside this change.Note
Low Risk
Settings UI and client-side state only; no auth or server contract changes. Process kill still goes through existing
signalProcessRPCs for the selected environment.Overview
Adds a Diagnostics source picker so process, resource, and trace diagnostics target a chosen backend instead of always using the primary server. Resolution order is explicit selection (while still available), then primary, active, then first listed environment; with no environments, the page links to Manage connections.
When the selected machine is not connected, the page shows phase-specific notices (via
diagnosticsConnectionNotice) instead of endless loading spinners, uses—for stats, and disables refresh and open-logs actions. Observability and queries are read from the selected environment’s config.Panel state is scoped per environment: logs-folder open/errors and in-flight process signals use environment+pid keys so switching machines does not leak UI state. Logic lives in
DiagnosticsSettings.logic.tswith unit tests.Reviewed by Cursor Bugbot for commit 0db411e. Bugbot is set up for automated code reviews on this repo. Configure here.
Note
Add environment selection and connection-aware controls to diagnostics settings
DiagnosticsSettingsPanelso users can target a specific environment for diagnostics, resolved viaresolveDiagnosticsEnvironmentId(selected > primary > active > first available).—placeholders instead of spinners, and adiagnosticsConnectionNoticemessage explains non-connected states.signalingPidtracking with a per-environment, per-pidPendingProcessSignalarray, allowing multiple in-flight process signals to be tracked concurrently.ProcessDiagnosticsTablenow receivessignalingPids: ReadonlySet<number>instead ofsignalingPid: number | null, which is a breaking prop change for any direct consumers.Macroscope summarized 0db411e.