feat: Add autocapture configuration [P-1040] #128
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
[P-1545] |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Greptile Overview
Greptile Summary
This PR adds configurable wallet event autocapture to the Formo SDK, allowing developers to control which wallet events (connect, disconnect, signature, transaction, chain) are automatically tracked.
Key Changes
- Type System: New
AutocaptureOptionsinterface insrc/types/base.tswithenabledflag and granulareventsconfiguration - Core Logic: Modified
trackProvider()insrc/FormoAnalytics.tsto conditionally register listeners based on config, withisWalletAutocaptureEnabled()method checking config throughout event handlers - State Management: State (
currentAddress,currentChainId) is always updated even when tracking is disabled to ensure disconnect events have valid data - Documentation: Comprehensive docs with 16 practical examples covering DeFi, NFT, multi-chain scenarios
- Tests: Basic configuration parsing tests (but no integration tests for runtime behavior)
Implementation Approach
The implementation conditionally registers event listeners during provider initialization and gates event emission in handlers. When tracking is disabled for an event type, the SDK updates internal state but doesn't emit analytics events. This ensures consistency (e.g., disconnect can reference the last known address even if connect tracking was disabled).
Confidence Score: 4/5
- Safe to merge with one logic concern about state management when both connect/disconnect tracking are disabled
- Well-structured feature with clean type definitions, comprehensive docs, and thoughtful state management. Deducted one point for: (1) potential state tracking issue when both connect/disconnect are disabled, and (2) tests that verify config but not actual runtime behavior
- src/FormoAnalytics.ts - review accountsChanged listener registration logic at line 788
Important Files Changed
File Analysis
| Filename | Score | Overview |
|---|---|---|
| src/types/base.ts | 5/5 | Added well-documented AutocaptureOptions interface with global enabled flag and granular event controls. Clean type definitions with default values clearly specified. |
| src/FormoAnalytics.ts | 4/5 | Implements conditional listener registration and event emission based on autocapture config. State is always updated even when tracking disabled (critical for disconnect events). Comprehensive isWalletAutocaptureEnabled() method with proper fallback logic. |
| test/FormoAnalytics.autocapture.spec.ts | 3/5 | Tests verify configuration parsing but lack runtime behavior tests. No actual verification of listener registration or event emission - only config object assertions. |
| examples/autocapture-examples.ts | 5/5 | Comprehensive examples covering 16 use cases from DeFi to NFT marketplaces. Well-documented with clear explanations and practical scenarios. |
Sequence Diagram
sequenceDiagram
participant User
participant App
participant SDK as FormoAnalytics
participant Provider as EIP1193Provider
participant API as Events API
Note over App,SDK: Initialization with autocapture config
App->>SDK: FormoAnalytics.init(writeKey, {autocapture: {...}})
SDK->>SDK: Parse autocapture options
SDK->>SDK: isWalletAutocaptureEnabled() bound to instance
alt autocapture enabled (default or true)
SDK->>Provider: Register listeners conditionally
Note over SDK,Provider: Only registers listeners for enabled events
alt connect/disconnect enabled
SDK->>Provider: registerAccountsChangedListener()
end
alt chain enabled
SDK->>Provider: registerChainChangedListener()
end
alt connect enabled
SDK->>Provider: registerConnectListener()
end
alt disconnect enabled
SDK->>Provider: registerDisconnectListener()
end
alt signature/transaction enabled
SDK->>Provider: registerRequestListeners()
SDK->>Provider: Wrap provider.request()
end
else autocapture disabled
Note over SDK,Provider: No listeners registered
end
Note over User,API: Runtime Event Flow
User->>Provider: Connect wallet
Provider->>SDK: accountsChanged event
SDK->>SDK: isWalletAutocaptureEnabled("connect")
alt connect tracking enabled
SDK->>SDK: Update state (address, chainId)
SDK->>API: Emit connect event
else connect tracking disabled
SDK->>SDK: Update state only (no event)
Note over SDK: State updated for future disconnect events
end
User->>Provider: Sign message
Provider->>SDK: Intercepted via wrapped request()
SDK->>SDK: isWalletAutocaptureEnabled("signature")
alt signature tracking enabled
SDK->>API: Emit signature requested
SDK->>Provider: Forward request
Provider-->>SDK: Signature result
SDK->>API: Emit signature confirmed/rejected
else signature tracking disabled
SDK->>Provider: Forward request (no tracking)
Provider-->>SDK: Signature result
end
User->>Provider: Disconnect wallet
Provider->>SDK: accountsChanged([])
SDK->>SDK: isWalletAutocaptureEnabled("disconnect")
alt disconnect tracking enabled
SDK->>API: Emit disconnect event
SDK->>SDK: Clear state
else disconnect tracking disabled
SDK->>SDK: Clear state only (no event)
end
10 files reviewed, 2 comments
src/FormoAnalytics.ts
Outdated
| const shouldTrackConnectOrDisconnect = | ||
| this.isWalletAutocaptureEnabled("connect") || this.isWalletAutocaptureEnabled("disconnect"); | ||
|
|
||
| if (shouldTrackConnectOrDisconnect) { | ||
| this.registerAccountsChangedListener(provider); | ||
| } |
There was a problem hiding this comment.
logic: If both connect and disconnect tracking are disabled, the accountsChanged listener won't be registered. This means state (currentAddress, currentChainId) won't be updated when the wallet connects/disconnects, which could cause issues if you later enable these events or if other parts of the SDK rely on current state.
Consider always registering accountsChanged for state management, but conditionally emit events.
| it('should default to all wallet events enabled when no config provided', () => { | ||
| // This test verifies backward compatibility | ||
| // When autocapture is not specified, all events should be tracked | ||
| const options = {}; | ||
|
|
||
| // Expected: isWalletAutocaptureEnabled() returns true | ||
| // Expected: isWalletEventEnabled('connect') returns true | ||
| // Expected: isWalletEventEnabled('signature') returns true | ||
| expect(options).toBeDefined(); | ||
| }); |
There was a problem hiding this comment.
style: Tests only verify configuration objects exist but don't actually test the SDK behavior. Consider adding integration tests that instantiate FormoAnalytics with various configs, mock providers and trigger events, then verify which listeners are registered and which events are emitted.
|
bugbot run |
There was a problem hiding this comment.
Greptile Overview
Greptile Summary
This PR introduces configurable wallet autocapture with per-event toggles while ensuring robust state management. The implementation addresses a critical bug where the accountsChanged listener wasn't always registered, which would have caused state inconsistencies.
Key Changes:
- Always registers
accountsChangedlistener for state management (lines 786-789), regardless of config - Conditionally registers other listeners (
chainChanged,connect,disconnect, request wrapper) based onautocaptureconfiguration (lines 792-806) - Gates event emission inside handlers using
isWalletAutocaptureEnabled()checks while maintaining state updates - Adds comprehensive integration tests (366 lines) covering listener registration, state management, and event gating
- Manual state clearing in all code paths when disconnect tracking is disabled to maintain consistency
Implementation Notes:
- State (
currentAddress,currentChainId,_provider) is always maintained regardless of tracking configuration - Event emission is controlled separately from state updates, allowing future flexibility
- The
disconnect()method automatically clears state (lines 353-355), which works when tracking is enabled - When disconnect tracking is disabled, state is manually cleared in multiple code paths (accountsChanged handler, provider switching logic)
Minor Issue:
- Defensive check in
registerDisconnectListenerat line 1212 is unreachable since the listener is only registered when tracking is enabled (addressed in style comment)
Confidence Score: 4/5
- This PR is safe to merge with minor style improvements recommended
- The implementation correctly fixes the critical state management bug and adds comprehensive test coverage. The only issue is unreachable defensive code (style concern, not a functional bug). All state clearing paths are properly handled, and the separation of state management from event emission is well-designed. Score of 4 (not 5) due to the minor code clarity issue with unreachable defensive code.
- No files require special attention - the unreachable code in
src/FormoAnalytics.ts:1212-1230is a minor style issue that can be addressed in a follow-up if desired
Important Files Changed
File Analysis
| Filename | Score | Overview |
|---|---|---|
| src/FormoAnalytics.ts | 4/5 | Implements configurable wallet autocapture with always-on state management and conditional event emission. Defensive check in disconnect listener is unreachable code but harmless. |
| test/FormoAnalytics.autocapture.integration.spec.ts | 5/5 | Comprehensive integration tests covering listener registration, state management, and event gating across various configurations. Well-structured with good coverage. |
| AUTOCAPTURE_FIXES.md | 5/5 | Documentation file explaining the two critical fixes: always registering accountsChanged listener and adding comprehensive integration tests. Clear and thorough. |
Sequence Diagram
sequenceDiagram
participant User
participant SDK as FormoAnalytics
participant Config as isWalletAutocaptureEnabled
participant Provider as Wallet Provider
participant State as Internal State
User->>SDK: init(apiKey, options)
SDK->>Config: Check autocapture config
alt autocapture.enabled !== false
SDK->>Provider: Always register accountsChanged listener
Note over SDK,Provider: State management always active
Config->>SDK: Check chain enabled
alt chain tracking enabled
SDK->>Provider: Register chainChanged listener
end
Config->>SDK: Check connect enabled
alt connect tracking enabled
SDK->>Provider: Register connect listener
end
Config->>SDK: Check disconnect enabled
alt disconnect tracking enabled
SDK->>Provider: Register disconnect listener
end
Config->>SDK: Check signature/transaction enabled
alt signature OR transaction enabled
SDK->>Provider: Register request wrapper
end
end
Note over SDK: Wallet Event Occurs
Provider->>SDK: accountsChanged event
SDK->>State: Always update currentAddress/currentChainId
Config->>SDK: Check if connect/disconnect enabled
alt Event tracking enabled
SDK->>SDK: Emit analytics event
else Event tracking disabled
Note over SDK: State updated, no event emitted
end
Provider->>SDK: chainChanged event (if listener registered)
SDK->>State: Update currentChainId
Config->>SDK: Check if chain enabled
alt chain tracking enabled
SDK->>SDK: Emit chain event
end
Provider->>SDK: disconnect event (if listener registered)
Config->>SDK: Check if disconnect enabled
alt disconnect tracking enabled
SDK->>SDK: Call disconnect() method
SDK->>State: Clear state (address, chainId, provider)
end
3 files reviewed, 1 comment
src/FormoAnalytics.ts
Outdated
| if (this.isWalletAutocaptureEnabled("disconnect")) { | ||
| try { | ||
| // Pass current state explicitly to ensure we have the data for the disconnect event | ||
| await this.disconnect({ | ||
| chainId: this.currentChainId, | ||
| address: this.currentAddress, | ||
| }); | ||
| // Provider remains tracked to allow for reconnection scenarios | ||
| } catch (e) { | ||
| logger.error("Error during disconnect in disconnect listener", e); | ||
| // Don't untrack if disconnect failed to maintain state consistency | ||
| } | ||
| } else { | ||
| logger.info("OnDisconnect: Disconnect tracking disabled, clearing state without emitting event"); | ||
| // Still need to clear state even if not tracking the event | ||
| this.currentAddress = undefined; | ||
| this.currentChainId = undefined; | ||
| this.clearActiveProvider(); | ||
| } |
There was a problem hiding this comment.
style: The else branch (lines 1224-1230) is unreachable defensive code. Since registerDisconnectListener is only called when isWalletAutocaptureEnabled("disconnect") returns true (line 804), the check at line 1212 will always be true. Consider removing the defensive check and else branch for clarity.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Greptile Overview
Greptile Summary
This PR adds extensive documentation for an autocapture configuration feature that was completely removed from the codebase.
Critical Issue: The PR contains 4 comprehensive documentation files (AUTOCAPTURE.md, CHANGELOG_AUTOCAPTURE.md, AUTOCAPTURE_FLOW.md, AUTOCAPTURE_QUICK_REFERENCE.md) that describe an autocapture configuration option allowing granular control over wallet event tracking. However, the actual implementation was removed in subsequent commits:
- The
autocapturefield is missing from theOptionsinterface insrc/types/base.ts:116-128 - The
AutocaptureOptionstype doesn't exist - Methods like
isWalletAutocaptureEnabled()andisWalletEventEnabled()are not implemented src/FormoAnalytics.ts:776-797unconditionally registers all event listeners without any configuration checks
What actually works: Only the README.md self-hosting section addition is valid and functional.
Impact: Users following the documentation will encounter TypeScript errors (if using TypeScript) or find that configuration has no effect (if using JavaScript). All code examples in the documentation are broken.
Confidence Score: 0/5
- This PR is NOT safe to merge - it contains extensive documentation for a feature that doesn't exist
- Score of 0 reflects that this PR adds broken documentation that will mislead users and cause errors. The documentation describes types, interfaces, methods, and behavior that were completely removed from the implementation. Merging would result in users following invalid examples that either fail at compile-time (TypeScript) or silently do nothing at runtime (JavaScript).
- All autocapture documentation files (AUTOCAPTURE.md, CHANGELOG_AUTOCAPTURE.md, AUTOCAPTURE_FLOW.md, AUTOCAPTURE_QUICK_REFERENCE.md) must either be removed OR the implementation must be restored to src/types/base.ts and src/FormoAnalytics.ts
Important Files Changed
File Analysis
| Filename | Score | Overview |
|---|---|---|
| src/types/base.ts | 0/5 | Missing autocapture field and AutocaptureOptions type that are extensively documented across multiple MD files |
| src/FormoAnalytics.ts | 0/5 | No conditional listener registration logic; always registers all listeners despite documentation claiming conditional behavior |
| AUTOCAPTURE.md | 0/5 | Complete documentation for non-existent feature; all code examples will fail with TypeScript errors or be ignored at runtime |
| CHANGELOG_AUTOCAPTURE.md | 0/5 | Changelog describes feature additions that don't exist: missing types, missing methods, missing configuration options |
| AUTOCAPTURE_FLOW.md | 0/5 | Flow diagrams depict conditional logic that doesn't exist in actual implementation |
| AUTOCAPTURE_QUICK_REFERENCE.md | 0/5 | Quick reference with invalid code examples that reference non-existent autocapture configuration option |
Sequence Diagram
sequenceDiagram
participant User
participant SDK as FormoAnalytics
participant Provider as Wallet Provider
Note over User,Provider: DOCUMENTED BEHAVIOR (Not Implemented)
User->>SDK: init(writeKey, {autocapture: {enabled: true, events: {signature: false}}})
SDK->>SDK: Check autocapture config
SDK->>SDK: isWalletEventEnabled('signature') → false
SDK->>Provider: Register connect/disconnect/chain listeners only
Note over SDK,Provider: Signature listener NOT registered (documented)
rect rgb(255, 230, 230)
Note over User,Provider: ACTUAL BEHAVIOR (Current Implementation)
User->>SDK: init(writeKey, {autocapture: {...}})
Note over SDK: Options interface has no 'autocapture' field<br/>TypeScript error or runtime ignored
SDK->>SDK: trackProvider() called
SDK->>Provider: ALWAYS register accountsChanged listener
SDK->>Provider: ALWAYS register chainChanged listener
SDK->>Provider: ALWAYS register connect listener
SDK->>Provider: ALWAYS register request wrapper
SDK->>Provider: ALWAYS register disconnect listener
Note over SDK,Provider: All listeners registered unconditionally
end
Additional Comments (2)
-
src/types/base.ts, line 116-128 (link)logic: The
Optionsinterface is missing theautocapturefield that is extensively documented inAUTOCAPTURE.md,CHANGELOG_AUTOCAPTURE.md,AUTOCAPTURE_FLOW.md, andAUTOCAPTURE_QUICK_REFERENCE.md. All documentation describes anautocapture?: boolean | AutocaptureOptionsoption, but this field doesn't exist in the actual interface.The
AutocaptureOptionstype is also completely missing from this file. -
src/FormoAnalytics.ts, line 776-797 (link)logic: The
trackProvidermethod unconditionally registers all event listeners (lines 786-790), but the documentation claims that listeners should be conditionally registered based onautocaptureconfiguration. The documentation states: "When wallet events are disabled, the corresponding listeners are not registered at all" (CHANGELOG_AUTOCAPTURE.md:75), but this implementation always registers all listeners regardless of configuration.The methods
isWalletAutocaptureEnabled()andisWalletEventEnabled()mentioned in CHANGELOG_AUTOCAPTURE.md:88-89 don't exist in this file.
8 files reviewed, 6 comments
|
|
🎉 This PR is included in version 1.22.0 🎉 |
Note
Introduce configurable autocapture for wallet events (connect, disconnect, signature, transaction, chain) and conditionally register/emit related listeners/events while preserving state updates.
isAutocaptureEnabledand bind it; default enabled, supports boolean or per-event options.accountsChangedfor state; conditionally registerchainChanged,connect,disconnect, and request wrappers based on autocapture.connect,disconnect,chain,signature, andtransactionevents; still updatecurrentAddress/currentChainIdeven when events are skipped.provider.requestconditionally; skip signature/transaction tracking when disabled.AutocaptureOptionsandOptions.autocapturefor granular control.1.22.0.Written by Cursor Bugbot for commit 463174f. This will update automatically on new commits. Configure here.