Repository navigation
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (7)
🚧 Files skipped from review as they are similar to previous changes (5)
📝 WalkthroughWalkthroughIntroduces a system plugin catalog feature: admins upload runtime-specific (claudecode/codex) plugin ZIP packages; users install/update both runtime variants together from a unified catalog. Backend adds ChangesSystem Plugin Management
Sequence Diagram(s)sequenceDiagram
participant Admin as Admin (Frontend)
participant AdminAPI as POST /api/admin/plugins
participant Service as InstalledPluginService
participant DB as Kind / SkillBinary
participant UserCatalog as User Catalog API
participant Executor as Executor (CapabilitySyncHandler)
rect rgba(59, 130, 246, 0.5)
Note over Admin,DB: Admin uploads runtime variants
Admin->>AdminAPI: Upload ZIP (runtime=claudecode)
AdminAPI->>Service: upload_system_plugin(bytes, runtime)
Service->>DB: upsert Kind (user_id=0, runtime=claudecode)
Service->>DB: upsert SkillBinary (package bytes)
Service-->>Admin: SystemPlugin {id, runtime, spec}
end
rect rgba(34, 197, 94, 0.5)
Note over UserCatalog,Executor: User install (both runtime variants)
Admin->>UserCatalog: GET /plugins/catalog
UserCatalog->>Service: list_system_plugin_catalog(user_id)
Service-->>UserCatalog: PluginCatalogListResponse (both variants)
Admin->>UserCatalog: POST /plugins/catalog/{id}/install
UserCatalog->>Service: install_system_plugin(user_id, system_plugin_id)
Service->>DB: _copy_system_plugin_to_installed (×2 runtimes)
UserCatalog->>Executor: _sync_global_capabilities()
Executor->>DB: _sync_plugins() — emit `#claudecode/`#codex keys
end
rect rgba(234, 88, 12, 0.5)
Note over Admin,Executor: Admin replaces package → user update flow
Admin->>AdminAPI: PUT /api/admin/plugins/{id}/package (new ZIP)
AdminAPI->>Service: replace_system_plugin_package(bytes)
Service->>DB: update Kind checksum + SkillBinary
UserCatalog->>Service: list_system_plugin_catalog
Service-->>UserCatalog: installState → update_available
Admin->>UserCatalog: POST /plugins/catalog/{id}/update
UserCatalog->>Service: update_installed_plugin_from_system
Service->>DB: re-copy payload/package (×2 runtimes)
UserCatalog->>Executor: _sync_global_capabilities()
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
wework/src/components/plugins/PluginsWorkspace.tsx (1)
661-713: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winAdd rejection handling for MCP install/uninstall actions.
installProviderServeranduninstallProviderServerdo not handle rejected promises. Network/API failures currently surface as unhandled rejections and leave users without actionable feedback.Proposed fix
const installProviderServer = (provider: MCPProviderInfo, server: MCPServer) => { mcpApi .installProviderMcp({ providerKey: provider.key, serverKey: serverKeyFromProviderServer(server), catalogItemId: server.id, displayName: server.name, description: server.description ?? '', server: serverConfigFromProviderServer(server), sourcePayload: server as unknown as Record<string, unknown>, }) .then(item => { setMcpMarketplaceState(previous => ({ ...previous, providerServers: { ...previous.providerServers, [provider.key]: (previous.providerServers[provider.key] ?? []).map(candidate => candidate.id === server.id ? { ...candidate, installState: 'installed', installedMcpId: getKindIdFromMetadata(item.metadata), enabled: item.spec.enabled, } : candidate ), }, })) }) + .catch((error: Error) => { + setMcpMarketplaceState(previous => ({ + ...previous, + error: error.message, + })) + }) } const uninstallProviderServer = (provider: MCPProviderInfo, server: MCPServer) => { if (!server.installedMcpId) return - mcpApi.uninstallInstalledMcp(server.installedMcpId).then(() => { - setMcpMarketplaceState(previous => ({ - ...previous, - providerServers: { - ...previous.providerServers, - [provider.key]: (previous.providerServers[provider.key] ?? []).map(candidate => - candidate.id === server.id - ? { - ...candidate, - installState: 'not_installed', - installedMcpId: null, - enabled: false, - } - : candidate - ), - }, - })) - }) + mcpApi + .uninstallInstalledMcp(server.installedMcpId) + .then(() => { + setMcpMarketplaceState(previous => ({ + ...previous, + providerServers: { + ...previous.providerServers, + [provider.key]: (previous.providerServers[provider.key] ?? []).map(candidate => + candidate.id === server.id + ? { + ...candidate, + installState: 'not_installed', + installedMcpId: null, + enabled: false, + } + : candidate + ), + }, + })) + }) + .catch((error: Error) => { + setMcpMarketplaceState(previous => ({ + ...previous, + error: error.message, + })) + }) }🤖 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 661 - 713, Both the installProviderServer and uninstallProviderServer functions lack error handling for their promise chains. Add a .catch() handler to the promise chain in installProviderServer (after the .then() for mcpApi.installProviderMcp) and similarly add a .catch() handler to the promise chain in uninstallProviderServer (after the .then() for mcpApi.uninstallInstalledMcp). Each catch handler should display an appropriate error notification to the user to inform them of the failure and allow them to take corrective action.
🧹 Nitpick comments (3)
frontend/src/features/admin/components/SystemPluginList.tsx (1)
70-589: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftSplit
SystemPluginListinto smaller hooks/subcomponents before further feature growth.The component currently combines fetching, state orchestration, mutation handlers, and four dialogs in one very large function, making future changes risky.
Based on learnings, function length should stay around 50 lines and complex modules should be managed via extraction into reusable modules/hooks.
🤖 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 `@frontend/src/features/admin/components/SystemPluginList.tsx` around lines 70 - 589, The SystemPluginList component is too large at 520 lines and should be refactored into smaller, more maintainable pieces. Extract the four dialog components (the Dialog and AlertDialog elements for upload, edit, replace, and delete operations) into separate subcomponents named SystemPluginUploadDialog, SystemPluginEditDialog, SystemPluginReplaceDialog, and SystemPluginDeleteDialog. Additionally, create custom hooks to manage related state: useSystemPlugins hook for the plugins list and loading/fetching logic, usePluginDialogs hook to consolidate all dialog open/close state management, and usePluginMutations hook to encapsulate the async handlers (handleUpload, handleUpdateMetadata, handleToggleEnabled, handleReplacePackage, handleDelete). Finally, extract the plugin list rendering logic into a separate SystemPluginListView subcomponent. This will reduce the main component to under 50 lines for better maintainability and testability.Source: Learnings
wework/src/components/plugins/PluginsWorkspace.test.tsx (1)
63-503: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftSplit
mockSystemSkillsFetchinto scenario-focused mock builders.This helper now mixes many unrelated API branches (skills, MCP, plugin catalog/install/update) in one large function, which makes failures harder to localize and increases coupling between tests.
Based on learnings: “Manage complexity through extraction: If a module is complex, prioritize extracting common logic into utilities or creating new modules rather than adding more complexity.”
🤖 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 63 - 503, The mockSystemSkillsFetch function combines multiple unrelated API mocking concerns (skills, MCP providers, and plugins) into a single large function, making it difficult to maintain and causing unnecessary coupling between tests. Extract the different API endpoint branches into separate, scenario-focused mock builder functions where each function handles only its specific concern (e.g., one for skills APIs, one for MCP APIs, one for plugin APIs). This allows tests to use only the mocks they need and makes failures easier to localize to specific API concerns.Source: Learnings
backend/app/api/endpoints/admin/plugins.py (1)
27-117: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winInstrument these new admin plugin endpoints with tracing.
The new upload/update/replace/delete flows are high-value operational paths and should emit OpenTelemetry spans/attributes for runtime, plugin ID, and outcome.
As per coding guidelines,
**/*.py: Use@trace_async(),@trace_sync(),add_span_event(), orset_span_attribute()for OpenTelemetry tracing in Python code.🤖 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/app/api/endpoints/admin/plugins.py` around lines 27 - 117, Add OpenTelemetry tracing instrumentation to all five admin plugin endpoints: list_system_plugins, upload_system_plugin, update_system_plugin, replace_system_plugin_package, and delete_system_plugin. Decorate each async endpoint function with `@trace_async`() and within each function use set_span_attribute() to capture relevant operational attributes such as the plugin runtime (for upload_system_plugin), system_plugin_id (for update_system_plugin, replace_system_plugin_package, and delete_system_plugin), and outcome of the operation to provide visibility into these high-value operational paths.Source: Coding guidelines
🤖 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/app/services/installed_plugin_service.py`:
- Around line 213-261: The install_system_plugin and
update_installed_plugin_from_system methods loop through variants and call
_copy_system_plugin_to_installed for each one, which commits after each
iteration. To make the operation atomic, refactor both methods to collect all
the results within a single transaction scope so that if any variant processing
fails mid-loop, the entire operation rolls back instead of leaving partial
state. Ensure that all calls to _copy_system_plugin_to_installed within the loop
are executed before the transaction commits, rather than committing after each
variant is processed.
- Around line 733-737: The code checks if installed_by_runtime exists but does
not verify that the installed object itself is valid before accessing
installed.id, which can cause a crash. Add a null/validity check on the
installed variable in addition to the installed_by_runtime check, so that
installed_id is only assigned to installed.id when both installed_by_runtime is
truthy AND installed is not None. Update the condition on the if statement that
assigns installed_id and sets install_state to "installed" to include a check
that installed is not None before attempting to access its id attribute.
In `@backend/tests/api/endpoints/test_plugins_api.py`:
- Around line 117-120: The test assertion using `all()` to verify version
numbers in the update_response.json()["items"] list will incorrectly pass when
the items list is empty, since Python's all() returns True for empty sequences.
To prevent this false positive, add an assertion that explicitly checks that
update_response.json()["items"] is not empty before or in conjunction with the
existing all() check to ensure the test properly validates that at least one
item was returned with the correct version.
In `@executor/modes/local/capabilities.py`:
- Around line 1524-1527: The code path returned for Codex plugins at the
_plugin_key method (lines 1524-1527) may conflict with existing local user
plugins. When _ensure_runtime_symlink is called at line 1333 to establish the
symlink, it raises an exception if the path is already occupied, which aborts
the entire capability sync. Wrap the _ensure_runtime_symlink call in a
try-except block to catch the exception when a path is occupied, and instead of
raising and aborting the sync, handle it as a per-plugin failure by logging the
error and returning a failure status that allows the sync to continue processing
other plugins.
- Around line 1506-1512: The _plugin_key method now returns runtime-specific
keys when runtime is "claudecode" or "codex", but the call to _plugin_key in the
apply_sync method around line 965 does not pass the runtime parameter when
computing desired plugin keys for replace mode. Update the _plugin_key call in
apply_sync to include the runtime parameter so that runtime-specific keys are
properly considered and legacy base-key records do not remain as stale data
without cleanup.
In `@frontend/src/apis/admin.ts`:
- Around line 621-625: The metadata field in AdminSystemPlugin is typed as
Record<string, unknown>, which is too loose and masks a contract mismatch where
the backend returns plugin IDs at metadata.labels.id but the consumer code
attempts to access metadata.id. Tighten the metadata type definition to properly
specify the nested labels structure with its id field, then locate and update
all consumer code that accesses plugin.metadata.id to instead use
plugin.metadata.labels.id to correctly extract the plugin ID.
In `@frontend/src/features/admin/components/SystemPluginList.tsx`:
- Around line 238-257: The delete confirmation button is not disabled while the
handleDelete function is executing, allowing users to click it multiple times
and trigger duplicate API calls. Add a disabled attribute to the delete
confirmation button that binds to the saving state variable. When saving is true
(during the deleteSystemPlugin API call), the button should be disabled to
prevent multiple submissions. Check the button rendering code that triggers
handleDelete and ensure it includes the disabled state guard.
- Around line 344-367: The action buttons in the SystemPluginList component
(specifically the edit button with openEditDialog, replace button with
openReplaceDialog, and the delete button below) currently use className "h-8
w-8" which is 32x32 pixels, below the minimum touch accessibility requirement of
44x44 pixels. Update the className prop on all three action buttons to use "h-11
min-w-[44px]" instead of "h-8 w-8" to meet the mobile touch target size
requirements.
- Around line 46-49: The metadataId function can return NaN when
plugin.metadata.id is malformed or missing (since Number() of an invalid string
returns NaN), which then causes issues when used as React keys and in API calls
like /admin/plugins/${pluginId}. Add validation in the metadataId function after
the Number conversion to check if the result is NaN and handle it
appropriately—either by throwing an error with a descriptive message about
invalid plugin metadata or by providing a safe fallback that prevents NaN from
leaking into React keys and API endpoints.
- Around line 448-583: Multiple Cancel and action buttons in the dialogs are
missing data-testid attributes, breaking the testing contract for interactive
elements. Add descriptive data-testid attributes to the Cancel buttons in the
edit dialog (the button calling setIsEditDialogOpen inside DialogFooter), the
replace dialog (the button calling setIsReplaceDialogOpen inside DialogFooter),
and both the Cancel and Action buttons in the delete AlertDialog
(AlertDialogCancel and AlertDialogAction). Follow the existing naming pattern
used in other dialogs like system-plugin-edit-submit and
system-plugin-replace-submit to maintain consistency.
In `@wework/src/components/plugins/PluginManagementWorkspace.test.tsx`:
- Around line 49-53: The beforeEach hook calls mockManagementFetch() to stub the
global fetch, but there is no corresponding afterEach hook to restore or tear
down this stub after each test completes. This causes test isolation violations
and potential state leakage between tests. Add an afterEach hook immediately
after the beforeEach block that restores the global fetch to its original state
or clears any stubbing/mocking that was applied by mockManagementFetch().
In `@wework/src/components/plugins/PluginsWorkspace.tsx`:
- Around line 377-404: The plugin action buttons in the conditional render block
(for 'update_available', 'installed', and default install states) are using
`sm:h-8 sm:w-8 sm:rounded-lg` which shrinks touch targets to 32px starting at
the 640px breakpoint. Since this repo defines mobile as screens up to 767px,
these buttons fall below the required 44×44px touch target minimum on mobile
devices. Replace all instances of the `sm:` prefixes with `md:` prefixes in the
size and border-radius classes for all three button/span elements to ensure the
smaller sizes only apply at the tablet breakpoint (768px+) and above, keeping
them at 44×44px through the full mobile range.
- Around line 799-807: The functions `installCatalogPlugin` and
`updateCatalogPlugin` do not handle errors from the async API calls
`pluginApi.installSystemPlugin()` and `pluginApi.updateSystemPlugin()`. Add
try-catch blocks around both async operations to catch rejections, then handle
the errors appropriately by logging them and updating the UI state to reflect
the failure (such as setting an error message or showing a notification). This
ensures that promise rejections are not left unhandled and the user receives
feedback when installation or update operations fail.
---
Outside diff comments:
In `@wework/src/components/plugins/PluginsWorkspace.tsx`:
- Around line 661-713: Both the installProviderServer and
uninstallProviderServer functions lack error handling for their promise chains.
Add a .catch() handler to the promise chain in installProviderServer (after the
.then() for mcpApi.installProviderMcp) and similarly add a .catch() handler to
the promise chain in uninstallProviderServer (after the .then() for
mcpApi.uninstallInstalledMcp). Each catch handler should display an appropriate
error notification to the user to inform them of the failure and allow them to
take corrective action.
---
Nitpick comments:
In `@backend/app/api/endpoints/admin/plugins.py`:
- Around line 27-117: Add OpenTelemetry tracing instrumentation to all five
admin plugin endpoints: list_system_plugins, upload_system_plugin,
update_system_plugin, replace_system_plugin_package, and delete_system_plugin.
Decorate each async endpoint function with `@trace_async`() and within each
function use set_span_attribute() to capture relevant operational attributes
such as the plugin runtime (for upload_system_plugin), system_plugin_id (for
update_system_plugin, replace_system_plugin_package, and delete_system_plugin),
and outcome of the operation to provide visibility into these high-value
operational paths.
In `@frontend/src/features/admin/components/SystemPluginList.tsx`:
- Around line 70-589: The SystemPluginList component is too large at 520 lines
and should be refactored into smaller, more maintainable pieces. Extract the
four dialog components (the Dialog and AlertDialog elements for upload, edit,
replace, and delete operations) into separate subcomponents named
SystemPluginUploadDialog, SystemPluginEditDialog, SystemPluginReplaceDialog, and
SystemPluginDeleteDialog. Additionally, create custom hooks to manage related
state: useSystemPlugins hook for the plugins list and loading/fetching logic,
usePluginDialogs hook to consolidate all dialog open/close state management, and
usePluginMutations hook to encapsulate the async handlers (handleUpload,
handleUpdateMetadata, handleToggleEnabled, handleReplacePackage, handleDelete).
Finally, extract the plugin list rendering logic into a separate
SystemPluginListView subcomponent. This will reduce the main component to under
50 lines for better maintainability and testability.
In `@wework/src/components/plugins/PluginsWorkspace.test.tsx`:
- Around line 63-503: The mockSystemSkillsFetch function combines multiple
unrelated API mocking concerns (skills, MCP providers, and plugins) into a
single large function, making it difficult to maintain and causing unnecessary
coupling between tests. Extract the different API endpoint branches into
separate, scenario-focused mock builder functions where each function handles
only its specific concern (e.g., one for skills APIs, one for MCP APIs, one for
plugin APIs). This allows tests to use only the mocks they need and makes
failures easier to localize to specific API concerns.
🪄 Autofix (Beta)
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
Run ID: b25747e2-b2cf-4cfd-99a7-dbbff45c686d
📒 Files selected for processing (33)
AGENTS.mdbackend/app/api/endpoints/admin/plugins.pybackend/app/api/endpoints/admin/router.pybackend/app/api/endpoints/installed_plugins.pybackend/app/schemas/installed_plugin.pybackend/app/services/device/capability_sync_service.pybackend/app/services/installed_plugin_service.pybackend/tests/api/endpoints/test_plugins_api.pybackend/tests/services/device/test_capability_sync_service.pybackend/tests/services/test_installed_plugin_upload.pydocs/en/developer-guide/system-plugin-management.mddocs/zh/developer-guide/system-plugin-management.mdexecutor/modes/local/capabilities.pyexecutor/tests/test_local_capability_sync_handler.pyfrontend/src/__tests__/features/admin/components/SystemPluginList.test.tsxfrontend/src/apis/admin.tsfrontend/src/app/admin/page.tsxfrontend/src/features/admin/components/AdminTabNav.tsxfrontend/src/features/admin/components/SystemPluginList.tsxfrontend/src/i18n/locales/en/admin.jsonfrontend/src/i18n/locales/zh-CN/admin.jsonwework/src/api/adapters.test.tswework/src/api/plugins.tswework/src/components/plugins/PluginCreateMenu.tsxwework/src/components/plugins/PluginManagementWorkspace.test.tsxwework/src/components/plugins/PluginManagementWorkspace.tsxwework/src/components/plugins/PluginUploadDialog.test.tsxwework/src/components/plugins/PluginUploadDialog.tsxwework/src/components/plugins/PluginsWorkspace.test.tsxwework/src/components/plugins/PluginsWorkspace.tsxwework/src/i18n/locales/en/common.jsonwework/src/i18n/locales/zh-CN/common.jsonwework/src/types/api.ts
💤 Files with no reviewable changes (3)
- wework/src/components/plugins/PluginUploadDialog.test.tsx
- wework/src/components/plugins/PluginUploadDialog.tsx
- wework/src/api/adapters.test.ts
Comment on lines
+213
to
+261
| def install_system_plugin( | ||
| self, *, db: Session, user_id: int, system_plugin_id: int | ||
| ) -> InstalledPluginListResponse: | ||
| system_row = self._get_enabled_system_plugin( | ||
| db, system_plugin_id=system_plugin_id | ||
| ) | ||
| variants = self._complete_enabled_variants_for_row(db, system_row) | ||
| installed_items = [] | ||
| for variant in self._ordered_variant_rows(variants): | ||
| existing = self._find_installed_system_plugin( | ||
| db, user_id=user_id, system_plugin_id=variant.id | ||
| ) | ||
| if existing and existing.is_active: | ||
| installed_items.append(self._kind_to_installed_plugin(existing)) | ||
| continue | ||
| installed_items.append( | ||
| self._copy_system_plugin_to_installed( | ||
| db, | ||
| user_id=user_id, | ||
| system_row=variant, | ||
| row=existing, | ||
| ) | ||
| ) | ||
| return InstalledPluginListResponse(items=installed_items) | ||
|
|
||
| def update_installed_plugin_from_system( | ||
| self, *, db: Session, user_id: int, system_plugin_id: int | ||
| ) -> InstalledPluginListResponse: | ||
| system_row = self._get_enabled_system_plugin( | ||
| db, system_plugin_id=system_plugin_id | ||
| ) | ||
| variants = self._complete_enabled_variants_for_row(db, system_row) | ||
| installed_items = [] | ||
| for variant in self._ordered_variant_rows(variants): | ||
| installed = self._find_installed_system_plugin( | ||
| db, user_id=user_id, system_plugin_id=variant.id | ||
| ) | ||
| if not installed or not installed.is_active: | ||
| installed = None | ||
| installed_items.append( | ||
| self._copy_system_plugin_to_installed( | ||
| db, | ||
| user_id=user_id, | ||
| system_row=variant, | ||
| row=installed, | ||
| ) | ||
| ) | ||
| return InstalledPluginListResponse(items=installed_items) | ||
|
|
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Install/update of runtime pairs is not atomic and can persist partial state.
Both pair operations loop runtimes, but _copy_system_plugin_to_installed commits each variant. A mid-loop failure leaves one runtime updated/installed and the other stale/missing.
🛠️ Proposed transactional pattern
def install_system_plugin(...):
@@
- installed_items = []
+ installed_items = []
for variant in self._ordered_variant_rows(variants):
@@
- installed_items.append(
- self._copy_system_plugin_to_installed(
+ installed_items.append(
+ self._copy_system_plugin_to_installed(
db,
user_id=user_id,
system_row=variant,
row=existing,
+ commit=False,
)
)
+ db.commit()
return InstalledPluginListResponse(items=installed_items)
def update_installed_plugin_from_system(...):
@@
- installed_items.append(
- self._copy_system_plugin_to_installed(
+ installed_items.append(
+ self._copy_system_plugin_to_installed(
db,
user_id=user_id,
system_row=variant,
row=installed,
+ commit=False,
)
)
+ db.commit()
return InstalledPluginListResponse(items=installed_items)
def _copy_system_plugin_to_installed(
@@
- ) -> InstalledPlugin:
+ commit: bool = True,
+ ) -> InstalledPlugin:
@@
- db.commit()
- db.refresh(row)
+ if commit:
+ db.commit()
+ db.refresh(row)
+ else:
+ db.flush()
return self._kind_to_installed_plugin(row)Also applies to: 770-815
🤖 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/app/services/installed_plugin_service.py` around lines 213 - 261, The
install_system_plugin and update_installed_plugin_from_system methods loop
through variants and call _copy_system_plugin_to_installed for each one, which
commits after each iteration. To make the operation atomic, refactor both
methods to collect all the results within a single transaction scope so that if
any variant processing fails mid-loop, the entire operation rolls back instead
of leaving partial state. Ensure that all calls to
_copy_system_plugin_to_installed within the loop are executed before the
transaction commits, rather than committing after each variant is processed.
Comment on lines
+733
to
+737
| install_state = "not_installed" | ||
| installed_id = None | ||
| if installed_by_runtime: | ||
| installed_id = installed.id | ||
| install_state = "installed" |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
installedPluginId assignment can crash when only one runtime remains installed.
installed is read from the claudecode slot, but installed_by_runtime may contain only codex (e.g., partial uninstall/state drift). installed.id then raises at runtime.
🐛 Proposed fix
- installed = installed_by_runtime.get("claudecode")
- installed_spec = self._get_spec(installed) if installed else {}
+ primary_installed = installed_by_runtime.get("claudecode")
+ if primary_installed is None and installed_by_runtime:
+ primary_installed = next(iter(installed_by_runtime.values()))
+ installed_spec = self._get_spec(primary_installed) if primary_installed else {}
installed_ref = installed_spec.get("packageRef") or {}
@@
if installed_by_runtime:
- installed_id = installed.id
+ installed_id = primary_installed.id if primary_installed else None
install_state = "installed"🤖 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/app/services/installed_plugin_service.py` around lines 733 - 737, The
code checks if installed_by_runtime exists but does not verify that the
installed object itself is valid before accessing installed.id, which can cause
a crash. Add a null/validity check on the installed variable in addition to the
installed_by_runtime check, so that installed_id is only assigned to
installed.id when both installed_by_runtime is truthy AND installed is not None.
Update the condition on the if statement that assigns installed_id and sets
install_state to "installed" to include a check that installed is not None
before attempting to access its id attribute.
Comment on lines
+117
to
+120
| assert update_response.status_code == 200 | ||
| assert all( | ||
| item["spec"]["version"] == "2.0.0" for item in update_response.json()["items"] | ||
| ) |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Prevent false-positive pass on empty update results.
Line 118 can pass on an empty items list (all([]) is True), so this test would miss a regression where /update returns nothing.
💡 Suggested fix
assert update_response.status_code == 200
- assert all(
- item["spec"]["version"] == "2.0.0" for item in update_response.json()["items"]
- )
+ updated_items = update_response.json()["items"]
+ assert len(updated_items) == 2
+ assert all(item["spec"]["version"] == "2.0.0" for item in updated_items)🤖 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/api/endpoints/test_plugins_api.py` around lines 117 - 120, The
test assertion using `all()` to verify version numbers in the
update_response.json()["items"] list will incorrectly pass when the items list
is empty, since Python's all() returns True for empty sequences. To prevent this
false positive, add an assertion that explicitly checks that
update_response.json()["items"] is not empty before or in conjunction with the
existing all() check to ensure the test properly validates that at least one
item was returned with the correct version.
Comment on lines
+1506
to
+1512
| def _plugin_key( | ||
| self, name: str, marketplace: str | None, runtime: str | None = None | ||
| ) -> str: | ||
| base_key = f"{name}@{marketplace}" if marketplace else name | ||
| if runtime in {"claudecode", "codex"}: | ||
| return f"{base_key}#{runtime}" | ||
| return base_key |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Include runtime when computing replace-mode desired plugin keys.
_plugin_key now persists runtime-specific keys, but apply_sync Line 965 still calls it without runtime. In replace mode, existing #claudecode/#codex records can be treated as stale, while legacy base-key records can be left behind without cleanup.
Proposed fix
desired_plugin_keys = {
- self._plugin_key(item.get("name"), self._plugin_marketplace(item))
+ self._plugin_key(
+ item.get("name"),
+ self._plugin_marketplace(item),
+ self._plugin_runtime(item),
+ )
for item in plugins
if isinstance(item, dict) and item.get("name")
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| def _plugin_key( | |
| self, name: str, marketplace: str | None, runtime: str | None = None | |
| ) -> str: | |
| base_key = f"{name}@{marketplace}" if marketplace else name | |
| if runtime in {"claudecode", "codex"}: | |
| return f"{base_key}#{runtime}" | |
| return base_key | |
| desired_plugin_keys = { | |
| self._plugin_key( | |
| item.get("name"), | |
| self._plugin_marketplace(item), | |
| self._plugin_runtime(item), | |
| ) | |
| for item in plugins | |
| if isinstance(item, dict) and item.get("name") | |
| } |
🤖 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 `@executor/modes/local/capabilities.py` around lines 1506 - 1512, The
_plugin_key method now returns runtime-specific keys when runtime is
"claudecode" or "codex", but the call to _plugin_key in the apply_sync method
around line 965 does not pass the runtime parameter when computing desired
plugin keys for replace mode. Update the _plugin_key call in apply_sync to
include the runtime parameter so that runtime-specific keys are properly
considered and legacy base-key records do not remain as stale data without
cleanup.
Comment on lines
+1524
to
+1527
| if runtime == "codex": | ||
| return self.codex_plugins_dir / self._safe_path_segment( | ||
| self._plugin_key(name, marketplace) | ||
| ) |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Handle occupied Codex plugin paths without aborting the sync.
Codex-only installs now target codex_plugins_dir directly. If a local user plugin already occupies that path, _ensure_runtime_symlink raises at Line 1333 and aborts the whole capability sync instead of returning a per-plugin failure.
Proposed fix
key = self._plugin_key(name, marketplace, runtime)
runtime_link = self._plugin_runtime_path(item, name, marketplace, runtime)
store_path = self._plugin_store_path(item, name, marketplace)
+ managed_record = (self.store.manifest.load().get("plugins") or {}).get(key)
+ if runtime == "codex" and self._is_runtime_path_occupied_by_local_user(
+ runtime_link,
+ managed_record,
+ ):
+ results.append(
+ {
+ "id": item.get("installed_plugin_id"),
+ "name": name,
+ "status": "failed",
+ "error": "Runtime Plugin path is occupied by a local user item",
+ }
+ )
+ continue
expected_checksum = item.get("checksum")🤖 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 `@executor/modes/local/capabilities.py` around lines 1524 - 1527, The code path
returned for Codex plugins at the _plugin_key method (lines 1524-1527) may
conflict with existing local user plugins. When _ensure_runtime_symlink is
called at line 1333 to establish the symlink, it raises an exception if the path
is already occupied, which aborts the entire capability sync. Wrap the
_ensure_runtime_symlink call in a try-except block to catch the exception when a
path is occupied, and instead of raising and aborting the sync, handle it as a
per-plugin failure by logging the error and returning a failure status that
allows the sync to continue processing other plugins.
Comment on lines
+344
to
+367
| <Button | ||
| variant="ghost" | ||
| size="icon" | ||
| className="h-8 w-8" | ||
| onClick={() => openEditDialog(plugin)} | ||
| title={t('system_plugins.actions.edit')} | ||
| data-testid={`system-plugin-edit-${pluginId}`} | ||
| > | ||
| <Pencil className="w-4 h-4" /> | ||
| </Button> | ||
| <Button | ||
| variant="ghost" | ||
| size="icon" | ||
| className="h-8 w-8" | ||
| onClick={() => openReplaceDialog(plugin)} | ||
| title={t('system_plugins.actions.replace_package')} | ||
| data-testid={`system-plugin-replace-${pluginId}`} | ||
| > | ||
| <RefreshCw className="w-4 h-4" /> | ||
| </Button> | ||
| <Button | ||
| variant="ghost" | ||
| size="icon" | ||
| className="h-8 w-8 hover:text-error" |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Increase icon button hit areas to meet touch accessibility minimums.
These action buttons are 32x32 (h-8 w-8), below the required mobile touch target size.
🔧 Suggested fix
- className="h-8 w-8"
+ className="h-11 min-w-[44px]"As per coding guidelines, touch-friendly interactive buttons must be at least 44px × 44px using h-11 min-w-[44px].
🤖 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 `@frontend/src/features/admin/components/SystemPluginList.tsx` around lines 344
- 367, The action buttons in the SystemPluginList component (specifically the
edit button with openEditDialog, replace button with openReplaceDialog, and the
delete button below) currently use className "h-8 w-8" which is 32x32 pixels,
below the minimum touch accessibility requirement of 44x44 pixels. Update the
className prop on all three action buttons to use "h-11 min-w-[44px]" instead of
"h-8 w-8" to meet the mobile touch target size requirements.
Source: Coding guidelines
Comment on lines
+448
to
+583
| <Button variant="outline" onClick={() => setIsUploadDialogOpen(false)}> | ||
| {t('common.cancel')} | ||
| </Button> | ||
| <Button | ||
| variant="primary" | ||
| onClick={handleUpload} | ||
| disabled={saving} | ||
| data-testid="system-plugin-upload-submit" | ||
| > | ||
| {saving && <Loader2 className="w-4 h-4 animate-spin" />} | ||
| {t('system_plugins.actions.upload')} | ||
| </Button> | ||
| </DialogFooter> | ||
| </DialogContent> | ||
| </Dialog> | ||
|
|
||
| <Dialog open={isEditDialogOpen} onOpenChange={setIsEditDialogOpen}> | ||
| <DialogContent className="max-w-lg"> | ||
| <DialogHeader> | ||
| <DialogTitle>{t('system_plugins.edit.title')}</DialogTitle> | ||
| </DialogHeader> | ||
| <div className="space-y-4 py-4"> | ||
| <div className="space-y-2"> | ||
| <Label htmlFor="system-plugin-display-name"> | ||
| {t('system_plugins.fields.display_name')} | ||
| </Label> | ||
| <Input | ||
| id="system-plugin-display-name" | ||
| value={formData.displayName} | ||
| onChange={event => | ||
| setFormData(prev => ({ ...prev, displayName: event.target.value })) | ||
| } | ||
| data-testid="system-plugin-display-name-input" | ||
| /> | ||
| </div> | ||
| <div className="space-y-2"> | ||
| <Label htmlFor="system-plugin-description"> | ||
| {t('system_plugins.fields.description')} | ||
| </Label> | ||
| <Textarea | ||
| id="system-plugin-description" | ||
| value={formData.description} | ||
| onChange={event => | ||
| setFormData(prev => ({ ...prev, description: event.target.value })) | ||
| } | ||
| className="min-h-[120px]" | ||
| data-testid="system-plugin-description-input" | ||
| /> | ||
| </div> | ||
| <div className="flex items-center justify-between"> | ||
| <Label htmlFor="system-plugin-edit-enabled"> | ||
| {t('system_plugins.fields.enabled')} | ||
| </Label> | ||
| <Switch | ||
| id="system-plugin-edit-enabled" | ||
| checked={formData.enabled} | ||
| onCheckedChange={checked => setFormData(prev => ({ ...prev, enabled: checked }))} | ||
| data-testid="system-plugin-edit-enabled-switch" | ||
| /> | ||
| </div> | ||
| </div> | ||
| <DialogFooter> | ||
| <Button variant="outline" onClick={() => setIsEditDialogOpen(false)}> | ||
| {t('common.cancel')} | ||
| </Button> | ||
| <Button | ||
| variant="primary" | ||
| onClick={handleUpdateMetadata} | ||
| disabled={saving} | ||
| data-testid="system-plugin-edit-submit" | ||
| > | ||
| {saving && <Loader2 className="w-4 h-4 animate-spin" />} | ||
| {t('common.save')} | ||
| </Button> | ||
| </DialogFooter> | ||
| </DialogContent> | ||
| </Dialog> | ||
|
|
||
| <Dialog open={isReplaceDialogOpen} onOpenChange={setIsReplaceDialogOpen}> | ||
| <DialogContent className="max-w-lg"> | ||
| <DialogHeader> | ||
| <DialogTitle>{t('system_plugins.replace.title')}</DialogTitle> | ||
| <DialogDescription> | ||
| {t('system_plugins.replace.description', { | ||
| name: selectedPlugin | ||
| ? selectedPlugin.spec.displayName || pluginName(selectedPlugin) | ||
| : '', | ||
| })} | ||
| </DialogDescription> | ||
| </DialogHeader> | ||
| <div className="space-y-2 py-4"> | ||
| <Label htmlFor="system-plugin-replace-file"> | ||
| {t('system_plugins.fields.zip_file')} * | ||
| </Label> | ||
| <Input | ||
| id="system-plugin-replace-file" | ||
| type="file" | ||
| accept=".zip,application/zip" | ||
| onChange={event => setSelectedFile(event.target.files?.[0] || null)} | ||
| data-testid="system-plugin-replace-file-input" | ||
| /> | ||
| </div> | ||
| <DialogFooter> | ||
| <Button variant="outline" onClick={() => setIsReplaceDialogOpen(false)}> | ||
| {t('common.cancel')} | ||
| </Button> | ||
| <Button | ||
| variant="primary" | ||
| onClick={handleReplacePackage} | ||
| disabled={saving} | ||
| data-testid="system-plugin-replace-submit" | ||
| > | ||
| {saving && <Loader2 className="w-4 h-4 animate-spin" />} | ||
| {t('system_plugins.actions.replace_package')} | ||
| </Button> | ||
| </DialogFooter> | ||
| </DialogContent> | ||
| </Dialog> | ||
|
|
||
| <AlertDialog open={isDeleteDialogOpen} onOpenChange={setIsDeleteDialogOpen}> | ||
| <AlertDialogContent> | ||
| <AlertDialogHeader> | ||
| <AlertDialogTitle>{t('system_plugins.delete.title')}</AlertDialogTitle> | ||
| <AlertDialogDescription> | ||
| {t('system_plugins.delete.description', { | ||
| name: selectedPlugin | ||
| ? selectedPlugin.spec.displayName || pluginName(selectedPlugin) | ||
| : '', | ||
| })} | ||
| </AlertDialogDescription> | ||
| </AlertDialogHeader> | ||
| <AlertDialogFooter> | ||
| <AlertDialogCancel>{t('common.cancel')}</AlertDialogCancel> | ||
| <AlertDialogAction onClick={handleDelete} className="bg-error hover:bg-error/90"> | ||
| {t('common.delete')} | ||
| </AlertDialogAction> |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add data-testid to newly added dialog action buttons.
Several new buttons (Cancel / confirm actions) are interactive but lack data-testid, which breaks the stated testing contract for new React UI controls.
🔧 Suggested fix
-<Button variant="outline" onClick={() => setIsUploadDialogOpen(false)}>
+<Button
+ variant="outline"
+ onClick={() => setIsUploadDialogOpen(false)}
+ data-testid="system-plugin-upload-cancel-button"
+>
@@
-<Button variant="outline" onClick={() => setIsEditDialogOpen(false)}>
+<Button
+ variant="outline"
+ onClick={() => setIsEditDialogOpen(false)}
+ data-testid="system-plugin-edit-cancel-button"
+>
@@
-<Button variant="outline" onClick={() => setIsReplaceDialogOpen(false)}>
+<Button
+ variant="outline"
+ onClick={() => setIsReplaceDialogOpen(false)}
+ data-testid="system-plugin-replace-cancel-button"
+>
@@
-<AlertDialogCancel>{t('common.cancel')}</AlertDialogCancel>
-<AlertDialogAction onClick={handleDelete} className="bg-error hover:bg-error/90">
+<AlertDialogCancel data-testid="system-plugin-delete-cancel-button">
+ {t('common.cancel')}
+</AlertDialogCancel>
+<AlertDialogAction
+ onClick={handleDelete}
+ className="bg-error hover:bg-error/90"
+ data-testid="system-plugin-delete-confirm-button"
+>As per coding guidelines, new interactive elements must include descriptive data-testid attributes.
🤖 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 `@frontend/src/features/admin/components/SystemPluginList.tsx` around lines 448
- 583, Multiple Cancel and action buttons in the dialogs are missing data-testid
attributes, breaking the testing contract for interactive elements. Add
descriptive data-testid attributes to the Cancel buttons in the edit dialog (the
button calling setIsEditDialogOpen inside DialogFooter), the replace dialog (the
button calling setIsReplaceDialogOpen inside DialogFooter), and both the Cancel
and Action buttons in the delete AlertDialog (AlertDialogCancel and
AlertDialogAction). Follow the existing naming pattern used in other dialogs
like system-plugin-edit-submit and system-plugin-replace-submit to maintain
consistency.
Source: Coding guidelines
Comment on lines
+49
to
+53
| describe('PluginManagementWorkspace', () => { | ||
| beforeEach(() => { | ||
| mockManagementFetch() | ||
| }) | ||
|
|
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Restore global stubs after each test to keep isolation.
fetch is globally stubbed in beforeEach but not torn down in this file. That can leak state into other tests and create flaky order-dependent failures.
As per coding guidelines **/*.test.{py,ts,tsx,js,jsx}: “Follow AAA pattern (Arrange, Act, Assert) in test code with mocked external services and independent isolated tests”.
💡 Suggested fix
-import { beforeEach, describe, expect, test, vi } from 'vitest'
+import { afterEach, beforeEach, describe, expect, test, vi } from 'vitest'
describe('PluginManagementWorkspace', () => {
beforeEach(() => {
mockManagementFetch()
})
+
+ afterEach(() => {
+ vi.unstubAllGlobals()
+ vi.restoreAllMocks()
+ })📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| describe('PluginManagementWorkspace', () => { | |
| beforeEach(() => { | |
| mockManagementFetch() | |
| }) | |
| import { afterEach, beforeEach, describe, expect, test, vi } from 'vitest' | |
| describe('PluginManagementWorkspace', () => { | |
| beforeEach(() => { | |
| mockManagementFetch() | |
| }) | |
| afterEach(() => { | |
| vi.unstubAllGlobals() | |
| vi.restoreAllMocks() | |
| }) |
🤖 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/PluginManagementWorkspace.test.tsx` around
lines 49 - 53, The beforeEach hook calls mockManagementFetch() to stub the
global fetch, but there is no corresponding afterEach hook to restore or tear
down this stub after each test completes. This causes test isolation violations
and potential state leakage between tests. Add an afterEach hook immediately
after the beforeEach block that restores the global fetch to its original state
or clears any stubbing/mocking that was applied by mockManagementFetch().
Source: Coding guidelines
Comment on lines
+799
to
807
| const installCatalogPlugin = async (plugin: PluginCatalogItem) => { | ||
| const installed = await pluginApi.installSystemPlugin(plugin.id) | ||
| updateCatalogAfterInstall(plugin.id, installed) | ||
| } | ||
|
|
||
| const updateCatalogPlugin = async (plugin: PluginCatalogItem) => { | ||
| const installed = await pluginApi.updateSystemPlugin(plugin.id) | ||
| updateCatalogAfterInstall(plugin.id, installed) | ||
| } |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Handle catalog install/update failures explicitly.
installCatalogPlugin and updateCatalogPlugin can reject, but callers invoke them with void in click handlers. Rejections become unhandled and the UI never surfaces failure state.
Proposed fix
const installCatalogPlugin = async (plugin: PluginCatalogItem) => {
- const installed = await pluginApi.installSystemPlugin(plugin.id)
- updateCatalogAfterInstall(plugin.id, installed)
+ try {
+ const installed = await pluginApi.installSystemPlugin(plugin.id)
+ updateCatalogAfterInstall(plugin.id, installed)
+ setPluginCatalogState(previous => ({ ...previous, error: null }))
+ } catch (error) {
+ setPluginCatalogState(previous => ({
+ ...previous,
+ error: error instanceof Error ? error.message : 'Failed to install plugin',
+ }))
+ }
}
const updateCatalogPlugin = async (plugin: PluginCatalogItem) => {
- const installed = await pluginApi.updateSystemPlugin(plugin.id)
- updateCatalogAfterInstall(plugin.id, installed)
+ try {
+ const installed = await pluginApi.updateSystemPlugin(plugin.id)
+ updateCatalogAfterInstall(plugin.id, installed)
+ setPluginCatalogState(previous => ({ ...previous, error: null }))
+ } catch (error) {
+ setPluginCatalogState(previous => ({
+ ...previous,
+ error: error instanceof Error ? error.message : 'Failed to update plugin',
+ }))
+ }
}🤖 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 799 - 807,
The functions `installCatalogPlugin` and `updateCatalogPlugin` do not handle
errors from the async API calls `pluginApi.installSystemPlugin()` and
`pluginApi.updateSystemPlugin()`. Add try-catch blocks around both async
operations to catch rejections, then handle the errors appropriately by logging
them and updating the UI state to reflect the failure (such as setting an error
message or showing a notification). This ensures that promise rejections are not
left unhandled and the user receives feedback when installation or update
operations fail.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
wework/src/components/plugins/PluginManagementWorkspace.test.tsx (1)
138-149: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert the toggle payload, not only the HTTP method.
Line 138-149 verifies fan-out
PUTs, but it doesn’t verifyenabledis actually toggled in the request body. A broken payload serializer would still pass this test.Suggested assertion hardening
expect(fetch).toHaveBeenCalledWith( '/api/plugins/installed/501', - expect.objectContaining({ method: 'PUT' }) + expect.objectContaining({ + method: 'PUT', + body: expect.stringContaining('"enabled":false'), + }) )🤖 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/PluginManagementWorkspace.test.tsx` around lines 138 - 149, The test assertions for the three fetch calls to toggle plugins (501, 502, 503) only verify the HTTP method is PUT but do not validate the request body payload. Update each of the three expect(fetch).toHaveBeenCalledWith calls to additionally verify that the request body contains the correct toggled enabled value by extending the expect.objectContaining matcher to include a body property that validates the JSON payload contains the appropriate enabled boolean value.wework/src/components/plugins/PluginManagementWorkspace.tsx (1)
237-912: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftSplit
PluginManagementWorkspaceinto focused hooks/components.This function is now a large multi-responsibility unit (data loading, optimistic mutation orchestration, filtering, and rendering), which makes reasoning and regression testing harder. Extracting plugin-variant state/mutation logic (e.g.,
useInstalledPlugins) and provider logic into dedicated hooks would materially improve maintainability.Based on learnings “Manage complexity through extraction: If a module is complex, prioritize extracting common logic into utilities or creating new modules rather than adding more complexity. When in doubt, split rather than extend.” As per coding guidelines “
**/*.{py,ts,tsx,jsx,js}: Maximum function length: 50 lines preferred”.🤖 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/PluginManagementWorkspace.tsx` around lines 237 - 912, The PluginManagementWorkspace function exceeds 50 lines and has multiple responsibilities that should be extracted into focused custom hooks. Extract the MCP-related state management (installedMcps, isLoadingMcps, mcpProviders, providerServers, providerErrors, providerLoadingByKey, providerSavingByKey, providerTokenInputs) and their associated mutations (toggleInstalledMcp, uninstallInstalledMcp, loadProviderServers, saveProviderToken, installProviderServer) into a dedicated hook like useInstalledMcps. Similarly, extract plugin-related state (installedPlugins, isLoadingPlugins, selectedPluginId) and mutations (toggleInstalledPlugin, togglePluginComponent, uninstallInstalledPlugin) into useInstalledPlugins, and extract skill-related state (installedSkills, isLoadingSkills) and mutations (toggleInstalledSkill, uninstallInstalledSkill) into useInstalledSkills. This will reduce the main component to focus solely on rendering and orchestration, improving testability and maintainability.Sources: Coding guidelines, Learnings
🤖 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/PluginManagementWorkspace.tsx`:
- Around line 454-466: The Promise.all() pattern in this grouped plugin mutation
creates cross-runtime state drift when partial failures occur because it reverts
the entire UI to the old plugin state if any single updateInstalledPlugin call
fails, even though some calls may have already succeeded on the backend. Replace
Promise.all() with Promise.allSettled() to handle each variant request
individually, then inspect the settled results array to determine which
updateInstalledPlugin calls succeeded versus failed. Update the UI state
reconciliation logic to only revert the items that actually failed while keeping
the successfully updated items, ensuring the UI accurately reflects the partial
mutation state on the backend.
---
Nitpick comments:
In `@wework/src/components/plugins/PluginManagementWorkspace.test.tsx`:
- Around line 138-149: The test assertions for the three fetch calls to toggle
plugins (501, 502, 503) only verify the HTTP method is PUT but do not validate
the request body payload. Update each of the three
expect(fetch).toHaveBeenCalledWith calls to additionally verify that the request
body contains the correct toggled enabled value by extending the
expect.objectContaining matcher to include a body property that validates the
JSON payload contains the appropriate enabled boolean value.
In `@wework/src/components/plugins/PluginManagementWorkspace.tsx`:
- Around line 237-912: The PluginManagementWorkspace function exceeds 50 lines
and has multiple responsibilities that should be extracted into focused custom
hooks. Extract the MCP-related state management (installedMcps, isLoadingMcps,
mcpProviders, providerServers, providerErrors, providerLoadingByKey,
providerSavingByKey, providerTokenInputs) and their associated mutations
(toggleInstalledMcp, uninstallInstalledMcp, loadProviderServers,
saveProviderToken, installProviderServer) into a dedicated hook like
useInstalledMcps. Similarly, extract plugin-related state (installedPlugins,
isLoadingPlugins, selectedPluginId) and mutations (toggleInstalledPlugin,
togglePluginComponent, uninstallInstalledPlugin) into useInstalledPlugins, and
extract skill-related state (installedSkills, isLoadingSkills) and mutations
(toggleInstalledSkill, uninstallInstalledSkill) into useInstalledSkills. This
will reduce the main component to focus solely on rendering and orchestration,
improving testability and maintainability.
🪄 Autofix (Beta)
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
Run ID: 55f2e3c3-074e-48be-8e82-d4142aedf1f8
📒 Files selected for processing (7)
wework/src/components/plugins/PluginManagementRows.tsxwework/src/components/plugins/PluginManagementWorkspace.test.tsxwework/src/components/plugins/PluginManagementWorkspace.tsxwework/src/components/plugins/PluginsWorkspace.test.tsxwework/src/components/plugins/PluginsWorkspace.tsxwework/src/i18n/locales/en/common.jsonwework/src/i18n/locales/zh-CN/common.json
💤 Files with no reviewable changes (2)
- wework/src/i18n/locales/zh-CN/common.json
- wework/src/i18n/locales/en/common.json
🚧 Files skipped from review as they are similar to previous changes (2)
- wework/src/components/plugins/PluginsWorkspace.test.tsx
- wework/src/components/plugins/PluginsWorkspace.tsx
Comment on lines
+454
to
+466
| Promise.all( | ||
| plugin.ids.map(pluginId => | ||
| pluginApi.updateInstalledPlugin(pluginId, { enabled: nextEnabled }) | ||
| ) | ||
| }) | ||
| ) | ||
| .then(updatedItems => { | ||
| const [nextItem] = groupInstalledPlugins(updatedItems) | ||
| if (!nextItem) return | ||
| setInstalledPlugins(previous => previous.map(item => (item.id === id ? nextItem : item))) | ||
| }) | ||
| .catch(() => { | ||
| setInstalledPlugins(previous => previous.map(item => (item.id === id ? plugin : item))) | ||
| }) |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Handle partial success for grouped variant mutations.
Lines 454-528 use Promise.all(...) + full rollback on any rejection. If one variant request fails but others succeed, UI is reverted to the old grouped snapshot while backend is already partially mutated. This creates cross-runtime state drift.
Safer reconciliation pattern
- Promise.all(plugin.ids.map(...))
- .then(updatedItems => { ... })
- .catch(() => {
- setInstalledPlugins(previous => previous.map(item => (item.id === id ? plugin : item)))
- })
+ Promise.allSettled(plugin.ids.map(...))
+ .finally(() => {
+ pluginApi.listInstalledPlugins()
+ .then(response => setInstalledPlugins(groupInstalledPlugins(response.items)))
+ .catch(() => {
+ // keep optimistic state or surface error toast; avoid restoring stale grouped snapshot
+ })
+ })Also applies to: 501-515, 524-528
🤖 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/PluginManagementWorkspace.tsx` around lines 454
- 466, The Promise.all() pattern in this grouped plugin mutation creates
cross-runtime state drift when partial failures occur because it reverts the
entire UI to the old plugin state if any single updateInstalledPlugin call
fails, even though some calls may have already succeeded on the backend. Replace
Promise.all() with Promise.allSettled() to handle each variant request
individually, then inspect the settled results array to determine which
updateInstalledPlugin calls succeeded versus failed. Update the UI state
reconciliation logic to only revert the items that actually failed while keeping
the successfully updated items, ensuring the UI accurately reflects the partial
mutation state on the backend.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit