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: dont show survey on url change, only hide it #1761

Merged
merged 1 commit into from
Feb 23, 2025

Conversation

lucasheriques
Copy link
Contributor

Changes

showing a survey should be the responsibility only of showSurvey, we shouldn't really call it anywhere else

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!)

@lucasheriques lucasheriques added bump patch Bump patch version when this PR gets merged feature/surveys labels Feb 23, 2025
@lucasheriques lucasheriques self-assigned this Feb 23, 2025
@lucasheriques lucasheriques requested a review from a team as a code owner February 23, 2025 15:03
Copy link

vercel bot commented Feb 23, 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 23, 2025 3:08pm

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 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 to useHideSurveyOnURLChange 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({
}
Copy link
Contributor

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

Suggested change
}
const handleClick = () => {
setShowSurvey(!showSurvey)
}
addEventListener(widget, 'click', handleClick)
widget?.setAttribute('PHWidgetSurveyClickListener', 'true')
return () => {
widget?.removeEventListener('click', handleClick)
}
}

Copy link

Size Change: -48 B (0%)

Total Size: 3.32 MB

Filename Size Change
dist/all-external-dependencies.js 218 kB -6 B (0%)
dist/array.full.es5.js 270 kB -6 B (0%)
dist/array.full.js 373 kB -6 B (0%)
dist/array.full.no-external.js 372 kB -6 B (0%)
dist/module.full.js 373 kB -6 B (0%)
dist/module.full.no-external.js 372 kB -6 B (0%)
dist/surveys-preview.js 70.1 kB -6 B (-0.01%)
dist/surveys.js 74.9 kB -6 B (-0.01%)
ℹ️ View Unchanged
Filename Size
dist/array.js 183 kB
dist/array.no-external.js 181 kB
dist/customizations.full.js 14 kB
dist/dead-clicks-autocapture.js 14.5 kB
dist/exception-autocapture.js 9.51 kB
dist/external-scripts-loader.js 2.64 kB
dist/main.js 183 kB
dist/module.js 183 kB
dist/module.no-external.js 182 kB
dist/recorder-v2.js 115 kB
dist/recorder.js 115 kB
dist/tracing-headers.js 1.76 kB
dist/web-vitals.js 10.4 kB

compressed-size-action

@@ -1043,67 +1043,6 @@ describe('useToggleSurveyOnURLChange', () => {
expect(mockSetSurveyVisible).not.toHaveBeenCalled()
})

it('should show survey when URL changes to match conditions', () => {
Copy link
Member

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?

Copy link
Member

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?

Copy link
Contributor Author

@lucasheriques lucasheriques Feb 23, 2025

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

@lucasheriques lucasheriques merged commit df80f79 into main Feb 23, 2025
32 checks passed
@lucasheriques lucasheriques deleted the fix/dont-show-survey-on-url-change branch February 23, 2025 15:27
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 feature/surveys
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants