Venue finder engineering audit — 2026-09-04
Status: Draft · engineering audit of the venue finder, low-risk fixes applied in the same commit
Scope: VenueFinder (packages/app/src/presentation/components/sessions/venue-finder.tsx), VenueSearchResultsList (packages/app/src/presentation/components/venue-search-results-list.tsx), use-venue-directory.ts, use-venue-view-counts.ts, use-saved-venues.ts, use-adopt-and-save-venue.ts, venue-directory.supabase.ts, the search_venues_ranked/count_venues_by_view RPCs, CourtsDirectoryScreen, VenueSearchTakeover. Rubric: docs/canon/data-and-hooks.md, docs/canon/networking.md, docs/canon/architecture.md, DEVELOPMENT_GUIDELINES.md, plus the standing memory rules (JSON-safe queryFns, edit-scope publish contract, no localeCompare/toLocale* on Hermes, FeedList recycling={false}).
Every item is grounded in the live code read for this audit — file:line citations point at the code as of this checkout (commit 0c5bb94c).
Edit-scope note: per the brief, code fixes were applied ONLY in use-venue-directory.ts, use-venue-view-counts.ts, venue-directory.supabase.ts, venue-search-results-list.tsx, and their __tests__. Findings that live in venue-finder.tsx, venue-filter-bar.tsx, directory-venue-card.tsx, or i18n are reported only — another agent owns those files.
Findings, ranked by severity
1. HIGH (fixed) — useVenueDirectoryAll skipped placeholderData: keepPreviousData, the one sibling hook in the file that did
File: packages/app/src/presentation/hooks/queries/use-venue-directory.ts:109-153 (now :109-160).
Before: useVenueDirectory (line 40-50) and useVenueDirectoryInfinite (line 64-91) both set placeholderData: keepPreviousData specifically for "keystroke/filter-switch continuity (state-grammar pass, 2026-08-29)" — every other query-key change keeps the previous rows visible under an isFetching cue instead of flipping isLoading back to true and swapping to the skeleton. useVenueDirectoryAll — the hook VenueFinder's browse-mode directoryQuery actually calls (venue-finder.tsx:384) — was missing it, despite keepPreviousData already being imported in the same file (line 1).
Impact: every nav-chip tap (전체/코트/연습장/샵), region/district filter change, indoor toggle, or verification-mode toggle in VenueFinder's default browse view (the MOST common surface — every explore mount and the club wizard's multi-pick usage per the file header) re-triggered isLoading=true and swapped the populated list for the 3-card skeleton (venue-finder.tsx:733-739), even though the softer isFetching-driven cue exists elsewhere in this exact codebase for exactly this class of transition. A visible flicker/regression on the highest-traffic interaction in the screen.
Fix applied: added placeholderData: keepPreviousData to useVenueDirectoryAll's useQuery config.
Test: use-venue-directory.test.ts — "keeps the previous result set visible (placeholderData) across a filter change instead of flashing back to isLoading" (asserts isLoading stays false and stale data stays present across a facilityView change before the new page resolves).
2. MEDIUM (fixed) — nav-chip counts used the realtime (60s) stale tier for data the same file's own docs call "days/weeks"-stable
File: packages/app/src/presentation/hooks/queries/use-venue-view-counts.ts:14-30 (before fix).
Before: staleTime: STALE_TIME.realtime (60s), with a doc comment claiming "same tier as the rest of the venue directory's browse surfaces." That claim was false: useVenueDirectory, useVenueDirectoryAll, and useVenueDirectoryInfinite — the sibling hooks backing the exact same browse surfaces — all use STALE_TIME.stable (15 min), each with an explicit "@freshness stable — platform-ingested venue spine … changes on the order of days/weeks" comment.
Impact: the nav-chip count RPC (count_venues_by_view) refetched on every refocus/remount past the 60s window even though the underlying spine is documented elsewhere as changing on the order of days, not minutes — pure extra load on a SECURITY DEFINER RPC with no freshness benefit. Minor but a clean violation of "efficient pipelines … cheapest sufficient rung" and the networking canon's staleTime discipline (data freshness tier should match the actual mutation cadence of the source).
Fix applied: changed to STALE_TIME.stable, corrected the doc comment to state the actual prior mismatch and match it explicitly against the sibling hooks' tier.
Test: use-venue-view-counts.test.ts covers filter pass-through, the enabled: false focus-gate contract, and the placeholderData continuity behavior (all three previously untested — this hook had zero coverage before this audit).
3. MEDIUM (fixed) — predicate mismatch: the plain (no-typed-query) browse branch didn't exclude null-coordinate rows that both server RPCs already exclude
File: packages/app/src/adapters/supabase/venue-directory.supabase.ts:178-183 (before fix); RPC comparison: supabase/migrations/00569_provider_names_as_aliases.sql:218-219 (search_venues_ranked's candidate CTE) and supabase/migrations/00574_floor_evidence_court_veto.sql:1133-1134 (count_venues_by_view's candidate CTE) — both gate on v.latitude IS NOT NULL AND v.longitude IS NOT NULL.
Before: the adapter's plain .from('venues') browse builder (used whenever VenueFinder's search box is empty — the default view and every 즐겨찾기/browse-mode load) had NO equivalent filter. DirectoryVenue.latitude/longitude are documented elsewhere in this same codebase as nullable — "a small unresolved tail of the ingested spine" (venue-search-results-list.tsx:104-111's comment on the exact same nullability).
Impact: this is precisely the parity the audit brief called out as a hard requirement ("whether count_venues_by_view runs with the same predicate as the list — it MUST"). Before the fix, it didn't, in one direction: a null-coordinate venue could appear in a plain browse page while (a) being permanently unreachable via any typed search (the ranked RPC already excludes it) and (b) being excluded from the RPC-backed nav-chip count that describes that very page — so the "전체" chip count could read lower than the actual number of cards rendered under it. A card for one of these rows also renders no map thumbnail/distance (already handled gracefully at the row level), so excluding it from browse loses nothing the UI was actually using.
Fix applied: added .not('latitude', 'is', null).not('longitude', 'is', null) to the shared builder chain, scoped correctly (verified this chain is NOT reused by the ranked-search hydration read, which builds its own .select(columns).in('id', rankedIds) query off the RPC's already-filtered id list — no double filtering, no wasted round-trip).
Test: venue-directory.supabase.test.ts — new describe block asserts (a) the plain branch calls .not() for both latitude and longitude before .range() slices the page, and (b) the ranked-branch hydration query does NOT re-apply the filter (a mock object exposing only .select().in() — the code would throw if it tried to chain .not() there).
4. MEDIUM (not fixed — out of edit scope, venue-finder.tsx) — search-mode results render in an un-virtualized ScrollView under infinite pagination
File: packages/app/src/presentation/components/sessions/venue-finder.tsx:673-715 (the ScrollView search-mode branch) wrapping VenueSearchResultsList with progressiveReveal={false} and an infinite-scroll onScroll handler that calls spineSearchQuery.fetchNextPage().
Why it's a real risk, not just a style nit: the browse branch explicitly moved OFF SectionList onto a FlashList-backed FeedList because SectionList "processes ALL ~1,875 items synchronously … froze the JS thread for 2.5–3.5s on load" (comment at venue-finder.tsx:158-162, device-measured). Search mode uses a plain ScrollView, which is unwindowed by construction — EVERY loaded page (30 rows at a time via VENUE_DIRECTORY_PAGE_SIZE) stays mounted as the user scrolls, and there is no upper bound: a broad, low-specificity query (e.g., a bare "테니스" against the ~1,900-row corroborated spine) can legitimately page through hundreds of rows if the user keeps scrolling. Each row (VenueSearchResultRow + its bookmark/adopt trailing) is not itself a cheap leaf — it renders through the same renderSaveTrailing machinery that recomputes adopt.resolveVenueId/pendingRowId lookups on every render. Suggested fix (needs the venue-finder.tsx owner): swap the search-mode body onto a virtualized list (FeedList/FlashList) the same way browse mode already did, with the infinite-scroll trigger moved to the list's own onEndReached instead of a manual scroll-offset listener. This is a real architecture change (new list host, keyExtractor/getItemType for the mixed group-header/row/naver-section shape VenueSearchResultsList currently renders as one flat YStack), not a one-line fix — hence reported, not applied, given the "low-risk fixes only" scope for this pass.
5. LOW (not fixed — out of edit scope, venue-finder.tsx) — Naver adopt-on-pick double-emit has a narrow same-tick re-entrancy window
File: packages/app/src/presentation/components/sessions/venue-finder.tsx:578-590 (selectSearchedVenue) and packages/app/src/presentation/hooks/use-adopt-and-save-venue.ts:95-118 (adopt); the guard lives in venue-search-results-list.tsx:296-299 (handleRowPress, in-scope but the guard itself is already correct — see below).
What's already right: handleRowPress checks if (v.id === adopt.pendingRowId) return; before calling onRowPress, and adopt() sets pendingRowId synchronously via setPendingRowId(v.id) before the mutation fires — this is the correct shape for the common case.
The narrow gap: pendingRowId only becomes visible to handleRowPress's closure after React commits the state update and re-renders. Two onPress calls dispatched in the same JS macrotask (a fast double-tap gesture registering as two discrete press events before the first render commits) would both read the SAME stale pendingRowId (null) and both pass the guard, firing venueAdopt.adopt() twice for the same row — two useAdoptNaverVenue mutations in flight for one Naver place. Because that mutation is dedup'd server-side on naver_place_id (per use-adopt-and-save-venue.ts's header comment), this would not corrupt data, but it does mean two network round-trips and two onResolved callbacks racing to call recordAdoptedId/onPick — a benign but wasteful race, not a correctness bug given the server-side dedup backstop. Suggested fix: disable the row's Pressable (not just skip the callback) while v.id === adopt.pendingRowId, so a second tap physically can't dispatch a second press event — closes the gap at the input layer instead of relying on state-commit timing. This touches VenueSearchResultRow (not in this pass's edit scope) to accept a disabled prop threaded from VenueSearchResultsList.
6. LOW (not fixed — out of edit scope, venue-finder.tsx) — nav chips get the generic button accessibility role, not tab
File: packages/app/src/presentation/components/sessions/venue-finder.tsx:642-662 (the NAV_CHIPS.map rendering SelectionChip) — no accessibilityRole prop is passed, so Pressable's default (packages/ui/src/pressable.tsx:153, accessibilityRole = 'button') applies. accessibilityState={{selected, disabled}} IS correctly wired (packages/ui/src/selection-chip.tsx:82), so a screen reader does announce the selected state — the gap is only the role, which for a row of mutually-exclusive view-switching chips (전체/코트/연습장/샵/즐겨찾기) is more accurately tab than button. Severity note: genuinely low — button + accessibilityState.selected is a common, understandable pattern (a "selected toggle button"), just not the most precise ARIA-equivalent role for this specific navigation-chip-row shape. Suggested fix: pass accessibilityRole="tab" on each SelectionChip in the nav-chip row (one-line change, venue-finder.tsx only — out of this pass's edit scope).
7. LOW (informational, already correct) — focus-gating is consistently applied
Checked every query VenueFinder fires: useSavedVenueIds (enabled: active), useVenueViewCounts (enabled: active), directoryQuery/useVenueDirectoryAll (enabled: active && !searchModeActive && !savedView), spineSearchQuery/useVenueDirectoryInfinite (enabled: searchModeActive, which itself is active && … — transitively gated), naverQuery/usePublicCourts (enabled: naverTailEnabled, transitively gated through searchModeActive). No query fires while the tab pane is unfocused (useTabPaneActive()). No fix needed — called out because the audit brief asked to check for exactly this class of bug and it was NOT found.
8. LOW (informational, already correct) — buildRows and regionKeyForVenue are not O(n²)
buildRows (venue-finder.tsx:169-207) groups ~1,900 venues into a Map (O(n)), sorts the resulting ~17 province groups (O(n log n) on a tiny n), and relies on the adapter's .order('name') for in-group ordering rather than re-sorting per section — explicitly to avoid localeCompare('ko')'s ICU cost (commented in-line, consistent with the standing "no localeCompare on Hermes" rule). regionKeyForVenue (venue-finder.tsx:540-549) is a bounded loop over ≤17 SIDO keys per Naver row, not per browse row. Neither is a hot spot at current scale.
9. LOW (informational, already correct) — count_venues_by_view predicate now matches the list on every remaining axis
Beyond the null-coordinate gap fixed in #3, region/district/indoor/capabilities/verification predicates were already byte-for-byte consistent between search_venues_ranked (00569) and count_venues_by_view (00574) — confirmed by reading both live SQL bodies side by side. VenueFinder also passes the identical otherFilters.verification value (defaulting to 'all' per emptyVenueFilters(), venue-filter-bar.tsx:71) to both the list query and the counts query — no drift between what a chip promises and what tapping it shows.
10. LOW (informational, already correct) — memoization chain (DirectoryVenueCard) holds
DirectoryVenueCard is React.memo-wrapped (directory-venue-card.tsx:229) with a stable onPress (venue-finder.tsx:440-449, useCallback with narrow deps) and a selected prop computed as a plain boolean per row rather than passed by reference — so even if a caller (e.g. a wizard) passes a fresh pickedVenueIds array each render, the per-card memo comparison still bails correctly since only the resulting boolean value is compared, not the array. VenueSearchResultRow (search-mode rows) is NOT memoized, but result counts there are currently bounded by progressive reveal/pagination — see #4 for the one scenario (unbounded infinite-scroll growth) where this could start to matter.
Test coverage gap closed
Before this pass, the entire use-venue-directory.ts/use-venue-view-counts.ts/venue-search-results-list.tsx cluster had zero unit tests — the only two test files touching this area (venue-directory.supabase.test.ts, venue-directory-empty-state.test.ts) covered a single prior regression (the id-tiebreak page-boundary bug) and the empty-state selector, respectively. A regression in the pagination early-stop loop, the cross-page dedup, the placeholderData continuity contract, the RPC predicate parity, or the entire loading/error/suggest/no-results/progressive-reveal/pending-guard state grammar of VenueSearchResultsList would previously have shipped silently. New coverage added this pass:
use-venue-directory.test.ts—useVenueDirectoryAll's early-stop pagination (fetches exactly as many pages as needed, not an unconditional sweep), cross-page id dedup, and the newplaceholderDatacontinuity.use-venue-view-counts.test.ts— filter pass-through (predicate parity with the list), theenabled: falsefocus-gate contract, andplaceholderDatacontinuity.venue-directory.supabase.test.ts(extended) — the null-coordinate predicate-parity fix, and that the ranked-search hydration leg does NOT double-apply it.venue-search-results-list.test.tsx(new file) — loading/error states, the "in-flight refetch over zero stale rows must render the skeleton, never the no-results empty state" state-grammar law, the suggest-phase empty state, progressive reveal (4-then-more slicing across curated groups), infinite mode (progressiveReveal=false, no reveal control,isLoadingMoretrailing skeleton), the announced Naver-pending skeleton section, the per-rowadopt.pendingRowIdguard (both the ignored-tap and normal-tap cases), and empty-group dropping.
Still open (not addressed by this pass — out of edit scope or requires the larger architecture change in #4): venue-finder.tsx itself (buildRows, regionKeyForVenue, the pick-result mappers, the double-emit selectSearchedVenue flow) has no direct unit coverage; use-adopt-and-save-venue.ts's adopt/adoptAndSave state machine (pending cue, overlay map, error rollback) has no direct unit coverage either — both are reachable indirectly through venue-search-results-list.test.tsx's pending-guard test but deserve their own focused suite. use-saved-venues.ts's optimistic toggle mutation also has no direct test. These are reasonable next candidates but were left to the file's owning agent / a follow-up pass given this pass's scope boundary.
Files changed this pass
packages/app/src/presentation/hooks/queries/use-venue-directory.ts— addedplaceholderData: keepPreviousDatatouseVenueDirectoryAll.packages/app/src/presentation/hooks/queries/use-venue-view-counts.ts—staleTimerealtime→stable; corrected the doc comment.packages/app/src/adapters/supabase/venue-directory.supabase.ts— added the null-coordinate predicate to the plain browse branch, matching both RPCs.packages/app/src/presentation/hooks/queries/__tests__/use-venue-directory.test.ts— new.packages/app/src/presentation/hooks/queries/__tests__/use-venue-view-counts.test.ts— new.packages/app/src/presentation/components/__tests__/venue-search-results-list.test.tsx— new.packages/app/src/adapters/supabase/__tests__/venue-directory.supabase.test.ts— extended with the predicate-paritydescribeblock.
Verified: yarn workspace @twomore/app run typecheck (clean), the four touched/added Jest files (23/23 passing), and npx eslint --max-warnings 0 on all seven changed/added source+test files (clean). Not committed — left for the caller to review and commit per instructions.