Conversation
tassoevan
had a problem deploying
to
approve_e2e_testing
GitHub Actions
Error
WalkthroughRemoves Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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 |
tassoevan
had a problem deploying
to
official_ios_build
GitHub Actions
Error
tassoevan
had a problem deploying
to
experimental_android_build
GitHub Actions
Error
tassoevan
had a problem deploying
to
official_android_build
GitHub Actions
Error
tassoevan
had a problem deploying
to
experimental_ios_build
GitHub Actions
Error
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/lib/methods/serializeAsciiUrl.test.ts (1)
8-8:⚠️ Potential issue | 🟠 Major
jest.mockinsidedescribedoes not produce per-suite platform isolation
jest.mockcalls are hoisted bybabel-jestto the top of the file regardless of where they appear in the source. When two factories are registered for the same module (react-native), Jest uses the last one for the entire file — meaning the iOSdescribeblock also runs under the Android mock ({ Platform: { OS: 'android' } }). The iOS tests therefore do not actually test iOS behaviour.A reliable approach is to use
jest.isolateModulesinside each test so the module registry is reset before each mock is applied:♻️ Proposed fix using `jest.isolateModules`
-describe('Serialize ASCII url on ios', () => { - jest.mock('react-native', () => ({ Platform: { OS: 'ios' } })); - test('ASCII url', () => { - const result = serializeAsciiUrl(ASCIIUrl); - expect(result).toBe(ASCIIUrlSerialized); - }); - test('Non ASCII url', () => { - const result = serializeAsciiUrl(NonASCIIUrl); - expect(result).toBe(NonASCIIUrl); - }); -}); - -describe('Serialize ASCII url on android', () => { - jest.mock('react-native', () => ({ Platform: { OS: 'android' } })); - // By default android converts ASCII addresses - test('ASCII url', () => { - const result = serializeAsciiUrl(ASCIIUrl); - expect(result).toBe(ASCIIUrlSerialized); - }); - test('Non ASCII url', () => { - const result = serializeAsciiUrl(NonASCIIUrl); - expect(result).toBe(NonASCIIUrl); - }); -}); +describe.each([ + ['ios', ASCIIUrlSerialized], + ['android', ASCIIUrlSerialized], // android converts ASCII addresses natively +])('Serialize ASCII url on %s', (os, expectedASCII) => { + let serializeAsciiUrlFn: typeof import('./serializeAsciiUrl').serializeAsciiUrl; + + beforeEach(() => { + jest.resetModules(); + jest.mock('react-native', () => ({ Platform: { OS: os } })); + // eslint-disable-next-line `@typescript-eslint/no-require-imports` + serializeAsciiUrlFn = require('./serializeAsciiUrl').serializeAsciiUrl; + }); + + test('ASCII url', () => { + expect(serializeAsciiUrlFn(ASCIIUrl)).toBe(expectedASCII); + }); + test('Non ASCII url', () => { + expect(serializeAsciiUrlFn(NonASCIIUrl)).toBe(NonASCIIUrl); + }); +});Also applies to: 19-25
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/lib/methods/serializeAsciiUrl.test.ts` at line 8, The jest.mock calls for 'react-native' are hoisted and cause cross-suite leakage; update the tests in serializeAsciiUrl.test.ts to remove per-describe jest.mock usage and instead wrap each test (or beforeEach) in jest.isolateModules so you can call jest.mock('react-native', () => ({ Platform: { OS: 'ios' } })) or the Android variant inside the isolate callback and then require the module under test (the serializeAsciiUrl import) inside that isolateModules block; ensure each test requires the module after mocking so Platform.OS is correctly isolated for tests referencing serializeAsciiUrl.
🧹 Nitpick comments (7)
app/containers/MessageComposer/components/Quotes/Quote.tsx (1)
52-53: Same optional root-cause fix applies as inDiscussionsView/Item.tsx.If
MarkdownPreview'sstyleprop acceptsStyleProp<TextStyle>,style={styles.message}removes the need for the suppress comment.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/containers/MessageComposer/components/Quotes/Quote.tsx` around lines 52 - 53, The inline eslint suppression exists because style is passed as a single-element array; update the JSX in Quote.tsx to pass the style directly (use style={styles.message} instead of style={[styles.message]}) and remove the eslint-disable-next-line comment; if TypeScript complains about prop typing, adjust MarkdownPreview's prop type to accept StyleProp<TextStyle> (or cast the value to that type) so the direct style prop is valid—look for the MarkdownPreview prop definition and styles.message to make the change, similar to the fix applied in DiscussionsView/Item.tsx.app/ee/omnichannel/lib/index.ts (1)
56-56: Considerconst _omnichannel = new Omnichannel()for consistency with the PR's underscore convention.The anonymous instantiation is functionally safe (the
EventEmitterholds references to the bound methods, preventing GC of the instance), but it departs from the underscore-prefix pattern applied everywhere else in this PR (_data,_b64,_messageId,_rid). Using_omnichannelmakes the "instantiate for side-effects" intent explicit and keeps the codebase idiom consistent.♻️ Suggested change
-new Omnichannel(); +const _omnichannel = new Omnichannel();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/ee/omnichannel/lib/index.ts` at line 56, The code instantiates Omnichannel anonymously which breaks the PR's underscore-prefix convention; change the anonymous call to a named const (e.g., _omnichannel) so the instance is assigned and follows the pattern used for other module-level side-effect instances; locate the new Omnichannel() invocation in the module (symbol: Omnichannel) and replace it with a const declaration named _omnichannel assigned to the new Omnichannel() instance.app/containers/List/ListRadio.tsx (1)
13-13: Prefer_valueover bare_for the discarded prop.Using
_drops all context about which prop is intentionally ignored._valuesatisfies both TypeScriptnoUnusedParametersand ESLintno-unused-vars(underscore prefix convention) while keeping the original name readable.♻️ Proposed rename
-const ListRadio = ({ value: _, isSelected, ...rest }: IListRadio) => { +const ListRadio = ({ value: _value, isSelected, ...rest }: IListRadio) => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/containers/List/ListRadio.tsx` at line 13, In ListRadio change the discarded prop name from "_" to "_value" so the parameter list becomes ({ value: _value, isSelected, ...rest }: IListRadio) — update the ListRadio function signature to use _value instead of _ to satisfy TypeScript/ESLint unused-var conventions while preserving the original prop name context.app/lib/encryption/room.ts (1)
503-503:_ridparameter is structurally dead — consider removing it.
file: TSendFileMessageFileInfoalready includesrid, so the first parameter has never been needed. The rename to_ridsatisfies the linter but leaves every caller supplying a redundant argument.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/lib/encryption/room.ts` at line 503, The function encryptFile currently accepts a redundant first parameter named _rid even though file: TSendFileMessageFileInfo already contains rid; remove the unused _rid parameter from encryptFile's signature and update the implementation to use file.rid wherever needed (refer to encryptFile and TSendFileMessageFileInfo), then update all callers to stop passing the extra rid argument so they call encryptFile(file) and adjust any related type references (e.g., TEncryptFileResult) as needed to match the new signature..eslintrc.js (3)
19-25:'prettier'inextendsis redundant when'plugin:prettier/recommended'is already present.
plugin:prettier/recommendedincludeseslint-config-prettier(which is what'prettier'resolves to). The same duplication appears in the TypeScript override at lines 81–82.♻️ Suggested cleanup
extends: [ '@rocket.chat/eslint-config', 'plugin:react/recommended', 'plugin:react-hooks/recommended', - 'plugin:prettier/recommended', - 'prettier' + 'plugin:prettier/recommended' ],And in the TS override:
extends: [ 'plugin:`@typescript-eslint/recommended`', 'plugin:`@typescript-eslint/eslint-recommended`', '@rocket.chat/eslint-config', - 'plugin:prettier/recommended', - 'prettier' + 'plugin:prettier/recommended' ],🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.eslintrc.js around lines 19 - 25, The extends array currently includes both "plugin:prettier/recommended" and the redundant "prettier"; remove the duplicate "prettier" entry from the top-level extends list and also remove the duplicate "prettier" entry from the TypeScript override extends to avoid redundant inclusion (locate the extends arrays where "plugin:prettier/recommended" and "prettier" appear and delete only the plain "prettier" entries).
96-100: Several turned-off rules are deprecated in@typescript-eslintv7.
@typescript-eslint/indent,@typescript-eslint/no-extra-parens,@typescript-eslint/no-var-requires, andno-spaced-funcare all deprecated (formatting rules were removed in v6+,no-var-requiresreplaced byno-require-imports). Turning them off is harmless but adds dead configuration. Consider removing them in a follow-up cleanup pass.Also applies to: 108-108, 112-112
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.eslintrc.js around lines 96 - 100, The .eslintrc.js currently disables several deprecated `@typescript-eslint` rules; remove the dead configuration entries for '@typescript-eslint/indent', '@typescript-eslint/no-extra-parens', 'no-spaced-func', and '@typescript-eslint/no-var-requires' (and any duplicate occurrences of those exact rule keys elsewhere in the file) so the config no longer contains deprecated/removed rule names; keep other valid rules like '@typescript-eslint/no-dupe-class-members' and '@typescript-eslint/no-empty-function' unchanged.
45-47:import/no-cyclecan be expensive on large codebases.This rule performs deep dependency graph traversal. In a React Native monorepo of this size, it can noticeably slow down lint runs. Consider setting
maxDepthto bound the cost, or running it only in CI rather than in editor integrations.♻️ Option to bound cycle detection depth
- 'import/no-cycle': 'error', + 'import/no-cycle': ['error', { maxDepth: 5 }],🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.eslintrc.js around lines 45 - 47, The 'import/no-cycle' rule in .eslintrc.js is expensive on large monorepos; update its configuration (the 'import/no-cycle' entry) to limit traversal by adding a maxDepth option (e.g., set maxDepth to a small integer like 3–5) or move/override the rule so it's only enforced in CI/editor-independent runs (e.g., enable it in an ESLint CI override or disable it for editor integrations), ensuring you reference the 'import/no-cycle' rule name when making the change.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (5)
app/containers/Avatar/__snapshots__/Avatar.test.tsx.snapis excluded by!**/*.snapapp/containers/markdown/__snapshots__/Markdown.test.tsx.snapis excluded by!**/*.snapapp/containers/message/__snapshots__/Message.test.tsx.snapis excluded by!**/*.snapapp/views/DiscussionsView/__snapshots__/Item.test.tsx.snapis excluded by!**/*.snapyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (39)
.eslintignore.eslintrc.js__mocks__/react-native-mmkv.jsapp/containers/ActionSheet/ActionSheet.tsxapp/containers/AudioPlayer/Seek.tsxapp/containers/Avatar/Avatar.stories.tsxapp/containers/Avatar/AvatarContainer.tsxapp/containers/Avatar/AvatarWithEdit.tsxapp/containers/List/ListRadio.tsxapp/containers/MessageComposer/components/Quotes/Quote.tsxapp/containers/MessageComposer/helpers/getMentionRegexp.test.jsapp/containers/UnreadBadge/getUnreadStyle.test.jsapp/containers/markdown/components/emoji/Emoji.tsxapp/containers/message/Components/Attachments/Reply.tsxapp/ee/omnichannel/lib/index.tsapp/ee/omnichannel/views/QueueListView.tsxapp/lib/database/utils.test.tsapp/lib/encryption/encryption.tsapp/lib/encryption/helpers/base64-js/index.tsapp/lib/encryption/room.tsapp/lib/helpers/formatText.tsapp/lib/hooks/useShortnameToUnicode/index.tsxapp/lib/methods/search.tsapp/lib/methods/serializeAsciiUrl.test.tsapp/sagas/createChannel.jsapp/sagas/createDiscussion.jsapp/sagas/deepLinking.jsapp/sagas/login.jsapp/sagas/rooms.jsapp/sagas/state.jsapp/views/CreateChannelView/RoomSettings/SwitchItemEncrypted.test.tsxapp/views/DiscussionsView/DiscussionDetails.tsxapp/views/DiscussionsView/Item.tsxapp/views/RoomView/Banner.tsxapp/views/SelectServerView.tsxapp/views/ThreadMessagesView/Item.tsxpackage.jsonscripts/build-icon-set.jstsconfig.json
💤 Files with no reviewable changes (1)
- .eslintignore
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2026-02-05T13:55:00.974Z
Learnt from: Rohit3523
Repo: RocketChat/Rocket.Chat.ReactNative PR: 6930
File: package.json:101-101
Timestamp: 2026-02-05T13:55:00.974Z
Learning: In this repository, the dependency on react-native-image-crop-picker should reference the RocketChat fork (RocketChat/react-native-image-crop-picker) with explicit commit pins, not the upstream ivpusic/react-native-image-crop-picker. Update package.json dependencies (and any lockfile) to point to the fork URL and a specific commit, ensuring edge-to-edge Android fixes are included. This pattern should apply to all package.json files in the repo that declare this dependency.
Applied to files:
package.json
🧬 Code graph analysis (6)
app/sagas/state.js (1)
app/lib/notifications/index.ts (1)
checkPendingNotification(100-135)
app/lib/encryption/encryption.ts (1)
app/lib/encryption/definitions.ts (1)
TDecryptFile(18-23)
app/views/SelectServerView.tsx (1)
app/containers/ServerItem/index.tsx (1)
ROW_HEIGHT(12-12)
app/sagas/createDiscussion.js (2)
app/sagas/createChannel.js (1)
subCollection(62-62)app/sagas/rooms.js (1)
subCollection(52-52)
app/lib/encryption/room.ts (2)
app/definitions/IUpload.ts (1)
TSendFileMessageFileInfo(26-29)app/lib/encryption/definitions.ts (1)
TEncryptFileResult(10-14)
app/containers/Avatar/AvatarWithEdit.tsx (1)
app/containers/message/MessageAvatar.tsx (1)
AvatarContainer(13-17)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: ESLint and Test / run-eslint-and-test
🔇 Additional comments (35)
app/lib/helpers/formatText.ts (1)
5-5: LGTM!Renaming
match→_matchcorrectly signals the parameter is intentionally unused, consistent with the lint rule being enforced across the PR. No behavioral change.app/lib/database/utils.test.ts (1)
13-21: LGTM — clean consolidation intotest.each.The array-of-arrays format,
%stitle interpolation,_prefix for the unused label parameter, and length-based'_'.repeat(disallowedChars.length)expectation (which correctly handles_being both a disallowed char and the replacement character) are all correct.app/lib/methods/serializeAsciiUrl.test.ts (1)
22-25: LGTM — placeholder replaced with the correct constantChanging the expected value from the stale
'filename.png'placeholder toASCIIUrlSerializedcorrectly reflects the behaviour described in the comment above (Android serializes ASCII URLs to their Punycode form).app/sagas/createDiscussion.js (1)
26-29: LGTM — parentheses removal is stylistically clean.app/lib/methods/search.ts (1)
141-142: LGTM —_resolverename correctly signals the unused parameter.The executor only assigns
rejecttodebounce;resolvewas and remains uncalled. The cancellation pattern is sound.app/sagas/login.js (1)
61-64: LGTM — consistent single-parameter arrow function style across WatermelonDB callbacks.app/lib/hooks/useShortnameToUnicode/index.tsx (1)
36-48: LGTM —_m1rename correctly reflects that capture group 1 is unused.
entire(position 0) already covers the full match;_m1(position 1) would be its duplicate, so the underscore prefix is appropriate.app/sagas/state.js (1)
31-33: LGTM.app/containers/message/Components/Attachments/Reply.tsx (1)
264-271: LGTM — removing the single-element array from the outerViewis correct.The outer
View(container) only needsstyles.backdrop; the innerView(lines 266–269) correctly retains the two-element array for dynamic opacity/color. The treatment is consistent with React Native'sStyleProp<ViewStyle>.app/views/DiscussionsView/Item.tsx (1)
80-81: The current implementation with the eslint-disable comment is correct and necessary.The
styleprop inMarkdownPreviewis explicitly typed asTextStyle[](seeapp/containers/markdown/components/Preview.tsxline 13), so the array wrapperstyle={[styles.markdown]}is required by the component's type signature. The component spreads this array into a larger style array (line 30), confirming the array type is intentional. The eslint-disable comment correctly suppresses a false-positive lint warning in this case—removing the array would violate the component's type contract.app/ee/omnichannel/views/QueueListView.tsx (1)
70-77: LGTM.Renaming
data→_datacorrectly communicates that the first parameter is intentionally unused, satisfying the newly enablednoUnusedParametersrule without any behavioral change.app/sagas/createChannel.js (1)
64-67: LGTM.Removing the parentheses around the single
sparameter is idiomatic and semantically identical.app/sagas/deepLinking.js (1)
49-56: LGTM.Removing the parentheses around the single
resolveparameter is a no-op style change consistent with the rest of the PR.app/sagas/rooms.js (1)
21-23: LGTM.All six single-parameter arrow functions consistently drop their optional parentheses. No behavioral change in any case.
Also applies to: 78-82, 83-101, 102-109, 111-115, 117-127
app/containers/UnreadBadge/getUnreadStyle.test.js (1)
5-5: LGTM.Removing parentheses around the single
themeparameter is a consistent style-only change.app/views/ThreadMessagesView/Item.tsx (1)
91-92: The ESLint disable comment is appropriate and necessary here.
MarkdownPreview'sstyleprop is explicitly typed asTextStyle[](an array), not as a scalar orStyleProp<TextStyle>. The component spreads the style into an array:style={[styles.text, { color: ... }, ...style]}. Therefore,style={[styles.markdown]}is the correct way to pass a value, and the ESLint suppress comment is the right approach—no refactoring needed.app/views/SelectServerView.tsx (1)
18-18: LGTM — correct underscore-prefix convention for the unuseddataparameter.tsconfig.json (1)
38-39: Good strictness uplift — enablingnoUnusedLocalsandnoUnusedParameters.These flags are the root driver for all the
_-prefixed parameter renames throughout the rest of this PR, so the codebase is already consistent with them.app/views/DiscussionsView/DiscussionDetails.tsx (1)
45-45: LGTM — removing the single-item style array wrapper is functionally equivalent in React Native.scripts/build-icon-set.js (1)
4-4: LGTM — explicit block body is functionally equivalent and more readable.app/lib/encryption/helpers/base64-js/index.ts (1)
47-48: LGTM —_b64is correctly marked unused; the function body only needsvalidLenandplaceHoldersLen.app/containers/markdown/components/emoji/Emoji.tsx (1)
72-72: LGTM — removing the single-element style array is functionally equivalent.app/lib/encryption/encryption.ts (1)
599-609: LGTM —_messageIdcorrectly signals the unused first parameter; implementation aligns with theTDecryptFilecontract.app/containers/Avatar/AvatarContainer.tsx (1)
74-76: LGTM — idiomatic JSX children pattern.Switching from
children={children}prop syntax to nested JSX is a style improvement with no behavioral difference.app/containers/MessageComposer/helpers/getMentionRegexp.test.js (1)
32-32: LGTM — quote normalization with no behavioral change.app/containers/AudioPlayer/Seek.tsx (1)
68-71: LGTM — unused parameter correctly prefixed.app/containers/Avatar/AvatarWithEdit.tsx (1)
67-69: LGTM — consistent with the children-as-JSX pattern adopted inAvatarContainer.tsx.app/containers/Avatar/Avatar.stories.tsx (1)
67-67: LGTM — unnecessary single-element array wrapper correctly removed.React Native's
StylePropaccepts a registered style number directly; the array wrapper was redundant.app/containers/ActionSheet/ActionSheet.tsx (1)
171-173: LGTM — same idiomatic JSX children pattern applied consistently.package.json (2)
177-178: Version range change from^to~for TypeScript ESLint packages.Switching from
^7.4.0to~7.18.0tightens the allowed range to patch-only updates, which is a sensible choice for linting tooling stability. 7.18.0 is the latest 7.x release, so this effectively pins at the final minor.
184-192: Botheslint-plugin-react-hooksv7.0.1 andeslint-plugin-react-nativev5.x are compatible with ESLint 8.57.0.
eslint-plugin-react-hooks7.0.1 declares ESLint peer dependency^8.0.0-0 || ^9.0.0, confirming ESLint 8 support.eslint-plugin-react-native5.x is also compatible with ESLint 8 using the classic.eslintrc*format. The three rules referenced (react-hooks/set-state-in-effect,react-hooks/immutability,react-hooks/refs) are already in use at lines 60–62 with no apparent issues. The ESLint 9 flat config migration is irrelevant for the current ESLint 8 setup.Likely an incorrect or invalid review comment.
__mocks__/react-native-mmkv.js (1)
87-87: Stylistic-only changes — LGTM.All changes remove redundant parentheses around single arrow function parameters, consistent with the Prettier/lint enforcement goals of this PR.
Also applies to: 105-105, 113-113, 129-129, 137-137, 153-153, 161-161, 180-180, 189-189
.eslintrc.js (3)
101-107: Good: strict unused-vars enforcement with underscore escape hatch.Configuring
args: 'all'withargsIgnorePattern: '^_'is a solid pattern — it catches genuinely unused parameters while allowing intentional placeholders. This aligns well with thetsconfig.jsonchanges enablingnoUnusedParameters.
14-16: Good structural improvements: auto-detecting React version and scoping environments.Auto-detecting the React version (line 15) eliminates the common "React version not specified" warning. The test-file override (line 131–133) and React Native env scoping (lines 134–139) properly isolate concerns.
Also applies to: 131-139
60-62: No action needed. All three rule names (react-hooks/set-state-in-effect,react-hooks/immutability,react-hooks/refs) are valid and confirmed to exist ineslint-plugin-react-hooksv7.0.0 as part of the React Compiler rules set enabled by default.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/views/CreateChannelView/RoomSettings/SwitchItemEncrypted.test.tsx`:
- Around line 59-61: The test is sending an object instead of a boolean to the
Switch handler and asserting on the returned object rather than the call
argument; update the fireEvent call that targets
screen.getByTestId(testEncrypted.testSwitchID) to pass a plain boolean (true) —
i.e. fireEvent(component, 'valueChange', true) — and replace the tautological
expect(onPressMock).toHaveReturnedWith(...) with
expect(onPressMock).toHaveBeenCalledWith(true) so the test verifies the callback
was invoked with a boolean as the Switch would in real usage (references:
testEncrypted, testSwitchID, fireEvent, onPressMock).
---
Outside diff comments:
In `@app/lib/methods/serializeAsciiUrl.test.ts`:
- Line 8: The jest.mock calls for 'react-native' are hoisted and cause
cross-suite leakage; update the tests in serializeAsciiUrl.test.ts to remove
per-describe jest.mock usage and instead wrap each test (or beforeEach) in
jest.isolateModules so you can call jest.mock('react-native', () => ({ Platform:
{ OS: 'ios' } })) or the Android variant inside the isolate callback and then
require the module under test (the serializeAsciiUrl import) inside that
isolateModules block; ensure each test requires the module after mocking so
Platform.OS is correctly isolated for tests referencing serializeAsciiUrl.
---
Duplicate comments:
In `@app/views/RoomView/Banner.tsx`:
- Around line 33-34: The Banner component is passing style as an array wrapper
to <MarkdownPreview> which triggered an eslint suppression; remove the array
wrapper so the prop is passed as style={styles.bannerText} (and delete the
eslint-disable-next-line comment) and, if necessary, update MarkdownPreview's
prop type to accept StyleProp<TextStyle> so this direct style object is valid;
locate the usage in Banner.tsx (MarkdownPreview msg={text}
style={[styles.bannerText]}) and change accordingly.
---
Nitpick comments:
In @.eslintrc.js:
- Around line 19-25: The extends array currently includes both
"plugin:prettier/recommended" and the redundant "prettier"; remove the duplicate
"prettier" entry from the top-level extends list and also remove the duplicate
"prettier" entry from the TypeScript override extends to avoid redundant
inclusion (locate the extends arrays where "plugin:prettier/recommended" and
"prettier" appear and delete only the plain "prettier" entries).
- Around line 96-100: The .eslintrc.js currently disables several deprecated
`@typescript-eslint` rules; remove the dead configuration entries for
'@typescript-eslint/indent', '@typescript-eslint/no-extra-parens',
'no-spaced-func', and '@typescript-eslint/no-var-requires' (and any duplicate
occurrences of those exact rule keys elsewhere in the file) so the config no
longer contains deprecated/removed rule names; keep other valid rules like
'@typescript-eslint/no-dupe-class-members' and
'@typescript-eslint/no-empty-function' unchanged.
- Around line 45-47: The 'import/no-cycle' rule in .eslintrc.js is expensive on
large monorepos; update its configuration (the 'import/no-cycle' entry) to limit
traversal by adding a maxDepth option (e.g., set maxDepth to a small integer
like 3–5) or move/override the rule so it's only enforced in
CI/editor-independent runs (e.g., enable it in an ESLint CI override or disable
it for editor integrations), ensuring you reference the 'import/no-cycle' rule
name when making the change.
In `@app/containers/List/ListRadio.tsx`:
- Line 13: In ListRadio change the discarded prop name from "_" to "_value" so
the parameter list becomes ({ value: _value, isSelected, ...rest }: IListRadio)
— update the ListRadio function signature to use _value instead of _ to satisfy
TypeScript/ESLint unused-var conventions while preserving the original prop name
context.
In `@app/containers/MessageComposer/components/Quotes/Quote.tsx`:
- Around line 52-53: The inline eslint suppression exists because style is
passed as a single-element array; update the JSX in Quote.tsx to pass the style
directly (use style={styles.message} instead of style={[styles.message]}) and
remove the eslint-disable-next-line comment; if TypeScript complains about prop
typing, adjust MarkdownPreview's prop type to accept StyleProp<TextStyle> (or
cast the value to that type) so the direct style prop is valid—look for the
MarkdownPreview prop definition and styles.message to make the change, similar
to the fix applied in DiscussionsView/Item.tsx.
In `@app/ee/omnichannel/lib/index.ts`:
- Line 56: The code instantiates Omnichannel anonymously which breaks the PR's
underscore-prefix convention; change the anonymous call to a named const (e.g.,
_omnichannel) so the instance is assigned and follows the pattern used for other
module-level side-effect instances; locate the new Omnichannel() invocation in
the module (symbol: Omnichannel) and replace it with a const declaration named
_omnichannel assigned to the new Omnichannel() instance.
In `@app/lib/encryption/room.ts`:
- Line 503: The function encryptFile currently accepts a redundant first
parameter named _rid even though file: TSendFileMessageFileInfo already contains
rid; remove the unused _rid parameter from encryptFile's signature and update
the implementation to use file.rid wherever needed (refer to encryptFile and
TSendFileMessageFileInfo), then update all callers to stop passing the extra rid
argument so they call encryptFile(file) and adjust any related type references
(e.g., TEncryptFileResult) as needed to match the new signature.
| const component = screen.getByTestId(testEncrypted.testSwitchID); | ||
| fireEvent(component, 'valueChange', { value: true }); | ||
| expect(onPressMock).toHaveReturnedWith({ value: !testEncrypted.encrypted }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
fd -e tsx -e ts SwitchItemEncrypted --exclude '*.test.*'Repository: RocketChat/Rocket.Chat.ReactNative
Length of output: 142
🏁 Script executed:
cat -n app/views/CreateChannelView/RoomSettings/SwitchItemEncrypted.tsxRepository: RocketChat/Rocket.Chat.ReactNative
Length of output: 1207
🏁 Script executed:
fd -e tsx -e ts SwitchItem --exclude '*.test.*' -path '*/RoomSettings/*'Repository: RocketChat/Rocket.Chat.ReactNative
Length of output: 246
🏁 Script executed:
fd SwitchItem --exclude '*.test.*' -path '*/RoomSettings/*'Repository: RocketChat/Rocket.Chat.ReactNative
Length of output: 246
🏁 Script executed:
fd SwitchItem --exclude '*.test.*'Repository: RocketChat/Rocket.Chat.ReactNative
Length of output: 386
🏁 Script executed:
cat -n app/views/CreateChannelView/RoomSettings/SwitchItem.tsxRepository: RocketChat/Rocket.Chat.ReactNative
Length of output: 2049
🏁 Script executed:
cat -n app/views/CreateChannelView/RoomSettings/SwitchItemEncrypted.test.tsxRepository: RocketChat/Rocket.Chat.ReactNative
Length of output: 3282
getByTestId upgrade is correct, but fireEvent event data format and the assertion need fixing.
Two issues:
-
fireEventdata format:fireEventpasses its extra arguments directly to the event handler. React Native'sSwitchcomponent callsonValueChange(boolean)with a plain boolean in real usage, but the test sendsfireEvent(component, 'valueChange', { value: true }), which dispatches an object instead. The mock (typed as(value: boolean) => value) receives and returns this object, violating its type contract. The event should befireEvent(component, 'valueChange', true)(a boolean). -
Tautological assertion: Since
testEncrypted.encrypted = false, the assertion{ value: !testEncrypted.encrypted }evaluates to{ value: true }— the exact object sent tofireEvent. Because the mock returns its argument unchanged,toHaveReturnedWith({ value: true })merely confirms data passes through unmodified; it validates nothing about component behavior. UsetoHaveBeenCalledWith(true)instead, which is both clearer and semantically correct for callback testing.
🛠️ Proposed fix
const component = screen.getByTestId(testEncrypted.testSwitchID);
- fireEvent(component, 'valueChange', { value: true });
- expect(onPressMock).toHaveReturnedWith({ value: !testEncrypted.encrypted });
+ fireEvent(component, 'valueChange', true);
+ expect(onPressMock).toHaveBeenCalledWith(true);📝 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.
| const component = screen.getByTestId(testEncrypted.testSwitchID); | |
| fireEvent(component, 'valueChange', { value: true }); | |
| expect(onPressMock).toHaveReturnedWith({ value: !testEncrypted.encrypted }); | |
| const component = screen.getByTestId(testEncrypted.testSwitchID); | |
| fireEvent(component, 'valueChange', true); | |
| expect(onPressMock).toHaveBeenCalledWith(true); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/views/CreateChannelView/RoomSettings/SwitchItemEncrypted.test.tsx` around
lines 59 - 61, The test is sending an object instead of a boolean to the Switch
handler and asserting on the returned object rather than the call argument;
update the fireEvent call that targets
screen.getByTestId(testEncrypted.testSwitchID) to pass a plain boolean (true) —
i.e. fireEvent(component, 'valueChange', true) — and replace the tautological
expect(onPressMock).toHaveReturnedWith(...) with
expect(onPressMock).toHaveBeenCalledWith(true) so the test verifies the callback
was invoked with a boolean as the Switch would in real usage (references:
testEncrypted, testSwitchID, fireEvent, onPressMock).
fd66233 to
800e82b
Compare
tassoevan
had a problem deploying
to
approve_e2e_testing
GitHub Actions
Error
There was a problem hiding this comment.
🧹 Nitpick comments (1)
app/lib/encryption/encryption.ts (1)
599-599: LGTM — unusedmessageIdparameter correctly prefixed.
_messageIdis never used in the body; decryption depends only onpath,encryption, andoriginalChecksum. The value is still correctly threaded throughaddFileToDecryptFileQueue→processFileQueue→decryptFile, so the queue contract is unaffected.For completeness, the
messageIdlabel in theTDecryptFiletype alias (app/lib/encryption/definitions.ts:17) could be updated to_messageIdto stay consistent with the implementation convention, though TypeScript does not enforce name parity between a type alias and its implementing assignment.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/lib/encryption/encryption.ts` at line 599, The TDecryptFile type alias's first parameter name should be renamed from messageId to _messageId in app/lib/encryption/definitions.ts to match the implementation's unused parameter naming; update the parameter label in the type alias (TDecryptFile) to _messageId so it aligns with decryptFile's signature and maintainers' convention—no behavioural changes needed, and calls through addFileToDecryptFileQueue and processFileQueue remain unaffected.
🤖 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/lib/encryption/encryption.ts`:
- Line 599: The TDecryptFile type alias's first parameter name should be renamed
from messageId to _messageId in app/lib/encryption/definitions.ts to match the
implementation's unused parameter naming; update the parameter label in the type
alias (TDecryptFile) to _messageId so it aligns with decryptFile's signature and
maintainers' convention—no behavioural changes needed, and calls through
addFileToDecryptFileQueue and processFileQueue remain unaffected.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (5)
app/containers/Avatar/__snapshots__/Avatar.test.tsx.snapis excluded by!**/*.snapapp/containers/markdown/__snapshots__/Markdown.test.tsx.snapis excluded by!**/*.snapapp/containers/message/__snapshots__/Message.test.tsx.snapis excluded by!**/*.snapapp/views/DiscussionsView/__snapshots__/Item.test.tsx.snapis excluded by!**/*.snapyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (39)
.eslintignore.eslintrc.js__mocks__/react-native-mmkv.jsapp/containers/ActionSheet/ActionSheet.tsxapp/containers/AudioPlayer/Seek.tsxapp/containers/Avatar/Avatar.stories.tsxapp/containers/Avatar/AvatarContainer.tsxapp/containers/Avatar/AvatarWithEdit.tsxapp/containers/List/ListRadio.tsxapp/containers/MessageComposer/components/Quotes/Quote.tsxapp/containers/MessageComposer/helpers/getMentionRegexp.test.jsapp/containers/UnreadBadge/getUnreadStyle.test.jsapp/containers/markdown/components/emoji/Emoji.tsxapp/containers/message/Components/Attachments/Reply.tsxapp/ee/omnichannel/lib/index.tsapp/ee/omnichannel/views/QueueListView.tsxapp/lib/database/utils.test.tsapp/lib/encryption/encryption.tsapp/lib/encryption/helpers/base64-js/index.tsapp/lib/encryption/room.tsapp/lib/helpers/formatText.tsapp/lib/hooks/useShortnameToUnicode/index.tsxapp/lib/methods/search.tsapp/lib/methods/serializeAsciiUrl.test.tsapp/sagas/createChannel.jsapp/sagas/createDiscussion.jsapp/sagas/deepLinking.jsapp/sagas/login.jsapp/sagas/rooms.jsapp/sagas/state.jsapp/views/CreateChannelView/RoomSettings/SwitchItemEncrypted.test.tsxapp/views/DiscussionsView/DiscussionDetails.tsxapp/views/DiscussionsView/Item.tsxapp/views/RoomView/Banner.tsxapp/views/SelectServerView.tsxapp/views/ThreadMessagesView/Item.tsxpackage.jsonscripts/build-icon-set.jstsconfig.json
💤 Files with no reviewable changes (1)
- .eslintignore
✅ Files skipped from review due to trivial changes (3)
- app/lib/methods/search.ts
- app/sagas/rooms.js
- app/views/SelectServerView.tsx
🚧 Files skipped from review as they are similar to previous changes (26)
- app/sagas/deepLinking.js
- app/lib/methods/serializeAsciiUrl.test.ts
- app/views/DiscussionsView/Item.tsx
- app/sagas/login.js
- app/containers/UnreadBadge/getUnreadStyle.test.js
- app/sagas/createChannel.js
- app/views/RoomView/Banner.tsx
- tsconfig.json
- mocks/react-native-mmkv.js
- app/lib/database/utils.test.ts
- app/containers/MessageComposer/components/Quotes/Quote.tsx
- app/ee/omnichannel/lib/index.ts
- app/sagas/state.js
- app/lib/encryption/helpers/base64-js/index.ts
- app/containers/List/ListRadio.tsx
- app/containers/message/Components/Attachments/Reply.tsx
- app/views/DiscussionsView/DiscussionDetails.tsx
- app/views/CreateChannelView/RoomSettings/SwitchItemEncrypted.test.tsx
- app/containers/Avatar/AvatarWithEdit.tsx
- app/containers/AudioPlayer/Seek.tsx
- app/sagas/createDiscussion.js
- app/containers/ActionSheet/ActionSheet.tsx
- app/containers/MessageComposer/helpers/getMentionRegexp.test.js
- app/containers/markdown/components/emoji/Emoji.tsx
- app/views/ThreadMessagesView/Item.tsx
- app/lib/helpers/formatText.ts
📜 Review details
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2026-02-05T13:55:00.974Z
Learnt from: Rohit3523
Repo: RocketChat/Rocket.Chat.ReactNative PR: 6930
File: package.json:101-101
Timestamp: 2026-02-05T13:55:00.974Z
Learning: In this repository, the dependency on react-native-image-crop-picker should reference the RocketChat fork (RocketChat/react-native-image-crop-picker) with explicit commit pins, not the upstream ivpusic/react-native-image-crop-picker. Update package.json dependencies (and any lockfile) to point to the fork URL and a specific commit, ensuring edge-to-edge Android fixes are included. This pattern should apply to all package.json files in the repo that declare this dependency.
Applied to files:
package.json
🧬 Code graph analysis (2)
app/lib/encryption/encryption.ts (1)
app/lib/encryption/definitions.ts (1)
TDecryptFile(18-23)
app/lib/encryption/room.ts (2)
app/definitions/IUpload.ts (1)
TSendFileMessageFileInfo(26-29)app/lib/encryption/definitions.ts (1)
TEncryptFileResult(10-14)
🔇 Additional comments (8)
app/ee/omnichannel/views/QueueListView.tsx (1)
70-70: Nice cleanup: intentional unused parameter is explicit.The
_datarename keeps theFlatListcallback signature while making non-usage clear for linting/readability.app/lib/hooks/useShortnameToUnicode/index.tsx (1)
36-36: Good lint-safe rename for unused capture group.Renaming to
_m1clearly marks it as intentionally unused while keeping them2/m3capture positions unchanged.app/containers/Avatar/AvatarContainer.tsx (1)
74-76: LGTM — idiomatic JSX child composition.Converting from
children={children}prop to nested JSX content is semantically equivalent in React and is the conventional pattern. No functional or type-safety regression.app/containers/Avatar/Avatar.stories.tsx (1)
67-67: LGTM — single-element style array correctly unwrapped.React Native's
styleprop accepts a plain style object directly; removing the redundant single-element array wrapper is valid and marginally more efficient (avoids an ephemeral array allocation on every render).app/lib/encryption/room.ts (1)
503-503: LGTM — unusedridparameter correctly prefixed.
_ridis never referenced inside the method body;this.roomIdis the source of truth for the room ID withinEncryptionRoom. The rename accurately signals intent.scripts/build-icon-set.js (1)
4-4: Reducer refactor preserves behavior.The block-bodied reducer still returns the same sorted object output while improving readability.
.eslintrc.js (1)
19-33: ESLint config refactor is clean and well-scoped.The updated base rules plus targeted TypeScript, Jest, and React Native overrides are organized clearly and align with the stated lint-consistency objective.
Also applies to: 45-73, 74-139
package.json (1)
177-178: No action needed. The@typescript-eslintversions are compatible with the repository's TypeScript version.The npm peer dependency metadata shows no TypeScript version constraint for
@typescript-eslint7.18.0, indicating compatibility with TypeScript 5.9.3. The concern about parser/lint breakage is unfounded.Likely an incorrect or invalid review comment.
tassoevan
force-pushed
the
chore/stylistic-changes-1
branch
from
800e82b to
dd86d07
Compare
tassoevan
had a problem deploying
to
approve_e2e_testing
GitHub Actions
Error
There was a problem hiding this comment.
♻️ Duplicate comments (1)
app/views/CreateChannelView/RoomSettings/SwitchItemEncrypted.test.tsx (1)
60-61:fireEventpayload and assertion are still incorrect.Both issues flagged in the prior review are unaddressed:
fireEvent(component, 'valueChange', { value: true })— passes an object to a handler typed as(value: boolean) => value; the argument should be the plain booleantrue.toHaveReturnedWith({ value: !testEncrypted.encrypted })— since the mock returns its argument and the argument is{ value: true }, this assertion is tautological and tests nothing meaningful. UsetoHaveBeenCalledWith(true)instead.🛠️ Proposed fix
- fireEvent(component, 'valueChange', { value: true }); - expect(onPressMock).toHaveReturnedWith({ value: !testEncrypted.encrypted }); + fireEvent(component, 'valueChange', true); + expect(onPressMock).toHaveBeenCalledWith(true);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/views/CreateChannelView/RoomSettings/SwitchItemEncrypted.test.tsx` around lines 60 - 61, The test fires the wrong payload and asserts the wrong behavior: change the fireEvent call in SwitchItemEncrypted.test to pass a plain boolean (true) to the handler (i.e., fireEvent(component, 'valueChange', true)) and replace the tautological toHaveReturnedWith check with a meaningful call assertion: assert that the mock onPress handler was called with true (i.e., toHaveBeenCalledWith(true)); refer to the test's fireEvent invocation and the onPressMock / testEncrypted.encrypted references to locate the lines to change.
🧹 Nitpick comments (1)
.eslintrc.js (1)
19-25: Redundant'prettier'entry inextends.
plugin:prettier/recommended(line 23) already incorporateseslint-config-prettieras its first extend, so the trailing'prettier'on line 24 has no effect and can be removed. The same duplication exists inside the TypeScript override (line 82).🧹 Proposed cleanup
extends: [ '@rocket.chat/eslint-config', 'plugin:react/recommended', 'plugin:react-hooks/recommended', 'plugin:prettier/recommended', - 'prettier' ],extends: [ 'plugin:`@typescript-eslint/recommended`', 'plugin:`@typescript-eslint/eslint-recommended`', '@rocket.chat/eslint-config', 'plugin:prettier/recommended', - 'prettier' ],🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.eslintrc.js around lines 19 - 25, Remove the redundant 'prettier' entry from the top-level extends array and the TypeScript override extends: both already include 'plugin:prettier/recommended' which internally applies eslint-config-prettier, so delete the duplicated 'prettier' string from the extends arrays (look for the top-level extends and the TypeScript override where 'plugin:prettier/recommended' and 'prettier' are listed).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@app/views/CreateChannelView/RoomSettings/SwitchItemEncrypted.test.tsx`:
- Around line 60-61: The test fires the wrong payload and asserts the wrong
behavior: change the fireEvent call in SwitchItemEncrypted.test to pass a plain
boolean (true) to the handler (i.e., fireEvent(component, 'valueChange', true))
and replace the tautological toHaveReturnedWith check with a meaningful call
assertion: assert that the mock onPress handler was called with true (i.e.,
toHaveBeenCalledWith(true)); refer to the test's fireEvent invocation and the
onPressMock / testEncrypted.encrypted references to locate the lines to change.
---
Nitpick comments:
In @.eslintrc.js:
- Around line 19-25: Remove the redundant 'prettier' entry from the top-level
extends array and the TypeScript override extends: both already include
'plugin:prettier/recommended' which internally applies eslint-config-prettier,
so delete the duplicated 'prettier' string from the extends arrays (look for the
top-level extends and the TypeScript override where
'plugin:prettier/recommended' and 'prettier' are listed).
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (5)
app/containers/Avatar/__snapshots__/Avatar.test.tsx.snapis excluded by!**/*.snapapp/containers/markdown/__snapshots__/Markdown.test.tsx.snapis excluded by!**/*.snapapp/containers/message/__snapshots__/Message.test.tsx.snapis excluded by!**/*.snapapp/views/DiscussionsView/__snapshots__/Item.test.tsx.snapis excluded by!**/*.snapyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (39)
.eslintignore.eslintrc.js__mocks__/react-native-mmkv.jsapp/containers/ActionSheet/ActionSheet.tsxapp/containers/AudioPlayer/Seek.tsxapp/containers/Avatar/Avatar.stories.tsxapp/containers/Avatar/AvatarContainer.tsxapp/containers/Avatar/AvatarWithEdit.tsxapp/containers/List/ListRadio.tsxapp/containers/MessageComposer/components/Quotes/Quote.tsxapp/containers/MessageComposer/helpers/getMentionRegexp.test.jsapp/containers/UnreadBadge/getUnreadStyle.test.jsapp/containers/markdown/components/emoji/Emoji.tsxapp/containers/message/Components/Attachments/Reply.tsxapp/ee/omnichannel/lib/index.tsapp/ee/omnichannel/views/QueueListView.tsxapp/lib/database/utils.test.tsapp/lib/encryption/encryption.tsapp/lib/encryption/helpers/base64-js/index.tsapp/lib/encryption/room.tsapp/lib/helpers/formatText.tsapp/lib/hooks/useShortnameToUnicode/index.tsxapp/lib/methods/search.tsapp/lib/methods/serializeAsciiUrl.test.tsapp/sagas/createChannel.jsapp/sagas/createDiscussion.jsapp/sagas/deepLinking.jsapp/sagas/login.jsapp/sagas/rooms.jsapp/sagas/state.jsapp/views/CreateChannelView/RoomSettings/SwitchItemEncrypted.test.tsxapp/views/DiscussionsView/DiscussionDetails.tsxapp/views/DiscussionsView/Item.tsxapp/views/RoomView/Banner.tsxapp/views/SelectServerView.tsxapp/views/ThreadMessagesView/Item.tsxpackage.jsonscripts/build-icon-set.jstsconfig.json
💤 Files with no reviewable changes (1)
- .eslintignore
✅ Files skipped from review due to trivial changes (1)
- app/sagas/createChannel.js
🚧 Files skipped from review as they are similar to previous changes (23)
- app/sagas/state.js
- app/views/RoomView/Banner.tsx
- app/containers/UnreadBadge/getUnreadStyle.test.js
- tsconfig.json
- app/containers/MessageComposer/components/Quotes/Quote.tsx
- app/sagas/deepLinking.js
- app/containers/List/ListRadio.tsx
- app/containers/Avatar/Avatar.stories.tsx
- app/lib/methods/serializeAsciiUrl.test.ts
- app/ee/omnichannel/lib/index.ts
- app/sagas/createDiscussion.js
- app/views/ThreadMessagesView/Item.tsx
- app/containers/markdown/components/emoji/Emoji.tsx
- app/containers/AudioPlayer/Seek.tsx
- app/containers/Avatar/AvatarWithEdit.tsx
- app/lib/encryption/encryption.ts
- app/lib/helpers/formatText.ts
- app/lib/database/utils.test.ts
- app/sagas/login.js
- app/views/DiscussionsView/Item.tsx
- app/views/SelectServerView.tsx
- scripts/build-icon-set.js
- package.json
📜 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). (1)
- GitHub Check: ESLint and Test / run-eslint-and-test
🧰 Additional context used
🧠 Learnings (1)
📚 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:
app/views/CreateChannelView/RoomSettings/SwitchItemEncrypted.test.tsx
🧬 Code graph analysis (3)
app/lib/methods/search.ts (2)
app/lib/encryption/helpers/deferred.ts (1)
reject(36-38)app/lib/methods/helpers/debounce.ts (1)
debounce(3-25)
app/lib/encryption/room.ts (2)
app/definitions/IUpload.ts (1)
TSendFileMessageFileInfo(26-29)app/lib/encryption/definitions.ts (1)
TEncryptFileResult(10-14)
app/ee/omnichannel/views/QueueListView.tsx (1)
app/definitions/IRoom.ts (1)
IOmnichannelRoom(107-143)
🔇 Additional comments (16)
app/ee/omnichannel/views/QueueListView.tsx (1)
70-70: LGTM!Renaming the unused
dataparameter to_datais the correct convention to satisfy the@typescript-eslint/no-unused-varsrule for intentionally-unused parameters. The function body and behavior are unchanged.__mocks__/react-native-mmkv.js (1)
87-87: Stylistic changes look good.All modifications are consistent parentheses removal from single-parameter arrow functions, aligning with the Prettier/ESLint style enforcement described in the PR objectives. No behavioral changes.
Also applies to: 105-105, 113-113, 129-137, 153-161, 180-189
app/lib/encryption/helpers/base64-js/index.ts (1)
47-47: Intentional unused parameter is clearly expressed.Line 47 is a clean, low-risk lint-alignment change; behavior remains unchanged.
app/lib/encryption/room.ts (1)
503-503: Good lint-oriented rename for an intentionally unused argument.Line 503 keeps the API shape while making unused intent explicit; this looks correct.
app/lib/methods/search.ts (1)
141-141: LGTM — correct lint-driven rename.Renaming
resolve→_resolveaccurately signals that the resolve callback is intentionally unused; onlyrejectis captured and assigned todebouncefor the cancellation race. No behavioral change.app/views/CreateChannelView/RoomSettings/SwitchItemEncrypted.test.tsx (1)
59-59:getByTestIdis the right query for this test.Switching from
queryByTestIdtogetByTestIdis correct here — the component is expected to be present, andgetByTestIdwill surface a meaningful assertion failure (vs. anull-reference error later) if the element is missing.app/containers/ActionSheet/ActionSheet.tsx (1)
171-173: LGTM!Converting from
children={data?.children}prop to nested JSX children is idiomatic React and functionally equivalent;data?.childrenbeingundefinedon the initial empty-state still renders nothing correctly.app/containers/Avatar/AvatarContainer.tsx (1)
74-76: TheAvatarcomponent's props interface (IAvatar) already explicitly includeschildren?: React.ReactElement | null;at the interface definition level. The component properly destructures and renders this prop, so the refactor is fully type-safe with no TypeScript errors.app/containers/message/Components/Attachments/Reply.tsx (1)
264-264: LGTM — correct unwrap of single-element style array.Aligns with the newly enforced
react-native/no-single-element-style-arraysrule; no runtime behaviour change.app/lib/hooks/useShortnameToUnicode/index.tsx (1)
36-48: LGTM —_m1prefix correctly marks the unused capture group.app/views/DiscussionsView/DiscussionDetails.tsx (1)
45-45: LGTM — single-element style array correctly unwrapped.app/sagas/rooms.js (1)
21-21: LGTM — cosmetic-only removal of redundant parentheses around single arrow-function parameters throughout the file.app/containers/MessageComposer/helpers/getMentionRegexp.test.js (1)
32-32: LGTM — quote style change to avoid backslash escaping; string value unchanged..eslintrc.js (3)
66-67: LGTM —react-native/no-single-element-style-arraysandreact-native/no-unused-stylesaterrorlevel are well-motivated.These two new rules directly drive the mechanical fixes across
Reply.tsx,DiscussionDetails.tsx, and the other style-array unwraps in this PR.
60-62: No action needed — all three React Compiler rules are properly supported.The installed
eslint-plugin-react-hooksv7.0.1 fully supportsset-state-in-effect,immutability, andrefsrules. These rules are properly defined in the plugin bundle with complete metadata and will function as configured in.eslintrc.jslines 60-62.Likely an incorrect or invalid review comment.
101-107: The ESLint rule configuration withargs: 'all'andargsIgnorePattern: '^_'has been properly implemented throughout the codebase. The example cited in the review—the_m1parameter inuseShortnameToUnicode—is correctly prefixed with an underscore on line 41. No remaining violations of this rule were found across the TypeScript files in the app directory. All unused callback parameters already follow the required pattern.
tassoevan
had a problem deploying
to
approve_e2e_testing
GitHub Actions
Error
tassoevan
had a problem deploying
to
official_android_build
GitHub Actions
Error
tassoevan
had a problem deploying
to
experimental_ios_build
GitHub Actions
Error
tassoevan
had a problem deploying
to
experimental_android_build
GitHub Actions
Error
tassoevan
had a problem deploying
to
official_ios_build
GitHub Actions
Error
tassoevan
requested a deployment
to
approve_e2e_testing
GitHub Actions
Waiting
There was a problem hiding this comment.
🧹 Nitpick comments (1)
app/views/DiscussionsView/Item.tsx (1)
80-81: The ESLint disable comment appears unnecessary.Since
styles.markdownis passed directly as a single style object (not wrapped in an array), thereact-native/no-single-element-style-arraysrule shouldn't trigger. With theStyleProp<TextStyle>typing change inPreview.tsx, the lint suppression can be removed.🧹 Suggested cleanup
- {/* eslint-disable-next-line react-native/no-single-element-style-arrays */} - {username ? <MarkdownPreview msg={makeThreadName(item)} numberOfLines={2} style={styles.markdown} /> : null} + {username ? <MarkdownPreview msg={makeThreadName(item)} numberOfLines={2} style={styles.markdown} /> : null}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/views/DiscussionsView/Item.tsx` around lines 80 - 81, Remove the unnecessary ESLint suppression: delete the comment "// eslint-disable-next-line react-native/no-single-element-style-arrays" before the JSX that renders MarkdownPreview since styles.markdown is passed as a single style object and the StyleProp<TextStyle> change in Preview.tsx already prevents the rule from triggering; update the line that renders MarkdownPreview (the JSX using MarkdownPreview, makeThreadName(item), and styles.markdown) to have no eslint-disable comment.
🤖 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/views/DiscussionsView/Item.tsx`:
- Around line 80-81: Remove the unnecessary ESLint suppression: delete the
comment "// eslint-disable-next-line
react-native/no-single-element-style-arrays" before the JSX that renders
MarkdownPreview since styles.markdown is passed as a single style object and the
StyleProp<TextStyle> change in Preview.tsx already prevents the rule from
triggering; update the line that renders MarkdownPreview (the JSX using
MarkdownPreview, makeThreadName(item), and styles.markdown) to have no
eslint-disable comment.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/containers/markdown/components/Preview.tsxapp/views/DiscussionsView/Item.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). (1)
- GitHub Check: ESLint and Test / run-eslint-and-test
🧰 Additional context used
🧬 Code graph analysis (2)
app/views/DiscussionsView/Item.tsx (2)
app/lib/methods/helpers/room.ts (1)
makeThreadName(69-70)app/sagas/deepLinking.js (1)
item(71-76)
app/containers/markdown/components/Preview.tsx (1)
app/lib/constants/colors.ts (1)
themes(304-304)
🔇 Additional comments (1)
app/containers/markdown/components/Preview.tsx (1)
2-31: LGTM!Good improvement to use
StyleProp<TextStyle>— the idiomatic React Native type for style props. The conditional spread logic correctly handles both single style objects and arrays, and the default empty array maintains backward compatibility.
tassoevan
requested a deployment
to
official_android_build
GitHub Actions
Waiting
tassoevan
requested a deployment
to
experimental_ios_build
GitHub Actions
Waiting
tassoevan
requested a deployment
to
official_ios_build
GitHub Actions
Waiting
tassoevan
requested a deployment
to
experimental_android_build
GitHub Actions
Waiting
2 participants
Proposed changes
It reviews lint rules to aim consistency.
Issue(s)
Task: NATIVE-49
How to test or reproduce
Screenshots
Types of changes
Checklist
Further comments
This is one in a sequence of incoming suggested changes.
Summary by CodeRabbit
Chores
Style / UI
Tests