fix & test: usePostMessage #427
Conversation
fix: incorrect origin handling test: add tests
There was a problem hiding this comment.
Pull request overview
This PR fixes a critical bug in the usePostMessage hook's origin filtering logic and adds comprehensive test coverage. The bug caused all messages to be rejected when the origin was specified as an array of strings without "*", due to an incorrect logical OR operator (||) that should have been an AND operator (&&). The PR also refactors origin handling to use a ref instead of closure capture to ensure the latest origin value is always used.
Key Changes
- Fixed the origin filtering logic from
(!origin.includes(event.origin) || !origin.includes('*'))to(!origin.includes(event.origin) && !origin.includes('*')), correctly implementing "reject if the origin is neither in the allowed list nor is wildcard" - Introduced
internalOriginRefto track the origin value, ensuring the postMessage function and event listener always use the current origin value - Added comprehensive test suite covering client/server rendering, origin filtering (single string and array), wildcard support, and proper cleanup
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| packages/core/src/hooks/usePostMessage/usePostMessage.ts | Fixed origin filtering bug and added internalOriginRef for proper origin tracking; refactored postMessage to use ref |
| packages/core/src/hooks/usePostMessage/usePostMessage.test.ts | Added comprehensive test suite with 8 test cases covering all hook functionality |
| packages/core/src/bundle/hooks/usePostMessage/usePostMessage.js | Applied same fixes as TypeScript version to JavaScript bundle |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const postMessage = (message: Message) => { | ||
| if (Array.isArray(origin)) { | ||
| origin.forEach((origin) => window.postMessage(message, origin)); | ||
| return; | ||
| } | ||
| if (Array.isArray(internalOriginRef.current)) | ||
| return internalOriginRef.current.forEach((origin) => window.postMessage(message, origin)); | ||
|
|
||
| window.postMessage(message, origin); | ||
| window.postMessage(message, internalOriginRef.current); | ||
| }; |
There was a problem hiding this comment.
The postMessage function accesses window.postMessage without checking if window is defined. This could cause a runtime error if the function is called during server-side rendering or before the component mounts. Consider adding a guard check like if (typeof window === 'undefined') return; at the beginning of the function, similar to how other hooks in this codebase handle window access.
| const postMessage = (message) => { | ||
| if (Array.isArray(origin)) { | ||
| origin.forEach((origin) => window.postMessage(message, origin)); | ||
| return; | ||
| } | ||
| window.postMessage(message, origin); | ||
| if (Array.isArray(internalOriginRef.current)) | ||
| return internalOriginRef.current.forEach((origin) => window.postMessage(message, origin)); | ||
| window.postMessage(message, internalOriginRef.current); | ||
| }; |
There was a problem hiding this comment.
The postMessage function accesses window.postMessage without checking if window is defined. This could cause a runtime error if the function is called during server-side rendering or before the component mounts. Consider adding a guard check like if (typeof window === 'undefined') return; at the beginning of the function, similar to how other hooks in this codebase handle window access.
| result.current({ data: 'test-message', origin: 'origin1' }); | ||
| }); | ||
|
|
||
| expect(mockPostMessage).toHaveBeenCalledOnce(); | ||
| expect(mockPostMessage).toHaveBeenCalledWith({ data: 'test-message', origin: 'origin1' }, '*'); |
There was a problem hiding this comment.
The test is calling postMessage with an object containing 'data' and 'origin' properties, but the hook's postMessage function expects just the message as the first parameter. The second parameter (origin) is already captured from the hook's configuration. This test should call result.current('test-message') instead of result.current({ data: 'test-message', origin: 'origin1' }). The expected window.postMessage call should be expect(mockPostMessage).toHaveBeenCalledWith('test-message', '*').
| result.current({ data: 'test-message', origin: 'origin1' }); | |
| }); | |
| expect(mockPostMessage).toHaveBeenCalledOnce(); | |
| expect(mockPostMessage).toHaveBeenCalledWith({ data: 'test-message', origin: 'origin1' }, '*'); | |
| result.current('test-message'); | |
| }); | |
| expect(mockPostMessage).toHaveBeenCalledOnce(); | |
| expect(mockPostMessage).toHaveBeenCalledWith('test-message', '*'); |
1 participant
Тесты: рендер на клиенте/сервере, корректный вызов postMessage, корректная фильтрация сообщений (для строки и массива строк), правильное размонтирование компонента.
Исправлено: некорректная обработка
originЕсли
originпередан как массив строк без"*", например:то из-за условия:
вторая проверка (
!origin.includes('*')) всегда возвращаетtrue, что приводит к отбрасыванию всех сообщений, включая сообщения с допустимымevent.origin.