feat: sort by rating#802
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
Adds UI + data plumbing to let users sort the eatery list by rating (asc/desc) in addition to the existing time-based ordering.
Changes:
- Introduces a new
SelectSortdropdown control and wires it intoListPage/EateryCardGrid. - Fetches review summary data per location to derive
averageRating/ratingCount, and extendsILocation_Fullwith rating fields. - Moves sorting responsibility into
EateryCardGrid(time vs rating), while keeping filtering inuseFilteredLocations.
Reviewed changes
Copilot reviewed 8 out of 14 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/util/storage.ts | Formatting-only change. |
| src/util/queryLocations.ts | Import formatting cleanup. |
| src/types/locationTypes.ts | Adds rating fields/types and extends ILocation_Full to include them. |
| src/pages/ListPage.tsx | Adds sort state + dropdown and passes sortOption down to the grid. |
| src/components/SelectSort.tsx | New sort dropdown component. |
| src/components/SelectLocation.tsx | Formatting-only change. |
| src/components/EateryCardHeader.module.css | Adds blank lines. |
| src/components/EateryCardGrid.tsx | Implements time/rating comparators and sorts locations based on sortOption. |
| src/components/EateryCardGrid.module.css | Formatting-only change. |
| src/components/EateryCardContent.tsx | Formatting-only change. |
| src/components/EateryCard.tsx | Formatting-only change. |
| src/components/EateryCard.module.css | Formatting-only change. |
| src/App.tsx | Adds per-location review summary queries and injects rating data into fullLocationData. |
| src/App.css | Mobile layout adjustment (.App { height: auto; }). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const locationIds = useMemo(() => locations?.map((location) => location.id) ?? [], [locations]); | ||
|
|
||
| const reviewSummaries = useQueries({ | ||
| queries: locationIds.map((locationId) => ({ | ||
| ...$api.queryOptions('get', '/v2/locations/{locationId}/reviews/summary', { | ||
| params: { path: { locationId } }, | ||
| }), | ||
| enabled: !!locationId, | ||
| })), | ||
| }); |
There was a problem hiding this comment.
useQueries is firing one /reviews/summary request per location on initial app load (regardless of whether the user is sorting by rating). This N+1 fan-out can significantly slow first paint and increases backend load; consider fetching summaries only when the sort mode is a rating mode (or batching via a single endpoint / limiting concurrency).
| const locationIds = useMemo(() => locations?.map((location) => location.id) ?? [], [locations]); | ||
|
|
||
| const reviewSummaries = useQueries({ | ||
| queries: locationIds.map((locationId) => ({ | ||
| ...$api.queryOptions('get', '/v2/locations/{locationId}/reviews/summary', { | ||
| params: { path: { locationId } }, | ||
| }), | ||
| enabled: !!locationId, | ||
| })), | ||
| }); | ||
|
|
||
| const ratingMap = useMemo(() => { | ||
| const averages: Record<string, { avg: number | null; count: number | null }> = {}; | ||
| reviewSummaries.forEach((summary, index) => { | ||
| const locationId = locationIds[index]; | ||
| if (!locationId) return; | ||
| const buckets = summary.data?.starData?.buckets ?? []; | ||
| const count = buckets.reduce((total, bucket) => total + bucket, 0); | ||
| averages[locationId] = { | ||
| avg: summary.data?.starData?.avg ?? null, | ||
| count: Number.isFinite(count) ? count : null, | ||
| }; | ||
| }); | ||
| return averages; | ||
| }, [reviewSummaries, locationIds]); | ||
|
|
||
| const fullLocationData: ILocation_Full[] | undefined = locations?.map((location) => ({ | ||
| ...location, | ||
| ...getLocationStatus(location.times, now), | ||
| cardViewPreference: | ||
| cardViewPreferences[location.id] ?? cardViewPreferences[location.conceptId ?? ''] ?? 'normal', // check for conceptid preference as well, fallback | ||
| averageRating: ratingMap[location.id]?.avg ?? null, | ||
| ratingCount: ratingMap[location.id]?.count ?? null, | ||
| })); |
There was a problem hiding this comment.
The PR description mentions a “placeholder dropdown”, but this change also adds eager rating fetching and rating-based sorting behavior (network + data model changes). Please update the PR description to match the actual scope, or defer the fetching/sorting if it’s not intended to ship yet.
| updateCardViewPreference, | ||
| sortOption, | ||
| }: { | ||
| /** locations should already be filtered and sorted - this component is just responsible for rendering the content as-is */ |
There was a problem hiding this comment.
The prop comment says locations are already “filtered and sorted”, but this component now performs sorting internally based on sortOption. Please update the comment (or move sorting back up) so the contract matches the implementation.
| /** locations should already be filtered and sorted - this component is just responsible for rendering the content as-is */ | |
| /** locations should already be filtered; this component handles sorting (via sortOption) and rendering */ |
| import assert from '../util/assert'; | ||
|
|
||
| const compareLocationsByTime = (location1: ILocation_Full, location2: ILocation_Full) => { | ||
| const state1 = location1.locationState; | ||
| const state2 = location2.locationState; | ||
|
|
||
| if (state1 !== state2) return state1 - state2; | ||
|
|
||
| // this if statement is janky but otherwise TS won't | ||
| // realize that the timeUntil property exists on both l1 and l2 | ||
| if (location1.closedLongTerm || location2.closedLongTerm) { | ||
| assert(location1.closedLongTerm && location2.closedLongTerm); | ||
| return location1.name.localeCompare(location2.name); | ||
| } | ||
| if (state1 === LocationState.OPEN || state1 === LocationState.CLOSES_SOON) { | ||
| return location2.minutesUntil - location1.minutesUntil; | ||
| } | ||
| return location1.minutesUntil - location2.minutesUntil; | ||
| }; | ||
|
|
||
|
|
There was a problem hiding this comment.
compareLocationsByTime duplicates the existing time-based comparator logic in src/util/useFilteredLocations.ts (used by useSortedLocations). To avoid the two implementations drifting over time, consider exporting a shared comparator (or reusing useSortedLocations / its comparator) instead of duplicating it here.
| import assert from '../util/assert'; | |
| const compareLocationsByTime = (location1: ILocation_Full, location2: ILocation_Full) => { | |
| const state1 = location1.locationState; | |
| const state2 = location2.locationState; | |
| if (state1 !== state2) return state1 - state2; | |
| // this if statement is janky but otherwise TS won't | |
| // realize that the timeUntil property exists on both l1 and l2 | |
| if (location1.closedLongTerm || location2.closedLongTerm) { | |
| assert(location1.closedLongTerm && location2.closedLongTerm); | |
| return location1.name.localeCompare(location2.name); | |
| } | |
| if (state1 === LocationState.OPEN || state1 === LocationState.CLOSES_SOON) { | |
| return location2.minutesUntil - location1.minutesUntil; | |
| } | |
| return location1.minutesUntil - location2.minutesUntil; | |
| }; | |
| import { compareLocationsByTime } from '../util/useFilteredLocations'; |
| @@ -0,0 +1,25 @@ | |||
| import css from './SelectLocation.module.css'; | |||
There was a problem hiding this comment.
SelectSort is importing styles from SelectLocation.module.css. This creates an implicit coupling between two unrelated controls; consider introducing SelectSort.module.css (or a shared select control style module) so future style changes don’t have surprising cross-component effects.
| import css from './SelectLocation.module.css'; | |
| import css from './SelectSort.module.css'; |
| style={ratingsAvg ? {} : { color: 'var(--black-500)' }} | ||
| > | ||
| {ratingsAvg?.toFixed(1) ?? '0.0'} |
There was a problem hiding this comment.
Bug: The rating displayed on cards (ratingsAvg) is from a different API source than the rating used for sorting (averageRating), which can cause a mismatched sort order.
Severity: MEDIUM
Suggested Fix
Unify the data source for both displaying and sorting ratings. Use the averageRating calculated from the /v2/locations/{locationId}/reviews/summary endpoint for both the sorting logic and the display on the EateryCardContent component to ensure consistency.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: src/components/EateryCardContent.tsx#L51-L53
Potential issue: The average rating displayed on each eatery card is sourced from the
`ratingsAvg` field from the `/v2/locations` API endpoint. However, the sorting logic
uses a different value, `averageRating`, which is derived from the `starData.avg` field
fetched from a separate `/v2/locations/{locationId}/reviews/summary` endpoint. This
discrepancy can cause the visual sort order to not match the ratings displayed on the
cards, leading to user confusion when they sort by "Highest Rating" or "Lowest Rating".
|
yoo looks fire z-index issue here 
|
|
pr #800 except with correct permissions for vercel and stuff. give user choice to sort by rating (highest to lowest or lowest to highest) or by the default times. i added a placeholder dropdown for this.