Skip to content

Change to Ably Chat SDK#14

Merged
VeskeR merged 4 commits intoably-labs:mainfrom
kavalerov:fix/add-ably-chat
May 8, 2025
Merged

Change to Ably Chat SDK#14
VeskeR merged 4 commits intoably-labs:mainfrom
kavalerov:fix/add-ably-chat

Conversation

@kavalerov
Copy link
Copy Markdown
Contributor

@kavalerov kavalerov commented May 6, 2025

This pull request refactors the chat components to use the new @ably/chat library, replacing the older ably/react integration. It also includes several improvements to the code structure and functionality, such as migrating to useRef for managing DOM elements, updating the message handling logic, and enhancing keyboard interaction. Additionally, the package.json file is updated to include the new dependency.

Migration to @ably/chat:

  • components/Chat.jsx: Replaced AblyProvider and ChannelProvider with ChatClientProvider and ChatRoomProvider from @ably/chat/react. Introduced a new ChatClient instance and configured room options for message history.
  • components/ChatBox.jsx: Replaced useChannel with useMessages from @ably/chat/react for handling message sending and receiving. Updated the message listener and sending logic to align with the new API.

Code structure improvements:

  • components/ChatBox.jsx: Replaced mutable DOM element variables (inputBox, messageEnd) with useRef for better React practices. Updated useEffect to handle smooth scrolling using messageEndRef. [1] [2]

Enhanced message handling:

  • components/ChatBox.jsx: Improved message mapping by using message.id as a key when available. Updated the message data structure to use message.text instead of message.data.

Keyboard interaction improvements:

  • components/ChatBox.jsx: Modified handleKeyPress to prevent sending messages when the "Enter" key is pressed with the Shift key held, enabling multi-line input.

Dependency updates:

  • package.json: Added @ably/chat version ^0.6.0 and upgraded ably to version 2.8.0 to support the new chat functionality.

@vercel
Copy link
Copy Markdown

vercel bot commented May 6, 2025

@kavalerov is attempting to deploy a commit to the ably Team on Vercel.

A member of the Team first needs to authorize it.

setMessages([...history, message]);
const { send: sendMessage } = useMessages({
listener: (payload) => {
setMessages((prevMessages) => [...prevMessages, payload.message]);
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is probably good enough for a demo, but its not entirely correct. Messages can arrive out of order, so the correct way to store them is to use the message.before()/after() funcs to ensure each message coming in is in the correct order first. Its very unlikely you'll experience this edge case if just playing around with the demo though.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added new in b37a0e2, is this better?

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeh that'll do! We do have an isSameAs() function too though, perhaps use that instead of directly comparing serials? :)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

100%, give me a moment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in c852583

const messages = receivedMessages.map((message, index) => {
const author = message.connectionId === ably.connection.id ? 'me' : 'other';
const messageElements = messages.map((message, index) => {
const key = message.id ?? index;
Copy link
Copy Markdown

@splindsay-92 splindsay-92 May 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the message.id field? Are you sure this is exposed by the message object? If its a chat message, we should use serial instead perhaps?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah yes, my bad, leftover from some other stuff. Will fix shortly.

Copy link
Copy Markdown
Contributor Author

@kavalerov kavalerov May 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 016631d

@vercel
Copy link
Copy Markdown

vercel bot commented May 7, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
next-js-chat-app ✅ Ready (Inspect) Visit Preview 💬 Add feedback May 7, 2025 1:27pm

@carolcarriazo
Copy link
Copy Markdown

@splindsay-92 @kavalerov before merge, I noticed one thing:

page.js which wasn't included in the commit has this import call:

import Head from 'next/head';
import dynamic from 'next/dynamic';

const Chat = dynamic(() => import('../components/Chat'), {
  ssr: false,
});

when I was testing this didn't work, presumably something to do with server side. shouldn't we edit it to a direct import:

import Head from 'next/head';
import Chat from '../components/Chat.jsx';

@splindsay-92
Copy link
Copy Markdown

@splindsay-92 @kavalerov before merge, I noticed one thing:

page.js which wasn't included in the commit has this import call:

import Head from 'next/head';
import dynamic from 'next/dynamic';

const Chat = dynamic(() => import('../components/Chat'), {
  ssr: false,
});

when I was testing this didn't work, presumably something to do with server side. shouldn't we edit it to a direct import:

import Head from 'next/head';
import Chat from '../components/Chat.jsx';

What was the error you are getting? 👀 I've pulled and tested locally, but I can't seem to reproduce any errors

@VeskeR
Copy link
Copy Markdown
Collaborator

VeskeR commented May 8, 2025

What was the error you are getting? 👀 I've pulled and tested locally, but I can't seem to reproduce any errors

From Slack discussion:

the error in the PR was around a newer version of node.js, which we've decided to refactor later. Ivan's repo has the correct version specified

it's ready to be merged.

@VeskeR VeskeR merged commit 7e4ec82 into ably-labs:main May 8, 2025
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants