fix: incorrect e2ee toggle behavior on Teams creation modal and private/broadcast toggling #36797
Conversation
|
Looks like this PR is ready to merge! 🎉 |
|
I have cherry picked the commit from the PR #36792 as it was the hotfix that should be present for this fix. |
🦋 Changeset detectedLatest commit: 8cf32fd The changes in this PR will be included in the next version bump. This PR includes changesets to release 40 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 |
abhinavkrin
force-pushed
the
fix/e2ee-toggle-behavior-on-teams
branch
from
e80ab03 to
8822b81
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #36797 +/- ##
===========================================
+ Coverage 66.25% 66.42% +0.17%
===========================================
Files 3339 3341 +2
Lines 113725 114284 +559
Branches 21064 21179 +115
===========================================
+ Hits 75347 75915 +568
+ Misses 35687 35683 -4
+ Partials 2691 2686 -5
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
…adcast toggling Signed-off-by: Abhinav Kumar <[email protected]>
Signed-off-by: Abhinav Kumar <[email protected]>
abhinavkrin
force-pushed
the
fix/e2ee-toggle-behavior-on-teams
branch
from
8822b81 to
8a7b9f6
Compare
dougfabris
left a comment
dougfabris
left a comment
There was a problem hiding this comment.
Nice work here, I just have a concern regarding set-readonly
apps/meteor/client/NavBarV2/NavBarPagesGroup/actions/CreateTeamModal.tsx
Show resolved
Hide resolved
Signed-off-by: Abhinav Kumar <[email protected]>
WalkthroughThis change updates Create Team modals and related hooks to decouple encryption from broadcast, gate read‑only by permission, and align encryption defaults with settings. It removes broadcast-specific encryption messaging and translations, adds comprehensive tests, and includes a changeset noting a patch bump and the Teams modal bug fix scope. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor U as User
participant M as CreateTeamModal
participant S as Settings (E2E_Enable, E2E_Default_Private)
participant P as Permissions (set-readonly)
U->>M: Open modal
M->>S: Read E2E settings
S-->>M: e2eEnabled, e2eDefaultPrivate
M->>M: Compute encrypted default = e2eEnabled && e2eDefaultPrivate
M->>P: Check 'set-readonly' (owner-scoped)
P-->>M: canSetReadOnly
M->>M: readOnlyDisabled = broadcast || !canSetReadOnly
M->>M: canChangeEncrypted = isPrivate && e2eEnabled
Note right of M: Encryption no longer depends on broadcast or read-only permission
U->>M: Toggle Private/Broadcast/Read-only/Encrypted
alt Toggle Read-only
M->>M: Allow only if !readOnlyDisabled
end
alt Toggle Encrypted
M->>M: Allow only if canChangeEncrypted
end
M-->>U: Update UI states and hints (no broadcast-specific hint)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Pre-merge checks (3 passed)✅ Passed checks (3 passed)
Poem
✨ Finishing Touches
🧪 Generate unit tests
Comment |
|
Note Unit test generation is an Early Access feature. Expect some limitations and changes as we gather feedback and continue to improve it. Generating unit tests... This may take up to 20 minutes. |
![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
♻️ Duplicate comments (1)
apps/meteor/client/NavBarV2/NavBarPagesGroup/actions/CreateTeamModal.tsx (1)
59-59: Kept scoped permission for read-only — aligns with channel behavior.
🧹 Nitpick comments (16)
.changeset/rare-walls-press.md (1)
1-6: Clarify the changeset and reference tickets explicitly.Recommend tightening the summary and referencing CORE-1326/CORE-1327 so the release notes are self-explanatory. Also mention the i18n key removal for completeness.
--- '@rocket.chat/i18n': patch '@rocket.chat/meteor': patch --- -Fixes an issue where the encryption toggle was incorrectly reset/disabled/enabled in the Teams creation modal when Broadcast or Private was toggled, or when the user lacked unrelated permissions. +fix(teams): correct E2EE toggle behavior in Create Team modal + +- Remove dependency on `set-readonly` for the E2EE toggle availability. +- Disable and turn OFF E2EE when "Private" is OFF; keep configurable when toggling "Broadcast". +- Align encrypted room description logic (drop broadcast-specific message). +- Remove `Not_available_for_broadcast` i18n key across locales. + +Refs CORE-1326, CORE-1327.apps/meteor/client/sidebar/header/hooks/useEncryptedRoomDescription.tsx (1)
8-19: Return a memoized callback to avoid unstable function identity.If consumers place this function in deps, it will currently change every render. Wrap it in useCallback.
-import { useTranslation } from 'react-i18next'; +import { useTranslation } from 'react-i18next'; +import { useCallback } from 'react'; export const useEncryptedRoomDescription = (roomType: 'channel' | 'team') => { const { t } = useTranslation(); const e2eEnabled = useSetting('E2E_Enable'); - return ({ isPrivate, encrypted }: { isPrivate: boolean; encrypted: boolean }) => { + return useCallback(({ isPrivate, encrypted }: { isPrivate: boolean; encrypted: boolean }) => { if (!e2eEnabled) { return t('Not_available_for_this_workspace'); } if (!isPrivate) { return t('Encrypted_not_available', { roomType }); } if (encrypted) { return t('Encrypted_messages', { roomType }); } return t('Encrypted_messages_false'); - }; + }, [e2eEnabled, t, roomType]); };apps/meteor/client/sidebar/header/CreateTeam/CreateTeamModal.tsx (4)
53-55: Coerce settings to booleans to avoidunknowntyping and truthy pitfalls.Prevents surprising values if settings are non-boolean or temporarily undefined.
-const e2eEnabled = useSetting('E2E_Enable'); -const e2eEnabledForPrivateByDefault = useSetting('E2E_Enabled_Default_PrivateRooms') && e2eEnabled; +const e2eEnabled = Boolean(useSetting('E2E_Enable')); +const e2eEnabledForPrivateByDefault = Boolean(useSetting('E2E_Enabled_Default_PrivateRooms')) && e2eEnabled;
97-104: Drop the cast and rely on boolean-coerced settings.defaultValues: { isPrivate: canOnlyCreateOneType ? canOnlyCreateOneType === 'p' : true, readOnly: false, - encrypted: (e2eEnabledForPrivateByDefault as boolean) ?? false, + encrypted: e2eEnabledForPrivateByDefault, broadcast: false, members: [], },
108-115: Don’t force read-only to false when broadcast turns off; only force it on when broadcast turns on.Current code overwrites a user’s explicit read-only choice when toggling broadcast off. Typically, broadcast ⇒ read-only, but read-only can exist without broadcast. Also, consider resetting
encryptedif E2E is disabled at runtime.-useEffect(() => { - if (!isPrivate) { - setValue('encrypted', false); - } - - setValue('readOnly', broadcast); -}, [watch, setValue, broadcast, isPrivate]); +useEffect(() => { + if (!isPrivate) { + setValue('encrypted', false); + } +}, [isPrivate, setValue]); + +useEffect(() => { + if (broadcast) { + setValue('readOnly', true); + } +}, [broadcast, setValue]); + +// Optional: keep encrypted consistent if the global E2E setting flips while the modal is open. +useEffect(() => { + if (!e2eEnabled) { + setValue('encrypted', false); + } +}, [e2eEnabled, setValue]);
129-141: Sanitize payload: guardencryptedserver-side param by privacy and E2E setting.Even though the UI enforces it, double-check on submit to avoid sending an inconsistent state.
room: { readOnly, extraData: { topic, broadcast, - encrypted, + encrypted: isPrivate && e2eEnabled && encrypted, }, },apps/meteor/client/NavBarV2/NavBarPagesGroup/actions/useEncryptedRoomDescription.spec.tsx (1)
47-54: LGTM: unencrypted/private path under E2E enabled is validated. Minor naming nit.Avoid shadowing Jest’s
describeby renaming the local variable.- const describe = result.current; - expect(describe({ isPrivate: true, encrypted: true })).toBe('Not_available_for_this_workspace'); + const getHint = result.current; + expect(getHint({ isPrivate: true, encrypted: true })).toBe('Not_available_for_this_workspace'); @@ - const describe = result.current; - expect(describe({ isPrivate: false, encrypted: false })).toBe('Encrypted_not_available'); + const getHint = result.current; + expect(getHint({ isPrivate: false, encrypted: false })).toBe('Encrypted_not_available'); @@ - const describe = result.current; - expect(describe({ isPrivate: true, encrypted: true })).toBe('Encrypted_messages'); + const getHint = result.current; + expect(getHint({ isPrivate: true, encrypted: true })).toBe('Encrypted_messages'); @@ - const describe = result.current; - expect(describe({ isPrivate: true, encrypted: false })).toBe('Encrypted_messages_false'); + const getHint = result.current; + expect(getHint({ isPrivate: true, encrypted: false })).toBe('Encrypted_messages_false');apps/meteor/client/NavBarV2/NavBarPagesGroup/actions/useEncryptedRoomDescription.tsx (1)
8-20: Solid simplification: broadcast removed; logic reads clean. Optional memoization + boolean cast.Consider casting settings to boolean and memoizing the returned function to avoid recreations on re-render.
-import { useSetting } from '@rocket.chat/ui-contexts'; +import { useSetting } from '@rocket.chat/ui-contexts'; +import { useCallback } from 'react'; @@ - const e2eEnabled = useSetting('E2E_Enable'); + const e2eEnabled = Boolean(useSetting('E2E_Enable')); @@ - return ({ isPrivate, encrypted }: { isPrivate: boolean; encrypted: boolean }) => { + const getEncryptedHint = useCallback(({ isPrivate, encrypted }: { isPrivate: boolean; encrypted: boolean }) => { if (!e2eEnabled) { return t('Not_available_for_this_workspace'); } if (!isPrivate) { return t('Encrypted_not_available', { roomType }); } if (encrypted) { return t('Encrypted_messages', { roomType }); } return t('Encrypted_messages_false'); - }; + }, [e2eEnabled, roomType, t]); + + return getEncryptedHint;apps/meteor/client/NavBarV2/NavBarPagesGroup/actions/CreateTeamModal.tsx (3)
56-56: Default E2E now depends on both settings — good. Minor: make booleans explicit to drop later casts.-const e2eEnabled = useSetting('E2E_Enable'); -const e2eEnabledForPrivateByDefault = useSetting('E2E_Enabled_Default_PrivateRooms') && e2eEnabled; +const e2eEnabled = Boolean(useSetting('E2E_Enable')); +const e2eEnabledDefaultPrivate = Boolean(useSetting('E2E_Enabled_Default_PrivateRooms')); +const e2eEnabledForPrivateByDefault = e2eEnabled && e2eEnabledDefaultPrivate;
100-106: Use the computed boolean directly; no need for cast/??.- encrypted: (e2eEnabledForPrivateByDefault as boolean) ?? false, + encrypted: e2eEnabledForPrivateByDefault,
111-117: Effect dependencies: dropwatchto avoid unnecessary re-runs.
watchis a stable function and not used as a value source here; including it can cause redundant executions.-}, [watch, setValue, broadcast, isPrivate]); +}, [setValue, broadcast, isPrivate]);apps/meteor/client/NavBarV2/NavBarPagesGroup/actions/CreateTeamModal.spec.tsx (5)
13-13: Remove stray ESLint disable.This disable doesn’t guard any symbol; keep only the one before the
_nameparam.-// eslint-disable-next-line @typescript-eslint/naming-convention
15-21: Clarify suite labels to match components.Minor naming mismatch can confuse readers; make the labels explicit.
- ['CreateTeamModal', CreateTeamModalOld], - ['CreateTeamModal in NavbarV2', CreateTeamModal], + ['CreateTeamModal (legacy)', CreateTeamModalOld], + ['CreateTeamModal (NavbarV2)', CreateTeamModal],
119-156: Optional: add parity check with set-readonly to assert Encrypted independence from permissions.To lock in the “E2EE not tied to set-readonly” guarantee, mirror this test with
.withPermission('set-readonly')and assert Encrypted remains enabled/configurable.it('private team: Encrypted unaffected by set-readonly permission', async () => { render(<CreateTeamModalComponent onClose={() => null} />, { wrapper: mockAppRoot() .withPermission('set-readonly') .withSetting('E2E_Enable', true) .withSetting('E2E_Enabled_Default_PrivateRooms', true) .build(), }); await userEvent.click(screen.getByText('Advanced_settings')); const encrypted = screen.getByLabelText('Teams_New_Encrypted_Label') as HTMLInputElement; expect(encrypted).toBeEnabled(); });
200-206: Fix misleading inline comments (“stays ON” → “turns ON”).ReadOnly flips from OFF to ON when Broadcast goes ON; it doesn’t “stay” ON.
- // Broadcast: OFF -> ON (ReadOnly stays ON + disabled) + // Broadcast: OFF -> ON (ReadOnly turns ON + disabled)Apply to both occurrences.
Also applies to: 221-225
22-256: Optional: factor helpers to reduce repetition.A tiny helper for opening Advanced settings and fetching toggles will trim repetition and ease future maintenance.
const openAdvanced = async (user = userEvent.setup()) => { await user.click(screen.getByText('Advanced_settings')); return user; }; const getToggles = () => ({ encrypted: screen.getByLabelText('Teams_New_Encrypted_Label') as HTMLInputElement, priv: screen.getByLabelText('Teams_New_Private_Label') as HTMLInputElement, broadcast: screen.getByLabelText('Teams_New_Broadcast_Label') as HTMLInputElement, readOnly: screen.getByLabelText('Teams_New_Read_only_Label') as HTMLInputElement, });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
.changeset/rare-walls-press.md(1 hunks)apps/meteor/client/NavBarV2/NavBarPagesGroup/actions/CreateTeamModal.spec.tsx(1 hunks)apps/meteor/client/NavBarV2/NavBarPagesGroup/actions/CreateTeamModal.tsx(5 hunks)apps/meteor/client/NavBarV2/NavBarPagesGroup/actions/useEncryptedRoomDescription.spec.tsx(1 hunks)apps/meteor/client/NavBarV2/NavBarPagesGroup/actions/useEncryptedRoomDescription.tsx(1 hunks)apps/meteor/client/sidebar/header/CreateTeam/CreateTeamModal.tsx(5 hunks)apps/meteor/client/sidebar/header/hooks/useEncryptedRoomDescription.tsx(1 hunks)packages/i18n/src/locales/en.i18n.json(0 hunks)packages/i18n/src/locales/nb.i18n.json(0 hunks)packages/i18n/src/locales/nn.i18n.json(0 hunks)packages/i18n/src/locales/pt-BR.i18n.json(0 hunks)packages/i18n/src/locales/sv.i18n.json(0 hunks)
💤 Files with no reviewable changes (5)
- packages/i18n/src/locales/pt-BR.i18n.json
- packages/i18n/src/locales/sv.i18n.json
- packages/i18n/src/locales/en.i18n.json
- packages/i18n/src/locales/nn.i18n.json
- packages/i18n/src/locales/nb.i18n.json
🧰 Additional context used
🧬 Code graph analysis (3)
apps/meteor/client/NavBarV2/NavBarPagesGroup/actions/CreateTeamModal.spec.tsx (1)
packages/mock-providers/src/index.ts (1)
mockAppRoot(3-3)
apps/meteor/client/NavBarV2/NavBarPagesGroup/actions/CreateTeamModal.tsx (1)
packages/ui-contexts/src/index.ts (2)
useSetting(69-69)usePermissionWithScopedRoles(56-56)
apps/meteor/client/sidebar/header/CreateTeam/CreateTeamModal.tsx (1)
packages/ui-contexts/src/index.ts (2)
useSetting(69-69)usePermissionWithScopedRoles(56-56)
⏰ 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: CodeQL-Build
- GitHub Check: CodeQL-Build
🔇 Additional comments (20)
apps/meteor/client/sidebar/header/hooks/useEncryptedRoomDescription.tsx (2)
8-19: LGTM: simplified, broadcast-free description logic matches the new UI behavior.
8-19: i18n keys verified
Encrypted_messages_falserequires no{ roomType }(definitions across locales contain no placeholders) andNot_available_for_broadcastis fully removed.apps/meteor/client/sidebar/header/CreateTeam/CreateTeamModal.tsx (3)
116-118: LGTM: gating rules mirror product requirements.
readOnlyDisabled = broadcast || !canSetReadOnlyandcanChangeEncrypted = isPrivate && e2eEnabledalign with the objectives.
261-271: LGTM: disabled state and hint usage correctly reflect the new rules.
282-293: LGTM: read-only toggle is disabled when broadcast is on or permission is missing.Pairs well with the effect change proposed above.
apps/meteor/client/NavBarV2/NavBarPagesGroup/actions/useEncryptedRoomDescription.spec.tsx (3)
26-26: LGTM: correct expectation when E2E is disabled.
29-36: LGTM: non-private path under E2E enabled is validated.
38-45: LGTM: encrypted/private path under E2E enabled is validated.apps/meteor/client/NavBarV2/NavBarPagesGroup/actions/CreateTeamModal.tsx (4)
119-120: Clear gating: readOnly disabled on broadcast or lack of permission; E2E toggle gated by private+E2E.
264-269: Correct: E2E toggle disables solely oncanChangeEncrypted.
273-274: Hint call updated to{ isPrivate, encrypted }— consistent with hook changes.
285-289: Read-only toggle disabled viareadOnlyDisabled— matches permission/broadcast rules.apps/meteor/client/NavBarV2/NavBarPagesGroup/actions/CreateTeamModal.spec.tsx (8)
1-4: Solid test scaffolding and providers.Imports and the mockAppRoot usage are appropriate for isolating settings/permissions.
8-10: Good: navigation side-effects mocked.Mocking goToRoomById prevents accidental routing during render.
22-72: E2E defaults matrix covered well.Assertions match the intended combinations of E2E_Enable and E2E_Enabled_Default_PrivateRooms.
74-117: Private toggle gating Encrypted is validated correctly.ON→OFF forces OFF+disabled; OFF→ON re-enables without auto-turning ON. Matches spec.
119-156: Broadcast no longer mutates Encrypted—behavior verified.Encrypted remains user-configurable regardless of Broadcast for private teams.
158-188: Non-private teams correctly forbid Encrypted.Encrypted stays OFF+disabled regardless of Broadcast state. Good guard.
190-207: ReadOnly forced ON and disabled when Broadcast is ON—verified for both permission states.Matches expected UX.
Also applies to: 209-226
228-256: ReadOnly enablement when Broadcast is OFF is correct.Disabled without permission; enabled with permission.
apps/meteor/client/NavBarV2/NavBarPagesGroup/actions/CreateTeamModal.spec.tsx
Show resolved
Hide resolved
|
Note Unit test generation is an Early Access feature. Expect some limitations and changes as we gather feedback and continue to improve it. Generating unit tests... This may take up to 20 minutes. |
|
✅ UTG Post-Process Complete No new issues were detected in the generated code and all check runs have completed. The unit test generation process has completed successfully. |
|
Creating a PR to put the unit tests in... The changes have been created in this pull request: View PR |
|
Note Unit test generation is an Early Access feature. Expect some limitations and changes as we gather feedback and continue to improve it. Generating unit tests... This may take up to 20 minutes. |
|
🎯 Unit Test Generation Post-Process Complete The unit test generation and CI/CD fixing process has completed after reaching the maximum number of iterations. Please review the generated tests and any remaining issues manually. |
|
Creating a PR to put the unit tests in... The changes have been created in this pull request: View PR |
|
Caution CodeRabbit timed out while planning changes for apps/meteor/client/sidebar/header/CreateTeam/CreateTeamModal.spec.tsx. Skipping this file. |
dougfabris
added
the
stat: QA assured
dionisio-bot
bot
added
the
stat: ready to merge
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
apps/meteor/client/NavBarV2/NavBarPagesGroup/actions/CreateTeamModal.spec.tsx (1)
27-27: Switch to userEvent.setup() (v14+) to avoid flakiness.Create a
userinstance and replace staticuserEvent.<action>calls withawait user.<action>(...).describe.each([ ['CreateTeamModal', CreateTeamModalOld], ['CreateTeamModal in NavbarV2', CreateTeamModal], ] as const)( '%s', (_name: string, CreateTeamModalComponent: CreateTeamModalComponentType) => { + let user: ReturnType<typeof userEvent.setup>; + beforeEach(() => { + user = userEvent.setup(); + }); it('should render with encryption option disabled and set to off when E2E_Enable=false and E2E_Enabled_Default_PrivateRooms=false', async () => { render(<CreateTeamModalComponent onClose={() => null} />, { wrapper: mockAppRoot().withSetting('E2E_Enable', false).withSetting('E2E_Enabled_Default_PrivateRooms', false).build(), }); - await userEvent.click(screen.getByText('Advanced_settings')); + await user.click(screen.getByText('Advanced_settings'));Apply the same replacement for the other occurrences on the listed lines.
Also applies to: 40-40, 53-53, 67-67, 79-79, 101-101, 124-124, 163-163, 195-195, 214-214, 233-233, 248-248
🧹 Nitpick comments (14)
apps/meteor/client/sidebar/header/hooks/useEncryptedRoomDescription.tsx (2)
6-6: Strengthen typing for settings.Use generic typing to avoid accidental truthy/falsy bugs if the setting shape changes.
- const e2eEnabled = useSetting('E2E_Enable'); + const e2eEnabled = useSetting<boolean>('E2E_Enable');
4-19: Avoid duplication between header and NavBarV2 hooks.Both files now contain identical logic; consider a single shared implementation (e.g., a util exported from one place) to prevent future drift.
apps/meteor/client/NavBarV2/NavBarPagesGroup/actions/useEncryptedRoomDescription.tsx (2)
6-6: Add explicit boolean typing to the setting.Same rationale as the header hook.
- const e2eEnabled = useSetting('E2E_Enable'); + const e2eEnabled = useSetting<boolean>('E2E_Enable');
4-19: Consider DRYing this with the header hook.Re-export from one module or pull shared logic into a small pure function for consistency.
apps/meteor/client/NavBarV2/NavBarPagesGroup/actions/useEncryptedRoomDescription.spec.tsx (2)
29-36: Covers non‑private path; add the encrypted=true variant for completeness.Currently only checks encrypted=false; add a case to ensure encrypted value is ignored when not private.
Add alongside the existing test:
it('returns "Encrypted_not_available" when room is not private even if encrypted=true', () => { const { result } = renderHook(() => useEncryptedRoomDescriptionHook(roomType), { wrapper: wrapper.withSetting('E2E_Enable', true).build(), }); const describeRoom = result.current; expect(describeRoom({ isPrivate: false, encrypted: true })).toBe('Encrypted_not_available'); });
24-24: Rename local variable to avoid shadowing Jest’s globaldescribe.Minor readability nit.
- const describe = result.current; + const describeRoom = result.current;And update subsequent usages accordingly.
Also applies to: 33-33, 42-42, 51-51
apps/meteor/client/sidebar/header/CreateTeam/CreateTeamModal.tsx (3)
97-104: Initializeencryptedonly when the initial room is private to avoid a brief flicker.Today
encryptedmay start true then be turned off by the effect if the initial room is public. Compute it from the initialisPrivate.} = useForm<CreateTeamModalInputs>({ defaultValues: { - isPrivate: canOnlyCreateOneType ? canOnlyCreateOneType === 'p' : true, + isPrivate: canOnlyCreateOneType ? canOnlyCreateOneType === 'p' : true, readOnly: false, - encrypted: (e2eEnabledForPrivateByDefault as boolean) ?? false, + encrypted: + ((canOnlyCreateOneType ? canOnlyCreateOneType === 'p' : true) && + (e2eEnabledForPrivateByDefault as boolean)) ?? false, broadcast: false, members: [], }, });
108-115: Effect deps: don’t includewatch; also consider reacting toe2eEnabled.
watchis stable but unnecessary here. Optional: forceencrypted=falseif E2E is turned off while the modal is open.-}, [watch, setValue, broadcast, isPrivate]); +}, [setValue, broadcast, isPrivate]);And (optional) add:
useEffect(() => { if (!e2eEnabled) setValue('encrypted', false); }, [e2eEnabled, setValue]);
301-307: A11y: avoidaria-describedbypointing to a missing element when broadcast is off.Only set it when the hint is rendered.
- aria-describedby={`${broadcastId}-hint`} + aria-describedby={value ? `${broadcastId}-hint` : undefined}apps/meteor/client/NavBarV2/NavBarPagesGroup/actions/CreateTeamModal.tsx (3)
56-56: Coerce settings to booleans to avoid tri-state issues.
useSettingreturns unknown; rely on explicit Boolean coercion to prevent'' | 0 | undefinedfrom leaking into logic.- const e2eEnabledForPrivateByDefault = useSetting('E2E_Enabled_Default_PrivateRooms') && e2eEnabled; + const e2eEnabledForPrivateByDefault = + Boolean(useSetting('E2E_Enabled_Default_PrivateRooms')) && Boolean(e2eEnabled);
100-106: Align encrypted default with initial privacy to avoid a transient “ON while public” state.When
canOnlyCreateOneType === 'c',isPrivatedefaults to false butencryptedcan still default to true until the effect runs. Computeencryptedfrom the same initial privacy.- isPrivate: canOnlyCreateOneType ? canOnlyCreateOneType === 'p' : true, + isPrivate: canOnlyCreateOneType ? canOnlyCreateOneType === 'p' : true, readOnly: false, - encrypted: (e2eEnabledForPrivateByDefault as boolean) ?? false, + encrypted: + ((canOnlyCreateOneType ? canOnlyCreateOneType === 'p' : true) && + Boolean(e2eEnabledForPrivateByDefault)) || + false, broadcast: false,
111-118: Split effects and dropwatchfrom deps to reduce unnecessary re-renders.Current effect runs more often than needed and sets values unconditionally.
-useEffect(() => { - if (!isPrivate) { - setValue('encrypted', false); - } - - setValue('readOnly', broadcast); -}, [watch, setValue, broadcast, isPrivate]); +useEffect(() => { + if (!isPrivate) { + setValue('encrypted', false); + } +}, [isPrivate, setValue]); + +useEffect(() => { + setValue('readOnly', broadcast); +}, [broadcast, setValue]);apps/meteor/client/NavBarV2/NavBarPagesGroup/actions/CreateTeamModal.spec.tsx (2)
23-33: Prefer getByRole for toggles to strengthen accessibility assertions.Using roles makes queries resilient to copy changes and matches ARIA semantics.
- const encrypted = screen.getByLabelText('Teams_New_Encrypted_Label') as HTMLInputElement; + const encrypted = screen.getByRole('checkbox', { name: 'Teams_New_Encrypted_Label' }) as HTMLInputElement;Repeat similarly for
Teams_New_Private_Label,Teams_New_Broadcast_Label, andTeams_New_Read_only_Label.
190-207: Test name/comment mismatch.The inline comment says “stays ON + disabled”, which matches the assertions; the test title says “disable and turn on ReadOnly toggle” (fine) — just ensure wording is consistent if you touch it next.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
.changeset/rare-walls-press.md(1 hunks)apps/meteor/client/NavBarV2/NavBarPagesGroup/actions/CreateTeamModal.spec.tsx(1 hunks)apps/meteor/client/NavBarV2/NavBarPagesGroup/actions/CreateTeamModal.tsx(5 hunks)apps/meteor/client/NavBarV2/NavBarPagesGroup/actions/useEncryptedRoomDescription.spec.tsx(1 hunks)apps/meteor/client/NavBarV2/NavBarPagesGroup/actions/useEncryptedRoomDescription.tsx(1 hunks)apps/meteor/client/sidebar/header/CreateTeam/CreateTeamModal.tsx(5 hunks)apps/meteor/client/sidebar/header/hooks/useEncryptedRoomDescription.tsx(1 hunks)packages/i18n/src/locales/en.i18n.json(0 hunks)packages/i18n/src/locales/nb.i18n.json(0 hunks)packages/i18n/src/locales/nn.i18n.json(0 hunks)packages/i18n/src/locales/pt-BR.i18n.json(0 hunks)packages/i18n/src/locales/sv.i18n.json(0 hunks)
💤 Files with no reviewable changes (5)
- packages/i18n/src/locales/nb.i18n.json
- packages/i18n/src/locales/pt-BR.i18n.json
- packages/i18n/src/locales/en.i18n.json
- packages/i18n/src/locales/nn.i18n.json
- packages/i18n/src/locales/sv.i18n.json
🧰 Additional context used
🧬 Code graph analysis (3)
apps/meteor/client/NavBarV2/NavBarPagesGroup/actions/CreateTeamModal.spec.tsx (2)
apps/meteor/client/lib/appLayout.tsx (1)
render(26-28)packages/mock-providers/src/index.ts (1)
mockAppRoot(3-3)
apps/meteor/client/NavBarV2/NavBarPagesGroup/actions/CreateTeamModal.tsx (1)
packages/ui-contexts/src/index.ts (2)
useSetting(69-69)usePermissionWithScopedRoles(56-56)
apps/meteor/client/sidebar/header/CreateTeam/CreateTeamModal.tsx (1)
packages/ui-contexts/src/index.ts (2)
useSetting(69-69)usePermissionWithScopedRoles(56-56)
⏰ 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). (3)
- GitHub Check: 📦 Build Packages
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
🔇 Additional comments (11)
.changeset/rare-walls-press.md (1)
1-6: Changeset reads well and matches the behavior change.Packages bumped to patch; description accurately scopes the Teams modal E2EE fixes.
apps/meteor/client/sidebar/header/hooks/useEncryptedRoomDescription.tsx (2)
8-19: Good removal of broadcast gating; logic is now minimal and consistent.Clear branches: workspace disabled → non‑private → encrypted true/false.
1-20: No stale broadcast references remain. Verified that theNot_available_for_broadcastkey isn’t present anywhere and no callers pass abroadcastargument touseEncryptedRoomDescription.apps/meteor/client/NavBarV2/NavBarPagesGroup/actions/useEncryptedRoomDescription.tsx (1)
8-19: Hook simplification looks good here as well.Matches header version; behavior aligns with PR objectives.
apps/meteor/client/NavBarV2/NavBarPagesGroup/actions/useEncryptedRoomDescription.spec.tsx (1)
26-26: LGTM: E2E disabled path covered.apps/meteor/client/sidebar/header/CreateTeam/CreateTeamModal.tsx (3)
54-54: Correct defaulting: require both E2E enable and private‑default flag.This fixes the earlier over‑eager default. Nice.
116-118: Gating looks right: read‑only depends on broadcast/permission; encrypted on privacy/E2E.Matches the PR intent (no dependency on
set-readonlyfor E2EE).
261-271: Encrypted toggle and hint wiring look correct.Minor nit: consider typing the hook as
useEncryptedRoomDescription('team')already returns translated text; good.apps/meteor/client/NavBarV2/NavBarPagesGroup/actions/CreateTeamModal.tsx (3)
119-121: Read-only/encryption gating looks correct.
readOnlyDisabled = broadcast || !canSetReadOnlyandcanChangeEncrypted = isPrivate && e2eEnabledmatch the intended behavior and PR objectives.
262-274: Encrypted toggle and hint wiring LGTM.Disabling via
!canChangeEncryptedand removing broadcast fromgetEncryptedHintalign with the new contract.
285-285: Read-only toggle disable condition is consistent with channel behavior.Matches prior feedback and keeps UX consistent.
Proposed changes (including videos or screenshots)
This PR fixes unexpected behaviors with the E2EE toggle in Teams:
set-readonlypermission that was disabling the E2EE toggle during Team creation.Issue(s)
Steps to test or reproduce
set-readonlypermission.set-readonly.E2E_Enable = trueandE2E_Enabled_Default_PrivateRooms = true:Further comments
CORE-1326
CORE-1327
Summary by CodeRabbit
Bug Fixes
Improvements
Internationalization
Tests
Chores