wrongmove: round-3 fix sweep — scrape pipeline, BUY tab, filter URL state, render hygiene, map polish

Coordinated fix across 31 bugs found in a parallel QA pass. Findings docs at /tmp/wrongmove-bugs/qa-round-3/qa{1,2,3,4}-*.md.

## Backend / scrape (Fix-1) — 8 bugs

- B1 [P0] Scrape totally broken on prod: pod UID 100 vs NFS dir 1000:1000 mode 775 → PermissionError on every never-seen listing. Switched Dockerfile to explicit `useradd --uid 1000 --gid 1000`; added securityContext + chown initContainer to k8s/{api,celery-beat}-deployment.yaml. Celery worker manifest lives outside this repo — Dockerfile UID change is the load-bearing fix.
- B4 [P1] Celery broker reaped every ~30s by Redis HAProxy idle timeout. Added `broker_transport_options` / `result_backend_transport_options` with `socket_keepalive=True, health_check_interval=25` in celery_app.py + same kwargs on every redis.from_url/Redis call across services/, utils/redis_lock.py, redis_repository.py.
- B5 [P1] dump_listings_task never published terminal FAILURE to the task_progress pub/sub channel — UI polled forever. Wrap body in try/except that publishes FAILURE before re-raising.
- B6 [P1] _process_worker had no per-listing exception handler — one bad listing killed the whole scrape via asyncio.gather. Wrap loop body in try/except Exception (re-raises CancelledError).
- B20 [P2] dump_listings_task gained time_limit=3600, soft_time_limit=3500, acks_late=True.
- B21 [P2] RedisRepository moved off shared db0 (was alongside paperless-ngx) to db3 via REDIS_USER_DB env var; keys prefixed `wrongmove:user:`.
- B32 [P3] redis_lock now uses uuid4() owner token + Lua compare-and-delete.
- B33 [P3] Slack notify in refresh_listings → asyncio.create_task (fire-and-forget).

## Frontend filter system (Fix-2) — 7 bugs

- B2 [P0] BUY tab click triggered "Maximum update depth exceeded" → ErrorBoundary. Replaced the three mutually-triggering useEffects in FilterBar with a single one-way controlled-value flow (URL → parent state → form), guarded by previousListingTypeRef so price-defaults fires once per real transition.
- B3 [P0] Filter values never reached the URL. Wired useFilterParams.setFilterValues into FilterBar/FilterPanel onSubmit + handleRemoveChip + new handleResetAllFilters; fed parsed filterValues into both forms' defaultValues; added URL→form sync via form.reset on browser back/forward.
- B8 [P1] Chip removal now resets form state via new FilterBar onFormReady callback — More badge no longer sticks.
- B12 [P2] Desktop swipe-review FAB added next to header (mobile FAB unchanged).
- B17 [P2] "Reset all" affordance on chip strip.
- B22 [P2] formatPrice precision: 1500 → £1.5k, 2500 → £2.5k (no longer collides with £2k/£3k defaults).
- B30 [P3] last_seen_days input gained min={0}.

## Frontend render hygiene + data integrity (Fix-3) — 8 bugs

- B7 [P1] streamingService bails on first non-NDJSON chunk (HTML response = backend down) and throws StreamParseError so the existing AlertError dialog surfaces a single user-visible error instead of 18× console.error spam.
- B9 [P1] formatDuration widened to (null|undefined|number): returns "—" for non-finite or negative, caps implausibly large values.
- B10 [P1] PropertyCard / PropertyCardCompact / SwipeCard JSX leaves render "—" for null total_price/qm/qmprice (was "£0/0 m²/£0/m²" — looked like free listings).
- B13 [P2] hexgrid worker reduceAverage uses Number.isFinite filter instead of !isNaN (which incorrectly accepted null → 0, biasing per-hex averages low).
- B14 [P2] ListingDetail Overview wraps agency in "Listed by" labelled block so it can't collapse to a bare agency name.
- B15 [P2] Compact POIDistanceBadges iterates all three travel modes with "—" for missing, matching the detail-sheet Travel table.
- B24 [P3] Drawer.Description (sr-only) added to ListingDetailSheet + MobileBottomSheet to silence Radix a11y warning.
- B25 [P3] lastSeenDays clamped to ≥0 so future timestamps don't render as "-7d ago".

## Frontend map / carousel / tasks polish (Fix-4) — 8 bugs

- B11 [P2] HexgridHeatmapClient destroy race: Map.tsx adds .catch() + ref guard so post-destroy promise rejections are silent no-ops. Verified by browser smoke (24 rapid Map↔List toggles → 0 pageErrors).
- B16 [P2] PhotoCarousel + inner CardCarousel gained keyboard nav (Arrow keys).
- B18 [P2] Default map center moved from Czech Republic to London (zoom 10).
- B19+B29 [P2/P3] Mapbox token: no longer hard-coded fallback; reads env-only and shows a clear "Map unavailable — set VITE_MAPBOX_TOKEN" banner when missing.
- B23 [P3] PhotoCarousel suppresses "1/1" counter for single-photo listings; added onError fallback for broken URLs.
- B26 [P3] PhotoCarousel only enables loop when photos.length > 1.
- B27 [P3] TaskIndicator cancel/clear-all buttons gained aria-label + data-testid.
- B28 [P3] useTaskProgress strips terminal-local task IDs from the polling union — no more forever-poll on completed tasks.

## Tests

74 new vitest tests + 18 new pytest tests. Local: tsc clean, 201 vitest tests pass, 633 pytest tests pass.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
This commit is contained in:
Viktor Barzin 2026-05-10 22:27:29 +00:00
parent 0b5308200e
commit a42944a756
46 changed files with 2260 additions and 238 deletions

View file

@ -55,7 +55,13 @@ RUN pytest tests/ -x -q
# Stage 4: Final image — combine venv from builder + runtime base # Stage 4: Final image — combine venv from builder + runtime base
FROM runtime-base AS production FROM runtime-base AS production
RUN adduser --system --no-create-home appuser # Create appuser with explicit UID 1000 / GID 1000 to match the NFS-backed
# data PVC ownership (mode 775 dirs from older container builds were owned by
# 1000:1000). Previously this used --system which assigned UID 100 / GID
# 65534, causing PermissionError when the scraper tried to create new listing
# directories on the NFS mount.
RUN groupadd --gid 1000 appuser && \
useradd --uid 1000 --gid 1000 --no-create-home --shell /usr/sbin/nologin appuser
WORKDIR /app WORKDIR /app
@ -67,7 +73,7 @@ ENV PATH="/app/.venv/bin:$PATH"
# Copy the application code # Copy the application code
COPY . . COPY . .
RUN chown -R appuser /app RUN chown -R appuser:appuser /app
USER appuser USER appuser

View file

@ -600,8 +600,14 @@ async def refresh_listings(
query_parameters: Annotated[QueryParameters, Depends(get_query_parameters)], query_parameters: Annotated[QueryParameters, Depends(get_query_parameters)],
) -> dict[str, str]: ) -> dict[str, str]:
"""Trigger a background task to refresh listings.""" """Trigger a background task to refresh listings."""
await send_notification( # Fire-and-forget the Slack notification so the API response isn't
f"{user.email} refreshing listings with query parameters {query_parameters.model_dump_json()}" # blocked on the webhook round-trip (and so the no-op path when
# SLACK_WEBHOOK_URL is unset doesn't add latency). send_notification
# already catches its own exceptions so an orphaned task is harmless.
asyncio.create_task(
send_notification(
f"{user.email} refreshing listings with query parameters {query_parameters.model_dump_json()}"
)
) )
repository = ListingRepository(engine) repository = ListingRepository(engine)

View file

@ -18,12 +18,26 @@ app = Celery(
include=["tasks.listing_tasks", "tasks.poi_tasks"], include=["tasks.listing_tasks", "tasks.poi_tasks"],
) )
# Keep broker / result-backend connections alive when sitting behind an
# HAProxy / load balancer that idles TCP connections (the in-cluster Redis
# HAProxy reaps idle conns after 30s). Without these options the worker
# logs a "Connection closed by server" every ~30s and progress publishes
# silently drop on the closed socket.
app.conf.update( app.conf.update(
task_serializer="json", task_serializer="json",
result_serializer="json", result_serializer="json",
accept_content=["json"], accept_content=["json"],
timezone="UTC", timezone="UTC",
enable_utc=True, enable_utc=True,
broker_transport_options={
"socket_keepalive": True,
"health_check_interval": 25,
},
result_backend_transport_options={
"socket_keepalive": True,
"health_check_interval": 25,
},
broker_heartbeat=10,
) )
# --------------------------------------------------------------------------- # ---------------------------------------------------------------------------

View file

