-
Notifications
You must be signed in to change notification settings - Fork 25
perf!: space permission loading #752
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
Conversation
be3f673 to
80d52e2
Compare
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.
Pull Request Overview
This PR shifts space permission handling to on-demand graph API requests, replacing the old member property with graphPermissions, and updates UI components and tests to handle the new model and loading states.
- Load
graphPermissionsper space when needed and show spinners until ready - Remove
SpaceMemberandgetPermissionsForSpaceMember, migrate tests to usePermission - Update components to use
root.permissionsand add batch/individual loading indicators
Reviewed Changes
Copilot reviewed 54 out of 54 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| packages/web-app-files/tests/unit/components/Spaces/SpaceHeader.spec.ts | Wait for async permission load in SpaceHeader tests |
| packages/web-app-files/tests/unit/components/Spaces/SpaceContextActions.spec.ts | Update test helper signature for SpaceContextActions |
| packages/web-app-files/src/views/spaces/Projects.vue | Add batchActionsLoading prop and load graph permissions on select |
| packages/web-app-files/src/components/Spaces/SpaceHeader.vue | Load member count via graph API and show spinner |
| packages/web-app-files/src/components/Spaces/SpaceContextActions.vue | Introduce loading prop and spinner for actions loading |
| packages/web-app-files/src/components/SideBar/Shares/SpaceMembers.vue | Refactor space fetch to use lazy graphPermissions |
| packages/web-app-files/src/components/SideBar/Shares/Collaborators/ListItem.vue | Adapt to new permissions model and reload user permissions |
| packages/web-app-files/src/components/SideBar/Shares/Collaborators/InviteCollaboratorForm.vue | Remove obsolete graphRoles usage and use graphPermissions |
| packages/web-app-files/src/components/AppBar/CreateAndUpload.vue | Remove old graphRoles param when refreshing drive |
| packages/web-app-external/src/App.vue | Remove getPermissionsForSpaceMember and use graphPermissions |
| packages/web-app-admin-settings/tests/unit/components/Spaces/SpacesList.spec.ts | Update fixture to mock root.permissions instead of members |
| packages/web-app-admin-settings/tests/unit/components/Spaces/SideBar/snapshots/MembersPanel.spec.ts.snap | Update snapshot keys from members to permissions |
| packages/web-app-admin-settings/tests/unit/components/Spaces/SideBar/MembersRoleSection.spec.ts | Update test to use Permission model |
| packages/web-app-admin-settings/tests/unit/components/Spaces/SideBar/MembersPanel.spec.ts | Migrate to permissions and adjust filters in tests |
| packages/web-app-admin-settings/src/views/Spaces.vue | Remove obsolete sharesStore and update listAllDrives call |
| packages/web-app-admin-settings/src/components/Users/SideBar/EditPanel.vue | Remove sharesStore and update drives.updateDrive call |
| packages/web-app-admin-settings/src/components/Spaces/SpacesList.vue | Add useSharesStore and implement new permission-mapping helpers |
| packages/web-app-admin-settings/src/components/Spaces/SideBar/MembersRoleSection.vue | Refactor component to script setup and use Permission |
| packages/web-app-admin-settings/src/components/Spaces/SideBar/MembersPanel.vue | Refactor to use permissions computed and client-store filters |
| .woodpecker.env | Update OpenCloud commit ID |
Comments suppressed due to low confidence (4)
packages/web-app-files/src/components/Spaces/SpaceHeader.vue:112
- [nitpick]
nextTickis imported but never used in this file. Consider removing it to keep imports clean.
import { computed, inject, nextTick, onBeforeUnmount, onMounted, Ref, ref, unref, watch } from 'vue'
packages/web-app-files/src/components/Spaces/SpaceContextActions.vue:1
- There’s a new
loadingbranch in this component. Consider adding a unit test to verify that the spinner renders whenloadingis true and the menu does not.
<template>
<div v-if="loading" class="oc-flex oc-flex-center oc-my-m">
packages/web-app-files/src/components/SideBar/Shares/SpaceMembers.vue:201
- The variable
resourceis undefined in this scope. You should reference theshareor a properly injectedresourceto retrieve the existinggraphPermissions, or pass it into this function.
upsertSpace({ ...space, graphPermissions: unref(resource).graphPermissions })
packages/web-app-files/src/components/SideBar/Shares/Collaborators/InviteCollaborator/InviteCollaboratorForm.vue:431
- The variable
spaceis not defined here. You likely meant to useresource(the prop) to access the original permissions when upserting.
upsertSpace({ ...updatedSpace, graphPermissions: unref(space).graphPermissions })
8afdb8d to
ddad34c
Compare
Don't rely on the permissions from the space listing anymore because the server will stop sending these soon. Instead, they now get loaded via a separate request per space when needed. This requires a bit more caution in the future because we can't just assume that space permissions are loaded yet. However, we should see substantial performance gains, especially when the server stops sending the permissions in the list requests. Also avoids the sidebar from reloading all the shares when creating or updating them.
289d16e to
0cd7853
Compare
kulmann
left a comment
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.
Noice 🤓 🐎
Description
Don't rely on the permissions from the space listing anymore because the server will stop sending these soon. Instead, they now get loaded via a separate request per space when needed.
This requires a bit more caution in the future because we can't just assume that space permissions are loaded yet. However, we should see substantial performance gains, especially when the server stops sending the permissions in the list requests.
Also avoids the sidebar from reloading all the shares when creating or updating them.
This is breaking for developers because it changes quite a bit in the
web-clientpackage:memberproperty has been removed from theSpaceResourceinterface (usegraphPermissionsinstead)SpaceMembertype has been removedgetPermissionsForSpaceMembermethod has been removedgetDrive,createDrive,updateDrive,listMyDrives,listAllDrivesrefs #706
Follow-up tasks
- expand the permissions in the admin settings as soon as the server is capable of doing that.SpaceHeadercomponent somehow different than by fetching the whole drive.