Skip to content

Conversation

fireairforce
Copy link
Contributor

WIP

@xusd320 xusd320 changed the title refactor: use turbo-fs-task to implement output.clean [WIP] refactor: use turbo-fs-task to implement output.clean Jun 10, 2025
@afc163 afc163 requested a review from Copilot September 2, 2025 07:06
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR refactors the output directory cleaning functionality to use turbo-fs-task operations instead of direct filesystem operations. The change replaces standard library fs operations with turbo-tasks-fs abstractions for better integration with the async task system.

  • Removes direct filesystem operations and replaces them with turbo-tasks-fs operations
  • Implements new delete_file and clean_directory functions as turbo-tasks operations
  • Updates the project emission flow to use the new cleaning mechanism

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

let dist_root = self.dist_root();
tracing::info!("Cleaning dist directory: {}", dist_root.await?.path);

let _ = clean_directory(dist_root.to_resolved().await?).connect().resolve().await?;
Copy link

Copilot AI Sep 2, 2025

Choose a reason for hiding this comment

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

The result of clean_directory is being discarded with let _, which ignores potential errors. Since this is a cleaning operation that could fail, the error should be handled properly, either by logging a warning or propagating the error.

Suggested change
let _ = clean_directory(dist_root.to_resolved().await?).connect().resolve().await?;
clean_directory(dist_root.to_resolved().await?).connect().resolve().await?;

Copilot uses AI. Check for mistakes.

Comment on lines +1222 to +1226
let _ = delete_file(entry_path.to_resolved().await?).connect().resolve().await?;
Ok::<(), anyhow::Error>(())
}
turbo_tasks_fs::DirectoryEntry::Directory(_) => {
let _ = clean_directory(entry_path.to_resolved().await?).connect().resolve().await?;
Copy link

Copilot AI Sep 2, 2025

Choose a reason for hiding this comment

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

The results of delete_file and clean_directory operations are being discarded with let _, which ignores potential errors during the cleaning process. These operations should handle errors properly or at least log failures since silent failures during cleanup could leave the directory in an inconsistent state.

Copilot uses AI. Check for mistakes.

Comment on lines +1222 to +1226
let _ = delete_file(entry_path.to_resolved().await?).connect().resolve().await?;
Ok::<(), anyhow::Error>(())
}
turbo_tasks_fs::DirectoryEntry::Directory(_) => {
let _ = clean_directory(entry_path.to_resolved().await?).connect().resolve().await?;
Copy link

Copilot AI Sep 2, 2025

Choose a reason for hiding this comment

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

The results of delete_file and clean_directory operations are being discarded with let _, which ignores potential errors during the cleaning process. These operations should handle errors properly or at least log failures since silent failures during cleanup could leave the directory in an inconsistent state.

Copilot uses AI. Check for mistakes.

Comment on lines +1238 to 1240
for task in delete_tasks {
task.await?;
}
Copy link

Copilot AI Sep 2, 2025

Choose a reason for hiding this comment

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

The delete operations are being executed sequentially in a for loop, which could be slow for directories with many files. Consider using futures::future::try_join_all(delete_tasks) or similar to execute the delete operations concurrently for better performance.

Copilot uses AI. Check for mistakes.

@xusd320 xusd320 force-pushed the next branch 5 times, most recently from 2ab3074 to 3345bb8 Compare September 12, 2025 08:03
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