Skip to content

Added some sdk things#75

Open
shaply wants to merge 3 commits intomainfrom
davidgu/EmailSenderAndDomainTable
Open

Added some sdk things#75
shaply wants to merge 3 commits intomainfrom
davidgu/EmailSenderAndDomainTable

Conversation

@shaply
Copy link
Copy Markdown
Contributor

@shaply shaply commented Apr 6, 2026

No description provided.

@shaply shaply requested a review from llam36 April 6, 2026 03:44
@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented Apr 6, 2026

Greptile Summary

This PR adds two new methods — getSenders and getDomains — to the EmailAPI class, along with basic tests for each. While the functionality fills a genuine gap in the SDK, the implementation departs significantly from the established patterns in the class and has several issues that should be addressed before merging.

  • Runtime bug: When EmailAPI is constructed without a baseURL, basePath defaults to '', causing fetch('/email/senders') to throw TypeError: Failed to parse URL in Node.js. The first test for each method actually hits this bug but masks it by only checking that something throws.
  • Inconsistent implementation pattern: All existing methods use this.internalApi.emailController*() with a typed middleware callback for header injection. The new methods bypass this entirely via (this.internalApi as any).configuration and call raw fetch, skipping any configured middleware and relying on a private implementation detail.
  • Duplicated logic: The ~17-line block for header construction, basePath extraction, and token retrieval is copy-pasted verbatim between getSenders and getDomains. A private helper method should be extracted.
  • Missing return types: Both methods return Promise<any>, abandoning the typed-response convention used throughout the rest of the class.
  • Weak tests: No success-path tests, no fetch mocking, and one scenario exercises an unparseable URL rather than an actual authentication failure.

PR checklist areas needing improvement:

  • ❌ Functions have been modularized as much as possible (duplicated logic in both methods)
  • ❌ Schemas are strict / avoids any types (Promise<any> return types)
  • ❌ Input is appropriately validated (no guard for empty basePath)
  • ❌ Uses try/catch blocks for error handling (no catch in new methods)

Score: 50 / 100

Confidence Score: 2/5

Not safe to merge — contains a runtime bug (relative URL parse error), bypasses the typed internal API client via unsafe casting, and has significant code quality issues inconsistent with the rest of the codebase

Score of 2 reflects a runtime bug in the no-baseURL case that is masked by inadequate tests, combined with systematic deviation from established patterns (as-any casting, raw fetch, Promise returns, and duplicated logic). The feature is functional only in the happy path when a valid baseURL and token are configured.

src/lib/email.ts requires the most attention for the implementation issues; tests/email/emailApi.test.ts needs fetch mocking and success-path coverage

Important Files Changed

Filename Overview
src/lib/email.ts Adds getSenders/getDomains via raw fetch with as-any casting of private internals, duplicated logic, Promise return types, and a relative-URL bug when no baseURL is configured
tests/email/emailApi.test.ts Adds error-path-only tests for getSenders/getDomains; no success cases, no fetch mocking, and the first test in each suite masks a URL parse bug

Sequence Diagram

sequenceDiagram
    participant Caller
    participant EmailAPI
    participant InternalApi as internalApi (typed)
    participant RawFetch as fetch (raw)
    participant Server

    Note over Caller,Server: Existing methods (e.g. getEmailConfig, registerDomain)
    Caller->>EmailAPI: getEmailConfig(projectId, credentials)
    EmailAPI->>InternalApi: emailControllerGetEmailConfigById({id}, middleware)
    InternalApi->>Server: GET /email/config/{id} with headers via middleware
    Server-->>InternalApi: EmailConfigResponse (typed)
    InternalApi-->>EmailAPI: typed response
    EmailAPI-->>Caller: Promise<EmailConfigResponse>

    Note over Caller,Server: New methods — bypass internalApi entirely
    Caller->>EmailAPI: getSenders(credentials)
    EmailAPI->>EmailAPI: cast internalApi as any to read .configuration
    EmailAPI->>RawFetch: fetch(basePath + /email/senders, headers)
    alt basePath is empty string
        RawFetch-->>EmailAPI: TypeError: Failed to parse URL
        EmailAPI-->>Caller: throws (unguarded)
    else basePath is valid
        RawFetch->>Server: GET /email/senders
        Server-->>RawFetch: JSON body
        RawFetch-->>EmailAPI: Response
        EmailAPI-->>Caller: Promise<any>
    end
Loading

Reviews (1): Last reviewed commit: "Added some sdk things" | Re-trigger Greptile

Comment thread src/lib/email.ts Outdated
Comment thread src/lib/email.ts Outdated
Comment thread src/lib/email.ts
Comment thread src/lib/email.ts Outdated
Comment thread tests/email/emailApi.test.ts
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant