Refactor/926 state driven instance navigation#973
Open
MaanilVerma wants to merge 25 commits into
Open
Conversation
Lift the implicit "which window does this click target" decision out of
three scattered sites into one pure, table-driven function. Behavior-
identical (current matrix encoded); deltas land in Phase 3.
- shared/viewKind: ViewKind/Category/NavClass + navClass + viewKindFor,
the single classifier (remote folds into cloud for navigation)
- shared/navigation/navDecision: total O(1) transition table →
{window, verb, confirm, primaryLabel, secondary}; runs in main + renderer
- main: computeViewKind + currentView/currentCategory on the picker
snapshot; new openInstallInNewWindow primitive (focus-existing else
spawn a fresh chooser host) + IPC + preload bridge; deliverPickToEntry
extracted and shared with the swap path
- renderer: useInstanceNavState (facts) + useInstanceActions (dispatcher);
footer CTA + caret split-button driven by the decision; picker routes
the emitted NavDecision through the dispatcher
typecheck (node+web) + lint clean; 1971 tests pass.
…e 3a) - pickInstallFromPicker: Switch / Open in new window / Cancel via openSystemModalChoiceAsync; 'secondary' → openInstallInNewWindow (keeps A running) - decision table: instance|instance|stopped gains the new-window caret No flag. typecheck + lint clean; 1971 tests pass.
- 3 e2e specs (dashboard/instance/cloud): bridge → IPC → window for the navigation deltas (8 tests, lifecycle project) - record open-install-new-window IPC for assertions - handoff doc: test-coverage table (unit vs e2e) + why some cells are unit/manual-only
- openInstallInNewWindow fails closed (try/catch) + telemetry after the spawn guard so a failed spawn can't record a false success - pick/restart picker IPC wirings now .catch() unhandled rejections - dispatch aborts (no unhandled rejection) when a confirm dialog rejects - remove dead 'switch-3way' Confirm variant; strip rollout/history from source comments; relocate the swap-contract JSDoc - tests: dispatch reject + bridge-undefined, caret new-window coverage, unknown-category, e2e parent-undisturbed + reset symmetry typecheck + lint clean; 1976 unit tests pass.
…d? column The handoff doc's matrix had been condensed (dropped the deferred Stop/Restart secondaries, folded row-13 into 'source of truth'). Reproduce the original 17-row matrix verbatim with an Implemented? column; deviations (row 13 new-window) and deferrals (rows 8/15) called out in §1a.
Clicking Switch showed a wrong 'Restart?' dialog then closed the picker before main's 'Switch?' modal. Now the picker shows ONE in-drawer 3-way (Switch / Open in new window / Cancel) and stays open until the user commits, matching Restart. - useInstanceActions: switch routes through confirmSwitch; pickInstall(confirmed:true) so main skips its modal - pickInstallFromPicker + pick-install IPC + preload bridge take a confirmed flag - remove the now-dead Confirm type/field from the decision
…op in caret - leading icons on the primary CTA, caret items, and the More menu (Switch→Replace, Stop→CircleStop, Forget→EyeOff vs Uninstall→Trash2) - caret dropdown gains a 'Window options' heading + divider, fits content width; MoreMenu supports an optional heading + per-item icon (More menu unchanged otherwise) - reuse the existing synthetic Stop action in the caret next to Restart (own running local instance)
…ey-files + coverage
…rop 'Start (new window)') It spawns a window, not a swap — so the open-in-new-window label + icon are clearer and consistent with the caret. Removes the now-unused startNewWindow key.
…f → Restart - targetKind: remote folds to the cloud target rows (fixes Remote→Remote / Cloud→Remote dead 'Start' and Instance→Remote wrong 3-way) - add cloud|cloud|stopped (open-new) + running-elsewhere (focus) cells; reachability test bans no-op CTAs - cloud/remote self → Restart; allowDuplicate kept dormant/reserved - per-category CTA wording + icon: 'Open Remote' (server) vs 'Open Cloud'
- Three-way modal helper resolving the raw confirm/secondary/cancel action was lost in the main merge while its index.ts caller survived, breaking typecheck. - Restores the definition verbatim from 40837aa.
MaanilVerma
requested review from
Kosinkadink and
deepme987
and removed request for
deepme987
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughImplements a state-driven navigation decision table ( ChangesInstance/Window Navigation Matrix
Sequence DiagramsequenceDiagram
participant User
participant Footer as ComfyUISettingsContent<br/>(Footer CTA)
participant Picker as InstancePickerView
participant Actions as useInstanceActions
participant Bridge as Preload Bridge
participant Main as Main Process
User->>Footer: click primary CTA or caret
Footer->>Footer: useInstanceNavState + decideNavigation(navInput)
Footer->>Footer: compute primaryDecision (NavDecision)
alt user clicks primary
Footer->>Picker: emit primary-action [NavDecision]
else user clicks caret menu
Footer->>Picker: emit open-new-window [NavDecision]
end
Picker->>Actions: dispatch(decision, target)
Actions->>Actions: confirmCloudCapacity (gate cloud)
alt decision.verb === switch
Actions->>Actions: confirmSwitch → switch|new-window|cancel
else decision.verb === restart
Actions->>Actions: confirmLocalKill (local only)
end
alt switch choice
Actions->>Bridge: pickInstall(id, {confirmed: true})
else new-window choice
Actions->>Bridge: openInstallNewWindow(id, {allowDuplicate?})
else cancel choice
Actions->>Actions: abort
end
Bridge->>Main: IPC comfy-titlepopup:pick-install or open-install-new-window
Main->>Main: 3-way swap modal if needed, then deliverPickToEntry
Possibly related PRs
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
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: 10
🤖 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 `@e2e/nav-matrix-cloud.test.ts`:
- Around line 77-80: The afterAll blocks across three test files dereference
ctx.app without checking if ctx was successfully initialized, causing crashes
when application launch fails. In e2e/nav-matrix-cloud.test.ts (lines 77-80),
wrap both the clearRunningSessions call and ctx?.cleanup() execution in a guard
that checks if ctx is defined before attempting to access ctx.app. Apply the
same ctx guard pattern to e2e/nav-matrix-dashboard.test.ts (lines 85-90) before
any references to ctx.app, and to e2e/nav-matrix-instance.test.ts (lines 81-86)
before any references to ctx.app. This ensures teardown operations only execute
when ctx was properly initialized during test setup.
In `@e2e/nav-matrix-dashboard.test.ts`:
- Around line 45-64: The functions liveWindowCount and openPicker are duplicated
across nav-matrix test specs and should be centralized. Create or update a
shared helper file in the e2e/support directory to export these two functions,
then remove them from the individual test files and import them from the
centralized helper module instead. This prevents code duplication and ensures
consistency across all nav-matrix test suites.
- Around line 117-120: The negative assertions checking for no IPC invocations
of focus-comfy-window are one-shot checks that can pass before delayed IPCs
arrive, causing test flakiness. Replace the one-shot expect calls on
getIpcInvocations with a polling mechanism that waits for a short settling
window to ensure the IPC invocations genuinely do not arrive. Apply this fix at
the anchor site in e2e/nav-matrix-dashboard.test.ts lines 117-120 where the
focus-comfy-window assertion occurs, and also at the sibling site in
e2e/nav-matrix-dashboard.test.ts lines 135-139 where similar negative assertions
are present. Use a polling utility with a generous timeout threshold to
stabilize these flaky negative assertions.
In `@e2e/nav-matrix-instance.test.ts`:
- Around line 117-131: The test is missing an explicit assertion to verify that
the running-elsewhere path does not emit a run-action: launch IPC call. Add an
additional expect statement after the existing focus-comfy-window check to
confirm that no run-action: launch IPC invocations were made during the test.
Use getIpcInvocations similar to the existing pattern to check for any launch
calls and verify the array is empty or contains no matching entries.
In `@src/main/popups/systemModal.ts`:
- Around line 177-195: The openSystemModalChoiceAsync and openSystemModalAsync
functions contain duplicated Promise wrapper logic around the openSystemModal
call. Extract the common Promise wrapper pattern into a shared helper function
that accepts the necessary configuration and callback handler, then refactor
both openSystemModalChoiceAsync and openSystemModalAsync to use this helper to
eliminate code duplication and prevent future divergence.
In `@src/main/popups/titlePopup.ts`:
- Around line 2604-2631: The IPC event handlers for
'comfy-titlepopup:pick-install' and 'comfy-titlepopup:open-install-new-window'
should validate that the popup entry is specifically an instance-picker before
processing the request, to prevent other popup surfaces from calling picker-only
actions. After retrieving the entry from titlePopupsByWebContents and checking
it exists, add an early return if entry.kind is not equal to 'instance-picker'.
Apply this same validation check in all affected IPC handler locations where
picker-specific actions are being performed.
- Around line 2628-2630: The call to bindings.openInstallInNewWindow in the
hideTitlePopup handler lacks error handling, which can cause unhandled
exceptions to propagate through the IPC listener. Wrap the
openInstallInNewWindow call with Promise.resolve(...).catch(...) error boundary
pattern, similar to the error handling applied to the pick and restart handlers
in the same function, to gracefully catch and handle any errors that might be
thrown.
In `@src/renderer/src/comfyTitlePopup/InstancePickerView.vue`:
- Around line 452-496: The confirmLocalKill and confirmSwitch functions contain
hardcoded dialog strings (titles, messages, labels, and messageDetails items)
that bypass the i18n localization system. Replace all hardcoded strings in both
functions with i18n calls using the appropriate i18n function (either $t() or
t() depending on what is imported and used elsewhere in this component). This
includes the dialog title, message, confirmLabel, cancelLabel, secondaryLabel,
the 'Heads up' label, and all items in the messageDetails arrays. Ensure each
translatable string is wrapped in the i18n function call while maintaining the
current dialog structure and logic.
In `@src/shared/navigation/navDecision.ts`:
- Around line 56-65: The decideNavigation function returns shared objects from
TABLE that consumers can mutate, particularly the secondary property array,
which can poison subsequent decisions across calls. Freeze all returned decision
objects using Object.freeze at the point of return in the decideNavigation
function implementation (found in the lines 94-117 and 124-281 range) to ensure
they are immutable. Apply Object.freeze recursively to the secondary array and
its contents to prevent any mutation of the decision tree structure that is
being reused across multiple function calls.
In `@src/shared/viewKind.ts`:
- Around line 1-14: The docblocks in src/shared/viewKind.ts are too verbose and
sprawling, making them harder for maintainers to quickly understand the key
behaviors. Condense the docblock at lines 1-14 to a brief, one-sentence or short
behavior-only comment that clearly describes what ViewKind, Category, and
NavClass are without unnecessary narrative details. Apply the same condensing
principle to the docblock at lines 26-33 to keep it strictly descriptive and
concise. Follow the guideline: "Write concise comments — one sentence is better
than five paragraphs to justify a small change."
🪄 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: d64a898e-20f8-42e7-b684-106e440ef1bf
📒 Files selected for processing (22)
docs/instance-navigation-926-handoff.mde2e/nav-matrix-cloud.test.tse2e/nav-matrix-dashboard.test.tse2e/nav-matrix-instance.test.tssrc/main/host/registry.test.tssrc/main/host/registry.tssrc/main/index.tssrc/main/popups/systemModal.tssrc/main/popups/titlePopup.tssrc/preload/comfyTitlePopupPreload.tssrc/renderer/src/comfyTitlePopup/InstancePickerView.test.tssrc/renderer/src/comfyTitlePopup/InstancePickerView.vuesrc/renderer/src/components/settings/ComfyUISettingsContent.test.tssrc/renderer/src/components/settings/ComfyUISettingsContent.vuesrc/renderer/src/composables/useInstanceActions.test.tssrc/renderer/src/composables/useInstanceActions.tssrc/renderer/src/composables/useInstanceNavState.test.tssrc/renderer/src/composables/useInstanceNavState.tssrc/renderer/src/views/comfyUISettings/MoreMenu.vuesrc/shared/navigation/navDecision.test.tssrc/shared/navigation/navDecision.tssrc/shared/viewKind.ts
- e2e nav-matrix: guard afterAll teardown when launch fails; extract shared liveWindowCount/openPicker into e2e/support/navMatrixHelpers.ts; poll the negative IPC assertions (no-focus / no-launch) so a late IPC can't slip past a one-shot check; add the missing no-launch assertion to the instance running-elsewhere test - systemModal: extract the shared open-and-resolve Promise wrapper - titlePopup: gate the pick/open-new-window/restart picker IPC handlers on entry.kind === instance-picker (reuses POPUP_KIND) - InstancePickerView: i18n the restart/switch confirm dialogs (new en + zh keys) - navDecision: freeze the shared decision singletons + mark NavDecision readonly so a consumer can't poison the table - viewKind: tighten the docblocks to behavior-only Skipped: openInstallInNewWindow error-boundary (fn is sync with internal try/catch — wrapping would mislead).
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
src/main/index.ts (3)
1557-1570:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winLocalize the system-modal copy.
These confirmation modals hard-code English strings instead of using
i18n.t, so non-English locales will still see English restart/switch prompts.Also applies to: 1986-1999
🤖 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/main/index.ts` around lines 1557 - 1570, Replace all hard-coded English strings in the system-modal dialog (title, message, details items, confirmLabel, secondaryLabel, and cancelLabel) with calls to i18n.t for localization. For strings containing dynamic content like targetName, use i18n.t with interpolation parameters. This issue appears in multiple locations: at lines 1557-1570 (the Switch instance modal) and lines 1986-1999 (another similar modal), so both locations must be updated with the appropriate i18n.t calls to ensure non-English locales display translated prompts instead of English text.
1549-1553: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winRemove the matrix-row reference from the code comment.
Describe the three-way choice behavior directly without tying source comments to a planning-table row.
As per coding guidelines, “Never reference plan steps, phases, tracks, or stage IDs in comments.”
🤖 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/main/index.ts` around lines 1549 - 1553, Remove the "matrix row 9" reference from the comment block starting with "Three-way choice" to comply with coding guidelines that prohibit referencing plan steps, phases, tracks, or stage IDs in comments. Rewrite the comment to describe the three-way choice behavior directly by explaining that when the current local instance is running, it offers to keep it alive in a separate window instead of only stopping and switching, without any reference to planning-table rows or stages. You can keep the issue reference (`#926`) if it provides functional context, but remove all matrix-row and planning-stage terminology.Source: Coding guidelines
1455-1463:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftThread
allowDuplicatethrough the actual launch path.
allowDuplicateonly skips the immediate focus, but the deferredpicker-pick-installpayload drops that flag andonLaunch()still reuses any live entry for the same install, so the “new window” flag goes poof before a duplicate can attach.Also applies to: 1474-1500
🤖 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/main/index.ts` around lines 1455 - 1463, The `allowDuplicate` flag is not being threaded through the deferred launch payload in the `deliverPickToEntry` function and other similar locations. Add the `allowDuplicate` flag as a parameter to `deliverPickToEntry` and include it in the `picker-pick-install` payload object alongside `kind` and `installationId`, so that when the payload is processed by `onLaunch()`, it has access to the flag and can properly prevent reusing existing entries when a duplicate launch should not occur. Apply this same fix to all other locations mentioned in the comment (lines 1474-1500) where similar deferred launch payloads are being sent.src/renderer/src/comfyTitlePopup/InstancePickerView.vue (1)
477-479:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDerive missing host context before routing decisions.
currentView/currentCategoryare optional, but falling back to dashboard/null makes an active local host skip the switch confirmation and feed dashboard-table decisions intoComfyUISettingsContent.Suggested fix
-import type { Category, ViewKind } from '../../../shared/viewKind' +import { normaliseCategory, viewKindFor, type Category, type ViewKind } from '../../../shared/viewKind' @@ const installations = computed<Installation[]>( () => props.snapshot.installs as unknown as Installation[] ) +const activeHostInstall = computed(() => + props.snapshot.activeInstallationId + ? installations.value.find((inst) => inst.id === props.snapshot.activeInstallationId) ?? null + : null +) +const resolvedCurrentCategory = computed<Category | null>( + () => props.snapshot.currentCategory ?? normaliseCategory(activeHostInstall.value?.sourceCategory) +) +const resolvedCurrentView = computed<ViewKind>( + () => props.snapshot.currentView ?? viewKindFor(props.snapshot.activeInstallationId, resolvedCurrentCategory.value) +) @@ const hostIsLocalRunning = - props.snapshot.currentView === 'instance' && props.snapshot.currentCategory === 'local' + resolvedCurrentView.value === 'instance' && resolvedCurrentCategory.value === 'local' @@ - :current-view="snapshot.currentView ?? 'dashboard'" - :current-category="snapshot.currentCategory ?? null" + :current-view="resolvedCurrentView" + :current-category="resolvedCurrentCategory"Also applies to: 616-618
🤖 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/comfyTitlePopup/InstancePickerView.vue` around lines 477 - 479, The routing decision logic in the `hostIsLocalRunning` variable (at line 477-479 and also at line 616-618) does not handle the case where `currentView` and `currentCategory` from the snapshot are optional and may be undefined. Before checking if `hostIsLocalRunning` based on whether `currentView === 'instance'` and `currentCategory === 'local'`, first establish default values for these optional properties (defaulting to 'dashboard' for currentView and null for currentCategory). Apply this same fix at both affected locations to ensure consistent host context derivation before any routing decisions are made that feed into downstream logic like `ComfyUISettingsContent`.src/renderer/src/components/settings/ComfyUISettingsContent.vue (1)
826-829:⚠️ Potential issue | 🟠 Major | ⚡ Quick winClose the caret menu when operations lock the footer.
opInflightcloses the regular More menu but leaves the split-button caret menu open, so a pre-opened “Window options” menu can still expose actions after the footer is disabled.Suggested fix
watch(opInflight, (yes) => { - if (yes && moreMenuOpen.value) closeMoreMenu() + if (!yes) return + if (moreMenuOpen.value) closeMoreMenu() + if (caretMenuOpen.value) closeCaretMenu() })🤖 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/components/settings/ComfyUISettingsContent.vue` around lines 826 - 829, The watcher on opInflight currently only closes the regular More menu by calling closeMoreMenu(), but the split-button caret menu remains open, allowing actions to be exposed after the footer is disabled. In the opInflight watcher that checks if yes is true and moreMenuOpen.value is true, add an additional call to close the caret menu (in addition to the existing closeMoreMenu() call) to ensure both menu types are closed when an operation begins and locks the footer.src/shared/navigation/navDecision.ts (1)
1-17: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winTrim these comments to behavior-only notes.
Several comments still explain provenance/history instead of just the cell behavior; please shorten them so the table stays a map, not a memoir. As per coding guidelines, “Write concise comments — one sentence is better than five paragraphs to justify a small change”, “Never reference plan steps, phases, tracks, or stage IDs in comments”, and “Don't narrate history in comments”.
Also applies to: 121-124, 162-165, 213-220, 238-243
🤖 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/shared/navigation/navDecision.ts` around lines 1 - 17, Review and trim all comments in src/shared/navigation/navDecision.ts at the specified locations (lines 1-17 as the anchor, and siblings at lines 121-124, 162-165, 213-220, and 238-243) that explain provenance, history, or justification rather than describing the actual cell behavior. For each comment, reduce it to a single concise sentence focused on what the behavior does, removing any references to plan steps, phases, tracks, stage IDs, or historical context. The goal is to keep the navigation table a clean behavior map, not a narrative of design decisions or changes.Source: Coding guidelines
♻️ Duplicate comments (1)
e2e/nav-matrix-dashboard.test.ts (1)
101-104:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
expect.pollcan troll these negative checks by passing on the first zero.Use a settle-window helper that samples for the full duration and fails on any observed call, because these
toBe(0|false)polls may still miss late IPCs and leave flakes in disguise.
e2e/nav-matrix-dashboard.test.ts#L101-L104: replace the zero-length poll with a full-window “nofocus-comfy-windowinvocation observed” helper.e2e/nav-matrix-dashboard.test.ts#L122-L125: apply the same full-window helper for “norun-actionlaunch” instead oftoBe(false)poll.e2e/nav-matrix-instance.test.ts#L101-L104: apply the same helper for “nofocus-comfy-windowinvocation observed.”e2e/nav-matrix-instance.test.ts#L122-L125: apply the same helper for “norun-actionlaunch.”
As per coding guidelines, "Flaky tests must be fixed immediately when discovered; avoid timing assertions (use generous thresholds or assert behavior instead), process lifecycle races (add explicit readiness signals), and account for platform-specific quirks in timeouts and assertions."In `@playwright/test` v1.58.2, does `expect.poll` stop after the first passing sample, and what is the recommended pattern to assert that an event/IPC does **not** occur during a settling window?🤖 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 `@e2e/nav-matrix-dashboard.test.ts` around lines 101 - 104, The current `expect.poll` assertions with `toBe(0)` and `toBe(false)` can pass on the first sample and miss late-arriving IPCs, causing flaky tests. Create or use a settle-window helper that samples IPC invocations for the full timeout duration and fails if any matching call is observed at any point, rather than just checking once. Apply this helper at the following locations: in e2e/nav-matrix-dashboard.test.ts lines 101-104, replace the `expect.poll` for `focus-comfy-window` invocation count with a full-window "no focus-comfy-window invocation observed" assertion; in e2e/nav-matrix-dashboard.test.ts lines 122-125, replace the `toBe(false)` poll for `run-action` launch with the same full-window "no run-action launch" helper; in e2e/nav-matrix-instance.test.ts lines 101-104, apply the full-window "no focus-comfy-window invocation" helper; and in e2e/nav-matrix-instance.test.ts lines 122-125, apply the full-window "no run-action launch" helper. The settle-window approach must sample throughout the duration to catch any IPC invocations that arrive after the initial check.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.
Outside diff comments:
In `@src/main/index.ts`:
- Around line 1557-1570: Replace all hard-coded English strings in the
system-modal dialog (title, message, details items, confirmLabel,
secondaryLabel, and cancelLabel) with calls to i18n.t for localization. For
strings containing dynamic content like targetName, use i18n.t with
interpolation parameters. This issue appears in multiple locations: at lines
1557-1570 (the Switch instance modal) and lines 1986-1999 (another similar
modal), so both locations must be updated with the appropriate i18n.t calls to
ensure non-English locales display translated prompts instead of English text.
- Around line 1549-1553: Remove the "matrix row 9" reference from the comment
block starting with "Three-way choice" to comply with coding guidelines that
prohibit referencing plan steps, phases, tracks, or stage IDs in comments.
Rewrite the comment to describe the three-way choice behavior directly by
explaining that when the current local instance is running, it offers to keep it
alive in a separate window instead of only stopping and switching, without any
reference to planning-table rows or stages. You can keep the issue reference
(`#926`) if it provides functional context, but remove all matrix-row and
planning-stage terminology.
- Around line 1455-1463: The `allowDuplicate` flag is not being threaded through
the deferred launch payload in the `deliverPickToEntry` function and other
similar locations. Add the `allowDuplicate` flag as a parameter to
`deliverPickToEntry` and include it in the `picker-pick-install` payload object
alongside `kind` and `installationId`, so that when the payload is processed by
`onLaunch()`, it has access to the flag and can properly prevent reusing
existing entries when a duplicate launch should not occur. Apply this same fix
to all other locations mentioned in the comment (lines 1474-1500) where similar
deferred launch payloads are being sent.
In `@src/renderer/src/comfyTitlePopup/InstancePickerView.vue`:
- Around line 477-479: The routing decision logic in the `hostIsLocalRunning`
variable (at line 477-479 and also at line 616-618) does not handle the case
where `currentView` and `currentCategory` from the snapshot are optional and may
be undefined. Before checking if `hostIsLocalRunning` based on whether
`currentView === 'instance'` and `currentCategory === 'local'`, first establish
default values for these optional properties (defaulting to 'dashboard' for
currentView and null for currentCategory). Apply this same fix at both affected
locations to ensure consistent host context derivation before any routing
decisions are made that feed into downstream logic like
`ComfyUISettingsContent`.
In `@src/renderer/src/components/settings/ComfyUISettingsContent.vue`:
- Around line 826-829: The watcher on opInflight currently only closes the
regular More menu by calling closeMoreMenu(), but the split-button caret menu
remains open, allowing actions to be exposed after the footer is disabled. In
the opInflight watcher that checks if yes is true and moreMenuOpen.value is
true, add an additional call to close the caret menu (in addition to the
existing closeMoreMenu() call) to ensure both menu types are closed when an
operation begins and locks the footer.
In `@src/shared/navigation/navDecision.ts`:
- Around line 1-17: Review and trim all comments in
src/shared/navigation/navDecision.ts at the specified locations (lines 1-17 as
the anchor, and siblings at lines 121-124, 162-165, 213-220, and 238-243) that
explain provenance, history, or justification rather than describing the actual
cell behavior. For each comment, reduce it to a single concise sentence focused
on what the behavior does, removing any references to plan steps, phases,
tracks, stage IDs, or historical context. The goal is to keep the navigation
table a clean behavior map, not a narrative of design decisions or changes.
---
Duplicate comments:
In `@e2e/nav-matrix-dashboard.test.ts`:
- Around line 101-104: The current `expect.poll` assertions with `toBe(0)` and
`toBe(false)` can pass on the first sample and miss late-arriving IPCs, causing
flaky tests. Create or use a settle-window helper that samples IPC invocations
for the full timeout duration and fails if any matching call is observed at any
point, rather than just checking once. Apply this helper at the following
locations: in e2e/nav-matrix-dashboard.test.ts lines 101-104, replace the
`expect.poll` for `focus-comfy-window` invocation count with a full-window "no
focus-comfy-window invocation observed" assertion; in
e2e/nav-matrix-dashboard.test.ts lines 122-125, replace the `toBe(false)` poll
for `run-action` launch with the same full-window "no run-action launch" helper;
in e2e/nav-matrix-instance.test.ts lines 101-104, apply the full-window "no
focus-comfy-window invocation" helper; and in e2e/nav-matrix-instance.test.ts
lines 122-125, apply the full-window "no run-action launch" helper. The
settle-window approach must sample throughout the duration to catch any IPC
invocations that arrive after the initial check.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: db6323a1-6ff2-4e90-be84-aee76fbd47af
📒 Files selected for processing (17)
e2e/nav-matrix-cloud.test.tse2e/nav-matrix-dashboard.test.tse2e/nav-matrix-instance.test.tse2e/support/navMatrixHelpers.tslocales/en.jsonlocales/zh.jsonsrc/main/host/registry.test.tssrc/main/host/registry.tssrc/main/index.tssrc/main/popups/systemModal.tssrc/main/popups/titlePopup.tssrc/preload/comfyTitlePopupPreload.tssrc/renderer/src/comfyTitlePopup/InstancePickerView.vuesrc/renderer/src/components/settings/ComfyUISettingsContent.test.tssrc/renderer/src/components/settings/ComfyUISettingsContent.vuesrc/shared/navigation/navDecision.tssrc/shared/viewKind.ts
💤 Files with no reviewable changes (1)
- src/main/host/registry.ts
- locales: add instancePicker.openCloud / openInNewWindow (en + zh). The navDecision table referenced these NAV_LABEL keys but they were never in the catalog, so the primary CTA / caret rendered the raw key (e.g. "instancePicker.openInNewWindow"). - navDecision.test: guard test asserting every NAV_LABEL value resolves in en.json, so a missing label fails CI instead of shipping (NavLabelKey is only a string union, so the compiler can't catch it). - e2e nav-matrix: replace the flaky negative `expect.poll(...).toBe(0|false)` checks with expectNoIpcInvocation, which samples the full settle window and fails on any observed call. poll resolves on the first passing sample, and the count starts at 0, so the old checks passed at t=0 and a late IPC slipped past.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@e2e/support/navMatrixHelpers.ts`:
- Around line 52-58: The do-while loop checking for unexpected IPCs can miss a
forbidden IPC that arrives in the final sampling interval before the deadline
expires because the loop exits immediately when Date.now() >= deadline without
sampling one last time. After the while loop completes, add one final call to
getIpcInvocations to sample at or after the deadline, find any match using the
same match predicate, and assert it is undefined to ensure no late-arriving IPCs
slip through the check.
🪄 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: ddb45872-f8f2-4cc7-9691-e6104f759206
📒 Files selected for processing (6)
e2e/nav-matrix-dashboard.test.tse2e/nav-matrix-instance.test.tse2e/support/navMatrixHelpers.tslocales/en.jsonlocales/zh.jsonsrc/shared/navigation/navDecision.test.ts
The do/while slept the final interval without re-sampling, so an IPC arriving in the last interval before windowMs went unobserved. Sample on a loop that always ends with a check at the deadline.
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 (6)
locales/en.json (1)
1218-1242: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winConsider adding the
openRemotekey for i18n completeness.The code in
ComfyUISettingsContent.vueusest('instancePicker.openRemote', 'Open Remote')as a fallback when displaying remote instances (line 551-553 in the code snippet). While the fallback string prevents crashes, adding the key to both locale files would follow i18n best practices and allow for future translation customization.➕ Suggested addition
"openCloud": "Open Cloud", + "openRemote": "Open Remote", "openInNewWindow": "Open in new window",🤖 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 `@locales/en.json` around lines 1218 - 1242, Add the missing `openRemote` key to the instancePicker object in the locale file to follow i18n best practices. The key should be added to the instancePicker section in locales/en.json with the value "Open Remote", which matches the fallback string currently used in the ComfyUISettingsContent.vue component's translation call for t('instancePicker.openRemote', 'Open Remote'). This ensures the translation key exists in the locale file rather than relying on the fallback string.locales/zh.json (1)
1218-1242: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winConsider adding the
openRemotekey for i18n completeness.Matching the English locale file, add the Chinese translation for
openRemoteto support remote instance display. The code inComfyUISettingsContent.vueuses this key with a fallback for remote connections.➕ Suggested addition
"openCloud": "打开云端", + "openRemote": "打开远程", "openInNewWindow": "在新窗口中打开",🤖 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 `@locales/zh.json` around lines 1218 - 1242, Add the missing "openRemote" translation key to the instancePicker section in the Chinese locale file (zh.json) to ensure completeness and consistency with the English locale file. The key is used in ComfyUISettingsContent.vue for remote instance display and should be placed with the other instance action keys like "open", "restart", and "switch" in the instancePicker object. Add the appropriate Chinese translation value for the "openRemote" key to support remote connection functionality.src/main/index.ts (4)
1586-1599:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winLocalize the switch confirmation copy.
These literals bypass the translated picker strings, so non-English users can see an English system prompt in this flow.
🤖 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/main/index.ts` around lines 1586 - 1599, The dialog confirmation copy in the switch instance flow contains hardcoded English strings that need to be localized. Replace the hardcoded English literals in the title, message, details items array, confirmLabel, secondaryLabel, and cancelLabel fields with localized/translated strings using the appropriate i18n or localization function available in this codebase. Ensure all user-facing text in this dialog is properly translated rather than hardcoded in English.
1569-1577:⚠️ Potential issue | 🟠 Major | ⚡ Quick winValidate the target before stopping the current instance.
If the picker sends a stale/deleted
installationId, the name lookup falls back to the id and the swap still detaches the current host before the launch path can fail.🛡️ Proposed target validation
// Picking the host's own install — picker already dismissed at // the IPC boundary, nothing more to do. if (parentEntry.installationId === installationId) return + + const target = await getInstallation(installationId).catch(() => null) + if (!target) return // Install-backed parent → swap by detaching first, then routing @@ if (parentEntry.installationId != null) { if (!opts?.confirmed && shouldConfirmKillForEntry(parentEntry)) { - let targetName = installationId - try { - const target = await getInstallation(installationId) - if (target?.name) targetName = target.name - } catch { - // Name lookup is cosmetic — fall through with the id as the label. - } + const targetName = target.name || installationIdAlso applies to: 1624-1631
🤖 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/main/index.ts` around lines 1569 - 1577, The code currently catches errors when looking up the installation via getInstallation but silently continues with the swap operation even if the target installation is invalid or deleted. Before proceeding with shouldConfirmKillForEntry and the subsequent swap that detaches the current host, validate that the target installation actually exists and is valid. Modify the error handling in the try-catch block around the getInstallation call to check if the target was successfully retrieved, and if not (either due to an error or because target is null), stop the process and prevent the swap from detaching the current instance before confirming the target is real.
1503-1529:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftKeep
allowDuplicatefrom taking a duplicate detour.
allowDuplicatebypasses the focus-existing branch here, but the later launch still hitsonLaunch’s existing-window reuse path first and drops the attach claim, so an intended duplicate can refresh the old window and leave the new chooser stranded; thread a duplicate-aware claim through launch, and only honor it for validated cloud/remote targets.🤖 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/main/index.ts` around lines 1503 - 1529, The `allowDuplicate` parameter in the openInstallInNewWindow function bypasses the existing window focus check but is not communicated to downstream processes, causing the subsequent launch to still reuse the existing window instead of creating a duplicate. Pass the allowDuplicate flag through the call chain (particularly through deliverPickToEntry and into the onLaunch process) so that the window reuse logic respects this flag and only reuses existing windows when allowDuplicate is false, while ensuring this duplicate-aware behavior is only honored for validated cloud/remote targets.
1578-1582:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRemove the row-number reference from this comment.
Describe the behavior directly instead of tying the code to an external row label.
🧹 Proposed comment cleanup
- // Three-way choice (issue `#926`, matrix row 9): the current local + // Three-way choice: the current localAs per coding guidelines, "Never reference plan steps, phases, tracks, or stage IDs in comments"; describe what the code does instead.
🤖 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/main/index.ts` around lines 1578 - 1582, Remove the "matrix row 9" reference from the comment block starting before the three-way choice logic. Rewrite the comment to describe the behavior directly without external row references, explaining that when the current local instance is running, the code offers to keep it alive in a separate window as an alternative to stopping and switching, with secondary routing to openInstallInNewWindow and confirm falling through to the in-place swap below.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/main/popups/titlePopup.ts`:
- Line 783: The 'reset-zoom' menu item label is hardcoded in English and does
not use the localization system (labelKey/i18n path) like adjacent file-menu
labels do. Refactor this menu item to use the i18n localization framework for
the base label text while dynamically interpolating the percent value into the
localized string, following the same pattern as other menu items in the file.
---
Outside diff comments:
In `@locales/en.json`:
- Around line 1218-1242: Add the missing `openRemote` key to the instancePicker
object in the locale file to follow i18n best practices. The key should be added
to the instancePicker section in locales/en.json with the value "Open Remote",
which matches the fallback string currently used in the
ComfyUISettingsContent.vue component's translation call for
t('instancePicker.openRemote', 'Open Remote'). This ensures the translation key
exists in the locale file rather than relying on the fallback string.
In `@locales/zh.json`:
- Around line 1218-1242: Add the missing "openRemote" translation key to the
instancePicker section in the Chinese locale file (zh.json) to ensure
completeness and consistency with the English locale file. The key is used in
ComfyUISettingsContent.vue for remote instance display and should be placed with
the other instance action keys like "open", "restart", and "switch" in the
instancePicker object. Add the appropriate Chinese translation value for the
"openRemote" key to support remote connection functionality.
In `@src/main/index.ts`:
- Around line 1586-1599: The dialog confirmation copy in the switch instance
flow contains hardcoded English strings that need to be localized. Replace the
hardcoded English literals in the title, message, details items array,
confirmLabel, secondaryLabel, and cancelLabel fields with localized/translated
strings using the appropriate i18n or localization function available in this
codebase. Ensure all user-facing text in this dialog is properly translated
rather than hardcoded in English.
- Around line 1569-1577: The code currently catches errors when looking up the
installation via getInstallation but silently continues with the swap operation
even if the target installation is invalid or deleted. Before proceeding with
shouldConfirmKillForEntry and the subsequent swap that detaches the current
host, validate that the target installation actually exists and is valid. Modify
the error handling in the try-catch block around the getInstallation call to
check if the target was successfully retrieved, and if not (either due to an
error or because target is null), stop the process and prevent the swap from
detaching the current instance before confirming the target is real.
- Around line 1503-1529: The `allowDuplicate` parameter in the
openInstallInNewWindow function bypasses the existing window focus check but is
not communicated to downstream processes, causing the subsequent launch to still
reuse the existing window instead of creating a duplicate. Pass the
allowDuplicate flag through the call chain (particularly through
deliverPickToEntry and into the onLaunch process) so that the window reuse logic
respects this flag and only reuses existing windows when allowDuplicate is
false, while ensuring this duplicate-aware behavior is only honored for
validated cloud/remote targets.
- Around line 1578-1582: Remove the "matrix row 9" reference from the comment
block starting before the three-way choice logic. Rewrite the comment to
describe the behavior directly without external row references, explaining that
when the current local instance is running, the code offers to keep it alive in
a separate window as an alternative to stopping and switching, with secondary
routing to openInstallInNewWindow and confirm falling through to the in-place
swap below.
🪄 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: c843b530-5c19-4b0d-bbfb-cb3ca1577542
📒 Files selected for processing (4)
locales/en.jsonlocales/zh.jsonsrc/main/index.tssrc/main/popups/titlePopup.ts
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
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 (6)
locales/en.json (1)
1218-1242: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winConsider adding the
openRemotekey for i18n completeness.The code in
ComfyUISettingsContent.vueusest('instancePicker.openRemote', 'Open Remote')as a fallback when displaying remote instances (line 551-553 in the code snippet). While the fallback string prevents crashes, adding the key to both locale files would follow i18n best practices and allow for future translation customization.➕ Suggested addition
"openCloud": "Open Cloud", + "openRemote": "Open Remote", "openInNewWindow": "Open in new window",🤖 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 `@locales/en.json` around lines 1218 - 1242, Add the missing `openRemote` key to the instancePicker object in the locale file to follow i18n best practices. The key should be added to the instancePicker section in locales/en.json with the value "Open Remote", which matches the fallback string currently used in the ComfyUISettingsContent.vue component's translation call for t('instancePicker.openRemote', 'Open Remote'). This ensures the translation key exists in the locale file rather than relying on the fallback string.locales/zh.json (1)
1218-1242: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winConsider adding the
openRemotekey for i18n completeness.Matching the English locale file, add the Chinese translation for
openRemoteto support remote instance display. The code inComfyUISettingsContent.vueuses this key with a fallback for remote connections.➕ Suggested addition
"openCloud": "打开云端", + "openRemote": "打开远程", "openInNewWindow": "在新窗口中打开",🤖 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 `@locales/zh.json` around lines 1218 - 1242, Add the missing "openRemote" translation key to the instancePicker section in the Chinese locale file (zh.json) to ensure completeness and consistency with the English locale file. The key is used in ComfyUISettingsContent.vue for remote instance display and should be placed with the other instance action keys like "open", "restart", and "switch" in the instancePicker object. Add the appropriate Chinese translation value for the "openRemote" key to support remote connection functionality.src/main/index.ts (4)
1586-1599:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winLocalize the switch confirmation copy.
These literals bypass the translated picker strings, so non-English users can see an English system prompt in this flow.
🤖 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/main/index.ts` around lines 1586 - 1599, The dialog confirmation copy in the switch instance flow contains hardcoded English strings that need to be localized. Replace the hardcoded English literals in the title, message, details items array, confirmLabel, secondaryLabel, and cancelLabel fields with localized/translated strings using the appropriate i18n or localization function available in this codebase. Ensure all user-facing text in this dialog is properly translated rather than hardcoded in English.
1569-1577:⚠️ Potential issue | 🟠 Major | ⚡ Quick winValidate the target before stopping the current instance.
If the picker sends a stale/deleted
installationId, the name lookup falls back to the id and the swap still detaches the current host before the launch path can fail.🛡️ Proposed target validation
// Picking the host's own install — picker already dismissed at // the IPC boundary, nothing more to do. if (parentEntry.installationId === installationId) return + + const target = await getInstallation(installationId).catch(() => null) + if (!target) return // Install-backed parent → swap by detaching first, then routing @@ if (parentEntry.installationId != null) { if (!opts?.confirmed && shouldConfirmKillForEntry(parentEntry)) { - let targetName = installationId - try { - const target = await getInstallation(installationId) - if (target?.name) targetName = target.name - } catch { - // Name lookup is cosmetic — fall through with the id as the label. - } + const targetName = target.name || installationIdAlso applies to: 1624-1631
🤖 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/main/index.ts` around lines 1569 - 1577, The code currently catches errors when looking up the installation via getInstallation but silently continues with the swap operation even if the target installation is invalid or deleted. Before proceeding with shouldConfirmKillForEntry and the subsequent swap that detaches the current host, validate that the target installation actually exists and is valid. Modify the error handling in the try-catch block around the getInstallation call to check if the target was successfully retrieved, and if not (either due to an error or because target is null), stop the process and prevent the swap from detaching the current instance before confirming the target is real.
1503-1529:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftKeep
allowDuplicatefrom taking a duplicate detour.
allowDuplicatebypasses the focus-existing branch here, but the later launch still hitsonLaunch’s existing-window reuse path first and drops the attach claim, so an intended duplicate can refresh the old window and leave the new chooser stranded; thread a duplicate-aware claim through launch, and only honor it for validated cloud/remote targets.🤖 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/main/index.ts` around lines 1503 - 1529, The `allowDuplicate` parameter in the openInstallInNewWindow function bypasses the existing window focus check but is not communicated to downstream processes, causing the subsequent launch to still reuse the existing window instead of creating a duplicate. Pass the allowDuplicate flag through the call chain (particularly through deliverPickToEntry and into the onLaunch process) so that the window reuse logic respects this flag and only reuses existing windows when allowDuplicate is false, while ensuring this duplicate-aware behavior is only honored for validated cloud/remote targets.
1578-1582:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRemove the row-number reference from this comment.
Describe the behavior directly instead of tying the code to an external row label.
🧹 Proposed comment cleanup
- // Three-way choice (issue `#926`, matrix row 9): the current local + // Three-way choice: the current localAs per coding guidelines, "Never reference plan steps, phases, tracks, or stage IDs in comments"; describe what the code does instead.
🤖 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/main/index.ts` around lines 1578 - 1582, Remove the "matrix row 9" reference from the comment block starting before the three-way choice logic. Rewrite the comment to describe the behavior directly without external row references, explaining that when the current local instance is running, the code offers to keep it alive in a separate window as an alternative to stopping and switching, with secondary routing to openInstallInNewWindow and confirm falling through to the in-place swap below.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/main/popups/titlePopup.ts`:
- Line 783: The 'reset-zoom' menu item label is hardcoded in English and does
not use the localization system (labelKey/i18n path) like adjacent file-menu
labels do. Refactor this menu item to use the i18n localization framework for
the base label text while dynamically interpolating the percent value into the
localized string, following the same pattern as other menu items in the file.
---
Outside diff comments:
In `@locales/en.json`:
- Around line 1218-1242: Add the missing `openRemote` key to the instancePicker
object in the locale file to follow i18n best practices. The key should be added
to the instancePicker section in locales/en.json with the value "Open Remote",
which matches the fallback string currently used in the
ComfyUISettingsContent.vue component's translation call for
t('instancePicker.openRemote', 'Open Remote'). This ensures the translation key
exists in the locale file rather than relying on the fallback string.
In `@locales/zh.json`:
- Around line 1218-1242: Add the missing "openRemote" translation key to the
instancePicker section in the Chinese locale file (zh.json) to ensure
completeness and consistency with the English locale file. The key is used in
ComfyUISettingsContent.vue for remote instance display and should be placed with
the other instance action keys like "open", "restart", and "switch" in the
instancePicker object. Add the appropriate Chinese translation value for the
"openRemote" key to support remote connection functionality.
In `@src/main/index.ts`:
- Around line 1586-1599: The dialog confirmation copy in the switch instance
flow contains hardcoded English strings that need to be localized. Replace the
hardcoded English literals in the title, message, details items array,
confirmLabel, secondaryLabel, and cancelLabel fields with localized/translated
strings using the appropriate i18n or localization function available in this
codebase. Ensure all user-facing text in this dialog is properly translated
rather than hardcoded in English.
- Around line 1569-1577: The code currently catches errors when looking up the
installation via getInstallation but silently continues with the swap operation
even if the target installation is invalid or deleted. Before proceeding with
shouldConfirmKillForEntry and the subsequent swap that detaches the current
host, validate that the target installation actually exists and is valid. Modify
the error handling in the try-catch block around the getInstallation call to
check if the target was successfully retrieved, and if not (either due to an
error or because target is null), stop the process and prevent the swap from
detaching the current instance before confirming the target is real.
- Around line 1503-1529: The `allowDuplicate` parameter in the
openInstallInNewWindow function bypasses the existing window focus check but is
not communicated to downstream processes, causing the subsequent launch to still
reuse the existing window instead of creating a duplicate. Pass the
allowDuplicate flag through the call chain (particularly through
deliverPickToEntry and into the onLaunch process) so that the window reuse logic
respects this flag and only reuses existing windows when allowDuplicate is
false, while ensuring this duplicate-aware behavior is only honored for
validated cloud/remote targets.
- Around line 1578-1582: Remove the "matrix row 9" reference from the comment
block starting before the three-way choice logic. Rewrite the comment to
describe the behavior directly without external row references, explaining that
when the current local instance is running, the code offers to keep it alive in
a separate window as an alternative to stopping and switching, with secondary
routing to openInstallInNewWindow and confirm falling through to the in-place
swap below.
🪄 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: c843b530-5c19-4b0d-bbfb-cb3ca1577542
📒 Files selected for processing (4)
locales/en.jsonlocales/zh.jsonsrc/main/index.tssrc/main/popups/titlePopup.ts
🛑 Comments failed to post (1)
src/main/popups/titlePopup.ts (1)
783-783:
⚠️ Potential issue | 🟡 Minor | ⚡ Quick winLocalize the dynamic Reset Zoom label.
This menu item bypasses the
labelKey/i18npath used by adjacent file-menu labels, so localized builds will show English here.🌍 Proposed localization hook
- items.push({ id: 'reset-zoom', label: `Reset Zoom (${percent}%)` }) + items.push({ id: 'reset-zoom', label: i18n.t('fileMenu.resetZoom', { percent }) })🤖 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/main/popups/titlePopup.ts` at line 783, The 'reset-zoom' menu item label is hardcoded in English and does not use the localization system (labelKey/i18n path) like adjacent file-menu labels do. Refactor this menu item to use the i18n localization framework for the base label text while dynamically interpolating the percent value into the localized string, following the same pattern as other menu items in the file.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Refactors instance/window navigation (#926) from logic scattered across three sites into one pure, table-driven decision function. Given the current view (Dashboard | Instance | Cloud) and a clicked target,
decideNavigationreturns whether to switch in place, restart, focus, or open a new window — and drives the footer CTA label, a new caret split-button, and an in-drawer confirm from that single source. Ships the CTO matrix's behavior changes (Instance→Instance 3-way, cloud/instance/remote always-new-window) and adds Remote handling the matrix never specified.Changes
What
src/shared/navigation/navDecision.ts(new) —decideNavigation(NavInput): NavDecision, a totalReadonlyMaptransition table overViewKind × TargetKind × TargetRun. Pure, O(1), runs identically in renderer + main.src/shared/viewKind.ts(new) — singleViewKind/Category/NavClassvocabulary +navClass(foldsremote ⇒ cloud) +viewKindFor(one classifier shared bycomputeViewKindand the snapshot builder).src/renderer/src/composables/useInstanceNavState.ts(new) — read-model derivingNavInputfacts; reusesuseInstallCtaso the one-install-one-window invariant stays single-sourced. A remote target routes via the cloud cells (non-local URL backend).src/renderer/src/composables/useInstanceActions.ts(new) — single dispatcher routing a decision's verb onto the bridge, with cloud-capacity + local-kill gates; aborts (no unhandled rejection) if a confirm dialog rejects.src/main/index.ts— extractsdeliverPickToEntry(shared by swap + new-window), adds theopenInstallInNewWindowprimitive (focus-existing else spawn a fresh chooser host), and threads aconfirmedflag throughpickInstallFromPickerso the renderer's in-drawer confirm replaces main's system modal.src/main/popups/titlePopup.ts— adds thecomfy-titlepopup:open-install-new-windowIPC +currentView/currentCategoryon the picker snapshot; picker IPC wirings wrapped in.catch().src/renderer/src/components/settings/ComfyUISettingsContent.vue— footer CTA + caret split-button driven bydecideNavigation; leading icons on the CTA and both menus; per-category CTA wording/icon (Open CloudvsOpen Remote); reuses the existingstopaction in the caret next to Restart.src/renderer/src/views/comfyUISettings/MoreMenu.vue— optionalheading+ per-itemicon(the plain "More" menu is unchanged).docs/instance-navigation-926-handoff.md(new) — verbatim CTO matrix + Implemented? column, Remote rows (§1b), deviations (§1a), manual-verification + unit-vs-e2e coverage tables.Breaking
primary-actionemit payload changedboolean → NavDecisionbut is internal to the picker. CTA labels for unchanged cells are byte-identical; existing telemetry (instance.switched,instance.opened_new_window) is preserved.Review Focus
navDecision.ts) is the whole behavior surface — each matrix cell is one entry. Review it as data against the CTO matrix in the handoff doc. A reachability test bans any no-op CTA for a reachable (host, target, run) combo.allowDuplicateplumbing is kept dormant/reserved).useInstallContextMenuthinning) is a deferred follow-up. The 3-way confirm rendering and focus-existing-for-a-real-window are unit/manual-only (e2e can't spawn a live ComfyUI process) — see the coverage table.Testing
State-driven navigation splits into a pure decision (unit-testable exhaustively) and side-effecting wiring (e2e via real bridge → IPC → window): unit pins what the decision is for every cell; e2e pins that the wiring fires for the risky paths. Un-e2e-able cells are documented with their reason in the handoff doc.
navDecision.test.ts— every matrix cell + the fullViewKind × TargetKind × TargetRun × class × intentcross-product (totality), caret/new-window selection, and the no-dead-no-op-CTA reachability guard.useInstanceActions.test.ts— verb→bridge routing for all verbs; cloud-capacity + switch/restart gates; the 3-way (switch / open-new / cancel) outcomes; dispatch-reject and bridge-undefined guards; dormantallowDuplicatepassthrough.useInstanceNavState.test.ts/registry.test.ts— run-state derivation; remote routes like cloud (incl. Instance→Remote ≡ Instance→Cloud);computeViewKind(null/unknown category).e2e/nav-matrix-{dashboard,instance,cloud}.test.ts— 8 tests asserting via recorded IPC + liveBrowserWindowcounts: same-window launch, focus-existing, new-window spawn (parent undisturbed),allowDuplicateprimitive.pnpm run typecheck(node/web/e2e/integration) +pnpm run lintclean; 1983 unit tests pass; e2e--project=lifecyclegreen (pnpm run buildfirst — e2e runs the built bundle).Manual verification matrix
Remote rows (not in the original CTO matrix — Remote ≡ Cloud target)
Full matrix, deviations, and coverage:
docs/instance-navigation-926-handoff.md.Closes #926 closes #911