diff --git a/.github/actions/node-npm-setup/action.yml b/.github/actions/node-npm-setup/action.yml index 8e4bb2999881..372fc0132c03 100644 --- a/.github/actions/node-npm-setup/action.yml +++ b/.github/actions/node-npm-setup/action.yml @@ -13,7 +13,8 @@ runs: SEGMENT_DOWNLOAD_TIMEOUT_MINS: '1' with: path: node_modules - key: ${{ runner.os }}-node_modules-${{ hashFiles('package*.json') }}-${{ hashFiles('.github/actions/node-npm-setup/action.yml') }} + # The postinstall script applies patches/ during npm ci, so patch changes must miss the cache. + key: ${{ runner.os }}-node_modules-${{ hashFiles('package*.json', 'patches/**') }}-${{ hashFiles('.github/actions/node-npm-setup/action.yml') }} # The npm download cache only helps npm ci, so skip its 153 MB restore when node_modules hits. - name: Setup Node.js diff --git a/.github/workflows/orphaned-features-check.yml b/.github/workflows/orphaned-features-check.yml index 0fa5fb4604f9..70f9780859d6 100644 --- a/.github/workflows/orphaned-features-check.yml +++ b/.github/workflows/orphaned-features-check.yml @@ -14,6 +14,7 @@ on: - 'package*.json' - 'src/data-directory/scripts/find-orphaned-features/**' - .github/actions/clone-translations/action.yml + - .github/actions/get-docs-early-access/action.yml - .github/actions/node-npm-setup/action.yml permissions: @@ -38,6 +39,11 @@ jobs: - uses: ./.github/actions/node-npm-setup + # Early access docs use data/features too, so include them before deciding that a feature is unused. + - uses: ./.github/actions/get-docs-early-access + with: + token: ${{ secrets.DOCS_BOT_PAT_BASE }} + - name: Check for orphaned features env: # The gh CLI reads GITHUB_TOKEN. diff --git a/.github/workflows/repo-sync.yml b/.github/workflows/repo-sync.yml index 8057b2770d3b..b3022cb9e60e 100644 --- a/.github/workflows/repo-sync.yml +++ b/.github/workflows/repo-sync.yml @@ -12,6 +12,12 @@ permissions: contents: write pull-requests: write +# Queue overlapping runs. A second run closes every open repo-sync pull request, including one +# the first run is still waiting to merge. Cancelling mid-merge would leave that pull request open. +concurrency: + group: ${{ github.workflow }} + cancel-in-progress: false + jobs: repo-sync: if: github.repository == 'github/docs-internal' || github.repository == 'github/docs' diff --git a/.github/workflows/reviewers-dependabot.yml b/.github/workflows/reviewers-dependabot.yml index dc3b615c89a6..c2eb715a9c9a 100644 --- a/.github/workflows/reviewers-dependabot.yml +++ b/.github/workflows/reviewers-dependabot.yml @@ -12,8 +12,32 @@ on: - reopened - synchronize paths: - - 'data/reusable/dependabot/**' - - 'content/code-security/dependabot/**' + - 'data/reusables/dependabot/**' + - 'data/reusables/**/*dependabot*' + - 'content/code-security/**/*dependabot*' + - 'content/code-security/**/*dependabot*/**' + # Dependabot articles whose paths do not contain "dependabot" + - 'content/code-security/concepts/supply-chain-security/malware-alerts.md' + - 'content/code-security/concepts/supply-chain-security/multi-ecosystem-updates.md' + - 'content/code-security/how-tos/secure-at-scale/configure-enterprise-security/configure-specific-tools/configure-limited-internet-access.md' + - 'content/code-security/how-tos/secure-your-supply-chain/manage-your-dependency-security/configure-access-to-private-registries.md' + - 'content/code-security/how-tos/secure-your-supply-chain/manage-your-dependency-security/configure-on-github-hosted-runners.md' + - 'content/code-security/how-tos/secure-your-supply-chain/manage-your-dependency-security/configure-on-self-hosted-runners.md' + - 'content/code-security/how-tos/secure-your-supply-chain/manage-your-dependency-security/configure-private-registries.md' + - 'content/code-security/how-tos/secure-your-supply-chain/manage-your-dependency-security/controlling-dependencies-updated.md' + - 'content/code-security/how-tos/secure-your-supply-chain/manage-your-dependency-security/list-configured-dependencies.md' + - 'content/code-security/how-tos/secure-your-supply-chain/manage-your-dependency-security/prioritize-with-preset-rules.md' + - 'content/code-security/how-tos/secure-your-supply-chain/manage-your-dependency-security/remove-access-to-public-registries.md' + - 'content/code-security/how-tos/secure-your-supply-chain/manage-your-dependency-security/resolve-a-blocked-host.md' + - 'content/code-security/how-tos/secure-your-supply-chain/secure-your-dependencies/auto-update-actions.md' + - 'content/code-security/how-tos/secure-your-supply-chain/secure-your-dependencies/configure-malware-alerts.md' + - 'content/code-security/how-tos/secure-your-supply-chain/secure-your-dependencies/configure-security-updates.md' + - 'content/code-security/how-tos/secure-your-supply-chain/secure-your-dependencies/configure-version-updates.md' + - 'content/code-security/how-tos/secure-your-supply-chain/secure-your-dependencies/configuring-multi-ecosystem-updates.md' + - 'content/code-security/reference/supply-chain-security/criteria-for-preset-rules.md' + - 'content/code-security/reference/supply-chain-security/supported-ecosystems-and-manifests-for-dependency-scope.md' + - 'content/code-security/reference/supply-chain-security/supported-ecosystems-and-repositories.md' + - 'content/code-security/tutorials/secure-your-dependencies/optimizing-pr-creation-version-updates.md' - 'content/rest/dependabot/**' - '.github/workflows/reviewers-dependabot.yml' diff --git a/.github/workflows/sync-audit-logs.yml b/.github/workflows/sync-audit-logs.yml index f19d5753d4b7..768d60a13205 100644 --- a/.github/workflows/sync-audit-logs.yml +++ b/.github/workflows/sync-audit-logs.yml @@ -47,61 +47,81 @@ jobs: env: 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 - echo "Branch $branchname already exists in 'github/docs-internal'. Exiting..." - exit 0 - fi - git checkout -b $branchname - echo "Created a new branch $branchname" - - echo "Preparing commit..." - git config --global user.name "docs-bot" - git config --global user.email "77750099+docs-bot@users.noreply.github.com" - git add -A . - echo "Prepared commit" - - echo "Check if there are changes..." - if git diff-index --cached --quiet HEAD -- . ':(exclude)src/audit-logs/lib/config.json' - then - echo "No real changes (only the SHA in config.json moved). Exiting…" + + # People close sync PRs that fail to deploy or merge, so do not reopen this SHA after a close. + # Reuse an open PR from an earlier run so a rerun does not restart its CI. + # --head matches fork branches too, so only trust same-repo PRs that docs-bot opened. + existing_pr=$(gh pr list --repo github/docs-internal --head "$branchname" --state all --json number,state,author,isCrossRepository \ + --jq 'map(select(.author.login == "docs-bot" and (.isCrossRepository | not))) | .[0] | select(.) | "\(.number) \(.state)"') + if [ -n "$existing_pr" ] && [ "${existing_pr#* }" != "OPEN" ]; then + echo "Pull request #${existing_pr% *} for branch $branchname is ${existing_pr#* }. Exiting..." exit 0 + elif [ -n "$existing_pr" ]; then + pr=${existing_pr% *} + echo "Pull request #$pr already exists for branch $branchname. Checking that it holds only the regenerated data..." + # Others can push to the branch, so approve only if it matches this run's output. + git add -A . + git fetch --depth=1 origin "$branchname" + pr_files=$(gh api --paginate "repos/github/docs-internal/pulls/$pr/files" --jq '.[].filename') + unexpected_files=$(echo "$pr_files" | grep -v -e '^src/audit-logs/data/' -e '^src/audit-logs/lib/config\.json$' || true) + if [ -n "$unexpected_files" ] || ! git diff --cached --quiet FETCH_HEAD -- src/audit-logs/data src/audit-logs/lib/config.json; then + echo "Pull request #$pr does not match the regenerated audit log data. Fix or close it by hand." + echo "Unexpected files: ${unexpected_files:-none}" + exit 1 + fi + echo "Pull request #$pr matches the regenerated data. Skipping to approval." + else + git checkout -b $branchname + echo "Created a new branch $branchname" + + echo "Preparing commit..." + git config --global user.name "docs-bot" + git config --global user.email "77750099+docs-bot@users.noreply.github.com" + git add -A . + echo "Prepared commit" + + echo "Check if there are changes..." + if git diff-index --cached --quiet HEAD -- . ':(exclude)src/audit-logs/lib/config.json' + then + echo "No real changes (only the SHA in config.json moved). Exiting…" + exit 0 + fi + echo "Changes detected, proceeding" + + echo "Creating commit..." + git commit -m "Add updated audit log event data" + echo "Created commit" + + # Force-push reruns over failed attempts; this workflow exclusively owns these branches. + echo "Pushing commit..." + git push --force -u origin $branchname + echo "Pushed commit" + + echo "Creating pull request..." + pr=$(gh pr create \ + --title "Update audit log event data" \ + --body '👋 Docs First Responder. This PR updates the audit log event data with the latest changes, synced from github/audit-log-allowlists. + Make sure the PR builds successfully and there are no gross errors (for example, a file is deleted). You do not need to validate the contents (that is the responsibility of product teams). + If CI does not pass or other problems arise, contact #technical-content on slack.' \ + --repo github/docs-internal \ + --label audit-log-pipeline,workflow-generated \ + --head=$branchname) + echo "Created pull request $pr" fi - echo "Changes detected, proceeding" - - echo "Creating commit..." - git commit -m "Add updated audit log event data" - echo "Created commit" - - echo "Pushing commit..." - git push origin $branchname - echo "Pushed commit" - - echo "Creating pull request..." - gh pr create \ - --title "Update audit log event data" \ - --body '👋 Docs First Responder. This PR updates the audit log event data with the latest changes, synced from github/audit-log-allowlists. - Make sure the PR builds successfully and there are no gross errors (for example, a file is deleted). You do not need to validate the contents (that is the responsibility of product teams). - If CI does not pass or other problems arise, contact #technical-content on slack.' \ - --repo github/docs-internal \ - --label audit-log-pipeline,workflow-generated \ - --head=$branchname - echo "Created pull request" # 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 + gh pr review "$pr" --approve echo "Approved pull request" # 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 }}" - gh pr merge --auto + gh pr merge "$pr" --auto echo "Set pull request to auto merge" - uses: ./.github/actions/create-workflow-failure-issue diff --git a/content/actions/how-tos/secure-your-work/security-harden-deployments/oidc-in-jfrog.md b/content/actions/how-tos/secure-your-work/security-harden-deployments/oidc-in-jfrog.md index 1f12a985cf17..8255568a3b20 100644 --- a/content/actions/how-tos/secure-your-work/security-harden-deployments/oidc-in-jfrog.md +++ b/content/actions/how-tos/secure-your-work/security-harden-deployments/oidc-in-jfrog.md @@ -87,7 +87,7 @@ jobs: ```yaml {% data reusables.actions.actions-not-certified-by-github-comment %} - name: Sign in to Artifactory Docker registry - uses: docker/login-action@v3 + uses: docker/login-action@c94ce9fb468520275223c153574b00df6fe4bcc9 # v3.7.0 with: registry: {% raw %}${{ env.JF_URL }}{% endraw %} username: {% raw %}${{ steps.setup-jfrog-cli.outputs.oidc-user }}{% endraw %} diff --git a/content/copilot/how-tos/copilot-sdk/auth/server-to-server-tokens.md b/content/copilot/how-tos/copilot-sdk/auth/server-to-server-tokens.md index a063fb2aa1ef..89e67caeea35 100644 --- a/content/copilot/how-tos/copilot-sdk/auth/server-to-server-tokens.md +++ b/content/copilot/how-tos/copilot-sdk/auth/server-to-server-tokens.md @@ -30,7 +30,7 @@ jobs: - uses: {% data reusables.actions.action-checkout %} - run: your-application env: - GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} + GITHUB_TOKEN: {% raw %}${{ secrets.GITHUB_TOKEN }}{% endraw %} ``` The organization's **Allow use of Copilot CLI billed to the organization** policy must be enabled. This approach needs no GitHub App or stored authentication secret. For details, see [AUTOTITLE](/copilot/how-tos/copilot-cli/use-copilot-cli-in-actions). diff --git a/data/ui.yml b/data/ui.yml index a88b9607bc29..131b00dd395f 100644 --- a/data/ui.yml +++ b/data/ui.yml @@ -210,7 +210,7 @@ rest: banner: api_versioned: The REST API is now versioned. api_version_info: For more information, see "About API versioning." - ghes_api_versioned: After a site administrator upgrades your Enterprise Server instance to {{ firstGhesReleaseWithApiVersions.versionTitle }} or later, the REST API will be versioned. To learn how to find your instance's version, see "About versions of GitHub Docs". + ghes_api_versioned: After a site administrator upgrades your Enterprise Server instance to {{ firstGhesReleaseWithApiVersions.versionTitle }} or later, the REST API will be versioned. To learn how to find your instance's version, see "About versions of GitHub Docs". redirect_notice: We've recently moved some of the REST API documentation. redirect_repo: If you can't find what you're looking for, you might try the new {{ newRestPagesLinks }} REST API pages. redirect_enterprise: If you can't find what you're looking for, you might try the {{ actionsPageLink }} REST API page. diff --git a/src/article-api/lib/summarize-schema.ts b/src/article-api/lib/summarize-schema.ts index ccf7c1cafc5f..93e0010eb08b 100644 --- a/src/article-api/lib/summarize-schema.ts +++ b/src/article-api/lib/summarize-schema.ts @@ -142,6 +142,22 @@ function renderProperties( if (propType === 'array' && prop.items) { const itemTitle = prop.items.title + const itemKey = (['oneOf', 'anyOf', 'allOf'] as const).find((k) => prop.items![k]) + if (itemKey) { + const label = `${itemTitle ? `array of \`${itemTitle}\`` : 'array of items'}${isNullable ? ' or null' : ''}` + if (shouldReference(itemTitle, depth < MAX_DEPTH, seen)) { + lines.push(`${prefix}* \`${name}\`: ${reqStr}${label} (see above)`) + } else { + lines.push(`${prefix}* \`${name}\`: ${reqStr}${label}:`) + if (prop.items.properties && depth < MAX_DEPTH) { + lines.push(renderProperties(prop.items, indent + 1, depth + 1, seen)) + } + lines.push( + renderCompositionVariants(itemKey, prop.items[itemKey]!, indent + 1, depth, seen), + ) + } + continue + } const canExpand = Boolean(prop.items.properties) && depth < MAX_DEPTH if (shouldReference(itemTitle, canExpand, seen)) { lines.push( diff --git a/src/article-api/middleware/article.ts b/src/article-api/middleware/article.ts index 8b71f9590350..fe0ca5192a6b 100644 --- a/src/article-api/middleware/article.ts +++ b/src/article-api/middleware/article.ts @@ -12,7 +12,7 @@ import { import { getArticleBody } from './article-body' import { getMetadata } from './article-pageinfo' import { - makeLanguageSurrogateKey, + makeContentSurrogateKeys, setFastlySurrogateKey, } from '@/frame/middleware/set-fastly-surrogate-key' import statsd from '@/observability/lib/statsd' @@ -60,6 +60,7 @@ router.get( recordBodySize(req, bodyContent) defaultCacheControl(res) + setArticleSurrogateKeys(req, res) return res.json({ meta, body: bodyContent, @@ -102,12 +103,11 @@ router.get( recordBodySize(req, bodyContent) defaultCacheControl(res) + setArticleSurrogateKeys(req, res) return res.type('text/markdown').send(bodyContent) }), ) -// /api/article/meta sets a language surrogate key because /api URLs lack a language segment. -// Fastly needs the key for staggered language purges. /** * Get metadata about an article. * @route GET /api/article/meta @@ -147,16 +147,21 @@ router.get( incrementArticleLookup(req, 'meta') defaultCacheControl(res) - - setFastlySurrogateKey( - res, - makeLanguageSurrogateKey(req.pageinfo?.page?.languageCode || 'en'), - true, - ) + setArticleSurrogateKeys(req, res) return res.json(meta) }), ) +// /api URLs lack a language segment, so every Article API route sets language and page keys. +// The page key matches the key that deploy purges build from changed content files. +function setArticleSurrogateKeys(req: ExtendedRequestWithPageInfo, res: Response) { + const keys = makeContentSurrogateKeys({ + langCode: req.pageinfo.page?.languageCode || 'en', + relativePath: req.pageinfo.page?.relativePath, + }) + setFastlySurrogateKey(res, keys.join(' '), true) +} + // Keep Datadog metric tags consistent across Article API endpoints. // Datadog tags max at 200 characters, so path and source tags are truncated. // See https://docs.datadoghq.com/getting_started/tagging/#define-tags diff --git a/src/article-api/templates/webhooks-page.template.md b/src/article-api/templates/webhooks-page.template.md index fd1aaf10bdb4..ed282118a35a 100644 --- a/src/article-api/templates/webhooks-page.template.md +++ b/src/article-api/templates/webhooks-page.template.md @@ -41,17 +41,23 @@ Events below list only their additional parameters beyond these common ones. **Action type:** {% for actionType in webhook.actionTypes %}`{{ actionType }}`{% unless forloop.last %}, {% endunless %}{% endfor %} {% endif %} -{% if webhook.description %} -{{ webhook.description }} +{% for action in webhook.actions %} +{% if webhook.actions.size > 1 %} +#### `{{ action.name }}` {% endif %} -{% if webhook.bodyParameters.size > 0 %} -#### Webhook payload object parameters +{% if action.description %} +{{ action.description }} + +{% endif %} +{% if action.bodyParameters.size > 0 %} +{% if webhook.actions.size > 1 %}#####{% else %}####{% endif %} Webhook payload object parameters | Name | Type | Description | |------|------|-------------| -{% for param in webhook.bodyParameters %}| `{{ param.name }}` | `{{ param.type }}` | {% if param.isRequired %}**Required.** {% endif %}{{ param.description }} | +{% for param in action.bodyParameters %}| `{{ param.name }}` | `{{ param.type }}` | {% if param.isRequired %}**Required.** {% endif %}{{ param.description }} | {% endfor %} {% endif %} {% endfor %} +{% endfor %} diff --git a/src/article-api/tests/article-body.ts b/src/article-api/tests/article-body.ts index 3e097f04f0ed..2a0461a11499 100644 --- a/src/article-api/tests/article-body.ts +++ b/src/article-api/tests/article-body.ts @@ -23,6 +23,14 @@ describe('article body api', () => { expect(res.headers['content-type']).toContain('text/markdown') }) + test('sets the page surrogate key that deploy purges use', async () => { + const res = await get(makeURL('/en/get-started/start-your-journey/hello-world')) + expect(res.statusCode).toBe(200) + const keys = res.headers['surrogate-key'].split(/\s/g) + expect(keys).toContain('language:en') + expect(keys).toContain('language:en,path:get-started/start-your-journey/hello-world.md') + }) + test('body includes title and intro', async () => { const res = await get(makeURL('/en/get-started/start-your-journey/hello-world')) expect(res.statusCode).toBe(200) diff --git a/src/article-api/tests/pageinfo.ts b/src/article-api/tests/pageinfo.ts index 2fac904119a1..00d34d71be2c 100644 --- a/src/article-api/tests/pageinfo.ts +++ b/src/article-api/tests/pageinfo.ts @@ -1,7 +1,10 @@ import { beforeAll, describe, expect, test } from 'vitest' import { get } from '@/tests/helpers/e2etest' -import { makeLanguageSurrogateKey } from '@/frame/middleware/set-fastly-surrogate-key' +import { + makeLanguageSurrogateKey, + makePageSurrogateKey, +} from '@/frame/middleware/set-fastly-surrogate-key' import { latest } from '@/versions/lib/enterprise-server-releases' const makeURL = (pathname: string): string => @@ -51,7 +54,10 @@ describe('pageinfo api', () => { expect(res.headers['cache-control']).toMatch(/max-age=[1-9]/) expect(res.headers['surrogate-control']).toContain('public') expect(res.headers['surrogate-control']).toMatch(/max-age=[1-9]/) - expect(res.headers['surrogate-key']).toBe(makeLanguageSurrogateKey('en')) + // The page key must match the purge key derived from the source file. + expect(res.headers['surrogate-key']).toBe( + `${makeLanguageSurrogateKey('en')} ${makePageSurrogateKey('en', 'get-started/start-your-journey/index.md')}`, + ) }) // Fullwidth colons make Japanese frontmatter scalar; English fallback prevents empty titles. @@ -62,6 +68,17 @@ describe('pageinfo api', () => { expect(meta.title).toBe('Broken frontmatter translation fallback') }) + // Japanese frontmatter has an empty title and a numeric intro, so each key falls back to English. + test('mistyped translation frontmatter values fall back to English', async () => { + const res = await get(makeURL('/ja/get-started/foo/mistyped-frontmatter-translation')) + expect(res.statusCode).toBe(200) + const meta = JSON.parse(res.body) as PageMetadata + expect(meta.title).toBe('Mistyped frontmatter translation fallback') + expect(meta.intro).toBe( + 'Demonstrates falling back to English when a translated frontmatter value has the wrong type.', + ) + }) + test('a pathname that does not exist', async () => { const res = await get(makeURL('/en/never/heard/of')) expect(res.statusCode).toBe(404) diff --git a/src/article-api/tests/secret-scanning-transformer.test.ts b/src/article-api/tests/secret-scanning-transformer.test.ts index 8ec8886c2400..cd0717355cc2 100644 --- a/src/article-api/tests/secret-scanning-transformer.test.ts +++ b/src/article-api/tests/secret-scanning-transformer.test.ts @@ -1,4 +1,5 @@ import { describe, expect, test, vi, beforeEach } from 'vitest' +import octicons from '@primer/octicons' import { SecretScanningTransformer } from '@/article-api/transformers/secret-scanning-transformer' import shortVersionsMiddleware from '@/versions/middleware/short-versions' @@ -76,3 +77,31 @@ describe('SecretScanningTransformer Liquid evaluation', () => { expect(context.secretScanningData![0].hasExtendedMetadata).toBe(true) }) }) + +describe('SecretScanningTransformer icon replacement', () => { + const transformer = new SecretScanningTransformer() + + beforeEach(() => { + vi.clearAllMocks() + }) + + test('replaces octicon SVGs with check and cross marks', async () => { + vi.mocked(getSecretScanningData).mockResolvedValue([]) + const supported = octicons.check.toSVG({ 'aria-label': 'Supported' }) + const notSupported = octicons.x.toSVG({ 'aria-label': 'Not supported' }) + const page = { + ...stubPage, + render: vi + .fn() + .mockResolvedValue( + `| User alerts | ${supported} | ${notSupported} | ${supported} (if partner) |`, + ), + } as unknown as Page + const context = await buildContext('free-pro-team@latest') + + const result = await transformer.transform(page, '/test', context) + + expect(result).toContain('| User alerts | ✓ | ✗ | ✓ (if partner) |') + expect(result).not.toContain(' { expect(result).toContain('**ForkEvent**') }) + test('nested array with oneOf items lists each variant', () => { + const result = summarizeSchema({ + type: 'object', + required: ['labels'], + properties: { + labels: { + type: 'array', + items: { + oneOf: [ + { type: 'string' }, + { type: 'object', properties: { name: { type: 'string' } } }, + ], + }, + }, + }, + }) + expect(result).toContain('* `labels`: required, array of items:') + expect(result).toContain('one of:') + expect(result).toContain('**string**') + expect(result).toContain('* `name`: string') + expect(result).not.toContain('array of object') + }) + + test('nested array with titled composition items keeps the title and variants', () => { + const result = summarizeSchema({ + type: 'object', + properties: { + rules: { + type: 'array', + items: { + title: 'Repository Rule', + type: 'object', + oneOf: [ + { title: 'creation', type: 'object', properties: { type: { type: 'string' } } }, + { title: 'deletion', type: 'object', properties: { type: { type: 'string' } } }, + ], + }, + }, + }, + }) + expect(result).toContain('* `rules`: array of `Repository Rule`:') + expect(result).toContain('**creation**') + expect(result).toContain('**deletion**') + expect(result).not.toContain('array of object') + }) + + test('depth-limited composition items do not block later expansion', () => { + const rules = { + type: 'array', + items: { + title: 'Rule', + oneOf: [ + { title: 'creation', type: 'object', properties: { ruleField: { type: 'string' } } }, + ], + }, + } + const result = summarizeSchema({ + type: 'object', + properties: { + a: { + type: 'object', + properties: { + b: { + type: 'object', + properties: { + c: { + type: 'object', + properties: { d: { type: 'object', properties: { rules } } }, + }, + }, + }, + }, + }, + later: rules, + again: rules, + }, + }) + expect(result.match(/`ruleField`/g)).toHaveLength(1) + expect(result).toContain('* `later`: array of `Rule`:') + expect(result).toContain('* `again`: array of `Rule` (see above)') + }) + + test('nested array items keep shared properties alongside composition variants', () => { + const result = summarizeSchema({ + type: 'object', + properties: { + items: { + type: 'array', + items: { + type: 'object', + required: ['id'], + properties: { id: { type: 'integer' } }, + allOf: [{ type: 'object', properties: { extra: { type: 'string' } } }], + }, + }, + }, + }) + expect(result).toContain('* `items`: array of items:') + expect(result).toContain(' * `id`: required, integer') + expect(result).toContain('all of:') + expect(result).toContain('* `extra`: string') + }) + test('nested objects recurse with title', () => { const result = summarizeSchema({ type: 'object', diff --git a/src/article-api/tests/webhooks-transformer.test.ts b/src/article-api/tests/webhooks-transformer.test.ts new file mode 100644 index 000000000000..56a1d7b1ecd0 --- /dev/null +++ b/src/article-api/tests/webhooks-transformer.test.ts @@ -0,0 +1,83 @@ +import { beforeAll, describe, expect, test, vi } from 'vitest' + +import { WebhooksTransformer } from '@/article-api/transformers/webhooks-transformer' +import { getWebhooks } from '@/webhooks/lib/index' +import { allVersions } from '@/versions/lib/all-versions' +import type { Context, Page } from '@/types' + +vi.mock('@/webhooks/lib/index', async (importOriginal) => { + const actual = await importOriginal() + return { ...actual, getWebhooks: vi.fn(actual.getWebhooks) } +}) + +const stubPage = { + autogenerated: 'webhooks', + title: 'Webhook events and payloads', + intro: '', + renderProp: async () => '', +} as unknown as Page + +function getSection(body: string, webhookName: string): string { + return body.split('\n## ').find((section) => section.startsWith(`${webhookName}\n`)) || '' +} + +const currentVersion = 'free-pro-team@latest' +const context = { + currentVersion, + currentVersionObj: allVersions[currentVersion], + allVersions, +} as unknown as Context + +function transform(): Promise { + return new WebhooksTransformer().transform( + stubPage, + '/en/webhooks/webhook-events-and-payloads', + context, + ) +} + +describe('WebhooksTransformer actions', () => { + let body = '' + let initialLoads = 0 + + beforeAll(async () => { + body = await transform() + initialLoads = vi.mocked(getWebhooks).mock.calls.length + }) + + test('renders the description and parameters of every action', () => { + const checkRun = getSection(body, 'check_run') + expect(checkRun).toContain('#### `completed`') + expect(checkRun).toContain('A check run was completed') + expect(checkRun).toContain('#### `requested_action`') + expect(checkRun).toContain('someone requested a followup action') + + const requestedAction = checkRun.split('#### `requested_action`')[1].split('\n#### `')[0] + expect(requestedAction).toMatch(/\|\s*`requested_action`\s*\|/) + }) + + test('keeps action-specific parameters under their own action', () => { + const pullRequest = getSection(body, 'pull_request') + expect(pullRequest).toContain('#### `synchronize`') + + const edited = pullRequest.split('#### `edited`')[1].split('\n#### `')[0] + expect(edited).toMatch(/\|\s*`changes`\s*\|/) + + const assigned = pullRequest.split('#### `assigned`')[1].split('\n#### `')[0] + expect(assigned).not.toMatch(/\|\s*`changes`\s*\|/) + }) + + test('single-action webhooks have no action subsection', () => { + const push = getSection(body, 'push') + expect(push).not.toContain('#### `default`') + expect(push).toContain('#### Webhook payload object parameters') + expect(push).toMatch(/\|\s*`commits`\s*\|/) + }) + + test('reuses prepared webhook data for the same version', async () => { + expect(initialLoads).toBe(1) + vi.mocked(getWebhooks).mockClear() + expect(await transform()).toBe(body) + expect(getWebhooks).not.toHaveBeenCalled() + }) +}) diff --git a/src/article-api/transformers/secret-scanning-transformer.ts b/src/article-api/transformers/secret-scanning-transformer.ts index 3375c68096c5..351dae50196b 100644 --- a/src/article-api/transformers/secret-scanning-transformer.ts +++ b/src/article-api/transformers/secret-scanning-transformer.ts @@ -99,8 +99,12 @@ export class SecretScanningTransformer implements PageTransformer { content = stripHtmlComments(content) - content = content.replace(/]*aria-label="Supported"[^>]*>[^<]*<\/span>/g, '✓') - content = content.replace(/]*aria-label="Unsupported"[^>]*>[^<]*<\/span>/g, '✗') + // The octicon Liquid tag renders an with nested elements. + content = content.replace(/<(svg|span)\b[^>]*aria-label="Supported"[^>]*>[\s\S]*?<\/\1>/g, '✓') + content = content.replace( + /<(svg|span)\b[^>]*aria-label="(?:Not supported|Unsupported)"[^>]*>[\s\S]*?<\/\1>/g, + '✗', + ) content = content.replace(//gi, '\n') content = content.replace(/]*>([^<]*)<\/a>/gi, '[$2]($1)') // Repeat stripping handles nested tags like ipt>; the limit stops loops. diff --git a/src/article-api/transformers/webhooks-transformer.ts b/src/article-api/transformers/webhooks-transformer.ts index a47cef345dff..3cdc7c7c06ac 100644 --- a/src/article-api/transformers/webhooks-transformer.ts +++ b/src/article-api/transformers/webhooks-transformer.ts @@ -5,6 +5,91 @@ import { fastTextOnly } from '@/content-render/unified/text-only' import { loadTemplate } from '@/article-api/lib/load-template' import matter from '@gr2m/gray-matter' +type PreparedWebhooks = Awaited> + +// Each version scans every webhook category, so reuse the prepared result across requests. +const preparedWebhooksCache = new Map>() + +async function getPreparedWebhooks(currentVersion: string): Promise { + let prepared = preparedWebhooksCache.get(currentVersion) + if (!prepared) { + prepared = prepareWebhooks(currentVersion) + preparedWebhooksCache.set(currentVersion, prepared) + } + try { + return await prepared + } catch (error) { + // Drop failed loads so a later request can retry. + if (preparedWebhooksCache.get(currentVersion) === prepared) { + preparedWebhooksCache.delete(currentVersion) + } + throw error + } +} + +async function prepareWebhooks(currentVersion: string) { + // Dynamic import avoids circular dependencies. + const { getWebhooks } = await import('@/webhooks/lib/index') + const webhooksData = await getWebhooks(currentVersion) + + // Omit payload examples so the generated webhook page stays compact. + const preparedWebhooks = Object.entries(webhooksData).map(([name, webhook]) => { + const actionTypes = Object.keys(webhook) + const firstAction = webhook[actionTypes[0]] || {} + return { + name, + actionTypes, + summary: firstAction.summaryHtml ? fastTextOnly(firstAction.summaryHtml) : '', + availability: firstAction.availability || [], + actions: actionTypes.map((actionType) => { + const data = webhook[actionType] + return { + name: actionType, + description: data.descriptionHtml ? fastTextOnly(data.descriptionHtml) : '', + bodyParameters: (data.bodyParameters || []).map((param) => ({ + name: param.name || '', + type: param.type || '', + description: param.description + ? fastTextOnly(param.description).replace(/\n/g, ' ').trim() + : '', + isRequired: param.isRequired || false, + })), + } + }), + } + }) + + // Common body parameters appear in at least 60 percent of webhooks, in any action. + const paramCounts = new Map() + for (const webhook of preparedWebhooks) { + const names = new Set( + webhook.actions.flatMap((action) => action.bodyParameters.map((param) => param.name)), + ) + for (const name of names) { + paramCounts.set(name, (paramCounts.get(name) || 0) + 1) + } + } + const threshold = preparedWebhooks.length * 0.6 + const commonParamNames = new Set( + [...paramCounts.entries()].filter(([, count]) => count >= threshold).map(([name]) => name), + ) + + // Omit high-frequency body parameters from rows; summarize matches from the first webhook. + const commonParams = + preparedWebhooks[0]?.actions[0]?.bodyParameters.filter((p: { name: string }) => + commonParamNames.has(p.name), + ) || [] + for (const webhook of preparedWebhooks) { + for (const action of webhook.actions) { + action.bodyParameters = action.bodyParameters.filter( + (p: { name: string }) => !commonParamNames.has(p.name), + ) + } + } + + return { preparedWebhooks, commonParams } +} + export class WebhooksTransformer implements PageTransformer { templateName = 'webhooks-page.template.md' @@ -13,12 +98,7 @@ export class WebhooksTransformer implements PageTransformer { } async transform(page: Page, pathname: string, context: Context): Promise { - // Dynamic import avoids circular dependencies. - const { getInitialPageWebhooks } = await import('@/webhooks/lib/index') - - const currentVersion = context.currentVersion! - - const webhooksData = await getInitialPageWebhooks(currentVersion) + const { preparedWebhooks, commonParams } = await getPreparedWebhooks(context.currentVersion!) const intro = page.intro ? await page.renderProp('intro', context, { textOnly: true }) : '' @@ -40,46 +120,6 @@ export class WebhooksTransformer implements PageTransformer { } } - // Omit payload examples so the generated webhook page stays compact. - const preparedWebhooks = webhooksData.map((webhook) => ({ - name: webhook.name, - actionTypes: webhook.actionTypes, - summary: webhook.data.summaryHtml ? fastTextOnly(webhook.data.summaryHtml) : '', - description: webhook.data.descriptionHtml ? fastTextOnly(webhook.data.descriptionHtml) : '', - availability: webhook.data.availability || [], - bodyParameters: (webhook.data.bodyParameters || []).map((param) => ({ - name: param.name || '', - type: param.type || '', - description: param.description - ? fastTextOnly(param.description).replace(/\n/g, ' ').trim() - : '', - isRequired: param.isRequired || false, - })), - })) - - // Common body parameters appear in at least 60 percent of webhooks. - const paramCounts = new Map() - for (const webhook of preparedWebhooks) { - for (const param of webhook.bodyParameters) { - paramCounts.set(param.name, (paramCounts.get(param.name) || 0) + 1) - } - } - const threshold = preparedWebhooks.length * 0.6 - const commonParamNames = new Set( - [...paramCounts.entries()].filter(([, count]) => count >= threshold).map(([name]) => name), - ) - - // Omit high-frequency body parameters from rows; summarize matches from the first webhook. - const commonParams = - preparedWebhooks[0]?.bodyParameters.filter((p: { name: string }) => - commonParamNames.has(p.name), - ) || [] - for (const webhook of preparedWebhooks) { - webhook.bodyParameters = webhook.bodyParameters.filter( - (p: { name: string }) => !commonParamNames.has(p.name), - ) - } - const templateData: Record = { page: { title: page.title, diff --git a/src/assets/scripts/deleted-assets-pr-comment.ts b/src/assets/scripts/deleted-assets-pr-comment.ts index 88626e24875b..c31567b92088 100755 --- a/src/assets/scripts/deleted-assets-pr-comment.ts +++ b/src/assets/scripts/deleted-assets-pr-comment.ts @@ -59,7 +59,8 @@ async function main({ owner, repo, baseSHA, headSHA }: MainArgs) { return '' } - let markdown = '**Please restore deleted assets**\n\n' + // dont-delete-assets.yml searches for this marker to update its earlier comment instead of posting a new one. + let markdown = '\n**Please restore deleted assets**\n\n' markdown += "Even if you don't reference these assets anymore, as of this branch, please do not delete them.\n" markdown += 'They might still be referenced in translated content.\n' diff --git a/src/audit-logs/lib/version-reference-links.ts b/src/audit-logs/lib/version-reference-links.ts new file mode 100644 index 000000000000..583aadbdf6ef --- /dev/null +++ b/src/audit-logs/lib/version-reference-links.ts @@ -0,0 +1,40 @@ +import getRedirect from '@/redirects/lib/get-redirect' +import nonEnterpriseDefaultVersion from '@/versions/lib/non-enterprise-default-version' + +import type { Context } from '@/types' + +type Options = { + language: string + currentVersion: string + hasPage: (href: string) => boolean +} + +// Upstream reference links have no language or version, so readers on other versions +// would land on the default version. Links to pages missing from the current version +// stay unchanged so they still redirect to a version that has the page. +export function versionReferenceLinks( + links: string, + { language, currentVersion, hasPage }: Options, +): string { + if (currentVersion === nonEnterpriseDefaultVersion) return links + + return links.replace(/[^,\s]+/g, (link) => { + if (link === 'N/A' || link.startsWith('http')) return link + const hashIndex = link.indexOf('#') + const pathname = hashIndex === -1 ? link : link.slice(0, hashIndex) + const hash = hashIndex === -1 ? '' : link.slice(hashIndex) + const versioned = `/${language}/${currentVersion}${pathname.startsWith('/') ? '' : '/'}${pathname}` + return hasPage(versioned) ? `${versioned}${hash}` : link + }) +} + +// A redirect alias counts only when it resolves inside the current version, because a +// cross-version redirect would move the reader off their version anyway. +export function createHasPage(context: Context, language: string, currentVersion: string) { + const pages = context.pages || {} + const versionPrefix = `/${language}/${currentVersion}/` + return (href: string) => { + const target = href in pages ? href : getRedirect(href, context) + return Boolean(target && target.startsWith(versionPrefix) && target in pages) + } +} diff --git a/src/audit-logs/pages/audit-log-events.tsx b/src/audit-logs/pages/audit-log-events.tsx index 9389b6be21e0..0031721f6e3a 100644 --- a/src/audit-logs/pages/audit-log-events.tsx +++ b/src/audit-logs/pages/audit-log-events.tsx @@ -61,6 +61,7 @@ export default function AuditLogEvents({ export const getServerSideProps: GetServerSideProps = async (context) => { const { getAutomatedPageMiniTocItems } = await import('@/frame/lib/get-mini-toc-items') const { getCategorizedAuditLogEvents, getCategoryNotes } = await import('../lib') + const { createHasPage, versionReferenceLinks } = await import('../lib/version-reference-links') const req = context.req as unknown as ExtendedRequest const res = context.res as unknown as Response @@ -82,6 +83,27 @@ export const getServerSideProps: GetServerSideProps = async (context) => auditLogEvents = getCategorizedAuditLogEvents('organization', currentVersion) } + const language = req.context!.currentLanguage! + const hasPage = createHasPage(req.context!, language, currentVersion) + // Copy events because every request and language shares the cached categorized events. + auditLogEvents = Object.fromEntries( + Object.entries(auditLogEvents).map(([category, events]) => [ + category, + events.map((event) => + event.docs_reference_links + ? { + ...event, + docs_reference_links: versionReferenceLinks(event.docs_reference_links, { + language, + currentVersion, + hasPage, + }), + } + : event, + ), + ]), + ) + const categoryNotes = getCategoryNotes() const auditLogEventsMiniTocs = await getAutomatedPageMiniTocItems( diff --git a/src/audit-logs/tests/unit/version-reference-links.ts b/src/audit-logs/tests/unit/version-reference-links.ts new file mode 100644 index 000000000000..9ab1c735490f --- /dev/null +++ b/src/audit-logs/tests/unit/version-reference-links.ts @@ -0,0 +1,124 @@ +import { describe, expect, test } from 'vitest' + +import { createHasPage, versionReferenceLinks } from '@/audit-logs/lib/version-reference-links' +import type { Context } from '@/types' + +const ghesPages = new Set([ + '/en/enterprise-server@3.18/admin/monitoring-activity-in-your-enterprise/reviewing-audit-logs-for-your-enterprise/exporting-audit-log-activity-for-your-enterprise', + '/en/enterprise-server@3.18/organizations/managing-organization-settings/renaming-an-organization', +]) + +function hasPage(href: string) { + return ghesPages.has(href) +} + +const ghesOptions = { language: 'en', currentVersion: 'enterprise-server@3.18', hasPage } + +describe('versionReferenceLinks', () => { + test('adds language and version when the page exists in the current version', () => { + expect( + versionReferenceLinks( + '/organizations/managing-organization-settings/renaming-an-organization#about-renaming', + ghesOptions, + ), + ).toBe( + '/en/enterprise-server@3.18/organizations/managing-organization-settings/renaming-an-organization#about-renaming', + ) + }) + + test('adds a leading slash to links that have none', () => { + expect( + versionReferenceLinks( + 'admin/monitoring-activity-in-your-enterprise/reviewing-audit-logs-for-your-enterprise/exporting-audit-log-activity-for-your-enterprise', + ghesOptions, + ), + ).toBe( + '/en/enterprise-server@3.18/admin/monitoring-activity-in-your-enterprise/reviewing-audit-logs-for-your-enterprise/exporting-audit-log-activity-for-your-enterprise', + ) + }) + + test('keeps the original link when the page is missing from the current version', () => { + const link = '/billing/managing-the-plan-for-your-github-account/about-billing-for-plans' + expect(versionReferenceLinks(link, ghesOptions)).toBe(link) + }) + + test('rewrites each link in a list and keeps the separators', () => { + expect( + versionReferenceLinks( + '/organizations/managing-organization-settings/renaming-an-organization, /billing/managing-the-plan-for-your-github-account/about-billing-for-plans', + ghesOptions, + ), + ).toBe( + '/en/enterprise-server@3.18/organizations/managing-organization-settings/renaming-an-organization, /billing/managing-the-plan-for-your-github-account/about-billing-for-plans', + ) + }) + + test('leaves links unchanged for the default version', () => { + const link = '/organizations/managing-organization-settings/renaming-an-organization' + expect( + versionReferenceLinks(link, { + language: 'en', + currentVersion: 'free-pro-team@latest', + hasPage: () => true, + }), + ).toBe(link) + }) + + test('leaves absolute URLs unchanged', () => { + const link = + 'https://docs.github.com/en/authentication/securing-your-account-with-two-factor-authentication-2fa/configuring-two-factor-authentication' + expect(versionReferenceLinks(link, { ...ghesOptions, hasPage: () => true })).toBe(link) + }) +}) + +describe('createHasPage', () => { + const actionsPolicies = + '/admin/enforcing-policies/enforcing-policies-for-your-enterprise/enforcing-policies-for-github-actions-in-your-enterprise' + const actionsPoliciesAlias = + '/admin/policies/enforcing-policies-for-your-enterprise/enforcing-policies-for-github-actions-in-your-enterprise' + const renaming = '/organizations/managing-organization-settings/renaming-an-organization' + const cloudOnly = '/admin/managing-your-enterprise-account/cloud-only-article' + const brokenAlias = '/admin/old/redirect-to-missing-page' + + const context = { + userLanguage: 'en', + pages: { + [`/en/enterprise-server@3.18${actionsPolicies}`]: {}, + [`/en/enterprise-server@3.18${renaming}`]: {}, + [`/en/enterprise-cloud@latest${cloudOnly}`]: {}, + }, + redirects: { + [`/enterprise-server@3.18${actionsPoliciesAlias}`]: `/enterprise-server@3.18${actionsPolicies}`, + [`/enterprise-server@3.18${cloudOnly}`]: `/enterprise-cloud@latest${cloudOnly}`, + [`/enterprise-server@3.18${brokenAlias}`]: '/enterprise-server@3.18/admin/missing-page', + }, + } as unknown as Context + const options = { + language: 'en', + currentVersion: 'enterprise-server@3.18', + hasPage: createHasPage(context, 'en', 'enterprise-server@3.18'), + } + + test('adds the current version to links for pages in that version', () => { + expect(versionReferenceLinks(renaming, options)).toBe(`/en/enterprise-server@3.18${renaming}`) + }) + + test('adds the current version to redirect aliases that stay in that version', () => { + expect(versionReferenceLinks(actionsPoliciesAlias, options)).toBe( + `/en/enterprise-server@3.18${actionsPoliciesAlias}`, + ) + }) + + test('keeps links whose redirect leaves the current version', () => { + expect(versionReferenceLinks(cloudOnly, options)).toBe(cloudOnly) + }) + + test('keeps links whose redirect target is not a page', () => { + expect(versionReferenceLinks(brokenAlias, options)).toBe(brokenAlias) + }) + + test('keeps links with no page and no redirect', () => { + const link = '/billing/managing-the-plan-for-your-github-account/about-billing-for-plans' + expect(versionReferenceLinks(link, options)).toBe(link) + }) +}) diff --git a/src/content-linter/lib/linting-rules/image-alt-text-length.ts b/src/content-linter/lib/linting-rules/image-alt-text-length.ts index 8c4477251600..64c6380ab8fd 100644 --- a/src/content-linter/lib/linting-rules/image-alt-text-length.ts +++ b/src/content-linter/lib/linting-rules/image-alt-text-length.ts @@ -18,8 +18,13 @@ export const incorrectAltTextLength = { tags: ['accessibility', 'images'], parser: 'markdownit', asynchronous: true, - function: (params: RuleParams, onError: RuleErrorCallback) => { - forEachInlineChild(params, 'image', async function forToken(token: ImageToken) { + function: async (params: RuleParams, onError: RuleErrorCallback) => { + const imageTokens: ImageToken[] = [] + forEachInlineChild(params, 'image', (token: ImageToken) => { + imageTokens.push(token) + }) + + for (const token of imageTokens) { let renderedString = token.content if (token.content.includes('{%') || token.content.includes('{{')) { @@ -27,7 +32,12 @@ export const incorrectAltTextLength = { currentLanguage: 'en', currentVersionObj: allVersions['free-pro-team@latest'], } - renderedString = await liquid.parseAndRender(token.content, context) + try { + renderedString = await liquid.parseAndRender(token.content, context) + } catch { + // GHD014 and GHD018 report bad data references and Liquid syntax errors. + continue + } } // Empty alt text cannot produce a range because getRange requires a string it can find. @@ -43,6 +53,6 @@ export const incorrectAltTextLength = { null, // No fix possible ) } - }) + } }, } diff --git a/src/content-linter/lib/linting-rules/third-party-action-pinning.ts b/src/content-linter/lib/linting-rules/third-party-action-pinning.ts index 8e883466e0a3..b98c642b8b11 100644 --- a/src/content-linter/lib/linting-rules/third-party-action-pinning.ts +++ b/src/content-linter/lib/linting-rules/third-party-action-pinning.ts @@ -32,24 +32,33 @@ export const thirdPartyActionPinning: Rule = { tags: ['feature', 'actions'], parser: 'markdownit', asynchronous: true, - function: (params: RuleParams, onError: RuleErrorCallback) => { - filterTokens(params, 'fence', async (token: MarkdownToken) => { - if (!token.info || !token.content) return + function: async (params: RuleParams, onError: RuleErrorCallback) => { + const fences: MarkdownToken[] = [] + filterTokens(params, 'fence', (token: MarkdownToken) => { + fences.push(token) + }) + for (const token of fences) { + if (!token.info || !token.content) continue const lang = token.info.trim().split(/\s+/u).shift()?.toLowerCase() - if (lang !== 'yaml' && lang !== 'yml') return - if (!token.content.includes('steps:')) return - if (!token.content.includes('uses:')) return + if (lang !== 'yaml' && lang !== 'yml') continue + if (!token.content.includes('uses:')) continue const context = { currentLanguage: 'en', currentVersionObj: allVersions['free-pro-team@latest'], } - // Parse Liquid first because yaml loading chokes on {% raw %} tags. - const renderedYaml = await liquid.parseAndRender(token.content, context) + let renderedYaml: string + try { + // Parse Liquid first because yaml loading chokes on {% raw %} tags. + renderedYaml = await liquid.parseAndRender(token.content, context) + } catch { + // Other rules report Liquid errors, and a fence inside an outer {% raw %} can fail to render alone. + continue + } try { - const yamlObj = load(renderedYaml) as WorkflowYaml + const yamlObj = load(renderedYaml) as WorkflowYaml | WorkflowStep[] const steps = getWorkflowSteps(yamlObj) - if (!steps.some((step) => step.uses)) return + if (!steps.some((step) => step.uses)) continue for (const step of steps) { if (step.uses) { @@ -77,12 +86,14 @@ export const thirdPartyActionPinning: Rule = { console.error('Error parsing YAML:', e) } } - }) + } }, } -function getWorkflowSteps(yamlObj: WorkflowYaml): WorkflowStep[] { - if (yamlObj?.jobs) { +function getWorkflowSteps(yamlObj: WorkflowYaml | WorkflowStep[]): WorkflowStep[] { + if (Array.isArray(yamlObj)) { + return yamlObj.filter((step) => step && typeof step === 'object') + } else if (yamlObj?.jobs) { const jobs = Object.values(yamlObj.jobs) return jobs.flatMap((job) => job.steps || []) } else if (yamlObj?.steps) { diff --git a/src/content-linter/lib/linting-rules/yaml-scheduled-jobs.ts b/src/content-linter/lib/linting-rules/yaml-scheduled-jobs.ts index 514ff512432e..b30e4529d1c2 100644 --- a/src/content-linter/lib/linting-rules/yaml-scheduled-jobs.ts +++ b/src/content-linter/lib/linting-rules/yaml-scheduled-jobs.ts @@ -24,26 +24,41 @@ export const yamlScheduledJobs: Rule = { tags: ['feature', 'actions'], parser: 'markdownit', asynchronous: true, - function: (params: RuleParams, onError: RuleErrorCallback) => { - filterTokens(params, 'fence', async (token: MarkdownToken) => { - if (!token.info) return - if (!token.content) return + function: async (params: RuleParams, onError: RuleErrorCallback) => { + const fences: MarkdownToken[] = [] + filterTokens(params, 'fence', (token: MarkdownToken) => { + fences.push(token) + }) + // Check fences in order so the module-level duplicate check is deterministic within a file. + for (const token of fences) { + if (!token.info) continue + if (!token.content) continue const lang = token.info.trim().split(/\s+/u).shift()?.toLowerCase() - if (lang !== 'yaml' && lang !== 'yml') return - if (!token.content.includes('schedule:')) return - if (!token.content.includes('- cron:')) return + if (lang !== 'yaml' && lang !== 'yml') continue + if (!token.content.includes('schedule:')) continue + if (!token.content.includes('- cron:')) continue const context = { currentLanguage: 'en', currentVersionObj: allVersions['free-pro-team@latest'], } - // Parse Liquid first because yaml loading chokes on {% raw %} tags. - const renderedYaml = await liquid.parseAndRender(token.content, context) - const yamlObj = load(renderedYaml) as YamlWorkflow - if (!yamlObj.on) return - if (!yamlObj.on.schedule) return + let yamlObj: YamlWorkflow | null | undefined + try { + // Parse Liquid first because yaml loading chokes on {% raw %} tags. + const renderedYaml = await liquid.parseAndRender(token.content, context) + yamlObj = load(renderedYaml) as YamlWorkflow | null | undefined + } catch (error) { + console.error( + `Skipping YAML fence that could not be parsed in ${params.name}:`, + error instanceof Error ? error.message : error, + ) + continue + } + const schedules = yamlObj?.on?.schedule + if (!Array.isArray(schedules)) continue - for (const schedule of yamlObj.on.schedule) { + for (const schedule of schedules) { + if (typeof schedule?.cron !== 'string') continue if (schedule.cron.split(' ')[0] === '0') { addError( onError, @@ -64,7 +79,7 @@ export const yamlScheduledJobs: Rule = { scheduledYamlJobs.push(schedule.cron) } - }) + } }, } diff --git a/src/content-linter/tests/unit/image-alt-text-length.ts b/src/content-linter/tests/unit/image-alt-text-length.ts index 5490d13bbf49..d94d412a1df5 100644 --- a/src/content-linter/tests/unit/image-alt-text-length.ts +++ b/src/content-linter/tests/unit/image-alt-text-length.ts @@ -40,4 +40,19 @@ describe(incorrectAltTextLength.names.join(' - '), () => { expect(errors[0].lineNumber).toBe(3) expect(errors[0].errorRange).toEqual(null) }) + test('image alt text with Liquid is checked after rendering', async () => { + const markdown = '![{% data variables.product.prodname_copilot_short %} icon.](./image.png)' + const result = await runRule(incorrectAltTextLength as Rule, { strings: { markdown } }) + const errors = result.markdown + expect(errors.length).toBe(1) + expect(errors[0].lineNumber).toBe(1) + expect(errors[0].errorDetail).toBe('Image alternate text is 13 characters long.') + }) + test('image alt text with a Liquid render error does not throw', async () => { + const markdown = + '![Screenshot of {% data variables.product.prodname_doesnotexist %} in the editor sidebar panel.](./image.png)' + const result = await runRule(incorrectAltTextLength as Rule, { strings: { markdown } }) + const errors = result.markdown + expect(errors.length).toBe(0) + }) }) diff --git a/src/content-linter/tests/unit/rai-app-card-structure.ts b/src/content-linter/tests/unit/rai-app-card-structure.ts index 516c4664a585..b07c90b2d733 100644 --- a/src/content-linter/tests/unit/rai-app-card-structure.ts +++ b/src/content-linter/tests/unit/rai-app-card-structure.ts @@ -1,8 +1,31 @@ +import fs from 'fs' +import path from 'path' + +import walk from 'walk-sync' import { describe, expect, test } from 'vitest' +import matter from '@/frame/lib/read-frontmatter' import { runRule } from '../../lib/init-test' import { raiAppCardStructure } from '../../lib/linting-rules/rai-app-card-structure' +// CI lints changed files only, so a templates.md edit would not otherwise recheck existing cards. +function findRaiCards(): string[] { + const contentDir = path.resolve('content') + // Early-access content is copied in from another repo during CI, so its drafts stay out of this check. + const files = walk(contentDir, { + globs: ['**/*.md'], + ignore: ['early-access/**'], + directories: false, + includeBasePath: true, + }) + return files.filter((file) => { + const markdown = fs.readFileSync(file, 'utf8') + if (!/contentType:\s*['"]?rai/.test(markdown)) return false + const { data } = matter(markdown) + return data?.contentType === 'rai' && !('children' in data) + }) +} + function validCard(): string { return [ '---', @@ -173,4 +196,15 @@ describe(raiAppCardStructure.names.join(' - '), () => { const errors = result.markdown expect(errors.some((e) => e.errorDetail?.includes('application-card-overreliance'))).toBe(true) }) + + // Reading every content file can exceed the 5 s default timeout on a cold checkout. + test('every published RAI card passes against the current template', async () => { + const cards = findRaiCards() + expect(cards.length).toBeGreaterThan(0) + const result = await runRule(raiAppCardStructure, { files: cards }) + const failures = Object.entries(result) + .filter(([, errors]) => errors.length > 0) + .map(([file, errors]) => [path.relative('.', file), errors.map((e) => e.errorDetail)]) + expect(Object.fromEntries(failures)).toEqual({}) + }, 120_000) }) diff --git a/src/content-linter/tests/unit/third-party-action-pinning.ts b/src/content-linter/tests/unit/third-party-action-pinning.ts index 03ae98198590..876fccdbe600 100644 --- a/src/content-linter/tests/unit/third-party-action-pinning.ts +++ b/src/content-linter/tests/unit/third-party-action-pinning.ts @@ -112,4 +112,66 @@ describe(thirdPartyActionPinning.names.join(' - '), () => { const errors = result.markdown expect(errors.length).toBe(1) }) + + test('should report an error for a bare list of steps', async () => { + const markdown = [ + '```yaml', + '- name: Sign in to Docker registry', + ' uses: docker/login-action@v3', + '```', + ].join('\n') + const result = await runRule(thirdPartyActionPinning, { strings: { markdown } }) + const errors = result.markdown + expect(errors.length).toBe(1) + expect(errors[0].lineNumber).toBe(3) + expect(errors[0].errorContext).toEqual('docker/login-action@v3') + }) + + test('should not report an error for a single mapping with uses', async () => { + const markdown = [ + '```yaml', + 'uses: fakeaction/publish@v3', + 'with:', + ' token: abc', + '```', + ].join('\n') + const result = await runRule(thirdPartyActionPinning, { strings: { markdown } }) + const errors = result.markdown + expect(errors.length).toBe(0) + }) + + test('should report an error in a code block that starts with a data tag', async () => { + const markdown = [ + '```yaml', + '{% data reusables.actions.actions-not-certified-by-github-comment %}', + 'steps:', + ' - uses: docker/login-action@v3', + '```', + ].join('\n') + const result = await runRule(thirdPartyActionPinning, { strings: { markdown } }) + const errors = result.markdown + expect(errors.length).toBe(1) + expect(errors[0].errorContext).toEqual('docker/login-action@v3') + }) + + test('should skip a code block that fails to render as Liquid', async () => { + const markdown = [ + '{% raw %}', + '```yaml', + 'steps:', + ' - run: echo ${{ a || b }}', + ' - uses: foo/bar@v1', + '```', + '{% endraw %}', + '', + '```yaml', + 'steps:', + ' - uses: monalisa/some-other-action@main', + '```', + ].join('\n') + const result = await runRule(thirdPartyActionPinning, { strings: { markdown } }) + const errors = result.markdown + expect(errors.length).toBe(1) + expect(errors[0].errorContext).toEqual('monalisa/some-other-action@main') + }) }) diff --git a/src/content-linter/tests/unit/yaml-scheduled-jobs.ts b/src/content-linter/tests/unit/yaml-scheduled-jobs.ts index cac559cec331..9a27f9871f56 100644 --- a/src/content-linter/tests/unit/yaml-scheduled-jobs.ts +++ b/src/content-linter/tests/unit/yaml-scheduled-jobs.ts @@ -81,6 +81,78 @@ describe(yamlScheduledJobs.names.join(' - '), () => { expect(errors.length).toBe(0) }) + test('yaml scheduled jobs on the hour with liquid report an error', async () => { + const markdown = [ + '```yaml', + 'on:', + ' schedule:', + " - cron: '0 5 * * *'", + 'jobs:', + ' build:', + ' runs-on: ubuntu-latest', + ' steps:', + ' - uses: {% data reusables.actions.action-checkout %}', + ' - run: echo {% raw %}${{ github.ref }}{% endraw %}', + '```', + ].join('\n') + const result = await runRule(yamlScheduledJobs, { strings: { markdown } }) + const errors = result.markdown + expect(errors.length).toBe(1) + expect(errors[0].lineNumber).toBe(4) + expect(errors[0].errorContext).toEqual('0 5 * * *') + }) + + test('yaml scheduled jobs that cannot be parsed are skipped', async () => { + const markdown = [ + '```yaml', + 'on:', + ' schedule:', + " - cron: '30 5 * * 1'", + ' ...', + '```', + ].join('\n') + const result = await runRule(yamlScheduledJobs, { strings: { markdown } }) + const errors = result.markdown + expect(errors.length).toBe(0) + }) + + test('yaml scheduled jobs that are not an array are skipped', async () => { + const markdown = [ + '```yaml', + 'on:', + ' schedule:', + " cron: '0 8 * * *'", + " # - cron: '0 8 * * *'", + '```', + '```yaml', + 'on:', + ' schedule:', + " - cron: '0 9 * * *'", + '```', + ].join('\n') + const result = await runRule(yamlScheduledJobs, { strings: { markdown } }) + const errors = result.markdown + expect(errors.length).toBe(1) + expect(errors[0].lineNumber).toBe(10) + expect(errors[0].errorContext).toEqual('0 9 * * *') + }) + + test('yaml scheduled jobs with an empty cron are skipped', async () => { + const markdown = [ + '```yaml', + 'on:', + ' schedule:', + ' - cron:', + " - cron: '0 7 * * *'", + '```', + ].join('\n') + const result = await runRule(yamlScheduledJobs, { strings: { markdown } }) + const errors = result.markdown + expect(errors.length).toBe(1) + expect(errors[0].lineNumber).toBe(5) + expect(errors[0].errorContext).toEqual('0 7 * * *') + }) + test('yaml scheduled jobs without on:', async () => { const markdown = [ '```yaml', diff --git a/src/content-render/lib/content-type.ts b/src/content-render/lib/content-type.ts new file mode 100644 index 000000000000..c48269550f42 --- /dev/null +++ b/src/content-render/lib/content-type.ts @@ -0,0 +1,50 @@ +import path from 'path' +import { contentTypesEnum } from '@/frame/lib/frontmatter' + +const RESPONSIBLE_USE_STRING = 'responsible-use' +const LANDING_TYPE = 'landing' +const RAI_TYPE = 'rai' +const OTHER_TYPE = 'other' + +export function determineContentType(relativePath: string): string { + // For copilot/how-tos/troubleshoot/index.md, pathSegments[1] is the content type. + const pathSegments = relativePath.split(path.sep) + + const topLevelDirectory = pathSegments[0] + const derivedContentType = pathSegments[1] + + // content/index.md is the only homepage. + if (topLevelDirectory === 'index.md') return 'homepage' + + // Responsible-use directories map to the rai content type. + if (derivedContentType.includes(RESPONSIBLE_USE_STRING)) { + return RAI_TYPE + } + + // getting-started directories map to get-started. + if (derivedContentType === 'getting-started') { + return 'get-started' + } + + // Directories matching contentTypesEnum map to their content type. + if (contentTypesEnum.includes(derivedContentType)) { + return derivedContentType + } + + // Product index.md files are landing pages. + if (derivedContentType === 'index.md') { + return LANDING_TYPE + } + + return OTHER_TYPE +} + +// These values depend on the file's location, so they are stale once the path no longer derives them. +const LOCATION_SPECIFIC_TYPES = ['homepage', LANDING_TYPE, RAI_TYPE] + +// Topic directories derive 'other', so a curated contentType there is kept instead of downgraded. +export function shouldWriteContentType(existing: string | undefined, derived: string): boolean { + if (!existing) return true + if (existing === derived) return false + return derived !== OTHER_TYPE || LOCATION_SPECIFIC_TYPES.includes(existing) +} diff --git a/src/content-render/scripts/add-content-type.ts b/src/content-render/scripts/add-content-type.ts index 0f5925e55768..dca7f2f85230 100644 --- a/src/content-render/scripts/add-content-type.ts +++ b/src/content-render/scripts/add-content-type.ts @@ -7,13 +7,9 @@ import { program } from 'commander' import frontmatter from '@/frame/lib/read-frontmatter' import walkFiles from '@/workflows/walk-files' import { contentTypesEnum } from '@/frame/lib/frontmatter' +import { determineContentType, shouldWriteContentType } from '@/content-render/lib/content-type' import type { MarkdownFrontmatter } from '@/types' -const RESPONSIBLE_USE_STRING = 'responsible-use' -const LANDING_TYPE = 'landing' -const RAI_TYPE = 'rai' -const OTHER_TYPE = 'other' - interface ScriptOptions { dryRun?: boolean paths?: string[] @@ -104,8 +100,9 @@ function processFile(filePath: string, scriptOptions: ScriptOptions) { return { processed: true, updated: false } } - const isChangingContentType = data.contentType && data.contentType !== newContentType const isAddingContentType = !data.contentType + const isChangingContentType = + !isAddingContentType && shouldWriteContentType(data.contentType, newContentType) if (isChangingContentType) { console.log( @@ -140,39 +137,6 @@ function processFile(filePath: string, scriptOptions: ScriptOptions) { return { processed: true, updated: true } } -function determineContentType(relativePath: string): string { - // For copilot/how-tos/troubleshoot/index.md, pathSegments[1] is the content type. - const pathSegments = relativePath.split(path.sep) - - const topLevelDirectory = pathSegments[0] - const derivedContentType = pathSegments[1] - - // content/index.md is the only homepage. - if (topLevelDirectory === 'index.md') return 'homepage' - - // Responsible-use directories map to the rai content type. - if (derivedContentType.includes(RESPONSIBLE_USE_STRING)) { - return RAI_TYPE - } - - // getting-started directories map to get-started. - if (derivedContentType === 'getting-started') { - return 'get-started' - } - - // Directories matching contentTypesEnum map to their content type. - if (contentTypesEnum.includes(derivedContentType)) { - return derivedContentType - } - - // Product index.md files are landing pages. - if (derivedContentType === 'index.md') { - return LANDING_TYPE - } - - return OTHER_TYPE -} - try { await main() } catch (error) { diff --git a/src/content-render/scripts/liquid-tags.ts b/src/content-render/scripts/liquid-tags.ts index 5f9aaac84914..4b204ae189d3 100644 --- a/src/content-render/scripts/liquid-tags.ts +++ b/src/content-render/scripts/liquid-tags.ts @@ -263,6 +263,11 @@ async function restoreReferences(options: ExpandOptions): Promise { process.exit(1) } + // Compare every file with pre-restore data so a later unedited copy cannot overwrite an edit. + const restorePaths: string[] = [] + const updates: ReturnType = [] + let hasAnyEdits = false + for (const filePath of paths) { try { if (verbose) { @@ -281,6 +286,7 @@ async function restoreReferences(options: ExpandOptions): Promise { const hasEdits = await detectContentEdits(content, verbose, allowedTypes) if (hasEdits) { + hasAnyEdits = true console.log( chalk.blue( `${filePath} contains expanded references; any edits made will be preserved in data files`, @@ -289,35 +295,47 @@ async function restoreReferences(options: ExpandOptions): Promise { if (!verbose) { console.log(chalk.dim(' Use --verbose to see details of the edits')) } + updates.push(...extractDataUpdates(content, allowedTypes)) + } + restorePaths.push(filePath) + } catch (error: unknown) { + console.error(chalk.red(`Error restoring ${filePath}: ${getErrorMessage(error)}`)) + } + } - // Write edited expanded blocks back to data files before restoring Liquid tags. - const updatedDataFiles = updateDataFiles(filePath, verbose, false, allowedTypes) - - // Restore updated data files so nested references return to Liquid tags too. - if (updatedDataFiles.length > 0) { - if (verbose) - console.log(chalk.blue(' Restoring updated data files back to liquid tags...')) - for (const dataFile of updatedDataFiles) { - try { - const dataContent = fs.readFileSync(dataFile, 'utf-8') - const restoredDataContent = restoreFileContent(dataContent, verbose, allowedTypes) - if (restoredDataContent !== dataContent) { - fs.writeFileSync(dataFile, restoredDataContent, 'utf-8') - if (verbose) { - const dataPath = convertFilePathToDataPath(dataFile) - console.log(chalk.green(` Restored: ${dataPath}`)) - } - } - } catch (error) { - if (verbose) { - const dataPath = convertFilePathToDataPath(dataFile) - console.log(chalk.yellow(` Could not restore ${dataPath}: ${error}`)) - } + if (hasAnyEdits) { + // Write edited expanded blocks back to data files before restoring Liquid tags. + const updatedDataFiles = updateDataFiles(updates, verbose, false) + + // Restore updated data files so nested references return to Liquid tags too. + if (updatedDataFiles.length > 0) { + if (verbose) console.log(chalk.blue(' Restoring updated data files back to liquid tags...')) + for (const dataFile of updatedDataFiles) { + try { + const dataContent = fs.readFileSync(dataFile, 'utf-8') + // Data files should never keep markers, whichever types were selected. + const restoredDataContent = restoreFileContent(dataContent, verbose) + if (restoredDataContent !== dataContent) { + fs.writeFileSync(dataFile, restoredDataContent, 'utf-8') + if (verbose) { + const dataPath = convertFilePathToDataPath(dataFile) + console.log(chalk.green(` Restored: ${dataPath}`)) } } + } catch (error) { + if (verbose) { + const dataPath = convertFilePathToDataPath(dataFile) + console.log(chalk.yellow(` Could not restore ${dataPath}: ${error}`)) + } } } + } + } + for (const filePath of restorePaths) { + try { + // Re-read so an input that is also an updated data file keeps its saved edit. + const content = fs.readFileSync(filePath, 'utf-8') // Restore the main file even when no data file changed. const restoredContent = restoreFileContent(content, verbose, allowedTypes) @@ -401,18 +419,17 @@ async function detectContentEdits( ): Promise { let hasEdits = false - let match - while ((match = EXPANDED_PATTERN.exec(content)) !== null) { - const [, type, dataPath, resolvedContent] = match + for (const [, type, dataPath, resolvedContent] of content.matchAll(EXPANDED_PATTERN)) { const refType = type as 'reusable' | 'variable' + const selected = !allowedTypes || allowedTypes.includes(refType) - if (!allowedTypes || allowedTypes.includes(refType)) { + if (selected) { try { const originalContent = loadDataValue(refType, dataPath.trim()) if (originalContent !== null) { - // Compare direct data file content to avoid false positives from nested resolution. - const currentContent = resolvedContent.trim() + // Nested expanded blocks must become Liquid tags again to match the raw data value. + const currentContent = restoreFileContent(resolvedContent).trim() if (currentContent !== originalContent.trim()) { hasEdits = true @@ -435,6 +452,11 @@ async function detectContentEdits( } } } + + // EXPANDED_PATTERN matches only the outermost block, and a selected block collapses all types. + if (await detectContentEdits(resolvedContent, verbose, selected ? undefined : allowedTypes)) { + hasEdits = true + } } return hasEdits @@ -480,7 +502,7 @@ function restoreFileContent( verbose?: boolean, allowedTypes?: Array<'reusable' | 'variable'>, ): string { - return content.replace(EXPANDED_PATTERN, (match, type, dataPath) => { + return content.replace(EXPANDED_PATTERN, (match, type, dataPath, resolvedContent) => { const refType = type as 'reusable' | 'variable' if (!allowedTypes || allowedTypes.includes(refType)) { @@ -493,19 +515,17 @@ function restoreFileContent( return originalLiquid } - return match + // Keep the unselected outer block, but restore selected blocks nested inside it. + const restoredInner = restoreFileContent(resolvedContent, verbose, allowedTypes) + return `${restoredInner}` }) } function updateDataFiles( - filePath: string, + updates: ReturnType, verbose?: boolean, dryRun?: boolean, - allowedTypes?: Array<'reusable' | 'variable'>, ): string[] { - const content = fs.readFileSync(filePath, 'utf8') - const updates = extractDataUpdates(content, allowedTypes) - if (updates.length === 0) { if (verbose) { console.log(chalk.yellow(' No content changes found')) @@ -547,31 +567,26 @@ function extractDataUpdates( ): Array<{ type: 'reusable' | 'variable'; path: string; newContent: string }> { const updates: Array<{ type: 'reusable' | 'variable'; path: string; newContent: string }> = [] - let match - while ((match = EXPANDED_PATTERN.exec(content)) !== null) { - const [, type, dataPath, resolvedContent] = match + for (const [, type, dataPath, resolvedContent] of content.matchAll(EXPANDED_PATTERN)) { const refType = type as 'reusable' | 'variable' + const selected = !allowedTypes || allowedTypes.includes(refType) - if (!allowedTypes || allowedTypes.includes(refType)) { - // Compare expanded blocks with their source before updating data files. + if (selected) { + // Restore nested blocks of every type so data files keep Liquid tags, not markers. + const newContent = restoreFileContent(resolvedContent).trim() try { const originalContent = loadDataValue(refType, dataPath.trim()) - if (originalContent !== null && resolvedContent.trim() !== originalContent.trim()) { - updates.push({ - type: refType, - path: dataPath.trim(), - newContent: resolvedContent.trim(), - }) + if (originalContent !== null && newContent !== originalContent.trim()) { + updates.push({ type: refType, path: dataPath.trim(), newContent }) } } catch { // Keep blocks on unexpected errors; unreadable files return null from loadDataValue. - updates.push({ - type: refType, - path: dataPath.trim(), - newContent: resolvedContent.trim(), - }) + updates.push({ type: refType, path: dataPath.trim(), newContent }) } } + + // EXPANDED_PATTERN matches only the outermost block, and a selected block collapses all types. + updates.push(...extractDataUpdates(resolvedContent, selected ? undefined : allowedTypes)) } return updates diff --git a/src/content-render/scripts/move-by-content-type.ts b/src/content-render/scripts/move-by-content-type.ts index 6f8798ebb998..a353533f0052 100644 --- a/src/content-render/scripts/move-by-content-type.ts +++ b/src/content-render/scripts/move-by-content-type.ts @@ -405,6 +405,7 @@ contentType: ${placeholderContentType} for (const productDir of productDirs) { const productName = path.basename(productDir) + const retainedChildren = new Set() try { const entries = await fs.readdir(productDir, { withFileTypes: true }) @@ -412,12 +413,44 @@ contentType: ${placeholderContentType} for (const entry of entries) { if (entry.isDirectory() && !validContentTypeDirs.has(entry.name)) { const oldDirPath = path.join(productDir, entry.name) + const relativeOldDirPath = path.relative(process.cwd(), oldDirPath) try { + // Unmoved articles and typed indexes would be lost; untyped category indexes go with the directory. + const remaining: string[] = [] + for (const file of walkFiles(oldDirPath, ['.md'], { includeEarlyAccess: true })) { + if (path.basename(file) !== 'index.md') { + remaining.push(file) + continue + } + const { data } = readFrontmatter(await fs.readFile(file, 'utf-8')) + if (!data || data.contentType) remaining.push(file) + } + if (remaining.length > 0) { + const examples = remaining + .slice(0, 3) + .map((file) => path.relative(process.cwd(), file)) + .join(', ') + console.log( + chalk.yellow( + `⚠ Not deleting ${relativeOldDirPath}: ${remaining.length} unmoved articles remain (${examples})`, + ), + ) + skipped.push({ + file: relativeOldDirPath, + reason: `Not deleted: ${remaining.length} unmoved articles remain`, + }) + try { + await fs.access(path.join(oldDirPath, 'index.md')) + retainedChildren.add(`/${entry.name}`) + } catch { + // A category without an index cannot be listed as a child. + } + continue + } + await fs.rm(oldDirPath, { recursive: true, force: true }) - console.log( - chalk.gray(`✓ Deleted old directory: ${path.relative(process.cwd(), oldDirPath)}`), - ) + console.log(chalk.gray(`✓ Deleted old directory: ${relativeOldDirPath}`)) if (!deletedByProduct.has(productName)) deletedByProduct.set(productName, []) deletedByProduct.get(productName)!.push(`/${productName}/${entry.name}`) @@ -446,7 +479,11 @@ contentType: ${placeholderContentType} } if (newChildren.length > 0) { - data.children = newChildren + // Retained categories stay reachable in the tree until their remaining articles move. + const keptChildren = Array.isArray(data.children) + ? data.children.filter((child: string) => retainedChildren.has(child)) + : [] + data.children = [...newChildren, ...keptChildren] updated = true } diff --git a/src/content-render/tests/content-type.ts b/src/content-render/tests/content-type.ts new file mode 100644 index 000000000000..70c0c59fa73b --- /dev/null +++ b/src/content-render/tests/content-type.ts @@ -0,0 +1,65 @@ +import path from 'path' +import { describe, expect, test } from 'vitest' +import { determineContentType, shouldWriteContentType } from '@/content-render/lib/content-type' + +describe('determineContentType', () => { + test('uses the content-type directory', () => { + expect(determineContentType(path.join('copilot', 'how-tos', 'troubleshoot', 'foo.md'))).toBe( + 'how-tos', + ) + expect(determineContentType(path.join('actions', 'concepts', 'foo.md'))).toBe('concepts') + }) + + test('maps getting-started and responsible-use directories', () => { + expect(determineContentType(path.join('copilot', 'getting-started', 'foo.md'))).toBe( + 'get-started', + ) + expect(determineContentType(path.join('copilot', 'responsible-use', 'foo.md'))).toBe('rai') + }) + + test('returns landing for product index pages and homepage for the root index', () => { + expect(determineContentType(path.join('admin', 'index.md'))).toBe('landing') + expect(determineContentType('index.md')).toBe('homepage') + }) + + test('returns other for topic directories', () => { + expect(determineContentType(path.join('admin', 'configuring-settings', 'foo.md'))).toBe('other') + }) +}) + +describe('shouldWriteContentType', () => { + test('adds a missing contentType', () => { + expect(shouldWriteContentType(undefined, 'other')).toBe(true) + expect(shouldWriteContentType(undefined, 'how-tos')).toBe(true) + }) + + test('keeps a curated contentType when the path does not imply one', () => { + const derived = determineContentType(path.join('admin', 'configuring-settings', 'foo.md')) + expect(shouldWriteContentType('how-tos', derived)).toBe(false) + expect(shouldWriteContentType('concepts', derived)).toBe(false) + }) + + test('replaces location-specific values when the path does not imply them', () => { + const derived = determineContentType(path.join('billing', 'sponsors', 'index.md')) + expect(shouldWriteContentType('landing', derived)).toBe(true) + expect(shouldWriteContentType('homepage', derived)).toBe(true) + expect(shouldWriteContentType('rai', derived)).toBe(true) + }) + + test('corrects a contentType that does not match its content-type directory', () => { + const derived = determineContentType(path.join('actions', 'how-tos', 'foo.md')) + expect(shouldWriteContentType('concepts', derived)).toBe(true) + expect(shouldWriteContentType('other', derived)).toBe(true) + }) + + test('corrects a product index page to landing', () => { + expect( + shouldWriteContentType('other', determineContentType(path.join('admin', 'index.md'))), + ).toBe(true) + }) + + test('leaves a matching contentType alone', () => { + expect(shouldWriteContentType('how-tos', 'how-tos')).toBe(false) + expect(shouldWriteContentType('other', 'other')).toBe(false) + }) +}) diff --git a/src/content-render/tests/liquid-tags.ts b/src/content-render/tests/liquid-tags.ts index 5151423349d5..24da0d1fed89 100644 --- a/src/content-render/tests/liquid-tags.ts +++ b/src/content-render/tests/liquid-tags.ts @@ -2,6 +2,7 @@ import { describe, test, expect, beforeEach, afterEach, vi } from 'vitest' import fs from 'fs/promises' import path from 'path' import { execSync } from 'child_process' +import { load } from 'js-yaml' const rootDir = path.join(__dirname, '../../..') const testContentDir = path.join(rootDir, 'content/test-integration') @@ -17,7 +18,10 @@ describe('liquid-tags script integration tests', () => { await fs.rm(testContentDir, { recursive: true, force: true }) }) - async function runScript(args: string): Promise<{ output: string; exitCode: number }> { + async function runScript( + args: string, + env: Record = {}, + ): Promise<{ output: string; exitCode: number }> { let output = '' let exitCode = 0 @@ -27,6 +31,7 @@ describe('liquid-tags script integration tests', () => { cwd: rootDir, stdio: 'pipe', timeout: 30000, + env: { ...process.env, ...env }, }) } catch (error: unknown) { const execError = error as { stdout?: string; stderr?: string; status?: number } @@ -88,4 +93,343 @@ This uses {% data variables.product.prodname_dotcom %} in content. expect(exitCode, `Help command failed with output: ${output}`).toBe(0) expect(output).toMatch(/resolve|usage|help|command/i) }) + + test('restore writes an edit in a later nested reusable to its data file', async () => { + // A fixture ROOT keeps a failing restore from rewriting the real data directory. + const fixtureRoot = path.join(testContentDir, 'root') + const outerFile = path.join(fixtureRoot, 'data/reusables/outer.md') + const innerFile = path.join(fixtureRoot, 'data/reusables/inner.md') + const yamlFile = path.join(fixtureRoot, 'data/variables/product.yml') + const outerContent = + 'Intro {% data variables.product.name %}.\n\n{% data reusables.inner %}\n\nOutro.\n' + const yamlContent = '# A comment that must survive\nname: Octo\n' + await fs.mkdir(path.dirname(outerFile), { recursive: true }) + await fs.mkdir(path.dirname(yamlFile), { recursive: true }) + await fs.writeFile(outerFile, outerContent) + await fs.writeFile(innerFile, 'Inner original text.\n') + await fs.writeFile(yamlFile, yamlContent) + const testFile = path.join(testContentDir, 'nested-edit.md') + const originalContent = '{% data reusables.outer %}\n\n{% data reusables.outer %}\n' + await fs.writeFile(testFile, originalContent) + const env = { ROOT: fixtureRoot } + + await runScript(`expand --paths "${testFile}"`, env) + const expandedContent = await fs.readFile(testFile, 'utf8') + expect(expandedContent).toContain('') + // Edit only the second copy, so an unedited first copy cannot win. + const secondInner = expandedContent.lastIndexOf('Inner original text.') + await fs.writeFile( + testFile, + expandedContent.slice(0, secondInner) + + expandedContent.slice(secondInner).replace('Inner original text.', 'Inner edited text.'), + ) + + const { output, exitCode } = await runScript(`restore --paths "${testFile}"`, env) + + expect(exitCode, `Restore script failed with output: ${output}`).toBe(0) + expect(await fs.readFile(testFile, 'utf8')).toBe(originalContent) + expect(await fs.readFile(innerFile, 'utf8')).toBe('Inner edited text.\n') + expect(await fs.readFile(outerFile, 'utf8')).toBe(outerContent) + expect(await fs.readFile(yamlFile, 'utf8')).toBe(yamlContent) + }) + + test('expand then restore leaves a variable that nests a variable unchanged', async () => { + const fixtureRoot = path.join(testContentDir, 'root') + const reusableFile = path.join(fixtureRoot, 'data/reusables/outer.md') + const yamlFile = path.join(fixtureRoot, 'data/variables/product.yml') + const reusableContent = 'Use {% data variables.product.UI_name %} here.\n' + const yamlContent = + "# A comment that must survive\nname: Octo\nUI_name: '{% data variables.product.name %}'\n" + await fs.mkdir(path.dirname(reusableFile), { recursive: true }) + await fs.mkdir(path.dirname(yamlFile), { recursive: true }) + await fs.writeFile(reusableFile, reusableContent) + await fs.writeFile(yamlFile, yamlContent) + const testFile = path.join(testContentDir, 'nested-variable.md') + const originalContent = '{% data reusables.outer %}\n' + await fs.writeFile(testFile, originalContent) + const env = { ROOT: fixtureRoot } + + await runScript(`expand --paths "${testFile}"`, env) + expect(await fs.readFile(testFile, 'utf8')).toContain('') + + const { output, exitCode } = await runScript(`restore --paths "${testFile}"`, env) + + expect(exitCode, `Restore script failed with output: ${output}`).toBe(0) + expect(output).not.toContain('contains expanded references') + expect(await fs.readFile(testFile, 'utf8')).toBe(originalContent) + expect(await fs.readFile(reusableFile, 'utf8')).toBe(reusableContent) + expect(await fs.readFile(yamlFile, 'utf8')).toBe(yamlContent) + }) + + test('restore keeps an edit when a later file has an unedited nested copy', async () => { + const fixtureRoot = path.join(testContentDir, 'root') + const outerFile = path.join(fixtureRoot, 'data/reusables/outer.md') + const innerFile = path.join(fixtureRoot, 'data/reusables/inner.md') + const outerContent = 'Outer text.\n\n{% data reusables.inner %}\n' + await fs.mkdir(path.dirname(outerFile), { recursive: true }) + await fs.writeFile(outerFile, outerContent) + await fs.writeFile(innerFile, 'Inner original text.\n') + const editedFile = path.join(testContentDir, 'edited.md') + const uneditedFile = path.join(testContentDir, 'unedited.md') + const editedOriginal = '{% data reusables.inner %}\n' + const uneditedOriginal = '{% data reusables.outer %}\n' + await fs.writeFile(editedFile, editedOriginal) + await fs.writeFile(uneditedFile, uneditedOriginal) + const env = { ROOT: fixtureRoot } + const paths = `--paths "${editedFile}" "${uneditedFile}"` + + await runScript(`expand ${paths}`, env) + const expandedContent = await fs.readFile(editedFile, 'utf8') + expect(await fs.readFile(uneditedFile, 'utf8')).toContain('') + await fs.writeFile( + editedFile, + expandedContent.replace('Inner original text.', 'Inner edited text.'), + ) + + const { output, exitCode } = await runScript(`restore ${paths}`, env) + + expect(exitCode, `Restore script failed with output: ${output}`).toBe(0) + expect(await fs.readFile(editedFile, 'utf8')).toBe(editedOriginal) + expect(await fs.readFile(uneditedFile, 'utf8')).toBe(uneditedOriginal) + expect(await fs.readFile(innerFile, 'utf8')).toBe('Inner edited text.\n') + expect(await fs.readFile(outerFile, 'utf8')).toBe(outerContent) + }) + + test('restore keeps an edit to a reusable that is also an input path', async () => { + const fixtureRoot = path.join(testContentDir, 'root') + const outerFile = path.join(fixtureRoot, 'data/reusables/outer.md') + const innerFile = path.join(fixtureRoot, 'data/reusables/inner.md') + await fs.mkdir(path.dirname(outerFile), { recursive: true }) + await fs.writeFile(outerFile, 'Outer text.\n\n{% data reusables.inner %}\n') + await fs.writeFile(innerFile, 'Inner original text.\n') + const testFile = path.join(testContentDir, 'article.md') + const originalContent = '{% data reusables.outer %}\n' + await fs.writeFile(testFile, originalContent) + const env = { ROOT: fixtureRoot } + const paths = `--paths "${testFile}" "${outerFile}"` + + await runScript(`expand ${paths}`, env) + expect(await fs.readFile(outerFile, 'utf8')).toContain('') + const expandedContent = await fs.readFile(testFile, 'utf8') + await fs.writeFile(testFile, expandedContent.replace('Outer text.', 'Outer edited text.')) + + const { output, exitCode } = await runScript(`restore ${paths}`, env) + + expect(exitCode, `Restore script failed with output: ${output}`).toBe(0) + expect(await fs.readFile(testFile, 'utf8')).toBe(originalContent) + expect(await fs.readFile(outerFile, 'utf8')).toBe( + 'Outer edited text.\n\n{% data reusables.inner %}\n', + ) + expect(await fs.readFile(innerFile, 'utf8')).toBe('Inner original text.\n') + }) + + test('restore --reusables-only keeps an edit in a variable inside a reusable', async () => { + const fixtureRoot = path.join(testContentDir, 'root') + const reusableFile = path.join(fixtureRoot, 'data/reusables/outer.md') + const yamlFile = path.join(fixtureRoot, 'data/variables/product.yml') + const reusableContent = 'Use {% data variables.product.name %} here.\n' + await fs.mkdir(path.dirname(reusableFile), { recursive: true }) + await fs.mkdir(path.dirname(yamlFile), { recursive: true }) + await fs.writeFile(reusableFile, reusableContent) + await fs.writeFile(yamlFile, 'name: Octo\n') + const testFile = path.join(testContentDir, 'reusables-only.md') + const originalContent = '{% data reusables.outer %}\n' + await fs.writeFile(testFile, originalContent) + const env = { ROOT: fixtureRoot } + + await runScript(`expand --paths "${testFile}"`, env) + const expandedContent = await fs.readFile(testFile, 'utf8') + expect(expandedContent).toContain('Octo<') + await fs.writeFile(testFile, expandedContent.replace('Octo<', 'Octopus<')) + + const { output, exitCode } = await runScript( + `restore --reusables-only --paths "${testFile}"`, + env, + ) + + expect(exitCode, `Restore script failed with output: ${output}`).toBe(0) + expect(await fs.readFile(testFile, 'utf8')).toBe(originalContent) + expect(await fs.readFile(reusableFile, 'utf8')).toBe(reusableContent) + expect(await fs.readFile(yamlFile, 'utf8')).toBe('name: Octopus\n') + }) + + test('restore --variables-only keeps an edit in a reusable inside a variable', async () => { + const fixtureRoot = path.join(testContentDir, 'root') + const innerFile = path.join(fixtureRoot, 'data/reusables/inner.md') + const yamlFile = path.join(fixtureRoot, 'data/variables/product.yml') + const yamlContent = "# A comment that must survive\nUI_name: '{% data reusables.inner %}'\n" + await fs.mkdir(path.dirname(innerFile), { recursive: true }) + await fs.mkdir(path.dirname(yamlFile), { recursive: true }) + await fs.writeFile(innerFile, 'Inner original text.\n') + await fs.writeFile(yamlFile, yamlContent) + const testFile = path.join(testContentDir, 'variables-only.md') + const originalContent = 'Click {% data variables.product.UI_name %}.\n' + await fs.writeFile(testFile, originalContent) + const env = { ROOT: fixtureRoot } + + await runScript(`expand --paths "${testFile}"`, env) + const expandedContent = await fs.readFile(testFile, 'utf8') + expect(expandedContent).toContain('') + await fs.writeFile( + testFile, + expandedContent.replace('Inner original text.', 'Inner edited text.'), + ) + + const { output, exitCode } = await runScript( + `restore --variables-only --paths "${testFile}"`, + env, + ) + + expect(exitCode, `Restore script failed with output: ${output}`).toBe(0) + expect(await fs.readFile(testFile, 'utf8')).toBe(originalContent) + expect(await fs.readFile(innerFile, 'utf8')).toBe('Inner edited text.\n') + expect(await fs.readFile(yamlFile, 'utf8')).toBe(yamlContent) + }) + + async function writeNestedFixture() { + const fixtureRoot = path.join(testContentDir, 'root') + const yamlFile = path.join(fixtureRoot, 'data/variables/test.yml') + const reusableFile = path.join(fixtureRoot, 'data/reusables/foo/bar.md') + const yamlContent = `# A comment that must survive +inner: Foo +outer: '{% data variables.test.inner %}' +` + const reusableContent = 'Use {% data variables.test.inner %} here.\n' + await fs.mkdir(path.dirname(yamlFile), { recursive: true }) + await fs.mkdir(path.dirname(reusableFile), { recursive: true }) + await fs.writeFile(yamlFile, yamlContent) + await fs.writeFile(reusableFile, reusableContent) + return { fixtureRoot, yamlFile, reusableFile, yamlContent, reusableContent } + } + + test('expand then restore leaves an article variable that nests a variable unchanged', async () => { + const { fixtureRoot, yamlFile, yamlContent } = await writeNestedFixture() + const testFile = path.join(testContentDir, 'nested-article-variable.md') + const originalContent = 'Uses {% data variables.test.outer %} here.\n' + await fs.writeFile(testFile, originalContent) + const env = { ROOT: fixtureRoot } + + await runScript(`expand --paths "${testFile}"`, env) + expect(await fs.readFile(testFile, 'utf8')).toContain('') + + const { output, exitCode } = await runScript(`restore --paths "${testFile}"`, env) + + expect(exitCode, `Restore script failed with output: ${output}`).toBe(0) + expect(output).not.toContain('contains expanded references') + expect(await fs.readFile(testFile, 'utf8')).toBe(originalContent) + expect(await fs.readFile(yamlFile, 'utf8')).toBe(yamlContent) + }) + + test('restore --reusables-only does not write nested variable markers to reusables', async () => { + const { fixtureRoot, yamlFile, reusableFile, yamlContent, reusableContent } = + await writeNestedFixture() + const testFile = path.join(testContentDir, 'nested-reusable.md') + const originalContent = '{% data reusables.foo.bar %}\n' + await fs.writeFile(testFile, originalContent) + const env = { ROOT: fixtureRoot } + + await runScript(`expand --paths "${testFile}"`, env) + const { output, exitCode } = await runScript( + `restore --reusables-only --paths "${testFile}"`, + env, + ) + + expect(exitCode, `Restore script failed with output: ${output}`).toBe(0) + expect(await fs.readFile(testFile, 'utf8')).toBe(originalContent) + expect(await fs.readFile(reusableFile, 'utf8')).toBe(reusableContent) + expect(await fs.readFile(yamlFile, 'utf8')).toBe(yamlContent) + }) + + test('restore --reusables-only writes reusable edits with nested variables as Liquid tags', async () => { + const { fixtureRoot, reusableFile } = await writeNestedFixture() + const testFile = path.join(testContentDir, 'edited-reusable.md') + const originalContent = '{% data reusables.foo.bar %}\n' + await fs.writeFile(testFile, originalContent) + const env = { ROOT: fixtureRoot } + + await runScript(`expand --paths "${testFile}"`, env) + const expandedContent = await fs.readFile(testFile, 'utf8') + await fs.writeFile(testFile, expandedContent.replace(' here.', ' there.')) + const { output, exitCode } = await runScript( + `restore --reusables-only --paths "${testFile}"`, + env, + ) + + expect(exitCode, `Restore script failed with output: ${output}`).toBe(0) + expect(await fs.readFile(testFile, 'utf8')).toBe(originalContent) + expect(await fs.readFile(reusableFile, 'utf8')).toBe( + 'Use {% data variables.test.inner %} there.\n', + ) + }) + + test('restore saves an edit made only inside a nested variable', async () => { + const { fixtureRoot, yamlFile } = await writeNestedFixture() + const testFile = path.join(testContentDir, 'edited-nested-variable.md') + const originalContent = 'Uses {% data variables.test.outer %} here.\n' + await fs.writeFile(testFile, originalContent) + const env = { ROOT: fixtureRoot } + + await runScript(`expand --paths "${testFile}"`, env) + const expandedContent = await fs.readFile(testFile, 'utf8') + await fs.writeFile(testFile, expandedContent.replace('Foo', 'Bar')) + const { output, exitCode } = await runScript(`restore --paths "${testFile}"`, env) + + expect(exitCode, `Restore script failed with output: ${output}`).toBe(0) + expect(await fs.readFile(testFile, 'utf8')).toBe(originalContent) + expect(load(await fs.readFile(yamlFile, 'utf8'))).toEqual({ + inner: 'Bar', + outer: '{% data variables.test.inner %}', + }) + }) + + test('restore saves edits to both an outer variable and its nested variable', async () => { + const { fixtureRoot, yamlFile } = await writeNestedFixture() + const testFile = path.join(testContentDir, 'edited-outer-and-nested.md') + const originalContent = 'Uses {% data variables.test.outer %} here.\n' + await fs.writeFile(testFile, originalContent) + const env = { ROOT: fixtureRoot } + + await runScript(`expand --paths "${testFile}"`, env) + const expandedContent = await fs.readFile(testFile, 'utf8') + await fs.writeFile( + testFile, + expandedContent + .replace('Foo', 'Bar') + .replace('', 'Hi '), + ) + const { output, exitCode } = await runScript(`restore --paths "${testFile}"`, env) + + expect(exitCode, `Restore script failed with output: ${output}`).toBe(0) + expect(await fs.readFile(testFile, 'utf8')).toBe(originalContent) + expect(load(await fs.readFile(yamlFile, 'utf8'))).toEqual({ + inner: 'Bar', + outer: 'Hi {% data variables.test.inner %}', + }) + }) + + test('restore --variables-only restores a variable inside a reusable and keeps its edit', async () => { + const { fixtureRoot, yamlFile, reusableFile, reusableContent } = await writeNestedFixture() + const testFile = path.join(testContentDir, 'variables-only-in-reusable.md') + await fs.writeFile(testFile, '{% data reusables.foo.bar %}\n') + const env = { ROOT: fixtureRoot } + + await runScript(`expand --paths "${testFile}"`, env) + const expandedContent = await fs.readFile(testFile, 'utf8') + await fs.writeFile(testFile, expandedContent.replace('Foo', 'Bar')) + const { output, exitCode } = await runScript( + `restore --variables-only --paths "${testFile}"`, + env, + ) + + expect(exitCode, `Restore script failed with output: ${output}`).toBe(0) + const restoredContent = await fs.readFile(testFile, 'utf8') + expect(restoredContent).toContain('') + expect(restoredContent).toContain('Use {% data variables.test.inner %} here.') + expect(restoredContent).not.toContain('\n**Please restore deleted features**\n\n' markdown += "Even if you don't reference these features anymore, as of this branch, please do not delete them.\n" markdown += 'They might still be referenced in translated content.\n' diff --git a/src/data-directory/scripts/find-orphaned-features/find.ts b/src/data-directory/scripts/find-orphaned-features/find.ts index 43f7b73afde0..ad8464230854 100644 --- a/src/data-directory/scripts/find-orphaned-features/find.ts +++ b/src/data-directory/scripts/find-orphaned-features/find.ts @@ -1,6 +1,8 @@ // 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. +// It scans title, intro, shortTitle, permissions, product, introLinks, body, and English versions +// frontmatter across all pages, including early access pages when they are merged in. +// It also scans English reusables, variables, tables, and early access data, 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. @@ -13,7 +15,6 @@ import { TokenizationError, TokenKind } from 'liquidjs' import type { TagToken } from 'liquidjs' import type { Page } from '@/types' -import warmServer from '@/frame/lib/warm-server' import { getDeepDataByLanguage } from '@/data-directory/lib/get-data' import { getLiquidTokens } from '@/content-linter/lib/helpers/liquid-utils' import languages from '@/languages/lib/languages-server' @@ -41,6 +42,8 @@ export async function find(options: Options) { ), ) } + // A lazy import keeps warm-server and its dependencies out of tests that load the helpers. + const { default: warmServer } = await import('@/frame/lib/warm-server') const site = await warmServer([]) const features = new Set( @@ -99,7 +102,6 @@ function formatDelta(t0: Date, t1: Date) { // 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 // Only English versions frontmatter can mark a feature used. if (page.languageCode === 'en') { for (const [key, value] of Object.entries(page.versions)) { @@ -111,17 +113,14 @@ function searchAndRemove(features: Set, pages: Page[], verbose = false) } } - const combined = ` - ${content} - ${page.title || ''} - ${page.shortTitle || ''} - ${page.intro || ''} - ` - - checkString(combined, features, { page, verbose, languageCode: page.languageCode }) + checkString(getPageSearchText(page), features, { + page, + verbose, + languageCode: page.languageCode, + }) } - for (const filePath of getVariableFiles(path.join(languages.en.dir, 'data', 'variables'))) { + for (const filePath of getEnglishDataFiles(path.join(languages.en.dir, 'data'))) { const fileContent = fs.readFileSync(filePath, 'utf-8') checkString(fileContent, features, { filePath, verbose, languageCode: 'en' }) } @@ -165,6 +164,38 @@ function searchAndRemove(features: Set, pages: Page[], verbose = false) } } +type PageSearchFields = Pick< + Page, + 'markdown' | 'title' | 'shortTitle' | 'intro' | 'rawPermissions' +> & { + rawProduct?: string + rawIntroLinks?: Record +} + +// Page.render runs Liquid on permissions, product, and introLinks, so their ifversion tags count too. +export function getPageSearchText(page: PageSearchFields) { + return ` + ${page.markdown} + ${page.title || ''} + ${page.shortTitle || ''} + ${page.intro || ''} + ${page.rawPermissions || ''} + ${page.rawProduct || ''} + ${Object.values(page.rawIntroLinks || {}).join('\n')} + ` +} + +// Pages render table rows with render_liquid. Early access data exists only after a merge. +export function getEnglishDataFiles(dataRoot: string): string[] { + const sources: [string, (root: string) => string[]][] = [ + [path.join(dataRoot, 'variables'), getVariableFiles], + [path.join(dataRoot, 'tables'), getVariableFiles], + [path.join(dataRoot, 'early-access', 'variables'), getVariableFiles], + [path.join(dataRoot, 'early-access', 'reusables'), getReusableFiles], + ] + return sources.flatMap(([dir, getFiles]) => (fs.existsSync(dir) ? getFiles(dir) : [])) +} + export function getReusableFiles(root: string): string[] { const here = [] for (const file of fs.readdirSync(root)) { @@ -193,7 +224,7 @@ export function getVariableFiles(root: string): string[] { const IGNORE_ARGS = new Set(['or', 'and', 'not', '<', '>', 'ghes', 'fpt', 'ghec', '!=', '=']) -function checkString( +export function checkString( string: string, features: Set, { diff --git a/src/data-directory/tests/get-data.ts b/src/data-directory/tests/get-data.ts index f311042a612e..489fe8f412e1 100644 --- a/src/data-directory/tests/get-data.ts +++ b/src/data-directory/tests/get-data.ts @@ -264,6 +264,9 @@ describe('get-data on corrupt translations', () => { beforeAll(() => { dd = new DataDirectory({ data: { + ui: { + key: 'Value', + }, variables: { everything: { is: 'Awesome', @@ -276,6 +279,19 @@ describe('get-data on corrupt translations', () => { }) languages.en.dir = dd.root + // getUIDataMerged caches by language code, so use codes no other test has read. + const esTranslationsRoot = path.join(dd.root, 'translations', 'es-ES') + fs.mkdirSync(path.join(esTranslationsRoot, 'data'), { recursive: true }) + fs.writeFileSync( + path.join(esTranslationsRoot, 'data', 'ui.yml'), + `>:This is not valid Yaml:>`, + 'utf-8', + ) + languages.es = Object.assign({}, languages.en, { dir: esTranslationsRoot }) + const frTranslationsRoot = path.join(dd.root, 'translations', 'fr-FR') + fs.mkdirSync(frTranslationsRoot, { recursive: true }) + languages.fr = Object.assign({}, languages.en, { dir: frTranslationsRoot }) + const jaTranslationsRoot = path.join(dd.root, 'translations', 'ja-JP') fs.mkdirSync(jaTranslationsRoot, { recursive: true }) languages.ja.dir = jaTranslationsRoot @@ -305,6 +321,8 @@ describe('get-data on corrupt translations', () => { afterAll(() => { dd.destroy() languages.en.dir = enDirBefore + delete languages.es + delete languages.fr }) test('getDataByLanguage on a corrupt .yml file', () => { @@ -328,6 +346,16 @@ describe('get-data on corrupt translations', () => { expect(result).toBe('*English* /Markdown/') } }) + + test('getUIDataMerged on a corrupt ui.yml file', () => { + const result = getUIDataMerged('es') + expect(result.key).toBe('Value') + }) + + test('getUIDataMerged on a missing ui.yml file', () => { + const result = getUIDataMerged('fr') + expect(result.key).toBe('Value') + }) }) describe('get-data applies corrections to translated variables', () => { diff --git a/src/data-directory/tests/orphaned-features.ts b/src/data-directory/tests/orphaned-features.ts index e63c7c1f8db7..78d9bb60d777 100644 --- a/src/data-directory/tests/orphaned-features.ts +++ b/src/data-directory/tests/orphaned-features.ts @@ -7,7 +7,7 @@ import { describe, expect, test } from 'vitest' const __dirname = path.dirname(fileURLToPath(import.meta.url)) const fixturesDir = path.join(__dirname, 'orphaned-features', 'fixtures') -const { getVariableFiles, getReusableFiles } = +const { getVariableFiles, getReusableFiles, getEnglishDataFiles, getPageSearchText, checkString } = await import('@/data-directory/scripts/find-orphaned-features/find') describe('orphaned features detection', () => { @@ -151,4 +151,54 @@ describe('orphaned features detection', () => { fs.rmSync(tempDir, { recursive: true, force: true }) } }) + + test('page frontmatter rendered with Liquid counts as feature usage', () => { + const features = new Set([ + 'only-in-permissions', + 'only-in-product', + 'only-in-introlinks', + 'truly-orphaned', + ]) + const page = { + markdown: '', + title: 'Test page', + intro: '', + rawPermissions: '{% ifversion only-in-permissions %}a{% else %}b{% endif %}', + rawProduct: '{% ifversion only-in-product %}x{% endif %}', + rawIntroLinks: { overview: '{% ifversion only-in-introlinks %}/a{% endif %}' }, + } + + checkString(getPageSearchText(page), features, { languageCode: 'en' }) + + expect(Array.from(features)).toEqual(['truly-orphaned']) + }) + + test('getEnglishDataFiles includes variables, tables, and early access data', () => { + const dataRoot = path.join(fixturesDir, 'data') + const files = getEnglishDataFiles(dataRoot) + + expect(files.map((filePath) => path.relative(dataRoot, filePath)).sort()).toEqual([ + path.join('early-access', 'reusables', 'test.md'), + path.join('early-access', 'variables', 'test.yml'), + path.join('tables', 'test.yml'), + path.join('variables', 'test.yml'), + ]) + + const features = new Set([ + 'used-in-variables', + 'used-in-tables', + 'used-in-early-access-variables', + 'used-in-early-access-reusables', + 'truly-orphaned', + ]) + for (const filePath of files) { + checkString(fs.readFileSync(filePath, 'utf-8'), features, { filePath, languageCode: 'en' }) + } + + expect(Array.from(features)).toEqual(['truly-orphaned']) + }) + + test('getEnglishDataFiles skips missing data directories', () => { + expect(getEnglishDataFiles(path.join(fixturesDir, 'content'))).toEqual([]) + }) }) diff --git a/src/data-directory/tests/orphaned-features/fixtures/data/early-access/reusables/test.md b/src/data-directory/tests/orphaned-features/fixtures/data/early-access/reusables/test.md new file mode 100644 index 000000000000..35f5ab6e2e4a --- /dev/null +++ b/src/data-directory/tests/orphaned-features/fixtures/data/early-access/reusables/test.md @@ -0,0 +1,3 @@ +{% ifversion used-in-early-access-reusables %} +This early access reusable uses a feature. +{% endif %} diff --git a/src/data-directory/tests/orphaned-features/fixtures/data/early-access/variables/test.yml b/src/data-directory/tests/orphaned-features/fixtures/data/early-access/variables/test.yml new file mode 100644 index 000000000000..bee88089aeb4 --- /dev/null +++ b/src/data-directory/tests/orphaned-features/fixtures/data/early-access/variables/test.yml @@ -0,0 +1,2 @@ +# Test early access variables file for orphaned features testing +ea_variable: '{% ifversion used-in-early-access-variables %}Feature enabled{% endif %}' diff --git a/src/data-directory/tests/orphaned-features/fixtures/data/tables/test.yml b/src/data-directory/tests/orphaned-features/fixtures/data/tables/test.yml new file mode 100644 index 000000000000..bec12b3d2ab5 --- /dev/null +++ b/src/data-directory/tests/orphaned-features/fixtures/data/tables/test.yml @@ -0,0 +1,3 @@ +# Test table file for orphaned features testing +- action: 'Row shown only with a feature' + versions: '{% ifversion used-in-tables %}show{% endif %}' diff --git a/src/events/scripts/analyze-comment-cli.ts b/src/events/scripts/analyze-comment-cli.ts index 4216a5d6aa1b..8538593ed02b 100644 --- a/src/events/scripts/analyze-comment-cli.ts +++ b/src/events/scripts/analyze-comment-cli.ts @@ -40,7 +40,7 @@ async function main(comment?: string, options?: Options) { let rating = 1.0 let broke = false for (const { reduction, name, validator } of SIGNAL_RATINGS) { - const hit = validator(comment, language) + const hit = await validator(comment, language) if (hit) { console.log( chalk.yellow( diff --git a/src/events/scripts/analyze-comments-csv.ts b/src/events/scripts/analyze-comments-csv.ts index 9ced524b9946..9fd3e736ae95 100644 --- a/src/events/scripts/analyze-comments-csv.ts +++ b/src/events/scripts/analyze-comments-csv.ts @@ -71,7 +71,7 @@ async function analyzeFile(csvFile: string, options: Options) { let rating = 1.0 let first = true for (const { reduction, name, validator } of SIGNAL_RATINGS) { - const hit = validator(record.survey_comment, language) + const hit = await validator(record.survey_comment, language) if (hit) { rating -= reduction if (first) { diff --git a/src/fixtures/fixtures/content/get-started/foo/index.md b/src/fixtures/fixtures/content/get-started/foo/index.md index d774a1ca7913..48f0722a534a 100644 --- a/src/fixtures/fixtures/content/get-started/foo/index.md +++ b/src/fixtures/fixtures/content/get-started/foo/index.md @@ -20,5 +20,6 @@ children: - /code-snippet-with-hashbang - /journey-test-article - /broken-frontmatter-translation + - /mistyped-frontmatter-translation - /anchor-scrolling --- diff --git a/src/fixtures/fixtures/content/get-started/foo/mistyped-frontmatter-translation.md b/src/fixtures/fixtures/content/get-started/foo/mistyped-frontmatter-translation.md new file mode 100644 index 000000000000..10ff01b2b9fd --- /dev/null +++ b/src/fixtures/fixtures/content/get-started/foo/mistyped-frontmatter-translation.md @@ -0,0 +1,14 @@ +--- +title: Mistyped frontmatter translation fallback +intro: Demonstrates falling back to English when a translated frontmatter value has the wrong type. +versions: + fpt: "*" + ghes: "*" + ghec: "*" +--- + +## Intro + +This page exists to test that a translated file with valid YAML but a +translatable frontmatter value of the wrong type, such as an empty `title` +or a number for `intro`, falls back to the English value for that key. diff --git a/src/fixtures/fixtures/data/ui.yml b/src/fixtures/fixtures/data/ui.yml index a88b9607bc29..131b00dd395f 100644 --- a/src/fixtures/fixtures/data/ui.yml +++ b/src/fixtures/fixtures/data/ui.yml @@ -210,7 +210,7 @@ rest: banner: api_versioned: The REST API is now versioned. api_version_info: For more information, see "About API versioning." - ghes_api_versioned: After a site administrator upgrades your Enterprise Server instance to {{ firstGhesReleaseWithApiVersions.versionTitle }} or later, the REST API will be versioned. To learn how to find your instance's version, see "About versions of GitHub Docs". + ghes_api_versioned: After a site administrator upgrades your Enterprise Server instance to {{ firstGhesReleaseWithApiVersions.versionTitle }} or later, the REST API will be versioned. To learn how to find your instance's version, see "About versions of GitHub Docs". redirect_notice: We've recently moved some of the REST API documentation. redirect_repo: If you can't find what you're looking for, you might try the new {{ newRestPagesLinks }} REST API pages. redirect_enterprise: If you can't find what you're looking for, you might try the {{ actionsPageLink }} REST API page. diff --git a/src/fixtures/fixtures/translations/ja-jp/content/get-started/foo/mistyped-frontmatter-translation.md b/src/fixtures/fixtures/translations/ja-jp/content/get-started/foo/mistyped-frontmatter-translation.md new file mode 100644 index 000000000000..fa8ec871244b --- /dev/null +++ b/src/fixtures/fixtures/translations/ja-jp/content/get-started/foo/mistyped-frontmatter-translation.md @@ -0,0 +1,13 @@ +--- +title: +intro: 2024 +versions: + fpt: "*" + ghes: "*" + ghec: "*" +--- + +## Intro + +このページは、翻訳ファイルのフロントマターの値の型が間違っている場合に、 +そのキーだけ英語の値にフォールバックすることをテストするために存在します。 diff --git a/src/fixtures/tests/playwright-rendering.spec.ts b/src/fixtures/tests/playwright-rendering.spec.ts index a55385230330..ce84515d24b4 100644 --- a/src/fixtures/tests/playwright-rendering.spec.ts +++ b/src/fixtures/tests/playwright-rendering.spec.ts @@ -564,6 +564,53 @@ test.describe('search dialog', () => { await expect(page).not.toHaveURL(/search-overlay-open/) }) + test('input method composition keys do not select, ask Copilot, or close', async ({ page }) => { + await mockSearch(page) + let aiRequests = 0 + await page.route('**/api/ai-search/v1', (route) => { + aiRequests++ + return route.abort() + }) + await page.goto('/') + await turnOffExperimentsInPage(page) + + await page.getByTestId('toggle-search').click() + const input = page.getByTestId('overlay-search-input') + await input.fill('foo') + await expect(page.locator('#search-option-general-0')).toBeVisible() + + // Chrome flags composing keys with isComposing. Safari flags keys that end a composition only with keyCode 229. + // Each case sets one flag so a missing isComposing or keyCode guard fails on its own. + const dispatchImeKey = (key: string, isComposing: boolean) => + input.evaluate( + (el, init) => + el.dispatchEvent( + new KeyboardEvent('keydown', { + ...init, + bubbles: true, + cancelable: true, + }), + ), + { key, isComposing, keyCode: isComposing ? 0 : 229 }, + ) + await dispatchImeKey('ArrowDown', true) + expect(await input.getAttribute('aria-activedescendant')).toBeNull() + await dispatchImeKey('Enter', true) + await dispatchImeKey('Enter', false) + await dispatchImeKey('Escape', true) + await dispatchImeKey('Escape', false) + + // Give a stray Ask Copilot request or URL write time to land. + await page.waitForTimeout(300) + await expect(input).toBeVisible() + await expect(page.getByRole('button', { name: 'Back to search' })).toBeHidden() + expect(page.url()).not.toMatch(/search-overlay-ask-ai=true/) + expect(aiRequests).toBe(0) + + await input.press('Escape') + await expect(input).toHaveCount(0) + }) + test('clicking a row keeps focus, and the arrow keys, on the input', async ({ page }) => { await mockSearch(page) await page.goto('/') @@ -832,6 +879,129 @@ test.describe('search dialog', () => { await expect(input).toHaveAttribute('aria-activedescendant', 'search-option-reference-0') }) + // Streams one answer chunk, then holds until releaseCopilot, then ends the stream as asked. + // Also records event beacons, since Chromium hides sendBeacon payloads from Playwright. + async function streamCopilotAnswer( + page: Page, + ending: 'close' | 'error-record' | 'network-error', + ) { + await page.addInitScript((streamEnding) => { + const testWindow = window as unknown as { + releaseCopilot: () => void + copilotBeacons: Promise[] + } + let release = () => {} + const released = new Promise((resolve) => { + release = resolve + }) + testWindow.releaseCopilot = () => release() + testWindow.copilotBeacons = [] + navigator.sendBeacon = (url, data) => { + if (url === '/api/events' && data instanceof Blob) { + testWindow.copilotBeacons.push(data.text()) + } + return true + } + const realFetch = window.fetch + window.fetch = async (input, init) => { + const url = + typeof input === 'string' ? input : input instanceof URL ? input.href : input.url + if (!url.includes('/api/ai-search/v1')) return realFetch(input, init) + const encoder = new TextEncoder() + const body = new ReadableStream({ + async start(controller) { + controller.enqueue( + encoder.encode('{"chunkType":"MESSAGE_CHUNK","text":"Streamed answer"}\n'), + ) + await released + if (streamEnding === 'network-error') { + controller.error(new TypeError('network error')) + return + } + if (streamEnding === 'error-record') { + controller.enqueue( + encoder.encode('{"errors":[{"message":"Internal server error"}]}\n'), + ) + } + controller.close() + }, + }) + return new Response(body, { + status: 200, + headers: { 'Content-Type': 'application/x-ndjson' }, + }) + } + }, ending) + } + + async function askCopilotAndRelease(page: Page) { + await mockSearch(page) + await page.goto('/') + await turnOffExperimentsInPage(page) + + await page.getByTestId('toggle-search').click() + const input = page.getByTestId('overlay-search-input') + await input.fill('foo') + await expect(page.locator('#search-option-general-0')).toBeVisible() + await input.press('Enter') // nothing selected: ask Copilot + // Let the client render the first chunk before the stream ends. + await expect(page.getByText('Streamed answer', { exact: true })).toBeVisible() + await releaseCopilot(page) + } + + function cachedCopilotAnswers(page: Page) { + return page.evaluate(() => + Object.keys(localStorage) + .filter((key) => /^ai-query-cache-\d+-/.test(key)) + .map((key) => JSON.parse(localStorage.getItem(key)!).data.message as string), + ) + } + + function sentCopilotResultStatuses(page: Page) { + return page.evaluate(async () => { + // Hiding the page flushes the queued events. + Object.defineProperty(document, 'visibilityState', { + configurable: true, + get() { + return 'hidden' + }, + }) + document.dispatchEvent(new Event('visibilitychange')) + const testWindow = window as unknown as { copilotBeacons: Promise[] } + const batches = await Promise.all(testWindow.copilotBeacons) + return batches + .flatMap((batch) => JSON.parse(batch)) + .filter((eventBody) => eventBody.type === 'aiSearchResult') + .map((eventBody) => eventBody.ai_search_result_response_status as number) + }) + } + + test('a finished Copilot answer is cached and reported as a success', async ({ page }) => { + test.skip(!ANALYTICS_ENABLED, 'Analytics are disabled') + await streamCopilotAnswer(page, 'close') + await askCopilotAndRelease(page) + + await expect.poll(() => cachedCopilotAnswers(page)).toEqual(['Streamed answer']) + expect(await sentCopilotResultStatuses(page)).toContain(200) + }) + + for (const [ending, cause] of [ + ['error-record', 'an error record'], + ['network-error', 'a network error'], + ] as const) { + test(`a Copilot answer cut off by ${cause} is not cached or reported as a success`, async ({ + page, + }) => { + test.skip(!ANALYTICS_ENABLED, 'Analytics are disabled') + await streamCopilotAnswer(page, ending) + await askCopilotAndRelease(page) + + await expect(page.getByText('There was an error loading Copilot.')).toBeVisible() + expect(await cachedCopilotAnswers(page)).toEqual([]) + expect(await sentCopilotResultStatuses(page)).not.toContain(200) + }) + } + test('Back after picking a result leaves the dialog closed', async ({ page }) => { await mockSearch(page) await page.goto('/get-started/foo/bar') diff --git a/src/fixtures/tests/sidebar.ts b/src/fixtures/tests/sidebar.ts index 5d5eadccc5b5..2b2ae2ed4769 100644 --- a/src/fixtures/tests/sidebar.ts +++ b/src/fixtures/tests/sidebar.ts @@ -56,11 +56,13 @@ describe('sidebar', () => { const $: CheerioAPI = await getDOM('/enterprise-server@latest/pages') const link = $('#allproducts-menu a') expect(link.text()).toBe('Pages (HubGit Enterprise Server)') + expect(link.attr('href')).toMatch(/^\/en\/enterprise-server@\d+\.\d+\/pages$/) } { const $: CheerioAPI = await getDOM('/enterprise-cloud@latest/pages') const link = $('#allproducts-menu a') expect(link.text()).toBe('Pages (HubGit Enterprise Cloud)') + expect(link.attr('href')).toBe('/en/enterprise-cloud@latest/pages') } }) diff --git a/src/frame/components/article/ViewMarkdownButton.tsx b/src/frame/components/article/ViewMarkdownButton.tsx index 6e69200805b6..5d5d226ef65f 100644 --- a/src/frame/components/article/ViewMarkdownButton.tsx +++ b/src/frame/components/article/ViewMarkdownButton.tsx @@ -17,6 +17,18 @@ import { useTranslation } from '@/languages/components/useTranslation' import cx from 'clsx' import styles from './ViewMarkdownButton.module.scss' +async function fetchMarkdown(markdownUrl: string) { + const res = await fetch(markdownUrl) + if (!res.ok) { + throw new Error(`Failed to fetch: ${res.status}`) + } + return res.text() +} + +async function fetchMarkdownBlob(markdownUrl: string) { + return new Blob([await fetchMarkdown(markdownUrl)], { type: 'text/plain' }) +} + interface CopyMarkdownMenuProps { currentPath: string } @@ -30,6 +42,7 @@ export const CopyMarkdownMenu = ({ currentPath }: CopyMarkdownMenuProps) => { const [copied, setCopied] = useState(false) const timeoutRef = useRef | null>(null) + const copyAttemptRef = useRef(0) useEffect(() => { return () => { @@ -71,18 +84,22 @@ export const CopyMarkdownMenu = ({ currentPath }: CopyMarkdownMenuProps) => { clipboard_target: markdownUrl, eventGroupKey: MARKDOWN_SOURCE_MENU_EVENT_GROUP, }) + const attempt = ++copyAttemptRef.current try { - const res = await fetch(markdownUrl) - if (!res.ok) { - throw new Error(`Failed to fetch: ${res.status}`) + if (typeof ClipboardItem !== 'undefined' && typeof navigator.clipboard.write === 'function') { + // Safari rejects clipboard writes that start after an await in a click handler, so start the write before the fetch resolves. + const blob = fetchMarkdownBlob(markdownUrl) + await navigator.clipboard.write([new ClipboardItem({ 'text/plain': blob })]) + } else { + await navigator.clipboard.writeText(await fetchMarkdown(markdownUrl)) } - const text = await res.text() - await navigator.clipboard.writeText(text) announce(t('copied')) setCopied(true) if (timeoutRef.current) clearTimeout(timeoutRef.current) timeoutRef.current = setTimeout(() => setCopied(false), 2000) } catch { + // Safari rejects a pending clipboard write when a newer click starts another, so a superseded attempt skips the fallback. + if (attempt !== copyAttemptRef.current) return // If fetch or clipboard fails, open the markdown source in a new tab. window.open(markdownUrl, '_blank') } diff --git a/src/frame/components/page-header/hooks/useHeaderNavigation.ts b/src/frame/components/page-header/hooks/useHeaderNavigation.ts index c2ad97b01745..24715dcda116 100644 --- a/src/frame/components/page-header/hooks/useHeaderNavigation.ts +++ b/src/frame/components/page-header/hooks/useHeaderNavigation.ts @@ -86,7 +86,8 @@ export function useHeaderNavigation({ useEffect(() => { const handleKeyDown = (event: KeyboardEvent) => { - if (event.defaultPrevented || event.isComposing) return + // Safari flags keys that end an input method composition only with keyCode 229. + if (event.defaultPrevented || event.isComposing || event.keyCode === 229) return if (event.key === 'Escape' && isSearchOpen) { setIsSearchOpen(false) return diff --git a/src/frame/components/sidebar/SidebarNav.tsx b/src/frame/components/sidebar/SidebarNav.tsx index f76b6eb3ae77..01cdbc491439 100644 --- a/src/frame/components/sidebar/SidebarNav.tsx +++ b/src/frame/components/sidebar/SidebarNav.tsx @@ -17,7 +17,7 @@ type Props = { } export const SidebarNav = ({ variant = 'full', mobileOpen = false }: Props) => { - const { currentProduct, currentProductName } = useMainContext() + const { currentProduct, currentProductName, sidebarTree } = useMainContext() const router = useRouter() const isRestPage = currentProduct && currentProduct.id === 'rest' @@ -69,7 +69,8 @@ export const SidebarNav = ({ variant = 'full', mobileOpen = false }: Props) => {

)[property] = (enData as Record)[property] + ;(data as Record)[key] = (enData as Record)[key] } } } catch (error) { diff --git a/src/frame/middleware/context/current-product-tree.ts b/src/frame/middleware/context/current-product-tree.ts index 29899bc3511e..679a488e5be0 100644 --- a/src/frame/middleware/context/current-product-tree.ts +++ b/src/frame/middleware/context/current-product-tree.ts @@ -104,7 +104,20 @@ async function getCurrentProductTreeTitles(input: Tree, context: Context): Promi childPages: childPages.filter(Boolean), } if (page.hidden) node.hidden = true - if (page.sidebarLink) node.sidebarLink = page.sidebarLink + if (page.sidebarLink) { + // Frontmatter hrefs are unversioned, and the page object is shared by every version. + node.sidebarLink = { + ...page.sidebarLink, + href: removeFPTFromPath( + path.posix.join( + '/', + context.currentLanguage!, + context.currentVersion!, + page.sidebarLink.href, + ), + ), + } + } if (page.layout && typeof page.layout === 'string') node.layout = page.layout if (input.crossProductChild) node.crossProductChild = true return node diff --git a/src/frame/middleware/resolve-carousels.ts b/src/frame/middleware/resolve-carousels.ts index a69c60253eff..f62a56a223c1 100644 --- a/src/frame/middleware/resolve-carousels.ts +++ b/src/frame/middleware/resolve-carousels.ts @@ -1,7 +1,6 @@ -import type { ExtendedRequest, Page, ResolvedArticle } from '@/types' +import type { Context, ExtendedRequest, Page, ResolvedArticle } from '@/types' import type { Response, NextFunction } from 'express' import findPage from '@/frame/lib/find-page' -import { renderContent } from '@/content-render/index' import Permalink from '@/frame/lib/permalink' import { createLogger } from '@/observability/logger/index' @@ -25,9 +24,10 @@ function tryResolveArticlePath( rawPath: string, pageRelativePath: string | undefined, req: ExtendedRequest, + language?: string, ): Page | undefined { const { pages, redirects } = req.context! - const currentLanguage = req.context!.currentLanguage || 'en' + const currentLanguage = language || req.context!.currentLanguage || 'en' if (!pages || !redirects) { return undefined @@ -78,6 +78,18 @@ function tryResolveArticlePath( return foundPage } +function findEnglishArticle( + rawPath: string, + pageRelativePath: string | undefined, + req: ExtendedRequest, +): Page { + const enPage = tryResolveArticlePath(rawPath, pageRelativePath, req, 'en') + if (!enPage) { + throw new Error(`Unable to find English carousel article: ${rawPath}`) + } + return enPage +} + function getPageHref(page: Page): string { if (page.relativePath) { return Permalink.relativePathToSuffix(page.relativePath) @@ -119,9 +131,18 @@ async function resolveCarousels( ? foundPage.relativePath.split('/').slice(0, -1).filter(Boolean) : [] + // The request's getEnglishPage resolves the landing page, not the carousel article. + const renderContext: Context = req.context!.getEnglishPage + ? { + ...req.context!, + getEnglishPage: () => findEnglishArticle(rawPath, page?.relativePath, req), + } + : req.context! + resolved.push({ - title: await renderContent(foundPage.title, req.context, { textOnly: true }), - intro: await renderContent(foundPage.intro, req.context, { textOnly: true }), + // The shared Page keeps title and intro from its last render, which can be another version. + title: await foundPage.renderProp('title', renderContext, { textOnly: true }), + intro: await foundPage.renderProp('intro', renderContext, { textOnly: true }), href, category, }) diff --git a/src/frame/tests/current-product-tree.test.ts b/src/frame/tests/current-product-tree.test.ts index 4a90e697a3d8..f1a500d808c4 100644 --- a/src/frame/tests/current-product-tree.test.ts +++ b/src/frame/tests/current-product-tree.test.ts @@ -99,4 +99,38 @@ describe('currentProductTree middleware', () => { expect(req.context!.sidebarTree!.childPages[0].title).toBe('English short title') expect(next).toHaveBeenCalled() }) + + test.each([ + ['free-pro-team@latest', '/en/product/category'], + ['enterprise-cloud@latest', '/en/enterprise-cloud@latest/product/category'], + ])('prefixes sidebarLink href with language and %s', async (version, expectedHref) => { + const sidebarLink = { text: 'All category articles', href: '/product/category' } + const product = createPage({ rawTitle: 'Product', languageCode: 'en' }) + const category = createPage({ rawTitle: 'Category', languageCode: 'en', sidebarLink }) + const article = createPage({ rawTitle: 'Article', languageCode: 'en' }) + const versionPrefix = version === currentVersion ? '' : `/${version}` + const tree = createTree(`/en${versionPrefix}/product`, product, [ + createTree(`/en${versionPrefix}/product/category`, category, [ + createTree(`/en${versionPrefix}/product/category/article`, article), + ]), + ]) + const req = { + context: { + page: article, + siteTree: { en: { [version]: tree } }, + currentLanguage: 'en', + currentVersion: version, + currentProduct: 'product', + }, + } as unknown as ExtendedRequest + + await currentProductTree(req, {} as Response, vi.fn() as NextFunction) + + expect(req.context!.sidebarTree!.childPages[0].sidebarLink).toEqual({ + text: 'All category articles', + href: expectedHref, + }) + // Pages are shared across requests for every version, so the page object must stay unversioned. + expect(sidebarLink.href).toBe('/product/category') + }) }) diff --git a/src/frame/tests/resolve-carousels.test.ts b/src/frame/tests/resolve-carousels.test.ts index a7a49d80f1c8..6ab7767ed4e4 100644 --- a/src/frame/tests/resolve-carousels.test.ts +++ b/src/frame/tests/resolve-carousels.test.ts @@ -1,6 +1,6 @@ import { describe, test, expect, vi, beforeEach } from 'vitest' import type { Response, NextFunction } from 'express' -import type { ExtendedRequest, Page, ResolvedArticle } from '@/types' +import type { Context, ExtendedRequest, Page, ResolvedArticle } from '@/types' import findPage from '@/frame/lib/find-page' import resolveCarousels from '../middleware/resolve-carousels' @@ -8,16 +8,6 @@ vi.mock('@/frame/lib/find-page', () => ({ default: vi.fn(), })) -vi.mock('@/content-render/index', () => ({ - renderContent: vi.fn((content, _context, options) => { - // textOnly returns plain text so carousel intros stay text-only. - if (options?.textOnly) { - return content - } - return `

${content}

` - }), -})) - describe('resolveCarousels middleware', () => { const mockFindPage = vi.mocked(findPage) @@ -25,6 +15,15 @@ describe('resolveCarousels middleware', () => { rawCarousels?: Record } + // renderProp reads the raw frontmatter, which is what the middleware should render. + const createMockPage = (data: Partial): Page => + ({ + ...data, + renderProp: vi.fn(async (prop: string) => + prop === 'title' ? (data.rawTitle ?? data.title) : (data.rawIntro ?? data.intro), + ), + }) as unknown as Page + const createMockRequest = ( pageData: TestPage = {}, contextData: Partial & { pages?: Record } = {}, @@ -107,7 +106,7 @@ describe('resolveCarousels middleware', () => { applicableVersions: ['free-pro-team@latest'], } - mockFindPage.mockReturnValue(testPage as unknown as Page) + mockFindPage.mockReturnValue(createMockPage(testPage)) const req = createMockRequest({ rawCarousels: { recommended: ['/copilot/tutorials/article'] }, @@ -150,8 +149,8 @@ describe('resolveCarousels middleware', () => { } mockFindPage - .mockReturnValueOnce(testPage1 as unknown as Page) - .mockReturnValueOnce(testPage2 as unknown as Page) + .mockReturnValueOnce(createMockPage(testPage1)) + .mockReturnValueOnce(createMockPage(testPage2)) const req = createMockRequest({ rawCarousels: { @@ -227,7 +226,7 @@ describe('resolveCarousels middleware', () => { applicableVersions: ['free-pro-team@latest'], } - mockFindPage.mockReturnValueOnce(testPage as unknown as Page).mockReturnValueOnce(undefined) + mockFindPage.mockReturnValueOnce(createMockPage(testPage)).mockReturnValueOnce(undefined) const req = createMockRequest({ rawCarousels: { recommended: ['/valid-article', '/invalid-article'] }, @@ -259,7 +258,7 @@ describe('resolveCarousels middleware', () => { } // First lookup is content-relative; second lookup is page-relative. - mockFindPage.mockReturnValueOnce(undefined).mockReturnValueOnce(testPage as unknown as Page) + mockFindPage.mockReturnValueOnce(undefined).mockReturnValueOnce(createMockPage(testPage)) const req = createMockRequest({ rawCarousels: { recommended: ['relative-article'] }, @@ -302,7 +301,7 @@ describe('resolveCarousels middleware', () => { applicableVersions: ['free-pro-team@latest'], } - mockFindPage.mockReturnValue(testPage as unknown as Page) + mockFindPage.mockReturnValue(createMockPage(testPage)) const req = createMockRequest({ rawCarousels: { recommended: ['/copilot/tutorials/tutorial-page'] }, @@ -339,7 +338,7 @@ describe('resolveCarousels middleware', () => { applicableVersions: ['free-pro-team@latest'], // Not available in ghec } - mockFindPage.mockReturnValue(fptOnlyPage as unknown as Page) + mockFindPage.mockReturnValue(createMockPage(fptOnlyPage)) const req = createMockRequest( { rawCarousels: { recommended: ['/test/fpt-only'] } }, @@ -365,7 +364,7 @@ describe('resolveCarousels middleware', () => { applicableVersions: ['free-pro-team@latest'], } - mockFindPage.mockReturnValue(testPage as unknown as Page) + mockFindPage.mockReturnValue(createMockPage(testPage)) const req = createMockRequest({ rawCarousels: { recommended: ['/test/duplicate', '/test/duplicate', '/test/duplicate'] }, @@ -387,4 +386,85 @@ describe('resolveCarousels middleware', () => { }) expect(mockNext).toHaveBeenCalled() }) + + test('renders title and intro from raw frontmatter, not the last rendered values', async () => { + // title and intro hold output from an earlier render for another version. + const testPage = createMockPage({ + title: 'Audit log for GHEC', + intro: 'Logs of audited user actions.', + rawTitle: 'Audit log for GHES', + rawIntro: 'Logs of audited system, user actions.', + relativePath: 'admin/audit-log.md', + applicableVersions: ['enterprise-server@3.19'], + }) + + mockFindPage.mockReturnValue(testPage) + + const req = createMockRequest( + { rawCarousels: { recommended: ['/admin/audit-log'] } }, + { currentVersion: 'enterprise-server@3.19' }, + ) + + await resolveCarousels(req, mockRes, mockNext) + + expect(testPage.renderProp).toHaveBeenCalledWith('title', req.context, { textOnly: true }) + expect(testPage.renderProp).toHaveBeenCalledWith('intro', req.context, { textOnly: true }) + expect( + (req.context!.page as Page & { carousels?: Record }).carousels, + ).toEqual({ + recommended: [ + { + title: 'Audit log for GHES', + intro: 'Logs of audited system, user actions.', + href: '/admin/audit-log', + category: ['admin'], + }, + ], + }) + }) + + test('falls back to the English carousel article, not the English landing page', async () => { + const enArticle = createMockPage({ + rawTitle: 'English article title', + rawIntro: 'English article intro', + relativePath: 'admin/audit-log.md', + applicableVersions: ['free-pro-team@latest'], + }) + // Mimic renderContentWithFallback when the translated Liquid throws. + const jaArticle = { + relativePath: 'admin/audit-log.md', + applicableVersions: ['free-pro-team@latest'], + renderProp: vi.fn(async (prop: string, context: Context) => { + const enPage = context.getEnglishPage!(context) + return prop === 'title' ? enPage.rawTitle : enPage.rawIntro + }), + } as unknown as Page + const enLanding = { rawTitle: 'English landing title' } as unknown as Page + + mockFindPage.mockImplementation((href) => { + if (href === '/ja/admin/audit-log') return jaArticle + if (href === '/en/admin/audit-log') return enArticle + return undefined + }) + + const req = createMockRequest( + { rawCarousels: { recommended: ['/admin/audit-log'] } }, + { currentLanguage: 'ja', getEnglishPage: () => enLanding }, + ) + + await resolveCarousels(req, mockRes, mockNext) + + expect( + (req.context!.page as Page & { carousels?: Record }).carousels, + ).toEqual({ + recommended: [ + { + title: 'English article title', + intro: 'English article intro', + href: '/admin/audit-log', + category: ['admin'], + }, + ], + }) + }) }) diff --git a/src/ghes-releases/lib/parse-release-notes.ts b/src/ghes-releases/lib/parse-release-notes.ts index b27fe866693f..080232749eb8 100644 --- a/src/ghes-releases/lib/parse-release-notes.ts +++ b/src/ghes-releases/lib/parse-release-notes.ts @@ -1,6 +1,5 @@ // generate-release-notes.ts uses side-effect-free parsers. // Tests can import them without starting the CLI. -import fs from 'fs' import { load } from 'js-yaml' export interface NoteEntry { @@ -67,14 +66,53 @@ export function parseNoteEntries(yamlStr: string, sourceUrl: string): NoteEntry[ return entries } -export function loadExistingEntries(yamlPath: string): { - entries: NoteEntry[] - coveredUrls: Set -} | null { - if (!fs.existsSync(yamlPath)) return null +const SECTION_KEY_PATTERN = /^ {2}([a-z_]+):/ + +// Notes in any other section, such as known_issues or errata, are hand-written and have no source issue. +const SECTION_HEADINGS: Record = { + changes: 'Changes', + closing_down: 'Closing down', + retired: 'Retired', +} + +const GENERATED_SECTIONS = new Set(['features', ...Object.keys(SECTION_HEADINGS)]) + +export interface PreservedBlocks { + header: string + knownIssues: string | null + otherSections: string[] +} + +// Keep the parts of an existing file that the generator cannot rebuild, so reruns do not drop writer edits. +export function extractPreservedBlocks(content: string): PreservedBlocks | null { + const lines = content.split('\n') + const sectionsIndex = lines.findIndex((line) => line.match(/^sections:/)) + if (sectionsIndex === -1) return null + + const blocks: { key: string; lines: string[] }[] = [] + for (const line of lines.slice(sectionsIndex + 1)) { + const keyMatch = line.match(SECTION_KEY_PATTERN) + if (keyMatch) { + blocks.push({ key: keyMatch[1], lines: [line] }) + } else if (blocks.length > 0) { + blocks[blocks.length - 1].lines.push(line) + } + } + + let knownIssues: string | null = null + const otherSections: string[] = [] + for (const block of blocks) { + if (GENERATED_SECTIONS.has(block.key)) continue + const text = block.lines.join('\n').trimEnd() + if (block.key === 'known_issues') knownIssues = text + else otherSections.push(text) + } - const content = fs.readFileSync(yamlPath, 'utf8') - return loadExistingEntriesFromString(content) + return { + header: lines.slice(0, sectionsIndex).join('\n'), + knownIssues, + otherSections, + } } // Parse release notes YAML manually so source issue URL comments stay attached to notes. @@ -91,6 +129,13 @@ export function loadExistingEntriesFromString(content: string): { let currentHeading: string | null = null let currentNote: string[] = [] let inNote = false + // Hand-written sections can use grouped headings too, so only generated sections may set a heading. + let inGeneratedSection = true + + const enterSection = (key: string) => { + inGeneratedSection = GENERATED_SECTIONS.has(key) + currentHeading = SECTION_HEADINGS[key] ?? null + } const flushNote = () => { if (currentSourceUrl && currentHeading && currentNote.length > 0) { @@ -115,37 +160,25 @@ export function loadExistingEntriesFromString(content: string): { inNote = false } - let currentSection: string | null = null - for (const line of lines) { const headingMatch = line.match(/^\s+-\s+heading:\s+(.+)/) if (headingMatch) { flushNote() - currentHeading = headingMatch[1].trim() + if (inGeneratedSection) currentHeading = headingMatch[1].trim() continue } - const sectionMatch = line.match(/^\s+(changes|closing_down|retired|known_issues):/) + const sectionMatch = line.match(SECTION_KEY_PATTERN) if (sectionMatch) { flushNote() - currentSection = sectionMatch[1] - if (currentSection === 'changes') currentHeading = 'Changes' - else if (currentSection === 'closing_down') currentHeading = 'Closing down' - else if (currentSection === 'retired') currentHeading = 'Retired' - else currentHeading = null // Known issues stay in the placeholder template. - continue - } - - if (line.match(/^\s+features:/)) { - flushNote() - currentSection = 'features' + enterSection(sectionMatch[1]) continue } const urlMatch = line.match(/^\s*#\s*(https:\/\/github\.com\/github\/releases\/issues\/\d+)/) if (urlMatch) { flushNote() - currentSourceUrl = urlMatch[1] + if (inGeneratedSection) currentSourceUrl = urlMatch[1] continue } @@ -162,25 +195,17 @@ export function loadExistingEntriesFromString(content: string): { flushNote() const reHeading = line.match(/^\s+-\s+heading:\s+(.+)/) if (reHeading) { - currentHeading = reHeading[1].trim() + if (inGeneratedSection) currentHeading = reHeading[1].trim() continue } - const reSection = line.match(/^\s+(changes|closing_down|retired|known_issues):/) + const reSection = line.match(SECTION_KEY_PATTERN) if (reSection) { - currentSection = reSection[1] - if (currentSection === 'changes') currentHeading = 'Changes' - else if (currentSection === 'closing_down') currentHeading = 'Closing down' - else if (currentSection === 'retired') currentHeading = 'Retired' - else currentHeading = null - continue - } - if (line.match(/^\s+features:/)) { - currentSection = 'features' + enterSection(reSection[1]) continue } const reUrl = line.match(/^\s*#\s*(https:\/\/github\.com\/github\/releases\/issues\/\d+)/) if (reUrl) { - currentSourceUrl = reUrl[1] + if (inGeneratedSection) currentSourceUrl = reUrl[1] continue } if (line.match(/^\s+-\s+\|\s*$/)) { @@ -211,32 +236,37 @@ export function buildReleaseNotesYaml( noteEntries: NoteEntry[], releaseCandidate: boolean, featureHeadings: string[], + preserved?: PreservedBlocks, ): string { const today = new Date().toISOString().split('T')[0] const lines: string[] = [] - lines.push(`date: '${today}'`) - lines.push(`release_candidate: ${releaseCandidate}`) - lines.push('deprecated: false') - - if (releaseCandidate) { - lines.push('intro: |') - lines.push( - ' > [!NOTE] Release candidate (RC) builds are intended solely for use in a test environment. Do not install an RC in a production environment.', - ) - lines.push(' >') - lines.push(' > Do not upgrade to an RC from a supported, earlier version.') - lines.push(' >') - lines.push( - ' > If {% data variables.location.product_location %} is running an RC, you cannot upgrade to the general availability (GA) release. You also cannot upgrade with a hotpatch.', - ) - lines.push('') - lines.push( - ' For upgrade instructions, see [AUTOTITLE](/admin/upgrading-your-instance/preparing-to-upgrade/overview-of-the-upgrade-process).', - ) + if (preserved) { + lines.push(preserved.header) } else { - lines.push('intro: |') - lines.push('') + lines.push(`date: '${today}'`) + lines.push(`release_candidate: ${releaseCandidate}`) + lines.push('deprecated: false') + + if (releaseCandidate) { + lines.push('intro: |') + lines.push( + ' > [!NOTE] Release candidate (RC) builds are intended solely for use in a test environment. Do not install an RC in a production environment.', + ) + lines.push(' >') + lines.push(' > Do not upgrade to an RC from a supported, earlier version.') + lines.push(' >') + lines.push( + ' > If {% data variables.location.product_location %} is running an RC, you cannot upgrade to the general availability (GA) release. You also cannot upgrade with a hotpatch.', + ) + lines.push('') + lines.push( + ' For upgrade instructions, see [AUTOTITLE](/admin/upgrading-your-instance/preparing-to-upgrade/overview-of-the-upgrade-process).', + ) + } else { + lines.push('intro: |') + lines.push('') + } } lines.push('sections:') @@ -275,11 +305,17 @@ export function buildReleaseNotesYaml( appendNoteLines(lines, changeEntries, ' ') } + const knownIssuesStart = lines.length lines.push('') lines.push(' known_issues:') lines.push(' # TODO: Add known issues from "GHES Release Note Tracking" project') lines.push(' - |') lines.push(' ...') + if (preserved) { + // Writers curate known issues by hand, so their block replaces the placeholder. + lines.length = knownIssuesStart + if (preserved.knownIssues) lines.push('', preserved.knownIssues) + } const closingEntries = otherEntries.filter((e) => e.heading === 'Closing down') if (closingEntries.length > 0) { @@ -295,6 +331,11 @@ export function buildReleaseNotesYaml( appendNoteLines(lines, retiredEntries, ' ') } + for (const section of preserved?.otherSections ?? []) { + lines.push('') + lines.push(section) + } + lines.push('') return lines.join('\n') } diff --git a/src/ghes-releases/scripts/README.md b/src/ghes-releases/scripts/README.md index c61761d7c4ec..551d2b94e8d5 100644 --- a/src/ghes-releases/scripts/README.md +++ b/src/ghes-releases/scripts/README.md @@ -49,6 +49,7 @@ npm run generate-release-notes -- --release [options] * The script is **incremental by default**. If a release notes file already exists, it loads those entries and only processes new issues. Use `--force` to start fresh. * If you run `--issue 6768`, it replaces just that issue's entry in the existing file. +* Reruns keep the existing `date`, `release_candidate`, `deprecated`, and `intro` fields, plus hand-written sections such as `known_issues` and `errata`. `--force` replaces them with the default template. * The file is updated after each issue, so if you cancel with Ctrl+C, you won't lose changes. * The Copilot agent follows the style rules defined in `.github/agents/ghes-release-notes.md`: present tense, `[AUTOTITLE]` links, proper Liquid variable usage, etc. diff --git a/src/ghes-releases/scripts/generate-release-notes.ts b/src/ghes-releases/scripts/generate-release-notes.ts index 9da6ed83aada..a2232ae845f0 100644 --- a/src/ghes-releases/scripts/generate-release-notes.ts +++ b/src/ghes-releases/scripts/generate-release-notes.ts @@ -13,10 +13,12 @@ import { load } from 'js-yaml' import { type NoteEntry, + type PreservedBlocks, extractYaml, extractSkipReason, parseNoteEntries, - loadExistingEntries, + loadExistingEntriesFromString, + extractPreservedBlocks, buildReleaseNotesYaml, appendNoteLines, } from '@/ghes-releases/lib/parse-release-notes' @@ -570,10 +572,13 @@ program // Existing entries make generation incremental by default. const allEntries: NoteEntry[] = [] let existingCoveredUrls = new Set() + let preserved: PreservedBlocks | undefined if (!force && !stdout && fs.existsSync(outputPath)) { - const existing = loadExistingEntries(outputPath) - if (existing && existing.entries.length > 0) { + const existingContent = fs.readFileSync(outputPath, 'utf8') + preserved = extractPreservedBlocks(existingContent) ?? undefined + const existing = loadExistingEntriesFromString(existingContent) + if (existing.entries.length > 0) { // Regenerate a single issue by dropping its previous source URL entry first. if (singleIssue) { const issueUrl = `https://github.com/github/releases/issues/${singleIssue}` @@ -629,7 +634,7 @@ program if (!fs.existsSync(outputDir)) { fs.mkdirSync(outputDir, { recursive: true }) } - const yamlOutput = buildReleaseNotesYaml(allEntries, rc, featureHeadings) + const yamlOutput = buildReleaseNotesYaml(allEntries, rc, featureHeadings, preserved) fs.writeFileSync(outputPath, yamlOutput, 'utf8') } catch (err) { console.error(` ⚠ Failed to write output file: ${(err as Error).message}`) diff --git a/src/ghes-releases/tests/generate-release-notes.ts b/src/ghes-releases/tests/generate-release-notes.ts index 02aee8cf6988..6d78f7826177 100644 --- a/src/ghes-releases/tests/generate-release-notes.ts +++ b/src/ghes-releases/tests/generate-release-notes.ts @@ -1,13 +1,46 @@ import { describe, expect, test } from 'vitest' +import { load } from 'js-yaml' import { extractYaml, extractSkipReason, parseNoteEntries, loadExistingEntriesFromString, + extractPreservedBlocks, buildReleaseNotesYaml, } from '@/ghes-releases/lib/parse-release-notes' +const handEditedFile = `date: '2026-05-12' +release_candidate: true +deprecated: true +intro: | + Custom intro written by hand. +sections: + + features: + + - heading: APIs + notes: + # https://github.com/github/releases/issues/100 + - | + New endpoint added. + + known_issues: + - | + First known issue. + - | + Second known issue. + + closing_down: + # https://github.com/github/releases/issues/300 + - | + Legacy API deprecated. + + errata: + - | + An erratum about a removed note. +` + describe('extractYaml', () => { test('extracts YAML from a fenced code block', () => { const input = `Here is the release note: @@ -254,6 +287,37 @@ sections: expect(result.entries).toHaveLength(1) expect(result.entries[0].notes).toEqual(['First note.', 'Second note from same issue.']) }) + + test('does not attribute errata or known issues to the previous source URL', () => { + const result = loadExistingEntriesFromString(handEditedFile) + const closing = result.entries.filter((e) => e.heading === 'Closing down') + expect(closing).toEqual([ + { + heading: 'Closing down', + notes: ['Legacy API deprecated.'], + sourceUrl: 'https://github.com/github/releases/issues/300', + }, + ]) + const allNotes = result.entries.flatMap((e) => e.notes) + expect(allNotes).not.toContain('An erratum about a removed note.') + }) +}) + +describe('extractPreservedBlocks', () => { + test('returns the header and the sections the generator does not write', () => { + const preserved = extractPreservedBlocks(handEditedFile) + expect(preserved).not.toBeNull() + expect(preserved!.header).toContain("date: '2026-05-12'") + expect(preserved!.header).toContain('deprecated: true') + expect(preserved!.header).toContain('Custom intro written by hand.') + expect(preserved!.knownIssues).toContain('Second known issue.') + expect(preserved!.otherSections).toHaveLength(1) + expect(preserved!.otherSections[0]).toMatch(/^ {2}errata:/) + }) + + test('returns null when there is no sections key', () => { + expect(extractPreservedBlocks("date: '2026-05-12'\n")).toBeNull() + }) }) describe('buildReleaseNotesYaml', () => { @@ -346,4 +410,87 @@ describe('buildReleaseNotesYaml', () => { expect(yaml).toContain('- |') expect(yaml).toContain('Line one of the note.') }) + + test('keeps hand-edited header, known issues, and errata on a rerun', () => { + const { entries } = loadExistingEntriesFromString(handEditedFile) + const preserved = extractPreservedBlocks(handEditedFile)! + const yaml = buildReleaseNotesYaml(entries, true, featureHeadings, preserved) + const parsed = load(yaml) as { + date: string + deprecated: boolean + intro: string + sections: Record + } + + expect(parsed.date).toBe('2026-05-12') + expect(parsed.deprecated).toBe(true) + expect(parsed.intro).toBe('Custom intro written by hand.\n') + expect(parsed.sections.known_issues).toEqual(['First known issue.\n', 'Second known issue.\n']) + expect(parsed.sections.errata).toEqual(['An erratum about a removed note.\n']) + expect(parsed.sections.closing_down).toEqual(['Legacy API deprecated.\n']) + expect(parsed.sections.features).toEqual([ + { heading: 'APIs', notes: ['New endpoint added.\n'] }, + ]) + }) + + test('keeps grouped known issues and errata out of generated entries across reruns', () => { + const groupedFile = `date: '2026-05-12' +release_candidate: false +deprecated: false +intro: | +sections: + + features: + + - heading: APIs + notes: + # https://github.com/github/releases/issues/100 + - | + New endpoint added. + + known_issues: + - heading: APIs + notes: + - | + A grouped API known issue. + + errata: + - heading: Repositories + notes: + - | + A grouped erratum. +` + const { entries } = loadExistingEntriesFromString(groupedFile) + expect(entries).toEqual([ + { + heading: 'APIs', + notes: ['New endpoint added.'], + sourceUrl: 'https://github.com/github/releases/issues/100', + }, + ]) + + const firstRun = buildReleaseNotesYaml( + entries, + false, + featureHeadings, + extractPreservedBlocks(groupedFile)!, + ) + const secondRun = buildReleaseNotesYaml( + loadExistingEntriesFromString(firstRun).entries, + false, + featureHeadings, + extractPreservedBlocks(firstRun)!, + ) + expect(secondRun).toBe(firstRun) + const parsed = load(secondRun) as { sections: Record } + expect(parsed.sections.features).toEqual([ + { heading: 'APIs', notes: ['New endpoint added.\n'] }, + ]) + expect(parsed.sections.known_issues).toEqual([ + { heading: 'APIs', notes: ['A grouped API known issue.\n'] }, + ]) + expect(parsed.sections.errata).toEqual([ + { heading: 'Repositories', notes: ['A grouped erratum.\n'] }, + ]) + }) }) diff --git a/src/landings/components/SidebarProduct.tsx b/src/landings/components/SidebarProduct.tsx index 030a03070d31..6480bcb99be4 100644 --- a/src/landings/components/SidebarProduct.tsx +++ b/src/landings/components/SidebarProduct.tsx @@ -304,12 +304,10 @@ const NavListItem = memo(function NavListItem({ }) { const nav = useSidebarNav() const { routePath, navigate, prefetch } = nav - const locale = routePath.split('/')[1] const hasChildren = childPage.childPages.length > 0 const specialCategory = childPage.layout === 'category-landing' const canNest = level < MAX_NAVLIST_LEVEL - // sidebarLink.href lacks a locale prefix; add it so href, aria-current, and navigation agree. - const sidebarLinkHref = childPage.sidebarLink ? `/${locale}${childPage.sidebarLink.href}` : '' + const sidebarLinkHref = childPage.sidebarLink?.href || '' // Leaf nodes use anchors so Brand draws the active bar from aria-current page. if (!hasChildren) { diff --git a/src/landings/tests/sidebar-custom-links.ts b/src/landings/tests/sidebar-custom-links.ts index 7b2c9f0aef89..437da4d53c1e 100644 --- a/src/landings/tests/sidebar-custom-links.ts +++ b/src/landings/tests/sidebar-custom-links.ts @@ -49,6 +49,25 @@ describe('sidebar custom links', () => { expect(customLink.text().trim()).toBe('All sidebar test items') }) + test('sidebar custom link keeps the current version', async () => { + { + const $ = await getDOM('/enterprise-cloud@latest/get-started/sidebar-test') + const customLink = $('[data-testid="sidebar"] a:contains("All sidebar test items")') + expect(customLink.length).toBe(1) + expect(customLink.attr('href')).toBe('/en/enterprise-cloud@latest/get-started/sidebar-test') + expect(customLink.attr('aria-current')).toBe('page') + } + { + const $ = await getDOM('/enterprise-server@latest/get-started/sidebar-test') + const customLink = $('[data-testid="sidebar"] a:contains("All sidebar test items")') + expect(customLink.length).toBe(1) + expect(customLink.attr('href')).toMatch( + /^\/en\/enterprise-server@\d+\.\d+\/get-started\/sidebar-test$/, + ) + expect(customLink.attr('aria-current')).toBe('page') + } + }) + test('sidebar custom link does not appear on unrelated pages', async () => { // The /actions page avoids the get-started section, which has fixture sidebarLink data. const $ = await getDOM('/actions') diff --git a/src/languages/lib/correct-translation-content.ts b/src/languages/lib/correct-translation-content.ts index 56a2632d1d5b..09e63a409af4 100644 --- a/src/languages/lib/correct-translation-content.ts +++ b/src/languages/lib/correct-translation-content.ts @@ -669,10 +669,15 @@ export function correctTranslatedContentStrings( 'você precisa instalá-lo na sua {% ifversion enterprise-installed-apps %}empresa, {% endif %}organização ou conta pessoal.', ) - content = content.replaceAll( - 'tornando mais difícil para os atores mal-intencionados acessarem os repositórios e as configurações de uma organização.', - 'tornando mais difícil para os atores mal-intencionados acessarem os repositórios e as configurações de uma organização.{% endif %}', - ) + // Other pages reuse this sentence without an open conditional, so the extra endif breaks them. + if ( + context.relativePath?.endsWith('/requiring-two-factor-authentication-in-your-organization.md') + ) { + content = content.replaceAll( + 'tornando mais difícil para os atores mal-intencionados acessarem os repositórios e as configurações de uma organização.', + 'tornando mais difícil para os atores mal-intencionados acessarem os repositórios e as configurações de uma organização.{% endif %}', + ) + } content = content.replaceAll( 'nos repositórios internos e {% endif %}privados {% ifversion ghec %}da sua organização.', @@ -954,6 +959,11 @@ export function correctTranslatedContentStrings( /(\{%-?\s*indented_data_reference\s+)повторн[а-яё]*(?:\s+[а-яё]+)*\./g, '$1reusables.', ) + // Genitive-case transliteration of octicon + content = content.replaceAll('{% октикона ', '{% octicon ') + content = content.replaceAll('{%- октикона ', '{%- octicon ') + content = content.replaceAll('{% oктикoнa ', '{% octicon ') + content = content.replaceAll('{%- oктикoнa ', '{%- octicon ') content = content.replaceAll('{% еще %}', '{% else %}') content = content.replaceAll('{%- еще %}', '{%- else %}') content = content.replaceAll('{% ещё %}', '{% else %}') @@ -1325,6 +1335,11 @@ export function correctTranslatedContentStrings( content = content.replaceAll('{%- 들여쓰기_데이터_참조 ', '{%- indented_data_reference ') content = content.replaceAll('{% 옥티콘 ', '{% octicon ') content = content.replaceAll('{%- 옥티콘 ', '{%- octicon ') + // "자료" (material) substituted for "data" in data variables/reusables tags + content = content.replaceAll('{% 자료 변수.', '{% data variables.') + content = content.replaceAll('{%- 자료 변수.', '{%- data variables.') + content = content.replaceAll('{% 자료 재사용 가능.', '{% data reusables.') + content = content.replaceAll('{%- 자료 재사용 가능.', '{%- data reusables.') content = content.replaceAll('{% 행 머리글 %}', '{% rowheaders %}') content = content.replaceAll('{%- 행 머리글 %}', '{%- rowheaders %}') content = content.replaceAll('{% 행머리글 %}', '{% rowheaders %}') @@ -1962,7 +1977,28 @@ export function correctTranslatedContentStrings( } } - content = content.replaceAll(' | | ', ' |\n| ') + // ' | | ' is also a legitimate empty table cell, so keep a row whose empty-cell layout matches an English row. + { + const rowShape = (line: string) => + line + .split('|') + .map((cell) => (cell.trim() ? 'x' : '')) + .join('|') + const englishRowShapes = new Set( + englishContent + .split('\n') + .filter((line) => line.includes(' | | ')) + .map(rowShape), + ) + content = content + .split('\n') + .map((line) => + line.includes(' | | ') && !englishRowShapes.has(rowShape(line)) + ? line.replaceAll(' | | ', ' |\n| ') + : line, + ) + .join('\n') + } // Run after specific fixes. Swap only when no tag sits between, so nesting survives. { diff --git a/src/languages/tests/api-search.ts b/src/languages/tests/api-search.ts index 06f4219ff911..8bafc4ccb40c 100644 --- a/src/languages/tests/api-search.ts +++ b/src/languages/tests/api-search.ts @@ -3,6 +3,8 @@ import { expect, test, vi } from 'vitest' import { describeIfElasticsearchURL } from '@/tests/helpers/conditional-runs' import { get } from '@/tests/helpers/e2etest' +import type { CombinedSearchResponse } from '@/search/types' + // This suite only runs if $ELASTICSEARCH_URL is set. // The basic Japanese search test uses localized /ja/foo and /ja/bar records from // src/search/tests/fixtures/search-indexes/tests_github-docs_general-search_fpt_ja-records.json. @@ -36,3 +38,30 @@ describeIfElasticsearchURL('search v1 middleware in non-English', () => { expect(hit.breadcrumbs).toBe('fooing') }) }) + +// The search suite enables only English, so non-English combined search coverage lives here. +describeIfElasticsearchURL('combined search v1 middleware in non-English', () => { + vi.setConfig({ testTimeout: 60 * 1000 }) + + test('Japanese uses English AI suggestions and localized general results', async () => { + const sp = new URLSearchParams() + sp.set('language', 'ja') + { + sp.set('query', 'rest') + const res = await get(`/api/search/combined-search/v1?${sp}`) + expect(res.statusCode).toBe(200) + const results = JSON.parse(res.body) as CombinedSearchResponse + expect(results.aiAutocompleteSuggestions.hits[0].term).toBe( + 'How do I manage OAuth app access restrictions for my organization?', + ) + } + { + sp.set('query', 'foo') + const res = await get(`/api/search/combined-search/v1?${sp}`) + expect(res.statusCode).toBe(200) + const results = JSON.parse(res.body) as CombinedSearchResponse + const titles = results.generalSearchResults.hits.map((hit) => hit.title) + expect(titles).toContain('フー') + } + }) +}) diff --git a/src/languages/tests/correct-translation-content.ts b/src/languages/tests/correct-translation-content.ts index 37a21d2016ca..b3b69b0f6259 100644 --- a/src/languages/tests/correct-translation-content.ts +++ b/src/languages/tests/correct-translation-content.ts @@ -651,6 +651,35 @@ describe('correctTranslatedContentStrings', () => { ), ).toBe('{% data reusables.audit_log.reference-grouped-by-category %}') }) + + describe('2FA intro missing endif', () => { + function fixAt(content: string, relativePath: string) { + return correctTranslatedContentStrings(content, '', { + code: 'pt', + relativePath, + skipOrphanStripping: true, + }) + } + const sentence = + 'tornando mais difícil para os atores mal-intencionados acessarem os repositórios e as configurações de uma organização.' + + test('appends endif in requiring-two-factor-authentication-in-your-organization intro', () => { + const broken = `{% ifversion fpt or ghec %}membros e gerentes de cobrança{% else %}membros ativem a autenticação de dois fatores, ${sentence}` + expect( + fixAt( + broken, + 'organizations/keeping-your-organization-secure/managing-two-factor-authentication-for-your-organization/requiring-two-factor-authentication-in-your-organization.md', + ), + ).toBe(`${broken}{% endif %}`) + }) + + test('leaves the same sentence alone in other files', () => { + const text = `* Exigir 2FA, ${sentence}{% ifversion ghec %} x{% endif %} Para obter mais informações.` + expect( + fixAt(text, 'code-security/tutorials/secure-your-organization/prevent-data-leaks.md'), + ).toBe(text) + }) + }) }) describe('Chinese (zh)', () => { @@ -900,6 +929,14 @@ describe('correctTranslatedContentStrings', () => { ) }) + test('fixes октикона → octicon', () => { + expect(fix('{% октикона "плюс" aria-label="plus" %}', 'ru')).toBe( + '{% octicon "плюс" aria-label="plus" %}', + ) + expect(fix('{%- октикона "plus" %}', 'ru')).toBe('{%- octicon "plus" %}') + expect(fix('{% octicon "plus" %}', 'ru')).toBe('{% octicon "plus" %}') + }) + test('fixes translated raw/endraw', () => { expect(fix('{% необработанного %}', 'ru')).toBe('{% raw %}') expect(fix('{%- необработанного %}', 'ru')).toBe('{%- raw %}') @@ -1382,6 +1419,18 @@ describe('correctTranslatedContentStrings', () => { expect(fix('{%- 그렇지 않으면 %}', 'ko')).toBe('{%- else %}') }) + test('fixes 자료 → data in variables/reusables tags', () => { + expect(fix('{% 자료 변수.product.prodname_vs %}', 'ko')).toBe( + '{% data variables.product.prodname_vs %}', + ) + expect(fix('{%- 자료 재사용 가능.enterprise.save %}', 'ko')).toBe( + '{%- data reusables.enterprise.save %}', + ) + expect(fix('{% data variables.product.prodname_vs %}', 'ko')).toBe( + '{% data variables.product.prodname_vs %}', + ) + }) + test('fixes 옥티콘 → octicon', () => { expect(fix('{% 옥티콘 "check" aria-label="Supported" %}', 'ko')).toBe( '{% octicon "check" aria-label="Supported" %}', @@ -2082,6 +2131,38 @@ describe('correctTranslatedContentStrings', () => { expect(fix('Cell1 | | Cell2', 'es')).toBe('Cell1 |\n| Cell2') }) + test('keeps empty table cells that the English source also has', () => { + const english = '| `displayName` | `string` | | Human-readable name |' + const translated = '| `displayName` | `string` | | 名前 |' + expect(fix(translated, 'ja', english)).toBe(translated) + }) + + test('fixes collapsed rows on a page that also has empty table cells', () => { + const english = [ + '| `displayName` | `string` | | Human-readable name |', + '| `name` | `string` | Yes | Unique name |', + '', + '| Agent | Config |', + '| Subagent | Runtime |', + ].join('\n') + const translated = [ + '| `displayName` | `string` | | 名前 |', + '| `name` | `string` | Yes | 一意 | | `tools` | `string[]` | No | ツール |', + '', + '| エージェント | 構成 | | サブエージェント | ランタイム |', + ].join('\n') + expect(fix(translated, 'ja', english)).toBe( + [ + '| `displayName` | `string` | | 名前 |', + '| `name` | `string` | Yes | 一意 |', + '| `tools` | `string[]` | No | ツール |', + '', + '| エージェント | 構成 |', + '| サブエージェント | ランタイム |', + ].join('\n'), + ) + }) + test('fixes final catch-all double-brace patterns', () => { expect(fix('{{% data variables.product.github %}', 'es')).toBe( '{% data variables.product.github %}', diff --git a/src/redirects/lib/get-redirect.ts b/src/redirects/lib/get-redirect.ts index 07bca4cfff59..1dd556aca8f6 100644 --- a/src/redirects/lib/get-redirect.ts +++ b/src/redirects/lib/get-redirect.ts @@ -154,7 +154,7 @@ export default function getRedirect(uri: string, context: Context): string | und if (supported.includes(version) || version === 'latest') { prefix = `/${majorVersion}@${version}` - suffix = `/${split.slice(2).join('/')}` + suffix = split.length > 2 ? `/${split.slice(2).join('/')}` : '' if ( suffix.includes('/user') || @@ -166,7 +166,7 @@ export default function getRedirect(uri: string, context: Context): string | und } else { // Unsupported versions still need prefix and suffix values for the fallback lookup. prefix = `/${majorVersion}@${version}` - suffix = `/${split.slice(2).join('/')}` + suffix = split.length > 2 ? `/${split.slice(2).join('/')}` : '' } const newURL = prefix + suffix diff --git a/src/redirects/middleware/handle-redirects.ts b/src/redirects/middleware/handle-redirects.ts index 0b25be9fb03c..d009e6d5f697 100644 --- a/src/redirects/middleware/handle-redirects.ts +++ b/src/redirects/middleware/handle-redirects.ts @@ -100,6 +100,8 @@ export default function handleRedirects(req: ExtendedRequest, res: Response, nex } redirect = queryParams ? redirectWithoutQueryParams + queryParams : redirectWithoutQueryParams + // Query values can contain ://, so check only the destination path. + const isExternal = redirectWithoutQueryParams.includes('://') if (!redirectTo && !pathLanguagePrefixed(req.path)) { // Add a language prefix only for pages or deprecated versions; /healthcheck passes through. @@ -119,7 +121,7 @@ export default function handleRedirects(req: ExtendedRequest, res: Response, nex if (!req.context.pages) throw new Error('req.context.pages not yet set') - if (!redirect.includes('://')) { + if (!isExternal) { const preference = getVersionPreference( req.path, removeQueryParams(redirect), @@ -145,7 +147,7 @@ export default function handleRedirects(req: ExtendedRequest, res: Response, nex req.context.pages[removeQueryParams(redirect).replace(/\.md$/, '')] || isDeprecatedVersion(req.path) ) && - !redirect.includes('://') + !isExternal ) { // Development responses expose the missing redirect target; production keeps the page clean. if (process.env.NODE_ENV !== 'production' && req.context) { @@ -155,13 +157,13 @@ export default function handleRedirects(req: ExtendedRequest, res: Response, nex } // Language-prefixed and external redirects do not vary by language preference. - if (pathLanguagePrefixed(req.path) || redirect.includes('://')) { + if (pathLanguagePrefixed(req.path) || isExternal) { defaultCacheControl(res) } else { languageCacheControl(res) } - const permanent = redirect.includes('://') || usePermanentRedirect(req) + const permanent = isExternal || usePermanentRedirect(req) return res.safeRedirect(permanent ? 301 : 302, redirect) } diff --git a/src/redirects/tests/redirects.ts b/src/redirects/tests/redirects.ts index 138303aeb376..cdc401b1dceb 100644 --- a/src/redirects/tests/redirects.ts +++ b/src/redirects/tests/redirects.ts @@ -59,6 +59,12 @@ describe('redirects', () => { expect(res.headers.location).toBe(expected) }) + test('have q= converted to query= on Enterprise Cloud', async () => { + const res = await get('/en/enterprise-cloud@latest/admin?q=pulls') + expect(res.statusCode).toBe(301) + expect(res.headers.location).toBe('/en/enterprise-cloud@latest/search?query=pulls') + }) + test('convert q= to query= on search page', async () => { const res = await get('/en/search?q=pulls') expect(res.statusCode).toBe(301) diff --git a/src/redirects/tests/unit/get-redirect.ts b/src/redirects/tests/unit/get-redirect.ts index 25ce8fca5b1e..d33c78471147 100644 --- a/src/redirects/tests/unit/get-redirect.ts +++ b/src/redirects/tests/unit/get-redirect.ts @@ -60,6 +60,19 @@ describe('getRedirect basics', () => { ) }) + // handle-redirects appends /search to this result, so a trailing slash would produce //search. + test('should not redirect language-prefixed version home pages', () => { + const ctx: TestContext = { + pages: {}, + redirects: {}, + } + expect(getRedirect('/en/enterprise-cloud@latest', ctx as unknown as Context)).toBeUndefined() + expect(getRedirect('/ja/enterprise-cloud@latest', ctx as unknown as Context)).toBeUndefined() + expect( + getRedirect(`/en/enterprise-server@${latest}`, ctx as unknown as Context), + ).toBeUndefined() + }) + test('should always "remove" the free-pro-team prefix', () => { const ctx = { pages: {}, diff --git a/src/redirects/tests/unit/handle-redirects.ts b/src/redirects/tests/unit/handle-redirects.ts new file mode 100644 index 000000000000..d37c803d5910 --- /dev/null +++ b/src/redirects/tests/unit/handle-redirects.ts @@ -0,0 +1,102 @@ +import { describe, expect, test } from 'vitest' +import type { NextFunction, Response } from 'express' + +import handleRedirects from '@/redirects/middleware/handle-redirects' +import type { Context, ExtendedRequest } from '@/types' + +type TestResponse = { + headers: Record + statusCode?: number + location?: string +} + +function run( + url: string, + { + userLanguage = 'en', + userVersion, + pages = {}, + redirects = {}, + }: { + userLanguage?: string + userVersion?: string + pages?: Record + redirects?: Record + } = {}, +) { + const parsed = new URL(url, 'https://docs.github.com') + const req = { + path: parsed.pathname, + url, + originalUrl: url, + query: Object.fromEntries(parsed.searchParams), + userVersion, + context: { userLanguage, pages, redirects } as unknown as Context, + } as unknown as ExtendedRequest + + const result: TestResponse = { headers: {} } + const res = { + set(key: string, value: string) { + result.headers[key.toLowerCase()] = value + return res + }, + append(key: string, value: string) { + const name = key.toLowerCase() + result.headers[name] = result.headers[name] ? `${result.headers[name]}, ${value}` : value + return res + }, + hasHeader(key: string) { + return key.toLowerCase() in result.headers + }, + safeRedirect(status: number, location: string) { + result.statusCode = status + result.location = location + }, + } as unknown as Response + + let nextCalled = false + const next: NextFunction = () => { + nextCalled = true + } + handleRedirects(req, res, next) + return { ...result, nextCalled } +} + +describe('handleRedirects with :// in the query string', () => { + test('language redirect stays temporary and varies by language', () => { + const result = run('/actions?ref=https://github.com', { + userLanguage: 'ja', + pages: { '/en/actions': {}, '/ja/actions': {} }, + }) + expect(result.statusCode).toBe(302) + expect(result.location).toBe('/ja/actions?ref=https://github.com') + expect(result.headers.vary).toMatch(/accept-language/) + expect(result.headers.vary).toMatch(/x-user-language/) + }) + + test('version cookie still redirects to the preferred version', () => { + const result = run('/en/actions?utm_source=https://x.example', { + userVersion: 'enterprise-cloud@latest', + pages: { '/en/actions': {}, '/en/enterprise-cloud@latest/actions': {} }, + }) + expect(result.statusCode).toBe(302) + expect(result.location).toBe('/en/enterprise-cloud@latest/actions?utm_source=https://x.example') + expect(result.headers.vary).toMatch(/x-user-version/) + }) + + test('internal redirect to a missing page falls through', () => { + const result = run('/old-article?ref=https://github.com', { + redirects: { '/old-article': '/missing-article' }, + }) + expect(result.nextCalled).toBe(true) + expect(result.statusCode).toBeUndefined() + }) + + test('external redirect stays permanent', () => { + const result = run('/external?ref=abc', { + redirects: { '/external': 'https://example.com/docs' }, + }) + expect(result.statusCode).toBe(301) + expect(result.location).toBe('https://example.com/docs?ref=abc') + }) +}) diff --git a/src/rest/components/get-rest-code-samples.ts b/src/rest/components/get-rest-code-samples.ts index 491fae97cd1c..a1c8369a17fd 100644 --- a/src/rest/components/get-rest-code-samples.ts +++ b/src/rest/components/get-rest-code-samples.ts @@ -133,6 +133,10 @@ export function getGHExample( const requiredQueryParams = getRequiredQueryParamsPath(operation, codeSample) requestPath += requiredQueryParams ? `?${requiredQueryParams}` : '' + // Quote paths containing ? so shells don't expand it as a glob or split on &. + if (requestPath.includes('?')) { + requestPath = `"${requestPath}"` + } let requestBodyParams = '' // Request bodies can be named object parameters or a single scalar value. diff --git a/src/rest/scripts/utils/create-rest-examples.ts b/src/rest/scripts/utils/create-rest-examples.ts index 6a94996a55a6..5539c0f5146c 100644 --- a/src/rest/scripts/utils/create-rest-examples.ts +++ b/src/rest/scripts/utils/create-rest-examples.ts @@ -103,7 +103,9 @@ export default async function getCodeSamples( } // mergeExamples applies direct, status-code, and example-key rules to pair requests with responses. -// If earlier rules do not apply, the fallback path matches request and response example keys. +// If earlier rules do not apply, the fallback path matches request and response example keys, +// then pairs every request with a success response when no keys match. +// That fallback prefers a 201 because a 200 beside it can mean the resource already existed. export function mergeExamples( requestExamples: RequestExample[], responseExamples: ResponseExample[], @@ -145,13 +147,22 @@ export function mergeExamples( const source = requestsExamplesLarger ? responseExamples : requestExamples // The longer list drives key matching. Requests win ties on length, and the first key match wins. - return target + const keyMatched = target .filter((targetEx) => { const match = source.find((srcEx) => srcEx.key === targetEx.key) if (match) return Object.assign(targetEx, match) return false }) .map((ex) => ex as MergedExample) + + if (keyMatched.length > 0 || responseExamples.length === 0) return keyMatched + + // Request and response example keys often differ, so pair every request with one success response. + const fallback = + responseExamples.find((ex) => ex.response.statusCode === '201') || + responseExamples.find((ex) => parseInt(ex.response.statusCode, 10) < 300) || + responseExamples[0] + return requestExamples.map((req) => ({ ...req, response: fallback.response })) } // Request examples fall back to path parameters or generic examples when bodies lack examples. diff --git a/src/rest/scripts/utils/get-body-params.ts b/src/rest/scripts/utils/get-body-params.ts index aa96b09c9e3e..787f414bd12b 100644 --- a/src/rest/scripts/utils/get-body-params.ts +++ b/src/rest/scripts/utils/get-body-params.ts @@ -105,6 +105,13 @@ async function handleObjectOnlyOneOf( return [] } +// Array-level nullability already renders as "or null", so a null item type adds nothing to the label. +function getArrayItemTypes(items: Schema): string[] { + return (Array.isArray(items.type) ? items.type : [items.type]).filter( + (t): t is string => t !== undefined && t !== 'null', + ) +} + // OpenAPI 3.0 allows one type value, while OpenAPI 3.1 also allows an array, so getBodyParams // normalizes type values to arrays before it builds the rendered type string. // For child parameters, getBodyParams reads object-valued additionalProperties recursively. @@ -123,12 +130,16 @@ export async function getBodyParams(schema: Schema, topLevel = false): Promise 0) { + paramType.splice( + paramType.indexOf('array'), + 1, + `array of ${itemTypes.map((t) => `${t}s`).join(' or ')}`, + ) } const paramDecorated = await getTransformedParam(schema, paramType, { required, @@ -172,15 +183,19 @@ export async function getBodyParams(schema: Schema, topLevel = false): Promise 0) { + paramType.splice( + paramType.indexOf('array'), + 1, + `array of ${itemTypes.map((t) => `${t}s`).join(' or ')}`, + ) } - if (arrayType === 'object') { + if (itemTypes.includes('object')) { childParamsGroups.push(...(await getBodyParams(param.items, false))) } - if (arrayType === 'string' && param.items.enum) { - param.description += `${ + if (itemTypes.includes('string') && param.items.enum) { + param.description = `${param.description || ''}${ param.description ? '\n' : '' }Supported values are: ${((param.items.enum || []) as string[]).map((lang: string) => `${lang}`).join(', ')}` } diff --git a/src/rest/scripts/utils/tests/get-body-params.test.ts b/src/rest/scripts/utils/tests/get-body-params.test.ts index 7210ea7780e7..c39779f2d727 100644 --- a/src/rest/scripts/utils/tests/get-body-params.test.ts +++ b/src/rest/scripts/utils/tests/get-body-params.test.ts @@ -324,4 +324,87 @@ describe('getBodyParams — OAS 3.1 nullable handling', () => { expect(params[0].type).toBe('string') expect(params[0].isRequired).toBe(true) }) + + it('recurses into array items with type ["object", "null"]', async () => { + const schema: Schema = { + type: 'object', + properties: { + assignees: { + type: 'array', + items: { + type: ['object', 'null'], + properties: { + login: { type: 'string', description: 'The login' }, + }, + }, + }, + }, + } + const params = await getBodyParams(schema, false) + expect(params).toHaveLength(1) + expect(params[0].type).toBe('array of objects') + expect(params[0].childParamsGroups?.map(({ name }) => name)).toEqual(['login']) + }) + + it('renders a nullable array of ["string", "null"] items as "array of strings or null"', async () => { + const schema: Schema = { + type: 'object', + properties: { + bullets: { + type: ['array', 'null'], + items: { type: ['string', 'null'] }, + }, + }, + } + const params = await getBodyParams(schema, false) + expect(params).toHaveLength(1) + expect(params[0].type).toBe('array of strings or null') + }) + + it('adds supported values for enum array items with type ["string", "null"]', async () => { + const schema: Schema = { + type: 'object', + properties: { + tags: { + type: 'array', + description: 'Tags.', + items: { type: ['string', 'null'], enum: ['a', 'b'] }, + }, + }, + } + const params = await getBodyParams(schema, false) + expect(params).toHaveLength(1) + expect(params[0].description).toContain('Supported values are: a, b') + }) + + it('adds supported values without a prefix when nullable enum items have no array description', async () => { + const schema: Schema = { + type: 'object', + properties: { + tags: { + type: 'array', + items: { type: ['string', 'null'], enum: ['a', 'b'] }, + }, + }, + } + const params = await getBodyParams(schema, false) + expect(params).toHaveLength(1) + expect(params[0].description).toBe('Supported values are: a, b') + }) + + it('recurses into top-level array items with type ["object", "null"]', async () => { + const schema: Schema = { + type: 'array', + items: { + type: ['object', 'null'], + properties: { + id: { type: 'integer', description: 'The ID' }, + }, + }, + } + const params = await getBodyParams(schema, true) + expect(params).toHaveLength(1) + expect(params[0].type).toBe('array') + expect(params[0].childParamsGroups?.map(({ name }) => name)).toEqual(['id']) + }) }) diff --git a/src/rest/tests/cli-examples.ts b/src/rest/tests/cli-examples.ts index bd07ac94569d..2f7a690ee701 100644 --- a/src/rest/tests/cli-examples.ts +++ b/src/rest/tests/cli-examples.ts @@ -160,4 +160,57 @@ describe('CLI examples generation', () => { expect(result).toContain("Here'\\''s my feedback") expect(result).toContain('--data-urlencode') }) + + test('GitHub CLI example quotes paths with required query params', () => { + const mockSearchOperation = { + verb: 'get', + requestPath: '/search/labels', + serverUrl: 'https://api.github.com', + subcategory: 'search', + parameters: [ + { name: 'repository_id', in: 'query', required: true }, + { name: 'q', in: 'query', required: true }, + { name: 'sort', in: 'query', required: false }, + ], + } as unknown as Operation + const codeSample = { request: {} } as unknown as CodeSample + + const result = getGHExample( + mockSearchOperation, + codeSample, + 'free-pro-team@latest', + mockVersions, + ) + + expect(result).toContain('"/search/labels?repository_id=REPOSITORY_ID&q=Q"') + }) + + test('GitHub CLI example leaves paths without query params unquoted', () => { + const codeSample = { + request: { + parameters: { + owner: 'OWNER', + repo: 'REPO', + }, + }, + } as unknown as CodeSample + + const mockSimpleOperation = { + verb: 'get', + requestPath: '/repos/{owner}/{repo}', + serverUrl: 'https://api.github.com', + subcategory: 'repos', + parameters: [], + } as unknown as Operation + + const result = getGHExample( + mockSimpleOperation, + codeSample, + 'free-pro-team@latest', + mockVersions, + ) + + expect(result).toContain(' /repos/OWNER/REPO') + expect(result).not.toContain('"/repos/OWNER/REPO"') + }) }) diff --git a/src/rest/tests/create-rest-examples.ts b/src/rest/tests/create-rest-examples.ts index e97051859eab..5677796d8b70 100644 --- a/src/rest/tests/create-rest-examples.ts +++ b/src/rest/tests/create-rest-examples.ts @@ -50,6 +50,129 @@ describe('rest example requests and responses', () => { expect(mergedExamples).toBe(JSON.stringify(examples)) }) + test('multiple request examples pair with a contentless response that has an unmatched key', () => { + const requests = [ + { + key: 'by-subject-digests', + request: { + contentType: 'application/json', + description: 'Delete by subject digests', + acceptHeader: 'application/vnd.github.v3+json', + bodyParameters: { subject_digests: ['sha256:abc123'] }, + }, + }, + { + key: 'by-attestation-ids', + request: { + contentType: 'application/json', + description: 'Delete by attestation IDs', + acceptHeader: 'application/vnd.github.v3+json', + bodyParameters: { attestation_ids: [111, 222] }, + }, + }, + ] + const responses = [{ key: '200', response: { statusCode: '200', description: 'Response' } }] + const examples = mergeExamples(requests, responses) + expect(examples.length).toBe(2) + expect(examples.map((ex) => ex.request.description)).toEqual([ + 'Delete by subject digests', + 'Delete by attestation IDs', + ]) + for (const example of examples) { + expect(example.response?.statusCode).toBe('200') + } + }) + + test('multiple request examples pair with the success response when no keys match', () => { + const requests = [ + { + key: 'issue', + request: { + contentType: 'application/json', + description: 'Create a draft issue', + acceptHeader: 'application/vnd.github.v3+json', + bodyParameters: { title: 'Draft' }, + }, + }, + { + key: 'issue-with-body', + request: { + contentType: 'application/json', + description: 'Create a draft issue with body', + acceptHeader: 'application/vnd.github.v3+json', + bodyParameters: { title: 'Draft', body: 'Body' }, + }, + }, + ] + const responses = [ + { + key: 'draft-issue', + response: { + statusCode: '201', + contentType: 'application/json', + description: 'Response', + example: { id: 1 }, + }, + }, + { key: '304', response: { statusCode: '304', description: 'Not modified' } }, + ] + const examples = mergeExamples(requests, responses) + expect(examples.length).toBe(2) + for (const example of examples) { + expect(example.response?.statusCode).toBe('201') + expect(example.response?.example).toEqual({ id: 1 }) + } + }) + + test('multiple request examples prefer a 201 over a 200 when no keys match', () => { + const requests = [ + { + key: 'repository', + request: { + contentType: 'application/json', + description: 'Add a repository', + acceptHeader: 'application/vnd.github.v3+json', + bodyParameters: { resource_type: 'repository' }, + }, + }, + { + key: 'free_text', + request: { + contentType: 'application/json', + description: 'Add free text', + acceptHeader: 'application/vnd.github.v3+json', + bodyParameters: { resource_type: 'free_text' }, + }, + }, + ] + const responses = [ + { + key: '200', + response: { + statusCode: '200', + contentType: 'application/json', + description: 'Duplicate github_file resource already exists', + example: { id: 1 }, + }, + }, + { + key: '201', + response: { + statusCode: '201', + contentType: 'application/json', + description: 'Resource created', + example: { id: 2 }, + }, + }, + ] + const examples = mergeExamples(requests, responses) + expect(examples.length).toBe(2) + for (const example of examples) { + expect(example.response?.statusCode).toBe('201') + expect(example.response?.description).toBe('Resource created') + } + }) + test('check example number and status code appear', async () => { const mergedExamples = await getCodeSamples(operation) for (let index = 0; index < mergedExamples.length; index++) { diff --git a/src/search/components/helpers/execute-search-actions.ts b/src/search/components/helpers/execute-search-actions.ts index 5c299ecf29c1..7ac09c3d7da9 100644 --- a/src/search/components/helpers/execute-search-actions.ts +++ b/src/search/components/helpers/execute-search-actions.ts @@ -62,6 +62,9 @@ export function executeGeneralSearch( if (params.has('search-overlay-open')) { params.delete('search-overlay-open') } + // A new query starts on page 1 of all results, so an old page or category filter cannot leave it empty. + params.delete('page') + params.delete('toplevel') asPath += `?${params}` router.push(asPath, undefined, { shallow: false }) } diff --git a/src/search/components/hooks/useAISearchLocalStorageCache.ts b/src/search/components/hooks/useAISearchLocalStorageCache.ts index b7ae9dfd4c14..5e611ab35563 100644 --- a/src/search/components/hooks/useAISearchLocalStorageCache.ts +++ b/src/search/components/hooks/useAISearchLocalStorageCache.ts @@ -37,25 +37,31 @@ export function useAISearchLocalStorageCache( const getItem = useCallback( (query: string, version: string, language: string): T | null => { const key = generateCacheKey(query, version, language) - const itemStr = localStorage.getItem(key) - if (!itemStr) return null - - let cachedItem: CachedItem try { - cachedItem = JSON.parse(itemStr) - } catch (e) { - console.error('Failed to parse cached item from localStorage', e) - localStorage.removeItem(key) - return null - } + const itemStr = localStorage.getItem(key) + if (!itemStr) return null - const now = Date.now() - const expirationTime = cachedItem.timestamp + expirationDays * 24 * 60 * 60 * 1000 - if (now < expirationTime) { - return cachedItem.data - } else { - localStorage.removeItem(key) - updateCacheIndex((index) => index.filter((entry) => entry.key !== key)) + let cachedItem: CachedItem + try { + cachedItem = JSON.parse(itemStr) + } catch (e) { + console.error('Failed to parse cached item from localStorage', e) + localStorage.removeItem(key) + return null + } + + const now = Date.now() + const expirationTime = cachedItem.timestamp + expirationDays * 24 * 60 * 60 * 1000 + if (now < expirationTime) { + return cachedItem.data + } else { + localStorage.removeItem(key) + updateCacheIndex((index) => index.filter((entry) => entry.key !== key)) + return null + } + } catch (e) { + // Storage can be blocked by browser settings, so treat failures as a cache miss. + console.warn('Failed to read AI search cache from localStorage', e) return null } }, @@ -68,33 +74,38 @@ export function useAISearchLocalStorageCache( const now = Date.now() const cachedItem: CachedItem = { data, timestamp: now } - localStorage.setItem(key, JSON.stringify(cachedItem)) - - const indexStr = localStorage.getItem(cacheIndexKey) - let index: CacheIndexEntry[] = [] - if (indexStr) { - try { - index = JSON.parse(indexStr) - } catch (e) { - console.error('Failed to parse cache index from localStorage', e) + try { + localStorage.setItem(key, JSON.stringify(cachedItem)) + + const indexStr = localStorage.getItem(cacheIndexKey) + let index: CacheIndexEntry[] = [] + if (indexStr) { + try { + index = JSON.parse(indexStr) + } catch (e) { + console.error('Failed to parse cache index from localStorage', e) + } } - } - - index = index.filter((entry) => entry.key !== key) - index.push({ key, timestamp: now }) - // Keep the newest entries when the cache exceeds maxEntries. - if (index.length > maxEntries) { - index.sort((a, b) => a.timestamp - b.timestamp) - const excess = index.length - maxEntries - const entriesToRemove = index.slice(0, excess) - for (const entry of entriesToRemove) { - localStorage.removeItem(entry.key) + index = index.filter((entry) => entry.key !== key) + index.push({ key, timestamp: now }) + + // Keep the newest entries when the cache exceeds maxEntries. + if (index.length > maxEntries) { + index.sort((a, b) => a.timestamp - b.timestamp) + const excess = index.length - maxEntries + const entriesToRemove = index.slice(0, excess) + for (const entry of entriesToRemove) { + localStorage.removeItem(entry.key) + } + index = index.slice(excess) } - index = index.slice(excess) - } - localStorage.setItem(cacheIndexKey, JSON.stringify(index)) + localStorage.setItem(cacheIndexKey, JSON.stringify(index)) + } catch (e) { + // Caching is optional, so quota or blocked storage errors must not break the answer UI. + console.warn('Failed to write AI search cache to localStorage', e) + } }, [cacheKeyPrefix, maxEntries], ) diff --git a/src/search/components/input/AskAIResults.tsx b/src/search/components/input/AskAIResults.tsx index 3e8d1a9f2d63..0c37d2e46c75 100644 --- a/src/search/components/input/AskAIResults.tsx +++ b/src/search/components/input/AskAIResults.tsx @@ -164,6 +164,8 @@ export function AskAIResults({ let messageBuffer = '' let sourcesBuffer: AIReference[] = [] let conversationIdBuffer = '' + // Errored or dropped streams leave a truncated answer that must not be cached as complete. + let streamSucceeded = false try { const response = await executeAISearch(version, query, debug) @@ -329,13 +331,14 @@ export function AskAIResults({ console.warn('Failed to parse tail JSON:', leftover, err) } } + streamSucceeded = true } catch (error: unknown) { if (!isCancelled) { console.error('Failed to fetch search results:', error) setAISearchError() } } finally { - if (!isCancelled && messageBuffer) { + if (!isCancelled && streamSucceeded && messageBuffer) { setItem( query, { diff --git a/src/search/components/input/SearchDialog.tsx b/src/search/components/input/SearchDialog.tsx index 48c8d6027036..650259713645 100644 --- a/src/search/components/input/SearchDialog.tsx +++ b/src/search/components/input/SearchDialog.tsx @@ -74,6 +74,10 @@ export function SearchDialog({ return } const handleKeyDown = (event: KeyboardEvent) => { + // An Escape that cancels an input method composition must not close the dialog. + if (event.isComposing || event.keyCode === 229) { + return + } // The input closes on Escape and prevents default, so this catches Escape elsewhere. if (event.key === 'Escape' && !event.defaultPrevented) { onClose() diff --git a/src/search/components/input/SearchOverlay.tsx b/src/search/components/input/SearchOverlay.tsx index f220de91bc63..50ee754d8f02 100644 --- a/src/search/components/input/SearchOverlay.tsx +++ b/src/search/components/input/SearchOverlay.tsx @@ -389,6 +389,10 @@ export function SearchOverlay({ } const handleKeyDown = (event: React.KeyboardEvent) => { + // Leave composition keys to the IME. Safari flags the confirming Enter only with keyCode 229. + if (event.nativeEvent.isComposing || event.keyCode === 229) { + return + } if (event.key === 'ArrowDown') { event.preventDefault() stepActiveOption(1) diff --git a/src/search/lib/ai-search-proxy.ts b/src/search/lib/ai-search-proxy.ts index 8b6d4761b850..ba684fe0b2ef 100644 --- a/src/search/lib/ai-search-proxy.ts +++ b/src/search/lib/ai-search-proxy.ts @@ -172,7 +172,8 @@ export const aiSearchProxy = async (req: ExtendedRequest, res: Response) => { if (!res.headersSent) { res.status(500).json({ errors: [{ message: 'Internal server error' }] }) } else { - const errorMessage = `${JSON.stringify({ errors: [{ message: 'Internal server error' }] })}\n` + // The leading newline ends any partial upstream record so the client can parse this one. + const errorMessage = `\n${JSON.stringify({ errors: [{ message: 'Internal server error' }] })}\n` res.write(errorMessage) res.end() } diff --git a/src/search/lib/get-elasticsearch-results/general-search.ts b/src/search/lib/get-elasticsearch-results/general-search.ts index 6fb21b2ba1d8..aefd3be6dea1 100644 --- a/src/search/lib/get-elasticsearch-results/general-search.ts +++ b/src/search/lib/get-elasticsearch-results/general-search.ts @@ -409,7 +409,7 @@ function getHits( result.score = hit._score ?? 0.0 result.popularity = source.popularity ?? 0.0 if (isDevMode) { - result.es_url = `http://localhost:9200/${indexName}/_doc/${hit._id}` + result.es_url = `http://localhost:9200/${indexName}/_doc/${encodeURIComponent(hit._id!)}` } } for (const field of include) { diff --git a/src/search/lib/routes/combined-search-route.ts b/src/search/lib/routes/combined-search-route.ts index b36f56930e47..6c94f57abc1f 100644 --- a/src/search/lib/routes/combined-search-route.ts +++ b/src/search/lib/routes/combined-search-route.ts @@ -20,6 +20,8 @@ export async function combinedSearchRoute(req: Request, res: Response) { } = getSearchFromRequestParams(req, 'aiSearchAutocomplete', { // force lets empty autocomplete queries return the top AI suggestions. query: typeof req.query.query !== 'string' ? '' : req.query.query, + // AI autocomplete indexes exist only in English; general search below uses the requested language. + language: 'en', }) const { diff --git a/src/search/scripts/analyze-text.ts b/src/search/scripts/analyze-text.ts index bf501b0b00f9..b02667d1d8e8 100755 --- a/src/search/scripts/analyze-text.ts +++ b/src/search/scripts/analyze-text.ts @@ -9,6 +9,7 @@ import chalk from 'chalk' import dotenv from 'dotenv' import { languageKeys } from '@/languages/lib/languages-server' +import { getElasticSearchIndex } from '@/search/lib/elasticsearch-indexes' import { allVersions } from '@/versions/lib/all-versions' import type { estypes } from '@elastic/elasticsearch' @@ -110,10 +111,9 @@ async function main(opts: Options, textArgs: string[]): Promise { } const { indexPrefix } = opts - const prefix = indexPrefix ? `${indexPrefix}_` : '' for (const languageKey of languagesToAnalyze) { - const indexName = `${prefix}github-docs-${versionKey}-${languageKey}` + const indexName = getIndexName(versionKey, languageKey, indexPrefix) console.log(chalk.yellow(`Analyzing in ${chalk.bold(indexName)}`)) await analyzeVersion(client, texts, indexName) } @@ -138,6 +138,11 @@ export function getLanguagesToAnalyze({ return ['en'] } +// Matches the names the general search indexer creates. +export function getIndexName(version: string, language: string, indexPrefix = ''): string { + return getElasticSearchIndex('generalSearch', version, language, indexPrefix).indexName +} + function safeUrlDisplay(url: string): string { const parsed = new URL(url) if (parsed.password) { diff --git a/src/search/scripts/index/lib/index-general-search.ts b/src/search/scripts/index/lib/index-general-search.ts index 3b57cdf27c70..87407d2b36e5 100644 --- a/src/search/scripts/index/lib/index-general-search.ts +++ b/src/search/scripts/index/lib/index-general-search.ts @@ -162,6 +162,7 @@ async function indexVersion( retries: opts.retries, sleepTime: opts.sleepTime * 1000, verbose: opts.verbose, + idField: 'url', }) await updateAlias(client, indexName, indexAlias, opts) diff --git a/src/search/scripts/index/utils/indexing-elasticsearch-utils.ts b/src/search/scripts/index/utils/indexing-elasticsearch-utils.ts index 4e793f803f29..c7c34ecf43d6 100644 --- a/src/search/scripts/index/utils/indexing-elasticsearch-utils.ts +++ b/src/search/scripts/index/utils/indexing-elasticsearch-utils.ts @@ -17,6 +17,8 @@ type Options = { retries?: number sleepTime?: number verbose?: boolean + // Record field to use as the document _id, so a retried bulk overwrites documents instead of duplicating them. + idField?: string } export async function createIndex( @@ -54,7 +56,10 @@ export async function populateIndex( () => client.helpers.bulk({ datasource: records, - onDocument: () => ({ index: { _index: indexAlias } }), + onDocument: (doc) => { + const id = options.idField ? doc[options.idField] : undefined + return { index: { _index: indexAlias, ...(typeof id === 'string' ? { _id: id } : {}) } } + }, // Keep bulk requests under 4 MB, because larger requests can return 429 from Elasticsearch. flushBytes: 4 * 1024 * 1024, concurrency: 2, diff --git a/src/search/tests/ai-search-local-storage-cache.ts b/src/search/tests/ai-search-local-storage-cache.ts new file mode 100644 index 000000000000..32027388d001 --- /dev/null +++ b/src/search/tests/ai-search-local-storage-cache.ts @@ -0,0 +1,106 @@ +import { createElement } from 'react' +import { renderToString } from 'react-dom/server' +import { afterEach, beforeEach, describe, expect, test, vi } from 'vitest' + +import { useAISearchLocalStorageCache } from '@/search/components/hooks/useAISearchLocalStorageCache' + +type Cache = ReturnType> + +function getCache(): Cache { + let cache: Cache | undefined + function Capture() { + cache = useAISearchLocalStorageCache('test-cache', 2, 7) + return null + } + renderToString(createElement(Capture)) + if (!cache) throw new Error('hook did not render') + return cache +} + +function domError(name: string): Error { + const error = new Error(`${name} from fake storage`) + error.name = name + return error +} + +function mapStorage() { + const map = new Map() + return { + getItem: vi.fn((key: string) => map.get(key) ?? null), + setItem: vi.fn((key: string, value: string) => { + map.set(key, value) + }), + removeItem: vi.fn((key: string) => { + map.delete(key) + }), + } +} + +describe('useAISearchLocalStorageCache', () => { + const originalDescriptor = Object.getOwnPropertyDescriptor(globalThis, 'localStorage') + + beforeEach(() => { + vi.spyOn(console, 'warn').mockImplementation(() => {}) + vi.spyOn(console, 'error').mockImplementation(() => {}) + }) + + afterEach(() => { + vi.restoreAllMocks() + if (originalDescriptor) { + Object.defineProperty(globalThis, 'localStorage', originalDescriptor) + } else { + delete (globalThis as { localStorage?: unknown }).localStorage + } + }) + + function installStorage(storage: unknown) { + Object.defineProperty(globalThis, 'localStorage', { + configurable: true, + value: storage, + }) + } + + test('stores and returns a cached value', () => { + installStorage(mapStorage()) + const { getItem, setItem } = getCache() + expect(getItem('How do I fork?', 'free-pro-team@latest', 'en')).toBeNull() + setItem('How do I fork?', 'answer', 'free-pro-team@latest', 'en') + expect(getItem(' how do i fork? ', 'free-pro-team@latest', 'en')).toBe('answer') + expect(getItem('How do I fork?', 'enterprise-cloud@latest', 'en')).toBeNull() + }) + + test('treats blocked storage as a cache miss and skips caching', () => { + Object.defineProperty(globalThis, 'localStorage', { + configurable: true, + get() { + throw domError('SecurityError') + }, + }) + const { getItem, setItem } = getCache() + expect(getItem('query', 'free-pro-team@latest', 'en')).toBeNull() + expect(() => setItem('query', 'answer', 'free-pro-team@latest', 'en')).not.toThrow() + }) + + test('does not throw when writing exceeds the storage quota', () => { + const storage = mapStorage() + storage.setItem.mockImplementation(() => { + throw domError('QuotaExceededError') + }) + installStorage(storage) + const { getItem, setItem } = getCache() + expect(() => setItem('query', 'answer', 'free-pro-team@latest', 'en')).not.toThrow() + expect(getItem('query', 'free-pro-team@latest', 'en')).toBeNull() + }) + + test('returns null when removing an expired entry fails', () => { + const storage = mapStorage() + installStorage(storage) + const { getItem, setItem } = getCache() + setItem('query', 'answer', 'free-pro-team@latest', 'en') + vi.spyOn(Date, 'now').mockReturnValue(Date.now() + 8 * 24 * 60 * 60 * 1000) + storage.removeItem.mockImplementation(() => { + throw domError('SecurityError') + }) + expect(getItem('query', 'free-pro-team@latest', 'en')).toBeNull() + }) +}) diff --git a/src/search/tests/ai-search-proxy.ts b/src/search/tests/ai-search-proxy.ts index 18342c17f5c6..0f1994c58ec2 100644 --- a/src/search/tests/ai-search-proxy.ts +++ b/src/search/tests/ai-search-proxy.ts @@ -146,3 +146,44 @@ describe('aiSearchProxy upstream 400 handling', () => { expect(res.statusCode).toBe(400) }) }) + +describe('aiSearchProxy mid-stream errors', () => { + beforeEach(() => { + vi.clearAllMocks() + }) + + test('error record starts on its own line after a partial upstream record', async () => { + const encoder = new TextEncoder() + let pulls = 0 + const body = new ReadableStream({ + pull(controller) { + pulls += 1 + if (pulls === 1) { + controller.enqueue(encoder.encode('{"chunkType":"MESSAGE_CHUNK","text":"To clo')) + } else { + throw new TypeError('terminated') + } + }, + }) + ;(fetchStream as ReturnType).mockResolvedValue({ + ok: true, + status: 200, + headers: { get: () => 'application/x-ndjson' }, + body, + }) + const res = buildResponse() + res.flushHeaders.mockImplementation(() => { + res.headersSent = true + }) + + await aiSearchProxy(buildRequest(), res as never) + + const written = res.write.mock.calls.map((call) => call[0] as string).join('') + const lines = written.split('\n').filter((line) => line.trim()) + expect(JSON.parse(lines[lines.length - 1])).toEqual({ + errors: [{ message: 'Internal server error' }], + }) + expect(incrementedMetrics()).toContain('ai-search.stream_error') + expect(res.end).toHaveBeenCalled() + }) +}) diff --git a/src/search/tests/analyze-text.ts b/src/search/tests/analyze-text.ts index b187ad4e2434..5d49f884dbec 100644 --- a/src/search/tests/analyze-text.ts +++ b/src/search/tests/analyze-text.ts @@ -1,7 +1,8 @@ -import { describe, expect, test } from 'vitest' +import { afterEach, describe, expect, test, vi } from 'vitest' import { languageKeys } from '@/languages/lib/languages-server' -import { getLanguagesToAnalyze } from '@/search/scripts/analyze-text' +import { getIndexName, getLanguagesToAnalyze } from '@/search/scripts/analyze-text' +import { allVersions } from '@/versions/lib/all-versions' describe('getLanguagesToAnalyze', () => { test('defaults to English', () => { @@ -24,3 +25,29 @@ describe('getLanguagesToAnalyze', () => { ) }) }) + +describe('getIndexName', () => { + afterEach(() => { + vi.unstubAllEnvs() + }) + + test('maps dotcom to the fpt general search index', () => { + expect(getIndexName('dotcom', 'en')).toBe('tests_github-docs_general-search_fpt_en') + }) + + test('uses the ghec general search index', () => { + expect(getIndexName('ghec', 'en')).toBe('tests_github-docs_general-search_ghec_en') + }) + + test('uses the release in GHES general search index names', () => { + const ghes = Object.values(allVersions).find((info) => info.hasNumberedReleases)! + const versionKey = `${ghes.miscBaseName}${ghes.currentRelease}` + expect(getIndexName(versionKey, 'en')).toBe(`tests_github-docs_general-search_${versionKey}_en`) + }) + + test('adds one underscore after the index prefix', () => { + vi.stubEnv('NODE_ENV', 'development') + expect(getIndexName('dotcom', 'en', 'tests')).toBe('tests_github-docs_general-search_fpt_en') + expect(getIndexName('dotcom', 'en', 'tests_')).toBe('tests_github-docs_general-search_fpt_en') + }) +}) diff --git a/src/search/tests/api-combined-search.ts b/src/search/tests/api-combined-search.ts index 0a8f893e718d..86350f70c660 100644 --- a/src/search/tests/api-combined-search.ts +++ b/src/search/tests/api-combined-search.ts @@ -82,15 +82,6 @@ describeIfElasticsearchURL('search/combined-autocomplete v1 middleware', () => { expect(JSON.parse(res.body).error).toBeTruthy() }) - test('only english supported', async () => { - const sp = new URLSearchParams() - sp.set('query', 'rest') - sp.set('language', 'ja') - const res = await get(getSearchEndpointWithParams(sp)) - expect(res.statusCode).toBe(400) - expect(JSON.parse(res.body).error).toBeTruthy() - }) - test('autocomplete term search', async () => { const sp = new URLSearchParams() sp.set('query', 'rest') diff --git a/src/search/tests/execute-search-actions.ts b/src/search/tests/execute-search-actions.ts new file mode 100644 index 000000000000..a5491ba36def --- /dev/null +++ b/src/search/tests/execute-search-actions.ts @@ -0,0 +1,48 @@ +import { afterEach, describe, expect, test, vi } from 'vitest' +import type { NextRouter } from 'next/router' + +import { executeGeneralSearch } from '@/search/components/helpers/execute-search-actions' +import { DEFAULT_VERSION } from '@/versions/components/useVersion' + +vi.mock('@/events/components/events', () => ({ sendEvent: vi.fn() })) + +function setup(search: string) { + vi.stubGlobal('window', { location: { search } }) + const push = vi.fn() + const router = { locale: 'en', push } as unknown as NextRouter + return { router, push } +} + +function pushedUrl(push: ReturnType) { + expect(push).toHaveBeenCalledTimes(1) + return new URL(push.mock.calls[0][0] as string, 'http://localhost') +} + +afterEach(() => { + vi.unstubAllGlobals() +}) + +describe('executeGeneralSearch', () => { + test('starts a new query on page 1 of all results', () => { + const { router, push } = setup( + '?query=git&toplevel=Get+started&page=4&tool=cli&search-overlay-open=true', + ) + executeGeneralSearch(router, DEFAULT_VERSION, 'codespaces') + const url = pushedUrl(push) + expect(url.pathname).toBe('/en/search') + expect(url.searchParams.get('query')).toBe('codespaces') + expect(url.searchParams.get('tool')).toBe('cli') + expect(url.searchParams.has('page')).toBe(false) + expect(url.searchParams.has('toplevel')).toBe(false) + expect(url.searchParams.has('search-overlay-open')).toBe(false) + }) + + test('includes a non-default version in the path', () => { + const { router, push } = setup('?page=2') + executeGeneralSearch(router, 'enterprise-cloud@latest', 'actions') + const url = pushedUrl(push) + expect(url.pathname).toBe('/en/enterprise-cloud@latest/search') + expect(url.searchParams.get('query')).toBe('actions') + expect(url.searchParams.has('page')).toBe(false) + }) +}) diff --git a/src/search/tests/fixtures/search-indexes/tests_github-docs_general-search_fpt_en-records.json b/src/search/tests/fixtures/search-indexes/tests_github-docs_general-search_fpt_en-records.json index dd778f43ccbe..927a24bc9617 100644 --- a/src/search/tests/fixtures/search-indexes/tests_github-docs_general-search_fpt_en-records.json +++ b/src/search/tests/fixtures/search-indexes/tests_github-docs_general-search_fpt_en-records.json @@ -29,7 +29,7 @@ "toplevel": "Baring" }, "/en/breadcrumbless": { - "objectID": "/en/bar", + "objectID": "/en/breadcrumbless", "breadcrumbs": "", "title": "Breadcrumbless", "headings": "Introduction", diff --git a/src/search/tests/fixtures/search-indexes/tests_github-docs_general-search_fpt_ja-records.json b/src/search/tests/fixtures/search-indexes/tests_github-docs_general-search_fpt_ja-records.json index 3e1f13fc12ce..04c2684ed52e 100644 --- a/src/search/tests/fixtures/search-indexes/tests_github-docs_general-search_fpt_ja-records.json +++ b/src/search/tests/fixtures/search-indexes/tests_github-docs_general-search_fpt_ja-records.json @@ -29,7 +29,7 @@ "toplevel": "Baring" }, "/ja/breadcrumbless": { - "objectID": "/ja/bar", + "objectID": "/ja/breadcrumbless", "breadcrumbs": "", "title": "パンくずなし", "headings": "", diff --git a/src/search/tests/indexing-elasticsearch-utils.ts b/src/search/tests/indexing-elasticsearch-utils.ts new file mode 100644 index 000000000000..35dbe83457cd --- /dev/null +++ b/src/search/tests/indexing-elasticsearch-utils.ts @@ -0,0 +1,83 @@ +import fs from 'fs' +import path from 'path' + +import { describe, expect, test, vi } from 'vitest' +import { errors } from '@elastic/elasticsearch' +import type { Client } from '@elastic/elasticsearch' + +import { populateIndex } from '@/search/scripts/index/utils/indexing-elasticsearch-utils' + +type Doc = Record +type BulkOptions = { + datasource: Doc[] + onDocument: (doc: Doc) => { index: { _index: string; _id?: string } } +} + +// Mimics an Elasticsearch bulk helper that stores some documents and then +// gets a request-level 429, which makes populateIndex retry the whole bulk. +function createFakeClient() { + const stored = new Map() + let autoId = 0 + let calls = 0 + const bulk = vi.fn(async ({ datasource, onDocument }: BulkOptions) => { + calls++ + const docs = calls === 1 ? datasource.slice(0, Math.ceil(datasource.length / 2)) : datasource + for (const doc of docs) { + const action = onDocument(doc) + stored.set(action.index._id ?? `auto-${autoId++}`, doc) + } + if (calls === 1) { + throw new errors.ResponseError({ + body: { error: { type: 'es_rejected_execution_exception' } }, + statusCode: 429, + headers: {}, + warnings: null, + meta: {} as never, + }) + } + return { total: docs.length, failed: 0, successful: docs.length } + }) + const client = { + helpers: { bulk }, + count: async () => ({ count: stored.size }), + } as unknown as Client + return { client, stored, bulk } +} + +const records = [{ url: '/en/a' }, { url: '/en/b' }, { url: '/en/c' }, { url: '/en/d' }] + +describe('populateIndex', () => { + test('uses idField as the document _id so a retried bulk does not duplicate documents', async () => { + const { client, stored, bulk } = createFakeClient() + await populateIndex(client, 'alias', 'name', records, { + retries: 1, + sleepTime: 1, + idField: 'url', + }) + expect(bulk).toHaveBeenCalledTimes(2) + expect(stored.size).toBe(records.length) + expect([...stored.keys()].sort()).toEqual(['/en/a', '/en/b', '/en/c', '/en/d']) + }) + + test('sends no _id when idField is not set', async () => { + const { client, bulk } = createFakeClient() + await populateIndex(client, 'alias', 'name', records, { retries: 1, sleepTime: 1 }) + const { onDocument } = bulk.mock.calls[0][0] + expect(onDocument({ url: '/en/a' })).toEqual({ index: { _index: 'alias' } }) + }) +}) + +describe('general search fixture records', () => { + const dir = 'src/search/tests/fixtures/search-indexes' + const files = fs.readdirSync(dir).filter((file) => file.endsWith('-records.json')) + + // The indexer uses objectID as the document _id, so a repeated objectID would overwrite another record. + test.each(files)('%s has unique objectIDs', (file) => { + const fixture = JSON.parse(fs.readFileSync(path.join(dir, file), 'utf8')) as Record< + string, + { objectID: string } + > + const ids = Object.values(fixture).map((record) => record.objectID) + expect(new Set(ids).size).toBe(ids.length) + }) +}) diff --git a/src/versions/components/VersionPicker.tsx b/src/versions/components/VersionPicker.tsx index 3d5dc7e98d6d..97432739949f 100644 --- a/src/versions/components/VersionPicker.tsx +++ b/src/versions/components/VersionPicker.tsx @@ -168,7 +168,7 @@ export const VersionPicker = ({ variant = 'default', onNavigate }: Props) => { allLinks.push({ text: t('about_versions'), selected: false, - href: `/${router.locale}${currentVersionPathSegment}/get-started/learning-about-github/about-versions-of-github-docs`, + href: `/${router.locale}${currentVersionPathSegment}/get-started/using-github-docs/about-versions-of-github-docs`, value: ABOUT_VERSIONS_VALUE, extra: { arrow: false, diff --git a/src/webhooks/scripts/sync.ts b/src/webhooks/scripts/sync.ts index 1ff0b958accc..4b7c099b2922 100644 --- a/src/webhooks/scripts/sync.ts +++ b/src/webhooks/scripts/sync.ts @@ -1,4 +1,4 @@ -import { mkdir, readFile, writeFile, unlink } from 'fs/promises' +import { mkdir, readdir, readFile, writeFile, unlink } from 'fs/promises' import { existsSync } from 'fs' import path from 'path' @@ -48,6 +48,8 @@ export async function syncWebhookData( await mkdir(targetDirectory, { recursive: true }) } + const writtenFiles = new Set() + // Split categories, such as check_run.json, from childParamsGroups sidecars. await Promise.all( Object.entries(data).map(async ([category, categoryData]) => { @@ -80,29 +82,43 @@ export async function syncWebhookData( } } + writtenFiles.add(`${category}.json`) const targetPath = path.join(targetDirectory, `${category}.json`) await writeFile(targetPath, JSON.stringify(slimCategoryData, null, 2)) console.log(`✅ Wrote ${targetPath}`) const childParamsPath = path.join(targetDirectory, `${category}.child-params.json`) if (Object.keys(childParams).length > 0) { + writtenFiles.add(`${category}.child-params.json`) await writeFile(childParamsPath, JSON.stringify(childParams, null, 2)) console.log(`✅ Wrote ${childParamsPath}`) - } else { - // Remove stale child-param sidecars so drill-down pages do not show removed nested params. - for (const stalePath of [childParamsPath, `${childParamsPath}.br`]) { - if (existsSync(stalePath)) { - await unlink(stalePath) - console.log(`🗑️ Removed stale ${stalePath}`) - } - } } }), ) + + await removeStaleWebhookDataFiles(targetDirectory, writtenFiles) }), ) } +// The site lists webhook events from the files on disk, so any category or +// child-params file this run did not write would keep a removed event on the docs. +// Brotli copies are removed too because the loader prefers them over plain JSON. +export async function removeStaleWebhookDataFiles( + targetDirectory: string, + writtenFiles: Set, +): Promise { + if (!existsSync(targetDirectory)) return + + for (const file of await readdir(targetDirectory)) { + if (!(file.endsWith('.json') || file.endsWith('.json.br'))) continue + if (writtenFiles.has(file)) continue + const filePath = path.join(targetDirectory, file) + await unlink(filePath) + console.log(`🗑️ Removed stale data file ${filePath}`) + } +} + async function processWebhookSchema(webhooks: Webhook[]): Promise { try { if (webhooks.length) { diff --git a/src/webhooks/tests/remove-stale-data-files.ts b/src/webhooks/tests/remove-stale-data-files.ts new file mode 100644 index 000000000000..09749919b846 --- /dev/null +++ b/src/webhooks/tests/remove-stale-data-files.ts @@ -0,0 +1,52 @@ +import { describe, expect, test, beforeEach, afterEach } from 'vitest' +import { mkdtemp, writeFile, readdir, rm } from 'fs/promises' +import path from 'path' +import os from 'os' + +import { removeStaleWebhookDataFiles } from '@/webhooks/scripts/sync' + +describe('removeStaleWebhookDataFiles', () => { + let versionDir: string + + beforeEach(async () => { + versionDir = await mkdtemp(path.join(os.tmpdir(), 'webhook-sync-test-')) + }) + + afterEach(async () => { + await rm(versionDir, { recursive: true, force: true }) + }) + + test('removes category and child-params files not written during sync', async () => { + await writeFile(path.join(versionDir, 'foo_bar.json'), '{}') + await writeFile(path.join(versionDir, 'foo_bar.child-params.json'), '{}') + await writeFile(path.join(versionDir, 'foo_bar.json.br'), '') + await writeFile(path.join(versionDir, 'foo_baz.json'), '{}') + await writeFile(path.join(versionDir, 'ping.json'), '{}') + await writeFile(path.join(versionDir, 'README.md'), '# readme') + + await removeStaleWebhookDataFiles(versionDir, new Set(['foo_baz.json', 'ping.json'])) + + const remaining = (await readdir(versionDir)).sort() + expect(remaining).toEqual(['README.md', 'foo_baz.json', 'ping.json']) + }) + + test('keeps child-params files that were written during sync', async () => { + await writeFile(path.join(versionDir, 'foo_baz.json'), '{}') + await writeFile(path.join(versionDir, 'foo_baz.child-params.json'), '{}') + + await removeStaleWebhookDataFiles( + versionDir, + new Set(['foo_baz.json', 'foo_baz.child-params.json']), + ) + + const remaining = (await readdir(versionDir)).sort() + expect(remaining).toEqual(['foo_baz.child-params.json', 'foo_baz.json']) + }) + + test('skips a directory that does not exist', async () => { + const nonexistent = path.join(versionDir, 'does-not-exist') + await expect( + removeStaleWebhookDataFiles(nonexistent, new Set(['ping.json'])), + ).resolves.toBeUndefined() + }) +}) diff --git a/src/workflows/sync-sdk-docs/convert-mermaid.ts b/src/workflows/sync-sdk-docs/convert-mermaid.ts index 06a202a19874..889c6bf24d23 100644 --- a/src/workflows/sync-sdk-docs/convert-mermaid.ts +++ b/src/workflows/sync-sdk-docs/convert-mermaid.ts @@ -16,6 +16,8 @@ import path from 'node:path' import { parseArgs } from 'node:util' import { execFileSync, execSync } from 'node:child_process' +import { MERMAID_BLOCK } from '@/workflows/sync-sdk-docs/mermaid-blocks' + const { values: args } = parseArgs({ options: { 'sdk-docs-dir': { type: 'string' }, @@ -105,8 +107,7 @@ function generateAltText(mermaidSource: string): string { function processFile(filePath: string, assetsUrlPath: string): number { const raw = fs.readFileSync(filePath, 'utf8') - const mermaidRegex = /```mermaid\n([\s\S]*?)```/g - const matches = [...raw.matchAll(mermaidRegex)] + const matches = [...raw.matchAll(MERMAID_BLOCK)] if (matches.length === 0) return 0 diff --git a/src/workflows/sync-sdk-docs/escape-liquid.ts b/src/workflows/sync-sdk-docs/escape-liquid.ts new file mode 100644 index 000000000000..8c2efbb5519e --- /dev/null +++ b/src/workflows/sync-sdk-docs/escape-liquid.ts @@ -0,0 +1,41 @@ +// Upstream SDK docs are plain GitHub Markdown, but docs.github.com renders every +// page through Liquid. Unescaped GitHub Actions expressions and template +// placeholders render as empty strings, and a stray tag breaks the page. + +import { MERMAID_BLOCK } from '@/workflows/sync-sdk-docs/mermaid-blocks' + +// Wrap whole single-line spans to match hand-authored content, which keeps the +// leading $ of an Actions expression inside the raw block. Lone opening +// delimiters get wrapped too, so an unclosed one cannot cause a parse error. +const LIQUID_SYNTAX = /\$?\{\{[^\n]*?\}\}|\{%[^\n]*?%\}|\{\{|\{%/g + +function wrapInRaw(match: string): string { + // A raw block ends at the first endraw tag, so wrap only the opening delimiter. + if (match.includes('endraw')) { + const delimiterEnd = match.indexOf('{') + 2 + const rest = escapeSpans(match.slice(delimiterEnd)) + return `{% raw %}${match.slice(0, delimiterEnd)}{% endraw %}${rest}` + } + return `{% raw %}${match}{% endraw %}` +} + +function escapeSpans(text: string): string { + return text.replace(LIQUID_SYNTAX, (match: string, offset: number) => { + // A preceding { would join the raw tag's brace into an output delimiter. + if (match.startsWith('$') && text[offset - 1] === '{') return `$${wrapInRaw(match.slice(1))}` + return wrapInRaw(match) + }) +} + +// convert-mermaid.ts renders these blocks later, and raw tags would corrupt the diagrams. +export function escapeLiquid(content: string): string { + let result = '' + let last = 0 + + for (const match of content.matchAll(MERMAID_BLOCK)) { + result += escapeSpans(content.slice(last, match.index)) + match[0] + last = match.index + match[0].length + } + + return result + escapeSpans(content.slice(last)) +} diff --git a/src/workflows/sync-sdk-docs/mermaid-blocks.ts b/src/workflows/sync-sdk-docs/mermaid-blocks.ts new file mode 100644 index 000000000000..c5d45a00b1ec --- /dev/null +++ b/src/workflows/sync-sdk-docs/mermaid-blocks.ts @@ -0,0 +1,3 @@ +// convert-mermaid.ts replaces these blocks with images, and escape-liquid.ts +// skips the same blocks, so both steps must agree on what a mermaid block is. +export const MERMAID_BLOCK = /```mermaid\n([\s\S]*?)```/g diff --git a/src/workflows/sync-sdk-docs/normalize-sdk-docs.ts b/src/workflows/sync-sdk-docs/normalize-sdk-docs.ts index 0d2470c30a68..a223ca89ccaa 100644 --- a/src/workflows/sync-sdk-docs/normalize-sdk-docs.ts +++ b/src/workflows/sync-sdk-docs/normalize-sdk-docs.ts @@ -1,8 +1,8 @@ #!/usr/bin/env node // Normalizes Copilot SDK docs for docs.github.com, including README landing -// pages, frontmatter, links, code fences, ordered lists, hidden validation -// samples, codetabs, and SDK-specific markdownlint suppressions. +// pages, Liquid escaping, frontmatter, links, code fences, ordered lists, +// hidden validation samples, codetabs, and SDK-specific markdownlint suppressions. // // Usage: // npx tsx src/workflows/sync-sdk-docs/normalize-sdk-docs.ts --content-dir \ @@ -13,6 +13,7 @@ import path from 'node:path' import { parseArgs } from 'node:util' import matter from '@gr2m/gray-matter' +import { escapeLiquid } from '@/workflows/sync-sdk-docs/escape-liquid' import { stripHiddenBlocks, nextFenceState, type OpenFence } from './strip-hidden-blocks' const { values: args } = parseArgs({ @@ -609,6 +610,16 @@ function fixBlanksAroundFences(filePath: string): void { } } +function escapeLiquidSyntax(filePath: string): void { + const raw = fs.readFileSync(filePath, 'utf8') + const updated = escapeLiquid(raw) + + if (updated !== raw) { + fs.writeFileSync(filePath, updated, 'utf8') + console.log(` LIQUID: ${path.relative(SDK_DOCS_DIR, filePath)}`) + } +} + // docs-validate: hidden ranges wrap validation-only samples that compile in // place of the reader-facing fragment. Remove them before codetabs conversion, // so validation samples do not publish beside the real examples. @@ -887,8 +898,15 @@ reportMissingRelocatedSources() console.log('\n--- Renaming README.md files to index.md ---\n') convertReadmesToIndex() -console.log('\n--- Adding frontmatter ---\n') const files = getAllMarkdownFiles(SDK_DOCS_DIR) + +// Escape upstream text before this script adds its own Liquid, such as codetabs tags. +console.log('\n--- Escaping Liquid syntax ---\n') +for (const file of files) { + escapeLiquidSyntax(file) +} + +console.log('\n--- Adding frontmatter ---\n') console.log(`Found ${files.length} markdown files.\n`) for (const file of files) { addFrontmatter(file) diff --git a/src/workflows/tests/escape-liquid.ts b/src/workflows/tests/escape-liquid.ts new file mode 100644 index 000000000000..e02b96512df9 --- /dev/null +++ b/src/workflows/tests/escape-liquid.ts @@ -0,0 +1,147 @@ +import { describe, expect, test } from 'vitest' +import { Liquid } from 'liquidjs' + +import { escapeLiquid } from '@/workflows/sync-sdk-docs/escape-liquid' + +const FENCE = '```' + +// Match the docs.github.com Liquid engine options. +const engine = new Liquid({ extname: '.html', dynamicPartials: false }) + +async function expectRendersUnchanged(input: string): Promise { + expect(await engine.parseAndRender(escapeLiquid(input), {})).toBe(input) +} + +describe('escapeLiquid', () => { + test('wraps a GitHub Actions expression in a code sample', async () => { + const input = [ + `${FENCE}yaml`, + 'env:', + ' GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}', + FENCE, + ].join('\n') + + expect(escapeLiquid(input)).toBe( + [ + `${FENCE}yaml`, + 'env:', + ' GITHUB_TOKEN: {% raw %}${{ secrets.GITHUB_TOKEN }}{% endraw %}', + FENCE, + ].join('\n'), + ) + await expectRendersUnchanged(input) + }) + + test('wraps each placeholder on a line', async () => { + const input = 'Hello {{name}}, welcome to {{ place }}.' + + expect(escapeLiquid(input)).toBe( + 'Hello {% raw %}{{name}}{% endraw %}, welcome to {% raw %}{{ place }}{% endraw %}.', + ) + await expectRendersUnchanged(input) + }) + + test('wraps each tag of a multi-line template block', async () => { + const input = [ + `${FENCE}jinja`, + '{% for item in items %}', + '- {{ item }}', + '{% endfor %}', + FENCE, + ].join('\n') + + expect(escapeLiquid(input)).toBe( + [ + `${FENCE}jinja`, + '{% raw %}{% for item in items %}{% endraw %}', + '- {% raw %}{{ item }}{% endraw %}', + '{% raw %}{% endfor %}{% endraw %}', + FENCE, + ].join('\n'), + ) + await expectRendersUnchanged(input) + }) + + test('wraps lone opening delimiters so they cannot break parsing', async () => { + const input = ['const open = "{{"', 'const tag = "{%"', '{{', ' name', '}}'].join('\n') + + expect(escapeLiquid(input)).toBe( + [ + 'const open = "{% raw %}{{{% endraw %}"', + 'const tag = "{% raw %}{%{% endraw %}"', + '{% raw %}{{{% endraw %}', + ' name', + '}}', + ].join('\n'), + ) + await expectRendersUnchanged(input) + }) + + test('keeps an endraw tag in the source from closing the raw block early', async () => { + const input = 'Liquid: {% raw %}{{ x }}{% endraw %} and ${{ "{% endraw %}" }}' + + await expectRendersUnchanged(input) + }) + + test('keeps a brace before an Actions expression out of the raw tag', async () => { + const input = 'Use {${{ secrets.TOKEN }}} here' + + expect(escapeLiquid(input)).toBe('Use {${% raw %}{{ secrets.TOKEN }}{% endraw %}} here') + await expectRendersUnchanged(input) + }) + + test('leaves mermaid diagrams for the diagram renderer', () => { + const input = [`${FENCE}mermaid`, 'flowchart LR', ' A{{Hexagon}} --> B', FENCE].join('\n') + + expect(escapeLiquid(input)).toBe(input) + }) + + test('leaves indented mermaid diagrams in lists for the diagram renderer', () => { + const input = [ + '1. Step', + '', + ` ${FENCE}mermaid`, + ' A{{Hexagon}} --> B', + ` ${FENCE}`, + ].join('\n') + + expect(escapeLiquid(input)).toBe(input) + }) + + test('escapes mermaid fences that the diagram renderer does not convert', async () => { + for (const opener of ['~~~mermaid', `${FENCE} mermaid`]) { + const closer = opener.slice(0, 3) + const input = [opener, 'flowchart LR', ' A{{Hexagon}} --> B', closer].join('\n') + + expect(escapeLiquid(input)).toBe( + [opener, 'flowchart LR', ' A{% raw %}{{Hexagon}}{% endraw %} --> B', closer].join('\n'), + ) + await expectRendersUnchanged(input) + } + }) + + test('escapes content after a mermaid diagram closes', () => { + const input = [`${FENCE}mermaid`, 'flowchart LR', FENCE, '', 'Hello {{name}}'].join('\n') + + expect(escapeLiquid(input)).toBe( + [`${FENCE}mermaid`, 'flowchart LR', FENCE, '', 'Hello {% raw %}{{name}}{% endraw %}'].join( + '\n', + ), + ) + }) + + test('leaves text without Liquid syntax unchanged', async () => { + const input = [ + '# Title', + '', + 'A { single } brace, a closing }} pair, and 50% %} text.', + '', + `${FENCE}js`, + 'const obj = { a: { b: 1 } }', + FENCE, + ].join('\n') + + expect(escapeLiquid(input)).toBe(input) + await expectRendersUnchanged(input) + }) +})