test: cover handleOpen navigate sub-cases + VoIP-fail completion#7307
test: cover handleOpen navigate sub-cases + VoIP-fail completion#7307diegolmello 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 URL-shape navigation contract coverage (channel/direct/group/livechat/thread/invite/messageId) and the VoIP-accept-failed recovery path (incl. group F2 routing).
|
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughExpands deepLinking saga tests: adds a collector middleware helper, boundary mocks/imports, and three test groups covering URL navigation shapes, VoIP accept-failed recovery, and an unknown-server VoIP fallback. ChangesdeepLinking Saga Test Expansion
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 (3)
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 (3)
app/sagas/__tests__/deepLinking.test.ts (3)
127-131: 💤 Low value
setupStoreWithCollectorcomment mis-describes why the approach is neededThe comment says saga middleware "short-circuits its own emissions before they reach
store.dispatch", but that is not accurate. redux-saga'sput()effects DO flow through the fullstore.dispatchchain. The real reason a post-creationstore.dispatchoverride misses saga puts is that redux-saga captures the store'sdispatchreference at middleware-init time, so reassigningstore.dispatchafterwards is not visible to the saga.The implementation is correct; only the rationale in the comment is misleading.
📝 Suggested wording
-/** - * Like setupStore, but adds a collector middleware so dispatched actions emitted via - * saga `put()` effects are captured. Plain `store.dispatch` overrides do NOT see saga puts - * because saga middleware short-circuits its own emissions before they reach `store.dispatch`. - */ +/** + * Like setupStore, but inserts a collector middleware that captures every dispatched action + * (including saga `put()` effects). Mutating `store.dispatch` after store creation does not + * capture saga puts because redux-saga holds the dispatch reference captured at middleware + * init time; this middleware approach intercepts at the correct level. + */🤖 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 127 - 131, The JSDoc for setupStoreWithCollector is misleading about redux-saga behavior; update the comment for the setupStoreWithCollector helper to state that redux-saga's put() effects do flow through the full store.dispatch chain but that redux-saga captures the store.dispatch reference at middleware initialization, so overwriting store.dispatch after middleware creation will not affect saga-produced dispatches; replace the incorrect phrase about middleware "short-circuiting its own emissions" with this accurate explanation and keep the rest of the implementation as-is.
655-667: ⚡ Quick winH3 is missing a
showToastassertion that H1 and H2 both makeH1 and H2 assert
showToastis called with'VoIP_Call_Issue'on every non-throw path throughhandleVoipAcceptFailed. H3 covers thepath-fallback navigation branch but omits this assertion. IfshowToastis unconditional after navigation in the saga (as the H1/H2 evidence suggests), this is a coverage gap that would allow a regression where the toast is silently dropped on the path-fallback branch.✅ Proposed addition
expect(jest.mocked(goRoom)).toHaveBeenCalledTimes(1); const call = jest.mocked(goRoom).mock.calls[0][0]; expect(call.item.name).toBe('bob'); + expect(jest.mocked(showToast)).toHaveBeenCalledWith('VoIP_Call_Issue');🤖 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 655 - 667, The H3 test ("without username but with params.path → uses params.path for navigation") is missing an assertion that showToast was invoked like H1/H2; update that test in deepLinking.test.ts (the H3 block) to assert that showToast was called with 'VoIP_Call_Issue' after the navigation branch (the driveVoipFail/handleVoipAcceptFailed path), mirroring the existing H1/H2 expectations (use jest.mocked(showToast) to check the call and optionally the call count).
525-535: ⚡ Quick winImport the
INVITE_LINKSconstant to eliminate the hardcoded action type stringReplace the hardcoded string
'INVITE_LINKS_REQUEST'withINVITE_LINKS.REQUESTfor compile-time safety and to prevent silent failures if the constant is ever renamed.♻️ Proposed fix
+import { INVITE_LINKS } from '../../actions/actionsTypes'; // inside G7: -const inviteAction = dispatched.find((a: any) => a.type === 'INVITE_LINKS_REQUEST'); +const inviteAction = dispatched.find((a: any) => a.type === INVITE_LINKS.REQUEST);🤖 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 525 - 535, The test currently asserts dispatched actions using the hardcoded string 'INVITE_LINKS_REQUEST'; import the INVITE_LINKS constant and replace that string with INVITE_LINKS.REQUEST in the test so it checks the correct action type at compile-time—update the import section to include INVITE_LINKS and change the dispatched.find predicate (and any related expectations) to use INVITE_LINKS.REQUEST when verifying the inviteAction after calling driveNavigate in this test (the surrounding helpers are setupStoreWithCollector, driveNavigate and the mocked goRoom).
🤖 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.test.ts`:
- Around line 550-559: The test currently asserts store.getState().app.root but
connectedState already initializes app.root to RootEnum.ROOT_INSIDE, making the
assertion a false positive; update the test to either use
setupStoreWithCollector(...) to capture dispatched actions and assert that
appStart(ROOT_INSIDE) appears in dispatched, or create the store with a
different preloaded root (e.g. clone connectedState and set app.root =
RootEnum.ROOT_OUTSIDE) so you can assert the state changed; keep the rest of the
flow (jest.mocked(canOpenRoom).mockResolvedValue(null), driveNavigate(...), and
verifying goRoom was not called) unchanged and reference canOpenRoom, goRoom,
driveNavigate, makeParams, setupStoreWithCollector, appStart, RootEnum, and
connectedState when locating the test to modify.
---
Nitpick comments:
In `@app/sagas/__tests__/deepLinking.test.ts`:
- Around line 127-131: The JSDoc for setupStoreWithCollector is misleading about
redux-saga behavior; update the comment for the setupStoreWithCollector helper
to state that redux-saga's put() effects do flow through the full store.dispatch
chain but that redux-saga captures the store.dispatch reference at middleware
initialization, so overwriting store.dispatch after middleware creation will not
affect saga-produced dispatches; replace the incorrect phrase about middleware
"short-circuiting its own emissions" with this accurate explanation and keep the
rest of the implementation as-is.
- Around line 655-667: The H3 test ("without username but with params.path →
uses params.path for navigation") is missing an assertion that showToast was
invoked like H1/H2; update that test in deepLinking.test.ts (the H3 block) to
assert that showToast was called with 'VoIP_Call_Issue' after the navigation
branch (the driveVoipFail/handleVoipAcceptFailed path), mirroring the existing
H1/H2 expectations (use jest.mocked(showToast) to check the call and optionally
the call count).
- Around line 525-535: The test currently asserts dispatched actions using the
hardcoded string 'INVITE_LINKS_REQUEST'; import the INVITE_LINKS constant and
replace that string with INVITE_LINKS.REQUEST in the test so it checks the
correct action type at compile-time—update the import section to include
INVITE_LINKS and change the dispatched.find predicate (and any related
expectations) to use INVITE_LINKS.REQUEST when verifying the inviteAction after
calling driveNavigate in this test (the surrounding helpers are
setupStoreWithCollector, driveNavigate and the mocked goRoom).
🪄 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: fab0327c-8092-4040-8def-3b59567bb3a1
📒 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 harness (introduced on
fix/deeplink-auth-room-nav) with the URL-shape → navigation contract and the VoIP-accept-failed recovery path. No production code changes — tests only.Coverage added on top of the slice-03 harness (
app/sagas/__tests__/deepLinking.test.ts):navigate()sub-cases (G1–G11) driven via the D1 path (same-server, already connected):channel/general→t:'c', name'general'direct/alice→t:'d',roomUserIdfromgetUidDirectMessagegroup/secret→t:'p'channels/livechat-1→t:'l'group/foo/thread/abc123→jumpToThreadId:'abc123'params.messageId='msg42'→jumpToMessageId:'msg42'invite/xyz789→ nogoRoom, dispatchesinviteLinksRequest('xyz789')params.ridonly →goRoominvoked with the room spreadcanOpenRoomreturns null → nogoRoom, still dispatchesappStart(ROOT_INSIDE)isMasterDetail=true) →goRoominvoked withisMasterDetail:truecanOpenRoomthrows → propagates (locks down current behavior)handleVoipAcceptFailed(H1–H4) driven via the C1 no-host +voipAcceptFailed=truebranch:callId+username→resetVoipState,RNCallKeep.endCall(callId),waitForNavigationReady, navigate todirect/${username},showToast('VoIP_Call_Issue')callId→endCallNOT called, other steps unchangedusernamebutparams.pathset → usesparams.pathfor navigationnavigatethrows → caught byhandleVoipAcceptFailed's try/catch,showToastnot reachedgetServerInfofail withvoipAcceptFailed=true→ routed through the F2 fallback branch (not C1).Implementation notes:
showToasthelper,InteractionManager.runAfterInteractionsviajest.spyOnperbeforeEach).setupStoreWithCollectorhelper captures sagaput()effects via a custom middleware. Plainstore.dispatchoverrides don't see saga puts because saga middleware short-circuits its own emissions before they reachstore.dispatch. Used by G7 to assertinviteLinksRequestwas dispatched.deepLinking.test.ts(under the 800-line split threshold called out in the issue).app/sagas/deepLinking.js. No new dependencies.Issue(s)
Extends the test coverage added in #7304. Base will be re-targeted to
developafter #7304 merges.How to test or reproduce
TZ=UTC yarn test app/sagas/__tests__/deepLinking.test.tsAll 21 tests should pass (5 from slice-03's F4 regression race + 16 added here: G1–G11, H1–H4, F2). Local run completes in ~1.5s.
Screenshots
N/A (test-only change).
Types of changes
Checklist
Further comments
Sibling PR to slice-04 (open separately) — both extend slice-03's harness and will need to reconcile when both merge into
fix/deeplink-auth-room-nav. Slice-04 introduces a similarsetupStoreWithCollectorpattern; the two helpers can be deduplicated downstream.Summary by CodeRabbit