Skip to content

Conversation

@NathanFlurry
Copy link
Member

No description provided.

@vercel
Copy link

vercel bot commented Oct 17, 2025

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

Project Deployment Preview Comments Updated (UTC)
rivet-cloud (staging) Ready Ready Preview Comment Oct 20, 2025 3:06pm
rivet-inspector Ready Ready Preview Comment Oct 20, 2025 3:06pm
2 Skipped Deployments
Project Deployment Preview Comments Updated (UTC)
rivet-cloud Ignored Ignored Preview Oct 20, 2025 3:06pm
rivet-site Ignored Ignored Preview Oct 20, 2025 3:06pm

💡 Enable Vercel Agent with $100 free credit for automated AI reviews

@graphite-app
Copy link
Contributor

graphite-app bot commented Oct 17, 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.

@NathanFlurry NathanFlurry changed the title Cloud hotfix chore(frontend): cloud hotfix Oct 18, 2025
Copy link
Contributor

jog1t commented Oct 20, 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.

@pkg-pr-new
Copy link

pkg-pr-new bot commented Oct 20, 2025

Open in StackBlitz

npm i https://pkg.pr.new/rivet-dev/engine/@rivetkit/engine-runner@3222
npm i https://pkg.pr.new/rivet-dev/engine/@rivetkit/engine-runner-protocol@3222

commit: 725df55

@claude
Copy link

claude bot commented Oct 20, 2025

Code Review - PR #3222: chore(frontend): cloud hotfix

Summary

This PR refactors the Vercel and Railway onboarding flows in the frontend, streamlining the user experience and improving configuration management. Overall, the changes are well-structured and improve the codebase. Below are my findings across several areas.


✅ Positive Changes

1. Improved Query Key Specificity

The changes to engine-data-provider.tsx improve query cache management:

  • Before: Query keys included entire opts object
  • After: Query keys only include specific fields (variant, name)
// frontend/src/app/data-providers/engine-data-provider.tsx:581-586
queryKey: [
  { namespace },
  "runners",
  "configs",
  { variant: opts?.variant },  // More specific
],

Impact: This prevents unnecessary cache invalidation when unrelated properties change, improving performance.

2. Enhanced Connection Health Check

The debouncing improvement in connect-manual-serverless-form.tsx:443-444 is excellent:

const [debouncedEndpoint] = useDebounceValue(endpoint, 300);
const [debouncedHeaders] = useDebounceValue(headers, 300);

Impact: Prevents excessive API calls when users are typing or modifying headers.

3. New Reusable Component

The ExternalLinkCard component (frontend/src/components/external-link-card.tsx) is a nice abstraction:

  • Consistent UI for external links
  • Proper accessibility with rel="noreferrer"
  • Good separation of concerns

4. Improved Default Values

Serverless configuration defaults are more sensible:

// connect-manual-serverless-frame.tsx:163-164
slotsPerRunner: 1,      // Was 25
maxRunners: 10000,      // Was 1000

Rationale: Better suited for serverless auto-scaling patterns.


⚠️ Issues & Concerns

1. Critical: Missing Null/Undefined Checks

Location: frontend/src/app/dialogs/connect-vercel-frame.tsx:148

The usePublishableToken(), useEndpoint(), and useNamespace() functions can potentially throw errors, but there's no error handling in the component:

const token = usePublishableToken();
const endpoint = useEndpoint();
const namespace = useNamespace();

Problem: If __APP_TYPE__ is neither "cloud" nor "engine", these throw errors that won't be caught gracefully.

Recommendation: Add error boundaries or handle the error case explicitly:

try {
  const token = usePublishableToken();
  // ...
} catch (error) {
  return <ErrorFallback error={error} />;
}

2. Commented Code Blocks

Location: frontend/src/app/dialogs/connect-vercel-frame.tsx:203-225

There's a large block of commented-out code for StepInitialInfo():

// function StepInitialInfo() {
//   return (
//     <>
//       <ConnectVercelForm.Plan />
// ...

Recommendation:

  • If this code is no longer needed, remove it entirely
  • If it might be reused, consider git history or add a TODO comment explaining why it's kept

3. Environment Variable Naming Inconsistency

Location: frontend/src/app/dialogs/connect-railway-frame.tsx:262,282,304,322

Changed from RIVET_* to NEXT_PUBLIC_RIVET_*:

// Before: RIVET_RUNNER
// After: NEXT_PUBLIC_RIVET_RUNNER

Concern:

  • This is a breaking change for existing users
  • No migration guide or deprecation notice visible
  • Environment variables should match the actual template being deployed

Recommendation:

  • Ensure the Railway and Vercel templates use these exact variable names
  • Consider adding a migration note in the PR description

4. Magic Numbers Without Context

Location: Multiple files

requestLifespan: VERCEL_SERVERLESS_MAX_DURATION - 5,

Issue: The -5 (5 seconds) is a magic number without explanation in the code comment.

Recommendation: Add inline comment:

// Subtract 5s buffer to ensure we don't hit Vercel's timeout
requestLifespan: VERCEL_SERVERLESS_MAX_DURATION - 5,

5. Potential Race Condition

Location: frontend/src/app/forms/connect-manual-serverless-form.tsx:463-465

useEffect(() => {
  onChange(isSuccess);
}, [isSuccess]);

Issue: Missing onChange in the dependency array could cause stale closures.

Recommendation:

useEffect(() => {
  onChange(isSuccess);
}, [isSuccess, onChange]);

Or use useCallback for onChange if it's causing re-renders.


🔒 Security Review

✅ Good Practices:

  1. Proper external link handling: Uses rel="noreferrer" consistently
  2. Token handling: Tokens are fetched via secure hooks, not hardcoded
  3. No exposed secrets: Environment variables properly abstracted

⚠️ Minor Concerns:

  1. Token visibility: In connect-railway-frame.tsx:290, tokens are now shown by default (show prop):
<DiscreteInput
  aria-label="environment variable value"
  value={(data as string) || ""}
  show  // Token is visible
/>

Recommendation: Consider making this opt-in via a "show" button to reduce shoulder-surfing risk.


🚀 Performance Considerations

✅ Improvements:

  1. Query key optimization reduces unnecessary re-fetches
  2. Debouncing prevents API spam
  3. Conditional query enabling with enabled flag

💡 Suggestions:

  1. Prefetching: The code uses usePrefetchInfiniteQuery well in connect-quick-railway-frame.tsx:52-55
  2. Consider memoization: Heavy computations in render functions could benefit from useMemo

🧪 Test Coverage

Observation: No test files were modified or added in this PR.

Recommendation: Consider adding tests for:

  1. The new ExternalLinkCard component
  2. Query key generation logic in engine-data-provider.tsx
  3. Error handling in usePublishableToken(), useEndpoint(), useNamespace()
  4. The debounced connection check behavior

📝 Code Quality

✅ Good Practices:

  • Consistent TypeScript usage
  • Good component composition
  • Clear naming conventions
  • Proper separation of concerns

💡 Minor Issues:

  1. Unused imports: Verify all imports in connect.tsx are used
  2. Type safety: Consider using as const for readonly arrays/objects where applicable
  3. Documentation: The new VERCEL_SERVERLESS_MAX_DURATION constant could benefit from a JSDoc comment explaining its purpose

🎯 Overall Assessment

Rating: ⭐⭐⭐⭐ (4/5)

This is a solid refactoring that improves the user onboarding experience. The main concerns are:

  1. Missing error handling for edge cases
  2. Breaking changes to environment variable names
  3. Commented code that should be removed
  4. Need for test coverage

Recommendation: Approve with minor changes requested

Action Items:

  1. ✅ Add error handling for usePublishableToken() and related hooks
  2. ✅ Remove or document commented code blocks
  3. ✅ Add migration notes for environment variable name changes
  4. ✅ Fix useEffect dependency array
  5. 💡 Consider adding tests (nice-to-have)
  6. 💡 Add JSDoc comments for new constants (nice-to-have)

Great work on improving the onboarding flow! 🚀

@graphite-app
Copy link
Contributor

graphite-app bot commented Oct 21, 2025

Merge activity

  • Oct 21, 6:48 PM UTC: jog1t added this pull request to the Graphite merge queue.
  • Oct 21, 6:49 PM UTC: CI is running for this pull request on a draft pull request (#3235) due to your merge queue CI optimization settings.
  • Oct 21, 6:49 PM UTC: Merged by the Graphite merge queue via draft PR: #3235.

graphite-app bot pushed a commit that referenced this pull request Oct 21, 2025
@graphite-app graphite-app bot closed this Oct 21, 2025
@graphite-app graphite-app bot deleted the cloud-hotfix branch October 21, 2025 18:49
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