-
Notifications
You must be signed in to change notification settings - Fork 156
👷♀️ [Devx] Make developer extension queries compatible with exclusion #3434
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
base: main
Are you sure you want to change the base?
Conversation
developer-extension/src/panel/components/tabs/eventsTab/facetList.tsx
Outdated
Show resolved
Hide resolved
developer-extension/src/panel/components/tabs/eventsTab/facetList.tsx
Outdated
Show resolved
Hide resolved
// Handle minus sign prefix for exclude mode | ||
const isExclude = queryPart.startsWith('-') | ||
const part = isExclude ? queryPart.slice(1) : queryPart | ||
const [key, ...valueParts] = part.split(':') | ||
const value = valueParts.join(':') // Rejoin in case there are colons in the value | ||
return [isExclude ? 'exclude' : 'include', key, value] |
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.
💬 suggestion: introduce a proper TypeScript type for this, example:
interface QueryPart {
key: string,
value: string,
type: "exclude" | "include"
}
💬 suggestion: unescape the key and values in parseQuery
, instead of doing it later.
const escapedPath = facetPath.replace(/\s+/g, '\\$&') | ||
const escapedValue = value.replace(/\s+/g, '\\$&') |
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.
💬 suggestion: expose standalone functions to escape and unescape strings, so you can test them correctly and reuse them.
const includeParts = queryParts.filter((part) => part[0] === 'include') | ||
const excludeParts = queryParts.filter((part) => part[0] === 'exclude') | ||
|
||
// Check if event matches any exclude condition | ||
const isExcluded = excludeParts.some(([_, searchKey, searchTerm]) => { | ||
const restoredSearchTerm = searchTerm ? searchTerm.replaceAll(/\\\s+/gm, ' ') : '' | ||
return matchQueryPart(event, searchKey, restoredSearchTerm) | ||
}) | ||
|
||
if (isExcluded) { | ||
return false | ||
} | ||
|
||
// If no include conditions, event passes | ||
if (includeParts.length === 0) { | ||
return true | ||
} | ||
// Check if event matches any include condition | ||
return includeParts.some(([_, searchKey, searchTerm]) => { | ||
const restoredSearchTerm = searchTerm ? searchTerm.replaceAll(/\\\s+/gm, ' ') : '' | ||
return matchQueryPart(event, searchKey, restoredSearchTerm) | ||
}) |
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.
💬 suggestion: I think this could be simplified by iterating over the query parts only once:
queryParts.every((queryPart) => {
const match = matchQueryPart(...)
const isExcluded = queryPart[0] === "exclude"
return match !== isExcluded
})
Or matchQueryPart(event, queryPart)
could take care of the "excluded" status, and we could just do:
queryParts.every((queryPart) => matchQueryPart(event, queryPart))
At least, extract that logic in a filterQuery
function similar to filterFacets
and filterOutdatedVersions
facetValuesFilter: newFacetValuesFilter, | ||
query: generateQueryFromFacetValues(newFacetValuesFilter), |
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.
💬 suggestion: query
and facetValuesFilter
now kind of represent the same thing but in two different formats. It would be nice to keep only one of the two, probably query
as it can express more things. This would:
- simplify the logic, as we don't need to sync the two
- make the UI more predictable, as the two cannot be out of sync
- make things faster, as we wouldn't need to filter events based on both the query and facets.
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.
Agreed, these two formats coexisting is kind of getting out of hands.
…ist.tsx Co-authored-by: Benoît <[email protected]>
…ist.tsx Co-authored-by: Benoît <[email protected]>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3434 +/- ##
==========================================
+ Coverage 92.73% 93.19% +0.46%
==========================================
Files 301 302 +1
Lines 7900 7941 +41
Branches 1807 1819 +12
==========================================
+ Hits 7326 7401 +75
+ Misses 574 540 -34 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Bundles Sizes Evolution
🚀 CPU Performance
🧠 Memory Performance
|
Motivation
Current filter queries do not support exclusion, despite that we support this mode in the filters. It would also be nice to update the query while selecting with facets.
Changes
Testing
I have gone over the contributing documentation.