aio-interface: show sub-steps for starting containers #7458
Conversation
szaimen
commented
Jan 19, 2026
szaimen
commented
- Close add progress bar or show sub-steps for starting containers in aio interface #6877
asavageiv
left a comment
asavageiv
left a comment
There was a problem hiding this comment.
Looking good so far, just a few comments.
| }); | ||
| serverSentEventSource.onerror = function() { serverSentEventSource.close(); }; | ||
| } catch (connectionError) { | ||
| /* ignore if Server-Sent Events are not available */ |
There was a problem hiding this comment.
Should we log something in case there is another unexpected source of an exception?
| $eventJson = json_encode($payload); | ||
|
|
||
| // Append new event (atomic via LOCK_EX) | ||
| file_put_contents($eventsFile, $eventJson . PHP_EOL, FILE_APPEND | LOCK_EX); |
There was a problem hiding this comment.
Does this work? Is $eventsFile in scope?
| return; | ||
| } | ||
|
|
||
| // Emit a container-start event for frontend clients (one JSON line per event) |
There was a problem hiding this comment.
I think DockerController is starting to have too many direct responsibilities. It already mixes the logic for container lifecycle with HTTP request/response handling and this adds ContainerEvents too. How about we create a ContainerEventLog class and encapsulate the file management there? I imagine an API like emitEvent and open, readNextEvent, close would be easy from where this is now.
| error_log('Could not write container-start event: ' . $e->getMessage()); | ||
| } | ||
|
|
||
| $this->dockerActionManager->DeleteContainer($container); |
There was a problem hiding this comment.
This is out of scope, but the fact that there are so many calls in a row to dockerActionManager here make it smell like this function should be moved into the DockerActionManager. WDYT? Would you accept a PR?
Generally, I would slowly move towards DockerController focusing on handling HTTP requests and responses and calling into deeper layers for the logic of the requests. The handling would start to all look like:
- read the request
- call a method or two to make it happen
- write back the response with the results
| } | ||
|
|
||
| public function StreamContainerEvents(Response $response): Response { | ||
| $eventsFile = \AIO\Data\DataConst::GetContainerEventsFile(); |
There was a problem hiding this comment.
Can this be moved next to the fopen or does it have to happen before setting the headers?
| // Attempt to connect to Server-Sent Events at /events/containers and listen for 'container-start' events | ||
| if (typeof EventSource !== 'undefined') { | ||
| try { | ||
| const serverSentEventSource = new EventSource('events/containers'); |
There was a problem hiding this comment.
https://developer.mozilla.org/en-US/docs/Web/API/EventSource says that this has significant limitations when used over HTTP1. Are we concerned about that at all?
There was a problem hiding this comment.
We disallow multiple tabs so that should not be a problem but we should probably add a check here as well to abort any connection attempt if a second tab was opened like done here:
| // Start at end of file so only new events are streamed | ||
| fseek($fileHandle, 0, SEEK_END); | ||
|
|
||
| while (!connection_aborted()) { |
There was a problem hiding this comment.
In the good case where all the containers start, will the connection abort?
| if ($line !== false) { | ||
| $data = trim($line); | ||
| // Write SSE event | ||
| $body->write("event: container-start\n"); |
There was a problem hiding this comment.
Creating a ContainerEvent class would also abstract the fact that right now these are only 'start' events.
| ->withHeader('Connection', 'keep-alive'); | ||
|
|
||
| $fileHandle = fopen($eventsFile, 'r'); | ||
| if ($fileHandle === false) { |
There was a problem hiding this comment.
We should probably log an error if this happens since we try and create the file above. Something's configured wrong if this failed.
Signed-off-by: Simon L. <szaimen@e.mail.de>
szaimen
force-pushed
the
enh/6877/show-sub-steps
branch
from
1b50fea to
b51943d
Compare
