refactor(wework): extract cloud project context - #2510
Conversation
|
Warning Review limit reached
Next review available in: 21 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThe change extracts cloud project, task, delivery, mention, binding, and navigation logic from ChangesWorkbench cloud context refactor
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant DesktopWorkbenchMain
participant useWorkbenchCloudProjectContext
participant CloudProjectApi
participant WorkspaceNavigation
DesktopWorkbenchMain->>useWorkbenchCloudProjectContext: prepareSubmission
useWorkbenchCloudProjectContext->>CloudProjectApi: resolve pending project or task binding
CloudProjectApi-->>useWorkbenchCloudProjectContext: return project and task context
useWorkbenchCloudProjectContext-->>DesktopWorkbenchMain: return submission context
DesktopWorkbenchMain->>useWorkbenchCloudProjectContext: openDelivery
useWorkbenchCloudProjectContext->>WorkspaceNavigation: navigate to todo view
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (9)
wework/src/components/layout/useWorkbenchCloudProjectContext.ts (5)
246-246: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReuse the memoized
todoBindingApisinstead of recomputingprojectSpaceApis(services).Line 170 already memoizes
projectSpaceApis(services). Lines 246, 439, and 652 call the same helper again with the same input. Reusing the memo removes the duplication and keeps one source of the API list.As per coding guidelines: "Before adding code, search for and reuse existing components, services, utilities, and patterns; extract shared logic instead of duplicating it."
Also applies to: 439-439
🤖 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/useWorkbenchCloudProjectContext.ts` at line 246, Update the API assignments at the referenced call sites in the workbench context to reuse the memoized `todoBindingApis` value created earlier, removing repeated `projectSpaceApis(services)` calls while preserving the existing API-list behavior.Source: Coding guidelines
411-419: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRoute the delivery mention labels through
t.Lines 414 and 416 build the candidate title and the reference chip from a hardcoded Chinese string. Every other label in this effect uses
twith a Chinese default. Add translation keys so the delivery candidates follow the same i18n path.🌐 Proposed fix
...deliveries.items.map(delivery => candidate( `cloud-delivery:${delivery.id}`, - `交付 ${delivery.id.slice(0, 8)}`, + `${t('workbench.mention_cloud_delivery_chip', '交付')} ${delivery.id.slice(0, 8)}`, delivery.delivered_at ?? delivery.created_at, - `[$交付 ${delivery.id.slice(0, 8)}](cloud://projects/${projectId}/deliveries/${delivery.id})`, + `[$${t('workbench.mention_cloud_delivery_chip', '交付')} ${delivery.id.slice(0, 8)}](cloud://projects/${projectId}/deliveries/${delivery.id})`, ['交付', 'delivery', delivery.id] ) ),🤖 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/useWorkbenchCloudProjectContext.ts` around lines 411 - 419, Update the delivery candidate mapping in the surrounding effect to pass both the candidate title and reference-chip label through the existing t function, using Chinese defaults and distinct translation keys for each label. Preserve the delivery ID interpolation and cloud URL unchanged.
643-645: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSimplify the
submissionItemcondition.
submissionProjectisnullwhenevercurrentRuntimeTaskis set, so Line 645 comparesundefinedwithpendingCloudProject?.id. WhencurrentRuntimeTaskis null, the comparison is always true. The expression reduces to a single check oncurrentRuntimeTask.♻️ Proposed fix
const submissionProject = currentRuntimeTask ? null : pendingCloudProject - const submissionItem = - submissionProject?.id === pendingCloudProject?.id ? pendingTodoItem : null + const submissionItem = submissionProject ? pendingTodoItem : null🤖 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/useWorkbenchCloudProjectContext.ts` around lines 643 - 645, In the submission setup, simplify the `submissionItem` assignment to depend only on whether `currentRuntimeTask` is absent, since `submissionProject` is derived from that same condition. Preserve `pendingTodoItem` when no runtime task exists and return `null` otherwise; update the expression near `submissionProject` without changing surrounding behavior.
448-469: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the unreachable
catchafterPromise.allSettled.
Promise.allSettlednever rejects. The handler at Lines 467-469 cannot run. Individual failures are already dropped by theresult.status === 'fulfilled'filter at Line 457.As per coding guidelines: "Delete dead code and do not add compatibility shims or fallback paths without agreement; correct the primary path."
♻️ Proposed fix
- void Promise.allSettled( + void Promise.allSettled( apis.map(async api => { const result = await api.listCloudProjects() return result.items }) - ) - .then(results => { - if (!active) return - const candidates = results.flatMap(result => - result.status === 'fulfilled' ? result.value : [] - ) - const uniqueProjects = candidates.filter( - (candidate, index) => - candidates.findIndex( - other => other.id === candidate.id && other.project_store === candidate.project_store - ) === index - ) - setCloudProjects(uniqueProjects) - }) - .catch(() => { - if (active) setCloudProjects([]) - }) + ).then(results => { + if (!active) return + const candidates = results.flatMap(result => + result.status === 'fulfilled' ? result.value : [] + ) + const uniqueProjects = candidates.filter( + (candidate, index) => + candidates.findIndex( + other => other.id === candidate.id && other.project_store === candidate.project_store + ) === index + ) + setCloudProjects(uniqueProjects) + })🤖 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/useWorkbenchCloudProjectContext.ts` around lines 448 - 469, Remove the unreachable catch handler from the Promise.allSettled chain in the cloud-project loading flow, preserving the existing fulfilled-result filtering and active-state guard in the then callback.Source: Coding guidelines
159-167: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffConsider splitting the hook along the layers it already contains.
useWorkbenchCloudProjectContextspans about 600 lines and mixes four concerns: bound-context loading, pending binding, mention candidate loading, and UI action handlers. The extraction fromDesktopWorkbenchMainis a clear improvement. A follow-up split into smaller hooks, for exampleuseCloudMentionCandidatesanduseCloudProjectBinding, would keep each unit focused.As per coding guidelines: "Comments must be in English, names must be clear, and functions should remain focused, preferably under 50 lines" and "Favor cohesive modules, explicit interfaces, and standard practices".
🤖 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/useWorkbenchCloudProjectContext.ts` around lines 159 - 167, Split useWorkbenchCloudProjectContext into focused hooks aligned with its existing responsibilities: extract cloud project binding/bound-context loading and mention candidate loading into cohesive hooks such as useCloudProjectBinding and useCloudMentionCandidates, while keeping UI action handlers separate. Preserve the current public context API and behavior, and use clear English names and explicit interfaces for the extracted hooks.Source: Coding guidelines
wework/src/components/layout/useWorkbenchCloudProjectContext.test.tsx (3)
106-120: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert that the delivery-scoped list APIs are not called during default-project selection.
The PR states that project-space loading no longer triggers duplicate project-list requests. Line 120 checks
listCloudProjectsonce, which covers half of that claim. Add assertions forlistCloudFilesandlistLoopItemscall counts so a future regression that re-fetches per project space fails this test.💚 Proposed addition
expect(deliveryApi.listCloudProjects).toHaveBeenCalledOnce() + expect(deliveryApi.listCloudFiles).toHaveBeenCalledOnce() + expect(deliveryApi.listLoopItems).toHaveBeenCalledOnce()Adjust the expected counts to the intended behavior after the pending project is selected.
🤖 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/useWorkbenchCloudProjectContext.test.tsx` around lines 106 - 120, Extend the default-project selection test around useWorkbenchCloudProjectContext to assert the delivery-scoped list APIs listCloudFiles and listLoopItems are each called exactly once, alongside the existing listCloudProjects assertion, preserving the intended no-duplicate-request behavior.
68-89: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for
onRuntimeTaskCreated.The test verifies
cloudProjectIdandadditionalContext. It does not exercise theonRuntimeTaskCreatedcallback returned byprepareSubmission, which records the runtime target onpendingTodoBindingand drives the binding effect. That callback is the mechanism that connects a pending cloud project to a newly created runtime task. Adding a case that invokes it and then re-renders with acurrentRuntimeTaskwould cover the main binding path.🤖 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/useWorkbenchCloudProjectContext.test.tsx` around lines 68 - 89, Add coverage in the pending-project test around prepareSubmission and its onRuntimeTaskCreated callback: invoke the callback with a runtime task, re-render the context with that task as currentRuntimeTask, and assert pendingTodoBinding records the runtime target. Preserve the existing cloudProjectId, additionalContext, and pending-context assertions.
88-88: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMove the pending-context cleanup into
afterEach.
useWorkbenchCloudProjectContext.tskeepspendingTodoBindingin module scope (Line 56). The cleanup calls at Lines 88 and 122 run only when every preceding assertion passes. If an assertion fails, the module state survives into the next test and produces a second, unrelated failure. AnafterEachhook makes the reset unconditional.💚 Proposed fix
describe('useWorkbenchCloudProjectContext', () => { + afterEach(() => { + const { result, unmount } = renderCloudContext() + act(() => result.current.clearPendingProjectContext()) + unmount() + }) + test('prepares a pending backend project and task for runtime creation', async () => {Import
afterEachfromvitest. A simpler alternative is to export a reset helper from the hook module for test use.Also applies to: 122-122
🤖 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/useWorkbenchCloudProjectContext.test.tsx` at line 88, Move the clearPendingProjectContext cleanup out of individual test bodies and into an unconditional afterEach hook in the test suite. Import afterEach from vitest and invoke result.current.clearPendingProjectContext() through the shared cleanup so module-scoped pendingTodoBinding is reset even when assertions fail.wework/src/components/layout/DesktopWorkbenchMain.tsx (1)
515-549: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDestructure the hook result directly and let the hook own
composerCloudProject.Two small points:
cloudProjectContextat Line 515 is only used by the destructuring at Line 524. Destructure the call result directly.- Line 549 recomputes
currentRuntimeTask ? boundCloudProject : pendingCloudProject. The hook already derives the same value internally (composerCloudProject). Export it from the hook so the rule lives in one place.♻️ Proposed fix
- const cloudProjectContext = useWorkbenchCloudProjectContext({ + const { + activeDeliveryItem, + boundCloudItem, + boundCloudProject, + clearCloudActionNotice, + clearPendingProjectContext, + clearTodoBindingError, + closeDeliveryDialog, + closeTodoBindingPicker, + cloudActionNotice, + composerCloudProject, + cloudProjectMentionCandidates, + deliveryDialogOpen, + finishLocalDelivery, + handleSelectCloudProject, + handleTodoBound, + openDelivery, + openTodoManager, + pendingCloudProject, + pendingTodoItem, + prepareSubmission, + todoBindingApis, + todoBindingError, + todoBindingPickerOpen, + visibleCloudMentionCandidates, + } = useWorkbenchCloudProjectContext({ currentRuntimeTask, currentProjectId: currentProject?.id, defaultProjectSpace, paneKey, runtimeTaskTitle, services, userId: state.user?.id, }) - const { - ... - } = cloudProjectContext - const composerCloudProject = currentRuntimeTask ? boundCloudProject : pendingCloudProjectAdd
composerCloudProjectto the hook return object inuseWorkbenchCloudProjectContext.ts.As per coding guidelines: "Before adding code, search for and reuse existing components, services, utilities, and patterns; extract shared logic instead of duplicating it."
🤖 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 515 - 549, Destructure the result of useWorkbenchCloudProjectContext directly instead of assigning cloudProjectContext first. Export composerCloudProject from the hook’s return object in useWorkbenchCloudProjectContext.ts, then consume that returned value in DesktopWorkbenchMain rather than recomputing it from currentRuntimeTask, boundCloudProject, and pendingCloudProject.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 `@wework/src/components/layout/useWorkbenchCloudProjectContext.ts`:
- Around line 270-282: Add a rejection handler to the hydrateLocalWorkItems
promise in the local work-item path, alongside its existing success callback.
When hydration fails and the component remains active, clear the stale bound
cloud item and delivery item consistently with the cloud-path error handling,
while preserving the current success behavior.
- Around line 739-742: Wrap the returned clear and close callbacks in
useCallback within the context hook: clearCloudActionNotice,
clearTodoBindingError, and closeDeliveryDialog should have stable identities
with appropriate dependencies. Preserve their existing state-reset behavior so
TransientNotice can complete its timeout and DeliveryDialog can benefit from
memoization.
- Around line 349-359: The cloud mention loading flow should use the
project-space-specific API instead of always reading services.deliveryApi.
Update the API selection in the effect around composerCloudProject and the
Promise.all calls to use projectSpaceApiFor(composerCloudProject), preserving
the existing early return and loading behavior.
---
Nitpick comments:
In `@wework/src/components/layout/DesktopWorkbenchMain.tsx`:
- Around line 515-549: Destructure the result of useWorkbenchCloudProjectContext
directly instead of assigning cloudProjectContext first. Export
composerCloudProject from the hook’s return object in
useWorkbenchCloudProjectContext.ts, then consume that returned value in
DesktopWorkbenchMain rather than recomputing it from currentRuntimeTask,
boundCloudProject, and pendingCloudProject.
In `@wework/src/components/layout/useWorkbenchCloudProjectContext.test.tsx`:
- Around line 106-120: Extend the default-project selection test around
useWorkbenchCloudProjectContext to assert the delivery-scoped list APIs
listCloudFiles and listLoopItems are each called exactly once, alongside the
existing listCloudProjects assertion, preserving the intended
no-duplicate-request behavior.
- Around line 68-89: Add coverage in the pending-project test around
prepareSubmission and its onRuntimeTaskCreated callback: invoke the callback
with a runtime task, re-render the context with that task as currentRuntimeTask,
and assert pendingTodoBinding records the runtime target. Preserve the existing
cloudProjectId, additionalContext, and pending-context assertions.
- Line 88: Move the clearPendingProjectContext cleanup out of individual test
bodies and into an unconditional afterEach hook in the test suite. Import
afterEach from vitest and invoke result.current.clearPendingProjectContext()
through the shared cleanup so module-scoped pendingTodoBinding is reset even
when assertions fail.
In `@wework/src/components/layout/useWorkbenchCloudProjectContext.ts`:
- Line 246: Update the API assignments at the referenced call sites in the
workbench context to reuse the memoized `todoBindingApis` value created earlier,
removing repeated `projectSpaceApis(services)` calls while preserving the
existing API-list behavior.
- Around line 411-419: Update the delivery candidate mapping in the surrounding
effect to pass both the candidate title and reference-chip label through the
existing t function, using Chinese defaults and distinct translation keys for
each label. Preserve the delivery ID interpolation and cloud URL unchanged.
- Around line 643-645: In the submission setup, simplify the `submissionItem`
assignment to depend only on whether `currentRuntimeTask` is absent, since
`submissionProject` is derived from that same condition. Preserve
`pendingTodoItem` when no runtime task exists and return `null` otherwise;
update the expression near `submissionProject` without changing surrounding
behavior.
- Around line 448-469: Remove the unreachable catch handler from the
Promise.allSettled chain in the cloud-project loading flow, preserving the
existing fulfilled-result filtering and active-state guard in the then callback.
- Around line 159-167: Split useWorkbenchCloudProjectContext into focused hooks
aligned with its existing responsibilities: extract cloud project
binding/bound-context loading and mention candidate loading into cohesive hooks
such as useCloudProjectBinding and useCloudMentionCandidates, while keeping UI
action handlers separate. Preserve the current public context API and behavior,
and use clear English names and explicit interfaces for the extracted hooks.
🪄 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: d3d6e641-335f-4e54-ac86-cb596d5a4242
📒 Files selected for processing (3)
wework/src/components/layout/DesktopWorkbenchMain.tsxwework/src/components/layout/useWorkbenchCloudProjectContext.test.tsxwework/src/components/layout/useWorkbenchCloudProjectContext.ts
1 participant
What changed
DesktopWorkbenchMainintouseWorkbenchCloudProjectContext.DesktopWorkbenchMain.tsxby roughly 600 lines and moved the cloud workflow behind an explicit hook interface.Why
DesktopWorkbenchMainis one of the highest-churn Wework files and was coordinating more than 100 hooks. The cloud project workflow was an independent asynchronous state machine embedded in the main layout, which increased the blast radius of unrelated workbench changes and made its request behavior difficult to test directly.Impact
Validation
pnpm --filter wework typecheckpnpm --filter wework exec vitest run src/components/layout/useWorkbenchCloudProjectContext.test.tsxpnpm --filter wework exec vitest run src/components/layout/DesktopWorkbenchLayout.test.tsx— 166 passedpnpm --filter wework test— 333 files, 3198 tests passedSummary by CodeRabbit
New Features
Bug Fixes
Tests