Repository navigation
fix(plugins): accelerate marketplace loading and harden install state - #2549
Conversation
Avoid blocking chat on Codex plugin/list, load personal catalogs from disk, and keep the expanded plugin preview chip during conversations. Co-authored-by: Cursor <cursoragent@cursor.com>
Refresh installed membership when peek is stale, overlay catalog install flags from account installs, open installed-strip details via marketplace enrichment, and paint cloud installs without waiting on plugin/list. Co-authored-by: Cursor <cursoragent@cursor.com>
…lobber Keep cloud installs visible when marketplace rows are still empty, and do not persist membership-only plugin/installed refreshes into the 7-day readState cache. Co-authored-by: Cursor <cursoragent@cursor.com>
📝 WalkthroughWalkthroughThe pull request batches backend plugin access checks and adds disk-based personal marketplace discovery. It introduces durable Codex plugin read-state caching, progressive marketplace loading, catalog merging, client-side search ranking, installed-plugin matching, and related UI and test updates. ChangesMarketplace access and local catalog
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (12)
backend/tests/services/test_plugin_marketplace_v2.py (1)
974-1010: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winExercise the namespace-grant batch path.
This fixture creates only
entity_type="user"grants. It does not execute the new granted-namespace name query. A per-plugin namespace lookup can regress while this test passes.Add an approved namespace membership and a namespace plugin grant. Assert that listing returns the expected item and keeps namespace-name resolution batched.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/tests/services/test_plugin_marketplace_v2.py` around lines 974 - 1010, Extend the test setup around PluginMarketplaceService.list_plugins to create an approved namespace membership and a corresponding namespace plugin grant, then assert the expected plugin is returned. Update the query-count assertion to cover the namespace-name resolution path and verify it remains batched rather than issuing per-plugin lookups.wework/src/features/plugins/loadComposerPluginApps.ts (1)
77-85: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
pluginStatessupersedespluginKeys.Each entry of
pluginStatesalready carries the samekeyvalue thatpluginKeysreports, computed with the identical expression. DroppluginKeysto remove the duplicated expression and keep the diagnostic payload smaller.♻️ Proposed change
localCount: localItems.length, - pluginKeys: installedPlugins.map( - plugin => plugin.spec.source?.pluginKey || plugin.metadata.name - ), pluginStates: installedPlugins.map(plugin => ({ key: plugin.spec.source?.pluginKey || plugin.metadata.name, enabled: plugin.spec.enabled, installState: plugin.spec.installState, })),🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@wework/src/features/plugins/loadComposerPluginApps.ts` around lines 77 - 85, Remove the redundant pluginKeys mapping from the diagnostic payload in loadComposerPluginApps, since pluginStates already provides the identical key values. Preserve pluginStates and its existing key, enabled, and installState fields unchanged.wework/src/components/plugins/PluginsWorkspace.tsx (3)
2603-2626: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSimplify the redundant rejection guard.
At Line 2607,
cloudItemsis[]whenevercloudResult.status === 'rejected'. The second clause at Line 2610 is therefore always true, and the condition reduces to a plain rejected check.♻️ Proposed change
- if (cloudResult.status === 'rejected' && cloudItems.length === 0) return + if (cloudResult.status === 'rejected') return🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@wework/src/components/plugins/PluginsWorkspace.tsx` around lines 2603 - 2626, In the Promise.allSettled callback, simplify the cloudResult guard by removing the redundant cloudItems.length check and return directly when cloudResult.status is 'rejected'. Preserve the existing fulfilled-result handling and partial catalog painting flow.
2396-2511: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftExtract the repeated catalog merge pipeline into one helper.
The same sequence now exists three times in this effect:
applyCatalogSnapshotat Line 2333-2350paintPartialCatalogat Line 2460-2489- the post-settlement disk branch at Line 2530-2554
Each block resolves cloud rows from cache, calls
mergeDiskPersonalIntoLocalRows, callsmergeInstalledPlugins, then callsmergeMarketplaceCatalog. The three copies already differ in how they source the previous installed list, which is where drift will start.
paintPartialCatalogis about 116 lines and the enclosing effect is about 466 lines. Extract a singlebuildCatalogSnapshot({ cloudItems, cloudInstalled, localState, diskPersonalItems })helper outside the component and reuse it in all three places.As per coding guidelines: "functions should remain focused, preferably under 50 lines" and "Before adding code, search for and reuse existing components, services, utilities, and patterns; extract shared logic instead of duplicating it".
Also applies to: 2520-2569
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@wework/src/components/plugins/PluginsWorkspace.tsx` around lines 2396 - 2511, Extract the shared catalog-merging pipeline into a focused buildCatalogSnapshot({ cloudItems, cloudInstalled, localState, diskPersonalItems }) helper outside the component. Move cache fallback resolution, mergeDiskPersonalIntoLocalRows, mergeInstalledPlugins, and mergeMarketplaceCatalog into that helper, while preserving each caller’s existing previous-installed source and inputs. Replace the duplicated logic in applyCatalogSnapshot, paintPartialCatalog, and the post-settlement disk branch with calls to the helper.Source: Coding guidelines
2339-2345: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMove the
diskPersonalItemsForMergedeclaration aboveapplyCatalogSnapshot.
applyCatalogSnapshotis defined at Line 2326 and readsdiskPersonalItemsForMerge, but that variable is declared withletat Line 2394. The current call sites are async, so no temporal dead zone occurs today. A future synchronous call before Line 2394 would throw aReferenceError. Declare the four merge accumulators beforeapplyCatalogSnapshotto remove the forward reference.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@wework/src/components/plugins/PluginsWorkspace.tsx` around lines 2339 - 2345, Move the declaration of diskPersonalItemsForMerge and the other three merge accumulators above applyCatalogSnapshot, preserving their existing let declarations and initialization. Ensure applyCatalogSnapshot can safely reference all four variables without relying on asynchronous call sites.wework/src/components/plugins/PluginsWorkspace.test.tsx (1)
2489-2528: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse one pattern to wrap the existing
fetchmock.Line 2518 captures
vi.mocked(fetch)and calls it directly. Line 3040 capturesvi.mocked(fetch).getMockImplementation()and calls the implementation. Both work, but the two patterns describe the same intent in the same file. Pick one, preferably thegetMockImplementation()form, sovi.mocked(fetch).mock.callskeeps recording every request in both tests.Also applies to: 3036-3047
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@wework/src/components/plugins/PluginsWorkspace.test.tsx` around lines 2489 - 2528, Use the same fetch-wrapper pattern in both tests: update the local fetch mock around the test beginning “paints local Codex plugins…” and the corresponding wrapper near the later test to obtain and call fetch’s existing mock implementation via vi.mocked(fetch).getMockImplementation(). Preserve marketplace request interception while ensuring all delegated requests continue through the mock so vi.mocked(fetch).mock.calls records them consistently.wework/src/api/local/listPersonalMarketplaceFromDisk.test.ts (1)
26-105: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd coverage for the failure path.
listPersonalMarketplacePluginsFromDiskcatches aninvokerejection and returns an empty array. That branch protects the personal tab from a broken Tauri command and is not covered. Add a case wheremocks.invoke.mockRejectedValue(...)and assert the result is[].🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@wework/src/api/local/listPersonalMarketplaceFromDisk.test.ts` around lines 26 - 105, Add a failure-path test to the listPersonalMarketplacePluginsFromDisk suite that configures mocks.invoke.mockRejectedValue(...), calls listPersonalMarketplacePluginsFromDisk, and asserts it returns an empty array. Keep the existing setup and success-path assertions unchanged.wework/src-tauri/src/local_executor.rs (2)
3844-3867: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low value
latest_cached_personal_plugin_rootselects by modification time, not by version.Codex cache directories are named by version (for example
0.1.0). This function returns the directory with the newestmtime. A re-extracted or touched older version then wins over the newest installed version. Consider comparing the directory names as versions and usingmtimeonly as a tie-break.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@wework/src-tauri/src/local_executor.rs` around lines 3844 - 3867, The latest_cached_personal_plugin_root function should select the highest semantic version from valid cache directory names instead of prioritizing modification time. Parse each directory name as a version, ignore unparseable names, and use modified time only when versions tie; preserve manifest validation and return the selected directory path.
3831-3841: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winThe installed-tree preference check ignores Claude manifests.
Every other manifest probe in this file accepts
.codex-plugin/plugin.jsonor.claude-plugin/plugin.json. This check only tests.codex-plugin/plugin.json. A marketplace entry whose tree contains only a Claude manifest is treated as a stub, soplugin_pathis replaced by the cached copy.♻️ Proposed fix
if candidate.installed { existing.installed = true; // Prefer the concrete installed tree when marketplace.json only had a stub. - if existing.plugin_path.is_empty() - || !Path::new(&existing.plugin_path) - .join(".codex-plugin/plugin.json") - .is_file() - { + let existing_root = Path::new(&existing.plugin_path); + let existing_has_manifest = !existing.plugin_path.is_empty() + && (existing_root.join(".codex-plugin/plugin.json").is_file() + || existing_root.join(".claude-plugin/plugin.json").is_file()); + if !existing_has_manifest { existing.plugin_path = candidate.plugin_path; } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@wework/src-tauri/src/local_executor.rs` around lines 3831 - 3841, The installed-tree preference check in the candidate merge logic must recognize both manifest locations. Update the condition guarding existing.plugin_path so it treats either .codex-plugin/plugin.json or .claude-plugin/plugin.json as a valid installed tree before replacing the path with candidate.plugin_path.wework/src/api/local/codexPlugins.ts (3)
470-488: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
warmLocalCodexPluginsReadStatecaches the resolved promise forever.
warmupReadStatePromiseis cleared on failure but not after a successful resolution. A later call returns the original state object instead of a current snapshot. OnlyclearLocalCodexPluginsReadStateCacheresets it. If warm-once is the intent, state that in the doc comment. Otherwise clear the memo after the promise settles and letreadStatededuplicate throughinflightReadState.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@wework/src/api/local/codexPlugins.ts` around lines 470 - 488, Update warmLocalCodexPluginsReadState so warmupReadStatePromise is cleared after successful settlement as well as failure, allowing later calls to obtain a current snapshot while relying on readState’s inflightReadState deduplication. Preserve the existing warmup logging and error propagation, and ensure the cache reset occurs only after the promise has settled.
1454-1485: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePassing
installedResponse.marketplacesas both arguments hides intent.
mergeInstalledPluginSummaries(installed, available)receives the same list twice. The call reads like a copy-paste defect even though it is deliberate. Add a named local or a short comment that states there is no available catalog in this path.♻️ Proposed fix
+ // No plugin/list here, so the installed response is also the only "available" source. + const installedMarketplaces = installedResponse.marketplaces const installedPlugins = applyPluginCloudLinks( preferWeworkPersonalInstalled( - mergeInstalledPluginSummaries(installedResponse.marketplaces, installedResponse.marketplaces) + mergeInstalledPluginSummaries(installedMarketplaces, installedMarketplaces) ), cloudLinks )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@wework/src/api/local/codexPlugins.ts` around lines 1454 - 1485, Add a clearly named local or concise comment near the mergeInstalledPluginSummaries call in loadInstalledPluginsOnly explaining that this path has no separate available plugin catalog and intentionally passes installedResponse.marketplaces for both arguments. Preserve the existing behavior and merge inputs.
435-462: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
peekLocalCodexPluginsReadStatemutates the shared cache, which makes the freshness check ambiguous.The peek writes
cachedState,cachedStateAt, andcachedStateParamsKeywhen it loads a persisted entry.isLocalCodexPluginsReadStateFreshthen comparesDate.now()againstcachedStateAt, which the peek may have just rebound to a differentparamsKey.listInstalledPluginscalls both a keyed peek and an unkeyed peek at Line 1726 and Line 1731, so the freshness result can describe a different snapshot than the returned one.Return the entry timestamp from a single lookup instead of reading module state twice.
♻️ Proposed direction
+function lookupReadStateSnapshot(paramsKey: string): PersistedReadStateEntry | null { + if (cachedState && cachedStateParamsKey === paramsKey) { + return { paramsKey, cachedAt: cachedStateAt, state: cachedState } + } + const persisted = readPersistedReadStateStore().entries[paramsKey] + if (!persisted) return null + if (Date.now() - persisted.cachedAt > READ_STATE_DURABLE_TTL_MS) return null + return persisted +}Build both
peekLocalCodexPluginsReadStateandisLocalCodexPluginsReadStateFreshon that helper.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@wework/src/api/local/codexPlugins.ts` around lines 435 - 462, Introduce a shared non-mutating lookup helper for the requested params that returns the persisted state and its timestamp after hydration and TTL validation. Update peekLocalCodexPluginsReadState and isLocalCodexPluginsReadStateFresh to use this helper, with freshness calculated from the same lookup result rather than cachedStateAt; preserve existing null and boolean behavior without rebinding the shared cache during a peek.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/tests/services/test_plugin_marketplace_v2.py`:
- Around line 934-990: Add type annotations to
test_list_plugins_batches_grant_lookups_instead_of_per_plugin_queries, including
its test_db and test_user fixture parameters, and to before_cursor_execute’s
connection, cursor, statement, parameters, context, executemany, and return
type. Use the repository’s existing SQLAlchemy and test fixture annotation
conventions without changing test behavior.
In `@wework/src-tauri/src/local_executor.rs`:
- Around line 3877-3886: Update the marketplace manifest handling in the
disk-listing flow around marketplace_plugin_names: treat failures from reading
or parsing the manifest as an empty marketplace result instead of propagating
the error with ?. Preserve the existing plugin insertion for valid manifests and
allow installed-tree and Codex cache scans to continue when the manifest is
invalid.
In `@wework/src/api/local/codexPlugins.readStateCache.test.ts`:
- Around line 312-320: Add teardown for the Date.now mock used in the
stale-cache test around api.readState, preferably with afterEach(() =>
vi.restoreAllMocks()) so the advanced clock is restored after every test. Ensure
later tests do not inherit the mocked timestamp.
In `@wework/src/api/local/codexPlugins.ts`:
- Around line 1334-1343: Update the background revalidation cleanup in the
cached-state branch around loadReadStateSnapshot so the promise returned by
finally has a rejection handler that logs failures instead of becoming
unhandled. Preserve the existing inflightReadState deletion guard, while leaving
direct awaiting callers of existingInflight able to receive the original
rejection.
In `@wework/src/components/plugins/PluginsWorkspace.test.tsx`:
- Around line 3018-3019: Update the asynchronous assertions in the
pendingLocalList resolution scenarios around the existing resolveLocalList
calls, including both the 3018/3019 case and the corresponding 3054-3060 case.
Wrap each post-resolution plugin strip assertion in waitFor so the promise
callbacks and React re-render complete before verifying the strip remains
present.
In `@wework/src/components/plugins/PluginsWorkspace.tsx`:
- Around line 2436-2458: Update the personal disk-row seeding logic around
toMarketplaceOptions and setSelectedMarketplaceKey so the selected key is chosen
only from the generated cloud-available marketplace options. Do not select
cloudMarketplaceKey() unless that key exists in seeded (or the corresponding
available market list); otherwise fall back to seeded[0]?.key, preserving the
existing current selection when valid.
In `@wework/src/features/workbench/WorkbenchProvider.tsx`:
- Around line 1748-1769: Update the composer plugin loading flow around
loadComposerPluginApps and readLocalInstalledPlugins so currentComposerDeviceId
is resolved from cached Codex state or ensureLocalExecutorStarted before calling
localPluginApi.listInstalledPlugins(). Keep the installed-plugin response
handling unchanged, and ensure the resolved device ID is available to cloud
plugin loading even when listInstalledPlugins() rejects.
---
Nitpick comments:
In `@backend/tests/services/test_plugin_marketplace_v2.py`:
- Around line 974-1010: Extend the test setup around
PluginMarketplaceService.list_plugins to create an approved namespace membership
and a corresponding namespace plugin grant, then assert the expected plugin is
returned. Update the query-count assertion to cover the namespace-name
resolution path and verify it remains batched rather than issuing per-plugin
lookups.
In `@wework/src-tauri/src/local_executor.rs`:
- Around line 3844-3867: The latest_cached_personal_plugin_root function should
select the highest semantic version from valid cache directory names instead of
prioritizing modification time. Parse each directory name as a version, ignore
unparseable names, and use modified time only when versions tie; preserve
manifest validation and return the selected directory path.
- Around line 3831-3841: The installed-tree preference check in the candidate
merge logic must recognize both manifest locations. Update the condition
guarding existing.plugin_path so it treats either .codex-plugin/plugin.json or
.claude-plugin/plugin.json as a valid installed tree before replacing the path
with candidate.plugin_path.
In `@wework/src/api/local/codexPlugins.ts`:
- Around line 470-488: Update warmLocalCodexPluginsReadState so
warmupReadStatePromise is cleared after successful settlement as well as
failure, allowing later calls to obtain a current snapshot while relying on
readState’s inflightReadState deduplication. Preserve the existing warmup
logging and error propagation, and ensure the cache reset occurs only after the
promise has settled.
- Around line 1454-1485: Add a clearly named local or concise comment near the
mergeInstalledPluginSummaries call in loadInstalledPluginsOnly explaining that
this path has no separate available plugin catalog and intentionally passes
installedResponse.marketplaces for both arguments. Preserve the existing
behavior and merge inputs.
- Around line 435-462: Introduce a shared non-mutating lookup helper for the
requested params that returns the persisted state and its timestamp after
hydration and TTL validation. Update peekLocalCodexPluginsReadState and
isLocalCodexPluginsReadStateFresh to use this helper, with freshness calculated
from the same lookup result rather than cachedStateAt; preserve existing null
and boolean behavior without rebinding the shared cache during a peek.
In `@wework/src/api/local/listPersonalMarketplaceFromDisk.test.ts`:
- Around line 26-105: Add a failure-path test to the
listPersonalMarketplacePluginsFromDisk suite that configures
mocks.invoke.mockRejectedValue(...), calls
listPersonalMarketplacePluginsFromDisk, and asserts it returns an empty array.
Keep the existing setup and success-path assertions unchanged.
In `@wework/src/components/plugins/PluginsWorkspace.test.tsx`:
- Around line 2489-2528: Use the same fetch-wrapper pattern in both tests:
update the local fetch mock around the test beginning “paints local Codex
plugins…” and the corresponding wrapper near the later test to obtain and call
fetch’s existing mock implementation via
vi.mocked(fetch).getMockImplementation(). Preserve marketplace request
interception while ensuring all delegated requests continue through the mock so
vi.mocked(fetch).mock.calls records them consistently.
In `@wework/src/components/plugins/PluginsWorkspace.tsx`:
- Around line 2603-2626: In the Promise.allSettled callback, simplify the
cloudResult guard by removing the redundant cloudItems.length check and return
directly when cloudResult.status is 'rejected'. Preserve the existing
fulfilled-result handling and partial catalog painting flow.
- Around line 2396-2511: Extract the shared catalog-merging pipeline into a
focused buildCatalogSnapshot({ cloudItems, cloudInstalled, localState,
diskPersonalItems }) helper outside the component. Move cache fallback
resolution, mergeDiskPersonalIntoLocalRows, mergeInstalledPlugins, and
mergeMarketplaceCatalog into that helper, while preserving each caller’s
existing previous-installed source and inputs. Replace the duplicated logic in
applyCatalogSnapshot, paintPartialCatalog, and the post-settlement disk branch
with calls to the helper.
- Around line 2339-2345: Move the declaration of diskPersonalItemsForMerge and
the other three merge accumulators above applyCatalogSnapshot, preserving their
existing let declarations and initialization. Ensure applyCatalogSnapshot can
safely reference all four variables without relying on asynchronous call sites.
In `@wework/src/features/plugins/loadComposerPluginApps.ts`:
- Around line 77-85: Remove the redundant pluginKeys mapping from the diagnostic
payload in loadComposerPluginApps, since pluginStates already provides the
identical key values. Preserve pluginStates and its existing key, enabled, and
installState fields unchanged.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 11cdc577-9139-407d-8c92-f02c146ba7aa
📒 Files selected for processing (22)
backend/app/services/plugin_marketplace_service.pybackend/tests/services/test_plugin_marketplace_v2.pywework/src-tauri/src/lib.rswework/src-tauri/src/local_executor.rswework/src/api/local/codexPlugins.readStateCache.test.tswework/src/api/local/codexPlugins.tswework/src/api/local/listPersonalMarketplaceFromDisk.test.tswework/src/components/chat/composer/ComposerToolbar.test.tsxwework/src/components/chat/composer/PluginPickerMenu.tsxwework/src/components/layout/DesktopWorkbenchMain.tsxwework/src/components/plugins/PluginsWorkspace.test.tsxwework/src/components/plugins/PluginsWorkspace.tsxwework/src/components/plugins/findMarketplaceItemForInstalled.test.tswework/src/components/plugins/findMarketplaceItemForInstalled.tswework/src/components/plugins/marketplaceCatalogMerge.test.tswework/src/components/plugins/marketplaceCatalogMerge.tswework/src/features/plugins/loadComposerPluginApps.tswework/src/features/plugins/marketplaceIdentity.test.tswework/src/features/plugins/marketplaceIdentity.tswework/src/features/plugins/marketplaceSearch.test.tswework/src/features/plugins/marketplaceSearch.tswework/src/features/workbench/WorkbenchProvider.tsx
💤 Files with no reviewable changes (1)
- wework/src/components/layout/DesktopWorkbenchMain.tsx
Hold back strip/actions while install+local-auth is in flight, keep catalog mounted during post-install refresh, and address CodeRabbit findings on background refresh, disk listing, and device-id resolution. Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@wework/src/components/plugins/holdBackInFlightMarketplaceInstalls.ts`:
- Around line 20-44: Scope blocked plugin identities by marketplace and plugin
key in holdBackInFlightMarketplaceInstalls, matching installed plugins via
raw.spec.source.marketplace or providerKey plus pluginKey; pass local-auth
identities in this scoped form rather than bare names. In
wework/src/components/plugins/holdBackInFlightMarketplaceInstalls.ts lines
20-44, update blocked tracking and matching so same keys in other marketplaces
remain visible. In
wework/src/components/plugins/holdBackInFlightMarketplaceInstalls.test.ts lines
92-116, add install and local-auth cases with duplicate keys across marketplaces
and assert only the pending marketplace row and installed-strip item are hidden.
In `@wework/src/features/workbench/WorkbenchProvider.tsx`:
- Around line 1767-1775: Update the catch around
localPluginApi.listInstalledPlugins() to log a focused diagnostic without
request-sensitive data, then preserve the last-known installed-plugin snapshot
instead of returning []. Keep successful responses, including an explicitly
empty response, unchanged so the caller can clear state only after confirmed
success.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 88573103-6888-4882-8508-a15688afd03c
📒 Files selected for processing (10)
backend/tests/services/test_plugin_marketplace_v2.pywework/e2e/desktop/task-flow.e2e.mjswework/src-tauri/src/local_executor.rswework/src/api/local/codexPlugins.readStateCache.test.tswework/src/api/local/codexPlugins.tswework/src/components/plugins/PluginsWorkspace.test.tsxwework/src/components/plugins/PluginsWorkspace.tsxwework/src/components/plugins/holdBackInFlightMarketplaceInstalls.test.tswework/src/components/plugins/holdBackInFlightMarketplaceInstalls.tswework/src/features/workbench/WorkbenchProvider.tsx
🚧 Files skipped from review as they are similar to previous changes (6)
- backend/tests/services/test_plugin_marketplace_v2.py
- wework/src/api/local/codexPlugins.readStateCache.test.ts
- wework/src-tauri/src/local_executor.rs
- wework/src/api/local/codexPlugins.ts
- wework/src/components/plugins/PluginsWorkspace.test.tsx
- wework/src/components/plugins/PluginsWorkspace.tsx
| const blockedNames = new Set<string>() | ||
| if (authKey) blockedNames.add(authKey) | ||
| const items = input.items.map(item => { | ||
| const blocked = | ||
| input.installingIds.has(item.id) || | ||
| (authKey !== '' && item.name.trim().toLowerCase() === authKey) | ||
| if (!blocked) return item | ||
| blockedNames.add(item.name.trim().toLowerCase()) | ||
| return { | ||
| ...item, | ||
| installed: false, | ||
| installedLocally: false, | ||
| installedPluginId: null, | ||
| } | ||
| }) | ||
|
|
||
| if (blockedNames.size === 0) { | ||
| return { items, installed: input.installed } | ||
| } | ||
|
|
||
| const installed = input.installed.filter(plugin => { | ||
| const pluginKey = (plugin.raw.spec.source.pluginKey || plugin.name).trim().toLowerCase() | ||
| const name = plugin.name.trim().toLowerCase() | ||
| return !blockedNames.has(pluginKey) && !blockedNames.has(name) | ||
| }) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Keep the blocked-plugin identity marketplace-scoped.
blockedNames contains only item.name. Lines 27 and 43 can therefore hide a plugin from a different marketplace when both marketplaces use the same plugin key. This also makes authPluginKey block every matching card name.
wework/src/components/plugins/holdBackInFlightMarketplaceInstalls.ts#L20-L44: Track the blocked identity with the marketplace and plugin key. Match installed rows withplugin.raw.spec.source.marketplaceorproviderKeypluspluginKey. Pass a scoped local-auth identity instead of a bare plugin name.wework/src/components/plugins/holdBackInFlightMarketplaceInstalls.test.ts#L92-L116: Add install and local-auth cases with equal plugin keys from two marketplaces. Assert that only the pending marketplace row and installed-strip item are hidden.
📍 Affects 2 files
wework/src/components/plugins/holdBackInFlightMarketplaceInstalls.ts#L20-L44(this comment)wework/src/components/plugins/holdBackInFlightMarketplaceInstalls.test.ts#L92-L116
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@wework/src/components/plugins/holdBackInFlightMarketplaceInstalls.ts` around
lines 20 - 44, Scope blocked plugin identities by marketplace and plugin key in
holdBackInFlightMarketplaceInstalls, matching installed plugins via
raw.spec.source.marketplace or providerKey plus pluginKey; pass local-auth
identities in this scoped form rather than bare names. In
wework/src/components/plugins/holdBackInFlightMarketplaceInstalls.ts lines
20-44, update blocked tracking and matching so same keys in other marketplaces
remain visible. In
wework/src/components/plugins/holdBackInFlightMarketplaceInstalls.test.ts lines
92-116, add install and local-auth cases with duplicate keys across marketplaces
and assert only the pending marketplace row and installed-strip item are hidden.
| try { | ||
| const response = await localPluginApi.listInstalledPlugins() | ||
| currentComposerDeviceId = | ||
| peekLocalCodexPluginsReadState({ mergeAllMarketplaces: true })?.deviceId || | ||
| peekLocalCodexPluginsReadState()?.deviceId || | ||
| currentComposerDeviceId | ||
| return response.items | ||
| } catch { | ||
| return [] |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Preserve failure state instead of returning an empty installation set.
When listInstalledPlugins() rejects, this catch returns []. The caller cannot distinguish a transient local API failure from a confirmed empty installation set. On the current-generation allowEmptySnapshot path at Line 1840, this can call replaceComposerApps([]) and blank the composer picker. Preserve the last-known snapshot on failure, and clear it only after a successful empty response. Add focused logging without sensitive request data.
As per coding guidelines, do not hide primary-path state or synchronization bugs behind fallback behavior; add focused diagnostic logging before changing behavior.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@wework/src/features/workbench/WorkbenchProvider.tsx` around lines 1767 -
1775, Update the catch around localPluginApi.listInstalledPlugins() to log a
focused diagnostic without request-sensitive data, then preserve the last-known
installed-plugin snapshot instead of returning []. Keep successful responses,
including an explicitly empty response, unchanged so the caller can clear state
only after confirmed success.
Source: Coding guidelines
Summary
pluginIddrives marketplace cardinstalled, installed-strip opens enriched marketplace detail, and cloud installs paint before slow Codexplugin/list.plugin/installedrefreshes in memory only so they do not clobber the durable readState snapshot.Test plan
plugin/listplugin/listpnpm --filter wework exec vitest runon marketplace-related testscd backend && uv run pytest tests/services/test_plugin_marketplace_v2.pyMade with Cursor
Summary by CodeRabbit
New Features
Bug Fixes