Skip to content

Fix version detection for disabled packs #5395

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 4 commits into from
Sep 7, 2025
Merged

Fix version detection for disabled packs #5395

merged 4 commits into from
Sep 7, 2025

Conversation

viva-jinyi
Copy link
Member

@viva-jinyi viva-jinyi commented Sep 6, 2025 •
edited by sync-by-unito bot
Loading

Summary

  • Fixed incorrect "Update Available" indicator for disabled packs
  • Added pack ID normalization to handle version suffixes consistently

Problem

When a pack is disabled, ComfyUI-Manager returns it with a version suffix (e.g., ComfyUI-GGUF@1_1_4) while enabled packs don't have this suffix. This inconsistency caused:

  1. Disabled packs to incorrectly show as having updates available even when on the latest version
  2. Version badge showing "Nightly" instead of the actual installed version

Solution

Created a normalizePackId utility function that:

  • Removes version suffixes from pack IDs for consistent access
  • Preserves version information in the object's ver field
  • Applies normalization in both HTTP responses and WebSocket updates

Changes

  • Added src/utils/packUtils.ts with normalization utilities
  • Updated comfyManagerStore to normalize pack keys
  • Updated useManagerQueue to normalize WebSocket pack data
  • Applied consistent normalization across conflict detection and node help modules

Test Plan

  • Disable an installed pack that's on the latest version
  • Verify it doesn't show "Update Available" indicator
  • Verify version badge shows correct version, not "Nightly"
  • Enable/disable packs and verify versions are maintained correctly
  • Check that WebSocket updates preserve version information

🤖 Generated with Claude Code

┆Issue is synchronized with this Notion page by Unito

@viva-jinyi viva-jinyi requested a review from a team as a code owner September 6, 2025 13:44
@dosubot dosubot bot added the size:M This PR changes 30-99 lines, ignoring generated files. label Sep 6, 2025
@github-actions GitHub Actions
Copy link

github-actions bot commented Sep 6, 2025 •
edited
Loading

🎭 Playwright Test Results

✅ All tests passed across all browsers!

⏰ Completed at: 09/07/2025, 05:20:19 AM UTC

📊 Test Reports by Browser

🎉 Your tests are passing across all browsers!

@viva-jinyi viva-jinyi requested a review from a team as a code owner September 6, 2025 13:54
@dosubot dosubot bot added size:L This PR changes 100-499 lines, ignoring generated files. and removed size:M This PR changes 30-99 lines, ignoring generated files. labels Sep 6, 2025
@viva-jinyi viva-jinyi self-assigned this Sep 6, 2025
@viva-jinyi viva-jinyi added needs-backport Fix/change that needs to be cherry-picked to the current feature freeze branch area:manager 1.26 labels Sep 6, 2025
christian-byrne
christian-byrne previously approved these changes Sep 6, 2025
View reviewed changes
Copy link
Contributor

@christian-byrne christian-byrne left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@viva-jinyi viva-jinyi force-pushed the manager/version-badge-update branch from be45523 to 104c06c Compare September 7, 2025 00:14
benceruleanlu
benceruleanlu previously approved these changes Sep 7, 2025
View reviewed changes
DrJKL
DrJKL reviewed Sep 7, 2025
View reviewed changes
tests-ui/tests/composables/useManagerQueue.test.ts
})

// Version suffixed keys should not exist
expect(installedPacks.value['ComfyUI-GGUF@1_1_4']).toBeUndefined()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this case, the string could be anything other than the two values above, right?

tests-ui/tests/store/comfyManagerStore.test.ts Outdated
}
}

;(mockManagerService.listInstalledPacks as any).mockResolvedValue(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you use vi.mocked to avoid as any?

arjansingh
arjansingh requested changes Sep 7, 2025
View reviewed changes
Copy link
Contributor

@arjansingh arjansingh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree with needs for tests for packUtils.ts Otherwise I like it.

viva-jinyi and others added 4 commits September 7, 2025 14:10
@viva-jinyi @claude
fix: normalize pack IDs to fix version detection for disabled packs
54f9c76 
When a pack is disabled, ComfyUI-Manager returns it with a version suffix
(e.g., "ComfyUI-GGUF@1_1_4") while enabled packs don't have this suffix.
This inconsistency caused disabled packs to incorrectly show as having
updates available even when they were on the latest version.

Changes:
- Add normalizePackId utility to consistently remove version suffixes
- Apply normalization in refreshInstalledList and WebSocket updates
- Use the utility across conflict detection and node help modules
- Ensure pack version info is preserved in the object's ver field

This fixes the "Update Available" indicator incorrectly showing for
disabled packs that are already on the latest version.

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <[email protected]>
@viva-jinyi
feature: test code added
06dc339
@viva-jinyi
test: packUtils test code added
5f17778
@viva-jinyi
test: address PR review feedback for test
1bc6944 
  improvements

  - Remove unnecessary .not.toThrow() assertion
  in useManagerQueue test
  - Add clarifying comments for version
  normalization test logic
  - Replace 'as any' with vi.mocked() for better
  type safety
@viva-jinyi viva-jinyi force-pushed the manager/version-badge-update branch from 9239995 to 1bc6944 Compare September 7, 2025 05:10
@viva-jinyi viva-jinyi requested a review from DrJKL September 7, 2025 05:10
@christian-byrne christian-byrne merged commit e2de4b1 into main Sep 7, 2025
19 checks passed
@christian-byrne christian-byrne deleted the manager/version-badge-update branch September 7, 2025 06:11
github-actions bot pushed a commit that referenced this pull request Sep 7, 2025
@viva-jinyi @claude @github-actions
Fix version detection for disabled packs (#5395)
b7eee0f 
* fix: normalize pack IDs to fix version detection for disabled packs

When a pack is disabled, ComfyUI-Manager returns it with a version suffix
(e.g., "ComfyUI-GGUF@1_1_4") while enabled packs don't have this suffix.
This inconsistency caused disabled packs to incorrectly show as having
updates available even when they were on the latest version.

Changes:
- Add normalizePackId utility to consistently remove version suffixes
- Apply normalization in refreshInstalledList and WebSocket updates
- Use the utility across conflict detection and node help modules
- Ensure pack version info is preserved in the object's ver field

This fixes the "Update Available" indicator incorrectly showing for
disabled packs that are already on the latest version.

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <[email protected]>

* feature: test code added

* test: packUtils test code added

* test: address PR review feedback for test
  improvements

  - Remove unnecessary .not.toThrow() assertion
  in useManagerQueue test
  - Add clarifying comments for version
  normalization test logic
  - Replace 'as any' with vi.mocked() for better
  type safety

---------

Co-authored-by: Claude <[email protected]>
@comfy-pr-bot comfy-pr-bot mentioned this pull request Sep 7, 2025
Merged
@comfy-pr-bot
Copy link
Member

@viva-jinyi Successfully backported to #5416

DrJKL
DrJKL approved these changes Sep 7, 2025
View reviewed changes
tests-ui/tests/utils/packUtils.test.ts
expect(normalizePackId('@1_0_0')).toBe('')

// Whitespace
expect(normalizePackId(' pack @1_0_0')).toBe(' pack ')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this what we want?
Do we allow for significant leading and trailing whitespace?

tests-ui/tests/utils/packUtils.test.ts
it('should handle edge cases', () => {
// Only @ symbol
expect(normalizePackId('@')).toBe('')
expect(normalizePackId('@1_0_0')).toBe('')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These feel like they should error out.
Wouldn't this put the item into the map keyed in empty string?

tests-ui/tests/utils/packUtils.test.ts
expect(normalizePackKeys(input)).toEqual(expected)
})

it('should work with different value types', () => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't we enforce that this is pack specific? When would we want to normalize the key to map to null?

tests-ui/tests/utils/packUtils.test.ts
})
})

describe('normalizePackKeys', () => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The duplication here makes me think that one of these methods should be private, probably the ID normalization, and that the public contract to test would just be this one.

tests-ui/tests/utils/packUtils.test.ts
// but there should only be one 'pack' key in the result
expect(Object.keys(result)).toEqual(['pack'])
expect(result.pack).toBeDefined()
expect(result.pack.ver).toBeDefined()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the last one wins this would be '3.0.0'?

@benceruleanlu benceruleanlu mentioned this pull request Sep 7, 2025
Merged
christian-byrne pushed a commit that referenced this pull request Sep 7, 2025
@comfy-pr-bot @viva-jinyi @claude
Fix version detection for disabled packs (#5395) (#5416)
b9881fa 
* fix: normalize pack IDs to fix version detection for disabled packs

When a pack is disabled, ComfyUI-Manager returns it with a version suffix
(e.g., "ComfyUI-GGUF@1_1_4") while enabled packs don't have this suffix.
This inconsistency caused disabled packs to incorrectly show as having
updates available even when they were on the latest version.

Changes:
- Add normalizePackId utility to consistently remove version suffixes
- Apply normalization in refreshInstalledList and WebSocket updates
- Use the utility across conflict detection and node help modules
- Ensure pack version info is preserved in the object's ver field

This fixes the "Update Available" indicator incorrectly showing for
disabled packs that are already on the latest version.

🤖 Generated with [Claude Code](https://claude.ai/code)



* feature: test code added

* test: packUtils test code added

* test: address PR review feedback for test
  improvements

  - Remove unnecessary .not.toThrow() assertion
  in useManagerQueue test
  - Add clarifying comments for version
  normalization test logic
  - Replace 'as any' with vi.mocked() for better
  type safety

---------

Co-authored-by: Jin Yi <[email protected]>
Co-authored-by: Claude <[email protected]>
@benceruleanlu benceruleanlu mentioned this pull request Sep 11, 2025
Merged
snomiao pushed a commit that referenced this pull request Sep 12, 2025
@viva-jinyi @claude @snomiao
Fix version detection for disabled packs (#5395)
2045277 
* fix: normalize pack IDs to fix version detection for disabled packs

When a pack is disabled, ComfyUI-Manager returns it with a version suffix
(e.g., "ComfyUI-GGUF@1_1_4") while enabled packs don't have this suffix.
This inconsistency caused disabled packs to incorrectly show as having
updates available even when they were on the latest version.

Changes:
- Add normalizePackId utility to consistently remove version suffixes
- Apply normalization in refreshInstalledList and WebSocket updates
- Use the utility across conflict detection and node help modules
- Ensure pack version info is preserved in the object's ver field

This fixes the "Update Available" indicator incorrectly showing for
disabled packs that are already on the latest version.

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <[email protected]>

* feature: test code added

* test: packUtils test code added

* test: address PR review feedback for test
  improvements

  - Remove unnecessary .not.toThrow() assertion
  in useManagerQueue test
  - Add clarifying comments for version
  normalization test logic
  - Replace 'as any' with vi.mocked() for better
  type safety

---------

Co-authored-by: Claude <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@DrJKL DrJKL DrJKL approved these changes

@arjansingh arjansingh arjansingh approved these changes

@christian-byrne christian-byrne christian-byrne approved these changes

@benceruleanlu benceruleanlu benceruleanlu left review comments

Assignees

@viva-jinyi viva-jinyi

Labels
1.26 area:manager needs-backport Fix/change that needs to be cherry-picked to the current feature freeze branch size:L This PR changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@viva-jinyi @comfy-pr-bot @DrJKL @arjansingh @christian-byrne @benceruleanlu