fix: prevent excessive requests to access endpoint #7597
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
1 Skipped Deployment
|
|
No changes to documentation |
|
Component Testing Report Updated Oct 9, 2024 10:03 AM (UTC) ✅ All Tests Passed -- expand for details
|
⚡️ Editor Performance ReportUpdated Wed, 09 Oct 2024 10:14:49 GMT
Detailed information🏠 Reference resultThe performance result of
🧪 Experiment resultThe performance result of this branch
📚 Glossary
|
|
Hey @rexxars. I've been reading through the changes, and the premise seems good to me. I could do with spending a bit more time to give a definitive 👍/👎. You mentioned you want to add some tests and that you opened this PR for an early look. Are you hoping to get this change merged by tomorrow? |
Might be a bit hopeful, I don't think realistically I/we will have much time for it this week :/ |
|
Okay, thanks for confirming. I will come back to it, as I'm likely to run out of time before tomorrow. |
rexxars
force-pushed
the
fix/excessive-access-requests
branch
from
ce02aa3 to
791cd61
Compare
Rendering the banner also triggers requests to APIs and calls other hooks. This is potentially expensive. The contents of the banner is never shown if the user has the right access to the document, and thus it is more efficient to do this check outside of the banner, before mounting it. Additionally, the name isn't quite right; it doesn't really _check_ for permissions, it just shows an error if told that it doesn't have the correct permissions. I've therefore chosen to rename it to `InsufficientPermissionsBanner` as I find that more precise.
rexxars
force-pushed
the
fix/excessive-access-requests
branch
from
791cd61 to
f7cfc6f
Compare
juice49
reviewed
Oct 14, 2024
| subscriptionRef.current = null | ||
| } | ||
| } | ||
| }, [grantsStore, document, permission]) |
There was a problem hiding this comment.
This is one potential difference I can see between the createHookFromObservableFactory implementation and your implementation: the former uses useUnique, which in turn uses lodash.isEqual to compare the args. You instead have added an effect with object dependencies that will behave differently.
Looks like you may have mitigated this by using disctinctUntilChanged with lodash.isEqual in the permissions$ observable—and there doesn't seem to be any issues—but thought I'd mention it in case.
juice49
approved these changes
Oct 14, 2024
bjoerge
approved these changes
Oct 14, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This one needs a proper read-through, I am not super confident with these changes - especially the last commit. There might be a lot easier ways to solve the problem outlined in the title here, but let me explain:
When researching why there were constant requests being fired to the
/accessAPI, I saw that it was due to a hook being run inside of the<PermissionCheckBanner />. Upon further inspection, I saw that this component remounts (unmounts and mounts) constantly while edits are going on, due to the continuous evaluation of permissions as the document changes. When this happens, theisPermissionsLoadingstate gets set totrue, which triggers the banner to unmount. Once it's calculated if it should be allowed/disallowed, it re-mounts. My hunch is that this is not only causing the additional requests that we want to get rid of, but potentially also affects performance negatively.While reading the code of the
useDocumentValuePermissionsand the underlyingcreateHookFromObservableFactoryand friends, it seems like the intention is to keep the old state while recalculating - but because thedocumentbeing passed as an argument is recreated on any edit, the observable factory gets called again and the state resets. In my mind it would be better (for the permission check in particular) to assume that the grants don't change that quickly, and keep the old cached value around. I refactored the code to do this, but the code is a lot simpler/more naive thancreateHookFromObservableFactory+useObservable, so I'd be happy to have some more eyes on this and see if there are edge cases it wouldn't account for.Note
While writing this, I realized that we should probably throw away that cached value on document ID change.
Running the new eFPS suite, the eFPS looks about the same, while blocking time seems lower - lets see what another run says once I open the PR.
Aside from the above, I also refactored some code a bit - using observable requests for cancellation, minimizing state updates with predefined constants and preferring to "return early" from functions for less indentation (easier to read in my book).
What to review
I'd do commit-by-commit - most of the changes should be fairly straightforward, except the last two. In particular, the last change needs a very thorough read-through.
Testing
I want to add some tests for
useDocumentValuePermissions, but figured I would open this up for an early look since it needs a proper read-through.Notes for release