Added global appstate object to track article and asset Loading #1310

THEBOSS0369 · 2025-02-17T06:39:44Z

This PR Fixes #1126

This PR implements a centralized loading state management system using the global appState object, i have also added an universal messaging without direct DOM manipulation.

Test

I have done all the necessary test

Checked in both "Restricted" and "ServiceWorker" Everything is working fine.
Unit tests npm test no issue
End-to-end (e2e) tests-e2e-firefox-> All tests Passed.
extension versions with production code tested
Browser Test -> Microsoft Edge.

Signed-off-by: THEBOSS0369 <[email protected]>

Jaifroid · 2025-03-19T13:08:49Z

@THEBOSS0369 Could you take a look at the Codefactor issues, and try to fix?

THEBOSS0369 · 2025-03-19T15:15:34Z

@Jaifroid out of curiosity, i was wondering if the codefactor checker has been updated recently or any other module related to it? as this codefactor test didn't failed last time i pushed the changes

Jaifroid · 2025-03-19T15:18:05Z

No, nothing has changed, but I do find codefactor to be a bit "inconsistent" 😅 !

Jaifroid · 2025-03-23T10:39:29Z

@THEBOSS0369 Sorry it's taken so long to test this one. I'm seeing a repeated error in console with this branch. See screenshot. Please check and debug it carefully! Also, branch needs updating again. Thanks!

Jaifroid

Please fix error clickedAnchor is not defined.

THEBOSS0369 · 2025-03-23T12:15:21Z

@Jaifroid Sir can you please help me to recreate that error because i am not able to recreate it in my system. i have made sure that my branch is updated with the latest changes and i am checking this on the global-appstate branch (this pr branch) please check the video below for reference

Recording.2025-03-23.173217.mp4

Jaifroid · 2025-03-23T12:36:48Z

@THEBOSS0369 You need to view debug logs! Click on "Default levels" (top-right of Console) and add "Verbose" debugging. It should then show as "All levels".

THEBOSS0369 · 2025-03-23T13:45:55Z

@Jaifroid My sincere apologies sir i am consuming your time but i still not able to recreate after turning verbose
here is the video

i have ensured that i am on correct branch and i have pulled the latest changes in my branch

verbose.mode.mp4

These are my setting in my config if it is causing any issue

THEBOSS0369 · 2025-03-23T13:53:10Z

Also sir if you think this pr demand more time of yours then we can come to this pr later whenever you want if it is not an urgent requirement.

Jaifroid · 2025-03-23T14:28:05Z

OK, let me check.

Unrelated, but just so you know, testing on the library previews is not a valid test, as that code is nothing to do with Kiwix JS -- that's Kiwix Serve running on a different server library.kiwix.org. The only valid tests are those done on local ZIMs loaded in the app (like your Wikivoyage test).

I'll see if I can isolate this discrepancy.

Jaifroid · 2025-03-23T16:09:10Z

@THEBOSS0369 My apologies, after puzzling over this for a while, I realized that there was a conditional breakpoint in my DevTools that was somehow only being evaluated on your branch, but not on main. The error was being produced by the attempt to evaluate a value that no longer exists at that spot in the code.

So, nothing to do with your PR. 🙏😊

I'm just doing some last tests...

Jaifroid

Although this PR is on the right tracks, it doesn't really achieve what the issue was envisaging. All this PR does is replace a simple uiUtil.spinnerDisplay with some "helper" functions that do exactly the same thing, ultimately making the code more complicated.

The display of asset loading is the new feature, but this only works in Restricted mode, as there is no similar code added for ServiceWorker mode. I'll explain in another post how this needs to be handled.

Jaifroid · 2025-03-23T16:14:29Z

www/js/app.js

+// eslint-disable-next-line no-unused-vars
+function updateUI () {
+    if (appstate.isLoadingArticle || appstate.isLoadingAsset) {
+        uiUtil.spinnerDisplay(true, appstate.loadingMessage);
+    } else {
+        uiUtil.spinnerDisplay(false);
+    }
+}
+


