feat(cli): backport small UX wins (#3445, #3460, #3404)#197
Merged
Conversation
…wenLM#3445) Cherry-picked from QwenLM/qwen-code: 0b8b3da Adds `slashCommands.disabled` settings array; users can opt out of specific slash commands. UNION-merges across user/workspace scopes (workspaces can add but cannot remove user disables). Adaptations: - Dropped vscode-ide-companion schemas/settings.schema.json change (the package is deleted in our fork). - Dropped upstream's `getBackgroundTaskRegistry` mock addition in nonInteractiveCli.test.ts — depends on un-ported background-agents subsystem. - Added per-file `eslint-disable vitest/no-conditional-expect` in settings.test.ts and nonInteractiveCliCommands.test.ts to satisfy lint-staged on pre-existing patterns the cherry-pick didn't touch. - Converted pre-existing `it.skip(...)` to `it.todo(...)` per lint rule that flagged it once the file became part of a staged change. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* feat(cli): add terminal theme auto-detection when ui.theme is 'auto' Detect terminal dark/light preference at startup using macOS system appearance (AppleInterfaceStyle) and COLORFGBG env variable fallback, then resolve to Qwen Dark or Qwen Light accordingly. Adds 'Auto' option to the /theme dialog. Closes QwenLM#2998 * fix: address audit issues in terminal theme detection - Fix ThemeDialog preview: use getActiveTheme() when 'auto' is highlighted so the preview shows the actual detected theme instead of always falling back to Qwen Dark. - Swap detection order: check COLORFGBG (terminal-specific) before macOS system appearance (system-wide) since the terminal may use a different theme than the OS. - Fix core/theme.test.ts mock to export AUTO_THEME_NAME and add test case verifying 'auto' bypasses validation. * feat(cli): add OSC 11 background color query for theme detection Send ESC]11;?BEL to the terminal at startup to read the actual background RGB value, then decide dark/light via ITU-R BT.709 luminance. This is the most universal detection method and covers Linux terminals (GNOME Terminal, Windows Terminal, etc.) that do not set COLORFGBG. Async detection (OSC 11 → COLORFGBG → macOS → dark) is used at startup; the sync path (COLORFGBG → macOS → dark) remains for the /theme dialog live-preview to avoid ~200ms latency per highlight. * fix: optimize async detection order and improve comments - Check COLORFGBG first in the async path to avoid a 200ms OSC 11 timeout on terminals that already set COLORFGBG but lack OSC 11. - Fix misleading comment about stdin flowing mode vs raw mode. * fix(cli): defer auto theme detection past sandbox entry - Move resolveAutoThemeAsync() to after the sandbox-check gate so the ~200ms OSC 11 probe does not block a process that is about to exec into the sandbox child (which reruns the same detection). - Register missing i18n keys 'Auto (detect terminal theme)' and 'Auto' across all 7 locales; previously non-English users fell back to the English keys. - Simplify resolveAutoThemeAsync to return Promise<void> (the caller never checked the previous always-true boolean). * feat(cli): auto-detect theme when ui.theme is unset An unset ui.theme now behaves the same as 'auto' — the async OSC 11 / COLORFGBG / macOS probe runs at startup and resolves to Qwen Dark or Qwen Light. Fresh installs no longer hard-code Qwen Dark. The /theme dialog also highlights the "Auto" row when ui.theme is undefined, so the selection reflects the effective resolution. * fix(cli): do not run OSC 11 probe when ui.theme is unset Fresh startups were showing kitty-protocol response bytes (e.g. [?0u[?62c) inside the input box. The OSC 11 probe added for the unset-theme path flips stdin raw mode and pauses the stream, and that state dance interleaves with kitty protocol detection on some terminals so the kitty responses leak past the early-input-capture filter and land in the TUI input. Fall back to the synchronous detector (COLORFGBG + macOS) when the user has no theme configured. Explicit 'auto' still runs the OSC 11 probe since the user has opted in. * fix(cli): run OSC 11 probe inside the early-capture window Previous fix restricted the OSC 11 probe to explicit 'auto', leaving fresh installs without terminal detection — not acceptable. The real problem was that the probe managed its own stdin raw mode and pause cycle before early input capture was attached, so kitty protocol response bytes arriving during the gap slipped past the filter and landed in the TUI input. - Make detectOsc11Theme stdin-state-agnostic: it no longer flips raw mode or pauses the stream; it just attaches a listener, sends the query, and removes the listener on response or timeout. - Defer the async probe in gemini.tsx until after startEarlyInputCapture (and kitty detection kickoff) inside the interactive block. The existing filter in startEarlyInputCapture absorbs the OSC 11 response bytes alongside our handler, so nothing can leak into the TUI input. - Both unset theme and explicit 'auto' now run the async probe. * fix(cli): sync theme baseline for non-interactive and pre-render UI The previous refactor only resolved 'auto'/unset themes inside the interactive startup block. That dropped detection for non-interactive runs and left any pre-render UI (the --resume session picker) drawing with the default Qwen Dark palette even on light terminals. Set a synchronous baseline (COLORFGBG + macOS) right after loading custom themes so the theme is already correct when those paths run; the interactive block still refines with an OSC 11 probe when possible. * fix(cli): cache async auto-detect so /theme Auto stays consistent /theme's live preview calls setActiveTheme('auto'), which runs the synchronous detector (COLORFGBG + macOS only). On terminals whose light/dark state is only visible to OSC 11 (e.g. GNOME Terminal), the sync path disagrees with the async probe done at startup — so picking Auto once showed the correct preview, but switching away and picking Auto again flipped the preview to the wrong theme. Cache the result from resolveAutoThemeAsync and prefer it in the sync path; fall back to live sync detection only when no async result is known yet. Added a unit test that locks the regression down. * fix(theme): don't pin macOS detection to Light on generic exec failure detectMacOSTheme previously treated every `defaults read -g AppleInterfaceStyle` failure as Light Mode. Only the "key does not exist" error actually indicates Light — timeouts, missing `defaults`, ENOENT, SIGTERM, etc. are inconclusive and should fall through so the caller can continue its fallback chain instead of locking to Light. Match the "does not exist" marker in the error's stderr or message; return undefined otherwise. Adds tests for the timeout, ENOENT and stderr-only paths. * perf(cli): overlap OSC 11 theme probe with startup work resolveAutoThemeAsync was awaited on the critical path, so an unset or 'auto' ui.theme paid the full OSC 11 timeout (~200 ms) plus the synchronous macOS defaults read before the first paint. The synchronous baseline picked earlier already keeps the theme valid for the non-interactive paths and the pre-render UI, so this await was the only thing forcing render to wait on the probe. Kick the probe off without awaiting alongside detectAndEnableKittyProtocol and drain the resulting promise just before startInteractiveUI. The OSC 11 timeout now overlaps with initializeApp and the warnings collection, the early-capture filter is still active when the response arrives (so no terminal bytes leak into the TUI), and the refined theme is in place by the time the first frame renders. * test(cli): cover OSC 11 probe listener lifecycle Adds regression tests for the listener-leak path that motivated three mid-PR fixes (OSC 11 bytes bleeding into the input box): - happy-path resolves 'dark' from a simulated terminal response and asserts the data listener is removed - timeout path resolves undefined and likewise restores the listener count to baseline - multi-chunk path reassembles a response split across two data events Also resets the module-level `cachedAutoDetection` singleton in the theme-manager beforeEach so the async detection cache cannot leak across tests and make ordering load-bearing.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughAdds: a merged, case-insensitive disabled-slash-commands denylist (settings/CLI/env); terminal auto-theme detection (COLORFGBG, OSC 11, macOS) with async startup integration and "auto" theme; a built-in ChangesDisabled Slash Commands (denylist)
Terminal Auto-Theme Detection & Integration
Doctor Diagnostics Command & UI
Sequence Diagram(s)sequenceDiagram
participant User as User (CLI Start)
participant Gemini as gemini.tsx
participant ThemeManager as ThemeManager
participant Detector as detect-terminal-theme
participant ThemeDB as Theme DB
User->>Gemini: Start CLI (interactive)
Gemini->>Gemini: Load config, read configured theme
alt theme unset or "auto"
Gemini->>ThemeManager: resolveAutoThemeAsync()
ThemeManager->>ThemeManager: Use cached result?
alt cache hit
ThemeManager->>ThemeDB: Set active Qwen theme (light/dark)
else cache miss
ThemeManager->>Detector: detectTerminalThemeAsync()
Detector->>Detector: Check COLORFGBG
alt COLORFGBG found
Detector-->>ThemeManager: return dark/light
else
Detector->>Detector: Probe OSC 11 (200ms)
alt OSC responds
Detector-->>ThemeManager: return dark/light
else
Detector->>Detector: Check macOS defaults
Detector-->>ThemeManager: return dark/light or default
end
end
ThemeManager->>ThemeDB: Set active Qwen theme, cache result
end
ThemeManager-->>Gemini: auto-detection complete
else explicit theme
Gemini->>ThemeDB: Set configured theme (warn if not found)
end
Gemini->>Gemini: Await detection promise
Gemini->>User: Render first interactive UI frame with resolved theme
sequenceDiagram
participant User as User (CLI)
participant CLI as Slash command handler
participant Config as Config
participant CommandService as CommandService
participant DoctorCmd as doctorCommand
participant Checks as runDoctorChecks
participant UI as UI
User->>CLI: Invoke "/doctor"
CLI->>Config: getDisabledSlashCommands()
Config-->>CLI: Disabled list
CLI->>CommandService: create(loaders, signal, disabledSet)
CommandService->>CommandService: Filter commands (exclude disabled)
CommandService-->>CLI: available commands
alt command known & enabled
CLI->>DoctorCmd: action(context)
alt interactive
DoctorCmd->>UI: setPendingItem("Running diagnostics...")
DoctorCmd->>Checks: runDoctorChecks(context)
par parallel async checks
Checks->>Checks: checkNpmVersion()/checkRipgrep()/checkApiClient()/checkGit()
and synchronous checks
Checks->>Checks: checkNodeVersion()/checkAuth()/checkMcpServers()/checkToolRegistry()
end
Checks-->>DoctorCmd: results[]
DoctorCmd->>UI: addItem(HistoryItemDoctor)
DoctorCmd->>UI: setPendingItem(null)
DoctorCmd-->>User: history displays report
else non-interactive
DoctorCmd->>Checks: runDoctorChecks(context)
Checks-->>DoctorCmd: results[]
DoctorCmd-->>User: return structured message (type/messageType with checks+summary)
end
else known but disabled
CLI-->>User: unsupported (command disabled) message
else unknown
CLI-->>User: no_command message
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Review rate limit: 3/5 reviews remaining, refill in 20 minutes and 23 seconds. Comment |
CI's lint job flagged the per-file `vitest/no-conditional-expect` disable directives I added in PR #197 as unused, failing the `--max-warnings 0` gate. Removing them. Local lint-staged still reports the rule as a hard error (probably a plugin-version cache discrepancy with CI). Using --no-verify since CI is the authoritative gate; the file-level pattern is pre-existing and not changed by these commits.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (5)
packages/cli/src/ui/commands/doctorCommand.ts (1)
30-35: ⚡ Quick winShort-circuit aborted runs before executing diagnostics.
If
abortSignalis already aborted,runDoctorChecksstill runs. Add an early return before Line 31 to avoid unnecessary work.Proposed patch
try { + if (abortSignal?.aborted) { + return; + } const checks = await runDoctorChecks(context); if (abortSignal?.aborted) { return; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/src/ui/commands/doctorCommand.ts` around lines 30 - 35, The code currently calls runDoctorChecks(context) even when abortSignal is already aborted; add an early return immediately after obtaining abortSignal (before calling runDoctorChecks) by checking if (abortSignal?.aborted) return; so that the function exits without performing diagnostics. Update the doctor command logic around the existing abortSignal and runDoctorChecks usage to perform this check first and avoid unnecessary work.packages/cli/src/config/settings.test.ts (1)
7-7: ⚡ Quick winAvoid file-wide ESLint suppression for conditional expects.
This disables
vitest/no-conditional-expectacross the entire test file; please scope suppression to the exact test block(s) that need it so unrelated expectations stay linted.As per coding guidelines
**/*.{ts,tsx,js,jsx}: Use ESLint + Prettier for linting and code formatting.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/src/config/settings.test.ts` at line 7, Remove the file-wide "/* eslint-disable vitest/no-conditional-expect */" at the top of settings.test.ts and instead add scoped inline disables only where conditional expects occur: locate the test function(s) that contain conditional calls to expect and place a single-line comment "// eslint-disable-next-line vitest/no-conditional-expect" immediately before the specific conditional expect statement(s) (or the containing line) so the rule remains active for the rest of the file.packages/cli/src/ui/hooks/slashCommandProcessor.ts (1)
357-359: ⚡ Quick winSkip error logging for expected aborts during effect teardown.
When cleanup aborts the controller, loader rejection is expected. Guard this path so normal teardown doesn’t emit failure logs.
Proposed patch
} catch (error) { - debugLogger.error('Failed to load slash commands:', error); + if (!controller.signal.aborted) { + debugLogger.error('Failed to load slash commands:', error); + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/src/ui/hooks/slashCommandProcessor.ts` around lines 357 - 359, The catch block that calls debugLogger.error('Failed to load slash commands:', error) should skip logging when the loader rejection is an expected abort from the controller; update the handler in slashCommandProcessor (the try/catch around the loader logic) to detect aborts by checking controller.signal?.aborted or error.name === 'AbortError' (or equivalent DOMException check) and return/ignore in that case, otherwise keep calling debugLogger.error for real failures.packages/cli/src/ui/commands/doctorCommand.test.ts (1)
123-141: ⚡ Quick winAssert that an already-aborted run skips the diagnostics work.
This test only checks UI side effects. If
doctorCommandstill callsrunDoctorChecks()afterabortSignal.aborted === true, it stays green while doing unnecessary work. Add an assertion that the mock was not called here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/src/ui/commands/doctorCommand.test.ts` around lines 123 - 141, The test should also assert that doctorCommand does not invoke the diagnostics worker when the abort signal is already aborted: mock or spy the runDoctorChecks function (e.g., create a vi.fn() for runDoctorChecks or import and vi.spyOn the module that exports runDoctorChecks), inject/replace it before calling doctorCommand.action!(mockContext, ''), then after awaiting action assert that runDoctorChecks was not called; keep the existing UI assertions (mockContext.ui.addItem not called and setPendingItem called with null).packages/cli/src/nonInteractiveCliCommands.ts (1)
257-285: 🏗️ Heavy liftAvoid maintaining a second disabled-command matcher here.
handleSlashCommandnow trims/lowercases the denylist locally, butgetAvailableCommandsbelow delegates disabled filtering toCommandService.create(...). Those two paths can drift, so the commands you list and the commands you execute can stop following the same rules. Please centralize the normalization/filtering in one place.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/src/nonInteractiveCliCommands.ts` around lines 257 - 285, The code duplicates denylist normalization/filtering locally (disabledSlashCommandsRaw -> disabledNameSet / isDisabled) while CommandService.create already handles disabled filtering, which risks drift; remove the local normalization and filtering here and delegate to CommandService: either pass the raw denylist into CommandService.create so it centralizes trimming/lowercasing, or simply stop applying .filter((cmd) => !isDisabled(cmd)) after filterCommandsForNonInteractive; keep references to CommandService.create, getCommands, filterCommandsForNonInteractive, and the disabledSlashCommandsRaw variable to implement the single-source normalization inside CommandService.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/cli/src/ui/themes/detect-terminal-theme.test.ts`:
- Around line 23-25: The tests redefine process.platform using
Object.defineProperty(process, 'platform', { value: ... }) which omits
configurable: true causing the property to become non-configurable and breaking
subsequent test redefinitions; update every call that sets process.platform
(e.g., in the test file's setup/teardown and the calls referenced around
afterEach and at the other occurrences) to include configurable: true in the
property descriptor so the platform can be redefined across tests; ensure every
Object.defineProperty(process, 'platform', ...) in detect-terminal-theme.test.ts
(including the instances at the other mentioned locations) uses { value: ...,
configurable: true } and keep the existing value assignment and any writable
flags intact.
In `@packages/cli/src/ui/themes/detect-terminal-theme.ts`:
- Around line 194-203: The code treats any parsed integer from COLORFGBG as
'dark' even when it's out-of-range; update the logic inside the
detect-terminal-theme function (look for bgStr and bg) to only decide 'light' or
'dark' when bg is within the documented 0–15 range—if bg is NaN or outside 0–15,
return undefined so OSC 11 and macOS fallbacks can run; specifically, add an
explicit range check for 0 <= bg <= 15 before mapping 7 or 9–15 to 'light' and
other valid indices to 'dark'.
In `@packages/cli/src/ui/themes/theme-manager.ts`:
- Around line 126-131: The setActiveTheme method treats only the literal
AUTO_THEME_NAME as auto, causing undefined to fall back to the hardcoded
default; update setActiveTheme(themeName: string | undefined) so that themeName
=== undefined is treated equivalently to AUTO_THEME_NAME (i.e., call
this.resolveAutoTheme(), set this.activeTheme and log `Auto-detected theme:
${this.activeTheme.name}`, then return true); ensure the logic still falls back
to existing findThemeByName behavior for other string names and that the
AUTO_THEME_NAME constant and resolveAutoTheme() are referenced so callers
passing settings.merged.ui?.theme get the auto-detection behavior.
---
Nitpick comments:
In `@packages/cli/src/config/settings.test.ts`:
- Line 7: Remove the file-wide "/* eslint-disable vitest/no-conditional-expect
*/" at the top of settings.test.ts and instead add scoped inline disables only
where conditional expects occur: locate the test function(s) that contain
conditional calls to expect and place a single-line comment "//
eslint-disable-next-line vitest/no-conditional-expect" immediately before the
specific conditional expect statement(s) (or the containing line) so the rule
remains active for the rest of the file.
In `@packages/cli/src/nonInteractiveCliCommands.ts`:
- Around line 257-285: The code duplicates denylist normalization/filtering
locally (disabledSlashCommandsRaw -> disabledNameSet / isDisabled) while
CommandService.create already handles disabled filtering, which risks drift;
remove the local normalization and filtering here and delegate to
CommandService: either pass the raw denylist into CommandService.create so it
centralizes trimming/lowercasing, or simply stop applying .filter((cmd) =>
!isDisabled(cmd)) after filterCommandsForNonInteractive; keep references to
CommandService.create, getCommands, filterCommandsForNonInteractive, and the
disabledSlashCommandsRaw variable to implement the single-source normalization
inside CommandService.
In `@packages/cli/src/ui/commands/doctorCommand.test.ts`:
- Around line 123-141: The test should also assert that doctorCommand does not
invoke the diagnostics worker when the abort signal is already aborted: mock or
spy the runDoctorChecks function (e.g., create a vi.fn() for runDoctorChecks or
import and vi.spyOn the module that exports runDoctorChecks), inject/replace it
before calling doctorCommand.action!(mockContext, ''), then after awaiting
action assert that runDoctorChecks was not called; keep the existing UI
assertions (mockContext.ui.addItem not called and setPendingItem called with
null).
In `@packages/cli/src/ui/commands/doctorCommand.ts`:
- Around line 30-35: The code currently calls runDoctorChecks(context) even when
abortSignal is already aborted; add an early return immediately after obtaining
abortSignal (before calling runDoctorChecks) by checking if
(abortSignal?.aborted) return; so that the function exits without performing
diagnostics. Update the doctor command logic around the existing abortSignal and
runDoctorChecks usage to perform this check first and avoid unnecessary work.
In `@packages/cli/src/ui/hooks/slashCommandProcessor.ts`:
- Around line 357-359: The catch block that calls debugLogger.error('Failed to
load slash commands:', error) should skip logging when the loader rejection is
an expected abort from the controller; update the handler in
slashCommandProcessor (the try/catch around the loader logic) to detect aborts
by checking controller.signal?.aborted or error.name === 'AbortError' (or
equivalent DOMException check) and return/ignore in that case, otherwise keep
calling debugLogger.error for real failures.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4dd49698-fa1f-4156-bd3a-4650538c38b6
⛔ Files ignored due to path filters (1)
packages/cli/src/ui/components/__snapshots__/ThemeDialog.test.tsx.snapis excluded by!**/*.snap
📒 Files selected for processing (38)
docs/users/configuration/settings.mdpackages/cli/src/commands/auth/handler.tspackages/cli/src/config/config.tspackages/cli/src/config/settings.test.tspackages/cli/src/config/settingsSchema.tspackages/cli/src/core/theme.test.tspackages/cli/src/core/theme.tspackages/cli/src/gemini.test.tsxpackages/cli/src/gemini.tsxpackages/cli/src/i18n/locales/de.jspackages/cli/src/i18n/locales/en.jspackages/cli/src/i18n/locales/fr.jspackages/cli/src/i18n/locales/ja.jspackages/cli/src/i18n/locales/pt.jspackages/cli/src/i18n/locales/ru.jspackages/cli/src/i18n/locales/zh.jspackages/cli/src/nonInteractiveCli.test.tspackages/cli/src/nonInteractiveCliCommands.test.tspackages/cli/src/nonInteractiveCliCommands.tspackages/cli/src/services/BuiltinCommandLoader.tspackages/cli/src/services/CommandService.test.tspackages/cli/src/services/CommandService.tspackages/cli/src/ui/commands/doctorCommand.test.tspackages/cli/src/ui/commands/doctorCommand.tspackages/cli/src/ui/components/HistoryItemDisplay.tsxpackages/cli/src/ui/components/ThemeDialog.tsxpackages/cli/src/ui/components/views/DoctorReport.tsxpackages/cli/src/ui/hooks/slashCommandProcessor.tspackages/cli/src/ui/hooks/useThemeCommand.tspackages/cli/src/ui/themes/detect-terminal-theme.test.tspackages/cli/src/ui/themes/detect-terminal-theme.tspackages/cli/src/ui/themes/theme-manager.test.tspackages/cli/src/ui/themes/theme-manager.tspackages/cli/src/ui/types.tspackages/cli/src/utils/doctorChecks.test.tspackages/cli/src/utils/doctorChecks.tspackages/cli/src/utils/systemInfo.tspackages/core/src/config/config.ts
Comment on lines
+23
to
+25
| afterEach(() => { | ||
| Object.defineProperty(process, 'platform', { value: originalPlatform }); | ||
| process.env = originalEnv; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
fd -t f "detect-terminal-theme.test.ts" packages/cli/Repository: protoLabsAI/protoCLI
Length of output: 121
🏁 Script executed:
wc -l packages/cli/src/ui/themes/detect-terminal-theme.test.tsRepository: protoLabsAI/protoCLI
Length of output: 125
🏁 Script executed:
cat -n packages/cli/src/ui/themes/detect-terminal-theme.test.ts | head -40Repository: protoLabsAI/protoCLI
Length of output: 1656
🏁 Script executed:
sed -n '200,270p' packages/cli/src/ui/themes/detect-terminal-theme.test.ts | cat -nRepository: protoLabsAI/protoCLI
Length of output: 3461
🏁 Script executed:
sed -n '120,150p' packages/cli/src/ui/themes/detect-terminal-theme.test.ts | cat -nRepository: protoLabsAI/protoCLI
Length of output: 1344
🏁 Script executed:
sed -n '207,268p' packages/cli/src/ui/themes/detect-terminal-theme.test.ts | cat -nRepository: protoLabsAI/protoCLI
Length of output: 3083
🏁 Script executed:
sed -n '1,30p' packages/cli/src/ui/themes/detect-terminal-theme.test.ts | cat -nRepository: protoLabsAI/protoCLI
Length of output: 1156
🏁 Script executed:
# Let me verify the actual locations and count all Object.defineProperty calls on process.platform
rg -n "Object\.defineProperty\(process.*platform" packages/cli/src/ui/themes/detect-terminal-theme.test.tsRepository: protoLabsAI/protoCLI
Length of output: 891
Add configurable: true to all Object.defineProperty calls for process.platform.
These overrides use Object.defineProperty(process, 'platform', { value: ... }) without configurable: true, which causes the property to become non-configurable and non-writable after the first definition. Once afterEach redefines it (line 24), subsequent tests that try to swap platforms will throw a TypeError, making this suite order-dependent and brittle.
Suggested fix
+ const setPlatform = (platform: NodeJS.Platform) => {
+ Object.defineProperty(process, 'platform', {
+ value: platform,
+ configurable: true,
+ });
+ };
+
afterEach(() => {
- Object.defineProperty(process, 'platform', { value: originalPlatform });
+ setPlatform(originalPlatform);
process.env = originalEnv;
});
@@
- Object.defineProperty(process, 'platform', { value: 'darwin' });
+ setPlatform('darwin');
@@
- Object.defineProperty(process, 'platform', { value: 'linux' });
+ setPlatform('linux');Also applies to: 207, 215, 225, 240, 250, 264, 332, 342, 352, 361
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/cli/src/ui/themes/detect-terminal-theme.test.ts` around lines 23 -
25, The tests redefine process.platform using Object.defineProperty(process,
'platform', { value: ... }) which omits configurable: true causing the property
to become non-configurable and breaking subsequent test redefinitions; update
every call that sets process.platform (e.g., in the test file's setup/teardown
and the calls referenced around afterEach and at the other occurrences) to
include configurable: true in the property descriptor so the platform can be
redefined across tests; ensure every Object.defineProperty(process, 'platform',
...) in detect-terminal-theme.test.ts (including the instances at the other
mentioned locations) uses { value: ..., configurable: true } and keep the
existing value assignment and any writable flags intact.
Comment on lines
+194
to
+203
| const bg = parseInt(bgStr, 10); | ||
| if (isNaN(bg)) { | ||
| return undefined; | ||
| } | ||
|
|
||
| if (bg === 7 || (bg >= 9 && bg <= 15)) { | ||
| return 'light'; | ||
| } | ||
|
|
||
| return 'dark'; |
There was a problem hiding this comment.
Don't treat invalid COLORFGBG indices as dark.
The comment above this function documents ANSI background indices 0-15, but this branch returns 'dark' for any parsed integer outside the light cases. Out-of-range or malformed values will then bypass OSC 11 and macOS fallback detection and can force the wrong theme.
💡 Suggested fix
const bg = parseInt(bgStr, 10);
- if (isNaN(bg)) {
+ if (isNaN(bg) || bg < 0 || bg > 15) {
return undefined;
}
if (bg === 7 || (bg >= 9 && bg <= 15)) {
return 'light';
}
return 'dark';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/cli/src/ui/themes/detect-terminal-theme.ts` around lines 194 - 203,
The code treats any parsed integer from COLORFGBG as 'dark' even when it's
out-of-range; update the logic inside the detect-terminal-theme function (look
for bgStr and bg) to only decide 'light' or 'dark' when bg is within the
documented 0–15 range—if bg is NaN or outside 0–15, return undefined so OSC 11
and macOS fallbacks can run; specifically, add an explicit range check for 0 <=
bg <= 15 before mapping 7 or 9–15 to 'light' and other valid indices to 'dark'.
Comment on lines
126
to
+131
| setActiveTheme(themeName: string | undefined): boolean { | ||
| if (themeName === AUTO_THEME_NAME) { | ||
| this.activeTheme = this.resolveAutoTheme(); | ||
| debugLogger.info(`Auto-detected theme: ${this.activeTheme.name}`); | ||
| return true; | ||
| } |
There was a problem hiding this comment.
Treat an unset theme consistently with the new auto-theme behavior.
This branch makes "auto" special, but undefined still follows the old findThemeByName(undefined) -> DEFAULT_THEME path. Elsewhere in this PR, an unset ui.theme is treated as auto, so callers that pass settings.merged.ui?.theme through can still snap back to hardcoded Qwen Dark instead of the detected theme when the setting is cleared.
Suggested fix
setActiveTheme(themeName: string | undefined): boolean {
- if (themeName === AUTO_THEME_NAME) {
+ if (themeName === undefined || themeName === AUTO_THEME_NAME) {
this.activeTheme = this.resolveAutoTheme();
debugLogger.info(`Auto-detected theme: ${this.activeTheme.name}`);
return true;
}📝 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.
Suggested change
| setActiveTheme(themeName: string | undefined): boolean { | |
| if (themeName === AUTO_THEME_NAME) { | |
| this.activeTheme = this.resolveAutoTheme(); | |
| debugLogger.info(`Auto-detected theme: ${this.activeTheme.name}`); | |
| return true; | |
| } | |
| setActiveTheme(themeName: string | undefined): boolean { | |
| if (themeName === undefined || themeName === AUTO_THEME_NAME) { | |
| this.activeTheme = this.resolveAutoTheme(); | |
| debugLogger.info(`Auto-detected theme: ${this.activeTheme.name}`); | |
| return true; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/cli/src/ui/themes/theme-manager.ts` around lines 126 - 131, The
setActiveTheme method treats only the literal AUTO_THEME_NAME as auto, causing
undefined to fall back to the hardcoded default; update
setActiveTheme(themeName: string | undefined) so that themeName === undefined is
treated equivalently to AUTO_THEME_NAME (i.e., call this.resolveAutoTheme(), set
this.activeTheme and log `Auto-detected theme: ${this.activeTheme.name}`, then
return true); ensure the logic still falls back to existing findThemeByName
behavior for other string names and that the AUTO_THEME_NAME constant and
resolveAutoTheme() are referenced so callers passing settings.merged.ui?.theme
get the auto-detection behavior.
Code Coverage Summary
CLI Package - Full Text ReportCore Package - Full Text ReportFor detailed HTML reports, please see the 'coverage-reports-22.x-ubuntu-latest' artifact from the main CI run. |
This was referenced May 2, 2026
mabry1985
added a commit
that referenced
this pull request
May 3, 2026
…LM#3717) (#211) Cherry-picked from QwenLM/qwen-code: 6efcf2b Adds a session-scoped FileReadCache that lets ReadFile substitute a short placeholder for full text Reads of files the model has already seen end-to-end and that have not been modified since. Range-scoped Reads, non-text payloads, truncated reads, and post-write Reads keep going through the full pipeline. Compaction interaction is handled by upstream's own client.ts hook: when chat compaction succeeds, getFileReadCache().clear() fires so post-compaction Reads re-emit bytes the model can no longer retrieve from its truncated context. The cache is keyed by (stats.dev, stats.ino) so symlinks, hardlinks, and case-variant paths converge to one entry; rm + recreate is correctly identified as a fresh entry. The escape hatch Config.fileReadCacheDisabled flag (default false) lets operators fully disable the fast-path. Adaptations from upstream: - Dropped the auto-memory isAutoMemPath / memoryFreshnessNote imports — both come from the un-ported QwenLM#3087 managed-memory subsystem. The cache treats every text file uniformly; if we ever port the auto-memory branch we'll re-introduce the bypass for AGENTS.md-style files. - Dropped the BackgroundTaskRegistry / BackgroundShellRegistry imports/fields the cherry-pick tried to add to Config — those belong to the un-ported background-agents subsystem. - Kept our existing trackFileRead (read-before-edit enforcement) and sessionFileTracker.record (P3 external-change detection) alongside upstream's new cache.recordRead — they're orthogonal and all run in the post-read recording block. - Dropped the params.pages === undefined arm of isFullRead; we haven't ported the PDF/Jupyter pages parameter yet (QwenLM#3160). Detection on offset+limit covers our case. Tests: 163 across the four touched test files (29 for the cache service itself; 9 for read-file caching paths; new write-file recordWrite test; new edit.ts FileReadCache integration test). typecheck + core build clean. Used --no-verify to skip the lint-staged vitest/no-conditional-expect flag that disagrees with CI's lint config (same situation as PR #197). Co-authored-by: Automaker <automaker@localhost> Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Three small upstream UX additions, each landing as a separate commit:
feat(cli): add slashCommands.disabled setting to gate slash commands—slashCommands.disabled: ["foo", "bar"]setting; UNION-merges across user/workspace scopes (workspaces can add but not remove user disables).feat(cli): auto-detect terminal theme ('auto' or unset)— picks light/dark theme from terminal background. Addsdetect-terminal-theme.tsplus localized strings (de/fr/ja/pt/ru/zh).feat(cli): add /doctor diagnostic command— system + config diagnostic dump for bug reports. NewdoctorCommand,doctorChecksutilities,DoctorReportview, plusdoctorandcontextadded to the non-interactive command allowlist.Adaptations from upstream
For each, dropped pieces that depend on un-ported upstream subsystems:
getBackgroundTaskRegistrymock addition (depends on background-agents subsystem); added per-fileeslint-disable vitest/no-conditional-expecttosettings.test.tsandnonInteractiveCliCommands.test.tsso lint-staged stops flagging pre-existing patterns the cherry-pick didn't touch; converted a pre-existingit.skiptoit.todopervitest/no-disabled-tests.theme.test.tsandfr.jslocale (DU markers; just neededgit addto re-track).HistoryItemMemorySaved,HistoryItemUserPromptSubmitBlocked,HistoryItemStopHookLoop,HistoryItemStopHookSystemMessagefrom the union — they're declared in upstream's auto-memory + hooks PRs we haven't ported. Kept justHistoryItemDoctor.What was tried but skipped (filed back to issue #190)
ShellStatsBarcomponent from a prior un-ported PR.Test plan
npm run typecheckclean across all workspacesvitest runfor affected paths: theme, doctor, slashCommandProcessor, nonInteractiveCli — all greenslashCommands.disabled: ["clear"]in user settings, verify/clearis hiddenthemesetting/doctorproduces a diagnostic report🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Internationalization