feat(in-app-analytics)/filters-rework #1166
Conversation
siiick
force-pushed
the
feat/in-app-analytics-dates
branch
from
89e4184 to
69cba3b
Compare
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
This is the final PR Bugbot will review for you during this billing cycle
Your free Bugbot reviews will reset on November 23
Details
Your team is on the Bugbot Free tier. On this plan, Bugbot will review limited PRs each billing cycle for each member of your team.
To receive Bugbot reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial.
| operator: filter.operator, | ||
| value: value.map((v) => v.trim()), | ||
| }, | ||
| ]); |
There was a problem hiding this comment.
Bug: Filter Type Mismatch Causes Runtime Errors
The ComparisonFilter<T>.value type now allows T | T[], which breaks NumberFilter's expectation of a single numeric value and can cause runtime errors in NumberValueFilter. Additionally, text filters incorrectly produce a nested array for their values (e.g., [{ operator: 'in', value: ['a', 'b'] }]) instead of a flat array of comparison objects.
Additional Locations (2)
| [descriptors, draftValue], | ||
| ); | ||
|
|
||
| console.log(mainFilters); |
There was a problem hiding this comment.
Bug: Unnecessary Debug Logs in FiltersBar Component
Several debug console.log statements remain in the FiltersBar component. These logs output internal state (value, mainFilters, allDescriptors) on every render, which can affect performance and expose unnecessary details in production.
Additional Locations (2)
| const values = (val as Array<{ operator: string; value: string | string[] }>) | ||
| .flatMap((f) => (Array.isArray(f.value) ? f.value : [f.value])) | ||
| .filter((v) => v != null && String(v).length > 0); | ||
| return values.length ? [{ name, op: 'in', value: values }] : []; |
There was a problem hiding this comment.
Bug: Text Filter Value Handling Error
Text filter values are being incorrectly flattened. When a text filter contains [{ operator: 'in', value: ['a', 'b'] }], the flatMap extracts ['a', 'b'] as the values array, but this assumes the nested array structure from the bug in TextMatchFilter.tsx. If the correct structure [{ operator: 'in', value: 'a' }, { operator: 'in', value: 'b' }] is used, this code will fail because it treats f.value as an array when it should be a single string.
| <Input | ||
| type="number" | ||
| placeholder={filter.placeholder} | ||
| value={Number(localValue.value) || 0} |
There was a problem hiding this comment.
Bug: Input Coercion Prevents Clearing Field
The input value is coerced with Number(localValue.value) || 0, which means if localValue.value is 0, it will be treated as falsy and displayed as 0. However, this creates a bug where users cannot clear the input to enter a new value - the field will always show at least 0. The old code used localValue.value directly which allowed the input to be empty during editing.
| getDecisionOutcomesPerDay: async ( | ||
| args: AnalyticsQuery, | ||
| ): Promise<DecisionOutcomesPerPeriod | null> => { | ||
| const parsed = transformAnalyticsQuery.parse(args); | ||
| if (!parsed.length) throw new Error('No date range provided'); | ||
|
|
||
| try { | ||
| const [raw, rawCompare] = await Promise.all([ | ||
| client.getDecisionOutcomesPerDay(parsed[0]!), | ||
| ...(parsed[1] ? [client.getDecisionOutcomesPerDay(parsed[1])] : []), | ||
| ]); | ||
|
|
||
| const merged = mergeDateRanges([raw, ...(rawCompare ? [rawCompare] : [])]); | ||
|
|
||
| const start = args.compareDateRange | ||
| ? [args.dateRange.start, args.compareDateRange.start].sort(compareAsc)[0]! | ||
| : args.dateRange.start; | ||
| const end = args.compareDateRange | ||
| ? [args.dateRange.end, args.compareDateRange.end].sort(compareDesc)[0]! | ||
| : args.dateRange.end; | ||
| const start = | ||
| parsed.length === 2 | ||
| ? [parsed[0]!.start, parsed[1]!.start].sort(compareAsc)[0]! | ||
| : parsed[0]!.start; | ||
| const end = | ||
| parsed.length === 2 | ||
| ? [parsed[0]!.end, parsed[1]!.end].sort(compareDesc)[0]! | ||
| : parsed[0]!.end; | ||
|
|
||
| const startDate: LimitDate = { | ||
| date: start, | ||
| rangeId: start === args.dateRange.start ? 'base' : 'compare', | ||
| rangeId: start === parsed[0]!.start ? 'base' : 'compare', | ||
| }; | ||
| const endDate: LimitDate = { | ||
| date: end, | ||
| rangeId: end === args.dateRange.end ? 'base' : 'compare', | ||
| rangeId: end === parsed[0]!.end ? 'base' : 'compare', | ||
| }; | ||
|
|
||
| if (!merged.length) { | ||
| merged.push({ | ||
| ...startDate, | ||
| approve: 0, | ||
| block_and_review: 0, | ||
| decline: 0, | ||
| review: 0, | ||
| }); | ||
| merged.push({ | ||
| ...endDate, | ||
| approve: 0, | ||
| block_and_review: 0, | ||
| decline: 0, | ||
| review: 0, | ||
| }); | ||
| } | ||
| const rangeSize = differenceInDays(end, start); | ||
|
|
||
| return adaptDecisionOutcomesPerDay( | ||
| rangeSize === merged.length ? merged : fillMissingDays(merged, startDate, endDate), | ||
| ); | ||
| } catch (error) { | ||
| console.error(error); | ||
| console.error('error in getDecisionOutcomesPerDay', error); | ||
| return null; | ||
| } |
There was a problem hiding this comment.
That's quite complex to understand what's happening with the naming. Do you think you can rename those vars and maybe create an util function (IMO input refining is not the main point of the repository function)? And why is there a need to try/catch all that code?
| try { | ||
| const urlParams = urlParamsSchema.parse(params); | ||
| const queryParams = queryParamsSchema.parse(await request.json()); | ||
| const query = await analytics.getAvailableFilters({ | ||
| scenarioId: urlParams.scenarioId, | ||
| ranges: queryParams.ranges, | ||
| }); | ||
| return Response.json(query); | ||
| } catch (error) { | ||
| console.error('error in available_filters', error); | ||
| return Response.json({ error: 'Invalid request' }, { status: 400 }); | ||
| } |
There was a problem hiding this comment.
No success true or false?
| return Response.json({ | ||
| decisionOutcomesPerDay, | ||
| ruleHitTable, | ||
| }); | ||
| } catch (error) { | ||
| console.error('error in analytics query', error); | ||
| return Response.json({ error: 'Internal server error' }, { status: 500 }); | ||
| } |
There was a problem hiding this comment.
Same here, no success field ?
| const toTextArray = (selected: unknown): string[] => { | ||
| const arr = selected as any[]; | ||
| if (!Array.isArray(arr)) return []; | ||
| return arr.flatMap((item) => { | ||
| if (typeof item === 'string') return item; | ||
| const val = (item as any)?.value; | ||
| return Array.isArray(val) ? val : val != null ? [val] : []; | ||
| }); | ||
| }; |
There was a problem hiding this comment.
Seems like this function could be created outside the component
| const isActive = (name: string) => active.includes(name); | ||
| return { emitSet, emitRemove, emitToggleActive, getValue, isActive }; | ||
| }, [value, active, onChange]); | ||
| console.log(value); |
| ...(additionalFilters.length > 0 ? [['additional', additionalFilters] as const] : []), | ||
| ]); | ||
| }, [mainFilters, additionalFilters]); | ||
| console.log(allDescriptors); |
2 participants
his PR refactors and enhances the filter logic used in the in-app analytics module to make it more robust, flexible, and user friendly. Key changes include UI improvements, logic simplification, and bug fixes.
🛠️ What’s Changed
• Refactor core filter logic to simplify and clarify flow, improving maintainability and reducing complexity.
• Add “Apply” button to explicitly trigger filter execution rather than applying changes immediately on interaction.
• Introduce “loading” + “touched” states to better communicate form interactivity and pending updates.
• Make compareRange optional, allowing filters to operate when comparison ranges are not defined.
• Move filter buttons to the second line for cleaner layout and better visual grouping.
• Support filter “unavailability” (i.e., marking filters as unavailable/inactive) to improve the UX when certain filters don’t apply.
• Fix scenario filter naming, correcting inconsistencies in how scenario filters are labeled.
• Fix select display issue, resolving UI glitches in how dropdown selections were rendered.
• Fix edit modal behavior, improving stability and addressing edge cases when editing existing filters.
• Prevent render warnings, eliminating React warnings triggered during certain render paths.
✅ Benefits & Impact
• Better UX clarity — users now see an explicit “Apply” action, and UI states (loading, touched) show better feedback.
• More robust filtering — optional comparison ranges and unavailable states allow filters to adapt to more data conditions.
• Cleaner, maintainable code — the refactor cleans up branching and makes the filter logic easier to follow and extend.
• Fewer UI glitches — thanks to fixes around select/label rendering and modal handling.