-
Notifications
You must be signed in to change notification settings - Fork 963
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?
Conversation
… be blocked by nodejs(without NEXT_PUBLIC* prefix)
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.
Summary of Changes
Hello @zcuder, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request resolves an issue where the SERVER_BASE_URL
environment variable was not properly accessible on the client-side of the Next.js application. By adhering to Next.js conventions and renaming the variable to NEXT_PUBLIC_SERVER_BASE_URL
, the application can now correctly determine and connect to the backend server from both server-side and client-side contexts, ensuring full functionality for features that rely on this configuration.
Highlights
- Environment Variable Renaming: Renamed the
SERVER_BASE_URL
environment variable toNEXT_PUBLIC_SERVER_BASE_URL
to ensure it is correctly exposed and accessible on the client-side within the Next.js application. - Configuration and Documentation Updates: Updated Dockerfiles (
Dockerfile
,Dockerfile-ollama-local
),docker-compose.yml
, and theREADME.md
documentation to reflect the newNEXT_PUBLIC_SERVER_BASE_URL
variable name. - Codebase Adaptations: Modified various Next.js configuration files, frontend page components, and API route handlers to consume the newly named
NEXT_PUBLIC_SERVER_BASE_URL
for backend server communication.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command>
or @gemini-code-assist <command>
. Below is a summary of the supported commands.
Feature | Command | Description |
---|---|---|
Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/
folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments or fill out our survey to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
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.
Code Review
The pull request correctly renames the SERVER_BASE_URL
environment variable to NEXT_PUBLIC_SERVER_BASE_URL
to make it accessible on the client side. The change is applied consistently across all relevant files, including Docker configurations, documentation, and source code.
However, my review identified a critical bug in the client-side WebSocket URL generation logic that this change interacts with. This bug will cause connection failures over http
. I've also provided suggestions to improve the separation of server-side and client-side environment configurations for better maintainability and deployment flexibility. Addressing these points will significantly improve the quality and robustness of the code.
src/app/[owner]/[repo]/page.tsx
Outdated
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 comment
The reason will be displayed to describe this comment to others. Learn more.
I've noticed a critical issue in how this serverBaseUrl
is used on the next line (522) to construct the WebSocket URL.
The logic on line 522 is flawed and will not work for http
protocols, only https
. This will cause WebSocket connection failures in development environments or any deployment using plain http
.
This buggy logic is also duplicated across multiple files:
src/app/[owner]/[repo]/page.tsx
(here and on line 818)src/app/[owner]/[repo]/slides/page.tsx
(lines 268, 544)src/app/[owner]/[repo]/workshop/page.tsx
(line 320)
A correct implementation to replace the logic on line 522 would be:
const wsBaseUrl = serverBaseUrl.replace(/^http/, 'ws');
@@ -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 comment
The reason will be displayed to describe this comment to others. Learn more.
The logic on this line is flawed and will not work for http
protocols, only https
. This will cause WebSocket connection failures in development environments or any deployment using plain http
.
A correct implementation to replace the logic on this line would be:
const wsBaseUrl = serverBaseUrl.replace(/^http/, 'ws');
@@ -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 comment
The reason will be displayed to describe this comment to others. Learn more.
The logic on this line is flawed and will not work for http
protocols, only https
. This will cause WebSocket connection failures in development environments or any deployment using plain http
.
A correct implementation to replace the logic on this line would be:
const wsBaseUrl = serverBaseUrl.replace(/^http/, 'ws');
@@ -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 comment
The reason will be displayed to describe this comment to others. Learn more.
The logic on this line is flawed and will not work for http
protocols, only https
. This will cause WebSocket connection failures in development environments or any deployment using plain http
.
A correct implementation to replace the logic on this line would be:
const wsBaseUrl = serverBaseUrl.replace(/^http/, 'ws');
src/app/[owner]/[repo]/page.tsx
Outdated
@@ -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 comment
The reason will be displayed to describe this comment to others. Learn more.
The logic on this line is flawed and will not work for http
protocols, only https
. This will cause WebSocket connection failures in development environments or any deployment using plain http
.
A correct implementation to replace the logic on this line would be:
const wsBaseUrl = serverBaseUrl.replace(/^http/, 'ws');
@@ -1,6 +1,6 @@ | |||
import type { NextConfig } from "next"; | |||
|
|||
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 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 like next.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.
const TARGET_SERVER_BASE_URL = process.env.NEXT_PUBLIC_SERVER_BASE_URL || 'http://localhost:8001'; | |
const TARGET_SERVER_BASE_URL = process.env.SERVER_BASE_URL || process.env.NEXT_PUBLIC_SERVER_BASE_URL || 'http://localhost:8001'; |
@@ -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 comment
The 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 NEXT_PUBLIC_
prefixed environment variable.
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.
const TARGET_SERVER_BASE_URL = process.env.NEXT_PUBLIC_SERVER_BASE_URL || 'http://localhost:8001'; | |
const TARGET_SERVER_BASE_URL = process.env.SERVER_BASE_URL || process.env.NEXT_PUBLIC_SERVER_BASE_URL || 'http://localhost:8001'; |
698cfe4
to
bbc60d8
Compare
@zcuder aren't you the one who proposed this changes in the first place? XD |
So confirming reverting is the best resolution? |
ENV without NEXT_PUBLIC_* prefix are blocked by nodejs