feat(chat): message overflow menu (⋯ + right-click) with Matrix actions - #2146
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 5 minutes and 43 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (10)
WalkthroughThis PR adds message deletion/redaction capability to the Matrix chat system. It introduces the Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant MessageBubble as Message Bubble
participant Overflow as Message Overflow
participant Dialog as Delete Dialog
participant RightPanel as Right Panel
participant Provider as Matrix Provider
participant Matrix as Matrix Server
User->>Overflow: Click delete action
Overflow->>Dialog: Open confirmation dialog
Dialog->>User: Show delete confirmation
User->>Dialog: Confirm deletion
Dialog->>Overflow: Close dialog & menu
Overflow->>RightPanel: Call onDeleteMessage(messageId)
RightPanel->>Provider: redactRoomEvent({ roomId, eventId })
Provider->>Matrix: Redact event
Matrix->>Provider: Success/Error
Provider->>RightPanel: Return (resolve/reject)
RightPanel->>RightPanel: Clear draft/alert on success
RightPanel->>User: Show error alert (if failed)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
155956a to
fa74cb3
Compare
Right-click or the ⋯ control opens a grouped menu: quick reactions from recent usage (fallback defaults), add reaction picker, edit/reply, copy text, matrix.to link, speech synthesis, and redact for own messages. Adds Radix context menu primitives to the UI package plus editRoomMessage and redactRoomEvent on the Matrix provider. Co-authored-by: webguru-hypha <webguru-hypha@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/epics/src/common/human-right-panel.tsx (2)
1024-1049:⚠️ Potential issue | 🟡 MinorWire edit state into the composer controls, not just the banner.
editPreviewonly changes the preview strip here. The file/image controls stay available, buthandleSend()later rejects attachments during edits, so users can still enter an unsupported compose state from the UI. Pass an explicit editing flag toHumanChatPanelChatBarand disable attachment picking whileeditDraftis active.As per coding guidelines:
packages/epics/**: Error and loading states are handled in all user flows.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/epics/src/common/human-right-panel.tsx` around lines 1024 - 1049, The chat composer currently only shows editPreview but still allows attachment picking while editing, causing handleSend (which rejects attachments when editing) to hit an unsupported state; update the HumanChatPanelChatBar invocation to pass an explicit editing flag (e.g. isEditing: Boolean(editDraft)) and wire that prop into the component so file/image controls are disabled when editDraft is active, and ensure draftAttachments are cleared or prevented from being set while isEditing is true (use draftAttachments and setDraftAttachments to guard/clear attachments when editDraft toggles).
764-780:⚠️ Potential issue | 🟠 MajorClear the old edit body when switching to reply mode.
This cancels
editDraft, but it keeps the edited message text ininput. If a user clicks Reply while editing another message, the composer changes mode while still containing the old edit body, which can then be sent as an unrelated reply.As per coding guidelines: `packages/epics/**`: State management patterns are consistent and predictable.🧹 Suggested fix
const excerpt = firstLineForReplyPreview(getMessagePlainText(target)); - setEditDraft(null); + if (editDraft) { + setEditDraft(null); + setInput(''); + } setReplyDraft({- [messages, resolveMemberLabel, t], + [messages, resolveMemberLabel, t, editDraft],🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/epics/src/common/human-right-panel.tsx` around lines 764 - 780, When switching to reply mode inside handleReplyToMessage, also clear the composer's input so an old edit body isn't sent as the new reply: after setEditDraft(null) add a call to clear the input state (e.g. setInput('') or the actual composer input setter used in this component), then setReplyDraft(...); also add that input setter to the useCallback dependency array so closures remain correct.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/core/src/matrix/client/providers/matrix-provider.tsx`:
- Around line 642-688: Ensure core enforces "own message only" by validating the
event sender before editing or redacting: in editRoomMessage (the block that
looks up room.findEventById(targetEventId) and builds the replacement) and in
redactRoomEvent, fetch the event (ev) and compare ev.getSender() to the current
user id (from client.getUserId() or equivalent on the Matrix client); if they
differ, throw an Error like "Cannot edit/redact events you do not own". Add this
guard early (after finding ev) so edits/redactions are rejected in core, not
only in the UI.
In `@packages/epics/src/common/human-chat-panel/human-chat-panel-chat-bar.tsx`:
- Around line 291-297: The effect that restores focus on edit preview open
should also resize the textarea so preloaded multi-line drafts aren't clipped:
in the existing useEffect that reads editPreview and uses textareaRef and
editPreviewWasOpenRef, after focusing call the textarea's autoResize() (e.g.,
textareaRef.current?.autoResize()) and ensure you call autoResize whenever
isOpen is true (not only on the transition) so external value changes trigger a
resize; keep the existing editPreviewWasOpenRef update logic.
In
`@packages/epics/src/common/human-chat-panel/human-chat-panel-message-overflow.tsx`:
- Around line 264-267: The "Copy message link" menu item currently calls
onCopyLink even when no valid Matrix link exists (matrixToLink is empty when
roomId is null or messageId === 'welcome'), which silently does nothing; update
the Item rendering in human-chat-panel-message-overflow.tsx to disable the menu
entry when matrixToLink is falsy by passing a disabled prop (or equivalent) and
avoid invoking onCopyLink when matrixToLink is empty — e.g., make Item's
onSelect no-op or guard onCopyLink with a check for matrixToLink, referencing
the Item element, onCopyLink handler, matrixToLink variable, and the
messageId/roomId conditions.
In `@packages/i18n/src/messages/en.json`:
- Line 1643: The label for the text-to-speech action is ambiguous: update the
"contextSpeakMessage" message key value from "Speak message" to a clearer phrase
like "Read message aloud" (or "Read aloud") in
packages/i18n/src/messages/en.json and ensure corresponding translation files
use the same clearer wording so all locales reflect the new label.
In `@packages/ui/src/context-menu.tsx`:
- Around line 21-197: Extract and export explicit prop interfaces for each
public wrapper (e.g., ContextMenuSubTriggerProps, ContextMenuContentProps,
ContextMenuItemProps, ContextMenuCheckboxItemProps, ContextMenuRadioItemProps,
ContextMenuLabelProps, ContextMenuSeparatorProps, ContextMenuShortcutProps) that
mirror the current inline types (including optional inset and HTML/primitive
props), then replace the anonymous inline types in the React.forwardRef generic
signatures for ContextMenuSubTrigger, ContextMenuSubContent, ContextMenuContent,
ContextMenuItem, ContextMenuCheckboxItem, ContextMenuRadioItem,
ContextMenuLabel, ContextMenuSeparator and the ContextMenuShortcut function
signature with the newly exported interfaces so consumers can import and extend
the prop contracts.
---
Outside diff comments:
In `@packages/epics/src/common/human-right-panel.tsx`:
- Around line 1024-1049: The chat composer currently only shows editPreview but
still allows attachment picking while editing, causing handleSend (which rejects
attachments when editing) to hit an unsupported state; update the
HumanChatPanelChatBar invocation to pass an explicit editing flag (e.g.
isEditing: Boolean(editDraft)) and wire that prop into the component so
file/image controls are disabled when editDraft is active, and ensure
draftAttachments are cleared or prevented from being set while isEditing is true
(use draftAttachments and setDraftAttachments to guard/clear attachments when
editDraft toggles).
- Around line 764-780: When switching to reply mode inside handleReplyToMessage,
also clear the composer's input so an old edit body isn't sent as the new reply:
after setEditDraft(null) add a call to clear the input state (e.g. setInput('')
or the actual composer input setter used in this component), then
setReplyDraft(...); also add that input setter to the useCallback dependency
array so closures remain correct.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: f351921a-bc22-4a57-be72-2752c597591d
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml,!pnpm-lock.yaml
📒 Files selected for processing (14)
packages/core/src/matrix/client/providers/matrix-provider.tsxpackages/epics/src/common/human-chat-panel/human-chat-panel-chat-bar.tsxpackages/epics/src/common/human-chat-panel/human-chat-panel-message-bubble.tsxpackages/epics/src/common/human-chat-panel/human-chat-panel-message-overflow.tsxpackages/epics/src/common/human-chat-panel/human-chat-panel-messages.tsxpackages/epics/src/common/human-right-panel.tsxpackages/i18n/src/messages/de.jsonpackages/i18n/src/messages/en.jsonpackages/i18n/src/messages/es.jsonpackages/i18n/src/messages/fr.jsonpackages/i18n/src/messages/pt.jsonpackages/ui/package.jsonpackages/ui/src/context-menu.tsxpackages/ui/src/index.ts
fa74cb3 to
7dac6da
Compare
Enforce own-message redaction in Matrix provider; resize composer on edit draft and external value; disable copy-link without matrix.to URL; clarify TTS menu label in all locales; export context menu prop interfaces. Co-authored-by: webguru-hypha <webguru-hypha@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
packages/core/src/matrix/client/providers/matrix-provider.tsx (1)
734-743:⚠️ Potential issue | 🟠 MajorEnforce ownership checks inside
redactRoomEvent(not only in UI).Line 742 redacts directly without verifying the event exists and belongs to the current user. This leaves a core mutation without the documented "own message only" rule.
🔒 Suggested fix
const redactRoomEvent = React.useCallback( async ({ roomId, eventId }: RedactRoomEventInput) => { if (!client) { throw new Error('Client should be specified'); } if (!roomId?.trim() || !eventId?.trim()) { return; } + const room = client.getRoom(roomId); + const ev = + room?.findEventById(eventId) ?? + (typeof room?.getPendingEvent === 'function' + ? room.getPendingEvent(eventId) + : null); + if (!room || !ev) { + throw new Error('Message to delete not found'); + } + const uid = client.getUserId(); + if (!uid || ev.getSender() !== uid) { + throw new Error('You can only delete your own messages'); + } await client.redactEvent(roomId, eventId); }, [client], );As per coding guidelines:
packages/core/**— "Business rules are well-encapsulated and testable".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/matrix/client/providers/matrix-provider.tsx` around lines 734 - 743, redactRoomEvent currently calls client.redactEvent directly; update it to first fetch the target event (using the Matrix client method that retrieves an event for a room/eventId) and verify its sender matches the current user (client.getUserId() or equivalent) before calling client.redactEvent; if the event is missing or sender !== current user, reject/throw an error (or return) so the "own message only" business rule is enforced server-side in redactRoomEvent rather than only in the UI.packages/ui/src/context-menu.tsx (1)
21-25: 🛠️ Refactor suggestion | 🟠 MajorExport explicit prop interfaces for public wrapper components.
Public components are exported, but their props are anonymous inline types. Please extract/export named interfaces and reuse them in the
forwardRefsignatures.As per coding guidelines:
packages/ui/**— "TypeScript: props are fully typed with exported interfaces".Also applies to: 42-45, 57-60, 74-78, 92-95, 116-119, 138-142, 156-159, 168-171
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui/src/context-menu.tsx` around lines 21 - 25, Export a named props interface for each public wrapper component currently using an inline anonymous prop type (for example, create and export ContextMenuSubTriggerProps that extends React.ComponentPropsWithoutRef<typeof ContextMenuPrimitive.SubTrigger> & { inset?: boolean }) and then replace the inline type in the React.forwardRef generic with the exported interface (e.g., use React.forwardRef<React.ElementRef<typeof ContextMenuPrimitive.SubTrigger>, ContextMenuSubTriggerProps> in the ContextMenuSubTrigger declaration); repeat the same pattern for the other public wrapper components mentioned (the other forwardRef wrappers in this file) so all props are fully typed with exported interfaces.packages/epics/src/common/human-chat-panel/human-chat-panel-message-overflow.tsx (1)
264-267:⚠️ Potential issue | 🟡 MinorValidate Matrix ids before enabling “Copy message link”.
matrixToLinkis built for any non-emptyroomId/messageId, so local placeholders such ashypha-send-pending...still produce a copyable but brokenmatrix.toURL. This action should stay disabled unless both values are real Matrix ids.Also applies to: 319-324
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/epics/src/common/human-chat-panel/human-chat-panel-message-overflow.tsx` around lines 264 - 267, The menu item currently calls onCopyLink and builds a matrixToLink for any non-empty roomId/messageId, which allows placeholder IDs like "hypha-send-pending..." to produce broken URLs; update the logic around the Item (and the similar block at the other occurrence) to first validate both roomId and messageId with a helper (e.g., isValidMatrixId) and only enable the Item/onCopyLink when both IDs pass validation; if either ID is invalid, render the Item disabled (or omit the onSelect) and ensure matrixToLink is not generated unless both roomId and messageId are valid.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@packages/epics/src/common/human-chat-panel/human-chat-panel-message-overflow.tsx`:
- Around line 354-359: confirmDelete currently awaits onDeleteMessage while
already closing the UI, which yields unhandled rejections when the delete fails;
change confirmDelete to wrap the await onDeleteMessage(messageId) in a try/catch
and only call setDeleteOpen(false) and setDropdownOpen(false) after a successful
await, and in the catch branch surface the error (e.g., set an error state or
re-open the dialog) so failures are handled; update the same pattern for the
analogous handler around lines 453-455 that uses onDeleteMessage as well.
- Around line 84-95: The helper currently falls back to attachment filenames
(via m.mediaSlots / m.media) when there is no text, causing "Copy text" and
"Speak message" to operate on filenames; update the function that computes
fromParts (uses stripMatrixReplyFallback and variable fromParts) to return only
the text body: return fromParts if fromParts.trim() else return '' and remove
the m.mediaSlots / m.media filename fallback; if other UI needs attachment
filenames, expose a separate helper or use an explicit attachment-name function
rather than changing this text-only helper.
- Around line 241-253: The overflow menu currently infers action availability
from callbacks (Boolean(onEdit)) causing mismatch with the hover bar; update the
component props for human-chat-panel-message-overflow to accept explicit
booleans (canEdit and canReply) and use those to set Item.disabled for the Edit
and Reply entries instead of checking the callbacks; ensure the parent
(human-chat-panel-message-bubble) passes the new canEdit/canReply props
consistently where it previously relied on onEdit/onReply, and apply the same
change for the second occurrence around the other Item block (the lines noted
~307-309) so hover and overflow behavior stays in sync.
---
Duplicate comments:
In `@packages/core/src/matrix/client/providers/matrix-provider.tsx`:
- Around line 734-743: redactRoomEvent currently calls client.redactEvent
directly; update it to first fetch the target event (using the Matrix client
method that retrieves an event for a room/eventId) and verify its sender matches
the current user (client.getUserId() or equivalent) before calling
client.redactEvent; if the event is missing or sender !== current user,
reject/throw an error (or return) so the "own message only" business rule is
enforced server-side in redactRoomEvent rather than only in the UI.
In
`@packages/epics/src/common/human-chat-panel/human-chat-panel-message-overflow.tsx`:
- Around line 264-267: The menu item currently calls onCopyLink and builds a
matrixToLink for any non-empty roomId/messageId, which allows placeholder IDs
like "hypha-send-pending..." to produce broken URLs; update the logic around the
Item (and the similar block at the other occurrence) to first validate both
roomId and messageId with a helper (e.g., isValidMatrixId) and only enable the
Item/onCopyLink when both IDs pass validation; if either ID is invalid, render
the Item disabled (or omit the onSelect) and ensure matrixToLink is not
generated unless both roomId and messageId are valid.
In `@packages/ui/src/context-menu.tsx`:
- Around line 21-25: Export a named props interface for each public wrapper
component currently using an inline anonymous prop type (for example, create and
export ContextMenuSubTriggerProps that extends
React.ComponentPropsWithoutRef<typeof ContextMenuPrimitive.SubTrigger> & {
inset?: boolean }) and then replace the inline type in the React.forwardRef
generic with the exported interface (e.g., use
React.forwardRef<React.ElementRef<typeof ContextMenuPrimitive.SubTrigger>,
ContextMenuSubTriggerProps> in the ContextMenuSubTrigger declaration); repeat
the same pattern for the other public wrapper components mentioned (the other
forwardRef wrappers in this file) so all props are fully typed with exported
interfaces.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 3280ef1e-7789-450c-aaf3-e96f8d4bd1c6
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml,!pnpm-lock.yaml
📒 Files selected for processing (13)
packages/core/src/matrix/client/providers/matrix-provider.tsxpackages/epics/src/common/human-chat-panel/human-chat-panel-message-bubble.tsxpackages/epics/src/common/human-chat-panel/human-chat-panel-message-overflow.tsxpackages/epics/src/common/human-chat-panel/human-chat-panel-messages.tsxpackages/epics/src/common/human-right-panel.tsxpackages/i18n/src/messages/de.jsonpackages/i18n/src/messages/en.jsonpackages/i18n/src/messages/es.jsonpackages/i18n/src/messages/fr.jsonpackages/i18n/src/messages/pt.jsonpackages/ui/package.jsonpackages/ui/src/context-menu.tsxpackages/ui/src/index.ts
Align core edit/redact errors with ownership wording; copy/speak use text body only; overflow menu uses explicit menuCanEdit/menuCanReply from bubble; delete confirm keeps dialog open on failure with localized error and loading state. Co-authored-by: webguru-hypha <webguru-hypha@users.noreply.github.com>
Summary
Implements a Discord-style message overflow menu opened from the three-dots control on the hover bar or by right-clicking the message row.
Menu contents (grouped with separators)
hypha-chat-recent-reactions), falling back to 👍 🎵 🙏 ✅ when empty. Inline/hover react paths also record usage so the strip stays fresh in the same tab.https://matrix.to/#/{roomId}/{eventId}when both are real Matrix ids.speechSynthesiswithdocument.documentElement.lang.redactRoomEventon the Matrix provider (own messages only).Supporting changes
@hypha-platform/ui:@radix-ui/react-context-menu+ newcontext-menu.tsxexport;@radix-ui/react-dropdown-menuadded topackages/uiso epics does not rely on the root app for that dependency.MatrixProvider:editRoomMessage(m.replace) andredactRoomEvent(wrapsclient.redactEvent).HumanRightPanel: edit draft + send path for edits, delete handler + error banner, passesroomId/currentUserIdinto the message list.i18n
New
HumanChatPanelstrings in en, de, es, fr, pt (menu labels, delete confirm, edit banner keys reused from prior chat work where applicable).Testing
pnpm --filter @hypha-platform/core testpnpm --filter @hypha-platform/epics check-typesSummary by CodeRabbit
New Features
Documentation