(fix) listen.py: clear out unused code in websocket_endpoint method #3446
Conversation
opendevin/server/listen.py
Outdated
| @@ -272,12 +272,7 @@ async def websocket_endpoint(websocket: WebSocket): | |||
| session = session_manager.add_or_restart_session(sid, websocket) | |||
| await websocket.send_json({'token': token, 'status': 'ok'}) | |||
|
|
|||
| latest_event_id = -1 | |||
| if websocket.query_params.get('latest_event_id'): | |||
| latest_event_id = int(websocket.query_params.get('latest_event_id')) | |||
There was a problem hiding this comment.
I think this is actually used? Can we double check if the latest_event_id is used in the FE?
There was a problem hiding this comment.
Within that standalone method, it is just a local variable without any outside reference.
There was a problem hiding this comment.
If that's the case, should we delete this from the FE too?
There was a problem hiding this comment.
If I see this right, this bit is trying to limit the events sent to the FE to those after latest_event_id. As far as backend is concerned, that value doesn't actually limit the session, it won't give events earlier than your session anyway. But when the session is loaded, it might be that we wanted to limit the messages sent to FE for display to the user? I had sessions with over 2k events in the stream, if we send all those to FE the poor chat box might not be too happy.
Just guessing here, I know this bit of code is in BE for a long time, but I don't remember seeing it actually limit any session yet. If this guess is remotely correct, though, we might want to implement something in FE for that?
There was a problem hiding this comment.
I can only remove the code from the FE 😁
If we want the FE to actually do something with that param, I'd rather have a specialist do that.
|
@xingyaoww I need your guidance on how to proceed: remove FE references or not? |
|
@tobitege i'm not super familiar with FE code though, so I'm not sure if i can give a good advice? If this is used (or will be used) in FE, i'd say we can keep it? |
|
Ah @tobitege this is actually a necessary param for when the frontend loses track of the session, and then reconnects. That way you don't have to totally refresh the page--you can just fast-forward to grab whatever events you missed while the websocket was dropped |
But there's the issue: the param is not actually passed into the frontend (anymore) ? 🤔 |
|
Closing this: I misunderstood the basic loop intent, doh! |
What is the problem that this fixes or functionality that this introduces? Does it fix any open issues?
Cleanup of method
websocket_endpoint(listen.py) to remove unused code.Give a summary of what the PR does, explaining any non-trivial design decisions
Variables
latest_event_idandstart_idare unused and can be removed.