feat: add update member name support #291
Conversation
WalkthroughA new public method Changes
Sequence DiagramsequenceDiagram
participant Client
participant RoomService
participant StateService
participant Federation
Client->>RoomService: updateMemberProfile(roomId, userId, displayName)
RoomService->>RoomService: Fetch room info & state
RoomService->>RoomService: Verify user is joined member
alt Member verification fails
RoomService-->>Client: Error
else Member verified
RoomService->>RoomService: Build m.room.member event<br/>(membership: 'join', displayName)
RoomService->>StateService: Process event
StateService-->>RoomService: Event processed
RoomService->>Federation: Broadcast event to servers
alt Federation rejected
Federation-->>RoomService: Error
RoomService-->>Client: Error thrown
else Federation accepted
Federation-->>RoomService: Acknowledged
RoomService-->>Client: Return PersistentEvent
end
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #291 +/- ##
==========================================
+ Coverage 60.31% 60.49% +0.18%
==========================================
Files 67 67
Lines 6672 6675 +3
==========================================
+ Hits 4024 4038 +14
+ Misses 2648 2637 -11 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/federation-sdk/src/services/room.service.ts (1)
703-707: Consider adding displayName validation.The method doesn't validate the
displayNameparameter (e.g., length limits, character restrictions). While validation might be handled elsewhere in the stack, adding basic validation here would provide defense in depth and fail fast on invalid input.Consider adding validation:
async updateMemberProfile( roomId: RoomID, userId: UserID, displayName: string, ): Promise<PersistentEventBase<RoomVersion, 'm.room.member'>> { + // Validate displayName + if (!displayName || displayName.trim().length === 0) { + throw new HttpException( + 'Display name cannot be empty', + HttpStatus.BAD_REQUEST, + ); + } + if (displayName.length > 256) { + throw new HttpException( + 'Display name too long (max 256 characters)', + HttpStatus.BAD_REQUEST, + ); + } + logger.info( `Updating member profile for user ${userId} in room ${roomId}`, );
📜 Review details
Configuration used: CodeRabbit 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)
packages/federation-sdk/src/services/room.service.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/federation-sdk/src/services/room.service.ts (2)
packages/federation-sdk/src/index.ts (4)
RoomID(29-29)UserID(28-28)PersistentEventBase(25-25)RoomVersion(26-26)packages/room/src/types/_common.ts (2)
RoomID(16-16)UserID(20-20)
🔇 Additional comments (1)
packages/federation-sdk/src/services/room.service.ts (1)
703-745: Improve error handling and logging; verify authorization when integrating this method into API endpoints.The
updateMemberProfilemethod is correctly implemented for updating member profile information, but requires attention in three areas:
Authorization verification needed upon integration: This method is not currently called from any controller. When exposed via HTTP endpoints, verify that authorization is properly enforced to ensure only the target user (or an admin) can update their own profile, preventing impersonation.
Inconsistent error handling: Throws generic
Errorobjects instead ofHttpExceptionwith proper HTTP status codes (lines 713, 737-739), inconsistent with other methods in this class likeleaveRoomandkickUser.Missing logging: Other similar methods (
updateRoomName,leaveRoom,kickUser) include logging statements. Add logger calls at method entry and exit for debugging and auditing.Apply error handling and logging improvements:
async updateMemberProfile( roomId: RoomID, userId: UserID, displayName: string, ): Promise<PersistentEventBase<RoomVersion, 'm.room.member'>> { + this.logger.info( + `Updating member profile for user ${userId} in room ${roomId}`, + ); + const roomInfo = await this.stateService.getRoomInformation(roomId); const currentState = await this.stateService.getLatestRoomState(roomId); const currentMembership = currentState.get(`m.room.member:${userId}`); if (!currentMembership || currentMembership.getMembership() !== 'join') { - throw new Error(`User ${userId} is not a member of room ${roomId}`); + throw new HttpException( + `User ${userId} is not a member of room ${roomId}`, + HttpStatus.FORBIDDEN, + ); } const memberEvent = await this.stateService.buildEvent<'m.room.member'>( { type: 'm.room.member', content: { membership: 'join', // SAME membership (not changing) displayname: displayName, // NEW displayname }, room_id: roomId, sender: userId, state_key: userId, auth_events: [], depth: 0, prev_events: [], origin_server_ts: Date.now(), }, roomInfo.room_version, ); await this.stateService.handlePdu(memberEvent); if (memberEvent.rejected) { - throw new Error( - `Member profile update rejected: ${memberEvent.rejectReason}`, - ); + throw new HttpException( + `Member profile update rejected: ${memberEvent.rejectReason}`, + HttpStatus.BAD_REQUEST, + ); } + this.logger.info( + `Successfully updated member profile for user ${userId} in room ${roomId}`, + ); + void this.federationService.sendEventToAllServersInRoom(memberEvent); return memberEvent; }
Uh oh!
There was an error while loading. Please reload this page.