Fix overlapping text and improve Arabic rendering in PDFs

Update PDF rendering logic to use fontkit for Arabic shaping and RTL reordering, eliminating pre-shaping and resolving text overlap issues.

Replit-Commit-Author: Agent
Replit-Commit-Session-Id: 77cfe984-2c65-4152-bb7a-0df28274fe66
Replit-Commit-Checkpoint-Type: full_checkpoint
Replit-Commit-Event-Id: 32b8a89e-ae1f-4a09-ae50-3a53ae3f3477
Replit-Commit-Screenshot-Url: https://storage.googleapis.com/screenshot-production-us-central1/c3c252e4-c83d-40ca-9fff-99a3ea60701e/77cfe984-2c65-4152-bb7a-0df28274fe66/Ow4s0aa
Replit-Helium-Checkpoint-Created: true

artifacts/api-server/src/lib/pdf-renderer.ts

@@ -17,15 +17,17 @@ const { ArabicShaper } = reshaperRequire("arabic-persian-reshaper") as {
 };
 
 // Convert base Arabic letters (U+0600..U+06FF) to their contextual
-// presentation forms (Arabic Presentation Forms-A/B). PDFKit has no
-// built-in OpenType layout / HarfBuzz, so without this every Arabic
-// character renders in its isolated form and — combined with the bidi
-// visual reorder — the printed text looks both disconnected and
-// reversed (e.g. "الاجتماعات" prints as "تاعامتجالا"). Must run BEFORE
-// `bidi.getReorderedString`: shaping needs the original logical order
-// to know each letter's neighbours, and the resulting presentation
-// forms are still classified as RTL so bidi reorders them correctly.
-// Idempotent on text that contains only presentation forms / Latin.
+// presentation forms (Arabic Presentation Forms-A/B).
+//
+// HISTORICAL NOTE: this used to run unconditionally before drawing,
+// because the original PDFKit (without fontkit GSUB features) had no
+// shaper. The current renderer hands raw Unicode + ARABIC_FEATURES to
+// pdfkit so fontkit performs shaping AND visual reorder itself; doing
+// both this pre-shape AND the in-pdfkit shaping double-processes the
+// string and renders it visually mirrored. The function is retained
+// only as a public export for downstream callers (tests, ad-hoc
+// scripts) that explicitly want pre-shaped output. The renderer no
+// longer calls it.
 export function shapeArabic(text: string): string {
   if (!text) return text;
   // Fast skip: nothing to shape if there are no base-form Arabic chars.
@@ -264,6 +266,25 @@ function isLatinChar(ch: string): boolean {
 
 type Run = { text: string; isArabic: boolean };
 
+// OpenType feature tags fontkit needs to (a) shape Arabic into contextual
+// joining forms and (b) reorder Arabic glyphs visually right-to-left
+// inside a single LTR draw call. We pass these via PDFKit's `features`
+// option whenever we draw a run that contains Arabic — pdfkit forwards
+// them straight to fontkit's GSUB layout. Crucially, this makes pdfkit
+// the SOLE owner of shaping + RTL reorder; if we also pre-shape or
+// pre-reorder the string ourselves, fontkit applies a second pass and
+// the result is visually mirrored (the bug this module used to ship).
+const ARABIC_FEATURES: PDFKit.Mixins.OpenTypeFeatures[] = [
+  "rtla", // right-to-left alternates: flips digits and mirrored glyphs
+  "rclt", // required contextual alternates (Arabic joining)
+  "calt", // contextual alternates
+  "liga", // standard ligatures (lam-alef, etc.)
+  "init",
+  "medi",
+  "fina",
+  "isol",
+];
+
 // Split a string into runs of consecutive same-script characters so we
 // can pick the Arabic font for Arabic glyphs and the Latin font for the
 // rest. Whitespace sticks to the surrounding run to avoid a stream of
@@ -293,16 +314,54 @@ function splitRunsByScript(text: string): Run[] {
   return runs;
 }
 
-// Apply Unicode Bidi to a string and return runs in *visual* order (which
-// is the order PDFKit will paint them along the line). Without this, mixed
-// Arabic/Latin/digit cells look wrong (e.g. "10:00 - 11:00" inside an
-// otherwise-RTL line gets digit-reversed by the renderer).
+// Build draw-order runs for a single line of mixed Arabic/Latin text.
+//
+// Returns an array of runs whose ORDER matches the visual left-to-right
+// painting order. For each run, `text` is the *raw, logical-order*
+// substring (Arabic in U+06xx code points, NOT pre-shaped presentation
+// forms) so that pdfkit/fontkit can run shaping + intra-run RTL reorder
+// itself via the `features` option when we draw.
+//
+// Algorithm:
+//   1. Compute Unicode Bidi embedding levels on the raw text.
+//   2. Walk the text in LOGICAL order, slicing at every level boundary.
+//      Each slice is internally consistent direction-wise, so fontkit can
+//      reorder it correctly with `rtla` features.
+//   3. If the paragraph base direction is RTL, REVERSE the run array.
+//      For RTL bases the visual L-to-R sequence is the logical sequence
+//      read backwards (with each LTR-level slice keeping its own LTR
+//      internal order — which is exactly what we want).
 function visualOrderRuns(text: string, baseDirection: "ltr" | "rtl"): Run[] {
   if (!text) return [];
-  const shaped = shapeArabic(text);
-  const embeddingLevels = bidi.getEmbeddingLevels(shaped, baseDirection);
-  const reordered = bidi.getReorderedString(shaped, embeddingLevels);
-  return splitRunsByScript(reordered);
+  const levels = bidi.getEmbeddingLevels(text, baseDirection);
+  const lvlArr = levels.levels as ArrayLike<number>;
+  // bidi-js returns one level per code unit — text and lvlArr share length.
+  const out: Run[] = [];
+  let i = 0;
+  const n = text.length;
+  while (i < n) {
+    const startLevel = lvlArr[i];
+    let j = i + 1;
+    while (j < n && lvlArr[j] === startLevel) j++;
+    const slice = text.slice(i, j);
+    // Detect Arabic content for font + features selection. A level-1+
+    // odd run is RTL but might still be punctuation/digits; only true
+    // Arabic chars need the Arabic font and shaping features.
+    const hasArabic = [...slice].some(isArabicChar);
+    out.push({ text: slice, isArabic: hasArabic });
+    i = j;
+  }
+  if (baseDirection === "rtl") out.reverse();
+  return out;
+}
+
+// Returns the pdfkit `features` option appropriate for a run. We want
+// shaping ON for any run containing Arabic glyphs and OFF otherwise so
+// Latin runs (e.g. "9:00 — 10:00") don't get treated as RTL.
+function featuresFor(
+  isArabic: boolean,
+): PDFKit.Mixins.OpenTypeFeatures[] | undefined {
+  return isArabic ? ARABIC_FEATURES : undefined;
 }
 
 function fontKeyFor(
@@ -412,14 +471,16 @@ function drawMixedLine(
   if (runs.length === 0) {
     return opts.fontSize * 1.2;
   }
-  // Measure full line width.
+  // Measure full line width — feature flags must match the draw call so
+  // ligatures + RTL alternates produce the same advance width.
   let totalWidth = 0;
   const measured = runs.map((r) => {
     const fk = fontKeyFor(r.isArabic, opts.weight, opts.mapping);
+    const features = featuresFor(r.isArabic);
     doc.font(fk).fontSize(opts.fontSize);
-    const w = doc.widthOfString(r.text);
+    const w = doc.widthOfString(r.text, features ? { features } : {});
     totalWidth += w;
-    return { ...r, width: w, fontKey: fk };
+    return { ...r, width: w, fontKey: fk, features };
   });
   const overflow = totalWidth - opts.width;
   let cursorX = opts.x;
@@ -439,6 +500,7 @@ function drawMixedLine(
     doc.text(r.text, cursorX, opts.y, {
       lineBreak: false,
       width: r.width + 1,
+      ...(r.features ? { features: [...r.features] } : {}),
     });
     cursorX += r.width;
   }
@@ -458,19 +520,17 @@ function drawWrappingLine(
   const dominantArabic = arabicChars * 2 > text.length;
   const fk = fontKeyFor(dominantArabic, opts.weight, opts.mapping);
   doc.font(fk).fontSize(opts.fontSize).fillColor(opts.color ?? "black");
-  // For Arabic text, shape contextual joining forms first, then run bidi
-  // so the font sees runs already in visual order — this matches what
-  // drawMixedLine does for single-line cells.
-  const shaped = shapeArabic(text);
-  const reordered = bidi.getReorderedString(
-    shaped,
-    bidi.getEmbeddingLevels(shaped, opts.baseDirection),
-  );
+  // Hand the RAW (logical-order) string to pdfkit and let fontkit do
+  // both Arabic shaping and intra-line RTL reorder via the `features`
+  // option. Pre-shaping here would double-process the string and
+  // produce visually-mirrored output (the bug we shipped previously).
+  const features = featuresFor(dominantArabic);
   const before = doc.y;
-  doc.text(reordered, opts.x, opts.y, {
+  doc.text(text, opts.x, opts.y, {
     width: opts.width,
     align: opts.align,
     lineBreak: true,
+    ...(features ? { features } : {}),
   });
   return Math.max(opts.fontSize * 1.25, doc.y - before + 2);
 }
@@ -613,15 +673,13 @@ export async function renderSchedulePdf(input: RenderPdfInput): Promise<Buffer>
       const dominantArabic = arabicChars * 2 > label.length;
       const fk = fontKeyFor(dominantArabic, "bold", mapping);
       doc.font(fk).fontSize(input.font.fontSize).fillColor("white");
-      const shapedLabel = shapeArabic(label);
-      const reordered = bidi.getReorderedString(
-        shapedLabel,
-        bidi.getEmbeddingLevels(shapedLabel, baseDir),
-      );
-      doc.text(reordered, cx + cellPadX, headerY + cellPadY, {
+      // Raw label + Arabic features → fontkit shapes & reorders.
+      const features = featuresFor(dominantArabic);
+      doc.text(label, cx + cellPadX, headerY + cellPadY, {
         width: headerWidths[i] - cellPadX * 2,
         align: "center",
         lineBreak: false,
+        ...(features ? { features } : {}),
       });
       cx += headerWidths[i];
     }
@@ -695,27 +753,41 @@ export async function renderSchedulePdf(input: RenderPdfInput): Promise<Buffer>
       const c = cells[i];
       const startProbeY = probeY + cellPadY;
       probe.y = startProbeY;
-      // Reuse drawWrappingLine to compute height — drop the actual draw
-      // by using a temporary off-screen page would be ideal, but PDFKit
-      // doesn't expose an undo. We instead measure by counting expected
-      // wraps at the dominant font.
+      // Use PDFKit's own layout engine (heightOfString) so the probe
+      // reserves EXACTLY as much vertical space as the eventual draw
+      // call will consume — passing the same font, features, width,
+      // and align as drawWrappingLine. A heuristic ceil(w/availableW)
+      // probe under-counts whenever pdfkit's word-break engine wraps
+      // sooner than a simple division would, causing rows to overlap.
       const lines = c.text.split("\n");
       let h = 0;
       const arabicChars = [...c.text].filter(isArabicChar).length;
       const dominantArabic = arabicChars * 2 > c.text.length;
       const fk = fontKeyFor(dominantArabic, input.font.fontWeight, mapping);
+      const features = featuresFor(dominantArabic);
       doc.font(fk).fontSize(input.font.fontSize);
+      const measureOpts: PDFKit.Mixins.TextOptions = {
+        width: widths[i] - cellPadX * 2,
+        align: c.align,
+        lineBreak: true,
+        ...(features ? { features } : {}),
+      };
       for (const line of lines) {
         if (!line) {
           h += lineHeight;
           continue;
         }
-        // Measure with shaped text — presentation forms are typically
-        // narrower than chained isolated glyphs, so probing the raw
-        // string would over-count wraps and reserve excess row height.
-        const w = doc.widthOfString(shapeArabic(line));
-        const wraps = Math.max(1, Math.ceil(w / (widths[i] - cellPadX * 2)));
-        h += wraps * lineHeight;
+        // heightOfString respects width + align + features and returns
+        // the layout height pdfkit would use in text(); we round up to
+        // a whole lineHeight so the probe matches the +2 padding we
+        // add inside drawWrappingLine.
+        const measured = doc.heightOfString(line, measureOpts);
+        const wraps = Math.max(1, Math.ceil(measured / lineHeight));
+        // drawWrappingLine returns max(lineHeight, wraps*lineHeight + 2)
+        // PER non-empty line, so each non-empty line consumes that much
+        // vertical space. Mirror it here exactly so multi-line cells
+        // (e.g. attendees split by \n) don't get under-reserved.
+        h += wraps * lineHeight + 2;
       }
       const finalH = h + cellPadY * 2;
       cellHeights.push(finalH);

artifacts/api-server/tests/executive-meetings.test.mjs

@@ -1024,15 +1024,19 @@ test("PDF content: title label, rowColor tint, dropped isHighlighted, fontColor"
   void ascii;
 });
 
-test("PDF Arabic shaping: title renders in connected presentation forms (not isolated)", async () => {
+test("PDF Arabic shaping: title renders in connected, correctly-ordered Arabic", async () => {
   // Regression for the bug where Arabic in the printed PDF appeared as
-  // disconnected, visually-reversed letters (e.g. "الاجتماعات" →
-  // "تاعامتجالا"). PDFKit has no built-in HarfBuzz, so we now run a
-  // pre-shaping pass that converts base Arabic letters (U+0600..U+06FF)
-  // to their contextual presentation forms (FE80..FEFF, plus FEFB
-  // lam-alef ligature) BEFORE handing text to bidi + PDFKit. We assert
-  // the rendered PDF contains presentation-form codepoints in its
-  // embedded ToUnicode CMap (proves shaped glyphs were drawn).
+  // disconnected, visually-reversed letters (e.g. "قائمة حضور" →
+  // "ةمئاقروضح"). The renderer now hands RAW Unicode (logical order,
+  // U+0600..06FF base letters) to PDFKit, which forwards it to fontkit
+  // along with `features: ['rtla', 'rclt', 'calt', 'liga', ...]`.
+  // Fontkit performs shaping AND visual right-to-left reorder at the
+  // glyph layer, so the PDF content stream contains glyph IDs, not
+  // characters. Per PDF 1.7 §9.10, every embedded TrueType font carries
+  // a ToUnicode CMap that maps each emitted glyph back to its LOGICAL
+  // base codepoint — that is what we assert here. (Pre-shaped output
+  // would put U+FExx presentation-form codepoints in the CMap; the
+  // current pipeline must NOT do that.)
 
   // End-to-end: render a real PDF and assert the inflated streams
   // (which include the embedded ToUnicode CMap) reference at least one
@@ -1081,13 +1085,25 @@ test("PDF Arabic shaping: title renders in connected presentation forms (not iso
     cursor = end + "\nendstream".length;
   }
   const allDecoded = decoded.join("\n");
-  // Match any FE80..FEFF or FB50..FDFF hex literal — these are the
-  // Arabic Presentation Forms ranges. Without shaping none would appear.
-  const presFormsRegex = /<(?:FE[89A-F][0-9A-F]|FEF[0-9A-F]|FE[7][0-9A-F]|FB[5-9A-F][0-9A-F]|FC[0-9A-F]{2}|FD[0-9A-F]{2})>/i;
+  // Assert (a) the embedded ToUnicode CMap references at least one base
+  // Arabic codepoint (U+0600..06FF) — proves Arabic content was drawn
+  // and survived round-tripping — and (b) the CMap does NOT reference
+  // any Arabic Presentation Forms (FE70..FEFF / FB50..FDFF). The
+  // presence of presentation-form codepoints in the CMap would mean
+  // someone re-introduced the pre-shaping pass and the PDF would
+  // render visually mirrored again.
+  const baseArabicRegex = /<(?:06[0-9A-F]{2})>/i;
   assert.match(
     allDecoded,
-    presFormsRegex,
-    "rendered PDF must reference Arabic Presentation Forms codepoints (proves shaping ran before PDFKit)",
+    baseArabicRegex,
+    "rendered PDF must reference base Arabic codepoints in its ToUnicode CMap",
+  );
+  const presFormsRegex = /<(?:FE[789A-F][0-9A-F]|FB[5-9A-F][0-9A-F]|FC[0-9A-F]{2}|FD[0-9A-F]{2})>/i;
+  const presMatch = allDecoded.match(presFormsRegex);
+  assert.equal(
+    presMatch,
+    null,
+    `rendered PDF must NOT reference Arabic Presentation Forms in its ToUnicode CMap — found ${presMatch?.[0]} (regression: pre-shaping has been re-introduced and the PDF will render visually mirrored)`,
   );
 });