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

Cleanup widgets and side effects on context invalidated (reload, update) #4258

Closed
2 tasks done
fregante opened this issue Sep 10, 2022 · 10 comments
Closed
2 tasks done
Assignees
Labels
bug Something isn't working developer experience runtime user experience Improve the user experience (UX)

Comments

@fregante
Copy link
Contributor

fregante commented Sep 10, 2022

Follows:

Pixiebrix does a good job at cleaning up the buttons across reloads, but some parts remain. This is probably mostly a DX issue meant to ensure PB developers don't lose their minds while debugging Schrödinger's bugs. 😅

kbar

kbar remains attached and sometimes it conflicts with itself, appearing multiple times on a single keystroke.

Screen Shot 1

Element picker overlay (✅ addressed by #4488)

Elements may remain selected when closing the editor as well, and no way to deselect them.

Screen Shot 2

Sidebar (partially addressed by #4117)

It remains unconnectable until the user manually reloads it. An ErrorBanner component with reload button would be helpful.

Screen Shot 3

Solution

  • Use onContextInvalidated in more places, perhaps wrapped in a React hook if preferred
  • Detect stuck sidebars in the content script instead of reporting "Sidebar not found" messenger errors while the sidebar is on screen
@fregante
Copy link
Contributor Author

fregante commented Jan 9, 2023

I think this is another side effect that isn't being cleaned up. I don't know how I got to this point, but you can see that there are 3 main "events" here, each of which produce like 9 logs:

fix me

@fregante fregante removed their assignment Jan 28, 2023
@BLoe
Copy link
Contributor

BLoe commented Jun 27, 2023

@grahamlangford Worth some investigation here to re-scope, I think some of this has been fixed already, but there might be other, newer examples of the same sort of issue, like #5441 for example is a vaguely similar sort of problem

@fregante
Copy link
Contributor Author

Also a UX issue experienced by Brandon and Corinne, presumably on Triggers that don't deactivate and keep showing the "PB restarted" notification over and over:

@fregante
Copy link
Contributor Author

fregante commented Jan 9, 2024

I found another issue for context menus:

Screenshot 12

Repro seems to be:

  1. Visit https://pbx.vercel.app/navigation
  2. Create context menu mod
  3. Reload extension on chrome://extensions
  4. Click #hash-2

@BLoe
Copy link
Contributor

BLoe commented Jan 9, 2024

@fregante I've been seeing that error a ton on my gmail inbox page as well

@fregante
Copy link
Contributor Author

fregante commented Jan 9, 2024

It's due to the URL change / ajax navigations.

This is likely a consequence of:

Before that PR, the URL changes were managed by the background page, which was killed on a reload so the content script no longer received those updates.

After that PR, the content script lives on. I'll open a PR to stop the listener onContextInvalidated

@fregante
Copy link
Contributor Author

I don't know yet which trigger is causing this, but I'm starting to see this message every time the extension is reloaded. The page doesn't really have anything going on, so the user should not be informed of this.

I'll look into what's triggering it and how to clean up the listener.

Screenshot 9

@fregante fregante changed the title Cleanup widgets and side effects when the content script is unloaded Cleanup widgets and side effects on context invalidated (reload, update) Jan 27, 2024
@fregante
Copy link
Contributor Author

fregante commented Feb 9, 2024

I think we're good for now. If you see anything else sticking around, please reopen

@fregante
Copy link
Contributor Author

TriggerExtensionPoint:eventHandler isn't removed

@fregante fregante reopened this Feb 14, 2024
@fregante
Copy link
Contributor Author

fregante commented Feb 18, 2024

Also the modal stays open and is unclosable. I pushed it but other stuff took priority:

Screen.Recording.6.mov

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working developer experience runtime user experience Improve the user experience (UX)
Development

No branches or pull requests

3 participants