fix(surveys): re-translate displayed surveys when user language changes - #4626
fix(surveys): re-translate displayed surveys when user language changes#4626nandinitiw wants to merge 6 commits into
Conversation
turnipdabeets
left a comment
There was a problem hiding this comment.
Thanks for reopening this and for the earlier rounds. Confirmed the diff here is identical to #4195.
A few things need a push before we can merge:
- Need to address merge conflict
- The end-to-end test you added now fails on this head. It passes in isolation but fails in the full file — main added tests to that describe and the ordering shifted.
- #4532 also introduced SurveyManager.dispose(), which is what posthog-surveys.ts actually calls on teardown — destroy() has no production caller anymore. Could you move the listener/subscription cleanup into dispose() so the languagechange listener and onFeatureFlags subscription don't leak on reset?
- All your commits need to be verified and signed to merge the PR.
I'll help run CI once the above is pushed.
| // surveys.tsx). Without destroying it, listeners from earlier tests' instances stay | ||
| // attached and fire on later tests' window.dispatchEvent(new Event('languagechange')), | ||
| // hitting a stale mockPostHog whose mocks may no longer exist. | ||
| surveyManager.destroy() |
There was a problem hiding this comment.
Confirmed with a run on this head: jest src/__tests__/extensions/surveys.test.ts fails with TypeError: this._posthog.get_property is not a function inside _onLanguageChange, while the same test passes on its own via -t "updates the rendered question text". It passed at the pre-merge head (92/92), so main's newer tests shifted the ordering. This afterEach only destroys whatever is in surveyManager at that moment — the nested describes reassign it (new SurveyManager(mockPH)) and orphan the outer instance, whose languagechange listener stays attached. I think we need to track every instance and destroy each one here.
| this._unsubscribeFeatureFlags = posthog.onFeatureFlags(() => this._onLanguageChange()) | ||
| } | ||
|
|
||
| public destroy(): void { |
There was a problem hiding this comment.
destroy() has no production caller — teardown goes through dispose(), which posthog-surveys.ts calls as this._surveyManager?.dispose?.(). dispose() landed in #4532 after this branch was written, so as it stands the languagechange listener and the onFeatureFlags subscription both survive a posthog.surveys.dispose(). I think we need to move this cleanup into dispose().
| } | ||
|
|
||
| private _onLanguageChange(): void { | ||
| if (isNull(this._surveyInFocus) || !this._surveyIsRendered) { |
There was a problem hiding this comment.
[question] _handleWidget never calls _addSurveyToFocus, so _surveyInFocus stays null for feedback_button surveys and this returns on the first line — and the render below builds a SurveyPopup rather than a FeedbackWidget. #4174 mentions widgets too; is popover-only the intended scope here?
| '@posthog/core': patch | ||
| --- | ||
|
|
||
| fix(surveys): re-translate displayed surveys when the user's language changes mid-session — handles both browser `languagechange` events and `posthog.identify()` calls that update the `language` person property. Also snapshots each question's displayed text and language at answer time (`questionSnapshots`), so `$survey_questions` and `$survey_language` reflect what the user actually saw rather than the language active when the event was sent. |
There was a problem hiding this comment.
[suggestion] "displayed surveys" reads as every survey type, but widgets aren't covered (see the _surveyInFocus gate). Could we name popovers explicitly so the changelog doesn't over-promise?
| fix(surveys): re-translate displayed surveys when the user's language changes mid-session — handles both browser `languagechange` events and `posthog.identify()` calls that update the `language` person property. Also snapshots each question's displayed text and language at answer time (`questionSnapshots`), so `$survey_questions` and `$survey_language` reflect what the user actually saw rather than the language active when the event was sent. | |
| Re-translate popover surveys when the display language changes while the survey is on screen, either from a browser `languagechange` event or from `identify()` updating the `language` person property. In-progress answers are preserved. `$survey_questions[].question` and `$survey_language` on `survey sent` / `survey dismissed` now report the text and language the user saw when they answered, not the language active when the event fired. Feedback-button (widget) surveys are unchanged. |
| $survey_questions: survey.questions.map((question: SurveyQuestionForResponses) => ({ | ||
| id: question.id, | ||
| question: question.question, | ||
| question: (question.id && questionSnapshots?.[question.id]) ?? question.question, |
There was a problem hiding this comment.
[nit] ?? only catches null/undefined, so a question with id: '' gives '' ?? question.question → ''. Undefined ids fall through fine. questionSnapshots?.[question.id ?? ''] ?? question.question would cover it, if empty ids are reachable.
When identify() sets a new 'language' person property, or the browser fires a languagechange event, any popover survey currently on screen stays in the old language until dismissed. This wires up both triggers to re-detect the resolved language and re-render in place when it differs from what was originally shown. Closes PostHog#4174
- Use addEventListener() wrapper (fixes CI lint rule) - Snapshot style/properties/isSurveyCompleted on initial render and restore them on language-change re-render, so NextToTrigger position and custom event properties are preserved - Add changeset entry - Update tests to no longer rely on renderPopover mock (now renders inline)
Prevents _onLanguageChange from rendering a survey early when it is queued with surveyPopupDelaySeconds > 0. The survey was gaining focus before the delay timer fired, so a concurrent languagechange event could bypass the delay and skip the eligibility recheck.
Ensures $survey_questions[].question and $survey_language in sent/dismissed events reflect the language the user saw when they answered, not whatever language is active at event-fire time after a mid-session switch. - buildSurveyResponseProperties accepts questionSnapshots (id → displayed question text) and prefers those over the current survey question text - InProgressSurveyState gains questionSnapshots, persisted to localStorage - onNextButtonClick snapshots currentQuestion.question before advancing - dismissedSurveyEvent prefers inProgressSurvey.surveyLanguage over the current display prop; falls back to the prop only when no in-progress state exists (dismissed before answering anything)
…g#4195 - Fix prettier failures in surveys.test.ts (pnpm lint:fix) - Regenerate terser-mangled-names.json for the 7 new private fields introduced by the language re-translation and answer-time snapshotting work (_currentLanguage, _languageChangeListener, _onLanguageChange, _surveyIsRendered, _surveyPopupProps, _unsubscribeFeatureFlags, and the pre-existing _surveyInFocus entry) - Add questionSnapshots to the two survey-popup.test.tsx assertions that pin the exact sendSurveyEvent call shape - Add @posthog/core to the changeset (questionSnapshots flows through buildSurveyResponseProperties there) and mention it in the description - Replace the internal-state-only language-change test with one that exercises the real render path: renders a popup via the unmocked handlePopoverSurvey, types an answer, fires a real languagechange event with navigator.language changed, and asserts both that the rendered question text updates and that the typed answer survives the re-render - Add an afterEach to destroy() the SurveyManager between tests in the SurveyManager describe block — without it, each test's construction left a 'languagechange' window listener attached, so later real window.dispatchEvent(new Event('languagechange')) calls (including the new test) hit stale instances whose mockPostHog no longer had a get_property mock, throwing during event dispatch
…istener leaks Addresses review feedback from turnipdabeets on PR PostHog#4626: - SurveyManager.destroy() had no production caller — PostHog#4532 introduced dispose(), which is what posthog-surveys.ts actually calls on teardown. Moved the languagechange listener removal and onFeatureFlags unsubscribe into dispose() and removed destroy(). - The real-render end-to-end test added for this PR passed in isolation but failed when the full file ran, because several other SurveyManager instances constructed elsewhere in the file were never disposed, leaving their 'languagechange' window listeners attached. Tracing it down surfaced two separate leaks: - Several nested describes/tests reassign the shared `surveyManager` variable to a new instance without disposing the previous one, so only the last-assigned instance ever got cleaned up. Added a createSurveyManager() helper that tracks every constructed instance so the outer afterEach disposes all of them. - `jest.clearAllMocks()` in the 'timeout management' describe left a jest.spyOn(global, 'clearTimeout') spy installed (only clearing its call history, not restoring the original), which went stale once fake timers were torn down and threw "clearTimeout is not defined" the next time something called clearTimeout - including SurveyManager.dispose(). Switched to jest.restoreAllMocks(). - generateSurveys() (survey display logic describe) constructs a real SurveyManager and never disposed it either; captured the returned manager and disposed it in a finally block.
2babe4b to
8bf867f
Compare
|
Thanks for the detailed review! Addressed the first three:
Full survey test suite (268 tests across |
Reopening #4195 as a fresh PR — the original was auto-closed by the stale bot after a week of no activity, not for cause. lucasheriques's last review was positive ("the re-translate path holds up") and all requested changes were already addressed; the GitHub reopen API is failing with an opaque validation error, so opening fresh from the same branch instead. Same commits, same diff.
What
When a survey popover is on screen, it was never re-translated if the user's language changed mid-session. This happened in two scenarios:
posthog.identify()/setPersonPropertiesForFlags()sets a newlanguageperson propertylanguagechangeevent (OS/browser language switch)The TODO comment in
survey-translations.tsalready called this out as a known gap.Also fixes a gap this surfaced:
$survey_questions[].questionand$survey_languagewere read at capture time, not answer time — if the language changed mid-survey, submitted events showed the new language's question text even for questions the user answered under the old language.How
In
SurveyManager:window.languagechangelistener in the constructorposthog.onFeatureFlags()(which fires afteridentify()reloads flags)_onLanguageChange(), which:surveyPopupDelaySeconds— being forced on screen early by a mid-delay reload, skipping the eligibility recheck the delay timeout does)_currentLanguageand re-renders the popover viarenderPopover()destroy()_currentLanguageis cleared when a survey is dismissedquestionSnapshots:onNextButtonClicknow snapshots the currently-displayed question text intoquestionSnapshots: Record<string, string>(question id → text) before advancing, and persists it toInProgressSurveyStatein localStorage.buildSurveyResponsePropertiesin@posthog/corenow accepts this optional param and prefers snapshot text over the current (possibly re-translated) survey question text, so$survey_questionsreflects what the user actually saw when they answered.$survey_languageondismissednow similarly prefers the in-progress language over the current display-time language.The iOS reference implementation in posthog-ios #686 follows the same pattern.
Tests
Unit tests covering:
_currentLanguageon dismisslanguagechangelistener ondestroy()questionSnapshotsrecorded and used in place of current question texthandlePopoverSurvey, types an answer, fires a reallanguagechangeevent, and asserts both that the rendered question text updates and that the typed answer survives the re-renderCloses #4174