README.md (1)

234-234: Minor wording nit – consider hyphenating “i3-compatible”.

Purely stylistic, but “i3-compatible” is the more common form.

- A Wayland compositor and i3 compatible window manager for Linux
+ A Wayland compositor and i3-compatible window manager for Linux

src/plugins.rs (1)

34-37: Avoid suppressing dead_code; mark unused fields explicitly.

Using #[allow(dead_code)] at field level hides potentially useful warnings project-wide.
If the fields are intentionally unused (for now), prefix them with an underscore instead; that keeps the intent clear without a blanket allow.

-    #[allow(dead_code)]
-    name: String,
-    #[allow(dead_code)]
-    script_path: PathBuf,
+    _name: String,
+    _script_path: PathBuf,

src/macos/mod.rs (1)

3-3: Export order nit.

Very minor: keeping the module list alphabetically sorted (accessibility, cgwindow, window_notifications, window_system) is helpful for quick scans in large crates.
No action needed if you don’t enforce alphabetical ordering elsewhere.

src/config.rs (1)

308-309: Good idiomatic change; one edge-case still slips through.

parts.is_empty() is clearer than len() < 1 – nice.
However, a combo like "alt+" yields ["alt", ""]; it passes this check but contains an empty key segment.

Consider additionally validating that the final part (actual key) is non-empty:

- if parts.is_empty() {
+ if parts.is_empty() || parts.last().unwrap().is_empty() {

That keeps malformed combos from silently entering the config.

src/main.rs (2)

33-82: Well-implemented dual logging system with room for optimization.

The DualLogger implementation provides comprehensive logging with timestamps and dual output. The design is solid with proper thread safety and buffering.

Consider these potential optimizations:

• Flushing after every log entry (line 72) may impact performance under high log volume. Consider batched flushing or configurable flush intervals.
• Silent error handling for file writes could mask storage issues. Consider logging write failures to stderr.

---

97-101: Consider using a standard log directory location.

The log file is created in the current working directory. Consider using a standard location like ~/.local/share/skew/ or /tmp/ for better system integration.

-    let log_file = std::env::current_dir()
-        .unwrap_or_else(|_| PathBuf::from("."))
-        .join("skew.log");
+    let log_file = PathBuf::from(std::env::var("HOME").unwrap_or_else(|_| ".".to_string()))
+        .join(".local/share/skew")
+        .join("skew.log");
+    
+    // Ensure directory exists
+    if let Some(parent) = log_file.parent() {
+        std::fs::create_dir_all(parent)?;
+    }

src/macos/window_system.rs (1)

124-128: Address the TODO comment for configurable window monitoring interval.

The TODO indicates this should be configurable via skew.toml. This would allow users to balance between responsiveness and CPU usage based on their needs.

Would you like me to implement the configuration option for window_monitor_interval_ms in the config system and open an issue to track this enhancement?

src/macos/window_notifications.rs (1)

422-439: Window owner PID extraction may fail silently.

The method attempts to get PID through window.app.runningApplication, but this chain may be nil at any point. The current implementation returns None gracefully, but consider logging when PID extraction fails for debugging purposes.

Add debug logging when PID extraction fails to help diagnose window identification issues.

src/layout.rs (1)

377-398: Consider optimizing BSP tree rebuilding for better performance.

Always rebuilding the entire tree from scratch ensures consistency but may impact performance with many windows. Consider implementing incremental updates in the future.

For now, this approach is fine for typical window counts. Monitor performance and consider optimization if users report lag with 10+ windows.

src/snap.rs (2)

58-59: Remove unnecessary #[allow(dead_code)] attribute.

The window_id field is used in debug logging at line 289, so the dead_code attribute is not needed.

-    #[allow(dead_code)]
     window_id: WindowId,

---

275-279: Update method doesn't track current position during drag.

The update_window_drag method receives current_rect but doesn't use it. Consider tracking the current position for more accurate drag handling.

-    pub fn update_window_drag(&mut self, window_id: WindowId, _current_rect: Rect) {
+    pub fn update_window_drag(&mut self, window_id: WindowId, current_rect: Rect) {
         if let Some(drag_state) = self.window_drag_states.get_mut(&window_id) {
             drag_state.is_dragging = true;
+            // Optionally track current position for drag visualization or analytics
+            // drag_state.current_rect = current_rect;
         }
     }

src/window_manager.rs (5)

159-165: Track the TODO for configurable intervals.

The TODO comment suggests making refresh and monitor intervals configurable, which would be a valuable enhancement for performance tuning.

Would you like me to create an issue to track this configuration enhancement?

---

603-603: Document or make the resize delay configurable.

The 10ms delay seems arbitrary. Consider documenting why this delay is needed or making it configurable.

-            // Use a small delay to ensure the resize has completed
-            tokio::time::sleep(tokio::time::Duration::from_millis(10)).await;
+            // Small delay to ensure macOS has completed the resize animation
+            // before we snap the window back to layout position
+            tokio::time::sleep(tokio::time::Duration::from_millis(
+                self.config.snap.resize_settle_delay_ms.unwrap_or(10)
+            )).await;

---

643-643: Very low movement threshold may cause performance issues.

The 0.5px threshold for detecting movement is extremely sensitive and might trigger too frequently, potentially causing performance issues or unwanted snapping behavior.

-            // Very low threshold to catch even small movements (>0.5px)
-            if distance > 0.5 {
+            // Reasonable threshold to detect intentional movements (>2.0px)
+            if distance > self.config.snap.movement_threshold.unwrap_or(2.0) {

---

885-1066: Consider extracting common swap logic to reduce duplication.

Both swap_windows and swap_windows_with_rects have nearly identical bulk move and fallback logic. Consider extracting this to a shared helper method.

+    async fn execute_window_swap(
+        &mut self,
+        swap_layouts: HashMap<WindowId, Rect>,
+        windows: Vec<Window>,
+    ) -> Result<()> {
+        // Mark all 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.last_window_positions.insert(*window_id, *rect);
+                }
+            }
+            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.last_window_positions.insert(*window_id, *rect);
+                        }
+                        Err(e) => {
+                            warn!("Failed to move window {:?} during swap: {}", window_id, e)
+                        }
+                    }
+                }
+            }
+        }
+        Ok(())
+    }

