fix: Replace actioningPlugin with useLoading hook by EurFelux · Pull Request #13095 · CherryHQ/cherry-studio

EurFelux · 2026-02-27T05:58:05Z

What this PR does

Before this PR:

Used local component state (useState<string | null>) to track loading status in PluginBrowser.tsx
This caused a bug where loading state is lost when component unmounts (user navigates away)
When user returns, they see "Install" button instead of loading state, and may click again causing duplicate operations
Used session-specific waiting state (sessionWaiting) in Redux store

After this PR:

Introduces a reusable useLoading hook that manages loading state in Redux store (persists across component lifecycle)
Replaces setSessionWaitingAction with more generic setLoadingAction that uses a loadingMap for multiple loading states
PluginBrowser.tsx now uses the centralized useLoading hook that survives component remount

Why we need it and why it was done in this way

This PR fixes a bug where loading state is lost when the PluginBrowser component unmounts. When the user navigates away and returns, they see "Install" button instead of loading state, leading them to click again and cause duplicate operations.

The following tradeoffs were made:

Centralized loading state allows multiple components to track loading status for the same operation (e.g., both the button and the modal can check if installation is in progress)
Using a key-based map (loadingMap) instead of single session-specific state provides flexibility for future use cases

The following alternatives were considered:

Keep local state in each component: rejected as it doesn't allow sharing loading state across components
Use existing sessionWaiting directly: rejected as it's session-specific and not reusable for plugins

Links to places where the discussion took place:

Breaking changes

If this PR introduces breaking changes, please describe the changes and the impact on users.

Special notes for your reviewer

This fix ensures loading state persists when user navigates away and returns to the plugin browser
The useLoading hook supports both global loading map and individual id-based usage
Also prevents duplicate clicks while loading

Checklist

This checklist is not enforcing, but it's a reminder of items that could be relevant to every PR.
Approvers are expected to review this list.

PR: The PR description is expressive enough and will help future contributors
Code: Write code that humans can understand and Keep it simple
Refactor: You have left the code cleaner than you found it (Boy Scout Rule)
Upgrade: Impact of this change on upgrade flows was considered and addressed if required
Documentation: A user-guide update was considered and is present (link) or not required. Check this only when the PR introduces or changes a user-facing feature or behavior.
Self-review: I have reviewed my own code (e.g., via /gh-pr-review, gh pr diff, or GitHub UI) before requesting review from others

Release note

fix: Persist plugin loading state in Redux to survive component unmount (prevents duplicate installs when user navigates away and returns)

to runtime root state

Fixes #13092

EurFelux

Thanks for the fix. I reviewed the diff and the overall approach looks solid: moving plugin action loading state into Redux and reusing it via useLoading should prevent loading-state loss after remount. I did not find blocking correctness or security issues in this change set.

DeJeune

Review Summary

The core idea — persisting plugin loading state in Redux to survive component unmount — is sound and addresses the reported bug well. However, there are a few issues that should be addressed before merging.

Critical / Significant

Memory leak in loadingMap: Setting value: false leaves stale keys in the map. The reducer should delete the key instead.
Unnecessary re-renders: useLoading() (no-arg) uses useRuntime() which subscribes to the entire RuntimeState. PluginBrowser (with its virtual list) will re-render on any unrelated runtime state change. Use useAppSelector((state) => state.runtime.loadingMap) instead.
Namespace collision risk: Session IDs are bare keys while plugin keys use plugin: prefix. Both share the same loadingMap. Session keys should also be namespaced (e.g., session:${id}).

Minor

Duplicate-click guard added for plugins but not for session deletion — inconsistent.

Positives

Clean hook API with well-designed function overloads.
Good createPluginLoadingKey namespacing pattern for plugins.
Complete migration — no dangling references to the old sessionWaiting / setSessionWaitingAction.

src/renderer/src/store/runtime.ts

src/renderer/src/hooks/useLoading.ts

src/renderer/src/pages/home/Tabs/components/Sessions.tsx

DeJeune · 2026-02-27T06:58:00Z

...rer/src/pages/settings/AgentSettings/components/PluginsSettings/components/PluginBrowser.tsx

