Skip to content

Fix PR #3 review issues: idempotent VNC auth, noVNC/VNC dependency, DISPLAY interpolation, stale xterm refs#5

Merged
soffchen merged 4 commits intoclaude/optimize-docker-image-sizefrom
copilot/solve-review-issues
Mar 25, 2026
Merged

Fix PR #3 review issues: idempotent VNC auth, noVNC/VNC dependency, DISPLAY interpolation, stale xterm refs#5
soffchen merged 4 commits intoclaude/optimize-docker-image-sizefrom
copilot/solve-review-issues

Conversation

Copy link
Copy Markdown
Contributor

Copilot AI commented Mar 24, 2026

Addresses all 6 review comments from the automated reviewer on PR #3, plus 3 follow-up review comments.

VNC password handling (entry.sh, supervisord.conf)

  • Replaced inline -passwd sed injection with -rfbauth file-based auth via x11vnc -storepasswd
  • Now idempotent on container restart: strips existing -passwd/-rfbauth/-nopw args before appending
  • Sed cleanup scoped to the x11vnc command= line to avoid unintended edits elsewhere in supervisord config
  • Password file stored at /root/.vncpass with 0600 permissions
  • Removed -nopw from the x11vnc base command in supervisord.conf to avoid contradicting -rfbauth when a password is configured

Service dependency validation (entry.sh)

  • Auto-disables noVNC when VNC is disabled, preventing supervisor restart loops from novnc_server failing to connect to localhost:5901

DISPLAY consistency (supervisord.conf, README.md)

  • Replaced hardcoded :0 with %(ENV_DISPLAY)s interpolation in Xvfb, x11vnc, and fluxbox configs
  • Removed DISPLAY from user-facing env var docs (internal detail, not meant to be overridden)

Stale package references (menu)

  • Replaced broken xterm -e top menu entry with [nop] (top (no terminal available)) since no terminal emulator is installed in the Alpine image

License clarity (README.md)

  • Replaced "Open source" claim with rights-reserved notice since no LICENSE file exists

⚡ Quickly spin up Copilot coding agent tasks from anywhere on your macOS or Windows machine with Raycast.

@soffchen soffchen marked this pull request as ready for review March 24, 2026 17:16
Copilot AI review requested due to automatic review settings March 24, 2026 17:16
Copilot AI changed the title [WIP] Fix issues raised during code review Fix PR #3 review issues: idempotent VNC auth, noVNC/VNC dependency, DISPLAY interpolation, stale xterm refs Mar 24, 2026
Copilot AI requested a review from soffchen March 24, 2026 17:17
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Updates container startup/configuration to address prior review feedback around VNC auth handling, service toggles, and documentation accuracy.

Changes:

  • Make VNC password configuration idempotent by generating an x11vnc auth file and injecting -rfbauth via entry.sh.
  • Prevent broken noVNC restart loops by auto-disabling noVNC when VNC is disabled.
  • Align configuration/docs/UI with current image contents (DISPLAY documentation, fluxbox menu terminal entry, supervisord DISPLAY interpolation, license wording).

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.

File Description
entry.sh Generates VNC passfile and rewrites supervisord.conf; adds guard to disable noVNC when VNC is disabled.
supervisord.conf Switches Xvfb/x11vnc/fluxbox to use %(ENV_DISPLAY)s interpolation for DISPLAY wiring.
menu Replaces xterm invocation in Fluxbox menu.
README.md Removes DISPLAY from documented env vars; updates license statement wording.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread menu Outdated
[exec] (Firefox) {firefox}
[submenu] (System Tools)
[exec] (top) {xterm -e top}
[exec] (top) {x-terminal-emulator -e top}
Copy link

Copilot AI Mar 24, 2026

Choose a reason for hiding this comment

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

