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

Add pagination to ChatFeed #6021

Closed
ahuang11 opened this issue Dec 8, 2023 · 10 comments · Fixed by #6031
Closed

Add pagination to ChatFeed #6021

ahuang11 opened this issue Dec 8, 2023 · 10 comments · Fixed by #6031
Labels
type: enhancement Minor feature or improvement to an existing feature

Comments

@ahuang11
Copy link
Contributor

ahuang11 commented Dec 8, 2023

I think I was preparing to add it to ChatBox, but that got deprecated.
#5177

Based on the discussion at Quansight/ragna#239, I realized that it probably should be attempted to be implemented again.

The question is whether this should be a feature of ChatFeed or Column?

@ahuang11 ahuang11 added TRIAGE Default label for untriaged issues type: enhancement Minor feature or improvement to an existing feature and removed TRIAGE Default label for untriaged issues labels Dec 8, 2023
@philippjfr
Copy link
Member

-1 on pagination, I think it's poor UX for a chat feed and the more common and nicer UX will be to dynamically stream chat messages in and out as you scroll through the history.

@philippjfr
Copy link
Member

Based on the referenced PR it sounds like I'm taking your wording of "pagination" too literally and we're on the same page :)

@ahuang11
Copy link
Contributor Author

Do you think it should be implemented in the Column TS model or in ChatFeed?

@philippjfr
Copy link
Member

Definitely a custom ChatFeed model, it'll have to do smart stuff like watching the visible elements, fetch new ones as you're scrolling etc. which all requires a bunch of orchestration that shouldn't be enabled on Column.

@hyamanieu
Copy link
Collaborator

hyamanieu commented Dec 20, 2023

If I may add a comment: I would say both performance and scrolling are linked as your now closed PR #5177 is about scrolling.

It seems for Ragna or whichever other application there is, you're focusing on scrolling to top message or scrolling to bottom. But a user could want to scroll to any message within a long conversation.

I think it's a trouble for some application that a specific message cannot be selected on python side for automatic scrolling to it. Specifically, most messaging systems have a feature to jump to specific messages, either by doing a search, or due to specific tags (has a message code? has the message a "like" mention? has the message a URL? Was I mentioned? etc.)

In a user-story style: "as a user, I want to be able to jump to a specific message below 1 second".

To me the solution to this performance shall not simply be focussing on some continuous pagination, but I think there must be a parameter "message_index" which controls what is seen on client side. And depending on the viewport height and this index, only a specific amount of message should be loaded in the client (with a buffer surrounding the viewport).

(I think this issue is somewhat linked to #6083 due to the user story I and you may have)

@ahuang11
Copy link
Contributor Author

I think what you're suggesting is complementary to pagination, so I think it can come after.

However, I am not sure whether scroll_index should live on Column or Log.

@hyamanieu
Copy link
Collaborator

I think what you're suggesting is complementary to pagination, so I think it can come after.

However, I am not sure whether scroll_index should live on Column or Log.

It's definitely complementary, but I want to be sure the solution to this issue doesn't complexify a solution to the other issue.

@hyamanieu
Copy link
Collaborator

I think what you're suggesting is complementary to pagination, so I think it can come after.

However, I am not sure whether scroll_index should live on Column or Log.

To respond to your question, looking at your implementation of Log, as it's a subclass of Column, I would say it must be first implemented on Log.

The implementation on Column will be more trivial. However if it's first implemented on Column, Log will inherit the methods/attributes, and they won't be functional.

@ahuang11
Copy link
Contributor Author

Thanks! Just wanted to say I read these responses, but I don't know the best way forward, especially since my knowledge of these Typescript models are fairly limited (not sure what methods is possible to scroll to the desired object).

My gut reaction is to try finishing up #6031 and then later refactoring to include scroll_index, modifying both column.ts and log.ts, but I'm not sure if that's ideal.

@ahuang11
Copy link
Contributor Author

After working more on #6031 I think I have a better clue on how to introduce scroll_index.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: enhancement Minor feature or improvement to an existing feature
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants