feat: 修复由于sql调整导致的代码调整 - #2200
Merged
Merged
Conversation
Contributor
|
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)
📝 WalkthroughWalkthroughThe PR adds MySQL sentinel adaptation for LoopNode persistence, normalizes sentinel API values to nullable fields, updates delivery binding queries, and improves cloud-project integrity-error logging and responses. ChangesBackend delivery normalization
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Database
participant LoopItemService
participant LoopNode
participant DeliverySchema
LoopItemService->>LoopNode: write dialect-adapted values
LoopNode->>Database: persist MySQL sentinel values
Database-->>DeliverySchema: return sentinel fields
DeliverySchema-->>LoopItemService: normalize sentinels to None
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
wework/e2e/desktop/task-flow.e2e.mjs (1)
4277-4280: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winBegin the TODO flow with a snapshot assertion.
Use
waitForSnapshotfor the initialcloud-todo-workspacecheck instead of starting withwaitFor, so this flow follows the required snapshot-first verification pattern.Proposed fix
- await control.command('waitFor', '[data-testid="cloud-todo-workspace"]', { - timeoutMs: WORKBENCH_READY_TIMEOUT_MS, - }) + await waitForSnapshot( + control, + snapshot => snapshot.testIds.includes('cloud-todo-workspace'), + 'The cloud TODO workspace did not render', + WORKBENCH_READY_TIMEOUT_MS + )As per coding guidelines, “begin verification with
snapshot”.🤖 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/e2e/desktop/task-flow.e2e.mjs` around lines 4277 - 4280, Update the initial cloud-todo-workspace verification in the TODO flow after the navigate command to use control.command('waitForSnapshot', ...) instead of waitFor, preserving the existing selector and WORKBENCH_READY_TIMEOUT_MS timeout.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/models/delivery.py`:
- Around line 189-250: Update _MYSQL_NON_NULL_DEFAULTS and
adapt_loop_node_values_for_dialect so nullable relationship fields, including
parent_id, loop_item_id, delivery_id, local_project_id, and backend_task_id,
preserve SQL NULL instead of receiving synthetic sentinels; add the required
Alembic schema migration with upgrade and rollback verification. In
backend/app/services/loop_items/service.py:290-292, retain the adaptation call
unchanged because it is corrected by the helper fix. In
backend/tests/schemas/test_delivery.py:45-56, assert nullable relationship IDs
remain None and limit sentinel checks to non-relational scalar fields such as
unset datetimes.
In `@wework/e2e/desktop/task-flow.e2e.mjs`:
- Around line 2226-2235: Update the remote-picker expectation in
verifyCloudProjectFlow to use this.remoteHome, matching the WEGENT_EXECUTOR_HOME
value configured in remoteEnv, instead of join(resultDir,
'cloud-executor-home').
---
Nitpick comments:
In `@wework/e2e/desktop/task-flow.e2e.mjs`:
- Around line 4277-4280: Update the initial cloud-todo-workspace verification in
the TODO flow after the navigate command to use
control.command('waitForSnapshot', ...) instead of waitFor, preserving the
existing selector and WORKBENCH_READY_TIMEOUT_MS timeout.
🪄 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: 3f9d7aa9-ce8c-45c3-a9ed-dfa8d01ebf08
📒 Files selected for processing (9)
backend/app/models/delivery.pybackend/app/schemas/cloud_file.pybackend/app/schemas/cloud_project.pybackend/app/schemas/delivery.pybackend/app/services/cloud_projects/service.pybackend/app/services/delivery/service.pybackend/app/services/loop_items/service.pybackend/tests/schemas/test_delivery.pywework/e2e/desktop/task-flow.e2e.mjs
Comment on lines
+189
to
+250
| _MYSQL_NON_NULL_DEFAULTS: dict[str, object] = { | ||
| "cloud_project_id": "", | ||
| "parent_id": "", | ||
| "loop_item_id": "", | ||
| "delivery_id": "", | ||
| "public_id": "", | ||
| "project_key": "", | ||
| "name": "", | ||
| "title": "", | ||
| "storage_prefix": "", | ||
| "sequence_number": 0, | ||
| "created_by_user_id": 0, | ||
| "updated_by_user_id": 0, | ||
| "assignee_user_id": 0, | ||
| "user_id": 0, | ||
| "added_by_user_id": 0, | ||
| "source": "", | ||
| "status": "", | ||
| "priority": "", | ||
| "due_at": _MYSQL_UNSET_DATETIME, | ||
| "current_delivery_id": "", | ||
| "local_project_id": 0, | ||
| "device_id": "", | ||
| "is_default": False, | ||
| "task_user_id": 0, | ||
| "task_id": "", | ||
| "task_title": "", | ||
| "backend_task_id": 0, | ||
| "linked_by_user_id": 0, | ||
| "linked_at": _MYSQL_UNSET_DATETIME, | ||
| "unlinked_at": _MYSQL_UNSET_DATETIME, | ||
| "path": "", | ||
| "kind": "", | ||
| "display_name": "", | ||
| "relative_path": "", | ||
| "object_key": "", | ||
| "content_type": "", | ||
| "size_bytes": 0, | ||
| "sha256": "", | ||
| "source_task_binding_id": "", | ||
| "source_task_snapshot": {}, | ||
| "markdown_object_key": "", | ||
| "chat_object_key": "", | ||
| "manifest_object_key": "", | ||
| "metadata_json": {}, | ||
| "completed_at": _MYSQL_UNSET_DATETIME, | ||
| "delivered_at": _MYSQL_UNSET_DATETIME, | ||
| } | ||
|
|
||
|
|
||
| def adapt_loop_node_values_for_dialect( | ||
| values: dict[str, object], dialect_name: str | ||
| ) -> dict[str, object]: | ||
| """Convert explicit nulls to sentinels required by the production schema.""" | ||
| if dialect_name != "mysql": | ||
| return values | ||
| adapted = values.copy() | ||
| for attribute, default in _MYSQL_NON_NULL_DEFAULTS.items(): | ||
| if attribute in adapted and adapted[attribute] is None: | ||
| adapted[attribute] = ( | ||
| default.copy() if isinstance(default, dict) else default | ||
| ) |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Preserve SQL NULL for relationship fields instead of synthetic sentinels. Empty strings and zero IDs are not nullable foreign keys: top-level TODO insertion and parent removal can fail FK validation.
backend/app/models/delivery.py#L189-L250: remove foreign-key/relationship fields such asparent_id,loop_item_id,delivery_id,local_project_id, andbackend_task_idfrom MySQL sentinel substitution; migrate the physical schema to allow NULL where the model declares it nullable.backend/app/services/loop_items/service.py#L290-L292: retain this adaptation call only after the helper no longer rewritesparent_id=None.backend/tests/schemas/test_delivery.py#L45-L56: assert that nullable relationship IDs remainNone; keep sentinel assertions limited to non-relational scalar fields such as unset datetimes.
As per coding guidelines, persistent backend model changes require an Alembic migration with upgrade and rollback verification.
📍 Affects 3 files
backend/app/models/delivery.py#L189-L250(this comment)backend/app/services/loop_items/service.py#L290-L292backend/tests/schemas/test_delivery.py#L45-L56
🤖 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 189 - 250, Update
_MYSQL_NON_NULL_DEFAULTS and adapt_loop_node_values_for_dialect so nullable
relationship fields, including parent_id, loop_item_id, delivery_id,
local_project_id, and backend_task_id, preserve SQL NULL instead of receiving
synthetic sentinels; add the required Alembic schema migration with upgrade and
rollback verification. In backend/app/services/loop_items/service.py:290-292,
retain the adaptation call unchanged because it is corrected by the helper fix.
In backend/tests/schemas/test_delivery.py:45-56, assert nullable relationship
IDs remain None and limit sentinel checks to non-relational scalar fields such
as unset datetimes.
Source: Coding guidelines
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/e2e/desktop/task-flow.e2e.mjs`:
- Around line 5303-5320: The firstTaskOpenedTerminal flag must match the
terminalOpened condition used by waitForSnapshot. Update its check to require
workspace-terminal-window and exclude workspace-tool-launcher, so
terminal-plus-launcher snapshots follow the limited-tools validation path.
🪄 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: 537f1ac8-0eba-4128-89d3-2d8b85a63c06
📒 Files selected for processing (1)
wework/e2e/desktop/task-flow.e2e.mjs
Contributor
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 1
🤖 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/e2e/desktop/task-flow.e2e.mjs`:
- Around line 5303-5320: The firstTaskOpenedTerminal flag must match the
terminalOpened condition used by waitForSnapshot. Update its check to require
workspace-terminal-window and exclude workspace-tool-launcher, so
terminal-plus-launcher snapshots follow the limited-tools validation path.
🪄 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: 537f1ac8-0eba-4128-89d3-2d8b85a63c06
📒 Files selected for processing (1)
wework/e2e/desktop/task-flow.e2e.mjs
🛑 Comments failed to post (1)
wework/e2e/desktop/task-flow.e2e.mjs (1)
5303-5320: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Keep the terminal-state flag consistent with the wait predicate.
Line 5318 treats any terminal element as success, while the predicate defines terminal success as a terminal without the launcher. A terminal-plus-limited-launcher snapshot takes the wrong restoration branch and skips validating the limited-tools state.
Proposed fix
- const firstTaskOpenedTerminal = firstTaskBottomWorkspaceSnapshot.testIds.includes( - 'workspace-terminal-window' - ) + const firstTaskOpenedTerminal = + firstTaskBottomWorkspaceSnapshot.testIds.includes('workspace-terminal-window') && + !firstTaskBottomWorkspaceSnapshot.testIds.includes('workspace-tool-launcher')📝 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.const firstTaskBottomWorkspaceSnapshot = await waitForSnapshot( control, value => { const terminalOpened = value.testIds.includes('workspace-terminal-window') && !value.testIds.includes('workspace-tool-launcher') const localTerminalUnavailable = value.testIds.includes('workspace-tool-launcher') && value.testIds.includes('workspace-local-device-limited-tools') return terminalOpened || localTerminalUnavailable }, 'The first task bottom workspace panel did not open a terminal or limited-tools launcher', UI_TIMEOUT_MS, ACTIVE_WORKBENCH_SELECTOR ) const firstTaskOpenedTerminal = firstTaskBottomWorkspaceSnapshot.testIds.includes('workspace-terminal-window') && !firstTaskBottomWorkspaceSnapshot.testIds.includes('workspace-tool-launcher')🤖 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/e2e/desktop/task-flow.e2e.mjs` around lines 5303 - 5320, The firstTaskOpenedTerminal flag must match the terminalOpened condition used by waitForSnapshot. Update its check to require workspace-terminal-window and exclude workspace-tool-launcher, so terminal-plus-launcher snapshots follow the limited-tools validation path.
This was referenced Aug 4, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
unlinked_at/timestamps.nullin API responses (IDs, users, snapshots, attachments, content types, and delivered/unlinked times).nullnormalization and dialect-specific adaptation behavior.