test(e2e): Electron IPC coverage — floating, braindump, settings - #96
Merged
Conversation
- Add 'QA after implementing, before ship' to Industry Standards table - Add 'Ship before QA passes' to Violations table - Add Definition of Done section (QA non-negotiable policy) - Rename 'smoke test/smoke' → 'QA' throughout (2 occurrences) - Add Dev vs Production Electron comparison table (bundle ID, app name, renderer URL, app.isPackaged, NODE_ENV, computer-use grantability)
…and braindump - settings-popover.spec.ts: settings namespace IPC (setShowInMenuBar, getStartupConfig, setStartupConfig, setHideAppIcon) - floating-window.spec.ts: toggleFloatingNavigator creates window; getAuxVisibility reflects visibility state; showFloatingNavigator is no-op when already visible; hideFloatingNavigator hides without destroying - braindump-window.spec.ts: brainDump.getOpacity/getSyncMode/getShortcut return valid values; brainDump.toggle creates window; getAuxVisibility reflects braindump state; setOpacity/setSyncMode persist values All tests pass typecheck and lint.
… values - Rename all test descriptions to observable behavior (no 'returns XXX' pattern) - Hard-code expected values where deterministic: setStartupConfig→true, setSyncMode→true, setOpacity(0.75)→0.75 - Add note explaining intentional shared-state coupling in floating/braindump (expensive app launch; toggle sequence is the feature path itself)
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
📝 WalkthroughWalkthroughThree new Playwright Electron end-to-end specs are added covering the BrainDump window IPC surface, Floating Navigator window lifecycle, and Settings IPC round-trips. ChangesQA Docs and Electron E2E Specs
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #96 +/- ##
=======================================
Coverage 69.35% 69.35%
=======================================
Files 125 125
Lines 4004 4004
Branches 1039 1042 +3
=======================================
Hits 2777 2777
Misses 1058 1058
Partials 169 169 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
e2e/electron/braindump-window.spec.ts (2)
100-125: ⚡ Quick winShared root cause: write-path tests validate IPC acknowledgements but not persisted state.
e2e/electron/braindump-window.spec.ts#L100-L125: aftersetOpacity(0.75)andsetSyncMode(false), callgetOpacity()/getSyncMode()and assert exact expected values.e2e/electron/settings-popover.spec.ts#L51-L68: aftersetStartupConfig(newConfig), callgetStartupConfig()and assert deep equality againstnewConfig.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@e2e/electron/braindump-window.spec.ts` around lines 100 - 125, The tests validate that IPC handlers return acknowledgements but do not verify that the values are actually persisted. In e2e/electron/braindump-window.spec.ts (lines 100-125), after calling setOpacity(0.75) in the first test, add a follow-up call to getOpacity() and assert it returns exactly 0.75; similarly, after calling setSyncMode(false) in the second test, add a follow-up call to getSyncMode() and assert it returns false. In e2e/electron/settings-popover.spec.ts (lines 51-68), after calling setStartupConfig(newConfig), add a follow-up call to getStartupConfig() and assert it deeply equals the newConfig object that was set. These getter calls verify that the write operations persist the correct values in the main process state, not just that the IPC calls succeed.
9-11: Add explicit serial mode to three Electron E2E specs that share state.These three files have order-dependent tests with shared Electron app instances but lack explicit serial configuration. When
workers: 1is removed (per the temporary workaround note in playwright.config.ts), these specs will fail unless serial mode is declared at file scope:
e2e/electron/braindump-window.spec.ts: Addtest.describe.configure({ mode: 'serial' })before the beforeAll hook.e2e/electron/floating-window.spec.ts: Addtest.describe.configure({ mode: 'serial' })before the beforeAll hook.e2e/electron/settings-popover.spec.ts: Addtest.describe.configure({ mode: 'serial' })before the beforeAll hook.This pattern already exists in
e2e/web/skill-tree.spec.tsand prevents race conditions when shared state (window visibility, settings mutations) changes across test boundaries.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@e2e/electron/braindump-window.spec.ts` around lines 9 - 11, Three Electron E2E spec files have order-dependent tests with shared Electron app instances but lack explicit serial configuration. Add test.describe.configure({ mode: 'serial' }) before the beforeAll hook in all three affected files to ensure tests run serially and prevent race conditions: in e2e/electron/braindump-window.spec.ts (before line 9 where the comment begins), in e2e/electron/floating-window.spec.ts (before line 9 where the comment section starts), and in e2e/electron/settings-popover.spec.ts (before line 17 where the comment begins). This ensures that when workers: 1 is removed from playwright.config.ts, these specs will maintain proper test execution order for their shared window visibility and settings state.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@CLAUDE.md`:
- Line 40: The heading "Definition of Done — QA before ship" currently includes
non-English Japanese text "(必須 / NON-NEGOTIABLE)" which violates the
repository's coding guidelines requiring all markdown documentation to be
written in English only. Remove the Japanese characters and parenthetical
content from this heading, keeping only the English text "Definition of Done —
QA before ship" to maintain consistency with repository documentation standards.
---
Nitpick comments:
In `@e2e/electron/braindump-window.spec.ts`:
- Around line 100-125: The tests validate that IPC handlers return
acknowledgements but do not verify that the values are actually persisted. In
e2e/electron/braindump-window.spec.ts (lines 100-125), after calling
setOpacity(0.75) in the first test, add a follow-up call to getOpacity() and
assert it returns exactly 0.75; similarly, after calling setSyncMode(false) in
the second test, add a follow-up call to getSyncMode() and assert it returns
false. In e2e/electron/settings-popover.spec.ts (lines 51-68), after calling
setStartupConfig(newConfig), add a follow-up call to getStartupConfig() and
assert it deeply equals the newConfig object that was set. These getter calls
verify that the write operations persist the correct values in the main process
state, not just that the IPC calls succeed.
- Around line 9-11: Three Electron E2E spec files have order-dependent tests
with shared Electron app instances but lack explicit serial configuration. Add
test.describe.configure({ mode: 'serial' }) before the beforeAll hook in all
three affected files to ensure tests run serially and prevent race conditions:
in e2e/electron/braindump-window.spec.ts (before line 9 where the comment
begins), in e2e/electron/floating-window.spec.ts (before line 9 where the
comment section starts), and in e2e/electron/settings-popover.spec.ts (before
line 17 where the comment begins). This ensures that when workers: 1 is removed
from playwright.config.ts, these specs will maintain proper test execution order
for their shared window visibility and settings state.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 861bb3ae-c737-4b75-8daa-a65910a94f59
📒 Files selected for processing (4)
CLAUDE.mde2e/electron/braindump-window.spec.tse2e/electron/floating-window.spec.tse2e/electron/settings-popover.spec.ts
7 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds Electron E2E coverage for three IPC namespaces that had zero automated test coverage:
New E2E specs
e2e/electron/floating-window.spec.ts— 5 ordered tests coveringwindow.toggleFloatingNavigator,showFloatingNavigator,hideFloatingNavigator, andgetAuxVisibility(verifies that toggle creates a window, idempotent show doesn't duplicate, hide reports as hidden, and aux window count stays exact after hide)e2e/electron/braindump-window.spec.ts— 7 ordered tests coveringbrainDump.toggle, opacity read/write, sync mode read/write, andgetAuxVisibilitycross-check (including an exact 0.75 opacity round-trip assertion)e2e/electron/settings-popover.spec.ts— 4 independent tests coveringsetShowInMenuBar,getStartupConfig,setStartupConfig, andsetHideAppIconIPC round-tripsDocumentation
CLAUDE.md— expanded Electron Native QA section with a comprehensive Dev vs Production comparison table (bundle ID, app name, renderer URL,app.isPackaged,NODE_ENV, and computer-use grantability), explaining exactly why native QA must run on the packaged buildReview fixes
.toBeLessThanOrEqualto.toBe(stricter — catches count increases that the previous assertion silently missed)Test Coverage
Test-only PR — 16 new Electron E2E tests across 3 spec files. No new application code paths.
Electron IPC surfaces now covered by E2E:
window.electronAPI.window.*—toggleFloatingNavigator,showFloatingNavigator,hideFloatingNavigator,getAuxVisibilitywindow.electronAPI.brainDump.*—toggle,getOpacity,setOpacity,getSyncMode,setSyncMode,getShortcutwindow.electronAPI.settings.*—getStartupConfig,setStartupConfig,setShowInMenuBar,setHideAppIconNote: MenuBar/tray tests cannot run in CI (
ELECTRON_E2E_DISABLE_SYSTEM_INTEGRATION=truedisables tray in E2E env). Native Cocoa surfaces are covered by local macOS native QA per CLAUDE.md.Pre-Landing Review
6 informational findings from Testing specialist (0 critical):
floating-window.spec.ts:113—toBeLessThanOrEqual(windowCountBefore)was too loose; changed totoBe(windowCountBefore)so a window count increase after hide is caughtbraindump-window.spec.ts:68— coverage gap: braindump toggle-to-close path not tested (SKIPPED — non-blocking)braindump-window.spec.ts:100— coverage gap: opacity clamping at out-of-range values not tested (SKIPPED)settings-popover.spec.ts:51— readback test aftersetStartupConfigwould be stronger (SKIPPED — IPC wire test is sufficient)settings-popover.spec.ts:43—typeof booleanassertion could be tighter (SKIPPED — medium confidence)_helpers/launch.ts:11— CDP port sharing concern if workers > 1 (SKIPPED — config enforces workers: 1)PR Quality Score: 7.0/10
Design Review
No frontend files changed — design review skipped.
Scope Drift
Scope Check: CLEAN
Intent: Add Electron E2E specs for floating window, braindump, and settings IPC
Delivered: exactly that + CLAUDE.md dev/prod comparison table (directly supports E2E testing context)
Plan Completion
No plan file detected.
TODOS
No TODO items completed in this PR.
Test plan
pnpm test)🤖 Generated with Claude Code
Summary by CodeRabbit
Documentation
Tests