Skip to content

Conversation

@NathanFlurry
Copy link
Member

No description provided.

@vercel vercel bot temporarily deployed to Preview – rivetkit-serverless December 1, 2025 05:47 Inactive
@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 Error Error Dec 2, 2025 10:01am
rivetkit-serverless Error Error Dec 2, 2025 10:01am
2 Skipped Deployments
Project Deployment Preview Comments Updated (UTC)
rivet-cloud Ignored Ignored Dec 2, 2025 10:01am
rivet-site Ignored Ignored Preview Dec 2, 2025 10:01am

Copy link
Member Author

NathanFlurry commented Dec 1, 2025

Warning

This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
Learn more


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 Dec 1, 2025

PR Review: fix(frontend): update env vars for dev

Summary

This PR simplifies frontend environment variable configuration by:

  1. Removing the __AUTO__ API endpoint resolution logic
  2. Removing the unused VITE_APP_CLOUD_ENGINE_URL variable
  3. Adding VITE_APP_CLERK_PUBLISHABLE_KEY to local dev environment
  4. Reordering .env file for better organization

✅ Positive Changes

  1. Simplified Configuration Logic - Removing the __AUTO__ endpoint resolution is a good change. The logic for automatically determining API endpoints based on hostname (hub.api.) with a fallback to staging was complex and error-prone.

  2. Clear Comments - Added helpful inline comments explaining the purpose of each API endpoint (Engine API endpoint vs Cloud API endpoint).

  3. Cleanup - Removed unused VITE_APP_CLOUD_ENGINE_URL variable that was defined in the schema but not present in .env.

  4. Better Dev Experience - Now local development uses explicit localhost URLs which is more predictable.

🔍 Issues & Concerns

1. Code Duplication Between Files (Medium Priority)

There are two nearly identical config.ts files:

  • frontend/packages/components/src/lib/config.ts (lines 27-33)
  • frontend/src/components/lib/config.ts (lines 26-32)

Issue: The only difference is the SSR check (typeof window !== "undefined"). This duplication makes maintenance harder.

Recommendation: Consider consolidating these files or adding a comment explaining why both exist.

2. Missing SSR Safety in One File (Low Priority)

frontend/packages/components/src/lib/config.ts:30 uses location.origin directly without checking if window exists (unlike the other file at line 29).

Current code:

if (apiEndpoint === "__SAME__") {
    return location.origin;  // Could fail in SSR
}

Recommendation: For consistency and SSR safety, use:

if (typeof window !== "undefined" && apiEndpoint === "__SAME__") {
    return window.location.origin;
}

3. Environment Variable Validation (Low Priority)

In frontend/src/lib/env.ts:28, VITE_APP_CLOUD_API_URL is validated as z.string().url(), but it's also used in the .env with localhost (http://localhost:43708/api).

Question: Is the getApiEndpoint() transformation intentionally bypassed for cloud API? The comment says "direct URL without transformation" which suggests this is intentional, but worth confirming this matches expected behavior.

4. Removed Environment Variable Reference (Low Priority)

The removed line from env.ts:

VITE_APP_CLOUD_ENGINE_URL: z.string().url(),

Question: Are there any other parts of the codebase that might still reference this variable? The grep showed no results, so this appears safe.

🧪 Testing Recommendations

  1. Local Development: Verify both engine and cloud frontends work with localhost APIs
  2. Docker Builds: Confirm __SAME__ still works correctly in Docker deployments
  3. SSR Scenarios: Test that the component package doesn't fail during server-side rendering
  4. Production Builds: Ensure staging/production deployments still work with direct URLs

📊 Code Quality Metrics

  • Lines changed: +27/-23 (net +4)
  • Files modified: 5
  • Risk level: Low to Medium
  • Breaking changes: Potentially yes - removal of __AUTO__ logic and VITE_APP_CLOUD_ENGINE_URL

✏️ Minor Suggestions

  1. Consider adding a comment in .env explaining that VITE_APP_CLERK_PUBLISHABLE_KEY is a test key safe for dev
  2. The pnpm lock update shows a dependency version change - ensure this was intentional

🎯 Verdict

Overall: This is a solid cleanup PR that simplifies the configuration. The main concerns are:

  • SSR safety inconsistency between the two config files
  • Code duplication that should be addressed

Recommendation: Approve with minor changes - fix the SSR safety issue in frontend/packages/components/src/lib/config.ts before merging.


Automated review by Claude Code

@NathanFlurry NathanFlurry changed the base branch from main to graphite-base/3557 December 1, 2025 05:57
@NathanFlurry NathanFlurry force-pushed the 11-30-fix_frontend_update_env_vars_for_dev branch from 91c693f to f28b5ac Compare December 1, 2025 05:57
@NathanFlurry NathanFlurry changed the base branch from graphite-base/3557 to 11-24-docs_clean_up_last_few_missing_pieces_of_content December 1, 2025 05:57
@graphite-app graphite-app bot changed the base branch from 11-24-docs_clean_up_last_few_missing_pieces_of_content to graphite-base/3557 December 1, 2025 05:59
@graphite-app graphite-app bot force-pushed the 11-30-fix_frontend_update_env_vars_for_dev branch from f28b5ac to e211bb1 Compare December 1, 2025 06:00
@graphite-app graphite-app bot force-pushed the graphite-base/3557 branch from 799cad2 to def9fd8 Compare December 1, 2025 06:00
@vercel vercel bot temporarily deployed to Preview – rivetkit-serverless December 1, 2025 06:00 Inactive
@graphite-app graphite-app bot changed the base branch from graphite-base/3557 to main December 1, 2025 06:00
@graphite-app graphite-app bot force-pushed the 11-30-fix_frontend_update_env_vars_for_dev branch from e211bb1 to 6134af8 Compare December 1, 2025 06:01
@vercel vercel bot temporarily deployed to Preview – rivetkit-serverless December 1, 2025 06:01 Inactive
@NathanFlurry NathanFlurry force-pushed the 11-30-fix_frontend_update_env_vars_for_dev branch from 6134af8 to bded0cb Compare December 2, 2025 05:18
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