search

getsentry / spotlight

Your Universal Debug Toolbar
https://spotlightjs.com
Other
377 stars 11 forks source link

Not all URLs are controllable by `sidecarUrl` #524

Closed freeatnet closed 1 week ago

freeatnet commented 2 months ago

Environment

Steps to Reproduce

  1. Set up a Next.js app and reverse-proxy it through an ngrok tunnel for mobile development.
  2. Set up Spotlight as documented.
  3. Set up a Sidecar URL as follows:
    1. Update the Spotlight.init({ sidecarUrl: "https://my-tunnel.ngrok.io/_sidecar/stream" }), using a prefix in the path, because /stream is needed for an app route.
    2. In next.config.js, set up a rewrite: { source: "/_spotlight/stream", destination: "http://localhost:8969/stream" }
  4. Launch the app and use the overlay within the page.

Expected Result

All calls needed for the overlay are done through sidecarUrl (like with tunnelRoute in Sentry SDK).

Actual Result

The overlay causes 404s calling https://my-tunnel.ngrok.io/clear (breaking the "clear events" button sync) and https://my-tunnel.ngrok.io/contextlines.

Triage

At least two locations in the code assume that the origin is the only custom part of the sidecarUrl:

  1. packages/overlay/src/integrations/sentry/data/sentryDataCache.ts:49
  2. packages/overlay/src/App.tsx:140

My current workaround is to add /clear and /contextlines to the Next.js rewrites above. It'd be helpful if sidecarUrl acted as the base URL for all sidecar-related endpoints; however, this might require a breaking change in the config semantics.

P.S. Happy to contribute a patch if I can have some guidance on whether it's better to change the semantics of sidecarUrl or add another config variable.

BYK commented 2 months ago

Hey @freeatnet, thanks for the awesome report! We never expected sidecar to work with URL rewriting and under a path that is not the root. That said I see no good reason to keep it that way so I think your suggestion of only changing the final part of the URL for these other paths make sense.

Ideally the sidecarURL would not have had the /stream part but I don't really see a non-breaking way to change that and am not sure if it's worth the effort at this point.

So my proposal is, in those 2 places where we change the entire path portion of the URL, we take the path, rsplit it and only change the final bit. This should not change the current behavior while allowing your use case. Does that make sense?