Add support for automatic stream compression (defaults off) - #186
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughAdds optional gzip compression for stream uploads, a direct write API on Stream, and exposes Stream.bytesWritten and Stream.compressed; updates StreamImpl constructor and request headers to support Content‑Encoding: gzip, plus tests and a patch changeset. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant U as User Code
participant SDK as SDK Stream API
participant G as Gzip Transform (optional)
participant HTTP as HTTP PUT
U->>SDK: createStream({ compress?: true, ... })
SDK->>SDK: build underlying ReadableStream
alt compress == true
SDK->>G: create gzip Transform, pipe underlying -> gzip
G-->>SDK: compressed ReadableStream
SDK->>SDK: add header `Content-Encoding: gzip`
else compress != true
SDK->>SDK: use original ReadableStream
end
U->>SDK: stream.write(chunk) or getWriter() -> writer.write(chunk)
SDK->>HTTP: PUT /stream with headers and body (ReadableStream)
HTTP-->>SDK: response
SDK-->>U: stream handle (bytesWritten, compressed)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/apis/stream.ts (2)
224-227: Wrap long declaration for 80-char limitThis destructuring declaration exceeds the 80-character limit in our guidelines. Break it across lines to stay within the style rules.
230-235: Also wrap long assignmentsBoth
createGzip()assignments exceed 80 characters. Please fold them to comply with the line-length requirement.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.changeset/hip-actors-worry.md(1 hunks)src/apis/stream.ts(3 hunks)src/types.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
{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/types.tssrc/apis/stream.ts
src/apis/**
📄 CodeRabbit inference engine (AGENT.md)
Place core API implementations under src/apis/ (email, discord, keyvalue, vector, objectstore)
Files:
src/apis/stream.ts
🧬 Code graph analysis (1)
src/apis/stream.ts (1)
src/apis/api.ts (1)
getFetch(12-14)
…erface Added Features: - Stream.write() method for direct writing without calling getWriter() - Stream.bytesWritten (readonly) - tracks total bytes written to stream - Stream.compressed (readonly) - indicates if stream uses compression - Both write() and getWriter() patterns work correctly with proper locking Implementation Details: - StreamImpl now maintains activeWriter for write() method - Added bytesWritten getter that references underlying sink's total counter - Added compressed property set during stream creation from props - Fixed locking mechanism to support both direct write() and getWriter() usage Bug Fixes: - Fixed test timeout issues caused by backpressure in stream mocks - Updated test mocks to continuously drain ReadableStream to prevent blocking - Resolved WritableStream locking conflicts between write() and getWriter() Test Coverage: - Added 8 new test cases for direct write() functionality - Tests verify write() with multiple data types (string, Uint8Array, ArrayBuffer, objects) - Tests verify bytesWritten tracking with write() and getWriter() - Tests verify bytesWritten accuracy across different data types - Tests verify compressed property for both compressed and uncompressed streams - All 44 stream tests passing (up from 36) Breaking Changes: None - purely additive changes to Stream interface Amp-Thread-ID: https://ampcode.com/threads/T-9e6fc111-b842-445d-a110-640caa51a5ac Co-authored-by: Amp <amp@ampcode.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
.changeset/hip-actors-worry.md (1)
8-8: Minor wording improvement.The phrase "has compression enabled" is clearer than "has compression" for describing boolean state.
Apply this diff:
-- Added property `compressed` to the Stream interface which represents if the stream has compression enabled +- Added property `compressed` to the Stream interface which represents whether the stream has compression enabledsrc/apis/stream.ts (1)
394-401: Consider alternative for bytesWritten exposure.Using
Object.definePropertyto dynamically bind_bytesWrittento the closure variabletotalworks but is unconventional. Consider instead storingtotalin the instance (e.g., updatingthis._bytesWrittenin the sink'swritemethod) for clarity.Example alternative approach (update the sink's write method):
async write(chunk: string | Uint8Array | ArrayBuffer | Buffer | object) { // ... existing coercion logic ... await writer.write(binaryChunk); total += binaryChunk.length; stream._bytesWritten = total; // Keep instance in sync }Then remove the
Object.definePropertyblock and rely on the instance field directly. This avoids closure magic and makes the data flow more explicit.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.changeset/hip-actors-worry.md(1 hunks)src/apis/stream.ts(6 hunks)src/types.ts(2 hunks)test/stream.test.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
{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/types.tstest/stream.test.tssrc/apis/stream.ts
test/**
📄 CodeRabbit inference engine (AGENT.md)
Tests must mirror the source structure under the test/ directory
Files:
test/stream.test.ts
src/apis/**
📄 CodeRabbit inference engine (AGENT.md)
Place core API implementations under src/apis/ (email, discord, keyvalue, vector, objectstore)
Files:
src/apis/stream.ts
🧬 Code graph analysis (2)
test/stream.test.ts (1)
src/apis/api.ts (1)
setFetch(8-10)
src/apis/stream.ts (3)
src/types.ts (1)
Stream(364-398)src/utils/stringify.ts (1)
safeStringify(6-40)src/apis/api.ts (1)
getFetch(12-14)
🔇 Additional comments (11)
src/types.ts (2)
346-351: LGTM!The
compress?: truetype is intentional per maintainer feedback—it enforces explicit opt-in by preventing boolean variables. The optional?handles the default-false case.
373-384: LGTM!The new Stream interface members are well-typed and documented:
bytesWrittenandcompressedare appropriatelyreadonlywrite()accepts the same types as the underlying sink- Signatures match the implementation
test/stream.test.ts (2)
1127-1390: LGTM!Comprehensive test coverage for the new
write()method andbytesWrittentracking:
- Verifies direct write() with multiple data types
- Tracks bytesWritten correctly across write() and getWriter() usage
- Tests compressed property for both enabled and disabled states
- Validates PUT request initiation behavior
1392-1596: LGTM!Thorough compression test coverage:
- Verifies Content-Encoding: gzip header when enabled
- Confirms actual data compression (size reduction)
- Tests error propagation through the compression pipeline
- Good edge-case handling
src/apis/stream.ts (7)
7-7: LGTM!Import of
createGzipfor compression support is appropriate.
15-24: LGTM!New private fields and updated constructor correctly store compression state and provide writer tracking for the direct write() API.
26-32: LGTM!Public getters for
bytesWrittenandcompressedexpose the internal state as specified in the Stream interface.
34-55: LGTM!The
write()method correctly:
- Coerces strings, ArrayBuffer, Uint8Array, and objects to Uint8Array
- Uses
safeStringifyfor object serialization- Reuses a single active writer across calls
61-104: LGTM!The
close()method gracefully handles:
- Active writer cleanup when using direct write() API
- Already-closed and locked stream states
- Proper error propagation for unexpected failures
262-294: LGTM!The compression pipeline is correctly wired:
- Gzip stream converts to Web streams via node:stream adapters
- Error handlers abort the fetch and writer on compression failures (addressing past review comment)
- Base readable is replaced with compressed readable for the PUT body
308-318: LGTM!Centralized headers object improves maintainability and correctly adds
Content-Encoding: gzipwhen compression is enabled.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/apis/stream.ts (1)
7-7: Consider conditional import of zlib.The
createGzipimport at the top level is only used whenprops?.compressis true. Since you already conditionally importnode:streamat line 274, consider doing the same fornode:zlibto avoid loading it when compression is disabled.-import { createGzip } from 'node:zlib';Then at line 283:
+ const { createGzip } = await import('node:zlib'); const gzipStream = createGzip();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/apis/api.ts(1 hunks)src/apis/stream.ts(8 hunks)src/router/context.ts(2 hunks)src/types.ts(2 hunks)test/apis/api.test.ts(7 hunks)test/stream.test.ts(21 hunks)test/utils/stringify.test.ts(5 hunks)
✅ Files skipped from review due to trivial changes (4)
- src/router/context.ts
- src/apis/api.ts
- test/apis/api.test.ts
- test/utils/stringify.test.ts
🧰 Additional context used
📓 Path-based instructions (3)
{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/types.tssrc/apis/stream.tstest/stream.test.ts
src/apis/**
📄 CodeRabbit inference engine (AGENT.md)
Place core API implementations under src/apis/ (email, discord, keyvalue, vector, objectstore)
Files:
src/apis/stream.ts
test/**
📄 CodeRabbit inference engine (AGENT.md)
Tests must mirror the source structure under the test/ directory
Files:
test/stream.test.ts
🧬 Code graph analysis (2)
src/apis/stream.ts (3)
src/types.ts (1)
Stream(364-400)src/utils/stringify.ts (1)
safeStringify(6-40)src/apis/api.ts (1)
getFetch(12-14)
test/stream.test.ts (2)
src/types.ts (1)
CreateStreamProps(335-352)src/apis/api.ts (1)
setFetch(8-10)
🪛 GitHub Actions: Run Tests
src/apis/stream.ts
[error] 361-361: TS2341: Property '_bytesWritten' is private and only accessible within class 'StreamImpl'. While running 'tsc --emitDeclarationOnly --declaration'.
🔇 Additional comments (9)
src/types.ts (3)
346-351: Documentation is clear and accurate.The JSDoc properly warns that clients must handle gzip decompression, and the literal
truetype enforces explicit opt-in as intended.
373-386: Public API additions look correct.The new
bytesWritten,compressedproperties andwrite(chunk)method properly extend the Stream interface with appropriate types and documentation.
478-479: Type constraint tightened appropriately.Constraining
VectorSearchParams<T extends JsonObject = JsonObject>ensures metadata filtering aligns with the JsonObject shapes used throughout the API surface.test/stream.test.ts (1)
1-1657: LGTM! Comprehensive test coverage for stream compression and direct write functionality.The test suite thoroughly covers:
- Stream creation validation and props handling
- Streaming functionality with various data types
- Content-Type handling (custom and default)
- Direct write() and close() methods
- bytesWritten tracking across different write patterns
- Compression behavior including Content-Encoding header verification
- Error handling and edge cases
The tests are well-structured, use proper mocking patterns, and verify both interface correctness and data flow architecture.
src/apis/stream.ts (5)
273-303: Compression pipeline correctly propagates errors.The gzip compression setup properly handles error propagation from both pipeline stages to abort the fetch and writer, addressing the concern from the previous review.
42-62: Direct write() method correctly reuses active writer.The implementation properly coerces various chunk types to
Uint8Arrayand maintains a singleactiveWriteracross multiple write calls, which is efficient and correct.
68-108: close() method gracefully handles multiple scenarios.The enhanced error handling correctly manages already-closed streams, locked streams with active writers, and other edge cases. The logic is sound and aligns with the test expectations.
317-326: Headers construction is clean and conditional.The centralized headers object with conditional
Content-Encoding: gzipis more maintainable than inline header construction.
361-361: Fix compilation error: Cannot access private property from closure.The pipeline failure indicates that
_bytesWrittenis private but accessed from theunderlyingSink.write()closure at line 361. This causes a TypeScript compilation error (TS2341).Solution: Change
_bytesWrittento public or add a setter method:- private _bytesWritten = 0; + public _bytesWritten = 0;Or add a setter method:
+ setBytesWritten(value: number): void { + this._bytesWritten = value; + }Then update line 361:
if (streamInstance) { - streamInstance._bytesWritten = total; + streamInstance.setBytesWritten(total); }Likely an incorrect or invalid review comment.
* 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>
Added Features:
Implementation Details:
Bug Fixes:
Test Coverage:
Summary by CodeRabbit
New Features
Tests
Chores