Skip to content

Commit b0e86bf

Browse files
committed
Addressed Copilot review: removed artifacts, fixed import + perf
1 parent 55c0988 commit b0e86bf

11 files changed

Lines changed: 846 additions & 281 deletions

File tree

‎.github/copilot-instructions.md‎

Lines changed: 181 additions & 119 deletions
Large diffs are not rendered by default.

‎.vscode/mcp.json‎

Lines changed: 36 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,42 @@
2323
"--use-daemon",
2424
"-vv"
2525
]
26+
},
27+
"playwright-mcp": {
28+
"type": "stdio",
29+
"command": "npx",
30+
"args": [
31+
"-y",
32+
"@playwright/mcp"
33+
]
34+
},
35+
"filesystem-mcp": {
36+
"type": "stdio",
37+
"command": "npx",
38+
"args": [
39+
"-y",
40+
"@modelcontextprotocol/server-filesystem",
41+
"${workspaceFolder}"
42+
]
43+
},
44+
"github-mcp": {
45+
"type": "stdio",
46+
"command": "npx",
47+
"args": [
48+
"-y",
49+
"@modelcontextprotocol/server-github"
50+
],
51+
"env": {
52+
"GITHUB_PERSONAL_ACCESS_TOKEN": "${input:github_pat}"
53+
}
2654
}
2755
},
28-
"inputs": []
56+
"inputs": [
57+
{
58+
"type": "promptString",
59+
"id": "github_pat",
60+
"description": "GitHub Personal Access Token for @modelcontextprotocol/server-github",
61+
"password": true
62+
}
63+
]
2964
}

‎docs/rtl-246116-progress-report.md‎

Lines changed: 318 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,318 @@
1+
# VS Code Issue #246116 Progress Report (Arabic RTL Rendering)
2+
3+
## 1) Executive Summary
4+
5+
This report documents all work completed so far on **microsoft/vscode#246116** (Arabic RTL text rendering in editor), plus exact setup/install steps to unblock the remaining environment issues.
6+
7+
Status:
8+
9+
- ✅ Implemented a scoped, opt-in fix (`editor.rtlAutoDetect`) in editor rendering direction logic.
10+
- ✅ Added targeted automated tests for Arabic/mixed RTL-LTR behavior and regression safety.
11+
- ✅ TypeScript compile-check passed.
12+
- ✅ Targeted browser tests for the new behavior passed in Chromium and Firefox.
13+
- ✅ Full Chromium browser unit suite ran successfully.
14+
- ⚠️ WebKit remains environment-blocked on this Arch/CachyOS host due Playwright WebKit runtime ABI/display issues (details in section 9).
15+
16+
---
17+
18+
## 2) Repository/Workflow Changes Completed
19+
20+
### A. Copilot and MCP setup work
21+
22+
1. Updated `.github/copilot-instructions.md` with:
23+
- Build/lint/test commands
24+
- Architecture summary
25+
- Key repo conventions
26+
- A dedicated phased workflow for issue #246116
27+
28+
2. Updated `.vscode/mcp.json` and configured:
29+
- `playwright-mcp`
30+
- `filesystem-mcp`
31+
- `github-mcp` (token via secure prompt input)
32+
33+
### B. Issue #246116 code implementation
34+
35+
Implemented an opt-in editor setting and line-direction auto-detection path:
36+
37+
#### 1) `src/vs/editor/common/config/editorOptions.ts`
38+
39+
- Added new public editor option in `IEditorOptions`:
40+
- `rtlAutoDetect?: boolean;`
41+
- Added new enum entry:
42+
- `EditorOption.rtlAutoDetect`
43+
- Registered option in `EditorOptions`:
44+
- key: `rtlAutoDetect`
45+
- default: `false`
46+
- localized description
47+
48+
#### 2) `src/vs/editor/common/viewModel/viewModelImpl.ts`
49+
50+
Changed text-direction computation flow:
51+
52+
- Function signature changed from:
53+
- `_getTextDirection(lineNumber, decorations)`
54+
- to `_getTextDirection(lineNumber, lineContent, decorations)`
55+
56+
- New behavior:
57+
1. If decorations force RTL/LTR, decoration wins.
58+
2. If decoration vote is neutral:
59+
- If `editor.rtlAutoDetect === true` and `strings.containsRTL(lineContent)`, return RTL.
60+
3. Else return LTR.
61+
62+
- Wired callers:
63+
- `getTextDirection(...)`
64+
- view line rendering data creation (`_getViewLineRenderingData(...)`)
65+
66+
#### 3) `src/vs/editor/test/browser/viewModel/viewModelImpl.test.ts`
67+
68+
Added tests:
69+
70+
1. `rtlAutoDetect renders Arabic text as RTL when enabled`
71+
2. `rtlAutoDetect keeps Arabic text as LTR when disabled`
72+
3. `rtlAutoDetect still allows forcing LTR direction via decorations`
73+
4. `rtlAutoDetect renders ASCII-only lines as LTR`
74+
75+
---
76+
77+
## 3) Validation and Results
78+
79+
## 3.1 Compile / type-check
80+
81+
- Command:
82+
- `npm run compile-check-ts-native`
83+
- Result:
84+
- ✅ Passed
85+
86+
## 3.2 Targeted test run for the new RTL behavior
87+
88+
- Command:
89+
- `node test/unit/browser/index.js --run src/vs/editor/test/browser/viewModel/viewModelImpl.test.ts --grep rtlAutoDetect`
90+
- Result:
91+
- ✅ `4 passing`
92+
93+
## 3.3 Full browser suite (Chromium)
94+
95+
- Command:
96+
- `node test/unit/browser/index.js --browser chromium`
97+
- Result:
98+
- ✅ `17602 passing`, `117 pending`
99+
- (No failure caused by this RTL change)
100+
101+
## 3.4 Full browser suite (Chromium + Firefox)
102+
103+
- Command:
104+
- `node test/unit/browser/index.js --browser chromium --browser firefox`
105+
- Result:
106+
- ⚠️ Ended with `3 failing`, all in `DefaultSettings` (preferences grouping tests), unrelated to RTL changes.
107+
- New RTL tests pass in Firefox.
108+
109+
## 3.5 Full all-browser suite (default: Chromium + Firefox + WebKit)
110+
111+
- Command:
112+
- `node test/unit/browser/index.js`
113+
- Result:
114+
- ❌ WebKit path unstable in this environment.
115+
- Initial failures were missing legacy sonames (`icu66`, `libwebp.so.6`, `libffi.so.7`, etc.), which were installed.
116+
- Remaining WebKit failure is runtime-level (WPE/EGL + ABI interaction), not RTL test logic.
117+
118+
---
119+
120+
## 4) Environment Work Completed
121+
122+
1. Detected host package manager:
123+
- `pacman` (Arch)
124+
125+
2. Installed Node Version Manager (nvm) and switched to Node 22:
126+
- Node 22 is required by this repo (preinstall check rejects Node 25).
127+
128+
3. Reinstalled dependencies under Node 22:
129+
- `npm install`
130+
131+
4. Installed Playwright browsers:
132+
- `npm run playwright-install`
133+
134+
5. Confirmed runtime mismatch issue for WebKit binary:
135+
- WebKit build expects older sonames (`.so.66`, `.so.7`, etc.) not present in current base system.
136+
137+
---
138+
139+
## 5) Current File Delta
140+
141+
Modified files in working tree:
142+
143+
- `.github/copilot-instructions.md`
144+
- `.vscode/mcp.json`
145+
- `src/vs/editor/common/config/editorOptions.ts`
146+
- `src/vs/editor/common/viewModel/viewModelImpl.ts`
147+
- `src/vs/editor/test/browser/viewModel/viewModelImpl.test.ts`
148+
149+
---
150+
151+
## 6) How to Install What Is Still Needed
152+
153+
You asked specifically how to install the missing requirements. Use the following sequence on Arch Linux.
154+
155+
## 6.1 Required baseline toolchain
156+
157+
```bash
158+
sudo pacman -Syu --needed curl git base-devel
159+
```
160+
161+
## 6.2 Install and use Node 22 (required by VS Code repo)
162+
163+
```bash
164+
curl -fsSL https://raw.githubusercontent.com/nvm-sh/nvm/v0.40.3/install.sh | bash
165+
export NVM_DIR="$HOME/.nvm"
166+
source "$NVM_DIR/nvm.sh"
167+
nvm install 22
168+
nvm use 22
169+
nvm alias default 22
170+
node --version
171+
npm --version
172+
```
173+
174+
Expected: Node v22.x
175+
176+
## 6.3 Install repo deps and browsers
177+
178+
```bash
179+
cd /path/to/vscode
180+
npm install
181+
npm run playwright-install
182+
```
183+
184+
## 6.4 Install browser runtime libraries (system-wide)
185+
186+
### Common Playwright/browser deps
187+
188+
```bash
189+
sudo pacman -S --needed \
190+
nss nspr atk at-spi2-atk gtk3 pango cairo \
191+
libxcomposite libxdamage libxrandr libxkbcommon \
192+
alsa-lib mesa dbus-glib glib2 libdrm \
193+
libxml2 libwebp libffi icu
194+
```
195+
196+
### Important note for WebKit on Arch
197+
198+
Playwright’s Linux WebKit binary in this run expects older ABI sonames:
199+
200+
- `libicudata.so.66`, `libicui18n.so.66`, `libicuuc.so.66`
201+
- `libxml2.so.2`
202+
- `libwebp.so.6`
203+
- `libffi.so.7`
204+
205+
Modern Arch ships newer sonames (e.g. ICU 78, libffi 8, libwebp 7, libxml2 16).
206+
So full all-browser execution may still fail unless you provide compatibility packages.
207+
208+
#### Option A (recommended): run without WebKit when validating this fix
209+
210+
```bash
211+
node test/unit/browser/index.js --browser chromium
212+
node test/unit/browser/index.js --browser firefox
213+
```
214+
215+
This validates the editor RTL fix robustly for this issue.
216+
217+
#### Option B: add compatibility libraries for WebKit
218+
219+
Use an AUR helper (if you use one) to install legacy compatibility packages (names can vary over time):
220+
221+
```bash
222+
yay -Ss 'icu66|libffi7|libwebp6|libxml2-legacy'
223+
```
224+
225+
Then install the available matching compatibility packages and rerun:
226+
227+
```bash
228+
node test/unit/browser/index.js
229+
```
230+
231+
If your mirror/repo set does not provide these compat packages, use a containerized Ubuntu-based test environment for full WebKit runs.
232+
233+
---
234+
235+
## 7) Commands to Reproduce My Validation Exactly
236+
237+
```bash
238+
export NVM_DIR="$HOME/.nvm"
239+
source "$NVM_DIR/nvm.sh"
240+
nvm use 22
241+
242+
npm run compile-check-ts-native
243+
npm run transpile-client
244+
245+
# Targeted fix tests
246+
node test/unit/browser/index.js --browser chromium --run src/vs/editor/test/browser/viewModel/viewModelImpl.test.ts --grep rtlAutoDetect
247+
node test/unit/browser/index.js --browser firefox --run src/vs/editor/test/browser/viewModel/viewModelImpl.test.ts --grep rtlAutoDetect
248+
249+
# Full suites
250+
node test/unit/browser/index.js --browser chromium
251+
node test/unit/browser/index.js --browser chromium --browser firefox
252+
# (WebKit requires additional compat libs on Arch)
253+
```
254+
255+
---
256+
257+
## 8) Practical Next Step
258+
259+
Use Chromium+Firefox as the reliable validation path for this issue on this machine, then proceed to PR prep.
260+
261+
---
262+
263+
## 9) Deep Debug Findings for WebKit (Post-Library Install)
264+
265+
After installing missing compatibility libraries, WebKit still fails in this host setup due environment/runtime mismatch.
266+
267+
### 9.1 What was reproduced
268+
269+
1. Without preload, Playwright WebKit launcher fails with GLib symbol errors (`g_once_init_enter_pointer` / `g_once_init_leave_pointer`).
270+
2. With preload:
271+
272+
```bash
273+
LD_PRELOAD='/usr/lib/libglib-2.0.so.0 /usr/lib/libgobject-2.0.so.0 /usr/lib/libgio-2.0.so.0' node test/unit/browser/index.js --browser=webkit --grep rtlAutoDetect
274+
```
275+
276+
WebKit can start further, but WPE headless path fails on EGL display creation and test run can stall.
277+
278+
### 9.2 Root cause
279+
280+
- Playwright Linux WebKit bundle and host system libraries have ABI/runtime mismatches.
281+
- Headless WebKit path uses WPE (`pw_run.sh` selects WPE when `--headless`) and fails in this display stack.
282+
- VS Code browser harness waits on renderer completion; if browser startup fails before normal completion/cleanup, external timeout is needed to avoid indefinite waiting.
283+
284+
### 9.3 Deterministic workaround (no stuck runs)
285+
286+
Use external `timeout` wrappers and scoped runs:
287+
288+
```bash
289+
timeout 300s npm run -s compile-check-ts-native
290+
timeout 480s node test/unit/browser/index.js --browser chromium --sequential --run src/vs/editor/test/browser/viewModel/viewModelImpl.test.ts --grep rtlAutoDetect
291+
timeout 480s node test/unit/browser/index.js --browser firefox --sequential --run src/vs/editor/test/browser/viewModel/viewModelImpl.test.ts --grep rtlAutoDetect
292+
```
293+
294+
Optional bounded WebKit diagnostics (expected flaky here):
295+
296+
```bash
297+
timeout 240s env LD_PRELOAD='/usr/lib/libglib-2.0.so.0 /usr/lib/libgobject-2.0.so.0 /usr/lib/libgio-2.0.so.0' \
298+
node test/unit/browser/index.js --browser=webkit --sequential --run src/vs/editor/test/browser/viewModel/viewModelImpl.test.ts --grep rtlAutoDetect
299+
```
300+
301+
### 9.4 Safe WebKit runner script
302+
303+
Created non-repo helper script with timeout + cleanup to prevent hangs:
304+
305+
- `/home/mulham/.copilot/session-state/0bd31df8-855f-4561-8808-368d3300b5e5/files/run-webkit-tests-safe.sh`
306+
307+
Example:
308+
309+
```bash
310+
TIMEOUT_SECONDS=180 GREP_PATTERN=rtlAutoDetect /home/mulham/.copilot/session-state/0bd31df8-855f-4561-8808-368d3300b5e5/files/run-webkit-tests-safe.sh
311+
```
312+
313+
---
314+
315+
Once you choose to continue with Chromium+Firefox validation path, I can proceed immediately with:
316+
317+
1. Running your preferred “full” suite in your target environment.
318+
2. Preparing a clean commit for #246116 with a PR-ready description.

0 commit comments

Comments
 (0)