[JENKINS-73968] Extract event handler in `QuickDiskUsagePlugin/sidepanel.jelly` #113

shlomomdahan · 2024-10-21T19:45:07Z

depends on #108 - this version of core has the updated task.jelly file needed for this to work.

Diff from #108: shlomomdahan/cloudbees-disk-usage-simple-plugin@update-base-jenkins-to-2.440.3...shlomomdahan:cloudbees-disk-usage-simple-plugin:JENKINS-73968

Testing done

status quo
CSP report only & restrictive

Submitter checklist

Make sure you are opening from a topic/feature/bugfix branch (right side) and not your main branch!
Ensure that the pull request title represents the desired changelog entry
Please describe what you did
Link to relevant issues in GitHub or Jira
Link to relevant pull requests, esp. upstream and downstream changes
Ensure you have provided tests - that demonstrates feature works or fixes the issue

src/main/resources/com/cloudbees/simplediskusage/QuickDiskUsagePlugin/refresh.js

src/main/resources/com/cloudbees/simplediskusage/QuickDiskUsagePlugin/sidepanel.jelly

basil · 2024-10-22T15:34:05Z

this version of core has the updated task.jelly file needed for this to work

Better not to depend on other PRs if possible. Can the core baseline be bumped in this PR to the minimal LTS version needed for the updated task.jelly?

shlomomdahan · 2024-10-22T15:54:23Z

this version of core has the updated task.jelly file needed for this to work

Better not to depend on other PRs if possible. Can the core baseline be bumped in this PR to the minimal LTS version needed for the updated task.jelly?

Hi @basil,

The version that contains the necessary change appears to have been released in jenkins-2.437
PR

2.437

Should I just change to this version in this PR?

Just so I understand, why wouldn't we just be able to upgrade to the version in #108 ?

Thanks

basil · 2024-10-22T16:18:56Z

Subjective maintainer preference as to what baseline they want to support. Since #108 hasn't been merged yet, that would imply a desire to continue supporting older baselines for the time being.

Please file an alternative to #108 upgrading to the latest LTS that contains jenkinsci/jenkins#7635 (2.440.3) and politely ask the maintainer to please merge one or the other, noting that this PR is blocked on the maintainer's choice.

basil · 2024-10-22T16:22:05Z

src/main/resources/com/cloudbees/simplediskusage/QuickDiskUsagePlugin/refresh-disk-usage.js

+    fetch("refresh", {
+        method: "POST",
+        headers: crumb.wrap({})
+    }).catch(() => {});


Why the change in behavior to ignore errors?

This is to avoid an error message in the console. If I exclude it, the console prints:
Unhandled Promise Rejection: TypeError: Load failed

Is that not a problem with the code before this PR?

I believe the difference is that the old onClick approach immediately triggered a page refresh, so the unresolved fetch promise was interrupted before any rejection could be reported. The new approach using js callbacks keeps the promise alive in the current page context, so we need to handle the promise with .catch()

Ignoring all errors indiscriminately smells wrong. See e.g. articles like https://stackify.com/best-practices-exceptions-java/. Errors should be exceptional, so either an error should not be generated in this case, or we should be catching a specific error (with a comment explaining why it is expected) rather than catching all errors indiscriminately without reasoning.

@basil

Looking into this further and discovered some broken functionality that I am investigating. Marking as draft in meantime.

shlomomdahan · 2024-10-22T17:17:32Z

Subjective maintainer preference as to what baseline they want to support. Since #108 hasn't been merged yet, that would imply a desire to continue supporting older baselines for the time being.

Please file an alternative to #108 upgrading to the latest LTS that contains jenkinsci/jenkins#7635 (2.440.3) and politely ask the maintainer to please merge one or the other, noting that this PR is blocked on the maintainer's choice.

@basil Sure I will do that right now.

For my understanding, can you please link to the page that shows the latest LTS that includes jenkinsci/jenkins#7635 is 2.440.3 ?

I see it skip from 2.426.3 to 2.440.1 with 2.437 in between those releases.

Why wouldn't I file a PR to use 2.440.1 instead of 2.440.3

basil · 2024-10-22T17:37:55Z

https://www.jenkins.io/changelog-stable/ is the list of LTS releases. https://www.jenkins.io/doc/developer/plugin-development/choosing-jenkins-baseline/ explains the reasoning of a x.3 or x.4 vs a x.1 LTS release.

shlomomdahan · 2024-10-22T21:51:26Z

@basil After debugging, I found a clean solution to handle the refresh without errors.

