fix(core): keep cursive words joined across a run face change - #556
Conversation
Joining_Type decides whether two characters were cursively connected. Almost all Transparent characters are absent from ArabicShaping.txt and inherit the value from General_Category Mn/Me/Cf in a separate file, so a hand-written table would misclassify every Arabic harakat and the failure would surface as mis-shaped text rather than a test failure. Derive the table from both pinned UCD sources instead, commit the generated output, and wire a check mode so a Unicode bump cannot land half-applied.
Query whether a boundary severed a cursive connection, skipping transparent marks on both sides. Gated by hasCursiveLetter so the Latin path is untouched. The property pinned by tests is the one most likely to regress: any number of combining marks at the boundary must not change the verdict, because vocalized Arabic puts them exactly where a run boundary lands.
Browsers shape across an inline box boundary only while no shaping-relevant property changes, so colour and underline marks already join. A face change does not: bold, italic, a different size or family selects another font and shaping stops, leaving an Arabic word split mid-word rendered as isolated forms. Word joins straight through the same boundary. Insert a zero-width joiner on each side, each in its own span wearing that side's styles. Keeping the joiner out of the run's own text node is what makes this safe: pm-position spans map DOM text offsets back to document positions, so growing a run's text would desync every offset after it. Face equality is decided by running applyRunStyles over a probe element and diffing the result, so there is no second copy of the font decisions to drift from the painter's. An unrecognised property counts as a face change, which adds a harmless joiner rather than silently skipping a repair.
Folio measures text with canvas and paints it as DOM text the browser shapes itself. Two engines, and nothing compared their outputs: they could disagree while every unit test passed, which is exactly what happened. Stamp the measurer's width on each painted line and compare it against a Range over the line's content. Both numbers come from one layout pass in the same browser, so unlike a screenshot baseline this cannot go flaky on cross-machine font rendering and is safe to gate CI on. The comparison is narrowed to what MeasuredLine.width is defined to cover: content runs, excluding list markers and trailing whitespace at a soft wrap, both of which are painted but deliberately outside the measured number. Counts are pinned rather than asserted non-zero, so neither a false positive nor a repair that stops happening can pass unnoticed.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds Unicode joining-table generation and cursive boundary utilities. Paragraph rendering inserts styled zero-width joiner spans across affected cursive run boundaries. Unit and Playwright tests validate joining behaviour, measurement parity, and document mapping. ChangesCursive joining repair
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant renderParagraph
participant planCursiveJoiners
participant joinsAcrossBoundary
participant withCursiveJoiners
participant createJoinerSpan
renderParagraph->>planCursiveJoiners: pass line runs and painter styles
planCursiveJoiners->>joinsAcrossBoundary: test adjacent cursive boundaries
joinsAcrossBoundary-->>planCursiveJoiners: return boundary joining result
planCursiveJoiners-->>renderParagraph: return joiner side plan
renderParagraph->>withCursiveJoiners: render planned runs
withCursiveJoiners->>createJoinerSpan: create styled zero-width joiner
createJoinerSpan-->>withCursiveJoiners: return marked joiner span
withCursiveJoiners-->>renderParagraph: return rendered elements
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (6)
packages/core/src/layout-painter/renderParagraph.ts (1)
1838-1842: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe
isTabRunbranch inapplyJoinerRunStylesis unreachable.
textOfreturnsundefinedfor a tab run, soplanCursiveJoinersnever compares a tab run's painted style, andwithCursiveJoinersnever creates a joiner span for one. Only text runs reachapplyJoinerRunStyles. Drop theisTabRuntest, or add a comment that states why it is kept.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/layout-painter/renderParagraph.ts` around lines 1838 - 1842, The isTabRun branch in applyJoinerRunStyles is unreachable because tab runs never produce joiner spans. Remove the isTabRun condition so the helper applies styles only to text runs, unless there is a documented reason to retain the branch.scripts/generate-joining-types.ts (1)
120-129: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReplace the
as JoiningTypecast with a type guard.The regular expression does not narrow
joining, so the cast is unchecked. A small guard removes the cast and keeps the validation in one place. The coding guidelines require narrowing with type guards instead ofascasts.♻️ Proposed refactor
+const isJoiningType = (value: string): value is JoiningType => /^[CDLRTU]$/u.test(value); + const parseArabicShaping = (text: string): Map<number, JoiningType> => {- if (!/^[CDLRTU]$/u.test(joining)) { + if (!isJoiningType(joining)) { throw new GenerateJoiningTypesError({ message: `Unexpected Joining_Type \`${joining}\` on line: ${line}`, }); } - explicit.set(Number.parseInt(code, 16), joining as JoiningType); + explicit.set(Number.parseInt(code, 16), joining);As per coding guidelines: "Avoid unnecessary as casts; narrow with type guards, in checks, or records, and document unavoidable casts with a SAFETY comment."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/generate-joining-types.ts` around lines 120 - 129, Replace the unchecked `joining as JoiningType` cast in the parsing loop with a type guard that validates and narrows `joining` to `JoiningType`; reuse that guard for the existing validation before passing the narrowed value to `explicit.set`.Source: Coding guidelines
tests/visual/fixtures/build-cursive-face-change.ts (1)
32-47: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
MEEM2duplicatesMEEM.Both constants hold the same character. Use
MEEMin the court and constitutional words and deleteMEEM2.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/visual/fixtures/build-cursive-face-change.ts` around lines 32 - 47, Remove the duplicate MEEM2 constant and update the court and constitutional word definitions to reuse MEEM wherever MEEM2 is referenced, preserving their existing characters.packages/core/src/layout-painter/cursiveJoiners.ts (2)
226-236: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse a params object for
createJoinerSpanandwithCursiveJoiners.Both functions take three or more positional parameters, and
withCursiveJoinerstakes five. Two of them are a callback and aDocument, so an accidental swap is not caught by the type checker in every call shape.planCursiveJoinersalready uses a named options object, so a params object also keeps the module consistent.♻️ Proposed refactor sketch
-export function createJoinerSpan<TRun>( - run: TRun, - applyRunStyles: ApplyRunStyles<TRun>, - doc: Document, -): HTMLElement { +type JoinerSpanOptions<TRun> = { + run: TRun; + applyRunStyles: ApplyRunStyles<TRun>; + doc: Document; +}; + +export function createJoinerSpan<TRun>({ + run, + applyRunStyles, + doc, +}: JoinerSpanOptions<TRun>): HTMLElement {As per coding guidelines: "Use typed positional parameters for one argument and readable two-argument calls; use named options, args, or params objects for three or more or interchangeable arguments."
Also applies to: 242-258
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/layout-painter/cursiveJoiners.ts` around lines 226 - 236, Refactor createJoinerSpan and withCursiveJoiners to accept typed params objects instead of three or more positional arguments, matching the existing planCursiveJoiners options-object pattern. Update all call sites and destructure the named run, applyRunStyles, doc, and other withCursiveJoiners parameters so callback and Document arguments cannot be accidentally swapped.Source: Coding guidelines
39-40: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winWrite joining-critical characters as
\uescapes. Three of these files state in their own header comment that characters are written as escapes, but each declares literal glyphs, including invisible ZWJ and ZWNJ. An invisible literal can be removed, duplicated, or normalised by an editor, a merge, or a copy-paste, and the change leaves no visible trace in review. The tests would then pin the wrong character.
packages/core/src/layout-painter/cursiveJoiners.ts#L39-L40: setZERO_WIDTH_JOINERto"\u200D".packages/core/src/utils/cursiveJoining.test.ts#L28-L42: replace the Arabic letter, mark, ZWJ, and ZWNJ constants with\uescapes, as the header comment states.packages/core/src/layout-painter/cursiveJoiners.test.ts#L21-L26: replace the Arabic letter constants andZWJwith\uescapes, as the header comment states.tests/visual/fixtures/build-cursive-face-change.ts#L32-L47: replace the Arabic letter constants with\uescapes, as the comment at Line 29 states.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/layout-painter/cursiveJoiners.ts` around lines 39 - 40, Replace the literal joining-critical glyphs with Unicode escapes in packages/core/src/layout-painter/cursiveJoiners.ts (39-40), setting ZERO_WIDTH_JOINER to \u200D; in packages/core/src/utils/cursiveJoining.test.ts (28-42), convert the Arabic letter, mark, ZWJ, and ZWNJ constants; in packages/core/src/layout-painter/cursiveJoiners.test.ts (21-26), convert the Arabic letter constants and ZWJ; and in tests/visual/fixtures/build-cursive-face-change.ts (32-47), convert the Arabic letter constants. Preserve the existing character values and header-comment conventions.package.json (1)
36-37: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe
checkscript is not yet run by CI.
scripts/generate-joining-types.tsstates thatcheckis wired into CI, and the PR description lists the CI wiring as a follow-up. Until the workflow callsgenerate:joining-types:check, a Unicode bump can land with a stalejoiningTypes.gen.ts. Do you want me to open an issue for the CI step?🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@package.json` around lines 36 - 37, Wire the generate:joining-types:check command into the CI workflow so it runs on relevant builds and detects stale joiningTypes.gen.ts files. Update the CI configuration rather than only package.json, preserving the existing generation and check scripts.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/core/src/layout-painter/renderParagraph.ts`:
- Line 2203: Update the right-anchored-tab flex rendering path around
renderLineTextRun(next) so joiners are not inserted as separate flex items
across child boundaries. Either skip withJoiners for this path or keep each run
and its joiners together within one inline flex item, preserving correct cursive
joining without standalone zero-width DOM nodes.
In `@scripts/generate-joining-types.ts`:
- Around line 93-102: Update SourceEntry and readCachedSource to retain each
source manifest’s sha256 and verify the cached file against that digest before
returning its contents. Reuse the existing artifact-fetch cache verifier if
available; otherwise hash the file and throw GenerateJoiningTypesError when the
digest is missing or mismatched, while preserving the current missing-cache
guidance.
In `@tests/visual/measure-parity.spec.ts`:
- Line 100: Update the `trimmed` expression in `measure-parity.spec.ts` to
replace the literal zero-width space in the regex character class with the
explicit `\u200B` escape, preserving the existing whitespace-trimming behavior.
- Around line 113-121: Update the runCount assignment in the results.push block
to use the already-collected content spans from the parity comparison instead of
lineEl.children.length, excluding joiner, break-marker, and list-marker elements
from the tolerance budget.
- Around line 131-132: Update the font readiness evaluation in
measure-parity.spec.ts to await document.fonts.ready inside the page context and
return nothing from the page.evaluate callback. Keep openFixture
synchronization-only and avoid exposing the non-serializable FontFaceSet to
Playwright.
---
Nitpick comments:
In `@package.json`:
- Around line 36-37: Wire the generate:joining-types:check command into the CI
workflow so it runs on relevant builds and detects stale joiningTypes.gen.ts
files. Update the CI configuration rather than only package.json, preserving the
existing generation and check scripts.
In `@packages/core/src/layout-painter/cursiveJoiners.ts`:
- Around line 226-236: Refactor createJoinerSpan and withCursiveJoiners to
accept typed params objects instead of three or more positional arguments,
matching the existing planCursiveJoiners options-object pattern. Update all call
sites and destructure the named run, applyRunStyles, doc, and other
withCursiveJoiners parameters so callback and Document arguments cannot be
accidentally swapped.
- Around line 39-40: Replace the literal joining-critical glyphs with Unicode
escapes in packages/core/src/layout-painter/cursiveJoiners.ts (39-40), setting
ZERO_WIDTH_JOINER to \u200D; in packages/core/src/utils/cursiveJoining.test.ts
(28-42), convert the Arabic letter, mark, ZWJ, and ZWNJ constants; in
packages/core/src/layout-painter/cursiveJoiners.test.ts (21-26), convert the
Arabic letter constants and ZWJ; and in
tests/visual/fixtures/build-cursive-face-change.ts (32-47), convert the Arabic
letter constants. Preserve the existing character values and header-comment
conventions.
In `@packages/core/src/layout-painter/renderParagraph.ts`:
- Around line 1838-1842: The isTabRun branch in applyJoinerRunStyles is
unreachable because tab runs never produce joiner spans. Remove the isTabRun
condition so the helper applies styles only to text runs, unless there is a
documented reason to retain the branch.
In `@scripts/generate-joining-types.ts`:
- Around line 120-129: Replace the unchecked `joining as JoiningType` cast in
the parsing loop with a type guard that validates and narrows `joining` to
`JoiningType`; reuse that guard for the existing validation before passing the
narrowed value to `explicit.set`.
In `@tests/visual/fixtures/build-cursive-face-change.ts`:
- Around line 32-47: Remove the duplicate MEEM2 constant and update the court
and constitutional word definitions to reuse MEEM wherever MEEM2 is referenced,
preserving their existing characters.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 8e9af497-7084-4afa-b567-506dcdb406c0
⛔ Files ignored due to path filters (2)
packages/core/src/utils/joiningTypes.gen.tsis excluded by!**/*.gen.tstests/visual/fixtures/cursive-face-change.docxis excluded by!**/*.docx
📒 Files selected for processing (11)
package.jsonpackages/core/src/layout-painter/cursiveJoiners.test.tspackages/core/src/layout-painter/cursiveJoiners.tspackages/core/src/layout-painter/renderParagraph.tspackages/core/src/utils/cursiveJoining.test.tspackages/core/src/utils/cursiveJoining.tsplaywright.config.tsscripts/generate-joining-types.tsspecifications/sources.jsontests/visual/fixtures/build-cursive-face-change.tstests/visual/measure-parity.spec.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (5)
- GitHub Check: Differential parser parity (folio vs python-docx + Open XML SDK)
- GitHub Check: Interaction e2e (playground)
- GitHub Check: Lint, typecheck, test, build
- GitHub Check: DOCX kernel (Rust and WebAssembly)
- GitHub Check: Packaged-consumer build (tarballs)
🧰 Additional context used
📓 Path-based instructions (8)
**/*
📄 CodeRabbit inference engine (AGENTS.md)
**/*: Treat legal data, personal data, and repository secrets as sensitive; keep generated repository artifacts limited to public engineering context.
Preserve security, auditability, least privilege, data minimization, workspace isolation, ownership boundaries, audit trails, encryption-aware handling, and explicit access checks.
Use Conventional Commits prefixes such as feat:, chore:, fix:, and docs:.
Rebase feature branches onto main to maintain linear history.
Enable git rerere and rerere.autoupdate for repeated conflict resolution.
Use vertical slices over horizontal layers; new capabilities should land in independent end-to-end slices and avoid unrelated existing code.
Never delete or regenerate bun.lock for package version bumps; run the workspace-version checker with --write, then bun install --frozen-lockfile.
Do not assume English language or typography conventions; highlight competing date, quotation, citation, and legal-terminology standards when relevant.
Do not publish private user, customer, infrastructure, incident, pricing, roadmap, competitive, identity, or security-architecture context in repository artifacts.
Files:
package.jsonspecifications/sources.jsontests/visual/fixtures/build-cursive-face-change.tstests/visual/measure-parity.spec.tsplaywright.config.tspackages/core/src/utils/cursiveJoining.tspackages/core/src/utils/cursiveJoining.test.tspackages/core/src/layout-painter/renderParagraph.tspackages/core/src/layout-painter/cursiveJoiners.tspackages/core/src/layout-painter/cursiveJoiners.test.tsscripts/generate-joining-types.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Prefer explicit TypeScript designs that make invalid states unrepresentable, including branded types, discriminated unions, exhaustive checks, and invariant/property tests for systemic defects.
Avoid boolean fields for extensible states; use named discriminators or domain types such as unions or enums.
Fail fast: validate at boundaries and return or throw early; minimize brace nesting with inverted conditions and early returns.
Use named constants instead of string literals for domain values.
Do not assign directly to document.cookie.
Avoid spread in loop accumulators; use .push().
Do not use enums; use as-const objects or union types.
Model mutually exclusive states as discriminated unions with a stable discriminator; avoid boolean flag sets with optional payloads.
Construct discriminated-union transitions by explicitly listing target-branch fields; read unions with switch and a never exhaustiveness check.
Avoid unnecessary as casts; narrow with type guards, in checks, or records, and document unavoidable casts with a SAFETY comment.
Trace type mismatches to their source rather than casting at the consumer.
Do not annotate or provide explicit type arguments when the compiler can infer them; let inference flow and narrow at boundaries.
Validate large-union object literals with as const satisfies T rather than a : T annotation.
Use .at(0) when an element may be absent; use [0] only after existence is established or with a SAFETY comment.
Skip barrel files named index.ts; import from explicit module paths.
Prefer arrow functions over function expressions.
Destructure parameters when the intermediate variable is not reused.
Prefer discriminator checks such as obj.type === "x" over in checks for discriminated unions; use in only without an available discriminator.
Use typed positional parameters for one argument and readable two-argument calls; use named options, args, or params objects for three or more or interchangeable arguments.
Reuse dependency-pr...
Files:
tests/visual/fixtures/build-cursive-face-change.tstests/visual/measure-parity.spec.tsplaywright.config.tspackages/core/src/utils/cursiveJoining.tspackages/core/src/utils/cursiveJoining.test.tspackages/core/src/layout-painter/renderParagraph.tspackages/core/src/layout-painter/cursiveJoiners.tspackages/core/src/layout-painter/cursiveJoiners.test.tsscripts/generate-joining-types.ts
**/*.{ts,tsx,rs}
📄 CodeRabbit inference engine (AGENTS.md)
Resolve OOXML elements by namespace URI and local name, explicitly support Strict and Transitional profiles, bound ZIP/XML resource use, and preserve paragraph identifiers as facts rather than durable identities.
Files:
tests/visual/fixtures/build-cursive-face-change.tstests/visual/measure-parity.spec.tsplaywright.config.tspackages/core/src/utils/cursiveJoining.tspackages/core/src/utils/cursiveJoining.test.tspackages/core/src/layout-painter/renderParagraph.tspackages/core/src/layout-painter/cursiveJoiners.tspackages/core/src/layout-painter/cursiveJoiners.test.tsscripts/generate-joining-types.ts
**/*.{test,spec}.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Test only behavior that can evade the type system, framework, or linter; prefer invariants over examples for large input spaces.
Files:
tests/visual/measure-parity.spec.tspackages/core/src/utils/cursiveJoining.test.tspackages/core/src/layout-painter/cursiveJoiners.test.ts
packages/*/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Add a changeset for every published-package src change, selecting all affected packages and the appropriate bump; private playground packages need none.
Files:
packages/core/src/utils/cursiveJoining.tspackages/core/src/utils/cursiveJoining.test.tspackages/core/src/layout-painter/renderParagraph.tspackages/core/src/layout-painter/cursiveJoiners.tspackages/core/src/layout-painter/cursiveJoiners.test.ts
packages/core/**/*
📄 CodeRabbit inference engine (packages/core/GEMINI.md)
Follow the coding guidelines and instructions defined in
AGENTS.md.
Files:
packages/core/src/utils/cursiveJoining.tspackages/core/src/utils/cursiveJoining.test.tspackages/core/src/layout-painter/renderParagraph.tspackages/core/src/layout-painter/cursiveJoiners.tspackages/core/src/layout-painter/cursiveJoiners.test.ts
packages/core/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (packages/core/AGENTS.md)
packages/core/**/*.{ts,tsx,js,jsx}: Keep@stll/folio-coreReact-free: never importreact,react-dom, or React-package types. Put framework-agnostic UI behavior in a core manager extendingSubscribable, use thin framework bindings, and define minimal structural types in core instead of importing adapter types.
Preserve the parser, normalized model, measurement, pagination, and painting boundaries; central pipeline files should orchestrate, while new state concepts or compatibility policies belong in typed helpers or focused modules.
Express every fidelity fix as a reusable OOXML or layout invariant; never branch on fixture identity, source metadata, document text, or other corpus-specific signals.
After roughly five to ten behavior fixes in one subsystem, create a standalone, behavior-preserving consolidation before adding more conditions there.
Files:
packages/core/src/utils/cursiveJoining.tspackages/core/src/utils/cursiveJoining.test.tspackages/core/src/layout-painter/renderParagraph.tspackages/core/src/layout-painter/cursiveJoiners.tspackages/core/src/layout-painter/cursiveJoiners.test.ts
packages/core/**/*.{ts,tsx}
📄 CodeRabbit inference engine (packages/core/AGENTS.md)
packages/core/**/*.{ts,tsx}: Prefer discriminated state machines and explicit coordinate-space types over related booleans, optional-field combinations, and mutable flags.
Keep normalization and layout inputs immutable and idempotent; derive effective values instead of overwriting authored model values during measurement or pagination.
Consolidate shared OOXML syntax, units, geometry, and compatibility rules; do not allow feature parsers to develop subtly different implementations.
Files:
packages/core/src/utils/cursiveJoining.tspackages/core/src/utils/cursiveJoining.test.tspackages/core/src/layout-painter/renderParagraph.tspackages/core/src/layout-painter/cursiveJoiners.tspackages/core/src/layout-painter/cursiveJoiners.test.ts
🔇 Additional comments (11)
scripts/generate-joining-types.ts (1)
145-164: LGTM!Also applies to: 166-185, 198-241, 243-254, 256-294, 296-354
specifications/sources.json (1)
155-192: LGTM!packages/core/src/utils/cursiveJoining.ts (1)
33-50: LGTM!Also applies to: 52-67, 69-83, 85-114, 116-131
packages/core/src/utils/cursiveJoining.test.ts (1)
54-79: LGTM!Also applies to: 81-132, 134-204
packages/core/src/layout-painter/cursiveJoiners.ts (1)
53-72: LGTM!Also applies to: 84-98, 111-132, 134-140, 160-217
packages/core/src/layout-painter/renderParagraph.ts (1)
45-45: LGTM!Also applies to: 1811-1817, 2257-2257
packages/core/src/layout-painter/cursiveJoiners.test.ts (1)
39-90: LGTM!Also applies to: 92-175, 177-232, 234-259
tests/visual/fixtures/build-cursive-face-change.ts (2)
96-110: LGTM!Also applies to: 112-152, 167-189
17-17: 🩺 Stability & AvailabilityNo change needed.
cursive-face-change.docxis present undertests/visual/fixtures/, and the playground loads fixtures from/fixtures/<name>, which matches this fixture location.playwright.config.ts (1)
33-36: LGTM!tests/visual/measure-parity.spec.ts (1)
48-99: LGTM!Also applies to: 102-125, 138-201
Both guards existed but nothing ran them on a PR: CI runs only the `interactions` Playwright project, and the generated joining table was never checked. A guard that does not gate is a comment. Measure/paint parity is safe to gate on despite being a browser test: it compares two numbers read from the same browser in one layout pass, so cross-machine font rendering cancels out rather than making it flaky. Checking the joining table needs its UCD sources, so `specifications:fetch` grows a `--profile` filter. Fetching everything would pull a 43 MB archive and a git clone for two text files totalling 318 KB.
The repaired-line assertion encoded "the measurer always over-reserves". That held for the macOS Arabic fallback and failed on Linux, where the same line painted 259.56px against 257.05px measured. The direction is a property of the font, not of the repair: joined forms are narrower than isolated ones in some faces and wider in others. With the face left to whatever the machine has installed, the test measured the runner's font inventory rather than folio. Pin the fixture's complex-script font to Noto Sans Arabic, which folio-react already loads as a webfont in both 400 and 700, so the relationship is the same everywhere. Correct the residual note in the engine, which made the same wrong claim about direction.
The assertion claimed the measurer always over-reserves on a repaired cursive line. That is a property of the font, not of the repair: unpinned it was -2.7px on the macOS Arabic fallback and +2.5px on the Linux one, so CI failed. Bound the magnitude instead. The residual can push a line either way, which means it is not the safe over-reservation the engine comment also claimed; that note is corrected too. Pinning the fixture to a bundled webfont was tried and reverted: the painted text then used Noto Sans Arabic while the measurer, which runs before the webfont loads, measured a fallback face, a 64px divergence on a 194px line. That font-loading race is a real separate defect and is noted in the fixture.
- Skip joiner insertion on flex-promoted lines. A right-anchored tab makes each child its own formatting context and shaping never crosses a flex item, so a joiner there joins nothing and only adds an inert node. Cursive words already break apart on those lines; repairing that means keeping runs inside one inline item, which belongs to the tab layout rather than here. - Verify the cached UCD files against their manifest digests before parsing. An edited cache would otherwise make `write` emit a table nobody could reproduce while `check` still passed, since both read the same bad input. - Count the tolerance budget over content runs, not every child: joiner spans and markers were each buying another half-pixel, judging a repaired line more loosely than the plain line beside it. - Escape the literal U+200B in a character class, and stop returning `document.fonts.ready` from `page.evaluate`, which hands back a FontFaceSet that cannot be serialised.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
scripts/specification-sources.ts (1)
464-475: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCentralise the
--profileoption token.The option string appears in the usage text, argument lookup, and missing-value error. Define a
PROFILE_FLAGconstant and reuse it.As per coding guidelines, use named constants instead of string literals for domain values.
Proposed fix
+const PROFILE_FLAG = "--profile"; + ... - message: "Usage: bun scripts/specification-sources.ts [check|fetch] [--profile <name>]", + message: `Usage: bun scripts/specification-sources.ts [check|fetch] [${PROFILE_FLAG} <name>]`, ... - const profileFlag = args.indexOf("--profile"); + const profileFlag = args.indexOf(PROFILE_FLAG); ... - throw new SpecificationSourceError({ message: "--profile needs a value" }); + throw new SpecificationSourceError({ message: `${PROFILE_FLAG} needs a value` });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/specification-sources.ts` around lines 464 - 475, Define a named PROFILE_FLAG constant near the argument parsing setup and replace the repeated "--profile" literals in the usage message, args.indexOf lookup, and missing-value error within the surrounding profile-selection logic. Preserve the existing parsing and validation behavior.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.changeset/cursive-joining-face-change.md:
- Line 5: Add the missing commas before “and” and “so” in the release-note
sentence describing shaping failures and ProseMirror text-node offsets, without
changing its wording or meaning.
---
Nitpick comments:
In `@scripts/specification-sources.ts`:
- Around line 464-475: Define a named PROFILE_FLAG constant near the argument
parsing setup and replace the repeated "--profile" literals in the usage
message, args.indexOf lookup, and missing-value error within the surrounding
profile-selection logic. Preserve the existing parsing and validation behavior.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 3b04f24e-607a-4418-8c7a-16313cb5b240
⛔ Files ignored due to path filters (1)
tests/visual/fixtures/cursive-face-change.docxis excluded by!**/*.docx
📒 Files selected for processing (9)
.changeset/cursive-joining-face-change.md.github/workflows/ci.ymlpackage.jsonpackages/core/src/layout-painter/cursiveJoiners.tspackages/core/src/layout-painter/renderParagraph.tsscripts/generate-joining-types.tsscripts/specification-sources.tstests/visual/fixtures/build-cursive-face-change.tstests/visual/measure-parity.spec.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- package.json
- tests/visual/fixtures/build-cursive-face-change.ts
- packages/core/src/layout-painter/renderParagraph.ts
- scripts/generate-joining-types.ts
- packages/core/src/layout-painter/cursiveJoiners.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (4)
**/*
📄 CodeRabbit inference engine (AGENTS.md)
**/*: Treat legal data, personal data, and repository secrets as sensitive; keep generated repository artifacts limited to public engineering context.
Preserve security, auditability, least privilege, data minimization, workspace isolation, ownership boundaries, audit trails, encryption-aware handling, and explicit access checks.
Use Conventional Commits prefixes such as feat:, chore:, fix:, and docs:.
Rebase feature branches onto main to maintain linear history.
Enable git rerere and rerere.autoupdate for repeated conflict resolution.
Use vertical slices over horizontal layers; new capabilities should land in independent end-to-end slices and avoid unrelated existing code.
Never delete or regenerate bun.lock for package version bumps; run the workspace-version checker with --write, then bun install --frozen-lockfile.
Do not assume English language or typography conventions; highlight competing date, quotation, citation, and legal-terminology standards when relevant.
Do not publish private user, customer, infrastructure, incident, pricing, roadmap, competitive, identity, or security-architecture context in repository artifacts.
Files:
scripts/specification-sources.tstests/visual/measure-parity.spec.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Prefer explicit TypeScript designs that make invalid states unrepresentable, including branded types, discriminated unions, exhaustive checks, and invariant/property tests for systemic defects.
Avoid boolean fields for extensible states; use named discriminators or domain types such as unions or enums.
Fail fast: validate at boundaries and return or throw early; minimize brace nesting with inverted conditions and early returns.
Use named constants instead of string literals for domain values.
Do not assign directly to document.cookie.
Avoid spread in loop accumulators; use .push().
Do not use enums; use as-const objects or union types.
Model mutually exclusive states as discriminated unions with a stable discriminator; avoid boolean flag sets with optional payloads.
Construct discriminated-union transitions by explicitly listing target-branch fields; read unions with switch and a never exhaustiveness check.
Avoid unnecessary as casts; narrow with type guards, in checks, or records, and document unavoidable casts with a SAFETY comment.
Trace type mismatches to their source rather than casting at the consumer.
Do not annotate or provide explicit type arguments when the compiler can infer them; let inference flow and narrow at boundaries.
Validate large-union object literals with as const satisfies T rather than a : T annotation.
Use .at(0) when an element may be absent; use [0] only after existence is established or with a SAFETY comment.
Skip barrel files named index.ts; import from explicit module paths.
Prefer arrow functions over function expressions.
Destructure parameters when the intermediate variable is not reused.
Prefer discriminator checks such as obj.type === "x" over in checks for discriminated unions; use in only without an available discriminator.
Use typed positional parameters for one argument and readable two-argument calls; use named options, args, or params objects for three or more or interchangeable arguments.
Reuse dependency-pr...
Files:
scripts/specification-sources.tstests/visual/measure-parity.spec.ts
**/*.{ts,tsx,rs}
📄 CodeRabbit inference engine (AGENTS.md)
Resolve OOXML elements by namespace URI and local name, explicitly support Strict and Transitional profiles, bound ZIP/XML resource use, and preserve paragraph identifiers as facts rather than durable identities.
Files:
scripts/specification-sources.tstests/visual/measure-parity.spec.ts
**/*.{test,spec}.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Test only behavior that can evade the type system, framework, or linter; prefer invariants over examples for large input spaces.
Files:
tests/visual/measure-parity.spec.ts
🪛 LanguageTool
.changeset/cursive-joining-face-change.md
[uncategorized] ~5-~5: Use a comma before “and” if it connects two independent clauses (unless they are closely connected and short).
Context: ...ide an Arabic word stopped shaping there and the word rendered as isolated letter fo...
(COMMA_COMPOUND_SENTENCE_2)
[uncategorized] ~5-~5: Possible missing article found.
Context: ...word rendered as isolated letter forms; Word joins straight through the same boundar...
(AI_HYDRA_LEO_MISSING_THE)
[uncategorized] ~5-~5: Possible missing comma found.
Context: ...ide of such a boundary, each in its own span so run text nodes keep their exact Pros...
(AI_HYDRA_LEO_MISSING_COMMA)
🔇 Additional comments (6)
scripts/specification-sources.ts (2)
478-486: LGTM!
496-498: LGTM!.github/workflows/ci.yml (2)
118-125: LGTM!
271-276: LGTM!tests/visual/measure-parity.spec.ts (1)
40-55: LGTM!Also applies to: 62-139, 141-172, 174-222, 224-235
.changeset/cursive-joining-face-change.md (1)
1-4: LGTM!
1 participant
A cursive word split mid-word by a run boundary renders as isolated letter
forms whenever the two sides resolve to a different font face. Word joins
straight through the same boundary. This fixes that, and adds the invariant
that should have caught it.
The defect
Browsers shape across an inline box boundary only while no shaping-relevant
property changes. Measured in Chrome against the exact span structure the
painter emits:
<a>(hyperlink)So tracked-change and comment marks were never affected: those differ in colour
and decoration, not face. Bold on a letter inside a word is the everyday case
that breaks, and it cannot be fixed by shaping alone because the two halves
genuinely need different faces.
The repair is a zero-width joiner on each side, each in its own span wearing
that side's styles, which measured identically to the joined baseline.
Why the joiner is not in the run's text
data-pm-startspans map DOM text offsets back to document positions forhit-testing. Appending a character to a run's text node would desync every
offset after it, and compensating at each mapping site is the kind of
hand-maintained mirror that drifts. A separate span carries no pm attributes,
so the offset contract is untouched. Pinned by a test.
Joining data is generated, not written
ArabicShaping.txtlists 4 Transparent characters. The other 375 ranges'worth inherit it from General_Category Mn/Me/Cf in a different file, so a
hand-authored table would have misclassified every Arabic harakat, and the
failure would have shown up as mis-shaped text rather than a red test. Both UCD
sources are pinned with checksums;
generate:joining-types:checkfails if thecommitted table drifts.
Face equality is decided by running
applyRunStylesover a probe element anddiffing the result, rather than re-deriving the face from run fields, so there
is no second copy of the font decisions. An unrecognised property counts as a
face change: that adds a harmless joiner instead of silently skipping a repair.
The invariant
Folio measures text with canvas and paints it as DOM text the browser shapes
itself. Two engines, and nothing compared their outputs, so they could disagree
while the whole suite stayed green.
tests/visual/measure-parity.spec.tsstamps the measurer's width on each lineand compares it against a Range over the line's content. Both numbers come from
one layout pass in the same browser, so unlike a screenshot baseline it cannot
go flaky on cross-machine font rendering.
It earned its place immediately, catching a real bug in this branch:
Object.entries()on aCSSStyleDeclarationyields indexed entries, so in abrowser the face comparison was diffing the list of property names rather than
their values, and every colour-only boundary looked like a face change. Unit
tests could not see it because the test fake's
styleis a plain object. Fixed,with a regression test that drives a real-shaped declaration.
Counts are pinned rather than asserted non-zero: the fixture must produce
exactly 6 joiner spans, so both a false positive and a repair that stops
happening fail the build.
Known residual
The measurer cannot see this repair. Canvas
measureTextignores a joiner at astring edge (measuring the halves with and without one returns bit-identical
widths) while DOM layout applies the joined forms, so a repaired word paints
narrower than was reserved. The error is bounded, arises only on words
containing a face change, and is conservative: layout over-reserves, so a line
may break marginally early and can never overflow. The spec pins that direction
so it cannot silently invert. Closing it needs widths from a real shaper rather
than canvas, which is separate work.
Verification
bun run test: docx-core 53, core 4521, react 156, vue 22, agents 98,scripts 100. Zero failures.
bunx playwright test --project=measure-parity: 3 passed.bun --filter @stll/folio-core typecheck,oxlint, andgenerate:joining-types:checkall clean.Follow-ups not in this PR
generate:joining-types:checkand themeasure-parityproject to CI soboth actually gate.
bun run specifications:fetchbefore regenerating thetable, same as the other pinned sources.
Summary by CodeRabbit
Bug Fixes
Tests