Skip to content

fix(code): keep focus in popovers opened from command center cells#1916

Open
VojtechBartos wants to merge 2 commits intomainfrom
posthog-code/fix-command-center-repo-and-branch-selector-focus
Open

fix(code): keep focus in popovers opened from command center cells#1916
VojtechBartos wants to merge 2 commits intomainfrom
posthog-code/fix-command-center-repo-and-branch-selector-focus

Conversation

@VojtechBartos
Copy link
Copy Markdown
Member

@VojtechBartos VojtechBartos commented Apr 28, 2026

Summary

  • GridCell's onClick redirected focus to the cell's first [tabindex='0'] element on every click, breaking popovers opened from a TaskInput cell (BranchSelector, GitHubRepoPicker, FolderPicker, WorkspaceModeSelect). React's synthetic events bubble through portals, so clicks inside the popover content reached the grid handler and pulled focus to the cell's workspace mode selector.
  • TaskInput now also focuses its prompt editor when you click empty space inside a cell. The shared FOCUSABLE_SELECTOR constant lives in @utils/overlay.
  • The active-cell indicator (red border) now follows activeCellIndex instead of activeTaskId, so empty and creating cells light up too.

Showcase

code-focus.mov

Created with PostHog Code

@VojtechBartos VojtechBartos self-assigned this Apr 28, 2026
@VojtechBartos VojtechBartos marked this pull request as draft April 28, 2026 16:30
@greptile-apps
Copy link
Copy Markdown

greptile-apps Bot commented Apr 28, 2026

Prompt To Fix All With AI
This is a comment left during a code review.
Path: apps/code/src/renderer/features/command-center/components/CommandCenterGrid.tsx
Line: 10-11

Comment:
**`[tabindex]` missing from focusable selector**

`FOCUSABLE_SELECTOR` doesn't include `[tabindex]`, so custom interactive elements that rely solely on `tabindex="0"` (e.g. a `<div tabindex="0">` acting as a focusable control) won't be caught by the guard. If such an element is the *second* `[tabindex="0"]` node inside a cell, a click on it would still redirect focus to the *first* `[tabindex="0"]` instead. Adding `[tabindex]:not([tabindex="-1"])` to the selector closes that gap defensively.

```suggestion
const FOCUSABLE_SELECTOR =
  'button, a, input, textarea, select, [tabindex]:not([tabindex="-1"]), [role="button"], [role="link"], [role="combobox"], [role="menuitem"], [contenteditable="true"], [data-interactive]';
```

How can I resolve this? If you propose a fix, please make it concise.

Reviews (1): Last reviewed commit: "fix(code): keep focus in popovers opened..." | Re-trigger Greptile

Comment on lines +10 to +11
const FOCUSABLE_SELECTOR =
'button, a, input, textarea, select, [role="button"], [role="link"], [role="combobox"], [role="menuitem"], [contenteditable="true"], [data-interactive]';
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 [tabindex] missing from focusable selector

FOCUSABLE_SELECTOR doesn't include [tabindex], so custom interactive elements that rely solely on tabindex="0" (e.g. a <div tabindex="0"> acting as a focusable control) won't be caught by the guard. If such an element is the second [tabindex="0"] node inside a cell, a click on it would still redirect focus to the first [tabindex="0"] instead. Adding [tabindex]:not([tabindex="-1"]) to the selector closes that gap defensively.

Suggested change
const FOCUSABLE_SELECTOR =
'button, a, input, textarea, select, [role="button"], [role="link"], [role="combobox"], [role="menuitem"], [contenteditable="true"], [data-interactive]';
const FOCUSABLE_SELECTOR =
'button, a, input, textarea, select, [tabindex]:not([tabindex="-1"]), [role="button"], [role="link"], [role="combobox"], [role="menuitem"], [contenteditable="true"], [data-interactive]';
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/code/src/renderer/features/command-center/components/CommandCenterGrid.tsx
Line: 10-11

Comment:
**`[tabindex]` missing from focusable selector**

`FOCUSABLE_SELECTOR` doesn't include `[tabindex]`, so custom interactive elements that rely solely on `tabindex="0"` (e.g. a `<div tabindex="0">` acting as a focusable control) won't be caught by the guard. If such an element is the *second* `[tabindex="0"]` node inside a cell, a click on it would still redirect focus to the *first* `[tabindex="0"]` instead. Adding `[tabindex]:not([tabindex="-1"])` to the selector closes that gap defensively.

```suggestion
const FOCUSABLE_SELECTOR =
  'button, a, input, textarea, select, [tabindex]:not([tabindex="-1"]), [role="button"], [role="link"], [role="combobox"], [role="menuitem"], [contenteditable="true"], [data-interactive]';
```

How can I resolve this? If you propose a fix, please make it concise.

GridCell's onClick redirected focus to the cell's first tab stop on every
click, including ones that bubbled in through React's synthetic event
system from portaled popovers (BranchSelector, GitHubRepoPicker, etc.).
Skip the redirect when the click already lands on a focusable control or
bubbled from a target outside the cell's DOM.

