Repository navigation
Conversation
|
The following comment was made by an LLM, it may be inaccurate: |
![opencode-agent[bot]](https://avatars.githubusercontent.com/in/1549082?s=60&v=4){kind=small}
There was a problem hiding this comment.
Thanks for porting this to V2. I couldn't check the header placement itself: it needs the Windows desktop app with an RTL system language. I ran the new tests (32 pass) and bun run typecheck in packages/app, which passes both with and without your custom-elements.d.ts change. A few things need fixing before this can go in.
Public API: unchanged. This adds an internal language.systemDirection() accessor and a direction.toggle app command bound to mod+shift+d. Nothing in the SDK, server, plugin or config surface is affected.
Concerns, most important first:
- The
custom-elements.d.tscommit breaks both files. They are git symlinks (mode120000) to../../ui/src/custom-elements.d.ts. The commit keeps them as symlinks but sets the link target to the text/// <reference path=... />, so the link now points nowhere on any system that supports symlinks. The TS1128 error you saw comes from a checkout with symlinks turned off (for example Windows withcore.symlinks=false), where git writes the target path as plain text. It isn't a bug ondev: typecheck passes there with the original links. Please drop this commit, and enable symlinks in your checkout instead (git config core.symlinks truewith Developer Mode, then re-checkout). - Overlap with #50661. That open PR fixes the same problem, keeping the titlebar clear of Windows caption buttons when they're mirrored to the left. It positions the header from
env(titlebar-area-x)/env(titlebar-area-width), which is where Chromium actually draws the overlay. This PR guesses the side fromnavigator.languages. In Electron that list reflects Chromium's locale and accept-languages, which can differ from the OS UI language, and whether the controls are mirrored follows Chromium's locale, not the user's language list. If the guess and the real position disagree, the header covers the caption buttons or leaves a gap on the wrong side. Please compare with #50661. The env-based approach doesn't need any language detection. - Scope. Setting
diron the whole<header>mirrors every titlebar control (tabs, menus, buttons) for users whose UI language is LTR, which goes beyond reserving space for the caption buttons. The newdirection.togglecommand and its global shortcut also aren't needed for the header fix. I'd suggest proposing them separately, if at all.
You also mentioned you haven't yet checked this visually on V2. Before/after screenshots on RTL Windows would help once the points above are addressed.
| @@ -1 +1 @@ | |||
| ../../ui/src/custom-elements.d.ts No newline at end of file | |||
| /// <reference path="../../ui/src/custom-elements.d.ts" /> No newline at end of file | |||
There was a problem hiding this comment.
This file is still a symlink (mode 120000), and its target is now the literal string /// <reference path="../../ui/src/custom-elements.d.ts" />, so it's a broken link. The same applies to packages/enterprise/src/custom-elements.d.ts. On dev the link works and bun run typecheck passes. The TS1128 error comes from a checkout with symlinks turned off. Please revert this commit.
| const RTL_LANGUAGE_CODES: ReadonlySet<string> = new Set(["ar", "he", "iw", "fa", "ur", "dv", "yi", "ps"]) | ||
|
|
||
| export function systemPrefersRtl(languages: readonly string[]): boolean { | ||
| return languages.some((tag) => { |
There was a problem hiding this comment.
.some over every preferred language means ["en-US", "he-IL"] counts as RTL (your test asserts exactly this). An English-Windows user who has also added Hebrew or Arabic as a secondary language would get an RTL header with margin-left: auto, while the caption buttons stay on the right. If you keep language detection at all, only the first entry should count. Reading the real overlay position (see #50661) avoids the guess entirely.
| // and on the right in LTR ones. The header reserves space on that side. | ||
| // Its direction follows the OS language only, never the UI language or content. | ||
| "margin-right": windows() && language.systemDirection() === "ltr" ? "auto" : undefined, | ||
| "margin-left": windows() && language.systemDirection() === "rtl" ? "auto" : undefined, |
There was a problem hiding this comment.
Where the Windows caption controls sit follows Chromium's locale, not navigator.languages, and env(titlebar-area-x) already says exactly where they are. #50661 uses that to position the header. Choosing margin-left/margin-right from a language guess can disagree with the real overlay and make the header cover the caption buttons.
| }, | ||
| ]) | ||
|
|
||
| command.register("direction", () => [ |
There was a problem hiding this comment.
This adds a user-facing command and a global mod+shift+d shortcut for what is currently a debug-bar switch. The header fix doesn't need it. I'd drop it from this PR and propose it separately if it's wanted.
c28adc1 to
daa1262
Compare
|
Thanks for the detailed review — all three points addressed in the latest push (single commit now, daa1262):
On the \dir\ scope question: the explicit ask behind this PR is an RTL header on RTL systems even when the UI language is LTR (e.g. English UI on Hebrew Windows). Happy to narrow further if you\u2019d rather the header inherit document direction, but then Hebrew-OS/English-UI goes back to an LTR header over left-side caption buttons. Screenshots on V2 + Hebrew Windows to follow. |
|
Automated PR Cleanup Thank you for contributing to opencode. Due to the high volume of PRs from users and AI agents, we periodically close older PRs using automated criteria so maintainers can focus review time on the most active and community-supported contributions. This PR was closed because it matched the following cleanup criteria:
PRs created within the last month are not affected by this cleanup. If you believe this PR was closed incorrectly, or if you are still actively working on it, please leave a comment explaining why it should be reopened. A maintainer can review and reopen it if appropriate. Thanks again for taking the time to contribute. |
Issue for this PR
Follow-up to #48856 (closed: V1 branch upgraded to V2). This ports that change onto the V2 codebase.
Type of change
What does this PR do?
V2 already ships full Hebrew locale support (including RTL), which covers the common case. What is still missing is tying the desktop titlebar to the OS language specifically:
systemPrefersRtl()inpackages/app/src/runtime/i18n/desktop-native.tsdetects RTL OS languages fromnavigator.languages(the OS language in Electron):he/iw/ar/fa/ur/dv/yi/ps.language.systemDirection(), consumed only by the titlebar. The<header>inpackages/app/src/shell/titlebar/titlebar.tsxgetsdirfrom it, so a Hebrew/Arabic system gets an RTL header even when the UI language is LTR.left: env(titlebar-area-x, 0px)), converging with fix(app): respect RTL Windows caption controls #50661 — no language guessing for positioning, and the oldmargin-right: autoassumption is gone.Out of scope for this PR (will propose separately): a
direction.togglecommand/hotkey for flipping content direction.How did you verify your code works?
bun test src/runtime/i18n/desktop-native.test.ts: 32 pass, 0 fail (extends the existingtest.eachtables withsystemPrefersRtlcases:he-IL,iw-IL,ar-EG, LTR negatives, tag normalization).bunx oxlinton all touched files: 0 errors.bun run typecheckinpackages/app: no errors in any file this PR touches. Repo-wide there are pre-existing latent errors (previously masked behind thecustom-elements.d.tssymlink issue below); none point at lines added here.Screenshots / recordings
Not yet re-verified visually on the V2 base (the V1 verification with the mirrored header on Hebrew Windows is in #48856). Will run the V2 desktop dev build and attach.
Checklist