Skip to content
Merged
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
Original file line number Diff line number Diff line change
Expand Up @@ -136,7 +136,10 @@ export function useRecordHotkeys(
}

const defaultKey = HOTKEYS[recordingId].key;
if (canonicalizeChord(captured) === canonicalizeChord(defaultKey)) {
if (
defaultKey &&
canonicalizeChord(captured) === canonicalizeChord(defaultKey)
) {
resetOverride(recordingId);
} else {
setOverride(recordingId, captured);
Expand Down
24 changes: 24 additions & 0 deletions apps/desktop/src/renderer/hotkeys/registry.ts
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,18 @@ export const HOTKEYS_REGISTRY = {
label: "Switch to Workspace 9",
category: "Workspace",
},
PREV_WORKSPACE: {
key: { mac: null, windows: null, linux: null },
label: "Previous Workspace",
category: "Workspace",
description: "Navigate to the previous workspace in the sidebar",
},
NEXT_WORKSPACE: {
key: { mac: null, windows: null, linux: null },
label: "Next Workspace",
category: "Workspace",
description: "Navigate to the next workspace in the sidebar",
},
CLOSE_WORKSPACE: {
key: {
mac: "meta+shift+backspace",
Expand Down Expand Up @@ -330,6 +342,18 @@ export const HOTKEYS_REGISTRY = {
label: "Next Tab (Alt)",
category: "Terminal",
},
PREV_TAB: {
key: { mac: null, windows: null, linux: null },
label: "Previous Tab",
category: "Terminal",
description: "Focus the previous tab in the active workspace",
},
NEXT_TAB: {
key: { mac: null, windows: null, linux: null },
label: "Next Tab",
category: "Terminal",
description: "Focus the next tab in the active workspace",
},
FOCUS_PANE_LEFT: {
key: {
mac: "meta+alt+left",
Expand Down
8 changes: 6 additions & 2 deletions apps/desktop/src/renderer/hotkeys/types.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,10 @@
export type Platform = "mac" | "windows" | "linux";

export type PlatformKey = { mac: string; windows: string; linux: string };
export type PlatformKey = {
mac: string | null;
windows: string | null;
linux: string | null;
};

export type HotkeyCategory =
| "Navigation"
Expand All @@ -18,7 +22,7 @@ export interface HotkeyDisplay {
}

export interface HotkeyDefinition {
key: string;
key: string | null;
label: string;
category: HotkeyCategory;
description?: string;
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import { afterEach, beforeEach, describe, expect, it } from "bun:test";
import { HOTKEYS } from "../registry";
import { HOTKEYS, type HotkeyId } from "../registry";
import { useHotkeyOverridesStore } from "../stores/hotkeyOverridesStore";
import type { HotkeyDefinition } from "../types";
import {
canonicalizeChord,
eventToChord,
Expand Down Expand Up @@ -148,10 +149,13 @@ describe("resolveHotkeyFromEvent — live override index", () => {
});

// Resolve once so registry reorders / removals surface as a test failure
// here instead of silently skipping the cases below.
const sampleEntry = (
Object.entries(HOTKEYS) as [keyof typeof HOTKEYS, { key: string }][]
).find(([, hotkey]) => !!hotkey.key);
// here instead of silently skipping the cases below. The type predicate
// narrows to a HotkeyDefinition whose .key is guaranteed non-null after
// the filter, so sampleDef.key can be passed to string-only helpers below.
const sampleEntry = Object.entries(HOTKEYS).find(
(entry): entry is [HotkeyId, HotkeyDefinition & { key: string }] =>
entry[1].key !== null,
);
if (!sampleEntry) throw new Error("HOTKEYS has no bound default");
const [sampleId, sampleDef] = sampleEntry;

Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { useNavigate } from "@tanstack/react-router";
import { useMatchRoute, useNavigate } from "@tanstack/react-router";
import { useCallback, useMemo } from "react";
import { useHotkey } from "renderer/hotkeys";
import { navigateToV2Workspace } from "renderer/routes/_authenticated/_dashboard/utils/workspace-navigation";
Expand Down Expand Up @@ -48,5 +48,33 @@ export function useDashboardSidebarShortcuts(
useHotkey("JUMP_TO_WORKSPACE_8", () => switchToWorkspace(7));
useHotkey("JUMP_TO_WORKSPACE_9", () => switchToWorkspace(8));

const matchRoute = useMatchRoute();
const currentWorkspaceMatch = matchRoute({
to: "/v2-workspace/$workspaceId",
fuzzy: true,
});
const currentWorkspaceId =
currentWorkspaceMatch !== false ? currentWorkspaceMatch.workspaceId : null;

useHotkey("PREV_WORKSPACE", () => {
if (!currentWorkspaceId || flattenedWorkspaces.length === 0) return;
const index = flattenedWorkspaces.findIndex(
(w) => w.id === currentWorkspaceId,
);
if (index === -1) return;
const prevIndex = index <= 0 ? flattenedWorkspaces.length - 1 : index - 1;
navigateToV2Workspace(flattenedWorkspaces[prevIndex].id, navigate);
Comment thread
cubic-dev-ai[bot] marked this conversation as resolved.
});
Comment on lines +59 to +67
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P2 PREV_WORKSPACE wraps to last when workspace not in filtered list

When findIndex returns -1 (the current workspace has creationStatus set and is therefore filtered out of flattenedWorkspaces), index <= 0 evaluates to true and prevIndex becomes flattenedWorkspaces.length - 1 (last workspace). The NEXT_WORKSPACE handler below explicitly guards index === -1 and sends the user to the first workspace instead. The asymmetry means the two hotkeys disagree on where to land for the same edge-case input.

Consider mirroring the explicit -1 guard here:

Suggested change
useHotkey("PREV_WORKSPACE", () => {
if (!currentWorkspaceId || flattenedWorkspaces.length === 0) return;
const index = flattenedWorkspaces.findIndex(
(w) => w.id === currentWorkspaceId,
);
const prevIndex = index <= 0 ? flattenedWorkspaces.length - 1 : index - 1;
navigateToV2Workspace(flattenedWorkspaces[prevIndex].id, navigate);
});
useHotkey("PREV_WORKSPACE", () => {
if (!currentWorkspaceId || flattenedWorkspaces.length === 0) return;
const index = flattenedWorkspaces.findIndex(
(w) => w.id === currentWorkspaceId,
);
if (index === -1) {
navigateToV2Workspace(flattenedWorkspaces[0].id, navigate);
return;
}
const prevIndex = index <= 0 ? flattenedWorkspaces.length - 1 : index - 1;
navigateToV2Workspace(flattenedWorkspaces[prevIndex].id, navigate);
});

Alternatively, bail out when index === -1 (do nothing) if navigating away from a workspace-being-created feels undesirable. Either way, the behavior should be intentional and consistent between the two handlers.


useHotkey("NEXT_WORKSPACE", () => {
if (!currentWorkspaceId || flattenedWorkspaces.length === 0) return;
const index = flattenedWorkspaces.findIndex(
(w) => w.id === currentWorkspaceId,
);
if (index === -1) return;
const nextIndex = index >= flattenedWorkspaces.length - 1 ? 0 : index + 1;
navigateToV2Workspace(flattenedWorkspaces[nextIndex].id, navigate);
});

return workspaceShortcutLabels;
}
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,23 @@ export function useWorkspaceHotkeys({
}
});

useHotkey("PREV_TAB", () => {
const state = store.getState();
if (!state.activeTabId || state.tabs.length === 0) return;
const index = state.tabs.findIndex((t) => t.id === state.activeTabId);
const prevIndex = index <= 0 ? state.tabs.length - 1 : index - 1;
state.setActiveTab(state.tabs[prevIndex].id);
});

useHotkey("NEXT_TAB", () => {
const state = store.getState();
if (!state.activeTabId || state.tabs.length === 0) return;
const index = state.tabs.findIndex((t) => t.id === state.activeTabId);
const nextIndex =
index >= state.tabs.length - 1 || index === -1 ? 0 : index + 1;
state.setActiveTab(state.tabs[nextIndex].id);
});

useHotkey("PREV_TAB_ALT", () => {
const state = store.getState();
if (!state.activeTabId || state.tabs.length === 0) return;
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import type { ExternalApp } from "@superset/local-db";
import { createFileRoute, notFound } from "@tanstack/react-router";
import { createFileRoute, notFound, useNavigate } from "@tanstack/react-router";
import { useCallback, useEffect, useMemo, useState } from "react";
import { useCopyToClipboard } from "renderer/hooks/useCopyToClipboard";
import { useFileOpenMode } from "renderer/hooks/useFileOpenMode";
Expand All @@ -8,6 +8,7 @@ import { electronTrpc } from "renderer/lib/electron-trpc";
import { electronTrpcClient as trpcClient } from "renderer/lib/trpc-client";
import { usePresets } from "renderer/react-query/presets";
import type { WorkspaceSearchParams } from "renderer/routes/_authenticated/_dashboard/utils/workspace-navigation";
import { navigateToWorkspace } from "renderer/routes/_authenticated/_dashboard/utils/workspace-navigation";
import { usePresetHotkeys } from "renderer/routes/_authenticated/_dashboard/workspace/$workspaceId/hooks/usePresetHotkeys";
import { useWorkspaceRunCommand } from "renderer/routes/_authenticated/_dashboard/workspace/$workspaceId/hooks/useWorkspaceRunCommand";
import { NotFound } from "renderer/routes/not-found";
Expand Down Expand Up @@ -90,6 +91,7 @@ function WorkspacePage() {
worktreePath: workspace?.worktreePath,
enabled: Boolean(workspace?.worktreePath),
});
const navigate = useNavigate();
const routeNavigate = Route.useNavigate();
const { tabId: searchTabId, paneId: searchPaneId } = Route.useSearch();

Expand Down Expand Up @@ -224,6 +226,20 @@ function WorkspacePage() {
}
});

useHotkey("PREV_TAB", () => {
if (!activeTabId || tabs.length === 0) return;
const index = tabs.findIndex((t) => t.id === activeTabId);
const prevIndex = index <= 0 ? tabs.length - 1 : index - 1;
setActiveTab(workspaceId, tabs[prevIndex].id);
});

useHotkey("NEXT_TAB", () => {
if (!activeTabId || tabs.length === 0) return;
const index = tabs.findIndex((t) => t.id === activeTabId);
const nextIndex = index >= tabs.length - 1 || index === -1 ? 0 : index + 1;
setActiveTab(workspaceId, tabs[nextIndex].id);
});

useHotkey("PREV_TAB_ALT", () => {
if (!activeTabId || tabs.length === 0) return;
const index = tabs.findIndex((t) => t.id === activeTabId);
Expand Down Expand Up @@ -393,6 +409,29 @@ function WorkspacePage() {
}
});

const getPreviousWorkspace =
electronTrpc.workspaces.getPreviousWorkspace.useQuery(
{ id: workspaceId },
{ enabled: !!workspaceId },
);
useHotkey("PREV_WORKSPACE", () => {
const prevWorkspaceId = getPreviousWorkspace.data;
if (prevWorkspaceId) {
navigateToWorkspace(prevWorkspaceId, navigate);
}
});

const getNextWorkspace = electronTrpc.workspaces.getNextWorkspace.useQuery(
{ id: workspaceId },
{ enabled: !!workspaceId },
);
useHotkey("NEXT_WORKSPACE", () => {
const nextWorkspaceId = getNextWorkspace.data;
if (nextWorkspaceId) {
navigateToWorkspace(nextWorkspaceId, navigate);
}
Comment on lines +412 to +432
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P2 Hotkey silently does nothing while tRPC queries are loading

getPreviousWorkspace.data and getNextWorkspace.data are undefined until the IPC queries resolve. If the user presses PREV_WORKSPACE or NEXT_WORKSPACE within that window (e.g. immediately after navigating to a workspace), the handler returns without side-effects and gives no feedback. For most users on a warm cache this window is imperceptible, but it can surface as a confusing "hotkey didn't work" on first load.

A minimal improvement is to check getPreviousWorkspace.isLoading and either disable the hotkey registration or show a loading indicator, though this may be over-engineering for a desktop-only path. Worth documenting the known limitation at minimum.

Also: { enabled: !!workspaceId } is always true because workspaceId is a required route param that is never an empty string. The guard can be simplified to { enabled: true } or removed entirely.

});

return (
<div className="flex-1 h-full flex flex-col overflow-hidden">
<div className="flex-1 min-h-0 flex overflow-hidden">
Expand Down
Loading