-
Notifications
You must be signed in to change notification settings - Fork 35
toggle cursor visibility #87
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
Conversation
Mesa DescriptionChecklist
This PR introduces a new API endpoint to programmatically toggle the visibility of the cursor in the remote browser. Summary of Changes
Description generated by Mesa. Update settings |
server/cmd/api/api/computer.go
Outdated
| // If hidden is true, start unclutter with the specified parameters | ||
| if body.Hidden { | ||
| unclutterCmd := exec.CommandContext(ctx, "unclutter", "-idle", "0", "-jitter", "9000000") | ||
| unclutterCmd.Env = append(os.Environ(), "DISPLAY=:1") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: Bug
The SetCursor function hard-codes the DISPLAY environment variable to :1 for the unclutter command. This is inconsistent with other functions in the file, such as TakeScreenshot, which dynamically resolve the display using s.resolveDisplayFromEnv(). This could cause unclutter to fail or target the wrong display if the system's display configuration is not :1.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
==
we have a config that should be used https://github.com/onkernel/kernel-images/blob/ce469829c779a1f26647c6373c2a55b3597e69ec/server/cmd/config/config.go#L16
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Performed full review of a0dca70...01c20fd
Analysis
-
Root privilege usage for cursor management introduces potential security risks - the system is running
unclutterwith root privileges (UID/GID 0), which could become a security issue if input validation is insufficient or if the command execution isn't properly isolated. -
Platform dependency on Linux-specific
unclutterutility creates a portability limitation that may affect cross-platform compatibility and could require alternative implementations for other operating systems. -
System call approach for cursor management introduces a shell command dependency instead of using more direct APIs, which could be fragile if the underlying utility changes behavior across versions.
-
Process lifecycle management adds complexity - the solution needs to carefully track and clean up external processes, which introduces potential failure modes if processes aren't properly terminated.
Tip
Help
Slash Commands:
/review- Request a full code review/review latest- Review only changes since the last review/describe- Generate PR description. This will update the PR body or issue comment depending on your configuration/help- Get help with Mesa commands and configuration options
4 files reviewed | 0 comments | Edit Agent Settings
| return oapi.SetCursor500JSONResponse{InternalErrorJSONResponse: oapi.InternalErrorJSONResponse{ | ||
| Message: "failed to start unclutter"}, | ||
| }, nil | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: Inconsistent Display Handling Causes Unclutter Failures
The SetCursor function hardcodes DISPLAY=:1 for unclutter, which is inconsistent with other display resolution methods and may cause cursor hiding to fail. The unclutter process is also started without being waited on, potentially creating zombie processes. Additionally, pkill's error handling broadly interprets exit code 1 as 'no processes found,' which could mask other failures.
images/chromium-headful/README.md
Outdated
| IdentityFile ~/.ssh/id_ed25519 # confirm this is same path | ||
| Port 22 | ||
| ``` | ||
| - TODO Sayan did something on his side confirm with him this step |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Sayan- can you enumerate the steps you did on your side. For docu completeness. Trying to reduce friction for future devs trying to test kernel-images locally
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
since this is public repo we should remove instructions for internal dev please!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
still needs to be addressed!
Sayan-
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
overall looks good. I agree with bugbot + pending merge conflicts!
images/chromium-headful/README.md
Outdated
| IdentityFile ~/.ssh/id_ed25519 # confirm this is same path | ||
| Port 22 | ||
| ``` | ||
| - TODO Sayan did something on his side confirm with him this step |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
since this is public repo we should remove instructions for internal dev please!
server/cmd/api/api/computer.go
Outdated
| // If hidden is true, start unclutter with the specified parameters | ||
| if body.Hidden { | ||
| unclutterCmd := exec.CommandContext(ctx, "unclutter", "-idle", "0", "-jitter", "9000000") | ||
| unclutterCmd.Env = append(os.Environ(), "DISPLAY=:1") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
==
we have a config that should be used https://github.com/onkernel/kernel-images/blob/ce469829c779a1f26647c6373c2a55b3597e69ec/server/cmd/config/config.go#L16
bcf2f1c to
21100e0
Compare
images/chromium-headful/README.md
Outdated
| -H 'Authorization: Bearer <JWT-AUTH-TOKEN>' \ | ||
| -H 'Content-Type: application/json' \ | ||
| -d "{\"hidden\": true}" | ||
| ``` No newline at end of file |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: Hide Internal Dev Instructions from Public Readme (Bugbot Rules)
The README contains internal development instructions including internal infrastructure details (IP address 3.91.184.17, hostnames, and internal endpoints). According to the PR discussion, "@Sayan-: since this is public repo we should remove instructions for internal dev please!", these internal instructions should not be in a public repository as they expose internal infrastructure information.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
== this should be removed!
21100e0 to
a6101c4
Compare
Sayan-
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
api changes look great! please clean up the readme and we'll be good to go!
images/chromium-headful/README.md
Outdated
| -H 'Authorization: Bearer <JWT-AUTH-TOKEN>' \ | ||
| -H 'Content-Type: application/json' \ | ||
| -d "{\"hidden\": true}" | ||
| ``` No newline at end of file |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
== this should be removed!
images/chromium-headful/README.md
Outdated
| IdentityFile ~/.ssh/id_ed25519 # confirm this is same path | ||
| Port 22 | ||
| ``` | ||
| - TODO Sayan did something on his side confirm with him this step |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
still needs to be addressed!
Checklist
Note
Introduces POST /computer/cursor to hide/show the cursor via unclutter, updates OpenAPI and generated client/server code, adds runtime dependency and docs for local testing.
POST /computer/cursorwithSetCursorRequest { hidden: boolean }to hide/show the cursor.SetCursorinserver/cmd/api/api/computer.go(kills existingunclutter, optionally starts new withDISPLAY, idle=0, large jitter).unclutterto runtime packages inimages/chromium-headful/Dockerfile.build-unikernel.sh.images/README.mdwith local testing steps and example curl for the new endpoint.Written by Cursor Bugbot for commit 51ef3a0. This will update automatically on new commits. Configure here.