feat: per-block direction detection in Auto mode - #2
Conversation
Response blocks now get dir=rtl/ltr by presence of RTL characters instead of first-strong-character plaintext heuristic. Fixes English-first mixed lines drifting left, bullets on the wrong side, and hard-to-read mixed Hebrew/English text. Pure-English blocks stay naturally left-aligned. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: edb0c90575
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| var SKIP_SEL = '[class*="codeBlockWrapper_"],pre,code,[class*="toolUse_"],[class*="toolResult_"],[class*="thinkingContent_"]'; | ||
|
|
||
| function tagBlocks(bubble) { | ||
| var els = bubble.querySelectorAll(BLOCK_SEL); |
There was a problem hiding this comment.
When an RTL timeline bubble contains non-markdown UI with matching tags—such as a <p> or <li> inside a tool summary/body—this bubble-wide query assigns that element a native dir attribute even though tool UI is intended to remain LTR. The skip selector only covers toolUse_ and toolResult_, while the existing LTR overrides identify additional tool containers such as toolSummary_, toolBody_, and toolBodyRow_; a direction specified on their child is not neutralized by an inherited direction on the container. Query blocks only beneath the eligible markdown root_ elements (while preserving the exclusions), or skip every protected tool/UI container.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
This PR improves Auto mode RTL handling by moving from unicode-bidi: plaintext (first-strong-character direction) to per-block direction tagging, so mixed Hebrew/English blocks render with more consistent alignment and list marker placement while keeping pure-LTR blocks naturally LTR.
Changes:
- Update Auto-mode CSS to style markdown blocks based on
dir="rtl"/dir="ltr"attributes instead of relying onunicode-bidi: plaintext. - Add Auto-mode webview JS that walks block elements inside
.YBYrtlbubbles and setsdirper block based on presence of RTL characters, with a debounced MutationObserver for streaming updates. - Bump extension version to
0.5.1and document the change via a new spec and changelog entry.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/content.ts | Switch Auto-mode styling to dir-based per-block direction and add a JS block walker + observer to tag blocks during streaming. |
| package.json | Bump extension version to 0.5.1. |
| docs/superpowers/specs/2026-08-04-auto-mode-per-block-direction-design.md | Add design/spec documentation for the per-block direction approach. |
| CHANGELOG.md | Add v0.5.1 entry describing the Auto-mode per-block direction change. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| new MutationObserver(function() { | ||
| if (dirTimer) return; | ||
| dirTimer = setTimeout(function() { | ||
| dirTimer = null; | ||
| scanAll(); |
…rs (Codex round-1 P2) The walker tagged block elements bubble-wide, so a <p>/<li> inside tool UI (toolSummary_/toolBody_ etc.) could receive a native dir attribute, which is not neutralized by the container's LTR direction rules. Blocks are now queried only under [class*="root_"] markdown containers, and the skip selector covers every container the LTR overrides protect. Adds a regression guard test (test/dir-walker-scope.test.cjs) wired into npm test. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ee95d9be83
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| "displayName": "RTL Support", | ||
| "description": "Adds RTL (Right-to-Left) text support for Hebrew, Arabic and Persian to Claude Code in VS Code, Cursor, Antigravity and Kiro", | ||
| "version": "0.5.0", | ||
| "version": "0.5.1", |
There was a problem hiding this comment.
Bumping the extension to 0.5.1 here leaves both version fields in package-lock.json at 0.5.0. This makes the committed package metadata inconsistent and causes tooling that reads the lockfile to report the previous release; regenerate or update the lockfile so its top-level and packages[""] versions are also 0.5.1.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.
Suppressed comments (2)
src/content.ts:923
SKIP_SELis documented as covering every container the LTR overrides protect, but it currently omits several protected tool/thinking/todo containers (e.g.toolBodyGrid_,toolBodyRow_,toolNameText_,thinkingContainer_,todoItem_). If any of those contain block tags (p,li, headings, etc.), the walker can still writedirattributes there, which the LTR override container rules cannot neutralize.
var SKIP_SEL = '[class*="codeBlockWrapper_"],pre,code,[class*="thinkingContent_"],[class*="thinking_"],[class*="toolUse_"],[class*="toolSummary_"],[class*="toolBody_"],[class*="toolResult_"],[class*="toolReference_"],[class*="todoList_"],[class*="todoListContainer_"]';
test/dir-walker-scope.test.cjs:33
- The regression test claims
SKIP_SELcovers every protected container, but the asserted class list is missing several containers that are already forced LTR inltrOverrideRules()(e.g.toolBodyGrid_,toolBodyRow*,toolNameText_,thinkingContainer_,todoItem_). This makes the guard incomplete and could allow future regressions to slip through.
for (const cls of [
'codeBlockWrapper_', 'thinkingContent_', 'thinking_',
'toolUse_', 'toolSummary_', 'toolBody_', 'toolResult_', 'toolReference_',
'todoList_', 'todoListContainer_',
]) {
Both [dir=rtl]/[dir=ltr] block rules now carry the same :not(thinkingContent_) scoping as the sibling markdown rule, so a stray dir attribute under thinking content can never restyle it. Regression guard extended to assert the guard on both rules. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b406945533
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| .YBYrtl [class*="root_"]:not([class*="thinkingContent_"] [class*="root_"]) :is(p, li, h1, h2, h3, h4, h5, h6, blockquote)[dir="rtl"] { | ||
| direction: rtl; | ||
| text-align: right; | ||
| unicode-bidi: isolate; |
There was a problem hiding this comment.
When an RTL-tagged paragraph contains an English link or URL surrounded by Hebrew/Arabic text, isolating only the entire block does not isolate the anchor because unicode-bidi is not inherited. This change also removes the previous anchor-specific unicode-bidi: plaintext rule, so URL punctuation and adjacent text can be reordered by the surrounding RTL bidi context. Preserve an inline isolation rule for anchors while using the new per-block direction rules.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 6 changed files in this pull request and generated no new comments.
Suppressed comments (1)
src/content.ts:922
- The comment above
SKIP_SELsays it includes “every container the LTR overrides protect”, butltrOverrideRules()protects additional containers (e.g.toolBodyGrid_,toolNameText_,todoItem_, etc.). This makes the comment misleading and weakens the value of the regression test, since it’s not clear whether SKIP_SEL is intended to be exhaustive vs “top-level/ancestor” containers only. Reword the comment to reflect the actual intent (top-level/ancestor skip regions) or explicitly reference which set it’s meant to cover.
/* Every container the LTR overrides protect — a native dir attribute on a
child is NOT neutralized by direction rules on the container, so the
walker must never tag inside these */
…nd-3 P2) The CSS rewrite dropped the anchor-specific unicode-bidi rule; since unicode-bidi is not inherited, block-level isolation left URL punctuation reorderable by the surrounding RTL context. Anchors inside markdown blocks get unicode-bidi: plaintext again. Regression guard added. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e3610997a9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| for (var i = 0; i < els.length; i++) { | ||
| var el = els[i]; | ||
| if (el.closest && el.closest(SKIP_SEL)) continue; | ||
| var want = RTL.test(el.textContent || '') ? 'rtl' : 'ltr'; |
There was a problem hiding this comment.
When an otherwise-English list item contains a fenced code block with Hebrew/Arabic text, the li itself is not inside SKIP_SEL, but its textContent includes the skipped code and therefore makes want become rtl. This moves the list marker and can right-align the item's prose even though the only RTL text is in a code block that is supposed to remain untouched. Compute the direction from text nodes outside skipped containers (and nested blocks) rather than from the entire subtree.
Useful? React with 👍 / 👎.
…ound-4 P2) A code block quoting Hebrew inside an English list item made the item's textContent test RTL and flipped it. Direction now comes only from text nodes outside SKIP_SEL containers (TreeWalker with a fast path when the subtree has no RTL characters at all). Regression guard added. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 6 changed files in this pull request and generated no new comments.
Suppressed comments (2)
docs/superpowers/specs/2026-08-04-auto-mode-per-block-direction-design.md:5
- The spec says the scope is
src/content.tsonly, but this PR also adds a regression test and bumps version/changelog. Consider clarifying that this is the runtime/code scope so the doc stays accurate when other supporting files change.
**Scope:** `src/content.ts` only (Auto mode CSS + JS)
test/dir-walker-scope.test.cjs:28
- The SKIP_SEL extraction only matches single-quoted assignments. If the injected JS ever switches to double quotes, this test will fail even though behavior is unchanged; matching both quote styles makes the regression guard less brittle.
const skipSel = (w.match(/var SKIP_SEL = '([^']+)'/) || [])[1] || '';
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 88be133024
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (p && p.closest && p.closest(SKIP_SEL)) continue; | ||
| return true; |
There was a problem hiding this comment.
When an English outer list item contains a nested Hebrew/Arabic item, hasOwnRtl walks into the nested li and this check excludes only skipped UI containers, so the outer item also receives dir="rtl" and moves its English text and marker to the wrong side. Fresh evidence in the updated walker is that the new TreeWalker filters SKIP_SEL ancestors but still accepts RTL text whose nearest BLOCK_SEL ancestor is a descendant of el; exclude text owned by nested blocks when determining the parent block's direction.
Useful? React with 👍 / 👎.
| var bubble = el && el.closest ? el.closest('.YBYrtl') : null; | ||
| if (!bubble) { pendingFull = true; return; } |
There was a problem hiding this comment.
When a pure-English response streams after a long RTL conversation, each character-data mutation has no .YBYrtl ancestor, so this branch sets pendingFull and the timer repeatedly calls scanAll(), traversing every block in every prior RTL bubble about once per debounce interval. This makes unrelated LTR streaming increasingly expensive as chat history grows; inspect added or newly classed RTL bubbles directly and ignore other non-RTL targets instead of falling back to a full scan.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 6 changed files in this pull request and generated no new comments.
Suppressed comments (1)
src/content.ts:944
hasOwnRtlbuildsel.textContentas a fast-path, but for blocks that contain RTL text this forces a full subtree traversal + string allocation and then immediately does a second traversal viaTreeWalker. In large bubbles (especially with big code/tool chunks), this double-walk can add noticeable overhead during streaming.
Consider removing the textContent fast-path and doing a single TreeWalker pass with early-exit when an RTL text node outside SKIP_SEL is found.
function hasOwnRtl(el) {
if (!RTL.test(el.textContent || '')) return false; /* fast path */
var walker = document.createTreeWalker(el, NodeFilter.SHOW_TEXT, null, false);
var n;
while ((n = walker.nextNode())) {
if (!RTL.test(n.nodeValue)) continue;
var p = n.parentElement;
if (p && p.closest && p.closest(SKIP_SEL)) continue;
…dex round-5 P2s) 1) hasOwnRtl now only accepts RTL text whose nearest block ancestor is the judged element, so a Hebrew sub-item no longer flips its English parent item (the nested block gets its own dir). 2) The mutation observer never falls back to a full-history scan: mutations outside bubbles are ignored, and new bubbles are detected directly from addedNodes and class changes — unrelated LTR streaming no longer re-walks every prior RTL bubble per debounce tick. Regression guards added for both. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7941df48ba
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| var p = n.parentElement; | ||
| if (!p) continue; | ||
| if (p.closest && p.closest(SKIP_SEL)) continue; | ||
| if (p.closest && p.closest(BLOCK_SEL) !== el) continue; /* owned by a nested block */ |
There was a problem hiding this comment.
When Markdown renders a loose or multi-paragraph list item as <li><p>…Hebrew…</p></li>, the paragraph is the text node's nearest BLOCK_SEL ancestor, so this check rejects all of the item's RTL text and the li is assigned dir="ltr". Although the child paragraph becomes RTL, the list marker belongs to the li and therefore remains on the left. Exclude independently nested list items while still counting paragraph content owned by the current li.
Useful? React with 👍 / 👎.
…ound-6 P2) The round-5 ownership rule rejected any text whose nearest block ancestor wasn't the judged element, so <li><p>Hebrew</p></li> left the li LTR with its marker on the wrong side. Ownership now stops only at independently rendered nested blocks (li, blockquote) — p and headings inside a loose list item count toward the item, whose marker they visually belong to. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 6 changed files in this pull request and generated no new comments.
Suppressed comments (2)
src/content.ts:928
- The comment above SKIP_SEL says it includes every container the LTR override rules protect, but ltrOverrideRules() also forces LTR for additional tool/thinking/todo containers (e.g. toolBodyGrid_/toolBodyRow_/thinkingContainer_/thinkingHeader_/spinnerRow_/timelineMessage_/todoItem_) that are not listed in SKIP_SEL. Either expand SKIP_SEL to truly mirror the protected containers, or soften the comment to avoid implying completeness.
/* Every container the LTR overrides protect — a native dir attribute on a
child is NOT neutralized by direction rules on the container, so the
walker must never tag inside these */
test/dir-walker-scope.test.cjs:6
- This header comment says the walker skip selector must cover every container the LTR overrides protect, but the test only asserts a subset of the LTR-protected containers (and ltrOverrideRules() lists more). Consider rewording this comment (or expanding the asserted list) so the test’s intent matches what it actually verifies.
* Regression guard for the Auto-mode per-block direction walker
* (Codex round-1 P2, PR #2): the walker must only tag blocks inside
* markdown containers, and its skip selector must cover every container
* the LTR overrides protect — a native dir attribute on a child is not
* neutralized by direction rules on its container.
|
Codex Review: Something went wrong. Try again later by commenting “@codex review”. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
@codex review |
|
Codex Review: Didn't find any major issues. Keep them coming! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Summary
Auto mode previously used
unicode-bidi: plaintexton response paragraphs, which picks direction from the first strong character. In Hebrew conversations this caused: English-first mixed lines drifting left, list bullets on the wrong side, and hard-to-read mixed Hebrew/English text.This PR switches Auto mode to per-block direction detection: a JS walker sets
dir="rtl"/dir="ltr"on each block element (p,li,h1–h6,blockquote) inside.YBYrtlbubbles by presence of Hebrew/Arabic/Persian characters, and the CSS keys off thedirattribute instead ofplaintext.delta sound) → stay naturally left-alignedclassonly so its owndirwrites can't self-triggerSpec:
docs/superpowers/specs/2026-08-04-auto-mode-per-block-direction-design.mdTesting
npm run buildclean,npm test(concurrency, 8 simulated windows) passesRound 0 local pre-review skipped (Codex plugin slash commands unavailable in this session); cloud loop is the gate.
🤖 Generated with Claude Code