Fix: useDocumentTitle uses focus events instead of mount lifecycle#83586
Fix: useDocumentTitle uses focus events instead of mount lifecycle#83586
Conversation
The useDocumentTitle hook was using useEffect to set the browser tab title, which relies on component unmount for cleanup. Since Settings screens are persistent (dontDetachScreen), ProfilePage never unmounts when navigating to other tabs, causing the title to stay stuck as "Settings - Profile". Switch to useFocusEffect so the title is set when the screen gains focus and cleared when it loses focus, correctly handling persistent screens. Co-authored-by: Aimane Chnaif <aimane-chnaif@users.noreply.github.com>
|
The Analysis: The CI runner failed to connect to Evidence: The error is a network connectivity issue in the CI environment, not a commit signing problem. All other checks (ESLint, typecheck, Prettier, builds, etc.) passed successfully. Resolution: Re-running the |
aimane-chnaif
left a comment
There was a problem hiding this comment.
@MelvinBot mark as ready for review. Thanks for the quick fix
| useCallback(() => { | ||
| setPageTitle(title); | ||
|
|
||
| // Reset to default title when component unmounts |
|
@aimane-chnaif 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] |
|
PR has been marked as ready for review! 🎉 |
Co-authored-by: Aimane Chnaif <aimane-chnaif@users.noreply.github.com>
|
Two checks were failing:
|
aimane-chnaif
left a comment
There was a problem hiding this comment.
@MelvinBot your change reintroduces original bug.
Title doesn't get updated while switching sub tabs in Account settings page
Screen.Recording.2026-02-26.at.2.51.13.pm.mov
|
I have failed to help you. I let you down and for that I am sorry. Please try again later. This may be due to a Claude outage (check here). Failure details: https://github.com/Expensify/Expensify/actions/runs/22447168380 |
|
I analyzed all checks on this PR. Here's the status: Previously failing (now fixed):
Currently failing:
All code checks are passing:
No code fix is needed — all actionable checks are green. |
|
The only failing check is All code-related checks (ESLint, typecheck, Prettier, tests, builds, spellcheck, etc.) are passing. This checklist failure is unrelated to the PR changes and should resolve on re-run. |
When switching between Settings sub-pages, the new screen's useFocusEffect
fires before the old screen's blur cleanup (due to React's child-before-parent
effect ordering). The cleanup was unconditionally clearing the title, overwriting
the new screen's title. Now the cleanup checks if the current title still matches
what this screen set — if another screen has already set a different title, the
cleanup is skipped.
Co-authored-by: {"message":"Not Found","documentation_url":"https://docs.github.com/rest/issues/comments#get-an-issue-comment","status":"404"} <{"message":"Not Found","documentation_url":"https://docs.github.com/rest/issues/comments#get-an-issue-comment","status":"404"}@users.noreply.github.com>
|
Thanks for catching this! I've pushed a fix in 64e0ad6. Root cause of the regression: When switching between Settings sub-pages (e.g., Profile → Preferences), the new screen's
Fix: The cleanup now checks if another screen has already set a new title by comparing the current page title to what this screen originally set. If they differ, it means another screen is already active and has set its own title, so the cleanup skips clearing: return () => {
if (getPageTitle() === title) {
setPageTitle('');
}
};This preserves both behaviors:
|
Co-authored-by: Aimane Chnaif <aimane-chnaif@users.noreply.github.com>
|
Fixed the failing |
Codecov Report❌ Looks like you've decreased code coverage for some files. Please write tests to increase, or at least maintain, the existing level of code coverage. See our documentation here for how to interpret this table.
|
|
@MelvinBot there's another glitch. Go to Settings > Profile > Display Name: this clears title I think the expected result should be to keep |
When a modal or RHP screen opens on top of a central pane screen (e.g., Display Name from Profile), the useFocusEffect cleanup was clearing the title because the central pane loses focus. This caused the title to flash to "New Expensify" and then flicker back when closing the modal. Now the cleanup checks if the topmost root route is a full-screen route. If the blur was caused by a non-full-screen overlay (RHP, onboarding modal, etc.), the title is preserved since the central pane is still visible behind the overlay. Co-authored-by: Aimane Chnaif <aimane-chnaif@users.noreply.github.com>
|
Good catch! Pushed a fix in 33c39b9. Root cause: When an RHP/modal screen (like Display Name) opens, it pushes Fix: The cleanup now checks whether the topmost root route is a full-screen route using the existing The three cases now work correctly:
|
|
@MelvinBot close this PR in favor of #83589. We're going on a revert path. |
|
Closed this PR in favor of #83589 (revert path), as requested. |
Explanation of Change
The
useDocumentTitlehook was usinguseEffectto set the browser tab title, relying on component unmount for cleanup. Since Settings screens usepersistentScreens(which setsdontDetachScreen: true),ProfilePagenever unmounts when navigating to other tabs like Workspaces or Inbox. This caused the browser tab title to remain stuck as "Settings - Profile".This PR switches
useDocumentTitlefromuseEffecttouseFocusEffect, so the title is set when the screen gains focus and cleared when it loses focus. This correctly handles persistent screens that stay mounted but lose focus when navigating away.Fixed Issues
$ #83582
PROPOSAL: #83582 (comment)
Tests
Offline tests
This change only affects the browser tab title on web, which is purely a client-side DOM operation. No network dependency.
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 —
useDocumentTitleis a web-only hook (document.title is a web API)Android: mWeb Chrome
N/A —
useDocumentTitleis a web-only hookiOS: Native
N/A —
useDocumentTitleis a web-only hookiOS: mWeb Safari
N/A —
useDocumentTitleis a web-only hookMacOS: Chrome / Safari
N/A — Screenshots to be added by reviewer during testing