Skip to content

refactor transaction details page items#1945

Closed
basantagoswami wants to merge 3 commits into
developfrom
feat/swap-tx-details-improvements
Closed

refactor transaction details page items#1945
basantagoswami wants to merge 3 commits into
developfrom
feat/swap-tx-details-improvements

Conversation

@basantagoswami

Copy link
Copy Markdown
Contributor

No description provided.

@basantagoswami basantagoswami linked an issue Mar 16, 2026 that may be closed by this pull request
11 tasks
@basantagoswami basantagoswami force-pushed the feat/swap-tx-details-improvements branch from e1060d7 to fd2faf3 Compare May 21, 2026 12:38
@basantagoswami

basantagoswami commented May 21, 2026

Copy link
Copy Markdown
Contributor Author

I need to add a screenshot here to make it easy to review

@basantagoswami basantagoswami marked this pull request as ready for review May 21, 2026 12:45
@basantagoswami basantagoswami force-pushed the feat/swap-tx-details-improvements branch 3 times, most recently from 831bbbd to 60f7ddf Compare May 25, 2026 09:10
@thibistaken thibistaken requested review from ethicnology and i5hi and removed request for i5hi May 25, 2026 09:41
@ethicnology

Copy link
Copy Markdown
Member

Hey @claude do you spot any bugs or regression among these changes?

@claude

claude Bot commented May 28, 2026

Copy link
Copy Markdown

Claude finished @ethicnology's task in 4m 39s —— View job


Review: bugs & regressions

I focused on the 3 PR commits (6ba3443, 60f7ddf, 3741fae) against develop. The refactor itself is clean — separating non-swap / swap / payjoin / order paths makes the file much easier to follow, and pushing transactional sections behind Swap/Transfer/Liquid/Bitcoin headers is a nice UX improvement. A few concerns:


🐛 1. LN swap address explorer link routes to the wrong network on Liquid wallets

lib/features/transactions/ui/widgets/transaction_details_table.dart:874

final addressIsLiquid = swap.isChainSwap ? !isLiquid : false;
return addressIsLiquid
    ? locator<MempoolUrlBuilder>().liquidAddress(toAddress, ...)
    : locator<MempoolUrlBuilder>().bitcoinAddress(toAddress, ...);

For LnSendSwap / LnReceiveSwap on a Liquid wallet (an LBTC ↔ LN swap), the address shown (swap.receiveAddress for receive, or transaction.toAddress lockup for send) is a Liquid address, but addressIsLiquid is hardcoded to false, so tapping routes to the Bitcoin mempool, which won't find the address. The old AddressViewer widget side-stepped this entirely by auto-detecting the network via Satoshifier.

Fix idea: derive addressIsLiquid from the swap type, or detect from the address itself. A simple proxy: for LN swaps, addressIsLiquid = isLiquid.

Fix this →


⚠️ 2. Regression: non-swap address loses interactive viewer

transaction_details_table.dart:223-228

Old code wrapped toAddress in AddressViewer, which on tap opened a dialog with copy address, copy explorer link, and view in explorer actions, plus BIP21 awareness. The new non-swap path drops that widget and just shows truncated text + the default right-edge copy icon — there is no longer any way to open the address in mempool from the transaction details page. The swap path got the link via _mempoolLink, but the non-swap path didn't.

Either restore AddressViewer here, or wrap with _mempoolLink as well.


⚠️ 3. Minor regression: txId tap UX is downgraded

transaction_details_table.dart:188-202 (and the liquid/bitcoin item lists)

Previously TransactionViewer.bitcoin / .liquid opened a dialog with copy/explore actions on tap. The new _mempoolLink does a direct launchUrl with no dialog. The link still works, but the user loses the copy-link / per-txid copy flow. If this was intentional simplification, ignore; otherwise consider keeping TransactionViewer.


Nits

  • Dead displayValue when displayWidget is setDetailsTableItem prefers displayWidget, so displayValue: StringFormatting.truncateMiddle(...) on lines 89, 138, 186, 868 is unused and can be removed.
  • app_zh.arb transactionDetailLabelBitcoinTxId: "页: 1" ("Page: 1") looks like a pre-existing bad translation — not introduced here, just flagging.
  • The new section headers are nicely scoped, but _sectionHeader and _mempoolLink are file-private top-level functions; matches the existing _feeRow style, so consistent.

Verified non-issues

  • Removed transactionDetailLabelTransactionId key is no longer referenced anywhere (grep -r in lib/ + localization/ confirms clean removal).
  • liquidTxId / bitcoinTxId derivation correctly handles both directions of chain swaps and LN swaps where counterpartWallet is null.
  • The new if (swap == null) wrapping makes the previous && swap == null guard on the transaction-fee row implicit but still correct.
  • transactionNetworkLiquid, transactionNetworkBitcoin, transactionDetailSectionSwap, transactionDetailSectionTransfer are all defined in app_en.arb.

  • Read main changed file
  • Compare against develop
  • Check localization
  • Identify bugs / regressions
  • Post review
    • Branch: feat/swap-tx-details-improvements

@ethicnology

Copy link
Copy Markdown
Member

Hey @basantagoswami we need your answers regarding claude suggestions

@basantagoswami basantagoswami force-pushed the feat/swap-tx-details-improvements branch from 3741fae to 016f317 Compare May 29, 2026 11:04
@ethicnology

Copy link
Copy Markdown
Member

@claude are you satisfied ?

@claude

claude Bot commented Jun 3, 2026

Copy link
Copy Markdown

Claude finished @ethicnology's task in 1m 13s —— View job


Not fully satisfied yet — 2 of my 3 earlier points still stand

I re-reviewed after the two new commits (e391181 dedupe mempool link, 016f317 fix missing improvements). The dedupe via _mempoolLink is a nice cleanup and the txId now has explorer links in both the swap and non-swap paths — so my old concern there is resolved. But two items remain:


🐛 1. LN swap address link still routes to the wrong network — NOT fixed

transaction_details_table.dart:910

final addressIsLiquid = swap.isChainSwap ? !isLiquid : false;

For LnSendSwap / LnReceiveSwap on a Liquid wallet (LBTC ↔ LN), toAddress (swap.receiveAddress or the lockup address) is a Liquid address, but the non-chain branch is still hardcoded to false, so the link opens the Bitcoin mempool and won't find it. For LN swaps the address sits on the same network as the wallet, so it should follow isLiquid:

final addressIsLiquid = swap.isChainSwap ? !isLiquid : isLiquid;

Fix this →


⚠️ 2. Non-swap address still has no explorer link — NOT fixed

transaction_details_table.dart:223-228

if (toAddress != null)
  DetailsTableItem(
    label: context.loc.transactionDetailLabelAddress,
    displayValue: StringFormatting.truncateMiddle(toAddress),
    copyValue: toAddress,
  ),

The swap path got a _mempoolLink for the address (line 906), but the non-swap path still only has copy — no way to open the recipient address in the explorer, which the old AddressViewer provided. Inconsistent with the swap path. Either wrap this in _mempoolLink too, or this was an intentional drop — worth confirming.


ℹ️ 3. txId tap (was a question) — acceptable

_mempoolLink now does a direct launchUrl instead of the old TransactionViewer dialog. Copy is still available via the row's copy icon, so this is a fine simplification — no action needed unless you want the dialog back.


Nit (optional)

displayValue is dead when displayWidget is set (lines 89, 138, 186, 904) — DetailsTableItem prefers displayWidget. Harmless, but can be dropped.


Verdict: the refactor is solid and reads much better. I'd want #1 fixed before merge (it's a real wrong-network bug for Liquid LN swaps), and a quick decision on #2 (restore the link or confirm it's intentional). Happy to push the one-line fix for #1 if you want — just say the word.
• Branch: feat/swap-tx-details-improvements

@basantagoswami

Copy link
Copy Markdown
Contributor Author

Closing this PR, working on the redesign of the screen and will raise another PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

In Transaction Details Page, Make Improvements

2 participants