Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

A preliminary PR for message persistence and notifications #342

Open
wants to merge 1 commit into
base: message-history
Choose a base branch
from

Conversation

andrewheadricke
Copy link

This PR implements message persistence/history using localStorage. It also displays an unread message icon when there are new messages. It also includes birdpumps #185 notification sound PR.

There are plenty of optimisations that can be done to this, here's some suggestions:

  • Use IndexedDB instead, perhaps dexie.js
  • Multiple devices not currently supported, in fact may screw up chat history
  • No way to purge or delete history (other than going into debug and removing the localStorage keys manually)
  • Store unread per node/channel, rather than per message
  • Currently a new message in a active channel will still display unread (bug)

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.


AndrewH seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

@Hunter275 Hunter275 added enhancement New feature or request major change This is a major change parity Feature is in other app (iOS, Android, etc) labels Nov 11, 2024
@blaknite
Copy link

blaknite commented Nov 12, 2024

This is something I've been wanting for AGES! Thanks for kicking this off! I'm excited to see where you take it. I'll definitely be happy to test this out.

The serialize/deserialize of the whole message history each time a new message is received or a new channel is loaded could get quite resource intensive. localStorage doesn't give us much in the way of fancy data structures, but it'd be possible to combine a few different localStorage items and encode useful data in the keyName. You could encode the device, channel, and array index of each message.

Perhaps something like:

// the array index of the most recent message
const channelCursor = localStorage.get(`device/${device_id}/channel/${channel_id}/cursor`);
// the most recent message for a channel
const message = localStorage.get(`device/${device_id}/channel/${channel_id}/message/${channelCursor}`);

This would also make it possible to load pages of messages on scroll rather than the whole history at once.

// the array index of the most recent message
const channelCursor = localStorage.get(`device/${device_id}/channel/${channel_id}/cursor`);

// the most recent "page" of messages
let messages = []
for (let i = channelCursor - 10; i < channelCursor; i++) {
  messages.push(localStorage.get(`device/${device_id}/channel/${channel_id}/message/${i}`));
}

As for deleting all history? Make life easy and clear the whole localStorage 😆

This is by no means definitive but I hope it gives you some motivation to continue 😊

@andrewheadricke
Copy link
Author

Thanks for the feedback @blaknite, for the amount of effort required to implement paginated serialization for localStorage i'd probably just prefer to spend that time switching to IndexedDb.

The message history is already broken up by channel and by node (for direct), even several hundred messages will serialize pretty easily. In fact as a quick and dirty solution I would probably just make the history truncate to the last 100 messages. That way speed never becomes an issue.

I don't have a lot of free time to spend on this, so others are welcome to run with it :)

@blaknite
Copy link

I've taken a stab at building upon this in #346. Let me know if you have any suggestions :)

@Hunter275 Hunter275 changed the base branch from master to message-history November 24, 2024 22:15
@Hunter275
Copy link
Member

Hunter275 commented Nov 24, 2024

@andrewheadricke @blaknite

Is there anything from this PR that didn't make it in #346 ?

I made a branch called message-history to merge these two PRs together if needed then we'll test and merge to master

@andrewheadricke
Copy link
Author

I don't think the unread message icons or the notification sound was in @blaknite PR. But I can tidy up the changes and make a new PR for that smaller change if you prefer.

@blaknite
Copy link

Yep, that's correct. I didn't copy over any of the notifications work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request major change This is a major change parity Feature is in other app (iOS, Android, etc)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants