perf(rich_workspace): only add property for parent

[ShGKme]

📝 Summary

Fixes a performance regression for instances with many external storages.

Steps to reproduce

1. Add 300-500 s3 external storages and mount to the root subfolder
2. Enable text app
3. See 10-15s loading time on /apps/files/files for PROPFIND request for local s3. With external s3, like Amazon, it could be 10 times slower.

That was a regression in 28.

s3	28	27
0	0.1s	0.1s
100	3s	0.2s
200	6s	0.5s
300	9s	0.9s
400	12s	1-2s
500	15s	1.1-2.4s

The problem

There is a new <nc:rich_workspace> property in each Node in the response. To add this property (even empty), text checks for a file in every child folder, which could be an external remote storage.

Having a hundred external storages mounted in one parent, it results in a hundred requests to remotes for the data that is not really required to have in advance.

🚧 TODO

• [x] Only add property for parent ignoring deep nodes as it was in the past

💥 Breaking changes

This breaks the Android Files client a bit—it uses a response from a parent to know about description in children.

Broken scenario:

1. Open root
2. Open a child without README
3. Go back to parent
4. Open a child with README
   • Before: README is shown as a description
   • After: README is not shown and a swipe-reload is required

Alternative solution

Only ignore external storages, which has the most performance impact.

🏁 Checklist

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

[juliusknorr]

So the test actually has a comment on this that proofs what we discussed in the call:

https://github.com/nextcloud/text/blob/3a9c4f6f7a0ce3e6d148314311730b35250e34fc/cypress/e2e/propfind.spec.js#L43-L56

@alperozturk96 Can you keep us posted here on if that is possible to address on Android?

[juliusknorr]

Other failure is unrelated.

[marinofaggiana]

Good to know ! @mpivchev

[juliusknorr]

Discussed with @tobiasKaminsky as we couldn't think of a proper way to make the root only result working with android:

• [ ] Check if we can just cache the existence of a rich workspace
• [ ] alternative would be to use a special property for web

[ShGKme]

• alternative would be to use a special property for web

Then performance regression is not fixed on Android, having ~10-100 times slower root update than it was in 27

[juliusknorr]

Then performance regression is not fixed on Android, having ~10-100 times slower root update than it was in 27

True, but this should already have been the case before 28. Though only considered as an alternative, caching is probably the most reasonable

[ShGKme]

True, but this should already have been the case before 28. Though only considered as an alternative, caching is probably the most reasonable

Why? It was added only in 28, or I got it wrong?

[juliusknorr]

No, the clients already request this property since a long time. 28 only introduced a change where the web frontend now also fetches those, so the performance issue is also there earlier, just was never noticed.

[ShGKme]

• Check if we can just cache the existence of a rich workspace

Working on this solution

[codecov[bot]]

Codecov Report

Attention: Patch coverage is 71.42857% with 2 lines in your changes missing coverage. Please review.

Project coverage is 51.86%. Comparing base (d9d209a) to head (be1604f).

Files with missing lines	Patch %	Lines
src/init.js	0.00%	2 Missing :warning:

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #5963      +/-   ##
==========================================
- Coverage   51.86%   51.86%   -0.01%     
==========================================
  Files         478      478              
  Lines       40705    40704       -1     
  Branches      993      993              
==========================================
- Hits        21112    21111       -1     
  Misses      19488    19488              
  Partials      105      105              

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

:rocket: New features to boost your workflow:

• :snowflake: Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• :package: JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[juliusknorr]

Initially added a commit here but split it now into https://github.com/nextcloud/text/pull/6891

[juliusknorr]

After this came up again as a performance bottleneck also for local folders, I pushed a change as an additional commit to introduce a separate property that only returns the workspace on the root node.

This is mainly to keep backward compatibility for the mobile clients that as far as I understood @tobiasKaminsky do not have a good way to work with only the parent workspace result. Performance wise the clients should also switch to the flat property at some point, so maybe you can check if that is still the case.

[juliusknorr]

Blackfire proof of saving queries on a folder with 100 subfolders that have a readme of random casing each (Readme/README/readme)

[tobiasKaminsky]

@juliusknorr if there is no "regression", then client will work. For future: to which property should the clients switch?

[juliusknorr]

Can you test the clients with this specific commit https://github.com/nextcloud/text/pull/5963/commits/e5efd6477f689d69d4cdbb00d65c46af9dd3fe40 ? That would help to decide if we actually need the extra one.

I remember in the past the clients were confused when navigating around folders as the property was then intitially not reported on the child folders, only once you did a propfind directly to it.

[icewind1991]

failing cypress test seems related

[icewind1991]

Can this be finished as it's preventing workspace from being enabled on c.nc.c

[max-nextcloud]

Can you test the clients with this specific commit e5efd64 ? That would help to decide if we actually need the extra one.

@tobiasKaminsky Could you test? (see https://github.com/nextcloud/text/pull/5963#issuecomment-2792124269)