-
Notifications
You must be signed in to change notification settings - Fork 969
rename SERVER_BASE_URL to NEXT_PUBLIC_SERVER_BASE_URL as previous one… #299
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: main
Are you sure you want to change the base?
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -518,7 +518,7 @@ Remember: | |
|
||
try { | ||
// Create WebSocket URL from the server base URL | ||
const serverBaseUrl = process.env.SERVER_BASE_URL || 'http://localhost:8001'; | ||
const serverBaseUrl = process.env.NEXT_PUBLIC_SERVER_BASE_URL || 'http://localhost:8001'; | ||
const wsBaseUrl = serverBaseUrl.replace(/^http/, 'ws')? serverBaseUrl.replace(/^https/, 'wss'): serverBaseUrl.replace(/^http/, 'ws'); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've noticed a critical issue in how this The logic on line 522 is flawed and will not work for This buggy logic is also duplicated across multiple files:
A correct implementation to replace the logic on line 522 would be: const wsBaseUrl = serverBaseUrl.replace(/^http/, 'ws'); |
||
const wsUrl = `${wsBaseUrl}/ws/chat`; | ||
|
||
|
@@ -815,7 +815,7 @@ IMPORTANT: | |
|
||
try { | ||
// Create WebSocket URL from the server base URL | ||
const serverBaseUrl = process.env.SERVER_BASE_URL || 'http://localhost:8001'; | ||
const serverBaseUrl = process.env.NEXT_PUBLIC_SERVER_BASE_URL || 'http://localhost:8001'; | ||
const wsBaseUrl = serverBaseUrl.replace(/^http/, 'ws')? serverBaseUrl.replace(/^https/, 'wss'): serverBaseUrl.replace(/^http/, 'ws'); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The logic on this line is flawed and will not work for A correct implementation to replace the logic on this line would be: const wsBaseUrl = serverBaseUrl.replace(/^http/, 'ws'); |
||
const wsUrl = `${wsBaseUrl}/ws/chat`; | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -265,7 +265,7 @@ Give me the numbered list with brief descriptions for each slide. Be creative bu | |
|
||
try { | ||
// Create WebSocket URL from the server base URL | ||
const serverBaseUrl = process.env.SERVER_BASE_URL || 'http://localhost:8001'; | ||
const serverBaseUrl = process.env.NEXT_PUBLIC_SERVER_BASE_URL || 'http://localhost:8001'; | ||
const wsBaseUrl = serverBaseUrl.replace(/^http/, 'ws')? serverBaseUrl.replace(/^https/, 'wss'): serverBaseUrl.replace(/^http/, 'ws'); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The logic on this line is flawed and will not work for A correct implementation to replace the logic on this line would be: const wsBaseUrl = serverBaseUrl.replace(/^http/, 'ws'); |
||
const wsUrl = `${wsBaseUrl}/ws/chat`; | ||
|
||
|
@@ -541,7 +541,7 @@ Please return ONLY the HTML with no markdown formatting or code blocks. Just the | |
|
||
try { | ||
// Create WebSocket URL from the server base URL | ||
const serverBaseUrl = process.env.SERVER_BASE_URL || 'http://localhost:8001'; | ||
const serverBaseUrl = process.env.NEXT_PUBLIC_SERVER_BASE_URL || 'http://localhost:8001'; | ||
const wsBaseUrl = serverBaseUrl.replace(/^http/, 'ws')? serverBaseUrl.replace(/^https/, 'wss'): serverBaseUrl.replace(/^http/, 'ws'); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The logic on this line is flawed and will not work for A correct implementation to replace the logic on this line would be: const wsBaseUrl = serverBaseUrl.replace(/^http/, 'ws'); |
||
const wsUrl = `${wsBaseUrl}/ws/chat`; | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -317,7 +317,7 @@ Make the workshop content in ${language === 'en' ? 'English' : | |
|
||
try { | ||
// Create WebSocket URL from the server base URL | ||
const serverBaseUrl = process.env.SERVER_BASE_URL || 'http://localhost:8001'; | ||
const serverBaseUrl = process.env.NEXT_PUBLIC_SERVER_BASE_URL || 'http://localhost:8001'; | ||
const wsBaseUrl = serverBaseUrl.replace(/^http/, 'ws')? serverBaseUrl.replace(/^https/, 'wss'): serverBaseUrl.replace(/^http/, 'ws'); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The logic on this line is flawed and will not work for A correct implementation to replace the logic on this line would be: const wsBaseUrl = serverBaseUrl.replace(/^http/, 'ws'); |
||
const wsUrl = `${wsBaseUrl}/ws/chat`; | ||
|
||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -2,7 +2,7 @@ import { NextRequest, NextResponse } from 'next/server'; | |||||
|
||||||
// The target backend server base URL, derived from environment variable or defaulted. | ||||||
// This should match the logic in your frontend's page.tsx for consistency. | ||||||
const TARGET_SERVER_BASE_URL = process.env.SERVER_BASE_URL || 'http://localhost:8001'; | ||||||
const TARGET_SERVER_BASE_URL = process.env.NEXT_PUBLIC_SERVER_BASE_URL || 'http://localhost:8001'; | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This API route is server-side code and doesn't strictly need to use a For better separation of concerns and deployment flexibility, it's recommended to use a server-only environment variable for server-to-server communication. This prevents accidentally exposing server-internal configuration to the client and makes the architecture clearer.
Suggested change
|
||||||
|
||||||
// This is a fallback HTTP implementation that will be used if WebSockets are not available | ||||||
// or if there's an error with the WebSocket connection | ||||||
|
@@ -110,4 +110,4 @@ export async function OPTIONS() { | |||||
'Access-Control-Allow-Headers': 'Content-Type, Authorization', // Adjust as per client's request headers | ||||||
}, | ||||||
}); | ||||||
} | ||||||
} |
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.
It's generally better to use a separate, non-public environment variable for server-side configurations like this one. The
NEXT_PUBLIC_
prefix is intended to expose variables to the browser, which isn't necessary for server-side code likenext.config.ts
and API routes.This separation allows for more flexible deployment scenarios. For example, you could use an internal service URL (e.g.,
http://api-service:8001
) on the server while the client uses a public-facing URL.To improve this, you could prioritize a server-specific variable and fall back to the public one. This would make the configuration more robust and align with best practices.