Generated-By: PostHog Code
Task-Id: c9fad25f-633d-4125-af8b-2a6da16e859f
Active cell tracking previously keyed off activeTaskId, so cells without a
task (empty or in the new-task flow) never lit up the red border. Track the
active cell by index instead, and show the indicator whenever a cell is the
active one regardless of whether it has a task.

Generated-By: PostHog Code
Task-Id: c9fad25f-633d-4125-af8b-2a6da16e859f
@VojtechBartos VojtechBartos force-pushed the posthog-code/fix-command-center-repo-and-branch-selector-focus branch from 96d5377 to 1ef6963 Compare April 28, 2026 16:51
@VojtechBartos VojtechBartos requested a review from a team April 28, 2026 16:52
@VojtechBartos VojtechBartos marked this pull request as ready for review April 28, 2026 16:53
@greptile-apps
Copy link
Copy Markdown

greptile-apps Bot commented Apr 28, 2026

Comments Outside Diff (1)

  1. apps/code/src/renderer/features/command-center/stores/commandCenterStore.ts, line 102-116 (link)

    P1 activeCellIndex not updated in assignTask

    assignTask sets activeTaskId but leaves activeCellIndex unchanged, so the red-border active indicator can point to a different cell than the one that just received the task. This affects all callers that don't previously invoke setActiveCell: the sidebar (SidebarMenu.assignTaskToCommandCenter), TaskSelector, and the drag-and-drop handler in CommandCenterGrid. After any of those code paths, the highlighted cell and the "active task" cell diverge.

    Consider updating activeCellIndex inside assignTask to keep them in sync:

    assignTask: (cellIndex, taskId) =>
      set((state) => {
        if (cellIndex < 0 || cellIndex >= state.cells.length) return state;
        const cells = [...state.cells];
        const existingIndex = cells.indexOf(taskId);
        if (existingIndex !== -1) {
          cells[existingIndex] = null;
        }
        cells[cellIndex] = taskId;
        return {
          cells,
          activeTaskId: taskId,
          activeCellIndex: cellIndex,
          creatingCells: state.creatingCells.filter((i) => i !== cellIndex),
        };
      }),
    Prompt To Fix With AI
    This is a comment left during a code review.
    Path: apps/code/src/renderer/features/command-center/stores/commandCenterStore.ts
    Line: 102-116
    
    Comment:
    **`activeCellIndex` not updated in `assignTask`**
    
    `assignTask` sets `activeTaskId` but leaves `activeCellIndex` unchanged, so the red-border active indicator can point to a different cell than the one that just received the task. This affects all callers that don't previously invoke `setActiveCell`: the sidebar (`SidebarMenu.assignTaskToCommandCenter`), `TaskSelector`, and the drag-and-drop handler in `CommandCenterGrid`. After any of those code paths, the highlighted cell and the "active task" cell diverge.
    
    Consider updating `activeCellIndex` inside `assignTask` to keep them in sync:
    
    ```ts
    assignTask: (cellIndex, taskId) =>
      set((state) => {
        if (cellIndex < 0 || cellIndex >= state.cells.length) return state;
        const cells = [...state.cells];
        const existingIndex = cells.indexOf(taskId);
        if (existingIndex !== -1) {
          cells[existingIndex] = null;
        }
        cells[cellIndex] = taskId;
        return {
          cells,
          activeTaskId: taskId,
          activeCellIndex: cellIndex,
          creatingCells: state.creatingCells.filter((i) => i !== cellIndex),
        };
      }),
    ```
    
    How can I resolve this? If you propose a fix, please make it concise.
Prompt To Fix All With AI
This is a comment left during a code review.
Path: apps/code/src/renderer/features/command-center/stores/commandCenterStore.ts
Line: 102-116

Comment:
**`activeCellIndex` not updated in `assignTask`**

`assignTask` sets `activeTaskId` but leaves `activeCellIndex` unchanged, so the red-border active indicator can point to a different cell than the one that just received the task. This affects all callers that don't previously invoke `setActiveCell`: the sidebar (`SidebarMenu.assignTaskToCommandCenter`), `TaskSelector`, and the drag-and-drop handler in `CommandCenterGrid`. After any of those code paths, the highlighted cell and the "active task" cell diverge.

Consider updating `activeCellIndex` inside `assignTask` to keep them in sync:

```ts
assignTask: (cellIndex, taskId) =>
  set((state) => {
    if (cellIndex < 0 || cellIndex >= state.cells.length) return state;
    const cells = [...state.cells];
    const existingIndex = cells.indexOf(taskId);
    if (existingIndex !== -1) {
      cells[existingIndex] = null;
    }
    cells[cellIndex] = taskId;
    return {
      cells,
      activeTaskId: taskId,
      activeCellIndex: cellIndex,
      creatingCells: state.creatingCells.filter((i) => i !== cellIndex),
    };
  }),
```

How can I resolve this? If you propose a fix, please make it concise.

Reviews (2): Last reviewed commit: "fix(code): show active cell indicator fo..." | Re-trigger Greptile

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant