Stop Grouping with Distance Range and (General) Directional filter #193
Conversation
- Add comments for code readability - Rename variables and methods with more meaningful names
WalkthroughThe recent changes introduce a new Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant StopRouteList
participant useStopGroup
participant getStopGroup
User->>StopRouteList: Render with {stops, routeId, isFocus}
StopRouteList->>useStopGroup: Call with {stopKeys, routeId}
useStopGroup->>getStopGroup: Fetch stop group with routeId filtering
getStopGroup->>useStopGroup: Return filtered stop group
useStopGroup->>StopRouteList: Return stop group
StopRouteList->>User: Render stop group
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 as PR comments)
Additionally, you can add CodeRabbit Configration 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: 3
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (6)
- src/components/bookmarked-stop/StopRouteList.tsx (1 hunks)
- src/components/bookmarked-stop/SwipeableStopList.tsx (1 hunks)
- src/components/route-eta/StopDialog.tsx (2 hunks)
- src/hooks/useStopGroup.tsx (1 hunks)
- src/pages/RouteEta.tsx (1 hunks)
- src/utils.ts (1 hunks)
Files skipped from review due to trivial changes (1)
- src/components/bookmarked-stop/SwipeableStopList.tsx
Additional context used
Biome
src/hooks/useStopGroup.tsx
[error] 51-54: This else clause can be omitted because previous branches break early.
Unsafe fix: Omit the else clause.
(lint/style/noUselessElse)
[error] 92-106: This else clause can be omitted because previous branches break early.
Unsafe fix: Omit the else clause.
(lint/style/noUselessElse)
GitHub Check: build (21.x)
src/hooks/useStopGroup.tsx
[warning] 178-178:
React Hook useMemo has missing dependencies: 'getAllRouteStopsBearings' and 'isBearingInRange'. Either include them or remove the dependency array
GitHub Check: build (20.x)
src/hooks/useStopGroup.tsx
[warning] 178-178:
React Hook useMemo has missing dependencies: 'getAllRouteStopsBearings' and 'isBearingInRange'. Either include them or remove the dependency array
Additional comments not posted (22)
src/components/bookmarked-stop/StopRouteList.tsx (4)
3-3: Import Approved.The
useStopGrouphook import is necessary for the functionality.
9-9: Prop Addition Approved.The
routeIdprop is necessary for the new functionality of grouping stops based on route.
13-13: Prop Addition with Default Value Approved.The
routeIdprop is added with a default value ofundefined, ensuring backward compatibility.
14-14: Hook Usage Approved.The
useStopGrouphook is correctly used to get the stop group based onstopKeysandrouteId.src/components/route-eta/StopDialog.tsx (3)
26-26: Prop Addition Approved.The
routeIdprop is necessary for the new functionality of grouping stops based on route.
31-31: Prop Addition Approved.The
routeIdprop is correctly added to theStopDialogcomponent, ensuring theStopRouteListcomponent receives it.
87-87: Component Usage Approved.The
StopRouteListcomponent is correctly used with therouteIdprop.src/pages/RouteEta.tsx (1)
256-256: Component Usage Approved.The
StopDialogcomponent is correctly used with therouteIdprop.src/hooks/useStopGroup.tsx (14)
1-1: Import Approved.The
useContextanduseMemohooks are necessary for the functionality.
2-2: Import Approved.The
Companytype is necessary for the functionality.
3-3: Import Approved.The
StopListEntrytype is necessary for the functionality.
4-4: Import Approved.The
DbContextis necessary for the functionality.
5-5: Import Approved.The
getDistanceandgetBearingfunctions are necessary for the functionality.
7-10: Interface Definition Approved.The
useStopGroupPropsinterface definition is correct and necessary for type safety.
11-15: Interface Definition Approved.The
StopListEntryExtendedinterface definition is correct and necessary for the functionality.
16-20: Interface Definition Approved.The
RouteStopCoSeqinterface definition is correct and necessary for the functionality.
23-25: Function Definition Approved.The
useStopGroupfunction definition is correct and aligns with the new functionality.
26-28: Context Usage Approved.The
DbContextis correctly used to accessrouteListandstopList.
30-33: Constants Definition Approved.The constants
DISTANCE_THRESHOLD,BEARING_THRESHOLD, andSTOP_LIST_LIMITare necessary for the functionality and the values seem reasonable.
35-40: Function Definitions Approved.The
getDistanceStopandgetBearingStopsfunctions are correctly defined to calculate distance and bearing between stops.
59-87: Logic for Route Stops Approved.The logic for route stops based on
routeIdis correct and aligns with the new functionality.
108-181: **Add Missing Dependencies to `useTools
GitHub Check: build (21.x)
[warning] 178-178:
React Hook useMemo has missing dependencies: 'getAllRouteStopsBearings' and 'isBearingInRange'. Either include them or remove the dependency arrayGitHub Check: build (20.x)
[warning] 178-178:
React Hook useMemo has missing dependencies: 'getAllRouteStopsBearings' and 'isBearingInRange'. Either include them or remove the dependency array
| export const getBearing = (a: GeoLocation, b: GeoLocation) => { | ||
| // Reference: https://www.movable-type.co.uk/scripts/latlong.html | ||
| const φ1 = a.lat * Math.PI / 180; // φ, λ = lat, lon in radians | ||
| const φ2 = b.lat * Math.PI / 180; | ||
| const λ1 = a.lng * Math.PI / 180; // φ, λ = lat, lon in radians | ||
| const λ2 = b.lng * Math.PI / 180; | ||
| const y = Math.sin(λ2 - λ1) * Math.cos(φ2); | ||
| const x = Math.cos(φ1) * Math.sin(φ2) - Math.sin(φ1) * Math.cos(φ2) * Math.cos(λ2-λ1); | ||
| const θ = Math.atan2(y, x); | ||
| const brng = (θ * 180 / Math.PI + 360) % 360; // in degrees | ||
| return brng; | ||
| }; |
There was a problem hiding this comment.
Optimize variable declarations and add comments for clarity.
The function is correctly implemented but can be optimized and improved for readability by combining the variable declarations and adding comments to explain the steps.
export const getBearing = (a: GeoLocation, b: GeoLocation) => {
// Reference: https://www.movable-type.co.uk/scripts/latlong.html
- const φ1 = a.lat * Math.PI / 180; // φ, λ = lat, lon in radians
- const φ2 = b.lat * Math.PI / 180;
- const λ1 = a.lng * Math.PI / 180; // φ, λ = lat, lon in radians
- const λ2 = b.lng * Math.PI / 180;
- const y = Math.sin(λ2 - λ1) * Math.cos(φ2);
- const x = Math.cos(φ1) * Math.sin(φ2) - Math.sin(φ1) * Math.cos(φ2) * Math.cos(λ2-λ1);
- const θ = Math.atan2(y, x);
- const brng = (θ * 180 / Math.PI + 360) % 360; // in degrees
- return brng;
+ const φ1 = a.lat * Math.PI / 180, φ2 = b.lat * Math.PI / 180;
+ const λ1 = a.lng * Math.PI / 180, λ2 = b.lng * Math.PI / 180;
+
+ // Calculate the differences
+ const Δλ = λ2 - λ1;
+ const y = Math.sin(Δλ) * Math.cos(φ2);
+ const x = Math.cos(φ1) * Math.sin(φ2) - Math.sin(φ1) * Math.cos(φ2) * Math.cos(Δλ);
+
+ // Calculate the bearing
+ const θ = Math.atan2(y, x);
+ return (θ * 180 / Math.PI + 360) % 360; // in degrees
};Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export const getBearing = (a: GeoLocation, b: GeoLocation) => { | |
| // Reference: https://www.movable-type.co.uk/scripts/latlong.html | |
| const φ1 = a.lat * Math.PI / 180; // φ, λ = lat, lon in radians | |
| const φ2 = b.lat * Math.PI / 180; | |
| const λ1 = a.lng * Math.PI / 180; // φ, λ = lat, lon in radians | |
| const λ2 = b.lng * Math.PI / 180; | |
| const y = Math.sin(λ2 - λ1) * Math.cos(φ2); | |
| const x = Math.cos(φ1) * Math.sin(φ2) - Math.sin(φ1) * Math.cos(φ2) * Math.cos(λ2-λ1); | |
| const θ = Math.atan2(y, x); | |
| const brng = (θ * 180 / Math.PI + 360) % 360; // in degrees | |
| return brng; | |
| }; | |
| export const getBearing = (a: GeoLocation, b: GeoLocation) => { | |
| // Reference: https://www.movable-type.co.uk/scripts/latlong.html | |
| const φ1 = a.lat * Math.PI / 180, φ2 = b.lat * Math.PI / 180; | |
| const λ1 = a.lng * Math.PI / 180, λ2 = b.lng * Math.PI / 180; | |
| // Calculate the differences | |
| const Δλ = λ2 - λ1; | |
| const y = Math.sin(Δλ) * Math.cos(φ2); | |
| const x = Math.cos(φ1) * Math.sin(φ2) - Math.sin(φ1) * Math.cos(φ2) * Math.cos(Δλ); | |
| // Calculate the bearing | |
| const θ = Math.atan2(y, x); | |
| return (θ * 180 / Math.PI + 360) % 360; // in degrees | |
| }; |
src/hooks/useStopGroup.tsx
Outdated
| const getAllRouteStopsBearings = (routeStops : RouteStopCoSeq[]) => { | ||
| // routeStop example: {"routeKey":"101+1+KENNEDY TOWN+KWUN TONG (YUE MAN SQUARE)","co":"ctb","seq":12} | ||
| return routeStops.map(routeStop => { | ||
| const { routeKey, co, seq } = routeStop; | ||
| const stopLength = routeList[routeKey].stops[co].length; | ||
| let stopA, stopB; | ||
| if(seq == stopLength - 1) { // last stop | ||
| // no next stop, hence no forward bearing, just use -1 as dummy value then discard it later | ||
| return -1; | ||
| } else { | ||
| stopA = stopList[routeList[routeKey].stops[co][seq]]; | ||
| stopB = stopList[routeList[routeKey].stops[co][seq + 1]]; | ||
| } | ||
| return getBearingStops(stopA, stopB); | ||
| }).filter(brng => brng !== -1); | ||
| } |
There was a problem hiding this comment.
Optimize the getAllRouteStopsBearings Function.
The function definition is correct but can be optimized by removing the unnecessary else clause.
- if(seq == stopLength - 1) { // last stop
- // no next stop, hence no forward bearing, just use -1 as dummy value then discard it later
- return -1;
- } else {
- stopA = stopList[routeList[routeKey].stops[co][seq]];
- stopB = stopList[routeList[routeKey].stops[co][seq + 1]];
- }
+ if(seq == stopLength - 1) return -1; // last stop
+ stopA = stopList[routeList[routeKey].stops[co][seq]];
+ stopB = stopList[routeList[routeKey].stops[co][seq + 1]];Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const getAllRouteStopsBearings = (routeStops : RouteStopCoSeq[]) => { | |
| // routeStop example: {"routeKey":"101+1+KENNEDY TOWN+KWUN TONG (YUE MAN SQUARE)","co":"ctb","seq":12} | |
| return routeStops.map(routeStop => { | |
| const { routeKey, co, seq } = routeStop; | |
| const stopLength = routeList[routeKey].stops[co].length; | |
| let stopA, stopB; | |
| if(seq == stopLength - 1) { // last stop | |
| // no next stop, hence no forward bearing, just use -1 as dummy value then discard it later | |
| return -1; | |
| } else { | |
| stopA = stopList[routeList[routeKey].stops[co][seq]]; | |
| stopB = stopList[routeList[routeKey].stops[co][seq + 1]]; | |
| } | |
| return getBearingStops(stopA, stopB); | |
| }).filter(brng => brng !== -1); | |
| } | |
| const getAllRouteStopsBearings = (routeStops : RouteStopCoSeq[]) => { | |
| // routeStop example: {"routeKey":"101+1+KENNEDY TOWN+KWUN TONG (YUE MAN SQUARE)","co":"ctb","seq":12} | |
| return routeStops.map(routeStop => { | |
| const { routeKey, co, seq } = routeStop; | |
| const stopLength = routeList[routeKey].stops[co].length; | |
| let stopA, stopB; | |
| if(seq == stopLength - 1) return -1; // last stop | |
| stopA = stopList[routeList[routeKey].stops[co][seq]]; | |
| stopB = stopList[routeList[routeKey].stops[co][seq + 1]]; | |
| return getBearingStops(stopA, stopB); | |
| }).filter(brng => brng !== -1); | |
| } |
Tools
Biome
[error] 51-54: This else clause can be omitted because previous branches break early.
Unsafe fix: Omit the else clause.
(lint/style/noUselessElse)
src/hooks/useStopGroup.tsx
Outdated
| const bearingTargets = getAllRouteStopsBearings(routeStops); | ||
| const isBearingInRange = (bearing : number) => { | ||
| if(BEARING_THRESHOLD >= 180 || bearingTargets.length == 0) { | ||
| return true; | ||
| } else { | ||
| for(let i = 0; i < bearingTargets.length; ++i) { | ||
| let bearingMin = bearingTargets[i] - BEARING_THRESHOLD; | ||
| let bearingMax = bearingTargets[i] + BEARING_THRESHOLD; | ||
| if(bearingMin < 0) | ||
| bearingMin += 360; | ||
| if(bearingMax > 360) | ||
| bearingMax -= 360; | ||
| if((bearingMin <= bearingMax && bearingMin <= bearing && bearing <= bearingMax) | ||
| || (bearingMin > bearingMax && (bearingMin <= bearing || bearing <= bearingMax))) // crossing 0/360° mark, eg min=340°,max=020° | ||
| return true; | ||
| } | ||
| return false; | ||
| } | ||
| } |
There was a problem hiding this comment.
Optimize the isBearingInRange Function.
The function definition is correct but can be optimized by removing the unnecessary else clause.
- if(BEARING_THRESHOLD >= 180 || bearingTargets.length == 0) {
- return true;
- } else {
- for(let i = 0; i < bearingTargets.length; ++i) {
- let bearingMin = bearingTargets[i] - BEARING_THRESHOLD;
- let bearingMax = bearingTargets[i] + BEARING_THRESHOLD;
- if(bearingMin < 0)
- bearingMin += 360;
- if(bearingMax > 360)
- bearingMax -= 360;
- if((bearingMin <= bearingMax && bearingMin <= bearing && bearing <= bearingMax)
- || (bearingMin > bearingMax && (bearingMin <= bearing || bearing <= bearingMax))) // crossing 0/360° mark, eg min=340°,max=020°
- return true;
- }
- return false;
- }
+ if(BEARING_THRESHOLD >= 180 || bearingTargets.length == 0) return true;
+ for(let i = 0; i < bearingTargets.length; ++i) {
+ let bearingMin = bearingTargets[i] - BEARING_THRESHOLD;
+ let bearingMax = bearingTargets[i] + BEARING_THRESHOLD;
+ if(bearingMin < 0) bearingMin += 360;
+ if(bearingMax > 360) bearingMax -= 360;
+ if((bearingMin <= bearingMax && bearingMin <= bearing && bearing <= bearingMax)
+ || (bearingMin > bearingMax && (bearingMin <= bearing || bearing <= bearingMax))) // crossing 0/360° mark, eg min=340°,max=020°
+ return true;
+ }
+ return false;Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const bearingTargets = getAllRouteStopsBearings(routeStops); | |
| const isBearingInRange = (bearing : number) => { | |
| if(BEARING_THRESHOLD >= 180 || bearingTargets.length == 0) { | |
| return true; | |
| } else { | |
| for(let i = 0; i < bearingTargets.length; ++i) { | |
| let bearingMin = bearingTargets[i] - BEARING_THRESHOLD; | |
| let bearingMax = bearingTargets[i] + BEARING_THRESHOLD; | |
| if(bearingMin < 0) | |
| bearingMin += 360; | |
| if(bearingMax > 360) | |
| bearingMax -= 360; | |
| if((bearingMin <= bearingMax && bearingMin <= bearing && bearing <= bearingMax) | |
| || (bearingMin > bearingMax && (bearingMin <= bearing || bearing <= bearingMax))) // crossing 0/360° mark, eg min=340°,max=020° | |
| return true; | |
| } | |
| return false; | |
| } | |
| } | |
| const bearingTargets = getAllRouteStopsBearings(routeStops); | |
| const isBearingInRange = (bearing : number) => { | |
| if(BEARING_THRESHOLD >= 180 || bearingTargets.length == 0) return true; | |
| for(let i = 0; i < bearingTargets.length; ++i) { | |
| let bearingMin = bearingTargets[i] - BEARING_THRESHOLD; | |
| let bearingMax = bearingTargets[i] + BEARING_THRESHOLD; | |
| if(bearingMin < 0) bearingMin += 360; | |
| if(bearingMax > 360) bearingMax -= 360; | |
| if((bearingMin <= bearingMax && bearingMin <= bearing && bearing <= bearingMax) | |
| || (bearingMin > bearingMax && (bearingMin <= bearing || bearing <= bearingMax))) // crossing 0/360° mark, eg min=340°,max=020° | |
| return true; | |
| } | |
| return false; | |
| } |
Tools
Biome
[error] 92-106: This else clause can be omitted because previous branches break early.
Unsafe fix: Omit the else clause.
(lint/style/noUselessElse)
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- src/hooks/useStopGroup.tsx (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- src/hooks/useStopGroup.tsx
add useMemo for routeStops
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- src/hooks/useStopGroup.tsx (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- src/hooks/useStopGroup.tsx
|
Temporarily changing status of this PR to "draft" as I found a critical problem (react keeps fetching data infinitely), seems react keep re-rendering infinitely. I suggest deploy the code to alpha site first if alpha site is available. |
- rename useStopGroup method as simple util method getStopGroup - use useMemo to cache result of getStopGroup to prevent infinite rendering
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/bookmarked-stop/StopRouteList.tsx (1 hunks)
- src/stopGroup.ts (1 hunks)
Additional comments not posted (13)
src/components/bookmarked-stop/StopRouteList.tsx (3)
3-3: Import statement approved.The
getStopGroupfunction is correctly imported from../../stopGroup.
11-11: Interface update approved.The
routeIdproperty is correctly added to theStopRouteListPropsinterface with the appropriate typestring | undefined.
15-20: Component update approved.The
StopRouteListcomponent correctly uses therouteIdprop and calls thegetStopGroupfunction with the new prop. The use ofuseMemoensures that the stop group is only recalculated when necessary, optimizing performance.src/stopGroup.ts (10)
1-3: Import statements approved.The necessary types and utility functions are correctly imported.
5-10: InterfaceuseStopGroupPropsapproved.The
useStopGroupPropsinterface is correctly defined with appropriate property types.
11-15: InterfaceStopListEntryExtendedapproved.The
StopListEntryExtendedinterface is correctly defined with appropriate additional properties.
16-20: InterfaceRouteStopCoSeqapproved.The
RouteStopCoSeqinterface is correctly defined with appropriate property types.
23-25: Function signature approved.The
getStopGroupfunction signature is correctly defined with appropriate parameters.
27-37: Constants and helper functions approved.The constants for distance, bearing, and stop list limits are appropriately set. The helper functions for distance and bearing calculations are correctly implemented.
39-52:getAllRouteStopsBearingsfunction approved.The function correctly calculates bearings for route stops and filters out invalid bearings.
54-82: Route stops collection logic approved.The logic for collecting route stops based on the presence of
routeIdand stop keys is correctly implemented.
103-139:searchNearbyStopsfunction approved.The function correctly searches for nearby stops within distance and bearing thresholds and adds them to the list if they meet the criteria.
145-175: Main logic ofgetStopGroupapproved.The function correctly implements the main logic for recursively searching for nearby stops, sorting them by distance, and returning the stop group.
|
This PR is "ready to reivew" again. The bug mentioned yesterday is solved by new commit. Can we deploy it to "alpha" site for testing just to play safee? By the way this PR should be linked to #160 |
|
This PR is being closed and will be replaced by new PR created at hkbus/hk-bus-crawling#26 |
Group bus stops in the vicinity to show ETAs together (although having different stop id)
Feature: Bus stops toward opposite direction are not shown
Current design:
Example screenshot (宋皇臺公園)
Before change (only 796X is shown)
After change (more routes are shown)
Summary by CodeRabbit
New Features
Bug Fixes
Refactor