connections: categorised, searchable connector grid (#600) - #639
Conversation
|
Warning Review limit reached
Next review available in: 16 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
There was a problem hiding this comment.
1 major, 1 minor. The major is sequencing with #637; the diff itself is sound.
"The layout was a symptom, not the defect" is the right read, and the evidence is the five-line map(|entry| normalize(&entry.slug)) that reduces a six-field catalog entry to a lowercased string one layer before the console. The flat list really was the honest rendering of what the frontend was handed. Pointing at the backend's own comment — that it assembles name/logo/description/categories so the frontend can read them straight from here — closes the argument.
Three decisions I checked and would keep:
effectiveCatalogadded alongsideeffectiveToolkits, withOpenModeToolkits::slugs()deriving the latter, so the two cannot disagree about what is on offer. Adding a richer field beside an existing contract rather than replacing it is the right call when the old shape is still all an authorize call needs.CatalogEntryliving incompany::composiorather thanharnessbecause the status route is always compiled whilesrc/harness/is behind theopenhumanfeature. Same reasoning #631 used to movegrants_cover_server— good that the two land on the same rule independently.mapComposioCategoryreturningnullon no match so the caller falls through to the slug/name heuristic. I went looking for a provider that could silently vanish from the grid when Composio invents a category nobody mapped; the fallthrough is what prevents it. Worth keeping that property pinned if it is not already.
Major — collides with #637 on the Composio core, both directions of the stack
#637 (feat(connections): broker providers through the company's own TinyHumans credential, also yours) shares five files with this PR:
frontend/src/api/composio.ts src/company/composio.rs
.../connections/ComposioSection.tsx src/harness/composio.rs
src/server/ops/composio.rs
Both reshape Composio resolution and the section that renders it, and neither mentions the other. #637 establishes the credential tier that decides whether a company can connect anything; this decides how the 123 options are presented. That argues for #637 first and this rebasing onto it — but it should be a stated order rather than whichever of you pushes last. I have raised the same point on #637.
Minor — mapComposioCategory is a second copy with no drift detection
It is ported from OpenHuman's mapComposioCategory in toolkitMeta.tsx, driving the same catalog from the same free-form strings, and the doc comment says keeping the two in step is cheaper than diverging. Agreed on the trade — but nothing makes a divergence visible. The two consoles will bucket the same provider differently and both will look correct in isolation.
Cheapest fix is a comment on each side naming the other as its twin, so a future edit at least knows a sibling exists. If the substring list ever grows past a handful of entries, a shared fixture asserting both map a known set identically would be worth more.
…inyhumansai#600) Review on tinyhumansai#639: `mapComposioCategory` is a second copy of OpenHuman's `toolkitMeta.tsx` function, and nothing made a divergence visible. The two consoles would bucket the same provider differently and both would look correct in isolation. There is no shared package to hoist it into, so the guard is cheap and explicit rather than mechanical: - A notice on this copy naming the twin by repository and path, with a matching one to land on the OpenHuman side. - `keeps the buckets its OpenHuman twin produces` pins the substring table case-by-case. It is not asserting that the code does what the code does — it is the diff a divergence has to survive. Editing the table without editing the list fails the suite, which is the moment the editor learns a twin exists. - `orders its buckets so the first hit wins` pins the branch order too. Both copies return on first match, so an entry carrying several categories depends on Chat → Social → Productivity → Platform; reordering one side only is the subtlest available drift. Also pins the property the review went looking for and could not find stated outright: a category string Composio invents tomorrow, matched by neither the table nor the keyword heuristic, still yields a tile in "Tools & Automation" rather than a provider silently leaving the grid. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Stacking note (raised in review on #637): this PR and #637 share the Composio core — #637 lands first. It establishes the company-credential tier and moves the credential-tier derivation into a single The overlap is adjacent rather than semantic — #637 touches credential resolution and the credential card, this one touches catalog presentation and the connector grid — so the rebase should be mechanical. Two things to pick up when rebasing:
Also worth mirroring here: #637's credential card carries copy distinguishing the TinyHumans account key from the model-provider key on the Inference card (#634), since all three cards end up on this screen. |
|
Re-checked at the new head — narrowing rather than re-blocking. Closed, and done better than asked. I suggested a comment naming the twin; you added Still open — the Major. No stated order against #637, which shares five files with this including |
|
Thanks — both addressed. Taking them in reverse order, since the minor is the one with code. Minor —
|
| File | Conflict | Resolution |
|---|---|---|
src/company/composio.rs |
#637 adds use std::sync::Arc, this adds use serde::Serialize |
both kept |
src/server/ops/composio.rs |
#637 drops token_configured (folded into resolve_credential), this adds CatalogEntry |
#637's import + CatalogEntry |
No semantic interaction — #637 rewrites the credential half of effective_status, this touches the catalog half (effective_toolkits, fetch_catalog, the DTO). Stacked, cargo test --locked passes 1822 + 10 + 1 and --features composio passes 88, the single failure being the pre-existing network case in the PR description.
One deliberate deviation from your suggestion: I have not retargeted this PR's base to #637's branch. #637 is currently 55 commits behind main, so stacking now would run this PR's CI against a stale base and throw away the green run against current main — and any rewrite of #637 under review would churn this branch. The order is stated in prose and I'll rebase onto main and re-verify the moment #637 merges. If you'd rather have it enforced structurally by GitHub, say so and I'll retarget — that's your call, not mine, and it's a one-command change.
Nothing here alters the diff you already reviewed: effectiveCatalog is still additive beside effectiveToolkits, CatalogEntry still lives in company::composio for the always-compiled reason, and the only new code is tests.
…inyhumansai#600) `list_catalog_toolkits` read the backend's `catalog[]` — whose entries carry `slug`, `name`, `logo`, `description` and `categories` — and reduced every one of them to a lowercased slug. That `Vec<String>` then survived the cache, `OpenModeToolkits` and the status DTO unchanged, so the console received 123 bare strings: nothing to group by, nothing to brand with, and nothing to search but the slug. Its flat list was the honest rendering of what it was handed. The backend already assembles all of it and says it does so "for the frontend"; this stops discarding it one layer short. - `CatalogEntry` lands in `company::composio`, not the harness: the status route is always compiled and `src/harness/` is behind the `openhuman` feature. Same reason `TOKEN_KEY` lives there. - The DTO gains `effectiveCatalog` **alongside** `effectiveToolkits` rather than replacing it. The slug list is the existing contract and is still all an authorize call needs. - De-duplication is `or_insert_with`, not `collect()` into the map: collecting keeps the LAST value per key, and a duplicate catalog entry is typically the degenerate one — the mock's metadata-less `Gmail (dup)` silently blanked the real Gmail's description until this was explicit. - Manifest, fallback, and pre-dynamic-catalog backends yield slug-only entries. That is a first-class state, not a reason to drop a provider. Agent-side admission is untouched: `toolkit_allowed` takes slugs and never consulted this function. This widens what is described, not what is permitted. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ai#600) 123 providers rendered as one flat vertical list of name + "Sign in" rows, twelve at a time behind a "Show all 123 providers" button. Ports the four behaviours OpenHuman's Skills grid already uses on the same catalog, now that the host forwards the metadata they need. - **Category chips** over a fixed, ordered bucket list, offering only buckets that are actually populated — a chip that reliably yields an empty grid teaches the operator to distrust the whole row. - **Buckets derived from Composio's own `categories[]`**, by substring, ported from `mapComposioCategory`. This is the piece worth copying verbatim: 123 providers bucket themselves and provider 124 does too, with no edit here. A slug-only entry falls through to a slug/name keyword heuristic rather than vanishing. - **Search over name, slug AND description**, composed with the chip rather than replacing it. Matching on description is new — there was no description on the wire before — so "invoices" now reaches Stripe and QuickBooks. - **A dense branded tile grid**, replacing the rows. The preview cut is deleted rather than relabelled: it was a workaround for a flat list being unreadable, and a grid does not need it. The tile is the affordance — an 8.5rem tile has no room for a label and a button both. Non-actionable tiles (connected, or a viewer who cannot manage) render as a div, not a disabled button, so "Gmail, connected" stays in the reading order for a member who opened this panel to learn exactly that. Logos are best-effort by construction — derived from the slug wherever the backend published none — so the tile catches the 404 and falls back to a monogram instead of a broken image. Verified against a live host with a 122-provider mock catalog: the Chat chip narrows to 10, "invoices" finds Stripe and QuickBooks, and connected providers sort first. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…inyhumansai#600) Review on tinyhumansai#639: `mapComposioCategory` is a second copy of OpenHuman's `toolkitMeta.tsx` function, and nothing made a divergence visible. The two consoles would bucket the same provider differently and both would look correct in isolation. There is no shared package to hoist it into, so the guard is cheap and explicit rather than mechanical: - A notice on this copy naming the twin by repository and path, with a matching one to land on the OpenHuman side. - `keeps the buckets its OpenHuman twin produces` pins the substring table case-by-case. It is not asserting that the code does what the code does — it is the diff a divergence has to survive. Editing the table without editing the list fails the suite, which is the moment the editor learns a twin exists. - `orders its buckets so the first hit wins` pins the branch order too. Both copies return on first match, so an entry carrying several categories depends on Chat → Social → Productivity → Platform; reordering one side only is the subtlest available drift. Also pins the property the review went looking for and could not find stated outright: a category string Composio invents tomorrow, matched by neither the table nor the keyword heuristic, still yields a tile in "Tools & Automation" rather than a provider silently leaving the grid. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
c00130f to
1f7779a
Compare
Live end-to-end test reportPosting this because the green checks above do not cover half of this PR. CI never builds the Setup: Host — the contract, on real data
The 3 entries without categories are deliberate — they exercise the slug/name heuristic fallthrough rather than the Host — the honesty contract, which no unit test reaches end to endKilled the catalog backend mid-session and forced a re-fetch through the credential-write eviction path: Restored it, re-evicted: Console — 20 assertions in a real browserThe defect is gone: all 122 tiles render at once, no "Show all N providers" button survives, multi-column grid. Ordering: the five connected providers sort to the front. Metadata reaches the tile: >100 logo images; the backend's description becomes the tile tooltip verbatim ( Buckets come from Composio's own strings: chips offer exactly the populated set; Search reaches what a name cannot: Chip AND search compose: Accessibility calls held: a connected tile is a The action is wired: clicking a tile POSTs No unexpected console errors. One check failed first, and it was my assertion
What this does not cover
|
oxoxDev
left a comment
There was a problem hiding this comment.
0 major. Approving — the sequencing item is closed, and answered harder than asked.
I asked for a stated order. You added ## Sequencing — #637 lands first with the reasoning (credential tier before presentation, and #637 is the older PR), then went further and actually stacked this branch's commits onto fix/586-brokered-company-credential locally, ran the suite, and enumerated the entire overlap — two import lines in src/company/composio.rs and src/server/ops/composio.rs, with the resolution for each.
That converts "these five shared files will probably be fine" into a checked claim. It is also the answer to the thing I could not verify from outside: five shared files between two large PRs is exactly where a clean textual merge hides a semantic break, and you demonstrated it does not here rather than asserting it.
The twin notice and mapComposioCategory keeps the buckets its OpenHuman twin produces from the earlier round stand. Nothing further from me — good to merge once #637 lands.
Closes #600.
Sequencing — #637 lands first
This PR is second in line behind #637 (
feat(connections): broker providers through the company's own TinyHumans credential). Stated here rather than left to whichever branch pushes last, per review.The two share five files. #637 establishes the credential tier that decides whether a company can connect anything; this decides how the resulting options are presented. Credential before presentation is the right order, and #637 is the older PR.
They compose. I stacked this branch's two commits onto
fix/586-brokered-company-credentiallocally and ran the suite. The entire overlap is two import lines:src/company/composio.rsuse std::sync::Arc, this addsuse serde::Serializesrc/server/ops/composio.rstoken_configured(moved intoresolve_credential), this addsCatalogEntryCatalogEntryNo semantic interaction: #637 rewrites
credential_source_for/effective_status's credential half, this toucheseffective_toolkits,fetch_catalogand the DTO's catalog half. Stacked,cargo test --lockedpasses 1822 + 10 + 1 and--features composiopasses 88 (the one failure is the pre-existing network case noted below).I have deliberately not retargeted this PR's base to #637's branch. #637 is currently 55 commits behind
main, so stacking now would run this PR's CI against a stale base and lose the green run against currentmain. When #637 merges I will rebase this ontomainand re-verify.Summary
Settings → Connections rendered 123 providers as one flat vertical list, twelve at a time behind a "Show all 123 providers" button.
The layout was a symptom, not the defect.
list_catalog_toolkits(src/harness/composio.rs) read the backend'scatalog[]— whose entries carryslug,name,logo,descriptionandcategories— and reduced every one of them to a lowercased slug:That
Vec<String>then survived every layer downstream unchanged — the catalog cache,OpenModeToolkits,ComposioStatusDto, andfrontend/src/api/composio.ts— so the console received 123 bare strings. Nothing to group by, nothing to brand with, and nothing to search but the slug. Its flat list was the honest rendering of what it was handed. The backend already assembles all of this metadata andbackend/src/services/composio/catalog.tsstates it does so "so the frontend readsname/logo/description/categoriesstraight from here"; it was being discarded one layer short.#397built this surface sized for 8 providers and#556un-capped it to 123 without revisiting the shape — thePREVIEW_COUNT = 12collapse is the seam where that showed.Host
CatalogEntry(slug,name,description,logo,categories) lands incompany::composio, not the harness — the status route is always compiled andsrc/harness/is behind theopenhumanfeature. Same reasonTOKEN_KEYandbackend_url_or_defaultlive there.OpenModeToolkitsand the status DTO.effectiveCatalogalongsideeffectiveToolkitsrather than replacing it: the slug list is the existing contract and is still all an authorize call needs.OpenModeToolkits::slugs()derives it, so the two cannot disagree about what is on offer.or_insert_with, notcollect()into theBTreeMap— collecting keeps the last value per key, and a duplicate catalog entry is typically the degenerate one. The test mock's metadata-lessGmail (dup)silently blanked the real Gmail's description until this was explicit.Console
Ports the four behaviours from OpenHuman's
Skills.tsx/toolkitMeta.tsx, now that the metadata they need is on the wire:categories[]by substring (mapComposioCategory, near-verbatim). This is the piece worth copying: 123 providers bucket themselves, and provider 124 does too, with no edit on either side of the wire. A slug-only entry falls through to a slug/name keyword heuristic rather than vanishing.Two deliberate calls in the tile: the whole tile is the affordance (an 8.5rem tile has no room for a label and a button both), and non-actionable tiles — connected, or a viewer who cannot manage — render as a
divrather than a disabled button, so "Gmail, connected" stays in the reading order for a member who opened this panel to learn exactly that. Logos are best-effort by construction (derived from the slug wherever the backend published none), so the tile catches the 404 and falls back to a monogram instead of a broken image.API Or Behavior Changes
GET …/composiogainseffectiveCatalog— an array of{slug, name, description, logo, categories}, same providers and same order aseffectiveToolkits. Additive:effectiveToolkitsis unchanged in type, contents and order, so existing consumers need no change. Pinned by the DTO key-order test.harness::composio::list_catalog_toolkitsreturnsVec<CatalogEntry>instead ofVec<String>. Internal; its only non-test caller is the open-mode status path.toolkit_allowedtakes slugs and never consulted this function. This widens what is described, not what is permitted — and a non-empty manifest allowlist still cannot be widened by the catalog.Tests
cargo fmt --all -- --checkcargo clippy --locked --all-targets -- -D warningscargo clippy --locked --no-deps --features openhuman,tinycortex --all-targets -- -D warningscargo build --all-targetscargo test --locked— 1854 + 10 + 1 passedcargo +1.96.1 test --locked --features openhuman,tinycortex --tests— 2775 + 10 + 1 + 11 + 2 passedcargo +1.96.1 test --features composio composio— 84 passed (see caveat below)npm run typecheck,typecheck:unit,typecheck:e2e,npm test(315 passed),npm run buildNew coverage:
list_catalog_toolkits_carries_the_display_metadata— the host-side regression test. The mock catalog now publisheslogo/description/categories, so a mock that omitted them could not have caught this bug. Also pins first-entry-wins de-duplication andlogo: Nonefor an unpublished logo.list_catalog_toolkits_falls_back_to_the_plain_allowlistnow asserts slug-onlyCatalogEntryvalues.mapComposioCategory,providerLabel,availableCategories,filterByCategory,permissionHint, description-search, metadata-carrying, and slug-derived category guessing.visibleProviderRowsnow asserts the absence of a preview cut — a test still asserting one would be pinning the bug.Added after review (drift guard on the ported
mapComposioCategory):keeps the buckets its OpenHuman twin produces— pins the substring table case-by-case. Not asserting that the code does what the code does: it is the diff a divergence has to survive, so editing the table without editing the list fails the suite. That is the moment the editor learns a twin exists.orders its buckets so the first hit wins, not the last— both copies return on first match, so a multi-category entry depends on Chat → Social → Productivity → Platform. Reordering one side only is the subtlest available drift and was otherwise unpinned.never drops a provider whose category Composio has just invented— the property the review went looking for and could not find stated outright. An unmapped category still yields a tile in "Tools & Automation" rather than a provider silently leaving the grid.Verified against a running host
Not just unit-tested — driven end to end against a
--features composiohost pointed at a 122-provider mock catalog. The status route returnedcatalogSource=backendwith 122 slugs and 122 metadata entries; all 122 tiles rendered with real Composio logos, connected-first; the Chat chip narrowed to 10; and"invoices"returned Stripe and QuickBooks — a description match that was impossible before this change.Two caveats, both stated plainly
server::ops::composio::tests::an_admin_is_unaffectedfails on my machine, and it is pre-existing. It expects a 409 from the build check, but under--features composiothe handler dials the realhttps://api.tinyhumans.ai, which my box can reach, so the 401 surfaces as a 502. Confirmed identical on a stashed tree. It passes in the default build, and CI does not build thecomposiofeature.inBuild: true, which needs acomposio-feature host, and no CI lane builds one. An e2e spec would typecheck and never run — precisely the trapplaywright.config.tsdocuments from workflows: the toolbar picker shows neither the selected workflow nor a placeholder #406. Coverage sits in unit tests plus the host tests instead; if a composio e2e lane is wanted, that is worth its own issue.Documentation
No doc changes needed.
docs/modules/server/does not document the Composio status DTO shape, and the module-level rustdoc inserver/ops/composio.rs,server/ops/composio_toolkits.rsandcompany/composio.rs— which is where this surface is actually documented — is updated in place, including whyCatalogEntrylives incompany::composiorather than beside the agent-facingcomposio_catalog::CatalogToolkit.Not in scope
MANAGED_COMPOSIO_TOOLKITSfallback catalog OpenHuman keeps. Skipped deliberately, as the issue anticipated: OpenCompany'scatalogSource: "fallback"path already covers the degraded case honestly, and importing a second hand-maintained list of 119 names would reintroduce exactly the drift this change removes.🤖 Generated with Claude Code