Conversation
Include src/exp-algo modules in FLEX and add an integration test for flex.exp.louvain. Co-Authored-By: Warp <agent@warp.dev>
Allow jest-coverage-report-action to write checks/PR comments on pull_request runs. Co-Authored-By: Warp <agent@warp.dev>
jest-coverage-report-action defaults to parallel Jest execution, which flakes for the integration tests; run it with --runInBand. Co-Authored-By: Warp <agent@warp.dev>
Document flex.exp.louvain and add an experimental warning in docs index and main README. Co-Authored-By: Warp <agent@warp.dev>
Co-Authored-By: Warp <agent@warp.dev>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds an experimental Louvain community-detection feature (exp.louvain) with traversal helpers (BFS/DFS), community utilities, tests, docs, and CI tweaks to run tests sequentially for stability. Changes
Sequence Diagram(s)sequenceDiagram
participant Test as Test / Client
participant Louvain as exp.louvain
participant BuildAdj as buildUndirectedAdjacency
participant Graph as graph.traverse
participant OneLvl as oneLevel
participant InducedG as inducedGraph
Test->>Louvain: invoke(nodes, direction, ...)
Louvain->>BuildAdj: build adjacency (nodes, direction, getWeight)
BuildAdj->>Graph: traverse node edges
Graph-->>BuildAdj: edges
BuildAdj-->>Louvain: adjacency, nodeIds
loop per level (<= maxLevels)
Louvain->>OneLvl: optimize partition (adjacency)
OneLvl-->>Louvain: partition, moved
alt moved
Louvain->>InducedG: create coarsened graph from partition
InducedG->>Graph: (traverse/aggregate edges as needed)
Graph-->>InducedG: aggregated edges
InducedG-->>Louvain: new adjacency
else no moves
Louvain-->>Louvain: stop iterations
end
end
Louvain->>Louvain: renumber partition -> communities
Louvain-->>Test: return { partition, communities, levels }
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 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 |
Coverage report
Show new covered files 🐣
Test suite run success79 tests passing in 27 suites. Report generated by 🧪jest coverage report action from 29437e9 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/test.yml (1)
33-54:⚠️ Potential issue | 🟡 MinorInconsistent parallelism between the two test steps.
Line 36 runs
npm test -- --coverage(parallel by default), while Line 54 runsnpx jest --runInBand --forceExit. If integration tests are unsafe to run in parallel (as the comment on Line 51-53 states), the first "Run Tests with Coverage" step on Line 36 has the same flakiness risk.Consider adding
--runInBand --forceExitto Line 36 as well, or extracting the test command into a shared npm script so both steps stay consistent.Proposed fix
- name: Run Tests with Coverage # Because coverageThreshold is in jest.config.js, # this step will fail if coverage is < 90% - run: npm test -- --coverage + run: npx jest --runInBand --forceExit --coverage env: DEBUG: "true"
🤖 Fix all issues with AI agents
In `@src/exp-algo/filterBFS.js`:
- Around line 52-54: The break condition in the BFS loop allows one extra
visited node because it uses "visited.size > maxVisited"; change this to
"visited.size >= maxVisited" inside the loop that contains currentLevel/depth
checks (the BFS function using variables currentLevel, depth, visited,
maxVisited) so the traversal stops once visited reaches the intended upper
bound; ensure any tests or documentation that state maxVisited as a strict cap
reflect this semantics.
- Line 32: The default getNeighbor in filterBFS.js (and the analogous one in
filterDFS.js) always returns edge.source and ignores the current node, causing
traversal to return to the same node; update getNeighbor(edge, current) to check
edge and current and return the opposite endpoint (if edge.source.id ===
current.id return edge.target, else return edge.source), handling missing
IDs/undefined edge safely — follow the pattern used in community.js to compare
node IDs and return the other endpoint so BFS/DFS advances to the real neighbor.
In `@src/exp-algo/leiden.js`:
- Around line 37-44: The seeding logic in makeSeededRandom currently uses "(seed
>>> 0) || 1" which maps an explicit seed of 0 to 1; change it so only
absent/undefined seeds default to 1 while preserving an explicit 0. Concretely,
in makeSeededRandom set s = (seed === undefined || seed === null) ? 1 : (seed
>>> 0) (so an explicit 0 stays 0), leaving the LCG body (s = (1664525 * s +
1013904223) >>> 0; return s / 4294967296;) unchanged; reference function
makeSeededRandom and variable s.
- Around line 76-92: The comment says "BFS" but the code uses q.pop(), which
implements DFS; either change the comment to reflect DFS or change q.pop() to
q.shift() to perform a true BFS; locate the loop using q, q.pop(), visited,
refined and adjacency in the component expansion logic and update the comment to
"DFS" if you want to keep the current stack-based traversal or replace pop()
with shift() (and optionally adjust any performance expectations) to implement
an actual BFS.
In `@src/exp-algo/louvain.js`:
- Around line 38-65: The JSDoc for the louvain function is missing the
documented `@param` for the debug option; update the comment block above function
louvain to add a line like "@param {boolean} [params.debug=false] Enable debug
logging/tracing" (or similar) so the debug parameter is documented; ensure the
param name matches the function signature (debug) and place it with the other
`@param` entries in the JSDoc for louvain.
🧹 Nitpick comments (9)
docs/exp-algo/louvain.md (1)
92-93: Add cross-reference toexp.leidenin See Also.
leiden.mdlinks back tolouvain.md, but this page doesn't link to Leiden. Adding the cross-reference keeps navigation symmetric.Proposed fix
## See Also +- [exp.leiden](./leiden.md) - [docs/README.md](../README.md)docs/exp-algo/leiden.md (1)
17-30: Markdownlint: add blank line before the parameters table.Per MD058, tables should be surrounded by blank lines. There's a missing blank line between the
## Parametersheading and the table.Proposed fix
## Parameters + | Parameter | Type | Required | Description |tests/exp-algo/louvain.test.js (2)
27-136: Consider extracting the shared test scaffold withleiden.test.js.The graph-creation Cypher (lines 36-75), the
idToNamemapping (lines 77-87), the community-size assertions (lines 102-117), thesameSethelper (lines 123-127), and the team-membership checks (lines 119-135) are nearly identical to the Leiden test. This makes future maintenance harder — any graph topology change must be mirrored in both files.Consider extracting the shared parts (graph seeding,
idToNamebuilder,sameSet, team definitions and assertions) into a common test helper, keeping only the algorithm-specific Cypher call in each test file.
96-97: No defensive check before accessingout.data[0].res.If the query returns no rows (e.g. graph setup failed silently),
out.data[0]will throw aTypeError. A guard or a precedingexpect(out.data.length).toBeGreaterThan(0)would make failures easier to diagnose.Proposed improvement
const out = await graph.query(resQ); + expect(out.data).toBeDefined(); + expect(out.data.length).toBeGreaterThan(0); const res = out.data[0].res;tests/exp-algo/leiden.test.js (1)
120-121: Same defensive-check gap as the Louvain test.
out.data[0].resis accessed without guarding against empty results. Same suggestion applies: add anexpect(out.data.length).toBeGreaterThan(0)before dereferencing.src/exp-algo/louvain.js (1)
140-148: Inconsistentmodule.exportssurface between Louvain and Leiden.Louvain re-exports shared community.js utilities (
getEdgeWeight,buildUndirectedAdjacency,oneLevel,inducedGraph) alongsidelouvain, while Leiden only exportsrefineByConnectedComponentsandleiden. If a consumer imports fromlouvain.jsexpecting these helpers, they'd be surprised they're absent fromleiden.js(and vice versa).Consider either making both modules export only their own algorithm (plus any module-local functions like
refineByConnectedComponents), or documenting that community.js is the canonical import path for shared utilities.src/exp-algo/community.js (3)
62-66: Edge-key separator::can collide with node IDs that contain::, breaking adjacency reconstruction.
makeUndirectedEdgeKey(line 65) joins IDs with::, andbuildUndirectedAdjacency(line 252) callskey.split('::')to recover endpoints. If a node ID ever contains the substring::, the split produces more than two parts and the wrong IDs are looked up.In practice FalkorDB node IDs are integers, so this is safe today, but it's fragile if IDs become user-controlled strings.
A simple fix is to limit the split:
Safer split
- const parts = key.split('::'); - const aStr = parts[0]; - const bStr = parts[1]; + const sepIdx = key.indexOf('::'); + const aStr = key.slice(0, sepIdx); + const bStr = key.slice(sepIdx + 2);This still won't handle IDs containing
::inmakeUndirectedEdgeKey's lexicographic ordering, but at least the split is correct for a single separator occurrence. A fully robust approach would use a delimiter that cannot appear in stringified IDs, or store endpoints in aMapkeyed by a composite tuple.Also applies to: 251-264
22-27:defineIfMissingalways assigns — name is misleading.The comment on lines 23-26 explains the rationale (reload safety, order-independence), but the function name implies conditional assignment. A name like
defineorregisterwould be more accurate.
140-285:buildUndirectedAdjacency— solid implementation with one assumption worth documenting.Line 180:
const edges = reachables[0] || []assumesgraph.traversereturns an array-of-arrays where the first element is the edge list. This is a FalkorDB-specific convention. Consider adding a brief inline comment clarifying this contract for future maintainers.
| while (currentLevel.length > 0) { | ||
| if (depth >= maxDepth) break; | ||
| if (visited.size > maxVisited) break; |
There was a problem hiding this comment.
maxVisited check uses > — allows maxVisited + 1 entries.
visited is initialized with startId (size 1), and the break condition is visited.size > maxVisited. If maxVisited = 5, this breaks at size 6. If the intent is to cap at exactly maxVisited nodes, use >=.
This is a minor semantic question — just verify the intended behavior matches the documentation (maxVisited as an upper bound).
🤖 Prompt for AI Agents
In `@src/exp-algo/filterBFS.js` around lines 52 - 54, The break condition in the
BFS loop allows one extra visited node because it uses "visited.size >
maxVisited"; change this to "visited.size >= maxVisited" inside the loop that
contains currentLevel/depth checks (the BFS function using variables
currentLevel, depth, visited, maxVisited) so the traversal stops once visited
reaches the intended upper bound; ensure any tests or documentation that state
maxVisited as a strict cap reflect this semantics.
src/exp-algo/leiden.js
Outdated
| function makeSeededRandom(seed) { | ||
| // Simple LCG (deterministic). | ||
| let s = (seed >>> 0) || 1; | ||
| return function random() { | ||
| s = (1664525 * s + 1013904223) >>> 0; | ||
| return s / 4294967296; | ||
| }; | ||
| } |
There was a problem hiding this comment.
seed = 0 silently maps to seed = 1, making them indistinguishable.
(seed >>> 0) || 1 converts 0 to 1. If a caller explicitly passes seed: 0 for reproducibility, they'd unknowingly get the same sequence as seed: 1.
Suggested fix
- let s = (seed >>> 0) || 1;
+ let s = seed === 0 ? 0 : (seed >>> 0) || 1;Note: an LCG starting at state 0 with additive constant 1013904223 (odd) will still cycle through all 2³² states, so s = 0 is a valid starting state for this particular LCG.
🧰 Tools
🪛 GitHub Check: Coverage annotations (🧪 jest-coverage-report-action)
[warning] 39-39: 🧾 Statement is not covered
Warning! Not covered statement
[warning] 40-43: 🧾 Statement is not covered
Warning! Not covered statement
[warning] 41-41: 🧾 Statement is not covered
Warning! Not covered statement
[warning] 42-42: 🧾 Statement is not covered
Warning! Not covered statement
🤖 Prompt for AI Agents
In `@src/exp-algo/leiden.js` around lines 37 - 44, The seeding logic in
makeSeededRandom currently uses "(seed >>> 0) || 1" which maps an explicit seed
of 0 to 1; change it so only absent/undefined seeds default to 1 while
preserving an explicit 0. Concretely, in makeSeededRandom set s = (seed ===
undefined || seed === null) ? 1 : (seed >>> 0) (so an explicit 0 stays 0),
leaving the LCG body (s = (1664525 * s + 1013904223) >>> 0; return s /
4294967296;) unchanged; reference function makeSeededRandom and variable s.
src/exp-algo/leiden.js
Outdated
| // BFS | ||
| const q = [start]; | ||
| visited.add(start); | ||
| refined.set(start, compKey); | ||
|
|
||
| while (q.length) { | ||
| const v = q.pop(); | ||
| const neigh = adjacency.get(v); | ||
| if (!neigh) continue; | ||
|
|
||
| for (const nbr of neigh.keys()) { | ||
| if (!inComm.has(nbr)) continue; | ||
| if (visited.has(nbr)) continue; | ||
| visited.add(nbr); | ||
| refined.set(nbr, compKey); | ||
| q.push(nbr); | ||
| } |
There was a problem hiding this comment.
Comment says "BFS" but q.pop() is DFS (stack).
Using pop() makes this a depth-first traversal. Both DFS and BFS correctly find connected components, so there's no functional bug, but the comment is misleading.
Fix: either update the comment or switch to shift()
- // BFS
const q = [start];
+ // DFS (stack) – any traversal order finds the same components.
visited.add(start);Or use q.shift() for actual BFS (though DFS is fine and faster here without a proper queue).
🧰 Tools
🪛 GitHub Check: Coverage annotations (🧪 jest-coverage-report-action)
[warning] 84-84: 🧾 Statement is not covered
Warning! Not covered statement
[warning] 87-87: 🧾 Statement is not covered
Warning! Not covered statement
🤖 Prompt for AI Agents
In `@src/exp-algo/leiden.js` around lines 76 - 92, The comment says "BFS" but the
code uses q.pop(), which implements DFS; either change the comment to reflect
DFS or change q.pop() to q.shift() to perform a true BFS; locate the loop using
q, q.pop(), visited, refined and adjacency in the component expansion logic and
update the comment to "DFS" if you want to keep the current stack-based
traversal or replace pop() with shift() (and optionally adjust any performance
expectations) to implement an actual BFS.
| /** | ||
| * Louvain algorithm. | ||
| * | ||
| * @param {object} params | ||
| * @param {any[]} params.nodes Input nodes (objects), must include `id` by default. | ||
| * @param {string|string[]} [params.direction='both'] Traversal direction(s) used to build adjacency. | ||
| * @param {number} [params.maxEdgesPerNode=Infinity] Safety cap per node traversal. | ||
| * @param {number} [params.resolution=1] Modularity resolution (gamma). | ||
| * @param {number} [params.maxPasses=10] Max passes per level. | ||
| * @param {number} [params.maxLevels=10] Max coarsening levels. | ||
| * @param {number} [params.minGain=1e-12] Minimum gain threshold to accept a move. | ||
| * @param {(node:any)=>any} [params.getNodeId] | ||
| * @param {(edge:any)=>number} [params.getWeight] | ||
| * | ||
| * @returns {{ partition: Object, communities: Object, levels: number }} | ||
| */ | ||
| function louvain({ | ||
| nodes, | ||
| direction = 'both', | ||
| maxEdgesPerNode = Infinity, | ||
| resolution = 1, | ||
| maxPasses = 10, | ||
| maxLevels = 10, | ||
| minGain = 1e-12, | ||
| getNodeId = exp.defaultGetNodeId, | ||
| getWeight = (edge) => exp.getEdgeWeight(edge), | ||
| debug = false, | ||
| }) { |
There was a problem hiding this comment.
debug parameter is undocumented in JSDoc.
Line 64 accepts debug = false but the JSDoc block (lines 38-53) doesn't mention it. This matters for consumers of the public API.
Add the missing `@param`
* `@param` {(edge:any)=>number} [params.getWeight]
- *
+ * `@param` {boolean} [params.debug=false] Emit debug data in the result.
* `@returns` {{ partition: Object, communities: Object, levels: number }}📝 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.
| /** | |
| * Louvain algorithm. | |
| * | |
| * @param {object} params | |
| * @param {any[]} params.nodes Input nodes (objects), must include `id` by default. | |
| * @param {string|string[]} [params.direction='both'] Traversal direction(s) used to build adjacency. | |
| * @param {number} [params.maxEdgesPerNode=Infinity] Safety cap per node traversal. | |
| * @param {number} [params.resolution=1] Modularity resolution (gamma). | |
| * @param {number} [params.maxPasses=10] Max passes per level. | |
| * @param {number} [params.maxLevels=10] Max coarsening levels. | |
| * @param {number} [params.minGain=1e-12] Minimum gain threshold to accept a move. | |
| * @param {(node:any)=>any} [params.getNodeId] | |
| * @param {(edge:any)=>number} [params.getWeight] | |
| * | |
| * @returns {{ partition: Object, communities: Object, levels: number }} | |
| */ | |
| function louvain({ | |
| nodes, | |
| direction = 'both', | |
| maxEdgesPerNode = Infinity, | |
| resolution = 1, | |
| maxPasses = 10, | |
| maxLevels = 10, | |
| minGain = 1e-12, | |
| getNodeId = exp.defaultGetNodeId, | |
| getWeight = (edge) => exp.getEdgeWeight(edge), | |
| debug = false, | |
| }) { | |
| /** | |
| * Louvain algorithm. | |
| * | |
| * `@param` {object} params | |
| * `@param` {any[]} params.nodes Input nodes (objects), must include `id` by default. | |
| * `@param` {string|string[]} [params.direction='both'] Traversal direction(s) used to build adjacency. | |
| * `@param` {number} [params.maxEdgesPerNode=Infinity] Safety cap per node traversal. | |
| * `@param` {number} [params.resolution=1] Modularity resolution (gamma). | |
| * `@param` {number} [params.maxPasses=10] Max passes per level. | |
| * `@param` {number} [params.maxLevels=10] Max coarsening levels. | |
| * `@param` {number} [params.minGain=1e-12] Minimum gain threshold to accept a move. | |
| * `@param` {(node:any)=>any} [params.getNodeId] | |
| * `@param` {(edge:any)=>number} [params.getWeight] | |
| * `@param` {boolean} [params.debug=false] Emit debug data in the result. | |
| * `@returns` {{ partition: Object, communities: Object, levels: number }} | |
| */ | |
| function louvain({ | |
| nodes, | |
| direction = 'both', | |
| maxEdgesPerNode = Infinity, | |
| resolution = 1, | |
| maxPasses = 10, | |
| maxLevels = 10, | |
| minGain = 1e-12, | |
| getNodeId = exp.defaultGetNodeId, | |
| getWeight = (edge) => exp.getEdgeWeight(edge), | |
| debug = false, | |
| }) { |
🤖 Prompt for AI Agents
In `@src/exp-algo/louvain.js` around lines 38 - 65, The JSDoc for the louvain
function is missing the documented `@param` for the debug option; update the
comment block above function louvain to add a line like "@param {boolean}
[params.debug=false] Enable debug logging/tracing" (or similar) so the debug
parameter is documented; ensure the param name matches the function signature
(debug) and place it with the other `@param` entries in the JSDoc for louvain.
Co-Authored-By: Warp <agent@warp.dev>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@docs/exp-algo/degreeCentrality.md`:
- Around line 19-26: Insert a blank line above the Parameters table to satisfy
the MD058 lint rule, and update the `maxEdgesPerNode` parameter type from
`integer` to `number` (since its default is `Infinity`) in the
degreeCentrality.md parameters table so the signature for `maxEdgesPerNode`
accurately reflects allowed values; leave descriptions and defaults (Infinity)
unchanged.
🧹 Nitpick comments (3)
src/exp-algo/degreeCentrality.js (2)
36-47: Missing input validation fornodesparameter.If
nodesisundefined,null, or not an array, the error will propagate frombuildUndirectedAdjacencywith an unclear message. A guard at the entry point ofdegreeCentralitywould produce a more actionable error for callers.🛡️ Suggested validation
function degreeCentrality({ nodes, direction = 'both', maxEdgesPerNode = Infinity, normalized = true, getNodeId = exp.defaultGetNodeId, getWeight = (edge) => exp.getEdgeWeight(edge), debug = false, }) { + if (!Array.isArray(nodes) || nodes.length === 0) { + throw new TypeError('exp.degreeCentrality: "nodes" must be a non-empty array'); + } if (typeof exp.buildUndirectedAdjacency !== 'function') {
62-87: Minor:normalizedByIdis allocated unconditionally.
normalizedByIdis created on Line 64 even whennormalizedisfalse, though it will remain empty and unused. This is negligible for typical graph sizes but could be tightened for consistency.tests/exp-algo/degreeCentrality.test.js (1)
8-25: Consider adding an edge-case test for a single-node or empty graph.The
normalizedcomputation has a special case forn <= 1(returns 0). A test with a single isolated node would exercise that branch and confirm no division-by-zero occurs.
docs/exp-algo/degreeCentrality.md
Outdated
| ## Parameters | ||
| | Parameter | Type | Required | Description | | ||
| |-----------|------|----------|-------------| | ||
| | `nodes` | `list<node>` | Yes | The set of nodes to analyze. Typically provided via `collect(n)`. | | ||
| | `direction` | `string \| list<string>` | No | Which traversal direction(s) to use when building adjacency. Common values: `'incoming'`, `'outgoing'`, `'both'`. Default: `'both'`. | | ||
| | `maxEdgesPerNode` | `integer` | No | Safety cap on how many edges to scan per node when building adjacency. Default: `Infinity`. | | ||
| | `normalized` | `boolean` | No | When `true`, also returns normalized degree (divide by `n-1`). Default: `true`. | | ||
| | `debug` | `boolean` | No | When `true`, returns additional debug information about adjacency building. Default: `false`. | |
There was a problem hiding this comment.
Add blank line before the parameters table (MD058).
The markdown linter flags a missing blank line before the table on Line 20. Also, maxEdgesPerNode is typed as integer but defaults to Infinity — consider updating the type to number for accuracy.
Suggested fix
## Parameters
+
| Parameter | Type | Required | Description |
|-----------|------|----------|-------------|
| `nodes` | `list<node>` | Yes | The set of nodes to analyze. Typically provided via `collect(n)`. |
| `direction` | `string \| list<string>` | No | Which traversal direction(s) to use when building adjacency. Common values: `'incoming'`, `'outgoing'`, `'both'`. Default: `'both'`. |
-| `maxEdgesPerNode` | `integer` | No | Safety cap on how many edges to scan per node when building adjacency. Default: `Infinity`. |
+| `maxEdgesPerNode` | `number` | No | Safety cap on how many edges to scan per node when building adjacency. Default: `Infinity`. |📝 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.
| ## Parameters | |
| | Parameter | Type | Required | Description | | |
| |-----------|------|----------|-------------| | |
| | `nodes` | `list<node>` | Yes | The set of nodes to analyze. Typically provided via `collect(n)`. | | |
| | `direction` | `string \| list<string>` | No | Which traversal direction(s) to use when building adjacency. Common values: `'incoming'`, `'outgoing'`, `'both'`. Default: `'both'`. | | |
| | `maxEdgesPerNode` | `integer` | No | Safety cap on how many edges to scan per node when building adjacency. Default: `Infinity`. | | |
| | `normalized` | `boolean` | No | When `true`, also returns normalized degree (divide by `n-1`). Default: `true`. | | |
| | `debug` | `boolean` | No | When `true`, returns additional debug information about adjacency building. Default: `false`. | | |
| ## Parameters | |
| | Parameter | Type | Required | Description | | |
| |-----------|------|----------|-------------| | |
| | `nodes` | `list<node>` | Yes | The set of nodes to analyze. Typically provided via `collect(n)`. | | |
| | `direction` | `string \| list<string>` | No | Which traversal direction(s) to use when building adjacency. Common values: `'incoming'`, `'outgoing'`, `'both'`. Default: `'both'`. | | |
| | `maxEdgesPerNode` | `number` | No | Safety cap on how many edges to scan per node when building adjacency. Default: `Infinity`. | | |
| | `normalized` | `boolean` | No | When `true`, also returns normalized degree (divide by `n-1`). Default: `true`. | | |
| | `debug` | `boolean` | No | When `true`, returns additional debug information about adjacency building. Default: `false`. | |
🧰 Tools
🪛 markdownlint-cli2 (0.20.0)
[warning] 20-20: Tables should be surrounded by blank lines
(MD058, blanks-around-tables)
🤖 Prompt for AI Agents
In `@docs/exp-algo/degreeCentrality.md` around lines 19 - 26, Insert a blank line
above the Parameters table to satisfy the MD058 lint rule, and update the
`maxEdgesPerNode` parameter type from `integer` to `number` (since its default
is `Infinity`) in the degreeCentrality.md parameters table so the signature for
`maxEdgesPerNode` accurately reflects allowed values; leave descriptions and
defaults (Infinity) unchanged.
This reverts commit b2026bf.
Co-Authored-By: Warp <agent@warp.dev>
Co-Authored-By: Warp <agent@warp.dev>
Co-Authored-By: Warp <agent@warp.dev>
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@src/exp-algo/community.js`:
- Around line 112-123: The error thrown in traverseEdgesForNode currently
references the wrong function name ("buildUndirectedAdjacency"); update the
thrown TypeError in traverseEdgesForNode to use the correct function name or a
generic message (e.g., "traverseEdgesForNode: global `graph.traverse` is not
available" or "global `graph.traverse` is not available") so the message matches
the function and aids debugging; modify the string in the TypeError thrown
inside traverseEdgesForNode accordingly.
- Around line 412-435: The tot map is updated incorrectly when a candidate
community has positive gain but ≤ minGain: after evaluating neighComms you
subtract nodeDegree from currentComm (tot.set(currentComm,...)) but always add
nodeDegree back to bestComm, which may differ from currentComm when the move was
rejected; fix by adding nodeDegree back to the community the node actually
remains in — use partition.get(nodeId) or re-check whether bestComm was assigned
as the final community and call tot.set(finalComm, (tot.get(finalComm)||0) +
nodeDegree) instead of unconditionally tot.set(bestComm,...); update the block
around symbols tot, partition, currentComm, bestComm, nodeId, nodeDegree,
minGain and movedThisPass so tot is restored to the correct community whenever a
move is rejected or accepted.
- Around line 49-53: The current stableKeyPart function converts node ids to
String(x) causing collisions between values like 1 and "1"; update stableKeyPart
to preserve type information in the produced key (for example incorporate typeof
x into the returned key or otherwise serialize type+value) so numeric and string
ids remain distinct; ensure the change is applied to the define('stableKeyPart',
function stableKeyPart(x) { ... }) implementation and adjust any consumers
expecting the old string-only key format if necessary.
🧹 Nitpick comments (3)
build.js (1)
22-23: The outer sort on Line 39-40 makes the inner sort on Line 23 redundant.The final
.sort()onallFilesby relative path already guarantees deterministic ordering of the complete list. The per-directory sort insidegetAllFilesis therefore unnecessary — though harmless, it adds a small amount of overhead on every recursive call.Not blocking; keeping both is a valid "belt and suspenders" approach.
Also applies to: 39-40
src/exp-algo/community.js (2)
62-70: Silent fallback when neither endpoint matchescurrentId.Line 70 returns
s(source) when both endpoints are valid but neither matchescurrentId. This silently picks an arbitrary neighbor rather than signaling a logic error. Consider logging or returningnullto surface misuse.
176-243: Potential double-counting of edge weights whendirection = 'both'.With the default
direction = 'both',normalizeTraverseDirectionsexpands to['incoming', 'outgoing']. For a directed edge A→B, traversing outgoing from A yields the edge, and traversing incoming to B also yields the same edge. SinceaddUndirectedaccumulates weights, the canonical pair (A, B) gets2winstead ofw.This is likely benign for Louvain because all weights and degrees inflate uniformly, preserving the relative modularity-gain ordering. However, it means
debug.totalUndirectedWeightwill report inflated values, and callers relying on absolute weight semantics (e.g., a future weighted-cut metric) will get incorrect results. Worth documenting this behavior or deduplicating edges.
| define('stableKeyPart', function stableKeyPart(x) { | ||
| // Stable ordering for undirected edge keys. | ||
| // Note: this can collide for ids like 1 and "1". | ||
| return String(x); | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Search for node ID patterns and usage
rg -n 'getNodeId|node\.id|nodeId' --type=js -C2 -g '!node_modules/**'Repository: FalkorDB/flex
Length of output: 12935
🏁 Script executed:
# Also check the louvain.js file directly to understand node ID sources
rg -n 'nodeId|node\.id' src/exp-algo/louvain.js -C3Repository: FalkorDB/flex
Length of output: 1013
🏁 Script executed:
# Look for test files that might show example node IDs
fd -e test.js -e spec.js | head -20Repository: FalkorDB/flex
Length of output: 647
🏁 Script executed:
cat -n tests/exp-algo/louvain.test.js | head -100Repository: FalkorDB/flex
Length of output: 3949
🏁 Script executed:
# Also check if there are any examples with mixed numeric and string node IDs
rg -A5 -B5 'getNodeId' tests/exp-algo/louvain.test.js | head -50Repository: FalkorDB/flex
Length of output: 39
🏁 Script executed:
# Check if there's any type validation or coercion documented
rg -n 'node.*id|nodeId' src/exp-algo/community.js -B2 -A2 | grep -E "(type|typeof|number|string|warning|invariant)" | head -20Repository: FalkorDB/flex
Length of output: 222
🏁 Script executed:
# Look at the defaultGetNodeId implementation more closely
sed -n '42,47p' src/exp-algo/community.jsRepository: FalkorDB/flex
Length of output: 261
🏁 Script executed:
# Check if there are any comments about mixed types or invariants
rg -n 'invariant|assume|consistent|type.*id' src/exp-algo/ -iRepository: FalkorDB/flex
Length of output: 824
Fix collision handling in stableKeyPart for mixed-type node IDs.
The stableKeyPart function converts all node IDs to strings, which causes numeric 1 and string "1" to collide and silently merge as distinct nodes. Since defaultGetNodeId explicitly accepts both number and string types, and the function signature is generic ((node:any)=>any), mixed-type node IDs are permissible. Either enforce type consistency in node IDs or use a deduping strategy that respects type (e.g., include typeof id in the stable key).
🤖 Prompt for AI Agents
In `@src/exp-algo/community.js` around lines 49 - 53, The current stableKeyPart
function converts node ids to String(x) causing collisions between values like 1
and "1"; update stableKeyPart to preserve type information in the produced key
(for example incorporate typeof x into the returned key or otherwise serialize
type+value) so numeric and string ids remain distinct; ensure the change is
applied to the define('stableKeyPart', function stableKeyPart(x) { ... })
implementation and adjust any consumers expecting the old string-only key format
if necessary.
| define('traverseEdgesForNode', function traverseEdgesForNode(node, dir) { | ||
| if (typeof graph === 'undefined' || !graph || typeof graph.traverse !== 'function') { | ||
| throw new TypeError('buildUndirectedAdjacency: global `graph.traverse` is not available'); | ||
| } | ||
|
|
||
| return ( | ||
| graph.traverse([node], { | ||
| direction: dir, | ||
| returnType: 'edges', | ||
| }) || [] | ||
| ); | ||
| }); |
There was a problem hiding this comment.
Error message references wrong function name.
Line 114 says 'buildUndirectedAdjacency: global \graph.traverse` is not available'but this is thetraverseEdgesForNode` function. The error message should reference the correct function name (or be generic) to avoid confusion during debugging.
Proposed fix
- throw new TypeError('buildUndirectedAdjacency: global `graph.traverse` is not available');
+ throw new TypeError('traverseEdgesForNode: global `graph.traverse` is not available');🤖 Prompt for AI Agents
In `@src/exp-algo/community.js` around lines 112 - 123, The error thrown in
traverseEdgesForNode currently references the wrong function name
("buildUndirectedAdjacency"); update the thrown TypeError in
traverseEdgesForNode to use the correct function name or a generic message
(e.g., "traverseEdgesForNode: global `graph.traverse` is not available" or
"global `graph.traverse` is not available") so the message matches the function
and aids debugging; modify the string in the TypeError thrown inside
traverseEdgesForNode accordingly.
| tot.set(currentComm, (tot.get(currentComm) || 0) - nodeDegree); | ||
|
|
||
| let bestComm = currentComm; | ||
| let bestGain = 0; | ||
|
|
||
| for (const [c, k_i_in] of neighComms.entries()) { | ||
| const totC = tot.get(c) || 0; | ||
| const gain = k_i_in - (resolution * totC * nodeDegree) / m2; | ||
| if (gain > bestGain) { | ||
| bestGain = gain; | ||
| bestComm = c; | ||
| } | ||
| } | ||
|
|
||
| if (bestComm !== currentComm && bestGain > minGain) { | ||
| partition.set(nodeId, bestComm); | ||
| movedThisPass = true; | ||
| movedAny = true; | ||
| moves += 1; | ||
| } else { | ||
| partition.set(nodeId, currentComm); | ||
| } | ||
|
|
||
| tot.set(bestComm, (tot.get(bestComm) || 0) + nodeDegree); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's read the relevant section of the community.js file
cat -n src/exp-algo/community.js | sed -n '400,445p'Repository: FalkorDB/flex
Length of output: 1796
🏁 Script executed:
# Also check the function signature to understand the context
cat -n src/exp-algo/community.js | sed -n '340,360p'Repository: FalkorDB/flex
Length of output: 638
🏁 Script executed:
# Find where oneLevel is called to understand minGain values
rg -n 'oneLevel|minGain' --type=js -B2 -A2Repository: FalkorDB/flex
Length of output: 2454
Bug: tot map corruption when bestGain is positive but below the minGain threshold.
On Line 412, nodeDegree is subtracted from currentComm. The node is evaluated for moves by finding the best neighboring community (lines 414–424). If a better community is found with a gain above the minGain threshold (default 1e-12), the move is accepted. However, when a better community is found but its gain is positive yet ≤ minGain, the condition on Line 426 fails, the node remains in currentComm (Line 432), yet Line 435 incorrectly adds nodeDegree to bestComm instead of currentComm. This permanently transfers weight from the correct community to an incorrect one, corrupting the tot map and leading to progressively incorrect modularity-gain estimates in subsequent iterations.
🐛 Proposed fix: add nodeDegree back to the community the node actually ends up in
if (bestComm !== currentComm && bestGain > minGain) {
partition.set(nodeId, bestComm);
movedThisPass = true;
movedAny = true;
moves += 1;
} else {
+ bestComm = currentComm;
partition.set(nodeId, currentComm);
}
tot.set(bestComm, (tot.get(bestComm) || 0) + nodeDegree);🤖 Prompt for AI Agents
In `@src/exp-algo/community.js` around lines 412 - 435, The tot map is updated
incorrectly when a candidate community has positive gain but ≤ minGain: after
evaluating neighComms you subtract nodeDegree from currentComm
(tot.set(currentComm,...)) but always add nodeDegree back to bestComm, which may
differ from currentComm when the move was rejected; fix by adding nodeDegree
back to the community the node actually remains in — use partition.get(nodeId)
or re-check whether bestComm was assigned as the final community and call
tot.set(finalComm, (tot.get(finalComm)||0) + nodeDegree) instead of
unconditionally tot.set(bestComm,...); update the block around symbols tot,
partition, currentComm, bestComm, nodeId, nodeDegree, minGain and movedThisPass
so tot is restored to the correct community whenever a move is rejected or
accepted.
1 participant
Louvain and Leiden algorithms with helper functions
Summary by CodeRabbit
New Features
Documentation
Tests
Chores