Merged
Conversation
Contributor
📝 WalkthroughWalkthroughUpgrades the Echo framework from v4 to v5 across the codebase, migrates handler and middleware signatures to use Changes
Sequence Diagram(s)sequenceDiagram
participant Test as Test Runner
participant App as Application
participant Server as HTTP Server
participant Logger as Audit Logger
Test->>App: Start(ctx, addr)
App->>Server: Start(ctx)
Server-->>App: started / serverDone channel
Note right of Server: Handles requests, middleware (Audit Logger) operate per request
Test->>App: cancel ctx (cleanup)
App->>Server: cancel/stop
Server-->>App: returns (nil | error)
App-->>Test: shutdown complete
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 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 |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/server/handlers_test.go`:
- Around line 123-125: The current helper setPathParam uses Echo v5's
SetPathValues and works for single "id" params but should be extended to support
multiple path params: change or add a helper (e.g., setPathParams) that accepts
either a map[string]string or variadic name/value pairs, build an
echo.PathValues slice from those entries, and call c.SetPathValues with the full
slice (using the SetPathValues method referenced in setPathParam) so tests can
inject multiple path parameters on the same context.
In `@internal/server/http.go`:
- Around line 296-325: parseBodySizeLimitBytes duplicates parsing logic already
implemented in config.ValidateBodySizeLimit; refactor to remove duplication by
extracting the shared parsing logic into a single helper (e.g., ParseSizeToBytes
or similar) and have both parseBodySizeLimitBytes and
config.ValidateBodySizeLimit call that helper, or alternatively have
parseBodySizeLimitBytes call config.ValidateBodySizeLimit and adapt its result
to int64 with the same fallback behavior; update references in
parseBodySizeLimitBytes and config.ValidateBodySizeLimit to use the new helper
and keep the same fallback/default (10MB) and logging behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 633e670b-0784-4cc0-a6c7-cdb72d3cad45
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (15)
cmd/gomodel/docs/docs.gocmd/gomodel/docs/swagger.jsongo.modinternal/admin/dashboard/dashboard.gointernal/admin/dashboard/dashboard_test.gointernal/admin/handler.gointernal/admin/handler_test.gointernal/auditlog/middleware.gointernal/server/auth.gointernal/server/auth_test.gointernal/server/handlers.gointernal/server/handlers_test.gointernal/server/http.gointernal/server/model_validation.gointernal/server/model_validation_test.go
| "user_agent", v.UserAgent, | ||
| "remote_ip", v.RemoteIP, | ||
| "request_id", c.Request().Header.Get("X-Request-ID"), | ||
| "request_id", v.RequestID, |
Contributor
Author
There was a problem hiding this comment.
@coderabbitai Does it take the value from X-Request-ID as before? Hmm
Contributor
There was a problem hiding this comment.
@SantiagoDePolonia Yes, v.RequestID does read from the X-Request-ID header — it's functionally equivalent to the old c.Request().Header.Get("X-Request-ID").
RequestID in RequestLoggerValues is documented as "request ID from request X-Request-ID header or response if request did not have value."
So the behavior is actually slightly more robust than before:
- It first checks the request
X-Request-IDheader — same as the old code. - If that's empty, it falls back to the response
X-Request-IDheader.
Since the custom middleware in http.go sets X-Request-ID on the request header (and also mirrors it to the response header) before the logger runs, v.RequestID will always pick up the UUID-generated value just as before. ✅
🧠 Learnings used
Learnt from: CR
Repo: ENTERPILOT/GOModel PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-08T15:34:30.801Z
Learning: Applies to internal/auditlog/*.go : `X-Request-ID` must be auto-generated (UUID) if not present in request and must propagate through context to providers and audit logs
Learnt from: SantiagoDePolonia
Repo: ENTERPILOT/GOModel PR: 33
File: internal/auditlog/auditlog_test.go:272-279
Timestamp: 2026-01-15T23:07:26.646Z
Learning: Go code guideline: Avoid using interface{} and map[string]interface{} for API request/response payload types. This should be restricted to public API surfaces and data payloads. It is acceptable to use interface{} in internal infrastructure packages (e.g., storage abstractions) only to break import cycles, and only when type assertions occur at initialization (not on every request) to minimize runtime type checks and improve readability. Apply this rule broadly to files under internal/ that deal with payload-like types, but not to internal components that are pure infrastructure or have runtime-per-request type assertions.
Learnt from: SantiagoDePolonia
Repo: ENTERPILOT/GOModel PR: 33
File: internal/auditlog/factory.go:112-143
Timestamp: 2026-01-15T23:07:37.652Z
Learning: Guideline: Do not use interface{} or map[string]interface{} for API request/response payload types. Prefer strongly-typed structs for API payload definitions to improve type safety, serialization, and documentation. Allow interface{} only in internal infrastructure code paths where pragmatic flexibility is necessary (e.g., to avoid import cycles or to handle highly dynamic internal contracts). In internal/auditlog/factory.go and similar non-API implementation files, applying this restriction is optional and should be evaluated on a case-by-case basis based on whether the type remains internal and does not define API boundary shapes.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/e2e/auditlog_test.go`:
- Around line 149-161: The deferred cleanup function uses require.NoError(t,
err) and require.NoError(t, logger.Close()) which call t.FailNow() and can panic
when run from a defer; update the cleanup closure (the function named cleanup
that reads from serverDone and calls logger.Close) to use assert.NoError(t, err)
/ assert.NoError(t, logger.Close()) or perform explicit checks (if err != nil {
t.Errorf("server shutdown error: %v", err) }) so the test reports errors without
invoking FailNow from a deferred context.
In `@tests/e2e/main_test.go`:
- Around line 96-105: In TestMain's shutdown sequence where it reads from
serverDone (the select block referencing testServer and serverDone), ensure
server shutdown errors cause a non-zero exit: when receiving err from serverDone
and err != nil, call os.Exit(1) (or set a local failure flag and call os.Exit(1)
after cleanup completes); also add the necessary import for os if missing so the
test process fails on shutdown errors instead of only logging them.
In `@tests/integration/setup_test.go`:
- Around line 113-116: The goroutine that calls application.Start(ctx, addr)
currently discards its error; change it to send the returned error into a
channel (e.g., errCh := make(chan error, 1)) from inside the anonymous goroutine
and have the test read from that channel (or select with a timeout) to fail the
test on startup errors. Locate the goroutine that builds addr and calls
application.Start and replace the blank assignment with errCh <-
application.Start(...), then in the test use a blocking receive or select to
capture the error and call t.Fatalf or similar when non-nil.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 249d6b4a-c0c9-4a9f-80d5-67fc9b7bf0e0
📒 Files selected for processing (9)
cmd/gomodel/main.goconfig/config.goconfig/config_test.gointernal/app/app.gointernal/auditlog/middleware.gointernal/server/http.gotests/e2e/auditlog_test.gotests/e2e/main_test.gotests/integration/setup_test.go
Comment on lines
+149
to
161
| cleanup := func() { | ||
| cancel() | ||
| select { | ||
| case err := <-serverDone: | ||
| require.NoError(t, err) | ||
| case <-time.After(5 * time.Second): | ||
| t.Fatalf("server shutdown timed out") | ||
| } | ||
| require.NoError(t, logger.Close()) | ||
| } | ||
|
|
||
| return serverURL, cleanup | ||
| } |
Contributor
There was a problem hiding this comment.
Potential issue: require.NoError in deferred cleanup may panic.
Using require.NoError(t, err) inside a deferred cleanup function can cause issues:
- If the server returns an error,
require.NoErrorcallst.FailNow()which panics - When called from a deferred function, this can lead to confusing test output
Consider using assert.NoError or explicit error checking:
🛡️ Suggested defensive handling
cleanup := func() {
cancel()
select {
case err := <-serverDone:
- require.NoError(t, err)
+ if err != nil {
+ t.Errorf("server shutdown error: %v", err)
+ }
case <-time.After(5 * time.Second):
t.Fatalf("server shutdown timed out")
}
- require.NoError(t, logger.Close())
+ if err := logger.Close(); err != nil {
+ t.Errorf("logger close error: %v", err)
+ }
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/e2e/auditlog_test.go` around lines 149 - 161, The deferred cleanup
function uses require.NoError(t, err) and require.NoError(t, logger.Close())
which call t.FailNow() and can panic when run from a defer; update the cleanup
closure (the function named cleanup that reads from serverDone and calls
logger.Close) to use assert.NoError(t, err) / assert.NoError(t, logger.Close())
or perform explicit checks (if err != nil { t.Errorf("server shutdown error:
%v", err) }) so the test reports errors without invoking FailNow from a deferred
context.
Comment on lines
+96
to
105
| if testServer != nil && serverDone != nil { | ||
| select { | ||
| case err := <-serverDone: | ||
| if err != nil { | ||
| fmt.Printf("Server shutdown error: %v\n", err) | ||
| } | ||
| case <-time.After(5 * time.Second): | ||
| fmt.Printf("Server shutdown timed out\n") | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider failing on server shutdown errors.
The cleanup logs errors but doesn't fail the test suite. In TestMain, there's no *testing.T available, so this is acceptable. However, consider returning a non-zero exit code if shutdown errors occur:
if err != nil {
fmt.Printf("Server shutdown error: %v\n", err)
os.Exit(1) // or set a flag to exit with error after cleanup completes
}This would catch shutdown issues that might indicate resource leaks or other problems.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/e2e/main_test.go` around lines 96 - 105, In TestMain's shutdown
sequence where it reads from serverDone (the select block referencing testServer
and serverDone), ensure server shutdown errors cause a non-zero exit: when
receiving err from serverDone and err != nil, call os.Exit(1) (or set a local
failure flag and call os.Exit(1) after cleanup completes); also add the
necessary import for os if missing so the test process fails on shutdown errors
instead of only logging them.
Comment on lines
113
to
116
| go func() { | ||
| addr := fmt.Sprintf("127.0.0.1:%d", port) | ||
| _ = application.Start(addr) | ||
| _ = application.Start(context.Background(), addr) | ||
| }() |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider capturing server start errors.
The server start error is discarded:
go func() {
addr := fmt.Sprintf("127.0.0.1:%d", port)
_ = application.Start(context.Background(), addr)
}()Unlike tests/e2e/main_test.go which captures errors via a channel, startup failures here would go unnoticed. Consider adding error coordination similar to the e2e pattern:
♻️ Suggested improvement
+ serverDone := make(chan error, 1)
go func() {
addr := fmt.Sprintf("127.0.0.1:%d", port)
- _ = application.Start(context.Background(), addr)
+ serverDone <- application.Start(context.Background(), addr)
}()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/integration/setup_test.go` around lines 113 - 116, The goroutine that
calls application.Start(ctx, addr) currently discards its error; change it to
send the returned error into a channel (e.g., errCh := make(chan error, 1)) from
inside the anonymous goroutine and have the test read from that channel (or
select with a timeout) to fail the test on startup errors. Locate the goroutine
that builds addr and calls application.Start and replace the blank assignment
with errCh <- application.Start(...), then in the test use a blocking receive or
select to capture the error and call t.Fatalf or similar when non-nil.
This was referenced Mar 12, 2026
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 CodeRabbit
New Features
Documentation
Chores