refactor #84
refactor #84
Conversation
WalkthroughReplaces the legacy Move implementation with a new Files.Move API that accepts a Connection model, adds impersonation helpers, centralized error handling, Helpers utilities, updated Definitions and tests, bumps projects to .NET 8, updates CI workflow inputs, and adds Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant Files as Files.Move
participant ImpSrc as ImpersonatedAction (Source)
participant ImpTgt as ImpersonatedAction (Target)
participant Help as Helpers
participant FS as File System
participant EH as ErrorHandler
Client->>Files: Move(Input, Connection, Options, CancellationToken)
rect #eef6ff
note right of Files: Discover & plan
Files->>ImpSrc: Execute(FindMatchingFiles, Source)
ImpSrc->>Help: FindMatchingFiles(SourceDirectory, pattern)
Help-->>ImpSrc: Matches
ImpSrc-->>Files: Matches
Files->>Help: GetFileTransferEntries(matches, TargetDirectory, preserve)
Help-->>Files: Entries
Files->>Help: AssertNoTargetFileConflicts(entries, IfTargetFileExists)
Help-->>Files: ok
end
alt CreateTargetDirectories
Files->>ImpTgt: Execute(Ensure target dirs, Target)
ImpTgt->>Help: Create directories
Help-->>ImpTgt: ok
end
loop For each entry
Files->>ImpTgt: Execute(Copy/Rename/Overwrite, Target)
ImpTgt->>Help: CopyFileImpersonated / GetNonConflictingTargetFilePath
Help->>FS: Read/Write
FS-->>Help: done
Help-->>ImpTgt: done
ImpTgt-->>Files: done
end
Files->>ImpSrc: Execute(Delete source files), Source
ImpSrc->>FS: Delete
FS-->>ImpSrc: done
ImpSrc-->>Files: done
Files-->>Client: Result { Success=true, Files=[...] }
opt On error (non-canceled)
Files->>EH: Handle(exception, ThrowErrorOnFailure, ErrorMessageOnFailure)
EH-->>Files: Result or throws
Files-->>Client: Result { Success=false, Error=... } or exception
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Frends.Files.Move/Frends.Files.Move/Definitions/FileExistsAction.cs (1)
4-4: Fix typo in summary.“exits” → “exists”.
Apply this diff:
-/// Action to be executed when the file exits in destination. +/// Action to be executed when the file exists at destination.
🧹 Nitpick comments (6)
Frends.Files.Move/Frends.Files.Move/Definitions/Result.cs (1)
26-26: Consider restricting the Files property setter.The public setter allows external code to replace the entire Files list, which could lead to unintended modifications. Consider using an init-only setter or private set for better encapsulation.
Apply this diff to use an init-only setter:
- public List<FileItem> Files { get; set; } = []; + public List<FileItem> Files { get; init; } = [];Frends.Files.Move/Frends.Files.Move.Tests/RemoteTests.cs (2)
12-33: Guard tests by OS and missing secrets; mark as integration; avoid null-forgiving risks.
- Skip on non‑Windows.
- Load .env (per guidelines) and skip if secrets missing.
- Mark as [Category("Integration")] and [NonParallelizable].
Apply this diff:
using System; using System.IO; +using System.Runtime.InteropServices; using System.Security.Principal; using Frends.Files.Move.Definitions; using NUnit.Framework; using System.Threading; using System.Threading.Tasks; using SimpleImpersonation; namespace Frends.Files.Move.Tests; -[TestFixture] +[TestFixture] +[Category("Integration")] +[NonParallelizable] internal class RemoteTests { + [OneTimeSetUp] + public void OneTimeSetUp() + { + try + { + // Load secrets via dotenv if available (aligns with guidelines) + // Requires DotNetEnv package if used; otherwise this is a no-op placeholder. + // DotNetEnv.Env.TraversePath().Load(); + } + catch + { + // Ignore dotenv load errors; env vars may be provided by CI. + } + } + + [SetUp] + public void SkipIfUnsupported() + { + if (!RuntimeInformation.IsOSPlatform(OSPlatform.Windows)) + Assert.Ignore("Impersonation tests run on Windows only."); + + var missing = string.IsNullOrWhiteSpace(SrcUser) + || string.IsNullOrWhiteSpace(SrcUserPassword) + || string.IsNullOrWhiteSpace(DstUser) + || string.IsNullOrWhiteSpace(DstUserPassword) + || string.IsNullOrWhiteSpace(AdminUser) + || string.IsNullOrWhiteSpace(AdminUserPassword); + if (missing) + Assert.Ignore("Missing required environment variables for remote tests."); + }
42-43: Avoid hard‑coded UNC; parameterize host/share; reduce flakiness and enable CI configuration.Use an env var for UNC base and build src/dst paths.
Apply this diff:
- TargetDirectory = @"\\20.67.234.98\Shared\dst", + TargetDirectory = Path.Combine( + Environment.GetEnvironmentVariable("FILES_UNC_BASE") ?? @"\\localhost\Shared", + "dst"),- SourceDirectory = @"\\20.67.234.98\Shared\src", + SourceDirectory = Path.Combine( + Environment.GetEnvironmentVariable("FILES_UNC_BASE") ?? @"\\localhost\Shared", + "src"),- SourceDirectory = @"\\20.67.234.98\Shared\src", + SourceDirectory = Path.Combine( + Environment.GetEnvironmentVariable("FILES_UNC_BASE") ?? @"\\localhost\Shared", + "src"), - TargetDirectory = @"\\20.67.234.98\Shared\dst", + TargetDirectory = Path.Combine( + Environment.GetEnvironmentVariable("FILES_UNC_BASE") ?? @"\\localhost\Shared", + "dst"),Additionally, consider unique file names per test (e.g., Guid) to avoid cross‑test collisions.
Also applies to: 65-66, 90-93
Frends.Files.Move/Frends.Files.Move/ErrorHandler.cs (1)
1-1: Preserve stack trace on rethrow; wrap only when custom message is provided.Use ExceptionDispatchInfo when no custom message is set.
Apply this diff:
-using System; +using System; +using System.Runtime.ExceptionServices; @@ - if (throwOnFailure) - { - if (string.IsNullOrEmpty(errorMessageOnFailure)) - throw new Exception(exception.Message, exception); - - throw new Exception(errorMessageOnFailure, exception); - } + if (throwOnFailure) + { + if (string.IsNullOrEmpty(errorMessageOnFailure)) + ExceptionDispatchInfo.Capture(exception).Throw(); + throw new Exception(errorMessageOnFailure, exception); + }Also applies to: 23-29
Frends.Files.Move/Frends.Files.Move/Helpers.cs (2)
52-57: Use precise FileAccess/FileShare for copy to avoid unnecessary locks and permissions.Use read-only on source and create/write on target.
Apply this diff:
- await using var sourceStream = ImpersonatedAction.Execute(() => File.Open(sourceFilePath, FileMode.Open), + await using var sourceStream = ImpersonatedAction.Execute( + () => new FileStream(sourceFilePath, FileMode.Open, FileAccess.Read, FileShare.Read), connection, ImpersonatedPart.Source); - await using var targetStream = ImpersonatedAction.Execute(() => File.Open(targetFilePath, FileMode.CreateNew), + await using var targetStream = ImpersonatedAction.Execute( + () => new FileStream(targetFilePath, FileMode.CreateNew, FileAccess.Write, FileShare.None), connection, ImpersonatedPart.Target);
63-69: Consider normalizing source paths to full paths for consistency.Minor consistency/readability improvement.
Apply this diff:
- f => Path.Combine(sourceDirectory, f.Path), + f => Path.GetFullPath(Path.Combine(sourceDirectory, f.Path)),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (22)
.github/workflows/Move_build_and_test_on_main.yml(2 hunks).github/workflows/Move_build_and_test_on_push.yml(2 hunks).gitignore(1 hunks)Frends.Files.Move/Frends.Files.Move.Tests/Frends.Files.Move.Tests.csproj(1 hunks)Frends.Files.Move/Frends.Files.Move.Tests/Helper.cs(2 hunks)Frends.Files.Move/Frends.Files.Move.Tests/ImpersonationTests.cs(2 hunks)Frends.Files.Move/Frends.Files.Move.Tests/RemoteTests.cs(1 hunks)Frends.Files.Move/Frends.Files.Move.Tests/UnitTests.cs(8 hunks)Frends.Files.Move/Frends.Files.Move/Definitions/Connection.cs(1 hunks)Frends.Files.Move/Frends.Files.Move/Definitions/Error.cs(1 hunks)Frends.Files.Move/Frends.Files.Move/Definitions/FileExistsAction.cs(1 hunks)Frends.Files.Move/Frends.Files.Move/Definitions/FileItem.cs(1 hunks)Frends.Files.Move/Frends.Files.Move/Definitions/ImpersonatedPart.cs(1 hunks)Frends.Files.Move/Frends.Files.Move/Definitions/Input.cs(2 hunks)Frends.Files.Move/Frends.Files.Move/Definitions/Options.cs(2 hunks)Frends.Files.Move/Frends.Files.Move/Definitions/Result.cs(1 hunks)Frends.Files.Move/Frends.Files.Move/ErrorHandler.cs(1 hunks)Frends.Files.Move/Frends.Files.Move/Frends.Files.Move.cs(1 hunks)Frends.Files.Move/Frends.Files.Move/Frends.Files.Move.csproj(2 hunks)Frends.Files.Move/Frends.Files.Move/Helpers.cs(1 hunks)Frends.Files.Move/Frends.Files.Move/ImpersonatedAction.cs(1 hunks)Frends.Files.Move/Frends.Files.Move/Move.cs(0 hunks)
💤 Files with no reviewable changes (1)
- Frends.Files.Move/Frends.Files.Move/Move.cs
🧰 Additional context used
📓 Path-based instructions (4)
Frends.*/**/*.cs
⚙️ CodeRabbit configuration file
Frends.*/**/*.cs: Code must follow Microsoft C# coding standards, including:
- PascalCase for public members and task parameters
- Proper naming for abbreviations (Csv, Url, Api)
- Use of var only when type is obvious
- Clean structure and no unused code
Files:
Frends.Files.Move/Frends.Files.Move/Definitions/Result.csFrends.Files.Move/Frends.Files.Move/Definitions/ImpersonatedPart.csFrends.Files.Move/Frends.Files.Move/Definitions/FileItem.csFrends.Files.Move/Frends.Files.Move/Definitions/Connection.csFrends.Files.Move/Frends.Files.Move/ErrorHandler.csFrends.Files.Move/Frends.Files.Move/Definitions/Error.csFrends.Files.Move/Frends.Files.Move.Tests/Helper.csFrends.Files.Move/Frends.Files.Move/Definitions/Input.csFrends.Files.Move/Frends.Files.Move/Definitions/Options.csFrends.Files.Move/Frends.Files.Move/Helpers.csFrends.Files.Move/Frends.Files.Move/ImpersonatedAction.csFrends.Files.Move/Frends.Files.Move/Definitions/FileExistsAction.csFrends.Files.Move/Frends.Files.Move.Tests/UnitTests.csFrends.Files.Move/Frends.Files.Move/Frends.Files.Move.csFrends.Files.Move/Frends.Files.Move.Tests/RemoteTests.csFrends.Files.Move/Frends.Files.Move.Tests/ImpersonationTests.cs
Frends.*/Frends.*/*.cs
⚙️ CodeRabbit configuration file
Frends.*/Frends.*/*.cs: Ensure every public method and class:
- Has
and XML comments
- If the documentation is very long then it can also use element
- Follows Microsoft C# code conventions
- Uses semantic task result documentation (e.g., Success, Error, Data)
Frends.*/Frends.*/*.cs: Validate all task result classes include:
- Success (bool)
- Task-specific return value (e.g. Data, FilePaths)
- Error object with Message and AdditionalInfo
- Ensure result structure is flat, simple, and avoids 3rd-party types.
- Use dynamic or JToken only when the structure is unknown.
Files:
Frends.Files.Move/Frends.Files.Move/ErrorHandler.csFrends.Files.Move/Frends.Files.Move.Tests/Helper.csFrends.Files.Move/Frends.Files.Move/Helpers.csFrends.Files.Move/Frends.Files.Move/ImpersonatedAction.csFrends.Files.Move/Frends.Files.Move.Tests/UnitTests.csFrends.Files.Move/Frends.Files.Move/Frends.Files.Move.csFrends.Files.Move/Frends.Files.Move.Tests/RemoteTests.csFrends.Files.Move/Frends.Files.Move.Tests/ImpersonationTests.cs
Frends.*/Frends.*.Tests/*
⚙️ CodeRabbit configuration file
Frends.*/Frends.*.Tests/*: Confirm unit tests exist and provide at least 80% coverage.
Tests should:
- Load secrets via dotenv
- Use mocking where real systems can't be simulated
- Follow Microsoft unit testing naming and structuring conventions
Files:
Frends.Files.Move/Frends.Files.Move.Tests/Helper.csFrends.Files.Move/Frends.Files.Move.Tests/Frends.Files.Move.Tests.csprojFrends.Files.Move/Frends.Files.Move.Tests/UnitTests.csFrends.Files.Move/Frends.Files.Move.Tests/RemoteTests.csFrends.Files.Move/Frends.Files.Move.Tests/ImpersonationTests.cs
Frends.*/Frends.*/*.csproj
⚙️ CodeRabbit configuration file
Frends.*/Frends.*/*.csproj: Ensure the .csproj targets .NET 6 or 8, uses the MIT license, and includes the following fields:
= Frends
= true
= MIT
Follow Microsoft C# project file conventions.
Files:
Frends.Files.Move/Frends.Files.Move.Tests/Frends.Files.Move.Tests.csprojFrends.Files.Move/Frends.Files.Move/Frends.Files.Move.csproj
🧬 Code graph analysis (11)
Frends.Files.Move/Frends.Files.Move/Definitions/Result.cs (3)
Frends.Files.Move/Frends.Files.Move/Definitions/Error.cs (1)
Error(8-21)Frends.Files.Move/Frends.Files.Move/Definitions/FileItem.cs (1)
FileItem(6-19)Frends.Files.Move/Frends.Files.Move/Frends.Files.Move.cs (1)
Files(15-128)
Frends.Files.Move/Frends.Files.Move/Definitions/ImpersonatedPart.cs (1)
Frends.Files.Move/Frends.Files.Move/Frends.Files.Move.cs (1)
Files(15-128)
Frends.Files.Move/Frends.Files.Move/Definitions/Connection.cs (1)
Frends.Files.Move/Frends.Files.Move/Frends.Files.Move.cs (1)
Files(15-128)
Frends.Files.Move/Frends.Files.Move/ErrorHandler.cs (3)
Frends.Files.Move/Frends.Files.Move/Frends.Files.Move.cs (1)
Files(15-128)Frends.Files.Move/Frends.Files.Move/Definitions/Result.cs (1)
Result(8-27)Frends.Files.Move/Frends.Files.Move/Definitions/Error.cs (1)
Error(8-21)
Frends.Files.Move/Frends.Files.Move/Definitions/Error.cs (1)
Frends.Files.Move/Frends.Files.Move/Frends.Files.Move.cs (1)
Files(15-128)
Frends.Files.Move/Frends.Files.Move/Helpers.cs (2)
Frends.Files.Move/Frends.Files.Move/Frends.Files.Move.cs (3)
Files(15-128)Task(26-43)Task(46-127)Frends.Files.Move/Frends.Files.Move/ImpersonatedAction.cs (2)
Execute(22-32)ImpersonatedAction(12-61)
Frends.Files.Move/Frends.Files.Move/ImpersonatedAction.cs (3)
Frends.Files.Move/Frends.Files.Move/Frends.Files.Move.cs (1)
Files(15-128)Frends.Files.Move/Frends.Files.Move/Definitions/Connection.cs (1)
Connection(9-58)Frends.Files.Move/Frends.Files.Move/Helpers.cs (1)
Helpers(14-108)
Frends.Files.Move/Frends.Files.Move.Tests/UnitTests.cs (4)
Frends.Files.Move/Frends.Files.Move/Definitions/Input.cs (1)
Input(9-32)Frends.Files.Move/Frends.Files.Move/Frends.Files.Move.cs (3)
Task(26-43)Task(46-127)Files(15-128)Frends.Files.Move/Frends.Files.Move/Helpers.cs (1)
Task(49-58)Frends.Files.Move/Frends.Files.Move/Definitions/Connection.cs (1)
Connection(9-58)
Frends.Files.Move/Frends.Files.Move/Frends.Files.Move.cs (8)
Frends.Files.Move/Frends.Files.Move/Helpers.cs (6)
Task(49-58)List(104-107)Helpers(14-108)AssertNoTargetFileConflicts(71-82)GetNonConflictingTargetFilePath(91-102)DeleteExistingFiles(84-89)Frends.Files.Move/Frends.Files.Move/Definitions/Result.cs (1)
Result(8-27)Frends.Files.Move/Frends.Files.Move/ErrorHandler.cs (2)
Result(18-44)ErrorHandler(9-45)Frends.Files.Move/Frends.Files.Move/Definitions/Input.cs (1)
Input(9-32)Frends.Files.Move/Frends.Files.Move/Definitions/Connection.cs (1)
Connection(9-58)Frends.Files.Move/Frends.Files.Move/Definitions/Options.cs (1)
Options(9-50)Frends.Files.Move/Frends.Files.Move/Definitions/FileItem.cs (1)
FileItem(6-19)Frends.Files.Move/Frends.Files.Move/ImpersonatedAction.cs (2)
ImpersonatedAction(12-61)Execute(22-32)
Frends.Files.Move/Frends.Files.Move.Tests/RemoteTests.cs (3)
Frends.Files.Move/Frends.Files.Move/Frends.Files.Move.cs (3)
Files(15-128)Task(26-43)Task(46-127)Frends.Files.Move/Frends.Files.Move/Helpers.cs (2)
Task(49-58)Tuple(16-22)Frends.Files.Move/Frends.Files.Move/Definitions/Connection.cs (1)
Connection(9-58)
Frends.Files.Move/Frends.Files.Move.Tests/ImpersonationTests.cs (5)
Frends.Files.Move/Frends.Files.Move/Frends.Files.Move.cs (3)
Files(15-128)Task(26-43)Task(46-127)Frends.Files.Move/Frends.Files.Move/Definitions/Input.cs (1)
Input(9-32)Frends.Files.Move/Frends.Files.Move/Definitions/Connection.cs (1)
Connection(9-58)Frends.Files.Move/Frends.Files.Move.Tests/Helper.cs (3)
Helper(9-64)CreateTestUser(39-52)DeleteTestUser(54-63)Frends.Files.Move/Frends.Files.Move/Helpers.cs (1)
Task(49-58)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Analyze (csharp)
- GitHub Check: build / Build on windows-latest
🔇 Additional comments (14)
Frends.Files.Move/Frends.Files.Move/Frends.Files.Move.csproj (2)
4-4: LGTM! Framework upgrade to .NET 8.The target framework upgrade from .NET 6 to .NET 8 is appropriate and aligns with the PR's objectives.
23-31: LGTM! Analyzer integration.The addition of FrendsTaskAnalyzers and the AdditionalFiles entry for FrendsTaskMetadata.json support static analysis and metadata packaging, which improves code quality and task documentation.
Frends.Files.Move/Frends.Files.Move/Definitions/ImpersonatedPart.cs (1)
1-12: LGTM! Clean enum definition.The enum is well-documented and follows C# naming conventions. The pragma directive to suppress CS1591 for self-explanatory enum members is acceptable.
Frends.Files.Move/Frends.Files.Move/Definitions/Error.cs (1)
16-20: LGTM! AdditionalInfo property.The use of Exception for additional error context is appropriate and follows the flat structure requirement.
Frends.Files.Move/Frends.Files.Move.Tests/Helper.cs (3)
9-9: LGTM! Static class declaration.Making the class static is appropriate since all its methods are static, preventing accidental instantiation.
44-52: LGTM! Improved resource management.The refactoring to use
using varfor DirectoryEntry instances ensures proper disposal and follows C# best practices.
59-62: LGTM! Consistent resource management.The use of
using varfor DirectoryEntry instances ensures proper cleanup and is consistent with the CreateTestUser refactoring.Frends.Files.Move/Frends.Files.Move.Tests/Frends.Files.Move.Tests.csproj (1)
4-4: LGTM! Test framework alignment.The test project's framework upgrade to .NET 8 aligns with the main project and ensures compatibility.
Frends.Files.Move/Frends.Files.Move/Definitions/Result.cs (1)
10-20: LGTM! Result structure aligns with coding guidelines.The addition of
SuccessandErrorproperties satisfies the coding guidelines requirement for task result classes. As per coding guidelines.Frends.Files.Move/Frends.Files.Move/Definitions/FileItem.cs (1)
11-18: LGTM! Improved documentation.The addition of concrete examples for both local (SourcePath) and UNC (TargetPath) paths improves the API documentation and helps users understand the expected format.
Frends.Files.Move/Frends.Files.Move/Definitions/Input.cs (2)
16-16: LGTM! Improved property naming.The rename from
DirectorytoSourceDirectoryis a breaking change but improves API clarity by explicitly distinguishing the source from the target directory. This aligns well with the broader refactoring objectives.
29-29: LGTM! Updated example.The UNC path example is appropriate and demonstrates remote file operations, which aligns with the PR's introduction of Connection-based operations.
Frends.Files.Move/Frends.Files.Move/Definitions/FileExistsAction.cs (2)
11-21: Explicit enum values look good.Clear, stable API surface.
8-11: Use Target impersonation for target cleanup in catchReplace the impersonation part in the catch block (Frends.Files.Move/Frends.Files.Move/Frends.Files.Move.cs:117–120):
- ImpersonatedAction.Execute( - () => Helpers.DeleteExistingFiles(fileResults.Select(x => x.TargetPath)), - connection, ImpersonatedPart.Source); + ImpersonatedAction.Execute( + () => Helpers.DeleteExistingFiles(fileResults.Select(x => x.TargetPath)), + connection, ImpersonatedPart.Target);This ensures remote target deletions are rolled back correctly.
Likely an incorrect or invalid review comment.
| /// <summary> | ||
| /// Method handling errors in the task | ||
| /// </summary> | ||
| public static class ErrorHandler |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add XML docs for public class and method (guideline requirement).
Provide minimal, meaningful examples.
Apply this diff:
/// <summary>
-/// Method handling errors in the task
+/// Method handling errors in the task
/// </summary>
+/// <example>
+/// try
+/// {
+/// // Do work
+/// }
+/// catch (Exception ex)
+/// {
+/// return ErrorHandler.Handle(ex, options.ThrowErrorOnFailure, options.ErrorMessageOnFailure);
+/// }
+/// </example>
public static class ErrorHandler
{
/// <summary>
/// Handler for exceptions
/// </summary>
/// <param name="exception">Caught exception</param>
/// <param name="throwOnFailure">Frends flag</param>
/// <param name="errorMessageOnFailure">Message to throw in error event</param>
/// <returns>Throw exception if a flag is true, else return Result with Error info</returns>
+ /// <example>
+ /// var result = ErrorHandler.Handle(ex, true, "Operation failed");
+ /// // This will throw a new Exception with message "Operation failed"
+ /// </example>
public static Result Handle(Also applies to: 11-22, 35-44
🤖 Prompt for AI Agents
In Frends.Files.Move/ErrorHandler.cs around lines 6-9 (also apply to lines 11-22
and 35-44), the public class and its public method(s) lack XML <example>
documentation per project guidelines; add minimal, meaningful <example> XML tags
for the public class and each public method showing a short usage snippet or
call pattern (one-liner or minimal code description) so the generated docs
include an example; ensure examples are valid XML CDATA or escaped as needed and
placed inside the existing /// comment blocks directly above the class and
method declarations.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
Frends.Files.Move/CHANGELOG.md(1 hunks)Frends.Files.Move/Frends.Files.Move/Frends.Files.Move.csproj(2 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
Frends.*/CHANGELOG.md
⚙️ CodeRabbit configuration file
Frends.*/CHANGELOG.md: Validate format against Keep a Changelog (https://keepachangelog.com/en/1.0.0/)
Include all functional changes and indicate breaking changes with upgrade notes.
Avoid notes like "refactored xyz" unless it affects functionality.
Files:
Frends.Files.Move/CHANGELOG.md
Frends.*/Frends.*/*.csproj
⚙️ CodeRabbit configuration file
Frends.*/Frends.*/*.csproj: Ensure the .csproj targets .NET 6 or 8, uses the MIT license, and includes the following fields:
= Frends
= true
= MIT
Follow Microsoft C# project file conventions.
Files:
Frends.Files.Move/Frends.Files.Move/Frends.Files.Move.csproj
🪛 LanguageTool
Frends.Files.Move/CHANGELOG.md
[grammar] ~6-~6: There might be a mistake here.
Context: ...files between remote places. ### Changed - Added options to handle errors - Moved p...
(QB_NEW_EN)
[grammar] ~8-~8: There might be a mistake here.
Context: ...ved parameters related to impersonation to new Connection tab - Add Success and Er...
(QB_NEW_EN)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Analyze (csharp)
- GitHub Check: build / Check Code Quality
- GitHub Check: build / Build on windows-latest
Frends.Files.Move/CHANGELOG.md
Outdated
| - Added options to handle errors | ||
| - Moved parameters related to impersonation to new Connection tab | ||
| - Add Success and Error parameters to Result object | ||
| - Upgrade target framework to net8.0 | ||
| - Renamed input Directory to SourceDirectory |
There was a problem hiding this comment.
Document the breaking API changes and upgrade steps.
The 2.0.0 release introduces breaking surface changes (e.g., mandatory Connection tab, renamed SourceDirectory, new result contract), but the entry just lists them without upgrade guidance. Please add an explicit “### Breaking” (or similar) section that spells out the concrete API changes and how existing Tasks should migrate, per our changelog guidelines. As per coding guidelines.
🧰 Tools
🪛 LanguageTool
[grammar] ~8-~8: There might be a mistake here.
Context: ...ved parameters related to impersonation to new Connection tab - Add Success and Er...
(QB_NEW_EN)
There was a problem hiding this comment.
We should describe this better, which parameters moved where and add this 'Breaking'
There was a problem hiding this comment.
@MichalFrends1, you're absolutely right. The CHANGELOG needs a dedicated "Breaking Changes" section with clear details about the API changes and migration steps. Here's a suggested rewrite for the 2.0.0 entry:
## [2.0.0] - 2025-10-14
### Breaking Changes
- **New `Connection` parameter**: The `Move` method now requires a `Connection` parameter between `Input` and `Options`:
- **Before**: `Move(Input input, Options options, CancellationToken cancellationToken)`
- **After**: `Move(Input input, Connection connection, Options options, CancellationToken cancellationToken)`
- **Credential parameters moved from `Options` to `Connection`**:
- Removed from `Options`: `UseGivenUserCredentialsForRemoteConnections`, `UserName`, `Password`
- Added to `Connection`: `SourceIsRemote`, `SourceUserName`, `SourcePassword`, `TargetIsRemote`, `TargetUserName`, `TargetPassword`
- **Input property renamed**: `Directory` → `SourceDirectory`
- **Result object structure changed**:
- Added `Success` (bool) property
- Added `Error` object with `Message` and `AdditionalInfo` properties
- **Options changes**:
- Added `ThrowErrorOnFailure` (default: true) to control error handling behavior
- Added `ErrorMessageOnFailure` to customize error messages
- Added `CreateTargetDirectories` (default: true) to control target directory creation
**Upgrade Notes**:
- Update your `Move` task calls to include the new `Connection` parameter
- Migrate credential settings from `Options` to the new `Connection` tab:
- If you were using `UseGivenUserCredentialsForRemoteConnections`, set `SourceIsRemote` and/or `TargetIsRemote` to `true` in `Connection`
- Move `UserName` and `Password` values to `SourceUserName`/`SourcePassword` or `TargetUserName`/`TargetPassword` as appropriate
- Update references to `input.Directory` to use `input.SourceDirectory` instead
- Update code that expects a simple file list result to handle the new `Success` and `Error` properties
### Fixed
- Fixed issue with impersonation when moving files between remote locations
### Changed
- Target framework upgraded from .NET 6.0 to .NET 8.0This structure follows the Keep a Changelog format and provides clear guidance for users upgrading from 1.x to 2.0.0.
Frends.Files.Move/CHANGELOG.md
Outdated
| - Added options to handle errors | ||
| - Moved parameters related to impersonation to new Connection tab | ||
| - Add Success and Error parameters to Result object | ||
| - Upgrade target framework to net8.0 | ||
| - Renamed input Directory to SourceDirectory |
There was a problem hiding this comment.
We should describe this better, which parameters moved where and add this 'Breaking'
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
Frends.Files.Move/Frends.Files.Move/Definitions/Connection.cs (1)
6-9: Add class-level<example>documentation.Public classes under
Frends.Files.Movemust include<summary>and<example>XML docs. Please add an example showing typical remote source/target credentials so the generated docs meet the guideline.Apply this diff:
/// <summary> /// Connection parameters to set file access permissions. /// </summary> +/// <example> +/// <![CDATA[ +/// new Connection +/// { +/// SourceIsRemote = true, +/// SourceUserName = "WORKGROUP\\John", +/// SourcePassword = "secret", +/// TargetIsRemote = true, +/// TargetUserName = "WORKGROUP\\Jane", +/// TargetPassword = "secret" +/// }; +/// ]]> +/// </example> public class Connection
🧹 Nitpick comments (4)
Frends.Files.Move/Frends.Files.Move/migration.json (1)
10-14: Clean up legacy property after copy.After copying
Input.DirectorytoInput.SourceDirectory, delete the legacy field to avoid stale data.Apply:
{ "Type": "Copy", "Source": "Input.Directory", "Target": "Input.SourceDirectory" }, + { + "Type": "Delete", + "Source": "Input.Directory" + },Frends.Files.Move/Frends.Files.Move/Helpers.cs (3)
31-45: Precompile regex and use case-insensitive matching.Avoid recompiling per file and align with Windows’ case-insensitive FS semantics.
Apply:
- if (pattern.StartsWith("<regex>")) + if (pattern.StartsWith("<regex>")) { var regexPattern = pattern[7..]; - - var matchingFiles = Directory.GetFiles(directoryPath, "*", SearchOption.AllDirectories) - .Where(file => Regex.IsMatch(Path.GetFileName(file), regexPattern)) + var regex = new Regex(regexPattern, RegexOptions.Compiled | RegexOptions.IgnoreCase); + var matchingFiles = Directory.GetFiles(directoryPath, "*", SearchOption.AllDirectories) + .Where(file => regex.IsMatch(Path.GetFileName(file))) .Select(file => { var relativePath = Path.GetRelativePath(directoryPath, file); return new FilePatternMatch(relativePath, relativePath); }) .ToList(); return new PatternMatchingResult(matchingFiles); }Optional: if you intend to support directory-level regex, match against
relativePathinstead ofPath.GetFileName(file).
53-62: Use explicit FileAccess/FileShare for correctness and least privilege.Open source as read-only and target as write-only to avoid requiring unnecessary permissions.
Apply:
- await using var sourceStream = ImpersonatedAction.Execute(() => File.Open(sourceFilePath, FileMode.Open), + await using var sourceStream = ImpersonatedAction.Execute( + () => new FileStream(sourceFilePath, FileMode.Open, FileAccess.Read, FileShare.Read), connection, ImpersonatedPart.Source); - await using var targetStream = ImpersonatedAction.Execute(() => File.Open(targetFilePath, FileMode.CreateNew), + await using var targetStream = ImpersonatedAction.Execute( + () => new FileStream(targetFilePath, FileMode.CreateNew, FileAccess.Write, FileShare.None), connection, ImpersonatedPart.Target);
95-106: Base rename on target filename (more robust when names differ).Current logic derives the stem from source; if mapping renames, suffixing should respect the target’s name.
Apply:
- var count = 1; + var count = 1; while (File.Exists(targetFilePath)) { - var tempFileName = $"{Path.GetFileNameWithoutExtension(sourceFilePath)}({count++})"; - targetFilePath = Path.Combine(Path.GetDirectoryName(targetFilePath) ?? string.Empty, - path2: tempFileName + Path.GetExtension(sourceFilePath)); + var tempFileName = $"{Path.GetFileNameWithoutExtension(targetFilePath)}({count++})"; + targetFilePath = Path.Combine( + Path.GetDirectoryName(targetFilePath) ?? string.Empty, + tempFileName + Path.GetExtension(targetFilePath)); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
Frends.Files.Move/CHANGELOG.md(1 hunks)Frends.Files.Move/Frends.Files.Move.Tests/UnitTests.cs(8 hunks)Frends.Files.Move/Frends.Files.Move/Definitions/Connection.cs(1 hunks)Frends.Files.Move/Frends.Files.Move/Definitions/Error.cs(1 hunks)Frends.Files.Move/Frends.Files.Move/Frends.Files.Move.cs(1 hunks)Frends.Files.Move/Frends.Files.Move/Helpers.cs(1 hunks)Frends.Files.Move/Frends.Files.Move/migration.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- Frends.Files.Move/Frends.Files.Move/Definitions/Error.cs
- Frends.Files.Move/Frends.Files.Move/Frends.Files.Move.cs
🧰 Additional context used
📓 Path-based instructions (4)
Frends.*/**/*.cs
⚙️ CodeRabbit configuration file
Frends.*/**/*.cs: Code must follow Microsoft C# coding standards, including:
- PascalCase for public members and task parameters
- Proper naming for abbreviations (Csv, Url, Api)
- Use of var only when type is obvious
- Clean structure and no unused code
Files:
Frends.Files.Move/Frends.Files.Move/Definitions/Connection.csFrends.Files.Move/Frends.Files.Move.Tests/UnitTests.csFrends.Files.Move/Frends.Files.Move/Helpers.cs
Frends.*/Frends.*.Tests/*
⚙️ CodeRabbit configuration file
Frends.*/Frends.*.Tests/*: Confirm unit tests exist and provide at least 80% coverage.
Tests should:
- Load secrets via dotenv
- Use mocking where real systems can't be simulated
- Follow Microsoft unit testing naming and structuring conventions
Files:
Frends.Files.Move/Frends.Files.Move.Tests/UnitTests.cs
Frends.*/Frends.*/*.cs
⚙️ CodeRabbit configuration file
Frends.*/Frends.*/*.cs: Ensure every public method and class:
- Has
and XML comments
- If the documentation is very long then it can also use element
- Follows Microsoft C# code conventions
- Uses semantic task result documentation (e.g., Success, Error, Data)
Frends.*/Frends.*/*.cs: Validate all task result classes include:
- Success (bool)
- Task-specific return value (e.g. Data, FilePaths)
- Error object with Message and AdditionalInfo
- Ensure result structure is flat, simple, and avoids 3rd-party types.
- Use dynamic or JToken only when the structure is unknown.
Files:
Frends.Files.Move/Frends.Files.Move.Tests/UnitTests.csFrends.Files.Move/Frends.Files.Move/Helpers.cs
Frends.*/CHANGELOG.md
⚙️ CodeRabbit configuration file
Frends.*/CHANGELOG.md: Validate format against Keep a Changelog (https://keepachangelog.com/en/1.0.0/)
Include all functional changes and indicate breaking changes with upgrade notes.
Avoid notes like "refactored xyz" unless it affects functionality.
Files:
Frends.Files.Move/CHANGELOG.md
🧬 Code graph analysis (3)
Frends.Files.Move/Frends.Files.Move/Definitions/Connection.cs (1)
Frends.Files.Move/Frends.Files.Move/Frends.Files.Move.cs (1)
Files(15-129)
Frends.Files.Move/Frends.Files.Move.Tests/UnitTests.cs (6)
Frends.Files.Move/Frends.Files.Move/Definitions/Input.cs (1)
Input(9-32)Frends.Files.Move/Frends.Files.Move/Definitions/Options.cs (1)
Options(9-50)Frends.Files.Move/Frends.Files.Move.Tests/Helper.cs (3)
Helper(9-64)CreateTestFiles(11-32)DeleteTestFolder(34-37)Frends.Files.Move/Frends.Files.Move/Frends.Files.Move.cs (3)
Task(26-43)Task(46-128)Files(15-129)Frends.Files.Move/Frends.Files.Move/Helpers.cs (1)
Task(53-62)Frends.Files.Move/Frends.Files.Move/Definitions/Connection.cs (1)
Connection(9-60)
Frends.Files.Move/Frends.Files.Move/Helpers.cs (2)
Frends.Files.Move/Frends.Files.Move/ImpersonatedAction.cs (2)
Execute(22-32)ImpersonatedAction(12-61)Frends.Files.Move/Frends.Files.Move/Definitions/Connection.cs (1)
Connection(9-60)
🪛 LanguageTool
Frends.Files.Move/CHANGELOG.md
[grammar] ~7-~7: There might be a mistake here.
Context: ...en regex patterns were used. ### Changed - Added options to handle errors - Add Suc...
(QB_NEW_EN)
[grammar] ~11-~11: There might be a mistake here.
Context: ...work to net8.0 #### Breaking Changes - Moved parameters related to impersonatio...
(QB_NEW_EN)
[grammar] ~12-~12: There might be a mistake here.
Context: ...ved parameters related to impersonation to new Connection tab - Renamed input Di...
(QB_NEW_EN)
[grammar] ~13-~13: There might be a mistake here.
Context: ...impersonation to new Connection tab - Renamed input Directory to SourceDirectory ## ...
(QB_NEW_EN)
🪛 markdownlint-cli2 (0.18.1)
Frends.Files.Move/CHANGELOG.md
11-11: Headings must start at the beginning of the line
(MD023, heading-start-left)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Analyze (csharp)
- GitHub Check: build / Build on windows-latest
- GitHub Check: build / Check Code Quality
🔇 Additional comments (1)
Frends.Files.Move/Frends.Files.Move.Tests/UnitTests.cs (1)
186-215: Great addition: regression test for regex + preserved directory structure.Covers the previously reported flattening bug; assertions look solid.
| - Added options to handle errors | ||
| - Add Success and Error parameters to Result object | ||
| - Upgrade target framework to net8.0 | ||
| #### Breaking Changes | ||
| - Moved parameters related to impersonation to new Connection tab | ||
| - Renamed input Directory to SourceDirectory | ||
|
|
There was a problem hiding this comment.
Document breaking changes with upgrade guidance and fix heading indentation.
Lines 11‑14 are still not in Keep a Changelog shape: the indented #### Breaking Changes renders as a code block (MD023) and the bullets only restate the changes without telling users how to migrate. We already asked for a proper “### Breaking Changes” section spelling out the new Connection parameter, credential moves, renamed SourceDirectory, result contract, and the concrete upgrade steps/tasks must perform. Please restructure this section (left-align the heading) and add explicit migration guidance so 2.0 adopters can update their Tasks without guesswork. As per coding guidelines.
🧰 Tools
🪛 LanguageTool
[grammar] ~11-~11: There might be a mistake here.
Context: ...work to net8.0 #### Breaking Changes - Moved parameters related to impersonatio...
(QB_NEW_EN)
[grammar] ~12-~12: There might be a mistake here.
Context: ...ved parameters related to impersonation to new Connection tab - Renamed input Di...
(QB_NEW_EN)
[grammar] ~13-~13: There might be a mistake here.
Context: ...impersonation to new Connection tab - Renamed input Directory to SourceDirectory ## ...
(QB_NEW_EN)
🪛 markdownlint-cli2 (0.18.1)
11-11: Headings must start at the beginning of the line
(MD023, heading-start-left)
🤖 Prompt for AI Agents
In Frends.Files.Move/CHANGELOG.md around lines 8 to 14, the breaking-changes
block is indented (renders as a code block) and lacks concrete upgrade guidance;
change the heading to a left‑aligned "### Breaking Changes" and replace the
current bullet restatement with explicit entries describing: the new Connection
tab and its new parameter names, exactly which impersonation/credential fields
moved and their new names/locations, the rename of Directory to SourceDirectory
(including any impacts on consumers and JSON/property names), the updated Result
contract (Success and Error parameters and their types), and then add
step‑by‑step migration instructions that 2.0 adopters must perform (e.g., open
each Task, move credentials to Connection, rename Directory→SourceDirectory,
update any code/config refs to Result.Success/Result.Error and adjust
deserialization if needed). Ensure items are left‑aligned, use "### Breaking
Changes" as the heading, and provide concrete examples of the exact parameter
names to change and the minimal code/config edits required.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
Frends.Files.Move/Frends.Files.Move/migration.json (1)
15-18: Set Value to an actual empty string literal.
"Value": "\"\""assigns two quote characters, not an empty string. Update Line 17 to use"Value": ""so the migration setsOptions.ErrorMessageOnFailureto empty as intended.- { - "Type": "Set", - "Target": "Options.ErrorMessageOnFailure", - "Value": "\"\"" - }, + { + "Type": "Set", + "Target": "Options.ErrorMessageOnFailure", + "Value": "" + },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Frends.Files.Move/Frends.Files.Move/migration.json(1 hunks)
🧰 Additional context used
🪛 Biome (2.1.2)
Frends.Files.Move/Frends.Files.Move/migration.json
[error] 38-38: Expected a property but instead found '}'.
Expected a property here.
(parse)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Analyze (csharp)
- GitHub Check: build / Build on windows-latest
- GitHub Check: build / Check Code Quality
[]# Frends Task Pull Request
Summary
Review Checklist
1. Frends Task Project Files
Frends.*/Frends.*/*.csproj<PackageLicenseExpression>MIT</PackageLicenseExpression>)<Version><Authors>Frends</Authors><Description><RepositoryUrl><GenerateDocumentationFile>true</GenerateDocumentationFile>2. File: FrendsTaskMetadata.json
Frends.*/Frends.*/FrendsTaskMetadata.json3. File: README.md
Frends.*/README.md4. File: CHANGELOG.md
Frends.*/CHANGELOG.md5. File: migration.json
Frends.*/Frends.*/migration.json6. Source Code Documentation
Frends.*/Frends.*/*.cs<summary>XML comments<example>XML comments<frendsdocs>XML comments, if needed7. GitHub Actions Workflows
.github/workflows/*.yml*_test.yml*_main.yml*_release.yml8. Task Result Object Structure
Frends.*/Frends.*/*.csSuccess(bool)Additional Notes
Summary by CodeRabbit
New Features
Refactor
Tests
Chores