fix: replace window.closeLangSwitcher global with CustomEvent (#1456)#1457
fix: replace window.closeLangSwitcher global with CustomEvent (#1456)#1457xyaz1313 wants to merge 1 commit intokubestellar:mainfrom
Conversation
Fixes kubestellar#1456 The Navbar component was using (window as any).closeLangSwitcher for cross-component communication, which: - Pollutes the global namespace - Is fragile (name collisions) - Has no cleanup on unmount - Requires eslint-disable comments for any-casts Replaced with a 'close-lang-switcher' CustomEvent: - Dropdown hover: window.dispatchEvent(new CustomEvent('close-lang-switcher')) - Escape key: same dispatch pattern - LanguageSwitcher section: window.addEventListener('close-lang-switcher', ...) - Proper cleanup in useEffect return function
|
Thanks for your pull request. Before we can look at it, you'll need to add a 'DCO signoff' to your commits. 📝 Please follow instructions in the contributing guide to update your commits with the DCO Full details of the Developer Certificate of Origin can be found at developercertificate.org. The list of commits missing DCO signoff:
DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
✅ Deploy Preview for kubestellar-docs ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
Hi @xyaz1313. Thanks for your PR. I'm waiting for a kubestellar member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
👋 Welcome to the KubeStellar community! 💖 Thanks and congrats 🎉 for opening your first PR here! We're excited to have you contributing. Before merge, please ensure:
📬 If you're using KubeStellar in your organization, please add your name to our Adopters list. 🙏 It really helps the project gain momentum and credibility — a small contribution back with a big impact. Resources:
A maintainer will review your PR soon. Hope you have a great time here! 🌟 ~~~~~~~~~~ 🌟 📬 If you like KubeStellar, please ⭐ star ⭐ our repo to support it! 🙏 It really helps the project gain momentum and credibility — a small contribution back with a big impact. |
There was a problem hiding this comment.
Pull request overview
This PR updates the Navbar component’s cross-dropdown coordination by removing the window.closeLangSwitcher global and replacing it with a CustomEvent-based pattern, reducing global namespace pollution and enabling proper listener cleanup.
Changes:
- Replaced calls to
window.closeLangSwitcher()withwindow.dispatchEvent(new CustomEvent("close-lang-switcher"))from hover and Escape key paths. - Added a
window.addEventListener("close-lang-switcher", ...)listener in the LanguageSwitcher initialization flow. - Added a
useEffectcleanup to clear the LanguageSwitcher init timer and unregister the window event listener.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| window.dispatchEvent(new CustomEvent("close-lang-switcher")); | ||
|
|
There was a problem hiding this comment.
The "close-lang-switcher" event name is duplicated in multiple places (two dispatch sites and the listener). To avoid future typos/mismatches, consider extracting it into a single constant (e.g. const CLOSE_LANG_SWITCHER_EVENT = ...) and reusing it for dispatch + add/removeEventListener.
| return () => { | ||
| clearTimeout(langTimer); | ||
| if (langSwitcherCloseHandlerRef.current) { | ||
| window.removeEventListener("close-lang-switcher", langSwitcherCloseHandlerRef.current); | ||
| langSwitcherCloseHandlerRef.current = null; | ||
| } | ||
| }; |
There was a problem hiding this comment.
The new useEffect cleanup only clears the LanguageSwitcher init timer and removes the window "close-lang-switcher" listener, but this effect also registers several other long-lived listeners/observers (e.g. document.addEventListener('keydown', ...), multiple container.addEventListener(...) handlers in initDropdowns, and a MutationObserver in initLanguageSwitcher). If Navbar ever unmounts/remounts, these will accumulate and can cause duplicated Escape handling + memory leaks. Consider capturing those handlers/observer instances and removing/disconnecting them in the same cleanup (or adjust the PR description/tests if full cleanup is out of scope).
|
Hi @xyaz1313 — gentle reminder that this PR is blocked on DCO. The git commit --amend -s # if single commit
# OR
git rebase --signoff master && git push --force-with-lease # for multiple commitsOnce DCO passes, I can bypass tide and merge this. Thanks! |
clubanderson
left a comment
There was a problem hiding this comment.
LGTM. Clean refactor replacing the window.closeLangSwitcher global-pollution pattern with a proper CustomEvent bus. The changes:
- Navbar dispatches
close-lang-switcherCustomEvent instead of calling(window as any).closeLangSwitcher(). - Language switcher listens for the event via
addEventListener. - Listener is properly removed in effect cleanup (via
langSwitcherCloseHandlerRef). setTimeoutid is captured and cleared in cleanup.
Two small pre-existing any ESLint disables got removed as a nice side benefit.
Thanks @xyaz1313. Note to maintainers: external contributor — awaiting human approval to merge.
|
LGTM label has been added. DetailsGit tree hash: dc36feb2439e8ffe98c73867cbc8d5a12e34106c |
|
Thanks @xyaz1313 — the PR is approved and the fix looks correct, but it's blocked on two CI checks:
Once DCO is green we can land this. |
|
@xyaz1313 thanks for the fix — this has git commit --amend -s --no-edit
git push --force-with-lease origin mainLet us know if you need help — once signed, this can merge. |
|
Thanks for the fix! Two items to address before this can merge:
Once DCO passes and conflicts are resolved, this is good to merge. |
|
Thanks for this — the CustomEvent pattern is the right direction and matches what #1471 is also cleaning up (they overlap the same
Once both are resolved I'll |
|
Thanks for the fix! The code approach (CustomEvent over global window function) looks good. The PR is blocked on DCO sign-off. To fix this, please amend your commit(s) to include the sign-off: git commit --amend -s
git push --force-with-leaseThis adds a |
|
Thanks for the fix! Two things to address before this can merge:
Once those are resolved, this should be good to go. |
Fixes #1456
Problem
The component uses to coordinate closing the language switcher when other dropdowns are hovered or Escape is pressed. This:
eslint-disablecomments for@typescript-eslint/no-explicit-anySolution
Replaced the global function with a CustomEvent pattern:
Dispatchers (2 locations)
window.dispatchEvent(new CustomEvent('close-lang-switcher'))Listener (LanguageSwitcher section)
window.addEventListener('close-lang-switcher', handleCloseLangSwitcher)Cleanup
useEffectreturn function removes the event listener on unmountChanges
src/components/Navbar.tsxeslint-disablecomments related towindow.closeLangSwitcherTesting