Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 7 additions & 6 deletions src/layout/header/shareUrl.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -92,19 +92,20 @@ describe('buildShareUrl — page params', () => {
// ---------------------------------------------------------------------------

// vehicleNumber is no longer a GlobalSearchState field; the /vehicle page keeps
// it page-local and appends it through PageShareParamsContext, sharing only the
// global `date`. These guard that contract (and that the page never leaks the
// operator/line/route global state a vehicle link must not carry).
// it page-local (usePageState, namespaced `vehicle.vehicleNumber`) and appends it
// through PageShareParamsContext, sharing only the global `date`. These guard that
// contract (and that the page never leaks the operator/line/route global state a
// vehicle link must not carry).

describe('buildShareUrl — /vehicle page', () => {
it('shares only the global date — not operator/line/route', () => {
const p = paramsOf(build('/vehicle', fullSearch))
expect(p).toEqual({ date: fullSearch.date })
})

it('appends the page-local vehicleNumber via extra params', () => {
const p = paramsOf(build('/vehicle', fullSearch, { vehicleNumber: '7489226' }))
expect(p).toEqual({ date: fullSearch.date, vehicleNumber: '7489226' })
it('appends the page-local vehicle.vehicleNumber via extra params', () => {
const p = paramsOf(build('/vehicle', fullSearch, { 'vehicle.vehicleNumber': '7489226' }))
expect(p).toEqual({ date: fullSearch.date, 'vehicle.vehicleNumber': '7489226' })
})

it('vehicleNumber is no longer a shareable global key on any page', () => {
Expand Down
4 changes: 2 additions & 2 deletions src/layout/header/shareUrl.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,8 @@ export const PAGE_SHARE_PARAMS: Partial<Record<string, ShareableKey[]>> = {
'/map': [],
'/velocity-heatmap': ['date'],
'/single-line-map': ['date', 'operatorId', 'lineNumber', 'routeKey', 'rideTime'],
// /vehicle shares the global date here; its page-local vehicleNumber is appended
// via PageShareParamsContext (like gaps_patterns' start/end dates).
// /vehicle shares the global date here; its page-local vehicle.vehicleNumber is
// appended via PageShareParamsContext (like gaps_patterns' start/end dates).
'/vehicle': ['date'],
'/operator': ['operatorId', 'date'],
'/train': ['date'],
Expand Down
9 changes: 7 additions & 2 deletions src/pages/components/VehicleSelector.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -22,9 +22,12 @@ const VehicleSelector = ({ vehicleNumber, disabled, setVehicleNumber }: VehicleS
const debouncedSetVehicleNumber = useCallback(debounce(setVehicleNumber, 200), [setVehicleNumber])
const { t } = useTranslation()

// The field keeps its own value so typing isn't throttled by the debounced parent
// update, but it must follow the prop when the parent changes it from elsewhere —
// a shared link seeds `vehicle.vehicleNumber` into usePageState one tick after mount.
useLayoutEffect(() => {
setValue(vehicleNumber)
}, [])
}, [vehicleNumber])

const handleClearInput = () => {
setValue(0)
Expand All @@ -43,7 +46,9 @@ const VehicleSelector = ({ vehicleNumber, disabled, setVehicleNumber }: VehicleS
className={textFieldClass}
label={t('choose_vehicle')}
type="text"
value={value && +value < 0 ? 0 : value}
// '' rather than undefined for "no vehicle": the field must stay controlled for
// its whole lifetime — a shared link fills it a tick after mount.
value={value && +value < 0 ? 0 : (value ?? '')}
onChange={(e) => {
const numericValue = normalizeVehicleNumber(e.target.value)
setValue(numericValue)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ describe('MapIndexLayer', () => {
})

const link = screen.getByRole('link', { name: '12-345-67' })
expect(link).toHaveAttribute('href', '/vehicle?vehicleNumber=1234567')
expect(link).toHaveAttribute('href', '/vehicle?vehicle.vehicleNumber=1234567')
// the number stays bracketed in the legend
expect(link.closest('bdi')).toHaveTextContent('(12-345-67)')
})
Expand All @@ -59,11 +59,11 @@ describe('MapIndexLayer', () => {
expect(container.querySelectorAll('.map-index-item')).toHaveLength(2)
expect(screen.getByRole('link', { name: '12-345-67' })).toHaveAttribute(
'href',
'/vehicle?vehicleNumber=1234567',
'/vehicle?vehicle.vehicleNumber=1234567',
)
expect(screen.getByRole('link', { name: '76-543-21' })).toHaveAttribute(
'href',
'/vehicle?vehicleNumber=7654321',
'/vehicle?vehicle.vehicleNumber=7654321',
)
})

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ function vehicleSubtitle(group: PositionGroup, t: TFunction): ReactNode {
const number = group.vehicleRef ? (
<MuiLink
component={Link}
to={`/vehicle?vehicleNumber=${group.vehicleRef}`}
to={`/vehicle?vehicle.vehicleNumber=${group.vehicleRef}`}
reloadDocument
underline="hover"
title={t('go_to_vehicle_page')}
Expand Down
2 changes: 1 addition & 1 deletion src/pages/singleLineMap/GpsCoverageStrip.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -226,7 +226,7 @@ export const GpsCoverageStrip = ({
<bdi>
<MuiLink
component={Link}
to={`/vehicle?vehicleNumber=${group.vehicleRef}`}
to={`/vehicle?vehicle.vehicleNumber=${group.vehicleRef}`}
reloadDocument
underline="hover">
{group.label ?? group.vehicleRef}
Expand Down
46 changes: 24 additions & 22 deletions src/pages/vehicle/index.tsx
Original file line number Diff line number Diff line change
@@ -1,13 +1,13 @@
import { CircularProgress, Grid, Typography } from '@mui/material'
import { useQuery } from '@tanstack/react-query'
import { useContext, useEffect, useMemo, useState } from 'react'
import { useCallback, useContext, useMemo } from 'react'
import { useTranslation } from 'react-i18next'
import { SIRI_API } from 'src/api/apiConfig'
import { getAllRoutesList } from 'src/api/gtfsService'
import dayjs, { ISRAEL_TIMEZONE, toIsraelTimezone, utcNoonForDateStr } from 'src/dayjs'
import { usePageState } from 'src/hooks/usePageState'
import { fromGtfsRoute } from 'src/model/busRoute'
import { GlobalSearchContext } from 'src/model/globalState'
import { InitialUrlParamsContext, PageShareParamsContext } from 'src/model/routeContext'
import { serviceDayBounds } from 'src/pages/components/utils/startTimeUtils'
import VehicleSelector, { normalizeVehicleNumber } from 'src/pages/components/VehicleSelector'
import { DateSelector } from '../components/DateSelector'
Expand All @@ -16,28 +16,33 @@ import { PageContainer } from '../components/PageContainer'
import { buildVehicleRideRows, VehicleRideRow } from './buildVehicleRideRows'
import { VehicleTable } from './VehicleTable'

// null, not '', is the "no vehicle chosen" value: usePageState omits null params
// from the share URL, so an unset page still produces a clean link.
type VehicleParams = { vehicleNumber: string | null }
type VehicleUi = { scrollPosition: number }

const VehiclePage = () => {
const { t } = useTranslation()
const { search, setSearch } = useContext(GlobalSearchContext)
const { date } = search
const initialUrlParams = useContext(InitialUrlParamsContext)
// LEGACY: manual share-param injection — replace with usePageState's per-page
// persistent `params` when this page is migrated.
const { setParams } = useContext(PageShareParamsContext)

// The vehicle number is page-local — never in GlobalSearchContext. Seeded once on
// mount from the URL captured at page load (InitialUrlParamsContext), and published
// to PageShareParamsContext for the Share button — the same page-local-param
// pattern gaps_patterns and timeBasedMap used before their usePageState migration.
const [vehicleNumber, setVehicleNumber] = useState<number | undefined>(() =>
normalizeVehicleNumber(initialUrlParams.vehicleNumber ?? ''),
// The vehicle number is page-local — never in GlobalSearchContext. usePageState
// persists it for the session, seeds it from `vehicle.vehicleNumber` in an
// incoming URL, and publishes it to the Share button.
const { params, setParams } = usePageState<VehicleParams, VehicleUi>('vehicle', {
params: { vehicleNumber: null },
ui: { scrollPosition: 0 },
})
const vehicleNumber = useMemo(
() => normalizeVehicleNumber(params.vehicleNumber ?? ''),
[params.vehicleNumber],
)
const setVehicleNumber = useCallback(
(value: number) => {
setParams((prev) => ({ ...prev, vehicleNumber: value ? String(value) : null }))
},
[setParams],
)

useEffect(() => {
if (vehicleNumber) setParams({ vehicleNumber: String(vehicleNumber) })
else setParams({})
return () => setParams({})
}, [vehicleNumber, setParams])

const { start: serviceDayStart, end: serviceDayEnd } = useMemo(
() => serviceDayBounds(date),
Expand Down Expand Up @@ -133,10 +138,7 @@ const VehiclePage = () => {
</Grid>
{/* choose vehicle */}
<Grid size={{ sm: 6, xs: 12 }}>
<VehicleSelector
vehicleNumber={vehicleNumber}
setVehicleNumber={(value) => setVehicleNumber(value || undefined)}
/>
<VehicleSelector vehicleNumber={vehicleNumber} setVehicleNumber={setVehicleNumber} />
</Grid>
</Grid>

Expand Down
6 changes: 6 additions & 0 deletions src/routes/MainRoute.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,12 @@ export const MainRoute = () => {
new URLSearchParams(window.location.search).forEach((v, k) => {
result[k] = v
})
// Accept a bare 'vehicleNumber' (old shared links, pre-usePageState) as the
// /vehicle page's namespaced param. Page params are namespaced `<page>.<key>`
// so they can't collide with a global search key.
if (result.vehicleNumber && !result['vehicle.vehicleNumber']) {
result['vehicle.vehicleNumber'] = result.vehicleNumber
}
return result
}, [])

Expand Down
18 changes: 16 additions & 2 deletions tests/vehicle.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,11 @@ import { mockVehicleApi, VEHICLE_NUMBER } from './vehicleMocks'
// Land directly on /vehicle with the number in the URL: a full navigation makes
// MainRoute capture it into InitialUrlParamsContext, which is how the page seeds
// its number (the same path the map legend's deep-link relies on).
const gotoSeededVehiclePage = async (page: Parameters<typeof setupTest>[0]) => {
await page.goto(`/vehicle?vehicleNumber=${VEHICLE_NUMBER}`)
const gotoSeededVehiclePage = async (
page: Parameters<typeof setupTest>[0],
key = 'vehicle.vehicleNumber',
) => {
await page.goto(`/vehicle?${key}=${VEHICLE_NUMBER}`)
await page.locator('.preloader').waitFor({ state: 'hidden' })
}

Expand Down Expand Up @@ -34,6 +37,17 @@ test.describe('Vehicle page', () => {
await page.waitForURL((u) => u.pathname === '/single-line-map')
})

test('still seeds from a pre-namespace link carrying a bare vehicleNumber', async ({ page }) => {
await setupTest(page)
await mockVehicleApi(page)
await gotoSeededVehiclePage(page, 'vehicleNumber')

await expect(rideRow(page, '04:30')).toBeVisible()
await expect(page.getByRole('textbox', { name: i18next.t('choose_vehicle') })).toHaveValue(
VEHICLE_NUMBER,
)
})

test('typing a vehicle number in the selector loads that vehicle rides', async ({ page }) => {
await setupTest(page)
await mockVehicleApi(page)
Expand Down
2 changes: 1 addition & 1 deletion tests/visual.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,7 @@ for (const mode of ['Light', 'Dark', 'LTR']) {
test(`Vehicle Page Should Look Good [${mode}]`, async ({ page, eyes }) => {
await mockVehicleApi(page)
// full navigation so MainRoute seeds the vehicle number from the URL
await page.goto(`/vehicle?vehicleNumber=${VEHICLE_NUMBER}`)
await page.goto(`/vehicle?vehicle.vehicleNumber=${VEHICLE_NUMBER}`)
await page.locator('.preloader').waitFor({ state: 'hidden' })
// wait for the resolved rides table (incl. the post-midnight 🌙 row) before snapping
await page.getByRole('row').filter({ hasText: '🌙 00:30' }).waitFor()
Expand Down
Loading