Repository: tjtanjin/react-chatbotify
Pull request #180 (2024-10-10 06:09:15)

fix: textArea is disabled after chatHistory is loaded
#### Description

`setTextAreaDisabled(settings.chatInput?.disabled)` is not called when the chatHistory is loaded.

Closes [#172](https://github.com/tjtanjin/react-chatbotify/issues/172)

#### What change does this PR introduce?

- Added a useState variable in BotStatesContext.tsx to track chatHistory being loaded.
- Added a function call setIsChatHistoryLoaded(true) after setIsLoadingChatHistory(false).
- Added `useEffect()` to call `setTextAreaDisabled(false)` after chatHistory is done loading.

Please select the relevant option(s).

- [x] Bug fix (non-breaking change which fixes an issue)
- [ ] New feature (non-breaking change which adds functionality)
- [ ] Breaking change (fix or feature that would cause existing functionality to not work as expected)
- [ ] Documentation update (changes to docs/code comments)

#### What is the proposed approach?

- Implement checking when chatHistory is done loading with a useState variable.
- Enable textArea when chatHistory is done loading.
- isChatHistoryLoaded stays remain true because there is no feature to hide the chat history.
- isChatHistoryLoaded can be set to false with hide chat history button (possible feature).

#### Checklist:

- [x] The commit message follows our adopted [guidelines](https://www.conventionalcommits.org/en/v1.0.0/)
- [x] Testing has been done for the change(s) added (for bug fixes/features)
- [x] Relevant comments/docs have been added/updated (for bug fixes/features)

yaminmomo15: @tjtanjin  This is strange. Cypress test is passed on my machine with React 19 RC. Please let me know what I can do. 😅 

<img width="1552" alt="image" src="https://github.com/user-attachments/assets/aa6388de-37ee-4d98-a79e-9823757f48dc">


tjtanjin: > @tjtanjin  This is strange. Cypress test is passed on my machine with React 19 RC. Please let me know what I can do. 😅 
> 
> 
> 
> <img width="1552" alt="image" src="https://github.com/user-attachments/assets/aa6388de-37ee-4d98-a79e-9823757f48dc">
> 
> 
> 
> 

Test cases are a little flaky. A re-run fixed it. With regard to your PR, I believe this will re-introduce the bugs fixed in v2.0.0-beta.15 🥹 Your current approach means that when chat history is loaded, the text area will be enabled/disabled based on settings. This is not always the case, because a conversation block also allows a `chatDisabled` attribute, which takes precedence over settings (i.e. follows order of specificity).
tjtanjin: Hey @yaminmomo15, I think it's worth considering the context of this feature:

Firstly, the behavior we want to achieve is for text area to be disabled when chat history is loading, and then for it to resume its previous state once loading is done. This is so users don't end up sending stuffs into the chat window while chat history messages are also being added, which may have implications on scroll behavior. This was true in the early days of the library, but it has been a long time since then.

Regardless, it is important to note that the previous state of the text area can be enabled or disabled, and this depends on the following:
1) Whether `chatDisabled` is specified for the current block
2) Settings (if the block has no `chatDisabled`)

This is why this existing PR will break, as it only relies on point 2.

I've said a bunch above but instead of the existing solution, let's think over it again. What we're trying to achieve here is this:
- Users cannot send anything into the chat while chat history messages are being loaded/added

To achieve this, we **don't actually have to disable the text area**. It suffices for us to prevent the users from sending input into the chat. There are currently 4 kinds of input (checkbox, options, text area and file uploads). Instead of disabling the text area (which is not a great experience if the user wants to just continue typing without sending yet), we catch and block submission actions (mainly via checking of `isLoadingChatHistory`). This involves looking into the 4 kinds of input above so it's a fair bit of work, but it's a cleaner and less intrusive approach. Let me know if you're still keen to explore this, because it will take some effort. Otherwise I can take this up as well. Appreciate the bug report!

