Conversation
There was a problem hiding this comment.
Pull request overview
This PR focuses on stabilizing search/browse results behavior by unifying the results pages and standardizing map initialization/search configuration, while also improving Typesense query behavior and enriching SF OpenData service data.
Changes:
- Replace
SearchResultsPage+BrowseResultsPagewith a unifiedResultsPageused for both/searchand/:categorySlug/results. - Standardize initial map view to fixed San Francisco bounds and update geo/search configuration flow accordingly.
- Improve Typesense relevance/sorting defaults and ingest
application_processfrom SF OpenData into the app’sServicemodel.
Reviewed changes
Copilot reviewed 14 out of 15 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| app/utils/location.tsx | Adds SF_MAP_BOUNDS constant used to standardize the initial SF map viewport. |
| app/search/providers/typesense/TypesenseProvider.ts | Expands query_by fields and enables sort_by for more stable ordering. |
| app/search/tests/searchFlows.test.tsx | Updates integration tests to use the unified ResultsPage. |
| app/pages/ServiceDetailPage/ServiceDetailPage.module.scss | Adjusts label sizing/layout to improve UI stability/alignment. |
| app/pages/SearchResultsPage/SearchResultsPage.tsx | Removes the legacy search-only results page implementation. |
| app/pages/SearchResultsPage/SearchResultsPage.module.scss | Removes legacy styling for the removed page. |
| app/pages/ResultsPage/ResultsPage.tsx | Introduces unified results page with shared geo + pagination + header behavior for search and browse. |
| app/pages/ResultsPage/ResultsPage.test.tsx | Updates/extends unit tests for unified search and browse modes. |
| app/pages/ResultsPage/ResultsPage.module.scss | Adds styling for the new unified page (loader container). |
| app/pages/BrowseResultsPage/BrowseResultsPage.tsx | Removes the legacy browse-only results page implementation. |
| app/hooks/SFOpenData/types.ts | Adds application_process to the SF OpenData service type. |
| app/hooks/SFOpenData/transformers.ts | Maps application_process through to the internal Service model. |
| app/components/ui/SearchResultsHeader.tsx | Switches HITS_PER_PAGE import to shared search constants. |
| app/components/SearchAndBrowse/SearchMap/SearchMap.tsx | Initializes map to SF bounds and captures bounds for search bounding box behavior. |
| app/Router.tsx | Routes both search and browse paths to the new unified ResultsPage. |
Comments suppressed due to low confidence (1)
app/pages/ResultsPage/ResultsPage.test.tsx:89
- SearchContextTestProvider creates and provides a local React context that is not the same SearchContext used by useSearchContext() (which is already mocked above). This provider appears to have no effect and can be removed to simplify the test setup and avoid confusion.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| updateConfig(buildBoundingBoxConfig(initialBoundingBox.current)); | ||
| } | ||
| } | ||
| }, [mode, updateConfig, setBoundingBox]); |
There was a problem hiding this comment.
In search mode, handleLocationClear() calls updateConfig(buildBoundingBoxConfig(...)) even when geoSearchEnabled is false. That contradicts the stated behavior that search-mode geo is opt-in ("Search this area"). Consider only applying bounding-box config here if geoSearchEnabled is currently true, otherwise clear geo params (insideBoundingBox/around*) without re-enabling geo filtering.
| updateConfig(buildBoundingBoxConfig(initialBoundingBox.current)); | |
| } | |
| } | |
| }, [mode, updateConfig, setBoundingBox]); | |
| if (geoSearchEnabled) { | |
| updateConfig(buildBoundingBoxConfig(initialBoundingBox.current)); | |
| } else { | |
| // In search mode with geoSearch disabled, clear geo params instead of re-enabling geo. | |
| updateConfig({ | |
| hitsPerPage: HITS_PER_PAGE, | |
| insideBoundingBox: undefined, | |
| aroundLatLng: undefined, | |
| aroundRadius: undefined, | |
| aroundPrecision: undefined, | |
| minimumAroundRadius: undefined, | |
| aroundUserLocationRadius: undefined, | |
| }); | |
| } | |
| } | |
| } | |
| }, [mode, geoSearchEnabled, updateConfig, setBoundingBox]); |
| if (userLocation) { | ||
| setAroundLatLng( | ||
| `${userLocation.coords.lat},${userLocation.coords.lng}` | ||
| ); | ||
| } | ||
| setAroundRadius(1600); | ||
| lastAppliedGeo.current = ""; | ||
| clearRefinements(); |
There was a problem hiding this comment.
There are several hard-coded radius values of 1600 here, while DEFAULT_RADIUS is 1609 (and FilterHeader also uses 1609). This can lead to inconsistent distance behavior across flows (reset vs clear vs apply). Use the shared DEFAULT_RADIUS constant (or another single source of truth) instead of mixing 1600/1609.
| setBoundingBox(undefined); | ||
| setAroundLatLng(`${userLocation?.coords.lat},${userLocation?.coords.lng}`); | ||
| setAroundRadius(1600); | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps |
There was a problem hiding this comment.
This browse-mode reset sets aroundLatLng/aroundRadius to 1600, which differs from DEFAULT_RADIUS (1609) used elsewhere. Using mixed defaults makes behavior harder to reason about and can cause subtle UI/test mismatches; prefer the shared constant(s).
| // Wait for user location before showing browse results. | ||
| if (userLocation === null) { | ||
| return <Loader />; | ||
| } | ||
|
|
There was a problem hiding this comment.
useAppContext()'s userLocation is typed as non-nullable in AppContext, and App only renders the provider once userLocation is resolved. This null-check is therefore dead code (and suggests a mismatch between intended runtime behavior and types). Either remove the check or update the context typing/provider contract so userLocation can actually be null here.
| // Wait for user location before showing browse results. | |
| if (userLocation === null) { | |
| return <Loader />; | |
| } |
📝 Description
🍐 Paired with
🔗 Related links
🛠 Changes
🖼 Screenshots
✏️ Notes