feat: add light mode toggle by kirti7617 · Pull Request #1513 · alibaba/open-code-review

kirti7617 · 2026-09-21T07:53:59Z

Description

Type of Change

Bug fix (non-breaking change that fixes an issue)
New feature (non-breaking change that adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)
Refactoring (no functional changes)
Documentation update
CI / Build / Tooling

How Has This Been Tested?

make test passes locally
Manual testing (describe below)

Checklist

My code follows the project's coding style (go fmt, go vet)
I have performed a self-review of my code
I have added tests that prove my fix is effective or my feature works
New and existing unit tests pass locally with my changes
I have updated the documentation accordingly (if applicable)
I have signed the CLA
I did not use AI/LLM to create this PR, or I disclosed the tool/model below and reviewed its output; I did not attribute commits to AI and will answer maintainer questions and review comments myself without AI/LLM.

Related Issues

CLAassistant · 2026-09-21T07:54:10Z

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

github-actions · 2026-09-21T07:58:29Z

🔍 OpenCodeReview found 6 issue(s) in this PR.

✅ Successfully posted inline: 6 comment(s)

github-actions · 2026-09-21T07:58:30Z

+    --badge-model-bg: rgba(129, 140, 248, 0.12);    --badge-model-fg: #a5b4fc;
+    --badge-tokens-bg: rgba(52, 211, 153, 0.12);   --badge-tokens-fg: #6ee7b7;
+    --badge-duration-bg: rgba(251, 191, 36, 0.12); --badge-duration-fg: #fcd34d;
+    --badge-error-bg: rgba(248, 113, 113, 0.12);   --badge-error-fg: #fca5a5;


Inconsistent formatting: two declarations on one line.

Multiple badge-related custom properties place two declarations on the same line (e.g., --badge-model-bg: ...; --badge-model-fg: ...;). This is inconsistent with the single-declaration-per-line style used throughout the rest of the file, reduces readability, and makes future diffs noisier. Consider splitting each pair onto separate lines.

github-actions · 2026-09-21T07:58:30Z

