fix: refactor canRenderSurvey and move logic to SurveyManager #1897
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
Size Change: -4.05 kB (-0.11%) Total Size: 3.68 MB
ℹ️ View Unchanged
|
| @@ -552,74 +552,6 @@ describe('SurveyManager', () => { | |||
| }) | |||
| }) | |||
|
|
|||
| describe('canRenderSurvey', () => { | |||
There was a problem hiding this comment.
tests removed as I moved that method to posthog-surveys.test.ts to stop duplicating rendering logic
![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 refactors the survey eligibility logic in posthog-js by centralizing it in the posthog-surveys.ts file, eliminating duplication between getActiveMatchingSurveys and canRenderSurvey functions.
- Added new
checkSurveyEligibilitymethod inPostHogSurveysclass that serves as a single source of truth for determining survey eligibility - Moved survey condition matching logic from
extensions/surveys.tsxtoposthog-surveys.tswith helper methods like_isSurveyConditionMatchedand_internalFlagCheckSatisfied - Added utility functions in
survey-utils.tsincludingdoesSurveyMatchSelector,isSurveyRunning,doesSurveyActivateByEvent, anddoesSurveyActivateByAction - Enhanced logging in
survey-event-receiver.tsfor better debugging of survey activation events - Added comprehensive tests in
posthog-surveys.test.tsto verify the refactored eligibility logic works correctly
9 file(s) reviewed, 3 comment(s)
Edit PR Review Bot Settings | Greptile
Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
can we do it the other way around? so we get the refactoring gains without increasing the main bundle? |
lucasheriques
changed the title
|
@marandaneto @ioannisj I decided to move more business logic to the |
| @@ -14,6 +14,7 @@ import { | |||
| import { DeadClicksAutocapture, isDeadClicksEnabledForAutocapture } from './extensions/dead-clicks-autocapture' | |||
| import { ExceptionObserver } from './extensions/exception-autocapture' | |||
| import { errorToProperties } from './extensions/exception-autocapture/error-conversion' | |||
| import { HistoryAutocapture } from './extensions/history-autocapture' | |||
There was a problem hiding this comment.
Most of your PRs auto-format the imports, which adds noise to the PR review.
Can you double-check that your auto-format config is the same as everyone else's?
There was a problem hiding this comment.
it's caused by a setting I have on VS Code. I created a PR to prevent it from having effect in this repo
nice, left 2 comments otherwise LGTM |
Co-authored-by: Manoel Aranda Neto <[email protected]>
Changes
both
getActiveMatchingSurveysandcanRenderSurveyshare a lot of similarities around business logic. Since their goal is to get check if a survey is eligible for an user or not.However, we had duplicated business logic happening on both.
So, I decided to remove
canRenderSurveyfromposthog-surveys.tstoextenstions/surveys.tsx, to centralize the logic in one place and also reduce the main bundle size, by only downloading it if there are surveys enabled.I also did the same to
getActiveMatchingSurveys. No need to have this logic be onposthog-surveys.tssince it's only relevant if surveys are actually loaded.Checklist