src/macos/window_system.rs (1)

137-143: Consider using trace level for verbose window scan logging.

The debug logging for every window during each scan (every 200ms) could generate excessive log output in production environments with many windows.

-debug!("Window scan found {} windows", current_windows.len());
-for window in &current_windows {
-    debug!(
-        "Window: {} ({}), workspace: {}, rect: {:?}",
-        window.title, window.owner, window.workspace_id, window.rect
-    );
-}
+debug!("Window scan found {} windows", current_windows.len());
+for window in &current_windows {
+    trace!(
+        "Window: {} ({}), workspace: {}, rect: {:?}",
+        window.title, window.owner, window.workspace_id, window.rect
+    );
+}

llms.txt (3)

9-9: Remove redundant "Interface" from "CLI Interface".

The acronym CLI already stands for "Command-Line Interface", making "CLI Interface" redundant.

-- **CLI Interface** (`main.rs`): Command-line entry point
+- **CLI** (`main.rs`): Command-line entry point

🧰 Tools

🪛 LanguageTool

[style] ~9-~9: This phrase is redundant (‘I’ stands for ‘Interface’). Use simply “CLIInterface”.
Context: ...ssibility APIs. ## Key Components - CLI Interface (main.rs): Command-line entry point...

(ACRONYM_TAUTOLOGY)

---

17-17: Fix macOS capitalization.

The correct capitalization for Apple's operating system is "macOS", not "macos".

-- **macOS Integration** (`macos/`): Accessibility API bindings, Core Graphics window ops
+- **macOS Integration** (`macOS/`): Accessibility API bindings, Core Graphics window ops

🧰 Tools

🪛 LanguageTool

[grammar] ~17-~17: The operating system from Apple is written “macOS”.
Context: ...ng integration - macOS Integration (macos/): Accessibility API bindings, Core Gr...

(MAC_OS)

---

56-56: Add missing article "the".

-- Integrates with macOS window system through Accessibility bindings
+- Integrates with the macOS window system through Accessibility bindings

🧰 Tools

🪛 LanguageTool

[uncategorized] ~56-~56: You might be missing the article “the” here.
Context: ...for window operations - Integrates with macOS window system through Accessibility bin...

(AI_EN_LECTOR_MISSING_DETERMINER_THE)

src/macos/cgwindow.rs (2)

105-107: Consider using a more appropriate default for owner_pid.

Using -1 as a default for a process ID could be confusing since PIDs are typically non-negative. Consider using 0 or an Option<i32> instead.

-// Extract owner PID
-let owner_pid =
-    Self::get_number_from_dict(dict, "kCGWindowOwnerPID").unwrap_or(-1.0) as i32;
+// Extract owner PID
+let owner_pid =
+    Self::get_number_from_dict(dict, "kCGWindowOwnerPID").unwrap_or(0.0) as i32;

---

130-144: Consider using trace level for window filtering logs.

These debug logs could generate excessive output in production when many windows are being filtered. Consider using trace! level instead.

-debug!(
+trace!(
     "Filtering out window {} ({}): layer={}, on_screen={}, alpha={}",
     title, owner, layer, is_on_screen, alpha
 );

-debug!(
+trace!(
     "Filtering out small window {} ({}): {}x{}",
     title, owner, rect.width, rect.height
 );

src/macos/window_notifications.rs (2)

106-128: Handle potential class registration conflicts.

The hardcoded class name "WindowDragObserver" could cause registration failures if the class already exists. Consider using a unique class name or checking if the class exists first.

 unsafe fn create_observer_class(&self) -> Result<*const Class, Box<dyn std::error::Error>> {
+    // Check if class already exists
+    if let Some(existing_class) = Class::get("WindowDragObserver") {
+        return Ok(existing_class);
+    }
+    
     let superclass = class!(NSObject);
     let mut decl = objc::declare::ClassDecl::new("WindowDragObserver", superclass)
         .ok_or("Failed to create class declaration")?;

---

171-173: Consider propagating channel send errors.

Failed event sends are only logged as warnings. If the channel is full or closed, drag events will be lost silently, potentially causing the UI to be out of sync.

Consider using send().await with proper async handling or increasing the channel buffer size to reduce the likelihood of dropped events. Alternatively, implement a retry mechanism or error callback to handle send failures more gracefully.

Also applies to: 217-219

src/snap.rs (3)

56-63: Remove unused window_id field.

The window_id field in WindowDragState is marked with #[allow(dead_code)] and appears to be unused. Since the window ID is already used as the HashMap key, storing it again in the value is redundant.

 #[derive(Debug, Clone)]
 struct WindowDragState {
-    #[allow(dead_code)]
-    window_id: WindowId,
     initial_rect: Rect,
     is_dragging: bool,
     drag_start_time: std::time::Instant,
 }

Also update the initialization:

 self.window_drag_states.insert(
     window_id,
     WindowDragState {
-        window_id,
         initial_rect: current_rect,
         is_dragging: true,
         drag_start_time: std::time::Instant::now(),
     },
 );

---

82-87: Consider making snap zone sizes configurable.

The hardcoded zone sizes (150px for edges, 100px for corners) may not work well across different screen sizes and resolutions. Consider making these values configurable or scale them based on screen dimensions.

 fn update_snap_zones(&mut self, screen_rect: Rect) {
     self.snap_zones.clear();

-    let edge_zone_width = 150.0; // Wider edge zones for easier targeting
-    let corner_size = 100.0; // Corner zones at edges
+    // Scale zone sizes based on screen dimensions
+    let edge_zone_width = (screen_rect.width * 0.1).min(150.0);
+    let corner_size = (screen_rect.width * 0.06).min(100.0);
     let margin = 8.0; // Small margin from screen edges

---

307-309: Make drag thresholds configurable.

The hardcoded minimum drag time (100ms) might not suit all users' preferences. Some users might want more responsive snapping while others might prefer more deliberate gestures.

Consider adding these as configurable fields:

 pub struct SnapManager {
     screen_rect: Rect,
     snap_zones: Vec<SnapZone>,
     snap_threshold: f64,
+    min_drag_time_ms: u128,
+    min_drag_distance: f64,
     window_drag_states: HashMap<WindowId, WindowDragState>,
 }

And in the drag handling:

-let min_time_ms = 100u128; // 100ms minimum drag time
-let min_distance = self.snap_threshold; // Use snap_threshold directly for distance
+let min_time_ms = self.min_drag_time_ms;
+let min_distance = self.min_drag_distance;

src/layout.rs (1)

149-157: Consider BSP tree balance for performance.

The spiral insertion pattern always inserts into the rightmost position, which can create highly unbalanced trees. With many windows, this could lead to O(n) depth and impact performance.

Consider implementing a rebalancing mechanism or using a different insertion strategy that maintains better balance. Alternatively, document the expected maximum number of windows to ensure performance remains acceptable.

src/window_manager.rs (4)

108-109: Consider making the snap threshold configurable.

The 50px snap threshold is hardcoded. Consider adding this to the Config struct to allow users to customize snap sensitivity.

-        let snap_manager = SnapManager::new(screen_rect, 50.0); // 50px snap threshold
+        let snap_threshold = config.snap.threshold.unwrap_or(50.0);
+        let snap_manager = SnapManager::new(screen_rect, snap_threshold);

---

147-153: Good documentation of timer intervals with actionable TODO.

The detailed explanation of the refresh and monitor intervals is helpful. The TODO clearly identifies what needs to be configurable.

Would you like me to implement the configurable intervals by adding window_refresh_interval_ms and window_monitor_interval_ms to the config structure?

---

502-502: Consider making movement thresholds configurable.

The method uses two hardcoded thresholds:

• 20px for detecting "significant" moves (line 502)
• 10px for avoiding redundant snap moves (line 539)

Consider making these configurable for different user preferences and display densities.

Also applies to: 539-539

---

578-719: Consider extracting common swap logic to reduce duplication.

Both swap_windows and swap_windows_with_rects share similar bulk move and fallback logic. Consider extracting the common pattern.

+    async fn perform_window_swap(&mut self, swap_layouts: HashMap<WindowId, Rect>, windows: Vec<Window>) -> Result<()> {
+        // Mark windows as programmatic moves
+        for window_id in swap_layouts.keys() {
+            self.programmatically_moving.insert(*window_id);
+        }
+        
+        // Try bulk move first
+        match self.macos.move_all_windows(&swap_layouts, &windows).await {
+            Ok(_) => {
+                debug!("✅ Successfully swapped windows using bulk move");
+                // Update internal state
+                for (window_id, rect) in swap_layouts {
+                    if let Some(w) = self.windows.get_mut(&window_id) {
+                        w.rect = rect;
+                    }
+                    self.previous_window_positions.insert(window_id, rect);
+                }
+                Ok(())
+            }
+            Err(e) => {
+                warn!("Bulk swap failed, trying individual moves: {}", e);
+                // Fallback to individual moves
+                for (window_id, rect) in swap_layouts {
+                    match self.macos.move_window(window_id, rect).await {
+                        Ok(_) => {
+                            if let Some(w) = self.windows.get_mut(&window_id) {
+                                w.rect = rect;
+                            }
+                            self.previous_window_positions.insert(window_id, rect);
+                        }
+                        Err(e) => warn!("Failed to move window {:?} during swap: {}", window_id, e),
+                    }
+                }
+                Ok(())
+            }
+        }
+    }

src/macos/accessibility.rs (1)

782-782: Consider making position matching tolerance configurable.

The 5-pixel tolerance for window position matching is hardcoded. Different display configurations might benefit from different tolerances.

Consider adding a constant or configuration option:

+const WINDOW_POSITION_MATCH_TOLERANCE: f64 = 5.0;
+
-                        if dx < 5.0 && dy < 5.0 && dw < 5.0 && dh < 5.0 {
+                        if dx < WINDOW_POSITION_MATCH_TOLERANCE && dy < WINDOW_POSITION_MATCH_TOLERANCE && 
+                           dw < WINDOW_POSITION_MATCH_TOLERANCE && dh < WINDOW_POSITION_MATCH_TOLERANCE {

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

src/macos/window_system.rs (2)

11-15: Private API usage already flagged in previous review.

---

124-129: Monitoring interval configuration already flagged in previous review.

src/macos/window_notifications.rs (1)

275-292: Add error handling for missing running application.

The function may return None when runningApplication is not available, but this could indicate a legitimate system state rather than an error.

Consider adding debug logging when PID cannot be retrieved:

 unsafe fn get_window_owner_pid(window: id) -> Option<i32> {
     let window_number: i32 = msg_send![window, windowNumber];
     if window_number <= 0 {
         return None;
     }

     // Use NSRunningApplication to get the PID from the window's owning app
     let app: id = msg_send![window, app];
     if app != nil {
         let running_app: id = msg_send![app, runningApplication];
         if running_app != nil {
             let pid: i32 = msg_send![running_app, processIdentifier];
             return Some(pid);
+        } else {
+            debug!("No running application found for window {}", window_number);
         }
+    } else {
+        debug!("No app found for window {}", window_number);
     }

     None
 }

src/window_manager.rs (2)

103-133: Consider error handling for drag observer initialization.

The drag observer setup properly handles initialization errors, but consider what happens if the observer fails during runtime.

Add monitoring for observer health:

         let mut drag_observer = WindowDragNotificationObserver::new(drag_event_tx);
         drag_observer
             .start_observing()
             .map_err(|e| anyhow::anyhow!("Failed to start drag observer: {}", e))?;
+        
+        // TODO: Add periodic health check for drag observer
+        // Consider restarting observer if it becomes unresponsive

---

1101-1124: Remove or implement placeholder method.

The update_layout_for_manual_move method is marked as dead code and contains only placeholder comments. Either implement it or remove it.

-    #[allow(dead_code)]
-    async fn update_layout_for_manual_move(
-        &mut self,
-        window_id: WindowId,
-        new_rect: Rect,
-    ) -> Result<()> {
-        // For now, we'll just apply the existing layout logic
-        // In a more sophisticated implementation, we might update the BSP tree
-        // to reflect the manual positioning
-        debug!(
-            "Window {:?} manually moved to {:?}, updating layout",
-            window_id, new_rect
-        );
-
-        // You could implement logic here to:
-        // 1. Remove the window from its current position in the BSP tree
-        // 2. Find where it should be placed based on its new position
-        // 3. Rebuild the tree structure accordingly
-
-        // For now, just ensure the layout is consistent
-        self.apply_layout().await?;
-
-        Ok(())
-    }

Do you want me to help implement the BSP tree update logic or remove this placeholder method?

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

src/layout.rs (1)

185-185: Ensure MSRV ≥ 1.82.0 and declare it in Cargo.toml.

Your code uses newer Rust features:

• is_some_and (lines 185, 227-232) requires Rust 1.70.0+
• is_none_or (line 200-201) requires Rust 1.82.0+

Without declaring the MSRV, users with older Rust versions will encounter compilation errors.

Add to your Cargo.toml under [package]:

rust-version = "1.82.0"

#!/bin/bash
# Check current MSRV declaration
grep -E "rust-version|edition" Cargo.toml

Also applies to: 200-201, 227-232

src/window_manager.rs (1)

575-575: Make distance thresholds configurable.

Multiple hardcoded distance thresholds (0.5px, 10.0px) are used throughout the code. These should be configurable for different display characteristics and user preferences.

Consider adding threshold configuration:

pub struct WindowConfig {
    pub movement_detection_threshold: f64,  // Default: 0.5
    pub snap_detection_threshold: f64,      // Default: 10.0
    // ... other fields
}

Also applies to: 589-589, 694-694

src/main.rs (2)

97-99: Make log file path configurable.

The log file path is hardcoded to skew.log in the current directory. This should be configurable and respect standard logging conventions.

Consider making the log file path configurable:

-let log_file = std::env::current_dir()
-    .unwrap_or_else(|_| PathBuf::from("."))
-    .join("skew.log");
+let log_file = std::env::var("SKEW_LOG_FILE")
+    .map(PathBuf::from)
+    .unwrap_or_else(|_| {
+        std::env::current_dir()
+            .unwrap_or_else(|_| PathBuf::from("."))
+            .join("skew.log")
+    });

---

33-47: Consider log rotation and file size management.

The current implementation appends indefinitely to the log file without any rotation or size limits, which could lead to very large log files over time.

Consider implementing log rotation features:

• Maximum file size limits
• Rotating to numbered backup files
• Configurable retention policies

You might also consider using established logging crates like tracing-appender or log4rs that provide these features out of the box.

src/snap.rs (4)

307-307: Make minimum drag distance configurable.

The minimum drag distance of 20.0 pixels is hardcoded. This should be configurable as user preferences for drag sensitivity vary.

Consider making this configurable:

-let min_distance = 20.0; // Fixed minimum distance - simpler than using snap_threshold
+let min_distance = self.snap_threshold * 0.4; // Scale with snap threshold

Or add it as a separate configurable parameter.

---

352-352: Consider floating-point precision for snap detection.

The hardcoded threshold of 10.0 for snap detection could be problematic on high-DPI displays or with floating-point precision issues.

Consider using a relative threshold:

-if dx > 10.0 || dy > 10.0 || dw > 10.0 || dh > 10.0 {
+let epsilon = 1.0; // Or make configurable
+if dx > epsilon || dy > epsilon || dw > epsilon || dh > epsilon {

---

92-216: Simplify zone configuration with a data structure.

The zone configuration is complex and repetitive. Consider using a more structured approach to define zones.

Consider using a more structured approach:

struct ZoneConfig {
    region: SnapRegion,
    bounds_fn: fn(Rect, f64, f64, f64) -> (f64, f64, f64, f64),
    snap_fn: fn(Rect, f64) -> (f64, f64, f64, f64),
}

const ZONE_CONFIGS: &[ZoneConfig] = &[
    ZoneConfig {
        region: SnapRegion::Center,
        bounds_fn: |screen, _, _, _| (0.2, 0.2, 0.6, 0.6),
        snap_fn: |screen, _| (0.25, 0.25, 0.5, 0.5),
    },
    // ... other zones
];

This would reduce duplication and make the zone definitions more maintainable.

---

58-59: Remove unused field warning.

The window_id field in WindowDragState is marked with #[allow(dead_code)] but could be useful for debugging or validation.

Consider either using this field for validation or removing it entirely:

struct WindowDragState {
-    #[allow(dead_code)]
-    window_id: WindowId,
    initial_rect: Rect,
    is_dragging: bool,
    drag_start_time: std::time::Instant,
}

src/layout.rs (3)

293-296: Validate window size constants against screen dimensions.

The hardcoded minimum and maximum window sizes may not be appropriate for all screen sizes, especially smaller displays or very large monitors.

Consider making these relative to screen size or configurable:

-const MIN_WINDOW_WIDTH: f64 = 150.0;  // Reduced for better space usage
-const MIN_WINDOW_HEIGHT: f64 = 100.0; // Reduced for better space usage
-const MAX_WINDOW_WIDTH: f64 = 5000.0; // Increased for larger monitors
-const MAX_WINDOW_HEIGHT: f64 = 4000.0; // Increased for larger monitors
+fn get_window_constraints(screen_rect: Rect) -> (f64, f64, f64, f64) {
+    let min_width = (screen_rect.width * 0.1).max(150.0);
+    let min_height = (screen_rect.height * 0.1).max(100.0);
+    let max_width = screen_rect.width * 0.9;
+    let max_height = screen_rect.height * 0.9;
+    (min_width, min_height, max_width, max_height)
+}

---

145-181: Consider BSP tree rebalancing for better space utilization.

The spiral insertion pattern always inserts into the rightmost/bottommost position, which can create unbalanced trees over time, leading to poor space utilization.

Consider implementing a rebalancing mechanism or alternative insertion strategies:

fn insert_window_balanced(&mut self, window_id: WindowId, split_ratio: f64) {
    // Find the leaf with the largest area for splitting
    // Or implement periodic tree rebalancing
}

This would help maintain better window size distribution, especially with many windows.

---

377-398: Optimize BSP layout computation.

The BSP layout always rebuilds the entire tree from scratch, which could be inefficient for large numbers of windows.

Consider implementing incremental updates:

fn update_bsp_tree_incrementally(&mut self, added: &[WindowId], removed: &[WindowId]) {
    // Only modify affected parts of the tree
    // This would be more efficient than full rebuilds
}

However, the current approach is simpler and more reliable, so this optimization should only be considered if performance becomes an issue.

src/window_manager.rs (2)

82-92: Consider using more descriptive field names.

The boolean tracking fields could be more descriptive to clarify their purpose.

Consider renaming for clarity:

-programmatically_moving: std::collections::HashSet<WindowId>,
-user_dragging_windows: std::collections::HashSet<WindowId>,
+windows_being_moved_programmatically: std::collections::HashSet<WindowId>,
+windows_being_dragged_by_user: std::collections::HashSet<WindowId>,

---

1172-1198: Add exponential backoff for retry logic.

The retry logic uses a fixed 50ms delay between attempts. Consider implementing exponential backoff for better resilience.

-tokio::time::sleep(tokio::time::Duration::from_millis(50)).await;
+let delay = 50 * (1 << attempt); // Exponential backoff: 50ms, 100ms, 200ms
+tokio::time::sleep(tokio::time::Duration::from_millis(delay)).await;

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro