Solution by luchali · Pull Request #344 · mate-academy/node_move-files

luchali · 2026-02-17T17:31:53Z

No description provided.

mateacademy-ai-mentor

Review Summary

I am requesting changes because there are a few critical path-handling issues that can break required behavior of the CLI.

Critical issues to fix before this can pass:

When fs.stat(destination) throws because the destination path doesn’t exist, you currently set destPath = destination and proceed, which can attempt a rename into a non‑existent parent directory. The requirement is to throw a clear error when the destination directory does not exist (e.g. node index file.txt ./someDir/test when someDir is missing). Update your logic (in src/app.js, around the setDestPath helper) to check path.dirname(destination) and explicitly error if that directory is missing instead of silently accepting the path.
You call setDestPath(destination, absoluteSource) and then rename with fs.rename(absoluteSource, destPath) where absoluteSource is absolute but destPath may still be a relative path. This mixed use of absolute and relative paths can lead to inconsistent behavior; make setDestPath always return an absolute destination (e.g. using path.resolve) and feed that value directly into fs.rename.
You currently compute absoluteDestination = path.resolve(destination) and compare it to absoluteSource before appending the basename for directory destinations. This can miss a true no‑op case where the final resolved target (after path.join(destination, path.basename(source))) is the same as the source. Instead, resolve the final destPath to an absolute path first and then compare that to absoluteSource before calling fs.rename.

As smaller improvements, ensure any error thrown from your helpers is caught in the main CLI flow so the process exits cleanly with a friendly message, and consider removing unused variables like the current absoluteDestination. You’ve already covered the core rename behavior and file-vs-directory handling well; once you tighten these path-resolution and directory-existence checks, your solution will fully align with the task requirements.

✨ Please rate this review in the Platform's notification center¹. I will be grateful for your feedback! ✨

Important

Fix the issues, commit the changes, and then re-request my review using the Re-request review button² in the Reviewers tab.

Tip

If you have any comments about this review, please leave a message in the Add a comment section below and mention me with the @mateacademy-ai-mentor tag³.
I will read your message during my next review of your pull request. Please do not reply directly to my comments, as that feature is not supported yet.

Caution

Do not close the pull request until the review is complete. Closing it early can lead to unexpected behavior and disrupt the review process.

mateacademy-ai-mentor · 2026-02-17T17:35:14Z

src/app.js

