diff --git a/src/content-linter/lib/linting-rules/frontmatter-landing-recommended.js b/src/content-linter/lib/linting-rules/frontmatter-landing-recommended.js index cdbc1667b19a..06bdbb4774cf 100644 --- a/src/content-linter/lib/linting-rules/frontmatter-landing-recommended.js +++ b/src/content-linter/lib/linting-rules/frontmatter-landing-recommended.js @@ -5,13 +5,23 @@ import { addError } from 'markdownlint-rule-helpers' import { getFrontmatter } from '../helpers/utils' function isValidArticlePath(articlePath, currentFilePath) { - // Article paths in recommended are relative to the current page's directory - const relativePath = articlePath.startsWith('/') ? articlePath.substring(1) : articlePath + const ROOT = process.env.ROOT || '.' + + // Strategy 1: Always try as an absolute path from content root first + const contentDir = path.join(ROOT, 'content') + const normalizedPath = articlePath.startsWith('/') ? articlePath.substring(1) : articlePath + const absolutePath = path.join(contentDir, `${normalizedPath}.md`) + + if (fs.existsSync(absolutePath) && fs.statSync(absolutePath).isFile()) { + return true + } + + // Strategy 2: Fall back to relative path from current file's directory const currentDir = path.dirname(currentFilePath) - const fullPath = path.join(currentDir, `${relativePath}.md`) + const relativePath = path.join(currentDir, `${normalizedPath}.md`) try { - return fs.existsSync(fullPath) && fs.statSync(fullPath).isFile() + return fs.existsSync(relativePath) && fs.statSync(relativePath).isFile() } catch { return false } diff --git a/src/content-linter/tests/fixtures/landing-recommended/test-absolute-only.md b/src/content-linter/tests/fixtures/landing-recommended/test-absolute-only.md new file mode 100644 index 000000000000..964f85cf6a49 --- /dev/null +++ b/src/content-linter/tests/fixtures/landing-recommended/test-absolute-only.md @@ -0,0 +1,13 @@ +--- +title: Test Absolute Only Path +layout: product-landing +versions: + fpt: '*' +recommended: + - /article-two +--- + +# Test Absolute Only Path + +This tests /article-two which exists in content/article-two.md but NOT in the current directory. +If relative paths were tried first, this would fail. diff --git a/src/content-linter/tests/fixtures/landing-recommended/test-absolute-priority.md b/src/content-linter/tests/fixtures/landing-recommended/test-absolute-priority.md new file mode 100644 index 000000000000..bc529b8d8b3e --- /dev/null +++ b/src/content-linter/tests/fixtures/landing-recommended/test-absolute-priority.md @@ -0,0 +1,13 @@ +--- +title: Test Absolute Path Priority +layout: product-landing +versions: + fpt: '*' +recommended: + - /article-one + - /subdir/article-three +--- + +# Test Absolute Path Priority + +Testing that absolute paths are prioritized over relative paths. diff --git a/src/content-linter/tests/fixtures/landing-recommended/test-path-priority.md b/src/content-linter/tests/fixtures/landing-recommended/test-path-priority.md new file mode 100644 index 000000000000..226c7f956a91 --- /dev/null +++ b/src/content-linter/tests/fixtures/landing-recommended/test-path-priority.md @@ -0,0 +1,17 @@ +--- +title: Test Path Priority Resolution +layout: product-landing +versions: + fpt: '*' +recommended: + - /article-one +--- + +# Test Path Priority Resolution + +This tests that /article-one resolves to the absolute path: + tests/fixtures/fixtures/content/article-one.md (absolute from fixtures root) +NOT the relative path: + tests/fixtures/landing-recommended/article-one.md (relative to this file) + +The absolute path should be prioritized over the relative path. diff --git a/src/content-linter/tests/fixtures/landing-recommended/test-priority-validation.md b/src/content-linter/tests/fixtures/landing-recommended/test-priority-validation.md new file mode 100644 index 000000000000..248f88d8433c --- /dev/null +++ b/src/content-linter/tests/fixtures/landing-recommended/test-priority-validation.md @@ -0,0 +1,14 @@ +--- +title: Test Priority Validation +layout: product-landing +versions: + fpt: '*' +recommended: + - /article-one + - /nonexistent-absolute +--- + +# Test Priority Validation + +This tests that /article-one resolves correctly AND that /nonexistent-absolute fails properly. +The first should pass (exists in absolute path), the second should fail (doesn't exist anywhere). diff --git a/src/content-linter/tests/unit/frontmatter-landing-recommended.js b/src/content-linter/tests/unit/frontmatter-landing-recommended.js index 12969e8c0fea..7dc86e0c197c 100644 --- a/src/content-linter/tests/unit/frontmatter-landing-recommended.js +++ b/src/content-linter/tests/unit/frontmatter-landing-recommended.js @@ -1,4 +1,4 @@ -import { describe, expect, test } from 'vitest' +import { describe, expect, test, beforeAll, afterAll } from 'vitest' import { runRule } from '@/content-linter/lib/init-test' import { frontmatterLandingRecommended } from '@/content-linter/lib/linting-rules/frontmatter-landing-recommended' @@ -10,6 +10,12 @@ const DUPLICATE_RECOMMENDED = 'src/content-linter/tests/fixtures/landing-recommended/duplicate-recommended.md' const INVALID_PATHS = 'src/content-linter/tests/fixtures/landing-recommended/invalid-paths.md' const NO_RECOMMENDED = 'src/content-linter/tests/fixtures/landing-recommended/no-recommended.md' +const ABSOLUTE_PRIORITY = + 'src/content-linter/tests/fixtures/landing-recommended/test-absolute-priority.md' +const PATH_PRIORITY = 'src/content-linter/tests/fixtures/landing-recommended/test-path-priority.md' +const ABSOLUTE_ONLY = 'src/content-linter/tests/fixtures/landing-recommended/test-absolute-only.md' +const PRIORITY_VALIDATION = + 'src/content-linter/tests/fixtures/landing-recommended/test-priority-validation.md' const ruleName = frontmatterLandingRecommended.names[1] @@ -17,6 +23,15 @@ const ruleName = frontmatterLandingRecommended.names[1] const fmOptions = { markdownlintOptions: { frontMatter: null } } describe(ruleName, () => { + const envVarValueBefore = process.env.ROOT + + beforeAll(() => { + process.env.ROOT = 'src/fixtures/fixtures' + }) + + afterAll(() => { + process.env.ROOT = envVarValueBefore + }) test('landing page with recommended articles passes', async () => { const result = await runRule(frontmatterLandingRecommended, { files: [VALID_LANDING], @@ -75,4 +90,61 @@ describe(ruleName, () => { }) expect(result[VALID_LANDING]).toEqual([]) }) + + test('absolute paths are prioritized over relative paths', async () => { + // This test verifies that when both absolute and relative paths exist with the same name, + // the absolute path is chosen over the relative path. + // + // Setup: + // - /article-one should resolve to src/fixtures/fixtures/content/article-one.md (absolute) + // - article-one (relative) would resolve to src/content-linter/tests/fixtures/landing-recommended/article-one.md + // + // The test passes because our logic prioritizes the absolute path resolution first + const result = await runRule(frontmatterLandingRecommended, { + files: [ABSOLUTE_PRIORITY], + ...fmOptions, + }) + expect(result[ABSOLUTE_PRIORITY]).toEqual([]) + }) + + test('path priority resolution works correctly', async () => { + // This test verifies that absolute paths are prioritized over relative paths + // when both files exist with the same name. + // + // Setup: + // - /article-one could resolve to EITHER: + // 1. src/fixtures/fixtures/content/article-one.md (absolute - should be chosen) + // 2. src/content-linter/tests/fixtures/landing-recommended/article-one.md (relative - should be ignored) + // + // Our prioritization logic should choose #1 (absolute) over #2 (relative) + // This test passes because the absolute path exists and is found first + const result = await runRule(frontmatterLandingRecommended, { + files: [PATH_PRIORITY], + ...fmOptions, + }) + expect(result[PATH_PRIORITY]).toEqual([]) + }) + + test('absolute-only paths work when no relative path exists', async () => { + // This test verifies that absolute path resolution works when no relative path exists + // /article-two exists in src/fixtures/fixtures/content/article-two.md + // but NOT in src/content-linter/tests/fixtures/landing-recommended/article-two.md + // This test would fail if we didn't prioritize absolute paths properly + const result = await runRule(frontmatterLandingRecommended, { + files: [ABSOLUTE_ONLY], + ...fmOptions, + }) + expect(result[ABSOLUTE_ONLY]).toEqual([]) + }) + + test('mixed valid and invalid absolute paths are handled correctly', async () => { + // This test has both a valid absolute path (/article-one) and an invalid one (/nonexistent-absolute) + // It should fail because of the invalid path, proving our absolute path resolution is working + const result = await runRule(frontmatterLandingRecommended, { + files: [PRIORITY_VALIDATION], + ...fmOptions, + }) + expect(result[PRIORITY_VALIDATION]).toHaveLength(1) + expect(result[PRIORITY_VALIDATION][0].errorDetail).toContain('nonexistent-absolute') + }) }) diff --git a/src/fixtures/fixtures/article-one.md b/src/fixtures/fixtures/article-one.md new file mode 100644 index 000000000000..feb27b1fbd64 --- /dev/null +++ b/src/fixtures/fixtures/article-one.md @@ -0,0 +1,8 @@ +--- +title: Article One (ABSOLUTE VERSION - Should be used) +--- + +# Article One (Absolute Version) + +This is the ABSOLUTE version in the fixtures root directory. +If this file is being resolved, the prioritization is CORRECT! diff --git a/src/fixtures/fixtures/content/article-one.md b/src/fixtures/fixtures/content/article-one.md new file mode 100644 index 000000000000..bd3091463779 --- /dev/null +++ b/src/fixtures/fixtures/content/article-one.md @@ -0,0 +1,8 @@ +--- +title: Article One (ABSOLUTE VERSION - Should be used) +--- + +# Article One (Absolute Version) + +This is the ABSOLUTE version in fixtures/content directory. +If this file is being resolved, the prioritization is CORRECT! diff --git a/src/fixtures/fixtures/content/article-two.md b/src/fixtures/fixtures/content/article-two.md new file mode 100644 index 000000000000..0bb5dfaa2653 --- /dev/null +++ b/src/fixtures/fixtures/content/article-two.md @@ -0,0 +1,7 @@ +--- +title: Article Two +--- + +# Article Two + +This is the second test article. diff --git a/src/fixtures/fixtures/content/subdir/article-three.md b/src/fixtures/fixtures/content/subdir/article-three.md new file mode 100644 index 000000000000..4543250472e1 --- /dev/null +++ b/src/fixtures/fixtures/content/subdir/article-three.md @@ -0,0 +1,7 @@ +--- +title: Article Three +--- + +# Article Three + +This is the third test article in a subdirectory. diff --git a/src/fixtures/fixtures/test-discovery-landing.md b/src/fixtures/fixtures/test-discovery-landing.md new file mode 100644 index 000000000000..5eb46113b294 --- /dev/null +++ b/src/fixtures/fixtures/test-discovery-landing.md @@ -0,0 +1,11 @@ +--- +title: Test Discovery Landing Page +intro: This is a test discovery landing page +recommended: + - /get-started/quickstart + - /actions/learn-github-actions +--- + +# Test Discovery Landing Page + +This page has recommended articles that should be resolved. diff --git a/src/frame/lib/page.ts b/src/frame/lib/page.ts index 5da0d76918a6..3ce5de6edcc6 100644 --- a/src/frame/lib/page.ts +++ b/src/frame/lib/page.ts @@ -95,6 +95,8 @@ class Page { public rawIncludeGuides?: string[] public introLinks?: Record public rawIntroLinks?: Record + public recommended?: string[] + public rawRecommended?: string[] // Derived properties public languageCode!: string @@ -211,6 +213,7 @@ class Page { this.rawLearningTracks = this.learningTracks this.rawIncludeGuides = this.includeGuides as any this.rawIntroLinks = this.introLinks + this.rawRecommended = this.recommended // Is this the Homepage or a Product, Category, Topic, or Article? this.documentType = getDocumentType(this.relativePath) diff --git a/src/frame/middleware/index.ts b/src/frame/middleware/index.ts index 1b9de2e6a309..0e64c35a0db7 100644 --- a/src/frame/middleware/index.ts +++ b/src/frame/middleware/index.ts @@ -43,6 +43,7 @@ import currentProductTree from './context/current-product-tree' import genericToc from './context/generic-toc' import breadcrumbs from './context/breadcrumbs' import glossaries from './context/glossaries' +import resolveRecommended from './resolve-recommended' import renderProductName from './context/render-product-name' import features from '@/versions/middleware/features' import productExamples from './context/product-examples' @@ -267,6 +268,7 @@ export default function (app: Express) { app.use(asyncMiddleware(glossaries)) app.use(asyncMiddleware(generalSearchMiddleware)) app.use(asyncMiddleware(featuredLinks)) + app.use(asyncMiddleware(resolveRecommended)) app.use(asyncMiddleware(learningTrack)) if (ENABLE_FASTLY_TESTING) { diff --git a/src/frame/middleware/resolve-recommended.ts b/src/frame/middleware/resolve-recommended.ts new file mode 100644 index 000000000000..842883bf6751 --- /dev/null +++ b/src/frame/middleware/resolve-recommended.ts @@ -0,0 +1,165 @@ +import type { ExtendedRequest, ResolvedArticle } from '@/types' +import type { Response, NextFunction } from 'express' +import findPage from '@/frame/lib/find-page' +import { renderContent } from '@/content-render/index' + +import { createLogger } from '@/observability/logger/index' + +const logger = createLogger('middleware:resolve-recommended') + +/** + * Build an article path by combining language, optional base path, and article path + */ +function buildArticlePath(currentLanguage: string, articlePath: string, basePath?: string): string { + const pathPrefix = basePath ? `/${currentLanguage}/${basePath}` : `/${currentLanguage}` + const separator = articlePath.startsWith('/') ? '' : '/' + return `${pathPrefix}${separator}${articlePath}` +} + +/** + * Try to resolve an article path using multiple resolution strategies + */ +function tryResolveArticlePath( + rawPath: string, + pageRelativePath: string | undefined, + req: ExtendedRequest, +): any { + const { pages, redirects } = req.context! + const currentLanguage = req.context!.currentLanguage || 'en' + + // Check if we have the required dependencies + if (!pages || !redirects) { + return undefined + } + + // Strategy 1: Try content-relative path (add language prefix to raw path) + const contentRelativePath = buildArticlePath(currentLanguage, rawPath) + let foundPage = findPage(contentRelativePath, pages, redirects) + + if (foundPage) { + return foundPage + } + + // Strategy 2: Try page-relative path if page context is available + if (pageRelativePath) { + const pageDirPath = pageRelativePath.split('/').slice(0, -1).join('/') + const pageRelativeFullPath = buildArticlePath(currentLanguage, rawPath, pageDirPath) + foundPage = findPage(pageRelativeFullPath, pages, redirects) + + if (foundPage) { + return foundPage + } + } + + // Strategy 3: Try with .md extension if not already present + if (!rawPath.endsWith('.md')) { + const pathWithExtension = `${rawPath}.md` + + // Try Strategy 1 with .md extension + const contentRelativePathWithExt = buildArticlePath(currentLanguage, pathWithExtension) + foundPage = findPage(contentRelativePathWithExt, pages, redirects) + + if (foundPage) { + return foundPage + } + + // Try Strategy 2 with .md extension + if (pageRelativePath) { + const pageDirPath = pageRelativePath.split('/').slice(0, -1).join('/') + const pageRelativeFullPathWithExt = buildArticlePath( + currentLanguage, + pathWithExtension, + pageDirPath, + ) + foundPage = findPage(pageRelativeFullPathWithExt, pages, redirects) + + if (foundPage) { + return foundPage + } + } + } + + return foundPage +} + +/** + * Get the href for a page from its permalinks + */ +function getPageHref(page: any, currentLanguage: string = 'en'): string { + if (page.permalinks?.length > 0) { + const permalink = page.permalinks.find((p: any) => p.languageCode === currentLanguage) + return permalink ? permalink.href : page.permalinks[0].href + } + return '' // fallback +} + +/** + * Middleware to resolve recommended articles from rawRecommended paths and legacy spotlight field + */ +async function resolveRecommended( + req: ExtendedRequest, + res: Response, + next: NextFunction, +): Promise { + try { + const page = req.context?.page + const rawRecommended = (page as any)?.rawRecommended + const spotlight = (page as any)?.spotlight + + // Collect article paths from both rawRecommended and spotlight + const articlePaths: string[] = [] + + // Add paths from rawRecommended + if (rawRecommended && Array.isArray(rawRecommended)) { + articlePaths.push(...rawRecommended) + } + + // Add paths from spotlight (legacy field) + if (spotlight && Array.isArray(spotlight)) { + const spotlightPaths = spotlight + .filter((item: any) => item && typeof item.article === 'string') + .map((item: any) => item.article) + articlePaths.push(...spotlightPaths) + } + + if (articlePaths.length === 0) { + return next() + } + + const currentLanguage = req.context?.currentLanguage || 'en' + const resolved: ResolvedArticle[] = [] + + for (const rawPath of articlePaths) { + try { + const foundPage = tryResolveArticlePath(rawPath, page?.relativePath, req) + + if (foundPage) { + const href = getPageHref(foundPage, currentLanguage) + const category = foundPage.relativePath + ? foundPage.relativePath.split('/').slice(0, -1).filter(Boolean) + : [] + + resolved.push({ + title: foundPage.title, + intro: await renderContent(foundPage.intro, req.context), + href, + category, + }) + } + } catch (error) { + logger.warn(`Failed to resolve recommended article: ${rawPath}`, { error }) + } + } + + // Replace the rawRecommended with resolved articles + if (page) { + ;(page as any).recommended = resolved + } + } catch (error) { + logger.error('Error in resolveRecommended middleware:', { error }) + } + + next() +} + +export default resolveRecommended diff --git a/src/frame/tests/content.ts b/src/frame/tests/content.ts index 624d3a81c642..0e145e55e82e 100644 --- a/src/frame/tests/content.ts +++ b/src/frame/tests/content.ts @@ -33,10 +33,22 @@ describe('content files', () => { return file.endsWith('.md') && !file.includes('README') }, ) as string[] - const orphanedFiles = contentFiles.filter((file: string) => !relativeFiles.includes(file)) + + const orphanedFiles = contentFiles.filter((file) => !relativeFiles.includes(file)) + + // Filter out intentional test fixture files that are meant to be orphaned + const allowedOrphanedFiles = [ + path.join(contentDir, 'article-one.md'), + path.join(contentDir, 'article-two.md'), + path.join(contentDir, 'subdir', 'article-three.md'), + ] + const unexpectedOrphanedFiles = orphanedFiles.filter( + (file) => !allowedOrphanedFiles.includes(file), + ) + expect( - orphanedFiles.length, - `${orphanedFiles} orphaned files found on disk but not in site tree`, + unexpectedOrphanedFiles.length, + `${unexpectedOrphanedFiles} orphaned files found on disk but not in site tree`, ).toBe(0) }, ) diff --git a/src/frame/tests/resolve-recommended.test.ts b/src/frame/tests/resolve-recommended.test.ts new file mode 100644 index 000000000000..2ed9f26ab12f --- /dev/null +++ b/src/frame/tests/resolve-recommended.test.ts @@ -0,0 +1,265 @@ +import { describe, test, expect, vi, beforeEach } from 'vitest' +import type { Response, NextFunction } from 'express' +import type { ExtendedRequest } from '@/types' +import findPage from '@/frame/lib/find-page' +import resolveRecommended from '../middleware/resolve-recommended' + +// Mock the findPage function +vi.mock('@/frame/lib/find-page', () => ({ + default: vi.fn(), +})) + +// Mock the renderContent function +vi.mock('@/content-render/index', () => ({ + renderContent: vi.fn((content) => `

${content}

`), +})) + +describe('resolveRecommended middleware', () => { + const mockFindPage = vi.mocked(findPage) + + const createMockRequest = (pageData: any = {}, contextData: any = {}): ExtendedRequest => + ({ + context: { + page: pageData, + pages: { + '/test-article': { + title: 'Test Article', + intro: 'Test intro', + relativePath: 'test/article.md', + }, + }, + redirects: {}, + ...contextData, + }, + }) as ExtendedRequest + + const mockRes = {} as Response + const mockNext = vi.fn() as NextFunction + + beforeEach(() => { + vi.clearAllMocks() + }) + + test('should call next when no context', async () => { + const req = {} as ExtendedRequest + + await resolveRecommended(req, mockRes, mockNext) + + expect(mockNext).toHaveBeenCalled() + expect(mockFindPage).not.toHaveBeenCalled() + }) + + test('should call next when no page', async () => { + const req = { context: {} } as ExtendedRequest + + await resolveRecommended(req, mockRes, mockNext) + + expect(mockNext).toHaveBeenCalled() + expect(mockFindPage).not.toHaveBeenCalled() + }) + + test('should call next when no pages collection', async () => { + const req = createMockRequest({ rawRecommended: ['/test-article'] }, { pages: undefined }) + + await resolveRecommended(req, mockRes, mockNext) + + expect(mockNext).toHaveBeenCalled() + expect(mockFindPage).not.toHaveBeenCalled() + }) + + test('should call next when no rawRecommended', async () => { + const req = createMockRequest() + + await resolveRecommended(req, mockRes, mockNext) + + expect(mockNext).toHaveBeenCalled() + expect(mockFindPage).not.toHaveBeenCalled() + }) + + test('should resolve recommended articles when they exist', async () => { + const testPage: Partial = { + mtime: Date.now(), + title: 'Test Article', + rawTitle: 'Test Article', + intro: 'Test intro', + rawIntro: 'Test intro', + relativePath: 'copilot/tutorials/article.md', + fullPath: '/full/path/copilot/tutorials/article.md', + languageCode: 'en', + documentType: 'article', + markdown: 'Test content', + versions: {}, + applicableVersions: ['free-pro-team@latest'], + permalinks: [ + { + languageCode: 'en', + pageVersion: 'free-pro-team@latest', + title: 'Test Article', + href: '/en/copilot/tutorials/article', + hrefWithoutLanguage: '/copilot/tutorials/article', + }, + ], + renderProp: vi.fn().mockResolvedValue('rendered'), + renderTitle: vi.fn().mockResolvedValue('Test Article'), + render: vi.fn().mockResolvedValue('rendered content'), + buildRedirects: vi.fn().mockReturnValue({}), + } + + mockFindPage.mockReturnValue(testPage as any) + + const req = createMockRequest({ rawRecommended: ['/copilot/tutorials/article'] }) + + await resolveRecommended(req, mockRes, mockNext) + + expect(mockFindPage).toHaveBeenCalledWith( + '/en/copilot/tutorials/article', + req.context!.pages, + req.context!.redirects, + ) + expect((req.context!.page as any).recommended).toEqual([ + { + title: 'Test Article', + intro: '

Test intro

', + href: '/en/copilot/tutorials/article', + category: ['copilot', 'tutorials'], + }, + ]) + expect(mockNext).toHaveBeenCalled() + }) + + test('should handle articles not found', async () => { + mockFindPage.mockReturnValue(undefined) + + const req = createMockRequest({ rawRecommended: ['/nonexistent-article'] }) + + await resolveRecommended(req, mockRes, mockNext) + + expect(mockFindPage).toHaveBeenCalledWith( + '/en/nonexistent-article', + req.context!.pages, + req.context!.redirects, + ) + expect((req.context!.page as any).recommended).toEqual([]) + expect(mockNext).toHaveBeenCalled() + }) + + test('should handle errors gracefully', async () => { + mockFindPage.mockImplementation(() => { + throw new Error('Test error') + }) + + const req = createMockRequest({ rawRecommended: ['/error-article'] }) + + await resolveRecommended(req, mockRes, mockNext) + + expect((req.context!.page as any).recommended).toEqual([]) + expect(mockNext).toHaveBeenCalled() + }) + + test('should handle mixed valid and invalid articles', async () => { + const testPage: Partial = { + mtime: Date.now(), + title: 'Valid Article', + rawTitle: 'Valid Article', + intro: 'Valid intro', + rawIntro: 'Valid intro', + relativePath: 'test/valid.md', + fullPath: '/full/path/test/valid.md', + languageCode: 'en', + documentType: 'article', + markdown: 'Valid content', + versions: {}, + applicableVersions: ['free-pro-team@latest'], + permalinks: [ + { + languageCode: 'en', + pageVersion: 'free-pro-team@latest', + title: 'Valid Article', + href: '/en/test/valid', + hrefWithoutLanguage: '/test/valid', + }, + ], + renderProp: vi.fn().mockResolvedValue('rendered'), + renderTitle: vi.fn().mockResolvedValue('Valid Article'), + render: vi.fn().mockResolvedValue('rendered content'), + buildRedirects: vi.fn().mockReturnValue({}), + } + + mockFindPage.mockReturnValueOnce(testPage as any).mockReturnValueOnce(undefined) + + const req = createMockRequest({ rawRecommended: ['/valid-article', '/invalid-article'] }) + + await resolveRecommended(req, mockRes, mockNext) + + expect((req.context!.page as any).recommended).toEqual([ + { + title: 'Valid Article', + intro: '

Valid intro

', + href: '/en/test/valid', + category: ['test'], + }, + ]) + expect(mockNext).toHaveBeenCalled() + }) + + test('should try page-relative path when content-relative fails', async () => { + const testPage: Partial = { + mtime: Date.now(), + title: 'Relative Article', + rawTitle: 'Relative Article', + intro: 'Relative intro', + rawIntro: 'Relative intro', + relativePath: 'copilot/relative-article.md', + fullPath: '/full/path/copilot/relative-article.md', + languageCode: 'en', + documentType: 'article', + markdown: 'Relative content', + versions: {}, + applicableVersions: ['free-pro-team@latest'], + permalinks: [ + { + languageCode: 'en', + pageVersion: 'free-pro-team@latest', + title: 'Relative Article', + href: '/en/copilot/relative-article', + hrefWithoutLanguage: '/copilot/relative-article', + }, + ], + renderProp: vi.fn().mockResolvedValue('rendered'), + renderTitle: vi.fn().mockResolvedValue('Relative Article'), + render: vi.fn().mockResolvedValue('rendered content'), + buildRedirects: vi.fn().mockReturnValue({}), + } + + // Mock findPage to fail on first call (content-relative) and succeed on second (page-relative) + mockFindPage.mockReturnValueOnce(undefined).mockReturnValueOnce(testPage as any) + + const req = createMockRequest({ + rawRecommended: ['relative-article'], + relativePath: 'copilot/index.md', + }) + + await resolveRecommended(req, mockRes, mockNext) + + expect(mockFindPage).toHaveBeenCalledTimes(2) + expect(mockFindPage).toHaveBeenCalledWith( + '/en/relative-article', + req.context!.pages, + req.context!.redirects, + ) + expect(mockFindPage).toHaveBeenCalledWith( + '/en/copilot/relative-article', + req.context!.pages, + req.context!.redirects, + ) + expect((req.context!.page as any).recommended).toEqual([ + { + title: 'Relative Article', + intro: '

Relative intro

', + href: '/en/copilot/relative-article', + category: ['copilot'], + }, + ]) + expect(mockNext).toHaveBeenCalled() + }) +}) diff --git a/src/landings/components/discovery/DiscoveryLanding.tsx b/src/landings/components/discovery/DiscoveryLanding.tsx index f1a13c14cd11..657d8e9f3d42 100644 --- a/src/landings/components/discovery/DiscoveryLanding.tsx +++ b/src/landings/components/discovery/DiscoveryLanding.tsx @@ -21,7 +21,7 @@ export const DiscoveryLanding = () => {
- +
diff --git a/src/landings/components/shared/LandingCarousel.tsx b/src/landings/components/shared/LandingCarousel.tsx index 0a651c77bc1e..aa085d5f0cf2 100644 --- a/src/landings/components/shared/LandingCarousel.tsx +++ b/src/landings/components/shared/LandingCarousel.tsx @@ -2,22 +2,13 @@ import { useState, useEffect, useRef } from 'react' import { ChevronLeftIcon, ChevronRightIcon } from '@primer/octicons-react' import { Token } from '@primer/react' import cx from 'classnames' -import type { TocItem } from '@/landings/types' +import type { ResolvedArticle } from '@/types' import { useTranslation } from '@/languages/components/useTranslation' import styles from './LandingCarousel.module.scss' -type ProcessedArticleItem = { - article: string - title: string - description: string - url: string - category: string[] -} - type LandingCarouselProps = { heading?: string - recommended?: string[] // Array of article paths - flatArticles: TocItem[] + recommended?: ResolvedArticle[] } // Hook to get current items per view based on screen size @@ -47,11 +38,7 @@ const useResponsiveItemsPerView = () => { return itemsPerView } -export const LandingCarousel = ({ - heading = '', - flatArticles, - recommended, -}: LandingCarouselProps) => { +export const LandingCarousel = ({ heading = '', recommended }: LandingCarouselProps) => { const [currentPage, setCurrentPage] = useState(0) const [isAnimating, setIsAnimating] = useState(false) const itemsPerView = useResponsiveItemsPerView() @@ -65,6 +52,8 @@ export const LandingCarousel = ({ setCurrentPage(0) }, [itemsPerView]) + const processedItems: ResolvedArticle[] = recommended || [] + // Cleanup timeout on unmount useEffect(() => { return () => { @@ -74,34 +63,6 @@ export const LandingCarousel = ({ } }, []) - // Helper function to find article data from tocItems - const findArticleData = (articlePath: string) => { - if (typeof articlePath !== 'string') { - console.warn('Invalid articlePath:', articlePath) - return null - } - const cleanPath = articlePath.startsWith('/') ? articlePath.slice(1) : articlePath - return flatArticles.find( - (item) => - item.fullPath?.endsWith(cleanPath) || - item.fullPath?.includes(cleanPath.split('/').pop() || ''), - ) - } - - // Process recommended articles to get article data - const processedItems = (recommended || []) - .filter((item) => typeof item === 'string' && item.length > 0) - .map((recommendedArticlePath) => { - const articleData = findArticleData(recommendedArticlePath) - return { - article: recommendedArticlePath, - title: articleData?.title || 'Unknown Article', - description: articleData?.intro || '', - url: articleData?.fullPath || recommendedArticlePath, - category: articleData?.category || [], - } - }) - const totalItems = processedItems.length const totalPages = Math.ceil(totalItems / itemsPerView) @@ -182,22 +143,27 @@ export const LandingCarousel = ({ className={cx(styles.itemsGrid, { [styles.animating]: isAnimating })} data-testid="carousel-items" > - {visibleItems.map((article: ProcessedArticleItem, index) => ( + {visibleItems.map((article: ResolvedArticle, index) => (
- {article.category.map((cat) => ( + {article.category.map((cat: string) => ( ))}

- + {article.title}

-

{article.description}

+
))}
diff --git a/src/landings/context/LandingContext.tsx b/src/landings/context/LandingContext.tsx index 0739c142295e..6e4030555c55 100644 --- a/src/landings/context/LandingContext.tsx +++ b/src/landings/context/LandingContext.tsx @@ -1,8 +1,9 @@ import { createContext, useContext } from 'react' -import { FeaturedLink, getFeaturedLinksFromReq } from '@/landings/components/ProductLandingContext' +import { getFeaturedLinksFromReq } from '@/landings/components/ProductLandingContext' import { mapRawTocItemToTocItem } from '@/landings/types' import type { TocItem } from '@/landings/types' import type { LearningTrack } from '@/types' +import type { FeaturedLink } from '@/landings/components/ProductLandingContext' export type LandingType = 'bespoke' | 'discovery' | 'journey' @@ -20,8 +21,7 @@ export type LandingContextT = { currentLayout: string heroImage?: string // For discovery landing pages - recommended?: string[] // Array of article paths - // For discovery landing pages + recommended?: Array<{ title: string; intro: string; href: string; category: string[] }> // Resolved article data introLinks?: Record } @@ -37,21 +37,21 @@ export const useLandingContext = (): LandingContextT => { return context } +/** + * Server-side function to create LandingContext data from a request. + */ export const getLandingContextFromRequest = async ( req: any, landingType: LandingType, ): Promise => { const page = req.context.page - let recommended: string[] = [] + let recommended: Array<{ title: string; intro: string; href: string; category: string[] }> = [] + if (landingType === 'discovery') { - // Support legacy `spotlight` property as `recommended` for pages like Copilot Cookbook - // However, `spotlight` will have lower priority than the `recommended` property - if (page.recommended && page.recommended.length > 0) { + // Use resolved recommended articles from the page after middleware processing + if (page.recommended && Array.isArray(page.recommended) && page.recommended.length > 0) { recommended = page.recommended - } else if (page.spotlight && page.spotlight.length > 0) { - // Remove the `image` property from spotlight items, since we don't use those for the carousel - recommended = page.spotlight.map((item: any) => item.article) } } diff --git a/src/types.ts b/src/types.ts index d5108fc5aad3..53d39f91b62a 100644 --- a/src/types.ts +++ b/src/types.ts @@ -4,6 +4,14 @@ import type { Failbot } from '@github/failbot' import type enterpriseServerReleases from '@/versions/lib/enterprise-server-releases.d' import type { ValidOcticon } from '@/landings/types' +// Shared type for resolved article information used across landing pages and carousels +export interface ResolvedArticle { + title: string + intro: string + href: string + category: string[] +} + // Throughout our codebase we "extend" the Request object by attaching // things to it. For example `req.context = { currentCategory: 'foo' }`. // This type aims to match all the custom things we do to requests