Skip to content

feat(ui): scaffold react vite frontend#277

Open
Agent-Hellboy wants to merge 7 commits into
mainfrom
ui/react-vite-scaffold
Open

feat(ui): scaffold react vite frontend#277
Agent-Hellboy wants to merge 7 commits into
mainfrom
ui/react-vite-scaffold

Conversation

@Agent-Hellboy

@Agent-Hellboy Agent-Hellboy commented Jun 2, 2026

Copy link
Copy Markdown
Owner

fixes #276

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread services/ui/main_test.go Outdated
Comment on lines +57 to +61
func TestStaticShellLoadsReactBundle(t *testing.T) {
htmlBody, err := os.ReadFile("static/index.html")
if err != nil {
t.Fatalf("read static index: %v", err)
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

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:

  1. Commit all built assets: If the intention is to keep built assets in version control, ensure that the entire static/ directory (including static/legacy/) is built and committed.
  2. 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 .gitignore and 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)
    }

Comment on lines +12 to +27
"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"
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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.

Suggested change
"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"
}

Comment thread services/ui/frontend/src/App.tsx Outdated
Comment on lines +4 to +6
export function App() {
readRuntimeConfig();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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.

Suggested change
export function App() {
readRuntimeConfig();
export function App() {

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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".

Comment on lines +7 to +9
<iframe
className="legacy-dashboard"
src={src}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Use React

1 participant