Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

major: refactor codebase without a breaking change #17

Merged
merged 13 commits into from
Dec 11, 2024
Merged

Conversation

FMotalleb
Copy link
Owner

@FMotalleb FMotalleb commented Dec 11, 2024

Summary by CodeRabbit

  • New Features

    • Introduced clipboard management functionality for Linux, macOS, and Windows.
    • Added commands for copying and pasting clipboard content.
    • Implemented error handling for clipboard operations.
  • Bug Fixes

    • Updated the deprecation notice for the enforce-daemon feature in the documentation.
  • Refactor

    • Removed the enforce-daemon feature and associated logging.
    • Restructured clipboard functionality and error handling.
    • Consolidated clipboard operations into dedicated modules.
    • Enhanced error handling and control flow in the main application logic.
  • Documentation

    • Updated README to reflect changes in feature support and usage.
    • Incremented the version of the nu_plugin_clipboard package to 0.100.1.
  • Chores

    • Cleaned up unused code and removed obsolete files related to clipboard operations.

Copy link

coderabbitai bot commented Dec 11, 2024

Walkthrough

The changes in this pull request involve modifications to the nu_plugin_clipboard package, including the removal of the enforce-daemon feature from Cargo.toml, updates to the README.md to reflect this deprecation, and the restructuring of clipboard functionality across various operating systems. New modules and structures have been introduced for clipboard management, while existing files related to clipboard copying and pasting have been removed. The overall architecture enhances error handling and organizes code for better platform-specific support.

Changes

File Path Change Summary
Cargo.toml Removed enforce-daemon feature from [features] section and updated version from 0.100.0 to 0.100.1.
README.md Updated description of enforce-daemon feature with deprecation notice.
build.nu Removed logging entry for enforce-daemon feature and altered logic for appending features based on OS.
src/clipboard/arboard_provider.rs Added with_clipboard_instance function for clipboard operations with error handling.
src/clipboard/clipboard.rs Introduced clipboard management module with CheckResult enum, create_clipboard function, and Clipboard trait with methods for clipboard operations.
src/clipboard/copy.rs Removed file containing ClipboardCopy struct and related methods for copying text.
src/clipboard/error_mapper.rs Added map_arboard_err_to_label function for error mapping.
src/clipboard/linux.rs Introduced ClipBoardLinux struct implementing the Clipboard trait for Linux.
src/clipboard/mac_os.rs Introduced ClipBoardMacos struct implementing the Clipboard trait for macOS.
src/clipboard/mod.rs Removed copy and paste modules; added arboard_provider and error_mapper modules; introduced platform-specific modules for clipboard management.
src/clipboard/paste.rs Removed file containing ClipboardPaste struct and related methods for pasting text.
src/clipboard/windows.rs Introduced ClipBoardWindows struct implementing the Clipboard trait for Windows.
src/command/copy.rs Added new ClipboardCopy struct implementing the PluginCommand trait for clipboard copy functionality.
src/command/mod.rs Introduced new modules copy and paste for encapsulating clipboard command functionalities.
src/command/paste.rs Added new ClipboardPaste struct implementing the PluginCommand trait for clipboard paste functionality.
src/main.rs Updated main function signature to return Result<(), io::Error>; restructured logic for plugin serving and error handling.
nupm.nuon Updated version from 0.100.0 to 0.100.1.
src/utils/mod.rs Added a newline at the end of the file.

Possibly related issues

  • issues with 0.94.0 #6: The changes in this PR address the issue of the version not being bumped in the nupm.nuon file, as it has been updated from 0.100.0 to 0.100.1.

Possibly related PRs

  • bump the Nump package to 0.96.0 #12: This PR is related as it also involves updating the version of the nu_plugin_clipboard package, similar to the version increment from 0.100.0 to 0.100.1 in the main PR.

Poem

In the land of code where rabbits hop,
Clipboard changes make our hearts stop!
Features removed, new paths we trace,
With every update, we quicken our pace.
Clipboard magic, both near and far,
Hopping along, we’ll raise the bar! 🐰✨


📜 Recent review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between acde309 and 5fa5dcd.

📒 Files selected for processing (1)
  • src/command/paste.rs (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • src/command/paste.rs

Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR. (Experiment)
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 10

🧹 Outside diff range and nitpick comments (9)
src/clipboard/clipboard.rs (1)

12-12: Ensure consistent naming conventions for Clipboard structs

Currently, the structs are named inconsistently: ClipBoardLinux, ClipboardMacOs, and ClipboardWindows. For consistency, consider using the same naming convention across all platforms.

Also applies to: 16-16, 20-20

src/clipboard/mod.rs (1)

5-11: Consider adding a default implementation for unsupported platforms.

The platform-specific modules are well-organized, but consider handling unsupported platforms to provide better error messages.

Add a fallback implementation:

#[cfg(target_os = "windows")]
pub(crate) mod windows;

+#[cfg(not(any(
+    target_os = "linux",
+    target_os = "macos",
+    target_os = "windows"
+)))]
+pub(crate) mod unsupported;
src/clipboard/windows.rs (2)

1-2: Remove unused import LabeledError.

The LabeledError import is not used in the current implementation.

use super::{arboard_provider::with_clipboard_instance, clipboard::Clipboard};
-use nu_protocol::LabeledError;

3-9: Add documentation for the Windows clipboard implementation.

Consider adding documentation to explain the purpose and behavior of this implementation.

Add documentation:

+/// Windows-specific clipboard implementation.
+/// 
+/// This struct provides clipboard operations for Windows platforms
+/// using the arboard provider.
pub struct ClipBoardWindows {}

impl ClipBoardWindows {
    pub fn new() -> Self {
        ClipBoardWindows {}
    }
}
src/clipboard/mac_os.rs (1)

1-3: Remove unused import and consider organizing imports.

The LabeledError import is not used in the current implementation.

-use nu_protocol::LabeledError;

use super::{arboard_provider::with_clipboard_instance, clipboard::Clipboard};
src/clipboard/arboard_provider.rs (1)

3-12: Add documentation comments to improve code clarity.

Consider adding documentation comments to explain:

  • The purpose of the function
  • The generic type parameter U
  • The closure parameter expectations
  • Error handling behavior

Add the following documentation:

+/// Executes a clipboard operation with proper error handling
+/// 
+/// # Arguments
+/// * `op` - Closure that performs the clipboard operation
+/// 
+/// # Type Parameters
+/// * `U` - The return type of the clipboard operation
+/// 
+/// # Returns
+/// Returns the operation result wrapped in Result<U, LabeledError>
 pub(crate) fn with_clipboard_instance<
     U,
     F: FnOnce(&mut arboard::Clipboard) -> Result<U, arboard::Error>,
src/main.rs (1)

29-42: Consider simplifying the match expression and improving error messages.

The current implementation handles errors appropriately but could be more concise and user-friendly.

Consider this refactoring:

 fn main() -> Result<(), io::Error> {
-    match create_clipboard().pre_execute_check() {
-        CheckResult::Continue => Ok(nu_plugin::serve_plugin(
-            &mut ClipboardPlugins {},
-            nu_plugin::MsgPackSerializer {},
-        )),
-        CheckResult::Exit(message, code) => {
-            if code != 0 {
-                writeln!(stderr(), "Error ({}): {}", code, message)?;
-            } else if !message.is_empty() {
-                writeln!(stdout(), "{}", message)?;
-            }
-            exit(code)
-        }
-    }
+    let check_result = create_clipboard().pre_execute_check();
+    match check_result {
+        CheckResult::Continue => Ok(nu_plugin::serve_plugin(
+            &mut ClipboardPlugins {},
+            nu_plugin::MsgPackSerializer {},
+        )),
+        CheckResult::Exit(message, code) => {
+            let writer = if code != 0 { &mut stderr() } else { &mut stdout() };
+            if !message.is_empty() {
+                let prefix = if code != 0 { "Error: " } else { "" };
+                writeln!(writer, "{}{}", prefix, message)?;
+            }
+            exit(code)
+        }
+    }
 }
src/command/paste.rs (1)

29-31: Enhance command description for better user experience.

The current description is brief and could provide more details about the command's behavior and limitations.

Consider expanding the description:

-        "outputs the current value in clipboard"
+        "Outputs the current text content from the system clipboard. Note: Only text content is supported."
README.md (1)

46-46: Enhance deprecation notice with additional details.

While the deprecation notice is clear, consider adding:

  1. Behavior on non-Linux platforms
  2. Rationale for the change
  3. Migration guide for existing users who might be using the --daemon flag

Consider expanding the notice like this:

