Repository navigation
feat: wework支持安装插件 - #1234
Merged
Merged
feat: wework支持安装插件#1234
Conversation
Contributor
📝 WalkthroughWalkthroughAdds Claude Code plugin management end-to-end: ZIP parsing, on-disk package storage and DB-backed installed-plugin records, REST CRUD endpoints (/plugins), executor-side secure download/extract and capability sync integration, and full frontend UI for listing, uploading, detail view, per-component toggles, and uninstall. ChangesPlugin Management End-to-End
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
|
![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: 8
🧹 Nitpick comments (6)
wework/src/api/plugins.ts (1)
24-26: 💤 Low valueConsider making the
enabledflag a parameter.The
enabledfield is currently hardcoded to'true'when uploading a plugin. Consider making this a parameter touploadPlugin(file: File, enabled = true)for greater flexibility, in case you later need to support uploading plugins in a disabled state.🤖 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/plugins.ts` around lines 24 - 26, The uploadPlugin function currently hardcodes enabled to 'true'; change its signature to accept an enabled parameter (e.g., uploadPlugin(file: File, enabled = true)) and use that parameter when building the FormData (replace formData.append('enabled', 'true') with formData.append('enabled', String(enabled))). Update any call sites of uploadPlugin if they need to upload a disabled plugin.wework/src/components/plugins/PluginDetailView.tsx (1)
79-85: ⚡ Quick winMCP component description is hardcoded instead of using actual metadata.
The MCP components have their description hardcoded to
'MCP server'(line 84), while all other component types (skills, commands, agents, hooks, lsps, monitors, bins) use the actualdescriptionorpathfrom the item. This creates an inconsistency and loses potentially useful information about each MCP server.Consider following the same pattern as other components:
Proposed fix
...components.mcps.map((item) => ({ key: `mcp-${item.name}`, componentKey: `mcp:${item.name}`, type: 'mcp', name: item.name, - description: 'MCP server', + description: item.description || item.command || 'MCP server', })),🤖 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/PluginDetailView.tsx` around lines 79 - 85, The MCP entries in the components.mcps.map are using a hardcoded description ('MCP server') instead of the item's metadata; update the mapping (the block that creates objects with key `mcp-${item.name}`, componentKey `mcp:${item.name}`, and type 'mcp') to set description to the actual metadata, e.g. use item.description || item.path || item.name (or similar fallback) so MCPs follow the same pattern as skills/commands/agents/hooks/lsps/monitors/bins.wework/src/components/plugins/PluginUploadDialog.tsx (1)
76-81: 💤 Low valueNo client-side size guard despite the 50MB hint.
The hint advertises a 50MB limit but the dialog never validates file size, unlike
SkillUploadDialog(which enforces its 10MB limit viaplugins_skill_upload_size_error). Large files will only fail after a full upload round-trip. Consider validatingnextFile.sizeinselectFile.🤖 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/PluginUploadDialog.tsx` around lines 76 - 81, The UI advertises a 50MB limit but PluginUploadDialog never checks file size client-side; update the selectFile handler in PluginUploadDialog to validate nextFile.size and reject files >50*1024*1024 bytes, setting the same error flow used by SkillUploadDialog (use the i18n key plugins_skill_upload_size_error or add a plugins_plugin_upload_size_error if preferred) and clear/reset state appropriately so the large file is rejected before upload starts. Ensure you reference the selectFile function and nextFile.size when adding the check and trigger the existing error display path used for skill uploads.backend/app/api/endpoints/installed_plugins.py (1)
26-120: ⚡ Quick winTrace the new plugin endpoints.
These handlers are now the install/update/remove control plane, but they do not use
@trace_async/@trace_syncor attach useful span attributes such asuser_id,installed_id, and upload size.As per coding guidelines, "Use OpenTelemetry tracing decorators:
@trace_asyncor@trace_syncfor entire functions, andadd_span_event()orset_span_attribute()for adding details to spans".🤖 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/installed_plugins.py` around lines 26 - 120, Add OpenTelemetry tracing to the plugin endpoint handlers by decorating synchronous handlers (list_installed_plugins, download_installed_plugin) with `@trace_sync` and async handlers (upload_plugin, update_installed_plugin, uninstall_installed_plugin) with `@trace_async`; inside each handler call set_span_attribute to record user_id and relevant resource ids (e.g., installed_id) and for upload_plugin also set upload_size (len(content)) and add_span_event for filename; ensure the decorators are imported and used consistently and keep tracing calls near the start of each function so spans capture the whole handler execution.backend/app/services/claude_plugin_parser.py (1)
23-26: ⚡ Quick winTrace and document the parser entrypoint.
parse_package()is a new public backend entrypoint for untrusted uploads, but it has neither a method docstring nor a@trace_syncspan with package-level attributes. That will make parse failures much harder to diagnose in production.As per coding guidelines, "Use descriptive names and docstrings for public functions and classes in Python code" and "Use OpenTelemetry tracing decorators:
@trace_asyncor@trace_syncfor entire functions, andadd_span_event()orset_span_attribute()for adding details to spans".🤖 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/claude_plugin_parser.py` around lines 23 - 26, Add a concise docstring to the ClaudePluginParser class and a method docstring for parse_package describing its purpose (parsing and validating untrusted Claude Code plugin ZIP bytes) and its return type PluginUploadInfo; decorate parse_package with `@trace_sync` and inside the function set span attributes (e.g., package_size_bytes, package_sha256 or filename if available) and add span events around major steps (e.g., "received_package", "validation_started", "validation_failed") so failures are traceable; ensure any caught exceptions call add_span_event()/set_span_attribute with error details before re-raising or returning an error result.backend/app/services/installed_plugin_service.py (1)
31-159: ⚡ Quick winInstrument the new service API surface.
upload_plugin(),list_installed_plugins(),update_installed_plugin(),uninstall_installed_plugin(), andpackage_path_for_download()are new public backend entrypoints, but none of them carry method docstrings or tracing decorators. That leaves install/update failures opaque in production.As per coding guidelines, "Use descriptive names and docstrings for public functions and classes in Python code" and "Use OpenTelemetry tracing decorators:
@trace_asyncor@trace_syncfor entire functions, andadd_span_event()orset_span_attribute()for adding details to spans".🤖 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 31 - 159, Add docstrings and OpenTelemetry tracing to the new public methods upload_plugin, list_installed_plugins, update_installed_plugin, uninstall_installed_plugin, and package_path_for_download: add a one-sentence docstring explaining the purpose and parameters for each method, decorate each method with `@trace_sync` (or `@trace_async` if converted to async) and instrument key steps with add_span_event() and set_span_attribute() calls — e.g., log start/finish, user_id and installed_id, parsed.name/checksum/filename in upload_plugin, updated fields in update_installed_plugin, installState in uninstall_installed_plugin, and storageKey/path existence checks in package_path_for_download; ensure you record errors/exceptions to spans before raising HTTPException or returning.
🤖 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/api/endpoints/installed_plugins.py`:
- Around line 43-57: In upload_plugin (function upload_plugin in
installed_plugins.py) avoid using await file.read() which buffers the whole
upload; instead enforce the 50MB cap before buffering by first checking the
request/file metadata (Content-Length) and reject with HTTPException(413) if
present and >50MB, and otherwise read the UploadFile in chunks (e.g., loop
calling await file.read(chunk_size)) accumulating bytes up to the 50MB limit and
immediately raise HTTPException(413) when the limit is exceeded; pass the
accumulated bytes to ClaudePluginParser / installed_plugin_service.upload_plugin
as before.
In `@backend/app/services/installed_plugin_service.py`:
- Around line 61-66: The current _safe_kind_name lossily slugs/truncates
parsed.name which causes collisions; change the name generation used when
constructing Kind (the Kind(...) call and the other usage around lines 289-291)
to append a short deterministic hash suffix of the original parsed.name (e.g.,
sha256/sha1 hex truncated to 8-10 chars) so the final name =
slugified_truncated_base + "-" + hash; ensure truncation keeps room for the
suffix (max total 100 chars), use the same deterministic hashing function in
both places, and keep namespace and user_id unchanged so the {namespace, name,
user_id} identity remains unique and stable.
In `@executor/modes/local/capabilities.py`:
- Around line 756-777: The installer currently skips downloads if install_path
exists, so when the backend sends a new checksum for the same
installed_plugin_id we must force-refresh; update the install logic (the branch
around install_path.exists() and calls to _download_plugin_package) to compare
the incoming item["checksum"] with the persisted checksum for that
installed_plugin_id and, if different, treat it as missing: download and replace
the install path and update the stored checksum; ensure this change is applied
to the same pattern used elsewhere (the other install branches that call
_download_plugin_package / check client and item.get("download_path")), and
update the record persistence so installed_plugin_id -> checksum is
saved/updated after a successful download.
- Around line 873-898: In _extract_plugin_zip ensure you never remove the
existing install_path until the ZIP has been fully extracted and validated in a
temp directory: create temp_path (as you already do), extract the archive into
temp_path, run the safety checks and call _normalized_plugin_root on temp_path,
then only after validation succeed atomically replace the existing install_path
(remove or rename install_path and move/replace root → install_path). Move the
existing shutil.rmtree(install_path) out of the pre-extraction path and perform
it (or use os.replace/os.rename for atomic swap) only after
archive.extractall(temp_path) and root normalization succeed, and keep the final
cleanup of temp_path in the existing finally block to avoid leaving a broken
install.
In `@wework/src/api/plugins.ts`:
- Around line 23-44: The uploadPlugin method currently uses raw fetch and reads
the auth token from localStorage; replace it with the shared HttpClient to keep
auth and error handling consistent: in uploadPlugin, build the FormData (as now)
and call HttpClient.post (or an added HttpClient.upload/multipart method) to
POST to `${apiBaseUrl}/plugins/upload`, remove direct localStorage access and
let HttpClient attach Authorization, and preserve throwing an Error on non-ok
responses and returning the parsed InstalledPlugin; if HttpClient lacks FormData
support, add a multipart/form-data codepath to HttpClient (e.g., HttpClient.post
can accept a FormData body and not set Content-Type) and use that from
uploadPlugin.
In `@wework/src/components/plugins/PluginsWorkspace.tsx`:
- Around line 604-618: uploadPlugin currently uses try/finally so upload errors
are swallowed; add an error path similar to uploadPersonalSkill: catch errors
from pluginApi.uploadPlugin and either set a plugin upload error state (e.g.,
setPluginUploadError) so the UI/dialog can show it, and then rethrow the error
so callers (PluginUploadDialog) can handle it, or at minimum rethrow after
setting state; update the uploadPlugin function to include a catch block that
references pluginApi.uploadPlugin, setIsUploadingPlugin,
setShowPluginUploadDialog, setInstalledPlugins, toInstalledPluginItem and
setActiveTab appropriately.
In `@wework/src/components/plugins/PluginUploadDialog.tsx`:
- Around line 24-28: The current .zip check in PluginUploadDialog uses
nextFile.name.endsWith('.zip') which is case-sensitive; change the condition to
compare a lowercased filename (e.g., use
nextFile.name?.toLowerCase().endsWith('.zip')) so names like "plugin.ZIP" pass;
update the if guard that calls setError(...) and setFile(null) to use this
case-insensitive check and ensure you handle a possibly undefined name safely.
- Around line 36-45: The form onSubmit currently calls void onUpload(file) which
discards the Promise and swallows upload failures; change the handler in
PluginUploadDialog's onSubmit to await onUpload(file) (or call
onUpload(file).catch(...)) and in the catch call setError(...) to surface the
backend error to the user (use the existing setError state and t() for i18n),
and optionally manage a local "uploading" flag to disable resubmission while
pending; also add a corresponding .catch to the workspace uploadPlugin function
to reject/propagate errors so failures bubble up to PluginUploadDialog's
handler.
---
Nitpick comments:
In `@backend/app/api/endpoints/installed_plugins.py`:
- Around line 26-120: Add OpenTelemetry tracing to the plugin endpoint handlers
by decorating synchronous handlers (list_installed_plugins,
download_installed_plugin) with `@trace_sync` and async handlers (upload_plugin,
update_installed_plugin, uninstall_installed_plugin) with `@trace_async`; inside
each handler call set_span_attribute to record user_id and relevant resource ids
(e.g., installed_id) and for upload_plugin also set upload_size (len(content))
and add_span_event for filename; ensure the decorators are imported and used
consistently and keep tracing calls near the start of each function so spans
capture the whole handler execution.
In `@backend/app/services/claude_plugin_parser.py`:
- Around line 23-26: Add a concise docstring to the ClaudePluginParser class and
a method docstring for parse_package describing its purpose (parsing and
validating untrusted Claude Code plugin ZIP bytes) and its return type
PluginUploadInfo; decorate parse_package with `@trace_sync` and inside the
function set span attributes (e.g., package_size_bytes, package_sha256 or
filename if available) and add span events around major steps (e.g.,
"received_package", "validation_started", "validation_failed") so failures are
traceable; ensure any caught exceptions call add_span_event()/set_span_attribute
with error details before re-raising or returning an error result.
In `@backend/app/services/installed_plugin_service.py`:
- Around line 31-159: Add docstrings and OpenTelemetry tracing to the new public
methods upload_plugin, list_installed_plugins, update_installed_plugin,
uninstall_installed_plugin, and package_path_for_download: add a one-sentence
docstring explaining the purpose and parameters for each method, decorate each
method with `@trace_sync` (or `@trace_async` if converted to async) and instrument
key steps with add_span_event() and set_span_attribute() calls — e.g., log
start/finish, user_id and installed_id, parsed.name/checksum/filename in
upload_plugin, updated fields in update_installed_plugin, installState in
uninstall_installed_plugin, and storageKey/path existence checks in
package_path_for_download; ensure you record errors/exceptions to spans before
raising HTTPException or returning.
In `@wework/src/api/plugins.ts`:
- Around line 24-26: The uploadPlugin function currently hardcodes enabled to
'true'; change its signature to accept an enabled parameter (e.g.,
uploadPlugin(file: File, enabled = true)) and use that parameter when building
the FormData (replace formData.append('enabled', 'true') with
formData.append('enabled', String(enabled))). Update any call sites of
uploadPlugin if they need to upload a disabled plugin.
In `@wework/src/components/plugins/PluginDetailView.tsx`:
- Around line 79-85: The MCP entries in the components.mcps.map are using a
hardcoded description ('MCP server') instead of the item's metadata; update the
mapping (the block that creates objects with key `mcp-${item.name}`,
componentKey `mcp:${item.name}`, and type 'mcp') to set description to the
actual metadata, e.g. use item.description || item.path || item.name (or similar
fallback) so MCPs follow the same pattern as
skills/commands/agents/hooks/lsps/monitors/bins.
In `@wework/src/components/plugins/PluginUploadDialog.tsx`:
- Around line 76-81: The UI advertises a 50MB limit but PluginUploadDialog never
checks file size client-side; update the selectFile handler in
PluginUploadDialog to validate nextFile.size and reject files >50*1024*1024
bytes, setting the same error flow used by SkillUploadDialog (use the i18n key
plugins_skill_upload_size_error or add a plugins_plugin_upload_size_error if
preferred) and clear/reset state appropriately so the large file is rejected
before upload starts. Ensure you reference the selectFile function and
nextFile.size when adding the check and trigger the existing error display path
used for skill uploads.
🪄 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: 068f9ebe-29a4-4f44-bbdb-2744ba0c118f
📒 Files selected for processing (19)
backend/app/api/api.pybackend/app/api/endpoints/installed_plugins.pybackend/app/schemas/installed_plugin.pybackend/app/services/claude_plugin_parser.pybackend/app/services/device/capability_sync_service.pybackend/app/services/installed_plugin_service.pyexecutor/modes/local/capabilities.pystart.shtests/test_start_sh.shwework/src/api/plugins.tswework/src/components/plugins/PluginCreateMenu.tsxwework/src/components/plugins/PluginDetailView.tsxwework/src/components/plugins/PluginManagementRows.tsxwework/src/components/plugins/PluginManagementWorkspace.tsxwework/src/components/plugins/PluginUploadDialog.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 (1)
- tests/test_start_sh.sh
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
wework/src/components/chat/composer/ModelSelector.tsx (1)
245-450: 🏗️ Heavy liftSplit the mobile picker out of
ModelSelector.This component now contains two substantially different interaction models plus a large mobile-only render path, which makes future state/layout changes harder to reason about.
Based on learnings, "Responsive architecture: separate components when layout differences exceed 30% or different interaction patterns exist, use Mobile/Desktop component separation or Tailwind responsive classes appropriately".
🤖 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/chat/composer/ModelSelector.tsx` around lines 245 - 450, ModelSelector mixes desktop and mobile UI/interaction logic (see renderMobileSheet, renderMobileControlSection, renderMobileAutomaticReasoningSection and mobile-specific state like mobileQuery, activateMobileFamily, mobileModels), making it hard to maintain; extract the entire mobile picker into a new MobileModelSelector component that owns its mobile-only state and handlers (mobileQuery, activeGroup selection, activateMobileFamily, handleSelectModel, handleSelectModelOption, mobileModels, selectedModelOptions), move renderMobile* helpers into that component, keep the same data-testid values and prop-drill only what’s necessary (current selectedModel, selectedModelOptions, onClose/closeMenu, onSelectModel callback, i18n t and controls/familyGroups), and replace the internal renderMobileSheet call in ModelSelector with <MobileModelSelector .../> so desktop logic/state stays focused and mobile layout/interaction is isolated.
🤖 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/chat/composer/ModelSelector.tsx`:
- Around line 260-270: The model chip and family tab buttons in the
ModelSelector component use a height of h-10 which is below the 44×44 mobile
hit-area requirement; update the button class strings (the elements rendered
with key={option.value} and the other similar button around
handleSelectModelOption/control.id/option.value) to use h-11 and min-w-[44px]
(e.g., replace 'h-10' with 'h-11 min-w-[44px]') so all interactive elements meet
the 44×44 guideline while preserving the existing rounded, border and text
classes.
- Around line 303-318: The mobile sheet rendered in ModelSelector.tsx is missing
dialog semantics; update the element with data-testid="model-selector-menu" to
include role="dialog" and aria-modal="true", add an id to the title (the h2
rendering t('workbench.model_picker_title')) and set aria-labelledby on the menu
to that id so screen readers announce the title, and ensure focus is moved into
the sheet when opened and returned when closed (implement a simple focus
trap/initial focus within the menu and restore focus on close) while keeping the
existing onClick={event => event.stopPropagation()} behavior.
In `@wework/src/components/layout/MobileWorkbenchLayout.test.tsx`:
- Around line 63-70: The helper renderAtMobileWidth mutates global
window.innerWidth and never resets it; save the original value at the start of
renderAtMobileWidth (const originalInnerWidth = window.innerWidth), set
innerWidth to 400 as you already do, then restore window.innerWidth in an
afterEach cleanup in this test file by redefining the property back to
originalInnerWidth (Object.defineProperty(window, 'innerWidth', { configurable:
true, writable: true, value: originalInnerWidth })) and dispatching a resize
event (window.dispatchEvent(new Event('resize'))) so subsequent tests don’t
inherit the mobile width; reference the renderAtMobileWidth helper and the
file-level afterEach when making the change.
In `@wework/src/components/plugins/PluginsWorkspace.tsx`:
- Line 1084: The search input's height is 40px due to "h-10" and must meet the
44px mobile touch target; update the className on the search input in
PluginsWorkspace.tsx by replacing "h-10" with "h-11" and add a min-height to
guarantee the touch target (e.g., include "min-h-[44px]" or the equivalent
utility), so the class string becomes something like "h-11 min-h-[44px] w-full
rounded-xl border ...".
---
Nitpick comments:
In `@wework/src/components/chat/composer/ModelSelector.tsx`:
- Around line 245-450: ModelSelector mixes desktop and mobile UI/interaction
logic (see renderMobileSheet, renderMobileControlSection,
renderMobileAutomaticReasoningSection and mobile-specific state like
mobileQuery, activateMobileFamily, mobileModels), making it hard to maintain;
extract the entire mobile picker into a new MobileModelSelector component that
owns its mobile-only state and handlers (mobileQuery, activeGroup selection,
activateMobileFamily, handleSelectModel, handleSelectModelOption, mobileModels,
selectedModelOptions), move renderMobile* helpers into that component, keep the
same data-testid values and prop-drill only what’s necessary (current
selectedModel, selectedModelOptions, onClose/closeMenu, onSelectModel callback,
i18n t and controls/familyGroups), and replace the internal renderMobileSheet
call in ModelSelector with <MobileModelSelector .../> so desktop logic/state
stays focused and mobile layout/interaction is isolated.
🪄 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: 66edd025-0f40-4455-9a99-0e9f4f9192ec
📒 Files selected for processing (33)
wework/src/App.plugins.test.tsxwework/src/components/chat/composer/AddContextMenu.tsxwework/src/components/chat/composer/AttachmentBadges.tsxwework/src/components/chat/composer/CompactChatComposer.tsxwework/src/components/chat/composer/ModelSelector.tsxwework/src/components/chat/composer/ProjectChatComposer.tsxwework/src/components/chat/composer/ProjectWorkBar.tsxwework/src/components/chat/composer/SkillSelector.tsxwework/src/components/layout/AppLayout.tsxwework/src/components/layout/DesktopSettingsMenu.tsxwework/src/components/layout/DesktopSidebar.tsxwework/src/components/layout/DesktopWorkbenchLayout.test.tsxwework/src/components/layout/DesktopWorkbenchLayout.tsxwework/src/components/layout/Header.tsxwework/src/components/layout/MobileDrawer.tsxwework/src/components/layout/MobileWorkbenchLayout.test.tsxwework/src/components/layout/MobileWorkbenchLayout.tsxwework/src/components/layout/workspace-panels/BottomWorkspacePanel.tsxwework/src/components/layout/workspace-panels/RightWorkspacePanel.tsxwework/src/components/plugins/McpManagementSections.tsxwework/src/components/plugins/PluginCatalogSections.tsxwework/src/components/plugins/PluginDetailView.tsxwework/src/components/plugins/PluginManagementWorkspace.tsxwework/src/components/plugins/PluginUploadDialog.tsxwework/src/components/plugins/PluginsWorkspace.tsxwework/src/components/plugins/SkillUploadDialog.tsxwework/src/i18n/locales/en/common.jsonwework/src/i18n/locales/zh-CN/common.jsonwework/src/pages/LoginPage.tsxwework/src/pages/OidcCallbackPage.tsxwework/src/pages/PluginManagementPage.tsxwework/src/pages/PluginsPage.tsxwework/tailwind.config.js
✅ Files skipped from review due to trivial changes (16)
- wework/src/components/layout/DesktopSettingsMenu.tsx
- wework/src/components/chat/composer/AttachmentBadges.tsx
- wework/src/components/chat/composer/SkillSelector.tsx
- wework/src/pages/OidcCallbackPage.tsx
- wework/src/components/layout/workspace-panels/RightWorkspacePanel.tsx
- wework/src/pages/PluginsPage.tsx
- wework/src/components/layout/Header.tsx
- wework/src/components/layout/DesktopWorkbenchLayout.tsx
- wework/src/components/layout/workspace-panels/BottomWorkspacePanel.tsx
- wework/src/components/layout/AppLayout.tsx
- wework/src/components/chat/composer/AddContextMenu.tsx
- wework/src/components/plugins/PluginCatalogSections.tsx
- wework/src/components/layout/DesktopWorkbenchLayout.test.tsx
- wework/src/components/chat/composer/CompactChatComposer.tsx
- wework/src/components/layout/MobileDrawer.tsx
- wework/src/components/plugins/McpManagementSections.tsx
🚧 Files skipped from review as they are similar to previous changes (5)
- wework/src/i18n/locales/zh-CN/common.json
- wework/src/i18n/locales/en/common.json
- wework/src/components/plugins/PluginDetailView.tsx
- wework/src/components/plugins/PluginUploadDialog.tsx
- wework/src/components/plugins/PluginManagementWorkspace.tsx
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
executor/modes/local/capabilities.py (1)
854-868:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftAvoid buffering the entire plugin ZIP in memory.
response.contentmaterializes the full archive before any guard runs. A large or malformed response can exhaust the executor before checksum verification or extraction starts. Stream the download to a temp file or enforce a hard byte cap while reading, ideally matching the backend package limit.🤖 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 854 - 868, The code currently uses response.content (materializing the whole ZIP) and then calls self._extract_plugin_zip(content, install_path); change this to stream the HTTP response into a temporary file (or a file-like stream) while enforcing a hard byte cap (match backend package limit) and computing the sha256 checksum incrementally; after streaming completes, compare the computed actual_checksum to expected_checksum and only then call a variant of _extract_plugin_zip that accepts a file path or file-like object (or refactor _extract_plugin_zip to accept a file), and ensure the temp file is deleted on success or error.wework/src/components/plugins/PluginUploadDialog.tsx (2)
80-84:⚠️ Potential issue | 🟠 Major | ⚡ Quick winBump the upload dialog actions to 44×44 touch targets.
The close button is
36×36, and the cancel/upload actions are only40pxtall. That misses the mobile minimum from the repo guidelines and makes the dialog harder to use on small screens.As per coding guidelines, "All interactive elements on mobile must be at least 44px × 44px - use
h-11 min-w-[44px]for buttons".Also applies to: 116-129
🤖 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/PluginUploadDialog.tsx` around lines 80 - 84, In PluginUploadDialog.tsx update the touch targets for the dialog buttons (the close button and the cancel/upload action buttons rendered in PluginUploadDialog) to meet mobile minimums by replacing their current sizing classes (e.g., "h-9 w-9" and the 40px tall action button classes) with the repo guideline classes "h-11 min-w-[44px]" (keep existing padding/rounded/text classes and onClick/aria-label props intact) so all interactive elements in the dialog are at least 44×44.
48-50:⚠️ Potential issue | 🟠 Major | ⚡ Quick winExpose this modal as an actual dialog.
The upload panel is rendered as a plain overlay/form, so assistive tech won’t announce it as a modal. Add
role="dialog",aria-modal="true", and wire it to the existing title viaaria-labelledby. Since this dialog already has dedicated tests, adding a stable dialog-leveldata-testidhere would also align with the repo’s testing guidance.Based on learnings, "Add
data-testidattributes to interactive elements (buttons, inputs, links, selects), key UI components, dynamic content containers, and modal dialogs when creating new tests".Also applies to: 68-72
🤖 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/PluginUploadDialog.tsx` around lines 48 - 50, The modal overlay in PluginUploadDialog is not exposed as an accessible dialog; update the root form/div used as the dialog (in the PluginUploadDialog component) to include role="dialog", aria-modal="true", and aria-labelledby pointing to the existing title element (give the title a stable id like "plugin-upload-title"); also add a stable data-testid on the dialog container (eg. data-testid="plugin-upload-dialog") so tests can target it; apply the same attributes/data-testid to the alternate dialog markup referenced around the other block (the secondary form at 68-72) so both dialog render paths are accessible and testable.wework/src/components/plugins/PluginDetailView.tsx (2)
173-236:⚠️ Potential issue | 🟠 Major | ⚡ Quick winIncrease the mobile hit areas for the detail actions.
The back button, uninstall button, and both switch controls are all below the repo’s 44×44 mobile minimum (
h-9,w-9,h-7). On phones this makes the detail view noticeably harder to operate.As per coding guidelines, "All interactive elements on mobile must be at least 44px × 44px - use
h-11 min-w-[44px]for buttons".Also applies to: 268-295
🤖 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/PluginDetailView.tsx` around lines 173 - 236, The interactive controls (back button, uninstall button, and toggle switch) have hit areas smaller than the 44×44 mobile minimum; update the classNames for the back button (onClick handler onBack), the uninstall button (onClick handler onUninstall / data-testid `plugin-detail-uninstall-...`), and the toggle switch button (onClick handler onToggle / data-testid `plugin-detail-toggle-...`) to use the recommended sizing: replace h-9 or h-7 with h-11 and add min-w-[44px] where appropriate (e.g., `h-11 min-w-[44px]`) and ensure the inner switch knob remains visually centered after the size change; apply the same adjustments to the other action buttons in this component referenced later (the section called out in the comment).
115-150:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winLocalize the detail metadata labels.
Version,Author,Source,Package,Homepage,Repository, and the breadcrumbaria-labelare hard-coded in English, so the new detail view will still render mixed-language UI underzh-CN.Also applies to: 169-171
🤖 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/PluginDetailView.tsx` around lines 115 - 150, The hard-coded English labels in detailRows and the breadcrumb aria-label should be replaced with localized strings: import/obtain the app's i18n translate function (e.g., t or useTranslation) in PluginDetailView and replace literal labels 'Version', 'Author', 'Source', 'Package', 'Homepage', 'Repository' with t(...) keys (e.g., t('plugin.version'), etc.) inside detailRows, ensure href logic is unchanged, and also replace the breadcrumb aria-label (currently hard-coded) with a localized string via the same translate function so all metadata and aria text are localized.
🤖 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 `@executor/modes/local/capabilities.py`:
- Around line 893-914: The temp_path and staged_path currently use os.getpid()
which can collide across concurrent syncs in the same process; change the
creation of temp_path and staged_path (used around archive.extractall,
_normalized_plugin_root, and the final os.replace to install_path) to use
per-call unique directories (for example via tempfile.mkdtemp() or appending a
uuid4() suffix or using tempfile.TemporaryDirectory()) so each extraction gets
its own unique temp/staged directories; ensure the rest of the logic (moving
root to staged_path, cleanup in the finally block) still operates on those
unique paths and that cleanup uses ignore_errors=True as before.
---
Outside diff comments:
In `@executor/modes/local/capabilities.py`:
- Around line 854-868: The code currently uses response.content (materializing
the whole ZIP) and then calls self._extract_plugin_zip(content, install_path);
change this to stream the HTTP response into a temporary file (or a file-like
stream) while enforcing a hard byte cap (match backend package limit) and
computing the sha256 checksum incrementally; after streaming completes, compare
the computed actual_checksum to expected_checksum and only then call a variant
of _extract_plugin_zip that accepts a file path or file-like object (or refactor
_extract_plugin_zip to accept a file), and ensure the temp file is deleted on
success or error.
In `@wework/src/components/plugins/PluginDetailView.tsx`:
- Around line 173-236: The interactive controls (back button, uninstall button,
and toggle switch) have hit areas smaller than the 44×44 mobile minimum; update
the classNames for the back button (onClick handler onBack), the uninstall
button (onClick handler onUninstall / data-testid
`plugin-detail-uninstall-...`), and the toggle switch button (onClick handler
onToggle / data-testid `plugin-detail-toggle-...`) to use the recommended
sizing: replace h-9 or h-7 with h-11 and add min-w-[44px] where appropriate
(e.g., `h-11 min-w-[44px]`) and ensure the inner switch knob remains visually
centered after the size change; apply the same adjustments to the other action
buttons in this component referenced later (the section called out in the
comment).
- Around line 115-150: The hard-coded English labels in detailRows and the
breadcrumb aria-label should be replaced with localized strings: import/obtain
the app's i18n translate function (e.g., t or useTranslation) in
PluginDetailView and replace literal labels 'Version', 'Author', 'Source',
'Package', 'Homepage', 'Repository' with t(...) keys (e.g., t('plugin.version'),
etc.) inside detailRows, ensure href logic is unchanged, and also replace the
breadcrumb aria-label (currently hard-coded) with a localized string via the
same translate function so all metadata and aria text are localized.
In `@wework/src/components/plugins/PluginUploadDialog.tsx`:
- Around line 80-84: In PluginUploadDialog.tsx update the touch targets for the
dialog buttons (the close button and the cancel/upload action buttons rendered
in PluginUploadDialog) to meet mobile minimums by replacing their current sizing
classes (e.g., "h-9 w-9" and the 40px tall action button classes) with the repo
guideline classes "h-11 min-w-[44px]" (keep existing padding/rounded/text
classes and onClick/aria-label props intact) so all interactive elements in the
dialog are at least 44×44.
- Around line 48-50: The modal overlay in PluginUploadDialog is not exposed as
an accessible dialog; update the root form/div used as the dialog (in the
PluginUploadDialog component) to include role="dialog", aria-modal="true", and
aria-labelledby pointing to the existing title element (give the title a stable
id like "plugin-upload-title"); also add a stable data-testid on the dialog
container (eg. data-testid="plugin-upload-dialog") so tests can target it; apply
the same attributes/data-testid to the alternate dialog markup referenced around
the other block (the secondary form at 68-72) so both dialog render paths are
accessible and testable.
🪄 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: ead98d1f-0e55-4200-83e6-f31d3215756a
📒 Files selected for processing (21)
backend/app/api/endpoints/installed_plugins.pybackend/app/services/installed_plugin_service.pybackend/tests/services/test_installed_plugin_upload.pyexecutor/modes/local/capabilities.pyexecutor/tests/test_local_capability_sync_handler.pywework/eslint.config.jswework/package.jsonwework/scripts/build-mac-app.shwework/src/api/adapters.test.tswework/src/api/http.test.tswework/src/api/http.tswework/src/api/plugins.tswework/src/components/chat/composer/ModelSelector.tsxwework/src/components/layout/MobileWorkbenchLayout.test.tsxwework/src/components/plugins/PluginDetailView.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.json
✅ Files skipped from review due to trivial changes (4)
- wework/eslint.config.js
- wework/package.json
- wework/src/i18n/locales/en/common.json
- wework/src/i18n/locales/zh-CN/common.json
🚧 Files skipped from review as they are similar to previous changes (4)
- wework/src/components/layout/MobileWorkbenchLayout.test.tsx
- backend/app/api/endpoints/installed_plugins.py
- backend/app/services/installed_plugin_service.py
- wework/src/components/plugins/PluginsWorkspace.tsx
Comment on lines
+893
to
+914
| temp_path = parent / f".{install_path.name}.tmp-{os.getpid()}" | ||
| staged_path = parent / f".{install_path.name}.staged-{os.getpid()}" | ||
| if temp_path.exists(): | ||
| shutil.rmtree(temp_path) | ||
| if staged_path.exists(): | ||
| shutil.rmtree(staged_path) | ||
| temp_path.mkdir(parents=True) | ||
| try: | ||
| archive.extractall(temp_path) | ||
| root = self._normalized_plugin_root(temp_path) | ||
| if root != temp_path: | ||
| shutil.move(str(root), str(staged_path)) | ||
| else: | ||
| os.replace(temp_path, staged_path) | ||
| if install_path.exists(): | ||
| shutil.rmtree(install_path) | ||
| os.replace(staged_path, install_path) | ||
| finally: | ||
| if temp_path.exists(): | ||
| shutil.rmtree(temp_path, ignore_errors=True) | ||
| if staged_path.exists(): | ||
| shutil.rmtree(staged_path, ignore_errors=True) |
Contributor
There was a problem hiding this comment.
Use unique staging directories per extraction.
Line 893 and Line 894 derive both temp paths from os.getpid() only. Two syncs for the same plugin in the same process will reuse the same tmp/staged directories, so one run can delete or replace the other's workspace mid-install. Use tempfile.mkdtemp() or another per-call unique suffix here.
🤖 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 893 - 914, The temp_path
and staged_path currently use os.getpid() which can collide across concurrent
syncs in the same process; change the creation of temp_path and staged_path
(used around archive.extractall, _normalized_plugin_root, and the final
os.replace to install_path) to use per-call unique directories (for example via
tempfile.mkdtemp() or appending a uuid4() suffix or using
tempfile.TemporaryDirectory()) so each extraction gets its own unique
temp/staged directories; ensure the rest of the logic (moving root to
staged_path, cleanup in the finally block) still operates on those unique paths
and that cleanup uses ignore_errors=True as before.
This was referenced Jun 4, 2026
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
2 participants
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
New Features
Style
Tests