fix: use CORE_SYMBOL_KINDS in flow matching#382
Conversation
flow's findMatchingNodes was limited to FUNCTION_KINDS, so `codegraph flow` couldn't trace from interfaces, types, or struct declarations. Pass CORE_SYMBOL_KINDS via opts.kinds override so all 10 core symbol kinds are searchable. Also support opts.kinds array in findMatchingNodes as a general-purpose override alongside the existing opts.kind scalar. Impact: 2 functions changed, 21 affected
|
Claude finished @carlos-alm's task —— View job PR Review CompleteReview Tasks:
Root Cause AnalysisProblem Being Solved: The Why This Approach: The fix expands the search scope in
Risk Assessment: LOW RISK - This is a focused change that only affects the flow command's node matching behavior. Configuration/Test Integrity Assessment✅ No configuration changes - No linting rules, build configs, or test thresholds modified Code Quality ReviewChanges Analysis:src/flow.js:101-102 ✅ GOOD const flowOpts = { ...opts, kinds: CORE_SYMBOL_KINDS };
let matchNode = findMatchingNodes(db, name, flowOpts)[0] ?? null;
src/flow.js:107 ✅ GOOD matchNode = findMatchingNodes(db, `${prefix}${name}`, flowOpts)[0] ?? null;
src/queries.js:167 ✅ EXCELLENT const kinds = opts.kind ? [opts.kind] : opts.kinds || FUNCTION_KINDS;
Technical Correctness✅ Import properly added: Potential Edge Cases Considered✅ CLI parameter conflicts: The Test Coverage Assessment
Recommendation: The manual test plan should be sufficient, but consider adding a regression test in the future. Final AssessmentExcellent work on this fix! The implementation is clean, the approach is sound, and the change directly addresses the stated problem without introducing risk. |
Greptile SummaryThis PR fixes
Confidence Score: 5/5
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["flowData(name, opts)"] --> B["flowOpts = { ...opts, kinds: CORE_SYMBOL_KINDS }"]
B --> C["Phase 1: findMatchingNodes(db, name, flowOpts)"]
C --> D{matchNode found?}
D -- No --> E["Phase 2: for each prefix in FRAMEWORK_ENTRY_PREFIXES\nfindMatchingNodes(db, prefix+name, flowOpts)"]
E --> F{matchNode found?}
F -- No --> G["Return empty result"]
D -- Yes --> H["BFS forward through callees"]
F -- Yes --> H
subgraph findMatchingNodes["findMatchingNodes(db, name, opts)"]
I{opts.kind set?} -- Yes --> J["kinds = [opts.kind]"]
I -- No --> K{opts.kinds?.length?}
K -- Yes --> L["kinds = opts.kinds\n(e.g. CORE_SYMBOL_KINDS)"]
K -- No --> M["kinds = FUNCTION_KINDS\n(function, method, class)"]
end
C --> I
E --> I
Last reviewed commit: 5e12131 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
src/queries.js
Outdated
| const kinds = opts.kind ? [opts.kind] : opts.kinds || FUNCTION_KINDS; | ||
| const placeholders = kinds.map(() => '?').join(', '); |
There was a problem hiding this comment.
Empty opts.kinds array would produce invalid SQL
If a caller passes opts.kinds = [] (an empty array), [] is truthy in JavaScript, so the fallback to FUNCTION_KINDS is skipped. The resulting placeholders string will be empty, producing invalid SQL:
WHERE n.name LIKE ? AND n.kind IN ()This will throw a SQLite syntax error at runtime. Consider adding a guard:
| const kinds = opts.kind ? [opts.kind] : opts.kinds || FUNCTION_KINDS; | |
| const placeholders = kinds.map(() => '?').join(', '); | |
| const kinds = opts.kind ? [opts.kind] : (opts.kinds?.length ? opts.kinds : FUNCTION_KINDS); |
This is safe for all current callers (which always pass CORE_SYMBOL_KINDS, a 10-element array), but protects the API surface if findMatchingNodes is called with an empty kinds override in the future.
There was a problem hiding this comment.
Fixed in 06fb91b. Changed to opts.kinds?.length ? opts.kinds : FUNCTION_KINDS so an empty array now correctly falls back to FUNCTION_KINDS instead of producing an invalid WHERE n.kind IN () clause.
Impact: 1 functions changed, 5 affected
Impact: 1 functions changed, 5 affected
# Conflicts: # src/queries.js Impact: 73 functions changed, 74 affected
|
Claude finished @carlos-alm's task —— View job PR Review CompleteReview Tasks:
Root Cause AnalysisProblem Being Solved: The Why This Approach: The fix expands the search scope in
Risk Assessment: LOW RISK - This is a focused change that only affects the flow command's node matching behavior. Configuration/Test Integrity Assessment✅ No configuration changes - No linting rules, build configs, or test thresholds modified Code Quality ReviewChanges Analysis:src/flow.js:101-102 ✅ GOOD const flowOpts = { ...opts, kinds: CORE_SYMBOL_KINDS };
let matchNode = findMatchingNodes(db, name, flowOpts)[0] ?? null;
src/flow.js:107 ✅ GOOD matchNode = findMatchingNodes(db, `${prefix}${name}`, flowOpts)[0] ?? null;
src/queries.js:167 ✅ EXCELLENT const kinds = opts.kind ? [opts.kind] : opts.kinds?.length ? opts.kinds : FUNCTION_KINDS;
Technical Correctness✅ Import properly added: Potential Edge Cases Considered✅ CLI parameter conflicts: The Test Coverage Assessment
Recommendation: The manual test plan should be sufficient, but consider adding a regression test in the future. Final AssessmentExcellent work on this fix! The implementation is clean, the approach is sound, and the change directly addresses the stated problem without introducing risk. |
|
Claude finished @carlos-alm's task —— View job PR Review in Progress
|
Resolve conflict in src/flow.js: combine CORE_SYMBOL_KINDS default (from this PR) with opts.kind passthrough (from main/PR #378). Impact: 42 functions changed, 65 affected
carlos-alm
force-pushed
the
fix/flow-kind-regression
branch
from
d249d40 to
0ff85a3
Compare
|
Claude finished @carlos-alm's task —— View job PR Review CompleteReview Tasks:
Root Cause AnalysisProblem Being Solved: The Why This Approach: The fix expands the search scope in
Risk Assessment: LOW RISK - This is a focused change that only affects the flow command's node matching behavior. Configuration/Test Integrity Assessment✅ No configuration changes - No linting rules, build configs, or test thresholds modified Code Quality ReviewChanges Analysis:src/flow.js:105 ✅ GOOD const flowOpts = { ...opts, kinds: opts.kind ? [opts.kind] : CORE_SYMBOL_KINDS };
src/flow.js:111 ✅ GOOD matchNode = findMatchingNodes(db, `${prefix}${name}`, flowOpts)[0] ?? null;
src/queries.js:167 ✅ EXCELLENT const kinds = opts.kind ? [opts.kind] : opts.kinds?.length ? opts.kinds : FUNCTION_KINDS;
Technical Correctness✅ Import properly added: Potential Edge Cases Considered✅ CLI parameter conflicts: The Test Coverage Assessment
Recommendation: The manual test plan should be sufficient, but consider adding a regression test in the future. Final AssessmentExcellent work on this fix! The implementation is clean, the approach is sound, and the change directly addresses the stated problem without introducing risk. |
|
Claude finished @carlos-alm's task —— View job PR Review in Progress
|
|
Claude finished @carlos-alm's task —— View job PR Review in Progress
|
1 participant
Summary
findMatchingNodeswas limited toFUNCTION_KINDS, socodegraph flowcouldn't trace from interfaces, types, or struct declarationsCORE_SYMBOL_KINDSviaopts.kindsoverride so all 10 core symbol kinds are searchableopts.kindsarray infindMatchingNodesas a general-purpose override alongside the existingopts.kindscalarSplit from #376.
Test plan
codegraph flow <interface-name>now finds and traces from interface declarationscodegraph flow <function-name>still works as before (FUNCTION_KINDS default unchanged)opts.kinds)