Repository navigation
fix(wework): align chat UI and skill compatibility - #1381
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 (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR adds provider to UnifiedModel.runtime, threads selectedModel through composers, gates local skills by model compatibility, improves project→device resolution and offline banners, introduces assistant error parsing + retry/switch flows, removes voice-input UI, and migrates many UI controls to token-based styling with a new blue accent. ChangesModel runtime and local skill gating
Device resolution and offline handling
Theme token and appearance refresh
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
|
![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: 5
🧹 Nitpick comments (4)
wework/src/features/appearance/AppearanceSettingsPage.tsx (1)
33-196: ⚖️ Poor tradeoffConsider extracting sub-sections into smaller components.
The
AppearanceSettingsPagefunction spans 163 lines, exceeding the 50-line guideline. While the function is cohesive, extracting the theme preview panel, accent/sidebar controls, and font inputs into separate components would improve maintainability.🤖 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/appearance/AppearanceSettingsPage.tsx` around lines 33 - 196, AppearanceSettingsPage is too large—extract logical subsections into smaller components: create ThemeModeButtons that maps themeModes and uses setAppearance and appearance.mode (used where data-testid `appearance-mode-*` is defined), a PreviewPanel that reads resolvedMode and renders the preview markup (the part with sidebar mock and preview action button), an AppearanceControls component that contains the accent color input, sidebarTranslucent toggle and contrast slider (wiring to appearance.accentColor, appearance.sidebarTranslucent, appearance.contrast and calling setAppearance), and a FontSettings component for the two inputs bound to appearance.uiFont and appearance.codeFont; replace the inline JSX in AppearanceSettingsPage with these components and pass only the needed props (appearance, resolvedMode, setAppearance) and keep usage of SettingRow unchanged.Source: Coding guidelines
wework/src/components/layout/DesktopWorkbenchMain.tsx (1)
35-38: ⚡ Quick winRemove unused import.
MAC_NATIVE_TOP_BAR_ACTION_INSETis imported but never used in this file.♻️ Proposed fix
-import { - DesktopTopBar, - MAC_NATIVE_TOP_BAR_ACTION_INSET, -} from './DesktopTopBar' +import { DesktopTopBar } from './DesktopTopBar' import { ConversationDeviceOfflineBanner } from './ConversationDeviceOfflineBanner'🤖 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 35 - 38, The import statement in DesktopWorkbenchMain.tsx includes an unused symbol MAC_NATIVE_TOP_BAR_ACTION_INSET; remove MAC_NATIVE_TOP_BAR_ACTION_INSET from the import list (keep DesktopTopBar) so the file only imports symbols that are actually used, and run a quick lint/compile to confirm no other references to MAC_NATIVE_TOP_BAR_ACTION_INSET remain.wework/src/components/chat/MessageList.tsx (1)
355-367: ⚡ Quick winConsider using theme tokens instead of hardcoded blue colors.
The skill token styling at lines 359 and 364 uses hardcoded
text-blue-600classes. According to the PR objectives, this PR introduces a "theme token and visual refresh" with "blue accent colors," but other files in the stack use theme tokens liketext-primaryandbg-primary.For consistency with the theme system and to support future theme customization, consider using theme tokens instead of hardcoded Tailwind color classes.
💡 Example using theme tokens
- className="inline-flex h-7 max-w-full items-center gap-1 rounded-xl bg-muted px-2 align-baseline text-[13px] font-medium leading-none text-blue-600 no-underline" + className="inline-flex h-7 max-w-full items-center gap-1 rounded-xl bg-muted px-2 align-baseline text-[13px] font-medium leading-none text-accent no-underline" onClick={event => event.preventDefault()} > <Package data-testid={`sent-local-skill-icon-${localSkillTokenTestId(skillName)}`} - className="h-3.5 w-3.5 shrink-0 text-blue-600" + className="h-3.5 w-3.5 shrink-0 text-accent" />(Assuming
text-accentis defined in your theme tokens.)🤖 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/MessageList.tsx` around lines 355 - 367, Replace the hardcoded Tailwind blue classes in the skill token anchor and icon with theme token classes so the component respects the app theme; specifically update the anchor rendered in MessageList (key=`skill-${start}`) to use theme tokens instead of "text-blue-600" and replace the Package icon's "text-blue-600" with the same token (e.g., "text-primary" or "text-accent"), and if applicable swap "bg-muted" to a themed background token so colors are consistent with the rest of the system.wework/src/components/chat/composer/ProjectWorkBar.tsx (1)
344-352: 💤 Low valueConsider removing the redundant online status sort.
Since
availableProjects(line 339) already filters for online devices viaisProjectAvailable(which callsisOnlineDeviceat line 335), all projects insortedProjectswill haveonlineAandonlineBequal to1. The sort at lines 348-350 is now a no-op.♻️ Simplify by removing the redundant sort
const sortedProjects = useMemo(() => { - return [...availableProjects].sort((a, b) => { - const deviceA = getDeviceForProject(a) - const deviceB = getDeviceForProject(b) - const onlineA = deviceA?.status === 'online' ? 1 : 0 - const onlineB = deviceB?.status === 'online' ? 1 : 0 - return onlineB - onlineA - }) + return [...availableProjects] - }, [availableProjects, getDeviceForProject]) + }, [availableProjects])🤖 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/composer/ProjectWorkBar.tsx` around lines 344 - 352, The sort in the useMemo for sortedProjects is redundant because availableProjects is already filtered by isProjectAvailable (which uses isOnlineDevice), so remove the online-status compare and simplify the memo to just return the availableProjects (e.g., return [...availableProjects] or availableProjects) inside the useMemo; also remove getDeviceForProject from the dependency array and leave only [availableProjects]; update the sortedProjects useMemo accordingly to avoid the no-op sort.
🤖 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/MessageList.tsx`:
- Around line 329-339: Extract the duplicated name-formatting logic into a
shared utility: create a new module exporting skillTokenTestId(name: string) and
displaySkillName(name: string) that implement the same regex replace and
split/map/join logic currently in localSkillTokenTestId and
displayLocalSkillName (and in ComposerTextarea's
localSkillTestId/displaySkillNameFromName); then replace the local
implementations in MessageList.tsx (localSkillTokenTestId,
displayLocalSkillName) and in ComposerTextarea.tsx (localSkillTestId,
displaySkillNameFromName) with imports from the new skill-formatting module so
both files call the shared skillTokenTestId and displaySkillName functions.
In `@wework/src/components/layout/DesktopSidebar.tsx`:
- Around line 214-216: Change the computed class so empty sections never reveal
the chevron on hover: update the iconVisibilityClass expression (used in
DesktopSidebar.tsx) to first check hasContent and return a plain 'opacity-0'
when hasContent is false; otherwise preserve the existing behavior (when
hasContent is true use 'opacity-100' if !expanded, else 'opacity-0
group-hover/section:opacity-100'). This ensures the chevron is fully hidden for
sections with no content while keeping the current hover/expanded behavior for
sections that do have content.
In `@wework/src/components/layout/MobileWorkbenchLayout.test.tsx`:
- Line 227: The current assertion checks that 'chat-message-scroll-area' does
not have 'pt-28', which won't catch regressions because the component already
renders 'pt-16'; change the test to assert the expected spacing directly by
checking that getByTestId('chat-message-scroll-area') has the 'pt-16' class (or
otherwise assert the exact class list used by MobileWorkbenchLayout) so the test
fails if mobile conversation spacing changes.
In `@wework/src/features/workbench/WorkbenchProvider.test.tsx`:
- Around line 106-128: The new ProjectTaskSendProbe component and its
device-resolution scenarios should be moved out of the large
WorkbenchProvider.test.tsx into a focused spec file (e.g.,
WorkbenchProvider.device-resolution.test.tsx): create a new test file, import
React, render, and the same helpers/providers used in the original tests, then
copy the ProjectTaskSendProbe component and associated tests that exercise
useWorkbench, workbench.openTask(71), workbench.setInput('continue'), and
workbench.sendCurrentInput() into that file so the original file stays under the
1000-line limit and the new project/device-resolution coverage is maintained in
a dedicated spec.
In `@wework/src/features/workbench/WorkbenchProvider.tsx`:
- Around line 1248-1253: When resolvedProjectId is undefined but
findProjectForTask(state.projects, detailTask) returns a project, preserve that
project's id in the canonical route by assigning routeProjectId to the found
project's id; update the logic that computes project/routeProjectId (referencing
resolvedProjectId, findProjectForTask, project, and routeProjectId) so that when
resolvedProjectId === undefined you set routeProjectId = project?.id (and keep
the existing branches for resolvedProjectId > 0 and <= 0); apply the same fix to
the analogous block around the other occurrence (lines referenced near
1291-1294) so recovered project-owned tasks generate
/projects/:projectId/tasks/:id instead of losing project context.
---
Nitpick comments:
In `@wework/src/components/chat/composer/ProjectWorkBar.tsx`:
- Around line 344-352: The sort in the useMemo for sortedProjects is redundant
because availableProjects is already filtered by isProjectAvailable (which uses
isOnlineDevice), so remove the online-status compare and simplify the memo to
just return the availableProjects (e.g., return [...availableProjects] or
availableProjects) inside the useMemo; also remove getDeviceForProject from the
dependency array and leave only [availableProjects]; update the sortedProjects
useMemo accordingly to avoid the no-op sort.
In `@wework/src/components/chat/MessageList.tsx`:
- Around line 355-367: Replace the hardcoded Tailwind blue classes in the skill
token anchor and icon with theme token classes so the component respects the app
theme; specifically update the anchor rendered in MessageList
(key=`skill-${start}`) to use theme tokens instead of "text-blue-600" and
replace the Package icon's "text-blue-600" with the same token (e.g.,
"text-primary" or "text-accent"), and if applicable swap "bg-muted" to a themed
background token so colors are consistent with the rest of the system.
In `@wework/src/components/layout/DesktopWorkbenchMain.tsx`:
- Around line 35-38: The import statement in DesktopWorkbenchMain.tsx includes
an unused symbol MAC_NATIVE_TOP_BAR_ACTION_INSET; remove
MAC_NATIVE_TOP_BAR_ACTION_INSET from the import list (keep DesktopTopBar) so the
file only imports symbols that are actually used, and run a quick lint/compile
to confirm no other references to MAC_NATIVE_TOP_BAR_ACTION_INSET remain.
In `@wework/src/features/appearance/AppearanceSettingsPage.tsx`:
- Around line 33-196: AppearanceSettingsPage is too large—extract logical
subsections into smaller components: create ThemeModeButtons that maps
themeModes and uses setAppearance and appearance.mode (used where data-testid
`appearance-mode-*` is defined), a PreviewPanel that reads resolvedMode and
renders the preview markup (the part with sidebar mock and preview action
button), an AppearanceControls component that contains the accent color input,
sidebarTranslucent toggle and contrast slider (wiring to appearance.accentColor,
appearance.sidebarTranslucent, appearance.contrast and calling setAppearance),
and a FontSettings component for the two inputs bound to appearance.uiFont and
appearance.codeFont; replace the inline JSX in AppearanceSettingsPage with these
components and pass only the needed props (appearance, resolvedMode,
setAppearance) and keep usage of SettingRow unchanged.
🪄 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: c389204a-1ee5-49bf-9244-2388fe111589
📒 Files selected for processing (47)
backend/app/services/model_aggregation_service.pybackend/tests/services/test_model_aggregation_service.pywework/src/components/chat/ChatInput.test.tsxwework/src/components/chat/ChatInput.tsxwework/src/components/chat/MessageList.test.tsxwework/src/components/chat/MessageList.tsxwework/src/components/chat/composer/CompactChatComposer.tsxwework/src/components/chat/composer/ComposerTextarea.tsxwework/src/components/chat/composer/ComposerToolbar.tsxwework/src/components/chat/composer/ProjectChatComposer.tsxwework/src/components/chat/composer/ProjectWorkBar.tsxwework/src/components/common/ActionMenu.tsxwework/src/components/common/BranchSelector.tsxwework/src/components/common/TextInputDialog.test.tsxwework/src/components/common/TextInputDialog.tsxwework/src/components/layout/ConversationDeviceOfflineBanner.tsxwework/src/components/layout/DesktopSidebar.tsxwework/src/components/layout/DesktopWorkbenchLayout.test.tsxwework/src/components/layout/DesktopWorkbenchLayout.tsxwework/src/components/layout/DesktopWorkbenchMain.tsxwework/src/components/layout/DeviceStatusPrompt.test.tsxwework/src/components/layout/DeviceStatusPrompt.tsxwework/src/components/layout/EnvironmentInfoPopover.tsxwework/src/components/layout/MobileWorkbenchLayout.test.tsxwework/src/components/layout/MobileWorkbenchLayout.tsxwework/src/components/plugins/McpManagementSections.tsxwework/src/components/plugins/PluginManagementWorkspace.tsxwework/src/components/plugins/PluginUploadDialog.tsxwework/src/components/plugins/PluginsWorkspace.tsxwework/src/components/plugins/SkillUploadDialog.tsxwework/src/components/projects/ProjectCreateDialog.test.tsxwework/src/components/projects/ProjectCreateDialog.tsxwework/src/components/settings/AddCloudDeviceDialog.tsxwework/src/components/settings/ConnectionsSettingsPage.test.tsxwework/src/components/settings/ConnectionsSettingsPage.tsxwework/src/components/ui/button.tsxwework/src/features/appearance/AppearanceProvider.test.tsxwework/src/features/appearance/AppearanceSettingsPage.test.tsxwework/src/features/appearance/AppearanceSettingsPage.tsxwework/src/features/appearance/presets.tswework/src/features/workbench/WorkbenchProvider.test.tsxwework/src/features/workbench/WorkbenchProvider.tsxwework/src/i18n/locales/en/common.jsonwework/src/i18n/locales/zh-CN/common.jsonwework/src/lib/workbench-device.test.tswework/src/lib/workbench-device.tswework/src/types/api.ts
Comment on lines
+329
to
+339
| function localSkillTokenTestId(name: string): string { | ||
| return name.replace(/[^a-zA-Z0-9_-]/g, '-') | ||
| } | ||
|
|
||
| function displayLocalSkillName(name: string): string { | ||
| return name | ||
| .split(/[-_\s]+/) | ||
| .filter(Boolean) | ||
| .map(part => part.charAt(0).toUpperCase() + part.slice(1)) | ||
| .join(' ') | ||
| } |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Extract duplicated skill name formatting to a shared utility.
The localSkillTokenTestId (lines 329-331) and displayLocalSkillName (lines 333-339) functions duplicate identical logic from ComposerTextarea.tsx lines 171-175 (localSkillTestId) and lines 65-71 (displaySkillNameFromName).
♻️ Extract to shared utility module
Create wework/src/lib/skill-formatting.ts:
export function skillTokenTestId(name: string): string {
return name.replace(/[^a-zA-Z0-9_-]/g, '-')
}
export function displaySkillName(name: string): string {
return name
.split(/[-_\s]+/)
.filter(Boolean)
.map(part => part.charAt(0).toUpperCase() + part.slice(1))
.join(' ')
}Then import and use in both ComposerTextarea.tsx and MessageList.tsx.
🤖 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/MessageList.tsx` around lines 329 - 339, Extract
the duplicated name-formatting logic into a shared utility: create a new module
exporting skillTokenTestId(name: string) and displaySkillName(name: string) that
implement the same regex replace and split/map/join logic currently in
localSkillTokenTestId and displayLocalSkillName (and in ComposerTextarea's
localSkillTestId/displaySkillNameFromName); then replace the local
implementations in MessageList.tsx (localSkillTokenTestId,
displayLocalSkillName) and in ComposerTextarea.tsx (localSkillTestId,
displaySkillNameFromName) with imports from the new skill-formatting module so
both files call the shared skillTokenTestId and displaySkillName functions.
Comment on lines
+214
to
+216
| const iconVisibilityClass = hasContent && !expanded | ||
| ? 'opacity-100' | ||
| : 'opacity-0 group-hover/section:opacity-100' |
Contributor
There was a problem hiding this comment.
Hide chevron entirely when the section has no content.
On Line 214, iconVisibilityClass still reveals the chevron on hover when hasContent is false, which makes empty sections look expandable.
Suggested fix
- const iconVisibilityClass = hasContent && !expanded
- ? 'opacity-100'
- : 'opacity-0 group-hover/section:opacity-100'
+ const iconVisibilityClass = !hasContent
+ ? 'opacity-0'
+ : !expanded
+ ? 'opacity-100'
+ : 'opacity-0 group-hover/section:opacity-100'📝 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 iconVisibilityClass = hasContent && !expanded | |
| ? 'opacity-100' | |
| : 'opacity-0 group-hover/section:opacity-100' | |
| const iconVisibilityClass = !hasContent | |
| ? 'opacity-0' | |
| : !expanded | |
| ? 'opacity-100' | |
| : 'opacity-0 group-hover/section:opacity-100' |
🤖 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/DesktopSidebar.tsx` around lines 214 - 216,
Change the computed class so empty sections never reveal the chevron on hover:
update the iconVisibilityClass expression (used in DesktopSidebar.tsx) to first
check hasContent and return a plain 'opacity-0' when hasContent is false;
otherwise preserve the existing behavior (when hasContent is true use
'opacity-100' if !expanded, else 'opacity-0 group-hover/section:opacity-100').
This ensures the chevron is fully hidden for sections with no content while
keeping the current hover/expanded behavior for sections that do have content.
| 'conversation-device-offline-banner', | ||
| ), | ||
| ).toBeInTheDocument() | ||
| expect(screen.getByTestId('chat-message-scroll-area')).not.toHaveClass('pt-28') |
Contributor
There was a problem hiding this comment.
This padding assertion won't catch the regression you care about.
MobileWorkbenchLayout already renders the scroll area with pt-16, so asserting that it does not have pt-28 passes before and after this change. Please assert the expected class/element directly instead, otherwise this test won't fail if the mobile conversation spacing regresses.
🤖 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/MobileWorkbenchLayout.test.tsx` at line 227, The
current assertion checks that 'chat-message-scroll-area' does not have 'pt-28',
which won't catch regressions because the component already renders 'pt-16';
change the test to assert the expected spacing directly by checking that
getByTestId('chat-message-scroll-area') has the 'pt-16' class (or otherwise
assert the exact class list used by MobileWorkbenchLayout) so the test fails if
mobile conversation spacing changes.
Comment on lines
+106
to
+128
| function ProjectTaskSendProbe() { | ||
| const workbench = useWorkbench() | ||
|
|
||
| return ( | ||
| <div> | ||
| <span data-testid="workbench-input">{workbench.state.input}</span> | ||
| <span data-testid="workbench-error">{workbench.state.error ?? ''}</span> | ||
| <span data-testid="current-task-id"> | ||
| {workbench.state.currentTask?.id ?? 'no-task'} | ||
| </span> | ||
| <span data-testid="project-count">{workbench.state.projects.length}</span> | ||
| <button type="button" onClick={() => void workbench.openTask(71)}> | ||
| open project task | ||
| </button> | ||
| <button type="button" onClick={() => workbench.setInput('continue')}> | ||
| set input | ||
| </button> | ||
| <button type="button" onClick={() => void workbench.sendCurrentInput()}> | ||
| send | ||
| </button> | ||
| </div> | ||
| ) | ||
| } |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy lift
Split these new provider scenarios into a dedicated spec file.
This test file is already far beyond the repo’s 1000-line limit, and adding another probe plus another device-resolution scenario makes it harder to navigate and maintain. Please move the new project/device-resolution coverage into a focused spec (for example, WorkbenchProvider.device-resolution.test.tsx).
As per coding guidelines, "File size limit: If a file exceeds 1000 lines, split it into multiple sub-modules."
Also applies to: 1835-1950
🤖 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` around lines 106 -
128, The new ProjectTaskSendProbe component and its device-resolution scenarios
should be moved out of the large WorkbenchProvider.test.tsx into a focused spec
file (e.g., WorkbenchProvider.device-resolution.test.tsx): create a new test
file, import React, render, and the same helpers/providers used in the original
tests, then copy the ProjectTaskSendProbe component and associated tests that
exercise useWorkbench, workbench.openTask(71), workbench.setInput('continue'),
and workbench.sendCurrentInput() into that file so the original file stays under
the 1000-line limit and the new project/device-resolution coverage is maintained
in a dedicated spec.
Source: Coding guidelines
Comment on lines
1248
to
1253
| const project = | ||
| resolvedProjectId === undefined | ||
| ? undefined | ||
| ? findProjectForTask(state.projects, detailTask) ?? undefined | ||
| : resolvedProjectId > 0 | ||
| ? state.projects.find(item => item.id === resolvedProjectId) ?? null | ||
| : null |
Contributor
There was a problem hiding this comment.
Preserve the inferred project id in the canonical task route.
When resolvedProjectId is undefined but findProjectForTask(...) finds a project, project is set correctly while routeProjectId stays undefined. That navigates project-owned chats to /tasks/:id instead of /projects/:projectId/tasks/:id, so the URL drops project context for any task that was recovered only through projects[].tasks.
Suggested fix
const project =
resolvedProjectId === undefined
? findProjectForTask(state.projects, detailTask) ?? undefined
: resolvedProjectId > 0
? state.projects.find(item => item.id === resolvedProjectId) ?? null
: null
@@
- const routeProjectId =
- resolvedProjectId === undefined ? undefined : resolvedProjectId
+ const routeProjectId = resolvedProjectId ?? project?.id
handledTaskRouteRef.current = getTaskRouteKey(taskId, routeProjectId)
navigateTo(buildTaskRoute({ taskId, projectId: routeProjectId }))Also applies to: 1291-1294
🤖 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.tsx` around lines 1248 -
1253, When resolvedProjectId is undefined but findProjectForTask(state.projects,
detailTask) returns a project, preserve that project's id in the canonical route
by assigning routeProjectId to the found project's id; update the logic that
computes project/routeProjectId (referencing resolvedProjectId,
findProjectForTask, project, and routeProjectId) so that when resolvedProjectId
=== undefined you set routeProjectId = project?.id (and keep the existing
branches for resolvedProjectId > 0 and <= 0); apply the same fix to the
analogous block around the other occurrence (lines referenced near 1291-1294) so
recovered project-owned tasks generate /projects/:projectId/tasks/:id instead of
losing project context.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
wework/src/components/layout/DesktopWorkbenchLayout.test.tsx (1)
1-2883: 🏗️ Heavy liftSplit this test module to satisfy the repository file-size rule.
This file is 2883 lines, and new assertions are still being added here. Please split this suite into smaller test modules (for example: sidebar behavior, project creation flows, workspace panels, offline/device banners) to keep tests maintainable and reviewable.
As per coding guidelines, "File size limit: If a file exceeds 1000 lines, split it into multiple sub-modules".
🤖 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/DesktopWorkbenchLayout.test.tsx` around lines 1 - 2883, The test module DesktopWorkbenchLayout.test.tsx is over the repo file-size limit (≈2883 lines); split it into multiple smaller test files by feature (e.g. sidebar.spec.tsx for sidebar behavior and persistence tests, project-creation.spec.tsx for all project create/clone/empty-folder flows, workspace-panels.spec.tsx for right/bottom panel open/resize/close tests, environment.spec.tsx for environment/info/branch/commit tests, and offline-devices.spec.tsx for device/offline/composer lock tests), extract shared fixtures/helpers (createRect, baseProps, createCloudWorkspacePanelState, renderWorkspacePanelLayout, vi.mock setups) into a new test-utils module and import them from each new spec, update each new file to import DesktopWorkbenchLayout and only the subset of tests relevant to that file (preserve individual test names and vi.mock usage), and ensure test runner (vitest) still discovers the new files by using the same naming pattern (*.spec.tsx or *.test.tsx).Source: Coding guidelines
wework/src/components/chat/composer/ModelSelector.tsx (1)
98-104: 💤 Low valueProgrammatic click reuses existing handler logic.
The
buttonRef.current?.click()approach is pragmatic and avoids duplicating the menu-opening logic from the button's click handler (lines 843-856). This works reliably in React 19, though a more declarative alternative would be callingsetOpen(true)directly and duplicating the initialization ofactiveFamilyIdandactiveDesktopSubmenu.The current pattern is acceptable and maintains DRY principles by reusing the click handler.
🤖 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/composer/ModelSelector.tsx` around lines 98 - 104, Keep the existing programmatic invocation using buttonRef.current?.click() to reuse the button click handler (avoid duplicating logic that initializes activeFamilyId and activeDesktopSubmenu); ensure handledOpenSignalRef.current is set to openSignal before calling click and keep [disabled, open, openSignal] as the effect deps; add a brief inline comment above this useEffect explaining that buttonRef.current?.click() intentionally reuses the button's click handler to maintain DRY and reference the button's handler (lines handling activeFamilyId/activeDesktopSubmenu) so future readers understand the rationale.wework/src/components/chat/MessageList.tsx (1)
495-500: 💤 Low valueConsolidate duplicate thinking indicator blocks.
The two separate blocks rendering "正在思考" are mutually exclusive but duplicate the same text. Consider consolidating into a single conditional.
♻️ Proposed consolidation
- {isThinking && ( - <span className="text-text-muted">正在思考</span> - )} - {isStreaming && hasVisibleContent && ( + {(isThinking || (isStreaming && hasVisibleContent)) && ( <span className="text-text-muted">正在思考</span> )}🤖 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/MessageList.tsx` around lines 495 - 500, The two duplicate spans rendering "正在思考" should be consolidated into one conditional to remove redundancy: replace the two separate checks with a single render that shows the span when isThinking || (isStreaming && hasVisibleContent) so you only have one <span className="text-text-muted">正在思考</span>; locate the logic in the MessageList component where isThinking, isStreaming, and hasVisibleContent are used and update that block accordingly while preserving the existing className and rendering order.
🤖 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/WorkbenchProvider.test.tsx`:
- Around line 2757-2763: The test's deviceApi.listDevices mock returns an empty
array which prevents WorkbenchProvider.sendCurrentInput from finding an online
compatible device so sendMessage is never called; update the
deviceApi.listDevices mock (and the second occurrence referenced around lines
2779-2781) to return a compatible online device object (matching the shape used
by the adjacent send-path tests) so the initial sendCurrentInput proceeds and
sendMessage is invoked, allowing the retry assertions to execute.
In `@wework/src/features/workbench/WorkbenchProvider.tsx`:
- Around line 1894-1943: retryFailedMessage currently skips the
device/offline/version checks used by sendCurrentInput and can send when the
active project device is missing/offline/old; update retryFailedMessage to
perform the same gating as sendCurrentInput by validating
prepared.activeDeviceId and the project's active device status/version before
queuing or calling sendPreparedMessage (or refactor the gating into a shared
helper used by both sendCurrentInput and retryFailedMessage); ensure you honor
hasActiveTurn/state.currentTask?.id behavior and only enqueue when the device
checks pass, otherwise dispatch the same error/notification flow as
sendCurrentInput.
---
Nitpick comments:
In `@wework/src/components/chat/composer/ModelSelector.tsx`:
- Around line 98-104: Keep the existing programmatic invocation using
buttonRef.current?.click() to reuse the button click handler (avoid duplicating
logic that initializes activeFamilyId and activeDesktopSubmenu); ensure
handledOpenSignalRef.current is set to openSignal before calling click and keep
[disabled, open, openSignal] as the effect deps; add a brief inline comment
above this useEffect explaining that buttonRef.current?.click() intentionally
reuses the button's click handler to maintain DRY and reference the button's
handler (lines handling activeFamilyId/activeDesktopSubmenu) so future readers
understand the rationale.
In `@wework/src/components/chat/MessageList.tsx`:
- Around line 495-500: The two duplicate spans rendering "正在思考" should be
consolidated into one conditional to remove redundancy: replace the two separate
checks with a single render that shows the span when isThinking || (isStreaming
&& hasVisibleContent) so you only have one <span
className="text-text-muted">正在思考</span>; locate the logic in the MessageList
component where isThinking, isStreaming, and hasVisibleContent are used and
update that block accordingly while preserving the existing className and
rendering order.
In `@wework/src/components/layout/DesktopWorkbenchLayout.test.tsx`:
- Around line 1-2883: The test module DesktopWorkbenchLayout.test.tsx is over
the repo file-size limit (≈2883 lines); split it into multiple smaller test
files by feature (e.g. sidebar.spec.tsx for sidebar behavior and persistence
tests, project-creation.spec.tsx for all project create/clone/empty-folder
flows, workspace-panels.spec.tsx for right/bottom panel open/resize/close tests,
environment.spec.tsx for environment/info/branch/commit tests, and
offline-devices.spec.tsx for device/offline/composer lock tests), extract shared
fixtures/helpers (createRect, baseProps, createCloudWorkspacePanelState,
renderWorkspacePanelLayout, vi.mock setups) into a new test-utils module and
import them from each new spec, update each new file to import
DesktopWorkbenchLayout and only the subset of tests relevant to that file
(preserve individual test names and vi.mock usage), and ensure test runner
(vitest) still discovers the new files by using the same naming pattern
(*.spec.tsx or *.test.tsx).
🪄 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: 089cd3ae-7cce-4011-8534-455bb233d88c
📒 Files selected for processing (24)
packages/chat-core/src/workbench-message-reducer.tswework/src/components/chat/ChatInput.test.tsxwework/src/components/chat/ChatInput.tsxwework/src/components/chat/MessageList.test.tsxwework/src/components/chat/MessageList.tsxwework/src/components/chat/ScrollableMessageArea.tsxwework/src/components/chat/composer/ComposerToolbar.tsxwework/src/components/chat/composer/ModelSelector.tsxwework/src/components/chat/composer/ProjectChatComposer.tsxwework/src/components/layout/DesktopSidebar.tsxwework/src/components/layout/DesktopWorkbenchLayout.test.tsxwework/src/components/layout/DesktopWorkbenchLayout.tsxwework/src/components/layout/DesktopWorkbenchMain.tsxwework/src/components/layout/MobileDrawer.tsxwework/src/components/layout/MobileWorkbenchLayout.tsxwework/src/features/workbench/WorkbenchProvider.test.tsxwework/src/features/workbench/WorkbenchProvider.tsxwework/src/features/workbench/messageReducer.test.tswework/src/i18n/locales/en/chat.jsonwework/src/i18n/locales/zh-CN/chat.jsonwework/src/lib/chat-error.test.tswework/src/lib/chat-error.tswework/src/pages/WorkbenchPage.tsxwework/src/types/api.ts
✅ Files skipped from review due to trivial changes (3)
- wework/src/i18n/locales/en/chat.json
- wework/src/i18n/locales/zh-CN/chat.json
- wework/src/components/layout/MobileDrawer.tsx
🚧 Files skipped from review as they are similar to previous changes (3)
- wework/src/components/layout/DesktopWorkbenchLayout.tsx
- wework/src/components/layout/DesktopSidebar.tsx
- wework/src/components/chat/composer/ProjectChatComposer.tsx
Comment on lines
+2757
to
+2763
| deviceApi: { | ||
| listDevices: vi.fn().mockResolvedValue([]), | ||
| getHomeDirectory: vi.fn(), | ||
| getProjectWorkspaceRoot: vi.fn(), | ||
| listDirectories: vi.fn(), | ||
| listSkills: vi.fn().mockResolvedValue([]), | ||
| }, |
Contributor
There was a problem hiding this comment.
This retry test never reaches the first send.
deviceApi.listDevices returns [], so the initial sendCurrentInput() is blocked by the standalone-device guard in WorkbenchProvider.sendCurrentInput(). That means sendMessage never gets its first call, and the retry assertions become unreachable. Mock one compatible online device here, like the adjacent send-path tests do.
Proposed fix
deviceApi: {
- listDevices: vi.fn().mockResolvedValue([]),
+ listDevices: vi.fn().mockResolvedValue([
+ {
+ id: 1,
+ device_id: 'local-online',
+ name: 'Local Online',
+ status: 'online',
+ is_default: false,
+ device_type: 'local',
+ bind_shell: 'claudecode',
+ executor_version: '1.8.5',
+ },
+ ]),
getHomeDirectory: vi.fn(),
getProjectWorkspaceRoot: vi.fn(),
listDirectories: vi.fn(),
listSkills: vi.fn().mockResolvedValue([]),
},Also applies to: 2779-2781
🤖 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` around lines 2757 -
2763, The test's deviceApi.listDevices mock returns an empty array which
prevents WorkbenchProvider.sendCurrentInput from finding an online compatible
device so sendMessage is never called; update the deviceApi.listDevices mock
(and the second occurrence referenced around lines 2779-2781) to return a
compatible online device object (matching the shape used by the adjacent
send-path tests) so the initial sendCurrentInput proceeds and sendMessage is
invoked, allowing the retry assertions to execute.
Comment on lines
+1894
to
+1943
| const retryFailedMessage = useCallback( | ||
| async (messageId: string) => { | ||
| const failedMessageIndex = messages.findIndex( | ||
| message => | ||
| message.id === messageId && | ||
| message.role === 'assistant' && | ||
| message.status === 'failed', | ||
| ) | ||
| if (failedMessageIndex === -1) { | ||
| dispatch({ type: 'error_set', error: '未找到可重试的失败消息' }) | ||
| return | ||
| } | ||
|
|
||
| const previousUserMessage = [...messages] | ||
| .slice(0, failedMessageIndex) | ||
| .reverse() | ||
| .find(message => message.role === 'user') | ||
| if (!previousUserMessage) { | ||
| dispatch({ type: 'error_set', error: '未找到可重试的用户消息' }) | ||
| return | ||
| } | ||
|
|
||
| const prepared = buildSendPayload( | ||
| previousUserMessage.content, | ||
| previousUserMessage.attachments ?? [], | ||
| ) | ||
| if (!prepared) return | ||
|
|
||
| if (hasActiveTurn && state.currentTask?.id) { | ||
| setQueuedSends(items => [ | ||
| ...items, | ||
| { | ||
| id: `queued-${state.currentTask?.id}-${Date.now()}`, | ||
| content: previousUserMessage.content, | ||
| status: 'queued', | ||
| createdAt: new Date().toISOString(), | ||
| payload: prepared.payload, | ||
| activeDeviceId: prepared.activeDeviceId, | ||
| attachments: previousUserMessage.attachments, | ||
| }, | ||
| ]) | ||
| return | ||
| } | ||
|
|
||
| await sendPreparedMessage( | ||
| previousUserMessage.content, | ||
| prepared.payload, | ||
| prepared.activeDeviceId, | ||
| previousUserMessage.attachments, | ||
| ) |
Contributor
There was a problem hiding this comment.
Retry currently bypasses the offline / incompatible-device gate.
retryFailedMessage() jumps straight from buildSendPayload(...) to queue/send, so it skips the device checks that sendCurrentInput() applies on Lines 1812-1850. A failed-message retry can therefore hit sendMessage even when the active project device is missing, offline, or below WEWORK_MIN_EXECUTOR_VERSION, which regresses the new offline-banner/device-resolution behavior.
🤖 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.tsx` around lines 1894 -
1943, retryFailedMessage currently skips the device/offline/version checks used
by sendCurrentInput and can send when the active project device is
missing/offline/old; update retryFailedMessage to perform the same gating as
sendCurrentInput by validating prepared.activeDeviceId and the project's active
device status/version before queuing or calling sendPreparedMessage (or refactor
the gating into a shared helper used by both sendCurrentInput and
retryFailedMessage); ensure you honor hasActiveTurn/state.currentTask?.id
behavior and only enqueue when the device checks pass, otherwise dispatch the
same error/notification flow as sendCurrentInput.
qdaxb
approved these changes
Jun 12, 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
2 participants
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
New Features
Improvements
Style