fix(run): preserve pnpm test selection arguments

src/features/github/diff/index.ts

@@ -3,11 +3,14 @@
  * (opened by `openPullRequestFileDiff` / the details panel's commit diffs, context
  * `PULL_REQUEST_DIFF`) get their review threads as inlays, the gutter "+" with single- and
  * multi-line comments, Start Review / Add Single Comment, the review toolbar and the viewed
- * toggle, through the vcs diff viewer provider point. The editor actions also serve review mode's editors.
+ * toggle, through the vcs diff viewer provider point. Also registers the editor actions and
+ * Settings › Version Control › GitHub. The editor actions also serve review mode's editors.
  */
 import { registerDiffViewerProvider } from "@/features/vcs/diffViewerProviders";
 import { registerActions } from "@/platform/actions";
 import { getContext } from "@/platform/context";
+import { lazyComponent } from "@/ui";
+import { registerSettingsPage } from "@/workbench/contributions";
 import { useGhRepository } from "../data/repository";
 import { activeReviewEditor } from "../reviewMode/editor";
 import { clearNewComments } from "./newComments";
@@ -79,6 +82,13 @@ export function activateReviewDiff(): () => void {
         run: () => reviewTarget()?.refresh(),
       },
     ]),
+    registerSettingsPage({
+      id: "vcs.github",
+      title: "GitHub",
+      order: 9,
+      keywords: ["pull request", "viewed", "review"],
+      component: lazyComponent(() => import("./GithubSettingsPage").then((m) => m.GithubSettingsPage)),
+    }),
     useGhRepository.subscribe((s, prev) => {
       const a = s.connection?.coordinates;
       const b = prev.connection?.coordinates;

src/features/github/diff/settings.ts

@@ -46,10 +46,6 @@ function load(): ReviewDiffSettings {
 
 export const useReviewDiffSettings = create<ReviewDiffSettings>(load);
 
-globalThis.addEventListener?.("storage", (event: StorageEvent) => {
-  if (event.key === KEY || event.key === null) useReviewDiffSettings.setState(load(), true);
-});
-
 export function setReviewDiffSettings(patch: Partial<ReviewDiffSettings>) {
   useReviewDiffSettings.setState(patch);
   try {

src/features/github/index.ts

@@ -22,9 +22,6 @@ import {
 import { useGithubShell } from "./shell";
 import { githubHandler, handlerContextKey } from "./toolwindow/handlers";
 import { GithubDialogHost } from "./ui/dialogs";
-import { activateSettingsPages } from "./settingsPages";
-
-export { activateSettingsPages } from "./settingsPages";
 
 const WINDOW = "pullRequests";
 const TIMELINE_KIND = "github.timeline";
@@ -258,7 +255,6 @@ export function activate(): () => void {
   active = true;
   setContext("githubPrTab", "list");
   const disposers = [
-    activateSettingsPages(),
     registerCloneSource({
       id: "github",
       label: "GitHub",

src/features/github/settingsPages.test.tsx

@@ -1,52 +0,0 @@
-import { beforeEach, describe, expect, it } from "vitest";
-import { act, fireEvent, render, screen } from "@testing-library/react";
-import { useSettingsPages } from "@/workbench/contributions";
-import { activateSettingsPages } from "./settingsPages";
-import { GithubSettingsPage } from "./diff/GithubSettingsPage";
-import { setReviewDiffSettings, useReviewDiffSettings } from "./diff/settings";
-
-const KEY = "openidea.github.reviewDiff.v1";
-
-beforeEach(() => {
-  localStorage.removeItem(KEY);
-  window.dispatchEvent(new StorageEvent("storage", { key: KEY }));
-});
-
-describe("GitHub settings pages", () => {
-  it("registers independently of the pull request runtime", () => {
-    const dispose = activateSettingsPages();
-    expect(useSettingsPages.getState().pages.get("vcs.github")).toMatchObject({ title: "GitHub" });
-    expect(useSettingsPages.getState().pages.get("vcs.github")?.component).toBeDefined();
-    dispose();
-    expect(useSettingsPages.getState().pages.has("vcs.github")).toBe(false);
-  });
-
-  it("updates an open page when another window saves or clears review settings", () => {
-    render(<GithubSettingsPage />);
-    const checkbox = screen.getByRole("checkbox", { name: "Automatically mark opened files as viewed" });
-    expect(checkbox).not.toBeChecked();
-    act(() => {
-      localStorage.setItem(KEY, JSON.stringify({ autoMarkViewed: true, threadsView: "all" }));
-      window.dispatchEvent(new StorageEvent("storage", { key: KEY }));
-    });
-    expect(checkbox).toBeChecked();
-    expect(useReviewDiffSettings.getState().threadsView).toBe("all");
-    act(() => {
-      localStorage.clear();
-      window.dispatchEvent(new StorageEvent("storage", { key: null }));
-    });
-    expect(checkbox).not.toBeChecked();
-    fireEvent.click(checkbox);
-    expect(JSON.parse(localStorage.getItem(KEY)!)).toMatchObject({ autoMarkViewed: true });
-  });
-
-  it("ignores unrelated storage changes and recovers from invalid saved settings", () => {
-    setReviewDiffSettings({ autoMarkViewed: true });
-    localStorage.removeItem(KEY);
-    window.dispatchEvent(new StorageEvent("storage", { key: "other" }));
-    expect(useReviewDiffSettings.getState().autoMarkViewed).toBe(true);
-    localStorage.setItem(KEY, "invalid JSON");
-    window.dispatchEvent(new StorageEvent("storage", { key: KEY }));
-    expect(useReviewDiffSettings.getState().autoMarkViewed).toBe(false);
-  });
-});

src/features/github/settingsPages.ts

@@ -1,12 +0,0 @@
-import { lazyComponent } from "@/ui";
-import { registerSettingsPage } from "@/workbench/contributions";
-
-export function activateSettingsPages(): () => void {
-  return registerSettingsPage({
-    id: "vcs.github",
-    title: "GitHub",
-    order: 9,
-    keywords: ["pull request", "viewed", "review"],
-    component: lazyComponent(() => import("./diff/GithubSettingsPage").then((m) => m.GithubSettingsPage)),
-  });
-}

src/features/index.ts

@@ -43,7 +43,7 @@ import { activate as notebook } from "./notebook";
 import { activate as web } from "./web";
 import { activate as analysis } from "./analysis";
 import { activate as services } from "./services";
-import { activate as github, activateSettingsPages as githubPages } from "./github";
+import { activate as github } from "./github";
 import { activate as httpClient } from "./http-client";
 import { activate as database } from "./database";
 import { activate as graphql } from "./graphql";
@@ -166,7 +166,6 @@ export function activateSettingsWindowFeatures(parent: string): () => void {
     ["agents", agentsPages],
     ["vim", vimPages],
     ["vcs", vcsPages],
-    ["github", githubPages],
   ];
   return activateAll("settings pages", pages);
 }

src/features/run/configs.test.ts

@@ -193,13 +193,32 @@ describe("context configuration", () => {
   it("runs a JS test file through the closest test script", () => {
     const c = contextConfig("web/src/cart.test.ts", configs)!;
     expect(c).toMatchObject({
-      args: ["run", "test", "--", "src/cart.test.ts"],
+      args: ["run", "test", "src/cart.test.ts"],
       cwd: "web",
       isTest: true,
       name: "cart.test.ts",
     });
     expect(c.id).toBe("context:npm:web:test:web/src/cart.test.ts");
-    expect(contextConfig("src/a.spec.js", configs)!.args).toEqual(["run", "test", "--", "src/a.spec.js"]);
+    expect(contextConfig("src/a.spec.js", configs)!.args).toEqual(["run", "test", "src/a.spec.js"]);
+  });
+
+  it.each([
+    ["npm", ["--"]],
+    ["pnpm", []],
+    ["/opt/bin/pnpm", []],
+    ["C:\\tools\\pnpm.CMD", []],
+    ["yarn", ["--"]],
+    ["bun", ["--"]],
+  ])("forwards test files through %s", (command, separator) => {
+    const base = cfg({ id: "js:test", kind: "npm", command, args: ["run", "test"], isTest: true });
+    expect(contextConfig("tests/cart.test.ts", [base])!.args).toEqual([
+      "run",
+      "test",
+      ...separator,
+      "tests/cart.test.ts",
+    ]);
+    const explicit = { ...base, args: ["run", "test", "--", "--reporter=verbose"] };
+    expect(contextConfig("tests/cart.test.ts", [explicit])!.args).toEqual([...explicit.args, "tests/cart.test.ts"]);
   });
 
   it("narrows rspec and rake test runs to the file", () => {

src/features/run/context.ts

@@ -11,6 +11,7 @@ import { goBuildType } from "./goTypes";
 import { isWindows } from "./jvmTypes";
 import { typeFor } from "./types";
 import { runTemplate } from "./runTemplates";
+import { appendScriptArgs } from "./scriptArgs";
 
 export type FileRole = "test" | "main" | "script" | null;
 
@@ -216,8 +217,7 @@ export function testConfigFor(relPath: string, configs: RunConfig[], base?: RunC
   switch (base.kind) {
     case "npm":
     case "bun": {
-      // `npm run test -- file` works for npm, pnpm, yarn and bun.
-      const args = base.args.includes("--") ? [...base.args, file] : [...base.args, "--", file];
+      const args = appendScriptArgs(base.command, base.args, [file]);
       return derived(base, relPath, { args });
     }
     case "deno":

src/features/run/scriptArgs.ts

@@ -0,0 +1,7 @@
+import { commandName } from "./commandName";
+
+/** pnpm forwards `--` to the script, where it ends the test runner's option parsing. */
+export function appendScriptArgs(command: string, args: string[], extra: string[]): string[] {
+  const separator = !args.includes("--") && ["npm", "yarn", "bun"].includes(commandName(command)) ? ["--"] : [];
+  return [...args, ...separator, ...extra];
+}

src/features/run/targets.test.ts

@@ -170,7 +170,6 @@ describe("targetConfig", () => {
     expect(first("describe('m', () => {\n  it('adds', () => {});\n});", "src/a.test.ts").args).toEqual([
       "run",
       "test",
-      "--",
       "src/a.test.ts",
       "-t",
       "^m adds$",

src/features/run/testing/rerun.test.ts

@@ -86,15 +86,23 @@ describe("rerunFailedConfig", () => {
     expect(c.args).toEqual(["test", "--", "--nocapture", "old", "a", "--exact"]);
   });
 
-  it("jest/vitest via npm: files + -t pattern", () => {
-    const c = rerunFailedConfig(cfg("npm", "pnpm", ["run", "test"]), [
+  it.each([
+    ["npm", ["--"]],
+    ["pnpm", []],
+    ["/opt/bin/pnpm", []],
+    ["C:\\tools\\pnpm.CMD", []],
+    ["yarn", ["--"]],
+    ["bun", ["--"]],
+    ["npx", []],
+  ])("jest/vitest via %s: files + -t pattern", (command, separator) => {
+    const c = rerunFailedConfig(cfg("npm", command, ["run", "test"]), [
       ["src/a.test.ts", "math", "adds (1+1)"],
       ["src/b.test.ts"],
     ])!;
     expect(c.args).toEqual([
       "run",
       "test",
-      "--",
+      ...separator,
       "src/a.test.ts",
       "src/b.test.ts",
       "-t",
@@ -102,6 +110,14 @@ describe("rerunFailedConfig", () => {
     ]);
   });
 
+  it.each([
+    ["run", "test", "-t", "old"],
+    ["run", "test", "--", "--testNamePattern=old"],
+  ])("replaces an existing pnpm test filter in %j", (...args) => {
+    const c = rerunFailedConfig(cfg("npm", "pnpm", args), [["src/a.test.ts", "new"]])!;
+    expect(c.args).toEqual(["run", "test", ...(args.includes("--") ? ["--"] : []), "src/a.test.ts", "-t", "^(?:new)$"]);
+  });
+
   it("pytest: node ids replace explicit paths", () => {
     const c = rerunFailedConfig(cfg("pytest", ".venv/bin/pytest", ["-q", "tests/", "-k", "slow"]), [
       ["tests/test_x.py", "TestC", "test_y[1]"],

src/features/run/testing/rerun.ts

@@ -5,6 +5,7 @@
 import { phpunitArgs, phpunitFilter } from "./phpunit";
 import type { RunConfig } from "@/platform/ipc";
 import { formatArgs } from "../configs";
+import { appendScriptArgs } from "../scriptArgs";
 import { djangoTestsOf } from "../djangoTypes";
 import { goTestParts, withoutGoSelection } from "../goArguments";
 import { frameworkFor } from "./index";
@@ -71,11 +72,10 @@ function jsArgs(config: RunConfig, paths: string[][]): string[] | null {
   const names = unique(paths.filter((p) => p.length > 1).map((p) => p.slice(1).join(" ")));
   if (!files.length) return null;
   const { before, after } = splitDashDash(base);
-  const rest = dropFlag(after ?? [], ["-t", "--testNamePattern"]);
+  const flags = ["-t", "--testNamePattern"];
+  const args = after === null ? dropFlag(before, flags) : [...before, "--", ...dropFlag(after, flags)];
   const filter = names.length ? ["-t", `^(?:${names.map(escapeRegExp).join("|")})$`] : [];
-  // npm/pnpm/yarn/bun pass everything after `--` to the script; deno/npx take them directly.
-  const needsDashDash = after !== null || /(^|\/)(npm|pnpm|yarn|bun)$/.test(config.command);
-  return needsDashDash ? [...before, "--", ...rest, ...files, ...filter] : [...before, ...files, ...filter];
+  return appendScriptArgs(config.command, args, [...files, ...filter]);
 }
 
 function pytestArgs(base: string[], paths: string[][]): string[] | null {