Repository navigation
Codex/feature/dialog message list optimization - #1254
Merged
qdaxb merged 1 commit intoJun 3, 2026
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughThis PR extends the workbench chat experience with queued and guidance message state, streaming response pause controls, broader file attachment handling, and corresponding UI updates. The changes add new type contracts for queue/guidance lifecycle, refactor the provider to manage queued sends and guidance messages with automatic queue processing, wire these through layouts and composers, render a new queue panel above chat inputs, remove file type restrictions, add streaming pause buttons, and enhance message rendering with attachments and improved styling. ChangesWorkbench Chat Queue and Attachment Flow
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
Task: 1017344_7#60200
FicoHu
force-pushed
the
codex/feature/dialog-message-list-optimization
branch
from
June 3, 2026 12:23
9db46b9 to
6feac8b
Compare
![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: 4
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/chat/ConversationQueuePanel.tsx (1)
1-156:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFile naming convention violation.
Per coding guidelines, "Component names should use PascalCase, and component files should use kebab-case naming in React/TypeScript." This file should be renamed from
ConversationQueuePanel.tsxtoconversation-queue-panel.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/ConversationQueuePanel.tsx` around lines 1 - 156, Rename the file from ConversationQueuePanel.tsx to conversation-queue-panel.tsx to follow kebab-case file naming while keeping the exported React component names (ConversationQueuePanel and QueueRow) in PascalCase; then update any imports throughout the codebase that reference the old filename to import from "conversation-queue-panel" (e.g., import { ConversationQueuePanel } from '...'), and run a quick search to ensure no remaining references to ConversationQueuePanel.tsx remain.
🧹 Nitpick comments (4)
wework/src/components/common/TextInputDialog.test.tsx (1)
23-24: ⚡ Quick winAssert full touch-target sizing, not only height.
The new test checks
h-11, but the component change also depends onmin-w-[44px]for the 44×44 touch target contract. Add class assertions formin-w-[44px](and ideally the close button too) to prevent regressions.As per coding guidelines: "All interactive elements on mobile must be at least 44px × 44px - use
h-11 min-w-[44px]for buttons".🤖 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/common/TextInputDialog.test.tsx` around lines 23 - 24, The test currently only asserts height for the rename dialog buttons; update the assertions for the elements identified by getByTestId('confirm-rename-project-button') and getByTestId('rename-project-input-cancel-button') to also assert the min-width class 'min-w-[44px]' so the 44×44 touch-target contract is enforced (i.e., check both 'h-11' and 'min-w-[44px]' on each relevant button).wework/src/features/workbench/WorkbenchProvider.tsx (1)
1-1406: ⚡ Quick winConsider splitting this file into smaller sub-modules.
The file has grown to ~1406 lines, exceeding the 1000-line guideline. Consider extracting concerns into separate modules:
- Queue/guidance orchestration (buildSendPayload, sendPreparedMessage, sendNextQueuedMessage, etc.)
- Message/attachment helpers (getSubtaskAttachments, normalizeAttachmentStatus, subtaskToMessage)
- Navigation/lifecycle (selectProject, openTask, createProject, etc.)
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/features/workbench/WorkbenchProvider.tsx` around lines 1 - 1406, This file is too large—split responsibilities into focused modules: extract queue/guidance orchestration (move buildSendPayload, sendPreparedMessage, sendNextQueuedMessage, sendQueuedAsGuidance, sendPreparedMessage helpers, queuedSends state and related hooks like sendCurrentInput, sendNextQueuedMessage, cancelQueuedMessage, editQueuedMessage, guidanceSendInFlightRef) into a workbench-sender module; extract message/attachment helpers (getSubtaskAttachments, normalizeAttachmentStatus, getPersistedToolBlocks, getSubtaskResult, subtaskToMessage, sortSubtasksForDisplay, normalizeBlockStatus usage) into a workbench-message-utils module; extract navigation/lifecycle operations (selectProject, selectStandaloneDevice, startNewChat, startStandaloneChat, startNewProjectChat, openTask, createProject, updateProjectName, removeProject, archive*/rename*/list*/delete* task/project functions, refreshWorkLists, refreshDevices) into a workbench-navigation module; keep WorkbenchProvider only orchestrating imported hooks and wiring context value (state, dispatch, and small glue logic), and update imports/exports accordingly so functions like listLocalSkills, rememberExecutionDevice, and useEffect subscribers still call into the new modules.wework/src/components/chat/MessageList.tsx (2)
135-205: ⚖️ Poor tradeoffExtract the shared image-preview logic to avoid duplication.
MessageImageAttachmentPreviewis nearly identical toImageAttachmentPreviewinAttachmentBadges.tsx(same auth fetch, blob type check, object-URL lifecycle, mounted guard). Only the styling anddata-testiddiffer. Consider extracting a shared hook (e.g.useAttachmentImagePreview(attachmentId)) returning{ previewUrl, hasError }, leaving each component to render its own markup. This also keeps the function under the 50-line preference.🤖 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 135 - 205, Extract the duplicated image-preview logic from MessageImageAttachmentPreview and ImageAttachmentPreview into a new hook useAttachmentImagePreview(attachmentId:string) that returns { previewUrl, hasError }; move fetch(getAttachmentImageUrl(attachmentId)) with auth header, blob type check, URL.createObjectURL/URL.revokeObjectURL lifecycle and the isMounted guard into that hook and call it from both MessageImageAttachmentPreview and ImageAttachmentPreview, keeping each component's rendering, data-testid and styling intact and ensuring the hook accepts attachment.id (or string) and cleans up objectUrl on unmount.
374-378: 💤 Low valueReuse
copyTextfor the code-block copy.
copyTextwas added with a clipboard fallback (textarea +execCommand), butCodeBlockstill callsnavigator.clipboard.writeTextdirectly, which throws in insecure/unsupported contexts. Consider reusingcopyTexthere for consistent behavior.♻️ Proposed change
const handleCopy = () => { - void navigator.clipboard.writeText(text) - setCopied(true) - setTimeout(() => setCopied(false), 1500) + void copyText(text).then(() => { + setCopied(true) + setTimeout(() => setCopied(false), 1500) + }) }🤖 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 374 - 378, The inline handleCopy in MessageList.tsx should use the existing copyText utility (which implements a clipboard fallback) instead of calling navigator.clipboard.writeText directly; replace the navigator.clipboard.writeText call with a call to copyText(text) (await or void as appropriate), keep the setCopied(true)/setTimeout logic, and ensure copyText is imported where handleCopy is defined so copying works in insecure/unsupported contexts.
🤖 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/composer/CompactChatComposer.tsx`:
- Line 4: The import of useTranslation in CompactChatComposer.tsx is coming from
'react-i18next' but should use the app wrapper; change the import to pull
useTranslation from '`@/hooks/useTranslation`' instead of 'react-i18next' (update
the import line that currently imports useTranslation to reference the
application hook so the component uses the app-level i18n wrapper).
In `@wework/src/components/chat/ConversationQueuePanel.tsx`:
- Around line 86-91: Replace the hardcoded Chinese fallbacks in the
ConversationQueuePanel by wiring the i18n translation hook (e.g., import and
call useTranslation to get t) at the component level and use
t('common.send_failed') and t('common.expired') (or whatever key names you
choose) instead of '发送失败' and '已过期' when computing statusText; then add the
corresponding keys and translations to wework/src/i18n/locales/zh-CN/common.json
and wework/src/i18n/locales/en/common.json so both locales provide values for
those keys.
In `@wework/src/components/chat/MessageList.tsx`:
- Around line 241-242: The aria-labels in MessageList.tsx are hardcoded Chinese
strings for the copy button (aria-label using the copied boolean), which
bypasses i18n; update the component to use the existing i18n hook
(useTranslation) and replace the literals with translation lookups (e.g.,
t('copied') when copied is true and t('copyMessage') when false), or reuse the
same translation keys used by ScrollableMessageArea to keep labels consistent
for all locales.
In `@wework/src/components/common/TextInputDialog.tsx`:
- Line 92: The confirm button in TextInputDialog.tsx is using a custom-styled
<button> (the element with className="h-11 min-w-[44px] ...") but must use the
shared Button component with variant="primary"; replace the raw <button> with
the project Button component (pass variant="primary" and preserve existing props
like onClick/disabled) and remove the custom className so the dialog's primary
action follows the standardized styling and behavior.
---
Outside diff comments:
In `@wework/src/components/chat/ConversationQueuePanel.tsx`:
- Around line 1-156: Rename the file from ConversationQueuePanel.tsx to
conversation-queue-panel.tsx to follow kebab-case file naming while keeping the
exported React component names (ConversationQueuePanel and QueueRow) in
PascalCase; then update any imports throughout the codebase that reference the
old filename to import from "conversation-queue-panel" (e.g., import {
ConversationQueuePanel } from '...'), and run a quick search to ensure no
remaining references to ConversationQueuePanel.tsx remain.
---
Nitpick comments:
In `@wework/src/components/chat/MessageList.tsx`:
- Around line 135-205: Extract the duplicated image-preview logic from
MessageImageAttachmentPreview and ImageAttachmentPreview into a new hook
useAttachmentImagePreview(attachmentId:string) that returns { previewUrl,
hasError }; move fetch(getAttachmentImageUrl(attachmentId)) with auth header,
blob type check, URL.createObjectURL/URL.revokeObjectURL lifecycle and the
isMounted guard into that hook and call it from both
MessageImageAttachmentPreview and ImageAttachmentPreview, keeping each
component's rendering, data-testid and styling intact and ensuring the hook
accepts attachment.id (or string) and cleans up objectUrl on unmount.
- Around line 374-378: The inline handleCopy in MessageList.tsx should use the
existing copyText utility (which implements a clipboard fallback) instead of
calling navigator.clipboard.writeText directly; replace the
navigator.clipboard.writeText call with a call to copyText(text) (await or void
as appropriate), keep the setCopied(true)/setTimeout logic, and ensure copyText
is imported where handleCopy is defined so copying works in insecure/unsupported
contexts.
In `@wework/src/components/common/TextInputDialog.test.tsx`:
- Around line 23-24: The test currently only asserts height for the rename
dialog buttons; update the assertions for the elements identified by
getByTestId('confirm-rename-project-button') and
getByTestId('rename-project-input-cancel-button') to also assert the min-width
class 'min-w-[44px]' so the 44×44 touch-target contract is enforced (i.e., check
both 'h-11' and 'min-w-[44px]' on each relevant button).
In `@wework/src/features/workbench/WorkbenchProvider.tsx`:
- Around line 1-1406: This file is too large—split responsibilities into focused
modules: extract queue/guidance orchestration (move buildSendPayload,
sendPreparedMessage, sendNextQueuedMessage, sendQueuedAsGuidance,
sendPreparedMessage helpers, queuedSends state and related hooks like
sendCurrentInput, sendNextQueuedMessage, cancelQueuedMessage, editQueuedMessage,
guidanceSendInFlightRef) into a workbench-sender module; extract
message/attachment helpers (getSubtaskAttachments, normalizeAttachmentStatus,
getPersistedToolBlocks, getSubtaskResult, subtaskToMessage,
sortSubtasksForDisplay, normalizeBlockStatus usage) into a
workbench-message-utils module; extract navigation/lifecycle operations
(selectProject, selectStandaloneDevice, startNewChat, startStandaloneChat,
startNewProjectChat, openTask, createProject, updateProjectName, removeProject,
archive*/rename*/list*/delete* task/project functions, refreshWorkLists,
refreshDevices) into a workbench-navigation module; keep WorkbenchProvider only
orchestrating imported hooks and wiring context value (state, dispatch, and
small glue logic), and update imports/exports accordingly so functions like
listLocalSkills, rememberExecutionDevice, and useEffect subscribers still call
into the new modules.
🪄 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: e56c749d-41e5-411b-a8fc-796ff96534f8
📒 Files selected for processing (35)
wework/src/api/attachments.tswework/src/components/chat/ChatInput.test.tsxwework/src/components/chat/ChatInput.tsxwework/src/components/chat/ConversationQueuePanel.tsxwework/src/components/chat/MessageList.test.tsxwework/src/components/chat/MessageList.tsxwework/src/components/chat/ScrollableMessageArea.test.tsxwework/src/components/chat/ScrollableMessageArea.tsxwework/src/components/chat/composer/AddContextMenu.tsxwework/src/components/chat/composer/AttachmentBadges.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/common/ActionMenu.tsxwework/src/components/common/TextInputDialog.test.tsxwework/src/components/common/TextInputDialog.tsxwework/src/components/layout/DesktopWorkbenchLayout.test.tsxwework/src/components/layout/DesktopWorkbenchLayout.tsxwework/src/components/layout/DesktopWorkbenchMain.tsxwework/src/components/layout/MobileWorkbenchLayout.tsxwework/src/features/workbench/WorkbenchProvider.test.tsxwework/src/features/workbench/WorkbenchProvider.tsxwework/src/features/workbench/messageReducer.test.tswework/src/features/workbench/messageReducer.tswework/src/features/workbench/useWorkbenchAttachments.tswework/src/features/workbench/workbenchProjectChatHooks.test.tsxwework/src/i18n/locales/en/common.jsonwework/src/i18n/locales/zh-CN/common.jsonwework/src/lib/attachments.tswework/src/pages/WorkbenchPage.tsxwework/src/stream/chatStream.test.tswework/src/stream/chatStream.tswework/src/types/api.tswework/src/types/workbench.ts
💤 Files with no reviewable changes (2)
- wework/src/features/workbench/useWorkbenchAttachments.ts
- wework/src/components/chat/composer/AddContextMenu.tsx
| import { ArrowUp, Camera, Image, Maximize2, Mic, Minimize2, Plus, Square } from 'lucide-react' | ||
| import type { ChangeEvent, ClipboardEventHandler } from 'react' | ||
| import { useEffect, useRef, useState } from 'react' | ||
| import { useTranslation } from 'react-i18next' |
Contributor
There was a problem hiding this comment.
Import useTranslation from the application hook, not react-i18next.
Line 4 imports useTranslation from react-i18next, but application code must import from @/hooks/useTranslation. Only i18n infrastructure files should import from react-i18next directly.
🔧 Proposed fix
-import { useTranslation } from 'react-i18next'
+import { useTranslation } from '`@/hooks/useTranslation`'Based on learnings: "Application code MUST import useTranslation from @/hooks/useTranslation, not from react-i18next - only i18n infrastructure files should import from react-i18next directly".
📝 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
| import { useTranslation } from 'react-i18next' | |
| import { useTranslation } from '`@/hooks/useTranslation`' |
🤖 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/CompactChatComposer.tsx` at line 4, The
import of useTranslation in CompactChatComposer.tsx is coming from
'react-i18next' but should use the app wrapper; change the import to pull
useTranslation from '`@/hooks/useTranslation`' instead of 'react-i18next' (update
the import line that currently imports useTranslation to reference the
application hook so the component uses the app-level i18n wrapper).
Comment on lines
+86
to
+91
| const statusText = | ||
| status === 'failed' | ||
| ? error ?? '发送失败' | ||
| : status === 'expired' | ||
| ? error ?? '已过期' | ||
| : null |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Replace hardcoded Chinese strings with i18n keys.
Lines 88 and 90 use hardcoded Chinese strings '发送失败' and '已过期' as fallback values. These should use translation keys for consistency with the rest of the codebase.
🌐 Proposed fix to use i18n
Add the translation hook at the component level and use keys:
+import { useTranslation } from '`@/hooks/useTranslation`'
+
export function ConversationQueuePanel({
queuedMessages,
guidanceMessages,
onCancelQueuedMessage,
onSendQueuedAsGuidance,
onEditQueuedMessage,
onCancelGuidanceMessage,
}: ConversationQueuePanelProps) {
+ const { t } = useTranslation('common')
+
if (queuedMessages.length === 0 && guidanceMessages.length === 0) {
return null
}
return (
<div
data-testid="conversation-queue-panel"
className="mb-2 rounded-[18px] border border-border bg-base px-3 py-2 shadow-[0_12px_32px_rgba(0,0,0,0.06)]"
>
<div className="flex flex-col gap-1">
{queuedMessages.map(message => (
<QueueRow
key={message.id}
id={message.id}
content={message.content}
status={message.status}
error={message.error}
mode="queue"
+ t={t}
onGuide={onSendQueuedAsGuidance}
onEdit={onEditQueuedMessage}
onCancel={onCancelQueuedMessage}
/>
))}
{guidanceMessages.map(message => (
<QueueRow
key={message.id}
id={message.id}
content={message.content}
status={message.status}
error={message.error}
mode="guidance"
+ t={t}
onCancel={onCancelGuidanceMessage}
/>
))}
</div>
</div>
)
}
interface QueueRowProps {
id: string
content: string
status: QueuedWorkbenchMessage['status'] | GuidanceWorkbenchMessage['status']
error?: string
mode: 'queue' | 'guidance'
+ t: (key: string, fallback: string) => string
onGuide?: (id: string) => void
onEdit?: (id: string) => void
onCancel?: (id: string) => void
}
function QueueRow({
id,
content,
status,
error,
mode,
+ t,
onGuide,
onEdit,
onCancel,
}: QueueRowProps) {
const isBusy = status === 'sending'
const statusText =
status === 'failed'
- ? error ?? '发送失败'
+ ? error ?? t('workbench.queue_send_failed', '发送失败')
: status === 'expired'
- ? error ?? '已过期'
+ ? error ?? t('workbench.queue_expired', '已过期')
: nullThen add the corresponding keys to wework/src/i18n/locales/zh-CN/common.json and wework/src/i18n/locales/en/common.json.
🤖 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/ConversationQueuePanel.tsx` around lines 86 - 91,
Replace the hardcoded Chinese fallbacks in the ConversationQueuePanel by wiring
the i18n translation hook (e.g., import and call useTranslation to get t) at the
component level and use t('common.send_failed') and t('common.expired') (or
whatever key names you choose) instead of '发送失败' and '已过期' when computing
statusText; then add the corresponding keys and translations to
wework/src/i18n/locales/zh-CN/common.json and
wework/src/i18n/locales/en/common.json so both locales provide values for those
keys.
Comment on lines
+241
to
+242
| aria-label={copied ? '已复制' : '复制消息'} | ||
| > |
Contributor
There was a problem hiding this comment.
Hardcoded Chinese aria-labels bypass i18n.
These labels ('已复制' / '复制消息') are hardcoded while the rest of the chat UI resolves copy via useTranslation. Non-Chinese locales will get untranslated screen-reader text. Route these through t(...) like ScrollableMessageArea does for its strings.
🤖 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 241 - 242, The
aria-labels in MessageList.tsx are hardcoded Chinese strings for the copy button
(aria-label using the copied boolean), which bypasses i18n; update the component
to use the existing i18n hook (useTranslation) and replace the literals with
translation lookups (e.g., t('copied') when copied is true and t('copyMessage')
when false), or reuse the same translation keys used by ScrollableMessageArea to
keep labels consistent for all locales.
| } | ||
| }} | ||
| className="h-8 rounded-md bg-[#14b8a6] px-4 text-[13px] font-medium leading-[18px] text-white hover:bg-[#0f9f93] disabled:opacity-50" | ||
| className="h-11 min-w-[44px] rounded-md bg-[#14b8a6] px-4 text-[13px] font-medium leading-[18px] text-white hover:bg-[#0f9f93] disabled:opacity-50" |
Contributor
There was a problem hiding this comment.
Use the dialog primary action variant for the confirm button.
The confirm action is the dialog’s primary action, but it is still implemented as a custom-styled <button> instead of using variant="primary" per the dialog button contract. Please switch this control to the shared button variant path so styling and behavior stay consistent across dialogs.
As per coding guidelines: "Primary action buttons in dialogs MUST use variant="primary" to ensure visual consistency and proper styling".
🤖 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/common/TextInputDialog.tsx` at line 92, The confirm
button in TextInputDialog.tsx is using a custom-styled <button> (the element
with className="h-11 min-w-[44px] ...") but must use the shared Button component
with variant="primary"; replace the raw <button> with the project Button
component (pass variant="primary" and preserve existing props like
onClick/disabled) and remove the custom className so the dialog's primary action
follows the standardized styling and behavior.
This was referenced Jun 4, 2026
This was referenced Jun 30, 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