Skip to content

feat(editor): RTL text-direction toggle for the built-in editor - #16291

Open
alefbt wants to merge 15 commits into
stablyai:mainfrom
alefbt:feat/editor-rtl-text-direction
Open

feat(editor): RTL text-direction toggle for the built-in editor#16291
alefbt wants to merge 15 commits into
stablyai:mainfrom
alefbt:feat/editor-rtl-text-direction

Conversation

@alefbt

@alefbt alefbt commented Aug 24, 2026

Copy link
Copy Markdown

ELI5

A Hebrew or Arabic document in Orca's editor is laid out as if it were English: the text hugs the left edge, because the editor has no notion of a document being right-to-left. This adds a small button in the editor header — plus a … menu item, a shortcut, and a Settings row — that marks a file as RTL so it lays out from the right, the way an RTL reader expects. It's off by default and nothing changes for anyone editing LTR code.

What Changed

A global default plus a per-file override.

  • settings.editorTextDirection: 'ltr' | 'auto' | 'rtl', default 'ltr'. 'auto' is the conservative middle setting: it sets each line's base direction from its first strong character without changing alignment.
  • editorTextDirectionByFile: Record<fileId, 'ltr' | 'rtl'> on the workspace session. Most people have one or two RTL documents among LTR code, so the toggle sets a per-file override rather than flipping the global default. Absent entry means follow Settings.

Four surfaces, mirroring how Word Wrap shipped in #10086:

Surface Behavior
Header icon button Revealed only once the file actually holds strong RTL text
… actions menu checkbox Always present, so discoverability doesn't depend on the button
editor.toggleTextDirection Unbound by default
Settings segmented control LTR / Auto / RTL

All of them set the per-file override, and clear it when it matches the global default so the file falls back to Settings instead of pinning a stale value. Diff surfaces are excluded — Monaco's diff sub-editors keep their own LTR-only layout math.

How direction reaches Monaco. Monaco 0.55 exposes no direction editor option (upstream VS Code RTL support is still open), so it ships as a container class whose CSS is scoped to .view-line:

.editor-dir-rtl .monaco-editor .view-line { direction: rtl; text-align: right; }
.editor-dir-auto .monaco-editor .view-line { unicode-bidi: plaintext; }

Scoping to .view-line rather than .view-lines is deliberate: it keeps the gutter, minimap and scrollbar LTR, where Monaco's own coordinate math expects them. Monaco sets no unicode-bidi or direction of its own on .view-line, so there is no specificity fight.

Being precise about the observed effect, because I measured it rather than assuming it. Monaco already renders bidi runs through the browser, so glyph shaping and intra-line run order are not what is missing today. What is missing is base direction, and the effect I can demonstrate is layout:

  • 'rtl' lays the document's lines out flush right — asserted in the e2e by measuring line 1's box inside .view-lines.
  • 'auto' applies unicode-bidi: plaintext (the Unicode first-strong-character rule) and leaves alignment alone, so lines stay flush left. That is the right default for mixed files: you do not want alignment flipping line to line as a file alternates between prose and code.

I first wrote that this also corrects trailing-punctuation placement and intra-line run order. Testing against the real app did not support that, so I removed the claim rather than ship a description the screenshots contradict. The value here is base direction and alignment — which is the actual complaint when you open a Hebrew file.

Rich Markdown gets real bidi. The TipTap surface is contenteditable, where dir is natively supported rather than best-effort, so it receives the same resolved direction as a real dir attribute.

Why

Orca has no RTL support anywhere in the editor. Verified against main: no dir= in any renderer source, no direction: rtl in src/renderer/src/assets/, no direction field in settings. Every other direction hit in the repo is split-pane layout.

But the project already ships best-effort RTL work in the terminal — terminal-arabic-shaping-joiner.ts joins strong-RTL runs so the browser can shape and reorder them, explicitly framed as a workaround for xterm's missing bidi (#5262, xterm.js#701). This is the same bargain for the editor, framed the same way: opt-in, default 'ltr', best-effort. Because Monaco's layout is LTR-only, indent guides, whitespace rendering and column selection will have rough edges in 'rtl'. That is a real limitation, stated up front, and it is why nothing changes unless a user opts in.

Three supporting changes, all needed to stay inside existing line budgets rather than raise them (per AGENTS.md):

  • isStrongRtlCodePoint / containsStrongRtlText move from the terminal joiner to src/shared/strong-rtl-text.ts. The editor needs the same detection, and importing from a terminal-prefixed module would violate the file-naming rule.
  • hydrateFileKeyedOverrides, so front matter and direction share one migration loop instead of duplicating it.
  • buildEditorSessionData and persistedOpenFileSchema split into sibling modules, matching the existing workspace-session-browser-schema.ts (whose header comment says it exists for exactly this reason).

removeMarkdownVisibilityKeys became the generic removeFileKeyedEntries — it was never visibility-specific.

Linked Issue

Fixes #16259

Visual Proof

Captured by the new e2e spec, which attaches both frames as Playwright artifacts:

pnpm run ensure:electron-runtime && npx playwright test tests/e2e/editor-rtl-text-direction.spec.ts \
  --config tests/playwright.config.ts --project=electron-headless --workers=1

Three frames, using a file whose first line is שלום עולם ABC so the RTL and LTR runs are both visible:

ltr (before) — the line is flush left against the gutter.

rtl (after) — the same line is flush right. const answer = 42 keeps its internal LTR order, and the line-number gutter stays on the left, confirming the .view-line scoping did its job.

auto — flush left like ltr, with per-line base direction applied. Byte-different from the ltr frame, but the difference is subtle by design; auto is the conservative mode.

I couldn't attach the PNGs inline — no gh image extension is available in this environment and the template rightly forbids committing them. The command above regenerates all three in test-results/.

Testing

Locally on macOS: pnpm lint, pnpm typecheck, pnpm test, pnpm build, and the new e2e spec all pass.

Reviewer steps:

  1. Create a file with Hebrew or Arabic text and open it in the editor.
  2. The direction button appears in the header; the … menu shows Right-to-Left.
  3. Click it — the document becomes flush right, and the gutter stays LTR.
  4. Quit and relaunch; the file reopens RTL. Toggle back and the override clears rather than pinning ltr.
  5. Settings → Editor → Editor Text Direction offers LTR / Auto / RTL. Auto keeps lines flush left while setting per-line base direction; RTL right-aligns them.
  • I manually tested these changes locally
  • Automated tests added/updated, or explained why not below

AI Disclosure

Claude Opus 5 (Claude Code) — used for the implementation, tests, and this description.

Review

Cross-platform. No paths, no child processes, no platform branches. The shortcut is unbound by default, so no cross-platform chord is claimed (#6169 precedent). Only surfaces to check on Linux/Windows are the button and Settings labels, which use existing primitives.

Remote / SSH / wire compatibility. editorTextDirectionByFile is a new optional field on WorkspaceSessionState, which docs/reference/remote-wire-compatibility.md classifies as safe: an old client paired with a new host ignores it. Registered 'fileKeyed' in the ownership map, so it isn't silently dropped on non-local hosts. No RPC params, stream frames, or opcodes changed.

Folder workspaces. Nothing here assumes a git worktree — the override is keyed by editor fileId, which folder workspaces already use.

Performance. The button's reveal check is a bounded 8KB prefix scan, and containsStrongRtlText bails out on a single charCodeAt comparison for ASCII, so LTR files exit almost immediately. Direction resolution is two comparisons. The CSS adds two selectors that match nothing unless a class is present.

Backwards compatibility. Default 'ltr'; a profile saved before this preference existed resolves to 'ltr', and a corrupted persisted value falls back to 'ltr' rather than reaching CSS (covered by tests).

Security. No new IPC, no new file or network access, no user content interpolated into anything executable.

Agent skill upstream boundary

  • Not applicable — no skill-installer source, tests, fixtures, or docs touched.

Notes

The honest caveat worth a maintainer's judgment: 'rtl' is best-effort because Monaco's layout is LTR-only. Caret placement works (Monaco queries the DOM for client rects, which are bidi-aware — verified in the e2e by clicking into RTL text), but indent guides, whitespace rendering, and column selection compute left offsets and will likely look off. I have not stress-tested those.

Because the demonstrated benefit is narrower than "full RTL support" — base direction and alignment, not new bidi shaping — it's fair to ask whether it earns four surfaces. I'd defend the button and the Settings row; the shortcut is unbound so it costs nothing; the … menu item is the one I'd cut first if you want this smaller.

Happy to split this into two PRs (global setting + surfaces, then the per-file override) if the diff is too large to review in one pass. The per-file override is an isolated commit.

Checklist

  • This PR is small and focused
  • I explained what changed and why (including ELI5)
  • Before/after screenshots or videos attached for UI changes, or N/A with reason
  • Self-reviewed for correctness, security, and performance
  • Cross-platform, SSH/remote, and path/shortcut impact considered (or N/A)
  • pnpm lint, pnpm typecheck, pnpm test, and pnpm build pass (or CI will cover; local preferred)

@Yehuda-Ness
refactor(rtl): extract strong-RTL predicates into src/shared
b96d6d9 
The terminal joiner owned isStrongRtlCodePoint and a private
containsStrongRtlText. The editor needs the same detection, and importing
it from a terminal-prefixed module would violate the file-naming rule in
AGENTS.md, so both predicates move to src/shared/strong-rtl-text.ts.

No behavior change.
@Yehuda-Ness
feat(editor): add an RTL/auto text-direction setting for the file editor
a3ab078 
Monaco 0.55 exposes no direction option and upstream VS Code RTL support is
still open, so direction ships as a container class scoped to .view-line.
The gutter, minimap and scrollbar stay LTR, where Monaco's own layout math
expects them. 'auto' uses CSS unicode-bidi: plaintext, which is the Unicode
first-strong-character rule, so a mixed prose-and-code file needs no
heuristic of ours.

Defaults to 'ltr': this is best-effort, like the terminal's Arabic shaping
joiner, and must not change LTR editing for anyone who does not opt in.

Refs stablyai#16259
@Yehuda-Ness
feat(editor): persist a per-file text-direction override
87ce098 
Most users have one or two RTL documents among LTR code, so the toggle sets
a per-file override rather than flipping the global default. Absent entry
means follow settings.editorTextDirection.

Mirrors markdownFrontmatterVisible exactly: 'fileKeyed' ownership, plus
hydration, rename-rekey, close, worktree-teardown and recently-closed
cleanup. The field is new and optional, so remote wire compatibility holds.

Two supporting extractions, both required to stay inside existing line
budgets rather than raise them:
- hydrateFileKeyedOverrides, so front matter and direction share one
  migration loop instead of duplicating it
- buildEditorSessionData and persistedOpenFileSchema into sibling modules,
  matching workspace-session-browser-schema.ts

removeMarkdownVisibilityKeys becomes the generic removeFileKeyedEntries; it
was never visibility-specific.

Refs stablyai#16259
@Yehuda-Ness
feat(editor): expose the direction toggle as a button, menu item and …
86b08a3 
…shortcut

Three surfaces, mirroring how Word Wrap shipped in stablyai#10086:

- Header icon button, revealed only once the file actually holds strong RTL
  text (a bounded 8KB prefix scan reusing the terminal joiner's predicate),
  so LTR-only headers gain no chrome.
- Checkbox in the '...' actions menu, always present, so discoverability
  does not depend on the button.
- editor.toggleTextDirection, unbound by default. Word Wrap could claim
  Alt+Z because VS Code uses it; direction has no such precedent, so users
  assign their own (stablyai#6169).

All three set the per-file override and clear it when it matches the global
default, so a file falls back to Settings instead of pinning a stale value.
Diff surfaces are excluded: Monaco's diff sub-editors keep their own
LTR-only layout math.

The TipTap rich-Markdown surface gets the resolved direction as a real
'dir' attribute, where contenteditable gives genuine bidi rather than
Monaco's best effort.

Refs stablyai#16259
@Yehuda-Ness
test(editor): cover the RTL direction toggle across unit and e2e layers
c0f012e 
Unit: direction resolution and class mapping, the per-file override
lifecycle, the content-gated header button, the menu item, the Settings
segmented control, session hydration, and patch pruning for a file that is
no longer open.

E2E: drives the real Electron app — opens a Hebrew file, asserts the button
only appears because the file holds strong RTL text, toggles it, and checks
that .editor-dir-rtl lands and the override clears on the way back.
Attaches before/after screenshots of the Monaco surface.

The patch-key fixtures needed updating because the editor patch group now
carries editorTextDirectionByFile.
@Yehuda-Ness
test(editor): add editorTextDirectionByFile to the MonacoEditor store…
d2e4af6 
… mocks

Both mocks build a partial store state by hand, so the new selector read
crashed on undefined. The real store always initializes the record in
createEditorDraftState.
@Yehuda-Ness
fix(editor): make the direction toggle reversible, and assert layout …
0309556 
…not CSS

Two problems found by driving the real app rather than trusting the tests.

1. The toggle pinned the opposite direction instead of clearing the
   override, so with a global 'auto' default a file could never get back to
   'auto' -- it ping-ponged between pinned ltr and rtl forever. Now an
   existing override is always cleared, which makes the control reversible
   for every global default.

2. The e2e asserted that the CSS class landed, not that anything rendered
   differently -- it passed while proving nothing. It now measures line 1's
   box inside .view-lines and asserts alignment: left for ltr and auto,
   right for rtl. The fixture line is 'shalom olam ABC' so an RTL and an
   LTR run are both on screen.

That measurement also disproved a claim I had written into the issue: this
does not fix trailing-punctuation placement or intra-line run order. Monaco
already renders bidi runs through the browser. What was missing is base
direction, and the demonstrable effect is alignment.
@alefbt
alefbt requested a review from brennanb2025 as a code owner August 24, 2026 19:19
@coderabbitai

coderabbitai Bot commented Aug 24, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Review Change Stack

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 2e2d0ee5-3028-4525-a1fd-ba56e50b0379

📥 Commits

Reviewing files that changed from the base of the PR and between afd9453 and 3f134a2.

📒 Files selected for processing (1)
  • tests/e2e/editor-rtl-text-direction.spec.ts

Included review availability: Your plan provides up to 10 included reviews per hour; 8 remain after this review.

📝 Walkthrough

Walkthrough

Adds global LTR, automatic, and RTL editor settings with per-file LTR or RTL overrides. Detects strong RTL content and exposes direction controls for editable files. Applies RTL and automatic direction styling to Monaco and rich markdown editors. Adds keyboard shortcut handling and localized controls. Persists, hydrates, migrates, and cleans per-file overrides across editor and worktree lifecycle changes. Adds unit, component, and end-to-end coverage.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 14.29% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 42 functions across 54 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely describes the primary change: adding an RTL text-direction toggle to the built-in editor.
Description check ✅ Passed The description is complete and structured. It explains the change, rationale, linked issue, visual behavior, testing, AI use, compatibility considerations, and checklist status. Visual proof is descr…
Linked Issues check ✅ Passed The implementation addresses issue #16259. It adds the requested controls, global and per-file direction modes, automatic detection, scoped Monaco styling, TipTap direction support, persistence and cl…
Out of Scope Changes check ✅ Passed The supporting refactors and shared utilities are directly required for RTL detection, session persistence, migration, cleanup, and file-keyed state handling. No unrelated code changes are evident.
Full details: Description check

Explanation

The description is complete and structured. It explains the change, rationale, linked issue, visual behavior, testing, AI use, compatibility considerations, and checklist status. Visual proof is described with a reproducible e2e command, although inline attachments are unavailable.

Full details: Linked Issues check

Explanation

The implementation addresses issue #16259. It adds the requested controls, global and per-file direction modes, automatic detection, scoped Monaco styling, TipTap direction support, persistence and cleanup handling, opt-in defaults, and unit/e2e coverage.

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

coderabbitai[bot]
coderabbitai Bot reviewed Aug 24, 2026
View reviewed changes

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 5

🧹 Nitpick comments (3)
tests/e2e/editor-rtl-text-direction.spec.ts (1)

12-14: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Shorten this test comment.

Replace the three-line explanation with one concise statement of the test condition.

As per coding guidelines, comments must be concise, non-obvious, and brief.

Source: Coding guidelines

src/renderer/src/components/editor/EditorTextDirectionButton.tsx (1)

25-37: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Use intent-named localization IDs for new strings.

New strings must not use the grandfathered hash-based ID format.

  • src/renderer/src/components/editor/EditorTextDirectionButton.tsx#L25-L37: replace the new IDs for “Text Direction”, “Left-to-Right”, and “Right-to-Left”.
  • src/renderer/src/components/editor/EditorPanelMarkdownActionsMenu.tsx#L86-L89: replace the new ID for “Right-to-Left”.

Based on learnings, new user-facing strings must use intent-named localization IDs; hash-based IDs are grandfathered only.

Source: Learnings

src/renderer/src/components/editor/EditorPanelMarkdownActionsMenu.tsx (1)

20-23: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Remove the redundant prop comments.

These comments restate the prop names and types. Keep only comments that explain behavior not evident from the declaration.

Source: Coding guidelines

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro Plus

Run ID: a6e5b297-3f01-4ddd-bb31-47a1377c934b

📥 Commits

Reviewing files that changed from the base of the PR and between 7e76bb3 and 0309556.

📒 Files selected for processing (58)
  • src/renderer/src/assets/main.css
  • src/renderer/src/components/Terminal.tsx
  • src/renderer/src/components/editor/EditorPanelHeader.test.tsx
  • src/renderer/src/components/editor/EditorPanelHeader.tsx
  • src/renderer/src/components/editor/EditorPanelMarkdownActionsMenu.test.tsx
  • src/renderer/src/components/editor/EditorPanelMarkdownActionsMenu.tsx
  • src/renderer/src/components/editor/EditorPanelShell.tsx
  • src/renderer/src/components/editor/EditorTextDirectionButton.tsx
  • src/renderer/src/components/editor/MonacoEditor.content-owner.test.tsx
  • src/renderer/src/components/editor/MonacoEditor.font-family.test.tsx
  • src/renderer/src/components/editor/MonacoEditor.tsx
  • src/renderer/src/components/editor/RichMarkdownEditor.tsx
  • src/renderer/src/components/editor/RichMarkdownEditorSurface.tsx
  • src/renderer/src/components/editor/editor-panel-render-model.test.ts
  • src/renderer/src/components/editor/editor-panel-render-model.ts
  • src/renderer/src/components/editor/editor-text-direction-class.test.ts
  • src/renderer/src/components/editor/editor-text-direction-class.ts
  • src/renderer/src/components/settings/EditorTextDirectionSetting.test.tsx
  • src/renderer/src/components/settings/EditorTextDirectionSetting.tsx
  • src/renderer/src/components/settings/GeneralEditorSettingsSection.tsx
  • src/renderer/src/i18n/locales/en.json
  • src/renderer/src/lib/pane-manager/terminal-arabic-shaping-joiner.test.ts
  • src/renderer/src/lib/pane-manager/terminal-arabic-shaping-joiner.ts
  • src/renderer/src/lib/workspace-session-browser-history.test.ts
  • src/renderer/src/lib/workspace-session-editor-data.ts
  • src/renderer/src/lib/workspace-session-editor-drafts.test.ts
  • src/renderer/src/lib/workspace-session-host-field-ownership.ts
  • src/renderer/src/lib/workspace-session-liveness.test.ts
  • src/renderer/src/lib/workspace-session-patch.test.ts
  • src/renderer/src/lib/workspace-session-patch.ts
  • src/renderer/src/lib/workspace-session-relevant-fields.test.ts
  • src/renderer/src/lib/workspace-session.ts
  • src/renderer/src/store/slices/editor-text-direction-override.test.ts
  • src/renderer/src/store/slices/editor/actions/close-file-action.ts
  • src/renderer/src/store/slices/editor/actions/editor-draft-state.ts
  • src/renderer/src/store/slices/editor/actions/hydrate-editor-session.ts
  • src/renderer/src/store/slices/editor/actions/hydrate-file-keyed-overrides.ts
  • src/renderer/src/store/slices/editor/actions/open-file-apply.ts
  • src/renderer/src/store/slices/editor/actions/recently-closed-editor-tabs.ts
  • src/renderer/src/store/slices/editor/actions/rekey-open-files-action.ts
  • src/renderer/src/store/slices/editor/actions/restored-editor-owner-transition.ts
  • src/renderer/src/store/slices/editor/tabs/workspace-editor-item.ts
  • src/renderer/src/store/slices/restored-editor-owner-reparent.test.ts
  • src/renderer/src/store/slices/store-session-editor-hydration.test.ts
  • src/renderer/src/store/slices/worktrees/teardown/remove-worktree-store-cleanup.ts
  • src/renderer/src/store/slices/worktrees/teardown/worktree-purge-state.ts
  • src/shared/constants.test.ts
  • src/shared/constants.ts
  • src/shared/editor-text-direction.test.ts
  • src/shared/editor-text-direction.ts
  • src/shared/global-settings-types.ts
  • src/shared/keybindings.ts
  • src/shared/strong-rtl-text.test.ts
  • src/shared/strong-rtl-text.ts
  • src/shared/workspace-session-editor-schema.ts
  • src/shared/workspace-session-schema.ts
  • src/shared/workspace-session-state-types.ts
  • tests/e2e/editor-rtl-text-direction.spec.ts
💤 Files with no reviewable changes (1)
  • src/renderer/src/lib/pane-manager/terminal-arabic-shaping-joiner.test.ts

Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review.

Comment thread src/renderer/src/components/editor/EditorPanelHeader.test.tsx Resolved
Comment thread src/renderer/src/components/settings/EditorTextDirectionSetting.test.tsx Resolved
Comment thread src/renderer/src/store/slices/worktrees/teardown/remove-worktree-store-cleanup.ts Resolved
Comment thread tests/e2e/editor-rtl-text-direction.spec.ts Resolved
Comment thread tests/e2e/editor-rtl-text-direction.spec.ts Resolved
@Yehuda-Ness
fix(editor): delete direction overrides when a worktree is removed
012a908 
The cleanup loop cloned editorTextDirectionByFile but never deleted from
it, so removing a worktree left stale overrides behind. fileIds derive from
the worktree path, so a recreated worktree reuses them and an unrelated
file could reopen in RTL. Caught in review of stablyai#16291.

Also makes two tests actually exercise behavior instead of asserting that
render output exists:

- EditorPanelHeader now mocks the direction button to capture onToggle, so
  the pin-then-clear sequence is verified for both an 'ltr' and an 'auto'
  global default. The previous version could not dispatch a click and so
  asserted nothing about the callback.
- EditorTextDirectionSetting asserts the persisted 'rtl' selection, which
  its title already claimed to cover.

The new worktree-teardown test was confirmed to fail without the fix.
@alefbt

alefbt commented Aug 24, 2026

Copy link
Copy Markdown
Author

Thanks — all three points were valid and are fixed in 012a908.

1. Direction overrides leaking on worktree removal — real bug, good catch. remove-worktree-store-cleanup.ts cloned editorTextDirectionByFile but I never added the delete to the loop that clears the sibling records. Since fileIds derive from the worktree path, a recreated worktree reuses them and an unrelated file could reopen in RTL.

Fixed, and I added a regression test to worktrees-removal-state-cleanup.test.ts alongside the existing markdownFrontmatterVisible one. I confirmed the test actually catches it by reverting the one-line fix and watching it fail:

AssertionError: expected { 'file-1': 'rtl', 'file-2': 'rtl' } to deeply equal { 'file-2': 'rtl' }

This is exactly the failure mode the "mirror markdownFrontmatterVisible at every site" approach was meant to prevent, and I got one of the ~11 sites wrong.

2. The header test never invoked the toggle. Correct — renderToStaticMarkup can't dispatch a click, so that test asserted nothing about the callback. It now mocks EditorTextDirectionButton to capture onToggle and verifies the full pin-then-clear sequence for both an ltr and an auto global default. The auto case matters: pinning ltr instead of clearing would strand the file and it could never return to auto.

3. The settings test only covered Auto despite its title. Split into separate auto and rtl assertions.

pnpm lint and pnpm typecheck pass.

On the docstring-coverage warning: I've left it. This diff is mostly small pure functions with // Why: comments explaining non-obvious decisions, which matches the surrounding code and AGENTS.md's explicit "concise, non-obvious comments only — WHY not HOW" rule. Adding docstrings to satisfy the threshold would cut against the house style. Happy to reconsider if a maintainer disagrees.

coderabbitai[bot]
coderabbitai Bot reviewed Aug 24, 2026
View reviewed changes

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro Plus

Run ID: fd8a0ab6-4d3a-42f7-9236-5cf0c684235c

📥 Commits

Reviewing files that changed from the base of the PR and between 0309556 and 012a908.

📒 Files selected for processing (4)
  • src/renderer/src/components/editor/EditorPanelHeader.test.tsx
  • src/renderer/src/components/settings/EditorTextDirectionSetting.test.tsx
  • src/renderer/src/store/slices/worktrees-removal-state-cleanup.test.ts
  • src/renderer/src/store/slices/worktrees/teardown/remove-worktree-store-cleanup.ts

Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review.

Comment thread src/renderer/src/store/slices/worktrees-removal-state-cleanup.test.ts Resolved
@alefbt

alefbt commented Aug 25, 2026

Copy link
Copy Markdown
Author

Heads-up for whoever picks this up: CI has not actually run on this PR. PR Checks and PR test LoC are both sitting at action_required, which is GitHub's default gate for a first-time contributor pushing from a fork — they need a maintainer to hit Approve and run workflows. The only workflow that did run is Track Community PRs (pull_request_target), which passed.

So the UNSTABLE merge state is not a failing test — it's ungated CI plus CodeRabbit's advisory docstring warning.

Everything I claim below was verified locally on macOS, not by CI:

  • pnpm lint (including the three localization gates), pnpm typecheck, pnpm build — all pass.
  • pnpm test — the direction-related suites pass. Full-suite runs show pre-existing failures unrelated to this branch: src/main/ssh/ssh-connection-host-key-store-wiring.test.ts fails identically on unmodified main (verified in a clean worktree), and a couple of src/main/ tests are load-flaky and pass in isolation. This branch touches no file under src/main/ (git diff main --stat -- src/main/ is empty).
  • The e2e spec drives the real Electron app and passes.

One local-environment gotcha worth recording: running pnpm test from inside an Orca terminal pane fails src/main/daemon/pty-subprocess-env-inheritance.test.ts, because the pane injects the ORCA_* shell-feature vars that test asserts are absent. It passes in a scrubbed env.

@coderabbitai

coderabbitai Bot commented Aug 26, 2026

Copy link
Copy Markdown
Contributor

Note

GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer.

coderabbitai[bot]
coderabbitai Bot reviewed Aug 26, 2026
View reviewed changes

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 97c70f52-47ab-417c-8090-ea2d755e453a

📥 Commits

Reviewing files that changed from the base of the PR and between 1c9fb84 and 5576be1.

📒 Files selected for processing (59)
  • src/renderer/src/assets/main.css
  • src/renderer/src/components/Terminal.tsx
  • src/renderer/src/components/editor/EditorPanelHeader.test.tsx
  • src/renderer/src/components/editor/EditorPanelHeader.tsx
  • src/renderer/src/components/editor/EditorPanelMarkdownActionsMenu.test.tsx
  • src/renderer/src/components/editor/EditorPanelMarkdownActionsMenu.tsx
  • src/renderer/src/components/editor/EditorPanelShell.tsx
  • src/renderer/src/components/editor/EditorTextDirectionButton.tsx
  • src/renderer/src/components/editor/MonacoEditor.content-owner.test.tsx
  • src/renderer/src/components/editor/MonacoEditor.font-family.test.tsx
  • src/renderer/src/components/editor/MonacoEditor.tsx
  • src/renderer/src/components/editor/RichMarkdownEditor.tsx
  • src/renderer/src/components/editor/RichMarkdownEditorSurface.tsx
  • src/renderer/src/components/editor/editor-panel-render-model.test.ts
  • src/renderer/src/components/editor/editor-panel-render-model.ts
  • src/renderer/src/components/editor/editor-text-direction-class.test.ts
  • src/renderer/src/components/editor/editor-text-direction-class.ts
  • src/renderer/src/components/settings/EditorTextDirectionSetting.test.tsx
  • src/renderer/src/components/settings/EditorTextDirectionSetting.tsx
  • src/renderer/src/components/settings/GeneralEditorSettingsSection.tsx
  • src/renderer/src/i18n/locales/en.json
  • src/renderer/src/lib/pane-manager/terminal-arabic-shaping-joiner.test.ts
  • src/renderer/src/lib/pane-manager/terminal-arabic-shaping-joiner.ts
  • src/renderer/src/lib/workspace-session-browser-history.test.ts
  • src/renderer/src/lib/workspace-session-editor-data.ts
  • src/renderer/src/lib/workspace-session-editor-drafts.test.ts
  • src/renderer/src/lib/workspace-session-host-field-ownership.ts
  • src/renderer/src/lib/workspace-session-liveness.test.ts
  • src/renderer/src/lib/workspace-session-patch.test.ts
  • src/renderer/src/lib/workspace-session-patch.ts
  • src/renderer/src/lib/workspace-session-relevant-fields.test.ts
  • src/renderer/src/lib/workspace-session.ts
  • src/renderer/src/store/slices/editor-text-direction-override.test.ts
  • src/renderer/src/store/slices/editor/actions/close-file-action.ts
  • src/renderer/src/store/slices/editor/actions/editor-draft-state.ts
  • src/renderer/src/store/slices/editor/actions/hydrate-editor-session.ts
  • src/renderer/src/store/slices/editor/actions/hydrate-file-keyed-overrides.ts
  • src/renderer/src/store/slices/editor/actions/open-file-apply.ts
  • src/renderer/src/store/slices/editor/actions/recently-closed-editor-tabs.ts
  • src/renderer/src/store/slices/editor/actions/rekey-open-files-action.ts
  • src/renderer/src/store/slices/editor/actions/restored-editor-owner-transition.ts
  • src/renderer/src/store/slices/editor/tabs/workspace-editor-item.ts
  • src/renderer/src/store/slices/restored-editor-owner-reparent.test.ts
  • src/renderer/src/store/slices/store-session-editor-hydration.test.ts
  • src/renderer/src/store/slices/worktrees-removal-state-cleanup.test.ts
  • src/renderer/src/store/slices/worktrees/teardown/remove-worktree-store-cleanup.ts
  • src/renderer/src/store/slices/worktrees/teardown/worktree-purge-state.ts
  • src/shared/constants.test.ts
  • src/shared/constants.ts
  • src/shared/editor-text-direction.test.ts
  • src/shared/editor-text-direction.ts
  • src/shared/global-settings-types.ts
  • src/shared/keybindings.ts
  • src/shared/strong-rtl-text.test.ts
  • src/shared/strong-rtl-text.ts
  • src/shared/workspace-session-editor-schema.ts
  • src/shared/workspace-session-schema.ts
  • src/shared/workspace-session-state-types.ts
  • tests/e2e/editor-rtl-text-direction.spec.ts
💤 Files with no reviewable changes (1)
  • src/renderer/src/lib/pane-manager/terminal-arabic-shaping-joiner.test.ts
🚧 Files skipped from review as they are similar to previous changes (57)
  • src/renderer/src/lib/workspace-session-browser-history.test.ts
  • src/renderer/src/assets/main.css
  • src/renderer/src/store/slices/editor/actions/recently-closed-editor-tabs.ts
  • src/renderer/src/components/editor/editor-text-direction-class.ts
  • src/renderer/src/lib/workspace-session-host-field-ownership.ts
  • src/renderer/src/components/editor/editor-panel-render-model.test.ts
  • src/shared/constants.test.ts
  • src/renderer/src/store/slices/worktrees/teardown/worktree-purge-state.ts
  • src/renderer/src/store/slices/restored-editor-owner-reparent.test.ts
  • src/renderer/src/store/slices/editor/actions/open-file-apply.ts
  • src/renderer/src/lib/workspace-session-relevant-fields.test.ts
  • src/renderer/src/store/slices/worktrees/teardown/remove-worktree-store-cleanup.ts
  • src/renderer/src/store/slices/editor/actions/hydrate-file-keyed-overrides.ts
  • src/renderer/src/lib/workspace-session-liveness.test.ts
  • src/renderer/src/store/slices/editor/actions/restored-editor-owner-transition.ts
  • src/renderer/src/store/slices/editor/actions/close-file-action.ts
  • src/renderer/src/components/settings/GeneralEditorSettingsSection.tsx
  • src/renderer/src/components/editor/EditorPanelShell.tsx
  • src/renderer/src/lib/workspace-session-patch.test.ts
  • src/renderer/src/components/editor/MonacoEditor.tsx
  • src/renderer/src/components/editor/MonacoEditor.content-owner.test.tsx
  • src/shared/strong-rtl-text.test.ts
  • src/renderer/src/store/slices/editor/actions/rekey-open-files-action.ts
  • src/renderer/src/store/slices/editor-text-direction-override.test.ts
  • src/renderer/src/i18n/locales/en.json
  • src/shared/editor-text-direction.test.ts
  • src/shared/keybindings.ts
  • src/renderer/src/store/slices/worktrees-removal-state-cleanup.test.ts
  • src/renderer/src/lib/workspace-session-editor-drafts.test.ts
  • src/renderer/src/components/Terminal.tsx
  • src/renderer/src/components/editor/editor-text-direction-class.test.ts
  • src/renderer/src/components/editor/RichMarkdownEditor.tsx
  • src/renderer/src/components/editor/EditorPanelMarkdownActionsMenu.test.tsx
  • src/shared/strong-rtl-text.ts
  • src/renderer/src/lib/pane-manager/terminal-arabic-shaping-joiner.ts
  • src/renderer/src/components/editor/editor-panel-render-model.ts
  • src/shared/constants.ts
  • src/renderer/src/store/slices/editor/actions/editor-draft-state.ts
  • src/renderer/src/components/editor/EditorPanelMarkdownActionsMenu.tsx
  • src/shared/editor-text-direction.ts
  • src/renderer/src/components/settings/EditorTextDirectionSetting.tsx
  • src/renderer/src/components/settings/EditorTextDirectionSetting.test.tsx
  • src/renderer/src/components/editor/EditorPanelHeader.test.tsx
  • src/shared/global-settings-types.ts
  • tests/e2e/editor-rtl-text-direction.spec.ts
  • src/renderer/src/store/slices/editor/tabs/workspace-editor-item.ts
  • src/renderer/src/store/slices/editor/actions/hydrate-editor-session.ts
  • src/renderer/src/store/slices/store-session-editor-hydration.test.ts
  • src/shared/workspace-session-state-types.ts
  • src/renderer/src/components/editor/EditorTextDirectionButton.tsx
  • src/renderer/src/lib/workspace-session-editor-data.ts
  • src/renderer/src/components/editor/EditorPanelHeader.tsx
  • src/renderer/src/lib/workspace-session-patch.ts
  • src/renderer/src/components/editor/RichMarkdownEditorSurface.tsx
  • src/shared/workspace-session-editor-schema.ts
  • src/shared/workspace-session-schema.ts
  • src/renderer/src/components/editor/MonacoEditor.font-family.test.tsx

Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review.

Comment thread src/renderer/src/lib/workspace-session.ts Resolved
@Yehuda-Ness
fix(merge): repair the main merge in the session-persistence layer
afd9453 
The GitHub UI merge of main auto-resolved several add/add and
move-vs-edit collisions incorrectly, leaving the branch uncompilable.
Upstream had independently made two of the same refactors this branch
made, which is what confused the merge:

- workspace-session-editor-schema.ts existed on both sides. The merge
  blended the two header comments into a syntactically invalid block and
  double-imported persistedOpenFileSchema. Now takes upstream's file plus
  this branch's direction additions.
- workspace-session.ts kept both the new imports and the old local bodies
  for buildEditorSessionData and buildBrowserSessionData, so each name was
  declared twice. Rebuilt from upstream, with the direction delta applied
  inline.

Upstream's own extraction of the browser builders freed enough room that
workspace-session-editor-data.ts is no longer needed to stay under
max-lines, so it is deleted and callers point back at workspace-session.
The two editor-slice field schemas move into
workspace-session-editor-schema.ts, which keeps workspace-session-schema.ts
inside its budget without a ratchet bump.

Also classifies editorTextDirectionByFile in
WORKSPACE_SESSION_FIELD_DISPOSITION, a new upstream exhaustiveness gate
that a new session field has to satisfy.

pnpm tc and pnpm lint pass.
@Yehuda-Ness
test(editor): strengthen the RTL e2e assertions
3f134a2 
Both weaknesses were raised in review and both were real.

Alignment alone could not distinguish 'direction: rtl' from a bare
'text-align: right', and the auto-mode check would have passed even if
'auto' did nothing, since the line stays left-aligned either way. Each mode
now also asserts the computed style that has to be in effect:
direction: rtl for rtl, unicode-bidi: plaintext for auto.

The click assertion only proved the line accepted a pointer event, because
the visibility check next to it was already true before the click. It now
seeds the cursor line, clicks a different line, and polls Orca's mirrored
editorCursorLine, so a broken caret path in RTL fails.

Run-order between the RTL and Latin runs is deliberately not asserted:
measuring it showed the order is identical in all three modes, since
Chromium already resolves this line the same way. Claiming otherwise is
what produced the inaccurate description I corrected earlier.
@Yehuda-Ness
Merge branch 'main' into feat/editor-rtl-text-direction
5f11f37 
Verified locally before pushing, because the previous main merge on this
branch auto-resolved cleanly and still left the tree uncompilable: pnpm tc,
oxlint, the 137 direction-related unit tests, and the RTL e2e all pass on
the merged tree.

Only three of this branch's files were also touched upstream
(en.json, constants.ts, global-settings-types.ts) and all three are
additive, so nothing needed hand-resolution this time.
@alefbt
alefbt force-pushed the feat/editor-rtl-text-direction branch from 7267300 to 5f11f37 Compare August 27, 2026 13:48
@Yehuda-Ness
Merge branch 'main' into feat/editor-rtl-text-direction
2b89461 
1149 commits of upstream drift, 11 conflicts. Resolved by taking upstream
wholesale wherever it had restructured a file and re-applying this branch's
delta into the new shape, rather than hand-merging hunks:

- keybindings.ts was split into keybindings/. The action id moves to
  keybindings/types.ts and the definition to definitions-core-4.ts (the
  overflow file; core-3 was already at its line budget).
- Terminal.tsx's editor shortcut handling moved to
  terminal-workspace-editor-shortcuts.ts, so the direction handler moves
  with it.
- The settings defaults moved from constants.ts to
  default-global-settings.ts.
- workspace-session-host-field-ownership.ts moved to src/shared (stablyai#18161).
- remove-worktree-store-cleanup.ts replaced the manual clone-and-delete
  loop with omitByFileId, so the field is registered there instead.
- editor-panel-render-model.ts was rewritten and kept the original
  inlineMarkdownContent name this branch had renamed; canShowTextDirectionToggle
  is re-derived from it.

Kept both sides where upstream and this branch each added an adjacent item:
the Show Whitespace menu entry and setting, and their en.json keys.

Three new upstream exhaustiveness gates now classify the field:
WORKSPACE_SESSION_FIELD_DISPOSITION, session-worktree-ownership, and
workspace-session-schema-field-coverage.

buildEditorSessionData is extracted again to keep workspace-session.ts
under max-lines.

Verified on the merged tree: pnpm tc, pnpm lint (exit 0, all localization
gates), 214 direction-related unit tests, and the RTL e2e all pass.
@alefbt

alefbt commented Sep 7, 2026

Copy link
Copy Markdown
Author

Rebased onto current main and ready for a look.

Requesting a maintainer action: CI has never run on this PR. PR Checks and PR test LoC have sat at action_required since it was opened — the default gate for a fork contributor — so every claim below is local-only verification. If someone can hit Approve and run workflows, that would give this real coverage for the first time.

On the update just pushed. The branch had drifted 1,149 commits behind with 11 conflicts. Wherever upstream had restructured a file I took upstream wholesale and re-applied this branch's delta into the new shape, rather than hand-merging hunks:

Upstream change What moved
keybindings.ts split into keybindings/ action id → keybindings/types.ts; definition → definitions-core-4.ts (core-3 was at its line budget)
Editor shortcuts extracted from Terminal.tsx handler → terminal-workspace-editor-shortcuts.ts
Settings defaults left constants.ts default → default-global-settings.ts
workspace-session-host-field-ownership.ts moved to src/shared (#18161) registration follows it
remove-worktree-store-cleanup.ts adopted omitByFileId field registered in the helper list
editor-panel-render-model.ts rewritten, kept the inlineMarkdownContent name this branch had renamed canShowTextDirectionToggle re-derived from it

Where upstream and this branch each added an adjacent item — the Show Whitespace menu entry and setting — both are kept, along with both sets of en.json keys.

Three exhaustiveness gates that didn't exist when this was opened now classify the new session field: WORKSPACE_SESSION_FIELD_DISPOSITION, session-worktree-ownership, and workspace-session-schema-field-coverage. Worth noting those gates did their job — each one failed the build until the field was classified, which is exactly what you'd want from a new persisted field.

Verified on the merged tree: pnpm tc, pnpm lint (exit 0, including all three localization gates), 214 direction-related unit tests, and the RTL e2e against real Electron.

Two open questions for a maintainer, both judgment calls I deliberately didn't make unilaterally:

  1. Docstring coverage — CodeRabbit's advisory check wants 80%. I left it, because this diff is small pure functions with // Why: comments and AGENTS.md is explicit about "concise, non-obvious comments only — WHY not HOW". Happy to add them if you'd rather satisfy the threshold.
  2. Surface count — the feature ships as a header button, a … menu item, an unbound shortcut, and a Settings row. If that's too much, the menu item is the one I'd cut first.

One scope note I'd rather state than have discovered in review: the demonstrable effect here is base direction and alignment, not new bidi shaping. Monaco already renders bidi runs through the browser. I originally claimed this also fixed intra-line run order and trailing punctuation; measuring it in the real app disproved that, so I corrected the issue description and the tests assert only what's real — computed direction/unicode-bidi plus line alignment.

@Yehuda-Ness
Merge branch 'main' into feat/editor-rtl-text-direction
d6c5e6f 
Upstream replaced the generic file-keyed hydration helper this branch added
with resolveHydratedEditorFrontmatter, which hardcodes the visible-by-default
filter and scans the smaller of the override/migration maps.

Frontmatter now goes through their tuned path. The direction override keeps
hydrateFileKeyedOverrides, since it needs a different value predicate and
reshaping their optimization mid-merge is the wrong place for that. Both
read the same usedOpenFileIds set upstream introduced.

pnpm tc, oxlint, and the hydration/direction tests pass.
@Yehuda-Ness
Merge branch 'main' into feat/editor-rtl-text-direction
d4db9bc 
One conflict: upstream added Reveal in Finder / Open Containing Folder keys
to the same en.json object this branch's direction keys live in. Both sets
kept.

pnpm tc, oxlint, verify:localization-catalog, and the direction tests pass.

This branch has not been deployed

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

Reviewers

@coderabbitai coderabbitai[bot] coderabbitai[bot] left review comments
@brennanb2025 brennanb2025 Awaiting requested review from brennanb2025 brennanb2025 is a code owner

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[Feature]: RTL text-direction toggle for the built-in editor (button + … menu + shortcut, like Word Wrap)

2 participants

@alefbt @Yehuda-Ness