From da1056fc7b21b42504c7139e12fd042eb3344efc Mon Sep 17 00:00:00 2001 From: Kevin Heis Date: Tue, 15 Sep 2026 20:16:21 +0000 Subject: [PATCH 01/16] Trim excessive comments in src/article-api lib, middleware, and scripts (#63279) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 17588eeb-788a-4f82-9d36-b5079fb36521 --- src/article-api/lib/get-all-toc-items.ts | 11 -------- src/article-api/lib/get-link-data.ts | 10 ++----- src/article-api/lib/graphql-helpers.ts | 5 +--- src/article-api/lib/load-template.ts | 22 ++-------------- src/article-api/lib/strip-html-comments.ts | 21 ++------------- src/article-api/lib/summarize-schema.ts | 4 --- src/article-api/liquid-renderers/index.ts | 9 +------ src/article-api/liquid-renderers/rest-tags.ts | 26 +++---------------- src/article-api/middleware/article-body.ts | 8 +----- src/article-api/middleware/pagelist.ts | 7 ----- src/article-api/middleware/validation.ts | 13 ++-------- src/article-api/scripts/generate-api-docs.ts | 15 +---------- 12 files changed, 15 insertions(+), 136 deletions(-) diff --git a/src/article-api/lib/get-all-toc-items.ts b/src/article-api/lib/get-all-toc-items.ts index bf3e94824931..985bb2667c1c 100644 --- a/src/article-api/lib/get-all-toc-items.ts +++ b/src/article-api/lib/get-all-toc-items.ts @@ -19,11 +19,6 @@ interface TocItem extends LinkData { * Recursively gathers all TOC items from a page and its descendants. * This mirrors the behavior of getTocItems() in the generic-toc middleware * but works with the page.children frontmatter property. - * - * @param page - The page to gather TOC items from - * @param context - The rendering context - * @param options - Configuration options - * @returns Array of TocItems with nested childTocItems */ export async function getAllTocItems( page: Page, @@ -41,7 +36,6 @@ export async function getAllTocItems( return [] } - // Get the page's pathname for resolving children const pagePermalink = page.permalinks.find( (p) => p.languageCode === languageCode && p.pageVersion === context.currentVersion, ) @@ -92,10 +86,6 @@ export async function getAllTocItems( /** * Flattens nested TOC items into a single array. * Only includes leaf nodes (items without children) or all items based on options. - * - * @param tocItems - The nested TOC items to flatten - * @param options - Configuration options - * @returns Flat array of LinkData items */ export function flattenTocItems( tocItems: TocItem[], @@ -125,7 +115,6 @@ export function flattenTocItems( } } - // Recurse into children if (hasChildren) { recurse(item.childTocItems!) } diff --git a/src/article-api/lib/get-link-data.ts b/src/article-api/lib/get-link-data.ts index f9c33c74608e..45749ec06a0e 100644 --- a/src/article-api/lib/get-link-data.ts +++ b/src/article-api/lib/get-link-data.ts @@ -5,15 +5,9 @@ import type { LinkData } from '@/article-api/transformers/types' * Resolves link data (title, href, intro) for a given href and page * * This helper is used by landing page transformers to build link lists. - * It resolves the page from an href, renders its title and intro, and + * It resolves the page from an href (relative or absolute), renders its title + * and intro, and * returns the canonical permalink. - * - * @param href - The href to resolve (can be relative or absolute) - * @param languageCode - The language code for the current page - * @param pathname - The current page's pathname (for relative resolution) - * @param context - The rendering context - * @param resolvePath - Function to resolve an href to a Page object - * @returns LinkData with resolved title, href, and optional intro */ export async function getLinkData( href: string, diff --git a/src/article-api/lib/graphql-helpers.ts b/src/article-api/lib/graphql-helpers.ts index 6db482d7ff06..dc07ef55ca18 100644 --- a/src/article-api/lib/graphql-helpers.ts +++ b/src/article-api/lib/graphql-helpers.ts @@ -2,10 +2,7 @@ import type { Context, Page } from '@/types' import { renderContent } from '@/content-render/index' import matter from '@gr2m/gray-matter' -/** - * Extract manual content from page markdown - * Used by GraphQL transformers to get content before the auto-generated marker - */ +// Returns the part of the page markdown before the auto-generated marker. export async function extractManualContent(page: Page, context: Context): Promise { if (!page.markdown) return '' diff --git a/src/article-api/lib/load-template.ts b/src/article-api/lib/load-template.ts index 2cf169ba791b..3e17398756ed 100644 --- a/src/article-api/lib/load-template.ts +++ b/src/article-api/lib/load-template.ts @@ -2,30 +2,12 @@ import { readFileSync } from 'fs' import { join, dirname } from 'path' import { fileURLToPath } from 'url' -// Get the directory path for the transformers directory -// This will be used to resolve template paths relative to transformers const __filename = fileURLToPath(import.meta.url) const __dirname = dirname(__filename) -/** - * Load a template file from the templates directory - * - * This helper loads Liquid template files used by transformers. - * Templates are located in src/article-api/templates/ - * - * @param templateName - The name of the template file (e.g., 'landing-page.template.md') - * @returns The template content as a string - * - * @example - * ```typescript - * const template = loadTemplate('landing-page.template.md') - * const rendered = await renderContent(template, context) - * ``` - */ +// Loads a Liquid template file from src/article-api/templates, for use by +// transformers. export function loadTemplate(templateName: string): string { - // Templates are in ../templates relative to the lib directory - // lib is at src/article-api/lib - // templates is at src/article-api/templates const templatePath = join(__dirname, '../templates', templateName) return readFileSync(templatePath, 'utf8') } diff --git a/src/article-api/lib/strip-html-comments.ts b/src/article-api/lib/strip-html-comments.ts index 354bef9b0b63..1f15e67fc084 100644 --- a/src/article-api/lib/strip-html-comments.ts +++ b/src/article-api/lib/strip-html-comments.ts @@ -1,27 +1,10 @@ -/** - * Strips HTML comments from markdown content. - * Removes single-line HTML comments like - * while preserving other content. - * - * @param content - The markdown content to process - * @returns The content with HTML comments removed - */ +// Removes single-line HTML comments such as . export function stripHtmlComments(content: string): string { - // Remove single-line HTML comments () - // This matches comments that are on their own line or inline return content.replace(//g, '').trim() } -/** - * Strips HTML comments and cleans up extra blank lines. - * Useful for cleaning up rendered markdown content where HTML comments - * were on separate lines and their removal creates gaps. - * - * @param content - The markdown content to process - * @returns The content with HTML comments removed and blank lines normalized - */ +// Strips HTML comments, then collapses the blank lines their removal leaves behind. export function stripHtmlCommentsAndNormalizeWhitespace(content: string): string { - // Remove HTML comments let cleaned = stripHtmlComments(content) // Normalize multiple consecutive blank lines to at most 2 blank lines diff --git a/src/article-api/lib/summarize-schema.ts b/src/article-api/lib/summarize-schema.ts index 8ae39784399b..57f0276d6bd2 100644 --- a/src/article-api/lib/summarize-schema.ts +++ b/src/article-api/lib/summarize-schema.ts @@ -194,14 +194,12 @@ export function summarizeSchema(schema: JsonSchema): string { // compact and fast to render (shared types can recur dozens of times). const seen = new Set() - // Handle top-level composition for (const keyword of ['oneOf', 'anyOf', 'allOf'] as const) { if (schema[keyword]) { return renderCompositionVariants(keyword, schema[keyword]!, 0, 0, seen) } } - // Handle top-level array const schemaTypes = Array.isArray(schema.type) ? schema.type : schema.type ? [schema.type] : [] const isNullable = schemaTypes.includes('null') const primaryType = schemaTypes.find((t) => t !== 'null') @@ -215,7 +213,6 @@ export function summarizeSchema(schema: JsonSchema): string { const constraintStr = constraints.length ? ` (${constraints.join(', ')})` : '' const itemTitle = items.title - // Composition inside items const compositionKey = (['oneOf', 'anyOf', 'allOf'] as const).find((k) => items[k]) if (compositionKey) { const label = compositionKey.replace('Of', ' of') @@ -248,7 +245,6 @@ export function summarizeSchema(schema: JsonSchema): string { return `Array${constraintStr} of ${renderTypeConstraints(items)}${isNullable ? ' or null' : ''}` } - // Handle top-level object if (schema.properties) { // Note: we deliberately do NOT pre-mark schema.title here. Unlike the // array-items case above, a top-level object emits no visible titled diff --git a/src/article-api/liquid-renderers/index.ts b/src/article-api/liquid-renderers/index.ts index b25734dbad69..7b2739dec3ea 100644 --- a/src/article-api/liquid-renderers/index.ts +++ b/src/article-api/liquid-renderers/index.ts @@ -1,16 +1,9 @@ -/** - * API Transformer Liquid Tags - * - * This module contains custom Liquid tags used by article-api transformers - * to render API documentation in a consistent format. - */ +// Custom Liquid tags used by article-api transformers. import { restTags } from './rest-tags' -// Export all API transformer tags for registration export const apiTransformerTags = { ...restTags, } -// Re-export individual tag modules for direct access if needed export { restTags } from './rest-tags' diff --git a/src/article-api/liquid-renderers/rest-tags.ts b/src/article-api/liquid-renderers/rest-tags.ts index 63b3ee52cd6f..e8121e40c9c2 100644 --- a/src/article-api/liquid-renderers/rest-tags.ts +++ b/src/article-api/liquid-renderers/rest-tags.ts @@ -7,10 +7,7 @@ import { createLogger } from '@/observability/logger' const logger = createLogger('article-api/liquid-renderers/rest-tags') -/** - * Custom Liquid tag for rendering REST API parameters - * Usage: {% rest_parameter param %} - */ +// Usage: {% rest_parameter param %} export class RestParameter { private paramName: string @@ -56,10 +53,7 @@ export class RestParameter { } } -/** - * Custom Liquid tag for rendering REST API body parameters - * Usage: {% rest_body_parameter param indent %} - */ +// Usage: {% rest_body_parameter param indent %} export class RestBodyParameter { constructor( token: TagToken, @@ -67,7 +61,6 @@ export class RestBodyParameter { liquid: Liquid, private liquidContext?: LiquidContext, ) { - // Parse arguments - param name and optional indent level const args = token.args.trim().split(/\s+/) this.param = args[0] this.indent = args[1] ? parseInt(args[1]) : 0 @@ -106,7 +99,6 @@ export class RestBodyParameter { lines.push(`${prefix} Can be one of: ${param.enum.map((v) => `\`${v}\``).join(', ')}`) } - // Handle nested parameters if (param.childParamsGroups && param.childParamsGroups.length > 0) { for (const childGroup of param.childParamsGroups) { lines.push(await renderChildParameter(childGroup, context, indent + 1)) @@ -117,10 +109,7 @@ export class RestBodyParameter { } } -/** - * Custom Liquid tag for rendering REST API status codes - * Usage: {% rest_status_code statusCode %} - */ +// Usage: {% rest_status_code statusCode %} export class RestStatusCode { private statusCodeName: string @@ -158,9 +147,6 @@ export class RestStatusCode { } } -/** - * Helper function to render child parameters recursively - */ async function renderChildParameter( param: ChildParameter, context: Context, @@ -186,7 +172,6 @@ async function renderChildParameter( lines.push(`${prefix} Can be one of: ${param.enum.map((v: string) => `\`${v}\``).join(', ')}`) } - // Recursively handle nested parameters if (param.childParamsGroups && param.childParamsGroups.length > 0) { for (const child of param.childParamsGroups) { lines.push(await renderChildParameter(child, context, indent + 1)) @@ -196,9 +181,6 @@ async function renderChildParameter( return lines.join('\n') } -/** - * Helper function to convert HTML to markdown - */ async function htmlToMarkdown(html: string, context: Context): Promise { if (!html) return '' @@ -215,12 +197,10 @@ async function htmlToMarkdown(html: string, context: Context): Promise { if (process.env.NODE_ENV !== 'production') { throw error } - // Fallback to simple text extraction return fastTextOnly(html) } } -// Export tag names for registration export const restTags = { rest_parameter: RestParameter, rest_body_parameter: RestBodyParameter, diff --git a/src/article-api/middleware/article-body.ts b/src/article-api/middleware/article-body.ts index cae5e7f003b8..ce0c6717c341 100644 --- a/src/article-api/middleware/article-body.ts +++ b/src/article-api/middleware/article-body.ts @@ -11,10 +11,7 @@ import { normalizeRenderedMarkdown } from '@/article-api/lib/normalize-markdown' import { allVersions } from '@/versions/lib/all-versions' import type { Page } from '@/types' -/** - * Creates a mocked rendering request and contextualizes it. - * This is used to prepare a request for rendering pages in markdown format. - */ +// Creates a mocked rendering request, contextualized for rendering a page as markdown. async function createContextualizedRenderingRequest(pathname: string, page: Page) { const mockedContext: Context = {} const renderingReq = { @@ -52,7 +49,6 @@ export async function getArticleBody(req: ExtendedRequestWithPageInfo) { if (archived?.isArchived) throw new Error(`Page ${pathname} is archived and can't be rendered in markdown.`) - // Extract apiVersion from query params if provided const apiVersion = req.query.apiVersion as string | undefined // With the catch-all ArticleTransformer registered last, @@ -60,7 +56,6 @@ export async function getArticleBody(req: ExtendedRequestWithPageInfo) { const transformer = transformerRegistry.findTransformer(page) if (!transformer) throw new Error(`No transformer found for page: ${pathname}`) - // Use the transformer const renderingReq = await createContextualizedRenderingRequest(pathname, page) // Determine the API version to use (provided or latest) @@ -68,7 +63,6 @@ export async function getArticleBody(req: ExtendedRequestWithPageInfo) { const currentVersion = renderingReq.context.currentVersion let effectiveApiVersion = apiVersion - // Use latest version if not provided if (!effectiveApiVersion && currentVersion && allVersions[currentVersion]) { effectiveApiVersion = allVersions[currentVersion].latestApiVersion || undefined } diff --git a/src/article-api/middleware/pagelist.ts b/src/article-api/middleware/pagelist.ts index 295a18c49acf..a80978c481c4 100644 --- a/src/article-api/middleware/pagelist.ts +++ b/src/article-api/middleware/pagelist.ts @@ -35,15 +35,12 @@ router.get( defaultCacheControl(res) const response = { - // Simple list of all version strings versions: allVersionKeys, - // GHES-specific information ghesVersions: enterpriseServerReleases.supported, ghesLatest: enterpriseServerReleases.latest, ghesLatestStable: enterpriseServerReleases.latestStable, ghesReleaseCandidate: enterpriseServerReleases.releaseCandidate, ghesDeprecated: enterpriseServerReleases.deprecated, - // Full version details allVersions, } @@ -77,9 +74,7 @@ router.get( ) const response = { - // Simple list of language codes languages: languageKeys, - // Full language details (without redirectPatterns) allLanguages: sanitizedLanguages, } @@ -142,10 +137,8 @@ router.get( const pages = req.context.pages - // the keys of `context.pages` are permalinks const keys = Object.keys(pages) - // we filter the permalinks to get only our target version and language const filteredPermalinks = keys.filter((key) => versionMatcher(key, req.context!.currentVersion!, req.context!.currentLanguage!), ) diff --git a/src/article-api/middleware/validation.ts b/src/article-api/middleware/validation.ts index 031b60704e49..eff2c9c842da 100644 --- a/src/article-api/middleware/validation.ts +++ b/src/article-api/middleware/validation.ts @@ -16,23 +16,19 @@ export const pagelistValidationMiddleware = ( res: Response, next: NextFunction, ) => { - // get version from path, fallback to default version if it can't be resolved const versionFromPath = getVersionStringFromPath(req.path) || nonEnterpriseDefaultVersion - // in the rare case that this failed, probably won't be reached + // Defensive: getVersionStringFromPath falls back above, so this is unreachable. if (!versionFromPath) return res.status(400).json({ error: `Couldn't get version from the given path.` }) - // get the language from path, fallback to english if it can't be resolved const langFromPath = getLangFromPath(req.path) || 'en' - // in the rare case that the language fallback failed if (!langFromPath) return res.status(400).json({ error: `Couldn't get language from the from the given path.`, }) - // set the version and language in the context, we'll use it later req.context!.currentVersion = versionFromPath req.context!.currentLanguage = langFromPath return next() @@ -96,7 +92,7 @@ export const pageValidationMiddleware = ( pathname = `/${req.context.currentLanguage}` } - // Initialize archived property to avoid it being undefined + // Initialized so downstream readers never see undefined. req.pageinfo.archived = { isArchived: false } if (!(pathname in req.context.pages)) { @@ -133,24 +129,20 @@ export const apiVersionValidationMiddleware = ( ) => { const apiVersion = req.query.apiVersion as string | string[] | undefined - // If no apiVersion is provided, continue (it will default to latest) if (!apiVersion) { return next() } - // Validate apiVersion is a single string, not an array if (Array.isArray(apiVersion)) { return res.status(400).json({ error: "Multiple 'apiVersion' keys" }) } - // Get the version from the pathname query parameter const pathname = req.pageinfo?.pathname || (req.query.pathname as string) if (!pathname) { // This should not happen as pathValidationMiddleware runs first throw new Error('pathname not available for apiVersion validation') } - // Extract version from the pathname const currentVersion = getVersionStringFromPath(pathname) || nonEnterpriseDefaultVersion const versionInfo = allVersions[currentVersion] @@ -160,7 +152,6 @@ export const apiVersionValidationMiddleware = ( const validApiVersions = versionInfo.apiVersions || [] - // If this version has API versioning, validate the provided version if (validApiVersions.length > 0 && !validApiVersions.includes(apiVersion)) { return res.status(400).json({ error: `Invalid apiVersion '${apiVersion}' for ${currentVersion}. Valid API versions are: ${validApiVersions.join(', ')}`, diff --git a/src/article-api/scripts/generate-api-docs.ts b/src/article-api/scripts/generate-api-docs.ts index ba1b64c5f83d..081a72799d09 100644 --- a/src/article-api/scripts/generate-api-docs.ts +++ b/src/article-api/scripts/generate-api-docs.ts @@ -11,23 +11,18 @@ type ApiDoc = { } function main({ sources, outputPath }: { sources: string[]; outputPath: string }): void { - // Extract API documentation comments from all source files const allDocs = sources.flatMap((sourcePath) => extractApiDocs(sourcePath)) - // Generate markdown const markdown = generateMarkdown(allDocs) - // Update README updateReadme(outputPath, markdown) console.log('API documentation generated successfully!') } -// Extract API docs from comments in the file function extractApiDocs(file: string): ApiDoc[] { const apiDocs: ApiDoc[] = [] - // get the content from the api routes const content = readFileSync(file, 'utf8') // Get the router method definitions with JSDOC-style comments @@ -40,15 +35,13 @@ function extractApiDocs(file: string): ApiDoc[] { const method = match[2] const path = match[3] - // The description is first line of the comment const description = commentBlock .trim() .split('\n')[0] .trim() .replace(/^\*\s*/, '') - // Grab the other elements from the comment - // we currently support: params, returns, examples, throws + // We currently support params, returns, examples, and throws. const params = extractParams(commentBlock) const returns = extractReturns(commentBlock) const examples = extractExample(commentBlock) @@ -82,7 +75,6 @@ function extractThrows(commentBlock: string): string[] { return throws } -// Extract parameters from comment block function extractParams(commentBlock: string): string[] { const paramRegex = /@param\s+{([^}]+)}\s+([^\s]+)\s+([^\n]+)/g const params: string[] = [] @@ -98,7 +90,6 @@ function extractParams(commentBlock: string): string[] { return params } -// Extract return info from comment block function extractReturns(commentBlock: string): string { const returnMatch = commentBlock.match(/@returns\s+{([^}]+)}\s+([^\n]+)/) if (returnMatch) { @@ -109,7 +100,6 @@ function extractReturns(commentBlock: string): string { return '' } -// Extract example from comment block function extractExample(commentBlock: string): string { const exampleMatch = commentBlock.match(/@example\b([\s\S]*?)(?=\s*\*\s*@|\s*\*\/|$)/) if (exampleMatch) { @@ -123,7 +113,6 @@ function extractExample(commentBlock: string): string { return '' } -// Generate markdown from parsed documentation function generateMarkdown(apiDocs: ApiDoc[]): string { let markdown = '## Reference: API endpoints\n\n' @@ -157,14 +146,12 @@ function generateMarkdown(apiDocs: ApiDoc[]): string { return markdown } -// Update README with generated documentation function updateReadme(readmePath: string, markdown: string): void { if (existsSync(readmePath)) { let readme = readFileSync(readmePath, 'utf8') const placeholderComment = `` - // Replace API documentation section, or append to end if (readme.includes(placeholderComment)) { const pattern = new RegExp(`${placeholderComment}[\\s\\S]*`, 'g') readme = readme.replace(pattern, `${placeholderComment}\n${markdown}`) From 140d7958269d0209f33ed65196291abd48d1580e Mon Sep 17 00:00:00 2001 From: Kevin Heis Date: Tue, 15 Sep 2026 20:16:36 +0000 Subject: [PATCH 02/16] Trim excessive comments in src/content-render/scripts (#63277) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 17588eeb-788a-4f82-9d36-b5079fb36521 --- .../scripts/add-content-type.ts | 4 - src/content-render/scripts/cta-builder.ts | 41 +--------- src/content-render/scripts/liquid-tags.ts | 78 +------------------ .../scripts/move-by-content-type.ts | 45 +---------- src/content-render/scripts/move-content.ts | 4 +- .../reusables-cli/find/potential-uses.ts | 2 - .../scripts/reusables-cli/find/used.ts | 1 - .../scripts/reusables-cli/shared.ts | 3 - .../scripts/update-filepaths.ts | 16 +--- 9 files changed, 7 insertions(+), 187 deletions(-) diff --git a/src/content-render/scripts/add-content-type.ts b/src/content-render/scripts/add-content-type.ts index 588b484bdaf0..f6286ea7b4a2 100644 --- a/src/content-render/scripts/add-content-type.ts +++ b/src/content-render/scripts/add-content-type.ts @@ -107,7 +107,6 @@ function processFile(filePath: string, scriptOptions: ScriptOptions) { return { processed: true, updated: false } } - // Check if we're actually changing an existing contentType const isChangingContentType = data.contentType && data.contentType !== newContentType const isAddingContentType = !data.contentType @@ -119,7 +118,6 @@ function processFile(filePath: string, scriptOptions: ScriptOptions) { console.log(`Adding contentType '${newContentType}' on ${relativePath}`) } - // Only update if there's actually a change needed if (isChangingContentType || isAddingContentType) { data.contentType = newContentType } else { @@ -127,7 +125,6 @@ function processFile(filePath: string, scriptOptions: ScriptOptions) { return { processed: true, updated: false } } - // Write the file back fs.writeFileSync( filePath, frontmatter.stringify( @@ -183,7 +180,6 @@ function determineContentType(relativePath: string): string { return LANDING_TYPE } - // Classify anything else as 'other'. return OTHER_TYPE } diff --git a/src/content-render/scripts/cta-builder.ts b/src/content-render/scripts/cta-builder.ts index bd633c63dbb6..96ca90b65f00 100644 --- a/src/content-render/scripts/cta-builder.ts +++ b/src/content-render/scripts/cta-builder.ts @@ -31,7 +31,6 @@ type CTASchemaProperties = { [K in keyof CTAParams]-?: CTASchemaProperty } -// Conversion mappings from old CTA format to new schema const ctaToTypeMapping: Record = { 'GHEC trial': 'trial', 'Copilot trial': 'trial', @@ -51,18 +50,15 @@ const ctaToPlanMapping: Record = { 'GHEC trial': 'enterprise', } -// Keywords that suggest a button context vs inline text link const buttonKeywords = ['landing', 'signup', 'download', 'trial'] const program = new Command() -// CLI setup program .name('cta-builder') .description('Create a properly formatted Call-to-Action URL with tracking parameters.') .version('1.0.0') -// Add conversion command program .command('convert') .description('Convert old CTA URLs to new schema format') @@ -72,7 +68,6 @@ program convertUrls(options) }) -// Add validation command program .command('validate') .description('Validate a CTA URL against the schema') @@ -81,7 +76,6 @@ program validateUrl(options) }) -// Add programmatic build command program .command('build') .description('Build a CTA URL programmatically with flags (outputs URL only)') @@ -94,7 +88,6 @@ program buildProgrammaticCTA(options) }) -// Default to interactive mode program.action(() => { interactiveBuilder() }) @@ -104,7 +97,6 @@ if (import.meta.url === `file://${process.argv[1]}`) { program.parse() } -// Helper function to select from lettered options async function selectFromOptions( paramName: string, message: string, @@ -139,7 +131,6 @@ async function selectFromOptions( } } -// Helper function to confirm yes/no async function confirmChoice( message: string, promptFn: (question: string) => Promise, @@ -161,7 +152,6 @@ async function confirmChoice( } } -// Extract CTA parameters from a URL function extractCTAParams(url: string): CTAParams { const urlObj = new URL(url) const ctaParams: CTAParams = {} @@ -224,7 +214,6 @@ function validateCTAParams(params: CTAParams): { isValid: boolean; errors: strin } } -// Build URL with CTA parameters function buildCTAUrl(baseUrl: string, params: CTAParams): string { const url = new URL(baseUrl) @@ -237,17 +226,15 @@ function buildCTAUrl(baseUrl: string, params: CTAParams): string { return url.toString() } -// Convert old CTA URL to new schema format export function convertOldCTAUrl(oldUrl: string): { newUrl: string; notes: string[] } { const notes: string[] = [] try { const url = new URL(oldUrl) - // Build new parameters const newParams: CTAParams = {} - // First, check if any of the new params already exist, and preserve those if so + // Preserve any new-style params that are already on the URL. for (const [key, value] of url.searchParams.entries()) { for (const param of Object.keys(ctaSchema.properties)) { if (key === param && key in ctaSchema.properties) { @@ -264,17 +251,14 @@ export function convertOldCTAUrl(oldUrl: string): { newUrl: string; notes: strin } } - // Try to convert old params to new params const refCta = url.searchParams.get('ref_cta') || '' const refLoc = url.searchParams.get('ref_loc') || '' - // Map ref_product if (!newParams.ref_product) { newParams.ref_product = inferProductFromUrl(oldUrl, refCta) notes.push(`- Missing ref_product - made an inference, manually update if needed`) } - // Map ref_type if (!newParams.ref_type) { newParams.ref_type = ctaToTypeMapping[refCta] || 'engagement' if (!ctaToTypeMapping[refCta]) { @@ -282,13 +266,11 @@ export function convertOldCTAUrl(oldUrl: string): { newUrl: string; notes: strin } } - // Map ref_style if (!newParams.ref_style) { newParams.ref_style = inferStyleFromContext(refLoc) notes.push(`- Missing ref_style - made an inference, manually update if needed`) } - // Map ref_plan (optional) if (!newParams.ref_plan) { if (ctaToPlanMapping[refCta]) { newParams.ref_plan = ctaToPlanMapping[refCta] @@ -298,12 +280,10 @@ export function convertOldCTAUrl(oldUrl: string): { newUrl: string; notes: strin // Build new URL - preserve all existing parameters except old ref_ parameters const newUrl = new URL(url.toString()) - // Remove old CTA parameters newUrl.searchParams.delete('ref_cta') newUrl.searchParams.delete('ref_loc') newUrl.searchParams.delete('ref_page') - // Add new CTA parameters for (const [key, value] of Object.entries(newParams)) { if (value) { newUrl.searchParams.set(key, value) @@ -343,7 +323,6 @@ function inferProductFromUrl(url: string, refCta: string): string { } catch { // Fallback if url isn't valid: leave hostname empty } - // Strict hostname check for desktop.github.com if (hostname === 'desktop.github.com' || refCta.includes('desktop')) { return 'desktop' } @@ -361,7 +340,6 @@ function inferProductFromUrl(url: string, refCta: string): string { ) { return 'ghec' } - // Default fallback return 'copilot' } @@ -372,15 +350,12 @@ function inferStyleFromContext(refLoc: string): string { return isButton ? 'button' : 'text' } -// Interactive CTA builder async function interactiveBuilder(): Promise { - // Create readline interface for interactive mode const rl = readline.createInterface({ input: process.stdin, output: process.stdout, }) - // Helper function to prompt user (scoped to this function) function prompt(question: string): Promise { return new Promise((resolve) => { rl.question(question, (answer) => { @@ -392,7 +367,6 @@ async function interactiveBuilder(): Promise { try { console.log(chalk.blue.bold('šŸš€ Guided CTA URL builder\n')) - // Get base URL with validation let baseUrl = '' while (!baseUrl) { const input = await prompt('Enter the base URL (e.g., https://github.com/features/copilot): ') @@ -406,7 +380,6 @@ async function interactiveBuilder(): Promise { const params: CTAParams = {} - // Required parameters console.log(chalk.white(`\nRequired parameters:`)) const schemaProps = ctaSchema.properties as CTASchemaProperties @@ -442,7 +415,6 @@ async function interactiveBuilder(): Promise { } } - // Validate parameters const validation = validateCTAParams(params) if (!validation.isValid) { @@ -454,7 +426,6 @@ async function interactiveBuilder(): Promise { return } - // Build and display URL const ctaUrl = buildCTAUrl(baseUrl, params) console.log(chalk.green('\nāœ… CTA URL generated successfully!')) @@ -477,7 +448,6 @@ async function interactiveBuilder(): Promise { } } -// Convert URLs command handler async function convertUrls(options: { url?: string; quiet?: boolean }): Promise { try { if (!options.quiet) { @@ -540,7 +510,6 @@ async function convertUrls(options: { url?: string; quiet?: boolean }): Promise< // The convert command doesn't use readline, so script should exit naturally } -// Validate URLs command handler async function validateUrl(options: { url?: string }): Promise { try { console.log(chalk.blue.bold('CTA URL validator')) @@ -549,7 +518,6 @@ async function validateUrl(options: { url?: string }): Promise { console.log(chalk.white('\nValidating URL:')) console.log(chalk.gray(options.url)) - // Extract CTA parameters from URL let ctaParams: CTAParams try { ctaParams = extractCTAParams(options.url) @@ -558,7 +526,6 @@ async function validateUrl(options: { url?: string }): Promise { return } - // Check if URL has any CTA parameters if (Object.keys(ctaParams).length === 0) { console.log(chalk.yellow('\nā„¹ļø No CTA parameters found in URL')) return @@ -598,7 +565,6 @@ async function validateUrl(options: { url?: string }): Promise { } } -// Programmatic build command handler async function buildProgrammaticCTA(options: { url: string product: string @@ -607,7 +573,6 @@ async function buildProgrammaticCTA(options: { plan?: string }): Promise { try { - // Validate base URL let baseUrl: string try { baseUrl = new URL(options.url).toString() @@ -618,19 +583,16 @@ async function buildProgrammaticCTA(options: { process.exit(1) } - // Build CTA parameters object const params: CTAParams = { ref_product: options.product, ref_type: options.type, ref_style: options.style, } - // Add optional parameters if (options.plan) { params.ref_plan = options.plan } - // Validate parameters against schema const validation = validateCTAParams(params) if (!validation.isValid) { // Output validation errors to stderr and exit with error code @@ -640,7 +602,6 @@ async function buildProgrammaticCTA(options: { process.exit(1) } - // Build and output the URL (stdout only) const ctaUrl = buildCTAUrl(baseUrl, params) console.log(ctaUrl) } catch (error) { diff --git a/src/content-render/scripts/liquid-tags.ts b/src/content-render/scripts/liquid-tags.ts index e15a3e3ab6f4..e24fdf6cfc73 100644 --- a/src/content-render/scripts/liquid-tags.ts +++ b/src/content-render/scripts/liquid-tags.ts @@ -11,7 +11,6 @@ import path from 'path' import { load, dump } from 'js-yaml' import chalk from 'chalk' -// Type definitions interface ExpandOptions { paths: string[] verbose?: boolean @@ -30,7 +29,6 @@ interface LiquidReference { endIndex: number } -// Constants const ROOT = process.env.ROOT || '.' const DATA_ROOT = path.resolve(path.join(ROOT, 'data')) const REUSABLES_ROOT = path.join(DATA_ROOT, 'reusables') @@ -43,14 +41,9 @@ function getErrorMessage(error: unknown): string { // Regex pattern to match expanded content blocks const EXPANDED_PATTERN = /(.+?)/gs -/** - * Get the file path for a data reference - * - * Validates and normalizes the incoming dataPath to prevent path traversal - * and ensure the final resolved path remains within the expected root. - */ +// Validates and normalizes the incoming dataPath to prevent path traversal +// and ensure the final resolved path remains within the expected root. function getDataFilePath(type: 'reusable' | 'variable', dataPath: string): string { - // Basic validation of the raw dataPath if (path.isAbsolute(dataPath)) { throw new Error(`Invalid ${type} data path: absolute paths are not allowed: ${dataPath}`) } @@ -89,9 +82,6 @@ function getDataFilePath(type: 'reusable' | 'variable', dataPath: string): strin } } -/** - * Convert a file path back to data path format (for consistent verbose output) - */ function convertFilePathToDataPath(filePath: string): string { const normalizedPath = path.normalize(filePath) @@ -134,9 +124,6 @@ program program.parse() -/** - * Get allowed types based on command options - */ function getAllowedTypes(options: ExpandOptions): Array<'reusable' | 'variable'> { if (options.reusablesOnly && options.variablesOnly) { console.log( @@ -155,13 +142,9 @@ function getAllowedTypes(options: ExpandOptions): Array<'reusable' | 'variable'> return ['variable'] } - // Default: process both types return ['reusable', 'variable'] } -/** - * Expand Liquid data references in content files - */ async function expandReferences(options: ExpandOptions): Promise { const { paths, verbose, markers, shallow } = options // markers will be true by default, false when --no-markers is used @@ -217,7 +200,6 @@ async function expandReferences(options: ExpandOptions): Promise { } } - // Check for remaining references const remainingRefs = findLiquidReferences(expandedContent, allowedTypes) hasRemainingRefs = remainingRefs.length > 0 @@ -278,9 +260,6 @@ async function expandReferences(options: ExpandOptions): Promise { } } -/** - * Restore content by restoring original Liquid statements from HTML comments - */ async function restoreReferences(options: ExpandOptions): Promise { const { paths, verbose } = options const allowedTypes = getAllowedTypes(options) @@ -306,7 +285,6 @@ async function restoreReferences(options: ExpandOptions): Promise { const content = fs.readFileSync(filePath, 'utf-8') - // Check for content edits before restoring const hasEdits = await detectContentEdits(content, verbose, allowedTypes) if (hasEdits) { console.log( @@ -361,9 +339,6 @@ async function restoreReferences(options: ExpandOptions): Promise { } } -/** - * Expand all Liquid data references in file content - */ async function expandFileContent( content: string, filePath: string, @@ -421,14 +396,10 @@ async function expandFileContent( } } - // Note: Remaining reference detection is now handled in expandReferences function for recursive mode - + // expandReferences handles remaining-reference detection in recursive mode. return expandedContent } -/** - * Detect if expanded content has been edited by comparing with original data - */ async function detectContentEdits( content: string, verbose?: boolean, @@ -441,7 +412,6 @@ async function detectContentEdits( const [, type, dataPath, resolvedContent] = match const refType = type as 'reusable' | 'variable' - // Only check if this type is allowed if (!allowedTypes || allowedTypes.includes(refType)) { try { // Load the original content from data files @@ -478,9 +448,6 @@ async function detectContentEdits( return hasEdits } -/** - * Load data value from file system (helper for edit detection) - */ function loadDataValue(type: 'reusable' | 'variable', dataPath: string): string | null { try { const targetPath = getDataFilePath(type, dataPath) @@ -498,7 +465,6 @@ function loadDataValue(type: 'reusable' | 'variable', dataPath: string): string const yamlContent = fs.readFileSync(targetPath, 'utf8') const data = load(yamlContent) as Record - // Navigate to the nested property const pathParts = dataPath.split('.') let current: unknown = data for (let i = 1; i < pathParts.length; i++) { @@ -517,9 +483,6 @@ function loadDataValue(type: 'reusable' | 'variable', dataPath: string): string return null } -/** - * Restore content by restoring original Liquid statements - */ function restoreFileContent( content: string, verbose?: boolean, @@ -528,7 +491,6 @@ function restoreFileContent( return content.replace(EXPANDED_PATTERN, (match, type, dataPath) => { const refType = type as 'reusable' | 'variable' - // Only restore if this type is allowed if (!allowedTypes || allowedTypes.includes(refType)) { const originalLiquid = `{% data ${type}s.${dataPath} %}` @@ -539,15 +501,10 @@ function restoreFileContent( return originalLiquid } - // Return unchanged if type is not allowed return match }) } -/** - * Update data files with content from expanded blocks - * Returns array of file paths that were updated - */ function updateDataFiles( filePath: string, verbose?: boolean, @@ -564,7 +521,6 @@ function updateDataFiles( return [] } - // Group updates by file path const updatesByFile = new Map() for (const update of updates) { const key = `${update.type}:${update.path}` @@ -576,7 +532,6 @@ function updateDataFiles( const updatedFiles: string[] = [] - // Apply updates to each data file for (const [key, contents] of updatesByFile) { const [type, dataPath] = key.split(':') const targetFilePath = applyDataUpdates( @@ -594,9 +549,6 @@ function updateDataFiles( return updatedFiles } -/** - * Extract data updates from expanded content blocks - */ function extractDataUpdates( content: string, allowedTypes?: Array<'reusable' | 'variable'>, @@ -608,13 +560,11 @@ function extractDataUpdates( const [, type, dataPath, resolvedContent] = match const refType = type as 'reusable' | 'variable' - // Only include if this type is allowed if (!allowedTypes || allowedTypes.includes(refType)) { // Check if this content was actually changed before including it try { const originalContent = loadDataValue(refType, dataPath.trim()) if (originalContent !== null && resolvedContent.trim() !== originalContent.trim()) { - // Only add to updates if content was actually changed updates.push({ type: refType, path: dataPath.trim(), @@ -635,10 +585,6 @@ function extractDataUpdates( return updates } -/** - * Apply updates to a specific data file - * Returns the file path if file was updated, null otherwise - */ function applyDataUpdates( type: 'reusable' | 'variable', dataPath: string, @@ -648,7 +594,6 @@ function applyDataUpdates( ): string | null { const targetPath = getDataFilePath(type, dataPath) - // Check if file exists if (!fs.existsSync(targetPath)) { if (verbose) { console.log(chalk.red(` Error: Data file not found: ${targetPath}`)) @@ -701,7 +646,6 @@ function applyDataUpdates( const yamlContent = fs.readFileSync(targetPath, 'utf8') const data = load(yamlContent) as Record - // Navigate to the nested property const pathParts = dataPath.split('.') const propertyPath = pathParts.slice(1) // Skip the file name @@ -713,7 +657,6 @@ function applyDataUpdates( current = current[propertyPath[i]] as Record } - // Update the final property const finalKey = propertyPath[propertyPath.length - 1] if (contents.length > 1) { console.log( @@ -728,7 +671,6 @@ function applyDataUpdates( const finalYaml = hasTrailingNewline && !yamlOutput.endsWith('\n') ? `${yamlOutput}\n` : yamlOutput - // Write back to file fs.writeFileSync(targetPath, finalYaml) if (verbose) { console.log(chalk.green(` Updated: ${type}s.${dataPath}`)) @@ -743,9 +685,6 @@ function applyDataUpdates( } } -/** - * Find all Liquid data references in content - */ function findLiquidReferences( content: string, allowedTypes?: Array<'reusable' | 'variable'>, @@ -761,7 +700,6 @@ function findLiquidReferences( const [original, type, dataPath] = match const refType = type.slice(0, -1) as 'reusable' | 'variable' // Remove 's' from end - // Only include if this type is allowed if (types.includes(refType)) { references.push({ original, @@ -776,9 +714,6 @@ function findLiquidReferences( return references } -/** - * Resolve a single Liquid data reference to its content - */ async function resolveLiquidReference( ref: LiquidReference, verbose?: boolean, @@ -798,9 +733,6 @@ async function resolveLiquidReference( return null } -/** - * Resolve a reusable reference by reading the markdown file - */ async function resolveReusable(reusablePath: string, verbose?: boolean): Promise { const filePath = getDataFilePath('reusable', reusablePath) @@ -826,9 +758,6 @@ async function resolveReusable(reusablePath: string, verbose?: boolean): Promise } } -/** - * Resolve a variable reference by reading from YAML files - */ async function resolveVariable(variablePath: string, verbose?: boolean): Promise { const pathParts = variablePath.split('.') @@ -866,7 +795,6 @@ async function resolveVariable(variablePath: string, verbose?: boolean): Promise } } - // Convert value to string if (typeof value === 'string') { return value } else if (value !== null && value !== undefined) { diff --git a/src/content-render/scripts/move-by-content-type.ts b/src/content-render/scripts/move-by-content-type.ts index c64ef0ed27fc..e7773d92d881 100644 --- a/src/content-render/scripts/move-by-content-type.ts +++ b/src/content-render/scripts/move-by-content-type.ts @@ -26,7 +26,6 @@ const contentTypeToDir = (contentType: string): string => { const validContentTypeDirs = new Set(CONTENT_TYPES.map(contentTypeToDir)) -// Helper: Should we skip this index.md file from processing? function shouldSkipIndexFile(filePath: string): boolean { const relativePath = path.relative(process.cwd(), filePath) const parts = relativePath.split(path.sep) @@ -44,17 +43,14 @@ function shouldSkipIndexFile(filePath: string): boolean { return false } -// Helper: Calculate target directory for a file function calculateTarget(filePath: string, contentType: string, productDir: string) { const relativePath = path.relative(process.cwd(), filePath) const parts = relativePath.split(path.sep) const contentIndex = parts.indexOf('content') const fileName = path.basename(filePath) - // Determine target content-type directory const targetContentType = contentTypeToDir(contentType) - // Calculate target path if (targetContentType === 'how-tos') { // Preserve subdirectory structure for how-tos const pathAfterProduct = parts.slice(contentIndex + 2, -1) @@ -85,9 +81,7 @@ program .description('Reorganize content files into subdirectories based on their contentType property') .argument('[paths...]', 'Content paths to process') .action(async (paths: string[]) => { - // ==================== - // 1. GATHER FILES - // ==================== + // Gather files. const filesToProcess: string[] = [] if (paths?.length > 0) { for (const p of paths) { @@ -104,9 +98,6 @@ program console.log(chalk.white(`Processing ${filesToProcess.length} files...\n`)) - // ==================== - // 2. ANALYZE & PLAN MOVES - // ==================== console.log(chalk.white('Analyzing files...\n')) const filesToMove: FileMove[] = [] @@ -125,7 +116,6 @@ program continue } - // Read and validate contentType const fileContent = await fs.readFile(filePath, 'utf-8') const { data } = readFrontmatter(fileContent) @@ -145,7 +135,6 @@ program continue } - // Validate contentType if (!CONTENT_TYPES.includes(contentType)) { skipped.push({ file: relativePath, reason: `Invalid contentType: ${contentType}` }) console.log( @@ -154,7 +143,6 @@ program continue } - // Get product directory if (contentIndex === -1 || contentIndex + 1 >= parts.length) { console.log( chalk.yellow(`⚠ Skipping ${relativePath}: Cannot determine product directory`), @@ -168,13 +156,10 @@ program if (contentType === 'rai') productsWithRai.add(productName) - // Calculate target const { targetDir, targetPath } = calculateTarget(filePath, contentType, productDir) - // Skip if already in correct location if (path.dirname(filePath) === targetDir) continue - // Skip if target exists try { await fs.access(targetPath) skipped.push({ file: relativePath, reason: 'Target already exists' }) @@ -184,7 +169,6 @@ program // Good, doesn't exist } - // Track this move filesToMove.push({ filePath, targetDir, targetPath, contentType }) const relativeTargetDir = path.relative(process.cwd(), targetDir) @@ -209,9 +193,6 @@ program } } - // ==================== - // 3. ENSURE STANDARD DIRECTORIES - // ==================== console.log(chalk.white('Ensuring standard content-type directories exist...\n')) // Add standard content-type directories for each affected product @@ -236,9 +217,6 @@ program } } - // ==================== - // 4. CREATE PLACEHOLDERS - // ==================== console.log(chalk.white('Creating placeholder index.md files...\n')) const newPlaceholders: string[] = [] @@ -295,9 +273,6 @@ contentType: ${placeholderContentType} } } - // ==================== - // 5. GENERATE INTROS - // ==================== if (newPlaceholders.length > 0) { console.log(chalk.white('\nGenerating intros for placeholder files...\n')) @@ -337,9 +312,6 @@ contentType: ${placeholderContentType} } } - // ==================== - // 6. MOVE FILES - // ==================== console.log(chalk.white('\nMoving files...\n')) const moved: Array<{ file: string; from: string; to: string }> = [] @@ -397,7 +369,6 @@ contentType: ${placeholderContentType} } } - // Move regular files for (const file of regularFiles) { try { await fs.mkdir(file.targetDir, { recursive: true }) @@ -427,7 +398,6 @@ contentType: ${placeholderContentType} } } - // Delete source subdirectory index files for (const sourcePath of indexFilesToDeleteLater) { try { await fs.unlink(sourcePath) @@ -437,7 +407,6 @@ contentType: ${placeholderContentType} } } - // Move top-level index files for (const file of topLevelIndexFiles) { try { await fs.mkdir(file.targetDir, { recursive: true }) @@ -462,9 +431,6 @@ contentType: ${placeholderContentType} } } - // ==================== - // 7. CLEANUP & UPDATE - // ==================== console.log( chalk.white('\nCleaning up old directories and updating parent index.md files...\n'), ) @@ -498,7 +464,6 @@ contentType: ${placeholderContentType} console.log(chalk.yellow(`⚠ Could not read product directory ${productDir}: ${error}`)) } - // Update product index.md const productIndexPath = path.join(productDir, 'index.md') try { const content = await fs.readFile(productIndexPath, 'utf-8') @@ -507,7 +472,6 @@ contentType: ${placeholderContentType} if (data) { let updated = false - // Build children array const productRelativePath = path.relative(process.cwd(), productDir) const newChildren: string[] = [] for (const ct of CONTENT_TYPES.map(contentTypeToDir)) { @@ -520,7 +484,6 @@ contentType: ${placeholderContentType} updated = true } - // Add redirects for deleted directories const deletedPaths = deletedByProduct.get(productName) || [] if (deletedPaths.length > 0) { if (!data.redirect_from) data.redirect_from = [] @@ -551,9 +514,6 @@ contentType: ${placeholderContentType} } } - // ==================== - // 8. SORT CHILDREN ARRAYS - // ==================== console.log(chalk.white('\nSorting children arrays...\n')) for (const dirPath of targetDirs) { @@ -612,9 +572,6 @@ contentType: ${placeholderContentType} } } - // ==================== - // 9. SUMMARY - // ==================== console.log(chalk.white(`\n${'='.repeat(60)}`)) console.log(chalk.white('Summary:')) console.log(chalk.white(` Moved: ${moved.length} files`)) diff --git a/src/content-render/scripts/move-content.ts b/src/content-render/scripts/move-content.ts index 6bbdcb1a0635..004cb880363d 100755 --- a/src/content-render/scripts/move-content.ts +++ b/src/content-render/scripts/move-content.ts @@ -33,7 +33,6 @@ import escapeStringRegexp from 'escape-string-regexp' import fm from '@/frame/lib/frontmatter' import readFrontmatter from '@/frame/lib/read-frontmatter' -// Type definitions interface MoveOptions { verbose: boolean undo: boolean @@ -156,8 +155,7 @@ async function main(opts: MoveOptions, nameTuple: string[]) { if (!fs.existsSync(indexFilePath)) { throw new Error(`${oldPath} does not have an index.md file`) } - // Gather individual files by walking `oldPath` recursively - // The second argument is + // Gather individual files by walking `oldPath` recursively. const files = findFilesInFolder(oldPath, newPath, opts) // First take care of the `git mv` (or regular rename) part. diff --git a/src/content-render/scripts/reusables-cli/find/potential-uses.ts b/src/content-render/scripts/reusables-cli/find/potential-uses.ts index cb423a475038..c0827117caa9 100644 --- a/src/content-render/scripts/reusables-cli/find/potential-uses.ts +++ b/src/content-render/scripts/reusables-cli/find/potential-uses.ts @@ -26,7 +26,6 @@ export function findPotentialUses({ const filesThatCouldUseReusable: FilesWithLineNumbers = [] const filesThatCouldUseReusableSimilar: FilesWithSimilarity = [] - // Read all content & data files into memory const allFileContents = allFilePaths.map((filePath) => { return { filePath, @@ -69,7 +68,6 @@ export function findPotentialUses({ const indices = findIndicesOfSubstringInString(reusableContents.trim(), fileContents) if (indices.length > 0) { - // Find line numbers of each index in fileContents const lineNumbers = indices.map((index) => fileContents.slice(0, index).split('\n').length) filesThatCouldUseReusable.push({ diff --git a/src/content-render/scripts/reusables-cli/find/used.ts b/src/content-render/scripts/reusables-cli/find/used.ts index 589b87e12e6c..6f56c31512d6 100644 --- a/src/content-render/scripts/reusables-cli/find/used.ts +++ b/src/content-render/scripts/reusables-cli/find/used.ts @@ -33,7 +33,6 @@ export function findUsed(reusablePath: string, { absolute }: { absolute: boolean const indices = getIndicesOfLiquidVariable(reusableLiquidVar, fileContents) if (indices.length > 0) { - // Find line numbers of each index in fileContents const lineNumbers = indices.map((index) => fileContents.slice(0, index).split('\n').length) filesWithReusables.push({ diff --git a/src/content-render/scripts/reusables-cli/shared.ts b/src/content-render/scripts/reusables-cli/shared.ts index 792f48828bdc..c04e24725d15 100644 --- a/src/content-render/scripts/reusables-cli/shared.ts +++ b/src/content-render/scripts/reusables-cli/shared.ts @@ -48,7 +48,6 @@ export function getAllContentFilePaths() { return [...allContentFiles, ...allDataFiles] } -// Get the string that represents the reusable in the content files export function getReusableLiquidString(reusablePath: string): string { const relativePath = path.relative(reusablesDirectory, reusablePath) return `reusables.${relativePath.slice(0, -3).split('/').join('.')}` @@ -73,7 +72,6 @@ export function getIndicesOfLiquidVariable(liquidVariable: string, fileContents: return indices } -// Find the path to a reusable file. export function resolveReusablePath(reusablePath: string): string { // Try .md if extension is not provided if (!reusablePath.endsWith('.md') && !reusablePath.endsWith('.yml')) { @@ -150,7 +148,6 @@ export function findSimilarSubStringInString(substr: string, str: string) { } } - // Normalize the similarity score return Math.round((similarityScore / substrSentences.length) * corpus.length) } diff --git a/src/content-render/scripts/update-filepaths.ts b/src/content-render/scripts/update-filepaths.ts index f86693010639..7760d5c7b441 100755 --- a/src/content-render/scripts/update-filepaths.ts +++ b/src/content-render/scripts/update-filepaths.ts @@ -101,13 +101,11 @@ async function processFile( const isDirectory = isDirectoryCheck(file) - // Assess the frontmatter and other conditions to determine if we want to process the path. const processPage: boolean = determineProcessStatus(data, isDirectory, scriptOptions) if (!processPage) return null let stringToSlugify: string = data.shortTitle || data.title - // Check if we need to process Liquid if (stringToSlugify.includes('{%')) { stringToSlugify = await renderContent(stringToSlugify, context, { textOnly: true }) } @@ -118,7 +116,6 @@ async function processFile( // Fall back to title if shortTitle doesn't exist. const slug: string = slugger.slug(decode(stringToSlugify)) - // Get the basename, depending on whether it's a file or dir. let basename: string if (isDirectory) { // Where: content location = content/foobar/index.md @@ -133,12 +130,10 @@ async function processFile( // If slug and basename already match, all set here. Return early. if (slug === basename) return null - // Build the new path based on file type. const newPath = isDirectory ? path.join(path.dirname(path.dirname(file)), slug, 'index.md') : path.join(path.dirname(file), `${slug}.md`) - // Get relative paths and adjust for directories. const getContentPath = (filePath: string): string => { const relativePath = path.relative(process.cwd(), filePath) return isDirectory ? path.dirname(relativePath) : relativePath @@ -187,26 +182,21 @@ function sortFiles(filesArray: string[]): string[] { // 2. Deepest subdirectory path // 3. Shallowest subdirectory path (up to category level, e.g., content/product/category) return filesArray.toSorted((a, b) => { - // If A is a file and B is a directory, A comes first (negative) if (!isDirectoryCheck(a) && isDirectoryCheck(b)) { return -1 } - // If A is a directory and B is a file, B comes first (positive) if (isDirectoryCheck(a) && !isDirectoryCheck(b)) { return 1 } - // If A and B are both files, neutral if (!isDirectoryCheck(a) && !isDirectoryCheck(b)) { return 0 } - // If both are directories, sort by depth (deepest first) if (isDirectoryCheck(a) && isDirectoryCheck(b)) { const aDepth = a.split(path.sep).length const bDepth = b.split(path.sep).length return bDepth - aDepth // Deeper paths first } - // This should never be reached, but return 0 for safety return 0 }) } @@ -246,20 +236,16 @@ function determineProcessStatus( isDirectory: boolean, scriptOptions: ScriptOptions, ): boolean { - // Assess the conditions in this order: - // If it's a directory AND we're excluding dirs, do not process it no matter what. + // A directory is never processed when dirs are excluded, whatever else is set. if (isDirectory && scriptOptions.excludeDirs) { return false } - // If the force option is passed, process it no matter what. if (scriptOptions.force) { return true } - // If the page has the override set, do not process it. if (data.allowTitleToDifferFromFilename) { return false } - // In all other cases, process it. return true } From 3c379b6305c6ec5a5105612c9527420ca68359b0 Mon Sep 17 00:00:00 2001 From: Kevin Heis Date: Tue, 15 Sep 2026 20:16:40 +0000 Subject: [PATCH 03/16] Trim excessive comments in src/graphql and src/fixtures (#63275) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 17588eeb-788a-4f82-9d36-b5079fb36521 --- src/fixtures/playwright.config.ts | 14 ------ src/fixtures/tests/api-article-body.ts | 15 ------ .../tests/categories-and-subcategory.ts | 4 +- src/fixtures/tests/playwright-a11y.spec.ts | 4 +- .../tests/playwright-rendering.spec.ts | 50 ++----------------- src/graphql/components/GraphqlItem.tsx | 2 +- src/graphql/components/GraphqlPage.tsx | 5 +- src/graphql/lib/validator.ts | 14 ------ src/graphql/scripts/build-changelog.ts | 21 +------- src/graphql/scripts/sync.ts | 8 +-- src/graphql/scripts/utils/process-previews.ts | 1 - src/graphql/scripts/utils/process-schemas.ts | 34 +++++-------- 12 files changed, 29 insertions(+), 143 deletions(-) diff --git a/src/fixtures/playwright.config.ts b/src/fixtures/playwright.config.ts index 9ba48a14ccaf..7d4e382171aa 100644 --- a/src/fixtures/playwright.config.ts +++ b/src/fixtures/playwright.config.ts @@ -1,11 +1,5 @@ import { defineConfig } from '@playwright/test' -/** - * Read environment variables from file. - * https://github.com/motdotla/dotenv - */ -// require('dotenv').config(); - const CI = Boolean(JSON.parse(process.env.CI || 'false')) const PLAYWRIGHT_START_SERVER_COMMAND = @@ -36,7 +30,6 @@ const EXPECT_TIMEOUT = process.env.PLAYWRIGHT_EXPECT_TIMEOUT */ export default defineConfig({ testDir: './tests', - /* Maximum time one test can run for. */ timeout: TIMEOUT, expect: { /** @@ -45,13 +38,9 @@ export default defineConfig({ */ timeout: EXPECT_TIMEOUT, }, - /* Run tests in files in parallel */ fullyParallel: true, - /* Fail the build on CI if you accidentally left test.only in the source code. */ forbidOnly: !!process.env.CI, - /* Retry on CI only */ retries: RETRIES, - /* Opt out of parallel tests on CI. */ workers: process.env.PLAYWRIGHT_WORKERS ? JSON.parse(process.env.PLAYWRIGHT_WORKERS) : CI @@ -63,14 +52,12 @@ export default defineConfig({ use: { /* Maximum time each action such as `click()` can take. Defaults to 0 (no limit). */ actionTimeout: 0, - /* Base URL to use in actions like `await page.goto('/')`. */ baseURL: 'http://localhost:4000', /* Collect trace when retrying the failed test. See https://playwright.dev/docs/trace-viewer */ trace: 'on-first-retry', }, - /* Configure projects for major browsers */ projects: [ // { // name: 'chromium', @@ -125,7 +112,6 @@ export default defineConfig({ /* Folder for test artifacts such as screenshots, videos, traces, etc. */ // outputDir: 'test-results/', - /* Run your local dev server before starting the tests */ webServer: { command: PLAYWRIGHT_START_SERVER_COMMAND, port: 4000, diff --git a/src/fixtures/tests/api-article-body.ts b/src/fixtures/tests/api-article-body.ts index 05006793549c..fb4d3de6d23a 100644 --- a/src/fixtures/tests/api-article-body.ts +++ b/src/fixtures/tests/api-article-body.ts @@ -55,9 +55,7 @@ describe('article body api', () => { const res = await get(makeURL('/en/get-started/start-your-journey/api-article-body-test-page')) expect(res.statusCode).toBe(200) - // Should not contain frontmatter expect(res.body).not.toMatch(/^---/) - // Should have at least one heading expect(res.body).toMatch(/^#{1,6}\s+\w+/m) }) @@ -80,21 +78,17 @@ describe('article body api', () => { expect(res.statusCode).toBe(200) expect(res.headers['content-type']).toContain('text/markdown') - // Should contain all tool-specific content variants expect(res.body).toContain('
') expect(res.body).toContain('
') expect(res.body).toContain('
') - // Should contain the actual content from each tool expect(res.body).toContain('This is webui content') expect(res.body).toContain('This is cli content') expect(res.body).toContain('This is desktop content') - // Should contain tool-specific sections expect(res.body).toContain('Webui section specific content') expect(res.body).toContain('Desktop section specific content') - // Verify multiple instances of the same tool are preserved const webuiMatches = res.body.match(/
/g) const desktopMatches = res.body.match(/
/g) expect(webuiMatches).toBeDefined() @@ -108,11 +102,9 @@ describe('article body api', () => { expect(res.statusCode).toBe(200) expect(res.headers['content-type']).toContain('text/markdown') - // Should contain both webui and codespaces tool content expect(res.body).toContain('
') expect(res.body).toContain('
') - // Should contain the actual content from both tools expect(res.body).toContain('Under your repository name, click **Pull requests**') expect(res.body).toContain('Open the pull request in your codespace') expect(res.body).toContain( @@ -122,7 +114,6 @@ describe('article body api', () => { 'After reviewing the files, you can submit your review directly from Codespaces', ) - // Verify both tools appear in multiple sections const webuiMatches = res.body.match(/
/g) const codespacesMatches = res.body.match(/
/g) expect(webuiMatches).toBeDefined() @@ -139,7 +130,6 @@ describe('article body api', () => { ), ) - // Skip test if page doesn't exist in fixture environment if (res.statusCode === 404) { console.log('Production page not available in fixture environment, skipping test') return @@ -147,11 +137,9 @@ describe('article body api', () => { expect(res.statusCode).toBe(200) - // Verify the fix is working - codespaces content should now be present const hasCodespacesContent = res.body.includes('
') expect(hasCodespacesContent).toBe(true) - // Also verify that webui content is still present expect(res.body).toContain('
') }) @@ -164,7 +152,6 @@ describe('article body api', () => { ), ) - // Skip test if page doesn't exist in fixture environment if (res.statusCode === 404) { console.log( 'Production page not available in fixture environment, skipping issue verification test', @@ -179,11 +166,9 @@ describe('article body api', () => { expect(res.body).toContain('
') expect(res.body).toContain('
') - // Verify specific codespaces content that was missing before the fix expect(res.body).toContain('GitHub Codespaces') expect(res.body).toContain('Open the pull request in a codespace') - // Ensure both tools are rendered with their respective content const webuiMatches = res.body.match(/
/g) const codespacesMatches = res.body.match(/
/g) diff --git a/src/fixtures/tests/categories-and-subcategory.ts b/src/fixtures/tests/categories-and-subcategory.ts index fcbf6375987d..24aa47af6056 100644 --- a/src/fixtures/tests/categories-and-subcategory.ts +++ b/src/fixtures/tests/categories-and-subcategory.ts @@ -17,7 +17,7 @@ describe('subcategories', () => { expect( hrefs.every((href: string) => href.startsWith('/en/get-started/start-your-journey/')), ).toBeTruthy() - // The all resolve to a 200 OK without redirects + // They all resolve to a 200 OK without redirects const responses = await Promise.all(hrefs.map((href: string) => head(href))) expect(responses.every((r: { statusCode: number }) => r.statusCode === 200)).toBeTruthy() }) @@ -53,7 +53,7 @@ describe('categories', () => { // They all have the same prefix const hrefs = links.map((i: number, el: Element) => $(el).attr('href')).get() expect(hrefs.every((href: string) => href.startsWith('/en/actions/category/'))).toBeTruthy() - // The all resolve to a 200 OK without redirects + // They all resolve to a 200 OK without redirects const responses = await Promise.all(hrefs.map((href: string) => head(href))) expect(responses.every((r: { statusCode: number }) => r.statusCode === 200)).toBeTruthy() }) diff --git a/src/fixtures/tests/playwright-a11y.spec.ts b/src/fixtures/tests/playwright-a11y.spec.ts index 0eacad14ed50..e3e24039c38d 100644 --- a/src/fixtures/tests/playwright-a11y.spec.ts +++ b/src/fixtures/tests/playwright-a11y.spec.ts @@ -54,8 +54,8 @@ for (const pageName of Object.keys(pages)) { // otherwise never be scanned. test.describe('search filters (narrow viewport)', () => { // Without a local Elasticsearch the middleware proxies to production, so there are no - // aggregations, the disclosure never renders, and this would time out rather than skip - // — matching the guard every search test in playwright-rendering.spec.ts uses. + // aggregations, the disclosure never renders, and this would time out rather than + // skip. Matches the guard every search test in playwright-rendering.spec.ts uses. test.skip(!SEARCH_TESTS, 'No local Elasticsearch, no tests involving search') test('expanded filter disclosure passes axe', async ({ page }) => { diff --git a/src/fixtures/tests/playwright-rendering.spec.ts b/src/fixtures/tests/playwright-rendering.spec.ts index 461b8e3643a6..c5e18a7880b8 100644 --- a/src/fixtures/tests/playwright-rendering.spec.ts +++ b/src/fixtures/tests/playwright-rendering.spec.ts @@ -145,12 +145,9 @@ test('do a search from home page and click on "Foo" page', async ({ page }) => { await page.goto('/') await turnOffExperimentsInPage(page) - // Use the search overlay await page.locator('[data-testid="search"]:visible').click() await page.getByTestId('overlay-search-input').fill('serve playwright') - // Wait for search results to load await page.waitForTimeout(1000) - // Click "View more results" to get to the search page await page.getByText('View more results').click() await expect(page).toHaveURL( @@ -202,9 +199,7 @@ test('open search, and select a general search article', async ({ page }) => { // Let new suggestions load const searchOverlay = page.getByTestId('general-autocomplete-suggestions') await expect(searchOverlay.getByText('For Playwright')).toBeVisible() - // Navigate to general search item, "For Playwright" await page.keyboard.press('ArrowDown') - // Select the general search item, "For Playwright" await page.keyboard.press('Enter') // We should now be on the page for "For Playwright" @@ -241,9 +236,7 @@ test('open search, and get auto-complete results', async ({ page }) => { await expect(searchInput).toBeVisible() await expect(searchInput).toBeEnabled() - // Type the text "rest" into the search input await searchInput.fill('rest') - // For for 1 second for the suggestions to load await page.waitForTimeout(1000) // Ask AI suggestions @@ -267,12 +260,9 @@ test('search from enterprise-cloud and filter by top-level Fooing', async ({ pag await page.goto('/enterprise-cloud@latest') await turnOffExperimentsInPage(page) - // Use the search overlay await page.locator('[data-testid="search"]:visible').click() await page.getByTestId('overlay-search-input').fill('fixture') - // Wait for search results to load await page.waitForTimeout(1000) - // Click "View more results" to get to the search page await page.getByText('View more results').click() // Now we're on the search results page, apply the filter @@ -309,7 +299,7 @@ test.describe('platform picker', () => { }) test('minitoc matches picker', async ({ page }) => { - // default platform set to windows in fixture fronmatter + // The fixture frontmatter defaults the platform to Windows. await page.goto('/get-started/liquid/platform-specific') await turnOffExperimentsInPage(page) await expect( @@ -450,12 +440,10 @@ test('navigate with side bar into article inside a subcategory inside a category }) test('sidebar custom link functionality works', async ({ page }) => { - // Test that sidebar functionality is not broken by custom links feature await page.goto('/get-started') await expect(page).toHaveTitle(/Getting started with HubGit/) - // Verify that regular sidebar navigation still works by clicking on known sections await page.getByTestId('product-sidebar').getByText('Start your journey').click() await page.getByTestId('product-sidebar').getByText('Hello World').click() await expect(page).toHaveURL(/\/en\/get-started\/start-your-journey\/hello-world/) @@ -550,7 +538,6 @@ test.describe('hover cards', () => { ), ).toBeVisible() - // Press Escape to close it await page.keyboard.press('Escape') await expect( page.getByText( @@ -574,7 +561,6 @@ test.describe('hover cards', () => { ), ).toBeVisible() - // click the Esc key to close the hovercard await page.keyboard.press('Escape') await expect( page.getByText( @@ -603,7 +589,7 @@ test.describe('test nav at different viewports', () => { await expect(page.getByTestId('breadcrumbs-bar')).toBeVisible() // breadcrumbs show up in one of the pages that use the AutomatedPage - // component (e.g. graphql, audit log, etc.) -- we test the webhooks + // component (e.g. graphql, audit log). This one uses the webhooks // reference page here await page.goto('/webhooks/webhook-events-and-payloads') await expect(page.getByTestId('breadcrumbs-bar')).toBeVisible() @@ -627,7 +613,7 @@ test.describe('test nav at different viewports', () => { height: 700, }) - // Opening the mobile nav must still render the doc-tree drawer -- before the + // Opening the mobile nav must still render the doc-tree drawer. Before the // fix, `collapsed` short-circuited the sidebar to null while the open state // hid the content column, leaving a blank area with no drawer. await page.getByTestId('sidebar-mobile-toggle').click() @@ -651,7 +637,7 @@ test.describe('test nav at different viewports', () => { const nav = page.locator('[data-container="nav"]') await expect(nav).toHaveAttribute('data-mobile-open', 'true') - // Resize up to the desktop breakpoint -- the inline nav should close and the + // Resize up to the desktop breakpoint. The inline nav should close and the // fixed desktop rail (326px) should take over rather than the full-width // mobile markup persisting over the page. await page.setViewportSize({ @@ -669,16 +655,13 @@ test.describe('test nav at different viewports', () => { }) await page.goto('/get-started/foo/bar') - // version picker should be visible await page.getByTestId('version-picker').getByRole('button').click() expect((await page.getByRole('menuitemradio').all()).length).toBeGreaterThan(0) await expect(page.getByRole('menuitemradio', { name: 'Enterprise Cloud' })).toBeVisible() - // language picker is visible await page.getByRole('button', { name: 'Select language: current language is English' }).click() await expect(page.getByRole('menuitemradio', { name: 'English' })).toBeVisible() - // header sign up button is visible await expect(page.getByTestId('header-signup')).toBeVisible() }) @@ -707,19 +690,15 @@ test.describe('test nav at different viewports', () => { }) await page.goto('/get-started/foo/bar') - // version picker is visible await page.getByTestId('version-picker').getByRole('button').click() expect((await page.getByRole('menuitemradio').all()).length).toBeGreaterThan(0) await expect(page.getByRole('menuitemradio', { name: 'Enterprise Cloud' })).toBeVisible() - // language picker is in mobile menu await page.getByTestId('mobile-menu').click() await expect(page.getByRole('menuitemradio', { name: 'English' })).toBeVisible() - // sign up button is in mobile menu await expect(page.getByTestId('mobile-signup')).toBeVisible() - // hamburger button for sidebar overlay is visible await expect(page.getByTestId('sidebar-mobile-toggle')).toBeVisible() await page.getByTestId('sidebar-mobile-toggle').click() await expect(page.getByTestId('sidebar')).toBeVisible() @@ -732,23 +711,17 @@ test.describe('test nav at different viewports', () => { }) await page.goto('/get-started/foo/bar') - // header sign-up button is not visible await expect(page.getByTestId('header-signup')).not.toBeVisible() - // language picker is not visible await expect(page.getByTestId('language-picker')).not.toBeVisible() - // version picker is visible await expect(page.getByTestId('version-picker').getByRole('button')).toBeVisible() - // language picker is in mobile menu await page.getByTestId('mobile-menu').click() await expect(page.getByRole('menuitemradio', { name: 'English' })).toBeVisible() - // sign up button is in mobile menu await expect(page.getByTestId('mobile-signup')).toBeVisible() - // hamburger button for sidebar overlay is visible await expect(page.getByTestId('sidebar-mobile-toggle')).toBeVisible() await page.getByTestId('sidebar-mobile-toggle').click() await expect(page.getByTestId('sidebar')).toBeVisible() @@ -762,27 +735,20 @@ test.describe('test nav at different viewports', () => { await page.goto('/get-started/foo/bar') await turnOffExperimentsInPage(page) - // header sign-up button is not visible await expect(page.getByTestId('header-signup')).not.toBeVisible() - // language picker is not visible await expect(page.getByTestId('language-picker')).not.toBeVisible() - // version picker is not visible await expect(page.getByTestId('version-picker').getByRole('button')).not.toBeVisible() - // version picker is in mobile menu await expect(page.getByTestId('version-picker')).not.toBeVisible() await page.getByTestId('mobile-menu').click() await expect(page.getByTestId('open-mobile-menu').getByTestId('version-picker')).toBeVisible() - // language picker is in mobile menu await expect(page.getByTestId('open-mobile-menu').getByTestId('language-picker')).toBeVisible() - // sign up button is in mobile menu await expect(page.getByTestId('mobile-signup')).toBeVisible() - // hamburger button for sidebar overlay is visible await expect(page.getByTestId('sidebar-mobile-toggle')).toBeVisible() await page.getByTestId('sidebar-mobile-toggle').click() await expect(page.getByTestId('sidebar')).toBeVisible() @@ -798,12 +764,9 @@ test.describe('test nav at different viewports', () => { await page.goto('/get-started/foo/bar') await turnOffExperimentsInPage(page) - // Use the search overlay await page.locator('[data-testid="mobile-search-button"]:visible').click() await page.getByTestId('overlay-search-input').fill('serve playwright') - // Wait for search results to load await page.waitForTimeout(1000) - // Click "View more results" to get to the search page await page.getByText('View more results').click() await expect(page).toHaveURL( @@ -822,12 +785,9 @@ test.describe('test nav at different viewports', () => { await page.goto('/get-started/foo/bar') await turnOffExperimentsInPage(page) - // Use the search overlay await page.locator('[data-testid="mobile-search-button"]:visible').click() await page.getByTestId('overlay-search-input').fill('serve playwright') - // Wait for search results to load await page.waitForTimeout(1000) - // Click "View more results" to get to the search page await page.getByText('View more results').click() await expect(page).toHaveURL( @@ -841,7 +801,7 @@ test.describe('secondary-bar breadcrumb scroller', () => { // The secondary bar (and its breadcrumb scroller) only renders at wide // viewports, and the fixture trail is short enough to fit there, so we cap the // scroller width to force a deterministic overflow independent of title - // lengths — then exercise the chevrons. + // lengths, then exercise the chevrons. test('chevrons scroll one crumb at a time instead of jumping to the ends', async ({ page }) => { // Smooth-scroll settle waits across several chevron clicks add up past the // default 5s cap. diff --git a/src/graphql/components/GraphqlItem.tsx b/src/graphql/components/GraphqlItem.tsx index c15af9d85014..fe05f8f0b74d 100644 --- a/src/graphql/components/GraphqlItem.tsx +++ b/src/graphql/components/GraphqlItem.tsx @@ -18,7 +18,7 @@ type Props = { kind?: SchemaKindKey } -// Clamp a numeric heading level to the valid HTML range (2–6). Used to +// Clamp a numeric heading level to the valid HTML range (2-6). Used to // build heading tag names like `h2`/`h3` from a numeric `headingLevel` // prop without producing invalid tags if a caller passes something odd. function headingTag(level: number): keyof JSX.IntrinsicElements { diff --git a/src/graphql/components/GraphqlPage.tsx b/src/graphql/components/GraphqlPage.tsx index f6201f6d5488..6af3dc924792 100644 --- a/src/graphql/components/GraphqlPage.tsx +++ b/src/graphql/components/GraphqlPage.tsx @@ -30,9 +30,8 @@ type Props = { export const GraphqlPage = ({ schema, pageName, objects }: Props) => { const graphqlItems: JSX.Element[] = [] // In the case of the H2s for Queries - // The queries page has two heading sections (connections and fields) - // So we need to add the heading component and the children under it - // for each section. + // The queries page has two heading sections (connections and fields), so add + // the heading component and its children once per section. if (pageName === 'queries') { graphqlItems.push( ...(schema as QueryT[]).map((item) => ), diff --git a/src/graphql/lib/validator.ts b/src/graphql/lib/validator.ts index be7e085db81c..89283c83e5a5 100644 --- a/src/graphql/lib/validator.ts +++ b/src/graphql/lib/validator.ts @@ -1,7 +1,6 @@ // the tests in tests/graphql.ts use this schema to ensure the integrity // of the data in src/graphql/data/*.json -// JSON Schema type definitions for AJV validation interface JSONSchema { type?: string required?: string[] @@ -16,7 +15,6 @@ interface ValidatorSchema extends JSONSchema { properties: Record } -// PREVIEWS export const previewsValidator: ValidatorSchema = { type: 'object', required: [ @@ -53,7 +51,6 @@ export const previewsValidator: ValidatorSchema = { }, } -// UPCOMING CHANGES export const upcomingChangesValidator: ValidatorSchema = { type: 'object', required: ['location', 'description', 'reason', 'date', 'criticality', 'owner'], @@ -82,7 +79,6 @@ export const upcomingChangesValidator: ValidatorSchema = { }, } -// SCHEMAS // many GraphQL schema members have these core properties const coreProps: JSONSchema = { properties: { @@ -133,13 +129,11 @@ delete corePropsNoType.properties!.type const corePropsNoDescription = dup(coreProps) delete corePropsNoDescription.properties!.description -// QUERIES const queries = dup(corePropsPlusArgs) as ValidatorSchema queries.type = 'object' queries.required = ['name', 'type', 'id', 'href', 'description'] -// MUTATIONS const mutations = dup(corePropsNoType) as ValidatorSchema mutations.type = 'object' @@ -161,7 +155,6 @@ mutations.properties.returnFields = { }, } -// OBJECTS const objects = dup(corePropsNoType) as ValidatorSchema objects.type = 'object' @@ -194,7 +187,6 @@ objects.properties.implements = { }, } -// INTERFACES const interfaces = dup(corePropsNoType) as ValidatorSchema interfaces.type = 'object' @@ -208,7 +200,6 @@ interfaces.properties.fields = { }, } -// ENUMS const enums = dup(corePropsNoType) as ValidatorSchema enums.type = 'object' @@ -230,7 +221,6 @@ enums.properties.values = { }, } -// UNIONS const unions = dup(corePropsNoType) as ValidatorSchema unions.type = 'object' @@ -255,7 +245,6 @@ unions.properties.possibleTypes = { }, } -// INPUT OBJECTS const inputObjects = dup(corePropsNoType) as ValidatorSchema inputObjects.type = 'object' @@ -269,18 +258,15 @@ inputObjects.properties.inputFields = { }, } -// SCALARS const scalars = dup(corePropsNoType) as ValidatorSchema scalars.type = 'object' scalars.required = ['name', 'id', 'href', 'description'] -// Deep clone utility function with proper typing function dup(obj: T): T { return JSON.parse(JSON.stringify(obj)) } -// Schema validator collection with proper typing interface SchemaValidators { queries: ValidatorSchema mutations: ValidatorSchema diff --git a/src/graphql/scripts/build-changelog.ts b/src/graphql/scripts/build-changelog.ts index 11acb66f209b..0b645a4e7f52 100644 --- a/src/graphql/scripts/build-changelog.ts +++ b/src/graphql/scripts/build-changelog.ts @@ -65,16 +65,12 @@ let lastIgnoredChanges: Change[] = [] * structure written to `targetPath`. (`changelogEntry` and that file are modified in place.) */ export function prependDatedEntry(changelogEntry: ChangelogEntry, targetPath: string): void { - // Build a `yyyy-mm-dd`-formatted date string - // and tag the changelog entry with it const todayString = new Date().toISOString().slice(0, 10) changelogEntry.date = todayString const previousChangelogString = fs.readFileSync(targetPath, 'utf8') const previousChangelog = JSON.parse(previousChangelogString) as ChangelogEntry[] - // add a new entry to the changelog data previousChangelog.unshift(changelogEntry) - // rewrite the updated changelog fs.writeFileSync(targetPath, JSON.stringify(previousChangelog, null, 2)) // Ensure a content page exists for this entry's year @@ -108,7 +104,6 @@ export function ensureYearPage( ].join('\n') fs.writeFileSync(yearPagePath, yearPage) - // Prepend the new year to children in index.md const indexPath = nodePath.join(contentDir, 'index.md') const indexContent = fs.readFileSync(indexPath, 'utf8') const updated = indexContent.replace(/^(children:\n)/m, `$1 - /${year}\n`) @@ -128,11 +123,9 @@ export async function createChangelogEntry( oldUpcomingChanges: UpcomingChange[], newUpcomingChanges: UpcomingChange[], ): Promise { - // Create schema objects out of the strings const oldSchema = await loadSchema(oldSchemaString, { loaders: [] }) const newSchema = await loadSchema(newSchemaString, { loaders: [] }) - // Generate changes between the two schemas const changes = await diff(oldSchema, newSchema) const changesToReport: Change[] = [] const ignoredChanges: Change[] = [] @@ -140,12 +133,10 @@ export async function createChangelogEntry( if (CHANGES_TO_REPORT.includes(change.type)) { changesToReport.push(change) } else { - // Track ignored changes for visibility ignoredChanges.push(change) } } - // Log warnings for ignored change types to provide visibility if (ignoredChanges.length > 0) { const ignoredTypes = [...new Set(ignoredChanges.map((change) => change.type))] console.warn( @@ -160,7 +151,6 @@ export async function createChangelogEntry( ) } - // Store ignored changes for potential workflow outputs lastIgnoredChanges = ignoredChanges const { schemaChangesToReport, previewChangesToReport } = segmentPreviewChanges( @@ -180,7 +170,6 @@ export async function createChangelogEntry( }) }) - // If there were any changes, create a changelog entry if ( schemaChangesToReport.length > 0 || Object.keys(previewChangesToReport).length > 0 || @@ -365,19 +354,13 @@ const CHANGES_TO_REPORT = [ ChangeType.DirectiveUsageFieldDefinitionRemoved, ] -// CHANGES_TO_IGNORE list removed - now we only process changes explicitly listed -// in CHANGES_TO_REPORT and silently ignore all others for future compatibility +// Anything not in CHANGES_TO_REPORT is logged as ignored rather than reported, +// so a new change type added upstream cannot break this script. -/** - * Get the ignored change types from the last changelog entry creation - */ export function getLastIgnoredChanges(): Change[] { return lastIgnoredChanges } -/** - * Get summary of ignored change types for workflow outputs - */ export function getIgnoredChangesSummary(): IgnoredChangesSummary | null { const ignored = getLastIgnoredChanges() if (ignored.length === 0) return null diff --git a/src/graphql/scripts/sync.ts b/src/graphql/scripts/sync.ts index 15fe3eb165e3..6293964ded4f 100755 --- a/src/graphql/scripts/sync.ts +++ b/src/graphql/scripts/sync.ts @@ -18,7 +18,6 @@ import { getIgnoredChangesSummary, } from './build-changelog' -// Type definitions interface GitHubRepoOptions { owner: string repo: string @@ -79,7 +78,7 @@ const allIgnoredChanges: IgnoredChange[] = [] async function main() { for (const version of versionsToBuild) { - // Get the relevant GraphQL name for the current version + // Get the relevant GraphQL name for the current version. // For example, free-pro-team@latest corresponds to dotcom, // enterprise-server@2.22 corresponds to ghes-2.22. const graphqlVersion = allVersions[version].openApiVersionName @@ -198,7 +197,7 @@ async function main() { // children and disappearance redirects, based on the presence collected above. await syncCategoryContentFiles(categoryPresence) - // Ensure the YAML linter runs before checkinging in files + // Run the YAML linter before anything is checked in. execSync('npx prettier -w "**/*.{yml,yaml}"') // Output ignored changes for GitHub Actions @@ -212,7 +211,6 @@ async function main() { '::notice title=GraphQL Ignored Changes::Found ignored change types that may need review', ) - // Write outputs to GitHub Actions output file if (process.env.GITHUB_OUTPUT) { appendFileSync( process.env.GITHUB_OUTPUT, @@ -237,7 +235,6 @@ async function getRemoteRawContent(filepath: string, graphqlVersion: string) { let took = new Date().getTime() - t0 console.log(`Got ref (${options.ref}) for '${graphqlVersion}'. Took ${formatTime(took)}`) - // add the filepath to the options so we can get the contents of the file options.path = `config/${path.basename(filepath)}` t0 = new Date().getTime() @@ -275,7 +272,6 @@ async function getBranchAsRef( // the first time this runs, it uses the branch found for the version above if (!branch) branch = branches[versionType] - // set the branch as the ref const ref = `heads/${branch}` // check whether the branch can be found in github/github diff --git a/src/graphql/scripts/utils/process-previews.ts b/src/graphql/scripts/utils/process-previews.ts index 3845fe1061a4..4a9197faa42f 100644 --- a/src/graphql/scripts/utils/process-previews.ts +++ b/src/graphql/scripts/utils/process-previews.ts @@ -37,7 +37,6 @@ export default function processPreviews(previews: RawPreview[]): ProcessedPrevie // remove unnecessary leading colon const toggled_by = raw.toggled_by.replace(':', '') - // add convenience properties const accept_header = `application/vnd.github.${toggled_by}+json` slugger.reset() diff --git a/src/graphql/scripts/utils/process-schemas.ts b/src/graphql/scripts/utils/process-schemas.ts index 9004c9c81234..9abfd2995ee2 100755 --- a/src/graphql/scripts/utils/process-schemas.ts +++ b/src/graphql/scripts/utils/process-schemas.ts @@ -235,13 +235,12 @@ const externalScalars: ScalarInfo[] = await Promise.all( }), ) -// select and format all the data from the schema that we need for the docs -// used in the build step // Shape of the per-version `category-map.json` used both at runtime by the // redirect middleware and (here) at build time as a fallback source of // categories when a schema lacks `@docsCategory` directives. type CategoryMapFallback = Partial>> +// Selects and formats the schema data the docs need. Runs in the build step. export default async function processSchemas( idl: Buffer | string, previewsPerVersion: PreviewInfo[], @@ -521,17 +520,18 @@ export default async function processSchemas( } } - // Normalize unknown categories (e.g. `:checks`, `:search`, `:packages`, - // `:security_advisories`) to `other`. The upstream gh/gh allowlist permits - // many categories that docs-internal hasn't yet built per-category landing - // pages for; without this fallback those types would be silently dropped - // by `writeCategoryFiles` (which only emits files for slugs in CATEGORIES) - // and their redirects would 404. Once a page exists for a category, add it - // to CATEGORIES in src/graphql/lib/categories.ts and types will move out - // of `other` on the next sync. - // Resolver used to populate the top-level `.category` field on every - // processed item. The bucketer reads `.category` to split the schema into - // per-category files and to rewrite cross-reference hrefs. + // Populates the top-level `.category` field on every processed item. The + // bucketer reads `.category` to split the schema into per-category files and + // to rewrite cross-reference hrefs. + // + // Unknown categories (e.g. `:checks`, `:search`, `:packages`, + // `:security_advisories`) normalize to `other`. The upstream gh/gh allowlist + // permits many categories that docs-internal has not built per-category + // landing pages for; without this fallback those types would be silently + // dropped by `writeCategoryFiles` (which only emits files for slugs in + // CATEGORIES) and their redirects would 404. Once a page exists for a + // category, add it to CATEGORIES in src/graphql/lib/categories.ts and types + // will move out of `other` on the next sync. const resolveCategory = (typeId: string): string => { const cat = typeCategoryMap.get(typeId) ?? fallbackTypeMap[typeId] ?? OTHER_CATEGORY return isValidCategory(cat) ? cat : OTHER_CATEGORY @@ -556,7 +556,6 @@ export default async function processSchemas( await Promise.all( schemaAST.definitions.map(async (def: DefinitionNode) => { - // QUERIES if (def.kind === 'ObjectTypeDefinition' && def.name.value === 'Query') { await Promise.all( (def.fields || []).map(async (field: FieldDefinitionNode) => { @@ -627,7 +626,6 @@ export default async function processSchemas( return } - // MUTATIONS if (def.kind === 'ObjectTypeDefinition' && def.name.value === 'Mutation') { await Promise.all( (def.fields || []).map(async (field: FieldDefinitionNode) => { @@ -732,7 +730,6 @@ export default async function processSchemas( return } - // OBJECTS if (def.kind === 'ObjectTypeDefinition') { // objects ending with 'Payload' are only used to derive mutation values // they are not included in the objects docs @@ -817,7 +814,6 @@ export default async function processSchemas( return } - // INTERFACES if (def.kind === 'InterfaceTypeDefinition') { const graphqlInterface: Partial = {} const interfaceFields: FieldInfo[] = [] @@ -882,7 +878,6 @@ export default async function processSchemas( return } - // ENUMS if (def.kind === 'EnumTypeDefinition') { const graphqlEnum: Partial = {} const enumValues: EnumValueInfo[] = [] @@ -922,7 +917,6 @@ export default async function processSchemas( return } - // UNIONS if (def.kind === 'UnionTypeDefinition') { const union: Partial = {} const possibleTypes: PossibleTypeInfo[] = [] @@ -1027,7 +1021,6 @@ export default async function processSchemas( return } - // SCALARS if (def.kind === 'ScalarTypeDefinition') { const scalar: ScalarInfo = { name: def.name.value, @@ -1058,7 +1051,6 @@ export default async function processSchemas( // add non-schema scalars and sort all scalars alphabetically data.scalars = sortBy(data.scalars.concat(externalScalars), 'name') - // sort all the types alphabetically data.queries = sortBy(data.queries, 'name') data.mutations = sortBy(data.mutations, 'name') data.objects = sortBy(data.objects, 'name') From dd5e7bc8df630940d8f6fedf0c97a1968b16b652 Mon Sep 17 00:00:00 2001 From: Kevin Heis Date: Tue, 15 Sep 2026 20:16:54 +0000 Subject: [PATCH 04/16] Clean up comments in src/links/scripts (#63272) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 17588eeb-788a-4f82-9d36-b5079fb36521 --- src/links/scripts/action-injections.ts | 6 +- .../scripts/check-github-github-links.ts | 7 +-- src/links/scripts/check-links-external.ts | 58 ++----------------- src/links/scripts/check-links-internal.ts | 44 +++++--------- src/links/scripts/check-links-pr.ts | 29 +--------- src/links/scripts/debug-time-taken.ts | 4 +- src/links/scripts/upload-artifact.ts | 8 +-- .../post-pr-comment.ts | 10 ++-- .../validate.ts | 2 +- 9 files changed, 37 insertions(+), 131 deletions(-) diff --git a/src/links/scripts/action-injections.ts b/src/links/scripts/action-injections.ts index 928bc6996ddb..9764df2ce79c 100644 --- a/src/links/scripts/action-injections.ts +++ b/src/links/scripts/action-injections.ts @@ -1,7 +1,5 @@ -/* - * Dependency injection for scripts that call .github/actions/ code - * Replaces action platform specific functionality with local machine functionality - */ +// Dependency injection for scripts that call .github/actions/ code. +// Swaps the Actions-platform pieces for local-machine equivalents. import fs from 'fs' import path from 'path' diff --git a/src/links/scripts/check-github-github-links.ts b/src/links/scripts/check-github-github-links.ts index dd612a304b20..e610830834c8 100755 --- a/src/links/scripts/check-github-github-links.ts +++ b/src/links/scripts/check-github-github-links.ts @@ -51,9 +51,8 @@ main(program.opts(), program.args) const retryConfiguration = { limit: 3, } -// According to our Datadog metrics, the *average* time for the -// the 'archive_enterprise_proxy' metric is ~70ms (excluding spikes) -// which much less than 500ms. +// Datadog puts the average time for the `archive_enterprise_proxy` metric at +// around 70ms, excluding spikes, well under the 3s request timeout below. const timeoutConfiguration = { request: 3000, } @@ -87,7 +86,7 @@ async function main(opts: MainOptions, args: string[]) { ) await fs.writeFile('/tmp/foundFiles.json', JSON.stringify(foundFiles, undefined, 2), 'utf-8') } - const searchFiles = [...new Set(foundFiles)] // filters out dupes + const searchFiles = [...new Set(foundFiles)] .filter((file) => endsWithAny(['.rb', '.yml', '.yaml', '.txt', '.pdf', '.erb', '.js'], file)) .filter( (file) => diff --git a/src/links/scripts/check-links-external.ts b/src/links/scripts/check-links-external.ts index fad3f210f0be..915c2360904f 100644 --- a/src/links/scripts/check-links-external.ts +++ b/src/links/scripts/check-links-external.ts @@ -35,17 +35,14 @@ import github from '@/workflows/github' import excludedLinks from '@/links/lib/excluded-links' import * as coreLib from '@actions/core' -// Cache configuration const CACHE_FILE = process.env.EXTERNAL_LINK_CACHE_FILE || 'external-link-cache.json' const CACHE_MAX_AGE_DAYS = parseInt(process.env.CACHE_MAX_AGE_DAYS || '7', 10) const CACHE_MAX_AGE_MS = CACHE_MAX_AGE_DAYS * 24 * 60 * 60 * 1000 -// Request configuration -const REQUEST_TIMEOUT_MS = 30000 // 30 seconds -const REQUEST_DELAY_MS = 100 // 100ms between requests to avoid rate limiting -const DEFAULT_DOMAIN_CONCURRENCY = 10 // Process this many domains in parallel +const REQUEST_TIMEOUT_MS = 30000 +const REQUEST_DELAY_MS = 100 // Avoids rate limiting a single domain. +const DEFAULT_DOMAIN_CONCURRENCY = 10 -// Create a set for fast lookups of excluded links const excludedLinksSet = new Set(excludedLinks.map(({ is }) => is).filter(Boolean)) const excludedLinksPrefixes = excludedLinks.map(({ startsWith }) => startsWith).filter(Boolean) @@ -54,7 +51,6 @@ function isExcludedLink(href: string): boolean { return excludedLinksPrefixes.some((prefix) => prefix && href.startsWith(prefix)) } -// Cache type interface CacheEntry { timestamp: number ok: boolean @@ -81,9 +77,7 @@ interface LinkOccurrence { * are treated as the same URL. */ function normalizeUrl(href: string): string { - // Remove fragment const withoutFragment = href.split('#')[0] - // Remove trailing slash only for origin/root URLs try { const parsed = new URL(withoutFragment) if (parsed.pathname === '/' && !parsed.search) { @@ -103,22 +97,14 @@ function isDocsGithubUrl(url: string): boolean { } } -/** - * Sleep for a given number of milliseconds - */ function sleep(ms: number): Promise { return new Promise((resolve) => setTimeout(resolve, ms)) } -/** - * Check a single external URL - * Uses HEAD first, falls back to GET if HEAD returns 4xx/5xx - */ async function checkUrl( url: string, cache: CacheData, ): Promise<{ ok: boolean; statusCode?: number; error?: string; cached: boolean }> { - // Check cache first const cached = cache.urls[url] if (cached) { const age = Date.now() - cached.timestamp @@ -138,7 +124,6 @@ async function checkUrl( } if (!response) { - // Timeout or network error return { ok: false, error: 'Request timed out or failed', cached: false } } @@ -149,7 +134,6 @@ async function checkUrl( cached: false, } - // Update cache cache.urls[url] = { timestamp: Date.now(), ok: result.ok, @@ -160,9 +144,6 @@ async function checkUrl( return result } -/** - * Fetch with timeout, returns null on error - */ async function fetchWithTimeout( url: string, method: 'HEAD' | 'GET', @@ -218,7 +199,6 @@ async function checkGithubRepoUrl( cached: boolean fallbackAllowed?: boolean }> { - // Check cache first const cached = cache.urls[url] if (cached) { const age = Date.now() - cached.timestamp @@ -314,13 +294,9 @@ async function checkGithubRepoUrl( } } -/** - * Extract all external links from content files - */ async function extractAllExternalLinks(): Promise> { const links = new Map() - // Find all Markdown files const files = await glob('content/**/*.md', { ignore: '**/README.md' }) console.log(`Found ${files.length} Markdown files to scan`) @@ -332,17 +308,15 @@ async function extractAllExternalLinks(): Promise> const result = extractLinksFromMarkdown(content) const fileMs = Date.now() - fileStart - // Warn if a single file takes longer than 1 second (possible regex issue) + // A slow file may mean a pathological regex in the extractor. if (fileMs > 1000) { console.warn(` āš ļø Slow extraction: ${file} took ${(fileMs / 1000).toFixed(1)}s`) } for (const link of result.externalLinks) { - // Only check HTTPS links if (!link.href.startsWith('https://')) continue if (isExcludedLink(link.href)) continue - // Normalize URL (remove anchors and trailing slashes for checking) const url = normalizeUrl(link.href) if (!links.has(url)) { @@ -363,9 +337,6 @@ async function extractAllExternalLinks(): Promise> return links } -/** - * Main entry point - */ async function main() { program .name('check-links-external') @@ -386,12 +357,10 @@ async function main() { console.log(chalk.blue('🌐 External Link Checker')) console.log('') - // Load cache const defaultData: CacheData = { urls: {} } const db = await JSONFilePreset(CACHE_FILE, defaultData) await db.read() - // Report cache stats const now = Date.now() let freshCount = 0 let staleCount = 0 @@ -405,12 +374,11 @@ async function main() { console.log(`Cache: ${freshCount} fresh, ${staleCount} stale entries`) console.log('') - // Extract all external links console.log('Extracting external links from content files...') const allLinks = await extractAllExternalLinks() - // Separate docs.github.com links — they're self-referential (this repo IS the docs site) - // and will be reported separately as candidates for conversion to internal links. + // Separate docs.github.com links. They're self-referential, since this repo is the docs + // site, and get reported separately as candidates for conversion to internal links. const selfReferentialLinks = new Map() for (const [url, occurrences] of allLinks) { if (isDocsGithubUrl(url)) { @@ -425,7 +393,6 @@ async function main() { if (options.dryRun) { console.log('Dry run mode - not checking URLs') - // Show sample of URLs const sample = Array.from(allLinks.keys()).slice(0, 20) for (const url of sample) { console.log(` ${url}`) @@ -436,7 +403,6 @@ async function main() { process.exit(0) } - // Check URLs const brokenLinks: BrokenLink[] = [] let checkedCount = 0 let cachedCount = 0 @@ -459,7 +425,6 @@ async function main() { if (!urlsByDomain.has(hostname)) urlsByDomain.set(hostname, []) urlsByDomain.get(hostname)!.push(url) } catch { - // Record malformed URLs as broken links malformedCount++ checkedCount++ const occurrences = allLinks.get(url)! @@ -529,7 +494,6 @@ async function main() { console.log(` āœ… ${url}`) } - // Progress update every 100 URLs if (checkedCount % 100 === 0) { const elapsed = ((Date.now() - startTime) / 1000).toFixed(0) const rate = (checkedCount / (Date.now() - startTime)) * 1000 * 60 @@ -540,7 +504,6 @@ async function main() { ) } - // Small delay between requests to the same domain to avoid rate limiting if (!result.cached) { await sleep(REQUEST_DELAY_MS) } @@ -564,17 +527,14 @@ async function main() { await Promise.all(workers.map(runWorker)) - // Save cache await db.write() - // Report results const duration = ((Date.now() - startTime) / 1000).toFixed(1) console.log('') console.log( chalk.blue(`Checked ${checkedCount} URLs in ${duration}s (${cachedCount} from cache)`), ) - // Build self-referential BrokenLink list for the report const selfReferentialBrokenLinks: BrokenLink[] = [] for (const occurrences of selfReferentialLinks.values()) { for (const occ of occurrences) { @@ -587,7 +547,6 @@ async function main() { process.exit(0) } - // Generate report const report = generateExternalLinkReport(brokenLinks, { actionUrl: process.env.ACTION_RUN_URL, selfReferentialLinks: selfReferentialBrokenLinks, @@ -605,7 +564,6 @@ async function main() { console.log(chalk.red(`āŒ ${report.uniqueTargets} domain(s) with broken links`)) console.log(chalk.red(` ${report.totalOccurrences} total occurrence(s)`)) - // Show summary by domain console.log('') console.log('Broken links by domain:') for (const group of report.groups.slice(0, 10)) { @@ -616,12 +574,10 @@ async function main() { } } - // Write artifact const markdown = reportToMarkdown(report, true) await uploadArtifact('external-link-report.md', markdown) await uploadArtifact('external-link-report.json', JSON.stringify(report, null, 2)) - // Create issue report if configured const createReport = process.env.CREATE_REPORT === 'true' const reportRepository = process.env.REPORT_REPOSITORY || 'github/docs-content' @@ -642,7 +598,6 @@ async function main() { reportLabel, }) - // Link to previous reports await linkReports({ core: coreLib, octokit, @@ -656,7 +611,6 @@ async function main() { } } -// Run if invoked directly ;(async () => { try { await main() diff --git a/src/links/scripts/check-links-internal.ts b/src/links/scripts/check-links-internal.ts index c298d98066ff..3de89ec9b7dc 100644 --- a/src/links/scripts/check-links-internal.ts +++ b/src/links/scripts/check-links-internal.ts @@ -56,7 +56,6 @@ import { getFeaturesByVersion } from '@/versions/middleware/features' import type { Page, Permalink, Context } from '@/types' import * as coreLib from '@actions/core' -// Create a set for fast lookups of excluded links const excludedLinksSet = new Set(excludedLinks.map(({ is }) => is).filter(Boolean)) const excludedLinksPrefixes = excludedLinks.map(({ startsWith }) => startsWith).filter(Boolean) @@ -78,7 +77,7 @@ interface CheckResult { * parsing are relative to the body. Adding this offset converts them to * actual file line numbers. * - * Results are cached by fullPath — the file is read once per page across + * Results are cached by fullPath, so the file is read once per page across * both getLinksFromMarkdown() and checkAnchorsOnPage(). */ const frontmatterLineOffsetCache = new Map() @@ -103,7 +102,7 @@ function getFrontmatterLineOffset(fullPath: string): number { } } } catch { - // ignore — fall back to no offset + // Ignore: fall back to no offset. } frontmatterLineOffsetCache.set(fullPath, offset) @@ -155,7 +154,7 @@ async function getLinksFromMarkdown( // extractLinksWithLiquid will produce, without affecting line positions. canonicalHref = (await renderLiquidFn(canonicalHref, context)).trim() } catch { - // fall back to raw href if rendering fails + // Fall back to the raw href if rendering fails. } } const existing = rawLinesByHref.get(canonicalHref) @@ -182,7 +181,7 @@ async function getLinksFromMarkdown( } } } catch { - // skip — can't resolve line number for this link + // Skip: can't resolve a line number for this link. } } } @@ -236,7 +235,7 @@ function checkAnchorsFromHeadings( } // Check only the anchor links that actually appear in the Liquid-rendered output - // (respects {% ifversion %} gates — links in non-applicable blocks are not checked). + // (respects {% ifversion %} gates, so links in non-applicable blocks are not checked). const brokenAnchors: BrokenLink[] = [] for (const link of renderedResult.anchorLinks) { const { href } = link @@ -288,7 +287,7 @@ async function checkPage( // Compute this page's heading anchor IDs once from the Liquid-rendered markdown. // Autogenerated pages (REST/GraphQL/webhooks) derive their anchors from OpenAPI - // operation IDs, not markdown headings, so we can't compute them here — leave them + // operation IDs, not markdown headings, so we can't compute them here. Leave them // out of the cache so links into them are never flagged (they resolve at runtime). // Skip the work entirely when anchor checking is disabled: nothing downstream reads // the heading cache in that mode. @@ -300,7 +299,6 @@ async function checkPage( for (const link of links) { if (isExcludedLink(link.href)) continue - // Check if this is an asset link (images, etc.) - verify file exists on disk if (isAssetLink(link.href)) { if (!checkAssetLink(link.href)) { brokenLinks.push({ @@ -392,7 +390,6 @@ async function checkVersion( throw new Error(`Unknown version: ${version}`) } - // Filter pages for this version and language const relevantPages = pageList.filter((page) => { if (page.languageCode !== language) return false if (!page.applicableVersions?.includes(version)) return false @@ -403,7 +400,8 @@ async function checkVersion( ` Checking ${relevantPages.length} pages for ${version}/${language} (concurrency: ${options.concurrency})`, ) - // Build a base context once per version — feature flags and version info are the same for all pages. + // Build a base context once per version: feature flags and version info are the same + // for all pages. // Each page gets a shallow copy so concurrent tasks don't share the mutable `page` property. const baseContext = { currentVersion: version, @@ -421,10 +419,10 @@ async function checkVersion( let totalLinksChecked = 0 // Cross-page anchor validation is a two-pass process within the version: - // pass 1 — render every page, caching its heading IDs and collecting the - // cross-page anchor links it contains (target may not be rendered yet) - // pass 2 — after all pages are rendered, validate each collected anchor against - // the now-complete heading cache + // pass 1: render every page, caching its heading IDs and collecting the + // cross-page anchor links it contains (target may not be rendered yet) + // pass 2: after all pages are rendered, validate each collected anchor against + // the now-complete heading cache // The cache is keyed by pageMap key (lang + version + path). A link whose target // resolves to a different version isn't in this run's cache and is skipped here; // it's validated when the workflow runs the checker for that target version. @@ -432,7 +430,7 @@ async function checkVersion( const pendingCrossPageAnchors: PendingCrossPageAnchor[] = [] // Bounded concurrency: process up to `options.concurrency` pages simultaneously. - // All workers drain from the same shared iterator — no page is processed twice. + // All workers drain from the same shared iterator, so no page is processed twice. const queue = relevantPages.entries() async function worker() { @@ -483,9 +481,6 @@ async function checkVersion( } } -/** - * Main entry point - */ async function main() { program .name('check-links-internal') @@ -508,7 +503,6 @@ async function main() { console.log(chalk.blue('šŸ”— Internal Link Checker')) console.log('') - // Determine version and language to check const version = options.version || process.env.VERSION const language = options.language || process.env.LANGUAGE || 'en' const checkAnchors = options.checkAnchors && process.env.CHECK_ANCHORS !== 'false' @@ -536,13 +530,11 @@ async function main() { console.log(`Check anchors: ${checkAnchors}`) console.log('') - // Load page data console.log('Loading page data...') const { pages: pageMap, redirects, pageList } = await warmServer([language]) console.log(`Loaded ${pageList.length} pages, ${Object.keys(redirects).length} redirects`) console.log('') - // Run the check const concurrency = Math.max(1, parseInt(process.env.CONCURRENCY || options.concurrency, 10)) const result = await checkVersion(version, language, pageList, pageMap, redirects, { checkAnchors, @@ -550,7 +542,6 @@ async function main() { concurrency, }) - // Report results const duration = ((Date.now() - startTime) / 1000).toFixed(1) console.log('') console.log( @@ -566,7 +557,6 @@ async function main() { process.exit(0) } - // Generate report const report = generateInternalLinkReport(allBrokenLinks, { actionUrl: process.env.ACTION_RUN_URL, version, @@ -578,12 +568,10 @@ async function main() { console.log(chalk.red(`āŒ ${result.brokenLinks.length} broken link(s)`)) console.log(chalk.yellow(`āš ļø ${result.redirectLinks.length} redirect(s) to update`)) - // Write artifact const markdown = reportToMarkdown(report) await uploadArtifact(`link-report-${version}-${language}.md`, markdown) await uploadArtifact(`link-report-${version}-${language}.json`, JSON.stringify(report, null, 2)) - // Create issue report if configured const createReport = process.env.CREATE_REPORT === 'true' const reportRepository = process.env.REPORT_REPOSITORY || 'github/docs-content' @@ -604,7 +592,6 @@ async function main() { reportLabel, }) - // Link to previous reports await linkReports({ core: coreLib, octokit, @@ -617,8 +604,8 @@ async function main() { console.log(`Created report issue: ${newReport.html_url}`) } - // Don't exit with error - the issue report is the mechanism for docs-content to act on broken links - // Exiting with error would trigger docs-alerts which only engineering monitors + // Don't exit with an error. The issue report is how docs-content hears about broken + // links, whereas a failing exit code only triggers docs-alerts. console.log('') console.log( chalk.yellow( @@ -627,7 +614,6 @@ async function main() { ) } -// Run if invoked directly ;(async () => { try { await main() diff --git a/src/links/scripts/check-links-pr.ts b/src/links/scripts/check-links-pr.ts index cebeefc61706..cab25e865218 100644 --- a/src/links/scripts/check-links-pr.ts +++ b/src/links/scripts/check-links-pr.ts @@ -57,9 +57,6 @@ interface CheckResult { totalLinksChecked: number } -/** - * Check all internal links in a single file - */ async function checkFile( filePath: string, pageMap: Record, @@ -71,7 +68,6 @@ async function checkFile( const redirectLinks: BrokenLink[] = [] let totalLinksChecked = 0 - // Read file content let content: string try { content = fs.readFileSync(filePath, 'utf-8') @@ -80,17 +76,14 @@ async function checkFile( return { file: filePath, brokenLinks, redirectLinks, totalLinksChecked } } - // Create context for Liquid rendering const context = createLiquidContext(version, language) - // Extract links after Liquid rendering const { internalLinks } = await extractLinksWithLiquid(content, context) - // Check each internal link (exclude imageLinks - they're static assets, not docs pages) + // imageLinks are excluded: they're static assets, not docs pages. totalLinksChecked = internalLinks.length for (const link of internalLinks) { - // Check if this is an asset link (images, etc.) - verify file exists on disk if (isAssetLink(link.href)) { if (!checkAssetLink(link.href)) { brokenLinks.push({ @@ -181,7 +174,7 @@ async function checkFileAnchors( if (link.fragment === 'top') continue // resolveLinkKeyForVersion only returns direct (non-redirect) page hits, so - // redirects, archived versions, and broken paths fall out here — they're not + // redirects, archived versions, and broken paths fall out here. They're not // anchor-scope flaws. Unversioned hrefs are retried against the version the source // page is currently rendered in, so GHEC/GHES-only targets resolve too. const targetKey = resolveLinkKeyForVersion(link.href, version, pageMap) @@ -235,7 +228,6 @@ function getChangedFiles(cliFiles?: string[]): string[] { return cliFiles } - // Check environment variable (from GitHub Actions) const filesChanged = process.env.FILES_CHANGED if (filesChanged) { // Try parsing as JSON first @@ -253,12 +245,8 @@ function getChangedFiles(cliFiles?: string[]): string[] { return [] } -/** - * Filter to only content/data files that might contain links - */ function filterContentFiles(files: string[]): string[] { return files.filter((file) => { - // Only check Markdown files in content/ or data/ if (!file.endsWith('.md')) return false // Skip README.md files. They're developer docs, not published pages, and use // repo-relative paths (e.g. /src/...) that aren't valid site links. @@ -268,9 +256,6 @@ function filterContentFiles(files: string[]): string[] { }) } -/** - * Post a comment on the PR with broken link results - */ async function commentOnPR( brokenLinks: BrokenLink[], brokenAnchors: CrossPageAnchorFlaw[], @@ -346,9 +331,6 @@ async function commentOnPR( } } -/** - * Main entry point - */ async function main() { program .name('check-links-pr') @@ -364,7 +346,6 @@ async function main() { console.log(chalk.blue('šŸ”— PR Link Checker')) console.log('') - // Get files to check let files = getChangedFiles(options.files) if (options.all) { @@ -377,7 +358,6 @@ async function main() { process.exit(0) } - // Filter to content files only const contentFiles = filterContentFiles(files) if (contentFiles.length === 0) { console.log('No content files in changed files. Exiting.') @@ -386,7 +366,6 @@ async function main() { console.log(`Checking ${contentFiles.length} file(s)...`) - // Load page data console.log('Loading page data...') const { pages: pageMap, pageList, redirects } = await warmServer(['en']) console.log( @@ -405,7 +384,6 @@ async function main() { const checkAnchors = process.env.CHECK_ANCHORS !== 'false' const headingCache = new Map>() - // Check each file const allBrokenLinks: BrokenLink[] = [] const allRedirectLinks: BrokenLink[] = [] const allBrokenAnchors: CrossPageAnchorFlaw[] = [] @@ -437,7 +415,6 @@ async function main() { } } - // Report results const duration = ((Date.now() - startTime) / 1000).toFixed(1) console.log('') console.log(chalk.blue(`Checked ${totalLinksChecked} links in ${duration}s`)) @@ -459,7 +436,6 @@ async function main() { process.exit(0) } - // Group and display results if (allBrokenLinks.length > 0) { console.log('') console.log(chalk.red(`āŒ ${allBrokenLinks.length} broken link(s):`)) @@ -537,7 +513,6 @@ async function main() { } } -// Run if invoked directly ;(async () => { try { await main() diff --git a/src/links/scripts/debug-time-taken.ts b/src/links/scripts/debug-time-taken.ts index c8e7d53cd8ce..f6b11dc15172 100644 --- a/src/links/scripts/debug-time-taken.ts +++ b/src/links/scripts/debug-time-taken.ts @@ -2,7 +2,7 @@ const timeInstances = new Map() type CoreLike = { warning: (message: string | Error) => void; debug: (message: string) => void } -/* Meant to be called before debugTimeEnd with the same instanceName to behave like console.time() */ +// Pair with debugTimeEnd using the same instanceName, like console.time(). export function debugTimeStart(core: CoreLike, instanceName: string) { if (timeInstances.has(instanceName)) { core.warning(`instanceName: ${instanceName} has already been used for a debug instance.`) @@ -12,7 +12,7 @@ export function debugTimeStart(core: CoreLike, instanceName: string) { timeInstances.set(instanceName, new Date()) } -/* Meant to be called after debugTimeStart with the same instanceName to behave like console.timeEnd() */ +// Pair with debugTimeStart using the same instanceName, like console.timeEnd(). export function debugTimeEnd(core: CoreLike, instanceName: string) { if (!timeInstances.has(instanceName)) { core.warning( diff --git a/src/links/scripts/upload-artifact.ts b/src/links/scripts/upload-artifact.ts index 3b1fba48dd0f..b37beb31103a 100644 --- a/src/links/scripts/upload-artifact.ts +++ b/src/links/scripts/upload-artifact.ts @@ -1,11 +1,7 @@ import fs from 'fs' -/* Writes string to file to be uploaded as an action artifact. - * Useful for debugging or passing results to downstream action - * - * @param {string} name - name of artifact - * @param {string} contents - string contents of artifact - */ +// Writes a string to a file for the workflow to upload as an artifact. +// Useful for debugging, or for passing results to a downstream action. export async function uploadArtifact(name: string, contents: string) { if (!fs.existsSync('./artifacts')) { fs.mkdirSync('./artifacts/') diff --git a/src/links/scripts/validate-github-github-docs-urls/post-pr-comment.ts b/src/links/scripts/validate-github-github-docs-urls/post-pr-comment.ts index c02e4e368ade..4b2d38a1786b 100644 --- a/src/links/scripts/validate-github-github-docs-urls/post-pr-comment.ts +++ b/src/links/scripts/validate-github-github-docs-urls/post-pr-comment.ts @@ -21,7 +21,6 @@ type PostPRCommentOptions = { // This function is designed to be able to run and potentially do nothing. export async function postPRComment(filePath: string, options: PostPRCommentOptions) { - // Check the options before we even begin if (!options.dryRun) { if (!options.issueNumber) { throw new Error( @@ -258,11 +257,10 @@ async function updateIssueComment( } } - // It found no comment to *edit*, so it create *create* a new comment. - // But `onlyIfAlreadyPosted` is true, so it does nothing. - // This is convenient when might have, during the lifetime of a PR, - // posted a comment, then committed more changes, and then realize - // that what was posted previously is no long the case. + // There is no comment to edit, so this would create one, but `onlyIfAlreadyPosted` + // is true so it does nothing. That matters when a PR previously had failing checks, + // got more commits, and no longer does: the old comment should be updated, but a + // PR that never failed should not gain one. if (onlyIfAlreadyPosted) { console.warn(`Deliberately not creating a new comment`) return diff --git a/src/links/scripts/validate-github-github-docs-urls/validate.ts b/src/links/scripts/validate-github-github-docs-urls/validate.ts index 8c5824c1d386..f6e9fb5ef081 100644 --- a/src/links/scripts/validate-github-github-docs-urls/validate.ts +++ b/src/links/scripts/validate-github-github-docs-urls/validate.ts @@ -27,7 +27,7 @@ export async function validate(filePath: string, options: Options) { console.log(prefix, `āœ… ${check.url} (${check.identifier})`) } } else { - // This is a 404 - page not found + // A 404: the page does not exist. if (options.ignoreNotFound) { console.log(prefix, `āš ļø ${check.url} (${check.identifier})`) } else { From f1114fe1eba491c5cf6303094ef0d7458cc7662a Mon Sep 17 00:00:00 2001 From: Kevin Heis Date: Tue, 15 Sep 2026 20:16:59 +0000 Subject: [PATCH 05/16] Trim excessive comments in src/languages (#63268) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 17588eeb-788a-4f82-9d36-b5079fb36521 --- src/languages/components/LanguagePicker.tsx | 2 +- src/languages/lib/get-english-headings.ts | 1 - src/languages/lib/languages-server.ts | 21 ++++++++----------- src/languages/lib/languages.ts | 2 +- src/languages/lib/render-with-fallback.ts | 9 -------- src/languages/lib/translation-utils.ts | 7 +------ .../scripts/count-translation-corruptions.ts | 12 +++++------ .../tests/count-translation-corruptions.ts | 4 ++-- .../tests/translation-error-comments.ts | 7 +------ 9 files changed, 21 insertions(+), 44 deletions(-) diff --git a/src/languages/components/LanguagePicker.tsx b/src/languages/components/LanguagePicker.tsx index 05c46694d8a9..c6c69b2b9a98 100644 --- a/src/languages/components/LanguagePicker.tsx +++ b/src/languages/components/LanguagePicker.tsx @@ -25,7 +25,7 @@ export const LanguagePicker = ({ xs, mediumOrLower }: Props) => { // that are available. // Also, if the current context has a page and that page has own ideas // about which languages it's available in (e.g. early-access) - // it would already have been paired down. + // it would already have been pared down. const langs = Object.values(languages) if (langs.length < 2) { diff --git a/src/languages/lib/get-english-headings.ts b/src/languages/lib/get-english-headings.ts index 470286078a5d..fa8362f0f72b 100644 --- a/src/languages/lib/get-english-headings.ts +++ b/src/languages/lib/get-english-headings.ts @@ -54,7 +54,6 @@ export default function getEnglishHeadings( const englishHeadings = getHeadings(englishPage.markdown) if (!englishHeadings.length) return - // return a map from translation:English const headingMap: Record = {} for (let i = 0; i < translatedHeadings.length; i++) { const k = translatedHeadings[i] diff --git a/src/languages/lib/languages-server.ts b/src/languages/lib/languages-server.ts index da2fa046e3a5..fcd6b84adc23 100644 --- a/src/languages/lib/languages-server.ts +++ b/src/languages/lib/languages-server.ts @@ -1,10 +1,8 @@ -/* -This file adds the following properties to languages in ./languages.ts: -- dir: string - -This file will also remove languages for local development and tests -that have not be specified by ENABLED_LANGUAGES -*/ +// Adds a `dir` property to each language in ./languages.ts. +// +// Also narrows the set of languages, in this order: to whatever has a +// directory under TRANSLATIONS_FIXTURE_ROOT, else to ENABLED_LANGUAGES, else +// to English alone when NODE_ENV is 'test'. import path from 'path' import fs from 'fs' @@ -15,7 +13,6 @@ import { languages as baseLanguages, type Language as BaseLanguage } from './lan dotenv.config({ quiet: true }) -// Server-side language extends base language with required dir property export interface Language extends BaseLanguage { dir: string } @@ -45,7 +42,6 @@ function getRoot(languageCode: string): string { return path.join(TRANSLATIONS_ROOT, languageCode) } -// Build server languages with directory paths const allLanguagesWithDirs: Languages = {} for (const [code, lang] of Object.entries(baseLanguages)) { allLanguagesWithDirs[code] = { @@ -86,9 +82,10 @@ export const languageKeys: string[] = Object.keys(languages) export const languagePrefixPathRegex: RegExp = new RegExp(`^/(${languageKeys.join('|')})(/|$)`) -/** Return true if the URL is something like /en/foo or /ja but return false - * if it's something like /foo or /foo/bar or /fr (because French (fr) - * is currently not an active language) +/** Return true if the URL starts with a currently active language code, e.g. + * /en/foo or /ja. Returns false for /foo or /foo/bar. Which codes count as + * active depends on TRANSLATIONS_FIXTURE_ROOT, ENABLED_LANGUAGES, and + * NODE_ENV, so this varies between production, local dev, and tests. */ export function pathLanguagePrefixed(urlPath: string): boolean { return languagePrefixPathRegex.test(urlPath) diff --git a/src/languages/lib/languages.ts b/src/languages/lib/languages.ts index ba319f697b4b..d8d16cb3278c 100644 --- a/src/languages/lib/languages.ts +++ b/src/languages/lib/languages.ts @@ -1,5 +1,5 @@ // See also languages-schema.ts -// Nota bene: If you are adding a new language, +// Note: if you are adding a new language, // change accept-language handling in CDN config as well. /** diff --git a/src/languages/lib/render-with-fallback.ts b/src/languages/lib/render-with-fallback.ts index a1bcd4f1b17e..d07e57645436 100644 --- a/src/languages/lib/render-with-fallback.ts +++ b/src/languages/lib/render-with-fallback.ts @@ -56,14 +56,11 @@ export function createTranslationFallbackComment(error: Error, property: string) const errorType = error.name || 'UnknownError' const errorDetails: string[] = [] - // Add basic error information errorDetails.push(`TRANSLATION_FALLBACK`) errorDetails.push(`prop=${property}`) errorDetails.push(`type=${errorType}`) - // Extract detailed error information based on error type if (isLiquidError(error)) { - // For Liquid errors, we can extract rich debugging information if (error.token) { if (error.token.file) { errorDetails.push(`file=${error.token.file}`) @@ -75,13 +72,10 @@ export function createTranslationFallbackComment(error: Error, property: string) } } - // Include the original error message if available const originalMessage = error.originalError?.message || error.message if (originalMessage) { - // Clean up the message but keep useful information let cleanMessage = originalMessage.replace(/\n/g, ' ').replace(/\s+/g, ' ').trim() - // Limit message length to keep comment manageable if (cleanMessage.length > 200) { cleanMessage = `${cleanMessage.substring(0, 200)}...` } @@ -89,7 +83,6 @@ export function createTranslationFallbackComment(error: Error, property: string) errorDetails.push(`msg="${cleanMessage.replace(/"/g, "'")}"`) } } else if (isAutotitleError(error)) { - // For AUTOTITLE errors, include the error message if (error.message) { const cleanMessage = error.message .replace(/\n/g, ' ') @@ -99,7 +92,6 @@ export function createTranslationFallbackComment(error: Error, property: string) errorDetails.push(`msg="${cleanMessage.replace(/"/g, "'")}"`) } } else if (isEmptyTitleError(error)) { - // For empty title errors, include the property info errorDetails.push(`msg="Content became empty after rendering"`) } @@ -148,7 +140,6 @@ export async function renderContentWithFallback( // like `data.ts` that uses `environment.scope.currentLanguage` const enContext = Object.assign({}, context, { currentLanguage: 'en' }) - // Render the English fallback content const fallbackContent = await renderContent(englishTemplate, enContext, options) // Add HTML comment with error details for non-English languages diff --git a/src/languages/lib/translation-utils.ts b/src/languages/lib/translation-utils.ts index 56f31fdcefd0..2e2273d2866b 100644 --- a/src/languages/lib/translation-utils.ts +++ b/src/languages/lib/translation-utils.ts @@ -50,7 +50,7 @@ export function createTranslationFunctions(uiData: UIStrings, namespaces: string // Try each namespace in order for (const namespace of namespacesArray) { if (!(namespace in uiData)) { - continue // Skip missing namespaces + continue } const deeper = uiData[namespace] if (typeof deeper === 'string') { @@ -105,7 +105,6 @@ export function createTranslationFunctions(uiData: UIStrings, namespaces: string * Enhanced with better error handling for missing keys and defensive fallbacks */ export function translate(uiData: UIStrings, key: string, fallback?: string): string { - // Defensive check for completely missing data if (!uiData || typeof uiData !== 'object') { console.warn(`UI data is missing or corrupted for key "${key}", using fallback`) return getCommonFallback(key, fallback) @@ -116,7 +115,6 @@ export function translate(uiData: UIStrings, key: string, fallback?: string): st } catch (error) { const finalFallback = getCommonFallback(key, fallback) - // Only warn in development if (process.env.NODE_ENV === 'development') { console.warn( `Server translation failed for "${key}":`, @@ -129,9 +127,6 @@ export function translate(uiData: UIStrings, key: string, fallback?: string): st } } -/** - * Get common fallback values for essential UI keys - */ function getCommonFallback(key: string, providedFallback?: string): string { const commonFallbacks: Record = { 'meta.oops': 'Ooops!', diff --git a/src/languages/scripts/count-translation-corruptions.ts b/src/languages/scripts/count-translation-corruptions.ts index d629fa886cab..3e0944935c9b 100644 --- a/src/languages/scripts/count-translation-corruptions.ts +++ b/src/languages/scripts/count-translation-corruptions.ts @@ -206,7 +206,7 @@ async function run( // version it surfaced under (rather than N near-duplicate entries). const byKey = new Map }>() - // Suppress console.warn during rendering — the {% data %} tag warns + // Suppress console.warn during rendering. The {% data %} tag warns // when it can't find translated data, which is expected noise. const originalWarn = console.warn console.warn = () => {} @@ -242,9 +242,9 @@ async function run( for (const page of site.pageList) { if (page.languageCode !== languageCode) continue - // Only render under versions the page is actually served in (intersected - // with the requested set) — a page is never scraped under a version it - // doesn't apply to, so corruptions there can't break indexing. + // Only render under versions the page is actually served in, intersected + // with the requested set. A page is never scraped under a version it does + // not apply to, so corruptions there cannot break indexing. const pageVersions = versions.filter((v) => page.applicableVersions.includes(v)) if (!pageVersions.length) continue @@ -280,8 +280,8 @@ async function run( relativePath, }) } catch (error) { - // A missing translated file (ENOENT) just means this language hasn't - // translated this reusable yet — skip it silently. Any other error (e.g. + // A missing translated file (ENOENT) just means this language has not + // translated this reusable yet, so skip it silently. Any other error (e.g. // a malformed reusable that breaks correctTranslatedContentStrings) is a // real corruption: record it and keep going rather than crashing the run. if (error instanceof Error) { diff --git a/src/languages/tests/count-translation-corruptions.ts b/src/languages/tests/count-translation-corruptions.ts index b4117e82764e..1a1a8fe2f0e7 100644 --- a/src/languages/tests/count-translation-corruptions.ts +++ b/src/languages/tests/count-translation-corruptions.ts @@ -3,7 +3,7 @@ import { describe, expect, test } from 'vitest' import { allVersions } from '@/versions/lib/all-versions' import { resolveRequestedVersions } from '@/languages/scripts/count-translation-corruptions' -// Pick a real GHES key at runtime rather than hard-coding one — allVersions +// Pick a real GHES key at runtime rather than hard-coding one. allVersions // only contains currently-supported releases, so a literal like // `enterprise-server@3.16` would start failing once it rolls off support. const someGhesVersion = Object.keys(allVersions).find((v) => v.startsWith('enterprise-server@'))! @@ -28,7 +28,7 @@ describe('resolveRequestedVersions', () => { }) test('rejects versions that are not real allVersions keys', () => { - // `enterprise-server@latest` is a common mistake — it is not a real key. + // `enterprise-server@latest` is a common mistake. It is not a real key. expect(() => resolveRequestedVersions('enterprise-server@latest')).toThrow( /Invalid version\(s\): enterprise-server@latest/, ) diff --git a/src/languages/tests/translation-error-comments.ts b/src/languages/tests/translation-error-comments.ts index af5986944f66..5e6d83c266a7 100644 --- a/src/languages/tests/translation-error-comments.ts +++ b/src/languages/tests/translation-error-comments.ts @@ -10,7 +10,6 @@ import { TitleFromAutotitleError } from '@/content-render/unified/rewrite-local- import Page from '@/frame/lib/page' describe('Translation Error Comments', () => { - // Mock renderContent for integration tests let mockRenderContent: MockedFunction< (template: string, context: Record) => string > @@ -179,7 +178,7 @@ describe('Translation Error Comments', () => { }) test('truncates very long error messages', () => { - const longMessage = 'A'.repeat(300) // Very long error message + const longMessage = 'A'.repeat(300) const error = new LiquidError(longMessage, 'ParseError') const result = createTranslationFallbackComment(error, 'rawTitle') @@ -223,7 +222,6 @@ describe('Translation Error Comments', () => { expect(result).toContain('type=ParseError') expect(result).toContain('prop=title') - // Should handle gracefully, might not have msg or have empty msg }) test('cleans up multiline messages', () => { @@ -246,11 +244,9 @@ describe('Translation Error Comments', () => { const result = createTranslationFallbackComment(error, 'rawTitle') - // Should be a proper HTML comment expect(result.startsWith('')).toBe(true) - // Should be on a single line expect(result).not.toContain('\n') }) @@ -281,7 +277,6 @@ describe('Translation Error Comments', () => { const result = createTranslationFallbackComment(error, 'title') - // Should follow the expected structure with all required fields expect(result.startsWith('/g, writeMode ? '' : '', @@ -166,7 +147,6 @@ export async function callEditor( writeMode ? '' : '', ) - // Remove sections marked for removal msg.content = msg.content.replace(/[\s\S]*?/g, '') } diff --git a/src/ai-tools/lib/spaces-utils.ts b/src/ai-tools/lib/spaces-utils.ts index e5859ec5b338..e0f993b3b046 100644 --- a/src/ai-tools/lib/spaces-utils.ts +++ b/src/ai-tools/lib/spaces-utils.ts @@ -1,6 +1,3 @@ -/** - * Copilot Space API response types - */ import { fetchWithRetry, readBodyWithTimeout } from '@/frame/lib/fetch-utils' export interface SpaceResource { @@ -25,9 +22,6 @@ export interface SpaceData { updated_at: string } -/** - * Parse a Copilot Space URL to extract org and space ID - */ export function parseSpaceUrl(url: string): { org: string; id: string } { // Expected format: https://api.github.com/orgs/{org}/copilot-spaces/{id} const match = url.match(/\/orgs\/([^/]+)\/copilot-spaces\/(\d+)/) @@ -44,9 +38,6 @@ export function parseSpaceUrl(url: string): { org: string; id: string } { } } -/** - * Fetch a Copilot Space from the GitHub API - */ export async function fetchCopilotSpace(spaceUrl: string): Promise { const { org, id } = parseSpaceUrl(spaceUrl) const apiUrl = `https://api.github.com/orgs/${org}/copilot-spaces/${id}` @@ -88,26 +79,20 @@ export async function fetchCopilotSpace(spaceUrl: string): Promise { return (await readBodyWithTimeout(response, () => response.json(), 30_000)) as SpaceData } -/** - * Convert a Copilot Space to a markdown prompt file - */ export function convertSpaceToPrompt(space: SpaceData): string { const timestamp = new Date().toISOString() const lines: string[] = [] - // Header with metadata lines.push(``) lines.push(``) lines.push(``) lines.push('') - // General instructions if (space.general_instructions) { lines.push(space.general_instructions.trim()) lines.push('') } - // Add each resource as a context section if (space.resources_attributes && space.resources_attributes.length > 0) { for (const resource of space.resources_attributes) { if (resource.resource_type === 'free_text' && resource.metadata) { diff --git a/src/ai-tools/scripts/ai-tools.ts b/src/ai-tools/scripts/ai-tools.ts index 62f2d3a04cfb..2c66e8c72938 100644 --- a/src/ai-tools/scripts/ai-tools.ts +++ b/src/ai-tools/scripts/ai-tools.ts @@ -19,7 +19,6 @@ dotenv.config({ quiet: true }) const promptDir = getPromptsDir() -// Ensure GitHub token is available ensureGitHubToken() const editorTypes = getAvailableEditorTypes(promptDir) @@ -68,7 +67,7 @@ program .option('-f, --files ', 'One or more content file paths in the content directory') .action((options: CliOptions) => { ;(async () => { - // Handle export-space workflow (standalone, doesn't process files) + // The export-space workflow is standalone and processes no files. if (options.exportSpace) { if (!options.output) { console.error('Error: --export-space requires --output option') @@ -95,13 +94,11 @@ program } } - // Validate mutually exclusive options if (options.space && options.prompt) { console.error('Error: Cannot use both --space and --prompt options') process.exit(1) } - // Files are required for processing workflows if (!options.files || options.files.length === 0) { console.error('Error: --files option is required (unless using --export-space)') process.exit(1) @@ -113,7 +110,7 @@ program let prompts: string[] = [] let promptContent: string | undefined - // Handle Space workflow (in-memory) + // Build the prompt in memory instead of reading a prompt file. if (options.space) { try { spinner.text = 'Fetching Copilot Space...' @@ -130,7 +127,6 @@ program process.exit(1) } } else { - // Handle local prompt workflow prompts = options.prompt || options.refine || [] if (prompts.length === 0) { @@ -140,7 +136,7 @@ program } } - // Validate local prompt types exist (skip for Space workflow) + // A Space prompt is not a local file, so there is nothing to validate. if (!options.space) { const availableEditors = editorTypes for (const editor of prompts) { @@ -168,16 +164,13 @@ program continue } - // Check if it's a directory const isDirectory = fs.statSync(filePath).isDirectory() for (const editorType of prompts) { try { - // For other editor types, process individual files const filesToProcess: string[] = [] if (isDirectory) { - // Find all markdown files in the directory recursively // Use process.cwd() as the root directory for safety const rootDir = fs.realpathSync(process.cwd()) filesToProcess.push(...findMarkdownFiles(filePath, rootDir)) @@ -197,7 +190,7 @@ program const relativePath = path.relative(process.cwd(), fileToProcess) spinner.text = `Processing: ${relativePath}` try { - // Expand Liquid references before processing + // Capture the intro before Liquid expansion rewrites it. let originalIntro = '' if (editorType === 'intro') { const originalContent = fs.readFileSync(fileToProcess, 'utf8') @@ -212,7 +205,6 @@ program let content = fs.readFileSync(fileToProcess, 'utf8') - // For intro prompt, add original intro and enrich context if (editorType === 'intro') { if (originalIntro) { content = `\n\n---\nOriginal intro (unresolved): ${originalIntro}\n---\n\n${content}` @@ -220,7 +212,6 @@ program content = enrichIndexContext(fileToProcess, content) } - // For content-type prompt, skip files that already have contentType if (editorType === 'content-type' && content.includes('contentType:')) { spinner.stop() console.log(`ā­ļø Skipping ${relativePath} (already has contentType)`) @@ -240,24 +231,22 @@ program if (options.write) { if (editorType === 'intro' || editorType === 'content-type') { - // For frontmatter addition/modification, merge properties instead of overwriting entire file + // Merge frontmatter instead of overwriting the whole file. const updatedContent = mergeFrontmatterProperties(fileToProcess, answer) fs.writeFileSync(fileToProcess, updatedContent, 'utf8') console.log(`āœ… Added frontmatter properties to: ${relativePath}`) } else { - // For other editor types, write the full result back to the original file fs.writeFileSync(fileToProcess, answer, 'utf8') console.log(`āœ… Updated: ${relativePath}`) } } else { - // Just output to console (current behavior) if (filesToProcess.length > 1) { console.log(`\n=== ${relativePath} ===`) } console.log(answer) } - // Always restore Liquid references after processing (even in non-write mode) + // Restore Liquid references even when not writing. if (options.verbose) { console.log(`Restoring Liquid references in: ${relativePath}`) } @@ -271,7 +260,6 @@ program try { runLiquidTagsScript('restore', [fileToProcess], false) } catch (restoreError) { - // Log restore failures in verbose mode for debugging if (options.verbose) { console.error(`Warning: Failed to restore Liquid references: ${restoreError}`) } @@ -291,7 +279,6 @@ program spinner.stop() - // Exit with appropriate code based on whether any errors occurred if (process.exitCode) { process.exit(process.exitCode) } @@ -300,9 +287,6 @@ program program.parse(process.argv) -/** - * Run liquid-tags command on specified file paths - */ function runLiquidTagsScript( command: 'expand' | 'restore', filePaths: string[], @@ -314,7 +298,6 @@ function runLiquidTagsScript( } try { - // Run liquid-tags script via tsx const liquidTagsScriptPath = path.join( process.cwd(), 'src/content-render/scripts/liquid-tags.ts', @@ -330,7 +313,6 @@ function runLiquidTagsScript( } } -// Handle graceful shutdown process.on('SIGINT', () => { console.log('\n\nšŸ›‘ Process interrupted by user') process.exit(0) diff --git a/src/automated-pipelines/lib/update-markdown.ts b/src/automated-pipelines/lib/update-markdown.ts index 49addfc6ee14..964e0c78bce2 100644 --- a/src/automated-pipelines/lib/update-markdown.ts +++ b/src/automated-pipelines/lib/update-markdown.ts @@ -14,7 +14,6 @@ import type { MarkdownFrontmatter } from '@/types' const logger = createLogger(import.meta.url) -// Type definitions - extending existing type to add missing fields and make most fields optional type FrontmatterData = Partial & { autogenerated?: string [key: string]: unknown @@ -67,8 +66,9 @@ type ChildrenComparison = { const ROOT_INDEX_FILE = 'content/index.md' export const MARKDOWN_COMMENT = '\n\n' -// Main entrypoint into this module. This function adds, removes, and updates -// versions frontmatter in all directories under the targetDirectory. +// Main entrypoint into this module. +// Walks every directory under targetDirectory, adding and removing Markdown +// files and keeping the index.md children and versions frontmatter in sync. export async function updateContentDirectory({ targetDirectory, sourceContent, @@ -87,7 +87,6 @@ async function removeMarkdownFiles( sourceFiles: string[], autogeneratedType: string | undefined, ): Promise { - // Copy the autogenerated Markdown files to the target directory const autogeneratedFiles = await getAutogeneratedFiles(targetDirectory, autogeneratedType) // If the first array contains items that the second array does not, // it means that a Markdown page was deleted from the OpenAPI schema @@ -98,7 +97,6 @@ async function removeMarkdownFiles( count: filesToRemove.length, }) } - // Markdown files that need to be deleted for (const file of filesToRemove) { unlinkSync(file) } @@ -140,19 +138,15 @@ async function updateMarkdownFiles( for (const [file, newContent] of Object.entries(sourceContent)) { await updateMarkdownFile(file, newContent.data, newContent.content) } - // This function recursively updates the index.md files in each - // of the directories under targetDirectory await updateDirectory(targetDirectory, frontmatter, { indexOrder }) - // We don't want to update directories that the pipelines don't affect - // so we make one call to update only the root index.md file - // in targetDirectory to prevent any unintended changes + // The pipelines should not touch directories they do not own, so this + // call updates only the index.md file in the parent directory. await updateDirectory(path.dirname(targetDirectory), frontmatter, { rootDirectoryOnly: true }) } -// If the Markdown file already exists on disk, we only update the -// content and version frontmatter to allow writers to manually -// edit the modifiable content of the file. If the Markdown file doesn't -// exists, we create a new Markdown file. +// If the Markdown file already exists on disk, we update only the content +// and the versions frontmatter, so writers can hand-edit the other fields. +// If it does not exist, we create it. async function updateMarkdownFile( file: string, sourceData: FrontmatterData, @@ -160,8 +154,6 @@ async function updateMarkdownFile( commentDelimiter: string = MARKDOWN_COMMENT, ): Promise { if (existsSync(file)) { - // update only the versions property of the file, assuming - // the other properties have already been added and edited const { data, content } = matter(await readFile(file, 'utf-8')) // Double check that the comment delimiter is only used once @@ -179,7 +171,6 @@ async function updateMarkdownFile( const isDelimiterMissing = !matches const isContentSame = automatedContent === sourceContent const isVersionsSame = isEqual(sourceData.versions, data.versions) - // Only proceed if the content or versions have changed if (isContentSame && isVersionsSame && !isDelimiterMissing) { logger.debug('Markdown file unchanged, skipping', { file }) return @@ -219,14 +210,12 @@ async function updateDirectory( { rootDirectoryOnly = false, shortTitle = false, indexOrder = {} }: UpdateDirectoryOptions = {}, ): Promise { const initialDirectoryListing = await getDirectoryInfo(directory) - // If there are no children on disk, remove the directory if (initialDirectoryListing.directoryContents.length === 0 && !rootDirectoryOnly) { logger.info('Removing empty directory', { directory }) await rimraf(directory) return } - // Recursively update child directories if (!rootDirectoryOnly) { await Promise.all( initialDirectoryListing.childDirectories.map(async (subDirectory) => { @@ -256,7 +245,6 @@ async function updateDirectory( } if (!rootDirectoryOnly) { - // Update the versions in the index.md file to match the directory contents directoryFiles.push(...childDirectories.map((dir) => path.join(dir, 'index.md'))) const newVersions = await getIndexFileVersions(directory, directoryFiles) const isVersionEqual = isEqual(newVersions, data.versions) @@ -265,7 +253,6 @@ async function updateDirectory( } } - // Update the index.md file contents and write the file to disk const dataUpdatedChildren = updateIndexChildren( data, { itemsToAdd, itemsToRemove }, @@ -295,7 +282,6 @@ function getChildrenToCompare( const isEarlyAccess = (item: string) => isRootIndexFile(indexFile) && item === 'early-access' - // Get the list of children from the directory contents const childrenOnDisk = directoryContents .map((file) => `${path.basename(file, '.md')}`) .filter((item) => !isEarlyAccess(item)) @@ -317,7 +303,7 @@ function getChildrenToCompare( // the order of the first items in the index files children // property. All other items are sorted and appended to list. // -// 2. If not config is defined and the index file is an +// 2. If no config is defined and the index file is an // autogenerated file, we sort all the children alphabetically. // // 3. If the index file is not autogenerated, we leave the ordering @@ -332,7 +318,6 @@ function updateIndexChildren( const { itemsToAdd, itemsToRemove } = childUpdates const childPrefix = rootIndex ? '' : '/' - // Get a new list of children with added and removed items const children = [...(data.children || [])] // remove the '/' prefix used in index.md children .map((item) => item.replace(childPrefix, '')) @@ -373,8 +358,8 @@ function updateIndexChildren( return updatedData } -// Gets the contents of the index.md file from disk if it exits or -// creates a new index.md file with the default frontmatter. +// Gets the contents of the index.md file from disk if it exists, +// or returns default frontmatter for a new one. async function getIndexFileContents( indexFile: string, frontmatter: FrontmatterData, @@ -413,7 +398,6 @@ async function getIndexFileVersions( `File ${filepath} does not exist while assembling directory index.md files to create parent version.`, ) } - // If not a markdown(x) file, skip it if (!file.endsWith('.md') && !file.endsWith('.mdx')) { return } @@ -536,12 +520,10 @@ function checkVersionContinuity(versions: (string | undefined)[]): boolean { return availableVersions.every(Boolean) } -// Returns true if the indexFile is the root index.md file function isRootIndexFile(indexFile: string): boolean { return indexFile === ROOT_INDEX_FILE } -// Creates a new directory if it doesn't exist async function createDirectory(targetDirectory: string): Promise { if (!existsSync(targetDirectory)) { await mkdirp(targetDirectory) diff --git a/src/automated-pipelines/tests/rendering.ts b/src/automated-pipelines/tests/rendering.ts index e4a1d2cb6e10..f8dfc17ca55f 100644 --- a/src/automated-pipelines/tests/rendering.ts +++ b/src/automated-pipelines/tests/rendering.ts @@ -6,7 +6,6 @@ import { describe, expect, test, vi } from 'vitest' import { loadPages } from '@/frame/lib/page-data' import { get } from '@/tests/helpers/e2etest' -// Type definitions for page objects type Page = { autogenerated?: string fullPath: string @@ -17,7 +16,6 @@ type Page = { } } -// Get a list of the autogenerated pages const pageList: Page[] = await loadPages(undefined, ['en']) describe('autogenerated docs render', () => { diff --git a/src/automated-pipelines/tests/update-markdown.ts b/src/automated-pipelines/tests/update-markdown.ts index 210ffb9dc447..3b2290b86864 100644 --- a/src/automated-pipelines/tests/update-markdown.ts +++ b/src/automated-pipelines/tests/update-markdown.ts @@ -10,7 +10,6 @@ import type { FrontmatterVersions } from '@/types' import { updateContentDirectory } from '../lib/update-markdown' -// Type definitions type ContentItem = { data: { title: string @@ -92,8 +91,6 @@ describe('automated content directory updates', () => { contentDataFullPath[path.join(targetDirectory, key)] = newContentData[key] } - // Rewrites the content directory in the operating system's - // temp directory. await updateContentDirectory({ targetDirectory, sourceContent: contentDataFullPath, diff --git a/src/shielding/middleware/handle-invalid-paths.ts b/src/shielding/middleware/handle-invalid-paths.ts index e8b8a9adc412..7b259d55c941 100644 --- a/src/shielding/middleware/handle-invalid-paths.ts +++ b/src/shielding/middleware/handle-invalid-paths.ts @@ -76,8 +76,8 @@ export default function handleInvalidPaths( next: NextFunction, ) { if (isJunkPath(req.path)) { - // We can all the CDN to cache these responses because they're - // they're not going to suddenly work in the next deployment. + // We can let the CDN cache these responses because they are not going + // to suddenly work in the next deployment. defaultCacheControl(res) res.status(404).type('text').send('Not found') return diff --git a/src/shielding/middleware/handle-invalid-query-strings.ts b/src/shielding/middleware/handle-invalid-query-strings.ts index 4020ff794953..df7c81577c12 100644 --- a/src/shielding/middleware/handle-invalid-query-strings.ts +++ b/src/shielding/middleware/handle-invalid-query-strings.ts @@ -65,9 +65,7 @@ export default function handleInvalidQuerystrings( if (method === 'GET' || method === 'HEAD') { const originalKeys = Object.keys(query) - // Check for invalid query string patterns (square brackets, etc.) const invalidKeys = originalKeys.filter((key) => { - // Check for square brackets which are invalid return key.includes('[') || key.includes(']') }) diff --git a/src/shielding/middleware/handle-malformed-urls.ts b/src/shielding/middleware/handle-malformed-urls.ts index f4f13e75c4b3..a5e364b9543b 100644 --- a/src/shielding/middleware/handle-malformed-urls.ts +++ b/src/shielding/middleware/handle-malformed-urls.ts @@ -4,23 +4,19 @@ import { defaultCacheControl } from '@/frame/middleware/cache-control' import { ExtendedRequest } from '@/types' /** - * Middleware to handle malformed UTF-8 sequences in URLs that cause - * decodeURIComponent to fail. This prevents crashes from malicious - * requests containing invalid URL-encoded sequences like %FF. + * Malformed UTF-8 in a URL, like `%FF`, makes decodeURIComponent throw. + * Express does not catch that while parsing, so without this the crash + * happens later at the router level. */ export default function handleMalformedUrls( req: ExtendedRequest, res: Response, next: NextFunction, ) { - // Check URL for malformed UTF-8 sequences - // Express/router doesn't catch these during initial parsing - they cause - // crashes later when decodeURIComponent is called at the router level const url = req.originalUrl || req.url try { decodeURIComponent(url) } catch { - // If any decoding fails, this is a malformed URL defaultCacheControl(res) res.status(400).type('text').send('Bad Request: Malformed URL') return diff --git a/src/shielding/middleware/handle-old-next-data-paths.ts b/src/shielding/middleware/handle-old-next-data-paths.ts index 33417340f742..ac47f1883cbe 100644 --- a/src/shielding/middleware/handle-old-next-data-paths.ts +++ b/src/shielding/middleware/handle-old-next-data-paths.ts @@ -45,7 +45,6 @@ export default function handleOldNextDataPaths( let _buildId: string function getCurrentBuildID() { - // Simple memoization if (!_buildId) { _buildId = fs.readFileSync('.next/BUILD_ID', 'utf-8').trim() } diff --git a/src/shielding/tests/invalid-querystrings.ts b/src/shielding/tests/invalid-querystrings.ts index 1619ae7f25a1..23871850c62e 100644 --- a/src/shielding/tests/invalid-querystrings.ts +++ b/src/shielding/tests/invalid-querystrings.ts @@ -28,7 +28,7 @@ describe('invalid query strings', () => { test('302 redirect for many unrecognized query strings', async () => { // This test depends on knowing exactly the number - // of unrecognized query strings that will trigger a 400. + // of unrecognized query strings that will trigger a redirect. const sp = new URLSearchParams() for (const letter of alphabet.slice(0, MAX_UNFAMILIAR_KEYS_REDIRECT)) { sp.set(letter, '1') diff --git a/src/shielding/tests/malformed-urls.ts b/src/shielding/tests/malformed-urls.ts index 9262e707f084..2777e90d6c9f 100644 --- a/src/shielding/tests/malformed-urls.ts +++ b/src/shielding/tests/malformed-urls.ts @@ -32,21 +32,19 @@ describe('malformed URLs', () => { }) test('allows URLs with control characters (valid UTF-8)', async () => { + // %01 decodes fine, so the middleware lets it through. const res = await get('/en/test-%01-page') - expect(res.statusCode).toBe(404) // Should be 404 since page doesn't exist, not 400 - // Control characters like %01 are valid UTF-8 and don't cause decoding errors + expect(res.statusCode).toBe(404) // 404 because the page does not exist, not 400 }) test('allows valid URLs with proper encoding', async () => { const res = await get('/en/get-started') expect(res.statusCode).not.toBe(400) - // Should not be blocked by malformed URL middleware }) test('allows valid URLs with proper percent encoding', async () => { const res = await get('/en/search?q=test%20query') expect(res.statusCode).not.toBe(400) - // Should not be blocked by malformed URL middleware }) test('blocks malformed query parameters', async () => { From 54ba6bde5ebcccf9a768303a83eb6270d2cba020 Mon Sep 17 00:00:00 2001 From: Kevin Heis Date: Tue, 15 Sep 2026 20:25:13 +0000 Subject: [PATCH 12/16] Clean up comments in src/ghes-releases (#63274) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> Copilot-Session: 17588eeb-788a-4f82-9d36-b5079fb36521 --- src/ghes-releases/lib/parse-release-notes.ts | 40 +++--------- src/ghes-releases/lib/release-issues.ts | 3 - .../scripts/create-enterprise-issue.ts | 2 - .../scripts/deprecate/rewrite-asset-paths.ts | 4 -- .../deprecate/update-automated-pipelines.ts | 4 +- .../scripts/deprecate/update-data.ts | 8 +-- .../scripts/generate-release-notes.ts | 65 +++++-------------- .../scripts/notify-release-pms.ts | 30 ++------- src/ghes-releases/scripts/release-banner.ts | 4 -- .../scripts/update-enterprise-dates.ts | 1 - .../tests/generate-release-notes.ts | 10 --- src/ghes-releases/tests/notify-release-pms.ts | 8 --- 12 files changed, 37 insertions(+), 142 deletions(-) diff --git a/src/ghes-releases/lib/parse-release-notes.ts b/src/ghes-releases/lib/parse-release-notes.ts index 7d47ecf37891..e235430cd314 100644 --- a/src/ghes-releases/lib/parse-release-notes.ts +++ b/src/ghes-releases/lib/parse-release-notes.ts @@ -5,22 +5,16 @@ import fs from 'fs' import { load } from 'js-yaml' -// ─── Types ─────────────────────────────────────────────────────────────────── - export interface NoteEntry { heading: string notes: string[] sourceUrl: string } -// ─── extractYaml ───────────────────────────────────────────────────────────── - /** - * Extract YAML content from agent output. * Looks for ```yaml ... ``` blocks, or falls back to lines starting with "- heading:" */ export function extractYaml(agentOutput: string): string | null { - // Try to extract from fenced YAML code block const fenced = agentOutput.match(/```ya?ml\s*\n([\s\S]*?)```/) if (fenced) return fenced[1].trim() @@ -43,21 +37,11 @@ export function extractYaml(agentOutput: string): string | null { return yamlLines.length > 0 ? yamlLines.join('\n').trim() : null } -// ─── extractSkipReason ─────────────────────────────────────────────────────── - -/** - * Extract a skip reason from YAML that contains `# SKIP: ` followed by `[]`. - */ export function extractSkipReason(yamlStr: string): string | null { const match = yamlStr.match(/^#\s*SKIP:\s*(.+)/m) return match ? match[1].trim() : null } -// ─── parseNoteEntries ──────────────────────────────────────────────────────── - -/** - * Parse extracted YAML into structured note entries - */ export function parseNoteEntries(yamlStr: string, sourceUrl: string): NoteEntry[] { const entries: NoteEntry[] = [] @@ -82,14 +66,12 @@ export function parseNoteEntries(yamlStr: string, sourceUrl: string): NoteEntry[ } } } catch { - // YAML parse failed + // Malformed YAML returns no entries rather than throwing. } return entries } -// ─── loadExistingEntries ───────────────────────────────────────────────────── - /** * Parse an existing release notes YAML file and extract NoteEntry[] from it, * along with a set of source issue URLs already covered. @@ -107,7 +89,7 @@ export function loadExistingEntries(yamlPath: string): { /** * Parse release notes YAML content (as a string) and extract NoteEntry[] from it. - * This is the testable core — no file I/O. + * This is the testable core, with no file I/O. * * Note: This uses manual line-by-line parsing instead of js-yaml because we need * to preserve the `# https://github.com/.../issues/NNN` source URL comments that @@ -168,7 +150,7 @@ export function loadExistingEntriesFromString(content: string): { if (currentSection === 'changes') currentHeading = 'Changes' else if (currentSection === 'closing_down') currentHeading = 'Closing down' else if (currentSection === 'retired') currentHeading = 'Retired' - else currentHeading = null // known_issues — skip + else currentHeading = null // known_issues: skip continue } @@ -231,8 +213,6 @@ export function loadExistingEntriesFromString(content: string): { return { entries, coveredUrls } } -// ─── buildReleaseNotesYaml ─────────────────────────────────────────────────── - /** * Append YAML lines for a list of note entries at a given indentation level. * Handles the `# sourceUrl`, `- |`, and multi-line note content pattern. @@ -249,9 +229,6 @@ export function appendNoteLines(lines: string[], noteEntries: NoteEntry[], inden } } -/** - * Build the final release notes YAML string from note entries. - */ export function buildReleaseNotesYaml( noteEntries: NoteEntry[], releaseCandidate: boolean, @@ -286,7 +263,7 @@ export function buildReleaseNotesYaml( lines.push('sections:') - // ── Features (grouped by heading) ── + // Features (grouped by heading). const featureEntries = noteEntries.filter((e) => featureHeadings.includes(e.heading)) const otherEntries = noteEntries.filter((e) => !featureHeadings.includes(e.heading)) @@ -294,7 +271,6 @@ export function buildReleaseNotesYaml( lines.push(' features:') if (featureEntries.length > 0) { - // Group by heading, preserving template order const byHeading = new Map() for (const entry of featureEntries) { const existing = byHeading.get(entry.heading) || [] @@ -315,7 +291,7 @@ export function buildReleaseNotesYaml( lines.push(' # TODO: Add feature notes') } - // ── Changes ── + // Changes. const changeEntries = otherEntries.filter((e) => !['Closing down', 'Retired'].includes(e.heading)) if (changeEntries.length > 0) { lines.push('') @@ -323,14 +299,14 @@ export function buildReleaseNotesYaml( appendNoteLines(lines, changeEntries, ' ') } - // ── Known issues ── + // Known issues. lines.push('') lines.push(' known_issues:') lines.push(' # TODO: Add known issues from "GHES Release Note Tracking" project') lines.push(' - |') lines.push(' ...') - // ── Closing down ── + // Closing down. const closingEntries = otherEntries.filter((e) => e.heading === 'Closing down') if (closingEntries.length > 0) { lines.push('') @@ -338,7 +314,7 @@ export function buildReleaseNotesYaml( appendNoteLines(lines, closingEntries, ' ') } - // ── Retired ── + // Retired. const retiredEntries = otherEntries.filter((e) => e.heading === 'Retired') if (retiredEntries.length > 0) { lines.push('') diff --git a/src/ghes-releases/lib/release-issues.ts b/src/ghes-releases/lib/release-issues.ts index dfcb373786b6..91e0d89cb3bd 100644 --- a/src/ghes-releases/lib/release-issues.ts +++ b/src/ghes-releases/lib/release-issues.ts @@ -23,9 +23,6 @@ export function parseIssueState(value?: string): IssueState { ) } -/** - * Build gh CLI args for listing release issues. - */ export function buildReleaseIssueListArgs(version: string, issueState: IssueState): string[] { const label = `GHES ${version}` return [ diff --git a/src/ghes-releases/scripts/create-enterprise-issue.ts b/src/ghes-releases/scripts/create-enterprise-issue.ts index 99a78b00d1e4..b19db2e4366c 100644 --- a/src/ghes-releases/scripts/create-enterprise-issue.ts +++ b/src/ghes-releases/scripts/create-enterprise-issue.ts @@ -99,7 +99,6 @@ async function createDeprecationIssue() { return } - // Create the deprecation issue const issueTemplate = readFileSync('src/ghes-releases/lib/deprecation-steps.md', 'utf8') const { data, content } = matter(issueTemplate) const { title, labels } = data @@ -348,7 +347,6 @@ function getNumberDaysUntilMilestone(milestoneDate: string): number { const nextMilestoneDateTime = new Date(milestoneDate).getTime() const todayTime = new Date(today).getTime() const differenceInMilliseconds = nextMilestoneDateTime - todayTime - // Return the difference in days return Math.floor(differenceInMilliseconds / (1000 * 60 * 60 * 24)) } diff --git a/src/ghes-releases/scripts/deprecate/rewrite-asset-paths.ts b/src/ghes-releases/scripts/deprecate/rewrite-asset-paths.ts index a86ff9231f81..0ae4cc3cd8ad 100644 --- a/src/ghes-releases/scripts/deprecate/rewrite-asset-paths.ts +++ b/src/ghes-releases/scripts/deprecate/rewrite-asset-paths.ts @@ -28,13 +28,10 @@ export class RewriteAssetPathsPlugin { registerAction: (event: string, callback: (args: ResourceSavedArgs) => Promise) => void, ) { registerAction('onResourceSaved', async ({ resource }: ResourceSavedArgs) => { - // Show some activity process.stdout.write('.') - // Only operate on HTML files if (!resource.isHtml() && !resource.isCss()) return - // Get the text contents of the resource const text = resource.getText() let newBody = text @@ -51,7 +48,6 @@ export class RewriteAssetPathsPlugin { newBody = newBody.replace(//g, '') if (!this.localDev) { - // Rewrite asset paths newBody = newBody.replace( /(?src|href)="(?:\.\.\/|\/)*(?_next\/static|javascripts|stylesheets|assets\/fonts|assets\/cb-\d+\/images|node_modules)/g, (match: string, attribute: string, basepath: string) => { diff --git a/src/ghes-releases/scripts/deprecate/update-automated-pipelines.ts b/src/ghes-releases/scripts/deprecate/update-automated-pipelines.ts index ca294175a8a9..d1240f2f5d2e 100755 --- a/src/ghes-releases/scripts/deprecate/update-automated-pipelines.ts +++ b/src/ghes-releases/scripts/deprecate/update-automated-pipelines.ts @@ -145,8 +145,8 @@ export async function updateAutomatedPipelines() { // Derive the previous release's corresponding directory by replacing // the current release number with the previous one. This correctly // maps each calendar-date variant to its predecessor, e.g.: - // ghes-3.20-2022-11-28 → ghes-3.19-2022-11-28 - // ghes-3.20-2026-03-10 → ghes-3.19-2026-03-10 + // ghes-3.20-2022-11-28 -> ghes-3.19-2022-11-28 + // ghes-3.20-2026-03-10 -> ghes-3.19-2026-03-10 const previousDirName = dirToAdd.replace(currentReleaseNumber, previousReleaseNumber) if (!existingDataDir.includes(previousDirName)) { throw new Error( diff --git a/src/ghes-releases/scripts/deprecate/update-data.ts b/src/ghes-releases/scripts/deprecate/update-data.ts index 9c214ec033ce..08e91ce9e9b1 100644 --- a/src/ghes-releases/scripts/deprecate/update-data.ts +++ b/src/ghes-releases/scripts/deprecate/update-data.ts @@ -36,7 +36,6 @@ export function updateDataFiles() { function updateReusableData() { const deletedDataFiles = [] - // Remove empty reusables for (const file of dataReusables) { const oldContents = fs.readFileSync(file, 'utf8').trim() if (oldContents === '') { @@ -71,10 +70,9 @@ function updateReusableData() { } } -// Removes deprecated data/feature files and outputs a list -// of data/features available in all versions - this list -// is currently only used used for reviewing purposes when -// deprecating a GHES release +// Removes deprecated data/feature files and outputs a list of data/features +// available in all versions. That list is only used for review during a GHES +// deprecation. function updateFeatureData() { const allFeatureFiles = new Set() diff --git a/src/ghes-releases/scripts/generate-release-notes.ts b/src/ghes-releases/scripts/generate-release-notes.ts index d5b30a3c35be..1ecdd3ab3b21 100644 --- a/src/ghes-releases/scripts/generate-release-notes.ts +++ b/src/ghes-releases/scripts/generate-release-notes.ts @@ -32,7 +32,6 @@ import { parseIssueState, } from '@/ghes-releases/lib/release-issues' -// ─── Ctrl+C handling ───────────────────────────────────────────────────────── // Copilot CLI puts the terminal in raw mode, so we catch Ctrl+C (0x03) manually. let activeChild: ChildProcess | null = null @@ -56,8 +55,6 @@ if (process.stdin.isTTY) { }) } -// ─── Types ─────────────────────────────────────────────────────────────────── - interface ReleaseIssue { number: number title: string @@ -83,8 +80,6 @@ function loadFeatureHeadings(): string[] { return _featureHeadingsCache } -// ─── Helpers ───────────────────────────────────────────────────────────────── - /** * Run `gh` CLI commands with native auth (no GITHUB_TOKEN interference) */ @@ -99,9 +94,6 @@ function gh(args: string[]): string { }) } -/** - * Fetch release issues labeled "GHES " using the selected issue state. - */ function fetchReleaseIssues(version: string, issueState: IssueState): ReleaseIssue[] { const output = gh(buildReleaseIssueListArgs(version, issueState)) const issues = JSON.parse(output) as ReleaseIssue[] @@ -123,9 +115,6 @@ function extractChangelogPrUrl(issueBody: string): string | null { return match ? match[0] : null } -/** - * Fetch the body of a changelog PR by URL. - */ function fetchChangelogPrBody(prUrl: string): string | null { try { const output = gh(['pr', 'view', prUrl, '--json', 'body']) @@ -176,7 +165,7 @@ function searchChangelogPr(issueNumber: number): ChangelogInfo | null { } } } catch { - // Search failed — fall back to no changelog + // Search failed, so fall back to no changelog. } return null } @@ -195,23 +184,20 @@ function findChangelogPr(issue: ReleaseIssue): ChangelogInfo | null { } /** - * Resolve the Copilot CLI path. Checks common locations. + * Resolve the Copilot CLI path. * Result is cached after first call. */ let _copilotCliPath: string | null = null function findCopilotCli(): string { if (_copilotCliPath) return _copilotCliPath - // Check if `copilot` is on PATH try { const result = execFileSync('which', ['copilot'], { encoding: 'utf8' }).trim() if (result) { _copilotCliPath = result return result } - } catch { - // not on PATH - } + } catch {} // Fallback: check VS Code extension storage locations. // These paths are macOS-only. On Linux/Windows the `which` check above should @@ -227,7 +213,6 @@ function findCopilotCli(): string { return vsCodePath } - // Check VS Code stable location (macOS) const vsCodeStablePath = path.join( homeDir, 'Library/Application Support/Code/User/globalStorage/github.copilot-chat/copilotCli/copilot', @@ -382,9 +367,6 @@ function runAgent(ctx: AgentContext): Promise { }) } -/** - * Sleep for the given number of milliseconds (non-blocking). - */ function sleep(ms: number): Promise { return new Promise((resolve) => setTimeout(resolve, ms)) } @@ -401,7 +383,7 @@ interface AgentResult { * Run the agent with retry logic. Retries up to `maxRetries` times on failure. * Validates that extracted YAML parses into non-empty entries before accepting. * If the agent tries to skip (returns `# SKIP: reason` + `[]`), treats it as a - * failed attempt and retries — issues that matched the GHES label filter should + * failed attempt and retries, because issues that matched the GHES label filter should * always get a release note. If all attempts result in skips, the last skip * reason is attached as a warning. */ @@ -425,9 +407,7 @@ async function runAgentWithRetry(ctx: AgentContext, maxRetries = 2): Promise 0) { return { @@ -438,7 +418,7 @@ async function runAgentWithRetry(ctx: AgentContext, maxRetries = 2): Promise { const title = issue.title.trim() - // Strip all trailing label tags like [GA], [Public Preview], etc. const stripped = title.replace(/\s*\[[^\]]*\]/g, '').trim() // Skip issues whose title is just "GHES X.Y GA" or "GHES X.Y release" if (/^ghes\s+\d+\.\d+\s+ga$/i.test(stripped)) return false @@ -611,7 +585,7 @@ program console.log(` Filtered out ${filteredCount} meta-issue(s) (GA/release tracking)`) } - // Filter out [Private Preview] issues — these never get release notes + // [Private Preview] issues never get release notes. const beforePrivateFilter = issues.length issues = issues.filter((issue) => { return !/\[Private Preview\]/i.test(issue.title) @@ -623,7 +597,7 @@ program ) } - // Filter out issues labeled "internal release" — these are internal-only and don't get release notes + // "internal release" issues are internal-only and get no release notes. const beforeInternalFilter = issues.length issues = issues.filter((issue) => { return !issue.labels.some((l) => l.name.toLowerCase() === 'internal release') @@ -634,13 +608,12 @@ program } } - // Compute output path upfront so we can check for existing file const dirName = release.replace('.', '-') const fileName = rc ? '0-rc1.yml' : '0.yml' const outputDir = path.join(process.cwd(), 'data/release-notes/enterprise-server', dirName) const outputPath = path.join(outputDir, fileName) - // ── Incremental mode: load existing entries ── + // Incremental mode: load existing entries. const allEntries: NoteEntry[] = [] let existingCoveredUrls = new Set() @@ -679,7 +652,7 @@ program } } - // ── Step 2: Find changelog PRs ── + // Step 2: Find changelog PRs. spinner.start('Finding changelog PRs...') const issueChangelogMap = new Map() let changelogFound = 0 @@ -691,11 +664,11 @@ program } spinner.succeed(`Found changelog PRs for ${changelogFound}/${issues.length} issues`) - // ── Step 3: Run agent on each issue ── + // Step 3: Run agent on each issue. const failures: { issue: ReleaseIssue; error: string }[] = [] const existingEntryCount = allEntries.length - // Load valid headings once (cached, but clearer when hoisted) + // Hoisted for clarity. The underlying load is already cached. const featureHeadings = loadFeatureHeadings() // Helper to write current entries to file (called after each success and on Ctrl+C) @@ -712,7 +685,6 @@ program } } - // Register so Ctrl+C saves progress flushBeforeExit = writeCurrentOutput for (let i = 0; i < issues.length; i++) { @@ -779,7 +751,6 @@ program spinner.suffixText = '' spinner.succeed(`${label}${retryNote}`) - // Write incrementally so progress is saved writeCurrentOutput() } catch (error) { const err = error as Error & { rawOutput?: string } @@ -788,7 +759,7 @@ program failures.push({ issue, error: msg }) spinner.suffixText = '' if (isSkipFailure) { - // Clean output for skip failures — no raw agent dump + // Skip failures get clean output, with no raw agent dump. const skipReason = msg.replace('Agent tried to skip: ', '') spinner.warn(`${label} — skipped by agent`) console.log(` Reason: ${skipReason}`) @@ -808,7 +779,7 @@ program } } - // ── Step 4: Final summary ── + // Step 4: Final summary. flushBeforeExit = null const newCount = allEntries.length - existingEntryCount if (existingEntryCount > 0) { @@ -827,7 +798,7 @@ program if (stdout) { if (failures.length > 0 && allEntries.length === 0) { - // All issues failed — don't print empty template + // All issues failed, so don't print an empty template. process.exit(1) } if (singleIssue) { diff --git a/src/ghes-releases/scripts/notify-release-pms.ts b/src/ghes-releases/scripts/notify-release-pms.ts index 446de67a46af..79adb25aad38 100644 --- a/src/ghes-releases/scripts/notify-release-pms.ts +++ b/src/ghes-releases/scripts/notify-release-pms.ts @@ -22,15 +22,11 @@ import fs from 'fs' import path from 'path' import ora from 'ora' -// ─── Types ─────────────────────────────────────────────────────────────────── - export interface SourceNote { issueUrl: string issueNumber: number } -// ─── Helpers ───────────────────────────────────────────────────────────────── - /** * Run read-only `gh` CLI commands. * Uses DOCS_BOT_PAT_BASE when available (CI), otherwise falls back to @@ -90,8 +86,8 @@ export function parseSourceNotes(content: string): SourceNote[] { const match = lines[i].match(/^\s*#\s*(https:\/\/github\.com\/github\/releases\/issues\/(\d+))/) if (match) { const issueNumber = parseInt(match[2], 10) - // Deduplicate — some issues appear multiple times (e.g., in features + changes) - // We use the first occurrence so the link points to the primary note + // Some issues appear multiple times (e.g. in features and changes). Keep the + // first occurrence so the link points to the primary note. if (!seen.has(issueNumber)) { seen.add(issueNumber) notes.push({ @@ -105,17 +101,11 @@ export function parseSourceNotes(content: string): SourceNote[] { return notes } -/** - * Read a release notes YAML file and extract source issue URLs. - */ function extractSourceNotes(yamlPath: string): SourceNote[] { const content = fs.readFileSync(yamlPath, 'utf8') return parseSourceNotes(content) } -/** - * Build the comment body for a release issue notification. - */ export function buildCommentBody( version: string, rc: boolean, @@ -142,15 +132,10 @@ You're welcome to edit it in the PR. If you do nothing, the note will be publish Any questions, ask in [#docs-ghes-releases](https://github-grid.enterprise.slack.com/archives/C0AQ37XBK7D).` } -/** - * Build the marker string used to identify notification comments. - */ export function buildMarker(version: string, releaseType: 'rc' | 'ga'): string { return `` } -// ─── CLI ───────────────────────────────────────────────────────────────────── - const program = new Command() program @@ -186,7 +171,6 @@ program const { release, pr: prNumber, dryRun, reviewDate } = options const spinner = ora() - // Validate --review-date format if provided if (reviewDate && !/^\d{4}-\d{2}-\d{2}$/.test(reviewDate)) { console.error( `Error: Invalid date format "${reviewDate}". Expected: YYYY-MM-DD (e.g., 2026-04-20)`, @@ -194,7 +178,6 @@ program process.exit(1) } - // Validate release version format if (!/^\d+\.\d+$/.test(release)) { console.error( `Error: Invalid release version format "${release}". Expected: X.Y (e.g., 3.20)`, @@ -202,7 +185,6 @@ program process.exit(1) } - // Determine RC vs GA const dirName = release.replace('.', '-') const rcPath = path.join( process.cwd(), @@ -257,7 +239,7 @@ program const relativeFilePath = path.relative(process.cwd(), yamlPath) - // ── Step 1: Extract source issue URLs ── + // Step 1: Extract source issue URLs. spinner.start('Parsing release notes file...') const sourceNotes = extractSourceNotes(yamlPath) spinner.succeed(`Found ${sourceNotes.length} unique release issue(s) in ${relativeFilePath}`) @@ -267,7 +249,7 @@ program process.exit(0) } - // ── Step 2: Check for existing comments (avoid duplicates) ── + // Step 2: Check for existing comments (avoid duplicates). const releaseType = rc ? 'rc' : 'ga' const marker = buildMarker(release, releaseType) const alreadyCommented = new Set() @@ -300,7 +282,7 @@ program spinner.succeed('No existing notifications found') } - // ── Step 3: Post comments ── + // Step 3: Post comments. const toNotify = sourceNotes.filter((n) => !alreadyCommented.has(n.issueNumber)) if (toNotify.length === 0) { @@ -360,7 +342,7 @@ program } } - // ── Summary ── + // Summary. console.log(`\n${'─'.repeat(40)}`) console.log(`${dryRun ? 'šŸ” Dry run' : 'āœ… Done'}`) console.log( diff --git a/src/ghes-releases/scripts/release-banner.ts b/src/ghes-releases/scripts/release-banner.ts index abbd36fd15a0..7c2136cb64b4 100644 --- a/src/ghes-releases/scripts/release-banner.ts +++ b/src/ghes-releases/scripts/release-banner.ts @@ -41,7 +41,6 @@ if (!Object.keys(allVersions).includes(options.version)) { process.exit(1) } -// Load the release candidate variable async function main(): Promise { let jsCode = await fs.readFile(releaseCandidateJSFile, 'utf8') const lineRegex = /export const releaseCandidate = .*/ @@ -51,7 +50,6 @@ async function main(): Promise { ) } - // Create or remove the variable if (options.action === 'create') { jsCode = jsCode.replace( lineRegex, @@ -61,10 +59,8 @@ async function main(): Promise { jsCode = jsCode.replace(lineRegex, `export const releaseCandidate = null`) } - // Update the file await fs.writeFile(releaseCandidateJSFile, jsCode) - // Display next steps console.log(`\nDone! Commit the update to ${releaseCandidateJSFile}. This ${options.action}s the banner for ${options.version}. - To change the banner text, you can edit header.notices.release_candidate in data/ui.yml. diff --git a/src/ghes-releases/scripts/update-enterprise-dates.ts b/src/ghes-releases/scripts/update-enterprise-dates.ts index 8387b60cad30..2eff3bc7c721 100644 --- a/src/ghes-releases/scripts/update-enterprise-dates.ts +++ b/src/ghes-releases/scripts/update-enterprise-dates.ts @@ -44,7 +44,6 @@ if (!process.env.GITHUB_TOKEN) { main() async function main(): Promise { - // send owner, repo, ref, path let rawDates: RawReleaseData = {} try { rawDates = JSON.parse( diff --git a/src/ghes-releases/tests/generate-release-notes.ts b/src/ghes-releases/tests/generate-release-notes.ts index 4082ee2a77ca..a656f6d0125d 100644 --- a/src/ghes-releases/tests/generate-release-notes.ts +++ b/src/ghes-releases/tests/generate-release-notes.ts @@ -8,8 +8,6 @@ import { buildReleaseNotesYaml, } from '@/ghes-releases/lib/parse-release-notes' -// ─── extractYaml ───────────────────────────────────────────────────────────── - describe('extractYaml', () => { test('extracts YAML from a fenced code block', () => { const input = `Here is the release note: @@ -46,8 +44,6 @@ Some trailing text that is not YAML` }) }) -// ─── extractSkipReason ─────────────────────────────────────────────────────── - describe('extractSkipReason', () => { test('extracts reason from "# SKIP: "', () => { expect(extractSkipReason('# SKIP: Not applicable to GHES')).toBe('Not applicable to GHES') @@ -58,8 +54,6 @@ describe('extractSkipReason', () => { }) }) -// ─── parseNoteEntries ──────────────────────────────────────────────────────── - describe('parseNoteEntries', () => { const sourceUrl = 'https://github.com/github/releases/issues/1234' @@ -108,8 +102,6 @@ describe('parseNoteEntries', () => { }) }) -// ─── loadExistingEntriesFromString ─────────────────────────────────────────── - describe('loadExistingEntriesFromString', () => { test('parses feature entries with source URL comments', () => { const content = `date: '2025-01-15' @@ -264,8 +256,6 @@ sections: }) }) -// ─── buildReleaseNotesYaml ─────────────────────────────────────────────────── - describe('buildReleaseNotesYaml', () => { const featureHeadings = ['GitHub Actions', 'Repositories', 'APIs'] diff --git a/src/ghes-releases/tests/notify-release-pms.ts b/src/ghes-releases/tests/notify-release-pms.ts index affefe38c49d..d9e87a09cd8f 100644 --- a/src/ghes-releases/tests/notify-release-pms.ts +++ b/src/ghes-releases/tests/notify-release-pms.ts @@ -7,8 +7,6 @@ import { } from '@/ghes-releases/scripts/notify-release-pms' import type { SourceNote } from '@/ghes-releases/scripts/notify-release-pms' -// ─── parseSourceNotes ──────────────────────────────────────────────────────── - describe('parseSourceNotes', () => { test('extracts issue URLs from YAML comments', () => { const content = `date: '2026-04-01' @@ -67,8 +65,6 @@ sections: }) }) -// ─── buildMarker ───────────────────────────────────────────────────────────── - describe('buildMarker', () => { test('produces a stable HTML comment marker for RC', () => { expect(buildMarker('3.21', 'rc')).toBe('') @@ -83,8 +79,6 @@ describe('buildMarker', () => { }) }) -// ─── buildCommentBody ──────────────────────────────────────────────────────── - describe('buildCommentBody', () => { test('includes the marker in the comment body', () => { const body = buildCommentBody('3.21', true, 100, ['octocat']) @@ -105,8 +99,6 @@ describe('buildCommentBody', () => { }) }) -// ─── Duplicate-prevention logic ────────────────────────────────────────────── - describe('duplicate-prevention filtering', () => { // This tests the core filtering logic used in the CLI action: // const toNotify = sourceNotes.filter((n) => !alreadyCommented.has(n.issueNumber)) From f65fee93f62467a9621e1a61183b39bb925324e9 Mon Sep 17 00:00:00 2001 From: Kevin Heis Date: Tue, 15 Sep 2026 20:32:54 +0000 Subject: [PATCH 13/16] Clean up code comments in src/observability source files (#63229) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 17588eeb-788a-4f82-9d36-b5079fb36521 Copilot-Session: c7ae285a-5311-4fe6-a3d3-093869aa5d4e --- src/observability/lib/failbot.ts | 20 +++------- .../lib/handle-package-not-found.ts | 2 +- src/observability/lib/runtime-metrics.ts | 14 ++----- src/observability/lib/statsd.ts | 11 ++---- src/observability/lib/to-error.ts | 3 +- src/observability/lib/tracing.browser.ts | 2 +- src/observability/lib/tracing.ts | 23 +++++------ src/observability/logger/index.ts | 38 +++---------------- src/observability/logger/lib/log-levels.ts | 13 +++---- .../logger/lib/logger-context.ts | 11 ++---- src/observability/logger/lib/pod-identity.ts | 4 +- src/observability/logger/lib/to-logfmt.ts | 12 +----- .../get-automatic-request-logger.ts | 23 ++--------- .../middleware/catch-middleware-error.ts | 1 - src/observability/middleware/handle-errors.ts | 34 ++++++----------- 15 files changed, 58 insertions(+), 153 deletions(-) diff --git a/src/observability/lib/failbot.ts b/src/observability/lib/failbot.ts index 226def0ae62d..e2b90878c26e 100644 --- a/src/observability/lib/failbot.ts +++ b/src/observability/lib/failbot.ts @@ -4,13 +4,11 @@ import { getLoggerContext } from '@/observability/logger/lib/logger-context' const HAYSTACK_APP = 'docs' +// Five attempts at a 3000ms timeout, with backoff delays of 1, 2, 4, and 8 seconds, +// bound a failing report at roughly 30 seconds. async function retryingFetch(input: RequestInfo | URL, init?: RequestInit): Promise { const url = typeof input === 'string' ? input : input.toString() - // Use fetchWithRetry with retry configuration matching got's behavior - // With the timeout at 3000 (milliseconds) and the retry.limit - // at 4 (times), the total worst-case is: - // 3000 * 4 + 1000 + 2000 + 3000 + 4000 + 8000 = 30 seconds const response = await fetchWithRetry( url, { @@ -29,7 +27,6 @@ async function retryingFetch(input: RequestInfo | URL, init?: RequestInit): Prom } export function report(error: Error, metadata?: Record) { - // If there's no HAYSTACK_URL set, bail early if (!process.env.HAYSTACK_URL) { return } @@ -45,9 +42,9 @@ export function report(error: Error, metadata?: Record) { backends, }) - // Add the request id from the logger context to the metadata - // Per https://github.com/github/failbotg/blob/main/docs/api.md#additional-data - // Metadata can only be a flat object with string & number values, so only add the requestUuid + // Metadata can only be a flat object with string & number values, + // so only add the requestUuid. + // https://github.com/github/failbotg/blob/main/docs/api.md#additional-data const loggerContext = getLoggerContext() return failbot.report(error, { @@ -56,12 +53,7 @@ export function report(error: Error, metadata?: Record) { }) } -// Kept for legacy so you can continue to do: -// -// import FailBot from './lib/failbot' -// ... -// FailBot.report(myError) -// +// Kept so legacy callers can keep doing `FailBot.report(myError)`. export default { report, } diff --git a/src/observability/lib/handle-package-not-found.ts b/src/observability/lib/handle-package-not-found.ts index 2476b21f2d4c..8bb738532c08 100644 --- a/src/observability/lib/handle-package-not-found.ts +++ b/src/observability/lib/handle-package-not-found.ts @@ -4,7 +4,7 @@ to prompt the contributor to run `npm ci`. This handler must be separate from handle-exceptions.ts in order to function. It's imported in package.json in nodemonConfig, whereas that is imported in start-server.ts. -This file should not import any packages. +This file must not import any packages. We are suggesting `npm ci` to contributors to avoid unexpected changes to the package-lock.json file. All other errors should fall through to the error handler in handle-exceptions.ts. diff --git a/src/observability/lib/runtime-metrics.ts b/src/observability/lib/runtime-metrics.ts index 3ab894cf3e52..42740357f5af 100644 --- a/src/observability/lib/runtime-metrics.ts +++ b/src/observability/lib/runtime-metrics.ts @@ -2,9 +2,9 @@ * Periodically emits Node.js runtime metrics to Datadog via StatsD. * * Covers three categories that are otherwise invisible: - * 1. V8 heap — used vs limit, so we can spot memory pressure before OOMs. - * 2. GC — pause duration, so we can correlate latency spikes with GC. - * 3. Event-loop delay — p50/p99, so we can see when the loop is blocked. + * 1. V8 heap: used vs limit, so we can spot memory pressure before OOMs. + * 2. GC: pause duration, so we can correlate latency spikes with GC. + * 3. Event-loop delay: p50/p99, so we can see when the loop is blocked. * * Only activates when StatsD is sending real metrics (MODA_PROD_SERVICE_ENV). */ @@ -23,7 +23,6 @@ function isMetricsEnabled(): boolean { /** * Call once at server start. Safe to call multiple times (no-op after first). - * Only starts collection when StatsD is sending real metrics. */ export function startRuntimeMetrics(): void { if (started) return @@ -31,19 +30,16 @@ export function startRuntimeMetrics(): void { if (!isMetricsEnabled()) return - // --- V8 heap stats (sampled on an interval) --- setInterval(() => { const heap = v8.getHeapStatistics() statsd.gauge('node.heap.used', heap.used_heap_size) statsd.gauge('node.heap.total', heap.total_heap_size) statsd.gauge('node.heap.limit', heap.heap_size_limit) statsd.gauge('node.heap.external', heap.external_memory) - // Percentage of heap limit currently in use const pct = heap.heap_size_limit > 0 ? (heap.used_heap_size / heap.heap_size_limit) * 100 : 0 statsd.gauge('node.heap.used_pct', pct) }, INTERVAL_MS).unref() - // --- GC pause durations --- const gcObserver = new PerformanceObserver((list) => { for (const entry of list.getEntries()) { const kind = (entry as unknown as { detail?: { kind?: number } }).detail?.kind @@ -55,7 +51,6 @@ export function startRuntimeMetrics(): void { }) gcObserver.observe({ entryTypes: ['gc'] }) - // --- Event-loop delay (histogram sampled every 20 ms) --- const eld = monitorEventLoopDelay({ resolution: 20 }) eld.enable() @@ -68,9 +63,6 @@ export function startRuntimeMetrics(): void { }, INTERVAL_MS).unref() } -/** - * Reset the started flag. Only for use in tests. - */ export function _resetForTesting(): void { started = false } diff --git a/src/observability/lib/statsd.ts b/src/observability/lib/statsd.ts index 117d5cb6269b..ff8254d18ffe 100644 --- a/src/observability/lib/statsd.ts +++ b/src/observability/lib/statsd.ts @@ -18,13 +18,10 @@ const tagCandidates = ['app:docs', modaApp] export const tags: string[] = tagCandidates.filter((tag): tag is string => Boolean(tag)) const statsd = new StatsD({ - // When host and port are not set, hot-shots will default to the - // DD_AGENT_HOST and DD_DOGSTATSD_PORT environment variables. - // If undefined, the host will default to 'localhost' and the port - // will default to 8125. - // Moda configuration defines DD_DOGSTATSD_PORT but not DD_AGENT_HOST. - // For Moda, the host must be set to the Kubernetes node name, which is - // set in KUBE_NODE_HOSTNAME. + // hot-shots falls back to DD_AGENT_HOST and DD_DOGSTATSD_PORT, + // then to localhost:8125. + // Moda defines DD_DOGSTATSD_PORT but not DD_AGENT_HOST, + // and needs the host set to the Kubernetes node name from KUBE_NODE_HOSTNAME. host: DD_AGENT_HOST || KUBE_NODE_HOSTNAME, port: DD_DOGSTATSD_PORT ? parseInt(DD_DOGSTATSD_PORT, 10) : undefined, prefix: 'docs.', diff --git a/src/observability/lib/to-error.ts b/src/observability/lib/to-error.ts index 19f7e551b1e5..583b66e5fbd8 100644 --- a/src/observability/lib/to-error.ts +++ b/src/observability/lib/to-error.ts @@ -1,5 +1,4 @@ -// Safely convert an unknown thrown value to an Error, avoiding JSON.stringify -// which can throw on circular references. +// JSON.stringify can throw on circular references, so fall back to String(). export function toError(value: Error | unknown): Error { if (value instanceof Error) return value try { diff --git a/src/observability/lib/tracing.browser.ts b/src/observability/lib/tracing.browser.ts index 847629fc1ff8..c9413916387a 100644 --- a/src/observability/lib/tracing.browser.ts +++ b/src/observability/lib/tracing.browser.ts @@ -1,3 +1,3 @@ -// Browser stub for tracing.ts — OTel is server-only. +// Browser stub for tracing.ts: OTel is server-only. // This file is aliased in by Next.js webpack and Turbopack for client bundles. // It's a no-op: tracing.ts is a side-effect-only module with no exports. diff --git a/src/observability/lib/tracing.ts b/src/observability/lib/tracing.ts index e14f29b859e6..91f4e9ecb874 100644 --- a/src/observability/lib/tracing.ts +++ b/src/observability/lib/tracing.ts @@ -1,16 +1,14 @@ -// OpenTelemetry distributed tracing setup for docs-internal. +// OpenTelemetry distributed tracing setup for docs-internal, +// following the same pattern as github/alloy and github/github-ui. // -// Follows the same pattern as github/alloy and github/github-ui: -// - Conditional on OTEL_EXPORTER_OTLP_TRACES_ENDPOINT being set -// - OTLP/HTTP (proto) exporter to OTel Collector mesh -// - W3C Trace Context + Baggage propagation -// - Explicit instrumentation list (HTTP, Express, Undici/fetch) instead of -// `getNodeAutoInstrumentations()`. The "auto" helper enables ~30 -// instrumentations including ones that patch Node core modules -// (`fs`, `net`, `dns`) on every server. Several of these are known to -// cause performance and listener-leak issues — OTel itself recommends -// disabling `instrumentation-fs` in production. We only have HTTP traffic -// and outbound fetch in this app, so we wire those up explicitly. +// The instrumentation list (HTTP, Express, Undici/fetch) is explicit +// instead of `getNodeAutoInstrumentations()`. +// The "auto" helper enables ~30 instrumentations, +// including ones that patch Node core modules (`fs`, `net`, `dns`) on every server. +// Several of these are known to cause performance and listener-leak issues, +// and OTel itself recommends disabling `instrumentation-fs` in production. +// We only have HTTP traffic and outbound fetch in this app, +// so we wire those up explicitly. // // References: // - https://thehub.github.com/epd/engineering/dev-practicals/observability/distributed-tracing/ @@ -53,7 +51,6 @@ if (process.env.OTEL_EXPORTER_OTLP_TRACES_ENDPOINT) { }) } - // Gracefully shut down the SDK on process exit. // Uses `once` to prevent duplicate shutdown if SIGTERM is delivered multiple times. process.once('SIGTERM', async () => { try { diff --git a/src/observability/logger/index.ts b/src/observability/logger/index.ts index 9e115207c3dc..3495a799c2d0 100644 --- a/src/observability/logger/index.ts +++ b/src/observability/logger/index.ts @@ -25,7 +25,6 @@ function formatTimestamp(): string { return `${h}:${m}:${s}.${ms}` } -// Format non-error included context as compact key=value pairs function formatContext(ctx: Record): string { const parts: string[] = [] for (const [key, value] of Object.entries(ctx)) { @@ -46,7 +45,6 @@ function formatContext(ctx: Record): string { return parts.length > 0 ? ` ${parts.join(' ')}` : '' } -// Safely resolve filePath to a relative path. // Handles file:// URLs (from import.meta.url) and plain string labels. function resolveFilePath(filePath: string): string { try { @@ -63,27 +61,18 @@ type IncludeContext = { [key: string]: unknown } // BUILD_SHA is baked into each Docker image via ARG/ENV in the Dockerfile. const BUILD_SHA = process.env.BUILD_SHA || undefined -// Type definitions for logger methods with overloads +// A trailing plain object is treated as extra context, not a message part. +// Error arguments are extracted from anywhere in the list, +// and their messages appended to the log message. interface LoggerMethod { - // Pattern 1: Just a message e.g. `logger.info('Hello world')` (message: string): void - // Pattern 2: Message with extraData object e.g. `logger.info('Hello world', { userId: 123 })` (message: string, extraData: IncludeContext): void - // Pattern 3: Multiple message parts e.g. `logger.info('Hello', 'world', 123, true)` (message: string, ...messageParts: (string | number | boolean)[]): void - // Pattern 4: Multiple message parts followed by extraData object e.g. - // `logger.info('Hello', 'world', 123, true, { userId: 123 })` - // Note: The extraData object must be the last argument ( message: string, ...args: [...messageParts: (string | number | boolean)[], extraData: IncludeContext] ): void - // Pattern 5: Message with Error object (automatically handled) e.g. - // `logger.error('Database error', error)` - // Note: This will append the error message to the final log message (message: string, error: Error): void - // Pattern 6: Message with multiple parts and Error objects - // e.g. `logger.error('Multiple failures', error1, error2)` (message: string, ...args: (string | number | boolean | Error | IncludeContext | object)[]): void } @@ -99,7 +88,6 @@ export function createLogger(filePath: string) { throw new Error('createLogger must be called with the import.meta.url argument') } - // Helper function to check if a value is a plain object (not Array, Error, Date, etc.) function isPlainObject(value: unknown): value is Record { return ( value !== null && @@ -111,13 +99,10 @@ export function createLogger(filePath: string) { ) } - // The actual log function used by each level-specific method. function logMessage(level: keyof typeof LOG_LEVELS, message: string, ...args: unknown[]) { - // Determine if we have extraData or additional message parts let finalMessage: string let includeContext: IncludeContext = {} - // First, extract any Error objects from the arguments and handle them specially const errorObjects: Error[] = [] const nonErrorArgs: unknown[] = [] @@ -129,42 +114,33 @@ export function createLogger(filePath: string) { } } - // Handle the non-error arguments for message building and extraData if (nonErrorArgs.length > 0 && isPlainObject(nonErrorArgs[nonErrorArgs.length - 1])) { - // Last non-error argument is a plain object - treat as extraData includeContext = { ...(nonErrorArgs[nonErrorArgs.length - 1] as IncludeContext) } const messageParts = nonErrorArgs.slice(0, -1) if (messageParts.length > 0) { - // There are message parts before the extraData object const allMessageParts = [ message, ...messageParts.map((arg) => (typeof arg === 'string' ? arg : String(arg))), ] finalMessage = allMessageParts.join(' ') } else { - // Only the extraData object, no additional message parts finalMessage = message } } else if (nonErrorArgs.length > 0) { - // Multiple arguments or non-plain-object - concatenate as message parts const allMessageParts = [ message, ...nonErrorArgs.map((arg) => (typeof arg === 'string' ? arg : String(arg))), ] finalMessage = allMessageParts.join(' ') } else { - // No additional non-error arguments finalMessage = message } - // Add Error objects to includeContext and optionally to the message if (errorObjects.length > 0) { if (errorObjects.length === 1) { - // Single error - use 'error' key and append error message to final message includeContext.error = errorObjects[0] finalMessage = `${finalMessage}: ${errorObjects[0].message}` } else { - // Multiple errors - use indexed keys and append all error messages for (let index = 0; index < errorObjects.length; index++) { const error = errorObjects[index] includeContext[`error_${index + 1}`] = error @@ -173,7 +149,6 @@ export function createLogger(filePath: string) { finalMessage = `${finalMessage}: ${errorMessages}` } } - // Compare the requested level's priority to current environment's level const currentLogLevel = getLogLevelNumber() if (LOG_LEVELS[level] > currentLogLevel) { return // Do not log if the requested level is lower priority @@ -183,7 +158,6 @@ export function createLogger(filePath: string) { const timestamp = new Date().toISOString() if (useProductionLogging()) { - // Logfmt logging in production const logObject: IncludeContext = { ...POD_IDENTITY, // pod_name, pod_namespace, node_hostname (static; {} in local dev) ...loggerContext, // requestUuid, path, method, headers, etc. (per-request) @@ -194,7 +168,6 @@ export function createLogger(filePath: string) { message: finalMessage, } - // Add any included context to the log object const includedContextWithFormattedError = {} as IncludeContext for (const [key, value] of Object.entries(includeContext)) { if (typeof value === 'object' && value instanceof Error) { @@ -208,12 +181,11 @@ export function createLogger(filePath: string) { } } - // Add extra context to its own key in the log object to prevent conflicts with loggerContext keys + // Nested under its own key to avoid colliding with loggerContext keys. logObject.included = includedContextWithFormattedError console.log(toLogfmt(logObject)) } else { - // Human-readable dev/script logging const relFile = resolveFilePath(filePath) const ts = formatTimestamp() const colorFn = LEVEL_COLORS[level] @@ -221,7 +193,7 @@ export function createLogger(filePath: string) { const fileTag = chalk.dim(`(${relFile})`) const contextStr = formatContext(includeContext) - // If the log includes an error, print the Error object to console.error in local dev + // Prints the message line once per error, so a two-error call repeats it. let wasErrorLog = false for (const [, value] of Object.entries(includeContext)) { if (typeof value === 'object' && value instanceof Error) { diff --git a/src/observability/logger/lib/log-levels.ts b/src/observability/logger/lib/log-levels.ts index 74aad544841c..fcf3fd40a419 100644 --- a/src/observability/logger/lib/log-levels.ts +++ b/src/observability/logger/lib/log-levels.ts @@ -1,9 +1,7 @@ /* -The log level is controlled by the `LOG_LEVEL` environment variable, where lower log levels = more verbose - examples: - if log level is 'info', only 'info', 'warn', and 'error' logs will be output - if log level is 'debug', all logs will be output - if log level is 'error', only 'error' logs will be output +The log level is controlled by the `LOG_LEVEL` environment variable, where lower +log levels = more verbose. If log level is 'info', only 'info', 'warn', and +'error' logs are output. */ export const LOG_LEVELS = { error: 0, @@ -19,11 +17,10 @@ function isValidLogLevel(level: string): level is LogLevel { return level in LOG_LEVELS } -// We set the log level based on the LOG_LEVEL environment variable -// but default to: +// Defaults when LOG_LEVEL isn't set: // - 'info' in development // - 'debug' in production -// - 'debug' in test - this is because `vitest` turns off logs unless --silent=false is passed +// - 'debug' in test, because `vitest` turns off logs unless --silent=false is passed export function getLogLevelNumber(): LogLevelValue { let defaultLogLevel: LogLevel = 'info' if ( diff --git a/src/observability/logger/lib/logger-context.ts b/src/observability/logger/lib/logger-context.ts index ca129a64933d..baff1072e685 100644 --- a/src/observability/logger/lib/logger-context.ts +++ b/src/observability/logger/lib/logger-context.ts @@ -1,10 +1,9 @@ import { AsyncLocalStorage } from 'async_hooks' import type { NextFunction, Request, Response } from 'express' -// Think of this like a Redux store, but for the backend -// During an early middleware, we call asyncLocalStorage.run(store, () => { next() }) -// This ensures that all downstream middleware can access `store` from the asyncLocalStorage, -// using the `getLoggerContext` function. +// Think of this like a Redux store, but for the backend. +// An early middleware calls asyncLocalStorage.run(store, ...), +// which lets all downstream middleware read the store via `getLoggerContext`. export const asyncLocalStorage = new AsyncLocalStorage() export type LoggerContext = { @@ -35,7 +34,6 @@ export function getLoggerContext(): LoggerContext { return store as LoggerContext } -// Called in subsequent middleware to update the request context export function updateLoggerContext(newContext: Partial): void { const store = asyncLocalStorage.getStore() if (!store) { @@ -65,7 +63,6 @@ export function initLoggerContext(req: Request, res: Response, next: NextFunctio const requestUuid = crypto.randomUUID() const headers = {} as Record - // Only include the headers we care about for (const [key, value] of Object.entries(req.headers)) { if (INCLUDE_HEADERS.includes(key)) { if (!value) { @@ -78,7 +75,6 @@ export function initLoggerContext(req: Request, res: Response, next: NextFunctio } } - // This is all of the context we want to include for each logger. call const store: LoggerContext = { requestUuid, path: req.path, @@ -88,7 +84,6 @@ export function initLoggerContext(req: Request, res: Response, next: NextFunctio body: req.body, } - // Subsequent middleware and route handlers will have access to the { requestId } store asyncLocalStorage.run(store, () => { next() }) diff --git a/src/observability/logger/lib/pod-identity.ts b/src/observability/logger/lib/pod-identity.ts index 9ece83385e4d..655b9d967597 100644 --- a/src/observability/logger/lib/pod-identity.ts +++ b/src/observability/logger/lib/pod-identity.ts @@ -1,5 +1,5 @@ -// Static pod-identity fields — read once at module load, never change. -// Only populated when running in Kubernetes (env vars set via Downward API + kube-cluster-metadata). +// Read once at module load, so these never change. +// Only populated in Kubernetes (env vars set via Downward API + kube-cluster-metadata). export const POD_IDENTITY: Record = {} if (process.env.POD_NAME) POD_IDENTITY.podName = process.env.POD_NAME if (process.env.POD_NAMESPACE) POD_IDENTITY.podNamespace = process.env.POD_NAMESPACE diff --git a/src/observability/logger/lib/to-logfmt.ts b/src/observability/logger/lib/to-logfmt.ts index f8c0afbb5964..c1b5f1648243 100644 --- a/src/observability/logger/lib/to-logfmt.ts +++ b/src/observability/logger/lib/to-logfmt.ts @@ -14,10 +14,7 @@ a=1 b.c=2 */ -/** - * Custom logfmt stringify implementation - * Based on the original node-logfmt library behavior - */ +// Matches the original node-logfmt library's quoting and escaping behavior. function stringify(data: Record): string { let line = '' @@ -54,7 +51,6 @@ function stringify(data: Record): string { } export function toLogfmt(jsonString: Record): string { - // Helper function to flatten nested objects const flattenObject = ( obj: Record, parentKey: string = '', @@ -66,25 +62,21 @@ export function toLogfmt(jsonString: Record): string { const value = obj[key] if (value && typeof value === 'object') { - // Handle circular references if (seen.has(value)) { result[newKey] = '[Circular]' continue } - // Handle Date objects specially if (value instanceof Date) { result[newKey] = value.toISOString() continue } - // Handle arrays if (Array.isArray(value)) { result[newKey] = value.join(',') continue } - // Handle other objects - only flatten if not empty const valueKeys = Object.keys(value as Record) if (valueKeys.length > 0) { seen.add(value) @@ -92,7 +84,7 @@ export function toLogfmt(jsonString: Record): string { seen.delete(value) } } else { - // Convert undefined values to null, as they are not supported by logfmt + // undefined and empty strings become null, which logfmt can represent. result[newKey] = value === undefined || (typeof value === 'string' && value === '') ? null : value } diff --git a/src/observability/logger/middleware/get-automatic-request-logger.ts b/src/observability/logger/middleware/get-automatic-request-logger.ts index 9e55d1c23eea..946695ace75e 100644 --- a/src/observability/logger/middleware/get-automatic-request-logger.ts +++ b/src/observability/logger/middleware/get-automatic-request-logger.ts @@ -5,11 +5,7 @@ import { getLogLevelNumber, useProductionLogging } from '@/observability/logger/ import { toLogfmt } from '@/observability/logger/lib/to-logfmt' import { POD_IDENTITY } from '@/observability/logger/lib/pod-identity' -/** - * Check if automatic development logging is enabled. - * We don't turn on automatic logging for tests & GitHub Actions by default, - * but you can override this using the ENABLE_DEV_LOGGING environment variable. - */ +// Off by default for tests and GitHub Actions. Override with ENABLE_DEV_LOGGING. function shouldEnableAutomaticDevLogging(): boolean { const isTest = process.env.NODE_ENV === 'test' || process.env.GITHUB_ACTIONS === 'true' return Boolean( @@ -17,23 +13,14 @@ function shouldEnableAutomaticDevLogging(): boolean { ) } -/** - * Returns a custom middleware that automatically logs request details. - * - * e.g. `GET /path/to/resource 200 5.000 ms - 1234` - * - * In production, we include the logger context and print in logfmt format - * In development, we print colored strings for better readability - * In test, the request details are not logged. - */ +// Emits one line per response, like: GET /path/to/resource 200 5.000 ms - 1234 +// Tests and Actions stay silent unless ENABLE_DEV_LOGGING overrides. export function getAutomaticRequestLogger() { return (req: Request, res: Response, next: NextFunction) => { const startTime = Date.now() - // Store original end method to capture response completion const originalEnd = res.end - // Override res.end to log when response completes res.end = function (...args: unknown[]) { const responseTime = Date.now() - startTime const status = res.statusCode || 200 @@ -42,7 +29,6 @@ export function getAutomaticRequestLogger() { const url = req.originalUrl || req.url if (useProductionLogging()) { - // Production: log in logfmt format with full context const loggerContext = getLoggerContext() console.log( toLogfmt({ @@ -56,7 +42,6 @@ export function getAutomaticRequestLogger() { }), ) } else if (shouldEnableAutomaticDevLogging()) { - // Development: log colored strings for readability const logLevelNum = getLogLevelNumber() // Don't log `/_next/` requests unless LOG_LEVEL is `debug` or higher @@ -64,7 +49,6 @@ export function getAutomaticRequestLogger() { return originalEnd.apply(this, args as Parameters) } - // Choose color based on status code const color = status >= 500 ? 'red' : status >= 400 ? 'yellow' : status >= 300 ? 'cyan' : 'green' @@ -81,7 +65,6 @@ export function getAutomaticRequestLogger() { console.log(logLine) } - // Call the original end method to complete the response return originalEnd.apply(this, args as Parameters) } diff --git a/src/observability/middleware/catch-middleware-error.ts b/src/observability/middleware/catch-middleware-error.ts index d4f754aa7808..99b1870f5747 100644 --- a/src/observability/middleware/catch-middleware-error.ts +++ b/src/observability/middleware/catch-middleware-error.ts @@ -1,6 +1,5 @@ import type { NextFunction, Request, Response, RequestHandler } from 'express' -// Middleware function type that accepts various Express handler signatures. // Generic over request/response types so callers with narrower types stay type-safe. export interface MiddlewareFn { (req: Req, res: Res, next: NextFunction): unknown diff --git a/src/observability/middleware/handle-errors.ts b/src/observability/middleware/handle-errors.ts index 0e61539d5c39..f43857cffdf1 100644 --- a/src/observability/middleware/handle-errors.ts +++ b/src/observability/middleware/handle-errors.ts @@ -32,7 +32,6 @@ function shouldLogException(error: ErrorWithCode) { return false } - // We should log this exception return true } @@ -55,17 +54,14 @@ async function handleError( if (req.path.startsWith('/assets') || req.path.startsWith('/_next/static')) { if (!responseDone) { - // By default, Fastly will cache 404 responses unless otherwise - // told not to. - // See https://docs.fastly.com/en/guides/how-caching-and-cdns-work#http-status-codes-cached-by-default - // Let's cache our 404'ing assets conservatively. - // The Cache-Control is short, and let's use the default surrogate - // key just in case it was a mistake. + // Fastly caches 404s by default, so cache 404'ing assets conservatively: + // a short Cache-Control, plus the default surrogate key + // in case the 404 was a mistake. + // https://docs.fastly.com/en/guides/how-caching-and-cdns-work#http-status-codes-cached-by-default errorCacheControl(res) - // Makes sure the surrogate key is NOT the manual one if it failed. - // This basically unsets what was assumed in the beginning of - // loading all the middlewares. Falls back to `no-language` when - // `req.language` isn't set yet (e.g. errors before language detection). + // Unsets the manual surrogate key assumed earlier in the middleware chain. + // Falls back to `no-language` when `req.language` isn't set yet, + // e.g. errors before language detection. setFastlySurrogateKey(res, makeLanguageSurrogateKey(req.language), true) } } else if (DEBUG_MIDDLEWARE_TESTS) { @@ -73,9 +69,7 @@ async function handleError( } try { - // If the headers have already been sent or the request was aborted... if (responseDone) { - // Report to Failbot if (typeof error !== 'number') { await logException(error, req) } @@ -105,21 +99,17 @@ async function handleError( req.context.error = error } - // If the error contains a status code, just send that back. This is usually - // from a middleware like `express.json()`. + // Errors with a status code usually come from a middleware like `express.json()`. if (error.statusCode) { res.sendStatus(error.statusCode) return } res.statusCode = 500 - // When in local development mode, we don't need the pretty HTML - // renderig of 500.tsx. - // Incidentally, as Jan 2024, if you try to execute nextApp.renderError - // when `NODE_ENV` is 'development' it will hang forever. A problem - // we can't fully explain but it's also moot because in local dev - // it's easier to just see the full stack trace in the console - // and in the client. + // Local dev doesn't need the pretty HTML rendering of 500.tsx. + // Also, as of Jan 2024, calling nextApp.renderError hangs forever + // when `NODE_ENV` is 'development'. We can't fully explain it, + // and it's moot because in local dev the full stack trace is more useful. if (process.env.NODE_ENV === 'development') { next(error) return From 20e9ba9e7b785e74f72582923df57c16c06340a4 Mon Sep 17 00:00:00 2001 From: Kevin Heis Date: Tue, 15 Sep 2026 20:32:57 +0000 Subject: [PATCH 14/16] Trim excessive comments in src/article-api tests (#63280) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> Copilot-Session: 17588eeb-788a-4f82-9d36-b5079fb36521 --- src/article-api/tests/article-body.ts | 3 -- .../tests/audit-logs-transformer.ts | 11 +------ .../tests/bespoke-landing-transformer.ts | 3 -- .../tests/codeql-cli-transformer.ts | 4 --- .../tests/discovery-landing-transformer.ts | 7 ----- src/article-api/tests/get-link-data.ts | 2 -- .../tests/github-apps-transformer.ts | 23 +------------- src/article-api/tests/graphql-transformer.ts | 29 ++--------------- .../tests/journey-landing-transformer.ts | 1 - src/article-api/tests/pageinfo.ts | 2 -- src/article-api/tests/pagelist.ts | 11 +------ src/article-api/tests/resolve-path.ts | 2 -- src/article-api/tests/rest-transformer.ts | 31 +------------------ .../tests/secret-scanning-transformer.ts | 6 +--- src/article-api/tests/toc-transformer.ts | 7 +---- src/article-api/tests/webhooks-transformer.ts | 13 -------- 16 files changed, 8 insertions(+), 147 deletions(-) diff --git a/src/article-api/tests/article-body.ts b/src/article-api/tests/article-body.ts index 932d3e230ebb..ea47acacb8b2 100644 --- a/src/article-api/tests/article-body.ts +++ b/src/article-api/tests/article-body.ts @@ -38,7 +38,6 @@ describe('article body api', () => { const res = await get(makeURL('/en/get-started/start-your-journey/hello-world')) expect(res.statusCode).toBe(200) - // Check that octicons without aria-label get auto-generated ones expect(res.body).toContain('aria-label="check icon"') expect(res.body).toContain('aria-label="git branch icon"') }) @@ -47,7 +46,6 @@ describe('article body api', () => { const res = await get(makeURL('/en/get-started/start-your-journey/hello-world')) expect(res.statusCode).toBe(200) - // Check that custom aria-labels are preserved expect(res.body).toContain('aria-label="Supported"') expect(res.body).toContain('aria-label="Not supported"') }) @@ -56,7 +54,6 @@ describe('article body api', () => { const res = await get(makeURL('/en/get-started/start-your-journey/hello-world')) expect(res.statusCode).toBe(200) - // Check that octicons with width attribute still get aria-labels expect(res.body).toContain('aria-label="rocket icon"') expect(res.body).toContain('width="32"') }) diff --git a/src/article-api/tests/audit-logs-transformer.ts b/src/article-api/tests/audit-logs-transformer.ts index d7869a6538ac..9e20c1cd2874 100644 --- a/src/article-api/tests/audit-logs-transformer.ts +++ b/src/article-api/tests/audit-logs-transformer.ts @@ -23,21 +23,16 @@ describe('Audit Logs transformer', () => { expect(res.statusCode).toBe(200) expect(res.headers['content-type']).toContain('text/markdown') - // Check for the main heading expect(res.body).toContain('# Security log events') - // Check for intro expect(res.body).toContain( 'Learn about security log events recorded for your personal account.', ) - // Check for manual content section heading expect(res.body).toContain('## About security log events') - // Check for new main heading expect(res.body).toContain('## Audit log events') - // Check for category heading // The template renders "### Category" expect(res.body).toMatch(/### \w+/) }) @@ -72,17 +67,14 @@ describe('Audit Logs transformer', () => { ) expect(res.statusCode).toBe(200) - // Check for event action header // #### `action.name` expect(res.body).toMatch(/#### `[\w.]+`/) - // Check for fields section - either common fields summary or additional fields per event const body = res.body const hasCommonFields = body.includes('### Common fields') const hasAdditionalFields = body.includes('**Additional fields:**') expect(hasCommonFields || hasAdditionalFields).toBe(true) - // Validate that a known common field is in the common section and not duplicated if (hasCommonFields) { const commonFieldsIndex = body.indexOf('### Common fields') const commonFieldsSection = body.slice( @@ -92,7 +84,7 @@ describe('Audit Logs transformer', () => { expect(commonFieldsSection).toContain('`action`') } - // Ensure common fields do not appear in any "Additional fields" section + // Common fields are listed once in their own section, never repeated per event. if (hasAdditionalFields) { const additionalSections = body.split('**Additional fields:**').slice(1) for (const section of additionalSections) { @@ -101,7 +93,6 @@ describe('Audit Logs transformer', () => { } } - // Check for reference section expect(res.body).toContain('**Reference:**') }) diff --git a/src/article-api/tests/bespoke-landing-transformer.ts b/src/article-api/tests/bespoke-landing-transformer.ts index 0e6828ea5c0a..3e300116fedc 100644 --- a/src/article-api/tests/bespoke-landing-transformer.ts +++ b/src/article-api/tests/bespoke-landing-transformer.ts @@ -12,10 +12,8 @@ describe('bespoke landing transformer', () => { expect(res.statusCode).toBe(200) expect(res.headers['content-type']).toContain('text/markdown') - // Check for title expect(res.body).toContain('# Article Grid Bespoke Landing') - // Should have intro expect(res.body).toContain('A test page for testing') }) @@ -23,7 +21,6 @@ describe('bespoke landing transformer', () => { const res = await get(makeURL('/en/get-started/article-grid-bespoke')) expect(res.statusCode).toBe(200) - // Should have Articles section with all descendant articles (recursive) expect(res.body).toContain('## Articles') expect(res.body).toContain('[Grid Article One]') expect(res.body).toContain('[Grid Article Two]') diff --git a/src/article-api/tests/codeql-cli-transformer.ts b/src/article-api/tests/codeql-cli-transformer.ts index 2f82ae5279c1..dca2b457f07f 100644 --- a/src/article-api/tests/codeql-cli-transformer.ts +++ b/src/article-api/tests/codeql-cli-transformer.ts @@ -21,20 +21,16 @@ describe('codeql cli article body api', () => { expect(res.statusCode).toBe(200) expect(res.headers['content-type']).toContain('text/markdown') - // Check for title injection expect(res.body).toContain('# database analyze') - // Check for intro injection expect(res.body).toContain( 'Analyze a database, producing meaningful results in the context of the source code.', ) - // Check for content expect(res.body).toContain('## Synopsis') expect(res.body).toContain('## Description') expect(res.body).toContain('## Options') - // Verify HTML comments are stripped expect(res.body).not.toContain('') expect(res.body).not.toContain('') expect(res.body).not.toContain('') diff --git a/src/article-api/tests/discovery-landing-transformer.ts b/src/article-api/tests/discovery-landing-transformer.ts index fe013bf7f5e6..8c300d0fd713 100644 --- a/src/article-api/tests/discovery-landing-transformer.ts +++ b/src/article-api/tests/discovery-landing-transformer.ts @@ -12,11 +12,9 @@ describe('discovery landing transformer', () => { expect(res.statusCode).toBe(200) expect(res.headers['content-type']).toContain('text/markdown') - // Check for title and intro expect(res.body).toContain('# Landing Page Carousel') expect(res.body).toContain('A test category page for testing the LandingCarousel component') - // Should have Articles section with all descendant articles expect(res.body).toContain('## Articles') expect(res.body).toContain('[Carousel Article One]') expect(res.body).toContain('[Carousel Article Two]') @@ -29,10 +27,8 @@ describe('discovery landing transformer', () => { expect(res.statusCode).toBe(200) expect(res.headers['content-type']).toContain('text/markdown') - // Check for title expect(res.body).toContain('# Article Grid Discovery') - // Should have Articles section with all descendant articles (recursive) expect(res.body).toContain('## Articles') expect(res.body).toContain('[Grid Article One]') expect(res.body).toContain('[Grid Article Two]') @@ -41,14 +37,11 @@ describe('discovery landing transformer', () => { }) test('handles discovery landing structure consistently', async () => { - // Discovery pages should have a consistent structure const res = await get(makeURL('/en/get-started/carousel')) expect(res.statusCode).toBe(200) - // Should have intro expect(res.body).toMatch(/^# .+\n\n.+\n\n/) - // Should have at least one section expect(res.body).toContain('##') }) diff --git a/src/article-api/tests/get-link-data.ts b/src/article-api/tests/get-link-data.ts index fc11254faba3..41f9a16b4e39 100644 --- a/src/article-api/tests/get-link-data.ts +++ b/src/article-api/tests/get-link-data.ts @@ -2,7 +2,6 @@ import { describe, expect, test, vi } from 'vitest' import { getLinkData } from '@/article-api/lib/get-link-data' import type { Context, Page, Permalink } from '@/types' -// Helper to create a minimal mock page function createMockPage(options: { title?: string intro?: string @@ -18,7 +17,6 @@ function createMockPage(options: { return page as unknown as Page } -// Helper to create a minimal context function createContext(currentVersion = 'free-pro-team@latest'): Context { return { currentVersion } as unknown as Context } diff --git a/src/article-api/tests/github-apps-transformer.ts b/src/article-api/tests/github-apps-transformer.ts index 80fc1acfc4bc..d2ff01d2dfcb 100644 --- a/src/article-api/tests/github-apps-transformer.ts +++ b/src/article-api/tests/github-apps-transformer.ts @@ -29,13 +29,10 @@ describe('GitHub Apps transformer', () => { expect(res.statusCode).toBe(200) expect(res.headers['content-type']).toContain('text/markdown') - // Check for the main heading expect(res.body).toContain('# Endpoints available for GitHub App installation access tokens') - // Should have category headings as h2 expect(res.body).toMatch(/^## /m) - // Should not contain HTML comments expect(res.body).not.toMatch(//) }) @@ -46,10 +43,8 @@ describe('GitHub Apps transformer', () => { expect(res.statusCode).toBe(200) expect(res.headers['content-type']).toContain('text/markdown') - // Check for the main heading expect(res.body).toContain('# Endpoints available for GitHub App user access tokens') - // Should have category headings as h2 expect(res.body).toMatch(/^## /m) }) @@ -62,10 +57,8 @@ describe('GitHub Apps transformer', () => { expect(res.statusCode).toBe(200) expect(res.headers['content-type']).toContain('text/markdown') - // Check for the main heading expect(res.body).toContain('# Endpoints available for fine-grained personal access tokens') - // Should have category headings as h2 expect(res.body).toMatch(/^## /m) }) @@ -81,7 +74,7 @@ describe('GitHub Apps transformer', () => { console.log(`[DEBUG] Test response: ${res.statusCode} in ${Date.now() - startTime}ms`) expect(res.statusCode).toBe(200) - // Check for bullet list items with asterisks (per content guidelines) + // Content guidelines require asterisk bullets, not hyphens. expect(res.body).toContain('*') expect(res.body).toMatch(/\* \[`[A-Z]+ \//) }) @@ -94,7 +87,6 @@ describe('GitHub Apps transformer', () => { ) expect(res.statusCode).toBe(200) - // Check for common HTTP verbs expect(res.body).toMatch(/`GET \//) // May also have POST, PUT, PATCH, DELETE depending on data }) @@ -118,10 +110,8 @@ describe('GitHub Apps transformer', () => { expect(res.statusCode).toBe(200) expect(res.headers['content-type']).toContain('text/markdown') - // Check for the main heading expect(res.body).toContain('# Permissions required for GitHub Apps') - // Should have permission group headings as h2 expect(res.body).toMatch(/^## /m) }) @@ -134,10 +124,8 @@ describe('GitHub Apps transformer', () => { expect(res.statusCode).toBe(200) expect(res.headers['content-type']).toContain('text/markdown') - // Check for the main heading expect(res.body).toContain('# Permissions required for fine-grained personal access tokens') - // Should have permission group headings as h2 expect(res.body).toMatch(/^## /m) }) @@ -145,7 +133,6 @@ describe('GitHub Apps transformer', () => { const res = await get(makeURL('/en/rest/authentication/permissions-required-for-github-apps')) expect(res.statusCode).toBe(200) - // Check for table structure expect(res.body).toContain('| Endpoint | Access | Tokens | Additional Permissions |') expect(res.body).toContain('|----------|--------|--------|------------------------|') }) @@ -154,7 +141,6 @@ describe('GitHub Apps transformer', () => { const res = await get(makeURL('/en/rest/authentication/permissions-required-for-github-apps')) expect(res.statusCode).toBe(200) - // Check for access levels in table cells expect(res.body).toMatch(/\| read \|/) expect(res.body).toMatch(/\| write \|/) // May also have admin depending on data @@ -164,11 +150,9 @@ describe('GitHub Apps transformer', () => { const res = await get(makeURL('/en/rest/authentication/permissions-required-for-github-apps')) expect(res.statusCode).toBe(200) - // Check for legend expect(res.body).toContain('UAT = user access token') expect(res.body).toContain('IAT = installation access token') - // Check that token types appear in actual table rows (not just the legend) expect(res.body).toMatch(/\|\s*(?:UAT|IAT|UAT, IAT|None)\s*\|/) }) @@ -176,7 +160,6 @@ describe('GitHub Apps transformer', () => { const res = await get(makeURL('/en/rest/authentication/permissions-required-for-github-apps')) expect(res.statusCode).toBe(200) - // Check for checkmark (āœ“) or cross (āœ—) symbols in Additional Permissions column expect(res.body).toMatch(/\| [āœ“āœ—] \|/) }) @@ -184,7 +167,6 @@ describe('GitHub Apps transformer', () => { const res = await get(makeURL('/en/rest/authentication/permissions-required-for-github-apps')) expect(res.statusCode).toBe(200) - // Should have multiple permission group headings const headings = res.body.match(/^## .* permissions for .*/gm) expect(headings).toBeTruthy() if (headings) { @@ -198,7 +180,6 @@ describe('GitHub Apps transformer', () => { const res = await get(makeURL('/en/rest/authentication/permissions-required-for-github-apps')) expect(res.statusCode).toBe(200) - // Check for manual content that should be in the markdown expect(res.body).toContain('GitHub Apps are created with a set of permissions') }) @@ -222,7 +203,6 @@ describe('GitHub Apps transformer', () => { const res = await get(makeURL('/en/rest/authentication/permissions-required-for-github-apps')) expect(res.statusCode).toBe(200) - // Check that AUTOTITLE has been resolved expect(res.body).not.toContain('[AUTOTITLE]') }) @@ -247,7 +227,6 @@ describe('GitHub Apps transformer', () => { test('Missing apiVersion defaults to latest', async () => { const res = await get(makeURL('/en/rest/authentication/permissions-required-for-github-apps')) expect(res.statusCode).toBe(200) - // Should work without explicit apiVersion }) test('Non-GitHub Apps pages are not transformed', async () => { diff --git a/src/article-api/tests/graphql-transformer.ts b/src/article-api/tests/graphql-transformer.ts index f0668c40b88c..127584b86d46 100644 --- a/src/article-api/tests/graphql-transformer.ts +++ b/src/article-api/tests/graphql-transformer.ts @@ -8,7 +8,7 @@ const makeURL = (pathname: string): string => { } describe('GraphQL transformer', { timeout: 10000 }, () => { - // Cache expensive responses to avoid duplicate requests + // Several tests hit the same URL, so cache slow responses to avoid repeated requests. const responseCache = new Map>>() const getCached = async (url: string) => { @@ -32,7 +32,6 @@ describe('GraphQL transformer', { timeout: 10000 }, () => { expect(res.statusCode).toBe(200) expect(res.headers['content-type']).toContain('text/markdown') - // Check for the main heading expect(res.body).toContain('# Repositories') // Items render as flat alphabetical level 2 headings with a kind suffix @@ -47,10 +46,8 @@ describe('GraphQL transformer', { timeout: 10000 }, () => { // Item headings are now at level 3 expect(res.body).toContain('## repository - query') - // Check for query description expect(res.body).toContain('Lookup a given repository by the owner and repository name.') - // Check for type (without link) expect(res.body).toContain('**Type:** Repository') }) @@ -61,7 +58,6 @@ describe('GraphQL transformer', { timeout: 10000 }, () => { // codeOfConduct query is in the meta category expect(res.body).toContain('### Arguments for `codeOfConduct`') - // Check for specific arguments in bullet format expect(res.body).toContain('`key` (String!)') expect(res.body).toContain("The code of conduct's key.") }) @@ -101,7 +97,6 @@ describe('GraphQL transformer', { timeout: 10000 }, () => { expect(res.body).toContain('## User - object') expect(res.body).toContain('`repositories`') - // Check for nested argument bullets expect(res.body).toContain('`first`') expect(res.body).toContain('Returns the first n elements from the list.') expect(res.body).toContain('`orderBy`') @@ -172,10 +167,8 @@ describe('GraphQL transformer', { timeout: 10000 }, () => { const res = await getCached('/en/graphql/reference') expect(res.statusCode).toBe(200) - // Check for main heading expect(res.body).toContain('# Reference') - // Check for intro with liquid variable rendered expect(res.body).toMatch(/(GitHub|HubGit) GraphQL API schema/) }) }) @@ -185,15 +178,12 @@ describe('GraphQL transformer', { timeout: 10000 }, () => { const res = await getCached('/en/graphql/overview/changelog') expect(res.statusCode).toBe(200) - // Check for main heading expect(res.body).toContain('# Changelog') - // Check for intro expect(res.body).toContain( 'The GraphQL schema changelog is a list of recent and upcoming changes', ) - // Check for manual content expect(res.body).toContain( 'Breaking changes include changes that will break existing queries', ) @@ -201,13 +191,10 @@ describe('GraphQL transformer', { timeout: 10000 }, () => { // Index page shows latest year (2026) entries only expect(res.body).toContain('## Schema changes for 2026-') - // Check for change items expect(res.body).toContain('### The GraphQL schema includes these changes:') - // Should NOT contain entries from other years expect(res.body).not.toContain('## Schema changes for 2025-') - // Check for year navigation expect(res.body).toContain('2026') expect(res.body).toContain('2025') }) @@ -216,23 +203,19 @@ describe('GraphQL transformer', { timeout: 10000 }, () => { const res = await getCached('/en/graphql/overview/changelog/2025') expect(res.statusCode).toBe(200) - // Check for year-specific heading expect(res.body).toContain('# GraphQL changelog for 2025') - // Check for date-based changelog sections from 2025 expect(res.body).toContain('## Schema changes for 2025-') - // Should NOT contain entries from other years expect(res.body).not.toContain('## Schema changes for 2026-') expect(res.body).not.toContain('## Schema changes for 2024-') }) test('changelog removes HTML tags from changes', async () => { - // Use a year page that has the specific test data + // The 2025 fixture is the one whose change descriptions contain HTML. const res = await getCached('/en/graphql/overview/changelog/2025') expect(res.statusCode).toBe(200) - // Check that HTML tags are removed expect(res.body).toContain('Field suggestedReviewerActors was added') expect(res.body).not.toContain('') expect(res.body).not.toContain('') @@ -244,17 +227,13 @@ describe('GraphQL transformer', { timeout: 10000 }, () => { const res = await getCached('/en/graphql/overview/breaking-changes') expect(res.statusCode).toBe(200) - // Check for main heading expect(res.body).toContain('# Breaking changes') - // Check for intro expect(res.body).toContain('Learn about recent and upcoming breaking changes') - // Check for manual content expect(res.body).toContain('## About breaking changes') expect(res.body).toContain('Breaking:** Changes that will break existing queries') - // Check for date-based sections expect(res.body).toContain('## Changes scheduled for 2025-04-01') expect(res.body).toContain('## Changes scheduled for 2026-04-01') }) @@ -263,7 +242,6 @@ describe('GraphQL transformer', { timeout: 10000 }, () => { const res = await getCached('/en/graphql/overview/breaking-changes') expect(res.statusCode).toBe(200) - // Check for breaking criticality expect(res.body).toMatch(/\*\*Breaking\*\*\s+A change will be made to `\w+\.\w+`\./) expect(res.body).toMatch(/\*\*Description:\*\*.*will be removed/) expect(res.body).toMatch(/\*\*Reason:\*\*/) @@ -274,7 +252,6 @@ describe('GraphQL transformer', { timeout: 10000 }, () => { expect(res.statusCode).toBe(200) expect(res.body).toContain('scheduled for') - // Check that HTML tags are removed from descriptions expect(res.body).not.toContain('

') expect(res.body).not.toContain('

') expect(res.body).not.toContain('') @@ -291,7 +268,6 @@ describe('GraphQL transformer', { timeout: 10000 }, () => { const res = await getCached('/en/graphql/reference/repos') expect(res.statusCode).toBe(200) - // Make sure the raw AUTOTITLE tag is not present expect(res.body).not.toContain('[AUTOTITLE]') }) @@ -308,7 +284,6 @@ describe('GraphQL transformer', { timeout: 10000 }, () => { const res = await getCached('/en/graphql/overview/breaking-changes') expect(res.statusCode).toBe(200) - // Check that liquid variables in intro are rendered expect(res.body).toMatch(/(GitHub|HubGit) GraphQL API/) expect(res.body).not.toContain('{% data variables.product.prodname_dotcom %}') }) diff --git a/src/article-api/tests/journey-landing-transformer.ts b/src/article-api/tests/journey-landing-transformer.ts index 09e56d9af8fc..63a4110695ea 100644 --- a/src/article-api/tests/journey-landing-transformer.ts +++ b/src/article-api/tests/journey-landing-transformer.ts @@ -12,7 +12,6 @@ describe('journey landing transformer', () => { expect(res.statusCode).toBe(200) expect(res.headers['content-type']).toContain('text/markdown') - // Check for journey tracks (now under Links section with track title as h3) expect(res.body).toContain('## Links') expect(res.body).toContain('### First Track') expect(res.body).toContain('* [Hello World](/en/get-started/start-your-journey/hello-world)') diff --git a/src/article-api/tests/pageinfo.ts b/src/article-api/tests/pageinfo.ts index f17804619633..5739bc97c2f6 100644 --- a/src/article-api/tests/pageinfo.ts +++ b/src/article-api/tests/pageinfo.ts @@ -47,9 +47,7 @@ describe('pageinfo api', () => { 'Get started using HubGit to manage Git repositories and collaborate with others.', ) expect(meta.documentType).toBe('category') - // Canonical URLs should not have redirectedFrom expect(meta.redirectedFrom).toBeUndefined() - // Check that it can be cached at the CDN expect(res.headers['set-cookie']).toBeUndefined() expect(res.headers['cache-control']).toContain('public') expect(res.headers['cache-control']).toMatch(/max-age=[1-9]/) diff --git a/src/article-api/tests/pagelist.ts b/src/article-api/tests/pagelist.ts index 0cdb55cfcd04..684d64cde4c8 100644 --- a/src/article-api/tests/pagelist.ts +++ b/src/article-api/tests/pagelist.ts @@ -23,7 +23,6 @@ describe.each(allVersionKeys)('pagelist api for %s', async (versionKey) => { } }) - // queries the pagelist API for each version const res = await get(`/api/pagelist/en/${versionKey}`) test('is reachable, returns 200 OK', async () => { @@ -82,7 +81,6 @@ describe('Versions API', () => { const data = JSON.parse(res.body) - // Check top-level keys exist expect(data).toHaveProperty('versions') expect(data).toHaveProperty('ghesVersions') expect(data).toHaveProperty('ghesLatest') @@ -91,20 +89,16 @@ describe('Versions API', () => { expect(data).toHaveProperty('ghesDeprecated') expect(data).toHaveProperty('allVersions') - // Versions array should contain expected values expect(Array.isArray(data.versions)).toBe(true) expect(data.versions).toContain('free-pro-team@latest') expect(data.versions).toContain('enterprise-cloud@latest') - // GHES versions should be an array of version strings expect(Array.isArray(data.ghesVersions)).toBe(true) expect(data.ghesVersions.length).toBeGreaterThan(0) expect(data.ghesVersions[0]).toMatch(/^\d+\.\d+$/) - // ghesLatest should be a valid version string expect(data.ghesLatest).toMatch(/^\d+\.\d+$/) - // allVersions should be an object with version details expect(typeof data.allVersions).toBe('object') expect(data.allVersions['free-pro-team@latest']).toHaveProperty('version') expect(data.allVersions['free-pro-team@latest']).toHaveProperty('versionTitle') @@ -119,17 +113,14 @@ describe('Languages API', () => { const data = JSON.parse(res.body) - // Check top-level keys exist expect(data).toHaveProperty('languages') expect(data).toHaveProperty('allLanguages') - // Languages array should contain expected values expect(Array.isArray(data.languages)).toBe(true) expect(data.languages).toContain('en') expect(data.languages).toContain('ja') expect(data.languages).toContain('es') - // allLanguages should be an object with language details expect(typeof data.allLanguages).toBe('object') expect(data.allLanguages.en).toHaveProperty('name') expect(data.allLanguages.en).toHaveProperty('code') @@ -137,7 +128,7 @@ describe('Languages API', () => { expect(data.allLanguages.en).toHaveProperty('locale') expect(data.allLanguages.en.name).toBe('English') - // Should not contain redirectPatterns (not JSON serializable) + // redirectPatterns is not JSON serializable, so it must not be in the response. expect(data.allLanguages.ja).not.toHaveProperty('redirectPatterns') }) }) diff --git a/src/article-api/tests/resolve-path.ts b/src/article-api/tests/resolve-path.ts index 6156b8c1e689..3fc654f84fd8 100644 --- a/src/article-api/tests/resolve-path.ts +++ b/src/article-api/tests/resolve-path.ts @@ -2,7 +2,6 @@ import { describe, expect, test } from 'vitest' import { resolvePath } from '@/article-api/lib/resolve-path' import type { Context, Page } from '@/types' -// Helper to create a minimal mock page function createMockPage(relativePath: string): Page { return { relativePath, @@ -10,7 +9,6 @@ function createMockPage(relativePath: string): Page { } as unknown as Page } -// Helper to create a minimal context with pages function createContext(pages: Record): Context { return { pages } as unknown as Context } diff --git a/src/article-api/tests/rest-transformer.ts b/src/article-api/tests/rest-transformer.ts index cee19cf3a62c..4673e9a89e2a 100644 --- a/src/article-api/tests/rest-transformer.ts +++ b/src/article-api/tests/rest-transformer.ts @@ -24,13 +24,10 @@ describe('REST transformer', () => { expect(res.statusCode).toBe(200) expect(res.headers['content-type']).toContain('text/markdown') - // Check for the main heading expect(res.body).toContain('# GitHub Actions Artifacts') - // Check for intro (using fixture's prodname_actions which is 'HubGit Actions') expect(res.body).toContain('Use the REST API to interact with artifacts in HubGit Actions.') - // Check for manual content section heading expect(res.body).toContain('## About artifacts in HubGit Actions') }) @@ -38,13 +35,10 @@ describe('REST transformer', () => { const res = await get(makeURL('/en/rest/actions/artifacts')) expect(res.statusCode).toBe(200) - // Check for operation heading expect(res.body).toContain('## List artifacts for a repository') - // Check for HTTP method and endpoint expect(res.body).toContain('GET /repos/{owner}/{repo}/actions/artifacts') - // Check for operation description expect(res.body).toContain('Lists all artifacts for a repository.') }) @@ -52,13 +46,10 @@ describe('REST transformer', () => { const res = await get(makeURL('/en/rest/actions/artifacts')) expect(res.statusCode).toBe(200) - // Check for parameters heading expect(res.body).toContain('### Parameters') - // Check for headers section expect(res.body).toContain('#### Headers') - // Check for accept header expect(res.body).toContain('**`accept`** (string)') expect(res.body).toContain('Setting to `application/vnd.github+json` is recommended.') }) @@ -67,10 +58,8 @@ describe('REST transformer', () => { const res = await get(makeURL('/en/rest/actions/artifacts')) expect(res.statusCode).toBe(200) - // Check for path and query parameters section expect(res.body).toContain('#### Path and query parameters') - // Check for specific parameters expect(res.body).toContain('**`owner`** (string) (required)') expect(res.body).toContain('The account owner of the repository.') @@ -90,10 +79,8 @@ describe('REST transformer', () => { console.log(`[DEBUG] Test response: ${res.statusCode} in ${Date.now() - startTime}ms`) expect(res.statusCode).toBe(200) - // Check for status codes section expect(res.body).toContain('### HTTP response status codes') - // Check for specific status code expect(res.body).toContain('**200**') expect(res.body).toContain('OK') }) @@ -102,14 +89,11 @@ describe('REST transformer', () => { const res = await get(makeURL('/en/rest/actions/artifacts')) expect(res.statusCode).toBe(200) - // Check for code examples section expect(res.body).toContain('### Code examples') - // Check for request/response labels expect(res.body).toContain('**Request:**') expect(res.body).toContain('**Response schema (Status: 200):**') - // Check for curl code block expect(res.body).toContain('```curl') expect(res.body).toContain('curl -L \\') expect(res.body).toContain('-X GET \\') @@ -120,7 +104,6 @@ describe('REST transformer', () => { const res = await get(makeURL('/en/rest/actions/artifacts')) expect(res.statusCode).toBe(200) - // Check that auth note is at the top using [!NOTE] syntax expect(res.body).toContain('[!NOTE]') expect(res.body).toContain('Authorization: Bearer ') expect(res.body).toContain('application/vnd.github+json') @@ -130,7 +113,6 @@ describe('REST transformer', () => { const res = await get(makeURL('/en/rest/actions/artifacts')) expect(res.statusCode).toBe(200) - // Check for API version in the auth note (any valid date format) expect(res.body).toMatch(/X-GitHub-Api-Version: \d{4}-\d{2}-\d{2}/) }) @@ -138,7 +120,6 @@ describe('REST transformer', () => { const res = await get(makeURL('/en/rest/actions/artifacts', '2022-11-28')) expect(res.statusCode).toBe(200) - // Check for the specified API version in auth note expect(res.body).toContain('X-GitHub-Api-Version: 2022-11-28') }) @@ -150,7 +131,6 @@ describe('REST transformer', () => { expect(res.body).toContain('HubGit Actions') expect(res.body).not.toContain('{% data variables.product.prodname_actions %}') - // Check in both the intro and the manual content section expect(res.body).toMatch(/Use the REST API to interact with artifacts in HubGit Actions/) expect(res.body).toMatch(/About artifacts in HubGit Actions/) }) @@ -159,15 +139,12 @@ describe('REST transformer', () => { const res = await get(makeURL('/en/rest/actions/artifacts')) expect(res.statusCode).toBe(200) - // Check that AUTOTITLE has been resolved to actual link text // The link should have the actual page title, not "AUTOTITLE" expect(res.body).toContain('[Storing workflow data as artifacts]') expect(res.body).toContain('(/en/actions/using-workflows/storing-workflow-data-as-artifacts)') - // Make sure the raw AUTOTITLE tag is not present expect(res.body).not.toContain('[AUTOTITLE]') - // Verify the link appears in the manual content section expect(res.body).toMatch( /About artifacts in HubGit Actions[\s\S]*Storing workflow data as artifacts/, ) @@ -177,7 +154,6 @@ describe('REST transformer', () => { const res = await get(makeURL('/en/rest/actions/artifacts')) expect(res.statusCode).toBe(200) - // Check that markdown links are preserved expect(res.body).toMatch(/\[.*?\]\(\/en\/.*?\)/) }) @@ -185,14 +161,12 @@ describe('REST transformer', () => { const res = await get(makeURL('/en/rest/actions/artifacts')) expect(res.statusCode).toBe(200) - // Check for markdown-formatted schema expect(res.body).toContain('**Response schema (Status: 200):**') // Schema should be rendered as a markdown bullet list, not JSON expect(res.body).toContain('* `total_count`:') expect(res.body).toContain('* `artifacts`:') - // Should not contain raw JSON Schema keywords expect(res.body).not.toContain('"properties":') }) @@ -240,7 +214,6 @@ describe('REST transformer', () => { const res = await get(makeURL('/en/rest/actions/artifacts')) expect(res.statusCode).toBe(200) - // Should include the default API version in auth note (any valid date format) expect(res.body).toMatch(/X-GitHub-Api-Version: \d{4}-\d{2}-\d{2}/) }) @@ -248,7 +221,6 @@ describe('REST transformer', () => { const res = await get(makeURL('/en/rest/actions/artifacts')) expect(res.statusCode).toBe(200) - // Check for multiple operation headings expect(res.body).toContain('## List artifacts for a repository') expect(res.body).toContain('## Get an artifact') expect(res.body).toContain('## Delete an artifact') @@ -281,7 +253,7 @@ describe('REST transformer', () => { // because it looks for 'rest' in the path and gets the category/subcategory after it // e.g. /ja/rest/actions/artifacts should work the same as /en/rest/actions/artifacts - // Verify the operation content is present (in English, since REST data is not translated) + // REST data is not translated, so the operation content is English either way. expect(res.body).toContain('## List artifacts for a repository') expect(res.body).toContain('GET /repos/{owner}/{repo}/actions/artifacts') @@ -294,7 +266,6 @@ describe('REST transformer', () => { // One of them must be present expect(hasJapaneseTitle || hasEnglishTitle).toBe(true) - // Verify the appropriate content based on which language was served if (hasJapaneseTitle) { // If Japanese is loaded, expect Japanese intro text expect(res.body).toContain('ć‚¢ćƒ¼ćƒ†ć‚£ćƒ•ć‚”ć‚Æćƒˆ') diff --git a/src/article-api/tests/secret-scanning-transformer.ts b/src/article-api/tests/secret-scanning-transformer.ts index be236faf377a..c5daac87568e 100644 --- a/src/article-api/tests/secret-scanning-transformer.ts +++ b/src/article-api/tests/secret-scanning-transformer.ts @@ -13,20 +13,16 @@ describe('secret scanning article body api', () => { expect(res.statusCode).toBe(200) expect(res.headers['content-type']).toContain('text/markdown') - // Check for expected content expect(res.body).toContain('# Supported secret scanning patterns') expect(res.body).toContain('## Supported secrets') - // Verify HTML comments are stripped expect(res.body).not.toMatch(//) - // Verify HTML icon spans are not present (would be replaced with āœ“/āœ—) + // The icon spans are replaced with plain āœ“/āœ— characters. expect(res.body).not.toMatch(/]*aria-label="Supported"/) expect(res.body).not.toMatch(/]*aria-label="Unsupported"/) - // Verify no raw HTML span tags remain expect(res.body).not.toMatch(/]*>/) - // Verify table content is present with providers expect(res.body).toMatch(/|\s*Provider\s*|/) expect(res.body).toMatch(/\| (Adafruit|AWS|Alibaba|Amazon)/) diff --git a/src/article-api/tests/toc-transformer.ts b/src/article-api/tests/toc-transformer.ts index 4bef199a0879..6edd660fadd0 100644 --- a/src/article-api/tests/toc-transformer.ts +++ b/src/article-api/tests/toc-transformer.ts @@ -12,12 +12,11 @@ describe('toc transformer', () => { expect(res.statusCode).toBe(200) expect(res.headers['content-type']).toContain('text/markdown') - // Check for title and intro expect(res.body).toContain('# Category page of GitHub Actions') expect(res.body).toContain('Learn how to migrate your existing CI/CD workflows') - // Should have Links section with children (uses full title, not shortTitle) expect(res.body).toContain('## Links') + // Child links use the full title, not shortTitle. expect(res.body).toContain('[Subcategory page about Actions](/en/actions/category/subcategory)') }) @@ -27,10 +26,8 @@ describe('toc transformer', () => { expect(res.statusCode).toBe(200) expect(res.headers['content-type']).toContain('text/markdown') - // Check for title expect(res.body).toContain('# Subcategory page about Actions') - // Should have Links section with children expect(res.body).toContain('## Links') }) @@ -46,10 +43,8 @@ describe('toc transformer', () => { const res = await get(makeURL('/en/actions/category/subcategory')) expect(res.statusCode).toBe(200) - // Should NOT have href paths as titles expect(res.body).not.toContain('* [/en/actions/category/subcategory/') - // Should have proper article titles (or shortTitle) expect(res.body).toMatch(/\[.*\]\(\/en\/actions\/category\/subcategory\/.*\)/) }) }) diff --git a/src/article-api/tests/webhooks-transformer.ts b/src/article-api/tests/webhooks-transformer.ts index 8c611b285177..32f6db087ac0 100644 --- a/src/article-api/tests/webhooks-transformer.ts +++ b/src/article-api/tests/webhooks-transformer.ts @@ -22,10 +22,8 @@ describe('Webhooks transformer', () => { expect(res.statusCode).toBe(200) expect(res.headers['content-type']).toContain('text/markdown') - // Should have title expect(res.body).toContain('# Webhook events and payloads') - // Should have intro expect(res.body).toContain('Learn about when each webhook event occurs') }) @@ -33,7 +31,6 @@ describe('Webhooks transformer', () => { const res = await get(makeURL('/en/webhooks/webhook-events-and-payloads')) expect(res.statusCode).toBe(200) - // Check for webhook event headers (## webhook_name) expect(res.body).toMatch(/^## \w+/m) }) @@ -41,7 +38,6 @@ describe('Webhooks transformer', () => { const res = await get(makeURL('/en/webhooks/webhook-events-and-payloads')) expect(res.statusCode).toBe(200) - // Should list action types for webhooks with multiple actions expect(res.body).toContain('**Action type:**') }) @@ -49,7 +45,6 @@ describe('Webhooks transformer', () => { const res = await get(makeURL('/en/webhooks/webhook-events-and-payloads')) expect(res.statusCode).toBe(200) - // Should show availability as a heading expect(res.body).toContain('### Availability') }) @@ -57,7 +52,6 @@ describe('Webhooks transformer', () => { const res = await get(makeURL('/en/webhooks/webhook-events-and-payloads')) expect(res.statusCode).toBe(200) - // Check for some known manual content from the markdown file expect(res.body).toContain('About webhook events and payloads') }) @@ -73,7 +67,6 @@ describe('Webhooks transformer', () => { const res = await get(makeURL('/en/webhooks/webhook-events-and-payloads')) expect(res.statusCode).toBe(200) - // Check that data variables are rendered (not left as Liquid syntax) expect(res.body).not.toContain('{% data') expect(res.body).not.toContain('{{') }) @@ -90,10 +83,8 @@ describe('Webhooks transformer', () => { const res = await get(makeURL('/en/webhooks/webhook-events-and-payloads')) expect(res.statusCode).toBe(200) - // Should show payload object parameters section expect(res.body).toContain('### Webhook payload object') expect(res.body).toContain('#### Webhook payload object parameters') - // Should have a markdown table with parameter columns (may have extra spacing from formatting) expect(res.body).toMatch(/\|\s*Name\s*\|\s*Type\s*\|\s*Description\s*\|/) }) @@ -101,7 +92,6 @@ describe('Webhooks transformer', () => { const res = await get(makeURL('/en/webhooks/webhook-events-and-payloads')) expect(res.statusCode).toBe(200) - // Should include webhook descriptions (converted from HTML to plain text) // Using actual descriptions from real webhook data expect(res.body).toContain('A check run was completed') }) @@ -110,11 +100,9 @@ describe('Webhooks transformer', () => { const res = await get(makeURL('/en/webhooks/webhook-events-and-payloads')) expect(res.statusCode).toBe(200) - // Should show parameter names and types in tables expect(res.body).toContain('`action`') expect(res.body).toContain('`string`') expect(res.body).toContain('`object`') - // Should mark required parameters expect(res.body).toContain('**Required.**') }) @@ -132,7 +120,6 @@ describe('Webhooks transformer', () => { const res = await get(makeURL('/en/webhooks/webhook-events-and-payloads')) expect(res.statusCode).toBe(200) - // Should have a common parameters section at the top expect(res.body).toContain('## Common payload parameters') expect(res.body).toContain('Most webhook events include these standard parameters') From e3034ac8902ceff954c76509eb30176c2a46dc62 Mon Sep 17 00:00:00 2001 From: Kevin Heis Date: Tue, 15 Sep 2026 21:44:55 +0000 Subject: [PATCH 15/16] Trim excessive comments in the rest of src/content-render (#63278) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 17588eeb-788a-4f82-9d36-b5079fb36521 --- src/content-render/liquid/data.ts | 2 -- src/content-render/liquid/engine.ts | 3 -- src/content-render/liquid/ifversion.ts | 16 ++-------- .../liquid/indented-data-reference.ts | 2 -- src/content-render/liquid/octicon.ts | 6 ---- src/content-render/liquid/prompt.ts | 4 --- src/content-render/tests/annotate.ts | 8 ----- .../tests/copilot-code-blocks.ts | 30 ------------------- .../tests/link-error-line-numbers.ts | 8 ----- src/content-render/tests/liquid-tags.ts | 9 ------ src/content-render/tests/prompt-id.ts | 1 - src/content-render/tests/prompt.ts | 4 --- src/content-render/types.ts | 19 ------------ src/content-render/unified/annotate.ts | 9 ------ .../unified/collect-mini-toc.ts | 1 - src/content-render/unified/copilot-prompt.ts | 1 - src/content-render/unified/module-types.d.ts | 4 --- .../unified/rewrite-local-links.ts | 6 +--- .../unified/rewrite-table-captions.ts | 18 ++--------- src/content-render/unified/text-only.ts | 1 - .../unified/use-english-headings.ts | 4 --- 21 files changed, 7 insertions(+), 149 deletions(-) diff --git a/src/content-render/liquid/data.ts b/src/content-render/liquid/data.ts index bafe90d364a0..44c0151cd40b 100644 --- a/src/content-render/liquid/data.ts +++ b/src/content-render/liquid/data.ts @@ -91,7 +91,6 @@ function handleIndent(tagToken: TagToken, text: string): string { // keep the blockquote character on every successive line. const blockquoteRegexp = /^\n?([ \t]*>[ \t]?)/ function handleBlockquote(tagToken: TagToken, text: string): string { - // If the text isn't multiline, skip if (text.split('\n').length <= 1) return text // If the line with the liquid tag starts with a blockquote... @@ -100,7 +99,6 @@ function handleBlockquote(tagToken: TagToken, text: string): string { const inputLine = input.split('\n').find((line) => line.includes(content)) if (!inputLine || !blockquoteRegexp.test(inputLine)) return text - // Keep the character on successive lines const match = inputLine.match(blockquoteRegexp) if (!match) return text const start = match[0] diff --git a/src/content-render/liquid/engine.ts b/src/content-render/liquid/engine.ts index 2d10036d94f8..11418f9cfd9a 100644 --- a/src/content-render/liquid/engine.ts +++ b/src/content-render/liquid/engine.ts @@ -77,9 +77,6 @@ engine.registerFilter('render_liquid', function (this: FilterScope, input: unkno return engine.parseAndRender(input, this.context.environments) }) -/** - * Convert the input to a slug - */ engine.registerFilter('slugify', (input: string): string => { const slugger = new GithubSlugger() return slugger.slug(input) diff --git a/src/content-render/liquid/ifversion.ts b/src/content-render/liquid/ifversion.ts index adc1b16d5ccb..38c724517a18 100644 --- a/src/content-render/liquid/ifversion.ts +++ b/src/content-render/liquid/ifversion.ts @@ -95,21 +95,19 @@ export default class Ifversion extends Tag { for (const branch of this.branches) { let resolvedBranchCond = branch.cond - // Resolve "not" keywords in the conditional, if any. resolvedBranchCond = this.handleNots(resolvedBranchCond) // Resolve special operators in the conditional, if any. // This will replace syntax like `fpt or ghes < 3.0` with `fpt or true` or `fpt or false`. resolvedBranchCond = this.handleOperators(resolvedBranchCond) - // Resolve version names to boolean values for Markdown API context. - // This will replace syntax like `fpt or ghec` with `true or false` based on current version. - // Only apply this transformation in Markdown API context to avoid breaking existing functionality. + // Replace syntax like `fpt or ghec` with `true or false` based on the current + // version. Only done for the Markdown API, where the version names would + // otherwise be undefined. if ((ctx.environments as IfversionEnvironments).markdownRequested) { resolvedBranchCond = this.handleVersionNames(resolvedBranchCond) } - // Use Liquid's native function for the final evaluation. const cond = yield new Value(resolvedBranchCond, this.liquid).value(ctx, ctx.opts.lenientIf) if (isTruthy(cond, ctx)) { @@ -125,7 +123,6 @@ export default class Ifversion extends Tag { const condArray = resolvedBranchCond.split(' ') - // Find the first index in the array that contains "not". const notIndex = condArray.findIndex((el: string) => el === 'not') // E.g., ['not', 'fpt'] @@ -156,7 +153,6 @@ export default class Ifversion extends Tag { // If this conditional contains multiple parts using `or` or `and`, get only the conditional with operators. const condArray = resolvedBranchCond.split(' ') - // Find the first index in the array that contains an operator. const operatorIndex = condArray.findIndex((el: string) => supportedOperators.find((op: string) => el === op), ) @@ -164,7 +160,6 @@ export default class Ifversion extends Tag { // E.g., ['ghes', '<', '3.1'] const condParts = condArray.slice(operatorIndex - 1, operatorIndex + 2) - // Assign to vars. const [versionShortName, operator, releaseToEvaluate] = condParts // Make sure the operator is supported and the release number matches `\d\d?\.\d\d?` @@ -219,17 +214,12 @@ export default class Ifversion extends Tag { return resolvedBranchCond } - // Split the condition into tokens for processing const tokens = resolvedBranchCond.split(/\s+/) const processedTokens = tokens.map((token: string) => { - // Check if the token is a version short name (fpt, ghec, ghes, ghae) const versionShortNames = ['fpt', 'ghec', 'ghes', 'ghae'] if (versionShortNames.includes(token)) { - // Transform version names to boolean values for Markdown API - // This fixes the original issue where version names were undefined in API context return token === this.currentVersionObj!.shortName ? 'true' : 'false' } - // Return the token unchanged if it's not a version name return token }) diff --git a/src/content-render/liquid/indented-data-reference.ts b/src/content-render/liquid/indented-data-reference.ts index 63ad23e1c942..b5b7f589d73b 100644 --- a/src/content-render/liquid/indented-data-reference.ts +++ b/src/content-render/liquid/indented-data-reference.ts @@ -47,7 +47,6 @@ const IndentedDataReference = { assert(parseInt(numSpaces) || numSpaces === '0', '"spaces=NUMBER" must include a number') - // Get the referenced value from the context const text: string | undefined = getDataByLanguage( dataReference, scope.environments.currentLanguage, @@ -63,7 +62,6 @@ const IndentedDataReference = { return } - // add spaces to each line const renderedReferenceWithIndent: string = text.replace(/^/gm, ' '.repeat(parseInt(numSpaces))) return this.liquid.parseAndRender(renderedReferenceWithIndent, scope.environments) diff --git a/src/content-render/liquid/octicon.ts b/src/content-render/liquid/octicon.ts index ca5a40fa7379..aacb8927e791 100644 --- a/src/content-render/liquid/octicon.ts +++ b/src/content-render/liquid/octicon.ts @@ -25,7 +25,6 @@ const Octicon = { throw new TokenizationError(SyntaxHelp, tagToken) } - // Memoize the icon this.icon = match.groups.icon // Breaking change in octicons 12 // https://github.com/primer/octicons/releases/tag/v12.0.0 @@ -36,29 +35,24 @@ const Octicon = { this.options = {} - // Memoize any options passed if (match.groups.options) { let optionsMatch: RegExpExecArray | null - // Loop through each option matching the OptionsSyntax regex while ((optionsMatch = OptionsSyntax.exec(match.groups.options))) { // Pull out the key/value ([0] is the whole input) const [, key, value] = optionsMatch this.options[key] = value - // Alias label to aria-label if (key === 'label') this.options['aria-label'] = value } } }, async render(): Promise { - // Throw an error if the requested octicon does not exist. if (!Object.prototype.hasOwnProperty.call(octicons, this.icon)) { throw new Error(`Octicon ${this.icon} does not exist`) } - // Auto-generate aria-label if not provided // Replace non-alphanumeric characters with spaces and append " icon" if (!this.options['aria-label']) { const defaultLabel = `${this.icon.toLowerCase().replace(/[^a-z0-9]+/gi, ' ')} icon` diff --git a/src/content-render/liquid/prompt.ts b/src/content-render/liquid/prompt.ts index dbba2b28b3c4..1df9e1bcd28a 100644 --- a/src/content-render/liquid/prompt.ts +++ b/src/content-render/liquid/prompt.ts @@ -1,4 +1,3 @@ -// src/content-render/liquid/prompt.ts // Defines {% prompt %}…{% endprompt %} to wrap its content in and append the Copilot icon. import octicons from '@primer/octicons' @@ -15,7 +14,6 @@ interface LiquidTag { export const Prompt: LiquidTag = { type: 'block', - // Collect everything until {% endprompt %} parse(tagToken: TagToken, remainTokens: TopLevelToken[]): void { this.templates = [] const stream = this.liquid.parser.parseStream(remainTokens) @@ -28,12 +26,10 @@ export const Prompt: LiquidTag = { stream.start() }, - // Render the inner Markdown, wrap in , then append the SVG *render(scope: unknown): Generator { const content = yield this.liquid.renderer.renderTemplates(this.templates, scope) const contentString = String(content) - // build a URL with the prompt text encoded as query parameter const promptParam: string = encodeURIComponent(contentString) const href: string = `https://github.com/copilot?prompt=${promptParam}` // Use murmur hash for deterministic ID (avoids hydration mismatch) diff --git a/src/content-render/tests/annotate.ts b/src/content-render/tests/annotate.ts index 20083164d3bf..c47a78d6fb76 100644 --- a/src/content-render/tests/annotate.ts +++ b/src/content-render/tests/annotate.ts @@ -22,33 +22,27 @@ describe('annotate', () => { const res = await renderContent(example) const $ = load(res) - // Check that the annotation structure is rendered correctly const annotation = $('.annotate') expect(annotation.length).toBe(1) expect(annotation.hasClass('beside')).toBe(true) - // Check annotation header exists const header = $('.annotate-header') expect(header.length).toBe(1) - // Check both beside and inline modes are rendered const beside = $('.annotate-beside') const inline = $('.annotate-inline') expect(beside.length).toBe(1) expect(inline.length).toBe(1) - // Check that we have the correct number of annotation rows const rows = $('.annotate-row') expect(rows.length).toBe(2) - // Check that each row has both code and note sections rows.each((i, row) => { const $row = $(row) expect($row.find('.annotate-code').length).toBe(1) expect($row.find('.annotate-note').length).toBe(1) }) - // Check specific content of the annotations const notes = $('.annotate-note p') const noteTexts = notes.map((i, el) => $(el).text()).get() expect(noteTexts).toEqual([ @@ -56,7 +50,6 @@ describe('annotate', () => { 'Add the pull_request event, so that the workflow runs automatically\nevery time a pull request is created.', ]) - // Check code content const codes = $('.annotate-code pre') const codeTexts = codes.map((i, el) => $(el).text()).get() expect(codeTexts).toEqual([ @@ -157,7 +150,6 @@ on: [push] const rows = $('.annotate-row') const notes = $('.annotate-note', rows) - // Check that AUTOTITLE links were resolved to actual titles const firstNote = notes.eq(0).html() const secondNote = notes.eq(1).html() diff --git a/src/content-render/tests/copilot-code-blocks.ts b/src/content-render/tests/copilot-code-blocks.ts index 51209f95663e..2dfe76d6b4c2 100644 --- a/src/content-render/tests/copilot-code-blocks.ts +++ b/src/content-render/tests/copilot-code-blocks.ts @@ -18,11 +18,8 @@ describe('code-header plugin', () => { const html = await renderContent(markdown) - // Should keep copilot as the language (not convert to text without copy meta) expect(html).toContain('language-copilot') - // Should NOT wrap in code-example div since no copy meta expect(html).not.toContain('code-example') - // Should NOT have header since no copy meta expect(html).not.toContain(' { const html = await renderContent(markdown) - // Should be wrapped in code-example div expect(html).toContain('code-example') - // Should have header with copy button expect(html).toContain(' { const html = await renderContent(markdown) - // Should be wrapped in code-example div expect(html).toContain('code-example') - // Should have header expect(html).toContain(' { const html = await renderContent(markdown) - // Should be wrapped in code-example div expect(html).toContain('code-example') - // Should have header with copy button expect(html).toContain(' { let mockContext: Context beforeEach(async () => { - // Set up file system mocking fs = await import('fs') originalReadFileSync = fs.default.readFileSync originalExistsSync = fs.default.existsSync fs.default.existsSync = () => true - // Set up basic mock context mockContext = { currentLanguage: 'en', currentVersion: 'free-pro-team@latest', @@ -30,13 +28,11 @@ describe('link error line numbers', () => { }) afterEach(() => { - // Restore original functions fs.default.readFileSync = originalReadFileSync fs.default.existsSync = originalExistsSync }) test('reports correct line numbers for broken AUTOTITLE links', async () => { - // Test content with frontmatter followed by content with a broken link const template = `--- title: Test Page version: 1.0 @@ -72,7 +68,6 @@ More content here.` fullPath: '/fake/test-file-2.md', } as unknown as Context['page'] - // Test with more extensive frontmatter const template = `--- title: Another Test Page description: This is a test @@ -109,7 +104,6 @@ Content with a [AUTOTITLE](/another/nonexistent/page) link.` fullPath: '/fake/no-frontmatter.md', } as unknown as Context['page'] - // Test content without frontmatter const template = `# Simple Title This is content without frontmatter. @@ -148,7 +142,6 @@ title: Message Test } catch (error) { expect(error).toBeInstanceOf(TitleFromAutotitleError) - // Check that the new error message format is used expect((error as TitleFromAutotitleError).message).toContain( 'could not be resolved in one or more versions', ) @@ -157,7 +150,6 @@ title: Message Test ) expect((error as TitleFromAutotitleError).message).toContain('/test/broken/link') - // Check that the old error message format is NOT used expect((error as TitleFromAutotitleError).message).not.toContain('Unable to find Page by') expect((error as TitleFromAutotitleError).message).not.toContain('To fix it, look at') } diff --git a/src/content-render/tests/liquid-tags.ts b/src/content-render/tests/liquid-tags.ts index 0209be92c090..db28d494733b 100644 --- a/src/content-render/tests/liquid-tags.ts +++ b/src/content-render/tests/liquid-tags.ts @@ -10,16 +10,13 @@ describe('liquid-tags script integration tests', () => { vi.setConfig({ testTimeout: 60 * 1000 }) beforeEach(async () => { - // Create test directory await fs.mkdir(testContentDir, { recursive: true }) }) afterEach(async () => { - // Clean up test files await fs.rm(testContentDir, { recursive: true, force: true }) }) - // Helper function to run script commands async function runScript(args: string): Promise<{ output: string; exitCode: number }> { let output = '' let exitCode = 0 @@ -41,7 +38,6 @@ describe('liquid-tags script integration tests', () => { } test('expand command should complete successfully with basic content', async () => { - // Create a test file with liquid reference const testFile = path.join(testContentDir, 'basic-test.md') const testContent = `--- title: Test @@ -54,11 +50,9 @@ This uses {% data variables.product.prodname_dotcom %} in content. const { output, exitCode } = await runScript(`expand --paths "${testFile}"`) - // Should complete without error expect(exitCode, `Script failed with output: ${output}`).toBe(0) expect(output.length).toBeGreaterThan(0) - // Check that the file was modified const expandedContent = await fs.readFile(testFile, 'utf8') expect(expandedContent).not.toBe(testContent) expect(expandedContent).toContain('GitHub') // Should expand to actual fixture value @@ -75,16 +69,13 @@ This uses {% data variables.product.prodname_dotcom %} in content. await fs.writeFile(testFile, originalContent) - // First expand await runScript(`expand --paths "${testFile}"`) - // Then restore const { output, exitCode } = await runScript(`restore --paths "${testFile}"`) expect(exitCode, `Restore script failed with output: ${output}`).toBe(0) expect(output.length).toBeGreaterThan(0) - // Should be back to original liquid tags const restoredContent = await fs.readFile(testFile, 'utf8') expect(restoredContent).toContain('{% data variables.product.prodname_dotcom %}') expect(restoredContent).not.toContain('GitHub') diff --git a/src/content-render/tests/prompt-id.ts b/src/content-render/tests/prompt-id.ts index ce7441e8832b..71e046b0fd48 100644 --- a/src/content-render/tests/prompt-id.ts +++ b/src/content-render/tests/prompt-id.ts @@ -55,7 +55,6 @@ describe('generatePromptId', () => { expect(typeof id).toBe('string') expect(id.length).toBeGreaterThan(0) - // Should be different from just the prompt text alone expect(id).not.toBe(generatePromptId(promptText)) }) diff --git a/src/content-render/tests/prompt.ts b/src/content-render/tests/prompt.ts index 65de51fea1c3..e8e250e166e1 100644 --- a/src/content-render/tests/prompt.ts +++ b/src/content-render/tests/prompt.ts @@ -6,18 +6,14 @@ describe('prompt tag', () => { const input: string = 'Here is your prompt: {% prompt %}example prompt text{% endprompt %}.' const output: string = await renderContent(input) - // Should have code element with ID expect(output).toContain('