fix: plausible date range filtering #8659
Conversation
WalkthroughAdds date-range filtering to the analytics overlay: new buildPlausibleDateRange utility, DateRangePicker state and UI in AnalyticsOverlaySwitch, GraphQL query support for custom periods using formatted dates, improved error handling, and tests for the utility and component behavior. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant DateRangePicker as DateRangePicker
participant Overlay as AnalyticsOverlaySwitch
participant Formatter as buildPlausibleDateRange
participant GraphQL as GraphQLQuery
User->>DateRangePicker: Select start & end dates
DateRangePicker->>Overlay: Update startDate/endDate
User->>Overlay: Enable analytics (toggle on)
Overlay->>Formatter: buildPlausibleDateRange(startDate,endDate,fallbacks)
Formatter->>Formatter: Validate & format YYYY-MM-DD,YYYY-MM-DD
Formatter-->>Overlay: Return formattedDateRange
Overlay->>GraphQL: Fetch analytics(period='custom', date=formattedDateRange)
GraphQL-->>Overlay: Return data or error
alt Invalid date range
Overlay->>User: Show "Invalid date range" snackbar
else Fetch error
Overlay->>User: Show "Error fetching analytics" snackbar
else Success
Overlay->>User: Render analytics overlay
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 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 |
|
View your CI Pipeline Execution ↗ for commit a5badae
☁️ Nx Cloud last updated this comment at |
github-actions
bot
requested a deployment
to
Preview - journeys-admin
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/journeys-admin/src/components/Editor/Slider/JourneyFlow/AnalyticsOverlaySwitch/AnalyticsOverlaySwitch.tsx (1)
58-70: Error handling logic appears inverted.When
isDateRangeValidistrue(dates are valid), the code shows "Invalid date range" error. This is backwards:
- If
isDateRangeValid === false→ show "Invalid date range"- If
isDateRangeValid === truebut query fails → show "Error fetching analytics"🐛 Proposed fix
onError: (_) => { - if (isDateRangeValid) { + if (!isDateRangeValid) { enqueueSnackbar(t('Invalid date range'), { variant: 'error', preventDuplicate: true }) return } enqueueSnackbar(t('Error fetching analytics'), { variant: 'error', preventDuplicate: true }) }
🤖 Fix all issues with AI agents
In
`@apps/journeys-admin/src/components/Editor/Slider/JourneyFlow/AnalyticsOverlaySwitch/buildPlausibleDateRange/buildPlausibleDateRange.spec.ts`:
- Around line 50-52: The test in buildPlausibleDateRange.spec.ts calls formatISO
on fallbackStart/fallbackEnd which are strings; update the expectation to match
the implementation by either converting those string fixtures to Date objects
before calling formatISO or (simpler) assert the raw string returned by
buildPlausibleDateRange: replace formatISO(fallbackStart/End, ...) with the
plain fallbackStart and fallbackEnd values so the expectation matches the
buildPlausibleDateRange behavior (refer to fallbackStart, fallbackEnd and the
buildPlausibleDateRange test case).
🧹 Nitpick comments (3)
apps/journeys-admin/src/components/Editor/Slider/JourneyFlow/AnalyticsOverlaySwitch/AnalyticsOverlaySwitch.tsx (1)
48-51: Note: Theskipcondition already prevents queries with invalid date ranges.Since the query is skipped when
isDateRangeValid !== true, theonErrorhandler will only fire when the date range is valid. This makes theisDateRangeValidcheck insideonError(lines 59-65) effectively dead code—if the logic is corrected per the previous comment, the "Invalid date range" branch would never execute.Consider simplifying the error handler to only show the general error message, or document why the defensive check is retained.
apps/journeys-admin/src/components/Editor/Slider/JourneyFlow/AnalyticsOverlaySwitch/AnalyticsOverlaySwitch.spec.tsx (2)
3-3: Unused import:userEventis never used.
fireEventis used on line 140 instead. Remove the unused import.♻️ Proposed fix
-import userEvent from '@testing-library/user-event'
19-20: Consider moving imports before the mock setup.While Jest hoists
jest.mockcalls, having regular imports scattered after the mock setup reduces readability. The conventional pattern groups all imports at the top before any mock setup.♻️ Suggested import ordering
import { MockedProvider } from '@apollo/client/testing' import { fireEvent, render, screen, waitFor } from '@testing-library/react' -import userEvent from '@testing-library/user-event' +import { formatISO } from 'date-fns' import { EditorProvider } from '@core/journeys/ui/EditorProvider' import { JourneyProvider } from '@core/journeys/ui/JourneyProvider' import { getJourneyAnalytics } from '@core/journeys/ui/useJourneyAnalyticsQuery/useJourneyAnalyticsQuery.mock' import { GetJourney_journey } from '../../../../../../__generated__/GetJourney' import { earliestStatsCollected } from './AnalyticsOverlaySwitch' import { buildPlausibleDateRange } from './buildPlausibleDateRange' +import { AnalyticsOverlaySwitch } from '.' jest.mock('./buildPlausibleDateRange') const mockBuildPlausibleDateRange = buildPlausibleDateRange as jest.MockedFunction<typeof buildPlausibleDateRange> -import { AnalyticsOverlaySwitch } from '.' -import { formatISO } from 'date-fns' - const mockCurrentDate = '2024-06-02'
| expect(result).toBe( | ||
| `${formatISO(fallbackStart, { representation: 'date' })},${formatISO(fallbackEnd, { representation: 'date' })}` | ||
| ) |
There was a problem hiding this comment.
Test expectation is incorrect — formatISO is called on a string.
fallbackStart is a string ('2024-06-01'), but formatISO expects a Date object. The implementation in buildPlausibleDateRange.ts uses fallbackStartDate directly as a string without formatting.
This test will fail because formatISO(fallbackStart, ...) will throw or produce unexpected output.
🐛 Proposed fix
expect(result).toBe(
- `${formatISO(fallbackStart, { representation: 'date' })},${formatISO(fallbackEnd, { representation: 'date' })}`
+ `${fallbackStart},${formatISO(fallbackEnd, { representation: 'date' })}`
)📝 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.
| expect(result).toBe( | |
| `${formatISO(fallbackStart, { representation: 'date' })},${formatISO(fallbackEnd, { representation: 'date' })}` | |
| ) | |
| expect(result).toBe( | |
| `${fallbackStart},${formatISO(fallbackEnd, { representation: 'date' })}` | |
| ) |
🤖 Prompt for AI Agents
In
`@apps/journeys-admin/src/components/Editor/Slider/JourneyFlow/AnalyticsOverlaySwitch/buildPlausibleDateRange/buildPlausibleDateRange.spec.ts`
around lines 50 - 52, The test in buildPlausibleDateRange.spec.ts calls
formatISO on fallbackStart/fallbackEnd which are strings; update the expectation
to match the implementation by either converting those string fixtures to Date
objects before calling formatISO or (simpler) assert the raw string returned by
buildPlausibleDateRange: replace formatISO(fallbackStart/End, ...) with the
plain fallbackStart and fallbackEnd values so the expectation matches the
buildPlausibleDateRange behavior (refer to fallbackStart, fallbackEnd and the
buildPlausibleDateRange test case).
|
The latest updates on your projects.
|
|
The latest updates on your projects.
|
|
The latest updates on your projects.
|
|
The latest updates on your projects.
|
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/journeys-admin/src/components/Editor/Slider/JourneyFlow/AnalyticsOverlaySwitch/AnalyticsOverlaySwitch.tsx (1)
59-71: Inverted error handling logic.The condition
if (isDateRangeValid)will always be true whenonErroris called, because the query skips whenisDateRangeValid !== true(line 52). This means the "Invalid date range" error message will display for all GraphQL errors, even server errors unrelated to the date range.If the intent is to show "Invalid date range" only when the server rejects the date parameters, you should check the error object for a specific error code or message rather than relying on
isDateRangeValid.🐛 Proposed fix
Either remove the date range check entirely (since it's always true at this point):
onError: (_) => { - if (isDateRangeValid) { - enqueueSnackbar(t('Invalid date range'), { - variant: 'error', - preventDuplicate: true - }) - return - } enqueueSnackbar(t('Error fetching analytics'), { variant: 'error', preventDuplicate: true }) }Or if the API returns a specific error for invalid date ranges, check for it:
- onError: (_) => { - if (isDateRangeValid) { - enqueueSnackbar(t('Invalid date range'), { + onError: (error) => { + // Check if the error is specifically about date range + if (error.message.includes('date range')) { enqueueSnackbar(t('Invalid date range'), {
🧹 Nitpick comments (1)
apps/journeys-admin/src/components/Editor/Slider/JourneyFlow/AnalyticsOverlaySwitch/AnalyticsOverlaySwitch.spec.tsx (1)
3-3: Remove unused import.
userEventis imported but never used in the test file. Either remove it or consider using it instead offireEventfor the checkbox click on line 140, asuserEventbetter simulates real user interactions.♻️ Proposed fix
If removing:
-import userEvent from '@testing-library/user-event'Or if using
userEventinstead offireEvent:- fireEvent.click(analyticsCheckbox) + await userEvent.click(analyticsCheckbox)
|
The latest updates on your projects.
|
|
The latest updates on your projects.
|
|
The latest updates on your projects.
|
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Localization
✏️ Tip: You can customize this high-level summary in your review settings.