feat: react router v2 #48
Conversation
WalkthroughThis update transitions the frontend from a Next.js-based setup to a Vite-powered React application using React Router for client-side routing. It introduces new configuration files for Vite and Nginx, updates environment variable usage, revises Docker and deployment configurations, and adapts documentation and code to reflect the new stack. Several new React route components are added. Changes
Sequence Diagram(s)sequenceDiagram
participant Browser
participant Nginx
participant ReactApp
participant API
Browser->>Nginx: Request / (or /survey/:slug)
Nginx->>Browser: Serve index.html and static assets
Browser->>ReactApp: Load JS bundle, mount #root
ReactApp->>API: Fetch surveys or survey details (using VITE_API_URL)
API-->>ReactApp: Return survey data
ReactApp-->>Browser: Render app UI with routing
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Possibly related issues
Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (6)
ui/.env.example (1)
1-1: Environment variable simplification looks good, but add a trailing newline.The consolidation to a single
VITE_API_URLvariable aligns well with the Vite migration and simplifies the configuration.Apply this diff to add the missing blank line at the end:
-VITE_API_URL=http://localhost:9900 +VITE_API_URL=http://localhost:9900 +ui/src/routes/app.surveys.$surveyUuid.responses.tsx (1)
26-66: Consider extracting data fetching logic.The
fetchSurveyDatafunction is quite long and handles multiple concerns. Consider extracting it into a custom hook or separate utility function for better reusability and testability.ui/nginx.conf (1)
41-44: Consider adding Content Security Policy.While the basic security headers are good, consider adding a Content Security Policy (CSP) header for enhanced security against XSS attacks.
Add this line within the server block:
# Security headers add_header X-Frame-Options DENY; add_header X-Content-Type-Options nosniff; add_header X-XSS-Protection "1; mode=block"; +add_header Content-Security-Policy "default-src 'self'; script-src 'self' 'unsafe-inline'; style-src 'self' 'unsafe-inline'; connect-src 'self' https:; img-src 'self' data: https:;";ui/src/lib/api.ts (1)
7-11: Remove debug console.log statements from production code.The console.log statement on line 10 should be removed or conditionally included only in development environments, as it can expose internal API URLs in production logs.
- if (!host) { - // Use environment variable or fallback to default API URL - host = import.meta.env.VITE_API_URL || 'http://localhost:9900' - console.log('Using API host:', host) - } + if (!host) { + // Use environment variable or fallback to default API URL + host = import.meta.env.VITE_API_URL || 'http://localhost:9900' + }ui/src/routes/survey.$urlSlug.tsx (1)
32-32: Consider extracting document title management to a custom hook.Direct manipulation of
document.titleworks but could be extracted to a reusableuseDocumentTitlehook for better separation of concerns.// Custom hook example function useDocumentTitle(title: string) { useEffect(() => { document.title = title }, [title]) } // Usage in component useDocumentTitle(notFound ? 'Survey not found' : survey?.config.title || 'Loading...')Also applies to: 41-41
ui/src/routes/app.tsx (1)
35-43: Consider extracting loading spinner to a reusable component.The loading spinner implementation is duplicated between this component and SurveyPage. Consider creating a shared
LoadingSpinnercomponent.// components/ui/LoadingSpinner.tsx export function LoadingSpinner() { return ( <div className="flex justify-center items-center h-64"> <div className="animate-spin rounded-full h-12 w-12 border-b-2 border-blue-600"></div> </div> ) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
package-lock.jsonis excluded by!**/package-lock.jsonui/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (20)
.gitignore(1 hunks)README.md(2 hunks)compose.yaml(1 hunks)ui/.env.example(1 hunks)ui/.eslintrc.json(1 hunks)ui/Dockerfile(1 hunks)ui/index.html(1 hunks)ui/nginx.conf(1 hunks)ui/package.json(2 hunks)ui/postcss.config.js(1 hunks)ui/src/components/app/survey/SurveyQuestions.tsx(7 hunks)ui/src/lib/api.ts(1 hunks)ui/src/main.tsx(1 hunks)ui/src/routes/app.surveys.$surveyUuid.responses.tsx(1 hunks)ui/src/routes/app.tsx(1 hunks)ui/src/routes/layout.tsx(1 hunks)ui/src/routes/not-found.tsx(1 hunks)ui/src/routes/survey.$urlSlug.tsx(1 hunks)ui/tsconfig.json(1 hunks)ui/vite.config.ts(1 hunks)
🧰 Additional context used
🪛 dotenv-linter (3.3.0)
ui/.env.example
[warning] 1-1: [EndingBlankLine] No blank line at the end of the file
🔇 Additional comments (37)
.gitignore (1)
9-9: LGTM! Correctly ignoring Vite build output.The addition of
ui/distis appropriate for the migration to Vite, which outputs built assets to thedistdirectory by default. This aligns with ignoring other build artifacts.ui/postcss.config.js (1)
1-6: LGTM! Correctly updated to ES module syntax for Vite compatibility.The migration from CommonJS to ES module syntax is necessary for Vite compatibility while maintaining the same PostCSS configuration.
README.md (2)
288-290: LGTM! Documentation correctly updated for the tech stack migration.The deployment section accurately reflects the new architecture components.
319-319: LGTM! Tech stack section correctly updated.The reference to React Router instead of Next.js accurately reflects the migration.
ui/src/routes/not-found.tsx (1)
1-21: LGTM! Well-structured 404 page component.The NotFoundPage component is cleanly implemented with:
- Proper document title setting
- Semantic HTML structure
- Consistent Tailwind CSS styling
- Appropriate use of the AppLayout wrapper
The component follows React best practices and integrates well with the new React Router architecture.
ui/src/routes/layout.tsx (1)
1-22: LGTM! Clean root layout implementation.The component correctly implements a root layout for React Router with proper document head management. The meta description handling logic appropriately checks for existing tags before creating new ones.
ui/vite.config.ts (1)
1-29: Vite configuration and alias usage verified and approved
- The
componentsandlibaliases are actively used across your.tsxand.tsfiles.- The
stylesalias is applied via bare CSS imports (e.g.import 'styles/app.css').- The
@alias is correctly pointed to./srcand ready for future use.- Server, build, and define settings align with the React Router migration.
All looks good—approving the changes.
compose.yaml (2)
22-22: Port mapping correctly updated for Nginx.The port change from 3000 to 80 correctly reflects the migration from Node.js server to Nginx static file serving.
24-24: No change needed to VITE_API_URLThe
VITE_API_URLis consumed client-side (inui/src/lib/api.tsand your route components), so at runtime it’s the browser making the fetch requests. In Docker-Compose you’ve mapped the host’s port 9900 to the API container, so from the browserhttp://localhost:9900correctly reaches your backend. Usinghttp://api:8080would only resolve inside the Docker network—not from the user’s browser—so leave the existing setting as is.ui/index.html (2)
1-17: Well-structured HTML entry point.The HTML file correctly sets up the Vite React application with proper meta tags, favicon links, and module script loading. The dark theme default and Tailwind utility classes are appropriate.
5-11: All referenced favicon assets are presentVerified that the following files exist under
ui/publicand match the HTML references:
- favicon.ico
- favicon-32x32.png
- favicon-16x16.png
- apple-touch-icon.png
- site.webmanifest
ui/src/components/app/survey/SurveyQuestions.tsx (4)
18-18: Good formatting improvement.Adding the trailing comma to the import improves consistency and makes future additions cleaner.
234-263: Excellent addition of explicit block scoping.Adding braces around the Rating case block improves code clarity and prevents potential variable scoping issues in switch statements.
429-450: Consistent block scoping improvements.The explicit braces added to Rating, Ranking, and YesNo case blocks maintain consistency and improve code readability throughout the switch statement.
470-478: Clean multiline formatting.The form element reformatting to multiline improves readability and follows consistent formatting patterns.
ui/Dockerfile (1)
19-33: LGTM! Clean migration to static file serving.The transition from Node.js runtime to Nginx for serving the static React SPA is well-implemented. The configuration correctly:
- Uses a lightweight nginx:alpine base
- Cleans up default Nginx files
- Copies build artifacts from the correct Vite output directory (
/app/dist)- Includes custom Nginx configuration
- Exposes the standard HTTP port 80
ui/src/main.tsx (2)
13-37: Well-structured React Router configuration.The router setup follows React Router best practices with:
- Proper nested routing structure
- Error boundary with
NotFoundPage- Catch-all route for unmatched paths
- Clear separation of concerns with dedicated route components
39-44: Root element verified in index.htmlThe search confirms that
ui/index.htmlcontains an element withid="root". No further changes are needed.ui/src/routes/app.surveys.$surveyUuid.responses.tsx (1)
71-79: LGTM! Good loading state implementation.The loading spinner with proper styling and layout structure provides good user experience during data fetching.
ui/.eslintrc.json (1)
1-19: Well-configured ESLint setup for Vite + React.The configuration properly transitions from Next.js to a Vite-based setup with:
- Appropriate environment settings for browser and ES2020
- Standard recommended rule sets
- React refresh plugin for development
- Proper ignore patterns for build artifacts
ui/nginx.conf (2)
30-33: LGTM! Proper SPA routing configuration.The
try_filesdirective correctly handles client-side routing by falling back toindex.htmlfor all unmatched routes, which is essential for React Router.
35-39: Good static asset caching strategy.The one-year cache duration with immutable headers is appropriate for fingerprinted static assets in a modern build system like Vite.
ui/src/lib/api.ts (2)
48-137: LGTM! Consistent formatting improvements.The formatting and indentation improvements enhance code readability while maintaining the original functionality.
150-150: LGTM! Consistent environment variable migration.The migration from process.env to import.meta.env.VITE_API_URL is correctly implemented across the API functions.
Also applies to: 157-157
ui/src/routes/survey.$urlSlug.tsx (2)
9-48: LGTM! Well-structured React component with proper error handling.The component follows React best practices with proper state management, error handling, and loading states. The useEffect dependency array is correctly set with
[urlSlug].
50-50: Environment variable access is correct.The use of
import.meta.env.VITE_API_URLis appropriate for the Vite migration.ui/package.json (4)
4-4: LGTM! Correct ES module specification for Vite.Adding
"type": "module"is appropriate for the Vite + React setup and aligns with modern ES module standards.
6-10: LGTM! Proper Vite script configuration.The migration from Next.js scripts to Vite commands is correctly implemented with appropriate ESLint configuration.
19-22: LGTM! Appropriate dependency updates for React Router migration.The React and React DOM version upgrades to 18.3.1 and addition of React Router 7.7.1 support the new architecture.
34-47: LGTM! Comprehensive Vite ecosystem integration.The devDependencies updates correctly include Vite, its React plugin, and updated TypeScript/ESLint tooling for the new setup.
ui/tsconfig.json (5)
4-10: LGTM! Appropriate target and library updates for modern environment.The ES2020 target and capitalized library references align with modern TypeScript and Vite best practices.
17-18: LGTM! Correct module resolution for Vite.The "bundler" module resolution and
allowImportingTsExtensionssetting are appropriate for Vite's module handling.
22-22: LGTM! Correct JSX setting for React 18.The "react-jsx" JSX setting is appropriate for React 18 and Vite, replacing Next.js's "preserve" setting.
24-24: LGTM! Proper Vite client types inclusion.Adding "vite/client" types replaces the Next.js plugin and provides proper Vite environment typings.
31-33: LGTM! Updated include patterns for new project structure.The include patterns correctly target the new Vite-based project structure with src directory and vite.config.ts.
ui/src/routes/app.tsx (2)
8-33: LGTM! Well-structured component with proper React patterns.The component correctly implements React hooks, error handling, and loading states. The useEffect dependency array is appropriately empty for mount-only execution.
53-53: LGTM! Consistent environment variable usage.The use of
import.meta.env.VITE_API_URLis consistent with the Vite migration and other components.
| if (init) { | ||
| init['cache'] = 'no-store' | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ensure cache setting is applied consistently.
The cache setting is now only applied when init is provided. This could cause caching issues for API calls that don't provide an init parameter.
- if (init) {
- init['cache'] = 'no-store'
- }
+ if (!init) {
+ init = {}
+ }
+ init['cache'] = 'no-store'📝 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.
| if (init) { | |
| init['cache'] = 'no-store' | |
| } | |
| if (!init) { | |
| init = {} | |
| } | |
| init['cache'] = 'no-store' |
🤖 Prompt for AI Agents
In ui/src/lib/api.ts around lines 3 to 5, the cache setting 'no-store' is only
applied when the init parameter is provided, which can lead to inconsistent
caching behavior. Modify the code to ensure that the cache setting is always
applied by initializing the init object if it is undefined, then setting
init['cache'] to 'no-store' consistently for all API calls.
| apiURL | ||
| ) | ||
| } | ||
| if (payload! instanceof FormData) { |
There was a problem hiding this comment.
Fix syntax error in instanceof check.
The negation operator placement is incorrect and will cause a runtime error.
- if (payload! instanceof FormData) {
+ if (!(payload instanceof FormData)) {📝 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.
| if (payload! instanceof FormData) { | |
| if (!(payload instanceof FormData)) { |
🤖 Prompt for AI Agents
In ui/src/lib/api.ts at line 233, the syntax for the instanceof check is
incorrect due to the misplaced negation operator. Fix this by properly grouping
the expression with parentheses so that the negation applies to the entire
instanceof check, for example, use !(payload instanceof FormData) instead of
payload! instanceof FormData.
| const apiURL = import.meta.env.VITE_API_URL || '' | ||
| const surveySessionsResp = await getSurveySessions( | ||
| survey.uuid, | ||
| `limit=${SurveySessionsLimit}&offset=0&sort_by=created_at&order=desc`, | ||
| apiURL | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Inconsistent API URL usage.
The component fetches the API URL from environment variables twice (lines 45 and 89) but only passes it to getSurveySessions. For consistency, either:
- Pass
apiURLto all API calls, or - Handle API URL resolution within the API functions themselves
Apply this diff to use consistent API URL handling:
- const apiURL = import.meta.env.VITE_API_URL || ''
- const surveySessionsResp = await getSurveySessions(
- survey.uuid,
- `limit=${SurveySessionsLimit}&offset=0&sort_by=created_at&order=desc`,
- apiURL
- )
+ const surveySessionsResp = await getSurveySessions(
+ survey.uuid,
+ `limit=${SurveySessionsLimit}&offset=0&sort_by=created_at&order=desc`
+ )📝 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.
| const apiURL = import.meta.env.VITE_API_URL || '' | |
| const surveySessionsResp = await getSurveySessions( | |
| survey.uuid, | |
| `limit=${SurveySessionsLimit}&offset=0&sort_by=created_at&order=desc`, | |
| apiURL | |
| ) | |
| const surveySessionsResp = await getSurveySessions( | |
| survey.uuid, | |
| `limit=${SurveySessionsLimit}&offset=0&sort_by=created_at&order=desc` | |
| ) |
🤖 Prompt for AI Agents
In ui/src/routes/app.surveys.$surveyUuid.responses.tsx around lines 45 to 50,
the API URL is fetched from environment variables and passed only to
getSurveySessions, while other API calls fetch the URL separately. To fix this
inconsistency, refactor the code to either pass the apiURL variable to all API
calls uniformly or modify the API functions to internally resolve the API URL,
ensuring consistent usage across the component.
| define: { | ||
| 'process.env': {}, | ||
| }, |
There was a problem hiding this comment.
Potential issue with process.env override.
Defining process.env as an empty object will prevent access to all environment variables including VITE_API_URL mentioned in the compose.yaml. This could break API communication.
Consider removing this definition or using a more targeted approach:
- define: {
- 'process.env': {},
- },If you need to define specific variables, use individual definitions:
+ define: {
+ 'process.env.NODE_ENV': JSON.stringify(process.env.NODE_ENV),
+ },📝 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.
| define: { | |
| 'process.env': {}, | |
| }, | |
| define: { | |
| 'process.env.NODE_ENV': JSON.stringify(process.env.NODE_ENV), | |
| }, |
🤖 Prompt for AI Agents
In ui/vite.config.ts around lines 15 to 17, defining 'process.env' as an empty
object overrides all environment variables, blocking access to needed ones like
VITE_API_URL. To fix this, remove the 'process.env' definition entirely or
replace it with individual environment variable definitions that explicitly set
only the required variables, preserving access to others.
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Chores