feat: conversation sharing#977
Open
JakPing wants to merge 7 commits into
Open
Conversation
Collaborator
JakPing
commented
Apr 24, 2026
JakPing
commented
Contributor
|
Code Coverage SummaryDetailsDiff against mainResults for commit: d21c343 Minimum allowed coverage is ♻️ This comment has been updated with latest results |
JakPing
force-pushed
the
jp/share-conversation
branch
from
6d409ff to
243bbe5
Compare
JakPing
force-pushed
the
jp/share-conversation
branch
from
453a335 to
8af36ee
Compare
JakPing
force-pushed
the
jp/share-conversation
branch
3 times, most recently
from
9dcb144 to
e680d52
Compare
Adds end-to-end conversation sharing built on top of the new core storage system: share API routes, SharePersistence + SQL backend, shared-chat example, UI share button/banner/icon, and tests.
JakPing
force-pushed
the
jp/share-conversation
branch
from
e680d52 to
132b192
Compare
Comment on lines
+212
to
+215
| "ConversationShareResponse", | ||
| "ConversationMeta", | ||
| "ConversationInteractionData", | ||
| "ConversationDetail", |
Collaborator
There was a problem hiding this comment.
Are all of these responses for sure? Or maybe some of them should be in core_data array?
Collaborator
Author
There was a problem hiding this comment.
Based on the current model semantics, those four are imo correctly placed under responses, not core_data.
| deleteConversation: (conversationId: string) => void; | ||
| deleteConversation: ( | ||
| conversationId: string, | ||
| ragbitsClient?: RagbitsClient, |
Collaborator
There was a problem hiding this comment.
shouldnt this type be as required?
Collaborator
Author
There was a problem hiding this comment.
deleteConversation is designed to always remove locally, and only hit the API when a client is available, so i would leave it as is.
Comment on lines
+35
to
+45
| console.log( | ||
| "[Guard] effect fire " + | ||
| JSON.stringify({ | ||
| conversationId, | ||
| isKnown, | ||
| isTemp, | ||
| isServerOnly, | ||
| loading, | ||
| conversations, | ||
| }), | ||
| ); |
Collaborator
puzzle-solver
left a comment
puzzle-solver
left a comment
There was a problem hiding this comment.
Overall, I think that this PR looks very good and we definitely need to include that in a new release. I think many projects may benefit from having a backend conversation history persistence, since so far it had to be manually implemented.
Left some nit picks in the comments, but the main thing I would suggest is a separate set up of conversation endpoints for storing conversation and for sharing them. From my experience with client projects, we may often want to have server-side history persistence, without thinking about setting up conversation sharing.
| @@ -0,0 +1,38 @@ | |||
| """SQL store abstractions. | |||
Collaborator
There was a problem hiding this comment.
Are we planning to use this module? Right now, a persistence of chat history is done with a sqlalchemy ORM, which I quite like. With this approach, we would need to operate on untyped dicts.
Not strongly against it, but I would be interested to know what are we going to do with this code.
puzzle-solver
approved these changes
May 12, 2026
puzzle-solver
approved these changes
May 12, 2026
| return router | ||
|
|
||
|
|
||
| def build_share_router( |
Collaborator
There was a problem hiding this comment.
I would delete this one, since this is a new PR and on existing code uses "build_share_router"
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.