+
  // Debounce search query
  const handleSearchChange = useCallback(
    (value: string) => {


Nit: Good that you added duplicate-click guards in handleInstall/handleUninstall. However, using the no-arg useLoading() overload here means the entire component re-renders on any loading state change (even from unrelated features like session deletion).

Since PluginBrowser renders a virtual list with many items, this could be noticeable. An alternative approach: keep actioningPlugin as local state for the UI rendering, but also set the Redux loading key for cross-component persistence. Or refactor to use the single-id overload per action.

Note
This issue/comment/review was translated by Claude.

Currently, to prevent memory leaks, each call to finishLoading causes the loadingMap reference to change, thereby triggering a re-render.

A better solution would be to not use delete and instead perform periodic GC operations during the application lifecycle. However, since there is currently no lifecycle management architecture, this solution is not being adopted for now.

Original Content

目前为了防止内存泄漏，每次调用finishLoading都会导致loadingMap引用发生变化，从而触发重渲染。

更好的解决方案是不使用delete，同时在应用生命周期内定时执行GC操作。然而，由于目前尚缺少生命周期管理架构，暂不采用此方案。

DeJeune

Follow-up Review

The latest commit addresses both critical issues from the previous review:

Memory leak fixed — finishLoadingAction now uses delete state.loadingMap[id] instead of setting false. Clean.
Re-render issue fixed — useLoading now uses useAppSelector((state) => state.runtime.loadingMap) instead of useRuntime(). Only re-renders when loadingMap changes.
Improved API — Splitting setLoading(value) into startLoading()/finishLoading() is more expressive and prevents misuse (no way to accidentally pass the wrong boolean).

Two non-blocking nits remain (session key namespacing, verbose JSDoc on internal overload) — can be addressed in a follow-up if desired.

LGTM, approving.

src/renderer/src/pages/home/Tabs/components/Sessions.tsx

src/renderer/src/hooks/useLoading.ts

EurFelux added 3 commits February 27, 2026 12:32

refactor: Rename setSessionWaitingAction to setLoadingAction and move it

2816c04

to runtime root state

feat: Add useLoading hook

ebdec1f

fix: Replace actioningPlugin with useLoading hook

3c802b3

Fixes #13092

EurFelux requested a review from 0xfullex as a code owner February 27, 2026 05:58

EurFelux commented Feb 27, 2026

View reviewed changes

kangfenmao requested a review from DeJeune February 27, 2026 06:27

DeJeune requested changes Feb 27, 2026

View reviewed changes

EurFelux added 2 commits February 27, 2026 15:59

refactor: Improve loading state management with dedicated actions

1a7bd36

refactor: Remove unused loading hooks from session deletion

dbba590

DeJeune approved these changes Feb 27, 2026

View reviewed changes

src/renderer/src/pages/home/Tabs/components/Sessions.tsx Show resolved Hide resolved

src/renderer/src/hooks/useLoading.ts Outdated Show resolved Hide resolved

EurFelux added 2 commits February 27, 2026 16:17

refactor: Normalize tailwind css class name

2164065

refactor: Simplify useLoading hook by removing redundant JSDoc

218593d

DeJeune requested a review from kangfenmao February 27, 2026 08:31

DeJeune added this to the v1.7.22 milestone Feb 28, 2026

EurFelux mentioned this pull request Mar 5, 2026

fix: install or uninstall button state issues in plugin browser #13114

Merged

DeJeune requested a review from GeorgeDong32 March 7, 2026 14:42

GeorgeDong32 approved these changes Mar 8, 2026

View reviewed changes

Merge remote-tracking branch 'origin/main' into fix/plugin-actioning

5a0940b

kangfenmao merged commit 1b94f3f into main Mar 9, 2026
7 checks passed

kangfenmao deleted the fix/plugin-actioning branch March 9, 2026 12:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Replace actioningPlugin with useLoading hook#13095

fix: Replace actioningPlugin with useLoading hook#13095
kangfenmao merged 8 commits intomainfrom
fix/plugin-actioning

EurFelux commented Feb 27, 2026 •

edited by DeJeune

Loading

Uh oh!

EurFelux left a comment

Uh oh!

DeJeune left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

DeJeune Feb 27, 2026

Uh oh!

EurFelux Feb 27, 2026 •

edited by kangfenmao

Loading

Uh oh!

DeJeune left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

EurFelux commented Feb 27, 2026 • edited by DeJeune Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What this PR does

Why we need it and why it was done in this way

Breaking changes

Special notes for your reviewer

Checklist

Release note

Uh oh!

EurFelux left a comment

Choose a reason for hiding this comment

Uh oh!

DeJeune left a comment

Choose a reason for hiding this comment

Review Summary

Critical / Significant

Minor

Positives

Uh oh!

Uh oh!

Uh oh!

Uh oh!

DeJeune Feb 27, 2026

Choose a reason for hiding this comment

Uh oh!

EurFelux Feb 27, 2026 • edited by kangfenmao Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DeJeune left a comment

Choose a reason for hiding this comment

Follow-up Review

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

EurFelux commented Feb 27, 2026 •

edited by DeJeune

Loading

EurFelux Feb 27, 2026 •

edited by kangfenmao

Loading