yaminmomo15: > I've said a bunch above but instead of the existing solution, let's think over it again. What we're trying to achieve here is this:
> 
> * Users cannot send anything into the chat while chat history messages are being loaded/added
> 
> To achieve this, we **don't actually have to disable the text area**. It suffices for us to prevent the users from sending input into the chat. There are currently 4 kinds of input (checkbox, options, text area and file uploads). Instead of disabling the text area (which is not a great experience if the user wants to just continue typing without sending yet), we catch and block submission actions (mainly via checking of `isLoadingChatHistory`). More specifically, the submission actions are:
> 
> * Clicking checkbox
> * Clicking options
> * Clicking send button
> * Clicking file upload
> 
> The checks can be targeted at blocking the above actions, but this involves looking into them which may be a fair bit of work if you are unfamiliar with the repository. That said, this is a cleaner and less intrusive approach, because a user should still be allowed to type even while chat history is loading. Let me know if you're still keen to explore this, because it will take some effort. Otherwise I can take this up as well. Appreciate the bug report!

@tjtanjin There are other things worth noting. 

ChatHistoryButton will only appear after user refreshed the page. At the moment, refreshing the page will also reset the conversation path to "start" so the new approach only makes sense if there is a feature to continue the conversion.

Also chatDisabled is not true in "start" path so my current PR can still act as a temporary fix.

If you want to go with the new approach, it is best if I hand this back to you as it needs other features.
yaminmomo15: I tested again and got your point now. I overlooked that fact that the ChatHistoryButton can also be clicked when the user is half way through a new conversion. chatDisabled also needs to be considered.
tjtanjin: > I tested again and got your point now. I overlooked that fact that the ChatHistoryButton can also be clicked when the user is half way through a new conversion. chatDisabled also needs to be considered.
> 
> I think it is better to go with the new approach. I will try it first before handing back to you. @tjtanjin

Hey @yaminmomo15, before you try the new approach, can you do me a favor:
- Try disabling this behavior entirely (i.e. keep everything enabled while chat history messages are being loaded/added
- Then try loading chat history messages while sending new messages at the same time (might be easier to test if you increase the 500ms delay to a few seconds for loading chat history messages)

I say this because the decision to have this disable behavior with chat history was taken a long time ago and I frankly can't recall the actual reason for it. If there are no buggy behaviors, this might not even be necessary anymore.
yaminmomo15: > > I tested again and got your point now. I overlooked that fact that the ChatHistoryButton can also be clicked when the user is half way through a new conversion. chatDisabled also needs to be considered.
> > I think it is better to go with the new approach. I will try it first before handing back to you. @tjtanjin
> 
> Hey @yaminmomo15, before you try the new approach, can you do me a favor:
> 
> * Try disabling this behavior entirely (i.e. keep everything enabled while chat history messages are being loaded/added
> * Then try loading chat history messages while sending new messages **at the same time** (might be easier to test if you increase the 500ms delay to a few seconds for loading chat history messages)
> 
> I say this because the decision to have this disable behavior with chat history was taken a long time ago and I frankly can't recall the actual reason for it. If there are no buggy behaviors, this might not even be necessary anymore.

Sure, I will disable that behavior and test to see if it can be removed without having bugs.
yaminmomo15: I tested with the textAreaDisabled feature removed when chat history is loading. User interaction can continue normally and doesn't affect the loading chat history. 

And, loading the chat history doen't interfere with the user's interaction(typing, checkbox, options, sending, and file upload) either, so it makes sense to remove the feature. 

I have updated the PR with the changes. Please take a look and let me know. @tjtanjin 
yaminmomo15: @tjtanjin it's ready for review now.

Patch:
diff --git a/src/hooks/internal/useChatHistoryInternal.ts b/src/hooks/internal/useChatHistoryInternal.ts
index 7101eff..da3022b 100644
--- a/src/hooks/internal/useChatHistoryInternal.ts
+++ b/src/hooks/internal/useChatHistoryInternal.ts
@@ -25,8 +25,6 @@ export const useChatHistoryInternal = () => {
 	const {
 		isLoadingChatHistory,
 		setIsLoadingChatHistory,
-		textAreaDisabled,
-		setTextAreaDisabled
 	} = useBotStatesContext();
 
 	// handles rcb events
@@ -51,10 +49,8 @@ export const useChatHistoryInternal = () => {
 			}
 		}
 		setIsLoadingChatHistory(true);
-		const prevTextAreaDisabled = textAreaDisabled;
-		setTextAreaDisabled(true);
-		loadChatHistory(settings, styles, chatHistory, setMessages, prevTextAreaDisabled, setTextAreaDisabled);
-	}, [settings, styles, setMessages, setTextAreaDisabled]);
+		loadChatHistory(settings, styles, chatHistory, setMessages);
+	}, [settings, styles, setMessages]);
 
 	return { isLoadingChatHistory, setIsLoadingChatHistory, showChatHistory };
 };
