Skip to content

fix: catch all errors that can occur during plugin installation and log the error#7375

Open
SamTV12345 wants to merge 1 commit intodevelopfrom
7365-uncaught-exception-causing-crash-during-startup-when-not-connected-to-the-internet
Open

fix: catch all errors that can occur during plugin installation and log the error#7375
SamTV12345 wants to merge 1 commit intodevelopfrom
7365-uncaught-exception-causing-crash-during-startup-when-not-connected-to-the-internet

Conversation

@SamTV12345
Copy link
Member

No description provided.

@qodo-free-for-open-source-projects

Review Summary by Qodo

Catch and log plugin installation errors to prevent startup crashes

🐞 Bug fix

Grey Divider

Walkthroughs

Description
• Wrap plugin installation in try-catch to prevent crashes
• Log errors during plugin installation for debugging
• Remove unnecessary npmrc configuration setting
Diagram
flowchart LR
  A["Plugin Installation Loop"] --> B["Try Block"]
  B --> C["Install Plugin"]
  C --> D{Success?}
  D -->|Yes| E["Continue"]
  D -->|No| F["Catch Error"]
  F --> G["Log Error Message"]
  G --> E
Loading

Grey Divider

File Changes

1. src/static/js/pluginfw/installer.ts 🐞 Bug fix +5/-1

Add error handling for plugin installation

• Wrapped linkInstaller.installPlugin() call in try-catch block
• Added error logging with plugin name and version details
• Prevents application crash when plugin installation fails

src/static/js/pluginfw/installer.ts


2. src/.npmrc ⚙️ Configuration changes +0/-1

Remove npmrc auto-install-peers setting

• Removed auto-install-peers=false configuration setting

src/.npmrc


Grey Divider

Qodo Logo

@qodo-free-for-open-source-projects
Copy link

qodo-free-for-open-source-projects bot commented Mar 16, 2026

Code Review by Qodo

🐞 Bugs (1) 📘 Rule violations (0) 📎 Requirement gaps (1)

Grey Divider


Remediation recommended

1. logger.error used, not warn 📎 Requirement gap ✧ Quality
Description
The new exception handling logs at error level, but the compliance criteria calls for logging a
warning when plugin installation fails so startup can continue using already-installed plugins.
Aligning the log level with the requirement improves expected operator signaling and compliance
consistency.
Code

src/static/js/pluginfw/installer.ts[R127-131]

+      try {
+        await linkInstaller.installPlugin(plugin.name, plugin.version);
+      } catch (e) {
+        logger.error(`Error installing plugin ${plugin.name} with version ${plugin.version}: ${e}`);
+      }
Evidence
PR Compliance ID 1 explicitly requires that failures reaching external plugin sources during startup
be caught and that a warning is logged while execution continues. The added code catches the
exception but logs via logger.error(...) rather than a warning.

Startup must not crash if plugin source is unreachable (handle installer exceptions)
src/static/js/pluginfw/installer.ts[125-132]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The new error handling for plugin installation failures logs using `logger.error(...)`, but the compliance requirement specifies logging a warning when external plugin sources are unreachable during startup.

## Issue Context
This runs during startup and failures should be handled without crashing, with a warning emitted to indicate degraded behavior while continuing to use already-installed plugins.

## Fix Focus Areas
- src/static/js/pluginfw/installer.ts[127-131]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


2. Error stack not logged 🐞 Bug ✧ Quality
Description
checkForMigration() catches installPlugin() failures but logs the caught value via template-string
interpolation, which typically omits the stack trace and can stringify to "[object Object]". This
significantly reduces the usefulness of the new error logging when plugin installation fails at
startup.
Code

src/static/js/pluginfw/installer.ts[R127-131]

+      try {
+        await linkInstaller.installPlugin(plugin.name, plugin.version);
+      } catch (e) {
+        logger.error(`Error installing plugin ${plugin.name} with version ${plugin.version}: ${e}`);
+      }
Evidence
The new code logs the caught exception via ${e}, which coerces the thrown value to a string and
usually drops stack traces. Elsewhere in the codebase, errors are logged with err.stack || err or
by passing the Error object as a separate argument to logger.error, indicating stack traces are
expected when available.

src/static/js/pluginfw/installer.ts[125-132]
src/static/js/pluginfw/plugins.ts[18-26]
src/node/server.ts[139-146]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`checkForMigration()` catches plugin installation errors but logs them using template-string interpolation (`${e}`), which typically loses the stack trace. This makes startup plugin install failures hard to diagnose.

### Issue Context
Other parts of the codebase log errors as `err.stack || err` or pass the Error object as a separate argument to `logger.error`, which preserves stack traces.

### Fix Focus Areas
- src/static/js/pluginfw/installer.ts[125-132]

### Suggested change
Update the catch block to pass the error object to log4js (and/or include stack):

```ts
} catch (e) {
 const err = e as any;
 logger.error(
   `Error installing plugin ${plugin.name} with version ${plugin.version}`,
   err
 );
 // Alternatively if you prefer single-argument messages:
 // logger.error(`Error installing plugin ${plugin.name}@${plugin.version}: ${err?.stack || err}`);
}
```

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

ⓘ The new review experience is currently in Beta. Learn more

Grey Divider

Qodo Logo

@JohnMcLear
Copy link
Member

You delete .npmrc for some reason? Is your AI haunted?

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Uncaught exception causing crash during startup when not connected to the Internet

2 participants