Minor Improvements

[ziptied]

updates for test coverage, refinments in the client, removed race condition and adjustments for mcp

[greptile-apps]

Confidence score: 3/5

• This PR contains complex changes to core client functionality that could impact reliability if the timeout implementation or response handling has edge cases
• Score reflects the significant refactoring of the client class and potential for timeout-related issues, plus the breaking change to upsertSubscriber API behavior that returns different data types
• Pay close attention to src/sdk/client/index.ts for timeout implementation details, src/versions/v1/index.ts for the API behavior change, and test files for comprehensive coverage validation

[greptile-apps]

Additional Comments (3)

1. __tests__/experimental/experimental.test.ts, line 312 (link)

   logic: Using 'example.com' as an IP parameter seems incorrect - this should likely be an IP address like '192.168.1.1' to match the parameter name and test intent

   Should this test use an actual IP address instead of a domain name for the ip parameter?

2. __tests__/helpers/mockFetch.ts, line 4-5 (link)

   style: Global mutable state in test utilities can cause test isolation issues if not properly reset between tests. Are these variables being reset between test cases to prevent test pollution?

   _{Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!}

3. __tests__/versions/v1/index.test.ts, line 331-395 (link)

   style: Duplicate test block for core functionality - this exact same test suite already exists at lines 109-195

_{17 files reviewed, 3 comments}

_{Edit Code Review Agent Settings | Greptile}

[jessehanley]

@ziptied thoughts on the Greptile comments?

[greptile-apps]

Greptile Overview

Greptile Summary

This PR adds timeout support to HTTP requests, refactors the client to use async/await, removes race conditions in batch operations, and updates API endpoints for broadcasts. Key changes include:

Client Refactoring

• Converted get(), post(), and patch() methods from Promise-based to async/await
• Added configurable timeout support (default: 30 seconds) with RequestTimeoutError
• Introduced _fetchWithTimeout() for AbortController-based timeout handling
• Added _handleResponse() to centralize response parsing and error handling
• Fixed query parameter string conversion

Batch Operations

• Removed timeout for batch imports (timeout: null) to prevent timeouts on large imports
• This eliminates race conditions where large batches would timeout before completion

Broadcasts API

• Updated endpoint URLs: /broadcasts → /fetch/broadcasts and /batch/broadcasts
• Updated emails endpoint: /emails → /batch/emails

API Behavior Changes

• upsertSubscriber() now returns Subscriber<S> | null instead of Subscriber<S>
• getSequences() and getWorkflows() now return empty array [] instead of null when no data

Test Coverage

• Added comprehensive tests for timeout handling, error scenarios, and batch operations
• Enhanced mock fetch helper to track AbortSignal usage

Confidence Score: 4/5

• Safe to merge with one minor bug fix needed in timeout error message
• The PR makes solid improvements to timeout handling and test coverage. The only issue found is the timeout error message using the wrong variable (reports default timeout instead of actual timeout used). This is a minor bug that doesn't break functionality but should be fixed for accurate error reporting. The refactoring to async/await is clean, batch timeout removal is intentional and well-tested, and API changes are documented.
• src/sdk/client/index.ts - Fix timeout error message to use actual timeout value

[greptile-apps]

[P1] Error message reports incorrect timeout value. Uses this._timeout instead of the actual timeout parameter that triggered the error. If a custom timeout is passed via requestOptions.timeout, the error will report the wrong duration.

Suggested change

	throw new RequestTimeoutError(`Request timed out after ${this._timeout}ms`);
	throw new RequestTimeoutError(`Request timed out after ${timeout}ms`);

Prompt To Fix With AI

This is a comment left during a code review.
Path: src/sdk/client/index.ts
Line: 182:182

Comment:
[P1] Error message reports incorrect timeout value. Uses `this._timeout` instead of the actual `timeout` parameter that triggered the error. If a custom timeout is passed via `requestOptions.timeout`, the error will report the wrong duration.

```suggestion
        throw new RequestTimeoutError(`Request timed out after ${timeout}ms`);
```

How can I resolve this? If you propose a fix, please make it concise.

[cursor]

The issues mentioned in the PR review have already been fixed in the codebase:

1. Timeout error message (line 182 in src/sdk/client/index.ts): The error message correctly uses the timeout parameter instead of this._timeout:

        throw new RequestTimeoutError(`Request timed out after ${timeout}ms`);

2. Typo fix (line 102 in src/sdk/batch/index.ts): The text correctly says "liquid tags" (not "liquid tsags")

All 235 tests pass with 100% function coverage. No changes were needed—the fixes were already applied to the code.

---

 

_{Learn more about Cursor Agents}

[greptile-apps]

Greptile Overview

Greptile Summary

This PR improves test coverage, refines the client implementation with timeout support, fixes a race condition in test helpers, and adjusts return types for better null handling.

Key Changes:

