Skip to content

Conversation

@jog1t
Copy link
Contributor

@jog1t jog1t commented Oct 28, 2025

No description provided.

@vercel
Copy link

vercel bot commented Oct 28, 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 28, 2025 5:55pm
rivet-inspector Ready Ready Preview Comment Oct 28, 2025 5:55pm
2 Skipped Deployments
Project Deployment Preview Comments Updated (UTC)
rivet-cloud Ignored Ignored Oct 28, 2025 5:55pm
rivet-site Ignored Ignored Preview Oct 28, 2025 5:55pm

@jog1t jog1t marked this pull request as ready for review October 28, 2025 17:50
Copy link
Contributor Author

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

@claude
Copy link

claude bot commented Oct 28, 2025

Code Review: Vercel Template Link Default Values

Summary

This PR enhances the Vercel deployment flow by dynamically generating the Vercel deployment link with pre-filled environment variables, improving the user experience by reducing manual configuration steps.

Positive Aspects ✓

  • Good UX improvement: Pre-filling environment variables will streamline the Vercel deployment process
  • Proper encapsulation: The logic is well-contained in a custom hook (useVercelTemplateLink)
  • Reusability: Exporting useSelectedDatacenter from the Railway frame promotes code reuse
  • URL encoding: Proper use of encodeURIComponent for URL safety

Issues & Concerns

1. Critical: Missing null/undefined checks 🔴

The hook doesn't handle cases where token or endpoint might be undefined:

const { data: token } = useQuery(
    dataProvider.engineAdminTokenQueryOptions(),
);
const endpoint = useSelectedDatacenter();

If the query is still loading or fails, token will be undefined, which will result in:

  • NEXT_PUBLIC_RIVET_TOKEN: undefined in the URL
  • JSON.stringify will convert undefined to null or omit it entirely

Recommendation: Add loading/error states or use optional chaining:

const { data: token, isLoading } = useQuery(
    dataProvider.engineAdminTokenQueryOptions(),
);
const endpoint = useSelectedDatacenter();

if (\!token || \!endpoint || isLoading) {
    return null; // or a fallback URL
}

2. Code formatting: Inconsistent indentation ⚠️

Lines 141-143 have inconsistent indentation:

const { data: token } = useQuery(
        dataProvider.engineAdminTokenQueryOptions(),
    );

Should be:

const { data: token } = useQuery(
    dataProvider.engineAdminTokenQueryOptions(),
);

3. Code formatting: Extra blank lines ⚠️

Line 145-146 has unnecessary blank lines before the useMemo call.

4. Import organization 📝

The useMemo import (line 24) is added at the end of the import list. Following best practices, React imports should be organized together at the top.

Suggestion:

import { useMemo } from "react";
import { faVercel, Icon } from "@rivet-gg/icons";
// ... other imports

5. Missing dependency in useMemo ⚠️

The useMemo dependency array might be incomplete. If dataProvider changes (which is unlikely but possible), the memoization won't update. Consider whether dataProvider should be in the dependency array.

6. Potential performance issue 💡

The hook always calls useQuery and useSelectedDatacenter even when the component might not render the link immediately (e.g., on different steps). Consider if this should be lazy-loaded or if the query should be disabled conditionally.

7. No test coverage ⚠️

There are no tests for this new functionality. Consider adding unit tests for:

  • URL generation with valid inputs
  • Handling of undefined/null values
  • Proper URL encoding

Security Considerations

  • Token exposure: The admin token is being passed in a URL. While this is part of Vercel's deployment flow, ensure that:
    • The token has appropriate scopes/permissions
    • Users are aware that the token will be visible in browser history and Vercel's deployment logs
    • Consider adding a note in the UI about token visibility

Recommendations

High Priority:

  1. Add null/undefined checks for token and endpoint
  2. Fix indentation issues (lines 141-143, 145-146)

Medium Priority:
3. Reorganize imports to group React imports together
4. Review useMemo dependency array completeness
5. Add test coverage

Low Priority:
6. Consider adding a UI indicator when token/endpoint are loading
7. Consider adding analytics/telemetry for when users click the Vercel deployment link

Testing Suggestions

To verify this works correctly:

  1. Test with a fresh session where the token query hasn't loaded yet
  2. Test when the token query fails
  3. Test with different datacenter selections
  4. Verify the generated URL works correctly in Vercel's deployment flow

Overall, this is a useful feature with good structure, but needs handling for edge cases around data availability.

@pkg-pr-new
Copy link

pkg-pr-new bot commented Oct 28, 2025

More templates

@rivetkit/actor

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

@rivetkit/cloudflare-workers

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

@rivetkit/core

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

@rivetkit/db

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

@rivetkit/framework-base

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

@rivetkit/next-js

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

@rivetkit/react

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

rivetkit

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

@rivetkit/sql-loader

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

@rivetkit/engine-runner

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

@rivetkit/engine-runner-protocol

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

commit: 5fc26da

@graphite-app
Copy link
Contributor

graphite-app bot commented Oct 28, 2025

Graphite Automations

"Test" took an action on this PR • (10/28/25)

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

@jog1t jog1t merged commit 2b1b80f into main Oct 28, 2025
10 checks passed
@jog1t jog1t deleted the 10-28-feat_fe_provide_default_values_for_deploy_to_vercel_link branch October 28, 2025 17:57
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