WebUI: Store durable settings in client data API #23191

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

This PR moves durable 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

Chocobo1 · 2025-09-14T10:39:16Z

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

+        get(key) {
+            return this.#cache.get(key);
+        }
+
+        /**
+         * @param {string[]} keys
+         * @returns {Record<string, any>}
+         */
+        async fetch(keys = []) {
+            const keysToFetch = keys.filter((key) => !this.#cache.has(key));
+            if (keysToFetch.length > 0) {
+                const fetchedData = await this.#fetch(keysToFetch);
+                for (const [key, value] of Object.entries(fetchedData))
+                    this.#cache.set(key, value);
+            }
+
+            return Object.fromEntries(keys.map((key) => ([key, this.#cache.get(key)])));
+        }


On a second look, I will redesign these two function:

get() will first try to get the value from #cache and if it does not exist, fetch it from the server.

fetch() will just get the values from server and store it in cache. It will not return any value but only Promise. This function will become a "prefetch" action. The caller should only use get() for getting the values. The API usage would be simpler this way.

This suggestion is not consistent with usage. See createtorrent.html.

This suggestion is not consistent with usage. See createtorrent.html.

My suggestion will work in createtorrent.html and it will be clearer.

I prefer the current interface and think it makes sense.

I prefer the current interface and think it makes sense.

The interface is confusing. It will produce questions like these: get() isn't really getting the value. It is only getting from the cache. If the cache doesn't already hold the value, it fails. This contracts the assumption that get() is getting value from the server.
And fetch() is also returning the values. Which function is actually preferred to get values? Someone not familiar with the API internals will probably just abuse fetch() for everything. And get() will just be left out.
Also, in order to use get() a previous fetch() is required. Why this restriction? It leads to duplicate code.
And not every time the returned value from fetch() is put into use which leads to wasted computation.

It seems to me you're imaging some convoluted, unlikely scenario because I made different design decisions than you would have. I can't imagine any developer actually facing the confusion you're describing. Good thing is, if people do somehow get confused by that, we can always change it later.

And not every time the returned value from fetch() is put into use which leads to wasted computation.

You're trying to over optimize JS code.

I would also like to note that get() was originally called getCached(), but was changed based on your recommendation. #23191 (comment)

On a second thought, I don't insist on suggested version anymore. It will bring await/async to everywhere which isn't necessary.

However parts of my previous reply still applies:

And fetch() is also returning the values. Which function is actually preferred to get values? Someone not familiar with the API internals will probably just abuse fetch() for everything. And get() will just be left out.

And not every time the returned value from fetch() is put into use which leads to wasted computation.

I can add some jsdoc if you really think people will get confused about which to use.

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

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

Chocobo1 · 2025-09-27T16:04:15Z

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

+        get(key) {
+            return this.#cache.get(key);
+        }
+
+        /**
+         * @param {string[]} keys
+         * @returns {Record<string, any>}
+         */
+        async fetch(keys = []) {
+            const keysToFetch = keys.filter((key) => !this.#cache.has(key));
+            if (keysToFetch.length > 0) {
+                const fetchedData = await this.#fetch(keysToFetch);
+                for (const [key, value] of Object.entries(fetchedData))
+                    this.#cache.set(key, value);
+            }
+
+            return Object.fromEntries(keys.map((key) => ([key, this.#cache.get(key)])));
+        }


I prefer the current interface and think it makes sense.

The interface is confusing. It will produce questions like these: get() isn't really getting the value. It is only getting from the cache. If the cache doesn't already hold the value, it fails. This contracts the assumption that get() is getting value from the server.
And fetch() is also returning the values. Which function is actually preferred to get values? Someone not familiar with the API internals will probably just abuse fetch() for everything. And get() will just be left out.
Also, in order to use get() a previous fetch() is required. Why this restriction? It leads to duplicate code.
And not every time the returned value from fetch() is put into use which leads to wasted computation.

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

Chocobo1 · 2025-09-27T16:43:54Z

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


    // Show Top Toolbar is enabled by default
-    let showTopToolbar = localPreferences.get("show_top_toolbar", "true") === "true";
+    let showTopToolbar = clientData.get("show_top_toolbar") !== false;


Continuing #23191 (comment)

The code has become less straightforward.
Later someone will probably try to change it to (or write new code) clientData.get("show_top_toolbar") === true and only to find out it doesn't work reliably.
This kind of usage hides a lot of elusive details and it just make the code harder to comprehend, maintain and write.

This code is now more straightforward. Rather than some additional parameter that applies in unknown situations, the caller can use native JS to get their default value.

See #23191 (comment)

clientData.get("show_top_toolbar") === true and only to find out it doesn't work reliably.

In what way does this not work reliably?

Rather than some additional parameter that applies in unknown situations

It is always clear that the default value only applies when the actual value doesn't exist. This pattern is extensively used in our C++ code base and in Qt class methods. We never had a problem/confusion with it.

clientData.get("show_top_toolbar") === true and only to find out it doesn't work reliably.

In what way does this not work reliably?

You told me about it. === true comparison will fail on first use when the value doesn't exist. Which is the exact reason why you coded !== false instead.
This hidden pitfall is going to be a nightmare in terms of maintenance and readability. I rather let the default value be explicit and non optional than to allow such trap to exist.

It is always clear that the default value only applies when the actual value doesn't exist. This pattern is extensively used in our C++ code base and in Qt class methods. We never had a problem/confusion with it.

Different languages have different paradigms. In JS, the nullish coalescing operator can be used to assign a default value when the value is nullish (either null or undefined). In our case, the value is only nullish when it does not exist.

This hidden pitfall is going to be a nightmare in terms of maintenance and readability. I rather let the default value be explicit and non optional than to allow such trap to exist.

Sorry, I really just don't see this as true. To me, my solution is modern idiomatic 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

Chocobo1 · 2025-09-28T14:57:59Z

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


    // Show Top Toolbar is enabled by default
-    let showTopToolbar = localPreferences.get("show_top_toolbar", "true") === "true";
+    let showTopToolbar = clientData.get("show_top_toolbar") !== false;


Rather than some additional parameter that applies in unknown situations

It is always clear that the default value only applies when the actual value doesn't exist. This pattern is extensively used in our C++ code base and in Qt class methods. We never had a problem/confusion with it.

clientData.get("show_top_toolbar") === true and only to find out it doesn't work reliably.

In what way does this not work reliably?

You told me about it. === true comparison will fail on first use when the value doesn't exist. Which is the exact reason why you coded !== false instead.
This hidden pitfall is going to be a nightmare in terms of maintenance and readability. I rather let the default value be explicit and non optional than to allow such trap to exist.

Chocobo1 · 2025-09-28T16:05:16Z

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

+        /**
+         * @param {Record<string, any>} data
+         */
+        async set(data, suppressError = false) {


After looking closely at the backend API, I kept wondering why not just serialize the cached data into a single json string in WebUI and store/load it at the server under a single key? This seems a lot simpler to me.
Also, what benefits does it bring for using multiple keys than just a single one?

We don't want to transmit the entire JSON blob any time we want to update a single key/value. The current approach is more granular.

Chocobo1 · 2025-10-11T09:34:15Z

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

+        get(key) {
+            return this.#cache.get(key);
+        }
+
+        /**
+         * @param {string[]} keys
+         * @returns {Record<string, any>}
+         */
+        async fetch(keys = []) {
+            const keysToFetch = keys.filter((key) => !this.#cache.has(key));
+            if (keysToFetch.length > 0) {
+                const fetchedData = await this.#fetch(keysToFetch);
+                for (const [key, value] of Object.entries(fetchedData))
+                    this.#cache.set(key, value);
+            }
+
+            return Object.fromEntries(keys.map((key) => ([key, this.#cache.get(key)])));
+        }


On a second thought, I don't insist on suggested version anymore. It will bring await/async to everywhere which isn't necessary.

However parts of my previous reply still applies:

And fetch() is also returning the values. Which function is actually preferred to get values? Someone not familiar with the API internals will probably just abuse fetch() for everything. And get() will just be left out.

And not every time the returned value from fetch() is put into use which leads to wasted computation.

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/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

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

WebUI: Store durable settings in client data API

7c539f3

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

Uh oh!

WebUI: Store durable settings in client data API #23191

Are you sure you want to change the base?

WebUI: Store durable settings in client data API #23191

Uh oh!

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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

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!

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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Piccirello commented Oct 5, 2025

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!

Piccirello commented Aug 30, 2025 •

edited

Loading

Chocobo1 Sep 27, 2025 •

edited

Loading

Chocobo1 Sep 27, 2025 •

edited

Loading

Chocobo1 Sep 27, 2025 •

edited

Loading

Chocobo1 Sep 27, 2025 •

edited

Loading

Chocobo1 Sep 27, 2025 •

edited

Loading