feat: introduce injectable windowService [WPB-21950]#20443
feat: introduce injectable windowService [WPB-21950]#20443
Conversation
e-maad
requested review from
V-Gira,
arjita-mitra,
mathias-niboulies,
nkaramarko,
otto-the-bot,
screendriver,
thisisamir98 and
zskhan
as code owners
|
There was a problem hiding this comment.
Pull request overview
This PR introduces a WindowService abstraction intended to make browser-window interactions injectable/mocked in tests, and refactors EventRepository (plus its unit tests) to use that service for event listener wiring.
Changes:
- Added a singleton
WindowServicewith helper methods for registering/unregistering common window event listeners. - Refactored
EventRepository.connectWebSocket()to useWindowServicefor focus/online/offline/visibility listeners. - Updated
EventRepositorytests to mockWindowServiceviatsyringeand adjusted Electron detection mocking.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| apps/webapp/src/script/window/windowService.ts | Introduces injectable service for window-related event listeners. |
| apps/webapp/src/script/repositories/event/EventRepository.ts | Switches event listener setup to use WindowService. |
| apps/webapp/src/script/repositories/event/EventRepository.test.ts | Mocks WindowService and updates expectations around event listener setup. |
| const mockService = { | ||
| onOnline: jest.fn().mockReturnValue(() => {}), | ||
| onOffline: jest.fn().mockReturnValue(() => {}), | ||
| onVisibilityChange: jest.fn().mockReturnValue(() => {}), | ||
| onFocus: jest.fn().mockReturnValue(() => {}), | ||
| } as unknown as WindowService; |
There was a problem hiding this comment.
[Suggestion] - mockService is created with jest.fn() at module scope, so its call history persists across tests; jest.restoreAllMocks() does not clear plain jest.fn mocks. This can make assertions less reliable over time.
Suggested fix: add jest.clearAllMocks() in an appropriate beforeEach, or explicitly mockClear() the mockService methods between tests.
| window.addEventListener('visibilitychange', callback); | ||
| return () => window.removeEventListener('visibilitychange', callback); |
There was a problem hiding this comment.
[Blocker] - visibilitychange is a document event, but WindowService.onVisibilityChange attaches the listener to window, so the callback may never fire in browsers.
Suggested fix: register/unregister visibilitychange on document (or inject a Document/WindowLike dependency so the correct target is used).
| window.addEventListener('visibilitychange', callback); | |
| return () => window.removeEventListener('visibilitychange', callback); | |
| document.addEventListener('visibilitychange', callback); | |
| return () => document.removeEventListener('visibilitychange', callback); |
| @@ -277,20 +278,12 @@ export class EventRepository { | |||
| } | |||
| }; | |||
|
|
|||
| document.addEventListener('visibilitychange', handleVisibilityChange); | |||
| cleanupHandlers.push(() => { | |||
| document.removeEventListener('visibilitychange', handleVisibilityChange); | |||
| }); | |||
| cleanupHandlers.push(this.windowService.onVisibilityChange(handleVisibilityChange)); | |||
| } | |||
There was a problem hiding this comment.
[Blocker] - The browser branch uses document.visibilityState but subscribes via windowService.onVisibilityChange(...). Since visibilitychange is dispatched on document, the handler can be missed if the service is wiring it to window.
Suggested fix: ensure the subscription is done on document (either in WindowService.onVisibilityChange or by introducing a dedicated DocumentService).
| const originalIsElectron = Runtime.isElectron; | ||
| jest.spyOn(Runtime, 'isElectron').mockReturnValue(false); | ||
|
|
||
| await eventRepository.connectWebSocket(mockAccount, false, jest.fn()); | ||
|
|
||
| expect(document.addEventListener).toHaveBeenCalledWith('visibilitychange', expect.any(Function)); | ||
| expect(mockService.onVisibilityChange).toHaveBeenCalledWith(expect.any(Function)); | ||
|
|
||
| (window as any).isElectron = originalIsElectron; | ||
| jest.spyOn(Runtime, 'isElectron').mockReturnValue(originalIsElectron()); | ||
| }); |
There was a problem hiding this comment.
[Blocker] - This test calls jest.spyOn(Runtime, 'isElectron') twice. Spying on the same method again will typically throw and/or leave the spy in an unexpected state.
Suggested fix: keep a reference to the spy and call mockRestore() (or rely on the existing afterEach(jest.restoreAllMocks) and remove the manual restore line entirely).
| testFactory.exposeClientActors(); | ||
| container.registerInstance(WindowService, mockService); | ||
| }); | ||
|
|
There was a problem hiding this comment.
[Important] - container.registerInstance(WindowService, mockService) mutates the global tsyringe container but is never undone. This can leak into other test files (module cache is shared) and cause order-dependent failures.
Suggested fix: unregister/reset the container in afterAll (or use a child container / container.clearInstances() pattern) so this registration is scoped to this suite.
| afterAll(() => { | |
| container.unregister(WindowService); | |
| }); |
Codecov Report❌ Patch coverage is
❌ Your patch check has failed because the patch coverage (26.66%) is below the target coverage (50.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## dev #20443 +/- ##
==========================================
- Coverage 45.34% 45.33% -0.01%
==========================================
Files 1635 1636 +1
Lines 40312 40317 +5
Branches 8335 8336 +1
==========================================
Hits 18278 18278
- Misses 20101 20107 +6
+ Partials 1933 1932 -1
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
2 participants
Pull Request
Summary
This PR introduces an injectable windowService abstraction, making the browser window object testable and allowing for dependency injection. This refactoring improves testability by enabling the window to be mocked in unit tests.
Security Checklist (required)
Accessibility (required)
Standards Acknowledgement (required)
Notes for reviewers