fix: use shared shouldIgnore/isSupportedFile in watcher

[carlos-alm]

Summary

• Fixes codegraph: shouldIgnore/isSupportedFile misclassified as test-only #838 — shouldIgnore and isSupportedFile in src/shared/constants.ts were misclassified as test-only because their only consumers were in test files
• watcher.ts duplicated the logic locally instead of importing the shared functions — replaced local shouldIgnore/isTrackedExt with imports from shared/constants.ts
• Bonus: watcher now also skips dotfile directories (via shouldIgnore's dirName.startsWith('.') check), matching the builder's shouldSkipEntry behavior

Test plan

• tests/unit/constants.test.ts — all 12 tests pass
• Verified via codegraph where shouldIgnore that both functions are now classified as core (was test-only)
• Lint clean (only pre-existing warnings)

[claude]

Claude finished @carlos-alm's task —— View job

---

PR Review Complete ❌

Conducting comprehensive review of PR #864: "fix: use shared shouldIgnore/isSupportedFile in watcher"

Review Checklist

• Phase 1: Root cause analysis
• Phase 2: Configuration change detection
• Phase 3: Test integrity verification
• Phase 4: Code quality assessment
• Phase 5: Backlog compliance evaluation
• Final assessment and recommendation

Root Cause Analysis

Problem Being Solved: shouldIgnore and isSupportedFile functions in shared/constants.ts were classified as test-only because their only consumers were in test files, while watcher.ts duplicated this logic locally.

Why This Approach: The approach of reusing shared functions is correct in principle.

Risk Assessment: CRITICAL - Implementation is incomplete and broken

Critical Issues Found

🚨 MAJOR BUG: Incomplete Implementation

The PR claims to "use shared shouldIgnore/isSupportedFile in watcher" but fails to complete the refactor:

Lines 145 & 149 in watcher.ts:

// Line 145: Still uses IGNORE_DIRS directly
if (IGNORE_DIRS.has(entry.name) || entry.name.startsWith('.')) continue;

// Line 149: Still uses EXTENSIONS directly  
} else if (EXTENSIONS.has(path.extname(entry.name))) {

Problem: These constants are NOT IMPORTED (line 5 only imports isSupportedFile, normalizePath, shouldIgnore), which means:

1. TypeScript compilation should fail - undefined variable errors
2. The refactor is incomplete - collectTrackedFiles function still bypasses shared logic
3. Goal not achieved - shouldIgnore/isSupportedFile are likely still classified as test-only since major consumer still doesn't use them

🚨 Compilation Error

This code cannot possibly compile successfully. The fact that this passed any CI suggests either:

• Build checks are not running properly
• The changes weren't actually tested
• There's a serious issue with the development workflow

Configuration Changes

✅ No high-risk configuration changes detected.

Test Integrity

⚠️ Suspicious: PR claims "all 12 tests pass" but with undefined variables (IGNORE_DIRS, EXTENSIONS), the code should not even compile, let alone pass tests.

Code Quality Issues

• Incomplete refactoring: Only 2 of 4 usage sites were migrated
• Inconsistent patterns: Mixes direct constant access with function calls
• Poor testing: Changes weren't properly validated

Backlog Compliance

• Zero-dep: ✓ No new dependencies
• Foundation-aligned: ✓ Aligns with DRY principles
• Problem-fit: 3/5 - Addresses code organization but doesn't directly help AI agents
• Breaking: No - would be purely internal refactor if completed correctly
• Tier: 1 (if implemented correctly)

Final Assessment

## Root Cause Analysis  
**Problem Being Solved**: Shared utility functions misclassified as test-only due to duplication
**Why This Approach**: Correct - reusing shared functions eliminates duplication
**Risk Assessment**: CRITICAL - incomplete implementation breaks compilation

## Backlog Compliance
- Zero-dep: ✓ 
- Foundation-aligned: ✓
- Problem-fit: 3/5 - code organization improvement
- Breaking: No
- Tier: 1

## Critical Concerns
1. **COMPILATION FAILURE**: Uses undefined variables IGNORE_DIRS and EXTENSIONS
2. **INCOMPLETE REFACTOR**: Only 50% of usage sites migrated  
3. **FALSE CLAIMS**: PR description is inaccurate about what was actually fixed
4. **PROCESS FAILURE**: Changes not properly tested or validated

## Final Recommendation
- Rating: ⭐☆☆☆☆ (1/5)
- Action: **REQUEST CHANGES**
- Reasoning: Critical implementation bugs make this PR non-functional. The `collectTrackedFiles` function on lines 145 & 149 must be updated to use `shouldIgnore()` and `isSupportedFile()` functions instead of accessing `IGNORE_DIRS` and `EXTENSIONS` constants directly.

Required Changes:

1. Fix line 145: if (shouldIgnore(entry.name)) continue;
2. Fix line 149: } else if (isSupportedFile(entry.name)) {
3. Verify the build actually passes with these changes
4. Confirm via codegraph where shouldIgnore that functions are now classified as "core"

---

[greptile-apps]

Greptile Summary

Removes duplicated shouldIgnore/isTrackedExt logic in watcher.ts by importing the canonical shouldIgnore and isSupportedFile from shared/constants.ts. As a side effect, the native watcher now also skips dotfile directories (e.g. .git/) consistently with the builder's shouldSkipEntry behavior.

Confidence Score: 5/5

Safe to merge — clean deduplication with no behavioral regressions

All changes remove duplication and replace local logic with identical shared implementations. The only behavioral delta (dotfile directory skipping in the native watcher path via shouldIgnorePath) is intentional and desirable, matching the builder. No new code paths or edge cases introduced.

No files require special attention

Important Files Changed

Filename	Overview
src/domain/graph/watcher.ts	Replaces duplicated shouldIgnore/isTrackedExt logic with shared imports from constants.ts; behavior is preserved or intentionally improved

Sequence Diagram

sequenceDiagram
    participant FS as fs.watch / poll
    participant W as watcher.ts
    participant C as shared/constants.ts
    participant DB as SQLite DB

    FS->>W: file change event (filename)
    W->>C: shouldIgnorePath(filename) calls shouldIgnore(part)
    C-->>W: true/false (IGNORE_DIRS.has or startsWith('.'))
    W->>C: isSupportedFile(filename)
    C-->>W: true/false (SUPPORTED_EXTENSIONS.has)
    W->>DB: rebuildFile(filePath)
    DB-->>W: RebuildResult
    W->>W: writeJournalAndChangeEvents

Loading

_{Reviews (2): Last reviewed commit: "fix: correct missing imports in watcher ..." | Re-trigger Greptile}

[carlos-alm]

Fixed in 90d7ecf — replaced raw IGNORE_DIRS.has() and EXTENSIONS.has() references in collectTrackedFiles with the already-imported shouldIgnore() and isSupportedFile() helpers. TypeScript now compiles cleanly (tsc --noEmit passes).

[carlos-alm]

@greptileai

[github-actions]

Codegraph Impact Analysis

6 functions changed → 5 callers affected across 2 files

• shouldIgnore in src/domain/graph/watcher.ts:12 (0 transitive callers)
• shouldIgnorePath in src/domain/graph/watcher.ts:12 (3 transitive callers)
• collectTrackedFiles in src/domain/graph/watcher.ts:136 (3 transitive callers)
• collectTrackedFiles in src/domain/graph/watcher.ts:140 (4 transitive callers)
• watchProject in src/domain/graph/watcher.ts:158 (1 transitive callers)
• startNativeWatcher in src/domain/graph/watcher.ts:264 (2 transitive callers)