Decouple node help between sidebar and right panel#8110
Merged
christian-byrne merged 6 commits intomainfrom Jan 17, 2026
Merged
Decouple node help between sidebar and right panel#8110christian-byrne merged 6 commits intomainfrom
christian-byrne merged 6 commits intomainfrom
Conversation
- Extract useNodeHelpContent composable so NodeHelpContent fetches its own
content, allowing multiple panels to show help independently
- Remove sync behavior from NodeHelpPage that caused left sidebar to change
when selecting different graph nodes
- Add telemetry tracking for node library help button
pythongosssss
requested review from
a team,
Myestery and
benceruleanlu
as code owners
dosubot
bot
added
the
size:XXL
Contributor
📝 WalkthroughWalkthroughExtracted node-help fetching/rendering into a new composable Changes
Sequence Diagram(s)sequenceDiagram
participant Component as NodeHelpContent
participant Composable as useNodeHelpContent
participant Service as nodeHelpService
participant Renderer as renderMarkdownToHtml
Component->>Composable: provide node ref / node change
Composable->>Service: fetchNodeHelp(url, locale)
Service-->>Composable: markdown OR error
alt fetch success
Composable->>Renderer: renderMarkdownToHtml(markdown, baseUrl)
Renderer-->>Composable: sanitized HTML
Composable-->>Component: renderedHelpHtml, isLoading=false
else fetch fails
Composable-->>Component: fallback description or error, isLoading=false
end
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 |
🎭 Playwright Tests: ✅ PassedResults: 507 passed, 0 failed, 0 flaky, 8 skipped (Total: 515) 📊 Browser Reports
|
🎨 Storybook Build Status✅ Build completed successfully! ⏰ Completed at: 01/16/2026, 10:00:20 PM UTC 🔗 Links🎉 Your Storybook is ready for review! |
Bundle Size ReportSummary
Category Glance Per-category breakdownApp Entry Points — 3.36 MB (baseline 3.36 MB) • 🟢 -3.52 kBMain entry bundles and manifests
Status: 3 added / 3 removed Graph Workspace — 1.14 MB (baseline 1.14 MB) • 🔴 +3.35 kBGraph editor runtime, canvas, workflow orchestration
Status: 1 added / 1 removed Views & Navigation — 6.66 kB (baseline 6.66 kB) • ⚪ 0 BTop-level views, pages, and routed surfaces
Status: 1 added / 1 removed Panels & Settings — 372 kB (baseline 372 kB) • ⚪ 0 BConfiguration panels, inspectors, and settings screens
Status: 6 added / 6 removed UI Components — 203 kB (baseline 203 kB) • 🔴 +39 BReusable component library chunks
Status: 8 added / 8 removed Data & Services — 12.5 kB (baseline 12.5 kB) • ⚪ 0 BStores, services, APIs, and repositories
Status: 2 added / 2 removed Utilities & Hooks — 1.86 kB (baseline 1.41 kB) • 🔴 +460 BHelpers, composables, and utility bundles
Status: 2 added / 1 removed Vendor & Third-Party — 9.34 MB (baseline 9.34 MB) • ⚪ 0 BExternal libraries and shared vendor chunks
Other — 5.38 MB (baseline 5.38 MB) • 🔴 +131 BBundles that do not match a named category
Status: 16 added / 16 removed |
![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: 7
🤖 Fix all issues with AI agents
In `@src/components/graph/selectionToolbox/InfoButton.test.ts`:
- Around line 60-66: Remove the style-only assertions in the test: delete the
expectations that check CSS classes and the 'severity' attribute and keep only
behavioral assertions (e.g., the click test). In the test file
InfoButton.test.ts, update the case using mountComponent() and
wrapper.find('button') to remove
expect(button.classes()).toContain('help-button') and
expect(button.attributes('severity')).toBe('secondary'), ensuring the test
verifies behavior (click handling) only.
- Around line 10-15: The test defines openPanelMock at module scope and closes
it over the vi.mock factory; change this to use vi.hoisted() so the mock is
hoisted and not module-level mutable state: replace the top-level openPanelMock
with a hoisted mock via vi.hoisted() and have the mocked useRightSidePanelStore
factory return { openPanel: hoistedMock } (referencing openPanelMock and
useRightSidePanelStore in the test so the mock instance is created via
vi.hoisted before vi.mock is called).
- Around line 42-47: Replace the Button stub in the InfoButton test with the
real component import and registration: import Button from
'@/components/ui/button/Button.vue' and register it in the mount/shallowMount
options (components: { Button }) instead of the stubbed template; remove the
stub block that declares props 'severity'/'text' and ensure any test
interactions use the real Button API (variant, size, disabled, loading) so the
test exercises production behavior of the InfoButton component.
In `@src/components/sidebar/tabs/nodeLibrary/NodeTreeLeaf.vue`:
- Around line 117-122: The click handler onHelpClick calls useTelemetry() each
time, which should instead be invoked once at component setup; move the
composable call to the top-level of the <script setup> (e.g., const telemetry =
useTelemetry()), then update onHelpClick to call
telemetry.trackUiButtonClicked(...) and keep the existing
props.openNodeHelp(nodeDef.value) call; this ensures useTelemetry is
instantiated once and reused.
In `@src/composables/useNodeHelpContent.test.ts`:
- Around line 93-95: Import the ComfyNodeDefImpl type from
"@/stores/nodeDefStore" and create a small helper makeNodeRef that casts a
Partial<ComfyNodeDefImpl> to ComfyNodeDefImpl and returns ref(...) so you stop
using `as any`; update the test to replace each `ref(mockCoreNode as any)` and
`ref(mockCustomNode as any)` with `makeNodeRef(mockCoreNode)` and
`makeNodeRef(mockCustomNode)` respectively (affecting the occurrences referenced
in the comment), ensuring all node refs use `Partial<ComfyNodeDefImpl> as
ComfyNodeDefImpl` inside the helper.
- Around line 85-91: The test currently assigns global.fetch = mockFetch at
module scope which leaks the stub; instead, in the test file replace that global
assignment by creating the mock inside beforeEach and use vi.stubGlobal('fetch',
mockFetch) there, call mockFetch.mockReset() in beforeEach, and restore/unstub
in afterEach (or use vi.restoreAllMocks()/vi.unstubAllGlobals()) to ensure the
global fetch stub is cleaned up; update references to the mockFetch variable and
lifecycle hooks (beforeEach/afterEach) in useNodeHelpContent.test.ts
accordingly.
In `@src/composables/useNodeHelpContent.ts`:
- Around line 38-60: The watcher for toValue(nodeRef) can let earlier
fetchNodeHelp calls overwrite newer results; inside the async watcher callback
(around the call to nodeHelpService.fetchNodeHelp) add a "current request"
guard: maintain a locally scoped increasing requestId (or token) that you
increment before calling fetchNodeHelp, capture it in a local variable, then
after the await only assign helpContent.value / error.value if the captured id
equals the latest id; alternatively, if fetchNodeHelp supports AbortController
pass an abort signal and cancel the previous request before starting a new one.
Ensure this logic references the existing symbols nodeRef,
nodeHelpService.fetchNodeHelp, helpContent, isLoading, error, and locale so
stale responses are ignored and loading state remains correct.
Comment on lines
+117
to
+122
| const onHelpClick = () => { | ||
| useTelemetry()?.trackUiButtonClicked({ | ||
| button_id: 'node_library_help_button' | ||
| }) | ||
| props.openNodeHelp(nodeDef.value) | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider calling useTelemetry() at the component setup level.
Calling useTelemetry() inside the click handler instantiates the composable on every click. While it likely works due to internal caching, the idiomatic Vue pattern is to call composables at the top level of <script setup> and reuse the returned instance.
♻️ Suggested refactor
const settingStore = useSettingStore()
+const telemetry = useTelemetry()
const sidebarLocation = computed<'left' | 'right'>(() =>
settingStore.get('Comfy.Sidebar.Location')
)
const toggleBookmark = async () => {
await nodeBookmarkStore.toggleBookmark(nodeDef.value)
}
const onHelpClick = () => {
- useTelemetry()?.trackUiButtonClicked({
+ telemetry?.trackUiButtonClicked({
button_id: 'node_library_help_button'
})
props.openNodeHelp(nodeDef.value)
}🤖 Prompt for AI Agents
In `@src/components/sidebar/tabs/nodeLibrary/NodeTreeLeaf.vue` around lines 117 -
122, The click handler onHelpClick calls useTelemetry() each time, which should
instead be invoked once at component setup; move the composable call to the
top-level of the <script setup> (e.g., const telemetry = useTelemetry()), then
update onHelpClick to call telemetry.trackUiButtonClicked(...) and keep the
existing props.openNodeHelp(nodeDef.value) call; this ensures useTelemetry is
instantiated once and reused.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/composables/useNodeHelpContent.test.ts`:
- Around line 32-63: The test currently fully mocks dompurify and marked via
vi.mock (mocking default.sanitize, marked.parse and the Renderer.image/link),
which prevents exercising real rendering/sanitization; change the test to import
the real dompurify and marked modules and replace the full vi.mock with
spies/partial mocks instead (use vi.spyOn on dompurify.default.sanitize and
marked.marked.parse or wrap those libraries in a thin adapter you control and
mock that adapter), remove the custom Renderer class mock so the real
marked.Renderer is used, and update assertions to verify calls to the spy/mocked
adapter rather than validate the mocked implementation.
♻️ Duplicate comments (1)
src/composables/useNodeHelpContent.test.ts (1)
96-110: Replaceas anynode refs with a typed helper.Line 96 (and repeated usages below) use
ref(mock… as any), which violates the repo’s “no any” rule and hides the intended node shape. Import the real type and use a helper that castsPartial<ComfyNodeDefImpl>to keep intent explicit and type-safe.✅ Suggested refactor
import { nextTick, ref } from 'vue' +import type { ComfyNodeDefImpl } from '@/stores/nodeDefStore' +function makeNodeRef(node: Partial<ComfyNodeDefImpl>) { + return ref(node as ComfyNodeDefImpl) +} @@ - const nodeRef = ref(mockCoreNode as any) + const nodeRef = makeNodeRef(mockCoreNode) @@ - const nodeRef = ref(mockCustomNode as any) + const nodeRef = makeNodeRef(mockCustomNode)Please apply the same replacement to all other
ref(mock… as any)occurrences in this file. Based on learnings, also keep theimport typeon its own line.#!/bin/bash # Verify no remaining `as any` in this test file rg -n "as any" src/composables/useNodeHelpContent.test.tsAlso applies to: 122-137, 149-164, 177-204, 226-243, 258-292, 304-323
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/composables/useNodeHelpContent.test.ts`:
- Around line 349-380: In the "should ignore stale requests when node changes"
test, avoid the non-null assertion on resolveFirst by giving resolveFirst a safe
initial value and proper typing so it's always defined; for example declare
resolveFirst as let resolveFirst: (value: unknown) => void = () => {} and then
reassign it inside the Promise executor (the existing firstRequest/new Promise
code) so resolveFirst can be called later without using resolveFirst!; update
references to resolveFirst and firstRequest in the test accordingly to remove
the non-null assertion while preserving the same race-condition behavior.
Comment on lines
+349
to
+380
| it('should ignore stale requests when node changes', async () => { | ||
| const nodeRef = ref(mockCoreNode) | ||
| let resolveFirst: (value: unknown) => void | ||
| const firstRequest = new Promise((resolve) => { | ||
| resolveFirst = resolve | ||
| }) | ||
|
|
||
| mockFetch | ||
| .mockImplementationOnce(() => firstRequest) | ||
| .mockResolvedValueOnce({ | ||
| ok: true, | ||
| text: async () => '# Second node content' | ||
| }) | ||
|
|
||
| const { helpContent } = useNodeHelpContent(nodeRef) | ||
| await nextTick() | ||
|
|
||
| // Change node before first request completes | ||
| nodeRef.value = mockCustomNode | ||
| await nextTick() | ||
| await flushPromises() | ||
|
|
||
| // Now resolve the first (stale) request | ||
| resolveFirst!({ | ||
| ok: true, | ||
| text: async () => '# First node content' | ||
| }) | ||
| await flushPromises() | ||
|
|
||
| // Should have second node's content, not first | ||
| expect(helpContent.value).toBe('# Second node content') | ||
| }) |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Good race condition coverage; minor pattern suggestion.
The stale request test correctly verifies that old responses are ignored when the node changes mid-flight. The resolveFirst! non-null assertion (line 372) is safe here since the Promise executor runs synchronously, but could be slightly more explicit:
♻️ Optional: Avoid late-bound assertion
it('should ignore stale requests when node changes', async () => {
const nodeRef = ref(mockCoreNode)
- let resolveFirst: (value: unknown) => void
+ let resolveFirst: (value: unknown) => void = () => {}
const firstRequest = new Promise((resolve) => {
resolveFirst = resolve
})
// ... rest unchanged
- resolveFirst!({
+ resolveFirst({🤖 Prompt for AI Agents
In `@src/composables/useNodeHelpContent.test.ts` around lines 349 - 380, In the
"should ignore stale requests when node changes" test, avoid the non-null
assertion on resolveFirst by giving resolveFirst a safe initial value and proper
typing so it's always defined; for example declare resolveFirst as let
resolveFirst: (value: unknown) => void = () => {} and then reassign it inside
the Promise executor (the existing firstRequest/new Promise code) so
resolveFirst can be called later without using resolveFirst!; update references
to resolveFirst and firstRequest in the test accordingly to remove the non-null
assertion while preserving the same race-condition behavior.
christian-byrne
approved these changes
Jan 17, 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
When the node library is open and you click on the node toolbar info button, this causes the node library info panel & right panel node info to show the same details.
Changes
┆Issue is synchronized with this Notion page by Unito