Open
Conversation
Contributor
Semver Impact of This PR⚪ None (no version bump detected) 📋 Changelog PreviewThis is how your changes will appear in the changelog.
🤖 This preview updates automatically when you update the PR. |
Contributor
Instructions and example for changelogPlease add an entry to Example: ## Unreleased
### Features
- Auto tracing for expo-image and expo-asset ([#5718](https://github.com/getsentry/sentry-react-native/pull/5718))If none of the above apply, you can opt out of this check by adding |
alwx
changed the title
alwx
changed the title
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
![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.
Autofix Details
Bugbot Autofix prepared fixes for both issues found in the latest run.
- ✅ Fixed: Identical
describeUrlfunction duplicated across two new files- I extracted
describeUrlinto a sharedtracing/describeUrl.tsutility and updated both wrappers to import it.
- I extracted
- ✅ Fixed: Missing try/catch leaves spans unclosed on synchronous throw
- I wrapped each original wrapper call in
try/catchto set error status and end spans on synchronous throws, and added tests for these paths.
- I wrapped each original wrapper call in
Or push these changes by commenting:
@cursor push 9eed63c488
Preview (9eed63c488)
diff --git a/packages/core/src/js/tracing/describeUrl.ts b/packages/core/src/js/tracing/describeUrl.ts
new file mode 100644
--- /dev/null
+++ b/packages/core/src/js/tracing/describeUrl.ts
@@ -1,0 +1,14 @@
+/**
+ * Extracts a human-readable URL identifier for span names.
+ */
+export function describeUrl(url: string): string {
+ try {
+ // Remove query string and fragment
+ const withoutQuery = url.split('?')[0] || url;
+ const withoutFragment = withoutQuery.split('#')[0] || withoutQuery;
+ const filename = withoutFragment.split('/').pop();
+ return filename || url;
+ } catch {
+ return url;
+ }
+}
diff --git a/packages/core/src/js/tracing/expoAsset.ts b/packages/core/src/js/tracing/expoAsset.ts
--- a/packages/core/src/js/tracing/expoAsset.ts
+++ b/packages/core/src/js/tracing/expoAsset.ts
@@ -1,4 +1,5 @@
import { SPAN_STATUS_ERROR, SPAN_STATUS_OK, startInactiveSpan } from '@sentry/core';
+import { describeUrl } from './describeUrl';
import { SPAN_ORIGIN_AUTO_RESOURCE_EXPO_ASSET } from './origin';
/**
@@ -80,17 +81,23 @@
},
});
- return originalLoadAsync(moduleId)
- .then(result => {
- span?.setStatus({ code: SPAN_STATUS_OK });
- span?.end();
- return result;
- })
- .catch((error: unknown) => {
- span?.setStatus({ code: SPAN_STATUS_ERROR, message: String(error) });
- span?.end();
- throw error;
- });
+ try {
+ return originalLoadAsync(moduleId)
+ .then(result => {
+ span?.setStatus({ code: SPAN_STATUS_OK });
+ span?.end();
+ return result;
+ })
+ .catch((error: unknown) => {
+ span?.setStatus({ code: SPAN_STATUS_ERROR, message: String(error) });
+ span?.end();
+ throw error;
+ });
+ } catch (error) {
+ span?.setStatus({ code: SPAN_STATUS_ERROR, message: String(error) });
+ span?.end();
+ throw error;
+ }
}) as T['loadAsync'];
}
@@ -104,15 +111,3 @@
}
return `${moduleIds.length} assets`;
}
-
-function describeUrl(url: string): string {
- try {
- // Remove query string and fragment
- const withoutQuery = url.split('?')[0] || url;
- const withoutFragment = withoutQuery.split('#')[0] || withoutQuery;
- const filename = withoutFragment.split('/').pop();
- return filename || url;
- } catch {
- return url;
- }
-}
diff --git a/packages/core/src/js/tracing/expoImage.ts b/packages/core/src/js/tracing/expoImage.ts
--- a/packages/core/src/js/tracing/expoImage.ts
+++ b/packages/core/src/js/tracing/expoImage.ts
@@ -1,4 +1,5 @@
import { SPAN_STATUS_ERROR, SPAN_STATUS_OK, startInactiveSpan } from '@sentry/core';
+import { describeUrl } from './describeUrl';
import { SPAN_ORIGIN_AUTO_RESOURCE_EXPO_IMAGE } from './origin';
/**
@@ -105,17 +106,23 @@
},
});
- return originalPrefetch(urls, cachePolicyOrOptions)
- .then(result => {
- span?.setStatus({ code: SPAN_STATUS_OK });
- span?.end();
- return result;
- })
- .catch((error: unknown) => {
- span?.setStatus({ code: SPAN_STATUS_ERROR, message: String(error) });
- span?.end();
- throw error;
- });
+ try {
+ return originalPrefetch(urls, cachePolicyOrOptions)
+ .then(result => {
+ span?.setStatus({ code: SPAN_STATUS_OK });
+ span?.end();
+ return result;
+ })
+ .catch((error: unknown) => {
+ span?.setStatus({ code: SPAN_STATUS_ERROR, message: String(error) });
+ span?.end();
+ throw error;
+ });
+ } catch (error) {
+ span?.setStatus({ code: SPAN_STATUS_ERROR, message: String(error) });
+ span?.end();
+ throw error;
+ }
}) as T['prefetch'];
}
@@ -144,32 +151,26 @@
},
});
- return originalLoadAsync(source, options)
- .then(result => {
- span?.setStatus({ code: SPAN_STATUS_OK });
- span?.end();
- return result;
- })
- .catch((error: unknown) => {
- span?.setStatus({ code: SPAN_STATUS_ERROR, message: String(error) });
- span?.end();
- throw error;
- });
+ try {
+ return originalLoadAsync(source, options)
+ .then(result => {
+ span?.setStatus({ code: SPAN_STATUS_OK });
+ span?.end();
+ return result;
+ })
+ .catch((error: unknown) => {
+ span?.setStatus({ code: SPAN_STATUS_ERROR, message: String(error) });
+ span?.end();
+ throw error;
+ });
+ } catch (error) {
+ span?.setStatus({ code: SPAN_STATUS_ERROR, message: String(error) });
+ span?.end();
+ throw error;
+ }
}) as T['loadAsync'];
}
-function describeUrl(url: string): string {
- try {
- // Remove query string and fragment
- const withoutQuery = url.split('?')[0] || url;
- const withoutFragment = withoutQuery.split('#')[0] || withoutQuery;
- const filename = withoutFragment.split('/').pop();
- return filename || url;
- } catch {
- return url;
- }
-}
-
function describeSource(source: ExpoImageSource | string | number): string {
if (typeof source === 'number') {
return `asset #${source}`;
diff --git a/packages/core/test/tracing/expoAsset.test.ts b/packages/core/test/tracing/expoAsset.test.ts
--- a/packages/core/test/tracing/expoAsset.test.ts
+++ b/packages/core/test/tracing/expoAsset.test.ts
@@ -158,6 +158,26 @@
expect(mockSpan.end).toHaveBeenCalled();
});
+ it('ends the span when loadAsync throws synchronously', () => {
+ const error = new Error('Invalid module id');
+ const mockLoadAsync = jest.fn(() => {
+ throw error;
+ });
+ const assetClass = {
+ loadAsync: mockLoadAsync,
+ fromModule: jest.fn(),
+ } as unknown as ExpoAsset;
+
+ wrapExpoAsset(assetClass);
+
+ expect(() => assetClass.loadAsync(99)).toThrow('Invalid module id');
+ expect(mockSpan.setStatus).toHaveBeenCalledWith({
+ code: SPAN_STATUS_ERROR,
+ message: 'Error: Invalid module id',
+ });
+ expect(mockSpan.end).toHaveBeenCalled();
+ });
+
it('passes the original moduleId argument through', async () => {
const mockLoadAsync = jest.fn().mockResolvedValue([]);
const assetClass = {
diff --git a/packages/core/test/tracing/expoImage.test.ts b/packages/core/test/tracing/expoImage.test.ts
--- a/packages/core/test/tracing/expoImage.test.ts
+++ b/packages/core/test/tracing/expoImage.test.ts
@@ -109,6 +109,23 @@
expect(mockSpan.end).toHaveBeenCalled();
});
+ it('ends the span when prefetch throws synchronously', () => {
+ const error = new Error('Invalid prefetch input');
+ const mockPrefetch = jest.fn(() => {
+ throw error;
+ });
+ const imageClass = { prefetch: mockPrefetch, loadAsync: jest.fn() } as unknown as ExpoImage;
+
+ wrapExpoImage(imageClass);
+
+ expect(() => imageClass.prefetch('https://example.com/image.png')).toThrow('Invalid prefetch input');
+ expect(mockSpan.setStatus).toHaveBeenCalledWith({
+ code: SPAN_STATUS_ERROR,
+ message: 'Error: Invalid prefetch input',
+ });
+ expect(mockSpan.end).toHaveBeenCalled();
+ });
+
it('handles URL without path correctly', async () => {
const mockPrefetch = jest.fn().mockResolvedValue(true);
const imageClass = { prefetch: mockPrefetch, loadAsync: jest.fn() } as unknown as ExpoImage;
@@ -230,6 +247,23 @@
expect(mockSpan.end).toHaveBeenCalled();
});
+ it('ends the span when loadAsync throws synchronously', () => {
+ const error = new Error('Invalid image source');
+ const mockLoadAsync = jest.fn(() => {
+ throw error;
+ });
+ const imageClass = { prefetch: jest.fn(), loadAsync: mockLoadAsync } as unknown as ExpoImage;
+
+ wrapExpoImage(imageClass);
+
+ expect(() => imageClass.loadAsync('https://example.com/broken.png')).toThrow('Invalid image source');
+ expect(mockSpan.setStatus).toHaveBeenCalledWith({
+ code: SPAN_STATUS_ERROR,
+ message: 'Error: Invalid image source',
+ });
+ expect(mockSpan.end).toHaveBeenCalled();
+ });
+
it('passes options through to original loadAsync', async () => {
const mockResult = { width: 100, height: 100, scale: 1, mediaType: null };
const mockLoadAsync = jest.fn().mockResolvedValue(mockResult);
Contributor
Author
|
@antonis @lucas-zimerman this one is ready now! |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
| attributes: { | ||
| 'sentry.origin': SPAN_ORIGIN_AUTO_RESOURCE_EXPO_IMAGE, | ||
| 'image.url_count': urlCount, | ||
| ...(urlCount === 1 ? { 'image.url': firstUrl } : undefined), |
There was a problem hiding this comment.
Full URL with query secrets stored in span attribute
Medium Severity
The image.url span attribute stores the raw, unmodified URL including query parameters, which may contain tokens or secrets. The span name is correctly sanitized via describeUrl (stripping query strings), but the image.url attribute is not. The test "does not leak query string for URL ending with trailing slash" only asserts on the name field, missing that image.url still contains ?token=SECRET. This inconsistency suggests an oversight — the same sanitization applied to name likely needs to apply to image.url, or the attribute needs gating behind sendDefaultPii.
Additional Locations (1)
Triggered by project rule: PR Review Guidelines for Cursor Bot
Contributor
There was a problem hiding this comment.
@alwx this seems valid. Let's fix or document the behavior
antonis
reviewed
Mar 9, 2026
| } from './tracing'; | ||
|
|
||
| export type { TimeToDisplayProps, ExpoRouter } from './tracing'; | ||
| export type { TimeToDisplayProps, ExpoRouter, ExpoImage, ExpoAsset, ExpoAssetInstance } from './tracing'; |
Contributor
There was a problem hiding this comment.
q: Do we need to export ExpoAssetInstance?
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
📢 Type of change
📜 Description
Fixes #5427
💡 Motivation and Context
💚 How did you test it?
📝 Checklist
sendDefaultPIIis enabled🔮 Next steps