Skip to content

fix(login): don't cancel login saga on 2s timeout (#7333)#7340

Merged
diegolmello merged 2 commits into
developfrom
fix-login-conditional-cancel
May 26, 2026
Merged

fix(login): don't cancel login saga on 2s timeout (#7333)#7340
diegolmello merged 2 commits into
developfrom
fix-login-conditional-cancel

Conversation

@diegolmello

@diegolmello diegolmello commented May 25, 2026

Copy link
Copy Markdown
Member

Proposed changes

The root() watcher in app/sagas/login.js raced SERVER.SELECT_REQUEST against a 2-second delay, then canceled handleLoginSuccess unconditionally — whichever arm of the race won. The cancel was meant only as a guard against a workspace switch interrupting login mid-flow, but as written it imposed a hard 2-second deadline on the saga.

Inside handleLoginSuccess, two awaited REST calls run before appStart(ROOT_INSIDE):

yield call(fetchPermissions);
yield call(fetchEnterpriseModules, { user });
// …
yield put(appStart({ root: RootEnum.ROOT_INSIDE }));

On a slow network / fresh install / large permission set, those exceed 2s, the saga is cancelled before navigation, and the user is stranded on the login screen. A retry succeeds because permissions are cached and the server is warm. Matches the symptom in #7333 and the Android variant reported in the same thread (home stack flashes for ~1s, then bounces back to login).

Regression introduced by the VoIP feature PR (#6918), which switched the two fetches from fork to call. The follow-up VoIP PR (#7270) noted the 2s race in a comment and routed VoIP out via spawn, but didn't address the underlying foot-gun.

This PR makes the cancel conditional — only fires when SERVER.SELECT_REQUEST actually wins the race. The 2s delay continues to bound how long the watcher listens for a workspace switch; it no longer doubles as a deadline on handleLoginSuccess.

Issue(s)

How to test or reproduce

  1. Fresh install or clear app data.
  2. Use a workspace where getPermissions + getEnterpriseModules together take >2s on first call (slow network, cold server, or large permission set; you can also pin this locally by adding yield delay(2500) to fetchPermissions on develop).
  3. Sign in (OAuth or password).
  4. Before fix: login screen does not dismiss; second attempt succeeds.
  5. After fix: login completes on first attempt regardless of fetch duration.

To verify the workspace-switch guard still works, dispatch SERVER.SELECT_REQUEST while handleLoginSuccess is in flight — the saga should still be cancelled.

Screenshots

n/a — saga-level fix.

Types of changes

  • Bugfix (non-breaking change which fixes an issue)
  • Improvement (non-breaking change which improves a current function)
  • New feature (non-breaking change which adds functionality)
  • Documentation update (if none of the other choices apply)

Checklist

  • I have read the CONTRIBUTING doc
  • I have signed the CLA
  • Lint and unit tests pass locally with my changes
  • I have added tests that prove my fix is effective or that my feature works (if applicable) — app/sagas/ has no unit-test harness today; tracked separately as Add unit-test coverage for app/sagas/ #7341.
  • I have added necessary documentation (if applicable)
  • Any dependent changes have been merged and published in downstream modules

Further comments

Alternative considered: change call(fetchPermissions) / call(fetchEnterpriseModules) to fork (as suggested in the issue thread). That would also unblock navigation, but leaves the unconditional cancel in place — any future awaited step in handleLoginSuccess would silently reintroduce the bug. Fixing the cancel itself addresses the root cause.

Summary by CodeRabbit

  • Bug Fixes
    • Resolved an issue where the login flow could be prematurely cancelled during long-running permission or enterprise data fetches. The login sequence now avoids an unnecessary timeout cancellation and lets critical background fetches complete before proceeding, improving reliability when switching workspaces or handling slow responses.

Review Change Stack

@coderabbitai

coderabbitai Bot commented May 25, 2026

Copy link
Copy Markdown
Contributor

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 90abb823-bd52-4b45-bad9-d18c1e993526

📥 Commits

Reviewing files that changed from the base of the PR and between 1d751aa and bd10b38.

📒 Files selected for processing (1)
  • app/sagas/login.js
📜 Recent 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 (2)
**/*.{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/login.js
**/*.{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-config base with React, React Native, TypeScript, Jest plugins

Files:

  • app/sagas/login.js
🧠 Learnings (1)
📚 Learning: 2026-05-07T13:19:52.152Z
Learnt from: diegolmello
Repo: RocketChat/Rocket.Chat.ReactNative PR: 7304
File: app/sagas/deepLinking.js:237-243
Timestamp: 2026-05-07T13:19:52.152Z
Learning: In this codebase’s Redux-Saga usage, remember that `yield put(action)` dispatches through the Redux store synchronously, and any saga(s) that synchronously react via action listeners (and synchronous `put` chains) will run to completion before the calling saga resumes at its next `yield`. As a result, within a single saga there is no scheduler interleaving between a `yield select(...)` and a subsequent `yield take(...)` at the next `yield` point, so a check-then-take pattern like `const state = yield select(...); if (state !== TARGET) { yield take(a => a.type === TARGET); }` is safe from TOCTOU races under the synchronous `put`/take model described above.

Applied to files:

  • app/sagas/login.js
🔇 Additional comments (1)
app/sagas/login.js (1)

482-492: LGTM!


Walkthrough

The login root saga's task cancellation logic is now conditional. The handleLoginSuccess task is cancelled only when a workspace switch (types.SERVER.SELECT_REQUEST) occurs, rather than being cancelled after a 2-second timeout unconditionally. This preserves long-running permission and enterprise fetches that may complete after the timeout.

Changes

Login Saga Task Cancellation

Layer / File(s) Summary
Conditional task cancellation on workspace switch
app/sagas/login.js
handleLoginSuccess cancellation now depends on the race outcome: cancelled only when types.SERVER.SELECT_REQUEST occurs, allowing the task to persist when the 2-second timeout elapses.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

Suggested labels

type: bug

Possibly related issues

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely summarizes the main fix: preventing cancellation of the login saga on a 2-second timeout, which directly addresses the bug described in the changeset.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

Warning

Review ran into problems

🔥 Problems

Errors were encountered while retrieving linked issues.

Errors (1)
  • NATIVE-1180: Request failed with status code 401

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

The cancel in login.js root() fired whichever arm of the race won, so
handleLoginSuccess had a hard 2-second deadline. When fetchPermissions
or fetchEnterpriseModules ran long, the saga was killed before
appStart(ROOT_INSIDE), leaving users on the login screen until they
retried.

Only cancel when SERVER.SELECT_REQUEST (workspace switch) wins; let the
timeout just bound the guard's listening window.

Fixes #7333
@github-actions

Copy link
Copy Markdown

iOS Build Available

Rocket.Chat 4.73.0.108946

@Rohit3523 Rohit3523 requested a deployment to approve_e2e_testing May 26, 2026 13:25 — with GitHub Actions Waiting

@Rohit3523 Rohit3523 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@diegolmello diegolmello merged commit 6b21875 into develop May 26, 2026
6 of 9 checks passed
@diegolmello diegolmello deleted the fix-login-conditional-cancel branch May 26, 2026 13:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

bug: Sign in with google does not let me sign in

2 participants