The server-side already handles the page refresh through res.forwardToPreviousPage(req) in doRefresh:

@RequirePOST
public void doRefresh(StaplerRequest req, StaplerResponse res) throws IOException, ServletException {
    refreshData();
    res.forwardToPreviousPage(req);
}

The original onClick version worked because it triggered immediate browser navigation, abandoning the fetch request before any errors could surface. We can instead use the XMLHttpRequest in the updated code. This solution lets the server handle the navigation.

the network log show the redirect being correctly handled.

Request
POST /jenkins/manage/disk-usage-simple/refresh
Accept: */*
Content-Type: application/x-www-form-urlencoded
Jenkins-Crumb: 06363e5ed5b967d82793536aa7cde561456fa084f58a52bd8711fdc8e0baf932
Origin: http://localhost:8080
Referer: http://localhost:8080/jenkins/manage/disk-usage-simple/
Sec-Fetch-Dest: empty
Sec-Fetch-Mode: cors
Sec-Fetch-Site: same-origin
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_7) AppleWebKit/605.1.15 (KHTML, like Gecko) Version/18.0 Safari/605.1.15

Redirect Response
302 Found
Date: Tue, 22 Oct 2024 21:48:51 GMT
Location: http://localhost:8080/jenkins/manage/disk-usage-simple/

Summary
URL: http://localhost:8080/jenkins/manage/disk-usage-simple/
Status: 200 OK
Source: Network
Address: 127.0.0.1:8080

I also did some manual testing by creating a freestyle project that clones the Jenkins repo and observed the disk space after refresh and cleanup.

yaroslavafenkin

CSP wise LGTM

yaroslavafenkin · 2024-10-23T08:37:56Z

src/main/resources/com/cloudbees/simplediskusage/QuickDiskUsagePlugin/refresh-disk-usage.js

@@ -0,0 +1,13 @@
+function refreshDiskUsage(a) {
+
+    var xhr = new XMLHttpRequest();


It feels a bit weird to me that we're switching to older APIs.

Regarding the errors that you're getting with fetch my guess is that the execution order is different in old and new version. callback sounds like something that will be executed after the main action, which is following the href in case of anchor tag. While in onclick that JavaScript would be executed before following the href.

Yaroslav's explanation seems spot on. I just checked and couldn't reproduce the issue.

I cannot say I'm thrilled to go back to an older API :/

Unfortunately since I couldn't reproduce on 2 different browsers, I'm having a hard time giving additional advice. What's for sure is that, as Basil said, we do not want to catch and drop every error, it's bad and might hide a real issue.
What we could consider, however, is to handle this specific error if we understand it.

Do you have additional details about the exception? When the exception is thrown, is the POST request still sent?

@PierreBtz

I tested the issue in Chrome and was unable to reproduce it. It appears this issue is specific to Safari, and I apologize for not testing in other browsers earlier. The error details were quite limited, but it's clear that Safari was causing the problem, even though the server handled the page refresh correctly, and the POST requests were sent without issue.

In Chrome, the fetch API works as expected.

src/main/resources/com/cloudbees/simplediskusage/QuickDiskUsagePlugin/refresh-disk-usage.js

basil · 2024-10-29T18:52:05Z

src/main/resources/com/cloudbees/simplediskusage/QuickDiskUsagePlugin/refresh-disk-usage.js

@@ -0,0 +1,13 @@
+function refreshDiskUsage(a, ev) {


This code was broken before this PR (presumably due to some previous Jenkins core changes) so I fixed the existing code and retested. The new code works like jenkinsci/jenkins#9787 where the callback takes two elements, the element and the event. The element isn't actually used, but it could be used to fetch data attributes for example. When the callback is executed, the POST request is made, and then we call ev.preventDefault() to prevent link navigation from taking place. When the POST response is received, we display a notification on the notification bar for either success or failure, using the appropriate color.

Great stuff, thanks!

basil

Tested the most recent set of changes manually, and functionality is working correctly on Firefox with no CSP violations. I tested pressing the Refresh button and verified that the page did not reload but that instead a green notification that the refresh was triggered was shown in the notification bar. I also confirmed in the Network tab of my browser developer tools that the POST request and response were sent/received successfully.

basil · 2024-11-04T19:20:44Z

@PierreBtz Gentle ping, is this ready for merge/release?

PierreBtz

lgtm, thanks for the contribution!

shlomomdahan added 2 commits October 21, 2024 14:59

Require 2.440.3 as minimum Jenkins version

10c26a8

extract JS in sidepanel.jelly - use updated <l:task>

34437e5

shlomomdahan requested a review from a team as a code owner October 21, 2024 19:45

extract JS in sidepanel.jelly - use updated <l:task>

b39c027

yaroslavafenkin reviewed Oct 22, 2024

View reviewed changes

address comments

b606078

address comments

61137eb

basil reviewed Oct 22, 2024

View reviewed changes

shlomomdahan mentioned this pull request Oct 22, 2024

Require 2.440.3 as minimum Jenkins version #114

Closed

6 tasks

shlomomdahan marked this pull request as draft October 22, 2024 20:12

use xhr instead of fetch

d2e4208

shlomomdahan marked this pull request as ready for review October 22, 2024 21:41

basil requested a review from yaroslavafenkin October 22, 2024 22:16

shlomomdahan added 2 commits October 22, 2024 19:39

remove comments

513ab4f

remove comments

1d46553

yaroslavafenkin approved these changes Oct 23, 2024

View reviewed changes

PierreBtz and others added 6 commits October 25, 2024 10:09

Merge branch 'master' into JENKINS-73968

449a69b

use fetch api

1e53003

use fetch api

1c171fa

Merge remote-tracking branch 'origin/JENKINS-73968' into JENKINS-73968

3aee937

use fetch api

44f6c00

use fetch api

cf981c9

yaroslavafenkin reviewed Oct 28, 2024

View reviewed changes

src/main/resources/com/cloudbees/simplediskusage/QuickDiskUsagePlugin/refresh-disk-usage.js Outdated Show resolved Hide resolved

use fetch api - remove explicit header

161b4fd

basil added 2 commits October 29, 2024 11:36

Fix functionality

c33593b

Use notification bar to show proper colors

a08412e

basil reviewed Oct 29, 2024

View reviewed changes

basil approved these changes Oct 29, 2024

View reviewed changes

basil requested a review from PierreBtz October 29, 2024 18:53

PierreBtz approved these changes Nov 6, 2024

View reviewed changes

PierreBtz added the enhancement label Nov 6, 2024

PierreBtz merged commit 713eeed into jenkinsci:master Nov 6, 2024
17 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[JENKINS-73968] Extract event handler in `QuickDiskUsagePlugin/sidepanel.jelly` #113

[JENKINS-73968] Extract event handler in `QuickDiskUsagePlugin/sidepanel.jelly` #113

shlomomdahan commented Oct 21, 2024 •

edited by basil

Loading

basil commented Oct 22, 2024

shlomomdahan commented Oct 22, 2024

basil commented Oct 22, 2024

basil Oct 22, 2024

shlomomdahan Oct 22, 2024

basil Oct 22, 2024

shlomomdahan Oct 22, 2024

basil Oct 22, 2024

shlomomdahan Oct 22, 2024

shlomomdahan commented Oct 22, 2024

basil commented Oct 22, 2024

shlomomdahan commented Oct 22, 2024

yaroslavafenkin left a comment

yaroslavafenkin Oct 23, 2024

PierreBtz Oct 25, 2024

shlomomdahan Oct 27, 2024

basil Oct 29, 2024

PierreBtz Nov 6, 2024

basil left a comment

basil commented Nov 4, 2024

PierreBtz left a comment

		@@ -0,0 +1,13 @@
		function refreshDiskUsage(a) {

		var xhr = new XMLHttpRequest();

[JENKINS-73968] Extract event handler in QuickDiskUsagePlugin/sidepanel.jelly #113

[JENKINS-73968] Extract event handler in QuickDiskUsagePlugin/sidepanel.jelly #113

Conversation

shlomomdahan commented Oct 21, 2024 • edited by basil Loading

Testing done

Submitter checklist

basil commented Oct 22, 2024

shlomomdahan commented Oct 22, 2024

basil commented Oct 22, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shlomomdahan commented Oct 22, 2024

basil commented Oct 22, 2024

shlomomdahan commented Oct 22, 2024

yaroslavafenkin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

basil left a comment

Choose a reason for hiding this comment

basil commented Nov 4, 2024

PierreBtz left a comment

Choose a reason for hiding this comment

[JENKINS-73968] Extract event handler in `QuickDiskUsagePlugin/sidepanel.jelly` #113

[JENKINS-73968] Extract event handler in `QuickDiskUsagePlugin/sidepanel.jelly` #113

shlomomdahan commented Oct 21, 2024 •

edited by basil

Loading