[refactor] The observability filter engine moves into @agenta/observability - #5956
[refactor] The observability filter engine moves into @agenta/observability#5956ardaerzin wants to merge 1 commit into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
dosubot
Bot
added
size:XL
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe PR centralizes filter types, operators, codecs, adapters, column helpers, and row reconciliation in ChangesFilter API centralization
Estimated code review effort: 3 (Moderate) | ~25 minutes Mergeability Score: 🔵 Low · up to Multi-value trace-type filters can cause the dialog to reconcile rows incorrectly and show the wrong filter state. The change is otherwise bounded, but this edge case should be fixed and the required frontend formatting check completed before merge. Sequence Diagram(s)sequenceDiagram
participant FilterUI
participant ObservabilityFilters
participant FieldConfigMap
FilterUI->>ObservabilityFilters: submit filter rows and trace_type
ObservabilityFilters->>FieldConfigMap: resolve evaluator or application field
FieldConfigMap-->>ObservabilityFilters: matching field metadata
ObservabilityFilters-->>FilterUI: reconciled filter rows
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
web/oss/src/components/Filters/types.d.ts (1)
15-16: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReduce this comment to one short line.
Lines 15-16 add a two-line comment for a direct type re-export. Replace it with one short line or remove it.
Proposed change
-// The filter-menu model belongs to the observability filter engine; re-exported -// here (type-only, so nothing lands in the bundle) for the app's existing callers. +// Re-export observability filter types for existing callers.As per coding guidelines, “Keep in-code comments to at most one short line; use longer comments only for genuinely surprising constraints such as bugs, races, or ordering requirements.”
Source: Coding guidelines
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 4b4bf8c8-bd47-41f5-b4a4-8167abfbb607
📒 Files selected for processing (22)
web/oss/src/components/Filters/Filters.tsxweb/oss/src/components/Filters/helpers/utils.tsweb/oss/src/components/Filters/types.d.tsweb/oss/src/components/pages/evaluations/onlineEvaluation/EmptyStateOnlineEvaluation/EmptyStateOnlineEvaluation.tsxweb/oss/src/components/pages/evaluations/onlineEvaluation/OnlineEvaluationDrawer.tsxweb/oss/src/components/pages/evaluations/onlineEvaluation/assets/helpers.tsweb/oss/src/components/pages/evaluations/onlineEvaluation/components/FiltersPreview.tsxweb/oss/src/components/pages/observability/assets/filterColumnIcons.tsweb/oss/src/components/pages/observability/assets/utils.tsweb/packages/agenta-observability/src/etl/exportUtils.tsweb/packages/agenta-observability/src/filters/attributeKeyOptions.tsweb/packages/agenta-observability/src/filters/fieldAdapter.tsweb/packages/agenta-observability/src/filters/getFilterColumns.tsweb/packages/agenta-observability/src/filters/index.tsweb/packages/agenta-observability/src/filters/operatorRegistry.tsweb/packages/agenta-observability/src/filters/operatorSets.tsweb/packages/agenta-observability/src/filters/reconcileFilterRows.tsweb/packages/agenta-observability/src/filters/referenceUtils.tsweb/packages/agenta-observability/src/filters/rulesEngine.tsweb/packages/agenta-observability/src/filters/types.tsweb/packages/agenta-observability/src/filters/valueCodec.tsweb/packages/agenta-observability/tests/unit/reconcileFilterRows.test.ts
💤 Files with no reviewable changes (1)
- web/oss/src/components/pages/observability/assets/utils.ts
| const rawValue = Array.isArray(tt?.value) ? tt?.value[0] : tt?.value | ||
| const isAffirm = op === "is" || op === "in" | ||
| const isNeg = op === "is_not" || op === "not_in" | ||
| const normalize = (x: unknown): "annotation" | "invocation" | null => | ||
| x === "annotation" ? "annotation" : x === "invocation" ? "invocation" : null | ||
| const flip = (x: unknown): "annotation" | "invocation" | null => | ||
| x === "annotation" ? "invocation" : x === "invocation" ? "annotation" : null | ||
| let effective: "annotation" | "invocation" | null = null | ||
| if (tt && isAffirm) effective = normalize(rawValue) | ||
| else if (tt && isNeg) effective = flip(rawValue) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Do not infer a trace type from a multi-value condition.
Line 31 selects only the first value. trace_type in ["annotation", "invocation"] matches both trace types. trace_type not_in ["annotation", "invocation"] matches neither trace type. In both cases, effective is undetermined.
Only reconcile when the condition contains exactly one value. Otherwise, return the existing rows unchanged. Add tests for both multi-value cases.
Proposed fix
- const rawValue = Array.isArray(tt?.value) ? tt?.value[0] : tt?.value
+ const rawValues = Array.isArray(tt?.value)
+ ? tt.value
+ : tt?.value === undefined
+ ? []
+ : [tt.value]
+ if (rawValues.length !== 1) return rows
+ const rawValue = rawValues[0]📝 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.
| const rawValue = Array.isArray(tt?.value) ? tt?.value[0] : tt?.value | |
| const isAffirm = op === "is" || op === "in" | |
| const isNeg = op === "is_not" || op === "not_in" | |
| const normalize = (x: unknown): "annotation" | "invocation" | null => | |
| x === "annotation" ? "annotation" : x === "invocation" ? "invocation" : null | |
| const flip = (x: unknown): "annotation" | "invocation" | null => | |
| x === "annotation" ? "invocation" : x === "invocation" ? "annotation" : null | |
| let effective: "annotation" | "invocation" | null = null | |
| if (tt && isAffirm) effective = normalize(rawValue) | |
| else if (tt && isNeg) effective = flip(rawValue) | |
| const rawValues = Array.isArray(tt?.value) | |
| ? tt.value | |
| : tt?.value === undefined | |
| ? [] | |
| : [tt.value] | |
| if (rawValues.length !== 1) return rows | |
| const rawValue = rawValues[0] | |
| const isAffirm = op === "is" || op === "in" | |
| const isNeg = op === "is_not" || op === "not_in" | |
| const normalize = (x: unknown): "annotation" | "invocation" | null => | |
| x === "annotation" ? "annotation" : x === "invocation" ? "invocation" : null | |
| const flip = (x: unknown): "annotation" | "invocation" | null => | |
| x === "annotation" ? "invocation" : x === "invocation" ? "annotation" : null | |
| let effective: "annotation" | "invocation" | null = null | |
| if (tt && isAffirm) effective = normalize(rawValue) | |
| else if (tt && isNeg) effective = flip(rawValue) |
1 participant
Context
The filter logic behind the observability filter dialog decided which inputs to show, how to normalise a filter, and how to reconcile rows when
trace_typechanges. All of it sat inside a 1,983-line antd component in the app, so mobile could not filter traces without copying the rules.Third of five stacked PRs. Base is
obs/wp1-observability-state.Changes
The engine moves to
@agenta/observability/filters:planInputs,normalizeFilter,toUIValue,fieldConfigByOptionKey,getFilterColumns,reconcileFilterRows, the attribute-key tree builder and the export utilities. Unit tests come with them.Field-menu icons stay out of the package.
FILTER_COLUMNSis icon-free andgetFilterColumns(attributeKeyOptions, icons)takes an icon map keyed by node label, so desktop can inject Phosphor and mobile can inject Lucide. The app's map lives in the newassets/filterColumnIcons.ts.Two contracts worth knowing before you read the diff:
FilterValueis deliberatelyobject, notRecord<string, unknown>. Interfaces have no implicit index signature, so the stricter form rejects every named filter-value type.reconcileFilterRowspreserves array length and per-index order, because the dialog mutates its rows by index.Tests / notes
@agenta/observabilitybuilds, lints and passes its unit tests, including 10 new ones coveringreconcileFilterRows.