Repository navigation
Conversation
n13
left a comment
There was a problem hiding this comment.
Reviewer model: GPT-6.1 Sol
REQUEST_CHANGES — keep transfer outcomes visible when simplifying the transaction tables.
[P2] Preserve the status indicator for unified transaction rows — UNIFIED_LIST_TRANSACTION_COLUMNS.tsx:97–102
The list queries still include scheduled, executed, and cancelled reversible transfers, but deleting the Status cell leaves no visible outcome or transaction-type indicator in the global, account, and multisig transaction tables. In account activity, createAccountUnifiedTransactionColumns also continues to render a green IN badge and the amount for a cancelled or merely scheduled incoming transfer. A base/head rendering comparison with otherwise identical fixtures confirms that all three outcomes now have identical visible cells (IN, 1 QTC, etc.); the base shows Scheduled, Executed, or Cancelled. This makes a transfer that never completed look like a completed credit. Retain an outcome badge, either as a column or within an existing cell, so users can distinguish completed transfers from pending/cancelled ones without opening each detail page.
Reviewed base ec933ec → head 63b37c5, including all 19 changed files and affected callers.
Validation:
- Frozen-lockfile dependency installation, production build/TypeScript check,
git diff --check, and Prettier checks for all changed files passed. - Actual base/head column-rendering reproduction confirmed the outcome loss described above.
- Raw
bun test: 173 passed, 3 failed from DOM cleanup in unchanged component suites. With a temporaryafterEach(cleanup)preload, all 176 tests passed; no tracked source files were changed. - Both GitHub checks passed. The workflow named “Build and Test” runs the build; its test step is disabled.
n13
left a comment
There was a problem hiding this comment.
Reviewer model: Opus 5.5
Nothing here blocks the merge. tsc --noEmit passes on the PR head. The lint errors in the touched files are all on lines this PR did not change. Jest cannot run in this repo right now. jest.setup.ts fails to parse in every suite, and that problem is older than this PR.
Findings, most severe first:
-
src/components/common/table-columns/EXTRINSIC_COLUMNS.tsx:93. The block's extrinsic list no longer shows which extrinsics failed, because the Result column is gone.createExtrinsicColumnsonly feeds the block detail table, and nothing else in that table marks a failure. Users now have to open each extrinsic to find a failed one. If this is intended, also dropsuccessfromGetBlockById(src/api/blocks.tsx:81) andBlockExtrinsic(src/schemas/blocks.ts:20). Nothing reads it now. -
src/components/features/multisig-details/MultisigDetailStatusFilter.tsx:21. Clicking the status button that is already active now breaks scroll restore on Back. The URL does not change, so router-core takes its same-URL branch. That branch still setsresetNextScroll = false.onRendereddoes not fire, so the flag is never reset. On the next browser Back, the scroll handler sees the stale flag and skipsrestoreScroll. The previous page then keeps the wrong scroll position. Fix: return early whennextStatus === status. The tabs are not affected, because Radix does not fireonValueChangefor the active tab. -
src/components/features/multisig-listing/MultisigDataTabs.tsx:28andMultisigStatusFilter.tsx:26. The/multisiglisting page has the same tab and status navigation, withMultisigStatsabove it. It did not getresetScroll: false, so switching tab or status there still jumps to the top. These two components are near copies of the detail ones. One shared implementation would stop the fixes from drifting apart. -
src/components/common/table-columns/UNIFIED_LIST_TRANSACTION_COLUMNS.tsx:122. Without the fallback, an unknown status throwsCannot read properties of undefined (reading 'variant'). The router error page then replaces the whole transactions page. Hasura typesstatusasString, so the TS union is only a cast. The indexer enum has exactly these five values today, so nothing triggers it yet. Failing loudly is fine, but throw an error that names the status so the cause is clear. Note thatMULTISIG_PROPOSAL_COLUMNS.tsx:29still falls back to'miner', so the two tables now behave differently. -
Removed sortable columns still sort.
useOrderBypasses anyorder_byfrom the URL straight to GraphQL. An old link with?order_by=fee:desc,deposit:ascorexpiry_block:descstill sorts by a column the user cannot see, and no header shows an arrow. This mechanism existed before, but this PR turns valid links into hidden sorts. Removefeefromsrc/constants/query-sorts/unified-list-transactions.ts:6,19,29anddeposit/expiry_blockfromsrc/constants/query-sorts/multisig-proposal.ts:6,8,15,19. The*_SORTS_LITERALSarrays are not imported anywhere, so they validate nothing. -
Unused fields are still fetched.
feeinUNIFIED_TX_FIELDS(src/api/unified-transactions.tsx:40) goes into every unified list query and the landing page, and nothing reads it.depositandexpiry_blockinMultisigProposalListFields(src/api/multisig-proposals.tsx:19-20) are now only needed by the detail fragment. -
src/components/ui/composites/account-address-cell/AccountAddressCell.tsx:42.truncate = trueis now the minority case. 46 call sites passtruncate={false}, and the 19 that use the default are all in table columns. Flip the default tofalseto matchLinkWithCopy, and passtruncatein the table columns. Otherwise every new detail page has to remember the flag. The same "cell orEmptyValue" JSX is also repeated about 22 times across the detail pages.accountLinkandwalletLinkinMultisigProposalInformation.tsx:80-98already wrap it. Moving them to a shared module would have made this a two-line change. -
Some classes now do nothing.
className="font-mono text-flare"atUNIFIED_LIST_TRANSACTION_COLUMNS.tsx:83,100repeatsLinkWithCopy's defaults, so the prop can go.className="break-all"next to the newtruncate={false}is also redundant, becauseLinkWithCopyalready addsbreak-allwhen truncation is off. This affects the reversible-transaction, miner-reward and deposits-claimed pages.
Verdict: comment. There are no blocking bugs. Please confirm that hiding extrinsic failures on the block page is intended, and consider the one-line fix for #2.
|
Closing in favor of #58 |
No description provided.