-
Notifications
You must be signed in to change notification settings - Fork 10
Dev: Fix useExhaustiveDependencies — Percentile Calculator #5533
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
base: main
Are you sure you want to change the base?
Changes from all commits
8266a9b
9dead61
6723510
603569b
6bb3652
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 |
|---|---|---|
| @@ -0,0 +1,55 @@ | ||
| import { fireEvent, render, screen } from '@testing-library/react' | ||
| import type { StationSummaryResponse } from '@wps/api/percentileAPI' | ||
| import { describe, expect, it } from 'vitest' | ||
| import { PercentileStationResultTable } from './PercentileStationResultTable' | ||
|
|
||
| const makeStationResponse = (years: number[]): StationSummaryResponse => ({ | ||
| ffmc: 88.5, | ||
| isi: 9.1, | ||
| bui: 72.0, | ||
|
Check warning on line 9 in web/apps/wps-web/src/features/percentileCalculator/components/percentileStationResultTable.test.tsx
|
||
| years, | ||
| station: { | ||
| code: 101, | ||
| name: 'Test Station', | ||
| lat: 49.0, | ||
|
Check warning on line 14 in web/apps/wps-web/src/features/percentileCalculator/components/percentileStationResultTable.test.tsx
|
||
| long: -120.0, | ||
|
Check warning on line 15 in web/apps/wps-web/src/features/percentileCalculator/components/percentileStationResultTable.test.tsx
|
||
| ecodivision_name: 'Montane Cordillera', | ||
| core_season: { start_month: 5, start_day: 1, end_month: 9, end_day: 30 } | ||
| } | ||
| }) | ||
|
|
||
| describe('PercentileStationResultTable snackbar', () => { | ||
| it('does not show warning when data covers the full time range', () => { | ||
| const years = [2020, 2021, 2022, 2023, 2024] | ||
| render(<PercentileStationResultTable stationResponse={makeStationResponse(years)} timeRange={5} />) | ||
|
|
||
| expect(screen.queryByText(/Data only available for/)).not.toBeInTheDocument() | ||
| }) | ||
|
|
||
| it('shows warning when fewer years of data are available than the time range', () => { | ||
| const years = [2022, 2023, 2024] | ||
| render(<PercentileStationResultTable stationResponse={makeStationResponse(years)} timeRange={10} />) | ||
|
|
||
| expect(screen.getByText('Data only available for 3 of 10 years')).toBeInTheDocument() | ||
| }) | ||
|
|
||
| it('re-shows the warning after dismissal when timeRange increases beyond available data', () => { | ||
| const years = [2022, 2023, 2024] | ||
| const { rerender } = render( | ||
| <PercentileStationResultTable stationResponse={makeStationResponse(years)} timeRange={2} /> | ||
| ) | ||
| // 3 years >= timeRange=2 — no warning | ||
| expect(screen.queryByText(/Data only available for/)).not.toBeInTheDocument() | ||
|
|
||
| // Increase time range beyond available data — warning appears | ||
| rerender(<PercentileStationResultTable stationResponse={makeStationResponse(years)} timeRange={5} />) | ||
| expect(screen.getByText('Data only available for 3 of 5 years')).toBeInTheDocument() | ||
|
|
||
| // User dismisses the snackbar | ||
| fireEvent.click(screen.getByRole('button', { name: 'Close' })) | ||
|
|
||
| // Slider increases time range further — warning re-opens | ||
| rerender(<PercentileStationResultTable stationResponse={makeStationResponse(years)} timeRange={10} />) | ||
| expect(screen.getByText('Data only available for 3 of 10 years')).toBeInTheDocument() | ||
| }) | ||
| }) | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,107 @@ | ||
| import { fireEvent, render, screen, waitFor } from '@testing-library/react' | ||
| import { fetchPercentiles } from 'features/percentileCalculator/slices/percentilesSlice' | ||
| import { Provider } from 'react-redux' | ||
| import { MemoryRouter } from 'react-router-dom' | ||
| import { beforeEach, describe, expect, it, vi } from 'vitest' | ||
| import { createTestStore } from '@/test/testUtils' | ||
| import PercentileCalculatorPage from './PercentileCalculatorPage' | ||
|
|
||
| const MOCKED_CURRENT_YEAR = 2024 | ||
|
|
||
| vi.mock('@wps/api/stationAPI', () => ({ | ||
| getStations: vi.fn(() => Promise.resolve([])), | ||
| StationSource: { unspecified: 'unspecified' } | ||
| })) | ||
|
|
||
| vi.mock('features/stations/slices/stationsSlice', async () => { | ||
| const actual = await vi.importActual<typeof import('features/stations/slices/stationsSlice')>( | ||
| 'features/stations/slices/stationsSlice' | ||
| ) | ||
| return { ...actual, fetchWxStations: vi.fn(() => () => {}) } | ||
| }) | ||
|
|
||
| vi.mock('features/percentileCalculator/slices/percentilesSlice', async () => { | ||
| const actual = await vi.importActual<typeof import('features/percentileCalculator/slices/percentilesSlice')>( | ||
| 'features/percentileCalculator/slices/percentilesSlice' | ||
| ) | ||
| return { ...actual, fetchPercentiles: vi.fn(() => () => {}) } | ||
| }) | ||
|
|
||
| vi.mock('features/percentileCalculator/components/TimeRangeSlider', () => ({ | ||
| TimeRangeSlider: ({ onYearRangeChange }: { onYearRangeChange: (n: number) => void }) => ( | ||
| <button type="button" data-testid="change-time-range" onClick={() => onYearRangeChange(5)}> | ||
| Change Range | ||
| </button> | ||
| ), | ||
| yearWhenTheCalculationIsDone: 2024 | ||
| })) | ||
|
|
||
| vi.mock('features/percentileCalculator/components/WxStationDropdown', () => ({ | ||
| default: ({ onChange }: { onChange: (codes: number[]) => void }) => ( | ||
| <button type="button" data-testid="select-station" onClick={() => onChange([101])}> | ||
| Select Station | ||
| </button> | ||
| ) | ||
| })) | ||
|
|
||
| vi.mock('features/percentileCalculator/components/PercentileResults', () => ({ default: () => null })) | ||
| vi.mock('features/percentileCalculator/components/PercentileTextfield', () => ({ PercentileTextfield: () => null })) | ||
|
|
||
| const renderPage = (search = '') => | ||
| render( | ||
| <Provider store={createTestStore()}> | ||
| <MemoryRouter initialEntries={[`/${search}`]}> | ||
| <PercentileCalculatorPage /> | ||
| </MemoryRouter> | ||
| </Provider> | ||
| ) | ||
|
|
||
| describe('PercentileCalculatorPage', () => { | ||
| beforeEach(() => { | ||
| vi.mocked(fetchPercentiles).mockClear() | ||
| }) | ||
|
|
||
| it('fetches percentiles on mount when station codes are in the URL', async () => { | ||
| renderPage('?codes=101,202') | ||
|
|
||
| await waitFor(() => { | ||
| expect(fetchPercentiles).toHaveBeenCalledOnce() | ||
| expect(fetchPercentiles).toHaveBeenCalledWith([101, 202], 90, expect.any(Object)) | ||
| }) | ||
| }) | ||
|
|
||
| it('does not fetch percentiles again when the time range slider changes', async () => { | ||
| renderPage('?codes=101') | ||
|
|
||
| await waitFor(() => { | ||
| expect(fetchPercentiles).toHaveBeenCalledOnce() | ||
| }) | ||
|
|
||
| fireEvent.click(screen.getByTestId('change-time-range')) | ||
|
|
||
| // Allow any potential re-renders to settle | ||
| await new Promise(resolve => setTimeout(resolve, 50)) | ||
|
|
||
| expect(fetchPercentiles).toHaveBeenCalledOnce() | ||
| }) | ||
|
|
||
| it('uses the slider yearRange at the time Calculate is clicked, not the default', async () => { | ||
| renderPage() | ||
|
|
||
| // Change slider to timeRange=5 before selecting any station | ||
| fireEvent.click(screen.getByTestId('change-time-range')) | ||
|
|
||
| // Select a station to enable Calculate | ||
| fireEvent.click(screen.getByTestId('select-station')) | ||
|
|
||
| // Click Calculate — this navigates to ?codes=101 | ||
| fireEvent.click(screen.getByText('Calculate')) | ||
|
|
||
| await waitFor(() => { | ||
| expect(fetchPercentiles).toHaveBeenCalledWith([101], 90, { | ||
| start: MOCKED_CURRENT_YEAR - 4, // timeRange=5 → start = 2024 - (5-1) | ||
| end: MOCKED_CURRENT_YEAR | ||
| }) | ||
| }) | ||
| }) | ||
| }) |
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.
This looks like it introduces a bit of a behaviour change, where the data will load without hitting
Calculateafter you adjust the slider. The warning snackbar also just flashes and disappears before being able to read itThere 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.
Good catch, fixed in: 6bb3652
The root cause was
yearRangebeing listed as a dep of thecodesFromQueryeffect. When the slider moved,yearRangechanged → the effect re-ran →fetchPercentilesfired without a Calculate click.Fix: store
yearRangein a ref that's updated inline during render (yearRangeRef.current = yearRange), then readyearRangeRef.currentinside the effect. The effect only depends oncodesFromQueryanddispatch, so it only fires when the URL changes (i.e. Calculate was clicked). The ref always holds the latest value, so the correct year range is still passed to the API.The snackbar flash was a downstream symptom, the spurious fetch set
loading: true, which unmounted the result table and reset itssnackbarOpenstate. That resolves with the fetch fix.Added regression tests as well.
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.
Could you try playing with percentile calc in dev? There's more significant issues now. Changing years and calculating doesn't do anything
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.
Oh yeah that's broken, at least we can test this in isolation, this app is a bit tricky.