Skip to content

Commit fc8eb35

Browse files
PatrickSysPatrickSys
authored
feat: v1.3.0 foundation (workspace utils, metadata fix, testing) (#4)
* feat: v1.3.0 foundation (testing, workspace utils, metadata fix) * fix: resolve dependency category type mismatch in build
1 parent 2fa3eb7 commit fc8eb35

File tree

14 files changed

+1114
-70
lines changed

14 files changed

+1114
-70
lines changed

‎.github/workflows/test.yml‎

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,20 @@
1+
name: Tests
2+
3+
on:
4+
push:
5+
branches: [master]
6+
pull_request:
7+
branches: [master]
8+
9+
jobs:
10+
test:
11+
runs-on: ubuntu-latest
12+
steps:
13+
- uses: actions/checkout@v4
14+
- uses: actions/setup-node@v4
15+
with:
16+
node-version: '20'
17+
cache: 'npm'
18+
- run: npm ci
19+
- run: npm run build
20+
- run: npm test

‎CHANGELOG.md‎

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,27 @@
11
# Changelog
22

33

4+
## [1.3.0] - 2026-01-01
5+
6+
### Added
7+
- **Workspace Detection**: Monorepo support for Nx, Turborepo, Lerna, and pnpm workspaces
8+
- New utility: `src/utils/workspace-detection.ts`
9+
- Functions: `scanWorkspacePackageJsons()`, `detectWorkspaceType()`, `aggregateWorkspaceDependencies()`
10+
- **Testing Infrastructure**: Vitest smoke tests for core utilities
11+
- Tests for workspace detection, analyzer registry, and indexer metadata
12+
- CI/CD workflow via GitHub Actions
13+
- **Dependency Detection**: Added `@nx/` and `@nrwl/` prefix matching for build tools
14+
15+
### Fixed
16+
- **detectMetadata() bug**: All registered analyzers now contribute to codebase metadata (previously only the first analyzer was called)
17+
- Added `mergeMetadata()` helper with proper array deduplication and layer merging
18+
19+
### Changed
20+
- Updated roadmap: v1.3 is now "Extensible Architecture Foundation"
21+
22+
### Acknowledgements
23+
Thanks to [@aolin480](https://github.com/aolin480) for accelerating the workspace detection roadmap and identifying the detectMetadata() limitation in their fork.
24+
425
## 1.2.2 (2025-12-31)
526

627
### Fixed

‎fork-code-review.md‎

Lines changed: 316 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,316 @@
1+
# Fork Code Review: codebase-context-react-nextjs
2+
3+
**Reviewer**: Repository Owner
4+
**Subject**: aolin480/codebase-context-react-nextjs fork analysis
5+
**Date**: 2025-12-31
6+
7+
> [!NOTE]
8+
> **Status Update (2026-01-01)**
9+
>
10+
> This document is now a living record of differences between main and fork.
11+
>
12+
> **Changes since initial review**:
13+
> - ✅ `detectMetadata()` bug fixed in main (loop through all analyzers with merge helpers)
14+
> - ✅ Fork's `EcosystemAnalyzer` removed by contributor (commit `40d500b`)
15+
> - ✅ Workspace scanning implemented in main as `workspace-detection.ts`
16+
> - ✅ Attribution added to CHANGELOG for @aolin480
17+
> - 🔄 React/Next.js analyzers: Awaiting contributor PR (main has stashed versions)
18+
19+
---
20+
21+
## Executive Summary
22+
23+
**Verdict: REJECT the architectural approach. Cherry-pick the useful patterns.**
24+
25+
The fork identified a legitimate bug in my code (`detectMetadata()` only calls the first analyzer). However, instead of fixing the 1-line bug, they built a 1,300+ line workaround that introduces unnecessary architectural complexity. This is a classic case of working AROUND a problem instead of fixing it.
26+
27+
---
28+
29+
## The Bug They Found (Valid)
30+
31+
**Location**: [src/core/indexer.ts:540](file:///c:/Users/bitaz/Repos/codebase-context/src/core/indexer.ts#L540)
32+
33+
```typescript
34+
async detectMetadata(): Promise<CodebaseMetadata> {
35+
const primaryAnalyzer = analyzerRegistry.getAll()[0]; // ← BUG: Only calls first analyzer
36+
// ...
37+
}
38+
```
39+
40+
This is a real bug. In a React project with AngularAnalyzer registered (priority 100), `detectMetadata()` calls AngularAnalyzer even though it's irrelevant. The fork correctly identified this.
41+
42+
---
43+
44+
## What They Built (The Workaround)
45+
46+
Instead of fixing the bug, they built an **EcosystemAnalyzer** with:
47+
- Priority 1000 (highest)
48+
- `canAnalyze() => false` (never handles files)
49+
- Intercepts `detectMetadata()` calls via the unchanged `getAll()[0]` bug
50+
51+
**The Hack**:
52+
```typescript
53+
// Fork's registration order (src/index.ts:33-37)
54+
analyzerRegistry.register(new EcosystemAnalyzer()); // priority: 1000 ← ALWAYS first
55+
analyzerRegistry.register(new AngularAnalyzer()); // priority: 100
56+
analyzerRegistry.register(new NextJsAnalyzer()); // priority: 90
57+
analyzerRegistry.register(new ReactAnalyzer()); // priority: 80
58+
analyzerRegistry.register(new GenericAnalyzer()); // priority: 10
59+
```
60+
61+
When MY unchanged `detectMetadata()` calls `getAll()[0]`, it gets EcosystemAnalyzer (highest priority), which then decides which "real" analyzer to delegate to.
62+
63+
**This doesn't fix the bug. It exploits it as a routing mechanism.**
64+
65+
---
66+
67+
## Verified Facts
68+
69+
### Priorities (Verified from source)
70+
71+
| Analyzer | My Repo | Fork |
72+
|----------|---------|------|
73+
| EcosystemAnalyzer | N/A | **1000** |
74+
| AngularAnalyzer | 100 | 100 |
75+
| NextJsAnalyzer | N/A | 90 |
76+
| ReactAnalyzer | N/A | 80 |
77+
| GenericAnalyzer | 10 | 10 |
78+
79+
### Line Counts (Verified)
80+
81+
| File | Lines | Purpose |
82+
|------|-------|---------|
83+
| `analyzers/orchestration/ecosystem.ts` | **117** | Orchestrator workaround |
84+
| `analyzers/orchestration/package-json.ts` | **136** | Package.json utilities |
85+
| `analyzers/react/index.ts` | **559** | React analyzer |
86+
| `analyzers/nextjs/index.ts` | **452** | Next.js analyzer |
87+
| `utils/async-json.ts` | **45** | Worker-based JSON parse |
88+
| **TOTAL NEW CODE** | **1,309** | |
89+
90+
### Version Comparison (Verified)
91+
92+
| Aspect | My Repo | Fork |
93+
|--------|---------|------|
94+
| package.json version | 1.2.2 | 1.2.0 |
95+
| Server version | 1.2.2 | 1.0.0 |
96+
| Server name | `codebase-context` | `codebase-context-mcp` |
97+
| `logging` capability | ❌ Not declared | ✅ Declared (unimplemented) |
98+
99+
---
100+
101+
## Code Comparison
102+
103+
| Approach | Lines of Code | Files Changed | Architectural Impact |
104+
|----------|---------------|---------------|----------------------|
105+
| **Proper fix** | ~20 lines | 1 file (indexer.ts) | None |
106+
| **Fork's workaround** | 1,309 lines | 6 new files | New orchestration layer |
107+
108+
### The Proper Fix (What I Should Do)
109+
110+
```typescript
111+
// indexer.ts - Fix detectMetadata()
112+
async detectMetadata(): Promise<CodebaseMetadata> {
113+
const framework = await this.detectPrimaryFramework();
114+
const analyzer = analyzerRegistry.get(framework) || analyzerRegistry.getAll()[0];
115+
116+
if (analyzer) {
117+
return analyzer.detectCodebaseMetadata(this.rootPath);
118+
}
119+
// ... fallback
120+
}
121+
122+
private async detectPrimaryFramework(): Promise<string> {
123+
try {
124+
const pkgPath = path.join(this.rootPath, 'package.json');
125+
const pkg = JSON.parse(await fs.readFile(pkgPath, 'utf-8'));
126+
const deps = { ...pkg.dependencies, ...pkg.devDependencies };
127+
128+
if (deps['next']) return 'nextjs';
129+
if (deps['@angular/core']) return 'angular';
130+
if (deps['react']) return 'react';
131+
} catch {}
132+
return 'generic';
133+
}
134+
```
135+
136+
**20 lines. Same result. No new architecture.**
137+
138+
---
139+
140+
## What They Added vs What's Actually Useful
141+
142+
### New Files in Fork (Verified)
143+
144+
| File | Lines | Verdict |
145+
|------|-------|---------|
146+
| `orchestration/ecosystem.ts` | 117 | ❌ **Reject** - Workaround, not fix |
147+
| `orchestration/package-json.ts` | 136 | ⚠️ **Useful patterns** - Extract ~30 lines as utility |
148+
| `react/index.ts` | 559 | ✅ **Useful** - Needed for React support |
149+
| `nextjs/index.ts` | 452 | ✅ **Useful** - Needed for Next.js support |
150+
| `utils/async-json.ts` | 45 | ⚠️ **Maybe** - Worker-based JSON parsing |
151+
152+
### Useful Patterns to Extract
153+
154+
**1. Library categorization** (from package-json.ts):
155+
```typescript
156+
const libraries = {
157+
forms: detectLibraries(allDeps, ['react-hook-form', 'formik']),
158+
validation: detectLibraries(allDeps, ['zod', 'yup', 'joi']),
159+
state: detectLibraries(allDeps, ['@reduxjs/toolkit', 'zustand', 'jotai']),
160+
data: detectLibraries(allDeps, ['@tanstack/react-query', 'swr']),
161+
styling: detectLibraries(allDeps, ['tailwindcss', '@mui/material']),
162+
};
163+
```
164+
165+
**This is ~20 lines. I can add this to GenericAnalyzer directly.**
166+
167+
**2. Workspace scanning** (from package-json.ts):
168+
```typescript
169+
const matches = await glob([
170+
'package.json',
171+
'apps/*/package.json', // Nx/Turborepo
172+
'packages/*/package.json' // Lerna/pnpm
173+
], { cwd: rootPath, ignore: ['**/node_modules/**'] });
174+
```
175+
176+
**This is ~15 lines. Can be a shared utility function.**
177+
178+
---
179+
180+
## What They're Missing
181+
182+
> [!WARNING]
183+
> The fork is based on v1.2.0 and is **missing critical bug fixes**:
184+
185+
| Version | Fix | Impact |
186+
|---------|-----|--------|
187+
| v1.2.1 | MCP protocol stderr output | Fork breaks Warp, OpenCode, MCPJam |
188+
| v1.2.2 | Windows startup crash | Fork crashes on Windows |
189+
190+
**Additional Issue**: Fork declares `logging: {}` capability (line 85 of index.ts) but doesn't implement it. My v1.2.2 correctly removed this unimplemented capability.
191+
192+
---
193+
194+
## Test Coverage (Verified)
195+
196+
| Codebase | Test Files | Count |
197+
|----------|------------|-------|
198+
| **Mine (main)** | `tests/` directory | 0 dedicated test files |
199+
| **Fork** | `tests/` directory | 4 test files |
200+
201+
### Fork's Test Files:
202+
- `react-analyzer.test.ts` (1,451 bytes)
203+
- `react-analyzer.in-depth.test.ts` (4,062 bytes)
204+
- `nextjs-analyzer.test.ts` (1,896 bytes)
205+
- `nextjs-analyzer.in-depth.test.ts` (6,246 bytes)
206+
207+
These are smoke tests that verify analyzers run without errors, not comprehensive unit tests validating behavior.
208+
209+
---
210+
211+
## Architectural Violations
212+
213+
### My Design Principle: Analyzers Are Peers
214+
215+
```
216+
AnalyzerRegistry
217+
|
218+
┌─────────────────┼─────────────────┐
219+
▼ ▼ ▼
220+
AngularAnalyzer ReactAnalyzer GenericAnalyzer
221+
(priority 100) (priority 80) (priority 10)
222+
```
223+
224+
### Fork's Design: Orchestrator Above Peers
225+
226+
```
227+
AnalyzerRegistry
228+
|
229+
▼
230+
EcosystemAnalyzer ← NEW LAYER (priority 1000)
231+
|
232+
┌─────────────────┼─────────────────┐
233+
▼ ▼ ▼
234+
AngularAnalyzer ReactAnalyzer GenericAnalyzer
235+
```
236+
237+
**The fork introduced a hierarchy where I designed a flat peer system.**
238+
239+
---
240+
241+
## EcosystemAnalyzer: The Exploit (Verified)
242+
243+
From [ecosystem.ts:32-34](file:///c:/Users/bitaz/Repos/codebase-context-react-nextjs/src/analyzers/orchestration/ecosystem.ts#L32-L34):
244+
245+
```typescript
246+
canAnalyze(): boolean {
247+
return false; // ← NEVER analyzes files
248+
}
249+
```
250+
251+
This confirms EcosystemAnalyzer is ONLY used for `detectCodebaseMetadata()`. It exploits the `getAll()[0]` bug to intercept project metadata calls while never participating in file analysis.
252+
253+
---
254+
255+
## Final Recommendations
256+
257+
### DO NOT Merge
258+
259+
- ❌ `EcosystemAnalyzer` - Workaround, not solution (117 lines)
260+
- ❌ `orchestration/` directory - Unnecessary layer (253 lines total)
261+
- ❌ Their changes to `index.ts` - Reverts my v1.2.1/v1.2.2 fixes
262+
263+
### DO Extract & Adapt
264+
265+
- ✅ `ReactAnalyzer` logic (559 lines) - Rewrite following my patterns, with tests
266+
- ✅ `NextJsAnalyzer` logic (452 lines) - Rewrite following my patterns, with tests
267+
- ✅ Library categorization (~20 lines) - Add to GenericAnalyzer
268+
- ✅ Workspace scanning (~15 lines) - Add as shared utility
269+
270+
### DO Fix My Bug
271+
272+
```typescript
273+
// indexer.ts:538-544 - REPLACE
274+
async detectMetadata(): Promise<CodebaseMetadata> {
275+
const framework = await this.detectPrimaryFramework();
276+
const analyzer = analyzerRegistry.get(framework)
277+
|| analyzerRegistry.getAll().find(a => a.name !== 'generic')
278+
|| analyzerRegistry.get('generic');
279+
280+
if (analyzer) {
281+
return analyzer.detectCodebaseMetadata(this.rootPath);
282+
}
283+
// ... existing fallback
284+
}
285+
```
286+
287+
---
288+
289+
## Action Items
290+
291+
| Priority | Task | Effort |
292+
|----------|------|--------|
293+
| 1 | Fix `detectMetadata()` bug | 20 lines |
294+
| 2 | Add `detectPrimaryFramework()` helper | 15 lines |
295+
| 3 | Add library categorization to GenericAnalyzer | 30 lines |
296+
| 4 | Add workspace scanning utility | 20 lines |
297+
| 5 | Implement ReactAnalyzer (my way, with tests) | ~300 lines |
298+
| 6 | Implement NextJsAnalyzer (my way, with tests) | ~300 lines |
299+
300+
**Total: ~685 lines of MY code, MY methodology, MY tests.**
301+
302+
vs
303+
304+
**Fork: 1,309 lines of workarounds, smoke tests only, missing bug fixes.**
305+
306+
---
307+
308+
## Conclusion
309+
310+
The fork contributor saw a problem and built something that works. But they didn't understand (or chose not to engage with) my architecture. They bolted on a parallel system instead of fixing the root cause.
311+
312+
I appreciate them identifying the bug. I will fix it properly. I will add React/Next.js support MY way.
313+
314+
**Attribution**: Thanks to @aolin480 for identifying the `detectMetadata()` limitation and demonstrating React/Next.js pattern detection concepts.
315+
316+
**Code**: I'll write my own implementation.

‎package-lock.json‎

Lines changed: 2 additions & 2 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

0 commit comments

Comments
 (0)