refactor: address pr comments.

Author: knightedcodemonkey

playwright/app.spec.ts

@@ -27,6 +27,8 @@ type PullRequestCreateBody = {
   base?: string
 }
 
+type BranchesByRepo = Record<string, string[]>
+
 const waitForAppReady = async (page: Page, path = appEntryPath) => {
   await page.goto(path)
   await expect(page.getByRole('heading', { name: '@knighted/develop' })).toBeVisible()
@@ -147,6 +149,28 @@ const ensureOpenPrDrawerOpen = async (page: Page) => {
   await expect(page.locator('#github-pr-drawer')).toBeVisible()
 }
 
+const mockRepositoryBranches = async (
+  page: Page,
+  branchesByRepo: BranchesByRepo = {},
+) => {
+  await page.route('https://api.github.com/repos/**/branches**', async route => {
+    const url = new URL(route.request().url())
+    const match = url.pathname.match(/^\/repos\/([^/]+)\/([^/]+)\/branches$/)
+    const repositoryKey = match ? `${match[1]}/${match[2]}` : ''
+
+    const branchNames =
+      branchesByRepo[repositoryKey] && branchesByRepo[repositoryKey].length > 0
+        ? branchesByRepo[repositoryKey]
+        : ['main']
+
+    await route.fulfill({
+      status: 200,
+      contentType: 'application/json',
+      body: JSON.stringify(branchNames.map(name => ({ name }))),
+    })
+  })
+}
+
 const connectByotWithSingleRepo = async (page: Page) => {
   await page.route('https://api.github.com/user/repos**', async route => {
     await route.fulfill({
@@ -165,16 +189,9 @@ const connectByotWithSingleRepo = async (page: Page) => {
     })
   })
 
-  await page.route(
-    'https://api.github.com/repos/knightedcodemonkey/develop/branches**',
-    async route => {
-      await route.fulfill({
-        status: 200,
-        contentType: 'application/json',
-        body: JSON.stringify([{ name: 'main' }, { name: 'release' }]),
-      })
-    },
-  )
+  await mockRepositoryBranches(page, {
+    'knightedcodemonkey/develop': ['main', 'release'],
+  })
 
   await page.locator('#github-token-input').fill('github_pat_fake_chat_1234567890')
   await page.locator('#github-token-add').click()
@@ -539,6 +556,11 @@ test('BYOT remembers selected repository across reloads', async ({ page }) => {
     })
   })
 
+  await mockRepositoryBranches(page, {
+    'knightedcodemonkey/develop': ['main', 'release'],
+    'knightedcodemonkey/css': ['main', 'release/1.x'],
+  })
+
   await waitForAppReady(page, `${appEntryPath}?feature-ai=true`)
 
   await page.locator('#github-token-input').fill('github_pat_fake_1234567890')
@@ -588,6 +610,10 @@ test('Open PR drawer confirms and submits component/styles filepaths', async ({
     })
   })
 
