Search stop numbers across the full region (#1432) - #1437
aaronbrethorst merged 1 commit into
Conversation
The circular stops-for-location path capped radius at 15 km, so same-code stops at distant agencies never reached disambiguation (OneBusAway#1432).
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (5)
Included review availability: Your plan provides up to 2 included reviews per hour; 1 remains after this review. 📝 WalkthroughWalkthroughStop-number searches now use the full service rectangle instead of a circular 15 km radius. The API service and URL builder support region-based stop lookup. Tests verify span-based requests and duplicated stop-number coverage. ChangesStop-number search
Priority: ➖ Normal Estimated code review effort: 2 (Simple) | ~15 minutes Change: Bug fix · Severity of issue fixed: Medium 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 |
Code reviewNo issues found. Checked for bugs and CLAUDE.md compliance. 🤖 Generated with Claude Code |
aaronbrethorst
left a comment
There was a problem hiding this comment.
This is a well-diagnosed fix. The root cause is exactly right — the circular overload clamps radius to 15 km, so a same-code stop at a distant agency in the same region could never reach the disambiguation sheet — and switching to latSpan/lonSpan over the region's service rect is the correct answer rather than just raising the cap.
I checked the parts that matter: both result consumers already branch on results.count > 1 (MapRegionManager.searchResponse and SearchSheetViewModel.SearchOutcome), so the fix lands on the UIKit and SwiftUI paths alike, and the new SearchManagerTests case asserts radius == nil alongside a non-nil latSpan, which means it genuinely fails against the old code instead of just documenting the new. The inline comments pointing at #1432 are the right amount of explanation.
One small thing if you're back in this file: getStops(region:query:) is a byte-for-byte copy of getStops(region:) plus one parameter, and a single method with query: String? = nil would collapse the duplication in both the builder and RESTAPIService+Get. The new doc comment also drops the - important: subset/zoom-in caveat its two siblings carry, which arguably matters more now that the span can cover an entire region.
Merging.
Summary
getStops(circularRegion:query:), which capsradiusat 15 km, so remote same-code stops never appeared (Android / span queries return both).getStops(region:query:)withlatSpan/lonSpanover the region'sserviceRect. Existing disambiguation UI already handlesresults.count > 1.Test plan
NetworkHelperTests— span+query URL has noradius=SearchManagerTests— stop-number search sendslatSpan/lonSpan+query1000→ disambiguation list with both agencies (not auto-open the downtown hit)Summary by CodeRabbit
Bug Fixes
Tests