diff --git a/src/services/ChatHistoryService.tsx b/src/services/ChatHistoryService.tsx
index bba2112..6c39834 100644
--- a/src/services/ChatHistoryService.tsx
+++ b/src/services/ChatHistoryService.tsx
@@ -128,8 +128,7 @@ const parseMessageToString = (message: Message) => {
  * @param setTextAreaDisabled setter for enabling/disabling user text area
  */
 const loadChatHistory = (settings: Settings, styles: Styles, chatHistory: Message[],
-	setMessages: Dispatch<SetStateAction<Message[]>>, prevTextAreaDisabled: boolean,
-	setTextAreaDisabled: Dispatch<SetStateAction<boolean>>) => {
+	setMessages: Dispatch<SetStateAction<Message[]>>) => {
 
 	historyLoaded = true;
 	if (chatHistory != null) {
@@ -160,7 +159,6 @@ const loadChatHistory = (settings: Settings, styles: Styles, chatHistory: Messag
 					}
 					return [...parsedMessages, lineBreakMessage, ...prevMessages];
 				});
-				setTextAreaDisabled(prevTextAreaDisabled ?? settings.chatInput?.disabled ?? false);
 			}, 500)
 		} catch {
 			// remove chat history on error (to address corrupted storage values)
diff --git a/src/utils/messageBuilder.ts b/src/utils/messageBuilder.ts
index d3caff3..d50c480 100644
--- a/src/utils/messageBuilder.ts
+++ b/src/utils/messageBuilder.ts
@@ -1,5 +1,3 @@
-import { isValidElement } from "react";
-
 import { generateSecureUUID } from "./idGenerator";
 
 /**
@@ -13,7 +11,7 @@ export const createMessage = (content: string | JSX.Element, sender: string, ) =
 		id: generateSecureUUID(),
 		content,
 		sender,
-		type: isValidElement(content) ? "object" : "string",
+		type: typeof content === "string" ? "string" : "object",
 		timestamp: new Date().toUTCString()
 	};
 }
\ No newline at end of file


Test patch:
diff --git a/__tests__/hooks/internal/useChatHistoryInternal.test.ts b/__tests__/hooks/internal/useChatHistoryInternal.test.ts
index 61e33f8..3bace45 100644
--- a/__tests__/hooks/internal/useChatHistoryInternal.test.ts
+++ b/__tests__/hooks/internal/useChatHistoryInternal.test.ts
@@ -27,25 +27,25 @@ describe("useChatHistoryInternal Hook", () => {
 
 	// initial values
 	const initialIsLoadingChatHistory = false;
-    const initialChatHistory = [
-        {
-            id: generateSecureUUID(),
-            sender: "user",
-            content: "Hello",
-            type: "string",
-            timestamp: new Date().toUTCString()
-        },
-        {
-            id: generateSecureUUID(),
-            sender: "bot",
-            content: "Hi there!",
-            type: "string",
-            timestamp: new Date().toUTCString()
-        },
-    ];
+	const initialChatHistory = [
+		{
+			id: generateSecureUUID(),
+			sender: "user",
+			content: "Hello",
+			type: "string",
+			timestamp: new Date().toUTCString()
+		},
+		{
+			id: generateSecureUUID(),
+			sender: "bot",
+			content: "Hi there!",
+			type: "string",
+			timestamp: new Date().toUTCString()
+		},
+	];
 
 	it("should load chat history correctly, change state and emit rcb-load-chat-history event", async () => {
-        // mocks rcb event handler
+		// mocks rcb event handler
 		const callRcbEventMock = jest.fn().mockReturnValue({ defaultPrevented: false });
 		mockUseRcbEventInternal.mockReturnValue({
 			callRcbEvent: callRcbEventMock,
@@ -56,9 +56,8 @@ describe("useChatHistoryInternal Hook", () => {
 
 		// mocks loadChatHistory to resolve successfully
 		mockLoadChatHistory.mockImplementation(
-			(settings, styles, chatHistory, setMessages, prevTextAreaDisabled, setTextAreaDisabled) => {
+			(settings, styles, chatHistory, setMessages) => {
 				setMessages(chatHistory);
-				setTextAreaDisabled(false);
 				return Promise.resolve();
 			}
 		);
@@ -82,20 +81,18 @@ describe("useChatHistoryInternal Hook", () => {
 		// checks if get history messages was called
 		expect(mockGetHistoryMessages).toHaveBeenCalledTimes(1);
 
-        // checks if callRcbEvent was called with rcb-load-chat-history and correct arguments
+		// checks if callRcbEvent was called with rcb-load-chat-history and correct arguments
 		expect(callRcbEventMock).toHaveBeenCalledWith(RcbEvent.LOAD_CHAT_HISTORY, {});
-		
-        // checks if load chat history was called
-        expect(mockLoadChatHistory).toHaveBeenCalledWith(
+
+		// checks if load chat history was called
+		expect(mockLoadChatHistory).toHaveBeenCalledWith(
 			MockDefaultSettings,
 			expect.any(Object),
 			initialChatHistory,
 			expect.any(Function),
-			expect.any(Boolean),
-			expect.any(Function)
 		);
 
-        // checks if history is being loaded
+		// checks if history is being loaded
 		expect(result.current.isLoadingChatHistory).toBe(true);
 	});
 
@@ -125,13 +122,13 @@ describe("useChatHistoryInternal Hook", () => {
 		// checks if get history messages was called
 		expect(mockGetHistoryMessages).toHaveBeenCalledTimes(1);
 
-        // checks if callRcbEvent was called with rcb-load-chat-history and correct arguments
+		// checks if callRcbEvent was called with rcb-load-chat-history and correct arguments
 		expect(callRcbEventMock).toHaveBeenCalledWith(RcbEvent.LOAD_CHAT_HISTORY, {});
 
-        // checks if load chat history was not called
+		// checks if load chat history was not called
 		expect(mockLoadChatHistory).not.toHaveBeenCalled();
 
-        // checks if history is being loaded
+		// checks if history is being loaded
 		expect(result.current.isLoadingChatHistory).toBe(false);
 	});
 });
diff --git a/__tests__/utils/messageBuilder.test.ts b/__tests__/utils/messageBuilder.test.ts
new file mode 100644
index 0000000..d92dd0d
--- /dev/null
+++ b/__tests__/utils/messageBuilder.test.ts
@@ -0,0 +1,161 @@
+import React from "react";
+
+import { createMessage } from "../../src/utils/messageBuilder";
+import { generateSecureUUID } from "../../src/utils/idGenerator";
+
+// mocks id generator
+jest.mock("../../src/utils/idGenerator");
+const mockedGenerateSecureUUID = generateSecureUUID as jest.MockedFunction<typeof generateSecureUUID>;
+
+/**
+ * Test for message builder.
+ */
+describe("createMessage", () => {
+	beforeEach(() => {
+		// Reset all mocks before each test
+		jest.clearAllMocks();
+	});
+
+	it("should create a message with string content correctly", () => {
+		// mocks message details
+		const mockId = "mocked-uuid";
+		mockedGenerateSecureUUID.mockReturnValue(mockId);
+		const content = "This is a test message";
+		const sender = "bot";
+
+		// creates message
+		const message = createMessage(content, sender);
+
+		// checks if message is created correctly
+		expect(generateSecureUUID).toHaveBeenCalledTimes(1);
+		expect(message).toEqual({
+			id: mockId,
+			content,
+			sender,
+			type: "string",
+			timestamp: expect.any(String),
+		});
+
+		// checks timestamp format
+		const timestampDate = new Date(message.timestamp);
+		expect(timestampDate.toUTCString()).toBe(message.timestamp);
+	});
+
+	it("should create a message with JSX.Element content correctly", () => {
+		// mocks message details
+		const mockId = "mocked-uuid";
+		mockedGenerateSecureUUID.mockReturnValue(mockId);
+		const content = React.createElement("div");
+		const sender = "user";
+
+		// creates message
+		const message = createMessage(content, sender);
+
+		// checks if message is created correctly
+		expect(generateSecureUUID).toHaveBeenCalledTimes(1);
+		expect(message).toEqual({
+			id: mockId,
+			content,
+			sender,
+			type: "object",
+			timestamp: expect.any(String), // We"ll validate the format separately
+		});
+
+		// checks timestamp format
+		const timestampDate = new Date(message.timestamp);
+		expect(timestampDate.toUTCString()).toBe(message.timestamp);
+	});
+
+	it("should handle empty string content correctly", () => {
+		// mocks message details
+		const mockId = "mocked-uuid";
+		mockedGenerateSecureUUID.mockReturnValue(mockId);
+		const content = "";
+		const sender = "bot";
+
+		// creates message
+		const message = createMessage(content, sender);
+
+		// checks if message is created correctly
+		expect(generateSecureUUID).toHaveBeenCalledTimes(1);
+		expect(message).toEqual({
+			id: mockId,
+			content,
+			sender,
+			type: "string",
+			timestamp: expect.any(String),
+		});
+
+		// checks timestamp format
+		const timestampDate = new Date(message.timestamp);
+		expect(timestampDate.toUTCString()).toBe(message.timestamp);
+	});
+
+	it("should handle special characters in content correctly", () => {
+		// mocks message details
+		const mockId = "mocked-uuid";
+		mockedGenerateSecureUUID.mockReturnValue(mockId);
+		const content = 'Special characters! @#$%^&*()_+-=[]{}|;\':",.<>/?`~';
+		const sender = "user";
+
+		// creates message
+		const message = createMessage(content, sender);
+
+		// checks if message is created correctly
+		expect(generateSecureUUID).toHaveBeenCalledTimes(1);
+		expect(message).toEqual({
+			id: mockId,
+			content,
+			sender,
+			type: "string",
+			timestamp: expect.any(String),
+		});
+
+		// checks timestamp format
+		const timestampDate = new Date(message.timestamp);
+		expect(timestampDate.toUTCString()).toBe(message.timestamp);
+	});
+
+	it("should handle content as a complex JSX.Element correctly", () => {
+		// mocks message details
+		const mockId = "mocked-uuid";
+		mockedGenerateSecureUUID.mockReturnValue(mockId);
+		const content = React.createElement(
+			'div',
+			{ className: 'container' },
+			React.createElement('h1', null, 'Title'),
+			React.createElement(
+				'p',
+				null,
+				'This is a paragraph with ',
+				React.createElement('strong', null, 'bold'),
+				' text.'
+			),
+			React.createElement(
+				'ul',
+				null,
+				React.createElement('li', null, 'Item 1'),
+				React.createElement('li', null, 'Item 2'),
+				React.createElement('li', null, 'Item 3')
+			)
+		);
+		const sender = "bot";
+
+		// creates message
+		const message = createMessage(content, sender);
+
+		// checks if message is created correctly
+		expect(generateSecureUUID).toHaveBeenCalledTimes(1);
+		expect(message).toEqual({
+			id: mockId,
+			content,
+			sender,
+			type: "object",
+			timestamp: expect.any(String),
+		});
+
+		// checks timestamp format
+		const timestampDate = new Date(message.timestamp);
+		expect(timestampDate.toUTCString()).toBe(message.timestamp);
+	});
+});