-
Notifications
You must be signed in to change notification settings - Fork 5.3k
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
refactor: move session initialization from WebSocket to REST API #5493
Conversation
- Add POST /api/conversation endpoint for session initialization - Update frontend to use new endpoint instead of WebSocket INIT event - Remove WebSocket INIT event handling from backend
… and rename middleware - Rename AttachSessionMiddleware to AttachConversationMiddleware - Move protected endpoints under /api/conversation/{convo_id}/... - Keep POST /api/conversation endpoint separate (without middleware) - Add conversation context and update frontend to handle new URL structure
- Add session initialization in ChatInterface when sending first message - Add loading state while initializing session - Disable chat input during session initialization
- Move session initialization to TaskForm where users start new conversations - Add loading state while initializing session - Revert ChatInterface changes as it shouldn't handle session initialization
- Create new session.py router for POST /api/conversation endpoint - Move session initialization code to new router - Fix middleware application to use protected_router instead of main router - Add comment to clarify middleware application
- Remove session initialization from WsClientProvider - Add conversation ID to websocket path - Update backend to validate conversation ID in websocket path - Store conversation ID in socket session data
- Move ConversationProvider to root component to fix useConversation hook - Remove ConversationProvider from App component - Fix TypeScript errors in build
- Update route path to match API structure - Update TaskForm to navigate to new URL - Update App component to get conversation ID from URL - Update all components that referenced /app path
- Generate conversation ID using UUID in backend - Return conversation_id in session init response - Update frontend to use conversation_id from response - Remove JWT token parsing in frontend
- Use existing session ID logic from main branch - Add auth data to websocket connection - Add token and GitHub token validation in websocket connect handler - Verify conversation ID matches token in websocket connection
The good:
The Questions:
|
If you've recently rebuilt the frontend, Vite does some dependency optimizations which causes some refreshes. It is a first time (per fresh build) only thing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have run the app again with the latest commits and it seems to be working OK. I was only able to replicate the same error message I got earlier by updating the :conversationId
param in the URL directly to simulate accessing a "non-existent" conversation. I assume this is expected though since I am going to handle this in #5376
LGTM!
OK all changes made, and things seem to be working well! 🚢 |
This is a good change, we've been wanting something like this too vs the custom websocket init handshake process. |
Currently the frontend starts a session by connecting to the websocket and then sending an INIT event with initialization data. This PR changes that to use a REST API endpoint instead.
Changes
/api/conversation/{id}/...
This PR CANNOT BE MERGED until [Refactor]: Changes to Github Authentication #5371 is merged, and a new auth mechanism is put in place here, at least for multi-tenantTBD if we need auth for OSS, since everything is on OSS--would love to hear any opinions hereResolves #4281
To run this PR locally, use the following command: