feat: add admin trip detail endpoint and user_id trip filter

[diveshpatil9104]

Summary

Adds the two pieces of admin trip functionality that main still lacks after 6ba0338 landed GET /api/v1/admin/trips: a trip detail endpoint, and a user_id filter on the trips list.

This PR originally added a full list+get implementation. That was superseded mid-review, so it has been rescoped down to the remaining delta — from 977 lines to 238.

Endpoints

Method	Path	Description
GET	/api/v1/admin/trips/{id}	Single trip summary, without the location trail
GET	/api/v1/admin/trips?user_id=	Existing list endpoint, now filterable by driver

Both are behind authMiddleware(adminMiddleware(...)) and covered by the route_wiring_test.go driver-rejected / admin-allowed tables.

Why these two

GET /api/v1/admin/trips/{id} — main only has /{id}/locations, which joins the trip summary to its full trail and is capped at 10k points. A caller that just wants trip metadata (status, driver, vehicle, times) shouldn't pay for that. This is the same handler shape minus the trail, reusing the existing GetTripSummary, so it adds no SQL, no store method, and no new interface — appStore already embeds TripSummaryGetter.

user_id filter — lets an admin review one driver's history. main's list filters on status, vehicle, and a text query, but not driver.

A present user_id must be >= 1. 0 and negatives are rejected with a 400 rather than falling through to the 0 = "all drivers" sentinel, which would silently return every trip instead of the caller's filter — the zero-value bug this avoids is covered by a test.

What changed

Modified (4):

• store_trips.go — UserID field on TripFilter, one condition in ListTrips
• admin_live_handlers.go — user_id parsing/validation in handleListTrips; new handleGetTrip
• main.go — one route registration
• route_wiring_test.go — new route in both admin wiring tables

Tests (2):

• admin_live_handlers_test.go — 9 tests: filter passthrough, absent/invalid user_id, and the detail endpoint's happy path, omitted end_time, non-numeric id, not-found, nil-without-error guard, and store error (asserting the underlying error isn't leaked)
• store_trips_test.go — TestListTripsUserIDFilter: narrows to one driver, composes with status, returns empty for a driver with no trips, and treats 0 as no filter

Notes on the review feedback

has_more via limit+1 and the ORDER BY t.start_time DESC, t.id DESC tiebreaker both already exist in main's implementation. The user_id filter composes with the existing conditions and inherits both, so neither needed changing.

Verified with go fmt, go vet, go mod tidy, and the full suite run against a live Postgres so the DB integration tests execute rather than skip.

[aaronbrethorst]

Code review

No issues found. Checked for bugs and CLAUDE.md compliance.

🤖 Generated with Claude Code

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Team

Run ID: 8d2c5dff-4b66-4cb7-9e3e-f2ff8b7d1e02

📥 Commits

Reviewing files that changed from the base of the PR and between 3f237cf and 8ca5e30.

📒 Files selected for processing (6)

• admin_live_handlers.go
• admin_live_handlers_test.go
• main.go
• route_wiring_test.go
• store_trips.go
• store_trips_test.go

Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review.

---

📝 Walkthrough

Walkthrough

The change adds optional driver filtering to admin trip listings and adds an admin-only endpoint for retrieving a trip summary by ID. Handlers validate query and path parameters, map store errors to HTTP responses, and include route authorization tests.

Changes

Admin trip API

Layer / File(s)	Summary
Trip filtering and store validation store_trips.go, store_trips_test.go	TripFilter supports UserID, and ListTrips applies the driver filter when it is non-zero. Store tests cover filtering, summaries, location windows, ordering, and test data cleanup.
Admin listing and detail handlers admin_live_handlers.go, admin_live_handlers_test.go	handleListTrips validates user_id. handleGetTrip returns trip summaries, omits inactive-only fields when needed, and maps invalid, missing, nil, and store-error cases to HTTP responses.
Admin route authorization main.go, route_wiring_test.go	The mux registers GET /api/v1/admin/trips/{id}. Middleware tests verify admin access and driver-token rejection.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Merge Risk: ⚪ Minimal · up to 8ca5e

