Skip to content

Feat/theme#54598

Closed
AbdulSadath77 wants to merge 5 commits intoopenclaw:mainfrom
AbdulSadath77:feat/theme
Closed

Feat/theme#54598
AbdulSadath77 wants to merge 5 commits intoopenclaw:mainfrom
AbdulSadath77:feat/theme

Conversation

@AbdulSadath77
Copy link
Copy Markdown

@AbdulSadath77 AbdulSadath77 commented Mar 25, 2026

Summary

Describe the problem and fix in 2–5 bullets:

  • Problem:
  • Why it matters:
  • What changed:
  • What did NOT change (scope boundary):

Change Type (select all)

  • Bug fix
  • Feature
  • Refactor required for the fix
  • Docs
  • Security hardening
  • Chore/infra

Scope (select all touched areas)

  • Gateway / orchestration
  • Skills / tool execution
  • Auth / tokens
  • Memory / storage
  • Integrations
  • API / contracts
  • UI / DX
  • CI/CD / infra

Linked Issue/PR

  • Closes #
  • Related #
  • This PR fixes a bug or regression

Root Cause / Regression History (if applicable)

For bug fixes or regressions, explain why this happened, not just what changed. Otherwise write N/A. If the cause is unclear, write Unknown.

  • Root cause:
  • Missing detection / guardrail:
  • Prior context (git blame, prior PR, issue, or refactor if known):
  • Why this regressed now:
  • If unknown, what was ruled out:

Regression Test Plan (if applicable)

For bug fixes or regressions, name the smallest reliable test coverage that should have caught this. Otherwise write N/A.

  • Coverage level that should have caught this:
    • Unit test
    • Seam / integration test
    • End-to-end test
    • Existing coverage already sufficient
  • Target test or file:
  • Scenario the test should lock in:
  • Why this is the smallest reliable guardrail:
  • Existing test that already covers this (if any):
  • If no new test is added, why not:

User-visible / Behavior Changes

List user-visible changes (including defaults/config).
If none, write None.

Security Impact (required)

  • New permissions/capabilities? (Yes/No)
  • Secrets/tokens handling changed? (Yes/No)
  • New/changed network calls? (Yes/No)
  • Command/tool execution surface changed? (Yes/No)
  • Data access scope changed? (Yes/No)
  • If any Yes, explain risk + mitigation:

Repro + Verification

Environment

  • OS:
  • Runtime/container:
  • Model/provider:
  • Integration/channel (if any):
  • Relevant config (redacted):

Steps

Expected

Actual

Evidence

Attach at least one:

  • Failing test/log before + passing after
  • Trace/log snippets
  • Screenshot/recording
  • Perf numbers (if relevant)

Human Verification (required)

What you personally verified (not just CI), and how:

  • Verified scenarios:
  • Edge cases checked:
  • What you did not verify:

Review Conversations

  • I replied to or resolved every bot review conversation I addressed in this PR.
  • I left unresolved only the conversations that still need reviewer or maintainer judgment.

If a bot review conversation is addressed by this PR, resolve that conversation yourself. Do not leave bot review conversation cleanup for maintainers.

Compatibility / Migration

  • Backward compatible? (Yes/No)
  • Config/env changes? (Yes/No)
  • Migration needed? (Yes/No)
  • If yes, exact upgrade steps:

Failure Recovery (if this breaks)

  • How to disable/revert this change quickly:
  • Files/config to restore:
  • Known bad symptoms reviewers should watch for:

Risks and Mitigations

List only real risks for this PR. Add/remove entries as needed. If none, write None.

  • Risk:
    • Mitigation:

Add new appearance system with GitHub inspired theme presets including light/dark variants with colorblind and tritanopia accessibility modes. Introduces new appearance configuration in settings with sync mode support and perscheme preset selection for light and dark schemes. Updates theme toggle UI to use sync terminology and adds visual preview cards for each appearance option.

