-
Notifications
You must be signed in to change notification settings - Fork 1
feat: enhance led strip #102
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
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
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.
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); |
Copilot
AI
Dec 8, 2025
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.
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> |
Copilot
AI
Dec 8, 2025
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.
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); | ||
| } | ||
| } |
Copilot
AI
Dec 8, 2025
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.
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'; |
Copilot
AI
Dec 8, 2025
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.
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; |
| } | ||
|
|
||
| // Check if we're in development mode | ||
| const isDevelopment = !!app.isPackaged; |
Copilot
AI
Dec 8, 2025
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.
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`, | ||
| }, | ||
| }); |
Copilot
AI
Dec 8, 2025
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.
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), |
Copilot
AI
Dec 8, 2025
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.
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'; |
Copilot
AI
Dec 8, 2025
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.
Unused import BOARDS.
| DialogHeader, | ||
| DialogTitle, | ||
| } from '@microflow/ui'; | ||
| import { useState, useMemo } from 'react'; |
Copilot
AI
Dec 8, 2025
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.
Unused import useMemo.
| import { useState, useMemo } from 'react'; | |
| import { useState } from 'react'; |
| DialogTitle, | ||
| DialogTrigger, | ||
| } from '@microflow/ui'; | ||
| import { PropsWithChildren, useState, useEffect, useMemo } from 'react'; |
Copilot
AI
Dec 8, 2025
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.
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.