[Better Expense Report View] Fix video player in expense report view & chat thread by JakubKorytko · Pull Request #59430 · Expensify/App

JakubKorytko · 2025-04-01T09:41:55Z

Explanation of Change

Related to #59459

Currently, VideoPlayerPreview uses usePlaybackContext to get currentlyPlayingURL and currentlyPlayingURLReportID from the PlaybackContextProvider.
Inside the context provider, there are actually 2 states related to the video report ID - currentlyPlayingURLReportID and currentReportID.
The currentReportID is modified when the navigation within the NavigationRoot changes (it is equal to topmost report), and the currentPlayingURLReportID is modified when the video is clicked to play.
The currentlyPlayingURLReportID is a copy of the currentReportID only when the video is actually playing.
The VideoPlayerPreview compares the video url and report ID associated with the video to context data to check if the video should be played via useEffect so that it stops when the user navigates to another route.
It also has isThumbnail for optimization purposes, so it will display a thumbnail if a video shouldn't be played.

This is a clever idea, but it creates problems that this PR solves.

For example, if a video is posted in a report with ID 123, and we enter the thread of that report (by replying to the video), then the topmost report is no longer a 123 (because we are in a thread), but the video report ID is still 123. Other videos posted within this thread will use a topmost ID (thread ID) except for the first one.
However, if we post a video (1), enter a thread, post a video (2) within that thread, and enter a nested thread (of video 2), then the first video in the nested thread will actually use the report ID 2 levels up where it was originally posted.
The other issue is that the topmost report does not exist if we use a narrow layout, because route change to /attachment and reportID is in route params.
There is also a new report table view, which also can't use the topmost report to index videos.

What this PR does is:

It stores a route without params where the video was originally rendered, and adds a conditional (isFocused) to check if the user is currently on that route
- But since the user can also be on a narrow layout and use the /attachment route, it is also included in the conditional
It adds a listener to set the thumbnail to true (basically disabling the video) if the user navigates to a different route
Since a video can be nested many levels down the thread, It finds an original report ID of the video (using it's url) when a video is in a thread using findUrlInReportOrAncestorAttachments
It adds a new condition to also store the report ID when a video is viewed using a new report table view and also when a user is using a narrow layout (so it is in an attachment route) in the updateCurrentPlayingReportID method.
Inside updateCurrentlyPlayingURL, which actually also sets currentlyPlayingURLReportID, it will use the first valid reportID from the array of:
- Chat thread ID or first report ascentor ID (including itself) with a video URL in message attachments - because as said before, it could be many levels up - if it is a chat thread
- ReportID we are currently in if a topmost report is valid
- ReportID from params or previous reportID as a fallback if user is using a narrow layout (/attachment route).

Fixed Issues

$ #57731
$ #58833
PROPOSAL: N/A

Tests

[Chat thread]

Open any chat.
Tap on the "+" button and select "Add Attachment"
Tap on "Choose File" and select any video.
Tap on "Send"
Once redirected to chat, select "Reply in thread" on the video
Once inside the thread, open the video.
Verify that the video can be played.

[Expense report view]

Go to workspace chat
Submit an expense
Go to Reports
Go to Expense Reports
Click on the expense report
Click + > Add attachment
Upload a video to the expense report
Verify that the video can be played.

Offline tests

N/A

QA Steps

Same as tests

PR Author Checklist

Screenshots/Videos

Android: Native

Android.mp4

Android: mWeb Chrome

iOS: Native

iOS.mp4

iOS: mWeb Safari

MacOS: Chrome / Safari

Web.mov

MacOS: Desktop

melvin-bot · 2025-04-03T08:41:49Z

@DylanDylann Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button]

DylanDylann · 2025-04-04T07:46:24Z

Discussing with @JakubKorytko on Slack to speed it up. WIll give a summary to the GH asap

src/components/VideoPlayerPreview/index.tsx

src/components/VideoPlayerContexts/PlaybackContext.tsx

src/components/VideoPlayerPreview/index.tsx

JakubKorytko · 2025-04-04T14:53:51Z

@DylanDylann I've fixed it according to your comments and also made some changes because this component logic depends on many factors. Please check again

src/components/VideoPlayerContexts/PlaybackContext.tsx

DylanDylann · 2025-04-04T17:05:21Z

BUG: Can't play video after reloading the attachment view

Screen.Recording.2025-04-05.at.00.01.43.mov

DylanDylann · 2025-04-04T17:17:06Z

Reviewer Checklist

I have verified the author checklist is complete (all boxes are checked off).
I verified the correct issue is linked in the ### Fixed Issues section above
I verified testing steps are clear and they cover the changes made in this PR
- I verified the steps for local testing are in the Tests section
- I verified the steps for Staging and/or Production testing are in the QA steps section
- I verified the steps cover any possible failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
- I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
I checked that screenshots or videos are included for tests on all platforms
I included screenshots or videos for tests on all platforms
I verified tests pass on all platforms & I tested again on:
- Android: Native
- Android: mWeb Chrome
- iOS: Native
- iOS: mWeb Safari
- MacOS: Chrome / Safari
- MacOS: Desktop
If there are any errors in the console that are unrelated to this PR, I either fixed them (preferred) or linked to where I reported them in Slack
I verified proper code patterns were followed (see Reviewing the code)
- I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick).
- I verified that the left part of a conditional rendering a React component is a boolean and NOT a string, e.g. myBool && <MyComponent />.
- I verified that comments were added to code that is not self explanatory
- I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
- I verified any copy / text shown in the product is localized by adding it to src/languages/* files and using the translation method
- I verified all numbers, amounts, dates and phone numbers shown in the product are using the localization methods
- I verified any copy / text that was added to the app is grammatically correct in English. It adheres to proper capitalization guidelines (note: only the first word of header/labels should be capitalized), and is either coming verbatim from figma or has been approved by marketing (in order to get marketing approval, ask the Bug Zero team member to add the Waiting for copy label to the issue)
- I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named "index.js". All platform-specific files are named for the platform the code supports as outlined in the README.
- I verified the JSDocs style guidelines (in STYLE.md) were followed
If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
I verified that this PR follows the guidelines as stated in the Review Guidelines
I verified other components that can be impacted by these changes have been tested, and I retested again (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar have been tested & I retested again)
I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
I verified any variables that can be defined as constants (ie. in CONST.js or at the top of the file that uses the constant) are defined as such
If a new component is created I verified that:
- A similar component doesn't exist in the codebase
- All props are defined accurately and each prop has a /** comment above it */
- The file is named correctly
- The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
- The only data being stored in the state is data necessary for rendering and nothing else
- For Class Components, any internal methods passed to components event handlers are bound to this properly so there are no scoping issues (i.e. for onClick={this.submit} the method this.submit should be bound to this in the constructor)
- Any internal methods bound to this are necessary to be bound (i.e. avoid this.submit = this.submit.bind(this); if this.submit is never passed to a component event handler like onClick)
- All JSX used for rendering exists in the render method
- The component has the minimum amount of code necessary for its purpose, and it is broken down into smaller components in order to separate concerns and functions
If any new file was added I verified that:
- The file has a description of what it does and/or why is needed at the top of the file if the code is not self explanatory
If a new CSS style is added I verified that:
- A similar style doesn't already exist
- The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(theme.componentBG)
If the PR modifies code that runs when editing or sending messages, I tested and verified there is no unexpected behavior for all supported markdown - URLs, single line code, code blocks, quotes, headings, bold, strikethrough, and italic.
If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
If the PR modifies a component or page that can be accessed by a direct deeplink, I verified that the code functions as expected when the deeplink is used - from a logged in and logged out account.
If the PR modifies the UI (e.g. new buttons, new UI components, changing the padding/spacing/sizing, moving components, etc) or modifies the form input styles:
- I verified that all the inputs inside a form are aligned with each other.
- I added Design label and/or tagged @Expensify/design so the design team can review the changes.
If a new page is added, I verified it's using the ScrollView component to make it scrollable when more elements are added to the page.
If the main branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to the Test steps.
I have checked off every checkbox in the PR reviewer checklist, including those that don't apply to this PR.

Screenshots/Videos

Android: Native

Screen.Recording.2025-04-05.at.00.06.32.mov

Android: mWeb Chrome

Screen.Recording.2025-04-05.at.00.00.02.mov

iOS: Native

Screen.Recording.2025-04-05.at.01.15.46.mov

iOS: mWeb Safari

Screen.Recording.2025-04-05.at.00.01.43.mov

MacOS: Chrome / Safari

Screen.Recording.2025-04-04.at.23.56.09.mov

MacOS: Desktop

Screen.Recording.2025-04-04.at.23.57.01.mov

JakubKorytko · 2025-04-04T17:27:18Z

BUG: Can't play video after reloading the attachment view

Fixed

mountiny

Leaving some NABs that you could address in future PRs

mountiny · 2025-04-05T07:19:57Z

src/components/VideoPlayerContexts/PlaybackContext.tsx

+            const attachmentReportID = Navigation.getActiveRouteWithoutParams() === `/${ROUTES.ATTACHMENTS.route}` ? prevCurrentReportID ?? reportIDFromUrlParams : undefined;
+            const reportIDWithUrl = isChatThread(topMostReport) ? findUrlInReportOrAncestorAttachments(topMostReport, url) : undefined;
+
+            // - if it is a chat thread, use chat thread ID or any ascentor ID since the video could have originally been sent on report many levels up


Suggested change

// - if it is a chat thread, use chat thread ID or any ascentor ID since the video could have originally been sent on report many levels up

// - if it is a chat thread, use chat thread ID or any ancestor ID since the video could have originally been sent on report many levels up

mountiny · 2025-04-05T07:25:36Z

src/components/VideoPlayerContexts/PlaybackContext.tsx

+
+            // Used for /attachment route
+            const topMostReport = getReportOrDraftReport(Navigation.getTopmostReportId());
+            const reportIDFromUrlParams = new URLSearchParams(Navigation.getActiveRoute()).get('reportID') ?? undefined;


I think all of these should have URL uppercase in name based on our style guides

Suggested change

const reportIDFromUrlParams = new URLSearchParams(Navigation.getActiveRoute()).get('reportID') ?? undefined;

const reportIDFromURLParams = new URLSearchParams(Navigation.getActiveRoute()).get('reportID') ?? undefined;

mountiny · 2025-04-05T07:25:48Z

src/components/VideoPlayerContexts/PlaybackContext.tsx

+    return !!route && !!route.params && route.name === SCREENS.SEARCH.MONEY_REQUEST_REPORT && 'reportID' in route.params;
+}
+
+function findUrlInReportOrAncestorAttachments(currentReport: OnyxEntry<Report>, url: string | null): string | undefined {


Suggested change

function findUrlInReportOrAncestorAttachments(currentReport: OnyxEntry<Report>, url: string | null): string | undefined {

function findURLInReportOrAncestorAttachments(currentReport: OnyxEntry<Report>, url: string | null): string | undefined {

mountiny · 2025-04-05T07:26:21Z

src/components/VideoPlayerContexts/PlaybackContext.tsx

+    return !!route && !!route.params && route.name === SCREENS.SEARCH.MONEY_REQUEST_REPORT && 'reportID' in route.params;
+}
+
+function findUrlInReportOrAncestorAttachments(currentReport: OnyxEntry<Report>, url: string | null): string | undefined {


NAB: I think adding more docs to this method would be helpful.

OSBotify · 2025-04-05T07:35:29Z

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

github-actions · 2025-04-07T23:09:10Z

🚀 Deployed to staging by https://github.com/mountiny in version: 9.1.24-2 🚀

platform	result
🖥 desktop 🖥	success ✅
🕸 web 🕸	success ✅
🤖 android 🤖	success ✅
🍎 iOS 🍎	success ✅

github-actions · 2025-04-09T02:14:26Z

🚀 Deployed to production by https://github.com/jasperhuangg in version: 9.1.24-10 🚀

platform	result
🖥 desktop 🖥	success ✅
🕸 web 🕸	success ✅
🤖 android 🤖	failure ❌
🍎 iOS 🍎	success ✅

github-actions · 2025-04-09T04:32:47Z

🚀 Deployed to production by https://github.com/jasperhuangg in version: 9.1.24-10 🚀

platform	result
🖥 desktop 🖥	success ✅
🕸 web 🕸	success ✅
🤖 android 🤖	failure ❌
🍎 iOS 🍎	success ✅

JakubKorytko added 4 commits April 1, 2025 11:29

Fix video player in search view

8f150cc

Fix video play in chat threads

94d72fb

Add comments to PlaybackContext

67e7722

Play video only on the original page

633938f

JakubKorytko changed the title ~~[WIP] [Better Expense Report View] Fix video player in expense report view & chat thread~~ [Better Expense Report View] Fix video player in expense report view & chat thread Apr 1, 2025

Add exception for attachments modal

52488f0

luacmartins assigned JakubKorytko Apr 1, 2025

JakubKorytko added 5 commits April 1, 2025 19:17

Add fallback for Attachment route

2b7f1b0

Merge branch 'main' into korytko/fix-video-player

c74aedb

Fix first video load in carousel on native apps

893a0a1

Fix for nested chat threads

dc26009

Merge branch 'main' into korytko/fix-video-player

3a7ff8d

JakubKorytko marked this pull request as ready for review April 3, 2025 08:41

JakubKorytko requested a review from a team as a code owner April 3, 2025 08:41

melvin-bot bot requested a review from DylanDylann April 3, 2025 08:41

melvin-bot bot removed the request for review from a team April 3, 2025 08:41

DylanDylann reviewed Apr 4, 2025

View reviewed changes

src/components/VideoPlayerPreview/index.tsx Outdated Show resolved Hide resolved

DylanDylann reviewed Apr 4, 2025

View reviewed changes

src/components/VideoPlayerContexts/PlaybackContext.tsx Outdated Show resolved Hide resolved

DylanDylann reviewed Apr 4, 2025

View reviewed changes

src/components/VideoPlayerContexts/PlaybackContext.tsx Outdated Show resolved Hide resolved

src/components/VideoPlayerContexts/PlaybackContext.tsx Outdated Show resolved Hide resolved

DylanDylann reviewed Apr 4, 2025

View reviewed changes

src/components/VideoPlayerPreview/index.tsx Outdated Show resolved Hide resolved

JakubKorytko added 2 commits April 4, 2025 16:50

Fix code according to PR comments

4b89080

Run prettier

f5bf193

JakubKorytko added 2 commits April 4, 2025 16:56

Change useFirstRenderRoute value getter name

703ea6c

Merge branch 'main' into korytko/fix-video-player

798156a

DylanDylann reviewed Apr 4, 2025

View reviewed changes

src/components/VideoPlayerContexts/PlaybackContext.tsx Outdated Show resolved Hide resolved

Fix attachment view after reload

5c45bf5

JakubKorytko requested a review from DylanDylann April 4, 2025 17:29

DylanDylann approved these changes Apr 4, 2025

View reviewed changes

melvin-bot bot requested a review from mountiny April 4, 2025 17:37

luacmartins approved these changes Apr 4, 2025

View reviewed changes

mountiny approved these changes Apr 5, 2025

View reviewed changes

mountiny merged commit a8ea179 into Expensify:main Apr 5, 2025
19 checks passed

JakubKorytko mentioned this pull request Apr 7, 2025

[Due for payment 2025-05-14] [Better Expense Report View] Report view follow-ups #59459

Closed

16 tasks

github-actions bot mentioned this pull request Apr 7, 2025

Deploy Checklist: New Expensify 2025-04-07 #59756

Closed

This was referenced Apr 9, 2025

[HOLD] [$250] Attachment - Unable to play video when inside a thread. #57731

Closed

[Due for payment 2025-04-16] [Better Expense Report View] Expense Reports - Unable to play video attachment #58833

Closed

JakubKorytko mentioned this pull request Apr 14, 2025

[No QA] [Better Expense Report View] Clean up remaining nested follow-ups & added code #60179

Merged

48 tasks

bernhardoj mentioned this pull request Jul 14, 2025

[$250] Chat - Video is duplicated when navigating through the conversation attachments #66015

Closed

8 tasks

	// - if it is a chat thread, use chat thread ID or any ascentor ID since the video could have originally been sent on report many levels up
	// - if it is a chat thread, use chat thread ID or any ancestor ID since the video could have originally been sent on report many levels up

	const reportIDFromUrlParams = new URLSearchParams(Navigation.getActiveRoute()).get('reportID') ?? undefined;
	const reportIDFromURLParams = new URLSearchParams(Navigation.getActiveRoute()).get('reportID') ?? undefined;

	function findUrlInReportOrAncestorAttachments(currentReport: OnyxEntry<Report>, url: string \| null): string \| undefined {
	function findURLInReportOrAncestorAttachments(currentReport: OnyxEntry<Report>, url: string \| null): string \| undefined {

Conversation

JakubKorytko commented Apr 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Explanation of Change

Fixed Issues

Tests

Offline tests

QA Steps

PR Author Checklist

Screenshots/Videos

Uh oh!

melvin-bot bot commented Apr 3, 2025

Uh oh!

DylanDylann commented Apr 4, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

JakubKorytko commented Apr 4, 2025

Uh oh!

Uh oh!

DylanDylann commented Apr 4, 2025

Uh oh!

DylanDylann commented Apr 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reviewer Checklist

Screenshots/Videos

Uh oh!

JakubKorytko commented Apr 4, 2025

Uh oh!

mountiny left a comment

Choose a reason for hiding this comment

Uh oh!

mountiny Apr 5, 2025

Choose a reason for hiding this comment

Uh oh!

mountiny Apr 5, 2025

Choose a reason for hiding this comment

Uh oh!

mountiny Apr 5, 2025

Choose a reason for hiding this comment

Uh oh!

mountiny Apr 5, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

OSBotify commented Apr 5, 2025

Uh oh!

github-actions bot commented Apr 7, 2025

Uh oh!

github-actions bot commented Apr 9, 2025

Uh oh!

github-actions bot commented Apr 9, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

JakubKorytko commented Apr 1, 2025 •

edited

Loading

DylanDylann commented Apr 4, 2025 •

edited

Loading