-
Notifications
You must be signed in to change notification settings - Fork 1.4k
feat: Disabled message read receipt in federated rooms #6858
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
|
Caution Review failedThe pull request is closed. WalkthroughAdds a runtime federated-room check in Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~7 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (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 |
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: 0
🧹 Nitpick comments (1)
app/views/RoomView/index.tsx (1)
1355-1355: Refactor: Compute federated status once per room, not per message.The
federatedflag is currently recomputed for every message inrenderItem. Since the room's federated status is a property of the room itself and doesn't change per message, this is inefficient—especially for rooms with many messages.Consider computing this once at the component level (e.g., in state, as a class property, or at the start of the render method) and reusing the cached value.
🔎 Proposed refactor options
Option 1: Add to component state
In the constructor or
observeRoommethod, compute and store in state:this.state = { joined: true, room, + isFederated: false, roomUpdate: {}, // ... rest of state };Then update when room changes:
observeRoom = (room: TSubscriptionModel) => { const observable = room.observe(); this.subSubscription = observable.subscribe(changes => { const roomUpdate = roomAttrsUpdate.reduce((ret: any, attr) => { ret[attr] = changes[attr]; return ret; }, {}); + const isFederated = 'id' in changes && isRoomFederated(changes as ISubscription); if (this.mounted) { - this.internalSetState({ room: changes, roomUpdate, isOnHold: !!changes?.onHold }); + this.internalSetState({ room: changes, roomUpdate, isOnHold: !!changes?.onHold, isFederated }); } else { // @ts-ignore this.state.room = changes; // @ts-ignore this.state.roomUpdate = roomUpdate; + // @ts-ignore + this.state.isFederated = isFederated; } }); };Then use in
renderItem:renderItem = (item: TAnyMessageModel, previousItem: TAnyMessageModel, highlightedMessage?: string) => { - const { room, lastOpen, canAutoTranslate } = this.state; + const { room, lastOpen, canAutoTranslate, isFederated } = this.state; // ... - const federated = 'id' in room && isRoomFederated(room); // ... content = ( <Message // ... - isReadReceiptEnabled={Message_Read_Receipt_Enabled && !federated} + isReadReceiptEnabled={Message_Read_Receipt_Enabled && !isFederated}Option 2: Compute once at the start of render method
render() { console.count(`${this.constructor.name}.render calls`); const { room, action, selectedMessages, isAutocompleteVisible, showMissingE2EEKey, showE2EEDisabledRoom } = this.state; const { user, baseUrl, theme, width, serverVersion, navigation } = this.props; const { rid, t } = room; + const isFederated = 'id' in room && isRoomFederated(room as ISubscription);Then pass it to the List component via a closure or store it as an instance variable that
renderItemcan access.Also applies to: 1423-1423
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
app/views/RoomView/index.tsx
🧰 Additional context used
🧬 Code graph analysis (1)
app/views/RoomView/index.tsx (1)
app/lib/methods/isRoomFederated.ts (1)
isRoomFederated(7-8)
🔇 Additional comments (1)
app/views/RoomView/index.tsx (1)
105-105: LGTM: Import is correct.The import statement is appropriate and follows the existing import patterns in the file.
Proposed changes
This pull request introduces a small but important change to how read receipts are handled in the
RoomViewcomponent. Specifically, it disables the read receipt feature for federated rooms by checking if the room is federated before enabling read receipts.Issue(s)
FB-77
How to test or reproduce
Screenshots
Types of changes
Checklist
Further comments
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.