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

refactor: move session initialization from WebSocket to REST API #5493

Merged
merged 90 commits into from
Dec 20, 2024

Conversation

rbren
Copy link
Collaborator

@rbren rbren commented Dec 9, 2024

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

  • 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
  • Move all conversation-related routes (e.g. list-files and submit-feedback) to /api/conversation/{id}/...
  • Main frontend now includes conversation ID in the URL
  • Removed all the JWT token encryption logic
    • SIDs (now conversation IDs) are no longer secret
    • 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-tenant
      • Auth mechanism is in place now
    • TBD if we need auth for OSS, since everything is on OSS--would love to hear any opinions here
      • I've explicitly mentioned that OSS is meant to be run locally now, with no isolation

Resolves #4281


To run this PR locally, use the following command:

docker run -it --rm   -p 3000:3000   -v /var/run/docker.sock:/var/run/docker.sock   --add-host host.docker.internal:host-gateway   -e SANDBOX_RUNTIME_CONTAINER_IMAGE=docker.all-hands.dev/all-hands-ai/runtime:9e9e308-nikolaik   --name openhands-app-9e9e308   docker.all-hands.dev/all-hands-ai/openhands:9e9e308

openhands-agent and others added 30 commits December 9, 2024 21:59
- 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
@tofarr
Copy link
Collaborator

tofarr commented Dec 20, 2024

The good:

  • I like the conversation_id in the URL (This makes so many things in the SAAS env easier) - the URL may be shorter if we used base62 vs hex for the URL (22 vs 32 chars - SocketIO does this too.)
  • I Love the appropriate use of contexts for conversations in the frontend 🙌
  • Love the removal of github_auth from the OSS api! 🙌
  • The new names make a lot of sense (start_local_session -> start_agent_loop, init_or_join_session -> join_conversation 🙌)

The Questions:

  • (Noted inline mostly)
  • The first time I loaded I got an odd blinking effect when initializing the session - I was not able to reproduce this on subsequent attempts.

@amanape
Copy link
Member

amanape commented Dec 20, 2024

The first time I loaded I got an odd blinking effect when initializing the session - I was not able to reproduce this on subsequent attempts.

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

Copy link
Member

@amanape amanape left a 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!

@rbren
Copy link
Collaborator Author

rbren commented Dec 20, 2024

OK all changes made, and things seem to be working well! 🚢

@rbren rbren enabled auto-merge (squash) December 20, 2024 15:35
@rbren rbren merged commit 73c38f1 into main Dec 20, 2024
16 checks passed
@rbren rbren deleted the refactor/session-init-api branch December 20, 2024 15:50
@diwu-sf
Copy link
Contributor

diwu-sf commented Dec 20, 2024

This is a good change, we've been wanting something like this too vs the custom websocket init handshake process.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Store session ID as a URI param instead of in-memory
5 participants