• Timeout Support: Added configurable request timeout (default 30s) using AbortController. Batch operations (subscribers/events) use timeout: null for unlimited timeout.
• Client Refactoring: Converted Promise constructor pattern to async/await, extracted _fetchWithTimeout and _handleResponse helper methods, improved type safety (Record<string, unknown> vs any).
• Return Type Consistency: Changed getSequences() and getWorkflows() to return [] instead of null for empty results. Updated upsertSubscriber to return Subscriber | null (returns null if subscriber not yet available after async import).
• Race Condition Fix: Added global state reset (resetMockFetchTracking) in test afterEach hooks to prevent cross-test contamination.
• API Endpoint Updates: Aligned broadcast endpoints to use /fetch/* for reads and /batch/* for writes.
• Error Handling: Added RequestTimeoutError, improved JSON parsing error messages, better content-type handling for error responses.

Testing: Comprehensive test coverage added for timeout behavior, error handling, endpoint verification, and edge cases.

Confidence Score: 4/5

• This PR is safe to merge with only a minor typo to fix
• The implementation is well-tested with comprehensive coverage of edge cases. The timeout implementation is correct with proper cleanup in finally blocks. The race condition fix properly isolates test state. Return type changes improve API consistency. Only one minor typo was found. The breaking change in upsertSubscriber return type is properly documented.
• src/sdk/batch/index.ts has a typo on line 102

[greptile-apps]

Additional Comments (1)

src/sdk/batch/index.ts
Typo: "liquid tsags" should be "liquid tags"

   * liquid tags that will be injected into the email.

Prompt To Fix With AI

This is a comment left during a code review.
Path: src/sdk/batch/index.ts
Line: 102:102

Comment:
Typo: "liquid tsags" should be "liquid tags"

```suggestion
   * liquid tags that will be injected into the email.
```

How can I resolve this? If you propose a fix, please make it concise.

[greptile-apps]

Greptile Overview

Greptile Summary

Overview

This PR introduces significant improvements to the Bento Node SDK focused on test coverage, client refinements, race condition fixes, and MCP-related adjustments.

Key Changes

1. Client Refactoring & Timeout Implementation

• Refactored from Promise constructor pattern (resolve/reject) to async/await, eliminating a potential race condition where resolve and error handlers could both execute
• Added configurable timeout support via ClientOptions.timeout (defaults to 30 seconds)
• Implemented _fetchWithTimeout method using AbortController for proper timeout handling
• Added timeout: null option for long-running operations (batch imports) to bypass timeout

2. Error Handling Improvements

• New RequestTimeoutError exception for timeout scenarios
• Enhanced _handleResponse method for better response parsing and error messages
• Improved _getErrorForResponse with content-type aware response handling (JSON, text/plain, unknown)
• Better JSON parsing error messages

3. API Endpoint Corrections

• Broadcasts: Corrected endpoints to /fetch/broadcasts (GET) and /batch/broadcasts (POST)
• Batch operations: Explicitly pass timeout: null to prevent premature timeouts on long-running imports

4. Return Type Refinements

• upsertSubscriber: Return type changed from Promise<Subscriber<S>> to Promise<Subscriber<S> | null> (now accurately reflects that it may return null)
• getSequences: Return type changed from Promise<Sequence[] | null> to Promise<Sequence[]> (returns empty array instead of null)
• getWorkflows: Return type changed from Promise<Workflow[] | null> to Promise<Workflow[]> (returns empty array instead of null)

5. Code Quality

• Fixed typo: "liquid tsags" → "liquid tags"
• String conversion in query parameters (URLSearchParams.append requires strings)
• Improved type safety with Record<string, unknown> for JSON responses

6. Comprehensive Test Coverage

• Added 200+ new test cases covering:
  • Timeout handling and edge cases
  • Base64 encoding implementations (btoa, Buffer, fallback)
  • Error handling across different HTTP status codes
  • Content-type specific response handling
  • Endpoint verification
  • Batch operations without timeout signals
  • All SDK modules (Broadcasts, Sequences, Workflows, EmailTemplates, Batch operations, etc.)

Breaking Changes

• upsertSubscriber now correctly typed to return Promise<Subscriber<S> | null> (was Promise<Subscriber<S>>)
• getSequences now returns Promise<Sequence[]> instead of Promise<Sequence[] | null>
• getWorkflows now returns Promise<Workflow[]> instead of Promise<Workflow[] | null>

These are API contract improvements that better reflect actual behavior.

Testing

All changes are thoroughly tested with comprehensive test coverage for happy paths, error scenarios, and edge cases.

Confidence Score: 4/5

• This PR is safe to merge with minimal risk. The refactoring improves reliability by eliminating race conditions, and comprehensive test coverage validates all changes.
• Score reflects: (1) Well-executed refactoring from Promise constructor to async/await eliminates a real race condition; (2) Proper timeout implementation using AbortController following web standards; (3) Breaking changes are intentional API improvements that correctly reflect actual behavior; (4) Exceptional test coverage (200+ new tests) validates timeout handling, error scenarios, and all modules; (5) No logical errors, security issues, or edge case problems found in the implementation; (6) The only minor concern is the breaking changes require users to update their code, but the changes are justified and improve type safety.
• No files require special attention. All changes have been thoroughly tested and the implementation is sound.