[Extending API] /screen/resolution #61
Conversation
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
![mesa-dot-dev[bot]](https://avatars.githubusercontent.com/in/1050077?s=60&v=4){kind=small}
There was a problem hiding this comment.
Performed full review of 7429755...291dc99
5 files reviewed | 0 comments | Review on Mesa | Edit Reviewer Settings
| parsedURL.Host = hostParts[0] | ||
| // Update the wsURL without the port | ||
| wsURL = parsedURL.String() | ||
| log.Info("fixed production WebSocket URL by removing port", "url", wsURL) |
There was a problem hiding this comment.
Bug: IPv6 Parsing Error in WebSocket URL
The getWebSocketURL function incorrectly parses IPv6 addresses when removing ports. It splits the host string by :, which is problematic for IPv6 addresses that naturally contain multiple colons, leading to an invalid host and failed WebSocket connections.
Additional Locations (1)
|
| $ref: "#/components/responses/InternalError" | ||
|
|
||
| /screen/resolution: | ||
| get: |
There was a problem hiding this comment.
POST may be more appropriate for modifying the screen state.
Adding a GET to retrieve the current screen state might be useful too.
| // SetScreenResolution endpoint | ||
| // (GET /screen/resolution) |
There was a problem hiding this comment.
Possible temporary note or outdated comment.
|
Neat resolution! A prior reduction of the default screen configuration for recording was done (PR #41). I'm unclear what the underlying host specifications are but perhaps someone else can chime in if there's value in reducing the maximum refresh rate from 240. |
| default: | ||
| http.Error(w, "unexpected response type", http.StatusInternalServerError) | ||
| } | ||
| } |
There was a problem hiding this comment.
@raiden-staging cursor is correct here. The rough process to add a new endpoint:
- add it to openapi.yaml
- run
make oapi-generate - implement the new method generated on StrictServerInterface
| "ws://browser:8080/ws?password=admin&username=kernel", | ||
| // Local development fallback | ||
| "ws://localhost:8080/ws?password=admin&username=kernel", | ||
| } |
There was a problem hiding this comment.
127.0.0.1 should always work, right? We run this server alongside neko on the same instance
There was a problem hiding this comment.
- should handle relaying to neko ws in different scenarios, resolves from internal / exposed / host subdomain
- i tested on deployed unikraft with server+neko on same instance; but please confirm the api case
| return SetScreenResolution409JSONResponse{ | ||
| Message: "detected ongoing replay recording process, close the recording first before switching resolution", | ||
| }, nil | ||
| } |
There was a problem hiding this comment.
cc @Sayan- on ideas for how this should interact with replays
There was a problem hiding this comment.
I think instead of a pgrep you could fetch the list of recorders and determine if we're actively recording via:
recs := s.recordManager.ListActiveRecorders(ctx)
for _, r := range recs {
if r.IsRecording(ctx) {
return true
}
}
return falseI'd also recommend grabbing a lock and blocking StartRecording until we've completed the screen resizing
| $ref: "#/components/responses/InternalError" | ||
|
|
||
| /screen/resolution: | ||
| get: |
4 participants
[ @rgarcia ]
/screen/resolutionto server, to update the neko screen resolution from the APIonkernel/neko:#3without changes to neko