From aae50a2fda2469b479d57dce9063ac3bde95fcd0 Mon Sep 17 00:00:00 2001
From: Thomas Scholtes <geigerzaehler@axiom.fm>
Date: Fri, 14 Jul 2023 18:26:15 +0200
Subject: [PATCH] Use structured routing for patch views

Signed-off-by: Thomas Scholtes <geigerzaehler@axiom.fm>
---
 src/views/projects/Cob/Revision.svelte |  12 +-
 src/views/projects/Patch.svelte        | 228 ++++++++++----------
 src/views/projects/View.svelte         |   3 +-
 src/views/projects/router.ts           | 288 ++++++++++++++++++-------
 tests/unit/router.test.ts              |  51 ++++-
 5 files changed, 374 insertions(+), 208 deletions(-)

diff --git a/src/views/projects/Cob/Revision.svelte b/src/views/projects/Cob/Revision.svelte
index d6db1d787..a89cb4c21 100644
--- a/src/views/projects/Cob/Revision.svelte
+++ b/src/views/projects/Cob/Revision.svelte
@@ -215,7 +215,11 @@
               project: projectId,
               seed: baseUrl,
               patch: patchId,
-              search: `diff=${previousRevOid}..${revisionOid}`,
+              view: {
+                name: "diff",
+                fromCommit: previousRevOid,
+                toCommit: revisionOid,
+              },
             }}>
             <Icon name="diff" />
           </Link>
@@ -237,7 +241,11 @@
                     project: projectId,
                     seed: baseUrl,
                     patch: patchId,
