feat: enhance led strip#102
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
Pull request overview
This pull request enhances the LED strip functionality and introduces a robust system for managing microcontroller board connections and port events in the Electron app. The changes improve device detection, error handling, and user experience while adding new preset management features for LED strips.
Key changes:
- Refactored board connection logic into dedicated modules (
board-connection.ts,port-manager.ts) with improved error handling for port disconnections - Enhanced LED strip functionality with preset patterns, bidirectional movement (
movemethod), and a visual preset editor UI - Added device event listeners and polling mechanisms for reliable USB/serial port detection and management
Reviewed changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated 12 comments.
Show a summary per file
| File | Description |
|---|---|
packages/runtime/test.ts |
Updated test to use new move() method instead of forward() |
packages/runtime/src/pixel/pixel.types.ts |
Added hex color validation regex, presets array schema, and default color constant |
packages/runtime/src/pixel/pixel.ts |
Implemented move() and show() methods, added flush throttling, improved component initialization |
packages/runtime/src/pixel/pixel.constants.ts |
Added DEFAULT_OFF_PIXEL_COLOR constant |
apps/electron-app/src/render/components/react-flow/nodes/pixel/PixelEditor.tsx |
New component for editing LED strip presets with color picker UI |
apps/electron-app/src/render/components/react-flow/nodes/pixel/PixelDisplay.tsx |
Refactored from Pixel.tsx to display LED strips with optional interactivity |
apps/electron-app/src/render/components/react-flow/nodes/pixel/Pixel.tsx |
Reorganized Pixel node with preset management UI and updated handles |
apps/electron-app/src/render/hooks/useFlowSync.ts |
Added toast error notification for board connection failures |
apps/electron-app/src/main/window.ts |
Extracted window dimensions to constants and reduced window recreation delay |
apps/electron-app/src/main/utils.ts |
Added Timer utility class for performance measurement |
apps/electron-app/src/main/port-manager.ts |
New module for managing connected ports, polling, and USB device event listeners |
apps/electron-app/src/main/ipc.ts |
Refactored to use new board-connection and port-manager modules |
apps/electron-app/src/main/board-connection.ts |
New module centralizing board connection logic with port disconnection error handling |
apps/electron-app/src/common/nodes.ts |
Updated import path for Pixel component |
apps/electron-app/vite.renderer.config.mjs |
Removed redundant optimizeDeps.exclude configuration |
apps/electron-app/vite.main.config.mjs |
Added unused projectRootDir variable |
apps/electron-app/package.json |
Added clean:vite script for development cleanup |
package.json |
Added flasher package to concurrent dev script |
Comments suppressed due to low confidence (5)
apps/electron-app/src/render/components/react-flow/nodes/pixel/PixelDisplay.tsx:166
- Inconsistent terminology: the tooltip uses "Pixel" when clickable (line 156) but "LED" when not clickable (line 166). For consistency, use the same terminology in both cases, preferably "LED" to match the label at line 173.
apps/electron-app/src/render/hooks/useFlowSync.ts:1 - Unused import useMemo.
import { useCallback, useEffect, useMemo, useRef } from 'react';
apps/electron-app/src/render/hooks/useFlowSync.ts:5
- Unused import useBoardPort.
import { useBoardPort, useBoardStore, useBoard } from '../stores/board';
apps/electron-app/vite.main.config.mjs:7
- Unused variable projectRootDir.
const projectRootDir = path.resolve(__dirname);
packages/runtime/test.ts:15
- Unused function getBoard.
function getBoard() {
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| const root = '.vite'; | ||
| const build = `${root}/build`; | ||
| const projectRootDir = path.resolve(__dirname); |
There was a problem hiding this comment.
The projectRootDir variable is defined but never used in this configuration file. Consider removing it to clean up unused code.
| const projectRootDir = path.resolve(__dirname); |
| <Button | ||
| className='grow' | ||
| variant='outline' | ||
| onClick={() => { | ||
| setPreset(newPreset({ length: props.length, fill: DEFAULT_OFF_PIXEL_COLOR })); | ||
| setSelectedPixel(null); | ||
| }} | ||
| > | ||
| Fill all white | ||
| </Button> |
There was a problem hiding this comment.
The button label says "Fill all white" but it actually fills all pixels with DEFAULT_OFF_PIXEL_COLOR which is #000000 (black). This should either be changed to "Fill all black" or use #FFFFFF (white) instead of DEFAULT_OFF_PIXEL_COLOR.
| return new Promise(async (resolve, reject) => { | ||
| try { | ||
| log.debug(`[FLASH] <start>`, flashTimer.duration); | ||
| await new Flasher(board, port.path).flash(firmataPath); | ||
| log.debug('[FLASH] <done>', flashTimer.duration); | ||
| resolve(null); | ||
| } catch (flashError) { | ||
| log.error('[FLASH] <error>', flashError, flashTimer.duration); | ||
|
|
||
| try { | ||
| await checkPortError(flashError, port.path, 'flashing'); | ||
| // Port still exists but couldn't open - preserve original error | ||
| reject(flashError); | ||
| } catch (portError) { | ||
| // Port disconnected or already PortDisconnectedError - reject with port error | ||
| reject(portError); | ||
| } | ||
| } |
There was a problem hiding this comment.
Using async modifier on Promise executor function is an anti-pattern and can lead to unhandled promise rejections. If the executor throws synchronously before the try block, the error won't be caught. Remove the async keyword and use synchronous code in the executor, or restructure to avoid wrapping async code in a Promise constructor.
| return new Promise(async (resolve, reject) => { | |
| try { | |
| log.debug(`[FLASH] <start>`, flashTimer.duration); | |
| await new Flasher(board, port.path).flash(firmataPath); | |
| log.debug('[FLASH] <done>', flashTimer.duration); | |
| resolve(null); | |
| } catch (flashError) { | |
| log.error('[FLASH] <error>', flashError, flashTimer.duration); | |
| try { | |
| await checkPortError(flashError, port.path, 'flashing'); | |
| // Port still exists but couldn't open - preserve original error | |
| reject(flashError); | |
| } catch (portError) { | |
| // Port disconnected or already PortDisconnectedError - reject with port error | |
| reject(portError); | |
| } | |
| } | |
| return new Promise((resolve, reject) => { | |
| const doFlash = async () => { | |
| try { | |
| log.debug(`[FLASH] <start>`, flashTimer.duration); | |
| await new Flasher(board, port.path).flash(firmataPath); | |
| log.debug('[FLASH] <done>', flashTimer.duration); | |
| resolve(null); | |
| } catch (flashError) { | |
| log.error('[FLASH] <error>', flashError, flashTimer.duration); | |
| try { | |
| await checkPortError(flashError, port.path, 'flashing'); | |
| // Port still exists but couldn't open - preserve original error | |
| reject(flashError); | |
| } catch (portError) { | |
| // Port disconnected or already PortDisconnectedError - reject with port error | |
| reject(portError); | |
| } | |
| } | |
| }; | |
| doFlash(); |
| constructor(private readonly startTime = performance.now()) {} | ||
|
|
||
| get duration() { | ||
| return performance.now() - this.startTime + 'ms'; |
There was a problem hiding this comment.
The duration getter returns a string by concatenating a number with 'ms', which could lead to unexpected behavior when used in contexts expecting a number. Consider returning a number and formatting as needed at usage sites, or explicitly typing the return as string.
| return performance.now() - this.startTime + 'ms'; | |
| return performance.now() - this.startTime; |
| @@ -59,12 +59,14 @@ export async function recreateWindowWhenNeeded() { | |||
| // Check if we're in development mode | |||
| const isDevelopment = !!app.isPackaged; | |||
There was a problem hiding this comment.
The logic for isDevelopment appears to be inverted. app.isPackaged is true when the app is packaged (production), but the variable is named isDevelopment and uses !!app.isPackaged. This should likely be !app.isPackaged to correctly identify development mode.
| const isDevelopment = !!app.isPackaged; | |
| const isDevelopment = !app.isPackaged; |
| port: connectedPort?.path, | ||
| message: `${connectedPort?.path} is no longer connected`, | ||
| }, | ||
| }); |
There was a problem hiding this comment.
The checkConnectedPort function detects a disconnected port but doesn't clear the connectedPort variable or call any cleanup. This could leave stale state. Consider calling setConnectedPort(undefined) when a port is no longer found.
| }); | |
| }); | |
| setConnectedPort(undefined); |
| JSON.stringify(data.nodes, null, 2), | ||
| JSON.stringify(data.edges, null, 2), |
There was a problem hiding this comment.
Logging the full JSON stringified nodes and edges on every flow update could produce excessive log output and impact performance, especially for large flows. Consider using a debug flag to conditionally enable this verbose logging, or log only a summary (e.g., node count and edge count).
| JSON.stringify(data.nodes, null, 2), | |
| JSON.stringify(data.edges, null, 2), | |
| `nodes: ${Array.isArray(data.nodes) ? data.nodes.length : 0}`, | |
| `edges: ${Array.isArray(data.edges) ? data.edges.length : 0}`, |
| import { | ||
| BOARDS, | ||
| Flasher, | ||
| getConnectedPorts, | ||
| UnableToOpenSerialConnection, | ||
| type BoardName, | ||
| type PortInfo, | ||
| } from '@microflow/flasher'; |
There was a problem hiding this comment.
Unused import BOARDS.
| DialogHeader, | ||
| DialogTitle, | ||
| } from '@microflow/ui'; | ||
| import { useState, useMemo } from 'react'; |
There was a problem hiding this comment.
Unused import useMemo.
| import { useState, useMemo } from 'react'; | |
| import { useState } from 'react'; |
| DialogTitle, | ||
| DialogTrigger, | ||
| } from '@microflow/ui'; | ||
| import { PropsWithChildren, useState, useEffect, useMemo } from 'react'; |
There was a problem hiding this comment.
Unused import useMemo.
| import { PropsWithChildren, useState, useEffect, useMemo } from 'react'; | |
| import { PropsWithChildren, useState, useEffect } from 'react'; |
This pull request introduces a new system for managing microcontroller board connections and port events in the Electron app, providing more robust detection and handling of device connections and disconnections. It adds new utilities and refactors existing code to improve maintainability and reliability, especially around the board runner process, port management, and device event listeners.
Board connection and runner process management:
board-connection.tsto centralize logic for starting, killing, and managing the runner process for microcontroller boards, including error handling for disconnected ports and flashing firmware when needed.Timerclass for measuring elapsed time in operations such as board connection and flashing.Port and device management:
port-manager.tsto manage the current connected port, poll for port changes, and set up Electron device event listeners for serial and USB devices. This includes handling permissions, device addition/removal, and fallback polling for robust port state tracking.Code organization and minor improvements:
Pixelnode to match new file structure, improving maintainability.Build and developer experience:
clean:vitescript topackage.jsonfor cleaning up Vite build artifacts, aiding in local development and troubleshooting.