Page editor: Use single source of truth for URL change #8074
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main pixiebrix/pixiebrix-extension#8074 +/- ##
==========================================
- Coverage 73.22% 73.22% -0.01%
==========================================
Files 1310 1310
Lines 40733 40734 +1
Branches 7568 7568
==========================================
Hits 29826 29826
- Misses 10907 10908 +1 ☔ View full report in Codecov by Sentry. |
| export function updatePageEditor() { | ||
| navigationEvent.emit(); | ||
| export default function useGrantedPermissions() { | ||
| return useAsyncExternalStore(subscribe, getSnapshot); |
There was a problem hiding this comment.
Gotta love this hook
| jest | ||
| .mocked(useCurrentInspectedUrl) | ||
| .mockReturnValue("https://test.url#updated-url"); | ||
| rerender(<PanelContent />); |
There was a problem hiding this comment.
Is this the right way to mock state changes and listen to useEffect calls? It works but I wonder if there's a better way
cc @fungairino
| const dynamicElement = formStateToDynamicElement(activeElement); | ||
| updateDynamicElement(allFramesInInspectedTab, dynamicElement); | ||
| } | ||
| }, [dispatch, activeElement]); |
There was a problem hiding this comment.
Note: this attempted to connectToContentScript when just the activeElement changed
| if (currentUrl && permissions) { | ||
| dispatch(tabStateActions.connectToContentScript()); | ||
| } | ||
| }, [dispatch, currentUrl, permissions]); |
There was a problem hiding this comment.
We no longer add/remove URL permissions, so maybe we can drop this?
The only reason to keep it would be if the "permission changed" events are fired via enterprise policy change. I don't know if that's the case though
|
Disregard, this is actually kinda wrong. The page editor should only connect to the content script when the document reloads, not when the URL changes. |
What does this PR do?
I was reviewing possible improvements for https://github.com/pixiebrix/pixiebrix-source/issues/322 and saw a comment mentioning this incongruence.
We already have a hook that listens to URL changes, we should use it to ensure UI consistency and avoid multiple listeners.
I think part of this can actually be dropped since we don't really do "URL permissions" anymore
Checklist