Decouple Desktop UI into monorepo app #5912

webfiltered · 2025-10-04T06:57:21Z

Summary

Extracts desktop UI into apps/desktop-ui package with minimal changes.

Changes

What:
- Separates desktop-specific code into standalone package with independent Vite config, router, and i18n
- Drastically simplifies the main app router by removing all desktop routes
- Adds a some code duplication, most due to the existing design
- Some duplication can be refactored to be simpler on either side - no need to split things by isElectron()
- Rudimentary storybook support has been added
Breaking: Stacked PR for publishing must be merged before this PR makes it to stable core (but publishing could be done manually)
- Add Desktop UI npm publishing workflows #5915
Dependencies: Takes full advantage of pnpm catalog. No additional dependencies added.

Review Focus

Should be no changes to normal frontend operation
Scripts added to root package.json are acceptable
The duplication in this PR is copied as is, wherever possible. Any corrections or fix-ups beyond the scope of simply migrating the functionality as-is, can be addressed in later PRs. That said, if any changes are made, it instantly becomes more difficult to separate the duplicated code out into a shared utility.
- Tracking issue to address concerns: Address all Desktop UI app review comments (post-migration) #5925

i18n

Fixing i18n is out of scope for this PR. It is a larger task that we should consider carefully and implement properly. Attempting to isolate the desktop i18n and duplicate the current localisation scripts would be wasted energy.

github-actions · 2025-10-04T06:57:47Z

🎨 Storybook Build Status

✅ Build completed successfully!

⏰ Completed at: 10/05/2025, 04:51:29 AM UTC

🔗 Links

📊 View Workflow Run
🎨 View Storybook

🎉 Your Storybook is ready for review!

github-actions · 2025-10-04T06:57:53Z

🎭 Playwright Test Results

⚠️ Tests passed with flaky tests

⏰ Completed at: 10/05/2025, 05:05:26 AM UTC

📈 Summary

Total Tests: 489
Passed: 457 ✅
Failed: 0
Flaky: 2 ⚠️
Skipped: 30 ⏭️

📊 Test Reports by Browser

✅ chromium: View Report • ✅ 448 / ❌ 0 / ⚠️ 2 / ⏭️ 30
✅ chromium-2x: View Report • ✅ 2 / ❌ 0 / ⚠️ 0 / ⏭️ 0
✅ chromium-0.5x: View Report • ✅ 1 / ❌ 0 / ⚠️ 0 / ⏭️ 0
✅ mobile-chrome: View Report • ✅ 6 / ❌ 0 / ⚠️ 0 / ⏭️ 0

🎉 Click on the links above to view detailed test results for each browser configuration.

Adds apps/desktop-ui package setup, Vite config, TypeScript config, base index.html, public assets, env/types, and base router/i18n/main wiring.

Relocates Desktop UI components, views, stores, types, and constants under apps/desktop-ui with no content changes (pure moves).

Updates existing app router to accommodate Desktop UI extraction.

Added by pattern from packages/* packages.

Copies style to local style.css

apps/desktop-ui/.storybook/main.ts

apps/desktop-ui/package.json

scripts/collect-i18n-general.ts

claude

Comprehensive PR Review

This review is generated by Claude. It may not always be accurate, as with human reviewers. If you believe that any of the comments are invalid or incorrect, please state why for each. For others, please implement the changes in one way or another.

Review Summary

PR: Decouple Desktop UI into monorepo app (#5912)
Impact: 1188 additions, 96 deletions across 67 files

Issue Distribution

Critical: 0
High: 1
Medium: 4
Low: 3

Category Breakdown

Architecture: 4 issues
Security: 2 issues
Performance: 1 issue
Code Quality: 1 issue

Key Findings

Architecture & Design

This PR successfully extracts desktop-specific functionality into a standalone apps/desktop-ui package, which is a positive architectural move. The separation creates clear boundaries and eliminates conditional isElectron() checks from the main app. However, several consistency issues were identified:

Missing functions in desktop utilities compared to main app (showNativeSystemMenu)
Port conflicts between desktop and main Storybook instances
Cross-dependencies from build scripts to desktop app constants
Missing TypeScript type exports for consistency

The move to use file renaming (R100) rather than copy-paste is excellent for git history preservation.

Security Considerations

Two security vulnerabilities were identified in development configurations:

Storybook server configured with allowedHosts: true creates host header attack vulnerability
Vite dev server exposes to all network interfaces (0.0.0.0) when VITE_REMOTE_DEV=true

Both should be addressed before production deployment of development tooling.

Performance Impact

The desktop app eagerly loads all locale files at startup, which increases initial bundle size. Consider implementing lazy locale loading based on detected language for better startup performance.

Integration Points

The desktop UI properly integrates with:

Nx build orchestration with appropriate caching and outputs
pnpm workspace catalog for dependency management
Shared frontend utilities package
Main app locale files via path aliasing

The CODEOWNERS file was properly updated to consolidate ownership under @webfiltered.

Positive Observations

Clean separation of concerns with minimal code duplication
Proper use of Nx monorepo architecture with appropriate tagging
Good use of pnpm catalog for dependency management
Comprehensive Storybook setup for component development
Maintains TypeScript strict mode compliance
Proper Vue 3 Composition API patterns throughout
Well-structured component architecture with proper prop/emit patterns

References

Next Steps

Address the high-priority security issue in Storybook configuration
Resolve API consistency between desktop and main utilities
Fix port conflicts for simultaneous development
Consider performance optimization for locale loading
Verify all development tooling works correctly in the new structure

This is a comprehensive automated review. For architectural decisions requiring human judgment, please request additional manual review.

webfiltered · 2025-10-04T19:35:55Z

Comprehensive PR Review

Addressed all comments individually. Resolved all that were not introduced by this PR, just replicated, hallucinations, requests to add unused code, or just ridiculous.

webfiltered · 2025-10-04T19:43:14Z

For visibility: I used both Sonnet 4.5 and Opus (it's a big PR, it needs some big understanding) to review this PR locally. Only came back with similar issues - things that are out of scope for this PR.

I did caveat the bots with the i18n situation (only), as I realise it's not great. I also don't have the time to make it a pre-req for merge. Have updated PR description.

webfiltered · 2025-10-04T19:49:55Z

@christian-byrne Did you guys get codex working on the repo? Can it review?

christian-byrne

LGTM

christian-byrne · 2025-10-04T20:26:12Z

@christian-byrne Did you guys get codex working on the repo? Can it review?

@benceruleanlu can you remind me how it's done again?

webfiltered · 2025-10-04T21:24:39Z

@codex review

chatgpt-codex-connector · 2025-10-04T21:24:44Z

To use Codex here, create a Codex account and connect to github.

DrJKL · 2025-10-05T00:36:33Z

scripts/collect-i18n-general.ts

@@ -1,12 +1,12 @@
 import * as fs from 'fs'

+import { DESKTOP_DIALOGS } from '../apps/desktop-ui/src/constants/desktopDialogs'


Want to add a path in the root tsconfig?
Maybe '@desktop/*': './apps/desktop-ui/src'

My thought on this is that we should do this when we move the frontend to an app - properly, and all at once.

Right now the root tsconfig is actually still just the frontend tsconfig - and it doesn't make sense to have it ref the desktop.

DrJKL · 2025-10-05T00:38:19Z

apps/desktop-ui/src/assets/css/style.css

+#desktop-app {
+  position: absolute;
+  inset: 0;
+}


Can't use tailwind utilities for these?

Like-for-like migration - honestly, I went one step too far here and didn't use top:0; left:0; width:100%; height:100%; (because .. no).

DrJKL · 2025-10-05T00:39:26Z

apps/desktop-ui/src/components/bottomPanel/tabs/terminal/BaseTerminal.vue

+      size="small"
+      :class="
+        cn('absolute top-2 right-8 transition-opacity', {
+          'opacity-0 pointer-events-none select-none': !isHovered


The CSS hover doesn't work for this?

This isn't a change, afaik. (edit: confirmed!)

ComfyUI_frontend/src/components/bottomPanel/tabs/terminal/BaseTerminal.vue

Lines 16 to 20 in 15783ed

size="small"

:class="

cn('absolute top-2 right-8 transition-opacity', {

'opacity-0 pointer-events-none select-none': !isHovered

})

These files were duplicated as is - i.e. the PR isn't to make changes, it's to do the split. Trying to fix things at the same time as you migrate them is almost always a misstep; if there are any issues, you will have no idea what caused it.

In my experience, it's usually the opportunistic fix.

I'll add context to the PR body.

DrJKL · 2025-10-05T00:41:13Z

apps/desktop-ui/src/components/bottomPanel/tabs/terminal/BaseTerminal.vue

+const terminalEl = ref<HTMLElement | undefined>()
+const rootEl = ref<HTMLElement | undefined>()


nit: The possibly-undefinedness is implicit when you don't pass in a default.

Suggested change

const terminalEl = ref<HTMLElement | undefined>()

const rootEl = ref<HTMLElement | undefined>()

const terminalEl = ref<HTMLElement>()

const rootEl = ref<HTMLElement>()

Should be no change here too - can address post-migration.

ComfyUI_frontend/src/components/bottomPanel/tabs/terminal/BaseTerminal.vue

Lines 46 to 47 in 15783ed

const terminalEl = ref<HTMLElement | undefined>()

const rootEl = ref<HTMLElement | undefined>()

DrJKL · 2025-10-05T00:45:52Z

apps/desktop-ui/src/main.ts

+const ComfyUIPreset = definePreset(Aura, {
+  semantic: {
+    // @ts-expect-error fixme ts strict error
+    primary: Aura['primitive'].blue
+  }
+})


Should some of this also be moved to the design system package?

Should be no change here, also - can address post-migration.

ComfyUI_frontend/src/main.ts

Lines 23 to 29 in 15783ed

const ComfyUIPreset = definePreset(Aura, {

semantic: {

// @ts-expect-error fixme ts strict error

primary: Aura['primitive'].blue

}

})

apps/desktop-ui/tsconfig.json

webfiltered

The original images were not removed, and are being duplicated here for likely no reason. Comfy brand mark will probably see re-use. This can be moved to an assets package.

Originally LLM-parsed and "fixed"

The external: () => false configuration causes a PostCSS error during CSS URL processing. Removing it fixes the build without affecting dependency bundling.

## Summary Adds automated npm publishing for @comfyorg/desktop-ui package with version management and release workflows. - Ref: #5912 ## Changes - **What**: Three GitHub Actions workflows for desktop-ui npm publishing automation ### Two functions 1. Bump action - Just creates a version bump PR for `desktop-ui` 2. Publish action - Can be run manually - essentially a function with params / void return ### One automation - Watches for matching commits, then calls the Publish action with pre-filled details ## Review Focus Security hardening and workflow correctness. ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-5915-Add-Desktop-UI-npm-publishing-workflows-2826d73d365081d9b7f8d7f752536ceb) by [Unito](https://www.unito.io)

## Summary Extracts desktop UI into apps/desktop-ui package with minimal changes. ## Changes - **What**: - Separates desktop-specific code into standalone package with independent Vite config, router, and i18n - Drastically simplifies the main app router by removing all desktop routes - Adds a some code duplication, most due to the existing design - Some duplication can be refactored to be *simpler* on either side - no need to split things by `isElectron()` - Rudimentary storybook support has been added - **Breaking**: Stacked PR for publishing must be merged before this PR makes it to stable core (but publishing _could_ be done manually) - #5915 - **Dependencies**: Takes full advantage of pnpm catalog. No additional dependencies added. ## Review Focus - Should be no changes to normal frontend operation - Scripts added to root package.json are acceptable - The duplication in this PR is copied as is, wherever possible. Any corrections or fix-ups beyond the scope of simply migrating the functionality as-is, can be addressed in later PRs. That said, if any changes are made, it instantly becomes more difficult to separate the duplicated code out into a shared utility. - Tracking issue to address concerns: #5925 ### i18n Fixing i18n is out of scope for this PR. It is a larger task that we should consider carefully and implement properly. Attempting to isolate the desktop i18n and duplicate the _current_ localisation scripts would be wasted energy.

## Summary Adds automated npm publishing for @comfyorg/desktop-ui package with version management and release workflows. - Ref: #5912 ## Changes - **What**: Three GitHub Actions workflows for desktop-ui npm publishing automation ### Two functions 1. Bump action - Just creates a version bump PR for `desktop-ui` 2. Publish action - Can be run manually - essentially a function with params / void return ### One automation - Watches for matching commits, then calls the Publish action with pre-filled details ## Review Focus Security hardening and workflow correctness. ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-5915-Add-Desktop-UI-npm-publishing-workflows-2826d73d365081d9b7f8d7f752536ceb) by [Unito](https://www.unito.io)

Extracts desktop UI into apps/desktop-ui package with minimal changes. - **What**: - Separates desktop-specific code into standalone package with independent Vite config, router, and i18n - Drastically simplifies the main app router by removing all desktop routes - Adds a some code duplication, most due to the existing design - Some duplication can be refactored to be *simpler* on either side - no need to split things by `isElectron()` - Rudimentary storybook support has been added - **Breaking**: Stacked PR for publishing must be merged before this PR makes it to stable core (but publishing _could_ be done manually) - #5915 - **Dependencies**: Takes full advantage of pnpm catalog. No additional dependencies added. - Should be no changes to normal frontend operation - Scripts added to root package.json are acceptable - The duplication in this PR is copied as is, wherever possible. Any corrections or fix-ups beyond the scope of simply migrating the functionality as-is, can be addressed in later PRs. That said, if any changes are made, it instantly becomes more difficult to separate the duplicated code out into a shared utility. - Tracking issue to address concerns: #5925 Fixing i18n is out of scope for this PR. It is a larger task that we should consider carefully and implement properly. Attempting to isolate the desktop i18n and duplicate the _current_ localisation scripts would be wasted energy.

dosubot bot added the size:XL This PR changes 500-999 lines, ignoring generated files. label Oct 4, 2025

webfiltered marked this pull request as draft October 4, 2025 07:00

webfiltered marked this pull request as ready for review October 4, 2025 08:51

dosubot bot added size:L This PR changes 100-499 lines, ignoring generated files. and removed size:XL This PR changes 500-999 lines, ignoring generated files. labels Oct 4, 2025

webfiltered changed the title ~~Decouple desktop UI into monorepo app~~ Decouple Desktop UI into monorepo app Oct 4, 2025

webfiltered added 22 commits October 4, 2025 20:07

Add app scaffolding (Vite, TS, router, i18n)

de76a37

Adds apps/desktop-ui package setup, Vite config, TypeScript config, base index.html, public assets, env/types, and base router/i18n/main wiring.

Move desktop code to apps/desktop-ui (pure moves)

2bef3f2

Relocates Desktop UI components, views, stores, types, and constants under apps/desktop-ui with no content changes (pure moves).

Adjust root src/router.ts for desktop app split

0534150

Updates existing app router to accommodate Desktop UI extraction.

Add duplicate components / utils

213fc98

Adjust CODEOWNERS

2f2dd58

Update package lock

ab28018

Fix desktop-ui import paths and remove broken stories file

373ba8c

Configure desktop-ui dev server to use port 5174

77dc5d3

Fix compiler errors in vite config

0c921fe

Add root repo scripts for desktop-ui dev and build

3573989

Decouple desktop-ui from root vite config

bc8ae41

Add shared-frontend-utils dependency

1b8d030

Use shared-frontend-utils package in desktop-ui

77ca4bb

Remove private flag from desktop-ui

b0c7ba5

Added by pattern from packages/* packages.

[ts] Fix type issue

53f2215

Autoformat

507cabb

Add missing dependencies to desktop-ui

e84f9de

Remove redundant ts @ path alias

543dd08

Update package lock file

b14d81d

Rename root #vue-app HTML ID to prevent conflict

1c32618

Copies style to local style.css

Fix i18n collection script broken by migration

0022c60

Remove unused files; migrated to desktop app

4a795bb

claude bot reviewed Oct 4, 2025

View reviewed changes

apps/desktop-ui/.storybook/main.ts Show resolved Hide resolved

claude bot reviewed Oct 4, 2025

View reviewed changes

apps/desktop-ui/package.json Show resolved Hide resolved

claude bot reviewed Oct 4, 2025

View reviewed changes

scripts/collect-i18n-general.ts Show resolved Hide resolved

claude bot reviewed Oct 4, 2025

View reviewed changes

christian-byrne previously approved these changes Oct 4, 2025

View reviewed changes

DrJKL previously approved these changes Oct 5, 2025

View reviewed changes

webfiltered mentioned this pull request Oct 5, 2025

Address all Desktop UI app review comments (post-migration) #5925

Open

Remove unnecessary globs from tsconfig

bc2fd61

webfiltered dismissed stale reviews from DrJKL and christian-byrne via bc2fd61 October 5, 2025 01:57

webfiltered commented Oct 5, 2025

View reviewed changes

webfiltered added 5 commits October 5, 2025 14:08

Remove migrated Desktop UI images

979a37c

Restore storybook config to migration of base repo

d5eff0e

Originally LLM-parsed and "fixed"

Fix Storybook build by removing broken external config

b6b2534

Fix Storybook won't build

2e0473e

Remove broken external config from root Storybook

b9d42c1

The external: () => false configuration causes a PostCSS error during CSS URL processing. Removing it fixes the build without affecting dependency bundling.

christian-byrne approved these changes Oct 5, 2025

View reviewed changes

webfiltered enabled auto-merge (squash) October 5, 2025 04:51

webfiltered merged commit 07a74e3 into main Oct 5, 2025
25 checks passed

webfiltered deleted the desktop-ui-c branch October 5, 2025 05:04

		@@ -1,12 +1,12 @@
		import * as fs from 'fs'

		import { DESKTOP_DIALOGS } from '../apps/desktop-ui/src/constants/desktopDialogs'

	size="small"
	:class="
	cn('absolute top-2 right-8 transition-opacity', {
	'opacity-0 pointer-events-none select-none': !isHovered
	})

		const terminalEl = ref<HTMLElement \| undefined>()
		const rootEl = ref<HTMLElement \| undefined>()

	const ComfyUIPreset = definePreset(Aura, {
	semantic: {
	// @ts-expect-error fixme ts strict error
	primary: Aura['primitive'].blue
	}
	})

Decouple Desktop UI into monorepo app #5912

Decouple Desktop UI into monorepo app #5912

Uh oh!

Conversation

webfiltered commented Oct 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Changes

Review Focus

i18n

Uh oh!

github-actions bot commented Oct 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🎨 Storybook Build Status

🔗 Links

Uh oh!

github-actions bot commented Oct 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🎭 Playwright Test Results

📈 Summary

📊 Test Reports by Browser

Uh oh!

Uh oh!

Uh oh!

Uh oh!

claude bot left a comment

Choose a reason for hiding this comment

Comprehensive PR Review

Review Summary

Issue Distribution

Category Breakdown

Key Findings

Architecture & Design

Security Considerations

Performance Impact

Integration Points

Positive Observations

References

Next Steps

Uh oh!

webfiltered commented Oct 4, 2025

Comprehensive PR Review

Uh oh!

webfiltered commented Oct 4, 2025

Uh oh!

webfiltered commented Oct 4, 2025

Uh oh!

christian-byrne left a comment

Choose a reason for hiding this comment

Uh oh!

christian-byrne commented Oct 4, 2025

Uh oh!

webfiltered commented Oct 4, 2025

Uh oh!

chatgpt-codex-connector bot commented Oct 4, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

webfiltered Oct 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

webfiltered Oct 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

webfiltered commented Oct 4, 2025 •

edited

Loading

github-actions bot commented Oct 4, 2025 •

edited

Loading

github-actions bot commented Oct 4, 2025 •

edited

Loading

webfiltered Oct 5, 2025 •

edited

Loading

webfiltered Oct 5, 2025 •

edited

Loading