WebUI: Store persistent settings in client data API #23191

Piccirello · 2025-08-30T15:51:49Z

This PR moves persistent WebUI settings from browser local storage to the new client data API. This allows these settings to be shared across multiple WebUI instances. This does not include settings that are specific to the local client, like window sizes, selected filters, selected tabs, and sorted columns. I also don't want to get hung up on a debate over whether these should be included - they can always be added later.

Depends on #23088. Closes #12100

src/webui/www/private/scripts/client-data.js

src/webui/www/private/scripts/addtorrent.js

Chocobo1 · 2025-09-12T19:49:51Z

src/webui/www/private/scripts/client.js

+    let clientDataPromise;
    const setup = () => {
        // fetch various data and store it in memory
+        clientDataPromise = window.qBittorrent.ClientData.get([


This should have a "MigrationVersion" field. This functionality will be required eventually. I don't insist it to be done in this PR but you should add it in a later PR.
Look at LocalPreferences.upgrade() for an example.

I agree it will likely eventually be needed. Since this API accepts arbitrary values, I think we can wait to add it until we actually need to use it for the first time.

I think we can wait to add it until we actually need to use it for the first time.

The actually upgrade() function can be added later but for now you should at least set the key MigrationVersion=1 so we don't have to handle it being missing later.

There's no guarantee it'll be set later - our code will still need a null check. Going with YAGNI on this one. We can add it later when it's needed since there's absolutely zero downside to waiting.

There's no guarantee it'll be set later - our code will still need a null check.

You don't need to check anything. Just set it to 1 unconditionally so that the actual migration code that needs it can just use it.

I'm referring to the code that fetches and checks for the current migration version. There's no guarantee that a migration version will have been set/will exist, so defensive code will looking something like clientData.get("migrationVersion") ?? 1.

src/webui/www/private/scripts/client.js

src/webui/www/private/scripts/client-data.js

src/webui/www/private/scripts/addtorrent.js

src/webui/www/private/scripts/client-data.js

Chocobo1 · 2025-09-27T16:06:03Z

src/webui/www/private/scripts/client.js

+    let clientDataPromise;
    const setup = () => {
        // fetch various data and store it in memory
+        clientDataPromise = window.qBittorrent.ClientData.get([


There's no guarantee it'll be set later - our code will still need a null check.

You don't need to check anything. Just set it to 1 unconditionally so that the actual migration code that needs it can just use it.

src/webui/www/private/scripts/client-data.js

src/webui/www/private/scripts/client.js

Piccirello · 2025-10-05T23:20:59Z

@qbittorrent/bug-handlers can I get a review and get this merged? This will make a big difference for WebUI users and belongs in v5.2.

src/webui/www/private/scripts/client-data.js

src/webui/www/private/scripts/client.js

src/webui/www/private/scripts/client-data.js

Piccirello · 2025-10-18T23:53:36Z

@qbittorrent/bug-handlers is anyone interested in moving this PR forward? @Chocobo1 seems disinterested because we disagree on some points, which means this PR is stuck.

Piccirello · 2025-10-28T15:40:43Z

@qbittorrent/bug-handlers hello? @glassez anyone?

Piccirello · 2025-10-29T00:22:11Z

Since this PR is currently stuck on (IMO) highly subjective opinions that don't affect the correctness of the code, I thought I'd share an excerpt from this excellent article¹ that's currently trending on Hacker News:

Don’t review with a “how would I write it?” filter

One reason engineers leave too many comments is that they review code like this:

Look at a hunk of the diff

Ask themselves “how would I write this, if I were writing this code?”

Leave a comment with each difference between how they would write it and the actual diff

This is a good way to end up with hundreds of comments on a pull request: an endless stream of “I would have done these two operations in a different order”, or “I would have factored this function slightly differently”, and so on.

I’m not saying that these minor comments are always bad. Sometimes the order of operations really does matter, or functions really are factored badly. But one of my strongest opinions about software engineering is that there are multiple acceptable approaches to any software problem, and that which one you choose often comes down to taste.

As a reviewer, when you come across cases where you would have done it differently, you must be able to approve those cases without comment, so long as either way is acceptable. Otherwise you’re putting your colleagues in an awkward position. They can either accept all your comments to avoid conflict, adding needless time and setting you up as the de facto gatekeeper for all changes to the codebase, or they can push back and argue on each trivial point, which will take even more time. Code review is not the time for you to impose your personal taste on a colleague.

https://www.seangoedecke.com/good-code-reviews/ ↩

glassez · 2025-10-29T06:05:56Z

@qbittorrent/bug-handlers hello? @glassez anyone?

You do know that I can't provide a comprehensive review of WebUI PRs (only discuss some common issues), as I am not competent in this area.

Piccirello · 2025-10-29T14:43:19Z

You do know that I can't provide a comprehensive review of WebUI PRs (only discuss some common issues), as I am not competent in this area.

Maybe you can help to mediate the discussion? Or just review the remaining open comments above.

Or is there another person in @qbittorrent/web-developers that can approve this?

glassez · 2025-11-10T07:10:30Z

@Piccirello
#23191 (comment)
The only thing I could add from a general point of view is the following:
If JS, due to its specificities, does not allow for the correct implementation of the cache in this case (i.e., to make the cache transparent, as the cache is supposed to behave), then I would not stick it there at all. Let ClientData interface remain simple and obvious, and the caller takes care of caching (when it is really needed). In any case, this is how it is done currently. Since the cache is not transparent to the caller, they still have to take care of it, but in an uglier way.

Piccirello · 2025-11-10T15:33:45Z

@Piccirello #23191 (comment) The only thing I could add from a general point of view is the following: If JS, due to its specificities, does not allow for the correct implementation of the cache in this case (i.e., to make the cache transparent, as the cache is supposed to behave), then I would not stick it there at all. Let ClientData interface remain simple and obvious, and the caller takes care of caching (when it is really needed). In any case, this is how it is done currently. Since the cache is not transparent to the caller, they still have to take care of it, but in an uglier way.

The only way to make the cache transparent, which I agree would be preferable, would be to propagate async in a bunch of places that are currently sync. So the current approach seems like the right balance. Unfortunately, no one is approving this PR.

glassez · 2025-11-10T16:46:25Z

So the current approach seems like the right balance.

Apologies, I'm not able to understand the balance you're referring to (between what and what).
IMO, if you can't implement correct (transparent) cache the only valid way remaining is having no cache in ClientData:

Let ClientData interface remain simple and obvious, and the caller takes care of caching (when it is really needed). In any case, this is how it is done currently. Since the cache is not transparent to the caller, they still have to take care of it, but in an uglier way.

Unfortunately, no one is approving this PR.

Personally, I still can't figure out exactly what's going on here. But I'm still neutral about WebUI PRs, so my opinion can't be considered as blocking this PR.

glassez · 2025-11-10T16:58:04Z

The only way to make the cache transparent, which I agree would be preferable, would be to propagate async in a bunch of places that are currently sync.

I still don't understand the essence of the problem. Shouldn't some value be retrieved from the server at least once in order to be cached (regardless of how caching is implemented)? Therefore, it should have an asynchronous handler, right? Or is it not the case? Additionally, how do "synchronous" callers know that the data is already present in the cache and can be retrieved? In short, I am unclear about the intended execution flow and where in it these reads/writes occur.

Piccirello · 2025-11-10T23:29:18Z

I still don't understand the essence of the problem. Shouldn't some value be retrieved from the server at least once in order to be cached (regardless of how caching is implemented)? Therefore, it should have an asynchronous handler, right?

The setup() function in client.js performs the async client data fetch from the server. This happens in the same place where our various window.qBittorrent.Cache classes perform their own fetch. In fact, these we reuse the same promise - see cacheAllSettled.

Additionally, how do "synchronous" callers know that the data is already present in the cache and can be retrieved?

All necessary data is cached on client load, so synchronous calls can later be made to the cache without issue. It's the same pattern that we use for the window.qBittorrent.Cache classes. These caches classes are inititalized on load, so later functions can rely on the data already being loaded.

glassez · 2025-11-11T13:49:57Z

All necessary data is cached on client load, so synchronous calls can later be made to the cache without issue.

OK, (I hope) I got it. This became the case after the last change made at the suggestion of @Chocobo1 so now ClientData.fetch() is used only to load data from the server into internal cache. The only method to retrieve the value from the ClientData is ClientData.get().

The setup() function in client.js performs the async client data fetch from the server. This happens in the same place where our various window.qBittorrent.Cache classes perform their own fetch. In fact, these we reuse the same promise - see cacheAllSettled.

I hope this is guaranteed by the design that it will be executed before the dependent code starts executing, so we can assert on caches are populated.

The remaining problem is that the user (of ClientData class) has to set the same keys both at the initial populating (using fetch()) and when getting the values (using get()), and this is done in separate code places, right? So a new contributor can easily be confused and forget to add new key to fetch() call and use get() only. Or am I wrong?

glassez · 2025-11-11T13:56:09Z

The setup() function in client.js performs the async client data fetch from the server. This happens in the same place where our various window.qBittorrent.Cache classes perform their own fetch. In fact, these we reuse the same promise - see cacheAllSettled.

The question is, is it possible to assign retrieved values directly to target variables instead of populating the cache, so that we don't need this cache at all?

Piccirello · 2025-11-11T14:13:35Z

The remaining problem is that the user (of ClientData class) has to set the same keys both at the initial populating (using fetch()) and when getting the values (using get()), and this is done in separate code places, right? So a new contributor can easily be confused and forget to add new key to fetch() call and use get() only. Or am I wrong?

That's true. I feel comfortable this would be caught in testing, since the get() would always return undefined and thus would never work. But an alternative would be to remove the key list from the fetch() and load the entire cache on WebUI load. The downside here is that this will load unnecessary data if the user is using (or previously used) an alternative WebUI that saved data to the cache.

The question is, is it possible to assign retrieved values directly to target variables instead of populating the cache, so that we don't need this cache at all?

The cache is accessed from many different places and classes, so the target variables would need to be global. At that point, I don't see what the benefit would be over having the cache class.

glassez · 2025-11-12T08:38:07Z

The cache is accessed from many different places and classes, so the target variables would need to be global. At that point, I don't see what the benefit would be over having the cache class.

If that's the case, then I agree.

glassez · 2025-11-12T08:49:09Z

I feel comfortable this would be caught in testing, since the get() would always return undefined and thus would never work.

But get() is also supposed to return undefined in case when the specified key is not exists in the server side storage, isn't it?

an alternative would be to remove the key list from the fetch() and load the entire cache on WebUI load. The downside here is that this will load unnecessary data if the user is using (or previously used) an alternative WebUI that saved data to the cache.

An alternative would be to store all the data of the same client under a single key. The disadvantage is that you also won't be able to update the values one at a time, but only all together.
This problem could have been avoided if you hadn't rejected my previous suggestion of adding another level of indirection to the ClientData WebAPI, i.e. the client ID. Then you could retrieve/put either all the values of the same client at once or one value at a time.

Piccirello · 2025-11-12T13:20:50Z

But get() is also supposed to return undefined in case when the specified key is not exists in the server side storage, isn't it?

That's right. What I mean is that testing would show that the saved value is never tried. For example, change a setting, reload, notice the setting isn't retained.

An alternative would be to store all the data of the same client under a single key. The disadvantage is that you also won't be able to update the values one at a time, but only all together. This problem could have been avoided if you hadn't rejected my previous suggestion of adding another level of indirection to the ClientData WebAPI, i.e. the client ID. Then you could retrieve/put either all the values of the same client at once or one value at a time.

I'm going to leave the current code as-is unless you feel strongly that the key list should only appear once. If you do, I'll remove it and have fetch() retrieve the entire contents of client data.

Piccirello · 2025-11-15T20:37:34Z

@qbittorrent/lead-developers any chance of getting this approved this weekend? This feature is fully functional without any performance issues and all feedback has been addressed.

src/webui/www/private/views/preferences.html

src/webui/www/private/views/filters.html

src/webui/www/private/views/preferences.html

Chocobo1 · 2025-11-16T09:16:05Z

src/webui/www/private/views/preferences.html

-                    window.parent.qBittorrent.Client.closeWindow(document.getElementById("preferencesPage"));
-                });
+            // Send data to qBT
+            const results = await Promise.allSettled([


Use .then() and not await for further handling. And therefore this function would not be tainted by async.

I find async/await easier to understand and reason about. There's a reason it's the default in modern async JS.

And therefore this function would not be tainted by async.

I don't know what this means, or why using async would be an issue.

or why using async would be an issue.

Because it turns a sync function into an async one. And generally async should only be declared when necessary. I don't see why applyPreferences would need to be an async operation.

Because it turns a sync function into an async one.

This is a correct statement, but I still don't see the issue. This code is easier to read with async/await than it is with promise chaining. They functionally work the same, but await allows you to limit nesting. I'm not sure why you're so opposed to async/await but, as I stated previously, it is preferred in modern JS.

I don't see why applyPreferences would need to be an async operation.

It produces code that is easier to read, easier to reason about, and easier to understand than promise chaining.

And therefore this function would not be tainted by async.

You view async as taining the code; I view then() as tainting the code. We don't have a style rule about this, which means this is up to the stylistic preference of the PR author. Both work the same and produce the same output.

src/webui/www/private/scripts/dynamicTable.js

src/webui/www/private/views/preferences.html

src/webui/www/private/scripts/client.js

Chocobo1 · 2025-11-16T09:39:01Z

src/webui/www/private/scripts/client.js

+    const initializeClientData = async () => {
+        try {
+            await clientDataPromise;
+        }
+        catch (error) {
+            console.error(`Failed to initialize client data. Reason: "${error}".`);
+        }
+    };
+


This function looks redundant. The setup() is guaranteed to run to completion before DOMContentLoaded event.

Hmm, how can we guarantee this? I'm happy to remove this function if it's not needed.

I'm happy to remove this function if it's not needed.

Yes, I would remove that and inline clientDataPromise as such:

cacheAllSettled = Promise.allSettled([ window.qBittorrent.Cache.buildInfo.init(), window.qBittorrent.Cache.preferences.init(), window.qBittorrent.Cache.qbtVersion.init(), window.qBittorrent.ClientData.fetch([ "add_torrent_default_category", "color_scheme", "dblclick_complete", ...]) ]);

Yes, I would remove that and inline clientDataPromise as such:

And the statement await window.qBittorrent.Client.initializeCaches(); should be moved from load event to DOMContentLoaded event.

You didn't answer my question - how can we guarantee that setup() is complete before DOMContentLoaded executes?

And the statement await window.qBittorrent.Client.initializeCaches(); should be moved from load event to DOMContentLoaded event.

This is existing behavior that I'm not going to modify in this PR. You literally just told me not to do this here.

src/webui/www/private/scripts/client-data.js

sledgehammer999 · 2025-11-25T21:33:23Z

A bit offtopic and sorry if someone has already mentioned this.
I think the correct term here is "persistent" and not "durable".

Piccirello · 2025-11-26T21:44:31Z

A bit offtopic and sorry if someone has already mentioned this. I think the correct term here is "persistent" and not "durable".

Good point. I've updated the naming to "persistent".

src/webui/www/private/views/filters.html

Chocobo1 · 2025-11-28T15:59:12Z

src/webui/www/private/scripts/client-data.js

+        #addKeyPrefix(data) {
+            return Object.fromEntries(Object.entries(data).map(([key, value]) => ([`${this.#keyPrefix}${key}`, value])));
+        }
+
+        #removeKeyPrefix(data) {


I don't insist and I think it would still be helpful to include type comments for these two internal functions.

Chocobo1 · 2025-11-28T16:10:56Z

src/webui/www/private/scripts/client.js

+    const initializeClientData = async () => {
+        try {
+            await clientDataPromise;
+        }
+        catch (error) {
+            console.error(`Failed to initialize client data. Reason: "${error}".`);
+        }
+    };
+


I'm happy to remove this function if it's not needed.

Yes, I would remove that and inline clientDataPromise as such:

cacheAllSettled = Promise.allSettled([ window.qBittorrent.Cache.buildInfo.init(), window.qBittorrent.Cache.preferences.init(), window.qBittorrent.Cache.qbtVersion.init(), window.qBittorrent.ClientData.fetch([ "add_torrent_default_category", "color_scheme", "dblclick_complete", ...]) ]);

Piccirello added this to the 5.2 milestone Aug 30, 2025

Piccirello added the WebUI WebUI-related issues/changes label Aug 30, 2025

Piccirello mentioned this pull request Aug 30, 2025

WebAPI: Support persisting WebUI client data #23088

Merged

Piccirello force-pushed the client-prefs-webui branch 2 times, most recently from bbffd53 to 4b1bf7b Compare September 8, 2025 03:16

Piccirello force-pushed the client-prefs-webui branch from 4b1bf7b to 1bd7ae3 Compare September 12, 2025 17:50

Piccirello marked this pull request as ready for review September 12, 2025 17:50

Piccirello requested a review from a team September 12, 2025 17:50

Chocobo1 reviewed Sep 12, 2025

View reviewed changes

Piccirello force-pushed the client-prefs-webui branch 2 times, most recently from 011ac5a to 30807d0 Compare September 12, 2025 20:29

Chocobo1 reviewed Sep 14, 2025

View reviewed changes

src/webui/www/private/scripts/client-data.js Show resolved Hide resolved

src/webui/www/private/scripts/client-data.js Outdated Show resolved Hide resolved

Chocobo1 reviewed Sep 14, 2025

View reviewed changes

src/webui/www/private/scripts/addtorrent.js Show resolved Hide resolved

Piccirello force-pushed the client-prefs-webui branch 2 times, most recently from 77c79d1 to dcec1fd Compare September 20, 2025 23:14

Piccirello requested review from a team September 27, 2025 02:41

thalieht previously approved these changes Sep 27, 2025

View reviewed changes

Chocobo1 reviewed Sep 27, 2025

View reviewed changes

Chocobo1 requested changes Sep 27, 2025

View reviewed changes

src/webui/www/private/scripts/client.js Outdated Show resolved Hide resolved

Chocobo1 reviewed Oct 11, 2025

View reviewed changes

Piccirello force-pushed the client-prefs-webui branch from dcec1fd to 83978ea Compare October 11, 2025 19:31

Piccirello dismissed thalieht’s stale review via 7c539f3 October 11, 2025 19:42

Piccirello force-pushed the client-prefs-webui branch from 83978ea to 7c539f3 Compare October 11, 2025 19:42

Piccirello requested a review from glassez November 9, 2025 08:39

qBittUser mentioned this pull request Nov 9, 2025

Create endpoints to read/write custom theme settings #23466

Open

This comment was marked as off-topic.

Sign in to view

the0neWhoKnocks mentioned this pull request Nov 9, 2025

[Feature Request]: Use new clientData API to persist data VueTorrent/VueTorrent#2559

Open

1 task

This comment was marked as off-topic.

Sign in to view

Chocobo1 reviewed Nov 16, 2025

View reviewed changes

Piccirello force-pushed the client-prefs-webui branch from 13e22ef to 8d55aaa Compare November 26, 2025 21:42

Piccirello changed the title ~~WebUI: Store durable settings in client data API~~ WebUI: Store persistent settings in client data API Nov 26, 2025

Piccirello mentioned this pull request Nov 26, 2025

WebUI: keep preferences window open after unsuccessful save #23549

Open

Chocobo1 reviewed Nov 28, 2025

View reviewed changes

WebUI: Store persistent settings in client data API

00a1e06

Piccirello force-pushed the client-prefs-webui branch from 8d55aaa to 00a1e06 Compare November 29, 2025 00:48

Uh oh!

WebUI: Store persistent settings in client data API #23191

Are you sure you want to change the base?

WebUI: Store persistent settings in client data API #23191

Conversation

Piccirello commented Aug 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Chocobo1 Sep 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Chocobo1 Sep 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Piccirello commented Oct 5, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Piccirello commented Oct 18, 2025

Uh oh!

Piccirello commented Oct 28, 2025

Uh oh!

Piccirello commented Oct 29, 2025

Don’t review with a “how would I write it?” filter

Footnotes

Uh oh!

glassez commented Oct 29, 2025

Uh oh!

Piccirello commented Oct 29, 2025

Uh oh!

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

glassez commented Nov 10, 2025

Uh oh!

Piccirello commented Nov 10, 2025

Uh oh!

glassez commented Nov 10, 2025

Uh oh!

glassez commented Nov 10, 2025

Uh oh!

Piccirello commented Nov 10, 2025

Uh oh!

glassez commented Nov 11, 2025

Uh oh!

glassez commented Nov 11, 2025

Uh oh!

Piccirello commented Nov 11, 2025

Uh oh!

glassez commented Nov 12, 2025

Piccirello commented Aug 30, 2025 •

edited

Loading

Chocobo1 Sep 27, 2025 •

edited

Loading

Chocobo1 Sep 27, 2025 •

edited

Loading

Piccirello Nov 26, 2025 •

edited

Loading

Piccirello Nov 29, 2025 •

edited

Loading

Piccirello Nov 26, 2025 •

edited

Loading