ref: use @sentry/api client for requests#226
Conversation
Semver Impact of This PR🟢 Patch (bug fixes) 📋 Changelog PreviewThis is how your changes will appear in the changelog. New Features ✨
Bug Fixes 🐛Telemetry
Other
Internal Changes 🔧
🤖 This preview updates automatically when you update the PR. |
BYK
changed the title
Resolve merge conflicts: - package.json: keep @sentry/api as dependency, remove ky - api-client.ts: adapt listRepositories and listTransactions to use SDK-based request pattern instead of ky-based orgScopedRequest - types/index.ts: split type vs value exports for Zod schemas - bun.lock: regenerated Co-authored-by: Cursor <cursoragent@cursor.com>
Codecov Results 📊❌ Patch coverage is 67.86%. Project has 4126 uncovered lines. Files with missing lines (71)
Coverage diff@@ Coverage Diff @@
## main #PR +/-##
==========================================
- Coverage 69.08% 68.12% -0.96%
==========================================
Files 107 108 +1
Lines 13195 12944 -251
Branches 0 0 —
==========================================
+ Hits 9115 8818 -297
- Misses 4080 4126 +46
- Partials 0 0 —Generated by Codecov Action |
MathurAditya724
requested review from
BYK and
betegon
and removed request for
BYK
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
BYK
left a comment
BYK
left a comment
There was a problem hiding this comment.
I guess we lost the retry logic now? (unless it is baked into the API SDK) We should get that back.
| type DetailedLogsResponse, | ||
| listAnOrganization_sIssues, | ||
| listAnOrganization_sProjects, | ||
| listAProject_sClientKeys, |
There was a problem hiding this comment.
Auto generated, so can't change that
| getControlSiloUrl, | ||
| getDefaultSdkConfig, | ||
| getSdkConfig, | ||
| } from "./sentry-client.js"; |
There was a problem hiding this comment.
What's the value of splitting sentry-client off? the api-client is the Sentry Client and it cannot be anything else anyway?
There was a problem hiding this comment.
I have 2 words for you - "Circular dependency", between sentry-client.ts, api-client.ts and region.ts
| export async function getUserRegions(): Promise<Region[]> { | ||
| // Always use control silo for this endpoint | ||
| // /users/me/regions/ is an internal endpoint - use raw request | ||
| const response = await apiRequestToRegion<UserRegionsResponse>( |
There was a problem hiding this comment.
We should automate this logic: if the SDK has the endpoint, use that, if not fallback to our hand-rolled one.
There was a problem hiding this comment.
These endpoints are not part of the public API Spec, and the reason why they are not in the SDK
There was a problem hiding this comment.
I'd argue all the types from this file should also come from the API based on the OpenAPI schema
There was a problem hiding this comment.
The current state of the @sentry/api OpenAPI schema has significant gaps that make it impractical for many of these types. Either they are too generic, missing fields or internal endpoints
There was a problem hiding this comment.
Let's make a note to fix this then?
We have the retry logic in |
Resolve conflict in src/types/sentry.ts: keep SDK-derived SentryIssue type from feat/api-client and add seerFixabilityScore field from main. Remove duplicate Zod-based SentryIssueSchema and TraceContextSchema (unused). Retain new non-conflicting types from main (ISSUE_PRIORITIES, ReleaseSchema, SpanSchema). Co-authored-by: Cursor <cursoragent@cursor.com>
| /** | ||
| * Update an issue's status. | ||
| */ | ||
| export function updateIssueStatus( |
| } else if (input instanceof URL) { | ||
| raw = input.href; | ||
| } else { | ||
| raw = input.url; | ||
| } |
There was a problem hiding this comment.
If it is already an instance of URL, why not just do return input.pathname??
MathurAditya724
force-pushed
the
feat/api-client
branch
from
46e9723 to
c319c6d
Compare
Resolved conflicts in api-client.ts: converted new listTeams endpoint from orgScopedRequest (ky-based) to @sentry/api SDK pattern using listAnOrganization_sTeams, consistent with the rest of the branch. Co-authored-by: Cursor <cursoragent@cursor.com>
| throw new ApiError( | ||
| `Short ID ${normalizedShortId} resolved but no issue group returned`, | ||
| 404, | ||
| "Issue not found" |
There was a problem hiding this comment.
Bug: An unsafe type cast in getIssueByShortId can cause a runtime crash if the API response is missing required fields like shortId or title.
Severity: MEDIUM
Suggested Fix
Replace the unsafe type assertion with runtime validation. Use a library like Zod to define a schema for the expected API response and parse the data. This will ensure the group object has all required fields before it is used, preventing crashes from unexpected API structures.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: src/lib/api-client.ts#L761
Potential issue: The function `getIssueByShortId` uses an unsafe type assertion `as
unknown as { group?: SentryIssue }` to cast the response from the Sentry API. This
bypasses TypeScript's type checking. While the code checks for the existence of the
`group` object, it does not validate its contents at runtime. Downstream functions like
`formatIssueRow()` unconditionally access required properties such as `issue.shortId`
and `issue.title`. If the API returns a `group` object that is missing these fields, the
application will crash with an error like "Cannot read property 'shortId' of undefined".
Did we get this right? 👍 / 👎 to inform future reviews.
2 participants
Summary
Migrates the Sentry API client from hand-rolled
kyHTTP calls with Zod-validated responses to the@sentry/apiSDK package. This gives us type-safe API calls generated from the OpenAPI spec, while keeping raw requests for internal/undocumented endpoints.This supersedes #164, which generated the client in-repo using
@hey-api/openapi-ts. Using the published@sentry/apipackage instead avoids maintaining ~20k lines of generated code in the repo.What changed
New:
src/lib/sentry-client.tsRequest configuration layer for
@sentry/apiSDK functions. Provides:getSdkConfig(regionUrl)Rewritten:
src/lib/api-client.tsAll API functions now use
@sentry/apiSDK functions where possible:listOrganizationsInRegionlistYourOrganizationsgetOrganizationretrieveAnOrganizationlistProjectslistAnOrganization_sProjectsgetProjectretrieveAProjectgetProjectKeyslistAProject_sClientKeysgetIssueByShortIdresolveAShortIdgetLatestEventretrieveAnIssueEventgetEventretrieveAnEventForAProjecttriggerRootCauseAnalysisstartSeerIssueFixgetAutofixStateretrieveSeerIssueFixStatelistLogs/getLogqueryExploreEventsInTableFormatFunctions that use raw requests (no SDK equivalent):
getUserRegions—/users/me/regions/(internal endpoint)getCurrentUser—/users/me/(internal endpoint)findProjectByDsnKey—/projects/?query=dsn:...(internal query param)listIssues— SDK missinglimit/sortparamsgetIssue/updateIssueStatus— legacy/issues/{id}/endpoint (no org slug needed)getDetailedTrace—/organizations/{org}/trace/{traceId}/(internal endpoint)triggerSolutionPlanning— SDK doesn't supportrun_id+stepbodyRefactored:
src/types/sentry.ts@sentry/apiresponse types usingPartial<SdkType> & RequiredCoreRemoved
kydependency (replaced by@sentry/api+ native fetch)SentryOrganizationSchemaand other schema exports fromtypes/index.ts