Add Privy integration for multi-wallet user identity clustering#304
Add Privy integration for multi-wallet user identity clustering#304yosriady wants to merge 15 commits into
Conversation
…llets Make tagging every wallet linked to a Privy user under that user's DID a one-liner, so the 8-wallet case clusters into a single user end to end. - identifyPrivyUser(analytics, user, options?): new exported helper next to parsePrivyProperties. Loops the user's linked wallets and calls identify() for each, forwarding the previously-dropped per-wallet metadata (wallet_client, chain_type, is_embedded) alongside the shared profile properties, all tagged with userId: user.id. - Event attribution: only the active/connected wallet is promoted to the SDK's current address. identify() gains an optional setCurrentAddress flag (default true); non-active linked wallets are identified with it false so they no longer hijack which wallet later events attribute to. The active wallet is identified last; callers pass activeAddress (from useWallets()), otherwise the SDK falls back to embedded-first, attribute-last ordering. - Dedup: fold userId into the session identify key (address[:rdns][:userId]), so attaching a DID to a wallet that was already identified anonymously re-emits, while repeats with the same userId stay deduped. Backward compatible when userId is absent. - useIdentifyPrivyUser(user, options?): optional React binding that re-runs identifyPrivyUser when the Privy DID or linked-wallet set changes, covering login, linkWallet, and unlinkWallet. Exported from the React entry only so the core entry stays React-free. - Docs: new docs/PRIVY_INTEGRATION.md centered on the one-liner, with parsePrivyProperties kept documented for advanced use; README pointer added. - Tests: unit coverage for identifyPrivyUser ordering/metadata/attribution and userId-aware dedup, plus an end-to-end integration spec driving the real identify() (no hijack after a real connect; re-emit on DID attach). Verified against Privy docs: linked wallet addresses are available on the frontend via usePrivy() user.linkedAccounts; the active wallet comes from useWallets() (wallets[0]). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NVjnFuV1Bod4Vev9JYt2wq
…er defaults Follow-up to the identifyPrivyUser review. Tightens the attribution contract and the ergonomics; documents the deliberate scope boundaries. - setActive contract (was setCurrentAddress): the flag now gates the *whole* active identity — both currentAddress AND currentUserId/SESSION_USER_ID_KEY. Previously setCurrentAddress:false still let a non-active identify repoint the global user ID, so a caller could end up with the active wallet paired with a different, non-active user. Renamed to reflect it protects active attribution state, not just the address. - identifyPrivyUser: activeAddress now falls back to the SDK's existing currentAddress (e.g. from a prior wagmi connect) before the embedded-first heuristic, so callers that already track connects can drop the option. Exposed IFormoAnalytics.currentAddress (readonly) to support this. Walletless users are now logged instead of dropped silently. - Dedup storage: identified entries get their own cap (30, up from the shared 20), sized for many-wallet Privy users (up to ~2N entries across anon + DID) while staying under the ~4KB per-cookie limit; documented the FIFO-eviction consequence. - React hook + options: documented that options.properties is captured at first identify (session dedup means property-only changes don't re-emit) and that unlink is additive server-side (no unlink event). Extracted a shared isPrivyWalletAccount predicate so the hook's re-run key and parsePrivyProperties can't drift. - Tests: added integration coverage for the currentAddress attribution fallback and for setActive:false leaving the active (address, userId) pair untouched. Deferred (need a backend/event contract, out of scope for the one-liner): walletless userId-keyed identify, and an explicit wallet-unlink event. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NVjnFuV1Bod4Vev9JYt2wq
…by ordering Per review: revert the changes to the core identify() surface that the Privy helper doesn't strictly need, keeping the diff contained to the Privy module. - Removed the `setActive` parameter from identify() and the readonly `currentAddress` field from the IFormoAnalytics interface. The public identify() contract is now unchanged from main. - identifyPrivyUser now handles event attribution purely by ordering — it identifies the active/connected wallet last (embedded-first, then the active wallet, as a best-effort fallback), so identify()'s existing "last write wins" behavior lands attribution on the right wallet. This was the task's sanctioned approach and sidesteps the setActive contract concern entirely (there is no partial-attribution flag to get wrong). - Dropped the activeAddress -> currentAddress fallback (it required exposing currentAddress on the interface); callers pass activeAddress when they want precise attribution. - Reverted MAX_IDENTIFIED_ENTRIES back to the shared MAX_SESSION_ENTRIES (20); the dedup-cap bump was a general session-storage change, not required for the feature. Kept: the userId-in-session-dedup-key fix (required so attaching a DID to an already-identified wallet re-emits), the identifyPrivyUser/useIdentifyPrivyUser helpers, per-wallet metadata forwarding, and docs. Updated tests to assert ordering instead of the removed flag. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NVjnFuV1Bod4Vev9JYt2wq
Replace the useIdentifyPrivyUser hook with a flag on identify() so a Privy app
needs only a single function: identify(user, { privy: true }).
- identify() gains an overload: identify(user, { privy: true, activeAddress?,
properties? }). When the { privy: true } flag is present, identify() treats
the first argument as a Privy user and delegates to identifyPrivyUser, which
expands user.linkedAccounts into one identify per wallet under the DID. The
Privy-specific logic stays in the privy module; core identify() only
dispatches. A normal identify({ address }, properties) is unaffected (the flag
lives in the options position, and an address-shaped first arg never matches).
- Removed the useIdentifyPrivyUser React hook and src/privy/react.ts. Apps call
formo.identify(user, { privy: true }) from their own effect (as the with-privy
example already does) instead of mounting a hook.
- identifyPrivyUser and parsePrivyProperties stay exported; the flag is sugar
over identifyPrivyUser.
- Docs rewritten around the single-call form; added integration tests that the
flag dispatches through the real identify() and that a normal identify with a
`privy` property is not misinterpreted.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NVjnFuV1Bod4Vev9JYt2wq
…ptional
You can now call formo.identify(user, { privy: true }) with no address at all.
- When activeAddress is omitted, identifyPrivyUser now falls back to Privy's own
surfaced primary wallet (user.wallet) before the embedded-first heuristic, so
the caller doesn't need to derive or pass the active wallet.
- activeAddress stays as an optional override for callers who want to pin
attribution to a specific wallet (e.g. the live connected wallet from
useWallets()[0]?.address / wagmi account), which reflects the active wallet
more precisely than Privy's persisted user.wallet.
- Docs + example simplified to the no-argument form; added tests that user.wallet
is used as the default active wallet and that an explicit activeAddress
overrides it.
Note: Privy does not persist a live "active wallet" on the user object — that is
a runtime useWallets() concept the SDK can't see from outside React. user.wallet
is Privy's surfaced primary and a reasonable default; pass activeAddress when you
need the precise live wallet.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NVjnFuV1Bod4Vev9JYt2wq
|
@codex review |
![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: b3f8ad1b09
ℹ️ 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".
| if ( | ||
| propertiesOrOptions && | ||
| (propertiesOrOptions as { privy?: unknown }).privy === true | ||
| ) { |
There was a problem hiding this comment.
Because IFormoEventProperties is Record<string, unknown>, a normal identify call can legitimately pass a property named privy. In that case, e.g. formo.identify({ address, userId }, { privy: true }), this guard takes the Privy branch, casts the address-shaped params to PrivyUser, and identifyPrivyUser finds no linkedAccounts, so the identify event is skipped entirely instead of recording the wallet/user. Please distinguish the overload by also validating that the first argument is a Privy user, or otherwise avoid using an un-namespaced property key as the dispatch signal.
Useful? React with 👍 / 👎.
| const activeAddress = ( | ||
| options.activeAddress ?? user.wallet?.address | ||
| )?.toLowerCase(); |
There was a problem hiding this comment.
When callers use the documented one-liner without activeAddress, this treats user.wallet as the active wallet and moves it to the final identify() call. In sessions where the SDK already tracks a connected external wallet but Privy's user.wallet is a different first/primary wallet (commonly the embedded wallet), the later identify overwrites currentAddress, so subsequent events are attributed to the wrong wallet unless every integration passes activeAddress. Prefer preserving the already-active SDK address when it is linked, or requiring/passing the connected wallet instead of defaulting to user.wallet.
Useful? React with 👍 / 👎.
| const hasActive = | ||
| !!activeAddress && | ||
| wallets.some((w) => w.address.toLowerCase() === activeAddress); |
There was a problem hiding this comment.
When a Privy user has Solana linked wallets and the caller passes an activeAddress that differs only by case (or links two Base58 addresses that collide after lowercasing), this case-insensitive match accepts the wrong wallet even though Solana addresses are case-sensitive; the later ordering can then move/drop the wrong entry and leave attribution on the wrong address. Compare EVM addresses case-insensitively only after detecting they are EVM, and keep Solana comparisons exact.
Useful? React with 👍 / 👎.
| return rdns ? `${address}:${rdns}` : address; | ||
| const parts = [address]; | ||
| if (rdns) parts.push(rdns); | ||
| if (userId) parts.push(userId); |
There was a problem hiding this comment.
With the new userId component included raw in the identified-wallet key, any app that uses an external userId containing a comma breaks session dedup: markWalletIdentified writes the key into a comma-joined cookie, and isWalletIdentified later splits on commas, so the exact key is never found and the same identify event re-emits on every call. Encode/escape the key components or store the list as JSON before adding arbitrary user IDs.
Useful? React with 👍 / 👎.
- identify() Privy dispatch: require the first argument to be Privy-user-shaped
(string `id`, no `address`), not just `{ privy: true }`. A normal identify
that happens to carry a property named `privy` no longer misdispatches and
drops the event. (Codex P2)
- identifyPrivyUser: prefer the SDK's already-active currentAddress (a connected
wallet) over Privy's user.wallet when resolving attribution, so the
multi-wallet identify loop doesn't overwrite the connected wallet. The flag
form passes this.currentAddress through as the default. (Codex P2)
- identifyPrivyUser: compare wallet addresses with chain-appropriate casing —
case-insensitive for EVM (0x hex), exact for Solana/Base58 — so a mis-cased
Solana address can't match the wrong wallet. (Codex P2)
- session dedup: percent-encode key components before the comma-joined cookie,
so an external userId containing a comma can't corrupt the key and defeat
dedup. Address/RDNS encode to themselves, so existing keys still match. (Codex P3)
Added tests for each; 673 passing, lint + build clean.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NVjnFuV1Bod4Vev9JYt2wq
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ed0006a3fe
ℹ️ 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".
| await analytics.identify( | ||
| { address: wallet.address, userId: user.id }, | ||
| walletProperties | ||
| ); |
There was a problem hiding this comment.
When formo.identify(user, { privy: true }) runs while the SDK's current address is a connected wallet that is not in user.linkedAccounts (for example a wagmi wallet before it has been linked in Privy), the convenience path passes that address as activeAddress but activeWallet is undefined. This loop still calls normal identify() for every linked wallet, and each call updates currentAddress, so the unlinked connected wallet is overwritten by the last linked wallet and later events are attributed to the wrong address. Preserve/restore the original current address when no linked wallet matches the active address, or avoid setting current address for clustering-only identifies.
Useful? React with 👍 / 👎.
Follow-up to Codex review of the identify(user, { privy: true }) path. When the
SDK's current address is a connected wallet that is NOT in user.linkedAccounts
(e.g. a wagmi wallet not yet linked in Privy), the previous fix passed it as
activeAddress but no linked wallet matched, so the per-wallet loop still left
currentAddress on an arbitrary linked wallet.
- identifyPrivyUser now returns the address of the wallet it made active (the
one owning attribution), or undefined when it fell back to the heuristic
because no linked wallet matched the active address.
- The identify() Privy dispatch snapshots currentAddress before the sync and
restores it when identifyPrivyUser returns undefined, so a connected-but-
unlinked wallet keeps attribution while the linked wallets are still
identified for clustering. No public setActive flag reintroduced.
Added an integration test for the unlinked-connected-wallet case; 674 passing,
lint + build clean.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NVjnFuV1Bod4Vev9JYt2wq
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dbd5d3c9e0
ℹ️ 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".
| await analytics.identify( | ||
| { address: wallet.address, userId: user.id }, | ||
| walletProperties | ||
| ); |
There was a problem hiding this comment.
When a Privy user's active wallet is Solana after an EVM wallet/chain was already current, this call only passes address/userId into identify(). identify() updates currentAddress but leaves currentChainId untouched, so subsequent tracking, excludeChains, and the active-wallet cookie can pair the Solana address with the stale EVM chain id and either suppress or lose attribution. Since wallet.chainType is known here, clear or update the chain state when making a Solana wallet the active one.
Useful? React with 👍 / 👎.
| if (!attributed && prevAddress && this.currentAddress !== prevAddress) { | ||
| this.currentAddress = prevAddress; | ||
| this.persistActiveWallet(); |
There was a problem hiding this comment.
When the connected wallet is not in user.linkedAccounts, the inner per-wallet identifies set currentUserId and the user-id cookie to the Privy DID before this fallback restores only currentAddress. The next track()/page() event is then sent with the unlinked connected wallet address plus the Privy DID, falsely associating that wallet with the Privy user; snapshot and restore the prior user id/cookie when falling back to the pre-sync address.
Useful? React with 👍 / 👎.
…tive state
Replaces the ordering + snapshot/restore approach (which kept spawning
attribution edge cases) with a structural fix, per review decision. Also owns
chain state for the active wallet.
Root cause: the per-wallet identify loop rewrote the SDK's active identity
(currentAddress / currentUserId / user-id cookie) for every wallet, so any
non-active wallet could win or corrupt attribution.
- identify() concrete impl gains an internal `setActive` flag (NOT on the public
IFormoAnalytics.identify overloads/interface). When false, the wallet↔user
link is still emitted and deduped for clustering, but currentAddress AND
currentUserId/cookie are left untouched.
- identifyPrivyUser emits clustering identifies with setActive:false and
promotes only the resolved active wallet with setActive:true. Ordering and the
dispatch snapshot/restore are gone; a connected wallet that isn't linked in
Privy is now preserved structurally (no wallet is promoted), which also fixes
the userId-restore gap (it's never repointed in the first place).
- Active-wallet resolution: an explicit/connected activeAddress is matched
strictly (no fallback), else user.wallet, else last-external heuristic.
- Chain state: identifyPrivyUser returns the active wallet's { address,
chainType }, and the identify(user,{privy:true}) dispatch clears currentChainId
when the active wallet's chain namespace no longer matches (e.g. a Solana
wallet while an EVM chain id was current), so events/cookie aren't paired with
a mismatched chain.
Tests reworked to assert setActive instead of loop order; added userId-preserve
and Solana-chain-clear coverage. Docs updated. 675 passing, lint + build clean.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NVjnFuV1Bod4Vev9JYt2wq
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
… JSDoc
- Add docs/privy-identity-integration-plan.md: design/status doc covering the
clustering approach, the gaps closed, the attribution model (internal
setActive), Privy-docs validation, resolved review findings, and the deferred
Phase 2 items. Complements the usage-focused PRIVY_INTEGRATION.md.
- Fix IdentifyPrivyUserOptions.activeAddress JSDoc that still described the old
ordering ("identified last"); it now reflects the setActive promotion + strict
matching / preservation semantics.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NVjnFuV1Bod4Vev9JYt2wq
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5c33237076
ℹ️ 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".
| await identify( | ||
| { | ||
| address: wallet.address, | ||
| userId: user.id, | ||
| setActive: wallet === activeWallet, | ||
| }, | ||
| walletProperties | ||
| ); |
There was a problem hiding this comment.
When consumers use the documented identifyPrivyUser(formo, user, { activeAddress }) entry point directly and select a Solana linked wallet after an EVM wallet is connected, these calls update currentAddress to the Solana address but leave the existing EVM currentChainId intact. The reconciliation exists only in the formo.identify(user, { privy: true }) dispatcher, despite the docs describing the two forms as equivalent, so subsequent events from the direct-helper flow can be emitted with a Solana address paired to an EVM chain ID.
Useful? React with 👍 / 👎.
| // still match (backward compatible). | ||
| const parts = [encodeURIComponent(address)]; | ||
| if (rdns) parts.push(encodeURIComponent(rdns)); | ||
| if (userId) parts.push(encodeURIComponent(userId)); |
There was a problem hiding this comment.
When an application supplies a userId but no rdns whose value equals a provider RDNS (for example, io.metamask), this creates the same address:io.metamask key as an anonymous identify with rdns: "io.metamask". If the provider identify occurs first, the later user-ID identify is deduped and its identity-link event is never emitted, defeating the new userId-aware re-emission behavior. Encode the presence of each optional component (or always retain an empty RDNS field) so these distinct tuples cannot collide.
Useful? React with 👍 / 👎.
| // Reconcile the chain id with the newly-active wallet's chain namespace | ||
| // so a Solana address isn't left paired with a stale EVM chain id. | ||
| if (active) this.syncPrivyActiveChain(active.chainType); |
There was a problem hiding this comment.
When tracking.excludeChains contains the currently connected EVM chain and this convenience form activates a linked Solana wallet, the helper emits its inner identify calls while currentChainId is still the excluded EVM value. Each inner trackEvent() therefore fails shouldTrack() and drops the wallet↔DID identify event; only after the loop does this line clear the stale chain ID. Reconcile the namespace before invoking the helper (or otherwise ensure those identify events are evaluated against the selected wallet) so the new Solana identity is not silently lost.
Useful? React with 👍 / 👎.
Addresses Codex review of 5c33237 (3 × P2). - Chain reconciliation moved into identifyPrivyUser, BEFORE the emit loop (previously in the dispatch, after). identify() runs each event through the tracking gate, which enforces excludeChains against the current chain id; if the stale chain was excluded, every clustering identify was dropped and the Privy identity silently lost. Reconciling first fixes that, and doing it in the helper means the direct identifyPrivyUser() entry point reconciles too (the two forms are now truly equivalent, as the docs claim). syncPrivyActiveChain is now an internal (non-public) method invoked via a structural cast. - Session dedup keys: when a userId is present, always emit the rdns slot (even empty), so a userId equal to a provider RDNS (e.g. "io.metamask") can't collide with an anonymous address:rdns key and get wrongly deduped. No-userId keys are unchanged (backward compatible). Added tests: identifies still emit when on an excluded chain but activating a Solana wallet; the direct helper reconciles chain; userId==rdns doesn't collide. Docs/JSDoc updated. 678 passing, lint + build clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NVjnFuV1Bod4Vev9JYt2wq
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2581432b8e
ℹ️ 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".
| await identify( | ||
| { | ||
| address: wallet.address, | ||
| userId: user.id, | ||
| setActive: wallet === activeWallet, | ||
| }, | ||
| walletProperties | ||
| ); |
There was a problem hiding this comment.
When a Privy user has more than the session store's 20-entry limit of linked wallets, this loop marks each wallet in sequence and evicts earlier (wallet, DID) keys. A later effect run then re-emits at least the evicted identities (and re-emits every wallet on each run once the set exceeds twice the limit), despite the documented per-session deduplication. Size the session identity store for the expanded linked-wallet set or avoid evicting keys generated by this synchronization.
Useful? React with 👍 / 👎.
Addresses Codex review of 2581432 (P2). The identified-wallet session store used a fixed 20-entry cap, so a Privy user with more than 20 linked wallets evicted earlier (wallet, DID) keys; a later sync then re-emitted the evicted identities, defeating the per-session dedup. Replace the fixed entry-count cap with a serialized-size bound: keep every identity that fits in the cookie and evict oldest only when the value would overflow (~3.5KB, safely under the ~4KB per-cookie limit — room for ~40 identities, beyond any realistic linked-wallet set). Chosen over a bigger magic cap (still bounded by cookie size) and over hashing keys (which would trade a harmless re-emit for a silent collision-drop of an identity link). The detected store keeps its small count cap. Added a test asserting 40 identities all remain recognized (past the old limit). 679 passing, lint + build clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NVjnFuV1Bod4Vev9JYt2wq
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 594dc71de8
ℹ️ 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".
| const activeWallet = resolveActiveWallet( | ||
| wallets, | ||
| options.activeAddress, | ||
| user.wallet?.address |
There was a problem hiding this comment.
When callers use the documented direct identifyPrivyUser(formo, user) form without activeAddress after Formo has already connected an external wallet, this passes undefined and resolveActiveWallet promotes user.wallet, overwriting attribution with (for example) an embedded wallet. The flag wrapper now passes this.currentAddress before reaching this helper, but the direct helper is documented as equivalent; preserve the existing Formo address here as well, or require callers to supply it.
Useful? React with 👍 / 👎.
Addresses Codex review of 594dc71 (P2). The direct identifyPrivyUser(formo, user) form (no activeAddress) fell back to user.wallet and overwrote attribution on an already-connected wallet, while the identify(user,{privy:true}) dispatch passed this.currentAddress first — so the two forms diverged despite being documented as equivalent. Move the currentAddress fallback INTO the helper: it reads the instance's currentAddress (via the existing internal cast) before user.wallet, so both entry points preserve a connected wallet identically. The dispatch is now a thin pass-through of opts.activeAddress. Unmatched connected wallets are still preserved (strict match, no promotion); stubs without currentAddress fall through to user.wallet as before. Added a direct-form preservation test. 680 passing, lint + build clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NVjnFuV1Bod4Vev9JYt2wq
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1aab64170c
ℹ️ 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".
| if (setActive !== false) { | ||
| this.currentAddress = validAddress; | ||
| this.persistActiveWallet(); | ||
| if (userId) { | ||
| this.currentUserId = userId; |
There was a problem hiding this comment.
For every linked wallet other than the resolved active one, setActive:false leaves currentUserId unchanged. trackEvent() passes that stale value into EventFactory.create(), which unconditionally overwrites the identify payload's user_id with its supplied current user ID (src/event/EventFactory.ts:804). Thus a fresh Privy sync sends user_id: null for all non-active wallets (or, worse, a previous user's ID), so only the active wallet is clustered under the Privy DID and the feature can corrupt existing identity associations. Pass the identify call's userId through to event creation without mutating persistent active state.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1aab64170c
ℹ️ 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".
| // clustering identifies from being silently dropped. Doing it here — rather | ||
| // than in the identify(user,{privy:true}) dispatch — means the direct | ||
| // identifyPrivyUser() entry point gets the same treatment. | ||
| target.syncPrivyActiveChain?.(activeWallet?.chainType); |
There was a problem hiding this comment.
When this helper is called on an excluded host/path (or after opt-out) with a linked wallet in the opposite namespace, this reconciliation runs before the inner identify() calls reach their suppression guard. It clears currentChainId even though none of the identifies may update the active wallet. In a SPA that then returns to an allowed route, the old EVM currentAddress remains but its excluded chain ID is gone, so shouldTrack() can no longer apply tracking.excludeChains and subsequent events for that wallet are collected. Guard reconciliation with the same suppression condition, or defer it until an active identify is actually accepted.
Useful? React with 👍 / 👎.
| if (setActive !== false) { | ||
| this.currentAddress = validAddress; | ||
| this.persistActiveWallet(); | ||
| if (userId) { | ||
| this.currentUserId = userId; | ||
| const domain = getIdentityCookieDomain(this.crossSubdomainCookies); | ||
| cookie().set(SESSION_USER_ID_KEY, userId, { | ||
| path: "/", | ||
| ...getIdentityCookieSecurity(), | ||
| ...(domain ? { domain } : {}), | ||
| }); | ||
| } |
There was a problem hiding this comment.
For every non-active linked wallet, setActive:false intentionally leaves currentUserId unchanged here, but trackEvent() passes that state to EventManager, and EventFactory.create() overwrites an identify event's payload userId with this passed state. Thus a normal multi-wallet sync emits only the active wallet under the Privy DID; the other wallet identifies have user_id: null (or, worse, a previous user's ID), so the advertised server-side wallet clustering fails or is misattributed. Preserve the payload DID when enqueueing these identify events rather than relying on active SDK state.
Useful? React with 👍 / 👎.
…n suppressed Two P1s from Codex review of 1aab641. 1. Clustering events lost the Privy DID. EventFactory.create() unconditionally overwrote each identify event's payload user_id with the active-session user id. Because setActive:false clustering identifies intentionally do NOT change currentUserId, every non-active wallet's identify was emitted with user_id: null (or a stale user), so only the active wallet clustered under the DID — silently breaking the whole feature. Fix: identify events keep their payload user_id (the per-wallet DID), falling back to the session user id only when the payload carries none. Other events are unchanged. This also preserves the WTo behavior (session currentUserId is still not repointed). 2. Chain reconciliation ran for suppressed visitors. identifyPrivyUser reconciles chain BEFORE the inner identifies reach their suppression guard, so on an excluded host/path (or after opt-out) it cleared currentChainId while no identify actually ran — leaving later events on an allowed route unable to apply excludeChains. Fix: gate the whole helper on isTrackingSuppressed (via the internal cast), covering both entry points. Added tests: EventFactory.create keeps the identify payload DID over/without the session user id; identifyPrivyUser emits nothing and does not reconcile chain when suppressed. 685 passing, lint + build clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NVjnFuV1Bod4Vev9JYt2wq
Final review pass over the branch. No runtime bugs found; the issues were documentation left behind by the ordering -> setActive refactor, plus one dead export: - identify() @example and the IFormoAnalytics Privy-overload JSDoc still described the removed mechanisms ("identifies the active wallet last", "defaults to user.wallet") — both now state the real resolution order (explicit activeAddress -> connected wallet -> user.wallet) and the clustering-only nature of non-active identifies. - IdentifyPrivyUserOptions.activeAddress JSDoc was missing the connected-wallet fallback step. - PRIVY_INTEGRATION.md still claimed wallets are "identified in a deliberate order — the active wallet last". - isPrivyWalletAccount was exported solely for the deleted React hook; made module-private (it was never re-exported from the package entries, so no public API change). - Plan doc: resolved-findings list updated with the last four review rounds; test count refreshed (685). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NVjnFuV1Bod4Vev9JYt2wq
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
…-one-liner-6rovf1
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
2 participants
Summary
This PR adds comprehensive Privy integration to the Formo Analytics SDK, enabling automatic clustering of multiple linked wallets under a single Privy user identity. It includes a one-liner helper function, a React hook, detailed documentation, and full test coverage.
Key Changes
New
identifyPrivyUser()function (src/privy/utils.ts): Core imperative helper that identifies every wallet linked to a Privy user under that user's DID. Handles per-wallet metadata forwarding (wallet_client,chain_type,is_embedded) and ensures event attribution stays on the active/connected wallet via asetCurrentAddressparameter.New
useIdentifyPrivyUser()React hook (src/privy/react.ts): Declarative wrapper that keeps Formo's identity in sync with Privy'susePrivy()user object. Automatically re-runs on login,linkWallet, andunlinkWalletevents. Uses a stable signature of the DID + linked wallet set to avoid unnecessary re-renders.Enhanced session deduplication (
src/session/index.ts): ExtendedisWalletIdentified()andmarkWalletIdentified()to accept an optionaluserIdparameter. When provided, the user ID is folded into the dedup key, allowing the same wallet to re-emit an identify event when a Privy DID is attached after initial anonymous identification.Updated
identify()signature (src/FormoAnalytics.ts,src/types/base.ts): Added optionalsetCurrentAddressparameter to control whether an identify call updates the SDK's current wallet address (used to prevent non-active linked wallets from hijacking event attribution).Comprehensive documentation (
docs/PRIVY_INTEGRATION.md): 220-line guide covering quick start, framework-agnostic usage, event attribution semantics, deduplication behavior, and FAQ.Full test coverage:
identifyPrivyUser()andparsePrivyProperties()(test/privy/privy.spec.ts)FormoAnalyticsinstance (test/privy/identifyPrivyUser.integration.spec.ts)test/session/dedup.spec.ts)Implementation Details
Per-wallet metadata: Each linked wallet is identified with its own
wallet_client,chain_type, andis_embeddedproperties, enabling segmentation by wallet type and chain in analytics.Attribution control: The
setCurrentAddressparameter ensures only the active wallet (passed viaactiveAddressoption) becomes the SDK's current address. Other linked wallets are identified withsetCurrentAddress: falseso they're recorded for clustering without hijacking attribution.Fallback ordering: When
activeAddressis not provided, embedded (Privy) wallets are identified first and external wallets last, with attribution defaulting to the last external wallet—a best-effort guess that prefers external wallets over embedded ones.Backward compatibility: Session dedup keys remain unchanged when
userIdis omitted, preserving existing browser-stored keys. ThesetCurrentAddressparameter defaults totrue, maintaining existing behavior for non-Privy flows.React hook optimization: Uses a derived signature string (DID + sorted wallet addresses + active address) as the effect dependency, not the
userobject reference, since Privy recreates the user object on every render.https://claude.ai/code/session_01NVjnFuV1Bod4Vev9JYt2wq
Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.