+  await mockRepositoryBranches(page, {
+    'knightedcodemonkey/develop': ['main', 'release'],
+  })
+
   await page.route(
     'https://api.github.com/repos/knightedcodemonkey/develop/git/ref/**',
     async route => {

src/modules/github-api.js

@@ -420,19 +420,22 @@ export const listRepositoryBranches = async ({ token, owner, repo, signal }) =>
     throw new Error('A GitHub token is required to load branches.')
   }
 
-  if (typeof owner !== 'string' || !owner || typeof repo !== 'string' || !repo) {
+  const normalizedOwner = typeof owner === 'string' ? owner.trim() : ''
+  const normalizedRepo = typeof repo === 'string' ? repo.trim() : ''
+
+  if (!normalizedOwner || !normalizedRepo) {
     throw new Error('A valid repository owner/name is required to load branches.')
   }
 
   const branches = []
   const dedupe = new Set()
-  let nextPageUrl = `${githubApiBaseUrl}/repos/${owner}/${repo}/branches?per_page=100`
-  let remainingPageBudget = 5
+  const collectBranchesByPage = async ({ url, remainingPageBudget }) => {
+    if (!url || remainingPageBudget <= 0) {
+      return
+    }
+
+    const page = await listBranchesPage({ token, url, signal })
 
-  while (nextPageUrl && remainingPageBudget > 0) {
-    /* Branch pagination depends on the prior Link header response. */
-    // eslint-disable-next-line no-await-in-loop
-    const page = await listBranchesPage({ token, url: nextPageUrl, signal })
     for (const name of page.branches) {
       if (dedupe.has(name)) {
         continue
@@ -442,10 +445,17 @@ export const listRepositoryBranches = async ({ token, owner, repo, signal }) =>
       branches.push(name)
     }
 
-    nextPageUrl = page.nextPageUrl
-    remainingPageBudget -= 1
+    await collectBranchesByPage({
+      url: page.nextPageUrl,
+      remainingPageBudget: remainingPageBudget - 1,
+    })
   }
 
+  await collectBranchesByPage({
+    url: `${githubApiBaseUrl}/repos/${normalizedOwner}/${normalizedRepo}/branches?per_page=100`,
+    remainingPageBudget: 5,
+  })
+
   branches.sort((left, right) => left.localeCompare(right))
   return branches
 }

src/modules/github-pr-drawer.js

@@ -278,6 +278,8 @@ export const createGitHubPrDrawer = ({
   let submitting = false
   let pendingAbortController = null
   let pendingBranchesAbortController = null
+  let pendingBranchesRequestKey = ''
+  let pendingBranchesPromise = null
   let resetOnNextOpen = false
   const baseBranchesByRepository = new Map()
   const submitButtonDefaultLabel =
@@ -398,6 +400,7 @@ export const createGitHubPrDrawer = ({
     const cacheKey = toBranchCacheKey(repository)
     const nextPreferredBranch =
       toSafeText(preferredBranch) || getPreferredBaseBranchForRepository(repository)
+    const requestKey = `${cacheKey}:${nextPreferredBranch}`
 
     if (!cacheKey) {
       renderBaseBranchOptions({ preferredBranch: nextPreferredBranch, branchNames: [] })
@@ -419,13 +422,18 @@ export const createGitHubPrDrawer = ({
       return
     }
 
+    if (pendingBranchesPromise && pendingBranchesRequestKey === requestKey) {
+      await pendingBranchesPromise
+      return
+    }
+
     abortPendingBranchesRequest()
     renderBaseBranchOptions({ preferredBranch: nextPreferredBranch, loading: true })
 
     const abortController = new AbortController()
     pendingBranchesAbortController = abortController
 
-    try {
+    const runBranchRequest = async () => {
       const branches = await listRepositoryBranches({
         token,
         owner: repository.owner,
@@ -442,6 +450,15 @@ export const createGitHubPrDrawer = ({
         preferredBranch: nextPreferredBranch,
         branchNames: branches,
       })
+    }
+
+    const requestPromise = runBranchRequest()
+
+    pendingBranchesRequestKey = requestKey
+    pendingBranchesPromise = requestPromise
+
+    try {
+      await requestPromise
     } catch {
       if (abortController.signal.aborted) {
         return
@@ -452,6 +469,11 @@ export const createGitHubPrDrawer = ({
       if (pendingBranchesAbortController === abortController) {
         pendingBranchesAbortController = null
       }
+
+      if (pendingBranchesPromise === requestPromise) {
+        pendingBranchesPromise = null
+        pendingBranchesRequestKey = ''
+      }
     }
   }
 
@@ -577,6 +599,10 @@ export const createGitHubPrDrawer = ({
     const selectedRepository = getSelectedRepositoryObject()
     syncRepositorySelect({ repositories, selectedRepository })
     syncFormForRepository()
+    if (!open) {
+      return
+    }
+
     void loadBaseBranchesForSelectedRepository({
       preferredBranch: getFormValues().baseBranch,
     })
@@ -778,6 +804,7 @@ export const createGitHubPrDrawer = ({
 
   componentPathInput?.addEventListener('blur', persistCurrentPaths)
   stylesPathInput?.addEventListener('blur', persistCurrentPaths)
+  baseBranchInput?.addEventListener('change', persistCurrentPaths)
   baseBranchInput?.addEventListener('blur', persistCurrentPaths)
   headBranchInput?.addEventListener('blur', persistCurrentPaths)
   prTitleInput?.addEventListener('blur', persistCurrentPaths)
@@ -810,6 +837,10 @@ export const createGitHubPrDrawer = ({
         return
       }
 
+      if (!open) {
+        return
+      }
+
       void loadBaseBranchesForSelectedRepository({
         preferredBranch: getFormValues().baseBranch,
       })