Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
125 changes: 125 additions & 0 deletions ISSUE-1452-WRITEUP.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,125 @@
# Issue #1452 — GLSL tutorial renders as a thin vertical column

**Repo:** processing/p5.js-website · **Issue:** [#1452](https://github.com/processing/p5.js-website/issues/1452)
**Page affected:** `/tutorials/intro-to-glsl/` (and all rendered-markdown tutorial pages)
**Label:** Reserved (CodeDay) · **Assigned to:** Jose
**File at the center of it all:** [`styles/markdown.scss`](styles/markdown.scss)

---

## TL;DR

A framework upgrade silently swapped one working CSS line for an invalid one. That removed the
maximum-width cap on tutorial text, which collapsed the layout into an unreadable vertical column
of text. It stayed broken for ~3.5 weeks, then a *different* PR — aimed at a *different* symptom —
happened to replace the broken line with valid CSS and healed it without anyone realizing they'd
closed #1452.

**The bug is already fixed in the current code.** What was missing is a regression test to stop it
from ever coming back silently. That test is my contribution.

---

## Cast of characters (the one line that matters)

The rule in `styles/markdown.scss` that caps how wide tutorial text can grow:

| State | The line | Works? | Effect on the page |
|-------|----------|--------|--------------------|
| Healthy (before) | `@apply max-w-screen-md;` || Text capped at a readable column width |
| Broken | `@reference max-w-screen-md;` | ❌ (invalid in Tailwind v4) | No cap → layout collapses → vertical column |
| Fixed (now) | `max-width: variables.$breakpoint-tablet;` || Text capped at 770px |

**Why `@reference max-w-screen-md;` silently fails:** In Tailwind v4, `@reference` is only valid
when it points at a *file* (`@reference "app.css";`). Pointed at a *utility name* it is invalid, so
the browser simply discards the whole line — no error, no warning. It looks almost correct, which is
exactly why it slipped through code review.

---

## Timeline

### 1. Before — healthy
`styles/markdown.scss` used `@apply max-w-screen-md;` to cap the width of every block of tutorial
content. Tutorials rendered normally.

### 2. 2026-05-27 — THE BREAK · commit `adb24d3e4` "Upgrade Astro to v6"
A large framework upgrade (6,700+ lines touched: `package-lock.json`, `package.json`, config).
Buried inside it, one word changed in `styles/markdown.scss`:

```diff
- @apply max-w-screen-md;
+ @reference max-w-screen-md;
```

This was almost certainly an **automated codemod** run as part of the upgrade, not a deliberate
edit. The invalid line was dropped by the browser → tutorial text lost its max-width → the GLSL
tutorial (and others) collapsed into a thin vertical column. **This is issue #1452.**

### 3. 2026-05-27 → 2026-06-21 — the broken window (~3.5 weeks)
Tutorials were visibly broken on the beta branch this entire time. This matches the maintainer
(ksen0) report exactly: *"it was broken a couple weeks ago but looks fine now — why?"*

### 4. 2026-06-21 — THE ACCIDENTAL FIX · commit `3ed344be6` "Fix tutorial content too wide" (PR #1457)
A different contributor (eupthere) was addressing what looked like a **separate** problem — content
being too wide. Their fix replaced the broken line with real CSS:

```diff
- @reference max-w-screen-md;
+ max-width: variables.$breakpoint-tablet;
```

This restored the width cap, and the vertical-column bug disappeared **as a side effect**. The
author was not targeting #1452, so the two were never connected and #1452 was never formally closed
against this commit.

---

## Why it "broke two weeks ago but is fine today"

Because two unrelated commits, ~3.5 weeks apart, touched the same line:
- `adb24d3e4` (May 27) broke it via an upgrade codemod.
- `3ed344be6` (Jun 21) fixed it while chasing a different symptom.

The heal was incidental, which is why nobody could explain it — there was no PR that *said* it fixed
#1452.

---

## The real risk

Nothing today guarantees this stays fixed. The exact same failure mode could reappear the next time
a codemod or a hurried edit turns valid CSS into a silently-invalid `@reference <utility>;`. There
was **no test** guarding it. That is the gap.

---

## My contribution — the regression test

**File:** [`test/styles/markdown-scss.test.ts`](test/styles/markdown-scss.test.ts) (Vitest, 5 tests)

It compiles `styles/markdown.scss` with `sass` and asserts:
1. Rendered content has a real, positive `max-width` (not missing/zero).
2. That max-width equals `$breakpoint-tablet` (the intended 770px cap).
3. `.full-width` correctly opts out to a wider limit.
4. No stray `@reference` survives into the compiled CSS.
5. A source scan of `styles/` + `src/` fails on **any** `@reference <utility>;` — this guards the
entire bug *class*, not just this one line.

**Verified:** 5/5 pass on the current (fixed) code; 4/5 fail when pointed at the pre-fix broken file.
So it genuinely catches the regression rather than passing vacuously.

---

## What happens next (PR plan)

1. Re-run the test against current code to confirm 5/5 green.
2. Commit `test/styles/markdown-scss.test.ts` to branch `josecodeday`.
3. Open a PR to upstream (processing/p5.js-website) that:
- Adds the regression test.
- Links this writeup so maintainers get the root-cause explanation for ksen0's question.
- References #1452.
4. Share this writeup with the CodeDay teammate.

**Note:** the PR does **not** re-fix the bug (already fixed by #1457). It adds the missing guardrail
so the bug — and its whole class — can't silently return.
130 changes: 130 additions & 0 deletions test/styles/markdown-scss.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,130 @@
import { describe, expect, test } from "vitest";
import { readFileSync, readdirSync } from "node:fs";
import { join } from "node:path";
import * as sass from "sass";

/**
* Regression tests for processing/p5.js-website#1452
*
* The shader/GLSL tutorial (and every `.rendered-markdown` page) rendered as a
* thin, unreadable vertical column on the 2.0 / beta branch.
*
* Root cause: the Astro v6 upgrade (commit adb24d3e4, 2026-05-25) rewrote the
* width constraint in `styles/markdown.scss` from a valid utility
*
* @apply max-w-screen-md;
*
* to an invalid one
*
* @reference max-w-screen-md;
*
* `@reference` is a Tailwind v4 at-rule whose ONLY valid form is
* `@reference "some.css";` (it pulls theme context into a scoped stylesheet so
* that `@apply` works). Written as `@reference <utility>;` it is not a way to
* apply a utility — Sass passes it straight through as an unknown at-rule and
* the browser silently drops it. The result: `.rendered-markdown` content had
* NO `max-width`, so the layout collapsed. Commit 3ed344be6 (PR #1457,
* 2026-06-21) fixed it with an explicit `max-width: $breakpoint-tablet;`.
*
* These tests fail against the broken source and pass against the fix, and they
* guard the whole class of regression (any `@reference <utility>;` in any SCSS
* file), not just this one line.
*/

const MARKDOWN_SCSS = "styles/markdown.scss";

/** Collect every declaration block whose selector list matches `selectorPart`. */
const declarationsFor = (css: string, selectorPart: string): string[] => {
const blocks: string[] = [];
const ruleRe = /([^{}]+)\{([^{}]*)\}/g;
let match: RegExpExecArray | null;
while ((match = ruleRe.exec(css)) !== null) {
const [, selector, body] = match;
if (selector.includes(selectorPart)) blocks.push(body);
}
return blocks;
};

/** Recursively list every `.scss` file under the given roots. */
const scssFilesUnder = (roots: string[]): string[] => {
const out: string[] = [];
for (const root of roots) {
for (const entry of readdirSync(root, {
recursive: true,
withFileTypes: true,
})) {
if (entry.isFile() && entry.name.endsWith(".scss")) {
out.push(join(entry.parentPath ?? entry.path, entry.name));
}
}
}
return out;
};

describe("markdown.scss width constraints (#1452 regression)", () => {
const { css } = sass.compile(MARKDOWN_SCSS);

test("top-level rendered-markdown content has a real max-width", () => {
// `.rendered-markdown > *` is what constrains prose/code width. If this loses
// its max-width the tutorial collapses into the reported vertical column.
const blocks = declarationsFor(css, ".rendered-markdown > *");
expect(blocks.length).toBeGreaterThan(0);

const maxWidths = blocks
.map((b) => b.match(/max-width:\s*([^;]+);/)?.[1]?.trim())
.filter(Boolean) as string[];

expect(maxWidths.length).toBeGreaterThan(0);
// Must be a concrete positive length, not `0`, `none`, or a dropped value.
for (const value of maxWidths) {
const px = Number.parseFloat(value);
expect(px).toBeGreaterThan(320);
}
});

test("max-width matches the $breakpoint-tablet variable", () => {
const variables = readFileSync("styles/variables.scss", "utf-8");
const tablet = variables.match(/\$breakpoint-tablet:\s*(\d+)px/)?.[1];
expect(tablet, "expected $breakpoint-tablet in variables.scss").toBeTruthy();

const [body] = declarationsFor(css, ".rendered-markdown > *");
expect(body).toContain(`max-width: ${tablet}px`);
});

test("full-width override still opts out to a wider max-width", () => {
const [body] = declarationsFor(css, ".rendered-markdown > .full-width");
const value = body?.match(/max-width:\s*([^;]+);/)?.[1]?.trim();
expect(Number.parseFloat(value ?? "0")).toBeGreaterThan(770);
});

test("compiled CSS contains no stray @reference at-rules", () => {
// A valid `@reference "file";` lives in scoped Astro <style> blocks, never
// in this compiled global stylesheet. Any @reference here means a utility
// was mis-applied and silently dropped (the original bug).
expect(css).not.toMatch(/@reference/);
});
});

describe("no invalid @reference utility usage in SCSS (#1452 root cause)", () => {
test("@reference is only ever used with a quoted stylesheet path", () => {
const offenders: string[] = [];
for (const file of scssFilesUnder(["styles", "src"])) {
const contents = readFileSync(file, "utf-8");
const lines = contents.split("\n");
lines.forEach((line, i) => {
const ref = line.match(/@reference\s+([^;]+);/);
if (!ref) return;
const arg = ref[1].trim();
// Valid: @reference "…"; or @reference '…'; Invalid: @reference max-w-screen-md;
const isQuotedPath = /^["'].*["']$/.test(arg);
if (!isQuotedPath) {
offenders.push(`${file}:${i + 1} @reference ${arg};`);
}
});
}
expect(
offenders,
`Found @reference used as a utility (invalid — Tailwind drops it, breaking layout):\n${offenders.join("\n")}`,
).toEqual([]);
});
});