Skip to content

Chathistory seeding and scroll back paging - #131

Open
Jokler wants to merge 21 commits into
prototypefrom
33-chathistory-seeding-and-scroll-back-paging
Open

Chathistory seeding and scroll back paging#131
Jokler wants to merge 21 commits into
prototypefrom
33-chathistory-seeding-and-scroll-back-paging

Conversation

@Jokler

@Jokler Jokler commented Jul 21, 2026

Copy link
Copy Markdown
Member

This PR adds the history functionality:

  • HISTORY LATEST gets sent on channel JOIN
  • a new command is added to core-wasm which sends HISTORY BEFORE to enable scroll-back functionality
  • the labeled-response` capability is enabled to track which channel a batch belongs to (this should be used for all requests that expect a response from now on)
  • ResponseChannels now times out channels after 1-2 seconds of no reply from the server, this means buggy command implementations don't freeze and can be logged but should be adjusted later to not fail on high latency connections
  • a demo of working chat functionality which starts with 5 lines of history and a button to request 5 more

Resolves: #33

Notes:

  • after talking to @dolanske we made the decision to send back events in the on_event callback even if they are also returned by ResponseChannel until we notice any performance cost to it
  • the Actor is starting to get large, the actor.rs file contains types that can be moved out but we might want to think about creating an abstraction that allows functionality to be separated by topic
  • this PR does NOT implement Message buffer ordering and dedup #34

Additions after review:

  • Fix panic on unrequested batch
  • Add state method to IrcChannel to load initial scrollback
  • Fix ResponseChannels timeout starvation
  • Fix QUIT message type
  • Remove History from the event callback
  • Fix reactor state update
  • Don't request history_before if the channel is empty
  • Minor code clean up

@Jokler Jokler linked an issue Jul 21, 2026 that may be closed by this pull request
If no replies are sent there is no way to know which target a batch
belongs to except for assuming that it's the next batch. Requested
messages were reduced to 5 to simplify testing.
@Jokler Jokler added this to Orbit Jul 21, 2026
@Jokler Jokler moved this to In progress in Orbit Jul 21, 2026
@Jokler
Jokler force-pushed the 33-chathistory-seeding-and-scroll-back-paging branch from 6603c6f to f8ec18c Compare July 24, 2026 13:35
@Jokler
Jokler force-pushed the 33-chathistory-seeding-and-scroll-back-paging branch from f8ec18c to bc8bf48 Compare July 24, 2026 13:44
@Jokler
Jokler marked this pull request as ready for review July 24, 2026 13:56
@Jokler
Jokler requested a review from zealsprince July 24, 2026 13:56
@Jokler Jokler moved this from In progress to In review in Orbit Jul 24, 2026

@zealsprince zealsprince left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The Tags::parse consolidation is a real improvement - the per-command tag loops were already drifting apart, and centralizing the msgid/server-time fallbacks into one place makes the JOIN/PART/QUIT handlers much easier to follow. The push_batch early-return pattern keeps the history plumbing out of the normal message flow nicely. Great work!

Three things I'd fix before merge: the unrequested-batch assert panic (actor.rs:475 - a multiline message from another client kills the connection), QUIT messages being typed as Part (actor.rs:445), and scrollback messages appending twice in the store (irc.ts:144).
Broader points that don't need to block:

  • The actor loop unwraps handle_incoming, so every assert in the handlers (batch id match, batch.channel equality in push_batch, server_time.is_some() on playback JOINs) is a connection-killer if a server misbehaves. Fine for the prototype, but once we're past labeled-response bring-up these should become warn-and-skip. Happy to make a ticket for that.
  • The is_chathistory checks after a push_batch early return (actor.rs:363, 381, 427, 463) can't be false anymore at those points - dropping them would make the flow clearer.
  • Preexisting, not this PR: the react/unreact branches look inverted (actor.rs:658 pushes the reactor on unreact and removes on react). Separate ticket?

I did notice some stray blank lines in core-shared/Cargo.toml (lines 2, 17), Tags::parse taking &Vec<Tag> over &[Tag], and channel_mut/user_mut/push_batch being async without awaiting anything; not a blocker but just for the sake of checking the diff I noticed these.

Comment thread packages/app/src/stores/irc.ts Outdated
Comment thread packages/app/src/stores/irc.ts Outdated
Comment thread packages/app/src/stores/irc.ts Outdated
Comment thread packages/core/core-shared/src/actor.rs
Comment thread packages/core/core-shared/src/actor.rs Outdated
Comment thread packages/core/core-shared/src/actor.rs
Comment thread packages/core/core-shared/src/actor.rs Outdated
let Some(idx) = self
.requested_batches
.iter()
.position(|b| b.label == tags.label)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Matching pending requests purely by label means that when labeled-response isn't enabled, any server-initiated unlabeled batch matches label == None and steals the pending history request, attributing its messages to the wrong channel. Should this also cross-check the channel from the batch param? Fine as a follow-up ticket if we're treating no-labeled-response servers as out of scope for now :)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

It's definitely and important consideration but I would say for the moment it's out of scope. Currently I add features gates at a best effort but without a server to test against it shouldn't holding anything else up.

Comment thread packages/core/core-shared/src/actor.rs
.unix_timestamp()
}

pub fn msgid_with_fallback(&self, hash_extras: &[&str]) -> String {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The fallback hash covers server_time plus the extras, but not the message type - a JOIN and PART from the same user in the same second (extras are just [source] for both) produce the same msgid and collide in channel.messages and in the Vue :key. Probably want to add the type to the hash. Only matters for servers that don't send msgid, so no rush since our server for testing does enable it.

Brings up the question if we should have a stupid traditional IRC server to test against.

@Jokler Jokler Jul 29, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

It would be nice to have another server for that, I attempted to connect to other servers out of interest but struggled a bit with the websocket connection.

Comment thread packages/core/core-wasm/src/lib.rs
@dolanske

Copy link
Copy Markdown
Member

I think the on_data event listener should not trigger on history request. Mainly because both are explicitly called for. Either during initialization or during scrollback. We should handle those immediately instead of delegating that work to the on_data handler.

@Jokler

Jokler commented Aug 4, 2026

Copy link
Copy Markdown
Member Author

The actor loop unwraps handle_incoming, so every assert in the handlers (batch id match, batch.channel equality in push_batch, server_time.is_some() on playback JOINs) is a connection-killer if a server misbehaves. Fine for the prototype, but once we're past labeled-response bring-up these should become warn-and-skip. Happy to make a ticket for that.

During development I prefer keeping some of these unwraps around to notice them more easily but a ticket for later sounds good.

@Jokler
Jokler requested a review from zealsprince August 4, 2026 12:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: In review

Development

Successfully merging this pull request may close these issues.

Chathistory seeding and scroll-back paging

3 participants