Skip to content

Conversation

@JammingBen
Copy link
Contributor

@JammingBen JammingBen commented May 26, 2025

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-client package:

  • member property has been removed from the SpaceResource interface (use graphPermissions instead)
  • SpaceMember type has been removed
  • getPermissionsForSpaceMember method has been removed
  • signatures for the following methods have changed: getDrive, createDrive, updateDrive, listMyDrives, listAllDrives

refs #706

Follow-up tasks

- expand the permissions in the admin settings as soon as the server is capable of doing that.

  • get the member count in the SpaceHeader component somehow different than by fetching the whole drive.

@JammingBen JammingBen self-assigned this May 26, 2025
@JammingBen JammingBen force-pushed the feat/lazy-space-actions branch 7 times, most recently from be3f673 to 80d52e2 Compare May 27, 2025 12:18
@JammingBen JammingBen changed the title perf: space permission loading perf!: space permission loading May 27, 2025
@JammingBen JammingBen marked this pull request as ready for review May 27, 2025 12:28
Copilot AI review requested due to automatic review settings May 27, 2025 12:28
Copy link
Contributor

Copilot AI left a 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 graphPermissions per space when needed and show spinners until ready
  • Remove SpaceMember and getPermissionsForSpaceMember, migrate tests to use Permission
  • Update components to use root.permissions and 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] nextTick is 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 loading branch in this component. Consider adding a unit test to verify that the spinner renders when loading is 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 resource is undefined in this scope. You should reference the share or a properly injected resource to retrieve the existing graphPermissions, 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 space is not defined here. You likely meant to use resource (the prop) to access the original permissions when upserting.
upsertSpace({ ...updatedSpace, graphPermissions: unref(space).graphPermissions })

JammingBen and others added 4 commits June 2, 2025 12:43
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.
@kulmann kulmann force-pushed the feat/lazy-space-actions branch from 289d16e to 0cd7853 Compare June 2, 2025 10:43
Copy link
Contributor

@kulmann kulmann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Noice 🤓 🐎

@kulmann kulmann merged commit c17a255 into main Jun 2, 2025
18 checks passed
@kulmann kulmann deleted the feat/lazy-space-actions branch June 2, 2025 12:00
@openclouders openclouders mentioned this pull request Jun 2, 2025
1 task
@openclouders openclouders mentioned this pull request Jun 2, 2025
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants