fix(api): partition and sort schedule-for-stop by direction ID#1126
fix(api): partition and sort schedule-for-stop by direction ID#1126ARCoder181105 wants to merge 4 commits into
Conversation
…by headsign - Add direction_id to SQL queries and order schedule stop times by route, direction, and departure time - Refactor schedule-for-stop handler to group stop times into stopRouteDirectionSchedules objects - Sort direction schedules alphabetically by tripHeadsign using modern Go slices sorting - Add comprehensive unit tests verifying direction grouping and alphabetical sorting across routes
|
Warning Review limit reached
Next review available in: 42 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 (1)
📝 WalkthroughWalkthroughThe ChangesDirection-based schedule partitioning
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant ScheduleForStopHandler
participant GetScheduleForStopOnDate
Client->>ScheduleForStopHandler: request schedule for stop
ScheduleForStopHandler->>GetScheduleForStopOnDate: query rows with direction_id
GetScheduleForStopOnDate-->>ScheduleForStopHandler: rows with DirectionID
ScheduleForStopHandler->>ScheduleForStopHandler: group stopTimes by route+direction
ScheduleForStopHandler->>ScheduleForStopHandler: compute headsign per direction & sort
ScheduleForStopHandler-->>Client: response with stopRouteDirectionSchedules
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
![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
🧹 Nitpick comments (1)
internal/restapi/schedule_for_stop_handler_test.go (1)
628-669: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd a regression case for
NULL+0direction_idon the same route.This test only checks alphabetical
tripHeadsignordering, so it would still pass if rows withNULLand explicit0direction_idvalues were merged into one"0"bucket withscheduleStopTimesno longer sorted by departure time. A fixture that exercises that acceptance criterion would lock down the new defaulting behavior.🤖 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/schedule_for_stop_handler_test.go` around lines 628 - 669, Add a regression test in TestScheduleForStopHandlerDirectionPartitioning that uses a route fixture containing both NULL and explicit 0 direction_id values on the same route, then assert they remain split into distinct stopRouteDirectionSchedules buckets instead of merging into one "0" group. Also verify each bucket’s scheduleStopTimes stays sorted by departure time, so the test covers the defaulting behavior as well as the existing tripHeadsign ordering.
🤖 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 `@gtfsdb/query.sql`:
- Line 562: The ORDER BY in the query that feeds schedule stop times is using
raw t.direction_id while the handler normalizes missing direction values to "0";
update the sort key in the scheduleStopTimes query to use the same normalization
via COALESCE so NULL and 0 trips are grouped together. Adjust the ordering near
the r.id, t.direction_id, st.departure_time sort logic so the SQL sort matches
the later direction bucketing and preserves departure order within the merged
"0" bucket.
---
Nitpick comments:
In `@internal/restapi/schedule_for_stop_handler_test.go`:
- Around line 628-669: Add a regression test in
TestScheduleForStopHandlerDirectionPartitioning that uses a route fixture
containing both NULL and explicit 0 direction_id values on the same route, then
assert they remain split into distinct stopRouteDirectionSchedules buckets
instead of merging into one "0" group. Also verify each bucket’s
scheduleStopTimes stays sorted by departure time, so the test covers the
defaulting behavior as well as the existing tripHeadsign ordering.
🪄 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: d9874139-8f85-4618-8440-730d1220ef22
📒 Files selected for processing (4)
gtfsdb/query.sqlgtfsdb/query.sql.gointernal/restapi/schedule_for_stop_handler.gointernal/restapi/schedule_for_stop_handler_test.go
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
|
@coderabbitai full review |
✅ Action performedFull review finished. |
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
internal/restapi/schedule_for_stop_handler_test.go (1)
629-669: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest validates sorting but not actual direction partitioning.
This test only checks alphabetical ordering of
tripHeadsignacrossstopRouteDirectionSchedules; it never asserts that stop times are actually split bydirection_id(e.g., a route with multiple direction IDs should yield >1 direction group, and each group's stop times should share consistent trip directions). A regression that collapses all directions back into a single group would still pass this 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/schedule_for_stop_handler_test.go` around lines 629 - 669, The test in TestScheduleForStopHandlerDirectionPartitioning only verifies alphabetical ordering of tripHeadsign and does not confirm that stop times are actually partitioned by direction_id. Update the assertions so each stopRouteDirectionSchedules group is validated for consistent trip direction membership, and add a check that routes with multiple direction IDs produce multiple direction groups rather than collapsing into one. Use the existing identifiers stopRouteDirectionSchedules, tripHeadsign, and TestScheduleForStopHandlerDirectionPartitioning to locate the test logic.
🤖 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/schedule_for_stop_handler.go`:
- Around line 318-328: The headsign selection in the route/direction loop is
non-deterministic because `routeDirectionHeadsignCounts[routeID][dirID]` is a
map and ties on `count` currently depend on random iteration order. Update the
representative headsign choice in the `schedule_for_stop_handler` logic so ties
are broken deterministically, for example by comparing headsign strings after
counts match, while keeping the existing max-count behavior in the
`dirHeadsigns` / `tripHeadsign` selection.
---
Nitpick comments:
In `@internal/restapi/schedule_for_stop_handler_test.go`:
- Around line 629-669: The test in
TestScheduleForStopHandlerDirectionPartitioning only verifies alphabetical
ordering of tripHeadsign and does not confirm that stop times are actually
partitioned by direction_id. Update the assertions so each
stopRouteDirectionSchedules group is validated for consistent trip direction
membership, and add a check that routes with multiple direction IDs produce
multiple direction groups rather than collapsing into one. Use the existing
identifiers stopRouteDirectionSchedules, tripHeadsign, and
TestScheduleForStopHandlerDirectionPartitioning to locate the test logic.
🪄 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: 7472a520-5517-43c1-81f8-29a34ac671f6
📒 Files selected for processing (4)
gtfsdb/query.sqlgtfsdb/query.sql.gointernal/restapi/schedule_for_stop_handler.gointernal/restapi/schedule_for_stop_handler_test.go
…aking in scheduleForStopHandler
|
1 participant
Description
Partitions the
schedule-for-stopAPI response bydirection_idand sorts direction groups alphabetically bytripHeadsignSpec Requirement
Stop times for each route must be grouped into
stopRouteDirectionSchedulesby direction ID. When a route serves multiple directions at a stop, the direction groups must be sorted alphabetically bytripHeadsign.Implementation Gap
direction_id.Acceptance Criteria
stopRouteDirectionSchedulesbydirection_id.tripHeadsign.make test) passes with full verification.closes #1027
closes #1028
Summary by CodeRabbit
New Features
Bug Fixes