This change adds admin-only trip retrieval and optional driver filtering for trip listings. No concrete merge-blocking risk remains.

Sequence Diagram(s)

sequenceDiagram
  participant AdminClient
  participant AdminRoute
  participant handleGetTrip
  participant TripSummaryGetter
  AdminClient->>AdminRoute: GET /api/v1/admin/trips/{id}
  AdminRoute->>handleGetTrip: pass path id
  handleGetTrip->>TripSummaryGetter: request trip summary
  TripSummaryGetter-->>handleGetTrip: return summary or store error
  handleGetTrip-->>AdminClient: return JSON, 404, or 500

Loading

Suggested reviewers: aaronbrethorst

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 63.33% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 30 functions across 8 files.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly describes the two main changes: the admin trip detail endpoint and the user_id trip filter.

• Fix all pre-merge checks with AI

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

🧹 Nitpick comments (1)

db/query.sql (1)

107-115: 🚀 Performance & Scalability | 🔵 Trivial

Pagination and filter performance risk on a growing trips table. ListTripsFiltered uses an IS NULL OR column = param pattern for optional filters and orders by start_time DESC, while the HTTP layer allows an unbounded offset. Together these can force full or large partial scans as the table grows.

• db/query.sql#L107-L115: verify indexes exist to support the status/vehicle_id/user_id predicates and the start_time DESC sort; add composite indexes if missing.
• trip_list_handlers.go#L58-L67: cap offset to a reasonable maximum, or plan a move to keyset (cursor-based) pagination once trip volume grows.

🤖 Prompt for 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.

In `@db/query.sql` around lines 107 - 115, Improve pagination performance across
db/query.sql lines 107-115 and trip_list_handlers.go lines 58-67: add or verify
indexes supporting ListTripsFiltered’s status, vehicle_id, user_id filters and
start_time DESC ordering, and cap the HTTP offset to a reasonable maximum while
preserving existing filtering and limit behavior.

🤖 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.

Nitpick comments:
In `@db/query.sql`:
- Around line 107-115: Improve pagination performance across db/query.sql lines
107-115 and trip_list_handlers.go lines 58-67: add or verify indexes supporting
ListTripsFiltered’s status, vehicle_id, user_id filters and start_time DESC
ordering, and cap the HTTP offset to a reasonable maximum while preserving
existing filtering and limit behavior.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 163cad48-8fd8-4a3d-9d1e-510463ec8e59

📥 Commits

Reviewing files that changed from the base of the PR and between 56c0d03 and 3f237cf.

📒 Files selected for processing (12)

• db/query.sql
• db/query.sql.go
• main.go
• route_wiring_test.go
• store_trips.go
• store_trips_test.go
• trip_handlers.go
• trip_handlers_test.go
• trip_list_handlers.go
• trip_list_handlers_test.go
• trip_list_store.go
• trip_list_store_test.go

[aaronbrethorst]

Code review

Found 1 issue:

1. This feature already exists on main — the route GET /api/v1/admin/trips, the handler handleListTrips, and the TripLister interface all landed in commit 6ba0338 (main.go:73, admin_live_handlers.go:144, store_trips.go:83). This is more than the reported textual merge conflict: trip_list_handlers.go and trip_list_store.go are new files that git will merge cleanly, so even after the conflicts in main.go/store_trips.go are resolved the package will not compile — func handleListTrips, type TripLister, and noopStore.ListTrips are all redeclared — and registering the same mux pattern twice panics http.ServeMux at startup. The version on main is also the more complete one (joins driver/vehicle labels, q search, has_more, and a deterministic ORDER BY t.start_time DESC, t.id DESC). This needs a rebase onto current main and a rescope to whatever it still adds on top — the GET /api/v1/admin/trips/{id} detail endpoint and the user_id filter.

vehicle-positions/main.go

