refactor: resolve three architecture issues after #456 and #458#459
refactor: resolve three architecture issues after #456 and #458#459carlos-alm merged 11 commits intomainfrom
Conversation
* refactor: reorganize src/ into domain/, features/, presentation/ layers Move source files into a domain-driven directory structure: - src/domain/ — core graph building, queries, analysis, search (embeddings) - src/features/ — composable feature modules (audit, check, complexity, etc.) - src/presentation/ — CLI formatting and output rendering - src/db/ — database layer (db.js → db/index.js) All 199 files updated with corrected import paths. 1858 tests pass, 0 cycles, all manifesto rules green. Impact: 22 functions changed, 5 affected * docs: update CLAUDE.md architecture table for new directory structure Update file paths in the architecture table to reflect the domain/, features/, presentation/, db/ reorganization. Addresses Greptile review. * refactor: move remaining flat src/ files into subdirectories Move 12 remaining files out of flat src/ so only cli.js and index.js remain as entry points: - config, logger, native, registry, update-check → infrastructure/ - constants, errors, kinds, paginate → shared/ - parser → domain/ - journal, change-journal → domain/graph/ Update all import paths across src/ and tests/. Update CLAUDE.md architecture table to match. Impact: 9 functions changed, 17 affected * style: fix import ordering after file moves
- Rename features/viewer.js → features/graph-enrichment.js to resolve naming collision with presentation/viewer.js (Issue 3) - Move 16 CLI formatter files from commands/ → presentation/ where they belong as presentation concerns, delete commands/ directory (Issue 1) - Split 866-LOC presentation/queries-cli.js into thematic modules: path.js, overview.js, inspect.js, impact.js, exports.js (Issue 2) - Fix broken import in cli/commands/plot.js (was referencing deleted src/viewer.js, now correctly points to features/graph-enrichment.js) - Fix wrong relative import in presentation/query.js - Update CLAUDE.md architecture table to reflect new structure Impact: 29 functions changed, 1 affected
# Conflicts: # src/cli/commands/batch.js
|
Claude finished @carlos-alm's task —— View job PR Review CompleteReviewing PR #459: refactor: resolve three architecture issues after #456 and #458 Root Cause AnalysisProblem Being Solved: This PR addresses legitimate architectural inconsistencies left over from recent src/ reorganization (#456, #458):
Why This Approach: The solution is systematic and minimal:
Risk Assessment: Backlog Compliance
Critical ConcernsNone. This is exemplary refactoring work: ✅ Phase 2 - Configuration: No risky changes (Biome rules, test thresholds, build config all unchanged) The split follows clear thematic boundaries:
Final Recommendation
|
- Rename features/viewer.js → features/graph-enrichment.js to resolve naming collision with presentation/viewer.js (Issue 3) - Move 16 CLI formatter files from commands/ → presentation/ where they belong as presentation concerns, delete commands/ directory (Issue 1) - Split 866-LOC presentation/queries-cli.js into thematic modules: path.js, overview.js, inspect.js, impact.js, exports.js (Issue 2) - Fix broken import in cli/commands/plot.js (was referencing deleted src/viewer.js, now correctly points to features/graph-enrichment.js) - Fix wrong relative import in presentation/query.js - Update CLAUDE.md architecture table to reflect new structure Impact: 54 functions changed, 19 affected
carlos-alm
force-pushed
the
refactor/arch-cleanup
branch
from
ab9a309 to
8f184fe
Compare
Greptile SummaryThis PR completes a three-part architectural cleanup after the
Confidence Score: 4/5
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
subgraph CLI ["src/cli/commands/"]
A[audit.js]
B[batch.js]
C[co-change.js]
D[plot.js]
E[check.js / triage.js / ...]
end
subgraph PRES ["src/presentation/"]
F[audit.js / batch.js / cochange.js / ...]
G[queries-cli.js\n barrel re-export]
subgraph QC ["queries-cli/"]
H[path.js]
I[overview.js]
J[inspect.js]
K[impact.js]
L[exports.js]
M[index.js]
end
end
subgraph FEAT ["src/features/"]
N[cochange.js]
O[graph-enrichment.js\n renamed from viewer.js]
P[communities.js]
end
subgraph DOM ["src/domain/"]
Q[queries.js]
end
A -->|static import| F
C -->|dynamic import| N
C -->|dynamic import| F
D -->|dynamic import| O
E -->|dynamic import| F
G --> M
M --> H & I & J & K & L
I -->|async import| P
H & I & J & K & L --> Q
O -->|imports| PRES
Last reviewed commit: 506c30f |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| /** | ||
| * Re-export all query CLI wrappers from queries-cli.js. | ||
| * This barrel file provides the standard src/commands/ import path. | ||
| */ |
There was a problem hiding this comment.
Stale comment and potential dead code
The comment on line 3 still references src/commands/ which no longer exists:
| /** | |
| * Re-export all query CLI wrappers from queries-cli.js. | |
| * This barrel file provides the standard src/commands/ import path. | |
| */ | |
| /** | |
| * Re-export all query CLI wrappers from queries-cli.js. | |
| * This barrel file provides a secondary import path for backward compat. | |
| */ |
Additionally, query.js is an exact duplicate of the barrel in queries-cli.js — both re-export the same 15 functions from ./queries-cli/index.js. I confirmed that nothing in the codebase imports presentation/query.js. If there are no external consumers (e.g. the programmatic API), this file could be removed entirely to avoid maintaining two identical barrels.
There was a problem hiding this comment.
Good catch — confirmed zero importers. Removed presentation/query.js entirely in 8108e1c rather than just updating the comment.
This file was a duplicate of queries-cli.js with zero importers. Removes the maintenance burden of keeping two identical barrels in sync. Addresses Greptile review feedback on #459. docs check acknowledged
This file was a duplicate of queries-cli.js with zero importers. Removes the maintenance burden of keeping two identical barrels in sync. Addresses Greptile review feedback on #459. docs check acknowledged
| const { communitySummaryForStats } = await import('../communities.js'); | ||
| data.communities = communitySummaryForStats(customDbPath, { noTests: opts.noTests }); |
There was a problem hiding this comment.
Wrong import path for communitySummaryForStats
communitySummaryForStats lives in src/features/communities.js (confirmed by the export at line 160 there and its integration test), but this import resolves to src/presentation/communities.js, which only exports communities. At runtime, communitySummaryForStats will be undefined; calling it immediately throws a TypeError, which the catch block silently swallows. As a result, the community summary section is never populated in codegraph stats output — the feature is silently broken.
The correct path from src/presentation/queries-cli/overview.js to the features layer is:
| const { communitySummaryForStats } = await import('../communities.js'); | |
| data.communities = communitySummaryForStats(customDbPath, { noTests: opts.noTests }); | |
| const { communitySummaryForStats } = await import('../../features/communities.js'); | |
| data.communities = communitySummaryForStats(customDbPath, { noTests: opts.noTests }); |
Note: the misleading comment /* graphology may not be available */ was originally written to guard against an optional peer dependency, but the real guard here is the wrong path. Fixing the path will let the import succeed when features/communities.js is present (it always is), and the catch can remain as a safety net for any genuine runtime failures.
There was a problem hiding this comment.
Fixed in the latest push — corrected the dynamic import path from ../communities.js to ../../features/communities.js so communitySummaryForStats resolves correctly.
…egraph into refactor/arch-cleanup
Impact: 1 functions changed, 3 affected
src/cli/commands/co-change.js
Outdated
| @@ -20,7 +20,7 @@ export const command = { | |||
| ], | |||
| async execute([file], opts, ctx) { | |||
| const { analyzeCoChanges, coChangeData, coChangeTopData } = await import('../../cochange.js'); | |||
There was a problem hiding this comment.
Broken dynamic import — resolves to non-existent file
'../../cochange.js' from src/cli/commands/co-change.js resolves to src/cochange.js, which does not exist in the repository. The actual functions analyzeCoChanges, coChangeData, and coChangeTopData are exported from src/features/cochange.js. This import will throw a ERR_MODULE_NOT_FOUND at runtime whenever the co-change command is invoked.
This is a pre-existing bug, but since this file is being touched in this PR to update the formatter import on the very next line, it's the right moment to fix the data-layer path too:
| const { analyzeCoChanges, coChangeData, coChangeTopData } = await import('../../cochange.js'); | |
| const { analyzeCoChanges, coChangeData, coChangeTopData } = await import('../../features/cochange.js'); |
There was a problem hiding this comment.
Fixed — corrected the dynamic import from \ to .
There was a problem hiding this comment.
Fixed in 506c30f — corrected the dynamic import path from ../../cochange.js to ../../features/cochange.js.
carlos-alm
force-pushed
the
refactor/arch-cleanup
branch
from
67b7735 to
43ff32e
Compare
# Conflicts: # src/cli/commands/batch.js Impact: 1 functions changed, 0 affected
Impact: 1 functions changed, 0 affected
1 participant
Summary
Resolves three structural issues left over after the src/ reorganization in #456 and #458:
commands/→presentation/: Moved 16 CLI formatter files fromsrc/commands/intosrc/presentation/where they belong as presentation concerns. Thesrc/commands/directory is deleted entirely. Updated all 15 dynamic imports insrc/cli/commands/.queries-cli.jssplit: Split the 866-LOCpresentation/queries-cli.jsinto 5 thematic modules underpresentation/queries-cli/:path.js,overview.js,inspect.js,impact.js,exports.js. A barrelqueries-cli.jsre-exports everything for backward compatibility — zero import changes needed in consumers.features/viewer.js→features/graph-enrichment.jsto disambiguate frompresentation/viewer.js(HTML renderer). Fixed a broken import incli/commands/plot.jsthat referenced the deletedsrc/viewer.js.presentation/query.js(../presentation/→./)Test plan
npm test— 1862 tests pass (104 files, 6 skipped)npx biome check src/ tests/— 0 lint issuesls src/commands/— directory does not existcli.jsandindex.jsatsrc/root