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

headerCommandPaletteButton undefined when JenkinsHeader not loaded #10089

Merged
merged 1 commit into from
Dec 22, 2024

Conversation

jglick
Copy link
Member

@jglick jglick commented Dec 20, 2024

Automated test suites (PCT + ATH) run by CloudBees CI against recent trunk code turned up numerous cryptic JavaScript failures; for example the button in a credentials dropdown to open a dialog to create new credentials would just report an error about missing window.dialog and do nothing, and so all acceptance tests using credentials failed. I tracked these down to an initial error

Uncaught TypeError: headerCommandPaletteButton is null
    command_palette_init index.js:40
    7807 app.js:11

In most cases this was because CloudBees CI overrides JenkinsHeader (#5909) and the replacement does not currently include the same search box functionality as in Jenkins. #7569 however included initialization code presuming that (for example) button-open-command-palette existed, and did not gracefully skip execution if it was absent, such as if a custom header were in use.

In another case I found, a page was using <l:layout type="full-screen" …> (#2445) which also omits this search box (but otherwise loads stock JavaScript). In fact Jenkins OSS does this in the setup wizard, and you can see this error by running that. It is mostly harmless in that context: stock JS fails to initialize, but was not being used anyway, and so the only apparent symptom is a warning in the browser log. An HtmlUnit-based test caught this.

Testing done

New functional test here, and also seems to correct automated test failures in CloudBees CI.

Proposed changelog entries

  • Since 2.489, JavaScript errors could be seen in some Jenkins pages omitting the usual header bar, such as the setup wizard.

Proposed upgrade guidelines

N/A

Desired reviewers

@janfaracik

Before the changes are marked as ready-for-merge:

Maintainer checklist

Preview Give feedback

@jglick jglick requested a review from janfaracik December 20, 2024 19:03
@@ -53,4 +53,9 @@ public class LayoutTest {
}
}

@Test public void fullScreen() throws Exception {
// Example page using <l:layout type="full-screen">:
r.createWebClient().goTo("setupWizard/proxy-configuration");
Copy link
Member Author

Choose a reason for hiding this comment

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

Without fix:

TypeError: Cannot call method "addEventListener" of null (http://…/jenkins/static/…/jsbundles/app.js#1069)

@timja timja added the bug For changelog: Minor bug. Will be listed after features label Dec 20, 2024
Copy link
Contributor

@janfaracik janfaracik left a comment

Choose a reason for hiding this comment

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

Thanks for this :)

@timja
Copy link
Member

timja commented Dec 21, 2024

/label ready-for-merge


This PR is now ready for merge, after ~24 hours, we will merge it if there's no negative feedback.

Thanks!

@comment-ops-bot comment-ops-bot bot added the ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback label Dec 21, 2024
@krisstern krisstern merged commit f643db8 into jenkinsci:master Dec 22, 2024
15 checks passed
@jglick jglick deleted the headerCommandPaletteButton branch December 23, 2024 14:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug For changelog: Minor bug. Will be listed after features ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants