fix: consider guaranteed interchange and max wait time when determining interchange possibility - #477
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
The current interchangeHolds logic can incorrectly suppress overlap detection for non-transit legs (e.g. foot legs), which may hide real timing inconsistencies.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Updates trip-status evaluation so that “impossible” is not triggered by timing overlaps at guaranteed interchanges, aligning backend behavior with planner-web and improving correctness of impossible-trip messaging.
Changes:
- Extend
hasTemporalOverlapto ignore overlaps when the relevant interchange is guaranteed (and still valid withinmaximumWaitTimewhen present). - Add helper functions to locate the prior transit leg and evaluate whether a guaranteed interchange still holds.
- Add unit tests covering guaranteed/non-guaranteed interchanges,
maximumWaitTime, and walk legs between transit legs; also coverdetermineTripStatus.
File summaries
| File | Description |
|---|---|
src/service/impl/trips/utils.ts |
Adjusts temporal-overlap detection to account for guaranteed interchanges (with max-wait handling). |
src/service/impl/trips/__tests__/utils.test.ts |
Adds test coverage for guaranteed interchange cases and trip-status behavior. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| function interchangeHolds(legs: Leg[], index: number): boolean { | ||
| const interchange = previousTransitLeg(legs, index)?.interchangeTo; | ||
| if (interchange?.guaranteed !== true) return false; | ||
| if (interchange.maximumWaitTime == null) return true; |
gorandalum
left a comment
gorandalum
left a comment
There was a problem hiding this comment.
The PR title should be more aligned with the changes, to make it easier to know if the right thing is implemented.
The title make it seem that guaranteed interchanges should never make trips impossible. But when looking at the code, it seems that if delay is greater than maxWaitTime, then the trip is impossible even if guaranteed interchange?
reidzeibel
changed the title
|
@gorandalum changed the title and description |
|
✅ |
Similar to planner-web fix here, guaranteed interchange will now be taken into account when determining interchange possibility. Non-guaranteed interchanges will show the message as usual.