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
3 changes: 2 additions & 1 deletion frontend/src/lib/components/keyboard/Cheatsheet.svelte
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
getAllActions,
getAllCheatsheetEntries,
} from "../../stores/keyboard/registry.svelte.js";
import { isActionVisible } from "../../stores/keyboard/visibility.js";
import type {
Action,
CheatsheetEntry,
Expand Down Expand Up @@ -46,7 +47,7 @@
// unit tests drive grouping without standing up the full app context.
const visibleActions = $derived<Action[]>(
stores
? getAllActions().filter((a) => a.when(buildContext(stores)))
? getAllActions().filter((a) => isActionVisible(a, buildContext(stores)))
: getAllActions(),
);

Expand Down
3 changes: 2 additions & 1 deletion frontend/src/lib/components/keyboard/Palette.svelte
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
import { buildContext } from "../../stores/keyboard/context.svelte.js";
import { handleCommandResult } from "../../stores/keyboard/dispatch.svelte.js";
import { getAllActions } from "../../stores/keyboard/registry.svelte.js";
import { isActionVisible } from "../../stores/keyboard/visibility.js";
import {
groupResults,
parsePaletteQuery,
Expand Down Expand Up @@ -70,7 +71,7 @@
return getAllActions();
}
const ctx = buildContext(stores);
return getAllActions().filter((a) => a.when(ctx));
return getAllActions().filter((a) => isActionVisible(a, ctx));
});
const grouped = $derived(
groupResults({
Expand Down
3 changes: 1 addition & 2 deletions frontend/src/lib/components/layout/AppHeader.svelte
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
<script lang="ts">
import { getStores, KbdBadge, SelectDropdown } from "@middleman/ui";
import { getStores, SelectDropdown } from "@middleman/ui";
import {
getBasePath,
getPage,
Expand Down Expand Up @@ -161,7 +161,6 @@
strokeWidth="1.5"
aria-hidden="true"
/>
<KbdBadge binding={{ key: "[", ctrlOrMeta: true }} />
</HeaderIconButton>
{/if}
<span class="brand">
Expand Down
10 changes: 10 additions & 0 deletions frontend/src/lib/components/layout/AppHeader.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -258,6 +258,16 @@ describe("AppHeader", () => {
expect(workspaceOptions[0]?.getAttribute("aria-selected")).toBe("true");
});

it("does not show the collapsed sidebar shortcut hint on Activity", () => {
initTheme();
navigate("/");
setSidebarCollapsed(true);
const { container } = render(AppHeader);

expect(container.querySelector("button[title='Expand sidebar']")).toBeTruthy();
expect(container.querySelector("kbd[aria-label]")).toBeNull();
});

it("opens selected Activity PR in PRs tab with files tab preserved", async () => {
initTheme();
navigate("/?selected=pr:1&provider=github&platform_host=github.com&repo_path=acme%2Fwidgets&selected_tab=files");
Expand Down
85 changes: 84 additions & 1 deletion frontend/src/lib/stores/keyboard/actions.test.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import { describe, expect, it } from "vite-plus/test";
import { afterEach, describe, expect, it } from "vite-plus/test";

import { defaultActions, setStoreInstances } from "./actions.js";
import { isSidebarCollapsed, setSidebarCollapsed } from "../sidebar.svelte.js";
import {
OPEN_LABEL_PICKER_EVENT,
type OpenLabelPickerDetail,
Expand All @@ -15,6 +16,7 @@ function ctx(page: Context["page"], overrides: Partial<Context> = {}): Context {
selectedIssue: null,
isDiffView: false,
detailTab: "conversation",
sidebarTargetAvailable: true,
...overrides,
};
}
Expand All @@ -38,6 +40,10 @@ const selected = {
};

describe("defaultActions", () => {
afterEach(() => {
setSidebarCollapsed(false);
});

it("includes the migrated globals", () => {
const ids = defaultActions.map((a) => a.id);
expect(ids).toEqual(
Expand Down Expand Up @@ -156,4 +162,81 @@ describe("defaultActions", () => {
expect(cheatsheet!.when(ctx("pulls"))).toBe(true);
expect(cheatsheet!.when(ctx("issues"))).toBe(true);
});

it("sidebar.toggle reserves the chord everywhere but only toggles pages with a sidebar target", () => {
const action = defaultActions.find((a) => a.id === "sidebar.toggle");
expect(action).toBeDefined();

const visible = action!.visible ?? action!.when;

expect(action!.when(ctx("activity"))).toBe(true);
expect(visible(ctx("activity"))).toBe(false);
setSidebarCollapsed(false);
action!.handler(ctx("activity"));
expect(isSidebarCollapsed()).toBe(false);

expect(action!.when(ctx("repos"))).toBe(true);
expect(visible(ctx("repos"))).toBe(false);
expect(
action!.when(
ctx("pulls", {
route: { page: "pulls", view: "list" } as never,
}),
),
).toBe(true);
expect(
visible(
ctx("pulls", {
route: { page: "pulls", view: "list" } as never,
}),
),
).toBe(true);
action!.handler(
ctx("pulls", {
route: { page: "pulls", view: "list" } as never,
}),
);
expect(isSidebarCollapsed()).toBe(true);
setSidebarCollapsed(false);
const compactPulls = ctx("pulls", {
route: { page: "pulls", view: "list" } as never,
sidebarTargetAvailable: false,
});
expect(action!.when(compactPulls)).toBe(true);
expect(visible(compactPulls)).toBe(false);
action!.handler(compactPulls);
expect(isSidebarCollapsed()).toBe(false);
expect(
action!.when(
ctx("pulls", {
route: { page: "pulls", view: "board" } as never,
}),
),
).toBe(true);
expect(
visible(
ctx("pulls", {
route: { page: "pulls", view: "board" } as never,
}),
),
).toBe(false);
expect(action!.when(ctx("issues"))).toBe(true);
expect(visible(ctx("issues"))).toBe(true);
expect(action!.when(ctx("workspaces"))).toBe(true);
expect(visible(ctx("workspaces"))).toBe(true);
expect(
action!.when(
ctx("terminal", {
route: { page: "terminal", workspaceId: "1" } as never,
}),
),
).toBe(true);
expect(
visible(
ctx("terminal", {
route: { page: "terminal", workspaceId: "1" } as never,
}),
),
).toBe(true);
});
});
19 changes: 18 additions & 1 deletion frontend/src/lib/stores/keyboard/actions.ts
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,20 @@ const onPullsListNotBoard = (ctx: Context): boolean => ctx.page === "pulls" && !

const onIssuesList = (ctx: Context): boolean => ctx.page === "issues";

function hasSidebarShortcutTarget(ctx: Context): boolean {
if (!ctx.sidebarTargetAvailable) return false;
switch (ctx.route.page) {
case "pulls":
return ctx.route.view === "list";
case "issues":
case "workspaces":
case "terminal":
return true;
default:
return false;
}
}

type LabelEditableSelection = Omit<OpenLabelPickerDetail, "itemType">;

type LabelEditableDetail = {
Expand Down Expand Up @@ -244,7 +258,10 @@ export const defaultActions: Action[] = [
binding: { key: "[", ctrlOrMeta: true },
priority: 0,
when: () => isSidebarToggleEnabled(),
handler: () => toggleSidebar(),
visible: (ctx) => isSidebarToggleEnabled() && hasSidebarShortcutTarget(ctx),
handler: (ctx) => {
if (hasSidebarShortcutTarget(ctx)) toggleSidebar();
},
},
{
id: "palette.open",
Expand Down
11 changes: 9 additions & 2 deletions frontend/src/lib/stores/keyboard/context.svelte.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { getRoute, getPage, getDetailTab, isDiffView, getSelectedPRFromRoute } from "../router.svelte.js";
import { getRoute, getPage, getDetailTab, isDiffView, getSelectedPRFromRoute, type Route } from "../router.svelte.js";
import type { Context } from "./types.js";
import type { PullSelection } from "@middleman/ui/stores/pulls";
import type { IssueSelection } from "@middleman/ui/stores/issues";
Expand All @@ -8,16 +8,23 @@ interface SelectionSources {
issues: { getSelectedIssue: () => IssueSelection | null };
}

function sidebarTargetAvailable(route: Route): boolean {
if (route.page !== "pulls" && route.page !== "issues") return true;
return document.querySelector(".sidebar") !== null;
}

export function buildContext(stores: SelectionSources): Context {
const route = getRoute();
return {
page: getPage(),
route: getRoute(),
route,
// Mirror App.svelte's render path: route-derived selection wins so PR
// detail navigation via deep-link or back/forward keeps actions enabled
// before the pulls store has hydrated.
selectedPR: getSelectedPRFromRoute() ?? stores.pulls.getSelectedPR(),
selectedIssue: stores.issues.getSelectedIssue(),
isDiffView: isDiffView(),
detailTab: getDetailTab(),
sidebarTargetAvailable: sidebarTargetAvailable(route),
};
}
24 changes: 23 additions & 1 deletion frontend/src/lib/stores/keyboard/dispatch.svelte.test.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,9 @@
import { beforeEach, describe, expect, it, vi } from "vite-plus/test";
import { afterEach, beforeEach, describe, expect, it, vi } from "vite-plus/test";

import { dispatchKeydown } from "./dispatch.svelte.js";
import { defaultActions } from "./actions.js";
import { registerScopedActions, resetRegistry } from "./registry.svelte.js";
import { isSidebarCollapsed, setSidebarCollapsed } from "../sidebar.svelte.js";
import { pushModalFrame, resetModalStack } from "@middleman/ui/stores/keyboard/modal-stack";
import type { Action, Context } from "./types.js";

Expand All @@ -14,13 +16,18 @@ const ctx: Context = {
selectedIssue: null,
isDiffView: false,
detailTab: "conversation",
sidebarTargetAvailable: true,
};

const event = (init: Partial<KeyboardEvent>) =>
Object.assign(new KeyboardEvent("keydown", init), {
preventDefault: vi.fn(),
});

afterEach(() => {
setSidebarCollapsed(false);
});

describe("dispatchKeydown — global registry", () => {
beforeEach(() => {
resetRegistry();
Expand Down Expand Up @@ -60,6 +67,21 @@ describe("dispatchKeydown — global registry", () => {
dispatchKeydown(event({ key: "j" }), () => ctx);
expect(handler).not.toHaveBeenCalled();
});

it("reserves Cmd+[ on pages where the sidebar command is hidden", () => {
registerScopedActions("defaults", defaultActions);
setSidebarCollapsed(false);
const e = event({ key: "[", metaKey: true });

dispatchKeydown(e, () => ({
...ctx,
page: "activity",
route: { page: "activity" } as never,
}));

expect(e.preventDefault).toHaveBeenCalled();
expect(isSidebarCollapsed()).toBe(false);
});
});

describe("dispatchKeydown — modal stack", () => {
Expand Down
2 changes: 2 additions & 0 deletions frontend/src/lib/stores/keyboard/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ export interface Context {
selectedIssue: IssueSelection | null;
isDiffView: boolean;
detailTab: DetailTab;
sidebarTargetAvailable: boolean;
}

export interface PreviewBlock {
Expand All @@ -32,6 +33,7 @@ export interface Action {
binding: KeySpec | KeySpec[] | null;
priority: number;
when: (ctx: Context) => boolean;
visible?: (ctx: Context) => boolean;
handler: (ctx: Context) => void | Promise<void>;
preview?: (ctx: Context) => PreviewBlock;
}
Expand Down
5 changes: 5 additions & 0 deletions frontend/src/lib/stores/keyboard/visibility.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
import type { Action, Context } from "./types.js";

export function isActionVisible(action: Action, ctx: Context): boolean {
return (action.visible ?? action.when)(ctx);
}
5 changes: 5 additions & 0 deletions frontend/tests/e2e-full/ci-dropdown.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -277,6 +277,11 @@ test.describe("CI dropdown", () => {
});
await expect(showMore).toBeVisible();
await showMore.evaluate((button: HTMLElement) => button.click());
await expect(
panel.getByRole("button", {
name: /Show fewer passed/i,
}),
).toBeVisible();

const passedRowsAfter = panel.locator(".ci-section-passed .ci-row");
await expect(passedRowsAfter).toHaveCount(12);
Expand Down
31 changes: 19 additions & 12 deletions frontend/tests/e2e-full/diff-view.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -437,13 +437,17 @@ function treeFileItem(pageOrLocator: Page | ReturnType<Page["locator"]>, path: s
return pageOrLocator.locator(`.diff-file-tree [data-item-path="${cssString(path)}"]`);
}

async function clickVisibleTarget(target: Locator): Promise<void> {
await expect(target).toBeVisible({ timeout: 10_000 });
await target.scrollIntoViewIfNeeded();
const box = await target.boundingBox();
expect(box).not.toBeNull();
await target.page().mouse.click(box!.x + box!.width / 2, box!.y + box!.height / 2);
}

async function clickTreeFileItem(pageOrLocator: Page | ReturnType<Page["locator"]>, path: string): Promise<void> {
const item = treeFileItem(pageOrLocator, path);
await expect(item).toBeVisible();
await item.scrollIntoViewIfNeeded();
await item.click({ timeout: 2_000 }).catch(async () => {
await item.evaluate((node) => (node as HTMLElement).click());
});
await clickVisibleTarget(item);
await expect(item).toHaveAttribute("aria-selected", "true");
}

Expand Down Expand Up @@ -596,8 +600,7 @@ async function clickPierreContextExpander(
.filter({ visible: true })
.nth(separatorIndex);
const expander = separator.locator(buttonSelector).filter({ visible: true }).first();
await expect(expander).toBeVisible();
await expander.evaluate((button: HTMLElement) => button.click());
await clickVisibleTarget(expander);
}

async function expectPierreDarkBackgroundMatchesAppSurface(file: ReturnType<Page["locator"]>) {
Expand Down Expand Up @@ -920,7 +923,7 @@ async function selectPierreReviewLine(file: Locator, line: number, side: "left"
].join(",");
const target = file.locator(`.pierre-diff ${selector}`).first();
await expect(target).toBeVisible({ timeout: 10_000 });
await target.locator("[data-middleman-line-comment-button]").click();
await clickVisibleTarget(target.locator("[data-middleman-line-comment-button]"));
}

// --- Functional tests ---
Expand Down Expand Up @@ -2625,14 +2628,16 @@ test.describe("diff view (git-backed)", () => {
const cacheFile = page.locator('[data-file-path="internal/cache.go"]');
await cacheFile.scrollIntoViewIfNeeded();
await selectPierreReviewLine(cacheFile, 1, "right");
await expect(page.getByPlaceholder("Leave a comment")).toBeVisible();
const rightComposer = page.getByPlaceholder("Leave a comment");
await expect(rightComposer).toBeVisible();
await expect(rightComposer).toBeFocused();
await expectPierreDiffFirstVisible(cacheFile, diffAdditionsSelector);
const cacheContentBox = await cacheFile.locator(".file-content").boundingBox();
const composerBox = await cacheFile.locator(".inline-composer").boundingBox();
expect(cacheContentBox).not.toBeNull();
expect(composerBox).not.toBeNull();
expect(composerBox!.x + composerBox!.width).toBeLessThanOrEqual(cacheContentBox!.x + cacheContentBox!.width + 1);
await page.getByPlaceholder("Leave a comment").fill("Right-side cache note");
await rightComposer.fill("Right-side cache note");
await page.getByRole("button", { name: "Add comment" }).click();
await expect(
page.locator(".inline-draft-comment", {
Expand All @@ -2643,8 +2648,10 @@ test.describe("diff view (git-backed)", () => {
const configFile = page.locator('[data-file-path="config.yaml"]');
await configFile.scrollIntoViewIfNeeded();
await selectPierreReviewLine(configFile, 1, "left");
await expect(page.getByPlaceholder("Leave a comment")).toBeVisible();
await page.getByPlaceholder("Leave a comment").fill("Left-side config note");
const leftComposer = page.getByPlaceholder("Leave a comment");
await expect(leftComposer).toBeVisible();
await expect(leftComposer).toBeFocused();
await leftComposer.fill("Left-side config note");
await page.getByRole("button", { name: "Add comment" }).click();
await expect(
page.locator(".inline-draft-comment", {
Expand Down
Loading
Loading