---

1315-1315: Consider making batch size configurable.

The hardcoded BATCH_SIZE of 2 could be made configurable to allow tuning based on system performance.

-        const BATCH_SIZE: usize = 2; // Move 2 windows at a time
+        let batch_size = self.config.layout.batch_size.unwrap_or(2);

src/macos/accessibility.rs (2)

181-183: Consider selective cache refresh for better performance.

Refreshing the entire windows cache on every move operation could be expensive. Consider a more targeted refresh approach.

-        // Force refresh the cache to ensure we have current windows
-        if let Err(e) = self.refresh_all_windows_cache() {
-            warn!("Failed to refresh all windows cache: {}", e);
-        }
+        // Try to find in cache first, refresh only if needed
+        let needs_refresh = !self.window_cache.contains_key(&window_id) ||
+                           self.last_cache_update.elapsed() > std::time::Duration::from_secs(1);
+        
+        if needs_refresh {
+            if let Err(e) = self.refresh_all_windows_cache() {
+                warn!("Failed to refresh all windows cache: {}", e);
+            }
+        }

---

447-447: Document the rationale for 16K PID limit.

The cap of 16,384 PIDs seems reasonable but should be documented or made configurable for systems with many processes.

-                // Limit buffer growth to prevent excessive memory usage
-                let new_size = std::cmp::min(buffer.len() * 2, 16384); // Cap at 16K PIDs
+                // Limit buffer growth to prevent excessive memory usage
+                // 16K PIDs should be sufficient for most systems (typical: <1000 processes)
+                const MAX_PIDS: usize = 16384;
+                let new_size = std::cmp::min(buffer.len() * 2, MAX_PIDS);

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

src/main.rs (2)

84-92: Make log level configurable instead of hardcoding Debug.

The current implementation always sets the log level to Debug, which may produce excessive logs in production environments.

Consider making the log level configurable:

