Conversation
WalkthroughThis pull request replaces the canvas rendering backend with DOM rendering for the terminal display, updates the xterm library from version 5.5.0 to 6.0.0, and removes the Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| } | ||
| } else { | ||
| if (this.canvasAddon != null) { | ||
| if (this.webglAddon == null) { |
There was a problem hiding this comment.
CRITICAL: Inverted logic bug - the condition should be != null instead of == null
This is a regression from the old CanvasAddon code. The old logic was: "if already using the requested renderer (canvas), return early to avoid reloading". The new logic should be: "if already using dom (webgl is NOT active), return early".
With the current == null check, when setTermRenderer("dom") is called and webglAddon is null (which happens on initial load or after WebGL context loss), the function returns early without:
- Actually ensuring the DOM renderer is active
- Cleaning up the
webglEnabledAtomstate (it stays "true" incorrectly) - Disposing the
webglContextLossDisposable
This specifically breaks the WebGL context loss fallback at line 295 - when WebGL loses context, the terminal will be left in a broken state with no active renderer.
Code Review SummaryStatus: 1 Issue Found | Recommendation: Address before merge Overview
Issue Details (click to expand)CRITICAL
Files Reviewed (6 files)
Note: This PR upgrades xterm.js from v5.5.0 to v6.0.0 and removes several special-case patches for IME and scrolling that were workaround for v5.5.0 issues. The upgrade appears well-planned, but contains one critical logic bug in the renderer fallback code. Reviewed by minimax-m2.5-20260211 · 645,807 tokens |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
frontend/app/view/term/term.tsx (1)
303-303: Potential contradiction:overviewRulerconfigured but hidden in CSS.The
overviewRuler: { width: 6 }is configured here, but interm.scss(lines 131-133),.xterm-decoration-overview-ruleris set todisplay: none !important. This creates unnecessary configuration overhead. Consider removing this option if the ruler is intentionally hidden, or remove the CSS override if the ruler should be visible.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/app/view/term/term.tsx` at line 303, The component sets overviewRuler: { width: 6 } while the CSS class .xterm-decoration-overview-ruler is forcibly hidden; reconcile by either removing the overviewRuler option from the terminal options in term.tsx (remove the overviewRuler config) if the ruler is intentionally hidden, or delete the display:none !important rule for .xterm-decoration-overview-ruler in term.scss so the configured ruler is visible; pick one approach and remove the conflicting artifact (option or CSS) so the configuration and styling are consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@frontend/app/view/term/xterm.css`:
- Around line 238-246: Remove the stray empty line between the background rule
and the transition declaration to satisfy stylelint; in the CSS block containing
"opacity: 1;", "background:rgba(0,0,0,0);" and "transition: opacity 100ms
linear;", ensure there is no blank line immediately before the "transition:
opacity 100ms linear;" declaration so the declarations are contiguous.
In `@package.json`:
- Around line 85-90: Update the xterm addon dependency versions in package.json
to the v6-compatible releases listed in the review: change `@xterm/addon-fit` to
0.10.0, `@xterm/addon-search` to 0.15.0, `@xterm/addon-serialize` to 0.13.0,
`@xterm/addon-web-links` to 0.11.0, and `@xterm/addon-webgl` to 0.18.0 so they match
`@xterm/xterm`@^6.0.0; locate the dependency entries for "@xterm/addon-fit",
"@xterm/addon-search", "@xterm/addon-serialize", "@xterm/addon-web-links", and
"@xterm/addon-webgl" and update their version strings accordingly, then
reinstall (npm/yarn) to lock the resolved versions.
---
Nitpick comments:
In `@frontend/app/view/term/term.tsx`:
- Line 303: The component sets overviewRuler: { width: 6 } while the CSS class
.xterm-decoration-overview-ruler is forcibly hidden; reconcile by either
removing the overviewRuler option from the terminal options in term.tsx (remove
the overviewRuler config) if the ruler is intentionally hidden, or delete the
display:none !important rule for .xterm-decoration-overview-ruler in term.scss
so the configured ruler is visible; pick one approach and remove the conflicting
artifact (option or CSS) so the configuration and styling are consistent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 7c74b64f-d63c-4148-be83-6ac5e20886f2
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (6)
frontend/app/view/term/term-model.tsfrontend/app/view/term/term.scssfrontend/app/view/term/term.tsxfrontend/app/view/term/termwrap.tsfrontend/app/view/term/xterm.csspackage.json
| opacity: 1; | ||
|
|
||
| /* Background rule added for IE9 - to allow clicks on dom node */ | ||
| background:rgba(0,0,0,0); | ||
|
|
||
| transition: opacity 100ms linear; | ||
| /* In front of peek view */ | ||
| z-index: 11; | ||
| } |
There was a problem hiding this comment.
Fix stylelint error: unexpected empty line before declaration.
Static analysis flagged an empty line before the transition declaration on line 243.
Proposed fix
.xterm .xterm-scrollable-element > .visible {
opacity: 1;
-
/* Background rule added for IE9 - to allow clicks on dom node */
background:rgba(0,0,0,0);
-
transition: opacity 100ms linear;
/* In front of peek view */
z-index: 11;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| opacity: 1; | |
| /* Background rule added for IE9 - to allow clicks on dom node */ | |
| background:rgba(0,0,0,0); | |
| transition: opacity 100ms linear; | |
| /* In front of peek view */ | |
| z-index: 11; | |
| } | |
| opacity: 1; | |
| /* Background rule added for IE9 - to allow clicks on dom node */ | |
| background:rgba(0,0,0,0); | |
| transition: opacity 100ms linear; | |
| /* In front of peek view */ | |
| z-index: 11; | |
| } |
🧰 Tools
🪛 Stylelint (17.4.0)
[error] 243-243: Unexpected empty line before declaration (declaration-empty-line-before)
(declaration-empty-line-before)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/app/view/term/xterm.css` around lines 238 - 246, Remove the stray
empty line between the background rule and the transition declaration to satisfy
stylelint; in the CSS block containing "opacity: 1;",
"background:rgba(0,0,0,0);" and "transition: opacity 100ms linear;", ensure
there is no blank line immediately before the "transition: opacity 100ms
linear;" declaration so the declarations are contiguous.
| "@xterm/addon-fit": "^0.11.0", | ||
| "@xterm/addon-search": "^0.16.0", | ||
| "@xterm/addon-serialize": "^0.14.0", | ||
| "@xterm/addon-web-links": "^0.12.0", | ||
| "@xterm/addon-webgl": "^0.19.0", | ||
| "@xterm/xterm": "^6.0.0", |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
xterm.js v6.0.0 addon compatibility versions
💡 Result:
For xterm.js v6.0.0, the official GitHub release notes list the compatible versions of official addons: - @xterm/addon-attach: 0.11.0 - @xterm/addon-canvas: 0.7.0 - @xterm/addon-fit: 0.10.0 - @xterm/addon-image: 0.8.0 - @xterm/addon-ligatures: 0.9.0 - @xterm/addon-search: 0.15.0 - @xterm/addon-serialize: 0.13.0 - @xterm/addon-unicode-graphemes: 0.3.0 - @xterm/addon-unicode11: 0.8.0 - @xterm/addon-web-links: 0.11.0 - @xterm/addon-webgl: 0.18.0 Note that the canvas renderer was removed from the core and moved to an addon (@xterm/addon-canvas). All addons are now under the @xterm scope. Some npm pages show slightly higher versions published later, but the release notes specify these as compatible with v6.0.0.
Citations:
- 1: https://github.com/xtermjs/xterm.js/releases
- 2: https://newreleases.io/project/github/xtermjs/xterm.js/release/6.0.0
- 3: https://snyk.io/redirect/github/xtermjs/xterm.js/releases
Update addon versions to match xterm v6.0.0 compatibility.
Several addons are pinned to versions newer than those officially compatible with @xterm/xterm@^6.0.0. Per the xterm.js v6.0.0 release notes, the compatible addon versions are:
@xterm/addon-fit@0.10.0(currently^0.11.0)@xterm/addon-search@0.15.0(currently^0.16.0)@xterm/addon-serialize@0.13.0(currently^0.14.0)@xterm/addon-web-links@0.11.0(currently^0.12.0)@xterm/addon-webgl@0.18.0(currently^0.19.0)
Update the versions to the officially compatible ranges to ensure stability and avoid potential runtime incompatibilities.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@package.json` around lines 85 - 90, Update the xterm addon dependency
versions in package.json to the v6-compatible releases listed in the review:
change `@xterm/addon-fit` to 0.10.0, `@xterm/addon-search` to 0.15.0,
`@xterm/addon-serialize` to 0.13.0, `@xterm/addon-web-links` to 0.11.0, and
`@xterm/addon-webgl` to 0.18.0 so they match `@xterm/xterm`@^6.0.0; locate the
dependency entries for "@xterm/addon-fit", "@xterm/addon-search",
"@xterm/addon-serialize", "@xterm/addon-web-links", and "@xterm/addon-webgl" and
update their version strings accordingly, then reinstall (npm/yarn) to lock the
resolved versions.
1 participant
removes lots of special case patches for IME and scrolling that were put in to patch over 5.5.0 issues