Development mode for sync server (React Fast Refresh on port 5006) #4372
Conversation
actual-github-bot
bot
changed the title
✅ Deploy Preview for actualbudget ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
lelemm
changed the title
WalkthroughThe changes introduce two new scripts in the main Suggested labels
Suggested reviewers
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. 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: 0
🧹 Nitpick comments (1)
packages/sync-server/src/app.js (1)
85-98: Consider extracting API paths to a constant.The list of API paths in the context function could be moved to a constant to improve maintainability.
+const API_PATHS = [ + '/api', + '/sync', + '/account', + '/gocardless', + '/simplefin', + '/secret', + '/admin', + '/openid', + '/mode' +]; + context: (pathname, req) => { return ( - !pathname.startsWith('/api') && - !pathname.startsWith('/sync') && - !pathname.startsWith('/account') && - !pathname.startsWith('/gocardless') && - !pathname.startsWith('/simplefin') && - !pathname.startsWith('/secret') && - !pathname.startsWith('/admin') && - !pathname.startsWith('/openid') && - !pathname.startsWith('/mode') && + !API_PATHS.some(path => pathname.startsWith(path)) && !req.url.includes('.') ); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
upcoming-release-notes/4372.mdis excluded by!**/*.mdyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (3)
package.json(1 hunks)packages/sync-server/package.json(1 hunks)packages/sync-server/src/app.js(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Visual regression
- GitHub Check: Functional
- GitHub Check: Build Docker image (alpine)
- GitHub Check: Build Docker image (ubuntu)
🔇 Additional comments (5)
packages/sync-server/package.json (1)
60-60: LGTM! Appropriate dependency configuration.The http-proxy-middleware is correctly added as a development dependency since it's only used in development mode.
package.json (1)
23-23: Verify port configuration for development mode.The script correctly sets up parallel execution of server and client, but the PR mentions port 5006 while the script doesn't specify any port configuration.
Could you clarify where the port 5006 configuration is set? This might be in a configuration file not included in the PR.
packages/sync-server/src/app.js (3)
28-37: LGTM! Good practice to disable rate limiting in development.The condition is well-structured and maintains rate limiting for production while allowing unrestricted access during development.
74-100: LGTM! Well-structured proxy configuration.The proxy middleware is well configured with:
- Appropriate target (localhost:3001)
- WebSocket support enabled
- Debug logging
- Comprehensive path exclusions
101-106: LGTM! Clear production mode handling.The else block appropriately handles static file serving in production mode.
There was a problem hiding this comment.
Added some thoughts here: lelemm#3
Main changes were to:
- Open the server url instead of the client url when launching
- Remove the config that wasn't working
Feel free to merge or to rework it but it would be good to have those things in this PR.
I changed to open 5006 when running yarn start:server-dev |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/sync-server/src/app.js (1)
87-92: Consider adding error handling for static file serving.While the static file serving works, it might benefit from error handling for cases where index.html is not found.
- app.get('/*', (req, res) => res.sendFile(config.webRoot + '/index.html')); + app.get('/*', (req, res, next) => { + res.sendFile(config.webRoot + '/index.html', err => { + if (err) { + console.error('Error serving index.html:', err); + next(err); + } + }); + });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (4)
package.json(1 hunks)packages/desktop-client/vite.config.mts(1 hunks)packages/sync-server/package.json(2 hunks)packages/sync-server/src/app.js(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/sync-server/package.json
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: compare
🔇 Additional comments (4)
package.json (1)
23-24: LGTM! Well-structured development scripts.The new scripts effectively enable development mode with automatic monitoring:
start:server-monitorprovides live reloading through nodemonstart:server-devcorrectly sets up the development environment and runs necessary processes in parallelpackages/sync-server/src/app.js (2)
28-37: LGTM! Appropriate rate limiting configuration.The rate limiting is correctly disabled in development mode while maintaining protection in production.
74-86:❓ Verification inconclusive
Verify WebSocket proxy configuration.
The proxy middleware is well-configured, but let's verify the WebSocket setup to ensure it properly handles React Fast Refresh connections.
🌐 Web query:
What is the recommended WebSocket configuration for http-proxy-middleware with React Fast Refresh?💡 Result:
To configure WebSockets with http-proxy-middleware for React Fast Refresh/HMR (Hot Module Replacement), follow these best practices:
1. Basic Proxy Configuration
Enable WebSocket support by setting
ws: trueand handle HTTP upgrade events:// src/setupProxy.js (Create React App) const { createProxyMiddleware } = require('http-proxy-middleware'); module.exports = function(app) { app.use( createProxyMiddleware('/sockjs-node', { target: 'http://localhost:3000', // Your dev server ws: true, changeOrigin: true }) ); };For non-CRA setups:
const express = require('express'); const { createProxyMiddleware } = require('http-proxy-middleware'); const app = express(); const wsProxy = createProxyMiddleware({ target: 'http://localhost:3000', ws: true, pathFilter: '/sockjs-node' }); app.use(wsProxy); const server = app.listen(8080); // Proxy port server.on('upgrade', wsProxy.upgrade); // Critical for WS upgrade [1][3]
2. React Fast Refresh Requirements
- WebSocket Endpoint: Fast Refresh uses
/sockjs-nodeby default- Dynamic Protocol Handling:
// In React components const protocol = window.location.protocol === 'https:' ? 'wss:' : 'ws:'; const socket = new WebSocket(`${protocol}//${window.location.host}/sockjs-node`);
Key Configuration Options
Feature Configuration Purpose WebSocket Support ws: trueEnables WS proxying [1][4][10] Path Filtering pathFilter: '/sockjs-node'Limits proxy to HMR endpoints HTTP Upgrade server.on('upgrade')Required for WS connection initialization [1][3] Secure Context changeOrigin: trueHandles cross-origin requests correctly
Common Issues & Fixes
Connection Drops:
- Ensure proxy server listens for
upgradeevents [3]- Verify no conflicting CORS policies
HTTPS Context:
// For HTTPS dev servers const socket = new WebSocket(`wss://${window.location.host}/sockjs-node`);Path Rewrites (if needed):
createProxyMiddleware({ target: 'http://localhost:3000', ws: true, pathRewrite: { '^/websocket': '' } // Removes path segment });
This configuration ensures proper WebSocket handling for React's Fast Refresh system while maintaining compatibility with http-proxy-middleware's capabilities[1][3][6]. Always test both HTTP and WS connections when modifying proxy settings.
Citations:
- 1: https://github.com/chimurai/http-proxy-middleware/blob/master/recipes/websocket.md
- 2: https://ably.com/blog/websockets-react-tutorial
- 3: https://github.com/chimurai/http-proxy-middleware
- 4: https://www.npmjs.com/package/http-proxy-middleware/v/2.0.3
- 5: https://maybe.works/blogs/react-websocket
- 6: http://saule1508.github.io/create-react-app-proxy-websocket/
- 7: https://rsbuild.dev/config/server/proxy
- 8: https://dev.to/itays123/using-websockets-with-react-js-the-right-way-no-library-needed-15d0
- 9: https://learn.microsoft.com/en-us/aspnet/core/fundamentals/websockets?view=aspnetcore-9.0
- 10: https://www.npmjs.com/package/http-proxy-middleware
Action: Confirm WebSocket Proxy Setup for React Fast Refresh
The current configuration correctly enables WebSocket proxying with
ws: trueand appears to work for standard requests. However, since React Fast Refresh typically uses the/sockjs-nodeendpoint, please verify that upgrade events are properly handled on that path. If you encounter issues with hot module replacement, consider constraining the proxy to/sockjs-nodeand explicitly wiring up the HTTP upgrade events (e.g. usingserver.on('upgrade', proxy.upgrade)).
- File: packages/sync-server/src/app.js (Lines 74-86)
- Details:
- Existing config enables WebSocket support.
- Best practices suggest verifying that
/sockjs-nodeconnections are routed correctly for Fast Refresh.Please test both HTTP and WS connections in development to ensure full Fast Refresh compatibility.
packages/desktop-client/vite.config.mts (1)
138-149: LGTM! Well-structured conditional configuration.The server.open configuration correctly:
- Redirects to port 5006 in development mode
- Preserves existing browser selection logic
- Uses type-safe conditional expressions
|
saw your changes on your branch. better the |
Bundle Stats — desktop-clientHey there, this message comes from a GitHub action that helps you and reviewers to understand how these changes affect the size of this project's bundle. As this PR is updated, I'll keep you updated on how the bundle size is impacted. Total
Changeset
View detailed bundle breakdownAdded No assets were added Removed No assets were removed Bigger
Smaller No assets were smaller Unchanged
|
Bundle Stats — loot-coreHey there, this message comes from a GitHub action that helps you and reviewers to understand how these changes affect the size of this project's bundle. As this PR is updated, I'll keep you updated on how the bundle size is impacted. Total
Changeset No files were changed View detailed bundle breakdownAdded No assets were added Removed No assets were removed Bigger No assets were bigger Smaller No assets were smaller Unchanged
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/sync-server/src/app.js (1)
74-87: Consider improving the proxy configuration.
- The
logLevel: 'debug'setting might be too verbose for regular development. Consider using environment variables to control the log level.- Add error handling for proxy connection failures to improve developer experience.
Apply this diff to implement the improvements:
app.use( createProxyMiddleware({ target: 'http://localhost:3001', changeOrigin: true, ws: true, - logLevel: 'debug', + logLevel: process.env.PROXY_LOG_LEVEL || 'info', + onError: (err, req, res) => { + console.error('Proxy error:', err); + res.status(502).send('React development server is not running. Please start it with `yarn start` in a separate terminal.'); + }, }), );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
package.json(1 hunks)packages/desktop-client/vite.config.mts(2 hunks)packages/sync-server/src/app.js(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/desktop-client/vite.config.mts
- package.json
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: build (macos-latest)
- GitHub Check: build (windows-latest)
- GitHub Check: Analyze
- GitHub Check: Wait for Netlify build to finish
- GitHub Check: Build Docker image (alpine)
- GitHub Check: build (ubuntu-latest)
- GitHub Check: Build Docker image (ubuntu)
🔇 Additional comments (3)
packages/sync-server/src/app.js (3)
8-8: LGTM!The import statement is correctly placed and the package is used appropriately for development mode proxying.
88-92: LGTM!The production mode implementation follows best practices for serving a single-page application:
- Disables automatic index serving
- Correctly handles all routes for client-side routing
- Uses the configured web root path
28-37:❓ Verification inconclusive
Verify security implications of disabling rate limiting in development.
While disabling rate limiting in development mode improves the development experience, ensure that:
- Development servers are not exposed to the public internet
- Production deployments have
NODE_ENVcorrectly set
🏁 Script executed:
#!/bin/bash # Description: Check for potential security issues with NODE_ENV # Check if NODE_ENV is set in deployment configurations rg -A 5 "NODE_ENV.*production" --type yaml --type dockerfile # Check for any exposed development servers in deployment configurations rg -A 5 "port.*5006|port.*3001" --type yaml --type dockerfileLength of output: 196
Verify the Security of Disabling Rate Limiting in Development Mode
The current implementation enables rate limiting only when
process.env.NODE_ENVis not'development', which is acceptable if we ensure that development servers remain isolated and are never accidentally exposed. However, misconfigurations—such as a production deployment inadvertently running withNODE_ENV=development—could disable rate limiting and introduce security risks.Action Items:
- Confirm Environment Settings: Double-check that all production deployment configurations (including Dockerfiles and docker-compose files) explicitly set
NODE_ENVto'production'.- Ensure Development Isolation: Verify that any development server instances are not accessible from public networks.
To further assess the deployment configurations, please run a revised verification script (using corrected file type filters) similar to the example below:
#!/bin/bash # Attempt to locate production NODE_ENV assignments in deployment-related files # Search in Dockerfiles rg -A 5 "NODE_ENV.*production" -g "Dockerfile*" # Search in docker-compose YAML files rg -A 5 "NODE_ENV.*production" -g "docker-compose*.yml" # Optionally, check for potential exposed ports in docker-compose files rg -A 5 "port.*(5006|3001)" -g "docker-compose*.yml"Please manually verify the output of these commands and ensure that production deployments are correctly configured.
done |
|
I believe this PR broke my edge server: |
…ctualbudget#4372) * devmode for sync server * removed pluggy from this version * md * code review * changed how open browser * missed this * linter * trigger actions
I've fixed it here: #4400 It'll hopefully go to edge some time today. |
I added a new command
yarn start:server-dev: this command proxies calls to react from 5006 to port 3001 automatically. This way we can have the react fast refresh from the server port, not needing to connect from the 3001 port manually.