Skip to content

Commit 2f17a09

Browse files
committed
feat(FR-2719): address Copilot review feedback
- pdf-renderer.ts: fix RenderOptions docstring (uses page, not context), move tmpDir creation after browser launch and guard cleanup so a failed chromium.launch() cannot leak a temp directory. - pdf-renderer.ts: extend DEFAULT_CJK_FONT_CANDIDATES with single-face paths for Japanese (TakaoGothic) and Thai (Loma.otf, Garuda) so the apt-installed fonts in the workflow are actually picked up. - generate-pdf.ts: normalize args.concurrency through Number.isFinite so a NaN input does not silently produce zero workers. - package.yml: drop job-level continue-on-error so an all-languages failure (which generate-pdf.ts surfaces via non-zero exit) still fails the job — partial failures continue to degrade gracefully inside the script. - upload-release.js: usage text now mentions PDF as an accepted asset type alongside DMG and ZIP.
1 parent dc3c902 commit 2f17a09

4 files changed

Lines changed: 36 additions & 17 deletions

File tree

.github/workflows/package.yml

Lines changed: 8 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -217,14 +217,16 @@ jobs:
217217
# and build_desktop, so a healthy run does not extend the overall wall
218218
# clock (mac is the current critical path at ~10 min).
219219
#
220-
# `continue-on-error: true` means a PDF failure logs warnings but does
221-
# NOT block the release. Tighten this once the pipeline has soaked.
220+
# Failure handling is scoped to the PDF build step itself (not the whole
221+
# job), so partial per-language failures degrade gracefully but a full
222+
# generator failure (all languages failed → generate-pdf.ts exits non-
223+
# zero) still fails the job. This prevents shipping a release with zero
224+
# PDF assets going unnoticed.
222225
# ──────────────────────────────────────────────────────────────────────
223226
build_docs:
224227
permissions:
225228
contents: write
226229
runs-on: ubuntu-latest
227-
continue-on-error: true
228230
steps:
229231
- name: Check out Git repository
230232
uses: actions/checkout@v5
@@ -261,9 +263,9 @@ jobs:
261263
# ships only a TTC bundle that pdf-lib/fontkit cannot embed, so we
262264
# use language-specific single-file packages whose paths are listed
263265
# in DEFAULT_CJK_FONT_CANDIDATES (pdf-renderer.ts):
264-
# - fonts-nanum → /usr/share/fonts/truetype/nanum/NanumBarunGothic.ttf (ko)
265-
# - fonts-takao-gothic → /usr/share/fonts/truetype/takao-gothic/TakaoGothic.ttf (ja)
266-
# - fonts-thai-tlwg → /usr/share/fonts/truetype/tlwg/Loma.ttf (th)
266+
# - fonts-nanum → /usr/share/fonts/truetype/nanum/NanumBarunGothic.ttf (ko)
267+
# - fonts-takao-gothic → /usr/share/fonts/truetype/takao-gothic/TakaoGothic.ttf (ja)
268+
# - fonts-thai-tlwg → /usr/share/fonts/opentype/tlwg/Loma.otf or Garuda.ttf (th)
267269
# Languages whose font is missing will be reported as a failure by
268270
# generate-pdf.ts but will not abort the other language renders.
269271
- name: Install CJK + Thai fonts (best effort)

packages/backend.ai-docs-toolkit/src/generate-pdf.ts

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -96,9 +96,12 @@ export async function generatePdf(
9696
process.env.BAI_DOCS_PDF_CONCURRENCY ?? '',
9797
10,
9898
);
99+
const optionConcurrency = Number.isFinite(args.concurrency)
100+
? args.concurrency
101+
: undefined;
99102
const concurrency = Math.max(
100103
1,
101-
args.concurrency ?? (Number.isFinite(envConcurrency) ? envConcurrency : 2),
104+
optionConcurrency ?? (Number.isFinite(envConcurrency) ? envConcurrency : 2),
102105
);
103106

104107
const productName = config.productName;

packages/backend.ai-docs-toolkit/src/pdf-renderer.ts

Lines changed: 23 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -19,10 +19,17 @@ const DEFAULT_CJK_FONT_CANDIDATES = [
1919
path.join(os.homedir(), 'Library/Fonts/NanumBarunGothic.ttf'),
2020
path.join(os.homedir(), 'Library/Fonts/NanumSquareRegular.ttf'),
2121
'/System/Library/Fonts/Supplemental/Arial Unicode.ttf',
22-
// Linux common paths
22+
// Linux common paths — Korean
2323
'/usr/share/fonts/opentype/noto/NotoSansCJK-Regular.ttc',
2424
'/usr/share/fonts/truetype/noto/NotoSansKR-Regular.ttf',
2525
'/usr/share/fonts/truetype/nanum/NanumBarunGothic.ttf',
26+
// Linux common paths — Japanese (fonts-takao-gothic on Debian/Ubuntu)
27+
'/usr/share/fonts/truetype/takao-gothic/TakaoGothic.ttf',
28+
'/usr/share/fonts/truetype/takao-mincho/TakaoMincho.ttf',
29+
// Linux common paths — Thai (fonts-thai-tlwg on Debian/Ubuntu)
30+
'/usr/share/fonts/truetype/tlwg/Loma.ttf',
31+
'/usr/share/fonts/opentype/tlwg/Loma.otf',
32+
'/usr/share/fonts/truetype/tlwg/Garuda.ttf',
2633
];
2734

2835
type EmbeddedFont = Awaited<ReturnType<PDFDocument['embedFont']>>;
@@ -63,9 +70,9 @@ export interface RenderOptions {
6370
config?: ResolvedDocConfig;
6471
/**
6572
* Optional shared Chromium instance. When provided, renderPdf reuses it
66-
* (via newContext/newPage) and the caller owns its lifecycle. This avoids
67-
* paying the per-call launch cost when generating multiple PDFs in one run
68-
* (e.g. one PDF per language during release builds).
73+
* by opening a new page on the existing browser, and the caller owns its
74+
* lifecycle. This avoids paying the per-call launch cost when generating
75+
* multiple PDFs in one run (e.g. one PDF per language during release builds).
6976
*/
7077
browser?: Browser;
7178
}
@@ -389,17 +396,22 @@ export async function renderPdf(options: RenderOptions): Promise<void> {
389396

390397
fs.mkdirSync(path.dirname(options.outputPath), { recursive: true });
391398

392-
const tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'bai-docs-'));
393-
const tmpHtmlPath = path.join(tmpDir, 'document.html');
394-
fs.writeFileSync(tmpHtmlPath, options.html, 'utf-8');
395-
396399
const ownsBrowser = !options.browser;
397400
const browser = options.browser ?? (await chromium.launch());
401+
402+
// tmpDir is created after the browser is ready so a chromium.launch()
403+
// failure cannot leave an orphaned temp directory behind. Use an outer
404+
// try/finally so a failure between mkdtempSync and the inner try also
405+
// cleans up the browser when we own it.
406+
let tmpDir: string | undefined;
398407
let pdfBuffer: Buffer;
399408
let chapterInfoList: ChapterInfo[] = [];
400409
let sectionList: SectionInfo[] = [];
401410
let page: Awaited<ReturnType<Browser['newPage']>> | undefined;
402411
try {
412+
tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'bai-docs-'));
413+
const tmpHtmlPath = path.join(tmpDir, 'document.html');
414+
fs.writeFileSync(tmpHtmlPath, options.html, 'utf-8');
403415
page = await browser.newPage({
404416
viewport: { width: PRINT_WIDTH_PX, height: 800 },
405417
});
@@ -494,7 +506,9 @@ export async function renderPdf(options: RenderOptions): Promise<void> {
494506
if (ownsBrowser) {
495507
await browser.close();
496508
}
497-
fs.rmSync(tmpDir, { recursive: true, force: true });
509+
if (tmpDir) {
510+
fs.rmSync(tmpDir, { recursive: true, force: true });
511+
}
498512
}
499513

500514
// ── Post-processing ──────────────────────────────────────────

upload-release.js

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -47,7 +47,7 @@ const getUploadURL = async (releaseId) => {
4747

4848
const main = async () => {
4949
if (process.argv.length !== 3) {
50-
console.error('usage: node upload-release.js <folder containing DMG/ZIP files>')
50+
console.error('usage: node upload-release.js <folder containing DMG/ZIP/PDF files>')
5151
process.exit(1)
5252
}
5353
const folder = process.argv[2]

0 commit comments

Comments
 (0)