show all shelters under map boundries (SDB-103)#2004
Open
Davit-BetterAngels wants to merge 1 commit intomainfrom
Open
show all shelters under map boundries (SDB-103)#2004Davit-BetterAngels wants to merge 1 commit intomainfrom
Davit-BetterAngels wants to merge 1 commit intomainfrom
Conversation
Davit-BetterAngels
temporarily deployed
to
preview
GitHub Actions
Inactive
Reviewer's GuideRefactors shelter listing to fetch and display all shelters within map boundaries in a single paginated query, simplifies image handling on shelter cards to use only the hero image, and removes now-unused multi-photo support and infinite scroll pagination wiring. Sequence diagram for fetching and displaying all shelters within map boundariessequenceDiagram
actor User
participant MapView
participant SheltersDisplay
participant ApolloClient
participant GraphQLAPI
User->>MapView: Pan or zoom map
MapView->>SheltersDisplay: onBoundsChange(newBounds)
SheltersDisplay->>SheltersDisplay: build queryVariables from bounds
SheltersDisplay->>ApolloClient: useQuery(ViewShelters, variables + pagination{limit:5000,offset:0})
ApolloClient->>GraphQLAPI: ViewShelters(queryVariables, pagination)
GraphQLAPI-->>ApolloClient: shelters{results, totalCount}
ApolloClient-->>SheltersDisplay: data, loading=false, error
SheltersDisplay->>SheltersDisplay: shelters = data.shelters.results
SheltersDisplay->>SheltersDisplay: useEffect setSheltersData(shelters)
SheltersDisplay->>InfiniteList: render with data=shelters, hasMore=false
InfiniteList->>User: Display all shelters in current bounds (no infinite scroll)
Class diagram for updated shelter card structures and shelters displayclassDiagram
class TShelter {
string id
string name
string heroImage
number distanceInMiles
TShelterLocation location
%% removed: exteriorPhotos
%% removed: interiorPhotos
}
class TShelterLocation {
number latitude
number longitude
string city
string state
}
class ShelterCard {
+ShelterCard(props)
-string id
-string name
-string heroImage
-number distanceInMiles
-TShelterLocation location
-function onNavigate()
}
class ShelterCardHero {
+ShelterCardHero(props)
-string shelterName
-string imageUrl
-string className
-string[] imageCss
}
class SheltersDisplay {
+SheltersDisplay(props)
-TShelter[] shelters
-number total
-function buildQueryVariables()
-function renderListHeader(visible, total)
}
class InfiniteList~TShelter~ {
+render()
-TShelter[] data
-number totalItems
-boolean loading
-boolean hasMore
-Error error
}
TShelterLocation --> TShelter : location
TShelter --> ShelterCard : shelter
ShelterCardHero --> ShelterCard : used for hero image
TShelter --> SheltersDisplay : sheltersData
SheltersDisplay --> InfiniteList~TShelter~ : passes data and total
class ViewSheltersQuery {
TShelterConnection shelters
}
class TShelterConnection {
TShelter[] results
number totalCount
}
ViewSheltersQuery --> TShelterConnection : shelters
TShelterConnection --> TShelter : results
class ApolloUseQuery {
+useQuery(document, variables)
}
SheltersDisplay --> ApolloUseQuery : ViewSheltersDocument
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 1 issue, and left some high level feedback:
- The fixed
pagination: { limit: 5000, offset: 0 }is a fairly arbitrary magic number and could lead to heavy responses for dense areas; consider making this configurable (or capped by the API) and documenting the expected upper bound of shelters per bounds. - Since
InfiniteListis now always passedhasMore={false}and noloadMorehandler, it might be worth simplifying to a non-infinite list component or ensuringInfiniteListgracefully handles this mode to avoid unnecessary complexity or assumptions about infinite scrolling.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The fixed `pagination: { limit: 5000, offset: 0 }` is a fairly arbitrary magic number and could lead to heavy responses for dense areas; consider making this configurable (or capped by the API) and documenting the expected upper bound of shelters per bounds.
- Since `InfiniteList` is now always passed `hasMore={false}` and no `loadMore` handler, it might be worth simplifying to a non-infinite list component or ensuring `InfiniteList` gracefully handles this mode to avoid unnecessary complexity or assumptions about infinite scrolling.
## Individual Comments
### Comment 1
<location path="libs/react/shelter/src/lib/components/ShelterSearch/SheltersDisplay.tsx" line_range="83-89" />
<code_context>
- variables: queryVariables,
- pageSize: 25,
- });
+ const { data, loading, error } = useQuery<
+ ViewSheltersQuery,
+ ViewSheltersQueryVariables
+ >(ViewSheltersDocument, {
+ variables: {
+ ...queryVariables,
+ pagination: { limit: 5000, offset: 0 },
+ },
+ skip: !queryVariables,
</code_context>
<issue_to_address>
**issue (bug_risk):** Hard-coded pagination limit of 5000 can truncate results and cause scalability issues.
This change introduces two issues:
1. When `totalCount` > 5000, results after the first 5000 are silently dropped because `hasMore` is effectively disabled and there’s no way to request additional pages.
2. Loading up to 5000 shelters in one request is expensive (bandwidth, memory, render time), particularly on mobile.
Instead of a single large page with `hasMore={false}`, consider either paginating properly (e.g., incrementing `offset` and wiring `hasMore` to `total > shelters.length`) or using a smaller, justified limit and clearly indicating any cap in the UI.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+83
to
+89
| const { data, loading, error } = useQuery< | ||
| ViewSheltersQuery, | ||
| ViewSheltersQueryVariables | ||
| >(ViewSheltersDocument, { | ||
| variables: { | ||
| ...queryVariables, | ||
| pagination: { limit: 5000, offset: 0 }, |
There was a problem hiding this comment.
issue (bug_risk): Hard-coded pagination limit of 5000 can truncate results and cause scalability issues.
This change introduces two issues:
- When
totalCount> 5000, results after the first 5000 are silently dropped becausehasMoreis effectively disabled and there’s no way to request additional pages. - Loading up to 5000 shelters in one request is expensive (bandwidth, memory, render time), particularly on mobile.
Instead of a single large page with hasMore={false}, consider either paginating properly (e.g., incrementing offset and wiring hasMore to total > shelters.length) or using a smaller, justified limit and clearly indicating any cap in the UI.
|
🔍 [shelter-web] Preview available at: https://shelter.dev.betterangels.la/branches/SDB-103-pins-locations Last updated: 2026-04-02T09:08:21.604Z |
When I first open the link there are more listed than displayed (110 listed, but fewer on the map)
|
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-103
Summary by Sourcery
Simplify shelter card imagery and switch shelter search to load all shelters within bounds in a single query instead of using infinite scrolling.
Bug Fixes:
Enhancements: