refactor: Replace Node.js/npm with Bun runtime#114
Conversation
- Replace npm scripts with bun equivalents - Replace Jest with Bun's built-in test runner - Use bun build for Electron main/preload bundling - Build preload.ts separately to fix contextBridge error - Mark @sentry/electron as external to prevent preload bundling - Add happy-dom for DOM testing environment - Remove Jest, ts-jest, tsc-alias dependencies - Update CI workflows to use Bun - Clean up outdated documentation files Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
github-actions
bot
added
docs
ci
dependencies
frontend
testing
database
backend
i18n
config
and removed
size/XXL
labels
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.OpenSSF Scorecard
Scanned Files
|
📝 WalkthroughWalkthroughThis PR executes a comprehensive migration from Jest/npm to Bun/bunx across the entire project, including all GitHub workflows, test framework infrastructure, build configuration, package management scripts, and documentation updates. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Bundle Size Report
Warning: Bundle size changed by more than 5%! This comment will be automatically updated on new commits |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| @@ -1,3 +1,4 @@ | |||
| import { describe, it, expect, beforeEach, afterEach, beforeAll, afterAll, mock, spyOn } from 'bun:test'; | |||
Check notice
Code scanning / CodeQL
Unused variable, import, function or class Note test
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 1 month ago
To fix unused imports, remove only the identifiers that are not referenced anywhere in the file, leaving the rest of the import intact. This avoids any functional change while cleaning up the code.
Concretely, in src/components/__tests__/ErrorBoundary.i18n.test.tsx, edit the import on line 1 from 'bun:test' to drop afterEach and spyOn from the destructuring list. No new imports, methods, or definitions are required; we are only tightening the existing import to contain the actually used names.
| @@ -1,4 +1,4 @@ | ||
| import { describe, it, expect, beforeEach, afterEach, beforeAll, afterAll, mock, spyOn } from 'bun:test'; | ||
| import { describe, it, expect, beforeEach, beforeAll, afterAll, mock } from 'bun:test'; | ||
| import React from 'react'; | ||
| import { render, screen, fireEvent } from '@testing-library/react'; | ||
| import { I18nextProvider } from 'react-i18next'; |
| @@ -1,41 +1,47 @@ | |||
| import { describe, it, expect, beforeEach, afterEach, beforeAll, afterAll, mock, spyOn } from 'bun:test'; | |||
Check notice
Code scanning / CodeQL
Unused variable, import, function or class Note test
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 1 month ago
In general, unused imports should be removed from the import list so the code reflects only what is actually used, reducing noise and potential confusion. For this file, the best fix is to edit the import from bun:test on line 1 and remove beforeAll and afterAll from the named imports, leaving the rest unchanged.
Concretely, in src/components/__tests__/FocusMode.test.tsx, update the first import statement to no longer include beforeAll and afterAll. No additional imports, functions, or definitions are required; the change is purely a cleanup of the import list and does not affect any runtime behavior or existing functionality.
| @@ -1,4 +1,4 @@ | ||
| import { describe, it, expect, beforeEach, afterEach, beforeAll, afterAll, mock, spyOn } from 'bun:test'; | ||
| import { describe, it, expect, beforeEach, afterEach, mock, spyOn } from 'bun:test'; | ||
| import { render, screen, fireEvent, waitFor } from '@testing-library/react'; | ||
| import '@testing-library/jest-dom'; | ||
| import { ThemeProvider } from '@/contexts/ThemeContext'; |
| @@ -1,3 +1,4 @@ | |||
| import { describe, it, expect, beforeEach, afterEach, beforeAll, afterAll, mock, spyOn } from 'bun:test'; | |||
Check notice
Code scanning / CodeQL
Unused variable, import, function or class Note test
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 1 month ago
In general, unused imports should be removed from the import statement. This avoids confusion and keeps the file consistent with static analysis rules without changing behavior.
In this specific case, the best fix is to edit the import from 'bun:test' on line 1 in src/components/__tests__/Settings.i18n.test.tsx to remove beforeAll, afterAll, and afterEach while leaving the other imported names (describe, it, expect, beforeEach, mock, spyOn) intact. No additional methods or imports are required, and no other lines need to change.
| @@ -1,4 +1,4 @@ | ||
| import { describe, it, expect, beforeEach, afterEach, beforeAll, afterAll, mock, spyOn } from 'bun:test'; | ||
| import { describe, it, expect, beforeEach, mock, spyOn } from 'bun:test'; | ||
| import React from 'react'; | ||
| import { render, screen, fireEvent, waitFor } from '@testing-library/react'; | ||
| import { I18nextProvider } from 'react-i18next'; |
| @@ -1,46 +1,69 @@ | |||
| import { describe, it, expect, beforeEach, afterEach, beforeAll, afterAll, mock, spyOn } from 'bun:test'; | |||
Check notice
Code scanning / CodeQL
Unused variable, import, function or class Note test
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 1 month ago
In general, to fix unused import issues, you either remove the unused imports or start using them meaningfully. Since there is no indication that beforeAll and afterAll are needed, the minimal, non‑behavior‑changing fix is to remove them from the named imports on line 1.
Concretely, in src/components/__tests__/Settings.test.tsx, update the Bun test import on line 1 by deleting beforeAll and afterAll from the destructuring import list, leaving only the identifiers that are actually used in the file (describe, it, expect, beforeEach, afterEach, mock, spyOn). No other code changes or additional imports are required.
| @@ -1,4 +1,4 @@ | ||
| import { describe, it, expect, beforeEach, afterEach, beforeAll, afterAll, mock, spyOn } from 'bun:test'; | ||
| import { describe, it, expect, beforeEach, afterEach, mock, spyOn } from 'bun:test'; | ||
| import { render, screen, fireEvent, waitFor } from '@testing-library/react'; | ||
| import '@testing-library/jest-dom'; | ||
| import { ThemeProvider } from '@/contexts/ThemeContext'; |
| @@ -1,3 +1,4 @@ | |||
| import { describe, it, expect, beforeEach, afterEach, beforeAll, afterAll, mock, spyOn } from 'bun:test'; | |||
Check notice
Code scanning / CodeQL
Unused variable, import, function or class Note test
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 1 month ago
In general, to fix unused import issues, remove the specific bindings from the import statement if they are not referenced anywhere in the file. This avoids confusion and aligns the imports with actual usage.
Here, the single best fix is to adjust the import from 'bun:test' to only include the identifiers that are in use: describe, it, expect, beforeEach, and mock. We will remove afterEach, beforeAll, afterAll, and spyOn from the curly-brace import list on line 1 of src/components/__tests__/Sidebar.i18n.test.tsx. No additional methods, imports, or definitions are needed.
| @@ -1,4 +1,4 @@ | ||
| import { describe, it, expect, beforeEach, afterEach, beforeAll, afterAll, mock, spyOn } from 'bun:test'; | ||
| import { describe, it, expect, beforeEach, mock } from 'bun:test'; | ||
| import React from 'react'; | ||
| import { render, screen, fireEvent } from '@testing-library/react'; | ||
| import { I18nextProvider } from 'react-i18next'; |
| @@ -1,9 +1,9 @@ | |||
| import { describe, it, expect, beforeEach, afterEach, mock, spyOn } from 'bun:test'; | |||
Check notice
Code scanning / CodeQL
Unused variable, import, function or class Note test
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 1 month ago
To fix unused imports, remove the specific identifiers from the import list while leaving the rest of the import intact. This preserves all existing behavior and only cleans up unused symbols.
Concretely, in src/components/features/dataQuality/__tests__/DataQualityPanel.test.tsx, update the first import from 'bun:test' to drop afterEach and spyOn, leaving describe, it, expect, beforeEach, mock. No other code changes, new methods, or new imports are required.
| @@ -1,4 +1,4 @@ | ||
| import { describe, it, expect, beforeEach, afterEach, mock, spyOn } from 'bun:test'; | ||
| import { describe, it, expect, beforeEach, mock } from 'bun:test'; | ||
| import { render, screen, fireEvent, waitFor } from '@testing-library/react'; | ||
| import '@testing-library/jest-dom'; | ||
|
|
| @@ -1,10 +1,9 @@ | |||
| import { describe, it, expect, beforeEach, afterEach, mock, spyOn } from 'bun:test'; | |||
Check notice
Code scanning / CodeQL
Unused variable, import, function or class Note test
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 1 month ago
To fix this, remove the unused named imports afterEach and spyOn from the bun:test import statement. This keeps only the testing utilities that are actually used in the file, improving readability and satisfying the static analysis rule.
Specifically, in src/components/features/dataQuality/__tests__/GapDetection.test.tsx, edit line 1 to change:
import { describe, it, expect, beforeEach, afterEach, mock, spyOn } from 'bun:test';to:
import { describe, it, expect, beforeEach, mock } from 'bun:test';No additional methods, imports, or definitions are needed.
| @@ -1,4 +1,4 @@ | ||
| import { describe, it, expect, beforeEach, afterEach, mock, spyOn } from 'bun:test'; | ||
| import { describe, it, expect, beforeEach, mock } from 'bun:test'; | ||
| import { render, screen, fireEvent, waitFor } from '@testing-library/react'; | ||
| import '@testing-library/jest-dom'; | ||
|
|
| @@ -1,22 +1,24 @@ | |||
| import { describe, it, expect, beforeEach, afterEach, mock, spyOn } from 'bun:test'; | |||
Check notice
Code scanning / CodeQL
Unused variable, import, function or class Note test
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 1 month ago
To fix the problem, remove the unused imports afterEach and spyOn from the bun:test import statement at the top of src/components/features/todos/__tests__/TodoCard.test.tsx. This keeps the file’s behavior unchanged while eliminating unused symbols.
Concretely, in TodoCard.test.tsx, edit line 1 so that the named imports only include the identifiers that are actually used: describe, it, expect, beforeEach, and mock. No other code or imports need to be modified, since the unused names are not referenced anywhere else.
| @@ -1,4 +1,4 @@ | ||
| import { describe, it, expect, beforeEach, afterEach, mock, spyOn } from 'bun:test'; | ||
| import { describe, it, expect, beforeEach, mock } from 'bun:test'; | ||
| import { render, screen, fireEvent } from '@testing-library/react'; | ||
| import '@testing-library/jest-dom'; | ||
|
|
|
|
||
| import { ActivityTrackingService } from '../ActivityTrackingService'; | ||
| import { ActivityMonitor } from '../ActivityMonitor'; | ||
| import { DatabaseManager } from '../../../database/DatabaseManager'; |
Check notice
Code scanning / CodeQL
Unused variable, import, function or class Note test
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 1 month ago
In general, to fix an "Unused import" issue, you remove the import of the symbol that is not referenced anywhere in the file, as long as it is not needed for types or side effects. Here, the import import { DatabaseManager } from '../../../database/DatabaseManager'; on line 19 is never used: the test uses mock.module to mock the module and constructs its own mockDbManager object without referring to DatabaseManager. The best fix that does not alter functionality is to delete this unused import line and leave the rest of the test unchanged.
Concretely, in src/services/activity/__tests__/ActivityTrackingService.test.ts, remove line 19 containing the DatabaseManager import. No new imports, methods, or definitions are required, and no other lines need to be updated because nothing references DatabaseManager.
| @@ -16,7 +16,6 @@ | ||
|
|
||
| import { ActivityTrackingService } from '../ActivityTrackingService'; | ||
| import { ActivityMonitor } from '../ActivityMonitor'; | ||
| import { DatabaseManager } from '../../../database/DatabaseManager'; | ||
|
|
||
| describe('ActivityTrackingService', () => { | ||
| let service: ActivityTrackingService; |
| (Notification as jest.MockedClass<typeof Notification>).mockImplementationOnce(() => { | ||
| const consoleError = spyOn(console, 'error').mockImplementation(() => {}); | ||
| // Override the mock to throw an error | ||
| const originalNotification = Notification; |
Check notice
Code scanning / CodeQL
Unused variable, import, function or class Note test
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 1 month ago
In general, unused variables should be removed to avoid confusion and potential maintenance issues. Here, originalNotification is assigned but never read, and the test does not rely on it, so deleting the declaration will not change existing functionality.
Concretely, in src/services/notifications/__tests__/NotificationService.test.ts, within the "should handle notification errors gracefully" test, remove the line that declares const originalNotification = Notification; (line 230). Leave the reassignment of Notification on line 231 as-is, since the test depends on overriding the mock to throw an error. No new methods, imports, or definitions are needed; this is purely a deletion of the unused variable.
| @@ -227,7 +227,6 @@ | ||
| it('should handle notification errors gracefully', () => { | ||
| const consoleError = spyOn(console, 'error').mockImplementation(() => {}); | ||
| // Override the mock to throw an error | ||
| const originalNotification = Notification; | ||
| (Notification as any) = function() { | ||
| throw new Error('Notification failed'); | ||
| }; |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 869fd77792
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| "dev:electron": "bunx wait-on http://localhost:5173 && electron dist/main/main.js", | ||
| "build": "bun run build:react && bun run build:electron && bun run build:mcp", | ||
| "build:react": "bunx vite build", | ||
| "build:electron": "bun build src/main/main.ts --outdir dist/main --target node --external electron --external better-sqlite3 --external @sentry/electron --external @sentry/electron/main && bun build src/main/preload.ts --outfile dist/preload.js --target browser --external electron", |
There was a problem hiding this comment.
bun build defaults to --format=esm (per bun build --help), so this build:electron command will emit ESM output with import statements for externals. Because package.json has no "type": "module" and start runs electron dist/main/main.js, Electron will load those .js files as CommonJS and throw SyntaxError: Cannot use import statement outside a module. This will break app launch in any environment using the current scripts unless you add --format=cjs or switch the app to ESM (type: module / .mjs).
Useful? React with 👍 / 👎.
| "build": "bun run build:react && bun run build:electron && bun run build:mcp", | ||
| "build:react": "bunx vite build", | ||
| "build:electron": "bun build src/main/main.ts --outdir dist/main --target node --external electron --external better-sqlite3 --external @sentry/electron --external @sentry/electron/main && bun build src/main/preload.ts --outfile dist/preload.js --target browser --external electron", | ||
| "build:mcp": "bun build src/mcp/server.ts --outdir dist/mcp/mcp --target node --external better-sqlite3 && cp src/mcp/package.json dist/mcp/mcp/package.json && chmod +x dist/mcp/mcp/server.js", |
There was a problem hiding this comment.
On Windows, bun run build (used by the Windows job in .github/workflows/build.yml) will execute this build:mcp script, but cp and chmod +x are not available in the default Windows shells. That means the Windows build will fail even if the TypeScript bundle succeeds. The previous Node helper handled this cross‑platform; consider restoring a script-based approach or using a cross-platform CLI.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 17
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (20)
src/services/pomodoro/__tests__/PomodoroService.test.ts (2)
348-401: Incomplete migration:jest.advanceTimersByTimereferences will cause runtime failures.These tests still reference
jest.advanceTimersByTime()but Jest has been removed from the project. This will throwReferenceError: jest is not definedat runtime.Bun does not currently have built-in fake timer support equivalent to Jest's. Options to address this:
- Use
@sinonjs/fake-timers— A standalone library compatible with Bun.- Reduce timer durations for testing — Inject shorter durations via settings in tests.
- Skip or mark tests as TODO — Until Bun adds fake timer support.
Example fix using `@sinonjs/fake-timers`
First, install the package:
bun add -d `@sinonjs/fake-timers` `@types/sinonjs__fake-timers`Then update the test file:
import { describe, it, expect, beforeEach, afterEach, mock, spyOn } from 'bun:test'; +import FakeTimers from '@sinonjs/fake-timers'; import { PomodoroService } from '../PomodoroService'; // ... other imports describe('PomodoroService', () => { let service: PomodoroService; let mockDbManager: any; let mockNotificationService: any; let consoleLog: ReturnType<typeof spyOn>; + let clock: FakeTimers.InstalledClock; beforeEach(() => { + clock = FakeTimers.install(); consoleLog = spyOn(console, 'log').mockImplementation(() => {}); // ... rest of setup }); afterEach(() => { consoleLog.mockRestore(); service.destroy(); + clock.uninstall(); }); // In tests, replace: // jest.advanceTimersByTime(1000); // with: // clock.tick(1000);
403-461: Same issue:jest.advanceTimersByTimecalls remain in session progression tests.All tests in this block rely on
jest.advanceTimersByTime()which is undefined after the Jest removal. These tests exercise critical session state machine logic and will fail.src/main/services/__tests__/ServiceContainer.test.ts (3)
254-283: Error handling tests also use invalid import reassignment.These tests attempt to override mocks per-test by reassigning imports, which won't work. You'll need an alternative approach for per-test mock customization in Bun.
Consider using Bun's
mock.module()with a shared mock reference that can be reconfigured, or restructure these tests into separatedescribeblocks with different top-levelmock.module()configurations. Alternatively, you can mock at the instance level aftercontainer.initialize()is called, similar to what's done in lines 308-309 and 340-342.
440-444: Jest type reference in Bun test file.
jest.MockedClassis a Jest-specific type that doesn't exist in Bun's test runtime. This will cause a TypeScript error or runtime issue.Proposed fix: Use Bun-compatible type casting
it('should resolve PomodoroService with all required dependencies', async () => { await container.initialize('/test/path', mockSettings); - const calls = (PomodoroService as jest.MockedClass<typeof PomodoroService>).mock.calls; + const calls = (PomodoroService as any).mock.calls; expect(calls.length).toBe(1); expect(calls[0]).toHaveLength(3); // DatabaseManager, NotificationService, settings expect(calls[0][2]).toEqual(mockSettings.pomodoro); });
459-476: Async category initialization test also uses invalid import reassignment.Line 460 reassigns
CategoriesServicewhich won't work as discussed above.This test needs to be restructured. One option is to move it to a separate test file with its own
mock.module()configuration, or use a different testing pattern that doesn't require replacing the module mock mid-suite.src/services/goals/__tests__/GoalsService.test.ts (1)
5-635: Fix test assertions to match the implementation's logger-based approach.The test file expects
console.logcalls with specific messages ("Goal created", "Progress updated", "Notification history cleared"), but theGoalsServiceimplementation uses the structuredloggerservice for error handling and performs no success logging. These tests will fail when executed:
- Line 79 (
addGoalsuccess test): expectsconsole.logcontaining "Goal created" → not logged- Line 248 (
updateGoalProgresstest): expectsconsole.logcontaining "Progress updated" → not logged- Line 579 (
clearNotificationHistorytest): expectsconsole.logcontaining "Notification history cleared" → not loggedEither update the tests to remove these
console.logassertions (since success paths don't log), or add explicit logging to the service implementation where the tests expect it.src/services/activity/__tests__/ActivityTrackingService.test.ts (1)
436-439: Importjestfrombun:testto enableadvanceTimersByTime.Line 438 calls
jest.advanceTimersByTime()butjestis not imported. Addjestto the import statement on line 1:import { describe, it, expect, beforeEach, afterEach, mock, spyOn, jest } from 'bun:test';src/services/notifications/__tests__/NotificationService.test.ts (1)
226-244: Cannot reassign an imported binding — test will fail.Line 231 attempts to reassign
Notification, which is a read-only ES module import. This violates ES module semantics and will either throw aTypeErroror silently fail. Additionally,originalNotification(line 230) is declared but never used to restore the mock.To test error handling, you need to conditionally throw from within the mock or use a separate mock.module override for this specific test.
🐛 Proposed fix using a throwable mock pattern
describe('Error handling', () => { it('should handle notification errors gracefully', () => { const consoleError = spyOn(console, 'error').mockImplementation(() => {}); - // Override the mock to throw an error - const originalNotification = Notification; - (Notification as any) = function() { - throw new Error('Notification failed'); - }; + + // Use a flag to control error throwing from the mock + let shouldThrow = true; + mock.module('electron', () => ({ + Notification: function(this: any, options: any) { + if (shouldThrow) { + throw new Error('Notification failed'); + } + this.title = options.title; + this.body = options.body; + this.silent = options.silent; + this.show = mockShow; + mockNotificationInstances.push(this); + return this; + }, + })); service = new NotificationService(true, true); service.notifyFocusComplete('Test Task'); expect(consoleError).toHaveBeenCalledWith( expect.stringContaining('Failed to show notification'), expect.any(Error) ); consoleError.mockRestore(); + shouldThrow = false; }); });Alternatively, consider restructuring the test file to isolate the error scenario in a separate describe block with its own mock.module setup.
src/main/ipc/handlers/__tests__/TodosHandlers.test.ts (3)
75-113: Pass the expectedAddTodoArgsshape to the handler.
The handler destructures{ todo }, but the test passes the todo object directly, soaddTodoreceivesundefined.🐛 Proposed fix
- const result = await handler!(null, mockTodo); + const result = await handler!(null, { todo: mockTodo }); - const result = await handler!(null, { title: 'Test' }); + const result = await handler!(null, { todo: { title: 'Test' } }); - const result = await handler!(null, { title: 'Test' }); + const result = await handler!(null, { todo: { title: 'Test' } });
121-165:update-todotests must send{ id, updates }as a single arg.
The handler expects an args object; current calls pass positional values and won’t validate real behavior.🐛 Proposed fix
- const result = await handler!(null, 1, updates); + const result = await handler!(null, { id: 1, updates }); - const result = await handler!(null, 999, { title: 'Test' }); + const result = await handler!(null, { id: 999, updates: { title: 'Test' } }); - const result = await handler!(null, 1, { title: 'Test' }); + const result = await handler!(null, { id: 1, updates: { title: 'Test' } }); - const result = await handler!(null, 1, { title: 'Test' }); + const result = await handler!(null, { id: 1, updates: { title: 'Test' } });
169-213:delete-todotests must send{ id }as a single arg.
Handlers expect an args object; positional values won’t exercise the real path.🐛 Proposed fix
- const result = await handler!(null, 1); + const result = await handler!(null, { id: 1 }); - const result = await handler!(null, 999); + const result = await handler!(null, { id: 999 }); - const result = await handler!(null, 1); + const result = await handler!(null, { id: 1 }); - const result = await handler!(null, 1); + const result = await handler!(null, { id: 1 });src/components/__tests__/ErrorBoundary.i18n.test.tsx (1)
235-268: Restorewindow.locationafter the reload mock to avoid test leakage.🧪 Suggested fix
- const reloadFn = mock(() => {}); + const reloadFn = mock(() => {}); + const originalLocation = window.location; Object.defineProperty(window, 'location', { writable: true, value: { reload: reloadFn }, }); @@ fireEvent.click(refreshButton); expect(reloadFn).toHaveBeenCalled(); + + // Restore original location to keep tests isolated + Object.defineProperty(window, 'location', { + writable: true, + value: originalLocation, + });src/services/activity/__tests__/ActivityMonitor.test.ts (3)
110-118: Jest API used instead of Bun'sspyOn.
jest.spyOnis not available in Bun. This test will fail. Use thespyOnfunction already imported frombun:test.Proposed fix
it('should restart monitor with new interval when tracking', () => { monitor.start(); - const stopSpy = jest.spyOn(monitor, 'stop'); - const startSpy = jest.spyOn(monitor, 'start'); + const stopSpy = spyOn(monitor, 'stop'); + const startSpy = spyOn(monitor, 'start'); monitor.setInterval(20000); expect(stopSpy).toHaveBeenCalled(); expect(startSpy).toHaveBeenCalled(); expect((monitor as any).intervalMs).toBe(20000); + + stopSpy.mockRestore(); + startSpy.mockRestore(); });
388-405: Jest timer API used —jest.advanceTimersByTimeis not available in Bun.Line 403 uses
jest.advanceTimersByTime(1000)which is a Jest-specific API. Bun's test runner does not have fake timers. This test will fail.Either remove the fake timer assertion, use real timers with
setTimeout/await, or skip this test until Bun adds fake timer support.Proposed fix — remove the Jest timer call
it('should cleanup and stop tracking when stop() is called during capture', async () => { // Simulate a long-running capture by not calling the callback (mockExec as any) = mock((_cmd: string, _callback: any) => { // Never call callback }); monitor.start(); expect(monitor.isTracking()).toBe(true); // Call stop while capture is in progress monitor.stop(); expect(monitor.isTracking()).toBe(false); - // Optionally, check that no further captures are scheduled - jest.advanceTimersByTime(1000); - expect(monitor.isTracking()).toBe(false); + // Monitor should remain stopped + await new Promise(resolve => setTimeout(resolve, 100)); + expect(monitor.isTracking()).toBe(false); });
164-176: ReassigningmockExecwon't update the module mock.Even after fixing
consttolet, reassigningmockExecinside helper functions won't change whatmock.modulereturned. The module mock is captured at registration time. Consider using a wrapper pattern:Alternative mocking approach
// At the top level, create a holder object const mockHolder = { exec: mock(() => {}) }; mock.module('child_process', () => ({ exec: (...args: any[]) => mockHolder.exec(...args), })); // Then in helpers: const mockExecSuccess = (stdout: string) => { mockHolder.exec = mock((_cmd: string, callback: any) => { callback(null, { stdout, stderr: '' }); }); };__tests__/package.validation.test.ts (2)
267-272: Test will fail due to@types/bun: "latest".The version format validation on line 270 expects versions matching
/^[\^~]?\d+\.\d+\.\d+/, butpackage.jsonhas"@types/bun": "latest"which won't match. Either update the package.json to pin a version (recommended), or add an exception for "latest" in the test.Option 1: Add exception for "latest" (if keeping latest)
it('should have valid version formats', () => { Object.entries(packageJson.devDependencies).forEach(([name, version]) => { expect(typeof version).toBe('string'); - expect(version).toMatch(/^[\^~]?\d+\.\d+\.\d+/); + // Allow "latest" for `@types/bun`, otherwise require semver + if (version !== 'latest') { + expect(version).toMatch(/^[\^~]?\d+\.\d+\.\d+/); + } }); });
395-403: Same issue: test requires semver but@types/bunuses "latest".The security and best practices check on line 401 will also fail for
"@types/bun": "latest".src/components/__tests__/Settings.test.tsx (2)
45-94: Align mocked settings with theSettingstype.
defaultCategoryisnumber | nullinsrc/main/core/SettingsManager.ts. Using''can hide type issues and mismodel UI state.✅ Suggested fix
- defaultCategory: '', + defaultCategory: null, @@ - defaultCategory: '', + defaultCategory: null,
369-412: Use translation keys consistently for the Save button label.At lines 378 and 396, the tests search for the literal text
"Save Settings", but the component renders the translation key'settings.saveSettings'(due to the i18n mock returning keys as-is). These assertions will fail at runtime.Suggested fix
- const saveButton = screen.getByText('Save Settings') as HTMLButtonElement; + const saveButton = screen.getByText('settings.saveSettings') as HTMLButtonElement;src/database/__tests__/DatabaseManager.test.ts (1)
138-146: Replacejest.clearAllMocks()with the Bun mock API.
jestisn't available inbun:test, so this will fail at runtime. UseDatabase.mockClear()to clear the constructor mock for this test.🐛 Proposed fix
- jest.clearAllMocks(); + Database.mockClear();
🤖 Fix all issues with AI agents
In @.github/workflows/bundle-size.yml:
- Around line 26-35: The "Setup Bun" step currently uses bun-version: latest
which makes CI non-deterministic; update the setup action configuration in the
workflow step named "Setup Bun" to pin a specific Bun version (e.g., "1.3.6") or
switch to using bun-version-file: ".bun-version" so CI matches local tooling;
edit the step that uses oven-sh/setup-bun@v2 (the "Setup Bun" step) to replace
bun-version: latest with a fixed version string or add bun-version-file to
ensure repeatable builds.
In `@README.md`:
- Around line 81-83: README currently lists "Bun 1.0+ (recommended) or Node.js
18+" but the project scripts in package.json use bun and bunx exclusively;
update the Prerequisites to state that Bun is required (remove the Node.js
fallback) or alternatively add documented npm/Node.js equivalents for all
scripts that call "bun" and "bunx" in package.json (e.g., replace bun
run/test/build usages with npm/yarn equivalents and document any CLI
differences). Ensure you reference the package.json scripts that invoke "bun"
and "bunx" when making the README change.
In `@src/components/__tests__/ErrorBoundary.i18n.test.tsx`:
- Line 1: The import line in ErrorBoundary.i18n.test.tsx currently imports many
test helpers (describe, it, expect, beforeEach, afterEach, beforeAll, afterAll,
mock, spyOn); static analysis flagged some as unused—open the test file, check
which of these symbols are actually referenced (e.g., used in tests or
setup/teardown) and remove the unused names from the import list so only the
used symbols remain; update the import statement that currently lists describe,
it, expect, beforeEach, afterEach, beforeAll, afterAll, mock, spyOn to include
only the referenced helpers.
In `@src/components/__tests__/FocusMode.test.tsx`:
- Line 1: The test file imports unused symbols beforeAll and afterAll from
'bun:test'; remove beforeAll and afterAll from the import list (leaving
describe, it, expect, beforeEach, afterEach, mock, spyOn) so the import
statement no longer includes unused identifiers and static analysis warnings go
away; update the import in the FocusMode.test.tsx file where the import line
contains beforeAll and afterAll.
- Around line 466-473: The test uses mockElectronAPI.pomodoro.sessions.getStats
but sets totalFocusTime and totalBreakTime as minutes while earlier tests use
seconds; update the mock in the getStats call to supply totalFocusTime and
totalBreakTime in seconds (values divisible by 60 to preserve minute
conversions) and update the inline comment to reflect seconds, so assertions
that expect minute-derived values remain correct.
In `@src/components/__tests__/Settings.test.tsx`:
- Line 1: The import line in Settings.test.tsx currently includes unused symbols
beforeAll and afterAll; remove them from the named import so only used test
helpers remain (e.g., keep describe, it, expect, beforeEach, afterEach, mock,
spyOn) by editing the import statement that imports from 'bun:test' to drop
beforeAll and afterAll.
In `@src/components/features/analytics/__tests__/HourlyHeatmap.test.tsx`:
- Line 1: The test import line currently includes unused symbols (beforeEach,
afterEach, spyOn); remove these unused imports from the import statement so it
only imports the actually used symbols (describe, it, expect, mock) to eliminate
linter/test warnings and keep the file tidy.
In `@src/components/features/analytics/__tests__/InsightCard.test.tsx`:
- Line 1: The test file imports unused symbols; update the import statement in
InsightCard.test.tsx to remove beforeEach, afterEach, and spyOn so it only
imports the actually used symbols (e.g., describe, it, expect, mock) — locate
the import line that currently declares describe, it, expect, beforeEach,
afterEach, mock, spyOn and delete the unused names to clean up the test imports.
In `@src/components/features/todos/__tests__/TodoCard.test.tsx`:
- Line 1: The import statement brings in unused symbols causing lint noise:
remove afterEach and spyOn from the import list (the line that currently imports
describe, it, expect, beforeEach, afterEach, mock, spyOn from 'bun:test') so it
only imports the actually used symbols (describe, it, expect, beforeEach, mock);
update the import to drop afterEach and spyOn.
In `@src/components/ui/__tests__/ProgressListCard.test.tsx`:
- Around line 1-3: Remove the redundant import of '@testing-library/jest-dom'
from the top of the test file; keep the existing imports (describe, it, expect
from 'bun:test' and render, screen from '@testing-library/react') intact since
the jest-dom matchers are already loaded via the global preload
(src/setupTests.ts), so delete the import statement that references
'@testing-library/jest-dom'.
In `@src/database/__tests__/DatabaseManager.test.ts`:
- Around line 1-16: The test currently reassigns the imported Database
constructor which breaks ESM bindings; instead, declare module‑scope variables
like mockDb and mockDatabaseCtor (e.g., let mockDb: any; let mockDatabaseCtor =
mock(() => mockDb)) and register mockDatabaseCtor in
mock.module('better-sqlite3') so the module uses the ctor, then in beforeEach
replace mockDb with a fresh object (with prepare/exec/close/pragma) to mutate
behavior per test; remove the inner redeclaration of mockDb inside describe and
drop the jest.clearAllMocks() call, using Bun's mock teardown or manual resets
as needed.
In `@src/main/ipc/handlers/__tests__/TodosHandlers.test.ts`:
- Around line 44-45: The test is spying on console.log/console.error
(consoleLog/consoleError) but TodosHandlers uses the app logger (logger.error),
so those spies/expectations never match; replace the console spies at the three
locations (lines shown) with spies/mocks against the logger used by
TodosHandlers (spy on the exported logger module/method that TodosHandlers
imports) or remove the console assertions entirely, and update all related
expectations to assert against logger.error (and any logger.info/debug) instead
of console.* so the tests reflect production behavior.
In `@src/main/services/__tests__/ServiceContainer.test.ts`:
- Around line 86-121: The test file incorrectly attempts to reassign ES module
imports (e.g., DatabaseManager, NotificationService, GoalsService,
CategoriesService, ActivityValidationService, ActivityMergeService,
ActivityTrackingService, PomodoroService) inside beforeEach using (X as any) =
mock(...), which is invalid; remove these reassignment blocks and rely on the
existing mock.module() calls at the top of the file for module-level mocks, or
if per-test customization is needed convert those module mocks to factories via
mock.module() so you can configure returned mock implementations in each test
instead of reassigning imports.
In `@src/services/activity/__tests__/ActivityMonitor.test.ts`:
- Around line 5-8: The issue is that mockExec is declared as const but
reassigned later, causing runtime errors; change the declaration of mockExec
from const to let and update the module mock usage so the child_process mock
always points to the current mockExec instance (e.g., re-register
mock.module('child_process', ...) or use a mutable wrapper/holder variable
passed into mock.module) and ensure test setup/teardown updates or restores the
mock before each test so reassignments to mockExec take effect everywhere that
uses the child_process exec mock.
In `@src/services/activity/__tests__/ActivityTrackingService.test.ts`:
- Line 19: Remove the unused import DatabaseManager from the test file: delete
the line importing DatabaseManager since it is not referenced anywhere in
ActivityTrackingService.test (remove the symbol "DatabaseManager" from the
import list or the entire import statement if nothing else is imported), then
run tests/lint to confirm no references remain.
- Line 48: Remove the redundant reassignment of the ESM import binding by
deleting the line "(ActivityMonitor as any) = mock(() => mockMonitor);" — the
module mock is already configured via mock.module (lines where mock.module is
used) so avoid attempting to reassign the imported ActivityMonitor; locate
references to ActivityMonitor in ActivityTrackingService.test (the test setup
using mock.module and mockMonitor) and ensure only the mock.module configuration
is used to inject mockMonitor.
In `@src/services/notifications/__tests__/NotificationService.test.ts`:
- Around line 4-17: The test's mock for Electron's Notification must be a spy
and isolated per test: wrap the constructor with mock() when registering it in
mock.module so assertions like expect(Notification).toHaveBeenCalledWith(...)
work, ensure mockShow is reset between tests (call mockReset/mockClear in
beforeEach/afterEach) to avoid accumulated call counts, and for the
error-handling test do not reassign the imported Notification binding—instead
change the mock implementation to throw (e.g., set the mock module's
Notification to a throwing mock for that test) so the test simulates constructor
failure without violating ES module semantics; update references in the test
file (mockShow, Notification) accordingly.
♻️ Duplicate comments (4)
src/services/goals/__tests__/GoalsService.test.ts (1)
306-311: Same synchronous mock pattern noted.Same consideration as line 274 regarding sync vs async returns. The verification script above will cover these methods as well.
.github/workflows/coverage.yml (1)
24-27: Pin Bun version (same concern as other workflows).What is the latest stable Bun 1.x release and the recommended pin format for oven-sh/setup-bun@v2?.github/workflows/build.yml (1)
30-33: Pin Bun version to avoid non-reproducible builds.What is the latest stable Bun 1.x release and the recommended pin format for oven-sh/setup-bun@v2?.github/workflows/ci.yml (1)
18-22: Pin Bun version across CI jobs for determinism.What is the latest stable Bun 1.x release and the recommended pin format for oven-sh/setup-bun@v2?Also applies to: 38-42, 67-71, 95-99, 116-120
🧹 Nitpick comments (21)
src/components/__tests__/Sidebar.i18n.test.tsx (1)
1-1: Remove unused imports.Static analysis correctly identifies that
afterEach,beforeAll,afterAll, andspyOnare imported but never used in this file.🧹 Proposed fix
-import { describe, it, expect, beforeEach, afterEach, beforeAll, afterAll, mock, spyOn } from 'bun:test'; +import { describe, it, expect, beforeEach, mock } from 'bun:test';src/components/features/analytics/__tests__/ProductivityLineChart.test.tsx (1)
1-1: Remove unused imports.The imports
beforeEach,afterEach, andspyOnare not used in this test file. As flagged by static analysis, these should be removed to keep the import statement clean.🧹 Suggested cleanup
-import { describe, it, expect, beforeEach, afterEach, mock, spyOn } from 'bun:test'; +import { describe, it, expect, mock } from 'bun:test';src/services/pomodoro/__tests__/PomodoroService.test.ts (1)
358-370: Minor: Redundant await on already-resolved promise.The
eventPromisewill have resolved during the 1100mssetTimeoutwait, making the finalawait eventPromisea no-op. While functionally harmless, the intent is clearer if structured differently.Suggested simplification
it('should emit tick event', async () => { service.start('Test Task', 'focus'); const eventPromise = new Promise<void>((resolve) => { service.on('tick', () => { resolve(); }); }); - // Wait a bit for the tick event - await new Promise(resolve => setTimeout(resolve, 1100)); - await eventPromise; + // Wait for the tick event (fires after ~1 second) + await eventPromise; });This relies solely on the event firing, with an implicit timeout from the test framework if it never does.
src/components/features/dataQuality/__tests__/DataQualityPanel.test.tsx (3)
1-1: Remove unused importsafterEachandspyOn.Static analysis correctly identifies these as unused. However,
spyOncould actually be useful—see the comment on lines 385-387 below.-import { describe, it, expect, beforeEach, afterEach, mock, spyOn } from 'bun:test'; +import { describe, it, expect, beforeEach, mock, spyOn } from 'bun:test';
382-405: UsespyOnfor cleaner console.error mocking.The manual save/replace/restore pattern is error-prone—if the test throws before line 404,
console.errorremains mocked for subsequent tests. SincespyOnis already imported, use it for cleaner implementation:♻️ Suggested refactor
it('handles merge failure gracefully', async () => { mockElectronAPI.activities.bulk.update = mock(() => Promise.resolve({ success: false })); - const originalConsoleError = console.error; - console.error = mock(() => {}); - const consoleErrorSpy = console.error; + const consoleErrorSpy = spyOn(console, 'error').mockImplementation(() => {}); render( <DataQualityPanel startDate={mockStartDate} endDate={mockEndDate} onRefreshActivities={mockOnRefreshActivities} /> ); fireEvent.click(screen.getByText('Duplicate Detection')); fireEvent.click(screen.getByText('Merge Activities')); await waitFor(() => { expect(consoleErrorSpy).toHaveBeenCalled(); }); - console.error = originalConsoleError; + consoleErrorSpy.mockRestore(); });Alternatively, wrap the cleanup in a
try/finallyblock if you prefer the current approach.
518-539: Consider wrapping electronAPI restoration in atry/finallyblock.If the
waitForassertion throws, line 538 won't execute andelectronAPIremainsundefinedfor subsequent tests. WhilebeforeEachdoesn't currently restore this, it's good practice to ensure cleanup:♻️ Optional improvement
it('handles missing electronAPI gracefully', async () => { (global as any).window.electronAPI = undefined; + try { render( <DataQualityPanel startDate={mockStartDate} endDate={mockEndDate} /> ); fireEvent.click(screen.getByText('Duplicate Detection')); fireEvent.click(screen.getByText('Merge Activities')); // Should not throw error await waitFor(() => { expect(screen.getByTestId('duplicate-detection')).toBeInTheDocument(); }); - - // Restore - (global as any).window.electronAPI = mockElectronAPI; + } finally { + (global as any).window.electronAPI = mockElectronAPI; + } });src/components/features/dataQuality/__tests__/GapDetection.test.tsx (3)
1-1: Remove unused importsafterEachandspyOn.These imports are not used anywhere in the test file.
Proposed fix
-import { describe, it, expect, beforeEach, afterEach, mock, spyOn } from 'bun:test'; +import { describe, it, expect, beforeEach, mock } from 'bun:test';
539-554: Redundant assertion — mock was never wired to the component.The test renders the component without passing
onCreateActivity, somockOnCreateActivity(created inbeforeEach) is never connected to the component. The assertion on line 553 will always pass regardless of component behavior.The test's actual value is verifying that clicking "Fill Gap" doesn't throw when the callback is absent. Consider removing the misleading assertion or adding a clearer comment.
Suggested clarification
await waitFor(() => { const fillButtons = screen.getAllByText('Fill Gap'); fireEvent.click(fillButtons[0]!); }); - // Should not throw error - expect(mockOnCreateActivity).not.toHaveBeenCalled(); + // Test passes if no error is thrown - the callback was intentionally not provided });
624-642: UseafterEachfor reliable cleanup of global state.This test sets
window.electronAPI = undefinedand manually restores it at the end. If the test fails or throws before line 641, the global state remains modified, potentially causing subsequent tests to fail unexpectedly.Since
afterEachwas already imported (though currently unused), consider using it to guarantee cleanup.Suggested approach
Add cleanup in the describe block or at file level:
afterEach(() => { // Ensure electronAPI is always restored (global as any).window.electronAPI = mockElectronAPI; });Then remove the manual restore from the test:
await waitFor(() => { // Should show loading spinner indefinitely or handle gracefully const spinner = document.querySelector('.animate-spin'); expect(spinner).toBeInTheDocument(); }); - - // Restore - (global as any).window.electronAPI = mockElectronAPI; });src/services/goals/__tests__/GoalsService.test.ts (1)
229-242: Consider using a consistent mock pattern throughout the file.The file mixes two approaches:
- Reassignment:
mockDb.updateGoalProgress = mock(() => Promise.resolve(undefined))(line 229)- Method chaining:
mockDb.getGoals.mockResolvedValue([...])(line 230)Both work, but reassignment creates a new mock instance while
mockResolvedValuemodifies the existing one. For consistency and easier debugging, consider standardizing on one approach.♻️ Example: Standardize on mockResolvedValue
- mockDb.updateGoalProgress = mock(() => Promise.resolve(undefined)); - mockDb.getGoals.mockResolvedValue([ + mockDb.updateGoalProgress.mockResolvedValue(undefined); + mockDb.getGoals.mockResolvedValue([src/database/__tests__/DatabaseManager.pomodoro.test.ts (1)
5-13: Tighten mock state typing to avoidanybleed-through.
Use a typed session shape so the mock matchesPomodoroSession+idand catches typos at compile time.♻️ Proposed refactor
-let sessions: Map<number, any>; +type StoredPomodoroSession = PomodoroSession & { id: number }; +let sessions: Map<number, StoredPomodoroSession>; let nextId: number;src/components/features/analytics/__tests__/CalendarHeatmap.test.tsx (1)
1-1: Remove unused imports.
beforeEach,afterEach, andspyOnare imported but not used in this test file.🧹 Proposed fix
-import { describe, it, expect, beforeEach, afterEach, mock, spyOn } from 'bun:test'; +import { describe, it, expect, mock } from 'bun:test';src/components/features/analytics/__tests__/ChartCard.test.tsx (1)
1-1: Remove unused imports.
beforeEach,afterEach, andspyOnare imported but not used in this test file.🧹 Proposed fix
-import { describe, it, expect, beforeEach, afterEach, mock, spyOn } from 'bun:test'; +import { describe, it, expect, mock } from 'bun:test';.github/workflows/nightly.yml (1)
60-63: Consider pinning Bun version for CI reproducibility.Using
bun-version: latestin CI can lead to unexpected build failures when new Bun versions are released. Consider pinning to a specific version (e.g.,1.2.0) for more predictable builds.Suggested change
- name: Setup Bun uses: oven-sh/setup-bun@v2 with: - bun-version: latest + bun-version: "1.2.0"src/components/__tests__/Settings.i18n.test.tsx (2)
1-1: Remove unused imports.
afterAll,afterEach, andbeforeAllare imported but onlybeforeEachis used in this file. As flagged by CodeQL, remove the unused imports.Proposed fix
-import { describe, it, expect, beforeEach, afterEach, beforeAll, afterAll, mock, spyOn } from 'bun:test'; +import { describe, it, expect, beforeEach, mock, spyOn } from 'bun:test';
26-39: Mock settings object doesn't match theSettingsinterface.The
defaultCategoryfield is set to''(empty string), but according toSettingsinterface inSettingsManager.ts, it should benumber | null. Also, fields likeautoStartOnLoginare missing. While tests may still pass, this could mask type-related bugs.Suggested fix for type alignment
const mockElectronAPI = { getSettings: mock(() => Promise.resolve({ autoTrackApps: true, showNotifications: true, minimizeToTray: false, autoStartTracking: false, - defaultCategory: '', + defaultCategory: null, + autoStartOnLogin: false, trackingInterval: 60, crashReporting: true, dataLocation: '/path/to/data', })),Also applies to: 57-68
package.json (1)
111-111: Pin@types/bunto a specific version.Using
"latest"for type definitions can cause unexpected TypeScript errors when new versions are published. This is especially problematic in CI where builds should be reproducible.Suggested fix
- "@types/bun": "latest", + "@types/bun": "^1.3.6",src/components/__tests__/App.test.tsx (1)
101-104: Remove or implement the emptybeforeEachhook.It’s currently a no‑op and can mislead readers. Either delete it or add actual reset logic if you intend to clear mocks between tests.
🧹 Minimal cleanup
- beforeEach(() => { - // Clear mocks before each test - });src/i18n/__tests__/locales.theme.test.ts (1)
1-4: Use ESM imports for JSON files to match existing locale tests.The project's other locale test file (
locales.test.ts) already uses ESM imports. Aligning this file improves consistency, avoids mixingrequirewith ESM in test files, and enables better type inference with Bun's native JSON import support.♻️ Suggested update
-import { describe, it, expect } from 'bun:test'; - -const enLocale = require('../locales/en.json'); -const arLocale = require('../locales/ar.json'); +import { describe, it, expect } from 'bun:test'; +import enLocale from '../locales/en.json'; +import arLocale from '../locales/ar.json';src/contexts/__tests__/PomodoroContext.test.tsx (1)
456-518: Real-time waits can make polling tests slow/flaky.These polling tests sleep for 1–2s using real timers, adding latency and flakiness in CI. The polling interval is hardcoded at 1000ms. Consider either:
- Making the polling interval injectable for tests so you can use shorter intervals during testing
- Using
jest.useFakeTimers()andjest.runAllTimers()(available in bun:test) to advance time deterministically instead of waiting for real delayssrc/components/__tests__/Settings.test.tsx (1)
325-345: Replace the real 3-second wait with fake timers.The 3-second
setTimeoutcall makes the test slow and potentially flaky. Bun'sbun:testsupports Jest-compatible fake timers viajest.useFakeTimers()andjest.advanceTimersByTime(ms). Replace the sleep with:jest.useFakeTimers(); // ... test code ... jest.advanceTimersByTime(3100); // ... assertions ... jest.useRealTimers();Alternatively, use
jest.runAllTimers()to run all pending timers without specifying a duration.
| - name: Setup Bun | ||
| uses: oven-sh/setup-bun@v2 | ||
| with: | ||
| node-version: '20' | ||
| cache: 'npm' | ||
| bun-version: latest | ||
|
|
||
| - name: Install dependencies (PR) | ||
| run: npm ci | ||
| run: bun install --frozen-lockfile | ||
|
|
||
| - name: Build application (PR) | ||
| run: npm run build | ||
| run: bun run build |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Bun latest stable 1.x version and oven-sh/setup-bun recommended version pinning
💡 Result:
- Latest stable Bun 1.x: Bun v1.3.6 (released January 13, 2026). [1]
oven-sh/setup-bunversion pinning (recommended for CI reproducibility):- Pin the action to a major:
uses: oven-sh/setup-bun@v2(so you get fixes without breaking changes). [2] - Pin Bun via either:
- an explicit version:
bun-version: "1.3.6"(most reproducible), or [2] - a version file (recommended for keeping local + CI in sync):
bun-version-file: ".bun-version"(also supports e.g..tool-versions/package.json). [2]
- an explicit version:
- Pin the action to a major:
Sources:
[1] GitHub Releases: oven-sh/bun (Bun v1.3.6 marked Latest) (github.com)
[2] oven-sh/setup-bun README / Marketplace usage + bun-version-file (github.com)
Citations:
- 1: https://github.com/oven-sh/bun/releases
- 2: https://github.com/oven-sh/setup-bun?utm_source=openai
🏁 Script executed:
git ls-files ".github/workflows/bundle-size.yml"Repository: osama1998H/lume
Length of output: 92
🏁 Script executed:
cat -n .github/workflows/bundle-size.yml | sed -n '20,80p'Repository: osama1998H/lume
Length of output: 2228
🏁 Script executed:
grep -n "setup-bun" .github/workflows/bundle-size.ymlRepository: osama1998H/lume
Length of output: 96
🏁 Script executed:
cat .github/workflows/bundle-size.yml | head -100Repository: osama1998H/lume
Length of output: 2926
Pin Bun to a specific version for deterministic CI behavior.
Using bun-version: latest allows breaking changes to silently slip into CI runs without code changes. Pin to an explicit version such as "1.3.6" or use bun-version-file: ".bun-version" to sync with local development:
Example fix (line 29):
- name: Setup Bun
uses: oven-sh/setup-bun@v2
with:
- bun-version: latest
+ bun-version: "1.3.6"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - name: Setup Bun | |
| uses: oven-sh/setup-bun@v2 | |
| with: | |
| node-version: '20' | |
| cache: 'npm' | |
| bun-version: latest | |
| - name: Install dependencies (PR) | |
| run: npm ci | |
| run: bun install --frozen-lockfile | |
| - name: Build application (PR) | |
| run: npm run build | |
| run: bun run build | |
| - name: Setup Bun | |
| uses: oven-sh/setup-bun@v2 | |
| with: | |
| bun-version: "1.3.6" | |
| - name: Install dependencies (PR) | |
| run: bun install --frozen-lockfile | |
| - name: Build application (PR) | |
| run: bun run build |
🤖 Prompt for AI Agents
In @.github/workflows/bundle-size.yml around lines 26 - 35, The "Setup Bun" step
currently uses bun-version: latest which makes CI non-deterministic; update the
setup action configuration in the workflow step named "Setup Bun" to pin a
specific Bun version (e.g., "1.3.6") or switch to using bun-version-file:
".bun-version" so CI matches local tooling; edit the step that uses
oven-sh/setup-bun@v2 (the "Setup Bun" step) to replace bun-version: latest with
a fixed version string or add bun-version-file to ensure repeatable builds.
| ### Prerequisites | ||
| - Node.js 18+ | ||
| - npm or yarn | ||
| - Bun 1.0+ (recommended) or Node.js 18+ | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check package.json scripts and README prerequisites section
echo "=== package.json scripts ==="
cat package.json | python3 -c "import sys, json; data = json.load(sys.stdin); [print(f' {k}: {v}') for k, v in data.get('scripts', {}).items()]"
echo -e "\n=== README.md lines around 81-83 ==="
sed -n '75,95p' README.mdRepository: osama1998H/lume
Length of output: 2233
Clarify that Bun is required, not optional.
The package.json contains only Bun-based scripts—no npm equivalents exist. Listing "Node.js 18+" as an alternative is misleading since all dev, build, and test commands rely exclusively on bun and bunx. Either remove the Node.js fallback or provide documented npm/Node.js equivalents.
Suggested wording update
- Bun 1.0+ (recommended) or Node.js 18+
+ Bun 1.0+ (required)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ### Prerequisites | |
| - Node.js 18+ | |
| - npm or yarn | |
| - Bun 1.0+ (recommended) or Node.js 18+ | |
| ### Prerequisites | |
| - Bun 1.0+ (required) | |
🤖 Prompt for AI Agents
In `@README.md` around lines 81 - 83, README currently lists "Bun 1.0+
(recommended) or Node.js 18+" but the project scripts in package.json use bun
and bunx exclusively; update the Prerequisites to state that Bun is required
(remove the Node.js fallback) or alternatively add documented npm/Node.js
equivalents for all scripts that call "bun" and "bunx" in package.json (e.g.,
replace bun run/test/build usages with npm/yarn equivalents and document any CLI
differences). Ensure you reference the package.json scripts that invoke "bun"
and "bunx" when making the README change.
| @@ -1,3 +1,4 @@ | |||
| import { describe, it, expect, beforeEach, afterEach, beforeAll, afterAll, mock, spyOn } from 'bun:test'; | |||
There was a problem hiding this comment.
Remove unused imports flagged by static analysis.
🧹 Suggested fix
-import { describe, it, expect, beforeEach, afterEach, beforeAll, afterAll, mock, spyOn } from 'bun:test';
+import { describe, it, expect, beforeEach, beforeAll, afterAll, mock } from 'bun:test';📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| import { describe, it, expect, beforeEach, afterEach, beforeAll, afterAll, mock, spyOn } from 'bun:test'; | |
| import { describe, it, expect, beforeEach, beforeAll, afterAll, mock } from 'bun:test'; |
🧰 Tools
🪛 GitHub Check: CodeQL
[notice] 1-1: Unused variable, import, function or class
Unused imports afterEach, spyOn.
🤖 Prompt for AI Agents
In `@src/components/__tests__/ErrorBoundary.i18n.test.tsx` at line 1, The import
line in ErrorBoundary.i18n.test.tsx currently imports many test helpers
(describe, it, expect, beforeEach, afterEach, beforeAll, afterAll, mock, spyOn);
static analysis flagged some as unused—open the test file, check which of these
symbols are actually referenced (e.g., used in tests or setup/teardown) and
remove the unused names from the import list so only the used symbols remain;
update the import statement that currently lists describe, it, expect,
beforeEach, afterEach, beforeAll, afterAll, mock, spyOn to include only the
referenced helpers.
| @@ -1,41 +1,47 @@ | |||
| import { describe, it, expect, beforeEach, afterEach, beforeAll, afterAll, mock, spyOn } from 'bun:test'; | |||
There was a problem hiding this comment.
Remove unused beforeAll/afterAll imports.
Static analysis flagged these as unused.
🧹 Suggested fix
-import { describe, it, expect, beforeEach, afterEach, beforeAll, afterAll, mock, spyOn } from 'bun:test';
+import { describe, it, expect, beforeEach, afterEach, mock, spyOn } from 'bun:test';📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| import { describe, it, expect, beforeEach, afterEach, beforeAll, afterAll, mock, spyOn } from 'bun:test'; | |
| import { describe, it, expect, beforeEach, afterEach, mock, spyOn } from 'bun:test'; |
🧰 Tools
🪛 GitHub Check: CodeQL
[notice] 1-1: Unused variable, import, function or class
Unused imports afterAll, beforeAll.
🤖 Prompt for AI Agents
In `@src/components/__tests__/FocusMode.test.tsx` at line 1, The test file imports
unused symbols beforeAll and afterAll from 'bun:test'; remove beforeAll and
afterAll from the import list (leaving describe, it, expect, beforeEach,
afterEach, mock, spyOn) so the import statement no longer includes unused
identifiers and static analysis warnings go away; update the import in the
FocusMode.test.tsx file where the import line contains beforeAll and afterAll.
| mockElectronAPI.pomodoro.sessions.getStats = mock(() => Promise.resolve({ | ||
| totalSessions: 999999999, | ||
| completedSessions: 888888888, | ||
| totalFocusTime: 999999999, // in minutes | ||
| totalBreakTime: 888888888, // in minutes | ||
| completionRate: 99.9999, | ||
| currentStreak: 123456789, | ||
| }); | ||
| })); |
There was a problem hiding this comment.
Keep large-stats units consistent.
Earlier tests imply totalFocusTime is in seconds (6000 → 100m). Here the large values are treated as minutes, so expectations can diverge. Use values divisible by 60 to keep the same unit.
✅ Suggested fix
- totalFocusTime: 999999999, // in minutes
- totalBreakTime: 888888888, // in minutes
+ totalFocusTime: 999999999 * 60, // in seconds
+ totalBreakTime: 888888888 * 60, // in seconds📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| mockElectronAPI.pomodoro.sessions.getStats = mock(() => Promise.resolve({ | |
| totalSessions: 999999999, | |
| completedSessions: 888888888, | |
| totalFocusTime: 999999999, // in minutes | |
| totalBreakTime: 888888888, // in minutes | |
| completionRate: 99.9999, | |
| currentStreak: 123456789, | |
| }); | |
| })); | |
| mockElectronAPI.pomodoro.sessions.getStats = mock(() => Promise.resolve({ | |
| totalSessions: 999999999, | |
| completedSessions: 888888888, | |
| totalFocusTime: 999999999 * 60, // in seconds | |
| totalBreakTime: 888888888 * 60, // in seconds | |
| completionRate: 99.9999, | |
| currentStreak: 123456789, | |
| })); |
🤖 Prompt for AI Agents
In `@src/components/__tests__/FocusMode.test.tsx` around lines 466 - 473, The test
uses mockElectronAPI.pomodoro.sessions.getStats but sets totalFocusTime and
totalBreakTime as minutes while earlier tests use seconds; update the mock in
the getStats call to supply totalFocusTime and totalBreakTime in seconds (values
divisible by 60 to preserve minute conversions) and update the inline comment to
reflect seconds, so assertions that expect minute-derived values remain correct.
| (DatabaseManager as any) = mock(() => ({ | ||
| initialize: mock(() => {}), | ||
| })); | ||
|
|
||
| // Mock NotificationService | ||
| (NotificationService as jest.MockedClass<typeof NotificationService>).mockImplementation(() => ({ | ||
| updateSettings: jest.fn(), | ||
| } as any)); | ||
| (NotificationService as any) = mock(() => ({ | ||
| updateSettings: mock(() => {}), | ||
| })); | ||
|
|
||
| // Mock GoalsService | ||
| (GoalsService as jest.MockedClass<typeof GoalsService>).mockImplementation(() => ({ | ||
| setNotificationService: jest.fn(), | ||
| } as any)); | ||
| (GoalsService as any) = mock(() => ({ | ||
| setNotificationService: mock(() => {}), | ||
| })); | ||
|
|
||
| // Mock CategoriesService with async method | ||
| (CategoriesService as jest.MockedClass<typeof CategoriesService>).mockImplementation(() => ({ | ||
| initializeDefaultCategories: jest.fn().mockResolvedValue(undefined), | ||
| } as any)); | ||
| (CategoriesService as any) = mock(() => ({ | ||
| initializeDefaultCategories: mock(() => Promise.resolve()), | ||
| })); | ||
|
|
||
| // Mock ActivityValidationService | ||
| (ActivityValidationService as jest.MockedClass<typeof ActivityValidationService>).mockImplementation(() => ({ | ||
| } as any)); | ||
| (ActivityValidationService as any) = mock(() => ({})); | ||
|
|
||
| // Mock ActivityMergeService | ||
| (ActivityMergeService as jest.MockedClass<typeof ActivityMergeService>).mockImplementation(() => ({ | ||
| } as any)); | ||
| (ActivityMergeService as any) = mock(() => ({})); | ||
|
|
||
| // Mock ActivityTrackingService | ||
| (ActivityTrackingService as jest.MockedClass<typeof ActivityTrackingService>).mockImplementation(() => ({ | ||
| updateSettings: jest.fn(), | ||
| isTracking: jest.fn().mockReturnValue(false), | ||
| stop: jest.fn().mockResolvedValue(undefined), | ||
| } as any)); | ||
| (ActivityTrackingService as any) = mock(() => ({ | ||
| updateSettings: mock(() => {}), | ||
| isTracking: mock(() => false), | ||
| stop: mock(() => Promise.resolve()), | ||
| })); | ||
|
|
||
| // Mock PomodoroService | ||
| (PomodoroService as jest.MockedClass<typeof PomodoroService>).mockImplementation(() => ({ | ||
| destroy: jest.fn(), | ||
| } as any)); | ||
| (PomodoroService as any) = mock(() => ({ | ||
| destroy: mock(() => {}), | ||
| })); |
There was a problem hiding this comment.
Reassigning imports is invalid in ES modules and will fail.
ES module imports are read-only bindings. The pattern (DatabaseManager as any) = mock(...) doesn't actually replace the import—it's a no-op or will throw at runtime depending on strict mode. Biome correctly flagged these as errors.
Since you already have mock.module() calls at the top of the file (lines 5-34), those module mocks are already in effect. The beforeEach reassignments are either dead code or will cause runtime errors.
Proposed fix: Remove redundant import reassignments
If you need per-test mock customization, use mock.module() with a factory that returns configurable mocks, or restructure tests to use the mocks defined at the module level. For now, remove the invalid reassignments:
beforeEach(() => {
container = new ServiceContainer();
consoleLog = spyOn(console, 'log').mockImplementation(() => {});
consoleError = spyOn(console, 'error').mockImplementation(() => {});
consoleWarn = spyOn(console, 'warn').mockImplementation(() => {});
-
- // Mock DatabaseManager
- (DatabaseManager as any) = mock(() => ({
- initialize: mock(() => {}),
- }));
-
- // Mock NotificationService
- (NotificationService as any) = mock(() => ({
- updateSettings: mock(() => {}),
- }));
-
- // Mock GoalsService
- (GoalsService as any) = mock(() => ({
- setNotificationService: mock(() => {}),
- }));
-
- // Mock CategoriesService with async method
- (CategoriesService as any) = mock(() => ({
- initializeDefaultCategories: mock(() => Promise.resolve()),
- }));
-
- // Mock ActivityValidationService
- (ActivityValidationService as any) = mock(() => ({}));
-
- // Mock ActivityMergeService
- (ActivityMergeService as any) = mock(() => ({}));
-
- // Mock ActivityTrackingService
- (ActivityTrackingService as any) = mock(() => ({
- updateSettings: mock(() => {}),
- isTracking: mock(() => false),
- stop: mock(() => Promise.resolve()),
- }));
-
- // Mock PomodoroService
- (PomodoroService as any) = mock(() => ({
- destroy: mock(() => {}),
- }));
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| (DatabaseManager as any) = mock(() => ({ | |
| initialize: mock(() => {}), | |
| })); | |
| // Mock NotificationService | |
| (NotificationService as jest.MockedClass<typeof NotificationService>).mockImplementation(() => ({ | |
| updateSettings: jest.fn(), | |
| } as any)); | |
| (NotificationService as any) = mock(() => ({ | |
| updateSettings: mock(() => {}), | |
| })); | |
| // Mock GoalsService | |
| (GoalsService as jest.MockedClass<typeof GoalsService>).mockImplementation(() => ({ | |
| setNotificationService: jest.fn(), | |
| } as any)); | |
| (GoalsService as any) = mock(() => ({ | |
| setNotificationService: mock(() => {}), | |
| })); | |
| // Mock CategoriesService with async method | |
| (CategoriesService as jest.MockedClass<typeof CategoriesService>).mockImplementation(() => ({ | |
| initializeDefaultCategories: jest.fn().mockResolvedValue(undefined), | |
| } as any)); | |
| (CategoriesService as any) = mock(() => ({ | |
| initializeDefaultCategories: mock(() => Promise.resolve()), | |
| })); | |
| // Mock ActivityValidationService | |
| (ActivityValidationService as jest.MockedClass<typeof ActivityValidationService>).mockImplementation(() => ({ | |
| } as any)); | |
| (ActivityValidationService as any) = mock(() => ({})); | |
| // Mock ActivityMergeService | |
| (ActivityMergeService as jest.MockedClass<typeof ActivityMergeService>).mockImplementation(() => ({ | |
| } as any)); | |
| (ActivityMergeService as any) = mock(() => ({})); | |
| // Mock ActivityTrackingService | |
| (ActivityTrackingService as jest.MockedClass<typeof ActivityTrackingService>).mockImplementation(() => ({ | |
| updateSettings: jest.fn(), | |
| isTracking: jest.fn().mockReturnValue(false), | |
| stop: jest.fn().mockResolvedValue(undefined), | |
| } as any)); | |
| (ActivityTrackingService as any) = mock(() => ({ | |
| updateSettings: mock(() => {}), | |
| isTracking: mock(() => false), | |
| stop: mock(() => Promise.resolve()), | |
| })); | |
| // Mock PomodoroService | |
| (PomodoroService as jest.MockedClass<typeof PomodoroService>).mockImplementation(() => ({ | |
| destroy: jest.fn(), | |
| } as any)); | |
| (PomodoroService as any) = mock(() => ({ | |
| destroy: mock(() => {}), | |
| })); | |
| beforeEach(() => { | |
| container = new ServiceContainer(); | |
| consoleLog = spyOn(console, 'log').mockImplementation(() => {}); | |
| consoleError = spyOn(console, 'error').mockImplementation(() => {}); | |
| consoleWarn = spyOn(console, 'warn').mockImplementation(() => {}); | |
| }); |
🧰 Tools
🪛 Biome (2.1.2)
[error] 86-86: The imported variable DatabaseManager is read-only
The variable is imported here
Use a local variable instead of reassigning an import.
(lint/suspicious/noImportAssign)
[error] 112-112: The imported variable ActivityTrackingService is read-only
The variable is imported here
Use a local variable instead of reassigning an import.
(lint/suspicious/noImportAssign)
[error] 119-119: The imported variable PomodoroService is read-only
The variable is imported here
Use a local variable instead of reassigning an import.
(lint/suspicious/noImportAssign)
[error] 91-91: The imported variable NotificationService is read-only
The variable is imported here
Use a local variable instead of reassigning an import.
(lint/suspicious/noImportAssign)
[error] 96-96: The imported variable GoalsService is read-only
The variable is imported here
Use a local variable instead of reassigning an import.
(lint/suspicious/noImportAssign)
[error] 101-101: The imported variable CategoriesService is read-only
The variable is imported here
Use a local variable instead of reassigning an import.
(lint/suspicious/noImportAssign)
[error] 106-106: The imported variable ActivityValidationService is read-only
The variable is imported here
Use a local variable instead of reassigning an import.
(lint/suspicious/noImportAssign)
[error] 109-109: The imported variable ActivityMergeService is read-only
The variable is imported here
Use a local variable instead of reassigning an import.
(lint/suspicious/noImportAssign)
🤖 Prompt for AI Agents
In `@src/main/services/__tests__/ServiceContainer.test.ts` around lines 86 - 121,
The test file incorrectly attempts to reassign ES module imports (e.g.,
DatabaseManager, NotificationService, GoalsService, CategoriesService,
ActivityValidationService, ActivityMergeService, ActivityTrackingService,
PomodoroService) inside beforeEach using (X as any) = mock(...), which is
invalid; remove these reassignment blocks and rely on the existing mock.module()
calls at the top of the file for module-level mocks, or if per-test
customization is needed convert those module mocks to factories via
mock.module() so you can configure returned mock implementations in each test
instead of reassigning imports.
| const mockExec = mock(() => {}); | ||
| mock.module('child_process', () => ({ | ||
| exec: mockExec, | ||
| })); |
There was a problem hiding this comment.
Mocking pattern will fail: reassigning const is not allowed.
The mockExec is declared as const on line 5, but is reassigned throughout the test file (lines 165, 172, 210, 248, 272, 299, 351, 390). This will cause runtime errors. As flagged by Biome, change const to let.
Additionally, the current mocking approach doesn't properly update the module mock. Consider restructuring to use mock.module with a mutable reference or use Bun's mock restoration patterns.
Proposed fix
// Mock child_process with proper callback-style exec
-const mockExec = mock(() => {});
+let mockExec = mock(() => {});
mock.module('child_process', () => ({
exec: mockExec,
}));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const mockExec = mock(() => {}); | |
| mock.module('child_process', () => ({ | |
| exec: mockExec, | |
| })); | |
| let mockExec = mock(() => {}); | |
| mock.module('child_process', () => ({ | |
| exec: mockExec, | |
| })); |
🤖 Prompt for AI Agents
In `@src/services/activity/__tests__/ActivityMonitor.test.ts` around lines 5 - 8,
The issue is that mockExec is declared as const but reassigned later, causing
runtime errors; change the declaration of mockExec from const to let and update
the module mock usage so the child_process mock always points to the current
mockExec instance (e.g., re-register mock.module('child_process', ...) or use a
mutable wrapper/holder variable passed into mock.module) and ensure test
setup/teardown updates or restores the mock before each test so reassignments to
mockExec take effect everywhere that uses the child_process exec mock.
|
|
||
| import { ActivityTrackingService } from '../ActivityTrackingService'; | ||
| import { ActivityMonitor } from '../ActivityMonitor'; | ||
| import { DatabaseManager } from '../../../database/DatabaseManager'; |
There was a problem hiding this comment.
Remove unused DatabaseManager import.
CodeQL flags this as unused, and it’s not referenced anywhere in the file.
🧹 Suggested cleanup
-import { DatabaseManager } from '../../../database/DatabaseManager';📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| import { DatabaseManager } from '../../../database/DatabaseManager'; |
🧰 Tools
🪛 GitHub Check: CodeQL
[notice] 19-19: Unused variable, import, function or class
Unused import DatabaseManager.
🤖 Prompt for AI Agents
In `@src/services/activity/__tests__/ActivityTrackingService.test.ts` at line 19,
Remove the unused import DatabaseManager from the test file: delete the line
importing DatabaseManager since it is not referenced anywhere in
ActivityTrackingService.test (remove the symbol "DatabaseManager" from the
import list or the entire import statement if nothing else is imported), then
run tests/lint to confirm no references remain.
| }; | ||
|
|
||
| (ActivityMonitor as jest.Mock).mockImplementation(() => mockMonitor); | ||
| (ActivityMonitor as any) = mock(() => mockMonitor); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n src/services/activity/__tests__/ActivityTrackingService.test.ts | head -60Repository: osama1998H/lume
Length of output: 2379
Remove the redundant reassignment of ActivityMonitor. The mocking is already properly configured via mock.module at lines 8-10, so the reassignment at line 48 is unnecessary and attempts to modify a read-only ESM import binding. Delete this line.
✅ Safer adjustment
- (ActivityMonitor as any) = mock(() => mockMonitor);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| (ActivityMonitor as any) = mock(() => mockMonitor); |
🧰 Tools
🪛 Biome (2.1.2)
[error] 48-48: The imported variable ActivityMonitor is read-only
The variable is imported here
Use a local variable instead of reassigning an import.
(lint/suspicious/noImportAssign)
🤖 Prompt for AI Agents
In `@src/services/activity/__tests__/ActivityTrackingService.test.ts` at line 48,
Remove the redundant reassignment of the ESM import binding by deleting the line
"(ActivityMonitor as any) = mock(() => mockMonitor);" — the module mock is
already configured via mock.module (lines where mock.module is used) so avoid
attempting to reassign the imported ActivityMonitor; locate references to
ActivityMonitor in ActivityTrackingService.test (the test setup using
mock.module and mockMonitor) and ensure only the mock.module configuration is
used to inject mockMonitor.
| let mockNotificationInstances: any[] = []; | ||
| const mockShow = mock(() => {}); | ||
|
|
||
| // Mock Electron's Notification | ||
| jest.mock('electron', () => ({ | ||
| Notification: jest.fn().mockImplementation(function(this: any, options: any) { | ||
| mock.module('electron', () => ({ | ||
| Notification: function(this: any, options: any) { | ||
| this.title = options.title; | ||
| this.body = options.body; | ||
| this.silent = options.silent; | ||
| this.show = jest.fn(); | ||
| this.show = mockShow; | ||
| mockNotificationInstances.push(this); | ||
| return this; | ||
| }), | ||
| }, | ||
| })); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Read the full test file to understand the assertions being made
wc -l src/services/notifications/__tests__/NotificationService.test.tsRepository: osama1998H/lume
Length of output: 127
🏁 Script executed:
#!/bin/bash
# Read the full test file to see what assertions are used with Notification
cat -n src/services/notifications/__tests__/NotificationService.test.tsRepository: osama1998H/lume
Length of output: 10165
🏁 Script executed:
#!/bin/bash
# Search for how mock.module is used elsewhere in the codebase
fd "\.test\.[jt]s$" -x grep -l "mock\.module" {}Repository: osama1998H/lume
Length of output: 353
🏁 Script executed:
#!/bin/bash
# Check another test file using mock.module to see the pattern
grep -A 20 "mock\.module" src/services/activity/__tests__/ActivityMonitor.test.ts | head -40Repository: osama1998H/lume
Length of output: 616
🏁 Script executed:
#!/bin/bash
# Search for toHaveBeenCalledWith usage in other mock.module test files
grep -n "toHaveBeenCalledWith" src/database/__tests__/DatabaseManager.test.ts | head -10Repository: osama1998H/lume
Length of output: 562
🏁 Script executed:
#!/bin/bash
# Check how mockExec is defined in ActivityMonitor.test.ts
grep -B 5 "mock\.module.*child_process" src/services/activity/__tests__/ActivityMonitor.test.ts | head -20Repository: osama1998H/lume
Length of output: 307
🏁 Script executed:
#!/bin/bash
# Check how Database is mocked in DatabaseManager.test.ts
grep -B 10 "mock\.module.*better-sqlite3" src/database/__tests__/DatabaseManager.test.ts | head -30Repository: osama1998H/lume
Length of output: 370
🏁 Script executed:
#!/bin/bash
# Check the NotificationService implementation to understand what it does
cat src/services/notifications/NotificationService.tsRepository: osama1998H/lume
Length of output: 2521
🏁 Script executed:
#!/bin/bash
# Check if there's a test runner configuration or results
find . -name "*.json" -o -name "*.config.*" | grep -E "(bun|test|jest)" | head -10Repository: osama1998H/lume
Length of output: 1008
🏁 Script executed:
#!/bin/bash
# Check how the mockDatabase is created and used for comparison
grep -A 15 "const mockDb" src/database/__tests__/DatabaseManager.test.ts | head -20Repository: osama1998H/lume
Length of output: 41
🏁 Script executed:
#!/bin/bash
# Verify if other tests reset their mocks in beforeEach
grep -A 5 "beforeEach" src/database/__tests__/DatabaseManager.test.ts | head -20Repository: osama1998H/lume
Length of output: 952
🏁 Script executed:
#!/bin/bash
# Check the exact pattern: in other tests, are mocks created with mock() first?
# Let's verify DatabaseManager more carefully
grep -B 20 "mock\.module" src/database/__tests__/DatabaseManager.test.ts | grep -E "(const mock|mock\()"Repository: osama1998H/lume
Length of output: 620
🏁 Script executed:
#!/bin/bash
# Let's search for the Database mock definition
grep -A 10 "const mockDb\|const Database" src/database/__tests__/DatabaseManager.test.ts | head -30Repository: osama1998H/lume
Length of output: 41
🏁 Script executed:
#!/bin/bash
# Check if Notification is being wrapped with mock() somewhere we missed
grep -n "mock(" src/services/notifications/__tests__/NotificationService.test.tsRepository: osama1998H/lume
Length of output: 93
🏁 Script executed:
#!/bin/bash
# Double-check: is Notification imported as a named import and then reassigned?
grep -n "import.*Notification\|from.*electron" src/services/notifications/__tests__/NotificationService.test.tsRepository: osama1998H/lume
Length of output: 167
🏁 Script executed:
#!/bin/bash
# Check if beforeEach has mockClear() or mockReset() calls
grep -A 10 "beforeEach" src/services/notifications/__tests__/NotificationService.test.tsRepository: osama1998H/lume
Length of output: 718
🏁 Script executed:
#!/bin/bash
# Search for any test results or CI configuration
find . -name "*.yml" -o -name "*.yaml" | xargs grep -l "bun.*test" 2>/dev/null | head -5Repository: osama1998H/lume
Length of output: 239
🏁 Script executed:
#!/bin/bash
# Check package.json for test script
cat package.json | grep -A 5 '"test"'Repository: osama1998H/lume
Length of output: 308
🌐 Web query:
Bun mock.module spy function toHaveBeenCalledWith documentation
💡 Result:
In Bun’s test runner (bun:test), .toHaveBeenCalledWith(...expectedArgs) is an expect() matcher you can use on mock functions (created via mock() / jest.fn() / vi.fn()) and on spies (created via spyOn()). It asserts the mock/spy was called at least once with the given arguments. [1] [2]
Example with a spy: [1] [3]
import { test, expect, spyOn } from "bun:test";
const obj = { greet(name: string) {} };
const spy = spyOn(obj, "greet");
test("called with arg", () => {
obj.greet("alice");
expect(spy).toHaveBeenCalledWith("alice");
});Example with mock.module() returning a mocked function you can assert on: [1]
import { test, expect, mock } from "bun:test";
mock.module("./api", () => ({
fetchUser: mock(async (id: string) => ({ id })),
}));
test("module mock called with arg", async () => {
const { fetchUser } = await import("./api");
await fetchUser("123");
expect(fetchUser).toHaveBeenCalledWith("123");
});Sources: Bun “Mocks” docs (incl. mock.module, mock, spyOn) [1], and the API reference for toHaveBeenCalledWith [2].
Wrap Notification with mock() to enable spy assertions.
The Notification constructor in mock.module is a plain function, not a spy. All assertions using expect(Notification).toHaveBeenCalledWith() and expect(Notification).not.toHaveBeenCalled() will fail. Additionally, mockShow is not reset between tests, causing call count accumulation, and the error handling test (lines 230–231) attempts to reassign an imported binding, which violates ES module semantics.
Suggested fix
// Track mock instances for testing
let mockNotificationInstances: any[] = [];
const mockShow = mock(() => {});
+const MockNotification = mock(function(this: any, options: any) {
+ this.title = options.title;
+ this.body = options.body;
+ this.silent = options.silent;
+ this.show = mockShow;
+ mockNotificationInstances.push(this);
+ return this;
+});
// Mock Electron's Notification
mock.module('electron', () => ({
- Notification: function(this: any, options: any) {
- this.title = options.title;
- this.body = options.body;
- this.silent = options.silent;
- this.show = mockShow;
- mockNotificationInstances.push(this);
- return this;
- },
+ Notification: MockNotification,
}));
import { NotificationService } from '../NotificationService';
import { Notification } from 'electron';
describe('NotificationService', () => {
let service: NotificationService;
let consoleLog: ReturnType<typeof spyOn>;
beforeEach(() => {
mockNotificationInstances = [];
+ mockShow.mockClear();
+ MockNotification.mockClear();
consoleLog = spyOn(console, 'log').mockImplementation(() => {});
});For the error handling test (lines 226–244), avoid reassigning the imported Notification. Instead, make the mock throw:
describe('Error handling', () => {
it('should handle notification errors gracefully', () => {
const consoleError = spyOn(console, 'error').mockImplementation(() => {});
- // Override the mock to throw an error
- const originalNotification = Notification;
- (Notification as any) = function() {
- throw new Error('Notification failed');
- };
+ MockNotification.mockImplementation(() => {
+ throw new Error('Notification failed');
+ });
service = new NotificationService(true, true);
service.notifyFocusComplete('Test Task');
expect(consoleError).toHaveBeenCalledWith(
expect.stringContaining('Failed to show notification'),
expect.any(Error)
);
consoleError.mockRestore();
+ MockNotification.mockClear();
});
});🤖 Prompt for AI Agents
In `@src/services/notifications/__tests__/NotificationService.test.ts` around
lines 4 - 17, The test's mock for Electron's Notification must be a spy and
isolated per test: wrap the constructor with mock() when registering it in
mock.module so assertions like expect(Notification).toHaveBeenCalledWith(...)
work, ensure mockShow is reset between tests (call mockReset/mockClear in
beforeEach/afterEach) to avoid accumulated call counts, and for the
error-handling test do not reassign the imported Notification binding—instead
change the mock implementation to throw (e.g., set the mock module's
Notification to a throwing mock for that test) so the test simulates constructor
failure without violating ES module semantics; update references in the test
file (mockShow, Notification) accordingly.
1 participant
Summary
bun buildfor Electron main/preload bundling instead of tsccontextBridgeimport error@sentry/electronas external to prevent preload code bundling into main processhappy-domfor DOM testing environmentTest plan
bun run devand verify Electron app startsbun testto verify tests pass🤖 Generated with Claude Code
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.