-
Notifications
You must be signed in to change notification settings - Fork 4k
Confirm new expenses with a “View” toast instead of scrolling to and highlighting the row #96496
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.
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
Open
mhawryluk
wants to merge
26
commits into
Expensify:main
Choose a base branch
from
software-mansion-labs:declarative-expense-added-growl
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
26 commits
Select commit
Hold shift + click to select a range
49cca88
Confirm new expenses with a “View” toast instead of scrolling to and …
mhawryluk 066854f
Merge branch 'main' into declarative-expense-added-growl
mhawryluk 956d2a2
Fix tests
mhawryluk b3e03b2
Invoke findSelfDMReportID only when a transaction is active in Expen…
mhawryluk a9be08a
Add comment for latestTransactionID last-created assumption
mhawryluk 62ee8e8
Clear EXPENSE_ADDED_GROWL_TRANSACTION_IDS on new session
mhawryluk e53cdd3
Add key to ensure remounting GrowlNotificationContent in ExpenseAdded…
mhawryluk ac42067
Remove mergeExpenseAddedGrowlTransactionIDs call from navigateAfterEx…
mhawryluk 698268c
Remove useAnimatedHighlightStyle usage from Spend table
mhawryluk e863597
Merge branch 'main' into declarative-expense-added-growl
mhawryluk ca1593a
Remove unused handleNavigateAfterExpenseCreate
mhawryluk 569cb42
Fix not showing growl when submitting photo from gallery
mhawryluk 2ab5a2c
Merge branch 'main' into declarative-expense-added-growl
mhawryluk c0c93d4
Fix after merge
mhawryluk 62419b6
Fix after merge 2
mhawryluk cb74505
Rename files from useSearchHighlightAndScroll to useSearchAutoRefetch
mhawryluk 3dc7dd2
Add jsdoc to UseSearchAutoRefetch
mhawryluk 13d868f
Separate ExpenseAddedGrowlContent to remove onyx connections when no …
mhawryluk 8d64539
Derive backTo from the existing RHP's own fallback route when forceRe…
mhawryluk 42fb7e7
Add jsdoc to NavigateAfterExpenseCreateParams
mhawryluk 5e8f6bc
Add a comment about surfacing only the latest transaction id from a b…
mhawryluk 8a9d087
Update outdated ExpenseFlatSearchView comment
mhawryluk 8db62fd
Merge branch 'main' into declarative-expense-added-growl
mhawryluk 67a4612
Fix after merge
mhawryluk 83ee6a4
Merge branch 'main' into declarative-expense-added-growl
mhawryluk 57509e3
Merge branch 'main' into declarative-expense-added-growl
mhawryluk File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,178 @@ | ||
| import useCurrentUserPersonalDetails from '@hooks/useCurrentUserPersonalDetails'; | ||
| import useLocalize from '@hooks/useLocalize'; | ||
| import useOnyx from '@hooks/useOnyx'; | ||
|
|
||
| import {createTransactionThreadReport, setOptimisticTransactionThread} from '@libs/actions/Report'; | ||
| import {mergeExpenseAddedGrowlTransactionIDs} from '@libs/actions/Transaction'; | ||
| import Log from '@libs/Log'; | ||
| import {navigateToCreatedExpense} from '@libs/Navigation/helpers/navigateAfterExpenseCreate'; | ||
| import Navigation from '@libs/Navigation/Navigation'; | ||
| import {getIOUActionForTransactionID} from '@libs/ReportActionsUtils'; | ||
| import {findSelfDMReportID, isInvoiceReport, isMoneyRequestReport} from '@libs/ReportUtils'; | ||
|
|
||
| import CONST from '@src/CONST'; | ||
| import ONYXKEYS from '@src/ONYXKEYS'; | ||
| import type {SearchDataTypes} from '@src/types/onyx/SearchResults'; | ||
|
|
||
| import type {Dispatch, SetStateAction} from 'react'; | ||
|
|
||
| import {useEffect, useRef, useState} from 'react'; | ||
|
|
||
| import GrowlNotificationContent from './GrowlNotification/GrowlNotificationContent'; | ||
|
|
||
| type ActiveGrowl = { | ||
| /** ID of the transaction the growl was raised for */ | ||
| transactionID: string; | ||
|
|
||
| /** Search data type the growl belongs to (expense, invoice, etc.) */ | ||
| dataType: SearchDataTypes; | ||
|
|
||
| /** Identifies each growl instance so a stale slide-out dismissal can't clear a newer growl */ | ||
| nonce: number; | ||
| }; | ||
|
|
||
| type ExpenseAddedGrowlContentProps = { | ||
| /** Transaction to read for the growl: the active growl's transaction, or the latest pending signal ID */ | ||
| transactionID: string; | ||
|
|
||
| /** Pending expense-added notifications, keyed by transaction ID with its search data type as the value */ | ||
| signal: Record<string, SearchDataTypes> | undefined; | ||
|
|
||
| /** Currently displayed growl */ | ||
| active: ActiveGrowl | null; | ||
|
|
||
| /** Setter for the active growl */ | ||
| setActive: Dispatch<SetStateAction<ActiveGrowl | null>>; | ||
| }; | ||
|
|
||
| /** Watches the "an expense was just added" Onyx signal and shows an "Expense added" growl with a "View" action. */ | ||
| function ExpenseAddedGrowl() { | ||
| const [active, setActive] = useState<ActiveGrowl | null>(null); | ||
| const [signal] = useOnyx(ONYXKEYS.EXPENSE_ADDED_GROWL_TRANSACTION_IDS); | ||
| const transactionID = active?.transactionID ?? Object.keys(signal ?? {}).at(-1); | ||
|
|
||
| if (!transactionID) { | ||
| return null; | ||
| } | ||
|
|
||
| return ( | ||
| <ExpenseAddedGrowlContent | ||
| transactionID={transactionID} | ||
| signal={signal} | ||
| active={active} | ||
| setActive={setActive} | ||
| /> | ||
| ); | ||
| } | ||
|
|
||
| /** | ||
| * Reads the candidate transaction and its report/actions and renders the growl. Split out from | ||
| * `ExpenseAddedGrowl` so these Onyx connections only exist while there is a pending signal or a visible growl - | ||
| * the outer component is mounted for the whole app lifetime and would otherwise hold them permanently while idle. | ||
| */ | ||
| function ExpenseAddedGrowlContent({transactionID, signal, active, setActive}: ExpenseAddedGrowlContentProps) { | ||
| const nonceRef = useRef(0); | ||
|
|
||
| const {translate} = useLocalize(); | ||
| const currentUserPersonalDetails = useCurrentUserPersonalDetails(); | ||
| const [betas] = useOnyx(ONYXKEYS.BETAS); | ||
| const [introSelected] = useOnyx(ONYXKEYS.NVP_INTRO_SELECTED); | ||
| const [transaction] = useOnyx(`${ONYXKEYS.COLLECTION.TRANSACTION}${transactionID}`); | ||
| const reportID = transaction?.reportID; | ||
| const [report] = useOnyx(`${ONYXKEYS.COLLECTION.REPORT}${reportID}`); | ||
| // A tracked/unreported expense's transaction.reportID is UNREPORTED_REPORT_ID, and its IOU/track action lives | ||
| // on the self-DM report - not on transaction.reportID. Read actions from the report that actually hosts the | ||
| // action so "View" resolves the real transaction thread (the action's childReportID) instead of fabricating | ||
| // a mismatched optimistic one. | ||
| const isUnreportedExpense = !reportID || reportID === CONST.REPORT.UNREPORTED_REPORT_ID; | ||
| const selfDMReportID = isUnreportedExpense ? findSelfDMReportID() : undefined; | ||
| const hostReportID = isUnreportedExpense ? selfDMReportID : reportID; | ||
| const [reportActions] = useOnyx(`${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${hostReportID}`); | ||
| // Only IOU/expense/invoice reports have the expense-report RHP that "View" opens. A tracked/unreported | ||
| // (self-DM) expense lives in a chat, so it has no such report - leaving iouReportID undefined makes | ||
| // navigateToCreatedExpense open the transaction thread directly instead of a super-wide report RHP it lacks. | ||
| const iouReport = isMoneyRequestReport(report) || isInvoiceReport(report) ? report : undefined; | ||
| const iouReportID = iouReport?.reportID; | ||
|
|
||
| useEffect(() => { | ||
|
mhawryluk marked this conversation as resolved.
|
||
| if (active) { | ||
| return; | ||
| } | ||
| const pendingTransactionIDs = Object.keys(signal ?? {}); | ||
| // The last key is the last-created transaction: IDs are large (rand64), so they exceed the array-index | ||
| // range and JS preserves insertion order rather than sorting them numerically ascending. | ||
| const latestTransactionID = pendingTransactionIDs.at(-1); | ||
|
mhawryluk marked this conversation as resolved.
|
||
| const dataType = latestTransactionID ? signal?.[latestTransactionID] : undefined; | ||
| if (!latestTransactionID || !dataType) { | ||
| return; | ||
| } | ||
| // Wait for the created expense's optimistic data to land before acting. The create's API.write is | ||
| // usually deferred (the deferred-for-search pattern) until Search's onLayout flushes it, and the whole | ||
| // optimistic dataset (transaction + IOU report/action + thread) is applied atomically - so the | ||
| // transaction appearing in Onyx means "View" can resolve AND its report is known for the check below. | ||
| if (!transaction) { | ||
| return; | ||
| } | ||
|
|
||
| // Clear every pending ID but surface only the latest: rapid creations that pile up before this runs | ||
| // collapse into a single growl for the newest expense. | ||
| mergeExpenseAddedGrowlTransactionIDs(Object.fromEntries(pendingTransactionIDs.map((id) => [id, null]))); | ||
|
mhawryluk marked this conversation as resolved.
|
||
|
|
||
| // Suppress the growl when the user is already viewing the expense's money-request report (its | ||
| // transaction list), where the new row is highlighted so a growl would be redundant. A tracked/self-DM | ||
| // expense has transaction.reportID === UNREPORTED_REPORT_ID (never a report you view), so this never | ||
| // matches for those and the growl always shows - matching "no transaction list to be in". | ||
| if (Navigation.getTopmostReportId() === transaction.reportID) { | ||
| return; | ||
| } | ||
| nonceRef.current += 1; | ||
| setActive({transactionID: latestTransactionID, dataType, nonce: nonceRef.current}); | ||
| }, [signal, active, transaction, setActive]); | ||
|
|
||
| if (!active) { | ||
| return null; | ||
| } | ||
|
|
||
| const isInvoice = active.dataType === CONST.SEARCH.DATA_TYPES.INVOICE; | ||
|
|
||
| // Materialize the transaction thread and navigate to it at press time (not show time): the thread is | ||
| // only built if the user actually taps "View", against the freshest Onyx data, matching how every other | ||
| // thread navigation entry point builds the thread at navigation time. | ||
| const navigateToExpense = () => { | ||
| const iouAction = getIOUActionForTransactionID(Object.values(reportActions ?? {}), active.transactionID); | ||
| let threadReportID = transaction?.transactionThreadReportID ?? iouAction?.childReportID; | ||
| if (threadReportID) { | ||
| setOptimisticTransactionThread(threadReportID, iouReport?.reportID, iouAction?.reportActionID, iouReport?.policyID); | ||
| } else { | ||
| const optimisticThread = createTransactionThreadReport({ | ||
| introSelected, | ||
| currentUserLogin: currentUserPersonalDetails?.login ?? '', | ||
| currentUserAccountID: currentUserPersonalDetails?.accountID ?? CONST.DEFAULT_NUMBER_ID, | ||
| betas, | ||
| iouReport, | ||
| iouReportAction: iouAction, | ||
| transaction, | ||
| }); | ||
| threadReportID = optimisticThread?.reportID; | ||
| } | ||
| if (!threadReportID) { | ||
| Log.warn('[ExpenseAddedGrowl] Unable to resolve transaction thread reportID on View press.'); | ||
| return; | ||
| } | ||
| navigateToCreatedExpense({threadReportID, transactionID: active.transactionID, iouReportID}); | ||
| }; | ||
|
|
||
| return ( | ||
| <GrowlNotificationContent | ||
|
mhawryluk marked this conversation as resolved.
|
||
| key={active.nonce} | ||
| nonce={active.nonce} | ||
| type={CONST.GROWL.SUCCESS} | ||
| duration={CONST.GROWL.DURATION_WITH_ACTION} | ||
| bodyText={translate(isInvoice ? 'iou.invoiceSent' : 'iou.expenseAdded')} | ||
| action={{label: translate('common.view'), onPress: navigateToExpense}} | ||
| onDismissed={(dismissedNonce) => setActive((prev) => (prev?.nonce === dismissedNonce ? null : prev))} | ||
| /> | ||
| ); | ||
| } | ||
|
|
||
| export default ExpenseAddedGrowl; | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This mounts a second, independently managed
GrowlNotificationContentalongside the existing singleton growl. On narrow layouts both use the same fixed top anchor, so if an existingGrowl.error()is shown during the expense toast—for example from the tab-navigation error path—the two notifications overlap and neither replaces or dismisses the other, obscuring text and touch targets. Route this notification through the shared growl host or otherwise serialize the two sources.Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's technically true, but in practice this will not happen currently - we very rarely show the imperative growl errors, and not when creating new expenses. and I don't think it's worth adding synchronization of the two growl mechanisms at this moment, I think this can be handled if we do end up using growls in more scenarios, which would cause the overlap. right now it would complicate the code and fix an issue that is not present and might never be