feat: Show confirmation modal with compiled warnings when changing settings#40909
Merged
KevLehman merged 5 commits intoJun 12, 2026
Conversation
Contributor
|
Looks like this PR is not ready to merge, because of the following issues:
Please fix the issues and try again If you have any trouble, please check the PR guidelines |
🦋 Changeset detectedLatest commit: 5af1e11 The changes in this PR will be included in the next version bump. This PR includes changesets to release 16 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Contributor
|
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 (4)
🚧 Files skipped from review as they are similar to previous changes (1)
📜 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). (2)
🧰 Additional context used📓 Path-based instructions (1)**/*.{ts,tsx,js}📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
Files:
🧠 Learnings (4)📚 Learning: 2026-02-26T19:25:44.063ZApplied to files:
📚 Learning: 2026-02-26T19:25:44.063ZApplied to files:
📚 Learning: 2026-03-04T14:16:40.911ZApplied to files:
📚 Learning: 2026-05-06T12:21:44.083ZApplied to files:
🔇 Additional comments (2)
WalkthroughAdds a settings-confirmation modal flow and wires it into SettingsProvider; reworks Apple OAuth signing/verification to Node crypto with JWKS caching and clientId validation; tightens file-download authorization with request identity extraction; migrates Storybook config and many stories to object-based format; and adds server info fields and changesets. ChangesSettings Change Confirmation Flow
Apple OAuth JWT Verification & Client Secret Generation
File Upload Authorization & Request User ID Extraction
Storybook Configuration & Story Pattern Migration
Server API Updates & Release Changesets
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches⚔️ Resolve merge conflicts
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
added
the
type: feature
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
apps/meteor/client/hooks/useShowSettingAlerts.tsx (3)
53-53: 💤 Low valueParameter shadowing reduces readability.
The destructured
{ _id }parameter shadows_idfrom line 45. Consider using a different name for clarity.♻️ Proposed fix
-const newValue = changedSettings.find(({ _id }) => _id === persistedSetting._id)?.value; +const newValue = changedSettings.find((cs) => cs._id === persistedSetting._id)?.value;🤖 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 `@apps/meteor/client/hooks/useShowSettingAlerts.tsx` at line 53, The destructured parameter {_id} in the find callback shadows the persistedSetting._id variable and hurts readability; update the callback in the assignment to newValue to use a distinct name (e.g., destructure as { _id: changedId } or use item._id) so the comparison explicitly reads changedId === persistedSetting._id, keeping changedSettings, persistedSetting, and newValue as the referenced symbols.
31-42: ⚡ Quick winInconsistent use of
returnbeforesetModal(null).Lines 37 and 41 include
returnbeforesetModal(null), but line 33 does not. SincesetModalreturnsvoid, thereturnis unnecessary. Remove it for consistency.♻️ Proposed fix
onCancel={() => { resolve(false); - return setModal(null); + setModal(null); }} onClose={() => { resolve(false); - return setModal(null); + setModal(null); }}🤖 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 `@apps/meteor/client/hooks/useShowSettingAlerts.tsx` around lines 31 - 42, The onCancel and onClose handlers use an unnecessary `return` before calling setModal(null); since setModal returns void, remove the `return` so all three handlers (onConfirm, onCancel, onClose) consistently call setModal(null) after resolving (resolve(false) for cancel/close, resolve(true) for confirm) — update the onCancel and onClose functions that reference setModal and resolve to match the onConfirm pattern.
58-60: ⚡ Quick winTODO: Hardcoded English labels need i18n.
The "From:" and "To:" labels are hardcoded and not translated. These should use i18n keys for proper localization.
♻️ Proposed fix
- {/* TODO i18n */} - <div>From: {getSettingValueDisplay(persistedSetting.value)}</div> - <div>To: {getSettingValueDisplay(newValue)}</div> + <div>{t('From')}: {getSettingValueDisplay(persistedSetting.value)}</div> + <div>{t('To')}: {getSettingValueDisplay(newValue)}</div>🤖 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 `@apps/meteor/client/hooks/useShowSettingAlerts.tsx` around lines 58 - 60, Replace the hardcoded "From:" and "To:" labels in useShowSettingAlerts.tsx with calls to the app's i18n translation function (e.g., t or useTranslation) so the labels are localized (for example t('settings.from') and t('settings.to')), keeping the existing getSettingValueDisplay(persistedSetting.value) and getSettingValueDisplay(newValue) usage; also add the corresponding keys ("settings.from", "settings.to") to the locale resource files for all supported languages.
🤖 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 `@apps/meteor/client/providers/SettingsProvider.tsx`:
- Around line 106-122: The code calls showAlerts(alerts, ...) even when alerts
is empty, causing an unnecessary modal; change the logic in the SettingsProvider
block that computes changedSettingIds and alerts (references: changedSettingIds,
cachedCollection.store.getState(), alerts) to only call showAlerts when
alerts.length > 0 and otherwise treat the change as accepted (e.g., set accepted
= true or skip awaiting showAlerts), keeping the existing Enterprise_License
invalidate behavior (queryClient.invalidateQueries) and the subsequent check
against accepted unchanged.
---
Nitpick comments:
In `@apps/meteor/client/hooks/useShowSettingAlerts.tsx`:
- Line 53: The destructured parameter {_id} in the find callback shadows the
persistedSetting._id variable and hurts readability; update the callback in the
assignment to newValue to use a distinct name (e.g., destructure as { _id:
changedId } or use item._id) so the comparison explicitly reads changedId ===
persistedSetting._id, keeping changedSettings, persistedSetting, and newValue as
the referenced symbols.
- Around line 31-42: The onCancel and onClose handlers use an unnecessary
`return` before calling setModal(null); since setModal returns void, remove the
`return` so all three handlers (onConfirm, onCancel, onClose) consistently call
setModal(null) after resolving (resolve(false) for cancel/close, resolve(true)
for confirm) — update the onCancel and onClose functions that reference setModal
and resolve to match the onConfirm pattern.
- Around line 58-60: Replace the hardcoded "From:" and "To:" labels in
useShowSettingAlerts.tsx with calls to the app's i18n translation function
(e.g., t or useTranslation) so the labels are localized (for example
t('settings.from') and t('settings.to')), keeping the existing
getSettingValueDisplay(persistedSetting.value) and
getSettingValueDisplay(newValue) usage; also add the corresponding keys
("settings.from", "settings.to") to the locale resource files for all supported
languages.
🪄 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: 49d4ecc4-50ee-46df-9891-4b9a0d339be9
📒 Files selected for processing (6)
apps/meteor/client/hooks/useShowSettingAlerts.tsxapps/meteor/client/providers/MeteorProvider.tsxapps/meteor/client/providers/SettingsProvider.tsxapps/meteor/client/views/admin/ABAC/ABACSettingTab/SettingField.spec.tsxapps/meteor/client/views/admin/ABAC/ABACSettingTab/SettingsPage.tsxpackages/i18n/src/locales/en.i18n.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). (2)
- GitHub Check: cubic · AI code reviewer
- GitHub Check: Hacktron Security Check
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.{ts,tsx,js}: Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Avoid code comments in the implementation
Files:
apps/meteor/client/providers/MeteorProvider.tsxapps/meteor/client/hooks/useShowSettingAlerts.tsxapps/meteor/client/views/admin/ABAC/ABACSettingTab/SettingsPage.tsxapps/meteor/client/views/admin/ABAC/ABACSettingTab/SettingField.spec.tsxapps/meteor/client/providers/SettingsProvider.tsx
🧠 Learnings (3)
📚 Learning: 2026-03-27T14:52:56.865Z
Learnt from: dougfabris
Repo: RocketChat/Rocket.Chat PR: 39892
File: apps/meteor/client/views/room/contextualBar/Threads/Thread.tsx:150-155
Timestamp: 2026-03-27T14:52:56.865Z
Learning: In Rocket.Chat, there are two different `ModalBackdrop` components with different prop APIs. During review, confirm the import source: (1) `rocket.chat/fuselage` `ModalBackdrop` uses `ModalBackdropProps` based on `BoxProps` (so it supports `onClick` and other Box/DOM props) and does not have an `onDismiss` prop; (2) `rocket.chat/ui-client` `ModalBackdrop` uses a narrower props interface like `{ children?: ReactNode; onDismiss?: () => void }` and handles Escape keypress and outside mouse-up, and it does not forward arbitrary DOM props such as `onClick`. Flag mismatched props (e.g., `onDismiss` passed to the fuselage component or `onClick` passed to the ui-client component) and ensure the usage matches the correct component being imported.
Applied to files:
apps/meteor/client/providers/MeteorProvider.tsxapps/meteor/client/hooks/useShowSettingAlerts.tsxapps/meteor/client/views/admin/ABAC/ABACSettingTab/SettingsPage.tsxapps/meteor/client/views/admin/ABAC/ABACSettingTab/SettingField.spec.tsxapps/meteor/client/providers/SettingsProvider.tsx
📚 Learning: 2026-05-06T12:21:44.083Z
Learnt from: juliajforesti
Repo: RocketChat/Rocket.Chat PR: 40256
File: apps/meteor/client/components/CreateDiscussion/CreateDiscussion.tsx:121-149
Timestamp: 2026-05-06T12:21:44.083Z
Learning: Field wrappers in rocket.chat/fuselage-forms (Field, FieldLabel, FieldRow, FieldError, FieldHint) auto-create htmlFor/id associations, aria-describedby, and role="alert" for errors. Do not manually set htmlFor, id, aria-describedby, or role attributes when using these wrappers. This automatic wiring does not apply to plain rocket.chat/fuselage components, which require explicit ID wiring per the accessibility docs. In code reviews, prefer using fuselage-forms wrappers for form fields and verify there is no unnecessary manual ID/aria wiring in files that use these wrappers. If a component uses plain fuselage components, ensure proper id wiring as per docs.
Applied to files:
apps/meteor/client/providers/MeteorProvider.tsxapps/meteor/client/hooks/useShowSettingAlerts.tsxapps/meteor/client/views/admin/ABAC/ABACSettingTab/SettingsPage.tsxapps/meteor/client/views/admin/ABAC/ABACSettingTab/SettingField.spec.tsxapps/meteor/client/providers/SettingsProvider.tsx
📚 Learning: 2026-03-06T18:10:15.268Z
Learnt from: tassoevan
Repo: RocketChat/Rocket.Chat PR: 39397
File: packages/gazzodown/src/code/CodeBlock.spec.tsx:47-68
Timestamp: 2026-03-06T18:10:15.268Z
Learning: In tests (especially those using testing-library/dom/jsdom) for Rocket.Chat components, the HTML <code> element has an implicit ARIA role of 'code'. Therefore, screen.getByRole('code') or screen.findByRole('code') will locate <code> elements even without a role attribute. Do not flag findByRole('code') as invalid in reviews; prefer using the implicit role instead of adding role="code" unless necessary for accessibility.
Applied to files:
apps/meteor/client/views/admin/ABAC/ABACSettingTab/SettingField.spec.tsx
🔇 Additional comments (5)
apps/meteor/client/providers/MeteorProvider.tsx (1)
32-70: LGTM!apps/meteor/client/providers/SettingsProvider.tsx (1)
125-125: LGTM!apps/meteor/client/views/admin/ABAC/ABACSettingTab/SettingsPage.tsx (1)
21-38: Re-check ABAC_Attribute_Store i18n + modal precedence
- The i18n keys
ABAC_Attribute_Store_Switch_Title,ABAC_Attribute_Store_Switch_Confirm, andABAC_Attribute_Store_Switch_Alertare present inpackages/i18n/src/locales/en.i18n.json.ABAC_Attribute_Storeis defined withalert: 'ABAC_Attribute_Store_Switch_Alert'inapps/meteor/ee/server/settings/abac.ts, soapps/meteor/client/providers/SettingsProvider.tsx+apps/meteor/client/hooks/useShowSettingAlerts.tsxwill show the standardConfirm_setting_changeGenericModalwhenever this setting is part of the changed ids.apps/meteor/client/views/admin/ABAC/ABACSettingTab/SettingsPage.tsxpassesrenderConfirmModalforABAC_Attribute_Store, butapps/meteor/client/views/admin/ABAC/ABACSettingTab/SettingField.tsxdoesn’t referencerenderConfirmModal(it only uses the setting’salertfor an inline callout). Confirm howrenderConfirmModalinteracts with theSettingsProvideralert modal (avoid double prompts / ensure clear precedence).packages/i18n/src/locales/en.i18n.json (1)
34-35: LGTM!apps/meteor/client/views/admin/ABAC/ABACSettingTab/SettingField.spec.tsx (1)
3-3: LGTM!Also applies to: 21-21, 26-26, 36-37, 63-118
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
3 issues found across 6 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
KevLehman
force-pushed
the
chore/confirm-modal-store-change-suggestion
branch
from
9d99fab to
af88e7b
Compare
⏭️ Hacktron Security Check — SkippedReason: This PR exceeds Hacktron's 200-file review cap and will not be scanned. Split the PR into smaller changes for review coverage.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
apps/meteor/app/apple/lib/handleIdentityToken.ts (1)
73-74: 🔒 Security & Privacy | 💤 Low valueConsider validating the JWT algorithm claim.
The header's
algis parsed but not validated against the expectedRS256. While the hardcodedRSA-SHA256verification is safe against algorithm confusion attacks (invalid signatures still fail), explicit validation is defense-in-depth and would fail fast with a clear error for malformed tokens.🛡️ Proposed fix
const header = JSON.parse(decodeBase64Url(headerB64)); +if (header.alg !== 'RS256') { + console.error(`Unsupported JWT algorithm: ${header.alg}. Expected RS256`); + return null; +} const payload = JSON.parse(decodeBase64Url(payloadB64)) as AppleJWTPayload;🤖 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 `@apps/meteor/app/apple/lib/handleIdentityToken.ts` around lines 73 - 74, Validate the JWT header's alg claim after parsing (the header variable in handleIdentityToken) by checking header.alg === "RS256" and throw or return a clear error if it differs; this should be done before calling the existing RSA-SHA256 verification logic (and before using decodeBase64Url output or verifySignature) to fail fast on algorithm confusion and provide a clear message for malformed tokens.apps/meteor/app/file-upload/server/lib/FileUpload.ts (1)
101-101: 📐 Maintainability & Code Quality | ⚡ Quick winRemove the implementation comment added at Line 101.
Please move this rationale to tests/PR context and keep implementation code comment-free.
As per coding guidelines, "Avoid code comments in the implementation."♻️ Suggested diff
- // UserDataFiles are GDPR data exports — only the owner of the export may download it. const uid = await FileUpload.getRequestUserId(req);🤖 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 `@apps/meteor/app/file-upload/server/lib/FileUpload.ts` at line 101, Remove the implementation comment at the top of FileUpload.ts (the line reading "UserDataFiles are GDPR data exports — only the owner of the export may download it."); delete this in-file rationale from the FileUpload class/module and instead add the explanation to the PR description or a test file (e.g., a README or tests/NOTE) so the implementation remains comment-free per guidelines.Source: Coding guidelines
apps/meteor/client/components/UserCard/UserCard.stories.tsx (1)
37-108: 📐 Maintainability & Code Quality | ⚡ Quick winDrop
as anycasts and type stories withStoryObj<typeof meta>.At Line 43 and repeated through Line 108,
as anyremoves compile-time checks for story args. Prefer typed CSF3 stories so invalid prop shapes are caught.Proposed refactor
-import type { Meta } from '`@storybook/react`'; +import type { Meta, StoryObj } from '`@storybook/react`'; @@ -export default { +const meta = { component: UserCard, @@ -} satisfies Meta<typeof UserCard>; +} satisfies Meta<typeof UserCard>; + +export default meta; +type Story = StoryObj<typeof meta>; @@ -export const Example = {}; +export const Example: Story = {}; @@ -export const Nickname = { +export const Nickname: Story = { args: { user: { ...user, nickname: 'nicknamenickname', }, - } as any, + }, };🤖 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 `@apps/meteor/client/components/UserCard/UserCard.stories.tsx` around lines 37 - 108, The stories use unsafe "as any" casts (e.g., Nickname, LargeName, NoRoles, NoActions, NoLocalTime, NoBio, NoBioAndNoLocalTime, NoBioNoLocalTimeNoRoles) which bypass type checking; change to typed CSF3 stories by defining a Story type like "type Story = StoryObj<typeof meta>" (where meta is your Storybook meta export) and annotate each exported story as "const Nickname: Story = { args: { user: { ... } } }" (and likewise for LargeName, NoRoles, etc.), removing all "as any" casts so args are statically validated.apps/meteor/.storybook/main.ts (1)
21-23: 📐 Maintainability & Code Quality | ⚡ Quick winRemove inline implementation comments added in this TypeScript file.
These new comments conflict with the repository rule for
**/*.{ts,tsx,js}files.As per coding guidelines,
**/*.{ts,tsx,js}should “Avoid code comments in the implementation”.Also applies to: 30-30
🤖 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 `@apps/meteor/.storybook/main.ts` around lines 21 - 23, Remove the inline implementation comment block that begins with "Meteor's bundler ignores the `exports` field, so source code reaches into swiper's internals via deep file paths. Webpack honors `exports` and rejects them, so map each subpath to the actual file." in main.ts; delete that comment (and the similar inline comment around the swiper subpath mapping) so the file no longer contains implementation comments and only retains the actual mapping/configuration code.Source: Coding guidelines
🤖 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 `@apps/meteor/.storybook/main.ts`:
- Around line 24-26: The webpack alias for the Swiper react package is pointing
'swiper/swiper-react$' at the TypeScript declaration file (swiper-react.d.ts)
which breaks runtime resolution; update the alias so 'swiper/swiper-react$'
resolves to the runtime JS (swiper-react.mjs) instead of the .d.ts file,
adjusting the aliases near the swiperRoot variable and the aliases object in
main.ts so both 'swiper/swiper-react.mjs$' and 'swiper/swiper-react$' map to
join(swiperRoot, 'swiper-react.mjs') (remove the .d.ts mapping).
In `@apps/meteor/app/file-upload/server/lib/FileUpload.ts`:
- Around line 458-475: The query and header credential values (rc_uid, rc_token,
headers['x-user-id'], headers['x-auth-token']) may be string[] at runtime but
are being type-cast to string and passed into hashLoginToken and
Users.findOneByIdAndLoginToken; normalize each credential by coercing arrays to
a single string (e.g., if Array.isArray(value) take value[0] or otherwise
convert to string) for rc_uid/rc_token after URL.parse and for
headers['x-user-id']/headers['x-auth-token'] before assigning uid/authToken,
then use those normalized strings when calling hashLoginToken and
Users.findOneByIdAndLoginToken.
In `@apps/meteor/client/hooks/useShowSettingAlerts.tsx`:
- Around line 13-16: The modal is opened unconditionally even when all entries
in persistedSettingsWithAlert lack an alert; fix by first filtering
persistedSettingsWithAlert for items with a non-empty alert (the same check used
around the skipped entries at the 35-38 logic), and if that filtered list is
empty immediately call resolve(true) and return without calling
setModal/GenericModal; otherwise proceed to open the modal as before. Ensure you
update the handler that receives persistedSettingsWithAlert so it uses this
short-circuit check before invoking setModal.
---
Nitpick comments:
In `@apps/meteor/.storybook/main.ts`:
- Around line 21-23: Remove the inline implementation comment block that begins
with "Meteor's bundler ignores the `exports` field, so source code reaches into
swiper's internals via deep file paths. Webpack honors `exports` and rejects
them, so map each subpath to the actual file." in main.ts; delete that comment
(and the similar inline comment around the swiper subpath mapping) so the file
no longer contains implementation comments and only retains the actual
mapping/configuration code.
In `@apps/meteor/app/apple/lib/handleIdentityToken.ts`:
- Around line 73-74: Validate the JWT header's alg claim after parsing (the
header variable in handleIdentityToken) by checking header.alg === "RS256" and
throw or return a clear error if it differs; this should be done before calling
the existing RSA-SHA256 verification logic (and before using decodeBase64Url
output or verifySignature) to fail fast on algorithm confusion and provide a
clear message for malformed tokens.
In `@apps/meteor/app/file-upload/server/lib/FileUpload.ts`:
- Line 101: Remove the implementation comment at the top of FileUpload.ts (the
line reading "UserDataFiles are GDPR data exports — only the owner of the export
may download it."); delete this in-file rationale from the FileUpload
class/module and instead add the explanation to the PR description or a test
file (e.g., a README or tests/NOTE) so the implementation remains comment-free
per guidelines.
In `@apps/meteor/client/components/UserCard/UserCard.stories.tsx`:
- Around line 37-108: The stories use unsafe "as any" casts (e.g., Nickname,
LargeName, NoRoles, NoActions, NoLocalTime, NoBio, NoBioAndNoLocalTime,
NoBioNoLocalTimeNoRoles) which bypass type checking; change to typed CSF3
stories by defining a Story type like "type Story = StoryObj<typeof meta>"
(where meta is your Storybook meta export) and annotate each exported story as
"const Nickname: Story = { args: { user: { ... } } }" (and likewise for
LargeName, NoRoles, etc.), removing all "as any" casts so args are statically
validated.
🪄 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: 087d49a2-f8d9-4e0b-b392-fad40c302b24
⛔ Files ignored due to path filters (1)
apps/meteor/.storybook/logo.svgis excluded by!**/*.svg
📒 Files selected for processing (34)
.changeset/all-readers-reply.md.changeset/bumpy-coats-reply.md.changeset/rich-bananas-shine.mdapps/meteor/.storybook/decorators.tsxapps/meteor/.storybook/logo.svg.d.tsapps/meteor/.storybook/main.tsapps/meteor/.storybook/preview.tsapps/meteor/app/api/server/lib/getServerInfo.tsapps/meteor/app/apple/lib/handleIdentityToken.spec.tsapps/meteor/app/apple/lib/handleIdentityToken.tsapps/meteor/app/apple/server/AppleCustomOAuth.tsapps/meteor/app/apple/server/appleOauthRegisterService.tsapps/meteor/app/apple/server/loginHandler.spec.tsapps/meteor/app/apple/server/loginHandler.tsapps/meteor/app/file-upload/server/lib/FileUpload.spec.tsapps/meteor/app/file-upload/server/lib/FileUpload.tsapps/meteor/client/components/ABAC/ABACUpsellModal/ABACUpsellModal.stories.tsxapps/meteor/client/components/CreateDiscussion/CreateDiscussion.stories.tsxapps/meteor/client/components/GenericUpsellModal/GenericUpsellModal.stories.tsxapps/meteor/client/components/ListItem.stories.tsxapps/meteor/client/components/SidebarToggler/SidebarTogglerButton.stories.tsxapps/meteor/client/components/UserAutoCompleteMultiple/UserAvatarChip.stories.tsxapps/meteor/client/components/UserCard/UserCard.stories.tsxapps/meteor/client/components/UserInfo/UserInfo.stories.tsxapps/meteor/client/components/connectionStatus/ConnectionStatusBar.stories.tsxapps/meteor/client/components/dataView/Counter.stories.tsxapps/meteor/client/components/dataView/CounterSet.stories.tsxapps/meteor/client/components/dataView/NegativeGrowthSymbol.stories.tsxapps/meteor/client/components/dataView/PositiveGrowthSymbol.stories.tsxapps/meteor/client/components/message/content/actions/MessageAction.stories.tsxapps/meteor/client/components/message/header/MessageRoles.stories.tsxapps/meteor/client/components/message/toolbar/items/actions/Timestamp/TimestampPicker/TimestampPicker.stories.tsxapps/meteor/client/components/message/variants/threadPreview/ThreadMessagePreviewBody.stories.tsxapps/meteor/client/hooks/useShowSettingAlerts.tsx
💤 Files with no reviewable changes (1)
- apps/meteor/.storybook/logo.svg.d.ts
✅ Files skipped from review due to trivial changes (6)
- .changeset/bumpy-coats-reply.md
- apps/meteor/client/components/UserAutoCompleteMultiple/UserAvatarChip.stories.tsx
- apps/meteor/client/components/ABAC/ABACUpsellModal/ABACUpsellModal.stories.tsx
- apps/meteor/client/components/CreateDiscussion/CreateDiscussion.stories.tsx
- .changeset/all-readers-reply.md
- .changeset/rich-bananas-shine.md
📜 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: CodeQL-Build
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.{ts,tsx,js}: Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Avoid code comments in the implementation
Files:
apps/meteor/client/components/message/variants/threadPreview/ThreadMessagePreviewBody.stories.tsxapps/meteor/app/api/server/lib/getServerInfo.tsapps/meteor/client/components/message/toolbar/items/actions/Timestamp/TimestampPicker/TimestampPicker.stories.tsxapps/meteor/client/components/dataView/CounterSet.stories.tsxapps/meteor/client/components/message/content/actions/MessageAction.stories.tsxapps/meteor/app/apple/server/AppleCustomOAuth.tsapps/meteor/client/components/GenericUpsellModal/GenericUpsellModal.stories.tsxapps/meteor/client/components/dataView/NegativeGrowthSymbol.stories.tsxapps/meteor/client/components/message/header/MessageRoles.stories.tsxapps/meteor/client/components/UserInfo/UserInfo.stories.tsxapps/meteor/client/hooks/useShowSettingAlerts.tsxapps/meteor/client/components/dataView/Counter.stories.tsxapps/meteor/app/apple/lib/handleIdentityToken.spec.tsapps/meteor/app/apple/server/loginHandler.spec.tsapps/meteor/client/components/dataView/PositiveGrowthSymbol.stories.tsxapps/meteor/client/components/connectionStatus/ConnectionStatusBar.stories.tsxapps/meteor/client/components/UserCard/UserCard.stories.tsxapps/meteor/app/apple/lib/handleIdentityToken.tsapps/meteor/app/file-upload/server/lib/FileUpload.spec.tsapps/meteor/client/components/SidebarToggler/SidebarTogglerButton.stories.tsxapps/meteor/client/components/ListItem.stories.tsxapps/meteor/app/apple/server/appleOauthRegisterService.tsapps/meteor/app/apple/server/loginHandler.tsapps/meteor/app/file-upload/server/lib/FileUpload.ts
**/*.spec.ts
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.spec.ts: Use descriptive test names that clearly communicate expected behavior in Playwright tests
Use.spec.tsextension for test files (e.g.,login.spec.ts)
Files:
apps/meteor/app/apple/lib/handleIdentityToken.spec.tsapps/meteor/app/apple/server/loginHandler.spec.tsapps/meteor/app/file-upload/server/lib/FileUpload.spec.ts
🧠 Learnings (6)
📚 Learning: 2026-03-27T14:52:56.865Z
Learnt from: dougfabris
Repo: RocketChat/Rocket.Chat PR: 39892
File: apps/meteor/client/views/room/contextualBar/Threads/Thread.tsx:150-155
Timestamp: 2026-03-27T14:52:56.865Z
Learning: In Rocket.Chat, there are two different `ModalBackdrop` components with different prop APIs. During review, confirm the import source: (1) `rocket.chat/fuselage` `ModalBackdrop` uses `ModalBackdropProps` based on `BoxProps` (so it supports `onClick` and other Box/DOM props) and does not have an `onDismiss` prop; (2) `rocket.chat/ui-client` `ModalBackdrop` uses a narrower props interface like `{ children?: ReactNode; onDismiss?: () => void }` and handles Escape keypress and outside mouse-up, and it does not forward arbitrary DOM props such as `onClick`. Flag mismatched props (e.g., `onDismiss` passed to the fuselage component or `onClick` passed to the ui-client component) and ensure the usage matches the correct component being imported.
Applied to files:
apps/meteor/client/components/message/variants/threadPreview/ThreadMessagePreviewBody.stories.tsxapps/meteor/client/components/message/toolbar/items/actions/Timestamp/TimestampPicker/TimestampPicker.stories.tsxapps/meteor/client/components/dataView/CounterSet.stories.tsxapps/meteor/client/components/message/content/actions/MessageAction.stories.tsxapps/meteor/client/components/GenericUpsellModal/GenericUpsellModal.stories.tsxapps/meteor/client/components/dataView/NegativeGrowthSymbol.stories.tsxapps/meteor/client/components/message/header/MessageRoles.stories.tsxapps/meteor/client/components/UserInfo/UserInfo.stories.tsxapps/meteor/client/hooks/useShowSettingAlerts.tsxapps/meteor/client/components/dataView/Counter.stories.tsxapps/meteor/client/components/dataView/PositiveGrowthSymbol.stories.tsxapps/meteor/client/components/connectionStatus/ConnectionStatusBar.stories.tsxapps/meteor/client/components/UserCard/UserCard.stories.tsxapps/meteor/client/components/SidebarToggler/SidebarTogglerButton.stories.tsxapps/meteor/client/components/ListItem.stories.tsx
📚 Learning: 2026-05-06T12:21:44.083Z
Learnt from: juliajforesti
Repo: RocketChat/Rocket.Chat PR: 40256
File: apps/meteor/client/components/CreateDiscussion/CreateDiscussion.tsx:121-149
Timestamp: 2026-05-06T12:21:44.083Z
Learning: Field wrappers in rocket.chat/fuselage-forms (Field, FieldLabel, FieldRow, FieldError, FieldHint) auto-create htmlFor/id associations, aria-describedby, and role="alert" for errors. Do not manually set htmlFor, id, aria-describedby, or role attributes when using these wrappers. This automatic wiring does not apply to plain rocket.chat/fuselage components, which require explicit ID wiring per the accessibility docs. In code reviews, prefer using fuselage-forms wrappers for form fields and verify there is no unnecessary manual ID/aria wiring in files that use these wrappers. If a component uses plain fuselage components, ensure proper id wiring as per docs.
Applied to files:
apps/meteor/client/components/message/variants/threadPreview/ThreadMessagePreviewBody.stories.tsxapps/meteor/app/api/server/lib/getServerInfo.tsapps/meteor/client/components/message/toolbar/items/actions/Timestamp/TimestampPicker/TimestampPicker.stories.tsxapps/meteor/client/components/dataView/CounterSet.stories.tsxapps/meteor/client/components/message/content/actions/MessageAction.stories.tsxapps/meteor/app/apple/server/AppleCustomOAuth.tsapps/meteor/client/components/GenericUpsellModal/GenericUpsellModal.stories.tsxapps/meteor/client/components/dataView/NegativeGrowthSymbol.stories.tsxapps/meteor/client/components/message/header/MessageRoles.stories.tsxapps/meteor/client/components/UserInfo/UserInfo.stories.tsxapps/meteor/client/hooks/useShowSettingAlerts.tsxapps/meteor/client/components/dataView/Counter.stories.tsxapps/meteor/app/apple/lib/handleIdentityToken.spec.tsapps/meteor/app/apple/server/loginHandler.spec.tsapps/meteor/client/components/dataView/PositiveGrowthSymbol.stories.tsxapps/meteor/client/components/connectionStatus/ConnectionStatusBar.stories.tsxapps/meteor/client/components/UserCard/UserCard.stories.tsxapps/meteor/app/apple/lib/handleIdentityToken.tsapps/meteor/app/file-upload/server/lib/FileUpload.spec.tsapps/meteor/client/components/SidebarToggler/SidebarTogglerButton.stories.tsxapps/meteor/client/components/ListItem.stories.tsxapps/meteor/app/apple/server/appleOauthRegisterService.tsapps/meteor/app/apple/server/loginHandler.tsapps/meteor/app/file-upload/server/lib/FileUpload.ts
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In the Rocket.Chat repository, do not reference Biome lint rules in code review feedback. Biome is not used even if biome.json exists; only reference Biome rules if there is explicit, project-wide usage documented. For TypeScript files, review lint implications without Biome guidance unless the project enables Biome rules.
Applied to files:
apps/meteor/app/api/server/lib/getServerInfo.tsapps/meteor/app/apple/server/AppleCustomOAuth.tsapps/meteor/app/apple/lib/handleIdentityToken.spec.tsapps/meteor/app/apple/server/loginHandler.spec.tsapps/meteor/app/apple/lib/handleIdentityToken.tsapps/meteor/app/file-upload/server/lib/FileUpload.spec.tsapps/meteor/app/apple/server/appleOauthRegisterService.tsapps/meteor/app/apple/server/loginHandler.tsapps/meteor/app/file-upload/server/lib/FileUpload.ts
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In this repository (RocketChat/Rocket.Chat), Biome lint rules are not used even if a biome.json exists. When reviewing TypeScript files (e.g., packages/ui-voip/src/providers/useMediaSession.ts), ensure lint suggestions do not reference Biome-specific rules. Rely on general ESLint/TypeScript lint rules and project conventions instead.
Applied to files:
apps/meteor/app/api/server/lib/getServerInfo.tsapps/meteor/app/apple/server/AppleCustomOAuth.tsapps/meteor/app/apple/lib/handleIdentityToken.spec.tsapps/meteor/app/apple/server/loginHandler.spec.tsapps/meteor/app/apple/lib/handleIdentityToken.tsapps/meteor/app/file-upload/server/lib/FileUpload.spec.tsapps/meteor/app/apple/server/appleOauthRegisterService.tsapps/meteor/app/apple/server/loginHandler.tsapps/meteor/app/file-upload/server/lib/FileUpload.ts
📚 Learning: 2026-02-24T19:22:48.358Z
Learnt from: juliajforesti
Repo: RocketChat/Rocket.Chat PR: 38493
File: apps/meteor/tests/e2e/omnichannel/omnichannel-send-pdf-transcript.spec.ts:66-67
Timestamp: 2026-02-24T19:22:48.358Z
Learning: In Playwright end-to-end tests (e.g., under apps/meteor/tests/e2e/...), prefer locating elements by translated text (getByText) and ARIA roles (getByRole) over data-qa attributes. If translation values change, update the corresponding test locators accordingly. Never use data-qa locators. This guideline applies to all Playwright e2e test specs in the repository and helps keep tests robust to UI text changes and accessible semantics.
Applied to files:
apps/meteor/app/apple/lib/handleIdentityToken.spec.tsapps/meteor/app/apple/server/loginHandler.spec.tsapps/meteor/app/file-upload/server/lib/FileUpload.spec.ts
📚 Learning: 2026-03-06T18:10:15.268Z
Learnt from: tassoevan
Repo: RocketChat/Rocket.Chat PR: 39397
File: packages/gazzodown/src/code/CodeBlock.spec.tsx:47-68
Timestamp: 2026-03-06T18:10:15.268Z
Learning: In tests (especially those using testing-library/dom/jsdom) for Rocket.Chat components, the HTML <code> element has an implicit ARIA role of 'code'. Therefore, screen.getByRole('code') or screen.findByRole('code') will locate <code> elements even without a role attribute. Do not flag findByRole('code') as invalid in reviews; prefer using the implicit role instead of adding role="code" unless necessary for accessibility.
Applied to files:
apps/meteor/app/apple/lib/handleIdentityToken.spec.tsapps/meteor/app/apple/server/loginHandler.spec.tsapps/meteor/app/file-upload/server/lib/FileUpload.spec.ts
🔇 Additional comments (39)
apps/meteor/client/hooks/useShowSettingAlerts.tsx (1)
16-33: LGTM!Also applies to: 40-71
apps/meteor/app/api/server/lib/getServerInfo.ts (1)
2-2: LGTM!Also applies to: 19-20
apps/meteor/app/apple/lib/handleIdentityToken.ts (5)
1-29: LGTM!
31-61: LGTM!
63-65: LGTM!
117-139: LGTM!
141-166: LGTM!apps/meteor/app/apple/server/appleOauthRegisterService.ts (5)
1-10: LGTM!
11-32: LGTM!
49-69: LGTM!
71-108: LGTM!
109-123: LGTM!apps/meteor/app/apple/server/AppleCustomOAuth.ts (1)
5-6: LGTM!Also applies to: 19-22
apps/meteor/app/apple/server/loginHandler.ts (1)
16-37: LGTM!apps/meteor/app/apple/lib/handleIdentityToken.spec.ts (4)
1-31: LGTM!
32-44: LGTM!
46-88: LGTM!
90-133: LGTM!apps/meteor/app/apple/server/loginHandler.spec.ts (4)
1-62: LGTM!
63-83: LGTM!
85-105: LGTM!
107-124: LGTM!apps/meteor/app/file-upload/server/lib/FileUpload.ts (1)
499-499: LGTM!apps/meteor/app/file-upload/server/lib/FileUpload.spec.ts (1)
291-359: LGTM!Also applies to: 362-470
apps/meteor/.storybook/decorators.tsx (1)
11-25: LGTM!apps/meteor/.storybook/preview.ts (1)
1-17: LGTM!apps/meteor/client/components/GenericUpsellModal/GenericUpsellModal.stories.tsx (1)
2-2: LGTM!apps/meteor/client/components/UserInfo/UserInfo.stories.tsx (1)
36-71: LGTM!apps/meteor/client/components/ListItem.stories.tsx (1)
2-2: LGTM!Also applies to: 45-68, 70-92
apps/meteor/client/components/dataView/Counter.stories.tsx (1)
15-40: LGTM!apps/meteor/client/components/dataView/CounterSet.stories.tsx (1)
1-1: LGTM!Also applies to: 13-23
apps/meteor/client/components/dataView/NegativeGrowthSymbol.stories.tsx (1)
2-2: LGTM!Also applies to: 22-24
apps/meteor/client/components/dataView/PositiveGrowthSymbol.stories.tsx (1)
2-2: LGTM!Also applies to: 22-24
apps/meteor/client/components/message/header/MessageRoles.stories.tsx (1)
2-2: LGTM!Also applies to: 11-22
apps/meteor/client/components/SidebarToggler/SidebarTogglerButton.stories.tsx (1)
2-2: LGTM!Also applies to: 16-16, 18-21
apps/meteor/client/components/connectionStatus/ConnectionStatusBar.stories.tsx (1)
4-4: LGTM!Also applies to: 26-36, 38-48, 50-60, 62-72, 74-84
apps/meteor/client/components/message/content/actions/MessageAction.stories.tsx (1)
1-2: LGTM!Also applies to: 13-18
apps/meteor/client/components/message/toolbar/items/actions/Timestamp/TimestampPicker/TimestampPicker.stories.tsx (1)
3-3: LGTM!apps/meteor/client/components/message/variants/threadPreview/ThreadMessagePreviewBody.stories.tsx (1)
2-2: LGTM!Also applies to: 28-28
KevLehman
force-pushed
the
chore/confirm-modal-store-change
branch
from
ef81453 to
57c3a34
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Proposed changes (including videos or screenshots)
Issue(s)
Steps to test or reproduce
Further comments
Summary by CodeRabbit
New Features
Bug Fixes
Improvements
Tests
Documentation
Refactor