-
Notifications
You must be signed in to change notification settings - Fork 13k
fix: Don't try to create a non-encrypted discussion if the parent room is encrypted #38279
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
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: ab17cf4 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 |
WalkthroughAdds propagation of parent-room encryption status into discussion creation UI and autocomplete results: the server now returns Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant CreateDiscussion
participant RoomAutoComplete
participant Form
participant Server
User->>CreateDiscussion: Open modal (may include encryptedParentRoom)
CreateDiscussion->>Form: Initialize (encrypted = encryptedParentRoom)
CreateDiscussion->>RoomAutoComplete: Render (renderRoomIcon, setSelectedRoom)
User->>RoomAutoComplete: Select parent room
RoomAutoComplete->>CreateDiscussion: setSelectedRoom(selectedRoom)
CreateDiscussion->>Server: (optional) GET /v1/rooms.info or use selectedRoom.encrypted
alt parent room is encrypted
CreateDiscussion->>Form: set encrypted = true (disabled)
CreateDiscussion->>Form: disable first-message input
else parent room unencrypted
CreateDiscussion->>Form: allow toggling encrypted
CreateDiscussion->>Form: enable first-message input
end
Form->>User: Update UI state
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
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🧪 Generate unit tests (beta)
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 |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #38279 +/- ##
===========================================
+ Coverage 70.79% 70.86% +0.06%
===========================================
Files 3158 3160 +2
Lines 109390 109768 +378
Branches 19682 19705 +23
===========================================
+ Hits 77445 77788 +343
- Misses 29919 29964 +45
+ Partials 2026 2016 -10
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No issues found across 8 files
abhinavkrin
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The endpoint rooms.createDiscussion does allow creating unencrypted discussions for encrypted rooms. It accepts a field encrypted which if not provided defaults to the parent room encryption status.
So, if the endpoint allows it, IMO, we should not prevent it from being done in the UI.
Currently the UI is always creating an encrypted discussion, just to be sure, lets confirm with product what is the expected behavior, since FE and BE have discordant behaviors |
apps/meteor/client/components/CreateDiscussion/CreateDiscussion.spec.tsx
Outdated
Show resolved
Hide resolved
apps/meteor/client/components/CreateDiscussion/CreateDiscussion.spec.tsx
Show resolved
Hide resolved
apps/meteor/client/components/CreateDiscussion/CreateDiscussion.spec.tsx
Outdated
Show resolved
Hide resolved
apps/meteor/client/components/CreateDiscussion/CreateDiscussion.spec.tsx
Outdated
Show resolved
Hide resolved
apps/meteor/client/components/CreateDiscussion/CreateDiscussion.stories.tsx
Outdated
Show resolved
Hide resolved
apps/meteor/client/components/CreateDiscussion/CreateDiscussion.stories.tsx
Outdated
Show resolved
Hide resolved
apps/meteor/client/components/CreateDiscussion/CreateDiscussion.stories.tsx
Outdated
Show resolved
Hide resolved
apps/meteor/client/components/CreateDiscussion/CreateDiscussion.stories.tsx
Outdated
Show resolved
Hide resolved
apps/meteor/client/components/CreateDiscussion/CreateDiscussion.stories.tsx
Outdated
Show resolved
Hide resolved
apps/meteor/client/components/CreateDiscussion/CreateDiscussion.stories.tsx
Outdated
Show resolved
Hide resolved
apps/meteor/client/components/CreateDiscussion/CreateDiscussion.spec.tsx
Outdated
Show resolved
Hide resolved
apps/meteor/client/components/CreateDiscussion/CreateDiscussion.spec.tsx
Outdated
Show resolved
Hide resolved
apps/meteor/client/components/CreateDiscussion/CreateDiscussion.spec.tsx
Outdated
Show resolved
Hide resolved
dougfabris
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @.changeset/odd-gorillas-obey.md:
- Line 5: Update the changeset description in .changeset/odd-gorillas-obey.md by
correcting the two typos: replace "unencrtypted" with "unencrypted" and
"disscussion" with "discussion" in the line "Fixes issue when trying to create
an unencrtypted disscussion when a parent channel is encrypted", then save the
file so the changeset reads "Fixes issue when trying to create an unencrypted
discussion when a parent channel is encrypted".
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>





Proposed changes (including videos or screenshots)
Issue(s)
SUP-955
Steps to test or reproduce
Further comments
Summary by CodeRabbit
Bug Fixes
Enhancements
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.