Fix ETA null, Add Route Operator and Service Type Info, Reduce Duplicate Route No #202
Conversation
WalkthroughThe changes introduce a new property, Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range, codebase verification and nitpick comments (2)
src/components/home/lists/NearbyRouteList.tsx (1)
Line range hint
132-158: Refactor suggestion forgetRoutesfunction to improve performance and readability.The
getRoutesfunction has been modified to include a new arrayaddedListto prevent duplicate route entries. This is a good approach to handle duplicates, but there are a few areas that could be improved for better performance and readability:
- Use of
findin a loop: The current implementation usesfindinside a nested loop, which can be inefficient for large datasets. Consider using aSetor aMapforaddedListto check for existence in constant time.- Complexity and readability: The nested loops and conditionals make the function quite complex. Refactoring to break down the function or using helper functions could improve readability.
Here's a suggested refactor using a
Mapto track added routes:- const addedList = [] as [string, Company, string][]; + const addedList = new Map<string, boolean>(); const nearbyRoutes = Object.entries(stopList) .map((stop: [string, StopListEntry]): [string, StopListEntry, number] => [...stop, getDistance(stop[1].location, geolocation)] ) .filter((stop) => stop[2] < searchRange) .sort((a, b) => a[2] - b[2]) .slice(0, 20) .reduce((acc, [stopId]) => { Object.entries(routeList).forEach(([key, route]) => { (["kmb", "lrtfeeder", "lightRail", "gmb", "ctb", "nlb"] as Company[]).forEach((co) => { if (route.stops[co] && route.stops[co].includes(stopId)) { if (acc[coToType[co]] === undefined) acc[coToType[co]] = []; - if(addedList.find(([_route, _co, _serviceType]) => _route == route.route && _co == co && _serviceType < route.serviceType) === undefined) { + const addedKey = `${route.route}-${co}-${route.serviceType}`; + if (!addedList.has(addedKey)) { acc[coToType[co]].push(key + "/" + route.stops[co].indexOf(stopId)); - addedList.push([route.route, co, route.serviceType]); + addedList.set(addedKey, true); } }); }); }); return acc; }, { bus: [], mtr: [], lightRail: [], minibus: [], ferry: [] } as Record<TransportType, string[]>);This refactor uses a
Mapto store the combination of route, company, and service type as a key, which allows for constant time complexity checks and reduces the overhead of searching through an array.Tools
Biome
[error] 151-152: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
src/components/home/SuccinctTimeReport.tsx (1)
Line range hint
101-294: EnhanceSuccinctTimeReportcomponent with service type handling and improve code readability.The
SuccinctTimeReportcomponent has been updated to handle a new propertyserviceType, which is used to conditionally render additional information in the UI. This is a valuable addition for providing context to the users. However, there are a few areas that could be improved:
- Conditional rendering logic: The current implementation directly checks the
serviceTypewithin the JSX. This could be abstracted into a helper function to improve readability and maintainability.- Styling: The new styles
companySxandspecialTripSxare defined at the bottom of the file. It's a good practice to keep related styles closer to their usage or abstract them into a separate file to improve the organization.Here's a suggested refactor to abstract the conditional rendering and improve the organization of styles:
+ const renderSpecialTripInfo = (serviceType, t) => { + if (parseInt(serviceType, 10) >= 2) { + return ( + <Typography variant="caption" sx={specialTripSx}> + {t("特別班")} + </Typography> + ); + } + return null; + }; return ( <> <ListItem onClick={handleClick} sx={rootSx}> <ListItemText primary={ <Box overflow="hidden"> <RouteNo routeNo={language === "zh" ? t(routeNo) : routeNo} fontSize={co[0] === "mtr" ? "1.1rem" : undefined} /> - {parseInt(serviceType, 10) >= 2 && ( - <Typography variant="caption" sx={specialTripSx}> - {t("特別班")} - </Typography> - )} + {renderSpecialTripInfo(serviceType, t)} </Box> } secondary={ <Typography component="h4" variant="caption" sx={companySx}> {co.map((co) => t(co)).join("+")} </Typography> } /> ... </ListItem> ... </> );This refactor introduces a helper function
renderSpecialTripInfothat encapsulates the logic for rendering the special trip information based on theserviceType. This makes the component more readable and easier to maintain.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- src/components/home/SuccinctTimeReport.tsx (3 hunks)
- src/components/home/lists/NearbyRouteList.tsx (2 hunks)
- src/hooks/useStopEtas.tsx (1 hunks)
| a = a.filter((e) => e.eta); | ||
| b = b.filter((e) => e.eta); | ||
| if (a.length === 0 && b.length === 0) return keyA < keyB ? -1 : 1; |
There was a problem hiding this comment.
Improve sorting logic in useStopEtas hook for better performance and clarity.
The useStopEtas hook has been updated to include additional filtering logic to handle cases where both filtered arrays are empty, returning a comparison based on the keys. This is a crucial update for handling edge cases more gracefully. However, the current implementation could be optimized further:
- Filtering logic: The filtering of
etavalues is done inline within the sorting function. This could be moved outside the sort function to avoid repeated computation. - Comparison logic: The comparison logic is somewhat convoluted and could be simplified for better readability.
Here's a suggested refactor to optimize the filtering and comparison logic:
- a = a.filter((e) => e.eta);
- b = b.filter((e) => e.eta);
+ const filteredA = a.filter((e) => e.eta);
+ const filteredB = b.filter((e) => e.eta);
- if (a.length === 0 && b.length === 0) return keyA < keyB ? -1 : 1;
- if (a.length === 0) return 1;
- if (b.length === 0) return -1;
+ if (filteredA.length === 0 && filteredB.length === 0) return keyA.localeCompare(keyB);
+ if (filteredA.length === 0) return 1;
+ if (filteredB.length === 0) return -1;
if (isLightRail) {
if (filteredA[0].remark.zh === filteredB[0].remark.zh) {
return filteredA[0].eta < filteredB[0].eta ? -1 : 1;
}
return filteredA[0].remark.zh < filteredB[0].remark.zh ? -1 : 1;
}
if (filteredA[0].eta === filteredB[0].eta) {
return keyA.localeCompare(keyB);
}
return filteredA[0].eta < filteredB[0].eta ? -1 : 1;This refactor moves the filtering logic outside the sort function, reducing the complexity and improving performance by avoiding repeated filtering. Additionally, the comparison logic is simplified using localeCompare for string comparisons, which is more robust and readable.
Committable suggestion was skipped due to low confidence.
…g and Home Screen (Favourites/Nearby Routes)
chengkeith
force-pushed
the
fix-eta-null
branch
from
337bed1 to
8041902
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- src/components/home/SuccinctTimeReport.tsx (3 hunks)
- src/components/home/lists/NearbyRouteList.tsx (2 hunks)
Files skipped from review as they are similar to previous changes (2)
- src/components/home/SuccinctTimeReport.tsx
- src/components/home/lists/NearbyRouteList.tsx
Description
Screenshot
|
Description
This PR includes three changes (each of which is an individual commit):
The PR partially addresses some issues raised in Issue #200. The compiled version of this PR can also be found in https://alpha.hkbus.app/
Screenshots
At Stop Dialog
At Home Screen
Summary by CodeRabbit
New Features
serviceTypeproperty in the time report, enhancing the display with context-specific labels.Bug Fixes