refactor: introduce TripsForRouteResponse type and update related tests#914
refactor: introduce TripsForRouteResponse type and update related tests#914Ahmedhossamdev wants to merge 3 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdded a new exported generic response type alias ChangesTyped Response Introduction & Test Refactoring
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Performance Smoke Test ResultsStatus: PASSED
Smoke test config: 5 VUs x 30s. Thresholds: p(95) < 300ms, error rate < 1%. Full results uploaded as workflow artifact: k6-smoke-summary. |
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 `@internal/restapi/trips_for_route_handler_test.go`:
- Around line 55-58: The test currently returns after asserting resp.StatusCode
for non-OK cases without checking the response body code; update the non-OK
branch to also assert that the parsed response model's Code equals
tt.expectStatus (i.e., add assert.Equal(t, tt.expectStatus, model.Code) before
the return) so the transport status and body status cannot diverge — locate this
in the test where resp, tt.expectStatus and model (model.Code) are available and
add the assertion immediately prior to returning.
🪄 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: baa16094-57d2-4b4a-9a03-c322042b4655
📒 Files selected for processing (2)
internal/restapi/response_types.gointernal/restapi/trips_for_route_handler_test.go
Performance Smoke Test ResultsStatus: PASSED
Smoke test config: 5 VUs x 30s. Thresholds: p(95) < 300ms, error rate < 1%. Full results uploaded as workflow artifact: k6-smoke-summary. |
baf9dae to
aed7914
Compare
|
Performance Smoke Test ResultsStatus: PASSED
Smoke test config: 5 VUs x 30s. Thresholds: p(95) < 300ms, error rate < 1%. Full results uploaded as workflow artifact: k6-smoke-summary. |
burma-shave
left a comment
There was a problem hiding this comment.
Consider removing dead types in the models package
Now that restapi.TripsForRouteResponse (the new alias) is in place, two types in internal/models/trips_for_route.go appear to be unused and can be cleaned up:
| Type | Can remove? | Reason |
|---|---|---|
models.TripsForRouteResponse |
✅ Yes | Shadowed by the new restapi.TripsForRouteResponse alias; no non-test callers remain |
models.TripsForRouteData |
✅ Yes | Only referenced by the dead struct above |
models.TripsForRouteListEntry |
❌ No | Still the element type for ListResponse[models.TripsForRouteListEntry] throughout the handler and tests |
There's also a subtle naming collision: models.TripsForRouteResponse (old struct) and restapi.TripsForRouteResponse (new alias) coexist in different packages, which compiles fine but can cause confusion. Removing the models one would resolve that ambiguity.



Summary by CodeRabbit
Tests
Refactor