Resolve TODO comments from codebase (#58691)

Author: heiskr

.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)
 
+          // Read the product index data for rendering
+          const productIndexContents = await fs.promises.readFile(productIndex, 'utf8')
+          const productIndexData = getFrontmatterData(productIndexContents)
+
           // Save the index title for later testing
-          indexTitle = data.title.includes('{')
-            ? await renderContent(data.title, req.context, { textOnly: true })
-            : data.title
-
-          if (data.shortTitle) {
-            indexShortTitle = data.shortTitle.includes('{')
-              ? await renderContent(data.shortTitle, req.context, { textOnly: true })
-              : data.shortTitle
+          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'
-
-  // 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]
+  // 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., '[email protected]')
+  // If yes, product is in pathParts[2], otherwise it's in pathParts[1]
+  // Examples:
+  //   /[email protected]/admin -> product is 'admin'
+  //   /github/getting-started -> product is 'github'
+  //   /[email protected] -> product is '[email protected]' (enterprise landing)
+  const hasVersionPrefix = supportedVersions.has(pathParts[1])
+  const productString = hasVersionPrefix && 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']
   }