Open
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (6)
💤 Files with no reviewable changes (1)
✅ Files skipped from review due to trivial changes (3)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe PR updates the module Go target to 🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 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 |
verbotenj
force-pushed
the
feat/switch-to-net-http
branch
2 times, most recently
from
5958ce0 to
04bb1da
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
internal/api/api_test.go (1)
34-41: Consider adding logging and recovery middleware to test mux for parity.
newTestMux()only appliescorsMiddleware, while production appliesloggingMiddlewareandrecoveryMiddlewaretoo. This is fine for current tests, but if you add tests that depend on recovery behavior, you'd need to update this.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/api/api_test.go` around lines 34 - 41, newTestMux() only wraps the test handler with corsMiddleware while production Start() uses loggingMiddleware and recoveryMiddleware too; update newTestMux to wrap the mux with loggingMiddleware and recoveryMiddleware (in the same order Start() uses) so tests get identical middleware behavior. Locate newTestMux and wrap the returned handler with recoveryMiddleware(loggingMiddleware(corsMiddleware(mux))) (or the correct order used by Start()) to ensure parity for tests relying on logging/recovery behavior.internal/api/api.go (1)
357-361: Remove redundant error string check.The condition
err.Error() != ""is always true for non-nil errors, making theelsebranch unreachable. Additionally,fmt.Sprintf("%s", err)is equivalent toerr.Error().♻️ Proposed simplification
} else { - if err.Error() != "" { - writeJSON(w, http.StatusBadRequest, err.Error()) - } else { - writeJSON(w, http.StatusBadRequest, fmt.Sprintf("%s", err)) - } + writeJSON(w, http.StatusBadRequest, err.Error()) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/api/api.go` around lines 357 - 361, The code currently checks err.Error() != "" which is redundant and has an unreachable else; simplify by removing the conditional and always return the error string via writeJSON (use err.Error()) when err is non-nil; update the block that references err and writeJSON(w, http.StatusBadRequest, ...) to a single call that passes err.Error() as the response payload.go.mod (1)
3-3: Consider upgrading to Go 1.26.1, the latest stable version.Go 1.25.8 is a valid patch version, but Go 1.26.1 is the latest available as of March 2026. Verify whether the project intentionally targets 1.25 or if upgrading to 1.26.1 is feasible, as staying on a minor version behind may limit access to the latest language improvements and bug fixes.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@go.mod` at line 3, Update the Go version declared in the module by changing the go directive in go.mod from 1.25.8 to 1.26.1; verify build, CI workflows, and any toolchains (e.g., Dockerfiles, github actions, Makefile) that reference the Go version to ensure they are compatible with and updated to use Go 1.26.1.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/api/api.go`:
- Around line 43-56: The metrics txSubmitFailCount and txSubmitCount are
declared as Gauges but represent monotonic totals; change their declarations to
use prometheus.NewCounter with prometheus.CounterOpts and update the variable
types to prometheus.Counter (or prometheus.CounterVec if needed), keeping the
same Name/Help values, and then register them with
prometheus.MustRegister(txSubmitFailCount, txSubmitCount) as before so
rate()/increase() work correctly in queries.
- Around line 134-138: The writeJSON helper discards json.Encode errors causing
silent failures; change it to encode into a temporary buffer (e.g.,
bytes.Buffer) using json.NewEncoder(&buf).Encode(data), check the returned
error, and if encoding fails call http.Error(w, "Internal Server Error",
http.StatusInternalServerError) and log the error; only when encoding succeeds
set the Content-Type, WriteHeader(status) and copy the buffer to the
ResponseWriter. This preserves correct status handling and surface encoding
errors instead of sending partial/corrupt responses (refer to writeJSON,
json.NewEncoder, bytes.Buffer, http.Error).
- Around line 208-220: Create and use explicit http.Server instances with sane
ReadTimeout, WriteTimeout and IdleTimeout instead of calling
http.ListenAndServe/ListenAndServeTLS with zero-value settings: replace the
direct calls that reference addr, cfg and handler (the current return
http.ListenAndServe/ListenAndServeTLS) with a configured &http.Server{Addr:
addr, Handler: handler, ReadTimeout: ..., WriteTimeout: ..., IdleTimeout: ...}
and call Server.ListenAndServe or Server.ListenAndServeTLS accordingly (preserve
TLS branch using cfg.Tls.CertFilePath/KeyFilePath). Also stop discarding the
metrics listener error: replace the goroutine that creates metricsMux and calls
http.ListenAndServe with starting a configured http.Server for metrics (or
capture the returned error) and log/return the error instead of assigning to _
so metrics listen failures are surfaced.
---
Nitpick comments:
In `@go.mod`:
- Line 3: Update the Go version declared in the module by changing the go
directive in go.mod from 1.25.8 to 1.26.1; verify build, CI workflows, and any
toolchains (e.g., Dockerfiles, github actions, Makefile) that reference the Go
version to ensure they are compatible with and updated to use Go 1.26.1.
In `@internal/api/api_test.go`:
- Around line 34-41: newTestMux() only wraps the test handler with
corsMiddleware while production Start() uses loggingMiddleware and
recoveryMiddleware too; update newTestMux to wrap the mux with loggingMiddleware
and recoveryMiddleware (in the same order Start() uses) so tests get identical
middleware behavior. Locate newTestMux and wrap the returned handler with
recoveryMiddleware(loggingMiddleware(corsMiddleware(mux))) (or the correct order
used by Start()) to ensure parity for tests relying on logging/recovery
behavior.
In `@internal/api/api.go`:
- Around line 357-361: The code currently checks err.Error() != "" which is
redundant and has an unreachable else; simplify by removing the conditional and
always return the error string via writeJSON (use err.Error()) when err is
non-nil; update the block that references err and writeJSON(w,
http.StatusBadRequest, ...) to a single call that passes err.Error() as the
response payload.
🪄 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
Run ID: d4753da8-acfd-4c4c-a7cd-777bdd488c17
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (6)
.github/workflows/go-test.ymlgo.modinternal/api/api.gointernal/api/api_test.gointernal/logging/gin.gointernal/logging/logging.go
💤 Files with no reviewable changes (1)
- internal/logging/gin.go
Comment on lines
+134
to
+138
| func writeJSON(w http.ResponseWriter, status int, data any) { | ||
| w.Header().Set("Content-Type", "application/json") | ||
| w.WriteHeader(status) | ||
| _ = json.NewEncoder(w).Encode(data) | ||
| } |
There was a problem hiding this comment.
Handle the JSON encoding error to fix pipeline failure.
The linter correctly flags that the Encode error is discarded. If marshalling fails (e.g., a channel or func in data), the client receives a partial/corrupt response with no server-side visibility.
🔧 Proposed fix
func writeJSON(w http.ResponseWriter, status int, data any) {
w.Header().Set("Content-Type", "application/json")
w.WriteHeader(status)
- _ = json.NewEncoder(w).Encode(data)
+ if err := json.NewEncoder(w).Encode(data); err != nil {
+ logging.GetLogger().Error("failed to encode JSON response", "err", err)
+ }
}🧰 Tools
🪛 GitHub Actions: golangci-lint
[error] 137-137: golangci-lint (errchkjson): Error return value of (*encoding/json.Encoder).Encode is not checked (unsafe type any found).
🪛 GitHub Check: lint
[failure] 137-137:
Error return value of (*encoding/json.Encoder).Encode is not checked: unsafe type any found (errchkjson)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/api/api.go` around lines 134 - 138, The writeJSON helper discards
json.Encode errors causing silent failures; change it to encode into a temporary
buffer (e.g., bytes.Buffer) using json.NewEncoder(&buf).Encode(data), check the
returned error, and if encoding fails call http.Error(w, "Internal Server
Error", http.StatusInternalServerError) and log the error; only when encoding
succeeds set the Content-Type, WriteHeader(status) and copy the buffer to the
ResponseWriter. This preserves correct status handling and surface encoding
errors instead of sending partial/corrupt responses (refer to writeJSON,
json.NewEncoder, bytes.Buffer, http.Error).
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
3 issues found across 7 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="internal/api/api.go">
<violation number="1" location="internal/api/api.go:44">
P2: Use `prometheus.NewCounter` instead of `prometheus.NewGauge` for `tx_submit_fail_count` and `tx_submit_count`. These metrics only ever increase (via `.Inc()`), making them counters by definition. Using `Gauge` misrepresents the metric semantics and breaks `rate()` / `increase()` in PromQL queries.</violation>
<violation number="2" location="internal/api/api.go:219">
P2: The metrics server error from `ListenAndServe` is silently discarded. If the metrics listener fails to bind (e.g., port conflict), there will be no indication of the failure. Log the error instead of discarding it.</violation>
<violation number="3" location="internal/api/api.go:325">
P2: The `Content-Type` check is too strict: exact string comparison rejects valid `application/cbor` media types with parameters (e.g. `application/cbor; charset=utf-8`). Parse the media type before comparing.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| // Check our headers for content-type | ||
| if c.ContentType() != "application/cbor" { | ||
| // Log the error, return an error to the user, and increment failed count | ||
| if r.Header.Get("Content-Type") != "application/cbor" { |
There was a problem hiding this comment.
P2: The Content-Type check is too strict: exact string comparison rejects valid application/cbor media types with parameters (e.g. application/cbor; charset=utf-8). Parse the media type before comparing.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At internal/api/api.go, line 325:
<comment>The `Content-Type` check is too strict: exact string comparison rejects valid `application/cbor` media types with parameters (e.g. `application/cbor; charset=utf-8`). Parse the media type before comparing.</comment>
<file context>
@@ -275,33 +316,34 @@ func handleHasTx(c *gin.Context) {
// Check our headers for content-type
- if c.ContentType() != "application/cbor" {
- // Log the error, return an error to the user, and increment failed count
+ if r.Header.Get("Content-Type") != "application/cbor" {
logger.Error("invalid request body, should be application/cbor")
- c.JSON(415, "invalid request body, should be application/cbor")
</file context>
verbotenj
force-pushed
the
feat/switch-to-net-http
branch
from
04bb1da to
bb7a37c
Compare
Contributor
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
internal/api/api.go (1)
219-221: Useerrors.Isfor error comparison.Direct comparison with sentinel errors doesn't account for wrapped errors. Use
errors.Isfor robust error checking.♻️ Proposed fix
- if err := metricsServer.ListenAndServe(); err != nil && err != http.ErrServerClosed { + if err := metricsServer.ListenAndServe(); err != nil && !errors.Is(err, http.ErrServerClosed) { logger.Error("metrics listener failed", "err", err) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/api/api.go` around lines 219 - 221, The current check compares err directly to http.ErrServerClosed which fails for wrapped errors; update the condition in the metrics server shutdown branch that calls metricsServer.ListenAndServe() to use errors.Is(err, http.ErrServerClosed) (i.e., treat the error as benign when errors.Is returns true) and add an import for the standard "errors" package; locate the comparison near the metricsServer.ListenAndServe() call and replace the direct equality check with !errors.Is(err, http.ErrServerClosed) while leaving logger.Error("metrics listener failed", "err", err) unchanged for real errors.internal/api/api_test.go (1)
59-65: Consider checking key existence in JSON assertion.
body["failed"]returnsfalseboth when the key exists with valuefalseand when the key is missing. An explicit existence check would make the test more precise.♻️ Proposed fix
var body map[string]bool if err := json.Unmarshal(rec.Body.Bytes(), &body); err != nil { t.Fatalf("failed to parse body: %s", err) } - if body["failed"] != false { + val, ok := body["failed"] + if !ok { + t.Fatal("expected 'failed' key in response") + } + if val != false { t.Errorf("expected failed=false, got %v", body["failed"]) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/api/api_test.go` around lines 59 - 65, The test currently unmarshals into body (map[string]bool) and asserts body["failed"] is false, which conflates missing key with false value; after json.Unmarshal (where body is defined) add an explicit presence check using the map lookup idiom (e.g., _, ok := body["failed"]) and fail the test if the key is missing, then assert the value is false using the same lookup (value, _ := body["failed"]) or combined form; reference the existing body variable, the json.Unmarshal call, and the rec.Body.Bytes() source when locating where to add the presence check and subsequent value assertion.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/api/api.go`:
- Around line 379-391: The current async error goroutine can cause
double-counting because txSubmitCount is incremented immediately while
txSubmitFailCount may be incremented later; modify the flow so txSubmitCount is
only incremented after the async error window completes (or a timeout) to ensure
mutual exclusivity: after calling submit.SubmitTx and before writeJSON, start a
short-lived goroutine or use a select with a timeout that reads from errorChan
(the same channel used in the existing anonymous goroutine) and only increment
txSubmitCount.Inc() when no error was received within that window, otherwise
increment txSubmitFailCount.Inc(); ensure you update/remove the existing
goroutine that reads errorChan to avoid duplicate reads and keep
writeJSON(txHash) semantics intact.
---
Nitpick comments:
In `@internal/api/api_test.go`:
- Around line 59-65: The test currently unmarshals into body (map[string]bool)
and asserts body["failed"] is false, which conflates missing key with false
value; after json.Unmarshal (where body is defined) add an explicit presence
check using the map lookup idiom (e.g., _, ok := body["failed"]) and fail the
test if the key is missing, then assert the value is false using the same lookup
(value, _ := body["failed"]) or combined form; reference the existing body
variable, the json.Unmarshal call, and the rec.Body.Bytes() source when locating
where to add the presence check and subsequent value assertion.
In `@internal/api/api.go`:
- Around line 219-221: The current check compares err directly to
http.ErrServerClosed which fails for wrapped errors; update the condition in the
metrics server shutdown branch that calls metricsServer.ListenAndServe() to use
errors.Is(err, http.ErrServerClosed) (i.e., treat the error as benign when
errors.Is returns true) and add an import for the standard "errors" package;
locate the comparison near the metricsServer.ListenAndServe() call and replace
the direct equality check with !errors.Is(err, http.ErrServerClosed) while
leaving logger.Error("metrics listener failed", "err", err) unchanged for real
errors.
🪄 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
Run ID: dbb26fc3-d5cb-4783-b780-f043eb5963b6
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (6)
.github/workflows/go-test.ymlgo.modinternal/api/api.gointernal/api/api_test.gointernal/logging/gin.gointernal/logging/logging.go
💤 Files with no reviewable changes (1)
- internal/logging/gin.go
Signed-off-by: Ales Verbic <verbotenj@blinklabs.io>
verbotenj
force-pushed
the
feat/switch-to-net-http
branch
from
bb7a37c to
a550570
Compare
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 by cubic
Replaced
ginwithnet/httpfor the API and metrics to reduce footprint while keeping behavior the same. Added basic API tests.Refactors
http.ServeMux; static UI at/uiwith root redirect.promhttp; counterstx_submit_countandtx_submit_fail_count.Accept: application/cbor.github.com/swaggo/http-swaggerat/swagger/.internal/logging/gin.go; consolidated logging; addedinternal/api/api_test.gocovering healthcheck, submit error paths, and CORS.Dependencies
github.com/gin-gonic/gin,github.com/gin-contrib/*,github.com/penglongli/gin-metrics,github.com/swaggo/gin-swagger.github.com/prometheus/client_golang,github.com/swaggo/http-swagger.1.25.8, bumpedgithub.com/blinklabs-io/gouroborostov0.160.3; CI runs on Go1.25.xand1.26.x.Written for commit a550570. Summary will update on new commits.
Summary by CodeRabbit
Chores
New Features
Tests
Chores (logging)