Conversation
** Why are these changes being introduced: Currently there are some parts of the application that throw errors in the console because they aren't meant to be executed. This seems to happen mostly with UI elements that are meant only for GeoData, but are still executed in USE. ** Relevant ticket(s): n/a ** How does this address that need: This slightly refactors the javascript for those elements that are throwing errors - adding a guard clause for some filter elements, and adding using optional chaining for some event listeners. ** Document any side effects to this change: Hopefully none - the console should be somewhat cleaner for USE, and unaffected for GeoData.
Pull Request Test Coverage Report for Build 22643369980Details
💛 - Coveralls |
There was a problem hiding this comment.
Pull request overview
This PR reduces JavaScript console errors by ensuring event listeners are only attached when the related DOM elements exist, particularly on USE pages that don’t render GeoData-only UI.
Changes:
- Use optional chaining when attaching the
#advanced-summaryclick listener to avoid errors when the element is absent. - Add an early-return guard in filter initialization to skip listener registration when filters aren’t rendered.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| app/javascript/search_form.js | Avoids addEventListener on a missing #advanced-summary element by using optional chaining. |
| app/javascript/filters.js | Prevents errors on pages without filters by returning early when #filter-toggle / #filter-container are missing. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| document.getElementById('advanced-summary')?.addEventListener('click', () => { | ||
| togglePanelState(advancedPanel); | ||
| }); | ||
| } else { | ||
| document.getElementById('advanced-summary').addEventListener('click', () => { | ||
| document.getElementById('advanced-summary')?.addEventListener('click', () => { |
There was a problem hiding this comment.
The advanced-summary click handler is identical in both the if and else branches. Consider registering this listener once (outside the conditional) to avoid duplication and reduce the chance of the two branches drifting over time.
| }); | ||
|
|
||
| document.getElementById('advanced-summary').addEventListener('click', () => { | ||
| document.getElementById('advanced-summary')?.addEventListener('click', () => { |
There was a problem hiding this comment.
The condition Array.from(allPanels).includes(geoboxPanel && geodistancePanel) is hard to read and doesn’t clearly express the intent (it relies on && returning the second operand). Consider rewriting it as an explicit presence check (e.g., if (geoboxPanel && geodistancePanel)) or two explicit includes(...) checks so it’s unambiguous.
djanelle-mit
left a comment
djanelle-mit
left a comment
There was a problem hiding this comment.
From what I can see, this works as intended. I no longer see console errorsfrom those scripts on the PR build as I do on stage.
Is there any easy way to test this PR build with GeoData? I think the only missing piece would be if the filter panel interactions still work as expected there.
jazairi
left a comment
jazairi
left a comment
There was a problem hiding this comment.
Thanks for this simple and effective refactor! It seems to be working in GeoData as well.
For what it's worth, I think CoPilot makes some good points. 👀 But, I'd consider them nonblocking, especially since this PR did not introduce those issues.
This is some cleanup to the javascript that I ended up doing while debugging Matomo, and thought we might want to consider the change for real.
The motivation was seeing some errors in the USE console, which this tries to resolve. I'm not sure that the strategy followed makes the most sense - particularly with
search_form.js, because the change on line 71 feels like it might not be necessary. I'm proposing this as-is to get feedback from reviewers. Details of the change are in the commit message, not this PR text.Please note - I requested the code review from Copilot as a test, not a commitment to make it happy. I'd welcome your thoughts.
Developer
Accessibility
New ENV
Approval beyond code review
Additional context needed to review
E.g., if the PR includes updated dependencies and/or data
migration, or how to confirm the feature is working.
Code Reviewer
Code
added technical debt.
Documentation
(not just this pull request message).
Testing