Fix quick filters in the network page #789
Conversation
WalkthroughThis change introduces a new reactive property, Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
src/views/components/location-input.html (1)
58-60: Avoid duplicatingoverrideValueintovalue; link instead
overrideValueis always forwarded verbatim tovalue. Ractive already supports one-way linking:this.link('overrideValue', 'value', { mode: 'oneWay' });Using
linkeliminates the extra state, the observer, and the risk of the two props diverging.src/views/pages/globalping/network.html (1)
637-643: Duplicate conditional branches—extract helper
setFilterRulenow repeats essentially the same branch three times, differing only in the value written. Inlining this many string concatenations makes future maintenance error-prone.Extract a helper:
const writeLocation = (loc) => this.set('overrideLocation', loc);Then call
writeLocation(...)in each branch to keep logic DRY.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/views/components/location-input.html(2 hunks)src/views/pages/globalping/network.html(5 hunks)
🧰 Additional context used
🧠 Learnings (2)
src/views/components/location-input.html (2)
Learnt from: xbpcb
PR: jsdelivr/www.jsdelivr.com#775
File: src/views/components/show-more-list.html:40-48
Timestamp: 2025-06-26T16:13:51.058Z
Learning: In the show-more-list component at src/views/components/show-more-list.html, the list property is static and will not be updated dynamically, so observing list changes in the observer is not necessary.
Learnt from: xbpcb
PR: jsdelivr/www.jsdelivr.com#775
File: src/views/components/show-more-list.html:0-0
Timestamp: 2025-06-30T15:52:04.874Z
Learning: In RactiveJS, observers fire immediately by default when they are set up, unless explicitly configured with `{ init: false }` option. This means that when you call `this.observe('property', callback)`, the callback executes immediately with the current value of the property, ensuring proper initialization of dependent data.
src/views/pages/globalping/network.html (1)
Learnt from: PavelKopecky
PR: jsdelivr/www.jsdelivr.com#775
File: src/views/pages/globalping/network-tools.html:147-156
Timestamp: 2025-07-04T14:51:34.927Z
Learning: In the globalping network-tools.html file, the errorMessage variable displayed with triple mustaches {{{errorMessage}}} is safe from XSS because it's completely generated by internal code, not user input or untrusted external sources.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build (24.x)
🔇 Additional comments (1)
src/views/pages/globalping/network.html (1)
55-56: Conflicting two-way & one-way bindingsPassing both
value="{{location}}"(two-way) andoverrideValue="{{overrideLocation}}"(one-way) to the same prop set means the component can receive two different truths. When the user types, onlylocationchanges; when quick filters fire, onlyoverrideLocationchanges. The observer inside the child tries to reconcile them, but the arrangement is fragile and easy to desynchronise.Consolidate to a single source of truth (either drop
valueor move quick-filter writes tolocation).⛔ Skipped due to learnings
Learnt from: xbpcb PR: jsdelivr/www.jsdelivr.com#775 File: src/views/components/show-more-list.html:40-48 Timestamp: 2025-06-26T16:13:51.058Z Learning: In the show-more-list component at src/views/components/show-more-list.html, the list property is static and will not be updated dynamically, so observing list changes in the observer is not necessary.
| this.observe('overrideValue', (value) => { | ||
| this.set('value', value); | ||
|
|
||
| if (!this.get('rawProbes') || !this.get('exposeProbes')) { return; } | ||
|
|
||
| filterProbesByCurrentParts(value); | ||
| this.set('completedValue', value); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Observer risks needless churn and re-entrancy
- The observer fires immediately on setup, overwriting any
valuethat the component has already normalised. - Each external change bubbles back through the two-way binding, possibly causing an extra (identical)
set('value')and a second render.
Guard and defer the update:
-this.observe('overrideValue', (value) => {
- this.set('value', value);
+this.observe('overrideValue', (value) => {
+ if (value === this.get('value')) { return; } // short-circuit identical values
+ this.set('value', value);
}, { init: false }); // avoid first overwriteThis keeps the DOM stable and prevents double work.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| this.observe('overrideValue', (value) => { | |
| this.set('value', value); | |
| if (!this.get('rawProbes') || !this.get('exposeProbes')) { return; } | |
| filterProbesByCurrentParts(value); | |
| this.set('completedValue', value); | |
| }); | |
| this.observe('overrideValue', (value) => { | |
| if (value === this.get('value')) { return; } // short-circuit identical values | |
| this.set('value', value); | |
| if (!this.get('rawProbes') || !this.get('exposeProbes')) { return; } | |
| filterProbesByCurrentParts(value); | |
| this.set('completedValue', value); | |
| }, { init: false }); |
🤖 Prompt for AI Agents
In src/views/components/location-input.html around lines 629 to 636, the
observer on 'overrideValue' triggers immediately on setup and causes redundant
updates by setting 'value' repeatedly, leading to unnecessary renders. To fix
this, add a guard to check if the new 'overrideValue' differs from the current
'value' before setting it, and defer the update using a microtask or timeout to
avoid re-entrancy and double rendering. This will stabilize the DOM and prevent
redundant work.
| this.set('overrideLocation', updatedParts.join(',')); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
rmFilterRule leaves location stale
Removing a quick filter writes only overrideLocation. Until the child component propagates back, location (used by updateFiltersSet) contains the old value. Update both simultaneously or switch to a single authoritative field.
🤖 Prompt for AI Agents
In src/views/pages/globalping/network.html at lines 659-660, the rmFilterRule
method updates only overrideLocation but leaves location stale, causing
inconsistency in updateFiltersSet. Modify the code to update both
overrideLocation and location simultaneously with the new value, or refactor to
use a single authoritative field for location data to ensure consistency.
| this.set('overrideLocation', location.replace(/%/g, '+')); | ||
| this.set('locationCompletedValue', location.replace(/%/g, '+')); | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Indirect update relies on child side-effects
filter.set writes only overrideLocation, expecting the component observer to push the value back into location. If the component is not yet rendered (e.g. during SSR or in tests), location never updates and downstream observers miss the change.
Update both directly to avoid hidden coupling:
this.set('overrideLocation', newLoc);
this.set('location', newLoc);Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/views/pages/globalping/network.html around lines 361 to 363, the code
updates only 'overrideLocation' and 'locationCompletedValue' but relies on a
component observer to update 'location', which may not run during SSR or tests,
causing missed updates. To fix this, update 'location' directly alongside
'overrideLocation' and 'locationCompletedValue' by setting 'location' explicitly
with the replaced value to avoid indirect coupling and ensure all observers
receive the change.
MartinKolarik
temporarily deployed
to
gh-750-quick-filters - globalping.io PR #789
Render
Destroyed
jimaek
temporarily deployed
to
gh-750-quick-filters - www.jsdelivr.com PR #789
Render
Destroyed
|
FYI it's still buggy. e.g. the X to remove a filter stops working after a while. Also I think we should not move the order of quick filters. When clicking on a filter it moves to the front and its confusing as the whole list changes. Just enable them in place. |
|
Too complicated and breaks after removing a filter via the X button. Fixed in fd58a44. |
It is a bit annoying, but it's how it worked before, and they'd move even if we kept them in place because of the extra "X" that appears when a filter is active. |
I hadn't considered quick filters when fixing an issue with location input. The only fix I could think of to avoid a regression was to add another 1-way bound variable.
With this the setup is as follows:
overrideLocationpasses value to location-inputlocationreceives value from location-inputAnd these are used as:
locationfor updating url andfiltersSetoverrideLocationfor passing the quick filters changes and initial url variables to location-input