fix: check url matching on URL change for widget-type surveys #1755
+603
−45
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
lucasheriques
added
bump patch
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Summary
This PR extracts URL matching logic into a reusable hook to fix widget-type surveys not properly hiding when URLs no longer match conditions, similar to a previous fix for popover surveys.
- Added
useToggleSurveyOnURLChangehook in/src/extensions/surveys.tsxto handle URL matching across different navigation types - Modified
doesSurveyUrlMatchin/src/posthog-surveys.tsto acceptPick<Survey, 'conditions'>for better type safety and reusability - Added comprehensive test coverage in
/src/__tests__/posthog-surveys.test.tsfor URL matching scenarios including exact, regex, and contains matches - Added tests in
/src/__tests__/extensions/surveys.test.tsfor browser navigation methods (pushState, replaceState, popstate) - Improved cleanup handling for event listeners and history state modifications
4 file(s) reviewed, 2 comment(s)
Edit PR Review Bot Settings | Greptile
Comment on lines
+132
to
+154
| it('should match using exact match type', () => { | ||
| mockWindowLocation('https://example.com') | ||
|
|
||
| const survey = { | ||
| conditions: { | ||
| url: 'https://example.com', | ||
| urlMatchType: 'exact' as const, | ||
| events: null, | ||
| actions: null, | ||
| }, | ||
| } | ||
| expect(doesSurveyUrlMatch(survey)).toBe(true) | ||
|
|
||
| const nonMatchingSurvey = { | ||
| conditions: { | ||
| url: 'https://example.com/path', | ||
| urlMatchType: 'exact' as const, | ||
| events: null, | ||
| actions: null, | ||
| }, | ||
| } | ||
| expect(doesSurveyUrlMatch(nonMatchingSurvey)).toBe(false) | ||
| }) |
There was a problem hiding this comment.
style: Consider adding test for exact matching with trailing slashes and query parameters
Comment on lines
+1073
to
+1074
| expect(mockSetSurveyVisible).not.toHaveBeenCalled() | ||
| expect(mockRemoveSurveyFromFocus).not.toHaveBeenCalled() |
There was a problem hiding this comment.
style: Initial mount visibility test may be flaky - should explicitly test initial visibility state matches URL condition
There was a problem hiding this comment.
it shouldn't, as this hook is only to trigger after URL changes - the initial filter is done by getActiveMatchingSurveys
lucasheriques
changed the title
|
Size Change: +2.12 kB (+0.06%) Total Size: 3.32 MB
ℹ️ View Unchanged
|
marandaneto
approved these changes
Feb 20, 2025
3 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Changes
we already did this fix for popover-type surveys here: #1732
however, we just got a ticket that the same is stil lhappening for widget-type surveys
so I extracted that new logic into a hook, create tests for it (and also for
doesSurveyUrlMatch), and then applied it to both casesChecklist