fix(uikit): migrate module-scoped ref to instance-local ref in Overflow#7435
fix(uikit): migrate module-scoped ref to instance-local ref in Overflow#7435Shevilll wants to merge 6 commits into
Conversation
Validate the username field against the server's UTF8_User_Names_Validation setting (with the default pattern as a fallback) before submitting the profile. This prevents invalid characters such as spaces from reaching the server, which previously surfaced only as a generic save error, and shows an inline message explaining the allowed characters. Closes RocketChat#1682
Drop the stale isMasterDetail destructured from useAppSelector (the selector no longer returns it) so only the useMasterDetail() hook declares it, fixing the no-redeclare lint/build failure introduced by merging develop. Also annotate isValidUsername with an explicit boolean return type per the TS guideline.
|
Warning Review limit reached
Next review available in: 55 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughProfileView now validates usernames against a server-provided rule, shows a localized invalid-username message, and updates submit tests. Overflow now uses a per-instance ref for its popover anchor instead of a shared ref map. ChangesProfile username validation
Overflow popover refactor
Estimated code review effort: 2 (Simple) | ~15 minutes Suggested labels: 🚥 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
app/containers/UIKit/Overflow.tsx (1)
48-48: 📐 Maintainability & Code Quality | 🔵 TrivialType the popover anchor ref as
Viewfor better type safetyUsing
anyinuseRefdiscards type safety. SinceTouch(which wrapsRectButton) forwards the ref to a nativeViewinstance, type the ref explicitly asViewto ensure type safety end-to-end.♻️ Proposed changes
- const touchableRef = useRef<any>(null); + const touchableRef = useRef<View>(null);Add
Viewto thereact-nativeimports:-import { FlatList, StyleSheet, Text } from 'react-native'; +import { FlatList, StyleSheet, Text, View } from 'react-native';🤖 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/UIKit/Overflow.tsx` at line 48, The popover anchor ref in Overflow is typed as any, which removes type safety. Update the touchableRef in the Overflow component to use the native View type instead, and add View to the react-native imports so the ref matches the Touch/RectButton forwarding behavior. Keep the change scoped to the useRef declaration and related import in Overflow.Source: Coding guidelines
🤖 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 `@app/i18n/locales/en.json`:
- Line 1005: The Username_invalid locale copy is hardcoded to the default ASCII
username rule, but the validator now depends on UTF8_User_Names_Validation from
settings. Update the message in the localization entry to be generic or driven
by the active username rule so it matches the actual validation behavior. Use
the Username_invalid key and the validation/settings path that reads
UTF8_User_Names_Validation to locate the affected text.
---
Nitpick comments:
In `@app/containers/UIKit/Overflow.tsx`:
- Line 48: The popover anchor ref in Overflow is typed as any, which removes
type safety. Update the touchableRef in the Overflow component to use the native
View type instead, and add View to the react-native imports so the ref matches
the Touch/RectButton forwarding behavior. Keep the change scoped to the useRef
declaration and related import in Overflow.
🪄 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: a34d0059-4da5-4da0-bfda-84644824adec
📒 Files selected for processing (4)
app/containers/UIKit/Overflow.tsxapp/i18n/locales/en.jsonapp/views/ProfileView/index.test.tsxapp/views/ProfileView/index.tsx
📜 Review details
🧰 Additional context used
📓 Path-based instructions (7)
**/*.{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/i18n/locales/en.jsonapp/views/ProfileView/index.test.tsxapp/containers/UIKit/Overflow.tsxapp/views/ProfileView/index.tsx
app/i18n/**/*.{ts,tsx,json}
📄 CodeRabbit inference engine (CLAUDE.md)
Place internationalization (i18n) configuration in 'app/i18n/' directory with support for 40+ locales and RTL
Files:
app/i18n/locales/en.json
**/*.{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/views/ProfileView/index.test.tsxapp/containers/UIKit/Overflow.tsxapp/views/ProfileView/index.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/views/ProfileView/index.test.tsxapp/containers/UIKit/Overflow.tsxapp/views/ProfileView/index.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/views/ProfileView/index.test.tsxapp/containers/UIKit/Overflow.tsxapp/views/ProfileView/index.tsx
app/views/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Place screen components in 'app/views/' directory
Files:
app/views/ProfileView/index.test.tsxapp/views/ProfileView/index.tsx
app/containers/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Place reusable UI components in 'app/containers/' directory
Files:
app/containers/UIKit/Overflow.tsx
🧠 Learnings (2)
📚 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/views/ProfileView/index.test.tsxapp/containers/UIKit/Overflow.tsxapp/views/ProfileView/index.tsx
📚 Learning: 2026-06-24T22:58:43.390Z
Learnt from: Rohit3523
Repo: RocketChat/Rocket.Chat.ReactNative PR: 7157
File: app/views/MessagesView/index.tsx:392-392
Timestamp: 2026-06-24T22:58:43.390Z
Learning: When wrapping a React Native component (e.g., via `withSafeAreaInsets`) ensure `hoistNonReactStatics` is only required if the wrapped component actually defines static properties/methods that consumers rely on. If the component has no statics (as in `app/views/MessagesView/index.tsx`), you can omit `hoistNonReactStatics` for this case.
Applied to files:
app/views/ProfileView/index.test.tsxapp/views/ProfileView/index.tsx
🔇 Additional comments (3)
app/containers/UIKit/Overflow.tsx (2)
1-2: LGTM!
44-69: LGTM!app/views/ProfileView/index.tsx (1)
55-60: 🎯 Functional CorrectnessKeep the client regex construction identical to the server.
Line 57 wraps the configured pattern in
(...), but the server code (apps/meteor/app/lib/server/functions/validateUsername.ts) builds the regex asnew RegExp(\^${settingsRegExp}$`)without that grouping. This difference alters the behavior for patterns containing alternation (|`), causing valid usernames to mismatch behavior between client and server.Suggested fix
const isValidUsername = (username: string, pattern: string): boolean => { try { - return new RegExp(`^(${pattern || DEFAULT_USERNAME_VALIDATION})$`).test(username); + return new RegExp(`^${pattern || DEFAULT_USERNAME_VALIDATION}$`).test(username); } catch { // Fall back to the default pattern if the server provides an invalid regex. - return new RegExp(`^(${DEFAULT_USERNAME_VALIDATION})$`).test(username); + return new RegExp(`^${DEFAULT_USERNAME_VALIDATION}$`).test(username); } };> Likely an incorrect or invalid review comment.
| "User_not_found_or": "User not found or incorrect password", | ||
| "User_sent_an_attachment": "{{user}} sent an attachment", | ||
| "Username": "Username", | ||
| "Username_invalid": "Use only letters, numbers, dots, hyphens and underscores", |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Make this validation message generic or rule-driven.
Line 1005 describes only the default ASCII rule, but the validator now reads UTF8_User_Names_Validation from settings. Workspaces with a custom or UTF-8-friendly username regex will show misleading guidance here.
🤖 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/i18n/locales/en.json` at line 1005, The Username_invalid locale copy is
hardcoded to the default ASCII username rule, but the validator now depends on
UTF8_User_Names_Validation from settings. Update the message in the localization
entry to be generic or driven by the active username rule so it matches the
actual validation behavior. Use the Username_invalid key and the
validation/settings path that reads UTF8_User_Names_Validation to locate the
affected text.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@app/containers/UIKit/Overflow.tsx`:
- Line 2: The import in Overflow.tsx mixes a value import with a symbol used
only as a type, causing a consistent-type-imports lint violation. Update the
React Native import so that View is brought in as a type-only import, while
keeping FlatList, StyleSheet, and Text as regular value imports. Use the
existing Overflow component and its useRef<View> usage as the reference point
when adjusting the import statement.
🪄 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: 426afb07-acf5-4ad1-ab17-b13f2cc2892d
📒 Files selected for processing (1)
app/containers/UIKit/Overflow.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/UIKit/Overflow.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/UIKit/Overflow.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/UIKit/Overflow.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/UIKit/Overflow.tsx
app/containers/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Place reusable UI components in 'app/containers/' directory
Files:
app/containers/UIKit/Overflow.tsx
🧠 Learnings (2)
📚 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/UIKit/Overflow.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/UIKit/Overflow.tsx
🪛 ESLint
app/containers/UIKit/Overflow.tsx
[error] 2-2: Imports "View" are only used as type.
(@typescript-eslint/consistent-type-imports)
🔇 Additional comments (1)
app/containers/UIKit/Overflow.tsx (1)
44-68: Per-instance ref correctly fixes the memory leak.Replacing the shared
createRef/blockId-keyed map with a localuseRef<View>ties the ref lifecycle to the component instance, resolving the described leak.
1 participant
Description
This Pull Request resolves a memory leak issue in the UIKit
Overflowcomponent (Issue #7095).Problem
Previously, a module-scoped registry was used to store element references. Because this registry existed at the module level rather than inside individual component instances:
Overflowcomponent, references and state could leak/bleed between different instances.Solution
Migrated the reference management to an instance-local React
useRefhook. This ensures that:Overflowcomponent instance.Summary by CodeRabbit
Bug Fixes
New Features
Tests