feat: 重写TODO - #2185
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds a cloud collaboration platform spanning backend projects, loop items, deliveries, shared files, MCP tools, runtime context, Tauri persistence, and WeWork UI. It also adds migrations, APIs, authorization, storage workflows, cloud mentions, delivery dialogs, workspace management, tests, and documentation. ChangesCloud collaboration platform
Estimated code review effort: 5 (Critical) | ~120 minutes 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 |
2c5df9d to
77b6633
Compare
# Conflicts: # wework/src/components/chat/composer/CompactChatComposer.tsx # wework/src/components/chat/composer/ProjectChatComposer.tsx
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 18
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/app/models/cloud_project.py (1)
1-19: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winRemove the unused compatibility shim.
app.models.cloud_projecthas no remaining Python importers and only re-exports models fromapp.models.delivery, so the old module path adds dead complexity. Update any future imports to useapp.models.deliverydirectly and deletebackend/app/models/cloud_project.py.🤖 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/models/cloud_project.py` around lines 1 - 19, Remove the obsolete app.models.cloud_project compatibility module and its re-exports. Verify there are no remaining imports using that module path, and update any references to import CloudProject, CloudProjectFile, CloudProjectLocalBinding, or LoopItemTaskBinding directly from app.models.delivery.Source: Path instructions
🧹 Nitpick comments (13)
backend/tests/mcp_server/test_delivery_tools.py (1)
55-60: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winType the pytest fixture parameters.
Annotate both
monkeypatchparameters aspytest.MonkeyPatchand add thepytestimport. This keeps the new test signatures consistent with the repository’s Python typing requirement.As per coding guidelines,
**/*.pyfiles should use Python type hints.Also applies to: 72-77
🤖 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/mcp_server/test_delivery_tools.py` around lines 55 - 60, Update both affected test functions, including test_regular_user_token_can_authenticate_for_user_scoped_mcp and the test at the referenced second location, to annotate their monkeypatch parameters as pytest.MonkeyPatch. Add the pytest import required for these annotations while preserving the existing test behavior.Source: Coding guidelines
backend/app/api/api.py (1)
135-138: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCloud-project sub-resources are split across two router modules.
cloud_projects.router(prefix/v1/cloud-projects) anddeliveries.router(prefix/v1, but defining/cloud-projects/{project_id}/tasksand/cloud-projects/{project_id}/loop-items) both own paths under the same/v1/cloud-projects/...namespace. Consider moving the loop-item/task-binding routes for a project intocloud_projects.py, or renaming thedeliveries.pyroutes, for a single, cohesive owner of the cloud-project resource family.🤖 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/api.py` around lines 135 - 138, Consolidate the cloud-project sub-resource routes currently defined in deliveries.router, especially the project task and loop-item binding endpoints, under cloud_projects.router in cloud_projects.py. Update the router registration and endpoint prefixes as needed so all /v1/cloud-projects/... paths have one cohesive owner without changing their external API behavior.backend/app/api/endpoints/deliveries.py (1)
41-47: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAvoid spreading ORM
__dict__intomodel_validate.
{**delivery.__dict__, ...}and{**binding.__dict__, ...}leak SQLAlchemy's internal_sa_instance_statekey into the dict passed tomodel_validate, and silently omit any attribute not already loaded on the instance. This happens to work only because pydantic's defaultextra="ignore"swallows the unexpected key. Prefer building the response from named attributes (orResponse.model_validate(obj, from_attributes=True)plus amodel_copy(update={...})for the extra fields) instead of relying on ORM instance internals.♻️ Example refactor for `_delivery_response`
def _delivery_response(db: Session, delivery: Delivery) -> DeliveryResponse: - return DeliveryResponse.model_validate( - { - **delivery.__dict__, - "assets": delivery_service.list_assets(db, delivery.id), - } - ) + base = DeliveryResponse.model_validate(delivery, from_attributes=True) + return base.model_copy( + update={"assets": delivery_service.list_assets(db, delivery.id)} + )Also applies to: 132-138
🤖 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/deliveries.py` around lines 41 - 47, Update _delivery_response and the corresponding binding response construction to stop spreading ORM __dict__ into model_validate. Build each response from declared model attributes, or validate the ORM object with from_attributes=True and apply extra fields such as assets via model_copy(update={...}); preserve the existing response data and avoid leaking _sa_instance_state or depending on loaded-instance attributes.backend/app/services/cloud_files/service.py (1)
106-165: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
upload()andmove()exceed the ~50-line function-length guideline.Both methods mix multiple concerns (validation, streaming/hashing, storage I/O, and DB persistence with error handling) in a single ~60-80 line function. Consider extracting the streaming/hashing loop and the copy/rollback logic into small private helpers for readability.
As per coding guidelines: "Use English comments, clear names, Python type hints, focused functions preferably under 50 lines, PEP 8, Black with 88 columns, and isort."
Also applies to: 180-257
🤖 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_files/service.py` around lines 106 - 165, Refactor upload() and move() into focused private helpers so each public method stays under roughly 50 lines. Extract the upload streaming, size validation, and SHA-256 staging logic from upload(), and extract move()’s copy and rollback handling into a dedicated helper. Preserve existing validation, storage, database persistence, cleanup, and exception behavior while using clear type-hinted helper names.Source: Coding guidelines
backend/app/schemas/cloud_project.py (1)
74-94: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate
reject_ownervalidator.
CloudProjectMemberCreateandCloudProjectMemberUpdateboth define an identicalreject_ownervalidator. As per path instructions,**/*.{py,ts,tsx}guidance says: "extract shared logic instead of duplicating it." Factor this into one reusable validator function shared by both models.♻️ Suggested shared validator
+def _reject_owner_role(value: BaseRole) -> BaseRole: + if value == BaseRole.Owner: + raise ValueError("Owner cannot be assigned") + return value + + class CloudProjectMemberCreate(BaseModel): user_id: int = Field(ge=1) role: BaseRole = BaseRole.Developer - `@field_validator`("role") - `@classmethod` - def reject_owner(cls, value: BaseRole) -> BaseRole: - if value == BaseRole.Owner: - raise ValueError("Owner cannot be assigned") - return value + _validate_role = field_validator("role")(classmethod(lambda cls, v: _reject_owner_role(v))) class CloudProjectMemberUpdate(BaseModel): role: BaseRole - `@field_validator`("role") - `@classmethod` - def reject_owner(cls, value: BaseRole) -> BaseRole: - if value == BaseRole.Owner: - raise ValueError("Owner cannot be assigned") - return value + _validate_role = field_validator("role")(classmethod(lambda cls, v: _reject_owner_role(v)))🤖 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 74 - 94, Extract the duplicated reject_owner logic from CloudProjectMemberCreate and CloudProjectMemberUpdate into one reusable validator function, then reference that function from both role validators. Preserve the existing BaseRole.Owner rejection and return behavior, while keeping each model’s validation contract unchanged.Source: Path instructions
backend/app/core/security.py (1)
102-105: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winDuplicated raw query across three auth entry points; verify parity with removed
get_user_by_name.All three functions now inline the identical
db.query(User).filter(User.user_name == username).first()instead of a shared helper. As per path instructions,**/*.{py,ts,tsx}guidance says: "Before adding code, search for and reuse existing components, services, utilities, and patterns; extract shared logic instead of duplicating it." Consider a small module-level helper (e.g._load_user_by_username(db, username)) used by all three call sites.Separately, since
user_service.get_user_by_nameis no longer used here, please confirm it didn't perform additional logic (username normalization/case-insensitive match, caching, etc.) that this direct query doesn't replicate — this is an authentication-critical path.♻️ Suggested helper extraction
def _load_user_by_username(db: Session, username: str) -> Optional[User]: """Load a user by name without decrypting optional Git credentials.""" return db.query(User).filter(User.user_name == username).first()Also applies to: 398-401, 1138-1139
🤖 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/security.py` around lines 102 - 105, Extract the duplicated username lookup into a module-level helper such as _load_user_by_username, preserving the direct User query without Git credential decryption, and update all three authentication entry points to use it. Verify the removed user_service.get_user_by_name behavior for normalization, case-insensitive matching, caching, or other logic, and preserve any authentication-relevant behavior in the shared helper.Source: Path instructions
backend/app/models/delivery.py (1)
29-182: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftSingle mega-table across 9 polymorphic entity types.
LoopNodebacksCloudProject,LoopItem,CloudProjectLocalBinding,LoopItemTaskBinding,CloudProjectFile,LoopItemAttachment,LoopItemCollaborator,Delivery, andDeliveryAssetvia single-table inheritance on oneloop_itemstable with ~90 largely-nullable columns. This is a classic STI "god table" trade-off: no DB-level constraint prevents adeliveryrow from havingdevice_id/task_idpopulated, every row carries columns irrelevant to its type, and adding a column for any one subtype widens every row. Four independent self-referencingondelete="CASCADE"FKs (cloud_project_id,parent_id,loop_item_id,delivery_id) into the same table also multiply cascade-delete paths that should be exercised with integration tests to confirm no unintended fan-out deletes.Given this model underpins the rest of the cloud-collaboration PR stack, a full split into per-entity tables now would be costly, but at minimum consider CHECK constraints (or DB triggers) enforcing which columns are legal per
resource_type, and explicit cascade-delete integration tests.🤖 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/models/delivery.py` around lines 29 - 182, Add database-level CHECK constraints to LoopNode.__table_args__ that restrict subtype-specific fields according to resource_type, preventing unrelated columns from being populated on each polymorphic entity. Preserve the existing single-table inheritance and foreign-key cascade behavior, and add integration coverage for deletes through cloud_project_id, parent_id, loop_item_id, and delivery_id to verify cascades do not remove unintended rows.wework/src/features/todo/CloudTodoWorkspace.tsx (2)
1185-1192: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffNew Wework UI should use the translation wrapper.
This component (and its sibling cloud dialogs:
CloudProjectSettingsDialog,CloudTodoModal,TaskDescriptionEditor) hardcode user-facing Chinese strings instead of going through the i18n wrapper. New copy should be added to the Wework namespace in bothenandzh-CNand consumed via the translation hook. As per path instructions: "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 1185 - 1192, Replace hardcoded user-facing Chinese strings in CloudTodoWorkspace and its sibling cloud dialogs CloudProjectSettingsDialog, CloudTodoModal, and TaskDescriptionEditor with the local `@/hooks/useTranslation` wrapper. Add each new message to the Wework namespace in both en and zh-CN, then consume the translated keys through the hook while preserving existing behavior.Source: Path instructions
1242-1263: 🚀 Performance & Scalability | 🔵 Trivial | ⚖️ Poor tradeoffProject counts fan out to one request per project (N+1).
For every project returned by
listCloudProjects, a separatelistLoopItemscall is issued just to read.items.length. With many projects this multiplies round-trips on initial load. Consider a backend-provided count field or a single aggregated counts endpoint.🤖 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 1242 - 1263, The project-loading effect currently creates an N+1 request pattern by calling listLoopItems for every project. Update the flow around listCloudProjects to use a backend-provided project count or a single aggregated counts endpoint, then populate setProjectCounts from that result while preserving the existing active guard and loading-state cleanup.wework/src/features/todo/CloudTodoModal.tsx (1)
10-33: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider keyboard dismissal / focus handling for the modal.
The modal closes only on backdrop
mousedown; there's no Escape-to-close or focus trapping. For a shared modal used across the cloud workspace, addingEscapehandling and initial focus would improve keyboard accessibility.🤖 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/CloudTodoModal.tsx` around lines 10 - 33, Enhance CloudTodoModal with keyboard accessibility by closing via Escape and moving initial focus into the modal when it opens. Add the necessary dialog semantics and focus management around the existing section, while preserving the current backdrop mousedown and close-button behavior.wework/src/features/todo/CloudFilesView.tsx (1)
79-90: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
deleteFiledoesn't callrefresh()after success, unlikecreateFolder/moveFile.Relies solely on the local optimistic filter; if the server-side deletion state diverges (e.g., concurrent edits), the view won't reconcile until the next full refresh.
🤖 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/CloudFilesView.tsx` around lines 79 - 90, Update deleteFile to call the existing refresh() after api.deleteCloudFile succeeds, while retaining the current local file filtering and error handling. Ensure refresh runs only after a successful deletion.wework/src/components/layout/useWorkbenchPaneSession.ts (1)
1062-1063: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicate additionalContext merge logic.
The
{ ...message.additionalContext, ...terminalContext }merge pattern is duplicated insendRuntimeMessageandinterruptAndSendQueuedMessage. Consider extracting a small helper to avoid drift if the merge semantics change later.Also applies to: 1109-1110
🤖 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/layout/useWorkbenchPaneSession.ts` around lines 1062 - 1063, Extract the duplicated additionalContext merge from sendRuntimeMessage and interruptAndSendQueuedMessage into a shared local helper that reads the runtime terminal context and combines it with message.additionalContext in the existing order. Replace both inline merge expressions with the helper while preserving terminalContext precedence.wework/src/features/todo/TodoWorkflowDialog.tsx (1)
261-270: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd
data-testidto newly added dialog buttons. Several new interactive buttons in these dialogs lack a descriptivedata-testid, which the wework guideline requires for E2E coverage.
wework/src/features/todo/TodoWorkflowDialog.tsx#L261-L270: add descriptivedata-testidvalues (e.g.todo-workflow-clear,todo-workflow-cancel) to the "清空流程" and "取消" buttons.wework/src/features/todo/TodoBindingPicker.tsx#L247-L251: add a descriptivedata-testid(e.g.todo-binding-create-back) to the "返回" back button.As per coding guidelines: "All new interactive elements must have descriptive
data-testidvalues."🤖 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/TodoWorkflowDialog.tsx` around lines 261 - 270, Add descriptive data-testid attributes to the clear-workflow and cancel Buttons in TodoWorkflowDialog.tsx, using distinct identifiers such as todo-workflow-clear and todo-workflow-cancel. Also add a descriptive data-testid, such as todo-binding-create-back, to the back Button in TodoBindingPicker.tsx.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/app/schemas/delivery.py`:
- Around line 162-174: Update DeliveryResponse.source_task_binding_id to use the
schema’s str-based SnowflakeId type, matching the String(64) model column and
other large ID fields in the schema. Preserve its optionality and existing null
behavior.
In `@backend/app/schemas/runtime_work.py`:
- Around line 720-722: Update RuntimeTaskCreateRequest.delivery_id to validate
the production delivery.id value rather than requiring a 36-character UUID,
matching the delivery model’s String(64) constraint and the finalize() payload.
Preserve the deliveryId alias and optional default while allowing the runtime
worker to serialize actual delivery IDs.
In `@backend/app/services/cloud_files/service.py`:
- Around line 180-223: Update move() to reject folder moves when target_path
equals file.path or is nested beneath it, before descendant path rewriting and
conflict detection. Raise the existing conflict HTTPException for these invalid
self-subtree moves, while preserving the current behavior for files and
destinations outside the folder’s subtree.
- Around line 259-287: Update the delete method to commit the database deletions
before calling self.storage.remove_objects(object_keys), matching the successful
ordering used by move. Preserve the existing object-key collection and recursive
child deletion behavior, and only remove storage objects after db.commit
succeeds.
In `@build_image.sh`:
- Line 18: Remove the stray “aass” suffix from the help-title echo command so
the build_image.sh help output displays exactly “Build docker images for Wegent
components”.
In `@docs/en/wegent/developer-guide/cloud-project-collaboration.md`:
- Line 7: Remove the developer-specific absolute design path from both
docs/en/wegent/developer-guide/cloud-project-collaboration.md:7-7 and
docs/zh/wegent/developer-guide/cloud-project-collaboration.md:7-7, replacing it
with a repository-hosted or published reference, or removing the note entirely.
In `@executor/src/agents/runtime_capabilities.rs`:
- Around line 1667-1677: The header override handling in the runtime
capabilities builder must not emit Authorization bearer tokens as plaintext
command-line config values. Update the logic around the headers iteration to
route bearer-token values through an environment-variable override and reference
that variable from the generated configuration, while ensuring the resulting
command arguments are excluded from executor launch logging.
In `@wework/src-tauri/src/lib.rs`:
- Around line 1937-1986: Update collect_selected_files and read_dropped_files to
bound a drop with explicit maximum total bytes, file count, and directory
recursion depth, rejecting or skipping entries once limits are reached. Handle
metadata, directory reads, recursion, and file reads per entry so failures are
reported or skipped without aborting the entire drop or discarding files
collected from other top-level paths. Preserve successfully collected files
while enforcing the caps before reading or appending file contents.
In `@wework/src-tauri/src/todo_store.rs`:
- Around line 82-99: Update save_todo_store so each invocation uses a unique
temporary path for the scoped target instead of the deterministic
target.with_extension("json.tmp") path. Generate the temporary filename from the
target plus a per-call unique identifier, while preserving the existing
write-then-rename commit flow and error handling.
In `@wework/src/components/layout/DesktopWorkbenchLayout.tsx`:
- Around line 106-112: Update the todoOpen computation in DesktopWorkbenchLayout
to depend only on whether currentPath equals '/todo', removing the
cloudConnection.isConnected requirement. Preserve the existing currentPath
routing so Todo remains accessible and can handle unavailable cloud connections
or display local items itself.
In `@wework/src/components/layout/DesktopWorkbenchMain.tsx`:
- Around line 174-192: Scope pendingTodoBinding to the active pane key so
bindings cannot leak between DesktopWorkbenchPane sessions. Update
pendingTodoForTask and pendingProjectForTask to read only the binding associated
with the current pane, and ensure pane identity changes do not reuse a prior
pane’s unscoped binding.
In `@wework/src/components/layout/EnvironmentInfoPopover.tsx`:
- Around line 469-498: Update the fallback labels in the EnvironmentInfoPopover
button block to use the existing t translation helper, and add matching keys
with English and Simplified Chinese values in the appropriate Wework locale
namespaces. Preserve todoLabel usage while ensuring both fallback strings
resolve through i18n.
In `@wework/src/features/todo/CloudFilesView.tsx`:
- Around line 108-321: The CloudFilesView component uses hardcoded Chinese UI
strings instead of the Wework translation system. Add the required keys to the
appropriate English and zh-CN locale namespaces, use the local useTranslation
hook and t(...) throughout CloudFilesView for headings, labels, buttons,
placeholders, empty states, status text, and aria-labels, and preserve the
existing dynamic values and behavior.
- Line 80: Replace the native window.confirm guard in the delete handler with
the application’s existing cross-platform dialog workflow, or Tauri’s
`@tauri-apps/plugin-dialog` confirm API. Await the confirmation result before
continuing deletion, and return immediately when the user declines or the dialog
cannot confirm consent.
In `@wework/src/features/todo/CloudTodoWorkspace.tsx`:
- Around line 664-686: The data-loading useEffect in CloudTodoWorkspace should
add an active/unmount guard and catch failures from the Promise.all request
batch. Reuse the error-state pattern already present elsewhere in the file, only
update state while the effect remains active, and deactivate the flag during
cleanup before applying delivery, task, attachment, collaborator, or
project-member results.
- Around line 1289-1311: Update startTask to wrap the onRunTodo, api.bindTask,
and conditional api.updateLoopItem flow in try/catch, report the failure through
the existing visible error mechanism, and preserve dialog cleanup while
preventing unhandled rejections from the StartTaskDialog call site.
- Around line 1846-1852: Update the DragOverlay rendering in CloudTodoWorkspace
to store or derive the result of items.find for activeDragItemId and render
TodoCardContent only when that item exists. Remove the non-null assertion, while
preserving the existing overlay styling and null rendering when no active drag
item is available.
In `@wework/src/features/todo/todoModel.ts`:
- Around line 320-329: Update the version-1 config normalization around
value.workTypes to default a missing or null workTypes value to an empty array
before calling map. Preserve the existing workType field defaults and return the
normalized config instead of allowing the mapping error to trigger the fallback
to DEFAULT_TODO_WORKFLOW.
---
Outside diff comments:
In `@backend/app/models/cloud_project.py`:
- Around line 1-19: Remove the obsolete app.models.cloud_project compatibility
module and its re-exports. Verify there are no remaining imports using that
module path, and update any references to import CloudProject, CloudProjectFile,
CloudProjectLocalBinding, or LoopItemTaskBinding directly from
app.models.delivery.
---
Nitpick comments:
In `@backend/app/api/api.py`:
- Around line 135-138: Consolidate the cloud-project sub-resource routes
currently defined in deliveries.router, especially the project task and
loop-item binding endpoints, under cloud_projects.router in cloud_projects.py.
Update the router registration and endpoint prefixes as needed so all
/v1/cloud-projects/... paths have one cohesive owner without changing their
external API behavior.
In `@backend/app/api/endpoints/deliveries.py`:
- Around line 41-47: Update _delivery_response and the corresponding binding
response construction to stop spreading ORM __dict__ into model_validate. Build
each response from declared model attributes, or validate the ORM object with
from_attributes=True and apply extra fields such as assets via
model_copy(update={...}); preserve the existing response data and avoid leaking
_sa_instance_state or depending on loaded-instance attributes.
In `@backend/app/core/security.py`:
- Around line 102-105: Extract the duplicated username lookup into a
module-level helper such as _load_user_by_username, preserving the direct User
query without Git credential decryption, and update all three authentication
entry points to use it. Verify the removed user_service.get_user_by_name
behavior for normalization, case-insensitive matching, caching, or other logic,
and preserve any authentication-relevant behavior in the shared helper.
In `@backend/app/models/delivery.py`:
- Around line 29-182: Add database-level CHECK constraints to
LoopNode.__table_args__ that restrict subtype-specific fields according to
resource_type, preventing unrelated columns from being populated on each
polymorphic entity. Preserve the existing single-table inheritance and
foreign-key cascade behavior, and add integration coverage for deletes through
cloud_project_id, parent_id, loop_item_id, and delivery_id to verify cascades do
not remove unintended rows.
In `@backend/app/schemas/cloud_project.py`:
- Around line 74-94: Extract the duplicated reject_owner logic from
CloudProjectMemberCreate and CloudProjectMemberUpdate into one reusable
validator function, then reference that function from both role validators.
Preserve the existing BaseRole.Owner rejection and return behavior, while
keeping each model’s validation contract unchanged.
In `@backend/app/services/cloud_files/service.py`:
- Around line 106-165: Refactor upload() and move() into focused private helpers
so each public method stays under roughly 50 lines. Extract the upload
streaming, size validation, and SHA-256 staging logic from upload(), and extract
move()’s copy and rollback handling into a dedicated helper. Preserve existing
validation, storage, database persistence, cleanup, and exception behavior while
using clear type-hinted helper names.
In `@backend/tests/mcp_server/test_delivery_tools.py`:
- Around line 55-60: Update both affected test functions, including
test_regular_user_token_can_authenticate_for_user_scoped_mcp and the test at the
referenced second location, to annotate their monkeypatch parameters as
pytest.MonkeyPatch. Add the pytest import required for these annotations while
preserving the existing test behavior.
In `@wework/src/components/layout/useWorkbenchPaneSession.ts`:
- Around line 1062-1063: Extract the duplicated additionalContext merge from
sendRuntimeMessage and interruptAndSendQueuedMessage into a shared local helper
that reads the runtime terminal context and combines it with
message.additionalContext in the existing order. Replace both inline merge
expressions with the helper while preserving terminalContext precedence.
In `@wework/src/features/todo/CloudFilesView.tsx`:
- Around line 79-90: Update deleteFile to call the existing refresh() after
api.deleteCloudFile succeeds, while retaining the current local file filtering
and error handling. Ensure refresh runs only after a successful deletion.
In `@wework/src/features/todo/CloudTodoModal.tsx`:
- Around line 10-33: Enhance CloudTodoModal with keyboard accessibility by
closing via Escape and moving initial focus into the modal when it opens. Add
the necessary dialog semantics and focus management around the existing section,
while preserving the current backdrop mousedown and close-button behavior.
In `@wework/src/features/todo/CloudTodoWorkspace.tsx`:
- Around line 1185-1192: Replace hardcoded user-facing Chinese strings in
CloudTodoWorkspace and its sibling cloud dialogs CloudProjectSettingsDialog,
CloudTodoModal, and TaskDescriptionEditor with the local `@/hooks/useTranslation`
wrapper. Add each new message to the Wework namespace in both en and zh-CN, then
consume the translated keys through the hook while preserving existing behavior.
- Around line 1242-1263: The project-loading effect currently creates an N+1
request pattern by calling listLoopItems for every project. Update the flow
around listCloudProjects to use a backend-provided project count or a single
aggregated counts endpoint, then populate setProjectCounts from that result
while preserving the existing active guard and loading-state cleanup.
In `@wework/src/features/todo/TodoWorkflowDialog.tsx`:
- Around line 261-270: Add descriptive data-testid attributes to the
clear-workflow and cancel Buttons in TodoWorkflowDialog.tsx, using distinct
identifiers such as todo-workflow-clear and todo-workflow-cancel. Also add a
descriptive data-testid, such as todo-binding-create-back, to the back Button in
TodoBindingPicker.tsx.
🪄 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: c3e32985-a92f-49f4-9135-962cd946cccb
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (114)
.gitignorebackend/alembic/versions/20260719_051cd1f603d6_reconcile_local_database_head.pybackend/alembic/versions/20260724_a6d94c3e5217_add_loop_items.pybackend/app/api/api.pybackend/app/api/endpoints/cloud_projects.pybackend/app/api/endpoints/deliveries.pybackend/app/core/config.pybackend/app/core/security.pybackend/app/main.pybackend/app/mcp_server/auth.pybackend/app/mcp_server/context.pybackend/app/mcp_server/server.pybackend/app/mcp_server/tools/delivery.pybackend/app/models/__init__.pybackend/app/models/cloud_project.pybackend/app/models/delivery.pybackend/app/models/share_link.pybackend/app/schemas/cloud_file.pybackend/app/schemas/cloud_project.pybackend/app/schemas/delivery.pybackend/app/schemas/runtime_work.pybackend/app/services/cloud_files/__init__.pybackend/app/services/cloud_files/service.pybackend/app/services/cloud_projects/__init__.pybackend/app/services/cloud_projects/access.pybackend/app/services/cloud_projects/service.pybackend/app/services/delivery/__init__.pybackend/app/services/delivery/access.pybackend/app/services/delivery/service.pybackend/app/services/delivery/storage.pybackend/app/services/loop_items/__init__.pybackend/app/services/loop_items/service.pybackend/app/services/project_service.pybackend/app/services/runtime_work_service.pybackend/tests/api/endpoints/test_runtime_work_api.pybackend/tests/api/test_cloud_projects_api.pybackend/tests/api/test_deliveries_api.pybackend/tests/core/test_security.pybackend/tests/mcp_server/test_delivery_tools.pybackend/tests/services/test_runtime_work_service.pybuild_image.shdocs/en/wegent/developer-guide/cloud-project-collaboration.mddocs/zh/wegent/developer-guide/cloud-project-collaboration.mdexecutor/src/agents/runtime_capabilities.rsexecutor/tests/codex_app_server_contract.rswework/package.jsonwework/src-tauri/src/lib.rswework/src-tauri/src/todo_store.rswework/src-tauri/tauri.conf.jsonwework/src/App.apps.test.tsxwework/src/App.tsxwework/src/api/backend/backendServices.tswework/src/api/deliveries.tswework/src/api/http.test.tswework/src/api/http.tswework/src/api/hybrid/hybridServices.test.tswework/src/api/hybrid/hybridServices.tswework/src/api/local/localServices.test.tswework/src/api/local/localServices.tswework/src/components/chat/ChatInput.tsxwework/src/components/chat/MessageList.test.tsxwework/src/components/chat/MessageList.tsxwework/src/components/chat/composer/CompactChatComposer.tsxwework/src/components/chat/composer/ComposerMentionMenu.tsxwework/src/components/chat/composer/ComposerTextarea.test.tsxwework/src/components/chat/composer/ComposerTextarea.tsxwework/src/components/chat/composer/ComposerToolbar.tsxwework/src/components/chat/composer/ProjectChatComposer.tsxwework/src/components/chat/composer/composerMentionCandidates.tswework/src/components/chat/composer/composerMentions.test.tswework/src/components/chat/composer/composerMentions.tswework/src/components/chat/composer/composerTextareaTypes.tswework/src/components/chat/composer/useComposerMentionCandidates.tswework/src/components/layout/DesktopAppSwitcher.test.tsxwework/src/components/layout/DesktopAppSwitcher.tsxwework/src/components/layout/DesktopWorkbenchLayout.tsxwework/src/components/layout/DesktopWorkbenchMain.tsxwework/src/components/layout/EnvironmentInfoPopover.test.tsxwework/src/components/layout/EnvironmentInfoPopover.tsxwework/src/components/layout/useWorkbenchPaneSession.tswework/src/components/layout/workspace-panels/WorkspacePanelActions.tsxwework/src/features/delivery/DeliveryDialog.test.tsxwework/src/features/delivery/DeliveryDialog.tsxwework/src/features/todo/CloudFilesView.test.tsxwework/src/features/todo/CloudFilesView.tsxwework/src/features/todo/CloudProjectSettingsDialog.tsxwework/src/features/todo/CloudTodoModal.tsxwework/src/features/todo/CloudTodoWorkspace.test.tsxwework/src/features/todo/CloudTodoWorkspace.tsxwework/src/features/todo/TaskDescriptionEditor.test.tsxwework/src/features/todo/TaskDescriptionEditor.tsxwework/src/features/todo/TodoBindingPicker.test.tsxwework/src/features/todo/TodoBindingPicker.tsxwework/src/features/todo/TodoCreateDialog.tsxwework/src/features/todo/TodoDetailPanel.tsxwework/src/features/todo/TodoMyWork.tsxwework/src/features/todo/TodoNavigation.tsxwework/src/features/todo/TodoWorkItems.tsxwework/src/features/todo/TodoWorkflowDialog.tsxwework/src/features/todo/TodoWorkspace.test.tsxwework/src/features/todo/TodoWorkspace.tsxwework/src/features/todo/localTodoProjects.test.tswework/src/features/todo/localTodoProjects.tswework/src/features/todo/taskDescription.tswework/src/features/todo/todoModel.test.tswework/src/features/todo/todoModel.tswework/src/features/workbench/useWorkbenchRuntimeMessaging.tswework/src/features/workbench/workbenchContextTypes.tswework/src/features/workbench/workbenchServices.tswework/src/i18n/locales/en/common.jsonwework/src/i18n/locales/zh-CN/common.jsonwework/src/styles/globals.csswework/src/tauri/droppedFiles.tswework/src/types/api.ts
💤 Files with no reviewable changes (2)
- wework/src/features/todo/TodoWorkspace.test.tsx
- wework/src/features/todo/TodoWorkspace.tsx
| class DeliveryResponse(BaseModel): | ||
| model_config = ConfigDict(from_attributes=True) | ||
|
|
||
| id: str | ||
| loop_item_id: str | ||
| created_by_user_id: int | ||
| source_task_binding_id: int | None | ||
| source_task_snapshot: dict[str, Any] | None | ||
| status: Literal["draft", "delivered"] | ||
| created_at: datetime | ||
| delivered_at: datetime | None | ||
| assets: list[DeliveryAssetResponse] = Field(default_factory=list) | ||
|
|
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
source_task_binding_id typed int but backed by a String(64) snowflake ID.
backend/app/models/delivery.py defines source_task_binding_id = Column(String(64), nullable=True), storing LoopItemTaskBinding.id values generated by _numeric_id() (up to ~9×10^18). Every other such ID in this schema file uses SnowflakeId (str-based) specifically to avoid JSON numeric precision loss for large IDs (e.g. id: SnowflakeId, cloud_project_id: SnowflakeId). Typing this field as int breaks that convention and risks precision loss for large IDs when serialized to JSON for JS/browser clients.
🐛 Proposed fix
- source_task_binding_id: int | None
+ source_task_binding_id: SnowflakeId | None📝 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.
| class DeliveryResponse(BaseModel): | |
| model_config = ConfigDict(from_attributes=True) | |
| id: str | |
| loop_item_id: str | |
| created_by_user_id: int | |
| source_task_binding_id: int | None | |
| source_task_snapshot: dict[str, Any] | None | |
| status: Literal["draft", "delivered"] | |
| created_at: datetime | |
| delivered_at: datetime | None | |
| assets: list[DeliveryAssetResponse] = Field(default_factory=list) | |
| class DeliveryResponse(BaseModel): | |
| model_config = ConfigDict(from_attributes=True) | |
| id: str | |
| loop_item_id: str | |
| created_by_user_id: int | |
| source_task_binding_id: SnowflakeId | None | |
| source_task_snapshot: dict[str, Any] | None | |
| status: Literal["draft", "delivered"] | |
| created_at: datetime | |
| delivered_at: datetime | None | |
| assets: list[DeliveryAssetResponse] = Field(default_factory=list) |
🤖 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/delivery.py` around lines 162 - 174, Update
DeliveryResponse.source_task_binding_id to use the schema’s str-based
SnowflakeId type, matching the String(64) model column and other large ID fields
in the schema. Preserve its optionality and existing null behavior.
| delivery_id: Optional[str] = Field( | ||
| default=None, alias="deliveryId", min_length=36, max_length=36 | ||
| ) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check how delivery ids are generated/returned and whether callers pass id vs public_id
rg -n "delivery_id|deliveryId" backend/app/services/delivery/service.py wework/src/api/deliveries.ts 2>/dev/nullRepository: wecode-ai/Wegent
Length of output: 3500
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== relevant files =="
git ls-files | rg '^(backend/app/schemas/runtime_work\.py|backend/app/services/delivery/service\.py|backend/app/api/deliveries\.py|backend/app/models/.+\.(py)$|wework/src/api/deliveries\.ts|executor/|shared/)' | head -200
echo
echo "== runtime_work DeliveryUpdate around delivery_id =="
sed -n '700,745p' backend/app/schemas/runtime_work.py
echo
echo "== delivery service model/schema references =="
sed -n '1,130p' backend/app/services/delivery/service.py
sed -n '220,250p' backend/app/services/delivery/service.py
sed -n '290,310p' backend/app/services/delivery/service.py
echo
echo "== delivery model and columns =="
rg -n "class Delivery|class LoopItem|id =|public_id |delivery" backend/app -t py | rg -n "Delivery|LoopItem|delivery|id =|String\(" . | head -200 || true
echo
echo "== precise schema/model definitions =="
python3 - <<'PY'
from pathlib import Path
for p in Path('backend/app').rglob('*.py'):
s = p.read_text(errors='ignore')
if 'class Delivery' in s or 'class LoopItem' in s or 'delivery_id: Optional[str] = Field' in s:
print(f'--- {p} ---')
lines=s.splitlines()
for i,l in enumerate(lines,1):
if 'class Delivery' in l or 'class LoopItem' in l or 'delivery_id: Optional[str] = Field' in l or 'delivery_id = ' in l or 'public_id = ' in l:
print(f'{i}: {l}')
PY
echo
echo "== runtime_work API/fields around DeliveryUpdate and deliveries endpoints =="
rg -n "DeliveryUpdate|deliveryId|current_delivery_id|create.*delivery|deliveries" backend/app/api -t py -nRepository: wecode-ai/Wegent
Length of output: 39726
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
import re
from pathlib import Path
for p in [Path("backend/app/models/delivery.py"), Path("backend/app/models/task.py")]:
print(f"--- {p} ---")
text = p.read_text(errors='ignore')
for i, line in enumerate(text.splitlines(), 1):
if any(x in line for x in ["class Delivery", "class Task", ".id = Column", "Column(String", "current_delivery_id", "id = str ", "uuid.uuid4", "uuid4()"]):
print(f"{i}: {line}")
print("\n--- RuntimeTaskCreateRequest delivery_id occurrences ---")
text = Path("backend/app/schemas/runtime_work.py").read_text()
lines = text.splitlines()
for i, line in enumerate(lines, 1):
if "delivery_id" in line or "deliveryId" in line:
print(f"{i}: {line}")
print("\n--- Executor RuntimeTaskCreateRequest references ---")
for p in Path("executor").rglob("*.rs"):
s = p.read_text(errors='ignore')
if "RuntimeTaskCreateRequest" in s or "deliveryId" in s or "runtime_work" in str(p):
print(f"--- {p} ---")
for i, line in enumerate(s.splitlines(), 1):
if "RuntimeTaskCreateRequest" in line or "deliveryId" in line or "delivery_id" in line:
print(f"{i}: {line}")
print("\n--- RuntimeTaskCreateRequest struct definition context ---")
for p in Path("executor").rglob("*.rs"):
s = p.read_text(errors='ignore')
for i, line in enumerate(s.splitlines(), 1):
if "struct RuntimeTaskCreateRequest" in line:
print(f"--- {p}:{i} ---")
start=max(1,i-25); end=min(len(s.splitlines()), i+60)
for n,l in enumerate(s.splitlines()[start-1:end], start):
print(f"{n}: {l}")
PYRepository: wecode-ai/Wegent
Length of output: 4481
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== RuntimeTaskCreateRequest definition =="
sed -n '640,730p' backend/app/schemas/runtime_work.py
echo
echo "== RuntimeWork schema aliases/struct =="
python3 - <<'PY'
from pathlib import Path
s = Path('backend/app/schemas/runtime_work.py').read_text(errors='ignore')
for name in ['RuntimeWork', 'RuntimeTaskCreateResponse', 'RuntimeTaskCreateRequest']:
idx = s.find(f'class {name}')
print(f'--- {name} at index {idx} ---')
if idx >= 0:
start=max(0, idx-20); end=len(s.splitlines())
lines=s.splitlines()
print(f'ends around line {lines[end-1].count(chr(10))+1}')
for i,l in enumerate(lines[max(0, idx-20):idx+200], idx-20):
if i<0: continue
if l.strip().startswith('class '):
break if i>idx else None
if i <= idx+180:
print(f'{i+1}: {l}')
PY
echo
echo "== backend Delivery model definition =="
sed -n '168,185p' backend/app/models/delivery.py
echo
echo "== DeliveryService finalize context =="
sed -n '220,255p' backend/app/services/delivery/service.pyRepository: wecode-ai/Wegent
Length of output: 3556
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== backend Delivery model and LoopNode =="
sed -n '1,45p' backend/app/models/delivery.py
sed -n '168,182p' backend/app/models/delivery.py
echo
echo "== DeliveryService create_delivery and finalize =="
sed -n '80,98p' backend/app/services/delivery/service.py
sed -n '220,248p' backend/app/services/delivery/service.py
echo
echo "== DeliveryResponse output =="
sed -n '162,180p' backend/app/schemas/delivery.py
echo
echo "== all backend delivery/public_id/id length references =="
rg -n "deliveryId|delivery_id|current_delivery_id|currentDeliveryId|public_id|String\\(64\\)|String\\(36\\)|uuid\\.uuid4\\(" backend/app services executor -t py -t rs | head -200 || true
echo
echo "== deterministic uuid length in Python stdlib =="
python3 - <<'PY'
import uuid
ids = [str(uuid.uuid4()) for _ in range(10)]
print("all 36:", all(len(id) == 36 for id in ids))
print(ids[:5])
PYRepository: wecode-ai/Wegent
Length of output: 4636
Use the actual production delivery ID in RuntimeTaskCreateRequest.
RuntimeTaskCreateRequest.delivery_id currently accepts only a 36-character public_id, but the created delivery passes that UUID as delivery.id (loop_items.id, String(64)) and finalize() returns "deliveryId": delivery.id. The runtime worker is serializing from RuntimeTaskCreateRequest, so runtime delivery IDs will be rejected unless they match the field the client is allowed to send.
🤖 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/runtime_work.py` around lines 720 - 722, Update
RuntimeTaskCreateRequest.delivery_id to validate the production delivery.id
value rather than requiring a 36-character UUID, matching the delivery model’s
String(64) constraint and the finalize() payload. Preserve the deliveryId alias
and optional default while allowing the runtime worker to serialize actual
delivery IDs.
| def move( | ||
| self, | ||
| db: Session, | ||
| file_id: int, | ||
| user_id: int, | ||
| path: str, | ||
| version: int, | ||
| ) -> CloudProjectFile: | ||
| file = self.get(db, file_id, user_id) | ||
| access = require_cloud_project_role( | ||
| db, file.cloud_project_id, user_id, BaseRole.Developer | ||
| ) | ||
| if file.version != version: | ||
| raise HTTPException(status.HTTP_409_CONFLICT, "Cloud file changed") | ||
| target_path = normalize_cloud_path(path) | ||
| if target_path == file.path: | ||
| return file | ||
| descendants = ( | ||
| db.query(CloudProjectFile) | ||
| .filter( | ||
| CloudProjectFile.cloud_project_id == file.cloud_project_id, | ||
| CloudProjectFile.path.like(f"{file.path}/%"), | ||
| ) | ||
| .all() | ||
| if file.kind == "folder" | ||
| else [] | ||
| ) | ||
| moving = [file, *descendants] | ||
| target_paths = { | ||
| entry.id: target_path + entry.path[len(file.path) :] for entry in moving | ||
| } | ||
| moving_ids = [entry.id for entry in moving] | ||
| conflict = ( | ||
| db.query(CloudProjectFile.id) | ||
| .filter( | ||
| CloudProjectFile.cloud_project_id == file.cloud_project_id, | ||
| CloudProjectFile.id.notin_(moving_ids), | ||
| CloudProjectFile.path.in_(list(target_paths.values())), | ||
| ) | ||
| .first() | ||
| ) | ||
| if conflict: | ||
| raise HTTPException(status.HTTP_409_CONFLICT, "Cloud path already exists") | ||
|
|
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
move() doesn't reject moving a folder into its own descendant.
There's no check that target_path is not equal to, or nested under, file.path when file.kind == "folder". Moving a folder into one of its own subfolders would still pass the conflict check (the descendant is excluded from conflict detection as one of the moving rows) and produce a corrupted, self-referential path hierarchy after the rewrite in lines 238-242.
🛡️ Proposed fix: reject nesting a folder into its own subtree
target_path = normalize_cloud_path(path)
if target_path == file.path:
return file
+ if file.kind == "folder" and target_path.startswith(f"{file.path}/"):
+ raise HTTPException(
+ status.HTTP_409_CONFLICT, "Cannot move a folder into its own subtree"
+ )
descendants = (📝 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.
| def move( | |
| self, | |
| db: Session, | |
| file_id: int, | |
| user_id: int, | |
| path: str, | |
| version: int, | |
| ) -> CloudProjectFile: | |
| file = self.get(db, file_id, user_id) | |
| access = require_cloud_project_role( | |
| db, file.cloud_project_id, user_id, BaseRole.Developer | |
| ) | |
| if file.version != version: | |
| raise HTTPException(status.HTTP_409_CONFLICT, "Cloud file changed") | |
| target_path = normalize_cloud_path(path) | |
| if target_path == file.path: | |
| return file | |
| descendants = ( | |
| db.query(CloudProjectFile) | |
| .filter( | |
| CloudProjectFile.cloud_project_id == file.cloud_project_id, | |
| CloudProjectFile.path.like(f"{file.path}/%"), | |
| ) | |
| .all() | |
| if file.kind == "folder" | |
| else [] | |
| ) | |
| moving = [file, *descendants] | |
| target_paths = { | |
| entry.id: target_path + entry.path[len(file.path) :] for entry in moving | |
| } | |
| moving_ids = [entry.id for entry in moving] | |
| conflict = ( | |
| db.query(CloudProjectFile.id) | |
| .filter( | |
| CloudProjectFile.cloud_project_id == file.cloud_project_id, | |
| CloudProjectFile.id.notin_(moving_ids), | |
| CloudProjectFile.path.in_(list(target_paths.values())), | |
| ) | |
| .first() | |
| ) | |
| if conflict: | |
| raise HTTPException(status.HTTP_409_CONFLICT, "Cloud path already exists") | |
| def move( | |
| self, | |
| db: Session, | |
| file_id: int, | |
| user_id: int, | |
| path: str, | |
| version: int, | |
| ) -> CloudProjectFile: | |
| file = self.get(db, file_id, user_id) | |
| access = require_cloud_project_role( | |
| db, file.cloud_project_id, user_id, BaseRole.Developer | |
| ) | |
| if file.version != version: | |
| raise HTTPException(status.HTTP_409_CONFLICT, "Cloud file changed") | |
| target_path = normalize_cloud_path(path) | |
| if target_path == file.path: | |
| return file | |
| if file.kind == "folder" and target_path.startswith(f"{file.path}/"): | |
| raise HTTPException( | |
| status.HTTP_409_CONFLICT, "Cannot move a folder into its own subtree" | |
| ) | |
| descendants = ( | |
| db.query(CloudProjectFile) | |
| .filter( | |
| CloudProjectFile.cloud_project_id == file.cloud_project_id, | |
| CloudProjectFile.path.like(f"{file.path}/%"), | |
| ) | |
| .all() | |
| if file.kind == "folder" | |
| else [] | |
| ) | |
| moving = [file, *descendants] | |
| target_paths = { | |
| entry.id: target_path + entry.path[len(file.path) :] for entry in moving | |
| } | |
| moving_ids = [entry.id for entry in moving] | |
| conflict = ( | |
| db.query(CloudProjectFile.id) | |
| .filter( | |
| CloudProjectFile.cloud_project_id == file.cloud_project_id, | |
| CloudProjectFile.id.notin_(moving_ids), | |
| CloudProjectFile.path.in_(list(target_paths.values())), | |
| ) | |
| .first() | |
| ) | |
| if conflict: | |
| raise HTTPException(status.HTTP_409_CONFLICT, "Cloud path already exists") |
🤖 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_files/service.py` around lines 180 - 223, Update
move() to reject folder moves when target_path equals file.path or is nested
beneath it, before descendant path rewriting and conflict detection. Raise the
existing conflict HTTPException for these invalid self-subtree moves, while
preserving the current behavior for files and destinations outside the folder’s
subtree.
| def delete( | ||
| self, db: Session, file_id: int, user_id: int, recursive: bool = False | ||
| ) -> None: | ||
| file = self.get(db, file_id, user_id) | ||
| require_cloud_project_role( | ||
| db, file.cloud_project_id, user_id, BaseRole.Developer | ||
| ) | ||
| if file.kind == "folder": | ||
| children = ( | ||
| db.query(CloudProjectFile) | ||
| .filter( | ||
| CloudProjectFile.cloud_project_id == file.cloud_project_id, | ||
| CloudProjectFile.path.like(f"{file.path}/%"), | ||
| ) | ||
| .all() | ||
| ) | ||
| if children and not recursive: | ||
| raise HTTPException(status.HTTP_409_CONFLICT, "Folder is not empty") | ||
| else: | ||
| children = [] | ||
| object_keys = [ | ||
| entry.object_key for entry in [file, *children] if entry.object_key | ||
| ] | ||
| if object_keys: | ||
| self.storage.remove_objects(object_keys) | ||
| for child in children: | ||
| db.delete(child) | ||
| db.delete(file) | ||
| db.commit() |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
delete() destroys storage objects before the DB commit succeeds; failure leaves dangling references.
self.storage.remove_objects(object_keys) (line 283) runs before db.commit() (line 287). If the commit fails for any reason, the transaction rolls back and the DB rows survive, but the storage objects are already permanently gone — the surviving rows now point at deleted objects. move() in this same file gets the ordering right (removes obsolete source objects only after a successful commit, lines 243-255); delete() should follow the same pattern.
🛡️ Proposed fix: commit before removing storage objects
for child in children:
db.delete(child)
db.delete(file)
- db.commit()
+ try:
+ db.commit()
+ except Exception:
+ db.rollback()
+ raise
+ if object_keys:
+ self.storage.remove_objects(object_keys)📝 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.
| def delete( | |
| self, db: Session, file_id: int, user_id: int, recursive: bool = False | |
| ) -> None: | |
| file = self.get(db, file_id, user_id) | |
| require_cloud_project_role( | |
| db, file.cloud_project_id, user_id, BaseRole.Developer | |
| ) | |
| if file.kind == "folder": | |
| children = ( | |
| db.query(CloudProjectFile) | |
| .filter( | |
| CloudProjectFile.cloud_project_id == file.cloud_project_id, | |
| CloudProjectFile.path.like(f"{file.path}/%"), | |
| ) | |
| .all() | |
| ) | |
| if children and not recursive: | |
| raise HTTPException(status.HTTP_409_CONFLICT, "Folder is not empty") | |
| else: | |
| children = [] | |
| object_keys = [ | |
| entry.object_key for entry in [file, *children] if entry.object_key | |
| ] | |
| if object_keys: | |
| self.storage.remove_objects(object_keys) | |
| for child in children: | |
| db.delete(child) | |
| db.delete(file) | |
| db.commit() | |
| def delete( | |
| self, db: Session, file_id: int, user_id: int, recursive: bool = False | |
| ) -> None: | |
| file = self.get(db, file_id, user_id) | |
| require_cloud_project_role( | |
| db, file.cloud_project_id, user_id, BaseRole.Developer | |
| ) | |
| if file.kind == "folder": | |
| children = ( | |
| db.query(CloudProjectFile) | |
| .filter( | |
| CloudProjectFile.cloud_project_id == file.cloud_project_id, | |
| CloudProjectFile.path.like(f"{file.path}/%"), | |
| ) | |
| .all() | |
| ) | |
| if children and not recursive: | |
| raise HTTPException(status.HTTP_409_CONFLICT, "Folder is not empty") | |
| else: | |
| children = [] | |
| object_keys = [ | |
| entry.object_key for entry in [file, *children] if entry.object_key | |
| ] | |
| for child in children: | |
| db.delete(child) | |
| db.delete(file) | |
| try: | |
| db.commit() | |
| except Exception: | |
| db.rollback() | |
| raise | |
| if object_keys: | |
| self.storage.remove_objects(object_keys) |
🤖 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_files/service.py` around lines 259 - 287, Update
the delete method to commit the database deletions before calling
self.storage.remove_objects(object_keys), matching the successful ordering used
by move. Preserve the existing object-key collection and recursive child
deletion behavior, and only remove storage objects after db.commit succeeds.
| # Function to show help | ||
| show_help() { | ||
| echo "Build docker images for Wegent components" | ||
| echo "Build docker images for Wegent components"aass |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Stray aass corrupts the help title output.
The unquoted trailing aass is concatenated onto the echoed string, so --help prints Build docker images for Wegent componentsaass. Appears to be an accidental edit.
🐛 Proposed fix
- echo "Build docker images for Wegent components"aass
+ echo "Build docker images for Wegent components"📝 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.
| echo "Build docker images for Wegent components"aass | |
| echo "Build docker images for Wegent components" |
🤖 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 `@build_image.sh` at line 18, Remove the stray “aass” suffix from the
help-title echo command so the build_image.sh help output displays exactly
“Build docker images for Wegent components”.
| return ( | ||
| <div className="p-7"> | ||
| <div className="flex items-start"> | ||
| <div> | ||
| <h2 className="heading-md">共享文件</h2> | ||
| <p className="mt-1 text-xs text-text-muted"> | ||
| 成员和 AI 可通过权限控制的云空间访问这些内容。 | ||
| </p> | ||
| </div> | ||
| <span className="flex-1" /> | ||
| <input | ||
| ref={inputRef} | ||
| type="file" | ||
| multiple | ||
| className="hidden" | ||
| onChange={event => { | ||
| const selected = [...(event.target.files ?? [])] | ||
| void uploadFiles(selected) | ||
| }} | ||
| /> | ||
| <button | ||
| type="button" | ||
| data-testid="cloud-folder-add" | ||
| onClick={() => setCreatingFolder(true)} | ||
| className="mr-2 flex h-8 items-center gap-1.5 rounded-md px-3 text-sm text-text-secondary hover:bg-hover" | ||
| > | ||
| <FolderPlus className="h-3.5 w-3.5" /> 新建文件夹 | ||
| </button> | ||
| <button | ||
| type="button" | ||
| data-testid="cloud-files-upload" | ||
| onClick={() => inputRef.current?.click()} | ||
| className="flex h-8 items-center gap-1.5 rounded-md bg-text-primary px-3 text-sm font-medium text-background" | ||
| > | ||
| <Upload className="h-3.5 w-3.5" /> | ||
| {uploadingCount > 0 ? `正在上传 ${uploadingCount} 项…` : '上传文件'} | ||
| </button> | ||
| </div> | ||
| {creatingFolder && ( | ||
| <div className="mt-4 flex items-center gap-2"> | ||
| <input | ||
| autoFocus | ||
| data-testid="cloud-folder-name" | ||
| value={folderName} | ||
| onChange={event => setFolderName(event.target.value)} | ||
| onKeyDown={event => event.key === 'Enter' && void createFolder()} | ||
| placeholder="文件夹路径，例如 docs/design" | ||
| className="h-8 min-w-0 flex-1 rounded-md border border-border px-3 text-sm outline-none focus:border-focus" | ||
| /> | ||
| <button | ||
| type="button" | ||
| data-testid="cloud-folder-create-confirm" | ||
| onClick={() => void createFolder()} | ||
| className="h-8 rounded-md bg-text-primary px-3 text-sm text-background" | ||
| > | ||
| 创建 | ||
| </button> | ||
| <button | ||
| type="button" | ||
| onClick={() => setCreatingFolder(false)} | ||
| className="h-8 rounded-md px-3 text-sm hover:bg-hover" | ||
| > | ||
| 取消 | ||
| </button> | ||
| </div> | ||
| )} | ||
| {error && ( | ||
| <p className="mt-3 text-xs text-destructive" role="alert"> | ||
| {error} | ||
| </p> | ||
| )} | ||
| <div className="mt-6 overflow-hidden rounded-md border border-border"> | ||
| <div className="grid h-9 grid-cols-[minmax(0,1fr)_120px_120px_80px_96px] items-center border-b border-border bg-muted/30 px-4 text-xs text-text-muted"> | ||
| <span>名称</span> | ||
| <span>类型</span> | ||
| <span>更新时间</span> | ||
| <span>大小</span> | ||
| <span /> | ||
| </div> | ||
| {files.length === 0 ? ( | ||
| <div className="flex h-40 items-center justify-center text-sm text-text-muted"> | ||
| 暂无共享文件 | ||
| </div> | ||
| ) : ( | ||
| files.map(entry => ( | ||
| <div | ||
| key={entry.id} | ||
| className="grid h-11 grid-cols-[minmax(0,1fr)_120px_120px_80px_96px] items-center border-b border-border px-4 text-xs last:border-b-0 hover:bg-hover" | ||
| > | ||
| <span className="flex min-w-0 items-center gap-2"> | ||
| {entry.kind === 'folder' ? ( | ||
| <Folder className="h-4 w-4 text-text-muted" /> | ||
| ) : ( | ||
| <File className="h-4 w-4 text-text-muted" /> | ||
| )} | ||
| {editingFileId === entry.id ? ( | ||
| <input | ||
| autoFocus | ||
| data-testid={`cloud-file-path-${entry.id}`} | ||
| value={editingPath} | ||
| onChange={event => setEditingPath(event.target.value)} | ||
| onKeyDown={event => { | ||
| if (event.key === 'Enter') void moveFile(entry) | ||
| if (event.key === 'Escape') setEditingFileId(null) | ||
| }} | ||
| className="h-7 min-w-0 flex-1 rounded border border-focus bg-background px-2 outline-none" | ||
| /> | ||
| ) : ( | ||
| <span className="truncate text-text-primary">{entry.path}</span> | ||
| )} | ||
| </span> | ||
| <span className="text-text-muted">{entry.content_type || '文件夹'}</span> | ||
| <span className="text-text-muted">{entry.updated_at.slice(0, 10)}</span> | ||
| <span className="text-text-muted"> | ||
| {entry.kind === 'file' ? `${entry.size_bytes} B` : '—'} | ||
| </span> | ||
| <span className="flex justify-end gap-1"> | ||
| <button | ||
| type="button" | ||
| data-testid={`cloud-file-rename-${entry.id}`} | ||
| onClick={() => { | ||
| setEditingFileId(entry.id) | ||
| setEditingPath(entry.path) | ||
| }} | ||
| className="flex h-7 w-7 items-center justify-center rounded-md text-text-muted hover:bg-muted" | ||
| aria-label={`重命名或移动 ${entry.path}`} | ||
| > | ||
| <Pencil className="h-3.5 w-3.5" /> | ||
| </button> | ||
| {entry.kind === 'file' && ( | ||
| <button | ||
| type="button" | ||
| data-testid={`cloud-file-open-${entry.id}`} | ||
| onClick={() => void openFile(entry)} | ||
| className="flex h-7 w-7 items-center justify-center rounded-md hover:bg-muted" | ||
| aria-label={`打开 ${entry.path}`} | ||
| > | ||
| <Download className="h-3.5 w-3.5" /> | ||
| </button> | ||
| )} | ||
| <button | ||
| type="button" | ||
| data-testid={`cloud-file-delete-${entry.id}`} | ||
| onClick={() => void deleteFile(entry)} | ||
| className="flex h-7 w-7 items-center justify-center rounded-md text-text-muted hover:bg-muted hover:text-destructive" | ||
| aria-label={`删除 ${entry.path}`} | ||
| > | ||
| <Trash2 className="h-3.5 w-3.5" /> | ||
| </button> | ||
| </span> | ||
| </div> | ||
| )) | ||
| )} | ||
| </div> | ||
| <section className="mt-8"> | ||
| <div className="flex items-baseline gap-2"> | ||
| <h3 className="text-sm font-medium text-text-primary">交付快照</h3> | ||
| <span className="text-xs text-text-muted">来自已完成任务，只读且不可修改</span> | ||
| </div> | ||
| <div className="mt-3 overflow-hidden rounded-md border border-border"> | ||
| <div className="grid h-9 grid-cols-[240px_minmax(0,1fr)_120px_120px_80px_40px] items-center border-b border-border bg-muted/30 px-4 text-xs text-text-muted"> | ||
| <span>任务</span> | ||
| <span>名称</span> | ||
| <span>类型</span> | ||
| <span>交付时间</span> | ||
| <span>大小</span> | ||
| <span /> | ||
| </div> | ||
| {deliveryFiles.length === 0 ? ( | ||
| <div className="flex h-24 items-center justify-center text-sm text-text-muted"> | ||
| 暂无交付文件 | ||
| </div> | ||
| ) : ( | ||
| deliveryFiles.map(entry => ( | ||
| <div | ||
| key={entry.asset_id} | ||
| data-testid={`delivery-file-${entry.asset_id}`} | ||
| className="grid min-h-11 grid-cols-[240px_minmax(0,1fr)_120px_120px_80px_40px] items-center border-b border-border px-4 text-xs last:border-b-0 hover:bg-hover" | ||
| > | ||
| <span | ||
| className="flex min-w-0 items-center gap-2" | ||
| title={`${entry.loop_item_id} · ${entry.loop_item_title}`} | ||
| > | ||
| <span className="shrink-0 font-mono text-text-muted">{entry.loop_item_id}</span> | ||
| <span className="truncate text-text-primary">{entry.loop_item_title}</span> | ||
| </span> | ||
| <span className="flex min-w-0 items-center gap-2 text-text-primary"> | ||
| <File className="h-4 w-4 shrink-0 text-text-muted" /> | ||
| <span className="truncate" title={entry.relative_path}> | ||
| {entry.relative_path} | ||
| </span> | ||
| </span> | ||
| <span className="truncate text-text-muted">{entry.content_type || '文件'}</span> | ||
| <span className="text-text-muted">{entry.delivered_at.slice(0, 10)}</span> | ||
| <span className="text-text-muted">{entry.size_bytes} B</span> | ||
| <button | ||
| type="button" | ||
| data-testid={`delivery-file-open-${entry.asset_id}`} | ||
| onClick={() => void openDeliveryFile(entry)} | ||
| className="flex h-7 w-7 items-center justify-center rounded-md hover:bg-muted" | ||
| aria-label={`打开交付文件 ${entry.relative_path}`} | ||
| > | ||
| <Download className="h-3.5 w-3.5" /> | ||
| </button> | ||
| </div> | ||
| )) | ||
| )} | ||
| </div> | ||
| </section> | ||
| <p className="mt-6 text-xs text-text-muted"> | ||
| 在 Wework 输入框中输入 @，即可让 AI 查看云项目、目录、任务或交付。 | ||
| </p> | ||
| </div> | ||
| ) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Entire component is missing i18n; all strings are hardcoded Chinese literals.
Unlike DeliveryDialog.tsx (added in the same cohort) which consistently uses useTranslation/t(...), every label here (共享文件, 新建文件夹, 上传文件, 名称, 类型, etc.) is a raw string. English-locale users will see untranslated Chinese text throughout this entire new screen.
As per coding guidelines, wework/**/*.{tsx,ts}: "Use the local @/hooks/useTranslation wrapper for new Wework code," and wework/src/i18n/locales/{en,zh-CN}/**/*: "Add new copy to the appropriate Wework namespace in both the English and Simplified Chinese locale directories."
🤖 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/CloudFilesView.tsx` around lines 108 - 321, The
CloudFilesView component uses hardcoded Chinese UI strings instead of the Wework
translation system. Add the required keys to the appropriate English and zh-CN
locale namespaces, use the local useTranslation hook and t(...) throughout
CloudFilesView for headings, labels, buttons, placeholders, empty states, status
text, and aria-labels, and preserve the existing dynamic values and behavior.
Source: Coding guidelines
| useEffect(() => { | ||
| void Promise.all([ | ||
| api.listDeliveries(item.id), | ||
| api.listTaskBindings(item.id), | ||
| api.listLoopItemAttachments(item.id), | ||
| api.listLoopItemCollaborators(item.id), | ||
| api.listCloudProjectMembers(item.cloud_project_id), | ||
| ]).then( | ||
| ([ | ||
| deliveryResponse, | ||
| taskResponse, | ||
| attachmentResponse, | ||
| collaboratorResponse, | ||
| memberResponse, | ||
| ]) => { | ||
| setDeliveries(deliveryResponse.items) | ||
| setTasks(taskResponse) | ||
| setAttachments(attachmentResponse) | ||
| setCollaborators(collaboratorResponse) | ||
| setProjectMembers(memberResponse) | ||
| } | ||
| ) | ||
| }, [api, item.cloud_project_id, item.id]) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Promise.all has no error handling.
This load has no .catch, so a failure in any of the five delivery API calls becomes an unhandled promise rejection and the detail panel silently stays empty. There's also no unmount guard, so setState may run after TodoDetail unmounts. Add a .catch (surface an error state) and an active flag as done elsewhere in this file.
🤖 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 664 - 686, The
data-loading useEffect in CloudTodoWorkspace should add an active/unmount guard
and catch failures from the Promise.all request batch. Reuse the error-state
pattern already present elsewhere in the file, only update state while the
effect remains active, and deactivate the flag during cleanup before applying
delivery, task, attachment, collaborator, or project-member results.
| async function startTask(project: ProjectWithTasks, item: CloudLoopItem, message: string) { | ||
| if (!onRunTodo) return | ||
| const address = await onRunTodo({ | ||
| project, | ||
| message, | ||
| goal: item.title, | ||
| attachments: [] as Attachment[], | ||
| collaborationMode: 'default', | ||
| cloudProjectId: item.cloud_project_id, | ||
| }) | ||
| if (!address) return | ||
| await api.bindTask(item.id, address, item.title) | ||
| if (item.status === 'completed') { | ||
| const reopened = await api.updateLoopItem(item.id, { | ||
| version: item.version, | ||
| status: 'in_progress', | ||
| }) | ||
| setItems(current => current.map(entry => (entry.id === reopened.id ? reopened : entry))) | ||
| } | ||
| setStartItem(null) | ||
| setSelectedItem(null) | ||
| await onOpenRuntimeTask?.(address) | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
startTask lacks error handling.
onRunTodo, bindTask, and updateLoopItem are awaited without a try/catch. In StartTaskDialog the call site is void onStart(...).finally(...) with no .catch, so a failure produces an unhandled rejection and the user gets no feedback (the dialog just resets starting). Wrap the flow and propagate a visible error.
🤖 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 1289 - 1311,
Update startTask to wrap the onRunTodo, api.bindTask, and conditional
api.updateLoopItem flow in try/catch, report the failure through the existing
visible error mechanism, and preserve dialog cleanup while preventing unhandled
rejections from the StartTaskDialog call site.
| <DragOverlay dropAnimation={null}> | ||
| {activeDragItemId ? ( | ||
| <div className="w-[210px] rotate-1 rounded-md border border-border bg-background p-3 text-left shadow-lg"> | ||
| <TodoCardContent item={items.find(item => item.id === activeDragItemId)!} /> | ||
| </div> | ||
| ) : null} | ||
| </DragOverlay> |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Non-null assertion on the drag overlay item can crash.
items.find(item => item.id === activeDragItemId)! assumes the dragged item is always present, but the 15s background refreshItems interval can replace items mid-drag; if the item disappears, TodoCardContent item={undefined} will throw on item.id. Guard against undefined before rendering.
🛡️ Proposed guard
- {activeDragItemId ? (
+ {(() => {
+ const dragItem = items.find(item => item.id === activeDragItemId)
+ return dragItem ? (
<div className="w-[210px] rotate-1 rounded-md border border-border bg-background p-3 text-left shadow-lg">
- <TodoCardContent item={items.find(item => item.id === activeDragItemId)!} />
+ <TodoCardContent item={dragItem} />
</div>
- ) : null}
+ ) : null
+ })()}📝 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.
| <DragOverlay dropAnimation={null}> | |
| {activeDragItemId ? ( | |
| <div className="w-[210px] rotate-1 rounded-md border border-border bg-background p-3 text-left shadow-lg"> | |
| <TodoCardContent item={items.find(item => item.id === activeDragItemId)!} /> | |
| </div> | |
| ) : null} | |
| </DragOverlay> | |
| <DragOverlay dropAnimation={null}> | |
| {(() => { | |
| const dragItem = items.find(item => item.id === activeDragItemId) | |
| return dragItem ? ( | |
| <div className="w-[210px] rotate-1 rounded-md border border-border bg-background p-3 text-left shadow-lg"> | |
| <TodoCardContent item={dragItem} /> | |
| </div> | |
| ) : null | |
| })()} | |
| </DragOverlay> |
🤖 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 1846 - 1852,
Update the DragOverlay rendering in CloudTodoWorkspace to store or derive the
result of items.find for activeDragItemId and render TodoCardContent only when
that item exists. Remove the non-null assertion, while preserving the existing
overlay styling and null rendering when no active drag item is available.
| return value?.version === 1 | ||
| ? { | ||
| ...value, | ||
| workTypes: value.workTypes.map(workType => ({ | ||
| ...workType, | ||
| dependsOn: workType.dependsOn ?? [], | ||
| defaultAssignee: workType.defaultAssignee ?? { type: 'unassigned' }, | ||
| })), | ||
| } | ||
| : DEFAULT_TODO_WORKFLOW |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Guard workTypes before mapping.
If a persisted config has version === 1 but a missing/null workTypes array, value.workTypes.map(...) throws and is swallowed by the catch, discarding an otherwise-valid config and falling back to DEFAULT_TODO_WORKFLOW. Default workTypes to [] before mapping.
🛡️ Proposed guard
- workTypes: value.workTypes.map(workType => ({
+ workTypes: (value.workTypes ?? []).map(workType => ({📝 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.
| return value?.version === 1 | |
| ? { | |
| ...value, | |
| workTypes: value.workTypes.map(workType => ({ | |
| ...workType, | |
| dependsOn: workType.dependsOn ?? [], | |
| defaultAssignee: workType.defaultAssignee ?? { type: 'unassigned' }, | |
| })), | |
| } | |
| : DEFAULT_TODO_WORKFLOW | |
| return value?.version === 1 | |
| ? { | |
| ...value, | |
| workTypes: (value.workTypes ?? []).map(workType => ({ | |
| ...workType, | |
| dependsOn: workType.dependsOn ?? [], | |
| defaultAssignee: workType.defaultAssignee ?? { type: 'unassigned' }, | |
| })), | |
| } | |
| : DEFAULT_TODO_WORKFLOW |
🤖 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/todoModel.ts` around lines 320 - 329, Update the
version-1 config normalization around value.workTypes to default a missing or
null workTypes value to an empty array before calling map. Preserve the existing
workType field defaults and return the normalized config instead of allowing the
mapping error to trigger the fallback to DEFAULT_TODO_WORKFLOW.
…1989-todo-workspace
# Conflicts: # wework/src/components/layout/DesktopWorkbenchMain.tsx
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 (4)
wework/src/components/layout/DesktopWorkbenchMain.tsx (4)
447-448: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy liftDo not reuse the previous task’s cloud context during hydration.
boundCloudProjectandboundCloudItemremain populated while the next task is being hydrated, so the composer can render or submit the previous task’s cloud context. The no-deliveryApibranch also clears only the item, allowing the stale project to persist. Associate bound state with the runtime task address or gate it synchronously, and clear both project and item in the local branch.Also applies to: 537-548
🤖 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/layout/DesktopWorkbenchMain.tsx` around lines 447 - 448, Update the composer cloud-context selection near composerCloudProject and composerTodoItem so boundCloudProject and boundCloudItem are used only when they belong to the current runtime task, preventing stale context during hydration; otherwise use the pending values. In the no-deliveryApi local branch, clear both the bound project and bound item together.
555-564: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winValidate the captured runtime-task target before binding.
onRuntimeTaskCreatedstorespendingTodoBinding.target, but this effect ignores it and binds whenever anycurrentRuntimeTaskexists. A quick task or pane switch can therefore bind the selected cloud item to the wrong runtime task. Require the pending target to match the current task before issuingbindTaskorbindProjectTask.🤖 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/layout/DesktopWorkbenchMain.tsx` around lines 555 - 564, The binding effect in DesktopWorkbenchMain must validate pendingTodoBinding.target against currentRuntimeTask before calling bindTask or bindProjectTask. Add an early return when the captured target does not match the current runtime task, while preserving the existing pendingTodoItem and pendingCloudProject binding paths for matching targets.
467-477: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy liftDo not use
applicationcontext for cloud project/task values.
RuntimeAdditionalContextKinddistinguishesapplicationfromuntrusted, and this runtime forwards onlyapplicationentries as<application_context>. Since the project name, task title, description, and reference are cloud-authored, moving the fixed tooling instructions into the application entry while keeping cloud values in entries withkind: 'untrusted'prevents cloud content from bypassing untrusted filtering.🤖 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/layout/DesktopWorkbenchMain.tsx` around lines 467 - 477, Update the context construction in the composer cloud context callback to keep cloud project/task values from composerCloudProject and composerTodoItem in entries with kind 'untrusted'. Keep only the fixed MCP tooling instructions in the kind 'application' entry, preserving the existing context text and filtering behavior.
1162-1180: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPreserve the existing model-selector open-change callback.
This wrapper spreads
projectChatand then replacesonModelSelectorOpenChangewithout forwarding to the original callback. Any existing parent state or cleanup attached to that callback will stop running.Suggested fix
onModelSelectorOpenChange: open => { + projectChat.onModelSelectorOpenChange?.(open) if (!open) pendingModelRetryRef.current = null },🤖 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/layout/DesktopWorkbenchMain.tsx` around lines 1162 - 1180, Update projectChatWithModelSelectorSignal’s onModelSelectorOpenChange wrapper to invoke the original projectChat callback with the same open value, while retaining the existing pendingModelRetryRef cleanup when closing. Preserve the current callback behavior for consumers without an existing handler.
🤖 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/layout/DesktopWorkbenchMain.tsx`:
- Line 578: Replace the hardcoded user-facing cloud-collaboration strings in the
affected code, including the fallback in the todo-binding error handling and
labels near the mention logic, with translations from the local
`@/hooks/useTranslation` t wrapper. Add or reuse the component’s translation hook
and keep only intentionally locale-independent search aliases hardcoded.
---
Outside diff comments:
In `@wework/src/components/layout/DesktopWorkbenchMain.tsx`:
- Around line 447-448: Update the composer cloud-context selection near
composerCloudProject and composerTodoItem so boundCloudProject and
boundCloudItem are used only when they belong to the current runtime task,
preventing stale context during hydration; otherwise use the pending values. In
the no-deliveryApi local branch, clear both the bound project and bound item
together.
- Around line 555-564: The binding effect in DesktopWorkbenchMain must validate
pendingTodoBinding.target against currentRuntimeTask before calling bindTask or
bindProjectTask. Add an early return when the captured target does not match the
current runtime task, while preserving the existing pendingTodoItem and
pendingCloudProject binding paths for matching targets.
- Around line 467-477: Update the context construction in the composer cloud
context callback to keep cloud project/task values from composerCloudProject and
composerTodoItem in entries with kind 'untrusted'. Keep only the fixed MCP
tooling instructions in the kind 'application' entry, preserving the existing
context text and filtering behavior.
- Around line 1162-1180: Update projectChatWithModelSelectorSignal’s
onModelSelectorOpenChange wrapper to invoke the original projectChat callback
with the same open value, while retaining the existing pendingModelRetryRef
cleanup when closing. Preserve the current callback behavior for consumers
without an existing handler.
🪄 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: 118454bb-3a3d-4b37-bb03-fbd0b3d012f9
📒 Files selected for processing (12)
wework/src/components/chat/ChatInput.tsxwework/src/components/chat/composer/CompactChatComposer.tsxwework/src/components/chat/composer/ComposerMentionMenu.tsxwework/src/components/chat/composer/ComposerTextarea.tsxwework/src/components/chat/composer/ComposerToolbar.tsxwework/src/components/chat/composer/ProjectChatComposer.tsxwework/src/components/layout/DesktopWorkbenchMain.tsxwework/src/components/layout/EnvironmentInfoPopover.test.tsxwework/src/components/layout/EnvironmentInfoPopover.tsxwework/src/components/layout/useWorkbenchPaneSession.tswework/src/features/todo/TodoBindingPicker.tsxwework/src/features/workbench/workbenchReducer.test.ts
🚧 Files skipped from review as they are similar to previous changes (10)
- wework/src/components/chat/composer/ComposerToolbar.tsx
- wework/src/components/layout/EnvironmentInfoPopover.test.tsx
- wework/src/components/chat/composer/CompactChatComposer.tsx
- wework/src/components/chat/composer/ComposerMentionMenu.tsx
- wework/src/components/chat/ChatInput.tsx
- wework/src/components/chat/composer/ProjectChatComposer.tsx
- wework/src/components/chat/composer/ComposerTextarea.tsx
- wework/src/components/layout/EnvironmentInfoPopover.tsx
- wework/src/features/todo/TodoBindingPicker.tsx
- wework/src/components/layout/useWorkbenchPaneSession.ts
| }) | ||
| .catch(cause => { | ||
| if (!active) return | ||
| setTodoBindingError(cause instanceof Error ? cause.message : '关联项目空间失败') |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Localize the new cloud-collaboration strings.
The new fallback and mention labels hardcode Chinese text (关联项目空间失败, 云空间, 文件, 任务, 交付), so English locales will show mixed-language UI. Use the existing t wrapper for user-facing strings and keep only deliberately locale-independent search aliases hardcoded.
As per coding guidelines, new Wework code must use the local @/hooks/useTranslation wrapper.
Also applies to: 618-659
🤖 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/layout/DesktopWorkbenchMain.tsx` at line 578, Replace
the hardcoded user-facing cloud-collaboration strings in the affected code,
including the fallback in the todo-binding error handling and labels near the
mention logic, with translations from the local `@/hooks/useTranslation` t
wrapper. Add or reuse the component’s translation hook and keep only
intentionally locale-independent search aliases hardcoded.
Source: Coding guidelines
Listing repositories returned an empty list for any user whose token was bound through the UI. The ciphertext was passed to the provider API as if it were the credential, which answers 401, and two layers of `continue` turned that into `200 []` -- indistinguishable from owning no repositories. The decryption used to happen once, when the session user was loaded: get_current_user called user_service.get_user_by_name, whose last line is `return self.decrypt_user_git_info(user)`. wecode-ai#2185 replaced those three call sites with a plain query so that authentication would not depend on Git crypto configuration -- correct in itself, and likely forced by wecode-ai#2110 making GIT_TOKEN_AES_IV mandatory two days earlier, since decrypting during login would otherwise reject every session on a deployment without the IV. But the decryption was removed rather than moved, and all five providers had been relying on it. Decrypt at the provider boundary instead, in each _get_git_infos, which is the single place entries are built. The placeholder '***' and empty strings are passed through untouched: the first marks a credential a deployment overlay substitutes at call time, the second is what callers test to raise "not configured". gitee.validate_token got the same treatment; it was the only provider that did not decrypt even there. Report the domains that drop out of an aggregated result, separating a refused credential from an unreachable host. The helper lives on the base class rather than in five copies -- five copies of _get_git_infos is how one omission became five. gerrit already logged this and is unchanged. Existing provider tests could not have caught any of it: their fixtures use plaintext tokens, which pass through decrypt_token untouched. The new tests encrypt, and are parametrized over all five providers so a sixth cannot be added with the omission intact. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
2 participants
Summary by CodeRabbit