@ -9,7 +9,7 @@ import LoginModal from './components/LoginModal';
import AuthCallback from './components/AuthCallback'; import AuthCallback from './components/AuthCallback';
import { Map } from './components/Map'; import { Map } from './components/Map';
import { type ParameterValues, DEFAULT_FILTER_VALUES, Metric, ListingType } from './components/FilterPanel'; import { type ParameterValues, DEFAULT_FILTER_VALUES, Metric, ListingType } from './components/FilterPanel';
import { FilterBar } from './components/FilterBar'; import { FilterBar, type FilterBarFormHandle } from './components/FilterBar';
import { FilterChips } from './components/FilterChips'; import { FilterChips } from './components/FilterChips';
import { VisualizationCard } from './components/VisualizationCard'; import { VisualizationCard } from './components/VisualizationCard';
import { Header } from './components/Header'; import { Header } from './components/Header';
@ -69,13 +69,26 @@ function AppContent() {
? { sub: 'dev-user', email: 'dev@localhost', name: 'Dev User', accessToken: 'dev-token', provider: 'passkey' as const } ? { sub: 'dev-user', email: 'dev@localhost', name: 'Dev User', accessToken: 'dev-token', provider: 'passkey' as const }
: null : null
); );
// URL-derived filter state. filterValues mirrors the current URL; setFilterValues writes back.
// Stable across re-renders for the chip/form reset paths.
const { filterValues: urlFilterValues, setFilterValues: setUrlFilterValues, viewMode, setViewMode } = useFilterParams();
// Initial filter values read from URL on first mount (deep-link support). Captured once so the
// form's defaultValues don't re-initialise on every URL change (RHF only honours mount-time defaults).
const initialFilterValuesRef = useRef<ParameterValues>(urlFilterValues);
const [queryParameters, setQueryParameters] = useState<ParameterValues | null>( const [queryParameters, setQueryParameters] = useState<ParameterValues | null>(
DEV_BYPASS_AUTH ? { ...DEFAULT_FILTER_VALUES, available_from: new Date() } : null DEV_BYPASS_AUTH ? urlFilterValues : null
); );
const [submitError, setSubmitError] = useState<string | null>(null); const [submitError, setSubmitError] = useState<string | null>(null);
const [alertDialogIsOpen, setAlertDialogIsOpen] = useState(false); const [alertDialogIsOpen, setAlertDialogIsOpen] = useState(false);
const [isLoading, setIsLoading] = useState(false); const [isLoading, setIsLoading] = useState(false);
const { viewMode, setViewMode } = useFilterParams();
// Form handle (from FilterBar) used to call form.reset() when chips/URL change.
const filterBarFormRef = useRef<FilterBarFormHandle | null>(null);
const handleFilterBarFormReady = useCallback((handle: FilterBarFormHandle) => {
filterBarFormRef.current = handle;
}, []);
const [mobileFilterOpen, setMobileFilterOpen] = useState(false); const [mobileFilterOpen, setMobileFilterOpen] = useState(false);
const [highlightedProperty, setHighlightedProperty] = useState<string | null>(null); const [highlightedProperty, setHighlightedProperty] = useState<string | null>(null);
const [streamingProgress, setStreamingProgress] = useState<StreamingProgress | null>(null); const [streamingProgress, setStreamingProgress] = useState<StreamingProgress | null>(null);
@ -88,8 +101,28 @@ function AppContent() {
travelMode: 'WALK' | 'BICYCLE' | 'TRANSIT'; travelMode: 'WALK' | 'BICYCLE' | 'TRANSIT';
} | null>(null); } | null>(null);
const [poiTravelFilters, setPoiTravelFilters] = useState<Record<number, POITravelFilter>>({}); const [poiTravelFilters, setPoiTravelFilters] = useState<Record<number, POITravelFilter>>({});
const [currentMetric, setCurrentMetric] = useState<Metric>(DEFAULT_FILTER_VALUES.metric); const [currentMetric, setCurrentMetric] = useState<Metric>(urlFilterValues.metric);
const [listingType, setListingType] = useState<ListingType>(DEFAULT_FILTER_VALUES.listing_type); const [listingType, setListingTypeState] = useState<ListingType>(urlFilterValues.listing_type);
// Wraps the listingType setter so any change (e.g. from the Header tab) also
// propagates to the URL — this is the link that makes B2's URL-deep-link visible
// in the address bar after a tab click.
const setListingType = useCallback(
(next: ListingType) => {
setListingTypeState(next);
// Only push to URL if the value actually changed (avoid no-op writes).
// Note: we read the current URL state below; queryParameters may not exist yet
// pre-first-load, in which case we fall back to urlFilterValues.
setUrlFilterValues({
...(urlFilterValues),
listing_type: next,
// Reset price defaults at the URL boundary so the form's price-defaults
// effect can pick them up via initialValues / the urlFilterValues effect.
min_price: next === ListingType.BUY ? 300000 : 2000,
max_price: next === ListingType.BUY ? 600000 : 3000,
});
},
[setUrlFilterValues, urlFilterValues],
);
const isMobile = useIsMobile(); const isMobile = useIsMobile();
const [, setActiveCardFeature] = useState<PropertyFeature | null>(null); const [, setActiveCardFeature] = useState<PropertyFeature | null>(null);
const [showReviewMode, setShowReviewMode] = useState(false); const [showReviewMode, setShowReviewMode] = useState(false);
@ -158,6 +191,33 @@ function AppContent() {
fetchUserPOIs(user).then(setUserPOIs).catch(() => {}); fetchUserPOIs(user).then(setUserPOIs).catch(() => {});
}, [user]); }, [user]);
// Sync URL changes back into form on browser back/forward (B3).
// We compare against queryParameters to avoid bouncing on our own setUrlFilterValues writes.
useEffect(() => {
if (!queryParameters) return;
// Compare key fields (skip available_from since defaults always differ on re-construction)
const same =
urlFilterValues.listing_type === queryParameters.listing_type &&
urlFilterValues.metric === queryParameters.metric &&
urlFilterValues.min_price === queryParameters.min_price &&
urlFilterValues.max_price === queryParameters.max_price &&
urlFilterValues.min_bedrooms === queryParameters.min_bedrooms &&
urlFilterValues.max_bedrooms === queryParameters.max_bedrooms &&
urlFilterValues.min_sqm === queryParameters.min_sqm &&
urlFilterValues.max_sqm === queryParameters.max_sqm &&
urlFilterValues.last_seen_days === queryParameters.last_seen_days &&
urlFilterValues.district === queryParameters.district;
if (same) return;
filterBarFormRef.current?.reset(urlFilterValues);
if (urlFilterValues.listing_type !== listingType) {
setListingType(urlFilterValues.listing_type);
}
setQueryParameters(urlFilterValues);
loadListings(urlFilterValues);
// intentionally omit loadListings from deps to avoid identity churn
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [urlFilterValues]);
// Load listings function - used by both auto-load and manual submit // Load listings function - used by both auto-load and manual submit
const loadListings = useCallback(async (parameters: ParameterValues) => { const loadListings = useCallback(async (parameters: ParameterValues) => {
if (!user) return; if (!user) return;
@ -347,12 +407,13 @@ function AppContent() {
} }
initialLoadTriggeredRef.current = true; initialLoadTriggeredRef.current = true;
const defaultParams: ParameterValues = { // Use URL-derived filter values on initial auto-load so deep-links work (B3).
...DEFAULT_FILTER_VALUES, const initialParams: ParameterValues = {
available_from: new Date(), ...initialFilterValuesRef.current,
available_from: initialFilterValuesRef.current.available_from ?? new Date(),
}; };
loadListings(defaultParams); loadListings(initialParams);
}, [user, loadListings]); }, [user, loadListings]);
const handleTaskCompleted = useCallback(() => { const handleTaskCompleted = useCallback(() => {
@ -375,6 +436,12 @@ function AppContent() {
} }
const onSubmit = async (action: 'fetch-data' | 'visualize', parameters: ParameterValues) => { const onSubmit = async (action: 'fetch-data' | 'visualize', parameters: ParameterValues) => {
// Persist filter state to URL on every submit so reload + share-URL works (B3).
setUrlFilterValues(parameters);
// Reflect listing-type changes from the form into the parent state (single source of truth).
if (parameters.listing_type !== listingType) {
setListingType(parameters.listing_type);
}
if (action === 'visualize') { if (action === 'visualize') {
loadListings(parameters); loadListings(parameters);
} else if (action === 'fetch-data') { } else if (action === 'fetch-data') {
@ -409,7 +476,7 @@ function AppContent() {
// Optionally: pan map to coordinates // Optionally: pan map to coordinates
}; };
/** Handle removing a filter chip: reset the field to its default value and re-submit */ /** Handle removing a filter chip: reset the field, sync form + URL, then re-submit (B8). */
const handleRemoveChip = (key: keyof ParameterValues) => { const handleRemoveChip = (key: keyof ParameterValues) => {
if (!queryParameters) return; if (!queryParameters) return;
const updated = { ...queryParameters }; const updated = { ...queryParameters };
@ -431,9 +498,26 @@ function AppContent() {
default: default:
(updated as Record<string, unknown>)[key] = (DEFAULT_FILTER_VALUES as Record<string, unknown>)[key]; (updated as Record<string, unknown>)[key] = (DEFAULT_FILTER_VALUES as Record<string, unknown>)[key];
} }
// Reset the form so the popover inputs and the "More (N)" badge stay in sync (B8).
filterBarFormRef.current?.reset(updated);
setUrlFilterValues(updated);
loadListings(updated); loadListings(updated);
}; };
/** Reset every filter to defaults (B17). Clears URL, form, and re-loads. */
const handleResetAllFilters = () => {
const defaults: ParameterValues = {
...DEFAULT_FILTER_VALUES,
available_from: new Date(),
};
filterBarFormRef.current?.reset(defaults);
setUrlFilterValues(defaults);
if (defaults.listing_type !== listingType) {
setListingType(defaults.listing_type);
}
loadListings(defaults);
};
const renderMainContent = () => { const renderMainContent = () => {
if (!processedListingData) { if (!processedListingData) {
return ( return (
@ -607,6 +691,7 @@ function AppContent() {
userPOIs={userPOIs} userPOIs={userPOIs}
poiTravelFilters={poiTravelFilters} poiTravelFilters={poiTravelFilters}
onPoiTravelFiltersChange={setPoiTravelFilters} onPoiTravelFiltersChange={setPoiTravelFilters}
initialValues={initialFilterValuesRef.current}
/> />
</div> </div>
<div className="shrink-0 p-4"> <div className="shrink-0 p-4">
@ -687,24 +772,28 @@ function AppContent() {
) : ( ) : (
/* Desktop layout: no sidebar, full-width main area */ /* Desktop layout: no sidebar, full-width main area */
<> <>
{/* Horizontal Filter Bar */} {/* Horizontal Filter Bar with adjacent Review entry (B12) */}
<FilterBar <div className="relative">
onSubmit={onSubmit} <FilterBar
isLoading={isLoading} onSubmit={onSubmit}
user={user} isLoading={isLoading}
userPOIs={userPOIs} user={user}
onPOIsChange={setUserPOIs} userPOIs={userPOIs}
poiTravelFilters={poiTravelFilters} onPOIsChange={setUserPOIs}
onPoiTravelFiltersChange={setPoiTravelFilters} poiTravelFilters={poiTravelFilters}
listingType={listingType} onPoiTravelFiltersChange={setPoiTravelFilters}
onListingTypeChange={setListingType} listingType={listingType}
poiPickerActive={poiPickerActive} onListingTypeChange={setListingType}
onPoiPickerActiveChange={setPoiPickerActive} poiPickerActive={poiPickerActive}
pickedPoiLocation={pickedPoiLocation} onPoiPickerActiveChange={setPoiPickerActive}
onPickedPoiLocationChange={setPickedPoiLocation} pickedPoiLocation={pickedPoiLocation}
currentMetric={currentMetric} onPickedPoiLocationChange={setPickedPoiLocation}
onTaskCreated={handlePOITaskCreated} currentMetric={currentMetric}
/> onTaskCreated={handlePOITaskCreated}
initialValues={initialFilterValuesRef.current}
onFormReady={handleFilterBarFormReady}
/>
</div>
{/* Active Filter Chips */} {/* Active Filter Chips */}
{queryParameters && ( {queryParameters && (
@ -712,6 +801,7 @@ function AppContent() {
values={queryParameters} values={queryParameters}
defaults={{ ...DEFAULT_FILTER_VALUES, available_from: new Date() }} defaults={{ ...DEFAULT_FILTER_VALUES, available_from: new Date() }}
onRemove={handleRemoveChip} onRemove={handleRemoveChip}
onResetAll={handleResetAllFilters}
/> />
)} )}
@ -727,8 +817,21 @@ function AppContent() {
{/* Main content area (full width) */} {/* Main content area (full width) */}
<main className="flex-1 flex flex-col min-h-0 min-w-0"> <main className="flex-1 flex flex-col min-h-0 min-w-0">
{/* Map/List Container */} {/* Map/List Container */}
<div className="flex-1 flex overflow-hidden min-h-0"> <div className="flex-1 flex overflow-hidden min-h-0 relative">
{renderMainContent()} {renderMainContent()}
{/* Desktop Swipe / Review entry point (B12) */}
<Button
type="button"
variant="outline"
size="lg"
className="absolute right-4 top-4 z-50 rounded-full shadow-lg h-12 w-12 bg-background"
onClick={() => setShowReviewMode(true)}
disabled={!processedListingData || processedListingData.features.length === 0}
aria-label="Open swipe review"
data-testid="desktop-review-fab"
>
<Heart className="h-5 w-5" />
</Button>
</div> </div>
{/* Stats Bar with Metric Selector */} {/* Stats Bar with Metric Selector */}

View file

@ -26,6 +26,20 @@ if (typeof globalThis.ResizeObserver === 'undefined') {
}; };
} }
// Polyfill IntersectionObserver for jsdom (used by embla-carousel internally)
if (typeof globalThis.IntersectionObserver === 'undefined') {
// @ts-expect-error minimal jsdom stub
globalThis.IntersectionObserver = class IntersectionObserver {
observe() {}
unobserve() {}
disconnect() {}
takeRecords() { return []; }
root = null;
rootMargin = '';
thresholds = [];
};
}
// Mock mapbox-gl (requires WebGL which jsdom doesn't support) // Mock mapbox-gl (requires WebGL which jsdom doesn't support)
vi.mock('mapbox-gl', () => ({ vi.mock('mapbox-gl', () => ({
default: { default: {

View file

@ -1,4 +1,4 @@
import { useState, useEffect, useCallback } from 'react'; import { useState, useEffect, useCallback, useRef } from 'react';
import { useForm } from 'react-hook-form'; import { useForm } from 'react-hook-form';
import { zodResolver } from '@hookform/resolvers/zod'; import { zodResolver } from '@hookform/resolvers/zod';
import { z } from 'zod'; import { z } from 'zod';
@ -20,6 +20,7 @@ import {
} from './FilterPanel'; } from './FilterPanel';
import type { AuthUser } from '@/auth/types'; import type { AuthUser } from '@/auth/types';
import type { POI, POITravelFilter } from '@/types'; import type { POI, POITravelFilter } from '@/types';
import type { UseFormReturn } from 'react-hook-form';
// ── Zod schema (same as FilterPanel) ── // ── Zod schema (same as FilterPanel) ──
const formSchema = z.object({ const formSchema = z.object({
@ -41,6 +42,13 @@ const formSchema = z.object({
type FormValues = z.infer<typeof formSchema>; type FormValues = z.infer<typeof formSchema>;
// ── Props ── // ── Props ──
export interface FilterBarFormHandle {
/** Reset the form to the given values (used by parent to mirror URL → form). */
reset: (values: ParameterValues) => void;
/** Direct access to the form (used by sibling components like FilterChips). */
form: UseFormReturn<FormValues>;
}
interface FilterBarProps { interface FilterBarProps {
onSubmit: (action: 'fetch-data' | 'visualize', parameters: ParameterValues) => void; onSubmit: (action: 'fetch-data' | 'visualize', parameters: ParameterValues) => void;
isLoading: boolean; isLoading: boolean;
@ -49,6 +57,7 @@ interface FilterBarProps {
onPOIsChange: (pois: POI[]) => void; onPOIsChange: (pois: POI[]) => void;
poiTravelFilters: Record<number, POITravelFilter>; poiTravelFilters: Record<number, POITravelFilter>;
onPoiTravelFiltersChange: (filters: Record<number, POITravelFilter>) => void; onPoiTravelFiltersChange: (filters: Record<number, POITravelFilter>) => void;
/** Controlled listing type (URL-driven). FilterBar reflects this value, never the inverse. */
listingType: ListingType; listingType: ListingType;
onListingTypeChange: (type: ListingType) => void; onListingTypeChange: (type: ListingType) => void;
poiPickerActive: boolean; poiPickerActive: boolean;
@ -57,15 +66,50 @@ interface FilterBarProps {
onPickedPoiLocationChange: (loc: { lat: number; lng: number } | null) => void; onPickedPoiLocationChange: (loc: { lat: number; lng: number } | null) => void;
currentMetric: Metric; currentMetric: Metric;
onTaskCreated?: (taskId: string) => void; onTaskCreated?: (taskId: string) => void;
/** Initial filter values (typically read from URL). Used once for defaultValues. */
initialValues?: ParameterValues;
/** Provides parent access to the form handle (e.g. for chip-remove resets). */
onFormReady?: (handle: FilterBarFormHandle) => void;
} }
// ── Helpers ── // ── Helpers ──
function formatPrice(v: number): string { /**
* Render a price as a compact label suitable for the chip-trigger button.
* For values below 10,000 we keep one decimal so a small change like 1500 -> 2000
* doesn't visually collide with the next-thousand value (e.g. 2k).
* Sub-1k values render as the raw integer.
*/
export function formatPrice(v: number): string {
if (v >= 1_000_000) return `\u00A3${(v / 1_000_000).toFixed(1)}M`; if (v >= 1_000_000) return `\u00A3${(v / 1_000_000).toFixed(1)}M`;
if (v >= 1_000) return `\u00A3${(v / 1_000).toFixed(0)}k`; if (v >= 10_000) return `\u00A3${(v / 1_000).toFixed(0)}k`;
if (v >= 1_000) {
const k = v / 1_000;
// Drop trailing .0 (so 2000 stays "2k", not "2.0k")
const label = Number.isInteger(k) ? `${k}` : k.toFixed(1);
return `\u00A3${label}k`;
}
return `\u00A3${v}`; return `\u00A3${v}`;
} }
/** Convert a ParameterValues to FormValues. ParameterValues may omit available_from; default to now. */
function toFormValues(values: ParameterValues): FormValues {
return {
listing_type: values.listing_type,
min_bedrooms: values.min_bedrooms,
max_bedrooms: values.max_bedrooms,
min_price: values.min_price,
max_price: values.max_price,
min_sqm: values.min_sqm,
max_sqm: values.max_sqm,
min_price_per_sqm: values.min_price_per_sqm,
max_price_per_sqm: values.max_price_per_sqm,
last_seen_days: values.last_seen_days,
available_from: values.available_from ?? new Date(),
district: values.district ?? '',
furnish_types: values.furnish_types,
};
}
/** Read current ParameterValues from the form state (merges metric and furnish) */ /** Read current ParameterValues from the form state (merges metric and furnish) */
function readFormParams( function readFormParams(
values: FormValues, values: FormValues,
@ -94,57 +138,84 @@ export function FilterBar({
onPickedPoiLocationChange, onPickedPoiLocationChange,
currentMetric, currentMetric,
onTaskCreated, onTaskCreated,
initialValues,
onFormReady,
}: FilterBarProps) { }: FilterBarProps) {
const [selectedFurnishTypes, setSelectedFurnishTypes] = useState<FurnishType[]>([]); const [selectedFurnishTypes, setSelectedFurnishTypes] = useState<FurnishType[]>(
initialValues?.furnish_types ?? [],
);
const [availableFromRawInput, setAvailableFromRawInput] = useState('now'); const [availableFromRawInput, setAvailableFromRawInput] = useState('now');
// Compute defaultValues ONCE from initialValues (or DEFAULT_FILTER_VALUES). React-Hook-Form
// does not re-read defaultValues after mount — use form.reset() for runtime updates.
const form = useForm<FormValues>({ const form = useForm<FormValues>({
resolver: zodResolver(formSchema), resolver: zodResolver(formSchema),
defaultValues: { defaultValues: initialValues
listing_type: DEFAULT_FILTER_VALUES.listing_type, ? toFormValues(initialValues)
min_bedrooms: DEFAULT_FILTER_VALUES.min_bedrooms, : {
max_bedrooms: DEFAULT_FILTER_VALUES.max_bedrooms, listing_type: DEFAULT_FILTER_VALUES.listing_type,
min_price: DEFAULT_FILTER_VALUES.min_price, min_bedrooms: DEFAULT_FILTER_VALUES.min_bedrooms,
max_price: DEFAULT_FILTER_VALUES.max_price, max_bedrooms: DEFAULT_FILTER_VALUES.max_bedrooms,
min_sqm: DEFAULT_FILTER_VALUES.min_sqm, min_price: DEFAULT_FILTER_VALUES.min_price,
max_sqm: undefined, max_price: DEFAULT_FILTER_VALUES.max_price,
min_price_per_sqm: undefined, min_sqm: DEFAULT_FILTER_VALUES.min_sqm,
max_price_per_sqm: undefined, max_sqm: undefined,
last_seen_days: DEFAULT_FILTER_VALUES.last_seen_days, min_price_per_sqm: undefined,
available_from: new Date(), max_price_per_sqm: undefined,
district: '', last_seen_days: DEFAULT_FILTER_VALUES.last_seen_days,
}, available_from: new Date(),
district: '',
},
}); });
const watchedListingType = form.watch('listing_type'); // ── Single source of truth for listing type ──
// The PARENT owns listingType; FilterBar mirrors it one-way.
// The header's BUY/RENT tabs call onListingTypeChange (parent setter), which causes a re-render
// with the new prop. We then push it into the form via the prop-driven effect below.
//
// We deliberately do NOT have a `form → parent` sync effect; instead any in-form control that
// wants to change listing_type calls onListingTypeChange directly (no internal listing_type tabs
// exist in FilterBar). This breaks the ping-pong loop that caused B2.
const previousListingTypeRef = useRef<ListingType>(form.getValues('listing_type'));
// Sync listing type with parent
useEffect(() => { useEffect(() => {
if (watchedListingType !== listingType) { if (form.getValues('listing_type') === listingType) return;
onListingTypeChange(watchedListingType);
}
}, [watchedListingType, listingType, onListingTypeChange]);
// Sync parent listing type changes back into form // Reflect parent listing type in the form.
useEffect(() => { form.setValue('listing_type', listingType, { shouldDirty: false });
if (listingType !== form.getValues('listing_type')) {
form.setValue('listing_type', listingType); // On a real transition (not the mount-time no-op above), reset price defaults
// and clear furnish (BUY only). This runs at most once per listing-type change.
if (previousListingTypeRef.current !== listingType) {
if (listingType === ListingType.BUY) {
form.setValue('min_price', 300000, { shouldDirty: false });
form.setValue('max_price', 600000, { shouldDirty: false });
setSelectedFurnishTypes([]);
} else {
form.setValue('min_price', 2000, { shouldDirty: false });
form.setValue('max_price', 3000, { shouldDirty: false });
}
previousListingTypeRef.current = listingType;
} }
}, [listingType, form]); }, [listingType, form]);
// Price defaults when listing type changes // Expose form handle to parent for chip-remove resets etc.
useEffect(() => { useEffect(() => {
if (watchedListingType === ListingType.BUY) { if (!onFormReady) return;
form.setValue('min_price', 300000); onFormReady({
form.setValue('max_price', 600000); reset: (values: ParameterValues) => {
} else { form.reset(toFormValues(values));
form.setValue('min_price', 2000); setSelectedFurnishTypes(values.furnish_types ?? []);
form.setValue('max_price', 3000); // Keep the local transition guard in sync so the next listingType prop change
} // doesn't trigger the price-defaults branch erroneously.
if (watchedListingType === ListingType.BUY) { previousListingTypeRef.current = values.listing_type;
setSelectedFurnishTypes([]); },
} form,
}, [watchedListingType, form]); });
}, [form, onFormReady]);
// `watchedListingType` is still used to drive conditional UI (e.g. furnish section, POI manager).
const watchedListingType = form.watch('listing_type');
const handleFormSubmit = useCallback( const handleFormSubmit = useCallback(
(action: 'fetch-data' | 'visualize') => { (action: 'fetch-data' | 'visualize') => {
@ -417,6 +488,7 @@ export function FilterBar({
<FormControl> <FormControl>
<Input <Input
type="number" type="number"
min={0}
placeholder="28" placeholder="28"
className="h-8 text-sm" className="h-8 text-sm"
{...field} {...field}

View file

@ -6,6 +6,8 @@ interface FilterChipsProps {
values: ParameterValues; values: ParameterValues;
defaults: ParameterValues; defaults: ParameterValues;
onRemove: (key: keyof ParameterValues) => void; onRemove: (key: keyof ParameterValues) => void;
/** Optional handler for the "Reset all" affordance — clears every active filter to defaults. */
onResetAll?: () => void;
} }
/** Format a price value for display */ /** Format a price value for display */
@ -98,13 +100,13 @@ function buildChips(values: ParameterValues, defaults: ParameterValues): ChipDef
return chips; return chips;
} }
export function FilterChips({ values, defaults, onRemove }: FilterChipsProps) { export function FilterChips({ values, defaults, onRemove, onResetAll }: FilterChipsProps) {
const chips = buildChips(values, defaults); const chips = buildChips(values, defaults);
if (chips.length === 0) return null; if (chips.length === 0) return null;
return ( return (
<div className="flex flex-wrap gap-1.5 px-4 py-1.5 border-b bg-muted/20"> <div className="flex flex-wrap items-center gap-1.5 px-4 py-1.5 border-b bg-muted/20">
{chips.map((chip) => ( {chips.map((chip) => (
<span <span
key={chip.key} key={chip.key}
@ -121,6 +123,16 @@ export function FilterChips({ values, defaults, onRemove }: FilterChipsProps) {
</button> </button>
</span> </span>
))} ))}
{onResetAll && chips.length > 0 && (
<button
type="button"
onClick={onResetAll}
className="text-xs text-muted-foreground hover:text-foreground underline underline-offset-2 px-1.5 py-0.5"
aria-label="Reset all filters"
>
Reset all
</button>
)}
</div> </div>
); );
} }

View file

@ -1,5 +1,5 @@
import { zodResolver } from "@hookform/resolvers/zod"; import { zodResolver } from "@hookform/resolvers/zod";
import { useState, useEffect } from "react"; import { useState, useEffect, useRef } from "react";
import { useForm } from "react-hook-form"; import { useForm } from "react-hook-form";
import { z } from "zod"; import { z } from "zod";
import { Button } from "./ui/button"; import { Button } from "./ui/button";
@ -82,6 +82,8 @@ interface FilterPanelProps {
userPOIs?: POI[]; userPOIs?: POI[];
poiTravelFilters?: Record<number, POITravelFilter>; poiTravelFilters?: Record<number, POITravelFilter>;
onPoiTravelFiltersChange?: (filters: Record<number, POITravelFilter>) => void; onPoiTravelFiltersChange?: (filters: Record<number, POITravelFilter>) => void;
/** Initial filter values (typically read from URL). Used once for defaultValues. */
initialValues?: ParameterValues;
} }
const formSchema = z.object({ const formSchema = z.object({
@ -110,45 +112,64 @@ const PRICE_BOUNDS = {
const BEDROOM_BOUNDS = { min: 0, max: 10, step: 1 } as const; const BEDROOM_BOUNDS = { min: 0, max: 10, step: 1 } as const;
export function FilterPanel({ onSubmit, currentMetric, isLoading, listingCount, user, onTaskCreated, onStartPoiPicking, pickedPoiLocation, userPOIs, poiTravelFilters, onPoiTravelFiltersChange }: FilterPanelProps) { export function FilterPanel({ onSubmit, currentMetric, isLoading, listingCount, user, onTaskCreated, onStartPoiPicking, pickedPoiLocation, userPOIs, poiTravelFilters, onPoiTravelFiltersChange, initialValues }: FilterPanelProps) {
const [availableFromRawInput, setAvailableFromRawInput] = useState("now"); const [availableFromRawInput, setAvailableFromRawInput] = useState("now");
const [selectedFurnishTypes, setSelectedFurnishTypes] = useState<FurnishType[]>([]); const [selectedFurnishTypes, setSelectedFurnishTypes] = useState<FurnishType[]>(
initialValues?.furnish_types ?? [],
);
const form = useForm<FormValues>({ const form = useForm<FormValues>({
resolver: zodResolver(formSchema), resolver: zodResolver(formSchema),
defaultValues: { defaultValues: initialValues
listing_type: DEFAULT_FILTER_VALUES.listing_type, ? {
min_bedrooms: DEFAULT_FILTER_VALUES.min_bedrooms, listing_type: initialValues.listing_type,
max_bedrooms: DEFAULT_FILTER_VALUES.max_bedrooms, min_bedrooms: initialValues.min_bedrooms,
min_price: DEFAULT_FILTER_VALUES.min_price, max_bedrooms: initialValues.max_bedrooms,
max_price: DEFAULT_FILTER_VALUES.max_price, min_price: initialValues.min_price,
min_sqm: DEFAULT_FILTER_VALUES.min_sqm, max_price: initialValues.max_price,
max_sqm: undefined, min_sqm: initialValues.min_sqm,
min_price_per_sqm: undefined, max_sqm: initialValues.max_sqm,
max_price_per_sqm: undefined, min_price_per_sqm: initialValues.min_price_per_sqm,
last_seen_days: DEFAULT_FILTER_VALUES.last_seen_days, max_price_per_sqm: initialValues.max_price_per_sqm,
available_from: new Date(), last_seen_days: initialValues.last_seen_days,
district: '', available_from: initialValues.available_from ?? new Date(),
}, district: initialValues.district ?? '',
furnish_types: initialValues.furnish_types,
}
: {
listing_type: DEFAULT_FILTER_VALUES.listing_type,
min_bedrooms: DEFAULT_FILTER_VALUES.min_bedrooms,
max_bedrooms: DEFAULT_FILTER_VALUES.max_bedrooms,
min_price: DEFAULT_FILTER_VALUES.min_price,
max_price: DEFAULT_FILTER_VALUES.max_price,
min_sqm: DEFAULT_FILTER_VALUES.min_sqm,
max_sqm: undefined,
min_price_per_sqm: undefined,
max_price_per_sqm: undefined,
last_seen_days: DEFAULT_FILTER_VALUES.last_seen_days,
available_from: new Date(),
district: '',
},
}); });
// Watch listing_type to make filters type-aware // Watch listing_type to make filters type-aware
const watchedListingType = form.watch('listing_type'); const watchedListingType = form.watch('listing_type');
const priceBounds = PRICE_BOUNDS[watchedListingType]; const priceBounds = PRICE_BOUNDS[watchedListingType];
// Update price defaults when listing type changes // Update price defaults ONLY on real listing-type transitions, not on every render.
// Use a ref to detect actual changes (mount-time render is a no-op).
const previousListingTypeRef = useRef<ListingType>(watchedListingType);
useEffect(() => { useEffect(() => {
if (watchedListingType === previousListingTypeRef.current) return;
previousListingTypeRef.current = watchedListingType;
if (watchedListingType === ListingType.BUY) { if (watchedListingType === ListingType.BUY) {
form.setValue('min_price', 300000); form.setValue('min_price', 300000);
form.setValue('max_price', 600000); form.setValue('max_price', 600000);
setSelectedFurnishTypes([]);
} else { } else {
form.setValue('min_price', 2000); form.setValue('min_price', 2000);
form.setValue('max_price', 3000); form.setValue('max_price', 3000);
} }
// Clear furnish types when switching to BUY
if (watchedListingType === ListingType.BUY) {
setSelectedFurnishTypes([]);
}
}, [watchedListingType, form]); }, [watchedListingType, form]);
const handleFormSubmit = (action: 'fetch-data' | 'visualize') => { const handleFormSubmit = (action: 'fetch-data' | 'visualize') => {
@ -451,6 +472,7 @@ export function FilterPanel({ onSubmit, currentMetric, isLoading, listingCount,
<FormControl> <FormControl>
<Input <Input
type="number" type="number"
min={0}
placeholder="28" placeholder="28"
className="h-8 text-sm" className="h-8 text-sm"
{...field} {...field}

View file

@ -156,11 +156,15 @@ export function ListingDetail({ detail, onDecide, onClearDecision }: ListingDeta
</div> </div>
)} )}
{/* Agency */} {/* Agency wrapped in a labelled block so the Overview tab doesn't
collapse to just "Foxtons" when description/key_features/floorplans are empty. */}
{detail.agency && ( {detail.agency && (
<div className="flex items-center gap-2 text-sm text-muted-foreground"> <div>
<Building className="h-4 w-4" /> <h3 className="text-sm font-semibold mb-2">Listed by</h3>
<span>{detail.agency}</span> <div className="flex items-center gap-2 text-sm text-muted-foreground">
<Building className="h-4 w-4" />
<span>{detail.agency}</span>
</div>
</div> </div>
)} )}

View file

@ -47,6 +47,9 @@ export function ListingDetailSheet({
<Drawer.Overlay className="fixed inset-0 bg-black/40 z-50" /> <Drawer.Overlay className="fixed inset-0 bg-black/40 z-50" />
<Drawer.Content className="fixed bottom-0 left-0 right-0 z-50 flex flex-col bg-background rounded-t-xl max-h-[90vh] sm:!max-w-2xl sm:mx-auto"> <Drawer.Content className="fixed bottom-0 left-0 right-0 z-50 flex flex-col bg-background rounded-t-xl max-h-[90vh] sm:!max-w-2xl sm:mx-auto">
<Drawer.Title className="sr-only">Listing Details</Drawer.Title> <Drawer.Title className="sr-only">Listing Details</Drawer.Title>
<Drawer.Description className="sr-only">
Property details including price, location, photos, travel times, and price history.
</Drawer.Description>
<div className="mx-auto w-12 h-1.5 flex-shrink-0 rounded-full bg-muted-foreground/20 my-3" /> <div className="mx-auto w-12 h-1.5 flex-shrink-0 rounded-full bg-muted-foreground/20 my-3" />
<div className="overflow-y-auto flex-1"> <div className="overflow-y-auto flex-1">
{isLoading && ( {isLoading && (

View file

@ -85,11 +85,18 @@ export function Map(props: MapProps) {
// Pass all features to the heatmap — filtering is done server-side // Pass all features to the heatmap — filtering is done server-side
heatmap.setData(data); heatmap.setData(data);
// Compute color scale in worker (sorts + percentiles off main thread) // Compute color scale in worker (sorts + percentiles off main thread).
// If the client is torn down mid-flight (rapid view-mode switch), the worker
// promise rejects with "HexgridHeatmapClient destroyed" — silently swallow
// that case rather than letting it surface as a pageerror (Bug B11).
const colorResult = await heatmap.computeColorScale(metricMode, { const colorResult = await heatmap.computeColorScale(metricMode, {
minBound: PERCENTILE_CONFIG.MIN_BOUND, minBound: PERCENTILE_CONFIG.MIN_BOUND,
maxBound: PERCENTILE_CONFIG.MAX_BOUND, maxBound: PERCENTILE_CONFIG.MAX_BOUND,
}); }).catch(() => null);
// If the heatmap was destroyed (different ref) or the promise was cancelled,
// bail out — the component is unmounting or recreating.
if (!colorResult || heatmapRef.current !== heatmap) return;
if (colorResult.hasValues) { if (colorResult.hasValues) {
makeLegend(colorScheme, colorResult.min, colorResult.max); makeLegend(colorScheme, colorResult.min, colorResult.max);
@ -111,20 +118,28 @@ export function Map(props: MapProps) {
const boundsResult = await heatmap.computeBounds({ const boundsResult = await heatmap.computeBounds({
clipMin: PERCENTILE_CONFIG.BOUNDS_CLIP_MIN, clipMin: PERCENTILE_CONFIG.BOUNDS_CLIP_MIN,
clipMax: PERCENTILE_CONFIG.BOUNDS_CLIP_MAX, clipMax: PERCENTILE_CONFIG.BOUNDS_CLIP_MAX,
}); }).catch(() => null);
mapRef.current?.fitBounds([ if (boundsResult && heatmapRef.current === heatmap) {
[boundsResult.minLng, boundsResult.minLat], mapRef.current?.fitBounds([
[boundsResult.maxLng, boundsResult.maxLat] [boundsResult.minLng, boundsResult.minLat],
], { duration: 0 }); [boundsResult.maxLng, boundsResult.maxLat]
], { duration: 0 });
}
} }
lastDataLengthRef.current = data.features.length; lastDataLengthRef.current = data.features.length;
}, [data, metricMode, colorScheme]); }, [data, metricMode, colorScheme]);
// Track whether the Mapbox token is configured. When missing we render a banner
// inside the map container instead of letting Mapbox 404 on the style request
// (B19/B29).
const isMapboxTokenMissing = !MAP_CONFIG.MAPBOX_TOKEN;
// Initialize map // Initialize map
useEffect(() => { useEffect(() => {
if (!mapContainerRef.current) return; if (!mapContainerRef.current) return;
if (isMapboxTokenMissing) return;
mapboxgl.accessToken = MAP_CONFIG.MAPBOX_TOKEN; mapboxgl.accessToken = MAP_CONFIG.MAPBOX_TOKEN;
mapRef.current = new mapboxgl.Map({ mapRef.current = new mapboxgl.Map({
@ -199,7 +214,7 @@ export function Map(props: MapProps) {
mapRef.current?.remove(); mapRef.current?.remove();
}; };
// eslint-disable-next-line react-hooks/exhaustive-deps // eslint-disable-next-line react-hooks/exhaustive-deps
}, []); }, [isMapboxTokenMissing]);
// Debounced update effect - only update after 200ms of no changes // Debounced update effect - only update after 200ms of no changes
useEffect(() => { useEffect(() => {
@ -417,6 +432,15 @@ export function Map(props: MapProps) {
return ( return (
<div className="relative w-full h-full"> <div className="relative w-full h-full">
<div id='map-container' ref={mapContainerRef}></div> <div id='map-container' ref={mapContainerRef}></div>
{isMapboxTokenMissing && (
<div
role="alert"
className="absolute inset-0 z-20 flex items-center justify-center bg-muted/95 text-muted-foreground text-sm font-medium p-4 text-center"
data-testid="mapbox-token-missing-banner"
>
Map unavailable &mdash; set <code className="font-mono text-xs bg-background px-1 py-0.5 rounded">VITE_MAPBOX_TOKEN</code> to enable the basemap.
</div>
)}
{props.isPickingPOI && ( {props.isPickingPOI && (
<div className="absolute top-3 left-1/2 -translate-x-1/2 z-10 bg-primary text-primary-foreground px-4 py-2 rounded-lg shadow-lg flex items-center gap-3 text-sm font-medium"> <div className="absolute top-3 left-1/2 -translate-x-1/2 z-10 bg-primary text-primary-foreground px-4 py-2 rounded-lg shadow-lg flex items-center gap-3 text-sm font-medium">
<Crosshair className="h-4 w-4" /> <Crosshair className="h-4 w-4" />

View file

@ -81,6 +81,9 @@ export function MobileBottomSheet({
style={{ maxHeight: '85vh' }} style={{ maxHeight: '85vh' }}
> >
<Drawer.Title className="sr-only">Property Listings</Drawer.Title> <Drawer.Title className="sr-only">Property Listings</Drawer.Title>
<Drawer.Description className="sr-only">
Swipeable list of matching properties. Drag the handle to expand or collapse the sheet.
</Drawer.Description>
{/* Drag handle */} {/* Drag handle */}
<div className="flex justify-center pt-2 pb-1"> <div className="flex justify-center pt-2 pb-1">
<div className="h-1.5 w-10 rounded-full bg-muted-foreground/30" /> <div className="h-1.5 w-10 rounded-full bg-muted-foreground/30" />

View file

@ -1,6 +1,6 @@
import { useState, useCallback, useEffect } from 'react'; import { useState, useCallback, useEffect } from 'react';
import useEmblaCarousel from 'embla-carousel-react'; import useEmblaCarousel from 'embla-carousel-react';
import { ChevronLeft, ChevronRight } from 'lucide-react'; import { ChevronLeft, ChevronRight, ImageOff } from 'lucide-react';
import type { ListingDetailPhoto } from '@/types'; import type { ListingDetailPhoto } from '@/types';
interface PhotoCarouselProps { interface PhotoCarouselProps {
@ -8,8 +8,13 @@ interface PhotoCarouselProps {
} }
export function PhotoCarousel({ photos }: PhotoCarouselProps) { export function PhotoCarousel({ photos }: PhotoCarouselProps) {
const [emblaRef, emblaApi] = useEmblaCarousel({ loop: true }); // Only enable Embla loop when there are multiple photos. Looping on a single
// image lets the user drag it off-screen and reveal it sliding back (B26).
const hasMultiple = photos.length > 1;
const [emblaRef, emblaApi] = useEmblaCarousel({ loop: hasMultiple });
const [selectedIndex, setSelectedIndex] = useState(0); const [selectedIndex, setSelectedIndex] = useState(0);
// Track which photo URLs failed so we can swap them for a placeholder (B23).
const [brokenIndexes, setBrokenIndexes] = useState<Set<number>>(() => new Set());
const onSelect = useCallback(() => { const onSelect = useCallback(() => {
if (!emblaApi) return; if (!emblaApi) return;
@ -22,6 +27,39 @@ export function PhotoCarousel({ photos }: PhotoCarouselProps) {
return () => { emblaApi.off('select', onSelect); }; return () => { emblaApi.off('select', onSelect); };
}, [emblaApi, onSelect]); }, [emblaApi, onSelect]);
// Keyboard navigation: ArrowLeft / ArrowRight advance the active slide (B16).
// Embla doesn't ship a built-in keyboard plugin in this repo's deps, so we
// attach a focus-scoped listener instead of registering a global window key.
useEffect(() => {
if (!emblaApi || !hasMultiple) return;
const root = emblaApi.rootNode();
if (!root) return;
// Make the carousel focusable so it can receive key events.
if (root.tabIndex === -1) {
root.tabIndex = 0;
}
const handleKey = (e: KeyboardEvent) => {
if (e.key === 'ArrowLeft') {
e.preventDefault();
emblaApi.scrollPrev();
} else if (e.key === 'ArrowRight') {
e.preventDefault();
emblaApi.scrollNext();
}
};
root.addEventListener('keydown', handleKey);
return () => { root.removeEventListener('keydown', handleKey); };
}, [emblaApi, hasMultiple]);
const handleImgError = useCallback((i: number) => {
setBrokenIndexes((prev) => {
if (prev.has(i)) return prev;
const next = new Set(prev);
next.add(i);
return next;
});
}, []);
if (photos.length === 0) { if (photos.length === 0) {
return ( return (
<div className="w-full h-48 bg-muted flex items-center justify-center text-muted-foreground"> <div className="w-full h-48 bg-muted flex items-center justify-center text-muted-foreground">
@ -32,22 +70,35 @@ export function PhotoCarousel({ photos }: PhotoCarouselProps) {
return ( return (
<div className="relative"> <div className="relative">
<div className="overflow-hidden" ref={emblaRef}> <div
className="overflow-hidden focus:outline-none focus-visible:ring-2 focus-visible:ring-primary"
ref={emblaRef}
aria-label="Property photos"
role="region"
>
<div className="flex"> <div className="flex">
{photos.map((photo, i) => ( {photos.map((photo, i) => (
<div key={i} className="flex-[0_0_100%] min-w-0"> <div key={i} className="flex-[0_0_100%] min-w-0">
<img {brokenIndexes.has(i) ? (
src={photo.url} <div className="w-full h-64 bg-muted flex flex-col items-center justify-center text-muted-foreground gap-1">
alt={photo.caption || `Photo ${i + 1}`} <ImageOff className="w-8 h-8" />
className="w-full h-64 object-cover" <span className="text-xs">Photo unavailable</span>
loading="lazy" </div>
/> ) : (
<img
src={photo.url}
alt={photo.caption || `Photo ${i + 1}`}
className="w-full h-64 object-cover"
loading="lazy"
onError={() => handleImgError(i)}
/>
)}
</div> </div>
))} ))}
</div> </div>
</div> </div>
{/* Prev/Next arrows */} {/* Prev/Next arrows */}
{photos.length > 1 && ( {hasMultiple && (
<> <>
<button <button
className="absolute left-1 top-1/2 -translate-y-1/2 bg-black/50 hover:bg-black/70 text-white rounded-full p-1 transition-colors" className="absolute left-1 top-1/2 -translate-y-1/2 bg-black/50 hover:bg-black/70 text-white rounded-full p-1 transition-colors"
@ -65,12 +116,14 @@ export function PhotoCarousel({ photos }: PhotoCarouselProps) {
</button> </button>
</> </>
)} )}
{/* Counter */} {/* Counter (suppressed when there's only one photo — B23) */}
<div className="absolute bottom-2 right-2 bg-black/60 text-white text-xs px-2 py-1 rounded"> {hasMultiple && (
{selectedIndex + 1} / {photos.length} <div className="absolute bottom-2 right-2 bg-black/60 text-white text-xs px-2 py-1 rounded">
</div> {selectedIndex + 1} / {photos.length}
</div>
)}
{/* Dots */} {/* Dots */}
{photos.length > 1 && photos.length <= 20 && ( {hasMultiple && photos.length <= 20 && (
<div className="flex justify-center gap-1 mt-2"> <div className="flex justify-center gap-1 mt-2">
{photos.map((_, i) => ( {photos.map((_, i) => (
<button <button
@ -79,6 +132,7 @@ export function PhotoCarousel({ photos }: PhotoCarouselProps) {
i === selectedIndex ? 'bg-primary' : 'bg-muted-foreground/30' i === selectedIndex ? 'bg-primary' : 'bg-muted-foreground/30'
}`} }`}
onClick={() => emblaApi?.scrollTo(i)} onClick={() => emblaApi?.scrollTo(i)}
aria-label={`Go to photo ${i + 1}`}
/> />
))} ))}
</div> </div>

View file

@ -2,7 +2,7 @@ import { useState, useCallback, useEffect } from 'react';
import useEmblaCarousel from 'embla-carousel-react'; import useEmblaCarousel from 'embla-carousel-react';
import { ExternalLink, Footprints, Bike, Train } from 'lucide-react'; import { ExternalLink, Footprints, Bike, Train } from 'lucide-react';
import type { PropertyProperties, POIDistanceInfo, POI } from '@/types'; import type { PropertyProperties, POIDistanceInfo, POI } from '@/types';
import { formatDuration } from '@/utils/format'; import { formatDuration, formatPrice, formatInteger, formatPricePerSqmShort, isFiniteNumber, EM_DASH } from '@/utils/format';
function TravelModeIcon({ mode }: { mode: string }) { function TravelModeIcon({ mode }: { mode: string }) {
switch (mode) { switch (mode) {
@ -13,15 +13,16 @@ function TravelModeIcon({ mode }: { mode: string }) {
} }
} }
const TRAVEL_MODES: Array<'WALK' | 'BICYCLE' | 'TRANSIT'> = ['WALK', 'BICYCLE', 'TRANSIT'];
function POIDistanceBadges({ distances }: { distances: POIDistanceInfo[] }) { function POIDistanceBadges({ distances }: { distances: POIDistanceInfo[] }) {
if (!distances || distances.length === 0) return null; if (!distances || distances.length === 0) return null;
// Group by POI name // Group by POI name, indexing by travel_mode for consistent rendering.
const byPoi = new Map<string, POIDistanceInfo[]>(); const byPoi = new Map<string, Map<string, POIDistanceInfo>>();
for (const d of distances) { for (const d of distances) {
const existing = byPoi.get(d.poi_name) || []; if (!byPoi.has(d.poi_name)) byPoi.set(d.poi_name, new Map());
existing.push(d); byPoi.get(d.poi_name)!.set(d.travel_mode, d);
byPoi.set(d.poi_name, existing);
} }
return ( return (
@ -29,20 +30,21 @@ function POIDistanceBadges({ distances }: { distances: POIDistanceInfo[] }) {
{Array.from(byPoi.entries()).map(([poiName, dists]) => ( {Array.from(byPoi.entries()).map(([poiName, dists]) => (
<div key={poiName} className="inline-flex items-center gap-1 text-xs text-muted-foreground"> <div key={poiName} className="inline-flex items-center gap-1 text-xs text-muted-foreground">
<span className="font-medium">{poiName}:</span> <span className="font-medium">{poiName}:</span>
{dists.map(d => ( {TRAVEL_MODES.map(mode => {
<span key={d.travel_mode} className="inline-flex items-center gap-0.5" title={`${d.travel_mode} to ${poiName}`}> const d = dists.get(mode);
<TravelModeIcon mode={d.travel_mode} /> return (
{formatDuration(d.duration_seconds)} <span key={mode} className="inline-flex items-center gap-0.5" title={`${mode} to ${poiName}`}>
</span> <TravelModeIcon mode={mode} />
))} {d ? formatDuration(d.duration_seconds) : EM_DASH}
</span>
);
})}
</div> </div>
))} ))}
</div> </div>
); );
} }
const TRAVEL_MODES: Array<'WALK' | 'BICYCLE' | 'TRANSIT'> = ['WALK', 'BICYCLE', 'TRANSIT'];
function AllPOIDistances({ pois, distances }: { pois: POI[]; distances?: POIDistanceInfo[] }) { function AllPOIDistances({ pois, distances }: { pois: POI[]; distances?: POIDistanceInfo[] }) {
// Index distances by poi_id + travel_mode for O(1) lookup // Index distances by poi_id + travel_mode for O(1) lookup
const distMap = new Map<string, POIDistanceInfo>(); const distMap = new Map<string, POIDistanceInfo>();
@ -73,7 +75,10 @@ function AllPOIDistances({ pois, distances }: { pois: POI[]; distances?: POIDist
} }
function CardCarousel({ photos, altText }: { photos: string[]; altText?: string }) { function CardCarousel({ photos, altText }: { photos: string[]; altText?: string }) {
const [emblaRef, emblaApi] = useEmblaCarousel({ loop: true }); // Only loop when there's more than one image (single-image carousels should
// be static — mirrors PhotoCarousel B26).
const hasMultiple = photos.length > 1;
const [emblaRef, emblaApi] = useEmblaCarousel({ loop: hasMultiple });
const [selectedIndex, setSelectedIndex] = useState(0); const [selectedIndex, setSelectedIndex] = useState(0);
const onSelect = useCallback(() => { const onSelect = useCallback(() => {
@ -87,7 +92,29 @@ function CardCarousel({ photos, altText }: { photos: string[]; altText?: string
return () => { emblaApi.off('select', onSelect); }; return () => { emblaApi.off('select', onSelect); };
}, [emblaApi, onSelect]); }, [emblaApi, onSelect]);
if (photos.length <= 1) { // Keyboard nav for the card carousel (B16). Listener is scoped to the
// embla root so it only fires when the user focuses this carousel.
useEffect(() => {
if (!emblaApi || !hasMultiple) return;
const root = emblaApi.rootNode();
if (!root) return;
if (root.tabIndex === -1) {
root.tabIndex = 0;
}
const handleKey = (e: KeyboardEvent) => {
if (e.key === 'ArrowLeft') {
e.preventDefault();
emblaApi.scrollPrev();
} else if (e.key === 'ArrowRight') {
e.preventDefault();
emblaApi.scrollNext();
}
};
root.addEventListener('keydown', handleKey);
return () => { root.removeEventListener('keydown', handleKey); };
}, [emblaApi, hasMultiple]);
if (!hasMultiple) {
return ( return (
<img <img
src={photos[0]} src={photos[0]}
@ -100,7 +127,12 @@ function CardCarousel({ photos, altText }: { photos: string[]; altText?: string
return ( return (
<div className="relative w-full h-full" onClick={e => e.stopPropagation()}> <div className="relative w-full h-full" onClick={e => e.stopPropagation()}>
<div className="overflow-hidden h-full" ref={emblaRef}> <div
className="overflow-hidden h-full focus:outline-none focus-visible:ring-2 focus-visible:ring-primary"
ref={emblaRef}
aria-label="Property photos"
role="region"
>
<div className="flex h-full"> <div className="flex h-full">
{photos.map((url, i) => ( {photos.map((url, i) => (
<div key={i} className="flex-[0_0_100%] min-w-0 h-full"> <div key={i} className="flex-[0_0_100%] min-w-0 h-full">
@ -145,22 +177,28 @@ export function PropertyCard({
allPOIs, allPOIs,
onClick, onClick,
}: PropertyCardProps) { }: PropertyCardProps) {
// BUY listings may have null numeric / date fields; coerce so renders don't throw. // BUY listings may have null numeric / date fields; render "—" at the JSX leaf
// when the source is null/undefined/non-finite so the user can't mistake a missing
// value for a real £0 / 0 m².
const lastSeenRaw = property.last_seen; const lastSeenRaw = property.last_seen;
const lastSeenDate = typeof lastSeenRaw === 'string' ? lastSeenRaw.split('T')[0] : null; const lastSeenDate = typeof lastSeenRaw === 'string' ? lastSeenRaw.split('T')[0] : null;
const lastSeenTime = lastSeenDate ? new Date(lastSeenDate).getTime() : NaN; const lastSeenTime = lastSeenDate ? new Date(lastSeenDate).getTime() : NaN;
const lastSeenDays = Number.isFinite(lastSeenTime) const lastSeenDaysRaw = Number.isFinite(lastSeenTime)
? Math.round((Date.now() - lastSeenTime) / (1000 * 60 * 60 * 24)) ? Math.round((Date.now() - lastSeenTime) / (1000 * 60 * 60 * 24))
: null; : null;
// Clamp future timestamps to 0 so we don't render "-7d ago" for stale BUY rows.
const lastSeenDays = lastSeenDaysRaw !== null ? Math.max(0, lastSeenDaysRaw) : null;
// Coerced numerics used only where a number is structurally required (alt text,
// boolean comparisons). All visible numeric leaves use the format helpers.
const safeNum = (v: unknown): number => (typeof v === 'number' && Number.isFinite(v) ? v : 0); const safeNum = (v: unknown): number => (typeof v === 'number' && Number.isFinite(v) ? v : 0);
const safeTotalPrice = safeNum(property.total_price); const safeTotalPrice = safeNum(property.total_price);
const safeQm = safeNum(property.qm); const safeQm = safeNum(property.qm);
const safeQmprice = safeNum(property.qmprice);
const safeRooms = safeNum(property.rooms); const safeRooms = safeNum(property.rooms);
// Determine if this is a good deal // Determine if this is a good deal (guard requires a finite qmprice > 0)
const isGoodDeal = avgPricePerSqm && property.qmprice > 0 && property.qmprice < avgPricePerSqm * 0.9; const qmpriceForCompare = isFiniteNumber(property.qmprice) ? property.qmprice : null;
const isExpensive = avgPricePerSqm && property.qmprice > avgPricePerSqm * 1.1; const isGoodDeal = avgPricePerSqm && qmpriceForCompare !== null && qmpriceForCompare > 0 && qmpriceForCompare < avgPricePerSqm * 0.9;
const isExpensive = avgPricePerSqm && qmpriceForCompare !== null && qmpriceForCompare > avgPricePerSqm * 1.1;
const priceIndicator = isGoodDeal const priceIndicator = isGoodDeal
? { dotColor: 'bg-[var(--deal-good)]', label: 'Good deal' } ? { dotColor: 'bg-[var(--deal-good)]', label: 'Good deal' }
@ -195,8 +233,8 @@ export function PropertyCard({
{/* Price */} {/* Price */}
<div className="flex items-center gap-1.5"> <div className="flex items-center gap-1.5">
<span className="text-lg font-bold tracking-tight"> <span className="text-lg font-bold tracking-tight">
£{safeTotalPrice.toLocaleString()} {formatPrice(property.total_price)}
{property.listing_type !== 'BUY' && ( {property.listing_type !== 'BUY' && isFiniteNumber(property.total_price) && (
<span className="text-muted-foreground font-normal text-sm">/mo</span> <span className="text-muted-foreground font-normal text-sm">/mo</span>
)} )}
</span> </span>
@ -210,11 +248,11 @@ export function PropertyCard({
{/* Key metrics on one line */} {/* Key metrics on one line */}
<div className="flex items-center gap-1 text-sm text-muted-foreground mt-0.5"> <div className="flex items-center gap-1 text-sm text-muted-foreground mt-0.5">
<span>{safeRooms}</span><span>bed</span> <span>{formatInteger(property.rooms)}</span><span>bed</span>
<span>·</span> <span>·</span>
<span>{safeQm} m²</span> <span>{formatInteger(property.qm)} m²</span>
<span>·</span> <span>·</span>
<span>£{safeQmprice}/m²</span> <span>{formatPricePerSqmShort(property.qmprice)}</span>
</div> </div>
{/* Agency + freshness */} {/* Agency + freshness */}
@ -271,8 +309,8 @@ export function PropertyCard({
{/* Price as dominant element */} {/* Price as dominant element */}
<div className="flex items-center gap-1.5"> <div className="flex items-center gap-1.5">
<span className="text-lg font-bold tracking-tight"> <span className="text-lg font-bold tracking-tight">
£{safeTotalPrice.toLocaleString()} {formatPrice(property.total_price)}
{property.listing_type !== 'BUY' && ( {property.listing_type !== 'BUY' && isFiniteNumber(property.total_price) && (
<span className="text-muted-foreground font-normal text-sm">/mo</span> <span className="text-muted-foreground font-normal text-sm">/mo</span>
)} )}
</span> </span>
@ -286,11 +324,11 @@ export function PropertyCard({
{/* Key metrics on one line */} {/* Key metrics on one line */}
<div className="flex items-center gap-1 text-sm text-muted-foreground"> <div className="flex items-center gap-1 text-sm text-muted-foreground">
<span>{safeRooms}</span><span>bed</span> <span>{formatInteger(property.rooms)}</span><span>bed</span>
<span>·</span> <span>·</span>
<span>{safeQm} m²</span> <span>{formatInteger(property.qm)} m²</span>
<span>·</span> <span>·</span>
<span>£{safeQmprice}/m²</span> <span>{formatPricePerSqmShort(property.qmprice)}</span>
</div> </div>
{/* Location */} {/* Location */}

View file

@ -1,5 +1,6 @@
import { Bed, MapPin } from 'lucide-react'; import { Bed, MapPin } from 'lucide-react';
import type { PropertyProperties } from '@/types'; import type { PropertyProperties } from '@/types';
import { formatPrice, formatInteger, isFiniteNumber } from '@/utils/format';
interface PropertyCardCompactProps { interface PropertyCardCompactProps {
property: PropertyProperties; property: PropertyProperties;
@ -16,15 +17,15 @@ export function PropertyCardCompact({
avgPricePerSqm, avgPricePerSqm,
onClick, onClick,
}: PropertyCardCompactProps) { }: PropertyCardCompactProps) {
// BUY listings may have null numeric fields; coerce so renders don't throw. // BUY listings may have null numeric fields; render "—" at the JSX leaf for
// missing values rather than coercing to 0 (which renders as "£0 / 0 m²").
const safeNum = (v: unknown): number => (typeof v === 'number' && Number.isFinite(v) ? v : 0); const safeNum = (v: unknown): number => (typeof v === 'number' && Number.isFinite(v) ? v : 0);
const safeTotalPrice = safeNum(property.total_price); const safeTotalPrice = safeNum(property.total_price);
const safeQm = safeNum(property.qm);
const safeQmprice = safeNum(property.qmprice);
const safeRooms = safeNum(property.rooms); const safeRooms = safeNum(property.rooms);
const isGoodDeal = avgPricePerSqm && safeQmprice > 0 && safeQmprice < avgPricePerSqm * 0.9; const qmpriceForCompare = isFiniteNumber(property.qmprice) ? property.qmprice : null;
const isExpensive = avgPricePerSqm && safeQmprice > avgPricePerSqm * 1.1; const isGoodDeal = avgPricePerSqm && qmpriceForCompare !== null && qmpriceForCompare > 0 && qmpriceForCompare < avgPricePerSqm * 0.9;
const isExpensive = avgPricePerSqm && qmpriceForCompare !== null && qmpriceForCompare > avgPricePerSqm * 1.1;
const priceIndicator = isGoodDeal const priceIndicator = isGoodDeal
? { dotColor: 'bg-[var(--deal-good)]', label: 'Good deal' } ? { dotColor: 'bg-[var(--deal-good)]', label: 'Good deal' }
@ -55,8 +56,8 @@ export function PropertyCardCompact({
{/* Price bold */} {/* Price bold */}
<div className="flex items-center gap-1.5"> <div className="flex items-center gap-1.5">
<span className="font-bold text-base"> <span className="font-bold text-base">
£{safeTotalPrice.toLocaleString()} {formatPrice(property.total_price)}
{property.listing_type !== 'BUY' && ( {property.listing_type !== 'BUY' && isFiniteNumber(property.total_price) && (
<span className="text-muted-foreground font-normal text-sm">/mo</span> <span className="text-muted-foreground font-normal text-sm">/mo</span>
)} )}
</span> </span>
@ -69,10 +70,10 @@ export function PropertyCardCompact({
<div className="flex items-center gap-2 text-sm text-muted-foreground"> <div className="flex items-center gap-2 text-sm text-muted-foreground">
<span className="flex items-center gap-1"> <span className="flex items-center gap-1">
<Bed className="h-3.5 w-3.5" /> <Bed className="h-3.5 w-3.5" />
{safeRooms} bed {formatInteger(property.rooms)} bed
</span> </span>
<span>·</span> <span>·</span>
<span>{safeQm} m²</span> <span>{formatInteger(property.qm)} m²</span>
</div> </div>
{/* Location */} {/* Location */}

View file

@ -4,6 +4,7 @@ import { useDrag } from '@use-gesture/react';
import useEmblaCarousel from 'embla-carousel-react'; import useEmblaCarousel from 'embla-carousel-react';
import { Bed, Maximize2, ExternalLink, ChevronLeft, ChevronRight, Building2, Calendar } from 'lucide-react'; import { Bed, Maximize2, ExternalLink, ChevronLeft, ChevronRight, Building2, Calendar } from 'lucide-react';
import type { PropertyFeature } from '@/types'; import type { PropertyFeature } from '@/types';
import { formatPrice, formatInteger, formatPricePerSqmShort, formatDuration, isFiniteNumber } from '@/utils/format';
interface SwipeCardProps { interface SwipeCardProps {
feature: PropertyFeature; feature: PropertyFeature;
@ -19,11 +20,12 @@ export function SwipeCard({ feature, onSwipe, onTap, isTop, stackIndex }: SwipeC
const hasSwiped = useRef(false); const hasSwiped = useRef(false);
const p = feature.properties; const p = feature.properties;
const photos = p.photos?.length ? p.photos : p.photo_thumbnail ? [p.photo_thumbnail] : []; const photos = p.photos?.length ? p.photos : p.photo_thumbnail ? [p.photo_thumbnail] : [];
// BUY listings may have null numeric fields; coerce so renders don't throw. // BUY listings may have null numeric fields; render "—" at the JSX leaf for
// missing values rather than coercing to 0. safeNum-coerced values are still used
// for alt text (where rendering "£0" is acceptable for non-visible content).
const safeNum = (v: unknown): number => (typeof v === 'number' && Number.isFinite(v) ? v : 0); const safeNum = (v: unknown): number => (typeof v === 'number' && Number.isFinite(v) ? v : 0);
const safeTotalPrice = safeNum(p.total_price); const safeTotalPrice = safeNum(p.total_price);
const safeQm = safeNum(p.qm); const safeQm = safeNum(p.qm);
const safeQmprice = safeNum(p.qmprice);
const safeRooms = safeNum(p.rooms); const safeRooms = safeNum(p.rooms);
const prefersReducedMotion = useMemo( const prefersReducedMotion = useMemo(
@ -177,8 +179,8 @@ export function SwipeCard({ feature, onSwipe, onTap, isTop, stackIndex }: SwipeC
> >
{/* Price */} {/* Price */}
<div className="text-2xl font-semibold"> <div className="text-2xl font-semibold">
£{safeTotalPrice.toLocaleString()} {formatPrice(p.total_price)}
{p.listing_type !== 'BUY' && ( {p.listing_type !== 'BUY' && isFiniteNumber(p.total_price) && (
<span className="text-muted-foreground font-normal text-base">/mo</span> <span className="text-muted-foreground font-normal text-base">/mo</span>
)} )}
</div> </div>
@ -186,12 +188,12 @@ export function SwipeCard({ feature, onSwipe, onTap, isTop, stackIndex }: SwipeC
{/* Key stats */} {/* Key stats */}
<div className="flex items-center gap-4 text-sm text-muted-foreground"> <div className="flex items-center gap-4 text-sm text-muted-foreground">
<span className="flex items-center gap-1"> <span className="flex items-center gap-1">
<Bed className="h-4 w-4" /> {safeRooms} bed <Bed className="h-4 w-4" /> {formatInteger(p.rooms)} bed
</span> </span>
<span className="flex items-center gap-1"> <span className="flex items-center gap-1">
<Maximize2 className="h-4 w-4" /> {safeQm} m² <Maximize2 className="h-4 w-4" /> {formatInteger(p.qm)} m²
</span> </span>
<span>£{safeQmprice}/m²</span> <span>{formatPricePerSqmShort(p.qmprice)}</span>
</div> </div>
{/* Agency & availability */} {/* Agency & availability */}
@ -216,7 +218,7 @@ export function SwipeCard({ feature, onSwipe, onTap, isTop, stackIndex }: SwipeC
key={`${d.poi_id}_${d.travel_mode}`} key={`${d.poi_id}_${d.travel_mode}`}
className="text-xs bg-muted px-2 py-0.5 rounded" className="text-xs bg-muted px-2 py-0.5 rounded"
> >
{d.poi_name}: {Math.round(d.duration_seconds / 60)}m {d.poi_name}: {formatDuration(d.duration_seconds)}
</span> </span>
))} ))}
</div> </div>

View file

@ -194,9 +194,11 @@ export function TaskIndicator({
size="icon" size="icon"
onClick={handleCancel} onClick={handleCancel}
disabled={isCancelling} disabled={isCancelling}
aria-label="Cancel task"
data-testid="task-cancel-button"
className="h-6 w-6 text-muted-foreground hover:text-destructive" className="h-6 w-6 text-muted-foreground hover:text-destructive"
> >
<X className="h-3 w-3" /> <X className="h-3 w-3 lucide-x" aria-hidden="true" />
</Button> </Button>
</TooltipTrigger> </TooltipTrigger>
<TooltipContent side="bottom"> <TooltipContent side="bottom">
@ -211,9 +213,11 @@ export function TaskIndicator({
size="icon" size="icon"
onClick={handleClearAll} onClick={handleClearAll}
disabled={isClearing} disabled={isClearing}
aria-label="Clear all tasks"
data-testid="task-clear-all-button"
className="h-6 w-6 text-muted-foreground hover:text-destructive" className="h-6 w-6 text-muted-foreground hover:text-destructive"
> >
<Trash2 className="h-3 w-3" /> <Trash2 className="h-3 w-3" aria-hidden="true" />
</Button> </Button>
</TooltipTrigger> </TooltipTrigger>
<TooltipContent side="bottom"> <TooltipContent side="bottom">

View file

@ -0,0 +1,182 @@
/**
* Regression tests for FilterBar (B2, B3, B22).
*
* B2: clicking BUY in the parent (header tab) MUST NOT cause a "Maximum update depth
* exceeded" loop. The previous implementation had three mutually-triggering
* useEffects (form parent sync + price defaults) that ping-ponged on every render.
*
* B3: FilterBar accepts initialValues (URL-derived) as its form defaults so deep-link
* URLs are honoured. The parent feeds these in from useFilterParams().
*
* B22: formatPrice() must visually distinguish 1500 from 2000 the previous
* toFixed(0) collapsed both to "£2k", hiding any sub-default change.
*/
import { render, screen, fireEvent } from '@testing-library/react';
import { useState } from 'react';
import { FilterBar, formatPrice } from '@/components/FilterBar';
import {
ListingType,
Metric,
type ParameterValues,
DEFAULT_FILTER_VALUES,
} from '@/components/FilterPanel';
import { mockUser } from '@/__tests__/helpers';
// Mock POIManager to avoid auth/poi dependencies
vi.mock('@/components/POIManager', () => ({
POIManager: () => <div data-testid="poi-manager">POIManager</div>,
}));
interface HarnessProps {
initialListingType?: ListingType;
initialValues?: ParameterValues;
}
/** Parent harness that controls listingType (matches App.tsx wiring). */
function Harness({ initialListingType = ListingType.RENT, initialValues }: HarnessProps) {
const [listingType, setListingType] = useState<ListingType>(initialListingType);
return (
<div>
<button
type="button"
data-testid="external-buy-button"
onClick={() => setListingType(ListingType.BUY)}
>
External BUY
</button>
<button
type="button"
data-testid="external-rent-button"
onClick={() => setListingType(ListingType.RENT)}
>
External RENT
</button>
<span data-testid="parent-listing-type">{listingType}</span>
<FilterBar
onSubmit={vi.fn()}
isLoading={false}
user={mockUser()}
userPOIs={[]}
onPOIsChange={vi.fn()}
poiTravelFilters={{}}
onPoiTravelFiltersChange={vi.fn()}
listingType={listingType}
onListingTypeChange={setListingType}
poiPickerActive={false}
onPoiPickerActiveChange={vi.fn()}
pickedPoiLocation={null}
onPickedPoiLocationChange={vi.fn()}
currentMetric={Metric.qmprice}
initialValues={initialValues}
/>
</div>
);
}
describe('FilterBar — listing type transition (B2 regression)', () => {
// Capture React's error log so we can assert no max-update-depth error fires.
let errorSpy: ReturnType<typeof vi.spyOn>;
beforeEach(() => {
errorSpy = vi.spyOn(console, 'error').mockImplementation(() => {});
});
afterEach(() => {
errorSpy.mockRestore();
});
it('mounts cleanly with parent listingType=RENT without infinite loop', () => {
render(<Harness initialListingType={ListingType.RENT} />);
expect(screen.getByTestId('parent-listing-type')).toHaveTextContent('RENT');
// The trigger button should reflect RENT defaults (£2k £3k).
expect(screen.getByText(/£2k.*£3k/)).toBeInTheDocument();
expect(errorSpy).not.toHaveBeenCalledWith(
expect.stringContaining('Maximum update depth'),
);
});
it('mounts cleanly with parent listingType=BUY (deep-link path)', () => {
render(<Harness initialListingType={ListingType.BUY} />);
expect(screen.getByTestId('parent-listing-type')).toHaveTextContent('BUY');
expect(errorSpy).not.toHaveBeenCalledWith(
expect.stringContaining('Maximum update depth'),
);
});
it('does NOT throw "Maximum update depth exceeded" when parent switches RENT → BUY', () => {
render(<Harness initialListingType={ListingType.RENT} />);
// Simulate clicking the header BUY tab (parent setListingType call).
fireEvent.click(screen.getByTestId('external-buy-button'));
expect(screen.getByTestId('parent-listing-type')).toHaveTextContent('BUY');
// The previous buggy code triggered React's "Maximum update depth exceeded" via
// console.error within a few renders. Assert it never fires.
const maxDepthCalls = errorSpy.mock.calls.filter((call) =>
call.some(
(arg) => typeof arg === 'string' && arg.includes('Maximum update depth'),
),
);
expect(maxDepthCalls).toHaveLength(0);
});
it('does NOT crash on rapid RENT ↔ BUY toggles', () => {
render(<Harness initialListingType={ListingType.RENT} />);
for (let i = 0; i < 5; i++) {
fireEvent.click(screen.getByTestId('external-buy-button'));
fireEvent.click(screen.getByTestId('external-rent-button'));
}
expect(screen.getByTestId('parent-listing-type')).toHaveTextContent('RENT');
const maxDepthCalls = errorSpy.mock.calls.filter((call) =>
call.some(
(arg) => typeof arg === 'string' && arg.includes('Maximum update depth'),
),
);
expect(maxDepthCalls).toHaveLength(0);
});
});
describe('FilterBar — initialValues / deep-link defaults (B3 regression)', () => {
it('renders URL-derived price range when initialValues is provided', () => {
const initial: ParameterValues = {
...DEFAULT_FILTER_VALUES,
listing_type: ListingType.BUY,
min_price: 500_000,
max_price: 800_000,
available_from: new Date(),
};
render(<Harness initialListingType={ListingType.BUY} initialValues={initial} />);
// Price trigger should reflect the URL values, not the defaults.
expect(screen.getByText(/£500k.*£800k/)).toBeInTheDocument();
});
it('falls back to defaults when initialValues is omitted', () => {
render(<Harness initialListingType={ListingType.RENT} />);
// RENT default 2000-3000.
expect(screen.getByText(/£2k.*£3k/)).toBeInTheDocument();
});
});
describe('formatPrice (B22 regression)', () => {
it('keeps one decimal for sub-10k values so 1500 and 2000 are distinguishable', () => {
expect(formatPrice(1500)).toBe('£1.5k');
expect(formatPrice(2500)).toBe('£2.5k');
// Whole-thousand values stay integer (no trailing .0)
expect(formatPrice(2000)).toBe('£2k');
expect(formatPrice(3000)).toBe('£3k');
});
it('rounds the chip-trigger label for >=10k values', () => {
expect(formatPrice(50_000)).toBe('£50k');
expect(formatPrice(500_000)).toBe('£500k');
expect(formatPrice(800_000)).toBe('£800k');
});
it('uses M-suffix for >=1M values', () => {
expect(formatPrice(1_000_000)).toBe('£1.0M');
expect(formatPrice(2_500_000)).toBe('£2.5M');
});
it('renders raw integer for sub-1k values', () => {
expect(formatPrice(500)).toBe('£500');
expect(formatPrice(0)).toBe('£0');
});
});

View file

@ -0,0 +1,82 @@
/**
* Tests for FilterChips (B17: Reset all affordance).
*/
import { render, screen, fireEvent } from '@testing-library/react';
import { FilterChips } from '@/components/FilterChips';
import { DEFAULT_FILTER_VALUES, type ParameterValues } from '@/components/FilterPanel';
const defaults: ParameterValues = {
...DEFAULT_FILTER_VALUES,
available_from: new Date(),
};
describe('FilterChips', () => {
it('renders no chips when values match defaults', () => {
const { container } = render(
<FilterChips values={defaults} defaults={defaults} onRemove={vi.fn()} />,
);
expect(container.firstChild).toBeNull();
});
it('renders a chip for a price-range change', () => {
const values: ParameterValues = { ...defaults, min_price: 1500, max_price: 2500 };
render(<FilterChips values={values} defaults={defaults} onRemove={vi.fn()} />);
// Chip should expose a remove button (proxy for chip existence)
expect(screen.getByLabelText(/Remove .* filter/)).toBeInTheDocument();
});
it('does NOT render Reset all when no onResetAll handler is provided (B17)', () => {
const values: ParameterValues = { ...defaults, district: 'Camden' };
render(<FilterChips values={values} defaults={defaults} onRemove={vi.fn()} />);
expect(screen.queryByText(/Reset all/i)).not.toBeInTheDocument();
});
it('renders Reset all button when onResetAll is provided and chips exist (B17)', () => {
const values: ParameterValues = { ...defaults, district: 'Camden' };
render(
<FilterChips
values={values}
defaults={defaults}
onRemove={vi.fn()}
onResetAll={vi.fn()}
/>,
);
expect(screen.getByText(/Reset all/i)).toBeInTheDocument();
});
it('does NOT render Reset all when there are no active filters', () => {
render(
<FilterChips
values={defaults}
defaults={defaults}
onRemove={vi.fn()}
onResetAll={vi.fn()}
/>,
);
// Since no chips render, the whole component returns null
expect(screen.queryByText(/Reset all/i)).not.toBeInTheDocument();
});
it('calls onResetAll when the Reset all button is clicked (B17)', () => {
const onResetAll = vi.fn();
const values: ParameterValues = { ...defaults, district: 'Camden' };
render(
<FilterChips
values={values}
defaults={defaults}
onRemove={vi.fn()}
onResetAll={onResetAll}
/>,
);
fireEvent.click(screen.getByText(/Reset all/i));
expect(onResetAll).toHaveBeenCalledTimes(1);
});
it('calls onRemove with the right key when a chip is removed', () => {
const onRemove = vi.fn();
const values: ParameterValues = { ...defaults, district: 'Camden' };
render(<FilterChips values={values} defaults={defaults} onRemove={onRemove} />);
fireEvent.click(screen.getByLabelText(/Remove .* filter/));
expect(onRemove).toHaveBeenCalledWith('district');
});
});

View file

@ -0,0 +1,156 @@
import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest';
import { render, screen, cleanup } from '@testing-library/react';
// Mock the constants module so we can flip the token between tests.
// Map.tsx reads MAP_CONFIG.MAPBOX_TOKEN at component render time so a per-test
// vi.doMock works as long as we re-import after the mock is registered.
const COLOR_SCHEMES_MOCK = {
getColorSchemeForMetric: () => [
[0, 'rgba(0,0,0,0)'] as [number, string],
[100, 'rgba(255,255,255,1)'] as [number, string],
],
getMetricInterpretation: () => ({ name: 'metric', low: 'low', high: 'high' }),
};
// Mock the heatmap worker client — instantiated by Map.tsx on map 'load'.
vi.mock('@/workers/HexgridHeatmapClient', () => ({
HexgridHeatmapClient: vi.fn().mockImplementation(() => ({
setIntensity: vi.fn(),
setSpread: vi.fn(),
setCellDensity: vi.fn(),
setPropertyName: vi.fn(),
setData: vi.fn(),
setColorStops: vi.fn(),
computeColorScale: vi.fn().mockResolvedValue({ hasValues: false, min: 0, max: 0 }),
computeBounds: vi.fn().mockResolvedValue({ minLng: 0, minLat: 0, maxLng: 0, maxLat: 0 }),
update: vi.fn(),
destroy: vi.fn(),
searchTree: vi.fn().mockReturnValue([]),
})),
}));
vi.mock('@/constants/colorSchemes', () => COLOR_SCHEMES_MOCK);
describe('Map — B19/B29 token-missing banner', () => {
const emptyData = { type: 'FeatureCollection' as const, features: [] };
beforeEach(() => {
vi.resetModules();
});
afterEach(() => {
cleanup();
vi.doUnmock('@/constants');
});
it('renders the "Map unavailable" banner when VITE_MAPBOX_TOKEN is empty', async () => {
vi.doMock('@/constants', async () => {
const actual = await vi.importActual<typeof import('@/constants')>('@/constants');
return {
...actual,
MAP_CONFIG: { ...actual.MAP_CONFIG, MAPBOX_TOKEN: '' },
};
});
const { Map } = await import('@/components/Map');
render(<Map listingData={emptyData} queryParameters={null} />);
const banner = screen.getByTestId('mapbox-token-missing-banner');
expect(banner).toBeInTheDocument();
expect(banner.textContent).toMatch(/VITE_MAPBOX_TOKEN/);
});
it('does NOT render the banner when the token is set', async () => {
vi.doMock('@/constants', async () => {
const actual = await vi.importActual<typeof import('@/constants')>('@/constants');
return {
...actual,
MAP_CONFIG: { ...actual.MAP_CONFIG, MAPBOX_TOKEN: 'pk.test' },
};
});
const { Map } = await import('@/components/Map');
render(<Map listingData={emptyData} queryParameters={null} />);
expect(screen.queryByTestId('mapbox-token-missing-banner')).not.toBeInTheDocument();
});
});
// B11 — When a HexgridHeatmapClient promise rejects post-destroy (rapid
// Map↔List view toggle), Map.tsx must catch and swallow the rejection rather
// than letting it bubble up as a pageerror.
describe('Map — B11 post-destroy promise rejection is swallowed', () => {
const emptyData = { type: 'FeatureCollection' as const, features: [] };
beforeEach(() => {
vi.resetModules();
});
afterEach(() => {
cleanup();
vi.doUnmock('@/workers/HexgridHeatmapClient');
vi.doUnmock('@/constants');
});
it('does not throw when computeColorScale rejects with "destroyed"', async () => {
// Replace the worker mock with one that rejects every async call
vi.doMock('@/workers/HexgridHeatmapClient', () => ({
HexgridHeatmapClient: vi.fn().mockImplementation(() => ({
setIntensity: vi.fn(),
setSpread: vi.fn(),
setCellDensity: vi.fn(),
setPropertyName: vi.fn(),
setData: vi.fn(),
setColorStops: vi.fn(),
computeColorScale: vi.fn().mockRejectedValue(
new Error('HexgridHeatmapClient destroyed'),
),
computeBounds: vi.fn().mockRejectedValue(
new Error('HexgridHeatmapClient destroyed'),
),
update: vi.fn(),
destroy: vi.fn(),
searchTree: vi.fn().mockReturnValue([]),
})),
}));
vi.doMock('@/constants', async () => {
const actual = await vi.importActual<typeof import('@/constants')>('@/constants');
return {
...actual,
MAP_CONFIG: { ...actual.MAP_CONFIG, MAPBOX_TOKEN: 'pk.test' },
};
});
// Spy on unhandledrejection at the global level
const unhandled: PromiseRejectionEvent[] = [];
const handler = (e: PromiseRejectionEvent) => unhandled.push(e);
window.addEventListener('unhandledrejection', handler);
try {
const { Map } = await import('@/components/Map');
const dataWithOne = {
type: 'FeatureCollection' as const,
features: [{
type: 'Feature' as const,
geometry: { type: 'Point' as const, coordinates: [0, 0] as [number, number] },
properties: {
id: 1, url: 'x', city: '', country: '', qm: 0, qmprice: 0,
total_price: 0, rooms: 0, agency: '', available_from: '',
last_seen: '', photo_thumbnail: '', price_history: [],
},
}],
};
render(<Map listingData={dataWithOne} queryParameters={null} />);
// Let microtasks settle so the awaited rejection has a chance to propagate
await new Promise((r) => setTimeout(r, 50));
// No unhandledrejection event should fire with "destroyed"
const destroyedErr = unhandled.find((e) =>
String((e.reason as Error)?.message ?? '').includes('destroyed'),
);
expect(destroyedErr).toBeUndefined();
} finally {
window.removeEventListener('unhandledrejection', handler);
}
});
});

View file

@ -0,0 +1,115 @@
import { render, screen, fireEvent } from '@testing-library/react';
import { PhotoCarousel } from '@/components/PhotoCarousel';
import type { ListingDetailPhoto } from '@/types';
function photo(url: string, caption: string | null = null): ListingDetailPhoto {
return { url, caption, type: null };
}
describe('PhotoCarousel', () => {
describe('B23 — single-photo and broken-image handling', () => {
it('suppresses the N/M counter when there is exactly one photo', () => {
render(<PhotoCarousel photos={[photo('https://example.com/a.jpg')]} />);
expect(screen.queryByText('1 / 1')).not.toBeInTheDocument();
});
it('renders the N/M counter when there are multiple photos', () => {
render(
<PhotoCarousel
photos={[
photo('https://example.com/a.jpg'),
photo('https://example.com/b.jpg'),
photo('https://example.com/c.jpg'),
]}
/>,
);
expect(screen.getByText('1 / 3')).toBeInTheDocument();
});
it('replaces a broken image with a placeholder tile when onError fires', () => {
render(
<PhotoCarousel
photos={[
photo('https://example.com/good.jpg'),
photo('https://example.com/bad.jpg'),
]}
/>,
);
const imgs = screen.getAllByRole('img');
expect(imgs).toHaveLength(2);
// Simulate the second image failing to load
fireEvent.error(imgs[1]);
// After error, the placeholder appears and the broken img is gone
expect(screen.getByText(/Photo unavailable/i)).toBeInTheDocument();
// The good image is still rendered
expect(screen.getAllByRole('img')).toHaveLength(1);
});
it('still renders "No photos available" when photos is empty', () => {
render(<PhotoCarousel photos={[]} />);
expect(screen.getByText(/No photos available/i)).toBeInTheDocument();
});
});
describe('B26 — single-photo carousel should not loop', () => {
it('does not render prev/next buttons for a single-photo carousel', () => {
render(<PhotoCarousel photos={[photo('https://example.com/a.jpg')]} />);
expect(screen.queryByLabelText(/Previous photo/i)).not.toBeInTheDocument();
expect(screen.queryByLabelText(/Next photo/i)).not.toBeInTheDocument();
});
it('does not render dots for a single-photo carousel', () => {
const { container } = render(
<PhotoCarousel photos={[photo('https://example.com/a.jpg')]} />,
);
// No "Go to photo X" buttons (dots) should be rendered when single
expect(container.querySelectorAll('button[aria-label^="Go to photo"]'))
.toHaveLength(0);
});
it('renders prev/next + dots for a multi-photo carousel', () => {
render(
<PhotoCarousel
photos={[
photo('https://example.com/a.jpg'),
photo('https://example.com/b.jpg'),
]}
/>,
);
expect(screen.getByLabelText(/Previous photo/i)).toBeInTheDocument();
expect(screen.getByLabelText(/Next photo/i)).toBeInTheDocument();
});
});
describe('B16 — keyboard navigation', () => {
it('makes the multi-photo carousel root keyboard-focusable', () => {
const { container } = render(
<PhotoCarousel
photos={[
photo('https://example.com/a.jpg'),
photo('https://example.com/b.jpg'),
]}
/>,
);
// The Embla overflow container is the focusable root
const region = container.querySelector('[role="region"][aria-label="Property photos"]');
expect(region).not.toBeNull();
// tabIndex should be set to 0 so the carousel can receive keydown
expect((region as HTMLElement).tabIndex).toBe(0);
});
it('does not make a single-photo carousel keyboard-focusable', () => {
const { container } = render(
<PhotoCarousel photos={[photo('https://example.com/a.jpg')]} />,
);
const region = container.querySelector('[role="region"][aria-label="Property photos"]');
// Region still exists (Embla wrapping) but should not be focusable, since
// there's nothing to navigate to.
expect(region).not.toBeNull();
const tabIndex = (region as HTMLElement).tabIndex;
// Default tabIndex on non-interactive elements is -1; we only flip to 0
// for multi-photo carousels.
expect(tabIndex).toBeLessThan(0);
});
});
});

View file

@ -120,4 +120,71 @@ describe('PropertyCard', () => {
expect(screen.queryByText(/NaN/)).not.toBeInTheDocument(); expect(screen.queryByText(/NaN/)).not.toBeInTheDocument();
expect(screen.queryByText(/d ago/)).not.toBeInTheDocument(); expect(screen.queryByText(/d ago/)).not.toBeInTheDocument();
}); });
// B10 regression: null numerics must render as em-dash, NOT "£0" / "0 m²" / "£0/m²"
it('renders em-dash placeholders for null total_price / qm / qmprice', () => {
const partial = {
...createMockProperty({ listing_type: 'BUY' }),
total_price: null,
qm: null,
qmprice: null,
} as unknown as PropertyProperties;
const { container } = render(<PropertyCard property={partial} />);
const text = container.textContent ?? '';
// No deceptive "£0" / "0 m²" / "£0/m²"
expect(text).not.toMatch(/£0\b/);
expect(text).not.toMatch(/\b0\s*m²/);
// Em-dash placeholders present
expect(text).toContain('—');
});
it('does not show /mo suffix when rent total_price is null', () => {
const partial = {
...createMockProperty({ listing_type: 'RENT' }),
total_price: null,
} as unknown as PropertyProperties;
render(<PropertyCard property={partial} />);
expect(screen.queryByText('/mo')).not.toBeInTheDocument();
});
// B25 regression: future last_seen must not produce negative "-7d ago"
it('clamps future last_seen to 0d ago instead of negative days', () => {
const future = new Date(Date.now() + 7 * 24 * 60 * 60 * 1000).toISOString();
const property = createMockProperty({ last_seen: future });
render(<PropertyCard property={property} />);
expect(screen.queryByText(/-\d+d ago/)).not.toBeInTheDocument();
expect(screen.getByText(/0d ago/)).toBeInTheDocument();
});
// B15 regression: compact POI badges show all three modes with em-dash for missing
it('renders all three travel modes per POI with em-dash for missing modes', () => {
const property = createMockProperty({
poi_distances: [
{ poi_id: 1, poi_name: 'Office', travel_mode: 'WALK', duration_seconds: 540, distance_meters: 800 },
],
});
const { container } = render(<PropertyCard property={property} />);
// The POI block should contain "Office:" with three mode entries
expect(container.textContent).toContain('Office:');
// Walk value is rendered
expect(container.textContent).toContain('9m');
// Two em-dash placeholders for the missing BICYCLE and TRANSIT modes within
// the badges. (Other em-dashes only appear if numerics are null — they aren't here.)
const emDashes = (container.textContent ?? '').match(/—/g) ?? [];
expect(emDashes.length).toBeGreaterThanOrEqual(2);
});
it('renders all three travel modes with values when all are present', () => {
const property = createMockProperty({
poi_distances: [
{ poi_id: 1, poi_name: 'Office', travel_mode: 'WALK', duration_seconds: 540, distance_meters: 800 },
{ poi_id: 1, poi_name: 'Office', travel_mode: 'BICYCLE', duration_seconds: 240, distance_meters: 1500 },
{ poi_id: 1, poi_name: 'Office', travel_mode: 'TRANSIT', duration_seconds: 900, distance_meters: 3000 },
],
});
const { container } = render(<PropertyCard property={property} />);
expect(container.textContent).toContain('9m');
expect(container.textContent).toContain('4m');
expect(container.textContent).toContain('15m');
});
}); });

View file

@ -80,6 +80,40 @@ describe('TaskIndicator', () => {
expect(screen.getByText('2')).toBeInTheDocument(); expect(screen.getByText('2')).toBeInTheDocument();
}); });
// B27 — Per-task X cancel button must render in the DOM while the task is
// in progress, and be discoverable both by aria-label and by data-testid.
describe('B27 — cancel button visibility during PROGRESS', () => {
it('renders the cancel X button when the active task is in progress', () => {
const tasks = { 't1': createMockTaskState({ status: 'STARTED' }) };
const { container } = renderIndicator(tasks, 't1');
expect(screen.getByTestId('task-cancel-button')).toBeInTheDocument();
expect(screen.getByLabelText('Cancel task')).toBeInTheDocument();
// The lucide-x svg must be present inside that button
const cancelBtn = container.querySelector('[data-testid="task-cancel-button"]');
expect(cancelBtn).not.toBeNull();
expect(cancelBtn?.querySelector('svg.lucide-x')).not.toBeNull();
});
it('does NOT render the cancel X button once the task succeeds', () => {
const tasks = { 't1': createMockTaskState({ status: 'SUCCESS' }) };
renderIndicator(tasks, 't1');
expect(screen.queryByTestId('task-cancel-button')).not.toBeInTheDocument();
});
it('does NOT render the cancel X button once the task fails', () => {
const tasks = { 't1': createMockTaskState({ status: 'FAILURE' }) };
renderIndicator(tasks, 't1');
expect(screen.queryByTestId('task-cancel-button')).not.toBeInTheDocument();
});
it('still renders the trash clear-all button when terminal', () => {
const tasks = { 't1': createMockTaskState({ status: 'SUCCESS' }) };
renderIndicator(tasks, 't1');
expect(screen.getByTestId('task-clear-all-button')).toBeInTheDocument();
});
});
it('fires onTaskCompleted when active task transitions to SUCCESS', async () => { it('fires onTaskCompleted when active task transitions to SUCCESS', async () => {
const onTaskCompleted = vi.fn(); const onTaskCompleted = vi.fn();
const tasks = { 't1': createMockTaskState({ status: 'STARTED' }) }; const tasks = { 't1': createMockTaskState({ status: 'STARTED' }) };

View file

@ -0,0 +1,36 @@
import { describe, it, expect } from 'vitest';
import { MAP_CONFIG } from '@/constants';
describe('MAP_CONFIG', () => {
describe('B18 — default map center is London', () => {
it('DEFAULT_CENTER points at London, not Czech Republic', () => {
// London ≈ [-0.1276, 51.5074]. The old default ([13.38032, 49.994210])
// landed in Czech Republic which is jarring for a UK-only app.
const [lng, lat] = MAP_CONFIG.DEFAULT_CENTER;
expect(lng).toBeGreaterThan(-1);
expect(lng).toBeLessThan(1);
expect(lat).toBeGreaterThan(51);
expect(lat).toBeLessThan(52);
});
it('DEFAULT_ZOOM gives a city-level view (not continent-level)', () => {
// Anything around 10 is a city / inner-borough view in Mapbox terms.
expect(MAP_CONFIG.DEFAULT_ZOOM).toBeGreaterThanOrEqual(9);
expect(MAP_CONFIG.DEFAULT_ZOOM).toBeLessThanOrEqual(13);
});
});
describe('B19 / B29 — Mapbox token sourced from env only', () => {
it('reads from VITE_MAPBOX_TOKEN (the test setup sets it to test-token)', () => {
// The test harness (src/__tests__/setup.ts) sets VITE_MAPBOX_TOKEN
// to "test-token". The constant module reads import.meta.env at import time.
expect(MAP_CONFIG.MAPBOX_TOKEN).toBe('test-token');
});
it('does not contain a hard-coded Mapbox public key as a fallback', () => {
// The previous code shipped a real public key (`pk.eyJ1...`). This regression
// test ensures we never leak a token into the bundle again.
expect(MAP_CONFIG.MAPBOX_TOKEN).not.toMatch(/^pk\.eyJ/);
});
});
});

View file

@ -16,11 +16,13 @@ export const API_ENDPOINTS = {
} as const; } as const;
// Map configuration // Map configuration
// MAPBOX_TOKEN is sourced exclusively from VITE_MAPBOX_TOKEN (B19/B29). When unset the
// map renders a banner instead of attempting to load tiles with an embedded fallback.
// DEFAULT_CENTER points at London since this is a UK-only listings app (B18).
export const MAP_CONFIG = { export const MAP_CONFIG = {
// Dev fallback token — production builds must set VITE_MAPBOX_TOKEN MAPBOX_TOKEN: import.meta.env.VITE_MAPBOX_TOKEN ?? '',
MAPBOX_TOKEN: import.meta.env.VITE_MAPBOX_TOKEN || 'pk.eyJ1IjoiZGktdG8iLCJhIjoiY2o0bnBoYXcxMW1mNzJ3bDhmc2xiNWttaiJ9.ZccatVk_4shzoAsEUXXecA', DEFAULT_CENTER: [-0.1276, 51.5074] as [number, number],
DEFAULT_CENTER: [13.38032, 49.994210] as [number, number], DEFAULT_ZOOM: 10,
DEFAULT_ZOOM: 5,
STYLE: 'mapbox://styles/mapbox/light-v9', STYLE: 'mapbox://styles/mapbox/light-v9',
} as const; } as const;

View file

@ -0,0 +1,133 @@
/**
* Tests for useFilterParams (B3 regression).
*
* B3: previously useFilterParams exposed setFilterValues but App.tsx never imported it.
* Filter changes never reached the URL reload reverted, share-URL dropped state,
* deep links were ignored. These tests cover the URL round-trip: parse, write, and
* restore via the React Router search params.
*/
import { renderHook, act } from '@testing-library/react';
import { MemoryRouter } from 'react-router-dom';
import type { ReactNode } from 'react';
import { useFilterParams } from '@/hooks/useFilterParams';
import { ListingType, Metric, FurnishType } from '@/components/FilterPanel';
function wrapperFactory(initialEntries: string[]) {
return ({ children }: { children: ReactNode }) => (
<MemoryRouter initialEntries={initialEntries}>{children}</MemoryRouter>
);
}
describe('useFilterParams — URL parsing (deep-link, B3)', () => {
it('parses ?type=BUY into ListingType.BUY', () => {
const { result } = renderHook(() => useFilterParams(), {
wrapper: wrapperFactory(['/?type=BUY']),
});
expect(result.current.filterValues.listing_type).toBe(ListingType.BUY);
});
it('parses ?minPrice=500000&maxPrice=800000 correctly', () => {
const { result } = renderHook(() => useFilterParams(), {
wrapper: wrapperFactory(['/?type=BUY&minPrice=500000&maxPrice=800000']),
});
expect(result.current.filterValues.min_price).toBe(500000);
expect(result.current.filterValues.max_price).toBe(800000);
});
it('parses ?minBeds=2&maxBeds=4 correctly', () => {
const { result } = renderHook(() => useFilterParams(), {
wrapper: wrapperFactory(['/?minBeds=2&maxBeds=4']),
});
expect(result.current.filterValues.min_bedrooms).toBe(2);
expect(result.current.filterValues.max_bedrooms).toBe(4);
});
it('parses ?minSqm=80&lastSeen=14 correctly', () => {
const { result } = renderHook(() => useFilterParams(), {
wrapper: wrapperFactory(['/?minSqm=80&lastSeen=14']),
});
expect(result.current.filterValues.min_sqm).toBe(80);
expect(result.current.filterValues.last_seen_days).toBe(14);
});
it('parses ?district=Camden&furnish=furnished,partFurnished correctly', () => {
const { result } = renderHook(() => useFilterParams(), {
wrapper: wrapperFactory(['/?district=Camden&furnish=furnished,partFurnished']),
});
expect(result.current.filterValues.district).toBe('Camden');
expect(result.current.filterValues.furnish_types).toEqual([
FurnishType.FURNISHED,
FurnishType.PART_FURNISHED,
]);
});
it('falls back to defaults for malformed values', () => {
const { result } = renderHook(() => useFilterParams(), {
wrapper: wrapperFactory(['/?minPrice=not-a-number&type=BOGUS']),
});
// Type defaults to RENT, price falls back to default.
expect(result.current.filterValues.listing_type).toBe(ListingType.RENT);
expect(result.current.filterValues.min_price).toBe(2000);
});
});
describe('useFilterParams — URL round-trip (B3 regression)', () => {
it('setFilterValues writes the filter state into the URL', () => {
const { result } = renderHook(() => useFilterParams(), {
wrapper: wrapperFactory(['/']),
});
act(() => {
result.current.setFilterValues({
...result.current.filterValues,
listing_type: ListingType.BUY,
min_price: 500000,
max_price: 800000,
min_bedrooms: 2,
max_bedrooms: 4,
min_sqm: 80,
last_seen_days: 14,
metric: Metric.qmprice,
district: '',
});
});
// After the write, re-render reads back the new URL state.
expect(result.current.filterValues.listing_type).toBe(ListingType.BUY);
expect(result.current.filterValues.min_price).toBe(500000);
expect(result.current.filterValues.max_price).toBe(800000);
expect(result.current.filterValues.min_bedrooms).toBe(2);
expect(result.current.filterValues.max_bedrooms).toBe(4);
expect(result.current.filterValues.min_sqm).toBe(80);
expect(result.current.filterValues.last_seen_days).toBe(14);
});
it('round-trips an arbitrary filter set unchanged', () => {
const { result } = renderHook(() => useFilterParams(), {
wrapper: wrapperFactory(['/']),
});
const written = {
...result.current.filterValues,
listing_type: ListingType.BUY,
min_price: 250000,
max_price: 450000,
min_bedrooms: 3,
max_bedrooms: 5,
district: 'Hackney',
last_seen_days: 7,
};
act(() => {
result.current.setFilterValues(written);
});
expect(result.current.filterValues.listing_type).toBe(written.listing_type);
expect(result.current.filterValues.min_price).toBe(written.min_price);
expect(result.current.filterValues.max_price).toBe(written.max_price);
expect(result.current.filterValues.min_bedrooms).toBe(written.min_bedrooms);
expect(result.current.filterValues.max_bedrooms).toBe(written.max_bedrooms);
expect(result.current.filterValues.district).toBe(written.district);
expect(result.current.filterValues.last_seen_days).toBe(written.last_seen_days);
});
});

View file

@ -224,4 +224,44 @@ describe('useTaskProgress', () => {
expect.objectContaining({ accessToken: 'test-access-token' }), expect.objectContaining({ accessToken: 'test-access-token' }),
); );
}); });
// B28 — Polling must NOT keep hitting /api/task_status for IDs that are
// already terminal locally, even if the server keeps returning them from
// /api/tasks_for_user during the retention window.
it('skips polling task_status for IDs whose local status is terminal', async () => {
const { fetchTasksForUser, fetchTaskStatus } = await import('@/services');
const fetchTasksForUserMock = fetchTasksForUser as unknown as ReturnType<typeof vi.fn>;
const fetchTaskStatusMock = fetchTaskStatus as unknown as ReturnType<typeof vi.fn>;
// Server keeps returning a terminal task ID
fetchTasksForUserMock.mockResolvedValue(['t-done']);
const { result } = renderHook(() => useTaskProgress(mockUser()));
const ws = MockWebSocket.instances[0];
// Seed the local cache via WS init so the hook knows t-done is SUCCESS
act(() => {
ws.simulateOpen();
ws.simulateMessage({
type: 'init',
tasks: [createMockTaskState({ task_id: 't-done', status: 'SUCCESS' })],
});
});
expect(result.current.tasks['t-done'].status).toBe('SUCCESS');
// Clear any fetch calls accumulated so far so we measure only what comes next
fetchTaskStatusMock.mockClear();
// Trigger a polling round and let microtasks settle
await act(async () => {
await vi.advanceTimersByTimeAsync(60_000);
});
// fetchTaskStatus must NOT have been called for the terminal-local task
const calledWithDone = fetchTaskStatusMock.mock.calls.some(
([, taskId]) => taskId === 't-done',
);
expect(calledWithDone).toBe(false);
});
}); });

View file

@ -137,7 +137,13 @@ export function useTaskProgress(user: AuthUser | null): UseTaskProgressReturn {
.filter(([, t]) => !isTerminalStatus(t.status)) .filter(([, t]) => !isTerminalStatus(t.status))
.map(([id]) => id); .map(([id]) => id);
const allIds = [...new Set([...taskIds, ...localIds])]; // Stop polling task IDs that are already terminal locally — the server may
// keep returning them from /api/tasks_for_user for some retention window
// but there's no point re-fetching their status (B28).
const allIds = [...new Set([...taskIds, ...localIds])].filter((id) => {
const local = tasksRef.current[id];
return !local || !isTerminalStatus(local.status);
});
if (allIds.length === 0) return; if (allIds.length === 0) return;
const results = await Promise.allSettled( const results = await Promise.allSettled(

View file

@ -1,6 +1,6 @@
import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest';
import { mockUser, createMockFeature } from '@/__tests__/helpers'; import { mockUser, createMockFeature } from '@/__tests__/helpers';
import { streamListingGeoJSON } from '@/services/streamingService'; import { streamListingGeoJSON, StreamParseError } from '@/services/streamingService';
import { ApiError } from '@/types'; import { ApiError } from '@/types';
import type { ParameterValues } from '@/components/FilterPanel'; import type { ParameterValues } from '@/components/FilterPanel';
@ -211,4 +211,46 @@ describe('streamingService', () => {
for await (const _ of streamListingGeoJSON(mockUser(), defaultParams)) { /* drain */ } for await (const _ of streamListingGeoJSON(mockUser(), defaultParams)) { /* drain */ }
}).rejects.toThrow('No response body'); }).rejects.toThrow('No response body');
}); });
// B7 regression: HTML response (e.g. proxy SPA fallback when backend is down)
// should bail on the first parse error instead of looping 18× per stream.
it('throws StreamParseError on the first unparseable line when nothing has parsed yet', async () => {
const consoleSpy = vi.spyOn(console, 'error').mockImplementation(() => {});
const htmlLines = [
'<!doctype html>',
'<html>',
' <head><title>App</title></head>',
' <body><div id="root"></div></body>',
'</html>',
];
globalThis.fetch = vi.fn().mockResolvedValue(createMockResponse(htmlLines));
let caught: unknown = null;
try {
for await (const _ of streamListingGeoJSON(mockUser(), defaultParams)) { /* drain */ }
} catch (e) {
caught = e;
}
expect(caught).toBeInstanceOf(StreamParseError);
// Critical: no console.error spam — bail immediately on first failure.
expect(consoleSpy).not.toHaveBeenCalled();
consoleSpy.mockRestore();
});
it('StreamParseError captures a snippet of the offending input', async () => {
globalThis.fetch = vi.fn().mockResolvedValue(
createMockResponse(['<!doctype html><html><head>...</head>']),
);
let caught: StreamParseError | null = null;
try {
for await (const _ of streamListingGeoJSON(mockUser(), defaultParams)) { /* drain */ }
} catch (e) {
caught = e as StreamParseError;
}
expect(caught).toBeInstanceOf(StreamParseError);
expect(caught?.snippet).toContain('<!doctype html>');
});
}); });

View file

@ -1,7 +1,7 @@
// Re-export all services // Re-export all services
export { apiRequest } from './apiClient'; export { apiRequest } from './apiClient';
export { fetchListingGeoJSON, refreshListings } from './listingService'; export { fetchListingGeoJSON, refreshListings } from './listingService';
export { streamListingGeoJSON, type StreamingProgress } from './streamingService'; export { streamListingGeoJSON, StreamParseError, type StreamingProgress } from './streamingService';
export { fetchTasksForUser, fetchTaskStatus, cancelTask, clearAllTasks, type CancelTaskResponse, type ClearAllTasksResponse } from './taskService'; export { fetchTasksForUser, fetchTaskStatus, cancelTask, clearAllTasks, type CancelTaskResponse, type ClearAllTasksResponse } from './taskService';
export { checkBackendHealth, type HealthStatus, type HealthCheckResult } from './healthService'; export { checkBackendHealth, type HealthStatus, type HealthCheckResult } from './healthService';
export { fetchUserPOIs, createPOI, updatePOI, deletePOI, triggerPOICalculation, fetchPOIDistances, fetchBulkPOIDistances } from './poiService'; export { fetchUserPOIs, createPOI, updatePOI, deletePOI, triggerPOICalculation, fetchPOIDistances, fetchBulkPOIDistances } from './poiService';

View file

@ -8,6 +8,22 @@ import { API_ENDPOINTS } from '@/constants';
import { fireUnauthorized } from './apiClient'; import { fireUnauthorized } from './apiClient';
import { record as recordPerf } from './perfCollector'; import { record as recordPerf } from './perfCollector';
/**
* Thrown when the stream endpoint returns a response that isn't NDJSON (e.g. HTML
* because the backend is down and a proxy served the SPA fallback, or an auth
* redirect). Surfacing this as a typed error lets `loadListings` show a single
* user-visible error dialog instead of dribbling out one console.error per line.
*/
export class StreamParseError extends Error {
constructor(public readonly snippet: string) {
super(
`Server returned non-streaming response (expected NDJSON). ` +
`First bytes: ${snippet.slice(0, 80)}${snippet.length > 80 ? '…' : ''}`,
);
this.name = 'StreamParseError';
}
}
/** /**
* Build query string from parameters object * Build query string from parameters object
*/ */
@ -102,6 +118,11 @@ export async function* streamListingGeoJSON(
let totalCount = 0; let totalCount = 0;
let streamStart = performance.now(); let streamStart = performance.now();
let firstBatchRecorded = false; let firstBatchRecorded = false;
// Track whether we've successfully parsed at least one NDJSON message. If the
// very first non-empty line fails to parse, the server almost certainly returned
// HTML (proxy SPA fallback, login redirect, etc) — bail with a typed error
// instead of spamming console for every line.
let parsedAny = false;
while (true) { while (true) {
if (options?.signal?.aborted) { if (options?.signal?.aborted) {
@ -120,6 +141,7 @@ export async function* streamListingGeoJSON(
try { try {
const message: StreamMessage = JSON.parse(line); const message: StreamMessage = JSON.parse(line);
parsedAny = true;
if (message.type === 'metadata') { if (message.type === 'metadata') {
onProgress?.({ count: 0, total: message.total_expected }); onProgress?.({ count: 0, total: message.total_expected });
@ -135,6 +157,15 @@ export async function* streamListingGeoJSON(
onProgress?.({ count: message.total ?? totalCount, total: message.total }); onProgress?.({ count: message.total ?? totalCount, total: message.total });
} }
} catch (e) { } catch (e) {
if (!parsedAny) {
// First line was unparseable — assume the entire response is non-NDJSON
// (e.g. HTML). Cancel the reader so we stop consuming the body and
// surface a typed error to the caller.
await reader.cancel().catch(() => {});
throw new StreamParseError(line);
}
// A mid-stream parse hiccup after we've already received valid messages
// — log and continue (matches the prior tolerant behaviour).
console.error('Failed to parse streaming message:', e); console.error('Failed to parse streaming message:', e);
} }
} }
@ -144,10 +175,14 @@ export async function* streamListingGeoJSON(
if (buffer.trim()) { if (buffer.trim()) {
try { try {
const message: StreamMessage = JSON.parse(buffer); const message: StreamMessage = JSON.parse(buffer);
parsedAny = true;
if (message.type === 'batch' && message.features) { if (message.type === 'batch' && message.features) {
yield message.features; yield message.features;
} }
} catch (e) { } catch (e) {
if (!parsedAny) {
throw new StreamParseError(buffer);
}
console.error('Failed to parse final streaming message:', e); console.error('Failed to parse final streaming message:', e);
} }
} }

View file

@ -0,0 +1,150 @@
import { describe, it, expect } from 'vitest';
import {
EM_DASH,
formatCurrency,
formatDate,
formatDuration,
formatInteger,
formatPrice,
formatPricePerSqm,
formatPricePerSqmShort,
isFiniteNumber,
} from '@/utils/format';
describe('formatCurrency', () => {
it('formats values >= 1000 in compact k notation', () => {
expect(formatCurrency(1500)).toBe('£1.5k');
expect(formatCurrency(2500)).toBe('£2.5k');
expect(formatCurrency(1000000)).toBe('£1000.0k');
});
it('formats sub-thousand values as rounded integers', () => {
expect(formatCurrency(950)).toBe('£950');
expect(formatCurrency(123.4)).toBe('£123');
});
});
describe('formatDuration', () => {
it('renders minutes for sub-hour durations', () => {
expect(formatDuration(0)).toBe('0m');
expect(formatDuration(120)).toBe('2m');
expect(formatDuration(3540)).toBe('59m');
});
it('renders hours-and-minutes for multi-hour durations', () => {
expect(formatDuration(3600)).toBe('1h');
expect(formatDuration(5400)).toBe('1h30m');
expect(formatDuration(86400)).toBe('24h');
});
// B9 regression cases
it('returns em-dash for negative seconds (was "-2m")', () => {
expect(formatDuration(-120)).toBe(EM_DASH);
expect(formatDuration(-1)).toBe(EM_DASH);
});
it('returns em-dash for null and undefined (was "0m")', () => {
expect(formatDuration(null)).toBe(EM_DASH);
expect(formatDuration(undefined)).toBe(EM_DASH);
});
it('returns em-dash for NaN (was "NaNh")', () => {
expect(formatDuration(NaN)).toBe(EM_DASH);
});
it('returns em-dash for non-finite values (Infinity)', () => {
expect(formatDuration(Infinity)).toBe(EM_DASH);
expect(formatDuration(-Infinity)).toBe(EM_DASH);
});
it('caps absurdly large values at >24h', () => {
expect(formatDuration(86401)).toBe('>24h');
expect(formatDuration(90000)).toBe('>24h');
expect(formatDuration(31536000)).toBe('>24h'); // 1 year
});
});
describe('formatDate', () => {
it('formats valid ISO dates', () => {
expect(formatDate('2025-01-03T00:00:00Z')).toContain('2025');
});
it('returns the input string for invalid dates', () => {
expect(formatDate('not-a-date')).toBe('not-a-date');
});
});
describe('formatPricePerSqm', () => {
it('returns the formatted string when both values are valid', () => {
expect(formatPricePerSqm(500000, 100)).toBe('£5,000/m²');
});
it('returns null when sqm is missing', () => {
expect(formatPricePerSqm(500000, null)).toBe(null);
expect(formatPricePerSqm(500000, undefined)).toBe(null);
expect(formatPricePerSqm(500000, 0)).toBe(null);
});
});
describe('isFiniteNumber', () => {
it('returns true for finite numbers', () => {
expect(isFiniteNumber(0)).toBe(true);
expect(isFiniteNumber(-1)).toBe(true);
expect(isFiniteNumber(1.5)).toBe(true);
});
it('returns false for non-finite, null, undefined, and other types', () => {
expect(isFiniteNumber(null)).toBe(false);
expect(isFiniteNumber(undefined)).toBe(false);
expect(isFiniteNumber(NaN)).toBe(false);
expect(isFiniteNumber(Infinity)).toBe(false);
expect(isFiniteNumber('5')).toBe(false);
});
});
describe('formatPrice', () => {
it('formats finite numbers with thousands separators', () => {
expect(formatPrice(0)).toBe('£0');
expect(formatPrice(2500)).toBe('£2,500');
expect(formatPrice(500000)).toBe('£500,000');
expect(formatPrice(1234.7)).toBe('£1,235');
});
// B10 regression: null/undefined must NOT render as "£0"
it('returns em-dash for null/undefined/non-finite', () => {
expect(formatPrice(null)).toBe(EM_DASH);
expect(formatPrice(undefined)).toBe(EM_DASH);
expect(formatPrice(NaN)).toBe(EM_DASH);
expect(formatPrice(Infinity)).toBe(EM_DASH);
});
});
describe('formatPricePerSqmShort', () => {
it('formats valid positive values', () => {
expect(formatPricePerSqmShort(38)).toBe('£38/m²');
expect(formatPricePerSqmShort(4500)).toBe('£4500/m²');
});
// B10: zero & negative are missing-data sentinels, not real values
it('returns em-dash for null/undefined/non-finite/zero/negative', () => {
expect(formatPricePerSqmShort(null)).toBe(EM_DASH);
expect(formatPricePerSqmShort(undefined)).toBe(EM_DASH);
expect(formatPricePerSqmShort(NaN)).toBe(EM_DASH);
expect(formatPricePerSqmShort(0)).toBe(EM_DASH);
expect(formatPricePerSqmShort(-5)).toBe(EM_DASH);
});
});
describe('formatInteger', () => {
it('rounds and stringifies finite numbers', () => {
expect(formatInteger(0)).toBe('0');
expect(formatInteger(3)).toBe('3');
expect(formatInteger(65.7)).toBe('66');
});
it('returns em-dash for null/undefined/non-finite', () => {
expect(formatInteger(null)).toBe(EM_DASH);
expect(formatInteger(undefined)).toBe(EM_DASH);
expect(formatInteger(NaN)).toBe(EM_DASH);
});
});

View file

@ -5,14 +5,62 @@
* PropertyCard, ListingDetail, MobileBottomSheet, and StatsBar. * PropertyCard, ListingDetail, MobileBottomSheet, and StatsBar.
*/ */
/** Em-dash placeholder used at render boundaries for missing/invalid values. */
export const EM_DASH = '—';
/** Returns true when v is a finite number — i.e. safe to render directly. */
export function isFiniteNumber(v: unknown): v is number {
return typeof v === 'number' && Number.isFinite(v);
}
/**
* Format an integer GBP price with thousands separators, e.g. "£2,500".
* Returns the em-dash sentinel for null / undefined / non-finite inputs.
* Use at the JSX leaf to distinguish "missing data" from "£0".
*/
export function formatPrice(value: number | null | undefined): string {
if (!isFiniteNumber(value)) return EM_DASH;
return `£${Math.round(value).toLocaleString()}`;
}
/**
* Format a price-per-square-metre integer, e.g. "£42/m²".
* Returns the em-dash sentinel for null / undefined / non-finite / zero / negative inputs.
* (Zero / negative qmprice values come from missing-data rows, not real listings.)
*/
export function formatPricePerSqmShort(value: number | null | undefined): string {
if (!isFiniteNumber(value) || value <= 0) return EM_DASH;
return `£${Math.round(value)}/m²`;
}
/**
* Format an integer count (bedrooms, square metres, etc).
* Returns the em-dash sentinel for null / undefined / non-finite inputs.
*/
export function formatInteger(value: number | null | undefined): string {
if (!isFiniteNumber(value)) return EM_DASH;
return `${Math.round(value)}`;
}
/** Format a number as a compact GBP string, e.g. "£1.2k" or "£950". */ /** Format a number as a compact GBP string, e.g. "£1.2k" or "£950". */
export function formatCurrency(value: number): string { export function formatCurrency(value: number): string {
if (value >= 1000) return `£${(value / 1000).toFixed(1)}k`; if (value >= 1000) return `£${(value / 1000).toFixed(1)}k`;
return `£${Math.round(value)}`; return `£${Math.round(value)}`;
} }
/** Format a duration in seconds as a human-readable string, e.g. "12m" or "1h30m". */ /**
export function formatDuration(seconds: number): string { * Format a duration in seconds as a human-readable string, e.g. "12m" or "1h30m".
*
* Returns the em-dash sentinel for non-finite or negative inputs (null, NaN, -120).
* Caps absurdly large values (> 24h) as ">24h" to avoid rendering nonsense like
* "8760h" when a backend bug produces year-scale values.
*/
export function formatDuration(seconds: number | null | undefined): string {
if (typeof seconds !== 'number' || !Number.isFinite(seconds) || seconds < 0) {
return EM_DASH;
}
// Cap at 24 hours — beyond this it's almost certainly bad data.
if (seconds > 24 * 3600) return '>24h';
const minutes = Math.round(seconds / 60); const minutes = Math.round(seconds / 60);
if (minutes < 60) return `${minutes}m`; if (minutes < 60) return `${minutes}m`;
const hours = Math.floor(minutes / 60); const hours = Math.floor(minutes / 60);

View file

@ -20,12 +20,14 @@ function isStale(type: string, requestId: number): boolean {
return (latestRequestId[type] ?? 0) > requestId; return (latestRequestId[type] ?? 0) > requestId;
} }
// Average non-NaN values (same reduce function as original HexgridHeatmap) // Average finite numeric values, excluding null/undefined/NaN/infinity.
// We can't use `!isNaN(data[i])` because isNaN(null) === false (Number(null) === 0),
// which would silently pull missing values in as zeros and bias the average.
function reduceAverage(data: number[]): number { function reduceAverage(data: number[]): number {
let sum = 0; let sum = 0;
let count = 0; let count = 0;
for (let i = 0; i < data.length; i++) { for (let i = 0; i < data.length; i++) {
if (!isNaN(data[i])) { if (typeof data[i] === 'number' && Number.isFinite(data[i])) {
sum += data[i]; sum += data[i];
count++; count++;
} }

View file

@ -27,6 +27,27 @@ spec:
kubernetes.io/cluster-service: "true" kubernetes.io/cluster-service: "true"
spec: spec:
automountServiceAccountToken: true automountServiceAccountToken: true
securityContext:
runAsUser: 1000
runAsGroup: 1000
fsGroup: 1000
initContainers:
# One-shot chown so existing NFS-backed listing dirs (mode 775, owned
# by various historical UIDs) become writable by the new uid=1000
# appuser. Idempotent: chown of already-owned files is a no-op.
- name: fix-data-permissions
image: busybox:1.36
command:
- sh
- -c
- |
chown -R 1000:1000 /app/data || true
chmod -R u+rwX,g+rwX /app/data || true
securityContext:
runAsUser: 0
volumeMounts:
- mountPath: /app/data
name: data
containers: containers:
- env: - env:
- name: ENV - name: ENV

View file

@ -25,6 +25,27 @@ spec:
app: realestate-crawler-celery-beat app: realestate-crawler-celery-beat
spec: spec:
automountServiceAccountToken: true automountServiceAccountToken: true
securityContext:
runAsUser: 1000
runAsGroup: 1000
fsGroup: 1000
initContainers:
# One-shot chown so existing NFS-backed listing dirs (mode 775, owned
# by various historical UIDs) become writable by the new uid=1000
# appuser. Idempotent: chown of already-owned files is a no-op.
- name: fix-data-permissions
image: busybox:1.36
command:
- sh
- -c
- |
chown -R 1000:1000 /app/data || true
chmod -R u+rwX,g+rwX /app/data || true
securityContext:
runAsUser: 0
volumeMounts:
- mountPath: /app/data
name: data
containers: containers:
- command: - command:
- python - python

View file

@ -1,6 +1,7 @@
from datetime import timedelta from datetime import timedelta
from functools import lru_cache from functools import lru_cache
import json import json
import os
from string import Template from string import Template
from typing import Any, TypeVar from typing import Any, TypeVar
from api.auth import User from api.auth import User
@ -9,16 +10,35 @@ from celery_app import app
T = TypeVar("T") T = TypeVar("T")
# Default Redis logical DB for per-user state (task lists, WebAuthn
# challenges). Previously this lived on db0 alongside the Celery broker
# AND another app's kombu bindings (paperless-ngx). Moving to db3 isolates
# user state from broker traffic and prevents key collisions.
_DEFAULT_USER_DB = 3
# Namespace for every key written by this class so any other process
# sharing the Redis instance can't collide.
_KEY_PREFIX = "wrongmove:user:"
class RedisRepository: class RedisRepository:
redis_client: redis.Redis # type: ignore[type-arg] redis_client: redis.Redis # type: ignore[type-arg]
tasks_key_template: Template = Template("user:{user_id}/tasks") # tasks_key_template is the *suffix* portion; set_key / get_key uniformly
# prepend ``_KEY_PREFIX`` so every key this class writes is namespaced.
tasks_key_template: Template = Template("${user_id}/tasks")
def __init__(self) -> None: def __init__(self) -> None:
redis_hostname: str = app.broker_connection().info()["hostname"] redis_hostname: str = app.broker_connection().info()["hostname"]
redis_port: int = app.broker_connection().info()["port"] redis_port: int = app.broker_connection().info()["port"]
db = int(os.getenv("REDIS_USER_DB", str(_DEFAULT_USER_DB)))
# socket_keepalive + health_check_interval keep the connection
# alive across the Redis HAProxy 30s idle timeout (see celery_app.py).
self.redis_client = redis.Redis( self.redis_client = redis.Redis(
host=redis_hostname, port=redis_port, db=0, decode_responses=True host=redis_hostname,
port=redis_port,
db=db,
decode_responses=True,
socket_keepalive=True,
health_check_interval=25,
) # decode_responses=True returns str, not bytes ) # decode_responses=True returns str, not bytes
@staticmethod @staticmethod
@ -26,15 +46,23 @@ class RedisRepository:
def instance() -> "RedisRepository": def instance() -> "RedisRepository":
return RedisRepository() return RedisRepository()
@staticmethod
def _prefixed(key: str) -> str:
"""Prepend the wrongmove user-namespace prefix if not already present."""
if key.startswith(_KEY_PREFIX):
return key
return f"{_KEY_PREFIX}{key}"
def set_key(self, key: str, value: Any, ttl: timedelta | None = None) -> None: def set_key(self, key: str, value: Any, ttl: timedelta | None = None) -> None:
full_key = self._prefixed(key)
serialized_value = self.__serialize_value(value) serialized_value = self.__serialize_value(value)
self.redis_client.set(key, serialized_value) self.redis_client.set(full_key, serialized_value)
ttl = ttl or timedelta(hours=3) ttl = ttl or timedelta(hours=3)
self.redis_client.expire(key, ttl) self.redis_client.expire(full_key, ttl)
def get_key(self, key: str) -> Any | None: def get_key(self, key: str) -> Any | None:
serialized_value = self.redis_client.get(key) serialized_value = self.redis_client.get(self._prefixed(key))
if serialized_value is None: if serialized_value is None:
return None return None
return self.__deserialize_value(serialized_value) return self.__deserialize_value(serialized_value)
@ -71,7 +99,9 @@ class RedisRepository:
"""Clear all tasks for a user. Returns the number of tasks cleared.""" """Clear all tasks for a user. Returns the number of tasks cleared."""
current_tasks: list[str] = self.get_tasks_for_user(user) current_tasks: list[str] = self.get_tasks_for_user(user)
count = len(current_tasks) count = len(current_tasks)
self.redis_client.delete(self.tasks_key_template.substitute(user_id=user.email)) self.redis_client.delete(
self._prefixed(self.tasks_key_template.substitute(user_id=user.email))
)
return count return count
def __serialize_value(self, value: Any) -> str: def __serialize_value(self, value: Any) -> str:

View file

@ -36,7 +36,14 @@ def _get_redis_client() -> redis.Redis:
broker_url = os.getenv("CELERY_BROKER_URL", "redis://localhost:6379/0") broker_url = os.getenv("CELERY_BROKER_URL", "redis://localhost:6379/0")
parsed = urlparse(broker_url) parsed = urlparse(broker_url)
cache_url = urlunparse(parsed._replace(path=f"/{CACHE_DB}")) cache_url = urlunparse(parsed._replace(path=f"/{CACHE_DB}"))
return redis.from_url(cache_url, decode_responses=True) # socket_keepalive + health_check_interval keep the connection alive
# across the Redis HAProxy 30s idle timeout (see celery_app.py).
return redis.from_url(
cache_url,
decode_responses=True,
socket_keepalive=True,
health_check_interval=25,
)
def make_cache_key(query_params: QueryParameters) -> str: def make_cache_key(query_params: QueryParameters) -> str:

View file

@ -23,7 +23,15 @@ def _get_redis_client() -> redis.Redis: # type: ignore[type-arg]
global _redis_client global _redis_client
if _redis_client is None: if _redis_client is None:
broker_url = os.getenv("CELERY_BROKER_URL", "redis://redis:6379/0") broker_url = os.getenv("CELERY_BROKER_URL", "redis://redis:6379/0")
_redis_client = redis.Redis.from_url(broker_url, decode_responses=True) # socket_keepalive + health_check_interval keep the connection
# alive across the Redis HAProxy 30s idle timeout that fronts the
# in-cluster Redis Sentinel.
_redis_client = redis.Redis.from_url(
broker_url,
decode_responses=True,
socket_keepalive=True,
health_check_interval=25,
)
return _redis_client return _redis_client

View file

@ -339,7 +339,13 @@ async def _process_worker(
state: _PipelineState, state: _PipelineState,
reporter: ProgressReporter, reporter: ProgressReporter,
) -> None: ) -> None:
"""Consumer worker: pull listing IDs from the queue and process them.""" """Consumer worker: pull listing IDs from the queue and process them.
Per-listing exceptions (PermissionError, OSError, asyncio.TimeoutError,
etc.) are caught, counted, logged, and skipped so a single bad listing
cannot abort the entire scrape pipeline. CancelledError is re-raised so
cooperative cancellation still works.
"""
while True: while True:
listing_id = await queue.get() listing_id = await queue.get()
if listing_id is None: if listing_id is None:
@ -353,19 +359,42 @@ async def _process_worker(
elif step_name == "ocr": elif step_name == "ocr":
state.ocr_completed += 1 state.ocr_completed += 1
listing = await processor.process_listing( try:
listing_id, on_step_complete=step_callback listing = await processor.process_listing(
) listing_id, on_step_complete=step_callback
if listing is not None: )
state.processed_count += 1 if listing is not None:
state.processed_listings.append(listing) state.processed_count += 1
else: state.processed_listings.append(listing)
else:
state.failed_count += 1
except asyncio.CancelledError:
# Cooperative cancellation — let it propagate so the gather()
# in dump_listings_full can unwind cleanly.
raise
except Exception:
state.failed_count += 1 state.failed_count += 1
celery_logger.exception(
"Unhandled exception processing listing %s; skipping", listing_id
)
reporter.notify() reporter.notify()
@app.task(bind=True, pydantic=True) @app.task(
bind=True,
pydantic=True,
# Hard kill if the task hasn't completed in 1h (matches the SLA — a
# full scrape takes ~10-15 min in practice). soft_time_limit raises
# SoftTimeLimitExceeded so cleanup / FAILURE-publish runs first.
time_limit=3600,
soft_time_limit=3500,
# acks_late so a worker crash mid-task re-queues the task instead of
# acknowledging-and-losing it. Combined with the visibility_timeout
# defaults, this is safe because the lock at the top of the body
# prevents two workers running concurrently.
acks_late=True,
)
def dump_listings_task(self: Task, parameters_json: str) -> dict[str, Any]: def dump_listings_task(self: Task, parameters_json: str) -> dict[str, Any]:
with redis_lock(SCRAPE_LOCK_NAME) as acquired: with redis_lock(SCRAPE_LOCK_NAME) as acquired:
if not acquired: if not acquired:
@ -383,7 +412,24 @@ def dump_listings_task(self: Task, parameters_json: str) -> dict[str, Any]:
self.update_state(state="Starting...", meta={"phase": PHASE_SPLITTING, "progress": 0}) self.update_state(state="Starting...", meta={"phase": PHASE_SPLITTING, "progress": 0})
publish_task_progress(self.request.id, "Starting...", {"phase": PHASE_SPLITTING, "progress": 0}) publish_task_progress(self.request.id, "Starting...", {"phase": PHASE_SPLITTING, "progress": 0})
asyncio.run(dump_listings_full(task=self, parameters=parsed_parameters)) try:
asyncio.run(dump_listings_full(task=self, parameters=parsed_parameters))
except Exception as exc:
# Publish a terminal FAILURE event so WebSocket subscribers update
# immediately, then re-raise so Celery records the failure in
# the result backend.
celery_logger.exception("dump_listings_task failed: %s", exc)
publish_task_progress(
self.request.id,
"FAILURE",
{
"phase": PHASE_COMPLETED,
"progress": 0,
"error": str(exc),
"exc_type": type(exc).__name__,
},
)
raise
result = {"phase": PHASE_COMPLETED, "progress": 1} result = {"phase": PHASE_COMPLETED, "progress": 1}
publish_task_progress(self.request.id, "SUCCESS", result) publish_task_progress(self.request.id, "SUCCESS", result)
return result return result

View file

@ -0,0 +1,37 @@
"""Regression test for Dockerfile UID/GID.
QA-round-3 B1: the production stage of the Dockerfile must create the
``appuser`` account with UID 1000 / GID 1000. Previously this used
``adduser --system`` which on Debian-slim assigns UID 100 / GID 65534
(nogroup), causing PermissionError when the scraper tried to create new
listing directories on the NFS-backed PVC (owned 1000:1000 mode 775).
"""
from pathlib import Path
REPO_ROOT = Path(__file__).resolve().parents[2]
DOCKERFILE = REPO_ROOT / "Dockerfile"
class TestDockerfileAppUser:
"""The Dockerfile production stage must run as uid 1000 / gid 1000."""
def test_production_stage_creates_user_with_uid_1000(self) -> None:
contents = DOCKERFILE.read_text()
# The fix uses `useradd --uid 1000 --gid 1000` (and a matching
# groupadd) instead of `adduser --system` which would assign uid 100.
assert "--uid 1000" in contents, (
"Dockerfile must create appuser with explicit --uid 1000 to "
"match NFS-backed data PVC ownership"
)
assert "--gid 1000" in contents, (
"Dockerfile must create appuser with explicit --gid 1000"
)
def test_production_stage_does_not_use_adduser_system(self) -> None:
"""`adduser --system` assigns uid 100 — must not be used."""
contents = DOCKERFILE.read_text()
assert "adduser --system" not in contents, (
"Dockerfile must not use `adduser --system` for appuser — "
"it assigns uid 100 which can't write to the 1000:1000 NFS mount"
)

View file

@ -52,6 +52,15 @@ class TestMakeCacheKey:
class TestGetRedisClient: class TestGetRedisClient:
"""Tests for _get_redis_client() URL parsing.""" """Tests for _get_redis_client() URL parsing."""
# Keepalive kwargs added to keep connection alive across the in-cluster
# Redis HAProxy 30s idle timeout. Same options are passed to every
# redis.from_url call in the codebase.
_KEEPALIVE_KWARGS = {
"decode_responses": True,
"socket_keepalive": True,
"health_check_interval": 25,
}
@mock.patch("services.listing_cache.redis") @mock.patch("services.listing_cache.redis")
def test_default_broker_url(self, mock_redis): def test_default_broker_url(self, mock_redis):
"""Uses default localhost URL when env var is not set.""" """Uses default localhost URL when env var is not set."""
@ -59,7 +68,7 @@ class TestGetRedisClient:
_get_redis_client() _get_redis_client()
mock_redis.from_url.assert_called_once_with( mock_redis.from_url.assert_called_once_with(
"redis://localhost:6379/2", decode_responses=True "redis://localhost:6379/2", **self._KEEPALIVE_KWARGS
) )
@mock.patch("services.listing_cache.redis") @mock.patch("services.listing_cache.redis")
@ -71,7 +80,7 @@ class TestGetRedisClient:
_get_redis_client() _get_redis_client()
mock_redis.from_url.assert_called_once_with( mock_redis.from_url.assert_called_once_with(
"redis://myhost:1234/2", decode_responses=True "redis://myhost:1234/2", **self._KEEPALIVE_KWARGS
) )
@mock.patch("services.listing_cache.redis") @mock.patch("services.listing_cache.redis")
@ -84,7 +93,7 @@ class TestGetRedisClient:
_get_redis_client() _get_redis_client()
mock_redis.from_url.assert_called_once_with( mock_redis.from_url.assert_called_once_with(
"redis://:secret@myhost:6379/2", decode_responses=True "redis://:secret@myhost:6379/2", **self._KEEPALIVE_KWARGS
) )
@mock.patch("services.listing_cache.redis") @mock.patch("services.listing_cache.redis")
@ -97,7 +106,7 @@ class TestGetRedisClient:
_get_redis_client() _get_redis_client()
mock_redis.from_url.assert_called_once_with( mock_redis.from_url.assert_called_once_with(
"redis://myhost:6379/2?timeout=5", decode_responses=True "redis://myhost:6379/2?timeout=5", **self._KEEPALIVE_KWARGS
) )

View file

@ -1,4 +1,5 @@
"""Unit tests for tasks/listing_tasks.py.""" """Unit tests for tasks/listing_tasks.py."""
import asyncio
import json import json
import os import os
from collections import deque from collections import deque
@ -10,6 +11,7 @@ import tasks.listing_tasks as module
from tasks.listing_tasks import ( from tasks.listing_tasks import (
_update_task_state, _update_task_state,
_PipelineState, _PipelineState,
_process_worker,
TaskLogHandler, TaskLogHandler,
SCRAPE_LOCK_NAME, SCRAPE_LOCK_NAME,
LOG_BUFFER_MAX_LINES, LOG_BUFFER_MAX_LINES,
@ -18,6 +20,7 @@ from tasks.listing_tasks import (
PHASE_FETCHING, PHASE_FETCHING,
PHASE_PROCESSING, PHASE_PROCESSING,
PHASE_COMPLETED, PHASE_COMPLETED,
dump_listings_task,
) )
@ -293,3 +296,174 @@ class TestPhaseConstants:
def test_num_workers(self): def test_num_workers(self):
assert NUM_WORKERS == 20 assert NUM_WORKERS == 20
# ---------------------------------------------------------------------------
# Regression tests for QA-round-3 backend bugs (B5, B6, B20)
# ---------------------------------------------------------------------------
class TestProcessWorkerExceptionHandling:
"""B6 regression: _process_worker must keep draining the queue when a
single listing raises an unhandled exception (e.g. PermissionError).
Previously one bad listing aborted the entire scrape."""
async def test_continues_after_per_listing_exception(self):
"""A PermissionError from one listing should not stop sibling listings."""
# Three listings in the queue followed by a None sentinel.
queue: asyncio.Queue[int | None] = asyncio.Queue()
for listing_id in [1, 2, 3]:
await queue.put(listing_id)
await queue.put(None)
# Processor: listing 1 succeeds, listing 2 raises, listing 3 succeeds.
good_listing = MagicMock()
async def fake_process_listing(listing_id, on_step_complete=None):
if listing_id == 2:
raise PermissionError("Permission denied: data/rs/2")
return good_listing
processor = MagicMock()
processor.process_listing = AsyncMock(side_effect=fake_process_listing)
state = _PipelineState()
reporter = MagicMock()
await _process_worker(queue, processor, state, reporter)
# All three IDs were attempted (queue drained before exit).
assert processor.process_listing.call_count == 3
# Two succeeded, one failed.
assert state.processed_count == 2
assert state.failed_count == 1
assert len(state.processed_listings) == 2
async def test_cancelled_error_propagates(self):
"""CancelledError must NOT be swallowed — cooperative cancellation
relies on it propagating up through asyncio.gather()."""
queue: asyncio.Queue[int | None] = asyncio.Queue()
await queue.put(99)
# No sentinel — the worker should bubble the CancelledError before
# ever getting a chance to drain further.
processor = MagicMock()
processor.process_listing = AsyncMock(side_effect=asyncio.CancelledError())
state = _PipelineState()
reporter = MagicMock()
with pytest.raises(asyncio.CancelledError):
await _process_worker(queue, processor, state, reporter)
class TestDumpListingsTaskFailurePublish:
"""B5 regression: dump_listings_task must publish a terminal FAILURE
event to the task_progress:<id> pub/sub channel when the underlying
scrape raises an exception. Previously only the happy-path SUCCESS
was published, leaving WebSocket subscribers stuck on the last
progress packet."""
@patch("tasks.listing_tasks.publish_task_progress")
@patch("tasks.listing_tasks.asyncio.run")
@patch("tasks.listing_tasks.redis_lock")
def test_publishes_failure_event_on_exception(
self, mock_redis_lock, mock_asyncio_run, mock_publish
):
"""When dump_listings_full raises, a FAILURE event is published."""
mock_cm = MagicMock()
mock_cm.__enter__ = MagicMock(return_value=True)
mock_cm.__exit__ = MagicMock(return_value=False)
mock_redis_lock.return_value = mock_cm
mock_asyncio_run.side_effect = PermissionError(
"[Errno 13] Permission denied: 'data/rs/12345'"
)
dump_listings_task.update_state = MagicMock()
# Force a deterministic task_id so we can assert on it.
with patch.object(
type(dump_listings_task),
"request",
new=MagicMock(id="fake-task-id"),
create=True,
):
with pytest.raises(PermissionError):
dump_listings_task.run(
'{"listing_type": "RENT", "min_price": 1000, "max_price": 5000}'
)
# Inspect publish_task_progress calls for a FAILURE event.
failure_calls = [
c for c in mock_publish.call_args_list
if len(c.args) >= 2 and c.args[1] == "FAILURE"
]
assert failure_calls, (
f"Expected a FAILURE publish, got: "
f"{[c.args[1] for c in mock_publish.call_args_list if len(c.args) >= 2]}"
)
# The meta payload must include an error message.
meta = failure_calls[0].args[2]
assert "error" in meta
assert "Permission denied" in meta["error"]
assert meta["exc_type"] == "PermissionError"
class TestDumpListingsTaskDecoratorConfig:
"""B20 regression: dump_listings_task must have time_limit /
soft_time_limit / acks_late configured so dead tasks reap themselves
even after pickup."""
def test_task_has_time_limits(self):
# Celery exposes these via the task attributes once decorated.
assert dump_listings_task.time_limit == 3600
assert dump_listings_task.soft_time_limit == 3500
def test_task_acks_late(self):
assert dump_listings_task.acks_late is True
class TestCeleryAppKeepaliveOptions:
"""B4 regression: broker / result-backend transport options must
enable TCP keepalive and a Celery-level health check so the Redis
HAProxy in front of the in-cluster Sentinel doesn't reap idle
connections every 30s."""
def test_broker_transport_options_present(self):
from celery_app import app as celery_app
opts = celery_app.conf.get("broker_transport_options") or {}
assert opts.get("socket_keepalive") is True
assert opts.get("health_check_interval") == 25
def test_result_backend_transport_options_present(self):
from celery_app import app as celery_app
opts = celery_app.conf.get("result_backend_transport_options") or {}
assert opts.get("socket_keepalive") is True
assert opts.get("health_check_interval") == 25
class TestRedisClientKeepalive:
"""B4 regression: every helper that creates a Redis client must
pass socket_keepalive=True and health_check_interval=25."""
@patch("services.task_progress_publisher.redis")
def test_task_progress_publisher_uses_keepalive(self, mock_redis):
# Reset the cached client so the patch takes effect.
import services.task_progress_publisher as m
m._redis_client = None
m._get_redis_client()
mock_redis.Redis.from_url.assert_called_once()
kwargs = mock_redis.Redis.from_url.call_args.kwargs
assert kwargs["socket_keepalive"] is True
assert kwargs["health_check_interval"] == 25
m._redis_client = None # leave the module clean for other tests
@patch("utils.redis_lock.redis")
def test_redis_lock_uses_keepalive(self, mock_redis):
from utils.redis_lock import get_redis_client
get_redis_client()
mock_redis.from_url.assert_called_once()
kwargs = mock_redis.from_url.call_args.kwargs
assert kwargs["socket_keepalive"] is True
assert kwargs["health_check_interval"] == 25

View file

@ -6,69 +6,103 @@ import pytest
from utils.redis_lock import redis_lock, get_redis_client from utils.redis_lock import redis_lock, get_redis_client
def _setup_client(mock_get_client: mock.MagicMock, set_return: object = True) -> mock.MagicMock:
"""Return a MagicMock redis client wired up for the lock helper."""
mock_client = mock.MagicMock()
mock_client.set.return_value = set_return
mock_get_client.return_value = mock_client
return mock_client
class TestRedisLock: class TestRedisLock:
"""Tests for redis_lock context manager.""" """Tests for redis_lock context manager."""
@mock.patch("utils.redis_lock.get_redis_client") @mock.patch("utils.redis_lock.get_redis_client")
def test_lock_acquired_successfully(self, mock_get_client): def test_lock_acquired_successfully(self, mock_get_client):
"""Test lock acquisition when no other lock exists.""" """Test lock acquisition when no other lock exists."""
mock_client = mock.MagicMock() mock_client = _setup_client(mock_get_client)
mock_client.set.return_value = True
mock_get_client.return_value = mock_client
with redis_lock("test_lock") as acquired: with redis_lock("test_lock") as acquired:
assert acquired is True assert acquired is True
mock_client.set.assert_called_once_with("lock:test_lock", "1", nx=True, ex=3600 * 4) # Lock is set with the owner UUID, nx=True, and the configured TTL.
mock_client.delete.assert_called_once_with("lock:test_lock") assert mock_client.set.call_count == 1
args, kwargs = mock_client.set.call_args
assert args[0] == "lock:test_lock"
assert isinstance(args[1], str) and len(args[1]) == 32 # uuid4 hex
assert kwargs == {"nx": True, "ex": 3600 * 4}
# Release happens via register_script (Lua CAS), not raw DEL.
mock_client.register_script.assert_called_once()
# The script wrapper is called once with the lock key and owner token.
release_script = mock_client.register_script.return_value
release_script.assert_called_once()
call_args = release_script.call_args
assert call_args.kwargs["keys"] == ["lock:test_lock"]
assert call_args.kwargs["args"][0] == args[1] # same owner token
@mock.patch("utils.redis_lock.get_redis_client") @mock.patch("utils.redis_lock.get_redis_client")
def test_lock_not_acquired(self, mock_get_client): def test_lock_not_acquired(self, mock_get_client):
"""Test lock not acquired when another lock exists.""" """Test lock not acquired when another lock exists."""
mock_client = mock.MagicMock() # Redis returns None when nx=True fails
mock_client.set.return_value = None # Redis returns None when nx=True fails mock_client = _setup_client(mock_get_client, set_return=None)
mock_get_client.return_value = mock_client
with redis_lock("test_lock") as acquired: with redis_lock("test_lock") as acquired:
assert acquired is False assert acquired is False
mock_client.set.assert_called_once_with("lock:test_lock", "1", nx=True, ex=3600 * 4) # Should NOT register or invoke the release script since we didn't acquire.
# Should NOT call delete since we didn't acquire the lock mock_client.register_script.assert_not_called()
mock_client.delete.assert_not_called()
@mock.patch("utils.redis_lock.get_redis_client") @mock.patch("utils.redis_lock.get_redis_client")
def test_lock_released_on_exception(self, mock_get_client): def test_lock_released_on_exception(self, mock_get_client):
"""Test lock is released even when exception occurs.""" """Test lock is released even when exception occurs."""
mock_client = mock.MagicMock() mock_client = _setup_client(mock_get_client)
mock_client.set.return_value = True
mock_get_client.return_value = mock_client
with pytest.raises(ValueError): with pytest.raises(ValueError):
with redis_lock("test_lock") as acquired: with redis_lock("test_lock") as acquired:
assert acquired is True assert acquired is True
raise ValueError("Test error") raise ValueError("Test error")
# Lock should still be released # Lock should still be released via the Lua CAS script.
mock_client.delete.assert_called_once_with("lock:test_lock") mock_client.register_script.assert_called_once()
mock_client.register_script.return_value.assert_called_once()
@mock.patch("utils.redis_lock.get_redis_client") @mock.patch("utils.redis_lock.get_redis_client")
def test_custom_timeout(self, mock_get_client): def test_custom_timeout(self, mock_get_client):
"""Test lock with custom timeout.""" """Test lock with custom timeout."""
mock_client = mock.MagicMock() mock_client = _setup_client(mock_get_client)
mock_client.set.return_value = True
mock_get_client.return_value = mock_client
with redis_lock("test_lock", timeout=300) as acquired: with redis_lock("test_lock", timeout=300) as acquired:
assert acquired is True assert acquired is True
mock_client.set.assert_called_once_with("lock:test_lock", "1", nx=True, ex=300) # Only one SET call with the configured TTL.
args, kwargs = mock_client.set.call_args
assert args[0] == "lock:test_lock"
assert kwargs == {"nx": True, "ex": 300}
@mock.patch("utils.redis_lock.get_redis_client")
def test_owner_token_is_unique_per_acquisition(self, mock_get_client):
"""Each acquisition gets a fresh UUID owner token (fencing token)."""
mock_client = _setup_client(mock_get_client)
with redis_lock("test_lock"):
pass
token_first = mock_client.set.call_args[0][1]
with redis_lock("test_lock"):
pass
token_second = mock_client.set.call_args[0][1]
assert token_first != token_second
@mock.patch("utils.redis_lock.redis") @mock.patch("utils.redis_lock.redis")
def test_get_redis_client_uses_broker_url(self, mock_redis): def test_get_redis_client_uses_broker_url(self, mock_redis):
"""Test Redis client is created from CELERY_BROKER_URL.""" """Test Redis client is created from CELERY_BROKER_URL with keepalive."""
with mock.patch.dict("os.environ", {"CELERY_BROKER_URL": "redis://testhost:1234/5"}): with mock.patch.dict("os.environ", {"CELERY_BROKER_URL": "redis://testhost:1234/5"}):
get_redis_client() get_redis_client()
mock_redis.from_url.assert_called_once_with( mock_redis.from_url.assert_called_once_with(
"redis://testhost:1234/5", decode_responses=True "redis://testhost:1234/5",
decode_responses=True,
socket_keepalive=True,
health_check_interval=25,
) )

View file

@ -0,0 +1,96 @@
"""Unit tests for redis_repository.RedisRepository.
Regression coverage for QA-round-3 B21: user state must live on a dedicated
Redis logical DB (default db3) with all keys prefixed by ``wrongmove:user:``.
"""
from datetime import timedelta
from unittest import mock
import pytest
def _patched_app() -> mock.MagicMock:
"""Return a MagicMock standing in for celery_app.app.broker_connection()."""
fake_app = mock.MagicMock()
fake_app.broker_connection.return_value.info.return_value = {
"hostname": "redis.test",
"port": 6379,
}
return fake_app
class TestRedisRepositoryDbSelection:
"""B21 regression: RedisRepository writes to a dedicated DB (default 3)."""
def test_defaults_to_db_3(self):
import redis_repository
with mock.patch.dict("os.environ", {}, clear=False) as _env:
_env.pop("REDIS_USER_DB", None)
with mock.patch.object(redis_repository, "redis") as mock_redis, \
mock.patch.object(redis_repository, "app", _patched_app()):
redis_repository.RedisRepository()
kwargs = mock_redis.Redis.call_args.kwargs
assert kwargs["db"] == 3
assert kwargs["socket_keepalive"] is True
assert kwargs["health_check_interval"] == 25
def test_honours_REDIS_USER_DB_env_var(self):
import redis_repository
with mock.patch.dict("os.environ", {"REDIS_USER_DB": "7"}):
with mock.patch.object(redis_repository, "redis") as mock_redis, \
mock.patch.object(redis_repository, "app", _patched_app()):
redis_repository.RedisRepository()
kwargs = mock_redis.Redis.call_args.kwargs
assert kwargs["db"] == 7
class TestRedisRepositoryKeyPrefix:
"""B21 regression: all keys written via set_key/get_key are namespaced
with the ``wrongmove:user:`` prefix to avoid collisions on a shared
Redis instance."""
def _fresh_repo(self) -> tuple[object, mock.MagicMock]:
import redis_repository
with mock.patch.object(redis_repository, "redis"), \
mock.patch.object(redis_repository, "app", _patched_app()):
repo = redis_repository.RedisRepository()
fake_client = mock.MagicMock()
repo.redis_client = fake_client
return repo, fake_client
def test_set_key_prepends_prefix(self):
repo, fake_client = self._fresh_repo()
repo.set_key("webauthn:challenge:abc", {"x": 1})
# First positional arg to .set is the key.
set_call_key = fake_client.set.call_args.args[0]
assert set_call_key == "wrongmove:user:webauthn:challenge:abc"
# expire uses the same prefixed key.
expire_call_key = fake_client.expire.call_args.args[0]
assert expire_call_key == "wrongmove:user:webauthn:challenge:abc"
def test_get_key_uses_prefix(self):
repo, fake_client = self._fresh_repo()
fake_client.get.return_value = '{"hello": "world"}'
result = repo.get_key("webauthn:challenge:abc")
fake_client.get.assert_called_once_with(
"wrongmove:user:webauthn:challenge:abc"
)
assert result == {"hello": "world"}
def test_does_not_double_prefix_already_prefixed_key(self):
repo, fake_client = self._fresh_repo()
repo.set_key("wrongmove:user:already-prefixed", {"x": 1})
set_call_key = fake_client.set.call_args.args[0]
assert set_call_key == "wrongmove:user:already-prefixed"
def test_add_task_for_user_uses_prefixed_key(self):
repo, fake_client = self._fresh_repo()
fake_client.get.return_value = None # no prior tasks
from api.auth import User
user = User(sub="", email="test@example.com", name="")
repo.add_task_for_user(user, "task-xyz")
# The redis SET should target the namespaced key.
set_call_key = fake_client.set.call_args.args[0]
assert set_call_key == "wrongmove:user:test@example.com/tasks"

View file

@ -1,6 +1,7 @@
"""Redis-based distributed locking for task coordination.""" """Redis-based distributed locking for task coordination."""
import logging import logging
import os import os
import uuid
from contextlib import contextmanager from contextlib import contextmanager
from typing import Generator from typing import Generator
@ -8,18 +9,35 @@ import redis
logger = logging.getLogger("uvicorn.error") logger = logging.getLogger("uvicorn.error")
# Lua compare-and-delete script: only DEL the key if its current value
# matches our owner token. This prevents a process that lost the lock
# (e.g. via TTL expiry) from accidentally releasing a different acquirer's
# lock.
_RELEASE_SCRIPT = (
"if redis.call('GET', KEYS[1]) == ARGV[1] then "
"return redis.call('DEL', KEYS[1]) "
"else return 0 end"
)
def get_redis_client() -> redis.Redis: def get_redis_client() -> redis.Redis:
"""Get Redis client from Celery broker URL.""" """Get Redis client from Celery broker URL."""
broker_url = os.getenv("CELERY_BROKER_URL", "redis://localhost:6379/0") broker_url = os.getenv("CELERY_BROKER_URL", "redis://localhost:6379/0")
return redis.from_url(broker_url, decode_responses=True) # socket_keepalive + health_check_interval keep the connection alive
# across the Redis HAProxy 30s idle timeout (see celery_app.py).
return redis.from_url(
broker_url,
decode_responses=True,
socket_keepalive=True,
health_check_interval=25,
)
@contextmanager @contextmanager
def redis_lock( def redis_lock(
lock_name: str, timeout: int = 3600 * 4 lock_name: str, timeout: int = 3600 * 4
) -> Generator[bool, None, None]: ) -> Generator[bool, None, None]:
"""Distributed lock using Redis. """Distributed lock using Redis with an owner-fencing token.
Args: Args:
lock_name: Unique name for the lock lock_name: Unique name for the lock
@ -37,14 +55,23 @@ def redis_lock(
""" """
client = get_redis_client() client = get_redis_client()
lock_key = f"lock:{lock_name}" lock_key = f"lock:{lock_name}"
# Per-acquirer fencing token: only the holder can release the lock.
owner_token = uuid.uuid4().hex
# Try to acquire the lock # Try to acquire the lock; store the owner token as the value.
acquired = client.set(lock_key, "1", nx=True, ex=timeout) acquired = client.set(lock_key, owner_token, nx=True, ex=timeout)
try: try:
yield bool(acquired) yield bool(acquired)
finally: finally:
# Release the lock only if we acquired it # Release the lock only if we acquired it AND we still own it.
# The Lua compare-and-delete guards against the case where our TTL
# expired and a different process picked up the lock; we won't
# accidentally delete their key.
if acquired: if acquired:
client.delete(lock_key) try:
logger.info(f"Released lock: {lock_name}") release = client.register_script(_RELEASE_SCRIPT)
release(keys=[lock_key], args=[owner_token])
logger.info(f"Released lock: {lock_name}")
except redis.RedisError as e:
logger.warning(f"Failed to release lock {lock_name}: {e}")