Skip to content

Conversation

@benceruleanlu
Copy link
Member

@benceruleanlu benceruleanlu commented Nov 22, 2025

Summary

  • during validation, recreate the expected ComfyUI directory structure under the configured basePath using ComfyConfigManager
  • mark basePath as error if the structure cannot be recreated, preventing launch with invalid paths
  • add unit tests covering the directory self-heal behavior

Context

User deleted the entire base path folder (~/Documents), leaving no user/input/output/custom_nodes/models dirs. The app still validated as installed and tried to launch with --user-directory /Users/ben/Documents/user, which failed with:

main.py: error: argument --user-directory: The path '/Users/ben/Documents/user' does not exist.

This change makes validation rebuild the directory structure and fail fast if it cannot, avoiding the runtime crash.

Testing

  • Unit tests added for directory self-heal in ComfyInstallation (lint/tsc via lint-staged)

@benceruleanlu benceruleanlu requested a review from a team as a code owner November 22, 2025 22:54
@dosubot dosubot bot added size:S This PR changes 10-29 lines, ignoring generated files. bug Something isn't working labels Nov 22, 2025
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.

How about tests?

@dosubot dosubot bot added size:M This PR changes 30-99 lines, ignoring generated files. and removed size:S This PR changes 10-29 lines, ignoring generated files. labels Nov 22, 2025
@dosubot dosubot bot added size:S This PR changes 10-29 lines, ignoring generated files. and removed size:M This PR changes 30-99 lines, ignoring generated files. labels Nov 22, 2025
@benceruleanlu benceruleanlu requested a review from a team as a code owner November 22, 2025 23:16
@dosubot dosubot bot added size:L This PR changes 100-499 lines, ignoring generated files. and removed size:S This PR changes 10-29 lines, ignoring generated files. labels Nov 22, 2025
@benceruleanlu
Copy link
Member Author

@codex review

Copy link

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment on lines +62 to +64
beforeEach(() => {
vi.clearAllMocks();
installation = new ComfyInstallation('installed', basePath, telemetry);

Choose a reason for hiding this comment

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

P1 Badge Reset mock implementation between tests

The second test replaces createComfyDirectories with a throwing implementation (lines 77‑80), but the beforeEach only calls vi.clearAllMocks (lines 62‑64), which clears call history without restoring implementations. As a result, the third test still uses the throwing mock and will fail before reaching isComfyDirectory, so the new suite is red as soon as the failing mock is set. Reset the mock implementation (e.g., mockReset/resetAllMocks) or reassign it per test so the later cases run with the intended behavior.

Useful? React with 👍 / 👎.

@benceruleanlu
Copy link
Member Author

This one isn't blocking for next release, so I'll draft until it's ready

@benceruleanlu benceruleanlu marked this pull request as draft November 22, 2025 23:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working size:L This PR changes 100-499 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants