fix(notifications): deduplicate stale video conference notifications on cold boot#7440
fix(notifications): deduplicate stale video conference notifications on cold boot#7440Shevilll 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 (2)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughAdds persistent deduplication for video conference notification handling and updates ProfileView username validation to match the server setting, with localization and tests. ChangesNotification deduplication
Profile username validation
Estimated code review effort: 4 (Complex) | ~45 minutes Suggested labels: 🚥 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: 4
🤖 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 localization string is hardcoded to a specific
allowed character set, but it should reflect the configurable username validator
instead. Update the localized copy in the en.json locale entry so the message is
generic and matches whichever regex is enforced by the server, and keep the
change scoped to the Username_invalid key used for username validation feedback.
In `@app/lib/notifications/deduplicator.ts`:
- Around line 10-19: The dedup helpers in NotificationDeduplicator are only
logging and returning when
UserPreferences.getString(NOTIFICATION_DEDUPLICATOR_KEY) contains malformed
data, which leaves the stored state poisoned. Update the read path in
isNotificationProcessed and the write path in the companion setter so they
validate the parsed value as a string array and, on parse/shape failure, clear
or reset NOTIFICATION_DEDUPLICATOR_KEY before returning. Keep the existing
warning logs, but ensure invalid stored state is removed so future reads and
writes can recover normally.
In `@app/lib/notifications/videoConf/getInitialNotification.ts`:
- Line 114: The getInitialNotification formatting needs cleanup: remove the
extra trailing blank line at the end of the function or file so Prettier no
longer flags a Delete ⏎ issue. Check the getInitialNotification.ts content
around the getInitialNotification implementation and ensure the final lines end
cleanly with no stray empty line.
In `@app/views/ProfileView/index.tsx`:
- Around line 95-101: The username validation in ProfileView’s validationSchema
is applied on every submit, which blocks saving unrelated profile changes when
the existing username no longer matches the latest UTF8_User_Names_Validation.
Update the logic in ProfileView so username is only validated when it is
actually editable or changed (for example, when Accounts_AllowUsernameChange
allows edits and the field value differs from the current account username), and
keep the existing Yup test and isValidUsername check scoped to that case.
🪄 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: fe74c5d2-321a-414c-aebd-3df23d253913
📒 Files selected for processing (7)
app/i18n/locales/en.jsonapp/lib/notifications/deduplicator.tsapp/lib/notifications/index.tsapp/lib/notifications/push.tsapp/lib/notifications/videoConf/getInitialNotification.tsapp/views/ProfileView/index.test.tsxapp/views/ProfileView/index.tsx
📜 Review details
🧰 Additional context used
📓 Path-based instructions (6)
**/*.{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/lib/notifications/deduplicator.tsapp/views/ProfileView/index.test.tsxapp/lib/notifications/push.tsapp/lib/notifications/videoConf/getInitialNotification.tsapp/lib/notifications/index.tsapp/views/ProfileView/index.tsx
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/lib/notifications/deduplicator.tsapp/views/ProfileView/index.test.tsxapp/lib/notifications/push.tsapp/lib/notifications/videoConf/getInitialNotification.tsapp/lib/notifications/index.tsapp/views/ProfileView/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 enabled
Files:
app/lib/notifications/deduplicator.tsapp/views/ProfileView/index.test.tsxapp/lib/notifications/push.tsapp/lib/notifications/videoConf/getInitialNotification.tsapp/lib/notifications/index.tsapp/views/ProfileView/index.tsx
**/*.{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/lib/notifications/deduplicator.tsapp/views/ProfileView/index.test.tsxapp/lib/notifications/push.tsapp/lib/notifications/videoConf/getInitialNotification.tsapp/lib/notifications/index.tsapp/views/ProfileView/index.tsx
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
🧠 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/lib/notifications/deduplicator.tsapp/views/ProfileView/index.test.tsxapp/lib/notifications/push.tsapp/lib/notifications/videoConf/getInitialNotification.tsapp/lib/notifications/index.tsapp/views/ProfileView/index.tsx
📚 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/lib/notifications/videoConf/getInitialNotification.ts
[error] 114-115: Delete ⏎
(prettier/prettier)
🔇 Additional comments (5)
app/lib/notifications/push.ts (1)
4-12: LGTM!Also applies to: 164-189, 293-302
app/lib/notifications/index.ts (1)
9-9: LGTM!Also applies to: 28-39, 56-67
app/lib/notifications/videoConf/getInitialNotification.ts (1)
8-8: LGTM!Also applies to: 20-26, 52-58, 84-96
app/views/ProfileView/index.test.tsx (1)
12-12: LGTM!Also applies to: 73-74, 115-135
app/views/ProfileView/index.tsx (1)
53-60: 🎯 Functional CorrectnessIncorrect assessment.
The claim that the pattern
[0-9a-zA-Z-_.]+accepts characters like[,\,], or^is factually wrong. Verification confirms that both the original pattern and the suggested alternative correctly reject inputs containing these special characters. While placing the hyphen at the end is stylistically preferable to avoid ambiguity, the original code does not functionally accept invalid characters.> Likely an incorrect or invalid review comment.
| "User_not_found_or": "User not found or incorrect password", | ||
| "User_sent_an_attachment": "{{user}} sent an attachment", | ||
| "Username": "Username", | ||
| "Username_invalid": "Use only letters, numbers, dots, hyphens and underscores", |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Make the error text match the configurable validator.
Username_invalid now backs a server-defined regex, but this copy hardcodes the fallback character set. On workspaces with a custom UTF8_User_Names_Validation, users will get the wrong guidance for a rejection that came from a different pattern.
Suggested copy
- "Username_invalid": "Use only letters, numbers, dots, hyphens and underscores",
+ "Username_invalid": "Username format is invalid for this workspace",📝 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.
| "Username_invalid": "Use only letters, numbers, dots, hyphens and underscores", | |
| "Username_invalid": "Username format is invalid for this workspace", |
🤖 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/i18n/locales/en.json` at line 1005, The Username_invalid localization
string is hardcoded to a specific allowed character set, but it should reflect
the configurable username validator instead. Update the localized copy in the
en.json locale entry so the message is generic and matches whichever regex is
enforced by the server, and keep the change scoped to the Username_invalid key
used for username validation feedback.
| const validationSchema = yup.object().shape({ | ||
| name: yup.string().required(I18n.t('Name_required')), | ||
| email: yup.string().email(I18n.t('Email_must_be_a_valid_email')).required(I18n.t('Email_required')), | ||
| username: yup | ||
| .string() | ||
| .required(I18n.t('Username_required')) | ||
| .test('valid-username', I18n.t('Username_invalid'), value => isValidUsername(value ?? '', UTF8_User_Names_Validation)) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Only validate usernames that the user is actually changing.
Line 101 re-checks the current username against the latest server regex on every submit. If a workspace tightens UTF8_User_Names_Validation, any legacy username that no longer matches will block unrelated profile edits, and that becomes impossible to recover from when Accounts_AllowUsernameChange is false because the field is locked in the same component.
Suggested fix
username: yup
.string()
.required(I18n.t('Username_required'))
- .test('valid-username', I18n.t('Username_invalid'), value => isValidUsername(value ?? '', UTF8_User_Names_Validation))
+ .test('valid-username', I18n.t('Username_invalid'), value => {
+ if ((value ?? '') === (user?.username ?? '')) {
+ return true;
+ }
+ return isValidUsername(value ?? '', UTF8_User_Names_Validation);
+ })
});📝 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 validationSchema = yup.object().shape({ | |
| name: yup.string().required(I18n.t('Name_required')), | |
| email: yup.string().email(I18n.t('Email_must_be_a_valid_email')).required(I18n.t('Email_required')), | |
| username: yup | |
| .string() | |
| .required(I18n.t('Username_required')) | |
| .test('valid-username', I18n.t('Username_invalid'), value => isValidUsername(value ?? '', UTF8_User_Names_Validation)) | |
| username: yup | |
| .string() | |
| .required(I18n.t('Username_required')) | |
| .test('valid-username', I18n.t('Username_invalid'), value => { | |
| if ((value ?? '') === (user?.username ?? '')) { | |
| return true; | |
| } | |
| return isValidUsername(value ?? '', UTF8_User_Names_Validation); | |
| }) |
🤖 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.tsx` around lines 95 - 101, The username
validation in ProfileView’s validationSchema is applied on every submit, which
blocks saving unrelated profile changes when the existing username no longer
matches the latest UTF8_User_Names_Validation. Update the logic in ProfileView
so username is only validated when it is actually editable or changed (for
example, when Accounts_AllowUsernameChange allows edits and the field value
differs from the current account username), and keep the existing Yup test and
isValidUsername check scoped to that case.
…age validation and spacing
1 participant
Description
Cold boots re-dispatched duplicate stale video conference notifications. This PR introduces a persistent ID deduplicator to resolve this issue by tracking and discarding already-processed notification IDs.
Changes
NotificationDeduplicatorutilizingUserPreferencesto store processed notification IDs.Summary by CodeRabbit
New Features
Bug Fixes
Tests