diff --git a/.github/instructions/all.instructions.md b/.github/instructions/all.instructions.md index 6c093e5aaa67..aba2b7f9663f 100644 --- a/.github/instructions/all.instructions.md +++ b/.github/instructions/all.instructions.md @@ -28,4 +28,5 @@ When you create a pull request: 3. Label with "llm-generated". 4. If an issue exists, include "fixes owner/repo#issue" or "towards owner/repo#issue" as appropriate. -5. Always _escape backticks_ when you use gh cli. +5. Always create PRs in **draft mode** using `--draft` flag. +6. Always _escape backticks_ when you use gh cli. diff --git a/content/pull-requests/collaborating-with-pull-requests/proposing-changes-to-your-work-with-pull-requests/about-pull-requests.md b/content/pull-requests/collaborating-with-pull-requests/proposing-changes-to-your-work-with-pull-requests/about-pull-requests.md index ae7843547532..7dffb722ea36 100644 --- a/content/pull-requests/collaborating-with-pull-requests/proposing-changes-to-your-work-with-pull-requests/about-pull-requests.md +++ b/content/pull-requests/collaborating-with-pull-requests/proposing-changes-to-your-work-with-pull-requests/about-pull-requests.md @@ -30,7 +30,7 @@ After initializing a pull request, you'll see a review page that shows a high-le Once you've created a pull request, you can push commits from your topic branch to add them to your existing pull request. These commits will appear in chronological order within your pull request and the changes will be visible in the "Files changed" tab. -Other contributors can review your proposed changes, add review comments, contribute to the pull request discussion, and even add commits to the pull request. {% ifversion pull-request-approval-limit %}{% data reusables.pull_requests.code-review-limits %}{% endif %} +Other contributors can review your proposed changes, add review comments, contribute to the pull request discussion, and even add commits to the pull request. {% ifversion pull-request-approval-limit %}By default, in public repositories, any user can submit reviews that approve or request changes to a pull request. Organization owners and repository admins can limit who is able to give approving pull request reviews or request changes. For more information, see [AUTOTITLE](/organizations/managing-organization-settings/managing-pull-request-reviews-in-your-organization) and [AUTOTITLE](/repositories/managing-your-repositorys-settings-and-features/managing-repository-settings/managing-pull-request-reviews-in-your-repository).{% endif %} {% ifversion fpt or ghec %} You can see information about the branch's current deployment status and past deployment activity on the "Conversation" tab. See [AUTOTITLE](/repositories/viewing-activity-and-data-for-your-repository/viewing-deployment-activity-for-your-repository). diff --git a/content/pull-requests/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/about-pull-request-reviews.md b/content/pull-requests/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/about-pull-request-reviews.md index 370f5b33f37c..4cc440566f1e 100644 --- a/content/pull-requests/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/about-pull-request-reviews.md +++ b/content/pull-requests/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/about-pull-request-reviews.md @@ -1,6 +1,6 @@ --- title: About pull request reviews -intro: 'Reviews allow collaborators to comment on the changes proposed in pull requests, approve the changes, or request further changes before the pull request is merged. Repository administrators can require that all pull requests are approved before being merged.' +intro: 'Collaborate on pull requests to improve code quality.' redirect_from: - /github/collaborating-with-issues-and-pull-requests/reviewing-changes-in-pull-requests/about-pull-request-reviews - /articles/about-pull-request-reviews @@ -14,51 +14,31 @@ topics: - Pull requests shortTitle: About PR reviews --- -## About pull request reviews -After a pull request is opened, anyone with _read_ access can review and comment on the changes it proposes. You can also suggest specific changes to lines of code, which the author can apply directly from the pull request. For more information, see [AUTOTITLE](/pull-requests/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/reviewing-proposed-changes-in-a-pull-request). +Pull request reviews are one of the primary ways people collaborate on {% data variables.product.github %}. Reviewers can comment on changes, suggest improvements, and approve or request changes before code is merged. This collaborative process enables teams to ensure code quality and share knowledge. -{% ifversion pull-request-approval-limit %}{% data reusables.pull_requests.code-review-limits %}{% endif %} +{% raw %}View pull requests awaiting your review{% endraw %} -Repository owners and collaborators can request a pull request review from a specific person. Organization members can also request a pull request review from a team with read access to the repository. For more information, see [AUTOTITLE](/pull-requests/collaborating-with-pull-requests/proposing-changes-to-your-work-with-pull-requests/requesting-a-pull-request-review). You can specify a subset of team members to be automatically assigned in the place of the whole team. For more information, see [AUTOTITLE](/organizations/organizing-members-into-teams/managing-code-review-settings-for-your-team). +## Reviewing pull requests -Reviews allow for discussion of proposed changes and help ensure that the changes meet the repository's contributing guidelines and other quality standards. You can define which individuals or teams own certain types or areas of code in a CODEOWNERS file. When a pull request modifies code that has a defined owner, that individual or team will automatically be requested as a reviewer. For more information, see [AUTOTITLE](/repositories/managing-your-repositorys-settings-and-features/customizing-your-repository/about-code-owners). +Anyone with read access can review and comment on proposed changes. When submitting a review, choose one of three statuses: -For an introduction to requesting and providing pull request reviews, see the [Review pull requests](https://github.com/skills/review-pull-requests) {% data variables.product.prodname_learning %} course. +* **Comment**: Share feedback without approving or requesting changes. +* **Approve**: Approve the changes for merging. +* **Request changes**: Identify issues that must be fixed before merging. -{% ifversion fpt or ghec %}You can schedule reminders for pull requests that need to be reviewed. For more information, see [AUTOTITLE](/organizations/organizing-members-into-teams/managing-scheduled-reminders-for-your-team).{% endif %} +You can comment on specific lines, suggest changes for authors to apply directly, and discuss implementation approaches. Reviews appear in the conversation timeline and merge box. Mark conversation threads as resolved to track addressed feedback. -A review has three possible statuses: -* **Comment:** Submit general feedback without explicitly approving the changes or requesting additional changes. -* **Approve:** Submit feedback and approve merging the changes proposed in the pull request. -* **Request changes:** Submit feedback that must be addressed before the pull request can be merged. +## Requesting reviews -{% data reusables.repositories.request-changes-tips %} - -You can view all of the reviews a pull request has received in the Conversation timeline, and you can see reviews by repository owners and collaborators in the pull request's merge box. - -![Screenshot of the merge box for a pull request. A review by Octocat with requested changes is listed.](/assets/images/help/pull_requests/merge_box/pr-reviews-in-merge-box.png) - -{% data reusables.search.requested_reviews_search_tip %} - -{% data reusables.pull_requests.resolving-conversations %} - -## Re-requesting a review - -{% data reusables.pull_requests.re-request-review %} +Repository owners and collaborators can request reviews from specific people or teams. When you define code owners in a CODEOWNERS file, they're automatically requested as reviewers when a pull request modifies their code. You can re-request reviews after making significant changes. ## Required reviews -{% data reusables.pull_requests.required-reviews-for-prs-summary %} For more information, see [AUTOTITLE](/repositories/configuring-branches-and-merges-in-your-repository/managing-protected-branches/about-protected-branches#require-pull-request-reviews-before-merging). - -> [!TIP] -> If necessary, people with _admin_ or _write_ access to a repository can dismiss a pull request review. For more information, see [AUTOTITLE](/pull-requests/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/dismissing-a-pull-request-review). +Repository administrators can require approvals before pull requests are merged, ensuring code quality and preventing accidental merges. For more information, see [AUTOTITLE](/repositories/configuring-branches-and-merges-in-your-repository/managing-protected-branches/about-protected-branches#require-pull-request-reviews-before-merging). ## Further reading * [AUTOTITLE](/pull-requests/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/reviewing-proposed-changes-in-a-pull-request) -* [AUTOTITLE](/pull-requests/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/viewing-a-pull-request-review) -* [AUTOTITLE](/communities/setting-up-your-project-for-healthy-contributions/setting-guidelines-for-repository-contributors){% ifversion copilot %} -* [AUTOTITLE](/copilot/using-github-copilot/code-review/using-copilot-code-review) -* [AUTOTITLE](/copilot/using-github-copilot/coding-agent/using-copilot-to-work-on-an-issue){% endif %}{% ifversion code-quality %} -* [AUTOTITLE](/code-security/code-quality/tutorials/fix-findings-in-prs){% endif %} +* [AUTOTITLE](/pull-requests/collaborating-with-pull-requests/proposing-changes-to-your-work-with-pull-requests/requesting-a-pull-request-review) +* Learn more in the [Review pull requests](https://github.com/skills/review-pull-requests?ref_product=github&ref_type=engagement&ref_style=text) {% data variables.product.prodname_learning %} course diff --git a/content/pull-requests/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/commenting-on-a-pull-request.md b/content/pull-requests/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/commenting-on-a-pull-request.md index c47b8101bfe4..a95b8e915b75 100644 --- a/content/pull-requests/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/commenting-on-a-pull-request.md +++ b/content/pull-requests/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/commenting-on-a-pull-request.md @@ -54,7 +54,23 @@ To reply to an existing line or file comment, you'll need to navigate to the com Anyone watching the pull request or repository will receive a notification of your comments. Batching your comments avoids multiple notifications being sent. {% ifversion copilot %}If you are commenting on a pull request created by {% data variables.product.prodname_copilot_short %}, batching your comments prevents {% data variables.product.prodname_copilot_short %} from starting to work on individual comments before you have completed your review. See [AUTOTITLE](/copilot/using-github-copilot/coding-agent/using-copilot-to-work-on-an-issue).{% endif %} -{% data reusables.pull_requests.resolving-conversations %} +### Resolving conversations + +You can resolve a conversation in a pull request if you opened the pull request or if you have write access to the repository where the pull request was opened. + +To indicate that a conversation on the **Files changed** tab is complete, click **Resolve conversation**. + +The entire conversation will be collapsed and marked as resolved, making it easier to find conversations that still need to be addressed. + +If the suggestion in a comment is out of your pull request's scope, you can open a new issue that tracks the feedback and links back to the original comment. For more information, see [AUTOTITLE](/issues/tracking-your-work-with-issues/creating-an-issue#creating-an-issue-from-a-comment). + +#### Discovering and navigating conversations + +You can discover and navigate to all the conversations in your pull request using the **Conversations** menu that's shown at the top of the **Files Changed** tab. + +From this view, you can see which conversations are unresolved, resolved, and outdated. This makes it easy to discover and resolve conversations. + +![Screenshot of the "Conversations" menu on the "Files Changed" tab of a pull request.](/assets/images/help/pull_requests/conversations-menu.png) ## Further reading diff --git a/content/pull-requests/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/incorporating-feedback-in-your-pull-request.md b/content/pull-requests/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/incorporating-feedback-in-your-pull-request.md index b7935b568244..f02f6fe0aaa4 100644 --- a/content/pull-requests/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/incorporating-feedback-in-your-pull-request.md +++ b/content/pull-requests/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/incorporating-feedback-in-your-pull-request.md @@ -38,7 +38,8 @@ Each person who suggested a change included in the commit will be a co-author of ## Re-requesting a review -{% data reusables.pull_requests.re-request-review %} +You can re-request a review, for example, after you've made substantial changes to your pull request. +To request a fresh review from a reviewer, in the sidebar of the **Conversation** tab, click the {% octicon "sync" aria-label="The sync icon" %} icon. ## Opening an issue for an out-of-scope suggestion diff --git a/data/reusables/pull_requests/code-review-limits.md b/data/reusables/pull_requests/code-review-limits.md deleted file mode 100644 index 9676d53d3723..000000000000 --- a/data/reusables/pull_requests/code-review-limits.md +++ /dev/null @@ -1 +0,0 @@ -By default, in public repositories, any user can submit reviews that approve or request changes to a pull request. Organization owners and repository admins can limit who is able to give approving pull request reviews or request changes. For more information, see [AUTOTITLE](/organizations/managing-organization-settings/managing-pull-request-reviews-in-your-organization) and [AUTOTITLE](/repositories/managing-your-repositorys-settings-and-features/managing-repository-settings/managing-pull-request-reviews-in-your-repository). diff --git a/data/reusables/pull_requests/re-request-review.md b/data/reusables/pull_requests/re-request-review.md deleted file mode 100644 index 5776451f772f..000000000000 --- a/data/reusables/pull_requests/re-request-review.md +++ /dev/null @@ -1,2 +0,0 @@ -You can re-request a review, for example, after you've made substantial changes to your pull request. -To request a fresh review from a reviewer, in the sidebar of the **Conversation** tab, click the {% octicon "sync" aria-label="The sync icon" %} icon. diff --git a/data/reusables/pull_requests/resolving-conversations.md b/data/reusables/pull_requests/resolving-conversations.md deleted file mode 100644 index 83d1c02c6669..000000000000 --- a/data/reusables/pull_requests/resolving-conversations.md +++ /dev/null @@ -1,17 +0,0 @@ -### Resolving conversations - -You can resolve a conversation in a pull request if you opened the pull request or if you have write access to the repository where the pull request was opened. - -To indicate that a conversation on the **Files changed** tab is complete, click **Resolve conversation**. - -The entire conversation will be collapsed and marked as resolved, making it easier to find conversations that still need to be addressed. - -If the suggestion in a comment is out of your pull request's scope, you can open a new issue that tracks the feedback and links back to the original comment. For more information, see [AUTOTITLE](/issues/tracking-your-work-with-issues/creating-an-issue). - -#### Discovering and navigating conversations - -You can discover and navigate to all the conversations in your pull request using the **Conversations** menu that's shown at the top of the **Files Changed** tab. - -From this view, you can see which conversations are unresolved, resolved, and outdated. This makes it easy to discover and resolve conversations. - -![Screenshot of the "Conversations" menu on the "Files Changed" tab of a pull request.](/assets/images/help/pull_requests/conversations-menu.png) diff --git a/data/ui.yml b/data/ui.yml index 1e3986a0f4ec..ff8b52542c51 100644 --- a/data/ui.yml +++ b/data/ui.yml @@ -258,6 +258,7 @@ footer: machine: Some of this content may be machine- or AI-translated. journey_landing: articles: '{{ number }} Articles' + articles_heading: Articles product_landing: article_grid: heading: Articles diff --git a/src/content-linter/lib/linting-rules/liquid-ifversion-versions.ts b/src/content-linter/lib/linting-rules/liquid-ifversion-versions.ts index c56439c41d64..96b7b70e6962 100644 --- a/src/content-linter/lib/linting-rules/liquid-ifversion-versions.ts +++ b/src/content-linter/lib/linting-rules/liquid-ifversion-versions.ts @@ -206,9 +206,10 @@ async function getApplicableVersionFromLiquidTag(conditionStr: string) { const ands = ver.split(' and ') const firstAnd = ands[0].split(' ')[0] // if all ands don't start with the same version it's invalid + // Note: This edge case (e.g., "fpt and ghes >= 3.1") doesn't occur in our content. + // All actual uses have matching versions (e.g., "ghes and ghes > 3.19"). + // If this edge case appears in the future, additional logic would be needed here. if (!ands.every((and) => and.startsWith(firstAnd))) { - // noop - we don't handle this case - // TODO - handle this case in the future return [] } const andValues = [] diff --git a/src/content-linter/tests/category-pages.ts b/src/content-linter/tests/category-pages.ts index a6add3b57499..5519287ee36f 100644 --- a/src/content-linter/tests/category-pages.ts +++ b/src/content-linter/tests/category-pages.ts @@ -50,14 +50,17 @@ describe.skip('category pages', () => { // otherwise, if one of them has no categories, the tests will fail. for (const tuple of productTuples) { const [, productIndex] = tuple + + const productDir = path.dirname(productIndex) + // Get links included in product index page. // Each link corresponds to a product subdirectory (category). // Example: "getting-started-with-github" + // Note: We need to read this synchronously here because vitest's describe.each + // can't asynchronously define tests const contents = fs.readFileSync(productIndex, 'utf8') const data = getFrontmatterData(contents) - const productDir = path.dirname(productIndex) - const children: string[] = data.children const categoryLinks = children // Only include category directories, not standalone category files like content/actions/quickstart.md @@ -118,15 +121,19 @@ describe.skip('category pages', () => { await contextualize(req as ExtendedRequest, res as Response, next) await shortVersions(req as ExtendedRequest, res as Response, next) + // Read the product index data for rendering + const productIndexContents = await fs.promises.readFile(productIndex, 'utf8') + const productIndexData = getFrontmatterData(productIndexContents) + // Save the index title for later testing - indexTitle = data.title.includes('{') - ? await renderContent(data.title, req.context, { textOnly: true }) - : data.title - - if (data.shortTitle) { - indexShortTitle = data.shortTitle.includes('{') - ? await renderContent(data.shortTitle, req.context, { textOnly: true }) - : data.shortTitle + indexTitle = productIndexData.title.includes('{') + ? await renderContent(productIndexData.title, req.context, { textOnly: true }) + : productIndexData.title + + if (productIndexData.shortTitle) { + indexShortTitle = productIndexData.shortTitle.includes('{') + ? await renderContent(productIndexData.shortTitle, req.context, { textOnly: true }) + : productIndexData.shortTitle } else { indexShortTitle = '' } diff --git a/src/fixtures/fixtures/data/ui.yml b/src/fixtures/fixtures/data/ui.yml index 1e3986a0f4ec..ff8b52542c51 100644 --- a/src/fixtures/fixtures/data/ui.yml +++ b/src/fixtures/fixtures/data/ui.yml @@ -258,6 +258,7 @@ footer: machine: Some of this content may be machine- or AI-translated. journey_landing: articles: '{{ number }} Articles' + articles_heading: Articles product_landing: article_grid: heading: Articles diff --git a/src/fixtures/tests/minitoc.ts b/src/fixtures/tests/minitoc.ts index 1fbeaf279f7f..9d2475853acd 100644 --- a/src/fixtures/tests/minitoc.ts +++ b/src/fixtures/tests/minitoc.ts @@ -4,7 +4,6 @@ import cheerio from 'cheerio' import { getDOM } from '@/tests/helpers/e2etest' describe('minitoc', () => { - // TODO disable the mini TOC tests when we replace it with sticky TOC header test('renders mini TOC in articles with more than one heading', async () => { const $: cheerio.Root = await getDOM('/en/get-started/minitocs/multiple-headings') expect($('h2#in-this-article').length).toBe(1) diff --git a/src/fixtures/tests/playwright-rendering.spec.ts b/src/fixtures/tests/playwright-rendering.spec.ts index bc775209b347..b45cecb73594 100644 --- a/src/fixtures/tests/playwright-rendering.spec.ts +++ b/src/fixtures/tests/playwright-rendering.spec.ts @@ -1071,7 +1071,7 @@ test.describe('Journey Tracks', () => { // Verify track has proper structure const firstTrack = tracks.first() - await expect(firstTrack.locator('h3')).toBeVisible() // Track title + await expect(firstTrack.locator('h2')).toBeVisible() // Track title await expect(firstTrack.locator('p')).toBeVisible() // Track description }) diff --git a/src/frame/lib/path-utils.ts b/src/frame/lib/path-utils.ts index 64c3fb1eef82..ff6f1d181c82 100644 --- a/src/frame/lib/path-utils.ts +++ b/src/frame/lib/path-utils.ts @@ -109,32 +109,37 @@ export function getVersionObjectFromPath(href: string | undefined) { return allVersions[versionFromPath] } -// TODO needs refactoring + tests // Return the product segment from the path +// Extracts the product identifier from various URL patterns including versioned paths export function getProductStringFromPath(href: string | undefined): string { + // Handle empty or undefined paths if (!href) return 'homepage' - href = getPathWithoutLanguage(href) - if (href === '/') return 'homepage' + const normalizedHref = getPathWithoutLanguage(href) + if (normalizedHref === '/') return 'homepage' - // The first segment will always be empty on this split - const pathParts = href.split('/') + // Split path into segments (first segment is always empty string) + const pathParts = normalizedHref.split('/') + // Handle special product paths that appear anywhere in the URL if (pathParts.includes('early-access')) return 'early-access' - // For rest pages the currentProduct should be rest - // We use this to show SidebarRest, which is a different sidebar than the rest of the site - if (pathParts[1] === 'rest') return 'rest' - if (pathParts[1] === 'copilot') return 'copilot' - if (pathParts[1] === 'get-started') return 'get-started' - - // Possible scenarios for href (assume part[0] is an empty string): - // - // * part[1] is a version and part[2] is undefined, so return part[1] as an enterprise landing page - // * part[1] is a version and part[2] is defined, so return part[2] as the product - // * part[1] is NOT a version, so return part[1] as the product - const isEnterprise = supportedVersions.has(pathParts[1]) - const productString = isEnterprise && pathParts[2] ? pathParts[2] : pathParts[1] + // Handle special products that always appear as the first segment + // These products use custom sidebars and need explicit handling + const specialProducts = ['rest', 'copilot', 'get-started'] + if (specialProducts.includes(pathParts[1])) { + return pathParts[1] + } + + // Determine if first segment is a version (e.g., 'enterprise-server@3.9') + // If yes, product is in pathParts[2], otherwise it's in pathParts[1] + // Examples: + // /enterprise-server@3.9/admin -> product is 'admin' + // /github/getting-started -> product is 'github' + // /enterprise-server@3.9 -> product is 'enterprise-server@3.9' (enterprise landing) + const hasVersionPrefix = supportedVersions.has(pathParts[1]) + const productString = hasVersionPrefix && pathParts[2] ? pathParts[2] : pathParts[1] + return productString } diff --git a/src/frame/middleware/find-page.ts b/src/frame/middleware/find-page.ts index 617b162d901d..a78feb5c3dc1 100644 --- a/src/frame/middleware/find-page.ts +++ b/src/frame/middleware/find-page.ts @@ -105,10 +105,9 @@ async function rereadByPath( const languageCode = match[1] const withoutLanguage = uri.replace(languagePrefixPathRegex, '/') const withoutVersion = withoutLanguage.replace(`/${currentVersion}`, '') - // TODO: Support loading translations the same way. - // NOTE: No one is going to test translations like this in development - // but perhaps one day we can always and only do these kinds of lookups - // at runtime. + // Note: We don't support loading translations at runtime. All translations + // are loaded at build time. This function only handles English content reloading + // during development. const possible = path.join(contentRoot, withoutVersion) const filePath = existsSync(possible) ? path.join(possible, 'index.md') : `${possible}.md` const relativePath = path.relative(contentRoot, filePath) diff --git a/src/frame/tests/path-utils.ts b/src/frame/tests/path-utils.ts new file mode 100644 index 000000000000..1df7e18f0bba --- /dev/null +++ b/src/frame/tests/path-utils.ts @@ -0,0 +1,47 @@ +import { describe, expect, test } from 'vitest' + +import { getProductStringFromPath } from '@/frame/lib/path-utils' + +describe('getProductStringFromPath', () => { + test('returns "homepage" for root paths', () => { + expect(getProductStringFromPath('/')).toBe('homepage') + expect(getProductStringFromPath('/en')).toBe('homepage') + expect(getProductStringFromPath(undefined)).toBe('homepage') + }) + + test('handles early-access product', () => { + expect(getProductStringFromPath('/en/early-access/github/overview')).toBe('early-access') + expect(getProductStringFromPath('/early-access/admin/guides')).toBe('early-access') + }) + + test('handles special products (rest, copilot, get-started)', () => { + expect(getProductStringFromPath('/en/rest/overview')).toBe('rest') + expect(getProductStringFromPath('/rest/reference/repos')).toBe('rest') + expect(getProductStringFromPath('/en/copilot/using-copilot')).toBe('copilot') + expect(getProductStringFromPath('/copilot/features')).toBe('copilot') + expect(getProductStringFromPath('/en/get-started/quickstart')).toBe('get-started') + expect(getProductStringFromPath('/get-started/onboarding')).toBe('get-started') + }) + + test('extracts product from non-versioned paths', () => { + expect(getProductStringFromPath('/en/github/getting-started')).toBe('github') + expect(getProductStringFromPath('/actions/quickstart')).toBe('actions') + expect(getProductStringFromPath('/en/admin/installation')).toBe('admin') + expect(getProductStringFromPath('/code-security/overview')).toBe('code-security') + }) + + test('extracts product from versioned paths', () => { + // Note: These tests use free-pro-team which is a supported version + expect(getProductStringFromPath('/en/free-pro-team@latest/admin/installation')).toBe('admin') + expect(getProductStringFromPath('/free-pro-team@latest/actions/quickstart')).toBe('actions') + expect(getProductStringFromPath('/en/free-pro-team@latest/github/getting-started')).toBe( + 'github', + ) + }) + + test('handles enterprise landing pages (version without product)', () => { + // When a version is present but no product segment follows, return the version string + expect(getProductStringFromPath('/en/free-pro-team@latest')).toBe('free-pro-team@latest') + expect(getProductStringFromPath('/enterprise-server@latest')).toBe('enterprise-server@latest') + }) +}) diff --git a/src/landings/components/journey/JourneyLearningTracks.tsx b/src/landings/components/journey/JourneyLearningTracks.tsx index 20cbcc355b34..dc9e456a9fdd 100644 --- a/src/landings/components/journey/JourneyLearningTracks.tsx +++ b/src/landings/components/journey/JourneyLearningTracks.tsx @@ -2,6 +2,7 @@ import { ChevronDownIcon, ChevronUpIcon } from '@primer/octicons-react' import { Details, Timeline, Token, useDetails } from '@primer/react' import { Link } from '@/frame/components/Link' +import { useTranslation } from '@/languages/components/useTranslation' import { JourneyTrack } from '@/journeys/lib/journey-path-resolver' import styles from './JourneyLearningTracks.module.scss' @@ -10,6 +11,8 @@ type JourneyLearningTracksProps = { } export const JourneyLearningTracks = ({ tracks }: JourneyLearningTracksProps) => { + const { t } = useTranslation('journey_landing') + if (!tracks || tracks.length === 0) { return null } @@ -26,7 +29,7 @@ export const JourneyLearningTracks = ({ tracks }: JourneyLearningTracksProps) => >
-

{track.title}

+

{track.title}

@@ -49,6 +52,29 @@ export const JourneyLearningTracks = ({ tracks }: JourneyLearningTracksProps) => ) } + // simple single journey + if (tracks.length === 1) { + const track = tracks[0] + + return ( +
+
+

{t('articles_heading')}

+
+
    + {(track.guides || []).map((article: { href: string; title: string }) => ( +
  1. + + {article.title} + +
  2. + ))} +
+
+ ) + } + + // more than one journey has timeline and mobile layout return (
{/* Desktop: Timeline component */} diff --git a/src/rest/scripts/utils/create-rest-examples.ts b/src/rest/scripts/utils/create-rest-examples.ts index a1ace57a182a..605d0701bfa0 100644 --- a/src/rest/scripts/utils/create-rest-examples.ts +++ b/src/rest/scripts/utils/create-rest-examples.ts @@ -352,10 +352,9 @@ export function getResponseExamples(operation: Operation): ResponseExample[] { contentType, description: examples[key].summary || operation.responses[statusCode].description, example: examples[key].value, - // TODO adding the schema quadruples the JSON file size. Changing - // how we write the JSON file helps a lot, but we should revisit - // adding the response schema to ensure we have a way to view the - // prettified JSON before minimizing it. + // Note: Including the schema significantly increases JSON file size (~4x), + // but it's necessary to support the schema/example toggle in the UI. + // Users can switch between viewing the example response and the full schema. schema: operation.responses[statusCode].content[contentType].schema, }, } diff --git a/src/rest/scripts/utils/inject-models-schema.ts b/src/rest/scripts/utils/inject-models-schema.ts index 4b40dd0c0ac9..485df1d67e04 100644 --- a/src/rest/scripts/utils/inject-models-schema.ts +++ b/src/rest/scripts/utils/inject-models-schema.ts @@ -64,36 +64,18 @@ export async function injectModelsSchema(schema: any, schemaName: string): Promi // Use values from the YAML where possible const name = operationObject.summary || '' - const description = operationObject.description || '' - const category = operationObject['x-github']?.category || 'models' console.log(`⏳ Processing operation: ${name} (${path} ${operation})`) - // Create enhanced operation preserving all original fields - // TODO this should be cleaned up, most can be removed + // Create enhanced operation with custom fields needed for our REST docs + // The spread operator preserves all original OpenAPI fields const enhancedOperation = { - ...operationObject, // Keep all original fields - operationId: operationObject.operationId, // Preserve original operationId with namespace - tags: operationObject.tags || ['models'], // Only use 'models' if no tags present + ...operationObject, + // Add custom fields for our docs processing verb: operation, requestPath: path, - category, - subcategory: operationObject['x-github']?.subcategory || '', - summary: name, - description, - 'x-github': { - ...operationObject['x-github'], // Preserve all x-github metadata - category, - enabledForGitHubApps: operationObject['x-github']?.enabledForGitHubApps, - githubCloudOnly: operationObject['x-github']?.githubCloudOnly, - permissions: operationObject['x-github']?.permissions || {}, - externalDocs: operationObject['x-github']?.externalDocs || {}, - }, - parameters: operationObject.parameters || [], - responses: { - ...operationObject.responses, - '200': operationObject.responses?.['200'], - }, + // Override tags with default if not present + tags: operationObject.tags || ['models'], } // Preserve operation-level servers if present diff --git a/src/search/lib/elasticsearch-indexes.ts b/src/search/lib/elasticsearch-indexes.ts index db6643e7f81d..d629ff6b23c4 100644 --- a/src/search/lib/elasticsearch-indexes.ts +++ b/src/search/lib/elasticsearch-indexes.ts @@ -71,10 +71,9 @@ export function getElasticSearchIndex( // e.g. free-pro-team becomes fpt for the index name let indexVersion = versionToIndexVersionMap[version] - // TODO: For AI Search, we initially only supported the latest GHES version - // Supporting more versions would involve adding more indexes and generating the data to fill them - // As a work around, we will just use the latest version for all GHES suggestions / autocomplete - // This is a temporary fix until we can support more versions + // For AI Search autocomplete, we use the latest GHES version for all GHES versions. + // This provides AI search functionality across all supported GHES versions without + // requiring separate indexes for each version. if (type === 'aiSearchAutocomplete' && indexVersion.startsWith('ghes')) { indexVersion = versionToIndexVersionMap['enterprise-server'] } diff --git a/src/search/lib/helpers/old-version-logic.ts b/src/search/lib/helpers/old-version-logic.ts deleted file mode 100644 index e582cc36b7b7..000000000000 --- a/src/search/lib/helpers/old-version-logic.ts +++ /dev/null @@ -1,44 +0,0 @@ -import { allVersions } from '@/versions/lib/all-versions' - -// TODO: Old version logic -type VersionAliases = { [key: string]: string } -export const versionAliases: VersionAliases = {} -export const prefixVersionAliases: VersionAliases = {} - -for (const info of Object.values(allVersions)) { - if (info.hasNumberedReleases) { - versionAliases[info.currentRelease] = info.miscVersionName - } else { - versionAliases[info.version] = info.miscVersionName - versionAliases[info.miscVersionName] = info.miscVersionName - } - prefixVersionAliases[info.plan] = info.shortName - prefixVersionAliases[info.shortName] = info.shortName -} - -// Temporary hard-coded switch -// -// We need to run workflows in production to index the search data -// We want the middleware + routes that consume the indexes to consume the old indexes -// until the new indexes are ready. - -// Once they are ready we can remove this file & cleanup the places it is used -export function isBeforeSearchIndexMigration() { - if (process.env.NODE_ENV === 'production') return true - return false -} - -// Old test prefix helper function -export function getGeneralSearchIndexPrefix(): string { - if (process.env.NODE_ENV === 'test') return 'tests_' - return '' -} - -export function getGeneralSearchIndexVersion(paramVersion: string): string { - const version = - prefixVersionAliases[paramVersion] || - versionAliases[paramVersion] || - allVersions[paramVersion].miscVersionName - - return version -} diff --git a/src/search/lib/routes/ai-search-autocomplete-route.ts b/src/search/lib/routes/ai-search-autocomplete-route.ts new file mode 100644 index 000000000000..1b421afe1bae --- /dev/null +++ b/src/search/lib/routes/ai-search-autocomplete-route.ts @@ -0,0 +1,44 @@ +import type { Request, Response } from 'express' + +import { searchCacheControl } from '@/frame/middleware/cache-control' +import { setFastlySurrogateKey, SURROGATE_ENUMS } from '@/frame/middleware/set-fastly-surrogate-key' +import { getAISearchAutocompleteResults } from '@/search/lib/get-elasticsearch-results/ai-search-autocomplete' +import { getSearchFromRequestParams } from '@/search/lib/search-request-params/get-search-from-request-params' +import { handleGetSearchResultsError } from '@/search/middleware/search-routes' + +export async function aiSearchAutocompleteRoute(req: Request, res: Response) { + // If no query is provided, we want to return the top 5 most popular terms + // This is a special case for AI search autocomplete + // So we use `force` to allow the query to be empty without the usual validation error + const force = {} as any + if (!req.query.query) { + force.query = '' + } + const { + indexName, + validationErrors, + searchParams: { query, size, debug }, + } = getSearchFromRequestParams(req, 'aiSearchAutocomplete', force) + if (validationErrors.length) { + return res.status(400).json(validationErrors[0]) + } + + const getResultOptions = { + indexName, + query, + size, + debug, + } + try { + const { meta, hits } = await getAISearchAutocompleteResults(getResultOptions) + + if (process.env.NODE_ENV !== 'development') { + searchCacheControl(res) + setFastlySurrogateKey(res, SURROGATE_ENUMS.DEFAULT) + } + + res.status(200).json({ meta, hits }) + } catch (error) { + await handleGetSearchResultsError(req, res, error, getResultOptions) + } +} diff --git a/src/search/lib/routes/general-search-route.ts b/src/search/lib/routes/general-search-route.ts new file mode 100644 index 000000000000..d4858316f146 --- /dev/null +++ b/src/search/lib/routes/general-search-route.ts @@ -0,0 +1,47 @@ +import type { Request, Response } from 'express' + +import { searchCacheControl } from '@/frame/middleware/cache-control' +import { setFastlySurrogateKey, SURROGATE_ENUMS } from '@/frame/middleware/set-fastly-surrogate-key' +import { getSearchFromRequestParams } from '@/search/lib/search-request-params/get-search-from-request-params' +import { getGeneralSearchResults } from '@/search/lib/get-elasticsearch-results/general-search' +import { handleGetSearchResultsError } from '@/search/middleware/search-routes' +import { handleExternalSearchAnalytics } from '@/search/lib/helpers/external-search-analytics' + +export async function generalSearchRoute(req: Request, res: Response) { + const { indexName, searchParams, validationErrors } = getSearchFromRequestParams( + req, + 'generalSearch', + ) + if (validationErrors.length) { + // We only send the first validation error to the user + return res.status(400).json(validationErrors[0]) + } + + // Handle search analytics and client_name validation + const analyticsError = await handleExternalSearchAnalytics(req, 'general-search') + if (analyticsError) { + return res.status(analyticsError.status).json({ + error: analyticsError.error, + }) + } + + const getResultOptions = { + indexName, + searchParams, + } + try { + const { meta, hits, aggregations } = await getGeneralSearchResults(getResultOptions) + + if (process.env.NODE_ENV !== 'development') { + searchCacheControl(res) + // We can cache this without purging it after every deploy + // because the API search is only used as a proxy for local + // and review environments. + setFastlySurrogateKey(res, SURROGATE_ENUMS.MANUAL) + } + + res.status(200).json({ meta, hits, aggregations }) + } catch (error) { + await handleGetSearchResultsError(req, res, error, getResultOptions) + } +} diff --git a/src/search/middleware/search-routes.ts b/src/search/middleware/search-routes.ts index 145addcf3a67..89342dae0968 100644 --- a/src/search/middleware/search-routes.ts +++ b/src/search/middleware/search-routes.ts @@ -3,18 +3,13 @@ For general search (client searches on docs.github.com) we use the middleware in ./general-search-middleware to get the search results */ -// TODO: Move the routes implementations in this files to lib/routes so you can at-a-glance see all of the routes without the implementation logic import express, { Request, Response } from 'express' import FailBot from '@/observability/lib/failbot' -import { searchCacheControl } from '@/frame/middleware/cache-control' import catchMiddlewareError from '@/observability/middleware/catch-middleware-error' -import { setFastlySurrogateKey, SURROGATE_ENUMS } from '@/frame/middleware/set-fastly-surrogate-key' -import { getAISearchAutocompleteResults } from '@/search/lib/get-elasticsearch-results/ai-search-autocomplete' -import { getSearchFromRequestParams } from '@/search/lib/search-request-params/get-search-from-request-params' -import { getGeneralSearchResults } from '@/search/lib/get-elasticsearch-results/general-search' +import { generalSearchRoute } from '@/search/lib/routes/general-search-route' +import { aiSearchAutocompleteRoute } from '@/search/lib/routes/ai-search-autocomplete-route' import { combinedSearchRoute } from '@/search/lib/routes/combined-search-route' -import { handleExternalSearchAnalytics } from '@/search/lib/helpers/external-search-analytics' const router = express.Router() @@ -22,96 +17,13 @@ router.get('/legacy', (req: Request, res: Response) => { res.status(410).send('Use /api/search/v1 instead.') }) -router.get( - '/v1', - catchMiddlewareError(async (req: Request, res: Response) => { - const { indexName, searchParams, validationErrors } = getSearchFromRequestParams( - req, - 'generalSearch', - ) - if (validationErrors.length) { - // We only send the first validation error to the user - return res.status(400).json(validationErrors[0]) - } +router.get('/v1', catchMiddlewareError(generalSearchRoute)) - // Handle search analytics and client_name validation - const analyticsError = await handleExternalSearchAnalytics(req, 'general-search') - if (analyticsError) { - return res.status(analyticsError.status).json({ - error: analyticsError.error, - }) - } - - const getResultOptions = { - indexName, - searchParams, - } - try { - const { meta, hits, aggregations } = await getGeneralSearchResults(getResultOptions) - - if (process.env.NODE_ENV !== 'development') { - searchCacheControl(res) - // We can cache this without purging it after every deploy - // because the API search is only used as a proxy for local - // and review environments. - setFastlySurrogateKey(res, SURROGATE_ENUMS.MANUAL) - } - - res.status(200).json({ meta, hits, aggregations }) - } catch (error) { - await handleGetSearchResultsError(req, res, error, getResultOptions) - } - }), -) - -router.get( - '/ai-search-autocomplete/v1', - catchMiddlewareError(async (req: Request, res: Response) => { - // If no query is provided, we want to return the top 5 most popular terms - // This is a special case for AI search autocomplete - // So we use `force` to allow the query to be empty without the usual validation error - const force = {} as any - if (!req.query.query) { - force.query = '' - } - const { - indexName, - validationErrors, - searchParams: { query, size, debug }, - } = getSearchFromRequestParams(req, 'aiSearchAutocomplete', force) - if (validationErrors.length) { - return res.status(400).json(validationErrors[0]) - } - - const getResultOptions = { - indexName, - query, - size, - debug, - } - try { - const { meta, hits } = await getAISearchAutocompleteResults(getResultOptions) - - if (process.env.NODE_ENV !== 'development') { - searchCacheControl(res) - setFastlySurrogateKey(res, SURROGATE_ENUMS.DEFAULT) - } - - res.status(200).json({ meta, hits }) - } catch (error) { - await handleGetSearchResultsError(req, res, error, getResultOptions) - } - }), -) +router.get('/ai-search-autocomplete/v1', catchMiddlewareError(aiSearchAutocompleteRoute)) // Route used by our frontend to fetch ai autocomplete search suggestions + general search results in a single request // Combining this into a single request results in less overall requests to the server -router.get( - '/combined-search/v1', - catchMiddlewareError(async (req: Request, res: Response) => { - combinedSearchRoute(req, res) - }), -) +router.get('/combined-search/v1', catchMiddlewareError(combinedSearchRoute)) export async function handleGetSearchResultsError( req: Request, diff --git a/src/search/scripts/scrape/lib/parse-page-sections-into-records.ts b/src/search/scripts/scrape/lib/parse-page-sections-into-records.ts index 8c7a8ecfeda8..c862ad5d2464 100644 --- a/src/search/scripts/scrape/lib/parse-page-sections-into-records.ts +++ b/src/search/scripts/scrape/lib/parse-page-sections-into-records.ts @@ -70,9 +70,8 @@ export default function parsePageSectionsIntoRecords(page: any): Record { // We need to avoid these because if you use `getAllText()` on these // pages, it will extract *everything* from the page, which will // include the side bar and footer. - // TODO: Come up a custom solution to extract some text from these - // pages that yields some decent content to be searched on, because - // when you view these pages in a browser, there's clearly text there. + // Note: We're not adding custom extraction for guide pages as they are + // being phased out and don't warrant the effort. if ($root.length > 0) { body = render($root) } diff --git a/src/types/types.ts b/src/types/types.ts index dddf8cc4bc66..343ce700ab1c 100644 --- a/src/types/types.ts +++ b/src/types/types.ts @@ -27,8 +27,12 @@ export type ExtendedRequest = Request & { FailBot?: Failbot } -// TODO: Make this type from inference using AJV based on the schema. -// For now, it's based on `schema` in frame/lib/frontmatter.ts +// This type is manually maintained based on `schema` in frame/lib/frontmatter.ts +// We're not auto-generating this from the AJV schema because: +// 1. It would require significant build tooling (json-schema-to-typescript or similar) +// 2. The schema is dynamically constructed with version-specific properties +// 3. Manual maintenance provides better type control and documentation +// 4. The effort/benefit tradeoff doesn't justify the complexity export type PageFrontmatter = { title: string versions: FrontmatterVersions