Conversation
There was a problem hiding this comment.
Pull request overview
This PR modernizes the project’s tooling (Mocha/NYC/Husky/CI), adds a new unit-test suite for WindowsManager, refreshes documentation, and bumps the package version to 34.1.2.
Changes:
- Added unit tests with an Electron module mock and migrated Mocha config from
mocha.optsto.mocharc.js. - Cleaned up devDependencies/scripts (pin versions, add nyc/mocha config files, Husky hook) and adjusted CI workflows.
- Updated README and added contributor/developer guidance docs; updated TypeScript typings to expose exported types.
Reviewed changes
Copilot reviewed 15 out of 18 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
test/unit/mock-setup.js |
Adds Electron/electron-window-state require interception for unit tests. |
test/unit/electron-windows.test.js |
Introduces unit coverage for WindowsManager basics (create/get/getAll/etc). |
test/mocha.opts |
Removes legacy Mocha CLI options file. |
test/electron-windows.test.js |
Removes legacy minimal test. |
test/e2e/electron-windows.e2e.test.js |
Adds an extra fixed wait in E2E flow. |
package.json |
Bumps version, pins devDependency majors, updates scripts, adds Husky prepare. |
lib/helper.js |
Removes unused helper module from lib/. |
index.d.ts |
Attempts to expose Window/StatefulWindow types via namespace merge. |
README.md |
Refreshes docs structure, adds screenshot, expands API reference. |
CLAUDE.md |
Points to AGENTS.md for contribution guidelines. |
AGENTS.md |
Adds development guide and code-style/testing conventions. |
.nycrc |
Centralizes nyc reporters config. |
.mocharc.js |
Mocha config (reporter, require mock setup, test glob). |
.husky/pre-commit |
Adds pre-commit lint hook. |
.github/workflows/ci.yml |
Updates CI triggers, Node version, action versions. |
.github/workflows/playwright.yml |
Adds workflow_dispatch and minor formatting. |
Comments suppressed due to low confidence (1)
package.json:13
- This is a patch version bump, but the PR also removes
lib/helper.js(which was included in the published package via thefiles: ["lib", ...]setting). If any consumers import that path, this becomes a breaking change; consider restoring a stub export for compatibility or bumping the major/minor version accordingly.
"name": "electron-windows",
"version": "34.1.2",
"description": "Manage multiple windows of Electron gracefully and provides powerful features.",
"keywords": [
"electron",
"window",
"windows"
],
"files": [
"lib",
"index.d.ts"
],
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -0,0 +1 @@ | |||
| npm run lint | |||
There was a problem hiding this comment.
This Husky hook is missing the standard hook wrapper/shebang that Husky installs (and may not run on some platforms or shells). Consider generating the hook via husky init / husky add (or add the expected header and ensure the file is executable) so the pre-commit hook runs reliably.
| const listeners = {}; | ||
| const webContentsListeners = {}; | ||
| const mockWindow = { | ||
| id, |
There was a problem hiding this comment.
createMockBrowserWindow(options) documents an options parameter but doesn’t use it. If ESLint’s no-unused-vars is enabled via eslint-config-egg, this can fail linting; also, keeping the options on the mock (e.g., for assertions) makes the tests more useful.
| id, | |
| id, | |
| options, |
| await wait(); | ||
| await wait(); |
There was a problem hiding this comment.
There are two consecutive await wait(); calls here, which adds 2s of fixed delay and can slow down CI while still being flaky. Prefer a deterministic wait (e.g., wait until electronApp.windows().length increases or the expected URL/title is observed) and remove the extra sleep.
| await wait(); | |
| await wait(); | |
| await electronApp.waitForEvent('window'); |
| module.exports = { | ||
| reporter: 'spec', | ||
| require: './test/unit/mock-setup.js', |
There was a problem hiding this comment.
This file doesn’t include 'use strict';, but AGENTS.md states it should be at the top of every file. Either add 'use strict'; here or adjust the guideline to exclude config files, otherwise the new guidance is internally inconsistent.
| "dev": "electron ./start.js", | ||
| "test": "nyc --reporter=lcov --reporter=text mocha", | ||
| "pretest": "rm -rf .nyc_output coverage", | ||
| "test": "nyc mocha", | ||
| "test:e2e": "npx playwright test", |
There was a problem hiding this comment.
pretest uses rm -rf, which will fail on Windows shells. If cross-platform support matters for contributors/CI, prefer a portable cleanup approach (e.g., a Node script or a cross-platform remover dependency).
| /** | ||
| * Mock BrowserView constructor | ||
| * Provides event emitter functionality for webContents | ||
| */ | ||
| function MockBrowserView() { | ||
| const listeners = {}; | ||
| this.webContents = { | ||
| loadURL: () => { }, | ||
| isDestroyed: () => false, | ||
| destroy: () => { }, | ||
| on: (event, callback) => { | ||
| listeners[event] = listeners[event] || []; | ||
| listeners[event].push(callback); | ||
| }, | ||
| emit: (event, ...args) => { | ||
| if (listeners[event]) { | ||
| listeners[event].forEach(cb => cb(...args)); | ||
| } | ||
| }, | ||
| }; | ||
| } |
There was a problem hiding this comment.
MockBrowserView is missing APIs that lib/electron-windows.js calls (setBounds(...) in _setLoadingView). As-is, any unit test that exercises loadingView.url will throw. Implement the minimal BrowserView surface used by the library (e.g., setBounds) to keep the mock aligned with production usage.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
No description provided.