Skip to content

Conversation

@jog1t
Copy link
Contributor

@jog1t jog1t commented Dec 1, 2025

No description provided.

@vercel
Copy link

vercel bot commented Dec 1, 2025

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Preview Comments Updated (UTC)
rivet-inspector Ready Ready Preview Comment Dec 2, 2025 9:42pm
rivetkit-serverless Ready Ready Preview Comment Dec 2, 2025 9:42pm
2 Skipped Deployments
Project Deployment Preview Comments Updated (UTC)
rivet-cloud Ignored Ignored Dec 2, 2025 9:42pm
rivet-site Ignored Ignored Preview Dec 2, 2025 9:42pm

@jog1t jog1t marked this pull request as ready for review December 1, 2025 21:08
Copy link
Contributor Author

jog1t commented Dec 1, 2025


How to use the Graphite Merge Queue

Add the label merge-queue to this PR to add it to the merge queue.

You must have a Graphite account in order to use the merge queue. Sign up using this link.

An organization admin has enabled the Graphite Merge Queue in this repository.

Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue.

This stack of pull requests is managed by Graphite. Learn more about stacking.

@graphite-app
Copy link
Contributor

graphite-app bot commented Dec 1, 2025

Graphite Automations

"Test" took an action on this PR • (12/01/25)

1 assignee was added to this PR based on Kacper Wojciechowski's automation.

@pkg-pr-new
Copy link

pkg-pr-new bot commented Dec 1, 2025

More templates

@rivetkit/cloudflare-workers

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/cloudflare-workers@3561

@rivetkit/db

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/db@3561

@rivetkit/framework-base

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/framework-base@3561

@rivetkit/next-js

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/next-js@3561

@rivetkit/react

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/react@3561

rivetkit

pnpm add https://pkg.pr.new/rivet-dev/rivet/rivetkit@3561

@rivetkit/sql-loader

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/sql-loader@3561

@rivetkit/engine-runner

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/engine-runner@3561

@rivetkit/engine-runner-protocol

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/engine-runner-protocol@3561

commit: 25e6ef4

@claude
Copy link

claude bot commented Dec 2, 2025

PR Review: New Onboarding Experience

Overview

This PR introduces a new onboarding experience for Rivet with template-based setup and improved provider selection. The changes include 891 additions and 720 deletions across 15 files.


✅ Strengths

  1. Well-structured component organization - Clear separation between getting started, template selection, and provider configuration
  2. Type safety - Good use of TypeScript with proper type inference for providers
  3. Reusable patterns - The TemplateProviders component is cleanly abstracted and reusable
  4. Consistent modal architecture - Modal dialogs follow existing patterns

🐛 Potential Issues

1. Interface naming mismatch (start-with-template-frame.tsx:13)

interface ConnectAwsFrameContentProps extends DialogContentProps {
    template: string;
}

export default function StartWithTemplateFrame({
    template,
    onClose,
}: ConnectAwsFrameContentProps) {

Issue: The interface is named ConnectAwsFrameContentProps but should be StartWithTemplateFrameProps to match the component name.

Fix:

interface StartWithTemplateFrameProps extends DialogContentProps {
    template: string;
}

2. Inconsistent indentation (start-with-template-frame.tsx:27-33)

Lines 27-33 use spaces for indentation instead of tabs, breaking the project's hard tabs convention (see rustfmt.toml reference in CLAUDE.md).

.with("cloudflare", () => <ConnectManualServerlessFrameContent provider="cloudflare-workers" onClose={onClose} />)
        .with("railway", () => <ConnectQuickRailwayFrameContent onClose={onClose} />)
        .with("kubernetes", () => <ConnectManualServerlfullFrameContent provider="k8s" onClose={onClose} />)

Fix: Use hard tabs consistently throughout the file.

3. Unused template prop (start-with-template-frame.tsx:14,39)

The template prop is passed to the component and displayed in the title, but it's never actually used to filter or configure the template. This suggests incomplete implementation.

Questions:

  • Should different templates show different providers?
  • Should the template be passed to the connect dialogs for pre-configuration?

4. Missing null/undefined check (getting-started.tsx:81)

The template search parameter might be undefined when the modal is opened:

search={{ modal: "start-with-template", template: slug }}

But in start-with-template-frame.tsx:39, it's used without validation:

<div>Start With {template}</div>

Fix: Add a fallback or validate the template prop.

5. Redundant grid column definition (template-providers.tsx:66)

<div className="grid grid-cols-1 gap-6 sm:grid-cols-1">

Both default and sm: breakpoint are set to grid-cols-1, making the sm:grid-cols-1 redundant.


🎨 Code Quality Concerns

1. Magic strings for provider slugs

Provider slugs like "cloudflare-workers", "k8s", "bare-metal" are hardcoded in multiple places. Consider extracting these to constants for consistency:

export const PROVIDER_SLUGS = {
  CLOUDFLARE_WORKERS: 'cloudflare-workers',
  KUBERNETES: 'k8s',
  BARE_METAL: 'bare-metal',
  // ...
} as const;

2. Component file organization

The new components are added to /app but dialog frames are in /app/dialogs. Consider consistency:

  • getting-started.tsx → Should this be in /app/dialogs?
  • template-providers.tsx → Could be in a shared component directory

3. Empty template preview (getting-started.tsx:83)

<div className="border-b min-h-40 w-full"></div>

This is just an empty bordered div. Consider adding a placeholder image or proper template preview.


🔒 Security Review

✅ No security concerns identified:

  • No user input sanitization issues
  • No XSS vulnerabilities
  • External links use proper rel="noopener noreferrer"

⚡ Performance Considerations

1. Large generated route file

The routeTree.gen.ts file changed significantly (470 additions, 471 deletions). This appears to be auto-generated, but ensure:

  • The build process properly regenerates this
  • No manual edits were made to generated code

2. New dependency: unplugin-macros

A new dependency was added to package.json:

"unplugin-macros": "^0.18.3"

Questions:

  • Is this dependency actually used in the codebase?
  • What macros are being utilized?
  • Could this be removed if unused?

🧪 Test Coverage

⚠️ No tests were added for the new onboarding flow. Consider adding:

  • Unit tests for TemplateProviders component
  • Integration tests for the template selection flow
  • E2E tests for the complete onboarding experience

📝 Recommendations

High Priority

  1. ✅ Fix interface naming mismatch (ConnectAwsFrameContentPropsStartWithTemplateFrameProps)
  2. ✅ Fix indentation to use hard tabs consistently
  3. ✅ Add null check for template prop or make it required
  4. ✅ Clarify if unplugin-macros is needed or remove it

Medium Priority

  1. Extract provider slug constants to avoid magic strings
  2. Add template preview content or images
  3. Consider adding tests for the new onboarding flow

Low Priority

  1. Clean up redundant Tailwind classes (sm:grid-cols-1)
  2. Review file organization for consistency

📊 Summary

Overall, this is a solid foundation for the new onboarding experience. The architecture is clean and follows existing patterns. The main concerns are:

  • A few naming/typing inconsistencies
  • Missing template implementation details
  • Lack of test coverage

Recommendation:Approve with minor changes - Fix the high-priority issues, particularly the interface naming and indentation, before merging.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants