Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces a "Pin Updates on Top" feature for the installed plugins list, allowing users to prioritize plugins with available updates. The changes include new internationalization keys across multiple locales, a UI toggle switch in the installed plugins tab, and updated sorting logic to handle the pinning behavior. The review feedback identifies a missing variable destructuring in the Vue component that would cause a runtime error, and provides suggestions for persisting the user's preference in local storage and adjusting UI constraints to prevent text truncation in different languages.
faf411f to
0068960
Compare
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The
pinUpdatesOnToplocalStorage key is hard-coded in multiple places; consider extracting it to a shared constant (and optionally namespacing it) to avoid typos and make future refactors safer. - Access to
localStorageis guarded by awindowcheck but not by a try/catch; in some environments (e.g., strict privacy modes) calls can still throw, so consider wrapping the get/set calls to fail gracefully without breaking the page.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `pinUpdatesOnTop` localStorage key is hard-coded in multiple places; consider extracting it to a shared constant (and optionally namespacing it) to avoid typos and make future refactors safer.
- Access to `localStorage` is guarded by a `window` check but not by a try/catch; in some environments (e.g., strict privacy modes) calls can still throw, so consider wrapping the get/set calls to fail gracefully without breaking the page.
## Individual Comments
### Comment 1
<location path="dashboard/src/views/extension/useExtensionPage.js" line_range="191-193" />
<code_context>
const installedStatusFilter = ref("all");
const installedSortBy = ref("default");
const installedSortOrder = ref("desc");
+ const getInitialPinUpdatesOnTop = () => {
+ if (typeof window !== "undefined" && window.localStorage) {
+ const saved = localStorage.getItem("pinUpdatesOnTop");
+ return saved !== "false";
+ }
</code_context>
<issue_to_address>
**issue:** Accessing localStorage without try/catch can throw in some environments (e.g. Safari private mode), which can break hook initialization.
`localStorage.getItem`/`setItem` can still throw (e.g. `SecurityError`) even when `window` and `localStorage` exist. Please wrap these calls in try/catch for both reads and writes, and fall back to the default `true` when storage access fails so the hook doesn’t break in restricted environments.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The
pinUpdatesOnTopsorting block runs before any other sort criteria but ignoresinstalledSortOrder; if users switch to ascending order they may still expect updated plugins to be grouped at the top but otherwise respect the chosen order, so it may be worth confirming or documenting this behavior in code (e.g., a short comment near the comparator). - In
extensionPreferenceStorage.mjs,resolveStoragetreats an explicitly passednulldifferently fromundefined(the former bypasses the resolution logic, the latter triggers it); if consumers might passnullas “no storage,” consider handling that case explicitly or tightening the function’s JSDoc/type hints to avoid accidental misuse.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `pinUpdatesOnTop` sorting block runs before any other sort criteria but ignores `installedSortOrder`; if users switch to ascending order they may still expect updated plugins to be grouped at the top but otherwise respect the chosen order, so it may be worth confirming or documenting this behavior in code (e.g., a short comment near the comparator).
- In `extensionPreferenceStorage.mjs`, `resolveStorage` treats an explicitly passed `null` differently from `undefined` (the former bypasses the resolution logic, the latter triggers it); if consumers might pass `null` as “no storage,” consider handling that case explicitly or tightening the function’s JSDoc/type hints to avoid accidental misuse.
## Individual Comments
### Comment 1
<location path="dashboard/src/views/extension/useExtensionPage.js" line_range="448-449" />
<code_context>
const fallbackResult =
fallbackNameCompare !== 0 ? fallbackNameCompare : left.index - right.index;
+ if (
</code_context>
<issue_to_address>
**suggestion (performance):** Defer `fallbackResult` computation until after the `pinUpdatesOnTop` block to avoid unnecessary work on early returns.
With the new early return, `fallbackResult` can be computed and then discarded. Please move its computation down to just before the branches that use it to avoid extra work on this hot path.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- The
sortcomparator inuseExtensionPagenow allocates agetFallbackResultclosure on every comparison; consider hoisting the fallback comparison logic (or precomputed values) out of the comparator or inlining it to avoid per-item function creation and improve readability. - In
extensionPreferenceStorage.mjs,resolveStoragecurrently returns any non-nullishstorageargument without validation; consider narrowing this to aStorage-like interface (or adding a simple shape check) so accidental non-storage values fail fast instead of causing subtle runtime issues whengetItem/setItemare called.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `sort` comparator in `useExtensionPage` now allocates a `getFallbackResult` closure on every comparison; consider hoisting the fallback comparison logic (or precomputed values) out of the comparator or inlining it to avoid per-item function creation and improve readability.
- In `extensionPreferenceStorage.mjs`, `resolveStorage` currently returns any non-nullish `storage` argument without validation; consider narrowing this to a `Storage`-like interface (or adding a simple shape check) so accidental non-storage values fail fast instead of causing subtle runtime issues when `getItem`/`setItem` are called.
## Individual Comments
### Comment 1
<location path="dashboard/src/views/extension/useExtensionPage.js" line_range="443" />
<code_context>
installedAtTimestamp: getInstalledAtTimestamp(plugin),
}))
.sort((left, right) => {
- const fallbackNameCompare = compareInstalledPluginNames(
- left.plugin,
</code_context>
<issue_to_address>
**issue (complexity):** Consider extracting shared comparator logic and update-pinning into small helper functions to simplify and clarify the sort comparator’s control flow.
You can simplify the comparator noticeably by pulling the shared pieces into small helpers and reusing them. That keeps the new feature, but makes the flow linear and easier to scan.
Also, the current condition:
```js
if (
!pinUpdatesOnTop.value &&
installedSortBy.value !== "update_status"
) {
// ...
}
```
seems inverted compared to the comment “Pinning updates is a primary grouping…”. It likely should be `pinUpdatesOnTop.value && …` (without the leading `!`).
### 1. Extract shared fallback comparator
Instead of redefining `getFallbackResult` per call, move it out as a pure helper:
```js
const compareInstalledFallback = (left, right) => {
const nameCompare = compareInstalledPluginNames(left.plugin, right.plugin);
if (nameCompare !== 0) return nameCompare;
return left.index - right.index;
};
```
Then in `sortInstalledPlugins`:
```js
.sort((left, right) => {
// ... pinning logic ...
if (installedSortBy.value === "install_time") {
// ...
return timeDiff !== 0 ? timeDiff : compareInstalledFallback(left, right);
}
if (installedSortBy.value === "author") {
// ...
return authorCompare !== 0
? (installedSortOrder.value === "desc" ? -authorCompare : authorCompare)
: compareInstalledFallback(left, right);
}
if (installedSortBy.value === "update_status") {
// ...
return updateDiff !== 0 ? updateDiff : compareInstalledFallback(left, right);
}
return compareInstalledFallback(left, right);
});
```
This removes the inner closure, reduces visual noise, and makes the branches symmetrical.
### 2. Extract update‑pinning helper
You can keep the “pin updates” concern clearly separated from the rest of the comparator:
```js
const compareUpdatePinning = (left, right) => {
const leftHasUpdate = left.plugin?.has_update ? 1 : 0;
const rightHasUpdate = right.plugin?.has_update ? 1 : 0;
if (leftHasUpdate === rightHasUpdate) return 0;
return rightHasUpdate - leftHasUpdate; // updates first
};
```
Then at the top of the sort callback:
```js
.sort((left, right) => {
if (pinUpdatesOnTop.value && installedSortBy.value !== "update_status") {
const pinCompare = compareUpdatePinning(left, right);
if (pinCompare !== 0) return pinCompare;
}
// existing installedSortBy branches using compareInstalledFallback(...)
});
```
This way the comparator reads as:
1. optional primary grouping by update pinning,
2. primary sort by chosen field,
3. shared fallback comparator,
without mixing those concerns inline.
</issue_to_address>
### Comment 2
<location path="dashboard/src/views/extension/extensionPreferenceStorage.mjs" line_range="9" />
<code_context>
+ * Resolve the storage backend for preference helpers.
+ * Pass `null` to explicitly disable storage access in callers/tests.
+ */
+const resolveStorage = (storage) => {
+ if (storage === null) {
+ return null;
</code_context>
<issue_to_address>
**issue (complexity):** Consider simplifying the storage helper API to a binary override-or-localStorage contract and let the boolean helpers focus solely on preference logic.
The main complexity comes from the tri‑state `resolveStorage` API (`undefined`/`null`/custom) and threading that through all helpers. You can keep all current functionality (including injection/override and safe fallback) with a simpler, binary contract:
- `storageOverride` provided → use it.
- Otherwise → try `window.localStorage`, fall back to `null` on error.
You can also drop the special meaning of `null` and handle “disable storage” purely in tests (by stubbing/mocking `window.localStorage`).
### Suggested refactor
Replace `resolveStorage` with a simpler helper and keep `readBooleanPreference` / `writeBooleanPreference` focused on boolean logic:
```ts
const getPreferenceStorage = (storageOverride?: Storage | null): Storage | null => {
if (storageOverride) {
return storageOverride;
}
if (typeof window === "undefined") {
return null;
}
try {
return window.localStorage ?? null;
} catch {
return null;
}
};
```
Then update the callers:
```ts
export const readBooleanPreference = (
key: string,
fallback: boolean,
storageOverride?: Storage | null,
): boolean => {
const storage = getPreferenceStorage(storageOverride);
if (!storage) return fallback;
try {
const saved = storage.getItem(key);
if (saved === "true") return true;
if (saved === "false") return false;
return fallback;
} catch {
return fallback;
}
};
export const writeBooleanPreference = (
key: string,
value: boolean,
storageOverride?: Storage | null,
): void => {
const storage = getPreferenceStorage(storageOverride);
if (!storage) return;
try {
storage.setItem(key, String(value));
} catch {
// Ignore restricted storage environments.
}
};
```
In tests:
- To disable storage entirely, stub `window.localStorage` to `null` or a throwing object instead of passing `null` into the API.
- To inject a fake storage, pass it as `storageOverride` (no special `null` semantics needed).
This reduces branching and mental load while preserving all current behaviors: safe access, injectable storage, and boolean parsing.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey - I've found 1 issue
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="dashboard/src/views/extension/extensionPreferenceStorage.mjs" line_range="5" />
<code_context>
+export const PLUGIN_LIST_VIEW_MODE_STORAGE_KEY = "pluginListViewMode";
+export const PIN_UPDATES_ON_TOP_STORAGE_KEY = "pinUpdatesOnTop";
+
+const hasStorageMethod = (storage, methodName) =>
+ storage != null && typeof storage[methodName] === "function";
+
</code_context>
<issue_to_address>
**issue (complexity):** Consider replacing the generic `resolveStorage`/`hasStorageMethod` abstraction with explicit read/write storage helpers to make the control flow clearer and easier to follow.
You can keep the same behaviors while simplifying the indirection around `methodName`/`hasStorageMethod` and making `readBooleanPreference` / `writeBooleanPreference` easier to follow.
### Simplify storage resolution into explicit read/write helpers
Instead of a generic `resolveStorage(storage, methodName)` plus `hasStorageMethod`, you can have two small helpers with the method checks inlined. This removes the string-based `methodName` plumbing and makes the control flow more direct, while preserving:
- `storage === null` → explicitly disable storage
- `storage !== undefined` → custom storage (tests/injection)
- `storage === undefined` → auto-detect `window.localStorage` where available
- Safe behavior in restricted environments
Example:
```js
const getStorageForRead = (storageOverride) => {
if (storageOverride === null) {
return null;
}
if (storageOverride !== undefined) {
return typeof storageOverride?.getItem === "function" ? storageOverride : null;
}
if (typeof window === "undefined") {
return null;
}
try {
const localStorage = window.localStorage ?? null;
return typeof localStorage?.getItem === "function" ? localStorage : null;
} catch {
return null;
}
};
const getStorageForWrite = (storageOverride) => {
if (storageOverride === null) {
return null;
}
if (storageOverride !== undefined) {
return typeof storageOverride?.setItem === "function" ? storageOverride : null;
}
if (typeof window === "undefined") {
return null;
}
try {
const localStorage = window.localStorage ?? null;
return typeof localStorage?.setItem === "function" ? localStorage : null;
} catch {
return null;
}
};
```
Then the preference helpers read more linearly, without the extra abstraction:
```js
export const readBooleanPreference = (key, fallback, storage) => {
const targetStorage = getStorageForRead(storage);
if (!targetStorage) {
return fallback;
}
try {
const saved = targetStorage.getItem(key);
if (saved === "true") return true;
if (saved === "false") return false;
return fallback;
} catch {
return fallback;
}
};
export const writeBooleanPreference = (key, value, storage) => {
const targetStorage = getStorageForWrite(storage);
if (!targetStorage) {
return;
}
try {
targetStorage.setItem(key, String(value));
} catch {
// Ignore restricted storage environments.
}
};
```
This keeps all existing behaviors (including explicit disabling via `null`) but reduces cognitive load by avoiding generic `methodName` and `hasStorageMethod` indirection.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The
compareInstalledUpdatePinning+update_statusbranch currently inverts arguments to flip sort order; consider passing a sort direction multiplier or boolean into a single comparison function to make the intent clearer and reduce the chance of subtle ordering bugs. - The
getStorageForReadandgetStorageForWritehelpers share a lot of structure; extracting a single parametrized helper (e.g., passing the required method name) would remove duplication and make future changes to storage resolution logic easier to maintain.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `compareInstalledUpdatePinning` + `update_status` branch currently inverts arguments to flip sort order; consider passing a sort direction multiplier or boolean into a single comparison function to make the intent clearer and reduce the chance of subtle ordering bugs.
- The `getStorageForRead` and `getStorageForWrite` helpers share a lot of structure; extracting a single parametrized helper (e.g., passing the required method name) would remove duplication and make future changes to storage resolution logic easier to maintain.
## Individual Comments
### Comment 1
<location path="dashboard/src/views/extension/extensionPreferenceStorage.mjs" line_range="9" />
<code_context>
+ * Resolve the storage backend for reading preferences.
+ * Pass `null` to explicitly disable storage access in callers/tests.
+ */
+const getStorageForRead = (storageOverride) => {
+ if (storageOverride === null) {
+ return null;
</code_context>
<issue_to_address>
**issue (complexity):** Consider unifying the storage resolver into a single function and moving read/write capability checks to the call sites to reduce duplication and narrow the API surface.
You can reduce duplication and surface area by collapsing `getStorageForRead`/`getStorageForWrite` into a single resolver and doing the capability checks at the call sites.
This keeps all current semantics intact, including:
- `storage === null` → explicitly disable storage
- `storage === undefined` → auto-detect `window.localStorage` if available
- `storage` object → use override (even if it only implements one of `getItem`/`setItem`)
Example refactor:
```ts
const getSafeLocalStorage = () => {
if (typeof window === "undefined") return null;
try {
return window.localStorage ?? null;
} catch {
return null;
}
};
const resolveStorage = (storageOverride?: Storage | null) => {
if (storageOverride === null) return null;
if (storageOverride !== undefined) return storageOverride;
return getSafeLocalStorage();
};
export const readBooleanPreference = (
key: string,
fallback: boolean,
storage?: Storage | null,
) => {
const targetStorage = resolveStorage(storage);
if (!targetStorage || typeof targetStorage.getItem !== "function") {
return fallback;
}
try {
const saved = targetStorage.getItem(key);
if (saved === "true") return true;
if (saved === "false") return false;
return fallback;
} catch {
return fallback;
}
};
export const writeBooleanPreference = (
key: string,
value: boolean,
storage?: Storage | null,
) => {
const targetStorage = resolveStorage(storage);
if (!targetStorage || typeof targetStorage.setItem !== "function") {
return;
}
try {
targetStorage.setItem(key, String(value));
} catch {
// Ignore restricted storage environments.
}
};
```
This:
- Removes the duplicated resolver logic and repeated `try/catch` blocks.
- Keeps the `null` override semantics and testability.
- Localizes the “can I read/write?” checks to the `readBooleanPreference`/`writeBooleanPreference` functions, aligning with the actual capability used.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
问题描述
插件有新版本时,用户难以发现。当前需要手动切换到"更新状态"排序才能看到哪些插件有更新。
解决方案
添加
pinUpdatesOnTop开关(默认开启),在任何排序方式下都优先显示有更新的插件。用户也可关闭此功能恢复原有排序行为。额外想法
除解决 issue 外,额外添加了可开关的设置,让用户可选择是否启用此功能。
修改内容
useExtensionPage.js:添加pinUpdatesOnTop状态和排序逻辑InstalledPluginsTab.vue:添加开关控件Related Issue
Fixes #7577
Summary by Sourcery
Add a preference-driven option to pin plugins with available updates to the top of the installed plugins list and centralize extension preference storage handling.
New Features:
Enhancements:
Tests: