Repository navigation
Conversation
![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 1 potential issue.
❌ 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 efe30f6. Configure here.
s1gr1d
left a comment
s1gr1d
left a comment
There was a problem hiding this comment.
Thank you so much for picking this up!
a small nit regarding the tests: they are all quite similar so you could use an it.each there, like this:
it.each([
['a relative path', 'https://app.example.com/dashboard?tab=billing', '/api/users/42', 'Failed to fetch (app.example.com)'],
])('resolves %s against location.href', async (_name, href, target, expected) => {}| /** Internal global with common properties and Sentry extensions */ | ||
| export type InternalGlobal = { | ||
| navigator?: { userAgent?: string; maxTouchPoints?: number }; | ||
| location?: { origin?: string }; |
There was a problem hiding this comment.
following the other comment
| location?: { origin?: string }; | |
| location?: { href?: string }; |
…nd an absolute URL on a different origin than the page
efe30f6 to
dc615b1
Compare
s1gr1d
changed the title
s1gr1d
left a comment
s1gr1d
left a comment
There was a problem hiding this comment.
Looks good now, thank you!
A few small things:
- Can you update the description so it mentions
href? - We should rename
Fixes #13168->Ref #13168. This PR covers the fetch error message, but the issue also mentions XHR and the empty URL in breadcrumbs, so we should keep that open
| it.each([ | ||
| ['a relative path', 'https://app.example.com/some/page?x=1', '/api/users', 'Failed to fetch (app.example.com)'], | ||
| ['an empty URL (`fetch("")`)', 'https://app.example.com/some/page?x=1', '', 'Failed to fetch (app.example.com)'], | ||
| ['a relative path with no current page', undefined, '/api/users', 'Failed to fetch'], | ||
| ['a relative path on a `file://` page', 'file:///C:/app/index.html', '/api/users', 'Failed to fetch'], | ||
| ['a relative path with an opaque origin', 'null', '/api/users', 'Failed to fetch'], | ||
| [ | ||
| 'an absolute URL, ignoring the current page', | ||
| 'https://app.example.com/some/page', | ||
| 'https://other.example.org/x', | ||
| 'Failed to fetch (other.example.org)', | ||
| ], |
There was a problem hiding this comment.
Thanks for the it.each!
About the "null" case the cursor bugbot flagged: Currently, there's not test for that, we could add those:
['a relative path in a sandboxed srcdoc iframe', 'about:srcdoc', '/api/users', 'Failed to fetch'],
['an absolute URL in a sandboxed srcdoc iframe', 'about:srcdoc', 'https://api.example.org/v1/users', 'Failed to fetch (api.example.org)'],This branch has not been deployed
2 participants
enhanceFetchErrorMessagesalready adds the host to generic fetch errors, soFailed to fetchbecomesFailed to fetch (example.com). The catch is that it rannew URL(url)with no base, and that throws for relative URLs like/api/fooor thefetch("")from the issue. The try/catch swallowed the error, so those requests never got the host.Now relative URLs are resolved against
location.originif there is one (the browser tracing code already passes the same thing asurlBase). Absolute URLs ignore the base, and where there's nolocation(Node, workers) nothing changes. I also skip the enhancement when the resolved host is empty, like onfile://pages, otherwise you'd getFailed to fetch ().locationis added toInternalGlobalfor the typing.I added six unit tests (relative URL, empty URL, no
location,file://origin, opaquenullorigin, absolute URL on another origin).This only touches the error message. The empty URL in the fetch breadcrumbs is a separate spot, so I left it out to discuss on the issue thread.
Fixes #13168