Fully rework the UI#1
Conversation
WalkthroughThis pull request refactors the Electron app from a Node.js-based architecture to a React-based application with webpack bundling. It disables ROS integration, introduces a unified window creation pattern, updates IPC naming conventions, and replaces the build pipeline with webpack and Babel tooling. Changes
Sequence DiagramsequenceDiagram
participant User as User (Touch)
participant Renderer as React Renderer
participant MainProcess as Main Process
participant IPC as IPC Bridge
User->>Renderer: Interacts with UI (button/PIN)
Renderer->>IPC: window.electronAPI.sendButtonAction(action)
IPC->>MainProcess: ipcMain 'button-action' event
MainProcess->>MainProcess: Update systemStatus field
MainProcess->>MainProcess: Publish to ROS (if available)
MainProcess->>IPC: mainWindow.webContents.send('update-display', msg)
IPC->>Renderer: electronAPI.onUpdateDisplay callback
Renderer->>Renderer: Update UI state
Renderer->>User: Reflect state change on screen
Note over MainProcess,Renderer: Initial app load (dev)
MainProcess->>MainProcess: Load dev server URL
MainProcess->>Renderer: Display webpack-served renderer
Note over MainProcess,Renderer: Production load
MainProcess->>MainProcess: Load dist/index.html
Renderer->>Renderer: Static bundled app
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Nitpick comments (13)
.gitignore (1)
15-16: Consider scoping build ignores"dist/" will match at any depth. If you only intend to ignore the renderer output, prefer adding "/test-app/dist/". Also consider ignoring typical Electron packaging outputs like "/test-app/out/" or "/test-app/release/" if you use electron-builder.
test-app/.babelrc (1)
1-3: Avoid Babel config duplicationYou define presets here and again in webpack’s babel-loader options. Keep one source of truth (prefer .babelrc) and drop the inline webpack options to prevent drift.
test-app/src/renderer/index.html (1)
7-15: Drop inline styles and add a basic CSPMove these styles into index.css and enable a CSP to harden the renderer. Example:
<head> <meta charset="UTF-8"> <meta name="viewport" content="width=device-width, initial-scale=1.0, user-scalable=no, maximum-scale=1.0, minimum-scale=1.0"> + <meta http-equiv="Content-Security-Policy" content="default-src 'self'; img-src 'self' data:; script-src 'self'; style-src 'self' 'unsafe-inline'"> <title>Wheelchair Control System</title> - <style> - /* Prevent any default input behaviors */ - body { - -webkit-user-select: none; - -webkit-touch-callout: none; - -webkit-tap-highlight-color: transparent; - overscroll-behavior: contain; - } - </style> </head>Note:
'unsafe-inline'is included to support injected CSS via style-loader; if you later switch to extracted CSS, this can be tightened. Also, confirm that disabling zoom in the viewport is a product requirement.test-app/src/renderer/index.jsx (1)
6-8: Guard against missing #rootAdd a null check before createRoot to fail fast with a clear message.
-const container = document.getElementById('root'); -const root = createRoot(container); +const container = document.getElementById('root'); +if (!container) throw new Error("Renderer bootstrap failed: #root not found"); +const root = createRoot(container); root.render(<App />);test-app/webpack.config.js (1)
15-23: De-duplicate Babel config and enable cacheRely on .babelrc and enable loader caching for faster builds.
{ test: /\.(js|jsx)$/, exclude: /node_modules/, use: { loader: 'babel-loader', - options: { - presets: ['@babel/preset-env', '@babel/preset-react'], - }, + options: { cacheDirectory: true }, }, },test-app/src/renderer/components/MainScreen.jsx (3)
89-94: Fix status color semantics (Brake ON should not be green)Make color dependent on the status key to avoid misleading signals (e.g., brake engaged = red).
- const getStatusColor = (status) => { - if (status === 'ON' || status === 'STEADY' || status === 'FLASHING' || status === 'FULL' || status === 'JOYSTICK') { - return '#22c55e'; - } - return '#6b7280'; - }; + const getStatusColor = (key, value) => { + if (key === 'brake') return value === 'ON' ? '#dc2626' : '#22c55e'; + const good = new Set(['ON', 'STEADY', 'FLASHING', 'FULL', 'JOYSTICK']); + return good.has(value) ? '#22c55e' : '#6b7280'; + }; @@ - <div style={{ ...statusValueStyle, color: getStatusColor(status.brake) }}>{status.brake}</div> + <div style={{ ...statusValueStyle, color: getStatusColor('brake', status.brake) }}>{status.brake}</div> @@ - <div style={{ ...statusValueStyle, color: getStatusColor(status.lidar) }}>{status.lidar}</div> + <div style={{ ...statusValueStyle, color: getStatusColor('lidar', status.lidar) }}>{status.lidar}</div> @@ - <div style={{ ...statusValueStyle, color: getStatusColor(status.selfdrive) }}>{status.selfdrive}</div> + <div style={{ ...statusValueStyle, color: getStatusColor('selfdrive', status.selfdrive) }}>{status.selfdrive}</div> @@ - <div style={{ ...statusValueStyle, color: getStatusColor(status.light) }}>{status.light}</div> + <div style={{ ...statusValueStyle, color: getStatusColor('light', status.light) }}>{status.light}</div>Also applies to: 136-137, 140-141, 144-145, 152-153
96-113: Remove unused prop from TouchButton
activeColorisn’t used. Drop it (or implement it).- const TouchButton = ({ onClick, style, children, activeColor }) => { + const TouchButton = ({ onClick, style, children }) => {
40-44: Gracefully handle missing electronAPI in devOptional: log a warning when electronAPI is absent to aid debugging.
const handleButtonClick = (action) => { - if (window.electronAPI) { - window.electronAPI.sendButtonAction(action); - } + if (window.electronAPI?.sendButtonAction) { + window.electronAPI.sendButtonAction(action); + } else { + console.warn('electronAPI.sendButtonAction is unavailable'); + } };test-app/src/renderer/components/LockScreen.jsx (1)
36-56: TouchButton: fix style override order and add pointer cancel; avoid duplication
- Move
...styleout ofbaseStyleand spread it last in the buttonstyleso callers can override width, height, etc.- Add
onPointerCancelto reset pressed state.- This component duplicates the
TouchButtonin MainScreen.jsx; extract a shared component tocomponents/TouchButton.jsx.Apply:
- const baseStyle = { + const baseStyle = { width: "85px", height: "85px", borderRadius: "20px", border: "none", fontSize: "28px", fontWeight: "600", cursor: "pointer", display: "flex", alignItems: "center", justifyContent: "center", transition: "all 0.2s cubic-bezier(0.4, 0, 0.2, 1)", userSelect: "none", touchAction: "manipulation", boxShadow: "0 4px 12px rgba(0, 0, 0, 0.15)", - ...style, }; @@ <button style={{ ...baseStyle, ...variants[variant], + ...style, transform: isPressed ? "scale(0.95) translateY(2px)" : "scale(1)", boxShadow: isPressed ? "0 2px 8px rgba(0, 0, 0, 0.2)" : "0 4px 12px rgba(0, 0, 0, 0.15)", }} onPointerDown={() => setIsPressed(true)} onPointerUp={() => setIsPressed(false)} onPointerLeave={() => setIsPressed(false)} + onPointerCancel={() => setIsPressed(false)} onClick={onClick} >Also applies to: 73-91
test-app/package.json (1)
27-27: Move material-icons to dependencies if used at runtimeIf imported in renderer for styles/fonts, it must be a runtime dependency; Electron packaging often excludes devDependencies.
Would you like me to move it and adjust lockfile?
test-app/src/main.js (3)
150-172: Harden shutdown when ROS is disabledGuard accesses to undeclared/absent modules to avoid noisy errors; prefer feature detection.
Apply:
- if (node) { + if (typeof node !== "undefined" && node && typeof node.destroy === "function") { try { node.destroy(); } catch (err) { console.warn("Error destroying ROS node:", err); } } @@ - try { - await rclnodejs.shutdown(); - } catch (err) { - console.warn("Error during ROS shutdown:", err); - } + try { + if (typeof rclnodejs !== "undefined" && typeof rclnodejs.shutdown === "function") { + await rclnodejs.shutdown(); + } + } catch (err) { + console.warn("Error during ROS shutdown:", err); + }
140-147: Clarify close semantics; prevent unintended quitsIf you intend to keep the window alive when
actualShutdown === false, callevent.preventDefault()and handle accordingly; otherwise remove the flag.Apply:
mainWindow.on("close", (event) => { - if (!actualShutdown) { - return; - } - - shutdownApp(); + if (!actualShutdown) { + event.preventDefault(); + return; + } + shutdownApp(); });
176-207: Use strict equality for IPC action/password checks; push status after updatesPrefer
===to avoid surprises; after mutatingsystemStatus, consider callingsendStatusToUI()to keep UI in sync even if no ROS publishers exist.Apply (illustrative subset):
- if (action == "LOCK") { + if (action === "LOCK") { @@ - if ( - action == "LIGHT_FLASHING" || - action == "LIGHT_STEADY" || - action == "LIGHT_OFF" - ) { + if ( + action === "LIGHT_FLASHING" || + action === "LIGHT_STEADY" || + action === "LIGHT_OFF" + ) { @@ - if (action == "BRAKE_ON" || action == "BRAKE_OFF") { + if (action === "BRAKE_ON" || action === "BRAKE_OFF") { @@ - if (action == "LIDAR_ON" || action == "LIDAR_OFF") { + if (action === "LIDAR_ON" || action === "LIDAR_OFF") { @@ - } else if (action == "SELFDRIVE_JOYSTICK") { + } else if (action === "SELFDRIVE_JOYSTICK") { @@ - } else if (action == "SELFDRIVE_OFF") { + } else if (action === "SELFDRIVE_OFF") {Optional after any state change:
+ sendStatusToUI();And for password:
- if (password == ACTUAL_PASSWORD) { + if (password === ACTUAL_PASSWORD) {Also applies to: 209-215, 219-244, 263-279
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
.gitignore(1 hunks)test-app/.babelrc(1 hunks)test-app/package.json(1 hunks)test-app/src/lock_screen.html(0 hunks)test-app/src/main.js(4 hunks)test-app/src/preload.js(1 hunks)test-app/src/renderer/App.jsx(1 hunks)test-app/src/renderer/components/LockScreen.jsx(1 hunks)test-app/src/renderer/components/MainScreen.jsx(1 hunks)test-app/src/renderer/index.css(1 hunks)test-app/src/renderer/index.html(1 hunks)test-app/src/renderer/index.jsx(1 hunks)test-app/src/style.css(0 hunks)test-app/webpack.config.js(1 hunks)
💤 Files with no reviewable changes (2)
- test-app/src/lock_screen.html
- test-app/src/style.css
🧰 Additional context used
🧬 Code graph analysis (5)
test-app/webpack.config.js (2)
test-app/src/main.js (2)
require(1-1)path(2-2)test-app/src/preload.js (1)
require(2-2)
test-app/src/renderer/components/MainScreen.jsx (1)
test-app/src/renderer/components/LockScreen.jsx (1)
TouchButton(36-91)
test-app/src/preload.js (1)
test-app/src/main.js (1)
require(1-1)
test-app/src/renderer/components/LockScreen.jsx (1)
test-app/src/renderer/components/MainScreen.jsx (1)
TouchButton(96-113)
test-app/src/main.js (1)
test-app/src/preload.js (1)
require(2-2)
🔇 Additional comments (2)
test-app/src/renderer/components/LockScreen.jsx (1)
93-121: Verify backdrop blur with GPU disabledMain process calls
app.disableHardwareAcceleration()(test-app/src/main.js Line 4). Backdrop filters may degrade or not render. Verify visuals on target hardware.test-app/package.json (1)
12-14: Versions are compatible—no changes requiredReact 19.1.1 is generally compatible with react-router-dom 7.8.2, as React Router v7 targets React 18+ and runs on React 19. babel-loader@10 is compatible with webpack@5.101.3, as v10.x supports webpack ^5.61.0. Monitor for any peer-dependency warnings from packages with outdated React peerDependencies, and watch the React Router changelog for any React 19-specific issues in v7.x releases.
| "start": "electron .", | ||
| "build": "webpack --mode production", | ||
| "dev": "webpack --mode production && electron ." |
There was a problem hiding this comment.
Fix dev workflow: use webpack-dev-server and set NODE_ENV
Current dev builds production and doesn’t start a dev server; NODE_ENV remains unset so main won’t load http://localhost:8080.
Apply:
"scripts": {
- "start": "electron .",
- "build": "webpack --mode production",
- "dev": "webpack --mode production && electron ."
+ "start": "electron .",
+ "build": "webpack --mode production",
+ "dev": "cross-env NODE_ENV=development concurrently \"webpack serve --mode development\" \"wait-on http://localhost:8080 && electron .\""
},And add cross-env:
"devDependencies": {
+ "cross-env": "^7.0.3",
"@babel/core": "^7.28.3",Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In test-app/package.json around lines 6 to 8, the "dev" script currently runs a
production build and doesn't start a dev server or set NODE_ENV; change the
"dev" script to run webpack-dev-server (so the renderer is served at
http://localhost:8080) and launch Electron after the server starts, and prefix
the command with cross-env NODE_ENV=development so the main process loads the
dev URL; also add cross-env as a devDependency (or document it in install steps)
so the environment variable works cross-platform.
| // let node = null; | ||
| let publisher_light, publisher_brake, publisher_lidar, publisher_selfdrive; |
There was a problem hiding this comment.
Fix undeclared identifiers: node, publisher_room, and sloppy temp
These cause runtime ReferenceErrors (and global leakage for temp).
Apply:
-// let node = null;
-let publisher_light, publisher_brake, publisher_lidar, publisher_selfdrive;
+let node = null;
+let publisher_light, publisher_brake, publisher_lidar, publisher_selfdrive;
+let publisher_room;
@@
- if (action.startsWith("ROOM_")) {
- temp = action.substring(action.indexOf("_") + 1);
- const msg = { data: temp };
- if (publisher_room) {
+ if (action.startsWith("ROOM_")) {
+ const room = action.substring(action.indexOf("_") + 1);
+ const msg = { data: room };
+ if (publisher_room) {
publisher_room.publish(msg);
console.log(`---> Published from Electron app: ${msg.data}`);
}
}Also applies to: 246-254
| function createWindow() { | ||
| mainWindow = new BrowserWindow({ | ||
| width: 970, | ||
| height: 750, | ||
| autoHideMenuBar: true, | ||
| webPreferences: { | ||
| preload: path.join(__dirname, 'preload.js'), | ||
| preload: path.join(__dirname, "preload.js"), | ||
| contextIsolation: true, | ||
| nodeIntegration: false | ||
| } | ||
| nodeIntegration: false, | ||
| }, | ||
| }); | ||
|
|
||
| lockWindow.loadFile(path.join(__dirname, 'lock_screen.html')); | ||
| lockWindow.maximize(); | ||
|
|
||
| lockWindow.on('close', (event) => { | ||
| if (!actualShutdown) { | ||
| return; | ||
| const isDev = process.env.NODE_ENV === 'development'; | ||
| if (isDev) { | ||
| mainWindow.loadURL('http://localhost:8080'); | ||
| } else { | ||
| mainWindow.loadFile(path.join(__dirname, '../dist/index.html')); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Dev/prod loader split relies on NODE_ENV
This will only use the dev server when NODE_ENV=development. The package.json change I suggested wires this correctly. Please test both paths.
Run:
🏁 Script executed:
#!/bin/bash
# Verify dev path hits http://localhost:8080
cross-env NODE_ENV=development electron .
# Verify prod path loads dist
NODE_ENV=production electron .Length of output: 94
🏁 Script executed:
#!/bin/bash
# Check package.json for NODE_ENV setup and scripts
cat package.json | grep -A 10 -B 2 "NODE_ENV\|scripts"Length of output: 44
🏁 Script executed:
#!/bin/bash
# Verify preload.js exists at the referenced path
fd -type f preload.jsLength of output: 170
🏁 Script executed:
#!/bin/bash
# Check main.js imports and path setup for context
head -20 test-app/src/main.jsLength of output: 598
🏁 Script executed:
#!/bin/bash
# Find package.json
fd -type f package.jsonLength of output: 170
🏁 Script executed:
#!/bin/bash
# Verify preload.js with correct syntax
fd -type f -name preload.jsLength of output: 170
🏁 Script executed:
#!/bin/bash
# Get the full createWindow function (lines 120-137)
sed -n '120,137p' test-app/src/main.jsLength of output: 488
🏁 Script executed:
#!/bin/bash
# Check if dist directory structure exists
fd -type d -name dist test-app/Length of output: 170
🏁 Script executed:
#!/bin/bash
# Use find instead of fd - locate package.json
find . -name package.json 2>/dev/nullLength of output: 23
🏁 Script executed:
#!/bin/bash
# Check test-app directory structure
ls -la test-app/Length of output: 351
🏁 Script executed:
#!/bin/bash
# Look for preload.js using find
find test-app/ -name preload.js 2>/dev/nullLength of output: 23
🏁 Script executed:
#!/bin/bash
# Check if dist directory exists or would be created
find test-app/ -type d -name dist 2>/dev/nullLength of output: 0
🏁 Script executed:
#!/bin/bash
# Check test-app/src directory
ls -la test-app/src/Length of output: 684
🏁 Script executed:
#!/bin/bash
# Read package.json to check scripts and NODE_ENV setup
cat test-app/package.jsonLength of output: 934
🏁 Script executed:
#!/bin/bash
# Read webpack.config.js to verify dist output path
cat test-app/webpack.config.jsLength of output: 1001
🏁 Script executed:
#!/bin/bash
# Check if there's a build directory or dist built elsewhere
find test-app/ -type d | head -20Length of output: 77
Dev/prod split cannot work: NODE_ENV not set in package.json scripts
The main.js logic is correct, but package.json scripts don't set NODE_ENV:
- "dev" and "build" scripts lack
NODE_ENV=development/NODE_ENV=productionconfiguration - webpack.config.js also depends on NODE_ENV being set but receives no value from npm scripts
- Result: isDev will always be false, and the app always loads dist even when running
npm run dev
The manual test commands you requested (with cross-env NODE_ENV=development/production) would work, but users running npm run dev would skip the dev server entirely.
Update package.json scripts to set NODE_ENV, e.g.:
"dev": "cross-env NODE_ENV=development webpack-dev-server ...",
"build": "cross-env NODE_ENV=production webpack --mode production"🤖 Prompt for AI Agents
In test-app/src/main.js around lines 120 to 137, the environment check using
process.env.NODE_ENV will always be false because package.json scripts do not
set NODE_ENV; update package.json scripts to explicitly set NODE_ENV for dev and
build (use cross-env for cross-platform compatibility), e.g. set dev to run with
NODE_ENV=development and start the dev server (webpack-dev-server) and set build
to NODE_ENV=production and run webpack in production mode; also add cross-env as
a devDependency (or document alternative) so running npm run dev and npm run
build actually toggles isDev as intended.
| onUpdateDisplay: (callback) => ipcRenderer.on('update-display', (_, msg) => callback(msg)), | ||
|
|
There was a problem hiding this comment.
Prevent listener leaks; return unsubscriber and type-check callback
Repeated mounts will accumulate ipcRenderer.on handlers.
Apply:
- onUpdateDisplay: (callback) => ipcRenderer.on('update-display', (_, msg) => callback(msg)),
+ onUpdateDisplay: (callback) => {
+ if (typeof callback !== 'function') return () => {};
+ const listener = (_evt, msg) => callback(msg);
+ ipcRenderer.on('update-display', listener);
+ return () => ipcRenderer.removeListener('update-display', listener);
+ },📝 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.
| onUpdateDisplay: (callback) => ipcRenderer.on('update-display', (_, msg) => callback(msg)), | |
| onUpdateDisplay: (callback) => { | |
| if (typeof callback !== 'function') return () => {}; | |
| const listener = (_evt, msg) => callback(msg); | |
| ipcRenderer.on('update-display', listener); | |
| return () => ipcRenderer.removeListener('update-display', listener); | |
| }, |
🤖 Prompt for AI Agents
In test-app/src/preload.js around lines 11 to 12, the exported onUpdateDisplay
attaches an ipcRenderer.on listener without removing it and does not validate
the callback; change it to validate that callback is a function before
registering, add the listener as a named function, and return an unsubscriber
function that removes that listener (using ipcRenderer.removeListener or
ipcRenderer.off) so repeated mounts do not leak handlers.
| useEffect(() => { | ||
| if (window.electronAPI) { | ||
| // Listen for navigation events from main process | ||
| window.electronAPI.onUpdateDisplay((message) => { | ||
| // Check if we should navigate to main screen | ||
| if (message === 'NAVIGATE_TO_MAIN') { | ||
| setIsUnlocked(true); | ||
| } else if (message === 'LOCK') { | ||
| setIsUnlocked(false); | ||
| } | ||
| }); | ||
| } | ||
| }, []); |
There was a problem hiding this comment.
Event listener leak: add cleanup for electronAPI subscription
The effect registers a listener but never removes it. In dev/Hot Reload this stacks handlers and can cause duplicate updates.
useEffect(() => {
- if (window.electronAPI) {
- // Listen for navigation events from main process
- window.electronAPI.onUpdateDisplay((message) => {
- // Check if we should navigate to main screen
- if (message === 'NAVIGATE_TO_MAIN') {
- setIsUnlocked(true);
- } else if (message === 'LOCK') {
- setIsUnlocked(false);
- }
- });
- }
+ if (!window.electronAPI?.onUpdateDisplay) return;
+ const handler = (message) => {
+ if (message === 'NAVIGATE_TO_MAIN') setIsUnlocked(true);
+ else if (message === 'LOCK') setIsUnlocked(false);
+ };
+ const unsubscribe = window.electronAPI.onUpdateDisplay(handler);
+ return () => {
+ if (typeof unsubscribe === 'function') unsubscribe();
+ else window.electronAPI?.offUpdateDisplay?.(handler);
+ };
}, []);🤖 Prompt for AI Agents
In test-app/src/renderer/App.jsx around lines 8 to 20, the useEffect registers
window.electronAPI.onUpdateDisplay but never removes the listener, causing
handler leaks on hot reload; update the effect to store the handler (or capture
the unsubscribe returned by onUpdateDisplay) and return a cleanup function that
removes the listener—either call the unsubscribe function returned by
onUpdateDisplay or invoke the appropriate removal API on window.electronAPI
(e.g., off/removeListener/removeOnUpdateDisplay) to unregister the same handler
when the component unmounts or effect re-runs.
| const submitPin = () => { | ||
| if (pin.length === 4) { | ||
| if (window.electronAPI) { | ||
| window.electronAPI.sendPassword(parseInt(pin)); | ||
| } | ||
| clearPin(); | ||
| } else if (pin.length > 0) { | ||
| // Add shake animation for incomplete PIN | ||
| setIsShaking(true); | ||
| setTimeout(() => setIsShaking(false), 500); | ||
| } | ||
| }; |
There was a problem hiding this comment.
Specify radix; consider preserving leading zeros
parseInt(pin) should include a radix; also converting to number strips leading zeros.
Apply:
- window.electronAPI.sendPassword(parseInt(pin));
+ window.electronAPI.sendPassword(parseInt(pin, 10));Optionally send as a string to preserve leading zeros and compare as string in main: sendPassword(pin).
📝 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 submitPin = () => { | |
| if (pin.length === 4) { | |
| if (window.electronAPI) { | |
| window.electronAPI.sendPassword(parseInt(pin)); | |
| } | |
| clearPin(); | |
| } else if (pin.length > 0) { | |
| // Add shake animation for incomplete PIN | |
| setIsShaking(true); | |
| setTimeout(() => setIsShaking(false), 500); | |
| } | |
| }; | |
| const submitPin = () => { | |
| if (pin.length === 4) { | |
| if (window.electronAPI) { | |
| window.electronAPI.sendPassword(parseInt(pin, 10)); | |
| } | |
| clearPin(); | |
| } else if (pin.length > 0) { | |
| // Add shake animation for incomplete PIN | |
| setIsShaking(true); | |
| setTimeout(() => setIsShaking(false), 500); | |
| } | |
| }; |
🤖 Prompt for AI Agents
In test-app/src/renderer/components/LockScreen.jsx around lines 19 to 30, the
code uses parseInt(pin) without a radix and converting to a number strips
leading zeros; update the send to preserve leading zeros by sending the PIN as a
string (window.electronAPI.sendPassword(pin)) or, if you must parse, call
parseInt(pin, 10) and only use that when numeric comparison is required—prefer
sending the string and change the main process to compare as a string.
| const [display, setDisplay] = useState('System Ready'); | ||
| const [status, setStatus] = useState({ |
There was a problem hiding this comment.
Remove unused display state and add proper unsubscribe
display isn’t rendered; keeping it triggers unnecessary re-renders. Also, add cleanup for the IPC listener.
- const [display, setDisplay] = useState('System Ready');
const [status, setStatus] = useState({
@@
- useEffect(() => {
- if (window.electronAPI) {
- window.electronAPI.onUpdateDisplay((message) => {
- setDisplay(message);
-
- // Update individual statuses based on the message
+ useEffect(() => {
+ if (!window.electronAPI?.onUpdateDisplay) return;
+ const handler = (message) => {
+ // Update individual statuses based on the message
if (message.startsWith('LIGHT_')) {
setStatus(prev => ({ ...prev, light: message.replace('LIGHT_', '') }));
} else if (message.startsWith('BRAKE_')) {
setStatus(prev => ({ ...prev, brake: message.replace('BRAKE_', '') }));
} else if (message.startsWith('LIDAR_')) {
setStatus(prev => ({ ...prev, lidar: message.replace('LIDAR_', '') }));
} else if (message.startsWith('SELFDRIVE_')) {
setStatus(prev => ({ ...prev, selfdrive: message.replace('SELFDRIVE_', '') }));
} else if (message.startsWith('BATTERY_')) {
setStatus(prev => ({ ...prev, battery: message.replace('BATTERY_', '') }));
} else if (message.startsWith('SPEED_')) {
setStatus(prev => ({ ...prev, speed: message.replace('SPEED_', '') }));
} else if (message.startsWith('ROOM_')) {
setStatus(prev => ({ ...prev, room: message.replace('ROOM_', '') }));
}
- });
- }
+ };
+ const unsubscribe = window.electronAPI.onUpdateDisplay(handler);
+ return () => {
+ if (typeof unsubscribe === 'function') unsubscribe();
+ else window.electronAPI?.offUpdateDisplay?.(handler);
+ };
}, []);Also applies to: 15-38
🤖 Prompt for AI Agents
In test-app/src/renderer/components/MainScreen.jsx around lines 4-5 (and
similarly lines 15-38), remove the unused display state variable and its setter
to avoid unnecessary re-renders; then update the IPC listener setup so you
capture the subscription reference (or the handler) and return a cleanup
function from useEffect that removes/unsubscribes the listener when the
component unmounts to prevent leaks and duplicate handlers.
| useEffect(() => { | ||
| if (window.electronAPI) { | ||
| window.electronAPI.onUpdateDisplay((message) => { | ||
| setDisplay(message); | ||
|
|
||
| // Update individual statuses based on the message | ||
| if (message.startsWith('LIGHT_')) { | ||
| setStatus(prev => ({ ...prev, light: message.replace('LIGHT_', '') })); | ||
| } else if (message.startsWith('BRAKE_')) { | ||
| setStatus(prev => ({ ...prev, brake: message.replace('BRAKE_', '') })); | ||
| } else if (message.startsWith('LIDAR_')) { | ||
| setStatus(prev => ({ ...prev, lidar: message.replace('LIDAR_', '') })); | ||
| } else if (message.startsWith('SELFDRIVE_')) { | ||
| setStatus(prev => ({ ...prev, selfdrive: message.replace('SELFDRIVE_', '') })); | ||
| } else if (message.startsWith('BATTERY_')) { | ||
| setStatus(prev => ({ ...prev, battery: message.replace('BATTERY_', '') })); | ||
| } else if (message.startsWith('SPEED_')) { | ||
| setStatus(prev => ({ ...prev, speed: message.replace('SPEED_', '') })); | ||
| } else if (message.startsWith('ROOM_')) { | ||
| setStatus(prev => ({ ...prev, room: message.replace('ROOM_', '') })); | ||
| } | ||
| }); | ||
| } | ||
| }, []); |
There was a problem hiding this comment.
🧩 Analysis chain
Verify preload exposes an unsubscribe/off API
To support the cleanup above, ensure preload exposes either a returned unsubscribe function or an offUpdateDisplay method.
🏁 Script executed:
#!/bin/bash
# Inspect preload exposure to confirm available unsubscribe/off
rg -n -C2 --glob "!**/node_modules/**" "contextBridge|exposeInMainWorld|onUpdateDisplay|offUpdateDisplay|ipcRenderer\.on|ipcRenderer\.removeListener" test-appLength of output: 1804
🏁 Script executed:
#!/bin/bash
# Check the full useEffect structure including any cleanup logic
rg -n -A15 "useEffect\(\(\)" test-app/src/renderer/components/MainScreen.jsx | head -30Length of output: 926
🏁 Script executed:
#!/bin/bash
# Get lines 15-40 to see the complete useEffect structure
sed -n '15,40p' test-app/src/renderer/components/MainScreen.jsxLength of output: 1220
Add cleanup mechanism to preload API and useEffect
The preload API does not expose an unsubscribe function or offUpdateDisplay method. The onUpdateDisplay call at line 17 creates a persistent listener via ipcRenderer.on() without removal capability, causing listener accumulation on each component mount.
Fix required:
- Expose an
offUpdateDisplaymethod intest-app/src/preload.jsusingipcRenderer.removeListener() - Add cleanup logic (return statement) to the useEffect in
test-app/src/renderer/components/MainScreen.jsxto unsubscribe on unmount
🤖 Prompt for AI Agents
In test-app/src/renderer/components/MainScreen.jsx around lines 15 to 38, the
useEffect registers a persistent listener via window.electronAPI.onUpdateDisplay
but does not unsubscribe on unmount; update the effect to capture the callback
in a const, call window.electronAPI.offUpdateDisplay(callback) in the cleanup
return so the listener is removed when the component unmounts, and ensure you
pass the same function reference to offUpdateDisplay; additionally modify
test-app/src/preload.js to export an offUpdateDisplay method that calls
ipcRenderer.removeListener(channel, listener) for the same channel used by
onUpdateDisplay so the renderer can remove the exact listener.
| /* Disable all keyboard input globally */ | ||
| input, textarea, select { | ||
| pointer-events: none; | ||
| -webkit-user-select: none; | ||
| -moz-user-select: none; | ||
| user-select: none; | ||
| } |
There was a problem hiding this comment.
Blocking all input controls breaks accessibility and usability
Disabling pointer events on inputs/textareas/selects prevents any text entry and violates basic accessibility. Scope this behavior to a specific kiosk container or remove it.
-/* Disable all keyboard input globally */
-input, textarea, select {
- pointer-events: none;
- -webkit-user-select: none;
- -moz-user-select: none;
- user-select: none;
-}
+/* Avoid globally blocking form controls; if needed, scope under a kiosk class */
+/* .kiosk input, .kiosk textarea, .kiosk select { pointer-events: none; } */🤖 Prompt for AI Agents
In test-app/src/renderer/index.css around lines 20 to 26, the global rule
disabling pointer-events and user-select on input, textarea, and select breaks
accessibility; restrict this behavior to a specific kiosk container (e.g. change
the selector to .kiosk-container input, .kiosk-container textarea,
.kiosk-container select) or remove the pointer-events/user-select rules entirely
so form controls remain interactive and keyboard-accessible; if you need to
prevent pointer interaction only in kiosk mode, scope the styles to a
kiosk-specific class or ID and ensure keyboard focus/aria attributes remain
functional.
| test: /\.css$/, | ||
| use: ['style-loader', 'css-loader', 'postcss-loader'], | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
Confirm PostCSS usage or remove the loader
If there’s no postcss.config.js or plugins (e.g., autoprefixer), drop postcss-loader to save work in the pipeline, or add a config.
🏁 Script executed:
#!/bin/bash
# Verify presence of PostCSS config or plugins
fd -H --glob "postcss.config.js"
rg -n --glob "!**/node_modules/**" -i "postcss|autoprefixer"Length of output: 256
---
🏁 Script executed:
```shell
#!/bin/bash
# Check full postcss-loader configuration in webpack.config.js
rg -n -A5 -B5 "postcss-loader" test-app/webpack.config.js
Length of output: 227
Add PostCSS configuration or remove postcss-loader from the build pipeline
PostCSS is configured to load but has no postcss.config.js file or inline options. The installed autoprefixer dependency cannot be applied. Either create a PostCSS configuration file with your desired plugins (e.g., autoprefixer), or remove postcss-loader from the loader chain if PostCSS processing isn't needed.
🤖 Prompt for AI Agents
In test-app/webpack.config.js around lines 26 to 28, the webpack rule includes
postcss-loader but there is no PostCSS configuration, so autoprefixer won't be
applied; either add a PostCSS configuration file (postcss.config.js) exporting
the plugins array (e.g., require('autoprefixer') in plugins) or add inline
options to the loader rule specifying the plugins, and ensure autoprefixer is
installed and versions are compatible with postcss-loader; alternatively, if
PostCSS processing is not required, remove 'postcss-loader' from the use array
to stop webpack from attempting to run PostCSS.
1 participant
Summary by CodeRabbit
New Features
Bug Fixes
Chores