Add survey support for React Native #333
Conversation
|
@ian-craig thanks a lot. |
|
@marandaneto any chance you've had some time to take a look at this? Thanks! |
hello, yes, I started looking into it already, expect a thoughtful review today or Monday latest =) sorry the delay! |
| flex: 1, | ||
| alignItems: 'center', | ||
| justifyContent: 'flex-end', | ||
| marginVertical: 40, // TODO Plus safe area? |
There was a problem hiding this comment.
Okay I've added an "Optional" dependency on safe area insets like you've done with other libraries, but I'm not sure how I feel about it in this case.
The trouble is we're also dependent on the developer having a <SafeAreaProvider> outside where they put the <PostHogProvider>. I've put a fallback to some default insets if not, but it still seems like a hidden gotcha.
There was a problem hiding this comment.
we can document/add to code snippets, most people will just copy-pasta anyway
| // Load surveys once | ||
| useEffect(() => { | ||
| posthog | ||
| .fetchSurveys() | ||
| .then(setSurveys) | ||
| .catch((error: unknown) => { | ||
| posthog.capture('PostHogSurveyProvider failed to fetch surveys', { error }) | ||
| }) | ||
| }, [posthog]) |
There was a problem hiding this comment.
This will likely change since we are adding a remote config API (WIP), and we'll only load surveys if there are surveys to be loaded.
That means we'll be calling another API when the SDK starts.
There was a problem hiding this comment.
I've moved the fetching of surveys to Core. If I understand this comment correctly it sounds like it'd be best to check this remote config, and possibly cache the fetched surveys to prevent re-fetch, in core as well later on.
I think this surveys state would still be needed though as it's allowing everything else in this provider to update once the surveys finish loading.
There was a problem hiding this comment.
yes, i will do the remote config next week so we can cherry pick from my changes
| @@ -22,6 +22,7 @@ import { | |||
| } from './types' | |||
| import { withReactNativeNavigation } from './frameworks/wix-navigation' | |||
| import { OptionalReactNativeSessionReplay } from './optional/OptionalSessionReplay' | |||
| import { Survey, SurveyResponse } from '../../posthog-core/src/posthog-surveys-types' | |||
There was a problem hiding this comment.
TODO Remove this import now the fetch method has moved to core
posthog-react-native/package.json
Outdated
| @@ -33,6 +33,7 @@ | |||
| "react-native": "^0.69.1", | |||
| "react-native-device-info": "^10.3.0", | |||
| "react-native-navigation": "^6.0.0", | |||
| "react-native-svg": "^15.2.0", | |||
| "posthog-react-native-session-replay": "^0.1" | |||
There was a problem hiding this comment.
TODO add peer dependency for safe area context
|
@ian-craig merging it to the remote feat/surveys branch so I can merge my things within that branch too. |
![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 adds comprehensive survey support to posthog-react-native by porting functionality from posthog-js, introducing a new survey provider component and related hooks for managing survey display and interactions.
- Added
posthog-core/src/posthog-surveys-types.tswith core survey type definitions to fix TypeScript module generation issues - Added
PostHogSurveyProvidercomponent in/posthog-react-native/src/surveys/PostHogSurveyProvider.tsxfor survey state management and display - Added survey components in
/posthog-react-native/src/surveys/components/for rendering different question types and survey UI - Added survey storage and activation hooks in
/posthog-react-native/src/surveys/for managing survey state and persistence - Potential issue: The start_date check in
getActiveMatchingSurveys.tsappears incorrect as it returns false when start_date exists
💡 (1/5) You can manually trigger the bot by mentioning @greptileai in a comment!
22 file(s) reviewed, 47 comment(s)
Edit PR Review Bot Settings | Greptile
|
|
||
| interface ResponseBasedBranching { | ||
| type: SurveyQuestionBranchingType.ResponseBased | ||
| responseValues: Record<string, any> |
There was a problem hiding this comment.
style: responseValues type is too permissive with Record<string, any>. Consider using a more specific type to ensure type safety
| /** @default StringMatching.Exact */ | ||
| text_matching?: ActionStepStringMatching | null | ||
| href?: string | null | ||
| /** @default ActionStepStringMatching.Exact */ | ||
| href_matching?: ActionStepStringMatching | null |
There was a problem hiding this comment.
style: text_matching and href_matching have default values in comments but these aren't enforced by types. Consider using default type parameters
| const response = await this.fetchWithRetry(url, fetchOptions) | ||
| .then((response) => response.json() as Promise<SurveyResponse>) | ||
| .catch((error) => { | ||
| this._events.emit('error', error) | ||
| return { surveys: [] } | ||
| }) |
There was a problem hiding this comment.
logic: error handling returns empty surveys array but doesn't retry failed requests like other API calls. Consider using fetchWithRetry with retry options for consistency.
| try { | ||
| // eslint-disable-next-line @typescript-eslint/no-var-requires | ||
| OptionalRNSafeArea = require('react-native-safe-area-context') | ||
| } catch (e) {} |
There was a problem hiding this comment.
style: empty catch block silently ignores errors that could be useful for debugging
| "react-native-safe-area-context": { | ||
| "optional": true | ||
| } | ||
| } |
There was a problem hiding this comment.
logic: react-native-svg is missing from peerDependenciesMeta but is listed as an optional peer dependency. Add it to maintain consistency
| "react-native-safe-area-context": { | |
| "optional": true | |
| } | |
| } | |
| "react-native-safe-area-context": { | |
| "optional": true | |
| }, | |
| "react-native-svg": { | |
| "optional": true | |
| } | |
| } |
| survey.questions.forEach((question, idx) => { | ||
| question.originalQuestionIndex = idx | ||
| }) |
There was a problem hiding this comment.
logic: Mutating the survey object directly by adding originalQuestionIndex could cause issues if the same survey is used multiple times. Consider creating a new array with the indices.
| posthogStorage.ready().then(() => { | ||
| const lastSeenSurveyDate = posthogStorage.getPersistedProperty(PostHogPersistedProperty.SurveyLastSeenDate) | ||
| if (typeof lastSeenSurveyDate === 'string') { | ||
| setLastSeenSurveyDate(new Date(lastSeenSurveyDate)) | ||
| } | ||
|
|
||
| const serialisedSeenSurveys = posthogStorage.getPersistedProperty(PostHogPersistedProperty.SurveysSeen) | ||
| if (typeof serialisedSeenSurveys === 'string') { | ||
| const parsedSeenSurveys: unknown = JSON.parse(serialisedSeenSurveys) | ||
| if (Array.isArray(parsedSeenSurveys) && typeof parsedSeenSurveys[0] === 'string') { | ||
| setSeenSurveys(parsedSeenSurveys) | ||
| } | ||
| } | ||
| }) |
There was a problem hiding this comment.
style: no error handling for failed storage operations or invalid JSON
| question.choices.push(openEndedChoice) | ||
| shuffledOptions.push(openEndedChoice) |
There was a problem hiding this comment.
logic: Mutating question.choices directly here could cause side effects. The choices array should be cloned before modification.
| const lastSeenSurveyDate = posthogStorage.getPersistedProperty(PostHogPersistedProperty.SurveyLastSeenDate) | ||
| if (typeof lastSeenSurveyDate === 'string') { | ||
| setLastSeenSurveyDate(new Date(lastSeenSurveyDate)) | ||
| } |
There was a problem hiding this comment.
style: check if lastSeenSurveyDate is a valid date string before creating Date object
| greenyellow: '#adff2f', | ||
| honeydew: '#f0fff0', | ||
| hotpink: '#ff69b4', | ||
| 'indianred ': '#cd5c5c', |
There was a problem hiding this comment.
syntax: The key 'indianred ' has a trailing space which could cause lookup issues. Remove the space from the key.
| 'indianred ': '#cd5c5c', | |
| 'indianred': '#cd5c5c', |
Problem
#110 posthog-react-native doesn't have built in support for surveys
Changes
This PR ports most of the survey logic from posthog-js into react native.
I've re-implemented most of the state handling and persistence to make it more appropriate for React Native and built on top of posthog-react-native.
See surveys/Readme.md for details on the hooks and config options I've added. I was originally going to post this as a separate library to start which is why I've split out
<PostHogSurveyProvider>etc., but we can probably merge some of that in to the main provider.Release info Sub-libraries affected
Bump level
Should all be backwards compatible, with the possible exception of adding a peer dependency for SVGs. We can look into what the best approach for this is.
Libraries affected
Changelog notes