Conversation
📝 WalkthroughWalkthroughAdds a new Dart CLI for the Nostr Development Kit: an entrypoint, a command interface and orchestrator, a "req" command implementation for querying relays, pubspec executables/dev_dependency updates, and a .gitignore entry for a local DB file. Changes
Sequence DiagramsequenceDiagram
participant User
participant NdkCliApp
participant ReqCliCommand
participant Ndk
participant Relay
User->>NdkCliApp: run(['req', ...])
activate NdkCliApp
NdkCliApp->>NdkCliApp: parse args, find command
NdkCliApp->>Ndk: _createNdk()
activate Ndk
NdkCliApp->>ReqCliCommand: run(remainingArgs, ndk)
activate ReqCliCommand
ReqCliCommand->>ReqCliCommand: parse args, build Filter
ReqCliCommand->>Ndk: requests.query(filter)
Ndk->>Relay: open subscription / query
Relay-->>Ndk: event stream
Ndk-->>ReqCliCommand: events
ReqCliCommand->>User: print events as JSON
ReqCliCommand-->>NdkCliApp: return exitCode
deactivate ReqCliCommand
NdkCliApp->>Ndk: destroy()
deactivate Ndk
NdkCliApp-->>User: exit with code / print help or errors
deactivate NdkCliApp
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (7)
.gitignore (1)
40-40: Consider whether a pattern-based ignore would be more robust.While the current specific path works well, you might want to consider whether other database files should also be ignored. For example:
*.db- to ignore all database files/packages/ndk/*.db- to ignore all database files in this directory**/wallets_db.db- to ignore this file in any locationHowever, if only this specific wallet database file should be ignored (and other
.dbfiles might legitimately be committed), then the current approach is correct.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.gitignore at line 40, The .gitignore currently lists the specific path /packages/ndk/wallets_db.db; decide whether to broaden it and replace or add a pattern-based rule: if you want to ignore all DB files globally use *.db, to ignore all DBs in the ndk package use /packages/ndk/*.db, or to ignore this filename anywhere use **/wallets_db.db; otherwise keep the exact entry if other .db files must remain tracked. Update the .gitignore accordingly by adding the chosen pattern (or leaving the specific path) to reflect the desired scope.packages/ndk/lib/src/cli/req_cli_command.dart (1)
151-157: Consider documenting the wss:// auto-prefix behavior.The
_parseRelaymethod silently addswss://if the URL doesn't validate directly. While user-friendly, this implicit behavior might be unexpected. Consider either:
- Documenting it in the usage/help text, or
- Printing a message when auto-prefixing occurs
This is a minor UX consideration.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ndk/lib/src/cli/req_cli_command.dart` around lines 151 - 157, The _parseRelay function silently auto-prefixes input with "wss://" by calling cleanRelayUrl('wss://$value') when the direct cleanRelayUrl(value) returns null; make this behavior visible by either adding documentation to the CLI help/usage text describing the automatic wss:// prefix or by emitting a short informational message when auto-prefixing occurs (e.g., write to stderr or the CLI logger). Update the CLI help string(s) and/or modify _parseRelay to call the existing logging/printing facility when it falls back to 'wss://$value', referencing _parseRelay and cleanRelayUrl so reviewers can find the change.packages/ndk/lib/src/cli/ndk_cli_app.dart (2)
73-83: Redundant log level configuration.
LogLevel.erroris set twice: once viaLogger.setLogLevel()(line 74) and again inNdkConfig.logLevel(line 80). Consider removing one to avoid confusion about which takes precedence.🔧 Suggested fix - remove duplicate
Ndk _createNdk() { - Logger.setLogLevel(LogLevel.error); return Ndk( NdkConfig( cache: MemCacheManager(), eventVerifier: Bip340EventVerifier(), bootstrapRelays: const [], logLevel: LogLevel.error, ), ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ndk/lib/src/cli/ndk_cli_app.dart` around lines 73 - 83, The log level is being set twice in _createNdk (via Logger.setLogLevel(LogLevel.error) and NdkConfig(logLevel: LogLevel.error)); remove the redundant setting to avoid ambiguity — delete the Logger.setLogLevel(LogLevel.error) call inside _createNdk and rely on the NdkConfig.logLevel property (or alternatively remove the NdkConfig.logLevel if you prefer global Logger control), updating _createNdk accordingly while keeping NdkConfig, MemCacheManager, and Bip340EventVerifier usage unchanged.
49-57: UseappNameinstead of hardcoded "ndk" for consistency.The constructor accepts
appNameas a parameter, but the help text hardcodes "ndk". This creates an inconsistency if the app is instantiated with a different name.🔧 Suggested fix
void printHelp([IOSink? out]) { out ??= stdout; out.writeln('$appName - $description'); - out.writeln('Usage: ndk <command> [args]'); + out.writeln('Usage: $appName <command> [args]'); out.writeln(''); out.writeln('Commands:'); for (final command in commands) { out.writeln(' ${command.name.padRight(12)} ${command.description}'); } out.writeln(' help Show this help'); out.writeln(''); - out.writeln('Use "ndk <command> --help" for command details.'); + out.writeln('Use "$appName <command> --help" for command details.'); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ndk/lib/src/cli/ndk_cli_app.dart` around lines 49 - 57, The help text is hardcoded to "ndk" instead of using the constructor parameter appName; update the out.writeln calls that produce the Usage and help example lines to interpolate appName (e.g., 'Usage: $appName <command> [args]' and 'Use "$appName <command> --help" for command details.') so the output reflects the provided appName; modify the occurrences in the block that iterates commands (uses out.writeln and commands) to use appName interpolation.packages/ndk/bin/ndk.dart (2)
15-19: Redundant error message on failure.When
exitCode != 0,NdkCliApp.run()already writes error details to stderr. The additional message here ("ndk CLI failed with exit code...") is redundant and doesn't add useful context.🔧 Suggested simplification
final exitCode = await app.run(args); if (exitCode != 0) { - stderr.writeln('ndk CLI failed with exit code $exitCode'); exit(exitCode); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ndk/bin/ndk.dart` around lines 15 - 19, The extra stderr.writeln call is redundant because NdkCliApp.run() already prints errors to stderr; update the failure branch that checks exitCode returned from app.run(args) to remove the stderr.writeln('ndk CLI failed with exit code $exitCode') line and simply call exit(exitCode) when exitCode != 0 (i.e., keep the exit(exitCode) but drop the additional message in the block that handles the non‑zero exit code for app.run).
3-4: Consider exporting CLI classes from a public barrel file.Importing directly from
package:ndk/src/cli/...exposes internal implementation paths. If these are intended to be public APIs, export them from a barrel file (e.g.,package:ndk/cli.dartor add topackage:ndk/ndk.dart).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ndk/bin/ndk.dart` around lines 3 - 4, Replace direct internal imports with a public barrel: create/update a public export file (e.g., package:ndk/cli.dart or add exports to package:ndk/ndk.dart) that exports the CLI symbols from src/cli (export 'src/cli/ndk_cli_app.dart'; export 'src/cli/req_cli_command.dart';), then update ndk.dart to import from the new public barrel (import 'package:ndk/cli.dart';) instead of importing package:ndk/src/cli/ndk_cli_app.dart and package:ndk/src/cli/req_cli_command.dart; ensure the exported symbols (NDKCliApp, ReqCliCommand or whatever public class names are declared) are re-exported by the barrel.packages/ndk/pubspec.yaml (1)
51-51: Consider pinning thetestdependency version.Using
anyfor thetestpackage allows any version, which could introduce breaking changes unexpectedly. Consider specifying a version constraint like^1.25.0for more predictable builds.🔧 Suggested fix
- test: any + test: ^1.25.0🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ndk/pubspec.yaml` at line 51, The pubspec currently uses an unpinned test dependency ("test: any"); update the test dependency entry in packages/ndk/pubspec.yaml to a specific version constraint (for example change "test: any" to a caret-constrained version like "^1.25.0") to avoid pulling breaking changes, then run pub get to lock the dependency; look for the "test" dependency key in the file when making this change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In @.gitignore:
- Line 40: The .gitignore currently lists the specific path
/packages/ndk/wallets_db.db; decide whether to broaden it and replace or add a
pattern-based rule: if you want to ignore all DB files globally use *.db, to
ignore all DBs in the ndk package use /packages/ndk/*.db, or to ignore this
filename anywhere use **/wallets_db.db; otherwise keep the exact entry if other
.db files must remain tracked. Update the .gitignore accordingly by adding the
chosen pattern (or leaving the specific path) to reflect the desired scope.
In `@packages/ndk/bin/ndk.dart`:
- Around line 15-19: The extra stderr.writeln call is redundant because
NdkCliApp.run() already prints errors to stderr; update the failure branch that
checks exitCode returned from app.run(args) to remove the stderr.writeln('ndk
CLI failed with exit code $exitCode') line and simply call exit(exitCode) when
exitCode != 0 (i.e., keep the exit(exitCode) but drop the additional message in
the block that handles the non‑zero exit code for app.run).
- Around line 3-4: Replace direct internal imports with a public barrel:
create/update a public export file (e.g., package:ndk/cli.dart or add exports to
package:ndk/ndk.dart) that exports the CLI symbols from src/cli (export
'src/cli/ndk_cli_app.dart'; export 'src/cli/req_cli_command.dart';), then update
ndk.dart to import from the new public barrel (import 'package:ndk/cli.dart';)
instead of importing package:ndk/src/cli/ndk_cli_app.dart and
package:ndk/src/cli/req_cli_command.dart; ensure the exported symbols
(NDKCliApp, ReqCliCommand or whatever public class names are declared) are
re-exported by the barrel.
In `@packages/ndk/lib/src/cli/ndk_cli_app.dart`:
- Around line 73-83: The log level is being set twice in _createNdk (via
Logger.setLogLevel(LogLevel.error) and NdkConfig(logLevel: LogLevel.error));
remove the redundant setting to avoid ambiguity — delete the
Logger.setLogLevel(LogLevel.error) call inside _createNdk and rely on the
NdkConfig.logLevel property (or alternatively remove the NdkConfig.logLevel if
you prefer global Logger control), updating _createNdk accordingly while keeping
NdkConfig, MemCacheManager, and Bip340EventVerifier usage unchanged.
- Around line 49-57: The help text is hardcoded to "ndk" instead of using the
constructor parameter appName; update the out.writeln calls that produce the
Usage and help example lines to interpolate appName (e.g., 'Usage: $appName
<command> [args]' and 'Use "$appName <command> --help" for command details.') so
the output reflects the provided appName; modify the occurrences in the block
that iterates commands (uses out.writeln and commands) to use appName
interpolation.
In `@packages/ndk/lib/src/cli/req_cli_command.dart`:
- Around line 151-157: The _parseRelay function silently auto-prefixes input
with "wss://" by calling cleanRelayUrl('wss://$value') when the direct
cleanRelayUrl(value) returns null; make this behavior visible by either adding
documentation to the CLI help/usage text describing the automatic wss:// prefix
or by emitting a short informational message when auto-prefixing occurs (e.g.,
write to stderr or the CLI logger). Update the CLI help string(s) and/or modify
_parseRelay to call the existing logging/printing facility when it falls back to
'wss://$value', referencing _parseRelay and cleanRelayUrl so reviewers can find
the change.
In `@packages/ndk/pubspec.yaml`:
- Line 51: The pubspec currently uses an unpinned test dependency ("test: any");
update the test dependency entry in packages/ndk/pubspec.yaml to a specific
version constraint (for example change "test: any" to a caret-constrained
version like "^1.25.0") to avoid pulling breaking changes, then run pub get to
lock the dependency; look for the "test" dependency key in the file when making
this change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2d3b3d04-2266-4a41-8833-f15115f50ac0
📒 Files selected for processing (6)
.gitignorepackages/ndk/bin/ndk.dartpackages/ndk/lib/src/cli/cli_command.dartpackages/ndk/lib/src/cli/ndk_cli_app.dartpackages/ndk/lib/src/cli/req_cli_command.dartpackages/ndk/pubspec.yaml
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/ndk/bin/ndk.dart (1)
6-19: Consider wrapping in try-catch for user-friendly error output.If
app.run(args)throws an unhandled exception, users will see a raw stack trace. For a CLI tool—especially one designed for agent use—graceful error handling improves UX.♻️ Proposed improvement
Future<void> main(List<String> args) async { final app = NdkCliApp( appName: 'ndk', description: 'Nostr Development Kit command line interface', commands: [ ReqCliCommand(), ], ); - final exitCode = await app.run(args); - if (exitCode != 0) { - exit(exitCode); + try { + final exitCode = await app.run(args); + if (exitCode != 0) { + exit(exitCode); + } + } catch (e) { + stderr.writeln('Error: $e'); + exit(1); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ndk/bin/ndk.dart` around lines 6 - 19, Wrap the main function's call to app.run(args) in a try-catch so uncaught exceptions produce a clean, user-friendly message and non-zero exit code; specifically, update main (which constructs NdkCliApp and calls app.run) to catch Exception/Throwable around await app.run(args) and use stderr or the CLI's logger to print a concise error message (including optional verbose/stack when a debug flag is set), then exit with a non-zero exit code if an exception occurred.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/ndk/bin/ndk.dart`:
- Around line 6-19: Wrap the main function's call to app.run(args) in a
try-catch so uncaught exceptions produce a clean, user-friendly message and
non-zero exit code; specifically, update main (which constructs NdkCliApp and
calls app.run) to catch Exception/Throwable around await app.run(args) and use
stderr or the CLI's logger to print a concise error message (including optional
verbose/stack when a debug flag is set), then exit with a non-zero exit code if
an exception occurred.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: be5f1aeb-3ef0-4884-8bca-3d4796dd7730
📒 Files selected for processing (2)
packages/ndk/bin/ndk.dartpackages/ndk/lib/src/cli/req_cli_command.dart
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/ndk/lib/src/cli/req_cli_command.dart
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #495 +/- ##
==========================================
- Coverage 76.74% 76.65% -0.10%
==========================================
Files 153 153
Lines 6407 6407
==========================================
- Hits 4917 4911 -6
- Misses 1490 1496 +6 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Merged
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.
A command line client for tool operations, convenient for agents to access using skills.
fixes #491
Summary by CodeRabbit
New Features
Chores