Live and complete permission list in options #5043
Conversation
fregante
added
user experience
Codecov Report
@@ Coverage Diff @@
## main #5043 +/- ##
==========================================
+ Coverage 59.67% 59.71% +0.03%
==========================================
Files 968 969 +1
Lines 29261 29274 +13
Branches 5599 5603 +4
==========================================
+ Hits 17462 17481 +19
+ Misses 11799 11793 -6
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
This reverts commit 2f672ed.
|
|
||
| browser.permissions.getAll = jest.fn(); | ||
| browser.permissions.onAdded = getChromeEventMocks(); | ||
| browser.permissions.onRemoved = getChromeEventMocks(); |
There was a problem hiding this comment.
They're missing clarkbw/jest-webextension-mock#170
| } | ||
|
|
||
| /** Returns a sorted array of all the permission with details, auto-updating */ | ||
| export default function useExtensionPermissions(): DetailedPermissions { |
There was a problem hiding this comment.
See useAllBlocks. I think we should be using useSyncExternalStore instead of useEffect for external data providers
It's basically the same, but useSyncExternalStore has some rendering operations, especially in React 18 (if/when we upgrade)
It's a bit tricky though because:
- getDetailedPermissions needs to return the same reference to an object if nothing has changed
- You still need the setState since the snapshot call is async
Hmm, on second thought, it might not be possible to use with non-memoized promises. I'm OK with this useEffect approach for now
There was a problem hiding this comment.
Thank you for that! I saw that API a month ago and in your recent PR but I haven't yet thought about using it. I'll have to learn how useState/useReducer/useSyncExternalStore work wrt memoization of objects to avoid unnecessary renders.
Something else I want to investigate is how to more closely link the status of a promise to React without having to have several useEffect/useAsyncState/useState/useCallback calls, including possible cancellation (e.g. include a way to cancel a callback in AsyncButton itself) and throttling/tailing callbacks (e.g. cancel previous HTTP calls when the user is typing in a field).
There was a problem hiding this comment.
I tried using useSyncExternalStore but its name suggests that the store must be sync, which getDetailedPermissions isn't, so this returns a promise:
export default function useExtensionPermissions(): DetailedPermissions {
return useSyncExternalStore(subscribe, getDetailedPermissions); // Promise
}With async data it becomes kind of awkward to manage, probably defeating its purpose. I used useAsyncState for now, which I quite like.
Here's a demo call log for the two functions in this file:
Screen.Recording.2.mov
There was a problem hiding this comment.
Yep, I think for this use case useAsyncState is your best bet
|
When the PR is merged, the first loom link found on this PR will be posted to |
What does this PR do?
This means we won't show a stale view to the user anymore. This change is akin to the Page Editor’s ability to detect other open sessions.
Screen.Recording.1.mov
also on loom: https://www.loom.com/share/84e53912d68a4c0ea09d9a244d893c89
Changes for DEV builds only
"Overlapping" or shadowed permission is an origin permission granted before a broader one. The overlapping permission cannot be revoked because the broader one will still cover it. We hid these permissions in #2778 but they're still there, so it's useful to see them during development.
Checklist