OCPBUGS-86491: Fix macOS Option key in pod terminal#16492
Conversation
openshift-ci-robot
added
jira/valid-reference
|
@logonoff: This pull request references Jira Issue OCPBUGS-86491, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
The bug has been updated to refer to the pull request using the external bug tracker. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: openshift/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (1)
WalkthroughThe PR changes Changesxterm.js Patch Application
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
🚥 Pre-merge checks | ✅ 12✅ Passed checks (12 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: logonoff The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
approved
|
@logonoff: This pull request references Jira Issue OCPBUGS-86491, which is valid. 3 validation(s) were run on this bug
DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
/hold Need to discuss if we want to set a precedent of patching packages |
openshift-ci
Bot
added
the
do-not-merge/hold
![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)
frontend/package.json (1)
174-174: ⚡ Quick winDocument the patch purpose and removal timing.
Consider adding a comment in this file or in a dedicated PATCHES.md document explaining:
- Why the patch is needed (webpack process polyfill breaks macOS Option key)
- What upstream fix it backports
- When it can be removed (after upgrading to xterm.js version that includes the fix)
📝 Suggested inline documentation
"`@xterm/addon-fit`": "0.10.0", + // Patched to fix macOS Option key handling when webpack's process polyfill is present. + // TODO: Remove patch when upgrading to xterm.js version that includes the upstream fix + // for isNode detection (adds navigator.userAgent check). See OCPBUGS-86491. "`@xterm/xterm`": "patch:`@xterm/xterm`@npm%3A5.5.0#~/.yarn/patches/@xterm-xterm-npm-5.5.0-106735eff7.patch",🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/package.json` at line 174, Add documentation explaining the purpose and removal timing of the `@xterm/xterm` patch referenced as "patch:`@xterm/xterm`@npm%3A5.5.0#~/.yarn/patches/@xterm-xterm-npm-5.5.0-106735eff7.patch": state that the patch fixes the webpack process polyfill breaking macOS Option key, cite the upstream PR/commit hash or link that this patch backports, and specify the removal condition (e.g., remove this patch after upgrading to xterm.js version that contains the upstream fix—include the minimum version number). Put this comment either inline near the dependency entry in package.json or in a dedicated PATCHES.md that references the exact patch filename and the target xterm.js version to drop the patch.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@frontend/package.json`:
- Line 174: The package.json references a missing Yarn patch entry for
"`@xterm/xterm`" (~/.yarn/patches/@xterm-xterm-npm-5.5.0-106735eff7.patch); add
the corresponding patch file into the repo under .yarn/patches using the
original patch contents or remove the patch entry and update the dependency to a
version that contains the upstream fix. Ensure the patch (or the replacement
xterm version) implements the upstream isNode detection using
process.versions.node-style detection (not only navigator.userAgent) so behavior
matches upstream fixes for Node v21+.
---
Nitpick comments:
In `@frontend/package.json`:
- Line 174: Add documentation explaining the purpose and removal timing of the
`@xterm/xterm` patch referenced as
"patch:`@xterm/xterm`@npm%3A5.5.0#~/.yarn/patches/@xterm-xterm-npm-5.5.0-106735eff7.patch":
state that the patch fixes the webpack process polyfill breaking macOS Option
key, cite the upstream PR/commit hash or link that this patch backports, and
specify the removal condition (e.g., remove this patch after upgrading to
xterm.js version that contains the upstream fix—include the minimum version
number). Put this comment either inline near the dependency entry in
package.json or in a dedicated PATCHES.md that references the exact patch
filename and the target xterm.js version to drop the patch.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 5611490e-fdf7-4841-855f-a212eab37e2a
⛔ Files ignored due to path filters (2)
frontend/.yarn/patches/@xterm-xterm-npm-5.5.0-106735eff7.patchis excluded by!**/.yarn/**frontend/yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (1)
frontend/package.json
|
/cherry-pick release-4.22 |
|
@logonoff: once the present PR merges, I will cherry-pick it on top of DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
The NodePolyfillPlugin injects a `process` polyfill with `process.title` set, which causes xterm.js to misidentify the browser as Node.js. This breaks platform detection (`isMac = false`), so the Option key is treated as Meta instead of a compose key on macOS. Backport the upstream fix (xtermjs/xterm.js PR 5571) via yarn patch, which adds a `navigator.userAgent` check to the `isNode` detection.
logonoff
force-pushed
the
OCPBUGS-86491-xterm
branch
from
0c63e33 to
6e7d2c8
Compare
|
Tested this locally — the patch doesn't work as-is. Using German keyboard and type Option + 8 will produce It targets After manually patching |
|
@logonoff: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
Damn... so it was caching when I had patched |
4 participants
Closes #16486
Analysis / Root cause:
The
NodePolyfillPlugininwebpack.config.tsinjects aprocesspolyfill (withadditionalAliases: ['process']) that setsprocess.title = 'browser'. xterm.js 5.5.0 detects Node.js viatypeof process !== 'undefined' && 'title' in process, which returnstruedue to the polyfill. This causes xterm to setisMac = false, so the macOS Option key is treated as Meta (sending ESC sequences) instead of functioning as a compose key.Solution description:
Backport the upstream xterm.js fix via a yarn patch. The patch adds a
navigator.userAgentcheck to theisNodedetection, so that even with aprocesspolyfill present, xterm correctly identifies the browser environment and preservesisMac = true.Test setup:
Test cases:
@,{,},|,\,~) instead of ESC sequencesBrowser conformance:
Additional info:
The upstream fix is merged into xterm.js master but not yet included in a stable release (likely
@xterm/xterm7.0.0). This yarn patch can be removed when upgrading to a future xterm.js version that includes the fix.Summary by CodeRabbit