-fn setup_dual_logging(log_file: &PathBuf) -> Result<()> {
+fn setup_dual_logging(log_file: &PathBuf, log_level: log::LevelFilter) -> Result<()> {
     let dual_logger = DualLogger::new(log_file)?;
 
     log::set_boxed_logger(Box::new(dual_logger))
-        .map(|()| log::set_max_level(log::LevelFilter::Debug))
+        .map(|()| log::set_max_level(log_level))
         .map_err(|e| anyhow::anyhow!("Failed to init dual logger: {}", e))?;
 
     Ok(())
 }

Then in main, determine the log level from environment variable or CLI args:

let log_level = std::env::var("RUST_LOG")
    .ok()
    .and_then(|s| s.parse().ok())
    .unwrap_or(log::LevelFilter::Info);
setup_dual_logging(&log_file, log_level)?;

---

97-101: Consider more robust log file location handling.

The current implementation writes logs to the current working directory, which may not be writable or appropriate in all environments (e.g., system services, read-only filesystems).

Consider using a more appropriate default location:

-    let log_file = std::env::current_dir()
-        .unwrap_or_else(|_| PathBuf::from("."))
-        .join("skew.log");
+    let log_file = if let Ok(log_dir) = std::env::var("LOG_DIR") {
+        PathBuf::from(log_dir).join("skew.log")
+    } else {
+        // Try standard locations in order
+        let candidates = [
+            dirs::cache_dir().map(|p| p.join("skew")),
+            dirs::home_dir().map(|p| p.join(".local/share/skew")),
+            Some(PathBuf::from("/tmp")),
+        ];
+        
+        candidates.into_iter()
+            .flatten()
+            .find(|p| p.exists() || std::fs::create_dir_all(p).is_ok())
+            .unwrap_or_else(|| PathBuf::from("."))
+            .join("skew.log")
+    };

src/layout.rs (2)

87-87: Complex minimum size enforcement could be simplified.

The current logic for enforcing minimum split sizes has magic numbers and complex conditional logic that could be hard to maintain.

Consider extracting constants and simplifying the logic:

+    const MIN_SPLIT_SIZE: f64 = 150.0;
+    const MIN_SPLIT_RATIO: f64 = 0.3;
+    const MAX_SPLIT_RATIO: f64 = 0.7;
+
     fn update_child_rects(&mut self) {
         if let (Some(ref mut left), Some(ref mut right)) = (&mut self.left, &mut self.right) {
-            const MIN_SPLIT_SIZE: f64 = 150.0; // Reduced minimum for better space usage
-
             let (left_rect, right_rect) = if self.is_horizontal {
-                let ideal_left_width = self.rect.width * self.split_ratio;
-                let ideal_right_width = self.rect.width - ideal_left_width;
-
-                // Ensure both sides meet minimum requirements with better distribution
-                let left_width = if ideal_left_width < MIN_SPLIT_SIZE {
-                    MIN_SPLIT_SIZE.min(self.rect.width * 0.3) // Max 30% for minimum
-                } else if ideal_right_width < MIN_SPLIT_SIZE {
-                    (self.rect.width - MIN_SPLIT_SIZE).max(self.rect.width * 0.7)
-                // Min 70% for main
-                } else {
-                    ideal_left_width
-                };
+                let left_width = self.calculate_split_dimension(
+                    self.rect.width,
+                    self.split_ratio
+                );
                 let right_width = self.rect.width - left_width;

With a helper method:

fn calculate_split_dimension(&self, total: f64, ratio: f64) -> f64 {
    let ideal_left = total * ratio;
    let ideal_right = total - ideal_left;
    
    if ideal_left < Self::MIN_SPLIT_SIZE && ideal_right < Self::MIN_SPLIT_SIZE {
        // Both would be too small, give equal space
        total / 2.0
    } else if ideal_left < Self::MIN_SPLIT_SIZE {
        Self::MIN_SPLIT_SIZE.min(total * Self::MIN_SPLIT_RATIO)
    } else if ideal_right < Self::MIN_SPLIT_SIZE {
        (total - Self::MIN_SPLIT_SIZE).max(total * Self::MAX_SPLIT_RATIO)
    } else {
        ideal_left
    }
}

Also applies to: 94-101, 118-125

---

379-393: Consider optimizing BSP tree rebuilding for better performance.

The current implementation always rebuilds the entire BSP tree on every layout computation, which could be inefficient for large numbers of windows.

While the comment explains this avoids synchronization issues, consider these alternatives:

1. Incremental updates: Track window additions/removals and only rebuild when the window set changes
2. Dirty flag: Set a flag when windows are added/removed, rebuild only when dirty
3. Immutable tree: Use persistent data structures for lock-free updates

Example approach:

 pub struct LayoutManager {
     current_layout: LayoutType,
     bsp_root: Option<BSPNode>,
     split_ratio: f64,
+    bsp_dirty: bool,
+    last_window_ids: Vec<WindowId>,
 }

 fn compute_bsp_layout(...) -> HashMap<WindowId, Rect> {
+    let current_ids: Vec<_> = windows.iter().map(|w| w.id).collect();
+    if !self.bsp_dirty && self.last_window_ids == current_ids {
+        // Tree is up to date, just collect rects
+        if let Some(ref root) = self.bsp_root {
+            return root.collect_window_rects(general_config.gap);
+        }
+    }
     
     // Rebuild tree...
+    self.bsp_dirty = false;
+    self.last_window_ids = current_ids;

src/snap.rs (1)

541-565: Window overlap detection only considers center point.

The current implementation only checks if the dragged window's center point is within another window, which might miss cases where windows significantly overlap but centers don't align.

Consider using area overlap for more accurate detection:

 pub fn find_window_under_drag(
     &self,
     dragged_window_id: WindowId,
     dragged_rect: Rect,
     all_windows: &[&Window],
 ) -> Option<WindowId> {
-    let center_x = dragged_rect.x + dragged_rect.width / 2.0;
-    let center_y = dragged_rect.y + dragged_rect.height / 2.0;
-
-    for window in all_windows {
-        if window.id == dragged_window_id {
-            continue;
-        }
-
-        if self.point_in_rect(center_x, center_y, &window.rect) {
-            debug!(
-                "Found window {:?} under dragged window {:?}",
-                window.id, dragged_window_id
-            );
-            return Some(window.id);
-        }
-    }
+    let mut best_overlap = 0.0;
+    let mut best_window = None;
+    
+    for window in all_windows {
+        if window.id == dragged_window_id {
+            continue;
+        }
+        
+        let overlap = self.calculate_rect_overlap(&dragged_rect, &window.rect);
+        if overlap > best_overlap && overlap > 0.25 { // At least 25% overlap
+            best_overlap = overlap;
+            best_window = Some(window.id);
+        }
+    }
+    
+    if let Some(id) = best_window {
+        debug!(
+            "Found window {:?} under dragged window {:?} with {:.1}% overlap",
+            id, dragged_window_id, best_overlap * 100.0
+        );
+    }
+    best_window
 }
+
+fn calculate_rect_overlap(&self, rect1: &Rect, rect2: &Rect) -> f64 {
+    let x_overlap = (rect1.x + rect1.width).min(rect2.x + rect2.width) - rect1.x.max(rect2.x);
+    let y_overlap = (rect1.y + rect1.height).min(rect2.y + rect2.height) - rect1.y.max(rect2.y);
+    
+    if x_overlap > 0.0 && y_overlap > 0.0 {
+        let overlap_area = x_overlap * y_overlap;
+        let dragged_area = rect1.width * rect1.height;
+        overlap_area / dragged_area
+    } else {
+        0.0
+    }
+}

src/window_manager.rs (4)

82-89: Add missing imports for collections types.

The code uses std::collections::HashSet and std::collections::HashMap with fully qualified paths. Add these imports at the top of the file for better readability.

Add these imports at the top of the file:

use std::collections::{HashMap, HashSet};

Then simplify the field declarations:

-    programmatically_moving: std::collections::HashSet<WindowId>,
+    programmatically_moving: HashSet<WindowId>,
-    user_dragging_windows: std::collections::HashSet<WindowId>,
+    user_dragging_windows: HashSet<WindowId>,
-    last_window_positions: std::collections::HashMap<WindowId, Rect>,
+    last_window_positions: HashMap<WindowId, Rect>,

---

160-166: Address TODO: Make window monitoring intervals configurable.

The TODO comment indicates that the window refresh and monitor intervals should be configurable via skew.toml.

Would you like me to create an issue to track implementing configurable intervals for window_refresh_interval_ms and window_monitor_interval_ms?

---

831-835: Consider making overlap detection tolerance configurable.

The overlap detection uses a hardcoded 1.0px tolerance which might be too strict for high-DPI displays or cause issues with rounding.

Make the overlap tolerance configurable:

+                let overlap_tolerance = self.config.general.overlap_tolerance.unwrap_or(1.0);
                 let overlaps = !(current_rect.x + current_rect.width <= target_rect.x + overlap_tolerance
-                    || target_rect.x + target_rect.width <= current_rect.x + 1.0
-                    || current_rect.y + current_rect.height <= target_rect.y + 1.0
-                    || target_rect.y + target_rect.height <= current_rect.y + 1.0);
+                    || target_rect.x + target_rect.width <= current_rect.x + overlap_tolerance
+                    || current_rect.y + current_rect.height <= target_rect.y + overlap_tolerance
+                    || target_rect.y + target_rect.height <= current_rect.y + overlap_tolerance);

---

1300-1316: Consider making layout application parameters configurable.

The layout throttling (50ms), batch size (2), and inter-batch delay (1ms) are hardcoded. Making these configurable would allow performance tuning for different systems.

Add configuration options:

-                if now.duration_since(last_time).as_millis() < 50 {
+                if now.duration_since(last_time).as_millis() < self.config.layout.throttle_ms.unwrap_or(50) {

-        const BATCH_SIZE: usize = 2; // Move 2 windows at a time
+        let batch_size = self.config.layout.batch_size.unwrap_or(2);

-                tokio::time::sleep(tokio::time::Duration::from_millis(1)).await;
+                let batch_delay = self.config.layout.batch_delay_ms.unwrap_or(1);
+                tokio::time::sleep(tokio::time::Duration::from_millis(batch_delay)).await;

Also applies to: 1378-1400

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