Remove useDocumentTitle cleanup to fix tab title race condition#85473
Remove useDocumentTitle cleanup to fix tab title race condition#85473yuwenmemon merged 2 commits intomainfrom
Conversation
…ondition
The cleanup function setPageTitle('') was clearing the browser tab title
to an empty string on blur, which caused a race condition with the
asynchronous title-update pipeline (setTimeout in updateDocumentTitle and
the debounced triggerUnreadUpdate). When navigating back to a previously
visited page, the title would revert to "New Expensify" instead of
showing the page-specific title.
By removing the cleanup, each page simply sets its own title on focus,
and the previous title naturally gets overwritten without any gap where
the title is empty.
Co-authored-by: Yuwen Memon <yuwenmemon@users.noreply.github.com>
|
The failing check is unrelated to this PR. Analysis: The failing test is This PR only removes a single cleanup line ( Evidence: The exact same test with the exact same timeout failure is also failing on |
|
@hoangzinh 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] |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c2b9dba9f3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| useCallback(() => { | ||
| setPageTitle(title); | ||
| return () => setPageTitle(''); | ||
| }, [title]), |
There was a problem hiding this comment.
Reset page title when focus effect is cleaned up
Dropping the cleanup here leaves currentPageTitle latched to the previous screen indefinitely, because updateUnread only falls back to CONFIG.SITE_TITLE when that value is empty. With NavigationContainer document titles disabled (src/libs/Navigation/NavigationRoot.tsx), any route that does not call useDocumentTitle (for example auth routes like SCREENS.VALIDATE_LOGIN in src/libs/Navigation/AppNavigator/AuthScreens.tsx) will inherit the last page title; a common case is leaving a report and then opening login/auth screens, where the tab can continue showing the old report name instead of the app title.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
The new screen should update it with its title. The way proposed above introduced a race condition that caused page titles to be reset more often than they were set to the correct ones.
|
The failing check Analysis: The failure is in Evidence: The exact same test is failing on
This is a pre-existing flaky test on |
|
The failing check is unrelated to this PR. Failing test: Analysis: This PR only removes one line from Evidence: The exact same test ( The only other failing check is |
…umentTitleCleanup
|
@ikevin127 @yuwenmemon One of you needs to 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] |
|
I had explicitly commented here about the clean up method bug. Will test this out today. |
|
Good call - my bad on that one! |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppandroid-dynamic-title.movAndroid: mWeb Chromemweb-chrome-dynamic-title.moviOS: HybridAppios-dynamic-title.moviOS: mWeb Safarimweb-safari-dynamic-title.movMacOS: Chrome / Safariweb-dynamic-title.mov |
|
🚧 @yuwenmemon has triggered a test Expensify/App build. You can view the workflow run here. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚀 Deployed to staging by https://github.com/yuwenmemon in version: 9.3.42-0 🚀
Bundle Size Analysis (Sentry): |
|
Deploy Blocker #86087 was identified to be related to this PR. |
|
🚀 Deployed to production by https://github.com/grgia in version: 9.3.42-3 🚀
|
cc @mananjadhav
Explanation of Change
The
useDocumentTitlehook had a cleanup function (return () => setPageTitle('')) that cleared the browser tab title to an empty string whenever a screen lost focus. This caused a race condition with the asynchronous title-update pipeline:updateDocumentTitle()function defersdocument.titleupdates viasetTimeout(fn, 0)triggerUnreadUpdatethat also callsupdateDocumentTitle()popstatelistener triggers a non-debouncedupdateDocumentTitle()callWhen navigating away from a page and returning, the cleanup would clear
currentPageTitleto'', and any of these asynchronous callers executing whilecurrentPageTitlewas empty would fall back toCONFIG.SITE_TITLE("New Expensify").The fix removes the cleanup function entirely. Each page simply sets its own title on focus via
setPageTitle(title), and the previous title naturally gets overwritten by the next focused page's title — no need to clear it first.Fixed Issues
$ #85352
PROPOSAL: #85352 (comment)
Tests
Offline tests
QA Steps
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
N/A — This change only affects the browser tab title (
document.title), which is web-only.Android: mWeb Chrome
N/A — Browser tab titles are visible but the change is purely cosmetic metadata.
iOS: Native
N/A — This change only affects the browser tab title (
document.title), which is web-only.iOS: mWeb Safari
N/A — Browser tab titles are visible but the change is purely cosmetic metadata.
MacOS: Chrome / Safari
N/A — Manual testing required to verify tab title persistence during navigation.