Skip to content

Commit b9f92c1

Browse files
committed
refactor: extract shared file and path utilities
- Create shared/file.utils.ts for safe file operations - Create shared/path.utils.ts for path manipulation - Refactor 6 modules to use shared utilities - Add comprehensive tests and implementation plan Eliminates duplicate file reading patterns and path matching logic. close #31
1 parent ebde4da commit b9f92c1

11 files changed

Lines changed: 483 additions & 99 deletions
Lines changed: 135 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,135 @@
1+
# REFACTOR-001 Implementation Plan
2+
3+
## Analysis Summary
4+
5+
### Issue 1: File Reading Pattern Duplication
6+
7+
**Originally estimated**: 3 locations
8+
**Actually found**: 6 locations (+ 3 additional in config.analyzer.ts)
9+
10+
| Module | Pattern | Return on Error |
11+
|--------|---------|-----------------|
12+
| ignore.parser.ts | readFile → catch → return empty | `{ patterns: [], source: null }` |
13+
| context.loader.ts (loadContextFile) | readFile → catch → return null | `null` |
14+
| context.loader.ts (getAllFiles) | readdir → catch → skip | (continues) |
15+
| code.sampler.ts | stat + readFile → catch → skip | (continues) |
16+
| package.analyzer.ts | readFile → catch → return null | `null` |
17+
| config.analyzer.ts | readFile → catch → skip (×3) | (continues) |
18+
19+
**Key observation**: None check for ENOENT specifically. All use bare `catch` blocks.
20+
21+
### Issue 2: Path Matching Logic Duplication
22+
23+
| Pattern Type | Location | Approach |
24+
|--------------|----------|----------|
25+
| Path separator handling | code.sampler.ts | Check both `/` and `\` manually |
26+
| Path separator handling | ignore.parser.ts | Normalize `\` to `/` upfront |
27+
| Wildcard matching | config.analyzer.ts | Simple `*``.*` regex |
28+
| Glob pattern matching | ignore.parser.ts | Full gitignore-style (**, !, anchors) |
29+
| Directory in path check | code.sampler.ts | Custom `pathContainsDir()` helper |
30+
| Directory in path check | directory.analyzer.ts | Inline string checks |
31+
32+
---
33+
34+
## Implementation Plan
35+
36+
### Phase 1: Create Shared File Utilities
37+
38+
**Goal**: Consolidate file reading patterns into reusable utilities
39+
40+
**New module**: `src/shared/file.utils.ts`
41+
42+
**Functions to create**:
43+
44+
1. `safeReadFile(path): Promise<string | null>`
45+
- Returns file content or null if not found
46+
- Throws on permission/other errors
47+
48+
2. `safeReadFileWithFallback<T>(path, fallback): Promise<T>`
49+
- Returns parsed content or fallback value
50+
- Useful for config files with default empty objects
51+
52+
3. `tryReadFile(path): Promise<string | undefined>`
53+
- Silent failure variant for skip-on-error patterns
54+
55+
**Refactoring targets** (6 files):
56+
- `src/config/ignore.parser.ts`
57+
- `src/config/context.loader.ts`
58+
- `src/analyzer/code.sampler.ts`
59+
- `src/analyzer/package.analyzer.ts`
60+
- `src/analyzer/config.analyzer.ts`
61+
62+
### Phase 2: Create Shared Path Utilities
63+
64+
**Goal**: Unify path matching approaches
65+
66+
**New module**: `src/shared/path.utils.ts`
67+
68+
**Functions to create**:
69+
70+
1. `normalizePath(path): string`
71+
- Converts all `\` to `/`
72+
- Single source of truth for path normalization
73+
74+
2. `pathContainsSegment(path, segment): boolean`
75+
- Checks if path contains a directory segment
76+
- Replaces manual `/dir/` and `\dir\` checks
77+
78+
3. `matchesGlobPattern(path, pattern): boolean`
79+
- Wrapper around existing `patternToRegex` in ignore.parser.ts
80+
- Reusable for config.analyzer.ts wildcard matching
81+
82+
**Refactoring targets** (3 files):
83+
- `src/analyzer/code.sampler.ts`
84+
- `src/analyzer/config.analyzer.ts`
85+
- `src/analyzer/directory.analyzer.ts`
86+
87+
---
88+
89+
## Execution Order
90+
91+
```
92+
Step 1: Create src/shared/file.utils.ts + tests
93+
94+
Step 2: Refactor config/ modules to use file.utils
95+
96+
Step 3: Refactor analyzer/ modules to use file.utils
97+
98+
Step 4: Create src/shared/path.utils.ts + tests
99+
100+
Step 5: Refactor analyzer/ modules to use path.utils
101+
102+
Step 6: Run full test suite, verify 269+ tests pass
103+
```
104+
105+
---
106+
107+
## Risk Mitigation
108+
109+
| Risk | Mitigation |
110+
|------|------------|
111+
| Breaking error handling behavior | Keep same return types (null, empty object, skip) |
112+
| Missing edge cases | Existing tests cover current behavior |
113+
| Import cycle | shared/ has no dependencies on config/ or analyzer/ |
114+
115+
---
116+
117+
## Success Metrics
118+
119+
- [ ] `safeReadFile` pattern used in all 6 locations
120+
- [ ] `normalizePath` used consistently for path operations
121+
- [ ] No duplicate file reading try-catch blocks remain
122+
- [ ] No duplicate path separator handling remains
123+
- [ ] All 269 existing tests pass
124+
- [ ] New utility functions have dedicated test coverage
125+
126+
---
127+
128+
## Estimated Scope
129+
130+
| Item | Count |
131+
|------|-------|
132+
| New files | 4 (2 utils + 2 spec files) |
133+
| Modified files | 6 |
134+
| New functions | 5-6 |
135+
| Risk level | Low |

mcp-server/src/analyzer/code.sampler.ts

Lines changed: 11 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
import * as fs from 'fs/promises';
22
import * as path from 'path';
33
import type { CodeSample, CodeCategory } from './analyzer.types';
4+
import { pathContainsSegment } from '../shared/path.utils';
45

56
/**
67
* Language detection by file extension
@@ -125,18 +126,6 @@ export function isCodeFile(filePath: string): boolean {
125126
return CODE_EXTENSIONS.has(ext);
126127
}
127128

128-
/**
129-
* Check if path contains a directory (handles both /dir/ and dir/ at start)
130-
*/
131-
function pathContainsDir(lowerPath: string, dirName: string): boolean {
132-
return (
133-
lowerPath.includes(`/${dirName}/`) ||
134-
lowerPath.includes(`\\${dirName}\\`) ||
135-
lowerPath.startsWith(`${dirName}/`) ||
136-
lowerPath.startsWith(`${dirName}\\`)
137-
);
138-
}
139-
140129
/**
141130
* Categorize a file based on its path and name
142131
*/
@@ -156,25 +145,25 @@ export function categorizeFile(filePath: string): CodeCategory {
156145

157146
// API/Route files (check before hooks to avoid false positives like 'users.ts')
158147
if (
159-
pathContainsDir(lower, 'api') ||
148+
pathContainsSegment(filePath, 'api') ||
160149
fileName === 'route.ts' ||
161150
fileName === 'route.js'
162151
) {
163152
return 'api';
164153
}
165154

166155
// Service files
167-
if (pathContainsDir(lower, 'services') || fileName.includes('.service.')) {
156+
if (pathContainsSegment(filePath, 'services') || fileName.includes('.service.')) {
168157
return 'service';
169158
}
170159

171160
// Model/Entity files
172-
if (pathContainsDir(lower, 'models') || pathContainsDir(lower, 'entities')) {
161+
if (pathContainsSegment(filePath, 'models') || pathContainsSegment(filePath, 'entities')) {
173162
return 'model';
174163
}
175164

176165
// Hook files (React hooks pattern - useXxx where X is uppercase)
177-
if (pathContainsDir(lower, 'hooks')) {
166+
if (pathContainsSegment(filePath, 'hooks')) {
178167
return 'hook';
179168
}
180169
// Check for useXxx pattern (use followed by uppercase letter)
@@ -184,13 +173,13 @@ export function categorizeFile(filePath: string): CodeCategory {
184173
}
185174

186175
// Component files
187-
if (pathContainsDir(lower, 'components') || pathContainsDir(lower, 'ui')) {
176+
if (pathContainsSegment(filePath, 'components') || pathContainsSegment(filePath, 'ui')) {
188177
return 'component';
189178
}
190179

191180
// Page files
192181
if (
193-
pathContainsDir(lower, 'pages') ||
182+
pathContainsSegment(filePath, 'pages') ||
194183
fileName === 'page.tsx' ||
195184
fileName === 'page.ts' ||
196185
fileName === 'page.jsx' ||
@@ -201,15 +190,15 @@ export function categorizeFile(filePath: string): CodeCategory {
201190

202191
// Utility files
203192
if (
204-
pathContainsDir(lower, 'utils') ||
205-
pathContainsDir(lower, 'lib') ||
206-
pathContainsDir(lower, 'helpers')
193+
pathContainsSegment(filePath, 'utils') ||
194+
pathContainsSegment(filePath, 'lib') ||
195+
pathContainsSegment(filePath, 'helpers')
207196
) {
208197
return 'util';
209198
}
210199

211200
// Config files
212-
if (pathContainsDir(lower, 'config')) {
201+
if (pathContainsSegment(filePath, 'config')) {
213202
return 'config';
214203
}
215204

mcp-server/src/analyzer/config.analyzer.ts

Lines changed: 7 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,3 @@
1-
import * as fs from 'fs/promises';
21
import { existsSync } from 'fs';
32
import * as path from 'path';
43
import type {
@@ -7,6 +6,7 @@ import type {
76
EslintSummary,
87
PrettierSummary,
98
} from './analyzer.types';
9+
import { tryReadFile } from '../shared/file.utils';
1010

1111
/**
1212
* Known config file patterns by type
@@ -171,15 +171,13 @@ export async function analyzeConfigs(
171171
if (!pattern.includes('*')) {
172172
const filePath = path.join(projectRoot, pattern);
173173
if (existsSync(filePath)) {
174-
try {
175-
const content = await fs.readFile(filePath, 'utf-8');
174+
const content = await tryReadFile(filePath);
175+
if (content !== undefined) {
176176
const parsed = parseTsConfig(content, pattern);
177177
if (parsed) {
178178
result.typescript = parsed;
179179
break;
180180
}
181-
} catch {
182-
// Ignore read errors
183181
}
184182
}
185183
}
@@ -190,15 +188,13 @@ export async function analyzeConfigs(
190188
for (const fileName of eslintJsonFiles) {
191189
const filePath = path.join(projectRoot, fileName);
192190
if (existsSync(filePath)) {
193-
try {
194-
const content = await fs.readFile(filePath, 'utf-8');
191+
const content = await tryReadFile(filePath);
192+
if (content !== undefined) {
195193
const parsed = parseEslintConfig(content, fileName, getEslintFormat(fileName));
196194
if (parsed) {
197195
result.eslint = parsed;
198196
break;
199197
}
200-
} catch {
201-
// Ignore read errors
202198
}
203199
}
204200
}
@@ -221,15 +217,13 @@ export async function analyzeConfigs(
221217
for (const fileName of prettierJsonFiles) {
222218
const filePath = path.join(projectRoot, fileName);
223219
if (existsSync(filePath)) {
224-
try {
225-
const content = await fs.readFile(filePath, 'utf-8');
220+
const content = await tryReadFile(filePath);
221+
if (content !== undefined) {
226222
const parsed = parsePrettierConfig(content, fileName);
227223
if (parsed) {
228224
result.prettier = parsed;
229225
break;
230226
}
231-
} catch {
232-
// Ignore read errors
233227
}
234228
}
235229
}

mcp-server/src/analyzer/directory.analyzer.ts

Lines changed: 22 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,8 @@
1-
import * as fs from 'fs/promises';
21
import { existsSync } from 'fs';
32
import * as path from 'path';
43
import type { DirectoryAnalysis, ArchitecturePattern } from './analyzer.types';
54
import { shouldIgnore, getDefaultIgnorePatterns } from '../config/ignore.parser';
5+
import { safeReadDirWithTypes } from '../shared/file.utils';
66

77
/**
88
* Architecture pattern definition
@@ -157,32 +157,28 @@ async function scanDirectory(
157157
return { files, dirs };
158158
}
159159

160-
try {
161-
const entries = await fs.readdir(currentPath, { withFileTypes: true });
162-
163-
for (const entry of entries) {
164-
const entryRelativePath = relativePath
165-
? `${relativePath}/${entry.name}`
166-
: entry.name;
167-
168-
// Check if should be ignored
169-
if (shouldIgnore(entryRelativePath, ignorePatterns)) {
170-
continue;
171-
}
172-
173-
if (entry.isDirectory()) {
174-
dirs.push(entryRelativePath);
175-
176-
// Recursively scan subdirectories
177-
const subResult = await scanDirectory(rootPath, ignorePatterns, entryRelativePath);
178-
files.push(...subResult.files);
179-
dirs.push(...subResult.dirs);
180-
} else if (entry.isFile()) {
181-
files.push(entryRelativePath);
182-
}
160+
const entries = await safeReadDirWithTypes(currentPath);
161+
162+
for (const entry of entries) {
163+
const entryRelativePath = relativePath
164+
? `${relativePath}/${entry.name}`
165+
: entry.name;
166+
167+
// Check if should be ignored
168+
if (shouldIgnore(entryRelativePath, ignorePatterns)) {
169+
continue;
170+
}
171+
172+
if (entry.isDirectory()) {
173+
dirs.push(entryRelativePath);
174+
175+
// Recursively scan subdirectories
176+
const subResult = await scanDirectory(rootPath, ignorePatterns, entryRelativePath);
177+
files.push(...subResult.files);
178+
dirs.push(...subResult.dirs);
179+
} else if (entry.isFile()) {
180+
files.push(entryRelativePath);
183181
}
184-
} catch {
185-
// Ignore permission errors
186182
}
187183

188184
return { files, dirs };

mcp-server/src/analyzer/package.analyzer.ts

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,11 +1,11 @@
1-
import * as fs from 'fs/promises';
21
import { existsSync } from 'fs';
32
import * as path from 'path';
43
import type {
54
PackageInfo,
65
DetectedFramework,
76
FrameworkCategory,
87
} from './analyzer.types';
8+
import { safeReadFile } from '../shared/file.utils';
99

1010
/**
1111
* Framework detection definition
@@ -171,8 +171,13 @@ export async function analyzePackage(projectRoot: string): Promise<PackageInfo |
171171
return null;
172172
}
173173

174+
const content = await safeReadFile(packagePath);
175+
176+
if (content === null) {
177+
return null;
178+
}
179+
174180
try {
175-
const content = await fs.readFile(packagePath, 'utf-8');
176181
const parsed = parsePackageJson(content);
177182
const frameworks = detectFrameworks(parsed.dependencies, parsed.devDependencies);
178183

0 commit comments

Comments
 (0)