Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(TransactionFeedV2): Fix persisted feed storing unmerged transactions #6205

Merged
merged 2 commits into from
Nov 5, 2024
Merged

fix(TransactionFeedV2): Fix persisted feed storing unmerged transactions #6205

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5e11789
fix persisted feed storing unmerged transactions
sviderock Nov 1, 2024
5e1a615
add space just to trigger PR checks re-run
sviderock Nov 4, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 14 additions & 1 deletion src/transactions/feed/TransactionFeedV2.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ import TokenApprovalFeedItem from 'src/transactions/feed/TokenApprovalFeedItem'
import TransferFeedItem from 'src/transactions/feed/TransferFeedItem'
import NoActivity from 'src/transactions/NoActivity'
import { allStandbyTransactionsSelector, feedFirstPageSelector } from 'src/transactions/selectors'
import { updateFeedFirstPage } from 'src/transactions/slice'
import {
FeeType,
TokenTransactionTypeV2,
Expand Down Expand Up @@ -203,7 +204,8 @@ function mergeStandByTransactionsInRange({
return inRange || newTransaction || veryOldTransaction
})
const deduplicatedTransactions = deduplicateTransactions([...transactions, ...standByInRange])
const transactionsFromAllowedNetworks = deduplicatedTransactions.filter((tx) =>
const sortedTransactions = sortTransactions(deduplicatedTransactions)
Copy link
Contributor Author

@sviderock sviderock Nov 1, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was also one of the issues as initial persisted feed appeared unsorted therefore also triggering the rearrangement. It does add up an extra sorting loop, but this shouldn't be a major performance issue for now.

const transactionsFromAllowedNetworks = sortedTransactions.filter((tx) =>
allowedNetworks.includes(tx.networkId)
)

Expand Down Expand Up @@ -427,6 +429,17 @@ export default function TransactionFeedV2() {
[newlyCompletedCrossChainSwaps]
)

useEffect(
function updatePersistedFeedFirstPage() {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if i understand this change correctly, the difference lies in that we want to store firstPageData instead of the received first page data in redux because firstPageData includes the sorted and merged standby transactions? if that is the case, aren't we storing the standby transactions potentially twice?

is it possible instead for the feedFirstPageSelector to return the first page that is merged with the standby transactions?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kathaypacific

  1. With the implementation from this PR, duplicates will be present but only for pending and failed transactions. Once pending turn to completed - they will get removed from stand by, therefore duplicates will only exist for failed ones but only for the first page of the feed anyways.
  2. Yes, it is possible!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kathaypacific Argh, it seems like if I move this to the selector - I won't be able to pass isLastPage to the mergeStandByTransactionsInRange function. The rest of un-merged stand by transactions are always pushed to the very bottom of the last page but for that we need to somehow figure out if the page is last which is impossible from the selector level.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hum, okay. i don't have a better suggestion but am slightly concerned that these small details are causing some complexity / workarounds that we will later forget about. i can see us potentially reintroducing these bugs during cleanup initiatives. perhaps @jeanregisser has a suggestion here, or leaving a small comment with a link to your comment explanation above is sufficient for now

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kathaypacific there are a few potential implementation changes that I have in mind but they involve a bit more time to implement as it will most likely require the overall change of managing paginationData on the side of Redux rather than on the component level. In that case, we'll have most of the logic on the reducer level and no duplication.

const isFirstPage = !data?.pageInfo.hasPreviousPage
if (isFirstPage) {
const firstPageData = paginatedData[FIRST_PAGE_CURSOR]
dispatch(updateFeedFirstPage({ transactions: firstPageData }))
}
},
[paginatedData, data?.pageInfo]
)

const confirmedTransactions = useMemo(() => {
const flattenedPages = Object.values(paginatedData).flat()
const deduplicatedTransactions = deduplicateTransactions(flattenedPages)
Expand Down
1 change: 1 addition & 0 deletions src/transactions/selectors.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,4 +118,5 @@ export const pendingStandbyTxHashesByNetworkIdSelector = createSelector(
)

const feedFirstPage = (state: RootState) => state.transactions.feedFirstPage

export const feedFirstPageSelector = createSelector(feedFirstPage, (feed) => feed)
18 changes: 11 additions & 7 deletions src/transactions/slice.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -204,6 +204,11 @@ const slice = createSlice({
standbyTransactions: updatedStandbyTransactions,
}
},

updateFeedFirstPage: (state, action: PayloadAction<{ transactions: TokenTransaction[] }>) => ({
...state,
feedFirstPage: action.payload.transactions,
}),
},

extraReducers: (builder) => {
Expand All @@ -221,10 +226,6 @@ const slice = createSlice({
* Whenever we get new data from the feed pagination - we need to perform updates on some portion
* of our reducer data, as side-effects. These scenarios include:
*
* - Updating "feedFirstPage" whenever we get new data for the first page. We use this to instantly
* show the user something that can be interacted with while we're actually refetching the latest
* state in the background.
*
* - In order to avoid bloating stand by transactions with confirmed transactions that are already
* present in the feed via pagination – we need to clean them up. This must run for every page
* as standByTransaction might include very old transactions. We should use the chance whenever
Expand All @@ -233,14 +234,12 @@ const slice = createSlice({
builder.addMatcher(
transactionFeedV2Api.endpoints.transactionFeedV2.matchFulfilled,
(state, { payload, meta }) => {
const isFirstPage = meta.arg.originalArgs.endCursor === undefined
const confirmedTransactionsFromNewPage = payload.transactions
.filter((tx) => tx.status !== TransactionStatus.Pending)
.map((tx) => tx.transactionHash)

return {
...state,
feedFirstPage: isFirstPage ? payload.transactions : state.feedFirstPage,
standbyTransactions: state.standbyTransactions.filter((tx) => {
/**
* - ignore empty hashes as there's no way to compare them
Expand All @@ -256,7 +255,12 @@ const slice = createSlice({
},
})

export const { addStandbyTransaction, transactionConfirmed, updateTransactions } = slice.actions
export const {
addStandbyTransaction,
transactionConfirmed,
updateTransactions,
updateFeedFirstPage,
} = slice.actions

export const { actions } = slice

Expand Down