1209-task-move-chrome-specifc-analtyics-and-logging-back-to-extension#1213
Conversation
Closes #1209
- Updated @onflow/frw-core, @onflow/frw-data-model, and related packages to version 0.2.0 in package.json. - Refactored analytics tracking from mixpanelTrack to analyticsService across various components and services. - Replaced storage usage with local data functions in multiple files for better data management. - Adjusted TypeScript configurations to include custom conditions and updated module resolution settings. - Cleaned up imports and improved code organization in several UI components and hooks. Closes #1209
Closes #1209
PR SummaryRefactored analytics and logging functionality from core packages back to the extension. Replaced mixpanel tracking with a new analytics service, initialized Chrome storage implementation, and updated dependencies. The changes improve separation of concerns by keeping browser-specific code in the extension. Changes
autogenerated by presubmit.ai |
There was a problem hiding this comment.
✅ LGTM!
Review Summary
Commits Considered (5)
- 4cd9767: Updated packages
Closes #1209
-
0f63991: Update dependencies and refactor analytics service
-
Updated @onflow/frw-core, @onflow/frw-data-model, and related packages to version 0.2.0 in package.json.
-
Refactored analytics tracking from mixpanelTrack to analyticsService across various components and services.
-
Replaced storage usage with local data functions in multiple files for better data management.
-
Adjusted TypeScript configurations to include custom conditions and updated module resolution settings.
-
Cleaned up imports and improved code organization in several UI components and hooks.
Closes #1209
- e59d448: Added promiseFlow test
Closes #1209
- 1cb1a47: Moved chrome specific stuff to extension
Closes #1209
- 33cd9cb: Move log listener and mixpanel back to extension
Closes #1209
Files Processed (30)
- .storybook/main.ts (1 hunk)
- docs/cache-data-model.md (1 hunk)
- package.json (1 hunk)
- pnpm-lock.yaml (0 hunks)
- pnpm-workspace.yaml (1 hunk)
- src/background/controller/tests/controller.test.ts (1 hunk)
- src/background/controller/tests/hash-comparison.test.ts (1 hunk)
- src/background/controller/provider/rpcFlow.ts (1 hunk)
- src/background/controller/wallet.ts (7 hunks)
- src/background/index.ts (5 hunks)
- src/background/utils/googleDriveAuthToken.ts (1 hunk)
- src/background/utils/log-listener.ts (1 hunk)
- src/background/utils/mixpanel-analytics.ts (1 hunk)
- src/background/utils/promiseFlow.test.ts (1 hunk)
- src/background/utils/promiseFlow.ts (1 hunk)
- src/ui/hooks/tests/useNetworkHook.test.ts (9 hunks)
- src/ui/hooks/preference-hooks.ts (2 hunks)
- src/ui/hooks/use-coin-hooks.ts (2 hunks)
- src/ui/hooks/use-data.ts (3 hunks)
- src/ui/hooks/useNetworkHook.ts (3 hunks)
- src/ui/index.tsx (1 hunk)
- src/ui/views/NFT/EditNFTAddress.tsx (2 hunks)
- src/ui/views/NftEvm/EditNFTAddress.tsx (2 hunks)
- src/ui/views/SendTo/index.tsx (1 hunk)
- src/ui/views/Setting/AccountList/AccountDetail.tsx (3 hunks)
- src/ui/views/Setting/DeveloperMode/DeveloperMode.tsx (4 hunks)
- src/ui/views/Welcome/Register/index.tsx (1 hunk)
- src/ui/views/Welcome/import-account/index.tsx (1 hunk)
- src/ui/views/Welcome/import-profile/index.tsx (1 hunk)
- tsconfig.json (1 hunk)
Actionable Comments (0)
Skipped Comments (4)
-
src/background/utils/mixpanel-analytics.ts [130-131]
enhancement: "Add timeout to analytics network request"
-
src/background/utils/googleDriveAuthToken.ts [7-14]
enhancement: "Improve error handling in auth token retrieval"
-
src/background/utils/mixpanel-analytics.ts [81-82]
best practice: "Function return type doesn't match implementation"
-
src/background/utils/promiseFlow.ts [5-6]
best practice: "Use generic type for better type safety"
Closes #1209
There was a problem hiding this comment.
🚨 Pull request needs attention.
Review Summary
Commits Considered (2)
- 55a3b3a: Upgrade ext shared
Closes #1209
- cac6b7e: Initialize chrome storage
Closes #1209
Files Processed (30)
- .storybook/main.ts (1 hunk)
- .vscode/settings.json (1 hunk)
- CLAUDE.md (1 hunk)
- docs/cache-data-model.md (1 hunk)
- package.json (1 hunk)
- pnpm-lock.yaml (0 hunks)
- pnpm-workspace.yaml (1 hunk)
- src/background/controller/tests/controller.test.ts (1 hunk)
- src/background/controller/tests/hash-comparison.test.ts (1 hunk)
- src/background/controller/provider/rpcFlow.ts (1 hunk)
- src/background/controller/wallet.ts (7 hunks)
- src/background/index.ts (6 hunks)
- src/background/utils/googleDriveAuthToken.ts (1 hunk)
- src/background/utils/log-listener.ts (1 hunk)
- src/background/utils/mixpanel-analytics.ts (1 hunk)
- src/background/utils/promiseFlow.test.ts (1 hunk)
- src/background/utils/promiseFlow.ts (1 hunk)
- src/ui/hooks/tests/useNetworkHook.test.ts (9 hunks)
- src/ui/hooks/preference-hooks.ts (2 hunks)
- src/ui/hooks/use-coin-hooks.ts (2 hunks)
- src/ui/hooks/use-data.ts (3 hunks)
- src/ui/hooks/useNetworkHook.ts (3 hunks)
- src/ui/index.tsx (2 hunks)
- src/ui/views/NFT/EditNFTAddress.tsx (2 hunks)
- src/ui/views/NftEvm/EditNFTAddress.tsx (2 hunks)
- src/ui/views/SendTo/index.tsx (1 hunk)
- src/ui/views/Setting/AccountList/AccountDetail.tsx (3 hunks)
- src/ui/views/Setting/DeveloperMode/DeveloperMode.tsx (4 hunks)
- src/ui/views/Welcome/Register/index.tsx (1 hunk)
- src/ui/views/Welcome/import-account/index.tsx (1 hunk)
Actionable Comments (1)
-
src/background/utils/mixpanel-analytics.ts [124-127]
security: "Use HTTPS for API requests"
Skipped Comments (3)
-
src/background/controller/wallet.ts [1254-1254]
best practice: "Add error handling for storage operation"
-
src/background/utils/mixpanel-analytics.ts [66-69]
best practice: "Limit exposure of browser information in analytics"
-
src/ui/hooks/use-data.ts [86-87]
enhancement: "Improve storage listener management"
…ogging-back-to-extension Closes #1209
There was a problem hiding this comment.
🚨 Pull request needs attention.
Review Summary
Commits Considered (1)
- e88356f: Merge branch 'dev' into 1209-task-move-chrome-specifc-analtyics-and-logging-back-to-extension
Closes #1209
Files Processed (1)
- src/ui/views/Welcome/Register/index.tsx (1 hunk)
Actionable Comments (1)
-
src/ui/views/Welcome/Register/index.tsx [7-7]
possible issue: "Import path change might affect dependency management"
Skipped Comments (0)
Closes #1209
There was a problem hiding this comment.
✅ LGTM!
Review Summary
Files Processed (1)
- src/ui/hooks/tests/useNetworkHook.test.ts (9 hunks)
Actionable Comments (0)
Skipped Comments (2)
-
src/ui/hooks/__tests__/useNetworkHook.test.ts [24-40]
enhancement: "Mock implementation could be simplified using a switch statement"
-
src/ui/hooks/__tests__/useNetworkHook.test.ts [122-122]
possible issue: "Potential undefined variable usage"
There was a problem hiding this comment.
🚨 Pull request needs attention.
Review Summary
Files Processed (1)
- src/background/index.ts (5 hunks)
Actionable Comments (2)
-
src/background/index.ts [64-65]
best practice: "Storage initialization should handle potential errors"
-
src/background/index.ts [104-108]
possible issue: "Potential race condition in data access"
Skipped Comments (1)
-
src/background/index.ts [87-95]
performance: "Analytics initialization could block other critical services"
| // 1. Initialize storage first | ||
| initializeStorage({ implementation: chromeStorage }); |
There was a problem hiding this comment.
The storage initialization is a critical operation but there's no error handling. Consider wrapping it in a try-catch block to gracefully handle potential storage initialization failures, which could occur due to browser permissions or storage quota issues.
Related Issue
Closes #1209
Summary of Changes
Now initialises chrome specific stuff in the ui and background. We really need to create a vite like config for this
Need Regression Testing
Quite a few fundamental changes. Check core functionality
Risk Assessment