regression: go back instead of closing app on rooms search #6750
Conversation
WalkthroughAdds hardware back-button handling to RoomsListView: when search is active, a BackHandler listener stops the search and navigates back (consuming the event); otherwise it allows default back behavior. Also adds a new Maestro test config for the Android search flow. Changes
Sequence DiagramsequenceDiagram
actor User
participant AndroidBack as BackHandler
participant RoomsListView
participant Navigation
User->>AndroidBack: Press hardware back button
AndroidBack->>RoomsListView: Invoke hardwareBackPress listener
alt searchEnabled == true
RoomsListView->>RoomsListView: clear/stop search
RoomsListView->>Navigation: navigate back
RoomsListView-->>AndroidBack: return true (consume)
else searchEnabled == false
RoomsListView-->>AndroidBack: return false (allow default)
AndroidBack->>Navigation: perform default back navigation
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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 |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit 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 (1)
app/views/RoomsListView/index.tsx(2 hunks)
🔇 Additional comments (2)
app/views/RoomsListView/index.tsx (2)
2-3: LGTM!The new imports are necessary for the back button handling functionality.
54-64: Note: Hardware back button behavior is Android-only.The
BackHandlerAPI only affects Android's hardware back button and will have no effect on iOS. This is typically expected behavior, but as noted in the PR comments, iOS testing hasn't been completed yet. Ensure that the iOS back navigation works as expected through the native navigation gestures.
| const subscription = BackHandler.addEventListener('hardwareBackPress', () => { | ||
| if (searchEnabled) { | ||
| stopSearch(); | ||
| navigation.goBack(); |
There was a problem hiding this comment.
🧩 Analysis chain
Verify navigation stack to prevent unintended app exit.
The PR aims to prevent the app from exiting, but navigation.goBack() could still exit the app if the RoomsListView is at the bottom of the navigation stack. Consider checking navigation.canGoBack() before calling goBack(), or verify that there's always a screen to navigate back to in your navigation structure.
Alternatively, consider this defensive approach:
const subscription = BackHandler.addEventListener('hardwareBackPress', () => {
if (searchEnabled) {
stopSearch();
- navigation.goBack();
+ if (navigation.canGoBack()) {
+ navigation.goBack();
+ }
return true;
}
return false;🏁 Script executed:
#!/bin/bash
# Search for navigation setup and how RoomsListView is mounted in the navigation stack
rg -n -C 5 "RoomsListView" --type=typescript --type=tsx -g "!*.test.*" -g "!*.spec.*"Length of output: 34
---
🏁 Script executed:
```shell
#!/bin/bash
# Search for RoomsListView with corrected rg syntax
rg -n -C 5 "RoomsListView" --type ts --type tsx -g "!*.test.*" -g "!*.spec.*"
Length of output: 27
🏁 Script executed:
#!/bin/bash
# Search for navigation.goBack() calls in the codebase
rg -n "navigation\.goBack" --type ts --type tsx -g "!*.test.*" -g "!*.spec.*"Length of output: 27
🏁 Script executed:
#!/bin/bash
# Search for navigation.canGoBack usage
rg -n "canGoBack" --type ts --type tsx -g "!*.test.*" -g "!*.spec.*"Length of output: 27
🏁 Script executed:
#!/bin/bash
# Look for the specific file to examine the current implementation
fd "RoomsListView" --type fLength of output: 0
---
🏁 Script executed:
```shell
#!/bin/bash
# Search for RoomsListView without type restrictions
rg -n -C 5 "RoomsListView" -g "!*.test.*" -g "!*.spec.*"
Length of output: 11909
🏁 Script executed:
#!/bin/bash
# Search for navigation.goBack without type restrictions
rg -n "navigation\.goBack" -g "!*.test.*" -g "!*.spec.*"Length of output: 912
🏁 Script executed:
#!/bin/bash
# Look for any tsx or ts files in app directory
fd -e tsx -e ts "." app/ 2>/dev/null | head -20Length of output: 792
🏁 Script executed:
#!/bin/bash
# Search for RoomsListView with broader pattern
fd "RoomsListView" . 2>/dev/nullLength of output: 26
🏁 Script executed:
#!/bin/bash
# Check if app/views/RoomsListView/index.tsx exists
cat app/views/RoomsListView/index.tsx 2>/dev/null | head -80Length of output: 3592
Add canGoBack() check before calling navigation.goBack() to prevent unintended app exit.
The current implementation calls navigation.goBack() without verifying the navigation stack state. Since RoomsListView is positioned as the first screen in the navigation stack, calling goBack() when the user triggers back from search mode could exit the app. Add a defensive check:
const subscription = BackHandler.addEventListener('hardwareBackPress', () => {
if (searchEnabled) {
stopSearch();
- navigation.goBack();
+ if (navigation.canGoBack()) {
+ navigation.goBack();
+ }
return true;
}
return false;📝 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.
| navigation.goBack(); | |
| const subscription = BackHandler.addEventListener('hardwareBackPress', () => { | |
| if (searchEnabled) { | |
| stopSearch(); | |
| if (navigation.canGoBack()) { | |
| navigation.goBack(); | |
| } | |
| return true; | |
| } | |
| return false; |
🤖 Prompt for AI Agents
In app/views/RoomsListView/index.tsx around line 58, the code calls
navigation.goBack() unguarded which can exit the app when this view is the root;
wrap the call in a defensive check by calling navigation.canGoBack() first and
only invoking navigation.goBack() when it returns true (otherwise handle the
back action differently, e.g., dismiss search mode or no-op) so the app cannot
unintentionally close.
Rohit3523
temporarily deployed
to
approve_e2e_testing
GitHub Actions
Inactive
Rohit3523
had a problem deploying
to
experimental_android_build
GitHub Actions
Error
Rohit3523
had a problem deploying
to
official_ios_build
GitHub Actions
Error
Rohit3523
had a problem deploying
to
official_android_build
GitHub Actions
Error
Rohit3523
had a problem deploying
to
experimental_ios_build
GitHub Actions
Error
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.maestro/tests/room/search.yaml (1)
18-31: Minor observation: Search setup has a potentially redundant wait.After tapping the search button (line 23), the test waits for
rooms-list-viewto be visible (lines 24–26). Since the search overlay typically appears on top of the rooms list, the list is likely already visible. Consider removing this redundant wait to tighten the test flow, though the current approach is safe.
📜 Review details
Configuration used: CodeRabbit 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 (1)
.maestro/tests/room/search.yaml(1 hunks)
⏰ 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). (2)
- GitHub Check: ESLint and Test / run-eslint-and-test
- GitHub Check: format
🔇 Additional comments (1)
.maestro/tests/room/search.yaml (1)
1-41: Test structure aligns well with the PR fix for back-button navigation.The test correctly validates the intended behavior: two back presses while in search mode should exit the search (hiding the search input) without closing the app. The use of
extendedWaitUntilwith 60-second timeouts for each state transition is appropriate for integration testing.
Rohit3523
requested a deployment
to
official_android_build
GitHub Actions
Waiting
Rohit3523
requested a deployment
to
official_ios_build
GitHub Actions
Waiting
Rohit3523
requested a deployment
to
experimental_android_build
GitHub Actions
Waiting
Rohit3523
requested a deployment
to
experimental_ios_build
GitHub Actions
Waiting
diegolmello
changed the title
diegolmello
changed the title
Proposed changes
Currently when we go to room search and press back button 2 times, on the first press it hides the keyboard but on the second click it exit the app where we have to open the app again, so with this PR users will go back to room list.
Issue(s)
https://rocketchat.atlassian.net/browse/COMM-71
How to test or reproduce
Screenshots
Types of changes
Checklist
Further comments
Summary by CodeRabbit
New Features
Tests