feat(connections): open a remote MCP server the way a Composio provider opens (#821) - #829
Conversation
) The two lookups a detail view for an MCP server needs, as pure functions so they can be pinned without a DOM. `mcpProviderSlug` mirrors `mcp_provider` in `src/metering/oauth.rs` and is fed to the existing `callsForProvider` rather than teaching that function a prefix: prefix-matching there would re-create by hand the `gmail` / `mcp:gmail` collision the namespace was chosen to prevent. `mcpStanding` keeps `enabled` apart from the last probe. MCP has no connection object, so "connected" is assembled from two facts, and a server nobody has pressed Test on has no probe at all — neither reachable nor broken, and no single badge renders that honestly. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`ProviderDetail` takes a `ConnectionSubject` union instead of a `GridProvider`, and grows an MCP arm beside the Composio one. A second MCP-specific panel was the alternative and is what tinyhumansai#404 exists to prevent: two surfaces for one idea acquire two vocabularies and drift. The handlers travel inside the Composio variant rather than as sibling props, so an MCP subject cannot be handed a Composio revoke and a Composio subject cannot be opened without one. That is the same defect `ConnectionsView.disconnect` routes around — `disconnectConnection` answers 200 for a Composio provider while releasing nothing — expressed as a type rather than as a comment asking callers to be careful. The MCP arm is read-only: enable, Test, Tools and Remove stay on the row until the panel is proven. What it adds is what the row cannot say. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The row's name becomes the handle on its detail view, with a chevron rather than hover styling alone: a name that opens something has to be distinguishable from one that does not before the pointer is on it, which on a touch screen is never. Rendered from this section rather than from the page above, because the live health an operator just pressed Test for lives in this component's state — and because this section is also the whole of Settings, MCP Servers, which gets the detail view for free. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 18 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (8)
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 |
oxoxDev
left a comment
oxoxDev
left a comment
There was a problem hiding this comment.
Opening MCP into the same panel on a ConnectionSubject union, rather than building a second MCP-specific one, is the decision #404 exists to force — and citing #414 as the time that already went wrong here is the right way to justify it. A second panel is always the cheaper next step and always the thing that has to be undone later.
The usage lookup is the part I checked hardest, because I asked about this namespace on #744, and it is handled better than I hoped:
Prefix-matching here would re-create by hand the collision that naming was chosen to prevent.
Matching on the bare slug against mcp:<server> — rather than stripping a prefix and comparing — means a company with a Composio gmail and an MCP server its operator also called gmail cannot read one total as the other's. That is the consumer-side half of #698's namespacing, and getting it wrong here would have quietly undone the reason the prefix exists. The prefix is a key, not a label, which also answers the question I left on #744 about whether mcp:linear reaches the operator's eye.
"Zero is a real answer, not a missing one" is the same discipline #819 and #827 apply, and it is worth the sentence: because every successful call is metered, a provider with no row has genuinely had none, so the panel can say zero rather than hedging. A claim table that says how each line is backed is the right shape for a read-only slice.
0 major. 1 question. Approving.
Question — this is the third cross-language mirror, and drift here makes "zero" lie
mcpProviderKey is described as "the mirror of mcp_provider in src/metering/oauth.rs: trimmed, lowercased…". That makes three mirrored predicates now living in this repo:
mayCarryACredential(TS) mirroringmay_carry_a_credential(Rust) — #747hub_accepts_redirect_uri(Rust) mirroring the backend'sisLoopbackHttpUri(JS) — #818- this one
The first two have something this one does not. #747's is advisory: the core enforces, so a drift yields a confusing UI rather than a wrong result. #818's mirrors an external contract and is built to be deleted when backend#1243 lands, with a test that goes red to force it.
This one mirrors an internal Rust function that can change at any time, and a drift is silent and wrong in a specific way: if mcp_provider's normalisation moves — a different sanitisation, a trailing-dot rule, anything — the TS key stops matching, byProvider returns no row, and the panel reports zero. Which this PR's own doc defines as "a real answer, not a missing one". The failure mode is the panel confidently telling an operator their MCP server has had no calls.
Cheap ways to close it, both with precedent here: assert the two agree on a shared fixture list of server names, the way I suggested for mayCarryACredential; or generate the constant from the Rust side, which is what #728 proposed for the gated-tool labels and #761 discussed again. Either turns a silent wrong answer into a failing test.
Not blocking — the normalisation is simple today and unlikely to move soon. But "unlikely to move" is what makes a mirror worth pinning rather than watching.
Before merging: base is 8 commits behind and the lanes are still reporting. #828 and #827 both land on the same surface, so worth a rebase and a re-check rather than trusting the current run.
![tinysweeper[bot]](https://avatars.githubusercontent.com/in/4526585?s=60&v=4){kind=small}
There was a problem hiding this comment.
tinysweeper found nothing blocking. Approving.
$0.0901 · 117,235 in / 18,905 out · 70,519 cached (60%) · z-ai/glm-5.2
critique: $0.0505 · 42,440 in / 11,196 out · 13,405 cached (32%) · z-ai/glm-5.2
security: $0.0156 · 30,118 in / 3,284 out · 24,160 cached (80%) · z-ai/glm-5.2
tests: $0.0151 · 21,897 in / 3,454 out · 15,615 cached (71%) · z-ai/glm-5.2
description: $0.0088 · 22,780 in / 971 out · 17,339 cached (76%) · z-ai/glm-5.2
What this change touches8 files, +980 -215 across 6 components. It reaches 6 untouched components (60 graph nodes walked). 2 further components left out to keep the diagram readable. flowchart LR
n0["frontend/src/views/connections<br/>2 files +493 -187<br/>1 finding"]:::flagged
n1["frontend/test/unit<br/>2 files +262 -17"]:::changed
n2["frontend/src/lib<br/>1 file +123 -3"]:::changed
n3["frontend/test/e2e<br/>1 file +49 -0"]:::changed
n4["frontend/src/views<br/>1 file +27 -8"]:::changed
n5["docs/modules<br/>1 file +26 -0"]:::changed
n6["frontend/src/components/ui<br/>7 files reached"]:::impacted
n7["frontend/src/api<br/>4 files reached"]:::impacted
n8["frontend/src/views/connections<br/>3 files reached"]:::impacted
n9["frontend/src/lib<br/>1 file reached"]:::impacted
n10["frontend/src/tour<br/>1 file reached"]:::impacted
n11["frontend/src/views<br/>1 file reached"]:::impacted
n8 -->|48 refs| n6
n8 -->|18 refs| n7
n6 -->|16 refs| n9
n11 -->|12 refs| n6
n8 -->|9 refs| n11
n11 -->|7 refs| n8
n11 -->|4 refs| n7
n11 -->|2 refs| n9
n11 -->|1 ref| n10
classDef changed fill:#0d4429,stroke:#238636,color:#e6edf3
classDef impacted fill:#161b22,stroke:#6e7681,color:#c9d1d9
classDef flagged fill:#5a1e02,stroke:#d93f0b,color:#ffffff
classDef blocking fill:#67060c,stroke:#f85149,color:#ffffff
Green: changed. Grey: untouched, reached through an import or a call. Orange: has findings. Red: has a finding that blocks the merge.
Changed files
|
tinysweeper
Bot
added
the
priority: p3
There was a problem hiding this comment.
tinysweeper found nothing blocking. Approving.
$0.0810 · 114,712 in / 20,874 out · 89,828 cached (78%) · z-ai/glm-5.2
critique: $0.0447 · 40,013 in / 14,967 out · 34,502 cached (86%) · z-ai/glm-5.2
security: $0.0141 · 30,128 in / 2,316 out · 22,915 cached (76%) · z-ai/glm-5.2
tests: $0.0109 · 21,897 in / 1,696 out · 15,628 cached (71%) · z-ai/glm-5.2
description: $0.0113 · 22,674 in / 1,895 out · 16,783 cached (74%) · z-ai/glm-5.2
| {/* The host scrubs this string — it can carry no credential, no | ||
| response body and no query string — which is why it can be shown | ||
| verbatim rather than re-spelled into a category. */} | ||
| {health && health.status !== "ok" && health.message && ( |
There was a problem hiding this comment.
Cover the MCP probe-message branch in the render suite
The new McpBody component renders the probe's error message when a probe ran but did not succeed, but no test exercises this path. The render suite covers the probe line (reachable, not-probed, needs_config, error) via mcpStanding, and the unit tests check mcpStanding's probe string, but no test passes a health with status !== "ok" and a non-empty message and asserts that health.message appears in the rendered panel. If this branch were removed or its condition changed, nothing in the suite would fail.
[RULE] Add focused tests with every behavior change. ·
tinysweeper
Bot
added
priority: p2
2 participants
Closes #821.
Follows #819, which is already merged —
ProviderDetail.tsxandconnection-detail.tscome from it, and this branch was cut from its head. Thediff here is #821's own: three commits, eight files.
What this does
#819 gave a Composio provider a detail view and deliberately left MCP as a list
— the uneven half of #404, and the wrong half to leave for a company routing its
real work through MCP servers. This opens a remote MCP server into the same
panel, on a
ConnectionSubjectunion. Not a second MCP-specific panel, which isexactly what #404 exists to prevent, and what happened to the MCP screen once
already (#414).
Read-only, as the issue's "smallest first slice" asks. Enable,
Test,Toolsand
Removestay on the row; the panel adds what the row cannot say:mcpStanding()—enabledand the last probe kept apartbyProviderundermcp:<server>(src/metering/oauth.rs, #698/#744)Two decisions worth review
callsForProviderwas not taught the prefix, though the issue suggests it"would take the prefix". Its existing test pins the opposite, and correctly:
prefix-matching would re-create by hand the
gmail/mcp:gmailcollision thenamespace was chosen to prevent.
mcpProviderSlug()builds the namespaced keyinstead and the lookup stays one exact match on one key.
The panel renders from
McpServersSection, notConnectionsView. The livehealth an operator just pressed
Testfor lives in that component's state, sohoisting it would let the panel and the row it opened from report different
health. It also means Settings, MCP Servers gets the detail view for free.
Two smaller ones.
mcpStandingkeeps "never probed" apart from every probestatus including
unknown— a server nobody has pressedTeston is neitherreachable nor broken, and it is the common case on a host without the
openhumanfeature. AndprobedOnreturns null rather than the epoch for azero
checkedAtMillis, on the same grounds as the issue's rule about zeros.The row's name got a chevron rather than hover styling alone — caught by looking
at a screenshot of the real page, where the clickable name was indistinguishable
from plain text.
Commands run
npm run typecheck,typecheck:unit,typecheck:e2e,npm run buildnpm test— 645 pass (52 files), including 22 render tests over both armsnpx playwright testagainst a realtarget/debug/opencompanyhost —124 pass, 10 skipped (live-brain), including a new
mcp.spec.tscaseasserting the four claims above
scripts/ci/assert-design-tokens.sh,scripts/ci/assert-md-line-cap.shNot covered
The Composio arm has no e2e walk. The default-feature host serves no Composio,
so the grid renders no openable tile — its coverage is the vitest render suite,
which is where #819 put it for the same reason.
🤖 Generated with Claude Code