Make Active Monitors list clickable to toggle individual monitors

[pirate]

Summary

• Added clickable toggle functionality to Active Monitors list in menubar
• Enabled monitors show with ✓ in green, disabled with ○ in gray
• Users can now toggle monitors on/off directly from menubar without opening xbar preferences

Fixes #56

🤖 Generated with Claude Code

[claude]

Code Review for PR #57

Thank you for this contribution! This PR adds a much-needed feature for toggling monitors directly from the menubar. Here's my comprehensive review:

✅ Positive Aspects

1. Clean Implementation: The toggle functionality is well-structured with separate functions for concerns (get/set overrides, checking enabled state)
2. Good UX Design: The visual feedback (✓ in green for enabled, ○ in gray for disabled) makes the state immediately clear
3. Proper File Organization: Using monitor_overrides.json in the state directory is a good approach for persistent user preferences
4. Non-Breaking: The changes maintain backward compatibility with existing environment variable configuration

🐛 Potential Bugs & Issues

1. Race Condition in toggle_monitor() (security-growler.30s.py:71-88)

The read-modify-write operation is not atomic. If multiple toggle operations happen simultaneously (unlikely but possible with rapid clicks), the last write could override others:

# Current implementation
overrides = get_monitor_overrides()  # Read
overrides[monitor_name] = not current_state  # Modify
with open(overrides_file, "w") as f:  # Write
    json.dump(overrides, f, indent=2)

Recommendation: While unlikely to be an issue in practice for this use case, consider adding file locking if this becomes a problem. For now, this is acceptable.

2. Error Handling Missing in toggle_monitor() (security-growler.30s.py:71-88)

If the file write fails (permissions, disk full, etc.), the toggle silently fails with no user feedback:

with open(overrides_file, "w") as f:
    json.dump(overrides, f, indent=2)

Recommendation: Add try-except around the write operation to handle IOError/OSError.

3. Edge Case: Malformed Monitor Names (security-growler.30s.py:1702-1706)

The main() function doesn't validate the monitor name passed via command line:

if len(sys.argv) > 2:
    monitor_name = sys.argv[2]
    toggle_monitor(monitor_name)

An attacker could potentially inject arbitrary keys into the overrides JSON. While this doesn't pose a direct security risk (it just creates extra keys), it could cause confusion.

Recommendation: Validate that monitor_name is in the expected set of monitor variables before toggling.

4. F-String Complexity (security-growler.30s.py:1626)

This line is very complex and hard to read:

("MONITOR_PORTS", MONITOR_PORTS, f"Ports: {', '.join(str(p) for p in PORTS_TO_MONITOR[:3])}{f' (+{len(PORTS_TO_MONITOR) - 3})' if len(PORTS_TO_MONITOR) > 3 else ''}"),

Recommendation: Extract the display name logic to a helper function for better readability.

⚡ Performance Considerations

1. File I/O on Every Render (security-growler.30s.py:59-69)
   get_monitor_overrides() is called 13+ times per render (once per monitor via is_monitor_enabled()). Each call reads and parses JSON from disk:

# Called repeatedly
def is_monitor_enabled(monitor_name: str, env_default: str = "true") -> bool:
    env_value = os.environ.get(monitor_name, env_default).lower() == "true"
    overrides = get_monitor_overrides()  # ← Reads file every time
    return overrides.get(monitor_name, env_value)

Impact: Low (the file is tiny, ~1KB max), but this is inefficient design.

Recommendation: Cache the overrides at module level or pass them as a parameter to avoid redundant I/O:

_OVERRIDES_CACHE = None

def get_monitor_overrides() -> Dict[str, bool]:
    global _OVERRIDES_CACHE
    if _OVERRIDES_CACHE is None:
        # Load from file
        _OVERRIDES_CACHE = {...}
    return _OVERRIDES_CACHE

🔒 Security Concerns

1. File Permissions (security-growler.30s.py:62, 74)
   The code creates the state directory with default permissions:

STATE_DIR.mkdir(parents=True, exist_ok=True)

Concern: On multi-user systems, this could allow other users to read/modify the overrides file if umask is permissive.

Recommendation: Explicitly set restrictive permissions (0o700 for directory, 0o600 for file) after creation.

2. Path Traversal (Very Low Risk) (security-growler.30s.py:1618)
   os.path.abspath(__file__) is used in the xbar command. While file is controlled by the system, not user input, it's worth noting.

Status: Not a real concern, but worth documenting.

🧪 Test Coverage

Observation: No test files exist in the repository.

Recommendations:

1. Add unit tests for the new toggle functionality:
   • Test get_monitor_overrides() with missing/corrupted files
   • Test toggle_monitor() toggling on/off
   • Test is_monitor_enabled() with various env var and override combinations
2. Add integration tests for the command-line toggle interface
3. Consider adding tests for the xbar output format

📝 Code Quality Suggestions

1. Type Hints: Excellent use of type hints throughout (security-growler.30s.py:59, 71, 90)
2. Documentation: Good docstrings on all new functions
3. Consistency: Code style matches the existing codebase well

🎯 Summary

Overall Assessment: This is a well-implemented feature that solves the stated problem effectively. The code is clean, readable, and integrates well with the existing architecture.

Priority Fixes:

• HIGH: Add error handling to toggle_monitor() for write failures
• MEDIUM: Add input validation for monitor names in main()
• MEDIUM: Set explicit file permissions for security on multi-user systems
• LOW: Cache overrides to reduce redundant file I/O
• LOW: Extract complex display name logic for readability

Recommendation: ✅ Approve with minor suggestions. The high-priority issues are nice-to-haves rather than blockers. The PR is functional and safe to merge as-is, but would benefit from the suggested improvements.

---

Note: This PR has already been merged. These suggestions can be considered for future improvements.