fix: Bundle fonts and patch Comfyroll for NixOS compatibility

[jamesbrink]

Summary

• Bundle fonts (DejaVu, Liberation, Noto, Roboto) from nixpkgs for custom nodes that require font files
• Auto-patch ComfyUI_Comfyroll_CustomNodes at startup to use bundled fonts instead of hardcoded /usr/share/fonts/truetype
• Fix ComfyUI-Manager config path (moved from user/default/ComfyUI-Manager to user/__manager/)
• Add Known Issues section to README documenting restart requirement after installing patched nodes

Test plan

• Install Comfyroll Studio via Manager
• Full restart (Ctrl+C + nix run)
• Verify "Patched ComfyUI_Comfyroll_CustomNodes for NixOS font compatibility" message appears
• Verify CR Select Font node loads without /usr/share/fonts/truetype error
• Verify fonts directory created in data dir with TTF symlinks

Fixes #17

🤖 Generated with Claude Code

[claude]

Code Review

Thanks for tackling this NixOS font compatibility issue! Overall this is a solid solution that provides a pragmatic workaround for hardcoded paths in third-party custom nodes. Here's my detailed feedback:

✅ Strengths

1. Clean Nix Implementation: The bundledFonts derivation using symlinkJoin is idiomatic and provides a good selection of common fonts (DejaVu, Liberation, Noto, Roboto).

2. Good Font Coverage: The font selection covers most common use cases - sans-serif, serif, and monospace variants.

3. Well-Documented: The README's "Known Issues" section clearly documents the restart requirement and provides actionable instructions.

4. Idempotent Patching: The grep -q check before patching prevents re-patching on subsequent runs.

5. Proper Fix Discovery: Updated the ComfyUI-Manager config path to the new location (user/__manager/).

🔍 Issues & Suggestions

1. Race Condition in Font Symlinking (Minor)

Location: nix/packages.nix:326-334

The glob pattern expansion might fail if no TTF files exist:

for font_file in "${bundledFonts}"/ttf/*.ttf "${bundledFonts}"/ttf/*.TTF; do

Issue: If no files match, the loop will iterate over the literal string "${bundledFonts}/ttf/*.ttf".

Suggested Fix:

shopt -s nullglob  # At the top of the script
# ... or use find instead:
find "${bundledFonts}/ttf" -name "*.ttf" -o -name "*.TTF" 2>/dev/null | while read -r font_file; do
  filename=$(basename "\")
  target="\/\"
  if [[ \! -e "\" ]]; then
    ln -sf "\" "\"
  fi
done

2. Brittle String Patching (Moderate)

Location: nix/packages.nix:341

sed -i "s|\"/usr/share/fonts/truetype\"|\"\\"|g" "\"

Issues:

• Hardcodes the exact string format with double quotes - fails if upstream changes to single quotes or f-strings
• No validation that the patch actually worked
• Could accidentally patch comments or strings in unexpected contexts

Suggested Improvements:

# More flexible pattern matching (handles both quote styles)
if sed -i "s|[\"\']/usr/share/fonts/truetype[\"\']|\"\\"|g" "\"; then
  echo "Patched ComfyUI_Comfyroll_CustomNodes for NixOS font compatibility"
else
  echo "Warning: Failed to patch ComfyUI_Comfyroll_CustomNodes" >&2
fi

Alternatively, consider a Python-based patch that modifies the AST for more robustness.

3. Missing Font Validation (Minor)

Location: nix/packages.nix:19-33

The postBuild script silences errors with 2>/dev/null || true, which could hide genuine issues.

Suggested Improvement:

postBuild = ''
  mkdir -p \/ttf
  # Only symlink .ttf files, log any issues
  find \/share/fonts -type f \( -name "*.ttf" -o -name "*.TTF" \) -exec ln -sf {} \/ttf/ \;
  
  # Validate we actually got some fonts
  if [ -z "$(ls -A \/ttf)" ]; then
    echo "Warning: No TTF fonts found in bundled fonts" >&2
    exit 1
  fi
'';

4. Potential Case-Sensitivity Issues (Minor)

The code handles both .ttf and .TTF extensions, but the patched Python code calls .lower().endswith(".ttf"), so uppercase extensions would still work. The dual handling is fine but slightly redundant.

5. Documentation: Suggest Preventive Approach (Enhancement)

Location: README.md:100

Consider adding a note about the root cause and suggesting users file upstream issues:

**Why this happens**: These nodes use hardcoded paths that assume traditional Linux filesystem layouts. We're working with upstream maintainers to make paths configurable.

**Help improve this**: If you encounter other custom nodes with hardcoded paths, please report them in our issue tracker.

🔒 Security Considerations

✅ No security issues identified. The approach is safe because:

• Fonts are sourced from trusted nixpkgs packages
• Symlinks are created in user-controlled directories
• No execution of untrusted code
• The sed replacement doesn't introduce injection risks

🎯 Performance Considerations

✅ Minimal performance impact:

• Font symlinking happens once at startup (idempotent)
• Pattern matching with grep -q is fast and short-circuits
• No runtime overhead after patching

🧪 Test Coverage Suggestions

Consider adding integration tests:

1. Verify bundledFonts derivation contains expected font files
2. Test that font directory is created with correct symlinks
3. Validate the patch is applied correctly (perhaps with a mock file)
4. Test behavior when Comfyroll node isn't installed (should skip gracefully)

📋 Nit: Code Style

1. Consistency: The comment style is good, but consider using `#``` blocks for multi-line explanations in shell scripts for better readability.

2. Variable Quoting: Good job on consistent quoting of variables ("\").

✅ Summary

This is a pragmatic solution to a real NixOS compatibility issue. The implementation is mostly solid with good documentation. The main improvements would be:

1. Hardening the glob expansion to handle edge cases
2. Making the string patching more robust (or switching to a more structured approach)
3. Adding validation for critical operations

Recommendation: ✅ Approve with minor suggestions. The current implementation works and solves the immediate problem. The suggested improvements are optimizations that can be addressed in follow-up PRs if needed.

Great work on documenting the Known Issues section - this will save users a lot of troubleshooting time!