#45 handle url hash when detecting / redirecting to a diff view

rikukissa · Jan 11, 2018 · ac66e8b · ac66e8b
1 parent 3472664
commit ac66e8b
Show file tree

Hide file tree

Showing 3 changed files with 32 additions and 8 deletions.
diff --git a/src/features/diff-select/index.e2e.test.ts b/src/features/diff-select/index.e2e.test.ts
@@ -29,14 +29,30 @@ describe("Automatic diff redirect", () => {
     });
   });
 
+  describe("with non-based PRs when in 'submit review' state", () => {
+    beforeEach(async () => {
+      await page.goto(
+        "https://github.com/rikukissa/stacker-e2e-repo/pull/2/files#submit-review"
+      );
+    });
+
+    it("redirects the browser to a view with only related commits visible", async () => {
+      await page.waitForNavigation();
+
+      expect(await page.evaluate(() => location.href)).toEqual(
+        "https://github.com/rikukissa/stacker-e2e-repo/pull/2/files/219075335f75f6428f4ffb68669e922f441b8146..db4ae1e85ba25f975e3541e191055cc5be8dd54f#submit-review"
+      );
+    });
+  });
+
   describe("with based PRs", () => {
     beforeEach(async () => {
       await page.goto(
         "https://github.com/rikukissa/stacker-e2e-repo/pull/6/files"
       );
     });
 
-    it("redirects the browser to a view with only related commits visible", async () => {
+    it("doesn't redirect the browser anywhere", async () => {
       await page.waitForSelector(".stacker-diff-view-initialized");
       expect(page.url()).toEqual(
         "https://github.com/rikukissa/stacker-e2e-repo/pull/6/files"

diff --git a/src/features/diff-select/index.tsx b/src/features/diff-select/index.tsx
@@ -26,15 +26,22 @@ async function getNewCommits(
   );
 }
 
-function getDiffViewUrl(commits: IGithubCommit[]): string | null {
+function getDiffViewUrl(
+  currentLocation: Location,
+  commits: IGithubCommit[]
+): string | null {
   if (commits.length === 1) {
-    return window.location.href.replace(/files$/, `commits/${commits[0].sha}`);
+    return (
+      currentLocation.pathname.replace(/files$/, `commits/${commits[0].sha}`) +
+      currentLocation.hash
+    );
   } else if (commits.length > 1) {
     return (
-      window.location.href +
+      currentLocation.pathname +
       // TODO this is a too big of an assumption
       // https://softwareengineering.stackexchange.com/questions/314215/can-a-git-commit-have-more-than-2-parents
-      `/${commits[0].parents[0].sha}..${commits[commits.length - 1].sha}`
+      `/${commits[0].parents[0].sha}..${commits[commits.length - 1].sha}` +
+      currentLocation.hash
     );
   } else {
     return null;
@@ -45,7 +52,7 @@ async function redirectToPullRequestView(
   context: IStackerContext,
   newCommits: IGithubCommit[]
 ) {
-  const newUrl = getDiffViewUrl(newCommits);
+  const newUrl = getDiffViewUrl(context.location, newCommits);
   if (newUrl) {
     window.location.href = newUrl;
   }
@@ -84,7 +91,7 @@ export default async function initialize(context: IStackerContext) {
       return redirectToPullRequestView(context, newCommits);
     }
 
-    const diffViewUrl = getDiffViewUrl(newCommits);
+    const diffViewUrl = getDiffViewUrl(context.location, newCommits);
 
     const $existingNotification = document.getElementById(
       "stacker-files-notification"

diff --git a/src/lib/location.ts b/src/lib/location.ts
@@ -21,8 +21,9 @@ export function isPRView(location: Location, config: IConfig): boolean {
 }
 
 export function isFilesView(location: Location) {
-  return /pull\/(\d+)\/files$/.test(location.href);
+  return /pull\/(\d+)\/files$/.test(location.pathname);
 }
+
 export function isFilesDiffView(location: Location) {
   return /pull\/(\d+)\/files\/[a-z0-9]+\.\.[a-z0-9]+$/i.test(location.href);
 }