Feat/titlebar theme match and version pill#1102
Open
MaanilVerma wants to merge 3 commits into
Open
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughRender current install version as its own pill and show an update action pill that can display "Update {version}". Add ITU-R luminance helpers and readableSymbolColor; propagate reported themeBg into title-bar CSS vars and use a canvas-normalized renderer helper to decide light/dark styling. Pill-ars aligned. ChangesVersion pill visibility and theme-aware title bar
Suggested reviewers
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install timed out. The project may have too many dependencies for the sandbox. 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/main/host/attach.ts`:
- Line 385: Replace the empty catch blocks on the four executeJavaScript calls
so errors are logged instead of silently swallowed: for
comfyContents.executeJavaScript(COMFY_THEME_OBSERVER_JS),
comfyContents.executeJavaScript(MODEL_DOWNLOAD_SCRIPT_JS),
comfyContents.executeJavaScript(TERMINAL_SCRIPT_JS) and
comfyContents.executeJavaScript(CLOUD_PATCHES_JS) attach a .catch(err => { /*
log error */ }) that records the error (e.g., console.warn or the module's
logger) with context about which script failed; do not rethrow — just log the
error and continue so injection failures are visible but non-blocking.
- Around line 232-233: The empty catch around comfyWindow.setTitleBarOverlay
swallows errors; change the catch to capture the error (e.g., catch (err)) and
log it so failures are visible — replace the current try {
comfyWindow.setTitleBarOverlay({ color: theme.bg, symbolColor: theme.text }) }
catch { } with a try/catch that logs the exception (for example
console.error("Failed to setTitleBarOverlay", { theme, err }) or hook into the
app's telemetry/logger) so the error and context are preserved for diagnostics.
In `@src/main/lib/theme.ts`:
- Line 27: Extract the duplicated formula into a single exported pure helper
function (e.g., computePerceivedLuminance(r:number,g:number,b:number):number)
and replace the inline expression `(r * 299 + g * 587 + b * 114) / 1000` with
calls to that helper; keep each file's local color parsing/normalization code
as-is and only change the place that computes luminance (replace the const
luminance = ... expression in the function that currently defines it with const
luminance = computePerceivedLuminance(r,g,b)), then import the shared helper
where needed so all consumers use the same single source of truth.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 34288eea-63a9-49a9-8be0-67c20665f030
📒 Files selected for processing (9)
locales/en.jsonlocales/zh.jsonsrc/main/host/attach.tssrc/main/lib/theme.tssrc/renderer/src/comfyTitleBar/TitleBarApp.vuesrc/renderer/src/comfyTitleBar/useTitleBarIdentity.tssrc/renderer/src/views/chooser/ChooserInstallTile.vuesrc/renderer/src/views/chooser/chooser-tiles.csssrc/types/ipc.ts
- Drive the title-bar header + OS window-controls overlay from ComfyUI's reported bg while inside an instance, instead of locking to brand purple; symbol color is luminance-derived so the window controls stay legible on any theme - Un-stub `isLight` so the existing `.is-light` chrome activates, and extend it to the resting/hover/open states it never covered (install pill, downloads tray, icon buttons) so nothing washes out or lifts to invisible yellow on a light bar - Scoped to attached instances only — the dashboard/chooser keeps its purple
- Render the current version pill independently so an available update no
longer hides it (was a v-if/v-else-if chain)
- Right-align the update/migrate action so it reads as the affordance, apart
from the source + version metadata; version becomes the secondary shrink
target after source
- Carry the target version in the update label ("Update v0.25.0") via the new
`chooser.updatePillVersion` key, sourced from `statusTag.version`
MaanilVerma
force-pushed
the
feat/titlebar-theme-match-and-version-pill
branch
from
1932c5f to
b0b6733
Compare
|
|
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/renderer/src/views/chooser/ChooserInstallTile.vue (1)
175-177: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winExtract one shared action dispatcher so this pill doesn’t drift and spill.
The guardedemit('trigger-action', ...)logic is duplicated across both pills, so move it into one helper and call that from the three event bindings.♻️ Proposed refactor
const sourcePillLabel = computed(() => inst.value.sourceId === 'desktop' ? inst.value.sourceLabel : inst.value.listPreview || inst.value.sourceLabel, ) +function triggerInstallAction(action: 'update' | 'migrate'): void { + if (props.isStoppedActionGated) return + emit('trigger-action', action, inst.value) +} + function handleClick(): void { if (isStopping.value) return emit('pick', inst.value) }- `@click.stop`="isStoppedActionGated || emit('trigger-action', 'update', inst)" - `@keydown.enter.stop`="isStoppedActionGated || emit('trigger-action', 'update', inst)" - `@keydown.space.prevent.stop`="isStoppedActionGated || emit('trigger-action', 'update', inst)" + `@click.stop`="triggerInstallAction('update')" + `@keydown.enter.stop`="triggerInstallAction('update')" + `@keydown.space.prevent.stop`="triggerInstallAction('update')" @@ - `@click.stop`="isStoppedActionGated || emit('trigger-action', 'migrate', inst)" - `@keydown.enter.stop`="isStoppedActionGated || emit('trigger-action', 'migrate', inst)" - `@keydown.space.prevent.stop`="isStoppedActionGated || emit('trigger-action', 'migrate', inst)" + `@click.stop`="triggerInstallAction('migrate')" + `@keydown.enter.stop`="triggerInstallAction('migrate')" + `@keydown.space.prevent.stop`="triggerInstallAction('migrate')"As per coding guidelines, “After creating or modifying code, check for duplicated logic and extract it into shared variables, computed properties, or helpers to prevent divergence.”
Also applies to: 190-192
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/renderer/src/views/chooser/ChooserInstallTile.vue` around lines 175 - 177, Duplicate guarded emit logic for triggering actions should be extracted into a single helper; add a method (e.g., handleTriggerAction(action, inst) or dispatchTriggerAction) that performs "if (!isStoppedActionGated) emit('trigger-action', action, inst)" and replace the inline expressions in the three bindings (`@click.stop`, `@keydown.enter.stop`, `@keydown.space.prevent.stop`) for the pill (and the equivalent three bindings around lines 190-192) to call that helper with the 'update' action and inst instead of repeating the guarded emit expression.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/renderer/src/comfyTitleBar/useTitleBarIdentity.ts`:
- Around line 96-108: Extract the duplicated color-normalization + lightness
test into a single exported helper (e.g., export function isColorLight(color:
string): boolean) that (1) normalizes the input color to a hex string using the
canvas trick, (2) parses r,g,b, and (3) calls perceivedLuminance(...) >=
LUMINANCE_LIGHT_THRESHOLD; then replace the inline logic inside the isLight
computed in useTitleBarIdentity (currently doing canvas fillStyle -> hex ->
parseInt -> perceivedLuminance) with a call to isColorLight(themeBg.value), and
update TitlePopupApp.vue to call the same helper instead of duplicating the
logic. Ensure the helper reuses the existing perceivedLuminance and
LUMINANCE_LIGHT_THRESHOLD symbols and export it from a shared utilities module
so both consumers can import it.
---
Outside diff comments:
In `@src/renderer/src/views/chooser/ChooserInstallTile.vue`:
- Around line 175-177: Duplicate guarded emit logic for triggering actions
should be extracted into a single helper; add a method (e.g.,
handleTriggerAction(action, inst) or dispatchTriggerAction) that performs "if
(!isStoppedActionGated) emit('trigger-action', action, inst)" and replace the
inline expressions in the three bindings (`@click.stop`, `@keydown.enter.stop`,
`@keydown.space.prevent.stop`) for the pill (and the equivalent three bindings
around lines 190-192) to call that helper with the 'update' action and inst
instead of repeating the guarded emit expression.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: d1b33788-26ec-44ed-bbb6-79fd3ec72761
📒 Files selected for processing (11)
locales/en.jsonlocales/zh.jsonsrc/main/host/attach.tssrc/main/lib/theme.tssrc/renderer/src/comfyTitleBar/TitleBarApp.vuesrc/renderer/src/comfyTitleBar/useTitleBarIdentity.tssrc/renderer/src/comfyTitlePopup/TitlePopupApp.vuesrc/renderer/src/views/chooser/ChooserInstallTile.vuesrc/renderer/src/views/chooser/chooser-tiles.csssrc/shared/colorLuminance.tssrc/types/ipc.ts
- Extract the renderer's canvas-normalize + lightness test into a single
`isColorLight` helper (`lib/colorScheme.ts`), reused by `useTitleBarIdentity`
and `TitlePopupApp` instead of two copies; it reuses the shared
`perceivedLuminance` math
- Collapse the guarded `emit('trigger-action', …)` repeated across the update +
migrate pills into one `triggerInstallAction` method in `ChooserInstallTile`
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
4 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
Match the desktop title bar to ComfyUI's in-page theme while inside an instance (dark/light, Win/Linux/macOS), and stop the dashboard tile from hiding an install's version when an update is available. Two independent fixes, one commit each.
Changes
What
applyComfyTheme(src/main/host/attach.ts) — drive the Vue header and the OS window-controls overlay from ComfyUI's reported--comfy-menu-bgin one call, so the strip behind min/max/close stays seamless with the bar (the divergence fixed in fix(titlebar): center pill at true window center + unify OS-control color on Windows #647).symbolColoris luminance-derived (readableSymbolColor) so the glyphs stay legible on any theme. Instance-only — the install-less chooser keeps--titlebar-bg.readableSymbolColor(src/main/lib/theme.ts) — new helper: parses#rgb/#rrggbb/rgb(), returns#333on light bgs /#dddon dark by perceived luminance; dark-safe fallback on parse failure.isLight(src/renderer/src/comfyTitleBar/useTitleBarIdentity.ts) — un-stubbed fromfalseto the real luminance test, activating the existing.is-lightchrome.TitleBarApp.vue— bind--titlebar-bg-active/--titlebar-iconinline from the reported theme; extend.is-lightto the resting/hover/open states it never covered (install pill, downloads tray, icon buttons) so nothing washes out or lifts to invisible yellow on a light bar.ChooserInstallTile.vue+chooser-tiles.css— render the version pill independently of the action pill (was av-if/v-else-ifchain), right-align the update/migrate action (.chooser-tile-pill-action), and carry the target version in the label (Update v0.25.0) viachooser.updatePillVersion, sourced fromstatusTag.version.src/types/ipc.ts,locales/en.json,locales/zh.json— addstatusTag.versionto the rendererInstallationtype and the newupdatePillVersionkey.Breaking
None. The theme path only runs for attached instances (
applyComfyThemeis bound insideattachInstall); the chooser/dashboard still reverts viaapplyChooserHostTheme. Thedesktop2-theme-reportIPC channel, overlay-at-creation defaults, and the migrate/update action emits are unchanged.Review Focus
theme.bgin one call — that's what keeps the min/max/close strip seamless (re fix(titlebar): center pill at true window center + unify OS-control color on Windows #647). Confirm no path updates the Vue header theme for an instance without the matching overlay repaint..is-lightwas already authored but gated behind a hardcodedisLight = false; this enables it and fills the resting/hover/open gaps. Worth a glance on a custom (non-standard) ComfyUI theme.Testing
Unit/typecheck-only by design: the title-bar binding and pill split are presentational, and the existing
TitleBarAppsuite already exercises the theme-changed and pill-render paths. No new user-visible IPC behavior to E2E.src/renderer/src/comfyTitleBar/TitleBarApp.test.ts— 59 tests pass (theme binding + pill render unaffected).pnpm typecheck(node/web/e2e/integration),pnpm lint— clean.Update v…pill both visible.Screen Recording
Screen.Recording.2026-06-13.at.5.50.54.PM.mov
closes #1060