chore: migrating touch component to RN touchable from RNGH#7473
chore: migrating touch component to RN touchable from RNGH#7473Rohit3523 wants to merge 8 commits into
Conversation
Rohit3523
had a problem deploying
to
approve_e2e_testing
GitHub Actions
Error
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
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 |
Rohit3523
had a problem deploying
to
approve_e2e_testing
GitHub Actions
Error
![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
🧹 Nitpick comments (1)
app/containers/Touch.tsx (1)
18-30: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider omitting
disabledfrom the extended interface.
ITouchPropsstill inheritsdisabled?: booleanfromTouchableWithoutFeedbackPropseven though the component's actual API contract moved toenabled. Since the render logic (Line 102) unconditionally overridesdisabledwith!enabled, any consumer that still passesdisabled(asMessage.tsxdoes) compiles fine but silently has no effect — see the critical issue raised inapp/containers/message/Message.tsx. UsingOmit<TouchableWithoutFeedbackProps, 'disabled'>would make this a type error instead of a silent runtime bug.♻️ Proposed fix
-export interface ITouchProps extends TouchableWithoutFeedbackProps { +export interface ITouchProps extends Omit<TouchableWithoutFeedbackProps, 'disabled'> {🤖 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 `@app/containers/Touch.tsx` around lines 18 - 30, ITouchProps currently inherits disabled from TouchableWithoutFeedbackProps even though Touch uses enabled as the real API, so passing disabled is silently ignored by the render logic in Touch. Update the Touch component’s props type to omit disabled from TouchableWithoutFeedbackProps, keep enabled as the only control flag, and then fix any callers such as Message to use enabled instead of disabled so TypeScript catches the bad usage.
🤖 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.
Inline comments:
In @.maestro/tests/room/message-markdown-click.yaml:
- Line 209: The text selector patterns in the message markdown click Maestro
YAML use the wrong contains-match form (`.*keyword*.`), which won’t match when
the keyword appears at the end of the element text. Update the affected selector
fields in this test to use the proper contains pattern (`.*keyword.*`)
consistently for the entries matching “User info” and the other two similar
cases in this file.
In `@app/containers/message/Message.tsx`:
- Line 26: The message press target is still using the old disabled prop on
Touch, but Touch now ignores it, so the Message component needs to gate
interaction with enabled instead. Update the touch wrapper in Message.tsx to
pass enabled={!isDisabled} and use the Message component’s existing
disabled-state logic (for info, archived, temp, and jitsi_call_started messages)
to determine whether the target should be tappable.
---
Nitpick comments:
In `@app/containers/Touch.tsx`:
- Around line 18-30: ITouchProps currently inherits disabled from
TouchableWithoutFeedbackProps even though Touch uses enabled as the real API, so
passing disabled is silently ignored by the render logic in Touch. Update the
Touch component’s props type to omit disabled from
TouchableWithoutFeedbackProps, keep enabled as the only control flag, and then
fix any callers such as Message to use enabled instead of disabled so TypeScript
catches the bad usage.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4d809f74-6526-42af-830d-d9488de4f6b2
📒 Files selected for processing (5)
.maestro/tests/room/message-markdown-click.yamlapp/containers/ThreadDetails.tsxapp/containers/Touch.tsxapp/containers/message/Message.tsxapp/containers/message/Touch.tsx
💤 Files with no reviewable changes (1)
- app/containers/message/Touch.tsx
📜 Review details
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{js,ts,jsx,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{js,ts,jsx,tsx}: Use descriptive names for functions, variables, and classes that clearly convey their purpose
Write comments that explain the 'why' behind code decisions, not the 'what'
Keep functions small and focused on a single responsibility
Use const by default, let when reassignment is needed, and avoid var
Prefer async/await over .then() chains for handling asynchronous operations
Use explicit error handling with try/catch blocks for async operations
Avoid deeply nested code; refactor complex logic into helper functions
Files:
app/containers/ThreadDetails.tsxapp/containers/message/Message.tsxapp/containers/Touch.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Use TypeScript for type safety; add explicit type annotations to function parameters and return types
Prefer interfaces over type aliases for defining object shapes in TypeScript
Use enums for sets of related constants rather than magic strings or numbersUse TypeScript with strict mode enabled
Files:
app/containers/ThreadDetails.tsxapp/containers/message/Message.tsxapp/containers/Touch.tsx
**/*.{js,jsx,ts,tsx,json}
📄 CodeRabbit inference engine (CLAUDE.md)
Use Prettier formatting with tabs, single quotes, 130 character line width, no trailing commas, and avoid arrow function parentheses
Files:
app/containers/ThreadDetails.tsxapp/containers/message/Message.tsxapp/containers/Touch.tsx
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Enforce ESLint rules from
@rocket.chat/eslint-configwith React, React Native, TypeScript, and Jest plugins
Files:
app/containers/ThreadDetails.tsxapp/containers/message/Message.tsxapp/containers/Touch.tsx
app/containers/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Place reusable UI components in 'app/containers/' directory
Files:
app/containers/ThreadDetails.tsxapp/containers/message/Message.tsxapp/containers/Touch.tsx
🧠 Learnings (4)
📚 Learning: 2026-04-30T17:07:51.020Z
Learnt from: diegolmello
Repo: RocketChat/Rocket.Chat.ReactNative PR: 7274
File: app/lib/services/voip/MediaCallEvents.ts:0-0
Timestamp: 2026-04-30T17:07:51.020Z
Learning: In this Rocket.Chat React Native codebase, the ESLint rule `no-void: error` is enforced. When you see a promise returned from an async call that is not awaited (a “floating promise”), do not silence it with the `void somePromise()` pattern. Instead, handle the promise explicitly by attaching `.catch(...)` (or otherwise awaiting/handling the error) so unhandled-rejection risks are addressed in a way that satisfies the existing ESLint configuration.
Applied to files:
app/containers/ThreadDetails.tsxapp/containers/message/Message.tsxapp/containers/Touch.tsx
📚 Learning: 2026-06-25T18:37:44.793Z
Learnt from: diegolmello
Repo: RocketChat/Rocket.Chat.ReactNative PR: 7434
File: app/views/ScreenLockConfigView.tsx:101-141
Timestamp: 2026-06-25T18:37:44.793Z
Learning: In the Rocket.Chat React Native codebase, do not treat passing an `async` function directly to an event prop in React/React Native UI components (e.g., `onPress={async () => ...}` in TSX) as a “floating promises” CI-blocking lint issue—this repo does not enable the ESLint `no-floating-promises` rule (while `no-void` is enforced). Only raise robustness follow-ups when there are genuinely unhandled promise paths (e.g., fire-and-forget calls like `save()` that return a Promise that is neither awaited nor handled), and prefer making sure failure paths are explicitly handled/reported rather than blocking on lint-style floating-promise concerns.
Applied to files:
app/containers/ThreadDetails.tsxapp/containers/message/Message.tsxapp/containers/Touch.tsx
📚 Learning: 2026-03-05T14:28:10.004Z
Learnt from: Rohit3523
Repo: RocketChat/Rocket.Chat.ReactNative PR: 6997
File: .maestro/tests/room/message-markdown-click.yaml:28-39
Timestamp: 2026-03-05T14:28:10.004Z
Learning: In Maestro YAML selector fields (text, id) within the Rocket.Chat React Native repository, use the contains pattern '.*keyword.*' (leading and trailing '.*') for matching text. The pattern '.*keyword*.' is incorrect and will fail to match cases where the keyword appears at the end of the element's text. This guideline applies to all Maestro YAML selector fields across the codebase.
Applied to files:
.maestro/tests/room/message-markdown-click.yaml
📚 Learning: 2026-03-17T19:15:26.536Z
Learnt from: Rohit3523
Repo: RocketChat/Rocket.Chat.ReactNative PR: 6970
File: .maestro/tests/room/share-message.yaml:77-79
Timestamp: 2026-03-17T19:15:26.536Z
Learning: In YAML test files under .maestro/tests/room, use tapping the empty area (e.g., tapOn: point: 5%,10%) to dismiss both the bottom sheet and keyboard when needed. Do not rely on action-sheet-handle alone if the keyboard also needs to be dismissed in the same step. This pattern is acceptable for tests where a single tap should close both UI elements.
Applied to files:
.maestro/tests/room/message-markdown-click.yaml
🔇 Additional comments (11)
app/containers/Touch.tsx (3)
1-18: LGTM!
50-109: LGTM!Accessibility props moved onto the focusable touchable and the inner
Viewkept layout-only — this correctly avoids duplicate a11y announcements.enabled-drivendisabled/focusable/canBeFocusedwiring is consistent with the new API (aside from the cross-filedisabledcontract issue flagged separately).
32-33: 🎯 Functional Correctnessapp/containers/Touch.tsx:32-33 — Check the platform-union typing
withKeyboardFocus(Component)still receivesTouchableOpacity | TouchableHighlight, soKeyboardComponentmay only expose the shared touchable props; the Android-onlyunderlayColorpath needs a type check againstreact-native-external-keyboard@0.9.0.app/containers/ThreadDetails.tsx (1)
82-85: LGTM!Also applies to: 94-94
.maestro/tests/room/message-markdown-click.yaml (7)
6-18: LGTM!
62-62: LGTM!Also applies to: 142-142, 162-162, 182-182
77-77: LGTM!Also applies to: 105-105
194-204: LGTM!Also applies to: 212-218, 228-269, 281-287
219-250: 🎯 Functional Correctness
thread-follow/thread-unfolloware already rendered in the room thread preview, so these taps are in the right context.> Likely an incorrect or invalid review comment.
271-280: 🎯 Functional CorrectnessConfirm the command spelling
The slash command and autocomplete id need to use the same spelling as the app; ifemphemeralis not the real identifier, this step will never match.
205-210: 🎯 Functional CorrectnessConfirm the avatar target is still the seeded sender.
rohit.bansalis hardcoded here, but this flow only shows admin login intomaestro-message-clickable-test-temp; if the room’s author changes, this step will fail.
| - longPressOn: "rohit.bansal's avatar" | ||
| - extendedWaitUntil: | ||
| visible: | ||
| text: '.*User info*.' |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Incorrect contains-pattern for text selectors.
These text selectors use '.*keyword*.' instead of the correct '.*keyword.*' form (leading and trailing .*). Since the target keyword sits at the end of the matched string in all three cases, the pattern will fail to match at runtime.
Based on learnings, in Maestro YAML selector fields, use the contains pattern '.*keyword.*' (leading and trailing .*); '.*keyword*.' is incorrect and will fail to match cases where the keyword appears at the end of the element's text.
🩹 Proposed fix
- text: '.*User info*.'
+ text: '.*User info.*'- text: '.*Button tap received! This reply is private to you*.'
+ text: '.*Button tap received! This reply is private to you.*'- text: '.*A very secret message visible after clicking Ephemeral Button*.'
+ text: '.*A very secret message visible after clicking Ephemeral Button.*'Also applies to: 267-267, 286-286
🤖 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 @.maestro/tests/room/message-markdown-click.yaml at line 209, The text
selector patterns in the message markdown click Maestro YAML use the wrong
contains-match form (`.*keyword*.`), which won’t match when the keyword appears
at the end of the element text. Update the affected selector fields in this test
to use the proper contains pattern (`.*keyword.*`) consistently for the entries
matching “User info” and the other two similar cases in this file.
Source: Learnings
| import { useResponsiveLayout } from '../../lib/hooks/useResponsiveLayout/useResponsiveLayout'; | ||
| import Quote from './Components/Attachments/Quote'; | ||
| import Touch from './Touch'; | ||
| import Touch from '../Touch'; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== File map ==\n'
git ls-files app/containers/message/Message.tsx app/containers/Touch.tsx app/containers/message | sed 's#^`#-` #'
printf '\n== Relevant outlines ==\n'
ast-grep outline app/containers/message/Message.tsx --view expanded || true
printf '\n---\n'
ast-grep outline app/containers/Touch.tsx --view expanded || true
printf '\n== Targeted reads ==\n'
sed -n '1,120p' app/containers/Touch.tsx
printf '\n--- Message.tsx around import and Touch usage ---\n'
sed -n '1,260p' app/containers/message/Message.tsx
printf '\n== Search for enabled/disabled usage with Touch ==\n'
rg -n "enabled=|disabled=|<Touch|Touch " app/containers/message app/containers/Touch.tsxRepository: RocketChat/Rocket.Chat.ReactNative
Length of output: 50390
Use enabled={!isDisabled} here
Touch ignores disabled now, so this prop no longer disables the message press target. Info, archived, temp, and jitsi_call_started messages stay tappable.
Suggested fix
- disabled={isDisabled}
+ enabled={!isDisabled}📝 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.
| import Touch from '../Touch'; | |
| enabled={!isDisabled} |
🤖 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 `@app/containers/message/Message.tsx` at line 26, The message press target is
still using the old disabled prop on Touch, but Touch now ignores it, so the
Message component needs to gate interaction with enabled instead. Update the
touch wrapper in Message.tsx to pass enabled={!isDisabled} and use the Message
component’s existing disabled-state logic (for info, archived, temp, and
jitsi_call_started messages) to determine whether the target should be tappable.
1 participant
Proposed changes
Note: This PR is related to the UI Kit button not being clickable on some Android devices. It is not the final solution, as I plan to test multiple approaches before finalizing it.
Issue(s)
How to test or reproduce
Screenshots
Types of changes
Checklist
Further comments
Summary by CodeRabbit