Repository navigation
fix(wework): release inactive pane graphics memory - #2142
Conversation
📝 WalkthroughWalkthroughAdds a macOS concurrent-memory desktop E2E mode for ten simultaneous tasks, deferred mock SSE completion, physical-footprint assertions, and documentation. Cached inactive workbench panes now use ChangesDesktop regression coverage
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 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 |
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
docs/en/wework/developer-guide/wework-e2e-automation.md (1)
111-112: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the
WEWORK_E2E_CONCURRENT_MEMORY_MAX_PHYSICAL_FOOTPRINT_KIBoverride.Unlike the memory-scenario paragraph above, this paragraph doesn't mention that the 800 MiB peak gate is configurable via
WEWORK_E2E_CONCURRENT_MEMORY_MAX_PHYSICAL_FOOTPRINT_KIB(defined intask-flow.e2e.mjs).🤖 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 `@docs/en/wework/developer-guide/wework-e2e-automation.md` around lines 111 - 112, Update the concurrent-memory scenario paragraph to document that its 800 MiB peak gate can be overridden with WEWORK_E2E_CONCURRENT_MEMORY_MAX_PHYSICAL_FOOTPRINT_KIB, as defined in task-flow.e2e.mjs.docs/zh/wework/developer-guide/wework-e2e-automation.md (1)
111-112: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the physical-footprint gate override env var.
Same gap as the English doc: the 800 MiB gate is configurable via
WEWORK_E2E_CONCURRENT_MEMORY_MAX_PHYSICAL_FOOTPRINT_KIB, but that isn't mentioned here (unlike the memory-scenario paragraph above it, which documents its two override vars).🤖 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 `@docs/zh/wework/developer-guide/wework-e2e-automation.md` around lines 111 - 112, Update the concurrent-memory scenario paragraph to document the WEWORK_E2E_CONCURRENT_MEMORY_MAX_PHYSICAL_FOOTPRINT_KIB environment variable as the override for the 800 MiB physical-footprint gate, matching the corresponding English documentation and nearby memory-scenario override format.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@wework/e2e/desktop/task-flow.e2e.mjs`:
- Around line 2499-2505: The concurrent_memory assertion in the scenario request
handling must validate the complete task marker rather than a prefix. Replace
the plain JSON.stringify(body).includes check with a boundary-safe match that
distinguishes requestNumber 1 from markers such as request 10, while preserving
the existing failure message and request recording flow.
- Around line 420-433: Update the concurrent memory task loop to use the
existing waitForSnapshot helper after awaitScenarioRequestCount, polling until
the sidebar contains the expected number of runtime-local-task-row entries.
Immediately assert that the expected new row is present and append it to
taskRows, rather than silently skipping when rendering lags; preserve the
prompt-to-row ordering for each iteration and provide a clear per-iteration
failure message.
---
Nitpick comments:
In `@docs/en/wework/developer-guide/wework-e2e-automation.md`:
- Around line 111-112: Update the concurrent-memory scenario paragraph to
document that its 800 MiB peak gate can be overridden with
WEWORK_E2E_CONCURRENT_MEMORY_MAX_PHYSICAL_FOOTPRINT_KIB, as defined in
task-flow.e2e.mjs.
In `@docs/zh/wework/developer-guide/wework-e2e-automation.md`:
- Around line 111-112: Update the concurrent-memory scenario paragraph to
document the WEWORK_E2E_CONCURRENT_MEMORY_MAX_PHYSICAL_FOOTPRINT_KIB environment
variable as the override for the 800 MiB physical-footprint gate, matching the
corresponding English documentation and nearby memory-scenario override format.
🪄 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: 9b68990a-e57c-4d53-a20c-53e6c0ce1402
📒 Files selected for processing (6)
docs/en/wework/developer-guide/wework-e2e-automation.mddocs/zh/wework/developer-guide/wework-e2e-automation.mdwework/e2e/desktop/task-flow.e2e.mjswework/package.jsonwework/src/components/layout/workbenchPaneStack.test.tsxwework/src/components/layout/workbenchPaneStack.tsx
| for (let index = 1; index <= CONCURRENT_MEMORY_TASK_COUNT; index += 1) { | ||
| if (index > 1) { | ||
| await control.command('click', '[data-testid="new-chat-button"]') | ||
| await control.command('waitFor', composerSelector, { timeoutMs: UI_TIMEOUT_MS }) | ||
| } | ||
| const prompt = `WEWORK_DESKTOP_E2E_CONCURRENT_MEMORY_${index}` | ||
| await control.command('fill', composerSelector, { value: prompt }) | ||
| await control.command('press', composerSelector, { key: 'Enter' }) | ||
| await control.awaitScenarioRequestCount('concurrent_memory', index) | ||
| const snapshot = JSON.parse(await control.command('snapshot', 'body')) | ||
| const rows = snapshot.testIds.filter(testId => testId.startsWith('runtime-local-task-row-')) | ||
| const nextRow = rows.find(row => !taskRows.includes(row)) | ||
| if (nextRow) taskRows.push(nextRow) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Task-row-to-prompt mapping can desync if the sidebar row renders after the request-count signal.
Each iteration takes a single immediate snapshot right after awaitScenarioRequestCount, and silently no-ops if the new row isn't visible yet (if (nextRow) taskRows.push(...)). If rendering lags on any iteration, a later snapshot can expose two new rows at once, and rows.find only captures one — permanently misaligning taskRows[i] with prompt i+1. The end-of-loop count assertion (line ~445-449) only catches a missing row, not a mis-attributed one, so the later navigation assertions (lines 468-480) would fail flakily against the wrong task text with no clear root-cause signal.
Poll deterministically for the expected row count (reusing the existing waitForSnapshot helper used elsewhere in this file) and assert immediately per iteration for a clear failure message.
🐛 Proposed fix
await control.awaitScenarioRequestCount('concurrent_memory', index)
- const snapshot = JSON.parse(await control.command('snapshot', 'body'))
- const rows = snapshot.testIds.filter(testId => testId.startsWith('runtime-local-task-row-'))
- const nextRow = rows.find(row => !taskRows.includes(row))
- if (nextRow) taskRows.push(nextRow)
+ const rowsSnapshot = await waitForSnapshot(
+ control,
+ snap =>
+ snap.testIds.filter(testId => testId.startsWith('runtime-local-task-row-')).length >=
+ index,
+ `Task ${index} did not appear in the sidebar`
+ )
+ const rows = rowsSnapshot.testIds.filter(testId => testId.startsWith('runtime-local-task-row-'))
+ const nextRow = rows.find(row => !taskRows.includes(row))
+ assert.ok(nextRow, `Task ${index} did not create a new sidebar row`)
+ taskRows.push(nextRow)
}📝 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.
| for (let index = 1; index <= CONCURRENT_MEMORY_TASK_COUNT; index += 1) { | |
| if (index > 1) { | |
| await control.command('click', '[data-testid="new-chat-button"]') | |
| await control.command('waitFor', composerSelector, { timeoutMs: UI_TIMEOUT_MS }) | |
| } | |
| const prompt = `WEWORK_DESKTOP_E2E_CONCURRENT_MEMORY_${index}` | |
| await control.command('fill', composerSelector, { value: prompt }) | |
| await control.command('press', composerSelector, { key: 'Enter' }) | |
| await control.awaitScenarioRequestCount('concurrent_memory', index) | |
| const snapshot = JSON.parse(await control.command('snapshot', 'body')) | |
| const rows = snapshot.testIds.filter(testId => testId.startsWith('runtime-local-task-row-')) | |
| const nextRow = rows.find(row => !taskRows.includes(row)) | |
| if (nextRow) taskRows.push(nextRow) | |
| } | |
| for (let index = 1; index <= CONCURRENT_MEMORY_TASK_COUNT; index += 1) { | |
| if (index > 1) { | |
| await control.command('click', '[data-testid="new-chat-button"]') | |
| await control.command('waitFor', composerSelector, { timeoutMs: UI_TIMEOUT_MS }) | |
| } | |
| const prompt = `WEWORK_DESKTOP_E2E_CONCURRENT_MEMORY_${index}` | |
| await control.command('fill', composerSelector, { value: prompt }) | |
| await control.command('press', composerSelector, { key: 'Enter' }) | |
| await control.awaitScenarioRequestCount('concurrent_memory', index) | |
| const rowsSnapshot = await waitForSnapshot( | |
| control, | |
| snap => | |
| snap.testIds.filter(testId => testId.startsWith('runtime-local-task-row-')).length >= | |
| index, | |
| `Task ${index} did not appear in the sidebar` | |
| ) | |
| const rows = rowsSnapshot.testIds.filter(testId => testId.startsWith('runtime-local-task-row-')) | |
| const nextRow = rows.find(row => !taskRows.includes(row)) | |
| assert.ok(nextRow, `Task ${index} did not create a new sidebar row`) | |
| taskRows.push(nextRow) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@wework/e2e/desktop/task-flow.e2e.mjs` around lines 420 - 433, Update the
concurrent memory task loop to use the existing waitForSnapshot helper after
awaitScenarioRequestCount, polling until the sidebar contains the expected
number of runtime-local-task-row entries. Immediately assert that the expected
new row is present and append it to taskRows, rather than silently skipping when
rendering lags; preserve the prompt-to-row ordering for each iteration and
provide a clear per-iteration failure message.
| if (this.scenario === 'concurrent_memory') { | ||
| const requestNumber = (this.scenarioRequests.get('concurrent_memory')?.length ?? 0) + 1 | ||
| this.recordScenarioRequest('concurrent_memory', modelRequest) | ||
| assert.ok( | ||
| JSON.stringify(body).includes(`WEWORK_DESKTOP_E2E_CONCURRENT_MEMORY_${requestNumber}`), | ||
| `Concurrent memory request ${requestNumber} did not contain its UI prompt` | ||
| ) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Substring check for the task marker isn't boundary-safe (..._1 is a prefix of ..._10).
JSON.stringify(body).includes(\WEWORK_DESKTOP_E2E_CONCURRENT_MEMORY_${requestNumber}`)uses a plain substring match. ForrequestNumber = 1, the marker text is a literal prefix of the marker for task 10, so this specific assertion would still pass even if request #1` actually carried task 10's prompt — weakening exactly the validation meant to catch request/prompt mis-routing.
🐛 Proposed fix
- assert.ok(
- JSON.stringify(body).includes(`WEWORK_DESKTOP_E2E_CONCURRENT_MEMORY_${requestNumber}`),
- `Concurrent memory request ${requestNumber} did not contain its UI prompt`
- )
+ assert.match(
+ JSON.stringify(body),
+ new RegExp(`WEWORK_DESKTOP_E2E_CONCURRENT_MEMORY_${requestNumber}(?!\\d)`),
+ `Concurrent memory request ${requestNumber} did not contain its UI prompt`
+ )📝 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.
| if (this.scenario === 'concurrent_memory') { | |
| const requestNumber = (this.scenarioRequests.get('concurrent_memory')?.length ?? 0) + 1 | |
| this.recordScenarioRequest('concurrent_memory', modelRequest) | |
| assert.ok( | |
| JSON.stringify(body).includes(`WEWORK_DESKTOP_E2E_CONCURRENT_MEMORY_${requestNumber}`), | |
| `Concurrent memory request ${requestNumber} did not contain its UI prompt` | |
| ) | |
| if (this.scenario === 'concurrent_memory') { | |
| const requestNumber = (this.scenarioRequests.get('concurrent_memory')?.length ?? 0) + 1 | |
| this.recordScenarioRequest('concurrent_memory', modelRequest) | |
| assert.match( | |
| JSON.stringify(body), | |
| new RegExp(`WEWORK_DESKTOP_E2E_CONCURRENT_MEMORY_${requestNumber}(?!\\d)`), | |
| `Concurrent memory request ${requestNumber} did not contain its UI prompt` | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@wework/e2e/desktop/task-flow.e2e.mjs` around lines 2499 - 2505, The
concurrent_memory assertion in the scenario request handling must validate the
complete task marker rather than a prefix. Replace the plain
JSON.stringify(body).includes check with a boundary-safe match that
distinguishes requestNumber 1 from markers such as request 10, while preserving
the existing failure message and request recording flow.
Summary
Root cause
Inactive cached panes used
visibility: hidden. WebKit continued retaining their rendering layers and backing stores, so graphics memory accumulated as more task panes were opened. Using the nativehiddenattribute moves inactive panes todisplay: none, releasing layout/compositor resources without unmounting React state.Verification
mainEvidence
The E2E writes per-process-group samples and its peak record to
concurrent-memory.json. The measured group includes Wework main, WebKit Web Content/GPU/Networking, all Executor processes, Codex app-server, and child processes.Summary by CodeRabbit
Bug Fixes
Documentation
Tests