fix: Deeplink auth when host is already connected#7358
Conversation
yash-rajpal
temporarily deployed
to
approve_e2e_testing
GitHub Actions
Inactive
WalkthroughThis PR preserves ChangesDeep-linking Optimization
Sequence Diagram(s)sequenceDiagram
participant DeepLinkSaga
participant SDKClient as sdk.current.client
participant Redux as ReduxStore
participant EventEmitter
DeepLinkSaga->>SDKClient: read host (sdk.current?.client?.host)
alt hostAlreadyConnected == false
DeepLinkSaga->>Redux: dispatch APP.START(ROOT_OUTSIDE)
DeepLinkSaga->>Redux: dispatch serverInitAdd(server)
DeepLinkSaga->>DeepLinkSaga: delay
DeepLinkSaga->>EventEmitter: emit NewServer
DeepLinkSaga->>Redux: wait SERVER.SELECT_SUCCESS
else hostAlreadyConnected == true
DeepLinkSaga->>DeepLinkSaga: skip server init and SELECT_SUCCESS wait
end
DeepLinkSaga->>Redux: dispatch loginRequest(resumeToken)
Redux->>DeepLinkSaga: LOGIN.SUCCESS
DeepLinkSaga->>Redux: dispatch APP.START(ROOT_INSIDE)
DeepLinkSaga->>DeepLinkSaga: call goRoom
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
|
Actionable comments posted: 0 |
yash-rajpal
requested a deployment
to
approve_e2e_testing
GitHub Actions
Waiting
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
app/sagas/__tests__/deepLinking.test.ts (1)
435-452: 💤 Low valueRestore the
emitspy inafterEachso it can't leak on assertion failure.If the assertion at Line 442 or Line 450 throws, Line 451 (
emitSpy.mockRestore()) is never reached, leavingEventEmitter.emitspied for any subsequent test. Restoring inafterEach(or relying onrestoreMocks) guarantees cleanup regardless of test outcome.♻️ Move restore to afterEach
afterEach(() => { jest.useRealTimers(); + jest.restoreAllMocks(); // Reset so other describe blocks see the default empty host (sdk.current as any).client.host = ''; });- expect(jest.mocked(goRoom)).toHaveBeenCalledTimes(1); - emitSpy.mockRestore(); + expect(jest.mocked(goRoom)).toHaveBeenCalledTimes(1);🤖 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/sagas/__tests__/deepLinking.test.ts` around lines 435 - 452, The test creates a spy via emitSpy = jest.spyOn(EventEmitter, 'emit') inside a test but only calls emitSpy.mockRestore() at the end of the test, which can leak if an assertion fails; move the restore into a global afterEach so cleanup always runs: declare emitSpy in the test file scope (or ensure access) and in an afterEach() call emitSpy?.mockRestore() (or use jest.restoreAllMocks()) to unspy EventEmitter.emit; reference the emitSpy variable and the EventEmitter.emit spy usage in deepLinking.test.ts to apply the change.
🤖 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/sagas/__tests__/deepLinking.test.ts`:
- Around line 435-452: The test creates a spy via emitSpy =
jest.spyOn(EventEmitter, 'emit') inside a test but only calls
emitSpy.mockRestore() at the end of the test, which can leak if an assertion
fails; move the restore into a global afterEach so cleanup always runs: declare
emitSpy in the test file scope (or ensure access) and in an afterEach() call
emitSpy?.mockRestore() (or use jest.restoreAllMocks()) to unspy
EventEmitter.emit; reference the emitSpy variable and the EventEmitter.emit spy
usage in deepLinking.test.ts to apply the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 67f1c609-338f-48b9-b49b-7bf0950ef76b
📒 Files selected for processing (1)
app/sagas/__tests__/deepLinking.test.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 (4)
**/*.{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/sagas/__tests__/deepLinking.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 numbersUse TypeScript with strict mode and baseUrl set to app/ for import resolution
Files:
app/sagas/__tests__/deepLinking.test.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx,js,jsx}: Use Prettier with tabs, single quotes, 130 char width, no trailing commas, arrow parens avoid, bracket same line
Use@rocket.chat/eslint-configbase with React, React Native, TypeScript, Jest plugins
Files:
app/sagas/__tests__/deepLinking.test.ts
app/sagas/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Sagas for side effects should be placed in app/sagas/ directory (init, login, rooms, messages, encryption, deepLinking, videoConf)
Files:
app/sagas/__tests__/deepLinking.test.ts
🧠 Learnings (1)
📚 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/sagas/__tests__/deepLinking.test.ts
🔇 Additional comments (3)
app/sagas/__tests__/deepLinking.test.ts (3)
100-101: LGTM!
388-396: LGTM!
403-427: LGTM!
yash-rajpal
requested a deployment
to
android_build
GitHub Actions
Waiting
2 participants
Proposed changes
During deeplink authentication, the deeplink saga does not check whether the server is already connected. Instead, it always attempts to add the server as a new one.
This triggers the
selectServersaga. Since the server is already connected,selectServerredirects the user directly into the app without performing login and dispatchesSERVER.SELECT_CANCEL.However, the deeplink saga is waiting for a
SERVER.SELECT_SUCCESSaction, which is never dispatched in this scenario, causing the deeplink authentication flow to break.This PR fixes this by checking if server is already connected in deeplink saga, before attempting to add the server. If server is already connected it continues the login flow without trying to add new server or wait for
SERVER.SELECT_SUCCESSIssue(s)
How to test or reproduce
Screenshots
Types of changes
Further comments
PRM-49
Summary by CodeRabbit
Bug Fixes
Tests