Skip to content

fix(UI)): ensure terminal theme application via xterm theme setter#3278

Draft
jhaumont wants to merge 1 commit intowavetermdev:mainfrom
jhaumont:fix-term-theme-application
Draft

fix(UI)): ensure terminal theme application via xterm theme setter#3278
jhaumont wants to merge 1 commit intowavetermdev:mainfrom
jhaumont:fix-term-theme-application

Conversation

@jhaumont
Copy link
Copy Markdown

@jhaumont jhaumont commented May 4, 2026

When you try to set a theme on a widget:

"test": {
    "icon": "dharmachakra",
    "label": "test",
    "color": "#326ce5",
    "blockdef": {
      "meta": {
        "view": "term",
        "controller": "cmd",
        "cmd": "wsl.exe -d Debian --shell-type login bash -ic \"k9s\"",
        "cmd:interactive": true,
        "cmd:runonstart": true,
        "term:theme": "Dracula"
      }
    }

It's not take in account. You have to manually change the theme of the block.

Purpose of this PR is to fix this and having the widget taking into account the theme from start.

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented May 4, 2026

Walkthrough

These changes refactor how the terminal theme is applied in the terminal view. A new setTheme() method is introduced in the TermWrap class that applies the theme using either the setOption() API (when available) or direct property assignment as a fallback. The TermThemeUpdater hook's useEffect is updated to call this new method instead of directly mutating the terminal options object.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 inconclusive)

Check name Status Explanation Resolution
Description check ❓ Inconclusive No description was provided by the author, making it impossible to evaluate whether the description relates to the changeset. Add a pull request description explaining the fix, the problem it addresses, and any testing performed.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately describes the main change: fixing terminal theme application by using xterm's theme setter method instead of direct mutation.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share
Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.

Comment @coderabbitai help to get the list of available commands and usage tips.

@jhaumont
Copy link
Copy Markdown
Author

jhaumont commented May 4, 2026

@coderabbitai review

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented May 4, 2026

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
frontend/app/view/term/termwrap.ts (1)

339-345: ⚡ Quick win

setOption is dead code for @xterm/xterm ^6.0.0 — simplify to direct assignment.

setOption and getOption were fully removed in xterm.js 5.0.0 in favour of terminal.options.* assignment. Since this project constrains to @xterm/xterm ^6.0.0, typeof (this.terminal as any).setOption will always evaluate to "undefined", making that branch permanently dead code. The (this.terminal as any) cast also bypasses TypeScript safety for a method that doesn't exist.

The fallback (this.terminal.options.theme = theme) is the only path ever executed, so setTheme can be simplified to match the existing sibling methods setCursorStyle and setCursorBlink, which already use direct options assignment without any guard:

♻️ Proposed refactor
     setTheme(theme: TermTypes.ITerminalOptions["theme"]) {
-        if (typeof (this.terminal as any).setOption === "function") {
-            (this.terminal as any).setOption("theme", theme);
-        } else {
-            this.terminal.options.theme = theme;
-        }
+        this.terminal.options.theme = theme;
     }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@frontend/app/view/term/termwrap.ts` around lines 339 - 345, The setTheme
method contains dead code checking for a non-existent setOption and uses an
unnecessary any cast; simplify it to directly assign the theme to the terminal
options like the sibling methods do: remove the typeof check and (this.terminal
as any) cast, and update setTheme to set this.terminal.options.theme = theme
using the existing ITerminalOptions typing for terminal (reference: setTheme
method and terminal.options.theme).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@frontend/app/view/term/termwrap.ts`:
- Around line 339-345: The setTheme method contains dead code checking for a
non-existent setOption and uses an unnecessary any cast; simplify it to directly
assign the theme to the terminal options like the sibling methods do: remove the
typeof check and (this.terminal as any) cast, and update setTheme to set
this.terminal.options.theme = theme using the existing ITerminalOptions typing
for terminal (reference: setTheme method and terminal.options.theme).

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 83a43cc9-da8c-4bc9-846c-9246233b282d

📥 Commits

Reviewing files that changed from the base of the PR and between 2e25ea1 and 769b541.

📒 Files selected for processing (2)
  • frontend/app/view/term/termtheme.ts
  • frontend/app/view/term/termwrap.ts

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant