fix(web): scope pull request errors to their environment (#6490)

Co-authored-by: Julius Marminge <julius0216@outlook.com>

Author: Adamulek123

apps/web/src/components/pullRequest/PullRequestListFilters.test.tsx

@@ -3,7 +3,7 @@ import { CircleIcon } from "lucide-react";
 import { Children, isValidElement, type ReactElement, type ReactNode } from "react";
 import { describe, expect, it, vi } from "vite-plus/test";
 
-import { PullRequestFiltersMenu } from "./PullRequestListFilters";
+import { PullRequestFiltersMenu, pullRequestProjectKey } from "./PullRequestListFilters";
 
 function findValueChange(
   node: ReactNode,
@@ -129,7 +129,7 @@ describe("pull request filters menu", () => {
     const radioGroup = findValueChange(view);
     expect(radioGroup).toBeDefined();
 
-    radioGroup?.props.onValueChange(`${environmentId} ${projectId}`);
+    radioGroup?.props.onValueChange(pullRequestProjectKey({ id: projectId, environmentId }));
     expect(onProject).not.toHaveBeenCalled();
 
     radioGroup?.props.onValueChange("all");
@@ -159,7 +159,23 @@ describe("pull request filters menu", () => {
     const radioGroup = findValueChange(view);
     expect(radioGroup).toBeDefined();
 
-    radioGroup?.props.onValueChange(`env-2 ${projectId}`);
+    radioGroup?.props.onValueChange(
+      pullRequestProjectKey({ id: projectId, environmentId: "env-2" as EnvironmentId }),
+    );
     expect(onProject).toHaveBeenCalledWith(projectId, "env-2");
   });
+
+  it("does not collide when environment and project ids contain spaces", () => {
+    expect(
+      pullRequestProjectKey({
+        environmentId: "a b" as EnvironmentId,
+        id: "c" as ProjectId,
+      }),
+    ).not.toBe(
+      pullRequestProjectKey({
+        environmentId: "a" as EnvironmentId,
+        id: "b c" as ProjectId,
+      }),
+    );
+  });
 });

apps/web/src/components/pullRequest/PullRequestListFilters.tsx

@@ -122,10 +122,10 @@ const UNFILTERED_VALUE = "all";
  * A project's own radio value, carrying the server along with the id: the id alone is only
  * unique within its own server, so two rows sharing one would otherwise both read as checked.
  */
-const projectMenuValue = (project: {
+export const pullRequestProjectKey = (project: {
   readonly id: ProjectId;
   readonly environmentId: EnvironmentId;
-}) => `${project.environmentId} ${project.id}`;
+}) => JSON.stringify([project.environmentId, project.id]);
 
 const DRAFT_OPTIONS = [
   { value: UNFILTERED_VALUE, label: "All", Icon: LayersIcon },
@@ -247,7 +247,7 @@ export function PullRequestFiltersMenu({
    * the reader is already choosing between projects, rather than as a count above the list
    * that says something is missing without saying which.
    */
-  unavailable: ReadonlyMap<ProjectId, string>;
+  unavailable: ReadonlyMap<string, string>;
   /** The environment comes with the project id, since picking a row picks a specific server's copy of it. */
   onProject: (projectId: ProjectId | undefined, environmentId: EnvironmentId | undefined) => void;
 }) {
@@ -350,7 +350,7 @@ export function PullRequestFiltersMenu({
           value={
             projectId === undefined || projectEnvironmentId === undefined
               ? ALL_PROJECTS_VALUE
-              : projectMenuValue({ id: projectId, environmentId: projectEnvironmentId })
+              : pullRequestProjectKey({ id: projectId, environmentId: projectEnvironmentId })
           }
           onValueChange={(next) => {
             if (next === ALL_PROJECTS_VALUE) {
@@ -359,7 +359,7 @@ export function PullRequestFiltersMenu({
             }
             // The value carries both halves, since the id alone cannot tell two servers' rows
             // apart once they share one.
-            const project = projects.find((candidate) => projectMenuValue(candidate) === next);
+            const project = projects.find((candidate) => pullRequestProjectKey(candidate) === next);
             if (
               project !== undefined &&
               (project.id !== projectId || project.environmentId !== projectEnvironmentId)
@@ -379,14 +379,16 @@ export function PullRequestFiltersMenu({
               as a broken menu rather than as a workspace with three unreadable repositories. */}
           {projects
             .toSorted(
-              (left, right) => Number(unavailable.has(left.id)) - Number(unavailable.has(right.id)),
+              (left, right) =>
+                Number(unavailable.has(pullRequestProjectKey(left))) -
+                Number(unavailable.has(pullRequestProjectKey(right))),
             )
             .map((project) => {
-              const reason = unavailable.get(project.id);
+              const reason = unavailable.get(pullRequestProjectKey(project));
               return (
                 <MenuRadioItem
-                  key={projectMenuValue(project)}
-                  value={projectMenuValue(project)}
+                  key={pullRequestProjectKey(project)}
+                  value={pullRequestProjectKey(project)}
                   disabled={reason !== undefined}
                   title={reason}
                 >

apps/web/src/components/pullRequest/pullRequestList.logic.test.ts

@@ -1,4 +1,4 @@
-import type { EnvironmentId, PullRequestListEntry } from "@t3tools/contracts";
+import type { EnvironmentId, ProjectId, PullRequestListEntry } from "@t3tools/contracts";
 import { describe, expect, it } from "vite-plus/test";
 
 import {
@@ -710,6 +710,20 @@ describe("merging the environments' own listings", () => {
     );
   });
 
+  it("keeps project errors scoped to the environment that reported them", () => {
+    const error = {
+      projectId: "project-1" as ProjectId,
+      projectTitle: "Web",
+      message: "Not signed in",
+    } as const;
+    const merged = mergePullRequestLists([
+      [ENV_1, answer({ errors: [error] })],
+      [ENV_2, answer()],
+    ]);
+
+    expect(merged?.errors).toEqual([{ ...error, environmentId: ENV_1 }]);
+  });
+
   it("folds a host reached from two environments into one switcher row", () => {
     const merged = mergePullRequestLists([
       [ENV_1, answer()],

apps/web/src/components/pullRequest/pullRequestList.logic.ts

@@ -3,6 +3,7 @@ import * as Schema from "effect/Schema";
 import {
   EnvironmentId,
   PullRequestListEntry,
+  PullRequestListProjectError,
   PullRequestListResult,
   resolvePullRequestAuthorFilter,
 } from "@t3tools/contracts";
@@ -27,6 +28,10 @@ export interface EnvironmentPullRequestStat extends PullRequestDiffStat {
   readonly environmentId: EnvironmentId;
 }
 
+export interface EnvironmentPullRequestError extends PullRequestListProjectError {
+  readonly environmentId: EnvironmentId;
+}
+
 export type PullRequestGroupKey = "reviewRequested" | "authored" | "others";
 
 export interface PullRequestGroup<Entry extends PullRequestListEntry = PullRequestListEntry> {
@@ -452,7 +457,7 @@ export interface MergedPullRequestList {
   readonly viewers: PullRequestViewers;
   readonly providers: PullRequestListResult["providers"];
   readonly entries: ReadonlyArray<EnvironmentPullRequestEntry>;
-  readonly errors: PullRequestListResult["errors"];
+  readonly errors: ReadonlyArray<EnvironmentPullRequestError>;
   readonly truncated: boolean;
   readonly nextCursors: Readonly<Record<string, PullRequestListCursors>>;
   /**
@@ -476,7 +481,7 @@ export function mergePullRequestLists(
   const truncatedEnvironments: string[] = [];
   const providers = new Map<string, PullRequestListResult["providers"][number]>();
   const entries: EnvironmentPullRequestEntry[] = [];
-  const errors: Array<PullRequestListResult["errors"][number]> = [];
+  const errors: EnvironmentPullRequestError[] = [];
   const nextCursors: Record<string, PullRequestListCursors> = {};
   let truncated = false;
   for (const [environmentId, answer] of answers) {
@@ -498,7 +503,7 @@ export function mergePullRequestLists(
       );
     }
     entries.push(...answer.entries.map((entry) => ({ ...entry, environmentId })));
-    errors.push(...answer.errors);
+    errors.push(...answer.errors.map((error) => ({ ...error, environmentId })));
     truncated ||= answer.truncated;
     if (answer.truncated) truncatedEnvironments.push(environmentId);
     if (Object.keys(answer.nextCursors).length > 0) {
@@ -559,12 +564,18 @@ const EnvironmentPullRequestEntrySchema = Schema.Struct({
   environmentId: EnvironmentId,
 });
 
+const EnvironmentPullRequestErrorSchema = Schema.Struct({
+  ...PullRequestListProjectError.fields,
+  environmentId: EnvironmentId,
+});
+
 const decodeSnapshot = Schema.decodeUnknownOption(
   Schema.Struct({
     scope: Schema.String,
     data: Schema.Struct({
       ...PullRequestListResult.fields,
       entries: Schema.Array(EnvironmentPullRequestEntrySchema),
+      errors: Schema.Array(EnvironmentPullRequestErrorSchema),
       // Per environment here, unlike the wire shape, which is per repository within one.
       nextCursors: Schema.Record(Schema.String, PullRequestListResult.fields.nextCursors),
       truncatedEnvironments: Schema.Array(Schema.String),

apps/web/src/routes/_chat.pull-requests.tsx

@@ -60,6 +60,7 @@ import {
   PullRequestFiltersMenu,
   PullRequestSearchInput,
   pullRequestHostLabel,
+  pullRequestProjectKey,
   type PullRequestExpectedHost,
   type PullRequestFilterOption,
 } from "../components/pullRequest/PullRequestListFilters";
@@ -1250,7 +1251,16 @@ function PullRequestsRouteView() {
 
   /** Reported per project rather than as a count, so the reader can see which one it was. */
   const unavailableProjects = useMemo(
-    () => new Map(listErrors.map((error) => [error.projectId, error.message] as const)),
+    () =>
+      new Map(
+        listErrors.map(
+          (error) =>
+            [
+              pullRequestProjectKey({ id: error.projectId, environmentId: error.environmentId }),
+              error.message,
+            ] as const,
+        ),
+      ),
     [listErrors],
   );