-
Notifications
You must be signed in to change notification settings - Fork 329
Recently used sections #4346
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
base: main
Are you sure you want to change the base?
Recently used sections #4346
Conversation
…st tests accordingly
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.
reviewing now
Key Issues Found:1. Documentation - 2. Code Duplication - 3. API Usage - Both components use TreeExplorer for flat lists requiring 'dummy' expandedKeys - might be overkill for simple lists 4. Error Handling - Add validation in sorting: Overall: Good Vue 3 patterns and excellent test coverage. Main concern is component duplication for maintainability. |
…r displaying recently added and used items
… with RecentItemsSection for improved item display
… and ensure proper filtering for created and modified dates
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.
The PR is looking really good overall. There are a lot of strengths:
- Excellent test coverage with realistic mock data
- Good i18n support
- Clean Vue 3.5 Composition API code quality/style
- Well-typed, good usage of generics and types
- Clean separation of concerns in the stores
- Follows existing ComfyUI_frontend patterns for settings and sidebar components excellently
- Proper accessibility with keyboard navigation support
- Efficient code with use of computed values and parallel requests
I added various comments about potential improvements we could make.
Also, I had some general ideas for things we could add but don't necessarily need to be added in the initial version (things to consider):
- Move model preview functionality to a separate composable/component
- Add performance optimizations: Consider virtualization for large item lists
- Extend usage tracking concept to include interactions: Current "recently used" only tracks file modification, not actual user interaction
…nd usage tracking
…usage logging test
… prop for model definition handling
Hi @christian-byrne |
This is looking good. We just need to test a bit more and can include this in 1.25 this week. |
@Myestery thank you for your contribution, i just issued you a payout! |
@kevinpaik1 when I get paid for my PRs, it seems I got scammed by you, I didn't recommend contributors to join comfy's bounty program, (@kevinpaik1 prompt you to work for ever for small amount +provide test files+ they have unlimited review cycles+your PR will not reviewed by core team/owners, whenever you finish the work @kevinpaik1 will said you should improve) |
Hi @BenraouaneSoufiane — I’m sorry to hear this has been a frustrating experience. We were genuinely interested in exploring a path to collaborate with you, and I appreciate the effort you put into both the PRs you worked on. In the end, the submissions weren’t merged due to a mix of technical concerns and architectural misalignment raised by the team. That said, I issued a partial bounty payment as a gesture of appreciation — even though the work wasn’t accepted — and was clear up front that non-merge outcomes were a possible part of how the program works. I wanted to acknowledge the real time and effort you put in. I understand the review process may have felt long or unclear at times — that’s something I’m actively working to improve. And I’m always open to feedback or a DM if you’d like to share thoughts on how the experience could be better for contributors going forward. |
Ok, that was before issuing the partial payout, can we continue with the other bounties (boundary error & resuming downloads) |
fix #3077
requires comfyanonymous/ComfyUI#8792 to be merged
┆Issue is synchronized with this Notion page by Unito