MM-69016 Keep uncached DMs in Find Channels#36769
Conversation
Co-authored-by: mattermost-code <matty-code@mattermost.com>
Co-authored-by: mattermost-code <matty-code@mattermost.com>
Co-authored-by: mattermost-code <matty-code@mattermost.com>
📝 WalkthroughWalkthrough
ChangesDM Channel Profile Fallback Handling
🎯 4 (Complex) | ⏱️ ~45 minutes Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
webapp/channels/src/components/suggestion/switch_channel_provider.tsx (1)
985-999: ⚡ Quick winReuse existing
statevariable instead of callingthis.store.getState()again.Line 986 calls
this.store.getState()butstateis already assigned at line 964. Use the existing variable for consistency.♻️ Proposed fix
} else if (channel.type === Constants.DM_CHANNEL) { const userId = Utils.getUserIdFromChannelId(channel.name, getCurrentUserId(state)); - const user = getUser(this.store.getState(), userId); + const user = getUser(state, userId); if (user) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@webapp/channels/src/components/suggestion/switch_channel_provider.tsx` around lines 985 - 999, The code calls this.store.getState() to compute user via getUser(...), but a local state variable already exists (assigned earlier); replace this.store.getState() with the existing state variable to avoid redundant store access and ensure consistency—update the getUser call in the block that uses Utils.getUserIdFromChannelId/getCurrentUserId to pass state, and keep the rest of the userWrappedChannel/channel merging logic unchanged (references: Utils.getUserIdFromChannelId, getCurrentUserId, getUser, userWrappedChannel).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@webapp/channels/src/components/suggestion/switch_channel_provider.tsx`:
- Around line 985-999: The code calls this.store.getState() to compute user via
getUser(...), but a local state variable already exists (assigned earlier);
replace this.store.getState() with the existing state variable to avoid
redundant store access and ensure consistency—update the getUser call in the
block that uses Utils.getUserIdFromChannelId/getCurrentUserId to pass state, and
keep the rest of the userWrappedChannel/channel merging logic unchanged
(references: Utils.getUserIdFromChannelId, getCurrentUserId, getUser,
userWrappedChannel).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: f94da5c0-9c88-49b0-9b36-f6e5ea55dd52
📒 Files selected for processing (2)
webapp/channels/src/components/suggestion/switch_channel_provider.test.tsxwebapp/channels/src/components/suggestion/switch_channel_provider.tsx
Co-authored-by: mattermost-code <matty-code@mattermost.com>
|
Creating a new SpinWick test server using Mattermost Cloud. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Babysit complete — ready for human review
PR: MM-69016 Keep uncached DMs in Find Channels
Status
| Check | Result |
|---|---|
| CodeRabbit | Approved (coderabbitai, review on 5e36c0c; post-0dbdd47 summary: no actionable comments) |
| CI | All checks green on 0dbdd47 (Web App CI, Enterprise CI/tests, e2e cypress/playwright enterprise, Snyk, CodeQL, etc.) |
| Merge conflicts | None (mergeable: MERGEABLE) |
Changes made during babysit
- Addressed CodeRabbit nitpick: reuse existing
stateinwrapChannelsinstead ofthis.store.getState()forgetUser(0dbdd47).
Handoff
- Removed
AI/babysitlabel; addedSetup Cloud Test Server. - Requested human reviewer via automation workflow.
Sent by Cursor Automation: Matty Code (Babysit Until Human Review)
|
Mattermost Spinwick PR #36769 🎉 Test server created! Access here: https://mattermost-pr-36769-eu3tf.test.mattermost.cloud Installation ID: Credentials: Posted securely in this Mattermost channel - Look for PR #36769 |
hmhealey
left a comment
hmhealey
left a comment
There was a problem hiding this comment.
I don't think this is likely to solve the issue because the DM channel's display_name won't be set (see below).
The root cause of this is that the user isn't loaded properly, so I think the only way to fix this will be to make the SwitchChannelProvider able to load the user itself. If it was just needed to render the SwitchChannelSuggestion, that'd be much easier because we could just use the useUser hook I added, but the SwitchChannelProvider needs that information for sorting as well, so that's not an option here. Instead, I think we would need the SwitchChannelProvider to return dispatch getMissingProfilesByIds to load those, but it might be tricky to do that at the right time because of how formatGroup and fetchAndFormatRecentlyViewedChannels need to truncate the data, sort it, and fetch the users at different times. For recently viewed channels, I think the order is sort then truncate then fetch users (since the names don't affect sorting), and for unread channels or search results, we would fetch users then sort then truncate.
Separate from that, to test this properly, you'd need to ensure that the user isn't loaded in some other way before opening the channel switcher. You can check the Redux store using the dev tools when testing manually to ensure that the user wasn't loaded first, but for an automated test, I did that recently for a similar issue in #36548, so the same technique would work here
| id: 'direct_joram_user', | ||
| type: 'D', | ||
| name: 'current_user_id__joram_user', | ||
| display_name: 'Joram User', |
There was a problem hiding this comment.
I don't think these tests are valid because the display_name for DMs is empty in the Redux state while this assumes that it's already set to "Joram User". Some of the selectors will populate that based when the other user is loaded, but I think the problem here is that the user likely hasn't been loaded.
The same also applies to the group channel below, although they have a default display_name made up by concatenated usernames which you see in Ian's screenshot when he reported this. The key tell that it's the default display name is that it only shows usernames and it includes the current user in it
|
Looking! |
PR Feedback Resolution SummaryAddressed (2 items):
Declined (0 items): Needs Human Review (0 items): Verification:
|
|
New commit detected. SpinWick will upgrade if the updated docker image is available. |
|
Test server creation failed. Review the error details here. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
webapp/channels/src/components/suggestion/switch_channel_provider.tsx (1)
936-969: 💤 Low valueConsider adding cancellation checks after async profile loading.
Other async methods like
fetchAndFormatLocalSearchResultsandfetchUsersAndChannelscheckshouldCancelDispatchafter awaiting async operations to avoid processing stale results. This method has twoawaitpoints (lines 948 and 968) but no cancellation checks. If the user types a search term while profiles are loading, this method may still callresultsCallbackwith recently-viewed channels after the search results have already been dispatched.Proposed fix
await this.loadMissingDirectChannelProfiles(unreadChannelsExclMuted); + if (this.shouldCancelDispatch('')) { + return; + } state = this.store.getState(); ... await this.loadMissingDirectChannelProfiles(sortedRecentRawChannels); + if (this.shouldCancelDispatch('')) { + return; + } sortedRecentChannels = this.wrapChannels(sortedRecentRawChannels, Constants.MENTION_RECENT_CHANNELS);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@webapp/channels/src/components/suggestion/switch_channel_provider.tsx` around lines 936 - 969, After each await of this.loadMissingDirectChannelProfiles inside fetchAndFormatRecentlyViewedChannels (the two async points where profiles are loaded for unreadChannelsExclMuted and sortedRecentRawChannels), call this.shouldCancelDispatch() and return early if it returns true so you don't continue processing stale results; i.e., immediately after the first await (before re-reading state and continuing) and immediately after the second await (before wrapping channels and calling resultsCallback/fetchChannels), check shouldCancelDispatch and abort if canceled.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@webapp/channels/src/components/suggestion/switch_channel_provider.tsx`:
- Around line 936-969: After each await of this.loadMissingDirectChannelProfiles
inside fetchAndFormatRecentlyViewedChannels (the two async points where profiles
are loaded for unreadChannelsExclMuted and sortedRecentRawChannels), call
this.shouldCancelDispatch() and return early if it returns true so you don't
continue processing stale results; i.e., immediately after the first await
(before re-reading state and continuing) and immediately after the second await
(before wrapping channels and calling resultsCallback/fetchChannels), check
shouldCancelDispatch and abort if canceled.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 8d27e5da-9326-4ffe-adde-7b5961a31e99
📒 Files selected for processing (2)
webapp/channels/src/components/suggestion/switch_channel_provider.test.tsxwebapp/channels/src/components/suggestion/switch_channel_provider.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- webapp/channels/src/components/suggestion/switch_channel_provider.test.tsx
5 participants
Summary
Fixed Find Channels direct-message suggestions so existing DM channels are not dropped when the teammate profile has not been loaded yet. The fallback preserves the DM channel with its channel display data until the profile cache is warmed, keeping the direct DM ahead of matching group DMs.
Ticket Link
Fixes: https://mattermost.atlassian.net/browse/MM-69016
Screenshots
Before:
After:
Release Note
Change Impact: 🟡 Medium
Regression Risk: Changes update SwitchChannelProvider's formatting/wrapping logic and add async profile-loading for missing DM profiles; unit tests were added covering fallback paths, but the provider is widely used by the channel switcher and Find Channels UI so asynchronous profile resolution could alter suggestion ordering/visibility in edge cases. There's moderate risk of regressions in suggestion ordering, display names, or unread/badge rendering when profiles are slow to load.
QA Recommendation: Moderate manual QA recommended: exercise Find Channels and the channel switcher with teammate profiles initially uncached, validate DM vs group-DM ordering and display before/after profile cache warms, and run regression checks for suggestion uniqueness and unread/badge behavior.
Generated by CodeRabbitAI