Conversation
📝 WalkthroughWalkthroughThis PR implements a complete pitch management system for trips, adding database schema, backend CRUD operations with S3 integration, HTTP endpoints, and frontend React Query hooks for create, read, update, delete, and list operations. Concurrently, it consolidates and refactors existing API hook implementations across comments, memberships, files, trips, and users with streamlined type signatures. Changes
Sequence DiagramsequenceDiagram
participant Client as Frontend Client
participant Controller as PitchController
participant Service as PitchService
participant Presign as S3PresignClient
participant Repo as PitchRepository
participant DB as Database
participant S3 as S3
Client->>Controller: POST /trips/:tripID/pitches (CreatePitchRequest)
Controller->>Service: Create(tripID, userID, request)
Service->>Repo: FindByID (check membership)
Repo->>DB: Query membership
DB-->>Repo: membership exists
Service->>Service: Generate pitch ID & audio key
Service->>Repo: Create(TripPitch)
Repo->>DB: INSERT trip_pitches
DB-->>Repo: Created pitch
Service->>Presign: PresignPUT(s3Key, contentType)
Presign->>S3: Generate presigned URL
S3-->>Presign: Upload URL (15 min expiry)
Presign-->>Service: Upload URL
Service-->>Controller: CreatePitchResponse (uploadURL, pitch, expiresAt)
Controller-->>Client: 201 Created
Client->>Controller: GET /trips/:tripID/pitches (with cursor, limit)
Controller->>Service: List(tripID, limit, cursor)
Service->>Repo: FindByTripIDWithCursor(tripID, limit, cursor)
Repo->>DB: SELECT with cursor ordering
DB-->>Repo: [Pitches], nextCursor
loop For each pitch
Service->>Presign: PresignGET(audioKey)
Presign->>S3: Generate presigned URL
S3-->>Presign: Download URL
Presign-->>Service: Download URL
Service->>Service: mapToPitchAPIResponse(pitch, downloadURL)
end
Service-->>Controller: PitchCursorPageResult
Controller-->>Client: 200 OK
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~45 minutes The PR introduces a new domain feature spanning database, backend services, and frontend integration with S3 presigning complexity. While individual patterns are familiar, the breadth requires verification across repository cursor pagination, presigned URL generation and expiration, membership authorization checks, and frontend React Query hook consistency. The frontend refactoring across multiple API modules, though largely mechanical formatting, adds volume and requires spot-checking type signature changes. Possibly Related Issues
Possibly Related PRs
Suggested Reviewers
🚥 Pre-merge checks | ✅ 1 | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts (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 |
There was a problem hiding this comment.
Actionable comments posted: 16
🤖 Fix all issues with AI agents
In `@backend/docs/swagger.json`:
- Around line 1252-1374: The Swagger spec for the createPitch endpoint is
documenting a 403 Forbidden but tests and controller behavior return 404 for
non-members; update the API documentation to match runtime behavior by changing
the controller's Swagger annotation (the createPitch endpoint's `@Failure` 403
entry) to `@Failure` 404 and update the generated swagger.json responses for
operationId "createPitch" (POST /api/v1/trips/{tripID}/pitches) to remove or
replace the 403 response with a 404 response (ensure the 404 uses errs.APIError
or the appropriate schema), and scan other pitch endpoints for similar
mismatches between `@Failure` annotations and actual status codes.
In `@backend/internal/controllers/pitches.go`:
- Around line 89-109: ListPitches (and the other controllers GetByID, Update,
Delete) never extract the caller userID from c.Locals("userID") nor pass it to
the service, so membership checks (like membershipRepo.IsMember used by Create)
cannot run; modify each controller (ListPitches, GetByID, Update, Delete) to
read userID := c.Locals("userID") (validate/convert as needed), pass that userID
into the corresponding pitchService methods (e.g., pitchService.List now accepts
userID), and update the service implementations to call
membershipRepo.IsMember(userID, tripID) before performing the operation,
returning a proper authorization error when not a member.
In `@backend/internal/migrations/20260205120000_create_trip_pitches.sql`:
- Around line 35-43: The down migration unconditionally drops the shared
function update_updated_at_column(), which may be used by other tables; instead,
make the function table-specific (e.g., update_trip_pitches_updated_at) and
update both the CREATE FUNCTION and CREATE TRIGGER statements in this migration
to use that name, then change the Down block to DROP TRIGGER IF EXISTS
update_trip_pitches_updated_at ON trip_pitches; and DROP FUNCTION IF EXISTS
update_trip_pitches_updated_at(); so only the per-table trigger function is
removed; ensure any references in this file to update_updated_at_column() are
replaced with the new per-table function name.
- Around line 22-28: The migration defines a shared function
update_updated_at_column() used by multiple migrations which causes
cross-migration breakage; rename the function to a table-specific name (e.g.,
update_trip_pitches_updated_at_column()) and update the trigger definition(s) in
this migration to call the new function name, and change the down migration to
DROP FUNCTION IF EXISTS update_trip_pitches_updated_at_column(); so this
migration creates, uses, and drops its own uniquely named function without
affecting other tables' triggers.
In `@backend/internal/models/pitches.go`:
- Around line 23-27: The ContentType field on CreatePitchRequest is currently
unrestricted and must be limited to an allowlist of audio MIME types; update
validation for CreatePitchRequest.ContentType to only accept known audio MIME
types (e.g., audio/mpeg, audio/mp3, audio/wav, audio/x-wav, audio/ogg,
audio/webm, audio/aac, audio/flac) either by replacing the struct tag with a
validate:"required,oneof=..." list or by adding a dedicated validator (register
a custom "audio" validator) that checks against a central audioMIMETypes
allowlist constant and then use validate:"required,audio" on ContentType; ensure
the validator is registered where request validation is wired so
CreatePitchRequest is rejected if ContentType is not in the allowlist (reference
CreatePitchRequest.ContentType and extensionFromContentType() for context).
In `@backend/internal/repository/pitches.go`:
- Around line 76-109: The Update method can build an invalid SQL when no fields
on UpdatePitchRequest are set; add a guard at the start of
pitchRepository.Update to detect when req.Title, req.Description and
req.Duration are all nil and return early with a clear error (e.g.,
errs.ErrBadRequest or a new validation error) instead of proceeding to
r.db.NewUpdate/ upd.Exec; ensure the guard references Update,
UpdatePitchRequest, the upd variable (r.db.NewUpdate()) and returns before
attempting to execute the update so no empty SET clause is produced, then keep
the rest of the method as-is to fetch the TripPitch on success.
In `@backend/internal/server/routers/trips.go`:
- Around line 41-46: The UpdatePitch and DeletePitch handlers currently only
require TripMemberRequired and therefore allow any member to change any pitch;
mirror the CreatePitch pattern by extracting the authenticated userID in the
UpdatePitch and DeletePitch controller handlers and pass it into the pitch
service (e.g., call service.UpdatePitch(ctx, userID, tripID, pitchID, ... ) and
service.DeletePitch(ctx, userID, tripID, pitchID) or similar). In the service
(or repository) add an ownership check that loads the pitch owner (e.g.,
repo.GetPitchOwner or repo.GetPitchByID) and compares ownerID == userID,
returning a 403/permission error if they differ; only proceed to update/delete
when the user is the owner. Ensure errors are propagated back to the controller
to return the correct HTTP status.
In `@backend/internal/services/pitches.go`:
- Around line 61-104: The Create handler currently inserts the DB record via
s.pitchRepo.Create and only then calls s.presignClient.PresignPutObject, which
leaves an orphan pitch if presign fails; change the flow in Create to generate
the presigned upload URL first (call s.presignClient.PresignPutObject to get
presigned.URL and expires), then construct the models.TripPitch and call
s.pitchRepo.Create (or alternatively, if you prefer to keep the insert-first
pattern, call s.pitchRepo.Create and on any presign error call a delete/rollback
via s.pitchRepo.Delete to remove the created pitch); update code paths that
build apiPitch via pitchToAPIResponse to use the created pitch and return
UploadURL/ExpiresAt from the successful presign so no pitch exists without a
usable upload URL.
- Around line 143-151: The List loop is calling s.presignGetURL sequentially for
each pitch (items, s.presignGetURL, pitchToAPIResponse), causing latency at
scale; fix by not presigning in the list path—remove/omit audio_url from the
list response and change the list handler to call pitchToAPIResponse without
presigned URLs (or have pitchToAPIResponse accept a nil/empty audioURL), and
move presigning into the GetByID path where s.presignGetURL is invoked on
demand; alternatively, if audio_url must remain in List, parallelize presign
calls with a bounded worker pool (e.g., spawn N goroutines, use a jobs/results
channel and context cancellation) so s.presignGetURL calls run concurrently but
limited to a safe concurrency level.
- Around line 46-58: NewPitchService currently doesn't validate required config
fields which leads to nil-pointer panics; modify NewPitchService to validate
PitchServiceConfig.PresignClient, .PitchRepo, .MembershipRepo and .BucketName
(non-nil and non-empty for BucketName) and return a clear error for
missing/invalid values instead of constructing a service with nil
dependencies—this will require changing the constructor signature to return
(PitchServiceInterface, error) and returning a descriptive error for each
missing dependency (referencing NewPitchService, PitchServiceConfig, and the
PitchService struct/presignClient, pitchRepo, membershipRepo, bucketName
fields).
- Around line 184-186: The Delete method (PitchService.Delete) currently only
calls s.pitchRepo.Delete and leaves the S3 audio object orphaned; change it to
first load the pitch (e.g., via s.pitchRepo.Get or similar) to obtain the
AudioS3Key, then call the standard S3 client's DeleteObject for that key before
deleting the DB record (or delete DB record only after successful S3 deletion),
handling and returning errors appropriately; if the service only has a presign
client, add a standard S3 client to PitchServiceConfig and use it here, and
ensure you handle missing AudioS3Key gracefully to avoid panics.
- Around line 214-229: The function extensionFromContentType currently returns
"m4a" for any unrecognized MIME type; change its signature to return (string,
error) and make it explicitly validate against an allowlist of known audio MIME
types (e.g., "audio/m4a", "audio/mp4", "audio/mpeg", "audio/mp3", "audio/wav",
"audio/ogg", "audio/webm"); for matches return the proper extension, otherwise
return a non-nil error indicating unsupported content type. Update all callers
of extensionFromContentType to handle the error (propagate or reject the upload)
so invalid types like "application/pdf" no longer silently become .m4a. Ensure
error messages include the original contentType value for easier diagnosis.
- Around line 107-118: GetByID, List, Update, and Delete currently lack a userID
parameter and thus skip authorization: update the service method signatures for
GetByID, List, Update, and Delete to accept a userID (same type used by Create),
have the controller extract and pass the userID, and add authorization checks
inside each service method (for GetByID/List call trip membership verification
before calling pitchRepo.FindByIDAndTripID or the list repo method; for
Update/Delete verify that the user is either the pitch owner or a trip admin
before proceeding to modify/delete; for Update ensure ownership/admin check
precedes any update logic and for GetByID include the membership check before
presignGetURL and pitchToAPIResponse). Ensure existing helper methods such as
presignGetURL and pitchToAPIResponse are used only after authorization succeeds.
In `@backend/internal/tests/pitch_test.go`:
- Around line 304-340: TestPitchDelete is missing a case that verifies
non-members are rejected; add a subtest under TestPitchDelete that creates a
separate non-member user (e.g., nonMemberID via createTestUser), reuses
createPitch to create pitchID, then issues a DELETE request to
"/api/v1/trips/"+tripID+"/pitches/"+pitchID using testkit.Request with UserID
set to &nonMemberID and AssertStatus(http.StatusNotFound); this mirrors the
Create test pattern and ensures the delete endpoint is scoped to trip members.
- Around line 148-200: In TestPitchList's "returns pitches with audio_url"
subtest remove the redundant requireS3 call: the helper createPitch(t, app,
userID, tripID, "P1", "audio/mpeg") already invokes requireS3 internally, so
delete the extra requireS3(t) invocation to avoid reloading S3 configuration
unnecessarily and keep the rest of the subtest (the subsequent testkit request
and assertions) unchanged.
In `@frontend/api/trips/useGetAllTrips.ts`:
- Line 24: The request helper is dropping query params so pagination params
passed from useGetAllTrips (and other generated hooks) are never sent; add an
optional params property to the RequestConfig type (e.g., params?: Record<string
| number, any> | URLSearchParams) and update the request function to serialize
those params onto the fetch URL (e.g., append new
URLSearchParams(config.params).toString() to `${BASE_URL}${config.url}` only
when params exist), ensuring callers like useGetAllTrips.ts that pass params are
transmitted to the server.
| "/api/v1/trips/{tripID}/pitches": { | ||
| "get": { | ||
| "description": "Returns pitches for the trip with cursor-based pagination", | ||
| "produces": [ | ||
| "application/json" | ||
| ], | ||
| "tags": [ | ||
| "pitches" | ||
| ], | ||
| "summary": "Get all pitches for a trip", | ||
| "operationId": "listPitches", | ||
| "parameters": [ | ||
| { | ||
| "type": "string", | ||
| "description": "Trip ID", | ||
| "name": "tripID", | ||
| "in": "path", | ||
| "required": true | ||
| }, | ||
| { | ||
| "type": "integer", | ||
| "description": "Max items per page (default 20, max 100)", | ||
| "name": "limit", | ||
| "in": "query" | ||
| }, | ||
| { | ||
| "type": "string", | ||
| "description": "Opaque cursor from previous response next_cursor", | ||
| "name": "cursor", | ||
| "in": "query" | ||
| } | ||
| ], | ||
| "responses": { | ||
| "200": { | ||
| "description": "OK", | ||
| "schema": { | ||
| "$ref": "#/definitions/models.PitchCursorPageResult" | ||
| } | ||
| }, | ||
| "400": { | ||
| "description": "Bad Request", | ||
| "schema": { | ||
| "$ref": "#/definitions/errs.APIError" | ||
| } | ||
| }, | ||
| "404": { | ||
| "description": "Not Found", | ||
| "schema": { | ||
| "$ref": "#/definitions/errs.APIError" | ||
| } | ||
| }, | ||
| "500": { | ||
| "description": "Internal Server Error", | ||
| "schema": { | ||
| "$ref": "#/definitions/errs.APIError" | ||
| } | ||
| } | ||
| } | ||
| }, | ||
| "post": { | ||
| "description": "Creates a new pitch for the trip and returns a presigned URL to upload the audio file", | ||
| "consumes": [ | ||
| "application/json" | ||
| ], | ||
| "produces": [ | ||
| "application/json" | ||
| ], | ||
| "tags": [ | ||
| "pitches" | ||
| ], | ||
| "summary": "Create a pitch", | ||
| "operationId": "createPitch", | ||
| "parameters": [ | ||
| { | ||
| "type": "string", | ||
| "description": "Trip ID", | ||
| "name": "tripID", | ||
| "in": "path", | ||
| "required": true | ||
| }, | ||
| { | ||
| "description": "Create pitch request", | ||
| "name": "request", | ||
| "in": "body", | ||
| "required": true, | ||
| "schema": { | ||
| "$ref": "#/definitions/models.CreatePitchRequest" | ||
| } | ||
| } | ||
| ], | ||
| "responses": { | ||
| "201": { | ||
| "description": "Created", | ||
| "schema": { | ||
| "$ref": "#/definitions/models.CreatePitchResponse" | ||
| } | ||
| }, | ||
| "400": { | ||
| "description": "Bad Request", | ||
| "schema": { | ||
| "$ref": "#/definitions/errs.APIError" | ||
| } | ||
| }, | ||
| "403": { | ||
| "description": "Forbidden", | ||
| "schema": { | ||
| "$ref": "#/definitions/errs.APIError" | ||
| } | ||
| }, | ||
| "422": { | ||
| "description": "Unprocessable Entity", | ||
| "schema": { | ||
| "$ref": "#/definitions/errs.APIError" | ||
| } | ||
| }, | ||
| "500": { | ||
| "description": "Internal Server Error", | ||
| "schema": { | ||
| "$ref": "#/definitions/errs.APIError" | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Swagger documents 403 for createPitch, but the test expects 404 for non-members.
The createPitch endpoint Swagger spec lists a 403 Forbidden response (line 1355) but does not list 404. However, the test TestPitchCreate / "non-member gets 404" asserts http.StatusNotFound. The Swagger annotation in the controller (line 38: @Failure 403) should be updated to match the actual behavior.
This same issue may apply to other pitch endpoints if non-member access returns 404 rather than being documented.
🤖 Prompt for AI Agents
In `@backend/docs/swagger.json` around lines 1252 - 1374, The Swagger spec for the
createPitch endpoint is documenting a 403 Forbidden but tests and controller
behavior return 404 for non-members; update the API documentation to match
runtime behavior by changing the controller's Swagger annotation (the
createPitch endpoint's `@Failure` 403 entry) to `@Failure` 404 and update the
generated swagger.json responses for operationId "createPitch" (POST
/api/v1/trips/{tripID}/pitches) to remove or replace the 403 response with a 404
response (ensure the 404 uses errs.APIError or the appropriate schema), and scan
other pitch endpoints for similar mismatches between `@Failure` annotations and
actual status codes.
There was a problem hiding this comment.
@aamogh16 let's add the error code in the doc just to be sure!
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| func (ctrl *PitchController) ListPitches(c *fiber.Ctx) error { | ||
| tripID, err := validators.ValidateID(c.Params("tripID")) | ||
| if err != nil { | ||
| return errs.InvalidUUID() | ||
| } | ||
|
|
||
| var params models.GetPitchesQueryParams | ||
| if err := utilities.ParseAndValidateQueryParams(c, ctrl.validator, ¶ms); err != nil { | ||
| return err | ||
| } | ||
| limit, cursorToken := utilities.ExtractLimitAndCursor(¶ms) | ||
|
|
||
| result, err := ctrl.pitchService.List(c.Context(), tripID, limit, cursorToken) | ||
| if err != nil { | ||
| if errors.Is(err, errs.ErrInvalidCursor) { | ||
| return errs.BadRequest(err) | ||
| } | ||
| return err | ||
| } | ||
| return c.Status(http.StatusOK).JSON(result) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# 1. Find pitch route registration and middleware
echo "=== Pitch Route Registration ==="
rg -n "pitches" backend/internal/server/routers/ -A 3 -B 3
# 2. Check PitchService method signatures
echo -e "\n=== PitchService Method Signatures ==="
rg -n "func \(.* PitchService\) (List|GetByID|Update|Delete|Create)" backend/internal/services/pitches.go
# 3. Look for complete service method signatures with parameters
echo -e "\n=== PitchService Complete Methods ==="
cat backend/internal/services/pitches.go | head -200Repository: GenerateNU/toggo
Length of output: 7597
🏁 Script executed:
#!/bin/bash
# Get the full path of pitches service to understand its structure
find backend -name "pitches.go" -type f | xargs wc -lRepository: GenerateNU/toggo
Length of output: 246
🏁 Script executed:
#!/bin/bash
# Read the PitchService to check method signatures and authorization logic
cat -n backend/internal/services/pitches.goRepository: GenerateNU/toggo
Length of output: 8943
🏁 Script executed:
#!/bin/bash
# Check the controller file to confirm what the review comment says
cat -n backend/internal/controllers/pitches.go | head -250Repository: GenerateNU/toggo
Length of output: 8296
GetByID, List, Update, and Delete do not accept userID, preventing membership authorization.
The Create method accepts userID and enforces membership checks via membershipRepo.IsMember(). The four other service methods (GetByID, List, Update, Delete) do not accept userID and therefore cannot verify that the caller is a trip member. The controller layers for these operations do not extract userID from c.Locals("userID"), so no authorization is enforced before calling the service.
Pass userID through the controller and service layers for GetByID, List, Update, and Delete, and add membership checks to each method (similar to Create).
🤖 Prompt for AI Agents
In `@backend/internal/controllers/pitches.go` around lines 89 - 109, ListPitches
(and the other controllers GetByID, Update, Delete) never extract the caller
userID from c.Locals("userID") nor pass it to the service, so membership checks
(like membershipRepo.IsMember used by Create) cannot run; modify each controller
(ListPitches, GetByID, Update, Delete) to read userID := c.Locals("userID")
(validate/convert as needed), pass that userID into the corresponding
pitchService methods (e.g., pitchService.List now accepts userID), and update
the service implementations to call membershipRepo.IsMember(userID, tripID)
before performing the operation, returning a proper authorization error when not
a member.
| CREATE OR REPLACE FUNCTION update_updated_at_column() | ||
| RETURNS TRIGGER AS $$ | ||
| BEGIN | ||
| NEW.updated_at = now(); | ||
| RETURN NEW; | ||
| END; | ||
| $$ language 'plpgsql'; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n 'update_updated_at_column' --type=sqlRepository: GenerateNU/toggo
Length of output: 1049
🏁 Script executed:
find . -name '20260205120000_create_trip_pitches.sql' -type f -exec cat -n {} \;Repository: GenerateNU/toggo
Length of output: 1851
Shared function name update_updated_at_column() creates breaking dependency across migrations.
This function is created identically in three separate migrations: 20260123213356_create_trip.sql, 20260123213543_create_membership.sql, and 20260205120000_create_trip_pitches.sql. Each migration creates its own trigger on its respective table, but all share the same function name.
The down migration (line 38) drops the function unconditionally with DROP FUNCTION IF EXISTS update_updated_at_column(). Rolling back any one of these migrations will break the triggers on all other tables that depend on the same function.
Rename the function to be table-specific, for example update_trip_pitches_updated_at_column(), so each migration manages its own function lifecycle independently.
🤖 Prompt for AI Agents
In `@backend/internal/migrations/20260205120000_create_trip_pitches.sql` around
lines 22 - 28, The migration defines a shared function
update_updated_at_column() used by multiple migrations which causes
cross-migration breakage; rename the function to a table-specific name (e.g.,
update_trip_pitches_updated_at_column()) and update the trigger definition(s) in
this migration to call the new function name, and change the down migration to
DROP FUNCTION IF EXISTS update_trip_pitches_updated_at_column(); so this
migration creates, uses, and drops its own uniquely named function without
affecting other tables' triggers.
| -- +goose Down | ||
| -- +goose StatementBegin | ||
| DROP TRIGGER IF EXISTS update_trip_pitches_updated_at ON trip_pitches; | ||
| DROP FUNCTION IF EXISTS update_updated_at_column(); | ||
| DROP INDEX IF EXISTS idx_trip_pitches_trip_created_id; | ||
| DROP INDEX IF EXISTS idx_trip_pitches_user_id; | ||
| DROP INDEX IF EXISTS idx_trip_pitches_trip_id; | ||
| DROP TABLE IF EXISTS trip_pitches; | ||
| -- +goose StatementEnd |
There was a problem hiding this comment.
Down migration drops the shared function unconditionally.
Line 38 (DROP FUNCTION IF EXISTS update_updated_at_column()) will remove the function even if other tables' triggers still reference it. This is the downstream consequence of the shared function name flagged above. If you rename the function per-table, this becomes safe.
🤖 Prompt for AI Agents
In `@backend/internal/migrations/20260205120000_create_trip_pitches.sql` around
lines 35 - 43, The down migration unconditionally drops the shared function
update_updated_at_column(), which may be used by other tables; instead, make the
function table-specific (e.g., update_trip_pitches_updated_at) and update both
the CREATE FUNCTION and CREATE TRIGGER statements in this migration to use that
name, then change the Down block to DROP TRIGGER IF EXISTS
update_trip_pitches_updated_at ON trip_pitches; and DROP FUNCTION IF EXISTS
update_trip_pitches_updated_at(); so only the per-table trigger function is
removed; ensure any references in this file to update_updated_at_column() are
replaced with the new per-table function name.
| func extensionFromContentType(contentType string) string { | ||
| contentType = strings.TrimSpace(strings.ToLower(contentType)) | ||
| switch { | ||
| case strings.HasSuffix(contentType, "m4a"), contentType == "audio/mp4": | ||
| return "m4a" | ||
| case strings.HasSuffix(contentType, "mpeg"), strings.HasSuffix(contentType, "mp3"): | ||
| return "mp3" | ||
| case strings.HasSuffix(contentType, "wav"): | ||
| return "wav" | ||
| case strings.HasSuffix(contentType, "ogg"): | ||
| return "ogg" | ||
| case strings.HasSuffix(contentType, "webm"): | ||
| return "webm" | ||
| default: | ||
| return "m4a" | ||
| } |
There was a problem hiding this comment.
extensionFromContentType silently defaults to "m4a" for unrecognized MIME types.
If an unexpected content type like application/pdf is passed, it will silently produce a .m4a extension. This could lead to files with incorrect extensions stored in S3.
Consider returning an error for unsupported content types, or validate the content type against an allowlist before reaching this function.
Proposed fix
-func extensionFromContentType(contentType string) string {
+func extensionFromContentType(contentType string) (string, error) {
contentType = strings.TrimSpace(strings.ToLower(contentType))
switch {
case strings.HasSuffix(contentType, "m4a"), contentType == "audio/mp4":
- return "m4a"
+ return "m4a", nil
case strings.HasSuffix(contentType, "mpeg"), strings.HasSuffix(contentType, "mp3"):
- return "mp3"
+ return "mp3", nil
case strings.HasSuffix(contentType, "wav"):
- return "wav"
+ return "wav", nil
case strings.HasSuffix(contentType, "ogg"):
- return "ogg"
+ return "ogg", nil
case strings.HasSuffix(contentType, "webm"):
- return "webm"
+ return "webm", nil
default:
- return "m4a"
+ return "", fmt.Errorf("unsupported content type: %s", contentType)
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func extensionFromContentType(contentType string) string { | |
| contentType = strings.TrimSpace(strings.ToLower(contentType)) | |
| switch { | |
| case strings.HasSuffix(contentType, "m4a"), contentType == "audio/mp4": | |
| return "m4a" | |
| case strings.HasSuffix(contentType, "mpeg"), strings.HasSuffix(contentType, "mp3"): | |
| return "mp3" | |
| case strings.HasSuffix(contentType, "wav"): | |
| return "wav" | |
| case strings.HasSuffix(contentType, "ogg"): | |
| return "ogg" | |
| case strings.HasSuffix(contentType, "webm"): | |
| return "webm" | |
| default: | |
| return "m4a" | |
| } | |
| func extensionFromContentType(contentType string) (string, error) { | |
| contentType = strings.TrimSpace(strings.ToLower(contentType)) | |
| switch { | |
| case strings.HasSuffix(contentType, "m4a"), contentType == "audio/mp4": | |
| return "m4a", nil | |
| case strings.HasSuffix(contentType, "mpeg"), strings.HasSuffix(contentType, "mp3"): | |
| return "mp3", nil | |
| case strings.HasSuffix(contentType, "wav"): | |
| return "wav", nil | |
| case strings.HasSuffix(contentType, "ogg"): | |
| return "ogg", nil | |
| case strings.HasSuffix(contentType, "webm"): | |
| return "webm", nil | |
| default: | |
| return "", fmt.Errorf("unsupported content type: %s", contentType) | |
| } | |
| } |
🤖 Prompt for AI Agents
In `@backend/internal/services/pitches.go` around lines 214 - 229, The function
extensionFromContentType currently returns "m4a" for any unrecognized MIME type;
change its signature to return (string, error) and make it explicitly validate
against an allowlist of known audio MIME types (e.g., "audio/m4a", "audio/mp4",
"audio/mpeg", "audio/mp3", "audio/wav", "audio/ogg", "audio/webm"); for matches
return the proper extension, otherwise return a non-nil error indicating
unsupported content type. Update all callers of extensionFromContentType to
handle the error (propagate or reject the upload) so invalid types like
"application/pdf" no longer silently become .m4a. Ensure error messages include
the original contentType value for easier diagnosis.
| func TestPitchList(t *testing.T) { | ||
| app := fakes.GetSharedTestApp() | ||
| userID := createTestUser(t, app, "ListUser", fakes.GenerateRandomUsername(), fakes.GenerateRandomPhoneNumber()) | ||
| tripID := createTestTrip(t, app, userID, "ListTrip", 100, 500) | ||
|
|
||
| t.Run("returns empty list", func(t *testing.T) { | ||
| resp := testkit.New(t). | ||
| Request(testkit.Request{ | ||
| App: app, | ||
| Route: "/api/v1/trips/" + tripID + "/pitches", | ||
| Method: testkit.GET, | ||
| UserID: &userID, | ||
| }). | ||
| AssertStatus(http.StatusOK). | ||
| AssertField("limit", float64(20)). | ||
| AssertFieldExists("items"). | ||
| GetBody() | ||
| items, ok := resp["items"].([]any) | ||
| require.True(t, ok) | ||
| assert.Len(t, items, 0) | ||
| }) | ||
|
|
||
| t.Run("returns pitches with audio_url when S3 configured", func(t *testing.T) { | ||
| _, _ = createPitch(t, app, userID, tripID, "P1", "audio/mpeg") | ||
| requireS3(t) | ||
| resp := testkit.New(t). | ||
| Request(testkit.Request{ | ||
| App: app, | ||
| Route: "/api/v1/trips/" + tripID + "/pitches", | ||
| Method: testkit.GET, | ||
| UserID: &userID, | ||
| }). | ||
| AssertStatus(http.StatusOK). | ||
| GetBody() | ||
| items, ok := resp["items"].([]any) | ||
| require.True(t, ok) | ||
| require.GreaterOrEqual(t, len(items), 1) | ||
| first := items[0].(map[string]any) | ||
| assert.Equal(t, "P1", first["title"]) | ||
| assert.Contains(t, first, "audio_url") | ||
| }) | ||
|
|
||
| t.Run("invalid trip ID returns 400", func(t *testing.T) { | ||
| testkit.New(t). | ||
| Request(testkit.Request{ | ||
| App: app, | ||
| Route: "/api/v1/trips/bad-uuid/pitches", | ||
| Method: testkit.GET, | ||
| UserID: &userID, | ||
| }). | ||
| AssertStatus(http.StatusBadRequest) | ||
| }) | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Redundant requireS3 call in "returns pitches with audio_url" subtest.
createPitch on line 171 already calls requireS3(t) internally. The second call on line 172 is redundant. Not a bug, but it loads the full configuration a second time for no reason.
Proposed fix
t.Run("returns pitches with audio_url when S3 configured", func(t *testing.T) {
_, _ = createPitch(t, app, userID, tripID, "P1", "audio/mpeg")
- requireS3(t)
resp := testkit.New(t).📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func TestPitchList(t *testing.T) { | |
| app := fakes.GetSharedTestApp() | |
| userID := createTestUser(t, app, "ListUser", fakes.GenerateRandomUsername(), fakes.GenerateRandomPhoneNumber()) | |
| tripID := createTestTrip(t, app, userID, "ListTrip", 100, 500) | |
| t.Run("returns empty list", func(t *testing.T) { | |
| resp := testkit.New(t). | |
| Request(testkit.Request{ | |
| App: app, | |
| Route: "/api/v1/trips/" + tripID + "/pitches", | |
| Method: testkit.GET, | |
| UserID: &userID, | |
| }). | |
| AssertStatus(http.StatusOK). | |
| AssertField("limit", float64(20)). | |
| AssertFieldExists("items"). | |
| GetBody() | |
| items, ok := resp["items"].([]any) | |
| require.True(t, ok) | |
| assert.Len(t, items, 0) | |
| }) | |
| t.Run("returns pitches with audio_url when S3 configured", func(t *testing.T) { | |
| _, _ = createPitch(t, app, userID, tripID, "P1", "audio/mpeg") | |
| requireS3(t) | |
| resp := testkit.New(t). | |
| Request(testkit.Request{ | |
| App: app, | |
| Route: "/api/v1/trips/" + tripID + "/pitches", | |
| Method: testkit.GET, | |
| UserID: &userID, | |
| }). | |
| AssertStatus(http.StatusOK). | |
| GetBody() | |
| items, ok := resp["items"].([]any) | |
| require.True(t, ok) | |
| require.GreaterOrEqual(t, len(items), 1) | |
| first := items[0].(map[string]any) | |
| assert.Equal(t, "P1", first["title"]) | |
| assert.Contains(t, first, "audio_url") | |
| }) | |
| t.Run("invalid trip ID returns 400", func(t *testing.T) { | |
| testkit.New(t). | |
| Request(testkit.Request{ | |
| App: app, | |
| Route: "/api/v1/trips/bad-uuid/pitches", | |
| Method: testkit.GET, | |
| UserID: &userID, | |
| }). | |
| AssertStatus(http.StatusBadRequest) | |
| }) | |
| } | |
| func TestPitchList(t *testing.T) { | |
| app := fakes.GetSharedTestApp() | |
| userID := createTestUser(t, app, "ListUser", fakes.GenerateRandomUsername(), fakes.GenerateRandomPhoneNumber()) | |
| tripID := createTestTrip(t, app, userID, "ListTrip", 100, 500) | |
| t.Run("returns empty list", func(t *testing.T) { | |
| resp := testkit.New(t). | |
| Request(testkit.Request{ | |
| App: app, | |
| Route: "/api/v1/trips/" + tripID + "/pitches", | |
| Method: testkit.GET, | |
| UserID: &userID, | |
| }). | |
| AssertStatus(http.StatusOK). | |
| AssertField("limit", float64(20)). | |
| AssertFieldExists("items"). | |
| GetBody() | |
| items, ok := resp["items"].([]any) | |
| require.True(t, ok) | |
| assert.Len(t, items, 0) | |
| }) | |
| t.Run("returns pitches with audio_url when S3 configured", func(t *testing.T) { | |
| _, _ = createPitch(t, app, userID, tripID, "P1", "audio/mpeg") | |
| resp := testkit.New(t). | |
| Request(testkit.Request{ | |
| App: app, | |
| Route: "/api/v1/trips/" + tripID + "/pitches", | |
| Method: testkit.GET, | |
| UserID: &userID, | |
| }). | |
| AssertStatus(http.StatusOK). | |
| GetBody() | |
| items, ok := resp["items"].([]any) | |
| require.True(t, ok) | |
| require.GreaterOrEqual(t, len(items), 1) | |
| first := items[0].(map[string]any) | |
| assert.Equal(t, "P1", first["title"]) | |
| assert.Contains(t, first, "audio_url") | |
| }) | |
| t.Run("invalid trip ID returns 400", func(t *testing.T) { | |
| testkit.New(t). | |
| Request(testkit.Request{ | |
| App: app, | |
| Route: "/api/v1/trips/bad-uuid/pitches", | |
| Method: testkit.GET, | |
| UserID: &userID, | |
| }). | |
| AssertStatus(http.StatusBadRequest) | |
| }) | |
| } |
🤖 Prompt for AI Agents
In `@backend/internal/tests/pitch_test.go` around lines 148 - 200, In
TestPitchList's "returns pitches with audio_url" subtest remove the redundant
requireS3 call: the helper createPitch(t, app, userID, tripID, "P1",
"audio/mpeg") already invokes requireS3 internally, so delete the extra
requireS3(t) invocation to avoid reloading S3 configuration unnecessarily and
keep the rest of the subtest (the subsequent testkit request and assertions)
unchanged.
| export async function getAllTrips(params?: GetAllTripsQueryParams, config: Partial<RequestConfig> & { client?: typeof fetch } = {}) { | ||
| const { client: request = fetch, ...requestConfig } = config | ||
|
|
||
| const res = await request<GetAllTripsQueryResponse, ResponseErrorConfig<GetAllTrips400 | GetAllTrips401 | GetAllTrips500>, unknown>({ method : "GET", url : `/api/v1/trips`, params, ... requestConfig }) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, let's find and examine the frontend/api/client.ts file
echo "=== Looking for frontend/api/client.ts ==="
fd "client.ts" frontend/api/
echo ""
echo "=== RequestConfig type definition ==="
cat -n frontend/api/client.ts | head -20
echo ""
echo "=== request function signature and params handling ==="
cat -n frontend/api/client.ts | sed -n '64,99p'Repository: GenerateNU/toggo
Length of output: 2324
🏁 Script executed:
#!/bin/bash
# Check generated files that pass params to request
echo "=== Files in frontend/api that use params ==="
fd -t f "\\.ts$" frontend/api/ --exec grep -l "params" {} \;
echo ""
echo "=== Check useGetAllTrips.ts to see params being passed ==="
cat -n frontend/api/trips/useGetAllTrips.ts | head -30Repository: GenerateNU/toggo
Length of output: 2973
🏁 Script executed:
#!/bin/bash
# Search for how params are handled in the request function
echo "=== Full request function implementation ==="
rg "function request|const request" -A 50 --type=ts frontend/api/client.ts | head -80Repository: GenerateNU/toggo
Length of output: 1227
Query parameters are silently dropped — pagination will not work.
The params field is spread into the request config on line 24, but RequestConfig in frontend/api/client.ts (lines 6-12) does not define a params property. The request function (lines 65-99) constructs the fetch URL as ${BASE_URL}${config.url} without serializing query parameters. This means limit, cursor, and any other query parameters passed to getAllTrips() are ignored.
The function documentation explicitly states "Use limit and cursor query params", but they are never sent to the server. This affects 22+ generated files across the codebase (useGetTrip.ts, useGetPaginatedComments.ts, useGetUser.ts, etc.).
Add a params field to RequestConfig and serialize it into the URL in the request function. Alternatively, configure Kubb to construct the full URL with query parameters in the generated code.
🤖 Prompt for AI Agents
In `@frontend/api/trips/useGetAllTrips.ts` at line 24, The request helper is
dropping query params so pagination params passed from useGetAllTrips (and other
generated hooks) are never sent; add an optional params property to the
RequestConfig type (e.g., params?: Record<string | number, any> |
URLSearchParams) and update the request function to serialize those params onto
the fetch URL (e.g., append new URLSearchParams(config.params).toString() to
`${BASE_URL}${config.url}` only when params exist), ensuring callers like
useGetAllTrips.ts that pass params are transmitted to the server.
| tripIDGroup.Delete("", tripController.DeleteTrip) | ||
|
|
||
| // /api/v1/trips/:tripID/pitches | ||
| tripIDGroup.Post("/pitches", pitchController.CreatePitch) |
There was a problem hiding this comment.
can we ensure that membership is checked for all of these routes so that people who are not members cannot get a trip's pitches. i believe there is a middleware for this so we can pass the middleware in here
in-mai-space
left a comment
There was a problem hiding this comment.
some nits on checking membership of the trip, but overall looking good 🔥
can you confirm if the audio recorded get played if you play it from the presigned url?
| AudioS3Key string `bun:"audio_s3_key,notnull" json:"audio_s3_key"` | ||
| Duration *int `bun:"duration" json:"duration,omitempty"` | ||
| CreatedAt time.Time `bun:"created_at,nullzero" json:"created_at"` | ||
| UpdatedAt time.Time `bun:"updated_at,nullzero" json:"updated_at"` |
There was a problem hiding this comment.
This is a change that's new since Thursday, but any chance we can add a new migration for ImageURLs?
At a minimum, I'm imagining that we'll automatically populate some kind of image based on the destination (so null?), but design seems to be leaning to having the choice of uploading your own images as well. You can keep this as a nullable list of strings, we can do the s3 upload logic later.
Alternatively, if you feel this is a hefty change, we can make this a separate PR :)
Description
How has this been tested?
Checklist