Conversation
|
Warning Rate limit exceeded@alexandretrotel has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 24 minutes and 2 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
WalkthroughAdds a YAML-backed configuration subsystem, expands the CLI with granular include/exclude filters and an init subcommand, makes scanning/filtering config-driven and gitignore-aware, refactors main to load/merge CLI+file config and exposes run_with_config/handle_init_subcommand, and updates tests and CI. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI as "cli::create_commands"
participant Main as "main (handle_init_subcommand / run_with_config)"
participant Config as "config (config_from_matches, discover, merge)"
participant GitIgnore as "gitignore::build_gitignore"
participant Scanner as "file_processing::get_directory_structure"
participant FS as "Filesystem"
User->>CLI: invoke command / subcommand
CLI->>Main: return ArgMatches
Main->>Main: handle_init_subcommand(matches)?
alt init invoked
Main->>FS: write fyai.yaml (local/global) with --force guard
Main->>User: print location and exit
else normal run
Main->>Config: config_from_matches(matches) -> (cli_config, explicit)
Main->>Config: discover_config_file() -> FileConfig?
Config->>Config: merge_config(file_config, cli_config, explicit) -> Config
Main->>GitIgnore: build_gitignore(root, IGNORED_FILES, IGNORED_DIRS, &Config)
GitIgnore->>FS: read .gitignore (optional)
Main->>Scanner: get_directory_structure(root, gitignore, IGNORED_DIRS, &Config)
Scanner->>GitIgnore: consult gitignore rules (if respect_gitignore)
Scanner->>FS: traverse filesystem and apply include/exclude rules
Scanner->>Main: return structure + files
Main->>FS: write output file
Main->>User: exit
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
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: 2
🧹 Nitpick comments (8)
src/tests/clipboard_tests.rs (1)
18-29: Consider simplifying the error handling logic.The current assertion accepts any
io::ErrorKind::Othererror, which might mask unexpected failures. Since the test already skips in CI/headless environments (line 14), this fallback logic may be redundant. Consider either:
- Removing the fallback logic and expecting
Okwhen the test runs- Or, if clipboard errors are expected even outside CI, document why and consider being more specific about which errors are acceptable
🔎 Simplified approach
- let result = copy_to_clipboard(&file_path); - // Accept both Ok and clipboard errors (for headless/unsupported environments) - if result.is_err() { - eprintln!("Clipboard error: {:?}", result); - } - assert!( - result.is_ok() - || result - .as_ref() - .err() - .map_or(false, |e| e.kind() == io::ErrorKind::Other) - ); + let result = copy_to_clipboard(&file_path); + assert!(result.is_ok(), "Clipboard operation failed: {:?}", result.err());src/tests/gitignore_tests.rs (1)
14-28: Consider adding a test helper for Config construction.The Config object has many fields, and constructing it with mostly
Nonevalues is verbose. A helper function likedefault_test_config()orConfig::test_default()could reduce boilerplate across test files.🔎 Example test helper
Add to
src/tests/common.rs:pub fn create_default_config(directory: PathBuf, output: PathBuf) -> Config { Config { directory, output, include_dirs: None, exclude_dirs: None, include_ext: None, exclude_ext: None, include_files: None, exclude_files: None, min_size: None, max_size: None, respect_gitignore: true, tree_only: false, } }Then use it in tests:
let config = create_default_config( temp_dir.path().to_path_buf(), temp_dir.path().join("output.txt") );src/tests/file_processing_tests.rs (1)
111-127: Config construction pattern throughout tests.Like in
gitignore_tests.rs, these tests would benefit from a helper function to create default Config objects. This pattern repeats throughout this file (lines 142-155, 174-187, 205-218, etc.), making a helper function especially valuable here.Consider the test helper pattern suggested in the review of
src/tests/gitignore_tests.rsto reduce boilerplate.src/file_processing.rs (2)
30-46: Minor redundancy in case handling.The
dirsvalues are already lowercased during CLI parsing (seesrc/cli.rslines 31-34). Usingeq_ignore_ascii_casewithname.to_lowercase()is redundant but safe. Consider using a direct equality check for consistency.🔎 Optional simplification
- if let Some(name) = comp.as_os_str().to_str() - && dirs - .iter() - .any(|dir| dir.eq_ignore_ascii_case(&name.to_lowercase())) + if let Some(name) = comp.as_os_str().to_str() + && dirs.iter().any(|dir| dir == &name.to_lowercase())
228-256: Duplicated filtering logic with helper functions.The file name and extension filtering logic here (lines 229-256) duplicates
is_file_included_excluded(lines 49-64) andis_ext_included_excluded(lines 67-83). Both are called duringprocess_files, leading to redundant checks. Consider reusing the helpers:🔎 Proposed refactor to reuse helpers
// File filtering (only for files) if !is_dir { let file_name = path .file_name() .and_then(|f| f.to_str()) .unwrap_or_default() .to_lowercase(); - if let Some(excludes) = &config.exclude_files - && excludes.iter().any(|f| f.eq_ignore_ascii_case(&file_name)) - { - return true; - } - if let Some(includes) = &config.include_files - && !includes.iter().any(|f| f.eq_ignore_ascii_case(&file_name)) - { - return true; - } - let ext = path.extension().and_then(|e| e.to_str()); - if let Some(excludes) = &config.exclude_ext - && ext.is_some() - && excludes.iter().any(|e| e == &ext.unwrap().to_lowercase()) - { - return true; - } - if let Some(includes) = &config.include_ext - && (ext.is_none() || !includes.iter().any(|e| e == &ext.unwrap().to_lowercase())) - { + if !is_file_included_excluded(&file_name, &config.include_files, &config.exclude_files) { + return true; + } + let ext = path.extension().and_then(|e| e.to_str()); + if !is_ext_included_excluded(ext, &config.include_ext, &config.exclude_ext) { return true; } }src/cli.rs (2)
31-73: Consider extracting the repeated parsing logic.The same parsing pattern (split, trim, lowercase, filter empty) is repeated 6 times. This is a minor duplication that could be simplified with a helper function:
🔎 Optional helper extraction
fn parse_comma_separated(value: &str) -> Vec<String> { value .split(',') .map(|s| s.trim().to_lowercase()) .filter(|s| !s.is_empty()) .collect() }Then use it as:
let include_dirs = matches .get_one::<String>("include_dirs") .map(|s| parse_comma_separated(s));
89-92: Consider using a boolean flag action forrespect_gitignore.The current parsing accepts "true" or "1" as truthy but treats everything else (including "yes", "on") as falsy. This may surprise users. Consider using
clap'sArgAction::SetFalsewith a--no-respect-gitignoreflag for clarity, or document the accepted values explicitly.🔎 Alternative: Use a negated flag
// In create_commands(): .arg( Arg::new("no_respect_gitignore") .long("no-respect-gitignore") .action(clap::ArgAction::SetTrue) .help("Do not respect .gitignore rules"), ) // In config_from_matches(): let respect_gitignore = !matches.get_flag("no_respect_gitignore");Also applies to: 174-179
src/data.rs (1)
2-60: Consider alphabetically sorting both ignore lists for easier maintenance.Both
IGNORED_FILESandIGNORED_DIRSwould benefit from alphabetical ordering to:
- Make it easier to check for duplicates
- Simplify adding new entries in the future
- Improve overall maintainability
🔎 Proposed alphabetical ordering
/// Files to ignore during directory scanning and processing. pub const IGNORED_FILES: &[&str] = &[ - ".DS_Store", + ".DS_Store", "bun.lock", "bun.lockb", "Cargo.lock", "package-lock.json", "pnpm-lock.yaml", "uv.lock", "yarn.lock", ]; /// Directories to ignore during directory scanning and processing. pub const IGNORED_DIRS: &[&str] = &[ + ".cache", + ".changeset", + ".circleci", + ".classpath", + ".config", + ".cursor", + ".docker", + ".git", + ".github", + ".gradle", + ".hg", + ".husky", + ".idea", + ".ipynb_checkpoints", + ".local", + ".meteor", + ".mypy_cache", + ".next", + ".parcel-cache", + ".project", + ".pylint.d", + ".pytest_cache", + ".ropeproject", + ".settings", + ".svn", + ".trash", + ".turbo", + ".venv", + ".vercel", + ".vite", + ".vscode", + "__pycache__", + "bin", + "build", + "cargo-target", + "coverage", + "dist", + "helm", + "k8s", + "node_modules", + "out", + "pkg", + "src-tauri", + "storybook-static", + "target", ];
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (13)
Cargo.toml(1 hunks)README.md(3 hunks)src/cli.rs(5 hunks)src/clipboard.rs(1 hunks)src/data.rs(1 hunks)src/file_processing.rs(5 hunks)src/gitignore.rs(3 hunks)src/main.rs(1 hunks)src/tests/cli_tests.rs(5 hunks)src/tests/clipboard_tests.rs(1 hunks)src/tests/file_processing_tests.rs(15 hunks)src/tests/gitignore_tests.rs(1 hunks)src/tests/main_tests.rs(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (7)
src/tests/clipboard_tests.rs (1)
src/clipboard.rs (1)
copy_to_clipboard(7-19)
src/tests/gitignore_tests.rs (1)
src/gitignore.rs (1)
build_gitignore(10-46)
src/file_processing.rs (1)
src/cli.rs (4)
dirs(32-35)dirs(39-42)ext(46-49)ext(53-56)
src/tests/file_processing_tests.rs (1)
src/file_processing.rs (3)
get_directory_structure(86-119)is_in_ignored_dir(9-28)should_skip_path_advanced(210-259)
src/tests/cli_tests.rs (1)
src/cli.rs (2)
create_commands(118-207)config_from_matches(21-110)
src/main.rs (2)
src/gitignore.rs (1)
build_gitignore(10-46)src/file_processing.rs (1)
get_directory_structure(86-119)
src/tests/main_tests.rs (2)
src/gitignore.rs (1)
build_gitignore(10-46)src/file_processing.rs (1)
get_directory_structure(86-119)
⏰ 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). (1)
- GitHub Check: Test (windows-latest, stable)
🔇 Additional comments (20)
src/clipboard.rs (1)
11-12: Formatting change looks good, but verify alignment with PR scope.The refactoring from a multi-line chained call to a more compact format is fine—the logic and error handling remain unchanged, and the code follows Rust idioms. However, this change appears unrelated to the stated PR objectives about restructuring CLI filtering and config-driven models. Please verify whether this is an intentional cleanup or if the PR description needs updating.
src/tests/clipboard_tests.rs (1)
14-14: LGTM! Headless environment check added.The addition of the
DISPLAYcheck appropriately handles headless CI environments where clipboard operations are unavailable.Cargo.toml (1)
3-5: LGTM! Version and description updates are appropriate.The minor version bump to 1.6.0 correctly reflects the new filtering features, and the updated description accurately describes the enhanced capabilities.
README.md (2)
58-72: LGTM! Comprehensive CLI documentation.The updated OPTIONS section clearly documents all the new filtering capabilities, including the
--respect-gitignoreflag with its default value. The documentation is clear and user-friendly.
76-121: Excellent examples showcasing new features.The examples effectively demonstrate various filtering scenarios, including:
- Extension filtering (include/exclude)
- Directory filtering
- File-specific filtering
- Combining multiple filters
- Disabling .gitignore respect
These examples will help users understand the flexible filtering options.
src/gitignore.rs (2)
8-14: LGTM! Config-driven signature is a good refactor.The function signature change from accepting individual parameters to accepting a
&Configreference centralizes configuration handling and makes the API more maintainable.
37-43: Correct integration with Config object.The function now properly reads
exclude_dirsfrom the config object, maintaining the same filtering logic while adopting the new config-driven approach.src/main.rs (1)
21-24: LGTM! Main updated for config-driven API.The main function correctly passes the full
&configreference to bothbuild_gitignoreandget_directory_structure, aligning with the refactored function signatures.src/tests/cli_tests.rs (3)
14-15: LGTM! Tests updated for new extension fields.The tests correctly validate the new
include_extandexclude_extfields, replacing the previousextensionsfield. The assertions properly check forNonedefaults and parsed values.Also applies to: 35-36, 50-52
45-46: CLI flag updated correctly.The tests now use
--include-extinstead of the old--extflag, aligning with the more explicit and flexible filtering model that includes both include and exclude options.Also applies to: 117-118
103-103: Error messages updated for clarity.The error messages now reference the specific flag names ("Invalid min-size" and "Invalid max-size"), making them more helpful for users.
Also applies to: 112-112
src/tests/file_processing_tests.rs (2)
5-5: Function rename noted:should_skip_path_advanced.The import correctly references the renamed function
should_skip_path_advanced, which likely indicates enhanced functionality in the config-driven model.
442-486: Test coverage for should_skip_path_advanced looks comprehensive.The tests properly validate the renamed function with various scenarios:
- Ignored directories (node_modules, .git, target)
- Paths that should and shouldn't be skipped
- Both file and directory paths
The test suite appropriately passes the full Config object to align with the new API.
src/tests/main_tests.rs (2)
39-54: LGTM! Config initialization aligns with the new struct.The test correctly initializes all new filtering fields (
include_dirs,exclude_dirs,include_ext,exclude_ext,include_files,exclude_files) toNoneand setsrespect_gitignore: true, which matches the default behavior expected from the CLI.
59-62: Function signatures updated correctly.The calls to
build_gitignoreandget_directory_structurenow pass the full&configreference as the last parameter, consistent with the updated function signatures in the external snippets.src/file_processing.rs (2)
85-119: LGTM!The signature update to accept
&Configand delegation toshould_skip_path_advancedis clean and correctly integrates with the new config-driven filtering model.
224-227: Good addition of therespect_gitignoregate.This correctly allows users to bypass
.gitignorerules when needed, which is useful for including ignored files in the output.src/cli.rs (2)
5-19: LGTM! Clean Config struct design.The struct appropriately uses
Option<Vec<String>>for optional filtering fields, allowing distinction between "not specified" and "empty list". Therespect_gitignoreboolean with a default oftrueis a sensible default.
138-173: Well-structured CLI arguments with clear help text.The new filtering options are well-defined with descriptive help text and examples. The naming convention is consistent (
--include-dirs,--exclude-dirs, etc.).src/data.rs (1)
6-6: Cargo.lock should remain in the ignored files list for code review scans.Lock files are generated artifacts that don't contain user-authored code requiring review. This is appropriate for all project types (binary and library) since code review tools analyze source code quality, not build configuration. The binary-vs-library distinction matters for version control decisions, not for code scanning filters.
There was a problem hiding this comment.
Actionable comments posted: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/tests/cli_tests.rs (3)
19-19: Fix Clippy warning: useassert!instead ofassert_eq!with bool.The pipeline fails because
assert_eq!with a literal bool is discouraged.🔎 Proposed fix
- assert_eq!(config.tree_only, false); + assert!(!config.tree_only);
40-40: Fix Clippy warning: useassert!instead ofassert_eq!with bool.🔎 Proposed fix
- assert_eq!(config.tree_only, false); + assert!(!config.tree_only);
132-132: Fix Clippy warning: useassert!instead ofassert_eq!with bool.🔎 Proposed fix
- assert_eq!(config.tree_only, true); + assert!(config.tree_only);
🧹 Nitpick comments (4)
src/main.rs (1)
49-49: Consider graceful error handling instead ofexpect().Using
expect()will panic if the home/config directory cannot be determined. Consider returning an error instead for better UX.🔎 Proposed fix
- let mut home = dirs::home_dir().expect("Could not determine home directory"); + let home = dirs::home_dir().ok_or_else(|| { + io::Error::new(io::ErrorKind::NotFound, "Could not determine home directory") + })?;src/tests/config_tests.rs (1)
29-35: Test file cleanup should use a scope guard or defer pattern.If
FileConfig::from_pathpanics, the test file won't be cleaned up. Consider usingtempfilecrate or a cleanup guard for more robust test hygiene.src/file_processing.rs (1)
227-284: Consider extracting duplicate filtering logic.
should_skip_path_advancedre-implements file/extension filtering logic that's also inis_file_included_excludedandis_ext_included_excluded. Consider reusing those helpers to reduce duplication.🔎 Possible refactor
// File filtering (only for files) if !is_dir { let file_name = path .file_name() .and_then(|f| f.to_str()) .unwrap_or_default() .to_lowercase(); - if let Some(excludes) = &config.exclude_files - && excludes.iter().any(|f| f.eq_ignore_ascii_case(&file_name)) - { - return true; - } - if let Some(includes) = &config.include_files - && !includes.iter().any(|f| f.eq_ignore_ascii_case(&file_name)) - { + if !is_file_included_excluded(&file_name, &config.include_files, &config.exclude_files) { return true; } let ext = path.extension().and_then(|e| e.to_str()); - let ext_str = ext.unwrap_or("").to_lowercase(); - - if let Some(excludes) = &config.exclude_ext - && excludes.iter().any(|e| e == &ext_str) - { - return true; - } - - if let Some(includes) = &config.include_ext - && !includes.iter().any(|e| e == &ext_str) - { + if !is_ext_included_excluded(ext, &config.include_ext, &config.exclude_ext) { return true; } }src/tests/file_processing_tests.rs (1)
850-940: Consider splitting this test into three separate tests for clarity.This 90-line test combines three distinct test scenarios (include_ext, exclude_ext, and no-extension handling). While the test isolation using separate temp directories is good, splitting into separate test functions would improve readability and make test failures easier to diagnose.
Minor: Subcase 2 (lines 885-911) verifies that
.mdfiles are excluded but doesn't explicitly assert that files with no extension (likenoext) are included. Consider adding:assert!(out_excl.contains("=== File: noext"));This would make the test more comprehensive and verify the full behavior of
exclude_ext.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (15)
.github/workflows/ci.yml(1 hunks)Cargo.toml(1 hunks)README.md(4 hunks)src/cli.rs(3 hunks)src/config.rs(1 hunks)src/data.rs(1 hunks)src/file_processing.rs(6 hunks)src/gitignore.rs(3 hunks)src/main.rs(2 hunks)src/tests/cli_tests.rs(6 hunks)src/tests/config_tests.rs(1 hunks)src/tests/file_processing_tests.rs(15 hunks)src/tests/gitignore_tests.rs(3 hunks)src/tests/main_tests.rs(1 hunks)src/tests/mod.rs(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- .github/workflows/ci.yml
🚧 Files skipped from review as they are similar to previous changes (3)
- Cargo.toml
- src/gitignore.rs
- README.md
🧰 Additional context used
🧬 Code graph analysis (8)
src/main.rs (4)
src/gitignore.rs (1)
build_gitignore(10-46)src/file_processing.rs (1)
get_directory_structure(87-130)src/cli.rs (1)
create_commands(3-107)src/config.rs (4)
config_from_matches(90-218)discover_config_file(56-68)from_path(42-51)merge_config(72-87)
src/tests/main_tests.rs (2)
src/cli.rs (1)
create_commands(3-107)src/main.rs (1)
handle_init_subcommand(43-104)
src/tests/gitignore_tests.rs (1)
src/gitignore.rs (1)
build_gitignore(10-46)
src/tests/config_tests.rs (1)
src/config.rs (19)
discover_config_file(56-68)merge_config(72-87)from_path(42-51)dirs(115-118)dirs(125-128)matches(91-92)matches(102-103)matches(113-113)matches(123-123)matches(133-133)matches(143-143)matches(153-153)matches(164-164)matches(175-175)matches(183-183)matches(191-191)matches(198-198)files(155-159)files(166-170)
src/file_processing.rs (1)
src/config.rs (4)
dirs(115-118)dirs(125-128)ext(135-138)ext(145-148)
src/cli.rs (1)
src/tests/main_tests.rs (1)
new(22-25)
src/config.rs (1)
src/tests/main_tests.rs (1)
new(22-25)
src/tests/file_processing_tests.rs (3)
src/file_processing.rs (4)
get_directory_structure(87-130)is_in_ignored_dir(10-29)process_files(133-225)should_skip_path_advanced(233-284)src/tests/main_tests.rs (1)
new(22-25)src/tests/common.rs (2)
setup_temp_dir(8-10)create_file(12-16)
🪛 GitHub Actions: CI
src/main.rs
[error] 1-1: linking with 'cc' failed. rust-lld: unable to find libraries -lxcb, -lxcb-render, -lxcb-shape, -lxcb-xfixes required by fyai binary. Ensure X11/XCB development libraries are installed and available in the linker path. Command 'cargo build --release --verbose' failed with exit code 101.
src/tests/main_tests.rs
[error] 98-98: test_init_global_uses_home_dir failed: Expected global config.yaml to be created.
[error] 64-64: test_init_local_creates_file failed: Called unwrap on an Err variant (PoisonError) when accessing a MutexGuard.
src/tests/gitignore_tests.rs
[error] 29-29: Clippy: this expression creates a reference which is immediately dereferenced (needlessly_borrow). Replace with direct usage of the referenced value. Command: cargo clippy --all-targets --all-features -- -D warnings.
[error] 110-110: Clippy: this expression creates a reference which is immediately dereferenced (needlessly_borrow). Replace with direct usage of the referenced value. Command: cargo clippy --all-targets --all-features -- -D warnings.
[error] 163-163: Clippy: this expression creates a reference which is immediately dereferenced (needlessly_borrow). Replace with direct usage of the referenced value. Command: cargo clippy --all-targets --all-features -- -D warnings.
src/tests/cli_tests.rs
[error] 19-19: Clippy: used assert_eq! with a literal bool. Replace with assert!(!config.tree_only) as advised by clippy. Command: cargo clippy --all-targets --all-features -- -D warnings.
[error] 40-40: Clippy: used assert_eq! with a literal bool. Replace with assert!(!config.tree_only). Command: cargo clippy --all-targets --all-features -- -D warnings.
[error] 132-132: Clippy: used assert_eq! with a literal bool. Replace with assert!(config.tree_only). Command: cargo clippy --all-targets --all-features -- -D warnings.
src/tests/config_tests.rs
[error] 44-44: Clippy: used assert_eq! with a literal bool. Replace with assert!(cfg.respect_gitignore.unwrap() == false) or better assert!(!cfg.respect_gitignore.unwrap()). Command: cargo clippy --all-targets --all-features -- -D warnings.
src/file_processing.rs
[error] 108-114: Clippy: this if statement can be collapsed. Suggest simplifying nested conditions. Command: cargo clippy --all-targets --all-features -- -D warnings.
[error] 109-113: Clippy: this if statement can be collapsed (collapsible_if). Suggest combining conditions. Command: cargo clippy --all-targets --all-features -- -D warnings.
[error] 152-156: Clippy: this if statement can be collapsed. Suggest simplifying nested conditions. Command: cargo clippy --all-targets --all-features -- -D warnings.
[error] 108-114: Note: multiple similar collapsible-if issues detected in this region. Command: cargo clippy --all-targets --all-features -- -D warnings.
[error] 152-156: Note: multiple similar collapsible-if issues detected in this region. Command: cargo clippy --all-targets --all-features -- -D warnings.
🔇 Additional comments (27)
src/tests/mod.rs (1)
8-15: LGTM!The test module declarations are consistent with the existing pattern, properly gated with
#[cfg(test)].src/data.rs (2)
14-59: LGTM! Improved organization and addressed previous feedback.The IGNORED_DIRS list is now alphabetically sorted, improving maintainability. The non-standard "cargo-target" entry has been removed as previously suggested. Generic directory names like "bin", "pkg", and "out" may match legitimate source directories in some projects, but users can override these using
--include-dirsif needed.
2-11: LGTM!The IGNORED_FILES additions are appropriate—
.DS_Storefor macOS artifacts and common lock files for package managers.src/tests/gitignore_tests.rs (2)
81-136: Good test coverage for existing .gitignore loading.The test properly validates that patterns from an existing
.gitignorefile are respected, covering both file patterns (special.ignore) and directory patterns (logs/).
138-175: Good test coverage for CLI-specified exclude directories.This test validates that
exclude_dirsfrom the Config are properly integrated into the gitignore builder.src/tests/cli_tests.rs (1)
43-53: LGTM!The test correctly reflects the CLI flag rename from
--extto--include-extand the field rename toinclude_ext.src/tests/main_tests.rs (3)
110-129: Good test coverage for error case.The test correctly validates that
initwithout--forcereturns anAlreadyExistserror when the config file exists.
131-152: Good test coverage for force overwrite.The test correctly validates that
init --forceoverwrites an existing config file.
41-59: Unsafe env var manipulation is correctly wrapped.The
unsafeblocks are required forstd::env::set_var/remove_varin newer Rust editions and are properly scoped here. Thedirs::home_dir()function uses$HOMEif it is set and not empty; if$HOMEis not set or empty, it usesgetpwuid_rto determine the home directory, so environment variable modifications toHOMEwill be respected on all platforms including macOS.src/main.rs (2)
18-41: LGTM! Clean extraction of core logic.The
run_with_configfunction provides a good separation of concerns, enabling direct testing with controlled configuration.
106-141: LGTM! Main flow is well-structured.The configuration loading, merging, and delegation pattern is clean. The warning message for failed config file loading is appropriate.
src/tests/config_tests.rs (4)
63-77: LGTM! Thorough tests for config discovery.Good coverage of local and global config discovery, including backup/restore logic for the global test case.
Also applies to: 79-123
125-165: LGTM! Merge precedence tests are well-designed.Tests correctly verify that CLI values take precedence over file config values.
167-232: LGTM! Parsing tests cover key scenarios.Good coverage of valid parsing, invalid input handling, and edge cases.
344-369: LGTM! Error message tests validate user-facing feedback.Ensures meaningful error messages for missing required arguments.
src/cli.rs (2)
6-6: LGTM! Comprehensive help text with config file documentation.The about text clearly explains config file precedence and usage.
23-58: LGTM! Well-structured include/exclude argument definitions.The new filtering options are logically organized with clear help text.
src/file_processing.rs (3)
187-217: File name casing is now preserved for display - previous review concern addressed.The code now uses
file_name(original case) for display in line 217, whilefile_name_loweris used for filtering. This addresses the concern from the previous review.
31-47: LGTM! Clean include directory check implementation.The
is_in_included_dirhelper properly handles the "include all if not specified" semantics.
49-84: LGTM! File and extension inclusion/exclusion helpers are well-designed.The logic correctly handles the precedence: exclude takes priority, then include, with "include all" as default.
src/config.rs (4)
6-21: LGTM! Config struct is well-defined.Clean separation of runtime configuration with appropriate types.
23-52: LGTM! FileConfig with YAML deserialization is properly structured.The
from_pathmethod handles both IO and YAML parse errors appropriately.
54-68: LGTM! Config discovery logic is correct.Proper precedence: local config first, then global config.
89-218: LGTM! Robust argument parsing with proper error handling.The use of
try_get_onewith fallback handling for unregistered arguments enables flexible testing. The comma-separated list parsing with trim, lowercase, and empty filtering is consistent.src/tests/file_processing_tests.rs (3)
3-6: LGTM! Import updates align with the config refactoring.The import changes correctly reflect the new Config location (
crate::config::Config) and the renamed function (should_skip_path_advanced). These changes are consistent with the config subsystem refactoring described in the summary.
942-976: LGTM! Critical test for output file exclusion.This test properly verifies that the output file is not processed by itself, which prevents potential issues like infinite loops or corrupted output. The test setup and assertions are clear and correct.
978-1011: LGTM! Good coverage for include_dirs functionality.This test properly verifies the include_dirs filtering behavior, ensuring that only specified directories and their contents are included in the directory structure. The assertions correctly verify both what should be included and what should be excluded.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
src/tests/gitignore_tests.rs (2)
163-163: Fix Clippy error: remove needless borrows.Same issue as line 110—remove the leading
&fromIGNORED_FILESandIGNORED_DIRS.🔎 Proposed fix
- let gitignore = build_gitignore(temp_dir.path(), &IGNORED_FILES, &IGNORED_DIRS, &config)?; + let gitignore = build_gitignore(temp_dir.path(), IGNORED_FILES, IGNORED_DIRS, &config)?;
110-110: Fix Clippy error: remove needless borrows.The pipeline is failing because
&IGNORED_FILESand&IGNORED_DIRScreate unnecessary references. These are already&[&str]slices.🔎 Proposed fix
- let gitignore = build_gitignore(temp_dir.path(), &IGNORED_FILES, &IGNORED_DIRS, &config)?; + let gitignore = build_gitignore(temp_dir.path(), IGNORED_FILES, IGNORED_DIRS, &config)?;
🧹 Nitpick comments (6)
src/tests/config_tests.rs (3)
10-14: Inconsistent mutex poisoning handling.Unlike
lock_tests()inmain_tests.rswhich recovers from poisoned mutexes usingunwrap_or_else, thistest_lock()uses bare.lock().unwrap()which will cause cascading failures if any test panics while holding the lock.🔎 Proposed fix for consistency
fn test_lock() -> std::sync::MutexGuard<'static, ()> { - TEST_LOCK.get_or_init(|| Mutex::new(())).lock().unwrap() + TEST_LOCK + .get_or_init(|| Mutex::new(())) + .lock() + .unwrap_or_else(|poisoned| poisoned.into_inner()) }
29-35: Tests write to current working directory which may cause conflicts.Multiple tests write files like
./test_fyai_config.yamland./fyai.yamlto the current directory. If tests run in parallel (despite the mutex) or cleanup fails, this could cause flaky tests or affect other test runs. Consider usingTempDirfor all file operations.🔎 Example using TempDir
#[test] fn test_fileconfig_from_path_valid() { let temp = TempDir::new().expect("create tempdir"); let path = temp.path().join("test_fyai_config.yaml"); // ... rest of test using path }Also applies to: 50-56, 67-73
79-123: Global config discovery test modifies system configuration directory.This test writes to the real
dirs::config_dir()location (e.g.,~/.config/fyai.yaml), which could interfere with real user configuration or other test runs. While the backup/restore logic mitigates data loss, this is still risky in shared CI environments.Consider mocking
discover_config_fileor using dependency injection to avoid touching real system paths in tests.src/main.rs (1)
49-51: Consider graceful error handling instead of expect().If both
dirs::config_dir()anddirs::home_dir()returnNone(rare but possible on unusual systems), theexpect()will panic with a non-descriptive error. Consider returning anio::Errorfor better error messaging.🔎 Proposed improvement
let (path, display_path) = if global { let cfg_dir = dirs::config_dir() .or_else(|| dirs::home_dir().map(|h| h.join(".config"))) - .expect("Could not determine config directory"); + .ok_or_else(|| { + io::Error::new( + io::ErrorKind::NotFound, + "Could not determine config directory (no $HOME or XDG_CONFIG_HOME)", + ) + })?;src/file_processing.rs (2)
150-158: Nested if statements can be collapsed.The nested
if letstatements at lines 150-156 can be collapsed into a single condition using let-chains, similar to what was done inget_directory_structure(lines 108-113).🔎 Proposed fix
- if let Some(output_canon) = output_canon.as_ref() { - if let Ok(path_canon) = fs::canonicalize(path) - && path_canon == *output_canon - { - continue; - } - } else if path == config.output { + if let Some(output_canon) = output_canon.as_ref() + && let Ok(path_canon) = fs::canonicalize(path) + && path_canon == *output_canon + { + continue; + } else if output_canon.is_none() && path == config.output { continue; }
251-280: Duplicate filtering logic creates maintenance burden.The file name and extension filtering logic here (lines 251-280) duplicates the
is_file_included_excludedandis_ext_included_excludedhelper functions defined earlier. Additionally,process_filesapplies bothshould_skip_path_advanced(line 162) and the helper functions (lines 193-204), resulting in redundant double-filtering.Consider either:
- Reusing the helper functions inside
should_skip_path_advanced, or- Removing the helper function calls from
process_filessinceshould_skip_path_advancedalready handles them🔎 Option 1: Reuse helpers in should_skip_path_advanced
// File filtering (only for files) if !is_dir { let file_name = path .file_name() .and_then(|f| f.to_str()) .unwrap_or_default() .to_lowercase(); - if let Some(excludes) = &config.exclude_files - && excludes.iter().any(|f| f.eq_ignore_ascii_case(&file_name)) - { - return true; - } - if let Some(includes) = &config.include_files - && !includes.iter().any(|f| f.eq_ignore_ascii_case(&file_name)) - { - return true; - } - let ext = path.extension().and_then(|e| e.to_str()); - let ext_str = ext.unwrap_or("").to_lowercase(); - - if let Some(excludes) = &config.exclude_ext - && excludes.iter().any(|e| e == &ext_str) - { - return true; - } - - if let Some(includes) = &config.include_ext - && !includes.iter().any(|e| e == &ext_str) - { + if !is_file_included_excluded(&file_name, &config.include_files, &config.exclude_files) { return true; } + let ext = path.extension().and_then(|e| e.to_str()); + if !is_ext_included_excluded(ext, &config.include_ext, &config.exclude_ext) { + return true; + } } false🔎 Option 2: Remove redundant calls in process_files
// Since should_skip_path_advanced already handles file/ext filtering, // these checks can be removed from process_files (lines 192-204) - // Extension filtering - if !is_ext_included_excluded(ext, &config.include_ext, &config.exclude_ext) { - continue; - } - - // File name filtering - if !is_file_included_excluded( - &file_name_lower, - &config.include_files, - &config.exclude_files, - ) { - continue; - }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/file_processing.rs(6 hunks)src/main.rs(2 hunks)src/tests/cli_tests.rs(6 hunks)src/tests/config_tests.rs(1 hunks)src/tests/gitignore_tests.rs(3 hunks)src/tests/main_tests.rs(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
src/tests/gitignore_tests.rs (1)
src/gitignore.rs (1)
build_gitignore(10-46)
src/tests/config_tests.rs (1)
src/config.rs (20)
config_from_matches(90-218)discover_config_file(56-68)merge_config(72-87)from_path(42-51)dirs(115-118)dirs(125-128)matches(91-92)matches(102-103)matches(113-113)matches(123-123)matches(133-133)matches(143-143)matches(153-153)matches(164-164)matches(175-175)matches(183-183)matches(191-191)matches(198-198)files(155-159)files(166-170)
src/tests/cli_tests.rs (2)
src/cli.rs (1)
create_commands(3-107)src/config.rs (1)
config_from_matches(90-218)
src/file_processing.rs (1)
src/config.rs (4)
dirs(115-118)dirs(125-128)ext(135-138)ext(145-148)
🪛 GitHub Actions: CI
src/tests/gitignore_tests.rs
[error] 110-110: Clippy: this expression creates a reference which is immediately dereferenced by the compiler. Change &IGNORED_FILES to IGNORED_FILES (needless_borrow)
[error] 110-110: Clippy: this expression creates a reference which is immediately dereferenced by the compiler. Change &IGNORED_DIRS to IGNORED_DIRS (needless_borrow)
[error] 163-163: Clippy: this expression creates a reference which is immediately dereferenced by the compiler. Change &IGNORED_FILES to IGNORED_FILES (needless_borrow)
[error] 163-163: Clippy: this expression creates a reference which is immediately dereferenced by the compiler. Change &IGNORED_DIRS to IGNORED_DIRS (needless_borrow)
src/main.rs
[error] Linking failed: unable to find required system libraries. rust-lld could not locate: libxcb, libxcb-render, libxcb-shape, libxcb-xfixes. Install the required X11 development libraries (libxcb-dev and related packages) to resolve this issue.
🔇 Additional comments (23)
src/tests/main_tests.rs (4)
16-20: LGTM! Mutex poisoning recovery correctly implemented.The
lock_tests()helper properly recovers from a poisoned mutex usingunwrap_or_else(|poisoned| poisoned.into_inner()), preventing cascading test failures.
91-118: Global init test may still be flaky on some platforms.The test sets
HOMEbutdirs::config_dir()on line 104-105 may not respect this environment variable on all platforms (e.g., macOS usesNSHomeDirectory). The fallback totemp_home.path().join(".config")helps, but ifdirs::config_dir()returns a real system path, the assertion on line 107-111 could fail.Consider injecting the home directory path directly into
handle_init_subcommandfor more reliable testing, as suggested in the past review comment.
67-89: Well-structured tests with proper isolation.The local init tests (
test_init_local_creates_file,test_init_already_exists_without_force_errors,test_init_force_overwrites_existing) correctly useCwdGuardfor directory isolation and properly validate file creation, error conditions, and force overwrite behavior.Also applies to: 120-162
47-65: The unsafe blocks are required for Rust 2024 edition and correctly implemented.The
EnvVarGuardpattern properly usesunsafeblocks forstd::env::set_varandstd::env::remove_varas required by the Rust 2024 edition. These functions are marked as unsafe starting in the 2024 Edition. The implementation is sound: state is saved before modification and restored in theDropimplementation. Test serialization vialock_tests()using a staticMutexensures environment modifications don't interfere with concurrent test execution.src/tests/gitignore_tests.rs (2)
81-136: Good test coverage for existing .gitignore loading.The
test_build_gitignore_loads_existing_gitignoretest properly validates that patterns from an existing.gitignorefile are respected by the builder.
138-175: Good test for CLI exclude_dirs configuration.The
test_build_gitignore_respects_cli_exclude_dirstest properly validates that directories specified viaconfig.exclude_dirsare correctly ignored.src/tests/cli_tests.rs (4)
5-5: Import path correctly updated for the new config module.The import now properly references
config::config_from_matchesfrom the new configuration subsystem.
7-20: Default config test properly updated for renamed fields.The assertions correctly validate the new field names (
include_ext,exclude_ext,min_size,max_size,exclude_dirs) and useassert!(!config.tree_only)instead of comparing with a literal bool.
43-53: Extension parsing test updated for new CLI flag.Test correctly uses
--include-extflag and validatesconfig.include_extfield.
97-113: Error message assertions updated to match new format.The error messages now correctly expect "Invalid min-size" and "Invalid max-size" matching the updated
config_from_matchesimplementation.src/tests/config_tests.rs (3)
44-44: LGTM! Past Clippy warning addressed.The assertion now correctly uses
assert!(!cfg.respect_gitignore.unwrap())instead of comparing with a literal bool.
125-165: Good coverage for merge_config precedence.The test properly validates that CLI values take precedence over file config values, and that file values are used as fallbacks when CLI values are
None.
167-211: Comprehensive test coverage for config_from_matches.Tests thoroughly cover parsing, normalization (trimming, lowercasing, filtering empties), boolean flag handling, and error cases for invalid inputs and missing required fields.
Also applies to: 234-342
src/main.rs (4)
18-41: Good refactoring for testability.Extracting
run_with_configas a public function allows tests to exercise the core logic with controlledConfiginstances, improving test isolation.
48-59: Global config path now correctly aligned with discovery.The init subcommand now writes to
dirs::config_dir().join("fyai.yaml")(with fallback to~/.config), which matches the path checked bydiscover_config_file(). This addresses the previous path inconsistency.
108-143: Well-structured main flow with config discovery and merging.The main function correctly:
- Handles init subcommand first
- Parses CLI config
- Discovers and loads optional config file with graceful error handling
- Merges configs with CLI precedence
- Delegates to
run_with_config
1-143: Add X11 development libraries to CI workflow for Linux clipboard support.The clipboard crate requires X11 development libraries on Linux to compile. Update
.github/workflows/ci.ymlto installlibxcb-devbefore building:- name: Install X11 libraries (Linux) if: runner.os == 'Linux' run: sudo apt-get update && sudo apt-get install -y libxcb1-devAdd this step in the
testjob before the "Build" step. This is a CI environment configuration requirement, not a code issue.src/file_processing.rs (6)
7-8: LGTM!The import of
Configis correctly added to support the new config-driven filtering API.
31-47: LGTM with a minor note.The function correctly filters paths based on included directories. The use of
eq_ignore_ascii_case(&name.to_lowercase())on line 38 is slightly redundant sinceeq_ignore_ascii_casealready performs case-insensitive comparison, but it doesn't affect correctness.
49-65: LGTM!The function correctly prioritizes exclusions over inclusions and handles the
Nonecases appropriately.
67-84: LGTM!The function correctly handles extension filtering with proper lowercase normalization before comparison.
86-129: LGTM!The function correctly:
- Accepts the new
Configparameter- Implements output file canonicalization to avoid self-processing
- Uses the new
should_skip_path_advancedfor config-driven filteringThe let-chains syntax (lines 108-113) addresses the previous Clippy
collapsible_ifwarning.
186-216: Good fix for file name casing.The implementation correctly preserves the original file name for display (line 216) while using the lowercased version for filtering (lines 190, 198-199). This addresses the previous review comment about file names being displayed in lowercase.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main.rs (1)
1-1: CI environment requires XCB system libraries for clipboard crate.The pipeline failure indicates missing X11 XCB libraries required by the
clipboardcrate on Linux. This is an infrastructure issue, not a code defect.Install the required development packages in your CI environment:
# For Ubuntu/Debian-based systems sudo apt-get install -y libxcb1-dev libxcb-render0-dev libxcb-shape0-dev libxcb-xfixes0-dev # For Alpine Linux apk add libxcb-devAlternatively, update your CI workflow configuration (
.github/workflows/ci.yml) to install these dependencies before the build step.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/config.rs(1 hunks)src/main.rs(2 hunks)src/tests/common.rs(3 hunks)src/tests/file_processing_tests.rs(15 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/tests/file_processing_tests.rs (1)
src/file_processing.rs (4)
get_directory_structure(87-129)is_in_ignored_dir(10-29)process_files(132-224)should_skip_path_advanced(232-283)
🪛 GitHub Actions: CI
src/main.rs
[error] 1-1: Linking failed: rustc could not link the binary due to missing system libraries. linker reported: 'rust-lld: error: unable to find library -lxcb', 'lxcb-render', 'lxcb-shape', 'lxcb-xfixes'. Ensure the XCB libraries are installed in the build environment or provide the necessary native dependencies. Command: cargo build --verbose
🔇 Additional comments (14)
src/tests/common.rs (2)
5-5: LGTM! Documentation and import improvements.The PathBuf import and added documentation comments enhance code clarity and are necessary for the new test helper.
Also applies to: 18-19
40-62: LGTM! Well-designed test helper reduces duplication.The
create_test_confighelper follows best practices by providing sensible defaults and using a closure for field-specific overrides, effectively addressing the earlier feedback about reducing Config initialization duplication across tests.src/main.rs (3)
13-13: LGTM! Clean extraction of core logic for testability.The new
configmodule andrun_with_configfunction properly separate concerns, making the core application logic independently testable with controlled configuration.Also applies to: 18-41
43-106: LGTM! Init subcommand properly handles config file creation.The implementation correctly aligns global config path with
discover_config_file()expectations, includes comprehensive error handling for existing files, and provides a well-documented template with all available configuration options.
108-145: LGTM! Well-structured main flow with proper config precedence.The refactored
mainfunction correctly implements the config loading and merging strategy: init subcommand handled first, file config discovered and loaded with graceful error handling, and CLI flags take precedence when explicitly provided via theExplicitFlagsmechanism.src/config.rs (5)
1-21: LGTM! Well-designed Config struct with appropriate types.The
Configstruct uses correct types:PathBuffor filesystem paths,Option<Vec<String>>to distinguish "not specified" from "empty list" for filters, andboolfor flags.
23-52: LGTM! FileConfig properly handles optional YAML fields.The
FileConfigstruct with all-optional fields enables partial configuration files, andfrom_pathcorrectly maps YAML parse errors toio::Errorfor consistent error handling.
54-68: LGTM! Config discovery follows correct precedence order.The function properly prioritizes local (
./fyai.yaml) over global config directory (~/.config/fyai.yamlor platform equivalent), allowing project-specific overrides.
74-127: LGTM! Merge logic correctly prioritizes explicit CLI values over file config.The
ExplicitFlagsmechanism properly distinguishes user-provided CLI values from defaults, ensuring file config values are respected when the user hasn't explicitly overridden them. This addresses the previous concern about ignoring file config entirely.
134-293: LGTM! Thorough CLI argument parsing with proper validation.The function correctly parses all CLI arguments, applies consistent normalization (trim, lowercase, filter empty) to list inputs, validates numeric types, and tracks explicit flags for proper merge behavior. The implementation is comprehensive and handles all edge cases appropriately.
src/tests/file_processing_tests.rs (4)
1-16: LGTM! Clear documentation and updated imports.The comment documenting the refactoring to use
create_test_configand the updated imports for the new config-driven API are appropriate.
21-603: LGTM! Test refactoring successfully reduces duplication.All existing tests have been consistently updated to use
create_test_config, making them more maintainable and resilient to futureConfigstructure changes. This fully addresses the previous feedback about Config initialization duplication.
604-677: LGTM! Comprehensive extension filtering test coverage.The new test thoroughly validates
include_extandexclude_extbehavior, including the edge case of filtering by empty extension. Using separate temporary directories for each subcase properly isolates test runs.
679-728: LGTM! Essential edge case coverage added.Both new tests validate critical scenarios: preventing output file re-processing (avoiding circular references) and verifying
include_dirsfiltering in directory structure generation.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/tests/config_tests.rs (1)
16-123: Consider usingstd::env::temp_dir()for temporary test files.The tests create temporary config files in the current directory (e.g.,
./test_fyai_config.yaml,./bad_fyai_config.yaml,./fyai.yaml). While the test lock provides synchronization, usingstd::env::temp_dir()would provide better isolation and avoid polluting the project directory if cleanup fails.Example approach
let temp_dir = std::env::temp_dir(); let path = temp_dir.join("test_fyai_config.yaml"); fs::write(&path, yaml).expect("write yaml"); // ... test logic ... let _ = fs::remove_file(&path);
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/config.rs(1 hunks)src/main.rs(2 hunks)src/tests/cli_tests.rs(6 hunks)src/tests/config_tests.rs(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/tests/cli_tests.rs (2)
src/cli.rs (1)
create_commands(3-107)src/config.rs (1)
config_from_matches(130-287)
src/tests/config_tests.rs (1)
src/config.rs (24)
config_from_matches(130-287)discover_config_file(56-68)merge_config(82-123)from_path(42-51)dirs(176-179)dirs(186-189)matches(131-131)matches(136-136)matches(141-141)matches(146-146)matches(152-153)matches(163-164)matches(174-174)matches(184-184)matches(194-194)matches(204-204)matches(214-214)matches(225-225)matches(236-236)matches(244-244)matches(252-252)matches(259-259)files(216-220)files(227-231)
🪛 GitHub Actions: CI
src/main.rs
[error] 1-1: Cargo build failed during linking: rust-lld cannot find system libraries. unable to find library -lxcb, -lxcb-render, -lxcb-shape, -lxcb-xfixes. This typically indicates missing system dependencies (e.g., X11/xcb development libraries) on the runner. Command: 'cargo build --verbose'.
🔇 Additional comments (5)
src/tests/cli_tests.rs (1)
5-133: LGTM! Tests properly updated for new configuration API.The test suite correctly reflects the new configuration API:
- Properly destructures the
(Config, ExplicitFlags)tuple fromconfig_from_matches- Uses updated field names (
include_ext,exclude_ext, etc.)- Covers default values, custom inputs, edge cases, and error handling
- Verifies normalization behavior (lowercase, trim, filter empty entries)
src/tests/config_tests.rs (1)
16-432: Excellent test coverage for the configuration subsystem.The test suite comprehensively covers:
- YAML loading (valid and invalid files)
- Config file discovery (local and global with proper backup/restore)
- Merge precedence between file and CLI configurations
- CLI argument parsing with normalization (lowercase, trim, filter empty)
- Error handling and edge cases
- Default behavior for missing or unregistered arguments
The use of
test_lock()for synchronizing filesystem operations is appropriate.src/config.rs (1)
1-287: Well-designed configuration subsystem.The configuration module provides a solid foundation:
- Clear separation between runtime
Configand file-basedFileConfig- Proper YAML parsing with error handling
- Config file discovery with local/global precedence
- Merge logic that respects explicit CLI overrides via
ExplicitFlags- Comprehensive CLI parsing with normalization (trim, lowercase, filter empty)
- Appropriate defaults for boolean flags
The past review comment about merge precedence has been properly addressed.
src/main.rs (2)
18-145: Well-structured refactoring for testability and config-driven flow.The changes successfully extract core logic into testable functions:
run_with_configencapsulates the main processing logichandle_init_subcommandisolates init command handlingmainorchestrates config loading, merging, and delegationThe config merging flow properly:
- Handles the init subcommand early
- Discovers and loads YAML config with fallback
- Merges file and CLI config with proper precedence
- Prints warnings for config loading failures
1-1: Install X11 development libraries in CI to build clipboard functionality.The project uses
clipboard = "0.5.0"which depends on x11-clipboard on Linux systems, requiring libxcb to compile. The CI configuration doesn't install these libraries, causing build failures on ubuntu-latest.Add this step to the test job in
.github/workflows/ci.yml(before the Build step):- name: Install X11 development libraries if: runner.os == 'Linux' run: sudo apt-get update && sudo apt-get install -y libxcb-dev libxcb-render0-dev libxcb-shape0-dev libxcb-xfixes0-devAlternatively, make clipboard an optional feature and disable it for CI builds.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
.github/workflows/ci.yml (2)
114-124: System dependencies may be unnecessary for format checking.The
cargo fmtcommand only checks code formatting and doesn't compile or link the project. The X11/XCB development libraries should not be required for this job.🔎 Consider removing system deps from fmt job
- name: Checkout repository uses: actions/checkout@v6 - - name: Install system deps - run: | - sudo apt-get update - sudo apt-get install -y --no-install-recommends \ - pkg-config \ - libx11-dev \ - libxcb1-dev \ - libxcb-render0-dev \ - libxcb-shape0-dev \ - libxcb-xfixes0-dev - sudo rm -rf /var/lib/apt/lists/* - - name: Install Rust toolchain
75-85: Consider extracting repeated system deps installation to reduce duplication.The same apt-get installation block is repeated across 4 jobs. While explicitness in CI is sometimes preferred, you could reduce maintenance burden by using a composite action or a reusable workflow.
This is optional since some teams prefer explicit steps in CI for visibility. If you choose to refactor, a composite action in
.github/actions/install-deps/action.ymlcould encapsulate this logic.src/tests/gitignore_tests.rs (1)
15-28: Consider extracting Config construction into a helper function.The Config construction is repeated nearly identically across all three tests (lines 15-28, 96-109, 149-162), with only
exclude_dirsdiffering in the third test. This duplication makes the tests harder to maintain.🔎 Proposed helper function
Add a helper function at the top of the test module:
fn create_test_config(temp_dir: &TempDir, exclude_dirs: Option<Vec<String>>) -> crate::config::Config { crate::config::Config { directory: temp_dir.path().to_path_buf(), output: temp_dir.path().join("output.txt"), include_dirs: None, exclude_dirs, include_ext: None, exclude_ext: None, include_files: None, exclude_files: None, min_size: None, max_size: None, respect_gitignore: true, tree_only: false, } }Then simplify the test calls:
- let config = crate::config::Config { - directory: temp_dir.path().to_path_buf(), - output: temp_dir.path().join("output.txt"), - include_dirs: None, - exclude_dirs: None, - include_ext: None, - exclude_ext: None, - include_files: None, - exclude_files: None, - min_size: None, - max_size: None, - respect_gitignore: true, - tree_only: false, - }; + let config = create_test_config(&temp_dir, None);Also applies to: 96-109, 149-162
src/tests/clipboard_tests.rs (2)
18-29: Consider whether relaxed assertion is necessary.The test now accepts both successful clipboard operations and
io::ErrorKind::Othererrors. Since the skip condition at line 14 already filters out known problematic environments (CI and missing DISPLAY), the remaining test execution should occur only in environments where clipboard is expected to work.Accepting clipboard errors in these environments reduces the test's ability to catch genuine clipboard integration issues.
Consider:
- Is this relaxation necessary, or are there specific environments where clipboard fails despite DISPLAY being set?
- Could we narrow the acceptance criteria or use a more specific environment check instead?
49-52: Consider consistent environment checks across clipboard tests.The
test_copy_to_clipboard_empty_filetest only checks theCIenvironment variable, whiletest_copy_to_clipboard_valid_filechecks bothCIandDISPLAY.For consistency, consider applying the same skip logic to both tests (though note the concern raised about the DISPLAY check being X11-specific).
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.github/workflows/ci.yml(5 hunks)src/tests/clipboard_tests.rs(1 hunks)src/tests/gitignore_tests.rs(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/tests/clipboard_tests.rs (1)
src/clipboard.rs (1)
copy_to_clipboard(7-19)
src/tests/gitignore_tests.rs (1)
src/gitignore.rs (1)
build_gitignore(10-46)
🔇 Additional comments (6)
.github/workflows/ci.yml (3)
46-57: LGTM!The conditional installation for Ubuntu-only is correctly implemented. Cleaning apt lists afterward is good practice for reducing cache/image size.
173-175: LGTM!The binary verification correctly references the new
fyaibinary name, consistent with the project rename mentioned in the summary.
27-28: No action needed.actions/checkout@v6exists and is available for use.src/tests/gitignore_tests.rs (3)
3-3: LGTM!The
std::fsimport is necessary for the new tests that create files and directories.
81-136: LGTM!The test properly validates that
build_gitignoreloads and respects patterns from an existing.gitignorefile. It covers both file patterns (special.ignore) and directory patterns (logs/).
138-175: LGTM!The test correctly validates that directories specified in
config.exclude_dirsare properly ignored bybuild_gitignore. The test setup and assertions are appropriate.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/tests/main_tests.rs (1)
67-133: Consider adding test coverage for global init.The local init functionality is well-tested, but there's no test coverage for the
--globalflag (which writes to~/.config/fyai.yaml). Based on past review comments, the previous global init test was removed due to platform-specific issues withdirs::home_dir().Consider adding a test that doesn't rely on environment variables. One approach: mock the config directory resolution by testing
handle_init_subcommandwith a temporary directory structure that simulates the config directory path, or consider refactoringhandle_init_subcommandto accept optional path overrides for testing (as suggested in past reviews but not yet implemented).
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/tests/clipboard_tests.rs(1 hunks)src/tests/main_tests.rs(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/tests/main_tests.rs (2)
src/cli.rs (1)
create_commands(3-107)src/main.rs (1)
handle_init_subcommand(43-106)
src/tests/clipboard_tests.rs (1)
src/clipboard.rs (1)
copy_to_clipboard(7-19)
🪛 GitHub Actions: CI
src/tests/main_tests.rs
[error] 42-42: error: struct EnvVarGuard is never constructed. note: -D dead-code implied by -D warnings. help: to override -D warnings add #[allow(dead_code)]
[error] 48-48: error: associated function set is never used
🔇 Additional comments (7)
src/tests/main_tests.rs (6)
1-14: LGTM! Clean test setup.The imports are appropriate and the static mutex initialization for test serialization is a standard pattern for preventing race conditions in tests that modify global state.
16-20: Excellent fix for mutex poisoning!The
lock_tests()helper correctly handlesPoisonErrorby recovering the inner guard, preventing cascading test failures when a test panics while holding the lock. This properly addresses the concern raised in previous reviews.
22-39: LGTM! Proper RAII guard implementation.The
CwdGuardcorrectly implements the RAII pattern to restore the working directory on drop. The best-effort restore indrop()is appropriate for test cleanup.
67-89: LGTM! Well-structured test.The test properly uses serialization, temporary directories, and guard utilities for clean isolation. The validation checks both file creation and template content, providing good coverage of the local init functionality.
91-110: LGTM! Good error case coverage.The test correctly validates the error handling when attempting to initialize a config file that already exists without the
--forceflag. Proper validation of both error occurrence and error kind.
112-133: LGTM! Force overwrite properly tested.The test correctly validates the
--forceflag behavior, ensuring that existing files are overwritten with the template content. Good coverage of this important edge case.src/tests/clipboard_tests.rs (1)
13-16: LGTM: CI skip comment is accurate.The updated comment correctly describes the CI-only skip logic and aligns with the actual check.
1 participant
Summary by CodeRabbit
New Features
Improvements
Chores
Tests
✏️ Tip: You can customize this high-level summary in your review settings.