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

Enhance search.js with Error Handling and Input Validation #10179

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

mahdiatubly
Copy link

Error Resilience:
Added error handling to the fetch call to ensure the application gracefully handles failed network requests or server errors.
Provided validation for searchTerm to prevent potential runtime issues caused by invalid inputs.

Robustness:
Ensured the data-search-url attribute is present before initiating the request, preventing unexpected crashes.

Modern JavaScript Practices:
Updated the code for readability and maintainability using optional chaining, descriptive error messages, and streamlined error propagation.

Backward Compatibility:
The changes preserve the current behavior while improving error reporting and debugging for developers.

Alignment with Contribution Goals:
The enhancements aim to improve the user experience and maintain a high-quality codebase, aligning with the Jenkins open-source community's standards.

Copy link

welcome bot commented Jan 20, 2025

Yay, your first pull request towards Jenkins core was created successfully! Thank you so much!

A contributor will provide feedback soon. Meanwhile, you can join the chats and community forums to connect with other Jenkins users, developers, and maintainers.

@janfaracik
Copy link
Contributor

Hey, thanks for this and welcome to the community!

This function is only used internally in Jenkins (and only in one place) so I don't think we really benefit from the added error handling, e.g. there shouldn't be a reason as to why buttonElement?.dataset?.searchUrl is null/undefined. As for the fetch() error catching, we could probably handle this more gracefully than we do (e.g. we don't currently) but I think it'd be such a rare use case it's not worth it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants