Refactor chatinput structured state 1132#1221
Open
7vignesh wants to merge 3 commits intoRocketChat:developfrom
Open
Refactor chatinput structured state 1132#12217vignesh wants to merge 3 commits intoRocketChat:developfrom
7vignesh wants to merge 3 commits intoRocketChat:developfrom
Conversation
- Remove typingHandlerLock busy-wait pattern; JS is single-threaded so array operations are already atomic - Add response.ok checks and throw on HTTP errors in sendAttachment, getUserStatus, userInfo, and userData - Fix missing await on response.json() calls in getUserStatus, userInfo, and userData - Use console.error instead of console.log for error logging - Wrap AttachmentPreview submit in try/catch/finally to handle upload failures and correctly reset isPending state Fixes RocketChat#1166
Implement useChatInputState hook to separate quote links from message text, fixing the accumulation bug where multiple quotes were duplicated. Quotes are now resolved at send-time via getFinalMarkdown() instead of being injected into the textarea value. Closes RocketChat#1132
There was a problem hiding this comment.
Pull request overview
Refactors ChatInput quote handling to build quoted markdown at send-time (instead of mutating/injecting quote links into the textarea), and adds initial unit test + Jest/Babel setup to support testing.
Changes:
- Introduces
useChatInputStateand updatesChatInputto use it for text/cursor tracking and forgetFinalMarkdown()quote assembly. - Fixes the prior quote duplication bug by removing mutation inside
Promise.all(...map()). - Adds Jest/Babel configuration and a unit test suite for
useChatInputState.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/react/src/views/ChatInput/ChatInput.js | Switches quote-link assembly to getFinalMarkdown() and synchronizes hook state with the textarea. |
| packages/react/src/views/AttachmentPreview/AttachmentPreview.js | Wraps attachment sending in try/catch/finally to correctly clear pending state and handle failures. |
| packages/react/src/hooks/useChatInputState.js | New hook managing text/cursor state + send-time quote markdown generation. |
| packages/react/src/hooks/tests/useChatInputState.test.js | Adds unit coverage for state updates and quote markdown generation (regression for duplication bug). |
| packages/react/jest.config.js | Adds React package Jest configuration (jsdom, CSS mapper, ESM transform exception). |
| packages/react/babel.config.js | Adjusts Babel behavior for test environment (Node target; omits Emotion preset during tests). |
| packages/api/src/EmbeddedChatApi.ts | Removes busy-wait typing “lock” and adds response.ok checks / improved error handling in some API calls. |
Comments suppressed due to low confidence (1)
packages/react/src/views/ChatInput/ChatInput.js:194
- This effect uses
editMessage,clearInput, andsetEditMessage(and alsosetDisableButton) but only lists[deletedMessage]as dependencies, which will failreact-hooks/exhaustive-depsand can lead to stale reads if the store ever changes how these values are updated. Include the referenced values/functions in the deps array (or refactor to avoid closing over them).
clearInput();
setDisableButton(true);
setEditMessage({});
}
}, [deletedMessage]);
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| return response; | ||
| }); | ||
| if (!response.ok) { | ||
| throw new Error(`Upload failed: ${response.status} ${response.statusText}`); |
Comment on lines
+1241
to
+1243
| if (!response.ok) { | ||
| throw new Error(`getUserStatus failed: ${response.status} ${response.statusText}`); | ||
| } |
Comment on lines
+1261
to
+1264
| if (!response.ok) { | ||
| throw new Error(`userInfo failed: ${response.status} ${response.statusText}`); | ||
| } | ||
| const data = await response.json(); |
Comment on lines
+1281
to
+1284
| if (!response.ok) { | ||
| throw new Error(`userData failed: ${response.status} ${response.statusText}`); | ||
| } | ||
| const data = await response.json(); |
Comment on lines
177
to
181
| } else { | ||
| messageRef.current.value = ''; | ||
| clearInput(); | ||
| } | ||
| }, [editMessage]); |
Comment on lines
55
to
71
| const submit = async () => { | ||
| setIsPending(true); | ||
| await RCInstance.sendAttachment( | ||
| data, | ||
| fileName, | ||
| messageRef.current.value, | ||
| ECOptions?.enableThreads ? threadId : undefined | ||
| ); | ||
| toggle(); | ||
| setData(null); | ||
| if (isPending) { | ||
| try { | ||
| await RCInstance.sendAttachment( | ||
| data, | ||
| fileName, | ||
| messageRef.current.value, | ||
| ECOptions?.enableThreads ? threadId : undefined | ||
| ); | ||
| toggle(); | ||
| setData(null); | ||
| } catch (err) { | ||
| console.error('Attachment upload failed:', err); | ||
| } finally { | ||
| setIsPending(false); | ||
| } | ||
| }; |
| toggle(); | ||
| setData(null); | ||
| } catch (err) { | ||
| console.error('Attachment upload failed:', err); |
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
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
Refactors ChatInput to use a structured state-based approach for managing quotes and cursor position, fixing the quote accumulation bug and improving input stability.
Changes
useChatInputStatehook managing text and cursor as stategetFinalMarkdown()resolves quote links at send-time only (not injected into textarea)Promise.all().map()Closes #1132