Chathistory seeding and scroll back paging - #131
Conversation
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.
6603c6f to
f8ec18c
Compare
Other response channels should also use labels from now on if possible
f8ec18c to
bc8bf48
Compare
zealsprince
left a comment
There was a problem hiding this comment.
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.channelequality 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_chathistorychecks after apush_batchearly 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.
| let Some(idx) = self | ||
| .requested_batches | ||
| .iter() | ||
| .position(|b| b.label == tags.label) |
There was a problem hiding this comment.
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 :)
There was a problem hiding this comment.
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.
| .unix_timestamp() | ||
| } | ||
|
|
||
| pub fn msgid_with_fallback(&self, hash_extras: &[&str]) -> String { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
I think the |
This should fix the assert getting hit when an unrequested batch gets handeled.
During development I prefer keeping some of these unwraps around to notice them more easily but a ticket for later sounds good. |
This PR adds the history functionality:
HISTORY LATESTgets sent on channelJOINcore-wasmwhich sendsHISTORY BEFOREto enable scroll-back functionalityResponseChannelsnow 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 connectionsResolves: #33
Notes:
on_eventcallback even if they are also returned byResponseChanneluntil we notice any performance cost to itActoris starting to get large, theactor.rsfile contains types that can be moved out but we might want to think about creating an abstraction that allows functionality to be separated by topicAdditions after review:
statemethod toIrcChannelto load initial scrollbackResponseChannelstimeout starvationQUITmessage typeHistoryfrom the event callbackhistory_beforeif the channel is empty