fix(connections): route the categorised tiles through Composio (#599) - #633
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe connection catalog now includes canonical Composio toolkit slugs and route selection helpers. ChangesConnection routing
Estimated code review effort: 4 (Complex) | ~45 minutes Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant ConnectionsView
participant ComposioStatus
participant ComposioAuthorize
participant CompanyConnections
ConnectionsView->>ComposioStatus: Probe Composio reachability
ComposioStatus-->>ConnectionsView: Return reachability and toolkit access
ConnectionsView->>ComposioAuthorize: Authorize provider toolkit
ComposioAuthorize-->>ConnectionsView: Return hosted OAuth URL
ConnectionsView->>CompanyConnections: Poll connection state
CompanyConnections-->>ConnectionsView: Return updated state
ConnectionsView->>ConnectionsView: Render connected or unavailable tile
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
frontend/test/unit/connection-route.test.ts (1)
49-73: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a case that pins direct-id precedence.
Every fixture holds one row, so the direct
states[provider.id]lookup and the normalized scan return the same value. A change that removed the direct lookup would still pass. Add a case with both a manifest row and a Composio-keyed row for the same tile.♻️ Proposed test
+ it("prefers the row keyed by the tile's own id over a normalized match", () => { + const manifest = { provider: "google-calendar", connected: false }; + const composio = { provider: "googlecalendar", connected: true }; + expect( + connectionStateFor(tile("google-calendar"), { + "google-calendar": manifest, + googlecalendar: composio, + }), + ).toBe(manifest); + });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/test/unit/connection-route.test.ts` around lines 49 - 73, Add a test in the connectionStateFor suite with both a direct manifest-id state and a Composio-keyed state for the same tile, using distinct objects. Assert that connectionStateFor returns the direct states[tile.id] entry, preserving direct-id precedence over the normalized scan.frontend/src/views/ConnectionsView.tsx (1)
126-132: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winCancel in-flight poll iterations, not only pending timers.
The cleanup clears scheduled
setTimeoutids. It does not stop apolliteration that is already awaitinglistComposioConnections. When that request resolves after a company switch, the iteration callssetBusy, can raise a success toast for the previous company, and re-arms a timer in the newpollTimers.current. Add a generation guard so an iteration started under one company stops after the switch.♻️ Proposed guard
+ // Bumped on every company change; a poll iteration that started under an + // older generation stops instead of touching the new company's state. + const pollGeneration = useRef(0); + useEffect(() => { const timers = pollTimers.current; return () => { + pollGeneration.current += 1; Object.values(timers).forEach((id) => window.clearTimeout(id)); pollTimers.current = {}; }; }, [company]);Then capture
const generation = pollGeneration.current;inconnectComposioand return early frompollwhengeneration !== pollGeneration.current.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/views/ConnectionsView.tsx` around lines 126 - 132, Update the polling flow in connectComposio to use a generation guard: capture the current pollGeneration when starting an iteration, and have poll return immediately when that captured generation no longer matches. Increment pollGeneration during the company-change cleanup in the useEffect so in-flight requests cannot update state, show toasts, or schedule timers for the previous company.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@frontend/src/lib/connections.ts`:
- Line 70: Update the shared toolkit normalization used by toolkit_slug so the
reconciliation alias "x" resolves to the canonical "twitter" slug, ensuring
manifest providers with "x" match Composio Twitter state; alternatively, replace
the toolkit value with "twitter" consistently while preserving all other
canonical toolkit slugs.
In `@frontend/src/views/ConnectionsView.tsx`:
- Around line 182-184: Handle the null result from window.open in the
authorization flow around startComposioAuthorize: when the popup is blocked,
show a toast instructing the operator to allow popups and return before
displaying the sign-in message or starting the 120-second polling/loading flow;
preserve the existing behavior when a window handle is returned.
- Line 346: Bound the status-loading flow around getComposioStatus so a
non-settling request cannot keep reachSettled false indefinitely. Add an abort
signal or timeout to the underlying OpenCompanyClient/BrowserTransport request,
and ensure timeout handling settles the loading state while preserving normal
successful responses.
---
Nitpick comments:
In `@frontend/src/views/ConnectionsView.tsx`:
- Around line 126-132: Update the polling flow in connectComposio to use a
generation guard: capture the current pollGeneration when starting an iteration,
and have poll return immediately when that captured generation no longer
matches. Increment pollGeneration during the company-change cleanup in the
useEffect so in-flight requests cannot update state, show toasts, or schedule
timers for the previous company.
In `@frontend/test/unit/connection-route.test.ts`:
- Around line 49-73: Add a test in the connectionStateFor suite with both a
direct manifest-id state and a Composio-keyed state for the same tile, using
distinct objects. Assert that connectionStateFor returns the direct
states[tile.id] entry, preserving direct-id precedence over the normalized scan.
🪄 Autofix
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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 9459cad6-1460-43ee-a0f9-6e3218e85b4a
📒 Files selected for processing (4)
frontend/src/lib/connections.tsfrontend/src/views/ConnectionsView.tsxfrontend/test/e2e/oauth-onboarding-resume.spec.tsfrontend/test/unit/connection-route.test.ts
oxoxDev
left a comment
oxoxDev
left a comment
There was a problem hiding this comment.
1 major, 1 minor. The major is sequencing with #637. Checks are green now — the five failures from earlier have cleared.
The RCA goes one level deeper than the issue and lands in the right place. platformManaged was Object.values(states).some(s => s.credentialSource === "attested"), and .some() over an empty array is false — so on a provisioned tenant that declares no manifest.connections, states is {}, the #319 guard never fires, and every tile falls through to a 400. "A guard keyed off per-provider rows protecting a grid that renders independently of the manifest" is the actual defect, and it explains all three symptoms rather than just the reported one.
Stating toolkit per tile rather than deriving it is the right call. The tempting fix is a console-side normalize() mirroring the backend's toolkit_slug(), and that would have been a second implementation of the same rule — the drift trap this codebase avoids elsewhere (grants_cover_server moved rather than reimplemented in #631, one validator for default servers in #554). Eleven explicit values are auditable in a way an algorithm copy is not.
Major — collides with #637 on frontend/src/views/ConnectionsView.tsx
#637 (feat(connections): broker providers through the company's own TinyHumans credential, also yours) edits the same view. It changes whether a tenant has a credential to connect with; this changes how a tile decides which route to take. Those two decisions meet in the same component, and both PRs are rewriting how a tile resolves its route — so this is a semantic overlap, not just a textual one: after both land, a tile's route depends on the manifest, the Composio reconciliation, and now the company credential tier.
Neither PR mentions the other. Worth agreeing an order, and whichever lands second should say what a tile does when the company credential is present but the provider is not in states — that combination does not exist in either PR alone.
Minor — nothing couples the toolkit table to the backend's toolkit_slug()
The doc says the field "mirrors the backend's toolkit_slug()" (src/server/ops/connections_read.rs:239). Explicit is right, but a mirror with no reflection test drifts silently: change toolkit_slug and eleven console tiles quietly authorize against slugs the backend no longer produces, with the failure landing as "provider not enabled" — the exact symptom this PR fixes.
A single test feeding all eleven console ids through the backend's normalizer and asserting equality with the table would pin it, and would fail loudly on the day someone edits either side.
|
Pushed Major — sequencing with #637Agreed the overlap is semantic, not just textual, and you're right that neither PR mentioned the other. Proposed order: #637 first, this second. #637 changes what a tenant has; this changes how a tile spends it. Landing the capability before the router means the router is written against the full tier set rather than being amended into it. It is also the cheaper rebase — #637 touches What a tile does when the company credential is set but the provider is not in
This is by construction rather than by luck, and I have made it explicit rather than leaving it as a happy accident. Pinned in it("routes on whether a Composio credential exists, not on which tier it is", …)It asserts both halves — a One consequence worth naming for whoever rebases: the interaction is confined to Minor — nothing coupled the table to
|
CI: the one red check is main's, not this branch's
Evidence it is not this branch:
What the failure actually is, for whoever owns it: the test asserts that under I have not touched it: it is outside #599 and belongs with the gate/approvals work rather than being smuggled into a connections PR. Happy to pick it up separately if you want it — say the word and I will file or fix it. Every other check is green, including both |
…umansai#599) Every Connect button in the categorised provider grid failed on a hosted tenant with "provider '<id>' is not enabled on this host". The tinyhumansai#319 guard that exists to prevent exactly this 400 was present and correct, but keyed off per-provider rows: `platformManaged` reads `states.some(s => s.credentialSource === "attested")`, and `GET …/connections` only answers for providers the manifest declares. A provisioned tenant declares none, so `states` was `{}`, `.some()` over it was false, no tile inherited `attested`, and all eleven fell through to a Connect that could only ever 400. `connectRoute` is now the one place that decides, and it can answer "neither", so a button that cannot work is never rendered: - `static` keeps the native hatch — a registered provider application is a deliberate act by a self-hoster and must not be taken away. - else Composio, when it can authorize that toolkit. This is the only route on a tenant, and the one that makes a connection a capability the agents actually receive. - else `managed` when a platform identity runs connections here. - else `unavailable`, which is the state that was missing. Also fixes two defects falling out of the same keying mismatch: - Tiles now carry their Composio toolkit slug and match host rows through it, so a connected `googlecalendar` / `googledrive` / `twitter` lights up its `google-calendar` / `google-drive` / `x` tile instead of still saying Connect. The eight ids with no `well_known()` key become connectable at all. - Disconnect is offered only when there is a native credential to revoke. There is no Composio disconnect route on the host, so surfacing it on a Composio-only connection would blank a secret that was never set, report success and change nothing. An absent `via` still gets the button — that is an older host, not a Composio-owned connection. The e2e assertion that a Connect button is enabled was asserting the bug: the harness company declares no connections, the binary carries no `composio` feature and host.sh passes no OPENCOMPANY_OAUTH_*, so nothing could complete. It now asserts the tile says so. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…le (tinyhumansai#599) Review follow-ups on the categorised-tile routing. **A connected row now wins over a disconnected one for the same tile.** The host can emit two rows for one provider when its id and Composio slug do not normalize together: `toolkit_slug("x")` is `x`, not `twitter`, so a manifest declaring `provider = "x"` yields a disconnected `x` row while Composio's connected `twitter` state arrives as a separate appended row. Taking the first match reported that tile disconnected while the account was connected — the "two surfaces disagreeing" failure tinyhumansai#316 set out to end. This extends the union the host already does within a row (`native || composio_connected`) across the alias it cannot see. Direct-id precedence still decides when nothing is connected. **A blocked popup is reported.** `window.open` returns null, and the operator was told to finish in a tab that never opened while the tile spun for the full two minutes. **The Composio status probe is bounded.** The shared client has no abort or timeout and the grid now waits on this call before painting, so a host that accepts the connection and never answers held the page on skeletons forever. Losing the race means "no Composio route we can confirm", which is what a null `reach` already means. 5s, mirroring the host's own COMPOSIO_PROBE_TIMEOUT. **The console slug table is pinned to the backend normalizer.** The table is explicit rather than derived (`x` → `twitter` is not derivable), and its doc called it a mirror of `toolkit_slug` with nothing enforcing that. A new test reads the real catalog and feeds it through the real normalizer, asserting each slug is a fixed point — verified to fail on drift, and counting the entries so a renamed field cannot make it pass vacuously. Also documents how this composes with tinyhumansai#586's `company` credential tier: `connectRoute` names only `static` and `attested`, the two tiers that describe the LOCAL host, and reads every other tier through `hasCredential`. So "the company credential is set but the provider is not in states" needs no case of its own — it lands on a working Composio Connect. Pinned by test. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2d6bb3e to
cf5e49a
Compare
…nyhumansai#599) `window.open` returns null whenever `noopener` or `noreferrer` is set — on a successful open exactly as on a blocked one. The null check added last round as "handle a blocked popup" therefore fired every time: manual testing against a live Composio backend opened the tab and told the operator it had not, with an error toast on a sign-in that was in fact underway. Detecting a blocked popup would mean dropping `noopener`, which is what keeps a third-party page we hand an OAuth URL from reaching back through `window.opener`. That is the wrong trade for a nicer error message, so the check goes and the reason it cannot come back is recorded at the call site. `ComposioSection.signIn` opens the same URL the same way and likewise does not check — the two now agree. Caught only by driving a real browser; neither the unit suite nor any CI lane builds the `composio` feature, so nothing automated exercises this path. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Manual test reportRebased onto It found a bug in the last round's review fix. Details at the end. RigTwo hosts, both on the harness company:
Host B's manifest is deliberate: it reproduces the two-row collision. { "provider": "x", "connected": false, "via": [] }
{ "provider": "twitter", "connected": true, "via": ["composio"] }
Scenario A — a host with no routeThe issue's evidence table, reproduced verbatim: all eleven
Scenario B — Composio live
The Per-tile, old route vs the one the console now calls: The alias fix, isolatedSame host, console swapped between the two commits of this PR:
Only Clicking itThe blocked-popup fix was wrong, and this is how it surfacedPushed
Detecting a genuinely blocked popup would mean dropping I took that review suggestion without testing it, which is what put it in. Flagging it plainly rather than quietly reverting. CIAll nine green on the rebase ( Local on the rebase: Still open for you#637 has not landed, so this would now merge first — the opposite of the order I proposed. The composition still holds either way ( |
2 participants
Summary
Closes #599.
Every Connect button in the categorised provider grid failed on a hosted tenant with
provider '<id>' is not enabled on this host. This routes the tiles through Composio — the surface directly above them that already works — while keeping the native path as the self-hosted route.Console-only, as the issue predicted: no manager or backend work, so it does not block on #319.
RCA
The issue's diagnosis is right but stops one level short. The #319 guard that exists to prevent exactly this 400 is present and correct — it just never fires.
CONNECTION_PROVIDERSand looks each up asstates[p.id].statescomes fromGET …/connections→project_connections(src/server/ops/connections_read.rs:352), which emits one row perrecord.manifest.connectionsentry, plus extras only for providers Composio reports already connected. A provisioned tenant declares none, sostatesis{}.platformManagedwasObject.values(states).some(s => s.credentialSource === "attested")..some()over an empty array isfalse, so no tile inheritedattested, andnoRouteneeds"none"wheresourcewasundefined.provider_config()finds noOPENCOMPANY_OAUTH_*→ 400.The root cause is a keying mismatch: a guard keyed off per-provider rows protecting a grid that renders independently of the manifest.
Two further defects fall out of the same mismatch:
googlecalendar,googledrive,twitter); tiles by console id (google-calendar,google-drive,x).toolkit_slug()normalizes backend-side but the console did a rawstates[p.id]lookup — so a genuinely connected Google Calendar never lit up its tile. This is why only some tiles showed "Connected ✓ via composio".well_known()key, so they could not connect even self-hosted with credentials registered.API Or Behavior Changes
No host changes. Console behaviour:
connectRouteis the single decision, used for both what renders and what the click calls, so the button shown and the call made cannot disagree. Precedence:static→ native (a registered provider application is a deliberate act by a self-hoster and must not be taken away); else Composio when it can authorize that toolkit; elsemanaged; elseunavailable.unavailableis the state that was missing — this is what stops the buttons lying.googlecalendar/googledrive/twitterlights up the right tile. All eleven become connectable rather than three./composioexposes status, token, authorize, connections and nothing else), so surfacing it on a Composio-only connection would blank a secret that was never set, report success and change nothing. An absentviastill gets the button — that is an older host, not a Composio-owned connection.Two things worth reviewer attention
An existing e2e assertion was weakened, deliberately.
oauth-onboarding-resume.spec.tsasserted a Connect button was enabled. On that harness — no[[connection]]entries, nocomposiofeature,host.shrunsenv -iwith noOPENCOMPANY_OAUTH_*— that button could never have completed a handshake. It was asserting the bug. It now asserts the tile explains itself; #300's actual property (bounce-back lands in a working console, param stripped) is untouched and still asserted.A regression this fix would otherwise have introduced. Fixing slug matching means more tiles correctly show as Composio-connected — which would have surfaced a Disconnect button that silently does nothing. Hence the
via-gated Disconnect above.Tests
npm run typechecknpm run typecheck:e2enpm run typecheck:unitnpm test— 291 passed (16 new intest/unit/connection-route.test.ts)npm run buildnpm run e2e— 96 passed, 8 skipped against a live hostThe 8 skips are pre-existing: the
LIVE_BRAIN-gated specs (#467) and the Brain spec parked by #302.New unit coverage pins the route matrix directly, including the two regression guards: every tile routes to Composio on a tenant that declares no connections, and every tile reports
unavailablewhen no route can succeed.test/unit/connection-route.test.tsalso asserts theunavailablecase empirically end-to-end — the e2e run confirms that on a host with no route the tile now says so rather than offering a Connect that 400s.Documentation
Module docs in
frontend/src/lib/connections.tsdescribe the two routes and whyconnectRoutecan answer "neither". Theiddoc no longer says an id outsidewell_known()is a dead tile — that is now the Composio path's job — and the twoDEAD TILEmarkers are removed as resolved.Not in scope
via-gated here; worth its own issue.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes