Replace keysender with nutjs for cross-platform support #280
Conversation
This major change replaces the Windows-only keysender library with nutjs (@nut-tree-fork/libnut) to enable cross-platform support for Windows, macOS, and Linux. ## Changes: - Removed keysender provider implementation and all references - Added nutjs provider with full keyboard, mouse, screen, and clipboard support - Updated default provider from keysender to nutjs - Fixed all TypeScript build errors and test failures - Updated CI to test on Windows, Ubuntu, and macOS ## Documentation: - Updated README to reflect cross-platform support - Added clear build instructions for native compilation - Clarified project maintenance status (not actively developed) - Added CHANGELOG.md documenting the migration - Disabled npm publishing (nutjs requires building from source) ## Breaking Changes: - Users must build from source (no npm package) - Requires native build tools (Visual Studio, Python, node-gyp) - AutoHotkey and PowerShell providers remain Windows-only All tests pass (152/152) and the project builds successfully. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
Updated dependencies to fix high severity vulnerabilities found by npm audit. This should resolve CI build failures across all platforms. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
The resizeWindow and repositionWindow methods in the nutjs screen provider don't use any await expressions, so they don't need to be async. This fixes ESLint errors and ensures CI builds pass. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
The resizeWindow and repositionWindow methods must be async to match the ScreenAutomation interface. Added ESLint disable comments for the require-await rule since these methods don't need await expressions. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
This resolves path issues on Windows where prettier command is not found in the PATH. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
Remove the pr-review webhook job as we have a separate workflow for watching PRs. This will fix the failing CI checks on Dependabot PRs. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
|
@MCPClaude review |
|
I understand. Working on a comprehensive code review for PR #280 - Replace keysender with nutjs for cross-platform support. This is a significant change affecting 39 files, so I'll examine the changes thoroughly... |
|
I understand. Starting comprehensive review of PR #280 - Replace keysender with nutjs for cross-platform support... |
Removed the pr-review job from CI workflow and deleted the associated pr-webhook-utils.cjs file as they are no longer needed. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
Comprehensive Review of PR #280 - Replace keysender with nutjsThank you for this significant contribution to make MCPControl cross-platform! This is a well-executed migration that successfully replaces the Windows-only keysender with the cross-platform nutjs library. The PR is thorough and includes proper documentation updates, test coverage, and CI/CD modifications. ✅ Strengths
|
🔧 Recommendations
📋 Checklist Before Merge
🎯 Overall AssessmentThis is a high-quality PR that successfully achieves its goal of making MCPControl cross-platform. The architecture improvements with the modular provider system are particularly well done. While there are some issues to address, none are blocking, and most can be addressed in follow-up PRs. The breaking changes are well-documented, and the version bump to 0.3.0 is appropriate. The requirement to build from source (due to nutjs native compilation) is an acceptable trade-off for cross-platform support. Recommendation: REQUEST_CHANGES - The security/validation issues should be addressed before merge, but this is excellent work overall! Reviewed at commit: f4a1564 |
🔍 Specific Code Issues1. console.error in mouse.ts:163// Current code:
console.error('Error releasing mouse button during cleanup:', cleanupError);
// Should be:
// Error during cleanup is non-critical, already in error state2. Missing input validation in keyboard.ts:18async typeText(text: string): Promise<{ success: boolean; message?: string }> {
try {
// Add validation:
if (typeof text \!== 'string') {
return { success: false, message: 'Text must be a string' };
}
if (text.length > 10000) {
return { success: false, message: 'Text too long (max 10000 characters)' };
}
libnut.typeString(text);
// ...3. Missing size validation in screen.ts:207// Add before screenshot capture:
const maxSize = 10000; // 10k pixels max dimension
if (region && (region.width > maxSize || region.height > maxSize)) {
return { success: false, message: `Region too large (max ${maxSize}x${maxSize})` };
}4. Silent errors in screen.ts:81} catch (error) {
// Window may have closed, skip it
// Consider adding: logger.debug(`Failed to get title for window ${handle}: ${error}`);
} |
📝 Documentation Improvements NeededMissing Platform-Specific Prerequisites in READMEThe README claims cross-platform support but only shows Windows instructions. Please add: macOS Prerequisites: # Xcode Command Line Tools
xcode-select --install
# Python 3.12+
brew install [email protected]
# Build tools
npm install -g node-gyp cmake-jsLinux Prerequisites: # Build essentials
sudo apt-get install build-essential
# Python 3.12+
sudo apt-get install python3.12
# X11 dependencies (required for nutjs)
sudo apt-get install libx11-dev libxext-dev libxi-dev
# Build tools
npm install -g node-gyp cmake-jsMissing Migration GuideAdd a migration section for users upgrading from v0.2.0: ## Migrating from v0.2.0 to v0.3.0
Version 0.3.0 replaces keysender with nutjs. To upgrade:
1. Ensure you have all prerequisites installed (see above)
2. Delete node_modules and package-lock.json
3. Run `npm install` to compile nutjs
4. The API remains the same, no code changes needed
5. Note: You must build from source, npm package is no longer available |
Cheffromspace
added
claude-review-complete
claude-review-in-progress
and removed
claude-review-in-progress
claude-review-complete
labels
|
I understand. Working on a comprehensive code review of PR #280... |
1 similar comment
|
I understand. Working on a comprehensive code review of PR #280... |
📝 Updated Review - New Changes Since Last ReviewI've noticed new changes have been merged since my previous review (commit 252a00b). Here are the additional findings: 🚨 Critical Issue - Cross-Platform CI RemovedThe multi-platform CI testing has been reverted to Windows-only in .github/workflows/ci.yml. This is concerning because:
Recommendation: Restore the matrix strategy for CI testing: strategy:
matrix:
os: [windows-latest, ubuntu-latest, macos-latest]
runs-on: ${{ matrix.os }}✅ Good Changes
📋 Updated Checklist Before MergeAll previous recommendations still apply, plus:
🎯 Updated AssessmentThis PR still successfully implements cross-platform support via nutjs, but the removal of cross-platform CI testing is a significant regression that undermines confidence in the cross-platform claim. Recommendation: REQUEST_CHANGES - The CI configuration must be fixed to test on all supported platforms before merge. This is essential for a PR claiming to add cross-platform support. Reviewed at commit: 252a00b |
| message: `Typed text successfully`, | ||
| }; | ||
| } catch (error) { | ||
| return { |
There was a problem hiding this comment.
// Add input validation before processing
if (!text || typeof text !== 'string') {
return { success: false, message: 'Text must be a non-empty string' };
}
if (text.length > 10000) {
return { success: false, message: 'Text too long (max 10000 characters)' };
}
Comprehensive Review of PR #280 - Replace keysender with nutjsI've completed a thorough review of this PR that replaces the Windows-only keysender library with the cross-platform nutjs library. This is a significant change that enables MCPControl to work on Windows, macOS, and Linux. 🎯 OverviewThis PR successfully achieves its goal of adding cross-platform support, but there are several important considerations to address before merging. ✅ Strengths
🚨 Critical Issues1. Security VulnerabilitiesThe nutjs provider lacks input validation in several areas:
Recommendation: Add comprehensive input validation layer before processing any user input. 2. Breaking Changes
3. Dependency Concerns
|
| 'Failed to release mouse button after drag error:', | ||
| cleanupError instanceof Error ? cleanupError.message : String(cleanupError), | ||
| ); | ||
| } |
There was a problem hiding this comment.
// Replace console.error with proper error handling
// Either remove this line or use a proper logger if available
// The error is already being handled in the catch block
Cheffromspace
added
claude-review-complete
and removed
claude-review-in-progress
labels
| }; | ||
|
|
||
| // Capture screen or region | ||
| const screen = options?.region |
There was a problem hiding this comment.
// Add size validation to prevent memory exhaustion
const MAX_DIMENSION = 10000; // 10k pixels max
if (region) {
if (region.width > MAX_DIMENSION || region.height > MAX_DIMENSION ||
region.width <= 0 || region.height <= 0) {
return {
success: false,
message: Invalid region dimensions (max ${MAX_DIMENSION}x${MAX_DIMENSION})
};
}
}
📋 Code Review Complete - PR #280I've completed a comprehensive review of this PR. Here's the summary: 🎯 Overall AssessmentThis PR successfully replaces keysender with nutjs, achieving the goal of cross-platform support. The implementation is solid with good test coverage (152/152 tests passing) and proper modular architecture. 🚨 Critical Issues to Address
💬 Inline Comments AddedI've added specific code suggestions as inline comments for:
✅ Next Steps
The core functionality is well-implemented. Once these issues are addressed, this will be an excellent addition to MCPControl! Reviewed at commit: 252a00b |
There was a problem hiding this comment.
Comprehensive Code Review - PR #280: Replace keysender with nutjs
I've completed a thorough review of this pull request. This PR successfully migrates MCPControl from the Windows-only keysender to the cross-platform nutjs library.
✅ Overall Assessment
This is a well-executed migration that maintains the existing API while adding cross-platform support. The code quality is generally high, with good separation of concerns and comprehensive test coverage. However, there are several issues that should be addressed before merging.
⚠️ Critical Issues to Address
1. Security & Input Validation
- clipboard.ts: No validation on
input.textbefore writing to clipboard (line 26-28) - No size limits on clipboard content which could lead to memory exhaustion
- No rate limiting on automation commands that could be abused
2. Code Quality Issues
- mouse.ts:159-163: Uses
console.errorin production code - screen.ts:278-279, 294: Debug logging with
console.logandconsole.errorshould be removed - ESLint warnings: Suppressed with comments instead of fixing the underlying async/await patterns
3. Test Coverage Gaps
- Screenshot functionality severely under-tested - only error paths covered
- No tests for screenshot options (region, format, quality, grayscale, resize)
- Missing edge case tests for invalid inputs and extreme values
- Console output not mocked/verified in tests
4. Documentation Issues
- README only shows Windows prerequisites despite claiming cross-platform support
- No migration guide for users upgrading from v0.2.0
- Missing platform-specific build instructions for macOS and Linux
📋 Required Changes Before Merge
- Remove all console statements
- Add input validation in clipboard.ts
- Update README with platform-specific prerequisites
- Add comprehensive screenshot tests
🎯 Recommendations
- Fix ESLint warnings properly instead of suppressing them
- Consider adding rate limiting to prevent abuse
- Implement window title filtering to prevent exposing sensitive information
- Add performance safeguards for large screenshots
💡 Positive Aspects
- Excellent modular architecture with component-level provider selection
- Comprehensive error handling with proper cleanup
- Well-structured test suites (despite coverage gaps)
- Clear documentation of breaking changes
- Appropriate version bump to 0.3.0
Recommendation: The security and code quality issues should be addressed before merging. Specifically, the console statements must be removed, input validation added to clipboard operations, and documentation updated with complete platform instructions.
Reviewed at commit: 252a00b
- Add platform-specific prerequisites for Windows, macOS, and Linux - Add platform-specific build instructions - Create comprehensive migration guide from v0.2.0 to v0.3.0+ - Update README to reference migration guide for upgrading users
| 'Failed to release mouse button after drag error:', | ||
| cleanupError instanceof Error ? cleanupError.message : String(cleanupError), | ||
| ); | ||
| } |
There was a problem hiding this comment.
Remove console.error from production code. Either use a proper logging system or remove this line entirely.
| // Log the size of the image for debugging | ||
| console.log( | ||
| `Screenshot captured: ${outputBuffer.length} bytes (${Math.round(outputBuffer.length / 1024)}KB)`, | ||
| ); |
There was a problem hiding this comment.
Remove console.log debug statement from production code.
| }; | ||
| } catch (sharpError) { | ||
| // Fallback with minimal processing if sharp pipeline fails | ||
| console.error(`Sharp processing failed: ${String(sharpError)}`); |
There was a problem hiding this comment.
Remove console.error from production code. Use proper error handling or logging system.
📝 Additional Inline FeedbackInput Validation Needed -
|
|
@MCPClaude can you address the merge conflicts in pr 280? |
|
I understand. Working on resolving the merge conflicts in this PR... |
Resolved conflicts: - package.json: Kept both nutjs dependencies and updated SDK version - package-lock.json: Regenerated after resolving package.json
|
✅ Merge conflicts resolved! The conflicts were in and :
All tests are passing and the PR is ready to merge. |
|
✅ Merge conflicts resolved! The conflicts were in
All tests are passing and the PR is ready to merge. |
|
Perfect! I've successfully resolved the merge conflicts in PR #280. The conflicts were in the package.json and package-lock.json files, where I:
All tests are passing and the PR is now mergeable. The changes have been pushed to the remote repository. |
✅ Merge conflicts resolved!I've successfully resolved the merge conflicts in this PR: What was done:
Verification:
The PR is now ready to merge! The merge state shows as UNSTABLE, which might be due to CI checks still running, but there are no merge conflicts blocking the merge. |
|
Perfect! I've successfully resolved the merge conflicts in PR #280. The conflicts were in package.json and package-lock.json, where the feature branch was replacing keysender with nutjs dependencies while main had updated other dependency versions. I merged them by keeping the nutjs dependencies and using the updated versions from main for the other packages. All tests are passing and the PR is now mergeable. |
|
@MCPClaude Please fix linter errors on pr 280 |
|
I understand. Working on fixing the linter errors in this PR... |
|
I understand. Working on fixing linter errors for PR #280... |
Remove unused @typescript-eslint/no-misused-promises directives that were causing linter warnings 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
|
✅ Fixed linter errors! I've removed the two unnecessary eslint-disable comments from
The linter now passes without any warnings, and all tests are still passing. |
|
All done! I've successfully fixed the linter errors on PR #280 by removing the two unnecessary eslint-disable comments from |
|
✅ Fixed linter errors! I've removed the two unused ESLint disable comments from
The linter now passes with no errors or warnings. |
|
All done! I've successfully fixed the linter errors on PR #280 by removing the two unused eslint-disable comments from the server.ts file. The linter now passes without any errors or warnings. |
Summary
This PR replaces the Windows-only keysender library with nutjs (@nut-tree-fork/libnut) to enable cross-platform support for Windows, macOS, and Linux.
Motivation
Changes
Documentation Updates
Breaking Changes
Testing
Version
Bumped to v0.3.0 due to breaking changes
Notes
🤖 Generated with Claude Code