+@media (prefers-color-scheme: dark) {
+    :root {
+        --bg: #000000;
+        --surface: #0a0a0a;
+        --surface-alt: #141414;
+        --surface-inset: #111111;


Missing variables in @media (prefers-color-scheme: dark) compared to body[data-theme="dark"].

This media query block is missing several variables that are defined in the body[data-theme="dark"] block above, including:

--text-faint

--code-bg, --inline-code-bg

--task-memory, --task-relocation

--badge-neutral-fg

--severity-critical-bg, --severity-critical-fg

--severity-low-bg, --severity-low-fg

--radius, --radius-sm, --radius-xs, --transition

For users with JS disabled and a dark OS preference, these variables will fall back to the :root defaults. While the :root defaults are now dark-themed (so most will match), this inconsistency is fragile — any future change to :root defaults could silently break auto-detected dark mode. Either make this block complete, or remove it entirely since the JS (a11y.js) always sets body[data-theme] on load.

github-actions · 2026-09-21T07:58:30Z

+        --task-main: #818cf8;
+        --task-plan: #a78bfa;
+        --task-grouping: #22d3ee;
+        --task-default: #6b7394;


Inconsistent --task-default value across dark theme definitions. The @media (prefers-color-scheme: dark) blocks set --task-default: #6b7394, but the body[data-theme="dark"] block (and the :root default) use --task-default: #57606a. This means users who get dark mode via system preference will see a different color than users who explicitly toggle to dark mode. These should be consistent unless the difference is intentional.

github-actions · 2026-09-21T07:58:30Z

+/* ── Reset & Base ── */
+*, *::before, *::after { box-sizing: border-box; margin: 0; padding: 0; }
+
+body {
+    font-family: var(--font);
+    background: var(--bg);
+    color: var(--text);


Duplicate CSS blocks. The Reset & Base section (*, *::before, *::after, body, code, pre) and the Navigation section (nav.breadcrumb, etc.) appear twice in this file — once here (newly added) and again starting at line 519 (original). This duplication adds ~80 lines of unnecessary CSS and could cause confusion during future maintenance. The duplicate @media (prefers-color-scheme: dark) block also appears twice (line 234 and line 459).

It appears the new theme variable blocks were correctly inserted after :root, but the subsequent reset/base/nav styles were added without removing the originals further down in the file.

github-actions · 2026-09-21T07:58:30Z

+nav.breadcrumb .nav-brand .brand-icon {
    /* Text */
    --text: rgba(0, 0, 0, 0.77);
    --text-strong: rgba(0, 0, 0, 0.87);


Critical: Old :root light-theme variables incorrectly scoped to .brand-icon.

The diff shows that the original :root { ... } block's light-theme variable declarations were not removed — they were re-scoped under nav.breadcrumb .nav-brand .brand-icon. This means:

The .brand-icon element now has dozens of CSS custom property overrides (full light-theme tokens) applied to it, which is almost certainly unintended.

The entire block of reset/base/navigation styles (lines 294–370) is duplicated later in the file (starting around line 519), because the old content was kept instead of replaced.

A second duplicate @media (prefers-color-scheme: dark) block also exists later (line 459).

The old :root light-theme variable block should be fully removed. The :root selector at line 7 already defines dark defaults, and body[data-theme="light"] / body[data-theme="dark"] handle explicit themes. The leftover declarations here create a bloated, duplicated stylesheet with an erroneously styled .brand-icon.

Confirmed. The light-theme variables were accidentally moved under .brand-icon, and the reset/navigation/theme blocks are duplicated. This is more than a cleanup issue because it can also make the logo inherit light-theme colors in dark mode.

github-actions · 2026-09-21T07:58:30Z

 */}}
 {{define "app-header"}}
-<nav class="breadcrumb">{{template "nav-brand" true}}{{template "breadcrumbs" .}}</nav>
+<nav class="breadcrumb">{{template "nav-brand" true}}{{template "breadcrumbs" .}}{{template "theme-toggle"}}{{end}}


Bug: The </nav> closing tag was accidentally replaced with {{end}}. This causes two problems:

The {{end}} here prematurely closes the {{define "app-header"}} block, making the {{end}} on the next line an unmatched extra end tag — this will cause a template parse error.

The <nav> element is never properly closed, resulting in invalid HTML.

The {{end}} should remain as </nav> to close the nav element. The {{define}} block's own {{end}} on the next line is sufficient.

Suggestion:

Suggested change

<nav class="breadcrumb">{{template "nav-brand" true}}{{template "breadcrumbs" .}}{{template "theme-toggle"}}{{end}}

<nav class="breadcrumb">{{template "nav-brand" true}}{{template "breadcrumbs" .}}{{template "theme-toggle"}}</nav>

Confirmed. </nav> was accidentally replaced by {{end}}, which makes the template invalid. This should be fixed before merging.

chaojixinren · 2026-09-21T10:40:46Z

Thanks for the contribution. I left replies on the blocking template/CSS issues above.

Also, the PR description is still the default empty template. Please fill in the description and testing sections so the intended theme behavior, persistence strategy, and verification steps are clear.

Given the current template/CSS issues, I think these should be addressed before merging. I can re-review once they are fixed.

kirti7617 · 2026-09-21T17:00:37Z

Hi! Thank you for reviewing my PR. Could you please add me to the contributors list? I would really appreciate it. Thank you!'

feat: add light mode toggle

4a94445

github-actions Bot reviewed Sep 21, 2026

View reviewed changes

Navigation Menu

feat: add light mode toggle - #1513

feat: add light mode toggle#1513
kirti7617 wants to merge 1 commit into
alibaba:mainfrom
kirti7617:feat/light-mode-button

kirti7617 commented Sep 21, 2026

Uh oh!

CLAassistant commented Sep 21, 2026

Uh oh!

github-actions Bot commented Sep 21, 2026 •

edited

Loading

Uh oh!

github-actions Bot Sep 21, 2026

Uh oh!

github-actions Bot Sep 21, 2026

Uh oh!

github-actions Bot Sep 21, 2026

Uh oh!

github-actions Bot Sep 21, 2026

Uh oh!

github-actions Bot Sep 21, 2026

Uh oh!

chaojixinren Sep 21, 2026

Uh oh!

github-actions Bot Sep 21, 2026

Uh oh!

chaojixinren Sep 21, 2026

Uh oh!

chaojixinren commented Sep 21, 2026

Uh oh!

kirti7617 commented Sep 21, 2026

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

	<nav class="breadcrumb">{{template "nav-brand" true}}{{template "breadcrumbs" .}}{{template "theme-toggle"}}{{end}}
	<nav class="breadcrumb">{{template "nav-brand" true}}{{template "breadcrumbs" .}}{{template "theme-toggle"}}</nav>

Conversation

kirti7617 commented Sep 21, 2026

Description

Type of Change

How Has This Been Tested?

Checklist

Related Issues

Uh oh!

CLAassistant commented Sep 21, 2026

Uh oh!

github-actions Bot commented Sep 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot Sep 21, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Sep 21, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Sep 21, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Sep 21, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Sep 21, 2026

Choose a reason for hiding this comment

Uh oh!

chaojixinren Sep 21, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Sep 21, 2026

Choose a reason for hiding this comment

Uh oh!

chaojixinren Sep 21, 2026

Choose a reason for hiding this comment

Uh oh!

chaojixinren commented Sep 21, 2026

Uh oh!

kirti7617 commented Sep 21, 2026

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

github-actions Bot commented Sep 21, 2026 •

edited

Loading