fix: vault watcher reload loop on iCloud .git symlink vaults#412
fix: vault watcher reload loop on iCloud .git symlink vaults#412kossoy wants to merge 48 commits intorefactoringhq:mainfrom
Conversation
Vaults stored on iCloud / Dropbox commonly use a `.git -> .git.nosync` symlink so the cloud sync engine ignores git internals. Background `git fetch` calls (auto-pull, remote-status, gitstatusd) wrote to the real `.git.nosync/` directory, which the watcher's `.git` component filter did not match — every fetch fired a `vault-changed` event and triggered `reloadVault`, flickering the "Reload vault" status badge multiple times per second. Resolve the real git directory at watcher start (symlink, regular dir, or `gitdir:` pointer for worktrees/submodules) and reject any path inside it. Also filter `.gitstatus.*` cache dirs and `*.icloud` placeholders, which are noisy on the same setups.
`run-vitest-coverage.mjs` reused `process.env.npm_execpath` with `process.execPath` (node) for a fast path that skips the pnpm wrapper. That assumes `npm_execpath` is a JS file, but standalone pnpm installs ship a native Mach-O / ELF binary, so node tries to load the binary as a CJS module and crashes with `SyntaxError: Invalid or unexpected token`, taking the pre-push hook down with it. Only take the fast path when the path actually looks like a JS module (`.js`/`.mjs`/`.cjs`); otherwise fall back to invoking `pnpm` from PATH, which is what the script already does when `npm_execpath` is unset.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: da8ece8c23
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| name == ".DS_Store" | ||
| || name == ".tolaria-rename-txn" | ||
| || name.starts_with(".#") | ||
| || name.starts_with(".gitstatus.") |
There was a problem hiding this comment.
Filter .gitstatus dirs by component, not basename
The new .gitstatus. ignore rule only inspects path.file_name(), so it ignores events for the directory node itself but still allows events from files inside it. For example, /vault/.gitstatus.ABCD/cache/state has basename state, so is_watchable_path returns true and still emits vault-changed, which can keep the reload churn this patch is trying to eliminate. This should be applied at the path-component level (like the .git check) so nested entries under .gitstatus.* are excluded too.
Useful? React with 👍 / 👎.
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 67 |
| Duplication | 0 |
AI Reviewer: first review requested successfully. AI can make mistakes. Always validate suggestions.
TIP This summary will be updated as you push new changes.
There was a problem hiding this comment.
Pull Request Overview
The PR successfully addresses the vault watcher reload loop issue related to iCloud symlinks and git worktrees. It also improves the compatibility of the Vitest coverage script with native pnpm binaries. While the implementation aligns with the requirements, there is a risk of build failures on Windows due to platform-specific test code. Additionally, the cyclomatic complexity of the file filtering logic has increased beyond recommended thresholds and should be refactored to maintain long-term quality.
About this PR
- Git directory resolution occurs only once during the vault watcher initialization. If a user initializes a new git repository or adds a worktree while the vault is already active, those paths will not be ignored until the application is restarted or the vault is re-opened.
1 comment outside of the diff
src-tauri/src/vault_watcher.rs
line 22🟡 MEDIUM RISK
This function's cyclomatic complexity (9) exceeds the recommended limit of 8. To improve maintainability, consider refactoring the patterns into a collection (e.g., a static array or HashSet) and using an iterator to check for matches rather than a growing chain of boolean operators.
Test suggestions
- Resolve .git as a symlink to a .nosync directory
- Resolve .git as a worktree pointer using 'gitdir: '
- Filter out file changes occurring within the resolved git directory
- Filter out temporary .gitstatus.* cache files
- Filter out .icloud placeholder files
- Vitest script correctly identifies native pnpm binary vs JS module
TIP Improve review quality by adding custom instructions
TIP How was this review? Give us feedback
| let dir = tempfile::tempdir().unwrap(); | ||
| let real_git = dir.path().join(".git.nosync"); | ||
| std::fs::create_dir(&real_git).unwrap(); | ||
| std::os::unix::fs::symlink(".git.nosync", dir.path().join(".git")).unwrap(); |
There was a problem hiding this comment.
🟡 MEDIUM RISK
This test case uses Unix-specific symlink creation (std::os::unix::fs::symlink), which will cause compilation or runtime errors on Windows. To ensure the test suite is cross-platform, wrap this test in a #[cfg(unix)] attribute or use a platform-agnostic crate like symlink for test setup.
| Some(if target.is_absolute() { | ||
| target.to_path_buf() | ||
| } else { | ||
| vault_path.join(target) | ||
| }) |
There was a problem hiding this comment.
⚪ LOW RISK
Suggestion: Redundant absolute path check; vault_path.join(target) is sufficient here as well.
| if let Ok(target) = std::fs::read_link(&git_path) { | ||
| let resolved = if target.is_absolute() { | ||
| target | ||
| } else { | ||
| vault_path.join(target) | ||
| }; | ||
| return Some(resolved); | ||
| } |
There was a problem hiding this comment.
⚪ LOW RISK
Suggestion: This manual absolute path check is redundant. In Rust, Path::join (and PathBuf::push) automatically handles absolute paths correctly; if the provided argument is absolute, it replaces the previous path entirely.
| if let Ok(target) = std::fs::read_link(&git_path) { | |
| let resolved = if target.is_absolute() { | |
| target | |
| } else { | |
| vault_path.join(target) | |
| }; | |
| return Some(resolved); | |
| } | |
| if let Ok(target) = std::fs::read_link(&git_path) { | |
| return Some(vault_path.join(target)); | |
| } |
Summary
.git -> .git.nosyncpaths, which was triggering areloadVaultloop on iCloud-backed vaults — the "Reload vault" status badge flickered every ~1s while idle. Resolves the symlink (andgitdir:worktree pointers) at watcher start and rejects any path inside the resolved directory. Also filters.gitstatus.*cache dirs and*.icloudplaceholders.scripts/run-vitest-coverage.mjsfor standalone pnpm installs: it was passing the pnpm Mach-O / ELF binary toprocess.execPath(node) and crashing the pre-push hook withSyntaxError: Invalid or unexpected token. Now only takes that fast path whennpm_execpathactually looks like a JS module.Repro for the watcher fix
iCloud / Dropbox Obsidian vaults commonly use the
.git -> .git.nosynctrick (see vault.gitignore). Backgroundgit fetchcalls (auto-pull,git_remote_status, gitstatusd) write to the real.git.nosync/directory, which the existing.gitcomponent filter does not match. Each event firedvault-changed→reloadVault, with a 350ms debounce that produced multiple concurrent vault scans per second (visible asSkipped replacing cache because another writer is activewarnings in the Rust log).Test plan
cargo test --lib vault_watcher— 7 tests, all green (3 new resolver cases + 1 new ignore case + extended temp-file coverage)pnpm test:coverage— 3378 tests pass with the script fix on a standalone pnpm install