Open
Conversation
OtavioStasiak
had a problem deploying
to
approve_e2e_testing
GitHub Actions
Error
Contributor
WalkthroughAdds Android keyboard accessibility for inverted message lists: new TurboModule to track keyboard-a11y state, focus utilities and native inverted scroll views that handle inverted navigation and exit focus, MainActivity remaps keyboard events when enabled, and React components/props updated to bridge native focus behavior. Changes
Sequence DiagramsequenceDiagram
actor User
participant MainActivity
participant KeyboardA11yModule
participant FocusUtils
participant InvertedScrollView
participant RoomHeader
User->>MainActivity: Press TAB / DPAD key
MainActivity->>KeyboardA11yModule: isEnabled()
KeyboardA11yModule-->>MainActivity: true
MainActivity->>FocusUtils: hasInvertedParent(focusedView)?
FocusUtils-->>MainActivity: true
MainActivity->>MainActivity: Remap key (TAB ↔ DPAD up/down)
MainActivity->>InvertedScrollView: Dispatch remapped KeyEvent
InvertedScrollView->>InvertedScrollView: Move focus between list cells
alt reached list boundary with exitFocusNativeId
InvertedScrollView->>RoomHeader: focus exit target (nativeID lookup)
RoomHeader->>RoomHeader: set accessibility focus (useImperativeHandle)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes 🚥 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. 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 |
OtavioStasiak
had a problem deploying
to
official_android_build
GitHub Actions
Error
OtavioStasiak
had a problem deploying
to
official_ios_build
GitHub Actions
Error
OtavioStasiak
had a problem deploying
to
experimental_ios_build
GitHub Actions
Error
OtavioStasiak
temporarily deployed
to
experimental_android_build
GitHub Actions
Inactive
OtavioStasiak
had a problem deploying
to
upload_experimental_android
GitHub Actions
Error
|
Android Build Available Rocket.Chat Experimental 4.70.0.108314 Internal App Sharing: https://play.google.com/apps/test/RQVpXLytHNc/ahAO29uNSHg7TdML4PFPLPaD3vbMSQN1kCdTXPzueVdh4N44q6GZfR1wk0ZNlhk_GdG11POn-I-qGjSYJWmIjmQQKs |
OtavioStasiak
had a problem deploying
to
approve_e2e_testing
GitHub Actions
Error
OtavioStasiak
had a problem deploying
to
approve_e2e_testing
GitHub Actions
Error
OtavioStasiak
temporarily deployed
to
experimental_android_build
GitHub Actions
Inactive
OtavioStasiak
had a problem deploying
to
official_android_build
GitHub Actions
Error
OtavioStasiak
had a problem deploying
to
official_ios_build
GitHub Actions
Error
OtavioStasiak
had a problem deploying
to
experimental_ios_build
GitHub Actions
Error
OtavioStasiak
had a problem deploying
to
upload_experimental_android
GitHub Actions
Error
|
Android Build Available Rocket.Chat Experimental 4.70.0.108317 Internal App Sharing: https://play.google.com/apps/test/RQVpXLytHNc/ahAO29uNRigBMM3Bes80dsLl0Wof7_pwq8w6zFWBUUGsRGp2rRCO4FnN6K1TeJ4P-FQhppg3r60B8pyKQNdZQTh6Zy |
OtavioStasiak
had a problem deploying
to
approve_e2e_testing
GitHub Actions
Error
OtavioStasiak
temporarily deployed
to
experimental_android_build
GitHub Actions
Inactive
OtavioStasiak
had a problem deploying
to
experimental_ios_build
GitHub Actions
Error
OtavioStasiak
had a problem deploying
to
official_android_build
GitHub Actions
Error
OtavioStasiak
had a problem deploying
to
official_ios_build
GitHub Actions
Error
OtavioStasiak
had a problem deploying
to
upload_experimental_android
GitHub Actions
Error
|
Android Build Available Rocket.Chat Experimental 4.70.0.108318 Internal App Sharing: https://play.google.com/apps/test/RQVpXLytHNc/ahAO29uNQfUhHNuTEb7FEFekyzv7gX_VAjowLUgqhtyPgrwQ7XZI58MKTI6S0_cS2vJpxejQfVdMxEh288m2qGEJxg |
OtavioStasiak
had a problem deploying
to
approve_e2e_testing
GitHub Actions
Error
OtavioStasiak
temporarily deployed
to
experimental_android_build
GitHub Actions
Inactive
OtavioStasiak
had a problem deploying
to
official_android_build
GitHub Actions
Error
OtavioStasiak
had a problem deploying
to
experimental_ios_build
GitHub Actions
Error
OtavioStasiak
had a problem deploying
to
official_ios_build
GitHub Actions
Error
OtavioStasiak
had a problem deploying
to
upload_experimental_android
GitHub Actions
Error
|
Android Build Available Rocket.Chat Experimental 4.71.0.108402 Internal App Sharing: https://play.google.com/apps/test/RQVpXLytHNc/ahAO29uNTYno_mVYZWLgeeLqqvSYzqCCdxONBflb9pYdCVAGkOSK_kszp7K1CMQb_acDzxkdFxSBa0aBqioyY5I5hV |
OtavioStasiak
had a problem deploying
to
approve_e2e_testing
GitHub Actions
Error
OtavioStasiak
had a problem deploying
to
official_ios_build
GitHub Actions
Error
OtavioStasiak
had a problem deploying
to
official_android_build
GitHub Actions
Error
|
Android Build Available Rocket.Chat Experimental 4.71.0.108405 Internal App Sharing: https://play.google.com/apps/test/RQVpXLytHNc/ahAO29uNTIPfz9s2BYeUgTdgwAbLARyfJ33Zu2uCC3-3khGJcUxnB3F036QKV341pD1p8E0edHZAtGH8Rj5BHqnrWZ |
Contributor
Author
|
@CodeRabbit review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
app/views/RoomView/List/components/RNLikeInvertedScrollView.tsx (1)
37-37: Assigning tocreateRef().currentis a TypeScript anti-pattern.
React.createRef()returns aRefObjectwith a readonlycurrentproperty. While this works at runtime, it bypasses TypeScript's intent. Since you need a mutable ref, use{ current: null }directly or a callback ref pattern:♻️ Proposed fix using mutable ref object
- private scrollRef = React.createRef<any>(); + private scrollRef: { current: any } = { current: null };Also applies to: 159-170
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/views/RoomView/List/components/RNLikeInvertedScrollView.tsx` at line 37, Replace the readonly RefObject created with React.createRef() with a mutable ref object so assignments to .current are type-safe: change the declaration private scrollRef = React.createRef<any>(); to a MutableRefObject like private scrollRef: React.MutableRefObject<any> = { current: null }; and update the other similar refs (the ones referenced in the later block that assign to .current) to use the same pattern or a callback ref; keep usages that set scrollRef.current unchanged but ensure their ref declarations are MutableRefObject types (referencing scrollRef and the other ref identifiers in this file).app/lib/native/KeyboardInversionA11yAndroid.ts (1)
17-19: Redundant default parameter.Since
scopeis typed as literal'room-view', the default value doesn't add flexibility—callers can only ever pass'room-view'. Consider simplifying:♻️ Proposed simplification
-export const enableRoomViewKeyboardA11y = (scope: 'room-view' = 'room-view') => { - NativeModule.enable(scope); +export const enableRoomViewKeyboardA11y = () => { + NativeModule.enable('room-view'); };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/lib/native/KeyboardInversionA11yAndroid.ts` around lines 17 - 19, The function enableRoomViewKeyboardA11y currently declares a redundant literal-typed parameter scope with a default; remove the parameter and hardcode the literal when calling NativeModule.enable (i.e., change enableRoomViewKeyboardA11y to take no args and call NativeModule.enable('room-view')), keeping the function and NativeModule.enable usage to locate the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@android/app/src/main/java/chat/rocket/reactnative/a11y/KeyboardA11yModule.java`:
- Around line 30-40: The enable()/disable() methods perform two separate
volatile writes to sEnabled and sScope which can be observed in an inconsistent
intermediate state by readers such as MainActivity.dispatchKeyEvent or getState;
wrap the compound write in a single atomic operation by synchronizing or using
an atomic holder: either place both assignments inside a synchronized block
(e.g., synchronized on KeyboardA11yModule.class) in enable(String) and
disable(), or replace sEnabled/sScope with a single AtomicReference or immutable
state object (e.g., KeyboardA11yState) updated atomically; update readers
(getState, dispatchKeyEvent) to read the single synchronized/atomic state to
ensure visibility and consistency.
In `@android/app/src/main/java/chat/rocket/reactnative/MainActivity.kt`:
- Around line 47-82: The current dispatchKeyEvent remaps only ACTION_DOWN events
causing mismatched ACTION_UP keycodes; update the condition that checks the
event action in dispatchKeyEvent (used with KeyboardA11yModule.isEnabled() and
FocusUtils.hasInvertedParent(current)) to also handle KeyEvent.ACTION_UP (e.g.,
if event.action == KeyEvent.ACTION_DOWN || event.action == KeyEvent.ACTION_UP),
perform the same keyCode mapping logic for ACTION_UP, and construct/dispatch the
inverted KeyEvent (preserving downTime, eventTime, repeatCount, metaState,
deviceId, scanCode, flags, source) so both press and release events are
consistently remapped.
In
`@android/app/src/main/java/chat/rocket/reactnative/scroll/InvertedScrollView.java`:
- Line 69: In InvertedScrollView.java the code casts getChildAt(0) directly to
ViewGroup into variable contentView which can throw ClassCastException if the
child is not a ViewGroup; change this to first retrieve the child into a View,
check "instanceof ViewGroup" (and for null), then assign to contentView only
when the check passes, and handle the else case safely (e.g., return early, fall
back to a no-op, or log an error) so the view hierarchy change won't crash the
method.
- Around line 85-92: The current focus-handling in InvertedScrollView (around
variables exitDir and exitTarget) always returns true even when
findExitTarget(exitDir) returns null, which consumes the key at the list
boundary and can trap keyboard users; update the method so that when exitTarget
== null it does not consume the event—either return false to allow default focus
handling or trigger accessible feedback (e.g., call
announceForAccessibility(...) or an EdgeEffect bounce) before returning true;
locate the block using exitDir/exitTarget in InvertedScrollView and replace the
final unconditional "return true" with logic that returns false when exitTarget
is null (or invokes an accessibility feedback helper then returns true) so users
receive proper feedback or focus moves.
In `@app/containers/Button/index.tsx`:
- Around line 91-101: Add an explicit accessibilityState to the RectButton so
screen readers announce when the button is disabled: update the RectButton (in
this file's Button component) to include accessibilityState={{ disabled:
!!isDisabled }} alongside existing props (e.g., onPress, enabled={!isDisabled},
accessibilityLabel={title}) so the disabled state is conveyed to assistive tech.
---
Nitpick comments:
In `@app/lib/native/KeyboardInversionA11yAndroid.ts`:
- Around line 17-19: The function enableRoomViewKeyboardA11y currently declares
a redundant literal-typed parameter scope with a default; remove the parameter
and hardcode the literal when calling NativeModule.enable (i.e., change
enableRoomViewKeyboardA11y to take no args and call
NativeModule.enable('room-view')), keeping the function and NativeModule.enable
usage to locate the change.
In `@app/views/RoomView/List/components/RNLikeInvertedScrollView.tsx`:
- Line 37: Replace the readonly RefObject created with React.createRef() with a
mutable ref object so assignments to .current are type-safe: change the
declaration private scrollRef = React.createRef<any>(); to a MutableRefObject
like private scrollRef: React.MutableRefObject<any> = { current: null }; and
update the other similar refs (the ones referenced in the later block that
assign to .current) to use the same pattern or a callback ref; keep usages that
set scrollRef.current unchanged but ensure their ref declarations are
MutableRefObject types (referencing scrollRef and the other ref identifiers in
this file).
🪄 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: 3ccf1ed2-867a-4803-8928-cebf49d7f3ba
⛔ Files ignored due to path filters (6)
app/containers/Button/__snapshots__/Button.test.tsx.snapis excluded by!**/*.snapapp/containers/LoginServices/__snapshots__/LoginServices.test.tsx.snapis excluded by!**/*.snapapp/containers/UIKit/__snapshots__/UiKitMessage.test.tsx.snapis excluded by!**/*.snapapp/containers/UIKit/__snapshots__/UiKitModal.test.tsx.snapis excluded by!**/*.snapapp/containers/message/__snapshots__/Message.test.tsx.snapis excluded by!**/*.snapapp/views/CannedResponsesListView/__snapshots__/CannedResponseItem.test.tsx.snapis excluded by!**/*.snap
📒 Files selected for processing (22)
android/app/src/main/java/chat/rocket/reactnative/MainActivity.ktandroid/app/src/main/java/chat/rocket/reactnative/a11y/KeyboardA11yModule.javaandroid/app/src/main/java/chat/rocket/reactnative/a11y/KeyboardA11ySpec.javaandroid/app/src/main/java/chat/rocket/reactnative/scroll/FocusUtils.javaandroid/app/src/main/java/chat/rocket/reactnative/scroll/InvertedScrollContentView.javaandroid/app/src/main/java/chat/rocket/reactnative/scroll/InvertedScrollContentViewManager.javaandroid/app/src/main/java/chat/rocket/reactnative/scroll/InvertedScrollView.javaandroid/app/src/main/java/chat/rocket/reactnative/scroll/InvertedScrollViewManager.javaandroid/app/src/main/res/values/ids.xmlapp/containers/Button/Button.test.tsxapp/containers/Button/index.tsxapp/containers/MessageComposer/interfaces.tsapp/containers/RoomHeader/RoomHeader.tsxapp/containers/RoomHeader/index.tsxapp/lib/methods/helpers/goRoom.tsapp/lib/native/KeyboardInversionA11yAndroid.tsapp/stacks/types.tsapp/views/RoomView/List/components/InvertedScrollView.tsxapp/views/RoomView/List/components/List.tsxapp/views/RoomView/List/components/RNLikeInvertedScrollView.tsxapp/views/RoomView/index.tsxapp/views/RoomsListView/index.tsx
📜 Review details
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: OtavioStasiak
Repo: RocketChat/Rocket.Chat.ReactNative PR: 6499
File: app/containers/ServerItem/index.tsx:34-36
Timestamp: 2025-12-17T15:56:22.578Z
Learning: In the Rocket.Chat React Native codebase, for radio button components on iOS, include the selection state ("Selected"/"Unselected") in the accessibilityLabel instead of using accessibilityState={{ checked: hasCheck }}, because iOS VoiceOver has known issues with accessibilityRole="radio" + accessibilityState that prevent correct state announcement.
📚 Learning: 2025-12-17T15:56:22.578Z
Learnt from: OtavioStasiak
Repo: RocketChat/Rocket.Chat.ReactNative PR: 6499
File: app/containers/ServerItem/index.tsx:34-36
Timestamp: 2025-12-17T15:56:22.578Z
Learning: In the Rocket.Chat React Native codebase, for radio button components on iOS, include the selection state ("Selected"/"Unselected") in the accessibilityLabel instead of using accessibilityState={{ checked: hasCheck }}, because iOS VoiceOver has known issues with accessibilityRole="radio" + accessibilityState that prevent correct state announcement.
Applied to files:
android/app/src/main/java/chat/rocket/reactnative/scroll/InvertedScrollContentView.javaandroid/app/src/main/java/chat/rocket/reactnative/a11y/KeyboardA11yModule.javaapp/containers/RoomHeader/RoomHeader.tsx
📚 Learning: 2026-03-04T20:13:17.288Z
Learnt from: divyanshu-patil
Repo: RocketChat/Rocket.Chat.ReactNative PR: 6957
File: ios/RocketChat Watch App/Views/MessageComposerView.swift:37-55
Timestamp: 2026-03-04T20:13:17.288Z
Learning: In the WatchOS app (ios/RocketChat Watch App) for Rocket.Chat React Native, using SwiftUI `Button` inside a `ScrollView` on WatchOS causes accidental message sends because button tap targets can be triggered during scroll gestures. `Text` with `.onTapGesture` is the preferred pattern for tappable items in scroll views on WatchOS. To preserve accessibility, add `.accessibilityAddTraits(.isButton)` and `.accessibilityLabel()` to the `Text` element instead.
Applied to files:
app/containers/Button/index.tsxapp/containers/RoomHeader/RoomHeader.tsx
📚 Learning: 2026-03-10T15:21:45.098Z
Learnt from: Rohit3523
Repo: RocketChat/Rocket.Chat.ReactNative PR: 7046
File: app/containers/InAppNotification/NotifierComponent.stories.tsx:46-75
Timestamp: 2026-03-10T15:21:45.098Z
Learning: In `app/containers/InAppNotification/NotifierComponent.tsx` (React Native, Rocket.Chat), `NotifierComponent` is exported as a Redux-connected component via `connect(mapStateToProps)`. The `isMasterDetail` prop is automatically injected from `state.app.isMasterDetail` and does not need to be passed explicitly at call sites or in Storybook stories that use the default (connected) export.
Applied to files:
app/containers/RoomHeader/index.tsxapp/views/RoomView/index.tsx
🔇 Additional comments (25)
android/app/src/main/res/values/ids.xml (1)
1-5: LGTM!The resource ID declaration follows Android conventions and properly supports the native view tagging mechanism for inverted list detection.
app/containers/RoomHeader/RoomHeader.tsx (1)
138-174: LGTM!The
forwardRefconversion anduseImperativeHandleimplementation correctly expose an imperativefocus()method. The use offindNodeHandlewithAccessibilityInfo.setAccessibilityFocusis the appropriate pattern for programmatic accessibility focus in React Native.app/containers/RoomHeader/index.tsx (1)
27-104: LGTM!The
React.memo(React.forwardRef(...))composition is correctly implemented, properly forwarding the ref to the innerRoomHeadercomponent while maintaining memoization benefits.app/containers/MessageComposer/interfaces.ts (1)
5-10: LGTM!The
focus()method addition toIMessageComposerRefaligns with the existingIComposerInput.focus()contract and provides a consistent imperative API for programmatic focus control.app/views/RoomsListView/index.tsx (1)
76-76: LGTM!Passing
focusHeaderOnOpen: trueenables proper keyboard focus management when navigating to rooms. ThegoRoomhelper correctly applies this only in master-detail mode.app/views/RoomView/List/components/List.tsx (1)
47-49: LGTM!The
exitFocusNativeIdprop correctly configures the Android-specific focus exit target, enabling keyboard navigation to reach the message composer from the inverted message list.app/stacks/types.ts (1)
44-44: LGTM!The optional
focusHeaderOnOpenparameter correctly extends theRoomViewroute params type to support the keyboard focus navigation feature.app/lib/methods/helpers/goRoom.ts (2)
28-47: LGTM!The
focusHeaderOnOpenparameter integration correctly extends the navigation helper to support header focus in master-detail mode while preserving existing route param behavior.
94-104: LGTM!The type declaration properly documents the new optional
focusHeaderOnOpenparameter alongside existing navigation options.android/app/src/main/java/chat/rocket/reactnative/scroll/InvertedScrollContentViewManager.java (1)
27-30: LGTM!The
@ReactPropimplementation follows React Native conventions correctly, forwarding the boolean prop to the native view.android/app/src/main/java/chat/rocket/reactnative/scroll/InvertedScrollViewManager.java (1)
29-32: LGTM!The
@ReactPropforexitFocusNativeIdis correctly implemented with@Nullableannotation for the optional string prop.android/app/src/main/java/chat/rocket/reactnative/scroll/FocusUtils.java (1)
14-28: LGTM!The utility method is well-implemented with proper null safety and type checking. The parent chain traversal is safe and correctly terminates when reaching a non-View parent.
android/app/src/main/java/chat/rocket/reactnative/scroll/InvertedScrollContentView.java (2)
39-53: Consider checking focusability in addition to visibility.In
onRequestFocusInDescendants, the loop checkschild.getVisibility() == VISIBLEbut doesn't verify if the child is actually focusable before callingrequestFocus(). WhilerequestFocus()will returnfalsefor non-focusable views, you could optimize by also checkingchild.isFocusable()or usechild.requestFocus()directly as you're doing (since it handles this internally).The current implementation is functionally correct, just noting this as a minor observation.
55-77: LGTM!The
addFocusablesoverride correctly reverses only the direct children's focusables in the list while preserving the order of focusables from nested descendants. The in-place swap algorithm is efficient.app/views/RoomView/index.tsx (2)
258-267: LGTM!The focus handling logic is well-structured:
- Uses
InteractionManager.runAfterInteractionsto avoid animation conflicts- Properly clears
focusHeaderOnOpenparam after focusing to prevent repeated focus- Correctly falls back to composer focus when header focus isn't needed
1574-1578: LGTM!The
nativeID='message-composer-exit-focus'correctly matches theexitFocusNativeIdprop passed to the inverted scroll component, enabling focus to exit the inverted list and land on the composer.android/app/src/main/java/chat/rocket/reactnative/a11y/KeyboardA11ySpec.java (1)
9-30: LGTM!The TurboModule spec follows React Native conventions correctly with proper
@ReactMethodannotations and the asyncPromiseparameter forgetState.app/views/RoomView/List/components/InvertedScrollView.tsx (1)
1-10: LGTM!Clean re-export pattern with proper TypeScript typing. The
exitFocusNativeIdprop extension aligns with the native Android implementation.app/lib/native/KeyboardInversionA11yAndroid.ts (1)
21-25: LGTM!Clean TurboModule wrapper with proper type definitions.
android/app/src/main/java/chat/rocket/reactnative/scroll/InvertedScrollView.java (1)
95-136: LGTM!The helper methods
findContainingCellIndex,findExitTarget, andisDescendantOfare well-implemented. The navigation logic correctly handles the inverted view-tree order by reversing the iteration direction.app/views/RoomView/List/components/RNLikeInvertedScrollView.tsx (3)
50-58: LGTM!Proper keyboard listener lifecycle management with cleanup in
componentWillUnmount.
172-203: LGTM!The render method correctly composes styles, forwards props including
exitFocusNativeId, and properly wraps children in the native content view withcollapsable={false}to prevent view flattening.
224-228: LGTM!Clean
forwardRefwrapper withdisplayNamefor easier debugging.app/containers/Button/Button.test.tsx (1)
67-67: Comment update is accurate and helpful.The revised note correctly documents the Jest/RNGestureHandlerButton mock behavior after migrating to
RectButton.app/containers/Button/index.tsx (1)
3-3:RectButtonprop-surface migration looks clean.Good move omitting
enabledfrom consumer props and mapping externaldisabledto internal enablement logic.Also applies to: 9-9, 20-20
Comment on lines
+30
to
+40
| @Override | ||
| public void enable(String scope) { | ||
| sEnabled = true; | ||
| sScope = scope; | ||
| } | ||
|
|
||
| @Override | ||
| public void disable() { | ||
| sEnabled = false; | ||
| sScope = null; | ||
| } |
Contributor
There was a problem hiding this comment.
Non-atomic compound write may produce inconsistent state under concurrent access.
The enable() method writes two volatile fields (sEnabled then sScope) without synchronization. If MainActivity.dispatchKeyEvent or getState() reads between these two writes, it could observe enabled=true with scope=null, or vice versa during disable().
For this use case (screen lifecycle toggles), the practical risk is low, but consider using a single atomic reference or synchronization if correctness matters:
🔒 Proposed fix using synchronized block
- private static volatile boolean sEnabled = false;
- `@Nullable`
- private static volatile String sScope = null;
+ private static final Object sLock = new Object();
+ private static boolean sEnabled = false;
+ `@Nullable`
+ private static String sScope = null;
+ public static boolean isEnabled() {
+ synchronized (sLock) {
+ return sEnabled;
+ }
+ }
+ `@Nullable`
+ public static String getScope() {
+ synchronized (sLock) {
+ return sScope;
+ }
+ }
`@Override`
public void enable(String scope) {
+ synchronized (sLock) {
sEnabled = true;
sScope = scope;
+ }
}
`@Override`
public void disable() {
+ synchronized (sLock) {
sEnabled = false;
sScope = null;
+ }
}
`@Override`
public void getState(Promise promise) {
+ synchronized (sLock) {
WritableMap state = com.facebook.react.bridge.Arguments.createMap();
state.putBoolean("enabled", sEnabled);
state.putString("scope", sScope);
promise.resolve(state);
+ }
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@android/app/src/main/java/chat/rocket/reactnative/a11y/KeyboardA11yModule.java`
around lines 30 - 40, The enable()/disable() methods perform two separate
volatile writes to sEnabled and sScope which can be observed in an inconsistent
intermediate state by readers such as MainActivity.dispatchKeyEvent or getState;
wrap the compound write in a single atomic operation by synchronizing or using
an atomic holder: either place both assignments inside a synchronized block
(e.g., synchronized on KeyboardA11yModule.class) in enable(String) and
disable(), or replace sEnabled/sScope with a single AtomicReference or immutable
state object (e.g., KeyboardA11yState) updated atomically; update readers
(getState, dispatchKeyEvent) to read the single synchronized/atomic state to
ensure visibility and consistency.
Comment on lines
+47
to
+82
| override fun dispatchKeyEvent(event: KeyEvent): Boolean { | ||
| if (KeyboardA11yModule.isEnabled()) { | ||
| val current: View? = currentFocus | ||
| if (current != null && FocusUtils.hasInvertedParent(current)) { | ||
| if (event.action == KeyEvent.ACTION_DOWN) { | ||
| val keyCode = event.keyCode | ||
| val isShiftPressed = event.isShiftPressed | ||
| val mapped = when (keyCode) { | ||
| // Invert DPAD vertical arrows for inverted lists | ||
| KeyEvent.KEYCODE_DPAD_DOWN -> KeyEvent.KEYCODE_DPAD_UP | ||
| KeyEvent.KEYCODE_DPAD_UP -> KeyEvent.KEYCODE_DPAD_DOWN | ||
| // Map Tab / Shift+Tab to vertical navigation as well | ||
| KeyEvent.KEYCODE_TAB -> | ||
| if (isShiftPressed) KeyEvent.KEYCODE_DPAD_UP else KeyEvent.KEYCODE_DPAD_DOWN | ||
| else -> keyCode | ||
| } | ||
| if (mapped != keyCode) { | ||
| val invertedEvent = KeyEvent( | ||
| event.downTime, | ||
| event.eventTime, | ||
| event.action, | ||
| mapped, | ||
| event.repeatCount, | ||
| event.metaState, | ||
| event.deviceId, | ||
| event.scanCode, | ||
| event.flags, | ||
| event.source | ||
| ) | ||
| return super.dispatchKeyEvent(invertedEvent) | ||
| } | ||
| } | ||
| } | ||
| } | ||
| return super.dispatchKeyEvent(event) | ||
| } |
Contributor
There was a problem hiding this comment.
Potential issue: ACTION_UP events are not remapped.
The remapping logic only handles ACTION_DOWN events (Line 51). When a key is pressed, Android generates both ACTION_DOWN and ACTION_UP events. By only remapping ACTION_DOWN, the system receives mismatched key codes:
ACTION_DOWNwithKEYCODE_DPAD_UP(remapped)ACTION_UPwithKEYCODE_DPAD_DOWN(original)
This mismatch could cause unpredictable behavior in some scenarios, such as keys appearing "stuck" or focus behavior not completing properly.
🔧 Proposed fix to also remap ACTION_UP
override fun dispatchKeyEvent(event: KeyEvent): Boolean {
if (KeyboardA11yModule.isEnabled()) {
val current: View? = currentFocus
if (current != null && FocusUtils.hasInvertedParent(current)) {
- if (event.action == KeyEvent.ACTION_DOWN) {
+ if (event.action == KeyEvent.ACTION_DOWN || event.action == KeyEvent.ACTION_UP) {
val keyCode = event.keyCode
val isShiftPressed = event.isShiftPressed
val mapped = when (keyCode) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@android/app/src/main/java/chat/rocket/reactnative/MainActivity.kt` around
lines 47 - 82, The current dispatchKeyEvent remaps only ACTION_DOWN events
causing mismatched ACTION_UP keycodes; update the condition that checks the
event action in dispatchKeyEvent (used with KeyboardA11yModule.isEnabled() and
FocusUtils.hasInvertedParent(current)) to also handle KeyEvent.ACTION_UP (e.g.,
if event.action == KeyEvent.ACTION_DOWN || event.action == KeyEvent.ACTION_UP),
perform the same keyCode mapping logic for ACTION_UP, and construct/dispatch the
inverted KeyEvent (preserving downTime, eventTime, repeatCount, metaState,
deviceId, scanCode, flags, source) so both press and release events are
consistently remapped.
| return false; | ||
| } | ||
|
|
||
| ViewGroup contentView = (ViewGroup) getChildAt(0); |
Contributor
There was a problem hiding this comment.
Missing type check before cast.
getChildAt(0) returns View, not necessarily ViewGroup. If the content view hierarchy changes, this will throw ClassCastException. Add a defensive check:
🛡️ Proposed defensive check
- ViewGroup contentView = (ViewGroup) getChildAt(0);
+ View child = getChildAt(0);
+ if (!(child instanceof ViewGroup)) {
+ return false;
+ }
+ ViewGroup contentView = (ViewGroup) child;📝 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.
Suggested change
| ViewGroup contentView = (ViewGroup) getChildAt(0); | |
| View child = getChildAt(0); | |
| if (!(child instanceof ViewGroup)) { | |
| return false; | |
| } | |
| ViewGroup contentView = (ViewGroup) child; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@android/app/src/main/java/chat/rocket/reactnative/scroll/InvertedScrollView.java`
at line 69, In InvertedScrollView.java the code casts getChildAt(0) directly to
ViewGroup into variable contentView which can throw ClassCastException if the
child is not a ViewGroup; change this to first retrieve the child into a View,
check "instanceof ViewGroup" (and for null), then assign to contentView only
when the check passes, and handle the else case safely (e.g., return early, fall
back to a no-op, or log an error) so the view hierarchy change won't crash the
method.
Comment on lines
+85
to
+92
| int exitDir = isForward ? View.FOCUS_DOWN : View.FOCUS_UP; | ||
| View exitTarget = findExitTarget(exitDir); | ||
| if (exitTarget != null) { | ||
| exitTarget.requestFocus(); | ||
| return true; | ||
| } | ||
|
|
||
| return true; |
Contributor
There was a problem hiding this comment.
Key consumed at boundary without visual feedback.
When at the list boundary and no exit target is found, returning true consumes the key event without moving focus anywhere. This could trap keyboard users at the list edge without indication.
Consider either returning false to allow default handling, or providing feedback (e.g., scroll bounce, accessibility announcement):
💡 Alternative: allow default handling at boundary
View exitTarget = findExitTarget(exitDir);
if (exitTarget != null) {
exitTarget.requestFocus();
return true;
}
- return true;
+ return false; // Let system handle boundary case
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@android/app/src/main/java/chat/rocket/reactnative/scroll/InvertedScrollView.java`
around lines 85 - 92, The current focus-handling in InvertedScrollView (around
variables exitDir and exitTarget) always returns true even when
findExitTarget(exitDir) returns null, which consumes the key at the list
boundary and can trap keyboard users; update the method so that when exitTarget
== null it does not consume the event—either return false to allow default focus
handling or trigger accessible feedback (e.g., call
announceForAccessibility(...) or an EdgeEffect bounce) before returning true;
locate the block using exitDir/exitTarget in InvertedScrollView and replace the
final unconditional "return true" with logic that returns false when exitTarget
is null (or invokes an accessibility feedback helper then returns true) so users
receive proper feedback or focus moves.
Comment on lines
+91
to
101
| <RectButton | ||
| onPress={onPress} | ||
| disabled={isDisabled} | ||
| enabled={!isDisabled} | ||
| activeOpacity={0.2} | ||
| underlayColor='transparent' | ||
| // No Android ink ripple; matches Pressable with `android_ripple` disabled. | ||
| rippleColor='transparent' | ||
| style={containerStyle} | ||
| accessibilityLabel={title} | ||
| accessibilityRole='button' | ||
| {...otherProps}> |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
For react-native-gesture-handler v2.24.0, does RectButton automatically expose disabled accessibility semantics when enabled={false}on Android/iOS, or isaccessibilityState={{ disabled: true }} still required for TalkBack/VoiceOver announcement?
💡 Result:
No, RectButton does not automatically expose disabled accessibility semantics when enabled={false}. The accessibilityState={{ disabled: true }} prop is still required for proper TalkBack (Android) and VoiceOver (iOS) announcements of the disabled state.
Citations:
- 1: https://docs.swmansion.com/react-native-gesture-handler/docs/components/buttons/
- 2: https://tessl.io/registry/tessl/npm-react-native-gesture-handler/2.28.0/files/docs/buttons.md
- 3: https://docs.swmansion.com/react-native-gesture-handler/docs/1.x/api/components/buttons
- 4: Buttons not accessible on Android software-mansion/react-native-gesture-handler#1251
- 5: Pressable cannot be accessible by itself software-mansion/react-native-gesture-handler#3428
- 6: https://docs.swmansion.com/react-native-gesture-handler/docs/2.x/components/pressable/
Add explicit accessibility state to indicate disabled buttons on RectButton.
When using RectButton with enabled={!isDisabled}, the disabled state is not automatically announced to screen readers (TalkBack on Android, VoiceOver on iOS). The accessibilityState={{ disabled: !!isDisabled }} prop must be set explicitly for proper accessibility announcements.
Suggested patch
<RectButton
onPress={onPress}
enabled={!isDisabled}
+ accessibilityState={{ disabled: !!isDisabled }}
activeOpacity={0.2}
underlayColor='transparent'📝 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.
Suggested change
| <RectButton | |
| onPress={onPress} | |
| disabled={isDisabled} | |
| enabled={!isDisabled} | |
| activeOpacity={0.2} | |
| underlayColor='transparent' | |
| // No Android ink ripple; matches Pressable with `android_ripple` disabled. | |
| rippleColor='transparent' | |
| style={containerStyle} | |
| accessibilityLabel={title} | |
| accessibilityRole='button' | |
| {...otherProps}> | |
| <RectButton | |
| onPress={onPress} | |
| enabled={!isDisabled} | |
| accessibilityState={{ disabled: !!isDisabled }} | |
| activeOpacity={0.2} | |
| underlayColor='transparent' | |
| // No Android ink ripple; matches Pressable with `android_ripple` disabled. | |
| rippleColor='transparent' | |
| style={containerStyle} | |
| accessibilityLabel={title} | |
| accessibilityRole='button' | |
| {...otherProps}> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/containers/Button/index.tsx` around lines 91 - 101, Add an explicit
accessibilityState to the RectButton so screen readers announce when the button
is disabled: update the RectButton (in this file's Button component) to include
accessibilityState={{ disabled: !!isDisabled }} alongside existing props (e.g.,
onPress, enabled={!isDisabled}, accessibilityLabel={title}) so the disabled
state is conveyed to assistive tech.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
app/containers/MessageComposer/interfaces.ts (1)
5-10:⚠️ Potential issue | 🔴 CriticalAdd
focus()toMessageComposer'suseImperativeHandle.The interface now requires
focus()onIMessageComposerRef, butMessageComposer.tsxlines 55-59 do not expose it inuseImperativeHandle. The currently exposed methods are onlycloseEmojiKeyboardAndAction,getText, andsetInput. This will cause runtime failures when external code callscomposerRef.current.focus().Current implementation
useImperativeHandle(forwardedRef, () => ({ closeEmojiKeyboardAndAction, getText: composerInputComponentRef.current?.getText, setInput: composerInputComponentRef.current?.setInput }));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/containers/MessageComposer/interfaces.ts` around lines 5 - 10, The IMessageComposerRef now includes focus(), but MessageComposer.tsx's useImperativeHandle currently only exposes closeEmojiKeyboardAndAction, getText and setInput; update the useImperativeHandle(forwardedRef, ...) return object to also expose focus by wiring it to composerInputComponentRef.current?.focus (or the component's focus method), ensuring the forwardedRef provides a focus() implementation that delegates to the inner composer input's focus method and matches the IMessageComposerRef signature.app/views/RoomView/index.tsx (1)
222-266:⚠️ Potential issue | 🟠 MajorRead
focusHeaderOnOpenfrom live props, not the mount-timeroute.
routeis captured once incomponentDidMount, so this listener keeps reading the original params. After Line 262 clearsfocusHeaderOnOpen, the callback can still see the oldtrueand refocus the header every time the room regains focus.Suggested fix
- const { navigation, dispatch, isMasterDetail, route } = this.props; + const { navigation, dispatch, isMasterDetail } = this.props; ... this.unsubscribeFocus = navigation.addListener('focus', () => { InteractionManager.runAfterInteractions(() => { - if (isMasterDetail && route?.params?.focusHeaderOnOpen) { + if (isMasterDetail && this.props.route?.params?.focusHeaderOnOpen) { this.roomHeaderRef.current?.focus(); navigation.setParams({ focusHeaderOnOpen: undefined }); return; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/views/RoomView/index.tsx` around lines 222 - 266, The focus listener currently closes over the mount-time route/navigation variables so it keeps reading the original route.params.focusHeaderOnOpen; update the navigation.addListener('focus', ...) callback (the code that references isMasterDetail, route?.params?.focusHeaderOnOpen, roomHeaderRef, messageComposerRef, and navigation.setParams) to read the live props instead (e.g. const focusHeaderOnOpen = this.props.route?.params?.focusHeaderOnOpen and use this.props.navigation.setParams) or use this.props.navigation.getParam, and similarly use this.props.navigation and this.props.route inside the callback so clearing focusHeaderOnOpen actually takes effect on subsequent focuses.
♻️ Duplicate comments (3)
android/app/src/main/java/chat/rocket/reactnative/MainActivity.kt (1)
51-76:⚠️ Potential issue | 🟡 MinorRemap the matching
ACTION_UPevent too.Android sends paired down/up events. Right now the press is remapped but the release keeps the original keycode, so the framework sees mixed pairs for the same navigation step.
Suggested fix
- if (event.action == KeyEvent.ACTION_DOWN) { + if (event.action == KeyEvent.ACTION_DOWN || event.action == KeyEvent.ACTION_UP) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@android/app/src/main/java/chat/rocket/reactnative/MainActivity.kt` around lines 51 - 76, The current dispatchKeyEvent remaps only ACTION_DOWN events (uses keyCode -> mapped and builds invertedEvent) but leaves ACTION_UP unmapped causing mismatched pairs; update dispatchKeyEvent to also detect KeyEvent.ACTION_UP, compute the same mapped key (reuse the mapped logic for keyCode), construct an invertedEvent for ACTION_UP preserving event.downTime, event.eventTime, repeatCount, metaState, deviceId, scanCode, flags, source, and call return super.dispatchKeyEvent(invertedEvent) for the UP case as well so both press and release use the same mapped key; keep existing behavior for other actions.android/app/src/main/java/chat/rocket/reactnative/scroll/InvertedScrollView.java (2)
69-69:⚠️ Potential issue | 🟡 MinorMissing type check before cast remains unaddressed.
The direct cast
(ViewGroup) getChildAt(0)can throwClassCastExceptionif the first child isn't aViewGroup. Add a defensive check as previously suggested.🛡️ Proposed defensive check
- ViewGroup contentView = (ViewGroup) getChildAt(0); + View child = getChildAt(0); + if (!(child instanceof ViewGroup)) { + return false; + } + ViewGroup contentView = (ViewGroup) child;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@android/app/src/main/java/chat/rocket/reactnative/scroll/InvertedScrollView.java` at line 69, In InvertedScrollView, avoid the unsafe cast to ViewGroup by checking the child first: call getChildAt(0) into a local (e.g., child), verify child != null and child instanceof ViewGroup before casting to contentView, and handle the else case (e.g., skip processing, return, or log a warning) so a ClassCastException cannot occur when the first child is not a ViewGroup.
85-92:⚠️ Potential issue | 🟡 MinorBoundary key consumption issue remains unaddressed.
When at the list boundary with no exit target found, returning
true(line 92) consumes the key event without moving focus anywhere. This traps keyboard users. Consider returningfalseto allow default system handling.💡 Proposed fix
View exitTarget = findExitTarget(exitDir); if (exitTarget != null) { exitTarget.requestFocus(); return true; } - return true; + return false; // Let system handle boundary case }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@android/app/src/main/java/chat/rocket/reactnative/scroll/InvertedScrollView.java` around lines 85 - 92, The key handler in InvertedScrollView currently returns true even when findExitTarget(exitDir) returns null, which consumes boundary key events and traps keyboard users; update the logic in the method handling directional focus (using exitDir and exitTarget) so that if exitTarget is null you return false (allowing default system handling) instead of true, while keeping the existing behavior of requesting focus and returning true when exitTarget != null.
🧹 Nitpick comments (2)
app/views/RoomView/List/components/RNLikeInvertedScrollView.tsx (1)
36-48: Heavy reliance onanytypes reduces type safety.Multiple refs and handlers use
any, which bypasses TypeScript's type checking. While this is common with native components, consider using more specific types where possible:
scrollRefcould be typed asReact.RefObject<typeof NativeInvertedScrollView>if the native component exposes typingsStateinterface is well-defined, which is goodThis is acceptable for now given native component limitations, but worth revisiting if the native layer adds TypeScript definitions.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/views/RoomView/List/components/RNLikeInvertedScrollView.tsx` around lines 36 - 48, The class RNLikeInvertedScrollView uses broad any types for scrollRef and keyboard subscription fields; replace them with more specific types (e.g., change scrollRef: React.createRef<any>() to React.createRef<NativeInvertedScrollView | null>() or React.RefObject<ScrollView | null> depending on the native component type you can import), and type the keyboard subscription fields as { remove(): void } | undefined instead of any; update the declarations for _subscriptionKeyboardDidShow, _subscriptionKeyboardDidHide, and _keyboardMetrics (e.g., { height: number } | null already ok) and adjust any handler params/signatures that currently use any to use the appropriate event/interface from React Native so TypeScript can check usages in RNLikeInvertedScrollView.app/lib/native/KeyboardInversionA11yAndroid.ts (1)
15-15: Consider adding a platform check for future safety, though currently unused.This module has no call sites in the codebase currently. However, since
TurboModuleRegistry.getEnforcingwill throw on iOS, consider adding a platform check at module load time to prevent crashes if this is imported from iOS code in the future:+import { Platform } from 'react-native'; + +if (Platform.OS !== 'android') { + throw new Error('KeyboardInversionA11yAndroid is only available on Android'); +} + const NativeModule = TurboModuleRegistry.getEnforcing<Spec>('KeyboardA11y');Alternatively, document the Android-only requirement in JSDoc.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/lib/native/KeyboardInversionA11yAndroid.ts` at line 15, The file currently calls TurboModuleRegistry.getEnforcing<Spec>('KeyboardA11y') at module load (const NativeModule = TurboModuleRegistry.getEnforcing<Spec>('KeyboardA11y')) which will throw on non-Android platforms; wrap this call in a Platform.OS === 'android' guard so the TurboModule is only resolved on Android and export a safe null/undefined fallback (or no-op shim) for other platforms, and add a short JSDoc note on the KeyboardInversionA11yAndroid export indicating it is Android-only; alternatively, if you prefer not to add runtime branching, add the Android-only JSDoc comment above the module to document the requirement.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@android/app/src/main/java/chat/rocket/reactnative/scroll/InvertedScrollContentView.java`:
- Around line 40-52: The onRequestFocusInDescendants method currently always
iterates children from last-to-first; when mIsInvertedContent is true you must
respect the direction parameter: if direction == View.FOCUS_UP iterate forward
from index 0 to getChildCount()-1 so the bottom-most child (index 0 in inverted
content) is tried first, otherwise iterate backward from getChildCount()-1 to 0;
update the loop in onRequestFocusInDescendants to choose the iteration order
based on the direction before calling getChildAt(...) and
child.requestFocus(...).
---
Outside diff comments:
In `@app/containers/MessageComposer/interfaces.ts`:
- Around line 5-10: The IMessageComposerRef now includes focus(), but
MessageComposer.tsx's useImperativeHandle currently only exposes
closeEmojiKeyboardAndAction, getText and setInput; update the
useImperativeHandle(forwardedRef, ...) return object to also expose focus by
wiring it to composerInputComponentRef.current?.focus (or the component's focus
method), ensuring the forwardedRef provides a focus() implementation that
delegates to the inner composer input's focus method and matches the
IMessageComposerRef signature.
In `@app/views/RoomView/index.tsx`:
- Around line 222-266: The focus listener currently closes over the mount-time
route/navigation variables so it keeps reading the original
route.params.focusHeaderOnOpen; update the navigation.addListener('focus', ...)
callback (the code that references isMasterDetail,
route?.params?.focusHeaderOnOpen, roomHeaderRef, messageComposerRef, and
navigation.setParams) to read the live props instead (e.g. const
focusHeaderOnOpen = this.props.route?.params?.focusHeaderOnOpen and use
this.props.navigation.setParams) or use this.props.navigation.getParam, and
similarly use this.props.navigation and this.props.route inside the callback so
clearing focusHeaderOnOpen actually takes effect on subsequent focuses.
---
Duplicate comments:
In `@android/app/src/main/java/chat/rocket/reactnative/MainActivity.kt`:
- Around line 51-76: The current dispatchKeyEvent remaps only ACTION_DOWN events
(uses keyCode -> mapped and builds invertedEvent) but leaves ACTION_UP unmapped
causing mismatched pairs; update dispatchKeyEvent to also detect
KeyEvent.ACTION_UP, compute the same mapped key (reuse the mapped logic for
keyCode), construct an invertedEvent for ACTION_UP preserving event.downTime,
event.eventTime, repeatCount, metaState, deviceId, scanCode, flags, source, and
call return super.dispatchKeyEvent(invertedEvent) for the UP case as well so
both press and release use the same mapped key; keep existing behavior for other
actions.
In
`@android/app/src/main/java/chat/rocket/reactnative/scroll/InvertedScrollView.java`:
- Line 69: In InvertedScrollView, avoid the unsafe cast to ViewGroup by checking
the child first: call getChildAt(0) into a local (e.g., child), verify child !=
null and child instanceof ViewGroup before casting to contentView, and handle
the else case (e.g., skip processing, return, or log a warning) so a
ClassCastException cannot occur when the first child is not a ViewGroup.
- Around line 85-92: The key handler in InvertedScrollView currently returns
true even when findExitTarget(exitDir) returns null, which consumes boundary key
events and traps keyboard users; update the logic in the method handling
directional focus (using exitDir and exitTarget) so that if exitTarget is null
you return false (allowing default system handling) instead of true, while
keeping the existing behavior of requesting focus and returning true when
exitTarget != null.
---
Nitpick comments:
In `@app/lib/native/KeyboardInversionA11yAndroid.ts`:
- Line 15: The file currently calls
TurboModuleRegistry.getEnforcing<Spec>('KeyboardA11y') at module load (const
NativeModule = TurboModuleRegistry.getEnforcing<Spec>('KeyboardA11y')) which
will throw on non-Android platforms; wrap this call in a Platform.OS ===
'android' guard so the TurboModule is only resolved on Android and export a safe
null/undefined fallback (or no-op shim) for other platforms, and add a short
JSDoc note on the KeyboardInversionA11yAndroid export indicating it is
Android-only; alternatively, if you prefer not to add runtime branching, add the
Android-only JSDoc comment above the module to document the requirement.
In `@app/views/RoomView/List/components/RNLikeInvertedScrollView.tsx`:
- Around line 36-48: The class RNLikeInvertedScrollView uses broad any types for
scrollRef and keyboard subscription fields; replace them with more specific
types (e.g., change scrollRef: React.createRef<any>() to
React.createRef<NativeInvertedScrollView | null>() or React.RefObject<ScrollView
| null> depending on the native component type you can import), and type the
keyboard subscription fields as { remove(): void } | undefined instead of any;
update the declarations for _subscriptionKeyboardDidShow,
_subscriptionKeyboardDidHide, and _keyboardMetrics (e.g., { height: number } |
null already ok) and adjust any handler params/signatures that currently use any
to use the appropriate event/interface from React Native so TypeScript can check
usages in RNLikeInvertedScrollView.
🪄 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: 1c5841f3-c217-42c5-8fd1-fcfe793941e1
⛔ Files ignored due to path filters (6)
app/containers/Button/__snapshots__/Button.test.tsx.snapis excluded by!**/*.snapapp/containers/LoginServices/__snapshots__/LoginServices.test.tsx.snapis excluded by!**/*.snapapp/containers/UIKit/__snapshots__/UiKitMessage.test.tsx.snapis excluded by!**/*.snapapp/containers/UIKit/__snapshots__/UiKitModal.test.tsx.snapis excluded by!**/*.snapapp/containers/message/__snapshots__/Message.test.tsx.snapis excluded by!**/*.snapapp/views/CannedResponsesListView/__snapshots__/CannedResponseItem.test.tsx.snapis excluded by!**/*.snap
📒 Files selected for processing (22)
android/app/src/main/java/chat/rocket/reactnative/MainActivity.ktandroid/app/src/main/java/chat/rocket/reactnative/a11y/KeyboardA11yModule.javaandroid/app/src/main/java/chat/rocket/reactnative/a11y/KeyboardA11ySpec.javaandroid/app/src/main/java/chat/rocket/reactnative/scroll/FocusUtils.javaandroid/app/src/main/java/chat/rocket/reactnative/scroll/InvertedScrollContentView.javaandroid/app/src/main/java/chat/rocket/reactnative/scroll/InvertedScrollContentViewManager.javaandroid/app/src/main/java/chat/rocket/reactnative/scroll/InvertedScrollView.javaandroid/app/src/main/java/chat/rocket/reactnative/scroll/InvertedScrollViewManager.javaandroid/app/src/main/res/values/ids.xmlapp/containers/Button/Button.test.tsxapp/containers/Button/index.tsxapp/containers/MessageComposer/interfaces.tsapp/containers/RoomHeader/RoomHeader.tsxapp/containers/RoomHeader/index.tsxapp/lib/methods/helpers/goRoom.tsapp/lib/native/KeyboardInversionA11yAndroid.tsapp/stacks/types.tsapp/views/RoomView/List/components/InvertedScrollView.tsxapp/views/RoomView/List/components/List.tsxapp/views/RoomView/List/components/RNLikeInvertedScrollView.tsxapp/views/RoomView/index.tsxapp/views/RoomsListView/index.tsx
📜 Review details
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-12-17T15:56:22.578Z
Learnt from: OtavioStasiak
Repo: RocketChat/Rocket.Chat.ReactNative PR: 6499
File: app/containers/ServerItem/index.tsx:34-36
Timestamp: 2025-12-17T15:56:22.578Z
Learning: In the Rocket.Chat React Native codebase, for radio button components on iOS, include the selection state ("Selected"/"Unselected") in the accessibilityLabel instead of using accessibilityState={{ checked: hasCheck }}, because iOS VoiceOver has known issues with accessibilityRole="radio" + accessibilityState that prevent correct state announcement.
Applied to files:
android/app/src/main/java/chat/rocket/reactnative/a11y/KeyboardA11yModule.javaapp/containers/Button/index.tsxandroid/app/src/main/java/chat/rocket/reactnative/scroll/InvertedScrollContentView.javaapp/containers/RoomHeader/RoomHeader.tsx
📚 Learning: 2026-03-10T15:21:45.098Z
Learnt from: Rohit3523
Repo: RocketChat/Rocket.Chat.ReactNative PR: 7046
File: app/containers/InAppNotification/NotifierComponent.stories.tsx:46-75
Timestamp: 2026-03-10T15:21:45.098Z
Learning: In `app/containers/InAppNotification/NotifierComponent.tsx` (React Native, Rocket.Chat), `NotifierComponent` is exported as a Redux-connected component via `connect(mapStateToProps)`. The `isMasterDetail` prop is automatically injected from `state.app.isMasterDetail` and does not need to be passed explicitly at call sites or in Storybook stories that use the default (connected) export.
Applied to files:
app/containers/RoomHeader/index.tsxapp/views/RoomView/index.tsx
📚 Learning: 2026-03-04T20:13:17.288Z
Learnt from: divyanshu-patil
Repo: RocketChat/Rocket.Chat.ReactNative PR: 6957
File: ios/RocketChat Watch App/Views/MessageComposerView.swift:37-55
Timestamp: 2026-03-04T20:13:17.288Z
Learning: In the WatchOS app (ios/RocketChat Watch App) for Rocket.Chat React Native, using SwiftUI `Button` inside a `ScrollView` on WatchOS causes accidental message sends because button tap targets can be triggered during scroll gestures. `Text` with `.onTapGesture` is the preferred pattern for tappable items in scroll views on WatchOS. To preserve accessibility, add `.accessibilityAddTraits(.isButton)` and `.accessibilityLabel()` to the `Text` element instead.
Applied to files:
app/containers/Button/index.tsxandroid/app/src/main/java/chat/rocket/reactnative/scroll/InvertedScrollContentView.javaapp/containers/RoomHeader/RoomHeader.tsx
📚 Learning: 2026-03-17T19:15:30.463Z
Learnt from: Rohit3523
Repo: RocketChat/Rocket.Chat.ReactNative PR: 6970
File: .maestro/tests/room/share-message.yaml:77-79
Timestamp: 2026-03-17T19:15:30.463Z
Learning: In `.maestro/tests/room/share-message.yaml` (Rocket.Chat React Native), the `tapOn: point: 5%,10%` step is intentional: it taps the empty area above the bottom sheet and keyboard to dismiss both simultaneously. Using `action-sheet-handle` instead would only close the sheet but not the keyboard. This pattern is acceptable when both need to be dismissed together in a single step.
Applied to files:
android/app/src/main/java/chat/rocket/reactnative/MainActivity.kt
🔇 Additional comments (17)
app/containers/Button/index.tsx (2)
91-101: MissingaccessibilityStatefor disabled button announcement.The
RectButtondoes not automatically expose disabled accessibility semantics whenenabled={false}. Screen readers (TalkBack on Android, VoiceOver on iOS) require explicitaccessibilityState={{ disabled: !!isDisabled }}to announce the disabled state.Suggested fix
<RectButton onPress={onPress} enabled={!isDisabled} + accessibilityState={{ disabled: !!isDisabled }} activeOpacity={0.2} underlayColor='transparent'
3-3: LGTM on the interface changes.The interface properly extends
RectButtonPropswhile omittingchildrenandenabledto avoid conflicts. Adding the explicitdisabled?: booleanprop maintains a clean API for consumers.Also applies to: 9-21
app/containers/Button/Button.test.tsx (1)
64-69: LGTM!The updated comment accurately documents the testing limitation with
RNGestureHandlerButtonin Jest. The assertion correctly validates theenabledprop for the newRectButtonimplementation.android/app/src/main/res/values/ids.xml (1)
1-5: LGTM!The resource ID declaration is correctly formatted and follows Android naming conventions. This ID will serve as a tag marker for inverted list detection in the focus navigation logic.
app/containers/RoomHeader/RoomHeader.tsx (1)
86-88: LGTM!The imperative focus API is correctly implemented using the standard React Native pattern with
findNodeHandleandAccessibilityInfo.setAccessibilityFocus. The null check onnodeHandleproperly guards against the case where the view hasn't mounted yet.Also applies to: 138-174
app/containers/RoomHeader/index.tsx (1)
27-107: LGTM!The ref forwarding is correctly implemented with
React.memo(React.forwardRef(...))pattern. TheIRoomHeaderReftype re-export enables proper typing at consumer sites.app/views/RoomView/List/components/List.tsx (1)
47-49: LGTM!The
exitFocusNativeIdprop is correctly conditionally passed only for non-iOS platforms, aligning with the Android-specific keyboard navigation fix. The hardcoded string'message-composer-exit-focus'should match thenativeIDset on the message composer wrapper inRoomView.app/views/RoomsListView/index.tsx (1)
74-77: LGTM!Adding
focusHeaderOnOpen: trueensures that when users navigate to a room from the rooms list (especially via keyboard), the accessibility focus moves to the room header. This is the correct behavior for keyboard navigation flow.app/stacks/types.ts (1)
44-44: LGTM!The optional
focusHeaderOnOpenparameter is correctly typed and positioned within theRoomViewroute params.android/app/src/main/java/chat/rocket/reactnative/scroll/InvertedScrollViewManager.java (1)
28-32: LGTM!The
@ReactPropregistration follows standard React Native view manager patterns. The nullable annotation correctly handles cases where the prop is not set from JavaScript.app/views/RoomView/List/components/InvertedScrollView.tsx (1)
1-10: Clean re-export pattern for the new inverted scroll implementation.The type cast approach correctly exposes
InvertedScrollViewPropswhile hiding the internalscrollViewRefprop that's managed by theforwardRefwrapper. TheexitFocusNativeIdprop is properly typed and flows through to the native layer.app/views/RoomView/List/components/RNLikeInvertedScrollView.tsx (3)
50-58: LGTM!Keyboard listener lifecycle is properly managed with setup in
componentDidMountand teardown incomponentWillUnmountusing optional chaining for safety.
68-75: Animation detection logic is well-designed.The
isAnimatingcheck using momentum scroll timing with a 16ms threshold (one frame at 60fps) effectively detects ongoing scroll animations. This prevents touch events from being stolen during smooth scrolling.
172-203: Render composition is correct.Destructuring
onContentSizeChangefromrestprevents passing it to the native scroll view (which doesn't support it), whilehandleContentOnLayouton the content view correctly emulates this behavior. The style composition and prop forwarding are well-structured.android/app/src/main/java/chat/rocket/reactnative/scroll/InvertedScrollView.java (3)
95-106: LGTM!
findContainingCellIndexcorrectly traverses the view hierarchy to locate the containing cell, with proper handling for the edge case where a parent isn't aView.
108-136: Well-structured exit target resolution.The two-phase approach (nativeID lookup then FocusFinder fallback) provides flexibility while maintaining system-like focus behavior. The
isDescendantOfcheck correctly prevents focus from cycling back into the scroll view.
35-57: Key event dispatch is correctly structured.The
mKeyConsumedflag pattern correctly handles the ACTION_DOWN/ACTION_UP pairing, processing navigation on ACTION_DOWN and returning the cached result for ACTION_UP. Direction mapping is correct for both DPAD and TAB keys.
Comment on lines
+40
to
+52
| protected boolean onRequestFocusInDescendants(int direction, Rect previouslyFocusedRect) { | ||
| if (mIsInvertedContent) { | ||
| for (int i = getChildCount() - 1; i >= 0; i--) { | ||
| View child = getChildAt(i); | ||
| if (child.getVisibility() == VISIBLE) { | ||
| if (child.requestFocus(direction, previouslyFocusedRect)) { | ||
| return true; | ||
| } | ||
| } | ||
| } | ||
| return false; | ||
| } | ||
| return super.onRequestFocusInDescendants(direction, previouslyFocusedRect); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find and read the InvertedScrollContentView file
find . -name "InvertedScrollContentView.java" -type fRepository: RocketChat/Rocket.Chat.ReactNative
Length of output: 167
🏁 Script executed:
# Check the app/views/RoomView/index.tsx file around lines 1574-1577
find . -path "*/app/views/RoomView/index.tsx" -type fRepository: RocketChat/Rocket.Chat.ReactNative
Length of output: 108
🏁 Script executed:
# Search for the onRequestFocusInDescendants implementation
rg "onRequestFocusInDescendants" -A 15 --type javaRepository: RocketChat/Rocket.Chat.ReactNative
Length of output: 3573
🏁 Script executed:
# Read the RoomView/index.tsx file around lines 1574-1577
sed -n '1570,1580p' ./app/views/RoomView/index.tsxRepository: RocketChat/Rocket.Chat.ReactNative
Length of output: 238
🏁 Script executed:
# Get more context around the footer/composer addition
sed -n '1550,1600p' ./app/views/RoomView/index.tsxRepository: RocketChat/Rocket.Chat.ReactNative
Length of output: 1426
🏁 Script executed:
# Read the full InvertedScrollContentView.java file to understand its structure
wc -l ./android/app/src/main/java/chat/rocket/reactnative/scroll/InvertedScrollContentView.javaRepository: RocketChat/Rocket.Chat.ReactNative
Length of output: 170
🏁 Script executed:
# Read the complete InvertedScrollContentView.java file
cat -n ./android/app/src/main/java/chat/rocket/reactnative/scroll/InvertedScrollContentView.javaRepository: RocketChat/Rocket.Chat.ReactNative
Length of output: 3070
🏁 Script executed:
# Research Android View focus direction constants
rg "FOCUS_UP|FOCUS_DOWN|FOCUS_FORWARD|FOCUS_BACKWARD" android/app/src/main/java --type java -iRepository: RocketChat/Rocket.Chat.ReactNative
Length of output: 366
🏁 Script executed:
# Check if there are other focus-related implementations in the codebase that handle direction
rg "onRequestFocusInDescendants|addFocusables" android/app/src/main/java --type java -B 2 -A 10Repository: RocketChat/Rocket.Chat.ReactNative
Length of output: 3762
🏁 Script executed:
# Search for any documentation or comments about focus direction handling
rg "FOCUS_UP|FOCUS_DOWN|direction" android/app/src/main/java/chat/rocket/reactnative/scroll/ --type javaRepository: RocketChat/Rocket.Chat.ReactNative
Length of output: 1466
🏁 Script executed:
# Read the InvertedScrollView.java file to understand how it uses focus direction
cat -n ./android/app/src/main/java/chat/rocket/reactnative/scroll/InvertedScrollView.java | head -100Repository: RocketChat/Rocket.Chat.ReactNative
Length of output: 4045
🏁 Script executed:
# Get more context from InvertedScrollView around focus direction handling
rg "findExitTarget|focusDir|exitDir" android/app/src/main/java/chat/rocket/reactnative/scroll/InvertedScrollView.java -B 3 -A 8Repository: RocketChat/Rocket.Chat.ReactNative
Length of output: 1058
🏁 Script executed:
# Final verification: check how direction constants are used in Android standard behavior
rg "FOCUS_UP|FOCUS_DOWN" android/app/src/main/java/chat/rocket/reactnative/scroll/ --type java -B 1 -A 1Repository: RocketChat/Rocket.Chat.ReactNative
Length of output: 777
Respect the direction parameter to determine iteration order.
When entering from the footer/composer below the list (with FOCUS_UP), the iteration should start from the bottom-most child (index 0), not the top. The current code always iterates last-to-first regardless of direction, causing focus to land on the wrong end of the list. Use the direction parameter to choose whether to iterate forward or backward through children.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@android/app/src/main/java/chat/rocket/reactnative/scroll/InvertedScrollContentView.java`
around lines 40 - 52, The onRequestFocusInDescendants method currently always
iterates children from last-to-first; when mIsInvertedContent is true you must
respect the direction parameter: if direction == View.FOCUS_UP iterate forward
from index 0 to getChildCount()-1 so the bottom-most child (index 0 in inverted
content) is tried first, otherwise iterate backward from getChildCount()-1 to 0;
update the loop in onRequestFocusInDescendants to choose the iteration order
based on the direction before calling getChildAt(...) and
child.requestFocus(...).
|
Android Build Available Rocket.Chat Experimental 4.71.0.108432 Internal App Sharing: https://play.google.com/apps/test/RQVpXLytHNc/ahAO29uNTtIwYdUnrVonpp1RfgQTAEX2ij3rh0R23ZYQhW3GqQEPZOmgSedjT5eSWj_xUeEZiNZq1RyP3wX_5QZ26W |
|
iOS Build Available Rocket.Chat Experimental 4.71.0.108433 |
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
app/containers/Touch.tsx (3)
50-51: Outdated comment referencesRectButton.The comment still mentions "RectButton" but the component now uses
Pressable. Update for accuracy.📝 Suggested fix
- // The background color must be applied to the RectButton, not the View. - // If set on the View, the touch opacity animation won't work properly. + // The background color must be applied to the Pressable, not the View. + // If set on the View, the touch opacity animation won't work properly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/containers/Touch.tsx` around lines 50 - 51, Update the outdated comment in Touch (app/containers/Touch.tsx) to refer to Pressable instead of RectButton: change the sentence that currently reads "The background color must be applied to the RectButton, not the View." to "The background color must be applied to the Pressable, not the View." so the comment accurately reflects the current component usage in the Touch component.
67-69: Outdated comment referencesRectButton.Same issue—update comment to reflect the switch to
Pressable.📝 Suggested fix
// The margin should be applied to the parent component. - // If set on the View, it will create an internal margin inside the RectButton. + // If set on the View, it will create an internal margin inside the Pressable.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/containers/Touch.tsx` around lines 67 - 69, The inline comment above the marginStyles constant still mentions RectButton even though the component now uses Pressable; update that comment in Touch.tsx to reference Pressable (or generically "Pressable/Touchable element") and clearly state that the margin must be applied to the parent to avoid internal spacing inside the Pressable; locate the comment immediately above const marginStyles and replace "RectButton" with "Pressable" and adjust phrasing to match the current implementation.
23-23: Consider renamingrectButtonStyleprop.This prop name references the now-removed
RectButton. Since this is a public interface and other components may depend on it, you could deprecate it with an alias or rename it to something generic likecontainerStylein a follow-up.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/containers/Touch.tsx` at line 23, The public prop rectButtonStyle on the Touch component still references the removed RectButton; add a new generic prop (e.g., containerStyle) to the TouchProps interface and update the Touch component to accept both containerStyle and rectButtonStyle, mapping them to the actual underlying style prop (merge containerStyle and rectButtonStyle, with containerStyle taking precedence) so existing users don’t break; keep rectButtonStyle as a deprecated alias and emit a single console.warn when rectButtonStyle is passed (or both are passed) to guide users to containerStyle, and update any prop-type/TS definitions and the component’s props destructuring/usage to use the merged style for the rendered wrapper element.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@app/containers/Touch.tsx`:
- Around line 50-51: Update the outdated comment in Touch
(app/containers/Touch.tsx) to refer to Pressable instead of RectButton: change
the sentence that currently reads "The background color must be applied to the
RectButton, not the View." to "The background color must be applied to the
Pressable, not the View." so the comment accurately reflects the current
component usage in the Touch component.
- Around line 67-69: The inline comment above the marginStyles constant still
mentions RectButton even though the component now uses Pressable; update that
comment in Touch.tsx to reference Pressable (or generically "Pressable/Touchable
element") and clearly state that the margin must be applied to the parent to
avoid internal spacing inside the Pressable; locate the comment immediately
above const marginStyles and replace "RectButton" with "Pressable" and adjust
phrasing to match the current implementation.
- Line 23: The public prop rectButtonStyle on the Touch component still
references the removed RectButton; add a new generic prop (e.g., containerStyle)
to the TouchProps interface and update the Touch component to accept both
containerStyle and rectButtonStyle, mapping them to the actual underlying style
prop (merge containerStyle and rectButtonStyle, with containerStyle taking
precedence) so existing users don’t break; keep rectButtonStyle as a deprecated
alias and emit a single console.warn when rectButtonStyle is passed (or both are
passed) to guide users to containerStyle, and update any prop-type/TS
definitions and the component’s props destructuring/usage to use the merged
style for the rendered wrapper element.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3eb629a8-33d6-45b3-8abb-6a365995821d
⛔ Files ignored due to path filters (2)
ios/Podfile.lockis excluded by!**/*.lockyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (6)
app/containers/Touch.tsxapp/containers/message/Reactions.tsxapp/containers/message/Thread.tsxapp/containers/message/Touchable.tsxapp/containers/message/index.tsxpackage.json
✅ Files skipped from review due to trivial changes (2)
- package.json
- app/containers/message/Reactions.tsx
📜 Review details
⏰ 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 context used
🧠 Learnings (2)
📚 Learning: 2026-03-10T15:21:45.098Z
Learnt from: Rohit3523
Repo: RocketChat/Rocket.Chat.ReactNative PR: 7046
File: app/containers/InAppNotification/NotifierComponent.stories.tsx:46-75
Timestamp: 2026-03-10T15:21:45.098Z
Learning: In `app/containers/InAppNotification/NotifierComponent.tsx` (React Native, Rocket.Chat), `NotifierComponent` is exported as a Redux-connected component via `connect(mapStateToProps)`. The `isMasterDetail` prop is automatically injected from `state.app.isMasterDetail` and does not need to be passed explicitly at call sites or in Storybook stories that use the default (connected) export.
Applied to files:
app/containers/message/index.tsxapp/containers/message/Thread.tsxapp/containers/Touch.tsx
📚 Learning: 2026-03-04T20:13:17.288Z
Learnt from: divyanshu-patil
Repo: RocketChat/Rocket.Chat.ReactNative PR: 6957
File: ios/RocketChat Watch App/Views/MessageComposerView.swift:37-55
Timestamp: 2026-03-04T20:13:17.288Z
Learning: In the WatchOS app (ios/RocketChat Watch App) for Rocket.Chat React Native, using SwiftUI `Button` inside a `ScrollView` on WatchOS causes accidental message sends because button tap targets can be triggered during scroll gestures. `Text` with `.onTapGesture` is the preferred pattern for tappable items in scroll views on WatchOS. To preserve accessibility, add `.accessibilityAddTraits(.isButton)` and `.accessibilityLabel()` to the `Text` element instead.
Applied to files:
app/containers/message/Touchable.tsxapp/containers/message/Thread.tsxapp/containers/Touch.tsx
🔇 Additional comments (6)
app/containers/message/Touchable.tsx (2)
24-25: Verify intentional fallback ofonHoverOuttoonLongPress.When no
onHoverOuthandler is provided, this falls back to triggeringonLongPresson hover-out. This means hovering away from the element will invoke the long-press action (typically showing message actions), which may cause unexpected behavior on desktop/mouse users.If this is intentional for accessibility or specific UX requirements, please add a comment explaining the rationale. Otherwise, consider using a neutral fallback like
undefined.
12-13: LGTM on keyboard focus integration.The use of
withKeyboardFocusHOC to wrapPressableand the addition of thefocusableprop correctly enables keyboard navigation for message items, aligning with the PR's objective to fix inverted keyboard navigation on Android.Also applies to: 21-28
app/containers/message/index.tsx (1)
444-444: LGTM on exposingonThreadPressvia context.This correctly adds
onThreadPressto theMessageContext.Providervalue, enablingThread.tsx(and potentially other child components) to invoke thread navigation via keyboard or press interaction. The wiring aligns with the existingonThreadPressmethod defined at lines 236-241.app/containers/message/Thread.tsx (1)
10-10: LGTM on Thread keyboard accessibility.The component correctly:
- Imports and uses the keyboard-focusable
Touchablewrapper- Consumes
onThreadPressfromMessageContext- Applies proper accessibility attributes (
accessibilityRole='button',accessibilityLabel)- Preserves the
testIDfor testingThis enables keyboard-driven thread navigation as intended by the PR.
Also applies to: 17-17, 28-35
app/containers/Touch.tsx (2)
80-106: LGTM on the Pressable implementation.The migration from
RectButtontoPressableis well-structured:
android_rippleprovides native feedback on Android- Dynamic
stylecallback handles pressed-state visuals- Accessibility props correctly applied to inner
Viewdisabledprop correctly wired toPressableThis aligns with the PR's goal of enabling proper keyboard navigation focus handling.
85-93: Dark theme press feedback is nearly imperceptible on iOS.With
activeOpacitydefaulting to1, iOS press feedback relies solely on backgroundColor shift fromsurfaceRoomtosurfaceNeutral. In light mode, this shift is adequate (#FFFFFF→#E4E7EA). However, in dark mode, the contrast is too subtle (#1F2329→#2D3039). Components without explicit backgroundColor—such asmessage/User(titleContainer),RoomView/LoadMore(button), andScreenLockedView(close)—show almost no visual feedback on press in dark theme.Consider setting
activeOpacityto a lower value (e.g.,0.8or0.7) as the default or for dark-theme variants to provide more obvious press feedback across iOS.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Proposed changes
Issue(s)
https://rocketchat.atlassian.net/browse/MA-266
How to test or reproduce
Screenshots
Types of changes
Checklist
Further comments
Summary by CodeRabbit
New Features
Bug Fixes
Refactor