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: make callback on confirmation message instead #1845

Merged
merged 6 commits into from
Mar 24, 2025

Conversation

lucasheriques
Copy link
Contributor

@lucasheriques lucasheriques commented Mar 21, 2025

Changes

after I fix I did for this issue, it introduced a bug for feedback surveys where the confirmation message is not shown for feedback survey since it's being closed in the same instant.

so, this PR does two things:

  • address it so the reset for the feedback widget popup happens when the survey is closed
  • creates an end to end test to make sure this error doesn't happen again

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.

Copy link

vercel bot commented Mar 21, 2025

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

Name Status Preview Updated (UTC)
posthog-js ✅ Ready (Inspect) Visit Preview Mar 24, 2025 1:31pm

Copy link

github-actions bot commented Mar 21, 2025

Size Change: -342 B (-0.01%)

Total Size: 3.63 MB

Filename Size Change
dist/all-external-dependencies.js 220 kB -23 B (-0.01%)
dist/array.full.es5.js 301 kB -58 B (-0.02%)
dist/array.full.js 381 kB -23 B (-0.01%)
dist/array.full.no-external.js 380 kB -23 B (-0.01%)
dist/array.js 188 kB -25 B (-0.01%)
dist/array.no-external.js 187 kB -25 B (-0.01%)
dist/main.js 189 kB -25 B (-0.01%)
dist/module.full.js 381 kB -23 B (-0.01%)
dist/module.full.no-external.js 380 kB -23 B (-0.01%)
dist/module.js 188 kB -25 B (-0.01%)
dist/module.no-external.js 187 kB -25 B (-0.01%)
dist/surveys-preview.js 71.3 kB -22 B (-0.03%)
dist/surveys.js 76.6 kB -22 B (-0.03%)
ℹ️ View Unchanged
Filename Size
dist/customizations.full.js 14.1 kB
dist/dead-clicks-autocapture.js 14.4 kB
dist/exception-autocapture.js 9.94 kB
dist/external-scripts-loader.js 2.75 kB
dist/posthog-recorder.js 211 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

@lucasheriques lucasheriques self-assigned this Mar 22, 2025
@lucasheriques lucasheriques added feature/surveys bump patch Bump patch version when this PR gets merged labels Mar 22, 2025
@lucasheriques lucasheriques marked this pull request as ready for review March 22, 2025 01:46
@lucasheriques lucasheriques requested a review from a team as a code owner March 22, 2025 01:46
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 fixes a bug where feedback survey confirmation messages were closing prematurely by moving the survey reset functionality to trigger on confirmation message close instead of survey submission.

  • Added new test in src/__tests__/extensions/surveys/survey-popup.test.tsx to verify onCloseConfirmationMessage callback behavior
  • Removed onPopupSurveySent from SurveyContext in src/extensions/surveys/surveys-utils.tsx in favor of confirmation message close handler
  • Added comprehensive end-to-end test in playwright/surveys/feedback-widget.spec.ts for multiple survey submissions with thank you messages
  • Improved accessibility in BottomSection.tsx and QuestionHeader.tsx with proper ARIA attributes

7 file(s) reviewed, 4 comment(s)
Edit PR Review Bot Settings | Greptile

previewPageIndex={mockSurvey.questions.length}
/>
)
const cancelButton2 = screen.getByRole('button', { name: 'Close survey', hidden: true })
Copy link
Contributor

Choose a reason for hiding this comment

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

logic: The button name 'Close survey' doesn't match the actual button text 'Close' defined in mockSurvey.appearance.thankYouMessageCloseButtonText

Suggested change
const cancelButton2 = screen.getByRole('button', { name: 'Close survey', hidden: true })
const cancelButton2 = screen.getByRole('button', { name: mockSurvey.appearance.thankYouMessageCloseButtonText, hidden: true })

// Click the cancel button
fireEvent.click(cancelButton2)
// Verify that onCloseConfirmationMessage was called
expect(mockOnCloseConfirmationMessage).toHaveBeenCalledTimes(1)
Copy link
Contributor

Choose a reason for hiding this comment

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

style: Should also verify mockRemoveSurveyFromFocus was called to ensure proper cleanup

@@ -31,6 +31,7 @@ export function BottomSection({
<button
className="form-submit"
disabled={submitDisabled}
aria-label="Submit survey"
Copy link
Member

Choose a reason for hiding this comment

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

was this missing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes

className="form-cancel"
onClick={onClick}
disabled={isPreviewMode}
aria-label="Close survey"
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

also missing

@lucasheriques lucasheriques merged commit 52cdc52 into main Mar 24, 2025
25 checks passed
@lucasheriques lucasheriques deleted the fix/make-callback-on-confirmation-message-instead branch March 24, 2025 13:50
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.

None yet

2 participants