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

Feat: Multi tab support! #5370

Merged
merged 14 commits into from
Dec 3, 2024
Merged

Feat: Multi tab support! #5370

merged 14 commits into from
Dec 3, 2024

Conversation

tofarr
Copy link
Collaborator

@tofarr tofarr commented Dec 2, 2024

Multiple tabs can now connect to the same session

  • Include this change in the Release Notes. If checked, you must provide an end-user friendly description for your change below

This is not a big functional change right now, but paves the way for future changes where we could potentially have a share session url / multiple users connect to the same session.

  • The server now echos Message Actions (So that other connected clients can process them)
  • Message Actions now have a secondary id (a UUID). This is required because we often display a prompt on the client before even sending it to the server - so we need a way of telling events apart when multiple clients may be producing prompts.
image image image

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:8a5357f-nikolaik   --name openhands-app-8a5357f   docker.all-hands.dev/all-hands-ai/openhands:8a5357f

@tofarr tofarr marked this pull request as ready for review December 2, 2024 19:17
#elif event.source == EventSource.USER and isinstance(
# event, CmdOutputObservation
#):
elif event.source == EventSource.USER:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please tell, why was this necessary?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If there are multiple tabs / users attached to a session, then we need to make sure that each of the tabs gets input from one user as output - otherwise they only see responses rather than the prompts that generated them - so we now echo all events where the source is USER rather than just CmdOutputObservation instances.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We may need to double check that this works as expected in all cases, for example, the lines below:

  • exclude ipython obs
  • send error obs as AGENT

I know I meant most obs to be ENV source rather than USER, so if they're ENV and nothing unwanted is USER, these should work fine (tm), but it's worth a quick check.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean, you're saying that this change should only refer to actual "user messages", that is, from a person. The change does all events with source USER though, which may be more than that.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ugh this logic has gotten pretty messy. We're doing a lot of monkeypatch-y things with the events we send down.

IMO the logic should just be self.send(event_to_dict(event)) in all cases, except maybe for a few little exceptions. (e.g. we had an exception here for user messages before, because those were being handled on the FE)

To Engel's point we should probably make sure that commands sent from the FE don't appear double

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or that the error observation is on "agent" side of the chatbox, not the "user" side. 😅

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tofarr just make sure that user interactions w/ the terminal behave as expected, and we can merge this. Would love to simplify this logic a bit in the future

@@ -56,6 +56,7 @@ describe("Empty state", () => {
content: "Hello",
imageUrls: [],
timestamp: new Date().toISOString(),
secondaryId: crypto.randomUUID(),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of doing this, can we just show the message when it comes back to us on the websocket?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have updated the PR with this change - we now have a pending flag (On the client only), and when a message from the server is received, any pending message on the client is removed.

@tofarr tofarr requested review from rbren and enyst December 2, 2024 21:11
@diwu-sf
Copy link
Contributor

diwu-sf commented Dec 3, 2024

O Nice, this is the exact kind of experience we needed. In fact, we expect to have multiple browser from different computers connected to the same sock-io session underneath.

tofarr and others added 2 commits December 3, 2024 08:24
Co-authored-by: Robert Brennan <accounts@rbren.io>
@tofarr tofarr merged commit 0dde160 into main Dec 3, 2024
13 checks passed
@tofarr tofarr deleted the feat-multi-tabs branch December 3, 2024 16:25
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.

4 participants