x-terminal-emulator is typically a Debian/Ubuntu alternative name and is unlikely to exist in this Alpine-based image (the Dockerfile doesn't install any terminal emulator). This menu entry will probably fail at runtime. Either install a terminal emulator package that provides the chosen command, or change/remove this menu item to call a terminal that is actually present in the image.

Suggested change
[exec] (top) {x-terminal-emulator -e top}
[nop] (top (no terminal available))

Copilot uses AI. Check for mistakes.
Comment thread supervisord.conf Outdated

[program:x11vnc]
command=/usr/bin/x11vnc -xkb -noxrecord -noxfixes -noxdamage -display :0 -forever -nopw -wait 5 -shared -permitfiletransfer -tightfilexfer -rfbport 5901
command=/usr/bin/x11vnc -xkb -noxrecord -noxfixes -noxdamage -display %(ENV_DISPLAY)s -forever -nopw -wait 5 -shared -permitfiletransfer -tightfilexfer -rfbport 5901
Copy link

Copilot AI Mar 24, 2026

Choose a reason for hiding this comment

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

The x11vnc command still includes -nopw (no password). With VNC password support now being injected via -rfbauth, keeping -nopw is contradictory and can cause the server to start without requiring authentication. Consider removing -nopw when a password is configured (or omitting it from the base command and only adding it when explicitly running passwordless).

Suggested change
command=/usr/bin/x11vnc -xkb -noxrecord -noxfixes -noxdamage -display %(ENV_DISPLAY)s -forever -nopw -wait 5 -shared -permitfiletransfer -tightfilexfer -rfbport 5901
command=/usr/bin/x11vnc -xkb -noxrecord -noxfixes -noxdamage -display %(ENV_DISPLAY)s -forever -wait 5 -shared -permitfiletransfer -tightfilexfer -rfbport 5901

Copilot uses AI. Check for mistakes.
Comment thread entry.sh Outdated
Comment on lines +8 to +9
# Make the change idempotent: remove any existing -passwd or -rfbauth args
sed -i 's/ -passwd [^ ]*//g; s/ -rfbauth [^ ]*//g' /etc/supervisord.conf
Copy link

Copilot AI Mar 24, 2026

Choose a reason for hiding this comment

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

The idempotent cleanup only strips existing -passwd and -rfbauth flags, but leaves -nopw intact in the x11vnc command. If -nopw is present, it may defeat the password you are adding via -rfbauth. Update the rewrite logic to also remove -nopw (at least when VNC_PASSWORD is set), ideally scoped to the x11vnc command= line to avoid unintended edits elsewhere in the supervisord config.

Suggested change
# Make the change idempotent: remove any existing -passwd or -rfbauth args
sed -i 's/ -passwd [^ ]*//g; s/ -rfbauth [^ ]*//g' /etc/supervisord.conf
# Make the change idempotent: remove any existing -passwd, -rfbauth, or -nopw args on the x11vnc command
sed -i \
-e '/command.*x11vnc/ s/ -passwd [^ ]*//g' \
-e '/command.*x11vnc/ s/ -rfbauth [^ ]*//g' \
-e '/command.*x11vnc/ s/ -nopw//g' \
/etc/supervisord.conf

Copilot uses AI. Check for mistakes.
@soffchen
Copy link
Copy Markdown
Owner

@copilot apply changes based on the comments in this thread

… line, fix menu for Alpine

Co-authored-by: soffchen <891708+soffchen@users.noreply.github.com>
Agent-Logs-Url: https://github.com/soffchen/tiny-remote-desktop/sessions/678e6e8b-3118-4965-b977-3a7fa836bc05
Copy link
Copy Markdown
Contributor Author

Copilot AI commented Mar 25, 2026

@copilot apply changes based on the comments in this thread

Applied all 3 review comments in commit c8473c8:

  1. menu: Replaced x-terminal-emulator with [nop] since no terminal emulator is installed in the Alpine image
  2. supervisord.conf: Removed -nopw from the x11vnc base command so it doesn't contradict -rfbauth when a password is configured
  3. entry.sh: Scoped the sed cleanup to the x11vnc command= line and also strips -nopw when VNC_PASSWORD is set

Copilot AI requested a review from soffchen March 25, 2026 16:11
@soffchen soffchen merged commit 3e57f83 into claude/optimize-docker-image-size Mar 25, 2026
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.

3 participants