-
Notifications
You must be signed in to change notification settings - Fork 374
Refactor conflict detection system and move to manager extension #5436
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/26/2025, 03:20:34 AM UTC 📈 Summary
📊 Test Reports by Browser
🎉 Click on the links above to view detailed test results for each browser configuration. |
94b76e0
to
658cfbc
Compare
5916b2e
to
a1fb8d2
Compare
55658de
to
5f3a666
Compare
🔧 Auto-fixes AppliedThis PR has been automatically updated to fix linting and formatting issues.
Changes made:
|
src/workbench/extensions/manager/composables/useConflictDetection.ts
Outdated
Show resolved
Hide resolved
src/workbench/extensions/manager/composables/useConflictDetection.ts
Outdated
Show resolved
Hide resolved
src/workbench/extensions/manager/composables/useConflictDetection.ts
Outdated
Show resolved
Hide resolved
src/workbench/extensions/manager/composables/useConflictDetection.ts
Outdated
Show resolved
Hide resolved
src/workbench/extensions/manager/composables/useConflictDetection.ts
Outdated
Show resolved
Hide resolved
src/workbench/extensions/manager/composables/useConflictDetection.ts
Outdated
Show resolved
Hide resolved
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.
Comprehensive PR Review
This review is generated by Claude. It may not always be accurate, as with human reviewers. If you believe that any of the comments are invalid or incorrect, please state why for each. For others, please implement the changes in one way or another.
Review Summary
PR: Manager/refactor/conflict detect (#5436)
Impact: 2591 additions, 2474 deletions across 46 files
Issue Distribution
- Critical: 0
- High: 2
- Medium: 5
- Low: 2
Category Breakdown
- Architecture: 0 issues
- Security: 0 issues
- Performance: 2 issues
- Code Quality: 7 issues
Key Findings
Architecture & Design
This PR represents a well-structured refactoring that improves code organization by moving manager-specific functionality to its dedicated extension folder. The architectural approach aligns with the repository's modular design principles:
Positive aspects:
- ✅ Proper separation of concerns by moving conflict detection to manager extension
- ✅ Clean extraction of utility functions into focused modules
- ✅ Maintained backward compatibility through proper interface design
- ✅ Good use of compositional patterns following Vue 3 best practices
Minor considerations:
- The consolidation logic properly handles package deduplication and conflict merging
- State management follows established Pinia patterns appropriately
Security Considerations
The refactored code maintains security best practices:
- ✅ Proper input validation in version compatibility checks
- ✅ Safe error handling that doesn't expose sensitive information
- ✅ Appropriate use of abort controllers for request cancellation
- ✅ No hardcoded secrets or credentials detected
Performance Impact
Performance improvements identified:
- ✅ Bulk API usage reduces network requests significantly
- ✅ Proper use of caching mechanisms through composables
- ✅ Efficient conflict consolidation using modern utilities (es-toolkit)
Performance concerns flagged:
- Race conditions in async guards that could cause unnecessary parallel executions
- Console logging statements that may impact production performance
- Some algorithmic inefficiencies in OS detection logic
Integration Points
The refactoring maintains clean integration with existing systems:
- ✅ Registry API integration through established service layers
- ✅ Manager store integration preserved
- ✅ Component interface compatibility maintained
- ✅ Test coverage expanded comprehensively
Code Quality Assessment
Strengths:
- Comprehensive test coverage for new utility functions (299+ test lines)
- Proper TypeScript usage in most areas
- Good documentation and JSDoc comments
- Clean separation of responsibilities
Areas for improvement:
- Type safety: Two instances of 'as any' casting that bypass TypeScript protection
- Array safety: Unsafe array access without bounds checking
- State mutation: Direct array mutation that may cause reactivity issues
- Error handling: Non-null assertions that could cause runtime crashes
Positive Observations
This is a high-quality refactoring that demonstrates several best practices:
- Methodical approach: The PR breaks down a large composable into focused utilities
- Comprehensive testing: New utility functions have extensive test coverage
- Type safety improvements: Elimination of 'any' types in test files
- Performance optimization: Bulk API usage and efficient conflict consolidation
- Code organization: Better separation of manager-specific functionality
- Backward compatibility: Maintained existing interfaces while improving internals
References
- ComfyUI Frontend Repository Guide
- Vue 3 Composition API Best Practices
- TypeScript Strict Mode Guidelines
Next Steps
- Address critical issues before merge: Fix the type casting issues to maintain type safety
- Consider architectural feedback for long-term maintainability: The current structure is solid
- Enhance error handling: Replace non-null assertions with safe fallbacks
- Performance optimizations: Consider implementing proper async locking mechanisms
This is a comprehensive automated review. For architectural decisions requiring human judgment, please request additional manual review.
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.
Could we move the utils and types files into src/workbench/extensions/manager?
2005e91
to
70b72f8
Compare
Did the tests start failing after rebasing on main? Reports: https://d0034130.comfyui-playwright-chromium.pages.dev/ |
@christian-byrne Yes |
999abdf
to
74b8ede
Compare
- Fix TypeScript errors in useConflictDetection, useImportFailedDetection, useUpdateAvailableNodes tests - Remove all any types and apply proper type assertions - Reflect backend SystemStats structure changes (add pytorch_version, embedded_python fields) - Fix OS mapping logic in systemCompatibility.ts (resolve darwin/win conflict) - Fix incorrect import paths (@/composables → @/workbench/extensions/manager/composables) - Remove unnecessary properties from mock objects and align with actual implementations - Add new utility tests (conflictUtils, systemCompatibility, versionUtil)
Prevents runtime error 'Invalid version. Must be a string. Got type undefined' when comparing versions with semver.compare() 🤖 Generated with Claude Code (https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
Same issue as usePackUpdateStatus - prevents runtime error when comparing versions with undefined installedVersion 🤖 Generated with Claude Code (https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
close by running conflict check in background
74b8ede
to
5aa20fe
Compare
## Refactor conflict detection system and move to manager extension ### Description This PR refactors the conflict detection system, moving it from the global composables to the manager extension folder for better code organization. Additionally, it improves test type safety and adds comprehensive test coverage for utility functions. ### Main Changes #### 📦 Code Organization - **Moved conflict detection to manager extension** - Relocated all conflict detection related composables, stores, and utilities from global scope to `/workbench/extensions/manager/` for better modularity (#5722) - **Moved from** `src/composables/useConflictDetection.ts` **to** `src/workbench/extensions/manager/composables/useConflictDetection.ts` - Moved related stores and composables to maintain cohesive module structure #### ♻️ Refactoring - **Extracted utility functions** - Split conflict detection logic into separate utility modules: - `conflictUtils.ts` - Conflict consolidation and summary generation - `systemCompatibility.ts` - OS and accelerator compatibility checking - `versionUtil.ts` - Version compatibility checking - **Removed duplicate state management** - Cleaned up redundant state and unused functions - **Improved naming conventions** - Renamed functions for better clarity - **Removed unused system environment code** - Cleaned up deprecated code #### 🔧 Test Improvements - **Fixed TypeScript errors** in all test files - removed all `any` type usage - **Added comprehensive test coverage**: - `conflictUtils.test.ts` - 299 lines of tests for conflict utilities - `systemCompatibility.test.ts` - 270 lines of tests for compatibility checking - `versionUtil.test.ts` - 342 lines of tests for version utilities - **Updated mock objects** to match actual implementations - **Aligned with backend changes** - Updated SystemStats structure to include `pytorch_version`, `embedded_python`, `required_frontend_version` #### 🐛 Bug Fixes - **Fixed OS detection bug** - Resolved issue where 'darwin' was incorrectly matched as 'Windows' due to containing 'win' substring - **Fixed import paths** - Updated all import paths after moving to manager extension - **Fixed unused exports** - Removed all unused function exports - **Fixed lint errors** - Resolved all ESLint and Prettier issues ### File Structure Changes ``` Before: src/ ├── composables/ │ └── useConflictDetection.ts (1374 lines) └── types/ After: src/ ├── utils/ │ ├── conflictUtils.ts (114 lines) │ ├── systemCompatibility.ts (125 lines) │ └── versionUtil.ts (enhanced) └── workbench/extensions/manager/ ├── composables/ │ ├── useConflictDetection.ts (758 lines) │ └── [other composables] └── stores/ └── conflictDetectionStore.ts ``` ### Testing All tests pass successfully: - ✅ **155 test files passed** - ✅ **2209 tests passed** - ⏩ 19 skipped (intentionally skipped subgraph-related tests) ### Impact - **Better code organization** - Manager-specific code is now properly isolated - **Improved maintainability** - Smaller, focused utility functions are easier to test and maintain - **Enhanced type safety** - No more `any` types in tests - **Comprehensive test coverage** - All utility functions are thoroughly tested ### Commits in this PR 1. OS detection bug fix and refactor 2. Remove unused system environment code 3. Improve function naming 4. Refactor conflict detection 5. Remove duplicate state and unused functions 6. Fix unused function exports 7. Move manager features to workbench extension folder 8. Fix import paths 9. Rename systemCompatibility file 10. Improve test type safety 11. Apply ESLint and Prettier fixes ## Screenshots (if applicable) [screen-capture.webm](https://github.com/user-attachments/assets/b4595604-3761-4d98-ae8e-5693cd0c95bd) ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-5436-Manager-refactor-conflict-detect-2686d73d36508186ba06f57dae3656e5) by [Unito](https://www.unito.io) --------- Co-authored-by: Claude <[email protected]>
Refactor conflict detection system and move to manager extension
Description
This PR refactors the conflict detection system, moving it from the global composables to the manager extension folder for better code organization. Additionally, it improves test type safety and adds comprehensive test coverage for utility functions.
Main Changes
📦 Code Organization
/workbench/extensions/manager/
for better modularity ([refactor] Migrate manager code fromsrc/composables
tosrc/workbench/extensions/manager
(2/2) #5722)src/composables/useConflictDetection.ts
tosrc/workbench/extensions/manager/composables/useConflictDetection.ts
♻️ Refactoring
conflictUtils.ts
- Conflict consolidation and summary generationsystemCompatibility.ts
- OS and accelerator compatibility checkingversionUtil.ts
- Version compatibility checking🔧 Test Improvements
any
type usageconflictUtils.test.ts
- 299 lines of tests for conflict utilitiessystemCompatibility.test.ts
- 270 lines of tests for compatibility checkingversionUtil.test.ts
- 342 lines of tests for version utilitiespytorch_version
,embedded_python
,required_frontend_version
🐛 Bug Fixes
File Structure Changes
Testing
All tests pass successfully:
Impact
any
types in testsCommits in this PR
Screenshots (if applicable)
screen-capture.webm
┆Issue is synchronized with this Notion page by Unito