-
Notifications
You must be signed in to change notification settings - Fork 20
Fix TypeError when receiving messages without action property #64
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Fix TypeError when receiving messages without action property #64
Conversation
Add optional chaining on action property to prevent "Cannot read properties of undefined (reading 'startsWith')" error when window receives messages that have data but no action property.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR fixes a TypeError that occurs when the window receives postMessage events with a data property but without an action property. The fix adds optional chaining to safely access the action property before calling startsWith().
Key Changes:
- Added optional chaining operator to prevent TypeError when
event.data.actionis undefined - Added unit tests to verify messages without the
actionproperty are properly ignored
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/services/WebExtensionService.ts | Added optional chaining to event.data.action to prevent TypeError when action is undefined |
| src/services/tests/WebExtensionService-test.ts | Added three test cases verifying that messages with missing/null/undefined data or action properties are properly ignored |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Verify that console.warn is not called when messages are ignored, rather than just checking that no error is thrown.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Thanks for noticing the issue! Made this mistake when I changed: if (!/^web-eid:/.test(event.data?.action)) return;to if (!event.data?.action.startsWith("web-eid:")) return;Your change does fix the issue, but only if we expect window.postMessage(
{
action: {
id: "<actionId>",
_t: "<actionTimestamp>",
},
payload: {
...
}
},
"*"
);We could add another optional chaining check, so window.postMessage(
{
action: {
startsWith: "2022-10-12",
endsWith: "2026-10-12
}
},
"*"
);Adding type checks would make this IF statement harder to read at a glance, so I think the best option would be to revert it back to: if (!/^web-eid:/.test(event.data?.action)) return;The regex What do you think, @ErkoRisthein ? |
Address code review feedback: - Add typeof check to reject non-string action values early - Use startsWith() safely after confirming action is a string - Add test cases for action as object, number, and array
|
Thanks for the review, @taneltm. You're right that the optional chaining approach doesn't handle all edge cases. I tried reverting to the regex approach you suggested: However, I discovered a non-intuitive edge case: JavaScript's So I've added an explicit type guard before calling This handles all edge cases:
I've added test cases for these scenarios. All tests pass. |
taneltm
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Original issue reproduced in
main - PR fixes the issue
- Confirmed that the tests are working properly (tests fail, as expected, when the fix is reverted)
|
@ErkoRisthein, heartfelt thanks for your contribution! A couple of formalities still remain, then I will gladly merge this: can you please squash the commits into one, sign the Certificate of Origin with |
Summary
Fixes
TypeError: Cannot read properties of undefined (reading 'startsWith')that occurs when the window receives postMessage events that have adataproperty but noactionproperty.Root Cause
The condition
event.data?.action.startsWith("web-eid:")safely handles whenevent.datais undefined, but throws whenevent.dataexists andevent.data.actionis undefined.Fix
Add optional chaining on
action:Test Plan
Signed-off-by: Erko Risthein erko@risthein.ee