refactor(deeplink): migrate deepLinking saga to TypeScript#7438
refactor(deeplink): migrate deepLinking saga to TypeScript#7438Shevilll wants to merge 5 commits into
Conversation
Validate the username field against the server's UTF8_User_Names_Validation setting (with the default pattern as a fallback) before submitting the profile. This prevents invalid characters such as spaces from reaching the server, which previously surfaced only as a generic save error, and shows an inline message explaining the allowed characters. Closes RocketChat#1682
Drop the stale isMasterDetail destructured from useAppSelector (the selector no longer returns it) so only the useMasterDetail() hook declares it, fixing the no-redeclare lint/build failure introduced by merging develop. Also annotate isValidUsername with an explicit boolean return type per the TS guideline.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (3)
📜 Recent review details🧰 Additional context used📓 Path-based instructions (6)**/*.{js,jsx,ts,tsx,json}📄 CodeRabbit inference engine (CLAUDE.md)
Files:
app/i18n/**/*.{ts,tsx,json}📄 CodeRabbit inference engine (CLAUDE.md)
Files:
**/*.{js,ts,jsx,tsx}📄 CodeRabbit inference engine (AGENTS.md)
Files:
**/*.{ts,tsx}📄 CodeRabbit inference engine (AGENTS.md)
Files:
**/*.{js,jsx,ts,tsx}📄 CodeRabbit inference engine (CLAUDE.md)
Files:
app/actions/**/*.{ts,tsx}📄 CodeRabbit inference engine (CLAUDE.md)
Files:
🧠 Learnings (1)📚 Learning: 2026-04-30T17:07:51.020ZApplied to files:
🔇 Additional comments (2)
WalkthroughThe PR reimplements deep-link handling in TypeScript for room, invite, VoIP, share-extension, OAuth, and video-conference flows. It also adds profile username validation against a server-configured pattern, plus a new English validation string and tests. ChangesDeep-linking saga rewrite
Profile username validation
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
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: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/actions/deepLinking.ts (1)
5-16: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winInclude OAuth credential fields in the exported params contract.
handleOAuthconsumescredentialTokenandcredentialSecret, but exportedIParamsdoes not allow those fields. Typed callers ofdeepLinkingOpen({ type: 'oauth', ... })will either fail excess-property checks or need casts.Suggested contract update
export interface IParams { path: string; rid: string; messageId: string; host: string; fullURL: string; type: string; token: string; + credentialToken?: string; + credentialSecret?: string; callId?: string; username?: string; voipAcceptFailed?: boolean; }🤖 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/actions/deepLinking.ts` around lines 5 - 16, The exported IParams contract in deepLinking.ts is missing the OAuth-specific fields used by handleOAuth. Add credentialToken and credentialSecret as optional properties on IParams so typed callers of deepLinkingOpen with type: 'oauth' can pass them without casts or excess-property errors, and keep the shape aligned with the existing OAuth handling logic.
🧹 Nitpick comments (1)
app/views/ProfileView/index.test.tsx (1)
73-74: 📐 Maintainability & Code Quality | 🔵 TrivialUse a non-default regex in these tests.
Both tests define
UTF8_User_Names_Validationas'[0-9a-zA-Z-_.]+', which matches theDEFAULT_USERNAME_VALIDATIONconstant inapp/views/ProfileView/index.tsx. If the component completely ignored the server setting and hardcoded this fallback, the tests would still pass. To verify that the server configuration is actually wired correctly, one test should use a distinct regex pattern (e.g., allowing spaces or different characters).🤖 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/views/ProfileView/index.test.tsx` around lines 73 - 74, The ProfileView tests are using the same username validation regex as the component fallback, so they won’t catch a hardcoded default. Update one of the test setups in index.test.tsx to use a distinct UTF8_User_Names_Validation value, and verify the ProfileView behavior through the relevant test cases so the server-provided setting is actually exercised instead of DEFAULT_USERNAME_VALIDATION.
🤖 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/i18n/locales/en.json`:
- Line 1005: The Username_invalid locale copy is too specific for the
server-driven validation used by UTF8_User_Names_Validation. Update the string
in the en.json locale entry referenced by Username_invalid to a generic message
like “Username format is invalid” so it stays accurate regardless of the
server’s regex pattern.
In `@app/sagas/deepLinking.ts`:
- Line 4: The `deepLinking.ts` saga has an unused `all` import from
`typed-redux-saga`, which will trigger lint failures. Remove `all` from the
import list and keep the remaining saga helpers (`call`, `delay`, `put`, `take`,
`takeLatest`) unchanged so the module stays lint-clean.
- Around line 281-283: The predicate passed to take in deepLinking.ts should be
formatted to stay on a single line under the project’s 130-character limit.
Update the take call’s action filter so the APP.START and RootEnum.ROOT_INSIDE
check remains inline, matching the configured Prettier formatting and the
existing style in this saga.
- Around line 314-319: The accepted-call flow in deepLinking.ts is coupling
joining the conference to notifyUser and caller metadata, which can block joins
when caller is missing or notification fails. In the saga branch that handles
event === 'accept' inside the deepLinking logic, make videoConfJoin run
independently of notifyUser and guard only on the required join inputs such as
uid/callId, using caller._id only if needed for notification. Keep the
notification call separate so a notifyUser error does not prevent the join path
from continuing.
- Around line 303-311: The call-room deep link flow in handleNavigateCallRoom is
navigating too early after appStart(ROOT_INSIDE). Add the same
navigation-readiness wait used by the regular deep-link path before calling
navigateToRoom, so this saga only opens the room once the target stack is
mounted and ready.
In `@app/views/ProfileView/index.tsx`:
- Around line 95-101: The username validation in ProfileView’s validationSchema
is too strict for read-only or unchanged usernames, which can block saving
unrelated profile edits. Update the username field’s test so it only runs when
username is actually editable or has changed, and skip the
UTF8_User_Names_Validation check for legacy values when
Accounts_AllowUsernameChange is false. Use the validationSchema setup and the
username field definition in ProfileView/index.tsx to guard the isValidUsername
test with the same condition that drives the disabled username field.
---
Outside diff comments:
In `@app/actions/deepLinking.ts`:
- Around line 5-16: The exported IParams contract in deepLinking.ts is missing
the OAuth-specific fields used by handleOAuth. Add credentialToken and
credentialSecret as optional properties on IParams so typed callers of
deepLinkingOpen with type: 'oauth' can pass them without casts or
excess-property errors, and keep the shape aligned with the existing OAuth
handling logic.
---
Nitpick comments:
In `@app/views/ProfileView/index.test.tsx`:
- Around line 73-74: The ProfileView tests are using the same username
validation regex as the component fallback, so they won’t catch a hardcoded
default. Update one of the test setups in index.test.tsx to use a distinct
UTF8_User_Names_Validation value, and verify the ProfileView behavior through
the relevant test cases so the server-provided setting is actually exercised
instead of DEFAULT_USERNAME_VALIDATION.
🪄 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: 2b4cfd48-c32e-438a-a3a9-556c0c6811fa
📒 Files selected for processing (6)
app/actions/deepLinking.tsapp/i18n/locales/en.jsonapp/sagas/deepLinking.jsapp/sagas/deepLinking.tsapp/views/ProfileView/index.test.tsxapp/views/ProfileView/index.tsx
💤 Files with no reviewable changes (1)
- app/sagas/deepLinking.js
📜 Review details
🧰 Additional context used
📓 Path-based instructions (8)
**/*.{js,jsx,ts,tsx,json}
📄 CodeRabbit inference engine (CLAUDE.md)
Use Prettier formatting with tabs, single quotes, 130 character line width, no trailing commas, and avoid arrow function parentheses
Files:
app/i18n/locales/en.jsonapp/actions/deepLinking.tsapp/views/ProfileView/index.test.tsxapp/views/ProfileView/index.tsxapp/sagas/deepLinking.ts
app/i18n/**/*.{ts,tsx,json}
📄 CodeRabbit inference engine (CLAUDE.md)
Place internationalization (i18n) configuration in 'app/i18n/' directory with support for 40+ locales and RTL
Files:
app/i18n/locales/en.json
**/*.{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/actions/deepLinking.tsapp/views/ProfileView/index.test.tsxapp/views/ProfileView/index.tsxapp/sagas/deepLinking.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 numbersUse TypeScript with strict mode enabled
Files:
app/actions/deepLinking.tsapp/views/ProfileView/index.test.tsxapp/views/ProfileView/index.tsxapp/sagas/deepLinking.ts
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Enforce ESLint rules from
@rocket.chat/eslint-configwith React, React Native, TypeScript, and Jest plugins
Files:
app/actions/deepLinking.tsapp/views/ProfileView/index.test.tsxapp/views/ProfileView/index.tsxapp/sagas/deepLinking.ts
app/actions/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Place action creators in 'app/actions/' directory
Files:
app/actions/deepLinking.ts
app/views/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Place screen components in 'app/views/' directory
Files:
app/views/ProfileView/index.test.tsxapp/views/ProfileView/index.tsx
app/sagas/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Place sagas in 'app/sagas/' directory for handling side effects
Files:
app/sagas/deepLinking.ts
🧠 Learnings (2)
📚 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/actions/deepLinking.tsapp/views/ProfileView/index.test.tsxapp/views/ProfileView/index.tsxapp/sagas/deepLinking.ts
📚 Learning: 2026-06-24T22:58:43.390Z
Learnt from: Rohit3523
Repo: RocketChat/Rocket.Chat.ReactNative PR: 7157
File: app/views/MessagesView/index.tsx:392-392
Timestamp: 2026-06-24T22:58:43.390Z
Learning: When wrapping a React Native component (e.g., via `withSafeAreaInsets`) ensure `hoistNonReactStatics` is only required if the wrapped component actually defines static properties/methods that consumers rely on. If the component has no statics (as in `app/views/MessagesView/index.tsx`), you can omit `hoistNonReactStatics` for this case.
Applied to files:
app/views/ProfileView/index.test.tsxapp/views/ProfileView/index.tsx
🪛 ESLint
app/sagas/deepLinking.ts
[error] 4-4: 'all' is defined but never used.
(@typescript-eslint/no-unused-vars)
[error] 281-283: Replace ⏎↹↹↹↹↹(action:·any)·=>·action.type·===·types.APP.START·&&·action.root·===·RootEnum.ROOT_INSIDE⏎↹↹↹↹ with (action:·any)·=>·action.type·===·types.APP.START·&&·action.root·===·RootEnum.ROOT_INSIDE
(prettier/prettier)
| const handleNavigateCallRoom = function* handleNavigateCallRoom({ params }: { params: ICallPushParams }) { | ||
| try { | ||
| yield* put(appStart({ root: RootEnum.ROOT_INSIDE })); | ||
| const db = database.active; | ||
| const subsCollection = db.get('subscriptions'); | ||
| const room = yield* call([subsCollection, 'find'], params.rid); | ||
| if (room) { | ||
| const isMasterDetail = getIsMasterDetail(); | ||
| yield* call(navigateToRoom, { item: room as any, isMasterDetail }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Wait for navigation readiness before opening the call room.
This starts ROOT_INSIDE and immediately calls navigateToRoom; the regular deep-link path already waits before room navigation to avoid dispatching into an unmounted stack.
Suggested fix
try {
yield* put(appStart({ root: RootEnum.ROOT_INSIDE }));
+ yield* call(waitForNavigationReady);
const db = database.active;📝 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.
| const handleNavigateCallRoom = function* handleNavigateCallRoom({ params }: { params: ICallPushParams }) { | |
| try { | |
| yield* put(appStart({ root: RootEnum.ROOT_INSIDE })); | |
| const db = database.active; | |
| const subsCollection = db.get('subscriptions'); | |
| const room = yield* call([subsCollection, 'find'], params.rid); | |
| if (room) { | |
| const isMasterDetail = getIsMasterDetail(); | |
| yield* call(navigateToRoom, { item: room as any, isMasterDetail }); | |
| const handleNavigateCallRoom = function* handleNavigateCallRoom({ params }: { params: ICallPushParams }) { | |
| try { | |
| yield* put(appStart({ root: RootEnum.ROOT_INSIDE })); | |
| yield* call(waitForNavigationReady); | |
| const db = database.active; | |
| const subsCollection = db.get('subscriptions'); | |
| const room = yield* call([subsCollection, 'find'], params.rid); | |
| if (room) { | |
| const isMasterDetail = getIsMasterDetail(); | |
| yield* call(navigateToRoom, { item: room as any, isMasterDetail }); |
🤖 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/deepLinking.ts` around lines 303 - 311, The call-room deep link
flow in handleNavigateCallRoom is navigating too early after
appStart(ROOT_INSIDE). Add the same navigation-readiness wait used by the
regular deep-link path before calling navigateToRoom, so this saga only opens
the room once the target stack is mounted and ready.
1 participant
Description
This PR migrates the
deepLinkingsaga to TypeScript, addressing Issue #7090.Key Changes
app/sagas/deepLinking.jstoapp/sagas/deepLinking.ts, introducing strict TypeScript typing across the entire file.IParams).Summary by CodeRabbit
New Features
Bug Fixes
Tests