feat: Simplify Docker builds and centralize frontend API configuration#28
Conversation
…API URLs - Use distroless/static base images instead of Alpine - Remove self-signed certificate generation from Dockerfiles - Add centralized config.js for API_BASE, WS_BASE, HTTP_TUNNEL_BASE - Remove hardcoded API_BASE strings from frontend components - Remove unused GUAC_CLIENT_URL env var from Vite config - Update page title to KubeBrowse Signed-off-by: Sai Sanjay <saisanjay7660@gmail.com>
|
📝 WalkthroughWalkthroughThe change moves container runtimes to non-root distroless images, removes certificate generation, centralizes frontend endpoint configuration, updates Guacamole lifecycle handling, removes Vite proxying, and adjusts deployment and local environment settings. ChangesRuntime and frontend integration
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Browser
participant FrontendConfig
participant GuacClient
participant BackendAPI
Browser->>FrontendConfig: Load VITE_API_BASE_URL
FrontendConfig->>GuacClient: Provide API_BASE, WS_BASE, HTTP_TUNNEL_BASE
GuacClient->>BackendAPI: Request session and tunnel endpoints
BackendAPI-->>GuacClient: Return session or tunnel data
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
sanjay7178
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
frontend/src/hooks/useGuacWebSocket.js (1)
628-645: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winPrefix the share request with
API_BASE. This call is still relative, so deployments withVITE_API_BASE_URLon a different origin will hit the frontend instead of the backend. Use${API_BASE}/api/v1/sessions/${sessionUUID}/shareto match the other session API calls.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/hooks/useGuacWebSocket.js` around lines 628 - 645, Update enableSessionSharing to prefix the share request URL with the existing API_BASE value, using the same /api/v1/sessions/${sessionUUID}/share path so deployments with a separate backend origin route the request correctly.frontend/src/components/GuacClient.jsx (1)
88-104: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winReconnect should tear down old listeners before re-initializing input
setupClientDisplay()/setupMouseAndKeyboard()andclipboard.install(client)run again on reconnect, but the previousGuacamole.Mouse,Guacamole.Keyboard,contextmenu, andwindowclipboard listeners stay attached. That can accumulate duplicate handlers and repeat input/clipboard dispatches; add cleanup or make these registrations idempotent.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/components/GuacClient.jsx` around lines 88 - 104, Update the useEffect initialization around setupClientDisplay() and clipboard.install(client) so reconnects remove or reuse the prior Guacamole.Mouse, Guacamole.Keyboard, contextmenu, and window clipboard listeners before registering new ones. Ensure cleanup runs when client or connected changes and on unmount, while preserving the existing client.onclipboard and client.onargv setup.frontend/src/components/WebSocketControl.jsx (1)
238-278: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winPrefix the share requests with
API_BASE.WebSocketControl.jsxanduseGuacWebSocket.jsstill call/api/v1/sessions/.../shareas a relative URL, while the rest of the frontend already usesAPI_BASEfor backend calls. On separate frontend/API origins, these requests will hit the wrong host and break session sharing.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/components/WebSocketControl.jsx` around lines 238 - 278, Update handleShareSession in WebSocketControl.jsx to prefix the share endpoint with the existing API_BASE value, matching the backend URL construction used elsewhere in the frontend. Preserve the current connectionId path, request options, response handling, and sharing behavior.
🧹 Nitpick comments (3)
frontend/src/components/ShareWSSession.jsx (1)
34-65: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract duplicated session-storage helpers into a shared utility.
saveSessionToStorage,loadSessionFromStorage, andclearSessionFromStorageare defined identically in bothShareWSSession.jsx(lines 34–65) andOfficeSession.jsx(lines 36–68), including theSESSION_STORAGE_KEYandSESSION_TIMEOUTconstants. Any future change to expiry logic, key naming, or error handling must be replicated in both files, risking silent drift.Extract these into a shared module (e.g.,
lib/sessionStorage.js) and import from both components.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/components/ShareWSSession.jsx` around lines 34 - 65, Extract saveSessionToStorage, loadSessionFromStorage, and clearSessionFromStorage, along with SESSION_STORAGE_KEY and SESSION_TIMEOUT, into a shared session-storage utility module. Remove the duplicated definitions from ShareWSSession and OfficeSession, import the shared helpers in both components, and preserve the existing expiry, serialization, cleanup, and error-handling behavior.Makefile (1)
33-36: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRedundant echo statements.
Lines 34 and 35 both print near-identical messages ("Running server..." and "Starting server..."). Consider removing one to avoid clutter.
♻️ Suggested cleanup
run: deps - `@echo` "Running server..." `@echo` "Starting server..." go run cmd/guac/main.go🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Makefile` around lines 33 - 36, Remove one of the redundant echo statements in the Makefile run target, keeping a single clear startup message before the go run command.frontend/src/config.js (1)
12-18: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueInconsistent use of
locationvswindow.location.Line 14 uses
location.hostwhile line 18 useswindow.location.host. Both resolve to the same value, but the inconsistency is confusing. Usewindow.locationconsistently for clarity.♻️ Suggested fix
export const WS_BASE = apiUrl ? `${apiUrl.protocol === "https:" ? "wss" : "ws"}://${apiUrl.host}` - : `${window.location.protocol === "https:" ? "wss" : "ws"}://${location.host}`; + : `${window.location.protocol === "https:" ? "wss" : "ws"}://${window.location.host}`;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/config.js` around lines 12 - 18, Use window.location consistently in the fallback branch of WS_BASE, replacing the bare location.host reference while preserving the existing protocol and host construction.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@frontend/src/components/ShareWSSession.jsx`:
- Around line 139-156: Update handleSubmit to remove the unreachable "creating"
status and its corresponding loading UI, since the handler has no await and
immediately transitions to "ready" or "error"; preserve the existing validation
and final session-state behavior.
In `@frontend/src/config.js`:
- Around line 7-10: Guard the apiUrl initialization derived from API_BASE so
relative values such as “/api” do not cause new URL(API_BASE) to throw. Update
the API_BASE/apiUrl setup to construct a URL only for valid absolute URLs,
otherwise use the existing null fallback, while preserving behavior for valid
full URLs.
---
Outside diff comments:
In `@frontend/src/components/GuacClient.jsx`:
- Around line 88-104: Update the useEffect initialization around
setupClientDisplay() and clipboard.install(client) so reconnects remove or reuse
the prior Guacamole.Mouse, Guacamole.Keyboard, contextmenu, and window clipboard
listeners before registering new ones. Ensure cleanup runs when client or
connected changes and on unmount, while preserving the existing
client.onclipboard and client.onargv setup.
In `@frontend/src/components/WebSocketControl.jsx`:
- Around line 238-278: Update handleShareSession in WebSocketControl.jsx to
prefix the share endpoint with the existing API_BASE value, matching the backend
URL construction used elsewhere in the frontend. Preserve the current
connectionId path, request options, response handling, and sharing behavior.
In `@frontend/src/hooks/useGuacWebSocket.js`:
- Around line 628-645: Update enableSessionSharing to prefix the share request
URL with the existing API_BASE value, using the same
/api/v1/sessions/${sessionUUID}/share path so deployments with a separate
backend origin route the request correctly.
---
Nitpick comments:
In `@frontend/src/components/ShareWSSession.jsx`:
- Around line 34-65: Extract saveSessionToStorage, loadSessionFromStorage, and
clearSessionFromStorage, along with SESSION_STORAGE_KEY and SESSION_TIMEOUT,
into a shared session-storage utility module. Remove the duplicated definitions
from ShareWSSession and OfficeSession, import the shared helpers in both
components, and preserve the existing expiry, serialization, cleanup, and
error-handling behavior.
In `@frontend/src/config.js`:
- Around line 12-18: Use window.location consistently in the fallback branch of
WS_BASE, replacing the bare location.host reference while preserving the
existing protocol and host construction.
In `@Makefile`:
- Around line 33-36: Remove one of the redundant echo statements in the Makefile
run target, keeping a single clear startup message before the go run command.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7aa03d26-71db-4974-88f2-e23aa0025b9e
📒 Files selected for processing (15)
DockerfileDockerfile.tiltMakefiledeployments/manifest.ymlfrontend/.env.examplefrontend/Caddyfilefrontend/index.htmlfrontend/src/components/BrowserSession.jsxfrontend/src/components/GuacClient.jsxfrontend/src/components/OfficeSession.jsxfrontend/src/components/ShareWSSession.jsxfrontend/src/components/WebSocketControl.jsxfrontend/src/config.jsfrontend/src/hooks/useGuacWebSocket.jsfrontend/vite.config.js
💤 Files with no reviewable changes (2)
- frontend/Caddyfile
- frontend/vite.config.js
| setSessionState((prev) => ({ ...prev, status: "creating", error: null })); | ||
|
|
||
| // Just take the UUID directly, no parsing | ||
| const uuid = inputUuid.trim(); | ||
|
|
||
| // Validate UUID format | ||
| if (!uuid.match(/^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$/i)) { | ||
| throw new Error('Invalid UUID format. Please enter a valid connection ID.'); | ||
| throw new Error("Invalid UUID format. Please enter a valid connection ID."); | ||
| } | ||
|
|
||
| const newSessionName = sessionName || `Shared Session: ${uuid.substring(0, 8)}`; | ||
| // Set the connection state with the UUID | ||
| setSessionState({ | ||
| connectionId: uuid, | ||
| status: 'ready', | ||
| status: "ready", | ||
| error: null, | ||
| name: newSessionName | ||
| name: newSessionName, | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
"creating" status is unreachable due to React automatic batching.
handleSubmit calls setSessionState with status: "creating" at line 139 and then immediately overwrites it with status: "ready" at line 151 (or status: "error" at line 165 in the catch block). Both updates execute synchronously in the same event handler, so React 18+ batches them and only commits the final state. The "creating" UI block at lines 232–237 will never render. Additionally, the function is declared async but contains no await.
Either remove the dead "creating" state and its UI, or introduce an actual async operation (e.g., an API call) between the two state updates so the loading state is visible to the user.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@frontend/src/components/ShareWSSession.jsx` around lines 139 - 156, Update
handleSubmit to remove the unreachable "creating" status and its corresponding
loading UI, since the handler has no await and immediately transitions to
"ready" or "error"; preserve the existing validation and final session-state
behavior.
| export const API_BASE = (import.meta.env.VITE_API_BASE_URL || "").replace(/\/$/, ""); | ||
|
|
||
| // Derive WebSocket / HTTP tunnel bases from the same origin so they stay in sync. | ||
| const apiUrl = API_BASE ? new URL(API_BASE) : null; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
new URL(API_BASE) will throw on relative URLs.
If VITE_API_BASE_URL is set to a relative path (e.g., /api), API_BASE becomes truthy and new URL(API_BASE) throws TypeError: Invalid URL. The comments recommend full URLs, but there's no guard. Consider wrapping in a try/catch or validating that the value is an absolute URL before constructing.
🛡️ Proposed guard
-const apiUrl = API_BASE ? new URL(API_BASE) : null;
+let apiUrl = null;
+if (API_BASE) {
+ try {
+ apiUrl = new URL(API_BASE);
+ } catch {
+ console.warn(`VITE_API_BASE_URL="${API_BASE}" is not a valid absolute URL; falling back to window.location`);
+ }
+}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export const API_BASE = (import.meta.env.VITE_API_BASE_URL || "").replace(/\/$/, ""); | |
| // Derive WebSocket / HTTP tunnel bases from the same origin so they stay in sync. | |
| const apiUrl = API_BASE ? new URL(API_BASE) : null; | |
| export const API_BASE = (import.meta.env.VITE_API_BASE_URL || "").replace(/\/$/, ""); | |
| // Derive WebSocket / HTTP tunnel bases from the same origin so they stay in sync. | |
| let apiUrl = null; | |
| if (API_BASE) { | |
| try { | |
| apiUrl = new URL(API_BASE); | |
| } catch { | |
| console.warn(`VITE_API_BASE_URL="${API_BASE}" is not a valid absolute URL; falling back to window.location`); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@frontend/src/config.js` around lines 7 - 10, Guard the apiUrl initialization
derived from API_BASE so relative values such as “/api” do not cause new
URL(API_BASE) to throw. Update the API_BASE/apiUrl setup to construct a URL only
for valid absolute URLs, otherwise use the existing null fallback, while
preserving behavior for valid full URLs.
1 participant
Summary by CodeRabbit
New Features
Improvements
Bug Fixes