-
Notifications
You must be signed in to change notification settings - Fork 374
feat: Let mode changes trigger a re-render for Vue nodes #5599
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
🎭 Playwright Test Results⏰ Completed at: 09/15/2025, 11:27:28 PM UTC 📈 Summary
📊 Test Reports by Browser
🎉 Click on the links above to view detailed test results for each browser configuration. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another great PR that solves the specific problem advertised, but uncovers a strange and related issue: If a single node is selected after swapping to vue mode, pressing the bypass keybind does not cause the state to toggle. The associated function in useCoreCommands isn't even called.
Excluding the missed log, everything else is functional and beautiful
}) | ||
break | ||
case 'mode': | ||
console.log('Do the thing') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this doesn't seem very descriptive or helpful, WDYT about removing this line?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am deeply ashamed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this be updated as well?
ComfyUI_frontend/src/lib/litegraph/test/LGraphNodeProperties.test.ts
Lines 23 to 30 in 2b57291
it('should initialize with default tracked properties', () => { | |
const propManager = new LGraphNodeProperties(mockNode) | |
const tracked = propManager.getTrackedProperties() | |
expect(tracked).toHaveLength(2) | |
expect(tracked).toContain('title') | |
expect(tracked).toContain('flags.collapsed') | |
}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I actually don't even know if that test is ran though, I had assumed litegraph tests were enabled some time ago, but if it's not complaining and CI is green then I think it should be fine for now until the tests are enabled later
Mind if I investigate that as a separate issue? I feel like it'll probably be a more involved fix (and maybe cover a few more scenarios) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That was my intent, yes. The code here is good.
Summary
Instrument mode to trigger node data updates.
Changes
┆Issue is synchronized with this Notion page by Unito