Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 17 commits into from
Nov 6, 2024
Merged
Changes from 8 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion pom.xml
Original file line number Diff line number Diff line change
@@ -44,7 +44,7 @@
<properties>
<changelist>999999-SNAPSHOT</changelist>
<gitHubRepo>jenkinsci/cloudbees-disk-usage-simple-plugin</gitHubRepo>
<jenkins.version>2.361.4</jenkins.version>
<jenkins.version>2.452.4</jenkins.version>
</properties>

<licenses>
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
function refreshDiskUsage(a) {

var xhr = new XMLHttpRequest();

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

xhr.open("POST", "refresh", true);
xhr.setRequestHeader("Content-Type", "application/x-www-form-urlencoded");

var headers = crumb.wrap({});
xhr.setRequestHeader("Jenkins-Crumb", headers['Jenkins-Crumb']);

xhr.send();

hoverNotification('Refresh scheduled', a.parentNode);
}
Original file line number Diff line number Diff line change
@@ -27,25 +27,16 @@
Side panel for the build view.
-->
<?jelly escape-by-default='true'?>
<j:jelly xmlns:j="jelly:core" xmlns:l="/lib/layout">
<j:jelly xmlns:j="jelly:core" xmlns:l="/lib/layout" xmlns:st="jelly:stapler">
<l:header />
<l:side-panel>
<l:tasks>
<l:task href="${rootURL}/" icon="icon-up icon-md" title="${%Back to Dashboard}"/>
<l:task href="${rootURL}/manage" icon="icon-setting icon-md" title="${%Manage Jenkins}"/>
<l:task href="." icon="/plugin/cloudbees-disk-usage-simple/images/disk.png" title="${%Disk usage}"/>
<j:if test="${not it.running}">
<l:task href="." onclick="return refresh(this)" icon="icon-refresh icon-md" title="${%Refresh disk usage}" post="true"/>
<script>
function refresh(a) {
fetch("refresh", {
method: "post",
headers: crumb.wrap({}),
});
hoverNotification('${%Refresh scheduled}',a.parentNode);
return true;
}
</script>
<st:adjunct includes="com.cloudbees.simplediskusage.QuickDiskUsagePlugin.refresh-disk-usage"/>
<l:task href="." data-callback="refreshDiskUsage" icon="icon-refresh icon-md" title="${%Refresh disk usage}" post="true"/>
</j:if>
</l:tasks>
</l:side-panel>