Add control-plane setup readiness surfaces#416
Open
vsumner wants to merge 9 commits intospacedriveapp:mainfrom
Open
Add control-plane setup readiness surfaces#416vsumner wants to merge 9 commits intospacedriveapp:mainfrom
vsumner wants to merge 9 commits intospacedriveapp:mainfrom
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds runtime readiness types/endpoints, a readiness classifier/hook, UI surfaces (shell banner, Overview card, Settings → System Health), client mutations for warmup and MCP reconnect, frontend and Rust tests, and doc updates describing the visible setup/runtime health UX. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~65 minutes Possibly related PRs
Suggested reviewers
🚥 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 unit tests (beta)
📝 Coding Plan
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 |
![tembo[bot]](https://avatars.githubusercontent.com/in/1223475?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
src/tools/send_message_to_another_channel.rs (1)
153-159: Add a call-path regression test for the named Signal adapter rewrite.This branch is the only place that remaps an explicit
signal:...target ontocurrent_adapter. The parser tests below will still pass if a future refactor sends these messages through plainsignalinstead ofsignal:gvoice1, so a smallSendMessageTool::calltest would lock down the account-selection behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tools/send_message_to_another_channel.rs` around lines 153 - 159, Add a regression test that exercises the named Signal adapter rewrite in SendMessageTool::call: construct a SendMessageTool with current_adapter Some("signal:gvoice1") and call SendMessageTool::call with a target whose adapter is "signal:someaccount" (or plain "signal") and assert the actual adapter used to send the message equals the current_adapter ("signal:gvoice1"); place the test alongside other SendMessageTool tests and ensure it fails if the code routes to plain "signal" instead of remapping to current_adapter, verifying the branch that checks target.adapter and self.current_adapter.as_ref().filter(|adapter| adapter.starts_with("signal:")) is exercised.interface/src/components/SetupReadiness.test.ts (1)
139-145: Inconsistent indentation.The
expectblock is outdented relative to the surrounding code in this test. This appears to be an unintentional formatting inconsistency.✏️ Suggested fix
- expect(items).toContainEqual( - expect.objectContaining({ - id: "warmup_degraded", - severity: "warning", - tab: "system-health", - }), - ); + expect(items).toContainEqual( + expect.objectContaining({ + id: "warmup_degraded", + severity: "warning", + tab: "system-health", + }), + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@interface/src/components/SetupReadiness.test.ts` around lines 139 - 145, The expect assertion for the "warmup_degraded" item is mis-indented compared to surrounding test code; re-indent the block so it aligns with the other assertions in the same test (match the surrounding indentation level for the expect(...) call and its nested expect.objectContaining({...}) properties), ensuring the lines containing expect(objectContaining({ id: "warmup_degraded", severity: "warning", tab: "system-health" })) follow the same indentation pattern as the other expect assertions in SetupReadiness.test.ts.interface/src/routes/Settings.tsx (2)
1823-1829: Consider using index for React keys in detail list.Using
detail(the string content) as the React key could cause warnings ifformatWarmupDetailsever returns duplicate strings. While currently unlikely, using the index would be more robust.✏️ Suggested fix
{details.length > 0 && ( <div className="mt-2 flex flex-col gap-1 text-sm text-ink-dull"> - {details.map((detail) => ( - <p key={detail}>{detail}</p> + {details.map((detail, idx) => ( + <p key={idx}>{detail}</p> ))} </div> )}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@interface/src/routes/Settings.tsx` around lines 1823 - 1829, The current JSX uses the detail string as the React key in the details.map (render block referencing details and formatWarmupDetails) which can produce duplicate-key warnings if identical strings appear; update the map to use the array index as the key (e.g., map((detail, idx) => ... key={idx})) so each <p> has a stable unique key for this list rendering.
1831-1838: Shared loading state across all buttons.The
loading={triggerWarmupMutation.isPending}applies to all Refresh buttons simultaneously. If a user triggers warmup for multiple agents in quick succession, all buttons will show the loading spinner until the last mutation completes. The same pattern appears for the MCP Reconnect buttons at line 1894.This is acceptable for the current implementation, but for better UX you could track the pending mutation target (e.g.,
pendingAgentId) to show loading only on the relevant button.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@interface/src/routes/Settings.tsx` around lines 1831 - 1838, The Refresh buttons all use the shared triggerWarmupMutation.isPending so every button shows loading; fix by tracking the targeted agent id (e.g., add a local state pendingAgentId) and set pendingAgentId = entry.agent_id when calling triggerWarmupMutation.mutate({ agentId: entry.agent_id, force: true }), then use loading={pendingAgentId === entry.agent_id && triggerWarmupMutation.isPending} on the Button; clear pendingAgentId in the mutation callbacks (onSuccess/onError/onSettled) to stop showing loading only for the completed target. Apply the same pattern for the MCP reconnect mutation (the reconnect mutation instance and its Button) so only the affected agent's button shows a spinner.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@interface/src/components/SetupReadiness.tsx`:
- Around line 209-230: The readiness UI is hidden when any probe fails because
isLoading treats errored queries like missing data; update the isLoading
calculation in the useMemo (the block that builds
items/actionableItems/blockerCount/warningCount) to ignore queries that have
errored (e.g., use .some(q => q.isLoading && !q.isError)) so a failed probe will
not suppress the banner, and also ensure classifySetupReadiness receives enough
info to surface probe errors (pass query.error or the full query objects instead
of only query.data if classifySetupReadiness needs error context).
- Around line 147-170: The readiness check currently hides the
"messaging_unbound" item whenever any configured instance has a binding because
it only triggers when boundInstances.length === 0; update the logic that builds
the messaging checklist (use the existing messaging, configuredInstances,
boundInstances variables) to instead detect if any configured instance remains
unbound (e.g., configuredInstances.some(instance => instance.binding_count ===
0)) and emit the item with id "messaging_unbound" when that condition is true so
unbound adapters are shown even in multi-platform setups.
---
Nitpick comments:
In `@interface/src/components/SetupReadiness.test.ts`:
- Around line 139-145: The expect assertion for the "warmup_degraded" item is
mis-indented compared to surrounding test code; re-indent the block so it aligns
with the other assertions in the same test (match the surrounding indentation
level for the expect(...) call and its nested expect.objectContaining({...})
properties), ensuring the lines containing expect(objectContaining({ id:
"warmup_degraded", severity: "warning", tab: "system-health" })) follow the same
indentation pattern as the other expect assertions in SetupReadiness.test.ts.
In `@interface/src/routes/Settings.tsx`:
- Around line 1823-1829: The current JSX uses the detail string as the React key
in the details.map (render block referencing details and formatWarmupDetails)
which can produce duplicate-key warnings if identical strings appear; update the
map to use the array index as the key (e.g., map((detail, idx) => ...
key={idx})) so each <p> has a stable unique key for this list rendering.
- Around line 1831-1838: The Refresh buttons all use the shared
triggerWarmupMutation.isPending so every button shows loading; fix by tracking
the targeted agent id (e.g., add a local state pendingAgentId) and set
pendingAgentId = entry.agent_id when calling triggerWarmupMutation.mutate({
agentId: entry.agent_id, force: true }), then use loading={pendingAgentId ===
entry.agent_id && triggerWarmupMutation.isPending} on the Button; clear
pendingAgentId in the mutation callbacks (onSuccess/onError/onSettled) to stop
showing loading only for the completed target. Apply the same pattern for the
MCP reconnect mutation (the reconnect mutation instance and its Button) so only
the affected agent's button shows a spinner.
In `@src/tools/send_message_to_another_channel.rs`:
- Around line 153-159: Add a regression test that exercises the named Signal
adapter rewrite in SendMessageTool::call: construct a SendMessageTool with
current_adapter Some("signal:gvoice1") and call SendMessageTool::call with a
target whose adapter is "signal:someaccount" (or plain "signal") and assert the
actual adapter used to send the message equals the current_adapter
("signal:gvoice1"); place the test alongside other SendMessageTool tests and
ensure it fails if the code routes to plain "signal" instead of remapping to
current_adapter, verifying the branch that checks target.adapter and
self.current_adapter.as_ref().filter(|adapter| adapter.starts_with("signal:"))
is exercised.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e9764a36-16e0-4d4f-aca8-44a0bad1192d
📒 Files selected for processing (11)
docs/content/docs/(deployment)/roadmap.mdxdocs/content/docs/(getting-started)/quickstart.mdxinterface/src/api/client.test.tsinterface/src/api/client.tsinterface/src/components/SetupBanner.tsxinterface/src/components/SetupReadiness.test.tsinterface/src/components/SetupReadiness.tsxinterface/src/router.tsxinterface/src/routes/Overview.tsxinterface/src/routes/Settings.tsxsrc/tools/send_message_to_another_channel.rs
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@interface/src/routes/Settings.tsx`:
- Around line 1673-1683: The warmup and MCP status queries use useQuery but
don't surface fetch errors; update the two hooks that call useQuery for queryFn:
api.warmupStatus and api.mcpStatus to also destructure isError and error (e.g.,
const { data: warmupData, isLoading: warmupLoading, isError: warmupError, error:
warmupFetchError } = useQuery(...), and similarly for mcp). Then update the UI
branches that currently render the empty states ("No agents…" and "No enabled
MCP servers…") to check the new isError flags first and render an error
message/component showing the fetch error (use the error object) before falling
back to the empty-state or loading behaviors; reference the warmup* and mcp*
symbols in the rendering logic so failures are clearly distinguished from empty
results.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9e584920-7799-41ea-b7a7-22aaa11cdfdb
📒 Files selected for processing (6)
interface/src/api/client.test.tsinterface/src/api/client.tsinterface/src/components/SetupReadiness.test.tsinterface/src/components/SetupReadiness.tsxinterface/src/routes/Settings.tsxsrc/tools/send_message_to_another_channel.rs
vsumner
force-pushed
the
feat/doctor-cli-slice-1
branch
from
7def3ac to
d2656de
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
interface/src/routes/Settings.tsx (1)
1715-1717: Pending-action state should track multiple in-flight mutations.A single
pendingWarmupTarget/pendingReconnectTargetcan be cleared by one settle while another request is still running, causing transient incorrect button/loading state. Consider tracking pending targets as aSet<string>.Based on learnings: For changes in async/stateful paths (worker lifecycle, cancellation, retrigger, recall cache behavior), include explicit race/terminal-state reasoning in the PR summary and run targeted tests in addition to
just gate-pr.Also applies to: 1731-1733, 1738-1740, 1751-1753
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@interface/src/routes/Settings.tsx` around lines 1715 - 1717, The current single-value pendingWarmupTarget/pendingReconnectTarget state causes races; change these to Set<string> (e.g., pendingWarmupTargets, pendingReconnectTargets) and update the mutation handlers: in onMutate (and similar spots referencing setPendingWarmupTarget/setPendingReconnectTarget) use a functional update to add params?.agentId ?? "__all__" to the Set, and in onSettled/onError remove that id from the Set; ensure UI checks set.has(id) (or set.size>0 for global) and always copy/replace the Set immutably when updating to avoid stale closures.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@interface/src/components/SetupReadiness.test.ts`:
- Around line 19-41: The providers object inside the configuredProviders() test
fixture is missing the required github_copilot field declared by ProviderStatus;
add "github_copilot: false" (or the appropriate boolean) to the providers map in
the configuredProviders() fixture in SetupReadiness.test.ts so the fixture
conforms to ProviderStatus and type-checks correctly.
In `@interface/src/components/SetupReadiness.tsx`:
- Around line 225-233: The probeErrors array-of-tuples is being inferred as
(string | UseQueryResult)[] which fails strict typing when passed to
classifySetupReadiness's probeErrors?: string[]; fix by narrowing the tuple
types—either annotate the tuple array as an array of [string, UseQueryResult]
tuples (e.g., const probes: [string, UseQueryResult][] = [...]) or append as
const to the literal array so the first elements are string literals, then
filter and map to produce a string[] before passing to classifySetupReadiness
(refer to the probeErrors variable and the classifySetupReadiness call).
In `@interface/src/routes/Settings.tsx`:
- Around line 1660-1662: The code currently pushes raw backend error text
(entry.status.last_error) into the UI details array; replace this by
sanitizing/redacting that string before rendering (e.g., map
entry.status.last_error through a sanitizeError or redactError utility that
strips provider/runtime details and returns either a sanitized message or a
generic "Error occurred (code: XYZ)" plus a non-sensitive structured code).
Update both occurrences that push entry.status.last_error (the block around
details.push(entry.status.last_error) and the similar block at 1871-1875) to use
the sanitizer function and surface only the sanitized message or generic
fallback, ensuring no raw backend error text is directly rendered.
---
Nitpick comments:
In `@interface/src/routes/Settings.tsx`:
- Around line 1715-1717: The current single-value
pendingWarmupTarget/pendingReconnectTarget state causes races; change these to
Set<string> (e.g., pendingWarmupTargets, pendingReconnectTargets) and update the
mutation handlers: in onMutate (and similar spots referencing
setPendingWarmupTarget/setPendingReconnectTarget) use a functional update to add
params?.agentId ?? "__all__" to the Set, and in onSettled/onError remove that id
from the Set; ensure UI checks set.has(id) (or set.size>0 for global) and always
copy/replace the Set immutably when updating to avoid stale closures.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 88ffaff2-4f9f-4f11-b631-3b8a5121b3ca
📒 Files selected for processing (11)
docs/content/docs/(deployment)/roadmap.mdxdocs/content/docs/(getting-started)/quickstart.mdxinterface/src/api/client.test.tsinterface/src/api/client.tsinterface/src/components/SetupBanner.tsxinterface/src/components/SetupReadiness.test.tsinterface/src/components/SetupReadiness.tsxinterface/src/router.tsxinterface/src/routes/Overview.tsxinterface/src/routes/Settings.tsxsrc/tools/send_message_to_another_channel.rs
🚧 Files skipped from review as they are similar to previous changes (3)
- interface/src/router.tsx
- interface/src/routes/Overview.tsx
- interface/src/api/client.test.ts
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
interface/src/components/SetupReadiness.tsx (1)
97-122: Consider surfacing warmup info even without providers.Currently, warmup status is only evaluated when
providers?.has_anyis true (line 97). If providers fail to load but warmup data is available, users won't see warmup-related items. This may be intentional (warmup is meaningless without providers), but consider whether a probe failure for providers should still allow warmup items to render when warmup data exists.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@interface/src/components/SetupReadiness.tsx` around lines 97 - 122, The warmup block is currently gated by providers?.has_any so warmup items won't render when provider probe fails; change the guard to check warmup data instead (e.g., if (warmup?.statuses?.length)) so the warmup evaluation always runs when warmup exists; keep the existing logic that computes degradedAgents via warmup.statuses.filter(...).map(...), warmAgents, and the items.push(...) calls (ids "warmup_degraded" and "warmup_pending" and use of formatAgentList) unchanged so only the outer condition is adjusted to rely on warmup presence rather than providers.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@interface/src/components/SetupReadiness.tsx`:
- Around line 97-122: The warmup block is currently gated by providers?.has_any
so warmup items won't render when provider probe fails; change the guard to
check warmup data instead (e.g., if (warmup?.statuses?.length)) so the warmup
evaluation always runs when warmup exists; keep the existing logic that computes
degradedAgents via warmup.statuses.filter(...).map(...), warmAgents, and the
items.push(...) calls (ids "warmup_degraded" and "warmup_pending" and use of
formatAgentList) unchanged so only the outer condition is adjusted to rely on
warmup presence rather than providers.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a67e1f3e-1d25-4273-a925-4c62c4fa12f0
⛔ Files ignored due to path filters (3)
interface/bun.lockis excluded by!**/*.lock,!**/*.lockinterface/package.jsonis excluded by!**/*.jsoninterface/tsconfig.jsonis excluded by!**/*.json
📒 Files selected for processing (4)
interface/src/api/client.test.tsinterface/src/components/SetupReadiness.test.tsinterface/src/components/SetupReadiness.tsxinterface/src/routes/Settings.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- interface/src/api/client.test.ts
vsumner
force-pushed
the
feat/doctor-cli-slice-1
branch
from
6ea89b9 to
e220f2e
Compare
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
doctorcommandSystem Healthsettings panel for warmup and MCP runtime state, plus the follow-up review fixes for routing and reconnect targetingChanges
System Healthpanelsend_message_to_another_channel.rssojust gate-prpasses cleanlyTest plan
bun test interface/src/components/SetupReadiness.test.ts interface/src/api/client.test.tsbun run buildininterface/just preflightjust gate-prNote
AI Summary: This PR replaces the
doctorCLI with integrated readiness surfaces. New UI surfaces show setup status (missing providers, secrets, MCP connectivity) in the shell banner and Overview page. A new System Health settings panel provides per-agent warmup state and MCP server status. Includes frontend warmup/reconnect methods and targeted tests. Fixes two pre-existing clippy warnings. Changes span frontend components (SetupReadiness.tsx), client API methods, documentation, and backend tests.Written by Tembo for commit 1e872df.