Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughAdds Router.NewServer(ctx) to bootstrap and return a runnable server, enhances test utilities to allow client-aware GraphQL requests, and introduces integration tests that exercise server lifecycle, health endpoint, and shutdown behavior using a mocked subgraph. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 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
Comment |
Router image scan passed✅ No security vulnerabilities found in image: |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2546 +/- ##
==========================================
+ Coverage 61.69% 62.32% +0.62%
==========================================
Files 239 239
Lines 25423 25456 +33
==========================================
+ Hits 15685 15865 +180
+ Misses 8418 8241 -177
- Partials 1320 1350 +30
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
router/core/router.go (1)
731-787: Consider mirroring usage-tracking calls fromStart().
NewServerconfigures usage tracking but doesn’t calltrackRouterConfigUsage/trackExecutionConfigUsage, so usage telemetry is skipped when this path is used. If that’s unintentional, consider invoking those once the config (static or polled) is available.💡 Possible alignment (illustrative)
if r.staticExecutionConfig != nil { + r.trackRouterConfigUsage() + r.trackExecutionConfigUsage(r.staticExecutionConfig, true) r.logger.Info("Static execution config provided. Polling is disabled. Updating execution config is only possible by providing a config.") return r.httpServer, r.newServer(ctx, r.staticExecutionConfig) } ... cfg, err := r.configPoller.GetRouterConfig(ctx) if err != nil { return nil, fmt.Errorf("failed to get initial execution config: %w", err) } + r.trackRouterConfigUsage() + r.trackExecutionConfigUsage(cfg.Config, false)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@router/core/router.go` around lines 731 - 787, NewServer configures usage tracking but never emits telemetry because it doesn't call trackRouterConfigUsage and trackExecutionConfigUsage; update NewServer to invoke r.trackRouterConfigUsage(...) and r.trackExecutionConfigUsage(...) once a config is available: for the static path call them before returning after r.newServer(ctx, r.staticExecutionConfig), and for the polled path call them after retrieving cfg (and/or after successfully applying it via r.newServer(ctx, cfg.Config)). Place these calls near the existing r.reloadPersistentState.UpdateReloadPersistentState(&r.Config) / right after successful r.newServer(...) so telemetry is recorded on both code paths.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@router-tests/lifecycle/new_server_test.go`:
- Around line 24-110: The subtest for shutdown only closes the httptest.Server
and never invokes the router/server shutdown, so update the "new server shutdown
prevents further requests" subtest to call rr.Shutdown (or the server shutdown
method) before asserting requests fail: after verifying the server works, call
rr.Shutdown(ctx) (or svr.Shutdown(ctx) if present) with a short timeout context,
then attempt the POST to ts.URL+"/graphql" and assert it errors; ensure you
still close ts (httptest.Server) and cancel the context to clean up.
---
Nitpick comments:
In `@router/core/router.go`:
- Around line 731-787: NewServer configures usage tracking but never emits
telemetry because it doesn't call trackRouterConfigUsage and
trackExecutionConfigUsage; update NewServer to invoke
r.trackRouterConfigUsage(...) and r.trackExecutionConfigUsage(...) once a config
is available: for the static path call them before returning after
r.newServer(ctx, r.staticExecutionConfig), and for the polled path call them
after retrieving cfg (and/or after successfully applying it via r.newServer(ctx,
cfg.Config)). Place these calls near the existing
r.reloadPersistentState.UpdateReloadPersistentState(&r.Config) / right after
successful r.newServer(...) so telemetry is recorded on both code paths.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
router-tests/lifecycle/new_server_test.gorouter-tests/testenv/testenv.gorouter/core/router.go
This PR adds back the NewServer function as it is needed by our AWS Lambda provider, additionally it could be used by external consumers.
Summary by CodeRabbit
Tests
New Features
Checklist