- * **enforce-daemon**: Deprecation notice: this method is now always enabled in linux environments
+ * **enforce-daemon**: Deprecation notice: This feature has been removed as the daemon mode is now:
+   - Always enabled on Linux environments
+   - Not applicable on other platforms
+   - The `--daemon` flag can be safely removed from your scripts
+   This change simplifies the clipboard handling and improves reliability.
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 00cc3d3 and 4695822.

📒 Files selected for processing (16)
  • Cargo.toml (0 hunks)
  • README.md (1 hunks)
  • build.nu (1 hunks)
  • src/clipboard/arboard_provider.rs (1 hunks)
  • src/clipboard/clipboard.rs (1 hunks)
  • src/clipboard/copy.rs (0 hunks)
  • src/clipboard/error_mapper.rs (1 hunks)
  • src/clipboard/linux.rs (1 hunks)
  • src/clipboard/mac_os.rs (1 hunks)
  • src/clipboard/mod.rs (1 hunks)
  • src/clipboard/paste.rs (0 hunks)
  • src/clipboard/windows.rs (1 hunks)
  • src/command/copy.rs (1 hunks)
  • src/command/mod.rs (1 hunks)
  • src/command/paste.rs (1 hunks)
  • src/main.rs (2 hunks)
💤 Files with no reviewable changes (3)
  • Cargo.toml
  • src/clipboard/paste.rs
  • src/clipboard/copy.rs
✅ Files skipped from review due to trivial changes (1)
  • src/command/mod.rs
🔇 Additional comments (4)
src/clipboard/mod.rs (1)

1-4: LGTM! Well-structured module organization.

The modularization follows good practices with:

  • Clear separation of provider and error handling
  • Internal modules for implementation details
  • Public module for the main clipboard interface
src/clipboard/mac_os.rs (1)

13-13: Empty trait implementation requires verification.

The empty Clipboard trait implementation suggests that required methods might be missing.

Let's verify if this implementation matches the Windows version:

✅ Verification successful

Let me check the trait definition to understand what methods are required.


Let me try a different approach to find the trait definition.


Empty trait implementation is correct as all methods have default implementations

The Clipboard trait defines three methods:

  • pre_execute_check with a default implementation returning CheckResult::Continue
  • copy_text with a default implementation using with_clipboard_instance
  • get_text with a default implementation using with_clipboard_instance

Since all methods have default implementations, the empty implementation for ClipBoardMacos is valid and matches the Windows implementation. Only Linux has a custom implementation of pre_execute_check for daemon-specific functionality.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Compare implementations across platform-specific files
rg -U "impl Clipboard for" -A 5

Length of output: 596


Script:

#!/bin/bash
# Find the Clipboard trait definition
ast-grep --pattern 'trait Clipboard {
  $$$
}'

Length of output: 48


Script:

#!/bin/bash
# Search for Clipboard trait definition using ripgrep
rg "trait Clipboard" -A 10

Length of output: 685

src/command/paste.rs (1)

1-44: Verify clipboard functionality across platforms.

The clipboard implementation should be tested across different operating systems to ensure consistent behavior.

Run the following script to check platform-specific implementations:

✅ Verification successful

Let me check the platform-specific implementations in more detail.


Clipboard functionality is properly implemented across platforms

The codebase shows a well-structured, platform-specific implementation with:

  • Dedicated modules for Linux (linux.rs), macOS (mac_os.rs), and Windows (windows.rs)
  • Proper conditional compilation using cfg(target_os) attributes
  • Consistent error handling across platforms using LabeledError
  • Common Clipboard trait ensuring uniform interface
  • Platform-specific implementations properly isolated in separate modules
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for platform-specific clipboard implementations

# Look for platform-specific code
rg -l "cfg\(target_os" 

# Check for platform-specific dependencies in Cargo.toml
rg -l "target_os.*dependencies" 

# Look for potential platform-specific issues
ast-grep --pattern 'if cfg!(target_os = $_) { $$$ }'

Length of output: 184


Script:

#!/bin/bash
# Check platform-specific clipboard implementations
rg -A 10 "cfg\(target_os" src/clipboard/mod.rs src/clipboard/clipboard.rs src/clipboard/linux.rs

# Check if there are other platform-specific files
fd -e rs clipboard

# Look for error handling in clipboard implementations
rg "Result|Error" src/clipboard/

Length of output: 3891

