Skip to content

fix(tui): use content majority for mixed bidi lines - #72508

Open
CodeinScrubs wants to merge 2 commits into
NousResearch:mainfrom
CodeinScrubs:fix/tui-content-majority-bidi
Open

fix(tui): use content majority for mixed bidi lines#72508
CodeinScrubs wants to merge 2 commits into
NousResearch:mainfrom
CodeinScrubs:fix/tui-content-majority-bidi

Conversation

@CodeinScrubs

Copy link
Copy Markdown

What does this PR do?

Hermes already applies bidi-js before Ink places terminal cells on Windows, Windows Terminal, WSL, and VS Code. The current call uses the Unicode first-strong (auto) paragraph base, so an English technical token at the start can force a Persian/Arabic-majority line onto the wrong base:

React یک کتابخانه جاوااسکریپت بسیار محبوب است.

This patch selects the base from all strong characters in the rendered line:

  • more R/AL characters -> RTL
  • more L characters -> LTR
  • a tie -> existing first-strong behavior

It uses Hermes's existing bidi-js dependency and adds no runtime dependency. Pure ASCII lines return the original array before bidi-js is initialized, preserving the common LTR fast path.

Maintainer disclosure: I maintain the MIT-licensed BidiLens project. Its mixed-direction failure model and English-first/Persian-majority corpus case motivated this focused host patch. BidiLens is not added as a dependency because this implementation can use Hermes's existing renderer dependency and retain Hermes's current Node.js compatibility.

Related Issue

Advances the TUI work in #41454. The same first-strong failure class is reported for other Hermes surfaces in #51318 and #71613.

This PR also incorporates the regression-test commit from #15238 with its original author metadata, then adds the implementation and broader direction-policy tests.

Type of Change

  • Bug fix (non-breaking change that fixes an issue)
  • New feature (non-breaking change that adds functionality)
  • Security fix
  • Documentation update
  • Tests (adding or improving test coverage)
  • Refactor (no behavior change)
  • New skill (bundled or hub)

Changes Made

  • ui-tui/packages/hermes-ink/src/ink/bidi.ts
    • select the paragraph base by strong-character majority
    • retain first-strong semantics for ties
    • preserve a zero-allocation return for lines with no RTL content
    • add an ASCII fast path before initializing/scanning with bidi-js
  • ui-tui/packages/hermes-ink/src/ink/bidi.test.ts
    • cover pure LTR identity/no-op behavior
    • cover Arabic and mixed Arabic/English rendering
    • cover English-first Persian-majority and RTL-first English-majority cases
    • cover both LTR-first and RTL-first ties

How to Test

  1. npm ci --workspace ui-tui --include-workspace-root
  2. npm test --workspace ui-tui -- packages/hermes-ink/src/ink/bidi.test.ts
  3. npm run typecheck --workspace ui-tui
  4. npm run build:ink --workspace ui-tui
  5. npm exec --workspace ui-tui -- eslint packages/hermes-ink/src/ink/bidi.ts packages/hermes-ink/src/ink/bidi.test.ts
  6. npx prettier --check ui-tui/packages/hermes-ink/src/ink/bidi.ts ui-tui/packages/hermes-ink/src/ink/bidi.test.ts

Fresh local results:

  • focused suite: 8/8 tests passed
  • TypeScript typecheck: passed
  • Ink build: passed
  • touched-file ESLint: passed
  • Prettier and git diff --check: passed

I also ran npm run check --workspace ui-tui. Build and typecheck passed, and 1,385 tests passed. Eight unrelated Windows/environment-sensitive tests failed in terminalSetup.test.ts, terminalParity.test.ts, and editor.test.ts; the command registry setup also reports the local Python environment missing yaml. I am not claiming the complete local suite is green.

Checklist

Code

  • I've read the Contributing Guide
  • My commit messages follow Conventional Commits (fix(scope):, feat(scope):, etc.)
  • I searched for existing PRs to make sure this isn't a duplicate
  • My PR contains only changes related to this fix/feature (no unrelated commits)
  • I've run pytest tests/ -q and all tests pass — not run; this patch only changes the TypeScript TUI renderer, and the relevant TUI checks are listed above
  • I've added tests for my changes (required for bug fixes, strongly encouraged for features)
  • I've tested on my platform: Windows 11

Documentation & Housekeeping

  • I've updated relevant documentation (README, docs/, docstrings) — N/A; behavior and constraints are documented inline and in tests
  • I've updated cli-config.yaml.example if I added/changed config keys — N/A
  • I've updated CONTRIBUTING.md or AGENTS.md if I changed architecture or workflows — N/A
  • I've considered cross-platform impact (Windows, macOS) per the compatibility guide
  • I've updated tool descriptions/schemas if I changed tool behavior — N/A

Screenshots / Logs

For the English-first Persian-majority fixture, the current first-strong path produces this visual-cell order:

React تسا بوبحم رایسب تپیرکسااواج هناخباتک کی.

The content-majority RTL base produces:

.تسا بوبحم رایسب تپیرکسااواج هناخباتک کی React

The patch remains intentionally scoped to Hermes's current software-bidi path and the scripts handled by its existing bidi-js version. Terminal font shaping and supplementary-plane limitations in that dependency are outside this PR.

@teknium1

Copy link
Copy Markdown
Contributor

Thanks for the focused renderer fix and regression coverage. The premise remains present on current main: ui-tui/packages/hermes-ink/src/ink/bidi.ts:69 passes auto to getEmbeddingLevels, and bidi-js resolves that mode by scanning for the first strong R/AL or L character (node_modules/bidi-js/dist/bidi.js:800-809). The PR changes that exact TUI path, which is invoked before terminal-cell placement by ui-tui/packages/hermes-ink/src/ink/output.ts:700.

No verified problems found in the two-file diff. It reuses the existing bidi-js dependency, preserves the no-op path for ASCII input, and adds targeted direction-policy tests. GitHub currently reports the branch as mergeable despite being behind main.

Automated hermes-sweeper review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/tui Terminal UI (ui-tui/ + tui_gateway/) P3 Low — cosmetic, nice to have sweeper:blast-contained Sweeper blast radius: contained — one narrow path / opt-in / few users sweeper:risk-platform-windows Sweeper risk: may break or behave differently on native Windows type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants