Repository navigation
fix(wework): stabilize cloud e2e shards - #2643
Conversation
📝 WalkthroughWalkthroughThe PR replaces modulo-based cloud E2E sharding with explicit balanced groups. It filters removed runtime workspaces during refresh, settles stopped local harness snapshots, and prevents duplicate embedded-browser navigation. ChangesCloud E2E shard balancing
Workbench runtime reconciliation
Embedded-browser navigation
Estimated code review effort: 3 (Moderate) | ~30 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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 |
qdaxb
changed the title
9d88854 to
17bc2cd
Compare
![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
🤖 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/features/workbench/runtimeTaskLifecycle/reducer.ts`:
- Around line 13-22: Update the stale-snapshot settlement logic around
shouldIgnoreStaleSnapshot and the following executionPhase update so
state.goalStatus === 'active' cannot be forced from running to idle when the
local harness reports running: false. Preserve the active Goal execution phase
and pause control consistently with the existing turn_settled branch, or exclude
active Goal turns from this settlement bypass. Add a regression case covering
goalStatus: 'active' during the inter-turn gap.
🪄 Autofix
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: 09135230-8651-4c7d-b635-918157a8d16f
📒 Files selected for processing (6)
.github/scripts/classify-wework-desktop-e2e.sh.github/scripts/test-classify-ci-changes.shwework/src/features/workbench/WorkbenchProvider.test.tsxwework/src/features/workbench/runtimeTaskLifecycle/RuntimeTaskLifecycleStore.test.tswework/src/features/workbench/runtimeTaskLifecycle/reducer.tswework/src/features/workbench/useWorkbenchDataRefresh.ts
| const settledLocalHarness = | ||
| snapshotRunning === false && | ||
| state.turnPhase !== 'streaming' && | ||
| isLocalHarnessRuntime(event.task.runtime) | ||
| const shouldIgnoreStaleSnapshot = | ||
| snapshotRunning !== null && | ||
| expectedRunning !== null && | ||
| snapshotRunning !== expectedRunning && | ||
| (!terminalStatus || hasIdentifiedActiveTurn) | ||
| (!terminalStatus || hasIdentifiedActiveTurn) && | ||
| !settledLocalHarness |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Preserve active-Goal execution state during settlement.
When state.goalStatus === 'active', the existing turn_settled branch keeps executionPhase running while the turn becomes idle. This new bypass also matches that state, but Lines 25-31 then force executionPhase to idle when the local harness reports running: false.
The workbench can lose the active Goal pause state during its inter-turn gap. Preserve the active-Goal execution phase, or restrict this settlement path to non-goal turns. Add a regression case with goalStatus: 'active'.
Based on learnings, the active Goal inter-turn gap must keep the pause control visible. The existing turn_settled branch in this file also preserves executionPhase for an active Goal.
🤖 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/runtimeTaskLifecycle/reducer.ts` around lines
13 - 22, Update the stale-snapshot settlement logic around
shouldIgnoreStaleSnapshot and the following executionPhase update so
state.goalStatus === 'active' cannot be forced from running to idle when the
local harness reports running: false. Preserve the active Goal execution phase
and pause control consistently with the existing turn_settled branch, or exclude
active Goal turns from this settlement bypass. Add a regression case covering
goalStatus: 'active' during the inter-turn gap.
Source: Learnings
7e7c4bd to
1515ecd
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
wework/src-tauri/src/embedded_browser/tests.rs (1)
409-420: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftCover the caller-level navigation behavior.
This test validates
browser_open_action_requires_navigationonly. It does not open a tab, inspect the URL, or verify the number of navigation calls. A regression inhandle_bridge_requestcould still pass. Add caller-level coverage, or confirm that the separate real-Tauri checkpoint verifies one navigation for a fast local URL.As per coding guidelines, “Any Wework UI, Tauri command, local-runtime, IPC, or desktop integration behavior change requires isolated real-Tauri verification in addition to unit and E2E tests; browser-only or mocked verification is insufficient.”
The PR objective lists a real-Tauri browser checkpoint. Confirm that it executes this route.🤖 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-tauri/src/embedded_browser/tests.rs` around lines 409 - 420, Extend coverage beyond browser_open_action_requires_navigation by exercising handle_bridge_request with a newly created tab, asserting it uses the creation URL and performs exactly one navigation; alternatively, confirm the isolated real-Tauri browser checkpoint executes this fast local-URL route and records that single navigation.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.
Nitpick comments:
In `@wework/src-tauri/src/embedded_browser/tests.rs`:
- Around line 409-420: Extend coverage beyond
browser_open_action_requires_navigation by exercising handle_bridge_request with
a newly created tab, asserting it uses the creation URL and performs exactly one
navigation; alternatively, confirm the isolated real-Tauri browser checkpoint
executes this fast local-URL route and records that single navigation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 7e603104-5a11-494f-8b1b-2e96499872de
📒 Files selected for processing (6)
wework/src-tauri/src/embedded_browser.rswework/src-tauri/src/embedded_browser/tests.rswework/src/features/workbench/WorkbenchProvider.test.tsxwework/src/features/workbench/runtimeTaskLifecycle/RuntimeTaskLifecycleStore.test.tswework/src/features/workbench/runtimeTaskLifecycle/reducer.tswework/src/features/workbench/useWorkbenchDataRefresh.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- wework/src/features/workbench/WorkbenchProvider.test.tsx
- wework/src/features/workbench/useWorkbenchDataRefresh.ts
- wework/src/features/workbench/runtimeTaskLifecycle/RuntimeTaskLifecycleStore.test.ts
- wework/src/features/workbench/runtimeTaskLifecycle/reducer.ts
What changed
deviceId + workspacePath, even when a stale cloud response assigns a different project idWhy
The old round-robin layout produced uneven Cloud E2E runtimes. The first merge-queue run then exposed a real product race: project removal completed, a replacement project was created, and an in-flight cloud refresh restored the deleted workspace under another project identity. The previous tombstone filter only matched project id, which is not stable across refreshes.
A later CI run exposed a second product race in
local-harness: the local process completed, the assistant reply was persisted and rendered, and the executor reportedrunning=false, but the UI stayed in the awaiting state after missing the terminal frontend event. Local harness snapshots are authoritative after send acceptance, so they now close a non-streaming turn. Active streaming turns and Codex optimistic sends retain the existing stale-snapshot protection.The next CI run exposed a built-in browser race introduced by navigation-completion waiting: opening a new browser tab navigated to the requested URL during native webview creation, then immediately navigated to the same URL again. On Linux the duplicate navigation could suppress the second tab’s finished-load callback even though the fixture request completed. New tabs now use their creation URL as the single navigation; existing tabs still navigate explicitly.
Impact
All 15 Cloud desktop checkpoints still run exactly once across five shards. Removed cloud projects remain removed while stale refreshes converge, and completed local harness turns no longer remain stuck on “thinking”, and newly created built-in browser tabs no longer receive duplicate navigation. No E2E scenario was weakened or skipped.
Validation
pnpm --dir wework test src/features/workbench/runtimeTaskLifecycle/RuntimeTaskLifecycleStore.test.ts src/features/workbench/WorkbenchProvider.test.tsx(196 passed)pnpm --dir wework typecheckbash .github/scripts/test-classify-ci-changes.shgit diff --checkcargo test --manifest-path wework/src-tauri/Cargo.toml embedded_browser::tests::(34 passed)browser-multi-tabscheckpoint passed in 2m54s with no assertion errorsSummary by CodeRabbit