Repository navigation
Conversation
- Add IN (SQL) and =| (PromQL) operators; conditions on the same key are OR'd into one IN (...) or =~"a|b" condition. - Ad hoc filters always broadcast (isBroadcastEnabled: z.literal(true)), and their variable name is a unique identifier only, no longer declared as a $variable. - Warn when another ad hoc filter already applies to the same sources. - Collapse the filter input to three conditions behind a "+N more" toggle.
🦋 Changeset detectedLatest commit: 61e898a The changes in this PR will be included in the next version bump. This PR includes changesets to release 4 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
🟡 Tier 3 — StandardIntroduces new logic, modifies core functionality, or touches areas with non-trivial risk. Why this tier:
Review process: Full human review — logic, architecture, edge cases. Stats
|
Duplicate code analysisNo new duplicate code introduced by this PR. jscpd compared this branch against |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| ); | ||
| })} | ||
| {conditions | ||
| .slice(0, conditions.length - hiddenCount) |
There was a problem hiding this comment.
🔵 minor — New and invalid conditions are hidden while the filter is collapsed
New conditions are appended to the end of the list, and the list is cut at the first three. Once a filter holds three or more conditions, a condition the user adds with "+" or by clicking a tile value is hidden right away; the only sign of it is the "+N more" count going up. An invalid (data-invalid) chip past the third position is hidden the same way, so its "Filter not applied" warning never shows. Fix: expand the list when a condition is added (set isExpanded in the add and onChange paths), or always show invalid chips. At minimum, mark the "+N more" button when a hidden chip is invalid.
Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond
| )} | ||
| <Separator size="xs" /> | ||
| <VariableNameInput control={control} otherFilters={otherFilters} /> | ||
| <VariableNameInput |
There was a problem hiding this comment.
🔵 minor — Ad hoc form still labels the field "Variable name" while its own description says it isn't a variable
VariableNameInput hardcodes label="Variable name" in VariableNameInput.tsx:30, so the ad hoc form shows "Variable name" directly above "Ad hoc filters can't be referenced as variables". Fix: make the label configurable as well as the description (or replace description with a variant/isReferenceable prop that sets both), and pass something like "Filter ID" from AdhocFilterEditForm.
Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond
PR Review
3 finding(s): 🔴 0 critical · 🟠 0 major · 🔵 3 minor 2 posted as inline comment(s) on the changed lines. 1 listed below. Findings outside the changed lines1 minor
Severity is the reviewer's own estimate and is used for ordering, not filtering. No finding blocks merge automatically; a maintainer decides. |
Deep ReviewScope: PR #3658 — ad hoc dashboard filter finishing touches (behind Intent: Treat an ad hoc filter's name as a unique identifier (not a The core logic is well built and thoroughly unit-tested. Correctness verified the chip-slice index alignment, mixed-operator grouping, the ✅ No critical issues found. Recommendations below. 🟡 P2 — recommended
🔵 P3 nitpicks (2)
Reviewers (7): correctness, security, testing, maintainability, project-standards, learnings-researcher, previous-comments. Coverage notes:
Testing gaps:
Residual note (pre-existing/by-design): a click-to-filter No finding blocks merge automatically. A maintainer will expect P0/P1 findings in code this PR changes to be fixed; P2/P3 are your call -- fix or reply. Never fix findings about surrounding code here; reply instead. Do not widen the PR. How to respond |
E2E Test Results✅ All tests passed • 467 passed • 1 skipped • 1362s
Tests ran across 4 shards in parallel. |
| const variableName = isFilterVariableEnabled(filter) | ||
| ? getFilterVariableName(filter) | ||
| : undefined; | ||
| // An ad hoc filter's variable name can't be referenced. |
There was a problem hiding this comment.
🔵 minor — The check for "is this name a referenceable variable" now lives in three places
The rule isFilterVariableEnabled(filter) && !isAdhocFilter(filter) is now written out in getFilterEffect, in DashboardFiltersList.tsx:166, and again as filter.type === 'ADHOC' inside getDashboardVariableFilters (common-utils/src/filters.ts:1290). If another non-referenceable type is added, all three have to be updated by hand. Fix: export one predicate, such as isFilterReferenceableVariable, from common-utils/src/filters.ts and use it in all three places.
Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond
One predicate decides whether a filter's name is a referenceable variable, used by getDashboardVariableFilters, getFilterEffect, and the filters list. Updates the ad hoc filter e2e specs for the removed broadcast checkbox and the list no longer showing an ad hoc filter's name as a variable.
| ); | ||
| })} | ||
| {conditions | ||
| .slice(0, conditions.length - hiddenCount) |
There was a problem hiding this comment.
🔵 minor — New conditions and invalid conditions get hidden behind the "+N more" toggle
When the filter is collapsed and already has 3 or more conditions, a new condition from the add popover or from click-to-filter is appended at the end. The slice hides it at once, so the user only sees the counter go up and never sees the chip they just added. Conditions past the first three that are marked invalid (=| on SQL, IN/LIKE on PromQL) are hidden in the same way, along with their 'Filter not applied' marker. Fix: expand when conditions.length grows, or always render the newest and invalid chips. As a fallback, show the invalid count on the toggle.
Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond
| case '!~': | ||
| return `NOT match(${target}, ${literal})`; | ||
| case 'IN': | ||
| return `${target} IN (${values.map(toLiteral).join(', ')})`; |
There was a problem hiding this comment.
🔵 minor — SQL IN list hand-rolls formatVariableValues(…, 'sqlstring')
The =| branch already reuses formatVariableValues({ values }, 'regex'). The IN branch rebuilds the same quoted, escapeSqlString-escaped, comma-joined list that formatVariableValues produces for the 'sqlstring' format (packages/common-utils/src/variables.ts:96-99). Fix: use ${target} IN (${formatVariableValues({ values }, 'sqlstring')}) and drop the local toLiteral mapping.
Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond
Summary
Finishing touches for ad hoc dashboard filters, which are still behind
NEXT_PUBLIC_ENABLE_ADHOC_FILTERS.$name, but its name still has to be unique because it keys the filter's conditions in thefilters=URL param. The edit form now says so. The filter-bar tooltip and the filters list no longer present it as a variable.getDashboardVariableFiltersskips ad hoc filters, so checkAlerts, MCP validation and the alert chart stop declaring them.isVariableEnabled: truestays, because name-uniqueness validation reads it.isBroadcastEnabled: z.literal(true), the same waySTATIC_LISTandPROMETHEUS_LABELusez.literal(false). The edit form drops the broadcast checkbox and always shows "Applies to sources".IN(SQL) and=|(PromQL) operators. Each condition still holds one key and one value. When rendered, conditions sharing a key are OR'd: SQL renderstoString(key) IN ('a', 'b')and PromQL renderskey=~"a|b", with regex characters escaped. These are two separate stored values, each valid only for its source type. A condition the filter's source type can't express is skipped and its chip is marked invalid.Compatibility: an ad hoc filter already stored with
isBroadcastEnabled: false, or without the field, will no longer parse. The feature is behind a flag, so there is no migration.Screenshots or video
I couldn't capture screenshots from my environment. @aaronknudtson, please add before/after shots of the ad hoc filter edit modal and of a filter input holding more than 3 conditions.
How to reproduce
Starting point:
yarn dev.packages/app/.env.developmentsetsNEXT_PUBLIC_ENABLE_ADHOC_FILTERS=true.On
main:inoption.On this branch:
ServiceName in apiandServiceName in web. Tiles on Logs filter withtoString(ServiceName) IN ('api', 'web'). On a Prometheus ad hoc filter,=|conditions on the same label render aslabel=~"(a|b)".How to test on Vercel preview
N/A. The preview builds without
NEXT_PUBLIC_ENABLE_ADHOC_FILTERS, so ad hoc filters aren't offered there.Preview routes: N/A
Steps:
References