Skip to content
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

feat: Rate-limit image previews #1478

Merged
merged 2 commits into from
Nov 4, 2024
Merged

feat: Rate-limit image previews #1478

merged 2 commits into from
Nov 4, 2024

Conversation

susnux
Copy link
Contributor

@susnux susnux commented Oct 29, 2024

Image preloading should be rate limited, to not overload the server. Now it is set to max. 5 requests at the same time.

@susnux susnux requested review from SystemKeeper and Pytal October 29, 2024 14:42
@susnux susnux added enhancement New feature or request 3. to review labels Oct 29, 2024
@susnux
Copy link
Contributor Author

susnux commented Oct 30, 2024

/backport to stable5

@nickvergessen nickvergessen requested review from Antreesy and DorraJaouad and removed request for SystemKeeper October 30, 2024 18:35
Copy link

@Antreesy Antreesy left a comment

Choose a reason for hiding this comment

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

Everything apart from rate-limiting looks and works fine 🙈

lib/utils/imagePreload.ts Outdated Show resolved Hide resolved
@susnux susnux requested a review from Antreesy November 1, 2024 17:17
@susnux
Copy link
Contributor Author

susnux commented Nov 1, 2024

Everything apart from rate-limiting looks and works fine 🙈

Should be fixed, somehow forgot to return the promise

@susnux susnux force-pushed the feat/rate-limit-previews branch from c8d2341 to 9d8265d Compare November 2, 2024 19:03
* @param url URL of the image
*/
export function preloadImage(url: string): Promise<boolean> {
const { resolve, promise } = Promise.withResolvers<boolean>()
Copy link

Choose a reason for hiding this comment

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

https://caniuse.com/?search=promise.withresolvers

Should be polyfilled for now, until it has normal support?

// @ts-expect-error This does not exist outside of polyfill which this is doing
if (typeof Promise.withResolvers === 'undefined') {
    if (window)
        // @ts-expect-error This does not exist outside of polyfill which this is doing
        window.Promise.withResolvers = function () {
            let resolve, reject
            const promise = new Promise((res, rej) => {
                resolve = res
                reject = rej
            })
            return { promise, resolve, reject }
        };
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it has normal support?

It already has, supported by every browser, also supported by Node - but we need to upgrade to LTS Node version 22.

Also: We already polyfill this for Nextcloud server, so the only problem here are the tests.

Image preloading should be rate limited, to not overload the server.
Now it is set to max. 5 requests at the same time.

Signed-off-by: Ferdinand Thiessen <opensource@fthiessen.de>
Signed-off-by: Ferdinand Thiessen <opensource@fthiessen.de>
@susnux susnux force-pushed the feat/rate-limit-previews branch from 9d8265d to 5cfd115 Compare November 4, 2024 12:56
@susnux susnux merged commit a3b19e7 into main Nov 4, 2024
11 of 12 checks passed
@susnux susnux deleted the feat/rate-limit-previews branch November 4, 2024 13:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Requesting file previews should be limited in concurrency
2 participants