fix(github): expose review settings in the native settings window

src/features/github/diff/index.ts

@@ -3,14 +3,11 @@
  * (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. Also registers the editor actions and
- * Settings › Version Control › GitHub. The editor actions also serve review mode's editors.
+ * toggle, through the vcs diff viewer provider point. 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";
@@ -82,13 +79,6 @@ 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,6 +46,10 @@ 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,6 +22,9 @@ 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";
@@ -255,6 +258,7 @@ export function activate(): () => void {
   active = true;
   setContext("githubPrTab", "list");
   const disposers = [
+    activateSettingsPages(),
     registerCloneSource({
       id: "github",
       label: "GitHub",

src/features/github/settingsPages.test.tsx

@@ -0,0 +1,52 @@
+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

@@ -0,0 +1,12 @@
+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 } from "./github";
+import { activate as github, activateSettingsPages as githubPages } from "./github";
 import { activate as httpClient } from "./http-client";
 import { activate as database } from "./database";
 import { activate as graphql } from "./graphql";
@@ -166,6 +166,7 @@ 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,32 +193,13 @@ 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"]);
-  });
-
-  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"]);
+    expect(contextConfig("src/a.spec.js", configs)!.args).toEqual(["run", "test", "--", "src/a.spec.js"]);
   });
 
   it("narrows rspec and rake test runs to the file", () => {

src/features/run/context.ts

@@ -11,7 +11,6 @@ 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;
 
@@ -217,7 +216,8 @@ export function testConfigFor(relPath: string, configs: RunConfig[], base?: RunC
   switch (base.kind) {
     case "npm":
     case "bun": {
-      const args = appendScriptArgs(base.command, base.args, [file]);
+      // `npm run test -- file` works for npm, pnpm, yarn and bun.
+      const args = base.args.includes("--") ? [...base.args, file] : [...base.args, "--", file];
       return derived(base, relPath, { args });
     }
     case "deno":

src/features/run/scriptArgs.ts

@@ -1,7 +0,0 @@
-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,6 +170,7 @@ 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,23 +86,15 @@ describe("rerunFailedConfig", () => {
     expect(c.args).toEqual(["test", "--", "--nocapture", "old", "a", "--exact"]);
   });
 
-  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"]), [
+  it("jest/vitest via npm: files + -t pattern", () => {
+    const c = rerunFailedConfig(cfg("npm", "pnpm", ["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",
@@ -110,14 +102,6 @@ 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,7 +5,6 @@
 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";
@@ -72,10 +71,11 @@ 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 flags = ["-t", "--testNamePattern"];
-  const args = after === null ? dropFlag(before, flags) : [...before, "--", ...dropFlag(after, flags)];
+  const rest = dropFlag(after ?? [], ["-t", "--testNamePattern"]);
   const filter = names.length ? ["-t", `^(?:${names.map(escapeRegExp).join("|")})$`] : [];
-  return appendScriptArgs(config.command, args, [...files, ...filter]);
+  // 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];
 }
 
 function pytestArgs(base: string[], paths: string[][]): string[] | null {