-
Notifications
You must be signed in to change notification settings - Fork 4k
86919: Message in IOU is not scrolled to when navigating via link, 'submitted' not shown #96428
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
abbasifaizan70
wants to merge
6
commits into
Expensify:main
Choose a base branch
from
abbasifaizan70:fix-86919
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
6 commits
Select commit
Hold shift + click to select a range
a02e7b6
86919: Message in IOU is not scrolled to when navigating via link, 's…
abbasifaizan70 01331a0
Merge branch 'Expensify:main' into fix-86919
abbasifaizan70 862e8c6
Fixed EsLint & test cases issues
abbasifaizan70 d28be07
Fixed Lint issues
abbasifaizan70 9d260f8
Fixed Lint issues
abbasifaizan70 7a8a51d
Fixed Lint issues
abbasifaizan70 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,108 @@ | ||
| import Clipboard from '@libs/Clipboard'; | ||
| import type * as EnvironmentModule from '@libs/Environment/Environment'; | ||
| import {getDisplayedReportID} from '@libs/ReportUtils'; | ||
| // eslint-disable-next-line no-restricted-imports -- type-only namespace (erased at runtime) used solely for the requireActual generic; no restricted functions are actually imported | ||
| import type * as ReportUtilsModule from '@libs/ReportUtils'; | ||
|
|
||
| import ContextMenuActions from '@pages/inbox/report/ContextMenu/ContextMenuActions'; | ||
| import type {ContextMenuActionPayload} from '@pages/inbox/report/ContextMenu/ContextMenuActions'; | ||
|
|
||
| import CONST from '@src/CONST'; | ||
|
|
||
| import createRandomReportAction from '../utils/collections/reportActions'; | ||
|
|
||
| // Verifies the "Copy link" context menu action (change A for issue #86919): for one-transaction | ||
| // expense flows the copied link must use the DISPLAYED (parent expense) report ID rather than the | ||
| // transaction thread's `originalReportID`, so the link opens the combined view where the parent | ||
| // "Submitted" system message is present and the linked message can be scrolled to. | ||
|
|
||
| jest.mock( | ||
| 'expo-web-browser', | ||
| () => ({ | ||
| openAuthSessionAsync: jest.fn(), | ||
| }), | ||
| {virtual: true}, | ||
| ); | ||
|
|
||
| jest.mock('@components/Reactions/MiniQuickEmojiReactions', () => 'MiniQuickEmojiReactions'); | ||
| jest.mock('@components/Reactions/QuickEmojiReactions', () => 'QuickEmojiReactions'); | ||
|
|
||
| jest.mock('@libs/Clipboard', () => ({ | ||
| __esModule: true, | ||
| default: { | ||
| canSetHtml: jest.fn(), | ||
| setString: jest.fn(), | ||
| setHtml: jest.fn(), | ||
| }, | ||
| })); | ||
|
|
||
| jest.mock('@libs/Environment/Environment', () => ({ | ||
| __esModule: true, | ||
| ...jest.requireActual<typeof EnvironmentModule>('@libs/Environment/Environment'), | ||
| getEnvironmentURL: jest.fn(() => Promise.resolve('https://new.expensify.com')), | ||
| })); | ||
|
|
||
| jest.mock('@libs/ReportUtils', () => ({ | ||
| __esModule: true, | ||
| ...jest.requireActual<typeof ReportUtilsModule>('@libs/ReportUtils'), | ||
| getDisplayedReportID: jest.fn(), | ||
| })); | ||
|
|
||
| const mockClipboard = jest.mocked(Clipboard); | ||
| const mockGetDisplayedReportID = jest.mocked(getDisplayedReportID); | ||
|
|
||
| // ContextMenuAction is a union; sentryLabel/onPress only exist on the icon variant, so narrow with `in`. | ||
| const copyLinkAction = ContextMenuActions.find((action) => 'sentryLabel' in action && action.sentryLabel === CONST.SENTRY_LABEL.CONTEXT_MENU.COPY_LINK); | ||
|
|
||
| // Flush the microtasks queued by getEnvironmentURL().then(...) inside the onPress handler. | ||
| const flushPromises = () => | ||
| new Promise((resolve) => { | ||
| process.nextTick(resolve); | ||
| }); | ||
|
|
||
| function createPayload(overrides: Partial<ContextMenuActionPayload>): ContextMenuActionPayload { | ||
| // The copy-link handler only reads reportAction, originalReportID, and isOffline; the rest of the | ||
| // (large) payload type is irrelevant to this action, so we assert the minimal shape it needs. | ||
| const payload = { | ||
| reportAction: {...createRandomReportAction(1), reportActionID: 'action-1'}, | ||
| originalReportID: 'transaction-thread-1', | ||
| isOffline: false, | ||
| ...overrides, | ||
| }; | ||
| // eslint-disable-next-line @typescript-eslint/no-unsafe-type-assertion -- test payload only needs the fields the copy-link handler reads | ||
| return payload as ContextMenuActionPayload; | ||
| } | ||
|
|
||
| describe('ContextMenuActions copy link', () => { | ||
| beforeEach(() => { | ||
| jest.clearAllMocks(); | ||
| }); | ||
|
|
||
| it('copies a link using the displayed (parent) report ID for one-transaction expense flows', async () => { | ||
| mockGetDisplayedReportID.mockReturnValue('parent-expense-1'); | ||
|
|
||
| if (!copyLinkAction || !('onPress' in copyLinkAction)) { | ||
| throw new Error('Copy link context menu action was not found'); | ||
| } | ||
|
|
||
| copyLinkAction.onPress(true, createPayload({originalReportID: 'transaction-thread-1', isOffline: false})); | ||
| await flushPromises(); | ||
|
|
||
| // The displayed report ID is resolved from the original (transaction-thread) report ID and the offline flag. | ||
| expect(mockGetDisplayedReportID).toHaveBeenCalledWith('transaction-thread-1', false); | ||
| // The copied link points at the parent expense report, not the transaction thread. | ||
| expect(mockClipboard.setString).toHaveBeenCalledWith('https://new.expensify.com/r/parent-expense-1/action-1'); | ||
| }); | ||
|
|
||
| it('does not resolve a displayed report ID when there is no original report ID', async () => { | ||
| if (!copyLinkAction || !('onPress' in copyLinkAction)) { | ||
| throw new Error('Copy link context menu action was not found'); | ||
| } | ||
|
|
||
| copyLinkAction.onPress(true, createPayload({originalReportID: undefined})); | ||
| await flushPromises(); | ||
|
|
||
| expect(mockGetDisplayedReportID).not.toHaveBeenCalled(); | ||
| expect(mockClipboard.setString).toHaveBeenCalledTimes(1); | ||
| }); | ||
| }); |
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,192 @@ | ||
| import {renderHook} from '@testing-library/react-native'; | ||
|
|
||
| import useOnyx from '@hooks/useOnyx'; | ||
| import usePaginatedReportActions from '@hooks/usePaginatedReportActions'; | ||
| import useReportIsArchived from '@hooks/useReportIsArchived'; | ||
|
|
||
| import CONST from '@src/CONST'; | ||
| import ONYXKEYS from '@src/ONYXKEYS'; | ||
| import type {Report, ReportAction} from '@src/types/onyx'; | ||
| import type Pages from '@src/types/onyx/Pages'; | ||
|
|
||
| import type {OnyxKey, UseOnyxResult} from 'react-native-onyx'; | ||
|
|
||
| import createRandomReportAction from '../../utils/collections/reportActions'; | ||
|
|
||
| // The behavior change under test lives in the `id` useMemo of usePaginatedReportActions: | ||
| // when a `reportActionID` is provided but does NOT exist in this report's own actions | ||
| // (e.g. a one-transaction expense link where the linked message lives in the merged-in | ||
| // transaction thread), the hook must fall back to the newest window instead of anchoring | ||
| // pagination to a missing action. Previously, getContinuousChain returned an empty array in | ||
| // that case (see tests/unit/PaginationUtilsTest.ts "given an input ID of 8 or 13 ... empty | ||
| // array"), which is what hid the parent-level "Submitted" system message. | ||
| // | ||
| // We deliberately use the REAL getContinuousChain (PaginationUtils is not mocked) so these | ||
| // tests exercise the true integration of the change with pagination, and only mock the Onyx | ||
| // subscriptions so we can control the report, its actions, and its pages. | ||
|
|
||
| jest.mock('@hooks/useOnyx', () => ({ | ||
| __esModule: true, | ||
| default: jest.fn(), | ||
| })); | ||
|
|
||
| jest.mock('@hooks/useReportIsArchived', () => ({ | ||
| __esModule: true, | ||
| default: jest.fn(() => false), | ||
| })); | ||
|
|
||
| const mockUseOnyx = jest.mocked(useOnyx); | ||
|
|
||
| const REPORT_ID = 'expense-report-1'; | ||
| const LINKED_ACTION_ID = 'action-in-this-report'; | ||
| const SIBLING_ACTION_ID = 'action-in-transaction-thread'; | ||
|
|
||
| // `{status: 'loaded'}` alone satisfies ResultMetadata (its sourceValue is optional). A single | ||
| // broad UseOnyxResult type keeps each mocked subscription value assignable without casts. | ||
| type MockOnyxResult = UseOnyxResult<Report | ReportAction[] | Pages>; | ||
|
|
||
| function makeReport(overrides: Partial<Report> = {}): Report { | ||
| return { | ||
| reportID: REPORT_ID, | ||
| type: CONST.REPORT.TYPE.EXPENSE, | ||
| chatReportID: 'chat-report-1', | ||
| ...overrides, | ||
| } as Report; | ||
| } | ||
|
|
||
| /** | ||
| * Minimal display-sorted actions (newest first). Ordering only needs to be internally | ||
| * consistent — getContinuousChain indexes by reportActionID, and the "newest window" | ||
| * result for empty pages returns the whole array regardless of order. | ||
| */ | ||
| function makeActions(reportActionIds: string[]): ReportAction[] { | ||
| return reportActionIds.map((reportActionID, index) => ({ | ||
| ...createRandomReportAction(index), | ||
| reportActionID, | ||
| created: `2024-01-01 10:0${reportActionIds.length - index}:00.000`, | ||
| })); | ||
| } | ||
|
|
||
| /** | ||
| * Wire the three Onyx subscriptions usePaginatedReportActions makes: the report, its | ||
| * (already display-sorted) actions, and its pages. The selector on the actions key is | ||
| * bypassed by the mock, so we pass pre-sorted actions directly. | ||
| */ | ||
| function wireOnyx({report, actions, pages}: {report: Report | undefined; actions: ReportAction[] | undefined; pages: Pages | undefined}): void { | ||
| mockUseOnyx.mockImplementation((key: OnyxKey): MockOnyxResult => { | ||
| if (key === `${ONYXKEYS.COLLECTION.REPORT}${REPORT_ID}`) { | ||
| return [report, {status: 'loaded'}]; | ||
| } | ||
| if (key === `${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${REPORT_ID}`) { | ||
| return [actions, {status: 'loaded'}]; | ||
| } | ||
| if (key === `${ONYXKEYS.COLLECTION.REPORT_ACTIONS_PAGES}${REPORT_ID}`) { | ||
| return [pages, {status: 'loaded'}]; | ||
| } | ||
| return [undefined, {status: 'loaded'}]; | ||
| }); | ||
| } | ||
|
|
||
| function actionIds(actions: ReportAction[] | undefined): string[] { | ||
| return (actions ?? []).map((action) => action.reportActionID); | ||
| } | ||
|
|
||
| describe('usePaginatedReportActions', () => { | ||
| beforeEach(() => { | ||
| mockUseOnyx.mockReset(); | ||
| jest.mocked(useReportIsArchived).mockReturnValue(false); | ||
| }); | ||
|
|
||
| describe('when the linked action exists in this report (no behavior change)', () => { | ||
| it('anchors to the linked action and returns the report actions with pages absent', () => { | ||
| const actions = makeActions(['c', 'b', LINKED_ACTION_ID]); | ||
| wireOnyx({report: makeReport(), actions, pages: []}); | ||
|
|
||
| const {result} = renderHook(() => usePaginatedReportActions(REPORT_ID, LINKED_ACTION_ID)); | ||
|
|
||
| // Non-empty result and the linked action is surfaced — identical to pre-change behavior. | ||
| expect(actionIds(result.current.reportActions)).toEqual(['c', 'b', LINKED_ACTION_ID]); | ||
| expect(result.current.linkedAction?.reportActionID).toBe(LINKED_ACTION_ID); | ||
| }); | ||
|
|
||
| it('anchors to the linked action within its page when pages are present', () => { | ||
| const actions = makeActions(['e', 'd', 'c', 'b', 'a']); | ||
| const pages: Pages = [['e', 'd', 'c', 'b', 'a']]; | ||
| wireOnyx({report: makeReport(), actions, pages}); | ||
|
|
||
| const {result} = renderHook(() => usePaginatedReportActions(REPORT_ID, 'c')); | ||
|
|
||
| expect(result.current.reportActions.length).toBeGreaterThan(0); | ||
| expect(result.current.linkedAction?.reportActionID).toBe('c'); | ||
| }); | ||
| }); | ||
|
|
||
| describe('when the linked action is NOT in this report (the fix)', () => { | ||
| it('falls back to the newest window instead of returning an empty list, with pages absent', () => { | ||
| const actions = makeActions(['c', 'b', 'a']); | ||
| wireOnyx({report: makeReport(), actions, pages: []}); | ||
|
|
||
| // SIBLING_ACTION_ID lives in the transaction thread, not in this report's own actions. | ||
| const {result} = renderHook(() => usePaginatedReportActions(REPORT_ID, SIBLING_ACTION_ID)); | ||
|
|
||
| // Before the fix this was [] (getContinuousChain empty-array behavior); now it is the newest window. | ||
| expect(actionIds(result.current.reportActions)).toEqual(['c', 'b', 'a']); | ||
| // No linked action is surfaced from this report — the host screen merges the thread separately. | ||
| expect(result.current.linkedAction).toBeUndefined(); | ||
| }); | ||
|
|
||
| it('falls back to the newest page instead of returning an empty list, with pages present', () => { | ||
| const actions = makeActions(['e', 'd', 'c', 'b', 'a']); | ||
| const pages: Pages = [['e', 'd', 'c']]; | ||
| wireOnyx({report: makeReport(), actions, pages}); | ||
|
|
||
| const {result} = renderHook(() => usePaginatedReportActions(REPORT_ID, SIBLING_ACTION_ID)); | ||
|
|
||
| expect(result.current.reportActions.length).toBeGreaterThan(0); | ||
| expect(result.current.linkedAction).toBeUndefined(); | ||
| }); | ||
| }); | ||
|
|
||
| describe('unchanged paths', () => { | ||
| it('returns the newest window when no reportActionID is provided', () => { | ||
| const actions = makeActions(['c', 'b', 'a']); | ||
| wireOnyx({report: makeReport(), actions, pages: []}); | ||
|
|
||
| const {result} = renderHook(() => usePaginatedReportActions(REPORT_ID)); | ||
|
|
||
| expect(actionIds(result.current.reportActions)).toEqual(['c', 'b', 'a']); | ||
| expect(result.current.linkedAction).toBeUndefined(); | ||
| }); | ||
|
|
||
| it('ignores the anchor and never surfaces a linked action when treatAsNoPaginationAnchor is set, even if the action exists', () => { | ||
| const actions = makeActions(['c', 'b', LINKED_ACTION_ID]); | ||
| wireOnyx({report: makeReport(), actions, pages: []}); | ||
|
|
||
| const {result} = renderHook(() => usePaginatedReportActions(REPORT_ID, LINKED_ACTION_ID, {treatAsNoPaginationAnchor: true})); | ||
|
|
||
| expect(result.current.reportActions.length).toBeGreaterThan(0); | ||
| expect(result.current.linkedAction).toBeUndefined(); | ||
| }); | ||
|
|
||
| it('resolves the oldest-unread anchor when shouldLinkToOldestUnreadReportAction is set and no reportActionID is provided', () => { | ||
| // lastReadTime is older than "b"/"c" but newer than "a", so the oldest unread action is "b". | ||
| const actions = makeActions(['c', 'b', 'a']); | ||
| const report = makeReport({lastReadTime: '2024-01-01 10:01:30.000'}); | ||
| wireOnyx({report, actions, pages: []}); | ||
|
|
||
| const {result} = renderHook(() => usePaginatedReportActions(REPORT_ID, undefined, {shouldLinkToOldestUnreadReportAction: true})); | ||
|
|
||
| expect(result.current.reportActions.length).toBeGreaterThan(0); | ||
| expect(result.current.oldestUnreadReportAction?.reportActionID).toBe('b'); | ||
| }); | ||
|
|
||
| it('returns an empty list without crashing when the report has no actions', () => { | ||
| wireOnyx({report: makeReport(), actions: [], pages: []}); | ||
|
|
||
| const {result} = renderHook(() => usePaginatedReportActions(REPORT_ID, SIBLING_ACTION_ID)); | ||
|
|
||
| expect(result.current.reportActions).toEqual([]); | ||
| expect(result.current.linkedAction).toBeUndefined(); | ||
| }); | ||
| }); | ||
| }); |
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.
When navigating to a valid linked message that is not currently in Onyx for this report (for example a cached chat with only the latest page loaded, then opening an older message link), this returns
undefinedandgetContinuousChainrenders the newest window instead of the previous empty/loading state. The list can mount beforeOpenReporthydrates the target action, and because linked-message positioning is calculated as an initial scroll, it will not scroll to the target after it arrives. Please only drop the anchor when we know the action belongs to the merged transaction thread, not merely when it is absent from the current cache.Useful? React with 👍 / 👎.