diff --git a/.github/workflows/send-to-triage-board.yml b/.github/workflows/send-to-triage-board.yml index f5defa974e68..d5bda9b77e7f 100644 --- a/.github/workflows/send-to-triage-board.yml +++ b/.github/workflows/send-to-triage-board.yml @@ -1,8 +1,7 @@ name: Add new issues and PRs to central triage board -# **What it does**: Adds newly opened or reopened issues and pull requests in github/docs to the right place for triage, and stamps the item with today's date. -# **Why we have it**: To ensure incoming work in the public docs repo is triaged properly. -# **Who does it impact**: Writers, FRs. +# Adds new, reopened, and ready-for-review github/docs work to the Central Triage Group board. +# Sets today's date so first responders can triage it. on: issues: @@ -22,7 +21,7 @@ jobs: env: GITHUB_TOKEN: ${{ secrets.DOCS_BOT_PAT_BASE }} ITEM_URL: ${{ github.event.issue.html_url || github.event.pull_request.html_url }} - # Add to the Central Triage Group project board and set date to now + # These IDs point to the Central Triage Group board and its date field. PROJECT_NUMBER: '19598' PROJECT_ID: 'PVT_kwDNJr_OAJ4AfQ' DATE_FIELD_ID: 'PVTF_lADNJr_OAJ4Afc4IAbbv' diff --git a/.github/workflows/site-policy-reminder.yml b/.github/workflows/site-policy-reminder.yml index 7c335093c669..db8c3902894e 100644 --- a/.github/workflows/site-policy-reminder.yml +++ b/.github/workflows/site-policy-reminder.yml @@ -1,8 +1,7 @@ name: Site Policy Reminder -# **What it does**: Automated comment reminder on a PR to change the title for public consumption before merging and to run the Site Policy repo sync action -# **Why we have it**: Titles of merged PRs to Site Policies are sent to the public site-policy repo when the repos are synced -# **Who does it impact**: Everyone merging changes to Site Policies +# Site Policy PR titles appear in github/site-policy after sync, so they need public wording. +# The reminder tells admins when to run the sync action. on: pull_request: diff --git a/.github/workflows/site-policy-sync.yml b/.github/workflows/site-policy-sync.yml index 318a2de1d387..7cc23c8e72b1 100644 --- a/.github/workflows/site-policy-sync.yml +++ b/.github/workflows/site-policy-sync.yml @@ -1,12 +1,8 @@ name: Site policy sync -# **What it does**: Creates a branch in our site-policy repo with changes to site policy docs. -# **Why we have it**: We want to keep the site-policy repo up to date. -# **Who does it impact**: site-policy-admins and Developer Policy teams. +# Keeps github/site-policy current with internal site policy docs. -# Controls when the action will run. on: - # Triggers the workflow pull requests merged to the main branch pull_request: branches: - main diff --git a/.github/workflows/sme-review-tracking-issue.yml b/.github/workflows/sme-review-tracking-issue.yml index a1e33167c8e4..c22deb06fb50 100644 --- a/.github/workflows/sme-review-tracking-issue.yml +++ b/.github/workflows/sme-review-tracking-issue.yml @@ -1,14 +1,12 @@ name: Create SME review tracking issue -# **What it does**: Creates an SME review tracking issue when the `needs SME` label is applied to a PR or issue -# **Why we have it**: We do not want to manually create an SME review tracking issue when an SME review is needed -# **Who does it impact**: Hubbers +# Creates a technical-content tracking issue when a github/docs PR or issue gets the needs SME label. on: issues: types: - labeled - # Required in lieu of `pull_request` so that this workflow can query users in org to determine membership. + # pull_request_target gives fork PRs the DOCS_BOT_PAT_BASE secret to create the issue. pull_request_target: types: - labeled @@ -31,7 +29,6 @@ jobs: const issueNo = context.number || context.issue.number - // Create an issue in technical-content repo await github.rest.issues.create({ owner: 'github', repo: 'technical-content', diff --git a/.github/workflows/stale.yml b/.github/workflows/stale.yml index d008aa8e812a..5f9456e02dbb 100644 --- a/.github/workflows/stale.yml +++ b/.github/workflows/stale.yml @@ -1,8 +1,6 @@ name: Stale check for stalled pull requests in the docs-internal repository -# **What it does**: Identifies pull requests that have been inactive for 30 days. -# **Why we have it**: We want to avoid pull requests that are stalled and not being reviewed. -# **Who does it impact**: Everyone that works in the internal repository. +# Marks internal PRs stale after 30 inactive days and closes them 14 days later without a response. on: schedule: diff --git a/.github/workflows/sync-audit-logs.yml b/.github/workflows/sync-audit-logs.yml index c80d44bef7d3..f19d5753d4b7 100644 --- a/.github/workflows/sync-audit-logs.yml +++ b/.github/workflows/sync-audit-logs.yml @@ -1,8 +1,6 @@ name: Sync Audit Log data -# **What it does**: This updates our Audit Logs schema. -# **Why we have it**: We want our Audit Logs up to date. -# **Who does it impact**: Docs engineering, people reading Audit Logs. +# Keeps Audit Logs schema docs current with github/audit-log-allowlists. on: workflow_dispatch: @@ -13,7 +11,7 @@ permissions: contents: write pull-requests: write -# This allows a subsequently queued workflow run to interrupt previous runs +# Cancel older syncs so stale schema data does not open older PRs. concurrency: group: '${{ github.workflow }} @ ${{ github.event.pull_request.head.label || github.head_ref || github.ref }}' cancel-in-progress: true @@ -29,7 +27,7 @@ jobs: - name: Run updater script env: - # need to use a token from a user with access to github/audit-log-allowlists for this step + # DOCS_BOT_PAT_BASE can read github/audit-log-allowlists. GITHUB_TOKEN: ${{ secrets.DOCS_BOT_PAT_BASE }} run: | npm run sync-audit-log @@ -47,14 +45,12 @@ jobs: - name: Create and merge pull request env: - # Needed for gh GITHUB_TOKEN: ${{ secrets.DOCS_BOT_PAT_BASE }} run: | echo "Creating a new branch if needed..." branchname=audit-logs-schema-update-${{ steps.audit-log-allowlists.outputs.COMMIT_SHA }} remotesha=$(git ls-remote --heads origin $branchname) if [ -n "$remotesha" ]; then - # output is not empty, it means the remote branch exists echo "Branch $branchname already exists in 'github/docs-internal'. Exiting..." exit 0 fi @@ -94,14 +90,14 @@ jobs: --head=$branchname echo "Created pull request" - # can't approve your own PR, approve with Actions + # docs-bot cannot approve its own PR, so GITHUB_TOKEN approves it. echo "Approving pull request..." unset GITHUB_TOKEN gh auth login --with-token <<< "${{ secrets.GITHUB_TOKEN }}" gh pr review --approve echo "Approved pull request" - # Actions can't merge the PR so back to docs-bot to merge the PR + # GITHUB_TOKEN cannot enable auto-merge here, so docs-bot enables it. echo "Setting pull request to auto merge..." unset GITHUB_TOKEN gh auth login --with-token <<< "${{ secrets.DOCS_BOT_PAT_BASE }}" diff --git a/.github/workflows/sync-codeql-cli.yml b/.github/workflows/sync-codeql-cli.yml index ed091a9450ff..60c4b93636aa 100644 --- a/.github/workflows/sync-codeql-cli.yml +++ b/.github/workflows/sync-codeql-cli.yml @@ -1,10 +1,7 @@ name: Sync CodeQL CLI -# **What it does**: This workflow is run manually approximately every two weeks. -# When run, this workflow syncs the CodeQL CLI automated pipeline with the semmle-code -# repository, and creates a pull request if there are updates. -# **Why we have it**: So we can automate CodeQL CLI documentation. -# **Who does it impact**: Anyone making CodeQL CLI changes in `github/semmle-code`, and wanting to get them published on the docs site. +# Updates CodeQL CLI docs from github/semmle-code and opens a PR when files change. +# Run it manually about every two weeks to publish upstream changes. on: workflow_dispatch: @@ -19,7 +16,7 @@ permissions: contents: write pull-requests: write -# This allows a subsequently queued workflow run to interrupt previous runs +# Cancel older syncs so stale CodeQL CLI data does not open older PRs. concurrency: group: '${{ github.workflow }} @ ${{ github.event.pull_request.head.label || github.head_ref || github.ref }}' cancel-in-progress: true @@ -34,8 +31,6 @@ jobs: - name: Checkout semmle-code repo uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7.0.0 with: - # By default, only the most recent commit of the `main` branch - # will be checked out token: ${{ secrets.DOCS_BOT_PAT_BASE }} repository: github/semmle-code path: semmle-code @@ -53,13 +48,9 @@ jobs: - name: Install pandoc run: | - # Remove all previous pandoc versions sudo apt-get purge --auto-remove pandoc - # Download pandoc wget https://github.com/jgm/pandoc/releases/download/3.0.1/pandoc-3.0.1-1-amd64.deb - # Install pandoc sudo dpkg -i pandoc-3.0.1-1-amd64.deb - # Output the pandoc version installed pandoc -v rm pandoc-3.0.1-1-amd64.deb @@ -72,10 +63,9 @@ jobs: - name: Create pull request env: - # Needed for gh GITHUB_TOKEN: ${{ secrets.DOCS_BOT_PAT_BASE }} run: | - # If nothing to commit, exit now. It's fine. No orphans. + # Exit before branch creation so a no-op sync does not leave an orphan branch. changes=$(git diff --name-only | wc -l) untracked=$(git status --untracked-files --short | wc -l) if [[ $changes -eq 0 ]] && [[ $untracked -eq 0 ]]; then @@ -92,13 +82,10 @@ jobs: git add . git commit -m "Update CodeQL CLI data" - # Force-push to handle reruns where the branch already exists on the - # remote from a prior failed attempt. Plain --force is safe here - # because these branches are exclusively managed by this workflow. + # Force-push reruns over failed attempts; this workflow exclusively owns these branches. git push --force -u origin $branchname - # If a PR already exists for this branch (e.g. a previous run - # succeeded but the workflow still reported failure), skip creation. + # Skip PR creation when a prior run already opened one for this branch. existing_pr=$(gh pr list --repo github/docs-internal --head "$branchname" --json number --jq '.[0].number') if [[ -n "$existing_pr" ]]; then echo "Pull request #$existing_pr already exists for branch $branchname. Skipping PR creation." diff --git a/.github/workflows/sync-graphql.yml b/.github/workflows/sync-graphql.yml index c814ca667ee2..09784a8705dc 100644 --- a/.github/workflows/sync-graphql.yml +++ b/.github/workflows/sync-graphql.yml @@ -1,8 +1,6 @@ name: Sync GraphQL schema -# **What it does**: This updates our GraphQL schemas. -# **Why we have it**: We want our GraphQL docs up to date. -# **Who does it impact**: Docs engineering, people reading GraphQL docs. +# Keeps GraphQL docs current with schema changes from github/github. on: workflow_dispatch: @@ -30,7 +28,7 @@ jobs: - name: Run updater scripts id: sync env: - # need to use a token from a user with access to github/github for this step + # DOCS_BOT_PAT_BASE can read github/github. GITHUB_TOKEN: ${{ secrets.DOCS_BOT_PAT_BASE }} NODE_OPTIONS: '--max-old-space-size=8192' run: npm run sync-graphql @@ -38,13 +36,12 @@ jobs: id: create-pull-request uses: peter-evans/create-pull-request@98357b18bf14b5342f975ff684046ec3b2a07725 # pin @v8.0.0 env: - # Disable pre-commit hooks; they don't play nicely here + # Disable Husky because create-pull-request commits inside Actions. HUSKY: '0' with: - # Need to use a token with repo and workflow scopes for this step. - # Token should be a PAT because actions performed with GITHUB_TOKEN - # don't trigger other workflows and this action force pushes updates - # from the default branch. + # DOCS_BOT_PAT_BASE has repo and workflow scopes. + # GITHUB_TOKEN does not trigger follow-up workflows. + # create-pull-request force-pushes updates from the default branch. token: ${{ secrets.DOCS_BOT_PAT_BASE }} commit-message: 'Update GraphQL data files' title: GraphQL schema update diff --git a/.github/workflows/sync-llms-txt.yml b/.github/workflows/sync-llms-txt.yml index f2592e684c30..e6a86c16dd03 100644 --- a/.github/workflows/sync-llms-txt.yml +++ b/.github/workflows/sync-llms-txt.yml @@ -1,12 +1,7 @@ name: Sync llms.txt -# **What it does**: Generates docs.github.com/llms.txt, github.com/llms.txt, and -# github.com/llms-full.txt from the page catalog and popularity data, then -# opens PRs to update them. -# **Why we have it**: Agents discover docs through llms.txt; the page list keeps -# pace with what's actually popular without writers updating it by hand. -# **Who does it impact**: Docs consumers via agents, and anyone landing on -# github.com/llms.txt, github.com/llms-full.txt, or docs.github.com/llms.txt. +# Generates docs.github.com/llms.txt, github.com/llms.txt, and github.com/llms-full.txt. +# Uses the page catalog and popularity data, then opens PRs to update them. on: workflow_dispatch: @@ -59,8 +54,6 @@ jobs: --output /tmp/monolith-llms.txt echo "Generated monolith llms.txt ($(wc -l < /tmp/monolith-llms.txt) lines, $(wc -c < /tmp/monolith-llms.txt) bytes)" - # ---------- PR to docs-internal: update data/llms-txt/docs.md ---------- - - name: Diff docs llms.txt against committed copy id: diff_docs run: | @@ -97,11 +90,8 @@ jobs: git config user.name "docs-bot" git config user.email "77750099+docs-bot@users.noreply.github.com" git add data/llms-txt/docs.md - # diff_docs compares against main, but the sync branch may already - # exist with this exact content (open PR from a prior run). In that - # case there is nothing new to stage, and `git commit` would exit 1 - # and fail the whole workflow. Skip the commit and push when the - # branch is already up to date. + # diff_docs compares against main, but an open sync-branch PR can already hold this content. + # Skip commit and push because git commit exits 1 with nothing staged. if git diff --cached --quiet; then echo "Sync branch already has the latest generated docs.md; nothing to commit." else @@ -135,8 +125,6 @@ jobs: --draft \ --label "llm-generated" - # ---------- PR to github/github: update public/llms*.txt ---------- - - name: Fetch current public llms files from github/github id: fetch_monolith env: diff --git a/.github/workflows/sync-openapi.yml b/.github/workflows/sync-openapi.yml index eb15a2db3d91..0349f529c5a1 100644 --- a/.github/workflows/sync-openapi.yml +++ b/.github/workflows/sync-openapi.yml @@ -1,8 +1,7 @@ name: Sync OpenAPI schema -# **What it does**: Syncs the REST, Webhooks, and GitHub Apps automated pipelines with the github/rest-api-description repository, and creates a pull request if there are updates to any of the data files we generate from the OpenAPI. Runs on a weekday schedule or a `sync-openapi` repository dispatch. -# **Why we have it**: So we can automate updates to REST, Webhooks, and GitHub Apps documentation -# **Who does it impact**: Anyone making OpenAPI changes in `github/github`, and wanting to get them published on the docs site. +# Updates REST, Webhooks, and GitHub Apps docs from github/rest-api-description. +# Opens a PR when generated OpenAPI data files change. on: workflow_dispatch: @@ -21,7 +20,7 @@ permissions: contents: write pull-requests: write -# This allows a subsequently queued workflow run to interrupt previous runs +# Cancel older syncs so stale OpenAPI data does not open older PRs. concurrency: group: '${{ github.workflow }} @ ${{ github.event.pull_request.head.label || github.head_ref || github.ref }}' cancel-in-progress: true @@ -34,12 +33,9 @@ jobs: - name: Checkout repository code uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7.0.0 - # Check out a nested repository inside of previous checkout - name: Checkout rest-api-description repo uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7.0.0 with: - # By default, only the most recent commit of the `main` branch - # will be checked out repository: github/rest-api-description path: rest-api-description ref: ${{ inputs.SOURCE_BRANCH || github.event.client_payload.ref || 'main' }} @@ -48,7 +44,6 @@ jobs: - name: Sync the REST, Webhooks, and GitHub Apps schemas env: - # Needed for gh GITHUB_TOKEN: ${{ secrets.DOCS_BOT_PAT_BASE }} NODE_OPTIONS: '--max-old-space-size=8192' run: | @@ -72,10 +67,9 @@ jobs: - name: Create pull request env: - # Needed for gh GITHUB_TOKEN: ${{ secrets.DOCS_BOT_PAT_BASE }} run: | - # If nothing to commit, exit now. It's fine. No orphans. + # Exit before branch creation so a no-op sync does not leave an orphan branch. changes=$(git diff --name-only | wc -l) if [[ $changes -eq 0 ]]; then echo "There are no changes to commit after running 'npm run sync-rest'. Exiting..." @@ -89,7 +83,6 @@ jobs: remotesha=$(git ls-remote --heads origin $branchname) if [ -n "$remotesha" ]; then - # output is not empty, it means the remote branch exists echo "Branch $branchname already exists in 'github/docs-internal'. Exiting..." exit 0 fi diff --git a/.github/workflows/sync-sdk-docs.yml b/.github/workflows/sync-sdk-docs.yml index 92796217453e..31394c19c172 100644 --- a/.github/workflows/sync-sdk-docs.yml +++ b/.github/workflows/sync-sdk-docs.yml @@ -1,11 +1,12 @@ name: 'Sync Copilot SDK docs' +# Syncs Copilot SDK docs after copilot-sdk docs changes and opens an update PR. +# Supports manual dry runs and pull request validation without publishing. + on: - # Event-driven sync — triggered by copilot-sdk when docs/ changes are pushed repository_dispatch: types: [sync-sdk-docs] - # Manual trigger for on-demand syncs and testing workflow_dispatch: inputs: dry_run: @@ -19,7 +20,6 @@ on: default: 'main' type: string - # PR validation — dry-run only, verifies scripts work on CI pull_request: types: [opened, synchronize, reopened] paths: @@ -50,12 +50,8 @@ jobs: - name: Checkout docs-internal uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7.0.0 - # `preserve-redirects.ts` reads the pre-sync state from `git HEAD` to learn - # which URLs are currently live. That is only a valid baseline when HEAD is - # the published branch. `pull_request` runs are safe because they never - # publish, but a `workflow_dispatch` from another branch would publish while - # comparing against that branch's tree, writing stale redirects into a real - # PR. Fail early rather than let the run reach the push step. + # Publishing runs must start from the default branch because preserve-redirects.ts reads + # live URLs from git HEAD. workflow_dispatch from another branch could write stale redirects. - name: Verify publishing runs start from the default branch if: github.event_name != 'pull_request' && inputs.dry_run != 'true' run: | @@ -98,10 +94,9 @@ jobs: - name: Copy SDK docs run: | mkdir -p "$SDK_DOCS_TARGET" - # Pages relocated out of this tree into hand-authored content are not - # excluded here — they are removed by the RELOCATED_PAGES map in - # src/workflows/sync-sdk-docs/normalize-sdk-docs.ts, which also - # repoints inbound links at their new URLs. + # Keep relocated pages in the rsync input. RELOCATED_PAGES in + # src/workflows/sync-sdk-docs/normalize-sdk-docs.ts removes them and + # repoints inbound links. rsync -av --exclude='.validation/' --exclude='developer-docs/' "$SDK_TMP/docs/" "$SDK_DOCS_TARGET/" echo "Copied $(find "$SDK_DOCS_TARGET" -name '*.md' | wc -l | tr -d ' ') markdown files" @@ -113,16 +108,10 @@ jobs: - name: Preserve redirects run: | - # `--git-ref HEAD` is the record of which URLs are currently live. On - # publishing runs a preceding step has verified HEAD is the default - # branch, and the sync branch is only created later with `checkout -B`. - # On `pull_request` runs HEAD is the merge commit instead, which is fine - # because those runs are dry-run only. - # - # A removed page that needs a redirect decision should still produce a - # PR, because the PR is where that decision gets made and committed. The - # script still writes the at-risk URLs and a copy-pasteable - # `redirect_from` block to the run summary either way. + # --git-ref HEAD points preserve-redirects.ts at the currently live URLs. Publishing runs + # have already verified HEAD is the default branch; pull_request runs stay dry-run only. + # Removed pages still produce a PR for redirect review. The script writes at-risk URLs and + # a copy-pasteable redirect_from block to the run summary either way. npx tsx src/workflows/sync-sdk-docs/preserve-redirects.ts \ --sdk-docs-dir "$SDK_DOCS_TARGET" \ --git-ref HEAD @@ -131,7 +120,7 @@ jobs: env: PUPPETEER_CHROMIUM_REVISION: '' run: | - # Puppeteer needs --no-sandbox on GitHub Actions runners + # Puppeteer needs --no-sandbox on GitHub Actions runners. echo '{ "args": ["--no-sandbox", "--disable-setuid-sandbox"] }' > /tmp/puppeteer-config.json npx tsx src/workflows/sync-sdk-docs/convert-mermaid.ts \ --sdk-docs-dir "$SDK_DOCS_TARGET" \ @@ -145,7 +134,6 @@ jobs: echo "No assets directory — nothing to clean." exit 0 fi - # Collect image filenames referenced in the current SDK docs REFERENCED=$(grep -roh '/assets/images/help/copilot/copilot-sdk/[^)]*' "$SDK_DOCS_TARGET" \ | sed 's|.*/||' | sort -u) STALE=0 @@ -190,7 +178,6 @@ jobs: git diff --cached --stat fi - # --- PR-only: upload artifacts for review --- - name: Upload normalized docs (PR validation) if: github.event_name == 'pull_request' uses: actions/upload-artifact@bbbca2ddaa5d8feaa63e36b76fdaad77386f024f # v7.0.0 @@ -201,7 +188,6 @@ jobs: ${{ env.ASSETS_TARGET }} retention-days: 7 - # --- Push and PR (only on dispatch, not dry-run, and changes exist) --- - name: Commit and push if: >- env.has_changes == 'true' @@ -213,7 +199,7 @@ jobs: git config user.name "github-actions[bot]" git config user.email "41898282+github-actions[bot]@users.noreply.github.com" - # Fetch the sync branch if it exists so force-with-lease knows the remote state + # Fetch the sync branch if it exists so force-with-lease knows the remote state. git fetch origin "$SYNC_BRANCH" 2>/dev/null || true git checkout -B "$SYNC_BRANCH" diff --git a/.github/workflows/sync-secret-scanning.yml b/.github/workflows/sync-secret-scanning.yml index 730464d206ce..bd35f814b776 100644 --- a/.github/workflows/sync-secret-scanning.yml +++ b/.github/workflows/sync-secret-scanning.yml @@ -1,8 +1,6 @@ name: Sync Secret Scanning data -# **What it does**: This updates the data used by the secret scanning patterns page. -# **Why we have it**: To automate updates to the secret scanning pattern data in our public-facing documentation. -# **Who does it impact**: Docs engineering, content writers. +# Keeps the public secret scanning patterns page current with github/token-scanning-service. on: workflow_dispatch: @@ -13,7 +11,7 @@ permissions: contents: write pull-requests: write -# This allows a subsequently queued workflow run to interrupt previous runs +# Cancel older syncs so stale secret scanning data does not open older PRs. concurrency: group: '${{ github.workflow }} @ ${{ github.event.pull_request.head.label || github.head_ref || github.ref }}' cancel-in-progress: true @@ -30,8 +28,7 @@ jobs: - name: Sync secret scanning data id: secret-scanning-sync env: - # need to use a token from a user with access to - # github/token-scanning-service for this step + # DOCS_BOT_PAT_BASE can read github/token-scanning-service. GITHUB_TOKEN: ${{ secrets.DOCS_BOT_PAT_BASE }} run: | npm run sync-secret-scanning @@ -40,10 +37,10 @@ jobs: id: create-pull-request uses: peter-evans/create-pull-request@98357b18bf14b5342f975ff684046ec3b2a07725 # pin @v8.0.0 env: - # Disable pre-commit hooks; they don't play nicely here + # Disable Husky because create-pull-request commits inside Actions. HUSKY: '0' with: - # need to use a token with repo and workflow scopes for this step + # DOCS_BOT_PAT_BASE has repo and workflow scopes. token: ${{ secrets.DOCS_BOT_PAT_BASE }} commit-message: 'Add updated secret scanning data' title: Sync secret scanning data diff --git a/.github/workflows/test-changed-content.yml b/.github/workflows/test-changed-content.yml index 29c9cd9704b6..9a4eddc71a5e 100644 --- a/.github/workflows/test-changed-content.yml +++ b/.github/workflows/test-changed-content.yml @@ -1,16 +1,11 @@ name: Test changed content -# **What it does**: Runs the vitest tests for changed and deleted content files. -# **Why we have it**: Use GitHub Actions to run tests on changed content files. -# **Who does it impact**: Docs engineering, open-source engineering contributors. +# Renders changed pages and checks that deleted or renamed URLs still resolve before main-bound PRs merge. on: pull_request: branches: - # This is important! If you make a PR against a megabranch, you - # might actually want to delete a file without setting up a - # redirect in its place. But if it's going into `main` we'll - # want to make sure that doesn't happen. + # Megabranch PRs may skip deleted-page checks, but main-bound PRs must keep old URLs resolving. - main paths: - 'content/**' @@ -24,8 +19,7 @@ jobs: runs-on: ubuntu-latest if: ${{ github.repository == 'github/docs-internal' || github.repository == 'github/docs' }} steps: - # Each of these ifs needs to be repeated at each step to make sure the required check still runs - # Even if if doesn't do anything + # Repeat each if on its step so skipped work still leaves the required check present. - name: Check out repo uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7.0.0 with: @@ -49,16 +43,14 @@ jobs: uses: tj-actions/changed-files@22103cc46bda19c2b464ffe86db46df6922fd323 # v47.0.5 with: files: 'content/**' - # Needed to expose `all_old_new_renamed_files` (old,new pairs for renames). - # Without this, files git classifies as renames (status R) are invisible to - # the deleted-file redirect check below and old URLs can silently 404. + # all_old_new_renamed_files exposes old,new pairs for renames. + # Without it, git status R files skip the deleted-file redirect check and old URLs 404. include_all_old_new_renamed_files: true - name: Run tests env: CHANGED_FILES: ${{ steps.changed_files.outputs.all_modified_files }} DELETED_FILES: ${{ steps.changed_files.outputs.deleted_files }} - # Space-separated `oldPath,newPath` pairs. The test treats the old paths - # like deleted files so missing redirects on renames are caught. + # Space-separated oldPath,newPath pairs; the test checks each old path like a deleted file. RENAMED_FILES: ${{ steps.changed_files.outputs.all_old_new_renamed_files }} run: npm test -- src/content-render/tests/render-changed-and-deleted-files.ts diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 640859c1d829..58dfd68c535c 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -1,11 +1,6 @@ name: Test -# **What it does**: Runs our tests. -# **Why we have it**: We want our tests to pass before merging code. -# **Who does it impact**: Docs engineering, open-source engineering contributors. -# -# For a catalog of what each suite covers and how risky it is to admin-merge -# past it when red, see src/tests/SUITES.md. +# Runs required test suites before merge. src/tests/SUITES.md explains coverage and merge risk. on: workflow_dispatch: @@ -16,14 +11,13 @@ permissions: contents: read pull-requests: read -# This allows a subsequently queued workflow run to interrupt previous runs +# Cancel older test runs for the same ref so newer commits get the runner. concurrency: group: '${{ github.workflow }} @ ${{ github.event.pull_request.head.label || github.head_ref || github.ref }}' cancel-in-progress: true env: - # Setting this will activate the vitest tests that depend on actually - # sending real search queries to Elasticsearch + # ELASTICSEARCH_URL enables Vitest suites that send real search queries. ELASTICSEARCH_URL: http://localhost:9200/ jobs: @@ -35,11 +29,9 @@ jobs: strategy: fail-fast: false matrix: - # Note that *if you add* to this, remember to also add that - # to the **required checks** in the branch protection rules. + # Add new matrix suites to branch protection required checks too. name: - # Every directory in src/ is listed here. - # A commented out entry has no test files of its own. + # Lists every src directory. Commented-out entries have no suite-specific tests. # - ai-tools # - app - archives @@ -47,7 +39,7 @@ jobs: - assets - audit-logs - automated-pipelines - # - codeql-cli # enable once github/docs-internal#63239 removes the broken scratch test + # - codeql-cli # Enable when the broken scratch test is removed. # - codeql-queries - color-schemes - content-linter @@ -85,7 +77,7 @@ jobs: - webhooks - workflows - # The languages suite only runs on docs-internal + # The languages suite only runs on docs-internal. isPrivateRepo: - ${{ github.repository == 'github/docs-internal' }} exclude: @@ -93,8 +85,7 @@ jobs: isPrivateRepo: false steps: - # Each of these ifs needs to be repeated at each step to make sure the required check still runs - # Even if if doesn't do anything + # Repeat each if on its step so skipped work still leaves the required check present. - name: Check out repo uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7.0.0 with: @@ -115,14 +106,12 @@ jobs: if: ${{ matrix.name == 'fixtures' }} run: npm run copy-fixture-data -- --check - # This keeps our fixture content/data in check - name: Check the test fixture content (if applicable) if: ${{ matrix.name == 'fixtures' }} env: ROOT: src/fixtures/fixtures run: | - # If either of these fail, it means our fixture content's internal - # links can and should be updated. + # A failure means fixture content has stale internal links the dry run can update. npm run update-internal-links -- --dry-run --check --strict \ src/fixtures/fixtures/content \ --exclude src/fixtures/fixtures/content/get-started/foo/typo-autotitling.md \ @@ -151,19 +140,17 @@ jobs: run: npm run build - uses: ./.github/actions/warmup-remotejson-cache - # Only the 'routing' tests include end-to-end tests about - # archived enterprise server URLs. + # Only routing tests cover archived enterprise server URLs. if: ${{ matrix.name == 'redirects' }} - uses: ./.github/actions/precompute-pageinfo - # Only the 'pageinfo' tests include end-to-end tests about this. + # Only pageinfo tests cover precomputed page info. if: ${{ matrix.name == 'article-api' }} env: ROOT: src/fixtures/fixtures - name: Index fixtures into the local Elasticsearch - # For the sake of saving time, only run this step if the group - # is one that will run tests against an Elasticsearch on localhost. + # Run indexing only for suites that query the local Elasticsearch service. if: ${{ matrix.name == 'search' || matrix.name == 'languages' }} run: npm run index-test-fixtures @@ -171,14 +158,11 @@ jobs: env: DIFF_FILE: get_diff_files.txt CHANGELOG_CACHE_FILE_PATH: src/fixtures/fixtures/changelog-feed.json - # By default, when `process.env.NODE_ENV === 'test'` it forces the - # tests run only in English. The exception is the - # `languages` suite which needs all languages to be set up. + # NODE_ENV=test forces English-only tests; the languages suite needs every language. ENABLED_LANGUAGES: ${{ matrix.name == 'languages' && 'all' || '' }} ROOT: ${{ (matrix.name == 'fixtures' || matrix.name == 'article-api' || matrix.name == 'landings' ) && 'src/fixtures/fixtures' || '' }} TRANSLATIONS_FIXTURE_ROOT: ${{ (matrix.name == 'fixtures' || matrix.name == 'article-api') && 'src/fixtures/fixtures/translations' || '' }} - # Enable debug logging when "Re-run jobs with debug logging" is used in GitHub Actions UI - # This will output additional timing and path information to help diagnose timeout issues + # RUNNER_DEBUG enables timing and path logs when Actions reruns jobs with debug logging. RUNNER_DEBUG: ${{ runner.debug }} VITEST_FLAGS: ${{ matrix.name == 'article-api' && '--no-file-parallelism --maxWorkers=1' || '' }} run: npm test -- $VITEST_FLAGS src/${{ matrix.name }}/tests/ diff --git a/.github/workflows/triage-issue-comments.yml b/.github/workflows/triage-issue-comments.yml index d56749eb48c3..70d836655b5e 100644 --- a/.github/workflows/triage-issue-comments.yml +++ b/.github/workflows/triage-issue-comments.yml @@ -1,8 +1,6 @@ name: Triage new issue comments -# **What it does**: Adds label triage to new issue comments in the open source repository. -# **Why we have it**: Update open source project board for review. -# **Who does it impact**: Docs open source. +# Labels public issues for triage when external contributors add new comments. on: issue_comment: diff --git a/.github/workflows/triage-issues.yml b/.github/workflows/triage-issues.yml index 55b333e5413c..e22a900f0b6d 100644 --- a/.github/workflows/triage-issues.yml +++ b/.github/workflows/triage-issues.yml @@ -1,8 +1,6 @@ name: Triage new issues -# **What it does**: Add the 'triage' label to new issues in the open source repository. -# **Why we have it**: We want to make sure that new issues are triaged and assigned to the right team. -# **Who does it impact**: Docs open source. +# Labels opened or reopened public docs issues for triage and team assignment. on: issues: diff --git a/.github/workflows/triage-pull-requests.yml b/.github/workflows/triage-pull-requests.yml index 39419711d2c5..bb001f429559 100644 --- a/.github/workflows/triage-pull-requests.yml +++ b/.github/workflows/triage-pull-requests.yml @@ -1,11 +1,9 @@ name: Triage new pull requests -# **What it does**: Adds triage label to new pull requests in the open source repository. -# **Why we have it**: Update project board for new pull requests for triage. -# **Who does it impact**: Docs open source. +# Labels opened or reopened public docs pull requests for triage. on: - # Needed in lieu of `pull_request` so that PRs from a fork can be triaged. + # pull_request_target lets PRs from forks be triaged. pull_request_target: types: - reopened diff --git a/.github/workflows/triage-stale-check.yml b/.github/workflows/triage-stale-check.yml index e7d2db0765e4..83a8d585cff7 100644 --- a/.github/workflows/triage-stale-check.yml +++ b/.github/workflows/triage-stale-check.yml @@ -1,8 +1,6 @@ name: Stale check for no activity -# **What it does**: Provides more aggressive stale checks in the open repo. -# **Why we have it**: In the open repo, we want more aggressive stale checking. -# **Who does it impact**: Anyone working in the open repo. +# Applies shorter stale windows in the public docs repo. on: schedule: diff --git a/.github/workflows/triage-unallowed-contributions.yml b/.github/workflows/triage-unallowed-contributions.yml index 0905051a8397..549b90437251 100644 --- a/.github/workflows/triage-unallowed-contributions.yml +++ b/.github/workflows/triage-unallowed-contributions.yml @@ -1,11 +1,9 @@ name: Check unallowed file changes -# **What it does**: If someone changes some files in the open repo, we prevent the pull request from merging. -# **Why we have it**: Some files can only be changed in the internal repository for security and workflow reasons. -# **Who does it impact**: Open source contributors. +# Blocks public pull requests that change files managed only in docs-internal. on: - # Needed in lieu of `pull_request` so that PRs from a fork can be notified of unallowed changes. + # pull_request_target lets PRs from forks receive unallowed-change comments. pull_request_target: permissions: @@ -30,22 +28,19 @@ jobs: uses: dorny/paths-filter@fbd0ab8f3e69293af611ebaee6363fc25e6d187d # v4.0.1 id: filter with: - # Base branch used to get changed files + # Compare against main to match the public docs repo base. base: 'main' - # Enables setting an output in the format in `${FILTER_NAME}_files - # with the names of the matching files formatted as JSON array + # list-files=json emits matching paths in FILTER_NAME_files outputs. list-files: json - # Returns list of changed files matching each filter filters: 'src/workflows/unallowed-contribution-filters.yml' - name: Set up Node and dependencies if: ${{ steps.filter.outputs.notAllowed == 'true' || steps.filter.outputs.contentTypes == 'true' }} uses: ./.github/actions/node-npm-setup - # When there are changes to files we can't accept, leave a comment - # explaining this to the PR author, and why their PR will close + # Comment when rejected file changes will close the PR, so the author knows why. - name: "Comment about changes we can't accept" if: ${{ steps.filter.outputs.notAllowed == 'true' || steps.filter.outputs.contentTypes == 'true' }} run: npm run unallowed-contributions diff --git a/.github/workflows/validate-asset-images.yml b/.github/workflows/validate-asset-images.yml index e29eb9eec307..f2e9a770c31d 100644 --- a/.github/workflows/validate-asset-images.yml +++ b/.github/workflows/validate-asset-images.yml @@ -1,8 +1,6 @@ name: Validate asset images -# **What it does**: Run ./src/assets/scripts/validate-asset-images.ts on all images in assets/ -# **Why we have it**: To protect from innocent and potentially malicious bad image assets -# **Who does it impact**: Docs content. +# Rejects malformed or risky image assets before they ship. on: workflow_dispatch: diff --git a/.github/workflows/validate-github-github-docs-urls.yml b/.github/workflows/validate-github-github-docs-urls.yml index b9577e49f7d4..b21893bb273b 100644 --- a/.github/workflows/validate-github-github-docs-urls.yml +++ b/.github/workflows/validate-github-github-docs-urls.yml @@ -1,23 +1,12 @@ name: Validate github/github docs URLs -# **What it does**: Checks the URLs in docs-urls.json in github/github -# **Why we have it**: To ensure the values in docs-urls.json are perfect. -# **Who does it impact**: Docs content. +# Checks github/github docs-urls.json entries against docs-internal content. on: workflow_dispatch: schedule: - cron: '20 16 * * 1' # Run every Monday at 16:20 UTC / 8:20 PST - # See https://gh.io/AAsyyao before uncommenting: - # pull_request: - # paths: - # - 'content/**' - # # In case a relevant dependency changes - # - 'package*.json' - # # The scripts - # - 'src/links/scripts/validate-github-github-docs-urls/**' - # # The workflow - # - .github/workflows/validate-github-github-docs-urls.yml + # Pull request triggers need https://gh.io/AAsyyao setup before they can run safely. permissions: contents: read @@ -46,8 +35,7 @@ jobs: - name: Run validation run: | - # This will generate a .json file which we can use to - # do other things in other steps. + # checks.json feeds the later update and comment steps. npm run validate-github-github-docs-urls -- validate \ --output checks.json \ --ignore-not-found \ @@ -79,10 +67,7 @@ jobs: git commit -a -m "Update Docs URLs from automation ($current_daystamp)" git push origin "$branch_name" - # XXX TODO - # Perhaps post an issue somewhere, about that the fact that this - # branch has been created and now needs to be turned into a PR - # that some human can take responsibility for. + # Scheduled and manual runs create update-docs-urls branches for a human to turn into PRs. - name: Clean up old branches in github/github if: ${{ github.event_name == 'schedule' || github.event_name == 'workflow_dispatch' }} @@ -94,15 +79,10 @@ jobs: echo "To see them all, go to:" echo "https://github.com/github/github/branches/all?query=update-docs-urls-" - # If a PR comes along to github/docs-internal that causes some - # URLs in docs-urls.json (in github/github) to now fail, then - # we'll want to make the PR author+reviewer aware of this. - # For example, you moved a page without setting up a redirect. - # Or you edited a heading that now breaks a URL with fragment. - # In the latter case, you might want to update the URL in docs-urls.json - # after this PR has landed, or consider using `` as a - # workaround for the time being. - # First, gather the URLs that were relevant + # When a PR breaks docs-urls.json entries in github/github, comment for the + # author and reviewer. + # Common causes are moved pages without redirects or edited headings that break URL fragments. + # The fix can update docs-urls.json after merge or add a stable anchor. - name: Get changed content/data files if: ${{ github.event_name == 'pull_request' }} id: changed_files diff --git a/.github/workflows/validate-openapi-check.yml b/.github/workflows/validate-openapi-check.yml index b602126df401..aa0edd90bef4 100644 --- a/.github/workflows/validate-openapi-check.yml +++ b/.github/workflows/validate-openapi-check.yml @@ -1,8 +1,6 @@ name: Validate OpenAPI Check Docker -# **What it does**: Tests building and running the OpenAPI check Docker container -# **Why we have it**: To ensure the Dockerfile and openapi-check script work correctly -# **Who does it impact**: Docs engineering. +# Tests the OpenAPI check Dockerfile and script before related changes merge. on: workflow_dispatch: @@ -16,7 +14,7 @@ on: - 'package.json' - 'package-lock.json' - 'tsconfig.json' - # Self-test + # Re-run this workflow when its own definition changes. - '.github/workflows/validate-openapi-check.yml' permissions: diff --git a/.github/workflows/zizmor.yml b/.github/workflows/zizmor.yml index 2b13f0935714..3052ec6ca99b 100644 --- a/.github/workflows/zizmor.yml +++ b/.github/workflows/zizmor.yml @@ -1,8 +1,6 @@ name: Workflow security lint -# **What it does**: Runs zizmor to detect security issues in GitHub Actions workflows. -# **Why we have it**: To catch injection vulnerabilities and other security misconfigurations before they ship. -# **Who does it impact**: Docs engineering. +# Runs zizmor so workflow injection vulnerabilities and security misconfigurations fail CI. on: pull_request: diff --git a/content/copilot/reference/ai-models/model-hosting.md b/content/copilot/reference/ai-models/model-hosting.md index b8d8c3502c71..4765f740441b 100644 --- a/content/copilot/reference/ai-models/model-hosting.md +++ b/content/copilot/reference/ai-models/model-hosting.md @@ -46,6 +46,7 @@ Used for: * {% data variables.copilot.copilot_claude_haiku_45 %} * {% data variables.copilot.copilot_claude_sonnet_46 %} * {% data variables.copilot.copilot_claude_sonnet_5 %} +* {% data variables.copilot.copilot_claude_sonnet_55 %} * {% data variables.copilot.copilot_claude_opus_47 %} * {% data variables.copilot.copilot_claude_opus_48 %} * {% data variables.copilot.copilot_claude_opus_48_fast %} diff --git a/content/copilot/reference/ai-models/supported-models.md b/content/copilot/reference/ai-models/supported-models.md index e18f9c2166ef..cc4530968d72 100644 --- a/content/copilot/reference/ai-models/supported-models.md +++ b/content/copilot/reference/ai-models/supported-models.md @@ -84,6 +84,7 @@ Choosing a larger context window or higher reasoning will impact {% data variabl | {% data variables.copilot.copilot_claude_opus_5 %} | {% octicon "check" aria-label="Supported" %} | {% octicon "check" aria-label="Supported" %} | | {% data variables.copilot.copilot_claude_opus_55 %} | {% octicon "check" aria-label="Supported" %} | {% octicon "check" aria-label="Supported" %} | | {% data variables.copilot.copilot_claude_sonnet_5 %} | {% octicon "check" aria-label="Supported" %} | {% octicon "check" aria-label="Supported" %} | +| {% data variables.copilot.copilot_claude_sonnet_55 %} | {% octicon "check" aria-label="Supported" %} | {% octicon "check" aria-label="Supported" %} | | {% data variables.copilot.copilot_claude_opus_48_fast %} | {% octicon "x" aria-label="Not supported" %} | {% octicon "check" aria-label="Supported" %} | | {% data variables.copilot.copilot_claude_fable_5 %} | {% octicon "check" aria-label="Supported" %} | {% octicon "check" aria-label="Supported" %} | | {% data variables.copilot.copilot_claude_fable_51 %} | {% octicon "check" aria-label="Supported" %} | {% octicon "check" aria-label="Supported" %} | @@ -143,6 +144,7 @@ Some {% data variables.product.prodname_copilot_short %} models require minimum | {% data variables.copilot.copilot_claude_opus_5 %} | `v1.128.0` | `17.14.22` | TBD | TBD | TBD | | {% data variables.copilot.copilot_claude_opus_55 %} | TBD | `17.14.6` | TBD | TBD | TBD | | {% data variables.copilot.copilot_claude_sonnet_5 %} | `v1.124` | `17.14.6` | TBD | TBD | TBD | +| {% data variables.copilot.copilot_claude_sonnet_55 %} | TBD | `17.14.6` | TBD | TBD | TBD | | {% data variables.copilot.copilot_claude_fable_5 %} | `v1.124` | `17.14.6` | TBD | TBD | TBD | | {% data variables.copilot.copilot_claude_fable_51 %} | TBD | TBD | TBD | TBD | TBD | | {% data variables.copilot.copilot_kimi_k27_code %} | `v1.127` | `17.14.6` | `1.9.1-251` | TBD | TBD | diff --git a/data/reusables/copilot/copilot-cloud-agent-non-auto-models.md b/data/reusables/copilot/copilot-cloud-agent-non-auto-models.md index be79f8ae57ec..d4d540cf1cae 100644 --- a/data/reusables/copilot/copilot-cloud-agent-non-auto-models.md +++ b/data/reusables/copilot/copilot-cloud-agent-non-auto-models.md @@ -1,6 +1,7 @@ * {% data variables.copilot.copilot_claude_opus_47 %} * {% data variables.copilot.copilot_claude_opus_5 %} * {% data variables.copilot.copilot_claude_opus_55 %} +* {% data variables.copilot.copilot_claude_sonnet_55 %} * {% data variables.copilot.copilot_claude_haiku_45 %} * {% data variables.copilot.copilot_gemini_35_flash %} * {% data variables.copilot.copilot_gemini_36_flash %} diff --git a/data/tables/copilot/model-comparison.yml b/data/tables/copilot/model-comparison.yml index 37d8fc3448e4..e809ca6b35ac 100644 --- a/data/tables/copilot/model-comparison.yml +++ b/data/tables/copilot/model-comparison.yml @@ -114,6 +114,11 @@ excels_at: Complex problem-solving challenges, sophisticated reasoning further_reading: '[Claude Sonnet 5 model card](https://www-cdn.anthropic.com/9e6a1044980d8c4ed85669faf9c2a8342e2e9f1e/Claude%20Sonnet%205%20System%20Card.pdf)' +- name: Claude Sonnet 5.5 + task_area: General-purpose coding and agent tasks + excels_at: Efficient task completion with fewer steps, tokens, and tool calls + further_reading: 'Coming soon' + # Google - name: Gemini 3.5 Flash task_area: Fast help with simple or repetitive tasks diff --git a/data/tables/copilot/model-release-status.yml b/data/tables/copilot/model-release-status.yml index e83b05141cab..2cee9f7eab8d 100644 --- a/data/tables/copilot/model-release-status.yml +++ b/data/tables/copilot/model-release-status.yml @@ -105,6 +105,10 @@ provider: 'Anthropic' release_status: 'GA' +- name: 'Claude Sonnet 5.5' + provider: 'Anthropic' + release_status: 'GA' + # Google models - name: 'Gemini 3.5 Flash' diff --git a/data/tables/copilot/model-supported-clients.yml b/data/tables/copilot/model-supported-clients.yml index 2d2807e3a9fc..60a7eb2cbf74 100644 --- a/data/tables/copilot/model-supported-clients.yml +++ b/data/tables/copilot/model-supported-clients.yml @@ -104,6 +104,15 @@ xcode: true jetbrains: true +- name: Claude Sonnet 5.5 + dotcom: true + cli: true + vscode: true + vs: true + eclipse: true + xcode: true + jetbrains: true + - name: Gemini 3.5 Flash dotcom: false cli: true diff --git a/data/tables/copilot/model-supported-plans.yml b/data/tables/copilot/model-supported-plans.yml index 32887443a1a2..1452069b07a9 100644 --- a/data/tables/copilot/model-supported-plans.yml +++ b/data/tables/copilot/model-supported-plans.yml @@ -82,6 +82,13 @@ business: true enterprise: true +- name: Claude Sonnet 5.5 + pro: true + pro_plus: true + max: true + business: true + enterprise: true + - name: Gemini 3.5 Flash pro: true pro_plus: true diff --git a/data/tables/copilot/models-and-pricing.yml b/data/tables/copilot/models-and-pricing.yml index b68d523a6e4e..795f366a8493 100644 --- a/data/tables/copilot/models-and-pricing.yml +++ b/data/tables/copilot/models-and-pricing.yml @@ -313,6 +313,15 @@ output: $10.00 cache_write: $2.50 +- model: Claude Sonnet 5.5 + provider: anthropic + release_status: GA + category: Versatile + input: $2.00 + cached_input: $0.20 + output: $10.00 + cache_write: $2.50 + - model: Claude Opus 4.8 (fast mode) (preview) provider: anthropic release_status: GA diff --git a/data/variables/copilot.yml b/data/variables/copilot.yml index 996425dd1e0a..42d046597d72 100644 --- a/data/variables/copilot.yml +++ b/data/variables/copilot.yml @@ -201,6 +201,7 @@ copilot_claude_sonnet_40: 'Claude Sonnet 4' copilot_claude_sonnet_45: 'Claude Sonnet 4.5' copilot_claude_sonnet_46: 'Claude Sonnet 4.6' copilot_claude_sonnet_5: 'Claude Sonnet 5' +copilot_claude_sonnet_55: 'Claude Sonnet 5.5' # Gemini: copilot_gemini: 'Gemini' copilot_gemini_flash: 'Gemini 2.0 Flash' diff --git a/src/app/layout.tsx b/src/app/layout.tsx index 9d823c65d72a..c4df4449bac7 100644 --- a/src/app/layout.tsx +++ b/src/app/layout.tsx @@ -1,6 +1,6 @@ -// Stub layout kept so Next.js enables App Router mode, which relaxes -// the "global CSS only in _app" restriction needed by transpilePackages. -// All routing is handled by the Pages Router (src/pages/). +// Stub layout enables App Router mode, relaxing the "global CSS only in _app" +// restriction for transpilePackages. +// The Pages Router still handles all routing through src/pages. import type { ReactNode } from 'react' export default function RootLayout({ children }: { children: ReactNode }) { diff --git a/src/archives/lib/is-archived-version.ts b/src/archives/lib/is-archived-version.ts index 8b08bacd856c..d2e44173e21b 100644 --- a/src/archives/lib/is-archived-version.ts +++ b/src/archives/lib/is-archived-version.ts @@ -8,14 +8,12 @@ type IsArchivedInfo = { } export function isArchivedVersion(req: ExtendedRequest): IsArchivedInfo { - // if this is an assets path, use the referrer - // if this is a docs path, use the req.path + // Asset requests carry the archive version in the Referrer, not req.path. const pathToCheck = patterns.assetPaths.test(req.path) ? req.get('referrer') : req.path return isArchivedVersionByPath(pathToCheck || '') } export function isArchivedVersionByPath(pathToCheck: string): IsArchivedInfo { - // ignore paths that don't have an enterprise version number if ( !( patterns.getEnterpriseVersionNumber.test(pathToCheck) || @@ -25,12 +23,10 @@ export function isArchivedVersionByPath(pathToCheck: string): IsArchivedInfo { return {} } - // extract enterprise version from path, e.g. 2.16 const requestedVersion = pathToCheck.includes('enterprise-server@') ? pathToCheck.match(patterns.getEnterpriseServerNumber)?.[1] : pathToCheck.match(patterns.getEnterpriseVersionNumber)?.[1] - // bail if the request version is not deprecated if (!requestedVersion || !deprecated.includes(requestedVersion)) { return {} } diff --git a/src/archives/lib/old-versions-utils.ts b/src/archives/lib/old-versions-utils.ts index 6804f8551d3d..880aaeed49d6 100644 --- a/src/archives/lib/old-versions-utils.ts +++ b/src/archives/lib/old-versions-utils.ts @@ -7,67 +7,55 @@ const latestNewVersion = `enterprise-server@${latest}` const oldVersions = ['dotcom'].concat(supported) const newVersions = Object.keys(allVersions) -// Utility functions for converting between old version paths and new version paths. -// See lib/path-utils.ts for utility functions based on new paths. -// Examples: -// OLD /github/category/article to NEW /free-pro-team@latest/github/category/article -// OLD /enterprise/2.21/user/github/category/article to NEW /enterprise-server@2.21/github/category/article -// OLD /enterprise/user/github/category/article to NEW /enterprise-server@/github/category/article +// Converts legacy version paths to versioned paths. +// See lib/path-utils.ts for utilities based on versioned paths. +// /github/category/article becomes /free-pro-team@latest/github/category/article. +// /enterprise/2.21/user/github/category/article becomes +// /enterprise-server@2.21/github/category/article. +// /enterprise/user/github/category/article becomes +// /enterprise-server@/github/category/article. -// Given a new version like enterprise-server@2.21, -// return an old version like 2.21. -// Fall back to latest GHES version if one can't be found, -// for example, if the new version is private-instances@latest. +// Unknown enterprise version names fall back to the latest GHES release. +// Example: private-instances@latest maps to the latest GHES release. export function getOldVersionFromNewVersion(newVersion: string) { return newVersion === nonEnterpriseDefaultVersion ? 'dotcom' : oldVersions.find((oldVersion) => newVersion.includes(oldVersion)) || latest } -// Given an old version like 2.21, -// return a new version like enterprise-server@2.21. -// Fall back to latest GHES version if one can't be found. +// Unknown legacy enterprise versions fall back to the latest versioned GHES path. export function getNewVersionFromOldVersion(oldVersion: string) { return oldVersion === 'dotcom' ? nonEnterpriseDefaultVersion : newVersions.find((newVersion) => newVersion.includes(oldVersion)) || latestNewVersion } -// Given an old path like /enterprise/2.21/user/github/category/article, -// return an old version like 2.21. export function getOldVersionFromOldPath(oldPath: string) { - // We should never be calling this function on a path that starts with a new version, - // so we can assume the path either uses the old /enterprise format or it's dotcom. + // Callers pass legacy enterprise paths or dotcom paths, not enterprise-server@ paths. if (!patterns.enterprise.test(oldPath)) return 'dotcom' const ghesNumber = oldPath.match(patterns.getEnterpriseVersionNumber) return ghesNumber ? ghesNumber[1] : latest } -// Given an old path like /en/enterprise/2.21/user/github/category/article, -// return a new path like /en/enterprise-server@2.21/github/category/article. +// /en/enterprise/2.21/user/github/category/article becomes +// /en/enterprise-server@2.21/github/category/article. +// Paths can already contain a versioned segment after currentVersion renders. +// Example: /en/enterprise/private-instances@latest/admin/category/article keeps +// private-instances@latest. export function getNewVersionedPath(oldPath: string, languageCode = '') { - // It's possible a new version has been injected into an old path - // via syntax like: /en/enterprise/{{ currentVersion }}/admin/category/article - // which could resolve to /en/enterprise/private-instances@latest/admin/category/article, - // in which case the new version is the `private-instances@latest` segment. - // Get the second or third segment depending on whether there is a lang code. const pathParts = oldPath.split('/') const possibleVersion = languageCode ? pathParts[3] : pathParts[2] let newVersion = newVersions.includes(possibleVersion) ? possibleVersion : '' - // If no new version was found, assume path contains an old version, like 2.21 if (!newVersion) { const oldVersion = getOldVersionFromOldPath(oldPath) newVersion = getNewVersionFromOldVersion(oldVersion) } - // Remove /?/enterprise?/?/user? if present. - // This leaves only the part of the string that starts with the product. - // Example: /github/category/article + // patterns.oldEnterprisePath leaves the product path, such as /github/category/article. const restOfString = oldPath.replace(patterns.oldEnterprisePath, '') - // Add the language and new version to the product part of the string return path.posix.join('/', languageCode, newVersion, restOfString) } diff --git a/src/archives/middleware/archived-asset-redirects.ts b/src/archives/middleware/archived-asset-redirects.ts index 5dac6e511bd3..c2579ba1358b 100644 --- a/src/archives/middleware/archived-asset-redirects.ts +++ b/src/archives/middleware/archived-asset-redirects.ts @@ -2,24 +2,15 @@ import type { Response, NextFunction } from 'express' import type { ExtendedRequest } from '@/types' -// When we archive old versions, we take a snapshot of rendered pages, -// which includes whatever bundles it used at the time. -// Sometimes those archived versions don't include all static assets -// it might refer to. -// This middleware is a chance to redirect to new assets that we can -// use instead. -// Yes, not all legacy assets *can* be redirected to something we have -// today. But for those that we can, this is the middleware to do it. -// And the reason we don't host a copy of these old files is because -// we strive to make the files in the repo only files that we actually -// use and refer to in the non-archived content. +// Archived rendered pages can reference static assets that no longer exist in the repo. +// Redirect legacy assets we can map instead of hosting unused files. -// Note that, we also have `archived-enterprise-versions-assets.ts` -// but that one assumes the whole path refers to a prefix which is -// considered archived. E.g. /en/enterprise-server@2.9/foo/bar.css +// archived-enterprise-versions-assets.ts handles whole archived path prefixes, such as +// /en/enterprise-server@2.9/foo/bar.css. const REDIRECTS: Record = { - // Example: https://docs.github.com/en/enterprise-server@2.22/authentication/connecting-to-github-with-ssh + // One archived source is + // https://docs.github.com/en/enterprise-server@2.22/authentication/connecting-to-github-with-ssh. '/assets/images/octicons/search.svg': '/assets/images/octicons/search-24.svg', } export default function archivedAssetRedirects( diff --git a/src/archives/middleware/archived-enterprise-versions-assets.ts b/src/archives/middleware/archived-enterprise-versions-assets.ts index 5e80dacc376e..3c8c213b6bec 100644 --- a/src/archives/middleware/archived-enterprise-versions-assets.ts +++ b/src/archives/middleware/archived-enterprise-versions-assets.ts @@ -10,28 +10,17 @@ import { createLogger } from '@/observability/logger' const logger = createLogger(import.meta.url) -// This module handles requests for the CSS and JS assets for -// deprecated GitHub Enterprise versions by routing them to static content in -// one of the docs-ghes- repos. -// See also ./archived-enterprise-versions.ts for non-CSS/JS paths +// Proxies archived CSS and JS assets from docs-ghes- repositories. +// archived-enterprise-versions.ts handles non-asset paths. export default async function archivedEnterpriseVersionsAssets( req: ExtendedRequest, res: Response, next: NextFunction, ) { - // Only match asset paths - // This can be true on /enterprise/2.22/_next/static/foo.css - // or /_next/static/foo.css if (!patterns.assetPaths.test(req.path)) return next() - // The URL is either in the format - // /enterprise/2.22/_next/static/foo.css, - // /enterprise-server@, - // or /_next/static/foo.css. - // If the URL is prefixed with the enterprise version and release number - // or if the Referrer contains the enterprise version and release number, - // then we'll fetch it from the docs-ghes- repo. + // Versioned and bare asset paths still require an archived Referrer before proxying. if ( !( patterns.getEnterpriseVersionNumber.test(req.path) || @@ -43,20 +32,10 @@ export default async function archivedEnterpriseVersionsAssets( return next() } - // Now we know the URL is definitely not /_next/static/foo.css - // So it's probably /enterprise/2.22/_next/static/foo.css and we - // should see if we might find this in the proxied backend. - // But `isArchivedVersion()` will only return truthy if the - // Referrer header also indicates that the request for this static - // asset came from a page const { isArchived, requestedVersion } = isArchivedVersion(req) if (!isArchived || !requestedVersion) return next() - // If this looks like a Next.js chunk or build manifest request from an archived page, - // just return 204 No Content instead of trying to proxy it. - // This suppresses noise from hydration requests that don't affect - // content viewing since archived pages render fine server-side. - // Only target specific problematic asset types, not all _next/static assets. + // Send 204 for chunks, _buildManifest.js, and _ssgManifest.js; archive pages render without them. if ( (req.path.includes('/_next/static/chunks/') || req.path.includes('/_buildManifest.js') || @@ -65,18 +44,15 @@ export default async function archivedEnterpriseVersionsAssets( ) { archivedCacheControl(res) setFastlySurrogateKey(res, SURROGATE_ENUMS.MANUAL) - return res.sendStatus(204) // No Content - silently ignore + return res.sendStatus(204) } - // In all of the `docs-ghes- - // These will thus be requested, with a Referrer header that - // forces us to give it a chance, but it'll find it can't find it - // but we mustn't return a 404 yet, because that - // /_next/static/styles.css will probably still succeed because the 404 - // page is not that of the archived enterprise version. + // Fall through on proxy misses; 404 pages request /_next/static/styles.css from archived pages. return next() } } diff --git a/src/archives/middleware/archived-enterprise-versions.ts b/src/archives/middleware/archived-enterprise-versions.ts index 6a678383e7c8..cae98add1a4f 100644 --- a/src/archives/middleware/archived-enterprise-versions.ts +++ b/src/archives/middleware/archived-enterprise-versions.ts @@ -24,22 +24,19 @@ import { ExtendedRequest } from '@/types' const logger = createLogger(import.meta.url) const OLD_PUBLIC_AZURE_BLOB_URL = 'https://githubdocs.azureedge.net' -// Old Azure Blob Storage `enterprise` container. +// Old Azure Blob Storage enterprise container. const OLD_AZURE_BLOB_ENTERPRISE_DIR = `${OLD_PUBLIC_AZURE_BLOB_URL}/enterprise` -// Old Azure Blob storage `github-images` container with -// the root directory of 'enterprise'. +// Old Azure Blob Storage github-images container rooted at enterprise. const OLD_GITHUB_IMAGES_ENTERPRISE_DIR = `${OLD_PUBLIC_AZURE_BLOB_URL}/github-images/enterprise` const OLD_DEVELOPER_SITE_CONTAINER = `${OLD_PUBLIC_AZURE_BLOB_URL}/developer-site` -// This is the new repo naming convention we use for each archived enterprise -// version. E.g. https://github.github.com/docs-ghes-2.10 +// Archived enterprise repositories use https://github.github.com/docs-ghes-2.10. const ENTERPRISE_GH_PAGES_URL_PREFIX = 'https://github.github.com/docs-ghes-' type ArchivedRedirects = { [url: string]: string | null } -// These files are huge so lazy-load them. But note that the -// `readJsonFileLazily()` function will, at import-time, check that -// the path does exist. +// Lazy-load the large redirect files. +// readCompressedJsonFileFallbackLazily verifies the path at import time. const archivedRedirects = readCompressedJsonFileFallbackLazily( './src/redirects/lib/static/archived-redirects-from-213-to-217.json', ) as () => ArchivedRedirects @@ -51,51 +48,23 @@ const archivedFrontmatterValidURLS = readCompressedJsonFileFallbackLazily( './src/redirects/lib/static/archived-frontmatter-valid-urls.json', ) as () => ArchivedFrontmatterURLs -// Combine all the things you need to make sure the response is -// aggressively cached. const cacheAggressively = (res: Response) => { archivedCacheControl(res) - // This sets a custom Fastly surrogate key so that this response - // won't get updated in every deployment. - // Essentially, this sets a surrogate key such that Fastly - // doesn't do soft-purges on these responses on every - // automated deployment. + // Manual surrogate keys avoid Fastly soft purges on every automated deployment. setFastlySurrogateKey(res, SURROGATE_ENUMS.MANUAL) } -// The way `got` does retries: -// -// sleep = 1000 * Math.pow(2, retry - 1) + Math.random() * 100 -// -// So, it means: -// -// 1. ~1000ms -// 2. ~2000ms -// 3. ~4000ms -// -// ...if the limit we set is 3. -// Our own timeout, in @/frame/middleware/timeout.ts defaults to 10 seconds. -// So there's no point in trying more attempts than 3 because it would -// just timeout on the 10s. (i.e. 1000 + 2000 + 4000 + 8000 > 10,000) +// Got sleeps about 1s, 2s, then 4s for three retries. +// A fourth retry would exceed MAX_REQUEST_TIMEOUT, which defaults to 10 seconds in production. const retryConfiguration = { limit: 3 } -// According to our Datadog metrics, the *average* time for the -// the 'archive_enterprise_proxy' metric is ~70ms (excluding spikes) -// which is much less than 3000ms. -// We have observed errors of timeout, in production, when it was -// set to 500ms and then 1500ms. Let's be more conservative here to -// avoid unnecessary error reporting during occasional slow responses. +// Datadog reports archive_enterprise_proxy averages about 70ms excluding spikes. +// Production timed out at 500ms and 1500ms, so 3000ms avoids noise from slow responses. const timeoutConfiguration = { response: 3000 } -// Monitoring thresholds for logging response times -// Log warnings when responses exceed half the timeout threshold -const WARN_RESPONSE_THRESHOLD = timeoutConfiguration.response / 2 // 1500ms -// Log info for responses that are noticeably slow but not concerning -const SLOW_RESPONSE_THRESHOLD = 500 // ms - -// This module handles requests for deprecated GitHub Enterprise versions -// by routing them to static content in -// one of the docs-ghes- repos. +const WARN_RESPONSE_THRESHOLD = timeoutConfiguration.response / 2 +// Log successful responses slower than 500ms. +const SLOW_RESPONSE_THRESHOLD = 500 export default async function archivedEnterpriseVersions( req: ExtendedRequest, @@ -109,14 +78,14 @@ export default async function archivedEnterpriseVersions( const redirectCode = pathLanguagePrefixed(req.path) ? 301 : 302 - // Redirects for releases 3.0+ if (deprecatedWithFunctionalRedirects.includes(requestedVersion)) { const redirectTo = req.context ? getRedirect(req.path, req.context) : undefined if (redirectTo) { if (redirectCode === 302) { - languageCacheControl(res) // call first to get `vary` + // languageCacheControl sets vary; archivedCacheControl extends the cache duration. + languageCacheControl(res) } - archivedCacheControl(res) // call second to extend duration + archivedCacheControl(res) return res.safeRedirect(redirectCode, redirectTo) } @@ -124,11 +93,7 @@ export default async function archivedEnterpriseVersions( try { redirectJson = (await getRemoteJSON(getProxyPath('redirects.json', requestedVersion), { retry: retryConfiguration, - // This is allowed to be different compared to the other requests - // we make because downloading the `redirects.json` once is very - // useful because it caches so well. - // And, as of 2021 that `redirects.json` is 10MB so it's more likely - // to time out. + // Cache misses use a 1-second time-to-first-byte limit; body transfer may take longer. timeout: { response: 1000 }, })) as Record } catch (err) { @@ -143,13 +108,14 @@ export default async function archivedEnterpriseVersions( const newRedirectTo = redirectJson[withoutLanguage] if (newRedirectTo && newRedirectTo !== withoutLanguage) { if (redirectCode === 302) { - languageCacheControl(res) // call first to get `vary` + // languageCacheControl sets vary; archivedCacheControl extends the cache duration. + languageCacheControl(res) } - archivedCacheControl(res) // call second to extend duration + archivedCacheControl(res) return res.safeRedirect(redirectCode, `/${language}${newRedirectTo}`) } } - // For releases 2.13 and lower, redirect language-prefixed URLs like /en/enterprise/2.10 -> /enterprise/2.10 + // Earlier releases redirect /en/enterprise/2.10 to /enterprise/2.10. if ( req.path.startsWith('/en/') && versionSatisfiesRange(requestedVersion, `<${firstVersionDeprecatedOnNewSite}`) @@ -158,30 +124,21 @@ export default async function archivedEnterpriseVersions( return res.safeRedirect(redirectCode, req.baseUrl + req.path.replace(/^\/en/, '')) } - // Redirects for releases 2.13 - 2.17 if ( versionSatisfiesRange(requestedVersion, `>=${firstVersionDeprecatedOnNewSite}`) && versionSatisfiesRange(requestedVersion, `<=${lastVersionWithoutArchivedRedirectsFile}`) ) { const [language, withoutLanguagePath] = splitByLanguage(req.path) - // `archivedRedirects` is a callable because it's a lazy function - // and memoized so calling it is cheap. - + // archivedRedirects is lazy and memoized, so calling it here is cheap. const newPath = withoutLanguagePath && archivedRedirects()[withoutLanguagePath] - // Some entries in the lookup exists purely for the sake of injecting - // language. - // E.g. '/enterprise/2.15/user' - // URLs like this only need to redirect the original `req.path` - // didn't already have a language + // Null entries inject /en when the original request has no language prefix. if (newPath !== undefined && (newPath || !language)) { const redirect = `/${language || 'en'}${newPath || withoutLanguagePath}` cacheAggressively(res) return res.safeRedirect(redirectCode, redirect) } } - // Redirects for 2.18 - 3.0. Starting with 2.18, we updated the archival - // script to create a redirects.json file if ( versionSatisfiesRange(requestedVersion, `>${lastVersionWithoutArchivedRedirectsFile}`) && !deprecatedWithFunctionalRedirects.includes(requestedVersion) @@ -190,11 +147,7 @@ export default async function archivedEnterpriseVersions( try { redirectJson = (await getRemoteJSON(getProxyPath('redirects.json', requestedVersion), { retry: retryConfiguration, - // This is allowed to be different compared to the other requests - // we make because downloading the `redirects.json` once is very - // useful because it caches so well. - // And, as of 2021 that `redirects.json` is 10MB so it's more likely - // to time out. + // Cache misses use a 1-second time-to-first-byte limit; body transfer may take longer. timeout: { response: 1000 }, })) as Record } catch (err) { @@ -205,15 +158,13 @@ export default async function archivedEnterpriseVersions( throw err } - // make redirects found via redirects.json redirect with a 301 if (redirectJson[req.path]) { res.set('x-robots-tag', 'noindex') cacheAggressively(res) return res.safeRedirect(redirectCode, redirectJson[req.path]) } } - // Short-circuit requests that will never resolve on the upstream - // GitHub Pages repos, avoiding unnecessary network requests. + // Short-circuit impossible archive paths to avoid unnecessary upstream requests. const earlyNotFound = getEarlyNotFoundReason(req.path, requestedVersion) if (earlyNotFound) { statsd.increment('middleware.archived_early_not_found', 1, [ @@ -224,9 +175,7 @@ export default async function archivedEnterpriseVersions( return res.status(404).type('text').send('Page not found') } - // Requests without a language prefix for versions > 2.17 will always - // 404 upstream (the archive repos store pages under /en/, /zh/, etc.). - // Skip the fetch and let downstream middleware handle the redirect. + // Archive repos after 2.17 require language prefixes; redirects handle unlanguaged paths. if ( versionSatisfiesRange(requestedVersion, `>${lastVersionWithoutArchivedRedirectsFile}`) && !pathLanguagePrefixed(req.path) @@ -235,7 +184,6 @@ export default async function archivedEnterpriseVersions( return next() } - // Retrieve the page from the archived repo const doGet = () => fetchWithRetry( getProxyPath(req.path, requestedVersion), @@ -265,14 +213,13 @@ export default async function archivedEnterpriseVersions( }) } - // Warn on 404s, which are expected for missing archived pages. - // Everything else is a genuine upstream failure. + // Missing archived pages are expected 404s; other upstream failures need error logs. if (r.status !== 200) { let upstreamBody: string | undefined try { upstreamBody = await readBodyWithTimeout(r, () => r.text(), timeoutConfiguration.response) } catch { - // A body we cannot read should not change how we handle the error. + // Ignore unreadable bodies so the original upstream status controls error handling. } const level = r.status === 404 ? 'warn' : 'error' logger[level]('Failed to fetch archived enterprise content', { @@ -286,7 +233,7 @@ export default async function archivedEnterpriseVersions( }) } - // Log successful responses with timing for monitoring trends + // Log slow successful responses for monitoring trends. if (r.status === 200 && responseTime > SLOW_RESPONSE_THRESHOLD) { logger.info('Archived enterprise content response', { version: requestedVersion, @@ -303,7 +250,7 @@ export default async function archivedEnterpriseVersions( ) res.set('x-robots-tag', 'noindex') - // make stubbed redirect files (which exist in versions <2.13) redirect with a 301 + // Stubbed redirect files in releases before 2.13 return a static redirect target. const staticRedirect = body.match(patterns.staticRedirect) if (staticRedirect) { cacheAggressively(res) @@ -314,15 +261,12 @@ export default async function archivedEnterpriseVersions( cacheAggressively(res) - // Releases 3.2 and higher contain image asset paths with the - // old Azure Blob Storage URL. These need to be rewritten to - // the new archived enterprise repo URL. + // Releases 3.2 through 3.9 contain old Azure Blob image URLs that need archive URLs. if ( versionSatisfiesRange(requestedVersion, `>=${firstReleaseStoredInBlobStorage}`) && versionSatisfiesRange(requestedVersion, `<=3.9`) ) { - // `x-host` is a custom header set by Fastly. - // GLB automatically deletes the `x-forwarded-host` header. + // Fastly sets x-host, and GLB removes x-forwarded-host. const host = req.get('x-host') || req.get('x-forwarded-host') || req.get('host') const modifiedBody = body .replaceAll( @@ -337,11 +281,7 @@ export default async function archivedEnterpriseVersions( return res.send(modifiedBody) } - // Releases 3.1 and lower were previously hosted in the - // help-docs-archived-enterprise-versions repo. Only the images - // were stored in the old Azure Blob Storage `github-images` container. - // The image paths all need to be updated to reference the images in the - // new archived enterprise repo's root assets directory. + // Releases before 3.2 need github-images Azure Blob paths rewritten to archive root assets. if (versionSatisfiesRange(requestedVersion, `<${firstReleaseStoredInBlobStorage}`)) { let modifiedBody = body.replaceAll( `${OLD_GITHUB_IMAGES_ENTERPRISE_DIR}/${requestedVersion}`, @@ -352,12 +292,11 @@ export default async function archivedEnterpriseVersions( `${OLD_DEVELOPER_SITE_CONTAINER}/${requestedVersion}`, `${ENTERPRISE_GH_PAGES_URL_PREFIX}${requestedVersion}/developer`, ) - // Update all hrefs to add /developer to the path modifiedBody = modifiedBody.replaceAll( `="/enterprise/${requestedVersion}`, `="/enterprise/${requestedVersion}/developer`, ) - // The changelog is the only thing remaining on developer.github.com + // The changelog remains on developer.github.com. modifiedBody = modifiedBody.replaceAll( 'href="/changes', 'href="https://developer.github.com/changes', @@ -369,46 +308,35 @@ export default async function archivedEnterpriseVersions( `="${ENTERPRISE_GH_PAGES_URL_PREFIX}${requestedVersion}/assets`, ) - // Fix broken hrefs on the 2.16 landing page + // The 2.16 landing page has hrefs missing the version segment. if (requestedVersion === '2.16' && req.path === '/en/enterprise/2.16') { modifiedBody = modifiedBody.replaceAll('ref="/en/enterprise', 'ref="/en/enterprise/2.16') } - // Remove the search results container from the page + // The empty search results container blocks clicks on page links. modifiedBody = modifiedBody.replaceAll('
', '') return res.send(modifiedBody) } - // In all releases, some assets were incorrectly scraped and contain - // deep relative paths. For example, releases 3.4+ use the webp format - // for images. The URLs for those images were never rewritten to pull - // from the Azure Blob Storage container. This may be due to not - // updating our scraping tool to handle the new image types. There - // are additional images in older versions that also have a relative path. - // We want to update the URLs in the format - // "../../../../../../assets/" to prefix the assets directory with the - // new archived enterprise repo URL. + // Deep relative asset paths like "../../../../../../assets/" need archive repo prefixes. let modifiedBody = body.replaceAll( /="(\.\.\/)*assets/g, `="${ENTERPRISE_GH_PAGES_URL_PREFIX}${requestedVersion}/assets`, ) - // Fix broken hrefs on the 2.16 landing page + // The 2.16 landing page has hrefs missing the version segment. if (requestedVersion === '2.16' && req.path === '/en/enterprise/2.16') { modifiedBody = modifiedBody.replaceAll('ref="/en/enterprise', 'ref="/en/enterprise/2.16') } - // Remove the search results container from the page, which removes a white - // box that prevents clicking on page links + // The empty search results container blocks clicks on page links. modifiedBody = modifiedBody.replaceAll('
', '') return res.send(modifiedBody) } - // In releases 2.13 - 2.17, we lost access to frontmatter redirects - // during the archival process. This workaround finds potentially - // relevant frontmatter redirects in currently supported pages + // Releases 2.13 through 2.17 need supported-page frontmatter redirects after data loss. if ( versionSatisfiesRange(requestedVersion, `>=${firstVersionDeprecatedOnNewSite}`) && versionSatisfiesRange(requestedVersion, `<=${lastVersionWithoutArchivedRedirectsFile}`) @@ -433,62 +361,39 @@ function getProxyPath(reqPath: string, requestedVersion: string) { `/enterprise/${requestedVersion}/developer`, ) - // This was the last release supported on developer.github.com + // Developer pages keep the developer-site path layout from the archived release. if (isDeveloperPage) { const enterprisePath = `/enterprise/${requestedVersion}` const newReqPath = reqPath.replace(enterprisePath, '') return ENTERPRISE_GH_PAGES_URL_PREFIX + requestedVersion + newReqPath } - // Releases 2.18 and higher + // Releases 2.18 and later store redirects.json at the repo root and pages at /index.html. if (versionSatisfiesRange(requestedVersion, `>${lastVersionWithoutArchivedRedirectsFile}`)) { const newReqPath = reqPath.includes('redirects.json') ? `/${reqPath}` : `${reqPath}/index.html` return ENTERPRISE_GH_PAGES_URL_PREFIX + requestedVersion + newReqPath } - // Releases 2.13 - 2.17 - // redirect.json files don't exist for these versions + // Releases 2.13 through 2.17 lack redirects.json files. if (versionSatisfiesRange(requestedVersion, `>=2.13`)) { return `${ENTERPRISE_GH_PAGES_URL_PREFIX + requestedVersion + reqPath}/index.html` } - // Releases 2.12 and lower + // Releases 2.12 and earlier omit the /enterprise/ path prefix. const enterprisePath = `/enterprise/${requestedVersion}` const newReqPath = reqPath.replace(enterprisePath, '') return ENTERPRISE_GH_PAGES_URL_PREFIX + requestedVersion + newReqPath } -// Module-level global cache object. -// Gets populated lazily inside getFallbackRedirect(). +// Caches fallback redirect lookups across requests. const fallbackRedirectLookups = new Map() +// archived-frontmatter-valid-urls.json maps valid destinations to acceptable source URLs. +// getFallbackRedirect inverts that structure once, so lookups avoid scanning every destination. +// Example source /enterprise/2.13/other/old/thing redirects to destination +// /enterprise/2.13/foo/bar. +// The JSON omits language prefixes, so lookups strip the request language and add it back. function getFallbackRedirect(req: ExtendedRequest) { - // The file `lib/redirects/static/archived-frontmatter-valid-urls.json` which - // we depend on here, is structured like this: - // - // { - // "/enterprise/2.13/foo/bar": [ - // "/enterprise/2.13/other/old/thing", - // "/enterprise/2.13/more/redirectable/url", - // "/enterprise/2.13/etc/etc" - // ], - // ... - // - // The keys are valid URLs that it can redirect to. I.e. these are - // URLs that we definitely know are valid and will be found - // in one of the docs-ghes- repos. - // The array values are possible URLs we deem acceptable redirect - // sources. - // But to avoid an unnecessary, O(n), loop every time, we turn this - // structure around to become: - // - // { - // "/enterprise/2.13/other/old/thing": "/enterprise/2.13/foo/bar", - // "/enterprise/2.13/more/redirectable/url": "/enterprise/2.13/foo/bar", - // "/enterprise/2.13/etc/etc": "/enterprise/2.13/foo/bar", - // ... - // - // Now potential lookups are fast. if (!fallbackRedirectLookups.size) { for (const [destination, sources] of Object.entries(archivedFrontmatterValidURLS())) { for (const source of sources) { @@ -497,14 +402,6 @@ function getFallbackRedirect(req: ExtendedRequest) { } } - // But before we proceed, remember that the - // file lib/redirects/static/archived-frontmatter-valid-urls.json never - // contains a language prefix. - // E.g. only `/enterprise/2.13/foo/bar` but the requested URL can be - // `/en/enterprise/2.13/foo/bar`, `/pt/enterprise/2.13/foo/bar`, - // or just `/enterprise/2.13/foo/bar`. - // Whatever it is, pop the language prefix, operate, and put it back - // again. In the end, it always has to have a language prefix. const [language, withoutLanguage] = splitPathByLanguage(req.path) const fallback = fallbackRedirectLookups.get(withoutLanguage) if (fallback) { @@ -523,43 +420,34 @@ function splitByLanguage(uri: string) { return [language, withoutLanguage] } -// Regex to extract any language-like prefix from the path, including -// "cn" which was the old Chinese language code used in archives ≤3.2. +// Matches language-like path prefixes, including the old Chinese cn code from archives through 3.2. const archiveLanguagePrefixRegex = new RegExp(`^/(${Object.keys(allLanguages).join('|')}|cn)(/|$)`) -// Detects request paths that will never resolve on the upstream GitHub -// Pages archive repos, so we can 404 immediately without making a -// network request. Returns a short reason string, or null if the -// request looks plausible. +// Identifies request paths that cannot resolve on upstream GitHub Pages archive repos. +// Returning a reason lets callers log and skip the network request. function getEarlyNotFoundReason(reqPath: string, version: string): string | null { - // Double slashes in the path never resolve (e.g. ".../about-2fa//index.html") + // Double slashes never resolve, such as /about-2fa//index.html. if (reqPath.includes('//')) { return 'double-slash' } - // A duplicated "/developer/developer/" segment means a broken crawler URL - // from the old developer.github.com site. + // Duplicated /developer/developer/ segments come from broken developer.github.com crawler URLs. if (reqPath.includes('/developer/developer/')) { return 'developer-developer' } - // Check if the language in the path actually exists in this version's - // archive. Each language has a `firstArchivedVersion` indicating when - // it was first included in the GHES archives. + // firstArchivedVersion records when each archive language became available. const langMatch = reqPath.match(archiveLanguagePrefixRegex) if (langMatch) { const lang = langMatch[1] - // "cn" was the old Chinese language code; those archives are ancient - // and effectively dead traffic. Always 404. + // cn was the old Chinese language code; always 404 it as dead archive traffic. if (lang === 'cn') { return 'language-not-in-version' } - const langDef = allLanguages[lang] if (langDef?.firstArchivedVersion) { - // 404 if the requested version is older than when this language - // was first archived (e.g. /zh/ on v3.0 → 404 because zh started in 3.3) + // 404 languages before firstArchivedVersion, such as /zh/ on 3.0 because zh starts in 3.3. if (!versionSatisfiesRange(version, `>=${langDef.firstArchivedVersion}`)) { return 'language-not-in-version' } diff --git a/src/archives/scripts/warmup-remotejson.ts b/src/archives/scripts/warmup-remotejson.ts index 978bcdd4cdfd..4bfd658e58b4 100755 --- a/src/archives/scripts/warmup-remotejson.ts +++ b/src/archives/scripts/warmup-remotejson.ts @@ -1,20 +1,7 @@ -// [start-readme] -// -// This calls a function directly that is used by our archived enterprise -// middleware. Namely, the `getRemoteJSON` function. That function is -// able to use the disk to cache responses quite aggressively. So when -// it's been run once, with the same disk, next time it can draw from disk -// rather than having to rely on network. -// -// We have this script to avoid excessive network fetches in production -// where, due to production deploys restarting new Node services, we -// can't rely on in-memory caching often enough. -// -// The list of URLs hardcoded in here is based on analyzing the URLs that -// were logged as tags in Datadog for entries that couldn't rely on -// in-memory cache. -// -// [end-readme] +// Warms getRemoteJSON's disk cache for archived redirects.json files. +// Production deploys restart Node services often enough that in-memory cache misses repeat. +// Production reuses these entries only when it starts from the same warmed cache directory. +// URLs come from Datadog tags for redirects.json requests that missed the in-memory cache. import { program } from 'commander' import semver, { SemVer } from 'semver' diff --git a/src/archives/tests/deprecated-enterprise-versions.ts b/src/archives/tests/deprecated-enterprise-versions.ts index 3515fb7bb587..ab9a668521ca 100644 --- a/src/archives/tests/deprecated-enterprise-versions.ts +++ b/src/archives/tests/deprecated-enterprise-versions.ts @@ -79,21 +79,19 @@ describe('enterprise deprecation', () => { const { $: $2, res } = await getDOM(`${guidesPath}/${firstLink}`) expect(res.statusCode).toBe(200) - // this test assumes the Installation guide is the first link on the guides page + // The test follows the first link, which is the Installation guide. expect($2('h2').text()).toBe('Installing and configuring GitHub Enterprise') }) }) -// Starting with the deprecation of 3.0, it's the first time we deprecate -// enterprise versions since redirects is a *function* rather than a -// lookup in a big object. +// Enterprise 3.0 redirects use getRedirect plus redirects.json instead of a static object. describe('recently deprecated redirects', () => { test('basic enterprise 3.0 redirects', async () => { const res = await get('/enterprise/3.0') expect(res.statusCode).toBe(302) expect(res.headers.location).toBe('/en/enterprise-server@3.0') expect(res.headers['set-cookie']).toBeUndefined() - // language specific caching + // Language-specific redirects vary by language headers. expect(res.headers['cache-control']).toContain('public') expect(res.headers['cache-control']).toMatch(/max-age=[1-9]/) expect(res.headers.vary).toContain('accept-language') @@ -104,7 +102,7 @@ describe('recently deprecated redirects', () => { const res = await get('/en/enterprise/3.0') expect(res.statusCode).toBe(301) expect(res.headers.location).toBe('/en/enterprise-server@3.0') - // 301 redirects are safe to cache aggressively + // 301 redirects can cache aggressively. expect(res.headers['set-cookie']).toBeUndefined() expect(res.headers['cache-control']).toContain('public') expect(res.headers['cache-control']).toMatch(/max-age=[1-9]/) @@ -116,13 +114,12 @@ describe('recently deprecated redirects', () => { ) expect(res.statusCode).toBe(302) expect(res.headers['set-cookie']).toBeUndefined() - // language specific caching + // Language-specific redirects vary by language headers. expect(res.headers['cache-control']).toContain('public') expect(res.headers['cache-control']).toMatch(/max-age=[1-9]/) expect(res.headers.vary).toContain('accept-language') expect(res.headers.vary).toContain('x-user-language') - // This is based on - // https://github.com/github/docs-ghes-3.0/blob/main/redirects.json + // Matches https://github.com/github/docs-ghes-3.0/blob/main/redirects.json. expect(res.headers.location).toBe( '/en/enterprise-server@3.0/get-started/learning-about-github/githubs-products', ) diff --git a/src/assets/middleware/asset-preprocessing.ts b/src/assets/middleware/asset-preprocessing.ts index a61be8044422..07493abf8ed4 100644 --- a/src/assets/middleware/asset-preprocessing.ts +++ b/src/assets/middleware/asset-preprocessing.ts @@ -2,15 +2,10 @@ import type { Response, NextFunction } from 'express' import type { ExtendedRequest } from '@/types' -// This middleware rewrites the URL of requests that contain the -// portion of `/cb-\d+/`. -// "cb" stands for "cache bust". -// There's a Markdown plugin that rewrites all values -// from `` to -// `` for example. -// We're doing this so that we can set a much more aggressive -// Cache-Control for assets and a CDN surrogate-key that doesn't -// soft-purge on every deployment. +// Markdown image URLs include a cache-busting path part, for example +// /assets/foo/bar.png becomes /assets/cb-123467/foo/bar.png. +// That path lets assets use aggressive Cache-Control and a Fastly surrogate key +// that avoids soft purges on every deployment. const regex = /\/cb-\d+\// @@ -20,27 +15,16 @@ export default function assetPreprocessing( next: NextFunction, ) { if (req.path.startsWith('/assets/')) { - // We didn't use to have a rule about all image assets must be - // lower case. So we've exposed things like: - // which means they could - // get a 404 if the file is actually named `foobar.png`. + // Mixed-case asset URLs can 404 when the file on disk is lowercase. if (req.url !== req.url.toLowerCase()) { - // The reason for doing a redirect instead rewriting the - // `req.url` attribute is that we don't want encourage this. - // By forcing this to be a redirect, it means we only serve - // 1 single file. All other requests will be redirects. - // Otherwise someone might trigger too much bypassing of the CDN. + // Redirecting instead of rewriting req.url serves one canonical file and protects CDN hit rates. return res.safeRedirect(req.url.toLowerCase()) } - // We're only confident enough to set the *manual* surrogate key if the - // asset contains the cache-busting piece. + // Only cache-busted assets can use the manual surrogate key safely. if (regex.test(req.url)) { - // We remove it so that when `express.static()` runs, it can - // find the file on disk by its original name. + // express.static() needs the original file path on disk. req.url = req.url.replace(regex, '/') - // The Cache-Control is managed by the configuration - // for express.static() later in the middleware. } } return next() diff --git a/src/assets/middleware/dynamic-assets.ts b/src/assets/middleware/dynamic-assets.ts index c95d53f7f7aa..f41d500387c6 100644 --- a/src/assets/middleware/dynamic-assets.ts +++ b/src/assets/middleware/dynamic-assets.ts @@ -13,35 +13,20 @@ import { createLogger } from '@/observability/logger' const logger = createLogger(import.meta.url) -/** - * This is the indicator that is a virtual part of the URL. - * Similar to `/cb-1234/` in asset URLs, it's just there to tell the - * middleware that the image can be aggressively cached. It's not - * part of the actual file-on-disk path. - * Similarly, `/mw-1000/` is virtual and will be observed and removed from - * the pathname before trying to look it up as disk-on-file. - * The exact pattern needs to match how it's set in whatever Markdown - * processing code that might make dynamic asset URLs. - * So if you change this, make sure you change the code that expects - * to be able to inject this into the URL. - */ +// Markdown processing injects a max-width segment such as /mw-1440/ into dynamic asset URLs. +// Like /cb-1234/, it marks cacheable work and is not part of the file path on disk. +// Keep this pattern in sync with the Markdown code that builds dynamic asset URLs. const maxWidthPathPartRegex = /\/mw-(\d+)\// -/** - * - * Why not any free number? If we allowed it to be any integer number - * someone would put our backend servers at risk by doing something like: - * - * const makeURL = () => `${BASE}/assets/mw-${Math.floor(Math.random()*1000)}/foo.png` - * await Promise.all([...Array(10000).keys()].map(makeURL)) - * - * Which would be lots of distinctly different and valid URLs that the - * CDN can never really "protect us" on because they're too often distinct. - * - * At the moment, the only business need is for 1,000 pixels, so the array - * only has one. But can change in the future and make this sentence moot. - */ +// Restrict widths to product-supported sizes, so attackers cannot create many +// distinct resize URLs that bypass CDN reuse. const VALID_MAX_WIDTHS = [1440, 1000] +// WebP effort 5 keeps output smaller without using sharp's slowest CPU setting. +// CDN caching lets production pay the conversion cost once per image. +// https://www.peterbe.com/plog/comparing-different-efforts-with-webp-in-sharp +// Lossy WebP is acceptable because these images are rendered for viewing, not source editing. +// Lossless output is slightly crisper but averages 1.8x larger. +// Sharp's default 80% quality and lossy mode make our images 2.8x smaller than PNGs on average. export default async function dynamicAssets( req: ExtendedRequest, res: Response, @@ -53,39 +38,16 @@ export default async function dynamicAssets( return res.status(405).type('text/plain').send('Method Not Allowed') } - // To protect from possible denial of service, we never allow what - // we're going to do (the image file operation), if the whole thing - // won't be aggressively cached. - // If we didn't do this, someone making 2 requests, ... - // - // > GET /assets/images/site/logo.web?random=10476583 - // > GET /assets/images/site/logo.web?random=20196996 - // - // ...would be treated as 2 distinct backend requests. Sure, each one - // would be cached in the CDN, but that's not helping if someone does... - // - // while (true) { - // startFetchThread(`/assets/images/site/logo.web?whatever=${rand()}`) - // } - // - // So we "force" any deviation of the URL to a redirect to the canonical - // URL (which, again, is heavily cached). + // Query strings create distinct dynamic asset URLs, so redirect them to the canonical cached URL. if (Object.keys(req.query).length > 0) { - // Cache the 404 so it won't be re-attempted over and over + // Cache the redirect so repeated noncanonical URLs do not keep reaching the backend. defaultCacheControl(res) - // This redirects to the same URL we're currently on, but with the - // query string part omitted. - // For example: - // - // > GET /assets/images/site/logo.web?foo=bar - // < 302 - // < location: /assets/images/site/logo.web - // + // /assets/images/site/logo.webp?foo=bar redirects to /assets/images/site/logo.webp. return res.safeRedirect(302, req.path) } - // From PNG to WEBP, if the PNG exists + // Dynamic WebP files are generated from PNG sources on demand. if (req.path.endsWith('.webp')) { const { url, maxWidth, error } = deconstructImageURL(req.path) if (error) { @@ -104,49 +66,15 @@ export default async function dynamicAssets( } } - // The default in sharp.webp() for effort is 4. It's a sensible - // balance between time and compression. - // If you make it low, it makes the webp conversion faster. - // If you make it high, the webp conversion is slower but the - // resulting WEBP file are smaller. - // Given that our App Service containers aren't very strong in - // terms of CPU, we avoid the highest effort. But given how - // well our CDN protects repeated requests for the same image, - // we can pay this cost once and reap it for a very long time. - // Be mindful at the highest (6), it can be extremely slow so - // let's avoid that for now. - // - // For more information about the effort option, see: - // https://www.peterbe.com/plog/comparing-different-efforts-with-webp-in-sharp - // let effort = 5 if (process.env.NODE_ENV === 'test') { - // When running tests, we want to make the conversion as fast - // as possible because the resulting WEBP buffer will most - // likely never be enjoyed by network or human eyes. + // Tests need fast conversion because the WebP buffer is not user-visible. effort = 1 } else if (process.env.NODE_ENV === 'development') { - // If you're doing local development (or review), the - // network is not precious (localhost:4000) and you have no - // CDN to cache it for you. Make it low but not too unrealistically - // low. + // Development has no CDN reuse, so reduce conversion CPU cost. effort = 1 } - // Note that by default, sharp will use a lossy compression. - // (i.e. `{lossless: false}` in the options) - // The difference is that a lossless image is slightly crisper - // but becomes on average 1.8x larger. - // Given how we serve images, no human would be able to tell the - // difference simply by looking at the image as it appears as an - // image tag in the web page. - // Also given that rendering-for-viewing is the "end of the line" - // for the image meaning it just ends up being viewed and not - // resaved as a source file. If we had intention to overwrite all - // original PNG source files to WEBP, we should consider lossless - // to preserve as much quality as possible at the source level. - // The default quality is 80% which, combined with `lossless:false` - // makes our images 2.8x smaller than the average PNG. const buffer = await image.webp({ effort }).toBuffer() assetCacheControl(res) return res.type('image/webp').send(buffer) @@ -162,23 +90,13 @@ export default async function dynamicAssets( } } - // Cache the 404 so it won't be re-attempted over and over + // Cache the 404 so repeated missing assets do not keep reaching the backend. defaultCacheControl(res) - // There's a preceeding middleware that sets the Surrogate-Key to - // "manual-purge" based on the URL possibly having the `/cb-xxxxx/` - // checksum in it. But, if it failed, we don't want that. So - // undo that if it was set. - // It's handy too to not overly cache 404s in the CDN because - // it could be that the next prod deployment fixes the missing image. - // For example, a PR landed that introduced the *reference* to the image - // but forgot to check in the new image, then a follow-up PR adds the image. + // Missing dynamic assets use the language surrogate key, not manual-purge, so a later deploy can add the image. setFastlySurrogateKey(res, makeLanguageSurrogateKey(), true) - // Don't use something like `next(404)` because we don't want a fancy - // HTML "Page not found" page response because a failed asset lookup - // is impossibly a typo in the browser address bar or an accidentally - // broken link, like it might be to a regular HTML page. + // Keep missing asset responses plain text instead of rendering the HTML page-not-found response. res.status(404).type('text/plain').send('Asset not found') } diff --git a/src/assets/middleware/static-asset-caching.ts b/src/assets/middleware/static-asset-caching.ts index 250aa2b85d00..75fb6b03f1e0 100644 --- a/src/assets/middleware/static-asset-caching.ts +++ b/src/assets/middleware/static-asset-caching.ts @@ -14,8 +14,7 @@ export default function setStaticAssetCaching( return next() } -// True if the URL is known to contain some pattern of a checksum that -// would make it intelligently different if its content has changed. +// Checksummed URLs can keep manual surrogate keys because content changes produce new URLs. function isChecksummed(path: string) { if (path.startsWith('/assets/cb-')) return true if (path.startsWith('/_next/static')) { diff --git a/src/assets/scripts/deleted-assets-pr-comment.ts b/src/assets/scripts/deleted-assets-pr-comment.ts index 55f129b40ca1..88626e24875b 100755 --- a/src/assets/scripts/deleted-assets-pr-comment.ts +++ b/src/assets/scripts/deleted-assets-pr-comment.ts @@ -8,7 +8,7 @@ if (!GITHUB_TOKEN) { throw new Error(`GITHUB_TOKEN environment variable not set`) } -// When this file is invoked directly from action as opposed to being imported +// Direct workflow execution writes the PR comment body to an action output. if (import.meta.url.endsWith(process.argv[1])) { const owner = context.repo.owner const repo = context.payload.repository?.name || '' @@ -27,7 +27,6 @@ type MainArgs = { } async function main({ owner, repo, baseSHA, headSHA }: MainArgs) { const octokit = getOctokit(GITHUB_TOKEN as string) - // get the list of file changes from the PR const response = await octokit.rest.repos.compareCommitsWithBasehead({ owner, repo, @@ -40,8 +39,7 @@ async function main({ owner, repo, baseSHA, headSHA }: MainArgs) { throw new Error('No files found in the PR') } - // Auto-generated asset directories managed by sync pipelines. - // These are deleted and recreated on each sync, so deletions are expected. + // Sync pipelines delete and recreate these auto-generated asset directories. const AUTO_GENERATED_ASSET_DIRS = ['assets/images/help/copilot/copilot-sdk/'] const oldFilenames = [] @@ -50,10 +48,8 @@ async function main({ owner, repo, baseSHA, headSHA }: MainArgs) { if (!filename.startsWith('assets')) continue if (AUTO_GENERATED_ASSET_DIRS.some((dir) => filename.startsWith(dir))) continue if (status === 'removed') { - // Bad oldFilenames.push(filename) } else if (status === 'renamed') { - // Also bad const previousFilename = file.previous_filename oldFilenames.push(previousFilename) } diff --git a/src/assets/scripts/find-orphaned-assets.ts b/src/assets/scripts/find-orphaned-assets.ts index 15143e4d7fc2..e85c54e87aaa 100755 --- a/src/assets/scripts/find-orphaned-assets.ts +++ b/src/assets/scripts/find-orphaned-assets.ts @@ -1,9 +1,4 @@ -// [start-readme] -// -// Print a list of all the asset files that can't be found mentioned -// in any of the source files (content & code). -// -// [end-readme] +// Prints assets that no content or code file mentions. import fs from 'fs' import path from 'path' @@ -79,7 +74,7 @@ const EXCEPTIONS = new Set([ 'assets/images/social-cards/subscriptions-and-notifications.png', 'assets/images/social-cards/support.png', 'assets/images/social-cards/webhooks.png', - // Hero images may not be used, but we keep them around for future use + // Hero images may be reused even when no source file mentions them. 'assets/images/banner-images/hero-1.png', 'assets/images/banner-images/hero-2.png', 'assets/images/banner-images/hero-3.png', @@ -89,11 +84,7 @@ const EXCEPTIONS = new Set([ ]) function isExceptionPath(imagePath: string) { - // We also check for .DS_Store because any macOS user that has opened - // a folder with images will have this on disk. It won't get added - // to git anyway thanks to our .DS_Store. - // But if we don't make it a valid exception, it can become inconvenient - // to run this script locally. + // Local macOS image folders can contain .DS_Store files that are not tracked. return ( EXCEPTIONS.has(imagePath) || path.basename(imagePath) === '.DS_Store' || @@ -131,10 +122,7 @@ async function main(opts: MainOptions) { sourceFiles.push(...englishFiles) if (!excludeTranslations) { - // Need to have this so we can filter the translations files and avoid - // including orphans. Because translations generally don't delete files. - // When the English content renames something, you later end up with - // 2 files in each translation repo. + // Translations keep files after English renames, so only search translated files that match English. const englishRelativeFiles = new Set( englishFiles.map((englishFile) => path.relative(languages.en.dir, englishFile)), ) @@ -173,7 +161,6 @@ async function main(opts: MainOptions) { ), ) } - // Add exceptions sourceFiles.push('.github/CONTRIBUTING.md') sourceFiles.push('README.md') if (verbose) { @@ -213,8 +200,7 @@ async function main(opts: MainOptions) { console.log(JSON.stringify([...allImages], undefined, 2)) } else { for (const imagePath of [...allImages].sort((a, b) => a.localeCompare(b))) { - // It's important to escape spaces if we're ever going to pipe this - // to xargs. + // Quotes preserve paths with spaces when piped to xargs. console.log(`"${imagePath}"`) } } diff --git a/src/assets/scripts/list-image-sizes.ts b/src/assets/scripts/list-image-sizes.ts index 34a4f8fcdd3f..8c37b9c6308e 100755 --- a/src/assets/scripts/list-image-sizes.ts +++ b/src/assets/scripts/list-image-sizes.ts @@ -1,8 +1,4 @@ -// [start-readme] -// -// This script lists all local image files, sorted by their dimensions. -// -// [end-readme] +// Lists local image files by pixel area, largest first. import { fileURLToPath } from 'url' import path from 'path' diff --git a/src/assets/scripts/validate-asset-images.ts b/src/assets/scripts/validate-asset-images.ts index f799bee711c1..56269406427a 100755 --- a/src/assets/scripts/validate-asset-images.ts +++ b/src/assets/scripts/validate-asset-images.ts @@ -1,15 +1,5 @@ -// [start-readme] -// -// Makes sure that all the image assets in `assets/` are safe. -// -// Generally writers don't check in bogus/corrupt images but mistakes -// can happen and it's ideally spotted in other processes such as -// reviewing PR review environment. -// This script also makes sure that all images really are what they're -// called. For example, an image might be named `screenshot.png` but -// it might actually be something mischievous. -// -// [end-readme] +// Validates asset files for corrupt images, unsafe SVG content, and mismatched file types. +// For example, screenshot.png must contain image/png data. import fs from 'fs/promises' import path from 'path' @@ -24,12 +14,11 @@ import isSVG from 'is-svg' const ASSETS_ROOT = path.resolve('assets') const ROOT = path.dirname(ASSETS_ROOT) -// We put images that are used by the React components in with the assets -// directory. These aren't really content-contibuted. +// React component images live under assets but are not content-contributed. const EXCLUDE_DIR = path.join(ASSETS_ROOT, 'images', 'site') const IGNORE_EXTENSIONS = new Set([ - // Currently has no known test for these + // CSV assets have no validator. '.csv', ]) @@ -105,7 +94,7 @@ async function checkFile(filePath: string) { } if (ext === '.svg') { - // Can't use `fileTypeFromFile` so have to check "manually" + // fileTypeFromFile cannot validate SVG, so parse the text content. const content = await fs.readFile(filePath, 'utf-8') if (!content.trim()) { return [CRITICAL, filePath, 'file is empty'] @@ -133,8 +122,6 @@ async function checkFile(filePath: string) { } else { return [WARNING, filePath, `Don't know how to validate '${ext}'`] } - - // All is well. Nothing to complain about. } function checkSVGContent(content: string) { @@ -148,10 +135,7 @@ function checkSVGContent(content: string) { throw new Error(`contains a <${tagName}> tag`) } for (const key in 'attribs' in el ? el.attribs : {}) { - // Looks for suspicious event handlers on tags. - // For example ` { return context } -// Non-throwing variant: returns null when there is no provider. For components that render -// both inside and outside an AutomatedPageContext.Provider (e.g. the product sidebar, shared -// across automated REST reference pages and conceptual REST pages). Call it unconditionally. +// Returns null outside a provider, so shared navigation can call the hook on REST +// reference and conceptual pages. export const useAutomatedPageContextOptional = (): AutomatedPageContextT | null => { return useContext(AutomatedPageContext) } diff --git a/src/automated-pipelines/components/parameter-table/ChildBodyParametersRows.module.scss b/src/automated-pipelines/components/parameter-table/ChildBodyParametersRows.module.scss index dd2631c562db..b6b5cd611701 100644 --- a/src/automated-pipelines/components/parameter-table/ChildBodyParametersRows.module.scss +++ b/src/automated-pipelines/components/parameter-table/ChildBodyParametersRows.module.scss @@ -3,15 +3,13 @@ border-top: none; } - // Remove any default markdown article padding for property cells + // Markdown article styles add padding that crowds nested property cells. details tr td { padding-bottom: 0.25rem; } - // Set the left border for in the nested property tables. Also need to override - // a default markdown file style that sets a table's font size based on - // percentage which would cause the table font size to shrink more and more - // as the properties nested more and more. + // The muted border distinguishes nested property tables. + // Inherit prevents markdown percentage font sizing from shrinking at each nesting level. td { table { border-left: 4px solid var(--color-border-muted); diff --git a/src/automated-pipelines/components/parameter-table/ParameterRow.tsx b/src/automated-pipelines/components/parameter-table/ParameterRow.tsx index aaf3c91809b1..4752b084e908 100644 --- a/src/automated-pipelines/components/parameter-table/ParameterRow.tsx +++ b/src/automated-pipelines/components/parameter-table/ParameterRow.tsx @@ -15,17 +15,9 @@ type Props = { clickedBodyParameterName?: string | undefined } -// Webhooks have these same properties in common that we describe separately in its -// own section on the webhooks page: -// -// https://docs.github.com/en/developers/webhooks-and-events/webhooks/webhook-events-and-payloads#webhook-payload-object-common-properties -// -// Since there's more details for these particular properties, we chose not -// show their child properties for each webhook and we also don't grab this -// information from the schema. -// -// We use this list of common properties to make sure we don't try and request -// the child properties for these specific properties. +// The webhooks page documents common webhook payload properties in one shared section. +// Skipping their child properties here avoids duplicate schema lookups and repeated docs. +// https://docs.github.com/en/webhooks/webhook-events-and-payloads const NO_CHILD_WEBHOOK_PROPERTIES = [ 'action', 'enterprise', @@ -46,8 +38,6 @@ export function ParameterRow({ }: Props) { const { t } = useTranslation(['parameter_table']) - // This will be true if `rowParams` does not have a key called `default` - // and it will be true if it does and its actual value is `undefined`. const hasDefault = rowParams.default !== undefined return ( <> @@ -58,14 +48,11 @@ export function ParameterRow({ {rowParams.name ? ( <> {rowParams.name} - {/* This whitespace is important otherwise, when the CSS is - ignored, the plain text becomes `foobar` if the HTML - was `foobar`. - */}{' '} + {/* Keeps foobar from rendering as foobar without CSS. */}{' '} {Array.isArray(rowParams.type) ? rowParams.type.join(' or ') : rowParams.type} - {/* Ditto about the important explicit whitespace */}{' '} + {/* Keeps readable text spacing if CSS fails to load. */}{' '} {rowParams.isRequired ? ( {t('required')} ) : null} @@ -75,7 +62,7 @@ export function ParameterRow({ {Array.isArray(rowParams.type) ? rowParams.type.join(' or ') : rowParams.type} - {/* Ditto about the important explicit whitespace */}{' '} + {/* Keeps readable text spacing if CSS fails to load. */}{' '} {rowParams.isRequired ? ( {t('required')} ) : null} @@ -96,10 +83,7 @@ export function ParameterRow({ {t('default')}: {typeof rowParams.default === 'string' - ? // In the schema, the default value for strings can - // potentially be the empty string so we handle this case - // in particular by rendering it as "". Otherwise we would - // display an empty code block which could be confusing. + ? // Empty string defaults need visible quotes. rowParams.default || '""' : JSON.stringify(rowParams.default)} @@ -143,16 +127,7 @@ export function ParameterRow({ /> )} - {/* These conditions tell us: - - 1. the param is an object or array AND: - 2. the param has no child param groups AND: - 3. the param isn't one of the common webhook properties - - If all these are true, then that means we haven't yet loaded the - nested parameters so we show a stub
element that triggers - an API request to get the nested parameter data. - */} + {/* Empty child groups mark unloaded nested params except shared webhook props; details lazy-loads them. */} {rowParams.type && (rowParams.type.includes('object') || rowParams.type.includes('array of')) && rowParams.childParamsGroups && diff --git a/src/automated-pipelines/components/parameter-table/ParameterTable.module.scss b/src/automated-pipelines/components/parameter-table/ParameterTable.module.scss index 2914769e4429..66b19a3bd84c 100644 --- a/src/automated-pipelines/components/parameter-table/ParameterTable.module.scss +++ b/src/automated-pipelines/components/parameter-table/ParameterTable.module.scss @@ -1,18 +1,12 @@ .parameterTable { - // this is for the child parameter table row that contains the top level - // properties details toggle element because we want it to match the - // background color of the top level parameter rows. We need the !important - // because the child parameter rows (the nested expanded properties) otherwise - // have the same background color. + // The top-level child row matches parent row backgrounds, not nested property rows. + // !important keeps the top-level child row from taking the nested child-row background. & > tbody > tr { background: var(--color-canvas-default) !important; } - // also for the top level child parameter table row, we want the details toggle - // to align with the top level parameter rows. Child parameter rows have some - // left padding so they can indent as they nest but we don't want that in - // this case. We need the !important to override general default markdown - // article styling. + // The top-level details toggle aligns with parent rows, while nested rows keep indentation. + // !important overrides default markdown article spacing. & > tbody > tr > td > details { padding-left: 0px !important; margin-bottom: 4px !important; diff --git a/src/automated-pipelines/lib/update-markdown.ts b/src/automated-pipelines/lib/update-markdown.ts index 67ab4bd34183..d5d9002d3e5d 100644 --- a/src/automated-pipelines/lib/update-markdown.ts +++ b/src/automated-pipelines/lib/update-markdown.ts @@ -64,9 +64,6 @@ type ChildrenComparison = { const ROOT_INDEX_FILE = 'content/index.md' export const MARKDOWN_COMMENT = '\n\n' -// Main entrypoint into this module. -// Walks every directory under targetDirectory, adding and removing Markdown -// files and keeping the index.md children and versions frontmatter in sync. export async function updateContentDirectory({ targetDirectory, sourceContent, @@ -79,15 +76,12 @@ export async function updateContentDirectory({ await updateMarkdownFiles(targetDirectory, sourceContent, frontmatter, indexOrder) } -// Remove markdown files that are no longer in the source data async function removeMarkdownFiles( targetDirectory: string, sourceFiles: string[], autogeneratedType: string | undefined, ): Promise { const autogeneratedFiles = await getAutogeneratedFiles(targetDirectory, autogeneratedType) - // If the first array contains items that the second array does not, - // it means that a Markdown page was deleted from the OpenAPI schema const filesToRemove = difference(autogeneratedFiles, sourceFiles) if (filesToRemove.length > 0) { logger.info('Removing stale markdown files', { @@ -100,8 +94,6 @@ async function removeMarkdownFiles( } } -// Gets a list of all files under targetDirectory that have the -// `autogenerated` frontmatter set to `autogeneratedType`. async function getAutogeneratedFiles( targetDirectory: string, autogeneratedType: string | undefined, @@ -124,9 +116,6 @@ async function getAutogeneratedFiles( ).filter(Boolean) as string[] } -// The `sourceContent` object contains the new content and target file -// path for the Markdown files. Ex: -// { : { data: , content: } } async function updateMarkdownFiles( targetDirectory: string, sourceContent: SourceContent, @@ -137,14 +126,12 @@ async function updateMarkdownFiles( await updateMarkdownFile(file, newContent.data, newContent.content) } await updateDirectory(targetDirectory, frontmatter, { indexOrder }) - // The pipelines should not touch directories they do not own, so this - // call updates only the index.md file in the parent directory. + // Update only the parent index because pipelines must not touch sibling directories. await updateDirectory(path.dirname(targetDirectory), frontmatter, { rootDirectoryOnly: true }) } -// If the Markdown file already exists on disk, we update only the content -// and the versions frontmatter, so writers can hand-edit the other fields. -// If it does not exist, we create it. +// Existing autogenerated pages keep writer-edited frontmatter except versions. +// New pages use source frontmatter because no writer edits exist yet. async function updateMarkdownFile( file: string, sourceData: FrontmatterData, @@ -154,7 +141,7 @@ async function updateMarkdownFile( if (existsSync(file)) { const { data, content } = matter(await readFile(file, 'utf-8')) - // Double check that the comment delimiter is only used once + // Multiple delimiters make the writer-owned and generated content split unsafe. const matcher = new RegExp(commentDelimiter, 'g') const matches = content.match(matcher) if (matches && matches.length > 1) { @@ -181,9 +168,7 @@ async function updateMarkdownFile( delimiterMissing: isDelimiterMissing, }) - // Create a new object so that we don't mutate the original data const newData = { ...data } - // Only modify the versions property when a file already exists newData.versions = sourceData.versions const targetContent = manuallyCreatedContent + commentDelimiter + sourceContent const newFileContent = appendVersionComment(matter.stringify(targetContent, newData)) @@ -198,10 +183,7 @@ async function updateMarkdownFile( } } -// Recursively walks through the directory structure and updates the -// index.md files to match the disk. Before calling this function -// ensure that the Markdown files have been updated and any files -// that need to be deleted have been removed. +// Call after Markdown updates and deletions because child index files mirror disk state. async function updateDirectory( directory: string, frontmatter: FrontmatterData, @@ -225,8 +207,7 @@ async function updateDirectory( const indexFile = `${directory}/index.md` const { data, content } = await getIndexFileContents(indexFile, frontmatter, shortTitle) - // We need to re-get the directory contents because a recursive call - // may have removed a directory since the initial directory read. + // Recursive calls may remove child directories, so read the directory again before syncing. const { directoryContents, childDirectories, directoryFiles } = await getDirectoryInfo(directory) const { childrenOnDisk, indexChildren } = getChildrenToCompare( @@ -262,12 +243,8 @@ async function updateDirectory( await writeFile(indexFile, matter.stringify(content, dataUpdatedChildren)) } -// Takes the children properties from the index.md file and the -// files/directories on disk and normalizes them to be comparable -// against each other. -// Children properties include a leading slash except when the -// index.md file is the root index.md file. We also want to -// remove the file extension from the files on disk. +// Root index children omit the leading slash; other index.md children include it. +// Disk entries include extensions, so normalize both sides before comparing them. function getChildrenToCompare( indexFile: string, directoryContents: string[], @@ -291,21 +268,8 @@ function getChildrenToCompare( return { childrenOnDisk, indexChildren } } -// Adds and removes children properties to the index.md file. -// There are three possible scenarios that we want to handle: -// -// 1. If the lib/config.json file for the pipeline defines a sort -// order for the index file we're currently processing, then -// we want to use that sort order. Currently, the config files -// only defined a startsWith parameter. This property defines -// the order of the first items in the index files children -// property. All other items are sorted and appended to list. -// -// 2. If no config is defined and the index file is an -// autogenerated file, we sort all the children alphabetically. -// -// 3. If the index file is not autogenerated, we leave the ordering -// as is and append new children to the end. +// Autogenerated indexes use config startsWith ordering first, then alphabetical entries. +// Manual indexes keep existing order and append new children to the end. function updateIndexChildren( data: FrontmatterData, childUpdates: ChildUpdates, @@ -317,17 +281,13 @@ function updateIndexChildren( const childPrefix = rootIndex ? '' : '/' const children = [...(data.children || [])] - // remove the '/' prefix used in index.md children .map((item) => item.replace(childPrefix, '')) .filter((item) => !itemsToRemove.includes(item)) children.push(...itemsToAdd) const orderedIndexChildren: string[] = [] - // Only used for tests. During testing, the content directory is - // in a temp directory so the paths are not relative to - // the current working directory. This gets the relative path - // from the full path to the index file. + // Tests run content in a temp directory, so config keys need repo-relative index paths. const indexRelativePath = process.env.TEST_OS_ROOT_DIR ? indexFile.replace(`${process.env.TEST_OS_ROOT_DIR}/`, '') : indexFile @@ -340,24 +300,19 @@ function updateIndexChildren( orderedIndexChildren.push(...indexOrderConfig.startsWith, ...sortableChildren) } } else if (isAutogenerated) { - // always sort autogenerated index files that have no override config orderedIndexChildren.push(...children) orderedIndexChildren.sort() } else { - // just leave the children in the order they are in the index file - // so they can be manually sorted + // Manual index files keep writer-defined ordering. orderedIndexChildren.push(...children) } const updatedData = { ...data } - // add the '/' prefix back to the children updatedData.children = orderedIndexChildren.map((item) => `${childPrefix}${item}`) return updatedData } -// Gets the contents of the index.md file from disk if it exists, -// or returns default frontmatter for a new one. async function getIndexFileContents( indexFile: string, frontmatter: FrontmatterData, @@ -379,9 +334,6 @@ async function getIndexFileContents( return existsSync(indexFile) ? matter(await readFile(indexFile, 'utf-8')) : indexFileContent } -// Builds the index.md versions frontmatter by consolidating -// the versions from each Markdown file in the directory + the -// index.md files in any subdirectories of directory. async function getIndexFileVersions( directory: string, files: string[], @@ -414,42 +366,22 @@ async function getIndexFileVersions( return await convertVersionsToFrontmatter(versionArray) } -/* Takes a list of versions in the format: -[ - 'free-pro-team@latest', - 'enterprise-cloud@latest', - 'enterprise-server@3.3', - 'enterprise-server@3.4', - 'enterprise-server@3.5', - 'enterprise-server@3.6', - 'enterprise-server@3.7' -] -and returns the frontmatter equivalent JSON: -{ - fpt: '*', - ghec: '*', - ghes: '*' -} -*/ +// Converts applicable versions to versions frontmatter. +// Example: free-pro-team@latest, enterprise-cloud@latest, and every supported GHES release +// become fpt: *, ghec: *, and ghes: *. export async function convertVersionsToFrontmatter( versions: string[], ): Promise<{ [key: string]: string }> { const frontmatterVersions: { [key: string]: string } = {} const numberedReleases: { [key: string]: { availableReleases: (string | undefined)[] } } = {} - // Currently, only GHES is numbered. Number releases have to be - // handled differently because they use semantic versioning. + // GHES uses semantic version ranges because it has numbered releases. for (const version of versions) { const docsVersion = allVersions[version] if (!docsVersion.hasNumberedReleases) { frontmatterVersions[docsVersion.shortName] = '*' } else { - // Each version that has numbered releases in allVersions - // has a string for the number (currentRelease) and an array - // of all of the available releases (e.g. ['3.3', '3.4', '3.5']) - // This creates an array of the applicable releases in the same - // order as the available releases array. This is used to track when - // a release is no longer supported. + // Track supported GHES releases by position so gaps become explicit ranges later. const i = docsVersion.releases.indexOf(docsVersion.currentRelease) if (!numberedReleases[docsVersion.shortName]) { const availableReleases: (string | undefined)[] = Array(docsVersion.releases.length).fill( @@ -465,15 +397,13 @@ export async function convertVersionsToFrontmatter( } } - // Create semantic versions for numbered releases for (const key of Object.keys(numberedReleases)) { const availableReleases = numberedReleases[key].availableReleases const versionContinuity = checkVersionContinuity(availableReleases) if (availableReleases.every(Boolean)) { frontmatterVersions[key] = '*' } else if (!versionContinuity) { - // If there happens to be version gaps, just enumerate each version - // using syntax like =3.x || =3.x + // Gapped releases must enumerate each supported version, such as =3.3 || =3.5. const semVer = availableReleases .filter(Boolean) .map((release) => `=${release}`) @@ -501,14 +431,11 @@ export async function convertVersionsToFrontmatter( return sortedFrontmatterVersions } -// This is uncommon, but we potentially could have the case where an -// article was versioned for say 3.2, not for 3.3, and then again -// versioned for 3.4. This will result in a custom semantic version range +// A gap between supported versions, such as 3.2 and 3.4 without 3.3, needs a custom range. function checkVersionContinuity(versions: (string | undefined)[]): boolean { const availableVersions = [...versions] - // values at the beginning or end of the array are not gaps but normal - // starts and ends of version ranges + // Missing values at the ends mark normal range boundaries, not gaps. while (!availableVersions[0]) { availableVersions.shift() } diff --git a/src/automated-pipelines/tests/rendering.ts b/src/automated-pipelines/tests/rendering.ts index f8dfc17ca55f..9e729afc102f 100644 --- a/src/automated-pipelines/tests/rendering.ts +++ b/src/automated-pipelines/tests/rendering.ts @@ -24,19 +24,17 @@ describe('autogenerated docs render', () => { const autogeneratedPages = pageList.filter((page: Page) => page.autogenerated) test('all automated pages', async () => { - // Each page should render with 200 OK. Also, check for duplicate - // heading IDs on each page. + // Each page must render with 200 OK and unique heading IDs. const errors = ( await Promise.all( autogeneratedPages.map(async (page: Page) => { const url = page.permalinks[0].href - // Some autogenerated pages can be very slow and might fail. - // So we allow a few retries to avoid false positives. + // Slow autogenerated pages get retries to avoid false-positive failures. const res = await get(url, { retries: 3 }) if (res.statusCode !== 200) { return `${res.statusCode} status error on ${url}` } - // Using `xmlMode: true` is marginally faster + // xmlMode is faster for this duplicate-ID scan. const $ = load(res.body, { xmlMode: true }) const headingIDs = $('body') .find('h2, h3, h4, h5, h6') @@ -64,10 +62,8 @@ describe('autogenerated docs render', () => { const ghappsPath: string = JSON.parse( readFileSync('src/github-apps/lib/config.json', 'utf-8'), ).targetDirectory - // Right now only the rest and codeqlcli pages get their frontmatter updated automatically. - // The apps pages do not get their frontmatter auto-updated since they apply to all versions and they are - // single pages. The apps pages are also nested inside of the rest pages. So we want to filter out only - // rest pages and the codeql cli pages for this test. + // Only REST and CodeQL CLI pages get automated version frontmatter updates. + // GitHub Apps pages apply to all versions and nest under REST, so exclude them. const filesWithAutoUpdatedVersions = autogeneratedPages.filter( (page: Page) => (!page.fullPath.startsWith(ghappsPath) && page.fullPath.startsWith(restPath)) || diff --git a/src/automated-pipelines/tests/update-markdown.ts b/src/automated-pipelines/tests/update-markdown.ts index a617287046ff..827d90d27228 100644 --- a/src/automated-pipelines/tests/update-markdown.ts +++ b/src/automated-pipelines/tests/update-markdown.ts @@ -70,10 +70,8 @@ const indexOrder: IndexOrder = { } describe('automated content directory updates', () => { - // Before all tests, copy the content directory fixture - // to the operating systems temp directory. We'll be modifying - // that temp directory during the tests and comparing the directory - // structure and contents after running updateContentDirectory. + // Tests mutate a temp copy of src/automated-pipelines/tests/fixtures/content, then compare + // the resulting file tree and frontmatter after updateContentDirectory runs. beforeAll(async () => { process.env.TEST_OS_ROOT_DIR = tempDirectory mkdirSync(`${tempContentDirectory}`, { recursive: true }) @@ -81,10 +79,7 @@ describe('automated content directory updates', () => { recursive: true, }) - // The updateContentDirectory uses relative paths to the content directory - // because outside of testing it only runs in the docs-internal repo. - // Because of that, we need to update the content paths to use the - // full file path. + // Temp fixtures need absolute paths because this test runs outside the repo content root. const contentDataFullPath: { [key: string]: ContentItem } = {} for (const key of Object.keys(newContentData)) { contentDataFullPath[path.join(targetDirectory, key)] = newContentData[key] @@ -127,7 +122,6 @@ describe('automated content directory updates', () => { }) test('rest/actions index file is updated as expected', async () => { - // workflows added and artifacts removed const actionsIndex = matter( await readFile(`${tempDirectory}/content/rest/actions/index.md`, 'utf8'), ) diff --git a/src/codeql-cli/scripts/convert-markdown-for-docs.ts b/src/codeql-cli/scripts/convert-markdown-for-docs.ts index 2462ba5d927e..7bdf8bcd34bb 100644 --- a/src/codeql-cli/scripts/convert-markdown-for-docs.ts +++ b/src/codeql-cli/scripts/convert-markdown-for-docs.ts @@ -68,7 +68,6 @@ export async function convertContentToDocs( visit(ast, 'heading', (rawNode) => { const node = rawNode as unknown as MdNode - // A level 1 heading is the article title. if (node.depth === 1) { frontmatter.title = node.children[0].value } @@ -78,15 +77,12 @@ export async function convertContentToDocs( node.children[0].value = node.children[0].value.split('{#')[0].trim() } - // Works around secondary options sitting at the wrong heading level - // in the source rst files. - // Everything after the "Synopsis", "Description", and "Options" - // headings moves up one level, so h4 becomes h3. + // Headings after Primary options sit one level too deep in source rst, so shift them up. if (secondaryOptions) { node.depth = Math.max(1, Math.min(6, node.depth - 1)) } - // This needs to be assigned after node.depth is modified above + // Capture depth after secondary options shift changes node.depth. depth = node.depth if (node.children[0].value === LAST_PRIMARY_HEADING && node.children[0].type === 'text') { secondaryOptions = true @@ -98,8 +94,7 @@ export async function convertContentToDocs( const node = rawNode as unknown as MdNode if (node.type !== 'heading' && node.type !== 'paragraph') return false - // The first paragraph after the "Description" heading - // becomes the intro frontmatter. + // The first paragraph after Description becomes intro frontmatter. if (node.children[0]?.value === 'Description' && node.children[0]?.type === 'text') { currentNodeIsDescription = true } @@ -125,10 +120,7 @@ export async function convertContentToDocs( node.meta = 'copy' } - // The start of a secondary options section, for example - // "Output format options." - // The rst file gives these no heading level, so nest them one level - // under `depth`, the last heading level seen by the walk above. + // Secondary labels "Output format options." lack depth; depth+1 nests under last heading. if (node.type === 'text' && node.value && node.value.includes(HEADING_BEGIN)) { node.value = node.value.replace(HEADING_BEGIN, '') // Ancestors run root first, so the last one is the parent. @@ -136,8 +128,7 @@ export async function convertContentToDocs( ancestors[ancestors.length - 1].depth = Math.max(1, Math.min(6, depth + 1)) } - // Keywords like [Plumbing] come from the source code comments - // and should not render in the docs. + // Source code keywords like [Plumbing] do not belong in docs output. if (node.type === 'text' && node.value) { for (const keyword of removeKeywords) { if (node.value.includes(keyword)) { @@ -146,8 +137,7 @@ export async function convertContentToDocs( } } - // Subsections under the level 2 headings are commands - // starting with `-` or `<`, so render them as inline code. + // Level 2 command headings start with - or <, so render them as inline code. if ( node.type === 'text' && ancestors[ancestors.length - 1].type === 'heading' && @@ -161,13 +151,7 @@ export async function convertContentToDocs( node.value = node.value.replace(END_SECTION, '') } - // Links to other CodeQL CLI docs, which need to become Markdown links. - // Pandoc converts the rst links to this shape: - // `codeql test run`{.interpreted-text role="doc"} - // giving a link title of `codeql test run` and a relative path of - // `test-run`. The rest can be dropped. - // The inline code tag is one node and the {.interpreted-text} string - // is another. + // Pandoc emits "codeql test run" as inline code plus role marker; convert to link. if (node.type === 'text' && node.value.includes('{.interpreted-text')) { const paragraph = ancestors[ancestors.length - 1].children const docRoleTagChild = paragraph.findIndex( @@ -189,7 +173,7 @@ export async function convertContentToDocs( node.value = node.value.replace(/\n/g, ' ').replace('{.interpreted-text role="doc"}', '') - // A link to the file being converted would be circular. + // Links to the file being converted would be circular. const currentFileBaseName = currentFileName.replace('.md', '') if (currentFileBaseName && linkPath === currentFileBaseName) { link.type = 'text' @@ -202,15 +186,12 @@ export async function convertContentToDocs( } } - // Collect aka.ms links to resolve after the tree walk. + // Resolve aka.ms redirects after the tree walk, because visit callbacks cannot await. if (node.type === 'link' && node.url.includes('aka.ms')) { akaMsLinkMatches.push(node) } - // Example links like https://containers.GHEHOSTNAME should not be - // checked by the link checker, so render them as inline code. - // The Java program that generates the rst files should do this instead. - // See https://github.com/syntax-tree/mdast#inlinecode + // Render https://containers.GHEHOSTNAME example links as inline code so the link checker skips them. if (node.type === 'link' && node.url.startsWith('https://containers')) { // Strip the double quotes from the nodes either side. const nodeBefore = ancestors[ancestors.length - 1].children[0] @@ -237,12 +218,11 @@ export async function convertContentToDocs( }, ) - // Convert all aka.ms links to the docs.github.com relative path + // aka.ms redirects supply the docs.github.com relative path. await Promise.all( akaMsLinkMatches.map(async (node: MdNode) => { const url = await getRedirect(node.url) - // These are already Markdown links in the ast, - // so only the url and the link text need updating. + // Existing Markdown links only need AUTOTITLE text and the resolved URL. if (node.children[0]) { node.children[0].value = 'AUTOTITLE' } diff --git a/src/codeql-cli/scripts/sync.ts b/src/codeql-cli/scripts/sync.ts index 479bd83de914..ec4390385eba 100755 --- a/src/codeql-cli/scripts/sync.ts +++ b/src/codeql-cli/scripts/sync.ts @@ -30,10 +30,7 @@ async function main() { for (const file of markdownFiles) { const sourceContent = await readFile(file, 'utf8') - // The source content is missing a "Primary Options" heading directly - // under "Options". - // Adding a node to the AST is fiddly when it is not a child of the - // previous heading, so append the heading to the raw Markdown instead. + // Source Markdown lacks a Primary Options heading under Options; raw text avoids AST insertion. const matchHeading = '## Options\n' const primaryHeadingSourceContent = sourceContent.replace( matchHeading, diff --git a/src/codeql-queries/scripts/generate-code-quality-query-list.ts b/src/codeql-queries/scripts/generate-code-quality-query-list.ts index a0348201000a..eb422526288c 100644 --- a/src/codeql-queries/scripts/generate-code-quality-query-list.ts +++ b/src/codeql-queries/scripts/generate-code-quality-query-list.ts @@ -1,41 +1,13 @@ -/** - * This script generates a block of Markdown that can be saved as a reusable. - * The reusable lists all the code quality queries for one programming language, with categories, as a Markdown table. - * - * To be able to execute this script, you need to have the CodeQL CLI installed. - * To do that, you need two things: - * - * 1. The directory where the github/codeql repo is cloned - * 2. The path to the executable `codeql` file. - * - * The directory where the github/codeql repo is cloned is needed because - * that's how it looks up files. You can set it up like this: - * - * cd /tmp - * git clone git@github.com:github/codeql.git - * cd codeql - * pwd - * - * To install the codeql executable, use `gh` like this: - * - * gh extension install github/gh-codeql - * gh codeql set-channel nightly - * gh codeql version - * - * Note that when you run the `gh codeql version` command, it will tell you - * where the executable is installed. For example: - * - * /Users/peterbe/.local/share/gh/extensions/gh-codeql/dist/nightly/codeql-bundle-20231204/codeql - * - * If you've git cloned github/codeql in /tmp/ now you can execute this script. - * For example, to generate the Markdown - * for Python: - * - * npm run generate-code-quality-query-list -- \ - * --codeql-path ~/.local/share/gh/extensions/gh-codeql/dist/nightly/codeql-bundle-20231204/codeql \ - * --codeql-dir /tmp/codeql python | tee /tmp/python.md - * less /tmp/python.md - */ +// Generates reusable Markdown listing code quality queries for one language, with categories. +// Requires a local github/codeql clone and a CodeQL CLI executable. +// Set up the clone with git clone git@github.com:github/codeql.git /tmp/codeql. +// Install the CLI with gh extension install github/gh-codeql, then gh codeql set-channel nightly. +// Run gh codeql version to find the installed codeql path. +// Example: +// npm run generate-code-quality-query-list -- \ +// --codeql-path ~/.local/share/gh/extensions/gh-codeql/dist/nightly/codeql-bundle-*/codeql \ +// --codeql-dir /tmp/codeql python | tee /tmp/python.md +// Inspect the generated Markdown with less /tmp/python.md. import fs from 'fs' import { execFileSync } from 'child_process' @@ -122,7 +94,7 @@ async function main(options: Options, language: string) { const categories = getCategories(tags || '') const url = getDocsLink(language, id) - // Only include queries that have categories + // Category-less queries have no code quality docs row. if (categories.length) { queries[id] = { url, name, categories, severity: severity || 'N/A' } } else { @@ -135,8 +107,7 @@ async function main(options: Options, language: string) { } function decorate(query: Query): QueryExtended { - // Determine primary category for sorting - // Prefer 'maintainability' over 'reliability' + // Maintainability outranks reliability for table sorting. const primaryCategory = query.categories.includes('maintainability') ? 'maintainability' : query.categories.includes('reliability') @@ -151,7 +122,7 @@ async function main(options: Options, language: string) { const entries = Object.values(queries).map(decorate) - // Sort by primary category (maintainability first), then alphabetically by name + // Sort by primary category, then alphabetically by name. entries.sort((a, b) => { if (a.primaryCategory === 'maintainability' && b.primaryCategory !== 'maintainability') return -1 @@ -170,25 +141,23 @@ async function main(options: Options, language: string) { function printQueries(options: Options, queries: QueryExtended[]) { const markdown: string[] = [] markdown.push('{% rowheaders %}') - markdown.push('') // blank line + markdown.push('') const header = ['Query name', 'Category', 'Severity'] markdown.push(`| ${header.join(' | ')} |`) markdown.push(`| ${header.map(() => '---').join(' | ')} |`) for (const query of queries) { const markdownLink = `[${query.name}](${query.url})` - // Capitalize first letter of category for display const categoryDisplay = query.categories .map((cat) => cat.charAt(0).toUpperCase() + cat.slice(1)) .join(', ') - // Capitalize first letter of severity for display const severityDisplay = query.severity.charAt(0).toUpperCase() + query.severity.slice(1) const row = [markdownLink, categoryDisplay, severityDisplay] markdown.push(`| ${row.join(' | ')} |`) } - markdown.push('') // blank line + markdown.push('') markdown.push('{% endrowheaders %}') - markdown.push('') // always end with a blank line + markdown.push('') if (options.outputFile === 'stdout') { console.log(markdown.join('\n')) @@ -203,9 +172,7 @@ function getMetadata(options: Options, queryFile: string): QueryMetadata { }) const parsed = JSON.parse(metadataJson) - // Extract severity from various possible locations in the metadata - // CodeQL metadata can have @problem.severity in the query file, which may be - // represented in different ways in the JSON output from `codeql resolve metadata` + // CodeQL emits severity through several metadata shapes, depending on the query source. const severity = parsed.problem?.severity || // Nested: { problem: { severity: "error" } } parsed['@problem']?.severity || // Nested with @: { "@problem": { severity: "error" } } @@ -215,7 +182,7 @@ function getMetadata(options: Options, queryFile: string): QueryMetadata { parsed['@severity'] // With @: { "@severity": "error" } if (options.verbose) { - // On first query only, show all available keys to help debug + // Verbose mode logs metadata keys once to avoid noisy output. if (!getMetadata.shownKeys) { console.log(chalk.yellow('Available metadata keys:'), Object.keys(parsed)) if (parsed.problem) { @@ -240,24 +207,15 @@ function getMetadata(options: Options, queryFile: string): QueryMetadata { } } -// Add a property to track if we've shown keys getMetadata.shownKeys = false -/** - * - * @param language 'cpp' - * @param queryId 'external-entity-expansion' - * @returns https://codeql.github.com/codeql-query-help/cpp/cpp-external-entity-expansion/ - */ +// Example: cpp and external-entity-expansion become +// https://codeql.github.com/codeql-query-help/cpp/cpp-external-entity-expansion/ function getDocsLink(language: string, queryId: string) { return `https://codeql.github.com/codeql-query-help/${language}/${queryId.replaceAll('/', '-')}/` } -/** - * - * @param tags 'maintainability readability reliability external/cwe/cwe-1078 external/cwe/cwe-670 security' - * @returns ['maintainability', 'reliability'] - */ +// Example tags with maintainability and reliability return those categories in source order. function getCategories(tags: string) { const categories: string[] = [] for (const tag of tags.split(/\s+/g)) { diff --git a/src/codeql-queries/scripts/generate-code-scanning-query-list.ts b/src/codeql-queries/scripts/generate-code-scanning-query-list.ts index 8dfa75149b2c..e9bafc2b98dd 100644 --- a/src/codeql-queries/scripts/generate-code-scanning-query-list.ts +++ b/src/codeql-queries/scripts/generate-code-scanning-query-list.ts @@ -1,59 +1,23 @@ -/** - * This script generates a block of Markdown that can be saved as a reusable. - * The reusable lists all the queries for one programming language, with CWEs, as a Markdown table. - * - * To be able to execute this script, you need to have the CodeQL CLI installed. - * To do that, you need two things: - * - * 1. The directory where the github/codeql repo is clone - * 2. The path to the executable `codeql` file. - * - * The directory where the github/codeql repo is cloned is needed because - * that's how it looks up files. You can set it up like this: - * - * cd /tmp - * git clone git@github.com:github/codeql.git - * cd codeql - * pwd - * - * To install the codeql executable, use `gh` like this: - * - * gh extension install github/gh-codeql - * gh codeql set-channel nightly - * gh codeql version - * - * Note that when you run the `gh codeql version` command, it will tell you - * where the executable is installed. For example: - * - * /Users/peterbe/.local/share/gh/extensions/gh-codeql/dist/nightly/codeql-bundle-20231204/codeql - * - * Finally, you need to install `@github/cocofix`. This is a private package, - * so you first need to get the `DOCS_BOT_PAT_BASE` PAT from the vault and - * store it in the environment variable `DOCS_BOT_PAT_BASE`. - * Then run the following command from the root of this repo: - * - * ```sh - * npm i --no-save '--@github:registry=https://npm.pkg.github.com' '--//npm.pkg.github.com/:_authToken=${DOCS_BOT_PAT_BASE}' @github/cocofix - * ``` - * - * If you've git cloned github/codeql in /tmp/ now you can execute this script. - * For example, to generate the Markdown - * for Python: - * - * npm run generate-code-scanning-query-list -- \ - * --codeql-path ~/.local/share/gh/extensions/gh-codeql/dist/nightly/codeql-bundle-20231204/codeql \ - * --codeql-dir /tmp/codeql python | tee /tmp/python.md - * less /tmp/python.md - */ +// Generates reusable Markdown listing CodeQL code scanning queries for one language, with CWEs. +// Requires a local github/codeql clone and a CodeQL CLI executable. +// Set up the clone with git clone git@github.com:github/codeql.git /tmp/codeql. +// Install the CLI with gh extension install github/gh-codeql, then gh codeql set-channel nightly. +// Run gh codeql version to find the installed codeql path. +// Also requires @github/cocofix, installed locally with DOCS_BOT_PAT_BASE from the vault: +// npm i --no-save '--@github:registry=https://npm.pkg.github.com' \ +// '--//npm.pkg.github.com/:_authToken=${DOCS_BOT_PAT_BASE}' @github/cocofix +// Example: +// npm run generate-code-scanning-query-list -- \ +// --codeql-path ~/.local/share/gh/extensions/gh-codeql/dist/nightly/codeql-bundle-*/codeql \ +// --codeql-dir /tmp/codeql python | tee /tmp/python.md +// Inspect the generated Markdown with less /tmp/python.md. import fs from 'fs' import { execFileSync } from 'child_process' import chalk from 'chalk' import { program } from 'commander' -// We don't want to introduce a global dependency on @github/cocofix, so we install it by hand -// as described above and suppress the import warning. -// eslint-disable-next-line import/no-unresolved -- @github/cocofix is installed manually +// eslint-disable-next-line import/no-unresolved -- @github/cocofix stays manual to avoid a global dependency import { getSupportedQueries } from '@github/cocofix/dist/querySuites' import type { Language } from 'codeql-ts' @@ -150,8 +114,7 @@ async function main(options: Options, language: string) { const url = getDocsLink(language, id) const autofixSupport = autofixSupportedQueryIds.includes(id) ? 'default' : 'none' - // Only include queries that have CWEs, since the other queries deal with code scanning - // metadata and metrics (e.g. counting lines of code or number of files) and have no docs link + // CWE-less queries cover metadata or metrics and have no docs link. if (cwes.length) { if (!(id in queries)) { queries[id] = { url, name, packs: [], cwes, autofixSupport } @@ -178,8 +141,7 @@ async function main(options: Options, language: string) { const entries = Object.values(queries).map(decorate) - // Spec: "Queries that are both in Default and Extended should come first, - // in alphabetical order. Followed by the queries that are in Extended only." + // Default-and-Extended queries sort before Extended-only queries; each group sorts by name. entries.sort((a, b) => { if (a.inDefault && !b.inDefault) return -1 else if (!a.inDefault && b.inDefault) return 1 @@ -196,7 +158,7 @@ async function main(options: Options, language: string) { function printQueries(options: Options, queries: QueryExtended[]) { const markdown: string[] = [] markdown.push('{% rowheaders %}') - markdown.push('') // blank line + markdown.push('') const header = [ 'Query name', 'Related CWEs', @@ -218,9 +180,9 @@ function printQueries(options: Options, queries: QueryExtended[]) { const row = [markdownLink, query.cwes.join(', '), defaultIcon, extendedIcon, autofixIcon] markdown.push(`| ${row.join(' | ')} |`) } - markdown.push('') // blank line + markdown.push('') markdown.push('{% endrowheaders %}') - markdown.push('') // always end with a blank line + markdown.push('') if (options.outputFile === 'stdout') { console.log(markdown.join('\n')) @@ -237,21 +199,13 @@ function getMetadata(options: Options, queryFile: string): QueryMetadata { return parsed } -/** - * - * @param language 'cpp' - * @param queryId 'external-entity-expansion' - * @returns https://codeql.github.com/codeql-query-help/cpp/cpp-external-entity-expansion/ - */ +// Example: cpp and external-entity-expansion become +// https://codeql.github.com/codeql-query-help/cpp/cpp-external-entity-expansion/ function getDocsLink(language: string, queryId: string) { return `https://codeql.github.com/codeql-query-help/${language}/${queryId.replaceAll('/', '-')}/` } -/** - * - * @param tags 'maintainability readability external/cwe/cwe-1078 external/cwe/cwe-670 security' - * @returns ['1078', '670'] - */ +// Example tags with external/cwe/cwe-1078 and external/cwe/cwe-670 return 1078 and 670. function getCWEs(tags: string) { const cwes: string[] = [] for (const tag of tags.split(/\s+/g)) { diff --git a/src/color-schemes/components/BrandThemeProvider.tsx b/src/color-schemes/components/BrandThemeProvider.tsx index 4c1387cfdb35..78a1192538f6 100644 --- a/src/color-schemes/components/BrandThemeProvider.tsx +++ b/src/color-schemes/components/BrandThemeProvider.tsx @@ -3,7 +3,7 @@ import { ThemeProvider } from '@primer/react-brand' import { getBrandColorMode, type BrandColorMode } from '@/color-schemes/lib/get-brand-color-mode' -// Brand reads `colorMode="auto"` as "snapshot the OS on mount" rather than +// Brand reads colorMode="auto" as "snapshot the OS on mount" rather than // "inherit", so this only ever passes a concrete mode. export const BrandThemeProvider = ({ children }: PropsWithChildren) => { // Seeded to match SSR; reading the DOM here would break hydration. @@ -11,7 +11,7 @@ export const BrandThemeProvider = ({ children }: PropsWithChildren) => { useEffect(() => { setColorMode(getBrandColorMode()) - // colorModeScript re-stamps when the OS flips under `auto`. + // colorModeScript re-stamps when the OS flips under auto. const observer = new MutationObserver(() => setColorMode(getBrandColorMode())) observer.observe(document.documentElement, { attributes: true, @@ -20,8 +20,7 @@ export const BrandThemeProvider = ({ children }: PropsWithChildren) => { return () => observer.disconnect() }, []) - // Brand spreads rest props after its own attribute, so `data-color-mode={undefined}` - // drops it from the wrapper div; the prop still feeds brand's context. + // Brand spreads rest props last, so undefined removes the wrapper attribute but keeps context. return ( {children} diff --git a/src/color-schemes/components/useTheme.ts b/src/color-schemes/components/useTheme.ts index b51c1daff79d..c6078196be94 100644 --- a/src/color-schemes/components/useTheme.ts +++ b/src/color-schemes/components/useTheme.ts @@ -62,8 +62,8 @@ function filterMode(mode = ''): CssColorMode | undefined { } } -// `?? {}` rather than a default parameter: a default only covers `undefined`, and -// the cookie can carry an explicit `null` (`{"light_theme":null}`). +// Use ?? {} because a default parameter covers undefined, but the cookie can carry +// explicit null, for example {"light_theme":null}. function filterTheme( theme?: { name?: string; color_mode?: string } | null, ): SupportedTheme | undefined { @@ -102,6 +102,8 @@ export function getComponentTheme(cookieValue = ''): ComponentColorTheme { } } +// setTimeout(0) defers cookie reads until after Primer React's effect, which otherwise +// overrides the cookie color mode and reverts the page to auto. export function useTheme() { const [theme, setTheme] = useState({ css: defaultCSSTheme, @@ -109,10 +111,6 @@ export function useTheme() { }) useEffect(() => { - // setTimeout(0) defers this past Primer React's own useEffect, - // which otherwise overrides the cookie's color mode and reverts the page to auto. - // Primer's migration to CSS variables should remove the need for this. - // https://github.com/primer/react/issues/2229 setTimeout(() => { const cookieValue = Cookies.get(COLOR_MODE_COOKIE_NAME) const css = getCssTheme(cookieValue) diff --git a/src/color-schemes/lib/color-mode-script.ts b/src/color-schemes/lib/color-mode-script.ts index bf5833843821..b9a4097cc4c6 100644 --- a/src/color-schemes/lib/color-mode-script.ts +++ b/src/color-schemes/lib/color-mode-script.ts @@ -1,21 +1,19 @@ import { COLOR_MODE_COOKIE_NAME } from '@/frame/lib/constants' import { CssColorMode, SupportedTheme, defaultCSSTheme } from '@/color-schemes/components/useTheme' -// A tiny script that runs synchronously in the document , before the -// browser's first paint. It reads the `color_mode` cookie (set by github.com, -// not HttpOnly) and writes the matching `data-color-mode`, `data-light-theme`, -// and `data-dark-theme` attributes onto the element. Without this, the -// page first paints with the SSR default theme and only switches to the user's -// real theme after the React bundle hydrates, causing a visible flash. +// This script runs synchronously in the document head before first paint. It reads the +// color_mode cookie from github.com, which is not HttpOnly, and writes data-color-mode, +// data-light-theme, and data-dark-theme attributes on html. Without it, the page paints +// with the SSR default theme before React hydrates and switches to the user's theme. // -// `data-color-mode` is always concrete, never `auto` — @primer/react-brand has -// no `auto` palette — and follows the effective theme, because a `light` mode -// can carry a dark day theme. See src/color-schemes/README.md. +// data-color-mode stays concrete, never auto, and follows the effective theme because +// @primer/react-brand lacks an auto palette and light mode can carry a dark day theme. +// See src/color-schemes/README.md. // -// The output is identical for every request, so the HTML stays shared-cacheable -// in our CDN. The validation allowlists and defaults are derived from the same -// enums used by `useTheme`, so they can't drift, and `helmet.ts` hashes this -// exact string for the CSP `script-src` allowance (no nonce, no unsafe-inline). +// The generated output is identical across requests, so CDN caches can share the HTML. +// useTheme supplies the validation allowlists and defaults so they cannot drift. +// helmet.ts hashes this exact string for the CSP script-src allowance, with no nonce +// and no unsafe-inline. const modes = JSON.stringify(Object.values(CssColorMode)) const themes = JSON.stringify(Object.values(SupportedTheme)) const defaults = JSON.stringify(defaultCSSTheme) diff --git a/src/color-schemes/lib/get-brand-color-mode.ts b/src/color-schemes/lib/get-brand-color-mode.ts index cd317a6e9766..9bacbe6864c5 100644 --- a/src/color-schemes/lib/get-brand-color-mode.ts +++ b/src/color-schemes/lib/get-brand-color-mode.ts @@ -1,7 +1,7 @@ export type BrandColorMode = 'light' | 'dark' -// Brand's palette follows 's `data-color-mode`, resolved to a concrete mode -// before first paint — not PRC's `resolvedColorScheme`, which is the THEME. +// Brand's palette follows html data-color-mode, resolved to a concrete mode before first +// paint, not Primer React's resolvedColorScheme, which tracks the theme. export function getBrandColorMode(): BrandColorMode { if (typeof document === 'undefined') return 'light' // SSR fallback return document.documentElement.getAttribute('data-color-mode') === 'dark' ? 'dark' : 'light' diff --git a/src/color-schemes/tests/color-mode-script.ts b/src/color-schemes/tests/color-mode-script.ts index ad6ce2fcb158..7ac8cb9c1f53 100644 --- a/src/color-schemes/tests/color-mode-script.ts +++ b/src/color-schemes/tests/color-mode-script.ts @@ -3,17 +3,15 @@ import { describe, expect, test } from 'vitest' import { colorModeScript } from '../lib/color-mode-script' import { getCssTheme, SupportedTheme } from '../components/useTheme' -// The inline script runs before any bundle loads, so it reimplements -// `useTheme`'s validation instead of importing it. These tests assert the two -// stay in sync. +// The inline script runs before any bundle loads, so it reimplements useTheme validation +// instead of importing it. These tests assert the two stay in sync. function runScript( rawCookie: string, { prefersDark = false, matchMedia = true, legacyListener = false } = {}, ) { const attrs: Record = {} const listeners: Array<(event: { matches: boolean }) => void> = [] - // `matches` reads this through a getter, so `flipSystemPreference` changes - // what an already-registered handler sees. + // matches reads os through a getter, so flipSystemPreference changes what handlers see. const os = { prefersDark } const subscribe = (handler: (event: { matches: boolean }) => void) => { listeners.push(handler) @@ -61,8 +59,7 @@ function cookieFor(value: object) { function expectMatchesGetCssTheme(rawCookie: string, cookieValue: string, prefersDark = false) { const css = getCssTheme(cookieValue) - // Primitives select on the (mode, theme) pair, so the effective theme has to - // land on the attribute for the resolved mode. + // Primer primitives use mode and theme together, so resolved mode gets the effective theme. const mode = css.colorMode === 'auto' ? (prefersDark ? 'dark' : 'light') : css.colorMode const theme = mode === 'dark' ? css.darkTheme : css.lightTheme const resolved = theme.startsWith('dark') ? 'dark' : 'light' @@ -111,7 +108,7 @@ describe('colorModeScript', () => { }) test('survives an explicitly null theme without discarding the mode', () => { - // A default parameter covers `undefined`, not `null`. + // A default parameter covers undefined, not null. const value = { color_mode: 'dark', light_theme: null } expectMatchesGetCssTheme(cookieFor(value), JSON.stringify(value)) expect(runScript(cookieFor(value)).attrs['data-color-mode']).toBe('dark') @@ -159,7 +156,7 @@ describe('colorModeScript', () => { }) test('falls back to the deprecated addListener when addEventListener is absent', () => { - // Pre-14 Safari exposes only `addListener`, so this branch is live. + // Older Safari exposes only addListener, so this branch is live. const run = runScript(cookieFor({ color_mode: 'auto' }), { legacyListener: true }) expect(run.attrs['data-color-mode']).toBe('light') expect(run.listeners).toHaveLength(1) @@ -172,8 +169,7 @@ describe('colorModeScript', () => { }) test('still writes the attributes when matchMedia is unavailable', () => { - // The script's DOM block sits in a try/catch, so an unguarded matchMedia - // call would leave with no attributes at all. + // Guard matchMedia so the DOM try/catch still writes html attributes when it is unavailable. const { attrs } = runScript(cookieFor({ color_mode: 'auto' }), { matchMedia: false }) expect(attrs['data-color-mode']).toBe('light') expect(attrs['data-color-mode-preference']).toBe('auto') @@ -211,8 +207,7 @@ describe('colorModeScript', () => { dark_theme: { name: 'dark_high_contrast', color_mode: 'dark' }, }), ) - // Resolved dark by the DAY theme, so data-dark-theme carries that, not the - // separately configured night theme. + // The resolved day theme supplies data-dark-theme, not the separately configured night theme. expect(attrs['data-color-mode']).toBe('dark') expect(attrs['data-dark-theme']).toBe('dark_dimmed') }) @@ -237,10 +232,9 @@ describe('colorModeScript', () => { expect(runScript(value, { prefersDark: true }).attrs['data-color-mode']).toBe('dark') }) + // html classifies dark themes with startsWith("dark"); Primer React checks includes("dark"). + // A theme name like high_contrast_dark would split those classifications. test('every supported theme classifies the same under both operators', () => { - // must classify a theme's lightness the same way @primer/react does - // for its own wrapper: `startsWith('dark')` here, `includes('dark')` there. - // A name like `high_contrast_dark` would split them. for (const name of Object.values(SupportedTheme)) { expect(`${name} startsWith:${name.startsWith('dark')}`).toBe( `${name} startsWith:${name.includes('dark')}`, diff --git a/src/color-schemes/tests/get-brand-color-mode.ts b/src/color-schemes/tests/get-brand-color-mode.ts index 1653ffcacc9c..13a57486370d 100644 --- a/src/color-schemes/tests/get-brand-color-mode.ts +++ b/src/color-schemes/tests/get-brand-color-mode.ts @@ -21,7 +21,7 @@ describe('getBrandColorMode', () => { }) test.each([ - // Brand has no `auto` mode, so anything not `dark` has to render light. + // Brand has no auto mode, so anything not dark renders light. ['auto', 'light'], ['nonsense', 'light'], [null, 'light'], diff --git a/src/content-pipelines/config.yml b/src/content-pipelines/config.yml index e2b9719e25a3..7a364ca70798 100644 --- a/src/content-pipelines/config.yml +++ b/src/content-pipelines/config.yml @@ -1,19 +1,17 @@ -# Content pipelines configuration +# Content pipelines sync source docs into allowed target articles with the +# content-pipeline-update agent. # -# Each entry defines a content pipeline that syncs docs from an external -# repository and uses the content-pipeline-update agent to update content articles. +# Run a pipeline manually with: +# npx tsx src/content-pipelines/scripts/update.ts --id copilot-cli # -# The update.ts script reads this file so you can run: -# $ npx tsx src/content-pipelines/scripts/update.ts --id copilot-cli -# -# The workflow matrix in .github/workflows/content-pipelines.yml only needs `id`; -# everything else is read from this file. -# -# `exclusions` lists source topics the agent should skip during gap analysis. -# Use an empty list ([]) when nothing should be excluded. Example: -# exclusions: -# - Internal debugging commands -# - Experimental telemetry flags +# The workflow matrix entry needs only id. +# Later workflow steps and update.ts read the other fields from this file. +# exclusions lists source topics the agent skips during gap analysis. +# Use [] when none. +# Example: +# exclusions: +# - Internal debugging commands +# - Experimental telemetry flags # copilot-cli: name: Copilot CLI @@ -55,13 +53,3 @@ gh-stack: The source uses "sh" code fences and title-case headings; use "shell" and sentence case instead. Write "pull request" rather than "PR". Do not remove the public preview reusable near the top of the article; it has no counterpart in the source docs. - -# TODO -# mcp-server: -# name: GitHub MCP Server -# source-repo: github/github-mcp-server -# source-path: docs -# # TBD — update this list as articles are created -# target-articles: [] -# exclusions: [] -# content-mapping: "" diff --git a/src/content-pipelines/scripts/update.ts b/src/content-pipelines/scripts/update.ts index f8b0f67ccc71..3fbb0fddd777 100644 --- a/src/content-pipelines/scripts/update.ts +++ b/src/content-pipelines/scripts/update.ts @@ -1,20 +1,15 @@ -// [start-readme] +// Clones an external source repository, detects changed docs, and runs the +// content-pipeline-update Copilot agent to update reference articles. // -// This script clones an external source repository, detects whether its docs -// have changed since the last processed commit, and if so runs the -// content-pipeline-update Copilot agent to update our reference articles. -// -// The workflow (.github/workflows/content-pipelines.yml) calls this script in CI. -// You can also run it locally for testing and iteration: +// .github/workflows/content-pipelines.yml calls this script in CI. +// Run it locally with: // // npx tsx src/content-pipelines/scripts/update.ts --id copilot-cli // npx tsx src/content-pipelines/scripts/update.ts --id copilot-cli --dry-run // npx tsx src/content-pipelines/scripts/update.ts --id copilot-cli --full-scan // -// Defaults (source-repo, source-path, target-articles) are read from -// src/content-pipelines/config.yml. You can override any value via CLI flags. -// -// [end-readme] +// src/content-pipelines/config.yml supplies source-repo, source-path, and +// target-articles defaults. CLI flags override them. import { execSync, execFileSync } from 'child_process' import fs from 'fs' @@ -149,8 +144,7 @@ async function main(): Promise { const repoUrl = `https://github.com/${SOURCE_REPO}.git` try { - // execFileSync passes the token as an argument instead of embedding it in the URL, - // where it would leak into error messages and logs. + // Use http.extraHeader for token, not clone URL; Git includes clone URLs in errors and logs. const args = ['clone'] if (token) { args.push( @@ -200,9 +194,7 @@ async function main(): Promise { diff = '(diff unavailable)' } - // Empty means no doc files changed. - // A leading "(" means the diff itself failed, - // so fall through and run the agent anyway. + // Empty output means no doc files changed; a leading "(" means diff failed, so run the agent. if (!nameStatus.startsWith('(') && !nameStatus.trim()) { console.log( `No changes in ${SOURCE_PATH} between ${storedSha.slice(0, 7)} and ${currentSha.slice(0, 7)}. Skipping agent run.`, @@ -222,8 +214,7 @@ async function main(): Promise { diff, ].join('\n') } else { - // Initial run or full scan, so list every source doc. - // Incremental runs get this inventory from git diff --name-status instead. + // Initial and full scans list all docs; incremental scans use git diff --name-status. const sourceDocs = path.join(sourceDir, SOURCE_PATH) let fileList: string try { diff --git a/src/content-pipelines/state/.gitignore b/src/content-pipelines/state/.gitignore index 425c0aa18d88..2e5cc2dc37a4 100644 --- a/src/content-pipelines/state/.gitignore +++ b/src/content-pipelines/state/.gitignore @@ -1,4 +1,4 @@ # This directory stores the last-processed commit SHA for each content pipeline. -# SHA files are created and updated by the content-pipelines workflow. -# Diff files (.diff) are ephemeral and should not be committed. +# The content-pipelines workflow creates and updates SHA files. +# Diff files are ephemeral and must not be committed. *.diff diff --git a/src/content-render/index.ts b/src/content-render/index.ts index 2333de8a5084..376b8f8d980a 100644 --- a/src/content-render/index.ts +++ b/src/content-render/index.ts @@ -15,14 +15,12 @@ interface RenderOptions { const globalCache = new Map() -// parse multiple times because some templates contain more templates. :] export async function renderContent( template = '', context: Context = {} as Context, options: RenderOptions = {}, ): Promise { - // If called with a falsy template, it can't ever become something - // when rendered. We can exit early to save some pointless work. + // Falsy templates cannot render into content, so skip Liquid and unified work. if (!template) return template let cacheKey: string | null = null if (options && options.cache) { @@ -42,8 +40,7 @@ export async function renderContent( try { template = await renderLiquid(template, context) if (context.markdownRequested) { - // Skip the remark pipeline when there are no internal links to rewrite, - // since link rewriting is the only transformation the pipeline performs. + // Skip remark without internal links; link rewriting is the only markdownRequested transformation. if (!/\]\(\s*'); mask-size: cover; - // Brand has no `subtle` text step; `muted` is the closest analogue to - // Primer's --color-fg-subtle (#6e7781 -> #58635b). + // Brand has no subtle text step; muted is closest to Primer --color-fg-subtle. + // Primer #6e7781 maps to Brand #58635b. background-color: var(--brand-color-text-muted, #58635b); @media (forced-colors: active) { background-color: LinkText; diff --git a/src/content-render/stylesheets/markdown-overrides.scss b/src/content-render/stylesheets/markdown-overrides.scss index 0c4af7f5909f..7af2a92e180b 100644 --- a/src/content-render/stylesheets/markdown-overrides.scss +++ b/src/content-render/stylesheets/markdown-overrides.scss @@ -1,21 +1,7 @@ -// What might happens is that we have a DOM of -// -//
-//
Note
-//

Heading

-// ... -// -// When this is the case, by default, that first
that is the first -// gets the `margin-top: 0 !important` and not the first

. -// Generally, the reason this even exists is because

(and

) elements -// are given extra margin-top so as to divide the article into sections -// with some extra whitespace. That's fine, but we don't to start the -// top of the page with too much whitespace. That's why @primer/css -// has a solution for that. Just the problem that it fails then first -// element isn't actually a heading. -// Note we're also doing it for a possible

being the first element. +// Primer's markdown-body first-child reset can hit a hidden first child instead +// of the first h2 or h3. Those headings carry section spacing, but the page top +// must not start with it, so reset the first h2 or h3 directly. // See https://github.com/primer/css/issues/2303 -// See internal issue #2368 .markdown-body { > h2:first-of-type, > h3:first-of-type { @@ -23,9 +9,8 @@ } } -// Horizontal scroll gets flagged as an accessibility violation. -// Updates all code examples to only allow vertical scroll, and -// break aggressively. +// Horizontal scroll gets flagged as an accessibility violation, so code examples wrap +// aggressively and allow only vertical scrolling. .markdown-body { pre { overflow-x: hidden; @@ -38,97 +23,71 @@ } } -// Fix for permissions icon collision with bulleted lists -// When permissions/product statements contain lists that start immediately, -// the list bullets can visually collide with the icons in the flex layout. -// This adds proper spacing to prevent the collision while supporting RTL languages -// and avoiding effects on nested lists. -// See: https://github.com/github/docs-engineering/issues/5199 +// Lists that start immediately in permissions and product statements can collide with +// the icon in the flex layout. Inline spacing preserves right-to-left layouts and avoids +// changing nested lists. .permissions-statement, .product-statement { ul { margin-inline-start: 0; - padding-inline-start: 1rem; // Ensure proper spacing from icon (RTL-aware) + padding-inline-start: 1rem; } ul > li { - margin-inline-start: 0.5rem; // Additional spacing to prevent bullet collision (direct children only) + margin-inline-start: 0.5rem; } } -// A CTA button written on its own line in markdown — `` — becomes its own

, and that paragraph already carries the 16px -// rhythm margin. The `mt-3` utility then stacks a second 16px inside it, so the -// button ends up 32px below the preceding line but only 16px above the next one. -// Drop the utility when the button is alone in its paragraph and let the -// paragraph margin do the spacing, which puts the CTA on the same rhythm as -// every other block. `!important` is required because Primer's spacing -// utilities are themselves !important. -// -// `:only-child` is doing real work here — it is what keeps the two cases apart: -// - CTA callouts (`product:`/`permissions:` frontmatter) put the button after -// a
INSIDE the prose paragraph, so there is no paragraph margin above -// it and `mt-3` is the only thing separating it from the text. -// - The side-by-side Yes/No `.btn-outline` pairs are two buttons in one -// paragraph. -// Neither is an only child, so both keep their margin. +// A CTA button written alone in markdown, such as
, +// becomes its own p. The p already has a 16px rhythm margin, and mt-3 adds another +// 16px, leaving 32px below the preceding line but 16px above the next one. +// Drop mt-3 only when the button is alone in its p; Primer spacing utilities use +// !important too. +// :only-child keeps CTA callouts and side-by-side Yes/No buttons unchanged. Frontmatter +// product: and permissions: put the CTA after a br inside the prose p, so mt-3 supplies +// its only top spacing. Yes/No .btn-outline pairs have two buttons in one p. .markdown-body p > a.btn:only-child { margin-top: 0 !important; } -// @primer/css holds `.btn` at `white-space: nowrap`, which a button cannot -// honour and still stay inside a narrow column. The longest CTA label — "Set up -// a trial of GitHub Enterprise Cloud", 322px — is wider than the article column -// below a ~420px viewport and wider than the callout's text column below ~390px, -// so the button ran past the content edge and was clipped. +// @primer/css sets .btn to white-space: nowrap, which makes long CTA labels overflow +// narrow columns. The longest CTA label, "Set up a trial of GitHub Enterprise Cloud", +// measures 322px, wider than the article column below about 420px and the callout text +// column below about 390px, so it gets clipped. // -// Letting the label wrap fixes it with no breakpoint to guess at. An -// inline-block is shrink-to-fit — min(max-content, available) — so -// `white-space: normal` changes nothing until max-content exceeds the space -// available: at every width where the button already fits it still renders on -// one line, byte-identical. That also makes it self-correcting for longer -// translated labels and for the narrower column a callout gives the same button. +// Let labels wrap instead of guessing a breakpoint. inline-block shrink-to-fit, +// min(max-content, available), means white-space: normal changes nothing until +// max-content exceeds the available space. Buttons that fit still render on one line, +// and longer translations or narrower callout columns self-correct. .markdown-body a.btn, .permissions-statement a.btn, .product-statement a.btn { white-space: normal; - // Wrapping alone orphaned the trailing octicon on a line of its own: the - // space between the label and the icon is a valid break point, and the - // label filled the first line exactly. Laying the button out as a flex row - // instead lets the label wrap within itself and keeps the icon beside it, - // vertically centred. At widths where nothing wraps the result is within a - // pixel of the inline-block it replaces: same 17px left inset, same 21px - // right inset, same 32px height, still one line. The `gap` below covers the - // one thing that does change. + // Wrapping alone can orphan the trailing octicon because the label/icon space can break. + // inline-flex lets the label wrap inside itself and keeps the icon beside it, centered. + // When nothing wraps, this stays within a pixel of inline-block: 17px left inset, + // 21px right inset, 32px height, and one line. The gap below covers the one change. display: inline-flex; align-items: center; - // Flex layout eats the one thing that was separating the label from the icon. - // The markup is `Label {% octicon "link-external" %}`, and that - // literal space does survive Liquid and the markdown pipeline as a real text - // node — but a whitespace-only text node between two flex items is not itself - // a flex item, so no box is generated for it and the label ends up touching - // the icon. `gap` puts the space back. + // Flex removes the literal space between the label and icon. In + // Label {% octicon "link-external" %}, Liquid and markdown preserve the + // space as a text node, but a whitespace-only text node between flex items creates no + // box, so the label touches the icon. // - // 4px rather than the measured width of that space glyph, because a space is - // font- and locale-dependent — it measures differently on two machines here — - // while 4px is the value Primer itself already uses between a button's icon - // and its label. The button ends up a fraction of a pixel wider than it was - // rather than most of a space narrower, on a number the design system owns. + // Use 4px instead of the measured space width because the space changes by font and + // locale. Primer already uses 4px between a button icon and label, so this + // design-system value makes the button slightly wider rather than most of a space narrower. // - // Only the label/icon gap is restored. Primer's `.btn .octicon` also carries - // `margin-right: 4px`, which assumes a LEADING icon and so lands outside the - // trailing icon on these CTAs, giving them 21px of inset on the right against - // 17px on the left. That asymmetry is what ships today, so it stays — zeroing - // it would restyle every CTA on the site, which is a different change from - // keeping a long label inside its column. + // Restore only the label/icon gap. Primer .btn .octicon also has margin-right: 4px + // for leading icons, which lands outside these trailing CTA icons and gives 21px right + // inset against 17px left. Keep that asymmetry; zeroing it would restyle every CTA. gap: 4px; - // The octicon is a flex item now, and flex items shrink before their container - // overflows. Once the label wraps, the icon is the only thing left to give, so - // the 16px glyph was rendering at 11px in a 240px callout column. It is a - // fixed-size icon; the label is what should absorb a narrow column. + // The octicon is a flex item, and flex items shrink before their container overflows. + // Once the label wraps, the icon is the only thing left to shrink, so the 16px glyph + // rendered at 11px in a 240px callout column. Keep the icon fixed and let the label absorb width. .octicon { flex-shrink: 0; } diff --git a/src/content-render/stylesheets/octicon-table-optimization.scss b/src/content-render/stylesheets/octicon-table-optimization.scss index 23d1f9af5728..777b9d30f538 100644 --- a/src/content-render/stylesheets/octicon-table-optimization.scss +++ b/src/content-render/stylesheets/octicon-table-optimization.scss @@ -1,5 +1,5 @@ -// Octicon table optimization for pages with hundreds of repeated icons -// Uses CSS background images instead of inline SVGs to dramatically reduce HTML size +// Pages with hundreds of repeated octicons use CSS background images instead of inline SVGs +// to reduce HTML size. $octicon-check-path: "M13.78 4.22a.75.75 0 0 1 0 1.06l-7.25 7.25a.75.75 0 0 1-1.06 0L2.22 9.28a.751.751 0 0 1 .018-1.042.751.751 0 0 1 1.042-.018L6 10.94l6.72-6.72a.75.75 0 0 1 1.06 0Z"; $octicon-x-path: "M3.72 3.72a.75.75 0 0 1 1.06 0L8 6.94l3.22-3.22a.749.749 0 0 1 1.275.326.749.749 0 0 1-.215.734L9.06 8l3.22 3.22a.749.749 0 0 1-.326 1.275.749.749 0 0 1-.734-.215L8 9.06l-3.22 3.22a.751.751 0 0 1-1.042-.018.751.751 0 0 1-.018-1.042L6.94 8 3.72 4.78a.75.75 0 0 1 0-1.06Z"; diff --git a/src/content-render/stylesheets/syntax-highlighting.scss b/src/content-render/stylesheets/syntax-highlighting.scss index 30b9b9efa5ae..e7b679732af0 100644 --- a/src/content-render/stylesheets/syntax-highlighting.scss +++ b/src/content-render/stylesheets/syntax-highlighting.scss @@ -8,10 +8,9 @@ from https://unpkg.com/highlight.js@9.15.8/styles/github.css .hljs { display: block; padding: 0.5em; - // The block's BASE text colour — the tokens below are prettylights, which has - // no Brand equivalent and stays on Primer deliberately, but this one is just - // "default text" and was painting Primer's #e6edf3 inside a Brand-framed code - // block. The `background` is inert here (markdown-overrides paints the `pre`). + // Use Brand's default text color for the block itself. The syntax tokens below stay + // on Primer prettylights because Brand has no equivalent. Inside .markdown-body, Primer + // paints the pre and makes pre code transparent, so this background is inert there. color: var(--brand-color-text-default); background: var(--color-canvas-subtle); } diff --git a/src/data-directory/lib/data-directory.ts b/src/data-directory/lib/data-directory.ts index 05e7049d93a6..56da0f47a565 100644 --- a/src/data-directory/lib/data-directory.ts +++ b/src/data-directory/lib/data-directory.ts @@ -17,6 +17,9 @@ interface DataDirectoryResult { [key: string]: unknown } +// dataDirectory uses setWith because lodash set creates arrays for numeric release-note paths. +// Example: release-notes.enterprise-server.2-20.0 must stay an object path. +// See https://lodash.com/docs#set. export default function dataDirectory( dir: string, opts: DataDirectoryOptions = {}, @@ -38,7 +41,6 @@ export default function dataDirectory( const data: DataDirectoryResult = {} - // find YAML and Markdown files in the given directory, recursively const filenames = walk(dir, { includeBasePath: true }).filter((filename: string) => { if (mergedOpts.ignorePatterns.some((pattern) => pattern.test(filename))) return false @@ -51,7 +53,6 @@ export default function dataDirectory( ]) for (const [filename, fileContent] of files) { - // derive `foo.bar.baz` object key from `foo/bar/baz.yml` filename const key = filenameToKey(path.relative(dir, filename)) const extension = path.extname(filename).toLowerCase() @@ -60,11 +61,6 @@ export default function dataDirectory( processedContent = mergedOpts.preprocess(fileContent) } - // Add this file's data to the global data object. - // Note we want to use `setWith` instead of `set` so we can customize the type during path creation. - // If we just use `set`, then e.g. `release-notes.enterprise-server.2-20.0` will be an Array but - // `release-notes.enterprise-server.3-0.0` will be an Object. - // See https://lodash.com/docs#set for an explanation. switch (extension) { case '.json': setWith(data, key, JSON.parse(processedContent), Object) @@ -74,9 +70,7 @@ export default function dataDirectory( break case '.md': case '.markdown': - // Use `matter` to drop frontmatter, since localized reusable Markdown files - // can potentially have frontmatter, but we want to prevent the frontmatter - // from being rendered. + // Localized reusable Markdown can have frontmatter; strip it so content rendering hides it. setWith(data, key, matter(processedContent).content, Object) break } diff --git a/src/data-directory/lib/data-schemas/ctas.ts b/src/data-directory/lib/data-schemas/ctas.ts index 2f97602bed03..31c2c0238c80 100644 --- a/src/data-directory/lib/data-schemas/ctas.ts +++ b/src/data-directory/lib/data-schemas/ctas.ts @@ -1,13 +1,9 @@ -// This schema enforces the structure for CTA (Call-to-Action) URL parameters -// Used to validate CTA tracking parameters in documentation links - export default { type: 'object', additionalProperties: false, required: ['ref_product', 'ref_type', 'ref_style'], properties: { - // GitHub Product: The GitHub product the CTA leads users to - // Format: ref_product=copilot + // Example query parameter: ref_product=copilot. ref_product: { type: 'string', name: 'Product', @@ -26,8 +22,7 @@ export default { ], }, - // Type of CTA: The type of action the CTA encourages users to take - // Format: ref_type=trial + // Example query parameter: ref_type=trial. ref_type: { type: 'string', name: 'Type', @@ -35,8 +30,7 @@ export default { enum: ['trial', 'purchase', 'engagement'], }, - // CTA style: The way we are formatting the CTA in the docs - // Format: ref_style=button + // Example query parameter: ref_style=button. ref_style: { type: 'string', name: 'Style', @@ -44,8 +38,7 @@ export default { enum: ['button', 'text'], }, - // Type of plan (Optional): For links to sign up for or trial a plan, the specific plan we link to - // Format: ref_plan=business + // Example query parameter: ref_plan=business. ref_plan: { type: 'string', name: 'Plan', diff --git a/src/data-directory/lib/data-schemas/features.ts b/src/data-directory/lib/data-schemas/features.ts index 1b9aa310350b..de81e35ff109 100644 --- a/src/data-directory/lib/data-schemas/features.ts +++ b/src/data-directory/lib/data-schemas/features.ts @@ -15,7 +15,6 @@ interface FeatureVersionsSchema { additionalProperties: false } -// Copy the properties from the frontmatter schema. const featureVersions: FeatureVersionsSchema = { type: 'object', properties: { @@ -24,8 +23,7 @@ const featureVersions: FeatureVersionsSchema = { additionalProperties: false, } -// Remove the feature versions properties. -// We don't want to allow features within features! We just want pure versioning. +// Each data/features file allows version gates but not nested feature gates. delete (featureVersions.properties.versions.properties as Record | undefined) ?.feature diff --git a/src/data-directory/lib/data-schemas/glossaries-candidates.ts b/src/data-directory/lib/data-schemas/glossaries-candidates.ts index cfe6393c32a6..ae86460d98ac 100644 --- a/src/data-directory/lib/data-schemas/glossaries-candidates.ts +++ b/src/data-directory/lib/data-schemas/glossaries-candidates.ts @@ -7,7 +7,8 @@ export interface TermSchema { export const term: TermSchema = { type: 'string', minLength: 1, - pattern: '^((?!\\*).)*$', // no asterisks allowed + // Reject asterisks in glossary terms. + pattern: '^((?!\\*).)*$', } export interface GlossaryCandidateItem { diff --git a/src/data-directory/lib/data-schemas/index.ts b/src/data-directory/lib/data-schemas/index.ts index bd157c2afb63..9a082301823a 100644 --- a/src/data-directory/lib/data-schemas/index.ts +++ b/src/data-directory/lib/data-schemas/index.ts @@ -12,16 +12,14 @@ function resolveSchemaPath(filename: string): string { const isTest = process.env.NODE_ENV === 'test' if (isTest) { - // Use relative paths that work for vitest and 4.x compatibility with - // dynamic imports in particular + // Vitest dynamic imports need relative schema paths. return `../lib/data-schemas/${filename}` } else { - // Use absolute paths that work for content linter and other contexts + // Content linter and other runtime contexts need absolute schema paths. return `@/data-directory/lib/data-schemas/${filename}` } } -// Auto-discover table schemas from data/tables/ directory function loadTableSchemas(): DataSchemas { const tablesDir = path.join(process.cwd(), 'data/tables') const schemasDir = path.join(__dirname, 'tables') @@ -43,7 +41,6 @@ function loadTableSchemas(): DataSchemas { return tableSchemas } -// Manual schema registrations for non-table data const manualSchemas: DataSchemas = { 'data/features': resolveSchemaPath('features.ts'), 'data/variables': resolveSchemaPath('variables.ts'), @@ -51,9 +48,8 @@ const manualSchemas: DataSchemas = { 'data/code-languages.yml': resolveSchemaPath('code-languages.ts'), 'data/glossaries/candidates.yml': resolveSchemaPath('glossaries-candidates.ts'), 'data/glossaries/external.yml': resolveSchemaPath('glossaries-external.ts'), - // Tables in subdirectories of data/tables are not picked up by loadTableSchemas(), - // which only reads the top level, so the matrix is registered explicitly here. - // The matrix/ entry is a directory schema: every per-IDE file is validated against it. + // Register the matrix directory schema because loadTableSchemas reads only top-level files. + // The directory schema validates every per-IDE file. 'data/tables/copilot/matrix': resolveSchemaPath('tables/copilot/matrix-ide.ts'), 'data/tables/copilot/matrix-meta.yml': resolveSchemaPath('tables/copilot/matrix-meta.ts'), } diff --git a/src/data-directory/lib/data-schemas/tables/copilot/auto-model-selection.ts b/src/data-directory/lib/data-schemas/tables/copilot/auto-model-selection.ts index 9b6ff927dd95..63b81d4f8cec 100644 --- a/src/data-directory/lib/data-schemas/tables/copilot/auto-model-selection.ts +++ b/src/data-directory/lib/data-schemas/tables/copilot/auto-model-selection.ts @@ -1,5 +1,3 @@ -// This schema enforces the structure in auto-model-selection.yml - const autoModelSelectionSchema = { type: 'array', items: { diff --git a/src/data-directory/lib/data-schemas/tables/copilot/matrix-ide.ts b/src/data-directory/lib/data-schemas/tables/copilot/matrix-ide.ts index e91ddcc37c4c..e2b85f6cdc9b 100644 --- a/src/data-directory/lib/data-schemas/tables/copilot/matrix-ide.ts +++ b/src/data-directory/lib/data-schemas/tables/copilot/matrix-ide.ts @@ -1,28 +1,15 @@ -// Schema for the per-IDE files in data/tables/copilot/matrix/ -// -// Registered as a directory schema in src/data-directory/lib/data-schemas/index.ts, -// so every file added to that directory is validated against this shape. +// The directory schema registration validates every data/tables/copilot/matrix/.yml file. -// Deliberately not an enum. The vocabulary is defined once, as data, in -// matrix-meta.yml, and is enforced against every IDE file by the -// 'every support level used is defined in matrix-meta' invariant in -// src/data-directory/tests/copilot-matrix.ts. Repeating the values here would -// be a fourth copy that can drift from the data — which is exactly what the -// schema this file replaces did: it was missing 'closing-down'. +// supportLevel stays open because matrix-meta.yml owns the vocabulary and tests enforce it. +// Repeating values here would create a fourth copy that can drift from data. const supportLevel = { type: 'string', } -// Every version tracked here is 3-part, and that follows from what is tracked -// rather than from convention: four of the six files track the Copilot -// extension (marketplace versions are required to be x.y.z) and the two that -// track the IDE itself, VS Code and Visual Studio, version that way natively. -// Kept strict on purpose. It catches a dropped or added segment — the mistake -// an updater reading release notes is most likely to make, and one the -// cross-file invariants cannot see, since they only check that a version is -// used consistently, not that it is real. If an IDE genuinely changes -// versioning scheme, that is a deliberate decision: change this pattern and say -// why in the PR. +// All six matrix files use three-part versions: four track Copilot extension marketplace versions, +// and VS Code and Visual Studio use three-part IDE versions natively. +// Keep the pattern strict because cross-file tests catch consistency, not malformed versions. +// Update this pattern if an IDE adopts a different version format. const VERSION_PATTERN = '^\\d+\\.\\d+\\.\\d+$' const copilotMatrixIdeSchema = { diff --git a/src/data-directory/lib/data-schemas/tables/copilot/matrix-meta.ts b/src/data-directory/lib/data-schemas/tables/copilot/matrix-meta.ts index 8853dc696125..873642ba58e8 100644 --- a/src/data-directory/lib/data-schemas/tables/copilot/matrix-meta.ts +++ b/src/data-directory/lib/data-schemas/tables/copilot/matrix-meta.ts @@ -1,7 +1,5 @@ -// Schema for data/tables/copilot/matrix-meta.yml -// -// Shared configuration for the Copilot IDE feature matrix. Per-IDE data lives in -// data/tables/copilot/matrix/.yml and is validated by matrix-ide.ts. +// matrix-meta.yml owns shared Copilot IDE matrix configuration. +// Per-IDE data lives in data/tables/copilot/matrix/.yml and matrix-ide.ts validates it. const copilotMatrixMetaSchema = { type: 'object', diff --git a/src/data-directory/lib/data-schemas/tables/copilot/model-comparison.ts b/src/data-directory/lib/data-schemas/tables/copilot/model-comparison.ts index 022eb8da25aa..6189c00e9ff6 100644 --- a/src/data-directory/lib/data-schemas/tables/copilot/model-comparison.ts +++ b/src/data-directory/lib/data-schemas/tables/copilot/model-comparison.ts @@ -1,5 +1,3 @@ -// This schema enforces the structure in model-comparison.yml - const modelComparisonSchema = { type: 'object', additionalProperties: false, diff --git a/src/data-directory/lib/data-schemas/tables/copilot/model-deprecation-history.ts b/src/data-directory/lib/data-schemas/tables/copilot/model-deprecation-history.ts index ba31dc99efb6..84fc98abac18 100644 --- a/src/data-directory/lib/data-schemas/tables/copilot/model-deprecation-history.ts +++ b/src/data-directory/lib/data-schemas/tables/copilot/model-deprecation-history.ts @@ -1,5 +1,3 @@ -// This schema enforces the structure in model-deprecation-history.yml - const modelDeprecationHistorySchema = { type: 'object', additionalProperties: false, diff --git a/src/data-directory/lib/data-schemas/tables/copilot/model-release-status.ts b/src/data-directory/lib/data-schemas/tables/copilot/model-release-status.ts index a00352c7735a..6918f92a8903 100644 --- a/src/data-directory/lib/data-schemas/tables/copilot/model-release-status.ts +++ b/src/data-directory/lib/data-schemas/tables/copilot/model-release-status.ts @@ -1,5 +1,3 @@ -// This schema enforces the structure in model-release-status.yml - const modelsReleaseStatusSchema = { type: 'object', additionalProperties: false, diff --git a/src/data-directory/lib/data-schemas/tables/copilot/model-supported-clients.ts b/src/data-directory/lib/data-schemas/tables/copilot/model-supported-clients.ts index ffb28af36dc2..84475d8305aa 100644 --- a/src/data-directory/lib/data-schemas/tables/copilot/model-supported-clients.ts +++ b/src/data-directory/lib/data-schemas/tables/copilot/model-supported-clients.ts @@ -1,5 +1,3 @@ -// This schema enforces the structure in model-supported-clients.yml - const modelsSupportedClientsSchema = { type: 'object', additionalProperties: false, diff --git a/src/data-directory/lib/data-schemas/tables/copilot/model-supported-plans.ts b/src/data-directory/lib/data-schemas/tables/copilot/model-supported-plans.ts index 401b46254091..1476e55a4774 100644 --- a/src/data-directory/lib/data-schemas/tables/copilot/model-supported-plans.ts +++ b/src/data-directory/lib/data-schemas/tables/copilot/model-supported-plans.ts @@ -1,5 +1,3 @@ -// This schema enforces the structure in model-supported-plans.yml - const modelSupportedPlansSchema = { type: 'object', additionalProperties: false, diff --git a/src/data-directory/lib/data-schemas/tables/copilot/models-and-pricing.ts b/src/data-directory/lib/data-schemas/tables/copilot/models-and-pricing.ts index 96f8127cd22e..992153a91100 100644 --- a/src/data-directory/lib/data-schemas/tables/copilot/models-and-pricing.ts +++ b/src/data-directory/lib/data-schemas/tables/copilot/models-and-pricing.ts @@ -1,5 +1,3 @@ -// This schema enforces the structure in models-and-pricing.yml - const modelsAndPricingSchema = { type: 'object', additionalProperties: false, diff --git a/src/data-directory/lib/data-schemas/tables/repository-roles.ts b/src/data-directory/lib/data-schemas/tables/repository-roles.ts index 6774b9739d4b..ab0a2af84e5c 100644 --- a/src/data-directory/lib/data-schemas/tables/repository-roles.ts +++ b/src/data-directory/lib/data-schemas/tables/repository-roles.ts @@ -1,5 +1,3 @@ -// This schema enforces the structure in data/tables/repository-roles.yml - const row = { type: 'object', additionalProperties: false, @@ -9,13 +7,11 @@ const row = { type: 'string', lintable: true, }, - // Liquid that renders non-empty when the row should be shown. When omitted, - // the row is shown on every version. + // Non-empty Liquid output limits the row to matching versions; omitting it renders everywhere. versions: { type: 'string', }, - // Comma separated list of the roles that can perform the action. Roles left - // out render as no. May contain Liquid, so a single role can be conditional. + // Comma-separated roles can contain Liquid; omitted roles render as no. roles: { type: 'string', }, diff --git a/src/data-directory/lib/data-schemas/tables/rest-api-versions.ts b/src/data-directory/lib/data-schemas/tables/rest-api-versions.ts index 046c02afe403..b0e5aef2741d 100644 --- a/src/data-directory/lib/data-schemas/tables/rest-api-versions.ts +++ b/src/data-directory/lib/data-schemas/tables/rest-api-versions.ts @@ -1,5 +1,3 @@ -// This schema enforces the structure in data/tables/rest-api-versions.yml - export default { type: 'object', additionalProperties: false, diff --git a/src/data-directory/lib/data-schemas/tables/supported-code-languages.ts b/src/data-directory/lib/data-schemas/tables/supported-code-languages.ts index a298f709ef15..a7836014e0d5 100644 --- a/src/data-directory/lib/data-schemas/tables/supported-code-languages.ts +++ b/src/data-directory/lib/data-schemas/tables/supported-code-languages.ts @@ -1,5 +1,3 @@ -// This schema enforces the structure in data/tables/supported-code-languages.yml - export default { type: 'object', additionalProperties: false, @@ -164,7 +162,7 @@ export default { type: 'object', additionalProperties: false, patternProperties: { - // Language names like C, C++, C#, Go, Java, JavaScript, etc. + // Matches language names like C, C++, C#, Go, Java, and JavaScript. '^[a-zA-Z+#]+$': { type: 'object', additionalProperties: false, @@ -188,15 +186,15 @@ export default { }, codeScanning: { type: 'string', - // Allow "supported", "not-supported", or custom text like "third-party [^1]" + // Accepts supported, not-supported, or custom text such as "third-party [^1]". }, depGraph: { type: 'string', - // Allow "supported", "not-supported", or specific package managers like "npm, Yarn" + // Accepts supported, not-supported, or package managers such as "npm, Yarn". }, depUpdates: { type: 'string', - // Allow "supported", "not-supported", or specific package managers + // Accepts supported, not-supported, or package managers. }, actions: { type: 'string', @@ -204,7 +202,7 @@ export default { }, packages: { type: 'string', - // Allow "supported", "not-supported", or specific package managers + // Accepts supported, not-supported, or package managers. }, }, }, diff --git a/src/data-directory/lib/filename-to-key.ts b/src/data-directory/lib/filename-to-key.ts index e46c27903709..b9eaf51fbaf7 100644 --- a/src/data-directory/lib/filename-to-key.ts +++ b/src/data-directory/lib/filename-to-key.ts @@ -3,14 +3,12 @@ import path from 'path' const leadingPathSeparator = new RegExp(`^${RegExp.escape(path.sep)}`) const windowsLeadingPathSeparator = new RegExp('^/') -// all slashes in the filename. path.sep is OS agnostic (windows, mac, etc) +// path.sep handles the current OS; the slash and backslash regexes handle paths from other systems. const pathSeparator = new RegExp(RegExp.escape(path.sep), 'g') const windowsPathSeparator = new RegExp('/', 'g') -// handle MS Windows style double-backslashed filenames const windowsDoubleSlashSeparator = new RegExp('\\\\', 'g') -// derive `foo.bar.baz` object key from `foo/bar/baz.yml` filename export default function filenameToKey(filename: string): string { const extension = new RegExp(`${RegExp.escape(path.extname(filename))}$`) const key = filename diff --git a/src/data-directory/lib/get-data.ts b/src/data-directory/lib/get-data.ts index 65b3d8d8bc91..85f7acf1bd09 100644 --- a/src/data-directory/lib/get-data.ts +++ b/src/data-directory/lib/get-data.ts @@ -20,14 +20,10 @@ interface FileSystemError extends Error { code?: string } -// If you run `export DEBUG_JIT_DATA_READS=true` in your terminal, -// next time it will mention every file it reads from disk. +// Set DEBUG_JIT_DATA_READS=true to log every data file read from disk. const DEBUG_JIT_DATA_READS = Boolean(JSON.parse(process.env.DEBUG_JIT_DATA_READS || 'false')) -// This is a list of files that we should always immediately fall back to -// English for. -// Having this is safer than trying to wrangle the translations to NOT -// have them translated. +// Product and Copilot paths belong in the English-only set; translations can change fixed names. const ALWAYS_ENGLISH_YAML_FILES = new Set([ 'data/variables/product.yml', 'data/variables/copilot.yml', @@ -37,17 +33,13 @@ const ALWAYS_ENGLISH_MD_FILES = new Set([ 'data/reusables/ssh/known_hosts.md', ]) -// Returns all the things inside a directory export const getDeepDataByLanguage = memoize( (dottedPath: string, langCode: string, dir: string | null = null): Record => { if (!(langCode in languages)) { throw new Error(`langCode '${langCode}' not a recognized language code`) } - // The `dir` argument is only used for testing purposes. - // For example, our unit tests that depend on using a fixtures root. - // If we don't allow those tests to override the `dir` argument, - // it'll be stuck from the first time `languages.ts` was imported. + // Tests pass a fixture root because languages-server.ts captures directories when it loads. if (dir === null) { dir = languages[langCode].dir } @@ -55,8 +47,7 @@ export const getDeepDataByLanguage = memoize( }, ) -// Doesn't need to be memoized because it's used by getDataKeysByLanguage -// which is already memoized. +// getDeepDataByLanguage caches each top-level path, so recursive reads need no extra cache. function getDeepDataByDir(dottedPath: string, dir: string): Record { const fullPath = ['data'] const split = dottedPath.split(/\./g) @@ -66,7 +57,8 @@ function getDeepDataByDir(dottedPath: string, dir: string): Record { const uiEnglish = getUIData('en') if (langCode === 'en') return uiEnglish as UIStrings - // Got to combine. Start with the English and put the translation on top. - // E.g. - // english = {food: "Food", drink: "Drink"} - // swedish = {food: "Mat"} - // => - // combind = {food: "Mat", drink: "Drink"} + // Merge translations over English so missing localized UI keys fall back to English. const combined: Record = {} merge(combined, uiEnglish) merge(combined, getUIData(langCode)) return combined as UIStrings }) -// Doesn't need to be memoized because it's used by another function -// that is memoized. +// getUIDataMerged memoizes results, so this reader needs no separate cache. const getUIData = (langCode: string): Record => { const fullPath = ['data', 'ui.yml'] const { dir } = languages[langCode] return getYamlContent(dir, fullPath.join(path.sep)) as Record } +// When translated data misses a dotted path, retry English. +// lodash get returns undefined for the missing dotted path instead of ENOENT. export const getDataByLanguage = memoize((dottedPath: string, langCode: string): unknown => { if (!(langCode in languages)) throw new Error(`langCode '${langCode}' not a recognized language code`) @@ -116,32 +104,20 @@ export const getDataByLanguage = memoize((dottedPath: string, langCode: string): try { const value = getDataByDir(dottedPath, dir, languages.en.dir, langCode) - // What could happens is that a new key has only been added to - // the English data/ui.yml but hasn't been added to Japanese, but - // there nevertheless exists a Japanese `data/ui.yml`. - // Since getDataByDir() uses `get(dataObject, 'dott.ed.path')` it - // will return `undefined` if it's not present. - // If this happens, we can't rely on `err.code === 'ENOENT'` to - // fall back the English one. So we just start over using the English data. if (value === undefined && langCode !== 'en') { return getDataByDir(dottedPath, languages.en.dir) } return value } catch (error) { if (error instanceof Error && (error as YAMLException).mark && error.message) { - // It's a load() generated error! - // Remember, the file that we read might have been a .yml or a .md - // file. If it was a .md file, with corrupt front-matter that too - // would have caused a YAMLException + // Corrupt YAML files and Markdown frontmatter raise YAMLException, so translations fall back. if (langCode !== 'en') { if (DEBUG_JIT_DATA_READS) { logger.warn('Unable to parse Yaml in translation', { langCode, dottedPath, error }) } - // Give it one more chance, but use English this time return getDataByDir(dottedPath, languages.en.dir) } - // Always throw English Yaml reading errors. Staff writers - // need to know early and explicitly that they are corrupt. + // Throw English YAML errors so staff writers see corrupt source data early. throw error } @@ -150,6 +126,10 @@ export const getDataByLanguage = memoize((dottedPath: string, langCode: string): } }) +// getSmartSplit preserves dotted path segments such as version-3.4. +// Release notes split normally because numeric paths such as 3-7/0.yml would combine incorrectly. +// getDataByDir keeps {% data early-access.reusables.foo.bar %} under data/early-access. +// That data lives at data/early-access/reusables/foo/bar.md. function getDataByDir( dottedPath: string, dir: string, @@ -158,28 +138,10 @@ function getDataByDir( ): unknown { const fullPath = ['data'] - // Using English here because it doesn't matter. We just want to - // figure out how to turn `foo.version-3.4.deeper.key' into - // `['foo', 'version-3.4', 'deeper', 'key']` here and we'll need - // any directory to do that and English is always the most up-to-date. - // We need the getSmartSplit() as long as there's a chance that a - // directory or file inside data/ might contain a dot in the name, - // however the exception is the file names in data/release-notes/**/*.yml - // because it contains files that are just numbers like 3-7/0.yml and - // that can cause problems inside getSmartSplit(). const split = dottedPath.startsWith('release-notes') ? dottedPath.split('.') : getSmartSplit(dottedPath) - // For early-access data stuff, they're referred to as... - // - // {% data early-access.reusables.foo.bar %} - // - // When we "merge" in the early-access data, we put the whole directory - // within the root `data/` so it exists, on disk, as - // - // data/early-access/reusables/foo/bar.md - // if (split[0] === 'early-access') { fullPath.push(split.shift()!) } @@ -233,24 +195,12 @@ function getDataByDir( const markdown = getMarkdownContent(dir, fullPath.join(path.sep), englishRoot) let { content } = matter(markdown) if (dir !== englishRoot) { - // If we're reading a translation, we need to replace the possible - // corruptions. For example `[AUTOTITLE"을](/foo/bar)`. - // To do this we'll need the English equivalent + // Translated reusables need English content to fix corruptions like [AUTOTITLE"을](/foo/bar). let englishContent = content try { englishContent = getMarkdownContent(englishRoot, fullPath.join(path.sep), englishRoot) } catch (error) { - // In some real but rare cases a reusable doesn't exist in English. - // At all. - // This can happen when the translation is really out of date. - // You might have an old `docs-internal.locale/content/**/*.md` - // file that mentions `{% data reusables.foo.bar %}`. And it's - // working fine, except none of that exists in English. - // If this is the case, we still want to executed the - // correctTranslatedContentStrings() function, but we can't - // genuinely give it the English equivalent content, which it - // sometimes uses to correct some Liquid tags. At least other - // good corrections might happen. + // Translated pages can reference reusables missing in English; other corrections still run. if ((error as FileSystemError).code !== 'ENOENT') { throw error } @@ -263,9 +213,9 @@ function getDataByDir( return content } - // E.g. {% data ui.pages.foo.bar %} + // UI data references such as {% data ui.pages.foo.bar %} read from data/ui.yml. if (first === 'ui') { - const basename = split.shift() // i.e. 'ui' + const basename = split.shift() fullPath.push(`${basename}.yml`) const allData = getYamlContent(dir, fullPath.join(path.sep), englishRoot) return get(allData, split.join('.')) @@ -292,7 +242,7 @@ function getSmartSplit(dottedPath: string): string[] { const next = split[i + 1] if (/\d$/.test(bit) && /^\d/.test(next)) { bits.push([bit, next].join('.')) - i++ // jump ahead one position in the loop + i++ } else { bits.push(bit) } @@ -301,36 +251,12 @@ function getSmartSplit(dottedPath: string): string[] { return bits } -// The reason this is memoized, even though the parent caller function -// (`getDataByLanguage`) is also memoized is because we might read -// the same file for two different keys. E.g. -// -// getDataByLanguage('variables.product.prodname_ghe_server', 'en') -// getDataByLanguage('variables.product.company_short', 'en') -// -// ...will actually depend on reading `data/variables/product.yml`. Twice. -// Well, actually not twice because we cache the disk reading. So the outcome -// becomes this: -// -// 1. getDataByLanguage('variables.product.prodname_ghe_server', 'en') -// -> cache MISS -// 1.1. read and parse data/variables/product.yml -// -> cache MISS -// 2. getDataByLanguage('variables.product.company_short', 'en') -// -> cache MISS -// 2.1. read and parse data/variables/product.yml -// -> cache HIT (Yay!) -// +// getDataByLanguage caches each dotted key, but different keys can read the same YAML file. +// Cache YAML reads too, so product name variables share data/variables/product.yml. const getYamlContent = memoize( (root: string | undefined, relPath: string, englishRoot?: string): unknown => { - // Certain Yaml files we know we always want the English one - // no matter what the specified language is. - // For example, we never want `data/variables/product.yml` translated - // so we know to immediately fall back to the English one. if (ALWAYS_ENGLISH_YAML_FILES.has(relPath)) { - // This forces it to read from English. Later, when it goes - // into `getFileContent(...)` it will note that `root !== englishRoot` - // so it won't try to fall back. + // Passing englishRoot prevents getFileContent from treating this as a translation fallback. root = englishRoot } const fileContent = getFileContent(root, relPath, englishRoot) @@ -338,13 +264,10 @@ const getYamlContent = memoize( }, ) -// The reason why this is memoized, is the same as for getYamlContent() above. +// Cache Markdown reads too because different dotted keys can hit the same file. const getMarkdownContent = memoize( (root: string | undefined, relPath: string, englishRoot?: string): string => { - // Certain reusables we never want to be pulled from the translations. - // For example, certain reusables don't contain any English prose. Just - // facts like numbers or hardcoded key words. - // If this is the case, forcibly always draw from the English files. + // SSH fingerprints and known_hosts contain facts, not prose, so they are meant to use English. if (ALWAYS_ENGLISH_MD_FILES.has(relPath)) { root = englishRoot } @@ -364,13 +287,9 @@ const getFileContent = ( try { return fs.readFileSync(filePath, 'utf-8') } catch (err) { - // It might fail because that particular data entry doesn't yet - // exist in a translation if ((err as FileSystemError).code === 'ENOENT') { - // If looking it up as a file fails, give it one more chance if the - // read was for a translation. if (englishRoot && root !== englishRoot) { - // We can try again but this time using the English files + // Missing translated data falls back to English when an English root is available. return getFileContent(englishRoot, relPath, englishRoot) } } @@ -378,24 +297,15 @@ const getFileContent = ( } } +// Development bypasses caching because repeated sync reads stay cheap enough for debugging. +// A benchmark sampled 10 common data files across 100 runs, with about 80% YAML files. +// Median sync reads took 0.5 ms per 10 files, or 2.1 ms per 10 files with YAML parsing. function memoize( func: (...args: Args) => Return, ): (...args: Args) => Return { const cache = new Map() return (...args: Args) => { if (process.env.NODE_ENV === 'development') { - // It is very possible that certain files, when caching is disabled, - // are read multiple times in short succession. E.g. `product.yml`. - // So how expensive is it to read these files excessively? - // To answer that, we benchmarked it by sampling 10 files from the - // most common files that are used from `data/`. In fact, we ran 100 - // runs of 10 *different* files. About 80% of them were `.yml` files. - // As a median, it takes **0.5ms to read 10 files from disk** - // all in a sync manner. - // Since most files coming through here is `.yml` files (e.g. - // product.yml and ui.yml) if you also do the `load()` of the - // read content, that number becomes **2.1ms to read and parse 10 files**. - // So in conclusion, not a lot of time. return func(...args) } diff --git a/src/data-directory/middleware/data-tables.ts b/src/data-directory/middleware/data-tables.ts index 8edf5262f938..bb3ed7c20a32 100644 --- a/src/data-directory/middleware/data-tables.ts +++ b/src/data-directory/middleware/data-tables.ts @@ -6,13 +6,12 @@ let tablesCache: Record | null = null const getTables = () => { if (!tablesCache) { - // Keep product-name-heavy reference tables in English only for now + // Product-name-heavy reference tables stay in English to avoid localized product names. tablesCache = getDeepDataByLanguage('tables', 'en') } return tablesCache } -// Loads the YAML files under data/tables/ into req.context. export default async function dataTables(req: ExtendedRequest, res: Response, next: NextFunction) { if (!req.context) throw new Error('request not contextualized') diff --git a/src/data-directory/scripts/deleted-features-pr-comment.ts b/src/data-directory/scripts/deleted-features-pr-comment.ts index a601190d9c16..a88cc4565b36 100644 --- a/src/data-directory/scripts/deleted-features-pr-comment.ts +++ b/src/data-directory/scripts/deleted-features-pr-comment.ts @@ -1,12 +1,6 @@ -/** - * This script is supposed to be used in Actions. When it's run in Actions - * there will be an env var called GITHUB_REPOSITORY. If it's not there, - * you can use this script as a CLI tool. For example: - * - * export GITHUB_TOKEN=github_pat_blablabla - * npm run deleted-features-pr-comment -- github docs-internal main 2ba53b6a - * - */ +// Produces deleted-feature Markdown as an Actions output; without GITHUB_REPOSITORY, prints it. +// Required: GITHUB_TOKEN. +// CLI: npm run deleted-features-pr-comment -- github docs-internal main 2ba53b6a import { context as github_context, getOctokit } from '@actions/github' import { setOutput } from '@actions/core' @@ -44,7 +38,6 @@ async function main(owner: string, repo: string, baseSHA: string, headSHA: strin throw new Error(`GITHUB_TOKEN environment variable not set`) } const octokit = getOctokit(GITHUB_TOKEN) - // get the list of file changes from the PR const response = await octokit.rest.repos.compareCommitsWithBasehead({ owner, repo, @@ -62,10 +55,10 @@ async function main(owner: string, repo: string, baseSHA: string, headSHA: strin console.warn(`Feature involved in this PR: ${filename}; Status: ${status}`) if (status === 'removed') { - // Bad + // Deleted feature files can stay referenced in translated content. oldFilenames.push(filename) } else if (status === 'renamed') { - // Also bad + // Renamed feature files can stay referenced by the old name in translated content. const previousFilename = file.previous_filename oldFilenames.push(previousFilename) } else { diff --git a/src/data-directory/scripts/find-orphaned-features/find.ts b/src/data-directory/scripts/find-orphaned-features/find.ts index 2398738b896c..43f7b73afde0 100644 --- a/src/data-directory/scripts/find-orphaned-features/find.ts +++ b/src/data-directory/scripts/find-orphaned-features/find.ts @@ -1,31 +1,8 @@ -/** - * This script will loop over all pages, in all languages, and look at - * the following: - * - * 1. `title` in frontmatter - * 2. `intro` in frontmatter - * 3. `shortTitle` in frontmatter (if present) - * 4. the markdown body itself - * 5. The `versions:` frontmatter key (if the page is in English) - * - * Then it will search out the features mentioned based on `data/features/*.yml` - * It will make a Set of these (e.g. `dependabot-grouped-dependencies` and - * `ghas-enablement-webhook`) and one by one pluck them away. - * - * After the pages, it will loop over the reusables in English, and do the - * same search there. Once it's done the English, it loops over the - * reusables in the translations (if they exist) and does the same search. - * - * Lastly, it will output the remaining features, as relative file paths. - * For example, `data/features/havent-been-used-in-years.yml` so now you - * know that file can be deleted. - * - * NOTE: A lot of translations have corrupted Liquid. So if we can't parse - * the Liquid we fall back to string search. A regex will try to find - * all `{% ifversion ... %}` (and `elsif`) and search for any features - * mentioned inside that as a string. - * - */ +// Finds data/features/*.yml entries that no page, reusable, or variable references. +// It scans title, intro, shortTitle, body, and English versions frontmatter across all pages. +// It also scans English reusables and variables, then matching translated reusables. +// Outputs remaining features as paths such as data/features/havent-been-used-in-years.yml. +// If translated Liquid cannot parse, regex searches feature names in ifversion and elsif tags. import { strictEqual } from 'node:assert' import fs from 'fs' @@ -118,12 +95,12 @@ function formatDelta(t0: Date, t1: Date) { return `${(ms / 1000).toFixed(1)} seconds` } +// searchAndRemove scans translated reusables only when English has the same relative path. +// English content lets correctTranslatedContentStrings repair Liquid before feature matching. function searchAndRemove(features: Set, pages: Page[], verbose = false) { for (const page of pages) { const content = page.markdown - // We actually never bother looking at the `versions:` frontmatter - // key in translations, so it doesn't matter if the translated - // frontmatter might have `versions: some-old-feature`. + // Only English versions frontmatter can mark a feature used. if (page.languageCode === 'en') { for (const [key, value] of Object.entries(page.versions)) { if (key === 'feature') { @@ -144,19 +121,6 @@ function searchAndRemove(features: Set, pages: Page[], verbose = false) checkString(combined, features, { page, verbose, languageCode: page.languageCode }) } - // Reusables are a bit special, as they are shared between languages. - // There'll always be a slight mismatch between files present on disk - // in English vs. translations. - // The translations never delete files, so there's often excess reusables - // on disk in translations. And the English might be ahead, meaning a file - // has been introduced in English but not yet translated. - // The code below loops over the English reusables, and takes note of the - // their relative paths and content. Then, we re-use the keys of that map - // to know which files, in the translations, to check. And when we read - // them in, we'll need the English equivalent content to be able to - // use the correctTranslatedContentStrings function. - - // Check the English variable files. for (const filePath of getVariableFiles(path.join(languages.en.dir, 'data', 'variables'))) { const fileContent = fs.readFileSync(filePath, 'utf-8') checkString(fileContent, features, { filePath, verbose, languageCode: 'en' }) @@ -170,7 +134,7 @@ function searchAndRemove(features: Set, pages: Page[], verbose = false) englishReusables.set(relativePath, fileContent) } for (const language of Object.values(languages)) { - if (language.code === 'en') continue // Already did that in the loop above + if (language.code === 'en') continue for (const [relativePath, englishFileContent] of Array.from(englishReusables.entries())) { const filePath = path.join(language.dir, relativePath) @@ -192,10 +156,7 @@ function searchAndRemove(features: Set, pages: Page[], verbose = false) }) } catch (error) { if (error instanceof Error && 'code' in error && error.code === 'ENOENT') { - // That a reusable does *not* exist in a translation is - // perfectly expected. It means that English reusable was - // most likely added recently and the translation hasn't been - // translated yet. + // Missing translated reusables are expected when English has newer files. continue } throw error @@ -243,10 +204,7 @@ function checkString( }: { page?: Page; filePath?: string; languageCode?: string; verbose?: boolean } = {}, ) { try { - // The reason for the `noCache: true` is that we're going to be sending - // a LOT of different strings in and the cache will fill up rapidly - // when testing every possible string in every possible language for - // every page. + // Disable the Liquid token cache because scanning many different strings would fill it quickly. const tokens = getLiquidTokens(string, { noCache: true }).filter( (token): token is TagToken => token.kind === TokenKind.Tag, ) @@ -264,11 +222,10 @@ function checkString( } } catch (error) { if (error instanceof TokenizationError) { - // If it happens in English, it's a serious error + // English Liquid parse failures are source errors. if (languageCode === 'en') throw error - // The translation might, currently, have corrupted liquid - // So treat it as a string + // Translated Liquid can be corrupt, so regex search still catches feature references. if (verbose) console.log( `TokenizationError in ${page ? page.fullPath : filePath}. Treating ${page ? page.fullPath : filePath} as a string and using regex`, diff --git a/src/data-directory/scripts/find-orphaned-tables.ts b/src/data-directory/scripts/find-orphaned-tables.ts index a254863b9b6a..9ce6e3e74898 100644 --- a/src/data-directory/scripts/find-orphaned-tables.ts +++ b/src/data-directory/scripts/find-orphaned-tables.ts @@ -1,20 +1,6 @@ -// [start-readme] -// -// Print a list of all the YAML-powered table files in ./data/tables/ that -// can't be found mentioned in any source file (content, data & code), along -// with their paired schema files. Mirrors find-orphaned-assets.ts. -// -// Tables are referenced from Liquid like: -// -// {% data tables.. %} -// {% for entry in tables.. %} -// -// so a table file `data/tables//.yml` is "used" if the string -// `tables..` appears anywhere. A deeper reference such as -// `tables...` also counts, because the file key is a -// prefix of it. -// -// [end-readme] +// Prints unreferenced YAML-powered table files under ./data/tables/ and paired schema files. +// Both {% data tables.copilot.matrix-meta %} and +// {% for level in tables.copilot.matrix-meta.supportLevels %} mark the table used. import fs from 'fs' import path from 'path' @@ -28,17 +14,15 @@ import languages from '@/languages/lib/languages-server' const TABLES_DIR = 'data/tables' const SCHEMAS_DIR = 'src/data-directory/lib/data-schemas/tables' -// Tables that are referenced dynamically (not via Liquid) and must never be -// flagged as orphans. Add an entry here (the dotted key, e.g. `copilot.foo`) -// if a table is loaded by code rather than mentioned in content. +// EXCEPTIONS protects tables loaded dynamically by code rather than mentioned in content. const EXCEPTIONS = new Set([]) export type TableFile = { - // Repo-relative path to the YAML file, e.g. data/tables/copilot/model-multipliers.yml + // Repo-relative YAML path, such as data/tables/copilot/model-multipliers.yml. yml: string // Repo-relative path to the paired schema, if it exists on disk. schema?: string - // Dotted key used in Liquid, e.g. copilot.model-multipliers + // Dotted Liquid key, such as copilot.model-multipliers. key: string } @@ -72,9 +56,7 @@ type MainOptions = { excludeTranslations: boolean } -// Given the table files and the contents of every source file, return the -// tables whose Liquid key is never mentioned. Pulled out of main() so it can -// be unit tested without touching the filesystem. +// Exported for tests so orphan detection can run without filesystem reads. export function getOrphanedTables( tables: TableFile[], sourceContents: Iterable, @@ -91,8 +73,7 @@ export function getOrphanedTables( return [...orphans.values()].sort((a, b) => a.yml.localeCompare(b.yml)) } -// Only parse argv and run when invoked directly (e.g. via `npm run -// find-orphaned-tables`), not when imported by a test. +// Guard main so tests can import getOrphanedTables; npm run find-orphaned-tables invokes it. if (process.argv[1] && import.meta.url === pathToFileURL(process.argv[1]).href) { program.parse(process.argv) main(program.opts()) @@ -108,10 +89,7 @@ async function main(opts: MainOptions) { const sourceFiles: string[] = [...englishFiles] if (!excludeTranslations) { - // Translations are often behind English. A table can still be referenced - // in a translation even when no English content references it, so we must - // search translations too. We only look at files that also exist in - // English, because translations rarely delete renamed/removed files. + // Search matching translations because translated content can still reference a table. const englishRelativeFiles = new Set( englishFiles.map((englishFile) => path.relative(languages.en.dir, englishFile)), ) @@ -133,9 +111,7 @@ async function main(opts: MainOptions) { } } - // Tables can also be referenced from code (e.g. table-rendering helpers), so - // search src and contributing as well. Searching more files only ever marks - // a table as used, never as an orphan, so it errs on the safe side. + // Search code because table-rendering helpers can reference tables without Liquid. for (const root of ['contributing', 'src']) { if (!fs.existsSync(root)) continue sourceFiles.push( @@ -165,9 +141,7 @@ async function main(opts: MainOptions) { const orphanTables = getOrphanedTables(tables, readContents()) - // Safety net: if every table looks orphaned, the detection is almost - // certainly broken (e.g. content wasn't checked out). Refuse to suggest - // deleting everything. + // If every table looks orphaned, detection is probably broken; refuse to list deletions. if (tables.length > 0 && orphanTables.length === tables.length) { console.error( 'Every table was flagged as orphaned, which is almost certainly a bug. ' + diff --git a/src/data-directory/tests/copilot-matrix.ts b/src/data-directory/tests/copilot-matrix.ts index d12ec6ae8f8e..529344ab9c43 100644 --- a/src/data-directory/tests/copilot-matrix.ts +++ b/src/data-directory/tests/copilot-matrix.ts @@ -4,23 +4,14 @@ import { join } from 'path' import { load } from 'js-yaml' import { describe, expect, test } from 'vitest' -// Cross-file invariants for the Copilot IDE feature matrix. -// -// The JSON schemas validate each file in isolation. These tests cover the -// relationships *between* matrix-meta.yml and the per-IDE files, which is where -// a hand edit — or, later, an automated changelog-driven update — is most -// likely to introduce a silent error. -// -// "Silent" is the operative word: a missing or mistyped key does not raise an -// error, it renders as ✗ (not supported) to customers. +// JSON schemas validate each file in isolation. These tests cover cross-file matrix relationships. +// Missing or mistyped keys silently render as ✗ (not supported) in customer-facing tables. const MATRIX_DIR = join(process.cwd(), 'data/tables/copilot/matrix') const META_PATH = join(process.cwd(), 'data/tables/copilot/matrix-meta.yml') -// Stands for "supported since before we tracked versions". Some IDEs list it in -// `versions` without putting it in a `versionGroup`, so it is the one version -// allowed to have no detail table. Removing it is a customer-visible content -// decision; until then it is excluded from the grouping invariant below. +// Some IDEs use 0.0.0 for supported-before-tracking without a versionGroup. +// Removing the sentinel is customer-visible, so the grouping invariant excludes it. const SENTINEL_VERSION = '0.0.0' type Ide = { @@ -70,8 +61,7 @@ describe('copilot matrix meta', () => { expect(new Set(meta.featureOrder).size).toBe(meta.featureOrder.length) }) - // A stale featureOrder entry that no IDE uses renders as a row of ✗ across - // every column of the summary table. + // A stale featureOrder entry renders as a row of ✗ across every summary-table column. test('every featureOrder entry is used by at least one IDE', () => { const used = new Set() for (const ide of Object.values(ides)) { @@ -114,13 +104,9 @@ describe.each(ideFilenames)('copilot matrix: %s', (slug) => { ).toEqual([]) }) - // Only versions listed in a versionGroup are rendered as a detail table. A - // version in `versions` that is in no group is data customers cannot see — - // and the summary table reads `versions | first`, so if it is the newest one - // the page shows support data for a version with no detail table at all. - // This is the most likely mistake for an automated updater that appends to - // `versions` and forgets `versionGroups`, and checking only the newest - // version would miss a backfilled older one. + // Only versions listed in versionGroups render detail tables. The test skips the 0.0.0 sentinel. + // The summary table reads versions | first, so an ungrouped newest version has no detail table. + // Checking every version also catches backfilled older versions that automated updates miss. test('every version appears in at least one versionGroup', () => { const grouped = new Set(Object.values(ide.versionGroups).flat()) const ungrouped = ide.versions.filter( diff --git a/src/data-directory/tests/data-schemas.ts b/src/data-directory/tests/data-schemas.ts index 7dd168ade55b..bc2b83a61f37 100644 --- a/src/data-directory/tests/data-schemas.ts +++ b/src/data-directory/tests/data-schemas.ts @@ -64,7 +64,6 @@ describe('YAML-powered tables', () => { const schemaPath = join(schemasDir, `${name}.ts`) expect(existsSync(schemaPath)).toBe(true) - // Also verify it's registered in the dataSchemas const dataKey = `data/tables/${yamlFile}` expect(dataSchemas[dataKey]).toBeDefined() } diff --git a/src/data-directory/tests/find-orphaned-tables.ts b/src/data-directory/tests/find-orphaned-tables.ts index b3ac92a04978..1dfc47015020 100644 --- a/src/data-directory/tests/find-orphaned-tables.ts +++ b/src/data-directory/tests/find-orphaned-tables.ts @@ -41,8 +41,6 @@ describe('getOrphanedTables', () => { }) test('counts a deeper sub-key reference as using the table file', () => { - // A reference to `tables.copilot.copilot-matrix.ides` should mark the - // `copilot.copilot-matrix` file as used. const orphans = getOrphanedTables( [table('copilot.copilot-matrix')], ['{% for row in tables.copilot.copilot-matrix.ides %}'], @@ -51,8 +49,6 @@ describe('getOrphanedTables', () => { }) test('does not let a longer key falsely mark a shorter, unrelated table', () => { - // `tables.copilot.annual-subscriber-model-multipliers` must NOT mark - // `copilot.model-multipliers` as used. const orphans = getOrphanedTables( [table('copilot.model-multipliers')], ['{% data tables.copilot.annual-subscriber-model-multipliers %}'], diff --git a/src/data-directory/tests/get-data.ts b/src/data-directory/tests/get-data.ts index 7b3a0414f929..68e834bce35e 100644 --- a/src/data-directory/tests/get-data.ts +++ b/src/data-directory/tests/get-data.ts @@ -14,7 +14,7 @@ import { DataDirectory } from '@/tests/helpers/data-directory' describe('get-data', () => { let dd: DataDirectory const enDirBefore = languages.en.dir - // Only `en` is available in tests, so pretend we also have Japanese + // Only en is available in tests, so copy English metadata for Japanese fixtures. languages.ja = Object.assign({}, languages.en, {}) beforeAll(() => { @@ -77,12 +77,10 @@ describe('get-data', () => { const result = getDataByLanguage('variables.stuff.foo', 'en') expect(result).toBe('Foo') } - // Test that memoization doesn't go wrong { const result = getDataByLanguage('variables.stuff.bar', 'en') expect(result).toBe('Bar') } - // Test that unrecognized keys just return `undefined` { const result = getDataByLanguage('variables.stuff.neverheardof', 'en') expect(result).toBeUndefined() @@ -94,12 +92,10 @@ describe('get-data', () => { const result = getDataByLanguage('variables.stuff.foo', 'ja') expect(result).toBe('フー') } - // Test fallback to English if not present in translation { const result = getDataByLanguage('variables.stuff.bar', 'ja') expect(result).toBe('Bar') } - // Test that unrecognized keys just return `undefined` { const result = getDataByLanguage('variables.stuff.neverheardof', 'ja') expect(result).toBeUndefined() @@ -111,12 +107,10 @@ describe('get-data', () => { const result = getDataByLanguage('variables.stuff.key_non_existent', 'en') expect(result).toBeUndefined() } - // Test fallback to English if not present in translation { const result = getDataByLanguage('variables.stuff.key_non_existent', 'ja') expect(result).toBeUndefined() } - // Returns undefined if not only the key is missing but the whole file too { const result = getDataByLanguage('variables.notpresent.whatever', 'en') expect(result).toBeUndefined() @@ -128,7 +122,6 @@ describe('get-data', () => { const result = getDataByLanguage('reusables.coolness', 'en') expect(result).toBe('This is *Markdown*') } - // Test that memoization doesn't go wrong { const result = getDataByLanguage('reusables.otherness', 'en') expect(result).toBe('**Also** Markdown') @@ -140,7 +133,6 @@ describe('get-data', () => { const result = getDataByLanguage('reusables.coolness', 'ja') expect(result).toBe('これがマークダウンです') } - // Test translations fall back to English if file doesn't exist { const result = getDataByLanguage('reusables.otherness', 'ja') expect(result).toBe('**Also** Markdown') @@ -152,7 +144,6 @@ describe('get-data', () => { const result = getDataByLanguage('reusables.neverheardof', 'en') expect(result).toBeUndefined() } - // Test translations will try English but fail if the fallback fails too { const result = getDataByLanguage('reusables.neverheardof', 'ja') expect(result).toBeUndefined() @@ -165,12 +156,10 @@ describe('get-data', () => { expect(result.key).toBe('Value') expect((result.deep as Record).er).toBe('Depth') } - // In a specific language { const result = getUIDataMerged('ja') expect(result.key).toBe('価値') expect((result.deep as Record).er).toBe('深さ') - // Note how it falls back to English on that key expect((result.deep as Record).est).toBe('Deepest') } }) @@ -181,7 +170,6 @@ describe('get-data', () => { expect((result.stuff as Record).foo).toBe('Foo') expect((result.stuff as Record).bar).toBe('Bar') } - // All reusables { const result = getDeepDataByLanguage('reusables', 'en') expect(result['coolness.md']).toBe('This is *Markdown*') @@ -213,7 +201,7 @@ front: >'matter describe('get-data on corrupt translations', () => { let dd: DataDirectory const enDirBefore = languages.en.dir - // Only `en` is available in vitest tests, so pretend we also have Japanese + // Only en is available in tests, so copy English metadata for Japanese fixtures. languages.ja = Object.assign({}, languages.en, {}) beforeAll(() => { @@ -263,12 +251,10 @@ describe('get-data on corrupt translations', () => { }) test('getDataByLanguage on a corrupt .yml file', () => { - // First make sure it works in English { const result = getDataByLanguage('variables.everything.is', 'en') expect(result).toBe('Awesome') } - // Japanese translations would fall back due to a corrupt Yaml file { const result = getDataByLanguage('variables.everything.is', 'ja') expect(result).toBe('Awesome') @@ -276,12 +262,10 @@ describe('get-data on corrupt translations', () => { }) test('getDataByLanguage on a corrupt .md file', () => { - // First make sure it works in English { const result = getDataByLanguage('reusables.cool', 'en') expect(result).toBe('*English* /Markdown/') } - // Japanese translations would fall back due to a corrupt Yaml file { const result = getDataByLanguage('reusables.cool', 'ja') expect(result).toBe('*English* /Markdown/') @@ -317,11 +301,11 @@ describe('get-data applies corrections to translated variables', () => { data: { variables: { myproduct: { - // Corrupted: `data` translated to Japanese `データ` + // Translation corrupts the data keyword to データ. name: '{% データ variables.myproduct.name %}', }, phases: { - // Not corrupted, so it should pass through unchanged + // Valid ifversion stays unchanged. preview: '{% ifversion ghes < 3.16 %}ベータ{% else %}パブリックプレビュー{% endif %}', }, }, @@ -337,12 +321,10 @@ describe('get-data applies corrections to translated variables', () => { }) test('corrects corrupted Liquid keywords in translated variables', () => { - // English variable is returned as-is { const result = getDataByLanguage('variables.myproduct.name', 'en') expect(result).toBe('GitHub') } - // Japanese translation with corrupted `データ` → `data` gets corrected { const result = getDataByLanguage('variables.myproduct.name', 'ja') expect(result).toBe('{% data variables.myproduct.name %}') @@ -350,7 +332,6 @@ describe('get-data applies corrections to translated variables', () => { }) test('leaves valid translated variables unchanged', () => { - // Valid ifversion in translated variable should pass through { const result = getDataByLanguage('variables.phases.preview', 'ja') expect(result).toBe( diff --git a/src/data-directory/tests/index.ts b/src/data-directory/tests/index.ts index cdef3164a438..03e0de1a292b 100644 --- a/src/data-directory/tests/index.ts +++ b/src/data-directory/tests/index.ts @@ -31,16 +31,14 @@ describe('data-directory', () => { const extensions = ['.yml', 'markdown'] const data = dataDirectory(fixturesDir, { extensions }) expect('bar' in data).toBe(true) - expect('foo' in data).toBe(false) // JSON file should be ignored + expect('foo' in data).toBe(false) }) test('option: ignorePatterns', async () => { const ignorePatterns: RegExp[] = [] - // README is ignored by default expect('README' in dataDirectory(fixturesDir)).toBe(false) - // README can be included by setting empty ignorePatterns array expect('README' in dataDirectory(fixturesDir, { ignorePatterns })).toBe(true) }) }) diff --git a/src/data-directory/tests/orphaned-features.ts b/src/data-directory/tests/orphaned-features.ts index ea931b6b4f21..e63c7c1f8db7 100644 --- a/src/data-directory/tests/orphaned-features.ts +++ b/src/data-directory/tests/orphaned-features.ts @@ -49,7 +49,6 @@ describe('orphaned features detection', () => { }) test('helper functions handle nested directories', () => { - // Create a temporary nested structure to test const tempDir = path.join(__dirname, 'temp-nested-test') const nestedVariablesDir = path.join(tempDir, 'variables', 'nested') const nestedReusablesDir = path.join(tempDir, 'reusables', 'nested') @@ -78,7 +77,6 @@ describe('orphaned features detection', () => { }) test('helper functions ignore non-target files', () => { - // Create a temporary directory with mixed file types const tempDir = path.join(__dirname, 'temp-mixed-files') fs.mkdirSync(tempDir, { recursive: true }) @@ -90,12 +88,10 @@ describe('orphaned features detection', () => { fs.writeFileSync(path.join(tempDir, 'README.md'), '# README') try { - // getVariableFiles should only find .yml files (excluding README.yml) const variableFiles = getVariableFiles(tempDir) expect(variableFiles).toHaveLength(1) expect(variableFiles[0]).toMatch(/test\.yml$/) - // getReusableFiles should only find .md files (excluding README.md) const reusableFiles = getReusableFiles(tempDir) expect(reusableFiles).toHaveLength(1) expect(reusableFiles[0]).toMatch(/test\.md$/) @@ -105,13 +101,9 @@ describe('orphaned features detection', () => { }) test('verify fix addresses the original issue scenario', () => { - // This test simulates the original issue where features were used only in variables - // but not detected by the orphaned features script - const variablesDir = path.join(fixturesDir, 'data', 'variables') const featuresDir = path.join(fixturesDir, 'data', 'features') - // Verify our test setup has the scenario described in the issue expect(fs.existsSync(path.join(featuresDir, 'used-in-variables.yml'))).toBe(true) expect(fs.existsSync(path.join(featuresDir, 'truly-orphaned.yml'))).toBe(true) @@ -121,8 +113,6 @@ describe('orphaned features detection', () => { const variableFiles = getVariableFiles(variablesDir) expect(variableFiles.length).toBeGreaterThan(0) - // This proves that the fix would catch features used in variables files - // because the orphaned features script now scans these files const foundFeatureUsage = variableFiles.some((filePath) => { const content = fs.readFileSync(filePath, 'utf-8') return content.includes('used-in-variables') @@ -132,8 +122,6 @@ describe('orphaned features detection', () => { }) test('functions correctly identify different file types in same directory', () => { - // Create a directory with both .yml and .md files to ensure each function - // only picks up its target file types const tempDir = path.join(__dirname, 'temp-mixed-target-files') fs.mkdirSync(tempDir, { recursive: true }) @@ -148,7 +136,6 @@ describe('orphaned features detection', () => { fs.writeFileSync(path.join(tempDir, 'other.txt'), 'other content') try { - // Each function should only find its target file type const variableFiles = getVariableFiles(tempDir) const reusableFiles = getReusableFiles(tempDir) diff --git a/src/data-directory/tests/ui-yml-structure.ts b/src/data-directory/tests/ui-yml-structure.ts index edc8cf7e789a..91803b1c281e 100644 --- a/src/data-directory/tests/ui-yml-structure.ts +++ b/src/data-directory/tests/ui-yml-structure.ts @@ -13,7 +13,7 @@ describe('data/ui.yml structure', () => { const violations: string[] = [] for (let i = 0; i < lines.length; i++) { - // A top-level key starts at column 0 with a word followed by ':' + // Top-level keys start at column 0 with a word followed by colon. if (/^[a-z_]+:/.test(lines[i]) && i > 0) { if (lines[i - 1].trim() !== '') { violations.push(`Line ${i + 1}: "${lines[i]}" is not preceded by a blank line`) diff --git a/src/deployments/production/build-scripts/clone-or-use-cached-repo.sh b/src/deployments/production/build-scripts/clone-or-use-cached-repo.sh index a908885ab652..0dad817f3ff9 100644 --- a/src/deployments/production/build-scripts/clone-or-use-cached-repo.sh +++ b/src/deployments/production/build-scripts/clone-or-use-cached-repo.sh @@ -1,11 +1,7 @@ set -e -# Reuses the repo cached by a previous Dockerfile build, or clones it fresh -# and checks out the given branch/SHA. -# Arguments: -# $1 - Repository name (for directory naming) -# $2 - Repository URL -# $3 - Branch to clone +# Reuses a cached repo or clones it, then checks out the requested branch. +# Arguments: $1 cache directory, $2 GitHub repo name under github, $3 branch. clone_or_use_cached_repo() { repo_name="$1" repo_url="$2" diff --git a/src/deployments/production/build-scripts/fetch-repos.sh b/src/deployments/production/build-scripts/fetch-repos.sh index 3f2239fc448e..9f24050f0013 100644 --- a/src/deployments/production/build-scripts/fetch-repos.sh +++ b/src/deployments/production/build-scripts/fetch-repos.sh @@ -1,7 +1,7 @@ #!/usr/bin/env sh -# Called from the production Dockerfile. The Dockerfile only COPYs what it -# needs, but these scripts still run as if from the docs-internal root. +# The production Dockerfile copies only required files, but these scripts still run from the +# docs-internal root. echo "Fetching and resolving early-access, and translations repos" @@ -9,7 +9,7 @@ set -e . ./build-scripts/clone-or-use-cached-repo.sh -# From the --secret mounted by the Docker build. +# Docker build mounts DOCS_BOT_PAT_BASE at /run/secrets/DOCS_BOT_PAT_BASE. GITHUB_TOKEN=$(cat /run/secrets/DOCS_BOT_PAT_BASE) echo "Fetching early access..." @@ -17,11 +17,11 @@ clone_or_use_cached_repo "docs-early-access" "docs-early-access" "main" echo "Merging early access..." . ./build-scripts/merge-early-access.sh -# Clone into `translations/` inside the Dockerfile's WORKDIR, the docs-internal root. +# Clone translations under the Dockerfile WORKDIR, the docs-internal root. mkdir -p translations cd translations -# Temporarily turn off exit-on-error so we can collect all PIDs +# Disable exit-on-error so the script can collect every background clone failure. set +e pids="" @@ -35,7 +35,6 @@ for pid in $pids; do wait "$pid" || failures=$((failures+1)) done -# Restore strict mode set -e if [ "$failures" -gt 0 ]; then @@ -45,8 +44,8 @@ else echo "✅ All translations fetched." fi -# Go back to the root of the docs-internal repo +# Return to the docs-internal root after cloning translations. cd .. -# Don't leave the token in the environment. +# Remove the token from the shell environment. unset GITHUB_TOKEN diff --git a/src/deployments/production/build-scripts/merge-early-access.sh b/src/deployments/production/build-scripts/merge-early-access.sh index 317f81a9dc87..00a3c3bfef7a 100755 --- a/src/deployments/production/build-scripts/merge-early-access.sh +++ b/src/deployments/production/build-scripts/merge-early-access.sh @@ -1,7 +1,6 @@ #!/usr/bin/env sh -# Merges docs-early-access files into docs-internal. Runs from the -# docs-internal root. +# Merges docs-early-access files into docs-internal from the docs-internal root. mv docs-early-access/assets/images assets/images/early-access mv docs-early-access/content content/early-access diff --git a/src/dev-toc/generate.ts b/src/dev-toc/generate.ts index f883ccbbad96..3d7c16ffc1ec 100644 --- a/src/dev-toc/generate.ts +++ b/src/dev-toc/generate.ts @@ -1,14 +1,10 @@ -/** - * @purpose Writer tool - * @description Generate a local table of contents for the GitHub Docs website - * - * This script creates static HTML files for each documentation version, renders page titles - * using Liquid templating, and opens the generated TOC in your browser for easy navigation - * during development. Supports command-line options to specify which sections should be - * open by default. - * - * Usage: tsx src/dev-toc/generate.ts [-o product-ids...] - */ +// @purpose Writer tool +// @description Generate a local table of contents for the GitHub Docs website +// +// Creates static HTML for each documentation version, renders Liquid page titles, and opens the +// generated table of contents in your browser. Use -o product-ids... to open sections by default. +// +// Run with: tsx src/dev-toc/generate.ts [-o product-ids...] import fs from 'fs' import path from 'path' diff --git a/src/dev-toc/layout.html b/src/dev-toc/layout.html index 24d8fab8fccd..78c08dde5811 100644 --- a/src/dev-toc/layout.html +++ b/src/dev-toc/layout.html @@ -38,7 +38,6 @@

TOC for {{ allVersions[currentVersion].versio
  • {{ productPage.renderedFullTitle }} - {% comment %} Unified nested rendering with depth control {% endcomment %} {% if productPage.childPages and productPage.childPages.size > 0 %}