Skip to content
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

feat: use BroadcastChannel to synchronize messages across multiple tabs #3599

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

CR4ZED
Copy link
Contributor

@CR4ZED CR4ZED commented Sep 29, 2024

Fixes #1443

Changes

  • Replaced window.opener.postMessage with BroadcastChannel for better tab synchronization.
  • Improved support for scenarios where new tabs are opened with cmd+click or target=_blank, ensuring messages are correctly posted to all tabs.

Events

Did you introduce any new tracking events?
No

Experiment

Did you introduce any new experiments?

No

Manual Testing

Caution

Please make sure existing components are not breaking/affected by this PR

- Replaced window.opener.postMessage with BroadcastChannel for better tab synchronization.
- Improved support for scenarios where new tabs are opened with cmd+click or target=_blank, ensuring messages are correctly posted to all tabs.
Copy link

vercel bot commented Sep 29, 2024

@CR4ZED is attempting to deploy a commit to the Daily Dev Team on Vercel.

A member of the Team first needs to authorize it.

@CLAassistant
Copy link

CLAassistant commented Sep 29, 2024

CLA assistant check
All committers have signed the CLA.

Copy link
Member

@sshanzel sshanzel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We really appreciate the initiative. Especially ones that apply good practices. However, this is a sensitive part of the application, we haven't done our own research on this end yet. Quite not sure also when we'd also do due to our priorities. Unless the team are certain with things would still behave as intended, we can't merge this yet. Amazing PR nonetheless 🙏

@CR4ZED
Copy link
Contributor Author

CR4ZED commented Sep 30, 2024

We really appreciate the initiative. Especially ones that apply good practices. However, this is a sensitive part of the application, we haven't done our own research on this end yet. Quite not sure also when we'd also do due to our priorities. Unless the team are certain with things would still behave as intended, we can't merge this yet. Amazing PR nonetheless 🙏

Thank you for your feedback! I understand this part is sensitive. If you need any help or more information from me, just let me know 🙂

@sshanzel
Copy link
Member

We really appreciate the initiative. Especially ones that apply good practices. However, this is a sensitive part of the application, we haven't done our own research on this end yet. Quite not sure also when we'd also do due to our priorities. Unless the team are certain with things would still behave as intended, we can't merge this yet. Amazing PR nonetheless 🙏

Thank you for your feedback! I understand this part is sensitive. If you need any help or more information from me, just let me know 🙂

I don't want to take so much of your time, but if you feel like it, feel free to drop some resources! ❤️

@CR4ZED
Copy link
Contributor Author

CR4ZED commented Sep 30, 2024

We really appreciate the initiative. Especially ones that apply good practices. However, this is a sensitive part of the application, we haven't done our own research on this end yet. Quite not sure also when we'd also do due to our priorities. Unless the team are certain with things would still behave as intended, we can't merge this yet. Amazing PR nonetheless 🙏

Thank you for your feedback! I understand this part is sensitive. If you need any help or more information from me, just let me know 🙂

I don't want to take so much of your time, but if you feel like it, feel free to drop some resources! ❤️

While debugging the issue, I found that when we call postWindowMessage, it uses window.opener?.postMessage, and I noticed that almost all the links opened had window.opener as null. To keep the tabs in sync, I looked for a solution and discovered the Broadcast Channel API (MDN Documentation). Initially, I planned to use it only for the login event, but as I explored the codebase, I found several other events emitted via window.opener?.postMessage, so I replaced those with the Broadcast Channel API as well.

However, I couldn’t run the app locally to test these changes because it throws this error when I try to start it:

Screenshot 2024-09-30 at 8 21 37 PM

It looks like the client-side is trying to connect to localhost:5000 instead of the Gitpod URL

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

🐛 BUG: Wrong password prompt when already logged in!
3 participants