Detect GitHub PR URLs with trailing segments (#2002)

* fix(github): detect pr urls with trailing segments

Reference docs/detect-github-pr-with-changes-ending.md for the parser behavior and validation plan.

* chore: remove github pr url design doc

src/renderer/src/lib/github-links.test.ts

@@ -1,15 +1,98 @@
 import { describe, expect, it } from 'vitest'
-import { normalizeGitHubLinkQuery, parseGitHubIssueOrPRNumber } from './github-links'
+import {
+  normalizeGitHubLinkQuery,
+  parseGitHubIssueOrPRLink,
+  parseGitHubIssueOrPRNumber
+} from './github-links'
 
 describe('parseGitHubIssueOrPRNumber', () => {
   it('parses plain issue numbers and GitHub pull request URLs', () => {
     expect(parseGitHubIssueOrPRNumber('42')).toBe(42)
     expect(parseGitHubIssueOrPRNumber('#42')).toBe(42)
     expect(parseGitHubIssueOrPRNumber('https://github.com/stablyai/orca/pull/123')).toBe(123)
+    expect(parseGitHubIssueOrPRNumber('https://github.com/stablyai/orca/issues/923')).toBe(923)
   })
 
-  it('rejects non-GitHub URLs', () => {
+  it('parses GitHub item URLs with trailing page segments', () => {
+    expect(parseGitHubIssueOrPRNumber('https://github.com/o/r/pull/1965/changes')).toBe(1965)
+    expect(parseGitHubIssueOrPRNumber('https://github.com/o/r/pull/1965/files')).toBe(1965)
+    expect(parseGitHubIssueOrPRNumber('https://github.com/o/r/pull/1965/commits')).toBe(1965)
+    expect(parseGitHubIssueOrPRNumber('https://github.com/o/r/issues/923/comments')).toBe(923)
+  })
+
+  it('parses trailing segments with query, fragment, and repeated slashes', () => {
+    expect(parseGitHubIssueOrPRNumber('https://github.com/o/r/pull/1965/changes?diff=split')).toBe(
+      1965
+    )
+    expect(
+      parseGitHubIssueOrPRNumber('https://github.com/o/r/issues/923/comments#issuecomment-1')
+    ).toBe(923)
+    expect(parseGitHubIssueOrPRNumber('https://github.com/o/r/pull/1965//changes///')).toBe(1965)
+    expect(parseGitHubIssueOrPRNumber('https://github.com/o/r/issues/923///')).toBe(923)
+  })
+
+  it('rejects invalid GitHub item URLs', () => {
     expect(parseGitHubIssueOrPRNumber('https://example.com/stablyai/orca/pull/123')).toBeNull()
+    expect(
+      parseGitHubIssueOrPRNumber('https://github.example.com/stablyai/orca/pull/123')
+    ).toBeNull()
+    expect(
+      parseGitHubIssueOrPRNumber('https://github.com/o/r/pull/not-a-number/changes')
+    ).toBeNull()
+    expect(parseGitHubIssueOrPRNumber('https://github.com/o/r/pull/')).toBeNull()
+    expect(parseGitHubIssueOrPRNumber('https://github.com/o/r/issues/123abc')).toBeNull()
+    expect(parseGitHubIssueOrPRNumber('https://github.com/owner/repo/pulls/123')).toBeNull()
+  })
+})
+
+describe('parseGitHubIssueOrPRLink', () => {
+  it('parses slug, number, and type for direct item URLs', () => {
+    expect(parseGitHubIssueOrPRLink('https://github.com/stablyai/orca/pull/123')).toEqual({
+      slug: { owner: 'stablyai', repo: 'orca' },
+      number: 123,
+      type: 'pr'
+    })
+    expect(parseGitHubIssueOrPRLink('https://github.com/stablyai/orca/issues/923')).toEqual({
+      slug: { owner: 'stablyai', repo: 'orca' },
+      number: 923,
+      type: 'issue'
+    })
+  })
+
+  it('derives item type from the route segment when trailing segments are present', () => {
+    expect(parseGitHubIssueOrPRLink('https://github.com/o/r/pull/1965/changes')).toEqual({
+      slug: { owner: 'o', repo: 'r' },
+      number: 1965,
+      type: 'pr'
+    })
+    expect(parseGitHubIssueOrPRLink('https://github.com/o/r/issues/923/comments')).toEqual({
+      slug: { owner: 'o', repo: 'r' },
+      number: 923,
+      type: 'issue'
+    })
+  })
+
+  it('accepts query, fragment, and repeated trailing slashes', () => {
+    expect(parseGitHubIssueOrPRLink('https://github.com/o/r/pull/1965/files?plain=1#diff')).toEqual(
+      {
+        slug: { owner: 'o', repo: 'r' },
+        number: 1965,
+        type: 'pr'
+      }
+    )
+    expect(parseGitHubIssueOrPRLink('https://github.com/o/r/issues/923/comments///')).toEqual({
+      slug: { owner: 'o', repo: 'r' },
+      number: 923,
+      type: 'issue'
+    })
+  })
+
+  it('rejects non-GitHub and malformed item URLs', () => {
+    expect(parseGitHubIssueOrPRLink('https://example.com/o/r/pull/1965/changes')).toBeNull()
+    expect(parseGitHubIssueOrPRLink('https://github.com/o/r/pull/not-a-number/changes')).toBeNull()
+    expect(parseGitHubIssueOrPRLink('https://github.com/o/r/pull/')).toBeNull()
+    expect(parseGitHubIssueOrPRLink('https://github.com/o/r/issues/123abc')).toBeNull()
+    expect(parseGitHubIssueOrPRLink('https://github.com/owner/repo/pulls/123')).toBeNull()
   })
 })
 

src/renderer/src/lib/github-links.ts

@@ -1,5 +1,4 @@
-const GH_ITEM_PATH_RE = /^\/[^/]+\/[^/]+\/(?:issues|pull)\/(\d+)(?:\/)?$/i
-const GH_ITEM_PATH_FULL_RE = /^\/([^/]+)\/([^/]+)\/(?:issues|pull)\/(\d+)(?:\/)?$/i
+const GH_ITEM_PATH_RE = /^\/([^/]+)\/([^/]+)\/(issues|pull)\/(\d+)(?:\/.*)?$/i
 
 export type RepoSlug = {
   owner: string
@@ -11,6 +10,10 @@ export type GitHubLinkQuery = {
   directNumber: number | null
 }
 
+function matchGitHubItemPath(url: URL): RegExpExecArray | null {
+  return GH_ITEM_PATH_RE.exec(url.pathname.replace(/\/+$/, ''))
+}
+
 /**
  * Parses a GitHub issue/PR reference from plain input.
  * Supports issue/PR numbers (e.g. "42"), "#42", and full GitHub URLs.
@@ -37,12 +40,12 @@ export function parseGitHubIssueOrPRNumber(input: string): number | null {
     return null
   }
 
-  const match = GH_ITEM_PATH_RE.exec(url.pathname)
+  const match = matchGitHubItemPath(url)
   if (!match) {
     return null
   }
 
-  return Number.parseInt(match[1], 10)
+  return Number.parseInt(match[4], 10)
 }
 
 /**
@@ -70,15 +73,15 @@ export function parseGitHubIssueOrPRLink(input: string): {
     return null
   }
 
-  const match = GH_ITEM_PATH_FULL_RE.exec(url.pathname)
+  const match = matchGitHubItemPath(url)
   if (!match) {
     return null
   }
 
   return {
     slug: { owner: match[1], repo: match[2] },
-    type: url.pathname.toLowerCase().includes('/pull/') ? 'pr' : 'issue',
-    number: Number.parseInt(match[3], 10)
+    type: match[3].toLowerCase() === 'pull' ? 'pr' : 'issue',
+    number: Number.parseInt(match[4], 10)
   }
 }