Offer top built-in integrations in onboarding step 3 - #1347
Merged
Merged
Conversation
The onboarding payload now carries builtInProviders: the top three
enabled platform OAuth apps ordered by user connection count (fails
open to an empty list). Step 3 of the wizard renders them above the
starter package grid as one-click connect pills pointing at
/connect/oauth?provider={slug}, with a bring-your-own-OAuth-app line
for users who want their own scopes and rate limits. The block hides
entirely when no platform apps are enabled.
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughOnboarding now loads up to three enabled platform OAuth providers, includes their metadata in loader and API data for verified users, and displays one-click integration links with optional logos and a BYO OAuth fallback. ChangesBuilt-in onboarding providers
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant OnboardingRoute
participant OnboardingHandler
participant PlatformApps
participant D1Database
OnboardingRoute->>OnboardingHandler: request onboarding data
OnboardingHandler->>PlatformApps: listTopPlatformAppsByUse(limit: 3)
PlatformApps->>D1Database: query enabled apps and user connection counts
D1Database-->>PlatformApps: ranked platform app rows
PlatformApps-->>OnboardingHandler: built-in provider metadata
OnboardingHandler-->>OnboardingRoute: loader/API data with builtInProviders
OnboardingRoute->>OnboardingRoute: render OAuth provider links
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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 |
Contributor
|
🔎 Preview deployed: https://kody-pr-1347.kody-a99.workers.dev Worker: Mocks:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/worker/src/integrations/platform-apps.node.test.ts (1)
262-293: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for equal connection counts.
The enabled fixtures have distinct connection counts. Add two enabled apps with equal counts and different
created_atvalues. Assert thatcreated_at ASCdetermines their order.🤖 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 `@packages/worker/src/integrations/platform-apps.node.test.ts` around lines 262 - 293, Add coverage in the listTopPlatformAppsByUse test by adding two enabled platform apps with equal connection counts but distinct created_at values. Include connections for both and assert the app with the earlier created_at appears first, verifying created_at ASC tie-breaking while preserving the existing ordering and disabled-app assertions.
🤖 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 `@packages/worker/client/routes/onboarding.tsx`:
- Around line 805-847: Reconcile the always-visible renderByokDetails disclosure
with the built-in OAuth callout: update renderByokDetails so it no longer claims
there is no one-click connection when builtInProviders is non-empty, or
conditionally hide it in that case. Preserve the existing BYO OAuth guidance
when no built-in providers are available.
In `@packages/worker/src/integrations/platform-apps.ts`:
- Around line 96-114: Update listTopPlatformAppsByUse to remove the global
user_integrations count from ordering, and return platforms in a deterministic
configuration order such as created_at ASC, slug ASC. Do not rank providers
using connector records from any user; preserve the enabled-platform filter and
result mapping.
---
Nitpick comments:
In `@packages/worker/src/integrations/platform-apps.node.test.ts`:
- Around line 262-293: Add coverage in the listTopPlatformAppsByUse test by
adding two enabled platform apps with equal connection counts but distinct
created_at values. Include connections for both and assert the app with the
earlier created_at appears first, verifying created_at ASC tie-breaking while
preserving the existing ordering and disabled-app assertions.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 08ebe008-ffaa-4996-98f8-b0c25f6f10a0
📒 Files selected for processing (10)
packages/worker/client/routes/onboarding-payload.tspackages/worker/client/routes/onboarding.tsxpackages/worker/src/app/handlers/onboarding.node.test.tspackages/worker/src/app/handlers/onboarding.tspackages/worker/src/app/onboarding-data.node.test.tspackages/worker/src/app/onboarding-data.tspackages/worker/src/app/ssr-render.node.test.tspackages/worker/src/integrations/platform-apps.node.test.tspackages/worker/src/integrations/platform-apps.tspackages/worker/universal/loader-data.ts
Comment on lines
+96
to
+114
| export async function listTopPlatformAppsByUse(input: { | ||
| db: D1Database | ||
| limit: number | ||
| }): Promise<Array<PlatformOauthApp>> { | ||
| const result = await input.db | ||
| .prepare( | ||
| `SELECT ${platformAppSelectColumns}, | ||
| ( | ||
| SELECT count(*) FROM user_integrations | ||
| WHERE user_integrations.platform_app_slug = platform_oauth_apps.slug | ||
| ) AS connection_count | ||
| FROM platform_oauth_apps | ||
| WHERE enabled = 1 | ||
| ORDER BY connection_count DESC, created_at ASC, slug ASC | ||
| LIMIT ?`, | ||
| ) | ||
| .bind(input.limit) | ||
| .all<PlatformOauthAppRow>() | ||
| return (result.results ?? []).map(mapPlatformOauthAppRow) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Do not rank providers from other users' connector records.
Lines 103-106 count user_integrations across all users. Line 109 returns that global ranking. The onboarding handler sends this ordering to anonymous and signed-in users. Each user's provider list therefore depends on other users' remote connectors.
Use a deterministic platform configuration order, or a ranking derived only from the current user's data. As per coding guidelines, “each signed-in user must have an independent assistant with separate … remote connectors … and durable storage.”
🤖 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 `@packages/worker/src/integrations/platform-apps.ts` around lines 96 - 114,
Update listTopPlatformAppsByUse to remove the global user_integrations count
from ordering, and return platforms in a deterministic configuration order such
as created_at ASC, slug ASC. Do not rank providers using connector records from
any user; preserve the enabled-platform filter and result mapping.
Source: Coding guidelines
- the bring-your-own-keys disclosure no longer claims there is no one-click connect when built-in providers exist: the heading becomes 'Why bring your own keys?' and the intro explains built-ins run on a Kody-hosted app while BYO remains the full-control lane - formatter fixes on the two new blocks (the earlier local validate ran before the final JSX tweak, so CI caught them) Skipped the 'do not rank providers from other users' connector records' finding: the ordering is an aggregate count over operator-owned platform apps (no per-user data or counts are exposed), the same class of aggregate as community star counts, and ranking by use is the explicit product requirement. Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
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
3 participants
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.
What
Onboarding step 3 ("Install a starter package") now opens with a Use a built-in integration callout when the deployment has enabled platform OAuth apps: the top three by user connection count render as one-click connect pills (logo + label) linking to
/connect/oauth?provider={slug}, followed by a "Bring your own OAuth app for more power" line for users who want their own scopes and rate limits. The block hides entirely when no platform apps are enabled.How
listTopPlatformAppsByUseinplatform-apps.ts: enabled apps ordered byuser_integrationsconnection count (ties by creation order), covered by a unit test that also checks disabled apps are excluded.builtInProviderson the onboarding payload (OnboardingBuiltInProvider: slug, label, logoPath), loaded fail-open in the onboarding handler so a D1 blip or an empty deployment never breaks the page. Anonymous visitors get the list too; unverified logged-in users get the same empty gating asfeaturedListings.data-testid="onboarding-built-in-integrations").Testing
npm run validategreen (new ordering test, updated payload fixtures).Summary by CodeRabbit
New Features
Bug Fixes