What's the point of this function if it's not used? You've disabled eslint errorchecking for it, but that doesn't fix the underlying issue that we have unused code... You should just remove it if it is of no use.

For this section i initially thought that i can directly add the spinner display logic in this instead of stop or start loading. But i think start and stop logic is working fine so its not required anymore i will remove it.

Jaifroid · 2025-03-23T16:21:08Z

www/js/app.js

- */
-var appstate = {};
+ * @type Object */
+var appstate = {


As we try to modernize code as we work on it, I suggest you change this var to const.

Jaifroid · 2025-03-23T16:22:46Z

www/js/app.js

@@ -3023,6 +3058,8 @@ function displayArticleContentInIframe (dirEntry, htmlArticle) {
            }
            // Get the image URL
            var imageUrl = image.getAttribute('data-kiwixurl');
+            // Start Loading the imaeg and update appstate


Typo ;-) You want "image", not "imaeg"

Jaifroid · 2025-03-23T16:26:15Z

www/js/app.js

@@ -3023,6 +3058,8 @@ function displayArticleContentInIframe (dirEntry, htmlArticle) {
            }
            // Get the image URL
            var imageUrl = image.getAttribute('data-kiwixurl');
+            // Start Loading the imaeg and update appstate
+            startLoadingAsset('Loading Image: ' + imageUrl);


This string isn't translated! All strings to be displayed in the UI must be passed through the translation system.

Jaifroid · 2025-03-23T17:23:57Z

