Repository navigation
[codex] Source Wework runtime work from executor threads - #1530
Merged
qdaxb merged 1 commit intoJun 23, 2026
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughThe PR removes the ChangesRuntime Work Refactor: executor-native grouping and Projects+Conversations UI
Sequence Diagram(s)sequenceDiagram
participant WeWork
participant BackendAPI as GET /api/runtime-work
participant RuntimeWorkService
participant ExecutorRpc as Executor runtime.tasks.list
WeWork->>BackendAPI: request (no client_origin)
BackendAPI->>RuntimeWorkService: list_runtime_work(db, user_id)
loop each online device
RuntimeWorkService->>ExecutorRpc: runtime.tasks.list via WebSocket RPC
ExecutorRpc-->>RuntimeWorkService: workspaces [{workspaceKind, workspacePath, tasks[{workspaceKind, worktreeId}]}]
end
RuntimeWorkService->>RuntimeWorkService: group by (device_id, workspace_path)
RuntimeWorkService->>RuntimeWorkService: chat → unmappedDeviceWorkspaces, others → projects
RuntimeWorkService->>RuntimeWorkService: _runtime_project_ref_from_workspace (hash-based id, basename name)
RuntimeWorkService-->>BackendAPI: RuntimeWorkListResponse(projects, unmappedDeviceWorkspaces, totalLocalTasks)
BackendAPI-->>WeWork: JSON response
WeWork->>WeWork: sidebarProjects from runtimeWork.projects
WeWork->>WeWork: Conversations section from unmappedDeviceWorkspaces (always shown)
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
frontend/src/apis/devices.tsESLint skipped: missing config or dependency (missing-dependency). The ESLint configuration references a package that is not available in the sandbox. 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 |
icycrystal4
marked this pull request as ready for review
June 23, 2026 13:16
icycrystal4
force-pushed
the
codex/runtime-work-thread-sidebar
branch
from
June 23, 2026 13:21
bf63214 to
ed72d76
Compare
icycrystal4
force-pushed
the
codex/runtime-work-thread-sidebar
branch
from
June 23, 2026 13:25
ed72d76 to
08ef24d
Compare
![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: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
wework/src/components/chat/composer/ProjectWorkBar.tsx (1)
441-442: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winSingle-workspace runtime-only projects are still blocked by
workspace.idgating.With the relaxed runtime model, a selectable single workspace can have
id = null, but this branch only callsonSelectProjectWorkspacewhenoption.workspace?.idis truthy. That prevents direct selection via project click for runtime-only projects.Proposed fix
- if (hasRuntimeWork && option?.kind === 'single' && option.workspace?.id && option.selectable) { - onSelectProjectWorkspace?.(projectId, option.workspace.id) + if (hasRuntimeWork && option?.kind === 'single' && option.workspace && option.selectable) { + onSelectProjectWorkspace?.(projectId, option.workspace.id ?? null) closeMenu() return }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@wework/src/components/chat/composer/ProjectWorkBar.tsx` around lines 441 - 442, The condition that gates the onSelectProjectWorkspace call is preventing runtime-only projects (which may have workspace.id = null) from being selectable. In the ProjectWorkBar component, modify the condition starting with hasRuntimeWork to remove the workspace.id truthiness check (the option.workspace?.id part), since a selectable single workspace can now have an id value of null in the relaxed runtime model. This will allow direct selection via project click for runtime-only projects.wework/src/components/layout/MobileDrawer.tsx (1)
346-408: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winRender runtime-only projects on mobile after removing the unmapped workspace section.
This replacement only renders unmapped chat tasks. Since the project list below still iterates
projects, any executor-reportedruntimeWork.projectswithout a backing project prop no longer has a mobile row after the unmapped workspace section was deleted. Mirror the desktopsidebarProjectsderivation so runtime-only project work remains reachable.Proposed direction
+ const runtimeProjects = useMemo(() => runtimeWork?.projects ?? [], [runtimeWork]) + const drawerProjects = useMemo( + () => + runtimeWork + ? runtimeProjects.map(item => ({ + id: item.project.id, + name: item.project.name, + description: item.project.description, + color: item.project.color, + tasks: [], + })) + : projects, + [projects, runtimeProjects, runtimeWork] + ) + - {projects.map(project => { + {drawerProjects.map(project => { const runtimeProjectWork = runtimeWorkByProjectId.get(project.id)If runtime-only rows are added here, also skip rename/remove long-press actions for rows that are not present in
projects, matching the desktopruntimeOnlyProjectbehavior.🤖 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/MobileDrawer.tsx` around lines 346 - 408, The mobile drawer currently renders only the unmapped chat task items in the section shown, but it does not display runtime-only projects (projects in runtimeWork.projects that don't have a corresponding backing project). You need to find where the projects list is rendered below the chat section (likely iterating over a `projects` prop) and create a derived projects list similar to the desktop implementation's `sidebarProjects`. This derived list should include both the regular projects and any runtime-only projects. Additionally, when rendering rows for runtime-only projects (those not in the original `projects` prop), ensure that long-press actions like rename and remove are disabled or skipped to match the desktop `runtimeOnlyProject` behavior.wework/src/components/layout/DesktopSidebar.tsx (1)
482-513: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winExpose online/offline state in the accessible label.
The dot color is the only status signal here;
titleandaria-labelcurrently contain only the host/device id. Include the status text so non-visual users get the same device-state information.Proposed accessible label update
function ProjectDeviceInlineStatus({ deviceState, testId, className, @@ }) { + const { t } = useTranslation('common') const label = getDeviceNetworkLabel(deviceState.device) || deviceState.deviceId const online = deviceState.status === 'online' + const statusLabel = online + ? t('workbench.device_status_online', '在线') + : t('workbench.device_status_unavailable', '不可用') + const accessibleLabel = `${label} · ${statusLabel}` return ( <span data-testid={testId} - title={label} - aria-label={label} + title={accessibleLabel} + aria-label={accessibleLabel}🤖 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/DesktopSidebar.tsx` around lines 482 - 513, The ProjectDeviceInlineStatus function currently only includes the device label in the title and aria-label attributes, without indicating the online/offline status that is visually conveyed through the dot color. Modify the label variable to include the device status information (either "online" or "offline") appended to the device name/ID, so that the title and aria-label attributes properly communicate the device state to non-visual users. Use the existing online boolean variable to conditionally append the appropriate status text to the label.
🧹 Nitpick comments (3)
executor/tests/test_local_websocket_client.py (1)
31-37: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExpand IP classification coverage to match helper contract.
The new test verifies LAN and loopback, but it misses
is_unspecified,is_multicast, andis_link_localcases introduced by the helper. Adding those assertions will prevent silent regressions.✅ Suggested test extension
def test_usable_device_ip_rejects_loopback_and_accepts_lan_address(): assert _is_usable_device_ip("10.201.3.200") is True assert _is_usable_device_ip("192.168.1.8") is True assert _is_usable_device_ip("127.0.0.1") is False assert _is_usable_device_ip("localhost") is False + assert _is_usable_device_ip("0.0.0.0") is False + assert _is_usable_device_ip("224.0.0.1") is False + assert _is_usable_device_ip("169.254.10.20") is False🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@executor/tests/test_local_websocket_client.py` around lines 31 - 37, The test function test_usable_device_ip_rejects_loopback_and_accepts_lan_address currently only covers LAN addresses and loopback cases, but the _is_usable_device_ip helper function also handles unspecified, multicast, and link-local IP address classifications. Add additional assertions to the test to cover these missing cases by testing representative IP addresses for each type (unspecified addresses, multicast addresses, and link-local addresses) and verify that _is_usable_device_ip returns False for all of them, ensuring comprehensive coverage and preventing silent regressions.wework/src/components/layout/MobileDrawer.tsx (1)
100-673: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftConsider splitting
MobileDraweras part of the runtime UI refactor.The function now owns pull-to-refresh, project actions, project creation, runtime chat rows, and runtime project task rows in one 500+ line component. Extracting the runtime chat/project list sections would make the changed flow easier to test and keep within the preferred function-size guideline. As per coding guidelines, “Functions should not exceed 50 lines of code (preferred maximum).”
🤖 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/MobileDrawer.tsx` around lines 100 - 673, The MobileDrawer component exceeds 500 lines and combines multiple responsibilities including pull-to-refresh, project actions, project creation, runtime chat rendering, and runtime task list rendering. Extract the runtime chat items section (the code rendering unmappedChatTaskItems.map), the projects section (projects.map with expanded task items), and the project actions menu (the div with projectActionTarget and mobile-project-actions-menu) into separate child components. Pass the necessary state setters, handlers, and data as props to these new components, keeping the main MobileDrawer function focused on layout structure, state management, and coordination. This will reduce MobileDrawer below the preferred 50-line guideline and make each section independently testable.Source: Coding guidelines
wework/src/components/layout/DesktopSidebar.tsx (1)
930-1396: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftConsider extracting the new runtime sections from
DesktopSidebar.This component is now 400+ lines inside a 1000+ line file, and the new Projects/Conversations runtime state adds more branching. Extracting
ProjectsSection,ConversationsSection, and device-status helpers would align this changed area with the repository size guidelines. As per coding guidelines, “Files should not exceed 1000 lines” and “Functions should not exceed 50 lines of code (preferred maximum).”🤖 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/DesktopSidebar.tsx` around lines 930 - 1396, The DesktopSidebar component exceeds the repository's size guidelines at over 1000 lines with the new runtime sections adding increased complexity. Extract the Projects section (containing SidebarSectionHeader and ProjectItem rendering logic) into a new ProjectsSection component that accepts props for projects, expanded state, and related callbacks. Similarly, extract the Conversations/Chats section (the section with data-testid="runtime-chat-section" containing RuntimeLocalTaskRow rendering) into a ConversationsSection component with appropriate props for chat items, expanded state, and callbacks. Update DesktopSidebar to instantiate and render these new components instead of containing the full JSX inline, passing the necessary state and handler functions as props to keep the main component focused and under the recommended size limits.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/app/services/runtime_work_service.py`:
- Around line 1750-1751: In the return statement where title is assigned and
then used with `.lower()`, the code does not properly handle the case where
local_tasks[0].title could be None. Modify the title assignment to ensure it
defaults to an empty string if local_tasks[0].title is None before calling
`.lower()`. Update the line that assigns title to check for None values (e.g.,
using `or ""`) in addition to the existing check for whether local_tasks is
empty, so that title is guaranteed to be a string when `.lower()` is called in
the return statement.
- Around line 281-287: The code creates a new RuntimeProjectWork entry for each
workspace, which causes duplicate projects with the same id when multiple
devices report the same workspace_path. Instead of always appending a new
RuntimeProjectWork to the projects list, first check if a project with the same
id (derived from _runtime_project_ref_from_workspace) already exists in the
projects collection. If it exists, append the current workspace to its existing
deviceWorkspaces list. If it does not exist, create and append a new
RuntimeProjectWork. This ensures each unique project appears only once with all
its associated deviceWorkspaces aggregated together.
In `@executor/runtime_work/local_task_store.py`:
- Around line 226-227: In the _optional_text_value method, the condition checks
if value.strip() is truthy to validate the string has content after trimming,
but then returns the untrimmed value instead of the trimmed version. Change the
return statement to return value.strip() instead of value when the condition is
satisfied, ensuring that whitespace is actually removed from the returned string
before it gets persisted or used downstream.
In `@wework/src/components/chat/composer/ProjectWorkBar.tsx`:
- Line 726: The data-testid attribute for the project workspace option uses
workspace.id as the primary identifier with workspace.deviceId as a fallback.
When workspace.id is null, this fallback creates duplicate test IDs across
different workspaces on the same device, causing test flakiness. Modify the
data-testid expression to include a stable per-workspace discriminator (such as
the workspace path or another unique identifier) in addition to
workspace.deviceId when workspace.id is unavailable, ensuring each workspace
gets a unique and stable test identifier regardless of whether it has an id
property.
In `@wework/src/components/layout/DesktopSidebar.tsx`:
- Around line 348-368: The isLoopbackNetworkHost function currently only filters
loopback addresses (localhost, ::1, 127.*) but allows other non-displayable
network addresses to pass through. Enhance the isLoopbackNetworkHost function to
also check for and filter out additional non-displayable addresses including
0.0.0.0, ::, link-local addresses (169.254.* and fe80::*), and multicast
addresses. This ensures that getDisplayableNetworkHost only returns actually
usable network addresses and properly falls back to device id or other
alternatives for non-displayable hosts.
In `@wework/src/features/workbench/WorkbenchProvider.tsx`:
- Around line 760-769: The findProjectDeviceWorkspace function treats null as
"pick the only workspace", which prevents selecting a specific idless runtime
workspace when multiple exist. Instead of storing just a numeric workspace ID in
selectProjectWorkspace, change it to store a discriminated union target type
that can represent both ID-based workspaces (deviceWorkspaceId) and idless
runtime-only workspaces (deviceId and workspacePath). Update
findProjectDeviceWorkspace to match against these discriminated targets by
comparing deviceId and workspacePath when no mapping ID exists. Apply this same
pattern change to the other affected locations noted in the comment at lines
1359-1368 and 1985-1994.
---
Outside diff comments:
In `@wework/src/components/chat/composer/ProjectWorkBar.tsx`:
- Around line 441-442: The condition that gates the onSelectProjectWorkspace
call is preventing runtime-only projects (which may have workspace.id = null)
from being selectable. In the ProjectWorkBar component, modify the condition
starting with hasRuntimeWork to remove the workspace.id truthiness check (the
option.workspace?.id part), since a selectable single workspace can now have an
id value of null in the relaxed runtime model. This will allow direct selection
via project click for runtime-only projects.
In `@wework/src/components/layout/DesktopSidebar.tsx`:
- Around line 482-513: The ProjectDeviceInlineStatus function currently only
includes the device label in the title and aria-label attributes, without
indicating the online/offline status that is visually conveyed through the dot
color. Modify the label variable to include the device status information
(either "online" or "offline") appended to the device name/ID, so that the title
and aria-label attributes properly communicate the device state to non-visual
users. Use the existing online boolean variable to conditionally append the
appropriate status text to the label.
In `@wework/src/components/layout/MobileDrawer.tsx`:
- Around line 346-408: The mobile drawer currently renders only the unmapped
chat task items in the section shown, but it does not display runtime-only
projects (projects in runtimeWork.projects that don't have a corresponding
backing project). You need to find where the projects list is rendered below the
chat section (likely iterating over a `projects` prop) and create a derived
projects list similar to the desktop implementation's `sidebarProjects`. This
derived list should include both the regular projects and any runtime-only
projects. Additionally, when rendering rows for runtime-only projects (those not
in the original `projects` prop), ensure that long-press actions like rename and
remove are disabled or skipped to match the desktop `runtimeOnlyProject`
behavior.
---
Nitpick comments:
In `@executor/tests/test_local_websocket_client.py`:
- Around line 31-37: The test function
test_usable_device_ip_rejects_loopback_and_accepts_lan_address currently only
covers LAN addresses and loopback cases, but the _is_usable_device_ip helper
function also handles unspecified, multicast, and link-local IP address
classifications. Add additional assertions to the test to cover these missing
cases by testing representative IP addresses for each type (unspecified
addresses, multicast addresses, and link-local addresses) and verify that
_is_usable_device_ip returns False for all of them, ensuring comprehensive
coverage and preventing silent regressions.
In `@wework/src/components/layout/DesktopSidebar.tsx`:
- Around line 930-1396: The DesktopSidebar component exceeds the repository's
size guidelines at over 1000 lines with the new runtime sections adding
increased complexity. Extract the Projects section (containing
SidebarSectionHeader and ProjectItem rendering logic) into a new ProjectsSection
component that accepts props for projects, expanded state, and related
callbacks. Similarly, extract the Conversations/Chats section (the section with
data-testid="runtime-chat-section" containing RuntimeLocalTaskRow rendering)
into a ConversationsSection component with appropriate props for chat items,
expanded state, and callbacks. Update DesktopSidebar to instantiate and render
these new components instead of containing the full JSX inline, passing the
necessary state and handler functions as props to keep the main component
focused and under the recommended size limits.
In `@wework/src/components/layout/MobileDrawer.tsx`:
- Around line 100-673: The MobileDrawer component exceeds 500 lines and combines
multiple responsibilities including pull-to-refresh, project actions, project
creation, runtime chat rendering, and runtime task list rendering. Extract the
runtime chat items section (the code rendering unmappedChatTaskItems.map), the
projects section (projects.map with expanded task items), and the project
actions menu (the div with projectActionTarget and mobile-project-actions-menu)
into separate child components. Pass the necessary state setters, handlers, and
data as props to these new components, keeping the main MobileDrawer function
focused on layout structure, state management, and coordination. This will
reduce MobileDrawer below the preferred 50-line guideline and make each section
independently testable.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 086bcce7-69b6-46e6-8437-6aa259543128
📒 Files selected for processing (28)
backend/app/api/endpoints/runtime_work.pybackend/app/services/runtime_work_service.pybackend/tests/api/endpoints/test_runtime_work_api.pybackend/tests/services/test_runtime_work_service.pydocs/en/developer-guide/runtime-local-work.mddocs/zh/developer-guide/runtime-local-work.mdexecutor/modes/local/websocket_client.pyexecutor/runtime_work/codex_discovery.pyexecutor/runtime_work/local_task_store.pyexecutor/runtime_work/rpc_handler.pyexecutor/tests/test_local_websocket_client.pyfrontend/src/apis/devices.tswework/src/api/devices.test.tswework/src/api/runtimeWork.test.tswework/src/api/runtimeWork.tswework/src/components/chat/composer/ProjectWorkBar.tsxwework/src/components/layout/DesktopSidebar.test.tsxwework/src/components/layout/DesktopSidebar.tsxwework/src/components/layout/DesktopWorkbenchLayout.test.tsxwework/src/components/layout/MobileDrawer.tsxwework/src/components/layout/MobileWorkbenchLayout.test.tsxwework/src/components/layout/runtimeTaskSidebarHelpers.tswework/src/features/workbench/WorkbenchProvider.test.tsxwework/src/features/workbench/WorkbenchProvider.tsxwework/src/lib/project-workspace-selection.test.tswework/src/lib/project-workspace-selection.tswework/src/types/api.tswework/src/types/devices.ts
💤 Files with no reviewable changes (1)
- wework/src/components/layout/runtimeTaskSidebarHelpers.ts
Comment on lines
+281
to
287
| project_ref = _runtime_project_ref_from_workspace(workspace_path) | ||
| projects.append( | ||
| RuntimeProjectWork( | ||
| project=project_ref, | ||
| deviceWorkspaces=[workspace], | ||
| ) | ||
| ) |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Aggregate by derived project instead of emitting duplicate project rows.
At Line 281–Line 287, each workspace creates a new RuntimeProjectWork. If two devices report the same workspace_path, the response can contain duplicate projects with the same derived project.id, breaking the project -> deviceWorkspaces[] contract shape.
Proposed fix
- projects: list[RuntimeProjectWork] = []
+ projects: list[RuntimeProjectWork] = []
+ projects_by_id: dict[int, RuntimeProjectWork] = {}
@@
- project_ref = _runtime_project_ref_from_workspace(workspace_path)
- projects.append(
- RuntimeProjectWork(
- project=project_ref,
- deviceWorkspaces=[workspace],
- )
- )
+ project_ref = _runtime_project_ref_from_workspace(workspace_path)
+ project_work = projects_by_id.get(project_ref.id)
+ if project_work is None:
+ project_work = RuntimeProjectWork(
+ project=project_ref,
+ deviceWorkspaces=[],
+ )
+ projects_by_id[project_ref.id] = project_work
+ projects.append(project_work)
+ project_work.device_workspaces.append(workspace)🤖 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/runtime_work_service.py` around lines 281 - 287, The
code creates a new RuntimeProjectWork entry for each workspace, which causes
duplicate projects with the same id when multiple devices report the same
workspace_path. Instead of always appending a new RuntimeProjectWork to the
projects list, first check if a project with the same id (derived from
_runtime_project_ref_from_workspace) already exists in the projects collection.
If it exists, append the current workspace to its existing deviceWorkspaces
list. If it does not exist, create and append a new RuntimeProjectWork. This
ensures each unique project appears only once with all its associated
deviceWorkspaces aggregated together.
Comment on lines
+1750
to
+1751
| title = local_tasks[0].title if local_tasks else "" | ||
| return (-latest, title.lower()) |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Handle nullable task titles in workspace sort key.
At Line 1750–Line 1751, title.lower() can raise if local_tasks[0].title is None, which would fail runtime-work listing for otherwise valid task payloads.
Proposed fix
- title = local_tasks[0].title if local_tasks else ""
+ title = (local_tasks[0].title or "") if local_tasks else ""
return (-latest, title.lower())📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| title = local_tasks[0].title if local_tasks else "" | |
| return (-latest, title.lower()) | |
| title = (local_tasks[0].title or "") if local_tasks else "" | |
| return (-latest, title.lower()) |
🤖 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/runtime_work_service.py` around lines 1750 - 1751, In
the return statement where title is assigned and then used with `.lower()`, the
code does not properly handle the case where local_tasks[0].title could be None.
Modify the title assignment to ensure it defaults to an empty string if
local_tasks[0].title is None before calling `.lower()`. Update the line that
assigns title to check for None values (e.g., using `or ""`) in addition to the
existing check for whether local_tasks is empty, so that title is guaranteed to
be a string when `.lower()` is called in the return statement.
Comment on lines
+226
to
+227
| def _optional_text_value(self, value: Any) -> Optional[str]: | ||
| return value if isinstance(value, str) and value.strip() else None |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Trim worktree_id before persisting parsed payloads.
Line 226 checks value.strip() but returns untrimmed value, so " wt-123 " is stored with whitespace and can cause downstream key mismatches.
💡 Proposed fix
def _optional_text_value(self, value: Any) -> Optional[str]:
- return value if isinstance(value, str) and value.strip() else None
+ if not isinstance(value, str):
+ return None
+ normalized = value.strip()
+ return normalized or None🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@executor/runtime_work/local_task_store.py` around lines 226 - 227, In the
_optional_text_value method, the condition checks if value.strip() is truthy to
validate the string has content after trimming, but then returns the untrimmed
value instead of the trimmed version. Change the return statement to return
value.strip() instead of value when the condition is satisfied, ensuring that
whitespace is actually removed from the returned string before it gets persisted
or used downstream.
| key={`${workspace.deviceId}:${workspace.workspacePath}`} | ||
| type="button" | ||
| data-testid={`project-workspace-option-${workspace.id}`} | ||
| data-testid={`project-workspace-option-${workspace.id ?? workspace.deviceId}`} |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
data-testid fallback can collide for multiple workspaces on one device.
When workspace.id is null, using only workspace.deviceId can duplicate IDs across different workspace paths, which can make E2E tests flaky. Include a stable per-workspace discriminator.
Proposed fix
- data-testid={`project-workspace-option-${workspace.id ?? workspace.deviceId}`}
+ data-testid={`project-workspace-option-${
+ workspace.id ?? `${workspace.deviceId}-${workspace.workspacePath}`
+ }`}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| data-testid={`project-workspace-option-${workspace.id ?? workspace.deviceId}`} | |
| data-testid={`project-workspace-option-${ | |
| workspace.id ?? `${workspace.deviceId}-${workspace.workspacePath}` | |
| }`} |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@wework/src/components/chat/composer/ProjectWorkBar.tsx` at line 726, The
data-testid attribute for the project workspace option uses workspace.id as the
primary identifier with workspace.deviceId as a fallback. When workspace.id is
null, this fallback creates duplicate test IDs across different workspaces on
the same device, causing test flakiness. Modify the data-testid expression to
include a stable per-workspace discriminator (such as the workspace path or
another unique identifier) in addition to workspace.deviceId when workspace.id
is unavailable, ensuring each workspace gets a unique and stable test identifier
regardless of whether it has an id property.
Source: Coding guidelines
Comment on lines
+348
to
+368
| function getDisplayableNetworkHost(value?: string | null): string | null { | ||
| if (!value) return null | ||
| const host = extractNetworkHost(value.trim()) | ||
| if (!host || isLoopbackNetworkHost(host)) return null | ||
| return host | ||
| } | ||
|
|
||
| function extractNetworkHost(value: string): string { | ||
| const bracketMatch = value.match(/^\[([^\]]+)\](?::\d+)?$/) | ||
| if (bracketMatch?.[1]) return bracketMatch[1] | ||
| const colonParts = value.split(':') | ||
| if (colonParts.length === 2 && /^\d+$/.test(colonParts[1])) { | ||
| return colonParts[0] | ||
| } | ||
| return value | ||
| } | ||
|
|
||
| function isLoopbackNetworkHost(host: string): boolean { | ||
| const normalized = host.trim().toLowerCase() | ||
| return normalized === 'localhost' || normalized === '::1' || normalized.startsWith('127.') | ||
| } |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Filter all non-displayable network hosts before rendering them.
getDisplayableNetworkHost currently drops only loopback hosts, so values like 0.0.0.0, ::, 169.254.*, fe80::*, or multicast addresses can still appear as the project network label. Treat these as non-displayable and fall back to the device id or a usable host.
Proposed filter tightening
function getDisplayableNetworkHost(value?: string | null): string | null {
if (!value) return null
const host = extractNetworkHost(value.trim())
- if (!host || isLoopbackNetworkHost(host)) return null
+ if (!host || isNonDisplayableNetworkHost(host)) return null
return host
}
-function isLoopbackNetworkHost(host: string): boolean {
+function isNonDisplayableNetworkHost(host: string): boolean {
const normalized = host.trim().toLowerCase()
- return normalized === 'localhost' || normalized === '::1' || normalized.startsWith('127.')
+ const firstOctet = Number(normalized.split('.')[0])
+ const ipv4Multicast = Number.isInteger(firstOctet) && firstOctet >= 224 && firstOctet <= 239
+
+ return (
+ normalized === 'localhost' ||
+ normalized === '::1' ||
+ normalized === '::' ||
+ normalized === '0.0.0.0' ||
+ normalized.startsWith('127.') ||
+ normalized.startsWith('169.254.') ||
+ normalized.startsWith('fe80:') ||
+ normalized.startsWith('ff') ||
+ ipv4Multicast
+ )
}🤖 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/DesktopSidebar.tsx` around lines 348 - 368, The
isLoopbackNetworkHost function currently only filters loopback addresses
(localhost, ::1, 127.*) but allows other non-displayable network addresses to
pass through. Enhance the isLoopbackNetworkHost function to also check for and
filter out additional non-displayable addresses including 0.0.0.0, ::,
link-local addresses (169.254.* and fe80::*), and multicast addresses. This
ensures that getDisplayableNetworkHost only returns actually usable network
addresses and properly falls back to device id or other alternatives for
non-displayable hosts.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/tests/services/test_runtime_work_service.py (1)
480-484: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAdd type annotations to the new async test signatures.
These newly added test functions are missing parameter and return type hints (
-> None), which violates the repo’s Python guideline.Proposed fix
+from typing import Any + `@pytest.mark.asyncio` async def test_list_runtime_work_groups_executor_workspaces_without_project_mapping( - test_db, - test_user, - monkeypatch, -): + test_db: Any, + test_user: Any, + monkeypatch: pytest.MonkeyPatch, +) -> None: @@ `@pytest.mark.asyncio` async def test_list_runtime_work_preserves_executor_workspace_kind( - test_db, - test_user, - monkeypatch, -): + test_db: Any, + test_user: Any, + monkeypatch: pytest.MonkeyPatch, +) -> None: @@ `@pytest.mark.asyncio` async def test_list_runtime_work_skips_offline_devices( - test_db, - test_user, - monkeypatch, -): + test_db: Any, + test_user: Any, + monkeypatch: pytest.MonkeyPatch, +) -> None:As per coding guidelines,
**/*.py: “Use type hints for all function parameters and return values.”Also applies to: 599-603, 661-665
🤖 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/services/test_runtime_work_service.py` around lines 480 - 484, Add type annotations to the test functions test_list_runtime_work_groups_executor_workspaces_without_project_mapping, and the other similar test functions mentioned at lines 599-603 and 661-665. For each function, add type hints for all parameters (test_db, test_user, monkeypatch, and any others) and add the return type annotation of -> None to comply with the repository's Python coding guidelines requiring type hints for all function parameters and return values.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 `@executor/modes/local/websocket_client.py`:
- Around line 76-87: The functions `_is_usable_device_ip` and `_get_client_ip`
are missing required tracing instrumentation. First, ensure the `trace_sync`
decorator is imported from `shared.telemetry.decorators` at the top of the file.
Then, add the `@trace_sync` decorator to both the `_is_usable_device_ip`
function definition and the `_get_client_ip` function definition (located at
lines 362-395) to comply with the coding guidelines for tracing sync functions.
---
Outside diff comments:
In `@backend/tests/services/test_runtime_work_service.py`:
- Around line 480-484: Add type annotations to the test functions
test_list_runtime_work_groups_executor_workspaces_without_project_mapping, and
the other similar test functions mentioned at lines 599-603 and 661-665. For
each function, add type hints for all parameters (test_db, test_user,
monkeypatch, and any others) and add the return type annotation of -> None to
comply with the repository's Python coding guidelines requiring type hints for
all function parameters and return values.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 12db4382-c4f9-4cb7-a720-3904e5dc8c8a
📒 Files selected for processing (28)
backend/app/api/endpoints/runtime_work.pybackend/app/services/runtime_work_service.pybackend/tests/api/endpoints/test_runtime_work_api.pybackend/tests/services/test_runtime_work_service.pydocs/en/developer-guide/runtime-local-work.mddocs/zh/developer-guide/runtime-local-work.mdexecutor/modes/local/websocket_client.pyexecutor/runtime_work/codex_discovery.pyexecutor/runtime_work/local_task_store.pyexecutor/runtime_work/rpc_handler.pyexecutor/tests/test_local_websocket_client.pyfrontend/src/apis/devices.tswework/src/api/devices.test.tswework/src/api/runtimeWork.test.tswework/src/api/runtimeWork.tswework/src/components/chat/composer/ProjectWorkBar.tsxwework/src/components/layout/DesktopSidebar.test.tsxwework/src/components/layout/DesktopSidebar.tsxwework/src/components/layout/DesktopWorkbenchLayout.test.tsxwework/src/components/layout/MobileDrawer.tsxwework/src/components/layout/MobileWorkbenchLayout.test.tsxwework/src/components/layout/runtimeTaskSidebarHelpers.tswework/src/features/workbench/WorkbenchProvider.test.tsxwework/src/features/workbench/WorkbenchProvider.tsxwework/src/lib/project-workspace-selection.test.tswework/src/lib/project-workspace-selection.tswework/src/types/api.tswework/src/types/devices.ts
💤 Files with no reviewable changes (1)
- wework/src/components/layout/runtimeTaskSidebarHelpers.ts
✅ Files skipped from review due to trivial changes (2)
- frontend/src/apis/devices.ts
- docs/zh/developer-guide/runtime-local-work.md
🚧 Files skipped from review as they are similar to previous changes (23)
- wework/src/api/runtimeWork.test.ts
- wework/src/api/runtimeWork.ts
- wework/src/types/api.ts
- backend/tests/api/endpoints/test_runtime_work_api.py
- executor/runtime_work/rpc_handler.py
- executor/runtime_work/codex_discovery.py
- wework/src/lib/project-workspace-selection.test.ts
- wework/src/lib/project-workspace-selection.ts
- wework/src/components/chat/composer/ProjectWorkBar.tsx
- executor/runtime_work/local_task_store.py
- wework/src/types/devices.ts
- executor/tests/test_local_websocket_client.py
- docs/en/developer-guide/runtime-local-work.md
- wework/src/components/layout/MobileWorkbenchLayout.test.tsx
- wework/src/api/devices.test.ts
- wework/src/features/workbench/WorkbenchProvider.test.tsx
- wework/src/components/layout/DesktopSidebar.test.tsx
- wework/src/components/layout/DesktopWorkbenchLayout.test.tsx
- backend/app/api/endpoints/runtime_work.py
- wework/src/components/layout/MobileDrawer.tsx
- wework/src/components/layout/DesktopSidebar.tsx
- backend/app/services/runtime_work_service.py
- wework/src/features/workbench/WorkbenchProvider.tsx
Comment on lines
+76
to
+87
| def _is_usable_device_ip(value: str) -> bool: | ||
| try: | ||
| address = ipaddress.ip_address(value.strip()) | ||
| except ValueError: | ||
| return False | ||
| return not ( | ||
| address.is_loopback | ||
| or address.is_unspecified | ||
| or address.is_multicast | ||
| or address.is_link_local | ||
| ) | ||
|
|
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Apply @trace_sync to the new/touched sync IP-detection functions.
_is_usable_device_ip is newly introduced and _get_client_ip is modified, but neither path is instrumented with the required tracing decorator.
As per coding guidelines, **/*.py: “Use @trace_async or @trace_sync decorators from shared.telemetry.decorators for tracing async and sync functions respectively; use add_span_event() and set_span_attribute() for adding details to spans.”
Also applies to: 362-395
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@executor/modes/local/websocket_client.py` around lines 76 - 87, The functions
`_is_usable_device_ip` and `_get_client_ip` are missing required tracing
instrumentation. First, ensure the `trace_sync` decorator is imported from
`shared.telemetry.decorators` at the top of the file. Then, add the
`@trace_sync` decorator to both the `_is_usable_device_ip` function definition
and the `_get_client_ip` function definition (located at lines 362-395) to
comply with the coding guidelines for tracing sync functions.
Source: Coding guidelines
This was referenced Jun 24, 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
Validation
Summary by CodeRabbit
Release Notes
New Features
Improvements
Bug Fixes