Skip to content

Perf: Optimize trashed pages#2312

Merged
mejo- merged 3 commits intomainfrom
feature/optimize-trashed-pages
Mar 9, 2026
Merged

Perf: Optimize trashed pages#2312
mejo- merged 3 commits intomainfrom
feature/optimize-trashed-pages

Conversation

@Koc
Copy link
Contributor

@Koc Koc commented Mar 3, 2026

📝 Summary

In our organization we are actively using Collectives. And currently we have 2 performance issues with trashed pages:

  • queries in the loop (each trashed page loaded using separate query) -> load all pages using single WHERE IN query
  • trashed pages loaded immediately on each Collective opening -> delay loading until popup with trashed pages opened

This PR aims to fix both of this issues

🖼️ Screenshots

image image

🚧 TODO

  • I can implement lazy loading for deleted collectives as well, just confirm that you ok with that
image

🏁 Checklist

  • Code is properly formatted (npm run lint / npm run stylelint / composer run cs:check)
  • Sign-off message is added to all commits
  • Tests (unit, integration and/or end-to-end) passing and the changes are covered with tests
  • Documentation (README or documentation) has been updated or is not required

@Koc Koc force-pushed the feature/optimize-trashed-pages branch from baaba14 to 67afc41 Compare March 3, 2026 23:04
@Koc Koc marked this pull request as ready for review March 3, 2026 23:04
@Koc Koc force-pushed the feature/optimize-trashed-pages branch from 67afc41 to fc9ed33 Compare March 3, 2026 23:10
Copy link
Member

@mejo- mejo- left a comment

Choose a reason for hiding this comment

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

Thanks a lot @Koc, once again much appreciated!

Code changes look good to me. I see two problems though:

  1. Deleting a page now results in an error if the trash was not opened before, as deleting the page will move it in pages store from allPages[collectiveIndex] array to allTrashPages[collectiveIndex]. I'd like to keep this logic if the trash pages already got loaded beforehands as this means that allPages and allTrashPages stay in sync. When notify_push is enabled, this even works for pages that got deleted by a remote user.

Maybe we should just change the logic to check if allTrashPages already got initialized for the collective in question and depending on that either only drop from allPages or also add to allTrashPages.

  1. Now getTrashPages() is called each time the page trash is opened. I wonder whether we should check whether allTrashPages already got initialized for the collective in question. Since we cache the store in browser local storage nowadays, we probably still need a way to ensure it gets updated after switching the collective.

So I'd suggest a variable to track whether allTrashPages got initialized for a particular collective that always holds the collectiveIndex for the active collective. If the trash page gets opened and the tracking variable is not set to the current collective yet, we fetch the page trash. If it's already set to the current collective, we don't.

What do you think?

@Koc Koc force-pushed the feature/optimize-trashed-pages branch 2 times, most recently from c0d6846 to fde8ecf Compare March 8, 2026 23:56
@Koc
Copy link
Contributor Author

Koc commented Mar 9, 2026

Hey @mejo-!

Deleting a page now results in an error if the trash was not opened before

I don't see any errors, see attached video with opened console

So I'd suggest a variable to track whether allTrashPages got initialized for a particular collective

Good catch, added loadedTrashedPagesPerCollective. Maybe not the best name, feel free to suggest or push into branch

nextcloud-trashed-pages-2026-03-09_00.58.30.mp4

@Koc Koc mentioned this pull request Mar 9, 2026
5 tasks
@mejo-
Copy link
Member

mejo- commented Mar 9, 2026

I don't see any errors, see attached video with opened console

You'll have to open a collective where the page trash was never fetched before to run into this bug. I'll push a fix for it.

Koc and others added 3 commits March 9, 2026 13:24
Signed-off-by: Kostiantyn Miakshyn <molodchick@gmail.com>
Everytime a collective is opened, we want to fetch the page trash when
opening it a first time to be in sync with the server state. Only
closing and opening the page trash while staying within the collective
should not fetch it again.

Signed-off-by: Jonas <jonas@freesources.org>
If the page trash got not fetched for the collective yet, we cannot add
to it.

Signed-off-by: Jonas <jonas@freesources.org>
@mejo- mejo- force-pushed the feature/optimize-trashed-pages branch from fde8ecf to cd736d6 Compare March 9, 2026 12:24
@mejo- mejo- merged commit 6a73071 into main Mar 9, 2026
60 of 61 checks passed
@mejo- mejo- deleted the feature/optimize-trashed-pages branch March 9, 2026 12:40
@Koc
Copy link
Contributor Author

Koc commented Mar 9, 2026

Thanx for pushing fixes and quick merge 🙏 . The only one thing that I thought that we should store flag that trashed pages are loaded per collective id, not globally.

image

Imagine that we have few collecties and each of them has their own trashed pages. We load trashes pages for collective 1, set flag as true, then open collective 2, try to open trashed pages popup but flag is already set to true

@mejo-
Copy link
Member

mejo- commented Mar 9, 2026

Imagine that we have few collecties and each of them has their own trashed pages. We load trashes pages for collective 1, set flag as true, then open collective 2, try to open trashed pages popup but flag is already set to true

That's why the flag is reset each time the collective is switched. And as the flag is not persisted in browser storage, its scope is limited to the current browser tab. The idea was to make sure that each time a collective gets opened (either by browsing there or by switching the collective from within the app), opening the page trash for the first time will fetch the trashed pages.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants