-
-
Notifications
You must be signed in to change notification settings - Fork 4.7k
fix(explore): cross events date selector allow 7d anytime within 30 days #116099
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
Changes from 2 commits
7ccab6e
321ad50
7d985cd
7780994
978d334
b42a6fc
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -561,6 +561,105 @@ describe('PageFiltersContainer', () => { | |
| }); | ||
| }); | ||
|
|
||
| describe('maxDateRange param', () => { | ||
| it('resets period when maxDateRange appears and current selection exceeds it', async () => { | ||
| const {rerender} = render(<PageFiltersContainer maxPickableDays={30} />, { | ||
| organization, | ||
| initialRouterConfig: { | ||
| location: { | ||
| pathname: '/organizations/org-slug/test/', | ||
| query: {statsPeriod: '14d'}, | ||
| }, | ||
| route: '/organizations/:orgId/test/', | ||
| }, | ||
| }); | ||
|
|
||
| await waitFor(() => | ||
| expect(PageFiltersStore.getState().selection.datetime).toEqual({ | ||
| period: '14d', | ||
| utc: null, | ||
| start: null, | ||
| end: null, | ||
| }) | ||
| ); | ||
|
|
||
| rerender(<PageFiltersContainer maxPickableDays={30} maxDateRange={7} />); | ||
|
|
||
| await waitFor(() => | ||
| expect(PageFiltersStore.getState().selection.datetime).toEqual({ | ||
| period: '7d', | ||
| utc: null, | ||
| start: null, | ||
| end: null, | ||
| }) | ||
| ); | ||
| }); | ||
|
|
||
| it('does not reset period when maxDateRange appears but selection is within it', async () => { | ||
| const {rerender} = render(<PageFiltersContainer maxPickableDays={30} />, { | ||
| organization, | ||
| initialRouterConfig: { | ||
| location: { | ||
| pathname: '/organizations/org-slug/test/', | ||
| query: {statsPeriod: '7d'}, | ||
| }, | ||
| route: '/organizations/:orgId/test/', | ||
| }, | ||
| }); | ||
|
|
||
| await waitFor(() => | ||
| expect(PageFiltersStore.getState().selection.datetime).toEqual({ | ||
| period: '7d', | ||
| utc: null, | ||
| start: null, | ||
| end: null, | ||
| }) | ||
| ); | ||
|
|
||
| rerender(<PageFiltersContainer maxPickableDays={30} maxDateRange={14} />); | ||
|
|
||
| await waitFor(() => | ||
| expect(PageFiltersStore.getState().selection.datetime).toEqual({ | ||
| period: '7d', | ||
| utc: null, | ||
| start: null, | ||
| end: null, | ||
| }) | ||
| ); | ||
| }); | ||
|
|
||
| it('resets absolute range when maxDateRange appears and range exceeds it', async () => { | ||
| const start = moment().subtract(10, 'days').format('YYYY-MM-DDTHH:mm:ss'); | ||
| const end = moment().subtract(1, 'days').format('YYYY-MM-DDTHH:mm:ss'); | ||
|
|
||
| const {rerender} = render(<PageFiltersContainer maxPickableDays={30} />, { | ||
| organization, | ||
| initialRouterConfig: { | ||
| location: { | ||
| pathname: '/organizations/org-slug/test/', | ||
| query: {start, end}, | ||
| }, | ||
| route: '/organizations/:orgId/test/', | ||
| }, | ||
| }); | ||
|
|
||
| await waitFor(() => | ||
| expect(PageFiltersStore.getState().selection.datetime.period).toBeNull() | ||
| ); | ||
|
|
||
| rerender(<PageFiltersContainer maxPickableDays={30} maxDateRange={7} />); | ||
|
|
||
| await waitFor(() => | ||
| expect(PageFiltersStore.getState().selection.datetime).toEqual({ | ||
| period: '7d', | ||
| utc: null, | ||
| start: null, | ||
| end: null, | ||
| }) | ||
| ); | ||
| }); | ||
| }); | ||
|
|
||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should a test be added in checking that absolute date ranges don't get reset as well, just a small regression test 👀 |
||
| describe('skipInitializeUrlParams', () => { | ||
| const skipInitProjects = [ | ||
| ProjectFixture({id: '1', slug: 'staging-project', environments: ['staging']}), | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -58,6 +58,7 @@ export function PageFiltersContainer({ | |
| skipLoadLastUsed, | ||
| skipLoadLastUsedEnvironment, | ||
| maxPickableDays, | ||
| maxDateRange, | ||
| children, | ||
| ...props | ||
| }: Props) { | ||
|
|
@@ -103,6 +104,7 @@ export function PageFiltersContainer({ | |
| skipLoadLastUsed, | ||
| skipLoadLastUsedEnvironment, | ||
| maxPickableDays, | ||
| maxDateRange, | ||
| memberProjects, | ||
| nonMemberProjects, | ||
| defaultSelection, | ||
|
|
@@ -132,20 +134,24 @@ export function PageFiltersContainer({ | |
| // eslint-disable-next-line react-hooks/exhaustive-deps | ||
| }, [projectsLoaded]); | ||
|
|
||
| // Handle dynamic maxPickableDays changes (e.g., switching between pages with different limits). | ||
| // Handle dynamic maxPickableDays/maxDateRange changes (e.g., switching between pages with different limits). | ||
| // When the limit decreases and the current selection exceeds it, reset to the new max. | ||
| const previousMaxPickableDays = usePrevious(maxPickableDays); | ||
| const previousMaxDateRange = usePrevious(maxDateRange); | ||
| const shouldResetDateTime = useMemo(() => { | ||
| // Don't act until page filters are initialized - selection.datetime contains | ||
| // default values until isReady, not the actual URL state | ||
| if (!isReady) { | ||
| return false; | ||
| } | ||
|
|
||
| // Only act when maxPickableDays decreases (increasing the limit never invalidates selection) | ||
| const effectiveMaxDays = maxDateRange ?? maxPickableDays; | ||
| const previousEffectiveMaxDays = previousMaxDateRange ?? previousMaxPickableDays; | ||
|
|
||
| // Only act when the effective limit decreases (increasing the limit never invalidates selection) | ||
| if ( | ||
| previousMaxPickableDays === maxPickableDays || | ||
| previousMaxPickableDays < maxPickableDays | ||
| previousEffectiveMaxDays === effectiveMaxDays || | ||
| previousEffectiveMaxDays < effectiveMaxDays | ||
| ) { | ||
| return false; | ||
| } | ||
|
|
@@ -154,19 +160,39 @@ export function PageFiltersContainer({ | |
|
|
||
| // For relative periods (e.g., "14d"), check if the period exceeds the new max | ||
| if (period) { | ||
| return statsPeriodToDays(period) > maxPickableDays; | ||
|
nikkikapadia marked this conversation as resolved.
|
||
| return statsPeriodToDays(period) > effectiveMaxDays; | ||
| } | ||
|
|
||
| // For absolute date ranges, check if the start date is before the allowed window. | ||
| // Uses same calculation as initialization in pageFilters.tsx | ||
| if (start && end) { | ||
| const periodStart = new Date(start); | ||
| const periodEnd = new Date(end); | ||
| const maxPeriod = parseStatsPeriod(`${maxPickableDays}d`); | ||
| const maxStart = new Date(maxPeriod.start); | ||
| return new Date(start).getTime() < maxStart.getTime(); | ||
|
|
||
| if (maxDateRange) { | ||
| const maxTimeRange = maxDateRange * 24 * 60 * 60 * 1000; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we have a shared const somewhere in the FE for one day? Wondering if it's worth pulling that out just to be consistent and clearer what we're doing rather than having the raw numbers 🤔 |
||
| return ( | ||
| periodEnd.getTime() - periodStart.getTime() > maxTimeRange || | ||
| periodStart.getTime() < maxStart.getTime() | ||
| ); | ||
| } | ||
|
|
||
| return periodStart.getTime() < maxStart.getTime(); | ||
| } | ||
|
|
||
| return false; | ||
| }, [isReady, maxPickableDays, previousMaxPickableDays, selection.datetime]); | ||
| }, [ | ||
| isReady, | ||
| maxDateRange, | ||
| maxPickableDays, | ||
| previousMaxDateRange, | ||
| previousMaxPickableDays, | ||
| selection.datetime, | ||
| ]); | ||
|
|
||
| const resetPeriodDays = maxDateRange ?? maxPickableDays; | ||
|
|
||
| useLayoutEffect(() => { | ||
| if (!shouldResetDateTime) { | ||
|
sentry[bot] marked this conversation as resolved.
|
||
|
|
@@ -175,15 +201,15 @@ export function PageFiltersContainer({ | |
|
|
||
| // Reset to a relative period matching the new max (clears any absolute dates) | ||
| const newDateState = getDatetimeFromState({ | ||
| period: `${maxPickableDays}d`, | ||
| period: `${resetPeriodDays}d`, | ||
| start: null, | ||
| end: null, | ||
| utc: selection.datetime.utc, | ||
| environment: [], | ||
| project: [], | ||
| }); | ||
| updateDateTime(newDateState, location, navigate); | ||
| }, [maxPickableDays, location, navigate, selection.datetime.utc, shouldResetDateTime]); | ||
| }, [location, navigate, resetPeriodDays, selection.datetime.utc, shouldResetDateTime]); | ||
|
|
||
| // Update store persistence when `disablePersistence` changes | ||
| useEffect(() => updatePersistence(!disablePersistence), [disablePersistence]); | ||
|
|
||
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.
nit: Instead of a nested ternary like this, could we use an
ifblock, imo it's a tad easier to read