Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Not all URLs are controllable by sidecarUrl #524

Open
freeatnet opened this issue Sep 12, 2024 · 1 comment
Open

Not all URLs are controllable by sidecarUrl #524

freeatnet opened this issue Sep 12, 2024 · 1 comment
Assignees
Labels
good first issue Good for newcomers Type: Enhancement New feature or request

Comments

@freeatnet
Copy link

freeatnet commented Sep 12, 2024

Environment

  • @spotlightjs/spotlight: 2.4.1
  • A Next.js 14.2.7 app running over an ngrok tunnel

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 BYK self-assigned this Sep 12, 2024
@BYK BYK added the Type: Enhancement New feature or request label Sep 12, 2024
@BYK
Copy link
Member

BYK commented Sep 12, 2024

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?

@BYK BYK added good first issue Good for newcomers up-for-grabs A free for all! labels Sep 13, 2024
@BYK BYK removed their assignment Sep 13, 2024
@BYK BYK assigned BYK and Shubhdeep12 and unassigned BYK Nov 9, 2024
@BYK BYK removed the up-for-grabs A free for all! label Nov 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers Type: Enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants