Workspaces 3 create a workspace#8221
Merged
christian-byrne merged 8 commits intomainfrom Jan 22, 2026
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughAdds workspace-first features: avatar, switcher, settings & subscription workspace panels, workspace dialogs, workspace UI composable, dialog service helpers, exported workspace types, firebase auth header helper, locales, and retrying teamWorkspaceStore initialization. Changes
Sequence Diagram(s)mermaid Possibly related PRs
Suggested reviewers
✨ Finishing touches
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 |
simula-r
force-pushed
the
workspaces-3-create-a-workspace
branch
from
adcee25 to
39e2691
Compare
🎨 Storybook Build Status✅ Build completed successfully! ⏰ Completed at: 01/22/2026, 02:58:36 AM UTC 🔗 Links🎉 Your Storybook is ready for review! |
🎭 Playwright Tests:
|
Bundle Size ReportSummary
Category Glance Per-category breakdownApp Entry Points — 22.6 kB (baseline 22.3 kB) • 🔴 +315 BMain entry bundles and manifests
Status: 1 added / 1 removed Graph Workspace — 1.02 MB (baseline 1.02 MB) • 🔴 +2.54 kBGraph editor runtime, canvas, workflow orchestration
Status: 1 added / 1 removed Views & Navigation — 80.7 kB (baseline 80.7 kB) • ⚪ 0 BTop-level views, pages, and routed surfaces
Status: 11 added / 11 removed Panels & Settings — 439 kB (baseline 430 kB) • 🔴 +8.52 kBConfiguration panels, inspectors, and settings screens
Status: 22 added / 21 removed User & Accounts — 3.94 kB (baseline 3.94 kB) • ⚪ 0 BAuthentication, profile, and account management bundles
Status: 3 added / 3 removed Editors & Dialogs — 2.83 kB (baseline 2.8 kB) • 🔴 +31 BModals, dialogs, drawers, and in-app editors
Status: 2 added / 2 removed UI Components — 33.3 kB (baseline 32.8 kB) • 🔴 +517 BReusable component library chunks
Status: 9 added / 8 removed Data & Services — 3.1 MB (baseline 3.06 MB) • 🔴 +37 kBStores, services, APIs, and repositories
Status: 9 added / 8 removed Utilities & Hooks — 24 kB (baseline 18.1 kB) • 🔴 +5.86 kBHelpers, composables, and utility bundles
Status: 10 added / 7 removed Vendor & Third-Party — 10.4 MB (baseline 10.4 MB) • ⚪ 0 BExternal libraries and shared vendor chunks
Status: 5 added / 5 removed Other — 6.34 MB (baseline 6.28 MB) • 🔴 +59.9 kBBundles that do not match a named category
Status: 85 added / 78 removed |
- Add edit, leave, and delete workspace dialog content components - Add workspace settings panel with subscription content - Fix subscription auth to use Firebase token instead of workspace token - Disable subscribe button until billing BE is ready - Hide edit option for personal workspaces in menu Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
simula-r
added
needs-backport
dosubot
bot
added
the
size:XXL
coderabbitai
bot
requested review from
KarryCharon,
Yorha4D,
christian-byrne and
shinshin86
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 30
🤖 Fix all issues with AI agents
In `@src/components/common/WorkspaceProfilePic.vue`:
- Around line 16-23: The computed `letter` can be an empty string so
`letter.value.charCodeAt(0)` returns NaN and breaks the gradient; update the
`letter` computed (or the `gradient` computed) to guard against empty
workspaceName by using a falsy fallback (e.g., use `|| '?'` when deriving
`letter`) or by checking `letter.value` before calling `charCodeAt`, and ensure
`gradient` uses a fallback numeric seed (e.g., charCode of '?') when
`charCodeAt` would be NaN; change references to workspaceName, letter, gradient,
and the charCodeAt call accordingly so the gradient always receives a valid
integer seed.
In `@src/components/dialog/content/setting/WorkspacePanelContent.vue`:
- Around line 4-7: Remove the Tailwind "!" prefix from the class on the
WorkspaceProfilePic usage — change class="size-12 !text-3xl" to class="size-12
text-3xl" and if that breaks internal styling of WorkspaceProfilePic, add a
size/textSize prop to the WorkspaceProfilePic component (e.g., textSize or
variant) and pass the desired value from this parent (WorkspaceProfilePic
:text-size="'3xl'") so the component can apply the correct text sizing
internally instead of relying on "!important" overrides.
- Around line 36-40: Replace the array-style :class binding in
WorkspacePanelContent.vue with the cn() utility from '@/utils/tailwindUtil':
import cn in the component script and change :class="['flex items-center gap-2
px-3 py-2', item.class, item.disabled ? 'pointer-events-auto' : '']" to use
:class="cn('flex items-center gap-2 px-3 py-2', item.class, item.disabled ?
'pointer-events-auto' : '')", preserving the conditional for item.disabled;
ensure the cn import is added to the script/setup or script block where the
component is defined.
In `@src/components/dialog/content/workspace/CreateWorkspaceDialogContent.vue`:
- Around line 30-38: The text input bound to workspaceName in
CreateWorkspaceDialogContent.vue should include maxlength and autofocus for
better UX; update the <input> (the element using v-model="workspaceName" and
`@keydown.enter`="isValidName && onCreate()") to add maxlength="50" to match the
validation rule and add the autofocus attribute so the field is focused when the
dialog opens.
- Around line 91-112: The dialog is closed before workspace creation completes
in onCreate, so move the dialogStore.closeDialog call to after the awaited
workspaceStore.createWorkspace(name) (and after the optional await
onConfirm?.(name) if you need the callback to run first) so the dialog only
closes on successful creation; keep the try/catch/finally and loading.value
handling as-is so errors still surface and loading is cleared on failure.
- Around line 12-18: Replace the plain HTML close <button> in
CreateWorkspaceDialogContent.vue with the repository's common Button component
(from components/button), import and register it in the component, and pass the
same props/attributes: :aria-label="$t('g.close')", `@click`="onCancel", matching
styling/variants (use the equivalent variant/size props instead of raw classes
or keep necessary classes), and render the existing icon (<i class="pi pi-times
size-4" />) as the Button's content; ensure the Button is used wherever the
close button is rendered so theming and behavior are consistent.
In `@src/components/dialog/content/workspace/DeleteWorkspaceDialogContent.vue`:
- Around line 12-18: Replace the raw <button> close icon with the shared
IconButton component from src/components/button/ to ensure consistent theming
and behavior; locate the close button in DeleteWorkspaceDialogContent.vue (the
element that currently binds `@click`="onCancel" and aria-label $t('g.close')) and
swap it to use the IconButton (or equivalent shared button) while preserving the
onCancel handler, aria-label, icon class (pi pi-times) and visual props
(size/variant) so the component matches existing repo styles and accessibility.
- Around line 39-40: The template has an invalid binding on the Button
component: `:loading` with no expression causes a compile error; update the
Button in DeleteWorkspaceDialogContent.vue to either remove the colon (use a
static prop like `loading` if the prop is boolean presence-based) or bind a real
expression such as `:loading="isDeleting"` and ensure `isDeleting` (or your
chosen state) is defined/reactive in the component; keep the `@click`="onDelete"
handler unchanged.
In `@src/components/dialog/content/workspace/EditWorkspaceDialogContent.vue`:
- Around line 27-32: The input for editing workspace name in
EditWorkspaceDialogContent.vue lacks a maxlength, so allowlist validation (max
50 chars) can be bypassed; add an explicit maxlength="50" attribute to the text
input bound to newWorkspaceName (the <input ... v-model="newWorkspaceName" ...
`@keydown.enter`="isValidName && onSave()"/>), ensuring the UI prevents typing
past 50 characters to match the existing validation logic.
- Around line 12-18: Replace the raw <button> in EditWorkspaceDialogContent.vue
with the shared Button component used by CreateWorkspaceDialogContent to ensure
consistency; keep the same aria-label and wire the `@click` to onCancel, pass the
equivalent styling/variant props (e.g., ghost/transparent or icon variant) and
include the close icon (pi pi-times) either via the Button's icon prop or slot
so behavior and accessibility match the existing CreateWorkspaceDialogContent
implementation.
In `@src/components/dialog/content/workspace/LeaveWorkspaceDialogContent.vue`:
- Around line 12-18: Replace the raw <button> element (the element with
:aria-label="$t('g.close')", `@click`="onCancel", and the inner <i class="pi
pi-times size-4" />) with the project’s shared icon button component (e.g.,
IconButton/AppIconButton) so styling, focus states and theming remain
consistent; ensure you pass the same aria-label and bind the click to onCancel
and render the same close icon (or appropriate icon prop) so behavior and
accessibility are preserved.
In `@src/components/dialog/GlobalDialog.vue`:
- Around line 7-12: The template in GlobalDialog.vue uses an array-based :class
binding (the existing :class with 'global-dialog' and the conditional on
item.key === 'global-settings' && teamWorkspacesEnabled) which violates the repo
guideline; replace the array with a call to the project cn() helper to compose
classes (pass 'global-dialog' plus the conditional 'settings-dialog-workspace'
when item.key === 'global-settings' && teamWorkspacesEnabled) so Tailwind class
merging follows conventions and avoids :class=[]. Ensure the cn() import/usage
matches other components.
In `@src/components/topbar/CurrentUserButton.vue`:
- Around line 90-94: The computed property workspaceName recreates store refs on
each recompute because storeToRefs(useTeamWorkspaceStore()) is called inside the
computed; hoist the storeToRefs call out of the computed (e.g., const {
workspaceName } = storeToRefs(useTeamWorkspaceStore())) and then reference
workspaceName and showWorkspaceIcon.value inside the computed, leaving the
computed only to return '' or workspaceName.value.
In `@src/components/topbar/CurrentUserPopoverWorkspace.vue`:
- Around line 27-50: The workspace selector div uses a click-only interactive
element; update the element that currently has `@click`="toggleWorkspaceSwitcher"
(the parent wrapper containing WorkspaceProfilePic, workspaceName, and
workspaceTierName) to be keyboard-accessible by converting it to a semantic
interactive element or adding accessibility attributes: either replace the <div>
with a <button> styled the same, or add role="button", tabindex="0", and keydown
handlers that invoke toggleWorkspaceSwitcher on Enter and Space; ensure
focus/hover visual styles remain intact and that ARIA labeling remains correct
for assistive tech.
- Around line 319-322: The logout handler currently awaits handleSignOut()
without catching errors; wrap the call in a try/catch inside handleLogout, await
handleSignOut() in the try block and only call emit('close') on success, and in
the catch block log the error (e.g., console.error or existing logger) and emit
a failure notification (e.g., emit('error', error.message) or emit a dedicated
logout-failed event) so the component doesn't close on failed logout and the
user/developer is informed.
In `@src/components/topbar/WorkspaceSwitcherPopover.vue`:
- Around line 57-64: The delete icon button in WorkspaceSwitcherPopover.vue
lacks an accessible name; update the icon-only control used when
canDeleteWorkspace(workspace) is true to include an aria-label (use the existing
translation $t('g.delete') for the label) and, preferably, replace the raw
<button> with the shared IconButton component so the click still calls
handleDeleteWorkspace(workspace) (preserve `@click.stop` behavior) and
accessibility is handled consistently; ensure the IconButton receives the
translated aria-label and the same classes/props for styling and hover behavior.
- Around line 73-106: The "create workspace" control in
WorkspaceSwitcherPopover.vue is a non-focusable div; replace the outer clickable
div with a native <button> element that uses :disabled="!canCreateWorkspace" and
call handleCreateWorkspace() on its `@click` (or `@click.prevent`) so keyboard users
can focus and activate it; preserve existing class bindings (cn(...)) and inner
structure, move the conditional opacity class to the icon container if needed,
and remove the manual aria-disabled usage since the native disabled attribute
covers it.
In `@src/platform/cloud/subscription/components/SubscriptionPanel.vue`:
- Around line 20-23: The async component SubscriptionPanelContentWorkspace is
being rendered directly when teamWorkspacesEnabled is true; wrap that v-if
branch in a Vue <Suspense> and provide a lightweight fallback slot (e.g., a
spinner or placeholder) so loading is handled predictably; keep the existing
v-else rendering of SubscriptionPanelContentLegacy unchanged but ensure the
Suspense encloses only the SubscriptionPanelContentWorkspace branch (reference
symbols: SubscriptionPanelContentWorkspace, SubscriptionPanelContentLegacy,
teamWorkspacesEnabled, Suspense, fallback).
- Around line 71-88: The value teamWorkspacesEnabled is currently a plain
constant and loses reactivity when flags.teamWorkspacesEnabled updates; change
it to a computed that returns isCloud && flags.teamWorkspacesEnabled so it stays
reactive to flag changes. Locate where flags is obtained via useFeatureFlags()
and replace the top-level const teamWorkspacesEnabled with a Vue computed (using
the computed API) that references flags.teamWorkspacesEnabled and isCloud,
ensuring the template v-if continues to react to remoteConfig updates.
In
`@src/platform/cloud/subscription/components/SubscriptionPanelContentLegacy.vue`:
- Around line 353-357: The component SubscriptionPanelContentLegacy.vue contains
a scoped style block overriding :deep(.bg-comfy-menu-secondary) which must be
removed from the SFC; delete the <style scoped> block from
SubscriptionPanelContentLegacy.vue and move the rule "selector
.bg-comfy-menu-secondary { background-color: transparent }" into a global
stylesheet or your shared Tailwind/theme (e.g., global CSS imported by the app
or tailwind.config.js theme/extensions) so the override applies app-wide and the
component SFC has no style block.
In
`@src/platform/cloud/subscription/components/SubscriptionPanelContentWorkspace.vue`:
- Around line 272-283: isMemberView currently returns true for any non-manager
causing members in subscribed workspaces to see the “not subscribed” zero-state;
update the computed isMemberView (and any dependent logic such as showZeroState)
so it only applies for members when the workspace is unsubscribed by adding a
check against isWorkspaceSubscribed.value (i.e., isMemberView should be true
only when !permissions.value.canManageSubscription &&
!isWorkspaceSubscribed.value), reference the computed names isMemberView,
isOwnerUnsubscribed, showZeroState, permissions, isWorkspaceSubscribed, and
workspaceRole to locate and adjust the logic.
- Around line 431-435: Remove the scoped <style> block from the
SubscriptionPanelContentWorkspace SFC and move the override for
:deep(.bg-comfy-menu-secondary) into a shared global stylesheet or Tailwind
theme file; specifically, delete the style block in
SubscriptionPanelContentWorkspace.vue and add an equivalent rule (e.g.,
.bg-comfy-menu-secondary { background-color: transparent; } or a Tailwind
plugin/extend entry) to your central styles (global CSS or tailwind.config.js /
base globals) so the component no longer contains any <style> blocks but the
visual override is preserved globally.
In `@src/platform/cloud/subscription/composables/useSubscription.test.ts`:
- Around line 65-68: Rename the mock variable to match the new API name: change
mockGetAuthHeader to mockGetFirebaseAuthHeader and update all references in the
test so the vi.mock replacement for useFirebaseAuthStore returns
getFirebaseAuthHeader: mockGetFirebaseAuthHeader; search for usages of
mockGetAuthHeader in useSubscription.test.ts and update them to the new name to
keep naming consistent with the production API.
In `@src/platform/settings/components/SettingDialogContent.vue`:
- Around line 132-134: The current assignment const teamWorkspacesEnabled =
isCloud && flags.teamWorkspacesEnabled evaluates once at setup and won't update
when the reactive flag changes; change it to a computed that returns isCloud &&
flags.teamWorkspacesEnabled (use Vue's computed()) so teamWorkspacesEnabled
stays reactive; update any template usage as needed (templates can use the
computed directly without .value).
In `@src/platform/settings/composables/useSettingUI.ts`:
- Around line 73-83: CORE_CATEGORIES_ORDER and CORE_CATEGORIES are static,
recreate on every composable call; move them out of the useSettingUI function to
top-level module constants so they are created once. Extract
CORE_CATEGORIES_ORDER (array) and CORE_CATEGORIES (Set) as module-level
constants (keeping the same names) and update any references in useSettingUI to
use these top-level symbols; ensure no reactive wrappers are applied when
referenced from the composable.
- Around line 43-44: teamWorkspacesEnabled is currently a plain const and will
not react to async changes in remoteConfig.value; change it to a
reactive/computed value (e.g., a computed that combines isCloud and
flags.teamWorkspacesEnabled) so it updates when flags change, and then update
all usages (e.g., in this file where you check it at lines referenced in the
review) to read teamWorkspacesEnabled.value.
- Around line 244-283: The group labels in workspaceMenuTreeNodes are not being
translated; wrap the group objects for 'workspace', 'general', and 'other'
through the existing translateCategory function (use translateCategory({ key:
'workspace', label: 'Workspace', children: [...] }) etc. or call
translateCategory on each group object) and update the Settings dialog rendering
to use the translatedLabel property (the template currently uses option.label —
switch to option.translatedLabel) so groups use vue-i18n like child nodes; also
add missing translation keys for 'Workspace' and 'Other' in the English locale
entries to match existing keys (the code symbols to change:
workspaceMenuTreeNodes, translateCategory, and the template reference
option.label in SettingDialogContent.vue).
In `@src/platform/workspace/composables/useWorkspaceUI.ts`:
- Around line 91-97: The current computed getters workspaceType and
workspaceRole default to 'personal' and 'owner' when store.activeWorkspace is
null which can inadvertently grant elevated permissions; change these computed
properties (workspaceType and workspaceRole in useWorkspaceUI.ts) to return
undefined (or a distinct 'unknown' value) instead of defaulting to safe-looking
roles, update their inferred types to allow undefined/unknown, and then update
any consumers to explicitly handle the null/undefined case (e.g., guard checks
or fallbacks in UI/permission logic) so no implicit elevated permissions are
assumed during loading or error states.
In `@src/views/GraphView.vue`:
- Around line 256-285: The watcher only observes flags.teamWorkspacesEnabled so
initialization won’t re-run after a later login or after a prior failure; update
the watch to depend on both flags.teamWorkspacesEnabled and
firebaseAuthStore.isAuthenticated (e.g. watch(() =>
[flags.teamWorkspacesEnabled, firebaseAuthStore.isAuthenticated], ...)) and move
the workspaceInitialized/backoff logic into the handler so failures trigger a
bounded retry/backoff (exponential or fixed attempts) before giving up; locate
symbols isCloud, useFeatureFlags, flags.teamWorkspacesEnabled,
firebaseAuthStore.isAuthenticated, workspaceInitialized, useTeamWorkspaceStore,
workspaceStore.initialize and implement retry/backoff inside the same async
handler that imports and calls useTeamWorkspaceStore().
Comment on lines
+16
to
+23
| const { workspaceName } = defineProps<{ | ||
| workspaceName: string | ||
| }>() | ||
|
|
||
| const letter = computed(() => workspaceName?.charAt(0)?.toUpperCase() ?? '?') | ||
|
|
||
| const gradient = computed(() => { | ||
| const seed = letter.value.charCodeAt(0) |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd /root && find . -type f -name "WorkspaceProfilePic.vue" 2>/dev/null | head -5Repository: Comfy-Org/ComfyUI_frontend
Length of output: 125
🏁 Script executed:
cat -n src/components/common/WorkspaceProfilePic.vueRepository: Comfy-Org/ComfyUI_frontend
Length of output: 1492
Guard against empty workspace names to avoid NaN gradients.
An empty string produces letter === '' (the nullish coalescing operator ?? only replaces null/undefined, not empty strings), causing charCodeAt(0) to return NaN, which generates invalid CSS gradients.
🐛 Proposed fix
-const letter = computed(() => workspaceName?.charAt(0)?.toUpperCase() ?? '?')
+const letter = computed(
+ () => workspaceName?.trim()?.charAt(0)?.toUpperCase() || '?'
+)📝 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
| const { workspaceName } = defineProps<{ | |
| workspaceName: string | |
| }>() | |
| const letter = computed(() => workspaceName?.charAt(0)?.toUpperCase() ?? '?') | |
| const gradient = computed(() => { | |
| const seed = letter.value.charCodeAt(0) | |
| const { workspaceName } = defineProps<{ | |
| workspaceName: string | |
| }>() | |
| const letter = computed( | |
| () => workspaceName?.trim()?.charAt(0)?.toUpperCase() || '?' | |
| ) | |
| const gradient = computed(() => { | |
| const seed = letter.value.charCodeAt(0) |
🤖 Prompt for AI Agents
In `@src/components/common/WorkspaceProfilePic.vue` around lines 16 - 23, The
computed `letter` can be an empty string so `letter.value.charCodeAt(0)` returns
NaN and breaks the gradient; update the `letter` computed (or the `gradient`
computed) to guard against empty workspaceName by using a falsy fallback (e.g.,
use `|| '?'` when deriving `letter`) or by checking `letter.value` before
calling `charCodeAt`, and ensure `gradient` uses a fallback numeric seed (e.g.,
charCode of '?') when `charCodeAt` would be NaN; change references to
workspaceName, letter, gradient, and the charCodeAt call accordingly so the
gradient always receives a valid integer seed.
Comment on lines
+4
to
+7
| <WorkspaceProfilePic | ||
| class="size-12 !text-3xl" | ||
| :workspace-name="workspaceName" | ||
| /> |
Contributor
There was a problem hiding this comment.
Avoid using ! prefix for Tailwind classes.
Line 5 uses !text-3xl which violates coding guidelines. Per guidelines, never use !important or the ! prefix for Tailwind classes; instead fix interfering styles.
♻️ Suggested fix
<WorkspaceProfilePic
- class="size-12 !text-3xl"
+ class="size-12 text-3xl"
:workspace-name="workspaceName"
/>If the ! is needed to override internal component styles, consider adding a prop to WorkspaceProfilePic to control text size instead.
📝 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
| <WorkspaceProfilePic | |
| class="size-12 !text-3xl" | |
| :workspace-name="workspaceName" | |
| /> | |
| <WorkspaceProfilePic | |
| class="size-12 text-3xl" | |
| :workspace-name="workspaceName" | |
| /> |
🤖 Prompt for AI Agents
In `@src/components/dialog/content/setting/WorkspacePanelContent.vue` around lines
4 - 7, Remove the Tailwind "!" prefix from the class on the WorkspaceProfilePic
usage — change class="size-12 !text-3xl" to class="size-12 text-3xl" and if that
breaks internal styling of WorkspaceProfilePic, add a size/textSize prop to the
WorkspaceProfilePic component (e.g., textSize or variant) and pass the desired
value from this parent (WorkspaceProfilePic :text-size="'3xl'") so the component
can apply the correct text sizing internally instead of relying on "!important"
overrides.
Comment on lines
+36
to
+40
| :class="[ | ||
| 'flex items-center gap-2 px-3 py-2', | ||
| item.class, | ||
| item.disabled ? 'pointer-events-auto' : '' | ||
| ]" |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Use cn() utility instead of array class binding.
Per coding guidelines, use cn() from @/utils/tailwindUtil to merge Tailwind class names instead of :class="[]".
♻️ Suggested refactor
+import { cn } from '@/utils/tailwindUtil'
+
+// In computed or inline:
+const menuItemClass = (item: MenuItem) => cn(
+ 'flex items-center gap-2 px-3 py-2',
+ item.class,
+ item.disabled && 'pointer-events-auto'
+)Then in template:
<div
v-tooltip="..."
- :class="[
- 'flex items-center gap-2 px-3 py-2',
- item.class,
- item.disabled ? 'pointer-events-auto' : ''
- ]"
+ :class="menuItemClass(item)"🤖 Prompt for AI Agents
In `@src/components/dialog/content/setting/WorkspacePanelContent.vue` around lines
36 - 40, Replace the array-style :class binding in WorkspacePanelContent.vue
with the cn() utility from '@/utils/tailwindUtil': import cn in the component
script and change :class="['flex items-center gap-2 px-3 py-2', item.class,
item.disabled ? 'pointer-events-auto' : '']" to use :class="cn('flex
items-center gap-2 px-3 py-2', item.class, item.disabled ? 'pointer-events-auto'
: '')", preserving the conditional for item.disabled; ensure the cn import is
added to the script/setup or script block where the component is defined.
Comment on lines
+12
to
+18
| <button | ||
| class="cursor-pointer rounded border-none bg-transparent p-0 text-muted-foreground transition-colors hover:text-base-foreground focus-visible:outline-none focus-visible:ring-1 focus-visible:ring-secondary-foreground" | ||
| :aria-label="$t('g.close')" | ||
| @click="onCancel" | ||
| > | ||
| <i class="pi pi-times size-4" /> | ||
| </button> |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Prefer common Button component for the close button.
Per repository conventions, prefer the common button components from src/components/button/ over plain HTML <button> elements for consistency and theming. Based on learnings.
♻️ Suggested refactor
- <button
- class="cursor-pointer rounded border-none bg-transparent p-0 text-muted-foreground transition-colors hover:text-base-foreground focus-visible:outline-none focus-visible:ring-1 focus-visible:ring-secondary-foreground"
- :aria-label="$t('g.close')"
- `@click`="onCancel"
- >
- <i class="pi pi-times size-4" />
- </button>
+ <Button
+ variant="muted-textonly"
+ size="icon"
+ :aria-label="$t('g.close')"
+ `@click`="onCancel"
+ >
+ <i class="pi pi-times size-4" />
+ </Button>🤖 Prompt for AI Agents
In `@src/components/dialog/content/workspace/CreateWorkspaceDialogContent.vue`
around lines 12 - 18, Replace the plain HTML close <button> in
CreateWorkspaceDialogContent.vue with the repository's common Button component
(from components/button), import and register it in the component, and pass the
same props/attributes: :aria-label="$t('g.close')", `@click`="onCancel", matching
styling/variants (use the equivalent variant/size props instead of raw classes
or keep necessary classes), and render the existing icon (<i class="pi pi-times
size-4" />) as the Button's content; ensure the Button is used wherever the
close button is rendered so theming and behavior are consistent.
Comment on lines
+30
to
+38
| <input | ||
| v-model="workspaceName" | ||
| type="text" | ||
| class="w-full rounded-lg border border-border-default bg-transparent px-3 py-2 text-sm text-base-foreground placeholder:text-muted-foreground focus:outline-none focus:ring-1 focus:ring-secondary-foreground" | ||
| :placeholder=" | ||
| $t('workspacePanel.createWorkspaceDialog.namePlaceholder') | ||
| " | ||
| @keydown.enter="isValidName && onCreate()" | ||
| /> |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Add maxlength and autofocus to input for better UX.
Consider adding maxlength="50" to match validation and autofocus for immediate keyboard input.
♻️ Suggested enhancement
<input
v-model="workspaceName"
type="text"
+ maxlength="50"
+ autofocus
class="w-full rounded-lg border border-border-default bg-transparent px-3 py-2 text-sm text-base-foreground placeholder:text-muted-foreground focus:outline-none focus:ring-1 focus:ring-secondary-foreground"
:placeholder="
$t('workspacePanel.createWorkspaceDialog.namePlaceholder')
"
`@keydown.enter`="isValidName && onCreate()"
/>📝 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
| <input | |
| v-model="workspaceName" | |
| type="text" | |
| class="w-full rounded-lg border border-border-default bg-transparent px-3 py-2 text-sm text-base-foreground placeholder:text-muted-foreground focus:outline-none focus:ring-1 focus:ring-secondary-foreground" | |
| :placeholder=" | |
| $t('workspacePanel.createWorkspaceDialog.namePlaceholder') | |
| " | |
| @keydown.enter="isValidName && onCreate()" | |
| /> | |
| <input | |
| v-model="workspaceName" | |
| type="text" | |
| maxlength="50" | |
| autofocus | |
| class="w-full rounded-lg border border-border-default bg-transparent px-3 py-2 text-sm text-base-foreground placeholder:text-muted-foreground focus:outline-none focus:ring-1 focus:ring-secondary-foreground" | |
| :placeholder=" | |
| $t('workspacePanel.createWorkspaceDialog.namePlaceholder') | |
| " | |
| `@keydown.enter`="isValidName && onCreate()" | |
| /> |
🤖 Prompt for AI Agents
In `@src/components/dialog/content/workspace/CreateWorkspaceDialogContent.vue`
around lines 30 - 38, The text input bound to workspaceName in
CreateWorkspaceDialogContent.vue should include maxlength and autofocus for
better UX; update the <input> (the element using v-model="workspaceName" and
`@keydown.enter`="isValidName && onCreate()") to add maxlength="50" to match the
validation rule and add the autofocus attribute so the field is focused when the
dialog opens.
Comment on lines
+73
to
+83
| // Core setting categories (built-in to ComfyUI) in display order | ||
| // 'Other' includes floating settings that don't have a specific category | ||
| const CORE_CATEGORIES_ORDER = [ | ||
| 'Comfy', | ||
| 'LiteGraph', | ||
| 'Appearance', | ||
| '3D', | ||
| 'Mask Editor', | ||
| 'Other' | ||
| ] | ||
| const CORE_CATEGORIES = new Set(CORE_CATEGORIES_ORDER) |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider extracting constants to module level.
CORE_CATEGORIES_ORDER and CORE_CATEGORIES are static values that don't depend on any reactive state. Defining them inside the composable means they're recreated on each invocation.
Suggested refactor
+ // Core setting categories (built-in to ComfyUI) in display order
+ const CORE_CATEGORIES_ORDER = [
+ 'Comfy',
+ 'LiteGraph',
+ 'Appearance',
+ '3D',
+ 'Mask Editor',
+ 'Other'
+ ]
+ const CORE_CATEGORIES = new Set(CORE_CATEGORIES_ORDER)
+
export function useSettingUI(
defaultPanel?: ...
) {
// ...
- // Core setting categories (built-in to ComfyUI) in display order
- const CORE_CATEGORIES_ORDER = [
- 'Comfy',
- ...
- ]
- const CORE_CATEGORIES = new Set(CORE_CATEGORIES_ORDER)📝 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
| // Core setting categories (built-in to ComfyUI) in display order | |
| // 'Other' includes floating settings that don't have a specific category | |
| const CORE_CATEGORIES_ORDER = [ | |
| 'Comfy', | |
| 'LiteGraph', | |
| 'Appearance', | |
| '3D', | |
| 'Mask Editor', | |
| 'Other' | |
| ] | |
| const CORE_CATEGORIES = new Set(CORE_CATEGORIES_ORDER) | |
| // Core setting categories (built-in to ComfyUI) in display order | |
| // 'Other' includes floating settings that don't have a specific category | |
| const CORE_CATEGORIES_ORDER = [ | |
| 'Comfy', | |
| 'LiteGraph', | |
| 'Appearance', | |
| '3D', | |
| 'Mask Editor', | |
| 'Other' | |
| ] | |
| const CORE_CATEGORIES = new Set(CORE_CATEGORIES_ORDER) | |
| export function useSettingUI( | |
| defaultPanel?: ... | |
| ) { | |
| // ... rest of function |
🤖 Prompt for AI Agents
In `@src/platform/settings/composables/useSettingUI.ts` around lines 73 - 83,
CORE_CATEGORIES_ORDER and CORE_CATEGORIES are static, recreate on every
composable call; move them out of the useSettingUI function to top-level module
constants so they are created once. Extract CORE_CATEGORIES_ORDER (array) and
CORE_CATEGORIES (Set) as module-level constants (keeping the same names) and
update any references in useSettingUI to use these top-level symbols; ensure no
reactive wrappers are applied when referenced from the composable.
Comment on lines
+91
to
+97
| const workspaceType = computed<WorkspaceType>( | ||
| () => store.activeWorkspace?.type ?? 'personal' | ||
| ) | ||
|
|
||
| const workspaceRole = computed<WorkspaceRole>( | ||
| () => store.activeWorkspace?.role ?? 'owner' | ||
| ) |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider edge case when activeWorkspace is null.
Defaulting to 'personal' type and 'owner' role when activeWorkspace is null may grant elevated permissions during loading states or error conditions. Consider if this is the safest default.
🤖 Prompt for AI Agents
In `@src/platform/workspace/composables/useWorkspaceUI.ts` around lines 91 - 97,
The current computed getters workspaceType and workspaceRole default to
'personal' and 'owner' when store.activeWorkspace is null which can
inadvertently grant elevated permissions; change these computed properties
(workspaceType and workspaceRole in useWorkspaceUI.ts) to return undefined (or a
distinct 'unknown' value) instead of defaulting to safe-looking roles, update
their inferred types to allow undefined/unknown, and then update any consumers
to explicitly handle the null/undefined case (e.g., guard checks or fallbacks in
UI/permission logic) so no implicit elevated permissions are assumed during
loading or error states.
src/views/GraphView.vue
Outdated
Comment on lines
256
to
285
| // Initialize workspace store when feature flag becomes available | ||
| // Uses watch because remoteConfig loads asynchronously after component mount | ||
| if (isCloud) { | ||
| const { flags } = useFeatureFlags() | ||
| let workspaceInitialized = false | ||
|
|
||
| watch( | ||
| () => flags.teamWorkspacesEnabled, | ||
| async (enabled) => { | ||
| if ( | ||
| enabled && | ||
| !workspaceInitialized && | ||
| firebaseAuthStore.isAuthenticated | ||
| ) { | ||
| workspaceInitialized = true | ||
| try { | ||
| const { useTeamWorkspaceStore } = | ||
| await import('@/platform/workspace/stores/teamWorkspaceStore') | ||
| const workspaceStore = useTeamWorkspaceStore() | ||
| if (workspaceStore.initState === 'uninitialized') { | ||
| await workspaceStore.initialize() | ||
| } | ||
| } catch (error) { | ||
| console.error('Failed to initialize workspace store:', error) | ||
| workspaceInitialized = false // Allow retry on error | ||
| } | ||
| } | ||
| }, | ||
| { immediate: true } | ||
| ) |
Contributor
There was a problem hiding this comment.
Workspace init won’t run after login or after failures.
The watcher only tracks the feature flag. If the flag is already true while the user logs in later, or if initialization fails once, the handler won’t run again (so the “Allow retry on error” comment is ineffective). Consider watching both the flag and auth state and add a bounded retry/backoff if initialization fails.
✅ Suggested fix (watch auth + flag, add retry)
if (isCloud) {
const { flags } = useFeatureFlags()
- let workspaceInitialized = false
+ const workspaceInitialized = ref(false)
+ let workspaceInitInFlight = false
+
+ const tryInitWorkspaceStore = async () => {
+ if (workspaceInitInFlight || workspaceInitialized.value) return
+ if (!flags.teamWorkspacesEnabled || !firebaseAuthStore.isAuthenticated) {
+ return
+ }
+ workspaceInitInFlight = true
+ try {
+ const { useTeamWorkspaceStore } =
+ await import('@/platform/workspace/stores/teamWorkspaceStore')
+ const workspaceStore = useTeamWorkspaceStore()
+ if (workspaceStore.initState === 'uninitialized') {
+ await workspaceStore.initialize()
+ }
+ workspaceInitialized.value = true
+ } catch (error) {
+ console.error('Failed to initialize workspace store:', error)
+ workspaceInitInFlight = false
+ // Retry after a short backoff
+ window.setTimeout(() => {
+ void tryInitWorkspaceStore()
+ }, 5000)
+ } finally {
+ if (workspaceInitialized.value) {
+ workspaceInitInFlight = false
+ }
+ }
+ }
watch(
- () => flags.teamWorkspacesEnabled,
- async (enabled) => {
- if (
- enabled &&
- !workspaceInitialized &&
- firebaseAuthStore.isAuthenticated
- ) {
- workspaceInitialized = true
- try {
- const { useTeamWorkspaceStore } =
- await import('@/platform/workspace/stores/teamWorkspaceStore')
- const workspaceStore = useTeamWorkspaceStore()
- if (workspaceStore.initState === 'uninitialized') {
- await workspaceStore.initialize()
- }
- } catch (error) {
- console.error('Failed to initialize workspace store:', error)
- workspaceInitialized = false // Allow retry on error
- }
- }
- },
+ () => [flags.teamWorkspacesEnabled, firebaseAuthStore.isAuthenticated],
+ () => {
+ void tryInitWorkspaceStore()
+ },
{ immediate: true }
)
}🧰 Tools
🪛 ESLint
[error] 273-273: Unable to resolve path to module '@/platform/workspace/stores/teamWorkspaceStore'.
(import-x/no-unresolved)
🤖 Prompt for AI Agents
In `@src/views/GraphView.vue` around lines 256 - 285, The watcher only observes
flags.teamWorkspacesEnabled so initialization won’t re-run after a later login
or after a prior failure; update the watch to depend on both
flags.teamWorkspacesEnabled and firebaseAuthStore.isAuthenticated (e.g. watch(()
=> [flags.teamWorkspacesEnabled, firebaseAuthStore.isAuthenticated], ...)) and
move the workspaceInitialized/backoff logic into the handler so failures trigger
a bounded retry/backoff (exponential or fixed attempts) before giving up; locate
symbols isCloud, useFeatureFlags, flags.teamWorkspacesEnabled,
firebaseAuthStore.isAuthenticated, workspaceInitialized, useTeamWorkspaceStore,
workspaceStore.initialize and implement retry/backoff inside the same async
handler that imports and calls useTeamWorkspaceStore().
847468a to
5f98533
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/components/topbar/WorkspaceSwitcherPopover.vue`:
- Line 61: In WorkspaceSwitcherPopover.vue remove the commented-out Divider stub
("<!-- <Divider class=\"mx-0 my-0\" /> -->") so the template contains no
commented markup; update the template in the WorkspaceSwitcherPopover component
to delete that line (no replacement needed) to keep the markup clean.
- Around line 32-35: The button in WorkspaceSwitcherPopover.vue currently lacks
an explicit type, which can cause accidental form submissions; update the button
element that calls handleSelectWorkspace(workspace) to include type="button" so
it does not act as a submit button when this component is rendered inside a form
(locate the <button ... `@click`="handleSelectWorkspace(workspace)"> in the
template and add the type attribute).
♻️ Duplicate comments (1)
src/components/topbar/WorkspaceSwitcherPopover.vue (1)
64-97: Use a native button for the create-workspace control.Clickable divs are not keyboard-accessible by default. This was already raised earlier.
Comment on lines
+32
to
+35
| <button | ||
| class="flex flex-1 cursor-pointer items-center gap-2 border-none bg-transparent p-0" | ||
| @click="handleSelectWorkspace(workspace)" | ||
| > |
Contributor
There was a problem hiding this comment.
Add type="button" to avoid accidental form submits.
If this popover is ever rendered inside a <form>, the default submit type can trigger unintended submissions.
🔧 Suggested fix
- <button
+ <button
+ type="button"
class="flex flex-1 cursor-pointer items-center gap-2 border-none bg-transparent p-0"
`@click`="handleSelectWorkspace(workspace)"
>📝 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
| <button | |
| class="flex flex-1 cursor-pointer items-center gap-2 border-none bg-transparent p-0" | |
| @click="handleSelectWorkspace(workspace)" | |
| > | |
| <button | |
| type="button" | |
| class="flex flex-1 cursor-pointer items-center gap-2 border-none bg-transparent p-0" | |
| `@click`="handleSelectWorkspace(workspace)" | |
| > |
🤖 Prompt for AI Agents
In `@src/components/topbar/WorkspaceSwitcherPopover.vue` around lines 32 - 35, The
button in WorkspaceSwitcherPopover.vue currently lacks an explicit type, which
can cause accidental form submissions; update the button element that calls
handleSelectWorkspace(workspace) to include type="button" so it does not act as
a submit button when this component is rendered inside a form (locate the
<button ... `@click`="handleSelectWorkspace(workspace)"> in the template and add
the type attribute).
| </template> | ||
| </template> | ||
|
|
||
| <!-- <Divider class="mx-0 my-0" /> --> |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Remove the commented-out Divider stub.
Commented markup adds noise and can drift from reality; better to delete it until needed. As per coding guidelines, ...
🤖 Prompt for AI Agents
In `@src/components/topbar/WorkspaceSwitcherPopover.vue` at line 61, In
WorkspaceSwitcherPopover.vue remove the commented-out Divider stub ("<!--
<Divider class=\"mx-0 my-0\" /> -->") so the template contains no commented
markup; update the template in the WorkspaceSwitcherPopover component to delete
that line (no replacement needed) to keep the markup clean.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@src/locales/en/main.json`:
- Around line 1285-1289: Remove the duplicate "General" entry in the
settingsCategories object: find the repeated key "General" (the second
occurrence shown in the diff alongside "PLY", "Workspace", "Other") and delete
that duplicate line so the settingsCategories JSON contains only one "General"
key.
In `@src/platform/workspace/stores/teamWorkspaceStore.test.ts`:
- Around line 211-238: Wrap the test that uses vi.useFakeTimers() in a
try/finally so timers are always restored: when testing the retry behavior in
the test that calls useTeamWorkspaceStore().initialize() and advances timers
(which uses vi.useFakeTimers(), vi.advanceTimersByTimeAsync(), and
vi.useRealTimers()), put the test body (including mocking mockWorkspaceApi.list,
starting initialize(), advancing timers and assertions) inside try and call
vi.useRealTimers() in finally to guarantee restoration even if an assertion
throws.
In `@src/platform/workspace/stores/teamWorkspaceStore.ts`:
- Around line 193-205: The code sets activeWorkspaceId from
workspaceAuthStore.currentWorkspace after initializeFromSession without
verifying that ID exists in the fetched list; update the block that calls
workspaceAuthStore.initializeFromSession, workspaceApi.list and processes
response.workspaces (mapped via createWorkspaceState) to check whether
workspaceAuthStore.currentWorkspace.id is present in response.workspaces; if
present keep setting activeWorkspaceId and initState.value='ready', otherwise
clear or fallback (e.g., pick the first workspace from workspaces.value or
unset/clear the session) and then set initState.value and
isFetchingWorkspaces.value appropriately so activeWorkspace/activeWorkspaceId
cannot be left pointing to a stale ID.
♻️ Duplicate comments (2)
src/views/GraphView.vue (1)
256-275: Missing error handling for workspace initialization.If
workspaceStore.initialize()throws, the promise rejection goes unhandled. The watcher correctly observes both auth and feature flag state now, but failures during initialization are not caught.🛠️ Suggested fix: Add try/catch with error logging
watch( () => [flags.teamWorkspacesEnabled, firebaseAuthStore.isAuthenticated], async ([enabled, isAuthenticated]) => { if (!enabled || !isAuthenticated) return - const { useTeamWorkspaceStore } = - await import('@/platform/workspace/stores/teamWorkspaceStore') - const workspaceStore = useTeamWorkspaceStore() - if (workspaceStore.initState === 'uninitialized') { - await workspaceStore.initialize() + try { + const { useTeamWorkspaceStore } = + await import('@/platform/workspace/stores/teamWorkspaceStore') + const workspaceStore = useTeamWorkspaceStore() + if (workspaceStore.initState === 'uninitialized') { + await workspaceStore.initialize() + } + } catch (error) { + console.error('Failed to initialize workspace store:', error) } }, { immediate: true } )src/platform/settings/composables/useSettingUI.ts (1)
75-85: Consider extracting static constants to module level.
CORE_CATEGORIES_ORDERandCORE_CATEGORIESare static values that don't depend on reactive state. Defining them inside the composable means they're recreated on each invocation. While the performance impact is minimal, extracting them to module level would be cleaner.♻️ Suggested refactor
+ // Core setting categories (built-in to ComfyUI) in display order + const CORE_CATEGORIES_ORDER = [ + 'Comfy', + 'LiteGraph', + 'Appearance', + '3D', + 'Mask Editor', + 'Other' + ] + const CORE_CATEGORIES = new Set(CORE_CATEGORIES_ORDER) + export function useSettingUI( defaultPanel?: ... ) { - // Core setting categories (built-in to ComfyUI) in display order - // 'Other' includes floating settings that don't have a specific category - const CORE_CATEGORIES_ORDER = [ - 'Comfy', - 'LiteGraph', - 'Appearance', - '3D', - 'Mask Editor', - 'Other' - ] - const CORE_CATEGORIES = new Set(CORE_CATEGORIES_ORDER)
Comment on lines
+1285
to
1289
| "PLY": "PLY", | ||
| "Workspace": "Workspace", | ||
| "General": "General", | ||
| "Other": "Other" | ||
| }, |
Contributor
There was a problem hiding this comment.
Duplicate JSON key: "General" already exists at line 1229.
The key "General": "General" at line 1287 duplicates the existing key at line 1229 within settingsCategories. In JSON, duplicate keys cause the later value to silently override the earlier one, which can lead to confusion and maintenance issues.
Since both values are identical ("General"), this is functionally harmless but should be removed to keep the locale file clean.
🐛 Proposed fix
"Execution": "Execution",
- "PLY": "PLY",
- "Workspace": "Workspace",
- "General": "General",
- "Other": "Other"
+ "PLY": "PLY",
+ "Workspace": "Workspace",
+ "Other": "Other"📝 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
| "PLY": "PLY", | |
| "Workspace": "Workspace", | |
| "General": "General", | |
| "Other": "Other" | |
| }, | |
| "PLY": "PLY", | |
| "Workspace": "Workspace", | |
| "Other": "Other" | |
| }, |
🤖 Prompt for AI Agents
In `@src/locales/en/main.json` around lines 1285 - 1289, Remove the duplicate
"General" entry in the settingsCategories object: find the repeated key
"General" (the second occurrence shown in the diff alongside "PLY", "Workspace",
"Other") and delete that duplicate line so the settingsCategories JSON contains
only one "General" key.
Comment on lines
+211
to
238
| it('sets error state when workspaces fetch fails after retries', async () => { | ||
| vi.useFakeTimers() | ||
| mockWorkspaceApi.list.mockRejectedValue(new Error('Network error')) | ||
|
|
||
| const store = useTeamWorkspaceStore() | ||
|
|
||
| await expect(store.initialize()).rejects.toThrow('Network error') | ||
| // Start initialization and catch rejections to prevent unhandled promise warning | ||
| let initError: unknown = null | ||
| const initPromise = store.initialize().catch((e: unknown) => { | ||
| initError = e | ||
| }) | ||
|
|
||
| // Fast-forward through all retry delays (1s, 2s, 4s) | ||
| await vi.advanceTimersByTimeAsync(1000) | ||
| await vi.advanceTimersByTimeAsync(2000) | ||
| await vi.advanceTimersByTimeAsync(4000) | ||
|
|
||
| await initPromise | ||
|
|
||
| expect(initError).toBeInstanceOf(Error) | ||
| expect((initError as Error).message).toBe('Network error') | ||
| expect(store.initState).toBe('error') | ||
| expect(store.error).toBeInstanceOf(Error) | ||
| // Should have been called 4 times (initial + 3 retries) | ||
| expect(mockWorkspaceApi.list).toHaveBeenCalledTimes(4) | ||
|
|
||
| vi.useRealTimers() | ||
| }) |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find the test file and read it to check for afterEach hooks and other patterns
fd 'teamWorkspaceStore.test.ts' --type f -x cat -nRepository: Comfy-Org/ComfyUI_frontend
Length of output: 34914
🏁 Script executed:
# Search for other uses of vi.useFakeTimers in the test files to see codebase patterns
rg 'vi\.useFakeTimers' --type ts -A 5 -B 2Repository: Comfy-Org/ComfyUI_frontend
Length of output: 17879
🏁 Script executed:
# Check if there's a general test setup file or vitest config
fd 'vitest.config|setup' --type f | head -20Repository: Comfy-Org/ComfyUI_frontend
Length of output: 148
Ensure fake timers are always restored to prevent test isolation issues.
If an assertion throws before vi.useRealTimers() executes, fake timers leak into subsequent tests. Wrap the test body in a try/finally block to guarantee timer restoration.
♻️ Suggested change
it('sets error state when workspaces fetch fails after retries', async () => {
vi.useFakeTimers()
- mockWorkspaceApi.list.mockRejectedValue(new Error('Network error'))
-
- const store = useTeamWorkspaceStore()
-
- // Start initialization and catch rejections to prevent unhandled promise warning
- let initError: unknown = null
- const initPromise = store.initialize().catch((e: unknown) => {
- initError = e
- })
-
- // Fast-forward through all retry delays (1s, 2s, 4s)
- await vi.advanceTimersByTimeAsync(1000)
- await vi.advanceTimersByTimeAsync(2000)
- await vi.advanceTimersByTimeAsync(4000)
-
- await initPromise
-
- expect(initError).toBeInstanceOf(Error)
- expect((initError as Error).message).toBe('Network error')
- expect(store.initState).toBe('error')
- expect(store.error).toBeInstanceOf(Error)
- // Should have been called 4 times (initial + 3 retries)
- expect(mockWorkspaceApi.list).toHaveBeenCalledTimes(4)
-
- vi.useRealTimers()
+ try {
+ mockWorkspaceApi.list.mockRejectedValue(new Error('Network error'))
+
+ const store = useTeamWorkspaceStore()
+
+ // Start initialization and catch rejections to prevent unhandled promise warning
+ let initError: unknown = null
+ const initPromise = store.initialize().catch((e: unknown) => {
+ initError = e
+ })
+
+ // Fast-forward through all retry delays (1s, 2s, 4s)
+ await vi.advanceTimersByTimeAsync(1000)
+ await vi.advanceTimersByTimeAsync(2000)
+ await vi.advanceTimersByTimeAsync(4000)
+
+ await initPromise
+
+ expect(initError).toBeInstanceOf(Error)
+ expect((initError as Error).message).toBe('Network error')
+ expect(store.initState).toBe('error')
+ expect(store.error).toBeInstanceOf(Error)
+ // Should have been called 4 times (initial + 3 retries)
+ expect(mockWorkspaceApi.list).toHaveBeenCalledTimes(4)
+ } finally {
+ vi.useRealTimers()
+ }📝 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
| it('sets error state when workspaces fetch fails after retries', async () => { | |
| vi.useFakeTimers() | |
| mockWorkspaceApi.list.mockRejectedValue(new Error('Network error')) | |
| const store = useTeamWorkspaceStore() | |
| await expect(store.initialize()).rejects.toThrow('Network error') | |
| // Start initialization and catch rejections to prevent unhandled promise warning | |
| let initError: unknown = null | |
| const initPromise = store.initialize().catch((e: unknown) => { | |
| initError = e | |
| }) | |
| // Fast-forward through all retry delays (1s, 2s, 4s) | |
| await vi.advanceTimersByTimeAsync(1000) | |
| await vi.advanceTimersByTimeAsync(2000) | |
| await vi.advanceTimersByTimeAsync(4000) | |
| await initPromise | |
| expect(initError).toBeInstanceOf(Error) | |
| expect((initError as Error).message).toBe('Network error') | |
| expect(store.initState).toBe('error') | |
| expect(store.error).toBeInstanceOf(Error) | |
| // Should have been called 4 times (initial + 3 retries) | |
| expect(mockWorkspaceApi.list).toHaveBeenCalledTimes(4) | |
| vi.useRealTimers() | |
| }) | |
| it('sets error state when workspaces fetch fails after retries', async () => { | |
| vi.useFakeTimers() | |
| try { | |
| mockWorkspaceApi.list.mockRejectedValue(new Error('Network error')) | |
| const store = useTeamWorkspaceStore() | |
| // Start initialization and catch rejections to prevent unhandled promise warning | |
| let initError: unknown = null | |
| const initPromise = store.initialize().catch((e: unknown) => { | |
| initError = e | |
| }) | |
| // Fast-forward through all retry delays (1s, 2s, 4s) | |
| await vi.advanceTimersByTimeAsync(1000) | |
| await vi.advanceTimersByTimeAsync(2000) | |
| await vi.advanceTimersByTimeAsync(4000) | |
| await initPromise | |
| expect(initError).toBeInstanceOf(Error) | |
| expect((initError as Error).message).toBe('Network error') | |
| expect(store.initState).toBe('error') | |
| expect(store.error).toBeInstanceOf(Error) | |
| // Should have been called 4 times (initial + 3 retries) | |
| expect(mockWorkspaceApi.list).toHaveBeenCalledTimes(4) | |
| } finally { | |
| vi.useRealTimers() | |
| } | |
| }) |
🤖 Prompt for AI Agents
In `@src/platform/workspace/stores/teamWorkspaceStore.test.ts` around lines 211 -
238, Wrap the test that uses vi.useFakeTimers() in a try/finally so timers are
always restored: when testing the retry behavior in the test that calls
useTeamWorkspaceStore().initialize() and advances timers (which uses
vi.useFakeTimers(), vi.advanceTimersByTimeAsync(), and vi.useRealTimers()), put
the test body (including mocking mockWorkspaceApi.list, starting initialize(),
advancing timers and assertions) inside try and call vi.useRealTimers() in
finally to guarantee restoration even if an assertion throws.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/platform/workspace/stores/teamWorkspaceStore.ts`:
- Around line 289-295: In the retry logging inside teamWorkspaceStore (where
BASE_RETRY_DELAY_MS and MAX_INIT_RETRIES are used), stop passing the raw error
object e to console.warn and instead log only the extracted error message (e.g.,
use e.message or a safe fallback like String(e) when e is not an Error); update
the warning to include the descriptive context and the extracted message only,
leaving out stacks or full error objects to avoid exposing sensitive
request/config data.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/platform/workspace/stores/teamWorkspaceStore.ts`:
- Around line 289-300: The assignment isFetchingWorkspaces.value = false after
the retry loop is unreachable because every path inside the loop returns or
throws; remove this redundant line from teamWorkspaceStore to avoid dead code.
Locate the retry loop and the isFetchingWorkspaces.value usage (referenced as
isFetchingWorkspaces.value and the retry logic around MAX_INIT_RETRIES) and
simply delete the trailing assignment; if you intended a guaranteed cleanup
instead, wrap the loop in a try/finally and set isFetchingWorkspaces.value =
false in the finally block so it always runs.
Comment on lines
+289
to
300
| // Retry with exponential backoff for transient errors | ||
| const delay = BASE_RETRY_DELAY_MS * Math.pow(2, attempt) | ||
| const errorMessage = e instanceof Error ? e.message : String(e) | ||
| console.warn( | ||
| `[teamWorkspaceStore] Init failed (attempt ${attempt + 1}/${MAX_INIT_RETRIES + 1}), retrying in ${delay}ms: ${errorMessage}` | ||
| ) | ||
| await new Promise((resolve) => setTimeout(resolve, delay)) | ||
| } | ||
| } | ||
|
|
||
| isFetchingWorkspaces.value = false | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Unreachable cleanup code after retry loop.
Lines 298-299 are unreachable. The retry loop always exits via:
returnon success (lines 215, 236, 276)throwon permanent error or exhausted retries (line 286)
The isFetchingWorkspaces.value = false is already set in all exit paths within the loop.
♻️ Remove unreachable code
await new Promise((resolve) => setTimeout(resolve, delay))
}
}
-
- isFetchingWorkspaces.value = false
}📝 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
| // Retry with exponential backoff for transient errors | |
| const delay = BASE_RETRY_DELAY_MS * Math.pow(2, attempt) | |
| const errorMessage = e instanceof Error ? e.message : String(e) | |
| console.warn( | |
| `[teamWorkspaceStore] Init failed (attempt ${attempt + 1}/${MAX_INIT_RETRIES + 1}), retrying in ${delay}ms: ${errorMessage}` | |
| ) | |
| await new Promise((resolve) => setTimeout(resolve, delay)) | |
| } | |
| } | |
| isFetchingWorkspaces.value = false | |
| } | |
| // Retry with exponential backoff for transient errors | |
| const delay = BASE_RETRY_DELAY_MS * Math.pow(2, attempt) | |
| const errorMessage = e instanceof Error ? e.message : String(e) | |
| console.warn( | |
| `[teamWorkspaceStore] Init failed (attempt ${attempt + 1}/${MAX_INIT_RETRIES + 1}), retrying in ${delay}ms: ${errorMessage}` | |
| ) | |
| await new Promise((resolve) => setTimeout(resolve, delay)) | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
In `@src/platform/workspace/stores/teamWorkspaceStore.ts` around lines 289 - 300,
The assignment isFetchingWorkspaces.value = false after the retry loop is
unreachable because every path inside the loop returns or throws; remove this
redundant line from teamWorkspaceStore to avoid dead code. Locate the retry loop
and the isFetchingWorkspaces.value usage (referenced as
isFetchingWorkspaces.value and the retry logic around MAX_INIT_RETRIES) and
simply delete the trailing assignment; if you intended a guaranteed cleanup
instead, wrap the loop in a try/finally and set isFetchingWorkspaces.value =
false in the finally block so it always runs.
|
simula-r
added a commit
that referenced
this pull request
Jan 22, 2026
Add workspace creation and management (create, edit, delete, leave, switch workspaces). Follow-up PR will add invite and membership flow. ## Changes - Workspace CRUD dialogs - Workspace switcher popover in topbar - Workspace settings panel - Subscription panel for workspace context ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-8221-Workspaces-3-create-a-workspace-2ef6d73d36508155975ffa6e315971ec) by [Unito](https://www.unito.io) --------- Co-authored-by: Claude Opus 4.5 <noreply@anthropic.com>
simula-r
added a commit
that referenced
this pull request
Jan 23, 2026
## Summary - Backport of #8221 to cloud/1.37 - Cherry-picked commit a08ccb5 with conflict resolution ## Conflicts resolved - `src/components/dialog/GlobalDialog.vue`: Added workspace mode CSS styling from PR - `src/platform/cloud/subscription/components/SubscriptionPanel.vue`: Accepted PR refactoring to use conditional workspace components 🤖 Generated with [Claude Code](https://claude.ai/code) ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-8252-backport-cloud-1-37-Workspaces-3-create-a-workspace-8221-2f06d73d365081e1a38ed4492a7bc6a8) by [Unito](https://www.unito.io) --------- Co-authored-by: Claude Opus 4.5 <noreply@anthropic.com> Co-authored-by: GitHub Action <action@github.com>
This was referenced Feb 3, 2026
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
2 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
Add workspace creation and management (create, edit, delete, leave, switch workspaces).
Follow-up PR will add invite and membership flow.
Changes
┆Issue is synchronized with this Notion page by Unito