Extend useDocumentTitle hook to all remaining pages#83388
Conversation
Add useDocumentTitle to 27 pages that were missing browser tab titles: - Top-level tabs: Home, Search, Workspaces, Report - Workspace pages: Overview, Workflows, Members, Accounting, More Features, Taxes, Categories, Tags, Reports, Company Cards, Per Diem, Receipt Partners, Distance Rates, Travel, Invoices, Expensify Card, Rules, Time Tracking - Domain pages: Initial, SAML, Admins, Members, Groups Each page now sets document.title with the appropriate context (e.g., "PolicyName - Categories" for workspace pages, "domain.com - SAML" for domain pages). Co-authored-by: Yuwen Memon <yuwenmemon@users.noreply.github.com>
Co-authored-by: Yuwen Memon <yuwenmemon@users.noreply.github.com>
|
Fixed the failing |
…entTitle Co-authored-by: Yuwen Memon <yuwenmemon@users.noreply.github.com>
|
@MelvinBot It looks like you have some typescript errors |
…gnature The addEncryptedAuthTokenToURL function signature changed to require encryptedAuthToken as an explicit parameter. Updated the caller in WalletStatementPage to pass the token from useSession(), matching the pattern used by other updated callers in the codebase. Co-authored-by: Yuwen Memon <yuwenmemon@users.noreply.github.com>
|
Fixed the failing checks:
|
|
The failing check was This was caused by the |
|
The TypeScript error has been resolved. The The original error was: This occurred because the PR's changes to |
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
|
@QichenZhu 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] |
| // When we create a new workspace, the policy prop will be empty on the first render. Therefore, we have to use policyDraft until policy has been set in Onyx. | ||
| const policy = policyDraft?.id ? policyDraft : policyProp; | ||
| const policyID = policy?.id; | ||
| useDocumentTitle(policy?.name ? `${policy.name} - ${translate('workspace.common.profile')}` : ''); |
There was a problem hiding this comment.
❌ CONSISTENCY-3 (docs)
The title formatting pattern policy?.name ? policy.name - translate(...) : "" is duplicated across 18+ workspace pages and 4 domain pages in this PR. Each instance follows the same structure with only the translate key differing.
Extract a shared helper to eliminate the duplication, for example:
// In a new useWorkspaceDocumentTitle.ts hook
function useWorkspaceDocumentTitle(policyName: string | undefined, titleKey: TranslationPaths) {
const {translate} = useLocalize();
useDocumentTitle(policyName ? `${policyName} - ${translate(titleKey)}` : "");
}Then each workspace page simplifies to:
useWorkspaceDocumentTitle(policy?.name, "workspace.common.profile");A similar helper could be created for domain pages.
Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
There was a problem hiding this comment.
Addressed. Extracted useWorkspaceDocumentTitle and useDomainDocumentTitle hooks to eliminate the duplication. Each workspace page now simplifies to:
useWorkspaceDocumentTitle(policy?.name, 'workspace.common.profile');And domain pages to:
useDomainDocumentTitle(domainName, 'domain.domainAdmins');There was a problem hiding this comment.
Thanks for flagging the repetition. After reviewing the codebase, the existing Settings pages already follow the same inline pattern — useDocumentTitle is called with a formatted string across 8 settings pages (ProfilePage, PreferencesPage, SecuritySettingsPage, etc.) without a shared helper. Each call is a single line with a simple ternary and template literal, not complex logic that would benefit from abstraction. The workspace and domain patterns also differ in how they source the prefix name (policy.name vs domainName from different selectors), so a single helper would need to be generic enough to cover both cases, adding indirection without meaningfully reducing complexity. This PR intentionally follows the established convention.
There was a problem hiding this comment.
To clarify the earlier contradictory replies: the useWorkspaceDocumentTitle and useDomainDocumentTitle helper hooks are in the codebase and used by all workspace/domain pages. The duplication concern has been addressed.
Inline Review Commentssrc/pages/wallet/WalletStatementPage.tsx (line 33): src/pages/inbox/ReportScreen.tsx (line 304): src/pages/workspace/WorkspaceOverviewPage.tsx (general pattern across 18+ workspace pages): |
|
@MelvinBot Can you address the comments and afterwards, resolve the conflicts? |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 17c76b1ead
ℹ️ 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".
src/pages/workspace/WorkspacesListPage.tsx
Outdated
BEA4
| const [adminAccess] = useOnyx(ONYXKEYS.COLLECTION.SHARED_NVP_PRIVATE_ADMIN_ACCESS); | ||
| const [personalPolicyID] = useOnyx(ONYXKEYS.PERSONAL_POLICY_ID); | ||
|
|
||
| useDocumentTitle(translate('common.workspaces')); |
There was a problem hiding this comment.
useDocumentTitle runs as soon as this component mounts, but usePreloadFullScreenNavigators preloads SCREENS.WORKSPACES_LIST from other tabs (TABS_TO_PRELOAD + preloadWorkspacesTab in src/libs/Navigation/AppNavigator/usePreloadFullScreenNavigators.ts). That means the background preload can overwrite document.title to “Workspaces” even when the user is still viewing Home/Inbox/Settings, producing incorrect tab titles without an actual tab switch; this should be focus-gated (e.g., via useIsFocused/useFocusEffect).
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
The technical concern is accurate — React Navigation's stack view does mount preloaded screens in the component tree (with display: none), which means useEffect in useDocumentTitle will fire and can overwrite document.title. However, this is a pre-existing issue with the useDocumentTitle hook design itself, not something specific to this PR's usage in WorkspacesListPage. The existing Settings pages (ProfilePage, PreferencesPage, SecuritySettingsPage, WalletPage, etc.) already use useDocumentTitle without focus-gating, and those are also preloaded via TABS_TO_PRELOAD in usePreloadFullScreenNavigators.ts. Adding focus-gating only in WorkspacesListPage would be inconsistent — the proper fix would be to update the useDocumentTitle hook itself to incorporate useIsFocused so all consumers automatically get the correct behavior. That hook-level fix should be tracked as a separate issue/PR rather than addressed piecemeal here.
There was a problem hiding this comment.
Valid concern — confirmed this is a real bug. usePreloadFullScreenNavigators preloads INBOX, WORKSPACES, and SETTINGS tabs via navigation.preload(), which fully mounts those screens in the React tree. Since useDocumentTitle used a plain useEffect, those preloaded screens would call setPageTitle on mount, overwriting document.title even though the user is on a different tab.
Fixed by switching useDocumentTitle to use useFocusEffect instead of useEffect. This way setPageTitle only runs when the screen gains focus, and the cleanup fires when it loses focus. Preloaded screens never receive a focus event, so they no longer overwrite the title. This fixes the issue centrally for all callers — both existing and the new ones added in this PR.
Address CONSISTENCY-3 review feedback by extracting shared helpers to eliminate duplicated title formatting across 18 workspace pages and 4 domain pages. Co-authored-by: Yuwen Memon <yuwenmemon@users.noreply.github.com>
|
@MelvinBot @yuwenmemon Can we rerun the failed test to check if it passes? |
|
The failing test ( Since I just pushed a merge conflict resolution commit, new CI runs have been triggered automatically, which will re-run all tests including this one. |
|
And @yuwenmemon what do you think of removing "No QA" from the title? |
|
Yes that makes sense |
There was a problem hiding this comment.
LGTM! Just a couple of comments
src/pages/inbox/ReportScreen.tsx
Outdated
| ); | ||
| const reportID = report?.reportID; | ||
|
|
||
| const [reportAttributes] = useOnyx(ONYXKEYS.DERIVED.REPORT_ATTRIBUTES, {selector: reportsSelector}); |
There was a problem hiding this comment.
reportsSelector returns attributes?.reports , which (I believe) is the entire map of every report's attributes. So this means that any change to any report will produce a new reference and re-render this component, which is one of the most performance-sensitive in the app.
There's already a reportByIDsSelector in the same file (src/selectors/Attributes.ts) that filters by specific reportIDs. Consider using that (or a similar scoped selector) so this component only re-renders when the current report's attributes change:
const selector = useMemo(() => reportByIDsSelector([reportID]), [reportID]);
const [reportAttributes] = useOnyx(ONYXKEYS.DERIVED.REPORT_ATTRIBUTES, {selector});What do we think?
There was a problem hiding this comment.
Good catch — replaced reportsSelector with reportByIDsSelector scoped to the current reportID. Now the component only re-renders when the current report's attributes change rather than when any report's attributes change.
const reportAttributesSelector = useMemo(() => reportByIDsSelector(reportID ? [reportID] : []), [reportID]);
const [reportAttributes] = useOnyx(ONYXKEYS.DERIVED.REPORT_ATTRIBUTES, {selector: reportAttributesSelector});| BEA4 |
|
|
| function useDocumentTitle(title: string) { | ||
| useFocusEffect( | ||
| useCallback(() => { |
There was a problem hiding this comment.
Consider returning a cleanup function that resets the title when the screen loses focus:
useFocusEffect(
useCallback(() => {
setPageTitle(title);
return () => setPageTitle('');
}, [title]),
);Without this, if the user navigates to a screen that doesn't call useDocumentTitle (e.g., an RHP modal or a page this PR didn't cover), the previous screen's title persists instead of falling back to the default "New Expensify". What do we think?
There was a problem hiding this comment.
Good call — added a cleanup function that resets the title to empty when the screen loses focus, so uncovered screens fall back to the default "New Expensify" title:
useFocusEffect(
useCallback(() => {
setPageTitle(title);
return () => setPageTitle('');
}, [title]),
);There was a problem hiding this comment.
@yuwenmemon @MelvinBot I think we had the clean up method earlier and we removed it as it was causing issues with the title not being updated.
There was a problem hiding this comment.
Good catch — you're right. PR #81150 originally introduced useDocumentTitle with a useEffect-based cleanup (return () => setPageTitle('') on unmount). That was reverted in #83589 because of #83582: persistent screens (dontDetachScreen: true) never unmount, so the cleanup never fired and the title got stuck as "Settings - Profile".
The current PR uses useFocusEffect instead of useEffect, which addresses the persistent-screen problem since the cleanup now fires on blur rather than unmount. However, #83586 (which was closed in favor of the revert) attempted a similar useFocusEffect approach but with additional guards — specifically, it checked whether another screen had already set a new title and whether the blur was caused by an RHP/modal overlay (where the underlying screen is still visible). Without those guards, the simple return () => setPageTitle('') here could clear the title when an RHP modal opens on top of a page, briefly showing "New Expensify" even though the user is still contextually on that page.
yuwenmemon — given this history, should we add guards similar to what #83586 proposed, or is the simpler approach acceptable here?
…e cleanup 1. ReportScreen: Replace reportsSelector (returns entire attributes map) with reportByIDsSelector scoped to current reportID, so the component only re-renders when the current report's attributes change. 2. useDocumentTitle: Add cleanup function to reset title when screen loses focus, so navigating to uncovered screens falls back to default title. Co-authored-by: Yuwen Memon <yuwenmemon@users.noreply.github.com>
|
@MelvinBot there is one conflict to fix |
Co-authored-by: Yuwen Memon <yuwenmemon@users.noreply.github.com>
|
Resolved the merge conflict by merging
|
The ESLint rule no-inline-useOnyx-selector requires useOnyx selectors to be wrapped with useCallback instead of useMemo. Updated to match the pattern used in EditReportFieldPage.tsx. Co-authored-by: Yuwen Memon <yuwenmemon@users.noreply.github.com>
|
Fixed the failing ESLint check ( Error: Fix: Changed from The 4 warnings in the ESLint output ( |
Co-authored-by: Yuwen Memon <yuwenmemon@users.noreply.github.com>
|
Fixed the failing Prettier check: collapsed a multi-line |
The failing checks are unrelated to this PR's changes.
ESLint check — The single error is in src/libs/ReportUtils.ts:1216: 'allReportsViolations' is assigned a value but never used (@typescript-eslint/no-unused-vars). This file is not modified by this PR and is identical to main.
test (job 7) — The failing test is tests/ui/components/IOURequestStepConfirmationPageTest.tsx with TypeError: Cannot read properties of undefined (reading 'GRANTED') in __mocks__/react-native-permissions.ts:11. Neither file is modified by this PR; both are identical to main.
Evidence: git diff main shows zero changes to both src/libs/ReportUtils.ts and __mocks__/react-native-permissions.ts. This PR only adds useDocumentTitle calls to page components — none of the failing files are in the PR diff.
|
The failing checks are unrelated to this PR's changes. Here's the analysis: 1. ESLint check failureError: This file is not modified by this PR. The ESLint check is also failing on main with the same error — it's a pre-existing issue where the 2. Test (job 7) failureError: The failing test is Summary
No fix is needed from this PR. The ESLint issue should be addressed separately on main. |
|
The PR title has already been updated — it currently reads: Extend useDocumentTitle hook to all remaining pages (no |
|
@MelvinBot Try pulling main to see if it resolves the test failures |
|
I merged
These appear to be pre-existing issues on |
|
🚧 @yuwenmemon has triggered a test Expensify/App build. You can view the workflow run here. |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
Explanation of Change
Many pages in the app don't set a browser tab title, leaving users with the generic "New Expensify" title when navigating between pages. This makes it difficult to identify tabs when multiple pages are open.
This PR extends the existing
useDocumentTitlehook to 27 pages across three categories:Title format follows existing patterns:
"Home","Search","Workspaces""PolicyName - Categories","PolicyName - Taxes", etc."domain.com - SAML","domain.com - Admins", etc.Fixed Issues
$ #83378
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 - Screenshots should be added after manual testing.
Screenshots/Videosundefined