chore(react-native): remove unused ActivityDetailSheet component / add missing translation keys - #1211
chore(react-native): remove unused ActivityDetailSheet component / add missing translation keys#1211lealobanov wants to merge 14 commits into
Conversation
PR SummaryAddressed multiple issues related to transaction activity monitoring in the browser extension and internationalization in the React Native app. Key changes include:
Changes
autogenerated by presubmit.ai |
GitHub Actions
Inactive
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
🚨 Pull request needs attention.
Review Summary
Commits Considered (1)
-
26948cb: feat: add ActivityDetailSheet modal for transaction details
-
Create ActivityDetailSheet component with Tamagui Sheet
-
Add ActivityDetailRow reusable component for detail rows
-
Wire up detail sheet in ActivityScreen with onPress handler
-
Add block explorer navigation via Linking.openURL
-
Add i18n translations for all supported languages (en, zh, ru, jp, es)
Co-Authored-By: Claude Opus 4.5 noreply@anthropic.com
Closes #1195
Files Processed (10)
- packages/screens/src/activity/ActivityScreen.query.tsx (5 hunks)
- packages/screens/src/locales/en.json (1 hunk)
- packages/screens/src/locales/es.json (1 hunk)
- packages/screens/src/locales/jp.json (1 hunk)
- packages/screens/src/locales/ru.json (1 hunk)
- packages/screens/src/locales/zh.json (1 hunk)
- packages/ui/src/components/ActivityDetailRow.tsx (1 hunk)
- packages/ui/src/components/ActivityDetailSheet.tsx (1 hunk)
- packages/ui/src/index.ts (1 hunk)
- packages/ui/src/types/index.ts (1 hunk)
Actionable Comments (2)
-
packages/ui/src/components/ActivityDetailSheet.tsx [362-362]
possible bug: "Hardcoded network fee value should be dynamic."
-
packages/ui/src/components/ActivityDetailSheet.tsx [353-356]
possible bug: "Hardcoded transaction fee values should be dynamic."
Skipped Comments (3)
-
packages/screens/src/activity/ActivityScreen.query.tsx [89-91]
possible issue: "Potential memory leak with setTimeout in callback."
-
packages/ui/src/components/ActivityDetailSheet.tsx [57-64]
enhancement: "Date formatting uses hardcoded locale."
-
packages/ui/src/components/ActivityDetailSheet.tsx [331-331]
possible bug: "Missing null check for address display."
GitHub Actions
Inactive
GitHub Actions
Inactive
There was a problem hiding this comment.
🚨 Pull request needs attention.
Review Summary
Commits Considered (3)
- 23b2469: fix: make ActivityCard onPress work reliably
Change from conditional spread pattern to direct prop assignment
for onPress handling on XStack.
Co-Authored-By: Claude Opus 4.5 noreply@anthropic.com
Closes #1195
-
26b2080: fix: align ActivityDetailSheet with Figma designs
-
Large amount for sent transactions uses neutral color (red in "You Sent" row only)
-
Hide Date row for App Interaction type (per Figma)
Co-Authored-By: Claude Opus 4.5 noreply@anthropic.com
Closes #1195
- 09be913: refactor: use ActivityDetailRow instead of inline DetailRow
Remove duplicate DetailRow component and import the existing
ActivityDetailRow component to reduce code duplication.
Co-Authored-By: Claude Opus 4.5 noreply@anthropic.com
Closes #1195
Files Processed (2)
- packages/ui/src/components/ActivityCard.tsx (1 hunk)
- packages/ui/src/components/ActivityDetailSheet.tsx (1 hunk)
Actionable Comments (1)
-
packages/ui/src/components/ActivityDetailSheet.tsx [311-311]
possible bug: "Hardcoded network fee value should be dynamic."
Skipped Comments (3)
-
packages/ui/src/components/ActivityDetailSheet.tsx [58-65]
enhancement: "Date formatting uses hardcoded locale instead of user's locale."
-
packages/ui/src/components/ActivityDetailSheet.tsx [315-329]
enhancement: "Button colors may not adapt to dark theme."
-
packages/ui/src/components/ActivityCard.tsx [144-146]
maintainability: "Props are always passed even when onPress is undefined."
GitHub Actions
Inactive
GitHub Actions
Inactive
There was a problem hiding this comment.
✅ LGTM!
Review Summary
Commits Considered (1)
- a509900: fix: use Sent/Received title for self-transfers instead of Transferred
Co-Authored-By: Claude Opus 4.5 noreply@anthropic.com
Closes #1195
Files Processed (1)
- packages/services/src/ActivityService.ts (2 hunks)
Actionable Comments (0)
Skipped Comments (1)
-
packages/services/src/ActivityService.ts [182-182]
possible issue: "Inconsistent transfer type assignment for interactions."
GitHub Actions
Inactive
GitHub Actions
Inactive
There was a problem hiding this comment.
✅ LGTM!
Review Summary
Commits Considered (1)
- 0958169: fix: use EVM block explorer for Flow EVM transactions
Co-Authored-By: Claude Opus 4.5 noreply@anthropic.com
Closes #1195
Files Processed (1)
- packages/screens/src/activity/ActivityScreen.query.tsx (5 hunks)
Actionable Comments (0)
Skipped Comments (2)
-
packages/screens/src/activity/ActivityScreen.query.tsx [90-90]
maintainability: "Hardcoded timeout for animation synchronization may be fragile."
-
packages/screens/src/activity/ActivityScreen.query.tsx [109-109]
possible bug: "Missing error handling for URL opening."
GitHub Actions
Inactive
GitHub Actions
Inactive
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
There was a problem hiding this comment.
✅ LGTM!
Review Summary
Commits Considered (1)
- 6733452: chore: merge dev branch, resolve conflict in ActivityService
Removed createMockActivityItems helper function to align with dev.
Closes #1195
Files Processed (11)
- docs/releaseNotes/ext/3-1-10-de.md (1 hunk)
- docs/releaseNotes/ext/3-1-10-en.md (1 hunk)
- docs/releaseNotes/ext/3-1-10-es.md (1 hunk)
- docs/releaseNotes/ext/3-1-10-fr.md (1 hunk)
- docs/releaseNotes/ext/3-1-10-ja.md (1 hunk)
- docs/releaseNotes/ext/3-1-10-ko.md (1 hunk)
- docs/releaseNotes/ext/3-1-10-pt.md (1 hunk)
- docs/releaseNotes/ext/3-1-10-zh.md (1 hunk)
- packages/screens/src/activity/ActivityScreen.query.tsx (5 hunks)
- packages/ui/src/components/ActivityCard.tsx (1 hunk)
- packages/ui/src/index.ts (1 hunk)
Actionable Comments (0)
Skipped Comments (3)
-
packages/screens/src/activity/ActivityScreen.query.tsx [82-82]
possible bug: "Potential memory leak with uncleared timeout."
-
packages/screens/src/activity/ActivityScreen.query.tsx [101-101]
possible bug: "Missing error handling for URL opening."
-
packages/ui/src/components/ActivityCard.tsx [158-160]
possible issue: "Unconditional onPress prop may change component behavior."
GitHub Actions
Inactive
GitHub Actions
Inactive
There was a problem hiding this comment.
✅ LGTM!
Review Summary
Commits Considered (1)
- 92f12b0: feat: convert activity detail from sheet to full screen
Replace bottom sheet with full-screen navigation for activity details:
- Add ActivityDetailScreen as standalone full-screen component
- Update ActivityScreen to navigate to detail screen on item press
- Add ActivityDetail route to RN app navigator with dynamic header title
- Header shows Sent/Received/App Interaction based on transaction type
Closes #1195
Co-Authored-By: Claude Opus 4.5 noreply@anthropic.com
Files Processed (5)
- apps/react-native/src/navigation/AppNavigator.tsx (4 hunks)
- packages/screens/src/activity/ActivityDetailScreen.query.tsx (1 hunk)
- packages/screens/src/activity/ActivityScreen.query.tsx (1 hunk)
- packages/screens/src/activity/index.ts (1 hunk)
- packages/screens/src/index.ts (1 hunk)
Actionable Comments (0)
Skipped Comments (4)
-
packages/screens/src/activity/ActivityDetailScreen.query.tsx [41-45]
enhancement: "Hardcoded locale in date formatting ignores user preferences."
-
packages/screens/src/activity/ActivityDetailScreen.query.tsx [160-160]
possible bug: "Missing error handling for external URL opening."
-
packages/screens/src/activity/ActivityDetailScreen.query.tsx [246-252]
possible issue: "Hardcoded fee values may not reflect actual transaction fees."
-
packages/screens/src/activity/ActivityDetailScreen.query.tsx [221-221]
possible bug: "Potential display of undefined address values."
GitHub Actions
Inactive
GitHub Actions
Inactive
There was a problem hiding this comment.
✅ LGTM!
Review Summary
Commits Considered (1)
- 5671cdc: fix: remove network fee row from activity detail screen
Network fee data is not available from the API.
Closes #1195
Co-Authored-By: Claude Opus 4.5 noreply@anthropic.com
Files Processed (1)
- packages/screens/src/activity/ActivityDetailScreen.query.tsx (1 hunk)
Actionable Comments (0)
Skipped Comments (4)
-
packages/screens/src/activity/ActivityDetailScreen.query.tsx [244-251]
possible issue: "Hardcoded transaction fee values may not reflect actual transaction costs."
-
packages/screens/src/activity/ActivityDetailScreen.query.tsx [40-46]
enhancement: "Date formatting uses hardcoded locale instead of user's locale."
-
packages/screens/src/activity/ActivityDetailScreen.query.tsx [220-220]
possible bug: "Missing null check for sender/receiver addresses before truncation."
-
packages/screens/src/activity/ActivityDetailScreen.query.tsx [195-196]
maintainability: "Type assertion
as anyreduces type safety."
GitHub Actions
Inactive
GitHub Actions
Inactive
There was a problem hiding this comment.
✅ LGTM!
Review Summary
Commits Considered (1)
- 528af0f: feat: add NFT support to activity detail screen with translations
Add NFT-specific display support for activity detail screen:
- Display NFT title as "NFTS from {collection}"
- Show NFT count with +/- prefix
- Add Account row for profile info
- Add missing translations to all locale files (en, es, jp, ru, zh)
Co-Authored-By: Claude Opus 4.5 noreply@anthropic.com
Closes #1195
Files Processed (6)
- packages/screens/src/activity/ActivityDetailScreen.query.tsx (1 hunk)
- packages/screens/src/locales/en.json (1 hunk)
- packages/screens/src/locales/es.json (1 hunk)
- packages/screens/src/locales/jp.json (1 hunk)
- packages/screens/src/locales/ru.json (1 hunk)
- packages/screens/src/locales/zh.json (1 hunk)
Actionable Comments (0)
Skipped Comments (5)
-
packages/screens/src/activity/ActivityDetailScreen.query.tsx [40-45]
possible issue: "Date formatting ignores user's locale preference."
-
packages/screens/src/activity/ActivityDetailScreen.query.tsx [81-82]
maintainability: "Unused translation variable for NFT count."
-
packages/screens/src/activity/ActivityDetailScreen.query.tsx [148-148]
best practice: "Manual string interpolation instead of using i18n interpolation."
-
packages/screens/src/activity/ActivityDetailScreen.query.tsx [338-345]
possible issue: "Hardcoded transaction fee values may not reflect actual fees."
-
packages/screens/src/activity/ActivityDetailScreen.query.tsx [153-153]
possible issue: "Hardcoded NFT/NFTS strings bypass i18n."
GitHub Actions
Inactive
GitHub Actions
Inactive
There was a problem hiding this comment.
✅ LGTM!
Review Summary
Commits Considered (1)
-
be9040c: fix(react-native): fix missing and mismatched i18n translation keys
-
Add missing activity.loadMore and claim.title keys to all 5 locales
-
Fix ActivityScreen to use existing activity.empty/error/common.retry keys
-
Add ActivityCardLabels prop to ActivityCard for translated status/direction labels
-
Fix ClaimTokenDetailScreen claim.nft.* keys to use existing claimNFTDetail.* keys
Closes #1195
Files Processed (9)
- packages/screens/src/activity/ActivityScreen.query.tsx (4 hunks)
- packages/screens/src/locales/en.json (2 hunks)
- packages/screens/src/locales/es.json (2 hunks)
- packages/screens/src/locales/jp.json (2 hunks)
- packages/screens/src/locales/ru.json (2 hunks)
- packages/screens/src/locales/zh.json (2 hunks)
- packages/screens/src/tokens/ClaimTokenDetailScreen.query.tsx (3 hunks)
- packages/ui/src/components/ActivityCard.tsx (6 hunks)
- packages/ui/src/types/index.ts (1 hunk)
Actionable Comments (0)
Skipped Comments (3)
-
packages/screens/src/activity/ActivityScreen.query.tsx [55-62]
possible issue: "Memoization dependency may not trigger re-computation on language change."
-
packages/screens/src/activity/ActivityScreen.query.tsx [160-161]
possible issue: "Missing fallback values for translation keys."
-
packages/screens/src/activity/ActivityScreen.query.tsx [171-171]
possible issue: "Missing fallback value for translation key."
There was a problem hiding this comment.
✅ LGTM!
Review Summary
Commits Considered (1)
-
34d62af: chore(react-native): remove unused ClaimSenderRow component and dead translation keys
-
Delete ClaimSenderRow which was never exported or imported
-
Remove unused activity.today, activity.yesterday from all locales
-
Remove unused claim.drawer.from, claim.drawer.token from all locales
-
Remove unused claimNFTDetail.claimButton from all locales
Closes #1195
Files Processed (6)
- packages/screens/src/locales/en.json (4 hunks)
- packages/screens/src/locales/es.json (4 hunks)
- packages/screens/src/locales/jp.json (4 hunks)
- packages/screens/src/locales/ru.json (4 hunks)
- packages/screens/src/locales/zh.json (4 hunks)
- packages/ui/src/components/ClaimSenderRow.tsx (1 hunk)
Actionable Comments (0)
Skipped Comments (0)
There was a problem hiding this comment.
🚨 Pull request needs attention.
Review Summary
Commits Considered (1)
- fcf1b6b: fix(extension): broaden transaction success status regex in cadence and ft tests
Transactions can settle as 'success' or 'Finalized' before the polling
window, causing the /Executed|Sealed/ regex to miss them. Use the same
comprehensive pattern already used by nft-transaction tests.
Closes #1195
Files Processed (2)
- apps/extension/e2e/transaction/cadence-transaction.test.ts (3 hunks)
- apps/extension/e2e/transaction/ft-transaction.test.ts (1 hunk)
Actionable Comments (1)
-
apps/extension/e2e/transaction/cadence-transaction.test.ts [33-33]
typo: "Typo in parameter name."
Skipped Comments (1)
-
apps/extension/e2e/transaction/cadence-transaction.test.ts [33-33]
possible issue: "Inconsistent casing in regex pattern."
| const txId = await waitForTransaction({ page, successtext: /Executed|Sealed/, ingoreFlowCharge }); | ||
| const txId = await waitForTransaction({ | ||
| page, | ||
| successtext: /success|Finalized|Executed|Sealed/, |
There was a problem hiding this comment.
There's a typo in the parameter name ingoreFlowCharge - it should be ignoreFlowCharge. This typo appears to be pre-existing in the codebase, but it's being propagated in the new code.
There was a problem hiding this comment.
🚨 Pull request needs attention.
Review Summary
Commits Considered (1)
-
85ffd54: fix(extension): ensure stable data-testid and case-insensitive tx lookup
-
Use cadenceTxId || hash as the stable data-testid on activity list items
so the e2e locator survives the pending→confirmed transition -
Make pending item lookup in setTransaction case-insensitive so cadenceTxId
is always preserved regardless of API/FCL hash casing differences
Closes #1195
Files Processed (2)
- apps/extension/src/core/service/transaction-activity.ts (2 hunks)
- apps/extension/src/ui/views/TransferList/index.tsx (1 hunk)
Actionable Comments (2)
-
apps/extension/src/core/service/transaction-activity.ts [435-437]
possible bug: "Hash comparison using
includes()may cause false positive matches." -
apps/extension/src/core/service/transaction-activity.ts [448-450]
possible bug: "Hash comparison using
includes()may cause false positive matches."
Skipped Comments (0)
| item.hash.toLowerCase().includes(normalizedTxid) || | ||
| item.cadenceTxId?.toLowerCase().includes(normalizedTxid) || | ||
| item.evmTxIds?.some((id) => id.toLowerCase().includes(normalizedTxid)) |
There was a problem hiding this comment.
Using includes() for hash comparison is potentially problematic. If normalizedTxid is a substring of another hash, this could lead to false positive matches. Consider using strict equality (===) instead of includes() for hash comparisons to ensure exact matches.
| item.hash.toLowerCase().includes(normalizedTxid) || | ||
| item.cadenceTxId?.toLowerCase().includes(normalizedTxid) || | ||
| item.evmTxIds?.some((id) => id.toLowerCase().includes(normalizedTxid)) |
There was a problem hiding this comment.
Same issue as above - using includes() for hash comparison could match unintended transactions if one hash is a substring of another. Consider using strict equality (===) for reliable hash matching.
There was a problem hiding this comment.
🚨 Pull request needs attention.
Review Summary
Commits Considered (1)
- e07b6c9: fix(extension): increase e2e test timeouts from 120s to 600s
Sequential transactions each require up to 160s (pending + success wait),
making 120s insufficient for tests sending multiple transactions.
Closes #1195
Files Processed (2)
- apps/extension/e2e/transaction/cadence-transaction.test.ts (6 hunks)
- apps/extension/e2e/transaction/ft-transaction.test.ts (6 hunks)
Actionable Comments (1)
-
apps/extension/e2e/transaction/cadence-transaction.test.ts [33-33]
typo: "Typo in parameter name - inconsistent casing."
Skipped Comments (2)
-
apps/extension/e2e/transaction/cadence-transaction.test.ts [94-94]
possible issue: "Significant test timeout increase may hide underlying issues."
-
apps/extension/e2e/transaction/cadence-transaction.test.ts [33-33]
possible issue: "Inconsistent casing in regex pattern."
| const txId = await waitForTransaction({ page, successtext: /Executed|Sealed/, ingoreFlowCharge }); | ||
| const txId = await waitForTransaction({ | ||
| page, | ||
| successtext: /success|Finalized|Executed|Sealed/, |
There was a problem hiding this comment.
There's a typo in successtext - it should be successText (camelCase). This inconsistency in naming convention could cause confusion.
There was a problem hiding this comment.
✅ LGTM!
Review Summary
Commits Considered (1)
-
ed421f9: fix(extension): increase success check timeout and recover pending items after SW restart
-
Increase waitForTransaction success check timeout from 100s to 200s to
account for slow testnet confirmation times in CI -
In setTransaction, recover pending items from the cached list when the
in-memory store is empty (e.g. after MV3 service worker restart), preventing
pending transactions from disappearing from the activity list before the
indexer picks them up
Closes #1195
Files Processed (2)
- apps/extension/e2e/utils/helper.ts (1 hunk)
- apps/extension/src/core/service/transaction-activity.ts (3 hunks)
Actionable Comments (0)
Skipped Comments (2)
-
apps/extension/src/core/service/transaction-activity.ts [400-407]
enhancement: "Redundant length check before filtering."
-
apps/extension/e2e/utils/helper.ts [641-641]
maintainability: "Large timeout increase may hide underlying issues."
There was a problem hiding this comment.
✅ LGTM!
Review Summary
Commits Considered (1)
- 156e52f: fix(extension): poll activity list while transactions are pending
When pending transactions exist, trigger a background refresh every 5s.
This ensures the UI updates even when chrome.storage.onChanged events
from the service worker are not reliably delivered to the popup,
which caused the activity item to stay at 'Pending' status indefinitely
in CI environments.
Closes #1195
Files Processed (1)
- apps/extension/src/ui/hooks/useTransferListHook.ts (2 hunks)
Actionable Comments (0)
Skipped Comments (2)
-
apps/extension/src/ui/hooks/useTransferListHook.ts [28-28]
readability: "Condition may be unclear when checking for zero pending count."
-
apps/extension/src/ui/hooks/useTransferListHook.ts [36-36]
enhancement: "Effect may restart frequently if pending count changes often."
There was a problem hiding this comment.
✅ LGTM!
Review Summary
Commits Considered (1)
- 7887612: fix(extension): resolve strict mode violation in e2e activity list test
Revert data-testid from txStableId back to txCombinedKey so each
TransferItem has a unique testid (includes transferType/interaction
suffix). This prevents multiple items from the same Cadence tx sharing
identical testids, which caused Playwright strict mode violations.
Add .first() to executedItem assertion in waitForTransaction to handle
cases where multiple activity items match the txId regexp (e.g. when
ingoreFlowCharge is false and fee items are also visible).
Closes #1195
Files Processed (2)
- apps/extension/e2e/utils/helper.ts (1 hunk)
- apps/extension/src/ui/views/TransferList/index.tsx (1 hunk)
Actionable Comments (0)
Skipped Comments (1)
-
apps/extension/src/ui/views/TransferList/index.tsx [175-175]
possible issue: "Unused variable declared."
There was a problem hiding this comment.
✅ LGTM!
Review Summary
Commits Considered (1)
- 564efd7: fix(extension): add .first() to NFT activity item assertions to prevent strict mode violations
Closes #1195
Files Processed (1)
- apps/extension/e2e/utils/helper.ts (3 hunks)
Actionable Comments (0)
Skipped Comments (3)
-
apps/extension/e2e/utils/helper.ts [551-551]
possible issue: "Using
.first()may hide issues with multiple matching elements." -
apps/extension/e2e/utils/helper.ts [594-594]
possible issue: "Using
.first()may hide issues with multiple matching elements." -
apps/extension/e2e/utils/helper.ts [640-641]
maintainability: "Significant timeout increase may slow down test feedback."
There was a problem hiding this comment.
✅ LGTM!
Review Summary
Commits Considered (1)
- 3a47123: fix(extension): recover in-progress transactions (Executed/Finalized) after SW restart
Previously only PENDING items were recovered from cache when the service
worker restarted. For EVM/Cadence hybrid transactions (e.g. withdrawCoa),
listenTransaction may have already called updatePending to advance the
status to Executed or Finalized before the SW restarted. Those items were
then excluded from recovery and disappeared from the activity list, causing
the send-from-evm-to-Flow e2e test to time out waiting for a Sealed item.
Extend the recovery filter to include all non-terminal states (excludes
Sealed, Expired, Error) so Executed and Finalized items are also preserved
across SW restarts until the indexer confirms them.
Closes #1195
Files Processed (1)
- apps/extension/src/core/service/transaction-activity.ts (3 hunks)
Actionable Comments (0)
Skipped Comments (2)
-
apps/extension/src/core/service/transaction-activity.ts [405-412]
possible bug: "Mutating the pending list array directly may cause side effects."
-
apps/extension/src/core/service/transaction-activity.ts [407-407]
possible bug: "Missing null check before calling toUpperCase() on status."
There was a problem hiding this comment.
✅ LGTM!
Review Summary
Commits Considered (1)
- 5fa8621: fix(extension): guard against undefined status in pending item recovery
Closes #1195
Files Processed (1)
- apps/extension/src/core/service/transaction-activity.ts (3 hunks)
Actionable Comments (0)
Skipped Comments (2)
-
apps/extension/src/core/service/transaction-activity.ts [405-412]
possible bug: "Mutating the pending list in-place may cause unintended side effects."
-
apps/extension/src/core/service/transaction-activity.ts [404-404]
performance: "Constant defined inside function scope could be moved to module level."
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Closes #1195
- Add missing activity.loadMore and claim.title keys to all 5 locales - Fix ActivityScreen to use existing activity.empty/error/common.retry keys - Add ActivityCardLabels prop to ActivityCard for translated status/direction labels - Fix ClaimTokenDetailScreen claim.nft.* keys to use existing claimNFTDetail.* keys Closes #1195
…translation keys - Delete ClaimSenderRow which was never exported or imported - Remove unused activity.today, activity.yesterday from all locales - Remove unused claim.drawer.from, claim.drawer.token from all locales - Remove unused claimNFTDetail.claimButton from all locales Closes #1195
…nd ft tests Transactions can settle as 'success' or 'Finalized' before the polling window, causing the /Executed|Sealed/ regex to miss them. Use the same comprehensive pattern already used by nft-transaction tests. Closes #1195
- Use cadenceTxId || hash as the stable data-testid on activity list items so the e2e locator survives the pending→confirmed transition - Make pending item lookup in setTransaction case-insensitive so cadenceTxId is always preserved regardless of API/FCL hash casing differences Closes #1195
Sequential transactions each require up to 160s (pending + success wait), making 120s insufficient for tests sending multiple transactions. Closes #1195
…ems after SW restart - Increase waitForTransaction success check timeout from 100s to 200s to account for slow testnet confirmation times in CI - In setTransaction, recover pending items from the cached list when the in-memory store is empty (e.g. after MV3 service worker restart), preventing pending transactions from disappearing from the activity list before the indexer picks them up Closes #1195
When pending transactions exist, trigger a background refresh every 5s. This ensures the UI updates even when chrome.storage.onChanged events from the service worker are not reliably delivered to the popup, which caused the activity item to stay at 'Pending' status indefinitely in CI environments. Closes #1195
Revert data-testid from txStableId back to txCombinedKey so each TransferItem has a unique testid (includes transferType/interaction suffix). This prevents multiple items from the same Cadence tx sharing identical testids, which caused Playwright strict mode violations. Add .first() to executedItem assertion in waitForTransaction to handle cases where multiple activity items match the txId regexp (e.g. when ingoreFlowCharge is false and fee items are also visible). Closes #1195
…nt strict mode violations Closes #1195
… after SW restart Previously only PENDING items were recovered from cache when the service worker restarted. For EVM/Cadence hybrid transactions (e.g. withdrawCoa), listenTransaction may have already called updatePending to advance the status to Executed or Finalized before the SW restarted. Those items were then excluded from recovery and disappeared from the activity list, causing the send-from-evm-to-Flow e2e test to time out waiting for a Sealed item. Extend the recovery filter to include all non-terminal states (excludes Sealed, Expired, Error) so Executed and Finalized items are also preserved across SW restarts until the indexer confirms them. Closes #1195
When the service worker restarts, all in-memory FCL onceExecuted/onceSealed promise chains are lost. Transactions that were pending at restart time are left permanently stuck in a non-terminal status (PENDING, Executed, Finalized) because the EVM transfers API does not return Cadence transactions like withdrawCoa. Add resumePendingTransactions to userWalletService which runs after init and re-attaches FCL monitoring for any cached non-terminal transactions. This uses a direct FCL subscription (reattachFclMonitoring) that updates the cached store via updatePending without duplicating the pending item. Closes #1195 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
🚨 Pull request needs attention.
Review Summary
Commits Considered (1)
- eea7e13: fix(extension): resume FCL monitoring for pending txs after SW restart
When the service worker restarts, all in-memory FCL onceExecuted/onceSealed
promise chains are lost. Transactions that were pending at restart time are
left permanently stuck in a non-terminal status (PENDING, Executed, Finalized)
because the EVM transfers API does not return Cadence transactions like
withdrawCoa.
Add resumePendingTransactions to userWalletService which runs after init and
re-attaches FCL monitoring for any cached non-terminal transactions. This uses
a direct FCL subscription (reattachFclMonitoring) that updates the cached
store via updatePending without duplicating the pending item.
Closes #1195
Co-Authored-By: Claude Sonnet 4.6 noreply@anthropic.com
Files Processed (3)
- apps/extension/src/background/index.ts (1 hunk)
- apps/extension/src/core/service/transaction-activity.ts (4 hunks)
- apps/extension/src/core/service/userWallet.ts (1 hunk)
Actionable Comments (1)
-
apps/extension/src/core/service/userWallet.ts [1198-1202]
possible issue: "Potential indefinite hang if FCL promises never resolve."
Skipped Comments (4)
-
apps/extension/src/background/index.ts [108-108]
best practice: "Silent error swallowing hides potential issues."
-
apps/extension/src/core/service/transaction-activity.ts [404-404]
maintainability: "Duplicated constant definition."
-
apps/extension/src/core/service/userWallet.ts [1182-1182]
best practice: "Silent error swallowing in loop."
-
apps/extension/src/core/service/userWallet.ts [1194-1194]
possible bug: "Regex validation may be too permissive."
| const txStatusExecuted = await fclTx.onceExecuted(); | ||
| await transactionActivityService.updatePending(network, address, txId, txStatusExecuted); | ||
| const txStatusSealed = await fclTx.onceSealed(); | ||
| await transactionActivityService.updatePending(network, address, txId, txStatusSealed); | ||
| await transactionActivityService.pollTransferList(address, txId, network); |
There was a problem hiding this comment.
If fclTx.onceExecuted() or fclTx.onceSealed() never resolves (e.g., transaction is already in a terminal state or network issues), this function will hang indefinitely. Consider adding a timeout mechanism to prevent blocking.
unlock() forcibly switches FCL to mainnet regardless of the wallet's persisted network. listenTransaction() then calls fcl.tx(txId).onceExecuted() while FCL is still configured for mainnet, so testnet transactions are never found and onceExecuted/onceSealed never resolve. This leaves EVM-to-Flow (withdrawCoa) transactions permanently stuck in PENDING status because getEVMTransfers also does not return Cadence txs. Add fclEnsureNetwork(network) before the fcl.tx() call in both listenTransaction and reattachFclMonitoring to guarantee FCL is on the right network before subscribing to transaction status events. Closes #1195 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
✅ LGTM!
Review Summary
Commits Considered (1)
- e889f5a: fix(extension): ensure FCL uses correct network before tx monitoring
unlock() forcibly switches FCL to mainnet regardless of the wallet's
persisted network. listenTransaction() then calls fcl.tx(txId).onceExecuted()
while FCL is still configured for mainnet, so testnet transactions are
never found and onceExecuted/onceSealed never resolve. This leaves
EVM-to-Flow (withdrawCoa) transactions permanently stuck in PENDING
status because getEVMTransfers also does not return Cadence txs.
Add fclEnsureNetwork(network) before the fcl.tx() call in both
listenTransaction and reattachFclMonitoring to guarantee FCL is on
the right network before subscribing to transaction status events.
Closes #1195
Co-Authored-By: Claude Sonnet 4.6 noreply@anthropic.com
Files Processed (1)
- apps/extension/src/core/service/userWallet.ts (2 hunks)
Actionable Comments (0)
Skipped Comments (2)
-
apps/extension/src/core/service/userWallet.ts [1204-1204]
possible bug: "Regex pattern may match invalid transaction IDs."
-
apps/extension/src/core/service/userWallet.ts [1192-1192]
maintainability: "Silent error swallowing hinders debugging."
2 participants
Closes #1195
Note: do not merge until the mock data is removed
🔗 Related Issues
Linked automatically from the branch name. If incorrect, edit:
Self-Checklist
📝 Description
📸 Screenshots/Videos