-
Notifications
You must be signed in to change notification settings - Fork 22
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
#8219: don't let frames control sidebar panel via setPanels #8225
Conversation
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.
Yeah this change makes sense, the iframes should not interact with the sidebar at all. I think all of this logic should be moved as high up as possible, e.g. discard any sidebar extensions when in iframes; discard all sidebar-related messages or ensure that they're never sent to frames.
The sidebar starter brick refuses to activate any mod components when running in a frame:
But there are other calls that made their way to the controller: e.g.,
As mentioned in Future Work, we need to review all the calls in the sidebarController to either ignore or throw an error. But we'll need to ensure we're not breaking backward compatibility for calls in MV2 (e.g., showing sidebar forms from frames) |
One thing I noticed is that the
There's a comment on how the sidebarSlice handles race conditions here: pixiebrix-extension/src/contentScript/sidebarController.tsx Lines 201 to 202 in e1775b5
I'm a bit worried that we could run into this sort of buggy behavior again in the future so I'll spend some time in my spec slicing to investigate if there's any work here we could do to better prevent this sort of issue again. |
What do you mean by not synchronized? Javascript is single-threaded
@fungairino that comment is specifically referring to the case of using the The comment is saying that the sidebarSlice handles the race condition, so the sidebarController doesn't have to handle the race condition |
Will this also fix #8141? If the frames don't receive/handle the message, then we don't have to move the logic. |
I'm thinking about improving the behavior if we have async calls that are removing all the panels and then updating them near the same time. Some way to more deterministically figure out the final state of panels given a series of aync calls to modify the panels, but maybe this is not exactly the right thing to worry about here. |
The sidebar currently handles that and the React app initialization race by enforcing message order. That logic has by and large been working well: pixiebrix-extension/src/sidebar/protocol.tsx Line 121 in 7908c96
|
Not in its current state. We need to clarify the behavior of open sidebar, show sidepanel form, etc. from iframes. I would recommend we merge this PR in though and treat those in separate PRs |
TIL! Thanks for the context! |
I tested in MV2 as well and it looks like it works well! |
When the PR is merged, the first loom link found on this PR will be posted to |
What does this PR do?
Discussion
Demo
@pixies/test/timestamp-panel
Future Work
target: top
? We need to be careful not to break MV2 behavior expecationsChecklist