feat: render invalid ICU placeholders as plain text

[bdshadow]

Problem

Invalid ICU expressions like {placeholder:space} (a colon isn't valid ICU) parsed as a partial {placeholder Expression plus trailing :space} text. In an RTL editor, htmlIsolatesPlugin puts the Expression in an LTR isolate and the trailing text in an RTL isolate, splitting the single token across two bidi boundaries and scrambling it.

Fix

Invalid {...} contents now parse leniently as a single Expression containing an InvalidExpressionBody (no Param, no styled tokens):

• No chip in placeholder mode — getPlaceholders skips it (inner node isn't FormatExpression).
• No highlighting in syntax mode — there are no Param/styled child tokens.
• Not split — the whole {...} stays one node, so RTL bidi isolation keeps it intact.
• Valid placeholders / plurals / selects are unchanged.

Mechanism: an extend: true external tokenizer forks the parse, and InvalidExpressionBody's @dynamicPrecedence=-1 makes a real format/select parse win whenever one exists. ICU validity itself is still reported by the formatjs-based linter.

Adds getInvalidPlaceholders(text, nested?) to detect such expressions (used by the platform to warn the user).

Tests

166 tests pass, including new parser cases and a getInvalidPlaceholders suite. tsc, eslint, and vite build are clean.

[Anty0]

I'm not very familiar with the lezer, but I tried my best to understand the patch's intentions. Most of the points are from the AI review, but the one about keeping invalid placeholders bidi is my idea.

[bdshadow]

Thanks for the thorough review @Anty0 — all points addressed in 03d894a.

1. Comma-containing invalid bodies split — fixed. The tokenizer no longer stops at ,; it scans to the closing brace, so {placeholder:space, foo} parses as a single Expression → InvalidExpressionBody.
2. Leading LRM/RLM split — fixed. Dropped the isWhiteSpace guard, so a bidi-mark-leading body is captured whole (test added with U+200F).
3. Invalid expr forced LTR (the important one) — fixed. htmlIsolatesPlugin now isolates an invalid Expression by its own content direction (first strong character, à la unicode-bidi: plaintext) instead of forcing LTR. Valid placeholders stay LTR; {هذا نص عربي} → RTL isolate and renders correctly.
4. Duplicated terminator set — gone; there's a single scan loop and no leading-char guard now.
5. getInvalidPlaceholders comment — dropped.
6. Grammar comment duplication — reduced to just the @dynamicPrecedence rationale, pointing at the tokenizer as the canonical home.

The guards were only needed before extend: true; with the fork in place, InvalidExpressionBody's @dynamicPrecedence=-1 alone keeps every valid format/select parse winning, so the tokenizer is now a plain scan-to-brace.

Behavior note: any closed {...} that isn't valid ICU is now a single lenient node, so { , }, { yo, }, { }, etc. render as plain text + the "invalid" warning instead of throwing — I updated those parser tests. Only empty {} and unclosed { still error.

168 tests pass; tsc / eslint / build clean.

[Anty0]

Seems like there are still some problems. Some introduced as part of the fix :3

[Anty0]

contentDirection is documented as first-strong / unicode-bidi: plaintext, but it detects strength only via \p{L} plus a hardcoded 5-script RTL_SCRIPT list. Two gaps reintroduce the scramble this PR set out to fix:

• Explicit bidi marks aren't \p{L}, so they're skipped: RLM (U+200F) and ALM (U+061C) are strong-RTL, LRM (U+200E) strong-LTR. An RLM-leading invalid body therefore infers LTR — and getInvalidPlaceholders.test.ts deliberately feeds RLM-leading input, so this is an expected case.
• RTL scripts outside the 5 (Adlam, Mandaic, Samaritan, Hanifi Rohingya, …) match \p{L} first and are classified LTR.

Could we use library for this? Feels like a bad idea to implement custom detector for this.

[bdshadow]

Replaced with bidi-js in a81a540. Its getEmbeddingLevels trips on surrogate pairs, but its per-character class data is complete (RLM/ALM as R/AL, Adlam R, Hanifi Rohingya AL), so contentDirection now iterates code points and applies the UAX#9 P2 rule over bidi-js classes. All Unicode data comes from the library; only the 3-line first-strong rule remains here. Bundled into dist like entities, so consumers need no new dependency (~8KB gzip).

[Anty0]

Nit: pull the compound condition into a named boolean so the ternary reads plainly:

const followsRtlContent =
  isInvalidExpression(node) &&
  contentDirection(view.state.doc.sliceString(node.from, node.to)) === Direction.RTL;
set.add(node.from, node.to, followsRtlContent ? isolateRTL : isolateLTR);

[bdshadow]

Done, extracted as followsRtlContent.

[Anty0]

Design note: enter now runs three direction policies — tags & valid Expression forced LTR, Text/TextRoot forced RTL, and invalid Expression content-derived (first-strong). By this PR's own reasoning an invalid expression is 'arbitrary user text' — yet the visually-identical Text node is forced RTL while the invalid Expression uses plaintext direction. Two rules for the same kind of content, and a direction boundary between an invalid expression and the text around it. Worth considering one content-direction policy for all user text (bigger blast radius, needs RTL regression tests) instead of a special case — and deriving direction from a maintained bidi utility rather than a bespoke 5-script regex in the view layer.

[bdshadow]

Kept the special case deliberately. This plugin only mounts when the language itself is RTL, so forcing Text to RTL is the point: a Latin-only fragment of an Arabic translation must still sit in the RTL paragraph flow. Switching Text to first-strong would relayout every Latin-heavy RTL translation. The invalid expression genuinely cannot reuse that policy: forcing it RTL renders Latin content with the braces flipped outward (}placeholder:space{ ), which is the neutral-brackets problem this PR fixes. The bespoke-regex half of the concern is gone now that bidi-js owns the data. If we later want one plaintext policy for all user text, I would do that as its own change with RTL regression tests.

[Anty0]

Nit (step-down): contentDirection and isInvalidExpression (plus their RTL_SCRIPT / STRONG_LETTER consts) are only called from computeIsolates but sit above it, while the rest of the file reads top-down (htmlIsolatesPlugin above computeIsolates). Consider moving these helpers below computeIsolates to keep the top-down flow.

[bdshadow]

Done, helpers moved below computeIsolates.

[Anty0]

This returns null whenever the parse throws. But the lenient tokenizer only rescues {...} bodies with at least one non-brace char — empty {} produces no token and still throws, as does truly unparseable input (unclosed {). So a realistic string like {good:bad} {} or {good:bad} more {unclosed returns null and suppresses the warning for the clearly-invalid {good:bad} too — precisely the messy input this warning API exists to catch. Consider making it error-tolerant (collect InvalidExpressionBody spans from a non-strict parse, skipping error nodes), or document that null means 'entire string unparseable' and confirm the platform consumer falls back accordingly.

[bdshadow]

Done in a81a540. It now parses non-strict (never throws), collects InvalidExpressionBody expressions and skips unclosed ones (last child must be ExpressionClose), and always returns an array. {good:bad} {} and {good:bad} more {unclosed both report {good:bad} now, and the walk descends into select variants so an invalid placeholder nested in a plural is found too. Tests added for all three.

[Anty0]

Convention: placeholder public types live in src/parser/types.ts (Position, Placeholder, TolgeeFormat), and getPlaceholders imports Placeholder from there. InvalidPlaceholder — a direct sibling of Placeholder that already reuses Position from types.ts — is defined inline here instead, which forces the bespoke export type { InvalidPlaceholder } line in tolgee-editor.ts. Suggest moving it into types.ts next to Placeholder so it re-exports via the existing export * from './parser/types'.

[bdshadow]

Done, moved next to Placeholder in types.ts.

[Anty0]

Design note: this re-parses the input and runs a tree.cursor() walk that closely mirrors getPlaceholders.ts (same configure({strict, top}).parse, same null-on-throw, same firstChild?.nextSibling unwrap). The existing Placeholder model already carries problematic placeholders via its error? channel, emitted by getPlaceholders in a single walk. Folding invalid expressions into getPlaceholders (e.g. error: 'invalid_expression') would give consumers valid + invalid placeholders from one parse in document order, instead of two functions that parse the same string twice and return two disjoint lists. If a separate entry point must stay, at least factor the shared parse+walk into one helper.

[bdshadow]

Went with the fallback option: kept the separate entry point and factored the shared parse into parseTolgee. Folding into getPlaceholders via the error channel would fight the feature itself: PlaceholderPlugin and the platform render chips for everything getPlaceholders returns (error-flagged tags render as error chips), while invalid placeholders must render as plain text, so every consumer would need to filter. And after the non-strict change the contracts genuinely differ: getPlaceholders is strict and null-on-failure (PlaceholderPlugin relies on that null for its shiftByChanges fallback), getInvalidPlaceholders is non-strict and never null.

[Anty0]

If InvalidPlaceholder moves to src/parser/types.ts (see the comment on its definition), this explicit export type line can be dropped — it'd be covered by the existing export * from './parser/types' below.

[bdshadow]

Done, covered by the existing export * from ./parser/types.

[Anty0]

This comment restates the leniency rationale that already lives (authoritatively) in the invalidExpression tokenizer comment, plus 'validity is reported by the linter' — behavior owned elsewhere. It duplicates rationale that rots independently. The helper name expectIcuOnlyThrows and its two assertions already say what it does; suggest dropping the comment.

[bdshadow]

Done, dropped.