feat(graph): multi-language support — Python, JS, Go, Rust, Java, Ruby, C, C++#241
Merged
Conversation
…y, C, C++
Extends the codebase-graph from TypeScript-only to 9 languages:
Languages added:
- JavaScript / JSX / MJS / CJS (tree-sitter-javascript)
- Python (tree-sitter-python) — functions, classes, methods, imports
- Go (tree-sitter-go) — funcs, methods, structs, interfaces, imports
- Rust (tree-sitter-rust) — fn, struct, enum, trait, impl methods, use
- Java (tree-sitter-java) — classes, interfaces, enums, methods, imports
- Ruby (tree-sitter-ruby) — methods, classes, modules, require/require_relative
- C (tree-sitter-c) — functions, structs, #include
- C++ (tree-sitter-cpp) — classes, namespaces, templates, methods, #include
Implementation:
- src/graph/extract/shared.ts — shared utilities (parser singleton factory,
parse-with-chunks, make-node helpers) used by all new extractors
- Per-language extractors in src/graph/extract/{python,javascript,go,rust,java,ruby,c,cpp}.ts
- NodeLanguage union expanded in src/graph/types.ts
- extractFile() router in graph.ts dispatches by extension
- isSourceFile() accepts all 9 language extension sets
- Grammar packages added as optionalDependencies (same pattern as tree-sitter-typescript)
- esbuild externals updated for both CC and CLI bundles
- ensure-tree-sitter.mjs healer extended to rebuild all grammar packages on arm64
All extractors implement the same 3-pass structure (declarations → imports → calls)
as the existing TypeScript extractor. TypeScript extractor is unchanged.
…ner missing Detects SSH_CLIENT, SSH_TTY, VSCODE_INJECTION, CODESPACES env vars and switches to --serve automatically so users on remote VMs get a clickable localhost URL instead of a file path that xdg-open can't reach. Also falls back to serve when the opener binary isn't found locally (e.g. Linux without xdg-open installed) instead of silently doing nothing.
|
Caution Review failedPull request was closed or merged during review No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (12)
🚧 Files skipped from review as they are similar to previous changes (9)
📝 WalkthroughWalkthroughUpgrades the codebase-graph to Phase 1.5: adds multi-language extractors (JS/TS, Go, Rust, Java, Ruby, C, C++), shared parsing utilities, build/package grammar wiring and pins, CLI source detection updates, dashboard remote-session auto-serve/fallback logic, and tests/coverage thresholds. ChangesPhase 1.5 Multi-Language Graph Extraction and CLI
Sequence Diagram(s)No additional sequence diagrams generated for user-visible docs beyond the packaging/runtime grammar validation flow included in the hidden artifact. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
scripts/ensure-tree-sitter.mjs (1)
33-45:⚠️ Potential issue | 🟠 Major | ⚡ Quick winExpand
bindingsLoad()to validate all newly supported grammars.You now rebuild 10 packages, but health-check success still depends on only TS + Python loading. A broken Go/Rust/Java/Ruby/C/C++/JS grammar can slip through postinstall and fail later at extraction time.
Suggested patch
function bindingsLoad() { try { const Parser = require('tree-sitter'); - const TS = require('tree-sitter-typescript').typescript; - const parser = new Parser(); - parser.setLanguage(TS); - parser.parse('const x = 1;'); - // B6: also verify the Python grammar loads on this platform / Node ABI. - const Py = require('tree-sitter-python'); - const pyParser = new Parser(); - pyParser.setLanguage(Py); - pyParser.parse('x = 1\n'); + const langs = [ + require('tree-sitter-typescript').typescript, + require('tree-sitter-javascript'), + require('tree-sitter-python'), + require('tree-sitter-go'), + require('tree-sitter-rust'), + require('tree-sitter-java'), + require('tree-sitter-ruby'), + require('tree-sitter-c'), + require('tree-sitter-cpp'), + ]; + for (const lang of langs) { + const p = new Parser(); + p.setLanguage(lang); + p.parse('x'); + } return true; } catch { return false; } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/ensure-tree-sitter.mjs` around lines 33 - 45, Expand the bindingsLoad() health check to require and instantiate all newly supported grammars (at minimum: tree-sitter-go, tree-sitter-rust, tree-sitter-java, tree-sitter-ruby, tree-sitter-c, tree-sitter-cpp, and tree-sitter-javascript) in addition to the existing typescript and python checks; for each grammar (use the existing Parser instantiation pattern in bindingsLoad), call parser.setLanguage(<Grammar>) and parser.parse a minimal valid snippet for that language to ensure it loads successfully, and return false or throw/log a clear error if any require/setLanguage/parse step fails so the postinstall health-check fails fast on broken grammars.src/commands/graph.ts (1)
51-52:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winCLI help text is stale for supported languages.
The
buildhelp still says(TS/JS/Python)even though discovery/extraction now supports additional languages. Updating this line prevents user confusion and support churn.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/commands/graph.ts` around lines 51 - 52, The CLI help text for the "build" command is stale — update the descriptive string that currently contains "(TS/JS/Python)" to a generic, accurate phrase like "(supported languages)" or "multiple languages (see docs)" so it no longer lists an incomplete language set; locate and change the help/description string used when registering the build subcommand (the CLI command named "build" in the graph command module) to the new wording.
🧹 Nitpick comments (3)
src/graph/extract/shared.ts (1)
116-130: ⚡ Quick winMake
pushNodemembership checks O(1) to avoid quadratic growth.
result.nodes.some(...)runs on every insert; in larger files this becomes unnecessarily expensive in the hottest extraction path.Suggested patch
+const _seenNodeIds = new WeakMap<FileExtraction, Set<string>>(); + export function pushNode( result: FileExtraction, declByName: Map<string, GraphNode>, node: GraphNode, lookupKey?: string, ): void { - if (result.nodes.some((n) => n.id === node.id)) { + let seen = _seenNodeIds.get(result); + if (seen === undefined) { + seen = new Set(result.nodes.map((n) => n.id)); + _seenNodeIds.set(result, seen); + } + + if (seen.has(node.id)) { if (!declByName.has(lookupKey ?? node.label)) { declByName.set(lookupKey ?? node.label, node); } return; } result.nodes.push(node); + seen.add(node.id); declByName.set(lookupKey ?? node.label, node); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/graph/extract/shared.ts` around lines 116 - 130, The current pushNode uses result.nodes.some(...) which is O(n); change membership checks to O(1) by tracking seen node ids: add a Set<string> (e.g., result.nodeIds) to FileExtraction and use result.nodeIds.has(node.id) instead of result.nodes.some(...); when adding a node push it into result.nodes, add node.id to result.nodeIds, and still update declByName.set(lookupKey ?? node.label, node) as before. Ensure FileExtraction type is updated to include nodeIds: Set<string> and that existing callers construct it (initialize as new Set()) so the fast membership test is available.src/graph/extract/go.ts (1)
168-188: 💤 Low value
var_specincorrectly uses"const"node kind.Both
const_specandvar_specdeclarations produce nodes withkind: "const". Variables should use a distinct kind (e.g.,"variable") to accurately represent the semantics.Proposed fix
if ( spec.type === "const_spec" || spec.type === "var_spec" ) { const nameNode = spec.childForFieldName("name"); const name = nameNode?.text ?? null; if (name !== null && name.length > 0) { - pushNode(result, declByName, makeNode(relativePath, name, "const", spec, name[0] === name[0].toUpperCase(), LANG)); + const kind = spec.type === "const_spec" ? "const" : "variable"; + pushNode(result, declByName, makeNode(relativePath, name, kind, spec, name[0] === name[0].toUpperCase(), LANG)); } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/graph/extract/go.ts` around lines 168 - 188, The collectGoVarConst function is creating nodes for both const_spec and var_spec with kind "const"; update the logic so that when spec.type === "var_spec" you call makeNode with kind "variable" (or another suitable variable-kind) instead of "const" before pushNode; locate the use of makeNode inside collectGoVarConst and branch the kind based on spec.type (const_spec => "const", var_spec => "variable") so pushNode receives the correct kind for variables.src/commands/dashboard.ts (1)
151-158: 💤 Low valueComment claims behavior the code doesn't implement.
The comment says "allow it when --serve is implicit", but the parser is pure and has no access to
isRemoteSession(). The check on lines 156-158 rejects--portwithout--serveunconditionally—remote or not.Either pass environment info to the parser to enable conditional acceptance, or update the comment to reflect the actual behavior (users must always pair
--portwith explicit--serve).Suggested comment clarification
- // --port is meaningful only with --serve (or when auto-serve kicks - // in on remote sessions). Silently accepting --port without --serve - // on a local machine would be a no-op that confuses users, so we - // keep the explicit guard — but allow it when --serve is implicit. - // The runtime auto-serve path will pick up `port` from DashboardArgs. + // --port is meaningful only with --serve. We require explicit --serve + // even on remote sessions where auto-serve would kick in, because + // the parser is pure and can't detect environment. Users who want a + // custom port should pass both: `--serve --port <n>`. if (port !== undefined && !serve) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/commands/dashboard.ts` around lines 151 - 158, The comment above the guard for port/serve incorrectly claims "--port is allowed when --serve is implicit (remote sessions)", but the code (the if check on port and serve that returns { error: "--port requires --serve" }) unconditionally rejects --port without explicit --serve; update the comment to accurately state that --port must always be paired with --serve (or, if you prefer the alternate fix, pass session/environment info into the parser and modify the validation to consult isRemoteSession() before rejecting). Ensure you update the explanatory text near the port/serve guard and leave the if (port !== undefined && !serve) { return { error: "--port requires --serve" }; } logic consistent with the chosen approach.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/dashboard/open.ts`:
- Around line 180-181: The isRemoteSession function currently uses nullish
coalescing (??) which ignores empty-string env vars; change the chain in
isRemoteSession to use logical OR (||) so truthy string values (including
non-empty strings) are evaluated correctly — replace the (env.SSH_CLIENT ??
env.SSH_TTY ?? env.VSCODE_INJECTION ?? env.CODESPACES) expression with
(env.SSH_CLIENT || env.SSH_TTY || env.VSCODE_INJECTION || env.CODESPACES) and
keep the surrounding boolean coercion as-is.
In `@src/graph/extract/cpp.ts`:
- Around line 104-112: collectCppDecls currently drops namespace context so
declarations inside a namespace are stored with unqualified keys while
collectCppCalls resolves qualified_identifier as `${scope.text}::${name.text}`;
thread an enclosingNamespace parameter through collectCppDecls (add to its
signature and all recursive calls, e.g., when recursing from the
namespace_definition branch pass the namespace name) and include it when
constructing declByName keys (compose namespace and class with the name, e.g.,
`${enclosingNamespace}::${enclosingClass ? enclosingClass + '::' : ''}${name}`)
so the keys created by makeNode/pushNode in collectCppDecls match the
`${scope}::${name}` format used by collectCppCalls.
In `@src/graph/types.ts`:
- Around line 10-11: The file-level phase wording is inconsistent: the top
header says cross-file call resolution is "Phase 2" while the EdgeRelation and
FileExtraction.raw_calls docs describe a "Phase 1.5" behavior; update the
comments so they match (choose one canonical phase label and apply it
everywhere). Edit the top-of-file header and the doc comments for EdgeRelation
and FileExtraction.raw_calls to use the same phase identifier and a short
consistent phrase (e.g., "Phase 1.5 — cross-file call linking" or "Phase 2 —
cross-file call resolution") so the semantics are identical across the file.
---
Outside diff comments:
In `@scripts/ensure-tree-sitter.mjs`:
- Around line 33-45: Expand the bindingsLoad() health check to require and
instantiate all newly supported grammars (at minimum: tree-sitter-go,
tree-sitter-rust, tree-sitter-java, tree-sitter-ruby, tree-sitter-c,
tree-sitter-cpp, and tree-sitter-javascript) in addition to the existing
typescript and python checks; for each grammar (use the existing Parser
instantiation pattern in bindingsLoad), call parser.setLanguage(<Grammar>) and
parser.parse a minimal valid snippet for that language to ensure it loads
successfully, and return false or throw/log a clear error if any
require/setLanguage/parse step fails so the postinstall health-check fails fast
on broken grammars.
In `@src/commands/graph.ts`:
- Around line 51-52: The CLI help text for the "build" command is stale — update
the descriptive string that currently contains "(TS/JS/Python)" to a generic,
accurate phrase like "(supported languages)" or "multiple languages (see docs)"
so it no longer lists an incomplete language set; locate and change the
help/description string used when registering the build subcommand (the CLI
command named "build" in the graph command module) to the new wording.
---
Nitpick comments:
In `@src/commands/dashboard.ts`:
- Around line 151-158: The comment above the guard for port/serve incorrectly
claims "--port is allowed when --serve is implicit (remote sessions)", but the
code (the if check on port and serve that returns { error: "--port requires
--serve" }) unconditionally rejects --port without explicit --serve; update the
comment to accurately state that --port must always be paired with --serve (or,
if you prefer the alternate fix, pass session/environment info into the parser
and modify the validation to consult isRemoteSession() before rejecting). Ensure
you update the explanatory text near the port/serve guard and leave the if (port
!== undefined && !serve) { return { error: "--port requires --serve" }; } logic
consistent with the chosen approach.
In `@src/graph/extract/go.ts`:
- Around line 168-188: The collectGoVarConst function is creating nodes for both
const_spec and var_spec with kind "const"; update the logic so that when
spec.type === "var_spec" you call makeNode with kind "variable" (or another
suitable variable-kind) instead of "const" before pushNode; locate the use of
makeNode inside collectGoVarConst and branch the kind based on spec.type
(const_spec => "const", var_spec => "variable") so pushNode receives the correct
kind for variables.
In `@src/graph/extract/shared.ts`:
- Around line 116-130: The current pushNode uses result.nodes.some(...) which is
O(n); change membership checks to O(1) by tracking seen node ids: add a
Set<string> (e.g., result.nodeIds) to FileExtraction and use
result.nodeIds.has(node.id) instead of result.nodes.some(...); when adding a
node push it into result.nodes, add node.id to result.nodeIds, and still update
declByName.set(lookupKey ?? node.label, node) as before. Ensure FileExtraction
type is updated to include nodeIds: Set<string> and that existing callers
construct it (initialize as new Set()) so the fast membership test is available.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 8f19dab1-cd87-4b5b-8688-e28018cf7a88
📒 Files selected for processing (17)
esbuild.config.mjspackage.jsonscripts/ensure-tree-sitter.mjssrc/commands/dashboard.tssrc/commands/graph.tssrc/dashboard/open.tssrc/graph/extract/c.tssrc/graph/extract/cpp.tssrc/graph/extract/go.tssrc/graph/extract/grammar-shims.d.tssrc/graph/extract/index.tssrc/graph/extract/java.tssrc/graph/extract/javascript.tssrc/graph/extract/ruby.tssrc/graph/extract/rust.tssrc/graph/extract/shared.tssrc/graph/types.ts
- fix(open.ts): use || instead of ?? in isRemoteSession so empty-string env vars are treated as falsy - fix(go.ts): var_spec nodes now get kind 'variable' instead of 'const' - fix(cpp.ts): thread enclosingNamespace through collectCppDecls so qualified calls (Ns::fn) resolve correctly - fix(ensure-tree-sitter): bindingsLoad now validates all 9 grammars, not just TS+Python - fix(types.ts): add 'variable' to NodeKind; align phase wording to 'Phase 1.5' - fix(graph.ts): update build help text to list all supported languages - fix(dashboard.ts): clarify --port/--serve comment to match actual behavior
Collaborator
Author
|
All actionable findings addressed in commit 6fe7d1b. Fixed:
Also fixed (trivial):
Skipped:
|
…tible versions tree-sitter-c 0.23.3+, tree-sitter-python 0.23.5+, tree-sitter-rust 0.23.2+ all require tree-sitter@^0.22.1 as peerDep, conflicting with the project's tree-sitter@0.21.x. Pin to last ABI-compatible releases: tree-sitter-c: 0.23.2 tree-sitter-python: 0.23.4 tree-sitter-rust: 0.23.1 Add overrides block to force same versions when pulled transitively (tree-sitter-cpp depends on tree-sitter-c@^0.23.1). Regenerate package-lock.json from scratch.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/graph/extract/cpp.ts (1)
74-75:⚠️ Potential issue | 🟠 Major | ⚡ Quick winCarry the full enclosing scope into call resolution.
Line 115 replaces the outer namespace instead of composing it, so
namespace a { namespace b { void f() {} } }is keyed asb::finstead ofa::b::f. Then Lines 179-193 and 214-237 still resolve many callees/callers as baref/Cls::m, so same-namespace calls and member calls inside namespaced classes stop producingcallsedges once declarations are stored asa::....Suggested direction
- collectCppDecls(body, relativePath, result, declByName, moduleNode, enclosingClass, name ?? enclosingNamespace); + const nextNamespace = + name !== null && name.length > 0 + ? enclosingNamespace !== null + ? `${enclosingNamespace}::${name}` + : name + : enclosingNamespace; + collectCppDecls( + body, + relativePath, + result, + declByName, + moduleNode, + enclosingClass, + nextNamespace, + ); +// Reuse the same namespace/class qualification when resolving: +// - identifier calls +// - this/member calls +// - enclosing caller function keys +// Try scoped candidates first (e.g. ns::Cls::name, ns::name) before falling back to `name`.Also applies to: 113-115, 179-193, 214-237
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/graph/extract/cpp.ts` around lines 74 - 75, The code builds and later resolves symbol keys incorrectly by replacing rather than composing the full enclosing scope; update the logic that computes nsPrefix/key (where nsPrefix is derived from enclosingNamespace and key from enclosingClass/enclosingNamespace) to always compose the full namespace/class stack (e.g., join nested namespaces with :: and prepend class scope) so declarations are stored as fully-qualified names (a::b::f), and mirror this same fully-qualified construction in the callee/caller resolution paths referenced around the blocks handling call resolution (the blocks that currently yield bare f or Cls::m). Ensure the functions/methods that generate keys and the resolution code use the same fully-qualified-building utility so lookups match stored declaration keys.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@src/graph/extract/cpp.ts`:
- Around line 74-75: The code builds and later resolves symbol keys incorrectly
by replacing rather than composing the full enclosing scope; update the logic
that computes nsPrefix/key (where nsPrefix is derived from enclosingNamespace
and key from enclosingClass/enclosingNamespace) to always compose the full
namespace/class stack (e.g., join nested namespaces with :: and prepend class
scope) so declarations are stored as fully-qualified names (a::b::f), and mirror
this same fully-qualified construction in the callee/caller resolution paths
referenced around the blocks handling call resolution (the blocks that currently
yield bare f or Cls::m). Ensure the functions/methods that generate keys and the
resolution code use the same fully-qualified-building utility so lookups match
stored declaration keys.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: f8315f11-e0e6-455e-a716-cbf5bd144803
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json,!**/package-lock.json
📒 Files selected for processing (8)
package.jsonscripts/ensure-tree-sitter.mjssrc/commands/dashboard.tssrc/commands/graph.tssrc/dashboard/open.tssrc/graph/extract/cpp.tssrc/graph/extract/go.tssrc/graph/types.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- src/dashboard/open.ts
- src/graph/types.ts
- src/graph/extract/go.ts
runDashboardCommand now accepts isRemote?: boolean in RunDashboardOptions so tests bypass isRemoteSession() process.env detection. Without this, tests time out in environments where SSH_CLIENT/VSCODE_INJECTION is set (VS Code Remote, SSH sessions) because autoServe kicks in unexpectedly. Also fix the 'opener attempted=false' test: inject fake server + fire SIGINT to exit the serve loop, and correct the expected message from the stale 'no opener for this platform' to the actual serve-loop output. Consolidate makeFakeServer/makeSignalSink to outer describe scope.
Contributor
Coverage ReportScope: files changed in this PR. Enforced threshold: 90% per metric (per file via
File Coverage — 14 files changed
Generated for commit 6fd8b7d. |
kaghni
approved these changes
Jun 8, 2026
Collaborator
|
increase the test coverage |
65 new tests covering node extraction (functions, classes, methods, enums, traits, imports, module nodes) and intra-file call edges for all 6 newly-added language extractors. Raises src/graph/extract coverage from ~35% to ~77% lines / ~82% statements. Annotates Go exported=true behavior and Ruby label convention (ClassName#method key as label) so tests document intent.
…rage thresholds - tests/shared/graph/javascript-extractor.test.ts: 10 tests for the native extractJavaScript() (tree-sitter-javascript), covering function/class/method/ arrow-fn/generator, ES import, require(), intra-file calls, module node. - tests/shared/graph/shared-utils.test.ts: 16 tests for shared.ts helpers — pushNode duplicate branch, collectParseErrors error/missing paths, firstOfType, nodeId, locationStr multi-line, makeModuleNode, textOfField edge cases. - vitest.config.ts: add per-file thresholds for the 8 new extractors from PR #241 (shared, javascript, go, rust, java, ruby, c, cpp) at 70–90% per metric, calibrated to measured coverage with a 5–10 pt margin for branch variance.
Go: +pointer receiver test, +method→free-function call (method_declaration branch) Rust: +use scoped path, +impl method→free-function call (endsWith ::name search) Ruby: +method inside conditional block (else recursion branch) JS: +class method→method call, +arrow-fn→function call (className/variable_declarator branches) Python: +decorated method, +aliased import, +aliased from-import, +raw_calls path Shared: +findEnclosingDecl direct test C++: +field_expression call (no crash on this->) extract/ branches: 67% → 73%
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (8)
tests/shared/graph/shared-utils.test.ts (3)
88-88: 💤 Low valueConsider using typed arrays instead of
any[].The error arrays are typed as
any[]. For better type safety, consider importingParseErrorfromsrc/graph/types.tsand typing these arrays asParseError[].♻️ Example
import type { FileExtraction } from "../../../src/graph/types.js"; +import type { ParseError } from "../../../src/graph/types.js"; // ... describe("collectParseErrors", () => { it("records nothing for a clean node tree", () => { - const errors: any[] = []; + const errors: ParseError[] = []; collectParseErrors(stubNode(), "f.ts", errors);Also applies to: 94-94, 102-102, 109-109
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/shared/graph/shared-utils.test.ts` at line 88, The tests declare error arrays as any[]; import the ParseError type from src/graph/types.ts and change those declarations to use ParseError[] for stronger typing (e.g., update the const errors: any[] = []; occurrences and the other similar declarations on the same file referenced at lines 94, 102, 109 to const errors: ParseError[] = [];), making sure the import is added at the top of tests/shared/graph/shared-utils.test.ts and all relevant error-array variables are retyped to ParseError[].
14-14: ⚡ Quick winRemove unused import.
makeNodeis imported but never used in the tests.♻️ Proposed fix
import { pushNode, collectParseErrors, firstOfType, - makeNode, makeModuleNode, nodeId,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/shared/graph/shared-utils.test.ts` at line 14, The test file imports makeNode but doesn't use it; remove the unused import to clean up the module. Edit tests/shared/graph/shared-utils.test.ts and delete makeNode from the import list (where makeNode appears alongside other imports) so only the actually used symbols remain imported (e.g., leave makeEdge or others intact).
164-196: ⚡ Quick winMove
findEnclosingDeclto top-level imports.
findEnclosingDeclis dynamically imported within each test (lines 167 and 191), but it's exported from the same module as the other helper functions. This creates unnecessary duplication and adds async overhead.♻️ Proposed fix
import { pushNode, collectParseErrors, firstOfType, makeNode, makeModuleNode, nodeId, locationStr, textOfField, + findEnclosingDecl, type TSNode, } from "../../../src/graph/extract/shared.js"; // ... describe("findEnclosingDecl", () => { - // Import findEnclosingDecl it("returns the enclosing declaration when found", async () => { - const { findEnclosingDecl } = await import("../../../src/graph/extract/shared.js"); const map = new Map<string, any>(); // ... }); - it("returns null when no enclosing decl matches", async () => { - const { findEnclosingDecl } = await import("../../../src/graph/extract/shared.js"); + it("returns null when no enclosing decl matches", () => { const map = new Map<string, any>();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/shared/graph/shared-utils.test.ts` around lines 164 - 196, Replace the per-test dynamic imports of findEnclosingDecl by importing it once at the top of the test file alongside the other helpers: add a top-level import for findEnclosingDecl from "../../../src/graph/extract/shared.js" and remove the await import(...) calls inside the two it(...) blocks; update the test callbacks to be synchronous if they no longer need async. Keep references to the same symbols (findEnclosingDecl, stubNode, makeGraphNode) and ensure the tests still use the existing map/node setup and assertions unchanged.tests/shared/graph/rust.test.ts (2)
94-98: ⚡ Quick winPrefer asserting on specific import targets.
The assertion uses
.startsWith("external:")which is a generic substring check. As per coding guidelines, tests should assert on specific values. Consider checking for the exact expected import targets.♻️ Proposed fix to use exact values
- expect(ex.edges.some(e => e.relation === "imports" && e.target.startsWith("external:"))).toBe(true); + expect(ex.edges.some(e => e.relation === "imports" && (e.target === "external:std::io::Read" || e.target === "external:std::io::Write"))).toBe(true);Or better yet, check each import individually:
- expect(ex.edges.some(e => e.relation === "imports" && e.target.startsWith("external:"))).toBe(true); + const imp = ex.edges.find(e => e.relation === "imports" && e.target === "external:std::io"); + expect(imp).toBeDefined();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/shared/graph/rust.test.ts` around lines 94 - 98, The test currently asserts any import edge whose target merely startsWith("external:"); update it to assert specific import targets produced by extractRust — check ex.edges for edges where e.relation === "imports" and e.target equals the exact expected strings (e.g., "external:std::io::Read" and "external:std::io::Write") or assert both exact targets exist individually; locate the assertion in tests/shared/graph/rust.test.ts near the it("extracts use with scoped path...") case and replace the generic startsWith check with exact equality checks for the expected import targets.Source: Coding guidelines
76-79: ⚡ Quick winPrefer asserting on the exact import target.
The assertion uses
.includes("std")which is a generic substring check. As per coding guidelines, tests should assert on specific values. Consider asserting the exact expected target value.♻️ Proposed fix to use exact value
- expect(ex.edges.some(e => e.relation === "imports" && e.target.includes("std"))).toBe(true); + const imp = ex.edges.find(e => e.relation === "imports" && e.target === "external:std::io::Read"); + expect(imp).toBeDefined();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/shared/graph/rust.test.ts` around lines 76 - 79, The test uses a substring check (.includes("std")) which is too broad; update the assertion in tests/shared/graph/rust.test.ts to assert the exact import target from extractRust instead: find the edge with relation "imports" and assert its target equals the precise expected string (e.g. "std::io::Read") rather than using .includes. This uses the existing extractRust call and the edges array to perform a strict equality check on the target.Source: Coding guidelines
tests/shared/graph/java.test.ts (1)
61-68: ⚡ Quick winPrefer asserting on the exact import target.
The assertion uses
.includes("java.util")which is a generic substring check. As per coding guidelines, tests should assert on specific values. Consider asserting the exact expected target value.♻️ Proposed fix to use exact value
- const imp = ex.edges.find(e => e.relation === "imports" && e.target.includes("java.util")); + const imp = ex.edges.find(e => e.relation === "imports" && e.target === "external:java.util.List"); expect(imp).toBeDefined();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/shared/graph/java.test.ts` around lines 61 - 68, The test uses a generic substring check (`includes("java.util")`) when locating the import edge; change it to assert the exact expected import target produced by extractJava. In the test `it("extracts import declaration as imports edge"... )` locate the edge via `ex.edges.find(e => e.relation === "imports" && e.target === "java.util.List")` (or assert `imp.target === "java.util.List"` after finding by relation), and then use `expect(imp).toBeDefined()` or `expect(imp.target).toBe("java.util.List")` to enforce the exact value; reference the `extractJava` call, `ex.edges`, `imp`, and the `"imports"` relation when making this change.Source: Coding guidelines
tests/shared/graph/python.test.ts (1)
49-56: ⚡ Quick winPrefer exact target values over substring checks.
Lines 53-54 use
.endsWith()and.includes()which are substring checks. As per coding guidelines, tests should assert on specific values. Consider asserting the exact expected target value instead.♻️ Proposed fix to use exact values
const extendsEdges = ex.edges.filter((e) => e.relation === "extends" && e.source === "a.py:C:class"); // dotted base abc.ABC → "ABC"; the metaclass=Meta keyword arg is NOT a base. - expect(extendsEdges.some((e) => e.target.endsWith(":ABC:class"))).toBe(true); - expect(extendsEdges.some((e) => e.target.includes("metaclass"))).toBe(false); + expect(extendsEdges).toHaveLength(1); + expect(extendsEdges[0].target).toBe("a.py:ABC:class"); expect(extendsEdges).toHaveLength(1);Note: Remove the duplicate length check after applying this fix.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/shared/graph/python.test.ts` around lines 49 - 56, The test is using substring checks on extendsEdges targets; update the assertions to check exact target values instead: use extractPython(...) and the existing extendsEdges array, find the specific edge objects and assert that one has target exactly "a.py:ABC:class" (or the exact full target string produced by extractPython for the ABC base) and assert none have target exactly "a.py:metaclass:..." (or simply assert there is no edge with target equal to the metaclass-related target); replace the .endsWith() and .includes() checks with these exact equality checks against the expected target strings on the extendsEdges entries and remove the now-redundant duplicate length check.Source: Coding guidelines
tests/shared/graph/ruby.test.ts (1)
35-41: ⚡ Quick winPrefer asserting on the exact target value.
The assertion uses
.toContain("Dog")which is a generic substring check. As per coding guidelines, tests should assert on specific values rather than generic substrings. Consider asserting the exact expected target value.♻️ Proposed fix to use exact value
- expect(ext!.target).toContain("Dog"); + expect(ext!.target).toBe("lib/poodle.rb:Dog:class");🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/shared/graph/ruby.test.ts` around lines 35 - 41, The test uses a substring check expect(ext!.target).toContain("Dog"); update it to assert the exact expected target value produced by extractRuby instead: inspect the actual edge.target produced by extractRuby in the failing case and replace the toContain assertion on ext!.target with a strict equality assertion (e.g., expect(ext!.target).toBe("<exact-target>")) so the test verifies the precise target value for the "extends" edge extracted by extractRuby when testing ext in this spec.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/claude-code/dashboard-command.test.ts`:
- Line 202: Update the test assertion to check the full expected message instead
of a substring: replace the current expect(stdout).toContain("no opener for this
platform") with an assertion that verifies the complete string "(no opener for
this platform; click the URL above or open it manually)" appears in stdout
(e.g., expect(stdout).toContain(fullMessage)). Locate the assertion by finding
the expect(...) call referencing stdout in dashboard-command.test.ts and ensure
the exact full message string is used.
In `@tests/shared/graph/shared-utils.test.ts`:
- Around line 101-106: Update the test to assert the exact error message and
validate location/source_file: call collectParseErrors(stubNode({ isMissing:
true, type: "identifier" }), "f.ts", errors) (or ensure stubNode returns type
"identifier"), then replace expect(errors[0].message).toContain("missing node")
with expect(errors[0].message).toBe("missing node: identifier"); also add
assertions for errors[0].location (compare to the expected location object/from
stubNode) and errors[0].source_file toBe("f.ts") so the test verifies message,
location, and source_file precisely while still using collectParseErrors and
stubNode.
- Around line 93-99: The test “records an isError node” uses a generic substring
check; update it to assert the exact message value and validate the location
field: call collectParseErrors(stubNode({ isError: true }), "f.ts", errors) as
before, then replace expect(errors[0].message).toContain("parse error") with an
exact equality check for the expected string ("parse error at L1") and add an
assertion on errors[0].location to match the stubNode's default coordinates
(e.g., start/end line/column values produced by stubNode); keep existing checks
for source_file. Use the existing symbols collectParseErrors, stubNode, and
errors to locate and change the assertions.
---
Nitpick comments:
In `@tests/shared/graph/java.test.ts`:
- Around line 61-68: The test uses a generic substring check
(`includes("java.util")`) when locating the import edge; change it to assert the
exact expected import target produced by extractJava. In the test `it("extracts
import declaration as imports edge"... )` locate the edge via `ex.edges.find(e
=> e.relation === "imports" && e.target === "java.util.List")` (or assert
`imp.target === "java.util.List"` after finding by relation), and then use
`expect(imp).toBeDefined()` or `expect(imp.target).toBe("java.util.List")` to
enforce the exact value; reference the `extractJava` call, `ex.edges`, `imp`,
and the `"imports"` relation when making this change.
In `@tests/shared/graph/python.test.ts`:
- Around line 49-56: The test is using substring checks on extendsEdges targets;
update the assertions to check exact target values instead: use
extractPython(...) and the existing extendsEdges array, find the specific edge
objects and assert that one has target exactly "a.py:ABC:class" (or the exact
full target string produced by extractPython for the ABC base) and assert none
have target exactly "a.py:metaclass:..." (or simply assert there is no edge with
target equal to the metaclass-related target); replace the .endsWith() and
.includes() checks with these exact equality checks against the expected target
strings on the extendsEdges entries and remove the now-redundant duplicate
length check.
In `@tests/shared/graph/ruby.test.ts`:
- Around line 35-41: The test uses a substring check
expect(ext!.target).toContain("Dog"); update it to assert the exact expected
target value produced by extractRuby instead: inspect the actual edge.target
produced by extractRuby in the failing case and replace the toContain assertion
on ext!.target with a strict equality assertion (e.g.,
expect(ext!.target).toBe("<exact-target>")) so the test verifies the precise
target value for the "extends" edge extracted by extractRuby when testing ext in
this spec.
In `@tests/shared/graph/rust.test.ts`:
- Around line 94-98: The test currently asserts any import edge whose target
merely startsWith("external:"); update it to assert specific import targets
produced by extractRust — check ex.edges for edges where e.relation ===
"imports" and e.target equals the exact expected strings (e.g.,
"external:std::io::Read" and "external:std::io::Write") or assert both exact
targets exist individually; locate the assertion in
tests/shared/graph/rust.test.ts near the it("extracts use with scoped path...")
case and replace the generic startsWith check with exact equality checks for the
expected import targets.
- Around line 76-79: The test uses a substring check (.includes("std")) which is
too broad; update the assertion in tests/shared/graph/rust.test.ts to assert the
exact import target from extractRust instead: find the edge with relation
"imports" and assert its target equals the precise expected string (e.g.
"std::io::Read") rather than using .includes. This uses the existing extractRust
call and the edges array to perform a strict equality check on the target.
In `@tests/shared/graph/shared-utils.test.ts`:
- Line 88: The tests declare error arrays as any[]; import the ParseError type
from src/graph/types.ts and change those declarations to use ParseError[] for
stronger typing (e.g., update the const errors: any[] = []; occurrences and the
other similar declarations on the same file referenced at lines 94, 102, 109 to
const errors: ParseError[] = [];), making sure the import is added at the top of
tests/shared/graph/shared-utils.test.ts and all relevant error-array variables
are retyped to ParseError[].
- Line 14: The test file imports makeNode but doesn't use it; remove the unused
import to clean up the module. Edit tests/shared/graph/shared-utils.test.ts and
delete makeNode from the import list (where makeNode appears alongside other
imports) so only the actually used symbols remain imported (e.g., leave makeEdge
or others intact).
- Around line 164-196: Replace the per-test dynamic imports of findEnclosingDecl
by importing it once at the top of the test file alongside the other helpers:
add a top-level import for findEnclosingDecl from
"../../../src/graph/extract/shared.js" and remove the await import(...) calls
inside the two it(...) blocks; update the test callbacks to be synchronous if
they no longer need async. Keep references to the same symbols
(findEnclosingDecl, stubNode, makeGraphNode) and ensure the tests still use the
existing map/node setup and assertions unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 96dd50d6-8cd1-4ebf-bca5-77c5fb61f123
📒 Files selected for processing (12)
src/commands/dashboard.tstests/claude-code/dashboard-command.test.tstests/shared/graph/c.test.tstests/shared/graph/cpp.test.tstests/shared/graph/go.test.tstests/shared/graph/java.test.tstests/shared/graph/javascript-extractor.test.tstests/shared/graph/python.test.tstests/shared/graph/ruby.test.tstests/shared/graph/rust.test.tstests/shared/graph/shared-utils.test.tsvitest.config.ts
Comment on lines
+93
to
+99
| it("records an isError node", () => { | ||
| const errors: any[] = []; | ||
| collectParseErrors(stubNode({ isError: true }), "f.ts", errors); | ||
| expect(errors).toHaveLength(1); | ||
| expect(errors[0].message).toContain("parse error"); | ||
| expect(errors[0].source_file).toBe("f.ts"); | ||
| }); |
There was a problem hiding this comment.
Assert on specific message values, not generic substrings.
The assertion expect(errors[0].message).toContain("parse error") uses a generic substring. As per coding guidelines, tests should prefer asserting on specific values. The actual message for an isError node with the default stub coordinates would be "parse error at L1".
Additionally, the test should validate the location field.
🔍 Proposed fix
it("records an isError node", () => {
const errors: any[] = [];
collectParseErrors(stubNode({ isError: true }), "f.ts", errors);
expect(errors).toHaveLength(1);
- expect(errors[0].message).toContain("parse error");
+ expect(errors[0].message).toBe("parse error at L1");
+ expect(errors[0].location).toBe("L1");
expect(errors[0].source_file).toBe("f.ts");
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| it("records an isError node", () => { | |
| const errors: any[] = []; | |
| collectParseErrors(stubNode({ isError: true }), "f.ts", errors); | |
| expect(errors).toHaveLength(1); | |
| expect(errors[0].message).toContain("parse error"); | |
| expect(errors[0].source_file).toBe("f.ts"); | |
| }); | |
| it("records an isError node", () => { | |
| const errors: any[] = []; | |
| collectParseErrors(stubNode({ isError: true }), "f.ts", errors); | |
| expect(errors).toHaveLength(1); | |
| expect(errors[0].message).toBe("parse error at L1"); | |
| expect(errors[0].location).toBe("L1"); | |
| expect(errors[0].source_file).toBe("f.ts"); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/shared/graph/shared-utils.test.ts` around lines 93 - 99, The test
“records an isError node” uses a generic substring check; update it to assert
the exact message value and validate the location field: call
collectParseErrors(stubNode({ isError: true }), "f.ts", errors) as before, then
replace expect(errors[0].message).toContain("parse error") with an exact
equality check for the expected string ("parse error at L1") and add an
assertion on errors[0].location to match the stubNode's default coordinates
(e.g., start/end line/column values produced by stubNode); keep existing checks
for source_file. Use the existing symbols collectParseErrors, stubNode, and
errors to locate and change the assertions.
Source: Coding guidelines
Comment on lines
+101
to
+106
| it("records an isMissing node with 'missing node' message", () => { | ||
| const errors: any[] = []; | ||
| collectParseErrors(stubNode({ isMissing: true }), "f.ts", errors); | ||
| expect(errors).toHaveLength(1); | ||
| expect(errors[0].message).toContain("missing node"); | ||
| }); |
There was a problem hiding this comment.
Assert on specific message values, not generic substrings.
The assertion expect(errors[0].message).toContain("missing node") uses a generic substring. As per coding guidelines, tests should prefer asserting on specific values. The actual message for an isMissing node with type "identifier" would be "missing node: identifier".
Additionally, the test should validate the location and source_file fields.
🔍 Proposed fix
it("records an isMissing node with 'missing node' message", () => {
const errors: any[] = [];
collectParseErrors(stubNode({ isMissing: true }), "f.ts", errors);
expect(errors).toHaveLength(1);
- expect(errors[0].message).toContain("missing node");
+ expect(errors[0].message).toBe("missing node: identifier");
+ expect(errors[0].location).toBe("L1");
+ expect(errors[0].source_file).toBe("f.ts");
});🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/shared/graph/shared-utils.test.ts` around lines 101 - 106, Update the
test to assert the exact error message and validate location/source_file: call
collectParseErrors(stubNode({ isMissing: true, type: "identifier" }), "f.ts",
errors) (or ensure stubNode returns type "identifier"), then replace
expect(errors[0].message).toContain("missing node") with
expect(errors[0].message).toBe("missing node: identifier"); also add assertions
for errors[0].location (compare to the expected location object/from stubNode)
and errors[0].source_file toBe("f.ts") so the test verifies message, location,
and source_file precisely while still using collectParseErrors and stubNode.
Source: Coding guidelines
…ed-utils/dashboard tests
c8 ignores added to unreachable defensive returns in go/rust/java/ruby/python/shared extractors
(null guards, empty-string fallbacks, findEnclosingFn not-found returns) — these require
crafting pathological tree-sitter ASTs to reach and add no safety value as test targets.
CodeRabbit fixes:
- shared-utils.test.ts: ParseError[] instead of any[], remove unused makeNode import,
move findEnclosingDecl to top-level import (no dynamic await import),
assert exact error messages ('parse error at L1', 'missing node: identifier')
- dashboard-command.test.ts: assert full opener message string
extract/ branches: 67% → 74%
…collectDecls
The C extractor's collectDecls() only inspected direct children and silently
dropped declarations nested under #ifdef/#if blocks, typedef wrappers, and
other container nodes. Add the same else { recurse } pattern used by cpp.ts
so functions, structs, and includes inside preprocessor conditionals are found.
Adds a regression test: function inside #ifdef is now extracted.
Collaborator
Author
|
Addressed all remaining actionable findings: c.ts (Major — already fixed in cpp.ts (Major): The namespace-qualification fix from commit shared-utils.test.ts (already fixed in
dashboard-command.test.ts (already fixed in open.ts / types.ts: |
…ge extractors Skips null-guard branches (namedChild null, name === null, body === null) and not-found returns that can't be triggered without pathological ASTs. extract/ branches: 74% → 82%.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
hivemind dashboardSSH auto-detect: automatically switches to--serveon SSH/VS Code Remote/Codespaces; falls back to serve when no local browser opener is foundGraph changes
New extractors (
src/graph/extract/)shared.tsjavascript.tspython.tsgo.tsrust.tsjava.tsruby.tsc.tscpp.tsDispatch (
src/graph/extract/index.ts)Extended
extractFile()to route all 9 languages.isSourceFile()regex updated to cover all extension sets.Grammar packages (
package.json)All pinned to the last ABI-14-compatible 0.23.x release (compatible with
tree-sitter@0.21):esbuild externals
New grammar packages added to CC bundle, CLI bundle, and Codex/Cursor/Hermes bundles (which include
graph-pull-workerthat transitively imports all extractors).Dashboard SSH fix (
src/dashboard/open.ts,src/commands/dashboard.ts)isRemoteSession()detectsSSH_CLIENT,SSH_TTY,VSCODE_INJECTION,CODESPACESenv vars. When any is set,hivemind dashboardauto-enables--serveand prints the localhost URL instead of tryingxdg-open. Also falls back to serve when the opener binary isn't found locally.E2E test results (all 0 skipped)
Session Context
Session transcript
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Chores
Tests