Feat: improved popovers and keyboard navigation by Aerilym · Pull Request #1818 · session-foundation/session-desktop

Aerilym · 2026-02-04T00:25:07Z

No description provided.

fix: animate right panel and remove it from the dom when closed

Copilot

Pull request overview

This PR enhances keyboard navigation and improves popover positioning throughout the application. The changes focus on making the UI more accessible through keyboard shortcuts, better focus management, and refined popover behavior.

Changes:

Added comprehensive keyboard shortcuts system with scope-based handlers for navigation, conversation actions, and message interactions
Improved popover and tooltip positioning with better viewport clamping and trigger position handling
Enhanced accessibility with focus states, tabIndex management, and keyboard event handlers across interactive elements

Reviewed changes

Copilot reviewed 69 out of 70 changed files in this pull request and generated 6 comments.

Show a summary per file

File	Description
ts/util/maths.ts	Added utility function for clamping numbers within min/max bounds
ts/util/keyboardShortcuts.ts	Introduced keyboard shortcuts system with scope management and handler utilities
ts/util/contextMenu.ts	Added helper functions for managing context menu visibility
ts/themes/globals.tsx	Added numeric variants of duration and height values for JS calculations
ts/state/selectors/modal.ts	Added keyboard shortcuts modal selector and scope-based focus management
ts/state/selectors/conversations.ts	Added selector for focused message ID
ts/state/ducks/modalDialog.tsx	Added keyboard shortcuts modal state and context menu closing on modal open
ts/state/ducks/conversations.ts	Added focused message ID state and separated right panel close actions
ts/models/message.ts	Removed global context menu state tracking
ts/hooks/useMessageInteractions.ts	Extracted reusable message interaction handlers into custom hook
ts/hooks/useKeyboardShortcut.tsx	Created hook for registering keyboard shortcuts with scope validation
ts/components/SessionPopover.tsx	Refactored popover positioning with improved viewport handling and clamping
ts/components/SessionTooltip.tsx	Enhanced tooltip with margin controls and scroll dismissal
ts/components/conversation/message	Updated message components with keyboard navigation and improved react bar positioning
ts/components/conversation/right-panel	Converted right panel to use AnimatePresence for smoother transitions
ts/components/conversation/composition	Added keyboard shortcuts for focus, emoji picker, and attachment upload
ts/components/conversation/header	Added keyboard shortcut for conversation settings modal
ts/components/leftpane	Added keyboard shortcuts for conversation navigation (Ctrl+1-9)
ts/components/search	Added focus styles and keyboard navigation to search results
ts/components/menu/items	Added icons to context menu items for better visual hierarchy
ts/components/dialog/KeyboardShortcutsModal.tsx	Created new modal displaying available keyboard shortcuts
stylesheets	Added focus-visible styles and updated hover states to include focus
package.json	Updated react-contexify version constraint

Files not reviewed (1)

pnpm-lock.yaml: Language not supported

Comments suppressed due to low confidence (4)

ts/components/leftpane/LeftPaneMessageSection.tsx:1

The keyboard shortcut registrations are repetitive with nearly identical code blocks. Consider creating a loop or helper function to register all 9 shortcuts, which would reduce duplication and make the code more maintainable.
ts/components/conversation/message/message-content/MessageReactBar.tsx:1
Magic numbers for padding and margin should use CSS variables from the theme system for consistency. Consider using existing margin/padding variables like var(--margins-xs) instead of hardcoded pixel values.
ts/util/keyboardShortcuts.ts:1
Corrected spelling of 'overlocked' to 'overlooked'.
ts/components/dialog/KeyboardShortcutsModal.tsx:1
Invalid CSS property value. The correct syntax is user-select: all; not select: all;.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-02-04T00:28:51Z

ts/components/conversation/header/ConversationHeaderItems.tsx

