-
Notifications
You must be signed in to change notification settings - Fork 5
RD-T39 Working on electron build #90
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
Conversation
📝 WalkthroughWalkthroughUpdates Expo web config to emit a static web build with base URL '/', and refactors Electron main process to explicitly branch on development vs production: dev loads the localhost URL, prod loads the local index.html file; devtools open only in development. Changes
Sequence Diagram(s)sequenceDiagram
participant DevServer as Dev Server
participant Main as Electron Main
participant Browser as BrowserWindow
participant FS as File System
Note over Main: Startup
Main->>Main: determine isDev
alt isDev == true
Main->>DevServer: loadURL('http://localhost:8081')
DevServer-->>Browser: serve app over HTTP
Main->>Browser: open DevTools
else isDev == false
Main->>FS: resolve index.html path
Main->>Browser: loadFile(index.html)
FS-->>Browser: provide local file
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
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.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@app.config.ts`:
- Around line 88-93: The app config currently places baseUrl inside the web
block; move the baseUrl setting into an experiments object (experiments.baseUrl)
and remove baseUrl from the web object, and use an absolute path (e.g., "/" or
"/myapp") or an empty string instead of a relative './' value; update the config
to define experiments with baseUrl and keep web containing favicon, bundler and
output only (referencing the symbols baseUrl, experiments, and web to locate the
change).
🧹 Nitpick comments (1)
electron/main.js (1)
22-29: Harden production index path resolution.Line 26 relies on a relative path from
__dirname; packaged layouts can vary. Please verify the built web assets land at that path in your packaged app; otherwise resolve fromapp.getAppPath()and guard missing files to fail fast.♻️ Suggested guard + path resolution
-const path = require('path'); +const path = require('path'); +const fs = require('fs'); ... - const indexPath = path.join(__dirname, '../dist/index.html'); + const indexPath = path.join(app.getAppPath(), 'dist', 'index.html'); + if (!fs.existsSync(indexPath)) { + console.error(`Missing web build at ${indexPath}`); + } console.log('Loading file:', indexPath); mainWindow.loadFile(indexPath);
|
Approve |
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.
This PR is approved.
Summary by CodeRabbit