Skip to content

Commit 5ba5e03

Browse files
committed
fix: address PR review comments on table rendering
- Clone preset styles in resolveTableStyle to prevent mutation of shared TABLE_STYLES singletons across renders - Use local pageBg from doc.theme.bg instead of caching stale _pageBg on the style object; remove _pageBg from TableStyle interface - Use validated style.bodyFg for row text color with autoTextColor fallback, fixing dead code where bodyFg was corrected but never used - Enable PDF visual tests on Windows via WSL pdftoppm - Add install-pdf-deps Justfile recipe for Linux and Windows/WSL - Regenerate golden baselines to match bodyFg rendering change Signed-off-by: Simon Davies <simongdavies@users.noreply.github.com>
1 parent e20c6b8 commit 5ba5e03

13 files changed

Lines changed: 254 additions & 790 deletions

File tree

.github/workflows/pr-validate.yml

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -74,9 +74,7 @@ jobs:
7474
GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}
7575

7676
- name: Install PDF test dependencies
77-
run: |
78-
sudo apt-get update -qq
79-
sudo apt-get install -y -qq poppler-utils qpdf fonts-dejavu-core
77+
run: just install-pdf-deps
8078

8179
- name: Setup
8280
run: just setup

.github/workflows/update-golden.yml

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -60,9 +60,7 @@ jobs:
6060
GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}
6161

6262
- name: Install PDF test dependencies
63-
run: |
64-
sudo apt-get update -qq
65-
sudo apt-get install -y -qq poppler-utils qpdf fonts-dejavu-core
63+
run: just install-pdf-deps
6664

6765
- name: Setup
6866
run: just setup

Justfile

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -256,6 +256,20 @@ fmt-all: fmt fmt-analysis-guest fmt-runtime
256256
test-all: test test-analysis-guest
257257
@echo "✅ All tests passed"
258258

259+
# Pinned versions for reproducible golden baselines.
260+
# Update ALL THREE locations together: Justfile, pr-validate.yml, update-golden.yml
261+
PDF_DEPS := "poppler-utils=24.02.0-1ubuntu9.8 qpdf=11.9.0-1.1ubuntu0.1 fonts-dejavu-core=2.37-8"
262+
263+
# Install PDF visual test dependencies (poppler-utils + fonts-dejavu-core).
264+
# On Windows, installs into WSL. On Linux, installs natively.
265+
[linux]
266+
install-pdf-deps:
267+
sudo apt-get update -qq && sudo apt-get install -y -qq {{PDF_DEPS}}
268+
269+
[windows]
270+
install-pdf-deps:
271+
wsl bash -c "sudo apt-get update -qq && sudo apt-get install -y -qq {{PDF_DEPS}}"
272+
259273
# PDF visual regression tests
260274
test-pdf-visual:
261275
npx vitest run tests/pdf-visual.test.ts

builtin-modules/pdf.json

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -3,8 +3,8 @@
33
"description": "PDF 1.7 document generation — text, graphics, metadata, standard fonts. Flow-based layout for auto-paginating documents.",
44
"author": "system",
55
"mutable": false,
6-
"sourceHash": "sha256:e6e604e51302ea45",
7-
"dtsHash": "sha256:f30fba88bfe5f977",
6+
"sourceHash": "sha256:fedd6d3bc7c638e8",
7+
"dtsHash": "sha256:e56bc4be1f1d0cd1",
88
"importStyle": "named",
99
"hints": {
1010
"overview": "Generate PDF documents with text, shapes, and metadata. Uses PDF's 14 standard fonts (no embedding required). Coordinates are in points (72 points = 1 inch), with top-left origin.",

builtin-modules/src/pdf.ts

Lines changed: 12 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -3015,8 +3015,6 @@ export interface TableStyle {
30153015
borderColor: string;
30163016
/** Border line width in points. */
30173017
borderWidth: number;
3018-
/** Page background colour (set internally for contrast checking). */
3019-
_pageBg?: string;
30203018
}
30213019

30223020
/** Built-in table styles matching PPTX table styles. */
@@ -3378,9 +3376,11 @@ export function comparisonTable(opts: ComparisonTableOptions): PdfElement {
33783376
return _createPdfElement("comparisonTable", data);
33793377
}
33803378

3381-
/** Resolve a style name or object to a TableStyle. */
3379+
/** Resolve a style name or object to a TableStyle.
3380+
* Preset styles are shallow-cloned so that contrast auto-correction
3381+
* in renderTable never mutates the shared TABLE_STYLES singletons. */
33823382
function resolveTableStyle(style?: string | TableStyle): TableStyle {
3383-
if (!style) return TABLE_STYLES.default;
3383+
if (!style) return { ...TABLE_STYLES.default };
33843384
if (typeof style === "string") {
33853385
const resolved = TABLE_STYLES[style];
33863386
if (!resolved) {
@@ -3389,7 +3389,7 @@ function resolveTableStyle(style?: string | TableStyle): TableStyle {
33893389
`Unknown table style "${style}". Valid styles: ${valid}.`,
33903390
);
33913391
}
3392-
return resolved;
3392+
return { ...resolved };
33933393
}
33943394
return style;
33953395
}
@@ -3487,16 +3487,13 @@ function renderTable(
34873487
const rowH = tableRowHeight(fontSize, compact);
34883488
const headerH = rowH;
34893489

3490-
// Ensure page background is available for contrast checking
3491-
if (!style._pageBg) {
3492-
style._pageBg = doc.theme.bg;
3493-
}
3494-
34953490
// ── Contrast auto-correction ─────────────────────────────────────
34963491
// Automatically fix text colors that have poor contrast against the
34973492
// page background. No errors — just silently correct to readable.
3493+
// Use a local pageBg derived from the current doc theme rather than
3494+
// caching on the style object (which could be stale across renders).
34983495
const MIN_CONTRAST = 3.0;
3499-
const pageBg = style._pageBg || "FFFFFF";
3496+
const pageBg = doc.theme.bg || "FFFFFF";
35003497

35013498
// If headerBg is too similar to pageBg, the header row won't stand out.
35023499
// Swap to theme accent1 so the header is visually distinct.
@@ -3652,11 +3649,12 @@ function renderTable(
36523649

36533650
// EVERY row gets an explicit fill — no transparent rows, no guessing
36543651
const isAlt = !!(style.altRowBg && r % 2 === 1);
3655-
const rowBg = isAlt ? style.altRowBg : (style._pageBg || "FFFFFF");
3652+
const rowBg = isAlt ? style.altRowBg : pageBg;
36563653
doc.drawRect(x, curY, totalWidth, rowH, { fill: rowBg });
36573654

3658-
// Text color computed against the ACTUAL fill we just drew
3659-
const rowFg = autoTextColor(rowBg);
3655+
// Prefer the validated body text color; fall back to an automatic
3656+
// contrast color if no explicit body foreground is configured.
3657+
const rowFg = style.bodyFg ?? autoTextColor(rowBg);
36603658

36613659
// Cell text AFTER background
36623660
const isBoldRow = rowBold?.[r] ?? false;

builtin-modules/src/types/ha-modules.d.ts

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1102,8 +1102,6 @@ declare module "ha:pdf" {
11021102
borderColor: string;
11031103
/** Border line width in points. */
11041104
borderWidth: number;
1105-
/** Page background colour (set internally for contrast checking). */
1106-
_pageBg?: string;
11071105
}
11081106
/** Built-in table styles matching PPTX table styles. */
11091107
export declare const TABLE_STYLES: Record<string, TableStyle>;

0 commit comments

Comments
 (0)