Fix/plugin metadata repo type guard#8207
Conversation
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- In the frontend, consider extracting a single helper like
normalizeRepoKey(repo)that wrapsnormalizeInstallUrl,safeRepoStr, and.toLowerCase()so the repo normalization logic isn’t repeated at multiple call sites and is less error-prone to change. - When building
installedReposandinstalledByRepo, you currently allowundefinedkeys into theSet/Map; it would be cleaner to filter out entries with falsyrepovalues before constructing these collections to avoid unnecessaryundefinedentries.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In the frontend, consider extracting a single helper like `normalizeRepoKey(repo)` that wraps `normalizeInstallUrl`, `safeRepoStr`, and `.toLowerCase()` so the repo normalization logic isn’t repeated at multiple call sites and is less error-prone to change.
- When building `installedRepos` and `installedByRepo`, you currently allow `undefined` keys into the `Set`/`Map`; it would be cleaner to filter out entries with falsy `repo` values before constructing these collections to avoid unnecessary `undefined` entries.
## Individual Comments
### Comment 1
<location path="dashboard/src/views/extension/useExtensionPage.js" line_range="1239" />
<code_context>
const checkAlreadyInstalled = () => {
const data = Array.isArray(extension_data?.data) ? extension_data.data : [];
- const installedRepos = new Set(data.map((ext) => ext.repo?.toLowerCase()));
+ const installedRepos = new Set(data.map((ext) => ext.repo ? safeRepoStr(ext.repo).toLowerCase() : undefined));
const installedNames = new Set(
data.map((ext) => normalizeStr(ext.name).replace(/_/g, "-")),
</code_context>
<issue_to_address>
**issue (bug_risk):** Avoid inserting `undefined` into `installedRepos`, which can cause false positives for plugins without `repo`.
With this mapping, any `ext` without a `repo` adds `undefined` to `installedRepos`, so `installedRepos.has(plugin.repo ? safeRepoStr(plugin.repo).toLowerCase() : undefined)` will return true for any plugin lacking `repo` as soon as a single extension is missing `repo`. This can incorrectly mark unrelated plugins as installed. Instead, skip entries without `repo` when building the set, e.g.:
```js
const installedRepos = new Set(
data
.filter((ext) => ext.repo)
.map((ext) => safeRepoStr(ext.repo).toLowerCase()),
);
```
and only call `.has(...)` when `plugin.repo` is truthy (or otherwise avoid using `undefined` as a key).
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
There was a problem hiding this comment.
Code Review
This pull request ensures that plugin repository fields are consistently treated as strings in both the Python backend and the Vue frontend to prevent runtime errors during string operations like .toLowerCase(). In the backend, plugin.repo is now explicitly cast to a string, while the frontend introduces a safeRepoStr helper. Feedback suggests that safeRepoStr is redundant because the existing normalizeInstallUrl function already handles string casting and trimming. Furthermore, it is recommended to use normalizeInstallUrl throughout the frontend logic to ensure consistent repository matching, as it correctly handles trailing slash normalization which safeRepoStr lacks.
|
@sourcery-ai review |
|
/gemini review |
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- The frontend now repeatedly calls
normalizeInstallUrl(...).toLowerCase()in several places; consider extracting a small helper (e.g.safeRepoKey(value)) to centralize the normalization logic and reduce the risk of inconsistent transformations across call sites. - On the backend,
repois coerced withstr(plugin.repo)whileNonebecomes an empty string; ifrepocan be an invalid type, you may want to validate and either omit the field or return a sentinel value instead of silently stringifying arbitrary types (e.g. dicts), which could make debugging misconfigured metadata harder.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The frontend now repeatedly calls `normalizeInstallUrl(...).toLowerCase()` in several places; consider extracting a small helper (e.g. `safeRepoKey(value)`) to centralize the normalization logic and reduce the risk of inconsistent transformations across call sites.
- On the backend, `repo` is coerced with `str(plugin.repo)` while `None` becomes an empty string; if `repo` can be an invalid type, you may want to validate and either omit the field or return a sentinel value instead of silently stringifying arbitrary types (e.g. dicts), which could make debugging misconfigured metadata harder.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
There was a problem hiding this comment.
Code Review
This pull request ensures that plugin repository identifiers are consistently handled as strings in the backend and normalized using normalizeInstallUrl in the frontend for reliable comparisons. Feedback suggests centralizing the string conversion in the backend metadata model to avoid repetitive casting, creating a frontend helper function to reduce duplication of the normalization logic, and optimizing data structure initialization in checkAlreadyInstalled to avoid redundant iterations.
| _t = { | ||
| "name": plugin.name, | ||
| "repo": "" if plugin.repo is None else plugin.repo, | ||
| "repo": "" if plugin.repo is None else str(plugin.repo), |
There was a problem hiding this comment.
While this fix addresses the serialization in get_plugins, the repo field is also returned by other endpoints like install_plugin and install_plugin_from_file. To ensure consistency and prevent similar issues elsewhere, consider casting repo to a string when it is first loaded in StarMetadata (within star_manager.py) or ensuring all serialization points are covered.
References
- When implementing similar functionality for different cases, refactor the logic into a shared helper function or central location to avoid code duplication and ensure consistency.
Fix #8206: When a plugin's metadata.yaml has a non-string repo field (e.g. integer 111), the backend only checks for None and passes the value through as-is. This causes the frontend to crash with TypeError: Pn.toLowerCase is not a function when calling .toLowerCase() in checkUpdate() and checkAlreadyInstalled(), displaying an error message on the plugin page when reloading any plugin.
Modifications / 改动点
astrbot/dashboard/routes/plugin.py: In get_plugins and get_plugin_detail, wrap the repo field with str() when serializing, ensuring the frontend always receives a string.dashboard/src/views/extension/useExtensionPage.js: Add a safeRepoStr() helper function and apply it to all 6 repo.toLowerCase() call sites in findMarketPluginForExtension, checkUpdate, and checkAlreadyInstalled to guard against non-string types.Screenshots or Test Results / 运行截图或测试结果
After setting repo to a non-string value (e.g. repo: 111) in a plugin's metadata.yaml, reloading the plugin no longer triggers the TypeError. The plugin reloads successfully without any error message.
Checklist / 检查清单
😊 If there are new features added in the PR, I have discussed it with the authors through issues/emails, etc.
/ 如果 PR 中有新加入的功能,已经通过 Issue / 邮件等方式和作者讨论过。
👀 My changes have been well-tested, and "Verification Steps" and "Screenshots" have been provided above.
/ 我的更改经过了良好的测试,并已在上方提供了“验证步骤”和“运行截图”。
🤓 I have ensured that no new dependencies are introduced, OR if new dependencies are introduced, they have been added to the appropriate locations in
requirements.txtandpyproject.toml./ 我确保没有引入新依赖库,或者引入了新依赖库的同时将其添加到
requirements.txt和pyproject.toml文件相应位置。😮 My changes do not introduce malicious code.
/ 我的更改没有引入恶意代码。
Summary by Sourcery
Ensure plugin repository metadata is consistently treated as a normalized string across backend responses and frontend extension matching to prevent runtime errors and improve installed/market plugin reconciliation.
Bug Fixes:
Enhancements: