Fixes, improvements, and E2E test coverage #10
Conversation
- Fix EnvironmentService to properly detect pkg-bundled binaries using multiple detection methods (PKG_EXECPATH, snapshot path, process.pkg) - Update WebUIManager to use EnvironmentService for static path resolution instead of hardcoded process.cwd(), fixing "/Get error" on binary builds - Add SPA fallback route to serve index.html for client-side routing - Add explicit 404 handlers for API routes and missing static files - Update README with comprehensive installation instructions including: - Pre-built binary download table with platform guidance - Clear instructions for Raspberry Pi users (ARM64 vs ARMv7) - Corrected "Running from Source" instructions - Command line options documentation - Troubleshooting section for common issues - Bump version to 1.0.2
- Remove misleading assumption that users downloaded wrong architecture - Correctly identify the issue as a static file serving bug fixed in 1.0.2 - Move platform selection guidance to separate reference section - Simplify Raspberry Pi note
- Update project status from "not fully tested" to "production-ready" - Remove Windows-specific path reference that was not relevant to users - Update Testing Notes section to reflect what has been verified working - Reorganize remaining testing areas as "continued testing" items - Remove FlashForgeUI-Electron from Related Projects (internal reference)
- Add Usage section explaining how to access WebUI (localhost:3000) - Document default password (changeme) and how to change it - Replace JSON config block with clearer settings table - Add descriptions for all configuration options - Rename Development section to Building from Source - Add comments for each platform build command - Remove redundant dev server instructions (already in Running from Source)
Implement complete test coverage for core functionality: - EnvironmentService: Package detection, path resolution, environment state (23 tests) - ConfigManager: Configuration loading, updates, validation, events (23 tests) - Error utilities: All error codes, factories, handlers, serialization (68 tests) - WebUIManager integration: Static files, API routes, SPA routing, middleware (24 tests) Test infrastructure: - Configure Jest 30.2.0 with ts-jest and ESM preset - Add supertest for HTTP integration testing - Implement Express 5.x compatible routing tests - Add test scripts: test, test:watch, test:coverage, test:verbose All 118 tests passing (100% pass rate) across 4 test suites.
Add automated E2E testing for Windows, macOS, and Linux builds (x64, ARM64, ARMv7). The workflow validates binary builds, startup, API endpoints, and graceful shutdown across all platforms.
Update workflow triggers to run on all branches and PRs, not just main.
The auth status endpoint returns authRequired, not authenticated. Update test to validate JSON structure instead.
- Skip execution tests for ARMv7 (cannot run on x64 runner) - Fix macOS stat command format (-f%z not -f%) - Fix Windows Start-Process parameters - Fix API test to validate JSON structure properly - Add can_execute matrix flag for cross-compiled binaries
- Use cmd.exe for background process start with proper redirection - Force bash shell for summary generation (PowerShell incompatible)
Skip log file validation on Windows (Git Bash doesn't handle background process I/O redirection reliably). Instead: - Use start /b to launch process - Verify process is running via tasklist - Kill process via taskkill - API tests provide actual validation Linux/macOS continue with full log validation.
Git Bash cannot properly execute 'start /b' command. Use PowerShell Start-Process with -WindowStyle Hidden instead, which properly detaches the process and returns immediately.
Remove problematic tasklist checks on Windows. API tests provide sufficient validation - if they pass, the binary started successfully. Can improve Windows checks later.
…erage - Fix path-to-regexp crash: Express 5 requires named wildcards (/*splat) - Fix local build scripts: yao-pkg → pkg (actual binary name from @yao-pkg/pkg) - Rewrite E2E workflow: proper Windows process management with PowerShell Start-Process and log capture, curl retry health checks, HTTP status code validation, and full parity between Windows and Unix test coverage
…ved var - Use 127.0.0.1 instead of localhost for curl health checks (Windows CI runners may not resolve localhost to the loopback interface) - Rename $pid to $serverPid in PowerShell cleanup ($pid is read-only)
Start-Process with -NoNewWindow and -RedirectStandardOutput may not fully detach the process on Windows CI runners. Use cmd.exe wrapper with -WindowStyle Hidden (matching the old working approach) while still capturing stdout/stderr via cmd redirection.
… matching Get-Process -Name glob may not match the child exe spawned via cmd wrapper. Use taskkill /F /IM for stop and tasklist /FI for verify, which match on the exact executable filename.
Summary of ChangesHello @GhostTypes, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request delivers a significant upgrade to the FlashForgeWebUI project by addressing critical routing and packaging issues, modernizing the build system, and establishing a robust testing infrastructure. The changes ensure greater stability for the web interface, improve the reliability of binary distributions across multiple operating systems, and provide a solid foundation for future development through comprehensive unit, integration, and end-to-end tests. Additionally, user and developer documentation has been thoroughly updated to reflect these improvements and offer clearer instructions. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Ignored Files
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request includes bug fixes, documentation improvements, and enhanced test coverage. However, a critical logic flaw exists in the routing order that makes the logout functionality ineffective. Additionally, the 404 handler for API routes and the SPA fallback route both reflect the raw req.path in the error response, which is a poor security practice. I've provided suggestions on WebUIManager to enhance routing logic, especially for API 404 handling and SPA fallback. Also, consider addressing security best practices for sensitive data and cryptographic implementations in the existing codebase.
| this.expressApp.get('/*splat', (req, res, next) => { | ||
| // Skip if this looks like a file request with extension (handled by static middleware) | ||
| if (path.extname(req.path) && req.path !== '/') { | ||
| // File request that wasn't found by static middleware - return 404 | ||
| const response: StandardAPIResponse = { | ||
| success: false, | ||
| error: `File not found: ${req.path}` | ||
| }; | ||
| res.status(404).json(response); | ||
| return; | ||
| } | ||
|
|
||
| // Serve index.html for SPA routes | ||
| const indexPath = path.join(this.webUIStaticPath, 'index.html'); | ||
| if (fs.existsSync(indexPath)) { | ||
| res.sendFile(indexPath); | ||
| } else { | ||
| next(); | ||
| } | ||
| }); |
There was a problem hiding this comment.
The use of path.extname(req.path) to detect file requests is brittle because it will incorrectly treat SPA routes that contain a dot (e.g., /users/john.doe) as a request for a file, leading to an incorrect 404 response.
A more robust method is to check the Accept header to distinguish between navigation requests (which accept text/html) and asset requests. This would correctly handle all valid SPA routes, including those with dots.
| } else { | ||
| next(); | ||
| } |
There was a problem hiding this comment.
If index.html is not found, this logic calls next(), which will likely fall through to Express's default 404 handler. Since a missing index.html indicates a server build or configuration error, it would be better to treat it as a server error.
Consider passing an error to next() so it can be handled by your createErrorMiddleware, for example: next(new AppError('SPA entry point (index.html) not found', ErrorCode.FILE_NOT_FOUND));
| } else { | |
| next(); | |
| } | |
| } else { | |
| next(new AppError('SPA entry point (index.html) not found', ErrorCode.FILE_NOT_FOUND)); | |
| } |
Fixes indefinite hangs during graceful shutdown (Ctrl+C) by implementing a three-tier timeout strategy that ensures the application always exits within 10 seconds, even when printers are unresponsive or HTTP connections are stuck. **Changes:** - **New timeout utility** (src/utils/ShutdownTimeout.ts): - TimeoutError class for timeout failures - withTimeout() wrapper for promises with timeout enforcement - createHardDeadline() for absolute maximum shutdown time - **ConnectionFlowManager.disconnectContext()**: - Added 5s timeout wrapper (configurable) - On timeout: forces cleanup (removes context, marks disconnected) - Backward compatible with existing callers - **WebUIManager.stop()**: - Added 3s timeout wrapper (configurable) - On timeout: calls closeAllConnections() to force-close stuck HTTP connections - Added isStopping guard flag to prevent concurrent stop calls - Added timing information logging - **Main shutdown logic** (src/index.ts): - Replaced sequential disconnect loop with parallel Promise.allSettled() - All printers disconnect concurrently instead of one-by-one - Added 10s hard deadline that calls process.exit(1) if exceeded - Added step-by-step logging with clear progress indicators - Preserved second Ctrl+C force-exit as safety net **Benefits:** - Single printer: < 5s shutdown - Multiple printers: < 10s (parallelized) - Hard deadline: Always exits within 10s maximum - Second Ctrl+C: Immediate force exit - Clear logging: Step-by-step progress visible **Timeout hierarchy:** ``` HARD DEADLINE (10s) ─────────────────────────────────┐ ├── Stop polling (immediate) │ ├── Parallel disconnects (5s each, concurrent) ─┤ └── WebUI server stop (3s, force close if timeout)─┘ ``` All changes are backward compatible - optional timeout parameters have sensible defaults and existing callers work without modification.
When index.html is not found during SPA fallback, the code now returns a
proper 500 server error with a clear message instead of a misleading 404.
This correctly indicates a build/deployment error rather than a user request
error.
Also fixes pre-existing ESLint configuration issues:
- Configured separate TypeScript project configs for backend and frontend
code to resolve parsing errors in src/webui/static files
- Added test files to tsconfig.json include array
- Replaced conditional require() with proper ES6 top-level import
for Windows readline handling
Changes:
- WebUIManager: Return AppError with ErrorCode.CONFIG_INVALID when
index.html is missing, including contextual debugging information
- WebUIManager: Added defensive documentation explaining why
path.extname() check is safe (app doesn't use client-side routing)
- eslint.config.mjs: Split configuration for backend (CommonJS) and
frontend (ES modules) with appropriate tsconfig references
- tsconfig.json: Added test files to include array
- index.ts: Replaced conditional require('readline') with top-level import
Fixes #10 comment #3 (Gemini Code Assist review)
|
@gemini-code-assist Let me know how this looks now after the two commits that were made to address your concerns |
There was a problem hiding this comment.
Code Review
This pull request introduces a wide range of improvements, including critical fixes for routing and static file serving, a more robust graceful shutdown mechanism, and comprehensive test coverage with Jest. The documentation has also been significantly updated. My review focuses on the new shutdown logic and API routing. I've identified a potential issue where shutdown failures could be masked and a couple of areas for minor refinement in the new routing and shutdown implementation. Overall, this is a fantastic set of changes that greatly improves the project's quality and reliability.
- Remove redundant withTimeout wrapper from shutdown disconnect calls since ConnectionFlowManager now handles timeouts internally - Add error re-throw in disconnectContext to ensure Promise.allSettled properly tracks rejected promises - Fix 404 catch-all route to use req.originalUrl instead of req.path for more accurate error reporting - Add Express.js skill documentation and permissions for git/pr workflows
|
@gemini-code-assist Let me know if the recent commit addresses all your concerns, and if you have any others |
|
Thank you for the update, @GhostTypes! I will perform a new review to assess the recent commits and see if all previous concerns have been addressed, and if there are any new observations. Please trigger a new review by commenting |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request primarily introduces a comprehensive set of documentation for Express.js 5.x, covering API references, migration guides, error handling, middleware, routing, security, performance best practices, and a quickstart guide. Additionally, it updates the project's internal CLAUDE.md and README.md files to reflect that the FlashForge WebUI project is now considered production-ready, detailing pre-built binaries, command-line options, and troubleshooting steps. The package.json version is incremented to 1.0.2, and the build scripts are updated to use pkg instead of yao-pkg. The eslint.config.mjs is refined to differentiate between backend and frontend TypeScript files, applying specific linting rules and sourceType for each. Crucially, the PR integrates Jest for testing, adding jest.config.js, a src/__tests__/setup.ts file, and new test scripts to package.json, along with corresponding dev dependencies. The core application logic in src/index.ts and src/managers/ConnectionFlowManager.ts is enhanced with a robust shutdown mechanism, introducing src/utils/ShutdownTimeout.ts to ensure graceful termination with timeouts for disconnecting printers and stopping the WebUI server. The src/services/EnvironmentService.ts is updated to improve detection of packaged environments and path resolution, and new test files (src/managers/ConfigManager.test.ts, src/services/EnvironmentService.test.ts, src/utils/error.utils.test.ts, src/webui/server/WebUIManager.integration.test.ts) are added to cover these changes and other core components. The WebUIManager now uses the EnvironmentService for static file paths and includes a SPA fallback mechanism, which a review comment notes might be misleading given the current comment about the app not using client-side routing.
- Remove incorrect claim that fallback "enables client-side routing" - Clarify that the app uses DOM manipulation, not URL-based routing - Explain the fallback's actual purpose: support page refreshes and direct URL access - Consolidate comments for better readability
Summary
path-to-regexpcrash: wildcard routes require named params (/*splatnot/*)yao-pkg→pkg(actual binary name from@yao-pkg/pkg)Start-Processwith log capture via cmd wrapperTest plan
npm run build:win) succeeds🤖 Generated with Claude Code