看板支持本地模式。支持用gitlab、github作为存储 - #2251
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThis PR adds provider-aware cloud projects across backend APIs, encrypted credential storage, executor task runtime services, MCP/IPC integrations, Workbench routing, and Todo UI flows for local, GitHub, and GitLab task sources. ChangesProvider-aware cloud task runtime
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant TodoWorkspace
participant CloudProjectSpaceApi
participant BackendDeliveryAPI
participant LocalExecutor
participant ExternalIssueAPI
TodoWorkspace->>CloudProjectSpaceApi: create or list cloud project
CloudProjectSpaceApi->>BackendDeliveryAPI: persist project metadata and credential
CloudProjectSpaceApi->>LocalExecutor: configure project provider
TodoWorkspace->>CloudProjectSpaceApi: list or create loop item
CloudProjectSpaceApi->>ExternalIssueAPI: route GitHub/GitLab task operation
ExternalIssueAPI->>LocalExecutor: execute provider request
LocalExecutor-->>TodoWorkspace: mapped loop item
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…7-114803 # Conflicts: # executor/src/bin/wegent-executor.rs
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 20
🧹 Nitpick comments (15)
wework/src/api/hybrid/cloudProjectSpaceApi.ts (1)
50-65: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winLog the swallowed credential/configuration failures.
Both
catchblocks discard the error, so an external project silently stops routing Issues with no trace. Aconsole.warnhere (matching the pattern used inhybridServices.ts) keeps the non-fatal behavior while making the failure diagnosable.🤖 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/hybrid/cloudProjectSpaceApi.ts` around lines 50 - 65, Update both catch blocks in the external project handling flow to log the caught credential retrieval and project configuration errors with console.warn, following the existing pattern in hybridServices.ts. Preserve the current non-fatal behavior by returning after each warning.wework/src/features/todo/CloudTodoWorkspace.test.tsx (1)
691-705: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAsserting raw Tailwind class names locks in styling details (and the
bg-blackliterals flagged inCloudProjectManageView.tsx).These assertions break on any purely visual refactor and don't verify user-observable behavior. Prefer asserting the disabled/enabled state (
toBeDisabled()) and leaving appearance to visual review.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@wework/src/features/todo/CloudTodoWorkspace.test.tsx` around lines 691 - 705, Replace the raw Tailwind class assertions in the CloudTodoWorkspace test with user-observable state assertions on the save and confirm controls, using toBeDisabled() or toBeEnabled() as appropriate for the scenario. Remove checks for bg-black, text-white, disabled classes, and disabled:opacity-50 while preserving coverage of the controls’ actual disabled/enabled behavior.wework/src/features/todo/CloudTodoWorkspace.tsx (1)
367-531: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNew Todo/provider UI ships hard-coded Chinese copy instead of translated strings. Both new sections add user-facing literals directly in JSX rather than routing them through the local translation wrapper.
wework/src/features/todo/CloudTodoWorkspace.tsx#L367-L531: move the project dialog labels/hints ("新建项目空间", "保存位置", "任务来源", token hints) into translation keys via@/hooks/useTranslation.wework/src/features/todo/CloudProjectManageView.tsx#L475-L543: do the same for the provider management section ("任务来源", "仓库地址", "访问令牌", "令牌已配置", "已保存").As per coding guidelines: "Use the local
@/hooks/useTranslationwrapper for new Wework 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 `@wework/src/features/todo/CloudTodoWorkspace.tsx` around lines 367 - 531, Replace the hard-coded Chinese user-facing copy in CloudTodoWorkspace.tsx lines 367-531 with keys through the local `@/hooks/useTranslation` wrapper, including the dialog title, project/location/task-source labels, repository and token labels, hints, placeholders, and descriptions. Apply the same translation approach to CloudProjectManageView.tsx lines 475-543 for the task-source, repository, token, configured, and saved-provider text; add or reuse appropriate translation keys while preserving the existing UI behavior.Source: Coding guidelines
wework/src/features/todo/projectProviderConfig.test.ts (1)
4-14: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThin coverage for a multi-branch parser.
repositoryProviderConfighas shorthand, SSH, default-domain, self-hosted, and three throw paths, plusrepositoryAddressis untested. Worth adding cases for the shorthand (owner/repo),git@host:group/project.git,github.com(nodomain/api_base), and the GitHubsegments.length !== 2rejection.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@wework/src/features/todo/projectProviderConfig.test.ts` around lines 4 - 14, Add focused tests in the repositoryProviderConfig suite for owner/repo shorthand, git@host:group/project.git SSH URLs, github.com handling without domain or api_base, and rejection when GitHub path segments are not exactly two; also add coverage for repositoryAddress. Preserve the existing GitLab test and assert each branch’s expected return value or thrown error.wework/src/api/local/localDelivery.ts (1)
245-247: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDouble cast defeats the declared return type.
The signature already claims
NonNullable<WorkbenchServices['deliveryApi']>, butapi as unknown as ...suppresses any structural mismatch betweenapiand that contract, so drift in the delivery API surface will not fail type-check. Consider typingapiexplicitly (const api: NonNullable<WorkbenchServices['deliveryApi']> = {...}) or narrowing the gaps so the cast can be dropped.Also applies to: 556-558
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@wework/src/api/local/localDelivery.ts` around lines 245 - 247, Remove the double cast in createLocalDeliveryApi and explicitly type the constructed api object as NonNullable<WorkbenchServices['deliveryApi']>. Ensure the object satisfies the declared delivery API contract directly so structural mismatches are caught by type-checking, including the corresponding return path.backend/app/mcp_server/tools/delivery.py (1)
537-548: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueRead
task_providerwhile the session is still open.
projectis detached once thewith SessionLocal()block exits; the read on Line 542 only works becausemetadata_jsonhappens to be cached by_serialize_project. Capturing the provider inside the block removes the dependency on that ordering.♻️ Proposed refactor
with SessionLocal() as db: project = cloud_project_service.get(db, project_id, token_info.user_id) project_data = _serialize_project(project) + task_provider = project.task_provider todos = ( list_cloud_todos(project_id, token_info) - if project.task_provider == "local" + if task_provider == "local" else { "items": [], - "taskProvider": project.task_provider, + "taskProvider": task_provider, "todoTool": "wegent_tasks.create_todo", } )🤖 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/mcp_server/tools/delivery.py` around lines 537 - 548, Capture project.task_provider inside the SessionLocal block before it closes, alongside _serialize_project, then use that local value in the todos provider check and response. Replace the post-session project.task_provider reads while preserving the existing local-provider and external-provider behavior.backend/app/services/cloud_projects/service.py (2)
158-192: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExtract the metadata merge into a helper.
updatenow carries four nesting levels of metadata/provider bookkeeping inline. Moving lines 158–192 into a private_merge_metadata(project, values) -> dictwould keepupdatefocused on the optimistic-locking write.🤖 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/cloud_projects/service.py` around lines 158 - 192, Extract the metadata and provider configuration merge logic from update into a private _merge_metadata(project, values) -> dict helper. Preserve the existing tags handling, provider_config normalization/storage, ValueError-to-HTTPException conversion, and metadata_json update behavior, then have update call the helper while retaining focus on the optimistic-locking write.
126-145: 🔒 Security & Privacy | 🔵 TrivialConsider auditing provider-credential reads.
This endpoint hands the plaintext PAT to every Developer-or-above member. An audit log entry (project id, requesting user id, timestamp — no token value) would make credential distribution traceable and support rotation forensics.
🤖 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/cloud_projects/service.py` around lines 126 - 145, The get_provider_credential method should record an audit event whenever a provider credential is successfully returned, including the project ID, requesting user ID, and timestamp but never the plaintext token. Add the audit write after token validation and before returning, using the existing audit logging mechanism and preserving current authorization and error behavior.backend/tests/api/test_cloud_projects_api.py (1)
177-183: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winAdd a negative test for the credential endpoint.
The happy path is covered, but the new
/provider-credentialroute is the one place a plaintext PAT leaves the backend. Worth asserting that a Reporter-level member (or non-member) gets 403 and that a project without a configured credential gets 409.🤖 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/test_cloud_projects_api.py` around lines 177 - 183, Add negative coverage alongside the existing provider-credential test: assert that a Reporter-level member and a non-member receive 403 from the /provider-credential endpoint, and assert that a project without a configured credential receives 409. Reuse the existing test client, authentication helpers, and project setup patterns in the surrounding tests.backend/app/schemas/cloud_project.py (1)
93-105: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate token/credential validation.
Lines 98–104 re-implement the exact checks in
normalize_provider_config(lines 49–55), whichCloudProjectService.updatealready applies with the project's real provider before persisting. Either drop this block or extract the shared token check into one helper so the two copies cannot diverge.As per coding guidelines: "Before adding code, search for and reuse existing components, services, utilities, and patterns; extract shared logic instead of duplicating it."
🤖 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/schemas/cloud_project.py` around lines 93 - 105, Remove the duplicated provider credential and token validation from CloudProjectUpdate.validate_provider, relying on CloudProjectService.update to invoke normalize_provider_config with the project’s actual provider before persistence. Preserve only the provider immutability behavior and avoid introducing a second validation helper unless the existing normalization flow cannot be reused.Source: Coding guidelines
backend/app/services/loop_items/service.py (1)
164-174: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueProject row is now fetched twice.
_require_internal_task_projectalready returns theCloudProject, then lines 169–174 re-query itwith_for_update. The lock is needed fornext_item_number, so this is correct, but you could drop the returned value on line 164 or reuse it for the provider check only — a short comment noting why the second locked read exists would help future readers.🤖 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/loop_items/service.py` around lines 164 - 174, Update the flow around _require_internal_task_project and the subsequent CloudProject query to make the intentional second, with_for_update read explicit: either discard the helper’s returned project value or use it only for the provider check, and add a brief comment explaining that the locked read is required for next_item_number.executor/src/task_runtime/mcp.rs (1)
39-62: 🚀 Performance & Scalability | 🔵 Trivial | ⚖️ Poor tradeoffRequests are served strictly serially.
Each line is awaited to completion before the next is read, so one slow GitHub/GitLab call stalls every queued tool call on this stdio server. If the agent issues concurrent tool calls, consider spawning per-request tasks with a shared writer channel.
🤖 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/src/task_runtime/mcp.rs` around lines 39 - 62, Update run so request handling is concurrent instead of awaiting each handle_request call before reading the next line. Spawn a task for each parsed request and send responses through a shared channel to a dedicated writer, preserving newline-delimited JSON output and serialized stdout writes.executor/src/task_runtime/credentials.rs (1)
272-296: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
fs::hard_linkfails on filesystems without link support.The atomic create-if-absent trick relies on hard links, which are unavailable on FAT/exFAT volumes and some network mounts; those return an error kind other than
AlreadyExists, so master-key creation fails outright and every provider credential operation breaks. Consider falling back toOpenOptions::new().create_new(true)directly on the final path whenhard_linkreturnsUnsupported/PermissionDenied.🤖 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/src/task_runtime/credentials.rs` around lines 272 - 296, Update write_new_master_key so fs::hard_link failures with Unsupported or PermissionDenied fall back to OpenOptions::new().create_new(true) on the final path, preserving exclusive creation and writing the encoded key with the existing permissions and sync behavior. Keep AlreadyExists returning false, continue cleaning up the temporary file, and propagate other errors through storage_error.executor/src/task_runtime/store.rs (1)
146-195: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winConfigure the credential row and catalog row in one transaction.
The two
INSERT … ON CONFLICTstatements run outside a transaction, so a failure between them leaves an encrypted credential with no catalog descriptor (or a stale descriptor). Wrap both writes intransaction_with_behavior(TransactionBehavior::Immediate)likecreate_taskdoes.🤖 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/src/task_runtime/store.rs` around lines 146 - 195, Wrap both INSERT … ON CONFLICT writes in the surrounding update method in a single transaction created with transaction_with_behavior(TransactionBehavior::Immediate), following the pattern used by create_task. Execute the credential and external_project_catalog statements through that transaction, commit only after both succeed, and preserve the existing error propagation and descriptor_loop_item return behavior.executor/src/local/app_ipc.rs (1)
763-847: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse
task_inputfor these nested payloads.
todos.create,todos.update,todos.reorderandtodos.bindre-implement the exact "nested key or whole params" decode thattask_input(Line 1023) already provides. Replacing the inlineserde_json::from_value(...unwrap_or_else(|| params.clone()))blocks withtask_input::<TaskCreate>(¶ms, "todo")etc. removes four copies of the same logic.🤖 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/src/local/app_ipc.rs` around lines 763 - 847, Replace the duplicated nested-payload deserialization in the todos.create, todos.update, todos.reorder, and todos.bind branches with the existing task_input helper. Use the corresponding types and keys—TaskCreate/TaskUpdate/TaskReorder with “todo” or “reorder”, and RuntimeTaskAddress with “task”—while preserving the existing error propagation and runtime calls.
🤖 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/core/provider_credentials.py`:
- Around line 45-55: Update the credential handling flow around token_supplied
and normalized_token so masked ("***") or empty tokens preserve the existing
credential instead of removing it. Reuse the existing _preserve_credential path
when current is available, while continuing to encrypt and store genuinely new
non-masked tokens via _encrypt_provider_token.
In `@executor/src/agents/mod.rs`:
- Around line 265-266: Restrict ensure_task_mcp_server calls in
AgentProcessEngine run and run_with_events to only the execution engines whose
child runtime consumes mcp_servers. Remove the injection for Dify and
ImageValidator paths, while preserving the existing default task-server behavior
for engines that actually pass mcp_servers onward.
In `@executor/src/local/app_ipc.rs`:
- Around line 676-677: Update handle_task_runtime_request to reuse a single
initialized TaskRuntime instead of calling TaskRuntime::from_env() for every IPC
request. Store the runtime in an appropriate OnceCell or AppIpcServer field,
initialize it once, and reuse its cloneable handle while preserving the existing
task_runtime_error mapping.
In `@executor/src/task_runtime/content.rs`:
- Around line 138-158: The move operation must enforce optimistic locking and
safely match descendant paths. Update the surrounding move method to keep the
version read and both UPDATE statements in one immediate transaction, add the
expected version to the primary UPDATE predicate, and surface a zero-row update
as VersionConflict; escape LIKE metacharacters in the old_path descendant
pattern and add the corresponding ESCAPE clause. Apply the same escaped-pattern
and ESCAPE handling in delete_project_file.
In `@executor/src/task_runtime/issue_provider.rs`:
- Around line 75-92: Update the create method to call validate_external_priority
on input.priority alongside the existing title and status validation, before the
value reaches labels_for_write. Preserve the existing validation and creation
flow for valid priorities.
- Around line 32-41: Configure explicit request and connection timeouts on the
HTTP client built in IssueProvider::new before calling build, using the
project’s established timeout configuration or a bounded duration appropriate
for provider calls. Preserve the existing user agent, error mapping, and
database_path initialization.
In `@executor/src/task_runtime/mcp.rs`:
- Around line 87-98: Update the "tools/call" branch in handle_request to
validate params.name without returning None after consuming the request id. When
the name is missing or not a string, return error_response(id, -32602, …);
preserve the existing call_tool and result_response flow for valid names.
In `@executor/src/task_runtime/router.rs`:
- Around line 194-209: The reorder_tasks method must not report success for
GitHub or GitLab projects while discarding the requested input. Update the
TaskProviderKind::Github | TaskProviderKind::Gitlab branch to return
TaskRuntimeError::UnsupportedProvider, preserving the existing local reorder
behavior and unsupported-provider handling; do not re-list external tasks as a
successful no-op.
In `@executor/src/task_runtime/store.rs`:
- Around line 1026-1039: Update normalize_project_key and the create_project
persistence flow so duplicate derived project keys no longer surface as a raw
SQLite UNIQUE error. Either generate an available suffixed key when the
normalized key conflicts, or catch the project_key unique violation and return
TaskRuntimeError::Invalid with “project key already exists”; preserve existing
normalization and generated-key behavior otherwise.
- Around line 968-1000: Update the external catalog project flow in
list_projects and descriptor_loop_item to carry the already-persisted
external_project_catalog.updated_at value into the resulting LoopItem. Replace
both empty timestamp values with that stored timestamp, preserving the existing
timestamp for local projects.
In `@executor/tests/local_task_mcp_contract.rs`:
- Around line 73-77: Update the panic fallback in the create_project parsing
flow to report responses[2] and identify the create_project response, matching
the source value and tool being validated. Leave the parsing and
successful-result behavior unchanged.
- Around line 96-208: Update
task_mcp_routes_cached_cloud_gitlab_projects_to_gitlab so its setup matches the
behavior being tested: either configure a working HTTPS localhost GitLab
endpoint and assert create_todo reaches create_issue and creates the issue, or
remove the unused Axum server, request handler, and related assertions while
keeping only the api_base HTTPS validation and zero-task assertions.
In `@wework/src/api/hybrid/cloudProjectSpaceApi.ts`:
- Around line 87-101: Update updateCloudProject so external-project updates do
not fail after the store write when provider credentials are absent. Skip
credential retrieval and externalIssueApi.configureProject when no
provider-related fields changed; otherwise tolerate missing-credential failures
consistently with listCloudProjects, while still propagating unrelated errors
and returning the updated project.
In `@wework/src/components/layout/DesktopWorkbenchLayout.test.tsx`:
- Around line 1335-1344: Update the test around the route setup to restore the
previous history URL after execution, preventing `/todo` from leaking into later
tests. Provide `services.deliveryApi` through the rendered
`DesktopWorkbenchLayout` props so the local board path is exercised, then assert
the rendered board itself rather than only `cloud-board-loading` and the hidden
content container.
In `@wework/src/components/layout/DesktopWorkbenchLayout.tsx`:
- Line 111: Update the fallback rendering in DesktopWorkbenchLayout, especially
the branch around the “正在加载云端看板…” placeholder, to distinguish non-transient
missing state.user or services.deliveryApi from an active loading condition;
render an error/empty state with a back action invoking navigateTo('/') for the
non-transient case so /todo remains escapable, while preserving loading behavior
for genuinely transient states. Add isolated real-Tauri verification for this
desktop UI behavior through scripts/ai-verify.mjs.
In `@wework/src/features/todo/CloudProjectManageView.tsx`:
- Line 386: Update the tag-create button at
wework/src/features/todo/CloudProjectManageView.tsx:386 to use bg-text-primary
text-background and disabled:opacity-50 instead of raw black/white classes.
Apply the same styling to the provider save button at
wework/src/features/todo/CloudProjectManageView.tsx:530-539, and relax the
corresponding class assertions in CloudTodoWorkspace.test.tsx.
In `@wework/src/features/todo/CloudTodoWorkspace.tsx`:
- Around line 1172-1180: Update the project row button rendered in the project
map within CloudTodoWorkspace to include a descriptive data-testid, and apply
the same convention to the new sidebar project buttons referenced near the
project selection UI. Keep the existing project.id key and selectProject click
behavior unchanged.
- Around line 709-715: Update the board loading state around boardItemsLoading
to depend only on whether the selected project’s item snapshot has arrived,
removing the items.length/boardError condition so successfully loaded empty
projects render their columns. In the refresh effect’s fetch failure branch,
stop calling applyBoardItems with an empty list; only set boardError so
itemsProjectId remains unchanged and the skeleton persists.
In `@wework/src/features/todo/projectProviderConfig.ts`:
- Around line 16-19: Update the shorthand parsing logic around the visible
value.match expression so GitLab accepts multi-segment repository paths such as
group/subgroup/project, while preserving the existing two-segment behavior for
other providers. Use the existing provider condition to select the
GitLab-specific pattern, and keep the repository normalization that removes a
trailing .git.
- Around line 28-36: Preserve the parsed host, including any port, and the URL
protocol when deriving the self-hosted repository configuration instead of using
parsed.hostname and a hardcoded HTTPS base. Update the related parsing and
api_base construction paths, including the analogous block, while retaining
HTTPS as the default for SSH-format inputs.
---
Nitpick comments:
In `@backend/app/mcp_server/tools/delivery.py`:
- Around line 537-548: Capture project.task_provider inside the SessionLocal
block before it closes, alongside _serialize_project, then use that local value
in the todos provider check and response. Replace the post-session
project.task_provider reads while preserving the existing local-provider and
external-provider behavior.
In `@backend/app/schemas/cloud_project.py`:
- Around line 93-105: Remove the duplicated provider credential and token
validation from CloudProjectUpdate.validate_provider, relying on
CloudProjectService.update to invoke normalize_provider_config with the
project’s actual provider before persistence. Preserve only the provider
immutability behavior and avoid introducing a second validation helper unless
the existing normalization flow cannot be reused.
In `@backend/app/services/cloud_projects/service.py`:
- Around line 158-192: Extract the metadata and provider configuration merge
logic from update into a private _merge_metadata(project, values) -> dict
helper. Preserve the existing tags handling, provider_config
normalization/storage, ValueError-to-HTTPException conversion, and metadata_json
update behavior, then have update call the helper while retaining focus on the
optimistic-locking write.
- Around line 126-145: The get_provider_credential method should record an audit
event whenever a provider credential is successfully returned, including the
project ID, requesting user ID, and timestamp but never the plaintext token. Add
the audit write after token validation and before returning, using the existing
audit logging mechanism and preserving current authorization and error behavior.
In `@backend/app/services/loop_items/service.py`:
- Around line 164-174: Update the flow around _require_internal_task_project and
the subsequent CloudProject query to make the intentional second,
with_for_update read explicit: either discard the helper’s returned project
value or use it only for the provider check, and add a brief comment explaining
that the locked read is required for next_item_number.
In `@backend/tests/api/test_cloud_projects_api.py`:
- Around line 177-183: Add negative coverage alongside the existing
provider-credential test: assert that a Reporter-level member and a non-member
receive 403 from the /provider-credential endpoint, and assert that a project
without a configured credential receives 409. Reuse the existing test client,
authentication helpers, and project setup patterns in the surrounding tests.
In `@executor/src/local/app_ipc.rs`:
- Around line 763-847: Replace the duplicated nested-payload deserialization in
the todos.create, todos.update, todos.reorder, and todos.bind branches with the
existing task_input helper. Use the corresponding types and
keys—TaskCreate/TaskUpdate/TaskReorder with “todo” or “reorder”, and
RuntimeTaskAddress with “task”—while preserving the existing error propagation
and runtime calls.
In `@executor/src/task_runtime/credentials.rs`:
- Around line 272-296: Update write_new_master_key so fs::hard_link failures
with Unsupported or PermissionDenied fall back to
OpenOptions::new().create_new(true) on the final path, preserving exclusive
creation and writing the encoded key with the existing permissions and sync
behavior. Keep AlreadyExists returning false, continue cleaning up the temporary
file, and propagate other errors through storage_error.
In `@executor/src/task_runtime/mcp.rs`:
- Around line 39-62: Update run so request handling is concurrent instead of
awaiting each handle_request call before reading the next line. Spawn a task for
each parsed request and send responses through a shared channel to a dedicated
writer, preserving newline-delimited JSON output and serialized stdout writes.
In `@executor/src/task_runtime/store.rs`:
- Around line 146-195: Wrap both INSERT … ON CONFLICT writes in the surrounding
update method in a single transaction created with
transaction_with_behavior(TransactionBehavior::Immediate), following the pattern
used by create_task. Execute the credential and external_project_catalog
statements through that transaction, commit only after both succeed, and
preserve the existing error propagation and descriptor_loop_item return
behavior.
In `@wework/src/api/hybrid/cloudProjectSpaceApi.ts`:
- Around line 50-65: Update both catch blocks in the external project handling
flow to log the caught credential retrieval and project configuration errors
with console.warn, following the existing pattern in hybridServices.ts. Preserve
the current non-fatal behavior by returning after each warning.
In `@wework/src/api/local/localDelivery.ts`:
- Around line 245-247: Remove the double cast in createLocalDeliveryApi and
explicitly type the constructed api object as
NonNullable<WorkbenchServices['deliveryApi']>. Ensure the object satisfies the
declared delivery API contract directly so structural mismatches are caught by
type-checking, including the corresponding return path.
In `@wework/src/features/todo/CloudTodoWorkspace.test.tsx`:
- Around line 691-705: Replace the raw Tailwind class assertions in the
CloudTodoWorkspace test with user-observable state assertions on the save and
confirm controls, using toBeDisabled() or toBeEnabled() as appropriate for the
scenario. Remove checks for bg-black, text-white, disabled classes, and
disabled:opacity-50 while preserving coverage of the controls’ actual
disabled/enabled behavior.
In `@wework/src/features/todo/CloudTodoWorkspace.tsx`:
- Around line 367-531: Replace the hard-coded Chinese user-facing copy in
CloudTodoWorkspace.tsx lines 367-531 with keys through the local
`@/hooks/useTranslation` wrapper, including the dialog title,
project/location/task-source labels, repository and token labels, hints,
placeholders, and descriptions. Apply the same translation approach to
CloudProjectManageView.tsx lines 475-543 for the task-source, repository, token,
configured, and saved-provider text; add or reuse appropriate translation keys
while preserving the existing UI behavior.
In `@wework/src/features/todo/projectProviderConfig.test.ts`:
- Around line 4-14: Add focused tests in the repositoryProviderConfig suite for
owner/repo shorthand, git@host:group/project.git SSH URLs, github.com handling
without domain or api_base, and rejection when GitHub path segments are not
exactly two; also add coverage for repositoryAddress. Preserve the existing
GitLab test and assert each branch’s expected return value or thrown error.
🪄 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 Plus
Run ID: a26db0db-6ba5-432e-85c7-318f86290c70
⛔ Files ignored due to path filters (1)
executor/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (48)
backend/app/api/endpoints/cloud_projects.pybackend/app/core/provider_credentials.pybackend/app/mcp_server/tools/delivery.pybackend/app/models/delivery.pybackend/app/schemas/cloud_project.pybackend/app/services/cloud_projects/service.pybackend/app/services/loop_items/service.pybackend/app/services/runtime_work_service.pybackend/tests/api/test_cloud_projects_api.pybackend/tests/mcp_server/test_delivery_todo_tools.pybackend/tests/services/test_runtime_work_service.pyexecutor/Cargo.tomlexecutor/src/agents/mod.rsexecutor/src/bin/wegent-executor.rsexecutor/src/lib.rsexecutor/src/local/app_ipc.rsexecutor/src/runtime_work/handler/turns.rsexecutor/src/task_runtime/content.rsexecutor/src/task_runtime/credentials.rsexecutor/src/task_runtime/issue_provider.rsexecutor/src/task_runtime/mcp.rsexecutor/src/task_runtime/mod.rsexecutor/src/task_runtime/model.rsexecutor/src/task_runtime/router.rsexecutor/src/task_runtime/store.rsexecutor/tests/local_app_ipc_contract.rsexecutor/tests/local_task_mcp_contract.rswework/src/api/backend/backendServices.tswework/src/api/deliveries.tswework/src/api/hybrid/cloudProjectSpaceApi.test.tswework/src/api/hybrid/cloudProjectSpaceApi.tswework/src/api/hybrid/hybridServices.tswework/src/api/local/localDelivery.test.tswework/src/api/local/localDelivery.tswework/src/api/local/localServices.test.tswework/src/api/local/localServices.tswework/src/components/layout/DesktopAppSwitcher.test.tsxwework/src/components/layout/DesktopAppSwitcher.tsxwework/src/components/layout/DesktopWorkbenchLayout.test.tsxwework/src/components/layout/DesktopWorkbenchLayout.tsxwework/src/features/todo/CloudProjectManageView.tsxwework/src/features/todo/CloudTodoModal.tsxwework/src/features/todo/CloudTodoWorkspace.test.tsxwework/src/features/todo/CloudTodoWorkspace.tsxwework/src/features/todo/projectProviderConfig.test.tswework/src/features/todo/projectProviderConfig.tswework/src/features/workbench/workbenchServices.test.tswework/src/features/workbench/workbenchServices.ts
💤 Files with no reviewable changes (1)
- wework/src/components/layout/DesktopAppSwitcher.tsx
| if not token_supplied and current: | ||
| _preserve_credential(task_provider, current, config) | ||
| return config | ||
|
|
||
| normalized_token = token.strip() if isinstance(token, str) else "" | ||
| if normalized_token and normalized_token != "***": | ||
| config[CREDENTIAL_KEY] = _encrypt_provider_token( | ||
| normalized_token, | ||
| _credential_context(task_provider, config), | ||
| ) | ||
| return config |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Masked/empty token wipes the stored credential.
token_supplied short-circuits the preserve path, so an update that echoes the masked placeholder ("token": "***") or an empty string drops the existing credential from the config entirely — the project silently loses its provider credential. Since mask_provider_config is what clients read back, echoing *** is a realistic client behavior (the != "***" check acknowledges it).
🐛 Proposed fix
- if not token_supplied and current:
- _preserve_credential(task_provider, current, config)
- return config
-
normalized_token = token.strip() if isinstance(token, str) else ""
+ if (not token_supplied or not normalized_token or normalized_token == "***") and current:
+ _preserve_credential(task_provider, current, config)
+ return config
+
if normalized_token and normalized_token != "***":
config[CREDENTIAL_KEY] = _encrypt_provider_token(
normalized_token,
_credential_context(task_provider, config),
)
return config📝 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.
| if not token_supplied and current: | |
| _preserve_credential(task_provider, current, config) | |
| return config | |
| normalized_token = token.strip() if isinstance(token, str) else "" | |
| if normalized_token and normalized_token != "***": | |
| config[CREDENTIAL_KEY] = _encrypt_provider_token( | |
| normalized_token, | |
| _credential_context(task_provider, config), | |
| ) | |
| return config | |
| normalized_token = token.strip() if isinstance(token, str) else "" | |
| if (not token_supplied or not normalized_token or normalized_token == "***") and current: | |
| _preserve_credential(task_provider, current, config) | |
| return config | |
| if normalized_token and normalized_token != "***": | |
| config[CREDENTIAL_KEY] = _encrypt_provider_token( | |
| normalized_token, | |
| _credential_context(task_provider, config), | |
| ) | |
| return config |
🧰 Tools
🪛 Ruff (0.15.21)
[error] 50-50: Possible hardcoded password assigned to: "normalized_token"
(S105)
🤖 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/core/provider_credentials.py` around lines 45 - 55, Update the
credential handling flow around token_supplied and normalized_token so masked
("***") or empty tokens preserve the existing credential instead of removing it.
Reuse the existing _preserve_credential path when current is available, while
continuing to encrypt and store genuinely new non-masked tokens via
_encrypt_provider_token.
| fn run(&self, mut request: ExecutionRequest) -> Self::RunFuture { | ||
| crate::task_runtime::mcp::ensure_task_mcp_server(&mut request); |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# How each engine consumes mcp_servers, and whether the task server is filtered.
rg -nP --type=rust -C4 '\bmcp_servers\b' executor/src | head -120
rg -nP --type=rust -C3 'TASK_MCP_SERVER_NAME' executor/srcRepository: wecode-ai/Wegent
Length of output: 10352
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== agent outlines =="
for f in executor/src/agents/mod.rs executor/src/agents/claude_options.rs executor/src/agents/codex.rs executor/src/agents/dify.rs executor/src/agents/image_validator.rs executor/src/task_runtime/mcp.rs executor/src/protocol/openai.rs; do
if [ -f "$f" ]; then
echo "--- $f ---"
wc -l "$f"
fi
done
echo
echo "== relevant sections =="
sed -n '240,285p' executor/src/agents/mod.rs
sed -n '330,365p' executor/src/agents/mod.rs
sed -n '1,70p' executor/src/agents/claude_options.rs
sed -n '2550,2690p' executor/src/agents/codex.rs
sed -n '2696,2725p' executor/src/agents/codex.rs
echo
echo "== task_mcp_server tests =="
sed -n '365,410p' executor/src/task_runtime/mcp.rsRepository: wecode-ai/Wegent
Length of output: 12631
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate engine structs and run implementations =="
rg -n --type=rust -C3 'struct (CodexAppServerEngine|DifyEngine|ImageValidatorEngine)|impl .*Engine|fn run\(&self, request: ExecutionRequest\)|pub struct .*Engine|pub fn run\(' executor/src/agents executor/src/protocol | head -240
echo
echo "== codex server engine area =="
rg -n --type=rust -C4 'CodexAppServerEngine|struct Codex|run\(&self, request: ExecutionRequest\)' executor/src/agents/codex.rs | head -220
echo
echo "== dify/imagevalidator implementation sections =="
sed -n '1,220p' executor/src/agents/dify.rs
sed -n '1,212p' executor/src/agents/image_validator.rsRepository: wecode-ai/Wegent
Length of output: 21776
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== run_codex_app_server_turn definition and config construction =="
rg -n --type=rust -C5 'async fn run_codex_app_server_turn|fn run_codex_app_server_turn|for mut overrides|config_overrides|mcp_servers|CodexLaunchConfig' executor/src/agents/codex.rs | sed -n '1,260p'
echo
echo "== task runtime mcp implementation =="
sed -n '1,45p' executor/src/task_runtime/mcp.rs
sed -n '280,80p' executor/src/task_runtime/mcp.rsRepository: wecode-ai/Wegent
Length of output: 10896
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== stream_process_engine implementation snippets =="
rg -n --type=rust -C8 'struct StreamProcessEngine|impl AgentEngine for StreamProcessEngine|mcp_servers|mcp' executor/src/agents/mod.rs executor/src -g '*.rs' | head -180
echo
echo "== process agent command config/mcp references =="
rg -n --type=rust -C4 'AgentCommandPlanner|CommandSpec|mcp_servers|run_pre_execute_hook' executor/src/agents executor/src -g '*.rs' | sed -n '1,260p'Repository: wecode-ai/Wegent
Length of output: 30147
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== local process implementation files =="
rg -n --type=rust -C3 'pub struct (CommandSpec|StreamProcessEngine)|impl StreamProcessEngine|pub fn new|fn run\(' executor/src/process executor/src -g '*.rs' | sed -n '1,260p'
echo
echo "== search mcp_servers usage in process module =="
fs=$(git ls-files executor/src/process || true)
if [ -n "$fs" ]; then
rg -n --type-rust -C3 '\bmcp_servers\b|mcp' $fs | head -240 || true
else
echo "no executor/src/process files found"
fiRepository: wecode-ai/Wegent
Length of output: 15358
Gate the task MCP injection on agents that use mcp_servers.
ensure_task_mcp_server(&mut request) runs for AgentProcessEngine run and run_with_events, so Claude code runs get the extra wegent_tasks stdio server, but Dify and ImageValidator just carry an extra value through without using it. Keep the task server default if all mcp_servers consumers need it; otherwise restrict this injection to the engines that pass mcp_servers to their child runtime.
Also applies to lines 350-357.
🤖 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/src/agents/mod.rs` around lines 265 - 266, Restrict
ensure_task_mcp_server calls in AgentProcessEngine run and run_with_events to
only the execution engines whose child runtime consumes mcp_servers. Remove the
injection for Dify and ImageValidator paths, while preserving the existing
default task-server behavior for engines that actually pass mcp_servers onward.
| async fn handle_task_runtime_request(method: &str, params: Value) -> Result<Value, AppIpcError> { | ||
| let runtime = TaskRuntime::from_env().map_err(task_runtime_error)?; |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
A fresh TaskRuntime (SQLite open + pragmas + migration batch) is built on every task IPC request.
TaskRuntime::from_env() calls LocalTaskStore::open, which creates directories, opens a new connection, sets pragmas and runs the full CREATE TABLE … migration batch — per request, on the async runtime thread with no spawn_blocking. Under Todo board traffic this is repeated file I/O plus a growing set of independent WAL connections. Build it once (e.g. OnceCell<TaskRuntime> or a field on AppIpcServer) and reuse the cloneable handle.
🤖 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/src/local/app_ipc.rs` around lines 676 - 677, Update
handle_task_runtime_request to reuse a single initialized TaskRuntime instead of
calling TaskRuntime::from_env() for every IPC request. Store the runtime in an
appropriate OnceCell or AppIpcServer field, initialize it once, and reuse its
cloneable handle while preserving the existing task_runtime_error mapping.
| connection.execute( | ||
| "UPDATE loop_items SET path = ?1, name = ?2, version = version + 1, | ||
| updated_at = ?3 WHERE id = ?4", | ||
| params![path, file_name(&path), timestamp, file_id], | ||
| )?; | ||
| connection.execute( | ||
| "UPDATE loop_items SET path = ?1 || substr(path, ?2), | ||
| version = version + 1, updated_at = ?3 | ||
| WHERE resource_type = 'file' AND cloud_project_id = ?4 | ||
| AND path LIKE ?5 AND deleted_at IS NULL", | ||
| params![ | ||
| path, | ||
| old_path.len() as i64 + 1, | ||
| timestamp, | ||
| project_id, | ||
| format!("{old_path}/%"), | ||
| ], | ||
| )?; | ||
| drop(connection); | ||
| self.get_project_file(&project_id, file_id) | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Version check is not enforced by the UPDATE, and the descendant rewrite treats old_path as a LIKE pattern.
Two problems in this block:
- The optimistic-lock read (Line 120-128) is released before the writes, and the
UPDATEfilters only onid. Two concurrent moves both pass the check and the second silently wins. PushAND version = ?into theUPDATE(and run both statements in one immediate transaction) so a lost update surfaces asVersionConflict. format!("{old_path}/%")is used as aLIKEpattern, so_and%inside a real path act as wildcards. A file nameda_bwould also re-path descendants ofaxb. AddESCAPEwith an escaped pattern (the same applies todelete_project_file, Lines 172-195).
🤖 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/src/task_runtime/content.rs` around lines 138 - 158, The move
operation must enforce optimistic locking and safely match descendant paths.
Update the surrounding move method to keep the version read and both UPDATE
statements in one immediate transaction, add the expected version to the primary
UPDATE predicate, and surface a zero-row update as VersionConflict; escape LIKE
metacharacters in the old_path descendant pattern and add the corresponding
ESCAPE clause. Apply the same escaped-pattern and ESCAPE handling in
delete_project_file.
| pub(crate) fn new(database_path: PathBuf) -> Result<Self, TaskRuntimeError> { | ||
| let client = Client::builder() | ||
| .user_agent(concat!("wegent-executor/", env!("CARGO_PKG_VERSION"))) | ||
| .build() | ||
| .map_err(provider_error)?; | ||
| Ok(Self { | ||
| client, | ||
| database_path, | ||
| }) | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
No request timeout on the provider HTTP client.
Client::builder() has no timeout/connect_timeout, so a hung GitHub/GitLab endpoint (self-hosted GitLab is common here) stalls the task-runtime call indefinitely, and list_* compounds that across up to MAX_PAGES sequential requests.
🛡️ Proposed fix
let client = Client::builder()
.user_agent(concat!("wegent-executor/", env!("CARGO_PKG_VERSION")))
+ .connect_timeout(std::time::Duration::from_secs(10))
+ .timeout(std::time::Duration::from_secs(30))
.build()
.map_err(provider_error)?;📝 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.
| pub(crate) fn new(database_path: PathBuf) -> Result<Self, TaskRuntimeError> { | |
| let client = Client::builder() | |
| .user_agent(concat!("wegent-executor/", env!("CARGO_PKG_VERSION"))) | |
| .build() | |
| .map_err(provider_error)?; | |
| Ok(Self { | |
| client, | |
| database_path, | |
| }) | |
| } | |
| pub(crate) fn new(database_path: PathBuf) -> Result<Self, TaskRuntimeError> { | |
| let client = Client::builder() | |
| .user_agent(concat!("wegent-executor/", env!("CARGO_PKG_VERSION"))) | |
| .connect_timeout(std::time::Duration::from_secs(10)) | |
| .timeout(std::time::Duration::from_secs(30)) | |
| .build() | |
| .map_err(provider_error)?; | |
| Ok(Self { | |
| client, | |
| database_path, | |
| }) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@executor/src/task_runtime/issue_provider.rs` around lines 32 - 41, Configure
explicit request and connection timeouts on the HTTP client built in
IssueProvider::new before calling build, using the project’s established timeout
configuration or a bounded duration appropriate for provider calls. Preserve the
existing user agent, error mapping, and database_path initialization.
| @@ -349,7 +383,7 @@ export function CloudProjectManageView({ | |||
| data-testid="cloud-project-tag-create-confirm" | |||
| disabled={!newTag.trim() || tagBusy} | |||
| onClick={() => void createTag()} | |||
| className="h-9 rounded-lg bg-text-primary px-3.5 text-sm font-medium text-background transition hover:opacity-90 disabled:opacity-50" | |||
| className="h-9 rounded-lg bg-black px-3.5 text-sm font-medium text-white transition hover:bg-black/90 disabled:cursor-not-allowed disabled:bg-black disabled:text-white" | |||
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Primary buttons use raw bg-black/text-white with a disabled state identical to the enabled state. Both buttons bypass the neutral token pair (bg-text-primary/text-background) used elsewhere in this file and pin disabled:bg-black disabled:text-white, removing any disabled affordance.
wework/src/features/todo/CloudProjectManageView.tsx#L386-L386: switch the tag-create button tobg-text-primary text-backgroundwithdisabled:opacity-50.wework/src/features/todo/CloudProjectManageView.tsx#L530-L539: apply the same token/disabled styling to the provider save button, and relax the class assertions inCloudTodoWorkspace.test.tsxaccordingly.
As per coding guidelines: "Follow the neutral-first visual system: … inverse-neutral primary actions".
📍 Affects 1 file
wework/src/features/todo/CloudProjectManageView.tsx#L386-L386(this comment)wework/src/features/todo/CloudProjectManageView.tsx#L530-L539
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@wework/src/features/todo/CloudProjectManageView.tsx` at line 386, Update the
tag-create button at wework/src/features/todo/CloudProjectManageView.tsx:386 to
use bg-text-primary text-background and disabled:opacity-50 instead of raw
black/white classes. Apply the same styling to the provider save button at
wework/src/features/todo/CloudProjectManageView.tsx:530-539, and relax the
corresponding class assertions in CloudTodoWorkspace.test.tsx.
Source: Coding guidelines
| // Only render board items that belong to the selected project. On a project | ||
| // switch this flips to the skeleton in the same render, before the fetch. | ||
| // `boardError` distinguishes a failed fetch (skeleton stays) from a | ||
| // successfully loaded but empty project (renders the empty columns). | ||
| const boardItemsLoading = | ||
| selectedProject !== null && | ||
| (itemsProjectId !== selectedProjectId || (items.length === 0 && !boardError)) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
boardItemsLoading inverts its own documented intent: a successfully loaded empty project renders the skeleton forever, while a failed fetch renders empty columns.
After a successful fetch that returns no items, itemsProjectId === selectedProjectId, items.length === 0 and boardError === null, so the second clause is true and the board stays on CloudTodoBoardSkeleton permanently — a new/empty project space never shows its columns or the per-column "新建任务" dropzones. Conversely on a failure boardError is set, so the clause is false and the board renders as a normal empty board behind the error banner.
The load state should be derived from whether a snapshot for this project has arrived, not from emptiness.
🐛 Suggested fix
- const boardItemsLoading =
- selectedProject !== null &&
- (itemsProjectId !== selectedProjectId || (items.length === 0 && !boardError))
+ // A snapshot has arrived once `itemsProjectId` matches the selection; a failed
+ // fetch keeps the skeleton because `applyBoardItems` is only reached on success.
+ const boardItemsLoading = selectedProject !== null && itemsProjectId !== selectedProjectIdThis requires the failure branch in the refresh effect to stop calling applyBoardItems(selectedProjectId, [], …) and instead only set boardError, so a failed fetch leaves itemsProjectId unchanged and the skeleton stays.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@wework/src/features/todo/CloudTodoWorkspace.tsx` around lines 709 - 715,
Update the board loading state around boardItemsLoading to depend only on
whether the selected project’s item snapshot has arrived, removing the
items.length/boardError condition so successfully loaded empty projects render
their columns. In the refresh effect’s fetch failure branch, stop calling
applyBoardItems with an empty list; only set boardError so itemsProjectId
remains unchanged and the skeleton persists.
| .map(project => { | ||
| const ProjectLocationIcon = project.location === 'local' ? HardDrive : Cloud | ||
| return ( | ||
| <button | ||
| key={project.id} | ||
| type="button" | ||
| onClick={() => selectProject(project.id)} | ||
| className="grid h-12 w-full grid-cols-[minmax(0,1fr)_80px_120px_170px] items-center border-t border-border px-4 text-left transition-colors hover:bg-muted/60" | ||
| > |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
New project rows are interactive but carry no data-testid.
Both the sidebar buttons (Line 1050) and these grid rows are new clickable elements; the existing tests fall back to getAllByText('Wegent V4')[0], which is exactly the brittle selector the testid convention avoids.
As per coding guidelines: "All new interactive elements must have descriptive data-testid values."
🏷️ Suggested change
<button
key={project.id}
type="button"
+ data-testid={`cloud-project-row-${project.id}`}
onClick={() => selectProject(project.id)}📝 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.
| .map(project => { | |
| const ProjectLocationIcon = project.location === 'local' ? HardDrive : Cloud | |
| return ( | |
| <button | |
| key={project.id} | |
| type="button" | |
| onClick={() => selectProject(project.id)} | |
| className="grid h-12 w-full grid-cols-[minmax(0,1fr)_80px_120px_170px] items-center border-t border-border px-4 text-left transition-colors hover:bg-muted/60" | |
| > | |
| .map(project => { | |
| const ProjectLocationIcon = project.location === 'local' ? HardDrive : Cloud | |
| return ( | |
| <button | |
| key={project.id} | |
| type="button" | |
| data-testid={`cloud-project-row-${project.id}`} | |
| onClick={() => selectProject(project.id)} | |
| className="grid h-12 w-full grid-cols-[minmax(0,1fr)_80px_120px_170px] items-center border-t border-border px-4 text-left transition-colors hover:bg-muted/60" | |
| > |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@wework/src/features/todo/CloudTodoWorkspace.tsx` around lines 1172 - 1180,
Update the project row button rendered in the project map within
CloudTodoWorkspace to include a descriptive data-testid, and apply the same
convention to the new sidebar project buttons referenced near the project
selection UI. Keep the existing project.id key and selectProject click behavior
unchanged.
Source: Coding guidelines
| const shorthand = value.match(/^([^/\s]+)\/([^/\s]+)$/) | ||
| if (shorthand) { | ||
| return { repository: `${shorthand[1]}/${shorthand[2].replace(/\.git$/, '')}` } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
GitLab nested-group shorthand is rejected.
The shorthand regex only accepts a single slash, so group/subgroup/project (common on GitLab) falls through to new URL() and throws "请输入完整仓库地址". Consider allowing multi-segment shorthand when provider === 'gitlab'.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@wework/src/features/todo/projectProviderConfig.ts` around lines 16 - 19,
Update the shorthand parsing logic around the visible value.match expression so
GitLab accepts multi-segment repository paths such as group/subgroup/project,
while preserving the existing two-segment behavior for other providers. Use the
existing provider condition to select the GitLab-specific pattern, and keep the
repository normalization that removes a trailing .git.
| let parsed: URL | ||
| try { | ||
| parsed = new URL(value) | ||
| } catch { | ||
| throw new Error('请输入完整仓库地址，或使用 owner/repository 格式') | ||
| } | ||
| domain = parsed.hostname.toLowerCase() | ||
| pathname = parsed.pathname | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Port and scheme are dropped for self-hosted instances.
parsed.hostname strips any port and api_base hardcodes https://, so a self-hosted GitLab/GitHub Enterprise at http://git.internal:8443/... yields an unreachable https://git.internal/api/v4. Prefer parsed.host plus the parsed protocol (SSH input can keep the https default).
🔧 Proposed fix
const ssh = value.match(/^git@([^:]+):(.+)$/)
let domain: string
+ let scheme = 'https'
let pathname: string
if (ssh) {
domain = ssh[1].toLowerCase()
pathname = ssh[2]
} else {
let parsed: URL
try {
parsed = new URL(value)
} catch {
throw new Error('请输入完整仓库地址，或使用 owner/repository 格式')
}
- domain = parsed.hostname.toLowerCase()
+ domain = parsed.host.toLowerCase()
+ scheme = parsed.protocol.replace(':', '') || 'https'
pathname = parsed.pathname
}
@@
return {
repository,
domain,
- api_base: provider === 'github' ? `https://${domain}/api/v3` : `https://${domain}/api/v4`,
+ api_base: provider === 'github' ? `${scheme}://${domain}/api/v3` : `${scheme}://${domain}/api/v4`,
}Also applies to: 51-57
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@wework/src/features/todo/projectProviderConfig.ts` around lines 28 - 36,
Preserve the parsed host, including any port, and the URL protocol when deriving
the self-hosted repository configuration instead of using parsed.hostname and a
hardcoded HTTPS base. Update the related parsing and api_base construction
paths, including the analogous block, while retaining HTTPS as the default for
SSH-format inputs.
2 participants
Summary by CodeRabbit