[CP staging] Revert "Remove useDocumentTitle cleanup to fix tab title race condition"#86112
[CP staging] Revert "Remove useDocumentTitle cleanup to fix tab title race condition"#86112
Conversation
|
@MarioExpensify 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: 68737dca1b
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| useFocusEffect( | ||
| useCallback(() => { | ||
| setPageTitle(title); | ||
| return () => setPageTitle(''); |
There was a problem hiding this comment.
Don't reset the global tab title in the focus cleanup
In our React Navigation 7 setup, the function returned from useFocusEffect is an effect cleanup, not a blur-only handler. Calling setPageTitle('') here mutates the shared currentPageTitle in src/libs/UnreadIndicatorUpdater/updateUnread/index.ts, so when the user switches between two screens that both use useDocumentTitle (for example HomePage, SearchPage, or ReportScreen), the outgoing screen can clear the title after the incoming screen has already focused. Because the document title is applied asynchronously in updateDocumentTitle(), this leaves the browser tab stuck on CONFIG.SITE_TITLE/New Expensify instead of the new page title, reintroducing the split-pane title race this hook just stopped having.
Useful? React with 👍 / 👎.
|
Reverting the PR will cause #85352. Hence I was a bit concerned on straight revert. |
|
AI is going around in circles: #85473 |
|
I think we should move forward with what @MelvinBot proposed here instead: #86087 (comment) Otherwise, we'll just go back to a point where we cause a different blocker. |
|
I've created a separate draft PR implementing the proposed fix instead of a straight revert: #86142 This adds a |
yuwenmemon
left a comment
There was a problem hiding this comment.
Let's go with the alternative: #86142
Also, I would say this isn't a blocker since it's not a major deviation from the "bugginess" in production
Reverts #85473
$ #86087