From 4f13d59a7994488f82d802c11004f5ad9b5d20ee Mon Sep 17 00:00:00 2001 From: Frankie Roberto <30665+frankieroberto@users.noreply.github.com> Date: Mon, 22 Jun 2026 10:42:30 +0100 Subject: [PATCH 1/7] Add markdown check for absolute URLs Links to other posts on the site are to be encouraged! But ideally they'd be relative links instead of full URLs. This means they'll work when previewing the site locally or in the review apps. It will also be helpful in the (hopefully unlikely) case that the site moves to a different domain name in future. Unlikely the existing check, for this one we can leave a direct suggestion on the pull request that authors can accept with a single click. --- .github/scripts/check-markdown.mjs | 24 ++++++++++++++++++++++++ .github/scripts/pr-review.mjs | 26 +++++++++++++++++++------- 2 files changed, 43 insertions(+), 7 deletions(-) diff --git a/.github/scripts/check-markdown.mjs b/.github/scripts/check-markdown.mjs index a21c09d1b..b7d35d99f 100644 --- a/.github/scripts/check-markdown.mjs +++ b/.github/scripts/check-markdown.mjs @@ -4,6 +4,10 @@ * Current checks: * - H1 headings (# ...): the H1 is already generated from the `title` field * in the frontmatter, so adding a `#` heading manually creates a duplicate. + * - Absolute URLs to the published site: links beginning with + * https://design-history.prevention-services.nhs.uk/ should use + * relative URLs instead, so that they work in previews and in case the URL + * changes in future. * * When run directly, scans all markdown files under app/: * npm run check:markdown @@ -24,6 +28,13 @@ const H1_MESSAGE = 'If this heading duplicates the title, remove it. ' + 'If it is a different heading, change it to an H2 using `##`.' +const SITE_URL = 'https://design-history.prevention-services.nhs.uk/' + +const ABSOLUTE_URL_MESSAGE = + 'Use a relative URL instead of a full URL for links to other posts on the site.\n\n' + + 'This means that the links will work in previews, and in case the site domain name changes in future.\n\n' + + 'For example, replace `https://design-history.prevention-services.nhs.uk/some/path/` with `/some/path/`.' + /** * Recursively finds all .md files under the given directory. * @@ -56,6 +67,9 @@ export function scanAllFiles() { if (/^# /.test(lines[i])) { mistakes.push({ path: filePath, line: i + 1, message: H1_MESSAGE }) } + if (lines[i].includes(SITE_URL)) { + mistakes.push({ path: filePath, line: i + 1, message: ABSOLUTE_URL_MESSAGE }) + } } } @@ -106,6 +120,16 @@ export function getMistakes(baseRef) { message: H1_MESSAGE }) } + // Added line containing an absolute URL to the published site + const lineContent = rawLine.slice(1) + if (lineContent.includes(SITE_URL)) { + mistakes.push({ + path: currentFile, + line: lineNumber, + message: ABSOLUTE_URL_MESSAGE, + suggestion: lineContent.replaceAll(SITE_URL, '/') + }) + } } else if (!rawLine.startsWith('-')) { // Context line -- still advances the new-file line number lineNumber++ diff --git a/.github/scripts/pr-review.mjs b/.github/scripts/pr-review.mjs index 8a2f67b87..221e1b63c 100644 --- a/.github/scripts/pr-review.mjs +++ b/.github/scripts/pr-review.mjs @@ -18,6 +18,18 @@ const { GITHUB_TOKEN, REPO, BASE_REF, PR_NUMBER, HEAD_SHA } = process.env const BOT_USER = 'github-actions[bot]' +/** + * Builds the comment body for a mistake. If the mistake includes a suggestion, + * appends a GitHub suggestion block so the author can apply the fix in one click. + * + * @param {{ message: string, suggestion?: string }} mistake + * @returns {string} + */ +function commentBody({ message, suggestion }) { + if (!suggestion) return message + return `${message}\n\n\`\`\`suggestion\n${suggestion}\n\`\`\`` +} + async function githubFetch(path, options = {}) { const response = await fetch(`https://api.github.com${path}`, { ...options, @@ -56,7 +68,7 @@ const botComments = existingComments const staleComments = botComments.filter( (c) => !mistakes.some( - (m) => m.path === c.path && m.line === c.line && m.message === c.body + (m) => m.path === c.path && m.line === c.line && commentBody(m) === c.body ) ) @@ -107,16 +119,16 @@ if (mistakes.length === 0) { // Post new comments for mistakes that don't already have a comment const newComments = mistakes .filter( - ({ path, line, message }) => + (m) => !botComments.some( - (c) => c.path === path && c.line === line && c.body === message + (c) => c.path === m.path && c.line === m.line && c.body === commentBody(m) ) ) - .map(({ path, line, message }) => ({ - path, - line, + .map((m) => ({ + path: m.path, + line: m.line, side: 'RIGHT', - body: message + body: commentBody(m) })) if (newComments.length === 0) { From e5a73c7c228622093c9afc874e79cddacc050b9d Mon Sep 17 00:00:00 2001 From: Frankie Roberto <30665+frankieroberto@users.noreply.github.com> Date: Mon, 22 Jun 2026 10:50:14 +0100 Subject: [PATCH 2/7] Only check for the URL within markdown links references to the URL elsewhere are ok. --- .github/scripts/check-markdown.mjs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/.github/scripts/check-markdown.mjs b/.github/scripts/check-markdown.mjs index b7d35d99f..89f88c9e5 100644 --- a/.github/scripts/check-markdown.mjs +++ b/.github/scripts/check-markdown.mjs @@ -67,7 +67,7 @@ export function scanAllFiles() { if (/^# /.test(lines[i])) { mistakes.push({ path: filePath, line: i + 1, message: H1_MESSAGE }) } - if (lines[i].includes(SITE_URL)) { + if (lines[i].includes('](' + SITE_URL)) { mistakes.push({ path: filePath, line: i + 1, message: ABSOLUTE_URL_MESSAGE }) } } @@ -122,7 +122,7 @@ export function getMistakes(baseRef) { } // Added line containing an absolute URL to the published site const lineContent = rawLine.slice(1) - if (lineContent.includes(SITE_URL)) { + if (lineContent.includes('](' + SITE_URL)) { mistakes.push({ path: currentFile, line: lineNumber, From 929e021ce65d7f2ba4ea9188c0786b59f3d2c420 Mon Sep 17 00:00:00 2001 From: Frankie Roberto <30665+frankieroberto@users.noreply.github.com> Date: Mon, 22 Jun 2026 10:59:44 +0100 Subject: [PATCH 3/7] Add test link --- app/test.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 app/test.md diff --git a/app/test.md b/app/test.md new file mode 100644 index 000000000..38378aeec --- /dev/null +++ b/app/test.md @@ -0,0 +1,5 @@ +--- +title: Test +--- + +This is a test [link](https://design-history.prevention-services.nhs.uk/breast-screening/). From 78506eeaad14d61fe876592f4472e821b63b29ae Mon Sep 17 00:00:00 2001 From: Frankie Roberto <30665+frankieroberto@users.noreply.github.com> Date: Mon, 22 Jun 2026 11:07:02 +0100 Subject: [PATCH 4/7] Remove test files --- app/test.md | 5 ----- 1 file changed, 5 deletions(-) delete mode 100644 app/test.md diff --git a/app/test.md b/app/test.md deleted file mode 100644 index 38378aeec..000000000 --- a/app/test.md +++ /dev/null @@ -1,5 +0,0 @@ ---- -title: Test ---- - -This is a test [link](https://design-history.prevention-services.nhs.uk/breast-screening/). From cde68679bf18b40bf6f84dbde9334865be66c9b2 Mon Sep 17 00:00:00 2001 From: Frankie Roberto Date: Mon, 20 Jul 2026 12:17:36 +0100 Subject: [PATCH 5/7] Update URL detection This now: * detects all full URLs, not just those in markdown link syntax * detects URLs using `http://` or just the domain name * is case-insensitive --- .github/scripts/check-markdown.mjs | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/.github/scripts/check-markdown.mjs b/.github/scripts/check-markdown.mjs index 89f88c9e5..3a7933d86 100644 --- a/.github/scripts/check-markdown.mjs +++ b/.github/scripts/check-markdown.mjs @@ -30,6 +30,11 @@ const H1_MESSAGE = const SITE_URL = 'https://design-history.prevention-services.nhs.uk/' +// For replacing any URL variant with '/' in suggestions (global, case-insensitive) +const SITE_URL_REPLACE_RE = /(https?:\/\/)?design-history\.prevention-services\.nhs\.uk\//gi +// Same pattern without 'g' flag, safe for repeated .test() calls in a loop +const SITE_LINK_RE = new RegExp(SITE_URL_REPLACE_RE.source, 'i') + const ABSOLUTE_URL_MESSAGE = 'Use a relative URL instead of a full URL for links to other posts on the site.\n\n' + 'This means that the links will work in previews, and in case the site domain name changes in future.\n\n' + @@ -67,7 +72,7 @@ export function scanAllFiles() { if (/^# /.test(lines[i])) { mistakes.push({ path: filePath, line: i + 1, message: H1_MESSAGE }) } - if (lines[i].includes('](' + SITE_URL)) { + if (SITE_LINK_RE.test(lines[i])) { mistakes.push({ path: filePath, line: i + 1, message: ABSOLUTE_URL_MESSAGE }) } } @@ -84,6 +89,10 @@ export function scanAllFiles() { * @returns {{ path: string, line: number, message: string }[]} */ export function getMistakes(baseRef) { + // Reject unexpected characters to prevent shell command injection + if (!/^[\w\/.\-]+$/.test(baseRef)) { + throw new Error(`Invalid baseRef: ${baseRef}`) + } const diff = execSync(`git diff ${baseRef}...HEAD`, { encoding: 'utf8' }) const mistakes = [] let currentFile = null @@ -122,12 +131,12 @@ export function getMistakes(baseRef) { } // Added line containing an absolute URL to the published site const lineContent = rawLine.slice(1) - if (lineContent.includes('](' + SITE_URL)) { + if (SITE_LINK_RE.test(lineContent)) { mistakes.push({ path: currentFile, line: lineNumber, message: ABSOLUTE_URL_MESSAGE, - suggestion: lineContent.replaceAll(SITE_URL, '/') + suggestion: lineContent.replace(SITE_URL_REPLACE_RE, '/') }) } } else if (!rawLine.startsWith('-')) { From f698ac5929d9a6204021a16c781716396a6ec801 Mon Sep 17 00:00:00 2001 From: Frankie Roberto Date: Mon, 20 Jul 2026 12:24:38 +0100 Subject: [PATCH 6/7] Refactor checks into functions --- .github/scripts/check-markdown.mjs | 80 ++++++++++++++++++++---------- 1 file changed, 53 insertions(+), 27 deletions(-) diff --git a/.github/scripts/check-markdown.mjs b/.github/scripts/check-markdown.mjs index 3a7933d86..a8e7bfff3 100644 --- a/.github/scripts/check-markdown.mjs +++ b/.github/scripts/check-markdown.mjs @@ -23,22 +23,46 @@ import { readFileSync, readdirSync } from 'fs' import { join } from 'path' import { fileURLToPath } from 'url' -const H1_MESSAGE = - 'The page title H1 is already generated from the `title` field in the frontmatter. ' + - 'If this heading duplicates the title, remove it. ' + - 'If it is a different heading, change it to an H2 using `##`.' - -const SITE_URL = 'https://design-history.prevention-services.nhs.uk/' - -// For replacing any URL variant with '/' in suggestions (global, case-insensitive) -const SITE_URL_REPLACE_RE = /(https?:\/\/)?design-history\.prevention-services\.nhs\.uk\//gi -// Same pattern without 'g' flag, safe for repeated .test() calls in a loop -const SITE_LINK_RE = new RegExp(SITE_URL_REPLACE_RE.source, 'i') +/** + * Checks if a line contains an H1 heading. + * + * @param {string} line + * @returns {{ message: string } | null} + */ +function checkH1Heading(line) { + const message = + 'The page title H1 is already generated from the `title` field in the frontmatter. ' + + 'If this heading duplicates the title, remove it. ' + + 'If it is a different heading, change it to an H2 using `##`.' + + if (/^# /.test(line)) { + return { message } + } + return null +} -const ABSOLUTE_URL_MESSAGE = - 'Use a relative URL instead of a full URL for links to other posts on the site.\n\n' + - 'This means that the links will work in previews, and in case the site domain name changes in future.\n\n' + - 'For example, replace `https://design-history.prevention-services.nhs.uk/some/path/` with `/some/path/`.' +/** + * Checks if a line contains an absolute URL to the published site. + * + * @param {string} line + * @returns {{ message: string, suggestion?: string } | null} + */ +function checkAbsoluteUrl(line) { + const siteUrlRe = /(https?:\/\/)?design-history\.prevention-services\.nhs\.uk\//i + + const message = + 'Use a relative URL instead of a full URL for links to other posts on the site.\n\n' + + 'This means that the links will work in previews, and in case the site domain name changes in future.\n\n' + + 'For example, replace `https://design-history.prevention-services.nhs.uk/some/path/` with `/some/path/`.' + + if (siteUrlRe.test(line)) { + return { + message, + suggestion: line.replaceAll(siteUrlRe, '/') + } + } + return null +} /** * Recursively finds all .md files under the given directory. @@ -69,11 +93,13 @@ export function scanAllFiles() { for (const filePath of files) { const lines = readFileSync(filePath, 'utf8').split('\n') for (let i = 0; i < lines.length; i++) { - if (/^# /.test(lines[i])) { - mistakes.push({ path: filePath, line: i + 1, message: H1_MESSAGE }) + const h1 = checkH1Heading(lines[i]) + if (h1) { + mistakes.push({ path: filePath, line: i + 1, message: h1.message }) } - if (SITE_LINK_RE.test(lines[i])) { - mistakes.push({ path: filePath, line: i + 1, message: ABSOLUTE_URL_MESSAGE }) + const url = checkAbsoluteUrl(lines[i]) + if (url) { + mistakes.push({ path: filePath, line: i + 1, message: url.message }) } } } @@ -121,22 +147,22 @@ export function getMistakes(baseRef) { if (rawLine.startsWith('+')) { lineNumber++ - // Added line that is an H1 (single `#` followed by a space) - if (/^\+# /.test(rawLine)) { + const lineContent = rawLine.slice(1) + const h1 = checkH1Heading(lineContent) + if (h1) { mistakes.push({ path: currentFile, line: lineNumber, - message: H1_MESSAGE + message: h1.message }) } - // Added line containing an absolute URL to the published site - const lineContent = rawLine.slice(1) - if (SITE_LINK_RE.test(lineContent)) { + const url = checkAbsoluteUrl(lineContent) + if (url) { mistakes.push({ path: currentFile, line: lineNumber, - message: ABSOLUTE_URL_MESSAGE, - suggestion: lineContent.replace(SITE_URL_REPLACE_RE, '/') + message: url.message, + suggestion: url.suggestion }) } } else if (!rawLine.startsWith('-')) { From 78236af5fc82b723ba7b1e0f156ebfaf03a3e215 Mon Sep 17 00:00:00 2001 From: Frankie Roberto Date: Mon, 20 Jul 2026 12:28:10 +0100 Subject: [PATCH 7/7] Refactor --- .github/scripts/check-markdown.mjs | 42 ++++++++++++++---------------- 1 file changed, 19 insertions(+), 23 deletions(-) diff --git a/.github/scripts/check-markdown.mjs b/.github/scripts/check-markdown.mjs index a8e7bfff3..b1e2d1efb 100644 --- a/.github/scripts/check-markdown.mjs +++ b/.github/scripts/check-markdown.mjs @@ -64,6 +64,21 @@ function checkAbsoluteUrl(line) { return null } +const checks = [checkH1Heading, checkAbsoluteUrl] + +/** + * Runs all checks against a single line and returns any mistakes found. + * + * @param {string} line + * @returns {{ message: string, suggestion?: string }[]} + */ +function checkLine(line) { + return checks.flatMap((check) => { + const result = check(line) + return result ? [result] : [] + }) +} + /** * Recursively finds all .md files under the given directory. * @@ -93,13 +108,8 @@ export function scanAllFiles() { for (const filePath of files) { const lines = readFileSync(filePath, 'utf8').split('\n') for (let i = 0; i < lines.length; i++) { - const h1 = checkH1Heading(lines[i]) - if (h1) { - mistakes.push({ path: filePath, line: i + 1, message: h1.message }) - } - const url = checkAbsoluteUrl(lines[i]) - if (url) { - mistakes.push({ path: filePath, line: i + 1, message: url.message }) + for (const result of checkLine(lines[i])) { + mistakes.push({ path: filePath, line: i + 1, ...result }) } } } @@ -148,22 +158,8 @@ export function getMistakes(baseRef) { if (rawLine.startsWith('+')) { lineNumber++ const lineContent = rawLine.slice(1) - const h1 = checkH1Heading(lineContent) - if (h1) { - mistakes.push({ - path: currentFile, - line: lineNumber, - message: h1.message - }) - } - const url = checkAbsoluteUrl(lineContent) - if (url) { - mistakes.push({ - path: currentFile, - line: lineNumber, - message: url.message, - suggestion: url.suggestion - }) + for (const result of checkLine(lineContent)) { + mistakes.push({ path: currentFile, line: lineNumber, ...result }) } } else if (!rawLine.startsWith('-')) { // Context line -- still advances the new-file line number