Fix short-line route schedules - #597
Conversation
Use per-trip stop headsigns so riders can identify trips that terminate early. Fixes OneBusAway#251
|
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 (9)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe schedule API now enriches multi-trip directions with cached trip headsigns. Shared grouping logic marks short-line trips. The route schedule table displays short-line notices, destinations, and highlighted arrival times. ChangesShort-line schedule handling
Priority: ➖ Normal — Impact reflects medium issue severity. Estimated code review effort: 4 (Complex) | ~45 minutes Severity of issue fixed: Medium Merge Risk: ⚪ Minimal · up to The route schedule changes include coverage for headsign enrichment, cache behavior, regional date handling, and short-line rendering, with no remaining concrete merge risk identified. Sequence Diagram(s)sequenceDiagram
participant SchedulePage
participant ScheduleAPI
participant tripHeadsigns
participant OneBusAway
SchedulePage->>ScheduleAPI: request stop schedule
ScheduleAPI->>OneBusAway: retrieve stop schedule
ScheduleAPI->>tripHeadsigns: load route headsigns
tripHeadsigns->>OneBusAway: retrieve route schedule
OneBusAway-->>tripHeadsigns: return trip headsigns
tripHeadsigns-->>ScheduleAPI: return cached mappings
ScheduleAPI-->>SchedulePage: return enriched schedule
SchedulePage->>SchedulePage: group and mark short-line trips
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 62.50% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 8 functions across 6 files. (3 skipped: 3 unsupported.)
✨ 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 |
|
Coverage is 93.622% — soumajitgh:fix/short-line-schedules into OneBusAway:develop. No base build found for |
|
@coderabbitai full review |
|
Code reviewFound 1 issue:
🤖 Generated with Claude Code - If this code review was useful, please react with 👍. Otherwise, react with 👎. |
aaronbrethorst
left a comment
aaronbrethorst
left a comment
There was a problem hiding this comment.
The supporting pieces of this are good. The response?.data guard on the schedule endpoint is a real fix, the retryable error state is a genuine improvement over a silently blank panel, and converting the per-hour cells from <td> to <th scope="row"> is the correct table semantics. The case-insensitive extractMinutes regex is a nice catch.
But the headline feature doesn't work against real OBA data, and I verified that rather than reasoning about it.
isShortLine can never be true
The detection is:
const destination = stopTime.stopHeadsign?.trim() || directionHeadsign;
isShortLine: destination !== directionHeadsignThat requires scheduleStopTimes[].stopHeadsign to be populated. It isn't. I pulled the exact stop from #251 — MTS_12434 on the San Diego server:
MTS_3 | tripHeadsign='UCSD Med Ctr/Hillcrest' | 77 stop times | stopHeadsign: {'': 77}
MTS_120 | tripHeadsign='Kearny Mesa' | 37 stop times | stopHeadsign: {'': 37}
references.trips: 0
Every one of the 114 stop times has stopHeadsign: "". So destination always falls back to directionHeadsign, the comparison is always false, hasShortLines is always false, and the entire amber UI — banner, chips, the short_line* strings — is unreachable in production. Merging this would close #251 without fixing anything a rider can see.
The mechanism behind #251 is the other way around: OBA collapses the variants into the direction label. On Puget Sound you can watch it happen — one direction comes back as tripHeadsign: "Northgate Station Roosevelt Station", two destinations concatenated. And references.trips is empty on this endpoint, so the per-trip headsign isn't available here at all. A real fix needs a different data source — probably trip-details per trip, or stops-for-route — which is a bigger design question worth settling before writing the UI.
The code comment asserting "A stop-specific headsign is supplied for trips that take a different path or terminate early" says the opposite of what the API returns, so that's worth correcting whichever direction this goes.
The tests can't catch that
RouteScheduleTable.test.js hand-injects isShortLine: true and destination as props, so groupStopTimesByHour — where the actual detection lives — has zero coverage. That's the same point Copilot raised on your earlier attempt at this in #441, and it's why the suite is green on a feature that never activates.
A couple of the other new assertions can't fail either:
expect(screen.queryByText(/05 am/i)).not.toBeInTheDocument()passes without the/ifix too — unfixed output is"05am"with no space, so the pattern misses either way. (ThegetByTitle('Full Time: 8:05')assertions in that test are real.)expect(screen.queryByText('Short line to Kearny Mesa')).not.toBeInTheDocument()can never match, since non-short-line chips render no destination text at all.- In
schedule-for-stop.test.js,expect(response).toBeNull()is asserting on the mock —handleOBAResponseis stubbed to echo its argument. It does still prove noTypeErrorescapes, which is the point of the guard, so that one earns partial credit.
On scope
The AM/PM redesign is a separate change riding along. It removes the section rowgroups and replaces the no_am_schedules_available / no_pm_schedules_available empty states for every route, short-line or not, and it leaves those two strings dead in en.json and the other 24 locale files. Same for the error/retry state — useful, unrelated.
I closed #441 asking for smaller PRs and I'll ask again here: the endpoint guard and the error/retry state would sail through on their own, and I'd merge that today. The short-line feature needs the data question answered first.
Happy to talk through where the real headsign data should come from if that's useful — it's the interesting part of this problem and I don't want the work you've already done to go to waste.
|
|
||
| // The direction headsign describes the route's usual destination. A stop-specific | ||
| // headsign is supplied for trips that take a different path or terminate early. | ||
| const destination = stopTime.stopHeadsign?.trim() || directionHeadsign; |
There was a problem hiding this comment.
stopHeadsign is empty on every stop time this endpoint returns, so this always falls through to directionHeadsign.
Verified against the stop named in #251:
GET /api/where/schedule-for-stop/MTS_12434.json
MTS_3 | tripHeadsign='UCSD Med Ctr/Hillcrest' | 77 stop times | stopHeadsign: {'': 77}
MTS_120 | tripHeadsign='Kearny Mesa' | 37 stop times | stopHeadsign: {'': 37}
114/114 empty. references.trips is also 0 on this endpoint, so the per-trip headsign isn't reachable from here either.
| arrivalTime: msToTimeString(stopTime.arrivalTime) | ||
| arrivalTime: msToTimeString(stopTime.arrivalTime), | ||
| destination, | ||
| isShortLine: destination !== directionHeadsign |
There was a problem hiding this comment.
Given the line above, this comparison is always false — which makes hasShortLines always false and the whole amber banner/chip UI dead code in production.
The comment two lines up ("A stop-specific headsign is supplied for trips that take a different path or terminate early") describes behavior the API doesn't have. What OBA actually does is merge the variants into the direction label — Puget Sound returns tripHeadsign: "Northgate Station Roosevelt Station" for one direction, which is the real shape of #251.
|
Addressed in commit 24304b4.
The full Vitest suite passes: 105 files, 1,799 tests. |
aaronbrethorst
left a comment
aaronbrethorst
left a comment
There was a problem hiding this comment.
The cleanup here is real and I want to acknowledge it before the bad news. Scope is tight now — the AM/PM redesign, the retry state, and the endpoint guard are all out, and the PR is just short lines. The false comment about stopHeadsign is gone and replaced with an accurate one. groupStopTimesByHour moved to src/lib/scheduleForStop.js and finally has direct unit coverage, which was the gap I cared most about. And the two bogus test assertions are fixed properly.
More importantly: you picked the right data source. schedule-for-route really does carry the per-trip headsigns that schedule-for-stop collapses away. The design is correct.
The field path isn't, and it's the same failure mode as last time in a new location.
data.entry.trips doesn't exist
addTripHeadsigns reads routeResponse?.data?.entry?.trips. I called the endpoint against Puget Sound to be sure:
GET /api/where/schedule-for-route/1_100259.json
data.entry keys : [ 'routeId', 'scheduleDate', 'serviceIds', 'stopTripGroupings' ]
data.entry.trips : undefined
data.references keys : [ 'agencies', 'routes', 'situations', 'stopTimes', 'stops', 'trips' ]
data.references.trips : 130
The trips are in data.references.trips. So the tripHeadsigns Map is always empty, no stopTime.tripHeadsign is ever set, destination always falls back to directionHeadsign, and isShortLine is always false. The amber banner and the chips are still unreachable in production — exactly the state I rejected last time.
I know where this came from and it isn't carelessness: the SDK's own generated type at node_modules/onebusaway-sdk/resources/schedule-for-route.d.ts declares entry.trips (and entry.stops). The generated type is wrong about the real API. Worth knowing generally — check OBA response shapes against a live call rather than the .d.ts.
The good news: you're one line from a working feature
I ran the corrected logic against live data. Trip-ID resolution was 100%, and it finds genuine short lines:
| stop | direction label | per-trip headsigns | flagged |
|---|---|---|---|
| 1_41255 (rt 4) | Judkins Park Downtown Seattle |
40x same, 3x Downtown Seattle |
3 |
| 1_18085 (rt 44) | UW Medical Center Wallingford |
69x same, 9x University District |
9 |
| 1_56151 (rt 36) | Downtown Seattle N Beacon Hill |
101x same, 6x Intl Dist Chinatown Station N Beacon Hill |
6 |
So references.trips makes this work. Two things have to come with that change:
The new API test will block the fix. src/tests/api/schedule-for-stop.test.js mocks scheduleForRoute.retrieve as { data: { entry: { trips: [...] } } } — a response the API never produces. That's worse than no coverage: correct the field path and this test starts failing. Please rewrite the mock to the real shape (data.references.trips) so it fails on the current code and passes on the fix.
The payload cost needs an answer. One schedule-for-route per route at the stop, measured live:
schedule-for-route 1_100259 : 1.2 MB
schedule-for-route 1_100219 : 1.3 MB
schedule-for-route 1_102615 : 2.5 MB
schedule-for-stop 1_23230 : 45 KB
Stop 1_23230 serves 4 routes, so that's roughly 5 MB fetched to enrich a 45 KB response, on every page load and every date change, uncached. Promise.all parallelizes the latency but not the bytes, and the > 1 distinct tripId guard prunes almost nothing — every direction I sampled had 38 to 107 distinct trips. I don't need it solved perfectly, but I do need a plan: caching by (routeId, scheduleDate) would probably do it, since the data is static for a service day.
Two smaller things
addTripHeadsignswill throw and 500 the whole endpoint if anystopRouteSchedulesentry is missingstopRouteDirectionSchedulesorscheduleStopTimes. Ondevelopthese were only touched client-side, where a malformed entry degraded one route instead of the request.RouteScheduleTable.test.jsdrops theexpect(amHeader.tagName).toBe('TH')/expect(pmHeader.tagName).toBe('TH')assertions that exist ondevelop. The component still renders<th scope="rowgroup">, so that's pure coverage loss — looks like leftover from reverting the AM/PM work. Please put them back.
Also: you removed the response?.data guard and the retry state rather than splitting them out. Those were the two pieces I said I'd merge today, so they'll need their own PR now — worth doing, they were both good.
Change the field path, fix the mock, tell me how you want to handle the payload size, and this lands.
|
Addressed in 988c702: corrected the lookup to Added a per-process cache keyed by route/date (24-hour TTL, 100-entry limit), storing only trip headsigns and sharing concurrent lookups. Failed lookups are retried; cold requests still fetch the full route schedule. Validation: all 1,809 tests and coverage checks pass; lint and production build pass. |
Code reviewFound 2 issues:
wayfinder/src/routes/api/oba/schedule-for-stop/[stopId]/+server.js Lines 21 to 25 in 988c702
wayfinder/src/tests/api/schedule-for-stop.test.js Lines 25 to 33 in 988c702 🤖 Generated with Claude Code - If this code review was useful, please react with 👍. Otherwise, react with 👎. |
Resolve an omitted date before fetching the stop schedule and reuse it for route lookups and their cache keys. Stop responses expose entry.date, which is wall-clock time when undated, not a stable service-day key. Correct the fixture envelope and cover cache reuse, region midnight, DST, and requests whose upstream calls span midnight. Refs OneBusAway#597
3 participants
Summary
schedule-for-routereferences.PUBLIC_OBA_TIMEZONEand send the same explicit date to both OBA endpoints and the cache.Design
Testing
npm run lintandnpm run buildpassed.entry.datetimestamps, region midnight, DST transitions, and upstream requests crossing midnight. The corrected tests fail against the previous implementation.Fixes #251
Summary by CodeRabbit
New Features
Bug Fixes