fix: limit the url of analytics pages to a max of 10k characters or 10kb #2383
Conversation
WalkthroughAdds validateSelection hooks to faceted and analytics filters, refactors faceted filter selection to non‑mutative patterns, and implements URL-length and filter-size validation with toast/tooltip feedback and utilities to compute and enforce URL limits. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
Comment |
…96-network-error-when-selecting-multiple-clients
…n on initial load
…ng URL length handling
JivusAyrus requested review from StarpTech, thisisnithin and wilsonrivera as code owners JivusAyrus changed the title ![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
studio/src/components/analytics/data-table.tsx (1)
344-388: Potential stale closure withrouterandtoast.The
validateSelectionfunction capturesrouterandtoastin its closure, but these are not included in theuseMemodependency array. Whilerouterfrom Next.js is typically stable andtoastfrom a hook is usually stable, this pattern can lead to subtle bugs if these references change.Consider using
useCallbackfor the validation logic or ensuring the dependencies are properly tracked.const filtersListWithValidation = useMemo(() => { return filtersList.map((filter) => { return { ...filter, // ALWAYS validate selection to check if NEW addition would exceed the limit validateSelection: (value: string[]) => { // Build the new filter state to check URL length const newSelectedFilters = [...selectedFilters]; const index = newSelectedFilters.findIndex((f) => f.id === filter.id); if (index >= 0) { newSelectedFilters[index] = { id: filter.id, value: value }; } else { newSelectedFilters.push({ id: filter.id, value: value }); } let stringifiedFilters; try { stringifiedFilters = JSON.stringify(newSelectedFilters); } catch { stringifiedFilters = "[]"; } const { exceeded, reason } = checkFilterLimits( router, stringifiedFilters, ); if (exceeded) { toast({ title: "Filter limit reached", description: `${reason}. Please remove some filters before adding new ones.`, }); return false; // Validation failed - prevents onSelect from being called } return true; // Validation passed - allows onSelect to be called }, onSelect: filter.onSelect, // Use original onSelect without wrapping }; }); - // Only filtersList and selectedFilters should trigger recalculation - // eslint-disable-next-line react-hooks/exhaustive-deps - }, [filtersList, selectedFilters]); + }, [filtersList, selectedFilters, router, toast]);studio/src/components/analytics/data-table-faceted-filter.tsx (1)
225-248: Consider adding validation to range filter updates for consistency.Unlike text and list selection paths,
updateRangeFiltersdoesn't invokevalidateSelectionbefore callingonSelect. While range filters produce predictable small data (2 values), adding validation would maintain consistency across all filter types.const updateRangeFilters = ({ rangeValue, }: { rangeValue: { start: number; end: number }; }) => { setRange({ start: rangeValue.start, end: rangeValue.end, }); // Create new filter values without mutating selectedValues const filterValues = [ JSON.stringify({ label: (rangeValue.start * 10 ** 9).toString(), value: (rangeValue.start * 10 ** 9).toString(), operator: 4, }), JSON.stringify({ label: (rangeValue.end * 10 ** 9).toString(), value: (rangeValue.end * 10 ** 9).toString(), operator: 5, }), ]; + + // Validate before applying (for consistency, though range data is small) + if (validateSelection && !validateSelection(filterValues)) { + return; + } + onSelect?.(filterValues); };studio/src/components/analytics/metrics.tsx (2)
377-410: Stale closure risk withrouterandtoastin useMemo.Similar to the data-table implementation, the
validateSelectionfunction capturesrouterandtoastin its closure, but these aren't in the dependency array. While a ref is correctly used forselectedFiltersto avoid stale closures, the same pattern could be applied forrouterif needed.The current implementation should work in practice since Next.js router and toast hook typically return stable references, but consider adding them to the dependency array for correctness.
const filtersListWithValidation = useMemo(() => { return filtersList.map((filter) => { return { ...filter, validateSelection: (value: string[]) => { const currentSelectedFilters = selectedFiltersRef.current; if ( wouldExceedUrlLimit( router, currentSelectedFilters, filter.id, value, ) ) { toast({ title: "Filter limit reached", description: `Maximum URL length of ${MAX_URL_LENGTH.toLocaleString()} characters reached. Please remove some filters before adding new ones.`, }); return false; } return true; }, onSelect: filter.onSelect, }; }); - // Only filtersList should trigger recalculation - // eslint-disable-next-line react-hooks/exhaustive-deps - }, [filtersList]); + }, [filtersList, router, toast]);
311-340: Consider extracting common filter update logic.The
onSelecthandler logic for updating filters (deep copy, find/update, stringify, applyNewParams) is duplicated between this hook and the data-table'sonColumnFiltersChange. While the contexts differ, extracting a utility function could reduce duplication.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
studio/src/components/analytics/data-table-faceted-filter.tsx(12 hunks)studio/src/components/analytics/data-table.tsx(7 hunks)studio/src/components/analytics/filters.tsx(1 hunks)studio/src/components/analytics/metrics.tsx(9 hunks)studio/src/pages/[organizationSlug]/[namespace]/graph/[slug]/analytics/index.tsx(6 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
studio/src/components/analytics/data-table.tsx (3)
studio/src/components/analytics/metrics.tsx (3)
checkFilterLimits(83-105)calculateUrlLength(113-171)MAX_URL_LENGTH(69-69)studio/src/components/analytics/getDataTableFilters.ts (1)
getDataTableFilters(25-52)studio/src/components/analytics/filters.tsx (1)
AnalyticsFilters(27-35)
studio/src/pages/[organizationSlug]/[namespace]/graph/[slug]/analytics/index.tsx (1)
studio/src/components/analytics/metrics.tsx (2)
useMetricsFilters(284-418)LatencyDistributionCard(1105-1217)
⏰ 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). (4)
- GitHub Check: build_test
- GitHub Check: build_push_image
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
🔇 Additional comments (15)
studio/src/components/analytics/filters.tsx (1)
14-14: LGTM!The
validateSelectioncallback is a clean addition to the interface with proper optional typing and clear documentation of the return value semantics.studio/src/pages/[organizationSlug]/[namespace]/graph/[slug]/analytics/index.tsx (3)
122-134: LGTM!The URL limit warning tooltip provides clear, actionable feedback to users. The conditional rendering and destructive icon styling appropriately communicate the constraint.
225-228: LGTM!Minor formatting adjustment for better readability.
48-48: TheAnalyticsViewResultFilterimport is actually used in this file. It appears on line 61 as a type annotation for thefiltersprop in theOverviewToolbarcomponent definition (filters?: AnalyticsViewResultFilter[];). The import is necessary and should be kept.Likely an incorrect or invalid review comment.
studio/src/components/analytics/data-table.tsx (3)
171-199: LGTM with minor observation.The initial URL validation on load is a good safety net for malicious/corrupted URLs. The shallow routing with
router.replacehandles the reset cleanly.
224-268: LGTM!The filter validation logic correctly distinguishes between removal and addition operations, only validating additions. The early return pattern effectively prevents limit-exceeding filters from being applied.
469-483: LGTM!The URL limit warning tooltip provides consistent UX with the analytics index page, showing a clear warning when the limit is reached.
studio/src/components/analytics/data-table-faceted-filter.tsx (3)
189-195: LGTM!The memoized
selectedValuesSet provides efficient O(1) lookups while keepingselectedOptionsas the source of truth for building filter arrays. Good separation of concerns.
268-285: LGTM!Good UX decision to preserve the input value when validation fails, allowing users to see what they attempted to add.
419-438: LGTM!The validation logic correctly gates additions while allowing deselections (removals) to proceed without validation.
studio/src/components/analytics/metrics.tsx (5)
69-77: LGTM!Clean utility implementations. The
BlobAPI is a reliable cross-browser method for calculating UTF-8 byte size, and the constants are well-documented.
83-171: LGTM!The
checkFilterLimitsandcalculateUrlLengthutilities are well-implemented with:
- Proper SSR handling for
window.location.origin- Correct query parameter merging semantics
- URL encoding matching Next.js behavior for arrays
177-215: LGTM!The
wouldExceedUrlLimithelper correctly:
- Creates a deep copy to avoid mutating the original filters
- Short-circuits for removal operations (always allowed)
- Reuses
checkFilterLimitsfor consistent validation
252-279: LGTM!The URL validation effect provides a safety net for manually manipulated or corrupted URLs. The dependency array comment correctly explains the intentional exclusions.
605-605: LGTM!Minor formatting change for props destructuring.
… in metrics component
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
studio/src/components/analytics/metrics.tsx (2)
122-135: Simplify the param merging logic.Lines 128-133 add existing values to
mergedQueryonly to override them immediately in the next loop (lines 138-142). This is confusing and inefficient.Apply this diff to simplify:
// First, copy existing params that aren't being replaced or removed for (const [key, value] of Object.entries(router.query)) { - if (!newParams.hasOwnProperty(key)) { + if (!(key in newParams)) { // Keep existing param if not in newParams if (value !== undefined && value !== null) { mergedQuery[key] = value as string | string[]; } - } else if (newParams[key] !== null) { - // Keep existing param if newParams[key] is not null (will be overridden below) - if (value !== undefined && value !== null) { - mergedQuery[key] = value as string | string[]; - } } // If newParams[key] is null, we skip it (removes the param) }The simplified version skips existing values that will be replaced by new params, making the logic clearer and slightly more efficient.
375-408: Consider memoizingfiltersListfor better performance.The
filtersListWithValidationuseMemo depends onfiltersList, which is recalculated on every render (lines 306-354). This causesfiltersListWithValidationto also recalculate on every render, even when the underlying data hasn't changed.Consider wrapping
filtersListcalculation in a useMemo:const filtersList = useMemo(() => { return (filters ?? []).map((filter) => { return { ...filter, id: filter.columnName, onSelect: (value) => { // ... existing logic ... }, selectedOptions: // ... existing logic ... options: // ... existing logic ... } as AnalyticsFilter; }); }, [filters, selectedFilters, applyNewParams]);This would prevent unnecessary recalculations of
filtersListWithValidationand improve performance, though the current implementation is functionally correct.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
studio/src/components/analytics/metrics.tsx(9 hunks)
⏰ 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). (4)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
- GitHub Check: build_test
- GitHub Check: build_push_image
🔇 Additional comments (7)
studio/src/components/analytics/metrics.tsx (7)
69-70: LGTM! Well-chosen limits.The 10k character URL limit and 10KB filter size provide reasonable safeguards. This conservative limit ensures compatibility across different browsers and prevents performance issues with overly complex filter states.
72-77: LGTM!The Blob API approach correctly calculates UTF-8 byte size and properly handles multi-byte characters. Since this is only called from client-side hooks, there are no SSR concerns.
79-105: LGTM!The function correctly validates both URL length and filter size limits, provides clear user-facing error messages, and returns a well-structured result.
173-215: LGTM! Solid validation logic.The function correctly handles filter removal, uses deep copies to prevent mutation, gracefully handles JSON stringification errors, and leverages the shared
checkFilterLimitsutility for consistent validation.
252-279: LGTM! Proper URL validation on load.The effect correctly validates filters loaded from the URL (e.g., from bookmarks or manual manipulation) and resets them if limits are exceeded. The eslint-disable is appropriate here because
routerandtoastare stable references, and including them in dependencies would not provide value.
371-382: LGTM! Excellent use of ref to avoid stale closures.The ref pattern correctly ensures that
validateSelectionalways accesses the latestselectedFiltersvalue, preventing stale closure issues. This is a well-implemented React pattern.
603-603: Minor cleanup.Formatting change with no functional impact.
… length validation for selected options
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
studio/src/components/analytics/data-table-faceted-filter.tsx (1)
419-434: Minor inconsistency: validation on removal differs between paths.The text input removal (lines 311-319) explicitly skips validation with the comment "Removal doesn't need validation (always allowed)", but this command item handler validates even when removing (when
isSelectedis true and result is non-empty).Since removal always reduces URL size, validation will always pass, making this harmless but inconsistent. Consider aligning the behavior:
- // Validate BEFORE calling onSelect to prevent optimistic UI updates if ( - filterValues.length > 0 && + !isSelected && // Only validate additions, not removals + filterValues.length > 0 && validateSelection && !validateSelection(filterValues) ) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
studio/src/components/analytics/data-table-faceted-filter.tsx(12 hunks)
⏰ 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). (4)
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: build_test
- GitHub Check: build_push_image
🔇 Additional comments (6)
studio/src/components/analytics/data-table-faceted-filter.tsx (6)
40-40: LGTM - Clear validation callback signature.The optional
validateSelectionprop with the inline comment clarifying the return semantics is well-designed.
189-195: LGTM - Good memoization for lookup efficiency.The
useMemocorrectly creates a derived Set for O(1) lookups while keepingselectedOptionsas the source of truth for building new filter arrays. The inline comments clearly document this pattern.
234-247: Range filters bypass validation - verify this is intentional.The
updateRangeFiltersfunction does not invokevalidateSelection, unlike the text input and command item paths. This appears intentional since range filters have a fixed structure (2 values), but for consistency, consider adding validation here as well or documenting why it's excluded.
268-285: LGTM - Correct validation before state update.The validation gate properly prevents
onSelectfrom being called when the limit is exceeded. Preserving the input on failure is good UX, allowing users to see what they tried to add.
466-481: LGTM - Clean conditional rendering for clear button.The simplified structure correctly shows the clear selection button only when there are selections, using the memoized
selectedValuesfor the size check.
493-546: LGTM - Validation prop correctly forwarded.The
validateSelectionprop is properly destructured and forwarded toDataTableFilterCommands.Note: Line 499 creates a non-memoized Set, unlike the memoized version in
DataTableFilterCommands. This is acceptable here since it's only used for size checks and badge display, not for heavy operations.
…clarifying comments in DataTableFilterCommands
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
studio/src/components/analytics/data-table-faceted-filter.tsx (1)
225-248: Range slider updates bypassvalidateSelection— confirm or align with other flowsIn
updateRangeFiltersforCustomOptions.Range, you build the two JSON range entries and callonSelect(filterValues)directly, without going throughvalidateSelection, whereas text and default option flows always validate before committing. IfvalidateSelectionis your only guard for URL length / payload size, range changes currently won’t be checked.If you want full consistency and central enforcement, consider routing range updates through
validateSelectionas well, e.g.:const updateRangeFilters = ({ rangeValue, }: { rangeValue: { start: number; end: number }; }) => { - setRange({ - start: rangeValue.start, - end: rangeValue.end, - }); - // Create new filter values without mutating selectedValues - const filterValues = [ + const nextFilterValues = [ JSON.stringify({ label: (rangeValue.start * 10 ** 9).toString(), value: (rangeValue.start * 10 ** 9).toString(), operator: 4, }), JSON.stringify({ label: (rangeValue.end * 10 ** 9).toString(), value: (rangeValue.end * 10 ** 9).toString(), operator: 5, }), ]; - onSelect?.(filterValues); + if (validateSelection && !validateSelection(nextFilterValues)) { + return; + } + setRange({ + start: rangeValue.start, + end: rangeValue.end, + }); + onSelect?.(nextFilterValues); };This keeps the range UI and filter state aligned while still letting
validateSelectionblock over‑long URLs when necessary. If you intentionally want range updates exempt from validation (because they’re always a fixed, small payload), adding a short comment to that effect here would help future maintainers.Please confirm against your URL-length reproduction case that range-only or “large selection + range change” scenarios can’t still breach the 10k/10kb limit, or adjust
validateSelectionas above.Also applies to: 350-373
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
studio/src/components/analytics/data-table-faceted-filter.tsx(11 hunks)
⏰ 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). (4)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
- GitHub Check: build_test
- GitHub Check: build_push_image
🔇 Additional comments (1)
studio/src/components/analytics/data-table-faceted-filter.tsx (1)
22-22:validateSelectionwiring and non‑mutating selection updates look solidThe new
validateSelection?: (value: string[]) => booleanprop is threaded correctly fromDataTableFacetedFilterdown intoDataTableFilterCommands, and it’s only invoked on additive/toggle flows where a non‑emptyfilterValuesarray is being applied. UsingselectedOptionsas the single source of truth and a memoized, read‑onlySetfor display/dedup (including the text custom input and command list toggles) avoids local mutations and keeps the UI consistent with the parent state. I don’t see correctness issues in these paths; the behavior matches the intended “validate before committing, but allow clears/removals unconditionally” semantics.Please verify via your analytics URL tests that
validateSelectionis indeed pure and only depends on the incomingvalue: string[]plus its closure (e.g., filter id and other active filters), and that it’s not relying on being called for clear/removal flows.Also applies to: 36-45, 181-188, 268-289, 317-323, 425-440, 472-487, 496-504, 549-557
Summary by CodeRabbit
New Features
Bug Fixes
UI
✏️ Tip: You can customize this high-level summary in your review settings.
Checklist