OK, so the idea of the issue was to add a global state object that would be available for use in the backend (e.g. in zimArchive, where HTML and binary files are loaded at a low level, and also in zimArchive.callLibzimWorker, and potentially other parts of the backend.

The tricky part is working out how to do this without creating a dependency of the backend on the UI, which is a no-no.

So, we just want a global variable that can be updated in zimArchive (but defined elsewhere). Elsewhere in the code, probably in uiUtil, while the spinner is showing, a timer process would check the variable's state and if it has changed, would update the UI. This way we don't break the architecture of the app, while still allowing for cross-module communication.

One possibility would be to define appstate in init.js, alongside params as a true global variable. But I think this could create problems for how we use appstate for search. So it might be better to define a different object there, maybe just appMessages. The main problem here is that the app can have more than one window open, and all requests from different windows are trapped by the Service Worker, so we could end up with cross-contamination if two windows happened to be getting assets or articles at the same time.

So, the architecture of this needs more thought... I don't have enough time to plan this out fully right now. Let me know if you have any ideas.

Jaifroid · 2025-03-23T22:58:10Z

As you say, this isn't an urgent PR. I thought it would be fairly simple, but turns out to have a few complications and needs more thought to re-architecture. I think we should focus on the Wikipedia Dark Mode first, while thinking about this.

THEBOSS0369 · 2025-03-24T05:25:07Z

@Jaifroid Sir this is the solution i can think of. it may not be perfect but at least it can give us some ideas to explore

so i am thinking that instead of using a global variable we can create something like a specific message system in init.js and then zimArchive can send the updates and then ui can get that without direct ui dependency.

For multiple window we can add a system such that every window will have its own unique id when it starts and when sending the request the id can tell from which window the request is coming and then sw mode can handle it properly before showing it to user. although i've not work on window functionality very much so after researching about it for a while this is what i was able to come up with.

Let me know if any of the above ideas i mentioned can help us in any way.

As you say, this isn't an urgent PR. I thought it would be fairly simple, but turns out to have a few complications and needs more thought to re-architecture. I think we should focus on the Wikipedia Dark Mode first, while thinking about this.

No objection from my side sir and i'm getting a feeling that this is gonna be a real complex one so it will demand more time of yours and mine.

Signed-off-by: THEBOSS0369 <[email protected]>

THEBOSS0369 added 2 commits February 17, 2025 11:16

Added global appstate object to track article

ff9a963

Signed-off-by: THEBOSS0369 <[email protected]>

Merge branch 'main' into global-appstate

995a371

THEBOSS0369 mentioned this pull request Feb 17, 2025

Use global appState object to track article and asset loading, and provide spinner messaging accordingly #1126

Open

Jaifroid assigned THEBOSS0369 Feb 17, 2025

Jaifroid added cleanup backend user interface labels Feb 17, 2025

Jaifroid added this to the v4.2 milestone Feb 17, 2025

THEBOSS0369 added 2 commits March 1, 2025 20:07

Merge branch 'main' into global-appstate

1b6b91e

Merge branch 'main' into global-appstate

220fc00

Update app.js

3673c3b

Jaifroid self-requested a review March 23, 2025 10:40

Jaifroid requested changes Mar 23, 2025

View reviewed changes

Merge branch 'main' into global-appstate

cb3a146

Jaifroid requested changes Mar 23, 2025

View reviewed changes

THEBOSS0369 added 2 commits March 24, 2025 11:47

Fixed some requested changes

2f4bd27

Signed-off-by: THEBOSS0369 <[email protected]>

Merge branch 'main' into global-appstate

ba0fdea

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added global appstate object to track article and asset Loading #1310

Added global appstate object to track article and asset Loading #1310

THEBOSS0369 commented Feb 17, 2025

Jaifroid commented Mar 19, 2025

THEBOSS0369 commented Mar 19, 2025

Jaifroid commented Mar 19, 2025

Jaifroid commented Mar 23, 2025

Jaifroid left a comment

THEBOSS0369 commented Mar 23, 2025 •

edited

Loading

Jaifroid commented Mar 23, 2025

THEBOSS0369 commented Mar 23, 2025 •

edited

Loading

THEBOSS0369 commented Mar 23, 2025

Jaifroid commented Mar 23, 2025

Jaifroid commented Mar 23, 2025

Jaifroid left a comment

Jaifroid Mar 23, 2025

THEBOSS0369 Mar 24, 2025 •

edited

Loading

Jaifroid Mar 23, 2025

Jaifroid Mar 23, 2025

Jaifroid Mar 23, 2025

Jaifroid commented Mar 23, 2025 •

edited

Loading

Jaifroid commented Mar 23, 2025

THEBOSS0369 commented Mar 24, 2025 •

edited

Loading

Added global appstate object to track article and asset Loading #1310

Are you sure you want to change the base?

Added global appstate object to track article and asset Loading #1310

Conversation

THEBOSS0369 commented Feb 17, 2025

Test

Jaifroid commented Mar 19, 2025

THEBOSS0369 commented Mar 19, 2025

Jaifroid commented Mar 19, 2025

Jaifroid commented Mar 23, 2025

Jaifroid left a comment

Choose a reason for hiding this comment

THEBOSS0369 commented Mar 23, 2025 • edited Loading

Jaifroid commented Mar 23, 2025

THEBOSS0369 commented Mar 23, 2025 • edited Loading

THEBOSS0369 commented Mar 23, 2025

Jaifroid commented Mar 23, 2025

Jaifroid commented Mar 23, 2025

Jaifroid left a comment

Choose a reason for hiding this comment

Jaifroid Mar 23, 2025

Choose a reason for hiding this comment

THEBOSS0369 Mar 24, 2025 • edited Loading

Choose a reason for hiding this comment

Jaifroid Mar 23, 2025

Choose a reason for hiding this comment

Jaifroid Mar 23, 2025

Choose a reason for hiding this comment

Jaifroid Mar 23, 2025

Choose a reason for hiding this comment

Jaifroid commented Mar 23, 2025 • edited Loading

Jaifroid commented Mar 23, 2025

THEBOSS0369 commented Mar 24, 2025 • edited Loading

THEBOSS0369 commented Mar 23, 2025 •

edited

Loading

THEBOSS0369 commented Mar 23, 2025 •

edited

Loading

THEBOSS0369 Mar 24, 2025 •

edited

Loading

Jaifroid commented Mar 23, 2025 •

edited

Loading

THEBOSS0369 commented Mar 24, 2025 •

edited

Loading