From 241d3d6cb00a7504daac0771434f42fb538b199a Mon Sep 17 00:00:00 2001 From: Vignesh G Date: Thu, 5 Mar 2026 18:02:06 +0530 Subject: [PATCH 1/2] fix(api): improve error handling and remove unnecessary spin lock - 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 #1166 --- packages/api/src/EmbeddedChatApi.ts | 38 ++++++++++--------- .../AttachmentPreview/AttachmentPreview.js | 21 +++++----- 2 files changed, 33 insertions(+), 26 deletions(-) diff --git a/packages/api/src/EmbeddedChatApi.ts b/packages/api/src/EmbeddedChatApi.ts index 72e25a046..40be1f8e7 100644 --- a/packages/api/src/EmbeddedChatApi.ts +++ b/packages/api/src/EmbeddedChatApi.ts @@ -7,8 +7,6 @@ import { ApiError, } from "@embeddedchat/auth"; -// mutliple typing status can come at the same time they should be processed in order. -let typingHandlerLock = 0; export default class EmbeddedChatApi { host: string; rid: string; @@ -358,13 +356,7 @@ export default class EmbeddedChatApi { typingUser: string; isTyping: boolean; }) { - // don't wait for more than 2 seconds. Though in practical, the waiting time is insignificant. - setTimeout(() => { - typingHandlerLock = 0; - }, 2000); - // eslint-disable-next-line no-empty - while (typingHandlerLock) {} - typingHandlerLock = 1; + // No lock needed — JS is single-threaded, so array operations are already atomic. // move user to front if typing else remove it. const idx = this.typingUsers.indexOf(typingUser); if (idx !== -1) { @@ -373,7 +365,6 @@ export default class EmbeddedChatApi { if (isTyping) { this.typingUsers.unshift(typingUser); } - typingHandlerLock = 0; const newTypingStatus = cloneArray(this.typingUsers); this.onTypingStatusCallbacks.forEach((callback) => callback(newTypingStatus) @@ -1065,17 +1056,21 @@ export default class EmbeddedChatApi { "description", fileDescription.length !== 0 ? fileDescription : "" ); - const response = fetch(`${this.host}/api/v1/rooms.upload/${this.rid}`, { + const response = await fetch(`${this.host}/api/v1/rooms.upload/${this.rid}`, { method: "POST", body: form, headers: { "X-Auth-Token": authToken, "X-User-Id": userId, }, - }).then((r) => r.json()); - return response; + }); + if (!response.ok) { + throw new Error(`Upload failed: ${response.status} ${response.statusText}`); + } + return await response.json(); } catch (err) { - console.log(err); + console.error(err); + throw err; } } @@ -1243,7 +1238,10 @@ export default class EmbeddedChatApi { }, } ); - const data = response.json(); + if (!response.ok) { + throw new Error(`getUserStatus failed: ${response.status} ${response.statusText}`); + } + const data = await response.json(); return data; } @@ -1260,7 +1258,10 @@ export default class EmbeddedChatApi { }, } ); - const data = response.json(); + if (!response.ok) { + throw new Error(`userInfo failed: ${response.status} ${response.statusText}`); + } + const data = await response.json(); return data; } @@ -1277,7 +1278,10 @@ export default class EmbeddedChatApi { }, } ); - const data = response.json(); + if (!response.ok) { + throw new Error(`userData failed: ${response.status} ${response.statusText}`); + } + const data = await response.json(); return data; } } diff --git a/packages/react/src/views/AttachmentPreview/AttachmentPreview.js b/packages/react/src/views/AttachmentPreview/AttachmentPreview.js index 01123dafb..03d2be53c 100644 --- a/packages/react/src/views/AttachmentPreview/AttachmentPreview.js +++ b/packages/react/src/views/AttachmentPreview/AttachmentPreview.js @@ -54,15 +54,18 @@ const AttachmentPreview = () => { 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); } }; From 8dd55034b9f024d3d2390265d3b4e9b55130b786 Mon Sep 17 00:00:00 2001 From: Vignesh G Date: Thu, 19 Mar 2026 01:18:43 +0530 Subject: [PATCH 2/2] feat(react): refactor ChatInput to use structured state for quotes 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 #1132 --- packages/react/babel.config.js | 16 +- packages/react/jest.config.js | 11 + .../hooks/__tests__/useChatInputState.test.js | 190 ++++++++++++++++++ packages/react/src/hooks/useChatInputState.js | 91 +++++++++ .../react/src/views/ChatInput/ChatInput.js | 61 +++--- 5 files changed, 330 insertions(+), 39 deletions(-) create mode 100644 packages/react/jest.config.js create mode 100644 packages/react/src/hooks/__tests__/useChatInputState.test.js create mode 100644 packages/react/src/hooks/useChatInputState.js diff --git a/packages/react/babel.config.js b/packages/react/babel.config.js index 5ccad93f9..a09c9448a 100644 --- a/packages/react/babel.config.js +++ b/packages/react/babel.config.js @@ -1,14 +1,18 @@ +const isTest = process.env.NODE_ENV === 'test'; + module.exports = { presets: [ [ '@babel/preset-env', - { - modules: false, - bugfixes: true, - targets: { browsers: '> 0.25%, ie 11, not op_mini all, not dead' }, - }, + isTest + ? { targets: { node: 'current' } } + : { + modules: false, + bugfixes: true, + targets: { browsers: '> 0.25%, ie 11, not op_mini all, not dead' }, + }, ], '@babel/preset-react', - '@emotion/babel-preset-css-prop', + ...(isTest ? [] : ['@emotion/babel-preset-css-prop']), ], }; diff --git a/packages/react/jest.config.js b/packages/react/jest.config.js new file mode 100644 index 000000000..609860857 --- /dev/null +++ b/packages/react/jest.config.js @@ -0,0 +1,11 @@ +module.exports = { + testEnvironment: 'jsdom', + // Allow Jest to transform ESM packages that ship /dist/esm/ builds + transformIgnorePatterns: [ + '/node_modules/(?!(react-syntax-highlighter)/)', + ], + moduleNameMapper: { + // Silence CSS/asset imports that aren't relevant to unit tests + '\\.(css|scss|sass)$': 'identity-obj-proxy', + }, +}; diff --git a/packages/react/src/hooks/__tests__/useChatInputState.test.js b/packages/react/src/hooks/__tests__/useChatInputState.test.js new file mode 100644 index 000000000..406c2c65d --- /dev/null +++ b/packages/react/src/hooks/__tests__/useChatInputState.test.js @@ -0,0 +1,190 @@ +import React from 'react'; +import { render, act } from '@testing-library/react'; +import useChatInputState from '../useChatInputState'; + +// --------------------------------------------------------------------------- +// Helper: renders the hook inside a minimal component and returns a live ref +// to the hook's return value. Works with @testing-library/react v12 which +// does not expose renderHook. +// +// IMPORTANT: always read `result.current` AFTER act() — never destructure +// `current` up front, because re-renders reassign `result.current` to the +// new hook return value while a destructured copy keeps pointing to the old +// object. +// --------------------------------------------------------------------------- +function renderHookShim() { + const result = { current: null }; + + const TestComponent = () => { + result.current = useChatInputState(); + return null; + }; + + render(); + return result; +} + +// --------------------------------------------------------------------------- +// 1. Initial state +// --------------------------------------------------------------------------- +describe('useChatInputState – initial state', () => { + it('starts with empty text and zero cursor position', () => { + const result = renderHookShim(); + expect(result.current.inputState.text).toBe(''); + expect(result.current.inputState.cursorPosition).toBe(0); + }); +}); + +// --------------------------------------------------------------------------- +// 2. setText +// --------------------------------------------------------------------------- +describe('useChatInputState – setText', () => { + it('updates the text field', () => { + const result = renderHookShim(); + act(() => result.current.setText('hello world')); + expect(result.current.inputState.text).toBe('hello world'); + }); + + it('does not change cursor position when only text is set', () => { + const result = renderHookShim(); + act(() => result.current.setText('some text')); + expect(result.current.inputState.cursorPosition).toBe(0); + }); +}); + +// --------------------------------------------------------------------------- +// 3. setCursorPosition +// --------------------------------------------------------------------------- +describe('useChatInputState – setCursorPosition', () => { + it('updates cursor position', () => { + const result = renderHookShim(); + act(() => result.current.setCursorPosition(5)); + expect(result.current.inputState.cursorPosition).toBe(5); + }); + + it('does not change text when only cursor is updated', () => { + const result = renderHookShim(); + act(() => result.current.setText('hello')); + act(() => result.current.setCursorPosition(3)); + expect(result.current.inputState.text).toBe('hello'); + expect(result.current.inputState.cursorPosition).toBe(3); + }); +}); + +// --------------------------------------------------------------------------- +// 4. clearInput +// --------------------------------------------------------------------------- +describe('useChatInputState – clearInput', () => { + it('resets text and cursorPosition to initial values', () => { + const result = renderHookShim(); + act(() => result.current.setText('typing something')); + act(() => result.current.setCursorPosition(8)); + act(() => result.current.clearInput()); + expect(result.current.inputState.text).toBe(''); + expect(result.current.inputState.cursorPosition).toBe(0); + }); +}); + +// --------------------------------------------------------------------------- +// 5. getFinalMarkdown – the core bug fix +// +// The old code mutated `quotedMessages` inside Promise.all.map() and then +// joined the cumulative array, producing duplicated link prefixes for every +// quote after the first. These tests verify the new behaviour is correct. +// --------------------------------------------------------------------------- +describe('useChatInputState – getFinalMarkdown', () => { + const makeLinkFn = (map) => (id) => Promise.resolve(map[id]); + + it('returns plain text when there are no quotes', async () => { + const hookRef = renderHookShim(); + const result = await hookRef.current.getFinalMarkdown('hello', [], makeLinkFn({})); + expect(result).toBe('hello'); + }); + + it('returns plain text when quotes array is null/undefined', async () => { + const hookRef = renderHookShim(); + const result = await hookRef.current.getFinalMarkdown( + 'hello', + null, + makeLinkFn({}) + ); + expect(result).toBe('hello'); + }); + + it('prepends a single quote link separated by a newline', async () => { + const hookRef = renderHookShim(); + const quotes = [{ _id: 'msg1', msg: 'original message', attachments: undefined }]; + const linkMap = { msg1: 'https://host/channel/general/?msg=msg1' }; + + const result = await hookRef.current.getFinalMarkdown( + 'my reply', + quotes, + makeLinkFn(linkMap) + ); + + expect(result).toBe( + '[ ](https://host/channel/general/?msg=msg1)\nmy reply' + ); + }); + + it('prepends multiple quote links WITHOUT duplication (old bug regression)', async () => { + // OLD behaviour: link1 was emitted twice → "[ ](link1)[ ](link1)[ ](link2)\ntext" + // NEW behaviour: each link appears exactly once → "[ ](link1)[ ](link2)\ntext" + const hookRef = renderHookShim(); + const quotes = [ + { _id: 'msg1', msg: 'first quoted', attachments: undefined }, + { _id: 'msg2', msg: 'second quoted', attachments: undefined }, + ]; + const linkMap = { + msg1: 'https://host/channel/general/?msg=msg1', + msg2: 'https://host/channel/general/?msg=msg2', + }; + + const result = await hookRef.current.getFinalMarkdown( + 'my reply', + quotes, + makeLinkFn(linkMap) + ); + + expect(result).toBe( + '[ ](https://host/channel/general/?msg=msg1)' + + '[ ](https://host/channel/general/?msg=msg2)' + + '\nmy reply' + ); + // Ensure first link does NOT appear twice (the old bug) + const occurrences = (result.match(/msg1/g) || []).length; + expect(occurrences).toBe(1); + }); + + it('skips quotes that have neither msg nor attachments', async () => { + const hookRef = renderHookShim(); + const quotes = [ + { _id: 'msg1', msg: undefined, attachments: undefined }, + { _id: 'msg2', msg: 'valid message', attachments: undefined }, + ]; + const linkMap = { msg2: 'https://host/channel/general/?msg=msg2' }; + + const result = await hookRef.current.getFinalMarkdown( + 'reply', + quotes, + makeLinkFn(linkMap) + ); + + // Only msg2 link should appear; msg1 had no content so it's skipped + expect(result).toBe('[ ](https://host/channel/general/?msg=msg2)\nreply'); + expect(result).not.toContain('msg1'); + }); + + it('returns plain text if all quotes have no msg or attachments', async () => { + const hookRef = renderHookShim(); + const quotes = [{ _id: 'msg1', msg: undefined, attachments: undefined }]; + + const result = await hookRef.current.getFinalMarkdown( + 'just text', + quotes, + makeLinkFn({}) + ); + + expect(result).toBe('just text'); + }); +}); diff --git a/packages/react/src/hooks/useChatInputState.js b/packages/react/src/hooks/useChatInputState.js new file mode 100644 index 000000000..2110ec534 --- /dev/null +++ b/packages/react/src/hooks/useChatInputState.js @@ -0,0 +1,91 @@ +import { useReducer, useCallback } from 'react'; + +const initialState = { + text: '', + cursorPosition: 0, +}; + +function reducer(state, action) { + switch (action.type) { + case 'SET_TEXT': + return { ...state, text: action.payload }; + case 'SET_CURSOR': + return { ...state, cursorPosition: action.payload }; + case 'CLEAR': + return initialState; + default: + return state; + } +} + +/** + * useChatInputState + * + * Manages the ChatInput's text and cursor position as structured state + * instead of reading them directly from the DOM ref on every access. + * + * Key benefit: quote links are assembled at send-time via getFinalMarkdown(), + * so they are never injected into the textarea value — preventing accidental + * corruption while the user edits the message. + */ +const useChatInputState = () => { + const [inputState, dispatch] = useReducer(reducer, initialState); + + const setText = useCallback( + (text) => dispatch({ type: 'SET_TEXT', payload: text }), + [] + ); + + const setCursorPosition = useCallback( + (pos) => dispatch({ type: 'SET_CURSOR', payload: pos }), + [] + ); + + const clearInput = useCallback(() => dispatch({ type: 'CLEAR' }), []); + + /** + * Builds the final markdown string to send. + * + * Quote links are resolved here, at send-time only, so the textarea always + * contains just the user's plain text. This avoids the two bugs present in + * the previous string-manipulation approach: + * 1. Accumulated duplicates from mutating `quotedMessages` inside .map() + * and then joining the cumulative array values. + * 2. Hidden markdown links being silently broken by cursor movement or + * editing inside the textarea. + * + * @param {string} text - The trimmed message text from the textarea + * @param {Array} quotes - Quote objects from the message store + * @param {Function} getMessageLink - Async fn: (id) => permalink string + * @returns {Promise} Final markdown string ready to send + */ + const getFinalMarkdown = useCallback( + async (text, quotes, getMessageLink) => { + if (!quotes || quotes.length === 0) return text; + + const quoteLinks = await Promise.all( + quotes.map(async ({ _id, msg, attachments }) => { + if (msg || attachments) { + const link = await getMessageLink(_id); + return `[ ](${link})`; + } + return ''; + }) + ); + + const quotePart = quoteLinks.filter(Boolean).join(''); + return quotePart ? `${quotePart}\n${text}` : text; + }, + [] + ); + + return { + inputState, + setText, + setCursorPosition, + clearInput, + getFinalMarkdown, + }; +}; + +export default useChatInputState; diff --git a/packages/react/src/views/ChatInput/ChatInput.js b/packages/react/src/views/ChatInput/ChatInput.js index e753b689a..638ba6fec 100644 --- a/packages/react/src/views/ChatInput/ChatInput.js +++ b/packages/react/src/views/ChatInput/ChatInput.js @@ -32,6 +32,7 @@ import QuoteMessage from '../QuoteMessage/QuoteMessage'; import { getChatInputStyles } from './ChatInput.styles'; import useShowCommands from '../../hooks/useShowCommands'; import useSearchMentionUser from '../../hooks/useSearchMentionUser'; +import useChatInputState from '../../hooks/useChatInputState'; import formatSelection from '../../lib/formatSelection'; import { parseEmoji } from '../../lib/emoji'; @@ -127,6 +128,9 @@ const ChatInput = ({ scrollToBottom, clearUnreadDividerRef }) => { const userInfo = { _id: userId, username, name }; + const { setText, setCursorPosition, clearInput, getFinalMarkdown } = + useChatInputState(); + const dispatchToastMessage = useToastBarDispatch(); const showCommands = useShowCommands( commands, @@ -161,14 +165,18 @@ const ChatInput = ({ scrollToBottom, clearUnreadDividerRef }) => { useEffect(() => { if (editMessage.attachments) { - messageRef.current.value = + const editText = editMessage.attachments[0]?.description || editMessage.msg; + messageRef.current.value = editText; + setText(editText); messageRef.current.focus(); } else if (editMessage.msg) { messageRef.current.value = editMessage.msg; + setText(editMessage.msg); messageRef.current.focus(); } else { messageRef.current.value = ''; + clearInput(); } }, [editMessage]); @@ -179,6 +187,7 @@ const ChatInput = ({ scrollToBottom, clearUnreadDividerRef }) => { deletedMessage._id === editMessage._id ) { messageRef.current.value = ''; + clearInput(); setDisableButton(true); setEditMessage({}); } @@ -213,6 +222,7 @@ const ChatInput = ({ scrollToBottom, clearUnreadDividerRef }) => { const textToAttach = () => { const message = messageRef.current.value.trim(); messageRef.current.value = ''; + clearInput(); setEditMessage({}); setIsMsgLong(false); const messageBlob = new Blob([message], { type: 'text/plain' }); @@ -285,37 +295,17 @@ const ChatInput = ({ scrollToBottom, clearUnreadDividerRef }) => { const handleSendNewMessage = async (message) => { messageRef.current.value = ''; setDisableButton(true); - - let pendingMessage = ''; - let quotedMessages = ''; - - if (quoteMessage.length > 0) { - // for (const quote of quoteMessage) { - // const { msg, attachments, _id } = quote; - // if (msg || attachments) { - // const msgLink = await getMessageLink(_id); - // quotedMessages += `[ ](${msgLink})`; - // } - // } - - const quoteArray = await Promise.all( - quoteMessage.map(async (quote) => { - const { msg, attachments, _id } = quote; - if (msg || attachments) { - const msgLink = await getMessageLink(_id); - quotedMessages += `[ ](${msgLink})`; - } - return quotedMessages; - }) - ); - quotedMessages = quoteArray.join(''); - pendingMessage = createPendingMessage( - `${quotedMessages}\n${message}`, - userInfo - ); - } else { - pendingMessage = createPendingMessage(message, userInfo); - } + clearInput(); + + // getFinalMarkdown resolves quote links at send-time only, keeping the + // textarea text clean and avoiding the previous accumulation bug where + // mutating quotedMessages inside .map() produced duplicate link prefixes. + const finalMsg = await getFinalMarkdown( + message, + quoteMessage, + getMessageLink + ); + const pendingMessage = createPendingMessage(finalMsg, userInfo); if (ECOptions.enableThreads && threadId) { pendingMessage.tmid = threadId; @@ -339,6 +329,7 @@ const ChatInput = ({ scrollToBottom, clearUnreadDividerRef }) => { const handleEditMessage = async (message) => { messageRef.current.value = ''; + clearInput(); setDisableButton(true); const editMessageId = editMessage._id; setEditMessage({}); @@ -363,6 +354,7 @@ const ChatInput = ({ scrollToBottom, clearUnreadDividerRef }) => { if (commands.find((c) => c.command === command.replace('/', ''))) { messageRef.current.value = ''; + clearInput(); setDisableButton(true); setEditMessage({}); await execCommand(command.replace('/', ''), params); @@ -416,7 +408,10 @@ const ChatInput = ({ scrollToBottom, clearUnreadDividerRef }) => { const onTextChange = (e, val) => { sendTypingStart(); const message = val || e.target.value; - messageRef.current.value = parseEmoji(message); + const parsed = parseEmoji(message); + messageRef.current.value = parsed; + setText(parsed); + if (e?.target) setCursorPosition(e.target.selectionStart ?? 0); setDisableButton(!messageRef.current.value.length); if (e !== null) { handleNewLine(e, false);