test: cover handleOpen server-resolution branches in deep linking saga#7306
test: cover handleOpen server-resolution branches in deep linking saga#7306diegolmello 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.
Extends the deep linking saga harness with coverage for share-extension, oauth, no-host, same-server, known-different-server, and unknown-server resolution branches. Lock down current behavior for all entry-point variants.
WalkthroughA deep linking test suite was refactored to introduce a ChangesDeep Linking Test Suite Refactoring
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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.
🧹 Nitpick comments (1)
app/sagas/__tests__/deepLinking.test.ts (1)
166-197: ⚡ Quick winF4
beforeEachshould delegate toresetMocks()to stay in sync.The F4
beforeEachwas written beforeresetMocks()existed and now duplicates a subset of it (6 of the 8 mocks it covers). If additional mocks are added toresetMocks()in the future, the F4 group won't pick them up, which can produce hard-to-diagnose state leaks.♻️ Proposed refactor
beforeEach(() => { jest.useFakeTimers(); - // Reset all mocks - jest.mocked(UserPreferences.getString).mockReset(); - jest.mocked(getServerById).mockReset(); - jest.mocked(canOpenRoom).mockReset(); - jest.mocked(getServerInfo).mockReset(); - jest.mocked(goRoom).mockReset(); - jest.mocked(waitForNavigationReady).mockReset(); + resetMocks(); // Default: unknown server (no current server match, no serverRecord)🤖 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 166 - 197, Replace the duplicated mock-reset/setup logic in the beforeEach block of deepLinking.test.ts with a call to the existing resetMocks() helper so this test stays in sync with global mock initialization; specifically, remove the manual jest.useFakeTimers() and jest.mocked(...).mockReset()/mockImplementation()/mockResolvedValue() calls and delegate to resetMocks(), then add any test-specific overrides (e.g., specific UserPreferences.getString, getServerById, getServerInfo, canOpenRoom, waitForNavigationReady, goRoom behavior) only after calling resetMocks() so the test retains its intended default stubs while inheriting future changes to resetMocks().
🤖 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 166-197: Replace the duplicated mock-reset/setup logic in the
beforeEach block of deepLinking.test.ts with a call to the existing resetMocks()
helper so this test stays in sync with global mock initialization; specifically,
remove the manual jest.useFakeTimers() and
jest.mocked(...).mockReset()/mockImplementation()/mockResolvedValue() calls and
delegate to resetMocks(), then add any test-specific overrides (e.g., specific
UserPreferences.getString, getServerById, getServerInfo, canOpenRoom,
waitForNavigationReady, goRoom behavior) only after calling resetMocks() so the
test retains its intended default stubs while inheriting future changes to
resetMocks().
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4db2f1a8-cbfe-4d22-a8ba-7ae708ac73ac
📒 Files selected for processing (1)
app/sagas/__tests__/deepLinking.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.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.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.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.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.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)
121-136: LGTM — collector ordering is correct.
collectorMiddlewareplaced beforesagaMiddlewareinapplyMiddlewaremeans the store's enhanceddispatch(which redux-saga'sput()uses) routes through the collector first, so saga-emitted actions are captured as intended.
778-805: F3 — LGTM, new-server bootstrap chain thoroughly verified.The test correctly advances the
delay(1000)withjest.advanceTimersByTimeAsync, then confirms the full chain:APP_START(ROOT_OUTSIDE),SERVER_INIT_ADD, andINVITE_LINKS_SET_TOKENwith the extracted token.
648-654: ⚡ Quick winNo action needed. The assertion is correct as written.
The
selectServerRequestaction creator (app/actions/server.ts lines 32–36) has an explicit default parameterchangeServer = false. When the saga callsselectServerRequest(host, version, true)with only three arguments, the fourth parameter defaults tofalse, soselectReq?.changeServerwill indeed befalseand the assertion will pass.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
app/sagas/__tests__/deepLinking.test.ts (2)
144-158: ⚡ Quick winReplace
Record<string, any>factory overrides with typed interfacesLine 144 and Line 154 use
Record<string, any>, which makes test fixtures less self-documenting and easier to misuse.Suggested diff
+interface DeepLinkParamsOverrides { + host?: string; + token?: string; + path?: string; + type?: string; + credentialToken?: string; + credentialSecret?: string; +} + +interface ServerRecordOverrides { + id?: string; + version?: string; + name?: string; + server?: string; +} + -const makeParams = (overrides: Record<string, any> = {}) => ({ +const makeParams = (overrides: DeepLinkParamsOverrides = {}) => ({ host: HOST, ...overrides }); -const makeServerRecord = (overrides: Record<string, any> = {}) => ({ +const makeServerRecord = (overrides: ServerRecordOverrides = {}) => ({ id: HOST, version: '6.0.0', ...overrides });As per coding guidelines: "**/*.{ts,tsx}: Prefer interfaces over type aliases for defining object shapes in TypeScript".
🤖 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 144 - 158, The tests use untyped override objects in makeParams, makeParamsWithToken and makeServerRecord (each currently typed as Record<string, any>); replace those with explicit interfaces describing allowed override fields (e.g., ParamsOverrides with optional host/path/token, and ServerRecordOverrides with optional id/version/etc.) and update the function signatures to use those interfaces so the fixtures are self-documenting and type-safe; adjust any callers in the test file to satisfy the stronger types and export or declare the interfaces near the factory functions for clarity (refer to makeParams, makeParamsWithToken, makeServerRecord).
126-135: ⚡ Quick winTighten collector typing to avoid
anyleakageLine 127–130 currently uses
any, which weakens assertions and can hide action-shape mistakes in these integration-style tests.Suggested diff
+import type { AnyAction, Middleware } from 'redux'; + function setupStoreWithCollector(preloadedState?: PreloadedState) { - const collected: any[] = []; - const collectorMiddleware = () => (next: any) => (action: any) => { + const collected: AnyAction[] = []; + const collectorMiddleware: Middleware = () => next => action => { collected.push(action); return next(action); };As per coding guidelines: "**/*.{ts,tsx}: 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.test.ts` around lines 126 - 135, The collector in setupStoreWithCollector currently uses broad any types; change collected to an array of Redux AnyAction (or a specific action union used in tests) and type collectorMiddleware with Redux Middleware/MiddlewareAPI/Dispatch signatures (use AnyAction for action param and proper store types for next/api) so that collected pushes typed actions and TypeScript can validate assertions; update the function signature of setupStoreWithCollector to return typed { store: Store, collected: AnyAction[] } (or the concrete action union) and replace all any usages in collected, collectorMiddleware, next, and action with those explicit types.
🤖 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 144-158: The tests use untyped override objects in makeParams,
makeParamsWithToken and makeServerRecord (each currently typed as Record<string,
any>); replace those with explicit interfaces describing allowed override fields
(e.g., ParamsOverrides with optional host/path/token, and ServerRecordOverrides
with optional id/version/etc.) and update the function signatures to use those
interfaces so the fixtures are self-documenting and type-safe; adjust any
callers in the test file to satisfy the stronger types and export or declare the
interfaces near the factory functions for clarity (refer to makeParams,
makeParamsWithToken, makeServerRecord).
- Around line 126-135: The collector in setupStoreWithCollector currently uses
broad any types; change collected to an array of Redux AnyAction (or a specific
action union used in tests) and type collectorMiddleware with Redux
Middleware/MiddlewareAPI/Dispatch signatures (use AnyAction for action param and
proper store types for next/api) so that collected pushes typed actions and
TypeScript can validate assertions; update the function signature of
setupStoreWithCollector to return typed { store: Store, collected: AnyAction[] }
(or the concrete action union) and replace all any usages in collected,
collectorMiddleware, next, and action with those explicit types.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7bf05714-9b85-4268-a3b7-772f9a1f6e49
📒 Files selected for processing (1)
app/sagas/__tests__/deepLinking.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.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.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.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.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.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
diegolmello
force-pushed
the
fix/deeplink-auth-room-nav
branch
from
46f56f9 to
9c8a9ee
Compare
1 participant
Proposed changes
Extends the deep linking saga test harness (established in the slice-03 PR) with integration-style coverage for every
handleOpenserver-resolution branch. Tests run real reducers + real saga middleware; only I/O boundaries are mocked.Groups added:
shareextensiontype: no user →appInit; user + sdk-host mismatch → waitLOGIN.SUCCESS+shareSetParams+ROOT_SHARE_EXTENSION; user + sdk-host match → same without the wait.oauthtype:loginOAuthOrSsocalled with credential params; rejection swallowed silently.fallbackNavigationno-ops; no root →appInit.goRoom; not connected →localAuthenticate+selectServerRequest(host, version, true)+LOGIN.SUCCESS+goRoom.localAuthenticate+selectServerRequest(host, version, true, true)(changeServer=true) +LOGIN.SUCCESS+goRoom.getServerInfofailure →fallbackNavigation→appInit.getServerInfook, no token, invite path → new-server bootstrap chain +inviteLinksSetToken(token).Note: Group F2 (voipAcceptFailed branch on unknown server) is deferred to slice 05. This PR's base will be re-targeted to
developonce #7304 merges.Issue(s)
Extends coverage for the deep-linking saga as part of the test coverage initiative for
app/sagas/deepLinking.js.How to test or reproduce
TZ=UTC yarn test app/sagas/__tests__/deepLinking.test.tsAll 17 tests should pass.
Screenshots
N/A — test-only change.
Types of changes
Checklist
Further comments
Uses a
setupStoreWithCollectorhelper (a Redux middleware that records every dispatched action through the chain, including sagaput()effects) to assert on saga-dispatched actions without brittle store.dispatch monkey-patching.Summary by CodeRabbit