Fix/stop routes missing from reference routes in trips-for-route-handler

[JohnAkindipe]

Fix dangling stop route references in trips-for-route

Closes #1339

Problem

In the trips-for-route response, references.routes was built exclusively from
the routes of the returned trips: buildTripReferences seeded sets.routes from
trip.RouteID (collectPreFetchedTrips) and then resolved routes and agencies in
fillRoutesAndAgencies. A stop served by a route that no returned trip runs on
emits a routeId that resolves to nothing in references.routes.

Fix

internal/restapi/trips_for_route_handler.go — buildTripReferences:

1. Call api.stopReferences before fillRoutesAndAgencies and keep the returned
   route-IDs-by-stop map.
2. Register those route IDs into sets.routes before route/agency resolution runs.
3. fillRoutesAndAgencies then resolves both the full route objects and their
   agencies (addAgencyReference), so routes and agencies are filled in by the one
   existing mechanism.

Testing

Added a regression test TestTripsForRouteHandler_StopRoutesResolveInReferences with a
synthetic fixture, orphanStopRouteFiles(). The test asserts that routes serving stops which
are not associated with any returned trips are correctly resolved in references.routes
alongside the route agencies in references.agencies. It fails in the pre-fix code and passes
post-fix

Summary by CodeRabbit

• Bug Fixes
  • Route and agency references are now included when they are associated with returned stops, even if no active trip is returned for those routes.
  • Results continue to include only active trips for the specifically queried route.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Repository UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 1ecdac70-0899-41bc-8112-d16fb3491d37

📥 Commits

Reviewing files that changed from the base of the PR and between 8554824 and e026efb.

📒 Files selected for processing (2)

• internal/restapi/trips_for_route_handler.go
• internal/restapi/trips_for_route_handler_test.go

Included review availability: Your plan includes up to 1 review per rolling hour; 0 remain after this review.

---

📝 Walkthrough

Walkthrough

The trips-for-route handler now includes routes and agencies referenced by stops, even when those routes have no active returned trip. Tests cover reference resolution and exclude the inactive trip from results.

Changes

Trips-for-route references

Layer / File(s)	Summary
Resolve stop-referenced routes and agencies internal/restapi/trips_for_route_handler.go	buildTripReferences registers route IDs from stop references, resolves missing routes and agencies, and documents the raw route ID map keys.
Validate orphan route references internal/restapi/trips_for_route_handler_test.go	The fixture adds an inactive trip for an additional route. Tests verify that the route and agency references resolve while the inactive trip remains excluded.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Merge Risk: ⚪ Minimal · up to e026e

This localized change adds stop-served routes and their agencies to trip references, with a regression test covering the behavior. No actionable merge-blocking risk remains beyond normal checks and review.

Suggested reviewers: 3rabiii, arcoder181105

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly identifies the fix for missing stop-referenced routes in trips-for-route responses.
Linked Issues check	✅ Passed	The implementation registers stop-referenced routes before resolution and verifies route and agency references with a regression test for issue #1339.
Out of Scope Changes check	✅ Passed	The code and test changes directly support issue #1339, with no unrelated functional changes identified.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[aaronbrethorst]

Code review

Found 1 issue:

1. The stop-route registration loop is inlined into buildTripReferences and is a third copy of logic that already exists as referenceBuilder.buildStopList in trips_for_location_handler.go:665-679 (same stopReferences → ExtractCodeID → seed the route set shape). buildTripReferences otherwise reads as a sequence of named stages, and commit c00a305 ("Build every stop reference through one helper") folded the previous trips-for-location/trips-for-route divergence of this exact pair of functions into reference_utils.go precisely because the copies drifted. Worth extracting a shared helper (e.g. in reference_utils.go), or at minimum a named sets.collectRoutesFromStops(routeIDsByStopID) method alongside the existing collectPreFetchedTrips/collectTripIDsFromEntries. CONTRIBUTING.md says "Before writing new logic, check whether it already exists — this is the single most common category of review comment" and "Wrap related code in functions. If a block performs a distinct sub-task, extract it into its own well-named function rather than inlining it." Note the inline copy also drops the explanatory comment the trips-for-location copy carries about why the raw rather than combined ID is registered.

maglev/internal/restapi/trips_for_route_handler.go

Lines 748 to 760 in e026efb

	for _, combinedRouteIDs := range routeIDsByStopID {
	for _, combinedID := range combinedRouteIDs {
	rawID, err := utils.ExtractCodeID(combinedID)
	if err != nil {
	continue
	}
	if _, exists := sets.routes[rawID]; !exists {
	sets.routes[rawID] = models.Route{}
	}
	}
	}

🤖 Generated with Claude Code

_{- If this code review was useful, please react with 👍. Otherwise, react with 👎.}

[aaronbrethorst]

Good fix, and the test fixture is the part I want to call out.

orphanStopRouteFiles builds a stop served by a route from a second agency
that no returned trip runs on, on a different block, at a different time — which
is precisely the case where the old code emitted a routeIds entry with nothing
behind it. Constructing that scenario deliberately is more work than asserting
against the RABA fixture and worth much more, because it fails pre-fix for the
right reason rather than by coincidence. The assert.Contains(stop.RouteIDs, combinedOrphanRouteID) self-guard on the fixture is a nice touch too — it means
the test can't silently stop testing anything if the fixture drifts.

Correctness checks that hold up:

• Moving stopReferences above fillRoutesAndAgencies and seeding sets.routes
  from the stops' route IDs is the right ordering — the routes have to be known
  before the batch fetch runs, not after.
• No N+1 introduced: the new loop is pure in-memory over the map
  stopReferences already returns, and route resolution still goes through the
  single batched GetRoutesByIDs.
• Agencies come along via addAgencyReference, so the second agency resolves too
  — which is the half of #1339 that's easy to miss.

One follow-up, not blocking: the stop-route registration loop is close to a
verbatim copy of buildStopList in trips_for_location_handler.go, and per
CONTRIBUTING.md's Code Reuse section that belongs in reference_utils.go
alongside stopReferences — commit c00a305 consolidated the sibling function
there for the same reason. I'd rather land the correctness fix now and do that
consolidation once, covering this and #1353 together, than block a 19-line bug
fix on it. If you want to take that follow-up PR, I'd welcome it.

Merging.