feat: role-targeted expiration messages#7448
Conversation
jeanfbrito
had a problem deploying
to
approve_e2e_testing
GitHub Actions
Error
|
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 ignored due to path filters (1)
📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughThe PR adds role-based visibility to supported-version messages, passes user roles into version checks, and expands tests for role-targeted selection and enforcement behavior. ChangesRole-targeted supported-version messages
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant getServerInfoSaga
participant ReduxState
participant checkSupportedVersions
participant getMessage
getServerInfoSaga->>ReduxState: read state.login.user.roles
getServerInfoSaga->>checkSupportedVersions: supportedVersions, serverVersion, userRoles
checkSupportedVersions->>getMessage: filter messages by userRoles
getMessage-->>checkSupportedVersions: selected message or none
checkSupportedVersions-->>getServerInfoSaga: supported or warn result
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/lib/methods/checkSupportedVersions.ts (1)
80-90: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDerive the enforcement-path status from
enforcementMessage.Line 80 always returns
status: 'warn', even when role filtering removes the message. That means a user outside the targeted roles can still get the warning state during theenforcementStartDategrace period, which contradicts the new visibility contract.Suggested fix
if (status === 'expired' && sv?.enforcementStartDate && new Date(sv.enforcementStartDate) > new Date()) { const enforcementMessage = getMessage({ messages, expiration: sv.enforcementStartDate, userRoles }); + const enforcementStatus = getStatus({ message: enforcementMessage, expiration: sv.enforcementStartDate }); return { - status: 'warn', + status: enforcementStatus, message: enforcementMessage, i18n: enforcementMessage ? sv?.i18n : undefined, expiration: sv.enforcementStartDate }; }🤖 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/lib/methods/checkSupportedVersions.ts` around lines 80 - 90, The enforcement branch in checkSupportedVersions should derive the returned status from the result of getMessage instead of always forcing warn. In the expired/enforcementStartDate path, use the enforcementMessage outcome to decide whether the user should see a warning or remain in the non-warning state when role filtering removes the message, and keep the returned message/i18n fields aligned with that decision.
🧹 Nitpick comments (1)
app/lib/methods/checkSupportedVersions.test.ts (1)
523-555: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd the explicit
roles: []case.The new contract treats omitted and empty
rolesthe same, but this suite only covers omission. A focusedbuildMessages([])assertion would lock down that backward-compatibility branch too.🤖 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/lib/methods/checkSupportedVersions.test.ts` around lines 523 - 555, The getMessage coverage in checkSupportedVersions.test.ts only verifies the omitted roles case, so add an explicit assertion using buildMessages([]) to confirm empty roles behaves the same as no roles. Update the existing test block around buildMessages and the getMessage expectations to include a case that passes an empty roles array and verifies the message is shown to everyone, preserving the backward-compatibility contract.
🤖 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.
Outside diff comments:
In `@app/lib/methods/checkSupportedVersions.ts`:
- Around line 80-90: The enforcement branch in checkSupportedVersions should
derive the returned status from the result of getMessage instead of always
forcing warn. In the expired/enforcementStartDate path, use the
enforcementMessage outcome to decide whether the user should see a warning or
remain in the non-warning state when role filtering removes the message, and
keep the returned message/i18n fields aligned with that decision.
---
Nitpick comments:
In `@app/lib/methods/checkSupportedVersions.test.ts`:
- Around line 523-555: The getMessage coverage in checkSupportedVersions.test.ts
only verifies the omitted roles case, so add an explicit assertion using
buildMessages([]) to confirm empty roles behaves the same as no roles. Update
the existing test block around buildMessages and the getMessage expectations to
include a case that passes an empty roles array and verifies the message is
shown to everyone, preserving the backward-compatibility contract.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a825ae0c-b9b0-4c30-a8d7-fd3716cd0562
📒 Files selected for processing (4)
app/definitions/IServer.tsapp/lib/methods/checkSupportedVersions.test.tsapp/lib/methods/checkSupportedVersions.tsapp/sagas/selectServer.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (1)
- GitHub Check: ESLint and Test / run-eslint-and-test
🧰 Additional context used
📓 Path-based instructions (6)
**/*.{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/definitions/IServer.tsapp/sagas/selectServer.tsapp/lib/methods/checkSupportedVersions.test.tsapp/lib/methods/checkSupportedVersions.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/definitions/IServer.tsapp/sagas/selectServer.tsapp/lib/methods/checkSupportedVersions.test.tsapp/lib/methods/checkSupportedVersions.ts
**/*.{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/definitions/IServer.tsapp/sagas/selectServer.tsapp/lib/methods/checkSupportedVersions.test.tsapp/lib/methods/checkSupportedVersions.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/definitions/IServer.tsapp/sagas/selectServer.tsapp/lib/methods/checkSupportedVersions.test.tsapp/lib/methods/checkSupportedVersions.ts
app/definitions/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Place shared TypeScript type definitions in 'app/definitions/' directory
Files:
app/definitions/IServer.ts
app/sagas/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Place sagas in 'app/sagas/' directory for handling side effects
Files:
app/sagas/selectServer.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/definitions/IServer.tsapp/sagas/selectServer.tsapp/lib/methods/checkSupportedVersions.test.tsapp/lib/methods/checkSupportedVersions.ts
📚 Learning: 2026-06-25T18:37:25.526Z
Learnt from: diegolmello
Repo: RocketChat/Rocket.Chat.ReactNative PR: 7434
File: app/views/ScreenLockConfigView.test.tsx:16-22
Timestamp: 2026-06-25T18:37:25.526Z
Learning: In Rocket.Chat ReactNative tests that mock selectors for `useAppSelector`, don’t require the mocked selector input to be typed as `IApplicationState` when the fixture only includes a partial Redux state slice (e.g., only `server` and `settings`). Requiring the full `IApplicationState` type in that scenario forces unsafe `as IApplicationState` casts and undermines type-safety. For these narrowly scoped selector-mock fixtures, use a less strict type (e.g., `any`) to keep the mock focused on the slice under test.
Applied to files:
app/lib/methods/checkSupportedVersions.test.ts
Honor the optional `roles` field on supportedVersions messages so a version-expiration warning can be restricted to specific roles (e.g. workspace admins) instead of every logged-in user. - TSVMessage.roles?: string[] - messageMatchesUserRoles filter threaded through getMessage and both checkSupportedVersions branches (normal + enforcement) - source current user roles from redux login state at the saga call site - tests: getMessage role filtering + checkSupportedVersions targeting Mobile side of Rocket.Chat.Electron#3371. No role-acquisition plumbing needed (bridge/REST/reducer) since mobile already has user roles natively.
diegolmello
force-pushed
the
feat/supported-versions-role-targeted-messages
branch
from
b86b121 to
fd8abff
Compare
diegolmello
temporarily deployed
to
approve_e2e_testing
GitHub Actions
Inactive
diegolmello
changed the title
…window The enforcement-start-date branch returned status 'warn' unconditionally. When a role-targeted expiration message was filtered out for a non-targeted user, it returned warn with an undefined message, surfacing an 'update required' indicator (sidebar item + header badge) that opened a blank sheet. Return 'supported' when no eligible enforcement message exists, so non-targeted members keep using the app with no warning and are not disconnected during the grace window. Claude-Session: https://claude.ai/code/session_01D7nAgeJHNRgjeerGn4YHCV
The relative-time render drifted from 'a year ago' to '2 years ago'. Unrelated to the role-targeting change; refreshes the stale snapshot so CI is green. Claude-Session: https://claude.ai/code/session_01D7nAgeJHNRgjeerGn4YHCV
diegolmello
temporarily deployed
to
approve_e2e_testing
GitHub Actions
Inactive
diegolmello
requested a deployment
to
upload_android
GitHub Actions
Waiting
|
Android Build Available Rocket.Chat 4.75.0.109254 Internal App Sharing: https://play.google.com/apps/test/RQQ8k09hlnQ/ahAO29uNTO8tj5BIVwULxSCw0mtyIejCQj4PGE74guPdS7rCkof9ZVkSGOIhgyTowPRwou8j4UlXf50f_FfRC_bZZH |
|
iOS Build Available Rocket.Chat 4.75.0.109255 |
2 participants
Jira: ARCH-2192
Proposal
Honor the optional
rolesfield on supportedVersionsmessagesso the signed payload can restrict a version-expiration warning to specific roles (e.g. workspace admins), instead of showing it to every logged-in user.This is the mobile side of the same contract change shipped on desktop in RocketChat/Rocket.Chat.Electron#3371. The
supportedVersionspayload is a shared contract (produced by cloud, consumed by desktop and mobile); this PR makes mobile respect therolesfield too.Behavior
rolesomitted or empty → message shown to everyone. Keeps current behavior; the field is backward compatible.rolespresent → message shown only to users whose roles intersect the list.rolespresent but the client doesn't know the user's roles → message is not shown (honors the restriction).Why mobile is simpler than desktop
The desktop app had no notion of user role, so most of #3371 was plumbing to acquire roles — a
setUserRolesbridge, a/api/v1/meREST fallback, aWEBVIEW_USER_ROLES_CHANGEDaction + reducer, and a per-serveruserRolesfield.Mobile is the native client and already has the logged-in user's roles in Redux (
state.login.user.roles, also persisted in the WatermelonDBUsermodel). So none of that plumbing is needed — only the two pieces that matter port over: therolestype field and the role-filtering of expiration messages.Changes
TSVMessage.roles?: string[]—app/definitions/IServer.tsmessageMatchesUserRolesfilter + auserRolesparam threaded throughgetMessageand bothcheckSupportedVersionsbranches (normal + enforcement-start-date) —app/lib/methods/checkSupportedVersions.tsgetServerInfoSaga) —app/sagas/selectServer.tsgetMessagerole filtering +checkSupportedVersionsrole targeting (7 new cases) —app/lib/methods/checkSupportedVersions.test.tsNotes
checkSupportedVersionsruns once per server-select/resume and stores the result in thesupportedVersionsRedux slice. On the resume pathsetUserruns before the check, so roles are available. On a first-ever fresh login roles may not yet be populated, in which case a role-targeted message surfaces on the next launch — consistent with desktop's async-roles semantics and acceptable for a non-urgent deprecation notice.rolesfield on the sharedsupportedVersionscontract should be agreed centrally so all clients honor it; this is the mobile side, matching the desktop change in Invalid regular expression: Quantifier has nothing to repeat Error on Xcode 12.5 #3371.Verification
Summary by CodeRabbit
New Features
Bug Fixes
Tests