Conversation
lsm
left a comment
There was a problem hiding this comment.
Review: Replace ThreadedChatComposer with ChatComposer in Space task view
Overall the approach is sound — creating a TaskSessionChatComposer wrapper that delegates to ChatComposer with feature flags disabled is the right pattern. The new test suite covers the prop threading well. A few issues need attention before merging:
Must fix
-
Delete dead code:
ThreadedChatComposer.tsxandThreadedChatComposer.test.tsxare no longer imported anywhere. They should be deleted in this PR rather than left as dead code. -
Draft loss on send failure (behavioral regression): The old
ThreadedChatComposeronly cleared the draft on successful send (if (sent) { setDraft('') }). The new flow viaMessageInput.handleSubmitcallsclearDraft()beforeawait onSend(...), so ifsendThreadMessagereturnsfalse(failure), the draft is already gone. The user sees the error banner but loses their message. This should be addressed — either by not clearing the draft pre-send for task sessions, or by restoring it on failure. -
isProcessinghardcoded tofalse:TaskSessionChatComposerpassesisProcessing={false}toChatComposer. This means theSessionStatusBarwill never show a processing indicator even when the task agent is actively working. The parentSpaceTaskPanehas access to agent working state — consider wiring it through. At minimum,isAgentWorkingfrom the old component was used for the interrupt button, and it should be connected.
Nice to have
-
@ts-nocheckin test file: The test file uses// @ts-nocheckat the top. The mocked modules should type-check fine on their own (the mocks match the return signatures). Consider removing the directive — if it fails, the specific type issues should be fixed rather than suppressed. -
Dual
useModelSwitcherinstantiation: BothTaskSessionChatComposerandMessageInputcalluseModelSwitcher(sessionId)for the same session. This is wasteful (two subscriptions/RPC calls) but not functionally broken since they operate independently. The model data from the outer hook flows toSessionStatusBar, whileMessageInputneeds its own instance for theInputActionsMenu. Consider whetherChatComposercould pass model data down toMessageInputto eliminate the duplication. -
Reference autocomplete suppression: When workflow agents exist, typing
@now triggers the agent mention autocomplete and completely suppresses the reference autocomplete (@ref{...}). This is likely intentional but worth documenting as a known trade-off.
| import { PendingCompletionActionBanner } from './PendingCompletionActionBanner'; | ||
| import { PendingTaskCompletionBanner } from './PendingTaskCompletionBanner'; | ||
| import { ThreadedChatComposer } from './ThreadedChatComposer'; | ||
| import { TaskSessionChatComposer } from './TaskSessionChatComposer'; |
There was a problem hiding this comment.
Dead code: ThreadedChatComposer.tsx is no longer imported anywhere (the only consumer SpaceTaskPane.tsx now imports TaskSessionChatComposer). Delete this file and its test ThreadedChatComposer.test.tsx in this PR.
There was a problem hiding this comment.
Fixed — deleted both ThreadedChatComposer.tsx and ThreadedChatComposer.test.tsx in the follow-up commit.
| _images?: MessageImage[], | ||
| _deliveryMode?: MessageDeliveryMode | ||
| ): Promise<void> => { | ||
| await onSend(content); |
There was a problem hiding this comment.
Behavioral regression: The old ThreadedChatComposer only cleared the draft on successful send (if (sent) { setDraft('') }). MessageInput.handleSubmit (line 340 in the parent) calls clearDraft() before await onSend(...), so if sendThreadMessage returns false (failure), the user's message is already gone. Consider either: (a) passing the boolean return through and only clearing on success, or (b) restoring the draft on failure.
There was a problem hiding this comment.
Fixed. Changed MessageInput.onSend return type to Promise<void | boolean>. handleSubmit now saves the content before clearing optimistically, then restores it via setContent(savedContent) when the result is false. TaskSessionChatComposer.handleSend now returns the boolean from sendThreadMessage so the signal propagates correctly.
| sessionId={sessionId} | ||
| readonly={false} | ||
| isProcessing={false} | ||
| features={{ |
There was a problem hiding this comment.
isProcessing hardcoded: The old ThreadedChatComposer used isAgentWorking.value for the interrupt button. Here isProcessing={false} means SessionStatusBar will never show a processing indicator. Consider wiring the actual agent working state through (the parent SpaceTaskPane has access to it via isAgentWorking or the task status).
There was a problem hiding this comment.
Fixed. Added isProcessing: boolean to TaskSessionChatComposerProps and passed isAgentActive from SpaceTaskPane (which already computes it from activity member states). ChatComposer now receives the live processing state and SessionStatusBar will correctly show the processing indicator.
| @@ -0,0 +1,123 @@ | |||
| // @ts-nocheck | |||
There was a problem hiding this comment.
Consider removing @ts-nocheck. The mocks match the return signatures and should type-check. If there are specific type errors, fix them individually rather than suppressing the whole file.
There was a problem hiding this comment.
Removed. Exported ChatComposerProps from ChatComposer.tsx, typed lastChatComposerProps as ChatComposerProps | null, and updated all assertions to use optional chaining. Also added an isProcessing forwarding test.
33a4b1b to
c03102b
Compare
Introduces TaskSessionChatComposer — a thin wrapper around the rich ChatComposer — for the Space task pane, replacing the minimal ThreadedChatComposer. Adds agent @mention autocomplete support to MessageInput/InputTextarea/ChatComposer so workflow agents can be mentioned directly in the full composer experience. - Add agentMentionCandidates prop + state to MessageInput with @-query detection, keyboard navigation (↑↓/Enter/Esc), and handleAgentMentionSelect - Add showAgentMentionAutocomplete/agentMentionCandidates/etc. props to InputTextarea; render MentionAutocomplete with highest priority over reference and command autocomplete - Add agentMentionCandidates and inputPlaceholder props to ChatComposer and thread them through to MessageInput - Create TaskSessionChatComposer with features disabled, model switcher, placeholder derived from hasTaskAgentSession, and onSend adapter - Update SpaceTaskPane to use TaskSessionChatComposer, removing the wrapping absolute div (ChatComposer owns its own positioning) - Add TaskSessionChatComposer unit tests (11 cases)
…atComposer ChatComposer renders absolute-positioned, so an external error banner in TaskSessionChatComposer would be hidden behind the footer. Add errorMessage prop to ChatComposer to render the banner inside the footer div, and thread it through TaskSessionChatComposer.
- Delete ThreadedChatComposer.tsx and its test (dead code, no longer imported) - Fix draft-loss regression: MessageInput.onSend now accepts boolean|void return; handleSubmit saves content before clearing and restores on false return so task send failures preserve the user's message - Wire isProcessing from SpaceTaskPane.isAgentActive instead of hardcoding false; SessionStatusBar now shows correct processing indicator for task sessions - Remove @ts-nocheck from test file; use typed ChatComposerProps capture and add isProcessing forwarding test
c03102b to
7c21d1d
Compare
lsm
left a comment
There was a problem hiding this comment.
Re-review: LGTM ✅
All 4 items from the previous review are addressed:
- Dead code deleted —
ThreadedChatComposer.tsxand its test removed. - Draft loss fixed —
MessageInput.handleSubmitsaves content before clearing, restores viasetContentwhenonSendreturnsfalse. Return type widened toPromise<void | boolean>acrossChatComposer→MessageInput→TaskSessionChatComposer. isProcessingwired — now passed fromSpaceTaskPaneasisAgentActive(derived from activity member states).@ts-nocheckremoved —ChatComposerPropsexported, mock properly typed, new forwarding test added (12 tests, up from 11).
All checks pass: tests ✅, lint ✅, typecheck ✅, PR mergeable ✅.
Replaces the minimal
ThreadedChatComposerin the Space task pane with the richChatComposerexperience, which includes theSessionStatusBar(model switcher, context usage) and fullMessageInput(attachments, command autocomplete, interrupt, rewind mode).Changes
MessageInput: newagentMentionCandidatesandplaceholderprops;@-query detection wired into content change handler with keyboard navigation (↑↓/Enter/Esc) before reference and command autocomplete.InputTextarea: new agent mention autocomplete props;MentionAutocompleterenders with highest priority over reference and command autocomplete.ChatComposer: threadsagentMentionCandidatesandinputPlaceholderdown toMessageInput.TaskSessionChatComposer(new): wrapsChatComposerwith all non-applicable features disabled,useModelSwitcherfor the task session, placeholder driven byhasTaskAgentSession, and anonSendadapter mapping the boolean-return contract toChatComposer's async void signature.SpaceTaskPane: swapsThreadedChatComposerforTaskSessionChatComposer; removes the now-redundant absolute wrapper div sinceChatComposerowns its own positioning.TaskSessionChatComposer.test.tsx(new): 11 unit tests covering rendering, prop threading, error banner, disabled state, and placeholder selection.All pre-existing tests pass; no regressions.