chore: small readability refactor to getActiveMatchingSurveys

[lucasheriques]

Changes

small readability refactor for getActiveMatchingSurveys

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

[vercel]

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

Name	Status	Preview	Updated (UTC)
posthog-js	✅ Ready (Inspect)	Visit Preview	Feb 5, 2025 10:08pm

[greptile-apps]

PR Summary

This PR modifies the survey targeting logic in posthog-js by refactoring the getActiveMatchingSurveys method for improved readability and changing how internal targeting flags are evaluated.

• Changed internal targeting flag logic in src/posthog-surveys.ts to use OR instead of AND, allowing surveys to show if either override check passes or feature flag is enabled
• Extracted feature flag checks into new private method isSurveyFeatureFlagEnabled for better code organization
• Simplified event/action condition checks using nullish coalescing operator
• Reordered imports for better code structure

_{1 file(s) reviewed, 1 comment(s)
Edit PR Review Bot Settings | Greptile}

[greptile-apps]

logic: This changes the behavior of internal targeting flags. Previously required flag check if no override, now passes if either condition is true. May cause surveys to show more frequently than intended.

[lucasheriques]

this is incorrect

in the first version, we had:

When overrideInternalTargetingFlagCheck is true:

• First version: returns true (second part of the ternary)
• Second version: returns true (first part of OR condition)

When overrideInternalTargetingFlagCheck is false (i.e. we call isSurveyFeatureFlagEnabled):

If survey.internal_targeting_flag_key is null:

• First version: returns true (second part of the ternary)
• Second version: returns true (since isSurveyFeatureFlagEnabled returns true)

If survey.internal_targeting_flag_key is defined:

• First version: returns isFeatureEnabled(survey.internal_targeting_flag_key)
• Second version: returns isFeatureEnabled(survey.internal_targeting_flag_key)

[marandaneto]

do we have unit tests for that so the change will break if not correct?
one thing to pay attention to is that the former uses && and you use ||.
the other thing is that your description says: 'When overrideInternalTargetingFlagCheck is true', but the former does '!overrideInternalTargetingFlagCheck', so I'm not confident that it's the same without running the code, can you double check?

[lucasheriques]

yes, there are unit tests for getActiveMatchingSurveys here, which is why I relied on them to keep the same functionality

[github-actions]

Size Change: -2.15 kB (-0.07%)

Total Size: 3.3 MB

Filename	Size	Change
dist/array.full.es5.js	267 kB	-193 B (-0.07%)
dist/array.full.js	370 kB	-217 B (-0.06%)
dist/array.full.no-external.js	369 kB	-217 B (-0.06%)
dist/array.js	183 kB	-217 B (-0.12%)
dist/array.no-external.js	181 kB	-217 B (-0.12%)
dist/main.js	183 kB	-217 B (-0.12%)
dist/module.full.js	370 kB	-217 B (-0.06%)
dist/module.full.no-external.js	369 kB	-217 B (-0.06%)
dist/module.js	183 kB	-217 B (-0.12%)
dist/module.no-external.js	181 kB	-217 B (-0.12%)

ℹ️ View Unchanged

Filename	Size
dist/all-external-dependencies.js	215 kB
dist/customizations.full.js	13.8 kB
dist/dead-clicks-autocapture.js	14.5 kB
dist/exception-autocapture.js	9.48 kB
dist/external-scripts-loader.js	2.64 kB
dist/recorder-v2.js	115 kB
dist/recorder.js	115 kB
dist/surveys-preview.js	69.4 kB
dist/surveys.js	72.4 kB
dist/tracing-headers.js	1.76 kB
dist/web-vitals.js	10.4 kB

_{compressed-size-action}

[lucasheriques]

@pauldambra slowly killing off some bytes from surveys part 🙏

[ioannisj]

LG