Repository navigation
fix(wework): prevent horizontal conversation drift - #2109
Merged
Merged
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds execution-device controls to the project composer, centralizes runtime transcript merging, removes running-task transcript polling, resets desktop horizontal scrolling, repairs legacy task timestamps, makes the settings sidebar vertically scrollable, and waits for marketplace controls before E2E snapshots. ChangesWorkbench UI updates
Runtime transcript handling
Runtime task repairs
Desktop end-to-end readiness
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant ProjectChatComposer
participant ComposerToolbar
participant ProjectWorkBar
participant ProjectMenu
ProjectChatComposer->>ComposerToolbar: provide resolved execution device
ComposerToolbar->>ProjectChatComposer: invoke device selection
ProjectChatComposer->>ProjectWorkBar: update external menu signal
ProjectWorkBar->>ProjectMenu: close execution menu and open project menu
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 |
![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: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@wework/src/components/chat/ChatInput.test.tsx`:
- Around line 3286-3359: Extend the execution-device coverage around the
existing ChatInput tests with a real-Tauri backend E2E: select both local and
cloud devices, launch a task, verify the actual runtime target, and confirm
device switching is blocked while the task is active. Add assertions for launch
failure and subsequent recovery, and document cleanup evidence in the QA plan;
keep the current control and menu unit-test coverage intact.
In `@wework/src/components/layout/DesktopWorkbenchMain.tsx`:
- Around line 476-483: Update the dependency list of the useLayoutEffect that
resets workbenchScroll.scrollLeft to include paneKey alongside paneActive, so
horizontal scrolling resets when the active pane/session identity changes
without altering the existing reset logic.
In `@wework/src/components/layout/runtimeTranscriptMessages.ts`:
- Around line 12-22: Normalize assistant subtask IDs consistently before storing
and retrieving entries in the maps used by the runtime transcript message flow,
using a single subtaskId helper that applies String(...) and no subtask_id
alias. Update both restored-message indexing and live-message lookup to use it,
and add a regression test covering a numeric restored ID matched with an
equivalent string live ID.
🪄 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: 341fdc66-9f61-4e09-9f7c-5fe6df0855f4
📒 Files selected for processing (14)
wework/src/components/chat/ChatInput.test.tsxwework/src/components/chat/ChatInput.tsxwework/src/components/chat/composer/ComposerToolbar.tsxwework/src/components/chat/composer/ProjectChatComposer.tsxwework/src/components/chat/composer/ProjectWorkBar.tsxwework/src/components/layout/DesktopWorkbenchLayout.test.tsxwework/src/components/layout/DesktopWorkbenchMain.tsxwework/src/components/layout/runtimeTranscriptMessages.test.tswework/src/components/layout/runtimeTranscriptMessages.tswework/src/components/layout/useWorkbenchPaneEnvironment.tswework/src/components/layout/useWorkbenchPaneSession.tswework/src/components/layout/useWorkbenchProjectWorkControls.tswework/src/i18n/locales/en/common.jsonwework/src/i18n/locales/zh-CN/common.json
Comment on lines
+3286
to
+3359
| test('shows a subtle cloud-device control and opens device selection for a new task', async () => { | ||
| const devices: DeviceInfo[] = [ | ||
| { | ||
| id: 1, | ||
| device_id: 'cloud-online', | ||
| name: 'Cloud Online', | ||
| status: 'online', | ||
| is_default: false, | ||
| device_type: 'cloud', | ||
| }, | ||
| ] | ||
|
|
||
| render( | ||
| <ChatInput | ||
| value="" | ||
| onChange={vi.fn()} | ||
| onSubmit={vi.fn()} | ||
| disabled={false} | ||
| variant="desktop" | ||
| projectWork={projectWorkControls({ | ||
| devices, | ||
| currentStandaloneDeviceId: 'cloud-online', | ||
| })} | ||
| /> | ||
| ) | ||
|
|
||
| const deviceButton = screen.getByTestId('composer-execution-device-button') | ||
| expect(deviceButton).toHaveAccessibleName('运行于云设备 · Cloud Online') | ||
| expect(deviceButton).toBeEnabled() | ||
|
|
||
| await userEvent.click(deviceButton) | ||
| expect(screen.getByTestId('project-work-menu')).toBeInTheDocument() | ||
| }) | ||
|
|
||
| test('shows the actual runtime device without allowing an active task to switch it', () => { | ||
| const devices: DeviceInfo[] = [ | ||
| { | ||
| id: 1, | ||
| device_id: 'local-online', | ||
| name: 'Local Online', | ||
| status: 'online', | ||
| is_default: false, | ||
| device_type: 'local', | ||
| }, | ||
| { | ||
| id: 2, | ||
| device_id: 'cloud-online', | ||
| name: 'Cloud Online', | ||
| status: 'online', | ||
| is_default: false, | ||
| device_type: 'cloud', | ||
| }, | ||
| ] | ||
|
|
||
| render( | ||
| <ChatInput | ||
| value="" | ||
| onChange={vi.fn()} | ||
| onSubmit={vi.fn()} | ||
| disabled={false} | ||
| variant="desktop" | ||
| showProjectWorkBar={false} | ||
| projectWork={projectWorkControls({ | ||
| devices, | ||
| currentStandaloneDeviceId: 'local-online', | ||
| currentRuntimeDeviceId: 'cloud-online', | ||
| })} | ||
| /> | ||
| ) | ||
|
|
||
| const deviceButton = screen.getByTestId('composer-execution-device-button') | ||
| expect(deviceButton).toHaveAccessibleName('运行于云设备 · Cloud Online') | ||
| expect(deviceButton).toHaveAttribute('aria-disabled', 'true') | ||
| }) |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Add real-Tauri execution-device launch coverage.
These tests only verify the control and menu. Add a real-backend E2E that selects local/cloud execution, launches a task, verifies the runtime target, then verifies switching is blocked while active. Include failure/recovery and cleanup evidence in the QA plan.
🤖 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/ChatInput.test.tsx` around lines 3286 - 3359,
Extend the execution-device coverage around the existing ChatInput tests with a
real-Tauri backend E2E: select both local and cloud devices, launch a task,
verify the actual runtime target, and confirm device switching is blocked while
the task is active. Add assertions for launch failure and subsequent recovery,
and document cleanup evidence in the QA plan; keep the current control and menu
unit-test coverage intact.
Source: Coding guidelines
Comment on lines
+476
to
+483
| useLayoutEffect(() => { | ||
| if (!paneActive) return | ||
|
|
||
| const workbenchScroll = workbenchScrollRef.current | ||
| if (workbenchScroll && workbenchScroll.scrollLeft !== 0) { | ||
| workbenchScroll.scrollLeft = 0 | ||
| } | ||
| }, [paneActive]) |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Reset on pane identity changes, not only activation changes.
paneKey is the existing pane/session identity, but this effect depends only on paneActive. If the mounted component switches from one active task to another while paneActive remains true, the effect will not rerun and stale horizontal scrolling can persist. Include paneKey in the dependency list.
Proposed fix
- }, [paneActive])
+ }, [paneActive, paneKey])📝 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
| useLayoutEffect(() => { | |
| if (!paneActive) return | |
| const workbenchScroll = workbenchScrollRef.current | |
| if (workbenchScroll && workbenchScroll.scrollLeft !== 0) { | |
| workbenchScroll.scrollLeft = 0 | |
| } | |
| }, [paneActive]) | |
| useLayoutEffect(() => { | |
| if (!paneActive) return | |
| const workbenchScroll = workbenchScrollRef.current | |
| if (workbenchScroll && workbenchScroll.scrollLeft !== 0) { | |
| workbenchScroll.scrollLeft = 0 | |
| } | |
| }, [paneActive, paneKey]) |
🤖 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 476 -
483, Update the dependency list of the useLayoutEffect that resets
workbenchScroll.scrollLeft to include paneKey alongside paneActive, so
horizontal scrolling resets when the active pane/session identity changes
without altering the existing reset logic.
Comment on lines
+12
to
+22
| if (message.role === 'assistant' && message.subtaskId) { | ||
| assistantIndexesBySubtask.set(message.subtaskId, index) | ||
| } | ||
| }) | ||
|
|
||
| for (const liveMessage of liveMessages) { | ||
| if (indexesById.has(liveMessage.id)) continue | ||
|
|
||
| const matchingSubtaskIndex = | ||
| liveMessage.role === 'assistant' && liveMessage.subtaskId | ||
| ? assistantIndexesBySubtask.get(liveMessage.subtaskId) |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Normalize subtaskId before indexing and lookup.
A restored numeric ID (42) and live string ID ('42') create different Map keys, so the same assistant turn is rendered twice. Normalize both values with String(...) in a single subtaskId helper; do not add a subtask_id alias. Add a regression test for the mixed numeric/string pair.
Proposed fix
+function assistantSubtaskId(message: WorkbenchMessage): string | null {
+ if (message.role !== 'assistant' || message.subtaskId == null) return null
+ return String(message.subtaskId) || null
+}
+
merged.forEach((message, index) => {
- if (message.role === 'assistant' && message.subtaskId) {
- assistantIndexesBySubtask.set(message.subtaskId, index)
+ const subtaskId = assistantSubtaskId(message)
+ if (subtaskId) {
+ assistantIndexesBySubtask.set(subtaskId, index)
}
})
for (const liveMessage of liveMessages) {
+ const liveSubtaskId = assistantSubtaskId(liveMessage)
if (indexesById.has(liveMessage.id)) continue
const matchingSubtaskIndex =
- liveMessage.role === 'assistant' && liveMessage.subtaskId
- ? assistantIndexesBySubtask.get(liveMessage.subtaskId)
+ liveSubtaskId
+ ? assistantIndexesBySubtask.get(liveSubtaskId)
: undefinedBased on learnings, normalize incoming transcript identities to strings while relying solely on subtaskId.
📝 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
| if (message.role === 'assistant' && message.subtaskId) { | |
| assistantIndexesBySubtask.set(message.subtaskId, index) | |
| } | |
| }) | |
| for (const liveMessage of liveMessages) { | |
| if (indexesById.has(liveMessage.id)) continue | |
| const matchingSubtaskIndex = | |
| liveMessage.role === 'assistant' && liveMessage.subtaskId | |
| ? assistantIndexesBySubtask.get(liveMessage.subtaskId) | |
| function assistantSubtaskId(message: WorkbenchMessage): string | null { | |
| if (message.role !== 'assistant' || message.subtaskId == null) return null | |
| return String(message.subtaskId) || null | |
| } | |
| merged.forEach((message, index) => { | |
| const subtaskId = assistantSubtaskId(message) | |
| if (subtaskId) { | |
| assistantIndexesBySubtask.set(subtaskId, index) | |
| } | |
| }) | |
| for (const liveMessage of liveMessages) { | |
| const liveSubtaskId = assistantSubtaskId(liveMessage) | |
| if (indexesById.has(liveMessage.id)) continue | |
| const matchingSubtaskIndex = | |
| liveSubtaskId | |
| ? assistantIndexesBySubtask.get(liveSubtaskId) | |
| : undefined |
🤖 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/runtimeTranscriptMessages.ts` around lines 12 -
22, Normalize assistant subtask IDs consistently before storing and retrieving
entries in the maps used by the runtime transcript message flow, using a single
subtaskId helper that applies String(...) and no subtask_id alias. Update both
restored-message indexing and live-message lookup to use it, and add a
regression test covering a numeric restored ID matched with an equivalent string
live ID.
Source: Learnings
…o66/Wegent into human/cobra-20260722-105257
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/src/runtime_work/handler.rs`:
- Around line 3146-3158: Update repair_legacy_task_activity_time to repair
timestamps only for terminal tasks or tasks explicitly marked as legacy, rather
than relying solely on running == false; preserve active non-running tasks
without setting completed_at. Add a regression test covering an idle active task
during transcript reads and verifying it remains resumable without stale
completion data.
In `@wework/src/components/layout/runtimeTranscriptMessages.ts`:
- Around line 7-13: Deduplicate transcript entries by ID before merging in the
runtime transcript flow: update the construction around the existing merge logic
in wework/src/components/layout/runtimeTranscriptMessages.ts lines 7-13 to
process transcriptMessages through the same ID-checked insertion path,
preserving the first occurrence and insertion order before adding
existingMessages. Add a test case in
wework/src/components/layout/runtimeTranscriptMessages.test.ts lines 17-27 with
duplicate transcript IDs and assert only the first entry remains.
🪄 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: a73cb20f-1b87-4b72-90ff-810ea11e13aa
📒 Files selected for processing (7)
executor/src/runtime_work/handler.rswework/src/components/layout/runtimeTranscriptMessages.test.tswework/src/components/layout/runtimeTranscriptMessages.tswework/src/components/layout/useWorkbenchPaneSession.tswework/src/components/settings/ConnectionsSettingsPage.test.tsxwework/src/components/settings/ConnectionsSettingsPage.tsxwework/src/features/workbench/WorkbenchProvider.test.tsx
Comment on lines
+3146
to
+3158
| fn repair_legacy_task_activity_time(&self, local_task_id: &str, thread: &Value) { | ||
| if self.is_active_local_task(local_task_id) { | ||
| return; | ||
| } | ||
| let Some(thread_updated_at) = timestamp_ms_field(thread, "updatedAt") else { | ||
| return; | ||
| }; | ||
| self.store.update_task(local_task_id, |link| { | ||
| if !link.running && link.completed_at.is_none() && link.updated_at > thread_updated_at { | ||
| link.updated_at = thread_updated_at; | ||
| link.completed_at = Some(thread_updated_at); | ||
| } | ||
| }); |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Restrict timestamp repair to terminal tasks.
running == false is not sufficient: this file also creates/restores non-terminal tasks with status = "active" and running = false (Lines 1576-1577 and 3763-3765). Since this helper runs during transcript reads (Line 1330), it can persist completed_at on an idle active task and leave stale completion data when the task is later resumed. Gate the repair on a terminal status or an explicit legacy marker, and add an active-task regression test.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@executor/src/runtime_work/handler.rs` around lines 3146 - 3158, Update
repair_legacy_task_activity_time to repair timestamps only for terminal tasks or
tasks explicitly marked as legacy, rather than relying solely on running ==
false; preserve active non-running tasks without setting completed_at. Add a
regression test covering an idle active task during transcript reads and
verifying it remains resumable without stale completion data.
Comment on lines
+7
to
+13
| const merged = [...transcriptMessages] | ||
| const messageIds = new Set(merged.map(message => message.id)) | ||
|
|
||
| for (const message of existingMessages) { | ||
| if (messageIds.has(message.id)) continue | ||
| messageIds.add(message.id) | ||
| merged.push(message) |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Deduplicate IDs within the incoming transcript page.
merged starts as a direct copy of transcriptMessages, so repeated IDs in that page remain rendered twice. Deduplicate both inputs in insertion order, preserving the first transcript entry.
wework/src/components/layout/runtimeTranscriptMessages.ts#L7-L13: buildmergedthrough the same ID-checked path used forexistingMessages.wework/src/components/layout/runtimeTranscriptMessages.test.ts#L17-L27: add a case with two transcript entries sharing an ID and assert only the first remains.
Proposed fix
- const merged = [...transcriptMessages]
- const messageIds = new Set(merged.map(message => message.id))
-
- for (const message of existingMessages) {
+ const merged: WorkbenchMessage[] = []
+ const messageIds = new Set<string>()
+ for (const message of [...transcriptMessages, ...existingMessages]) {
if (messageIds.has(message.id)) continue
messageIds.add(message.id)
merged.push(message)
}📝 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
| const merged = [...transcriptMessages] | |
| const messageIds = new Set(merged.map(message => message.id)) | |
| for (const message of existingMessages) { | |
| if (messageIds.has(message.id)) continue | |
| messageIds.add(message.id) | |
| merged.push(message) | |
| const merged: WorkbenchMessage[] = [] | |
| const messageIds = new Set<string>() | |
| for (const message of [...transcriptMessages, ...existingMessages]) { | |
| if (messageIds.has(message.id)) continue | |
| messageIds.add(message.id) | |
| merged.push(message) | |
| } |
📍 Affects 2 files
wework/src/components/layout/runtimeTranscriptMessages.ts#L7-L13(this comment)wework/src/components/layout/runtimeTranscriptMessages.test.ts#L17-L27
🤖 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/runtimeTranscriptMessages.ts` around lines 7 -
13, Deduplicate transcript entries by ID before merging in the runtime
transcript flow: update the construction around the existing merge logic in
wework/src/components/layout/runtimeTranscriptMessages.ts lines 7-13 to process
transcriptMessages through the same ID-checked insertion path, preserving the
first occurrence and insertion order before adding existingMessages. Add a test
case in wework/src/components/layout/runtimeTranscriptMessages.test.ts lines
17-27 with duplicate transcript IDs and assert only the first entry remains.
This was referenced Jul 28, 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
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