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

fix: remove query params from notebooks parsing #25802

Merged
merged 4 commits into from
Oct 28, 2024

Conversation

daibhin
Copy link
Contributor

@daibhin daibhin commented Oct 24, 2024

Problem

First reported in https://posthoghelp.zendesk.com/agent/tickets/19758

Changes

  • If a user drags a link to a posthog node into a notebook it may contain query params
  • We cannot strip these query params because a node could need them to infer its attributes e.g. replay filters are in the URL params
  • It is only possible the remove the query params within the getAttributes handler

Copy link
Contributor

github-actions bot commented Oct 24, 2024

Size Change: 0 B

Total Size: 1.15 MB

ℹ️ View Unchanged
Filename Size
frontend/dist/toolbar.js 1.15 MB

compressed-size-action

@@ -15,6 +15,10 @@ export function reportNotebookNodeCreation(nodeType: string): void {
posthog.capture('notebook node created', { type: nodeType })
}

export function removeQueryParams(input: string): string {
Copy link
Member

Choose a reason for hiding this comment

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

i've no idea what input is coming in here... some of them were being forced to particular types like insight short id or number

should this be taking input: unknown
should we be converting to a URL before removing search params instead of splitting on question mark

only questions sorry 🙈

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Questions are good - they forced me to rethink it!

The basic premise is that we need to match the whole URL but was trying to strip anything after the "id" in each node

I instead made use of the fact that we know the format of each id and can codify it as a separate match group within the URL

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's hard to test the paste handling exactly because it's tightly coupled to TipTap but hopefully the regex tests at least show the intention here

Copy link
Member

@pauldambra pauldambra left a comment

Choose a reason for hiding this comment

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

that's way clearer to me 🙌

didn't test it 🙈

@daibhin daibhin merged commit 94aafdf into master Oct 28, 2024
94 checks passed
@daibhin daibhin deleted the dn-fix/remove-query-params branch October 28, 2024 10:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants