Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Owner
Author
|
@coderabbitai help |
ChatThere are 3 ways to chat with CodeRabbit:
CodeRabbit commands
Other keywords and placeholders
CodeRabbit configuration file (
|
Owner
Author
|
@coderabbitai resume |
📝 WalkthroughWalkthroughExports a new Neon-backed getAffectedPkg function from Rust and exposes it alongside the existing dag export via updated CommonJS and ESM entrypoints. Bumps package version to 0.3.0 and updates CHANGELOG and README with usage and examples. Changes
Sequence DiagramsequenceDiagram
participant Client
participant NodeAPI as Node.js (CJS/ESM)
participant Native as Native Addon (index.node)
participant Rust as Rust/Neon
Client->>NodeAPI: request getAffectedPkg(packageName)
NodeAPI->>Native: call getAffectedPkg
Native->>Rust: invoke get_affected_pkg(pkgName)
Rust->>Rust: discover package.json files (skip .yarn/node_modules)
Rust->>Rust: build package name map and direct dependents
Rust->>Rust: recurse to collect transitive/circular dependents
Rust-->>Native: return JS array of affected package names
Native-->>NodeAPI: forward result
NodeAPI-->>Client: return affected packages array
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
✅ Actions performedReviews resumed. |
Owner
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@lib/index.js`:
- Around line 3-6: The change removed the previous function-style default export
and broke callers that required the package and invoked it directly; restore
backward compatibility by exporting native.dag as the module itself and also
attach the other exports as properties: set module.exports to native.dag and
then assign native.dag.dag = native.dag (optional) and native.dag.getAffectedPkg
= native.getAffectedPkg (or at minimum module.exports.getAffectedPkg =
native.getAffectedPkg) so consumers can both require('dag-rs')() and access
getAffectedPkg; update any references to native.dag and getAffectedPkg in the
file accordingly.
In `@src/lib.rs`:
- Around line 51-73: The get_affected_pkg function currently returns unit and
only prints matches; change get_affected_pkg to a Neon JS-returning signature
(e.g., fn get_affected_pkg<'a>(cx: FunctionContext<'a>) -> JsResult<'a,
JsObject>), parse the pkg_name argument from the context, collect dependent
names by iterating res (built via get_dependents and get_pkg_name) into a
Vec<String>, then construct a JsArray of those names and a JsObject mapping
pkg_name -> that JsArray (similar to how dag builds and returns its
JsArray/JsObject) and return it via cx.null() or Ok(object) accordingly using
Neon APIs. Ensure you remove println! usage and return the created JsObject so
JS receives { packageName: [affected packages] }.
🧹 Nitpick comments (2)
src/lib.rs (2)
67-67: Unnecessary.enumerate()— index is unused.
_iis discarded; just iterate directly.Proposed fix
- for (_i, (_dependent, dependency)) in res.into_iter().enumerate() { + for (_dependent, dependency) in res.into_iter() {
58-60:paths.clone()can be avoided on the first loop.The first loop only needs shared references; iterating
&pathsavoids cloning. Same pattern exists indag(line 84), so this is a broader cleanup opportunity.Proposed fix
- for path in paths.clone() { - pkg_names.push(get_pkg_name(path.clone())); + for path in &paths { + pkg_names.push(get_pkg_name(path.clone())); }Note:
get_pkg_nametakes an ownedString, so the inner.clone()remains, but you avoid cloning the entireVec. A further improvement would be to changeget_pkg_nameto accept&str.
Comment on lines
3
to
6
| dag: native.dag, | ||
| getAffectedPkg: native.getAffectedPkg | ||
| }; |
There was a problem hiding this comment.
Breaking change: existing consumers calling the module as a function will break.
Previously the module exported native.dag directly, so consumers could write:
const dag = require('dag-rs');
dag(); // workedNow they must destructure or access the property:
const { dag } = require('dag-rs');If this library has downstream users, this is a semver-major change. Ensure the version bump reflects this (or consider a backward-compatible approach such as also attaching getAffectedPkg as a property on the exported dag function).
🤖 Prompt for AI Agents
In `@lib/index.js` around lines 3 - 6, The change removed the previous
function-style default export and broke callers that required the package and
invoked it directly; restore backward compatibility by exporting native.dag as
the module itself and also attach the other exports as properties: set
module.exports to native.dag and then assign native.dag.dag = native.dag
(optional) and native.dag.getAffectedPkg = native.getAffectedPkg (or at minimum
module.exports.getAffectedPkg = native.getAffectedPkg) so consumers can both
require('dag-rs')() and access getAffectedPkg; update any references to
native.dag and getAffectedPkg in the file accordingly.
Comment on lines
51
to
73
| #[neon::export] | ||
| fn get_affected_pkg(pkg_name: String) { | ||
| let filter = vec![".yarn", "node_modules"]; | ||
| let paths = find_pkg_json::find_pkg_json(filter); | ||
| let mut res: Vec<(String, String)> = Vec::new(); | ||
| let mut pkg_names = Vec::new(); | ||
|
|
||
| for path in paths.clone() { | ||
| pkg_names.push(get_pkg_name(path.clone())); | ||
| } | ||
|
|
||
| for path in paths.clone() { | ||
| let mut r = get_dependents(&path, &pkg_names); | ||
| res.append(&mut r); | ||
| } | ||
| // res stores the dependents and dependency | ||
| for (_i, (_dependent, dependency)) in res.into_iter().enumerate() { | ||
| if dependency == pkg_name { | ||
| println!("pkg found"); | ||
| } | ||
| } | ||
| println!("{}", pkg_name); | ||
| } |
There was a problem hiding this comment.
Function does not return any value — the JS caller receives undefined.
The function signature returns () (unit), so calling getAffectedPkg(name) from JavaScript will always yield undefined. Per issue #4, it should return a mapping in the shape { packageName: [affected packages] }.
Currently the function only println!s matches instead of collecting and returning them. Compare with the dag function (lines 74–101), which builds and returns a JsArray.
You need to:
- Change the return type to produce a JS-compatible value (e.g.,
JsResult<'a, JsObject>). - Collect the matching dependent names into a list.
- Build and return a JS object with the
pkg_namekey mapped to that list.
🐛 Sketch of a possible fix
-#[neon::export]
-fn get_affected_pkg(pkg_name: String) {
+#[neon::export]
+fn get_affected_pkg<'a>(cx: &mut FunctionContext<'a>, pkg_name: String) -> JsResult<'a, JsObject> {
let filter = vec![".yarn", "node_modules"];
let paths = find_pkg_json::find_pkg_json(filter);
let mut res: Vec<(String, String)> = Vec::new();
let mut pkg_names = Vec::new();
for path in paths.clone() {
pkg_names.push(get_pkg_name(path.clone()));
}
for path in paths.clone() {
let mut r = get_dependents(&path, &pkg_names);
res.append(&mut r);
}
- // res stores the dependents and dependency
- for (_i, (_dependent, dependency)) in res.into_iter().enumerate() {
- if dependency == pkg_name {
- println!("pkg found");
- }
- }
- println!("{}", pkg_name);
+
+ let affected: Vec<String> = res
+ .into_iter()
+ .filter(|(_dependent, dependency)| dependency == &pkg_name)
+ .map(|(dependent, _)| dependent)
+ .collect();
+
+ let result = cx.empty_object();
+ let js_array = JsArray::new(cx, affected.len() as u32);
+ for (i, name) in affected.iter().enumerate() {
+ let js_str = cx.string(name);
+ js_array.set(cx, i as u32, js_str)?;
+ }
+ result.set(cx, pkg_name.as_str(), js_array)?;
+ Ok(result)
}📝 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
| #[neon::export] | |
| fn get_affected_pkg(pkg_name: String) { | |
| let filter = vec![".yarn", "node_modules"]; | |
| let paths = find_pkg_json::find_pkg_json(filter); | |
| let mut res: Vec<(String, String)> = Vec::new(); | |
| let mut pkg_names = Vec::new(); | |
| for path in paths.clone() { | |
| pkg_names.push(get_pkg_name(path.clone())); | |
| } | |
| for path in paths.clone() { | |
| let mut r = get_dependents(&path, &pkg_names); | |
| res.append(&mut r); | |
| } | |
| // res stores the dependents and dependency | |
| for (_i, (_dependent, dependency)) in res.into_iter().enumerate() { | |
| if dependency == pkg_name { | |
| println!("pkg found"); | |
| } | |
| } | |
| println!("{}", pkg_name); | |
| } | |
| #[neon::export] | |
| fn get_affected_pkg<'a>(cx: &mut FunctionContext<'a>, pkg_name: String) -> JsResult<'a, JsObject> { | |
| let filter = vec![".yarn", "node_modules"]; | |
| let paths = find_pkg_json::find_pkg_json(filter); | |
| let mut res: Vec<(String, String)> = Vec::new(); | |
| let mut pkg_names = Vec::new(); | |
| for path in paths.clone() { | |
| pkg_names.push(get_pkg_name(path.clone())); | |
| } | |
| for path in paths.clone() { | |
| let mut r = get_dependents(&path, &pkg_names); | |
| res.append(&mut r); | |
| } | |
| let affected: Vec<String> = res | |
| .into_iter() | |
| .filter(|(_dependent, dependency)| dependency == &pkg_name) | |
| .map(|(dependent, _)| dependent) | |
| .collect(); | |
| let result = cx.empty_object(); | |
| let js_array = JsArray::new(cx, affected.len() as u32); | |
| for (i, name) in affected.iter().enumerate() { | |
| let js_str = cx.string(name); | |
| js_array.set(cx, i as u32, js_str)?; | |
| } | |
| result.set(cx, pkg_name.as_str(), js_array)?; | |
| Ok(result) | |
| } |
🤖 Prompt for AI Agents
In `@src/lib.rs` around lines 51 - 73, The get_affected_pkg function currently
returns unit and only prints matches; change get_affected_pkg to a Neon
JS-returning signature (e.g., fn get_affected_pkg<'a>(cx: FunctionContext<'a>)
-> JsResult<'a, JsObject>), parse the pkg_name argument from the context,
collect dependent names by iterating res (built via get_dependents and
get_pkg_name) into a Vec<String>, then construct a JsArray of those names and a
JsObject mapping pkg_name -> that JsArray (similar to how dag builds and returns
its JsArray/JsObject) and return it via cx.null() or Ok(object) accordingly
using Neon APIs. Ensure you remove println! usage and return the created
JsObject so JS receives { packageName: [affected packages] }.
Owner
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
package.json (1)
5-18:⚠️ Potential issue | 🔴 Critical
"main"points tolib/index.js, which does not exist — update tolib/index.cjs.The
"main"field is currently broken. Older Node versions or tools that don't support the"exports"field will fail to resolve the module. Onlylib/index.cjsandlib/index.mjsexist. Change"main"to"lib/index.cjs"to ensure consistent behavior across all Node versions.
🤖 Fix all issues with AI agents
In `@CHANGELOG.md`:
- Around line 1-7: Swap the two release sections so the newest release heading
"## 0.3.0" appears above "## 0.2.1", and update the bullet under 0.3.0 to fix
the grammar by changing "add getAffectedPkg function which return the dependents
including direct, transitive and circular dependency" to use "returns" (e.g.,
"add getAffectedPkg function which returns the dependents including direct,
transitive and circular dependency").
In `@src/lib.rs`:
- Around line 51-59: The function get_dependents_recursively currently takes
ownership of pkg_name and res and re-clones res on each call and lacks a visited
guard, causing infinite recursion for circular dependency graphs; change its
signature to take &str or &String for pkg_name, take res by reference
(&Vec<(String,String)> or a slice) and add a visited HashSet<&str> (or
HashSet<String>) parameter plus a mutable dependents Vec parameter, check/infer
visited before recursing (mark pkg_name visited, skip if already present) and
remove all res.clone() uses—update all call sites to create and pass a mutable
visited set and the mutable dependents vector when invoking
get_dependents_recursively.
🧹 Nitpick comments (1)
src/lib.rs (1)
86-88: Dead code:dependentsVec is populated but never used.Lines 86–88 push values from
unique_dependentsintodependents, butdependentsis never read afterward. The iteration on line 90 usesunique_dependentsdirectly. Remove the unused variable and loop.Proposed cleanup
for pkg in duplicate_dependents { unique_dependents.insert(pkg); } - for pkg in unique_dependents.clone() { - dependents.push(pkg); - }
Comment on lines
1
to
+7
| ## 0.2.1 | ||
|
|
||
| ## 0.3.0 | ||
|
|
||
| ### Minor Changes | ||
|
|
||
| - add getAffectedPkg function which return the dependents including direct, transitive and circular dependency |
There was a problem hiding this comment.
Changelog is in wrong order — newest version should appear first.
Version 0.3.0 (lines 3–7) is placed below 0.2.1 (line 1). Changelogs conventionally list the most recent release at the top. Also, minor grammar nit: "return" → "returns".
Proposed fix
-## 0.2.1
-
## 0.3.0
### Minor Changes
-- add getAffectedPkg function which return the dependents including direct, transitive and circular dependency
+- add getAffectedPkg function which returns the dependents including direct, transitive and circular dependency
+
+## 0.2.1
### Patch Changes🤖 Prompt for AI Agents
In `@CHANGELOG.md` around lines 1 - 7, Swap the two release sections so the newest
release heading "## 0.3.0" appears above "## 0.2.1", and update the bullet under
0.3.0 to fix the grammar by changing "add getAffectedPkg function which return
the dependents including direct, transitive and circular dependency" to use
"returns" (e.g., "add getAffectedPkg function which returns the dependents
including direct, transitive and circular dependency").
Comment on lines
+51
to
+59
| fn get_dependents_recursively(pkg_name: String, res: Vec<(String, String)>, duplicate_dependents:&mut Vec<String>) { | ||
| for (_i, (dependent, dependency)) in res.clone().into_iter().enumerate() { | ||
| if dependency == pkg_name { | ||
| duplicate_dependents.push(dependent.clone()); | ||
| get_dependents_recursively(dependent.clone(), res.clone(),duplicate_dependents); | ||
| } | ||
| } | ||
| return; | ||
| } |
There was a problem hiding this comment.
Critical: infinite recursion on circular dependencies — will stack overflow.
The changelog explicitly mentions "circular dependency" support, yet there is no visited-set guard. If package A depends on B and B depends on A, this function recurses infinitely until the stack overflows.
Additionally, res.clone() on every recursive call copies the entire graph each time, which is unnecessarily expensive. Pass res by reference instead.
Proposed fix using a visited set
-fn get_dependents_recursively(pkg_name: String, res: Vec<(String, String)>, duplicate_dependents:&mut Vec<String>) {
- for (_i, (dependent, dependency)) in res.clone().into_iter().enumerate() {
- if dependency == pkg_name {
- duplicate_dependents.push(dependent.clone());
- get_dependents_recursively(dependent.clone(), res.clone(),duplicate_dependents);
- }
- }
- return;
-}
+fn get_dependents_recursively(
+ pkg_name: &str,
+ res: &[(String, String)],
+ visited: &mut HashSet<String>,
+ result: &mut Vec<String>,
+) {
+ for (dependent, dependency) in res.iter() {
+ if dependency == pkg_name && !visited.contains(dependent) {
+ visited.insert(dependent.clone());
+ result.push(dependent.clone());
+ get_dependents_recursively(dependent, res, visited, result);
+ }
+ }
+}Update the call site accordingly:
let mut visited = HashSet::new();
let mut dependents = Vec::new();
get_dependents_recursively(&pkg_name, &res, &mut visited, &mut dependents);🤖 Prompt for AI Agents
In `@src/lib.rs` around lines 51 - 59, The function get_dependents_recursively
currently takes ownership of pkg_name and res and re-clones res on each call and
lacks a visited guard, causing infinite recursion for circular dependency
graphs; change its signature to take &str or &String for pkg_name, take res by
reference (&Vec<(String,String)> or a slice) and add a visited HashSet<&str> (or
HashSet<String>) parameter plus a mutable dependents Vec parameter, check/infer
visited before recursing (mark pkg_name visited, skip if already present) and
remove all res.clone() uses—update all call sites to create and pass a mutable
visited set and the mutable dependents vector when invoking
get_dependents_recursively.
Updated usage examples and added contribution section.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@README.md`:
- Line 16: The README header "### CommonJs" uses nonstandard casing; update the
header text (the line containing "### CommonJs") to use the standard "CommonJS"
spelling so it reads "### CommonJS" for consistency with Node.js docs and
project style.
- Around line 16-17: Add language identifiers to the fenced code blocks in
README.md so they satisfy MD040 and improve syntax highlighting: change the
CommonJs and ESM blocks containing "const { dag, getAffectedPkg } =
require('dag-rs');" and "import { dag, getAffectedPkg } from 'dag-rs';" to use
```js, and change the comment/example blocks containing "// dependents of
`@rocket.chat/fuselage` package" and "// dependent : dependency" to use ```text;
apply the same conversions to the other affected blocks referenced (lines around
the other snippets).
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
1 participant
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.
resolve #4
Summary by CodeRabbit
New Features
Documentation
Chores