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(frontend): Add active status for ws #5944

Merged
merged 3 commits into from
Jan 3, 2025
Merged

feat(frontend): Add active status for ws #5944

merged 3 commits into from
Jan 3, 2025

Conversation

amanape
Copy link
Member

@amanape amanape commented Dec 31, 2024

End-user friendly description of the problem this fixes or functionality that this introduces

  • Endpoints such as /list-files should only call once runtime is active

  • We have two status' CONNECTED and DISCONNECTED. WS is set to CONNECTED if a ws connection is established, not when runtime is active

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


Give a summary of what the PR does, explaining any non-trivial design decisions

  • Create a new ACTIVE status

Link of any specific issues this addresses


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

@amanape amanape self-assigned this Dec 31, 2024
Copy link
Collaborator

@rbren rbren left a comment

Choose a reason for hiding this comment

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

This is the wrong fix. will follow up

@rbren
Copy link
Collaborator

rbren commented Dec 31, 2024

@amanape we should check AgentStatus rather than adding agent state into the websocket state

@amanape amanape requested a review from rbren January 2, 2025 18:25
@@ -71,7 +71,7 @@ export const useWSStatusChange = () => {
}
statusRef.current = status;

if (status === WsClientProviderStatus.CONNECTED && initialQuery) {
if (status !== WsClientProviderStatus.DISCONNECTED && initialQuery) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

any particular reason for these flips? Or just a remnant of the original implementation?

Copy link
Member Author

Choose a reason for hiding this comment

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

They are remnants of the original implementation, but are also helpful in the (unlikely) case we extend the WsClientProviderStatus enum

Copy link
Member Author

Choose a reason for hiding this comment

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

LMK if you think we should change it, should be a minute-PR

@amanape amanape merged commit eb93113 into main Jan 3, 2025
20 checks passed
@amanape amanape deleted the feat/active-status branch January 3, 2025 12:38
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.

2 participants