Open
Conversation
Reviewer's GuideImplements shelter name search alongside existing location/property filters, centralizes shelter property filters atom naming, updates query composition and results-source messaging, and wires name-based searches to recenter the map on returned shelter pins with symmetric bounds. Sequence diagram for shelter name search and map recenteringsequenceDiagram
actor User
participant HomePage
participant ShelterSearch
participant shelterPropertyFiltersAtom
participant SheltersDisplay
participant useInfiniteScrollQuery
participant Map
User->>ShelterSearch: Type name into nameSearchValue
User->>ShelterSearch: Click Search button
ShelterSearch->>ShelterSearch: onSearchClick()
ShelterSearch->>ShelterSearch: setQueryFilters(undefined)
ShelterSearch->>shelterPropertyFiltersAtom: reset()
ShelterSearch->>ShelterSearch: setNameFilter(trim(nameSearchValue))
ShelterSearch->>HomePage: onNameSearch()
HomePage->>HomePage: onNameSearch()
HomePage->>HomePage: setMapBoundsFilter(undefined)
HomePage->>HomePage: setShowSearchButton(false)
HomePage->>HomePage: increment nameSearchPinFitRequestId
HomePage->>ShelterSearch: Render with updated nameSearchPinFitRequestId
ShelterSearch->>SheltersDisplay: Render with nameFilter and nameSearchPinFitRequestId
SheltersDisplay->>SheltersDisplay: useMemo build queryVariables
SheltersDisplay->>SheltersDisplay: filters.name = nameFilter
SheltersDisplay->>useInfiniteScrollQuery: fetch shelters with queryVariables
useInfiniteScrollQuery-->>SheltersDisplay: items, loading=false
SheltersDisplay->>SheltersDisplay: useEffect on loading, nameSearchPinFitRequestId
SheltersDisplay->>SheltersDisplay: shelterListToPinLatLng(items)
SheltersDisplay->>HomePage: onShelterPinsReadyForMapFit(pinLocations)
HomePage->>HomePage: symmetricBoundsAroundPinCentroid(pinLocations)
HomePage->>Map: fitBounds(bounds)
Updated class diagram for shelter search components and filters atomclassDiagram
class ShelterSearch {
+mapBoundsFilter: TMapBounds
+nameSearchPinFitRequestId: number
+onShelterPinsReadyForMapFit(pinLocations: TLatLng[]): void
+onNameSearch(): void
-location: TLatLng
-queryFilters: TShelterPropertyFilters
-nameFilter: string
-nameSearchValue: string
-submitQueryTs: number
+onPlaceSelect(place: TPlaceResult): void
+onSubmitFilters(): void
+onFilterClick(): void
+onNameSearchChange(value: string): void
+onSearchClick(): void
}
class SheltersDisplay {
+className: string
+coordinates: TLatLng
+mapBoundsFilter: TMapBounds
+propertyFilters: TShelterPropertyFilters
+rangeInMiles: number
+nameFilter: string
+nameSearchPinFitRequestId: number
+onShelterPinsReadyForMapFit(pinLocations: TLatLng[]): void
-queryVariables: ViewSheltersQueryVariables
-sheltersData: TShelter[]
+SheltersDisplay(props: TProps): void
+shelterListToPinLatLng(shelters: TShelter[]): TLatLng[]
+pruneFilters(filters: TShelterPropertyFilters): TShelterPropertyFilters
}
class ResultsSource {
+className: string
+queryFilters: ViewSheltersQueryVariables_filters
+ResultsSource(props: TProps): void
-propertyFiltersAffectQuery(propertyFilters: TShelterPropertyFilters): boolean
-formatWithOxfordComma(parts: string[]): string
}
class AddressAutocomplete {
+className: string
+placeholder: string
+countryRestrictions: ISO3166Alpha2
+leftIcon: ReactElement
+onPlaceSelect(place: TPlaceResult): void
}
class shelterPropertyFiltersAtom {
+value: TShelterPropertyFilters
+reset(): void
}
class HomePage {
-location: TLatLng
-mapBoundsFilter: TMapBounds
-nameSearchPinFitRequestId: number
+onShelterPinsReadyForMapFit(pinLocations: TLatLng[]): void
+onNameSearch(): void
+symmetricBoundsAroundPinCentroid(pinLocations: TLatLng[]): LatLngBounds
}
ShelterSearch --> SheltersDisplay
ShelterSearch --> AddressAutocomplete
ShelterSearch --> shelterPropertyFiltersAtom
SheltersDisplay --> ResultsSource
HomePage --> ShelterSearch
HomePage --> SheltersDisplay
HomePage --> shelterPropertyFiltersAtom
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- In
SheltersDisplay, the geolocation filter is now skipped entirely whenmapBoundsFilteris present, whereas previously onlyrangeInMileswas omitted; if the intent is still to bias results by user location while constraining to map bounds, consider retaininggeolocationwithoutrangeInMilesinstead of dropping it altogether. - The logic in
ResultsSource.propertyFiltersAffectQuerydiverges frompruneFiltersinSheltersDisplay(e.g., it doesn’t check for empty arrays), which can cause the source text to say filters affect the query even when they don’t; consider reusingpruneFiltersor aligning the conditions so the display matches the actual applied filters.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `SheltersDisplay`, the geolocation filter is now skipped entirely when `mapBoundsFilter` is present, whereas previously only `rangeInMiles` was omitted; if the intent is still to bias results by user location while constraining to map bounds, consider retaining `geolocation` without `rangeInMiles` instead of dropping it altogether.
- The logic in `ResultsSource.propertyFiltersAffectQuery` diverges from `pruneFilters` in `SheltersDisplay` (e.g., it doesn’t check for empty arrays), which can cause the source text to say filters affect the query even when they don’t; consider reusing `pruneFilters` or aligning the conditions so the display matches the actual applied filters.
## Individual Comments
### Comment 1
<location path="libs/react/shelter/src/lib/components/ShelterSearch/ResultsSource.tsx" line_range="45-47" />
<code_context>
+ if (queryFilters?.mapBounds) {
+ resultSourceParts.push('map area');
+ }
+ if (
+ propertyFiltersAffectQuery(
+ queryFilters?.properties as TShelterPropertyFilters | undefined
+ )
+ ) {
</code_context>
<issue_to_address>
**issue (bug_risk):** The "filters" label ignores the standalone `openNow` filter and only looks at `properties`, which can misrepresent the source of results.
Here `propertyFiltersAffectQuery` only receives `queryFilters?.properties`, but `openNow` lives on `filters.openNow`. So an "open now"–only query won’t add "filters" to `resultSourceParts`, even though the query is constrained. Consider either:
- Passing the full `TShelterPropertyFilters` (including `openNow`) into `propertyFiltersAffectQuery` and checking both `openNow` and other keys, or
- Adding an explicit `queryFilters?.openNow` check alongside the `properties` check.
This will keep the "based on …" description consistent for open‑now‑only queries.
</issue_to_address>
### Comment 2
<location path="libs/react/shelter/src/lib/components/ShelterSearch/SheltersDisplay.tsx" line_range="45" />
<code_context>
const [_sheltersData, setSheltersData] = useAtom(sheltersAtom);
- let queryVariables: ViewSheltersQueryVariables | undefined;
+ const queryVariables = useMemo<ViewSheltersQueryVariables | undefined>(() => {
+ let vars: ViewSheltersQueryVariables | undefined;
</code_context>
<issue_to_address>
**issue (complexity):** Consider simplifying the query variable construction and splitting the map-fit effect into smaller focused effects to keep the new behaviors while making the code easier to follow.
You can reduce the added complexity in two focused spots without changing behavior: the query construction and the map‑fit effect.
---
### 1) Simplify `queryVariables` construction
The current `useMemo` uses a mutable `vars` with repeated `vars = vars || {}; vars.filters = vars.filters || {};` which makes it harder to scan.
You can build a `filters` object linearly and wrap it at the end:
```ts
const queryVariables = useMemo<ViewSheltersQueryVariables | undefined>(() => {
const filters: NonNullable<ViewSheltersQueryVariables['filters']> = {};
if (coordinates && !mapBoundsFilter) {
const { latitude, longitude } = coordinates;
filters.geolocation = {
latitude,
longitude,
rangeInMiles,
};
}
if (mapBoundsFilter) {
filters.mapBounds = mapBoundsFilter;
}
if (propertyFilters) {
const { openNow, ...propertyOnlyFilters } = propertyFilters;
if (openNow) {
filters.openNow = true;
}
const prunedFilters = pruneFilters(propertyOnlyFilters);
if (prunedFilters) {
filters.properties = prunedFilters;
}
}
if (nameFilter) {
filters.name = nameFilter;
}
// Return undefined when no filters were set, to preserve current behavior
return Object.keys(filters).length > 0 ? { filters } : undefined;
}, [coordinates, mapBoundsFilter, propertyFilters, rangeInMiles, nameFilter]);
```
This keeps all existing behavior (including the “no mapBounds ⇒ geolocation range” rule and `pruneFilters` semantics) but removes the repeated lazy-init boilerplate and makes each branch’s intent clearer.
---
### 2) Decompose `onShelterPinsReadyForMapFit` logic
The current effect mixes:
- loading transition detection,
- “handle each `nameSearchPinFitRequestId` once” logic, and
- optional `requestAnimationFrame` scheduling
…all in one block. You can split the concerns and keep the same semantics (“call once per request id, after results are ready; use `requestAnimationFrame` when we didn’t *just* finish loading”).
First, track loading transitions in a tiny effect:
```ts
const wasLoadingRef = useRef(loading);
useEffect(() => {
wasLoadingRef.current = loading;
}, [loading]);
```
Then, in a separate effect, handle the pin-fit callback:
```ts
const lastHandledRequestIdRef = useRef(0);
useEffect(() => {
if (!onShelterPinsReadyForMapFit || nameSearchPinFitRequestId <= 0) {
return;
}
if (lastHandledRequestIdRef.current === nameSearchPinFitRequestId) {
return; // already handled this request
}
if (loading) {
return; // wait until loading finishes
}
const emit = () => {
if (lastHandledRequestIdRef.current === nameSearchPinFitRequestId) {
return;
}
lastHandledRequestIdRef.current = nameSearchPinFitRequestId;
onShelterPinsReadyForMapFit(shelterListToPinLatLng(items ?? []));
};
const loadingJustFinished = wasLoadingRef.current === true;
if (loadingJustFinished) {
emit();
return;
}
const raf = requestAnimationFrame(emit);
return () => cancelAnimationFrame(raf);
}, [loading, items, nameSearchPinFitRequestId, onShelterPinsReadyForMapFit]);
```
This preserves:
- the “only once per `nameSearchPinFitRequestId`” behavior,
- the special handling when a request has *just* finished loading vs. when we’re already idle, and
- the use of `requestAnimationFrame` when appropriate,
while making the mental model (“track loading separately, then react to `(id, items, loading)`”) more explicit and easier to maintain.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
libs/react/shelter/src/lib/components/ShelterSearch/ResultsSource.tsx
Outdated
Show resolved
Hide resolved
| const [_sheltersData, setSheltersData] = useAtom(sheltersAtom); | ||
|
|
||
| let queryVariables: ViewSheltersQueryVariables | undefined; | ||
| const queryVariables = useMemo<ViewSheltersQueryVariables | undefined>(() => { |
There was a problem hiding this comment.
issue (complexity): Consider simplifying the query variable construction and splitting the map-fit effect into smaller focused effects to keep the new behaviors while making the code easier to follow.
You can reduce the added complexity in two focused spots without changing behavior: the query construction and the map‑fit effect.
1) Simplify queryVariables construction
The current useMemo uses a mutable vars with repeated vars = vars || {}; vars.filters = vars.filters || {}; which makes it harder to scan.
You can build a filters object linearly and wrap it at the end:
const queryVariables = useMemo<ViewSheltersQueryVariables | undefined>(() => {
const filters: NonNullable<ViewSheltersQueryVariables['filters']> = {};
if (coordinates && !mapBoundsFilter) {
const { latitude, longitude } = coordinates;
filters.geolocation = {
latitude,
longitude,
rangeInMiles,
};
}
if (mapBoundsFilter) {
filters.mapBounds = mapBoundsFilter;
}
if (propertyFilters) {
const { openNow, ...propertyOnlyFilters } = propertyFilters;
if (openNow) {
filters.openNow = true;
}
const prunedFilters = pruneFilters(propertyOnlyFilters);
if (prunedFilters) {
filters.properties = prunedFilters;
}
}
if (nameFilter) {
filters.name = nameFilter;
}
// Return undefined when no filters were set, to preserve current behavior
return Object.keys(filters).length > 0 ? { filters } : undefined;
}, [coordinates, mapBoundsFilter, propertyFilters, rangeInMiles, nameFilter]);This keeps all existing behavior (including the “no mapBounds ⇒ geolocation range” rule and pruneFilters semantics) but removes the repeated lazy-init boilerplate and makes each branch’s intent clearer.
2) Decompose onShelterPinsReadyForMapFit logic
The current effect mixes:
- loading transition detection,
- “handle each
nameSearchPinFitRequestIdonce” logic, and - optional
requestAnimationFramescheduling
…all in one block. You can split the concerns and keep the same semantics (“call once per request id, after results are ready; use requestAnimationFrame when we didn’t just finish loading”).
First, track loading transitions in a tiny effect:
const wasLoadingRef = useRef(loading);
useEffect(() => {
wasLoadingRef.current = loading;
}, [loading]);Then, in a separate effect, handle the pin-fit callback:
const lastHandledRequestIdRef = useRef(0);
useEffect(() => {
if (!onShelterPinsReadyForMapFit || nameSearchPinFitRequestId <= 0) {
return;
}
if (lastHandledRequestIdRef.current === nameSearchPinFitRequestId) {
return; // already handled this request
}
if (loading) {
return; // wait until loading finishes
}
const emit = () => {
if (lastHandledRequestIdRef.current === nameSearchPinFitRequestId) {
return;
}
lastHandledRequestIdRef.current = nameSearchPinFitRequestId;
onShelterPinsReadyForMapFit(shelterListToPinLatLng(items ?? []));
};
const loadingJustFinished = wasLoadingRef.current === true;
if (loadingJustFinished) {
emit();
return;
}
const raf = requestAnimationFrame(emit);
return () => cancelAnimationFrame(raf);
}, [loading, items, nameSearchPinFitRequestId, onShelterPinsReadyForMapFit]);This preserves:
- the “only once per
nameSearchPinFitRequestId” behavior, - the special handling when a request has just finished loading vs. when we’re already idle, and
- the use of
requestAnimationFramewhen appropriate,
while making the mental model (“track loading separately, then react to (id, items, loading)”) more explicit and easier to maintain.
|
🔍 [shelter-web] Preview available at: https://shelter.dev.betterangels.la/branches/SDB-121-shelter-search Last updated: 2026-04-02T22:36:08.038Z |
|
🔍 [storybook-react] Preview available at: https://storybook.dev.betterangels.la/branches/SDB-121-shelter-search Last updated: 2026-04-02T22:36:08.050Z |
|
🔍 [betterangels-admin] Preview available at: https://admin.dev.betterangels.la/branches/SDB-121-shelter-search Last updated: 2026-04-02T22:36:08.052Z |
Davit-BetterAngels
approved these changes
Apr 2, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
SDB-121
the order in which the filters are selected affects which are applied:
Summary by Sourcery
Add shelter name search with map fitting behavior and update how search sources and property filters are handled.
New Features:
Bug Fixes:
Enhancements: