fix(rtl): extend automatic bidi handling to thinking cards and session list

[zagee1]

Summary

Extends the automatic RTL/bidi handling from #6560 to two surfaces that were missed because they render outside .msg-body:

1. Thinking-card body (_thinkingCardHtml in static/ui.js) — reasoning text renders in a plain <pre> for whitespace preservation. Without dir="auto", the browser inherits the chat's base direction instead of resolving per-paragraph bidi for Hebrew/Arabic reasoning text, so it never right-aligns or reorders correctly.
2. Sidebar session list (.session-title / .session-preview in static/style.css) — rendered by sessions.js, a completely separate code path from the chat message pipeline that _applyAutomaticMessageDirections() walks. It never got any dir attribute or bidi-aware CSS, so Hebrew/Arabic session titles/previews always render left-aligned.

Changes

• static/ui.js: add dir="auto" to the thinking-card <pre>.
• static/style.css:
  • .chat-content-rtl .thinking-card-body pre[dir="auto"]{ text-align:right; } so the visual alignment follows the resolved direction.
  • .chat-content-rtl .session-title, .chat-content-rtl .session-preview{ direction:rtl; unicode-bidi:plaintext; text-align:right; } — unicode-bidi:plaintext is the CSS equivalent of dir="auto" (resolves base direction per-element from its first strong character); text-align:right matches the RTL-skin default used elsewhere for .msg-body.

Testing

Added tests/test_sessions_thinking_rtl_bidi.py pinning both fixes at the source level (asserts the dir="auto" attribute and the specific CSS rules exist), so a future refactor of _thinkingCardHtml or the sidebar selectors can't silently regress this.

$ uv run pytest tests/test_sessions_thinking_rtl_bidi.py tests/test_issue6560_mixed_direction_rendering.py -q
.......                                                                  [100%]
7 passed in 5.81s

No build step involved — plain CSS/JS edits per the project's no-bundler architecture.

Related: #6560 (this PR only extends its bidi coverage to two surfaces outside its scope; does not modify anything from that PR).

[greptile-apps]

Greptile Summary

The PR extends automatic bidirectional-text handling to thinking cards and sidebar session text while completing the previously requested fixes for session-search previews and Markdown tables.

• Adds automatic direction resolution to free-form thinking-card content.
• Applies RTL alignment and plaintext bidi handling to rendered session titles, metadata, and search previews.
• Keeps ordinary Markdown table structure explicitly LTR.
• Adds source-level and DOM-shim regression coverage for these direction contracts.

Confidence Score: 5/5

The PR appears safe to merge.

No blocking failure remains.

Important Files Changed

Filename	Overview
static/style.css	Adds RTL rules for thinking cards and the actual session-list classes, while explicitly isolating Markdown tables as LTR.
static/ui.js	Adds automatic direction processing across rendered message prose, preserves thinking-card auto direction, and classifies table structures as machine-oriented LTR content.
tests/test_issue6560_mixed_direction_rendering.py	Extends regression coverage to verify table structural elements remain LTR after automatic direction processing.
tests/test_sessions_thinking_rtl_bidi.py	Pins the thinking-card attribute and the renderer-backed session title, metadata, and search-preview selectors.

_{Reviews (5): Last reviewed commit: "Merge branch 'master' into fix/rtl-bidi-..." | Re-trigger Greptile}

[nesquena-hermes]

Summary

I read the four-file patch at head 486785b8, including the full changed regression files and the relevant renderer and sidebar contexts against origin/master. The thinking-card change itself is sound, but the PR currently has two user-visible gaps in the broader automatic-direction code that is also part of this diff. One sidebar selector never matches rendered markup, and ordinary Markdown tables are assigned automatic direction despite the test claiming they are machine-oriented LTR content.

Code reference

The sidebar renderer creates .session-title, .session-meta, and .session-search-preview at static/sessions.js:8148-8268. It never creates .session-preview. The new rule at static/style.css:7232-7240 therefore reaches the title but not either rendered preview surface.

The second mismatch is inside static/ui.js:1547-1557:

const blockSelector="p,li,blockquote,h1,h2,h3,h4,h5,h6,ul,ol,table,thead,tbody,tfoot,tr,th,td";
for(const block of blocks) block.setAttribute("dir","auto");
const machineSelector=[
  "pre","code","kbd","samp","tt",".hljs",".code-block",
  ".diff-block",".csv-table-wrap",".csv-table"
].join(",");

Plain table elements are in the auto-direction set but absent from the LTR set. The CSS override at static/style.css:7242-7259 likewise covers .csv-table, not ordinary Markdown tables. tests/test_issue6560_mixed_direction_rendering.py:35-45 only checks that the word table occurs in blockSelector, so it currently locks in the opposite of its stated LTR contract.

Diagnosis / recommendation

Please change the sidebar selector to the real preview class or classes, most likely .session-search-preview and, if detailed metadata is intended, .session-meta. Add a rendered-selector test rather than a CSS substring test so a nonexistent class cannot pass.

For tables, either remove table,thead,tbody,tfoot,tr,th,td from blockSelector and add them to machineSelector, or add explicit ordinary-table LTR CSS and mark those nodes LTR after the prose pass. The tests should execute the helper against a Markdown table and assert the final dir values.

Verification step

A focused DOM fixture should render one title, one search preview, and one ordinary Markdown table under .chat-content-rtl; assert both sidebar text nodes receive the intended bidi behavior and every table node finishes LTR. I did not execute PR-authored code during this read-only review.

[zagee1]

Thanks for the review — both issues were real, fixed in d7673d0:

1. Sidebar preview class mismatch: static/sessions.js never creates .session-preview (it creates .session-title, .session-meta, .session-search-preview). Fixed the CSS selector to target the actual rendered classes, and added a test (test_renderer_still_uses_expected_classnames) that reads the classNames sessions.js assigns via regex, so a future rename can't cause the same silent mismatch.

2. Ordinary Markdown tables not forced LTR: table,thead,tbody,tfoot,tr,th,td were in blockSelector (per-element dir="auto", prose treatment) with no matching entry in machineSelector/CSS — the .csv-table LTR rule doesn't apply to plain Markdown tables. Moved these tags into machineSelector (forced dir="ltr") and added .chat-content-rtl .msg-body table, .chat-content-rtl .msg-body table *{ direction:ltr; ... }.

Also replaced the CSS-substring test for the mixed-direction helper with one that actually executes _applyAutomaticMessageDirections against a real table/paragraph element tree in a minimal Node DOM shim (no jsdom dependency, following the pattern already used in test_issue5514_composer_grow_scroll_pin.py) and asserts the resulting dir attributes — this is what caught that the original fix's table handling was backwards.

Verification:

$ uv run pytest tests/test_sessions_thinking_rtl_bidi.py tests/test_issue6560_mixed_direction_rendering.py -v
...
============================== 12 passed ==============================

$ uv run pytest tests/ -q -k 'rtl or bidi or direction or session_list or thinking or 6560'
261 passed, 6 skipped, 14409 deselected in 124.60s

[nesquena-hermes]

Gate: no XSS and the approach is right, but an existing postprocessor overwrites dir="auto" back to ltr — so the fix doesn't take effect yet

Thanks @zagee1 — the direction and security are sound (the dir="auto" value is a literal, and a script/image injection payload stays escaped through the unchanged esc(clean) path — no new XSS sink). But reproducing it in Chromium shows the feature doesn't actually reach the rendered DOM, plus one surface is missed:

SILENT 1 — the machine-classification pass rewrites the thinking <pre> back to dir="ltr" (static/ui.js:1556/1566)

Right after your dir="auto" is applied, the machine-content selector at line 1556 includes 'pre', and the loop at 1566 does:

node.setAttribute('dir','ltr');
node.classList.add('message-machine-ltr');

_thinkingCardHtml()'s <pre dir="auto"> matches that pre selector, so it's forced to dir="ltr" (class message-machine-ltr) — I confirmed in Chromium the element ends up dir="ltr", not auto. This both defeats the new mixed-direction behavior AND regresses the prior RTL inheritance. Fix: exclude .thinking-card-body pre from the machine-classification selector, and add a final-DOM assertion (after _applyAutomaticMessageDirections()) that the thinking <pre> is still dir="auto".

SILENT 2 — the live/legacy thinking path still emits a bare <pre> (static/ui.js:20120)

appendThinking() reaches the _thinkingMarkup() constructor via _renderThinkingInto(), which emits <pre> with no dir="auto" — so live-streaming Hebrew/Arabic reasoning has no automatic direction (and would be forced LTR by the same postprocessor). Fix: add dir="auto" to the live/legacy _thinkingMarkup() <pre> too, so both the cached-render and live paths behave identically.

Verified working (baseline)

• No new XSS: injection payload stayed escaped through esc(clean).
• Before the postprocessor runs, English dir="auto" has identical LTR geometry to baseline; Hebrew/English/code lines resolve independently — so the approach is correct, it's just being overwritten.
• 21 bidi/RTL + 116 neighboring thinking/activity tests pass — but they don't assert the FINAL thinking-card DOM after postprocessing, which is why the overwrite slipped through.

Add a test that checks the thinking <pre> is still dir="auto" AFTER the machine-classification pass (both the cached-render and live constructors), plus an escaping-preservation assertion, and this should be straightforward to land. The concept is good and needed — it just needs to survive the postprocessor.

[zagee1]

Thanks for reproducing in Chromium and pinpointing exactly where — this was the real bug. Fixed in dc7ae1f:

SILENT 1 — confirmed: _applyAutomaticMessageDirections's machine-classification loop matched plain pre unconditionally and ran after dir="auto" was set, forcing it back to dir="ltr" (+message-machine-ltr). Fixed by skipping any node inside .thinking-card-body in that loop (node.closest('.thinking-card-body')), same pattern as excluding .csv-table from prose reprocessing.

SILENT 2 — confirmed: _thinkingMarkup() (the live/legacy streaming constructor) never had dir="auto" at all — only _thinkingCardHtml() (cached/history render) did. Added it there too so both paths behave identically.

Added TestThinkingCardFinalDom, which runs the REAL _thinkingCardHtml/_thinkingMarkup constructors AND the REAL _applyAutomaticMessageDirections in a Node DOM shim, then asserts the FINAL dir attribute on the resulting <pre> — exactly the gap you called out, since prior tests only checked constructor source and never exercised the full pipeline where the overwrite happened. Also added an escaping-preservation assertion against a script/image injection payload through the unchanged esc() path (no new XSS sink, confirmed).

$ uv run pytest tests/test_sessions_thinking_rtl_bidi.py tests/test_issue6560_mixed_direction_rendering.py -v
...
============================== 17 passed ==============================

$ uv run pytest tests/ -q -k 'rtl or bidi or direction or session_list or thinking or 6560'
266 passed, 6 skipped, 14409 deselected in 126.89s

This should now actually reach the rendered DOM in Chromium too — happy to hear if you want to re-verify.

[nesquena-hermes]

Re-gate: both cycle-10 findings are fixed and the RTL rendering is correct — but the new renderMessages call breaks an existing test (real regression, full suite caught it)

Great progress @zagee1 — both prior findings are genuinely resolved (Codex + a live Chromium capture confirm it):

• The thinking-card <pre> now retains dir="auto" after the machine-classification pass (the .thinking-card-body exclusion works), and both _thinkingCardHtml and the live _thinkingMarkup emit dir="auto".
• I rendered a mixed Hebrew/Arabic/English/code thinking card on a live server: Hebrew and Arabic lines resolve RTL (right-aligned), English and code lines stay LTR, each line per-line correct — exactly the intended bidi. No XSS (payloads produced zero executable nodes).

But the full suite caught a real regression that the RTL-focused checks didn't:

tests/test_anchor_fallback_ownership.py::test_render_messages_keeps_anchor_owned_turn_out_of_legacy_activity_rebuilds — FAILS on this branch, PASSES on clean master

Root cause (I isolated it — reproduces standalone on your branch, green on origin/master same box):

ReferenceError: _applyAutomaticMessageDirections is not defined
    at renderMessages (…ui.js:1617)   ← your new bare call `_applyAutomaticMessageDirections(inner);`

That test extracts renderMessages and runs it in isolated Node. Your new call site inside renderMessages (static/ui.js:18190, _applyAutomaticMessageDirections(inner);) is unguarded, so when the harness runs the extracted function without the helper in scope, it throws. Note the sibling line right above it already uses the safe pattern:

if(typeof _syncLiveRunStatusAfterRender==='function') _syncLiveRunStatusAfterRender();
_applyAutomaticMessageDirections(inner);   // ← this one is bare

And you already guarded your OTHER call elsewhere with if(window._applyAutomaticMessageDirections) — this call site (and the one at ui.js:19425) just missed the same treatment.

Fix (either, prefer the guard for consistency with the sibling)

1. Guard the two call sites the way the sibling call is guarded:

   if(typeof _applyAutomaticMessageDirections==='function') _applyAutomaticMessageDirections(inner);

   at static/ui.js:18190 and :19425. This matches the existing renderMessages idiom and keeps the extraction harness green.
2. Or update test_anchor_fallback_ownership.py's function-extraction to include the _applyAutomaticMessageDirections helper (heavier, and doesn't help any other harness that extracts renderMessages).

The guard (option 1) is the smaller, safer change and mirrors what's already there. Once the two call sites are guarded and the full suite is green, this is ready — the feature itself is correct and needed. Thanks for the persistence.

[nesquena-hermes]

Summary

I re-read the four-file head at ddfcd2621, the complete two regression files, and the focused delta from dc7ae1f8. The latest commit fixes the remaining regression from my prior gate without changing the RTL behavior itself. The thinking-card constructors still emit automatic direction, the machine-content pass still excludes thinking-card bodies, and the isolated renderMessages harness can now run without defining the new helper. I do not see another static blocker.

Code reference

The complete latest delta is the two guards at static/ui.js:18187 and static/ui.js:19427:

if(typeof _applyAutomaticMessageDirections==='function')
  _applyAutomaticMessageDirections(inner);

if(typeof _applyAutomaticMessageDirections==='function')
  _applyAutomaticMessageDirections(container);

That matches the defensive pattern already used immediately above the render call for _syncLiveRunStatusAfterRender. In the shipped page the helper is defined at static/ui.js:1519, so normal browser behavior is unchanged. In extracted-function tests, the call becomes a safe no-op rather than throwing a ReferenceError.

I also checked the original behavior chain remains intact: _thinkingCardHtml and _thinkingMarkup emit <pre dir="auto"> at static/ui.js:11584 and static/ui.js:20122; _applyAutomaticMessageDirections skips nodes under .thinking-card-body at static/ui.js:1564-1571; and the RTL visual rule remains at static/style.css:7230-7238. The table LTR classification and actual sidebar selectors are unchanged by this final patch.

Diagnosis / recommendation

The previous failure was a harness-scope regression, not a browser RTL defect. Guarding the optional postprocessor is the smaller and safer correction because tests/test_anchor_fallback_ownership.py:411 intentionally extracts and evaluates the real renderMessages() body with a narrow dependency set. Requiring every isolated harness to import the entire direction subsystem would make unrelated renderer tests more brittle.

Verification step

The required check is the previously failing anchor fallback test plus the two bidi files. The focused direction tests at tests/test_issue6560_mixed_direction_rendering.py:196-200 still prove both production call sites exist, while the anchor test proves an environment without the helper no longer crashes. CI or a maintainer should run those tests because I did not execute PR-authored code during this read-only review.

Verdict

The blocker from my prior review is resolved on static inspection. With the focused CI checks green, this is ready to merge from my side.

[nesquena-hermes]

Thanks @zagee1 — this is careful, well-commented work, and the core approach is right: the adversarial gate confirmed the thinking-card carve-out works in the real DOM (Hebrew reasoning gets dir="auto", machine content stays LTR), English message/thinking geometry is byte-identical to master, there's no MutationObserver feedback loop, observer count stays bounded, and no XSS sink is introduced. Two real issues need fixing before it can ship, both reproduced in Chromium.

1. Live-streamed messages miss direction handling until the final rerender (static/ui.js:1531-1544)

The per-.msg-body observer design only covers bodies that already exist when _applyAutomaticMessageDirections runs. When a new assistant message streams in, static/messages.js:2631-2633 appends the new live body as a sibling of the existing bodies, not as a child of any observed body — so the new message gets no dir handling until the final settled rerender. RTL/mixed-direction text therefore renders in the wrong direction first and then visibly snaps when the rerender lands.

Reproduced: an existing body was processed, but a subsequently appended sibling remained without dir attributes.

Fix: replace the per-existing-body observers with a single observer on the stable transcript root (the container that persists across renders), and process every added subtree — including a newly added .msg-body root — in the callback. This also removes the scaling cost below.

2. English session titles/metadata/previews flip alignment when RTL mode is on (static/style.css:7237-7247 + static/sessions.js:8178,8287,8294)

The sidebar rules force direction:rtl + text-align:right on .session-title, .session-meta, and .session-search-preview. That overrides the first-strong (unicode-bidi:plaintext) behavior the comment intends — so an English session title shifts from left to right the moment RTL mode is enabled, instead of staying left-aligned per its own first strong character.

Fix: assign dir="auto" on the title/meta/preview elements in static/sessions.js (lines 8178, 8287, 8294 — the element-creation sites), remove the unconditional direction:rtl/text-align:right, and use text-align:start so each item aligns to its own resolved direction. This makes the session list behave like the message prose already does.

Perf note (fixed for free by #1)

A synthetic 1,000-body probe spent ~68 ms in mutation-callback fan-out with the per-body observers. The single stable-root observer in fix #1 removes this.

Tests

The two new suites are non-vacuous (15 of 17 assertions fail on master, the 2 source-guard assertions pass by design) — good. But they don't cover (a) a live-added sibling body, or (b) computed session-list alignment. Please add a regression for each: a streamed sibling .msg-body gets dir handling without a full rerender, and an English session title stays start-aligned in RTL mode.

Ping me on re-push and I'll re-gate. The direction of travel here is good — these are architecture tweaks (stable-root observer + let the sidebar resolve its own direction), not a rethink.