fix: resolve deep links by room id for channels and groups - #7111
fix: resolve deep links by room id for channels and groups#7111Rohit3523 wants to merge 24 commits into
Conversation
GitHub Actions
Error
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
Walkthrough
ChangesGroup deeplink behavior
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Deeplink
participant canOpenRoom
participant getRoomByTypeAndName
participant groups.open
Deeplink->>canOpenRoom: Provide channel or group path
canOpenRoom->>getRoomByTypeAndName: Resolve room by name or ID
getRoomByTypeAndName-->>canOpenRoom: Return resolved room
canOpenRoom->>groups.open: Open group by room ID
groups.open-->>canOpenRoom: Return success or already-open error
canOpenRoom-->>Deeplink: Return resolved room or false
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
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 |
GitHub Actions
Error
GitHub Actions
Error
GitHub Actions
Error
GitHub Actions
Error
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
app/lib/methods/canOpenRoom.ts (2)
45-54: Potential redundant API call for GROUP type.For
ERoomTypes.GROUPwithoutrid, the code now:
- Calls
getRoomByTypeAndName('p', name)at line 32- Then calls
groups.infowith{ roomName: name }at line 48This results in two API calls to fetch room information. Since you already have
result._idfrom line 32, consider reusing that data or passingroomIdto the info endpoint to avoid the redundant call.♻️ Suggested approach
// if it's a group we need to check if you can open if (type === ERoomTypes.GROUP) { try { const result = await getRoomByTypeAndName('p', name); // RC 0.61.0 // `@ts-ignore` await sdk.post(`${restTypes[type]}.open`, { roomId: result._id }); + // Return room info directly since we already have it + if (!rid) { + return { + ...result, + rid: result._id + }; + } } catch (e: any) { if (!(e.data && /is already open/.test(e.data.error))) { return false; } + // Room is already open, still need to fetch info if no rid } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/lib/methods/canOpenRoom.ts` around lines 45 - 54, The GROUP branch is making a redundant API call: you already fetch the group via getRoomByTypeAndName('p', name) (result._id) earlier, then call groups.info when rid is missing; update canOpenRoom to reuse the previously obtained room object or pass the roomId to the info endpoint instead of calling groups.info with roomName—specifically, modify the logic around getRoomByTypeAndName and the block handling ERoomTypes.GROUP so that if you have result._id (or a room object), you set room.rid = result._id and return that room directly (or call groups.info with { roomId: result._id } if more details are required), eliminating the extra groups.info call.
36-40: Error handling may mask failures fromgetRoomByTypeAndName.If
getRoomByTypeAndNamefails for reasons other than "room is already open" (e.g., room not found, network error), the code returnsfalseat line 38. This is likely correct behavior, but the error condition at line 37 only checks for the "already open" case fromsdk.post, not fromgetRoomByTypeAndName.If
getRoomByTypeAndNamethrows an error (e.g., room not found by name/ID), it will hit this catch block and returnfalse. This may be intentional, but consider whether a more specific error should be propagated or logged for debugging deeplink failures.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/lib/methods/canOpenRoom.ts` around lines 36 - 40, The catch currently around both getRoomByTypeAndName and sdk.post can swallow errors from getRoomByTypeAndName; split the error handling so getRoomByTypeAndName failures are not mistaken for the "already open" sdk.post case. Specifically, call getRoomByTypeAndName (the function) in its own try/catch and either propagate or log/return a distinct error for failures, then wrap only the sdk.post call in a try/catch that checks e.data && /is already open/ to return false; rethrow or surface other unexpected errors instead of returning false.
🤖 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/methods/canOpenRoom.ts`:
- Around line 45-54: The GROUP branch is making a redundant API call: you
already fetch the group via getRoomByTypeAndName('p', name) (result._id)
earlier, then call groups.info when rid is missing; update canOpenRoom to reuse
the previously obtained room object or pass the roomId to the info endpoint
instead of calling groups.info with roomName—specifically, modify the logic
around getRoomByTypeAndName and the block handling ERoomTypes.GROUP so that if
you have result._id (or a room object), you set room.rid = result._id and return
that room directly (or call groups.info with { roomId: result._id } if more
details are required), eliminating the extra groups.info call.
- Around line 36-40: The catch currently around both getRoomByTypeAndName and
sdk.post can swallow errors from getRoomByTypeAndName; split the error handling
so getRoomByTypeAndName failures are not mistaken for the "already open"
sdk.post case. Specifically, call getRoomByTypeAndName (the function) in its own
try/catch and either propagate or log/return a distinct error for failures, then
wrap only the sdk.post call in a try/catch that checks e.data && /is already
open/ to return false; rethrow or surface other unexpected errors instead of
returning false.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 43a644fe-331b-49fc-b446-7d4e57c30d05
📒 Files selected for processing (1)
app/lib/methods/canOpenRoom.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{js,jsx,ts,tsx,json}
📄 CodeRabbit inference engine (CLAUDE.md)
Configure Prettier with tabs, single quotes, 130 character width, no trailing commas, arrow parens avoid, and bracket same line
Files:
app/lib/methods/canOpenRoom.ts
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use ESLint with
@rocket.chat/eslint-configbase configuration including React, React Native, TypeScript, and Jest plugins
Files:
app/lib/methods/canOpenRoom.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use TypeScript with strict mode enabled and configure baseUrl to app/ for import resolution
**/*.{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 numbers
Files:
app/lib/methods/canOpenRoom.ts
**/*.{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/lib/methods/canOpenRoom.ts
🧠 Learnings (1)
📚 Learning: 2026-04-07T17:49:17.519Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat.ReactNative PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-07T17:49:17.519Z
Learning: Applies to app/lib/database/model/**/*.{ts,tsx} : Place database models in app/lib/database/model/ for entities like Message, Room, Subscription, User, Thread, Upload, Server, CustomEmoji, Permission, and Role
Applied to files:
app/lib/methods/canOpenRoom.ts
🔇 Additional comments (1)
app/lib/methods/canOpenRoom.ts (1)
30-41: The fix may not fully address the PR objective whennameis actually a group ID.According to the PR objectives, the deeplink can contain either a group name or a group ID. The current implementation still passes
namedirectly togetRoomByTypeAndName('p', name). Ifnameis actually an ID (not a room name), this call might fail depending on how the API handles the parameter.Additionally, the magic string
'p'for the room type could benefit from a brief comment or constant.[raise_major_issue, request_verification]
#!/bin/bash # Description: Check how getRoomByTypeAndName is implemented and whether it can accept both IDs and names # Find the implementation of getRoomByTypeAndName ast-grep --pattern 'export function getRoomByTypeAndName($$$) { $$$ }' # Also search for its definition in restApi rg -n -A 20 'getRoomByTypeAndName' --type ts
GitHub Actions
Error
GitHub Actions
Error
GitHub Actions
Error
GitHub Actions
Error
GitHub Actions
Error
GitHub Actions
Error
GitHub Actions
Error
GitHub Actions
Error
GitHub Actions
Error
GitHub Actions
Error
GitHub Actions
Error
GitHub Actions
Error
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
app/lib/methods/canOpenRoom.ts (2)
51-60:⚠️ Potential issue | 🟡 MinorNarrowing the fallback to
CHANNELis fine, but couple it with the regression fix above.Removing
ERoomTypes.GROUPfrom this fallback is reasonable now that GROUP has its own dedicated block, but it amplifies the “is already open” regression flagged at lines 30–47: there is no longer a safety net that callsgroups.infofor GROUP. Once the GROUP block is fixed to always return the resolved room, this narrowing is consistent.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/lib/methods/canOpenRoom.ts` around lines 51 - 60, The narrowing of the fallback to ERoomTypes.CHANNEL removed the safety-net for GROUP; update canOpenRoom.ts so the GROUP path mirrors the CHANNEL fallback: when type === ERoomTypes.GROUP and no rid, call sdk.get(`${restTypes[type]}.info`, params) (same as the CHANNEL block), extract the room from result[type], set room.rid = room._id and return the room; also ensure the primary GROUP handling block (the earlier regression) always returns the resolved room object so the new fallback is consistent with the fixed GROUP logic.
30-47:⚠️ Potential issue | 🔴 CriticalRegression: room data is lost when group is already open.
When
sdk.post('groups.open', …)throws the “is already open” error, control jumps to thecatchblock whereresponse(declared withconstinside thetry) is out of scope, so the fetched room cannot be returned. Execution then falls through to the lines below — and because theCHANNEL-only fallback at line 51 no longer coversGROUP,open()ends up returningfalse, leaving the user on the room list. Previously, the${restTypes[type]}.infofallback handled this case for groups.🐛 Suggested fix — return the resolved room even when the open call reports it was already open
// if it's a group we need to check if you can open if (type === ERoomTypes.GROUP) { + let response; try { - const response = await getRoomByTypeAndName('p', name); + response = await getRoomByTypeAndName('p', name); // RC 0.61.0 // `@ts-ignore` await sdk.post('groups.open', { roomId: response._id }); - - return { - ...response, - rid: response._id - }; } catch (e: any) { - console.log('e', e); if (!(e.data && /is already open/.test(e.data.error))) { return false; } } + if (response) { + return { + ...response, + rid: response._id + }; + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/lib/methods/canOpenRoom.ts` around lines 30 - 47, Regression: when sdk.post('groups.open', ...) throws the "is already open" error the const response is out of scope in the catch, so the room data is lost and false is returned. Fix by hoisting response (declare let response outside the try), assign it via getRoomByTypeAndName(...) inside the try, and in the catch detect the "is already open" case (e.data && /is already open/.test(e.data.error)) and return the resolved room (e.g., return { ...response, rid: response._id }); also remove/replace the debugging console.log; referenced symbols: getRoomByTypeAndName, sdk.post('groups.open'), response, and the catch block in canOpenRoom.ts.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.maestro/tests/assorted/group-deeplink.yaml:
- Around line 21-23: The test uses a flat selector for the extendedWaitUntil
step—change the visible selector from the string form to the nested testID form
used elsewhere: replace extendedWaitUntil.visible: 'discussion-with-name' with a
nested object using id: 'discussion-with-name' (i.e., extendedWaitUntil.visible
-> id) for both occurrences; update the same pattern for any other
extendedWaitUntil.visible entries to maintain consistency with the project's
testID matching convention and ensure the selector uses visible: { id:
'discussion-with-name' } under the extendedWaitUntil block.
- Around line 15-30: Fix the duplicated comment and ensure the first test
exercises name resolution: change the second block's comment from "# open group
using name" to "# open group using id" to match the assertion expecting
'discussion-with-id', and verify the first deeplink env.link value ('link:
'https://go.rocket.chat/room?host=mobile.qa.rocket.chat&path=group/4t6Mw3K4M9JLeHuCH'')
is a human-readable room name used for name-resolution; if that token is
actually a Meteor-style room ID, replace it with a real human-readable group
slug (e.g., 'private-deeplink-test') so the runFlow (file 'open-deeplink.yaml')
plus the 'discussion-with-name' assertion exercises the name resolution path
while the second block plus 'discussion-with-id' exercises the ID path.
In `@app/lib/methods/canOpenRoom.ts`:
- Line 42: In canOpenRoom.ts inside the canOpenRoom function remove the debug
console.log('e', e) statement; instead either propagate the error or log it via
the module's standard logger (do not leave a raw console.log). Locate the catch
block referencing the variable e, delete the console.log line and replace with a
call to the existing logging/error handling utility used in this module (or
rethrow e) so production logs won't be spammed or leak error payloads.
- Around line 30-40: The code in canOpenRoom (inside the ERoomTypes.GROUP
branch) uses response._id without validating that getRoomByTypeAndName returned
a value; update the logic in that block (the call to getRoomByTypeAndName and
subsequent sdk.post('groups.open', { roomId: response._id })) to explicitly
check that response and response._id exist (e.g., if (!response?._id) return
false) before calling sdk.post and before returning the room object, and keep
the existing try/catch for sdk.post errors so only post-call errors are handled
there; ensure you reference getRoomByTypeAndName and sdk.post('groups.open')
when making the change.
---
Outside diff comments:
In `@app/lib/methods/canOpenRoom.ts`:
- Around line 51-60: The narrowing of the fallback to ERoomTypes.CHANNEL removed
the safety-net for GROUP; update canOpenRoom.ts so the GROUP path mirrors the
CHANNEL fallback: when type === ERoomTypes.GROUP and no rid, call
sdk.get(`${restTypes[type]}.info`, params) (same as the CHANNEL block), extract
the room from result[type], set room.rid = room._id and return the room; also
ensure the primary GROUP handling block (the earlier regression) always returns
the resolved room object so the new fallback is consistent with the fixed GROUP
logic.
- Around line 30-47: Regression: when sdk.post('groups.open', ...) throws the
"is already open" error the const response is out of scope in the catch, so the
room data is lost and false is returned. Fix by hoisting response (declare let
response outside the try), assign it via getRoomByTypeAndName(...) inside the
try, and in the catch detect the "is already open" case (e.data && /is already
open/.test(e.data.error)) and return the resolved room (e.g., return {
...response, rid: response._id }); also remove/replace the debugging
console.log; referenced symbols: getRoomByTypeAndName, sdk.post('groups.open'),
response, and the catch block in canOpenRoom.ts.
🪄 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: 8a2b3530-2a29-47ee-b688-5b7ad497d1c4
📒 Files selected for processing (2)
.maestro/tests/assorted/group-deeplink.yamlapp/lib/methods/canOpenRoom.ts
📜 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
📓 Path-based instructions (3)
**/*.{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/lib/methods/canOpenRoom.ts
**/*.{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 numbers
**/*.{ts,tsx}: Use TypeScript with strict mode enabled and baseUrl set to app/ for module imports
Support iOS 13.4+ and Android 6.0+ as minimum target platforms
Files:
app/lib/methods/canOpenRoom.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx,js,jsx}: Use tabs for indentation with single quotes, 130 character line width, no trailing commas, and avoid arrow function parentheses when possible
Use ESLint with@rocket.chat/eslint-configbase including React, React Native, TypeScript, and Jest plugins
Files:
app/lib/methods/canOpenRoom.ts
🧠 Learnings (2)
📚 Learning: 2026-03-05T14:28:10.004Z
Learnt from: Rohit3523
Repo: RocketChat/Rocket.Chat.ReactNative PR: 6997
File: .maestro/tests/room/message-markdown-click.yaml:28-39
Timestamp: 2026-03-05T14:28:10.004Z
Learning: In Maestro YAML selector fields (text, id) within the Rocket.Chat React Native repository, use the contains pattern '.*keyword.*' (leading and trailing '.*') for matching text. The pattern '.*keyword*.' is incorrect and will fail to match cases where the keyword appears at the end of the element's text. This guideline applies to all Maestro YAML selector fields across the codebase.
Applied to files:
.maestro/tests/assorted/group-deeplink.yaml
📚 Learning: 2026-04-22T22:57:58.545Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat.ReactNative PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-22T22:57:58.545Z
Learning: Applies to app/lib/database/model/**/*.{ts,tsx} : Define WatermelonDB models in app/lib/database/model/ directory for Message, Room, Subscription, User, Thread, Upload, Server, CustomEmoji, Permission, Role, etc.
Applied to files:
app/lib/methods/canOpenRoom.ts
🔇 Additional comments (1)
.maestro/tests/assorted/group-deeplink.yaml (1)
9-13: Verify thatopen-deeplink.yamlproperly launches the app from cold-start afterkillApp.The pattern you identified is correct:
login-with-deeplink.yamlruns once for the suite, then each scenario doeskillAppfollowed byopen-deeplink.yaml. However,open-deeplink.yamluses onlyopenLink: ${link}without explicit app restart logic—confirm that this properly relaunches the app with the deeplink (rather than assuming a warm session). If the auth tokens don't persist through the kill-restart cycle or the deeplink handler doesn't activate on cold-start, the second scenario could silently fail to reach an authenticated state, and the 60000ms wait would mask the race condition. Local testing with the app fully killed is necessary to validate.
GitHub Actions
Error
GitHub Actions
Error
GitHub Actions
Error
GitHub Actions
Error
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
GitHub Actions
Error
There was a problem hiding this comment.
🧹 Nitpick comments (1)
app/lib/methods/canOpenRoom.test.ts (1)
26-35: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffAdd explicit return types to the new async test callbacks.
Use
Promise<void>for each added asyncitcallback. Apply the same rule to the other added callback functions where applicable.Proposed change
- it('returns room when getRoomByTypeAndName succeeds and groups.open succeeds', async () => { + it('returns room when getRoomByTypeAndName succeeds and groups.open succeeds', async (): Promise<void> => {As per coding guidelines, “add explicit type annotations to function parameters and return types.”
🤖 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/lib/methods/canOpenRoom.test.ts` around lines 26 - 35, Add an explicit Promise<void> return type to the async callback in the canOpenRoom test case, and apply the same annotation to every other newly added async it/test callback in this diff. Keep the test behavior and assertions unchanged.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.
Nitpick comments:
In `@app/lib/methods/canOpenRoom.test.ts`:
- Around line 26-35: Add an explicit Promise<void> return type to the async
callback in the canOpenRoom test case, and apply the same annotation to every
other newly added async it/test callback in this diff. Keep the test behavior
and assertions unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 5df86ac2-5784-493b-9aae-220d7917cf72
📒 Files selected for processing (4)
.maestro/tests/assorted/group-deeplink.yamlapp/definitions/rest/v1/groups.tsapp/lib/methods/canOpenRoom.test.tsapp/lib/methods/canOpenRoom.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- .maestro/tests/assorted/group-deeplink.yaml
📜 Review details
⏰ Context from checks skipped due to timeout. (3)
- GitHub Check: ESLint and Test / run-eslint-and-test
- GitHub Check: E2E Shard Preflight
- GitHub Check: format
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{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/definitions/rest/v1/groups.tsapp/lib/methods/canOpenRoom.tsapp/lib/methods/canOpenRoom.test.ts
**/*.{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 numbers
Files:
app/definitions/rest/v1/groups.tsapp/lib/methods/canOpenRoom.tsapp/lib/methods/canOpenRoom.test.ts
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{js,jsx,ts,tsx}: Format JavaScript and TypeScript code with Oxfmt using the repository configuration: tabs, single quotes, 130-character width, no trailing commas, omitted arrow-function parentheses where possible, and same-line brackets.
Follow Oxlint rules configured in.oxlintrc.json, including the import, React, Jest, TypeScript, and React Native plugins.
Files:
app/definitions/rest/v1/groups.tsapp/lib/methods/canOpenRoom.tsapp/lib/methods/canOpenRoom.test.ts
🧠 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/definitions/rest/v1/groups.tsapp/lib/methods/canOpenRoom.tsapp/lib/methods/canOpenRoom.test.ts
📚 Learning: 2026-06-25T18:37:25.526Z
Learnt from: diegolmello
Repo: RocketChat/Rocket.Chat.ReactNative PR: 7434
File: app/views/ScreenLockConfigView.test.tsx:16-22
Timestamp: 2026-06-25T18:37:25.526Z
Learning: In Rocket.Chat ReactNative tests that mock selectors for `useAppSelector`, don’t require the mocked selector input to be typed as `IApplicationState` when the fixture only includes a partial Redux state slice (e.g., only `server` and `settings`). Requiring the full `IApplicationState` type in that scenario forces unsafe `as IApplicationState` casts and undermines type-safety. For these narrowly scoped selector-mock fixtures, use a less strict type (e.g., `any`) to keep the mock focused on the slice under test.
Applied to files:
app/lib/methods/canOpenRoom.test.ts
🔇 Additional comments (2)
app/definitions/rest/v1/groups.ts (1)
93-97: LGTM!app/lib/methods/canOpenRoom.ts (1)
5-5: LGTM!Also applies to: 22-48
Rohit3523
changed the title
Adding Promise to every it() callback in canOpenRoom.test.ts would deviate from the surrounding test suite and add noise with no lint backing. |
GitHub Actions
Error
GitHub Actions
Waiting
OtavioStasiak
left a comment
OtavioStasiak
left a comment
There was a problem hiding this comment.
lint is failing after update with develop...
Fix it and run e2e tests, all tests must pass.
|
|
||
| # open group using name | ||
| - killApp | ||
| - runFlow: |
There was a problem hiding this comment.
The flow hardcodes four things that data-setup.js never creates: the room names discussion-with-name / discussion-with-id and the ids 6a74cb92d09f52225dd8f0af / 8Lhm98suz3bGKMuK2. They only resolve because someone created them by
hand on mobile.qa.rocket.chat — the group id is a 24-char Mongo ObjectId while RC room ids are 17-char randoms (8Lhm98suz3bGKMuK2), which gives it away. When that server is reset or those rooms are renamed, this flow fails and the
failure will read as a product regression in deeplinking rather than missing test data.
data-setup.js already exposes exactly what this needs — createRandomRoom(username, password, type) returns { _id, name } — so both the name case and the id case can provision themselves, following the pattern in
tests/room/discussion.yaml:
onFlowStart:
- runFlow: '../../helpers/setup.yaml'
onFlowComplete:
- evalScript: ${output.utils.deleteCreatedUsers()}
- evalScript: ${output.user = output.utils.createUser()}
- evalScript: ${output.group = output.utils.createRandomRoom(output.user.username, output.user.password, 'p')}
- evalScript: ${output.channel = output.utils.createRandomRoom(output.user.username, output.user.password, 'c')}
then path=group/${output.group.name} and path=group/${output.group._id}, asserting room-view-title-${output.group.name} in both cases. Same for the channel. That also drops the dependency on the shared admin account, which the four
killApp + deeplink steps currently mutate state for.
Two cases that are closer to the bug this PR fixes and aren't covered:
- A private group with a closed subscription. All four rooms are already open for the admin, so groups.open in canOpenRoom never actually does anything here — the one branch the fix reorders (resolve room → then open, instead of
open → then fetch info) is untested end-to-end. output.utils.post('rooms.hide', user, pass, { roomId }) before the deeplink makes it real. - A public channel the user is not a member of — the new code resolves it via canAccessRoomAsync rather than channels.info + view-c-room, and that's a genuine behavior difference worth pinning.
Non-blocking, but (1) is the one I'd push for, since it's the branch the fix actually restructures.
2 participants
Proposed changes
Deep links to a room sometimes carry a room id in the path segment instead of a room name (e.g.
rocketchat://room?host=...&path=group/6997e23f362b278aeb3d369b).The previous implementation passed that path segment straight to the REST
groups.info/channels.infoendpoint asroomName. Those endpoints only ever match a room name exactly, so whenever the link contained an id for a group, the lookup returned "not found" and the deep link failed to open the room.This PR makes room resolution for both
groupandchanneldeep links use thegetRoomByTypeAndNamemethod, which resolves both a room name and a room id — with no code anymore tied to "name-only" REST params.Issue(s)
https://rocketchat.atlassian.net/browse/CORE-1857
How to test or reproduce
path=group/<id>) → the group now opens (previously failed).path=channel/<name|id>→ the channel opens in both cases.Screenshots
N/A — deep-link behavior change, no visual change.
Types of changes
Checklist
Further comments
The previous implementation used two different REST endpoints (
channels.infofor channels,groups.infofor groups) with aroomNameparam, so it silently missed id-based links. We consolidated resolution on the DDPgetRoomByTypeAndName, which the server supports for bothcandproom types, matching here also used by the in-app Directory. A regression test was added and the e2e coverage was adjusted to fixed reusable fixtures (old group fixtures no longer exist on the QA server).Summary by CodeRabbit
New Features
Bug Fixes
Tests