Skip to content

Size every image served from public/, and fail CI on a missing one - #1135

Open
Iamfle4ka wants to merge 21 commits into
mainfrom
claude/serene-dirac-o1hc1u
Open

Iamfle4ka wants to merge 21 commits into
mainfrom
claude/serene-dirac-o1hc1u

Conversation

@Iamfle4ka

@Iamfle4ka Iamfle4ka commented Sep 21, 2026 •

Copy link
Copy Markdown
Collaborator

Follow-up to #1132, closing what Michal Jeřábek's front-end audit left open. Tracked as PRDCT-691.

Updated 14 Oct after Jordan's review. The sizing moved from a remark plugin to a pass over the built HTML, the image frame is now a ring, and a missing image fails the build in CI. The link-fragment check is no longer in this PR: #1073 does the same job and already reports fragments that go through a redirect stub.

What changes

Every image from public/ gets width and height: 2,015 of 2,031 <img> in the build are sized. Astro sizes the images it processes (/_astro/…) but never touches public/, and AGENTS.md has authors reference screenshots there (![Alt](/section/page/image.png)). Those shipped as a bare <img>, so the browser had no aspect ratio to reserve space with and the page jumped as images loaded.

  • src/integrations/image-dimensions.mjs runs in astro:build:done. It walks every built .html page and adds width/height to each <img> whose file is in dist/, resolving the src against the page URL the way a browser does. That covers markdown images, figures that beacon-transforms writes as raw HTML, and hand-written <img> tags alike.
  • It skips /_astro/ images (Astro sized them), external images, and any tag where the author already set a width or height.
  • Sizes come from the file header: PNG, JPEG, GIF, WebP and SVG, picked by the file's first bytes rather than its extension. No dependency is added. SVG width/height count only as a plain number or px; anything else falls back to the viewBox.
  • Because it reads the files that shipped, a screenshot replaced under the same name can't keep its old size from Astro's content cache. It doesn't run in astro dev.

A missing image fails the build in CI. Every <img> whose file isn't in the build is a warning; when CI is set (GitHub Actions and Vercel), the build throws. Local builds only warn, so a page can be edited before its screenshot lands. An image whose size can't be read is a warning too.

The image frame is a ring. .sl-markdown-content img had border: 1px under Starlight's border-box sizing, so once a tag carries width, the border comes out of that width and every image narrower than the column would render 2px small. It's box-shadow: 0 0 0 1px now, which draws the same frame outside the box.

Not sized, on purpose

16 of 2,031: 6 external images, 8 <img> in the dbt comparison table that carry an author width (width="226px"), and 2 width="45%" images in components/index.md. Those widths are layout choices, so the pass leaves the tags alone.

