Repository navigation
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughAdds a narrow/wide terminal setting for East Asian ambiguous characters. The change introduces Unicode range classification and width resolution, configurable Unicode provider behavior, CJK font fallback ordering, and propagation through terminal initialization, appearance updates, and previews. The setting is persisted with a narrow default and exposed through advanced typography controls, labels, and localized search metadata. Unit tests cover classification, width resolution, provider behavior, idempotent activation, and font ordering. 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/shared/terminal-unicode-provider.test.ts (1)
54-61: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExercise wide-mode
charPropertiesas well.The test only checks
wcwidth; the newly added packed-property path is what buffer writes consume. Assert that an ambiguous code point has width bits2after switching to wide mode.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 0015135e-dae9-4f27-b7f2-fb02d679f6ae
📒 Files selected for processing (15)
src/renderer/src/components/settings/TerminalAdvancedTypographyControls.tsxsrc/renderer/src/components/settings/TerminalSettingsPreview.tsxsrc/renderer/src/components/settings/setting-labels.tssrc/renderer/src/components/settings/terminal-typography-search.tssrc/renderer/src/components/terminal-pane/layout-serialization.test.tssrc/renderer/src/components/terminal-pane/layout-serialization.tssrc/renderer/src/components/terminal-pane/terminal-appearance.tssrc/renderer/src/components/terminal-pane/use-terminal-pane-lifecycle.tssrc/renderer/src/lib/pane-manager/pane-lifecycle.tssrc/shared/constants.tssrc/shared/east-asian-ambiguous-width.test.tssrc/shared/east-asian-ambiguous-width.tssrc/shared/terminal-unicode-provider.test.tssrc/shared/terminal-unicode-provider.tssrc/shared/types.ts
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 0b4ee663-013c-4bc9-a708-edad9d9fb749
📒 Files selected for processing (1)
src/renderer/src/components/settings/TerminalSettingsPreview.tsx
Add terminalEastAsianAmbiguousWidth (narrow|wide, default narrow) so CJK locales can match conhost double-width ambiguous glyphs (① ■ ★ →) without changing the modern CLI-aligned default. Wide mode also inserts CJK UI fonts ahead of Consolas so full-size glyphs fill the two-cell slot. Closes stablyai#9958
buildFontFamily only adjusts the CJK fallback stack; activate Orca's unicode provider (and wide ambiguous mode) before writing the sample buffer so the preview measures like live panes.
f73f7df to
bae2e4e
Compare
|
Hi @innocarpe, thanks for picking this up and for the quick PR. Just checking in on progress — it's been a few days since the last commit, and CodeRabbit's linked-issues check is still flagging that the headless emulator ( Those two paths matter for the SSH and mobile use cases specifically, so I'd like to see all three activation sites covered before merge. Are you still actively working on this, or would it help to have someone else pick up the remaining wiring? Happy to help if you're stuck on anything. |
Sync update (
|
DoDAON / stablyai#9958 asked for all three activation sites. Desktop already set mode + OrcaUnicodeProvider; this pass completes the twins: - Headless: honor eastAsianAmbiguousWidth (runtime store + settings sync) - Mobile: bundle Orca unicode into the WebView engine and activate after Unicode11 (was bypassing Orca entirely); CJK font stack when wide - Provider as plain object so the Chrome 74 engine floor parses cleanly Evidence: headless-emulator-unicode-width + mobile engine/html tests
Place TERMINAL_EAW_JS after the default terminalFontFamily assignment so vm-based font fallback tests still slice a valid isIOSWebView block.
Sync update (
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Greptile SummaryThis PR adds an opt-in
Confidence Score: 5/5Safe to merge. The default is narrow (no visible change for existing users), and the opt-in wide path is well-isolated through the module-level mode variable and correct font-stack injection ordering. The feature is entirely opt-in with a conservative default. The core EAW logic (binary search table, resolver, provider patching) is straightforward and well-tested. The global mutable mode design is intentional and all write sites converge to the same store value. The only findings are style-level concerns — a mobile CJK fallback list that omits one font the desktop includes, a hardcoded wide font string that could drift from its narrow counterpart, and a duplicated type declaration — none of which affect correctness for the current release. Files Needing Attention: mobile/src/terminal/terminal-webview-eaw-injected.ts — the CJK fallback list and hardcoded wide font string should be kept in sync with the desktop equivalents in layout-serialization.ts and terminal-webview-html.ts.
|
| Filename | Overview |
|---|---|
| src/shared/east-asian-ambiguous-width.ts | New file: EAW range table (with intentional PUA exclusion), binary search classifier, and resolveEastAsianAmbiguousCellWidth. Well-structured with correct range ordering. |
| src/shared/terminal-unicode-provider.ts | Converts OrcaUnicodeProvider from a class to a plain-object factory (Chrome 74 / ES2019 compat), adds module-level EAW mode state read at measurement time, and correctly patches both wcwidth and charProperties for EAW widening including ZWJ paths. |
| src/shared/types.ts | Adds TerminalEastAsianAmbiguousWidth type and optional terminalEastAsianAmbiguousWidth field to GlobalSettings; the type is also declared independently in east-asian-ambiguous-width.ts creating a duplication. |
| mobile/src/terminal/terminal-webview-eaw-injected.ts | New file: injects IIFE-scoped EAW mode, CJK fallback list, and activateOrcaUnicodeOnTerm. Mobile CJK_AMBIGUOUS_FALLBACKS omits Meiryo present on the desktop list, and the wide font stack is hardcoded rather than derived from TERMINAL_FONT_FALLBACKS. |
| src/renderer/src/components/terminal-pane/layout-serialization.ts | buildFontFamily gains CJK_AMBIGUOUS_FALLBACK_FONTS splice and BuildFontFamilyOptions; correctly adds lowerParts.push(lower) for within-fallback deduplication that was previously missing. |
| src/renderer/src/components/terminal-pane/terminal-appearance.ts | Moves setTerminalEastAsianAmbiguousWidthMode and buildFontFamily call before the pane loop, eliminating the redundant per-pane global setter noted in previous review. |
| src/renderer/src/lib/pane-manager/pane-lifecycle.ts | openTerminal now syncs EAW mode from the app store before activating the Orca provider, ensuring restore/replay paths bake correct cell widths from the first write. |
| src/main/daemon/headless-emulator.ts | Constructor now applies EAW mode before loading Unicode11 and activating the Orca provider; HeadlessEmulatorOptions extracted to a separate file. |
| src/main/index.ts | Seeds EAW mode at startup from the store and keeps it in sync on settings changes via onSettingsChanged; correctly ensures all HeadlessEmulator instances in the main process share the right mode. |
Reviews (2): Last reviewed commit: "fix(terminal): merge main; exclude Nerd ..." | Re-trigger Greptile
Resolve conflict with upstream wheel-scroll inject (keep both EAW + wheel). Review (Greptile stablyai#10078): - Remove BMP PUA U+E000–U+F8FF from ambiguous ranges so Powerline/OMP icons stay single-cell in wide mode. - Set process-wide EAW mode once before the appearance pane loop. - Extract HeadlessEmulatorOptions to stay under max-lines. Evidence: east-asian-ambiguous-width PUA tests + width suites.
Sync update (
|
This branch has not been deployed
4 participants
Summary
Opt-in setting for East Asian Ambiguous character cell width (iTerm2-style escape hatch), requested in #9958.
On Windows CJK locales, characters like
①■●★→render poorly in Orca because:This PR keeps the modern CLI-aligned default (
narrow) and adds Wide mode that:OrcaUnicodeProviderChanges
terminalEastAsianAmbiguousWidth: 'narrow' | 'wide'(defaultnarrow)resolveEastAsianAmbiguousCellWidthbuildFontFamily(..., { eastAsianAmbiguousWide })CJK fallback insertionFixes #9958
Screenshots
Narrow/Wide)Testing
pnpm exec vitest run --config config/vitest.config.ts src/shared/east-asian-ambiguous-width.test.ts src/shared/terminal-unicode-provider.test.ts src/renderer/src/components/terminal-pane/layout-serialization.test.ts(40/40)pnpm exec vitest run --config config/vitest.config.ts src/renderer/src/lib/pane-manager/pane-lifecycle.test.ts(30/30)pnpm lint/pnpm typecheck/pnpm test/pnpm buildAI Review Report
wcwidthandcharPropertiesstay aligned when wide expands ambiguous cellsSecurity Audit
Notes
ELI5
Some CJK symbols looked too skinny or wrong in the terminal on Windows. There is an optional Wide mode for East Asian Ambiguous characters so shapes like stars and arrows get a full cell width when you want that.