Skip to content

fix: wallet remove cmd aciton with global error handler#28

Open
Drincann wants to merge 1 commit intomainfrom
fix/remove-cmd-error-handler
Open

fix: wallet remove cmd aciton with global error handler#28
Drincann wants to merge 1 commit intomainfrom
fix/remove-cmd-error-handler

Conversation

@Drincann
Copy link
Copy Markdown
Owner

@Drincann Drincann commented Apr 5, 2026

PR Type

Bug fix


Description

  • Use global CLI error handling

  • Wrap wallet remove action safely

  • Preserve confirmation and deletion flow


Diagram Walkthrough

flowchart LR
  remove["wallet remove command"]
  handler["global withErrorHandler wrapper"]
  flow["remove wallet execution"]
  output["CLI error or success output"]
  remove -- "wraps action with" --> handler
  handler -- "executes" --> flow
  flow -- "reports through" --> output
Loading

File Walkthrough

Relevant files
Bug fix
remove.mts
Apply global error handling to wallet removal                       

src/cli/commands/wallet/remove.mts

  • Import withErrorHandler for command execution
  • Wrap the .action(...) callback globally
  • Keep wallet lookup, confirmation, and removal logic
  • Rely on centralized error handling instead of local catch
+19/-24 

@github-actions
Copy link
Copy Markdown

github-actions bot commented Apr 5, 2026

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 1 🔵⚪⚪⚪⚪
🧪 No relevant tests
🔒 No security concerns identified
⚡ No major issues detected

@github-actions
Copy link
Copy Markdown

github-actions bot commented Apr 5, 2026

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Throw on missing wallet

Don't just print the error and return here. Because the command is now wrapped in
withErrorHandler, this path should throw a CliError so the CLI exits with a failure
status instead of reporting success to calling scripts.

src/cli/commands/wallet/remove.mts [19-22]

 if (walletData === undefined) {
-  printer.error(`Wallet with alias '${walletAlias}' not found`);
-  return;
+  throw new CliError(`Wallet with alias '${walletAlias}' not found`);
 }
Suggestion importance[1-10]: 7

__

Why: This correctly identifies that returning after printer.error(...) can leave the command appearing successful, while throwing new CliError(...) would let withErrorHandler handle it consistently as a failure. It is a meaningful correctness improvement for CLI behavior, though not a critical bug.

Medium

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant