diff --git a/content/code-security/how-tos/secure-at-scale/configure-enterprise-security/establish-complete-coverage/create-custom-configuration.md b/content/code-security/how-tos/secure-at-scale/configure-enterprise-security/establish-complete-coverage/create-custom-configuration.md index 989de0ca97c7..15cc256d2e3e 100644 --- a/content/code-security/how-tos/secure-at-scale/configure-enterprise-security/establish-complete-coverage/create-custom-configuration.md +++ b/content/code-security/how-tos/secure-at-scale/configure-enterprise-security/establish-complete-coverage/create-custom-configuration.md @@ -70,7 +70,7 @@ When creating a security configuration, keep in mind that: 1. Optionally, in the "Policy" section, you can use additional options to control how the configuration is applied: * **Use as default for newly created repositories**. Select the **None** {% octicon "triangle-down" aria-hidden="true" aria-label="triangle-down" %} dropdown menu, then click **Public**, **Private and internal**, or **All repositories**. {% data reusables.security-configurations.default-configuration-exception-repo-transfers %} - * **Enforce configuration**. Block repository owners from changing features that are enabled or disabled by the configuration (features that are not set aren't enforced). Select **Enforce** from the dropdown menu. + * **Enforce configuration**. {% ifversion security-configuration-enterprise-organization-enforcement %}{% data reusables.permissions.security-configuration-enterprise-enable %} can block repository owners only, or both repository and organization owners, from changing features that are enabled or disabled by the configuration. Features that are not set aren't enforced. Select **Don't enforce**, **Enforce for repository owners**, or **Enforce for repository and organization owners** from the dropdown menu.{% else %}Block repository owners from changing features that are enabled or disabled by the configuration (features that are not set aren't enforced). Select **Enforce** from the dropdown menu.{% endif %} {% data reusables.code-scanning.security-configuration-enforcement-edge-cases %} @@ -111,8 +111,7 @@ When creating a security configuration, keep in mind that: 1. Optionally, in the "Policy" section, you can use additional options to control how the configuration is applied: * **Use as default for newly created repositories**. Select the **None** {% octicon "triangle-down" aria-hidden="true" aria-label="triangle-down" %} dropdown menu, then click **Public**, **Private and internal**, or **All repositories**. {% data reusables.security-configurations.default-configuration-exception-repo-transfers %} - * **Enforce configuration**. Block repository owners from changing features that are enabled or disabled by the configuration (features that are not set aren't enforced). Select **Enforce** from the dropdown menu. - + * **Enforce configuration**. {% ifversion security-configuration-enterprise-organization-enforcement %}{% data reusables.permissions.security-configuration-enterprise-enable %} can block repository owners only, or both repository and organization owners, from changing features that are enabled or disabled by the configuration. Features that are not set aren't enforced. Select **Don't enforce**, **Enforce for repository owners**, or **Enforce for repository and organization owners** from the dropdown menu.{% else %}Block repository owners from changing features that are enabled or disabled by the configuration (features that are not set aren't enforced). Select **Enforce** from the dropdown menu.{% endif %} {% data reusables.code-scanning.security-configuration-enforcement-edge-cases %} {% data reusables.code-scanning.save-custom-configuration %} diff --git a/content/code-security/how-tos/secure-at-scale/configure-enterprise-security/manage-your-coverage/edit-custom-configuration.md b/content/code-security/how-tos/secure-at-scale/configure-enterprise-security/manage-your-coverage/edit-custom-configuration.md index d961ce8a9a75..eedd15db54b6 100644 --- a/content/code-security/how-tos/secure-at-scale/configure-enterprise-security/manage-your-coverage/edit-custom-configuration.md +++ b/content/code-security/how-tos/secure-at-scale/configure-enterprise-security/manage-your-coverage/edit-custom-configuration.md @@ -25,7 +25,7 @@ After creating and applying a {% data variables.product.prodname_custom_security 1. In the "Configurations" section, click the name of the {% data variables.product.prodname_custom_security_configuration %} you want to edit. 1. Edit the name and description of your {% data variables.product.prodname_custom_security_configuration %} as desired. 1. Edit the enablement settings of your {% data variables.product.prodname_custom_security_configuration %} as desired. -1. In the "Policy" section, you can modify the configuration's enforcement status. Enforcing a configuration will block repository owners from changing features that are enabled or disabled by the configuration, but features that are not set aren't enforced. Next to "Enforce configuration", select **Enforce** or **Don't enforce** from the dropdown menu. +1. In the "Policy" section, you can modify the configuration's enforcement status. {% ifversion security-configuration-enterprise-organization-enforcement %}{% data reusables.permissions.security-configuration-enterprise-enable %} can block repository owners only, or both repository and organization owners, from changing features that are enabled or disabled by the configuration. Features that are not set aren't enforced. Select **Don't enforce**, **Enforce for repository owners**, or **Enforce for repository and organization owners** from the dropdown menu.{% else %}Enforcing a configuration will block repository owners from changing features that are enabled or disabled by the configuration, but features that are not set aren't enforced. Next to "Enforce configuration", select **Enforce** or **Don't enforce** from the dropdown menu.{% endif %} {% data reusables.code-scanning.security-configuration-enforcement-edge-cases %} diff --git a/content/code-security/reference/security-at-scale/configuration-enforcement.md b/content/code-security/reference/security-at-scale/configuration-enforcement.md index 19b15400c191..4eddf3bcfd15 100644 --- a/content/code-security/reference/security-at-scale/configuration-enforcement.md +++ b/content/code-security/reference/security-at-scale/configuration-enforcement.md @@ -13,8 +13,18 @@ redirect_from: - /code-security/reference/security-at-scale/security-configuration-enforcement --- +{% ifversion security-configuration-enterprise-organization-enforcement %} + +{% data variables.product.prodname_security_configurations_caps %} can be enforced at the organization{% ifversion security-configuration-enterprise-level %} or enterprise{% endif %} level to prevent owners from changing the enablement status of configured security features. +{% data reusables.permissions.security-configuration-enterprise-enable %} +* At the **organization** level, enforcement means repository owners cannot change the enablement status of features that are enabled or disabled by the configuration. +* At the **enterprise** level, you can enforce for repository owners only, or for both repository and organization owners. When enforcement applies to both, neither repository owners nor organization owners can change the enablement status of features that are enabled or disabled by the configuration. +{% else %} + {% data variables.product.prodname_security_configurations_caps %} can be enforced, meaning repository owners cannot change the enablement status of features that are enabled or disabled by the configuration. +{% endif %} + ## Situations that break enforcement Some situations can break the enforcement of {% data variables.product.prodname_security_configurations %}. For example, the enablement of {% data variables.product.prodname_code_scanning %} will not apply to a repository if: diff --git a/content/copilot/concepts/enterprise/agent-management.md b/content/copilot/concepts/enterprise/agent-management.md index b6cdf46976d2..56168e98d551 100644 --- a/content/copilot/concepts/enterprise/agent-management.md +++ b/content/copilot/concepts/enterprise/agent-management.md @@ -9,6 +9,8 @@ category: - Learn about Copilot redirect_from: - /copilot/concepts/agents/enterprise-management +docsTeamMetrics: + - ai-governance --- ## Overview diff --git a/content/copilot/concepts/enterprise/learning-about-new-features-and-models.md b/content/copilot/concepts/enterprise/learning-about-new-features-and-models.md index 5764c1f24522..74245e782348 100644 --- a/content/copilot/concepts/enterprise/learning-about-new-features-and-models.md +++ b/content/copilot/concepts/enterprise/learning-about-new-features-and-models.md @@ -10,6 +10,8 @@ category: redirect_from: - /copilot/concepts/preparing-for-new-features-and-models - /copilot/concepts/learning-about-new-features-and-models +docsTeamMetrics: + - ai-governance --- {% data variables.product.prodname_copilot %} is evolving rapidly, with new features and models released regularly. If you're an enterprise administrator, staying informed helps you make confident decisions about which capabilities to enable, when to adopt them, and how to manage risk across your organizations. diff --git a/content/copilot/concepts/enterprise/policies.md b/content/copilot/concepts/enterprise/policies.md index 94c19a612d98..4918686dbcaf 100644 --- a/content/copilot/concepts/enterprise/policies.md +++ b/content/copilot/concepts/enterprise/policies.md @@ -10,6 +10,8 @@ redirect_from: - /copilot/concepts/policies category: - Manage Copilot for a team +docsTeamMetrics: + - ai-governance --- ## How do policies work? diff --git a/content/copilot/get-started/enterprise-ai-governance.md b/content/copilot/get-started/enterprise-ai-governance.md index 0436c2cde676..e112b6bfd835 100644 --- a/content/copilot/get-started/enterprise-ai-governance.md +++ b/content/copilot/get-started/enterprise-ai-governance.md @@ -50,4 +50,6 @@ journeyTracks: guides: - href: '/copilot/concepts/enterprise/learning-about-new-features-and-models' - href: '/copilot/tutorials/roll-out-at-scale/govern-at-scale/pilot-a-feature-or-model' +docsTeamMetrics: + - ai-governance --- diff --git a/content/copilot/how-tos/administer-copilot/manage-for-enterprise/manage-agents/enable-copilot-cloud-agent.md b/content/copilot/how-tos/administer-copilot/manage-for-enterprise/manage-agents/enable-copilot-cloud-agent.md index af4b977c6b48..ac0556986519 100644 --- a/content/copilot/how-tos/administer-copilot/manage-for-enterprise/manage-agents/enable-copilot-cloud-agent.md +++ b/content/copilot/how-tos/administer-copilot/manage-for-enterprise/manage-agents/enable-copilot-cloud-agent.md @@ -20,6 +20,8 @@ redirect_from: contentType: how-tos category: - Manage Copilot for a team +docsTeamMetrics: + - ai-governance --- ## Prerequisites diff --git a/content/copilot/how-tos/administer-copilot/manage-for-enterprise/manage-agents/enable-copilot-code-review.md b/content/copilot/how-tos/administer-copilot/manage-for-enterprise/manage-agents/enable-copilot-code-review.md index eb80fe054285..cd7a3270d38f 100644 --- a/content/copilot/how-tos/administer-copilot/manage-for-enterprise/manage-agents/enable-copilot-code-review.md +++ b/content/copilot/how-tos/administer-copilot/manage-for-enterprise/manage-agents/enable-copilot-code-review.md @@ -12,6 +12,8 @@ redirect_from: contentType: how-tos category: - Manage Copilot for a team +docsTeamMetrics: + - ai-governance --- {% data variables.product.prodname_copilot %} can review pull requests in your enterprise. This can save time for human reviewers by detecting bugs or vulnerabilities and enforcing consistent coding standards. diff --git a/content/copilot/how-tos/administer-copilot/manage-for-enterprise/manage-agents/monitor-agentic-activity.md b/content/copilot/how-tos/administer-copilot/manage-for-enterprise/manage-agents/monitor-agentic-activity.md index 6dc84c880741..5278e978d60b 100644 --- a/content/copilot/how-tos/administer-copilot/manage-for-enterprise/manage-agents/monitor-agentic-activity.md +++ b/content/copilot/how-tos/administer-copilot/manage-for-enterprise/manage-agents/monitor-agentic-activity.md @@ -8,6 +8,8 @@ shortTitle: Monitor agentic activity contentType: how-tos category: - Manage Copilot for a team +docsTeamMetrics: + - ai-governance --- ## Viewing active and recent agentic sessions in your enterprise diff --git a/content/copilot/how-tos/administer-copilot/manage-for-enterprise/review-audit-logs.md b/content/copilot/how-tos/administer-copilot/manage-for-enterprise/review-audit-logs.md index b9769e99bf6a..3b3f9e6fa23c 100644 --- a/content/copilot/how-tos/administer-copilot/manage-for-enterprise/review-audit-logs.md +++ b/content/copilot/how-tos/administer-copilot/manage-for-enterprise/review-audit-logs.md @@ -23,6 +23,8 @@ versions: contentType: how-tos category: - Manage Copilot for a team +docsTeamMetrics: + - ai-governance --- You can use the audit log to review actions taken in your enterprise. The audit log includes a record of: diff --git a/content/copilot/how-tos/administer-copilot/manage-mcp-usage/configure-enterprise-allowlist.md b/content/copilot/how-tos/administer-copilot/manage-mcp-usage/configure-enterprise-allowlist.md index ec9868c37ab3..146c11e81d72 100644 --- a/content/copilot/how-tos/administer-copilot/manage-mcp-usage/configure-enterprise-allowlist.md +++ b/content/copilot/how-tos/administer-copilot/manage-mcp-usage/configure-enterprise-allowlist.md @@ -10,6 +10,8 @@ contentType: how-tos category: - Configure Copilot - Manage Copilot for a team +docsTeamMetrics: + - ai-governance --- ## About allowlists diff --git a/content/copilot/how-tos/copilot-cli/administer-copilot-cli-for-your-enterprise.md b/content/copilot/how-tos/copilot-cli/administer-copilot-cli-for-your-enterprise.md index f24f57769888..c81695274eb0 100644 --- a/content/copilot/how-tos/copilot-cli/administer-copilot-cli-for-your-enterprise.md +++ b/content/copilot/how-tos/copilot-cli/administer-copilot-cli-for-your-enterprise.md @@ -12,6 +12,7 @@ category: - Administer Copilot CLI # Copilot CLI bespoke landing page docsTeamMetrics: - copilot-cli + - ai-governance --- {% data variables.copilot.copilot_cli_short %} brings agentic capabilities to developers' command line. When {% data variables.copilot.copilot_cli_short %} is enabled, developers can use it to ask {% data variables.product.prodname_copilot_short %} to work on tasks locally or delegate work to {% data variables.copilot.copilot_cloud_agent %}. diff --git a/content/copilot/how-tos/copilot-on-github/set-up-copilot/enable-copilot/set-up-for-enterprise.md b/content/copilot/how-tos/copilot-on-github/set-up-copilot/enable-copilot/set-up-for-enterprise.md index 47287e83f030..147f10a3cfcd 100644 --- a/content/copilot/how-tos/copilot-on-github/set-up-copilot/enable-copilot/set-up-for-enterprise.md +++ b/content/copilot/how-tos/copilot-on-github/set-up-copilot/enable-copilot/set-up-for-enterprise.md @@ -17,6 +17,8 @@ redirect_from: contentType: how-tos category: - Configure Copilot +docsTeamMetrics: + - ai-governance --- ## Enable {% data variables.product.prodname_copilot %} diff --git a/content/copilot/tutorials/cloud-agent/build-guardrails.md b/content/copilot/tutorials/cloud-agent/build-guardrails.md index 00698ffbf2ac..43b0bc337b57 100644 --- a/content/copilot/tutorials/cloud-agent/build-guardrails.md +++ b/content/copilot/tutorials/cloud-agent/build-guardrails.md @@ -8,6 +8,8 @@ contentType: tutorials category: - Roll Copilot out at scale - Manage Copilot for a team +docsTeamMetrics: + - ai-governance --- Before you enable {% data variables.copilot.copilot_cloud_agent %}, it is good practice to set up your enterprise so you can be confident {% data variables.product.prodname_copilot_short %} will operate within secure, predictable guardrails. diff --git a/content/copilot/tutorials/cloud-agent/give-access-to-resources.md b/content/copilot/tutorials/cloud-agent/give-access-to-resources.md index 0ceb67d89c6a..77ebffbfe526 100644 --- a/content/copilot/tutorials/cloud-agent/give-access-to-resources.md +++ b/content/copilot/tutorials/cloud-agent/give-access-to-resources.md @@ -8,6 +8,8 @@ contentType: tutorials category: - Manage Copilot for a team - Roll Copilot out at scale +docsTeamMetrics: + - ai-governance --- {% data variables.copilot.copilot_cloud_agent %} can connect to MCP servers, use private packages, and access external services, but only if your organization's repositories are configured to allow it. diff --git a/content/copilot/tutorials/copilot-cli-hooks.md b/content/copilot/tutorials/copilot-cli-hooks.md index 6e537908575c..02f8d787c7fc 100644 --- a/content/copilot/tutorials/copilot-cli-hooks.md +++ b/content/copilot/tutorials/copilot-cli-hooks.md @@ -12,6 +12,7 @@ category: allowTitleToDifferFromFilename: true docsTeamMetrics: - copilot-cli + - ai-governance --- This tutorial is for DevOps engineers, platform teams, and engineering leaders who support developers using {% data variables.copilot.copilot_cli_short %}. diff --git a/content/copilot/tutorials/roll-out-at-scale/assign-licenses/choose-enterprise-plan.md b/content/copilot/tutorials/roll-out-at-scale/assign-licenses/choose-enterprise-plan.md index b017f03df758..5133be553827 100644 --- a/content/copilot/tutorials/roll-out-at-scale/assign-licenses/choose-enterprise-plan.md +++ b/content/copilot/tutorials/roll-out-at-scale/assign-licenses/choose-enterprise-plan.md @@ -13,6 +13,8 @@ redirect_from: contentType: tutorials category: - Manage Copilot for a team +docsTeamMetrics: + - ai-governance --- ## Introduction diff --git a/content/copilot/tutorials/roll-out-at-scale/enable-developers/integrate-ai-agents.md b/content/copilot/tutorials/roll-out-at-scale/enable-developers/integrate-ai-agents.md index 90501978beec..3e2e345973d9 100644 --- a/content/copilot/tutorials/roll-out-at-scale/enable-developers/integrate-ai-agents.md +++ b/content/copilot/tutorials/roll-out-at-scale/enable-developers/integrate-ai-agents.md @@ -14,6 +14,8 @@ contentType: tutorials category: - Roll Copilot out at scale - Manage Copilot for a team +docsTeamMetrics: + - ai-governance --- ## About AI agents on {% data variables.product.github %} diff --git a/content/copilot/tutorials/roll-out-at-scale/govern-at-scale/govern-for-adoption.md b/content/copilot/tutorials/roll-out-at-scale/govern-at-scale/govern-for-adoption.md index 694929e9995e..303d0d0b08f1 100644 --- a/content/copilot/tutorials/roll-out-at-scale/govern-at-scale/govern-for-adoption.md +++ b/content/copilot/tutorials/roll-out-at-scale/govern-at-scale/govern-for-adoption.md @@ -11,6 +11,8 @@ contentType: tutorials allowTitleToDifferFromFilename: true redirect_from: - /copilot/tutorials/roll-out-at-scale/govern-for-adoption +docsTeamMetrics: + - ai-governance --- Getting the most from {% data variables.product.prodname_copilot %} means finding the right balance between governance and developer access. Too restrictive, and developers can't use the features that make them productive. Too permissive, and you may not meet your compliance requirements. diff --git a/content/copilot/tutorials/roll-out-at-scale/govern-at-scale/maintain-codebase-standards.md b/content/copilot/tutorials/roll-out-at-scale/govern-at-scale/maintain-codebase-standards.md index 19bef4034d35..a754bbc074dd 100644 --- a/content/copilot/tutorials/roll-out-at-scale/govern-at-scale/maintain-codebase-standards.md +++ b/content/copilot/tutorials/roll-out-at-scale/govern-at-scale/maintain-codebase-standards.md @@ -11,6 +11,8 @@ category: contentType: tutorials redirect_from: - /copilot/tutorials/roll-out-at-scale/maintain-codebase-standards +docsTeamMetrics: + - ai-governance --- Most enterprises are aware of the productivity benefits that AI coding tools can bring. However, many worry that improper usage in their company, such as malicious prompts or developers accepting AI suggestions without review, will lead to their codebase's standards being compromised. diff --git a/content/copilot/tutorials/roll-out-at-scale/govern-at-scale/pilot-a-feature-or-model.md b/content/copilot/tutorials/roll-out-at-scale/govern-at-scale/pilot-a-feature-or-model.md index 36a15d5f8e9c..4eb33f9f13da 100644 --- a/content/copilot/tutorials/roll-out-at-scale/govern-at-scale/pilot-a-feature-or-model.md +++ b/content/copilot/tutorials/roll-out-at-scale/govern-at-scale/pilot-a-feature-or-model.md @@ -9,6 +9,8 @@ category: - Roll Copilot out at scale contentType: tutorials allowTitleToDifferFromFilename: true +docsTeamMetrics: + - ai-governance --- This tutorial walks you through running a pilot end to end: setting a budget, enabling the feature for a contained group, monitoring results, and using the evidence to make and report a go or no-go decision. diff --git a/content/copilot/tutorials/roll-out-at-scale/govern-at-scale/resources-for-approval.md b/content/copilot/tutorials/roll-out-at-scale/govern-at-scale/resources-for-approval.md index 2f466a7cf270..6d1011097447 100644 --- a/content/copilot/tutorials/roll-out-at-scale/govern-at-scale/resources-for-approval.md +++ b/content/copilot/tutorials/roll-out-at-scale/govern-at-scale/resources-for-approval.md @@ -9,6 +9,8 @@ category: - Manage Copilot for a team redirect_from: - /copilot/get-started/resources-for-approval +docsTeamMetrics: + - ai-governance --- Before you can roll out a tool like {% data variables.product.prodname_copilot %} in your company, you will likely need signoff from legal, compliance, and cybersecurity teams. diff --git a/data/features/security-configuration-enterprise-organization-enforcement.yml b/data/features/security-configuration-enterprise-organization-enforcement.yml new file mode 100644 index 000000000000..ebd86bd49801 --- /dev/null +++ b/data/features/security-configuration-enterprise-organization-enforcement.yml @@ -0,0 +1,5 @@ +# Reference: github/releases#9912 +# Enterprise security configuration enforcement for organization owners +versions: + ghec: '*' + ghes: '>=3.24' diff --git a/src/article-api/transformers/article-transformer.ts b/src/article-api/transformers/article-transformer.ts index bbe81684dee8..94a61e52eab4 100644 --- a/src/article-api/transformers/article-transformer.ts +++ b/src/article-api/transformers/article-transformer.ts @@ -2,14 +2,11 @@ import type { Context, Page } from '@/types' import type { PageTransformer } from './types' /** - * Transformer for regular articles. - * - * This is a catch-all transformer registered last in the registry. - * It renders the page body as markdown and prepends the title and intro. + * Catch-all transformer, registered last. Renders the page body as markdown + * and prepends the title and intro. */ export class ArticleTransformer implements PageTransformer { canTransform(page: Page): boolean { - // Catch-all: handles any page not matched by a more specific transformer. return page != null } diff --git a/src/article-api/transformers/audit-logs-transformer.ts b/src/article-api/transformers/audit-logs-transformer.ts index ad20d022db3d..fd3ff4c6aa7d 100644 --- a/src/article-api/transformers/audit-logs-transformer.ts +++ b/src/article-api/transformers/audit-logs-transformer.ts @@ -7,8 +7,7 @@ import { loadTemplate } from '@/article-api/lib/load-template' import matter from '@gr2m/gray-matter' /** - * Transformer for Audit Logs pages - * Converts audit log events and their data into markdown format using a Liquid template + * Converts audit log events and their data into markdown using a Liquid template. */ export class AuditLogsTransformer implements PageTransformer { templateName = 'audit-logs-page.template.md' @@ -22,7 +21,6 @@ export class AuditLogsTransformer implements PageTransformer { const { getCategorizedAuditLogEvents, getCategoryNotes, resolveReferenceLinksToMarkdown } = await import('@/audit-logs/lib/index') - // Extract version from context const currentVersion = context.currentVersion! let pageType = '' @@ -36,11 +34,9 @@ export class AuditLogsTransformer implements PageTransformer { throw new Error(`Unknown audit log page type for path: ${pathname}`) } - // Get the audit log events data const categorizedEvents = getCategorizedAuditLogEvents(pageType, currentVersion) const categoryNotes = getCategoryNotes() - // Prepare manual content let manualContent = '' if (page.markdown) { const markerIndex = page.markdown.indexOf( @@ -63,7 +59,6 @@ export class AuditLogsTransformer implements PageTransformer { } } - // Prepare data for template const templateData = await this.prepareTemplateData( page, categorizedEvents, @@ -73,10 +68,8 @@ export class AuditLogsTransformer implements PageTransformer { resolveReferenceLinksToMarkdown, ) - // Load and render template const templateContent = loadTemplate(this.templateName) - // Render the template with Liquid const rendered = await renderContent(templateContent, { ...context, ...templateData, @@ -86,9 +79,6 @@ export class AuditLogsTransformer implements PageTransformer { return rendered } - /** - * Prepare data for the Liquid template - */ private async prepareTemplateData( page: Page, categorizedEvents: CategorizedEvents, @@ -100,7 +90,6 @@ export class AuditLogsTransformer implements PageTransformer { context: TitleResolutionContext, ) => Promise, ): Promise> { - // Prepare page intro const intro = page.intro ? await page.renderProp('intro', context, { textOnly: true }) : '' // Sort categories and events, and compute fields shared by most (≥80%) events diff --git a/src/article-api/transformers/bespoke-landing-transformer.ts b/src/article-api/transformers/bespoke-landing-transformer.ts index 8f3916f390e3..490b2cc1094d 100644 --- a/src/article-api/transformers/bespoke-landing-transformer.ts +++ b/src/article-api/transformers/bespoke-landing-transformer.ts @@ -13,10 +13,9 @@ interface BespokeLandingPage extends Omit { } /** - * Transforms bespoke-landing pages into markdown format. - * Handles carousels and full article listings. - * Note: Unlike discovery-landing, bespoke-landing shows ALL articles - * regardless of includedCategories. + * Transforms bespoke-landing pages into markdown. + * Unlike discovery-landing, this shows every article regardless of + * includedCategories, which only filters discovery-landing pages. */ export class BespokeLandingTransformer implements PageTransformer { templateName = 'landing-page.template.md' @@ -80,7 +79,6 @@ export class BespokeLandingTransformer implements PageTransformer { const validLinks = links.filter((l) => l.href && l.title) if (validLinks.length > 0) { - // Use carousel key as title (capitalize first letter) const sectionTitle = carouselKey.charAt(0).toUpperCase() + carouselKey.slice(1) sections.push({ title: sectionTitle, @@ -90,14 +88,12 @@ export class BespokeLandingTransformer implements PageTransformer { } } - // Articles section: recursively gather ALL descendant articles - // This matches the behavior of the site which uses genericTocFlat/genericTocNested - // Note: For bespoke-landing pages, the site shows ALL articles regardless of includedCategories - // (includedCategories only filters for discovery-landing pages) + // Recursively gather every descendant article, matching the site's + // genericTocFlat/genericTocNested behaviour. if (bespokePage.children && bespokePage.children.length > 0) { const tocItems = await getAllTocItems(page, context) - // Flatten to get all leaf articles (excludeParents: true means only get articles, not category pages) + // excludeParents keeps only leaf TOC items, dropping anything with children. const allArticles = flattenTocItems(tocItems, { excludeParents: true }) if (allArticles.length > 0) { diff --git a/src/article-api/transformers/category-landing-transformer.ts b/src/article-api/transformers/category-landing-transformer.ts index 3678372fb36c..f75cdf08579f 100644 --- a/src/article-api/transformers/category-landing-transformer.ts +++ b/src/article-api/transformers/category-landing-transformer.ts @@ -32,11 +32,8 @@ export class CategoryLandingTransformer implements PageTransformer { }) } - /** - * Recursively collects all descendant articles from the given parent hrefs. - * Traverses the page tree, adding non-index pages and recursing into children. - * Uses a visited set to prevent infinite loops from circular references. - */ + // Walks the page tree from the given parent hrefs, collecting every non-index + // descendant. The visited set guards against circular references. private async getAllDescendantArticles( parentHrefs: string[], languageCode: string, @@ -47,7 +44,6 @@ export class CategoryLandingTransformer implements PageTransformer { const allArticles: LinkData[] = [] for (const href of parentHrefs) { - // Prevent infinite loops from circular references if (visited.has(href)) continue visited.add(href) @@ -56,7 +52,6 @@ export class CategoryLandingTransformer implements PageTransformer { | undefined if (!parentPage) continue - // Add this page if it's an article (not an index) if (!parentPage.relativePath.endsWith('index.md')) { const linkData = await getLinkData(href, languageCode, pathname, context, resolvePath) if (linkData.href) { @@ -64,7 +59,6 @@ export class CategoryLandingTransformer implements PageTransformer { } } - // Recursively get children const children = parentPage.children if (children && Array.isArray(children) && children.length > 0) { // Get the parent's permalink to use as the base path for resolving children @@ -96,7 +90,6 @@ export class CategoryLandingTransformer implements PageTransformer { const languageCode = page.languageCode || 'en' const sections: Section[] = [] - // Spotlight section const spotlight = categoryPage.spotlight if (spotlight && spotlight.length > 0) { const links = await Promise.all( @@ -126,7 +119,6 @@ export class CategoryLandingTransformer implements PageTransformer { } } - // Children - get all descendant articles recursively if (categoryPage.children) { const allArticles = await this.getAllDescendantArticles( categoryPage.children, diff --git a/src/article-api/transformers/codeql-cli-transformer.ts b/src/article-api/transformers/codeql-cli-transformer.ts index f32c8fbc903e..2aa07e4a187b 100644 --- a/src/article-api/transformers/codeql-cli-transformer.ts +++ b/src/article-api/transformers/codeql-cli-transformer.ts @@ -5,10 +5,7 @@ import { loadTemplate } from '@/article-api/lib/load-template' import { stripHtmlCommentsAndNormalizeWhitespace } from '@/article-api/lib/strip-html-comments' /** - * Transformer for CodeQL CLI reference pages. - * Renders autogenerated CodeQL CLI documentation pages as markdown using a Liquid template. - * Sets `markdownRequested` to true in the context to ensure the page is rendered as markdown, - * bypassing the default article type check. + * Renders autogenerated CodeQL CLI reference pages as markdown using a Liquid template. */ export class CodeQLCliTransformer implements PageTransformer { templateName = 'codeql-cli-page.template.md' @@ -25,7 +22,6 @@ export class CodeQLCliTransformer implements PageTransformer { const intro = page.intro ? await page.renderProp('intro', context, { textOnly: true }) : '' - // Prepare template data const templateData: Record = { page: { title: page.title, @@ -34,7 +30,6 @@ export class CodeQLCliTransformer implements PageTransformer { content, } - // Load and render template const templateContent = loadTemplate(this.templateName) const result = await renderContent(templateContent, { @@ -43,7 +38,7 @@ export class CodeQLCliTransformer implements PageTransformer { markdownRequested: true, }) - // Strip HTML comments (e.g., markdownlint-disable comments) from the output + // Strip markdownlint-disable and any other HTML comments. return stripHtmlCommentsAndNormalizeWhitespace(result) } } diff --git a/src/article-api/transformers/discovery-landing-transformer.ts b/src/article-api/transformers/discovery-landing-transformer.ts index 6cf5a3d9e0fd..bcf16217b4ac 100644 --- a/src/article-api/transformers/discovery-landing-transformer.ts +++ b/src/article-api/transformers/discovery-landing-transformer.ts @@ -80,7 +80,6 @@ export class DiscoveryLandingTransformer implements PageTransformer { const validLinks = links.filter((l) => l.href && l.title) if (validLinks.length > 0) { - // Use carousel key as title (capitalize first letter) const sectionTitle = carouselKey.charAt(0).toUpperCase() + carouselKey.slice(1) sections.push({ title: sectionTitle, @@ -90,7 +89,6 @@ export class DiscoveryLandingTransformer implements PageTransformer { } } - // Intro links (getting started) const rawIntroLinks = discoveryPage.introLinks ?? discoveryPage.rawIntroLinks if (rawIntroLinks) { const { default: getPageLinkData } = await import('@/frame/lib/get-link-data') @@ -134,10 +132,9 @@ export class DiscoveryLandingTransformer implements PageTransformer { if (discoveryPage.children && discoveryPage.children.length > 0) { const tocItems = await getAllTocItems(page, context) - // Flatten to get all leaf articles (excludeParents: true means only get articles, not category pages) + // excludeParents keeps only leaf TOC items, dropping anything with children. let allArticles = flattenTocItems(tocItems, { excludeParents: true }) - // Apply includedCategories filter if specified if (discoveryPage.includedCategories && discoveryPage.includedCategories.length > 0) { const includedCategories = discoveryPage.includedCategories.map((c) => c.toLowerCase()) diff --git a/src/article-api/transformers/github-apps-transformer.ts b/src/article-api/transformers/github-apps-transformer.ts index 72cecd46999b..bcc49cb9eae7 100644 --- a/src/article-api/transformers/github-apps-transformer.ts +++ b/src/article-api/transformers/github-apps-transformer.ts @@ -6,7 +6,6 @@ import matter from '@gr2m/gray-matter' const DEBUG = process.env.RUNNER_DEBUG === '1' || process.env.DEBUG === '1' -// GitHub Apps data types interface GitHubAppsOperation { slug: string subcategory?: string @@ -105,17 +104,14 @@ export class GithubAppsTransformer implements PageTransformer { // Import getAppsData dynamically to avoid circular dependencies const { getAppsData } = await import('@/github-apps/lib/index') - // Extract version from context const currentVersion = context.currentVersion! - // Use the provided apiVersion, or fall back to the latest from context const effectiveApiVersion = apiVersion || (context.currentVersionObj?.apiVersions?.length ? context.currentVersionObj.latestApiVersion : undefined) - // Determine page type from the page's relative path const filename = page.relativePath.split('/').pop() const pageType = filename ? PAGE_TYPE_MAP[filename] : undefined @@ -123,12 +119,10 @@ export class GithubAppsTransformer implements PageTransformer { throw new Error(`Unknown GitHub Apps page type for path: ${page.relativePath}`) } - // Get the GitHub Apps data const appsData = (await getAppsData(pageType, currentVersion, effectiveApiVersion)) as | GitHubAppsListData | GitHubAppsPermissionsData - // Prepare manual content let manualContent = '' if (page.markdown) { const { content } = matter(page.markdown) @@ -139,7 +133,6 @@ export class GithubAppsTransformer implements PageTransformer { markdownRequested: true, }) - // Strip HTML comments and normalize whitespace manualContent = manualContent .replace(//gs, '') .replace(/\n{3,}/g, '\n\n') @@ -147,7 +140,6 @@ export class GithubAppsTransformer implements PageTransformer { } } - // Prepare data for template based on page type const isListPage = LIST_PAGE_TYPES.has(pageType) const isPermissionsPage = PERMISSIONS_PAGE_TYPES.has(pageType) @@ -160,13 +152,11 @@ export class GithubAppsTransformer implements PageTransformer { isPermissionsPage, ) - // Load template const templateContent = loadTemplate(this.templateName) // For permissions pages, we need to construct the tables manually to avoid Liquid escaping let finalContent: string if (isPermissionsPage) { - // Build the intro manually let introMarkdown = `# ${templateData.page.title}\n\n` if (templateData.page.intro) { introMarkdown += `${templateData.page.intro}\n\n` @@ -180,7 +170,6 @@ export class GithubAppsTransformer implements PageTransformer { introMarkdown += `**Token types:** UAT = user access token, IAT = installation access token\n\n` } - // Build the tables manually let tablesMarkdown = '' for (const item of templateData.items as PreparedPermissionItem[]) { tablesMarkdown += `## ${item.displayTitle}\n\n` @@ -229,9 +218,6 @@ export class GithubAppsTransformer implements PageTransformer { return finalContent } - /** - * Prepare data for the Liquid template - */ private async prepareTemplateData( page: Page, appsData: GitHubAppsListData | GitHubAppsPermissionsData, @@ -246,13 +232,10 @@ export class GithubAppsTransformer implements PageTransformer { isPermissionsPage: boolean items: PreparedListItem[] | PreparedPermissionItem[] }> { - // Prepare page intro const intro = page.intro ? await page.renderProp('intro', context, { textOnly: true }) : '' - // Get categories without subcategories from rest lib const { categoriesWithoutSubcategories } = await import('@/rest/lib/index') - // Prepare items based on page type let preparedItems: PreparedListItem[] | PreparedPermissionItem[] = [] if (isListPage) { @@ -270,7 +253,6 @@ export class GithubAppsTransformer implements PageTransformer { preparedItems = Object.entries(appsData as GitHubAppsPermissionsData).map( ([permissionName, permissionObject]) => { const { displayTitle, permissions } = permissionObject - // Sort by access level (admin, write, read) const adminPermissions = permissions.filter((p) => p.access === 'admin') const writePermissions = permissions.filter((p) => p.access === 'write') const readPermissions = permissions.filter((p) => p.access === 'read') @@ -299,9 +281,6 @@ export class GithubAppsTransformer implements PageTransformer { } } - /** - * Prepare a single operation for list-based rendering (endpoints) - */ private prepareOperation( operation: GitHubAppsOperation, category: string, @@ -321,9 +300,6 @@ export class GithubAppsTransformer implements PageTransformer { } } - /** - * Prepare a single operation for permissions-based rendering (tables) - */ private preparePermissionOperation( operation: GitHubAppsPermissionOperation, categoriesWithoutSubcategories: string[], diff --git a/src/article-api/transformers/graphql-breaking-changes-transformer.ts b/src/article-api/transformers/graphql-breaking-changes-transformer.ts index e0268c7351c5..6a1d6fb62ea6 100644 --- a/src/article-api/transformers/graphql-breaking-changes-transformer.ts +++ b/src/article-api/transformers/graphql-breaking-changes-transformer.ts @@ -8,8 +8,7 @@ import { extractManualContent } from '@/article-api/lib/graphql-helpers' import GithubSlugger from 'github-slugger' /** - * Transformer for GraphQL breaking changes page - * Renders breaking changes organized by date + * Renders the GraphQL breaking changes page, organized by date. */ export class GraphQLBreakingChangesTransformer implements PageTransformer { templateName = 'graphql-breaking-changes.template.md' @@ -32,7 +31,6 @@ export class GraphQLBreakingChangesTransformer implements PageTransformer { const slugger = new GithubSlugger() - // Process breaking changes by date const breakingChangesByDate = Object.keys(schema).map((date) => { const items = schema[date] const heading = `Changes scheduled for ${date}` diff --git a/src/article-api/transformers/graphql-changelog-transformer.ts b/src/article-api/transformers/graphql-changelog-transformer.ts index 53d764cbdb09..513d66f6d73f 100644 --- a/src/article-api/transformers/graphql-changelog-transformer.ts +++ b/src/article-api/transformers/graphql-changelog-transformer.ts @@ -7,8 +7,7 @@ import { fastTextOnly } from '@/content-render/unified/text-only' import { extractManualContent } from '@/article-api/lib/graphql-helpers' /** - * Transformer for GraphQL changelog page - * Renders the changelog with schema changes, preview changes, and upcoming changes + * Renders the GraphQL changelog: schema changes, preview changes, and upcoming changes. */ export class GraphQLChangelogTransformer implements PageTransformer { templateName = 'graphql-changelog.template.md' @@ -25,7 +24,6 @@ export class GraphQLChangelogTransformer implements PageTransformer { const { getGraphqlChangelogByYear, getGraphqlChangelogYears } = await import('@/graphql/lib/index') - // Determine if this is a year-specific page const yearMatch = page.relativePath.match(/changelog\/(\d{4})\.md$/) const year = yearMatch ? Number(yearMatch[1]) : null const years = getGraphqlChangelogYears(currentVersion) @@ -42,13 +40,11 @@ export class GraphQLChangelogTransformer implements PageTransformer { const intro = page.intro ? await page.renderProp('intro', context, { textOnly: true }) : '' const manualContent = await extractManualContent(page, context) - // Process changelog items const changelogItems = schema.map((item) => { const processChanges = (changes: Array<{ title: string; changes: string[] }>) => changes.map((change) => ({ title: change.title, changes: change.changes.map((html: string) => { - // Remove wrapping

tags if present if (html.startsWith('

') && html.endsWith('

')) { return fastTextOnly(html.slice(3, -4)) } @@ -64,7 +60,6 @@ export class GraphQLChangelogTransformer implements PageTransformer { } }) - // Build year navigation links const displayYear = year || years[0] const yearNavItems = years.map((y) => ({ year: y, diff --git a/src/article-api/transformers/graphql-index-transformer.ts b/src/article-api/transformers/graphql-index-transformer.ts index b46b41cf9100..dd400a8173c7 100644 --- a/src/article-api/transformers/graphql-index-transformer.ts +++ b/src/article-api/transformers/graphql-index-transformer.ts @@ -5,8 +5,7 @@ import { loadTemplate } from '@/article-api/lib/load-template' import { extractManualContent } from '@/article-api/lib/graphql-helpers' /** - * Transformer for GraphQL reference index page - * Renders the index page with links to child pages + * Renders the GraphQL reference index page with links to its child pages. */ export class GraphQLIndexTransformer implements PageTransformer { templateName = 'graphql-index.template.md' @@ -14,7 +13,6 @@ export class GraphQLIndexTransformer implements PageTransformer { canTransform(page: Page): boolean { if (page.autogenerated !== 'graphql') return false - // Match the reference index page (no specific page type after /reference) return page.relativePath.endsWith('graphql/reference/index.md') } @@ -23,7 +21,6 @@ export class GraphQLIndexTransformer implements PageTransformer { const manualContent = await extractManualContent(page, context) - // Get children links from page metadata const children = page.children || [] const childrenLinks = children .map((child) => { diff --git a/src/article-api/transformers/graphql-reference-transformer.ts b/src/article-api/transformers/graphql-reference-transformer.ts index bde951e507a0..73989be04107 100644 --- a/src/article-api/transformers/graphql-reference-transformer.ts +++ b/src/article-api/transformers/graphql-reference-transformer.ts @@ -17,8 +17,7 @@ import { fastTextOnly } from '@/content-render/unified/text-only' import { extractManualContent } from '@/article-api/lib/graphql-helpers' /** - * Transformer for GraphQL reference pages (queries, mutations, objects, etc.) - * Renders schema items with their fields and arguments + * Renders GraphQL reference pages: schema items with their fields and arguments. */ export class GraphQLReferenceTransformer implements PageTransformer { templateName = 'graphql-reference.template.md' @@ -26,7 +25,6 @@ export class GraphQLReferenceTransformer implements PageTransformer { canTransform(page: Page): boolean { if (page.autogenerated !== 'graphql') return false - // Match reference pages that have a specific page type (not index) const isReference = page.relativePath.includes('graphql/reference/') const isNotIndex = !page.relativePath.endsWith('index.md') @@ -36,7 +34,6 @@ export class GraphQLReferenceTransformer implements PageTransformer { async transform(page: Page, pathname: string, context: Context): Promise { const currentVersion = context.currentVersion! - // Determine the page slug from the pathname const pathParts = pathname.split('/').filter(Boolean) const graphqlIndex = pathParts.indexOf('graphql') const pageType = pathParts[graphqlIndex + 2] // category slug like 'repos', 'issues', etc. @@ -300,10 +297,7 @@ export class GraphQLReferenceTransformer implements PageTransformer { private static STANDARD_PAGINATION_ARGS = new Set(['after', 'before', 'first', 'last']) - /** - * Check if a field has only the standard pagination arguments - * (after, before, first, last) with no additional args. - */ + // True when the field's only arguments are after, before, first and last. private hasOnlyStandardPaginationArgs(field: FieldT): boolean { if (!field.arguments || field.arguments.length !== 4) return false return field.arguments.every((arg) => diff --git a/src/article-api/transformers/index.ts b/src/article-api/transformers/index.ts index d5efd9407235..a6fc2d634860 100644 --- a/src/article-api/transformers/index.ts +++ b/src/article-api/transformers/index.ts @@ -18,10 +18,7 @@ import { SearchPageTransformer } from './search-page-transformer' import { ReleaseNotesTransformer } from './release-notes-transformer' import { ArticleTransformer } from './article-transformer' -/** - * Global transformer registry - * Registers all available page-to-markdown transformers - */ +// Every page-to-markdown transformer is registered here. export const transformerRegistry = new TransformerRegistry() transformerRegistry.register(new RestTransformer()) @@ -41,7 +38,7 @@ transformerRegistry.register(new CategoryLandingTransformer()) transformerRegistry.register(new DiscoveryLandingTransformer()) transformerRegistry.register(new SearchPageTransformer()) transformerRegistry.register(new ReleaseNotesTransformer()) -// ArticleTransformer is the catch-all — must be registered last. +// ArticleTransformer is the catch-all, so it must be registered last. transformerRegistry.register(new ArticleTransformer()) export { TransformerRegistry } from './types' diff --git a/src/article-api/transformers/journey-landing-transformer.ts b/src/article-api/transformers/journey-landing-transformer.ts index 959b28deaf6a..e966d2b58a7f 100644 --- a/src/article-api/transformers/journey-landing-transformer.ts +++ b/src/article-api/transformers/journey-landing-transformer.ts @@ -23,9 +23,9 @@ interface JourneyPage extends Page { } /** - * Transforms journey-landing pages into markdown format. - * Handles journey tracks (grouped learning paths) with guides, - * falling back to children listings when tracks aren't available. + * Transforms journey-landing pages into markdown. Renders journey tracks + * (grouped learning paths), falling back to a children listing when no track + * produces a renderable link. */ export class JourneyLandingTransformer implements PageTransformer { templateName = 'landing-page.template.md' @@ -54,7 +54,6 @@ export class JourneyLandingTransformer implements PageTransformer { const languageCode = page.languageCode || 'en' const sections: Section[] = [] - // Journey tracks const journeyTracks = journeyPage.journeyTracks if (journeyTracks) { const groups: LinkGroup[] = [] @@ -87,7 +86,6 @@ export class JourneyLandingTransformer implements PageTransformer { } } - // Children fallback if (sections.length === 0 && journeyPage.children) { const links = await Promise.all( journeyPage.children.map(async (childHref) => { diff --git a/src/article-api/transformers/rest-transformer.ts b/src/article-api/transformers/rest-transformer.ts index 3685f5a87b4f..c3af44277ed8 100644 --- a/src/article-api/transformers/rest-transformer.ts +++ b/src/article-api/transformers/rest-transformer.ts @@ -52,15 +52,13 @@ type PreparedTemplateData = { } /** - * Transformer for REST API pages - * Converts REST operations and their data into markdown format using a Liquid template + * Converts REST operations and their data into markdown using a Liquid template. */ export class RestTransformer implements PageTransformer { templateName = 'rest-page.template.md' canTransform(page: Page): boolean { - // Only transform REST pages that are not landing pages - // Landing pages (like /en/rest) will be handled by a separate transformer + // Landing pages like /en/rest are handled by a different transformer. return page.autogenerated === 'rest' && !page.relativePath.endsWith('index.md') } @@ -76,17 +74,14 @@ export class RestTransformer implements PageTransformer { // Import getRest dynamically to avoid circular dependencies const { default: getRest } = await import('@/rest/lib/index') - // Extract version from context const currentVersion = context.currentVersion! - // Use the provided apiVersion, or fall back to the latest from context const effectiveApiVersion = apiVersion || (context.currentVersionObj?.apiVersions?.length ? context.currentVersionObj.latestApiVersion : undefined) - // Parse the category and subcategory from the page path // e.g. /en/rest/actions/artifacts -> category: actions, subcategory: artifacts const pathParts = pathname.split('/').filter(Boolean) const restIndex = pathParts.indexOf('rest') @@ -98,7 +93,6 @@ export class RestTransformer implements PageTransformer { const category = pathParts[restIndex + 1] const subcategory = pathParts[restIndex + 2] // May be undefined for category-only pages - // Get the REST operations data const categoryData = await getRest(currentVersion, effectiveApiVersion, category) let operations: Operation[] = [] @@ -106,12 +100,10 @@ export class RestTransformer implements PageTransformer { if (subcategory && categoryData?.[subcategory]) { operations = categoryData[subcategory] } else if (category && categoryData) { - // For categories without subcategories, operations are nested directly - // Flatten all operations from all subcategories + // Categories without subcategories nest their operations one level deeper. operations = Object.values(categoryData).flat() } - // Prepare manual content let manualContent = '' if (page.markdown) { const markerIndex = page.markdown.indexOf( @@ -134,7 +126,6 @@ export class RestTransformer implements PageTransformer { } } - // Prepare data for template const templateData = await this.prepareTemplateData( page, operations, @@ -143,7 +134,6 @@ export class RestTransformer implements PageTransformer { effectiveApiVersion, ) - // Load and render template const templateContent = loadTemplate(this.templateName) // Render the template with Liquid. templateData intentionally replaces @@ -159,9 +149,6 @@ export class RestTransformer implements PageTransformer { return rendered } - /** - * Prepare data for the Liquid template - */ private async prepareTemplateData( page: Page, operations: Operation[], @@ -169,10 +156,8 @@ export class RestTransformer implements PageTransformer { manualContent: string, apiVersion?: string, ): Promise { - // Prepare page intro const intro = page.intro ? await page.renderProp('intro', context, { textOnly: true }) : '' - // Prepare operations for the template const preparedOperations = await Promise.all( operations.map(async (operation) => await this.prepareOperation(operation)), ) @@ -211,35 +196,26 @@ export class RestTransformer implements PageTransformer { } } - /** - * Prepare a single operation for template rendering - */ private async prepareOperation(operation: Operation): Promise { - // Convert HTML description to text const description = operation.descriptionHTML ? fastTextOnly(operation.descriptionHTML) : '' - // Determine header settings const needsContentTypeHeader = operation.subcategory === 'inference' const omitHeaders = operation.subcategory === 'management-console' || operation.subcategory === 'manage-ghes' const showHeaders = !omitHeaders - // Check if operation has parameters const hasParameters = (operation.parameters?.length || 0) > 0 || (operation.bodyParameters?.length || 0) > 0 - // Process status codes to convert HTML descriptions to plain text const statusCodes = operation.statusCodes?.map((statusCode) => ({ ...statusCode, description: statusCode.description ? fastTextOnly(statusCode.description) : undefined, })) - // Prepare code examples with processed URLs const codeExamples = operation.codeExamples?.map((example) => { let url = `${operation.serverUrl}${operation.requestPath}` - // Replace path parameters in URL if (example.request?.parameters && Object.keys(example.request.parameters).length > 0) { for (const [key, value] of Object.entries(example.request.parameters)) { url = url.replace(`{${key}}`, String(value)) diff --git a/src/article-api/transformers/search-page-transformer.ts b/src/article-api/transformers/search-page-transformer.ts index c3756298ecc5..ed5b7e7a8871 100644 --- a/src/article-api/transformers/search-page-transformer.ts +++ b/src/article-api/transformers/search-page-transformer.ts @@ -2,15 +2,13 @@ import type { Context, Page } from '@/types' import type { PageTransformer } from './types' /** - * Transformer for the search page (/en/search). - * This is a special UI-only page with no markdown content. - * Returns minimal markdown with just the title. + * /en/search is a UI-only page with no markdown content, so this returns the + * title plus a pointer to the Search API. */ export class SearchPageTransformer implements PageTransformer { templateName = '' canTransform(page: Page): boolean { - // Only match the search page specifically return page.relativePath === 'search/index.md' } diff --git a/src/article-api/transformers/secret-scanning-transformer.ts b/src/article-api/transformers/secret-scanning-transformer.ts index 2d8550e9b50d..e1ecaeed4bbf 100644 --- a/src/article-api/transformers/secret-scanning-transformer.ts +++ b/src/article-api/transformers/secret-scanning-transformer.ts @@ -8,9 +8,7 @@ import { loadTemplate } from '@/article-api/lib/load-template' import { getSecretScanningData } from '@/secret-scanning/lib/get-secret-scanning-data' /** - * Transformer for Secret Scanning pages. - * Loads pattern data and converts secret scanning documentation into markdown format. - * Used by the Article API to render Secret Scanning documentation dynamically. + * Loads secret scanning pattern data and converts it into markdown. */ export class SecretScanningTransformer implements PageTransformer { templateName = 'secret-scanning-page.template.md' @@ -38,7 +36,6 @@ export class SecretScanningTransformer implements PageTransformer { try { const data = await getSecretScanningData(filepath) - // Process Liquid in values for (const entry of data) { // Process Liquid for the hasValidityCheck field, as in the middleware if (typeof entry.hasValidityCheck === 'string' && entry.hasValidityCheck.includes('{%')) { @@ -104,13 +101,10 @@ export class SecretScanningTransformer implements PageTransformer { content += table } - // Strip HTML comments from the rendered content content = content.replace(//gs, '') - // Replace HTML icon spans with plain text equivalents content = content.replace(/]*aria-label="Supported"[^>]*>[^<]*<\/span>/g, '✓') content = content.replace(/]*aria-label="Unsupported"[^>]*>[^<]*<\/span>/g, '✗') - // Convert
tags to newlines and text to markdown links content = content.replace(//gi, '\n') content = content.replace(/]*>([^<]*)<\/a>/gi, '[$2]($1)') // Strip any remaining HTML tags. Loop until stable to handle nested or @@ -125,12 +119,11 @@ export class SecretScanningTransformer implements PageTransformer { iterations++ } - // Normalize whitespace after stripping comments content = content.replace(/\n{3,}/g, '\n\n').trim() const intro = page.intro ? await page.renderProp('intro', context, { textOnly: true }) : '' - // Render the template with Liquid only — page.render() already ran + // Render the template with Liquid only. page.render() already ran // rewriteLocalLinks on all markdown links, and the regex cleanup above // only creates fragment links (e.g. #token-versions) which don't need // link rewriting. So we skip the expensive remark re-parse. diff --git a/src/article-api/transformers/toc-transformer.ts b/src/article-api/transformers/toc-transformer.ts index 1ff9f79f1f9c..cbe51d4204ba 100644 --- a/src/article-api/transformers/toc-transformer.ts +++ b/src/article-api/transformers/toc-transformer.ts @@ -10,16 +10,14 @@ interface CategoryPage extends Page { } /** - * Transformer for table of contents (TOC) landing pages - pages with children but no specific layout. - * These are simple navigation pages (category/subcategory/product/homepage) that list child pages with titles and intros. - * Corresponds to TocLanding component in the web UI. + * Transformer for pages that have children but no specific layout: category, + * subcategory, product and homepage. Lists each child with its title and intro. + * Corresponds to the TocLanding component in the web UI. */ export class TocTransformer implements PageTransformer { templateName = 'landing-page.template.md' canTransform(page: Page): boolean { - // Transform pages that have children but no layout specified - // These are typically category, subcategory, product, or homepage pages const categoryPage = page as CategoryPage const validDocTypes = ['category', 'subcategory', 'product', 'homepage'] return ( @@ -50,7 +48,6 @@ export class TocTransformer implements PageTransformer { const languageCode = page.languageCode || 'en' const sections: Section[] = [] - // Get all child pages if (mapPage.children?.length) { const links = await Promise.all( mapPage.children.map(async (childHref) => { diff --git a/src/article-api/transformers/types.ts b/src/article-api/transformers/types.ts index aa2441f816e2..cc43d421b52d 100644 --- a/src/article-api/transformers/types.ts +++ b/src/article-api/transformers/types.ts @@ -9,17 +9,11 @@ export interface LinkData { intro?: string } -/** - * Group of links with an optional title - */ export interface LinkGroup { title: string | null links: LinkData[] } -/** - * Section containing multiple link groups - */ export interface Section { title: string | null groups: LinkGroup[] @@ -35,106 +29,34 @@ export interface TemplateData { } /** - * Base interface for page-to-markdown transformers - * - * Transformers convert autogenerated pages (REST, webhooks, etc.) - * into markdown format for the Article API + * Converts a page into markdown. */ export interface PageTransformer { - /** - * Optional: The name of the template file to use for rendering - * If provided, can be used by helper functions to load the template - * Example: 'landing-page.template.md' - */ + /** Template file to render with, e.g. 'landing-page.template.md'. */ templateName?: string - /** - * Check if this transformer can handle the given page - */ canTransform(page: Page): boolean - /** - * Transform the page into markdown format - * @param page - The page to transform - * @param pathname - The pathname of the page - * @param context - The rendering context - * @param apiVersion - Optional API version (e.g., '2022-11-28' for REST API calendar versioning) - */ + /** `apiVersion` is the REST calendar version, e.g. '2022-11-28'. */ transform(page: Page, pathname: string, context: Context, apiVersion?: string): Promise } /** - * Registry of available transformers for converting pages to markdown - * - * The TransformerRegistry manages a collection of PageTransformer instances - * and provides a mechanism to find the appropriate transformer for a given page. - * - * Transformers are evaluated in registration order. The first transformer - * whose `canTransform()` method returns true will be selected. - * - * @example - * ```typescript - * const registry = new TransformerRegistry() - * - * // Register transformers in priority order - * registry.register(new RestTransformer()) - * registry.register(new WebhookTransformer()) - * registry.register(new GraphQLTransformer()) - * - * // Find and use a transformer - * const transformer = registry.findTransformer(page) - * if (transformer) { - * const markdown = await transformer.transform(page, pathname, context) - * } - * ``` + * Transformers are evaluated in registration order, and the first one whose + * `canTransform()` returns true wins. Register the specific ones before the + * general ones. * - * @remarks - * This class is not thread-safe. In server environments with concurrent requests, - * register all transformers during initialization before handling requests. + * This class is not thread-safe, so register everything during initialization + * rather than while handling requests. */ export class TransformerRegistry { private transformers: PageTransformer[] = [] - /** - * Register a new transformer - * - * Transformers are evaluated in registration order when finding a match. - * Register more specific transformers before more general ones. - * - * @param transformer - The transformer to register - * - * @example - * ```typescript - * const registry = new TransformerRegistry() - * registry.register(new RestTransformer()) - * ``` - */ register(transformer: PageTransformer): void { this.transformers.push(transformer) } - /** - * Find a transformer that can handle the given page - * - * Iterates through registered transformers in registration order and returns - * the first transformer whose `canTransform()` method returns true. - * - * @param page - The page to find a transformer for - * @returns The first matching transformer, or null if: - * - The page is null/undefined - * - No registered transformer can handle the page - * - * @example - * ```typescript - * const transformer = registry.findTransformer(page) - * if (transformer) { - * const markdown = await transformer.transform(page, pathname, context) - * } else { - * // Handle case where no transformer is available - * console.warn('No transformer found for page:', page.relativePath) - * } - * ``` - */ + /** Returns null when `page` is nullish or nothing can handle it. */ findTransformer(page: Page): PageTransformer | null { if (page == null) { return null diff --git a/src/article-api/transformers/webhooks-transformer.ts b/src/article-api/transformers/webhooks-transformer.ts index ba2a4c623bd5..d780665852fb 100644 --- a/src/article-api/transformers/webhooks-transformer.ts +++ b/src/article-api/transformers/webhooks-transformer.ts @@ -6,8 +6,7 @@ import { loadTemplate } from '@/article-api/lib/load-template' import matter from '@gr2m/gray-matter' /** - * Transformer for Webhooks pages. - * Converts webhook events and payloads into markdown format using a Liquid template. + * Converts webhook events and payloads into markdown using a Liquid template. */ export class WebhooksTransformer implements PageTransformer { templateName = 'webhooks-page.template.md' @@ -20,16 +19,12 @@ export class WebhooksTransformer implements PageTransformer { // Import getInitialPageWebhooks dynamically to avoid circular dependencies const { getInitialPageWebhooks } = await import('@/webhooks/lib/index') - // Extract version from context const currentVersion = context.currentVersion! - // Get the webhook data const webhooksData = await getInitialPageWebhooks(currentVersion) - // Prepare page intro const intro = page.intro ? await page.renderProp('intro', context, { textOnly: true }) : '' - // Prepare manual content let manualContent = '' if (page.markdown) { const { content } = matter(page.markdown) @@ -88,7 +83,6 @@ export class WebhooksTransformer implements PageTransformer { ) } - // Prepare template data const templateData: Record = { page: { title: page.title, @@ -99,7 +93,6 @@ export class WebhooksTransformer implements PageTransformer { commonParams, } - // Load and render template const templateContent = loadTemplate(this.templateName) return await renderContent(templateContent, { diff --git a/src/frame/lib/frontmatter.ts b/src/frame/lib/frontmatter.ts index b07dd08b6779..6abc6f1d8777 100644 --- a/src/frame/lib/frontmatter.ts +++ b/src/frame/lib/frontmatter.ts @@ -66,7 +66,7 @@ export const contentTypesEnum = [ // Values supported in the docsTeamMetrics frontmatter property. Used to track // related articles (e.g. by feature or subject) that may span different directories. -export const docsTeamMetricsEnum = ['copilot-cli', 'enterprise-onboarding'] +export const docsTeamMetricsEnum = ['ai-governance', 'copilot-cli', 'enterprise-onboarding'] export const schema: Schema = { type: 'object', diff --git a/src/languages/lib/correct-translation-content.ts b/src/languages/lib/correct-translation-content.ts index a251edebecea..a48c0453968f 100644 --- a/src/languages/lib/correct-translation-content.ts +++ b/src/languages/lib/correct-translation-content.ts @@ -31,6 +31,16 @@ export function correctTranslatedContentStrings( // patterns, so this is safe globally. content = content.replace(/\{\s*%(-?)(\s*\S[^%]*?\s*)(-?)%\s*\}/g, '{%$1$2$3%}') + // Translators sometimes left a stray `%` right before the closing `}}` + // of a Liquid output tag (e.g. `{{ allVersions[currentVersion].currentRelease %}}` + // instead of `{{ allVersions[currentVersion].currentRelease }}`), likely from + // copy-pasting a neighboring `{% ... %}` tag's closer. Liquid then tries to + // parse `%` as a filter pipe and fails with `expected "|" before filter`. + // Strip the stray `%` — the English source never has one before `}}`. + // Excludes `{{%` openers (a different corruption handled elsewhere, where + // the whole tag is double-braced). + content = content.replace(/\{\{(?!%)([^{}]*[^{}%])%(\}\})/g, '{{$1$2') + // Translators sometimes dropped the `data` keyword in front of a // `variables.X.Y` / `reusables.X.Y` / `product.X` path. The English // source never starts a Liquid tag with these prefixes; they always @@ -657,6 +667,22 @@ export function correctTranslatedContentStrings( // `roleColumns` interpreted as an unknown tag name (`tag "roleColumns" // not found`). content = content.replace(/\{%(-?)\s*roleColumns\s*=\s*/g, '{%$1 assign roleColumns = ') + + // [SCRAPE] data/reusables/actions/github-token-scope-descriptions.md + // (also included by data/reusables/actions/jobs/section-assigning-permissions-to-jobs.md): + // the `{% ifversion vulnerability-alerts-permission %}...{% else %}...{% endif %}` + // block describing the `security-events` row's Dependabot/secret-scanning + // caveat is missing its closing `{% endif %}`, leaving the tag never + // closed (`tag {% ifversion vulnerability-alerts-permission %} not closed`). + if ( + context.dottedPath === 'reusables.actions.github-token-scope-descriptions' || + context.relativePath?.endsWith('data/reusables/actions/github-token-scope-descriptions.md') + ) { + content = content.replace( + '「シークレット スキャン アラート」のリポジトリのアクセス許可を参照してください。 |', + '「シークレット スキャン アラート」のリポジトリのアクセス許可を参照してください。{% endif %} |', + ) + } } if (context.code === 'pt') { diff --git a/src/languages/tests/correct-translation-content.ts b/src/languages/tests/correct-translation-content.ts index 3acbd0880536..547e2fb8dc7f 100644 --- a/src/languages/tests/correct-translation-content.ts +++ b/src/languages/tests/correct-translation-content.ts @@ -1676,6 +1676,27 @@ describe('correctTranslatedContentStrings', () => { ) }) + test('fixes stray % before closing }} in an output tag', () => { + // Translators sometimes left a stray `%` right before the closing `}}` + // of a Liquid output tag, likely from copy-pasting a neighboring + // `{% ... %}` tag's closer. This breaks Liquid parsing with + // `expected "|" before filter`. Confirmed in a ko translation + // (SCRAPE: configuring-interactive-maps.md). + expect(fix('{{ allVersions[currentVersion].currentRelease %}}', 'ko')).toBe( + '{{ allVersions[currentVersion].currentRelease }}', + ) + expect(fix('{{ foo.bar %}}', 'de')).toBe('{{ foo.bar }}') + // Already-correct input is left unchanged. + expect(fix('{{ allVersions[currentVersion].currentRelease }}', 'ko')).toBe( + '{{ allVersions[currentVersion].currentRelease }}', + ) + // Doesn't touch normal `{% ... %}` tags or `{{ ... }}` output tags + // that legitimately contain a `%` character in their content. + expect(fix('{% ifversion ghes %}A{% endif %}', 'ko')).toBe('{% ifversion ghes %}A{% endif %}') + expect(fix('{{ "100%" }}', 'ko')).toBe('{{ "100%" }}') + expect(fix('{{ rate | append: "%" }}', 'ko')).toBe('{{ rate | append: "%" }}') + }) + test('strips LLM sentinel markers and preserves word boundaries', () => { expect(fix('Hello<|endoftext|>World', 'es')).toBe('Hello World') expect(fix('Hello <|endoftext|> World', 'es')).toBe('Hello World') @@ -3146,4 +3167,43 @@ Para más información, consulta "[AUTOTITLE](/path)". expect(fix('Use `{% note %}` here.', 'es')).toBe('Use `{% note %}` here.') }) }) + + describe('ja: github-token-scope-descriptions.md per-file fix', () => { + test('restores the missing endif in the security-events row', () => { + const broken = + '「シークレット スキャン アラート」のリポジトリのアクセス許可を参照してください。 |' + const fixed = + '「シークレット スキャン アラート」のリポジトリのアクセス許可を参照してください。{% endif %} |' + const ctx = { + code: 'ja', + relativePath: 'data/reusables/actions/github-token-scope-descriptions.md', + skipOrphanStripping: true, + } + expect(correctTranslatedContentStrings(broken, '', ctx)).toBe(fixed) + // already correct input is unchanged + expect(correctTranslatedContentStrings(fixed, '', ctx)).toBe(fixed) + }) + + test('restores the missing endif when scoped by dottedPath', () => { + // Production reusable rendering passes `dottedPath`, not `relativePath` + // (src/data-directory/lib/get-data.ts). + const broken = + '「シークレット スキャン アラート」のリポジトリのアクセス許可を参照してください。 |' + const fixed = + '「シークレット スキャン アラート」のリポジトリのアクセス許可を参照してください。{% endif %} |' + const ctx = { + code: 'ja', + dottedPath: 'reusables.actions.github-token-scope-descriptions', + skipOrphanStripping: true, + } + expect(correctTranslatedContentStrings(broken, '', ctx)).toBe(fixed) + expect(correctTranslatedContentStrings(fixed, '', ctx)).toBe(fixed) + }) + + test('does not affect the same text in other files', () => { + const other = + '「シークレット スキャン アラート」のリポジトリのアクセス許可を参照してください。 |' + expect(fix(other, 'ja')).toBe(other) + }) + }) }) diff --git a/src/rest/components/ApiVersionPicker.tsx b/src/rest/components/ApiVersionPicker.tsx index 173cefb55a43..16f07260386f 100644 --- a/src/rest/components/ApiVersionPicker.tsx +++ b/src/rest/components/ApiVersionPicker.tsx @@ -18,11 +18,8 @@ function rememberApiVersion(apiVersion: string) { const apiVersionNormalized = apiVersion.replace(API_VERSION_SUFFIX, '') Cookies.set(API_VERSION_COOKIE_NAME, apiVersionNormalized) } catch (err) { - // You can never be too careful because setting a cookie - // can fail. For example, some browser - // extensions disallow all setting of cookies and attempts - // at the `document.cookie` setter could throw. Just swallow - // and move on. + // Some browser extensions disallow setting cookies at all, so the + // `document.cookie` setter can throw. Swallow it and move on. console.warn('Unable to set preferred api version cookie', err) } } @@ -33,7 +30,8 @@ export const ApiVersionPicker = () => { const { allVersions } = useMainContext() const { t } = useTranslation('rest') const basePath = router.asPath.split('#')[0].split('?')[0] - // Get current date from cookie, query path, or lastly set it to latest rest version date + // Use the version from the URL when it's valid, otherwise the latest date. + // RestRedirect is what applies the cookie preference to the URL. const isValidApiVersion = (router.query.apiVersion && typeof router.query.apiVersion === 'string' && @@ -77,7 +75,7 @@ export const ApiVersionPicker = () => { }, }) - // This only shows the REST Version picker if it's calendar date versioned + // A non-empty `apiVersions` means the version is calendar-date versioned. return allVersions[currentVersion].apiVersions.length > 0 ? (
{ - // Because we have an async call to fetch, it's possible that this - // component unmounts before we perform the redirect, however, React - // will still try to perform the redirect even after the component - // is unmounted. To prevent this, we can use the AbortController signal - // to abort the Web request when the component unmounts. + // The fetch can resolve after the component unmounts and still call + // router.replace, so abort it during cleanup. const controller = new AbortController() const signal = controller.signal const { hash, pathname } = window.location - // path without a version or language const barePath = pathname .replace(`/${router.locale}`, '') .replace(`/${router.query.versionId || ''}`, '') @@ -36,19 +25,16 @@ export default function ClientSideRedirectExceptions() { sp.set('path', barePath) sp.set('hash', hash.replace(/^#/, '')) - // call the anchor-redirect endpoint to get the redirect url const response = await fetch(`/api/anchor-redirect?${sp.toString()}`, { signal, }) - // the response status will always be 200 unless there - // was a problem with the fetch request. When the - // redirect doesn't exist the json response will be empty + // A missing redirect is a 200 with an empty object, so only a + // successful response is worth parsing. if (response.ok) { const { to } = await response.json() if (to) { - // we want to redirect with the language and version in tact - // so we'll replace the full url's path and hash + // Keep the language and version, so swap only the path and hash. const fromUrl = pathname + hash const bareUrl = barePath + hash const toUrl = fromUrl.replace(bareUrl, to) diff --git a/src/rest/components/ClientSideRedirects.tsx b/src/rest/components/ClientSideRedirects.tsx index 266a1a924e4e..ad21a8adb72e 100644 --- a/src/rest/components/ClientSideRedirects.tsx +++ b/src/rest/components/ClientSideRedirects.tsx @@ -11,28 +11,17 @@ const ClientSideRedirectExceptions = dynamic( export function ClientSideRedirects() { const { asPath } = useRouter() - // We have some one-off redirects for rest api docs - // currently those are limited to the repos page, but - // that will grow soon as we restructure the rest api docs. - // This is a workaround to updating the hardcoded links - // directly in the REST API code in a separate repo, which - // requires many file changes and teams to sign off. - // While the organization is turbulent, we can do this. - // Once it's more settled, we can refactor the rest api code - // to leverage the OpenAPI urls rather than hardcoded urls. - // The code below determines if we should bother loading this redirecting - // component at all. - // The reason this isn't done at the server-level is because there you - // can't possibly access the URL hash. That's only known in client-side - // code. + // One-off redirects for the REST docs, as a workaround for fixing the + // hardcoded links in the REST API code, which lives in a separate repo and + // needs many file changes and sign-off from several teams. + // + // This decides whether to load the redirecting component at all. It can't + // happen server-side because the URL hash is only known on the client. const [load, setLoad] = useState(false) useEffect(() => { const { hash } = window.location - // Today, Jan 2022, it's known explicitly what the pathname. - // In the future there might be more. - // Hopefully, we can some day delete all of this and no longer - // be dependent on the URL hash to do the redirect. + // Only /rest has these redirects today. More paths may need adding. if (hash && asPath.startsWith('/rest')) { setLoad(true) } diff --git a/src/rest/components/RestAuth.tsx b/src/rest/components/RestAuth.tsx index 0f15102cf675..3eabc959c6cf 100644 --- a/src/rest/components/RestAuth.tsx +++ b/src/rest/components/RestAuth.tsx @@ -29,8 +29,7 @@ export function RestAuth({ progAccess, slug, operationTitle }: Props) { if (currentVersion === 'enterprise-server@3.9' || currentVersion === 'enterprise-server@3.8') return null - // There are some operations that have no progAccess access defined - // For those operations, we shouldn't display this component + // Some operations define no progAccess at all. if (!progAccess) return null const { userToServerRest, @@ -112,8 +111,6 @@ function FineGrainedAccess({ progAccess }: FineGrainedProps) { basePath += `/${currentVersion}` } - // Pluralize the message if needed or customize it - // when no permissions are defined const numPermissionSets = progAccess.permissions.length const permissionMsg = numPermissionSets === 0 diff --git a/src/rest/components/RestBanner.tsx b/src/rest/components/RestBanner.tsx index 4e864c23a3fb..abb6fbf75f8b 100644 --- a/src/rest/components/RestBanner.tsx +++ b/src/rest/components/RestBanner.tsx @@ -36,7 +36,8 @@ const restRepoCategoryExceptionsTitles = { export const RestBanner = () => { const router = useRouter() const { t } = useTranslation('rest') - // Having a productId === 'rest' and no router.query.category would mean a product landing page like http://docs.github.com/en/rest?apiVersion=2022-08-09 + // A productId of 'rest' with no category is the product landing page, e.g. + // /en/rest?apiVersion=2022-08-09. const isRestPage = router.query.productId === 'rest' || router.query.category const restPage = router.query.category as string const { currentVersion } = useVersion() diff --git a/src/rest/components/RestCodeSamples.tsx b/src/rest/components/RestCodeSamples.tsx index 257292f23017..a0c205a7d8f3 100644 --- a/src/rest/components/RestCodeSamples.tsx +++ b/src/rest/components/RestCodeSamples.tsx @@ -48,7 +48,6 @@ export function RestCodeSamples({ operation, slug, heading }: Props) { const { currentVersion } = useVersion() const { allVersions } = useMainContext() - // Get format examples for each language const languageExamples = operation.codeExamples.map((sample) => ({ description: sample.request.description, curl: getShellExample(operation, sample, currentVersion, allVersions), @@ -58,7 +57,6 @@ export function RestCodeSamples({ operation, slug, heading }: Props) { request: sample.request, })) - // Menu options for the language selector const languageSelectOptions: CodeSampleKeys[] = [CodeSampleKeys.curl] // Management Console and GHES Manage API operations are not supported @@ -73,7 +71,6 @@ export function RestCodeSamples({ operation, slug, heading }: Props) { } } - // Menu options for the example selector const exampleSelectOptions = generateExampleOptions(languageExamples) const [selectedLanguage, setSelectedLanguage] = useState(languageSelectOptions[0]) @@ -96,7 +93,6 @@ export function RestCodeSamples({ operation, slug, heading }: Props) { Cookies.set(CODE_SAMPLE_LANGUAGE_COOKIE_NAME, languageKey) } - // Change the language based on cookies useEffect(() => { // If the user previously selected a language preference and the language // is available in this component set it as the selected language @@ -145,7 +141,6 @@ export function RestCodeSamples({ operation, slug, heading }: Props) {

{isSingleExample ? t('request_example') : t('request_examples')}

- {/* Display an example selector if more than one example */} {!isSingleExample && (
@@ -204,7 +199,6 @@ export function RestCodeSamples({ operation, slug, heading }: Props) {
- {/* Example requests */}
) : null}
- {/* Status code */} {displayedExample.response.statusCode && (
{`Status: ${displayedExample.response.statusCode}`}
)} - {/* Example response */} {displayedExample.response.example && (
{ - // Convert numeric strings back to numbers for API compatibility if (typeof value === 'string' && /^\d+$/.test(value)) { return parseInt(value, 10) } - // Convert boolean strings to actual booleans if (value === 'true') return true if (value === 'false') return false return value @@ -256,7 +234,6 @@ type NestedObjectParameter = | { [key: string]: NestedObjectParameter } | NestedObjectParameter[] -// Helper function to detect if an object has nested arrays function hasNestedArrays(obj: NestedObjectParameter): boolean { if (Array.isArray(obj)) { return true @@ -284,13 +261,11 @@ function handleSingleParameter( separator = '' } if (typeof value === 'string') { - // Escape single quotes in string values to prevent shell command issues with contractions const escapedValue = escapeShellValue(value) cliLine += ` -f '${keyString}${separator}${escapedValue}'` } else if (typeof value === 'number' || typeof value === 'boolean' || value === null) { cliLine += ` -F "${keyString}${separator}${value}"` } else if (Array.isArray(value)) { - // For simple arrays, use individual parameters with indices for (let i = 0; i < value.length; i++) { const param = value[i] if (Array.isArray(param)) { @@ -303,7 +278,7 @@ function handleSingleParameter( (nextKey: string): string => `${keyString}[${i}]${nextKey}`, ) } else { - // Transform key in this case needs to account for the `key` being passed in and use array index + // The transform has to fold the array index into the passed-in key. const arrayTransform = () => `${transformKey(key)}[${i}]` cliLine += handleSingleParameter(key, param, arrayTransform) } @@ -360,26 +335,13 @@ function handleObjectParameter( return cliLine } -/* - Generates an octokit.js example - - For example: - await octokit.request('POST /repos/{owner}/{repo}/deployments'{ - "owner": "OWNER", - "repo": "REPO", - "ref": "topic-branch", - "payload": "{ \"deploy\": \"migrate\" }", - "description": "Deploy request from hubot" - }) - -*/ +// Generates an octokit.js example for one code sample. export function getJSExample( operation: Operation, codeSample: CodeSample, currentVersion: string, allVersions: Record, ) { - // Check if we should omit authentication for this operation const omitAuth = shouldOmitAuthentication(operation, currentVersion) const parameters: { [key: string]: string | object } = {} diff --git a/src/rest/lib/code-example-utils.ts b/src/rest/lib/code-example-utils.ts index aa949f70705c..e828a55cded5 100644 --- a/src/rest/lib/code-example-utils.ts +++ b/src/rest/lib/code-example-utils.ts @@ -1,11 +1,6 @@ -/** - * Interface for processed code examples as used by the RestCodeSamples component. - * This represents the structure AFTER the component processes the original API data, - * where sample.request.description is moved to the top level as description. - * - * Original API structure: { request: { description: string, contentType: string }, response: {...} } - * Processed structure: { description: string, request: { contentType: string }, response: {...} } - */ +// The part of a code example these label helpers need. RestCodeSamples copies +// `sample.request.description` up to a top-level `description` and keeps the +// original `request` object as-is. export interface CodeExample { request?: { contentType?: string @@ -21,9 +16,6 @@ export interface ExampleOption { languageIndex: number } -/** - * Determines if request content types differ between examples - */ export function shouldShowRequestContentType(examples: CodeExample[]): boolean { return ( examples.length > 1 && @@ -31,9 +23,6 @@ export function shouldShowRequestContentType(examples: CodeExample[]): boolean { ) } -/** - * Determines if response content types differ between examples - */ export function shouldShowResponseContentType(examples: CodeExample[]): boolean { return ( examples.length > 1 && @@ -41,10 +30,7 @@ export function shouldShowResponseContentType(examples: CodeExample[]): boolean ) } -/** - * Generates example option objects with appropriate content type labels - * This matches the exact logic from RestCodeSamples.tsx - */ +// Labels each example option with whichever content types vary across the set. export function generateExampleOptions(examples: CodeExample[]): ExampleOption[] { const responseContentTypesDiffer = shouldShowResponseContentType(examples) const requestContentTypesDiffer = shouldShowRequestContentType(examples) @@ -58,13 +44,10 @@ export function generateExampleOptions(examples: CodeExample[]): ExampleOption[] if (showExampleOptionMediaType) { if (requestContentTypesDiffer && responseContentTypesDiffer) { - // Show both request and response content types text = `${example.description} (${requestContentType} → ${responseContentType})` } else if (requestContentTypesDiffer) { - // Show only request content type text = `${example.description} (${requestContentType})` } else if (responseContentTypesDiffer) { - // Show only response content type text = `${example.description} (${responseContentType})` } } @@ -76,9 +59,6 @@ export function generateExampleOptions(examples: CodeExample[]): ExampleOption[] }) } -/** - * Generates just the text labels for example options (useful for testing) - */ export function generateExampleOptionTexts(examples: CodeExample[]): string[] { return generateExampleOptions(examples).map((option) => option.text) } diff --git a/src/rest/lib/config.ts b/src/rest/lib/config.ts index a1bdb88dd53b..d264723a9105 100644 --- a/src/rest/lib/config.ts +++ b/src/rest/lib/config.ts @@ -1,8 +1,6 @@ -// This separate file from config.json was necessary in order to load -// static information into our client-side React components. But, -// we also need a JSON file to have a format with static data that can be -// written to automatically. The sync scripts for REST update information -// in the JSON file. +// Separate from config.json because client-side React components need to +// import static values, while the REST sync scripts need a JSON file they can +// write to. // These paths must match the paths in src/pages/[versionId]/rest export const nonAutomatedRestPaths: readonly string[] = [ @@ -13,7 +11,5 @@ export const nonAutomatedRestPaths: readonly string[] = [ '/rest/guides', ] as const -// This path is used to set the page in the -// src/rest/components/ApiVersionPicker.tsx component. That component -// has a link to the page that describes what api versioning is. +// ApiVersionPicker links here to explain what API versioning is. export const apiVersionPath: string = '/rest/about-the-rest-api/api-versions' diff --git a/src/rest/lib/index.ts b/src/rest/lib/index.ts index bf56ddb37467..d76b96d734f5 100644 --- a/src/rest/lib/index.ts +++ b/src/rest/lib/index.ts @@ -13,7 +13,6 @@ import type { Operation } from '@/rest/components/types' export const REST_DATA_DIR = 'src/rest/data' const REST_CONTENT_DIR = 'content/rest' -// Type definitions for REST operations export interface RestOperationCategory { [subcategory: string]: Operation[] } @@ -22,33 +21,9 @@ interface RestMiniTocData { restOperationsMiniTocItems: MiniTocItem[] } -/* - Loads the schemas from the static/decorated folder into a single - object organized by version. Not all products are calendar date - versioned. - Example: - { - free-pro-team@latest: { - 2022-08-09: { - category: { - subcategory: [operations], - } - }, - 2022-11-14: { - category: { - subcategory: [operations], - } - } - } - enterprise-server@3.2: { - 'not_api_versioned': { - category: { - subcategory: [operations], - } - } - } - } -*/ +// Caches generated mini-TOC data, keyed by language, then docs version, then +// API date, then category, then subcategory. A version with no calendar dates +// uses `not_api_versioned` in place of a date. const NOT_API_VERSIONED = 'not_api_versioned' const brotliDecompressAsync = promisify(brotliDecompress) const restOperationData = new Map< @@ -60,7 +35,7 @@ const restOperationData = new Map< // they account for >90% of traffic and each version needs ~100 slots alone. // All other versions (ghes) go into a bounded LRU cache. const PINNED_OPEN_API_VERSIONS = new Set(['fpt', 'ghec']) -export const pinnedCache = new Map() // @internal — stores deflate-compressed JSON +export const pinnedCache = new Map() // @internal, stores deflate-compressed JSON const LRU_MAX_SIZE = Math.max(1, parseInt(process.env.REST_SCHEMA_LRU_SIZE ?? '', 10) || 96) export const lruCache = new QuickLRU({ maxSize: LRU_MAX_SIZE }) // @internal @@ -70,15 +45,13 @@ const inflight = new Map>() for (const language of Object.keys(languages)) { restOperationData.set(language, new Map()) for (const version of Object.keys(allVersions)) { - // setting to undefined will allow us to perform checks - // more easily later on restOperationData.get(language)!.set(version, new Map()) if (allVersions[version].apiVersions && allVersions[version].apiVersions.length > 0) { for (const date of allVersions[version].apiVersions) { restOperationData.get(language)!.get(version)!.set(date, new Map()) } } else { - // Products that are not been calendar date versioned + // Products that are not calendar-date versioned. restOperationData.get(language)!.get(version)!.set(NOT_API_VERSIONED, new Map()) } } @@ -91,10 +64,10 @@ export const categoriesWithoutSubcategories: string[] = fs }) .map((filteredFile: string) => filteredFile.replace('.md', '')) -// version: plan + release e.g. For ghes-3.5, ghes is the plan and 3.5 is the release -// apiVersion (not all versions have apiVersions): REST API Calendar Dates -// openApiVersion (below, every version has an openApiVersion mapping): There's a mapping between our Docs versions -// and the OpenApi Version bc it's not the same +// version: a docs version, e.g. `enterprise-server@3.5`. +// apiVersion: a REST API calendar date. Not every version has these. +// openApiVersion: the matching OpenAPI name, e.g. `ghes-3.5`. Every docs +// version maps to one, because the two naming schemes differ. export default async function getRest( version: string, @@ -151,7 +124,7 @@ async function loadCategoryFile(basePath: string): Promise = async (context) => const categoryData = await getRest(currentVersion, apiVersion, category) const restOperations = (categoryData && categoryData[subcategory]) || [] - // Build table of contents for all category operations for TocLanding: - // - // * get all operations for a category (will be broken up by subcategory) - // * loop over subcategories and get the operations per subcategory - // * get the minitoc items per set of subcategory operations - // * with this data, build a collection of toc items that can be used by TocLanding + // Build the TocLanding table of contents for every operation in the category. + // The operations come back grouped by subcategory, so walk the subcategories, + // take the minitoc items for each one's operations, and collect them. const restCategoryOperations = categoryData || {} const restCategoryTocItems = [] @@ -182,12 +179,8 @@ export const getServerSideProps: GetServerSideProps = async (context) => // content/rest/* const { miniTocItems } = getAutomatedPageContextFromRequest(req) - // When operations exist, update the miniTocItems in the article context - // with the list of operations in the OpenAPI. - - // The context passed will have the Markdown content for the language - // of the page being requested and the Markdown will be rendered - // using the `currentVersion` + // Build mini-TOC items from the operation titles, using the request context + // for the language and version, and append them to the article's mini-TOC. if (restOperations) { const { restOperationsMiniTocItems } = (await getRestMiniTocItems( category, @@ -204,8 +197,6 @@ export const getServerSideProps: GetServerSideProps = async (context) => } } - // Replace the toc items in the context with the REST toc items we just - // created. tocLandingContext.tocItems = restCategoryTocItems const mainContext = await getMainContext(req, res as unknown as Response) diff --git a/src/rest/pages/subcategory.tsx b/src/rest/pages/subcategory.tsx index 773ae3913861..eacb232bbb04 100644 --- a/src/rest/pages/subcategory.tsx +++ b/src/rest/pages/subcategory.tsx @@ -66,12 +66,8 @@ export const getServerSideProps: GetServerSideProps = async (context) => // content/rest/* const { miniTocItems } = getAutomatedPageContextFromRequest(req) - // When operations exist, update the miniTocItems in the article context - // with the list of operations in the OpenAPI. - - // The context passed will have the Markdown content for the language - // of the page being requested and the Markdown will be rendered - // using the `currentVersion` + // Build mini-TOC items from the operation titles, using the request context + // for the language and version, and append them to the article's mini-TOC. if (restOperations) { const { restOperationsMiniTocItems } = (await getRestMiniTocItems( category, diff --git a/src/rest/scripts/openapi-check.ts b/src/rest/scripts/openapi-check.ts index 15d9f0c71530..bd5c06897e73 100755 --- a/src/rest/scripts/openapi-check.ts +++ b/src/rest/scripts/openapi-check.ts @@ -42,9 +42,7 @@ async function check(files: string[]): Promise { for (const [filename, schema] of documents as [string, unknown][]) { try { - // munge OpenAPI definitions object in an array of operations objects const operations = await createOperations(schema as SchemaInput) - // process each operation, asynchronously rendering markdown and stuff await processOperations(operations, {}) console.log(`Successfully could decorate OpenAPI operations for document ${filename}`) diff --git a/src/rest/scripts/test-open-api-schema.ts b/src/rest/scripts/test-open-api-schema.ts index 59472c67f46b..d85871c03f93 100755 --- a/src/rest/scripts/test-open-api-schema.ts +++ b/src/rest/scripts/test-open-api-schema.ts @@ -23,13 +23,12 @@ type ErrorMessages = Record { const contentFiles = getAutomatedMarkdownFiles('content/rest') - // Creating the categories/subcategories based on the current content directory const checkContentDir = await createCheckContentDirectory(contentFiles) - // Create categories/subcategories from OpenAPI Schemas const openAPISchemaCheck = await createOpenAPISchemasCheck() - // Get Differences between categories/subcategories from dereferenced schemas and the content/rest directory frontmatter versions + // Compare the categories and subcategories in the dereferenced schemas + // against the versions in the content/rest frontmatter. const differences = getDifferences(openAPISchemaCheck, checkContentDir) const errorMessages: ErrorMessages = {} diff --git a/src/rest/scripts/update-files.ts b/src/rest/scripts/update-files.ts index 93dbd77136aa..ea40a5f10057 100755 --- a/src/rest/scripts/update-files.ts +++ b/src/rest/scripts/update-files.ts @@ -185,14 +185,13 @@ async function getBundledFiles(): Promise { execSync('git pull', { cwd: GITHUB_REP_DIR }) } - // Create a tmp directory to store schema files generated from github/github await rimraf(TEMP_OPENAPI_DIR) await mkdirp(TEMP_BUNDLED_OPENAPI_DIR) console.log( `\n🏃‍♀️🏃🏃‍♀️Running \`bin/openapi bundle\` in branch '${githubBranch}' of your github/github checkout to generate the dereferenced OpenAPI schema files.\n`, ) - // Format the command supplied to the bundle script in `github/github` + // Build the command for the bundle script in `github/github`. const bundlerOptions = await getBundlerOptions() const bundleCommand = `bundle -v -w${ next ? ' -n' : '' @@ -239,7 +238,6 @@ async function validateInputParameters(): Promise { throw new Error(errorMsg) } - // Check that the source repo exists. for (const sourceRepoDirectory of sourceRepoDirectories) { if (!existsSync(sourceRepoDirectory)) { const errorMsg = @@ -255,13 +253,9 @@ async function validateInputParameters(): Promise { } } -// Version names in the data consumed by the docs site varies depending on the -// team that owns the data we consume. This function translates the version -// names to use the names in the src//lib/config.json file. -// The names in the config.json file maps the incoming version name to -// the short name of the version defined in lib/allVersions.ts. -// This function also translates calendar-date format from .2022-11-28 to -// -2022-11-28 +// Version names in the incoming data vary by the team that owns it. This +// renames the files using the versionMapping in src/rest/lib/config.json, and +// rewrites a calendar date suffix from .2022-11-28 to -2022-11-28. export async function normalizeDataVersionNames(sourceDirectory: string): Promise { const schemas = await readdir(sourceDirectory) @@ -279,10 +273,8 @@ export async function normalizeDataVersionNames(sourceDirectory: string): Promis // Match a calendar version if it exists, e.g., .2022-11-28 const regex = /.\d{4}-\d{2}-\d{2}/ const matches = baseName.match(regex) - // Separate the version name from the calendar date version const versionName = matches ? docsBaseName.replace(matches[0], '') : docsBaseName const calendarSuffix = matches ? matches[0].replace('.', '-') : '' - // Build the new version name const translatedVersion = `${versionName}${calendarSuffix}.json` await rename(path.join(sourceDirectory, schema), path.join(sourceDirectory, translatedVersion)) } diff --git a/src/rest/scripts/utils/create-rest-examples.ts b/src/rest/scripts/utils/create-rest-examples.ts index ac8114b4606e..00d2d3bb173d 100644 --- a/src/rest/scripts/utils/create-rest-examples.ts +++ b/src/rest/scripts/utils/create-rest-examples.ts @@ -105,7 +105,7 @@ export default async function getCodeSamples( }) } - // Strip the key field — it's only needed during merging, not at runtime + // The key is only needed while merging, not at runtime. for (const example of mergedExamples) delete (example as { key?: string }).key return mergedExamples } @@ -163,13 +163,10 @@ export function mergeExamples( const target = requestsExamplesLarger ? requestExamples : responseExamples const source = requestsExamplesLarger ? responseExamples : requestExamples - // Iterates over the larger array or "target" (or if equal requests) to see - // if there are any matches in the smaller array or "source" - // (or if equal responses) that can be added to target array. If a request - // If a request - // example and response example have matching keys they will be merged into - // an example. If there is more than one key match, the first match will - // be used. + // Walk the longer array ("target", or the requests when the two are equal + // length) looking for a matching key in the other one ("source"). A request + // and a response with the same key are merged into one example. If several + // keys match, the first one wins. return target .filter((targetEx) => { const match = source.find((srcEx) => srcEx.key === targetEx.key) @@ -179,23 +176,10 @@ export function mergeExamples( .map((ex) => ex as MergedExample) } -/* - Create an example object for each example in the requestBody property - of the schema. Each requestBody can have more than one content type. - Each content type can have more than one example. We create an object - for each permutation of content type and example. - Returns an array of objects in the format: - { - key, - request: { - contentType, - description, - acceptHeader, - bodyParameters, - parameters, - } - } -*/ +// Builds request examples from the media types in the operation's requestBody, +// falling back to a path-parameter or generic example when there is no body +// example. Every result has a key plus a request with description and +// acceptHeader; contentType, bodyParameters and parameters are optional. export function getRequestExamples(operation: CodeSampleOperation): RequestExample[] { const requestExamples: RequestExample[] = [] const parameterExamples = getParameterExamples(operation) @@ -288,12 +272,9 @@ export function getRequestExamples(operation: CodeSampleOperation): RequestExamp return requestExamples } -/* - Recursively removes `example` and `examples` annotation fields from a JSON - Schema object. These fields are OpenAPI annotation-only and are never read - by the runtime rendering code, but they account for ~131 MB of the total - schema.json size across all versions. -*/ +// Recursively removes the `example` and `examples` annotation fields from a +// JSON Schema object. Nothing at runtime reads them, and they account for +// ~131 MB of the total schema.json size across all versions. function stripSchemaExamples(schema: unknown): unknown { if (!schema || typeof schema !== 'object') return schema if (Array.isArray(schema)) return schema.map(stripSchemaExamples) @@ -306,23 +287,9 @@ function stripSchemaExamples(schema: unknown): unknown { return result } -/* - Create an example object for each example in the response property - of the schema. Each response can have more than one status code, - each with more than one content type. And each content type can - have more than one example. We create an object - for each permutation of status, content type, and example. - Returns an array of objects in the format: - { - key, - response: { - statusCode, - contentType, - description, - example, - } - } -*/ +// Builds examples for the operation's responses below status 400. Every result +// has a key plus a response with statusCode and description; contentType, +// example and schema are only present when the media type had an example. export function getResponseExamples(operation: CodeSampleOperation): ResponseExample[] { const responseExamples: ResponseExample[] = [] const responses = operation.responses as Record @@ -415,17 +382,12 @@ export function getResponseExamples(operation: CodeSampleOperation): ResponseExa return responseExamples } -/* - Path parameters can have more than one example key. We need to create - an example for each and then choose the most appropriate example when - we merge requests with responses. - Parameter examples are in the format: - { - [parameter key]: { - [parameter name]: value - } - } -*/ +// Groups the operation's path parameter values by example key, in the shape: +// +// { [example key]: { [parameter name]: value } } +// +// A parameter with no examples contributes its uppercased name under the +// `default` key. export function getParameterExamples( operation: CodeSampleOperation, ): Record> { diff --git a/src/rest/scripts/utils/get-body-params.ts b/src/rest/scripts/utils/get-body-params.ts index 87bd27cb8a10..7dd90544c57b 100644 --- a/src/rest/scripts/utils/get-body-params.ts +++ b/src/rest/scripts/utils/get-body-params.ts @@ -75,8 +75,7 @@ async function getTopLevelOneOfProperty( return { properties, required } } -// Gets the body parameters for a given schema recursively. -// Helper function to handle oneOf fields where all items are objects +// Handles a oneOf whose items are all objects. Returns [] for anything else. async function handleObjectOnlyOneOf( param: Schema, paramType: string[], @@ -89,6 +88,7 @@ async function handleObjectOnlyOneOf( return [] } +// Gets the body parameters for a schema, recursively. export async function getBodyParams(schema: Schema, topLevel = false): Promise { const bodyParametersParsed: TransformedParam[] = [] const schemaObject = schema.oneOf && topLevel ? await getTopLevelOneOfProperty(schema) : schema @@ -133,15 +133,13 @@ export async function getBodyParams(schema: Schema, topLevel = false): Promise 0) { childParamsGroups.push(...oneOfChildren) } else { - // Handle mixed types or non-object oneOf cases const descriptions: { type: string; description: string }[] = [] for (const childParam of param.oneOf) { paramType.push( @@ -287,8 +283,8 @@ async function getTransformedParam( paramDecorated.isRequired = true } if (childParamsGroups && childParamsGroups.length > 0 && !param.oneOfObject) { - // Since the allOf properties can have multiple duplicate properties we want to get rid of the duplicates with the same name, but keep the - // the one that has isRequired set to true. + // allOf can contribute the same property more than once. Drop the + // duplicates by name, keeping whichever one has isRequired set. const mergedChildParamsGroups = Array.from( childParamsGroups .reduce((childParam, obj) => { diff --git a/src/rest/scripts/utils/get-openapi-schemas.ts b/src/rest/scripts/utils/get-openapi-schemas.ts index 513a941103a6..39cc53fe2e4c 100644 --- a/src/rest/scripts/utils/get-openapi-schemas.ts +++ b/src/rest/scripts/utils/get-openapi-schemas.ts @@ -8,11 +8,10 @@ const OPEN_API_RELEASES_DIR = '../github/app/api/description/config/releases' const configData: { versionMapping: Record } = JSON.parse( await readFile('src/rest/lib/config.json', 'utf8'), ) -// Gets the full list of unpublished + active, deprecated + active, -// or active schemas from the github/github repo -// `openApiReleaseDir` is the path to the `app/api/description/config/releases` -// directory in `github/github` -// You can also specify getting specific versions of schemas. +// Reads the release YAML files in `directory`, which points at +// app/api/description/config/releases in github/github, and returns the +// generated schema filenames split into currentReleases, unpublished and +// deprecated. export async function getSchemas( directory: string = OPEN_API_RELEASES_DIR, ): Promise<{ currentReleases: string[]; unpublished: string[]; deprecated: string[] }> { @@ -68,7 +67,6 @@ export async function getSchemas( export async function validateVersionsOptions(versions: string[]): Promise { const schemas = await getSchemas() - // Validate individual versions provided for (const version of versions) { if ( schemas.deprecated.includes(`${version}.deref.json`) || diff --git a/src/rest/scripts/utils/get-operations.ts b/src/rest/scripts/utils/get-operations.ts index bf02d21536e7..3449e72d29d1 100644 --- a/src/rest/scripts/utils/get-operations.ts +++ b/src/rest/scripts/utils/get-operations.ts @@ -7,9 +7,8 @@ interface ProgAccessData { export type SchemaInput = OpenApiSchema -// The module accepts a JSON schema object as input -// and returns an array of its operation objects with their -// HTTP verb and requestPath attached as properties +// Runs `process` on every operation with the programmatic access data, then +// returns the same array. export async function processOperations( operations: Operation[], progAccessData: ProgAccessData, diff --git a/src/rest/scripts/utils/get-redirects.ts b/src/rest/scripts/utils/get-redirects.ts index 1c03865913a5..b70039aa4591 100644 --- a/src/rest/scripts/utils/get-redirects.ts +++ b/src/rest/scripts/utils/get-redirects.ts @@ -18,8 +18,7 @@ interface RedirectMap { [oldUrl: string]: string } -// This is way to add redirects from one fragment to another from the -// client's browser. +// Adds redirects from one URL fragment to another, applied in the browser. export async function syncRestRedirects(): Promise { const clientSideRedirects = await getClientSideRedirects() diff --git a/src/rest/scripts/utils/normalize-docs-urls.ts b/src/rest/scripts/utils/normalize-docs-urls.ts index 703a30fab153..0274ab661ca4 100644 --- a/src/rest/scripts/utils/normalize-docs-urls.ts +++ b/src/rest/scripts/utils/normalize-docs-urls.ts @@ -1,7 +1,6 @@ -// Normalize double slashes that appear in GHEC docs URLs. -// The upstream OpenAPI spec in github/github contains URLs like -// "enterprise-cloud@latest//rest/..." — the extra slash is harmless -// in browsers but triggers CCR lint errors. +// Normalizes the double slashes in GHEC docs URLs. The upstream OpenAPI spec in +// github/github contains URLs like "enterprise-cloud@latest//rest/...". The +// extra slash is harmless in browsers but trips CCR lint errors. const DOUBLE_SLASH_RE = /(docs\.github\.com\/[^/]+@[^/]+)\/\//g export function normalizeDocsUrls(html: string): string { diff --git a/src/rest/scripts/utils/operation.ts b/src/rest/scripts/utils/operation.ts index c0d83a334e75..71432b186e59 100644 --- a/src/rest/scripts/utils/operation.ts +++ b/src/rest/scripts/utils/operation.ts @@ -187,7 +187,6 @@ export default class Operation { // Operation Id: markdown/render-raw const contentType = Object.keys(this.#operation.requestBody.content)[0] const schema = get(this.#operation, `requestBody.content.${contentType}.schema`, {}) - // Merges any instances of allOf in the schema using a deep merge const mergedAllofSchema = mergeAllOf(schema as Parameters[0]) try { this.bodyParameters = isPlainObject(schema) diff --git a/src/rest/scripts/utils/sync-changelogs.ts b/src/rest/scripts/utils/sync-changelogs.ts index a733757e5b45..c07b17f809da 100644 --- a/src/rest/scripts/utils/sync-changelogs.ts +++ b/src/rest/scripts/utils/sync-changelogs.ts @@ -111,7 +111,6 @@ export function parseVersionSections(markdown: string): VersionSection[] { // Skip intro paragraph lines before the first ## Version heading const versionMatch = line.match(/^## Version (\d{4}-\d{2}-\d{2})/) if (versionMatch) { - // Save previous section if any if (currentVersion) { sections.push({ version: currentVersion, @@ -129,7 +128,6 @@ export function parseVersionSections(markdown: string): VersionSection[] { } } - // Save last section if (currentVersion) { sections.push({ version: currentVersion, diff --git a/src/rest/scripts/utils/sync.ts b/src/rest/scripts/utils/sync.ts index ecc9e2907494..bb8de5013a53 100644 --- a/src/rest/scripts/utils/sync.ts +++ b/src/rest/scripts/utils/sync.ts @@ -99,10 +99,9 @@ export async function syncRestData( await updateRestConfigData(restSchemas) } -// After syncing, remove any .json category files on disk that were not -// written during this run. This handles the case where an entire API -// category is removed upstream — without this cleanup, stale data files -// would persist and continue to generate docs pages. +// After syncing, removes every .json file in each version directory that this +// run didn't write. Without it, a category removed upstream would leave stale +// data files behind that keep generating docs pages. export async function removeStaleRestDataFiles( writtenFilesByVersion: Map>, ): Promise { @@ -152,14 +151,12 @@ async function formatRestData(operations: Operation[]): Promise { const restConfigFilename = 'src/rest/lib/config.json' const restConfigData = JSON.parse(await readFile(restConfigFilename, 'utf8')) as Record< @@ -168,9 +165,9 @@ async function updateRestConfigData(schemas: string[]): Promise { > const restApiVersionData = (restConfigData['api-versions'] as Record) || {} - // Phase 1: Collect the dates present in the incoming schemas, keyed by - // OpenAPI version name. Only calendar-date schemas contribute — those that - // don't exactly match a base OPENAPI_VERSION_NAMES entry but do start with one. + // Phase 1: collect the dates in the incoming schemas, keyed by OpenAPI + // version name. Only calendar-date schemas count, meaning the ones that start + // with an OPENAPI_VERSION_NAMES entry without exactly matching it. const incomingDates: Record> = {} for (const schema of schemas) { diff --git a/src/rest/scripts/utils/update-markdown.ts b/src/rest/scripts/utils/update-markdown.ts index 75b46dcf8b17..64162259248d 100644 --- a/src/rest/scripts/utils/update-markdown.ts +++ b/src/rest/scripts/utils/update-markdown.ts @@ -48,13 +48,7 @@ export async function updateRestFiles() { }) } -// Reads data files from the directory provided and returns a -// JSON object that lists the versions for each category/subcategory -/** - * Extract GHES version from a file path if it's a GHES directory - * @param {string} filePath - File path to parse - * @returns {string|null} - GHES version or null if not a GHES file - */ +// The GHES version in a file path, or null if the path isn't a GHES one. export function getGHESVersionFromFilepath(filePath: string): string | null { // Normalize path separators to handle both Unix and Windows paths const normalizedPath = filePath.replace(/\\/g, '/') @@ -65,7 +59,6 @@ export function getGHESVersionFromFilepath(filePath: string): string | null { return null } - // Extract version from ghes-X.Y or ghes-X.Y-YYYY-MM-DD format const versionMatch = ghesDir.match(/^ghes-(\d+\.\d+)/) return versionMatch ? versionMatch[1] : null } @@ -78,10 +71,9 @@ async function getDataFrontmatter(dataDirectory: string): Promise // Exclude non-category JSON files that live alongside per-category data. // If new non-category files are added to version directories, update this filter. .filter((file) => !file.endsWith('client-side-rest-api-redirects.json')) - // Exclude the legacy monolithic schema files. These are retained on disk as a - // safety net until the cleanup PR removes them, but they must not be read here — - // their top-level keys are category names, not subcategories, so they would be - // misread as a bogus "schema" category. + // Exclude any legacy monolithic schema files. Their top-level keys are + // category names, not subcategories, so they would be misread here as a + // bogus "schema" category. .filter((file) => !file.endsWith('schema.json')) // Ignore any deprecated versions. This allows us to stop supporting // the most recent deprecated version but still allow data to exist. @@ -117,26 +109,16 @@ async function getDataFrontmatter(dataDirectory: string): Promise return restVersions } -/* - Take an object that includes the version frontmatter - that should be applied to the Markdown page that corresponds - to the category and subcategory. The format looks like this: - { - "actions": { - "artifacts": { - "versions": { - "free-pro-team@latest", - "enterprise-cloud@latest", - "enterprise-server@3.4", - "enterprise-server@3.5", - "enterprise-server@3.6", - "enterprise-server@3.7", - "enterprise-server@3.8" - } - } - } - } -*/ +// Takes the version frontmatter to apply to the Markdown page for each category +// and subcategory, in the shape: +// +// { +// "actions": { +// "artifacts": { +// "versions": ["free-pro-team@latest", "enterprise-server@3.8", ...] +// } +// } +// } async function getMarkdownContent(versions: RestVersions): Promise { const markdownUpdates: MarkdownUpdates = {} diff --git a/src/rest/tests/api.ts b/src/rest/tests/api.ts index 8d65269d2b62..a1a1a07a2409 100644 --- a/src/rest/tests/api.ts +++ b/src/rest/tests/api.ts @@ -11,7 +11,6 @@ describe('anchor-redirect api', () => { ) test('returns correct redirect to url', async () => { - // test the first entry const [key, value] = Object.entries(clientSideRedirects)[0] const [path, hash] = key.split('#') const sp = new URLSearchParams() @@ -23,7 +22,6 @@ describe('anchor-redirect api', () => { expect(to).toBe(value) }) test('errors when path is not passed', async () => { - // test the first entry const key = Object.keys(clientSideRedirects)[0] const hash = key.split('#')[1] const sp = new URLSearchParams() @@ -32,7 +30,6 @@ describe('anchor-redirect api', () => { expect(res.statusCode).toBe(400) }) test('errors when path is not passed', async () => { - // test the first entry const key = Object.keys(clientSideRedirects)[0] const path = key.split('#')[0] const sp = new URLSearchParams() diff --git a/src/rest/tests/cli-examples.ts b/src/rest/tests/cli-examples.ts index 2f3b99f844a7..bd07ac94569d 100644 --- a/src/rest/tests/cli-examples.ts +++ b/src/rest/tests/cli-examples.ts @@ -11,7 +11,6 @@ describe('CLI examples generation', () => { serverUrl: 'https://api.github.com', subcategory: 'code-scanning', parameters: [], - // Partial mock object for testing - 'as unknown as' bypasses strict type checking for missing properties } as unknown as Operation const mockVersions = { @@ -19,7 +18,6 @@ describe('CLI examples generation', () => { apiVersions: ['2022-11-28'], latestApiVersion: '2022-11-28', }, - // Partial mock object for testing - 'as unknown as' bypasses strict type checking for missing properties } as unknown as Record test('GitHub CLI example properly escapes contractions in string values', () => { @@ -37,14 +35,11 @@ describe('CLI examples generation', () => { "This alert is not actually correct, because there's a sanitizer included in the library.", }, }, - // Partial mock object for testing - 'as unknown as' bypasses strict type checking for missing properties } as unknown as CodeSample const result = getGHExample(mockOperation, codeSample, 'free-pro-team@latest', mockVersions) - // Check that the contraction is properly escaped expect(result).toContain("there'\\''s") - // Ensure the command is properly formatted expect(result).toContain('gh api') expect(result).toContain('--method PATCH') expect(result).toContain("-f 'dismissed_comment=") @@ -66,12 +61,10 @@ describe('CLI examples generation', () => { }, contentType: 'application/json', }, - // Partial mock object for testing - 'as unknown as' bypasses strict type checking for missing properties } as unknown as CodeSample const result = getShellExample(mockOperation, codeSample, 'free-pro-team@latest', mockVersions) - // Check that the JSON string is properly escaped expect(result).toContain("there'\\''s") expect(result).toContain('curl -L') expect(result).toContain('-X PATCH') @@ -89,7 +82,6 @@ describe('CLI examples generation', () => { body: "It's not working because there's an issue and we can't fix it", }, }, - // Partial mock object for testing - 'as unknown as' bypasses strict type checking for missing properties } as unknown as CodeSample const mockSimpleOperation = { @@ -98,7 +90,6 @@ describe('CLI examples generation', () => { serverUrl: 'https://api.github.com', subcategory: 'issues', parameters: [], - // Partial mock object for testing - 'as unknown as' bypasses strict type checking for missing properties } as unknown as Operation const result = getGHExample( @@ -108,7 +99,6 @@ describe('CLI examples generation', () => { mockVersions, ) - // Check that all contractions are properly escaped expect(result).toContain("Here'\\''s what'\\''s") expect(result).toContain("It'\\''s not working") expect(result).toContain("there'\\''s an issue") @@ -129,12 +119,10 @@ describe('CLI examples generation', () => { 'This alert is not actually correct because there is a sanitizer included in the library.', }, }, - // Partial mock object for testing - 'as unknown as' bypasses strict type checking for missing properties } as unknown as CodeSample const result = getGHExample(mockOperation, codeSample, 'free-pro-team@latest', mockVersions) - // Check that normal text is not modified expect(result).toContain('there is a sanitizer') expect(result).not.toContain("'\\''") expect(result).toContain('gh api') @@ -152,7 +140,6 @@ describe('CLI examples generation', () => { }, contentType: 'application/x-www-form-urlencoded', }, - // Partial mock object for testing - 'as unknown as' bypasses strict type checking for missing properties } as unknown as CodeSample const mockSimpleOperation = { @@ -161,7 +148,6 @@ describe('CLI examples generation', () => { serverUrl: 'https://api.github.com', subcategory: 'pulls', parameters: [], - // Partial mock object for testing - 'as unknown as' bypasses strict type checking for missing properties } as unknown as Operation const result = getShellExample( @@ -171,7 +157,6 @@ describe('CLI examples generation', () => { mockVersions, ) - // Check that form data values are properly escaped expect(result).toContain("Here'\\''s my feedback") expect(result).toContain('--data-urlencode') }) diff --git a/src/rest/tests/create-rest-examples.ts b/src/rest/tests/create-rest-examples.ts index bdb85fd0e494..b019c34e3010 100644 --- a/src/rest/tests/create-rest-examples.ts +++ b/src/rest/tests/create-rest-examples.ts @@ -55,7 +55,6 @@ describe('rest example requests and responses', () => { test('check example number and status code appear', async () => { const mergedExamples = await getCodeSamples(operation) - // example has specific structure from getCodeSamples for (let index = 0; index < mergedExamples.length; index++) { const example = mergedExamples[index] as { request: { description: string } diff --git a/src/rest/tests/get-rest-code-samples-2.ts b/src/rest/tests/get-rest-code-samples-2.ts index 7aa265003a1d..98e59b8b3268 100644 --- a/src/rest/tests/get-rest-code-samples-2.ts +++ b/src/rest/tests/get-rest-code-samples-2.ts @@ -4,7 +4,6 @@ import { getShellExample, getGHExample, getJSExample } from '../components/get-r import type { CodeSample, Operation } from '../components/types' import type { VersionItem } from '@/frame/components/context/MainContext' -// Mock version data similar to what's used in the actual app const mockVersions: Record = { 'free-pro-team@latest': { version: 'free-pro-team@latest', @@ -38,7 +37,6 @@ const mockVersions: Record = { }, } -// Mock operation with standard authentication requirements const standardOperation: Operation = { verb: 'post', title: 'Create an issue', @@ -61,7 +59,7 @@ const standardOperation: Operation = { }, } -// Mock operation with allowPermissionlessAccess (like revoke credentials) +// Sets allowPermissionlessAccess, like the revoke-credentials endpoint. const unauthenticatedOperation: Operation = { verb: 'post', title: 'Revoke a list of credentials', @@ -84,7 +82,6 @@ const unauthenticatedOperation: Operation = { }, } -// Mock operation with basic auth (like OAuth apps) const basicAuthOperation: Operation = { verb: 'post', title: 'Create an OAuth app', @@ -107,7 +104,6 @@ const basicAuthOperation: Operation = { }, } -// Mock operation for GHES manage API const ghesManageOperation: Operation = { verb: 'post', title: 'Set maintenance mode', @@ -129,7 +125,6 @@ const ghesManageOperation: Operation = { }, } -// Mock code sample const mockCodeSample = { key: 'default', request: { @@ -494,7 +489,6 @@ describe('REST code samples authentication header handling', () => { mockVersions, ) - // Should default to including authentication when progAccess is undefined expect(shellResult).toContain('-H "Authorization: Bearer "') }) @@ -518,7 +512,6 @@ describe('REST code samples authentication header handling', () => { mockVersions, ) - // Should default to including authentication when property is missing expect(shellResult).toContain('-H "Authorization: Bearer "') }) @@ -555,13 +548,13 @@ describe('REST code samples authentication header handling', () => { mockVersions, ) - // Should still use management console auth even for allowPermissionlessAccess operations + // Management console auth wins even for allowPermissionlessAccess. expect(result).toContain('-u "api_key:your-password"') expect(result).not.toContain('-H "Authorization: Bearer "') }) test('respects authentication precedence order: enterprise > basic auth > unauthenticated > standard', () => { - // Test enterprise management auth takes precedence over unauthenticated + // Enterprise management auth beats unauthenticated. const enterpriseUnauthOp = { ...unauthenticatedOperation, subcategory: 'manage-ghes', @@ -577,7 +570,7 @@ describe('REST code samples authentication header handling', () => { expect(enterpriseResult).toContain('-u "api_key:your-password"') expect(enterpriseResult).not.toContain('-H "Authorization: Bearer "') - // Test basic auth takes precedence over unauthenticated + // Basic auth beats unauthenticated. const basicAuthUnauthOp = { ...unauthenticatedOperation, progAccess: { diff --git a/src/rest/tests/get-rest-code-samples.ts b/src/rest/tests/get-rest-code-samples.ts index 309729a2f829..7bec0515844a 100644 --- a/src/rest/tests/get-rest-code-samples.ts +++ b/src/rest/tests/get-rest-code-samples.ts @@ -95,7 +95,6 @@ describe('getGHExample - GitHub CLI code generation', () => { const result = getGHExample(operation, codeSample, currentVersion, allVersions) - // The result should use --input for complex objects with arrays expect(result).toContain("--input - <<< '") expect(result).toContain('"bypass_actors": [') expect(result).toContain('"actor_id": 234') @@ -104,7 +103,6 @@ describe('getGHExample - GitHub CLI code generation', () => { expect(result).toContain('"rules": [') expect(result).toContain('"type": "commit_author_email_pattern"') - // Verify the JSON structure is properly formatted expect(result).toContain('"name": "super cool ruleset"') expect(result).toContain('"target": "branch"') expect(result).toContain('"enforcement": "active"') diff --git a/src/rest/tests/lib-index.ts b/src/rest/tests/lib-index.ts index dae6bfbc3e7f..833673f90d55 100644 --- a/src/rest/tests/lib-index.ts +++ b/src/rest/tests/lib-index.ts @@ -1,18 +1,7 @@ -/** - * Unit tests for src/rest/lib/index.ts (PR #60342 changes) - * - * Covers: - * 1. Two-tier cache: fpt/ghec → pinnedCache, other versions → lruCache - * 2. In-flight deduplication: concurrent cold-cache requests share one readFile call - * 3. Brotli fallback: any error reading/decompressing .br falls back to .json - */ - import { describe, test, expect, vi, beforeEach } from 'vitest' -// --------------------------------------------------------------------------- -// Module-level mocks – declared before any dynamic imports so that vi.mock -// hoisting places them before the module under test is first evaluated. -// --------------------------------------------------------------------------- +// These mocks are declared before any dynamic import so that vi.mock hoisting +// places them ahead of the first evaluation of the module under test. vi.mock('fs', async (importOriginal) => { const real = await importOriginal() @@ -65,10 +54,6 @@ vi.mock('@/versions/lib/all-versions', async (importOriginal) => { } }) -// --------------------------------------------------------------------------- -// Helpers -// --------------------------------------------------------------------------- - function enoent(path = 'fake'): NodeJS.ErrnoException { const err = new Error( `ENOENT: no such file or directory, open '${path}'`, @@ -80,10 +65,8 @@ function enoent(path = 'fake'): NodeJS.ErrnoException { const FAKE_DATA: Record = { ops: ['GET /repos'] } const FAKE_JSON = JSON.stringify(FAKE_DATA) -// --------------------------------------------------------------------------- -// Re-import a fresh module instance before each test so module-level state -// (pinnedCache, lruCache, inflight) is empty. -// --------------------------------------------------------------------------- +// Each test re-imports a fresh module instance so that the module-level state +// (pinnedCache, lruCache, inflight) starts out empty. type GetRest = ( version: string, @@ -104,15 +87,12 @@ let fsMock: FsMock beforeEach(async () => { vi.resetModules() - // Import fs mock to configure per-test readFile behaviour. const fsModule = await import('fs') fsMock = fsModule.default as unknown as FsMock - // Reset all call counts. vi.mocked(fsMock.promises.readFile).mockReset() vi.mocked(fsMock.readdirSync).mockReset() - // Re-import the module under test with a clean slate. const mod = await import('@/rest/lib/index') getRest = mod.default as unknown as GetRest pinnedCache = mod.pinnedCache as unknown as Map @@ -123,10 +103,6 @@ beforeEach(async () => { } }) -// --------------------------------------------------------------------------- -// 1. Two-tier cache routing -// --------------------------------------------------------------------------- - describe('two-tier cache routing', () => { test('fpt version lands in pinnedCache, not lruCache', async () => { vi.mocked(fsMock.promises.readFile) @@ -176,10 +152,6 @@ describe('two-tier cache routing', () => { }) }) -// --------------------------------------------------------------------------- -// 1b. Pinned cache compression -// --------------------------------------------------------------------------- - describe('pinned cache compression', () => { test('pinnedCache stores a Buffer (compressed), not a parsed object', async () => { vi.mocked(fsMock.promises.readFile) @@ -197,9 +169,7 @@ describe('pinned cache compression', () => { .mockRejectedValueOnce(enoent()) .mockResolvedValueOnce(FAKE_JSON as unknown as Buffer) - // First call populates the cache const first = await getRest('free-pro-team@latest', undefined, 'actions') - // Second call reads from compressed cache const second = await getRest('free-pro-team@latest', undefined, 'actions') expect(first).toEqual(FAKE_DATA) @@ -219,10 +189,6 @@ describe('pinned cache compression', () => { }) }) -// --------------------------------------------------------------------------- -// 2. In-flight deduplication -// --------------------------------------------------------------------------- - describe('in-flight deduplication', () => { test('N concurrent cold-cache requests for same key share one readFile call', async () => { // Use a deferred to keep all three getRest() calls in flight simultaneously. @@ -236,8 +202,8 @@ describe('in-flight deduplication', () => { return deferred as unknown as Promise }) - // Launch 3 concurrent calls before the deferred resolves — all should be - // in flight at the same time and share the single inflight promise. + // Launch 3 concurrent calls before the deferred resolves, so all three are + // in flight at once and share the single inflight promise. const allPromise = Promise.all([ getRest('free-pro-team@latest', undefined, 'actions'), getRest('free-pro-team@latest', undefined, 'actions'), @@ -247,22 +213,16 @@ describe('in-flight deduplication', () => { resolveJson(FAKE_JSON) const results = await allPromise - // All three callers should receive the same data. expect(results[0]).toEqual(FAKE_DATA) expect(results[1]).toEqual(FAKE_DATA) expect(results[2]).toEqual(FAKE_DATA) - // Dedup: all 3 callers share a single loadCategoryFile() invocation. - // That results in exactly 2 readFile calls: one for .br (rejected) and - // one for .json — NOT 3×2=6 calls. + // All 3 callers share one loadCategoryFile() call, so there are exactly 2 + // readFile calls: one for .br (rejected) and one for .json, not 6. expect(vi.mocked(fsMock.promises.readFile)).toHaveBeenCalledTimes(2) }) }) -// --------------------------------------------------------------------------- -// 3. loadCategoryFile brotli fallback -// --------------------------------------------------------------------------- - describe('loadCategoryFile brotli fallback', () => { test('.br missing (ENOENT) → falls back to .json and returns parsed data', async () => { vi.mocked(fsMock.promises.readFile) diff --git a/src/rest/tests/openapi-schema.ts b/src/rest/tests/openapi-schema.ts index 9351ef5a3f90..2ae9e8caee07 100644 --- a/src/rest/tests/openapi-schema.ts +++ b/src/rest/tests/openapi-schema.ts @@ -36,11 +36,10 @@ async function getFlatListOfOperations(version: string): Promise { } describe('markdown for each rest version', () => { - // Unique set of all categories across all versions of the OpenAPI schema const allCategories = new Set() - // Entire schema including categories and subcategories, keyed by version then category + // Keyed by version, then category. const openApiSchema: Record> = {} - // All applicable version of categories based on frontmatter in the categories index.md file + // Read from the frontmatter of each category's index.md. const categoryApplicableVersions: Record = {} function getApplicableVersionFromFile(file: string) { @@ -76,8 +75,7 @@ describe('markdown for each rest version', () => { } } - // Read the versions from each index.md file to build a list of - // applicable versions for each category + // Each category's applicable versions come from its index.md frontmatter. for (const file of walk('content/rest', { includeBasePath: true, directories: false }).filter( (filename) => filename.includes('index.md'), )) { diff --git a/src/rest/tests/remove-stale-data-files.ts b/src/rest/tests/remove-stale-data-files.ts index b6a0a153f851..325491306093 100644 --- a/src/rest/tests/remove-stale-data-files.ts +++ b/src/rest/tests/remove-stale-data-files.ts @@ -21,12 +21,10 @@ describe('removeStaleRestDataFiles', () => { }) test('removes stale .json files not written during sync', async () => { - // Simulate pre-existing files on disk await writeFile(path.join(versionDir, 'actions.json'), '{}') await writeFile(path.join(versionDir, 'agent-tasks.json'), '{}') await writeFile(path.join(versionDir, 'repos.json'), '{}') - // Only actions.json and repos.json were produced by the sync const writtenFiles = new Map>() writtenFiles.set(versionDir, new Set(['actions.json', 'repos.json'])) diff --git a/src/rest/tests/rendering.ts b/src/rest/tests/rendering.ts index c683b74cafb0..8e698f1b8023 100644 --- a/src/rest/tests/rendering.ts +++ b/src/rest/tests/rendering.ts @@ -26,7 +26,7 @@ describe('REST references docs', () => { } }) - // These tests exists because of issue #1960 + // These tests exist because of issue #1960. test('rest subcategory with fpt in URL', async () => { const categories = [ 'migrations', @@ -122,40 +122,34 @@ describe('REST references docs', () => { }) test('markdown/raw endpoint shows request content types in example selector', async () => { - // Test the specific endpoint that has multiple examples with different request content types + // This endpoint has several examples with differing request content types. const $ = await getDOM('/en/rest/markdown/markdown?apiVersion=2022-11-28') - // Find the render raw mode operation section by its specific ID const rawModeSection = $('#render-a-markdown-document-in-raw-mode--code-samples').parent() expect(rawModeSection.length).toBeGreaterThan(0) - // Should have an example selector dropdown since there are multiple examples + // Several examples means there has to be a selector dropdown. const exampleSelector = rawModeSection.find('select[aria-labelledby], select').first() expect(exampleSelector.length).toBe(1) - // Get the option texts from the dropdown const optionTexts = exampleSelector .find('option') .map((i, option) => $(option).text().trim()) .get() .filter((text) => text.length > 0) - // Should show request content types since they differ between examples + // The content types differ between examples, so they show in the labels. expect(optionTexts).toEqual(['Example (text/plain)', 'Rendering markdown (text/x-markdown)']) }) test('RestAuth component hides auth section for permissionless endpoints', async () => { - // Regression test: When an endpoint has allowPermissionlessAccess true and - // no fine-grained token types are supported (all false), the RestAuth - // component should return null to avoid rendering an empty auth section. - // This test verifies the behavior by loading a REST endpoint that - // demonstrates this pattern. + // This only checks that a page carrying permissionless endpoints still + // renders. It does not reach the RestAuth null path: all five permissionless + // operations under /rest/meta support every fine-grained token type, so + // `noFineGrainedAccess` is false and the guard never fires. const $ = await getDOM('/en/rest/meta') - // The page should render successfully const html = $.html() expect(html.length).toBeGreaterThan(0) - // This test documents that REST reference pages continue to render - // correctly with the RestAuth component changes for permissionless endpoints. }) }) diff --git a/src/rest/tests/sync-changelogs.ts b/src/rest/tests/sync-changelogs.ts index e825cbcf9c73..e543c322a2e4 100644 --- a/src/rest/tests/sync-changelogs.ts +++ b/src/rest/tests/sync-changelogs.ts @@ -9,7 +9,6 @@ import { syncChangelogs, } from '../scripts/utils/sync-changelogs' -// Suppress console.log output during tests beforeEach(() => { vi.spyOn(console, 'log').mockImplementation(() => {}) }) @@ -17,9 +16,6 @@ afterEach(() => { vi.restoreAllMocks() }) -// --------------------------------------------------------------------------- -// parseVersionSections -// --------------------------------------------------------------------------- describe('parseVersionSections', () => { test('parses a changelog with multiple version sections', () => { const markdown = `# REST API Breaking Changes for GitHub Free, Pro & Team @@ -126,9 +122,6 @@ Just some intro text with no version headings.` }) }) -// --------------------------------------------------------------------------- -// getChangelogPath -// --------------------------------------------------------------------------- describe('getChangelogPath', () => { test('returns descriptions-next path for rest-api-description source', () => { const result = getChangelogPath('rest-api-description', 'api.github.com') @@ -168,9 +161,6 @@ describe('getChangelogPath', () => { }) }) -// --------------------------------------------------------------------------- -// syncChangelogs (integration tests using temp directories) -// --------------------------------------------------------------------------- describe('syncChangelogs', () => { let tmpDir: string let outputPath: string @@ -218,21 +208,17 @@ No breaking changes.`, const output = await readFile(outputPath, 'utf-8') - // Should have ifversion fpt wrapping expect(output).toContain('{% ifversion fpt %}') expect(output).toContain('{% endif %}') - // Should have apiVersion filtering for each version section expect(output).toContain('{% if query.apiVersion == nil or "2026-03-10" <= query.apiVersion %}') expect(output).toContain('{% if query.apiVersion == nil or "2022-11-28" <= query.apiVersion %}') - // Should include the actual content expect(output).toContain('Breaking change A') expect(output).toContain('No breaking changes') }) test('generates GHES sections with ghes = X.Y ifversion syntax', async () => { - // Find a real GHES version from allVersions to use const { allVersions } = await import('@/versions/lib/all-versions') const ghesVersion = Object.values(allVersions).find((v) => v.shortName === 'ghes') if (!ghesVersion) return @@ -295,7 +281,8 @@ No breaking changes.`, Content.`, ) - // ghec has a changelog but no version sections — still gets the hardcoded initial version + // ghec has a changelog but no version sections, so it still gets the + // hardcoded initial version. await createChangelog( githubDir, 'ghec', @@ -390,7 +377,6 @@ No breaking changes.`, const output = await readFile(outputPath, 'utf-8') - // Both product versions should be present expect(output).toContain('{% ifversion fpt %}') expect(output).toContain('{% ifversion ghec %}') diff --git a/src/rest/tests/update-markdown.ts b/src/rest/tests/update-markdown.ts index 99b134b8fd67..33b9a6bdea4a 100644 --- a/src/rest/tests/update-markdown.ts +++ b/src/rest/tests/update-markdown.ts @@ -41,21 +41,16 @@ describe('GHES version extraction for update-markdown', () => { }) test('demonstrates the original bug scenario', () => { - // This test demonstrates the bug described in the issue - // where ghes-3.10 would incorrectly match deprecated version 3.1 + // The old substring match found '3.1' inside 'ghes-3.10' and wrongly + // treated 3.10 as deprecated. const filePath = 'src/rest/data/ghes-3.10-2022-11-28/schema.json' const extractedVersion = getGHESVersionFromFilepath(filePath) - // Mock deprecated versions array like in the actual code const deprecated = ['3.0', '3.1', '3.2', '2.22', '2.21'] expect(extractedVersion).toBe('3.10') - // This should be false - 3.10 is NOT in the deprecated list if (extractedVersion) { expect(deprecated.includes(extractedVersion)).toBe(false) } - - // The old buggy logic would have incorrectly flagged this as deprecated - // because it would find '3.1' as a substring in the path }) }) diff --git a/src/search/components/context/SearchOverlayContext.tsx b/src/search/components/context/SearchOverlayContext.tsx index 80f0bebecd9f..fb1378ecacf9 100644 --- a/src/search/components/context/SearchOverlayContext.tsx +++ b/src/search/components/context/SearchOverlayContext.tsx @@ -1,4 +1,3 @@ -// Context to manage the state of the SearchOverlay import { createContext, useContext, PropsWithChildren } from 'react' import { useQueryParam } from '@/frame/components/hooks/useQueryParam' diff --git a/src/search/components/helpers/ai-search-links-json.ts b/src/search/components/helpers/ai-search-links-json.ts index 2b7ee94d414d..a1114ac1a0f1 100644 --- a/src/search/components/helpers/ai-search-links-json.ts +++ b/src/search/components/helpers/ai-search-links-json.ts @@ -36,14 +36,8 @@ export function generateAISearchLinksJson( return JSON.stringify(linksJson) } -// Get all links in a markdown text function extractMarkdownLinks(markdownResponse: string) { - // This regex matches markdown links of the form [text](url) - // Explanation: - // \[([^\]]+)\] : Matches the link text inside square brackets (one or more non-']' characters). - // \( : Matches the opening parenthesis. - // ([^)]+) : Captures the URL (one or more characters that are not a closing parenthesis). - // \) : Matches the closing parenthesis. + // Matches markdown links of the form [text](url). const regex = /\[([^\]]+)\]\(([^)]+)\)/g const urls = [] @@ -53,7 +47,6 @@ function extractMarkdownLinks(markdownResponse: string) { urls.push(match[2]) } - // Filter out any invalid URLs return urls.filter((url) => { try { new URL(url) @@ -65,7 +58,6 @@ function extractMarkdownLinks(markdownResponse: string) { }) } -// Given a Docs URL, extract the product name function extractProductFromDocsUrl(url: string): string { const urlObject = new URL(url) if (urlObject.hostname !== 'docs.github.com') { diff --git a/src/search/components/helpers/execute-search-actions.ts b/src/search/components/helpers/execute-search-actions.ts index cc4899fdab91..6b3bb9087d52 100644 --- a/src/search/components/helpers/execute-search-actions.ts +++ b/src/search/components/helpers/execute-search-actions.ts @@ -64,9 +64,8 @@ export async function executeAISearch(version: string, query: string, debug = fa return response } -/** - * The AJAX request logic that fetches combined search results AI autocomplete suggestions + general search suggestions - */ +// Fetches combined search results: AI autocomplete suggestions plus general +// search suggestions. export async function executeCombinedSearch( router: NextRouter, version: string, @@ -91,7 +90,6 @@ export async function executeCombinedSearch( headers: { 'Content-Type': 'application/json', }, - // Allow the caller to pass in an AbortSignal to cancel the request signal: abortSignal || undefined, }) if (!response?.ok) { diff --git a/src/search/components/helpers/fix-incomplete-markdown.ts b/src/search/components/helpers/fix-incomplete-markdown.ts index c4b1fcde4930..785501e9bf9a 100644 --- a/src/search/components/helpers/fix-incomplete-markdown.ts +++ b/src/search/components/helpers/fix-incomplete-markdown.ts @@ -1,22 +1,17 @@ -// When streaming markdown response, e.g., from a GPT, the response will come in chunks that may have opening tags but no closing tags. -// This function seeks to fix the partial markdown by closing the tags it detects. +// A streamed markdown response arrives in chunks, so a construct can be left +// open. This closes the delimiters it can detect and repairs partial table +// rows. export function fixIncompleteMarkdown(content: string): string { - // First, fix code blocks content = fixCodeBlocks(content) - // Then, fix inline code content = fixInlineCode(content) - // Then, fix links content = fixLinks(content) - // Then, fix images content = fixImages(content) - // Then, fix emphasis (bold, italic, strikethrough) content = fixEmphasis(content) - // Then, fix tables content = fixTables(content) return content @@ -43,13 +38,11 @@ function fixInlineCode(content: string): string { } function fixLinks(content: string): string { - // Handle unclosed link text '[' const linkTextRegex = /\[([^\]]*)$/ if (linkTextRegex.test(content)) { content += ']' } - // Handle unclosed link URL '(' const linkURLRegex = /\]\(([^)]*)$/ if (linkURLRegex.test(content)) { content += ')' @@ -59,13 +52,11 @@ function fixLinks(content: string): string { } function fixImages(content: string): string { - // Handle unclosed image alt text '![' const imageAltTextRegex = /!\[([^\]]*)$/ if (imageAltTextRegex.test(content)) { content += ']' } - // Handle unclosed image URL '(' const imageURLRegex = /!\[[^\]]*\]\(([^)]*)$/ if (imageURLRegex.test(content)) { content += ')' @@ -84,10 +75,8 @@ function fixEmphasis(content: string): string { for (const token of tokens) { if (content.substr(i, token.length) === token) { if (stack.length > 0 && stack[stack.length - 1].token === token) { - // Closing token found stack.pop() } else { - // Opening token found stack.push({ token, index: i }) } i += token.length @@ -118,32 +107,23 @@ function fixTables(content: string): string { while (i < lines.length) { const line = lines[i] if (/^\s*\|.*$/.test(line)) { - // Line starts with '|', possible table line if (!inTable) { - // Potential start of table if (i + 1 < lines.length && /^\s*\|[-\s|:]*$/.test(lines[i + 1])) { - // Next line is separator, confirm table header inTable = true - // Count number of '|' in header line headerPipeCount = (lines[i].match(/\|/g) || []).length i += 1 // Move to separator line } else { - // Not a table, continue i += 1 continue } } else { - // In table body const linePipeCount = (line.match(/\|/g) || []).length if (linePipeCount < headerPipeCount) { - // Calculate missing pipes const missingPipes = headerPipeCount - linePipeCount - // Append missing ' |' to match header columns lines[i] = line.trimEnd() + ' |'.repeat(missingPipes) } } } else { - // Exiting table inTable = false headerPipeCount = 0 } diff --git a/src/search/components/hooks/useAISearchAutocomplete.ts b/src/search/components/hooks/useAISearchAutocomplete.ts index 0c7e17c4d67f..ff917941fce9 100644 --- a/src/search/components/hooks/useAISearchAutocomplete.ts +++ b/src/search/components/hooks/useAISearchAutocomplete.ts @@ -27,20 +27,17 @@ type UseCombinedSearchReturn = { const DEBOUNCE_TIME = 100 // In milliseconds -// Results are only cached for the current session -// We cache results so if a user presses backspace, we can show the results immediately without burdening the API +// Cached for the current page session only, so backspacing reuses results +// instead of hitting the API again. const sessionCache = {} as Record -// Helper to incorporate version & locale into the cache key function getCacheKey(query: string, version: string, locale: string) { return `${query}__${version}__${locale}` } -// Helpers surrounding the ai-search-autocomplete request to lessen the # of requests made to our API -// There are 3 methods for reducing the # of requests: -// 1. Debouncing the request to prevent multiple requests while the user is typing -// 2. Caching the results of the request so if the user presses backspace, we can show the results immediately without burdening the API -// 3. Aborting in-flight requests if the user types again before the previous request has completed +// Three things keep the number of ai-search-autocomplete requests down: +// debouncing while the user types, caching so backspace is free, and each new +// fetch aborting the previous in-flight one. export function useCombinedSearchResults({ router, currentVersion, @@ -54,16 +51,14 @@ export function useCombinedSearchResults({ const [searchLoading, setSearchLoading] = useState(true) const [searchError, setSearchError] = useState(false) - // Support for aborting in-flight requests (e.g. user starts typing while a request is still pending) const abortControllerRef = useRef(null) - // Debounce to prevent requests while user is (quickly) typing const debouncedFetchRef = useRef | null>(null) useEffect(() => { debouncedFetchRef.current = debounce((value: string) => { fetchAutocompleteResults(value) - }, DEBOUNCE_TIME) // 1ms debounce + }, DEBOUNCE_TIME) return () => { debouncedFetchRef.current?.cancel() @@ -72,24 +67,20 @@ export function useCombinedSearchResults({ const fetchAutocompleteResults = useCallback( async (queryValue: string) => { - // Cancel any ongoing request if (abortControllerRef.current) { abortControllerRef.current.abort() } setSearchLoading(true) - // Build cache key based on query, version, and locale const cacheKey = getCacheKey(queryValue, currentVersion, router.locale || 'en') - // Check if the result is in cache if (sessionCache[cacheKey]) { setSearchOptions(sessionCache[cacheKey]) setSearchLoading(false) return } - // If there is an existing search error, don't return any results if (searchError) { setSearchOptions({ aiAutocompleteOptions: [], @@ -100,7 +91,6 @@ export function useCombinedSearchResults({ return } - // Create a new AbortController for the new request const controller = new AbortController() abortControllerRef.current = controller @@ -119,10 +109,8 @@ export function useCombinedSearchResults({ totalGeneralSearchResults: generalSearchResults?.meta?.found?.value || 0, } - // Update cache sessionCache[cacheKey] = results - // Update state with fetched results setSearchOptions(results) setSearchLoading(false) } catch (error: unknown) { @@ -151,8 +139,7 @@ export function useCombinedSearchResults({ // Entry function called when the user types in the search input const updateAutocompleteResults = useCallback((queryValue: string) => { - // When the input is empty, don't debounce the request - // We want to immediately show the autocomplete options (that may be cached) + // Don't debounce an empty input: show the (possibly cached) options at once. if (queryValue === '') { debouncedFetchRef.current?.cancel() fetchAutocompleteResults('') diff --git a/src/search/components/hooks/useAISearchLocalStorageCache.ts b/src/search/components/hooks/useAISearchLocalStorageCache.ts index 4e3ef07aef96..56710892276d 100644 --- a/src/search/components/hooks/useAISearchLocalStorageCache.ts +++ b/src/search/components/hooks/useAISearchLocalStorageCache.ts @@ -10,12 +10,11 @@ interface CacheIndexEntry { timestamp: number } -/** - * We cache AI Search response as individual entries in localStorage, with a separate index to track the keys. - * This allows the cache to be updated without having to read a single large entry into memory and parse it each time a key is accessed - * - * Cached items are cached under a prefix, for a fixed number of days - */ +// AI Search responses are cached as individual localStorage entries, with a +// separate index tracking the keys. Updating the cache therefore doesn't mean +// reading and parsing one large entry every time a key is accessed. +// +// Entries live under a prefix and expire after a fixed number of days. export function useAISearchLocalStorageCache( cacheKeyPrefix: string = 'ai-query-cache', maxEntries: number = 1000, @@ -23,7 +22,6 @@ export function useAISearchLocalStorageCache( ) { const cacheIndexKey = `${cacheKeyPrefix}-index` - // Generates a unique key based on the query string, version, and language const generateCacheKey = (query: string, version: string, language: string): string => { query = query.trim().toLowerCase() // Simple hash function to generate a unique key from the query @@ -56,7 +54,6 @@ export function useAISearchLocalStorageCache( if (now < expirationTime) { return cachedItem.data } else { - // Item expired, remove it localStorage.removeItem(key) updateCacheIndex((index) => index.filter((entry) => entry.key !== key)) return null @@ -83,13 +80,11 @@ export function useAISearchLocalStorageCache( } } - // Remove existing entry for this key if any index = index.filter((entry) => entry.key !== key) index.push({ key, timestamp: now }) // If cache exceeds max entries, remove oldest entries if (index.length > maxEntries) { - // Sort entries by timestamp index.sort((a, b) => a.timestamp - b.timestamp) const excess = index.length - maxEntries const entriesToRemove = index.slice(0, excess) @@ -99,7 +94,6 @@ export function useAISearchLocalStorageCache( index = index.slice(excess) } - // Store updated index localStorage.setItem(cacheIndexKey, JSON.stringify(index)) }, [cacheKeyPrefix, maxEntries], diff --git a/src/search/components/hooks/useMultiQueryParams.ts b/src/search/components/hooks/useMultiQueryParams.ts index 34f2b7eb4373..5f5956897439 100644 --- a/src/search/components/hooks/useMultiQueryParams.ts +++ b/src/search/components/hooks/useMultiQueryParams.ts @@ -30,7 +30,8 @@ export function useMultiQueryParams(options?: { const router = useRouter() const pushTimeoutRef = useRef | null>(null) const useHistory = options?.useHistory ?? false - // When using browser history, exclude these params from being updated on back/forward navigation (like search input which causes race conditions) + // These keys keep their current state across a back/forward navigation + // instead of being re-read from the URL, which would race. const excludeFromHistory = options?.excludeFromHistory ?? [] const getInitialParams = (): QueryParams => { @@ -65,7 +66,6 @@ export function useMultiQueryParams(options?: { // But preserve excluded params from current state to avoid race conditions setParams((currentParams) => { const newParams = getInitialParams() - // Keep excluded params from current state instead of reading from URL for (const key of excludeFromHistory) { newParams[key] = currentParams[key] } @@ -127,8 +127,7 @@ export function useMultiQueryParams(options?: { scroll: false, }) - // Restore scroll position after router update - // This prevents unintended scrolling; intentional scrolling is handled by components + // Restore scroll position after the router update. window.scrollTo(scrollX, scrollY) }, 100) diff --git a/src/search/components/input/AskAIResults.tsx b/src/search/components/input/AskAIResults.tsx index eefd71c2d6be..753c18c62a19 100644 --- a/src/search/components/input/AskAIResults.tsx +++ b/src/search/components/input/AskAIResults.tsx @@ -86,7 +86,6 @@ export function AskAIResults({ let copyUrl = `` if (window?.location?.href) { - // Get base path from current URL const url = new URL(window.location.href) copyUrl = `${url.origin}/?search-overlay-open=true&search-overlay-ask-ai=true&search-overlay-input=${encodeURIComponent(query)}` } @@ -233,7 +232,7 @@ export function AskAIResults({ const decoder = new TextDecoder('utf-8') const reader = response.body.getReader() let done = false - let leftover = '' // <= carry‑over buffer + let leftover = '' setInitialLoading(false) type ParsedLine = { @@ -291,19 +290,18 @@ export function AskAIResults({ const { value, done: readerDone } = await reader.read() done = readerDone - // The sources JSON chunk may be sent in multiple parts, so we need to decode it with a leftover buffer so that it can be parsed all at once - // So when we say "incomplete" or "leftover" we mean that the JSON is not complete yet, not that the message is incomplete + // A newline-delimited JSON record can span stream chunks, so decoded + // text goes into a leftover buffer and is parsed once a whole line + // arrives. "Incomplete" and "leftover" refer to the JSON, not to the + // message. if (value) { - // 1 append this chunk's text to whatever was left over leftover += decoder.decode(value, { stream: true }) - // 2 split on newline const lines = leftover.split('\n') - // 3 keep the *last* item (maybe incomplete) for next round + // Keep the last item, which may be incomplete, for the next round. leftover = lines.pop() ?? '' - // 4 parse all complete lines for (const raw of lines) { if (!raw.trim()) continue @@ -331,7 +329,7 @@ export function AskAIResults({ } } - // 5 flush whatever remains after the stream ends + // Flush whatever remains after the stream ends. if (!isCancelled && leftover.trim()) { try { const tail = JSON.parse(leftover) diff --git a/src/search/components/input/SearchBarButton.tsx b/src/search/components/input/SearchBarButton.tsx index 804b9806c11d..e4d60f1562fb 100644 --- a/src/search/components/input/SearchBarButton.tsx +++ b/src/search/components/input/SearchBarButton.tsx @@ -27,13 +27,11 @@ export function SearchBarButton({ const urlSearchInputQuery = params['search-overlay-input'] - // Handle click events const handleClick = (e: React.MouseEvent) => { e.preventDefault() setIsSearchOpen(true) } - // Handle key down events const handleKeyDown = (event: React.KeyboardEvent) => { if (event.key === 'Enter' || event.key === 'Space') { event.preventDefault() diff --git a/src/search/components/input/SearchOverlay.tsx b/src/search/components/input/SearchOverlay.tsx index 487b724def62..61c38532a96f 100644 --- a/src/search/components/input/SearchOverlay.tsx +++ b/src/search/components/input/SearchOverlay.tsx @@ -64,7 +64,6 @@ export function SearchOverlay({ const { currentVersion } = useVersion() const router = useRouter() - // Map props from multi query state const urlSearchInputQuery = params['search-overlay-input'] const isAskAIState = params['search-overlay-ask-ai'] === 'true' @@ -158,7 +157,8 @@ export function SearchOverlay({ autoCompleteSearchError, ]) - // Filter out any options that match the local query and replace them with a custom user query option that include isUserQuery: true + // Drop the option that duplicates what the user typed. It comes back below + // as a user-query option carrying isUserQuery: true. const filteredAIOptions = aiAutocompleteOptions.filter( (option) => option.term !== urlSearchInputQuery, ) @@ -299,10 +299,10 @@ export function SearchOverlay({ const handleSearchQueryChange = (event: React.ChangeEvent) => { event.preventDefault() const newQuery = event.target.value - setSelectedIndex(-1) // Reset selected index when query changes - // Whenever the query changes, we want to leave the Ask AI state + setSelectedIndex(-1) setSearchLoading(true) updateAutocompleteResults(newQuery) + // Changing the query leaves the Ask AI state. if (isAskAIState) { updateParams({ 'search-overlay-ask-ai': '', @@ -411,7 +411,6 @@ export function SearchOverlay({ event.preventDefault() if (optionsLength > 0) { let newIndex = 0 - // If no item is selected, select the first item if (selectedIndex === -1) { newIndex = 0 } else { @@ -440,16 +439,13 @@ export function SearchOverlay({ event.preventDefault() if (optionsLength > 0) { let newIndex = 0 - // If no item is selected, select the last item if (selectedIndex === -1) { newIndex = optionsLength - 1 } else { - // Otherwise, select the previous item newIndex = (selectedIndex - 1 + optionsLength) % optionsLength // If we go "out of bounds" (i.e. the index is greater than the selected index), unselect the item if (newIndex > selectedIndex) { newIndex = -1 - // If it's the "no results found" option, skip it } } // If it's the "no results found" option, skip it @@ -527,7 +523,6 @@ export function SearchOverlay({ 'search-overlay-ask-ai': '', 'search-overlay-input': urlSearchInputQuery, }) - // Focus the search input inputRef.current?.focus() } @@ -698,7 +693,8 @@ export function SearchOverlay({ maxLength={MAX_QUERY_LENGTH} leadingVisual={} role="combobox" - // In AskAI the search input not longer "controls" the suggestions list, because there is no list, so we remove the aria-controls attribute + // In Ask AI the input controls the results region instead of the + // suggestions list. aria-controls={isAskAIState ? 'ask-ai-result-container' : 'search-suggestions-list'} aria-expanded={combinedOptions.length > 0} aria-label={t('search.overlay.input_aria_label')} diff --git a/src/search/components/results/Aggregations.module.scss b/src/search/components/results/Aggregations.module.scss index cbe04ef00be4..8b072bc420b8 100644 --- a/src/search/components/results/Aggregations.module.scss +++ b/src/search/components/results/Aggregations.module.scss @@ -6,7 +6,7 @@ // it carries side and bottom borders with no top border, so it reads as one box with // the disclosure bar above it. // -// From `medium` up it drops its border entirely and sits flush in the rail column — the +// From `medium` up it drops its border entirely and sits flush in the rail column. The // rail's own divider is the single border there, so the filters don't read as a box // inside a box. It still bounds itself to the rail's height and scrolls its option list // internally, so the heading and "Clear all" stay put and the page behind doesn't move. @@ -39,7 +39,7 @@ .group { // Brand's ControlGroup stacks its children with an 8px gap; the design uses 12px. // This class lands on the same element as ControlGroup__container (the
), - // so the override goes here directly — a descendant selector never matches. + // so the override goes here directly. A descendant selector never matches. gap: 12px !important; // From `medium` up the option list is the scrolling region, so the card's heading and @@ -68,8 +68,8 @@ // itself, so without this the checkbox settles low and the row reads as misaligned. align-items: center !important; - // The design draws each option as a single button, so the whole row — the box included - // — should read as one target. Brand leaves the input and its wrapper on the default + // The design draws each option as a single button, so the whole row, the box + // included, should read as one target. Brand leaves the input and its wrapper on the default // cursor, which makes the box itself look inert even though clicking it works. cursor: pointer; @@ -90,8 +90,8 @@ } // A selected facet steps up to the default text colour, so the active filters are legible -// at a glance against the muted ones — the same muted/default emphasis the result titles -// use for their search match. +// at a glance against the muted ones. That is the same muted/default emphasis the result +// titles use for their search match. .optionLabelSelected { color: var(--brand-color-text-default) !important; } @@ -99,8 +99,8 @@ .count { // Figma: 10px Medium, line-height 1.5. In the design the count is a sibling of the // checkbox+label group in an `items-start` row, so it rides at the top of the line - // rather than on the label's baseline. Ours is inline inside the label — for - // accessibility, so the name still reads "Account and profile (2)" — so it is raised + // rather than on the label's baseline. Ours is inline inside the label, for + // accessibility, so the name still reads "Account and profile (2)". It is raised // here instead. `super` on a 10px run lifts it without growing the 16px line box. margin-left: 2px; font-size: 10px; diff --git a/src/search/components/results/Aggregations.tsx b/src/search/components/results/Aggregations.tsx index 41142de62e95..56aaa8cfacad 100644 --- a/src/search/components/results/Aggregations.tsx +++ b/src/search/components/results/Aggregations.tsx @@ -19,7 +19,7 @@ export function SearchResultsAggregations({ aggregations }: Props) { const selectedQuery = query.toplevel ? query.toplevel : [] const selected = Array.isArray(selectedQuery) ? selectedQuery : [selectedQuery] - // Checking a facet navigates, and the checkbox's state is derived from the URL — so + // Checking a facet navigates, and the checkbox's state is derived from the URL, so // without this the input snaps straight back under React and nothing moves until the // server responds. That round trip is short, but a control that ignores the first // click reads as a frozen page. Hold the intended state locally so the box responds @@ -38,8 +38,8 @@ export function SearchResultsAggregations({ aggregations }: Props) { const params = new URLSearchParams(asPathQuery) // Build from the optimistic state, not from `selected`. Both `asPath` and `selected` // still describe the pre-navigation URL while a facet click is in flight, so a second - // click before the first lands would otherwise drop the first selection — the UI - // showing two boxes ticked and the URL carrying only one. + // click before the first lands would otherwise drop the first selection, leaving the + // UI with two boxes ticked and the URL carrying only one. const nextSelected = new Set( aggregations.toplevel.filter((agg) => isChecked(agg.key)).map((agg) => agg.key), ) @@ -70,7 +70,7 @@ export function SearchResultsAggregations({ aggregations }: Props) { return (
{/* The visible heading sits outside the fieldset so it can stay pinned - while the option list scrolls beneath it — brand renders the group's + while the option list scrolls beneath it. Brand renders the group's own label as a , which is a sibling of the options and would scroll away with them. The legend is kept, visually hidden, so the checkbox group still has an accessible name. */} @@ -103,10 +103,10 @@ export function SearchResultsAggregations({ aggregations }: Props) { {/* Always rendered, so the control is a stable part of the panel rather than - appearing only once you have already filtered — the design shows it in a + appearing only once you have already filtered. The design shows it in a persistent footer row. It pairs with an "Apply" button there, but filters apply immediately on change today, so an Apply control would imply nothing - had happened yet. Staged filtering is Phase 2 — + had happened yet. Staged filtering is Phase 2: github/docs-engineering#6709. With nothing selected there is nothing to clear, so it renders as a disabled diff --git a/src/search/components/results/SearchPage.module.scss b/src/search/components/results/SearchPage.module.scss index ad4a64427d82..303dce6a0d79 100644 --- a/src/search/components/results/SearchPage.module.scss +++ b/src/search/components/results/SearchPage.module.scss @@ -9,7 +9,7 @@ // // And deliberately no `background-color`: the result rows below are transparent // over the page canvas, and brand's canvas-default is pure black in dark mode -// while the page sits on Primer's #0d1117 — painting it here put a visible seam +// while the page sits on Primer's #0d1117. Painting it here put a visible seam // between the band and the rows. // // The heading itself needs no breakpoints: brand's Heading size="3" resolves to diff --git a/src/search/components/results/SearchResults.module.scss b/src/search/components/results/SearchResults.module.scss index 6164a3531e0c..fcd73cba8c98 100644 --- a/src/search/components/results/SearchResults.module.scss +++ b/src/search/components/results/SearchResults.module.scss @@ -2,7 +2,7 @@ // The result row is a grid rather than nested flex wrappers so that the title, category // chip, snippet and debug line stay direct children of the row. See the comment in -// SearchResults.tsx — the search rendering tests read elements by tag inside a result. +// SearchResults.tsx: the search rendering tests read elements by tag inside a result. .searchResult { display: grid; grid-template-columns: minmax(0, 1fr) auto; @@ -45,7 +45,7 @@ // single 550 run, its unmatched text is muted, and the matched text is default. So the // link is muted here and the below restores the default colour. // - // (An earlier attempt leaned on font-weight instead, which was the wrong lever — and + // (An earlier attempt leaned on font-weight instead, which was the wrong lever, and // it barely rendered, since the Mona Sans instance that resolves puts 550 and 700 // within half a pixel of each other.) mark { diff --git a/src/search/components/results/SidebarSearchAggregates.module.scss b/src/search/components/results/SidebarSearchAggregates.module.scss index d94eabe25d3c..fc754df6757e 100644 --- a/src/search/components/results/SidebarSearchAggregates.module.scss +++ b/src/search/components/results/SidebarSearchAggregates.module.scss @@ -6,7 +6,7 @@ // Below `medium` the rail sits above the results in the page flow, so // expanding the panel grows content that the browser's scroll anchoring then - // compensates for — scrolling the disclosure bar off the top of the viewport + // compensates for by scrolling the disclosure bar off the top of the viewport // the moment you open it. Opt this subtree out so opening stays put. overflow-anchor: none; diff --git a/src/search/components/results/SidebarSearchAggregates.tsx b/src/search/components/results/SidebarSearchAggregates.tsx index 692e9d49a2ad..e79471aa111a 100644 --- a/src/search/components/results/SidebarSearchAggregates.tsx +++ b/src/search/components/results/SidebarSearchAggregates.tsx @@ -13,7 +13,7 @@ import styles from './SidebarSearchAggregates.module.scss' // "Show filters" disclosure, because the filters are otherwise unreachable on a // narrow viewport. // -// The facet markup is rendered exactly once and restyled per breakpoint — never +// The facet markup is rendered exactly once and restyled per breakpoint, never // a rail copy plus a drawer copy. Two copies would duplicate every checkbox id // and make the strict-mode `getByText('Fooing (1)')` click in // src/fixtures/tests/playwright-rendering.spec.ts ambiguous. @@ -48,7 +48,7 @@ export function SidebarSearchAggregates() { const { results } = search // `aggregations` is truthy but empty (`{ toplevel: [] }`) for a zero-hit search, and - // SearchResultsAggregations renders nothing in that case — so checking only for the + // SearchResultsAggregations renders nothing in that case, so checking only for the // object left an empty bordered rail on desktop and a disclosure that opened onto an // empty box on mobile. Check for facets to actually show. if (!results?.aggregations?.toplevel?.length) { diff --git a/src/search/lib/ai-search-proxy.ts b/src/search/lib/ai-search-proxy.ts index 6ae2a44e98eb..84f083a69ae7 100644 --- a/src/search/lib/ai-search-proxy.ts +++ b/src/search/lib/ai-search-proxy.ts @@ -41,7 +41,6 @@ export const aiSearchProxy = async (req: ExtendedRequest, res: Response) => { const errors = [] - // Validate request body if (!query) { errors.push({ message: `Missing required key 'query' in request body` }) } else if (typeof query !== 'string') { @@ -74,7 +73,6 @@ export const aiSearchProxy = async (req: ExtendedRequest, res: Response) => { return } - // Handle search analytics and client_name validation const analyticsError = await handleExternalSearchAnalytics(req, 'ai-search') if (analyticsError) { res.status(analyticsError.status).json({ @@ -135,11 +133,9 @@ export const aiSearchProxy = async (req: ExtendedRequest, res: Response) => { return } - // Set response headers res.setHeader('Content-Type', 'application/x-ndjson') res.flushHeaders() - // Stream the response body if (!response.body) { res.status(500).json({ errors: [{ message: 'No response body' }] }) return @@ -156,15 +152,12 @@ export const aiSearchProxy = async (req: ExtendedRequest, res: Response) => { break } - // Decode chunk and count characters const chunk = decoder.decode(value, { stream: true }) totalChars += chunk.length - // Write chunk to response res.write(chunk) } - // Calculate metrics on stream end const totalResponseTime = Date.now() - startTime // in ms const charPerMsRatio = totalResponseTime > 0 ? totalChars / totalResponseTime : 0 // chars per ms @@ -180,7 +173,6 @@ export const aiSearchProxy = async (req: ExtendedRequest, res: Response) => { if (!res.headersSent) { res.status(500).json({ errors: [{ message: 'Internal server error' }] }) } else { - // Send error message via the stream const errorMessage = `${JSON.stringify({ errors: [{ message: 'Internal server error' }] })}\n` res.write(errorMessage) res.end() diff --git a/src/search/lib/elasticsearch-indexes.ts b/src/search/lib/elasticsearch-indexes.ts index d629ff6b23c4..ec4be64c5a61 100644 --- a/src/search/lib/elasticsearch-indexes.ts +++ b/src/search/lib/elasticsearch-indexes.ts @@ -13,20 +13,15 @@ export type SearchIndex = { type: string } -/* Elasticsearch uses indexes to group categories of data - - We currently have 2 top-level categories of indexes: - 1. General search: This is populated using data from all of our Docs pages - 2. AI autocomplete: This is populated with human-readable questions using a GPT query in docs-internal-data - - This file is intended to be the source of truth for Docs Elasticsearch indexes. - - Indexes are in the form: - --- - e.g. github-docs-general-search-fpt-en - - might be "tests_" for tests -*/ +// The source of truth for Docs Elasticsearch indexes. +// +// There are two top-level categories: +// 1. General search, populated from all of our Docs pages. +// 2. AI autocomplete, populated with human-readable questions from a GPT +// query in docs-internal-data. +// +// Index names take the form ___, +// e.g. github-docs_general-search_fpt_en. is "tests_" in tests. const prefix = 'github-docs' const indexes: SearchIndexes = { generalSearch: { @@ -54,14 +49,12 @@ export function getElasticSearchIndex( } const index = indexes[type] as SearchIndex - // Validate language if (!(language in languages)) { throw new Error( `Language ${language} not found in languages for getElasticSearchIndex function.`, ) } - // Validate version if (!allIndexVersionKeys.includes(version)) { throw new Error( `Version '${version}' does not map to a valid version for getElasticSearchIndex function.`, @@ -81,7 +74,6 @@ export function getElasticSearchIndex( // In the index-test-fixtures.sh script, we use the tests_ prefix index for testing const testPrefix = process.env.NODE_ENV === 'test' ? 'tests_' : '' - // If a manual prefix is provided, append an underscore to it if (manualPrefix && !manualPrefix.endsWith('_')) { manualPrefix += '_' } diff --git a/src/search/lib/elasticsearch-versions.ts b/src/search/lib/elasticsearch-versions.ts index 7e09dc27181b..256437c17dc7 100644 --- a/src/search/lib/elasticsearch-versions.ts +++ b/src/search/lib/elasticsearch-versions.ts @@ -1,20 +1,17 @@ -/* - * Source of truth for versioning in the context of Elasticsearch - * We have a unique index for each version of the docs - * so consistency is important for creating/accessing ES Indexes. - * - * Example versions (these may not be up to date): - * - * 1. free-pro-team@latest. Previously known as "dotcom". This is the default version of the docs. - * - short name: fpt - * 2. enterprise-cloud@latest - * - short name: ghec - * 3. enterprise-server@X: This is the source of versioning complexity since the version is dynamic - * - short name: ghes-X - * - * However, for (3) someone might enter `&version=3.5` as the version in the request query string. - * This would map to `ghes-3.5` - */ +// The source of truth for versioning in the context of Elasticsearch. It maps +// every accepted version identifier to the version segment of an index name. +// Several identifiers can share one segment. +// +// Example versions (these may not be up to date): +// +// 1. free-pro-team@latest, previously known as "dotcom", the default version. +// Short name: fpt +// 2. enterprise-cloud@latest. Short name: ghec +// 3. enterprise-server@X, the source of the complexity because the version is +// dynamic. Short name: ghes-X +// +// For (3) someone might pass `&version=3.5` in the request query string, which +// maps to `ghes-3.5`. import { allVersions } from '@/versions/lib/all-versions' @@ -74,7 +71,6 @@ for (const version of Object.values(allVersions)) { allVersionPlans.push(version.plan) } } -// Remove duplicates export const supportedAutocompletePlanVersions = Array.from(new Set(allVersionPlans)) // Returns the plan name for the given version diff --git a/src/search/lib/get-elasticsearch-results/ai-search-autocomplete.ts b/src/search/lib/get-elasticsearch-results/ai-search-autocomplete.ts index e16bbe0bbac5..c6aec94628fd 100644 --- a/src/search/lib/get-elasticsearch-results/ai-search-autocomplete.ts +++ b/src/search/lib/get-elasticsearch-results/ai-search-autocomplete.ts @@ -84,7 +84,6 @@ function getAISearchAutocompleteMatchQueries( const matchQueries: estypes.QueryDslQueryContainer[] = [] - // Use match_phrase for exact term matches matchQueries.push({ match_phrase: { term: { @@ -95,7 +94,6 @@ function getAISearchAutocompleteMatchQueries( }, }) - // Use match for general matching matchQueries.push({ match: { term: { @@ -105,7 +103,6 @@ function getAISearchAutocompleteMatchQueries( }, }) - // Match phrase prefix for partial term matches matchQueries.push({ match_phrase_prefix: { term: { diff --git a/src/search/lib/get-elasticsearch-results/general-search.ts b/src/search/lib/get-elasticsearch-results/general-search.ts index da96492cd724..a6a987d58685 100644 --- a/src/search/lib/get-elasticsearch-results/general-search.ts +++ b/src/search/lib/get-elasticsearch-results/general-search.ts @@ -132,9 +132,7 @@ export async function getGeneralSearchResults( { field_value_factor: { field: 'popularity', - // modifier: 'log1p', factor: 1.0, - // missing: 0.0001, missing: 1.0, }, }, diff --git a/src/search/lib/get-elasticsearch-results/helpers/elasticsearch-highlight-config.ts b/src/search/lib/get-elasticsearch-results/helpers/elasticsearch-highlight-config.ts index ebfd83e81157..72cd57d03d18 100644 --- a/src/search/lib/get-elasticsearch-results/helpers/elasticsearch-highlight-config.ts +++ b/src/search/lib/get-elasticsearch-results/helpers/elasticsearch-highlight-config.ts @@ -22,9 +22,7 @@ export function getHighlightConfiguration( const fields = {} as HighlightFields if (highlightsFields.includes('title')) { fields.title = { - // Fast Vector Highlighter - // Using this requires that you first index these fields - // with {term_vector: 'with_positions_offsets'} + // fvh requires the field to be indexed with {term_vector: 'with_positions_offsets'}. type: 'fvh', fragment_size: 200, number_of_fragments: 1, @@ -32,9 +30,7 @@ export function getHighlightConfiguration( } if (highlightsFields.includes('content')) { fields.content = { - // Fast Vector Highlighter - // Using this requires that you first index these fields - // with {term_vector: 'with_positions_offsets'} + // fvh requires the field to be indexed with {term_vector: 'with_positions_offsets'}. type: 'fvh', fragment_size: 150, number_of_fragments: 1, @@ -50,9 +46,7 @@ export function getHighlightConfiguration( }, } fields.content_explicit = { - // Fast Vector Highlighter - // Using this requires that you first index these fields - // with {term_vector: 'with_positions_offsets'} + // fvh requires the field to be indexed with {term_vector: 'with_positions_offsets'}. type: 'fvh', fragment_size: 150, number_of_fragments: 1, @@ -69,9 +63,7 @@ export function getHighlightConfiguration( } if (highlightsFields.includes('term')) { fields.term = { - // Fast Vector Highlighter - // Using this requires that you first index these fields - // with {term_vector: 'with_positions_offsets'} + // fvh requires the field to be indexed with {term_vector: 'with_positions_offsets'}. type: 'fvh', } } diff --git a/src/search/lib/helpers/external-search-analytics.ts b/src/search/lib/helpers/external-search-analytics.ts index 931c4f217012..d1f0409e2b20 100644 --- a/src/search/lib/helpers/external-search-analytics.ts +++ b/src/search/lib/helpers/external-search-analytics.ts @@ -5,10 +5,9 @@ import { createLogger } from '@/observability/logger' const logger = createLogger(import.meta.url) -/** - * Handles search analytics and client_name validation for external requests - * Returns null if the request should continue, or an error response object if validation failed - */ +// Validates client_name and sends analytics for external requests. Returns null +// when the request should continue, or an error response object when validation +// failed. export async function handleExternalSearchAnalytics( req: ExtendedRequest, searchContext: string, @@ -16,10 +15,8 @@ export async function handleExternalSearchAnalytics( const host = req.headers['x-host'] || req.headers.host const normalizedHost = stripPort(host as string) - // Check if this is likely an external API call rather than a browser request const isLikelyExternalAPI = isExternalAPIRequest(req) - // Get client_name from query or body let client_name = req.query.client_name || req.body?.client_name // Rule 1: Skip analytics for browser requests from our own frontend @@ -51,7 +48,6 @@ export async function handleExternalSearchAnalytics( client_name = 'localhost' } - // Log when we detect an external request that we will send analytics for if (client_name && client_name !== 'docs.github.com-client') { logger.info('External search analytics: Sending analytics for external client', { client_name, @@ -62,7 +58,6 @@ export async function handleExternalSearchAnalytics( }) } - // Send search event with client identifier try { const analyticsPayload = { schema: hydroNames.search, @@ -96,10 +91,7 @@ export async function handleExternalSearchAnalytics( return null } -/** - * Sanitizes user agent by extracting only the main client type - * Returns a safe string with just the primary client identifier - */ +// Reduces a user agent down to its primary client identifier. function sanitizeUserAgent(userAgent: string | undefined): string { if (!userAgent) return 'unknown' @@ -124,18 +116,12 @@ function sanitizeUserAgent(userAgent: string | undefined): string { return 'other' } -/** - * Strips port number from host string - */ function stripPort(host: string): string { const [hostname] = host.split(':') return hostname } -/** - * Determines if a request is likely from an external API client rather than a browser - * Uses multiple heuristics to detect programmatic vs browser requests - */ +// Heuristics for telling a programmatic request from a browser one. const userAgentRegex = /^(curl|wget|python-requests|axios|node-fetch|Go-http-client|okhttp)/i function isExternalAPIRequest(req: ExtendedRequest): boolean { const headers = req.headers diff --git a/src/search/lib/helpers/time.ts b/src/search/lib/helpers/time.ts index 2353d513c1ea..7b21d8361959 100644 --- a/src/search/lib/helpers/time.ts +++ b/src/search/lib/helpers/time.ts @@ -34,16 +34,7 @@ export function utcTimestamp() { ) } -/** - * Converts a given number of seconds into a formatted time string "HH:mm:ss". - * - * @param {number} seconds - The total number of seconds to format. - * @returns {string} A string representing the time in "hours:minutes:seconds" format. - * - * @example - * // returns "01:30:45" - * formatSeconds(5445); - */ +// Formats seconds as "HH:mm:ss". 5445 becomes "01:30:45". Wraps at 24 hours. export function formatSecondsToHHMMSS(seconds: number): string { return new Date(seconds * 1000).toISOString().substr(11, 8) } diff --git a/src/search/lib/routes/combined-search-route.ts b/src/search/lib/routes/combined-search-route.ts index 5876f3c37cf3..b8743af4e535 100644 --- a/src/search/lib/routes/combined-search-route.ts +++ b/src/search/lib/routes/combined-search-route.ts @@ -36,7 +36,6 @@ export async function combinedSearchRoute(req: Request, res: Response) { return res.status(400).json(combinedValidationErrors[0]) } - // Handle search analytics and client_name validation const analyticsError = await handleExternalSearchAnalytics(req, 'combined-search') if (analyticsError) { return res.status(analyticsError.status).json({ @@ -87,7 +86,6 @@ export async function combinedSearchRoute(req: Request, res: Response) { }) } - // Async fetch both results from Elasticsearch const [aiSearchResults, generalSearchResults] = await Promise.all([ autocompletePromise, generalSearchPromise, diff --git a/src/search/lib/routes/general-search-route.ts b/src/search/lib/routes/general-search-route.ts index d4858316f146..713fbaa20d1a 100644 --- a/src/search/lib/routes/general-search-route.ts +++ b/src/search/lib/routes/general-search-route.ts @@ -17,7 +17,6 @@ export async function generalSearchRoute(req: Request, res: Response) { return res.status(400).json(validationErrors[0]) } - // Handle search analytics and client_name validation const analyticsError = await handleExternalSearchAnalytics(req, 'general-search') if (analyticsError) { return res.status(analyticsError.status).json({ diff --git a/src/search/lib/search-request-params/search-params-objects.ts b/src/search/lib/search-request-params/search-params-objects.ts index ce133c8aff12..89fa47adfabe 100644 --- a/src/search/lib/search-request-params/search-params-objects.ts +++ b/src/search/lib/search-request-params/search-params-objects.ts @@ -1,8 +1,7 @@ -/* - When a request is made to a /search endpoint with query parameters, e.g. ?query=foo&version=free-pro-team, - we need to validate and parse the parameters. This file contains the configuration for which parameters - to expect based on the type of search request "e.g. general search vs autocomplete search" and how to validate them. - */ +// A request to a /search endpoint carries query parameters, e.g. +// ?query=foo&version=free-pro-team, which have to be validated and parsed. This +// file configures which parameters to expect for each type of search request +// (general search vs autocomplete search) and how to validate them. import languages from '@/languages/lib/languages-server' import { allIndexVersionKeys, versionToIndexVersionMap } from '@/search/lib/elasticsearch-versions' import { SearchTypes } from '@/search/types' @@ -17,9 +16,6 @@ export function getSearchRequestParamsObject(type: SearchTypes): SearchRequestQu return GENERAL_SEARCH_PARAMS_OBJ } -// - - - Everything below this line is for building the search query param objects - - - // - -// Constants const DEFAULT_AUTOCOMPLETE_SIZE = 5 const MAX_AUTOCOMPLETE_SIZE = 10 const DEFAULT_SIZE = 10 diff --git a/src/search/scripts/aggregate-search-index-failures.ts b/src/search/scripts/aggregate-search-index-failures.ts index 122d8b6548ff..a82839793859 100644 --- a/src/search/scripts/aggregate-search-index-failures.ts +++ b/src/search/scripts/aggregate-search-index-failures.ts @@ -1,14 +1,10 @@ #!/usr/bin/env tsx -/** - * Aggregates search index failures from multiple language runs into a single - * consolidated report. Groups failures by page path to show which versions - * and languages failed for each page. - * - * Usage: tsx aggregate-search-index-failures.ts [--workflow-url ] - * - * Reads failures-summary.json files from subdirectories and outputs a formatted - * message suitable for Slack notifications. - */ +// Reads the failures-summary.json files written by the language index jobs +// that had failures, and prints a JSON AggregationResult whose `message` is a +// single report grouped by page path. index-general-search.yml posts that +// message to both a GitHub issue and Slack. +// +// Usage: tsx aggregate-search-index-failures.ts [--workflow-url ] import fs from 'fs' import path from 'path' @@ -49,10 +45,8 @@ const MAX_ERRORS_PER_PAGE = 3 const MAX_ERROR_LENGTH = 200 const MAX_MESSAGE_LENGTH = 30000 -/** - * Renders a failure as a single line of `errorType: error`, collapsing any - * whitespace so one failure can never span multiple lines of the report. - */ +// Renders a failure as a single line of `errorType: error`, collapsing any +// whitespace so one failure can never span multiple lines of the report. function formatError(failure: Failure): string { const normalize = (value: unknown) => typeof value === 'string' ? value.replace(/\s+/g, ' ').trim() : '' @@ -63,23 +57,19 @@ function formatError(failure: Failure): string { return errorType && detail ? `${errorType}: ${detail}` : errorType || detail } -/** - * Escapes the characters Slack treats as control syntax, so error text lifted - * from an API response cannot inject a mention such as `` into the - * notification. The slack-alert action escapes its own interpolated fields for - * this reason, but passes a caller-supplied message through verbatim. - * - * The same string is also posted as a GitHub issue body, where these entities - * render back to the original characters. - */ +// Escapes the characters Slack treats as control syntax, so error text lifted +// from an API response cannot inject a mention such as `` into the +// notification. The slack-alert action escapes its own interpolated fields for +// this reason, but passes a caller-supplied message through verbatim. +// +// The same string is also posted as a GitHub issue body, where these entities +// render back to the original characters. function escapeSlackControlCharacters(text: string): string { return text.replace(/&/g, '&').replace(//g, '>') } -/** - * Truncates on code points so a multi-byte character is never split in half. - * Docs content is translated, so error text routinely carries non-ASCII. - */ +// Truncates on code points so a multi-byte character is never split in half. +// Docs content is translated, so error text routinely carries non-ASCII. function truncate(text: string, maxLength: number): string { const characters = Array.from(text) if (characters.length <= maxLength) return text @@ -92,10 +82,6 @@ export interface AggregationResult { totalCount?: number } -/** - * Aggregates failures from multiple summaries into a single report. - * Groups failures by page path to show which versions and languages failed for each. - */ export function aggregateFailures( allFailures: FailuresSummary[], workflowUrl?: string, @@ -104,7 +90,6 @@ export function aggregateFailures( return { hasFailures: false, message: '' } } - // Group failures by page path const pageFailures = new Map() for (const summary of allFailures) { @@ -130,10 +115,10 @@ export function aggregateFailures( } } - // Use unique page count, not total failure instances + // Count pages, not failure instances: one page fails once per version and + // language it appears in. const uniquePageCount = pageFailures.size - // Format the message const lines: string[] = [ `:warning: ${uniquePageCount} page(s) failed to scrape for general search indexing`, '', @@ -141,7 +126,6 @@ export function aggregateFailures( '', ] - // Sort pages alphabetically and format each const sortedPages = Array.from(pageFailures.entries()).sort((a, b) => a[0].localeCompare(b[0])) const renderedPages = sortedPages.map(([pagePath, data]) => { @@ -181,7 +165,8 @@ export function aggregateFailures( // Reserve room for the footer up front, using the longest the truncation // notice could get, so MAX_MESSAGE_LENGTH bounds the whole message rather - // than just the part written inside the loop. + // than just the part written inside the loop. The one exception is the forced + // first page below, which can push the message past the limit on its own. const footerReserve = truncatedPagesLine(sortedPages.length).length + 1 + @@ -226,9 +211,6 @@ export function aggregateFailures( return { hasFailures: true, message, totalCount: uniquePageCount } } -/** - * Reads failure summaries from artifact directories. - */ export function readFailureSummaries(artifactsDir: string): FailuresSummary[] { const allFailures: FailuresSummary[] = [] const subdirs = fs.readdirSync(artifactsDir, { withFileTypes: true }) @@ -269,7 +251,6 @@ function main() { console.log(JSON.stringify(result)) } -// Only run main when executed directly (not imported) if (import.meta.url === `file://${process.argv[1]}`) { main() } diff --git a/src/search/scripts/index/lib/index-ai-search-autocomplete.ts b/src/search/scripts/index/lib/index-ai-search-autocomplete.ts index 4f68943ccb33..acb62f126962 100644 --- a/src/search/scripts/index/lib/index-ai-search-autocomplete.ts +++ b/src/search/scripts/index/lib/index-ai-search-autocomplete.ts @@ -101,13 +101,11 @@ function loadQueriesWithPriority(options: LoadOptions): TermsWithFrequency { let popularity = topQueries.length + allQueries.length - // Assign higher popularity to topQueries for (const term of topQueries) { terms[term] = popularity popularity -= 1 } - // Assign remaining popularity to allQueries using the order they have in the JSON for (const term of allQueries) { // Don't read in the topQueries again (duplicates) if (!(term in terms)) { diff --git a/src/search/scripts/index/lib/index-general-search.ts b/src/search/scripts/index/lib/index-general-search.ts index 4da6a381a181..d5ca941426a9 100644 --- a/src/search/scripts/index/lib/index-general-search.ts +++ b/src/search/scripts/index/lib/index-general-search.ts @@ -73,7 +73,6 @@ export async function indexGeneralSearch(sourceDirectory: string, opts: Options) } } - // Validate if (versions !== 'all') { for (const version of versions) { if (!allIndexVersionOptions.includes(version || '')) { diff --git a/src/search/scripts/index/utils/retry-on-error-test.ts b/src/search/scripts/index/utils/retry-on-error-test.ts index 93e251b32d01..4776fac68a56 100644 --- a/src/search/scripts/index/utils/retry-on-error-test.ts +++ b/src/search/scripts/index/utils/retry-on-error-test.ts @@ -15,9 +15,9 @@ // } // const ok = await retry(errorTest, mainFunction, config) // -// Note that, by default, the sleep time is "exponential" by a factor of -// 1.5. So the first sleep will, in the above example, -// be 800ms. Then 1,200ms, Then 1,800ms. etc. +// When `exponential` is truthy the sleep time doubles on each retry, so in the +// example above it goes 800ms, 1,600ms, 3,200ms. Note that the value of +// `exponential` is only ever read as a boolean, never used as the factor. import { sleep } from '@/search/lib/helpers/time' @@ -53,11 +53,6 @@ export async function retryOnErrorTest( // the next attempt these independent processes/threads will now // start at slightly different times. - // According to the Oxford English dictionary, they define "jitter" as: - // - // slight irregular movement, variation, or unsteadiness, - // especially in an electrical signal or electronic device. - // await sleep(addJitter(sleepTime, jitterPercent)) if (exponential) { sleepTime *= 2 diff --git a/src/search/scripts/scrape/lib/build-records-from-api.ts b/src/search/scripts/scrape/lib/build-records-from-api.ts index bf2572f17171..5ec6a4cf1b20 100644 --- a/src/search/scripts/scrape/lib/build-records-from-api.ts +++ b/src/search/scripts/scrape/lib/build-records-from-api.ts @@ -1,13 +1,7 @@ -/** - * Build search records using the Article API instead of HTML scraping. - * - * This module provides functions to fetch article content via the Article API - * and convert it to search index records. This approach is faster and more - * reliable than HTML scraping because it: - * - Fetches pre-rendered markdown directly (no full HTML rendering) - * - Uses structured metadata (title, intro, breadcrumbs) from API - * - Parses headings from markdown using mdast (proper AST parsing) - */ +// Builds search records from the Article API rather than by scraping HTML. +// Fetching pre-rendered markdown skips the full HTML render, takes title, +// intro and breadcrumbs from structured metadata, and parses headings out of +// an mdast tree instead of a DOM. import Bottleneck from 'bottleneck' import chalk from 'chalk' @@ -35,10 +29,9 @@ import type { Redirects, } from '@/search/scripts/scrape/types' -// GitHub-style alert markers (> [!NOTE], > [!TIP], etc.) that appear in -// markdown returned by the Article API. The rehype alerts plugin only runs -// in the HTML pipeline, so these leak through as literal text when we index -// the markdown-only output. Strip them so they don't appear in search results. +// The rehype alerts plugin only runs in the HTML pipeline, so GitHub-style +// alert markers such as `> [!NOTE]` reach the markdown-only output as literal +// text. Strip them so they stay out of search results. const ALERT_MARKER_REGEXP = /\[!(NOTE|TIP|WARNING|IMPORTANT|CAUTION)\]\n?/gi // Same ignored headings as the HTML scraping approach @@ -95,7 +88,6 @@ dotenv.config() const MAX_CONCURRENT = parseInt(process.env.BUILD_RECORDS_MAX_CONCURRENT || '5', 10) const MIN_TIME = parseInt(process.env.BUILD_RECORDS_MIN_TIME || '200', 10) -// These products forcibly get a popularity of 0 const FORCE_0_POPULARITY_PRODUCTS = new Set(['contributing']) const pageMarker = chalk.green('|') @@ -122,9 +114,6 @@ export interface ArticleApiErrorResponse { export type ArticleApiResult = ArticleApiResponse | ArticleApiErrorResponse -/** - * Parse markdown into an AST with GFM support (tables, strikethrough, etc.). - */ function parseMarkdown(markdown: string) { return fromMarkdown(markdown, { extensions: [gfm()], @@ -146,11 +135,9 @@ const BLOCK_CONTAINER_TYPES = new Set([ 'footnoteDefinition', ]) -/** - * Convert an AST to plain text, joining block-level children with newlines. - * Recurses into block containers (lists, blockquotes, etc.) so that nested - * block boundaries also get whitespace — not just the root level. - */ +// Converts an AST to plain text, joining block-level children with newlines. +// It recurses into block containers such as lists and blockquotes so nested +// block boundaries get whitespace too, not just the ones at the root. function astToPlainText(node: Node): string { const parent = node as Parent if (!parent.children) { @@ -166,16 +153,12 @@ function astToPlainText(node: Node): string { return toString(node) } -/** - * Extract headings and plain-text content from markdown in a single AST pass. - * Headings are extracted first, then the full AST (including code blocks) - * is converted to plain text so that terms inside code examples remain - * searchable (e.g. `ssh_url`, `ssh://`). - */ +// Parses the markdown once, then extracts both headings and plain-text +// content from the tree. Code blocks stay in the text so terms that only +// appear in an example, such as `ssh_url` or `ssh://`, stay searchable. export function extractFromMarkdown(markdown: string): { headings: string; content: string } { const ast = parseMarkdown(markdown) - // 1. Extract h2 headings from the AST const headings: string[] = [] const slugger = new GithubSlugger() @@ -194,44 +177,30 @@ export function extractFromMarkdown(markdown: string): { headings: string; conte headings.push(headingText) }) - // 2. Convert full AST to plain text (code blocks are kept so that terms - // appearing only in code examples remain searchable). const content = astToPlainText(ast).replace(ALERT_MARKER_REGEXP, '') return { headings: headings.join('\n'), content } } -/** - * Extract h2 headings from markdown content using mdast parser. - * Filters out navigational headings (in-this-article, further-reading, prerequisites). - */ +// Extracts h2 headings, minus the navigational ones: in-this-article, +// further-reading and prerequisites. export function extractHeadingsFromMarkdown(markdown: string): string { return extractFromMarkdown(markdown).headings } -/** - * Convert markdown to plain text for search indexing using mdast. - * This extracts all text content from the markdown AST, including code blocks. - */ export function markdownToPlainText(markdown: string): string { return extractFromMarkdown(markdown).content } -/** - * Convert Article API response to a search record. - */ export function articleApiResponseToRecord(pathname: string, data: ArticleApiResponse): Record { - // Build breadcrumbs string (excluding the last one which is the current page) const breadcrumbsArray = data.meta.breadcrumbs?.map((b) => b.title) || [] const breadcrumbs = breadcrumbsArray .slice(0, breadcrumbsArray.length > 1 ? -1 : breadcrumbsArray.length) .join(' / ') || '' - // Single-pass extraction: parse markdown once to get both headings and content const { headings, content: bodyText } = extractFromMarkdown(data.body) - // Combine intro with body if intro isn't already in body const intro = data.meta.intro || '' const content = intro && !bodyText.includes(intro.trim()) @@ -258,9 +227,6 @@ function isErrorResponse(data: ArticleApiResult): data is ArticleApiErrorRespons return 'error' in data } -/** - * Fetch article from API and convert to search record. - */ export async function fetchArticleAsRecord( pathname: string, baseUrl: string = `http://localhost:${DEFAULT_PORT}`, @@ -297,7 +263,6 @@ export async function fetchArticleAsRecord( const data = (await response.json()) as ArticleApiResult - // Check for error response (e.g., archived pages) if (isErrorResponse(data)) { return { record: null, @@ -340,10 +305,7 @@ export interface BuildRecordsResult { failedPages: FailedPage[] } -/** - * Build search records for a given index using the Article API. - * This is a drop-in replacement for buildRecords from build-records.ts. - */ +// A drop-in replacement for buildRecords in build-records.ts. export default async function buildRecordsFromApi( indexName: string, indexablePages: Page[], @@ -360,7 +322,6 @@ export default async function buildRecordsFromApi( const records: Record[] = [] const failedPages: FailedPage[] = [] - // Filter pages for this language and version const pages = indexablePages .filter((page) => page.languageCode === languageCode) .filter((page) => page.permalinks.some((permalink) => permalink.pageVersion === pageVersion)) @@ -397,13 +358,11 @@ export default async function buildRecordsFromApi( const hasPopularPages = Object.keys(popularPages).length > 0 const baseUrl = `http://localhost:${DEFAULT_PORT}` - // Use Bottleneck for rate limiting const limiter = new Bottleneck({ maxConcurrent: MAX_CONCURRENT, minTime: MIN_TIME, }) - // Process all permalinks with rate limiting const fetchPromises = permalinks.map((permalink) => limiter.schedule(async () => { const result = await fetchArticleAsRecord(permalink.href, baseUrl) @@ -416,7 +375,6 @@ export default async function buildRecordsFromApi( } if (result.record) { - // Validate required fields before adding to records if (!result.record.title) { failedPages.push({ url: permalink.href, @@ -428,7 +386,6 @@ export default async function buildRecordsFromApi( return null } - // Apply popularity const pathArticle = permalink.relativePath.replace('/index.md', '').replace('.md', '') let popularity = (hasPopularPages && popularPages[pathArticle]) || 0.0 @@ -455,7 +412,6 @@ export default async function buildRecordsFromApi( console.log('\nrecords in index: ', records.length) - // Report failed pages (same format as build-records.ts) if (failedPages.length > 0) { const failureCount = failedPages.length const header = chalk.bold.red(`${failureCount} page(s) failed to scrape\n\n`) diff --git a/src/search/scripts/scrape/lib/find-indexable-pages.ts b/src/search/scripts/scrape/lib/find-indexable-pages.ts index 2938d3393a4e..3aa4f576e1cf 100644 --- a/src/search/scripts/scrape/lib/find-indexable-pages.ts +++ b/src/search/scripts/scrape/lib/find-indexable-pages.ts @@ -5,9 +5,9 @@ import type { Page } from '@/search/scripts/scrape/types' export default async function findIndexablePages(match = ''): Promise { const allPages: Page[] = await loadPages() const indexablePages = allPages - // exclude hidden pages .filter((page) => !page.hidden) - // exclude pages that are part of WIP or hidden products + // exclude pages in visible WIP products. The `|| hidden` was added in + // f4e05b189c8 to exclude hidden products too, but it keeps them instead. .filter((page) => !page.parentProduct || !page.parentProduct.wip || page.parentProduct.hidden) // exclude absolute home page (e.g. /en or /ja) .filter((page) => page.relativePath !== 'index.md') diff --git a/src/search/scripts/scrape/lib/scrape-into-index-json.ts b/src/search/scripts/scrape/lib/scrape-into-index-json.ts index b350e0a3e9ec..5ae768e1c9d6 100644 --- a/src/search/scripts/scrape/lib/scrape-into-index-json.ts +++ b/src/search/scripts/scrape/lib/scrape-into-index-json.ts @@ -8,8 +8,8 @@ import { getElasticSearchIndex } from '@/search/lib/elasticsearch-indexes' import type { Options, Config, Page, Redirects } from '@/search/scripts/scrape/types' -// Build a search data file for every combination of product version and language -// e.g. `github-docs-dotcom-en.json` and `github-docs-2.14-ja.json` +// Build a search data file for every combination of product version and +// language, e.g. `github-docs_general-search_fpt_en-records.json`. export default async function scrapeIntoIndexJson({ language, notLanguage, @@ -19,7 +19,6 @@ export default async function scrapeIntoIndexJson({ }: Options): Promise { const t0 = new Date() - // build indices for a specific language if provided; otherwise build indices for all languages const languagesToBuild = Object.keys(languages).filter((lang) => notLanguage ? notLanguage !== lang : language ? language === lang : true, ) @@ -30,7 +29,7 @@ export default async function scrapeIntoIndexJson({ )}.\n`, ) - // Exclude WIP pages, hidden pages, index pages, etc + // Excludes hidden pages, the absolute homepage, and visible WIP products. const indexablePages: Page[] = await findIndexablePages(config.filter) const redirects: Redirects = {} for (const page of indexablePages) { @@ -53,7 +52,6 @@ export default async function scrapeIntoIndexJson({ failures: Array<{ url?: string; relativePath?: string; error: string; errorType: string }> }> = [] - // Build and validate all indices for (const languageCode of languagesToBuild) { for (const indexVersion of versionsToBuild) { const { indexName } = getElasticSearchIndex('generalSearch', indexVersion, languageCode) @@ -125,7 +123,6 @@ export default async function scrapeIntoIndexJson({ const rate = (countRecordsTotal / tookSec).toFixed(1) console.log(`Rate ~${chalk.bold(rate)} pages per second.`) - // Write failures summary to a file for GitHub Actions to read if (totalFailedPages > 0) { const fs = await import('fs') const path = await import('path') diff --git a/src/search/scripts/scrape/lib/search-index-records.ts b/src/search/scripts/scrape/lib/search-index-records.ts index 394c295febaf..5dd88bbe2c3c 100644 --- a/src/search/scripts/scrape/lib/search-index-records.ts +++ b/src/search/scripts/scrape/lib/search-index-records.ts @@ -30,7 +30,6 @@ export async function writeIndexRecords( const recordsObject = Object.fromEntries(validRecords.map((record) => [record.objectID, record])) const content = JSON.stringify(recordsObject, undefined, 0) - // If the outDirectory doesn't exist, create it if (!fsSync.existsSync(outDirectory)) { await fs.mkdir(outDirectory, { recursive: true }) } @@ -60,7 +59,7 @@ function validateRecords(name: string, records: Record[]): ValidateResult { } } - // each ID is unique — deduplicate rather than crash + // Deduplicate rather than crash when an objectID repeats. const objectIDs = records.map((record) => record.objectID) const dupes = countArrayValues(objectIDs) .filter(({ count }) => count > 1) diff --git a/src/search/tests/ai-search-local-proxy.ts b/src/search/tests/ai-search-local-proxy.ts index e301fb07d255..f0ee6db7e7e6 100644 --- a/src/search/tests/ai-search-local-proxy.ts +++ b/src/search/tests/ai-search-local-proxy.ts @@ -4,19 +4,15 @@ import { get, post } from '@/tests/helpers/e2etest' describe('AI Search Local Proxy Middleware', () => { test('should successfully proxy to docs.github.com when CSE_COPILOT_ENDPOINT is not localhost', async () => { - // In local development, the middleware should proxy to docs.github.com - // This test verifies the middleware handles the proxy correctly - - // We can't easily test the actual proxying without setting up a mock for docs.github.com - // But we can test that the route exists and handles requests + // Under NODE_ENV=test, frame/middleware/api.ts mounts the real aiSearch + // middleware rather than the proxy, so nothing here reaches the proxy. This + // is a smoke test that the route exists and answers. const body = { query: 'test query', version: 'dotcom' } const response = await post('/api/ai-search/v1', { body: JSON.stringify(body), headers: { 'Content-Type': 'application/json' }, }) - // The response should either succeed or fail gracefully - // depending on whether docs.github.com is reachable expect([200, 500, 502, 503, 504]).toContain(response.statusCode) }) @@ -33,7 +29,6 @@ describe('AI Search Local Proxy Middleware', () => { headers: { 'Content-Type': 'application/json' }, }) - // Should handle complex request bodies without crashing expect([200, 500, 502, 503, 504]).toContain(response.statusCode) }) @@ -43,7 +38,6 @@ describe('AI Search Local Proxy Middleware', () => { headers: { 'Content-Type': 'application/json' }, }) - // Should handle empty body gracefully expect([200, 400, 500, 502, 503, 504]).toContain(response.statusCode) }) @@ -53,7 +47,6 @@ describe('AI Search Local Proxy Middleware', () => { headers: { 'Content-Type': 'application/json' }, }) - // Should handle malformed JSON gracefully expect([400, 500]).toContain(response.statusCode) }) @@ -67,7 +60,6 @@ describe('AI Search Local Proxy Middleware', () => { }, }) - // Headers should be processed without causing errors expect([200, 500, 502, 503, 504]).toContain(response.statusCode) }) @@ -78,29 +70,27 @@ describe('AI Search Local Proxy Middleware', () => { 'Content-Type': 'application/json', 'User-Agent': 'test-agent', 'X-Custom-Header': 'test-value', - // Note: Connection, Transfer-Encoding, Upgrade are forbidden headers in fetch - // We test with other headers that should be filtered by the middleware + // fetch forbids Connection, Transfer-Encoding and Upgrade, so a client + // cannot send the hop-by-hop headers the proxy filters. These are + // forwarded as-is. }, }) - // Should succeed despite hop-by-hop headers being present expect([200, 500, 502, 503, 504]).toContain(response.statusCode) }) test('should handle various request methods correctly', async () => { - // Test that only POST is supported const getResponse = await get('/api/ai-search/v1') expect([404, 405]).toContain(getResponse.statusCode) }) test('should handle large request bodies in proxy', async () => { - const largeQuery = 'test query '.repeat(1000) // Create a large query string + const largeQuery = 'test query '.repeat(1000) const response = await post('/api/ai-search/v1', { body: JSON.stringify({ query: largeQuery, version: 'dotcom' }), headers: { 'Content-Type': 'application/json' }, }) - // Should handle large bodies without crashing expect([200, 413, 500, 502, 503, 504]).toContain(response.statusCode) }) }) diff --git a/src/search/tests/api-ai-search-autocomplete.ts b/src/search/tests/api-ai-search-autocomplete.ts index f59f9476b4dd..239b004048a3 100644 --- a/src/search/tests/api-ai-search-autocomplete.ts +++ b/src/search/tests/api-ai-search-autocomplete.ts @@ -1,15 +1,8 @@ -/** - * To be able to run these tests you need to index the fixtures! - * And you need to have an Elasticsearch URL to connect to for the server. - * - * To index the fixtures, run: - * - * ELASTICSEARCH_URL=http://localhost:9200 npm run index-test-fixtures - * - * This will replace any "real" Elasticsearch indexes you might have so - * once you're done working on vitest tests you need to index real - * content again. - */ +// These tests need indexed fixtures and an Elasticsearch URL for the server: +// +// ELASTICSEARCH_URL=http://localhost:9200 npm run index-test-fixtures +// +// That writes `tests_`-prefixed indexes and leaves your regular ones alone. import { expect, test, vi } from 'vitest' @@ -29,7 +22,6 @@ const aiSearchEndpoint = '/api/search/ai-search-autocomplete/v1' const getSearchEndpointWithParams = (searchParams: URLSearchParams) => `${aiSearchEndpoint}?${searchParams}` -// This suite only runs if $ELASTICSEARCH_URL is set. describeIfElasticsearchURL('search/ai-search-autocomplete v1 middleware', () => { vi.setConfig({ testTimeout: 60 * 1000 }) diff --git a/src/search/tests/api-ai-search.ts b/src/search/tests/api-ai-search.ts index a322834bec13..39ee064abeed 100644 --- a/src/search/tests/api-ai-search.ts +++ b/src/search/tests/api-ai-search.ts @@ -37,19 +37,14 @@ describe('AI Search Routes', () => { done = readerDone if (value) { - // Decode the Uint8Array chunk into a string const chunkStr = decoder.decode(value, { stream: true }) chunks.push(chunkStr) } } - // Combine all chunks into a single string const fullResponse = chunks.join('') - // Split the response into individual chunk lines const chunkLines = fullResponse.split('\n').filter((line) => line.trim() !== '') - // Assertions: - // 1. First chunk should be the SOURCES chunk expect(chunkLines.length).toBeGreaterThan(0) const firstChunkMatch = chunkLines[0].match(/^Chunk: (.+)$/) @@ -126,7 +121,6 @@ describe('AI Search Routes', () => { }) test('should handle streaming response correctly', async () => { - // This test verifies the streaming response processing works const body = { query: 'test streaming query', version: 'dotcom' } const response = await fetch('http://localhost:4000/api/ai-search/v1', { method: 'POST', @@ -137,7 +131,6 @@ describe('AI Search Routes', () => { expect(response.ok).toBe(true) expect(response.headers.get('content-type')).toBe('application/x-ndjson') - // Verify we can read the stream without errors if (response.body) { const reader = response.body.getReader() const decoder = new TextDecoder() diff --git a/src/search/tests/api-combined-search.ts b/src/search/tests/api-combined-search.ts index 8f01358ff4e4..d320a038d89a 100644 --- a/src/search/tests/api-combined-search.ts +++ b/src/search/tests/api-combined-search.ts @@ -1,15 +1,8 @@ -/** - * To be able to run these tests you need to index the fixtures! - * And you need to have an Elasticsearch URL to connect to for the server. - * - * To index the fixtures, run: - * - * ELASTICSEARCH_URL=http://localhost:9200 npm run index-test-fixtures - * - * This will replace any "real" Elasticsearch indexes you might have so - * once you're done working on vitest tests you need to index real - * content again. - */ +// These tests need indexed fixtures and an Elasticsearch URL for the server: +// +// ELASTICSEARCH_URL=http://localhost:9200 npm run index-test-fixtures +// +// That writes `tests_`-prefixed indexes and leaves your regular ones alone. import { expect, test, vi } from 'vitest' @@ -29,7 +22,6 @@ const combinedSearchEndpoint = '/api/search/combined-search/v1' const getSearchEndpointWithParams = (searchParams: URLSearchParams) => `${combinedSearchEndpoint}?${searchParams}` -// This suite only runs if $ELASTICSEARCH_URL is set. describeIfElasticsearchURL('search/combined-autocomplete v1 middleware', () => { vi.setConfig({ testTimeout: 60 * 1000 }) diff --git a/src/search/tests/api-search.ts b/src/search/tests/api-search.ts index b20227b40b24..9a6763141cc0 100644 --- a/src/search/tests/api-search.ts +++ b/src/search/tests/api-search.ts @@ -1,15 +1,8 @@ -/** - * To be able to run these tests you need to index the fixtures! - * And you need to have an Elasticsearch URL to connect to for the server. - * - * To index the fixtures, run: - * - * ELASTICSEARCH_URL=http://localhost:9200 npm run index-test-fixtures - * - * This will replace any "real" Elasticsearch indexes you might have so - * once you're done working on vitest tests you need to index real - * content again. - */ +// These tests need indexed fixtures and an Elasticsearch URL for the server: +// +// ELASTICSEARCH_URL=http://localhost:9200 npm run index-test-fixtures +// +// That writes `tests_`-prefixed indexes and leaves your regular ones alone. import { expect, test, vi } from 'vitest' import { describeIfElasticsearchURL } from '@/tests/helpers/conditional-runs' @@ -23,15 +16,13 @@ if (!process.env.ELASTICSEARCH_URL) { ) } -// This suite only runs if $ELASTICSEARCH_URL is set. describeIfElasticsearchURL('search v1 middleware', () => { vi.setConfig({ testTimeout: 60 * 1000 }) test('basic search', async () => { const sp = new URLSearchParams() - // To see why this will work, - // see src/search/tests/fixtures/search-indexes/github-docs-dotcom-en-records.json - // which clearly has a record with the title "Foo" + // src/search/tests/fixtures/search-indexes/tests_github-docs_general-search_fpt_en-records.json + // has a record with the title "Foo". sp.set('query', 'foo') const res = await get(`/api/search/v1?${sp.toString()}`) expect(res.statusCode).toBe(200) @@ -74,7 +65,7 @@ describeIfElasticsearchURL('search v1 middleware', () => { test('debug search', async () => { const sp = new URLSearchParams() sp.set('query', 'foo') - sp.set('debug', '1') // Note! + sp.set('debug', '1') const res = await get(`/api/search/v1?${sp.toString()}`) expect(res.statusCode).toBe(200) const results: GeneralSearchResponse = JSON.parse(res.body) diff --git a/src/search/tests/build-records-from-api.ts b/src/search/tests/build-records-from-api.ts index 8dfa24d97cba..47c142702767 100644 --- a/src/search/tests/build-records-from-api.ts +++ b/src/search/tests/build-records-from-api.ts @@ -72,11 +72,9 @@ Setup steps. ## **Bold** heading ` const headings = extractHeadingsFromMarkdown(markdown) - // Verify complete heading text with formatting stripped expect(headings).toContain('Using code in headings') expect(headings).toContain('A link heading') expect(headings).toContain('Bold heading') - // Should not contain markdown syntax expect(headings).not.toContain('`') expect(headings).not.toContain('**') expect(headings).not.toContain('](') @@ -161,7 +159,6 @@ This is **bold** and *italic* text. expect(text).toContain('italic') expect(text).toContain('List item 1') expect(text).toContain('A link') - // Should not contain markdown syntax expect(text).not.toContain('**') expect(text).not.toContain('](') }) @@ -256,7 +253,6 @@ More text. expect(text).toContain('Column A') expect(text).toContain('Cell 1') expect(text).toContain('More text') - // Should not contain raw GFM table syntax artifacts expect(text).not.toContain('| ---') expect(text).not.toContain('---') }) @@ -655,11 +651,9 @@ describe('fetchArticleAsRecord', () => { const result = await fetchArticleAsRecord('/en/test', 'http://localhost:4002') - // Verify the shape of FetchResult expect(result).toHaveProperty('record') expect(result).toHaveProperty('failure') - // When successful, record should have all expected fields expect(result.record).toHaveProperty('objectID') expect(result.record).toHaveProperty('title') expect(result.record).toHaveProperty('intro') diff --git a/src/search/tests/fix-incomplete-markdown.ts b/src/search/tests/fix-incomplete-markdown.ts index aee62baedc0d..fa94e649006d 100644 --- a/src/search/tests/fix-incomplete-markdown.ts +++ b/src/search/tests/fix-incomplete-markdown.ts @@ -1,7 +1,6 @@ import { expect, test, describe } from 'vitest' import { fixIncompleteMarkdown } from '@/search/components/helpers/fix-incomplete-markdown' -// Unit tests for the `fixIncompleteMarkdown` function describe('fixIncompleteMarkdown', () => { test('should close unclosed bold syntax with double asterisks', () => { const input = 'This is **bold text' diff --git a/src/search/tests/rendering.ts b/src/search/tests/rendering.ts index 5c61f4aa70ea..296dd9bc6c39 100644 --- a/src/search/tests/rendering.ts +++ b/src/search/tests/rendering.ts @@ -1,15 +1,8 @@ -/** - * To be able to run these tests you need to index the fixtures! - * And you need to have an Elasticsearch URL to connect to for the server. - * - * To index the fixtures, run: - * - * ELASTICSEARCH_URL=http://localhost:9200 npm run index-test-fixtures - * - * This will replace any "real" Elasticsearch indexes you might have so - * once you're done working on vitest tests you need to index real - * content again. - */ +// These tests need indexed fixtures and an Elasticsearch URL for the server: +// +// ELASTICSEARCH_URL=http://localhost:9200 npm run index-test-fixtures +// +// That writes `tests_`-prefixed indexes and leaves your regular ones alone. import { expect, test, vi } from 'vitest' @@ -23,14 +16,12 @@ if (!process.env.ELASTICSEARCH_URL) { ) } -// This suite only runs if $ELASTICSEARCH_URL is set. describeIfElasticsearchURL('search rendering page', () => { vi.setConfig({ testTimeout: 60 * 1000 }) test('happy path', async () => { - // To see why this will work, - // see src/search/tests/fixtures/search-indexes/github-docs-dotcom-en-records.json - // which clearly has a record with the title "Foo" + // src/search/tests/fixtures/search-indexes/tests_github-docs_general-search_fpt_en-records.json + // has a record with the title "Foo". const { $ } = await getDOM('/en/search?query=foo') expect($('h1').text()).toMatch(/\d+ Search results for "foo"/) @@ -53,7 +44,6 @@ describeIfElasticsearchURL('search rendering page', () => { test('response headers', async () => { const res = await get('/en/search?query=foo') - // Assuming `res` has a type with a `headers` property expect(res.headers['set-cookie']).toBeUndefined() expect(res.headers['cache-control']).toContain('public') expect(res.headers['cache-control']).toMatch(/max-age=[1-9]/) @@ -66,7 +56,6 @@ describeIfElasticsearchURL('search rendering page', () => { const { $ } = await getDOM('/en/search?query=foo&debug=1') expect($('h1').text()).toMatch(/\d+ Search results for "foo"/) - // Note it testid being 'search-result', not 'search-results' const results = $('[data-testid="search-result"]') expect(results.length).toBeGreaterThan(0) const result = results.first() @@ -103,10 +92,8 @@ describeIfElasticsearchURL('search rendering page', () => { test('links per version in pathname', async () => { const { $ } = await getDOM('/en/enterprise-cloud@latest/search?query=foo') expect($('[data-testid="search-results"]').text()).toMatch('Exclusively for GHEC') - // Note it testid being 'search-result', not 'search-results' const results = $('[data-testid="search-result"]') expect(results.length).toBeGreaterThan(0) - // Each link should have enterprise-cloud@latest in the pathname const links = $('[data-testid="search-result"] a') const hrefs: string[] = links.map((_, el) => $(el).attr('href') ?? '').get() for (const href of hrefs) { @@ -145,7 +132,6 @@ describeIfElasticsearchURL('search rendering page', () => { const { $ } = await getDOM('/en/search?query=foo&toplevel=Baring') expect($('h1').text()).toMatch(/\d+ Search results for "foo"/) - // Note it testid being 'search-result', not 'search-results' const results = $('[data-testid="search-result"]') expect(results.length).toBeGreaterThan(0) const result = results.first() diff --git a/src/workflows/benchmark-pages.ts b/src/workflows/benchmark-pages.ts index 1a876cfcef8c..d27ea86ccf69 100644 --- a/src/workflows/benchmark-pages.ts +++ b/src/workflows/benchmark-pages.ts @@ -116,7 +116,6 @@ function percentile(sorted: number[], p: number): number { } async function main() { - // Check server try { const res = await fetch(`${BASE}/api/pagelist/versions`) if (!res.ok) throw new Error() @@ -125,7 +124,6 @@ async function main() { process.exit(1) } - // Resolve versions and languages let versions: string[] if (args.versions === 'all') { const res = await fetch(`${BASE}/api/pagelist/versions`) @@ -183,7 +181,6 @@ async function main() { } } - // Report problems const errors = all.filter((r) => r.status >= 400) const slow = all.filter((r) => r.timeMs >= SLOW_MS).sort((a, b) => b.timeMs - a.timeMs) @@ -214,7 +211,6 @@ async function main() { console.log(`\n✅ All clear!`) } - // Write JSON for CI consumption if (args.json) { const fs = await import('fs') const allTimes = all.map((r) => r.timeMs).sort((a, b) => a - b) diff --git a/src/workflows/content-changes-table-comment.ts b/src/workflows/content-changes-table-comment.ts index 2fb037b3b97a..2edc5a4d12ab 100755 --- a/src/workflows/content-changes-table-comment.ts +++ b/src/workflows/content-changes-table-comment.ts @@ -1,11 +1,6 @@ -/** - * Hi there! 👋 - * To test this code locally, outside of Actions, you need to run - * the script src/workflows/content-changes-table-comment-cli.ts - * - * See the instructions in the doc string comment at the - * top of src/workflows/content-changes-table-comment-cli.ts - */ +// To test this locally, outside of Actions, run +// src/workflows/content-changes-table-comment-cli.ts. Its file header has the +// instructions. import fs from 'node:fs' import path from 'node:path' @@ -26,11 +21,10 @@ import { inLiquid } from './lib/in-liquid' const { GITHUB_TOKEN, APP_URL, BASE_SHA, HEAD_SHA } = process.env const context = github.context -// the max size of the comment (in bytes) -// the action we use to post the comment caps out at about 144kb -// see docs-engineering#1849 and peter-evans/create-or-update-comment#271 for more info. -// The max size the action allows is 2^16, but our table calculates near the end -// of its rendering before we add a key, so playing it safe with 2^15. +// Max table size in characters. peter-evans/create-or-update-comment allows a +// 2^16 character comment, but the table measures itself near the end of +// rendering, before the key is added, so this stays at 2^15 for headroom. See +// github/docs-engineering#1849 and peter-evans/create-or-update-comment#271. const MAX_COMMENT_SIZE = 32768 const PROD_URL = 'https://docs.github.com' @@ -57,8 +51,7 @@ async function main(owner: string, repo: string, baseSHA: string, headSHA: strin const octokit = retryingGithub(GITHUB_TOKEN) - // get the list of file changes from the PR - // this works even if the head commit is from a fork + // The list of file changes, which works even for a head commit from a fork. const response = await octokit.rest.repos.compareCommitsWithBasehead({ owner, repo, @@ -109,15 +102,12 @@ async function main(owner: string, repo: string, baseSHA: string, headSHA: strin const fileName = file.filename.slice(pathPrefix.length) const fileUrl = fileName.replace('/index.md', '').replace(/\.md$/, '') - // get the file contents and decode them // this script is called from the main branch, so we need the API call to get the contents from the branch, instead const fileContents = await getContents( owner, repo, - // Can't get its content if it no longer exists. - // Meaning, you'd get a 404 on the `getContents()` utility function. - // So, to be able to get necessary meta data about what it *was*, - // if it was removed, fall back to the 'base'. + // `getContents()` 404s on a file that no longer exists, so for a + // removed file read the base sha to get metadata about what it was. file.status === 'removed' ? baseSHA : headSHA, file.filename, ) @@ -209,23 +199,20 @@ function makeRow({ contentCell += `[\`${fileName}\`](${sourceUrl})` try { - // the try/catch is needed because getApplicableVersions() returns either [] or throws an error when it can't parse the versions frontmatter - // try/catch can be removed if docs-engineering#1821 is resolved - // i.e. for feature based versioning, like ghec: 'issue-6337' + // getApplicableVersions() throws on missing, invalid or unsupported + // versions frontmatter. Remove the try/catch once + // github/docs-engineering#1821 is fixed. const fileVersions: string[] = getApplicableVersions(data?.versions) for (const plan in allVersionShortnames) { - // plan is the shortName (i.e., fpt) - // allVersionShortNames[plan] is the planName (i.e., free-pro-team) - - // walk by the plan names since we generate links differently for most plans + // `plan` is the short name, e.g. fpt, used as the link label. + // allVersionShortnames[plan] is the plan name, e.g. free-pro-team, used + // to pick the file's matching versions. Most plans link differently. const versions = fileVersions.filter((fileVersion) => fileVersion.includes(allVersionShortnames[plan]), ) if (versions.length === 1) { - // for fpt and ghec - if (versions.toString() === nonEnterpriseDefaultVersion) { // omit version from fpt url diff --git a/src/workflows/fm-utils.ts b/src/workflows/fm-utils.ts index eb06fd9f3018..3009a6c4309d 100644 --- a/src/workflows/fm-utils.ts +++ b/src/workflows/fm-utils.ts @@ -1,9 +1,7 @@ import { existsSync, readFileSync } from 'fs' import matter from '@gr2m/gray-matter' -// Filters out files from a list of filePaths -// that have a contentType property in their frontmatter -// where the contentType value matches the given contentType argument +// The file paths whose frontmatter `contentType` matches `contentType`. export function checkContentType(filePaths: string[], contentType: string) { const unallowedChangedFiles = [] for (const filePath of filePaths) { diff --git a/src/workflows/fr-add-docs-reviewers-requests.ts b/src/workflows/fr-add-docs-reviewers-requests.ts index 079831d4e474..2191ea386503 100644 --- a/src/workflows/fr-add-docs-reviewers-requests.ts +++ b/src/workflows/fr-add-docs-reviewers-requests.ts @@ -105,7 +105,6 @@ async function getAllOpenPRs() { } async function run() { - // Get info about open github/github PRs const prData = await getAllOpenPRs() // Get the PRs that are: @@ -136,7 +135,6 @@ async function run() { const prAuthors = prs.map((pr) => pr.author.login) console.log(`PRs found: ${prIDs}`) - // Get info about the docs-content review board project const projectData = await graphql( ` query ($organization: String!, $projectNumber: Int!) { @@ -177,7 +175,6 @@ async function run() { }, ) - // Get the project ID const projectID = projectData.organization.projectV2.id // Get the IDs of the last 100 items on the board. @@ -188,7 +185,6 @@ async function run() { (node: { id: string }) => node.id, ) - // Get the ID of the fields that we want to populate const datePostedID = findFieldID('Date posted', projectData) const reviewDueDateID = findFieldID('Review due date', projectData) const statusID = findFieldID('Status', projectData) @@ -197,13 +193,11 @@ async function run() { const sizeTypeID = findFieldID('Size', projectData) const authorID = findFieldID('Contributor', projectData) - // Get the ID of the single select values that we want to set const readyForReviewID = findSingleSelectID('Ready for review', 'Status', projectData) const hubberTypeID = findSingleSelectID('Hubber or partner', 'Contributor type', projectData) const docsMemberTypeID = findSingleSelectID('Docs team', 'Contributor type', projectData) const sizeMediumID = findSingleSelectID('M', 'Size', projectData) - // Add the PRs to the project const itemIDs = await addItemsToProject(prIDs, projectID) // If an item already existed on the project, the existing ID will be returned. @@ -225,8 +219,7 @@ async function run() { return } - // Populate fields for the new project items - // (Using for...of instead of forEach since the function uses await) + // for...of rather than forEach because the body awaits. for (const [index, itemID] of newItemIDs.entries()) { const updateProjectV2ItemMutation = generateUpdateProjectV2ItemFieldMutation({ item: itemID, diff --git a/src/workflows/get-env-inputs.ts b/src/workflows/get-env-inputs.ts index 7019a6f6e2b5..d0e3c4a00937 100644 --- a/src/workflows/get-env-inputs.ts +++ b/src/workflows/get-env-inputs.ts @@ -1,10 +1,4 @@ -/* - * Validates and returns an object of expected environment variables - * - * @param {Array} options - Array of environment variables expected - * - * @returns {Object} - key value of expected env variables and their values - */ +// Validates the named environment variables and returns them as an object. export function getEnvInputs(options: string[]) { return Object.fromEntries( options.map((envVarName) => { @@ -17,18 +11,9 @@ export function getEnvInputs(options: string[]) { ) } -/* - * Given an environment variable key, return `true` or `false` if the - * value is recognizable. - * Turn 'true' or '1' into `true`. And '', '0', or 'false' into `false`. - * All other values are invalid. - * Now you can't accidentally set `export FOO=falsee` which as string `'falsee'` - * could have been interpreted as a truthy value. - * - * @param {string} key - name of the environment variable - * - * @returns {boolean} - */ +// Reads an environment variable as a boolean. 'true' and '1' are true; '', '0' +// and 'false' are false. Anything else throws, so a typo like +// `export FOO=falsee` can't be read as truthy. export function boolEnvVar(key: string) { const value = process.env[key] || '' if (value === '' || value === 'false' || value === '0') return false diff --git a/src/workflows/git-utils.ts b/src/workflows/git-utils.ts index ed328820e34f..3dbfd6ac074f 100644 --- a/src/workflows/git-utils.ts +++ b/src/workflows/git-utils.ts @@ -64,8 +64,6 @@ export async function getTree(owner: string, repo: string, ref: string) { tree_sha: treeSha, recursive: 'true', }) - // only return files that match the patterns in allowedPaths - // skip actions/changes files return data.tree } catch (err) { console.log('error getting tree', owner, repo, ref) @@ -80,7 +78,6 @@ export async function getContentsForBlob(owner: string, repo: string, sha: strin repo, file_sha: sha, }) - // decode blob contents return Buffer.from(data.content, 'base64').toString() } @@ -91,7 +88,6 @@ export async function getContents(owner: string, repo: string, ref: string, path if (!('content' in data) || !data.content) { return await getContentsForBlob(owner, repo, data.sha) } - // decode Base64 encoded contents return Buffer.from(data.content, 'base64').toString() } @@ -103,7 +99,6 @@ export async function getContentAndData(owner: string, repo: string, ref: string 'content' in data && data.content ? Buffer.from(data.content, 'base64').toString() : await getContentsForBlob(owner, repo, data.sha) - // decode Base64 encoded contents return { content, blobSha: data.sha } } @@ -156,7 +151,7 @@ export async function createIssueComment( } } -// Search for a string in a file in code and return the array of paths to files that contain string +// The paths of files in the repo containing any of the given strings. export async function getPathsWithMatchingStrings( strArr: string[], org: string, @@ -238,10 +233,7 @@ async function searchCode( } } -// Recursively gets the contents of a directory within a repo. Returns an -// array of file contents. This function could be modified to return an array -// of objects that include the path and the content of the file if needed -// in the future. +// Recursively gets the contents of a directory within a repo. export async function getDirectoryContents( owner: string, repo: string, @@ -260,7 +252,6 @@ export async function getDirectoryContents( const blobContents = await getContentsForBlob(owner, repo, blob.sha) files.push({ path: blob.path, content: blobContents }) } else { - // decode Base64 encoded contents const decodedContent = Buffer.from(blob.content, 'base64').toString() files.push({ path: blob.path, content: decodedContent }) } diff --git a/src/workflows/issue-report.ts b/src/workflows/issue-report.ts index 72aab147daff..68bb81dbcfe8 100644 --- a/src/workflows/issue-report.ts +++ b/src/workflows/issue-report.ts @@ -18,7 +18,6 @@ export async function createReportIssue({ reportLabel, }: CRIArgs) { const [owner, repo] = reportRepository.split('/') - // Create issue let newReport try { const { data } = await octokit.request('POST /repos/{owner}/{repo}/issues', { @@ -60,7 +59,6 @@ export async function linkReports({ const [owner, repo] = reportRepository.split('/') core.info('Attempting to link reports...') - // Find previous report issue let previousReports try { previousReports = await octokit.rest.issues.listForRepo({ @@ -88,7 +86,6 @@ export async function linkReports({ // 2nd report should be most recent previous report const previousReport = previousReports[1] - // Comment the old report link on the new report try { await octokit.rest.issues.createComment({ owner, @@ -102,7 +99,6 @@ export async function linkReports({ throw error } - // Comment on all previous reports that are still open for (const oldReport of previousReports) { if (oldReport.state === 'closed' || oldReport.html_url === newReport.html_url) { continue diff --git a/src/workflows/projects.ts b/src/workflows/projects.ts index 8332c4b6c582..b6271bf2fb22 100644 --- a/src/workflows/projects.ts +++ b/src/workflows/projects.ts @@ -71,7 +71,6 @@ export interface ItemData { } } -// Pull out the node ID of a project field export function findFieldID(fieldName: string, data: ProjectV2Data) { const field = data.organization.projectV2.fields.nodes.find( (fieldNode) => fieldNode.name === fieldName, @@ -84,7 +83,6 @@ export function findFieldID(fieldName: string, data: ProjectV2Data) { } } -// Pull out the node ID of a single select field value export function findSingleSelectID( singleSelectName: string, fieldName: string, @@ -108,9 +106,8 @@ export function findSingleSelectID( } } -// Given a list of PR/issue node IDs and a project node ID, -// adds the PRs/issues to the project -// and returns the node IDs of the project items +// Adds the PRs/issues to the project and returns their project item IDs. An +// item already on the board keeps its existing ID. export async function addItemsToProject(items: string[], project: string) { console.log(`Adding ${items} to project ${project}`) @@ -140,9 +137,7 @@ export async function addItemsToProject(items: string[], project: string) { }, }) - // The output of the mutation is - // {"item_0":{"projectNextItem":{"id":ID!}},...} - // Pull out the ID for each new item + // The mutation returns {"item_0":{"item":{"id":ID!}},...}. const newItemIDs = Object.entries(newItems).map((item) => item[1].item.id) @@ -157,14 +152,12 @@ export async function addItemToProject(item: string, project: string) { return newItemID } -// Given a GitHub login, returns a bool indicating -// whether the login is part of the docs team export async function isDocsTeamMember(login: string) { - // Returns true if login is docs-bot or copilot, to bypass the checks and make PRs opened by docs-bot or copilot be treated as though they were made by a docs team member + // docs-bot and copilot bypass the check so their PRs are treated as though a + // docs team member opened them. if (login === 'docs-bot' || login === 'copilot') { return true } - // Get all members of the docs team const data: TeamMemberData = await graphql( ` query ($slug: String!) { @@ -206,8 +199,6 @@ export async function isDocsTeamMember(login: string) { return teamMembers.includes(login) } -// Given a GitHub login, returns a bool indicating -// whether the login is part of the GitHub org export async function isGitHubOrgMember(login: string) { const data: OrgMemberData = await graphql( ` @@ -229,14 +220,14 @@ export async function isGitHubOrgMember(login: string) { return Boolean(data.user.organization) } -// Formats a date object into the required format for projects export function formatDateForProject(date: Date) { return date.toISOString() } -// Given a date object and optional turnaround time -// Calculate the date {turnaround} business days from now -// (excluding weekends; not considering holidays) +// `turnaround` days from `datePosted`, plus two days if posted on a Thursday +// or Friday and one if posted on a Saturday. With the default turnaround of 2 +// that lands on a weekday; a larger turnaround can still land on a weekend. +// Holidays are not considered. export function calculateDueDate(datePosted: Date, turnaround = 2) { let daysUntilDue switch (datePosted.getDay()) { @@ -257,14 +248,11 @@ export function calculateDueDate(datePosted: Date, turnaround = 2) { return dueDate } -// Given a project item node ID and author login -// generates a GraphQL mutation to populate: -// - "Status" (as variable passed with the request) -// - "Date posted" (as today) -// - "Review due date" (as today + {turnaround} weekdays) -// - "Contributor type" (as variable passed with the request) -// - "Feature" (as {feature}) -// - "Author" (as {author})" +// A GraphQL mutation that populates these fields on one project item: +// - "Status", "Contributor type" and "Size", passed as request variables +// - "Date posted", today +// - "Review due date", see calculateDueDate +// - "Feature" and "Contributor" export function generateUpdateProjectV2ItemFieldMutation({ item, author, @@ -279,8 +267,8 @@ export function generateUpdateProjectV2ItemFieldMutation({ const datePosted = new Date() const dueDate = calculateDueDate(datePosted, turnaround) - // Build the mutation to update a single project field - // Specify literal=true to indicate that the value should be used as a string, not a variable + // Builds the mutation for a single field. literal=true means the value is a + // string rather than a variable reference. function generateMutationToUpdateField({ item: itemId, fieldID, @@ -296,8 +284,8 @@ export function generateUpdateProjectV2ItemFieldMutation({ }) { const parsedValue = literal ? `${fieldType}: "${value}"` : `${fieldType}: ${value}` - // Strip all non-alphanumeric out of the item ID when creating the mutation ID to avoid a GraphQL parsing error - // (statistically, this should still give us a unique mutation ID) + // Anything outside [a-z0-9] in the mutation ID is a GraphQL parse error, + // so strip it. The result is still unique in practice. return ` set_${fieldID.slice(1)}_item_${itemId.replaceAll( /[^a-z0-9]/g, @@ -381,19 +369,17 @@ export function generateUpdateProjectV2ItemFieldMutation({ return mutation } -// Guess the affected docs sets based on the files that the PR changed +// Guesses the affected docs sets from the files the PR changed. export function getFeature(data: ItemData) { - // For issues, just use an empty string if (data.item.__typename !== 'PullRequest') { return '' } const paths = data.item.files.nodes.map((node) => node.path) - // For docs and docs-internal and docs-early-access PRs, - // determine the affected docs sets by looking at which - // directories under `content/` were affected. - // (Ignores changes to the data files.) + // For docs, docs-internal and docs-early-access, take the docs sets from the + // directories under `content/` that changed. Changes to data files are + // ignored. if ( process.env.REPO === 'github/docs-internal' || process.env.REPO === 'github/docs' || @@ -411,7 +397,7 @@ export function getFeature(data: ItemData) { return feature } - // for github/github PRs, try to classify the OpenAPI files + // For github/github, classify by the OpenAPI files instead. if (process.env.REPO === 'github/github') { const features: Set = new Set([]) if (paths.some((path: string) => path.startsWith('app/api/description'))) { @@ -443,14 +429,14 @@ export function getFeature(data: ItemData) { return '' } -// Guess the size of an item +// Guesses the size of an item. export function getSize(data: ItemData) { - // We need to set something in case this is an issue, so just guesstimate small + // An issue has no files to measure, so guess small. if (data.item.__typename !== 'PullRequest') { return 'S' } - // for github/github PRs, estimate the size based on the number of OpenAPI files that were changed + // For github/github, size by the count and line changes of OpenAPI files. if (process.env.REPO === 'github/github') { let numFiles = 0 let numChanges = 0 @@ -471,7 +457,7 @@ export function getSize(data: ItemData) { return 'L' } } else { - // Otherwise, estimated the size based on all files + // Otherwise size by the count and line changes of all changed files. let numFiles = 0 let numChanges = 0 for (const node of data.item.files.nodes) { diff --git a/src/workflows/purge-fastly-changed-content.ts b/src/workflows/purge-fastly-changed-content.ts index 14cffcd80c26..3a1cdb46bdad 100644 --- a/src/workflows/purge-fastly-changed-content.ts +++ b/src/workflows/purge-fastly-changed-content.ts @@ -111,7 +111,7 @@ type ChangedFile = { // The most recent production deployment that was actually live before `headSha`. // We diff against this to find what changed in the current deploy. The merge // queue can batch several PRs into one deploy, so this range can span multiple -// merge commits — that's intentional, we want every changed file in the batch. +// merge commits. That's intentional: we want every changed file in the batch. export async function resolvePreviousProductionSha( octokit: Octokit, owner: string, diff --git a/src/workflows/ready-for-docs-review.ts b/src/workflows/ready-for-docs-review.ts index 06757373302a..bc01a13b4d67 100644 --- a/src/workflows/ready-for-docs-review.ts +++ b/src/workflows/ready-for-docs-review.ts @@ -13,45 +13,34 @@ import { type ItemData, } from './projects' -/** - * Determines if a PR is authored by Copilot and extracts the human assignee - * @param data GraphQL response data containing PR information - * @returns Object with isCopilotAuthor boolean and copilotAssignee string - */ +// Whether copilot-swe-agent authored the PR, and its first other assignee. function getCopilotAuthorInfo(data: ItemData): { isCopilotAuthor: boolean copilotAssignee: string } { const item = data.item - // Check if this is a Copilot-authored PR const isCopilotAuthor = !!( item.__typename === 'PullRequest' && item.author && item.author.login === 'copilot-swe-agent' ) - // For Copilot PRs, find the appropriate assignee (excluding Copilot itself) + // copilot-swe-agent is usually an assignee too, so skip it. let copilotAssignee = '' if (isCopilotAuthor && item.assignees && item.assignees.nodes) { const assigneeLogins = item.assignees.nodes .map((assignee) => assignee.login) .filter((login) => login !== 'copilot-swe-agent') - // Use the first non-Copilot assignee copilotAssignee = assigneeLogins.length > 0 ? assigneeLogins[0] : '' } return { isCopilotAuthor, copilotAssignee: copilotAssignee || '' } } -/** - * Determines the appropriate author field value based on contributor type - * @param isCopilotAuthor Whether the PR is authored by Copilot - * @param copilotAssignee The human assignee for Copilot PRs (empty string if none) - * @param firstTimeContributor Whether this is a first-time contributor - * @returns The formatted author field value - */ +// The value for the board's "Contributor" field, which depends on whether +// Copilot authored the PR and whether the author is a first-time contributor. function getAuthorFieldValue( isCopilotAuthor: boolean, copilotAssignee: string, @@ -69,7 +58,6 @@ function getAuthorFieldValue( } async function run() { - // Get info about the docs-content review board project const data = (await graphql( ` query ($organization: String!, $projectNumber: Int!, $id: ID!) { @@ -126,10 +114,8 @@ async function run() { }, )) as ProjectV2Data & ItemData - // Get the project ID const projectID = data.organization.projectV2.id - // Get the ID of the fields that we want to populate const datePostedID = findFieldID('Date posted', data) const reviewDueDateID = findFieldID('Review due date', data) const statusID = findFieldID('Status', data) @@ -138,16 +124,13 @@ async function run() { const sizeTypeID = findFieldID('Size', data) const authorID = findFieldID('Contributor', data) - // Get the ID of the single select values that we want to set const readyForReviewID = findSingleSelectID('Ready for review', 'Status', data) const hubberTypeID = findSingleSelectID('Hubber or partner', 'Contributor type', data) const docsMemberTypeID = findSingleSelectID('Docs team', 'Contributor type', data) const osContributorTypeID = findSingleSelectID('OS contributor', 'Contributor type', data) - // Add the PR to the project const newItemID = await addItemToProject(process.env.ITEM_NODE_ID || '', projectID) - // Determine the feature and size const feature = getFeature(data) const size = getSize(data) const sizeType = findSingleSelectID(size, 'Size', data) @@ -225,17 +208,14 @@ async function run() { } const turnaround = process.env.REPO === 'github/docs' ? 3 : 2 - // Check if this is a Copilot-authored PR and get the human assignee const { isCopilotAuthor, copilotAssignee } = getCopilotAuthorInfo(data) - // Determine the author field value const authorFieldValue = getAuthorFieldValue( isCopilotAuthor, copilotAssignee, firstTimeContributor, ) - // Generate a mutation to populate fields for the new project item const updateProjectV2ItemMutation = generateUpdateProjectV2ItemFieldMutation({ item: newItemID, author: authorFieldValue, @@ -243,7 +223,6 @@ async function run() { feature, }) - // Determine which variable to use for the contributor type let contributorType if (isCopilotAuthor || isBotAuthor) { // Treat Copilot and bot-authored PRs (e.g. dependabot[bot]) as Docs team diff --git a/src/workflows/tests/actions-workflows.ts b/src/workflows/tests/actions-workflows.ts index 2c7062970ebf..f18c6ff0b6e3 100644 --- a/src/workflows/tests/actions-workflows.ts +++ b/src/workflows/tests/actions-workflows.ts @@ -79,7 +79,7 @@ const scheduledWorkflows = workflows.filter(({ data }) => data.on.schedule) // running in docs-internal itself. When a job is scoped to the public // github/docs fork via `if: github.repository == 'github/docs'`, those // triggers fire from external reporters/commenters, and the issue or -// comment itself is the natural failure surface — piling on automated +// comment itself is the natural failure surface. Piling on automated // alert-issues there is duplicative and noisy. const ALWAYS_UNATTENDED_TRIGGERS = ['schedule', 'workflow_run', 'repository_dispatch', 'push'] const DOCS_INTERNAL_ONLY_UNATTENDED_TRIGGERS = ['issues', 'issue_comment'] @@ -102,7 +102,7 @@ function jobRequiresFailureAlerts(workflow: WorkflowMeta, job: WorkflowJob): boo return false } -// Workflows where at least one job requires failure alerts — used to drive +// Workflows where at least one job requires failure alerts. Used to drive // the parameterised tests below. Per-job filtering happens inside each test. const alertWorkflows = workflows.filter(({ data }) => Object.values(data.jobs).some((job) => job.steps), @@ -231,8 +231,8 @@ describe('GitHub Actions workflows', () => { // GitHub App token instead. // // NOTE: this intentionally does NOT cover plain `pull_request`. That trigger - // does not expose secrets to fork PRs — only to same-repo branch PRs from - // contributors who already have write access — and passing the PAT to local + // does not expose secrets to fork PRs, only to same-repo branch PRs from + // contributors who already have write access, and passing the PAT to local // actions there (e.g. get-docs-early-access) is a longstanding, accepted // pattern across many workflows. See #62343. const pullRequestTargetWorkflows = workflows.filter(({ data }) => { diff --git a/src/workflows/unallowed-contributions.ts b/src/workflows/unallowed-contributions.ts index 51cdccc814f7..be643e0dc823 100755 --- a/src/workflows/unallowed-contributions.ts +++ b/src/workflows/unallowed-contributions.ts @@ -24,7 +24,6 @@ const filters = load(readFileSync('src/workflows/unallowed-contribution-filters. main() async function main() { - // Files in the diff that match specific paths we don't allow const unallowedChangedFiles = [...JSON.parse(FILE_PATHS_NOT_ALLOWED || '')] // Content files that are added in a forked repo won't be in the @@ -42,7 +41,6 @@ async function main() { if (unallowedChangedFiles.length === 0) return - // Format into Markdown bulleted list to use in the PR comment const listUnallowedChangedFiles = unallowedChangedFiles.map((file) => `\n - ${file}`).join('') const listUnallowedFiles = filters.notAllowed .map((file: string) => `\n - ${file === '*' ? 'Anything in the root directory' : file}`)