feat(backend): complete six template gaps for production readiness#33
Merged
Conversation
- Wire Redis Streams end-to-end: Producer initialised in bootstrap, consumer goroutine in server fans UserCreatedEvent → Asynq welcome email task - Fix welcome email handler to actually call Mailjet via NewHandleWelcomeEmail constructor (was only logging before) - Add user profile CRUD: migration adds firebase_uid/email/photo_url to users, PATCH /api/v1/me upserts profile, DELETE /api/v1/me removes DB record - Make WebSocket bi-directional: ReadPump forwards inbound Envelopes to hub, Hub.OnMessage registers typed handlers dispatched per message type - Add generic pagination types (Page[T], CursorPage[T], PageRequest) to domain - Add bindJSON/bindQuery validation helpers; refactor existing handlers to use them - Remove TestClose that closed shared testDB mid-suite, breaking later tests
📝 WalkthroughWalkthroughAdds a ChangesUser profile management and event-driven welcome email pipeline
WebSocket inbound message dispatch
Sequence Diagram(s)sequenceDiagram
participant FCMHandler
participant StreamProducer
participant RedisStream
participant ServerConsumer
participant Enqueuer
participant QueueWorker
participant EmailSender
FCMHandler->>StreamProducer: publish user created event
StreamProducer->>RedisStream: append stream message
ServerConsumer->>RedisStream: read user created message
ServerConsumer->>Enqueuer: enqueue welcome email task
Enqueuer->>QueueWorker: deliver task
QueueWorker->>EmailSender: send welcome email
sequenceDiagram
participant WSClient
participant ReadPump
participant Hub
participant InboundHandler
WSClient->>ReadPump: send websocket message
ReadPump->>ReadPump: unmarshal envelope
ReadPump->>Hub: push inbound message
Hub->>Hub: lookup handler by message type
Hub->>InboundHandler: invoke handler asynchronously
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
🚥 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (1)
backend/internal/infrastructure/queue/handlers_test.go (1)
13-23: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the actual send path.
TestHandleWelcomeEmail_ValidPayloadconstructs the handler with anilsender, so it only verifies the graceful no-op branch — thesender.SendWelcomeEmail(...)path and its argument mapping (p.Email, p.Email) are never exercised. Consider adding a test with a mockEmailSenderthat asserts it is invoked with the expected arguments.As per coding guidelines for
backend/**/*_test.go: "Use TDD approach: write failing tests first, then implement the functionality".🤖 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 `@backend/internal/infrastructure/queue/handlers_test.go` around lines 13 - 23, The current test TestHandleWelcomeEmail_ValidPayload only verifies behavior with a nil sender, so the actual email sending path through sender.SendWelcomeEmail(...) is never exercised. Add a second test that constructs the handler with a mock or test double that implements EmailSender, then verify that the mock's SendWelcomeEmail method is called exactly once with the expected arguments (p.Email for both the email and display name parameters from the WelcomeEmailPayload).Source: Coding guidelines
🤖 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 `@backend/internal/bootstrap/bootstrap.go`:
- Around line 139-146: The StreamProducer created from streams.NewProducer() in
the bootstrap code owns a Redis client with a Close() method but is never closed
during graceful shutdown, causing a connection leak. The usecase.StreamProducer
interface only exposes the Publish method, preventing callers from closing the
resource. Add a Close() error method to the usecase.StreamProducer interface to
make the close operation part of the contract, then update the concrete
implementation in streams/producer.go to implement this method if not already
present, and finally invoke streamProducer.Close() in cmd/api/main.go during the
graceful shutdown sequence alongside the existing cleanup of Cache and Enqueuer
resources.
In `@backend/internal/domain/pagination.go`:
- Around line 19-36: The Offset() method can return a negative value when Page
is 0 because the binding constraint allows min=0, but pagination pages should
start at 1. Guard the Offset() method to ensure it handles unnormalized input by
applying the same default normalization logic that Defaults() uses, ensuring
Page is treated as at least 1 when calculating the offset value, so callers get
a valid result even if they forget to call Defaults() first.
In
`@backend/internal/infrastructure/database/migrations/20260623063851_add_user_profile.sql`:
- Around line 2-8: The firebase_uid column is currently nullable but serves as
the identity key, which violates the contract expected by the new API handler.
Add NOT NULL constraint to the firebase_uid column definition in the ALTER TABLE
users statement. Additionally, the separate CREATE UNIQUE INDEX statement on
firebase_uid is redundant since the column already has a UNIQUE constraint;
remove this index creation line. Before applying the NOT NULL constraint, ensure
any existing rows in the users table are backfilled with firebase_uid values to
prevent migration failures.
In `@backend/internal/infrastructure/queue/handlers.go`:
- Line 24: The SendWelcomeEmail method call in handlers.go is passing p.Email
for both the toEmail and toName parameters, which means the welcome email will
address the recipient by their email address instead of a proper display name.
To fix this, extend the WelcomeEmailPayload struct (from tasks.go) to include a
display name field, propagate this name through the upstream UserCreatedEvent,
and then use this display name when calling SendWelcomeEmail instead of passing
p.Email twice. If a display name is not available, at minimum document and
confirm that using the email address as a fallback for the recipient's name is
the intended behavior.
In `@backend/internal/infrastructure/ws/client.go`:
- Around line 65-72: The code currently silently ignores json.Unmarshal errors
when processing inbound WebSocket frames. When jsonErr is not nil in the
json.Unmarshal call on the Envelope type, no logging or error handling occurs
and the frame is dropped without any signal. Add an else block or separate error
handling path after the json.Unmarshal of the Envelope to log the error using
slog.Warn or slog.Error when unmarshaling fails, and continue processing
subsequent messages rather than silently swallowing the error.
In `@backend/internal/infrastructure/ws/hub.go`:
- Line 77: In the hub.go file where fn is being invoked with
context.Background() on line 77, replace context.Background() with the hub's
lifecycle context. This ensures that when the hub's Run method is cancelled, the
context passed to handler fn will also be cancelled, allowing handlers to
properly terminate instead of continuing to run. Use the context that is
associated with the hub's lifecycle (typically a context that is cancelled when
the hub stops running).
- Around line 75-80: The inbound handler in the hub spawns an unbounded
goroutine for each inbound message received, which can cause memory exhaustion
under bursty traffic. Replace the unbounded `go func(msg InboundMessage)`
goroutine launch with a bounded concurrency mechanism, such as a semaphore
channel or worker pool pattern, to limit the maximum number of concurrent
inbound handler executions. Ensure the error handling and logging behavior
within the fn(context.Background(), msg) call remains intact while enforcing the
concurrency limit.
In `@backend/internal/server/server.go`:
- Around line 65-84: The Redis Streams consumer goroutine has two issues that
need fixing. First, replace context.Background() in the consumer.Run call with a
cancellable context that is tied to the server lifecycle (similar to the
workerCancel pattern used in main.go), and ensure this context is cancelled
during graceful shutdown so that the Run method returns and allows the
consumer.Close() call to execute. Second, the error returned from
streams.NewConsumer at line 67 is silently discarded via the if err == nil guard
- instead log this error so that failed consumer setup is observable to
operators, following the guideline to never swallow errors.
In `@backend/internal/transport/handlers/fcm_handler.go`:
- Around line 54-59: The error returned by the streamProducer.Publish call in
the FCM handler is being silently discarded using the blank identifier. Instead
of ignoring the error with `_`, capture the error returned by
streamProducer.Publish when publishing the UserCreatedEvent and log it using an
appropriate logger at the error level. This ensures that any failures in the
publish operation are observable and can be debugged, while still allowing the
request to proceed as a best-effort operation.
In `@backend/internal/transport/handlers/validation.go`:
- Around line 11-13: The error handling in the ShouldBindJSON validation block
is exposing internal validation details by using err.Error() directly, which
violates the error-handling principle of never exposing internal error messages
to clients. Replace the err.Error() call with a stable, client-facing message
such as "invalid request body" in the JSON response to prevent leaking internal
details like field names and struct tags while still informing the client that
their request was malformed.
---
Nitpick comments:
In `@backend/internal/infrastructure/queue/handlers_test.go`:
- Around line 13-23: The current test TestHandleWelcomeEmail_ValidPayload only
verifies behavior with a nil sender, so the actual email sending path through
sender.SendWelcomeEmail(...) is never exercised. Add a second test that
constructs the handler with a mock or test double that implements EmailSender,
then verify that the mock's SendWelcomeEmail method is called exactly once with
the expected arguments (p.Email for both the email and display name parameters
from the WelcomeEmailPayload).
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: e7bb682e-0d8f-4fd5-8a48-af1e1d9b9043
📒 Files selected for processing (29)
backend/cmd/api/main.gobackend/docs/swagger/docs.gobackend/docs/swagger/swagger.jsonbackend/docs/swagger/swagger.yamlbackend/internal/bootstrap/bootstrap.gobackend/internal/domain/pagination.gobackend/internal/domain/user.gobackend/internal/infrastructure/database/migrations/20260623063851_add_user_profile.sqlbackend/internal/infrastructure/database/postgres/health_repository_test.gobackend/internal/infrastructure/database/postgres/user_repository.gobackend/internal/infrastructure/database/postgres/user_repository_test.gobackend/internal/infrastructure/queue/handlers.gobackend/internal/infrastructure/queue/handlers_test.gobackend/internal/infrastructure/ws/client.gobackend/internal/infrastructure/ws/hub.gobackend/internal/infrastructure/ws/hub_test.gobackend/internal/infrastructure/ws/message.gobackend/internal/server/server.gobackend/internal/transport/handlers/fcm_handler.gobackend/internal/transport/handlers/handler.gobackend/internal/transport/handlers/health_handler_test.gobackend/internal/transport/handlers/me_handler.gobackend/internal/transport/handlers/me_handler_test.gobackend/internal/transport/handlers/routes.gobackend/internal/transport/handlers/storage_handler.gobackend/internal/transport/handlers/swagger_types.gobackend/internal/transport/handlers/validation.gobackend/internal/usecase/streams.gobackend/internal/usecase/user.go
💤 Files with no reviewable changes (1)
- backend/internal/infrastructure/database/postgres/health_repository_test.go
- Add Close() to StreamProducer interface; close producer on graceful shutdown - Move streams consumer goroutine from server.go to main.go with a proper cancellable context (streamCancel) so it exits cleanly on SIGINT/SIGTERM - Log consumer init failure instead of silently dropping via if err == nil guard - Add Name field to UserCreatedEvent and WelcomeEmailPayload so welcome emails address recipients by name instead of their email address - Guard Offset() against page <= 1 to prevent negative SQL OFFSET values - Make firebase_uid NOT NULL in migration; backfill legacy rows before constraint - Log publish errors in fcm_handler instead of swallowing with _ = - Bound inbound WebSocket handler concurrency with a 64-slot semaphore - Pass hub lifecycle context to inbound handlers instead of context.Background() - Log malformed inbound WebSocket frames instead of silently discarding - Use stable error messages in bindJSON/bindQuery to avoid leaking internals - Use bindJSON helper in UnregisterFCMToken (was still using inline pattern) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Managed Redis providers (Upstash, Redis Cloud) terminate idle connections after ~10 s, which surfaces as an i/o timeout on the blocking XReadGroup call. go-redis reconnects automatically on the next attempt, so these are not real errors — log them at WARN and retry immediately. For genuine unexpected errors, sleep 500 ms before retrying so the loop does not hammer Redis when it is actually unhealthy.
…o use The blocking XReadGroup poll holds a persistent Redis connection even when there is no business logic consuming events, wasting a connection slot on managed Redis plans. The consumer is not wired at startup until a concrete use case exists. All infrastructure remains in place (Producer, Consumer, events, types) and the producer still publishes UserCreatedEvent on FCM registration. A fully worked example of how to wire the consumer back in is left as a comment in cmd/api/main.go. Also adds MaxIdleConns=1 and ConnMaxIdleTime=8s to the consumer client so idle connections are recycled before managed Redis providers kill them, preventing i/o timeout noise if the consumer is re-enabled.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
firebase_uid NOT NULL UNIQUE,email,photo_url,updated_attousers; backfills existing rows before applying NOT NULL;PATCH /api/v1/meupserts profile;DELETE /api/v1/meremoves the DB recordNewHandleWelcomeEmail(sender)constructor actually calls Mailjet;WelcomeEmailPayload+UserCreatedEventnow carry aNamefield so recipients are addressed by name (falls back to email)StreamProducerinterface wired through bootstrap → FCM handler publishesUserCreatedEventon token registration; consumer intentionally not started at runtime (avoids holding a persistent idle Redis connection with no business logic); full wiring example left as a comment incmd/api/main.go;Consumerclient setsMaxIdleConns=1+ConnMaxIdleTime=8sto prevent managed-Redis idle-kill noise when re-enabledReadPumpparses inbound frames asEnvelope;Hub.OnMessage(msgType, handler)routes them to registered handlers; 64-slot semaphore bounds concurrency; hub lifecycle context propagated to handlersdomain.Page[T],domain.CursorPage[T],PageRequestwithDefaults()/Offset()(guards against negative offset)bindJSON/bindQueryintransport/handlers/validation.go; return stable messages ("invalid request body","invalid query parameters") instead of rawerr.Error()StreamProducer.Close()called alongside Cache and Enqueuer; consumer goroutine (when wired) cancels via context before Redis client closesDocs updated
routing.md,database.md,migrations.md,queue.md,streams.md,websocket.md,email.md,error-handling.md,_index.md— all updated to reflect new routes, types, patterns, and wiring status.Test plan
cd backend && go build ./...— compiles cleancd backend && go vet ./...— no issuescd backend && go test ./internal/transport/handlers/... ./internal/usecase/... ./internal/infrastructure/ws/... ./internal/infrastructure/queue/...— unit tests passcd backend && go test -tags integration ./...— all 12 packages pass (requires Docker)cd backend && make swagger— docs regenerate;PATCH /api/v1/meandDELETE /api/v1/meappear in Swagger UI