-
Notifications
You must be signed in to change notification settings - Fork 37
09-16-fix_next-js_fix_example_for_next-js #1250
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: graphite-base/1250
Are you sure you want to change the base?
09-16-fix_next-js_fix_example_for_next-js #1250
Conversation
The latest updates on your projects. Learn more about Vercel for GitHub.
|
How to use the Graphite Merge QueueAdd either label to this PR to merge it via 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. |
Claude finished @jog1t's task —— View job Pull Request Review ✅Reviewing PR #1250: Next.js example fixes Todo List:
Analysis OverviewThis PR contains two commits that fix deployment and configuration issues for both Express and Next.js examples:
Detailed ReviewExpress Example Changes (examples/express/)✅ Good Changes:
✅ Import Resolution:
Next.js Example Changes (examples/next-js/src/lib/rivet-client.ts)✅ Excellent Changes:
Code Quality Assessment: const apiOrigin =
process.env.NEXT_PUBLIC_VERCEL_URL ||
process.env.NEXT_PUBLIC_URL ||
typeof window === "undefined"
? "http://localhost:3000"
: window.location.origin; This logic correctly handles:
Framework Base Changes (packages/framework-base/lib/mod.ts)✅ Code Cleanup:
Security Review✅ No Security Concerns:
Performance Considerations✅ Performance Neutral/Positive:
Test Coverage Implications✅ Low Risk:
Compliance with Repository Guidelines✅ Fully Compliant:
Final Assessment✅ APPROVED - Ready to Merge This PR makes necessary and well-implemented fixes for deployment scenarios:
All changes follow established patterns, repository guidelines, and modern JavaScript/TypeScript best practices. No security, performance, or compatibility concerns identified. |
3d9aa34
to
7780ec2
Compare
93458d9
to
10af3aa
Compare
Claude finished @jog1t's task —— View job Reviewing PR #1250: Next.js example fixes ✅ Todo List:
Analysis OverviewThis PR contains two commits that fix deployment and configuration issues for both Express and Next.js examples:
Detailed ReviewExpress Example Changes (examples/express/)✅ Good Changes:
✅ Import Resolution:
Next.js Example Changes (examples/next-js/src/lib/rivet-client.ts)✅ Excellent Changes:
Code Quality Assessment: const apiOrigin =
process.env.NEXT_PUBLIC_VERCEL_URL ||
process.env.NEXT_PUBLIC_URL ||
typeof window === "undefined"
? "http://localhost:3000"
: window.location.origin; This logic correctly handles:
Framework Base Changes (packages/framework-base/lib/mod.ts)✅ Code Cleanup:
Security Review✅ No Security Concerns:
Performance Considerations✅ Performance Neutral/Positive:
Test Coverage Implications✅ Low Risk:
Compliance with Repository Guidelines✅ Fully Compliant:
Final Assessment✅ APPROVED - Ready to Merge This PR makes necessary and well-implemented fixes for deployment scenarios:
All changes follow established patterns, repository guidelines, and modern JavaScript/TypeScript best practices. No security, performance, or compatibility concerns identified. |
Claude finished @jog1t's task —— View job Code Review Todo List:
Pull Request Review ✅Reviewing PR #1250: Next.js example fixes Analysis OverviewThis PR contains two commits that fix deployment and configuration issues for both Express and Next.js examples:
Detailed ReviewExpress Example Changes (examples/express/)✅ Good Changes:
✅ Import Resolution:
Next.js Example Changes (examples/next-js/src/lib/rivet-client.ts)✅ Excellent Changes:
Code Quality Assessment: const apiOrigin =
process.env.NEXT_PUBLIC_VERCEL_URL ||
process.env.NEXT_PUBLIC_URL ||
typeof window === "undefined"
? "http://localhost:3000"
: window.location.origin; This logic correctly handles:
Framework Base Changes (packages/framework-base/lib/mod.ts)✅ Code Cleanup:
Security Review✅ No Security Concerns:
Performance Considerations✅ Performance Neutral/Positive:
Test Coverage Implications✅ Low Risk:
Compliance with Repository Guidelines✅ Fully Compliant:
Final Assessment✅ APPROVED - Ready to Merge This PR makes necessary and well-implemented fixes for deployment scenarios:
All changes follow established patterns, repository guidelines, and modern JavaScript/TypeScript best practices. No security, performance, or compatibility concerns identified. |
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.
How to use the Graphite Merge QueueAdd either label to this PR to merge it via 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. |
@@ -1,5 +1,5 @@ | |||
import express from "express"; | |||
import { registry } from "./registry"; | |||
import { registry } from "./registry.js"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
plz no
|
||
const client = createClient<typeof registry>(`${window.location.origin}/api`, { | ||
const apiOrigin = | ||
process.env.NEXT_PUBLIC_VERCEL_URL || |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
which one is standard?
7780ec2
to
49ed6fb
Compare
Claude finished @jog1t's task —— View job Reviewing Pull Request Code Review Todo List:
Pull Request Review SummaryThis PR contains fixes for deployment configuration of the Express example and updates to engine-runner dependencies. Based on my analysis of the actual git commits, here's my comprehensive review: Changes AnalysisCommit 1 (ce5dc6d): "fix vercel deploy"
Commit 2 (49ed6fb): "fix(next-js): fix example for next-js"
Code Quality Assessment✅ Positive Changes
|
Claude finished @jog1t's task —— View job Pull Request Review ✅Reviewing PR #1250: Next.js example fixes Code Review Todo List:
Analysis OverviewThis PR contains two commits that fix deployment and configuration issues for the Express example and update the engine runner dependency:
Detailed ReviewExpress Example Changes (examples/express/)✅ Good Changes:
Engine Runner Dependency Update (packages/rivetkit/package.json)✅ Dependency Update:
Security Review✅ No Security Concerns:
Performance Considerations✅ Performance Neutral/Positive:
Code Quality Assessment✅ Follows Repository Guidelines:
Test Coverage Implications✅ Low Risk:
Addressing Existing Review CommentsRegarding
Regarding Next.js standards question: Potential Issues Found❌ Missing Next.js Changes:
Final Assessment✅ APPROVED with Minor Suggestions This PR makes necessary deployment fixes for the Express example and updates a core dependency. The changes are well-implemented and follow repository guidelines:
Recommendations:
All changes follow established patterns, repository guidelines, and modern JavaScript/TypeScript best practices. No security, performance, or compatibility concerns identified. |
More templates
@rivetkit/actor
@rivetkit/cloudflare-workers
@rivetkit/core
@rivetkit/db
@rivetkit/framework-base
@rivetkit/next-js
@rivetkit/react
rivetkit
@rivetkit/sql-loader
commit: |
More templates
@rivetkit/actor
@rivetkit/cloudflare-workers
@rivetkit/core
@rivetkit/db
@rivetkit/framework-base
@rivetkit/next-js
@rivetkit/react
rivetkit
@rivetkit/sql-loader
commit: |
No description provided.