Repository navigation
refactor(wework): isolate desktop control extension boundary - #2082
Conversation
Move desktop-specific control commands and workspace actions behind an extension contract so the public automation runner and workspace panels carry no concrete product protocol. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
📝 WalkthroughWalkthroughThe PR adds optional product desktop scenarios to the E2E runner, delegates unsupported desktop controls through an extension contract, replaces the workspace desktop launcher with a typed cloud desktop action, and extracts a reusable stable-event hook with related workbench tests. ChangesDesktop extension boundaries
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant DesktopRunner
participant ScenarioLoader
participant DesktopE2EServer
participant ProductScenario
DesktopRunner->>ScenarioLoader: Load configured scenario module
ScenarioLoader-->>DesktopRunner: Return desktop scenario
DesktopRunner->>DesktopE2EServer: Attach scenario
DesktopE2EServer->>ProductScenario: Delegate HTTP handling
DesktopRunner->>ProductScenario: Run scenario verification
ProductScenario-->>DesktopRunner: Return diagnostics
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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
ESLint install timed out. The project may have too many dependencies for 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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5c04980a41
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (desktopScenario) { | ||
| phase = 'desktop-extension-scenario' | ||
| await desktopScenario.verify(control) |
There was a problem hiding this comment.
When this runner is invoked with the existing --cloud-only mode, it returns from the cloud-project branch before reaching this new scenario invocation, so setting WEWORK_E2E_DESKTOP_SCENARIO_MODULE silently never runs the product scenario (and WEWORK_E2E_DESKTOP_SCENARIO_ONLY=true is likewise ignored after passing the module-presence check). This prevents product desktop scenarios that require the real cloud environment from being exercised by e2e:desktop:cloud; invoke the scenario before that return or explicitly reject the unsupported combination.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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)
wework/src/components/layout/workspace-panels/WorkspacePanelCards.test.tsx (1)
392-414: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRestore the extension launch rejection coverage The new
WorkspacePanelCards.test.tsxcase only covers the happy path, andwework/src/extensions/cloud-desktop.test.tsxstill only exercises the unavailable stub. Add a regression test for the rejection/retry path so this failure mode stays covered.🤖 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/workspace-panels/WorkspacePanelCards.test.tsx` around lines 392 - 414, Extend the cloud desktop workspace coverage around WorkspacePanelCards and the cloud-desktop extension to exercise when the extension launch rejects, then verify the retry path succeeds and the close behavior remains correct. Add the regression test alongside the existing “renders the project desktop through the cloud desktop workspace extension” case, and cover the real rejection flow rather than only the unavailable stub.Source: Path instructions
🧹 Nitpick comments (1)
wework/src/components/layout/workspace-panels/WorkspacePanelCards.test.tsx (1)
20-29: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for
handleExtensionBusyChangebusy-state wiring.The mock's
onClickcallsonOpeneddirectly, so no test currently exercises the extension reportingbusy=true, which is what should disable other workspace-tool cards while the cloud desktop action is in flight. Consider extending the mock to also exposeonBusyChangeand asserting that other cards become disabled while busy.🤖 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/workspace-panels/WorkspacePanelCards.test.tsx` around lines 20 - 29, Extend the WorkspaceAction mock and its tests around handleExtensionBusyChange to expose and invoke onBusyChange(true), then assert that the other workspace-tool cards become disabled while the cloud desktop action is busy. Preserve the existing onOpened behavior and verify the cards return to their normal enabled state when busy ends.
🤖 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`:
- Line 1079: Await both optional desktop scenario hooks: update the cleanup call
around desktopScenario.close and the failure diagnostics construction around
desktopScenario.diagnostics to use optional awaiting. Ensure asynchronous
diagnostics are resolved before serialization, and preserve the original test
error when cleanup in main’s finally block also fails by preventing the cleanup
rejection from replacing the already-propagating failure.
---
Outside diff comments:
In `@wework/src/components/layout/workspace-panels/WorkspacePanelCards.test.tsx`:
- Around line 392-414: Extend the cloud desktop workspace coverage around
WorkspacePanelCards and the cloud-desktop extension to exercise when the
extension launch rejects, then verify the retry path succeeds and the close
behavior remains correct. Add the regression test alongside the existing
“renders the project desktop through the cloud desktop workspace extension”
case, and cover the real rejection flow rather than only the unavailable stub.
---
Nitpick comments:
In `@wework/src/components/layout/workspace-panels/WorkspacePanelCards.test.tsx`:
- Around line 20-29: Extend the WorkspaceAction mock and its tests around
handleExtensionBusyChange to expose and invoke onBusyChange(true), then assert
that the other workspace-tool cards become disabled while the cloud desktop
action is busy. Preserve the existing onOpened behavior and verify the cards
return to their normal enabled state when busy ends.
🪄 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: 58ea2995-5ad1-46bd-8542-4f59e30c8f76
📒 Files selected for processing (16)
docs/en/developer-guide/wework-e2e-automation.mddocs/en/developer-guide/wework-embedded-browser.mddocs/zh/developer-guide/wework-e2e-automation.mddocs/zh/developer-guide/wework-embedded-browser.mdwework/e2e/desktop/scenario-loader.mjswework/e2e/desktop/task-flow.e2e.mjswework/src/components/layout/workspace-panels/WorkspacePanelCards.test.tsxwework/src/components/layout/workspace-panels/WorkspacePanelCards.tsxwework/src/e2e/automation.tswework/src/e2e/desktop-scenario-loader.test.tswework/src/extensions/cloud-desktop-contract.tswework/src/extensions/cloud-desktop.test.tsxwework/src/extensions/cloud-desktop.tsxwework/src/extensions/desktop-control-contract.tswework/src/extensions/desktop-control.test.tswework/src/extensions/desktop-control.ts
| async close() { | ||
| for (const response of this.blockedCloudResponses) response.destroy() | ||
| this.blockedCloudResponses.clear() | ||
| this.desktopScenario?.close?.() |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Unawaited optional scenario hooks can mask failures or drop diagnostics.
this.desktopScenario?.close?.() (Line 1079) isn't awaited, unlike handleHttp which is (Line 1258). If close() rejects, control.close()'s returned promise rejects too; since this runs in the finally block of main(), that would replace the original test failure that was about to propagate via throw error — the real cause becomes invisible in CI.
Separately, desktopScenario?.diagnostics?.() (Line 2580) is invoked without awaiting inside the failure diagnostics payload. If a scenario author implements diagnostics() asynchronously, the stored value will be a Promise object, which JSON.stringify serializes as {}, silently losing scenario diagnostics exactly when they're most needed (on failure).
🔧 Proposed fix
async close() {
for (const response of this.blockedCloudResponses) response.destroy()
this.blockedCloudResponses.clear()
- this.desktopScenario?.close?.()
+ try {
+ await this.desktopScenario?.close?.()
+ } catch (error) {
+ console.error('Desktop scenario close() failed:', error)
+ }
this.server.closeAllConnections?.()- desktopScenario: desktopScenario?.diagnostics?.() ?? null,
+ desktopScenario: (await desktopScenario?.diagnostics?.()) ?? null,Also applies to: 2580-2580
🤖 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` at line 1079, Await both optional
desktop scenario hooks: update the cleanup call around desktopScenario.close and
the failure diagnostics construction around desktopScenario.diagnostics to use
optional awaiting. Ensure asynchronous diagnostics are resolved before
serialization, and preserve the original test error when cleanup in main’s
finally block also fails by preventing the cleanup rejection from replacing the
already-propagating failure.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
wework/src/features/workbench/useStableEvent.ts (1)
3-13: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider using the native
useEffectEventhook.Since this project uses React 19.2, you might be able to utilize the newly added native
useEffectEventhook instead of maintaining a customuseStableEventimplementation. This built-in hook is specifically designed to provide stable event handler identities natively. Based on learnings, React 19.2 addsuseEffectEvent()for stable event handler identities.♻️ Proposed refactor to use native API
-import { useCallback, useLayoutEffect, useRef } from 'react' +import { useEffectEvent } from 'react' -export function useStableEvent<TArgs extends unknown[], TResult>( - handler: (...args: TArgs) => TResult -): (...args: TArgs) => TResult { - const handlerRef = useRef(handler) - - useLayoutEffect(() => { - handlerRef.current = handler - }, [handler]) - - return useCallback((...args: TArgs) => handlerRef.current(...args), []) -} +export const useStableEvent = useEffectEvent🤖 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/workbench/useStableEvent.ts` around lines 3 - 13, Replace the custom useStableEvent implementation’s useRef, useLayoutEffect, and useCallback logic with React 19.2’s native useEffectEvent hook, while preserving the existing handler signature and stable callback behavior.
🤖 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/process-lifecycle.mjs`:
- Around line 11-29: Update waitForProcessExit to return immediately when the
ChildProcess has no pid, alongside its existing exited-process checks. Preserve
the current exit-event waiting and timeout behavior for processes with a valid
pid.
In `@wework/src/features/workbench/WorkbenchProvider.test.tsx`:
- Line 3849: Wrap the final native click on the “send pane input” element in
flushSync, matching the surrounding interactions, so React state updates
triggered by the click are completed within the test’s act boundary.
---
Nitpick comments:
In `@wework/src/features/workbench/useStableEvent.ts`:
- Around line 3-13: Replace the custom useStableEvent implementation’s useRef,
useLayoutEffect, and useCallback logic with React 19.2’s native useEffectEvent
hook, while preserving the existing handler signature and stable callback
behavior.
🪄 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: 00541a14-314d-4c14-892d-4f0e62216122
📒 Files selected for processing (7)
wework/e2e/desktop/process-lifecycle.mjswework/e2e/desktop/task-flow.e2e.mjswework/src/e2e/desktop-process-lifecycle.test.tswework/src/features/workbench/WorkbenchProvider.test.tsxwework/src/features/workbench/WorkbenchProvider.tsxwework/src/features/workbench/useStableEvent.test.tsxwework/src/features/workbench/useStableEvent.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- wework/e2e/desktop/task-flow.e2e.mjs
| function waitForProcessExit(child, timeoutMs) { | ||
| if (child.exitCode !== null || child.signalCode !== null) return Promise.resolve() | ||
| return withTimeout( | ||
| new Promise(resolvePromise => child.once('exit', resolvePromise)), | ||
| timeoutMs, | ||
| `Timed out waiting for process ${child.pid ?? 'unknown'} to exit` | ||
| ) | ||
| } | ||
|
|
||
| export async function stopProcess(child) { | ||
| if (!child || child.exitCode !== null || child.signalCode !== null) return | ||
| child.kill('SIGTERM') | ||
| try { | ||
| await waitForProcessExit(child, PROCESS_STOP_TIMEOUT_MS) | ||
| } catch { | ||
| child.kill('SIGKILL') | ||
| await waitForProcessExit(child, PROCESS_STOP_TIMEOUT_MS) | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Avoid 20-second teardown hangs for processes that failed to spawn.
If a ChildProcess fails to spawn, child.pid will be undefined, and the process will never emit an exit event. Currently, waitForProcessExit will wait for this non-existent event until the timeout expires twice (once for SIGTERM, once for SIGKILL fallback), causing an unnecessary 20-second delay per failed process during test teardown.
Add an early return for missing pid to prevent these hangs and speed up teardown on failure.
⚡ Proposed fix
function waitForProcessExit(child, timeoutMs) {
- if (child.exitCode !== null || child.signalCode !== null) return Promise.resolve()
+ if (child.pid === undefined || child.exitCode !== null || child.signalCode !== null) return Promise.resolve()
return withTimeout(
new Promise(resolvePromise => child.once('exit', resolvePromise)),
timeoutMs,
`Timed out waiting for process ${child.pid ?? 'unknown'} to exit`
)
}
export async function stopProcess(child) {
- if (!child || child.exitCode !== null || child.signalCode !== null) return
+ if (!child || child.pid === undefined || child.exitCode !== null || child.signalCode !== null) return
child.kill('SIGTERM')
try {
await waitForProcessExit(child, PROCESS_STOP_TIMEOUT_MS)
} catch {
child.kill('SIGKILL')
await waitForProcessExit(child, PROCESS_STOP_TIMEOUT_MS)
}
}📝 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.
| function waitForProcessExit(child, timeoutMs) { | |
| if (child.exitCode !== null || child.signalCode !== null) return Promise.resolve() | |
| return withTimeout( | |
| new Promise(resolvePromise => child.once('exit', resolvePromise)), | |
| timeoutMs, | |
| `Timed out waiting for process ${child.pid ?? 'unknown'} to exit` | |
| ) | |
| } | |
| export async function stopProcess(child) { | |
| if (!child || child.exitCode !== null || child.signalCode !== null) return | |
| child.kill('SIGTERM') | |
| try { | |
| await waitForProcessExit(child, PROCESS_STOP_TIMEOUT_MS) | |
| } catch { | |
| child.kill('SIGKILL') | |
| await waitForProcessExit(child, PROCESS_STOP_TIMEOUT_MS) | |
| } | |
| } | |
| function waitForProcessExit(child, timeoutMs) { | |
| if (child.pid === undefined || child.exitCode !== null || child.signalCode !== null) return Promise.resolve() | |
| return withTimeout( | |
| new Promise(resolvePromise => child.once('exit', resolvePromise)), | |
| timeoutMs, | |
| `Timed out waiting for process ${child.pid ?? 'unknown'} to exit` | |
| ) | |
| } | |
| export async function stopProcess(child) { | |
| if (!child || child.pid === undefined || child.exitCode !== null || child.signalCode !== null) return | |
| child.kill('SIGTERM') | |
| try { | |
| await waitForProcessExit(child, PROCESS_STOP_TIMEOUT_MS) | |
| } catch { | |
| child.kill('SIGKILL') | |
| await waitForProcessExit(child, PROCESS_STOP_TIMEOUT_MS) | |
| } | |
| } |
🤖 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/process-lifecycle.mjs` around lines 11 - 29, Update
waitForProcessExit to return immediately when the ChildProcess has no pid,
alongside its existing exited-process checks. Preserve the current exit-event
waiting and timeout behavior for processes with a valid pid.
| await waitFor(() => expect(screen.getByTestId('runtime-project-count')).toHaveTextContent('1')) | ||
| flushSync(() => screen.getByText('start new project task').click()) | ||
| flushSync(() => screen.getByText('set pane input').click()) | ||
| screen.getByText('send pane input').click() |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Wrap the final click in flushSync to prevent act(...) warnings.
The native .click() on this line is not wrapped in flushSync like the previous interactions. Because native DOM clicks are not automatically wrapped in act() (unlike @testing-library/user-event or fireEvent), any React state updates triggered by this click will result in a "not wrapped in act(...)" console warning and could cause async timing issues in the test.
🐛 Proposed fix
- screen.getByText('send pane input').click()
+ flushSync(() => screen.getByText('send pane input').click())📝 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.
| screen.getByText('send pane input').click() | |
| flushSync(() => screen.getByText('send pane input').click()) |
🤖 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/workbench/WorkbenchProvider.test.tsx` at line 3849, Wrap
the final native click on the “send pane input” element in flushSync, matching
the surrounding interactions, so React state updates triggered by the click are
completed within the test’s act boundary.
Move desktop-specific control commands and workspace actions behind an extension contract so the public automation runner and workspace panels carry no concrete product protocol.
Summary by CodeRabbit