+    try {
+      const destStat = await fs.stat(destination);
+
+      if (destStat.isDirectory()) {
+        destPath = path.join(destination, path.basename(source));
+      } else {
+        destPath = destination;
+      }
+    } catch (error) {
+      destPath = destination;


When fs.stat(destination) throws (destination doesn't exist) you set destPath = destination (so a rename is attempted into a path whose parent directory might not exist). The task requires throwing an error when the destination directory doesn’t exist (e.g. moving to ./someDir/test when someDir is missing). Check the parent directory (path.dirname(destination)) and throw a clear error if it doesn't exist instead of accepting the path here.

mateacademy-ai-mentor · 2026-02-17T17:35:14Z

src/app.js

+  const destPath = await setDestPath(destination, absoluteSource);
+
+  try {
+    await fs.rename(absoluteSource, destPath);


You call setDestPath(destination, absoluteSource) and then pass the returned destPath to fs.rename(absoluteSource, destPath). absoluteSource is absolute but destPath can be the original (relative) destination, which mixes absolute and relative paths — this can be inconsistent and cause unexpected behavior. Return an absolute destination from setDestPath (e.g. with path.resolve) and use that for fs.rename.

mateacademy-ai-mentor · 2026-02-17T17:35:14Z

src/app.js

+  const absoluteSource = path.resolve(source);
+  const absoluteDestination = path.resolve(destination);
+
+  if (absoluteSource === absoluteDestination) {
+    return;


You compute absoluteDestination = path.resolve(destination) and compare it to absoluteSource before resolving the final destination. That check can miss cases where the final destination (after appending basename for directories) equals the source. Resolve the final destPath to an absolute path first and compare absoluteSource to that final absolute destination before proceeding.

mateacademy-ai-mentor

Sorry, the pull request review was unsuccessful for the following reasons:

Warning

No changes were made in the pull request since the last review. Please make sure that you have added files to the pull request and committed the changes.
_{[CODE: NCM]}

If the issue is within your control, please address it.
The next review attempt will automatically launch in about 1 hour.
If you believe this is an error or need assistance, please contact the support team.

mateacademy-ai-mentor

Sorry, the pull request review was unsuccessful for the following reasons:

Warning

No changes were made in the pull request since the last review. Please make sure that you have added files to the pull request and committed the changes.
_{[CODE: NCM]}

If the issue is within your control, please address it.
The next review attempt will automatically launch in about 1 hour.
If you believe this is an error or need assistance, please contact the support team.

mateacademy-ai-mentor

Sorry, the pull request review was unsuccessful for the following reasons:

Warning

No changes were made in the pull request since the last review. Please make sure that you have added files to the pull request and committed the changes.
_{[CODE: NCM]}

If the issue is within your control, please address it.
The next review attempt will automatically launch in about 1 hour.
If you believe this is an error or need assistance, please contact the support team.

mateacademy-ai-mentor

Review Summary

I am requesting changes because there is still a critical correctness issue that can hide real errors from the user. In src/app.js, you currently return early when absoluteSource === destPath before calling your source validation logic; if the source file does not actually exist, this path will silently no-op instead of throwing the required missing-source error. Please move source validation (e.g. validateSource) so it runs before this equality/no-op check, or otherwise ensure the source is statted and confirmed to exist before you skip the move.

Two smaller but important robustness fixes are also needed: (1) your trailing-slash detection only checks destination.endsWith(path.sep), which on Windows will miss a user-typed /, so update the condition to accept both '/' and path.sep; and (2) when a trailing-slash destination directory does not exist, you currently throw a generic Destination path is invalid!, but the requirement is to clearly indicate that the destination directory does not exist, so align this message with your other missing-directory errors. Functionally you are very close, and these targeted adjustments will make your CLI more predictable, cross-platform friendly, and aligned with the task’s error-handling semantics.

✨ Please rate this review in the Platform's notification center¹. I will be grateful for your feedback! ✨

Important

Fix the issues, commit the changes, and then re-request my review using the Re-request review button² in the Reviewers tab.

Tip

If you have any comments about this review, please leave a message in the Add a comment section below and mention me with the @mateacademy-ai-mentor tag³.
I will read your message during my next review of your pull request. Please do not reply directly to my comments, as that feature is not supported yet.

Caution

Do not close the pull request until the review is complete. Closing it early can lead to unexpected behavior and disrupt the review process.

mateacademy-ai-mentor · 2026-02-18T23:03:45Z

src/app.js

+async function setDestPath(destination, source) {
+  let destPath;
+
+  if (destination.endsWith(path.sep)) {


Checking only destination.endsWith(path.sep) can miss when users type a trailing '/' on platforms where path.sep is '\' (Windows). Consider checking both '/' and path.sep (e.g. destination.endsWith('/') || destination.endsWith(path.sep)) so trailing-slash detection is robust across platforms.

mateacademy-ai-mentor · 2026-02-18T23:03:45Z

src/app.js

+      }
+      destPath = path.join(destination, path.basename(source));
+    } catch (error) {
+      throw new Error('Destination path is invalid!');


When the attempt to stat a trailing-slash destination fails you throw a generic 'Destination path is invalid!'. The task requires that non-existent destination directories produce a clear error; prefer a clearer message such as 'Destination directory does not exist' (consistent with other branches) so callers/tests can rely on consistent semantics.

mateacademy-ai-mentor · 2026-02-18T23:03:45Z

src/app.js

+    return;
+  }
+
+  if (absoluteSource === destPath) {


You short-circuit and return if absoluteSource === destPath before calling validateSource. If the source file does not exist this will silently return instead of reporting the missing source. Move source validation before this equality check (or at least ensure the source exists when you skip the operation) so attempting to move a non-existent file still produces an error as expected.

mateacademy-ai-mentor

Review Summary

Excellent work on this submission! I am approving your solution as you've successfully addressed all the previous feedback. The logic is now much more robust, particularly with the source validation correctly happening before the self-move check. The platform-agnostic trailing slash detection and specific error messages are great improvements. Your code now correctly handles all the required move and rename scenarios. Well done!

✨ Please rate this review in the Platform's notification center¹. I will be grateful for your feedback! ✨

Rate AI review example ↩

Solution

8ca224e

mateacademy-ai-mentor suggested changes Feb 17, 2026

View reviewed changes

fix solution

fb0cbcc

luchali requested a review from mateacademy-ai-mentor February 18, 2026 19:34

mateacademy-ai-mentor reviewed Feb 18, 2026

View reviewed changes

luchali requested a review from mateacademy-ai-mentor February 18, 2026 19:35

mateacademy-ai-mentor reviewed Feb 18, 2026

View reviewed changes

fixed solution

bcbdca0

luchali requested a review from mateacademy-ai-mentor February 18, 2026 22:04

mateacademy-ai-mentor suggested changes Feb 18, 2026

View reviewed changes

fix

53e9225

luchali requested a review from mateacademy-ai-mentor February 19, 2026 13:08

mateacademy-ai-mentor approved these changes Feb 19, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Solution#344

Solution#344
luchali wants to merge 4 commits intomate-academy:masterfrom
luchali:develop

luchali commented Feb 17, 2026

Uh oh!

mateacademy-ai-mentor left a comment

Uh oh!

mateacademy-ai-mentor Feb 17, 2026

Uh oh!

mateacademy-ai-mentor Feb 17, 2026

Uh oh!

mateacademy-ai-mentor Feb 17, 2026

Uh oh!

mateacademy-ai-mentor left a comment

Uh oh!

mateacademy-ai-mentor left a comment

Uh oh!

mateacademy-ai-mentor left a comment

Uh oh!

mateacademy-ai-mentor left a comment

Uh oh!

mateacademy-ai-mentor Feb 18, 2026

Uh oh!

mateacademy-ai-mentor Feb 18, 2026

Uh oh!

mateacademy-ai-mentor Feb 18, 2026

Uh oh!

mateacademy-ai-mentor left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

luchali commented Feb 17, 2026

Uh oh!

mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

Review Summary

Footnotes

Uh oh!

mateacademy-ai-mentor Feb 17, 2026

Choose a reason for hiding this comment

Uh oh!

mateacademy-ai-mentor Feb 17, 2026

Choose a reason for hiding this comment

Uh oh!

mateacademy-ai-mentor Feb 17, 2026

Choose a reason for hiding this comment

Uh oh!

mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

Uh oh!

mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

Uh oh!

mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

Uh oh!

mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

Review Summary

Footnotes

Uh oh!

mateacademy-ai-mentor Feb 18, 2026

Choose a reason for hiding this comment

Uh oh!

mateacademy-ai-mentor Feb 18, 2026

Choose a reason for hiding this comment

Uh oh!

mateacademy-ai-mentor Feb 18, 2026

Choose a reason for hiding this comment

Uh oh!

mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

Review Summary

Footnotes

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants