P-1925 P-1926 Enhance analytics API with optional parameters and referral config#2
Conversation
… parity - Make signature() chainId optional to match web SDK (chain-agnostic signatures) - Add function_name and function_args to transaction() for contract call tracking - Add optional category parameter to screen() matching web page() API - Consolidate FormoAnalyticsProviderProps into single complete type - Replace require() with static import in setTrafficSourceFromUrl - Add errorHandler option to Options for global error handling - Add ReferralOptions for configurable referral query parameter parsing - Update tests for new optional chainId behavior https://claude.ai/code/session_0117WwRELH1nzVbxRj7Kmm5N
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This PR enhances the Formo Analytics SDK with improved flexibility for tracking events and managing referral sources. It makes several previously required parameters optional, adds support for custom referral query parameter configuration, and improves error handling. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
The screen() signature changed to screen(name, category?, properties?, ...), so the test needs to pass undefined for category before the properties object. https://claude.ai/code/session_0117WwRELH1nzVbxRj7Kmm5N
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces several enhancements to the analytics SDK. Key changes include making chainId optional for signature events, adding an optional category parameter to screen events, and introducing function_name and function_args for transaction events. An errorHandler option has been added for global error handling, and the traffic source parsing now supports custom referral query parameters. The review suggests an improvement in parseTrafficSource to use a Set for merging default and custom referral parameters, which would enhance clarity and prevent redundant checks.
| const refParams = customRefParams | ||
| ? [...defaultRefParams, ...customRefParams] | ||
| : defaultRefParams; |
There was a problem hiding this comment.
To improve clarity and prevent iterating over duplicate referral parameter names, you can use a Set to merge the default and custom parameters. This ensures each parameter is unique and avoids redundant checks in the loop that follows.
| const refParams = customRefParams | |
| ? [...defaultRefParams, ...customRefParams] | |
| : defaultRefParams; | |
| const refParams = customRefParams | |
| ? [...new Set([...defaultRefParams, ...customRefParams])] | |
| : defaultRefParams; |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ff8c13a7dd
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| public async screen( | ||
| name: string, | ||
| category?: string, | ||
| properties?: IFormoEventProperties, |
There was a problem hiding this comment.
Changing screen to screen(name, category?, properties?, context?, callback?) shifts every existing positional call by one slot. Calls like screen('Home', { section: 'featured' }) now pass the properties object as category, so custom properties/context are dropped or misaligned and a non-string category can be sent in the payload. This is a silent analytics regression for existing JS consumers and a breaking API change for TS users that previously used screen(name, properties, ...).
Useful? React with 👍 / 👎.
| public setTrafficSourceFromUrl(url: string): void { | ||
| const { parseTrafficSource, storeTrafficSource } = require("./utils/trafficSource"); | ||
| const trafficSource = parseTrafficSource(url); | ||
| const trafficSource = parseTrafficSource(url, this.options.referral?.queryParams); |
There was a problem hiding this comment.
The new ReferralOptions type advertises pathPattern, but URL parsing only receives referral.queryParams here. As a result, configuring { referral: { pathPattern: ... } } has no effect and referral codes encoded in path segments are never extracted, which makes the public option misleading and breaks expected attribution behavior for path-based referral links.
Useful? React with 👍 / 👎.
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
| public setTrafficSourceFromUrl(url: string): void { | ||
| const { parseTrafficSource, storeTrafficSource } = require("./utils/trafficSource"); | ||
| const trafficSource = parseTrafficSource(url); | ||
| const trafficSource = parseTrafficSource(url, this.options.referral?.queryParams); |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
…ern, dedup refParams - Revert screen() to original (name, properties?, context?, callback?) signature to avoid breaking existing callers. Category is passed via properties.category. - Implement pathPattern support in parseTrafficSource so ReferralOptions.pathPattern actually extracts referral codes from URL paths via regex capture group. - Deduplicate referral query params using Set to avoid redundant lookups. https://claude.ai/code/session_0117WwRELH1nzVbxRj7Kmm5N
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: de6f846ea1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } catch (error) { | ||
| logger.error("Error tracking event:", error); | ||
| if (this.options.errorHandler) { | ||
| this.options.errorHandler(error instanceof Error ? error : new Error(String(error))); |
There was a problem hiding this comment.
The new global errorHandler is invoked from within the catch block, but it is not wrapped in its own try/catch. If a consumer-provided handler throws (for example due to a bug in their reporter), that exception escapes trackEvent and causes public calls like track()/signature() to reject even though tracking errors are otherwise handled internally. This turns telemetry failures into app-level failures; wrap the handler call and log secondary handler errors instead of letting them bubble.
Useful? React with 👍 / 👎.
… from bubbling A throwing errorHandler would cause trackEvent to reject, turning telemetry failures into app-level failures. Now secondary handler errors are logged instead of propagated. https://claude.ai/code/session_0117WwRELH1nzVbxRj7Kmm5N
Since this is v1.0.0 with no existing consumers, use the clean signature screen(name, category?, properties?, context?, callback?) instead of the properties.category workaround. Matches web SDK's page(category?, name?). https://claude.ai/code/session_0117WwRELH1nzVbxRj7Kmm5N
|
@codex review |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
There was a problem hiding this comment.
🟡 New referral and errorHandler options missing from optionsKey, preventing SDK re-initialization on change
The PR adds two new options to Options: referral (serializable object with queryParams and pathPattern) and errorHandler (function). Neither is included in the optionsKey computation in FormoAnalyticsProvider.tsx. The existing pattern extracts each option individually (lines 126-137) and includes serializable ones in serializableOptions (lines 141-152) and useMemo deps (line 160). For example, app (a similar serializable config object) is included. For functions, ready is tracked as hasReady: !!options?.ready. However, referral is not extracted or serialized, and errorHandler has no hasErrorHandler equivalent. If a consumer changes referral config or toggles errorHandler, the optionsKey won't change, the useEffect (line 162) won't re-run, and the SDK instance will retain stale options—meaning setTrafficSourceFromUrl() at src/FormoAnalytics.ts:180-184 would use outdated referral config.
(Refers to lines 140-160)
Prompt for agents
In src/FormoAnalyticsProvider.tsx, around line 125-160:
1. Extract the new options from the options object (after line 137):
const referral = options?.referral;
const hasErrorHandler = !!options?.errorHandler;
2. Add them to the serializableOptions object (around line 141-152):
Add referral and hasErrorHandler to the object.
3. Add them to the useMemo dependency array (line 160):
Add referral and hasErrorHandler to the dependency list.
This follows the existing pattern where serializable options (like app) are included directly, and function options (like ready) are tracked as boolean flags (hasReady).
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cdf7205c64
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| public async screen( | ||
| name: string, | ||
| category?: string, | ||
| properties?: IFormoEventProperties, |
There was a problem hiding this comment.
screen properties as second argument
Adding category before properties changes runtime behavior for existing integrations that call screen(name, properties) (including JS users without type checks). Calls like formo.screen('Profile', { userId: '123' }) now treat the object as category, drop properties, and emit malformed screen metadata, so upgrades silently lose custom screen properties. Please preserve backward compatibility with an overload/runtime argument check (or by appending category after existing optional args).
Useful? React with 👍 / 👎.
yosriady
changed the title
… backgrounded) Implements the Segment/RudderStack mobile lifecycle spec using a JS-side approach (no native modules required, works in Expo Go): - Application Installed: first launch (no stored version in AsyncStorage) - Application Updated: version or build changed from stored values - Application Opened: every cold start (from_background: false) and foreground return (from_background: true), includes deep link URL - Application Backgrounded: app goes to background Detection compares stored app version/build in AsyncStorage against current values from react-native-device-info, expo-application, or options.app config. Controlled via autocapture.lifecycle option (default: true). Adds AppLifecycleManager with its own AppState listener, separate from EventQueue's flush-on-background listener. https://claude.ai/code/session_0117WwRELH1nzVbxRj7Kmm5N
| if (previousVersion === null && previousBuild === null) { | ||
| // No stored version — first install | ||
| logger.info("AppLifecycleManager: Application Installed"); | ||
| await this.analytics.track("Application Installed", { |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
When AsyncStorage is not provided, the SDK falls back to MemoryStorage which is empty on every cold start. Without this guard, detectInstallOrUpdate would false-positive as "Application Installed" on every launch. Now checks if AsyncStorage adapter is actually available before comparing stored version/build. Skips install/update events (with warning) if only MemoryStorage is available. Application Opened/Backgrounded still fire. https://claude.ai/code/session_0117WwRELH1nzVbxRj7Kmm5N
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
StorageManager.getStorage("asyncStorage") silently falls back to
MemoryStorage when AsyncStorage hasn't been initialized, and
MemoryStorage.isAvailable() always returns true. This made the
lifecycle guard ineffective — install events would still fire on
every cold start without AsyncStorage.
Fix: Add StorageManager.hasPersistentStorage() that checks if an
initialized AsyncStorage adapter exists in the storages map, bypassing
the fallback logic in getStorage().
https://claude.ai/code/session_0117WwRELH1nzVbxRj7Kmm5N
| storage().set(LOCAL_APP_VERSION_KEY, version); | ||
| storage().set(LOCAL_APP_BUILD_KEY, build); |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
…install events storage().set() writes to the in-memory cache synchronously but persists to AsyncStorage asynchronously without being awaited. If the app crashes or is force-quit before the async write completes, the version data is lost and the next launch would fire a duplicate "Application Installed" event. Switch to awaiting setAsync() to ensure persistence completes. https://claude.ai/code/session_0117WwRELH1nzVbxRj7Kmm5N
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
A transient AsyncStorage write failure in lifecycle tracking (e.g., storage full, permission error) would reject the init() promise and leave the SDK uninitialized with no-op methods for the entire session. Lifecycle tracking is non-critical — it should not block the core analytics SDK from functioning. https://claude.ai/code/session_0117WwRELH1nzVbxRj7Kmm5N
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 67b07281b5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| name: string, | ||
| category?: string, | ||
| properties?: IFormoEventProperties, |
There was a problem hiding this comment.
Changing screen to accept category as the second positional argument breaks existing integrations that call screen(name, properties, ...): the properties object is now treated as category, so the intended screen properties are lost and an object-valued category is emitted instead. This is a silent analytics regression for JavaScript users (and TypeScript users compiled against the old signature), so the method should keep legacy argument handling or add new parameters without shifting existing positions.
Useful? React with 👍 / 👎.
| const stored = this.storages.get("asyncStorage"); | ||
| return stored !== undefined && stored.isAvailable(); |
There was a problem hiding this comment.
hasPersistentStorage() currently returns true whenever something is stored at the asyncStorage slot, but this slot can contain the memory fallback after getStorage("asyncStorage") fails availability checks. In that common path (no AsyncStorage provided), lifecycle code will think persistence exists and run install/update detection every launch, producing repeated false Application Installed/Application Updated analytics instead of skipping detection.
Useful? React with 👍 / 👎.
getStorage("asyncStorage") caches a MemoryStorage fallback at the
asyncStorage slot when AsyncStorage hasn't been initialized. The
previous check (stored !== undefined && stored.isAvailable()) would
pass for this cached MemoryStorage since MemoryStorage.isAvailable()
always returns true.
Use instanceof AsyncStorageAdapter to ensure we're checking the real
persistent adapter, not a MemoryStorage fallback cached at that slot.
https://claude.ai/code/session_0117WwRELH1nzVbxRj7Kmm5N
| to, | ||
| value, | ||
| ...(transactionHash && { transactionHash }), | ||
| ...(function_name && { function_name }), | ||
| ...(function_args && { function_args }), | ||
| ...properties, | ||
| }, | ||
| address, |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
The toSnakeCase utility recursively converts all object keys, which corrupts function_args values that represent smart contract ABI parameter names (e.g., "tokenId" becomes "token_id"). Extract function_args before conversion and re-attach after to preserve the original key casing. https://claude.ai/code/session_0117WwRELH1nzVbxRj7Kmm5N
Upgrade pnpm from 9.0.0 to 10.27.0 and add security settings per https://pnpm.io/supply-chain-security: 1. onlyBuiltDependencies: [] — block lifecycle scripts by default, only explicitly approved packages can run postinstall scripts 2. blockExoticSubdeps: true — prevent transitive deps from using git repos or direct tarball URLs 3. minimumReleaseAge: 1440 — wait 24h before installing newly published versions to avoid the malware exposure window 4. trustPolicy: no-downgrade — prevent installation if a package's trust level has decreased from previous releases Also removes package-lock.json (npm artifact) in favor of pnpm-lock.yaml. https://claude.ai/code/session_0117WwRELH1nzVbxRj7Kmm5N
2 participants
Summary
This PR enhances the Formo Analytics SDK with improved flexibility for tracking events and managing referral sources. It makes several previously required parameters optional, adds support for custom referral query parameter configuration, and improves error handling.
Key Changes
categoryparameter to thescreen()method for better event categorizationchainIdparameter optional insignature()method, removing the validation that rejected null/undefined/0 valuesfunction_nameandfunction_argsparameters totransaction()method for better smart contract interaction trackingReferralOptionsinterface allowing custom query parameter names for referral code extraction viaparseTrafficSource()errorHandlercallback in SDK options for centralized error managementasyncStorage,onReady, andonErrorprops from separate interface into mainFormoAnalyticsProviderPropsfor cleaner APIdata,to, andvalueoptional in transaction tracking (previously required)Implementation Details
parseTrafficSource()utility now accepts custom referral parameter names and merges them with default parameters (ref,referral,refcode,referrer_code)EventFactoryto handle all new optional parameters consistently across event generationhttps://claude.ai/code/session_0117WwRELH1nzVbxRj7Kmm5N