refactor: use structs for location params - #892
Merged
Conversation
Update several functions that passed LocationParams individually to just accept a LocationParams struct directly. Move checkIfOurOfBounds logic into manager to remove some duplication.
Contributor
📝 WalkthroughWalkthroughConsolidates location query parameters into a new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
internal/restapi/trips_for_location_handler.go (1)
133-141: Consider simplifying by passinglocdirectly to the handler.The
parseAndValidateRequestfunction parses location into a*internalgtfs.LocationParamsat line 133-134, then immediately unpacks it back into separatelat,lon,latSpan,lonSpanvariables at lines 137-140, only to reconstruct newLocationParamsstructs at lines 40 and 120.This could be simplified by returning
locdirectly fromparseAndValidateRequestand using it throughout the handler, avoiding the unpacking/repacking cycle.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/restapi/trips_for_location_handler.go` around lines 133 - 141, parseLocationParams is parsed into a *internalgtfs.LocationParams then immediately unpacked into lat/lon/latSpan/lonSpan only to be reconstructed later; change parseAndValidateRequest (or the handler signature) to return or accept the *internalgtfs.LocationParams directly so the handler uses that loc throughout instead of unpacking/repacking. Update the code paths that currently build new LocationParams (the constructions near the top and around where trips are requested) to consume the existing loc pointer, and adjust call sites of parseAndValidateRequest/parseLocationParams to propagate the *internalgtfs.LocationParams return value instead of separate primitives. Ensure any nil checks and validation remain (use loc == nil) and remove the redundant variable unpacking (lat, lon, latSpan, lonSpan).internal/gtfs/location_params.go (1)
23-27: HandleNoRadiusLimitsentinel value in radius fallback.The constant
NoRadiusLimit = -1is defined ingtfs_manager.go. If a caller passesRadius: -1(NoRadiusLimit) without validLatSpan/LonSpan, this code will pass-1toCalculateBounds, potentially producing invalid bounds.Current call sites appear safe (they provide valid spans when using -1), but this is fragile.
Proposed fix
radius := loc.Radius - if radius == 0 { + if radius <= 0 { radius = models.DefaultSearchRadiusInMeters } return utils.CalculateBounds(loc.Lat, loc.Lon, radius)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/gtfs/location_params.go` around lines 23 - 27, The current radius fallback uses loc.Radius directly and can pass the sentinel NoRadiusLimit (-1) into utils.CalculateBounds; change the logic in the function that computes bounds (the block using loc.Radius, models.DefaultSearchRadiusInMeters and calling utils.CalculateBounds) so that if loc.Radius == 0 OR loc.Radius == NoRadiusLimit and both loc.LatSpan and loc.LonSpan are zero, you set radius to models.DefaultSearchRadiusInMeters before calling utils.CalculateBounds; only allow NoRadiusLimit (-1) to be used as-is when a caller also provides non-zero loc.LatSpan or loc.LonSpan. This ensures utils.CalculateBounds never receives -1 unexpectedly while preserving the intended NoRadiusLimit behavior when spans are present.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/restapi/arrivals_and_departure_for_stop.go`:
- Around line 642-643: The LocationParams is currently setting LatSpan and
LonSpan which causes boundsFromParams to prefer span-based bounds and ignore
Radius; update the construction of the params passed to
api.GtfsManager.GetStopIDsWithinBounds so that LatSpan and LonSpan are omitted
or set to zero (e.g., instantiate &internalgtfs.LocationParams{Lat: lat, Lon:
lon, Radius: 10000}) so the radius-based search is used when calling
GetStopIDsWithinBounds.
In `@internal/restapi/trips_for_location_handler.go`:
- Line 40: Remove the unnecessary Radius: -1 field from the LocationParams
passed to api.GtfsManager.GetStopsInBounds on this call; update the call that
currently uses &internalgtfs.LocationParams{Lat: lat, Lon: lon, Radius: -1,
LatSpan: latSpan, LonSpan: lonSpan} to omit Radius entirely (use
&internalgtfs.LocationParams{Lat: lat, Lon: lon, LatSpan: latSpan, LonSpan:
lonSpan}) so it matches the other call pattern and avoids violating the
non-negative radius validation in GetStopsInBounds.
---
Nitpick comments:
In `@internal/gtfs/location_params.go`:
- Around line 23-27: The current radius fallback uses loc.Radius directly and
can pass the sentinel NoRadiusLimit (-1) into utils.CalculateBounds; change the
logic in the function that computes bounds (the block using loc.Radius,
models.DefaultSearchRadiusInMeters and calling utils.CalculateBounds) so that if
loc.Radius == 0 OR loc.Radius == NoRadiusLimit and both loc.LatSpan and
loc.LonSpan are zero, you set radius to models.DefaultSearchRadiusInMeters
before calling utils.CalculateBounds; only allow NoRadiusLimit (-1) to be used
as-is when a caller also provides non-zero loc.LatSpan or loc.LonSpan. This
ensures utils.CalculateBounds never receives -1 unexpectedly while preserving
the intended NoRadiusLimit behavior when spans are present.
In `@internal/restapi/trips_for_location_handler.go`:
- Around line 133-141: parseLocationParams is parsed into a
*internalgtfs.LocationParams then immediately unpacked into
lat/lon/latSpan/lonSpan only to be reconstructed later; change
parseAndValidateRequest (or the handler signature) to return or accept the
*internalgtfs.LocationParams directly so the handler uses that loc throughout
instead of unpacking/repacking. Update the code paths that currently build new
LocationParams (the constructions near the top and around where trips are
requested) to consume the existing loc pointer, and adjust call sites of
parseAndValidateRequest/parseLocationParams to propagate the
*internalgtfs.LocationParams return value instead of separate primitives. Ensure
any nil checks and validation remain (use loc == nil) and remove the redundant
variable unpacking (lat, lon, latSpan, lonSpan).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 54b16ae9-8dd9-420d-841b-07185e5e7177
📒 Files selected for processing (10)
internal/gtfs/gtfs_manager.gointernal/gtfs/gtfs_manager_test.gointernal/gtfs/location_params.gointernal/restapi/arrivals_and_departure_for_stop.gointernal/restapi/arrivals_and_departures_for_stop_handler_test.gointernal/restapi/context_cancellation_test.gointernal/restapi/location_params.gointernal/restapi/routes_for_location_handler.gointernal/restapi/stops_for_location_handler.gointernal/restapi/trips_for_location_handler.go
Comment on lines
+642
to
+643
| loc := &internalgtfs.LocationParams{Lat: lat, Lon: lon, Radius: 10000, LatSpan: 100, LonSpan: 100} | ||
| nearbyIDs := api.GtfsManager.GetStopIDsWithinBounds(ctx, loc, 5) |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify how GetStopIDsWithinBounds handles the combination of Radius and LatSpan/LonSpan
ast-grep --pattern $'func ($_ *Manager) GetStopIDsWithinBounds($$$) $_ {
$$$
}'
# Check if boundsFromParams prioritizes one over the other
rg -n "LatSpan|LonSpan|Radius" internal/gtfs/location_params.go -B 2 -A 2Repository: OneBusAway/maglev
Length of output: 2388
Remove LatSpan and LonSpan to use the intended radius-based search.
The boundsFromParams function prioritizes span-based bounds when both LatSpan > 0 and LonSpan > 0, completely ignoring the Radius parameter. Setting LatSpan: 100, LonSpan: 100 causes a continent-scale search (~11,000km per side) instead of the intended 10km radius. Change to:
Diff
-loc := &internalgtfs.LocationParams{Lat: lat, Lon: lon, Radius: 10000, LatSpan: 100, LonSpan: 100}
+loc := &internalgtfs.LocationParams{Lat: lat, Lon: lon, Radius: 10000}📝 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.
Suggested change
| loc := &internalgtfs.LocationParams{Lat: lat, Lon: lon, Radius: 10000, LatSpan: 100, LonSpan: 100} | |
| nearbyIDs := api.GtfsManager.GetStopIDsWithinBounds(ctx, loc, 5) | |
| loc := &internalgtfs.LocationParams{Lat: lat, Lon: lon, Radius: 10000} | |
| nearbyIDs := api.GtfsManager.GetStopIDsWithinBounds(ctx, loc, 5) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/restapi/arrivals_and_departure_for_stop.go` around lines 642 - 643,
The LocationParams is currently setting LatSpan and LonSpan which causes
boundsFromParams to prefer span-based bounds and ignore Radius; update the
construction of the params passed to api.GtfsManager.GetStopIDsWithinBounds so
that LatSpan and LonSpan are omitted or set to zero (e.g., instantiate
&internalgtfs.LocationParams{Lat: lat, Lon: lon, Radius: 10000}) so the
radius-based search is used when calling GetStopIDsWithinBounds.
Remove custom bounding box logic, reuse logic from gtfsmanager. Fix bug that was revealed by this change.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/gtfs/location_params.go`:
- Around line 19-28: BoundsFromParams currently dereferences a nilable
*LocationParams and can panic; change its signature from func
BoundsFromParams(loc *LocationParams) to func BoundsFromParams(loc
LocationParams) (and update upstream callers and manager methods to accept a
value) so nil is impossible, or alternatively add an immediate nil-guard at the
top of BoundsFromParams that returns a sensible default bounds when loc==nil;
ensure any references to loc.Lat/loc.Lon/loc.Radius are updated to the value
parameter (or guarded) and keep the same calls to utils.CalculateBoundsFromSpan
and utils.CalculateBounds with models.DefaultSearchRadiusInMeters fallback.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 8ceb407e-2d51-441b-b19c-42062602c160
📒 Files selected for processing (8)
internal/gtfs/gtfs_manager.gointernal/gtfs/gtfs_manager_test.gointernal/gtfs/location_params.gointernal/restapi/arrivals_and_departure_for_stop.gointernal/restapi/arrivals_and_departures_for_stop_handler_test.gointernal/restapi/stops_for_location_handler.gointernal/restapi/trips_for_location_handler.gointernal/restapi/trips_for_location_handler_test.go
✅ Files skipped from review due to trivial changes (1)
- internal/restapi/trips_for_location_handler_test.go
🚧 Files skipped from review as they are similar to previous changes (4)
- internal/restapi/arrivals_and_departures_for_stop_handler_test.go
- internal/restapi/arrivals_and_departure_for_stop.go
- internal/restapi/stops_for_location_handler.go
- internal/restapi/trips_for_location_handler.go
Comment on lines
+19
to
+28
| func BoundsFromParams(loc *LocationParams) utils.CoordinateBounds { | ||
| if loc.LatSpan > 0 && loc.LonSpan > 0 { | ||
| return utils.CalculateBoundsFromSpan(loc.Lat, loc.Lon, loc.LatSpan/2, loc.LonSpan/2) | ||
| } | ||
| radius := loc.Radius | ||
| if radius == 0 { | ||
| radius = models.DefaultSearchRadiusInMeters | ||
| } | ||
| return utils.CalculateBounds(loc.Lat, loc.Lon, radius) | ||
| } |
Contributor
There was a problem hiding this comment.
Guard shared bounds conversion against nil input to prevent panics.
BoundsFromParams dereferences loc unconditionally. Because multiple manager APIs now accept *LocationParams, a nil value will crash request handling instead of returning a safe response.
A robust fix is to make LocationParams a value parameter on this helper (and upstream manager methods), so nil is impossible by type.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/gtfs/location_params.go` around lines 19 - 28, BoundsFromParams
currently dereferences a nilable *LocationParams and can panic; change its
signature from func BoundsFromParams(loc *LocationParams) to func
BoundsFromParams(loc LocationParams) (and update upstream callers and manager
methods to accept a value) so nil is impossible, or alternatively add an
immediate nil-guard at the top of BoundsFromParams that returns a sensible
default bounds when loc==nil; ensure any references to
loc.Lat/loc.Lon/loc.Radius are updated to the value parameter (or guarded) and
keep the same calls to utils.CalculateBoundsFromSpan and utils.CalculateBounds
with models.DefaultSearchRadiusInMeters fallback.
This was referenced Jul 7, 2026
This was referenced Aug 1, 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.
Update several functions that passed LocationParams individually to just accept a LocationParams struct directly.
Move checkIfOurOfBounds logic into manager to remove some duplication.
Summary by CodeRabbit
Release Notes
Refactors