Disambiguate stops-for-route directions sharing a headsign - #1251
Conversation
When two direction groups resolved to the same most-common headsign, both were emitted with identical name.names, and the name-based group sort left their order nondeterministic. The reference implementation appends a direction disambiguator whenever directions collide. Append " - <direction id>" to every group's name on a collision, before the sort so ordering is deterministic. Update the RABA end-to-end test (both directions are "Shasta Lake") and add a unit test for the collision and no-collision paths.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughRoute stop-group assembly now detects shared destination names, appends each group ID to colliding name fields, and sorts the disambiguated groups. End-to-end and unit tests cover distinct names, collisions, and order-independent collision resolution. ChangesStop group name disambiguation
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
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. |
![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
🤖 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/stops_for_route_handler.go`:
- Around line 364-367: Update the group-name disambiguation logic around the
groups iteration in internal/restapi/stops_for_route_handler.go: retain
per-original-name occurrence counts and suffix only groups whose name occurs
more than once, leaving unique names unchanged. Update the expectation in
internal/restapi/stops_for_route_handler_test.go at lines 370-372 so the unique
Express group remains Express.
🪄 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: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 16d253b3-c587-4aa9-abcf-3484185baeaa
📒 Files selected for processing (2)
internal/restapi/stops_for_route_handler.gointernal/restapi/stops_for_route_handler_test.go
…ving unique names 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. |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/restapi/stops_for_route_handler.go (1)
350-370: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftMake generated names collision-safe.
A unique headsign can already equal a generated suffix:
A/A/A - 0becomesA - 0/A - 1/A - 0. The final names are still duplicated, and sorting byName.Nameremains input-order dependent. Ensure final names are unique and add a regression test for this case; an ID tie-breaker alone would not fix the duplicate wire names.🤖 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/stops_for_route_handler.go` around lines 350 - 370, Update disambiguateGroupNames so generated names are checked against all final names, including originally unique names, and collisions are resolved deterministically with an additional suffix or otherwise unique naming scheme. Preserve unchanged names when they remain unique, ensure both Name.Name and Name.Names match the final value, and add a regression test covering A/A/A - 0 to verify unique names and stable sorting independent of input order.
🤖 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.
Outside diff comments:
In `@internal/restapi/stops_for_route_handler.go`:
- Around line 350-370: Update disambiguateGroupNames so generated names are
checked against all final names, including originally unique names, and
collisions are resolved deterministically with an additional suffix or otherwise
unique naming scheme. Preserve unchanged names when they remain unique, ensure
both Name.Name and Name.Names match the final value, and add a regression test
covering A/A/A - 0 to verify unique names and stable sorting independent of
input order.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 0612fda2-b285-42b5-9d65-ef684ec6cddd
📒 Files selected for processing (2)
internal/restapi/stops_for_route_handler.gointernal/restapi/stops_for_route_handler_test.go
…lliding group names and add tests for edge cases
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. |
|
Code reviewNo issues found. Checked for bugs and CLAUDE.md compliance. 🤖 Generated with Claude Code |
aaronbrethorst
left a comment
aaronbrethorst
left a comment
There was a problem hiding this comment.
This is a tidy fix for a genuinely annoying bug — two groups both named "Shasta Lake" is useless to a rider, and it also left the name-based sort with a tie, so group order wasn't even stable. Nice that fixing the first problem fixes the second for free.
What I checked:
- The suffix format matches the reference implementation exactly:
name + " - " + direction. (The Java side also capitalizes the first character, but that's a no-op for numeric direction ids, so the output is identical.) name.typestays"destination"andname.namesis kept in sync withname.name— easy to forget one of those.- The function only rewrites name fields; it never adds, removes, or reorders groups, so nothing downstream can see a duplicate or empty group appear.
- Mutating through
groups[i]rather than a range copy is correct, and the result is a function of the group set alone — theTestDisambiguateGroupNamesorder-independence case is the right property to pin down, since it's what makes the subsequent sort deterministic. - Re-checking against the resulting names rather than the original ones is a nice touch, and the bound is safe: direction ids are unique, so any two colliding groups separate on the next pass.
The one deliberate divergence — suffixing only the colliding groups instead of every group whenever any collision exists — is documented in the doc comment, which I appreciate. It's unreachable today regardless: groupTripsByDirection keys on direction_id, which GTFS restricts to 0 and 1 (NULL coerces to 0), so you can't get three groups without the similarity-based grouping we haven't implemented. If we ever add that, this is worth revisiting, and the comment will point whoever does it in the right direction.
Updating the RABA end-to-end expectations rather than working around them was the right call too.
Merging.
2 participants
Fixes: #1250
When two direction groups resolved to the same most-common headsign,
both were emitted with identical name.names, and the name-based group
sort left their order nondeterministic. The reference implementation
appends a direction disambiguator whenever directions collide.
Append " - " to every group's name on a collision, before
the sort so ordering is deterministic. Update the RABA end-to-end test
(both directions are "Shasta Lake") and add a unit test for the
collision and no-collision paths.
Summary by CodeRabbit