Skip to content

fix(indexer): skip directories starting with '.' (#20)#22

Merged
dubadub merged 4 commits into
mainfrom
fix/skip-dot-directories
May 18, 2026
Merged

fix(indexer): skip directories starting with '.' (#20)#22
dubadub merged 4 commits into
mainfrom
fix/skip-dot-directories

Conversation

@dubadub

@dubadub dubadub commented May 18, 2026

Copy link
Copy Markdown
Member

Summary

  • Stops the local indexer from picking up any file located inside a directory whose name starts with . (e.g. .git/, .vscode/, notes/.tmp/).
  • Files starting with . directly under the storage root remain eligible, preserving the existing .shopping-list / .shopping-checked / .bookmarks whitelist in chunker::is_text.
  • Implemented with WalkDir::filter_entry so the indexer prunes hidden subtrees during the walk rather than walking and discarding — real IO win for big hidden dirs like .git/.

Closes #20.

Scope decisions

  • Upload-side only. Pre-existing dot-dir content on the server can still be downloaded; we just stop indexing more. Symmetric download-side filtering was considered and explicitly deferred.
  • Storage-root exemption. is_dot_dir guards on depth() > 0, so users with a hidden storage path (e.g. ~/.cooklang) still get their files indexed.
  • Root dotfile whitelist preserved. Files (not directories) with a leading dot at the root flow through filter_entry and are evaluated by filter_eligible / chunker::is_text as before.

Full design and rationale: docs/superpowers/specs/2026-05-17-skip-dot-directories-design.md.

Test plan

  • New unit tests in client/src/indexer.rs cover four behaviours: prune top-level dot-dir, prune nested dot-dir, keep whitelisted root dotfile, keep files when storage root itself is hidden.
  • cargo test --workspace — all crates green.
  • Manual smoke test on a real recipe folder containing a .git/ directory.

Out of scope

  • Configurable ignore patterns (e.g. .syncignore). YAGNI.
  • Symmetric download-side filtering.

@claude

claude Bot commented May 18, 2026

Copy link
Copy Markdown

Code Review

Overview

This PR fixes issue #20 by preventing the client indexer from walking into hidden directories (those starting with .). The core change is minimal — one helper function and one extra .filter_entry step in the WalkDir chain — and the approach is well-reasoned: filter_entry prunes entire subtrees rather than discarding visited entries post-walk, which is a real IO win for large directories like .git/.


Implementation feedback

Minor: is_dot_dir should own its is_dir check

The call site reads:

.filter_entry(|e| !(e.file_type().is_dir() && is_dot_dir(e)))

The is_dot_dir function checks the name and depth but not whether the entry is actually a directory. That split is slightly surprising — a reader calling is_dot_dir on a file entry would get true for .gitignore at depth 1, which contradicts the function name. Moving the is_dir() guard into the helper makes both sides cleaner:

fn is_dot_dir(e: &walkdir::DirEntry) -> bool {
    e.file_type().is_dir()
        && e.depth() > 0
        && e.file_name().to_str().is_some_and(|s| s.starts_with('.'))
}

// call site becomes:
.filter_entry(|e| !is_dot_dir(e))

Question: dotfiles inside normal subdirectories

The filter only prunes directories whose names start with .; a file named recipes/.notes (dotfile inside a normal dir, not itself inside a dot-dir) would still flow through filter_entry and reach filter_eligible. Is that intentional? The spec says only dotfiles at the storage root should be whitelisted, but the current implementation effectively whitelists them everywhere. If this is fine, a brief comment on the call site clarifying that files (not dirs) with leading dots always pass filter_entry would save the next reader from having to puzzle it out.


Tests

The four test cases cover exactly the behavioural contract described in the spec, and using a real filesystem via TempDir is the right call here. Two small observations:

  1. Repeated diagnostic boilerplate. Every assertion duplicates records.keys().collect::<Vec<_>>() as its failure message. A one-liner binding at the top of each test body reduces the noise without losing debuggability:

    let keys: Vec<_> = records.keys().collect();
    assert_eq!(records.len(), 1, "expected 1; got {keys:?}");
  2. Missing edge case: dotfile inside a normal subdirectory. Following on the question above — a test for recipes/.hidden-notes (a dotfile file, not inside a dot-dir) would confirm the intended behaviour and prevent accidental regressions if someone later tightens the filter.


Documentation files

docs/superpowers/plans/2026-05-17-skip-dot-directories.md and docs/superpowers/specs/... look like scaffolding artifacts from an agentic workflow: the plan file opens with For agentic workers: REQUIRED SUB-SKILL and contains step-by-step checkbox instructions. Committing these to the repo seems unintentional. The spec/design doc has some value as a decision log, but the plan file is purely procedural scaffolding. Recommend dropping docs/superpowers/plans/ before merge, and evaluating whether the spec belongs in docs/ or just in the PR description/issue.


Summary

Core logic Correct and efficient
is_dot_dir encapsulation Minor — move is_dir() into the helper
Test coverage Four cases, all meaningful
Dotfile-in-subdir behaviour Needs clarification or a test
Docs scaffolding committed Plan file shouldn't be in the repo

Good fix overall — the filter_entry choice in particular shows care about real-world performance.

@dubadub
dubadub merged commit ec49a05 into main May 18, 2026
7 checks passed
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.

don't sync directories starting with '.'

1 participant