the new appearance preset system in ui/src/ui/*
styling updates in ui/src/styles/*
bundled preview assets in ui/src/assets/appearance/*
supporting docs touchups in README.md and docs/reference/credits.md
@openclaw-barnacle openclaw-barnacle bot added docs Improvements or additions to documentation app: web-ui App: web-ui size: XL labels Mar 25, 2026
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: a46786b33a

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

handle_rate_limit() {
local response="$1"
if echo "$response" | grep -q "API rate limit exceeded"; then
fatal "Error: GitHub API rate limit exceeded. Please try again later"
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Define fatal before using it in the Codacy wrapper

This script calls fatal in multiple failure paths, but no fatal function is defined anywhere in the file. When one of these paths is hit (e.g., GitHub API rate limit or missing downloader), the script aborts with fatal: command not found instead of a clear error message, which makes CI/debugging failures opaque and breaks intended error handling.

Useful? React with 👍 / 👎.

@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps bot commented Mar 25, 2026

Greptile Summary

This PR introduces a two-layer appearance system on top of the existing OpenClaw theme families, adding GitHub-style surface palettes (default, dimmed, colorblind, and tritanopia variants for both light and dark) that can be selected independently per scheme slot. It also refactors the flat themeMode: "system" | "light" | "dark" into a structured AppearanceConfig object with an explicit mode: "single" | "sync", lightPreset, darkPreset, and singleScheme, with a proper migration path from the legacy persisted value.

Key changes:

  • New AppearanceConfig / AppearancePreset / ThemeScheme types replace the old ThemeMode type throughout the codebase.
  • resolveTheme is now a pure function: system-scheme resolution is moved to resolveAppearanceScheme / resolveSystemScheme, called at the call site rather than inside resolveTheme.
  • createConfigProps helper eliminates duplicated prop objects across six renderConfig call sites in app-render.ts.
  • Seven GitHub-palette SVG preview assets added for the settings UI.
  • persistSettings / loadSettings correctly serialize/deserialize the new AppearanceConfig struct and fall back to legacy themeMode when the key is absent.

Issues found:

  • appearanceResolved in app.ts is hard-coded to darkPreset at initialization rather than computed via resolveAppearancePreset(...). For a light-mode user or a user with singleScheme: "light", this starts incorrect and is corrected when syncThemeWithSettings fires — a potential brief flash of stale state in the settings panel.
  • set -e +o pipefail in .codacy/cli.sh disables pipefail (+o unsets a flag) instead of enabling it; should be set -eo pipefail.

Confidence Score: 4/5

  • Safe to merge with one targeted fix: the incorrect appearanceResolved initialization in app.ts.
  • The appearance system refactor is well-designed with correct migration logic, good type safety, comprehensive test coverage, and proper persistence. The only logic bug is the appearanceResolved default in app.ts which would cause a brief wrong initial state for light-mode users; it is corrected by the first lifecycle call to syncThemeWithSettings so the user-visible impact is minimal. The cli.sh pipefail issue is in an infra-only file and does not affect the UI at all. No data loss, security risk, or broken primary path.
  • ui/src/ui/app.ts (incorrect appearanceResolved initial value) and .codacy/cli.sh (disabled pipefail).

Comments Outside Diff (1)

  1. ui/src/ui/app.ts, line 2413-2415 (link)

    P2 appearanceResolved initialized to darkPreset unconditionally

    appearanceResolved is always set to darkPreset on component initialization, regardless of the current mode or singleScheme. This means for a user whose mode is "single" with singleScheme: "light", or a user in "sync" mode whose OS is in light mode, the initial component state will report the wrong resolved preset until syncThemeWithSettings runs in the lifecycle.

    The correct initialization should compute the preset with the same logic used everywhere else:

    Note: resolveAppearancePreset is already imported in this file via defaultAppearanceConfig, so this only requires adding it to the existing import from "./theme.ts".

    Prompt To Fix With AI
    This is a comment left during a code review.
    Path: ui/src/ui/app.ts
    Line: 2413-2415
    
    Comment:
    **`appearanceResolved` initialized to `darkPreset` unconditionally**
    
    `appearanceResolved` is always set to `darkPreset` on component initialization, regardless of the current `mode` or `singleScheme`. This means for a user whose mode is `"single"` with `singleScheme: "light"`, or a user in `"sync"` mode whose OS is in light mode, the initial component state will report the wrong resolved preset until `syncThemeWithSettings` runs in the lifecycle.
    
    The correct initialization should compute the preset with the same logic used everywhere else:
    
    
    
    Note: `resolveAppearancePreset` is already imported in this file via `defaultAppearanceConfig`, so this only requires adding it to the existing import from `"./theme.ts"`.
    
    How can I resolve this? If you propose a fix, please make it concise.
Prompt To Fix All With AI
This is a comment left during a code review.
Path: ui/src/ui/app.ts
Line: 2413-2415

Comment:
**`appearanceResolved` initialized to `darkPreset` unconditionally**

`appearanceResolved` is always set to `darkPreset` on component initialization, regardless of the current `mode` or `singleScheme`. This means for a user whose mode is `"single"` with `singleScheme: "light"`, or a user in `"sync"` mode whose OS is in light mode, the initial component state will report the wrong resolved preset until `syncThemeWithSettings` runs in the lifecycle.

The correct initialization should compute the preset with the same logic used everywhere else:

```suggestion
  @state() appearanceResolved: AppearancePreset = resolveAppearancePreset(
    this.settings.appearance ?? defaultAppearanceConfig()
  );
```

Note: `resolveAppearancePreset` is already imported in this file via `defaultAppearanceConfig`, so this only requires adding it to the existing import from `"./theme.ts"`.

How can I resolve this? If you propose a fix, please make it concise.

---

This is a comment left during a code review.
Path: .codacy/cli.sh
Line: 10

Comment:
**`set -e +o pipefail` disables pipefail instead of enabling it**

`+o pipefail` (with a `+`) *disables* the `pipefail` option, which is the opposite of what is likely intended. With pipefail disabled, a failed `curl | grep` or similar pipeline won't cause the script to exit even with `set -e` active, because `set -e` only checks the last command in a pipeline.

The conventional pattern to enable both `errexit` and `pipefail` is:

```suggestion
set -eo pipefail
```

How can I resolve this? If you propose a fix, please make it concise.

Reviews (1): Last reviewed commit: "Merge branch 'main' into feat/theme" | Re-trigger Greptile

bin_name="codacy-cli-v2"

# Determine OS-specific paths
os_name=$(uname)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P2 set -e +o pipefail disables pipefail instead of enabling it

+o pipefail (with a +) disables the pipefail option, which is the opposite of what is likely intended. With pipefail disabled, a failed curl | grep or similar pipeline won't cause the script to exit even with set -e active, because set -e only checks the last command in a pipeline.

The conventional pattern to enable both errexit and pipefail is:

Suggested change
os_name=$(uname)
set -eo pipefail
Prompt To Fix With AI
This is a comment left during a code review.
Path: .codacy/cli.sh
Line: 10

Comment:
**`set -e +o pipefail` disables pipefail instead of enabling it**

`+o pipefail` (with a `+`) *disables* the `pipefail` option, which is the opposite of what is likely intended. With pipefail disabled, a failed `curl | grep` or similar pipeline won't cause the script to exit even with `set -e` active, because `set -e` only checks the last command in a pipeline.

The conventional pattern to enable both `errexit` and `pipefail` is:

```suggestion
set -eo pipefail
```

How can I resolve this? If you propose a fix, please make it concise.

Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 4b981f478c

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Comment on lines +233 to +236
const resolvedScheme =
next === "sync"
? resolveAppearanceScheme({ ...appearance, mode: next })
: host.appearanceSingleScheme;
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Preserve active scheme when entering single mode

When switching appearance mode to single, the resolved scheme is taken from host.appearanceSingleScheme instead of the currently active scheme. In the common case where users are in sync on a light OS and singleScheme is still the default dark, clicking “Single” immediately flips the UI to dark even though they only changed mode, which is an unexpected behavior change. Derive the initial single-mode scheme from the currently resolved/system scheme at the time of the toggle.

Useful? React with 👍 / 👎.

},
],
diagnostics: [],
main
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Remove undefined identifier from registry mock setup

The mock object now includes main as a shorthand property, but no main variable is defined in this module. That makes beforeEach throw ReferenceError: main is not defined, so this test suite fails during setup instead of validating provider resolution behavior.

Useful? React with 👍 / 👎.

- Fix appearance scheme resolution to use resolveAppearanceScheme consistently instead of fallback to host.appearanceSingleScheme
- Update appearanceResolved state to use resolveAppearancePreset function for proper preset calculation
- Fix storage.ts to use _mode parameter when available for theme mode fallback
- Remove unused main property from test fixture in web-search-providers.runtime.test.ts
- Improve bash error handling in codacy cli script with set -eo pipefail and fatal error function
- Add missing resolveAppearancePreset import to app.ts
- Ensure consistent appearance configuration handling across UI initialization
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: c8e50548c1

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Comment on lines 337 to +338
theme: next.theme,
themeMode: next.themeMode,
appearance,
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Keep writing themeMode until the preload script is migrated

The settings payload now persists only appearance, but the pre-hydration bootstrap in ui/index.html still reads themeMode (and does not read appearance) to set data-theme before the app boots. After users save once on this version, themeMode disappears from localStorage, so first paint falls back to system mode and can render the wrong scheme/palette (notably for single mode and preset users) until Lit re-applies settings, causing a visible theme flash on every reload.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: bf6a1951f9

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

if [ "$#" -eq 1 ] && [ "$1" = "download" ]; then
echo "Codacy cli v2 download succeeded"
else
eval "$run_command $*"
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Invoke Codacy binary directly instead of via eval

The wrapper currently executes eval "$run_command $*", which re-parses all arguments in a shell context. This breaks valid inputs that contain spaces (for example, paths) and can execute shell metacharacters if they appear in forwarded arguments, so CI behavior depends on argument contents rather than the intended argv. Execute the binary with quoted argv forwarding instead (direct call with "$@") so arguments are passed verbatim.

Useful? React with 👍 / 👎.

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

Labels

app: web-ui App: web-ui docs Improvements or additions to documentation size: XL

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant