-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
feat(frontend): add configurable dev server port and DevTools #448
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,9 @@ | ||
| import { defineConfig, externalizeDepsPlugin } from 'electron-vite'; | ||
| import react from '@vitejs/plugin-react'; | ||
| import { resolve } from 'path'; | ||
| import { config as loadEnvFile } from 'dotenv'; | ||
|
|
||
| loadEnvFile({ quiet: true }); | ||
|
|
||
| export default defineConfig({ | ||
| main: { | ||
|
|
@@ -55,6 +58,7 @@ export default defineConfig({ | |
| } | ||
| }, | ||
| server: { | ||
| port: Number(process.env.VITE_DEV_PORT) || 5173, | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧹 Nitpick | 🔵 Trivial Port parsing handles common cases well. The
The implementation works well for development configuration. If you want to be defensive against edge cases (floats, out-of-range ports), you could add validation, but it's not critical since Vite will fail clearly if the port is invalid. Optional: Add port validation for edge cases server: {
- port: Number(process.env.VITE_DEV_PORT) || 5173,
+ port: (() => {
+ const port = Number(process.env.VITE_DEV_PORT);
+ if (!port || !Number.isInteger(port) || port < 1 || port > 65535) {
+ return 5173;
+ }
+ return port;
+ })(),This handles: non-integers (5173.5), out-of-range values, and ensures the port is valid before Vite starts. 🤖 Prompt for AI Agents |
||
| watch: { | ||
| // Ignore directories to prevent HMR conflicts during merge operations | ||
| // Using absolute paths and broader patterns | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -93,7 +93,7 @@ function createWindow(): void { | |||||
| } | ||||||
|
|
||||||
| // Open DevTools in development | ||||||
| if (is.dev) { | ||||||
| if (is.dev && process.env.OPEN_DEVTOOLS !== 'false') { | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧹 Nitpick | 🔵 Trivial Consider case-insensitive and additional falsy value support. The current check 🔎 Optional improvement for more flexible handling- if (is.dev && process.env.OPEN_DEVTOOLS !== 'false') {
+ if (is.dev && !['false', '0', 'no'].includes(process.env.OPEN_DEVTOOLS?.toLowerCase() ?? '')) {
mainWindow.webContents.openDevTools({ mode: 'right' });
}This handles 📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||
| mainWindow.webContents.openDevTools({ mode: 'right' }); | ||||||
| } | ||||||
|
|
||||||
|
|
||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧹 Nitpick | 🔵 Trivial
Documentation is clear and matches implementation.
The environment variable documentation accurately describes the defaults and usage. The examples correctly show how to configure the dev server port and disable DevTools.
Optional enhancement: Consider adding a note that
OPEN_DEVTOOLSis case-sensitive (only lowercase'false'disables DevTools):# Auto-open Chrome DevTools in dev mode (default: true) +# Note: Must be exactly 'false' (lowercase) to disable # OPEN_DEVTOOLS=false📝 Committable suggestion
🤖 Prompt for AI Agents