test: cover handleClickCallPush + handleNavigateCallRoom in deep linking saga#7308
test: cover handleClickCallPush + handleNavigateCallRoom in deep linking saga#7308diegolmello wants to merge 4 commits into
Conversation
…stack When a deep link with userId/token/path lands on an unknown server, the saga waited for LOGIN.SUCCESS and then immediately called goRoom while app.root was still ROOT_OUTSIDE. The login saga's appStart(ROOT_INSIDE) would only fire later inside handleLoginSuccess, after the deeplink saga had already dispatched navigation into the still-mounted OutsideStack — losing it when InsideStack later mounted at its default RoomsListView. Wait for the APP.START with root=ROOT_INSIDE before navigating so that goRoom dispatches into the correct stack.
Establishes integration-style test harness for app/sagas/deepLinking.js with boundary mocks, fake timers, and manual cross-saga dispatch. Locks down the take-based ordering between LOGIN.SUCCESS and APP.START(ROOT_INSIDE) that PR #7304 introduced.
…ing saga New isolated test file for the video-conference push deep-link entry point. Covers all server-resolution branches (same/known/unknown) and the downstream subscription-lookup + accept/decline matrix.
WalkthroughThis PR adds a comprehensive Jest/Redux-saga test suite for the deepLinking saga's video conference functionality, covering empty host short-circuits, same/known/unknown server authentication flows, resume-login behavior, and downstream room navigation with notifications and error cases. ChangesVideo Conference Deep Linking Test Suite
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
app/sagas/__tests__/deepLinking.videoConf.test.ts (2)
174-182: ⚡ Quick winExtract repeated mock-reset block into a helper.
The same
jest.mocked(X).mockReset()chain (plusmockSubsCollection.find.mockReset()) is duplicated almost verbatim inbeforeEachof Groups A–E (lines 174-182, 201-211, 278-287, 335-344, 456-465). A single helper avoids drift (e.g., Group A omitslocalAuthenticate.mockReset()) and shortens the file.♻️ Suggested helper
+function resetAllMocks() { + jest.mocked(UserPreferences.getString).mockReset(); + jest.mocked(getServerById).mockReset(); + jest.mocked(getServerInfo).mockReset(); + jest.mocked(localAuthenticate).mockReset(); + jest.mocked(navigateToRoom).mockReset(); + jest.mocked(notifyUser).mockReset(); + jest.mocked(videoConfJoin).mockReset(); + mockSubsCollection.find.mockReset(); +}Then each
beforeEachbecomes:beforeEach(() => { - jest.mocked(UserPreferences.getString).mockReset(); - jest.mocked(getServerById).mockReset(); - jest.mocked(getServerInfo).mockReset(); - jest.mocked(navigateToRoom).mockReset(); - jest.mocked(notifyUser).mockReset(); - jest.mocked(videoConfJoin).mockReset(); - mockSubsCollection.find.mockReset(); + resetAllMocks(); // ...test-specific setup });🤖 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.videoConf.test.ts` around lines 174 - 182, Extract the repeated mock-reset chain into a single helper (e.g., function resetCommonMocks()) that calls jest.mocked(UserPreferences.getString).mockReset(), jest.mocked(getServerById).mockReset(), jest.mocked(getServerInfo).mockReset(), jest.mocked(navigateToRoom).mockReset(), jest.mocked(notifyUser).mockReset(), jest.mocked(videoConfJoin).mockReset(), and mockSubsCollection.find.mockReset() (and add localAuthenticate.mockReset() if omitted) and replace each beforeEach block in the test groups with a call to resetCommonMocks(); this consolidates the duplicated resets and ensures all groups reset the same set of mocks consistently.
126-136: 💤 Low valueType the collector middleware instead of
any.Per the project's TypeScript guideline (explicit type annotations), the collector middleware can use Redux's
Middleware/Dispatchtypes rather thanany. This also gives autocomplete inside tests.♻️ Suggested typing
-import { AnyAction, applyMiddleware, createStore } from 'redux'; +import { AnyAction, Dispatch, Middleware, applyMiddleware, createStore } from 'redux'; @@ -function setupStore(preloadedState?: PreloadedState) { - const dispatched: AnyAction[] = []; - const collectorMiddleware = () => (next: any) => (action: any) => { - dispatched.push(action); - return next(action); - }; +function setupStore(preloadedState?: PreloadedState) { + const dispatched: AnyAction[] = []; + const collectorMiddleware: Middleware = () => (next: Dispatch<AnyAction>) => (action: AnyAction) => { + dispatched.push(action); + return next(action); + };As per coding guidelines: "Use TypeScript for type safety; 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/sagas/__tests__/deepLinking.videoConf.test.ts` around lines 126 - 136, The collector middleware in setupStore is currently typed with any; replace those any types with proper Redux types to satisfy TypeScript guidelines: type dispatched as AnyAction[], annotate collectorMiddleware using Redux's Middleware (and Dispatch where needed) so the signature becomes a Middleware that captures actions into dispatched, and annotate next/action parameters with Dispatch/AnyAction types; keep sagaMiddleware.run(deepLinkingRoot) and the returned { store, dispatched } untouched.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/sagas/__tests__/deepLinking.videoConf.test.ts`:
- Around line 495-499: The test's ordering check is a no-op: computing
rootInsideIdx after finding rootInsideAction only asserts it exists, not that it
occurred before navigateToRoom; update the test to assert ordering by comparing
positions in dispatched against navigateToRoom's invocation. For example,
capture the dispatched array (or its length) at the moment
jest.mocked(navigateToRoom) is invoked or get the index of the
navigateToRoom-related action/side-effect and assert
dispatched.indexOf(rootInsideAction!) < indexOfNavigate; modify the assertions
around rootInsideAction, dispatched and jest.mocked(navigateToRoom) so you
explicitly verify appStart(ROOT_INSIDE) appears before navigateToRoom was
called.
---
Nitpick comments:
In `@app/sagas/__tests__/deepLinking.videoConf.test.ts`:
- Around line 174-182: Extract the repeated mock-reset chain into a single
helper (e.g., function resetCommonMocks()) that calls
jest.mocked(UserPreferences.getString).mockReset(),
jest.mocked(getServerById).mockReset(), jest.mocked(getServerInfo).mockReset(),
jest.mocked(navigateToRoom).mockReset(), jest.mocked(notifyUser).mockReset(),
jest.mocked(videoConfJoin).mockReset(), and mockSubsCollection.find.mockReset()
(and add localAuthenticate.mockReset() if omitted) and replace each beforeEach
block in the test groups with a call to resetCommonMocks(); this consolidates
the duplicated resets and ensures all groups reset the same set of mocks
consistently.
- Around line 126-136: The collector middleware in setupStore is currently typed
with any; replace those any types with proper Redux types to satisfy TypeScript
guidelines: type dispatched as AnyAction[], annotate collectorMiddleware using
Redux's Middleware (and Dispatch where needed) so the signature becomes a
Middleware that captures actions into dispatched, and annotate next/action
parameters with Dispatch/AnyAction types; keep
sagaMiddleware.run(deepLinkingRoot) and the returned { store, dispatched }
untouched.
🪄 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: 17e99c17-49cf-4621-9c94-01692ed3853d
📒 Files selected for processing (1)
app/sagas/__tests__/deepLinking.videoConf.test.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{js,ts,jsx,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{js,ts,jsx,tsx}: Use descriptive names for functions, variables, and classes that clearly convey their purpose
Write comments that explain the 'why' behind code decisions, not the 'what'
Keep functions small and focused on a single responsibility
Use const by default, let when reassignment is needed, and avoid var
Prefer async/await over .then() chains for handling asynchronous operations
Use explicit error handling with try/catch blocks for async operations
Avoid deeply nested code; refactor complex logic into helper functions
Files:
app/sagas/__tests__/deepLinking.videoConf.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/sagas/__tests__/deepLinking.videoConf.test.ts
**/*.{ts,tsx,js,jsx,json}
📄 CodeRabbit inference engine (CLAUDE.md)
Use Prettier formatting with tabs, single quotes, 130 character width, no trailing commas, avoid arrow parens, and bracket same line
Files:
app/sagas/__tests__/deepLinking.videoConf.test.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use ESLint with
@rocket.chat/eslint-configbase including React, React Native, TypeScript, and Jest plugins
Files:
app/sagas/__tests__/deepLinking.videoConf.test.ts
app/sagas/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Place side effects in 'app/sagas/' directory (init, login, rooms, messages, encryption, deepLinking, videoConf)
Files:
app/sagas/__tests__/deepLinking.videoConf.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.videoConf.test.ts
There was a problem hiding this comment.
♻️ Duplicate comments (1)
app/sagas/__tests__/deepLinking.videoConf.test.ts (1)
495-499:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winE1 ordering assertion is still a no-op.
rootInsideIdx >= 0is trivially true oncerootInsideActionis defined; nothing here verifies thatappStart(ROOT_INSIDE)was dispatched beforenavigateToRoomran. Either drop the misleading comment/index lines, or capture a snapshot ofdispatchedfrom insidejest.mocked(navigateToRoom).mockImplementation(...)and assert the ROOT_INSIDE action is present in that snapshot.🤖 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.videoConf.test.ts` around lines 495 - 499, The current check using rootInsideIdx is a no-op for ordering; update the test to capture the dispatched array at the moment navigateToRoom is invoked by replacing/augmenting the jest.mocked(navigateToRoom).mockImplementation(...) to copy the current dispatched (e.g. const dispatchedAtNavigate = [...dispatched]) and then assert that dispatchedAtNavigate includes rootInsideAction (e.g. dispatchedAtNavigate.indexOf(rootInsideAction!) >= 0); remove the misleading comment/index assertions or replace them with this snapshot-based assertion so you verify appStart(ROOT_INSIDE) happened before navigateToRoom ran.
🧹 Nitpick comments (1)
app/sagas/__tests__/deepLinking.videoConf.test.ts (1)
174-182: 💤 Low valueConsolidate the repeated
beforeEachmock-reset boilerplate.The same 7-line
mockReset()block is duplicated across all fourdescribeblocks (lines 174-182, 201-211, 278-287, 335-344, 456-465). Extract a single helper (or use a top-levelbeforeEachplusjest.resetAllMocks()augmented withmockSubsCollection.find.mockReset()) to reduce duplication and keep new mocks from being forgotten in one block but not the others.♻️ Suggested helper
const resetAllSagaMocks = () => { jest.mocked(UserPreferences.getString).mockReset(); jest.mocked(getServerById).mockReset(); jest.mocked(getServerInfo).mockReset(); jest.mocked(localAuthenticate).mockReset(); jest.mocked(navigateToRoom).mockReset(); jest.mocked(notifyUser).mockReset(); jest.mocked(videoConfJoin).mockReset(); mockSubsCollection.find.mockReset(); };Then call
resetAllSagaMocks()from eachbeforeEach.🤖 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.videoConf.test.ts` around lines 174 - 182, Extract the repeated mockReset block into a single helper and call it from each beforeEach: create a function resetAllSagaMocks that calls jest.mocked(UserPreferences.getString).mockReset(), jest.mocked(getServerById).mockReset(), jest.mocked(getServerInfo).mockReset(), jest.mocked(localAuthenticate).mockReset() (if used elsewhere), jest.mocked(navigateToRoom).mockReset(), jest.mocked(notifyUser).mockReset(), jest.mocked(videoConfJoin).mockReset() and mockSubsCollection.find.mockReset(); then replace the duplicated mockReset lines in each describe's beforeEach with a call to resetAllSagaMocks() (or alternatively use a top-level beforeEach that calls jest.resetAllMocks() plus mockSubsCollection.find.mockReset()).
🤖 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.
Duplicate comments:
In `@app/sagas/__tests__/deepLinking.videoConf.test.ts`:
- Around line 495-499: The current check using rootInsideIdx is a no-op for
ordering; update the test to capture the dispatched array at the moment
navigateToRoom is invoked by replacing/augmenting the
jest.mocked(navigateToRoom).mockImplementation(...) to copy the current
dispatched (e.g. const dispatchedAtNavigate = [...dispatched]) and then assert
that dispatchedAtNavigate includes rootInsideAction (e.g.
dispatchedAtNavigate.indexOf(rootInsideAction!) >= 0); remove the misleading
comment/index assertions or replace them with this snapshot-based assertion so
you verify appStart(ROOT_INSIDE) happened before navigateToRoom ran.
---
Nitpick comments:
In `@app/sagas/__tests__/deepLinking.videoConf.test.ts`:
- Around line 174-182: Extract the repeated mockReset block into a single helper
and call it from each beforeEach: create a function resetAllSagaMocks that calls
jest.mocked(UserPreferences.getString).mockReset(),
jest.mocked(getServerById).mockReset(), jest.mocked(getServerInfo).mockReset(),
jest.mocked(localAuthenticate).mockReset() (if used elsewhere),
jest.mocked(navigateToRoom).mockReset(), jest.mocked(notifyUser).mockReset(),
jest.mocked(videoConfJoin).mockReset() and mockSubsCollection.find.mockReset();
then replace the duplicated mockReset lines in each describe's beforeEach with a
call to resetAllSagaMocks() (or alternatively use a top-level beforeEach that
calls jest.resetAllMocks() plus mockSubsCollection.find.mockReset()).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 55be3bb1-3fdd-43bb-ad03-7275bcaa1e35
📒 Files selected for processing (1)
app/sagas/__tests__/deepLinking.videoConf.test.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{js,ts,jsx,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{js,ts,jsx,tsx}: Use descriptive names for functions, variables, and classes that clearly convey their purpose
Write comments that explain the 'why' behind code decisions, not the 'what'
Keep functions small and focused on a single responsibility
Use const by default, let when reassignment is needed, and avoid var
Prefer async/await over .then() chains for handling asynchronous operations
Use explicit error handling with try/catch blocks for async operations
Avoid deeply nested code; refactor complex logic into helper functions
Files:
app/sagas/__tests__/deepLinking.videoConf.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/sagas/__tests__/deepLinking.videoConf.test.ts
**/*.{ts,tsx,js,jsx,json}
📄 CodeRabbit inference engine (CLAUDE.md)
Use Prettier formatting with tabs, single quotes, 130 character width, no trailing commas, avoid arrow parens, and bracket same line
Files:
app/sagas/__tests__/deepLinking.videoConf.test.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use ESLint with
@rocket.chat/eslint-configbase including React, React Native, TypeScript, and Jest plugins
Files:
app/sagas/__tests__/deepLinking.videoConf.test.ts
app/sagas/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Place side effects in 'app/sagas/' directory (init, login, rooms, messages, encryption, deepLinking, videoConf)
Files:
app/sagas/__tests__/deepLinking.videoConf.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.videoConf.test.ts
diegolmello
force-pushed
the
fix/deeplink-auth-room-nav
branch
from
46f56f9 to
9c8a9ee
Compare
1 participant
Proposed changes
Adds a new isolated test file
app/sagas/__tests__/deepLinking.videoConf.test.tscovering every branch ofhandleClickCallPushand the downstreamhandleNavigateCallRoominapp/sagas/deepLinking.js. Mirrors the harness/mock conventions established by the slice-1 test file (deepLinking.test.ts) and adds a controllableapp/lib/databasemock so subscription lookup can be driven per test.The 12 cases lock down current behavior across:
params.hostempty short-circuits with no dispatches/nav/join.localAuthenticate+selectServerRequest(host, version, true)+ waitLOGIN.SUCCESS+ nav.localAuthenticate+selectServerRequest(host, version, true, true)+ waitLOGIN.SUCCESS+ nav.getServerInfofail →fallbackNavigation(novoipAcceptFailedbranch in this handler, unlikehandleOpen);getServerInfook with token →appStart(ROOT_OUTSIDE)+serverInitAdd+delay(1000)+NewServer emit+ waitSERVER.SELECT_SUCCESS+loginRequest({ resume })+ waitLOGIN.SUCCESS+ nav (note: this branch lacksappReady/take(APP.START)ordering; that ishandleOpen-only);getServerInfook no token → no further action.handleNavigateCallRoom) — always dispatchesappStart(ROOT_INSIDE)before subscription lookup;accept→notifyUser('accepted')+videoConfJoin(callId, true, false, true);decline→notifyUser('rejected')only;isMasterDetail=truepropagated tonavigateToRoom; subscriptionfindrejection caught/logged withappStart(ROOT_INSIDE)still dispatched.No edits to
app/sagas/deepLinking.js. No new package dependencies.Issue(s)
Part of the deep-linking saga test suite — see
.scratch/deeplinking-saga-tests/for the parent PRD and slice breakdown.How to test or reproduce
TZ=UTC yarn test app/sagas/__tests__/deepLinking.videoConf.test.tsAll 12 cases should pass. The console.error noise on case E5 is expected — it is the saga's
log(e)catch on the rejected subscriptionfind.Screenshots
N/A (test-only change).
Types of changes
Checklist
Further comments
Test isolation: this file is standalone — separate
describeblocks, its own mock surface (addsapp/lib/databasemock withmockSubsCollection.findcontrollable per test). Uses the same collector-middleware pattern from slices 04/05 to assert saga-put()actions, sinceput()does not flow throughstore.dispatchoverrides.Summary by CodeRabbit