fix(middleware): pass through http.Flusher for SSE streaming#52
Conversation
The request-logging, metrics, and tracing middlewares each wrap the ResponseWriter to capture status/bytes, but none implemented http.Flusher. Streaming handlers (/v1/answer/treewalk?stream=true, /v1/query/stream) do w.(http.Flusher) and errored with 'response writer does not support http.Flusher'. Add Flush() (delegating to the wrapped writer) + Unwrap() to statusRecorder, metricsRecorder, and tracingRecorder so flushes propagate through the whole chain and SSE streams in real time.
📝 WalkthroughWalkthroughThree middleware ResponseWriter wrapper types—statusRecorder, metricsRecorder, and tracingRecorder—each gain a Flush() method that delegates to the underlying http.Flusher when supported, and an Unwrap() method returning the wrapped http.ResponseWriter for http.ResponseController use. ChangesMiddleware ResponseWriter streaming compatibility
Estimated code review effort: 1 (Trivial) | ~5 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/middleware/tracing.go (1)
70-80: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCorrect fix; consider consolidating duplicated Flush/Unwrap logic.
The
Flush()/Unwrap()bodies here are identical to the ones just added instatusRecorder(logging.go) andmetricsRecorder(metrics.go). Since all three live in themiddlewarepackage, consider factoring the common logic into a small embeddable helper (e.g., aflusherstruct wrappinghttp.ResponseWriter) to avoid triplicated boilerplate going forward.🤖 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 `@internal/middleware/tracing.go` around lines 70 - 80, The Flush and Unwrap implementations in tracingRecorder duplicate the same response-writer passthrough logic already added in statusRecorder and metricsRecorder. Refactor this shared behavior into a small reusable helper in the middleware package (for example, an embeddable wrapper around http.ResponseWriter that provides Flush and Unwrap), then have tracingRecorder, statusRecorder, and metricsRecorder reuse it instead of keeping three copies.
🤖 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 `@internal/middleware/tracing.go`:
- Around line 70-80: The Flush and Unwrap implementations in tracingRecorder
duplicate the same response-writer passthrough logic already added in
statusRecorder and metricsRecorder. Refactor this shared behavior into a small
reusable helper in the middleware package (for example, an embeddable wrapper
around http.ResponseWriter that provides Flush and Unwrap), then have
tracingRecorder, statusRecorder, and metricsRecorder reuse it instead of keeping
three copies.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 91adfbc2-9405-4ce6-9618-c08db50d0edc
📒 Files selected for processing (3)
internal/middleware/logging.gointernal/middleware/metrics.gointernal/middleware/tracing.go
Streaming endpoints (
/v1/answer/treewalk?stream=true,/v1/query/stream) failed with "response writer does not support http.Flusher": the logging/metrics/tracing middlewares wrap theResponseWriterto capture status+bytes but none forwardedFlush(). AddedFlush()(delegating to the wrapped writer) +Unwrap()to all three recorders so flushes propagate through the chain. Unblocks the observable streaming Playground.Summary by CodeRabbit