Skip to content

Conversation

@Helmi
Copy link
Owner

@Helmi Helmi commented Aug 10, 2025

Summary

Changes Made

Core Refactoring

  • Unified project structure: Eliminated duplication between _auto_discovered.projects and project_names.replacements by consolidating into a single projects section with rich metadata (folder, display_name, pronunciation)
  • Migration logic: Added migrate_replacements_format() function to automatically convert old configurations to new format on first load
  • Updated functions: Modified all 5 replacement functions to use the new structure:
    • load_replacements() - Now includes migration logic and returns new default structure
    • auto_add_project_to_replacements() - Writes to single projects section
    • get_project_pronunciation() - Reads from projects[name].pronunciation
    • apply_project_name_replacement() - Uses projects[name].display_name
    • apply_command_replacement() - Accesses commands and patterns directly

Default Configuration

  • Added comprehensive default commands (ls, cd, rm, mkdir, npm, uvx)
  • Added useful pattern replacements for common development commands
  • Improved TTS pronunciation for technical terms

Documentation

  • Updated README.md with new configuration structure example
  • Added migration note for existing users
  • Provided clear examples of each configuration section

Testing Performed

  • ✅ Python syntax validation passed
  • ✅ Code formatted with black
  • ✅ Migration logic handles both old formats (_auto_discovered and project_names.replacements)
  • ✅ Backward compatibility maintained through automatic migration

Migration Path

Existing users' configurations will be automatically migrated when ccnotify loads the replacements file for the first time after this update. The migration preserves all existing data while reorganizing it into the cleaner structure.

Example of New Structure

{
  "projects": {
    "ccnotify": {
      "folder": "-Users-helmi-code-ccnotify",
      "display_name": "CCNotify",
      "pronunciation": "CC notify"
    }
  },
  "commands": {
    "ls": "list",
    "rm": "remove"
  },
  "patterns": [
    {
      "pattern": "npm run (\\w+)",
      "replacement": "N P M run {1}"
    }
  ]
}

- Consolidate project storage to single 'projects' section with rich metadata
- Add automatic migration from old format to new format
- Update all replacement functions to use new unified structure
- Add comprehensive default commands and patterns
- Update README documentation with new configuration format

Closes #6
@claude
Copy link

claude bot commented Aug 10, 2025

Claude finished @Helmi's task —— View job


PR Review: Refactor replacements.json structure

Summary

This PR successfully refactors the replacements.json structure to eliminate duplication by consolidating the dual structure into a single unified projects section. The implementation includes automatic migration logic and maintains backward compatibility.

Strengths

Migration Logic

  • Robust migration: The migrate_replacements_format() function (notify.py:446-500) properly handles both old formats:
    • _auto_discovered.projects structure
    • project_names.replacements structure
  • Data preservation: Migration preserves all existing data while reorganizing into the new structure
  • Automatic detection: Migration triggers automatically on first load when old format is detected
  • Default values: Adds sensible defaults for commands and patterns if missing

Code Quality

  • Consistent patterns: Follows existing codebase patterns with proper error handling and logging
  • Clean API: The new unified structure simplifies the replacement functions:
    • get_project_pronunciation() - reads from projects[name].pronunciation
    • apply_project_name_replacement() - uses projects[name].display_name
    • auto_add_project_to_replacements() - writes to single projects section
  • Documentation: Well-documented functions with clear variable names

Structure Improvements

  • Eliminates duplication: Consolidates project storage from dual structure to single projects section with rich metadata
  • Better organization: New structure is more intuitive: {projects: {}, commands: {}, patterns: []}
  • Rich metadata: Projects now store folder, display_name, and pronunciation in one place

⚠️ Potential Issues & Recommendations

Migration Edge Cases

# notify.py:464 - Potential data loss scenario
if proj_name not in new_data["projects"]:
    new_data["projects"][proj_name] = {
        "folder": None,  # Will be filled on next auto-discovery
        "display_name": proj_name,
        "pronunciation": pronunciation,
    }

Issue: If a project exists in project_names.replacements but not in _auto_discovered.projects, the folder field becomes None. This could break folder-based lookups until the next auto-discovery.

Recommendation: Add validation or fallback logic to handle missing folder information more gracefully.

Error Handling in Auto-Add Function

# notify.py:308-309 - Case-insensitive matching could be improved
exists = any(project_name.lower() == key.lower() for key in projects.keys())

Issue: Uses linear search through all project keys for case-insensitive matching, which could be slow with many projects.

Recommendation: Consider using a case-insensitive set or normalized key lookup for better performance.

Pattern Matching Concerns

# notify.py:583-589 - Potential regex injection
match = re.search(pattern, command, re.IGNORECASE)
if match:
    result = replacement  
    for i, group in enumerate(match.groups(), 1):
        result = result.replace(f"${i}", group)

Issue: User-provided patterns are used directly in re.search() without validation, potentially allowing regex injection.

Recommendation: Add pattern validation or use re.escape() for user inputs that shouldn't be treated as regex.

📊 Performance & Security

Performance

  • Migration overhead: Migration runs only once on first load after update - acceptable performance impact
  • File I/O: Multiple file reads/writes during migration, but this is a one-time cost
  • Memory usage: Reasonable memory footprint for typical project configurations

Security

  • File permissions: No changes to file permission handling
  • Input validation: Most user inputs are properly sanitized, except for regex patterns (see issue above)
  • No privilege escalation: Changes don't affect system-level operations

🧪 Test Coverage

Existing Tests

  • ✅ Installation scenarios covered in test_installation_scenarios.py
  • ✅ Installer logic tested in various test_*.py files
  • ✅ CI/CD setup with black formatting and basic testing

Missing Test Coverage

  • ⚠️ No specific migration tests: No unit tests for migrate_replacements_format() function
  • ⚠️ Edge case testing: Missing tests for mixed old/new format scenarios
  • ⚠️ Error condition testing: No tests for migration failure scenarios

Recommendations for Additional Tests:

# Suggested test cases
def test_migration_from_auto_discovered_only():
    # Test migration when only _auto_discovered exists

def test_migration_from_project_names_only(): 
    # Test migration when only project_names.replacements exists

def test_migration_data_preservation():
    # Verify all data is preserved during migration

def test_migration_edge_cases():
    # Test malformed input, missing fields, etc.

🎯 Overall Assessment

Score: 8.5/10 - This is a well-executed refactoring with solid implementation.

Ready for Merge

  • Migration logic is sound and preserves data integrity
  • Code quality meets project standards
  • Backward compatibility maintained through automatic migration
  • No breaking changes for existing users

Suggested Improvements (Non-blocking)

  1. Add specific unit tests for migration logic
  2. Improve error handling for edge cases in migration
  3. Add pattern validation for regex security
  4. Optimize case-insensitive project lookups

This refactoring successfully eliminates code duplication while maintaining stability and user experience. The automatic migration ensures seamless updates for existing users.

@Helmi Helmi merged commit 4bc2f0b into main Aug 10, 2025
3 checks passed
@Helmi Helmi deleted the feature/refactor-replacements-structure branch August 10, 2025 10:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Refactor replacements.json structure to eliminate duplication

2 participants