Checks

  • npm test: 28/28, including scripts/image-dimensions.test.ts (JPEG named .png, the SVG cases from review, relative and /_astro/ sources, missing image warns locally and throws in CI).
  • CI=1 npm run build: clean, "Sized 1197 images on 243 pages" (the rest of the 2,015 are Astro's own).
  • dist/ against a build of main: 383 HTML files change, and none differ once width/height on <img> and the CSS bundle hash are ignored. The only CSS change is the border → ring rule.

Not in this PR

  • The link-fragment check: #1073.
  • Meta descriptions: the PRDCT-696 PRs.
  • lastUpdated: still off, see PRDCT-691.

🤖 Generated with Claude Code

1,193 of the site's 1,977 <img> tags shipped with no width or height, so a
browser had no aspect ratio to reserve space with until the bytes arrived.
The split was not arbitrary: Astro sizes the images it processes itself,
but it never touches public/, and AGENTS.md prescribes referencing
screenshots as `![Alt](/section/page/image.png)` — which lands in public/.

A remark plugin reads the intrinsic size from the file header and attaches
it via hProperties. Starlight already ships `max-width: 100%; height: auto`
for content images, which is exactly the pairing that makes intrinsic
attributes correct — the ratio comes from the attributes, CSS still
controls the displayed size — so nothing about how images render changes.

Header parsing is synchronous and hand-rolled for PNG, JPEG, GIF, WebP and
SVG rather than calling sharp, so the remark pipeline stays synchronous
like every other plugin here and no dependency is added. Results are cached
per build, since one screenshot is often referenced from several pages.
JPEG retries on the full file, because its frame header can sit past the
first 64KB.

beacon-transforms' transformFigureCaptions builds its <img> by hand, so it
had to be taught to carry the dimensions over — otherwise figures would be
the one place still shipping an unsized image. The plugin runs first for
that reason.

Result: 784 -> 1,958 of 1,977 sized. The 19 left are 6 external images,
which cannot be measured, and 13 hand-written tags inside the dbt page's
bespoke comparison table that already carry an explicit display width —
changing those is a layout decision, not a mechanical fix.

A missing file warns rather than throws: audit-phase2.mjs is what fails on
a genuinely missing image, and a hard error here would block editing a page
whose screenshot has not landed yet.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PEKnDNvPqkNrBonYN6S61r
audit-phase2.mjs checked that a link's path resolves but never that its
#fragment names an anchor that exists. That is the gap a rename falls
through: the path stays valid, the anchor quietly stops existing, and the
link lands at the top of a long reference page instead of its section. It
is also the class of bug review caught in PR #1132, where a link rewrite
changed path prefixes and carried fragments over verbatim.

The check follows redirect_from's meta-refresh stubs before looking for
ids. Without that, every link through a redirect would report as broken —
a false-positive class that would make the check worth ignoring.

It also covers same-page `#fragment` links, which the scan previously
skipped wholesale. That turned out to matter: 7 of the 18 breakages were
same-page, including three on /storage/api/configurations/ pointing at
#modifying-a-configuration when its own heading is #modifying-configuration.

Fixed, each verified against the target page's real headings:
  #dataType                              -> #data-type
  #merging-response                      -> #merging-responses
  #an-object-with-nested-object          -> #object-with-nested-object
  #an-object-with-a-deeply-nested-object -> #object-with-a-deeply-nested-object
  #configuration-sections                -> #json-configuration-sections
  api/#baseurl                           -> api/#base-url
  oauth20/#configuration                 -> oauth20/#configuration-parameters
  publish/#submission                    -> publish/#publishing
  #api-default-parameter                 -> #api-default-parameters
  #modifying-a-configuration             -> #modifying-configuration
  /storage/#backends                     -> #storage-backend-types-and-features
  /management/limits/#project-power      -> #project-power--time-credits
  (/#function-contexts)                  -> (#function-contexts), a stray
                                            leading slash on a same-page link

One is a judgement call and wants a reviewer's eye: /workspace/table-export
said "download the file with the standard file download flow" and pointed
at the Storage API *importer*, where no such anchor exists and the page is
about uploading. Repointed at /storage/files/#file-links, the only section
that describes downloading a file. If that is the wrong home, say so.

audit-phase2 now reports 0 broken links, 0 broken fragments, 0 missing
images.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PEKnDNvPqkNrBonYN6S61r
…back on

PR #1132 switched lastUpdated on, the preview showed the "Updated <date>"
line on no page at all, and it came back out. The cause was established
then: Starlight takes each page's date from its last commit, Vercel clones
shallow, getLastUpdated catches the lookup error and returns undefined.

Vercel documents no clone-depth setting, so the build has to deepen the
clone itself. That runs from the `prebuild` npm script rather than a
`buildCommand` in vercel.json, deliberately: `npm run build` is what every
environment already runs, so one mechanism covers Vercel, GitHub Actions
and a local build, and nothing overrides the build command configured in
the Vercel project — which is not visible from this repository.

The script never fails a build. A missing .git, no network, or a credential
the build step cannot use all fall through to a warning, and the site
builds exactly as it does today with the dates absent. That matters because
whether Vercel's build step can fetch is the one thing that cannot be
settled from here — it has to be read off the preview, same as last time.

The workflows are untouched: `fetch-depth: 0` is not reintroduced, since
prebuild now deepens the clone in Actions too.

Verified: all three paths exercised (shallow clone deepened, full clone
no-op, no repository warns and exits 0), then a full build — 366 of 366
content pages carry an Updated line across 33 distinct dates, so the dates
are per-file history rather than one build-time stamp.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PEKnDNvPqkNrBonYN6S61r
@vercel

vercel Bot commented Sep 21, 2026 •

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated
connection-docs Ready Ready Preview Oct 6, 2026 7:11am UTC

Request Review

…turn it back on"

This reverts 8166dce. The preview still shows no "Updated <date>" line, so
the criterion this PR set applies and lastUpdated comes back out rather
than holding up the two changes that do work.

Reverted whole rather than left unwired: a script nothing calls is exactly
the kind of dead weight the previous PR existed to remove. The code is
recoverable from 8166dce when there is something to act on.

What is NOT yet known is *why* it still fails, and there are two candidates
that look identical from outside:

  a) Vercel runs `npm run build`, prebuild fires, and the `git fetch
     --unshallow` is refused because the build step has no usable
     credential. Fix: a different way to get history, or accept no dates.
  b) Vercel's configured build command is not `npm run build`, so prebuild
     never runs at all and the script is inert. Fix: set `buildCommand` in
     vercel.json — which this PR deliberately avoided, because the project's
     current command is not visible from this repository.

One look at a Vercel build log settles it: search for `[unshallow]`.
Present and "done" means the clone was deepened and the cause is elsewhere;
present and "could not deepen" is (a); absent entirely is (b).

Verified locally before reverting: the mechanism itself is sound — a
shallow clone is deepened, a full clone no-ops, a missing repository warns
and exits 0, and a full-history build puts an Updated line on 366 of 366
content pages across 33 distinct dates. So this is about the build
environment, not the script.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PEKnDNvPqkNrBonYN6S61r
The comment left behind by the revert proposed exactly the approach that
had just failed. Replaced with what is actually known: the mechanism works
locally, the preview still showed no dates, and the next person should
first search a Vercel build log for `[unshallow]` — present-and-done,
present-and-refused, and absent each point at a different fix, and only
the last one makes vercel.json's buildCommand the right lever.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PEKnDNvPqkNrBonYN6S61r
@Iamfle4ka Iamfle4ka changed the title Size every image from public/, repair 18 dangling anchors, make lastUpdated actually work Size every image from public/, repair 18 dangling anchors, add a fragment check Sep 21, 2026
Keeps the PR mergeable — 7 commits behind after the Kai pricing work
(PR #1136). No overlap with this PR's files.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PEKnDNvPqkNrBonYN6S61r
@Iamfle4ka Iamfle4ka added the site-tooling Touches build config, CI, scripts or runtime code — the reviewer bot always routes these to a human label Sep 23, 2026
@Iamfle4ka

Copy link
Copy Markdown
Collaborator Author

@keboola-pr-reviewer review

1 similar comment
@Iamfle4ka

Copy link
Copy Markdown
Collaborator Author

@keboola-pr-reviewer review

Main landed PR #1112 (PRDCT-679), which fixed the same dangling anchors
this branch did, independently. Three files conflicted; main's side is
taken in all three, and its version is the better one twice over:

- functions/index.md — main writes the absolute path where this branch
  used a same-page fragment. Equivalent in the browser, more explicit.
- incremental/index.md — main points at /management/project/limits/, the
  canonical page. This branch pointed at /management/limits/, which only
  works because a redirect stub catches it. Main's skips the hop.
- table-export.md — this branch repointed "the standard file download
  flow" at /storage/files/#file-links, arguing the Storage API importer
  page is about uploading. Main instead kept the importer page and dropped
  the dangling fragment. That was flagged as an owner's call in the PR
  description and merging #1112 answered it, so main's stands. The defect
  — an anchor that resolves nowhere — is gone either way.

Eleven of the thirteen anchor fixes turned out identical on both sides,
which is reassuring about both. What remains here is only the work main
does not have: the image-dimensions remark plugin and the fragment check
in audit-phase2.mjs — which now validates main's anchor work too.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PEKnDNvPqkNrBonYN6S61r
keboola-pr-reviewer-bot

This comment was marked as outdated.

@Iamfle4ka Iamfle4ka changed the title Size every image from public/, repair 18 dangling anchors, add a fragment check Size every image served from public/, and check link fragments in the audit Sep 23, 2026
keboola-pr-reviewer-bot

This comment was marked as outdated.

@jordanrburger jordanrburger left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verdict: request changes. The idea is right, and the header parsers are solid. I checked readDimensions() against sips on all 1,531 raster files in public/ and every PNG, JPEG, GIF and WebP matched exactly. I built the PR head and its main parent (f192b232) and diffed every <img> in dist/. Nothing changed except the added width/height, and the sized count went from 798 to 1,978 of 1,997. The audit also reports 0 broken fragments and 0 missing images on the head, as the description says. Even so, several things need fixing before this merges: one claim in the description is wrong, one correctness bug is waiting to bite, and the audit's redirect handling is backwards.

Merge conflict. git merge-tree against current main conflicts in astro.config.mjs. Main now has remarkPlugins: [remarkStripComments, beaconTransforms] from #1175 and also adds pagefindTitles(). I'd resolve it as [remarkStripComments, imageDimensions, beaconTransforms].

Blocking

  1. Images render 2px smaller, so the claim that "nothing about how images render changes" doesn't hold. custom.css gives content images border: 1px solid var(--border-1), and Starlight's reset sets box-sizing: border-box. Without a width attribute, an image rendered at its natural width plus the border. With width="N", the border now comes out of N, so the bitmap is downscaled by 2px. I measured this with both builds served side by side at 1280px. The Kai permission icons went from 74×74 to 72×72, which means a 72px icon is drawn at 70px. The 447px screenshot on /kai/settings/ went from 449×703 to 447×700, and /components/variables/api/ went from 542×634 to 540×632. Images that hit the 718px column cap don't change. About 163 of the sized public/ image instances are narrower than the column, and all of them become slightly resampled and soft. That matters most for icons and 1:1 UI-locator crops. Fix: in src/styles/custom.css, replace the border on .sl-markdown-content img with box-shadow: 0 0 0 1px var(--border-1). It draws the same ring without affecting layout.

  2. The relative-URL branch of resolveImage sizes Astro-managed images from the wrong file. Details are inline. I reproduced both failure modes: an image that is valid in Astro was squashed from 772×659 to 72×72, and a valid image triggered a spurious "missing image" warning. It's a one-line fix.

  3. The fragment check treats links through redirect stubs as valid, but browsers drop the fragment on a meta refresh. Details are inline. I verified it in a browser: /management/limits/#platform-limits lands at the top of /management/project/limits/ with no hash, and production serves the same meta-refresh stub. There are 0 such links today, so nothing is hidden right now. Still, the check's headline feature currently passes exactly the kind of link it should flag.

Important

  • A missing image fails nothing. The warning comment and the PR description both say audit-phase2.mjs "is what fails on a genuinely missing image". It never exits non-zero (the only process.exit is the no-dist/ case), and .github/workflows/branch.yml doesn't run it. A mistyped public/ path still ships silently, as it did before this PR. Either make the audit exit 1 on broken links, fragments or missing images and add it to the Build job, or make the plugin throw when process.env.CI is set. The second option keeps local editing unblocked, which was the concern in the description.
  • Cached content goes stale when a screenshot changes. The plugin's output depends on files in public/, but Astro's content-layer cache (node_modules/.astro/data-store.json) is keyed on the markdown, not on those files. Replace a screenshot with a different-size file under the same name, rebuild, and the old dimensions ship. I reproduced this locally, and it only cleared after deleting the data store. Re-shooting a screenshot under the same filename is the normal docs workflow, so this will happen. I haven't verified whether Vercel's build cache restores node_modules/.astro.
  • The SVG width/height regex mis-parses common forms. width="100%" becomes 100×100, stroke-width="2" on the root becomes 2px wide (\b matches after the -), and em units are read as px. Because the width attribute sets the displayed width here, any of these would visibly shrink the diagram. No current SVG triggers this. Mermaid and some Figma exports would. Details are inline.
  • The description's breakdown of what is still unsized is off. On the current base the 19 are 6 external, 8 in the dbt table, 2 width="45%" tags in components/index.md, and 3 markdown images on bdm-guide (desk.png, post-its-step3.png, post-its-step4.png). That's not "13 in the dbt page". The bdm-guide files are JPEGs with a .png extension. Parser dispatch is by extension, so they fail silently. Details are inline.

Design suggestion (not blocking)

Moving this to an astro:build:done pass over dist/**/*.html that sizes any <img src="/…"> lacking dimensions would avoid most of the above in one move:

  • it sizes raw HTML <img> too, so the hand-coded n8n sizes aren't needed;
  • it removes the plugin-ordering coupling with beacon-transforms;
  • it can't be served stale from the content cache;
  • it never sees Astro's /_astro/ images.

The cost is that it doesn't run in dev, which is fine for a layout-shift fix.

Nits

  • audit-phase2.mjs fragment edge cases, checked against a synthetic fixture:

    • #top is flagged, but per the HTML spec it scrolls to the top with no matching id.
    • <a name="x"> targets are flagged as broken.
    • An id="…" that appears inside rendered code text counts as a real anchor.
    • Relative hrefs (../x/#y) are never checked.

    All of these are 0 on the site today, so this is just hardening.

  • The module-level cache in image-dimensions.mjs is never invalidated, so astro dev serves stale dimensions after a screenshot changes until restart. Keying on path + mtimeMs fixes it.

  • The rewritten lastUpdated comment block in astro.config.mjs is unrelated to this PR. It also cites 8166dcea, which exists only on this branch and won't be in main's history after a squash merge. Cite #1135 or move the note to PRDCT-691.

Comment thread astro.config.mjs Outdated
// imageDimensions runs first: beaconTransforms turns some images into raw
// HTML, and reads the dimensions this attaches. Passed uncalled — unified
// invokes the plugin itself to get the transformer; all options default.
remarkPlugins: [imageDimensions, beaconTransforms],

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line conflicts with main. #1175 changed it to [remarkStripComments, beaconTransforms]. I'd resolve to [remarkStripComments, imageDimensions, beaconTransforms] so author comments are stripped before anything else walks the tree.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 667135d

Merged main in b70c9ac. The image pass is now an astro:build:done integration, so remarkPlugins is main's [remarkStripComments, beaconTransforms] again and there is no plugin order to get right.

Comment thread src/integrations/image-dimensions.mjs Outdated
if (decoded.startsWith('/')) return path.join(publicDir, decoded);
// A relative URL resolves against the page's own URL, which mirrors its
// directory under public/ for anything not picked up by Astro's pipeline.
return pageDir ? path.join(publicDir, pageDir, decoded) : null;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This branch only ever fires on images that Astro manages. remarkCollectImages adds every markdown image URL that isn't a full URL and doesn't start with / to localImagePaths. rehypeImages then passes the node's properties, including the width/height set here, into getImage(). So the public/<pageDir>/<file> lookup feeds a different file's size into Astro's resize.

I reproduced this on workspace/jdbc-driver/dbeaver.md, which has duplicates in both src/ and public/:

  • I swapped the public/ copy of DBeaver_DB_1.png for a 72×72 icon. The real 772×659 screenshot then shipped as /_astro/DBeaver_DB_1…webp at width="72" height="72".
  • I deleted the public/ copy of DBeaver_driver_2.png, and the build logged [image-dimensions] missing image ./DBeaver_driver_2.png for an image that renders fine.

Today's duplicates are byte-identical, so nothing is visibly wrong yet. Astro already sizes these images, so just skip them:

if (!decoded.startsWith('/')) return null; // relative → Astro's pipeline sizes it
return path.join(publicDir, decoded);

That also lets you drop contentDir/pageDir entirely.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 667135d

Moved the sizing to a pass over the built HTML, as you suggested. It skips /_astro/ images, so Astro-managed relative images keep Astro's own size, and it reads the file that actually shipped in dist/. contentDir/pageDir are gone.

Comment thread src/integrations/image-dimensions.mjs Outdated
// Not fatal: audit-phase2.mjs is what fails on a genuinely missing
// image, and a hard error here would block editing a page whose
// screenshot has not landed yet.
console.warn(`[image-dimensions] missing image ${node.url} (referenced by ${file?.path ?? 'unknown page'})`);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

audit-phase2.mjs doesn't fail on anything. It prints a total and exits 0, and CI (branch.yml) doesn't run it. So right now a missing public/ image is only this console.warn, buried in build output. Either gate the audit (exit 1 when broken links, fragments or missing images are found, and add node scripts/audit-phase2.mjs after npm run build in the Build job), or throw here when process.env.CI is set. Then update the comment and the PR description to match.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in d21363a

Took the second option. The pass warns for every <img> whose file isn't in dist/ and throws when CI is set, so GitHub Actions and Vercel fail while local builds only warn. The comment says so now, and there's a test for both cases (7a1f731).

const HEADER_BYTES = 65536;

/** Resolved path → {width, height} | null (null = parsed and unusable). */
const cache = new Map();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are two staleness problems here.

  1. This map lives for the whole process, so in astro dev a replaced screenshot keeps its old dimensions until restart. Keying on ${file}:${statSync(file).mtimeMs} fixes that.
  2. The bigger one: Astro's content layer caches rendered markdown in node_modules/.astro/data-store.json, keyed on the entry, not on the public/ files this reads. I swapped a public/ image for a different-size one and rebuilt, and the old dimensions were still in dist/. They only updated after I deleted the data store. A remark plugin can't add a file dependency to that digest. That's the main reason I'd move this to an astro:build:done pass over the built HTML.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 667135d

Both go away with the HTML pass: it reads the built files on every build, and the size cache is cleared at the start of each one. The pass doesn't run in astro dev.

const head = b.toString('utf8', 0, Math.min(b.length, 8192));
const tag = head.match(/<svg\b[^>]*>/i);
if (!tag) return null;
const attr = (name) => {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This regex has a few false matches. I tested each against readDimensions():

  • <svg width="100%" height="100%" viewBox="0 0 800 400"> returns {100, 100}.
  • <svg stroke-width="2" viewBox="0 0 800 400" height="400"> returns {2, 400}, because \bwidth matches after the -.
  • width="20em" returns {20, …} read as px.

The width attribute sets the displayed width, so each of these would render the diagram tiny. Anchor the attribute name with (?:^|\s)width\s*=, accept only a unitless or px value followed by the closing quote, and fall back to the viewBox for anything else (%, em, missing). No SVG in public/ hits this today, but Mermaid exports use width="100%".

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 2138da5

The attribute name has to start the attribute now, and only a unitless or px value counts. 100%, em and the rest fall back to the viewBox. Tests cover your three cases; the 8 SVGs in public/ get the same sizes as before.

Comment thread scripts/audit-phase2.mjs Outdated
// A fragment linked through one belongs to the destination, so follow it —
// otherwise every link through a redirect would look broken.
const hop = body.match(/<meta\s+http-equiv=["']refresh["']\s+content=["'][^"']*url=([^"']+)["']/i);
if (hop && hops < 5) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This passes exactly the links it should flag. The stubs from redirect-from.mjs are <meta http-equiv="refresh" content="0;url=/target/">. A meta refresh is a fresh navigation to that URL, and unlike an HTTP 3xx it doesn't inherit the original fragment. I checked locally: /management/limits/#platform-limits ends at /management/project/limits/ with scrollY=0 and no hash. Production serves the same stub (200 + meta refresh), so readers hit this too. Following the hop and checking the destination's ids reports /old/#real-section as fine when the reader actually lands at the top of the page.

There are 0 such links on the site today; I counted all 13,067 fragment links. So I'd make this a finding instead: any internal link with a #fragment whose path is a redirect stub gets reported as "fragment through redirect, point at the canonical URL". This is the same reasoning you used for taking main's side on incremental/index.md.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in b343460

Dropped the fragment check from this PR. #1073 adds the same check and already reports a fragment linked through a redirect stub as its own finding (anchor-on-redirect-stub), which is what you describe here, so the fragment work lives there.

Comment thread scripts/audit-phase2.mjs Outdated
if (hop && hops < 5) {
ids = idsFor(hop[1].split('#')[0], hops + 1);
} else {
ids = new Set([...body.matchAll(/\sid=["']([^"']+)["']/g)].map((m) => m[1]));

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit, hardening only. On a synthetic fixture this misses two cases in opposite directions:

  • An id="ghost" that appears literally inside rendered code text (<code>&lt;div id="ghost"&gt;</code>) counts as a real anchor, so /a/#ghost passes. That's a false negative.
  • <a name="legacy"> isn't collected, so /a/#legacy is reported as broken even though browsers honor it. That's a false positive.

Neither occurs on the site today. Matching ids only inside tags (<[a-z][^>]*\sid="…") and also collecting <a name> would close both.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in b343460

Moot here now that the fragment check is gone (b343460). I'll carry both cases to #1073.

Comment thread scripts/audit-phase2.mjs Outdated
const isImg = m[0].toLowerCase().startsWith('src');

if (/^(mailto:|tel:|javascript:|data:|#)/i.test(url)) continue;
if (url.startsWith('#')) { checkFragment(r, r, url); continue; }

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: #top gets reported as broken, but per the HTML spec it scrolls to the top even with no matching id. Skip it alongside _top. Separately, relative hrefs with fragments (../x/#y) hit the !url.startsWith('/') skip below and are never checked. There are 0 today, but it's a silent gap worth a comment.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in b343460

Same: moot here, noted for #1073 (#top and relative hrefs).

Comment thread scripts/audit-phase2.mjs Outdated
dump('MALFORMED TABLES (source)', tableIssues, (x) => `${x.file}:${x.line} header=${x.header} sep=${x.sep}`);

const total = findings.brokenLinks.length + findings.missingImages.length +
const total = findings.brokenLinks.length + findings.brokenFragments.length + findings.missingImages.length +

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The PR relies on this script to "fail" on missing images, but nothing here sets an exit code. Suggest something like if (findings.brokenLinks.length + findings.brokenFragments.length + findings.missingImages.length) process.exitCode = 1;, with the Jekyll smells and the rest staying informational, then wiring it into CI.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in d21363a

A missing image now fails the build itself in CI (see the thread on image-dimensions.mjs), so this PR no longer touches the audit. The audit's exit code can go with the fragment check in #1073.

5. Accept the community node disclaimer and confirm installation.

<img src="./01-self-hosted-installation.png" alt="Install Keboola Node for Self-hosted n8n" />
<img src="./01-self-hosted-installation.png" alt="Install Keboola Node for Self-hosted n8n" width="4096" height="2378" />

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: the values are correct (sips agrees for all five), but hand-coded sizes go out of date silently the next time someone re-shoots these screenshots. If the sizing moves to a pass over the built HTML (see the summary), raw <img> tags like these get sized automatically and these edits can be reverted.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 667135d

Reverted. The HTML pass sizes these raw <img> tags itself, with the same values.

@Iamfle4ka Iamfle4ka closed this Oct 14, 2026
Nikita and others added 9 commits October 14, 2026 17:35
#1073 adds the same check and also reports a fragment linked through a redirect stub as its own finding (anchor-on-redirect-stub), which is what review asked for here. Keeping one implementation avoids two PRs editing the same audit section.

Addresses comments by @jordanrburger on scripts/audit-phase2.mjs:79, :82, :129

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
An astro:build:done pass over the built .html pages stamps width/height on every <img> whose file shipped in dist/. It skips /_astro/ images, which Astro sizes itself, so relative markdown images can no longer pick up a public/ copy's size. It reads the built files on every build, so Astro's content cache can't serve stale sizes after a screenshot is replaced. Raw <img> tags are covered too, so the hand-coded n8n sizes and the beacon-transforms figure change are reverted, and the plugin-ordering coupling is gone.

Sized: 2,012 of 2,031 <img> in dist (rest: 6 external, 8 dbt tags with an author width, 2 width=45%, 3 bdm-guide JPEGs named .png).

Addresses comments by @jordanrburger on src/integrations/image-dimensions.mjs:182, :36, src/content/docs/external-integrations/n8n/index.md:31 and astro.config.mjs:24

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The image pass collects every <img> whose file isn't in dist/, warns for each, and throws when CI is set (GitHub Actions and Vercel both set it). Local builds only warn, so a page can be edited before its screenshot lands. The audit stays informational; a missing image no longer depends on it.

Addresses comments by @jordanrburger on src/integrations/image-dimensions.mjs:214 and scripts/audit-phase2.mjs:205

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…e read

Dispatch now goes by magic bytes (PNG, JPEG, GIF, WebP, SVG/XML) instead of the extension, so the three bdm-guide JPEGs saved as .png get sized. A local image whose size still can't be read is now a build warning instead of a silent skip, and the comment that claimed the caller warned is corrected. Adds scripts/image-dimensions.test.ts (npm test).

Addresses comment by @jordanrburger on src/integrations/image-dimensions.mjs:146

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The attribute name must start the attribute, so stroke-width no longer matches, and only a unitless or px value counts. width="100%", em and other units fall back to the viewBox. The 8 SVGs in public/ get the same sizes as before; tests cover the three cases from review.

Addresses comment by @jordanrburger on src/integrations/image-dimensions.mjs:125

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Content images had border: 1px under Starlight's border-box sizing. Once the tag carries width/height, the border comes out of that width and every image narrower than the column was downscaled by 2px (72px icons drawn at 70px). box-shadow: 0 0 0 1px draws the same ring outside the box without affecting layout.

Addresses review summary by @jordanrburger (blocking item 1)

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The rewrite was unrelated to image sizing and cited a commit that only exists on this branch. The investigation notes belong in PRDCT-691, not in this PR.

Addresses review nit by @jordanrburger

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@Iamfle4ka

Copy link
Copy Markdown
Collaborator Author

Thanks for the review. Everything is in; one commit per point:

  • 2px downscale: the frame is a box-shadow: 0 0 0 1px ring now (6b588d7), so a sized image renders at its natural width.
  • Design: sizing moved to an astro:build:done pass over the built HTML (667135d). That fixed the relative-URL bug, the stale cache and the n8n hand-coded sizes in one go.
  • The lastUpdated comment is back to main's text (7a44bbf).
  • The fragment check moved out to feat(audit): validate link anchors, not just link paths #1073 (b343460).

Checked: npm test 28/28; CI=1 npm run build passes and sizes 2,015 of 2,031 <img>; diffing dist/ against a main build, nothing changes except width/height on <img> and the one CSS rule. Still unsized: 6 external images, 8 dbt <img> with an author width and the two width="45%" in components/index.md, both left alone on purpose.

@keboola-pr-reviewer-bot keboola-pr-reviewer-bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verdict: needs_human (risk 4/5) · profile connection-docs

Escalating: this PR changes build/config and tooling, which the policy always routes to a human.

Suggested reviewers: @keboola/docs

@Iamfle4ka Iamfle4ka changed the title Size every image served from public/, and check link fragments in the audit Size every image served from public/, and fail CI on a missing one Oct 15, 2026

This branch was successfully deployed

1 active deployment
Preview — 7a1f7313 Deployed Oct 14, 2026 by vercel[bot]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

agent-needs-human agent-profile:connection-docs site-tooling Touches build config, CI, scripts or runtime code — the reviewer bot always routes these to a human

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants