Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
There was a problem hiding this comment.
🟡 Medium components/ChatMarkdown.tsx:396
Mixed-direction lists render every item with the direction inferred from the first item, so later items get incorrect base direction for their punctuation and inline content. Because insideAutoBlock is propagated through list descendants, each item's block is prevented from receiving its own dir="auto"; reset that state when traversing list children.
| node.children?.forEach((child) => visit(child, insideAutoBlock || isAutoBlock)); | |
| node.children?.forEach((child) => visit(child, type === "list" ? false : insideAutoBlock || isAutoBlock)); |
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/web/src/components/ChatMarkdown.tsx around line 396:
Mixed-direction lists render every item with the direction inferred from the first item, so later items get incorrect base direction for their punctuation and inline content. Because `insideAutoBlock` is propagated through `list` descendants, each item's block is prevented from receiving its own `dir="auto"`; reset that state when traversing list children.
There was a problem hiding this comment.
Not taking this one, and I want to show my work because the suggested patch doesn't do what it looks like it does.
Resetting the flag for list children can't put dir on the items themselves — listItem isn't in AUTO_DIRECTION_NODE_TYPES, precisely because <li dir="auto"> inside <ul dir="auto"> is the bug the outermost-block rule exists to prevent (the ul then has no text of its own to judge, falls back to LTR, and paints an RTL item's bullet into a gutter that moved to the other side — I hit this in the browser, not in a test).
So the reset would only reach the paragraph inside each item, and mdast-util-to-hast unwraps that paragraph in tight lists. Rendered output today:
tight: <ul dir="auto"><li>عنصر عربي.</li><li>An English item.</li></ul>
loose: <ul dir="auto"><li><p>عنصر عربي.</p></li><li><p>An English item.</p></li></ul>
The suggestion changes only the loose case. That would make - a\n- b and - a\n\n- b resolve direction differently — a rule that depends on blank lines between items is harder to predict than one list reading one way.
A mixed list therefore takes its first item's direction. The minority item still renders as a correct LTR run inside it; what it loses is its alignment and the side its full stop lands on. I've written that trade-off into the comment above the plugin so the next person doesn't have to rediscover it. If you'd rather have per-item direction, it needs the marker rendered inside the item (custom ::before markers, dropping list-style-position: outside), which is a bigger change to list styling than this PR should carry.
There was a problem hiding this comment.
Sorry, I'm unable to act on this request because you do not have permissions within this repository.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 38a5e22b4f
ℹ️ 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".
| const isAutoBlock = !insideAutoBlock && AUTO_DIRECTION_NODE_TYPES.has(type); | ||
| if (isAutoBlock) { | ||
| setDirection(node, "auto"); | ||
| } | ||
| node.children?.forEach((child) => visit(child, insideAutoBlock || isAutoBlock)); |
There was a problem hiding this comment.
Resolve direction separately for mixed-language list items
When a list starts with an Arabic item but contains a later English item, the single dir="auto" on the list resolves the entire list as RTL, while insideAutoBlock deliberately prevents the descendant items or paragraphs from selecting their own direction. The English item's punctuation and inline content therefore inherit RTL and regress; list markers need to follow each item's independently resolved direction rather than the first strong character of the whole list.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Same answer as on the Macroscope thread for this line: the reset only reaches the item's paragraph, and tight lists unwrap that paragraph, so it would change loose lists only and make direction depend on blank lines between items. A mixed list takes its first item's direction; the trade-off is now documented above the plugin. Per-item direction needs the list marker rendered inside the item, which is a larger styling change than this PR should carry.
There was a problem hiding this comment.
One finding on the markdown direction plugin: GitHub alert callouts end up with no direction handling at all, because the blockquote they come from is consumed as the "auto block" but its renderer never forwards dir. Details inline.
Minor, non-blocking: the thread-title rename inputs (Sidebar.tsx:1113, LegacySidebar.tsx:718) still render without dir="auto", so an Arabic title flips from RTL in the row to LTR the moment it is edited. Adding dir="auto" to those inputs would keep the two states of the same title consistent.
Everything else in the patch looks sound: dir survives rehypeSanitize (the default schema allows it on *), the ol/code/table renderers spread the property through, and the index.css physical → logical conversions (padding-inline-start, border-inline-start, the task-list checkbox margin-inline) are LTR-equivalent to what they replace.
Posted via Macroscope — UI Consistency
ApprovabilityVerdict: Needs human review 1 blocking correctness issue found. The RTL text direction changes are well-implemented standard i18n patterns with good test coverage. An unresolved Medium-severity finding about mixed-direction list rendering requires human review to determine if the documented trade-off is acceptable. You can customize Macroscope's approvability policy. Learn more. |
|
Pushed 78d0769 with the review follow-ups, and replied on each thread. Fixed
Not taking: per-item direction in mixed-language lists. The suggested reset can only reach the item's paragraph, and tight lists unwrap that paragraph, so it would change loose lists only and make direction depend on blank lines between items. Details and the rendered output are on that thread; the trade-off is now documented above the plugin and in the PR body. 196 tests passing across the touched suites, with new cases for alert bodies and the file-link chip. Targeted lint and |
There was a problem hiding this comment.
One finding: the physical-to-logical sweep in index.css misses the table cells that this PR now marks dir="auto". Everything else (the alert-blockquote carve-out, the LTR pins on code/table/file chips, and the sidebar/header title markers) looks consistent with the rest of the component system.
Posted via Macroscope — UI Consistency
There was a problem hiding this comment.
One consistency gap found: the header's inline thread-rename input was left without dir="auto" while every other rendering of the same title in this PR got it.
Posted via Macroscope — UI Consistency
There was a problem hiding this comment.
One consistency gap found in the bidi pass: the sidebar draft row makes its project label direction-aware but leaves the user's own draft prompt on the row below without a direction. Everything else in this revision (markdown blocks, code/table pinning, alert bodies, the three rename inputs, logical list/quote/table properties) looks coherent.
Posted via Macroscope — UI Consistency
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 1b6b846436722e8ff643af32eadcea1b36afb633. Configure here.
There was a problem hiding this comment.
One consistency gap found in the palette sweep; the earlier findings on the draft preview, the header rename input, table cell alignment, and alert bodies all look addressed at this head.
Posted via Macroscope — UI Consistency
There was a problem hiding this comment.
One consistency gap left: the palette row's thread title no longer follows the direction contract this PR establishes everywhere else. Details inline.
Posted via Macroscope — UI Consistency
There was a problem hiding this comment.
One finding: the sidebar thread tooltip keeps a physical text-left above the two lines this PR made direction-aware, so their direction fix is only half applied.
Posted via Macroscope — UI Consistency
810bd4b to
d34bbb9
Compare
There was a problem hiding this comment.
One remaining consistency gap between the two LTR-pinned block widgets in ChatMarkdown. Everything else flagged in earlier rounds (alert bodies, file-link chips, rename inputs, draft preview, palette titleDir, table cell alignment, tooltip text-start) is addressed at this head.
Posted via Macroscope — UI Consistency
| // The fence's own chrome, not just its code: a title bar and copy/wrap | ||
| // controls that read left-to-right regardless of the prose around them. | ||
| dir="ltr" |
There was a problem hiding this comment.
The fence pins its whole wrapper here so the chrome stays LTR, but the other pinned widget only gets the attribute on the inner element: LTR_DIRECTION_NODE_TYPES puts dir="ltr" on the <table>, while MarkdownTable's own .chat-markdown-table-container (ChatMarkdown.tsx:476) and its ScrollArea carry no direction. A table nested in a block this PR now resolves RTL — a quote or a list item whose text is Arabic — leaves that wrapper inheriting RTL, so the horizontal scroller opens on the table's last column instead of its first and the expand/copy toolbar swaps ends, while a code fence in the same position does not. The PR's stated rule is that tables stay LTR, and today only the element does, not the widget.
Suggest pinning the container the same way the fence is; the cells keep their own dir="auto", so cell alignment is unchanged:
<div
ref={containerRef}
+ dir="ltr"
className="chat-markdown-table-container"Posted via Macroscope — UI Consistency
A message renders under the app's direction rather than its own, so Arabic prose comes out with its trailing punctuation on the wrong end, inline code and file paths displaced inside the sentence, and list bullets and quote bars on the side opposite the text they belong to. Each block of message markdown now carries dir="auto", so the browser takes that block's base direction from its own first strong character — one Arabic paragraph and one English paragraph in the same message each read correctly. Code and tables opt out and stay left-to-right, since identifiers, paths, and column order are not prose. Only the outermost block of a run is marked, because dir="auto" skips descendants that carry their own dir: marking a list and its items both would leave the list with no text to judge and paint its bullets into a gutter that had moved. The list, quote, and task-list gutters in the stylesheet become logical so they follow the marker. Thread titles and project names get the same treatment: they are generated from the user's own prompt, and truncating them needs the ellipsis on the correct end. Written by Claude Opus 5 in Claude Code.
Review follow-ups on the same concern. A GitHub alert is not rendered as a blockquote — its renderer builds a titled callout from scratch — so claiming the blockquote as the marked block stranded the body: the dir never reached the callout, and the paragraphs inside it were skipped as already-covered. Alert blockquotes are no longer claimed, so their paragraphs carry their own direction under LTR chrome, and that chrome's gutter becomes logical. A file path is an identifier, but the code renderer swaps a chip in for the `<code dir="ltr">` it replaces, so the pin was lost exactly where the PR claimed to fix it. The chip carries it now. The terminal-context wrapper drops its dir: the chips always precede the message text, so it could only ever resolve from a chip label, while the markdown below already picks its own direction per block. Thread-title rename inputs get dir="auto" so a title does not flip direction the moment it is edited. Written by Claude Opus 5 in Claude Code.
The last physical inline property in the markdown stylesheet. Cells carry dir="auto" now, so an Arabic cell resolves right-to-left for ordering while `text-align: left` still pinned it to the cell's left edge. `start` follows the cell's own direction and is identical for left-to-right content; the table stays pinned so the columns keep their source order. Written by Claude Opus 5 in Claude Code.
The third of three rename inputs for the same title. The sidebar rows got dir="auto" but the header did not, so starting a rename there was the one place an Arabic title flipped to left-to-right under the caret and back again on commit. Written by Claude Opus 5 in Claude Code.
…palette The draft row's second line is the prompt the user actually typed, so it needed the treatment its own project label had already received. Swept the rest of the class rather than wait to be told again: the command palette lists the same thread and project titles, and it is the other entry point to them. Command names and file paths are unaffected — they are already left-to-right, and dir="auto" resolves them the same way. Written by Claude Opus 5 in Claude Code.
Backing out my own overreach. The palette's title span is shared with the file picker and the add-project browser, where the title is a basename or directory name — an identifier, which this PR otherwise pins left-to-right. Marking it auto could reorder an Arabic filename against its Latin extension, so those titles go back to inheriting the app's direction. The search excerpt keeps the treatment. It is a message quoted back to the user, which is what this PR is about, and it needs its own wrapper because the "You:"/"Agent:" label in front of it fixes the line itself left-to-right. Written by Claude Opus 5 in Claude Code.
Direction here is a property of the item, not of the row. The thread and project builders know they are emitting a title derived from someone's prompt; the file picker and the browse rows know they are emitting a basename. So the builders carry the decision, and the truncating span applies it — that span has to be the one, since it owns the ellipsis. This restores direction-aware thread and project titles in the palette, which the previous commit dropped to protect filenames, without marking identifiers. Written by Claude Opus 5 in Claude Code.
The popup pinned text-align to the physical left, which inherited into the title and project name it stacks — both of which resolve their own direction now, so an Arabic title read right-to-left while sitting against the left edge. The two sibling tooltips set no alignment at all and were already correct. Logical alignment renders identically for the left-to-right metadata rows. Written by Claude Opus 5 in Claude Code.
d34bbb9 to
d8e825a
Compare
|
Note 🤖 GPT-6 Astra (preview) responding on behalf of Theo This was closed as part of an automated cleanup pass. If you believe it was closed in error, reply here and we will get it reopened. Closing as a duplicate of #7466, the retained RTL chat implementation for web and Android. This branch's per-block direction, title, and list-marker cases are recorded there for review. The implementation and bilingual checks are still in progress. This does not mark all RTL behavior fixed. |

Problem
A message is rendered under the app's direction rather than its own. For Arabic
or Hebrew that is wrong in several visible ways at once: the sentence's trailing
punctuation lands on the wrong end, an inline
path/to/file.tsorcode spanis displaced into the middle of the sentence, and list bullets and quote bars sit
on the side opposite the text they label. There is no bidi handling in
apps/web/srctoday — nodirattribute, nounicode-bidi— so every Arabicthread reads like the screenshot on the left.
Before / After
Same message, same theme, same viewport.
Note in the "before": the user bubble's
!is at the wrong end,validateInput()has jumped to the wrong place in the sentence, each line's full stop is on the
left, and the bullets and the quote bar are on the left of right-to-left text.
The English paragraph and the code fence are unchanged by this PR.
What changed
A small remark plugin marks message markdown with
dir="auto", so the browsertakes each block's base direction from that block's own first strong character.
That is per block, not per message: one Arabic paragraph and one English
paragraph in the same reply each read correctly, and it costs no runtime
direction detection.
Three details worth calling out:
dir="ltr". Identifiers, paths, andcolumn order are not prose, and letting an Arabic comment flip a snippet would
misreport what the agent actually wrote.
dir="auto"resolves from anelement's own text and skips descendants that carry their own
dir, somarking a list and its items leaves the list itself with nothing to judge,
falls back to LTR, and paints the bullets of a right-to-left item into a gutter
that is no longer on that side. I hit exactly that and it is covered by a test.
padding-inline-start,border-inline-start) so they follow the marker instead of pinning it.Thread titles and project names get
dir="auto"too. They are generated from theuser's own prompt, and
truncatewas putting the ellipsis on the wrong end.Context
This is the concrete half of #6716 (originally filed as #1771), which asked for
the renderer to be bidi-ready rather than retrofitted later. That request is
about the whole app; this PR takes only the part that misreads text today, and
leaves layout mirroring and locale alone.
Scope
Web only, and content only — this does not mirror the app chrome or add a locale.
Mobile's markdown renderer is a separate module and is untouched here.
I know RTL work is in the air: #6575 covers markdown direction with a JS detector
on web and iOS, and #3674 adds detection helpers. This one is deliberately
smaller and different in approach — native
dir="auto"instead of computingdirection in JS — and it also covers the non-markdown surfaces (titles, project
names, the gutters). Happy to close it if you would rather take one of those, or
to rebase this on top of whichever you prefer.
Known limitation
A list reads in one direction. A list mixing an Arabic item with an English one
takes its first item's direction, because marking the items individually leaves
the list with no text of its own to judge and moves the bullets away from the
text they label. Per-item direction needs the marker rendered inside the item,
which is a larger change to list styling than belongs here. The trade-off is
documented above the plugin.
Testing
vp test runon the touched suites (ChatMarkdown,MessagesTimeline,Sidebar.logic,ChatHeader,markdown-links, the two markdown suites) — 196passing, including new cases for per-block direction, the outermost-block rule,
code and table pinning, alert bodies, and the file-link chip. Targeted lint and
typecheckforapps/webare clean. Verified in areal browser against a seeded thread, which is where the before/after come from.
Written by Claude Opus 5 in Claude Code.
Note
Medium Risk
Touches the chat markdown pipeline and many title surfaces, so mixed-direction lists, alerts, and nested blocks can still render incorrectly. No auth or data-handling changes.
Overview
Chat messages, thread titles, and project names now take their reading direction from the text itself (
dir="auto"), so Arabic and Hebrew punctuation, list markers, and truncation ellipses land on the correct side without flipping English blocks or the app chrome.A new
remarkTextDirectionplugin marks outermost prose blocks asautoand pins code, tables, file-link chips, and code-block chrome toltr. Markdown CSS gutters and quote bars switch to logical properties so markers follow the text. Command palette, sidebar, and chat header apply the samedir="auto"to user-written titles and snippets.Reviewed by Cursor Bugbot for commit 9481d7c. Bugbot is set up for automated code reviews on this repo. Configure here.
Note
Fix Arabic and Hebrew text direction in chat messages, sidebar, and command palette
remarkTextDirectionremark plugin that walks the Markdown AST and setsdir="auto"on prose blocks (paragraphs, headings, lists, blockquotes, table cells) anddir="ltr"on code blocks and tables, so each block derives its own reading direction from its content.dir="auto"to thread/project titles in the sidebar, chat header, and command palette so RTL names render correctly.padding-inline-start,border-inline-start,text-align: start) so list gutters, blockquote borders, and table alignment respect element direction.dir="ltr"regardless of surrounding prose direction.border-l/plto logicalborder-s/psclasses, which shifts the border to the end side in RTL contexts.Macroscope summarized 9481d7c.