-
Notifications
You must be signed in to change notification settings - Fork 141
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: dont show survey on url change, only hide it #1761
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
There was a problem hiding this 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 modifies the survey visibility behavior in posthog-js to make it more predictable and controlled. Here's a concise summary of the key changes:
The PR changes how URL-based survey visibility is handled by:
- Renamed
useToggleSurveyOnURLChange
touseHideSurveyOnURLChange
to better reflect its focused responsibility - Removed functionality that automatically shows surveys when URLs match conditions
- Surveys can now only be hidden when URLs change to non-matching paths
- Survey display is now exclusively controlled by the
showSurvey
function
Key points about the changes:
- Prevents surveys from unexpectedly reappearing during navigation between matching URLs
- Maintains backwards compatibility while improving UX predictability
- Centralizes survey display logic in a single function
- Includes updated tests to verify the new behavior
The changes make survey visibility behavior more deterministic by ensuring surveys only appear through explicit calls to showSurvey
rather than URL-based triggers.
2 file(s) reviewed, 1 comment(s)
Edit PR Review Bot Settings | Greptile
@@ -880,7 +879,7 @@ export function FeedbackWidget({ | |||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logic: potential memory leak - missing cleanup function to remove click event listener
} | |
const handleClick = () => { | |
setShowSurvey(!showSurvey) | |
} | |
addEventListener(widget, 'click', handleClick) | |
widget?.setAttribute('PHWidgetSurveyClickListener', 'true') | |
return () => { | |
widget?.removeEventListener('click', handleClick) | |
} | |
} |
Size Change: -48 B (0%) Total Size: 3.32 MB
ℹ️ View Unchanged
|
@@ -1043,67 +1043,6 @@ describe('useToggleSurveyOnURLChange', () => { | |||
expect(mockSetSurveyVisible).not.toHaveBeenCalled() | |||
}) | |||
|
|||
it('should show survey when URL changes to match conditions', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if there was a test for it, I wonder if there was a reason for it and now we are changing it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there an issue caused by this code path so I can get better context?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this was a test i added on my last PR, however, i was doing some tests locally, and this is causing delayed surveys to overcome the delay and also skip the survey sent
event (because it was showing the survey directly)
that logic is handled by the showSurvey()
method
Changes
showing a survey should be the responsibility only of
showSurvey
, we shouldn't really call it anywhere elseChecklist