-
-
Notifications
You must be signed in to change notification settings - Fork 843
fix(toolkit): prevent "?" from being appended to URL when no filters are active #8180
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
fix(toolkit): prevent "?" from being appended to URL when no filters are active #8180
Conversation
|
Want to review this pull request? Take a look at this documentation for a step by step guide! From your project repository, check out a new branch and test the changes. |
rogerioduenas
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @eunicode!
Nice! Great job with this pull request!
- The branching was done correctly.
- Issue number was listed.
- The PR title is descriptive of the changes.
- The bug was fixed correctly in the code.
- Relevant images were included to show visual changes.
- The PR request clearly states what was updated.
- The new implementation replaces manual query string construction with native APIs (
URLandURLSearchParams), making the code cleaner and more readable. Parameters are now handled automatically, reducing the risk of subtle bugs caused by manual concatenation or formatting. I tested it many times and everything seems to be working correctly. I couldn't find any issues.
Again, great work and thanks for taking the time to contribute to the website! This was the most extensive PR I've reviewed here, but the way you explained everything and structured the code made it very easy to understand.
|
Availability: Varying weekdays, 7-9PM |
|
Availability: Monday-Friday 9AM - 5PM |
katiejnete
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @eunicode , thank you for taking on this issue.
Things you did well:
PR is done with the correct branch.
PR contains correctly formatted linked issue.
CodeQL Alerts have been properly checked.
Before and after screenshots are appropriately included.
PR follows correct format and is properly written.
The changes are applicable to the issue.
Website is still user-friendly and links and components still work as intended.
Source code changes are applicable and clean.
Suggested changes:
None
Thank you for your hard work, I approve this PR.
aadilahmed
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good job @eunicode ! You've correctly refactored the toolkit page and removed the trailing '?' character from the URL when no filters are applied. Great job on completing this issue and writing an extremely detailed pull request.
Fixes #7636
What changes did you make?
This PR fixes an issue where a
?was appended to the URL even when no query parameters were present.The bug stemmed from
applyFiltersToURL(), wherereplaceState()was always invoked with a hardcoded?${filters}string—even when no filters were active—resulting in a stray?in the URL. The function has been refactored to use the URL and URLSearchParams APIs, eliminating the need to manually construct the query string and reducing the risk of similar errors.Why did you make the changes (we will use this info to test)?
The changes were made to fix the bug and to make the code more maintainable.
Previous Implementation
The previous implementation constructed the query string manually by iterating over the filterParams object1.
The logic:
key=value1,value2&from the final stringwindow.history.replaceState(null, '',?${filters})The Problem
When no filters are applied, the query string is empty, yet the code still appends a
?to the URL, resulting in the URL beinghackforla.org/toolkit/?. This occurs becausewindow.history.replaceState(null, '',?${filters})is executed unconditionally—even when filters is an empty string. SinceapplyFiltersToURL()is invoked on both DOMContentLoaded and on every filter checkbox or tag interaction, this leads to?being appended unnecessarily on initial page load and when clearing filters.Current Implementation
The updated version uses the URL and URLSearchParams Web APIs for safer and more standardized URL manipulation:
window.locationfilterParamsurl.search = params.toString()window.history.replaceState(null, '', url)This ensures that if no filters are active,
params.toString()returns an empty string and the query portion is omitted entirely—removing the dangling?.Testing
I have tested that
1 The filterParams object is initialized on the DOMContentLoaded event and updated in response to checkbox interactions and page loads containing filters in the URL.
CodeQL Alerts
After the PR has been submitted and the resulting GitHub actions/checks have been completed, developers should check the PR for CodeQL alert annotations.
Check the PR's comments. If present on your PR, the CodeQL alert looks similar as shown
Please let us know that you have checked for CodeQL alerts. Please do not dismiss alerts.
Instructions for resolving CodeQL alerts
If CodeQL alert/annotations appear, refer to How to Resolve CodeQL alerts.
In general, CodeQL alerts should be resolved prior to PR reviews and merging
Screenshots of Proposed Changes To The Website (if any, please do not include screenshots of code changes)
Visuals before changes are applied
Visuals after changes are applied