feat(api): Add sentryCellFetch with injectable error handling#116114
feat(api): Add sentryCellFetch with injectable error handling#116114ryan953 wants to merge 6 commits into
Conversation
New fetch implementation that calls fetch() directly instead of going through Client.request(), with all UI/navigation error handling extracted into configurable callbacks (onAuthError, onSudoRequired, onProjectRenamed, onError). Includes factory functions that replicate the existing error handling logic, a Jest mock backed by MockApiClient, and comprehensive tests for both the fetch layer and error handlers.
Both sentryCellFetch and sentryCellFetchInfinite now delegate to a shared fetchWithUrl that accepts a plain url string instead of a query key, reducing duplication and enabling direct use outside of TanStack Query contexts.
Verifies that both the old path (apiFetch → Client.requestPromise) and the new path (sentryCellFetch → fetch directly) produce identical results for success responses, error responses, request construction, and error handler side effects (sudo, project-moved, auth 401).
Configure sentryCellFetch with default error handlers (sudo, auth, project-renamed) in both app entrypoints alongside existing setApiNavigate calls, sharing the same navigate instance.
github-actions
Bot
added
the
Scope: Frontend
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 139b580. Configure here.
|
|
||
| // 4. Generic catch-all | ||
| if (errorHandlers?.onError?.(responseMeta, options)) { | ||
| return {headers: buildResponseHeaders(response), json: undefined as unknown}; |
There was a problem hiding this comment.
Suppressed errors resolve as success
Medium Severity
When onAuthError, onProjectRenamed, or onError suppress a failed response, handleErrorResponse resolves with { json: undefined } instead of rejecting or staying pending. Used as a React Query queryFn, that marks the query successful and can cache undefined, skip retries, and render empty “loaded” UI if navigation is delayed or incomplete.
Reviewed by Cursor Bugbot for commit 139b580. Configure here.
| ) { | ||
| return errorHandlers.onSudoRequired(responseMeta, () => | ||
| executeFetch(fullUrl, options, undefined) | ||
| ); |
There was a problem hiding this comment.
Sudo retry ignores abort signal
Low Severity
The sudo retry callback re-invokes executeFetch with signal set to undefined, so a React Query cancellation on the original context.signal does not abort the retried request after the sudo modal completes.
Reviewed by Cursor Bugbot for commit 139b580. Configure here.
![semgrep-code-getsentry[bot]](https://avatars.githubusercontent.com/in/1153895?s=60&v=4){kind=small}
| } | ||
|
|
||
| if (!isDemoModeActive()) { | ||
| Cookies.set('session_expired', '1'); |
There was a problem hiding this comment.
High severity and reachable issue identified in your code:
Line 66 has a vulnerable usage of js-cookie, introducing a high severity vulnerability.
ℹ️ Why this is reachable
A reachable issue is a real security risk because your project actually executes the vulnerable code. This issue is reachable because your code uses a certain version of js-cookie.
Affected versions of js-cookie are vulnerable to Improperly Controlled Modification of Object Prototype Attributes ('Prototype Pollution'). The internal assign() helper in js-cookie walks attribute objects with a for..in loop and writes properties directly onto the target, which lets a __proto__ key from a JSON-parsed source trigger the Object.prototype.__proto__ setter. An attacker who controls any attributes object passed to set, remove, withAttributes, or withConverter can inject cookie attributes (domain, path, secure, samesite, expires) and pull off session fixation or downgrade Secure/SameSite protections.
References: GHSA
To resolve this comment:
Upgrade this dependency to at least version 3.0.7 at pnpm-lock.yaml.
💬 Ignore this finding
To ignore this, reply with:
/fp <comment>for false positive/ar <comment>for acceptable risk/other <comment>for all other reasons
You can view more details on this finding in the Semgrep AppSec Platform here.
📊 Type Coverage Diff
🔍 12 new type safety issues introduced
Type assertions (
This is informational only and does not block the PR. |
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
| body = JSON.stringify(options.data); | ||
| } | ||
|
|
||
| const baseHeaders = currentConfig.headers ?? JSON_HEADERS; |
There was a problem hiding this comment.
Bug: Calling configureSentryCellFetch with custom headers replaces the default JSON_HEADERS instead of merging them, which can silently drop required Content-Type and Accept headers.
Severity: LOW
Suggested Fix
The configureSentryCellFetch function should merge the provided newConfig.headers with the default JSON_HEADERS instead of replacing them. Alternatively, add a development-time warning or documentation to clarify that callers must include all required JSON headers when overriding.
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: static/app/utils/api/sentryCellFetch.tsx#L126
Potential issue: The `configureSentryCellFetch` function allows custom headers to
completely replace the default `JSON_HEADERS`. If a developer configures it with partial
headers (e.g., only an `Authorization` header), the `Content-Type` and `Accept` headers
required for JSON API communication will be silently dropped. This happens because
`currentConfig.headers` is used in place of `JSON_HEADERS` if it's provided, rather than
being merged. While current code does not trigger this, it is a latent design issue that
could lead to broken API requests in the future.
Did we get this right? 👍 / 👎 to inform future reviews.
…lysis Document how Client.clear()/cancel() patterns (unmount cleanup, stale search cancellation, stale data fetch) are handled natively by TanStack Query via AbortSignal, and why the parity test intentionally skips this.
1 participant
Summary
sentryCellFetch— a new fetch function that owns thefetch()call directly and accepts injectable error handlers viaconfigureSentryCellFetch(), replacing the baked-in side effects inClient.request()(sudo modal, auth redirects, project-renamed redirects).fetchWithUrlhelper so callers can fetch by URL string without constructing a fullQueryFunctionContext.sentryCellFetchErrorHandlers.tsxwith factory functions (createDefaultAuthErrorHandler,createDefaultSudoHandler,createDefaultProjectRenamedHandler) that replicate the existing behavior fromapi.tsxbut are testable and overridable.configureSentryCellFetchin both app entrypoints (main.tsx,gsAdmin/init.tsx) alongside existingsetApiNavigatecalls.__mocks__/sentryCellFetch.tsx) that delegates toMockApiClientso existing test patterns work unchanged.Test plan
sentryCellFetch.spec.tsx— 17 tests covering success responses, error responses, request construction, error handler dispatch, and CSRF/credentialssentryCellFetchErrorHandlers.spec.tsx— 17 tests covering sudo modal, auth redirects, project-renamed, and edge casesfetchParity.spec.tsx— 38 tests running the same scenarios against bothapiFetch(old path) andsentryCellFetch(new path) to verify identical behavior for success, errors, request construction, and error handler side effectspnpm run typecheck)pnpm run lint:js)