build.nu (1)

4-4: LGTM! Messages dictionary cleanup is consistent with feature deprecation.

The removal of the "enforce-daemon" message aligns with the feature's deprecation while preserving the "use-wayland" functionality.

Let's verify if any scripts depend on the removed message:

✅ Verification successful

LGTM! Messages dictionary cleanup is safe and consistent with feature deprecation

The removal of "enforce-daemon" message is safe because:

  • The feature is explicitly marked as deprecated in README.md: "enforce-daemon: Deprecation notice: this method is now always enabled in linux environments"
  • No other scripts depend on the removed message, as it's only referenced in documentation
  • The remaining "use-wayland" message and feature handling remain intact
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for any references to the removed enforce-daemon message
rg -l "enforce-daemon.*activating" --type nu

Length of output: 73


Script:

#!/bin/bash
# Search for any references to enforce-daemon in all files
rg -l "enforce-daemon"

Length of output: 32


Script:

#!/bin/bash
# Search for files with .nu extension
fd -e nu

Length of output: 19


Script:

#!/bin/bash
# Get content of build.nu to understand the context better
cat build.nu

Length of output: 1368


Script:

#!/bin/bash
# Check README.md for any critical usage of enforce-daemon
cat README.md

Length of output: 1669

src/clipboard/clipboard.rs Outdated Show resolved Hide resolved
src/clipboard/linux.rs Outdated Show resolved Hide resolved
src/clipboard/linux.rs Show resolved Hide resolved
src/clipboard/linux.rs Outdated Show resolved Hide resolved
src/command/copy.rs Outdated Show resolved Hide resolved
src/command/copy.rs Outdated Show resolved Hide resolved
src/clipboard/error_mapper.rs Outdated Show resolved Hide resolved
src/clipboard/windows.rs Show resolved Hide resolved
src/clipboard/mac_os.rs Outdated Show resolved Hide resolved
src/command/paste.rs Outdated Show resolved Hide resolved
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Outside diff range and nitpick comments (1)
src/clipboard/linux.rs (1)

1-68: Consider adding documentation for the daemon-based approach.

The implementation uses a daemon-based approach for Linux clipboard operations, which is a significant architectural decision. Consider adding documentation (either as code comments or in the README) explaining:

  • Why a daemon is necessary for Linux clipboard operations
  • How the daemon process handles clipboard persistence
  • Any limitations or considerations users should be aware of
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 4695822 and 631091b.

📒 Files selected for processing (5)
  • src/clipboard/clipboard.rs (1 hunks)
  • src/clipboard/error_mapper.rs (1 hunks)
  • src/clipboard/linux.rs (1 hunks)
  • src/clipboard/mac_os.rs (1 hunks)
  • src/clipboard/windows.rs (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
  • src/clipboard/error_mapper.rs
  • src/clipboard/windows.rs
  • src/clipboard/mac_os.rs
  • src/clipboard/clipboard.rs
🔇 Additional comments (4)
src/clipboard/linux.rs (4)

1-15: LGTM! Well-organized imports and constant definition.

The imports are properly organized and the constant is appropriately named and scoped.


16-16: LGTM! Appropriate struct definition.

The empty struct is correctly used as a trait implementation container.


54-68: LGTM! Clean trait implementation.

The Clipboard trait implementation is clean and follows the expected contract. The daemon mode handling in pre_execute_check and delegation in copy_text are well-implemented.


44-51: ⚠️ Potential issue

Fix clipboard text setting and error handling.

The copy_with_daemon method has two issues that need to be addressed:

  1. Incorrect method chain for setting clipboard text
  2. Improper error handling for clip.clear()

Apply this diff to fix both issues:

 fn copy_with_daemon() -> Result<(), nu_protocol::LabeledError> {
     with_clipboard_instance(|clip: &mut arboard::Clipboard| {
-        clip.clear()?;
-        let args: Vec<String> = env::args().skip(2).collect();
-        let data = args.join(" ");
-        clip.set().wait().text(data)
+        clip.clear().map_err(|err| nu_protocol::LabeledError::new(
+            "Failed to clear clipboard".to_string(),
+            err.to_string()
+        ))?;
+        let args: Vec<String> = env::args().skip(2).collect();
+        let data = args.join(" ");
+        clip.set_text(data).map_err(|err| nu_protocol::LabeledError::new(
+            "Failed to set clipboard text".to_string(),
+            err.to_string()
+        ))
     })
 }

Likely invalid or redundant comment.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Outside diff range and nitpick comments (3)
src/command/paste.rs (2)

9-15: Add documentation comments for better API documentation.

Consider adding documentation comments to improve the API documentation:

+/// Implements clipboard paste functionality for the Nu shell.
 pub struct ClipboardPaste;

 impl ClipboardPaste {
+    /// Creates a new instance of the clipboard paste command.
     pub fn new() -> ClipboardPaste {
         ClipboardPaste {}
     }
 }

33-46: Enhance error handling and user feedback.

While the basic error handling is in place, consider these improvements:

     fn run(
         &self,
         _plugin: &Self::Plugin,
         _engine: &EngineInterface,
         call: &EvaluatedCall,
         _input: PipelineData,
     ) -> Result<PipelineData, LabeledError> {
         let text = create_clipboard().get_text()?;
         if text.trim().is_empty() {
-            return Err(LabeledError::new("Empty clipboard".to_string()));
+            return Err(LabeledError {
+                label: "Empty clipboard".into(),
+                msg: "The clipboard does not contain any text content".into(),
+                span: Some(call.head),
+            });
         }
         Ok(Value::string(text, call.head).into_pipeline_data())
     }

Also, consider enhancing the command description:

     fn description(&self) -> &str {
-        "outputs the current value in clipboard"
+        "Retrieves and outputs the current text content from the system clipboard"
     }
src/command/copy.rs (1)

13-25: Consider enhancing error messages with more context.

The error handling is well-structured, but the error messages could be more descriptive to help with debugging.

Consider this improvement:

-            Ok(Err(err)) => Err(LabeledError::new(format!(
-                "JSON Serialization Error: {}",
-                err,
-            ))),
-            Err(err) => Err(LabeledError::new(format!("JSON Conversion Error: {}", err))),
+            Ok(Err(err)) => Err(LabeledError::new(format!(
+                "Failed to serialize value to JSON: {}",
+                err,
+            ))),
+            Err(err) => Err(LabeledError::new(format!(
+                "Failed to convert value to JSON structure: {}",
+                err
+            ))),
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 631091b and 5b86db9.

⛔ Files ignored due to path filters (1)
  • Cargo.lock is excluded by !**/*.lock
📒 Files selected for processing (2)
  • src/command/copy.rs (1 hunks)
  • src/command/paste.rs (1 hunks)
🔇 Additional comments (6)
src/command/paste.rs (3)

1-7: LGTM! Well-organized imports.

The imports are cleanly organized with external dependencies first, followed by internal crate imports.


16-32: LGTM! Well-structured command implementation.

The command implementation follows Nu shell plugin conventions with appropriate type signatures and category marking.


40-40: Verify clipboard functionality across platforms.

The create_clipboard() function is used to handle clipboard operations. Let's verify the implementation across different platforms.

✅ Verification successful

Clipboard implementation is properly handled across platforms

The clipboard functionality is well-implemented with proper platform-specific handling:

  • Common interface through the Clipboard trait with default implementations for copy/paste operations
  • Platform-specific implementations for Windows, macOS, and Linux
  • Linux implementation includes additional daemon support for persistent clipboard
  • Uses arboard as the underlying clipboard provider with proper error handling
  • Factory pattern (create_clipboard()) ensures correct platform-specific implementation is used
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check clipboard implementations across platforms

# Look for platform-specific implementations
echo "Checking platform-specific clipboard implementations:"
rg -l "impl.*Clipboard.*for" --type rust

# Check for platform-specific tests
echo -e "\nChecking platform-specific tests:"
rg -l "test.*clipboard" --type rust

Length of output: 352


Script:

#!/bin/bash
# Let's examine the clipboard implementations and test coverage

# Check the content of platform-specific implementations
echo "=== Windows Clipboard Implementation ==="
rg "impl.*Clipboard" src/clipboard/windows.rs -A 5

echo -e "\n=== macOS Clipboard Implementation ==="
rg "impl.*Clipboard" src/clipboard/mac_os.rs -A 5

echo -e "\n=== Linux Clipboard Implementation ==="
rg "impl.*Clipboard" src/clipboard/linux.rs -A 5

# Look for any test modules in these files
echo -e "\n=== Test Modules in Clipboard Files ==="
rg "mod tests" src/clipboard/ -A 5

# Check if there are any integration tests
echo -e "\n=== Integration Tests ==="
fd "test" tests/ --type f --exec rg "clipboard" {}

Length of output: 1146


Script:

#!/bin/bash
# Let's examine the clipboard implementations more thoroughly

# Check the actual implementation details for each platform
echo "=== Windows Clipboard Details ==="
cat src/clipboard/windows.rs

echo -e "\n=== macOS Clipboard Details ==="
cat src/clipboard/mac_os.rs

echo -e "\n=== Linux Clipboard Details ==="
cat src/clipboard/linux.rs

# Check for any unit tests in the clipboard module
echo -e "\n=== Unit Tests in Clipboard Module ==="
rg "#\[test\]" src/clipboard/ -B 1 -A 5

Length of output: 2958


Script:

#!/bin/bash
# Let's check the clipboard trait and provider implementation

# Check the clipboard trait definition
echo "=== Clipboard Trait Definition ==="
rg "trait Clipboard" src/clipboard/ -A 10

# Check the arboard provider implementation
echo -e "\n=== Arboard Provider Implementation ==="
cat src/clipboard/arboard_provider.rs

# Check the main clipboard factory
echo -e "\n=== Clipboard Factory/Creation ==="
rg "create_clipboard" -A 5

Length of output: 3623

src/command/copy.rs (3)

1-7: LGTM! Well-organized imports and clean struct declaration.

The imports are specific and follow Rust best practices. The empty struct declaration is appropriate for this command pattern implementation.


64-73: ⚠️ Potential issue

Fix incorrect usage of input.into_value and error handling

The method input.into_value(call.head) returns a Value, not a Result. Therefore, pattern matching on value for Ok and Err is incorrect.

Apply this diff to fix the issue:

-        let value = input.into_value(call.head);
-        match value {
-            Ok(value) => {
-                if let Err(err) = Self::copy(&value) {
-                    return Err(err);
-                }
-                Ok(value.into_pipeline_data())
-            }
-            Err(err) => Err(LabeledError::new(err.to_string())),
-        }
+        let value = input.into_value(call.head);
+        if let Err(err) = Self::copy(&value) {
+            return Err(err);
+        }
+        Ok(value.into_pipeline_data())

46-55: Consider adding input size validation

The command accepts various input types (String, Record, Table, List) but doesn't validate their size. Large inputs could cause performance issues or clipboard buffer overflow.

Consider adding size validation before processing the input. You can use the following script to check if size limits are handled elsewhere in the codebase:

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

🧹 Outside diff range and nitpick comments (2)
src/command/copy.rs (2)

9-12: Consider using a more idiomatic implementation for the constructor

Since ClipboardCopy is an empty struct, you can simplify the constructor.

-    pub fn new() -> ClipboardCopy {
-        ClipboardCopy {}
-    }
+    pub fn new() -> Self {
+        Self
+    }

13-25: Consider simplifying the error handling using combinators

The nested match pattern can be simplified while maintaining the same error handling.

     fn format_json(input: &Value) -> Result<String, LabeledError> {
-        let json_value =
-            json::value_to_json_value(&input).map(|v| nu_json::to_string_with_indent(&v, 4));
-
-        match json_value {
-            Ok(Ok(text)) => Ok(text.to_owned()), // Return the owned String
-            Ok(Err(err)) => Err(LabeledError::new(format!(
-                "JSON Serialization Error: {}",
-                err,
-            ))),
-            Err(err) => Err(LabeledError::new(format!("JSON Conversion Error: {}", err))),
-        }
+        json::value_to_json_value(&input)
+            .map_err(|e| LabeledError::new(format!("JSON Conversion Error: {}", e)))
+            .and_then(|v| {
+                nu_json::to_string_with_indent(&v, 4)
+                    .map_err(|e| LabeledError::new(format!("JSON Serialization Error: {}", e)))
+            })
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between e31cb3e and acde309.

⛔ Files ignored due to path filters (1)
  • Cargo.lock is excluded by !**/*.lock
📒 Files selected for processing (8)
  • Cargo.toml (1 hunks)
  • README.md (1 hunks)
  • nupm.nuon (1 hunks)
  • src/clipboard/clipboard.rs (1 hunks)
  • src/clipboard/linux.rs (1 hunks)
  • src/command/copy.rs (1 hunks)
  • src/command/paste.rs (1 hunks)
  • src/main.rs (2 hunks)
✅ Files skipped from review due to trivial changes (1)
  • nupm.nuon
🚧 Files skipped from review as they are similar to previous changes (2)
  • Cargo.toml
  • README.md
🔇 Additional comments (6)
src/clipboard/linux.rs (2)

40-43: Ensure LabeledError::new is called with both label and message

The LabeledError::new function requires two arguments: a label and a message. Currently, it's being called with only one argument, which may lead to a compilation error.

Apply this diff to fix the issue:

             Err(err) | Ok(Err(err)) => Err(nu_protocol::LabeledError::new(
-                format!(
-                    "Failed to spawn daemon process: {}",
-                    err.to_string()
-                )
+                "Failed to spawn daemon process".to_string(),
+                err.to_string()
             )),

51-51: Correct method call for setting clipboard text

The method chain clip.set().wait().text(data) is incorrect. The correct method to set text to the clipboard is clip.set_text(data).

Apply this diff to fix the method call:

-            clip.set().wait().text(data)
+            clip.set_text(data)
src/main.rs (1)

29-42: Code changes look good

The restructuring of the main function and the enhanced error handling improve the robustness of the application.

src/command/paste.rs (1)

42-42: ⚠️ Potential issue

Ensure LabeledError::new includes both label and message

The LabeledError::new function requires both a label and a message. Currently, only one argument is provided, which may cause a compilation error.

Apply this diff to fix the issue:

-            return Err(LabeledError::new("Empty clipboard".to_string()));
+            return Err(LabeledError::new(
+                "Empty clipboard".to_string(),
+                "The clipboard does not contain any text content".to_string(),
+            ));

Likely invalid or redundant comment.

src/command/copy.rs (2)

1-7: LGTM!

The imports are well-organized and the struct declaration is appropriate.


60-77: ⚠️ Potential issue

Fix incorrect error handling in run method

The current implementation has incorrect error handling for input.into_value, as mentioned in a previous review.

     fn run(
         &self,
         _plugin: &Self::Plugin,
         engine: &EngineInterface,
         call: &EvaluatedCall,
         input: PipelineData,
     ) -> Result<PipelineData, LabeledError> {
         let value = input.into_value(call.head);
-        match value {
-            Ok(value) => {
-                if let Err(err) = Self::copy(engine, &value) {
-                    return Err(err);
-                }
-                Ok(value.into_pipeline_data())
-            }
-            Err(err) => Err(LabeledError::new(err.to_string())),
-        }
+        let value = value.map_err(|e| LabeledError::new(format!("Failed to get value: {}", e)))?;
+        Self::copy(engine, &value)?;
+        Ok(value.into_pipeline_data())
     }

src/clipboard/clipboard.rs Show resolved Hide resolved
pub fn create_clipboard(config: Option<nu_protocol::Value>) -> impl Clipboard {
#[cfg(target_os = "linux")]
{
crate::clipboard::linux::ClipBoardLinux::new(!no_daemon(config).unwrap_or(false))
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Handle potential errors from no_daemon function

The call to no_daemon(config).unwrap_or(false) may mask underlying errors from the no_daemon function. It's better to handle the Result properly to avoid unintended side effects.

Consider propagating the error or handling it appropriately. You can modify the code as follows:

-            crate::clipboard::linux::ClipBoardLinux::new(!no_daemon(config).unwrap_or(false))
+            let use_daemon = match no_daemon(config) {
+                Ok(value) => !value,
+                Err(err) => {
+                    eprintln!("Error determining daemon usage: {}", err);
+                    false
+                }
+            };
+            crate::clipboard::linux::ClipBoardLinux::new(use_daemon)
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
crate::clipboard::linux::ClipBoardLinux::new(!no_daemon(config).unwrap_or(false))
let use_daemon = match no_daemon(config) {
Ok(value) => !value,
Err(err) => {
eprintln!("Error determining daemon usage: {}", err);
false
}
};
crate::clipboard::linux::ClipBoardLinux::new(use_daemon)

src/command/copy.rs Show resolved Hide resolved
@FMotalleb FMotalleb merged commit 5c95fe0 into main Dec 11, 2024
1 check passed
@FMotalleb FMotalleb deleted the refactor branch December 11, 2024 12:38
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.

1 participant