fix: add null safeguard and source-asymmetry test for buildVehiclePopupData - #511
Conversation
|
Coverage is 81.05% — VishalRaut2106:fix/vehicleUtils-null-safeguard-508 into OneBusAway:develop. No base build found for |
Code reviewFound 1 issue:
wayfinder/src/lib/vehicleUtils.js Lines 96 to 100 in 0697fd1 🤖 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 two tests here are genuinely good and are the part of #508 I most wanted. The asymmetry test is constructed correctly — giving vehicle and activeTrip the same field names with different values is the only way that source-swap regression gets caught deliberately rather than by luck, and putting the wrong value on the other object is the right way to write it. The activeTrip: undefined test would really throw a TypeError without the optional chaining, so it's a true regression guard rather than a passing-by-construction test.
What needs another pass is the guard itself — not because it's wrong, but because it leaves the file contradicting itself.
OpenStreetMapProvider.svelte.js currently carries this, three lines above getVehicleLabel:
// activeTrip is always truthy here: the sole caller (vehicleUtils.js) guards on
// it, and buildVehiclePopupData reads activeTrip.tripHeadsign without optional
// chaining. Keep this contract consistent rather than implying null is expected.That comment is accurate on develop — I confirmed the guard it refers to at vehicleUtils.js, where applyRouteVehicles does if (activeTrip && activeTrip.routeId === routeId && ...) before ever calling addVehicleMarker or updateVehicleMarker. After this PR, the middle clause is simply false, and the last sentence is arguing against the change sitting one file over.
There's a functional wrinkle underneath the comment, too. In addVehicleMarker, getVehicleLabel(activeTrip) runs at line 461 and reads activeTrip.tripHeadsign unguarded; buildVehiclePopupData isn't reached until line 478. Same ordering in the update path (547 before 557). So if the contract ever did break, the crash just moves seventeen lines earlier and the optional chaining never gets a chance to help.
So one of these two, your call:
- Keep the guard — then update that comment block to say the precondition is enforced by the caller and the chaining is belt-and-braces, and give
getVehicleLabelthe same treatment so the file is internally consistent. - Drop the guard and take the other branch #508 offered — document the precondition on
buildVehiclePopupDatainstead. The caller already guarantees it, so this is defensible.
I'd lean toward (1). Either way, keep both tests exactly as they are — under option 2 the undefined test becomes a documentation-of-precondition test and should assert the throw instead.
Small fix, and the testing instinct here is right. Send it back and I'll take another look.
0697fd1 to
ebc4768
Compare
|
Warning Review limit reached
Next review available in: 52 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe change adds null-safe ChangesVehicle popup safety
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![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 `@src/lib/__tests__/vehicleUtils.test.js`:
- Around line 76-98: Update the test fixture in buildVehiclePopupData’s
asymmetry case so activeTrip also provides conflicting lastUpdateTime and
predicted values, while retaining the vehicle values asserted in the expected
result. Ensure all three overlapping fields—vehicleId, lastUpdateTime, and
predicted—verify that data is sourced from vehicle.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 0f7ff56c-0f0f-4151-b3ac-aa575609a2cf
📒 Files selected for processing (3)
src/lib/Provider/OpenStreetMapProvider.svelte.jssrc/lib/__tests__/vehicleUtils.test.jssrc/lib/vehicleUtils.js
…com/VishalRaut2106/wayfinder into fix/vehicleUtils-null-safeguard-508 # Conflicts: # src/lib/__tests__/vehicleUtils.test.js
|
hey @aaronbrethorst , I have implemented the requested changes kindly review and let me know if any changes required |
Code reviewFound 1 issue:
wayfinder/src/lib/__tests__/vehicleUtils.test.js Lines 615 to 617 in c04edfe 🤖 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.
You took option 1 and you took it correctly. The contract comment above getVehicleLabel now says the right thing — precondition enforced by the caller, chaining is belt-and-braces — and getVehicleLabel got the same guard, which closes the real wrinkle I raised: it runs at line 461 (and 547 on the update path) before buildVehiclePopupData, so without that change the crash just moved seventeen lines earlier and the new optional chaining never got a chance to help. I re-checked both call paths at your head commit and activeTrip isn't dereferenced anywhere else in either one, so the window is genuinely closed. vehicleUtils.js:64 still guards with if (activeTrip && ...), so the comment's claim is accurate.
Both tests survive intact, and extending the asymmetry test to also conflict on lastUpdateTime and predicted made it strictly stronger. These are real assertions — revert the ?. and the undefined test throws; swap any source in the implementation and the asymmetry test fails.
One mechanical thing to fix before this lands.
The branch mangles seven em dashes into mojibake in vehicleUtils.test.js.
Every — (UTF-8 E2 80 94) on the lines this PR touched came back as ΓÇö (CE 93 C3 87 C3 B6) — the classic CP437 misreading of a UTF-8 em dash. That's an editor or terminal encoding setting somewhere in your toolchain, not something you typed. I hexdumped the raw file at c04edfe to be sure, and confirmed develop has seven clean em dashes and zero mojibake while your head has the inverse.
Lines 127, 128, 612, 616, 652, 679, 779. Line 616 is the one that actually bites:
describe('fetchAndUpdateVehiclesForRoutes ΓÇö live highlight', () => {That's a string literal, not a comment, so the corruption shows up in test runner output rather than sitting quietly in the source. Prettier and ESLint don't normalize text inside comments or string literals, which is why CI is green on this.
Restoring those seven lines to their develop bytes is the whole fix — nothing else in the diff needs to change. Worth checking your editor's file encoding is set to UTF-8 so it doesn't recur.
The substance here is done and done well. Push the encoding fix and I'll merge it.
|
Hey @VishalRaut2106 , Let me know if you're still working on this PR :) |
Hey Tarun, I was working on this but I’d like you to take a look and handle fixing it. Once you’re done, please let me know how you solved it — I’d like to understand the approach. |
4 participants
This PR addresses the two follow-up suggestions from the maintainer regarding
buildVehiclePopupDataedge cases.Changes
activeTrip?.tripHeadsign) insidebuildVehiclePopupDataso that ifactiveTripMap.get(...)ever returnsundefined, the code gracefully returnsundefinedinstead of throwing a fatal TypeError.vehicleUtils.test.jswherevehicleandactiveTriphave identically-named properties with different values. This explicitly proves that the helper is pulling data from the correct source object.undefinedforactiveTripto guarantee the optional chaining correctly prevents crashes.Closes #508
Summary by CodeRabbit
Bug Fixes
Tests