fix: add clear back navigation when viewing direct messages #38136
Conversation
|
|
Looks like this PR is not ready to merge, because of the following issues:
Please fix the issues and try again If you have any trouble, please check the PR guidelines |
WalkthroughThe PR simplifies NavBarControlsSection by removing mobile-specific branching and unifying rendering with conditional group displays. It also adds a back navigation button to RoomHeader for direct message rooms, addressing navigation discoverability issues when viewing DMs. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Hardik301002
changed the title
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @apps/meteor/client/views/room/Header/RoomHeader.tsx:
- Around line 49-59: Several lines in the RoomHeader JSX (the Header block
containing the isDirectMessage conditional and IconButton) use spaces instead of
tabs; update the indentation for that block to use tabs consistently (replace
leading spaces on the lines containing Header, the isDirectMessage conditional,
IconButton props like icon/aria-label/title/onClick, and the slots?.start line)
so the file follows the project's useTabs: true Prettier rule while keeping the
existing structure and handlers (isDirectMessage, IconButton, handleBack,
slots?.start) unchanged.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
apps/meteor/client/navbar/NavBarControls/NavBarControlsSection.tsxapps/meteor/client/views/room/Header/RoomHeader.tsx
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.{ts,tsx,js}: Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Avoid code comments in the implementation
Files:
apps/meteor/client/views/room/Header/RoomHeader.tsxapps/meteor/client/navbar/NavBarControls/NavBarControlsSection.tsx
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: cubic · AI code reviewer
🔇 Additional comments (3)
apps/meteor/client/views/room/Header/RoomHeader.tsx (2)
7-9: LGTM: Import additions are appropriate.The imported
IconButtonanduseHistoryare correctly sourced and support the back navigation feature for direct messages.
39-45: The '/' destination is correct for back navigation. The root path acts as a navigation hub that either redirects to the user's default room or to/home(the workspace channel list), making it an appropriate destination for backing out of a direct message view.apps/meteor/client/navbar/NavBarControls/NavBarControlsSection.tsx (1)
20-32: Verify whether mobile/desktop rendering divergence is intentional and clarify AI summary accuracy.Two valid concerns:
AI summary conflict: The summary claims this creates a "unified render path," but the code clearly maintains separate mobile (lines 20–31) and desktop (lines 35–44) branches with an early return at line 20.
Mobile includes
NavBarControlsWithData, desktop does not: The mobile branch conditionally rendersNavBarControlsWithData(line 25) whenshowOmnichannel || callAction, while the desktop branch (lines 35–44) rendersNavBarVoipGroupandNavBarOmnichannelGroupdirectly withoutNavBarControlsWithData.This divergence may be intentional UX design (mobile consolidates controls into a menu component; desktop shows separate groups), but please confirm whether this asymmetry is the intended behavior.
| <Header> | ||
| {isDirectMessage && ( | ||
| <IconButton | ||
| icon='chevron-left' | ||
| small | ||
| aria-label={t('Back')} | ||
| title={t('Back')} | ||
| onClick={handleBack} | ||
| /> | ||
| )} | ||
| {slots?.start} |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's read the file to see the actual indentation at lines 49-59
head -60 apps/meteor/client/views/room/Header/RoomHeader.tsx | tail -20Repository: RocketChat/Rocket.Chat
Length of output: 365
🏁 Script executed:
# Let's see a bit more context (lines 40-70) to understand the indentation pattern
head -70 apps/meteor/client/views/room/Header/RoomHeader.tsx | tail -35Repository: RocketChat/Rocket.Chat
Length of output: 818
🏁 Script executed:
# Check if there's a prettier or eslint config to understand the project's indentation standard
find . -maxdepth 2 -type f \( -name ".prettierrc*" -o -name ".eslintrc*" -o -name "prettier.config.*" -o -name "eslint.config.*" \) | head -10Repository: RocketChat/Rocket.Chat
Length of output: 79
🏁 Script executed:
# Check the prettier configuration
cat .prettierrcRepository: RocketChat/Rocket.Chat
Length of output: 303
Fix indentation to use tabs throughout.
Lines 49-59 use spaces instead of tabs, violating the project's Prettier configuration (useTabs: true). The rest of the component correctly uses tabs (see line 60+). The back button placement before slots?.start is intentional and appropriate.
🔧 Proposed fix for indentation
- <Header>
- {isDirectMessage && (
- <IconButton
- icon='chevron-left'
- small
- aria-label={t('Back')}
- title={t('Back')}
- onClick={handleBack}
- />
- )}
- {slots?.start}
+ <Header>
+ {isDirectMessage && (
+ <IconButton
+ icon='chevron-left'
+ small
+ aria-label={t('Back')}
+ title={t('Back')}
+ onClick={handleBack}
+ />
+ )}
+ {slots?.start}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <Header> | |
| {isDirectMessage && ( | |
| <IconButton | |
| icon='chevron-left' | |
| small | |
| aria-label={t('Back')} | |
| title={t('Back')} | |
| onClick={handleBack} | |
| /> | |
| )} | |
| {slots?.start} | |
| <Header> | |
| {isDirectMessage && ( | |
| <IconButton | |
| icon='chevron-left' | |
| small | |
| aria-label={t('Back')} | |
| title={t('Back')} | |
| onClick={handleBack} | |
| /> | |
| )} | |
| {slots?.start} |
🤖 Prompt for AI Agents
In @apps/meteor/client/views/room/Header/RoomHeader.tsx around lines 49 - 59,
Several lines in the RoomHeader JSX (the Header block containing the
isDirectMessage conditional and IconButton) use spaces instead of tabs; update
the indentation for that block to use tabs consistently (replace leading spaces
on the lines containing Header, the isDirectMessage conditional, IconButton
props like icon/aria-label/title/onClick, and the slots?.start line) so the file
follows the project's useTabs: true Prettier rule while keeping the existing
structure and handlers (isDirectMessage, IconButton, handleBack, slots?.start)
unchanged.
|
Hi there, thanks for the contribution! 🚀 💯 Closing this PR because it's stale now. We came up with another solution for this here. Questions? Help needed? Feature Requests?
|
3 participants
What i have done-
Adds a visible back button in the header when viewing Direct Messages.
Why-
Users had no clear or discoverable way to return to the workspace from a DM.
How-
Shows a back navigation icon in the DM header that routes back to the workspace home.
Steps to test-
Fixes #38135
Summary by CodeRabbit
New Features
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.