Resolve TODO comments from codebase (#58691)

.github/instructions/all.instructions.md

@@ -28,4 +28,5 @@ When you create a pull request:
    
 3. Label with "llm-generated". 
 4. If an issue exists, include "fixes owner/repo#issue" or "towards owner/repo#issue" as appropriate. 
-5. Always _escape backticks_ when you use gh cli.
+5. Always create PRs in **draft mode** using `--draft` flag.
+6. Always _escape backticks_ when you use gh cli.

src/content-linter/lib/linting-rules/liquid-ifversion-versions.ts

@@ -206,9 +206,10 @@ async function getApplicableVersionFromLiquidTag(conditionStr: string) {
       const ands = ver.split(' and ')
       const firstAnd = ands[0].split(' ')[0]
       // if all ands don't start with the same version it's invalid
+      // Note: This edge case (e.g., "fpt and ghes >= 3.1") doesn't occur in our content.
+      // All actual uses have matching versions (e.g., "ghes and ghes > 3.19").
+      // If this edge case appears in the future, additional logic would be needed here.
       if (!ands.every((and) => and.startsWith(firstAnd))) {
-        // noop - we don't handle this case
-        // TODO - handle this case in the future
         return []
       }
       const andValues = []

src/content-linter/tests/category-pages.ts

@@ -50,14 +50,17 @@ describe.skip('category pages', () => {
   // otherwise, if one of them has no categories, the tests will fail.
   for (const tuple of productTuples) {
     const [, productIndex] = tuple
+
+    const productDir = path.dirname(productIndex)
+
     // Get links included in product index page.
     // Each link corresponds to a product subdirectory (category).
     // Example: "getting-started-with-github"
+    // Note: We need to read this synchronously here because vitest's describe.each
+    // can't asynchronously define tests
     const contents = fs.readFileSync(productIndex, 'utf8')
     const data = getFrontmatterData(contents)
 
-    const productDir = path.dirname(productIndex)
-
     const children: string[] = data.children
     const categoryLinks = children
       // Only include category directories, not standalone category files like content/actions/quickstart.md
@@ -118,15 +121,19 @@ describe.skip('category pages', () => {
           await contextualize(req as ExtendedRequest, res as Response, next)
           await shortVersions(req as ExtendedRequest, res as Response, next)
 
-          // Save the index title for later testing
-          indexTitle = data.title.includes('{')
-            ? await renderContent(data.title, req.context, { textOnly: true })
-            : data.title
+          // Read the product index data for rendering
+          const productIndexContents = await fs.promises.readFile(productIndex, 'utf8')
+          const productIndexData = getFrontmatterData(productIndexContents)
 
-          if (data.shortTitle) {
-            indexShortTitle = data.shortTitle.includes('{')
-              ? await renderContent(data.shortTitle, req.context, { textOnly: true })
-              : data.shortTitle
+          // Save the index title for later testing
+          indexTitle = productIndexData.title.includes('{')
+            ? await renderContent(productIndexData.title, req.context, { textOnly: true })
+            : productIndexData.title
+
+          if (productIndexData.shortTitle) {
+            indexShortTitle = productIndexData.shortTitle.includes('{')
+              ? await renderContent(productIndexData.shortTitle, req.context, { textOnly: true })
+              : productIndexData.shortTitle
           } else {
             indexShortTitle = ''
           }

src/fixtures/tests/minitoc.ts

@@ -4,7 +4,6 @@ import cheerio from 'cheerio'
 import { getDOM } from '@/tests/helpers/e2etest'
 
 describe('minitoc', () => {
-  // TODO disable the mini TOC tests when we replace it with sticky TOC header
   test('renders mini TOC in articles with more than one heading', async () => {
     const $: cheerio.Root = await getDOM('/en/get-started/minitocs/multiple-headings')
     expect($('h2#in-this-article').length).toBe(1)

src/frame/lib/path-utils.ts

@@ -109,32 +109,37 @@ export function getVersionObjectFromPath(href: string | undefined) {
   return allVersions[versionFromPath]
 }
 
-// TODO needs refactoring + tests
 // Return the product segment from the path
+// Extracts the product identifier from various URL patterns including versioned paths
 export function getProductStringFromPath(href: string | undefined): string {
+  // Handle empty or undefined paths
   if (!href) return 'homepage'
-  href = getPathWithoutLanguage(href)
 
-  if (href === '/') return 'homepage'
+  const normalizedHref = getPathWithoutLanguage(href)
+  if (normalizedHref === '/') return 'homepage'
 
-  // The first segment will always be empty on this split
-  const pathParts = href.split('/')
+  // Split path into segments (first segment is always empty string)
+  const pathParts = normalizedHref.split('/')
 
+  // Handle special product paths that appear anywhere in the URL
   if (pathParts.includes('early-access')) return 'early-access'
 
-  // For rest pages the currentProduct should be rest
-  // We use this to show SidebarRest, which is a different sidebar than the rest of the site
-  if (pathParts[1] === 'rest') return 'rest'
-  if (pathParts[1] === 'copilot') return 'copilot'
-  if (pathParts[1] === 'get-started') return 'get-started'
+  // Handle special products that always appear as the first segment
+  // These products use custom sidebars and need explicit handling
+  const specialProducts = ['rest', 'copilot', 'get-started']
+  if (specialProducts.includes(pathParts[1])) {
+    return pathParts[1]
+  }
+
+  // Determine if first segment is a version (e.g., 'enterprise-server@3.9')
+  // If yes, product is in pathParts[2], otherwise it's in pathParts[1]
+  // Examples:
+  //   /enterprise-server@3.9/admin -> product is 'admin'
+  //   /github/getting-started -> product is 'github'
+  //   /enterprise-server@3.9 -> product is 'enterprise-server@3.9' (enterprise landing)
+  const hasVersionPrefix = supportedVersions.has(pathParts[1])
+  const productString = hasVersionPrefix && pathParts[2] ? pathParts[2] : pathParts[1]
 
-  // Possible scenarios for href (assume part[0] is an empty string):
-  //
-  // * part[1] is a version and part[2] is undefined, so return part[1] as an enterprise landing page
-  // * part[1] is a version and part[2] is defined, so return part[2] as the product
-  // * part[1] is NOT a version, so return part[1] as the product
-  const isEnterprise = supportedVersions.has(pathParts[1])
-  const productString = isEnterprise && pathParts[2] ? pathParts[2] : pathParts[1]
   return productString
 }
 

src/frame/middleware/find-page.ts

@@ -105,10 +105,9 @@ async function rereadByPath(
   const languageCode = match[1]
   const withoutLanguage = uri.replace(languagePrefixPathRegex, '/')
   const withoutVersion = withoutLanguage.replace(`/${currentVersion}`, '')
-  // TODO: Support loading translations the same way.
-  // NOTE: No one is going to test translations like this in development
-  // but perhaps one day we can always and only do these kinds of lookups
-  // at runtime.
+  // Note: We don't support loading translations at runtime. All translations
+  // are loaded at build time. This function only handles English content reloading
+  // during development.
   const possible = path.join(contentRoot, withoutVersion)
   const filePath = existsSync(possible) ? path.join(possible, 'index.md') : `${possible}.md`
   const relativePath = path.relative(contentRoot, filePath)

src/frame/tests/path-utils.ts

@@ -0,0 +1,47 @@
+import { describe, expect, test } from 'vitest'
+
+import { getProductStringFromPath } from '@/frame/lib/path-utils'
+
+describe('getProductStringFromPath', () => {
+  test('returns "homepage" for root paths', () => {
+    expect(getProductStringFromPath('/')).toBe('homepage')
+    expect(getProductStringFromPath('/en')).toBe('homepage')
+    expect(getProductStringFromPath(undefined)).toBe('homepage')
+  })
+
+  test('handles early-access product', () => {
+    expect(getProductStringFromPath('/en/early-access/github/overview')).toBe('early-access')
+    expect(getProductStringFromPath('/early-access/admin/guides')).toBe('early-access')
+  })
+
+  test('handles special products (rest, copilot, get-started)', () => {
+    expect(getProductStringFromPath('/en/rest/overview')).toBe('rest')
+    expect(getProductStringFromPath('/rest/reference/repos')).toBe('rest')
+    expect(getProductStringFromPath('/en/copilot/using-copilot')).toBe('copilot')
+    expect(getProductStringFromPath('/copilot/features')).toBe('copilot')
+    expect(getProductStringFromPath('/en/get-started/quickstart')).toBe('get-started')
+    expect(getProductStringFromPath('/get-started/onboarding')).toBe('get-started')
+  })
+
+  test('extracts product from non-versioned paths', () => {
+    expect(getProductStringFromPath('/en/github/getting-started')).toBe('github')
+    expect(getProductStringFromPath('/actions/quickstart')).toBe('actions')
+    expect(getProductStringFromPath('/en/admin/installation')).toBe('admin')
+    expect(getProductStringFromPath('/code-security/overview')).toBe('code-security')
+  })
+
+  test('extracts product from versioned paths', () => {
+    // Note: These tests use free-pro-team which is a supported version
+    expect(getProductStringFromPath('/en/free-pro-team@latest/admin/installation')).toBe('admin')
+    expect(getProductStringFromPath('/free-pro-team@latest/actions/quickstart')).toBe('actions')
+    expect(getProductStringFromPath('/en/free-pro-team@latest/github/getting-started')).toBe(
+      'github',
+    )
+  })
+
+  test('handles enterprise landing pages (version without product)', () => {
+    // When a version is present but no product segment follows, return the version string
+    expect(getProductStringFromPath('/en/free-pro-team@latest')).toBe('free-pro-team@latest')
+    expect(getProductStringFromPath('/enterprise-server@latest')).toBe('enterprise-server@latest')
+  })
+})

src/rest/scripts/utils/create-rest-examples.ts

@@ -352,10 +352,9 @@ export function getResponseExamples(operation: Operation): ResponseExample[] {
             contentType,
             description: examples[key].summary || operation.responses[statusCode].description,
             example: examples[key].value,
-            // TODO adding the schema quadruples the JSON file size. Changing
-            // how we write the JSON file helps a lot, but we should revisit
-            // adding the response schema to ensure we have a way to view the
-            // prettified JSON before minimizing it.
+            // Note: Including the schema significantly increases JSON file size (~4x),
+            // but it's necessary to support the schema/example toggle in the UI.
+            // Users can switch between viewing the example response and the full schema.
             schema: operation.responses[statusCode].content[contentType].schema,
           },
         }

src/rest/scripts/utils/inject-models-schema.ts

@@ -64,36 +64,18 @@ export async function injectModelsSchema(schema: any, schemaName: string): Promi
 
         // Use values from the YAML where possible
         const name = operationObject.summary || ''
-        const description = operationObject.description || ''
-        const category = operationObject['x-github']?.category || 'models'
 
         console.log(`⏳ Processing operation: ${name} (${path} ${operation})`)
 
-        // Create enhanced operation preserving all original fields
-        // TODO this should be cleaned up, most can be removed
+        // Create enhanced operation with custom fields needed for our REST docs
+        // The spread operator preserves all original OpenAPI fields
         const enhancedOperation = {
-          ...operationObject, // Keep all original fields
-          operationId: operationObject.operationId, // Preserve original operationId with namespace
-          tags: operationObject.tags || ['models'], // Only use 'models' if no tags present
+          ...operationObject,
+          // Add custom fields for our docs processing
           verb: operation,
           requestPath: path,
-          category,
-          subcategory: operationObject['x-github']?.subcategory || '',
-          summary: name,
-          description,
-          'x-github': {
-            ...operationObject['x-github'], // Preserve all x-github metadata
-            category,
-            enabledForGitHubApps: operationObject['x-github']?.enabledForGitHubApps,
-            githubCloudOnly: operationObject['x-github']?.githubCloudOnly,
-            permissions: operationObject['x-github']?.permissions || {},
-            externalDocs: operationObject['x-github']?.externalDocs || {},
-          },
-          parameters: operationObject.parameters || [],
-          responses: {
-            ...operationObject.responses,
-            '200': operationObject.responses?.['200'],
-          },
+          // Override tags with default if not present
+          tags: operationObject.tags || ['models'],
         }
 
         // Preserve operation-level servers if present

src/search/lib/elasticsearch-indexes.ts

@@ -71,10 +71,9 @@ export function getElasticSearchIndex(
   // e.g. free-pro-team becomes fpt for the index name
   let indexVersion = versionToIndexVersionMap[version]
 
-  // TODO: For AI Search, we initially only supported the latest GHES version
-  // Supporting more versions would involve adding more indexes and generating the data to fill them
-  // As a work around, we will just use the latest version for all GHES suggestions / autocomplete
-  // This is a temporary fix until we can support more versions
+  // For AI Search autocomplete, we use the latest GHES version for all GHES versions.
+  // This provides AI search functionality across all supported GHES versions without
+  // requiring separate indexes for each version.
   if (type === 'aiSearchAutocomplete' && indexVersion.startsWith('ghes')) {
     indexVersion = versionToIndexVersionMap['enterprise-server']
   }

src/search/lib/helpers/old-version-logic.ts

@@ -1,44 +0,0 @@
-import { allVersions } from '@/versions/lib/all-versions'
-
-// TODO: Old version logic
-type VersionAliases = { [key: string]: string }
-export const versionAliases: VersionAliases = {}
-export const prefixVersionAliases: VersionAliases = {}
-
-for (const info of Object.values(allVersions)) {
-  if (info.hasNumberedReleases) {
-    versionAliases[info.currentRelease] = info.miscVersionName
-  } else {
-    versionAliases[info.version] = info.miscVersionName
-    versionAliases[info.miscVersionName] = info.miscVersionName
-  }
-  prefixVersionAliases[info.plan] = info.shortName
-  prefixVersionAliases[info.shortName] = info.shortName
-}
-
-// Temporary hard-coded switch
-//
-// We need to run workflows in production to index the search data
-// We want the middleware + routes that consume the indexes to consume the old indexes
-//  until the new indexes are ready.
-
-// Once they are ready we can remove this file & cleanup the places it is used
-export function isBeforeSearchIndexMigration() {
-  if (process.env.NODE_ENV === 'production') return true
-  return false
-}
-
-// Old test prefix helper function
-export function getGeneralSearchIndexPrefix(): string {
-  if (process.env.NODE_ENV === 'test') return 'tests_'
-  return ''
-}
-
-export function getGeneralSearchIndexVersion(paramVersion: string): string {
-  const version =
-    prefixVersionAliases[paramVersion] ||
-    versionAliases[paramVersion] ||
-    allVersions[paramVersion].miscVersionName
-
-  return version
-}

src/search/lib/routes/ai-search-autocomplete-route.ts

@@ -0,0 +1,44 @@
+import type { Request, Response } from 'express'
+
+import { searchCacheControl } from '@/frame/middleware/cache-control'
+import { setFastlySurrogateKey, SURROGATE_ENUMS } from '@/frame/middleware/set-fastly-surrogate-key'
+import { getAISearchAutocompleteResults } from '@/search/lib/get-elasticsearch-results/ai-search-autocomplete'
+import { getSearchFromRequestParams } from '@/search/lib/search-request-params/get-search-from-request-params'
+import { handleGetSearchResultsError } from '@/search/middleware/search-routes'
+
+export async function aiSearchAutocompleteRoute(req: Request, res: Response) {
+  // If no query is provided, we want to return the top 5 most popular terms
+  // This is a special case for AI search autocomplete
+  // So we use `force` to allow the query to be empty without the usual validation error
+  const force = {} as any
+  if (!req.query.query) {
+    force.query = ''
+  }
+  const {
+    indexName,
+    validationErrors,
+    searchParams: { query, size, debug },
+  } = getSearchFromRequestParams(req, 'aiSearchAutocomplete', force)
+  if (validationErrors.length) {
+    return res.status(400).json(validationErrors[0])
+  }
+
+  const getResultOptions = {
+    indexName,
+    query,
+    size,
+    debug,
+  }
+  try {
+    const { meta, hits } = await getAISearchAutocompleteResults(getResultOptions)
+
+    if (process.env.NODE_ENV !== 'development') {
+      searchCacheControl(res)
+      setFastlySurrogateKey(res, SURROGATE_ENUMS.DEFAULT)
+    }
+
+    res.status(200).json({ meta, hits })
+  } catch (error) {
+    await handleGetSearchResultsError(req, res, error, getResultOptions)
+  }
+}

src/search/lib/routes/general-search-route.ts

@@ -0,0 +1,47 @@
+import type { Request, Response } from 'express'
+
+import { searchCacheControl } from '@/frame/middleware/cache-control'
+import { setFastlySurrogateKey, SURROGATE_ENUMS } from '@/frame/middleware/set-fastly-surrogate-key'
+import { getSearchFromRequestParams } from '@/search/lib/search-request-params/get-search-from-request-params'
+import { getGeneralSearchResults } from '@/search/lib/get-elasticsearch-results/general-search'
+import { handleGetSearchResultsError } from '@/search/middleware/search-routes'
+import { handleExternalSearchAnalytics } from '@/search/lib/helpers/external-search-analytics'
+
+export async function generalSearchRoute(req: Request, res: Response) {
+  const { indexName, searchParams, validationErrors } = getSearchFromRequestParams(
+    req,
+    'generalSearch',
+  )
+  if (validationErrors.length) {
+    // We only send the first validation error to the user
+    return res.status(400).json(validationErrors[0])
+  }
+
+  // Handle search analytics and client_name validation
+  const analyticsError = await handleExternalSearchAnalytics(req, 'general-search')
+  if (analyticsError) {
+    return res.status(analyticsError.status).json({
+      error: analyticsError.error,
+    })
+  }
+
+  const getResultOptions = {
+    indexName,
+    searchParams,
+  }
+  try {
+    const { meta, hits, aggregations } = await getGeneralSearchResults(getResultOptions)
+
+    if (process.env.NODE_ENV !== 'development') {
+      searchCacheControl(res)
+      // We can cache this without purging it after every deploy
+      // because the API search is only used as a proxy for local
+      // and review environments.
+      setFastlySurrogateKey(res, SURROGATE_ENUMS.MANUAL)
+    }
+
+    res.status(200).json({ meta, hits, aggregations })
+  } catch (error) {
+    await handleGetSearchResultsError(req, res, error, getResultOptions)
+  }
+}

src/search/middleware/search-routes.ts

@@ -3,18 +3,13 @@
 
  For general search (client searches on docs.github.com) we use the middleware in ./general-search-middleware to get the search results
 */
-// TODO: Move the routes implementations in this files to lib/routes so you can at-a-glance see all of the routes without the implementation logic
 import express, { Request, Response } from 'express'
 
 import FailBot from '@/observability/lib/failbot'
-import { searchCacheControl } from '@/frame/middleware/cache-control'
 import catchMiddlewareError from '@/observability/middleware/catch-middleware-error'
-import { setFastlySurrogateKey, SURROGATE_ENUMS } from '@/frame/middleware/set-fastly-surrogate-key'
-import { getAISearchAutocompleteResults } from '@/search/lib/get-elasticsearch-results/ai-search-autocomplete'
-import { getSearchFromRequestParams } from '@/search/lib/search-request-params/get-search-from-request-params'
-import { getGeneralSearchResults } from '@/search/lib/get-elasticsearch-results/general-search'
+import { generalSearchRoute } from '@/search/lib/routes/general-search-route'
+import { aiSearchAutocompleteRoute } from '@/search/lib/routes/ai-search-autocomplete-route'
 import { combinedSearchRoute } from '@/search/lib/routes/combined-search-route'
-import { handleExternalSearchAnalytics } from '@/search/lib/helpers/external-search-analytics'
 
 const router = express.Router()
 
@@ -22,96 +17,13 @@ router.get('/legacy', (req: Request, res: Response) => {
   res.status(410).send('Use /api/search/v1 instead.')
 })
 
-router.get(
-  '/v1',
-  catchMiddlewareError(async (req: Request, res: Response) => {
-    const { indexName, searchParams, validationErrors } = getSearchFromRequestParams(
-      req,
-      'generalSearch',
-    )
-    if (validationErrors.length) {
-      // We only send the first validation error to the user
-      return res.status(400).json(validationErrors[0])
-    }
+router.get('/v1', catchMiddlewareError(generalSearchRoute))
 
-    // Handle search analytics and client_name validation
-    const analyticsError = await handleExternalSearchAnalytics(req, 'general-search')
-    if (analyticsError) {
-      return res.status(analyticsError.status).json({
-        error: analyticsError.error,
-      })
-    }
-
-    const getResultOptions = {
-      indexName,
-      searchParams,
-    }
-    try {
-      const { meta, hits, aggregations } = await getGeneralSearchResults(getResultOptions)
-
-      if (process.env.NODE_ENV !== 'development') {
-        searchCacheControl(res)
-        // We can cache this without purging it after every deploy
-        // because the API search is only used as a proxy for local
-        // and review environments.
-        setFastlySurrogateKey(res, SURROGATE_ENUMS.MANUAL)
-      }
-
-      res.status(200).json({ meta, hits, aggregations })
-    } catch (error) {
-      await handleGetSearchResultsError(req, res, error, getResultOptions)
-    }
-  }),
-)
-
-router.get(
-  '/ai-search-autocomplete/v1',
-  catchMiddlewareError(async (req: Request, res: Response) => {
-    // If no query is provided, we want to return the top 5 most popular terms
-    // This is a special case for AI search autocomplete
-    // So we use `force` to allow the query to be empty without the usual validation error
-    const force = {} as any
-    if (!req.query.query) {
-      force.query = ''
-    }
-    const {
-      indexName,
-      validationErrors,
-      searchParams: { query, size, debug },
-    } = getSearchFromRequestParams(req, 'aiSearchAutocomplete', force)
-    if (validationErrors.length) {
-      return res.status(400).json(validationErrors[0])
-    }
-
-    const getResultOptions = {
-      indexName,
-      query,
-      size,
-      debug,
-    }
-    try {
-      const { meta, hits } = await getAISearchAutocompleteResults(getResultOptions)
-
-      if (process.env.NODE_ENV !== 'development') {
-        searchCacheControl(res)
-        setFastlySurrogateKey(res, SURROGATE_ENUMS.DEFAULT)
-      }
-
-      res.status(200).json({ meta, hits })
-    } catch (error) {
-      await handleGetSearchResultsError(req, res, error, getResultOptions)
-    }
-  }),
-)
+router.get('/ai-search-autocomplete/v1', catchMiddlewareError(aiSearchAutocompleteRoute))
 
 // Route used by our frontend to fetch ai autocomplete search suggestions + general search results in a single request
 // Combining this into a single request results in less overall requests to the server
-router.get(
-  '/combined-search/v1',
-  catchMiddlewareError(async (req: Request, res: Response) => {
-    combinedSearchRoute(req, res)
-  }),
-)
+router.get('/combined-search/v1', catchMiddlewareError(combinedSearchRoute))
 
 export async function handleGetSearchResultsError(
   req: Request,

src/search/scripts/scrape/lib/parse-page-sections-into-records.ts

@@ -70,9 +70,8 @@ export default function parsePageSectionsIntoRecords(page: any): Record {
   // We need to avoid these because if you use `getAllText()` on these
   // pages, it will extract *everything* from the page, which will
   // include the side bar and footer.
-  // TODO: Come up a custom solution to extract some text from these
-  // pages that yields some decent content to be searched on, because
-  // when you view these pages in a browser, there's clearly text there.
+  // Note: We're not adding custom extraction for guide pages as they are
+  // being phased out and don't warrant the effort.
   if ($root.length > 0) {
     body = render($root)
   }

src/types/types.ts

@@ -27,8 +27,12 @@ export type ExtendedRequest = Request & {
   FailBot?: Failbot
 }
 
-// TODO: Make this type from inference using AJV based on the schema.
-// For now, it's based on `schema` in frame/lib/frontmatter.ts
+// This type is manually maintained based on `schema` in frame/lib/frontmatter.ts
+// We're not auto-generating this from the AJV schema because:
+// 1. It would require significant build tooling (json-schema-to-typescript or similar)
+// 2. The schema is dynamically constructed with version-specific properties
+// 3. Manual maintenance provides better type control and documentation
+// 4. The effort/benefit tradeoff doesn't justify the complexity
 export type PageFrontmatter = {
   title: string
   versions: FrontmatterVersions