feat(api): add core API client primitives#14
Conversation
Introduce a dedicated official Notion REST API client package and config loader scaffolding. Adds API base URL, Notion-Version, and token config with env overrides, plus tests for request behavior and config normalization.
📝 WalkthroughWalkthroughA new Notion API HTTP client is introduced with structured request/response handling, configuration management system, and CLI integration. The client supports PATCH operations with error handling and 429 throttling retry logic. Configuration is loaded from a JSON file with environment variable overrides. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 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 docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
internal/config/config_test.go (1)
37-47: Consider adding coverage forLoad()integration scenarios.The unit tests cover the individual helper functions well, but there's no direct test for
Load()that exercises file reading with JSON parsing (valid/invalid JSON, partial config, etc.). This could help catch integration issues.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/config/config_test.go` around lines 37 - 47, Add integration tests for Load() that create a real config file at the path returned by Path() (use t.Setenv("HOME", ...) or a temp dir and ensure Path() points there), then call Load() and assert behavior: one test with valid JSON to verify all fields are parsed into the Config struct, one test with partial JSON to verify defaults/zero-values are handled, and one test with invalid JSON to assert Load() returns a parsing error. Use os.WriteFile to create the file before calling Load() and remove/cleanup after; reference the Path() and Load() functions and the Config type when asserting expected output.internal/api/client.go (1)
156-171: Consider capping the Retry-After duration to prevent excessive waits.The current implementation honors any
Retry-Aftervalue returned by the server. A malicious or misconfigured server could return a very large value (e.g., hours), causing the client to block indefinitely even with a context timeout (since theselectonly checksctx.Done()at wait start). Consider capping to a reasonable maximum (e.g., 60 seconds).Suggested improvement
if resp.StatusCode == http.StatusTooManyRequests { retryAfter := parseRetryAfter(resp.Header.Get("Retry-After")) _ = resp.Body.Close() + const maxRetryWait = 60 * time.Second + if retryAfter > maxRetryWait { + retryAfter = maxRetryWait + } if retryAfter > 0 { select { case <-ctx.Done(): return ctx.Err() case <-time.After(retryAfter): } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/api/client.go` around lines 156 - 171, The 429 retry wait should be capped to avoid very long server-provided Retry-After values; in the block that checks resp.StatusCode == http.StatusTooManyRequests (using parseRetryAfter and before calling c.sendOnce), compute a capped duration (e.g., maxRetry := 60 * time.Second and wait := min(parseRetryAfter(...), maxRetry)), use that capped wait in the select that checks ctx.Done() and time.After(wait), and then proceed to call c.sendOnce; keep the existing resp.Body.Close() and error handling but replace the raw retryAfter usage with the capped value to prevent excessive waits.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@internal/api/client.go`:
- Around line 156-171: The 429 retry wait should be capped to avoid very long
server-provided Retry-After values; in the block that checks resp.StatusCode ==
http.StatusTooManyRequests (using parseRetryAfter and before calling
c.sendOnce), compute a capped duration (e.g., maxRetry := 60 * time.Second and
wait := min(parseRetryAfter(...), maxRetry)), use that capped wait in the select
that checks ctx.Done() and time.After(wait), and then proceed to call
c.sendOnce; keep the existing resp.Body.Close() and error handling but replace
the raw retryAfter usage with the capped value to prevent excessive waits.
In `@internal/config/config_test.go`:
- Around line 37-47: Add integration tests for Load() that create a real config
file at the path returned by Path() (use t.Setenv("HOME", ...) or a temp dir and
ensure Path() points there), then call Load() and assert behavior: one test with
valid JSON to verify all fields are parsed into the Config struct, one test with
partial JSON to verify defaults/zero-values are handled, and one test with
invalid JSON to assert Load() returns a parsing error. Use os.WriteFile to
create the file before calling Load() and remove/cleanup after; reference the
Path() and Load() functions and the Config type when asserting expected output.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ef107418-44e9-4bc9-9682-95a26f7d28ee
📒 Files selected for processing (5)
internal/api/client.gointernal/api/client_test.gointernal/cli/official_api.gointernal/config/config.gointernal/config/config_test.go
Summary by CodeRabbit