Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 0 additions & 10 deletions src/renderer/App.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -10,13 +10,11 @@ import {
} from "./store";
import { EmailList } from "./components/EmailList";
import { EmailDetail } from "./components/EmailDetail";
import { EmailPreviewSidebar } from "./components/EmailPreviewSidebar";
import { SettingsPanel } from "./components/SettingsPanel";
import { SetupWizard } from "./components/SetupWizard";
import { SearchBar } from "./components/SearchBar";
import { CommandPalette } from "./components/CommandPalette";
import { AgentCommandPalette } from "./components/AgentCommandPalette";
import { AgentsSidebar } from "./components/AgentsSidebar";
import { ShortcutHelp } from "./components/ShortcutHelp";
import { KeyboardHints } from "./components/KeyboardHints";
import { OfflineBanner } from "./components/OfflineBanner";
Expand Down Expand Up @@ -646,7 +644,6 @@ export default function App() {
const isCommandPaletteOpen = useAppStore((s) => s.isCommandPaletteOpen);
const isFindBarOpen = useAppStore((s) => s.isFindBarOpen);
const isAgentPaletteOpen = useAppStore((s) => s.isAgentPaletteOpen);
const isAgentsSidebarOpen = useAppStore((s) => s.isAgentsSidebarOpen);
const viewMode = useAppStore((s) => s.viewMode);
const activeSearchQuery = useAppStore((s) => s.activeSearchQuery);
const _activeSearchResults = useAppStore((s) => s.activeSearchResults);
Expand Down Expand Up @@ -2242,9 +2239,6 @@ export default function App() {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Agent Sidebar Becomes Unreachable

This removes the mounted AgentsSidebar along with the right preview sidebar, but the agent palette entry point still lets users start agent runs. When a user runs agents through the remaining command path, isAgentsSidebarOpen and toggleAgentsSidebar() no longer have any rendered consumer, so the workflow can start without the sidebar surface that shows its state or progress.

Prompt To Fix With AI
This is a comment left during a code review.
Path: src/renderer/App.tsx
Line: 2239

Comment:
**Agent Sidebar Becomes Unreachable**

This removes the mounted `AgentsSidebar` along with the right preview sidebar, but the agent palette entry point still lets users start agent runs. When a user runs agents through the remaining command path, `isAgentsSidebarOpen` and `toggleAgentsSidebar()` no longer have any rendered consumer, so the workflow can start without the sidebar surface that shows its state or progress.

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

{/* Main content */}
<div className="flex-1 flex overflow-hidden">
{/* Agents sidebar (collapsible left panel) */}
{isAgentsSidebarOpen && <AgentsSidebar />}

{/* Search results view (shown when search is active and not viewing a specific email) */}
{activeSearchQuery && viewMode !== "full" && <SearchResultsView />}

Expand All @@ -2262,10 +2256,6 @@ export default function App() {

{/* Full mode: full email detail view */}
{viewMode === "full" && <EmailDetail isFullView />}

{/* Preview sidebar — kept mounted across view mode transitions to avoid
expensive unmount/remount of agent trace timelines */}
{(!activeSearchQuery || viewMode === "full") && <EmailPreviewSidebar />}
</div>

{/* Keyboard hints bar */}
Expand Down
7 changes: 0 additions & 7 deletions src/renderer/components/CommandPalette.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -575,13 +575,6 @@ export function CommandPalette({ isOpen, onClose }: CommandPaletteProps) {
},

// --- Agents ---
{
id: "open-agents-sidebar",
label: "Open Agents Sidebar",
category: "Agents",
icon: ICONS.settings,
execute: () => useAppStore.getState().toggleAgentsSidebar(),
},
{
id: "run-with-agents",
label: "Run with Selected Agents",
Expand Down
1 change: 0 additions & 1 deletion src/renderer/components/KeyboardHints.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@ const DEFAULT_HINTS: Hint[] = [
{ key: "x", label: "select" },
{ key: "c", label: "compose" },
{ key: "/", label: "search" },
{ key: "b", label: "sidebar" },
{ key: "\u2318K", label: "commands" },
];

Expand Down
26 changes: 4 additions & 22 deletions src/renderer/hooks/useKeyboardShortcuts.ts

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 Keyboard shortcut help dialog still advertises arrow keys for navigation after they were removed

The shortcuts help dialog still lists "j / ↓" and "k / ↑" as navigation keys (getKeyboardShortcuts at src/renderer/hooks/useKeyboardShortcuts.ts:1135-1136), but the arrow key handlers were removed from the keyboard event handler in this PR, so pressing ↓ or ↑ no longer navigates the email list.

Impact: Users who open the help dialog (?) will see arrow keys documented as working shortcuts, but pressing them does nothing for email navigation.

Stale help text after arrow key handler removal

The PR removed case "ArrowDown": and case "ArrowUp": from the switch statement in the keyboard handler (around lines 766-785), but the getKeyboardShortcuts() function at src/renderer/hooks/useKeyboardShortcuts.ts:1135-1136 still returns:

{ key: "j / ↓", description: "Move down" },
{ key: "k / ↑", description: "Move up" },

These should be updated to just "j" and "k" respectively. The ShortcutHelp component at src/renderer/components/ShortcutHelp.tsx:36 consumes this data and renders it in the help modal.

(Refers to lines 1135-1136)

Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

Original file line number Diff line number Diff line change
Expand Up @@ -251,7 +251,7 @@ export function useKeyboardShortcuts(options: UseKeyboardShortcutsOptions = {})

// In compose mode, only handle Cmd+Enter for send — except when the
// editor isn't focused (e.g. auto-opened draft without focus), where
// Enter should focus the editor and "b" should switch sidebar tabs.
// Enter should focus the editor.
if (mode === "compose" && isInputFocused()) {
return;
}
Expand Down Expand Up @@ -674,19 +674,15 @@ export function useKeyboardShortcuts(options: UseKeyboardShortcutsOptions = {})
return;
}

// --- Shift+J/K/Arrow: extend selection up/down ---
if (
e.shiftKey &&
(e.key === "J" || e.key === "K" || e.key === "ArrowDown" || e.key === "ArrowUp") &&
!activeSearchQuery
) {
// --- Shift+J/K: extend selection up/down ---
if (e.shiftKey && (e.key === "J" || e.key === "K") && !activeSearchQuery) {
e.preventDefault();
markNavigationActive();
if (visibleThreads.length === 0) return;
const currentIndex = visibleThreads.findIndex((t) => t.threadId === selectedThreadId);
if (currentIndex < 0) return;

const direction = e.key === "J" || e.key === "ArrowDown" ? 1 : -1;
const direction = e.key === "J" ? 1 : -1;
const nextIndex = currentIndex + direction;
if (nextIndex < 0 || nextIndex >= visibleThreads.length) return;

Expand Down Expand Up @@ -768,11 +764,6 @@ export function useKeyboardShortcuts(options: UseKeyboardShortcutsOptions = {})
switch (e.key) {
// Navigation
case "j":
case "ArrowDown":
// Skip Shift+Arrow — arrow keys don't change e.key when shift is held
// (unlike j→J), so without this guard Shift+ArrowDown would navigate
// instead of being a no-op like Shift+J in the switch.
if (e.shiftKey && e.key.startsWith("Arrow")) break;
e.preventDefault();
// Defer any pending sync-driven store updates while navigating
markNavigationActive();
Expand All @@ -784,8 +775,6 @@ export function useKeyboardShortcuts(options: UseKeyboardShortcutsOptions = {})
break;

case "k":
case "ArrowUp":
if (e.shiftKey && e.key.startsWith("Arrow")) break;
e.preventDefault();
markNavigationActive();
if (activeSearchQuery) {
Expand Down Expand Up @@ -1076,12 +1065,6 @@ export function useKeyboardShortcuts(options: UseKeyboardShortcutsOptions = {})
}
break;

// Switch sidebar tab
case "b":
e.preventDefault();
state.cycleSidebarTab();
break;

// z: undo last action (Gmail only — no modifier needed)
case "z":
if (isGmail && !e.shiftKey) {
Expand Down Expand Up @@ -1201,7 +1184,6 @@ export function getKeyboardShortcuts(bindings: "superhuman" | "gmail") {
{ key: "Cmd+J", description: "Agent action palette" },
],
other: [
{ key: "b", description: "Switch sidebar tab" },
{ key: "Cmd+,", description: "Settings" },
{ key: "?", description: "Show shortcuts" },
],
Expand Down
141 changes: 46 additions & 95 deletions tests/e2e/arrow-key-navigation.spec.ts
Original file line number Diff line number Diff line change
@@ -1,14 +1,5 @@
import { test, expect, Page, ElectronApplication } from "@playwright/test";
import { launchElectronApp , closeApp } from "./launch-helpers";

/** Best-effort screenshot */
async function screenshot(page: Page, name: string) {
const { mkdirSync } = await import("fs");
mkdirSync("tests/screenshots", { recursive: true });
await page.screenshot({ path: `tests/screenshots/${name}.png`, timeout: 5000 }).catch(() => {
console.log(`Screenshot '${name}' timed out, skipping`);
});
}
import { launchElectronApp, closeApp, waitForEmailListReady } from "./launch-helpers";

/** Get the data-thread-id of the currently selected (highlighted) row */
async function getSelectedThreadId(page: Page): Promise<string | null> {
Expand All @@ -19,7 +10,24 @@ async function getSelectedThreadId(page: Page): Promise<string | null> {
return null;
}

test.describe("Arrow Key Navigation", () => {
async function clearSelection(page: Page): Promise<void> {
for (let i = 0; i < 3; i++) {
if (!(await getSelectedThreadId(page))) return;
await page.keyboard.press("Escape");
await page.waitForTimeout(150);
}
}

async function selectFirstThread(page: Page): Promise<string> {
const selectedRow = page.locator("div[data-thread-id][data-selected='true']");
await page.keyboard.press("j");
await expect(selectedRow).toBeVisible({ timeout: 5000 });
const selectedThreadId = await getSelectedThreadId(page);
expect(selectedThreadId).not.toBeNull();
return selectedThreadId!;
}

test.describe("Arrow keys keep native scroll behavior", () => {
test.describe.configure({ mode: "serial" });
let electronApp: ElectronApplication;
let page: Page;
Expand All @@ -42,109 +50,52 @@ test.describe("Arrow Key Navigation", () => {
}
});

test("ArrowDown selects first email, same as j", async () => {
await page.waitForTimeout(500);
await screenshot(page, "arrow-nav-01-initial");
test("ArrowDown does not select or open mail from the inbox", async () => {
await waitForEmailListReady(page);
await clearSelection(page);

// Press ArrowDown to select the first thread
await page.keyboard.press("ArrowDown");
await page.waitForTimeout(300);

const selectedAfterArrow = await getSelectedThreadId(page);
expect(selectedAfterArrow).not.toBeNull();

await screenshot(page, "arrow-nav-02-after-arrow-down");
expect(await getSelectedThreadId(page)).toBeNull();
await expect(page.locator("button[title='Reply All']").first()).toBeHidden({ timeout: 3000 });
await expect(page.locator(".w-96.exo-preview-shell")).toBeHidden({ timeout: 3000 });
await expect(page.locator(".w-64.exo-preview-shell")).toBeHidden({ timeout: 3000 });
});

test("ArrowDown and j navigate to the same positions", async () => {
// Start fresh: press Escape to deselect
await page.keyboard.press("Escape");
await page.waitForTimeout(300);

// Navigate down with j
await page.keyboard.press("j");
await page.waitForTimeout(300);
const afterJ1 = await getSelectedThreadId(page);

await page.keyboard.press("j");
await page.waitForTimeout(300);
const afterJ2 = await getSelectedThreadId(page);

// Go back up to the first position with k
await page.keyboard.press("k");
await page.waitForTimeout(300);

// Now navigate with ArrowDown from the same start position
const backToFirst = await getSelectedThreadId(page);
expect(backToFirst).toBe(afterJ1);
test("ArrowUp and ArrowDown do not move the highlighted inbox row", async () => {
await waitForEmailListReady(page);
await clearSelection(page);
const selectedBefore = await selectFirstThread(page);

await page.keyboard.press("ArrowDown");
await page.waitForTimeout(300);
const afterArrow = await getSelectedThreadId(page);

// ArrowDown should land on the same thread as the second j press
expect(afterArrow).toBe(afterJ2);

await screenshot(page, "arrow-nav-03-arrow-matches-j");
});

test("ArrowUp navigates up, same as k", async () => {
// Navigate down a couple times first
await page.keyboard.press("Escape");
await page.waitForTimeout(300);

await page.keyboard.press("j");
await page.waitForTimeout(200);
await page.keyboard.press("j");
await page.waitForTimeout(200);
await page.keyboard.press("j");
await page.waitForTimeout(200);
const thirdPosition = await getSelectedThreadId(page);

// Go up with k
await page.keyboard.press("k");
await page.waitForTimeout(300);
const afterK = await getSelectedThreadId(page);

// Go back down to third position
await page.keyboard.press("j");
await page.waitForTimeout(300);
expect(await getSelectedThreadId(page)).toBe(thirdPosition);
expect(await getSelectedThreadId(page)).toBe(selectedBefore);

// Go up with ArrowUp
await page.keyboard.press("ArrowUp");
await page.waitForTimeout(300);
const afterArrowUp = await getSelectedThreadId(page);
expect(await getSelectedThreadId(page)).toBe(selectedBefore);

// Should match the k result
expect(afterArrowUp).toBe(afterK);

await screenshot(page, "arrow-nav-04-arrow-up-matches-k");
await expect(page.locator(".w-96.exo-preview-shell")).toBeHidden({ timeout: 3000 });
await expect(page.locator(".w-64.exo-preview-shell")).toBeHidden({ timeout: 3000 });
});

test("mixed arrow and j/k navigation works together", async () => {
await page.keyboard.press("Escape");
await page.waitForTimeout(300);
test("Arrow keys do not reveal a sidebar in full mail view", async () => {
await waitForEmailListReady(page);
await clearSelection(page);
await selectFirstThread(page);

// j → ArrowDown → k → ArrowUp should return to start
await page.keyboard.press("j");
await page.waitForTimeout(200);
const start = await getSelectedThreadId(page);
await page.keyboard.press("Enter");
const replyButton = page.locator("button[title='Reply All']").first();
await expect(replyButton).toBeVisible({ timeout: 5000 });

await page.keyboard.press("ArrowDown");
await page.waitForTimeout(200);

await page.keyboard.press("k");
await page.waitForTimeout(200);
expect(await getSelectedThreadId(page)).toBe(start);

await page.keyboard.press("ArrowDown");
await page.waitForTimeout(200);

await page.waitForTimeout(300);
await page.keyboard.press("ArrowUp");
await page.waitForTimeout(200);
expect(await getSelectedThreadId(page)).toBe(start);
await page.waitForTimeout(300);

await screenshot(page, "arrow-nav-05-mixed-navigation");
await expect(replyButton).toBeVisible({ timeout: 3000 });
await expect(page.locator(".w-96.exo-preview-shell")).toBeHidden({ timeout: 3000 });
await expect(page.locator(".w-64.exo-preview-shell")).toBeHidden({ timeout: 3000 });
});
});
14 changes: 6 additions & 8 deletions tests/e2e/keyboard-flow.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -85,28 +85,26 @@ test.describe("Keyboard Navigation - j/k Movement", () => {
}
});

test("ArrowDown works the same as j", async () => {
test("ArrowDown does not move the selected email", async () => {
const before = await getSelectedThreadId(page);
expect(before).not.toBeNull();

await page.keyboard.press("ArrowDown");
await page.waitForTimeout(300);

const after = await getSelectedThreadId(page);
expect(after).not.toBeNull();
expect(after).toBe(before);
});

test("ArrowUp works the same as k", async () => {
// Move down first
await page.keyboard.press("ArrowDown");
await page.waitForTimeout(200);

test("ArrowUp does not move the selected email", async () => {
const before = await getSelectedThreadId(page);
expect(before).not.toBeNull();

await page.keyboard.press("ArrowUp");
await page.waitForTimeout(300);

const after = await getSelectedThreadId(page);
expect(after).not.toBeNull();
expect(after).toBe(before);
});

test("j at the bottom of list stays at last email", async () => {
Expand Down
Loading