Repository navigation
Conversation
Only send Content-Type: application/json when request body is present. Prevents HTTP servers from rejecting GET requests with unnecessary headers. Co-authored-by: Amp <amp@ampcode.com> Amp-Thread-ID: https://ampcode.com/threads/T-81e76828-0341-4b71-9974-0b7486dd13df
|
Warning Rate limit exceeded@jhaynie has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 8 minutes and 2 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (3)
WalkthroughRemoved the default Content-Type header from internal request sending so callers must provide Content-Type when needed; also removed a special-case writer-close on TypeError('locked') in StreamImpl.close. Tests and changelog adjusted accordingly. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Caller
participant API as API.send
participant Fetch as fetch()
Caller->>API: send(url, method, headers?, body?)
Note over API: Build headers without a default Content-Type\n(do not insert Content-Type based on body)
alt caller provided Content-Type
API->>API: Use provided Content-Type in headers
else no Content-Type provided
API->>API: Send headers as-is (no Content-Type)
end
API->>Fetch: fetch(url, { method, headers, body? })
Fetch-->>API: Response/Promise
API-->>Caller: Result
sequenceDiagram
autonumber
participant User
participant Stream as StreamImpl
User->>Stream: close()
Stream->>Stream: attempt close (best-effort)
alt previous behavior: TypeError('locked') occurred
Note over Stream: previously tried to close active writer and clear reference\nnow that special-case is removed
end
Stream-->>User: resolved/returned (no writer-force-close on locked error)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/apis/api.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
src/apis/**
📄 CodeRabbit inference engine (AGENT.md)
Place core API implementations under src/apis/ (email, discord, keyvalue, vector, objectstore)
Files:
src/apis/api.ts
{src,test}/**/!(*.d).ts
📄 CodeRabbit inference engine (AGENT.md)
{src,test}/**/!(*.d).ts: Use strict TypeScript and prefer unknown over any
Use ESM import/export syntax; avoid CommonJS require/module.exports
Use relative imports for internal modules
Keep imports organized (sorted, no unused imports)
Use tabs with a visual width of 2 spaces
Limit lines to a maximum of 80 characters
Use single quotes for strings
Use proper Error types; do not throw strings
Prefer template literals over string concatenation
Files:
src/apis/api.ts
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.changeset/tame-meals-occur.md (1)
5-6: Fix incomplete sentence and clarify scope (GET/HEAD, body presence).Current text trails off and doesn’t state when Content-Type should be set. Suggest explicit, user-facing wording.
-Remove the explicit Content-Type header for application/json in send (internal) to allow each service caller to properly set +Only set a Content-Type header when a request body is present. Remove the default `Content-Type: application/json` from the internal `send` path so each service sets the header explicitly for requests with bodies. This prevents sending Content-Type on GET/HEAD and avoids unnecessary CORS preflights and server rejections. + +Refs: AGENT-793 (vector.get not working), Amp-Thread-ID T-81e76828-0341-4b71-9974-0b7486dd13df.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.changeset/tame-meals-occur.md(1 hunks)src/apis/api.ts(1 hunks)src/apis/stream.ts(0 hunks)test/apis/api.test.ts(0 hunks)
💤 Files with no reviewable changes (2)
- src/apis/stream.ts
- test/apis/api.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/apis/api.ts
🔇 Additional comments (1)
.changeset/tame-meals-occur.md (1)
1-3: Verify default Content-Type in internal requests
Several body-bearing calls in your SDK implementation (e.g. under src/apis, src/io, src/server, src/otel) don’t set an explicitContent-Typeheader. Confirm your HTTP client wrapper still defaults toapplication/jsonfor request bodies; if it doesn’t, add the header or bump this change to a minor release.
* Update changelog for sdk-js v0.0.147 - Reformat v0.0.147 entry to use Keep a Changelog format with Added and Fixed sections - Add version comparison links at the bottom of the changelog - Document PR #186: Stream compression features - Document PR #185: Fix unnecessary Content-Type header on GET requests Co-Authored-By: unknown <> * update --------- Co-authored-by: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> Co-authored-by: Jeff Haynie <jhaynie@gmail.com>
Only send Content-Type: application/json when request body is present. Prevents HTTP servers from rejecting GET requests with unnecessary headers.
Amp-Thread-ID: https://ampcode.com/threads/T-81e76828-0341-4b71-9974-0b7486dd13df
Summary by CodeRabbit