Conversation
There was a problem hiding this comment.
Pull request overview
Improves keyboard navigation/focus behavior in several UI surfaces and adjusts message reply behavior based on message send status/direction.
Changes:
- Gate “reply” interaction to avoid replying to certain outgoing message states.
- Adjust
tabIndexvalues for better keyboard tab order in dialogs/modals/search UI. - Add Escape-key handling to dismiss the emoji reaction bar popover.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| ts/hooks/useMessageInteractions.ts | Changes reply behavior by checking message direction/status before calling replyToMessage. |
| ts/components/dialog/UpdateConversationDetailsDialog.tsx | Sets textarea tabIndex to 0 to avoid positive tab ordering. |
| ts/components/dialog/KeyboardShortcutsModal.tsx | Makes the modal content container focusable via tabIndex={0}. |
| ts/components/conversation/SessionEmojiReactBarPopover.tsx | Adds Escape key handler to close the reaction bar. |
| ts/components/SessionSearchInput.tsx | Removes the clear button from tab order with tabIndex={-1} and documents the intent. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
| }, [focusedMessageId, messageId, onClickAwayFromReactionBar]); | ||
|
|
||
| useKey('Escape', onClickAwayFromReactionBar); |
There was a problem hiding this comment.
I think we should stop using useKey as much as possible, and maybe even add a comment/eslint rule that we disable when we know what we are doing for that particular usecase.
The issue is that every useKey is reacting to the Escape at the same time, and depending on what is mounted it can get chaotic to track down. Especially on a common key as this one. Sometimes the chain of propagation is easy, but sometimes it isn't.
For instance, this one will be firing as soon as a SessionEmojiReactBarPopover is mounted, which is pretty much as often as when a message is shown.
Now, if you've been quoting a message and shown this, you might press Escape and 2 "escape" actions could be done. Your staged reply is removed and the SessionEmoji react bar is removed too.
I am not sure what is the best fix is though. We could have similar logic to createButtonOnKeyDownForClickEventHandler, but in this case I guess we want ESC press to work when the emoji bar is also not focused?
Is the ContextMenu/SessionEmojiReactBarPopover part of a focus trap?
ts/hooks/useMessageInteractions.ts
Outdated
| void replyToMessage(messageId); | ||
| } | ||
| // NOTE: we dont want to reply to failed to send messages | ||
| }; |
There was a problem hiding this comment.
the logic for all of those hooks is to return null when the option should not be shown.
Doing your change means that the user would be offered the reply to message but then that wouldn't do anything.
In this case, I think what you want is something like
function useReply(messageId?: string) {
const isSelectedBlocked = useSelectedIsBlocked();
const direction = useMessageDirection(messageId);
const status = useMessageStatus(messageId);
const isOutgoing = direction === 'outgoing';
const isSendingOrError = status === 'sending' || status === 'error';
// NOTE: we dont want to allow to reply to outgoing messages that failed to send or is sending
const cannotReply = isOutgoing && isSendingOrError;
return cannotReply ? null : () => {
if (!messageId) {
return;
}
if (isSelectedBlocked) {
pushUnblockToSend();
return;
}
void replyToMessage(messageId);
} ;
|
|
||
| return ( | ||
| <MessageHighlighter $highlight={highlight} onClick={onClick}> | ||
| <MessageHighlighter $highlight={highlight} onClick={onClick} onKeyDown={onKeyDown} tabIndex={0}> |
There was a problem hiding this comment.
should the selectable thing be the StyledGenericAttachmentContainer instead of the messagehighliter itself?
| onKeyDown={onKeyDown} | ||
| tabIndex={tabIndex} |
There was a problem hiding this comment.
Yeah I think this should be on the content.
Same for onClick actually. MessageHighlighter shouldn't take that prop. If the content is clickable, the content should react to it.
It seems we already do it in the only place where onClick is provided to MessageHighlighter, so we should be able to just remove onClick from MessageHighlighter (and move the onKeyDown/tabIndex to the content too)
<MessageHighlighter $highlight={highlight} onClick={onClick}>
<StyledGenericAttachmentContainer
selected={selected}
className={'module-message__generic-attachment'}
onClick={onClick}
>| const firstEmojiRef = useRef<HTMLSpanElement>(null); | ||
|
|
||
| useMount(() => { | ||
| // NOTE: this allows the fist emoji to be focused when the | ||
| // reaction bar appears if auto focus is enabled | ||
| if (autoFocusFirstEmoji) { | ||
| firstEmojiRef?.current?.focus(); | ||
| } | ||
| }); |
There was a problem hiding this comment.
If we wrapped the MessagereactBad and the context menu into a SessionFocusTrap, this would be done for us, right?
ts/hooks/useMessageInteractions.ts
Outdated
| import { deleteMessagesForX } from '../interactions/conversations/unsendingInteractions'; | ||
|
|
||
| function useSaveAttachemnt(messageId?: string) { | ||
| export function useMessageSaveAttachement(messageId?: string) { |
There was a problem hiding this comment.
| export function useMessageSaveAttachement(messageId?: string) { | |
| export function useMessageSaveAttachment(messageId?: string) { |
No description provided.