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: check url match twice because of potential race condition #1722

Merged

Conversation

lucasheriques
Copy link
Contributor

@lucasheriques lucasheriques commented Feb 7, 2025

Changes

Also checks if the URL condition is matching before actually rendering it.

This is to test wether or not there's some sort of race condition where the survey is rendered on the correct URL, but the URL changes, and before we process that, the survey is already rendered.

Instance event where it happened. Survey is set to not sohw up in this event, but it did.

...

Checklist

  • Tests for new code (see advice on the tests we use)
  • Accounted for the impact of any changes across different browsers
  • Accounted for backwards compatibility of any changes (no breaking changes in posthog-js!)

Sorry, something went wrong.

@lucasheriques lucasheriques added the bump patch Bump patch version when this PR gets merged label Feb 7, 2025
@lucasheriques lucasheriques self-assigned this Feb 7, 2025
@lucasheriques lucasheriques requested a review from a team as a code owner February 7, 2025 02:22
Copy link

vercel bot commented Feb 7, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
posthog-js ✅ Ready (Inspect) Visit Preview Feb 7, 2025 1:06pm

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

PR Summary

This PR adds a defensive URL matching check to prevent race conditions in survey rendering. Here's a concise summary of the key changes:

  • Added secondary URL match verification in handlePopoverSurvey method before rendering surveys
  • Prevents edge case where URL changes between initial survey matching and actual rendering
  • Ensures surveys only display when URL conditions are still valid at render time
  • Maintains backward compatibility while improving reliability of URL-based survey targeting

The change is focused and targeted at preventing incorrect survey displays due to timing issues between URL checks and rendering.

1 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings | Greptile

Copy link

github-actions bot commented Feb 7, 2025

Size Change: +33 B (0%)

Total Size: 3.3 MB

Filename Size Change
dist/all-external-dependencies.js 215 kB -1 B (0%)
dist/array.full.es5.js 267 kB +7 B (0%)
dist/array.full.js 370 kB +7 B (0%)
dist/array.full.no-external.js 369 kB +7 B (0%)
dist/module.full.js 370 kB +7 B (0%)
dist/module.full.no-external.js 369 kB +7 B (0%)
dist/surveys.js 72.4 kB -1 B (0%)
ℹ️ View Unchanged
Filename Size
dist/array.js 183 kB
dist/array.no-external.js 181 kB
dist/customizations.full.js 13.8 kB
dist/dead-clicks-autocapture.js 14.5 kB
dist/exception-autocapture.js 9.48 kB
dist/external-scripts-loader.js 2.64 kB
dist/main.js 183 kB
dist/module.js 183 kB
dist/module.no-external.js 181 kB
dist/recorder-v2.js 115 kB
dist/recorder.js 115 kB
dist/surveys-preview.js 69.4 kB
dist/tracing-headers.js 1.76 kB
dist/web-vitals.js 10.4 kB

compressed-size-action

@lucasheriques lucasheriques merged commit 57fe202 into main Feb 7, 2025
28 checks passed
@lucasheriques lucasheriques deleted the fix/check-url-match-twice-for-potential-race-condition branch February 7, 2025 13:29
lucasheriques added a commit that referenced this pull request Feb 13, 2025
#1722)"

This reverts commit 57fe202.
lucasheriques added a commit that referenced this pull request Feb 13, 2025
#1722)" (#1733)

This reverts commit 57fe202.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bump patch Bump patch version when this PR gets merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants