Update url on navigation [WPB-27977] - #22271
Conversation
Covers Connect/Preferences/Cells/Meetings switching, conversation list tabs, and refresh staying on Meetings (WPB-27977).
Folders is a toggle, not an aria-selected tab, and a new team has no conversation search field.
thisisamir98
left a comment
thisisamir98
left a comment
There was a problem hiding this comment.
Meetings URL restore on refresh looks good.
Clearing the hash to / can reopen the last conversation and steal Cells or Connect a tick later. Please fix that race before merge.
If Meetings is turned off while that tab is selected, also clear #/meetings. The tab-keep tests should lock a successful showConversation, not a thrown lookup.
| // never run. Force route re-evaluation async, matching the timing of a real hashchange, so | ||
| // callers that synchronously update content state right after calling this still take effect | ||
| // first. | ||
| setTimeout(parseRoute, 0); |
There was a problem hiding this comment.
When the hash is already /, this schedules parseRoute on the next tick. / always opens the most recent conversation (showMostRecentConversation in appMain).
changeTab for Cells or Connect often calls setHistoryParam('/') while already on /. The timeout then runs after the tab switch. showConversation maps Cells back to Recent, so the user lands in a conversation instead of Files or Connect.
Could we skip the forced parse when the next view is not a conversation, or make / a no-op while Cells, Connect, Preferences, or Meetings is selected?
Please add a regression: start on Recent (#/), open Cells, wait one macrotask, still on Files, not a conversation.
| const isSwitchingConversationListTab = listState === ListState.CONVERSATIONS && isConversationListTab(nextTab); | ||
|
|
||
| if (shouldClearDeepLinkForTab(nextTab) && !isSwitchingConversationListTab) { | ||
| setHistoryParam('/'); |
There was a problem hiding this comment.
This is the call that hits the Router race. After setCurrentTab(CELLS) you still clear to /. If the hash did not change, Router re-parses / and the conversation handler can undo Cells.
Could we avoid forcing a / re-parse when switching to Cells or Connect? Setting setHistoryParam('/') is fine for leaving Meetings or Preferences. Re-running the / route should not open a conversation in that case.
| const contentViewModel = buildContentViewModel(); | ||
| useSidebarStore.getState().setCurrentTab(tab); | ||
|
|
||
| await expect(contentViewModel.showConversation({domain: 'example.com', id: 'conversation-id'})).rejects.toThrow(); |
There was a problem hiding this comment.
This asserts that conversation lookup throws, then that the tab is unchanged. That does not lock the real case: a conversation is shown while Cells or Connect is selected, and the tab stays.
Could we drive a successful showConversation and assert tab plus content, instead of a failed lookup?
Format conversations.test.tsx for Prettier and drop an unused pages binding that failed noUnusedLocals.
2 participants
Uh oh!
There was an error while loading. Please reload this page.