+
  return (
-    <StyledConversationHeaderAvatar>
+    <StyledConversationHeaderAvatar onKeyDown={onKeyDown} tabIndex={0} role="button">


The avatar header wrapper should only be focusable when it's clickable. Move the onKeyDown, tabIndex, and role props inside the conditional based on optOnAvatarClick to avoid creating a non-functional keyboard trap.

Copilot · 2026-02-04T00:28:51Z

ts/components/SessionTooltip.tsx

+    const handleScroll = () => {
+      setHovered(false);
+      setDebouncedHover(false);
+    };


The scroll event listener is attached to window with capture: true, which will fire on every scroll event in the entire application. Consider adding throttling or debouncing to improve performance, especially when multiple tooltips are rendered.

sounds like a valid point

Kinda, i think we should only set to false if its already true, but otherwise we do want to hide an open tooltip if we scroll so debouncing wouldnt help here, ill take a look for a good solution tho

Copilot · 2026-02-04T00:28:52Z

ts/components/conversation/message/message-content/MessageBubble.tsx

+   * NOTE: this doesnt seem to always catch the resize/zoom change case well, we may need
+   * to consider listening to a resize or zoom event or something.
   */
  const scrollHeight = msgBubbleRef.current?.firstElementChild?.scrollHeight;


Using scrollHeight as a useLayoutEffect dependency will cause excessive re-renders on every scroll. Consider using a ResizeObserver or a more targeted approach to detect when the content height actually changes.

ts/components/conversation/SessionConversation.tsx

Copilot · 2026-02-04T00:28:52Z

ts/components/SessionSearchInput.tsx

+  const isDisabled = () =>
+    !(inputRef.current !== null && inputRef.current !== document.activeElement);
+
+  useKeyboardShortcut({
+    shortcut: KbdShortcut.conversationListSearch,
+    handler: () => {
+      if (!isDisabled() && inputRef.current) {
+        inputRef.current.focus();
+      }
+    },
+    disabled: isDisabled,


The function name isDisabled is misleading as it actually checks if the shortcut should be enabled (returns true when disabled, but is used in a disabled prop). Consider renaming to shouldDisableShortcut or inverting the logic for clarity.

Suggested change

const isDisabled = () =>

!(inputRef.current !== null && inputRef.current !== document.activeElement);

useKeyboardShortcut({

shortcut: KbdShortcut.conversationListSearch,

handler: () => {

if (!isDisabled() && inputRef.current) {

inputRef.current.focus();

}

},

disabled: isDisabled,

const shouldDisableShortcut = () =>

!(inputRef.current !== null && inputRef.current !== document.activeElement);

useKeyboardShortcut({

shortcut: KbdShortcut.conversationListSearch,

handler: () => {

if (!shouldDisableShortcut() && inputRef.current) {

inputRef.current.focus();

}

},

disabled: shouldDisableShortcut,

Copilot · 2026-02-04T00:28:52Z

ts/components/conversation/SessionConversation.tsx

+      const targetElement = document.getElementById(`msg-${mostRecentMessage.id}`);
+      if (!targetElement) {
+        await openConversationToSpecificMessage({
+          conversationKey,
+          messageIdToNavigateTo: mostRecentMessage.id,
+          shouldHighlightMessage: false,
+        });
+        return 100;
+      }


The magic number 100 (milliseconds) for retry delay lacks explanation. Consider extracting this as a named constant like SCROLL_RETRY_DELAY_MS to improve code clarity.

Bilb · 2026-02-04T05:12:18Z

ts/components/conversation/header/ConversationHeader.tsx

+  const showConvoSettings = showConvoSettingsCb
+    ? () =>
+        convoSettings
+          ? dispatch(updateConversationSettingsModal(null))
+          : showConvoSettingsCb({ settingsModalPage: 'default' })
+    : undefined;


Shouldn't this be in useShowConversationSettingsFor directly? This is kind of the point to have one hook that returns a cb that is what we need for all the cases, when possible.

Bilb · 2026-02-04T05:16:10Z

ts/components/conversation/message/message-content/MessageBubble.tsx

-  useLayoutEffect(() => {
-    if (expanded) {
-      const msgContainerAfter = messagesContainerRef.current;
-      if (!msgBubbleRef.current || !msgContainerAfter) {


Bilb · 2026-02-04T22:25:13Z

ts/components/conversation/message/message-content/MessageReactions.tsx

+    width: ${EMOJI_REACTION_HEIGHT}px;
+    height: ${EMOJI_REACTION_HEIGHT}px;


those are expected to have the same var?

Yep makes it a circle

Bilb · 2026-02-04T22:26:16Z

ts/components/conversation/message/reactions/Reaction.tsx

+  height: ${EMOJI_REACTION_HEIGHT}px;
+  min-width: ${props => (props.$showCount ? 2 * EMOJI_REACTION_HEIGHT : EMOJI_REACTION_HEIGHT)}px;


same comment

Oh interesting i didnt realise min-width was a width, the values themselves are the same as before my change, so there is no change in behavior here, not sure why there is a min-width

Bilb · 2026-02-04T22:29:23Z

ts/components/conversation/SessionMessagesList.tsx

+            <ComponentToRender key={messageId} messageId={messageId} />,
+          ];
+        })
+        // TODO: check if we reverse this upstream, we might be reversing twice


Bilb · 2026-02-04T22:40:13Z

ts/util/keyboardShortcuts.ts

+    withCtrl: true,
+    keys: ['/'],
+  },
+  zoomIn: { name: 'Zoom In', scope: 'global', withCtrl: true, keys: ['+'] },


looking good 👍

Bilb · 2026-02-04T22:40:36Z

ts/util/keyboardShortcuts.ts

+    withCtrl: true,
+    keys: ['s'],
+  },
+  // NOTE: these are currently dummy shortcuts, they are native or implmeneted differently


Suggested change

// NOTE: these are currently dummy shortcuts, they are native or implmeneted differently

// NOTE: these are currently dummy shortcuts, they are native or implemented differently

Bilb · 2026-02-04T22:41:06Z

package.json

    "qrcode.react": "4.2.0",
    "react": "19.2.1",
-    "react-contexify": "6.0.0",
+    "react-contexify": "^6.0.0",


as discussed this needs to point to our fork

Bilb · 2026-02-04T22:52:44Z

ts/components/conversation/message/message-content/MessageContextMenu.tsx

+const CONTEXTIFY_MENU_WIDTH_PX = 200;
+const SCREEN_RIGHT_MARGIN_PX = 104;


🧙‍♂️

Bilb · 2026-02-04T22:54:12Z

ts/components/conversation/message/message-content/MessageContextMenu.tsx

            }}
          >
-            <Localizer token="info" />
+            <SessionLucideIconButton


should we have a component (even local to that file) that is a ItemWithDataTestId with TrArgs & icon to display?

Yeah could be a good idea, will do that

Aerilym and others added 5 commits January 23, 2026 15:52

fix: animate right panel and remove it from the dom when closed

bd7fb81

Merge pull request #1794 from session-foundation/fix/animate_right_panel

d04e3c9

fix: animate right panel and remove it from the dom when closed

fix: context menu triggers and keyboard nav

c0b3b9c

feat: rework popover content system

7a660fb

chore: merge origin/dev into fix/context_menus

68f33a6

Copilot AI review requested due to automatic review settings February 4, 2026 00:25

Copilot AI reviewed Feb 4, 2026

View reviewed changes

feat: add shortcuts for conversation and message actions

fa869d9

Bilb reviewed Feb 4, 2026

View reviewed changes

		width: ${EMOJI_REACTION_HEIGHT}px;
		height: ${EMOJI_REACTION_HEIGHT}px;

		height: ${EMOJI_REACTION_HEIGHT}px;
		min-width: ${props => (props.$showCount ? 2 * EMOJI_REACTION_HEIGHT : EMOJI_REACTION_HEIGHT)}px;

	// NOTE: these are currently dummy shortcuts, they are native or implmeneted differently
	// NOTE: these are currently dummy shortcuts, they are native or implemented differently

		const CONTEXTIFY_MENU_WIDTH_PX = 200;
		const SCREEN_RIGHT_MARGIN_PX = 104;

Conversation

Aerilym commented Feb 4, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Feb 4, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 4, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 4, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI Feb 4, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 4, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Aerilym Feb 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Aerilym Feb 4, 2026 •

edited

Loading