Conversation
WalkthroughThis change migrates the React frontend from CRA/Webpack/Jest to Vite. It removes CRA-specific configs and scripts (webpack, dev server, Jest transforms, env/modules/paths utilities, build/start/test scripts) and adds Vite config, a new index.html, and a new ESLint config. Package scripts and dependencies are updated accordingly. The Dockerfile splits yarn install from build and removes frontend tests. React rendering is updated to createRoot. Minor component tweaks adjust props and event handler signatures. TypeScript ambient declarations and Jest setup are removed. The Go webserver switches static handling from /pub to /assets with hashed asset serving and explicit routes for common static files. Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Broad build-system migration with many deletions and new configs, plus webserver routing changes. Although many files are removed wholesale, reviewers must verify Vite parity (index.html, asset handling, environment usage), package.json/scripts, Dockerfile flow, and the Go server’s new static/asset paths and caching semantics. Frontend code edits are small but need sanity checks. Changes are heterogeneous across JS/TS, HTML, Docker, and Go, requiring cross-cutting validation. Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
build/react-frontend/eslint.config.js (1)
1-28: LGTM! Consider updating ecmaVersion for modern features.The ESLint config correctly uses the flat config format required for ESLint 9 and includes appropriate plugins for React/TypeScript/Vite development.
Consider updating
ecmaVersionfrom2020to'latest'or2023to support modern JavaScript features:languageOptions: { - ecmaVersion: 2020, + ecmaVersion: 'latest', globals: globals.browser, },build/react-frontend/src/index.jsx (1)
4-7: LGTM! Correct React 18 createRoot migration.The migration from
ReactDOM.rendertocreateRootcorrectly follows React 18 API patterns. The code properly obtains the container, creates a root, and renders the app.Consider wrapping
<App />with<StrictMode>to surface React 18.3's new deprecation warnings during development:+import { StrictMode } from 'react'; import { createRoot } from 'react-dom/client'; const container = document.getElementById('root'); const root = createRoot(container); -root.render(<App />); +root.render( + <StrictMode> + <App /> + </StrictMode> +);Based on learnings: React 18.3.0 adds deprecation warnings to prepare for React 19, and StrictMode helps surface these warnings during development.
internal/webserver/webserver.go (1)
111-116: Consider adding Cache-Control headers for public static files.The static file serving is functionally correct. However, unlike the
/assets/handler (which sets explicit caching policies), these routes rely on default browser caching behavior.For better cache control:
manifest.json,robots.txt:Cache-Control: public, no-cache(revalidate on each use)logo192.png,logo512.png,favicon.ico:Cache-Control: public, max-age=86400(1 day with revalidation)Example refactor for explicit caching:
staticServer := http.FileServer(http.Dir("./web/react-frontend")) -r.HandleFunc("/manifest.json", staticServer.ServeHTTP) -r.HandleFunc("/robots.txt", staticServer.ServeHTTP) -r.HandleFunc("/logo192.png", staticServer.ServeHTTP) -r.HandleFunc("/logo512.png", staticServer.ServeHTTP) -r.HandleFunc("/favicon.ico", staticServer.ServeHTTP) + +staticHandler := func(cacheControl string) http.HandlerFunc { + return func(w http.ResponseWriter, r *http.Request) { + w.Header().Add("Cache-Control", cacheControl) + staticServer.ServeHTTP(w, r) + } +} +r.HandleFunc("/manifest.json", staticHandler("public, no-cache")) +r.HandleFunc("/robots.txt", staticHandler("public, no-cache")) +r.HandleFunc("/logo192.png", staticHandler("public, max-age=86400")) +r.HandleFunc("/logo512.png", staticHandler("public, max-age=86400")) +r.HandleFunc("/favicon.ico", staticHandler("public, max-age=86400"))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
build/react-frontend/yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (26)
build/package/Dockerfile(1 hunks)build/react-frontend/config/env.js(0 hunks)build/react-frontend/config/getHttpsConfig.js(0 hunks)build/react-frontend/config/jest/babelTransform.js(0 hunks)build/react-frontend/config/jest/cssTransform.js(0 hunks)build/react-frontend/config/jest/fileTransform.js(0 hunks)build/react-frontend/config/modules.js(0 hunks)build/react-frontend/config/paths.js(0 hunks)build/react-frontend/config/webpack.config.js(0 hunks)build/react-frontend/config/webpack/persistentCache/createEnvironmentHash.js(0 hunks)build/react-frontend/config/webpackDevServer.config.js(0 hunks)build/react-frontend/eslint.config.js(1 hunks)build/react-frontend/index.html(1 hunks)build/react-frontend/package.json(1 hunks)build/react-frontend/public/index.html(0 hunks)build/react-frontend/scripts/build.js(0 hunks)build/react-frontend/scripts/start.js(0 hunks)build/react-frontend/scripts/test.js(0 hunks)build/react-frontend/src/Components/EnhancedTableHead/EnhancedTableHead.jsx(2 hunks)build/react-frontend/src/Components/TextPage/TextPage.jsx(1 hunks)build/react-frontend/src/index.jsx(1 hunks)build/react-frontend/src/lib/rollback-api.ts(1 hunks)build/react-frontend/src/react-app-env.d.ts(0 hunks)build/react-frontend/src/setupTests.js(0 hunks)build/react-frontend/vite.config.js(1 hunks)internal/webserver/webserver.go(1 hunks)
💤 Files with no reviewable changes (16)
- build/react-frontend/public/index.html
- build/react-frontend/config/webpack/persistentCache/createEnvironmentHash.js
- build/react-frontend/config/webpackDevServer.config.js
- build/react-frontend/config/env.js
- build/react-frontend/scripts/build.js
- build/react-frontend/src/react-app-env.d.ts
- build/react-frontend/scripts/test.js
- build/react-frontend/scripts/start.js
- build/react-frontend/src/setupTests.js
- build/react-frontend/config/jest/cssTransform.js
- build/react-frontend/config/paths.js
- build/react-frontend/config/jest/fileTransform.js
- build/react-frontend/config/modules.js
- build/react-frontend/config/getHttpsConfig.js
- build/react-frontend/config/jest/babelTransform.js
- build/react-frontend/config/webpack.config.js
🧰 Additional context used
🧬 Code graph analysis (1)
internal/webserver/webserver.go (1)
internal/utility/cache-control.go (1)
UncachedErrorWriter(8-11)
🔇 Additional comments (11)
build/react-frontend/src/lib/rollback-api.ts (1)
161-172: LGTM! Modern global reference usage.The replacement of
windowwithglobalThisis correct and aligns with modern JavaScript best practices. While both are equivalent in browser contexts,globalThisis the standardized ECMAScript global object reference that works across all JavaScript environments.For browser-only code like this React app,
windowwould also be perfectly valid, but usingglobalThisis a sound choice that aligns with the Vite migration's modernization efforts.build/react-frontend/src/Components/TextPage/TextPage.jsx (1)
25-25: LGTM! Correct Material-UI variant usage.
body1is the standard Material-UI Typography variant for body text. The previousvariant="p"was not a documented variant in Material-UI v4.build/package/Dockerfile (1)
13-15: Verify test coverage is maintained outside Docker build.The separation of
yarn installandyarn buildimproves Docker layer caching. However, the removal ofyarn testfrom the Docker build means frontend tests are no longer run during the image build process.Confirm that frontend tests are executed in your CI/CD pipeline before merging, as the migration from CRA to Vite removed Jest-based test infrastructure.
build/react-frontend/package.json (4)
5-5: LGTM! Required for Vite's ES module support.Adding
"type": "module"enables ES modules in the package, which is required for Vite's default configuration and build system.
28-33: LGTM! Vite scripts correctly configured.The scripts correctly migrate from
react-scriptsto Vite commands. Note that notestscript is present, which aligns with the removal of Jest-based testing infrastructure as part of the CRA migration.
20-20: LGTM! Self-hosted font addition.Adding
@fontsource/interprovides self-hosted font files, improving performance and privacy compared to CDN-hosted fonts.
11-17: @vitejs/plugin-react v4.3.1 is compatible with Vite 5.4.1.build/react-frontend/index.html (1)
1-21: LGTM! Correct Vite HTML entry point.The HTML structure is correctly configured for Vite:
- Module script with
/src/index.jsxentry point- Root-relative asset paths that Vite will resolve
- Proper meta tags and static asset links
build/react-frontend/vite.config.js (1)
1-11: LGTM! Minimal but correct Vite configuration.The Vite config correctly:
- Uses
defineConfigfor type safety- Sets
outDir: 'build'to match Docker expectations (line 24 in Dockerfile)- Includes the React plugin for JSX/Fast Refresh support
build/react-frontend/src/Components/EnhancedTableHead/EnhancedTableHead.jsx (1)
7-9: Approve createSortHandler refactor
Verified that bothhandleRequestSort(in ReleaseDetails) andsetReleaseOrder(fromuseSortStatein ReleaseList) accept only a single property argument, so dropping the event parameter is safe.internal/webserver/webserver.go (1)
100-109: LGTM! Asset serving strategy aligns well with Vite.The hashed assets are correctly served with immutable caching, and the
UncachedErrorWriterwrapper appropriately loosens caching on 404s to prevent mixed-fleet cache poisoning during deployments.
| key={headCell.id} | ||
| align={headCell.numeric ? 'right' : 'left'} | ||
| padding={headCell.disablePadding ? 'none' : 'default'} | ||
| padding={headCell.disablePadding ? 'none' : 'normal'} |
There was a problem hiding this comment.
Incorrect padding value for Material-UI v4.
The padding prop value 'normal' is not valid in Material-UI v4. Valid values are 'default', 'checkbox', or 'none'. The value 'normal' was introduced in Material-UI v5.
Since package.json specifies @material-ui/core@^4.12.4, this will cause a runtime warning or incorrect rendering.
Apply this diff to fix:
- padding={headCell.disablePadding ? 'none' : 'normal'}
+ padding={headCell.disablePadding ? 'none' : 'default'}📝 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.
| padding={headCell.disablePadding ? 'none' : 'normal'} | |
| padding={headCell.disablePadding ? 'none' : 'default'} |
🤖 Prompt for AI Agents
In build/react-frontend/src/Components/EnhancedTableHead/EnhancedTableHead.jsx
around line 18, the padding prop currently uses 'normal' which is invalid for
Material-UI v4; change the non-none value to 'default' so the prop becomes
padding={headCell.disablePadding ? 'none' : 'default'} to match
@material-ui/core@^4.12.4 valid values and avoid runtime warnings.
1 participant
Clear out almost all the
create-react-appstuff since it's effectively an abandoned project and is starting to age on a dependency and technical level.