Skip to content

fix(browser): Instrument empty-URL XHR and show the page URL in breadcrumbs - #26201

Merged
s1gr1d merged 4 commits into
getsentry:developfrom
kunal649:kunal/fix/empty-url-xhr-and-breadcrumb
Nov 11, 2026
Merged

s1gr1d merged 4 commits into
getsentry:developfrom
kunal649:kunal/fix/empty-url-xhr-and-breadcrumb

Conversation

@kunal649

@kunal649 kunal649 commented Nov 7, 2026

Copy link
Copy Markdown
Contributor

Why:
fetch('') is instrumented, but xhr.open('GET', '') was not, and once an empty URL request is instrumented its breadcrumb shows url: "". This PR fixes both

  • xhr.ts bailed on !url, which treats an empty string like an unparseable argument, so empty-URL XHRs got no breadcrumb and no span. It now only bails on url === undefined, so XHR matches fetch.
  • The fetch and XHR breadcrumb handlers now show the page URL for an empty URL instead of "". Query and fragment are stripped with stripUrlQueryAndFragment, so params like ?token= from the page URL don't end up in breadcrumbs. Since an empty URL resolves to the page URL itself, the helper uses location.href directly instead of calling new URL() (bcoz its throws for hrefs like about:srcdoc). without a location object (e.g. workers), nothing changes.

Only empty URLs are rewritten, absolute and relative URLs keep their current breadcrumb behaviour.

How to review:
Start with:

  • packages/browser-utils/src/instrumentation/xhr.ts
  • packages/browser/src/integrations/breadcrumbs.ts

and the regression coverage in:

  • packages/browser-utils/test/instrumentation/xhr.test.ts
  • packages/browser/test/integrations/breadcrumbs.test.ts

The key things to check are that only empty URLs are rewritten, and that the query and fragment are stripped from the page URL in all three breadcrumb paths (fetch, failed fetch, XHR).

Unsure about:
Whether relative URLs (like /api/foo?x=1) should also be resolved against location.href and stripped in breadcrumbs. I kept this limited to empty URLs so the change stays scoped, but yeah I'm happy to widen it.

Ref #13168

@kunal649
kunal649 requested a review from a team as a code owner November 7, 2026 13:33
@kunal649
kunal649 requested review from Lms24 and msonnb and removed request for a team November 7, 2026 13:33
@github-actions github-actions Bot added the external PR from an external contributor label Nov 7, 2026

@s1gr1d s1gr1d left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, the XHR fix looks good!

Regarding your "Unsure about": let's keep it to empty URLs

Comment on lines +328 to +331
function getBreadcrumbUrl(url: string): string {
const pageurl = url === '' ? WINDOW.location?.href : undefined;
return filterCollectedUrl(pageurl ? stripUrlQueryAndFragment(pageurl) : url);
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

filterCollectedUrl already filters sensitive params (e.g. token=[Filtered]) based on dataCollection.urlQueryParams.

We could only drop the fragment and let filterCollectedUrl handle the rest.

Suggested change
function getBreadcrumbUrl(url: string): string {
const pageurl = url === '' ? WINDOW.location?.href : undefined;
return filterCollectedUrl(pageurl ? stripUrlQueryAndFragment(pageurl) : url);
}
function getBreadcrumbUrl(url: string): string {
const pageurl = url === '' ? WINDOW.location?.href : undefined;
return filterCollectedUrl(pageUrl ? pageUrl.split('#')[0]! : url, client);
}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

okay!

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some test nits:

  • consider using toHaveBeenCalledWith instead of mock.calls
  • restoreAllMocks instead of resetAllMocks, and unset the client after each test

@s1gr1d s1gr1d self-assigned this Nov 10, 2026
@kunal649
kunal649 force-pushed the kunal/fix/empty-url-xhr-and-breadcrumb branch from 6206414 to 149a4b3 Compare November 10, 2026 15:09
@kunal649
kunal649 requested a review from a team November 10, 2026 15:09

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes and found 2 potential issues.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Want reviews to match your repository better? Bugbot Learning can learn team-specific rules from PR activity. A team admin can enable Learning in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 149a4b3. Configure here.

@s1gr1d
s1gr1d enabled auto-merge (squash) November 10, 2026 15:54
auto-merge was automatically disabled November 10, 2026 16:00

Head branch was pushed to by a user without write access

@kunal649

Copy link
Copy Markdown
Contributor Author

your bugbot scares me off @s1gr1d

@s1gr1d
s1gr1d merged commit 15f3d8d into getsentry:develop Nov 11, 2026
272 checks passed
JPeer264 pushed a commit that referenced this pull request Nov 11, 2026
This PR adds the external contributor to the CHANGELOG.md file, so that
they are credited for their contribution. See #26201

Co-authored-by: s1gr1d <32902192+s1gr1d@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

external PR from an external contributor

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants