Fix PR preview and Copilot visual review skipping docs home page when layouts/index.html changes (#6922)
* Initial plan * fix: include docs home page in PR preview and Copilot visual review when layouts/index.html changes Co-authored-by: jstirnaman <212227+jstirnaman@users.noreply.github.com> * fix: improve detect-preview-pages to avoid false needs-author-input when home page auto-detected Co-authored-by: jstirnaman <212227+jstirnaman@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: jstirnaman <212227+jstirnaman@users.noreply.github.com> Co-authored-by: Jason Stirnaman <jstirnaman@influxdata.com>pull/6935/head
Copilot
GitHub
parent
c600dda272
commit
0a0bebc42c
|
|
@ -35,10 +35,10 @@ if (!/^origin\/[a-zA-Z0-9._\/-]+$/.test(BASE_REF)) {
|
|||
*/
|
||||
function getAllChangedFiles() {
|
||||
try {
|
||||
const output = execSync(
|
||||
`git diff --name-only ${BASE_REF}...HEAD`,
|
||||
{ encoding: 'utf-8', stdio: ['pipe', 'pipe', 'pipe'] }
|
||||
);
|
||||
const output = execSync(`git diff --name-only ${BASE_REF}...HEAD`, {
|
||||
encoding: 'utf-8',
|
||||
stdio: ['pipe', 'pipe', 'pipe'],
|
||||
});
|
||||
return output.trim().split('\n').filter(Boolean);
|
||||
} catch (err) {
|
||||
console.error(`Error detecting changes: ${err.message}`);
|
||||
|
|
@ -53,11 +53,13 @@ function getAllChangedFiles() {
|
|||
*/
|
||||
function categorizeChanges(files) {
|
||||
return {
|
||||
content: files.filter(f => f.startsWith('content/') && f.endsWith('.md')),
|
||||
layouts: files.filter(f => f.startsWith('layouts/')),
|
||||
assets: files.filter(f => f.startsWith('assets/')),
|
||||
data: files.filter(f => f.startsWith('data/')),
|
||||
apiDocs: files.filter(f => f.startsWith('api-docs/') || f.startsWith('openapi/')),
|
||||
content: files.filter((f) => f.startsWith('content/') && f.endsWith('.md')),
|
||||
layouts: files.filter((f) => f.startsWith('layouts/')),
|
||||
assets: files.filter((f) => f.startsWith('assets/')),
|
||||
data: files.filter((f) => f.startsWith('data/')),
|
||||
apiDocs: files.filter(
|
||||
(f) => f.startsWith('api-docs/') || f.startsWith('openapi/')
|
||||
),
|
||||
};
|
||||
}
|
||||
|
||||
|
|
@ -127,7 +129,7 @@ function main() {
|
|||
const htmlPaths = mapContentToPublic(expandedContent, 'public');
|
||||
|
||||
// Convert HTML paths to URL paths
|
||||
pagesToDeploy = Array.from(htmlPaths).map(htmlPath => {
|
||||
pagesToDeploy = Array.from(htmlPaths).map((htmlPath) => {
|
||||
return '/' + htmlPath.replace('public/', '').replace('/index.html', '/');
|
||||
});
|
||||
console.log(` Found ${pagesToDeploy.length} affected pages\n`);
|
||||
|
|
@ -135,34 +137,53 @@ function main() {
|
|||
|
||||
// Strategy 2: Layout/asset changes - parse URLs from PR body
|
||||
if (hasLayoutChanges) {
|
||||
console.log('🎨 Layout/asset changes detected, checking PR description for URLs...');
|
||||
console.log(
|
||||
'🎨 Layout/asset changes detected, checking PR description for URLs...'
|
||||
);
|
||||
|
||||
// Auto-detect home page when the root template changes
|
||||
if (changes.layouts.includes('layouts/index.html')) {
|
||||
pagesToDeploy = [...new Set([...pagesToDeploy, '/'])];
|
||||
console.log(
|
||||
' 🏠 Home page template (layouts/index.html) changed - auto-adding / to preview pages'
|
||||
);
|
||||
}
|
||||
|
||||
const prUrls = extractDocsUrls(PR_BODY);
|
||||
|
||||
if (prUrls.length > 0) {
|
||||
console.log(` Found ${prUrls.length} URLs in PR description`);
|
||||
// Merge with content pages (deduplicate)
|
||||
pagesToDeploy = [...new Set([...pagesToDeploy, ...prUrls])];
|
||||
} else if (changes.content.length === 0) {
|
||||
// No content changes AND no URLs specified - need author input
|
||||
console.log(' ⚠️ No URLs found in PR description - author input needed');
|
||||
} else if (pagesToDeploy.length === 0) {
|
||||
// No content changes, no auto-detected pages, and no URLs specified - need author input
|
||||
console.log(
|
||||
' ⚠️ No URLs found in PR description - author input needed'
|
||||
);
|
||||
setOutput('pages-to-deploy', '[]');
|
||||
setOutput('has-layout-changes', 'true');
|
||||
setOutput('needs-author-input', 'true');
|
||||
setOutput('change-summary', 'Layout/asset changes detected - please specify pages to preview');
|
||||
setOutput(
|
||||
'change-summary',
|
||||
'Layout/asset changes detected - please specify pages to preview'
|
||||
);
|
||||
return;
|
||||
}
|
||||
}
|
||||
|
||||
// Apply page limit
|
||||
if (pagesToDeploy.length > MAX_PAGES) {
|
||||
console.log(`⚠️ Limiting preview to ${MAX_PAGES} pages (found ${pagesToDeploy.length})`);
|
||||
console.log(
|
||||
`⚠️ Limiting preview to ${MAX_PAGES} pages (found ${pagesToDeploy.length})`
|
||||
);
|
||||
pagesToDeploy = pagesToDeploy.slice(0, MAX_PAGES);
|
||||
}
|
||||
|
||||
// Generate summary
|
||||
const summary = pagesToDeploy.length > 0
|
||||
? `${pagesToDeploy.length} page(s) will be previewed`
|
||||
: 'No pages to preview';
|
||||
const summary =
|
||||
pagesToDeploy.length > 0
|
||||
? `${pagesToDeploy.length} page(s) will be previewed`
|
||||
: 'No pages to preview';
|
||||
|
||||
console.log(`\n✅ ${summary}`);
|
||||
|
||||
|
|
|
|||
|
|
@ -63,6 +63,9 @@ function isValidUrlPath(path) {
|
|||
// Must start with /
|
||||
if (!path.startsWith('/')) return false;
|
||||
|
||||
// Allow root path (docs home page at /)
|
||||
if (path === '/') return true;
|
||||
|
||||
// Must start with known product prefix (loaded from products.yml)
|
||||
const validPrefixes = PRODUCT_NAMESPACES.map((ns) => `/${ns}/`);
|
||||
|
||||
|
|
@ -101,7 +104,8 @@ export function extractDocsUrls(text) {
|
|||
|
||||
// Pattern 1: Full production URLs
|
||||
// https://docs.influxdata.com/influxdb3/core/get-started/
|
||||
const prodUrlPattern = /https?:\/\/docs\.influxdata\.com(\/[^\s)\]>"']+)/g;
|
||||
// https://docs.influxdata.com/ (home page)
|
||||
const prodUrlPattern = /https?:\/\/docs\.influxdata\.com(\/[^\s)\]>"']*)/g;
|
||||
let match;
|
||||
while ((match = prodUrlPattern.exec(text)) !== null) {
|
||||
const path = normalizeUrlPath(match[1]);
|
||||
|
|
@ -112,7 +116,8 @@ export function extractDocsUrls(text) {
|
|||
|
||||
// Pattern 2: Localhost dev URLs
|
||||
// http://localhost:1313/influxdb3/core/
|
||||
const localUrlPattern = /https?:\/\/localhost:\d+(\/[^\s)\]>"']+)/g;
|
||||
// http://localhost:1313/ (home page)
|
||||
const localUrlPattern = /https?:\/\/localhost:\d+(\/[^\s)\]>"']*)/g;
|
||||
while ((match = localUrlPattern.exec(text)) !== null) {
|
||||
const path = normalizeUrlPath(match[1]);
|
||||
if (isValidUrlPath(path)) {
|
||||
|
|
|
|||
|
|
@ -10,6 +10,7 @@
|
|||
*/
|
||||
|
||||
import { appendFileSync } from 'fs';
|
||||
import { execSync } from 'child_process';
|
||||
import {
|
||||
getChangedContentFiles,
|
||||
mapContentToPublic,
|
||||
|
|
@ -27,11 +28,33 @@ if (!/^origin\/[a-zA-Z0-9._/-]+$/.test(BASE_REF)) {
|
|||
const changed = getChangedContentFiles(BASE_REF);
|
||||
const htmlPaths = mapContentToPublic(changed, 'public');
|
||||
|
||||
const urls = Array.from(htmlPaths)
|
||||
const contentUrls = Array.from(htmlPaths)
|
||||
.sort()
|
||||
.map((p) => '/' + p.replace(/^public\//, '').replace(/\/index\.html$/, '/'))
|
||||
.slice(0, MAX_PAGES);
|
||||
|
||||
// Check if the home page template changed (layouts/index.html → /)
|
||||
let homePageUrls = [];
|
||||
try {
|
||||
const homePageChanged = execSync(
|
||||
`git diff --name-only ${BASE_REF}...HEAD -- layouts/index.html`,
|
||||
{ encoding: 'utf-8', stdio: ['pipe', 'pipe', 'pipe'] }
|
||||
).trim();
|
||||
if (homePageChanged) {
|
||||
homePageUrls = ['/'];
|
||||
console.log(
|
||||
'Home page template (layouts/index.html) changed - adding / to review URLs'
|
||||
);
|
||||
}
|
||||
} catch {
|
||||
// Ignore errors - fall back to content-only URLs
|
||||
}
|
||||
|
||||
const urls = [...new Set([...homePageUrls, ...contentUrls])].slice(
|
||||
0,
|
||||
MAX_PAGES
|
||||
);
|
||||
|
||||
appendFileSync(GITHUB_OUTPUT, `urls=${JSON.stringify(urls)}\n`);
|
||||
appendFileSync(GITHUB_OUTPUT, `url-count=${urls.length}\n`);
|
||||
|
||||
|
|
|
|||
|
|
@ -145,7 +145,11 @@ test('Special characters: backticks are delimiters', () => {
|
|||
// This prevents command substitution injection
|
||||
const text = '/influxdb3/`whoami`/';
|
||||
const result = extractDocsUrls(text);
|
||||
assertEquals(result, ['/influxdb3/'], 'Should truncate at backtick delimiter');
|
||||
assertEquals(
|
||||
result,
|
||||
['/influxdb3/'],
|
||||
'Should truncate at backtick delimiter'
|
||||
);
|
||||
});
|
||||
|
||||
test('Special characters: single quotes truncate at extraction', () => {
|
||||
|
|
@ -257,31 +261,51 @@ test('Normalization: removes query string', () => {
|
|||
test('Normalization: strips wildcard from path', () => {
|
||||
const text = '/influxdb3/enterprise/*';
|
||||
const result = extractDocsUrls(text);
|
||||
assertEquals(result, ['/influxdb3/enterprise/'], 'Should strip wildcard character');
|
||||
assertEquals(
|
||||
result,
|
||||
['/influxdb3/enterprise/'],
|
||||
'Should strip wildcard character'
|
||||
);
|
||||
});
|
||||
|
||||
test('Normalization: strips wildcard in middle of path', () => {
|
||||
const text = '/influxdb3/*/admin/';
|
||||
const result = extractDocsUrls(text);
|
||||
assertEquals(result, ['/influxdb3/admin/'], 'Should strip wildcard from middle of path');
|
||||
assertEquals(
|
||||
result,
|
||||
['/influxdb3/admin/'],
|
||||
'Should strip wildcard from middle of path'
|
||||
);
|
||||
});
|
||||
|
||||
test('Normalization: strips multiple wildcards', () => {
|
||||
const text = '/influxdb3/*/admin/*';
|
||||
const result = extractDocsUrls(text);
|
||||
assertEquals(result, ['/influxdb3/admin/'], 'Should strip all wildcard characters');
|
||||
assertEquals(
|
||||
result,
|
||||
['/influxdb3/admin/'],
|
||||
'Should strip all wildcard characters'
|
||||
);
|
||||
});
|
||||
|
||||
test('Wildcard in markdown-style notation', () => {
|
||||
const text = '**InfluxDB 3 Enterprise pages** (`/influxdb3/enterprise/*`)';
|
||||
const result = extractDocsUrls(text);
|
||||
assertEquals(result, ['/influxdb3/enterprise/'], 'Should extract and normalize path with wildcard in backticks');
|
||||
assertEquals(
|
||||
result,
|
||||
['/influxdb3/enterprise/'],
|
||||
'Should extract and normalize path with wildcard in backticks'
|
||||
);
|
||||
});
|
||||
|
||||
test('Wildcard in parentheses', () => {
|
||||
const text = 'Affects pages under (/influxdb3/enterprise/*)';
|
||||
const result = extractDocsUrls(text);
|
||||
assertEquals(result, ['/influxdb3/enterprise/'], 'Should extract and normalize path with wildcard in parentheses');
|
||||
assertEquals(
|
||||
result,
|
||||
['/influxdb3/enterprise/'],
|
||||
'Should extract and normalize path with wildcard in parentheses'
|
||||
);
|
||||
});
|
||||
|
||||
// Test deduplication
|
||||
|
|
@ -360,6 +384,31 @@ test('BASE_REF: rejects without origin/ prefix', () => {
|
|||
assertEquals(isValid, false, 'Should require origin/ prefix');
|
||||
});
|
||||
|
||||
// Home page URL support
|
||||
test('Home page: production URL https://docs.influxdata.com/', () => {
|
||||
const text = 'Preview: https://docs.influxdata.com/';
|
||||
const result = extractDocsUrls(text);
|
||||
assertEquals(result, ['/'], 'Should extract root path for docs home page');
|
||||
});
|
||||
|
||||
test('Home page: localhost URL http://localhost:1313/', () => {
|
||||
const text = 'Testing at http://localhost:1313/';
|
||||
const result = extractDocsUrls(text);
|
||||
assertEquals(result, ['/'], 'Should extract root path from localhost URL');
|
||||
});
|
||||
|
||||
test('Home page: relative root path / in text', () => {
|
||||
// Relative '/' alone is not extractable by the relative pattern (requires product prefix),
|
||||
// but full URLs with / path are supported
|
||||
const text = 'https://docs.influxdata.com/ and /influxdb3/core/';
|
||||
const result = extractDocsUrls(text);
|
||||
assertEquals(
|
||||
result.sort(),
|
||||
['/', '/influxdb3/core/'].sort(),
|
||||
'Should extract both root path and product path'
|
||||
);
|
||||
});
|
||||
|
||||
// Print summary
|
||||
console.log('\n=== Test Summary ===');
|
||||
console.log(`Total: ${totalTests}`);
|
||||
|
|
|
|||
Loading…
Reference in New Issue