feat: Login on web#7360
Conversation
yash-rajpal
requested a deployment
to
approve_e2e_testing
GitHub Actions
Waiting
WalkthroughThe PR adds a "Login on web" button to the LoginServices mobile view. Services can now opt out of showing buttons on mobile via ChangesMobile Login on Web Feature
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Suggested labels
🚥 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 (1)
app/containers/LoginServices/index.tsx (1)
63-63: ⚡ Quick winSimplify the boolean check.
The
=== truecomparison is redundant when the result is already wrapped inBoolean(). Thefind()method returns either a truthy value (the service object) orundefined, so the explicit comparison adds no value.♻️ Proposed simplification
-const showLoginOnWeb = Boolean(Object.values(services).find((service: IItemService) => service.hideButtonOnMobile === true)); +const showLoginOnWeb = Boolean(Object.values(services).find((service: IItemService) => service.hideButtonOnMobile));🤖 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/containers/LoginServices/index.tsx` at line 63, The boolean expression for showLoginOnWeb is overcomplicated: remove the redundant "=== true" and simplify the check by using the truthiness of the found service (e.g., use Object.values(services).find(...) directly wrapped with Boolean or use .some(...)) so that the expression references services, IItemService, and hideButtonOnMobile and relies on find()/some() returning a truthy value instead of comparing to true.
🤖 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/containers/LoginServices/index.tsx`:
- Around line 39-46: The call to Linking.openURL inside the conditional render
(when showLoginOnWeb is true) is unhandled and may reject; update the Button
onPress handler in LoginServices so
Linking.openURL(`${server}/home?loginClient=mobile`) is explicitly handled with
a Promise rejection handler (e.g., .catch(err => /* log or show error */)) to
satisfy ESLint's no-void rule and avoid unhandled rejections; keep the same
URL/template and ensure the catch logs or surfaces the error (using the app
logger or console) rather than leaving the promise unhandled.
---
Nitpick comments:
In `@app/containers/LoginServices/index.tsx`:
- Line 63: The boolean expression for showLoginOnWeb is overcomplicated: remove
the redundant "=== true" and simplify the check by using the truthiness of the
found service (e.g., use Object.values(services).find(...) directly wrapped with
Boolean or use .some(...)) so that the expression references services,
IItemService, and hideButtonOnMobile and relies on find()/some() returning a
truthy value instead of comparing to true.
🪄 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: c0aeed82-c575-4ea4-992d-1366f08eb79f
📒 Files selected for processing (3)
app/containers/LoginServices/index.tsxapp/containers/LoginServices/interfaces.tsapp/i18n/locales/en.json
📜 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/containers/LoginServices/interfaces.tsapp/containers/LoginServices/index.tsx
**/*.{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/containers/LoginServices/interfaces.tsapp/containers/LoginServices/index.tsx
**/*.{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/containers/LoginServices/interfaces.tsapp/containers/LoginServices/index.tsx
app/containers/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Reusable UI components should be placed in app/containers/ directory
Files:
app/containers/LoginServices/interfaces.tsapp/containers/LoginServices/index.tsx
🧠 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/containers/LoginServices/interfaces.tsapp/containers/LoginServices/index.tsx
🔇 Additional comments (6)
app/containers/LoginServices/interfaces.ts (1)
27-27: LGTM!app/containers/LoginServices/index.tsx (4)
64-67: LGTM!
77-77: LGTM!
86-92: LGTM!
107-113: LGTM!app/i18n/locales/en.json (1)
517-517: LGTM!
| {showLoginOnWeb && ( | ||
| <Button | ||
| title={I18n.t('Login_on_web')} | ||
| onPress={() => { | ||
| Linking.openURL(`${server}/home?loginClient=mobile`); | ||
| }} | ||
| /> | ||
| )} |
There was a problem hiding this comment.
Add error handling for Linking.openURL.
Linking.openURL returns a Promise that can reject if the URL cannot be opened. The unhandled rejection could cause runtime errors. Based on learnings, you must handle the promise explicitly with .catch(...) to satisfy ESLint's no-void: error rule.
🛡️ Proposed fix to add error handling
{showLoginOnWeb && (
<Button
title={I18n.t('Login_on_web')}
onPress={() => {
- Linking.openURL(`${server}/home?loginClient=mobile`);
+ Linking.openURL(`${server}/home?loginClient=mobile`).catch(err => {
+ // Handle error - could log or show user feedback
+ console.error('Failed to open login URL:', err);
+ });
}}
/>
)}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| {showLoginOnWeb && ( | |
| <Button | |
| title={I18n.t('Login_on_web')} | |
| onPress={() => { | |
| Linking.openURL(`${server}/home?loginClient=mobile`); | |
| }} | |
| /> | |
| )} | |
| {showLoginOnWeb && ( | |
| <Button | |
| title={I18n.t('Login_on_web')} | |
| onPress={() => { | |
| Linking.openURL(`${server}/home?loginClient=mobile`).catch(err => { | |
| // Handle error - could log or show user feedback | |
| console.error('Failed to open login URL:', err); | |
| }); | |
| }} | |
| /> | |
| )} |
🤖 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/containers/LoginServices/index.tsx` around lines 39 - 46, The call to
Linking.openURL inside the conditional render (when showLoginOnWeb is true) is
unhandled and may reject; update the Button onPress handler in LoginServices so
Linking.openURL(`${server}/home?loginClient=mobile`) is explicitly handled with
a Promise rejection handler (e.g., .catch(err => /* log or show error */)) to
satisfy ESLint's no-void rule and avoid unhandled rejections; keep the same
URL/template and ensure the catch logs or surfaces the error (using the app
logger or console) rather than leaving the promise unhandled.
| const server = useAppSelector(state => state.server.server); | ||
| const services = useAppSelector(state => state.login.services as IServices, shallowEqual); | ||
| const { length } = Object.values(services); | ||
| const showLoginOnWeb = Boolean(Object.values(services).find((service: IItemService) => service.hideButtonOnMobile === true)); |
There was a problem hiding this comment.
If the service has hideButtonOnMobile we show login on web?
It looks wrong and can generate confusion for the user and for maintainability.
Not sure if it's a new prop or not, but if it's new and we ever want to hide login services on mobile, it's going to be very weird.
| const showLoginOnWeb = Boolean(Object.values(services).find((service: IItemService) => service.hideButtonOnMobile === true)); | ||
| const filteredServices = Object.fromEntries( | ||
| Object.entries(services).filter(([, service]) => !service.hideButtonOnMobile) |
There was a problem hiding this comment.
Do we really need to map the array twice, one with service.hideButtonOnMobile === true and another with !service.hideButtonOnMobile?
|
|
||
| const onPressButtonSeparator = () => { | ||
| heightButtons.value = collapsed ? SERVICE_HEIGHT * length : SERVICES_COLLAPSED_HEIGHT; | ||
| heightButtons.value = collapsed ? SERVICE_HEIGHT * (length + (showLoginOnWeb ? 1 : 0)) : SERVICES_COLLAPSED_HEIGHT; |
There was a problem hiding this comment.
The new login should belong to the same filteredServices, so length would have the correct value.
Shouldn't this new login service be returned on settings.oauth endpoint?
| "Login_error": "Your credentials were rejected! Please try again.", | ||
| "Login_has_been_temporarily_blocked_for_this_IP": "Login has been temporarily blocked for this IP", | ||
| "Login_has_been_temporarily_blocked_for_this_User": "Login has been temporarily blocked for this user", | ||
| "Login_on_web": "Login on web", |
There was a problem hiding this comment.
Use AI to write translations for all supported languages
Rocket.Chat.ReactNative/app/i18n/index.ts
Line 20 in 815ff6c
diegolmello
left a comment
diegolmello
left a comment
There was a problem hiding this comment.
Also missing stories and tests
2 participants
Proposed changes
Hides the required login services buttons and shows new login on web button based on server response.
Requires - Rocket.Chat/Rocket.Chat#40733
Issue(s)
How to test or reproduce
Screenshots
Types of changes
Checklist
Further comments
PRM-48
Summary by CodeRabbit
New Features
Updates