-                    search: `diff=${item}..${revisionOid}`,
+                    view: {
+                      name: "diff",
+                      fromCommit: item,
+                      toCommit: revisionOid,
+                    },
                   }}>
                   {#if item === projectHead}
                     <DropdownItem selected={false} size="small">
diff --git a/src/views/projects/Patch.svelte b/src/views/projects/Patch.svelte
index 4842afa61..b8362f4b3 100644
--- a/src/views/projects/Patch.svelte
+++ b/src/views/projects/Patch.svelte
@@ -30,6 +30,8 @@
 <script lang="ts">
   import type { BaseUrl, Patch } from "@httpd-client";
   import type { Variant } from "@app/components/Badge.svelte";
+  import { type Route } from "@app/lib/router";
+  import type { PatchView } from "./router";
 
   import * as utils from "@app/lib/utils";
   import { capitalize } from "lodash";
@@ -43,7 +45,6 @@
   import CommitTeaser from "@app/views/projects/Commit/CommitTeaser.svelte";
   import Dropdown from "@app/components/Dropdown.svelte";
   import DropdownItem from "@app/components/Dropdown/DropdownItem.svelte";
-  import ErrorMessage from "@app/components/ErrorMessage.svelte";
   import Floating, { closeFocused } from "@app/components/Floating.svelte";
   import Icon from "@app/components/Icon.svelte";
   import Link from "@app/components/Link.svelte";
@@ -53,18 +54,12 @@
   import SquareButton from "@app/components/SquareButton.svelte";
   import TagInput from "@app/views/projects/Cob/TagInput.svelte";
 
-  export let search: string | undefined = undefined;
-  export let projectId: string;
   export let baseUrl: BaseUrl;
   export let patch: Patch;
+  export let projectId: string;
   export let projectHead: string;
   export let projectDefaultBranch: string;
-  export let revision: string | undefined = undefined;
-
-  $: searchParams = new URLSearchParams(search || "");
-  $: currentTab =
-    (searchParams.get("tab") as "activity" | "commits" | "files") || "activity";
-  $: diff = searchParams.get("diff") || undefined;
+  export let view: PatchView;
 
   const api = new HttpdClient(baseUrl);
 
@@ -77,7 +72,7 @@
         patch.id,
         {
           type: "thread",
-          revision: currentRevision.id,
+          revision: revisionId,
           action: {
             type: "comment",
             body: reply.body,
@@ -109,9 +104,16 @@
       if (add.length === 0 && remove.length === 0) {
         return;
       }
+
+      let revision;
+      if (view.view.name === "diff") {
+        revision = patch.revisions[patch.revisions.length - 1].id;
+      } else {
+        revision = view.view.revision;
+      }
       await api.project.updatePatch(
         projectId,
-        currentRevision.id,
+        revision,
         { type: "tag", add, remove },
         $httpdStore.session.id,
       );
@@ -123,11 +125,34 @@
     $httpdStore.state === "authenticated" && utils.isLocal(baseUrl.hostname)
       ? "edit"
       : "view";
-  const options = ["activity", "commits", "files"].map(o => ({
-    value: o,
-    title: capitalize(o),
-    disabled: false,
-  }));
+
+  let tabs: Record<string, Route>;
+  $: {
+    const baseRoute = {
+      resource: "project.patch",
+      project: projectId,
+      seed: baseUrl,
+      patch: patch.id,
+    } as const;
+    // For cleaner URLs, we omit the the revision part when we link to the
+    // latest revision.
+    const latestRevisionId = patch.revisions[patch.revisions.length - 1].id;
+    const revision = latestRevisionId === revisionId ? undefined : revisionId;
+    tabs = {
+      activity: {
+        ...baseRoute,
+        view: { name: "activity" },
+      },
+      commits: {
+        ...baseRoute,
+        view: { name: "commits", revision },
+      },
+      files: {
+        ...baseRoute,
+        view: { name: "files", revision },
+      },
+    };
+  }
 
   function computeReviews(patch: Patch) {
     const patchReviews: Record<string, { latest: boolean; review: Review }> =
@@ -143,10 +168,14 @@
     return patchReviews;
   }
 
+  let revisionId: string;
+  $: if (view.view.name === "diff") {
+    revisionId = patch.revisions[patch.revisions.length - 1].id;
+  } else {
+    revisionId = view.view.revision;
+  }
+
   $: patchReviews = computeReviews(patch);
-  $: currentRevision =
-    (revision && patch.revisions.find(r => r.id === revision)) ||
-    patch.revisions[patch.revisions.length - 1];
   $: timelineTuple = patch.revisions.map<
     [
       {
@@ -307,7 +336,7 @@
             rawPath={utils.getRawBasePath(
               projectId,
               baseUrl,
-              currentRevision.oid,
+              patch.revisions[0].id,
             )} />
         {:else}
           <span class="txt-missing">No description available</span>
@@ -323,59 +352,44 @@
 
     <div class="tab-line">
       <div style="display: flex; gap: 0.5rem;">
-        {#each options as option}
-          {#if !option.disabled}
-            <Link
-              route={{
-                resource: "project.patch",
-                project: projectId,
-                seed: baseUrl,
-                patch: patch.id,
-                revision,
-                search: `tab=${option.value}`,
-              }}>
-              <SquareButton
-                size="small"
-                clickable={option.disabled}
-                active={option.value === currentTab && !diff}
-                disabled={option.disabled}>
-                {option.title}
-              </SquareButton>
-            </Link>
-          {:else}
-            <SquareButton
-              size="small"
-              clickable={option.disabled}
-              active={option.value === currentTab}
-              disabled={option.disabled}>
-              {option.title}
+        {#each Object.entries(tabs) as [name, route]}
+          <Link {route}>
+            <SquareButton size="small" active={name === view.view.name}>
+              {capitalize(name)}
             </SquareButton>
-          {/if}
+          </Link>
         {/each}
-        {#if diff}
+        {#if view.view.name === "diff"}
           <Link
             route={{
               resource: "project.patch",
               project: projectId,
               seed: baseUrl,
               patch: patch.id,
-              search: `diff=${diff}`,
+              view: {
+                name: "diff",
+                fromCommit: view.view.fromCommit,
+                toCommit: view.view.toCommit,
+              },
             }}>
             <SquareButton size="small" active={true}>
-              Diff {diff.substr(0, 6)}..{diff.split("..")[1].substr(0, 6)}
+              Diff {view.view.fromCommit.substring(
+                0,
+                6,
+              )}..{view.view.toCommit.substring(0, 6)}
             </SquareButton>
           </Link>
         {/if}
       </div>
 
-      {#if currentTab !== "activity"}
+      {#if view.view.name === "commits" || view.view.name === "files"}
         <Floating disabled={patch.revisions.length === 1}>
           <svelte:fragment slot="toggle">
             <SquareButton
               size="small"
               clickable={patch.revisions.length > 1}
               disabled={patch.revisions.length === 1}>
-              Revision {utils.formatObjectId(currentRevision.id)}
+              Revision {utils.formatObjectId(view.view.revision)}
             </SquareButton>
           </svelte:fragment>
           <svelte:fragment slot="modal">
@@ -388,11 +402,13 @@
                     project: projectId,
                     seed: baseUrl,
                     patch: patch.id,
-                    revision: item.id,
-                    search: `tab=${currentTab}`,
+                    view: {
+                      name: view.view.name,
+                      revision: item.id,
+                    },
                   }}>
                   <DropdownItem
-                    selected={item.id === currentRevision.id}
+                    selected={item.id === view.view.revision}
                     size="tiny">
                     Revision {utils.formatObjectId(item.id)}
                   </DropdownItem>
@@ -403,66 +419,56 @@
         </Floating>
       {/if}
     </div>
-    {#if currentTab === "activity"}
-      {#if diff}
-        {@const [base, revision] = diff.split("..")}
-        {#await api.project.getDiff(projectId, base, revision) then { diff }}
-          <div style:margin-top="1rem">
-            <Changeset {projectId} {baseUrl} {revision} {diff} />
-          </div>
-        {:catch e}
-          <ErrorMessage
-            message="Not able to load revision diff."
-            stackTrace={e} />
-        {/await}
+    {#if view.view.name === "diff"}
+      <div style:margin-top="1rem">
+        <Changeset
+          {projectId}
+          {baseUrl}
+          revision={view.view.toCommit}
+          diff={view.view.diff} />
+      </div>
+    {:else if view.view.name === "activity"}
+      {#each timelineTuple as [revision, timelines], index}
+        {@const previousRevision =
+          index > 0 ? patch.revisions[index - 1] : undefined}
+        <RevisionComponent
+          {baseUrl}
+          {projectId}
+          {timelines}
+          {projectDefaultBranch}
+          {projectHead}
+          {...revision}
+          first={index === 0}
+          on:reply={createReply}
+          patchId={patch.id}
+          expanded={index === patch.revisions.length - 1}
+          previousRevId={previousRevision?.id}
+          previousRevOid={previousRevision?.oid} />
       {:else}
-        {#each timelineTuple as [revision, timelines], index}
-          {@const previousRevision =
-            index > 0 ? patch.revisions[index - 1] : undefined}
-          <RevisionComponent
-            {baseUrl}
-            {projectId}
-            {timelines}
-            {projectDefaultBranch}
-            {projectHead}
-            {...revision}
-            first={index === 0}
-            on:reply={createReply}
-            patchId={patch.id}
-            expanded={index === patch.revisions.length - 1}
-            previousRevId={previousRevision?.id}
-            previousRevOid={previousRevision?.oid} />
-        {:else}
-          <Placeholder emoji="🍂">
-            <div slot="title">No activity</div>
-            <div slot="body">No activity on this patch yet</div>
-          </Placeholder>
+        <Placeholder emoji="🍂">
+          <div slot="title">No activity</div>
+          <div slot="body">No activity on this patch yet</div>
+        </Placeholder>
+      {/each}
+    {:else if view.view.name === "commits"}
+      <div class="commit-list">
+        {#each view.view.commits as commit}
+          <CommitTeaser {projectId} {baseUrl} {commit} />
         {/each}
-      {/if}
-    {:else if currentTab === "commits"}
-      {#await api.project.getDiff(projectId, currentRevision.base, currentRevision.oid) then diff}
-        <div class="commit-list">
-          {#each diff.commits as commit}
-            <CommitTeaser {projectId} {baseUrl} {commit} />
-          {/each}
-        </div>
-      {:catch e}
-        <ErrorMessage message="Not able to load commits." stackTrace={e} />
-      {/await}
-    {:else if currentTab === "files"}
-      {#await api.project.getDiff(projectId, currentRevision.base, currentRevision.oid) then diff}
-        <div style:margin-top="1rem">
-          <Changeset
-            {projectId}
-            {baseUrl}
-            revision={currentRevision.oid}
-            diff={diff.diff} />
-        </div>
-      {:catch e}
-        <ErrorMessage message="Not able to load files diff." stackTrace={e} />
-      {/await}
+      </div>
+    {:else if view.view.name === "files"}
+      <div style:margin-top="1rem">
+        <Changeset
+          {projectId}
+          {baseUrl}
+          revision={view.view.revision}
+          diff={view.view.diff} />
+      </div>
+    {:else}
+      {utils.unreachable(view.view.name)}
     {/if}
   </div>
+
   <div class="metadata">
     <div>
       <div class="metadata-section-header">Reviews</div>
diff --git a/src/views/projects/View.svelte b/src/views/projects/View.svelte
index 8c9a4cca8..4b50b232c 100644
--- a/src/views/projects/View.svelte
+++ b/src/views/projects/View.svelte
@@ -126,8 +126,7 @@
       projectId={id}
       projectDefaultBranch={project.defaultBranch}
       projectHead={project.head}
-      revision={view.revision}
-      search={view.search} />
+      {view} />
   {:else}
     {unreachable(view)}
   {/if}
diff --git a/src/views/projects/router.ts b/src/views/projects/router.ts
index 4d96146af..0b6ba0f2f 100644
--- a/src/views/projects/router.ts
+++ b/src/views/projects/router.ts
@@ -4,6 +4,7 @@ import type {
   Blob,
   Commit,
   CommitHeader,
+  Diff,
   Issue,
   Patch,
   Project,
@@ -40,20 +41,8 @@ export type ProjectRoute =
       project: string;
       issue: string;
     }
-  | {
-      resource: "project.patches";
-      seed: BaseUrl;
-      project: string;
-      search?: string;
-    }
-  | {
-      resource: "project.patch";
-      seed: BaseUrl;
-      project: string;
-      patch: string;
-      revision?: string;
-      search?: string;
-    };
+  | ProjectPatchesRoute
+  | ProjectPatchRoute;
 
 interface ProjectTreeRoute {
   resource: "project.tree";
@@ -73,6 +62,33 @@ interface ProjectHistoryRoute {
   revision?: string;
 }
 
+interface ProjectPatchRoute {
+  resource: "project.patch";
+  seed: BaseUrl;
+  project: string;
+  patch: string;
+  view?:
+    | {
+        name: "activity";
+      }
+    | {
+        name: "commits" | "files";
+        revision?: string;
+      }
+    | {
+        name: "diff";
+        fromCommit: string;
+        toCommit: string;
+      };
+}
+
+interface ProjectPatchesRoute {
+  resource: "project.patches";
+  seed: BaseUrl;
+  project: string;
+  search?: string;
+}
+
 export interface ProjectLoadedRoute {
   resource: "projects";
   params: ProjectLoadedParams;
@@ -119,17 +135,34 @@ export type ProjectLoadedView =
   | { resource: "issues"; search: string }
   | { resource: "newIssue" }
   | { resource: "patches"; search: string }
-  | {
-      resource: "patch";
-      patch: Patch;
-      revision?: string;
-      search: string;
-    };
+  | PatchView;
 
 export type BlobResult =
   | { ok: true; blob: Blob; highlighted: Syntax.Root | undefined }
   | { ok: false; error: { message: string; path: string } };
 
+export interface PatchView {
+  resource: "patch";
+  patch: Patch;
+  view:
+    | {
+        name: "activity";
+        revision: string;
+      }
+    | {
+        name: "commits" | "files";
+        revision: string;
+        diff: Diff;
+        commits: CommitHeader[];
+      }
+    | {
+        name: "diff";
+        diff: Diff;
+        fromCommit: string;
+        toCommit: string;
+      };
+}
+
 // Check whether the input is a SHA1 commit.
 function isOid(input: string): boolean {
   return /^[a-fA-F0-9]{40}$/.test(input);
@@ -232,40 +265,7 @@ export async function loadProjectRoute(
         };
       }
     } else if (route.resource === "project.patch") {
-      try {
-        const projectPromise = api.project.getById(route.project);
-        const patchPromise = api.project.getPatchById(
-          route.project,
-          route.patch,
-        );
-        const [project, patch] = await Promise.all([
-          projectPromise,
-          patchPromise,
-        ]);
-        return {
-          resource: "projects",
-          params: {
-            id: route.project,
-            baseUrl: route.seed,
-            project,
-            view: {
-              resource: "patch",
-              patch,
-              revision: route.revision,
-              search: route.search || "",
-            },
-          },
-        };
-      } catch (error: any) {
-        return {
-          resource: "loadError",
-          params: {
-            title: route.patch,
-            errorMessage: "Not able to load this patch.",
-            stackTrace: error.stack,
-          },
-        };
-      }
+      return loadPatchView(route);
     } else if (route.resource === "project.issues") {
       const project = await api.project.getById(route.project);
       return {
@@ -473,6 +473,73 @@ async function loadHistoryView(
   };
 }
 
+async function loadPatchView(
+  route: ProjectPatchRoute,
+): Promise<ProjectLoadedRoute> {
+  const api = new HttpdClient(route.seed);
+  const [project, patch] = await Promise.all([
+    api.project.getById(route.project),
+    api.project.getPatchById(route.project, route.patch),
+  ]);
+  const latestRevision = patch.revisions[patch.revisions.length - 1];
+
+  let view: PatchView["view"];
+  switch (route.view?.name) {
+    case "activity":
+    case undefined: {
+      view = { name: "activity", revision: latestRevision.id };
+      break;
+    }
+    case "commits":
+    case "files": {
+      const revisionId = route.view.revision;
+      const revision =
+        patch.revisions.find(r => r.id === revisionId) || latestRevision;
+      if (!revision) {
+        throw new Error(
+          `revision ${revisionId} of patch ${route.patch} not found`,
+        );
+      }
+      const { diff, commits } = await api.project.getDiff(
+        route.project,
+        revision.base,
+        revision.oid,
+      );
+      view = {
+        name: route.view?.name,
+        revision: revision.id,
+        diff,
+        commits,
+      };
+      break;
+    }
+    case "diff": {
+      const { fromCommit, toCommit } = route.view;
+      const { diff } = await api.project.getDiff(
+        route.project,
+        fromCommit,
+        toCommit,
+      );
+
+      view = { name: "diff", fromCommit, toCommit, diff };
+      break;
+    }
+  }
+  return {
+    resource: "projects",
+    params: {
+      id: route.project,
+      baseUrl: route.seed,
+      project,
+      view: {
+        resource: "patch",
+        patch,
+        view,
+      },
+    },
+  };
+}
+
 async function getPeerBranches(
   api: HttpdClient,
   project: string,
@@ -580,30 +647,63 @@ export function resolveProjectRoute(
       };
     }
   } else if (content === "patches") {
-    const patch = segments.shift();
-    const revision = segments.shift();
-    if (patch) {
-      return {
-        resource: "project.patch",
-        seed,
-        project,
-        patch,
-        revision,
-        search: sanitizeQueryString(urlSearch),
-      };
-    } else {
-      return {
-        resource: "project.patches",
-        seed,
-        project,
-        search: sanitizeQueryString(urlSearch),
-      };
-    }
+    return resolvePatchesRoute(seed, project, segments, urlSearch);
   } else {
     return null;
   }
 }
 
+function resolvePatchesRoute(
+  seed: BaseUrl,
+  project: string,
+  segments: string[],
+  urlSearch: string,
+): ProjectPatchRoute | ProjectPatchesRoute {
+  const patch = segments.shift();
+  const revision = segments.shift();
+  if (patch) {
+    const searchParams = new URLSearchParams(sanitizeQueryString(urlSearch));
+    const tab = searchParams.get("tab");
+    const base = {
+      resource: "project.patch",
+      seed,
+      project,
+      patch,
+    } as const;
+    const diff = searchParams.get("diff");
+    if (diff) {
+      const [fromCommit, toCommit] = diff.split("..");
+      if (isOid(fromCommit) && isOid(toCommit)) {
+        return {
+          ...base,
+          view: { name: "diff", fromCommit, toCommit },
+        };
+      }
+    }
+
+    if (tab === "commits" || tab === "files") {
+      return {
+        ...base,
+        view: { name: tab, revision },
+      };
+    } else if (tab === "activity") {
+      return {
+        ...base,
+        view: { name: tab },
+      };
+    } else {
+      return base;
+    }
+  } else {
+    return {
+      resource: "project.patches",
+      seed,
+      project,
+      search: sanitizeQueryString(urlSearch),
+    };
+  }
+}
+
 export function projectRouteToPath(route: ProjectRoute): string {
   const seed = seedPath(route.seed);
 
@@ -666,21 +766,43 @@ export function projectRouteToPath(route: ProjectRoute): string {
     }
     return url;
   } else if (route.resource === "project.patch") {
-    pathSegments.push("patches", route.patch);
-    if (route.revision) {
-      pathSegments.push(route.revision);
-    }
-
-    let url = pathSegments.join("/");
-    if (route.search) {
-      url += `?${route.search}`;
-    }
-    return url;
+    return patchRouteToPath(route);
   } else {
     return unreachable(route);
   }
 }
 
+function patchRouteToPath(route: ProjectPatchRoute): string {
+  const seed = seedPath(route.seed);
+
+  const pathSegments = [seed, route.project];
+
+  pathSegments.push("patches", route.patch);
+  if (route.view?.name === "commits" || route.view?.name === "files") {
+    if (route.view.revision) {
+      pathSegments.push(route.view.revision);
+    }
+  }
+
+  let url = pathSegments.join("/");
+  if (!route.view) {
+    return url;
+  } else {
+    const searchParams = new URLSearchParams();
+
+    if (route.view.name === "diff") {
+      searchParams.set(
+        "diff",
+        `${route.view.fromCommit}..${route.view.toCommit}`,
+      );
+    } else {
+      searchParams.set("tab", route.view.name);
+    }
+    url += `?${searchParams.toString()}`;
+    return url;
+  }
+}
+
 export function projectTitle(loadedRoute: ProjectLoadedRoute) {
   const title: string[] = [];
 
diff --git a/tests/unit/router.test.ts b/tests/unit/router.test.ts
index eb50c6cd7..bb74170da 100644
--- a/tests/unit/router.test.ts
+++ b/tests/unit/router.test.ts
@@ -158,45 +158,76 @@ describe("route invariant when parsed", () => {
     });
   });
 
-  test("projects.patch", () => {
+  test("projects.patch default view", () => {
     expectParsingInvariant({
       resource: "project.patch",
       seed,
       project: "PROJECT",
       patch: "PATCH",
-      search: "",
     });
   });
 
-  test("projects.patch with revision", () => {
+  test("projects.patch activity", () => {
     expectParsingInvariant({
       resource: "project.patch",
       seed,
       project: "PROJECT",
       patch: "PATCH",
-      search: "",
-      revision: "REVISION",
+      view: { name: "activity" },
     });
   });
 
-  test("projects.patch with search", () => {
+  test("projects.patch commits", () => {
     expectParsingInvariant({
       resource: "project.patch",
       seed,
       project: "PROJECT",
       patch: "PATCH",
-      search: "SEARCH",
+      view: { name: "commits" },
     });
   });
 
-  test("projects.patch with revision and search", () => {
+  test("projects.patch commits with revision", () => {
     expectParsingInvariant({
       resource: "project.patch",
       seed,
       project: "PROJECT",
       patch: "PATCH",
-      revision: "REVISION",
-      search: "SEARCH",
+      view: { name: "commits", revision: "REVISION" },
+    });
+  });
+
+  test("projects.patch files", () => {
+    expectParsingInvariant({
+      resource: "project.patch",
+      seed,
+      project: "PROJECT",
+      patch: "PATCH",
+      view: { name: "files" },
+    });
+  });
+
+  test("projects.patch files with revision", () => {
+    expectParsingInvariant({
+      resource: "project.patch",
+      seed,
+      project: "PROJECT",
+      patch: "PATCH",
+      view: { name: "files", revision: "REVISION" },
+    });
+  });
+
+  test("projects.patch diff", () => {
+    expectParsingInvariant({
+      resource: "project.patch",
+      seed,
+      project: "PROJECT",
+      patch: "PATCH",
+      view: {
+        name: "diff",
+        fromCommit: "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa",
+        toCommit: "bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb",
+      },
     });
   });