feat(ui): scaffold react vite frontend#277
Conversation
There was a problem hiding this comment.
Code Review
This pull request migrates the platform UI to a React, Vite, and TypeScript frontend under services/ui/frontend, while maintaining the Go backend for static asset embedding and proxying. The build process is updated via a multi-stage Dockerfile, and the legacy dashboard is temporarily embedded within an iframe. The review feedback highlights a potential test failure in main_test.go due to missing uncommitted assets, and suggests skipping tests gracefully if these files are absent. Additionally, it recommends moving build-time dependencies to devDependencies in package.json and removing an unused, no-op call to readRuntimeConfig() in App.tsx.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| func TestStaticShellLoadsReactBundle(t *testing.T) { | ||
| htmlBody, err := os.ReadFile("static/index.html") | ||
| if err != nil { | ||
| t.Fatalf("read static index: %v", err) | ||
| } |
There was a problem hiding this comment.
Missing Built Assets & Fragile Go Tests
The Go test suite now reads built frontend assets from static/index.html, static/legacy/index.html, and static/legacy/app.js. However, the static/legacy/ directory is not committed to the repository, which will cause go test ./... to fail immediately on clean checkouts or in CI environments that run Go tests before building the frontend.
To resolve this and make the Go tests robust, you should consider one of the following approaches:
- Commit all built assets: If the intention is to keep built assets in version control, ensure that the entire
static/directory (includingstatic/legacy/) is built and committed. - Skip tests when assets are missing: If built assets should not be committed (which is standard practice to avoid git noise), add
services/ui/static/to.gitignoreand update the Go tests to skip gracefully when the files are missing:htmlBody, err := os.ReadFile("static/index.html") if err != nil { if os.IsNotExist(err) { t.Skip("Skipping test: static/index.html not found. Run frontend build first.") } t.Fatalf("read static index: %v", err) }
| "dependencies": { | ||
| "@vitejs/plugin-react": "^5.0.0", | ||
| "vite": "^7.0.0", | ||
| "typescript": "^5.8.0", | ||
| "react": "^19.1.0", | ||
| "react-dom": "^19.1.0" | ||
| }, | ||
| "devDependencies": { | ||
| "@testing-library/jest-dom": "^6.6.0", | ||
| "@testing-library/react": "^16.3.0", | ||
| "@types/node": "^22.15.0", | ||
| "@types/react": "^19.1.0", | ||
| "@types/react-dom": "^19.1.0", | ||
| "jsdom": "^26.1.0", | ||
| "vitest": "^4.1.8" | ||
| } |
There was a problem hiding this comment.
Dependency Organization
The dependencies @vitejs/plugin-react, vite, and typescript are currently listed under dependencies. Since these are build-time and development tools rather than runtime dependencies required by the browser, they should be moved to devDependencies.
This keeps the production bundle clean and adheres to standard npm package management best practices.
| "dependencies": { | |
| "@vitejs/plugin-react": "^5.0.0", | |
| "vite": "^7.0.0", | |
| "typescript": "^5.8.0", | |
| "react": "^19.1.0", | |
| "react-dom": "^19.1.0" | |
| }, | |
| "devDependencies": { | |
| "@testing-library/jest-dom": "^6.6.0", | |
| "@testing-library/react": "^16.3.0", | |
| "@types/node": "^22.15.0", | |
| "@types/react": "^19.1.0", | |
| "@types/react-dom": "^19.1.0", | |
| "jsdom": "^26.1.0", | |
| "vitest": "^4.1.8" | |
| } | |
| "dependencies": { | |
| "react": "^19.1.0", | |
| "react-dom": "^19.1.0" | |
| }, | |
| "devDependencies": { | |
| "@testing-library/jest-dom": "^6.6.0", | |
| "@testing-library/react": "^16.3.0", | |
| "@types/node": "^22.15.0", | |
| "@types/react": "^19.1.0", | |
| "@types/react-dom": "^19.1.0", | |
| "@vitejs/plugin-react": "^5.0.0", | |
| "jsdom": "^26.1.0", | |
| "typescript": "^5.8.0", | |
| "vite": "^7.0.0", | |
| "vitest": "^4.1.8" | |
| } |
| export function App() { | ||
| readRuntimeConfig(); | ||
|
|
There was a problem hiding this comment.
Unused Function Call (No-op)
The call to readRuntimeConfig() on line 5 is currently a no-op because its return value is ignored, and the function itself has no side effects.
If the configuration is not needed in the App component, this call should be removed to keep the code clean and avoid confusion.
| export function App() { | |
| readRuntimeConfig(); | |
| export function App() { |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9c0a93f9ba
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| <iframe | ||
| className="legacy-dashboard" | ||
| src={src} |
There was a problem hiding this comment.
Allow the dashboard iframe in CSP
With this shell, the entire dashboard is now rendered inside a same-origin iframe at /legacy/index.html, but securityHeadersMiddleware still sends frame-src https://accounts.google.com without 'self'. Because an explicit frame-src directive restricts child frames to only those sources, browsers will block this iframe under the app's own CSP, leaving the new React shell unable to display the legacy dashboard in normal UI loads.
Useful? React with 👍 / 👎.
# Conflicts: # services/api/go.mod # services/api/go.sum # services/ingest/go.mod # services/ingest/go.sum # services/mcp-gateway/go.mod # services/mcp-gateway/go.sum # services/processor/go.mod # services/processor/go.sum # services/ui/go.mod # services/ui/go.sum
fixes #276