Java-parity for arrivals/departures: deviation + distance + tripStatus#1000
Open
Ahmedhossamdev wants to merge 17 commits into
Open
Java-parity for arrivals/departures: deviation + distance + tripStatus#1000Ahmedhossamdev wants to merge 17 commits into
Ahmedhossamdev wants to merge 17 commits into
Conversation
Align distanceFromStop, numberOfStopsAway, and tripStatus with Java OneBusAway behavior. * Add scheduled_block_helper.go implementing Java BlockLocation logic * Port block snapshot and traversal algorithms from Java * Refactor GetScheduleDeviation to use block-wide last-wins iteration * Add 1-hour blockNotActive discard guard from GtfsRealtimeSource * Resolve response differences between Go and Java arrivals/departures APIs
|
Warning Review limit reached
Next review available in: 26 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughAdds scheduled-block snapshot computation, block-scoped schedule deviation lookup, and block-based distance/stop calculations in the REST handlers and trip-status path. Also updates nearby-stop selection, GTFS layover timing, OpenAPI nullability, and related tests. ChangesBlock snapshot, deviation, and handler integration
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Performance Smoke Test ResultsStatus: PASSED
Smoke test config: 5 VUs x 30s. Thresholds: p(95) < 300ms, error rate < 1%. Full results uploaded as workflow artifact: k6-smoke-summary. |
Performance Smoke Test ResultsStatus: PASSED
Smoke test config: 5 VUs x 30s. Thresholds: p(95) < 300ms, error rate < 1%. Full results uploaded as workflow artifact: k6-smoke-summary. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/restapi/trips_helper.go (1)
127-167:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftDon't switch
ActiveTripIDafter deriving stop fields from another trip.In the no-vehicle flow, Lines 153-166 compute
ClosestStop/NextStopfromdbTripID, then Lines 223-239 can rewritestatus.ActiveTripID, distance, position, and orientation tosnapshot.ActiveTripID. For blocks where a different trip is currently active, the response mixes stop IDs from one trip with geometry from another. Resolve the snapshot trip first, or recompute the stop fields after the active trip is known.Also applies to: 223-239
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/restapi/trips_helper.go` around lines 127 - 167, The code currently computes ClosestStop/NextStop from dbTripID (via findClosestStopByTimeWithDelays/findNextStopByTimeWithDelays and GetStopDelaysFromTripUpdates) before resolving snapshot.ActiveTripID and later overwrites status.ActiveTripID (status.ActiveTripID/snapshot.ActiveTripID), which can mix stop IDs from one trip with geometry from another; fix by resolving which trip is active first (determine snapshot.ActiveTripID) and then compute ClosestStop/NextStop for that active trip (or, if you must keep the existing dbTripID computation, recompute stop fields after you set status.ActiveTripID), updating calls to findClosestStopByTimeWithDelays/findNextStopByTimeWithDelays or the other stop-finding helpers accordingly so stop offsets and IDs always come from the final ActiveTripID used in the status.
🧹 Nitpick comments (3)
internal/restapi/scheduled_block_helper_test.go (1)
436-473: ⚡ Quick winAdd coverage for mixed authoritative/geometric stop projection.
The suite covers all-geometric and unusable-shape paths, but it still misses the important case where one stop uses
shape_dist_traveledand the next one falls back to geometry. That mixed path is exactly where the monotonic cursor can regress on loop routes.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/restapi/scheduled_block_helper_test.go` around lines 436 - 473, Add a new unit test exercising the mixed authoritative/geometric projection case so projectStopsInSequence is exercised when one StopTime has ShapeDistTraveled and the next falls back to geometry; create stopTimes where one element sets ShapeDistTraveled (e.g., 10.0) and the next has no ShapeDistTraveled but has resolvable stop coordinates, supply a simple shapePoints/cumDistances and a matching stopByID map (or use api.fetchStopCoordsForStopTimes pattern used elsewhere), call projectStopsInSequence and assert the returned distances length, non-negative values, and monotonic non-decreasing order (distances[i] >= distances[i-1]) to ensure the cursor does not regress on loop routes.internal/restapi/trip_updates_helper_test.go (1)
12-17: ⚡ Quick winThe new tests still bypass the primary static-schedule path.
Because these cases use synthetic trip IDs with no stop times,
GetScheduleDeviationForBlockonly exercises the trip-level-delay branch or the reverse-walk fallback. The changed “closest-in-time against scheduled arrivals/departures” logic never runs here, so the main block-level selection path is still unpinned. Please add at least one case backed by real stop times that goes through the scheduled/STU comparison branch.Also applies to: 23-150
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/restapi/trip_updates_helper_test.go` around lines 12 - 17, Tests in trip_updates_helper_test.go still use synthetic trip IDs (devDate/devNow) with no stop times so GetScheduleDeviationForBlock only exercises trip-level-delay or reverse-walk fallback and never hits the "closest-in-time against scheduled arrivals/departures" block-level selection path; add at least one test case that uses a real trip/block with actual stop_times (a known static DB trip) and assert behavior that exercises the scheduled vs STU comparison branch inside GetScheduleDeviationForBlock by creating a mock update for that real trip ID and timestamps that force the code to evaluate scheduled arrivals/departures rather than falling back to reverse-walk. Ensure the new test references the real trip ID you added and uses devDate/devNow or equivalent to place the update in time so the scheduled/STU comparison logic is executed.internal/restapi/trip_updates_helper.go (1)
33-179: 🏗️ Heavy liftSplit the deviation-selection branches into helpers.
This helper now mixes trip-update collection, trip-level overwrite semantics, static-schedule candidate ranking, and reverse-walk fallback in one path. That is already showing up as 73 cognitive complexity, and it will make future Java-parity fixes harder to reason about and test.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/restapi/trip_updates_helper.go` around lines 33 - 179, GetScheduleDeviationForBlock is doing four distinct responsibilities (collecting tripUpdates, computing trip-level overwrite, ranking scheduled-stop-time candidates, and reverse-walk fallback), making it too complex; refactor by extracting helpers: 1) collectTripUpdates(ctx, tripIDs) to build []tripUpdateForTrip (used instead of the inline loops), 2) selectTripLevelDeviation(tripUpdates) to implement the "LAST trip-level delay wins" logic and threshold check, 3) selectBestSTUDeviation(tripUpdates, serviceDate, currentTime, loadScheduledForTrip) that contains the scheduled-stop-time candidate ranking and the per-trip scheduled cache (move loadScheduled into loadScheduledForTrip helper), and 4) selectFallbackDelay(tripUpdates) to do the reverse-walk per-stop delay fallback; then make GetScheduleDeviationForBlock simply call these helpers in order and return early when a helper finds a value. Use existing symbols tripUpdates, tripUpdateForTrip, GetScheduleDeviationForBlock, and the Java threshold constant so callers/logic remain unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/restapi/arrival_and_departure_for_stop_handler.go`:
- Around line 357-365: The code currently applies the schedule shift only when a
vehicle is present (guarded by "vehicle != nil && vehicle.Trip != nil &&
vehicle.Trip.ID.ID != \"\""), which causes predicted times (Delay) to be applied
but distanceFromStop/numberOfStopsAway to be computed from an unshifted
schedule; remove that vehicle-presence guard so the deviation is applied
whenever api.GetScheduleDeviationForBlock returns hasRT. Specifically, keep the
blockTripIDs retrieval (api.blockTripIDsForServiceDate and
api.blockTripIDsSortedByStartTime) and call
api.GetScheduleDeviationForBlock(ctx, blockTripIDs, serviceDate, currentTime)
unconditionally; if hasRT is true set effectiveTime =
currentTime.Add(-time.Duration(dev) * time.Second).
In `@internal/restapi/arrivals_and_departures_for_stop_handler.go`:
- Around line 373-380: The effectiveTime is only adjusted when a vehicle is
present, so trips with delay-only TripUpdates still use schedule-based
distanceFromStop/numberOfStopsAway; call api.GetScheduleDeviationForBlock and
apply the same shift to effectiveTime even when vehicle == nil but a trip-level
delay is present (i.e., treat delay-only TripUpdates like RT vehicles).
Specifically, after computing blockTripIDs via api.blockTripIDsForServiceDate
and api.blockTripIDsSortedByStartTime, check for a trip-level delay (the
TripUpdate delay for st.TripID) and if present use dev from
api.GetScheduleDeviationForBlock to set effectiveTime =
params.Time.Add(-time.Duration(dev) * time.Second) just as in the
vehicle-present branch so arrivals, distanceFromStop and numberOfStopsAway use
the same shifted time.
In `@internal/restapi/scheduled_block_helper_test.go`:
- Around line 418-433: The test must not hard-code noon; instead derive
currentTime from the loaded snapshot data: call computeScheduledBlockSnapshot to
obtain the block/trip stop times, pick a deterministic stop timestamp from that
snapshot (e.g. use the last stop's ScheduledDeparture or a stop time guaranteed
after the trip start via snap.Stops[len(snap.Stops)-1].ScheduledDeparture), then
recreate the snapshot using that derived currentTime before calling
metricsForStop on firstStop (referencing computeScheduledBlockSnapshot,
snap.Stops, firstStop, and metricsForStop) so the sign assertions are
deterministic.
In `@internal/restapi/scheduled_block_helper.go`:
- Around line 69-71: computeScheduledBlockSnapshot is causing O(rows ×
blockSize) DB round-trips because it queries block/service-date once then calls
GetStopTimesForTrip, GetShapePointsByTripID, and GetStopsByIDs per trip and is
invoked per arrival row; batch these lookups before the handlers by collecting
all needed (blockID, serviceDate) pairs (and all tripIDs/stopIDs) and performing
a single batched query for stop times, shape points, and stops (or add a
memoized cache keyed by blockID+serviceDate) and then have
computeScheduledBlockSnapshot accept or read from that batch/cache so handlers
call it with pre-fetched results, reducing DB calls to O(1) per unique block
rather than per row.
- Around line 333-343: The code currently swallows errors from
GetShapePointsByTripID and appends trips with totalDist==0 which causes
downstream distance corruption; change the logic in the block using
GetShapePointsByTripID/shapeRowsToPoints/preCalculateCumulativeDistances so that
if GetShapePointsByTripID returns an error or shapePoints has fewer than 2
points you do NOT append the trip as a zero-length trip — either skip this trip
entirely or implement a dedicated stop-only fallback (call path:
projectStopsInSequence) before appending; specifically check and handle the
error from api.GtfsManager.GtfsDB.Queries.GetShapePointsByTripID, only compute
cumDistances/totalDist when len(shapePoints) >= 2 and otherwise branch to skip
or invoke the stop-only projection, and ensure any appending code uses the
validated totalDist/cumulative data.
- Around line 427-433: In the stopTimes loop inside the helper, when the branch
that uses st.ShapeDistTraveled.Valid writes distances[i] and continues, advance
the monotonic scan cursor by updating lastMatchedIndex (e.g., set
lastMatchedIndex = i) so subsequent geometry-based scans start from the
most-recent matched stop; this preserves the monotonicity guarantee for the
function that fills the distances slice.
In `@internal/restapi/trip_updates_helper.go`:
- Around line 78-93: The scheduledByTrip cache and loadScheduled function
currently collapse stop times by StopID, losing duplicate visits; update
loadScheduled (and any lookup logic that later matches STUs) to store multiple
scheduled entries per trip per stop by keying on stop_sequence when available
and otherwise appending occurrences (e.g., map[string][]{arr,dep,stop_sequence}
or similar) instead of a single struct; then change the matching logic that uses
scheduledByTrip to first attempt a match by stop_sequence (if STU has a
stop_sequence) and fall back to checking all stored occurrences for that StopID
to find the correct arrival/departure for loop/lasso trips.
- Around line 135-148: The code computes predicted using Time.Unix() -
serviceDate.Unix(), which uses epoch seconds and breaks across DST; instead
compute predicted as the service-day (wall-clock) seconds offset consistent with
schedArr/schedDep. Replace the two branches that set predicted from
stu.Arrival.Time and stu.Departure.Time so they convert the Time value into
seconds since the service-day start (the same basis used to compute
schedArr/schedDep) before subtracting the service-day start, and then pass that
value to considerCandidate; update the Arrival case (stu.Arrival.Time) and
Departure case (stu.Departure.Time) accordingly (keep other branches that use
Delay unchanged).
---
Outside diff comments:
In `@internal/restapi/trips_helper.go`:
- Around line 127-167: The code currently computes ClosestStop/NextStop from
dbTripID (via findClosestStopByTimeWithDelays/findNextStopByTimeWithDelays and
GetStopDelaysFromTripUpdates) before resolving snapshot.ActiveTripID and later
overwrites status.ActiveTripID (status.ActiveTripID/snapshot.ActiveTripID),
which can mix stop IDs from one trip with geometry from another; fix by
resolving which trip is active first (determine snapshot.ActiveTripID) and then
compute ClosestStop/NextStop for that active trip (or, if you must keep the
existing dbTripID computation, recompute stop fields after you set
status.ActiveTripID), updating calls to
findClosestStopByTimeWithDelays/findNextStopByTimeWithDelays or the other
stop-finding helpers accordingly so stop offsets and IDs always come from the
final ActiveTripID used in the status.
---
Nitpick comments:
In `@internal/restapi/scheduled_block_helper_test.go`:
- Around line 436-473: Add a new unit test exercising the mixed
authoritative/geometric projection case so projectStopsInSequence is exercised
when one StopTime has ShapeDistTraveled and the next falls back to geometry;
create stopTimes where one element sets ShapeDistTraveled (e.g., 10.0) and the
next has no ShapeDistTraveled but has resolvable stop coordinates, supply a
simple shapePoints/cumDistances and a matching stopByID map (or use
api.fetchStopCoordsForStopTimes pattern used elsewhere), call
projectStopsInSequence and assert the returned distances length, non-negative
values, and monotonic non-decreasing order (distances[i] >= distances[i-1]) to
ensure the cursor does not regress on loop routes.
In `@internal/restapi/trip_updates_helper_test.go`:
- Around line 12-17: Tests in trip_updates_helper_test.go still use synthetic
trip IDs (devDate/devNow) with no stop times so GetScheduleDeviationForBlock
only exercises trip-level-delay or reverse-walk fallback and never hits the
"closest-in-time against scheduled arrivals/departures" block-level selection
path; add at least one test case that uses a real trip/block with actual
stop_times (a known static DB trip) and assert behavior that exercises the
scheduled vs STU comparison branch inside GetScheduleDeviationForBlock by
creating a mock update for that real trip ID and timestamps that force the code
to evaluate scheduled arrivals/departures rather than falling back to
reverse-walk. Ensure the new test references the real trip ID you added and uses
devDate/devNow or equivalent to place the update in time so the scheduled/STU
comparison logic is executed.
In `@internal/restapi/trip_updates_helper.go`:
- Around line 33-179: GetScheduleDeviationForBlock is doing four distinct
responsibilities (collecting tripUpdates, computing trip-level overwrite,
ranking scheduled-stop-time candidates, and reverse-walk fallback), making it
too complex; refactor by extracting helpers: 1) collectTripUpdates(ctx, tripIDs)
to build []tripUpdateForTrip (used instead of the inline loops), 2)
selectTripLevelDeviation(tripUpdates) to implement the "LAST trip-level delay
wins" logic and threshold check, 3) selectBestSTUDeviation(tripUpdates,
serviceDate, currentTime, loadScheduledForTrip) that contains the
scheduled-stop-time candidate ranking and the per-trip scheduled cache (move
loadScheduled into loadScheduledForTrip helper), and 4)
selectFallbackDelay(tripUpdates) to do the reverse-walk per-stop delay fallback;
then make GetScheduleDeviationForBlock simply call these helpers in order and
return early when a helper finds a value. Use existing symbols tripUpdates,
tripUpdateForTrip, GetScheduleDeviationForBlock, and the Java threshold constant
so callers/logic remain unchanged.
🪄 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: 65ddc6c0-59fd-4329-9988-a1d7a41b0f35
📒 Files selected for processing (12)
internal/restapi/arrival_and_departure_for_stop_handler.gointernal/restapi/arrival_and_departure_for_stop_handler_test.gointernal/restapi/arrivals_and_departures_for_stop_handler.gointernal/restapi/block_distance_helper.gointernal/restapi/block_sequence_helper.gointernal/restapi/scheduled_block_helper.gointernal/restapi/scheduled_block_helper_test.gointernal/restapi/trip_updates_helper.gointernal/restapi/trip_updates_helper_test.gointernal/restapi/trips_helper.gointernal/restapi/trips_helper_test.gointernal/restapi/vehicles_helper.go
💤 Files with no reviewable changes (3)
- internal/restapi/block_sequence_helper.go
- internal/restapi/block_distance_helper.go
- internal/restapi/vehicles_helper.go
…ling - Update `projectStopsInSequence` to advance `lastMatchedIndex` after an authoritative `shape_dist_traveled` match, preventing subsequent geometric matches from regressing before the previous confirmed stop. - Fix `GetScheduleDeviationForBlock` for loop trips by preserving all `stop_time` occurrences for a given `stop_id` instead of collapsing them, and matching STUs using `stop_sequence` to select the correct scheduled arrival. - Add `TestProjectStopsInSequence_MixedAuthoritativeAndGeometric` to verify correct ordering when authoritative and geometric stop matching are combined. - Add `TestGetScheduleDeviationForBlock_ClosestInTimeAgainstRealSchedule` to verify schedule deviation calculations against real loop-trip schedules. - Add a TODO note documenting a potential DST edge case in time-based prediction calculations where UTC epoch timestamps are compared with service-day-relative seconds.
Performance Smoke Test ResultsStatus: PASSED
Smoke test config: 5 VUs x 30s. Thresholds: p(95) < 300ms, error rate < 1%. Full results uploaded as workflow artifact: k6-smoke-summary. |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/restapi/trip_updates_helper.go (1)
119-123:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRestrict the future-over-past preference to equal-distance candidates.
At Line 119,
(!isInPast && bestIsInPast)runs even when the future candidate is farther fromcurrentTimethan the current best past candidate. That can changescheduleDeviationaway from the true closest event, and the handlers then shift block interpolation time by the wrong amount.Suggested fix
- if delta < bestDelta || (!isInPast && bestIsInPast) { + if delta < bestDelta || (delta == bestDelta && !isInPast && bestIsInPast) { bestDelta = delta bestDeviation = int(deviation) bestIsInPast = isInPast found = true }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/restapi/trip_updates_helper.go` around lines 119 - 123, The current selection compares future vs past with "(!isInPast && bestIsInPast)" unconditionally, allowing a farther future candidate to override a closer past candidate; change the selection logic in the loop that updates bestDelta/bestDeviation/bestIsInPast/found so the future-over-past tie-breaker only applies when distances are equal (i.e., only prefer a future candidate if delta == bestDelta and !isInPast && bestIsInPast); keep the primary comparison delta < bestDelta unchanged and update the condition accordingly where bestDelta, delta, isInPast, and bestIsInPast are used.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/restapi/scheduled_block_helper.go`:
- Around line 436-444: The loop that advances lastMatchedIndex stops too early
when distances[i] exactly equals a cumulative breakpoint: change the check so
equality advances to the segment on the right. In the loop that iterates over
cumulativeDistances (using variables cumulativeDistances, distances and
lastMatchedIndex), replace the current if cumulativeDistances[j+1] >=
distances[i] logic with logic that treats equality as belonging to the
right-hand segment (e.g., use if cumulativeDistances[j+1] > distances[i] {
lastMatchedIndex = j; break } else if cumulativeDistances[j+1] == distances[i] {
lastMatchedIndex = j+1; break }) so the monotonic cursor moves to the correct
segment.
In `@internal/restapi/trip_updates_helper_test.go`:
- Around line 250-258: The mocked StopTimeUpdates in MockAddTripUpdate only set
StopID which breaks tests when mustGetTrip returns a trip with repeated stop
IDs; update the test to set unique StopSequence values on the
gtfs.StopTimeUpdate entries (e.g., set StopSequence for nearStop and farStop) so
the code that resolves updates by stop sequence (GetScheduleDeviationForBlock /
mustGetTrip) can disambiguate duplicate StopIDs, or alternatively detect when
the chosen stop IDs collide and skip the test case; modify the calls to
Api.GtfsManager.MockAddTripUpdate in this fixture to include StopSequence on the
gtfs.StopTimeUpdate structs (and mirror the same fix at the other block around
lines referenced).
---
Outside diff comments:
In `@internal/restapi/trip_updates_helper.go`:
- Around line 119-123: The current selection compares future vs past with
"(!isInPast && bestIsInPast)" unconditionally, allowing a farther future
candidate to override a closer past candidate; change the selection logic in the
loop that updates bestDelta/bestDeviation/bestIsInPast/found so the
future-over-past tie-breaker only applies when distances are equal (i.e., only
prefer a future candidate if delta == bestDelta and !isInPast && bestIsInPast);
keep the primary comparison delta < bestDelta unchanged and update the condition
accordingly where bestDelta, delta, isInPast, and bestIsInPast are used.
🪄 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: 761b6b85-5a0b-42ff-9399-d3503871787c
📒 Files selected for processing (5)
internal/restapi/scheduled_block_helper.gointernal/restapi/scheduled_block_helper_test.gointernal/restapi/trip_updates_helper.gointernal/restapi/trip_updates_helper_test.gotestdata/openapi.yml
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/restapi/scheduled_block_helper_test.go
…lexity threshold without changing behavior. - Refactor `GetScheduleDeviationForBlock` into smaller helper functions for trip update collection, schedule matching, and deviation calculation. - Refactor `projectStopsInSequence` by extracting shape-distance scaling, cursor advancement, and geometric stop projection helpers. - Refactor `computeScheduledBlockSnapshot` by moving stop emission logic into a dedicated helper. All tests continue to pass and functionality remains unchanged.
Performance Smoke Test ResultsStatus: PASSED
Smoke test config: 5 VUs x 30s. Thresholds: p(95) < 300ms, error rate < 1%. Full results uploaded as workflow artifact: k6-smoke-summary. |
Performance Smoke Test ResultsStatus: PASSED
Smoke test config: 5 VUs x 30s. Thresholds: p(95) < 300ms, error rate < 1%. Full results uploaded as workflow artifact: k6-smoke-summary. |
…nd enhance test coverage - Updated error handling in arrivalsAndDeparturesForStopHandler to surface 404 for unknown agencies. - Adjusted nearby stop fetching logic to align with Java implementation, setting radius to 100m. - Enhanced test cases for arrivals and departures, ensuring trip IDs are correctly matched and delays are accurately reflected. - Introduced isTripActive function to determine if trip updates are still relevant based on current time. - Modified GetStopDelaysFromTripUpdates to include current time for better accuracy in delay calculations. - Improved performance comments and TODOs in scheduled block helper and trip updates helper.
Performance Smoke Test ResultsStatus: PASSED
Smoke test config: 5 VUs x 30s. Thresholds: p(95) < 300ms, error rate < 1%. Full results uploaded as workflow artifact: k6-smoke-summary. |
Performance Smoke Test ResultsStatus: PASSED
Smoke test config: 5 VUs x 30s. Thresholds: p(95) < 300ms, error rate < 1%. Full results uploaded as workflow artifact: k6-smoke-summary. |
Performance Smoke Test ResultsStatus: PASSED
Smoke test config: 5 VUs x 30s. Thresholds: p(95) < 300ms, error rate < 1%. Full results uploaded as workflow artifact: k6-smoke-summary. |
Performance Smoke Test ResultsStatus: PASSED
Smoke test config: 5 VUs x 30s. Thresholds: p(95) < 300ms, error rate < 1%. Full results uploaded as workflow artifact: k6-smoke-summary. |
Performance Smoke Test ResultsStatus: PASSED
Smoke test config: 5 VUs x 30s. Thresholds: p(95) < 300ms, error rate < 1%. Full results uploaded as workflow artifact: k6-smoke-summary. |
Member
Author
|
Hey @aaronbrethorst, |
… in arrival and departure handler
…rrivals and departures
…ivals and departures
…shift logic with overlapping trips
…eDate and fetchStopCoordsForStopTimes functions
…tartTime function
Performance Smoke Test ResultsStatus: PASSED
Smoke test config: 5 VUs x 30s. Thresholds: p(95) < 300ms, error rate < 1%. Full results uploaded as workflow artifact: k6-smoke-summary. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/restapi/scheduled_block_helper_test.go (1)
530-536: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTop comment mismatches actual fixture.
The header comment says "stop_B at sequence 4 is at the same (0, 0) coords" and projects "near point[3]", but the actual fixture (lines 555-571) puts the duplicate coords on
stop_C(sequence 3), and the assertion at line 586 correctly comparesdistances[2](stop_C) vsdistances[0], notdistances[3]. The later comment block (lines 556-559, 584-588) is accurate; only this top summary is stale/inconsistent and could mislead anyone debugging this complex loop-route projection logic.📝 Suggested comment fix
-// We build a figure-eight shape that revisits (0, 0) at the midpoint: -// stop_A at sequence 1 is at the start (shape segment 0); stop_B at -// sequence 4 is at the same (0, 0) coords but should project to the -// LATER segment near point[3]. With the monotonic cursor advancing, -// distances[3] must be strictly greater than distances[0] — both -// physical coordinates are identical, only the cursor tells them apart. +// We build a figure-eight shape that revisits (0, 0) at the midpoint: +// stop_A at sequence 1 is at the start (shape segment 0); stop_C at +// sequence 3 is at the same (0, 0) coords but should project to the +// LATER segment near point[2]. With the monotonic cursor advancing, +// distances[2] must be strictly greater than distances[0] — both +// physical coordinates are identical, only the cursor tells them apart.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/restapi/scheduled_block_helper_test.go` around lines 530 - 536, The top summary comment in the scheduled block helper test is stale and no longer matches the fixture or assertion. Update the narrative near the figure-eight setup to refer to the actual duplicated point holder and the correct sequence/index mapping used by the projection logic in this test, so it aligns with the `stop_C` fixture and the `distances[2]` vs `distances[0]` assertion. Keep the later detailed comment block unchanged if it is already accurate; only revise the mismatched summary text to reflect the current test data and cursor-based projection behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@internal/restapi/scheduled_block_helper_test.go`:
- Around line 530-536: The top summary comment in the scheduled block helper
test is stale and no longer matches the fixture or assertion. Update the
narrative near the figure-eight setup to refer to the actual duplicated point
holder and the correct sequence/index mapping used by the projection logic in
this test, so it aligns with the `stop_C` fixture and the `distances[2]` vs
`distances[0]` assertion. Keep the later detailed comment block unchanged if it
is already accurate; only revise the mismatched summary text to reflect the
current test data and cursor-based projection behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: d5a806cf-2402-40eb-94dc-1fc9f6bb81be
📒 Files selected for processing (6)
internal/restapi/arrival_and_departure_for_stop_handler.gointernal/restapi/arrivals_and_departures_for_stop_handler.gointernal/restapi/arrivals_and_departures_for_stop_handler_test.gointernal/restapi/scheduled_block_helper.gointernal/restapi/scheduled_block_helper_test.gointernal/restapi/trip_updates_helper.go
🚧 Files skipped from review as they are similar to previous changes (4)
- internal/restapi/arrivals_and_departures_for_stop_handler.go
- internal/restapi/trip_updates_helper.go
- internal/restapi/arrival_and_departure_for_stop_handler.go
- internal/restapi/scheduled_block_helper.go
|
Performance Smoke Test ResultsStatus: PASSED
Smoke test config: 5 VUs x 30s. Thresholds: p(95) < 300ms, error rate < 1%. Full results uploaded as workflow artifact: k6-smoke-summary. |
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
1 participant
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.
Java parity for arrival / departure trip status
Summary
This PR makes Maglev's arrival/departure responses follow Java OneBusAway's
block-location algorithm for:
distanceFromStopnumberOfStopsAwaytripStatus.scheduleDeviationtripStatus.scheduledDistanceAlongTriptripStatusposition/orientation fieldsThe previous implementation mixed GPS projection, per-trip GTFS-RT delay,
and publisher-specific
current_stop_sequencesemantics. That producedzeros for scheduled-only rows, large loop-route distance errors, unit
mismatches when
shape_dist_traveledwas not metres, and schedule-deviationdifferences on multi-trip blocks.
The new path mirrors Java's model: compute a block-wide schedule deviation,
interpolate the scheduled block at
currentTime - scheduleDeviation, thenderive stop metrics from block distance and block stop sequence.
When GTFS-RT is unavailable for an arrival, Maglev now still computes the bus's
scheduled position by interpolating the static GTFS block at the request time.
That gives scheduled-only rows meaningful
distanceFromStop,numberOfStopsAway, andtripStatusfields instead of leaving them at zero.Scheduled interpolation math
With GTFS-RT, the scheduled snapshot is evaluated at:
Without GTFS-RT,
effectiveTimeis just the request time.The snapshot builder finds the two scheduled block stops around that time:
Then it computes how far the effective time is through that scheduled segment:
Finally, it linearly interpolates the block distance:
If the request time is halfway between two scheduled stops, the snapshot
position is halfway along the scheduled block distance between those stops.
Before the first stop it clamps to the first stop distance; after the last stop
it clamps to the last stop distance.
The arrival metrics are then:
Positive means the target stop is still ahead; negative means the scheduled
position has already passed it.
Positive means the target stop is that many stops ahead; zero means it is the
next/current stop; negative means it is behind.
Worked example
Assume the scheduled block has these stops:
If the request time is 12:05:00 and there is no GTFS-RT, then:
For target stop B:
The bus's scheduled position is 500 m before B, and B is the next scheduled
stop.
For target stop C:
C is 2,000 m ahead of the scheduled position and one stop after the next stop.
If GTFS-RT says the bus is 2 minutes late at 12:05:00, then:
The same formulas still apply; the only difference is that the scheduled
snapshot is evaluated 2 minutes earlier because the bus is late.
What Changed
Added
internal/restapi/scheduled_block_helper.go.DistanceAlongBlock,NextStopIndex, active trip, and active-trip scheduled distance.distanceFromStopandnumberOfStopsAway.Reworked
GetScheduleDeviationForBlock.blockNotActiveguard by discarding deviations over 1 hour.Updated both arrivals handlers.
tripStatus.minutesBeforenow caps at 240 minutes, matchingminutesAfter.Updated
BuildTripStatus.scheduledDistanceAlongTripfrom the active block trip.vehicleIdinstead of double-prefixing it.Removed superseded helpers.
block_distance_helper.goblock_sequence_helper.goRefreshed
testdata/openapi.ymlfrom upstream.Java References
The implementation was checked against these Java paths:
GtfsRealtimeTripLibrary.applyTripUpdatesToRecordGtfsRealtimeSource.blockNotActiveArrivalsAndDeparturesBeanServiceImpl.applyBlockLocationToBeandistanceFromStop = stopTime.distanceAlongBlock - blockLocation.distanceAlongBlocknumberOfStopsAway = stopTime.blockSequence - nextStop.blockSequenceTripStatusBeanServiceImplscheduledDistanceAlongTrip = scheduledDistanceAlongBlock - activeBlockTrip.distanceAlongBlockDistanceAlongShapeLibrary.computeBestAssignmentValidation
Added focused tests for:
Live comparison against Java showed exact schedule-deviation parity on the
sampled Unitrans arrivals, with remaining differences limited to stop-boundary
and independent-feed-snapshot timing effects.
Follow-Ups
amplification.
Java's stepped GTFS-RT-poll cadence.
agency-idsfiltering case-insensitive so config casing cannotsilently drop feeds.
Summary by CodeRabbit
Bug Fixes
Improvements
Documentation