Lines 83 to 87 in 1406ba8

	// Admin trip listing
	mux.Handle("GET /api/v1/admin/trips", authMiddleware(adminMiddleware(handleListTrips(store))))
	mux.Handle("GET /api/v1/admin/trips/{id}", authMiddleware(adminMiddleware(handleGetTrip(store))))

🤖 Generated with Claude Code

_{- If this code review was useful, please react with 👍. Otherwise, react with 👎.}

[aaronbrethorst]

Thank you for this, and apologies that it sat long enough for main to move underneath it. The code itself holds up well: both routes are correctly gated behind authMiddleware(adminMiddleware(...)) and covered by the route_wiring_test.go driver-rejected/admin-allowed tables, all SQL goes through sqlc so there is no injection surface, limit is capped, there is no N+1, and the ~28 tests are real tests. None of what follows is a criticism of the work.

The list half of this PR has been superseded. 6ba0338 landed GET /api/v1/admin/trips on main on 2026-08-24, eleven days after this branch's last commit, with a somewhat richer implementation (driver and vehicle label joins, a q search parameter, has_more, and a deterministic start_time DESC, id DESC ordering).

The important part: this is not only the merge conflict GitHub is showing you. The conflicting hunks are just main.go, db/query.sql.go and store_trips.go — but trip_list_handlers.go and trip_list_store.go are new files that git merges cleanly. So even after resolving every conflict textually, package main ends up with handleListTrips, TripLister, and (*Store).ListTrips each declared twice, which will not compile, plus a duplicate mux.Handle pattern that http.ServeMux panics on at startup. Resolving the conflicts is not enough here.

What is still genuinely new and worth keeping:

• GET /api/v1/admin/trips/{id} — main only has /{id}/locations, there is no plain detail endpoint.
• The user_id filter, which main's version does not have.

Could you rebase onto main and rescope this to just those two? Drop trip_list_store.go/trip_list_handlers.go in favor of extending the existing handleListTrips and TripLister in admin_live_handlers.go/store_trips.go. That should be a much smaller PR than this one and I would be glad to land it.

Two notes for the rescoped version, both matching patterns already in the repo: fetch limit+1 and report has_more rather than returning count: len(trips) (see location_history_handlers.go and admin_live_handlers.go), and give the ORDER BY a unique tiebreaker so LIMIT/OFFSET paging cannot skip or duplicate rows.

[diveshpatil9104]

Thanks for the detailed review — and for spelling out that the duplicate declarations wouldn't surface as merge conflicts. That saved me from "resolving" this into something that doesn't compile.

Rebased onto main and rescoped as you asked. trip_list_handlers.go and trip_list_store.go are gone; the list half is dropped in favor of the one that landed in 6ba0338.

What's left is the two things main doesn't have:

• GET /api/v1/admin/trips/{id} — the trail-free counterpart to /{id}/locations, so callers that only want trip metadata don't pay for up to 10k location points. It reuses the existing GetTripSummary, so it adds no SQL, no store method, and no new interface — appStore already embedded TripSummaryGetter. 404 on both non-numeric and unknown ids, matching handleTripLocations.
• The user_id filter — one field on TripFilter and one WHERE clause in ListTrips. A present user_id must be >= 1: 0 and negatives are rejected with a 400 rather than collapsing into the "all drivers" sentinel and quietly returning every trip.

On your two notes — both already come from your implementation, so I left them alone rather than reimplementing them: handleListTrips already fetches limit+1 and reports has_more, and ListTrips already orders by t.start_time DESC, t.id DESC. The user_id filter composes with the existing conditions and inherits both.

The PR is now +238 lines (184 of them tests) against the original 977. Tests: 9 handler tests covering the filter passthrough, the rejected user_id values, and the detail endpoint's 200/404/500 paths, plus TestListTripsUserIDFilter for the store. I ran the suite against a live Postgres rather than letting the DB tests skip, so the integration test is actually exercised.

Both new routes are in the route_wiring_test.go driver-rejected and admin-allowed tables.