Modernize JS and tighten things by XhmikosR · Pull Request #3388 · pi-hole/web

XhmikosR · 2025-04-05T16:14:19Z

This is #3338 rebased.

The changes are split but I want to squash the autofix-patches before merging.

Non-whitespace diff: https://github.com/pi-hole/web/pull/3388/files?w=1

TODO for this PR, feel free to push to this branch:

we should really fix the TODOs I've added, but I need help from someone more familiar with these parts of the code

TODO for later, but feel free to push to this branch:

~~reduce for...in usage. We should use for...of along with Object methods because this way we only loop through own keys only so no need to check for Object.hasOwn~~ Already tackled in my jQuery branch
we should stop using hidden global variables. If something is supposed to be global, we need to use globalThis so that it's clear
NEVER ever use var anymore from now on; this is now enforced by the xo rules
~~we should enable strict mode; I'll try to fix this later, but xo by default enforces type: module so it might be tricky~~
I also have a branch based on this one that I started the work to reduce the jQuery usage in our code, see https://github.com/XhmikosR/web/compare/xmr/js...XhmikosR:web:xmr/reduce-jquery?w=1. I plan to open a PR later after this PR is merged

Thank you for your contribution to the Pi-hole Community!

Please read the comments below to help us consider your Pull Request.

We are all volunteers and completing the process outlined will help us review your commits quicker.

Please make sure you

Base your code and PRs against the repositories developmental branch.
Sign Off all commits as we enforce the DCO for all contributions
Sign all your commits as they must have verified signatures
File a pull request for any change that requires changes to our documentation at our documentation repo

What does this PR aim to accomplish?:

How does this PR accomplish the above?:

Link documentation PRs if any are needed to support this PR:

By submitting this pull request, I confirm the following:

I have read and understood the contributors guide, as well as this entire template. I understand which branch to base my commits and Pull Requests against.
I have commented my proposed changes within the code and I have tested my changes.
I am willing to help maintain this change if there are issues with it later.
It is compatible with the EUPL 1.2 license
I have squashed any insignificant commits. (git rebase)
I have checked that another pull request for this purpose does not exist.
I have considered, and confirmed that this submission will be valuable to others.
I accept that this submission may not be used, and the pull request closed at the will of the maintainer.
I give this submission freely, and claim no ownership to its content.

I have read the above and my PR is ready for review. Check this box to confirm

github-actions · 2025-04-21T11:34:53Z

This pull request has conflicts, please resolve those before we can evaluate the pull request.

github-actions · 2025-04-21T11:46:42Z

Conflicts have been resolved.

XhmikosR · 2025-04-22T05:48:04Z

This is ready for review. I don't mark it as ready because I want to rebase to squash the autofix patches before the merge.

Can someone please review so that we move forward?

dschaper · 2025-04-22T15:51:30Z

Tests are not passing and it's in draft status. It is not known if these failing conditions are intentional or not.

XhmikosR · 2025-04-22T15:54:07Z

All tests pass, it's CodeFactor that complains about the two/three TODO comments I have added that would be nice to address, but we can live with it for now if it helps moving forward.

The PR is still in draft because I want to rebase the autofix patches before the merge. I only split them to make review easier.

dschaper · 2025-04-22T17:20:58Z

Okay, I don't remember offhand if changing from Draft to Ready will remove any existing approvals. Would it be better to set the PR as Ready with Squash for the merge strategy?

XhmikosR · 2025-04-22T18:24:08Z

It needs a manual rebase unfortunately. IIRC this report doesn't have the enforcement you described.

…

On Tue, Apr 22, 2025, 20:21 Dan Schaper ***@***.***> wrote: Okay, I don't remember offhand if changing from Draft to Ready will remove any existing approvals. Would it be better to set the PR as Ready with Squash for the merge strategy? — Reply to this email directly, view it on GitHub <#3388 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AACVLNNZ2VWHWLZL6ZPIECT22Z3BBAVCNFSM6AAAAAB2QVB7IWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDQMRRHE4TEMJUHA> . You are receiving this because you authored the thread.Message ID: ***@***.***> *dschaper* left a comment (pi-hole/web#3388) <#3388 (comment)> Okay, I don't remember offhand if changing from Draft to Ready will remove any existing approvals. Would it be better to set the PR as Ready with Squash for the merge strategy? — Reply to this email directly, view it on GitHub <#3388 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AACVLNNZ2VWHWLZL6ZPIECT22Z3BBAVCNFSM6AAAAAB2QVB7IWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDQMRRHE4TEMJUHA> . You are receiving this because you authored the thread.Message ID: ***@***.***>

PromoFaux

My eyes hurt now.

PromoFaux · 2025-04-22T19:34:28Z

scripts/js/footer.js

-  const XForwardedFor = globalThis.atob(advancedInfoSource.data("xff") ?? "");
-  const starttime = parseFloat(advancedInfoSource.data("starttime"));
-  const endtime = parseFloat(advancedInfoSource.data("endtime"));
+  const XForwardedFor = globalThis.atob(advancedInfoSource.data("xff") || "") || null;


I note we go from a "If null then blank string" to "if null then blank string, but if that's null then null"

What difference is made here?

This was wrong before. I changed it so that it gets properly set to null thus the line where it's checked can use the ?? operator.

$("#client-id").text(XForwardedFor ?? clientIP);

scripts/js/groups-domains.js

scripts/js/groups-lists.js

PromoFaux · 2025-04-22T19:54:40Z

scripts/js/index.js

      timeLineChart.data.datasets.push({
        data: [],
        // If we ran out of colors, make a random one
        backgroundColor: colors[i],


I know this isn't part of your changeset, but how does colors[i] make a random one? Maybe a comment that no longer matches the code?

Not sure if the comment is right TBH.

web/scripts/js/index.js

Lines 165 to 168 in f7a07c7

backgroundColor:

i < THEME_COLORS.length

? THEME_COLORS[i]

: "#" + (0x1_00_00_00 + Math.random() * 0xff_ff_ff).toString(16).substr(1, 6),

It's possible that either the comment is wrong or the code wasn't copied from the other location fully.

Someone closer to the code here might be best placed to answer -though it's not a blocker for this PR.

@rdwebdesign / @DL6ER

This was a copy paste error.

If you look at line 51 you will see colors[] has only 4 items. This is used for the first graphic (containing up to 4 items: Blocked, permitted, cached and other). This will never need extra random colors.

The comment about "creating random colors" was copied from lines 161-174, where random colors are actually created if there are more clients than available colors in THEME_COLORS[i].

It's possible that either the comment is wrong or the code wasn't copied from the other location fully.

I think the code was initially copied (probably as a "template") and changed, but the comment was left.

The comment can be removed.

scripts/js/interfaces.js

PromoFaux · 2025-04-22T21:45:51Z

No blockers from a code point of view but I have not had time to test it yet (though I can't see anything that would cause any issues per se)

Happy to ignore the TODO warnings from codefactor.

I'm also not precious about squashing the commits, makes it easier to follow each change.

Signed-off-by: XhmikosR <xhmikosr@gmail.com>

XhmikosR · 2025-04-23T04:55:40Z

Your call. I would squash the autofix patches because that's what I initially did, but not going to lose my sleep over it either :)

XhmikosR · 2025-04-23T05:01:29Z

This is the rebased branch development...XhmikosR:web:xmr/js-2. Let me know and I can force push it here.

PromoFaux · 2025-04-23T16:57:03Z

OK, given each page a look through - and everything appears to still be working. Please FP the branch you mentioned

Signed-off-by: XhmikosR <xhmikosr@gmail.com>

PromoFaux · 2025-04-24T17:10:21Z

@XhmikosR can you push something to this PR please? Codefactor is being... annoying. Maybe close it, delete the branch and then recreate the PR?

XhmikosR · 2025-04-24T17:15:27Z

You will need to bypass Code factor because of the newly added TODO comments.

…

On Thu, Apr 24, 2025, 20:10 Adam Warner ***@***.***> wrote: *PromoFaux* left a comment (pi-hole/web#3388) <#3388 (comment)> @XhmikosR <https://github.com/XhmikosR> can you push something to this PR please? Codefactor is being... annoying. Maybe close it, delete the branch and then recreate the PR? — Reply to this email directly, view it on GitHub <#3388 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AACVLNM6O56H53YV4NR6RJT23ELJJAVCNFSM6AAAAAB2QVB7IWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDQMRYGMYDSNBSG4> . You are receiving this because you were mentioned.Message ID: ***@***.***>

PromoFaux · 2025-04-24T17:16:15Z

You will need to bypass Code factor because of the newly added TODO

Yep - that's what I was trying to do, but had to uninstall it from the org and add it again. It's being really stupid. But I've got it back working for me at least now (nobody else can access the config though)

PromoFaux · 2025-04-24T17:17:14Z

Holy crap that was hard work

github-actions bot added Merge Conflicts and removed Merge Conflicts labels Apr 21, 2025

PromoFaux reviewed Apr 22, 2025

View reviewed changes

XhmikosR added 2 commits April 23, 2025 07:53

Tighten xo rules

f8a0a1d

Signed-off-by: XhmikosR <xhmikosr@gmail.com>

Manual xo fixes and suppressions

f598a8e

Signed-off-by: XhmikosR <xhmikosr@gmail.com>

XhmikosR added 2 commits April 23, 2025 20:50

xo: enable strict mode

756239a

Signed-off-by: XhmikosR <xhmikosr@gmail.com>

Fix package name to be valid

3338145

Signed-off-by: XhmikosR <xhmikosr@gmail.com>

PromoFaux closed this Apr 24, 2025

PromoFaux reopened this Apr 24, 2025

PromoFaux closed this Apr 24, 2025

PromoFaux reopened this Apr 24, 2025

PromoFaux approved these changes Apr 24, 2025

View reviewed changes

PromoFaux merged commit d9a7acf into pi-hole:development Apr 24, 2025
16 checks passed

PromoFaux mentioned this pull request May 6, 2025

Pi-hole Web v6.2 #3444

Merged

	backgroundColor:
	i < THEME_COLORS.length
	? THEME_COLORS[i]
	: "#" + (0x1_00_00_00 + Math.random() * 0xff_ff_ff).toString(16).substr(1, 6),

Uh oh!

Conversation

XhmikosR commented Apr 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Thank you for your contribution to the Pi-hole Community!

Uh oh!

github-actions bot commented Apr 21, 2025

Uh oh!

github-actions bot commented Apr 21, 2025

Uh oh!

XhmikosR commented Apr 22, 2025

Uh oh!

dschaper commented Apr 22, 2025

Uh oh!

XhmikosR commented Apr 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dschaper commented Apr 22, 2025

Uh oh!

XhmikosR commented Apr 22, 2025 via email

Uh oh!

PromoFaux left a comment

Choose a reason for hiding this comment

Uh oh!

PromoFaux Apr 22, 2025

Choose a reason for hiding this comment

Uh oh!

XhmikosR Apr 22, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

PromoFaux Apr 22, 2025

Choose a reason for hiding this comment

Uh oh!

XhmikosR Apr 22, 2025

Choose a reason for hiding this comment

Uh oh!

PromoFaux Apr 22, 2025

Choose a reason for hiding this comment

Uh oh!

rdwebdesign Apr 22, 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!

rdwebdesign Apr 22, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

PromoFaux commented Apr 22, 2025

Uh oh!

XhmikosR commented Apr 23, 2025

Uh oh!

XhmikosR commented Apr 23, 2025

Uh oh!

PromoFaux commented Apr 23, 2025

Uh oh!

PromoFaux commented Apr 24, 2025

Uh oh!

XhmikosR commented Apr 24, 2025 via email

Uh oh!

PromoFaux commented Apr 24, 2025

Uh oh!

Uh oh!

PromoFaux commented Apr 24, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

XhmikosR commented Apr 5, 2025 •

edited

Loading

XhmikosR commented Apr 22, 2025 •

edited

Loading

rdwebdesign Apr 22, 2025 •

edited

Loading