Skip to content

Fix Linux About dialog icon handling - #431

Closed
lw33 wants to merge 1 commit into
ilysenko:mainfrom
lw33:codex/fix-linux-about-icon
Closed

lw33 wants to merge 1 commit into
ilysenko:mainfrom
lw33:codex/fix-linux-about-icon

Conversation

@lw33

@lw33 lw33 commented Jun 8, 2026

Copy link
Copy Markdown
Contributor

Summary

This PR fixes the Linux Help -> About flow so the About dialog reliably opens and shows the app icon from the packaged Linux asset.

User-visible change

On Linux, clicking Help -> About now opens the About dialog reliably and uses the bundled app icon instead of depending on desktop-specific file icon resolution.

Root cause

The upstream About dialog path tried to build its icon state from app.getFileIcon(process.execPath). On Linux, that call can fail or return an unusable result because process.execPath points at the launcher/Electron executable rather than a stable desktop-integrated app icon source. The About dialog code swallowed that failure, so users saw the menu item do nothing.

This fix makes the Linux wrapper patch prefer the packaged webview/assets/app-*.png icon asset for the About dialog, and keeps the remaining icon handling null-safe so the dialog still opens even if icon loading falls back.

Source-of-truth files changed

  • scripts/patches/main-process.js
  • scripts/patches/core/all-linux/main-process/window-shell/patch.js
  • scripts/patch-linux-window-ui.js
  • scripts/patch-linux-window-ui.test.js

Validation

  • node --test /home/lw/workspace/src/codex-desktop-linux/scripts/patch-linux-window-ui.test.js

@ilysenko

ilysenko commented Jun 8, 2026

Copy link
Copy Markdown
Owner

Closing this as a duplicate of #430. The same Linux About dialog icon handling patch was already merged into main in #430, so there is nothing left to merge here.

@ilysenko ilysenko closed this Jun 8, 2026
ilysenko pushed a commit that referenced this pull request Nov 7, 2026
* test(computer-use): accept a readback timeout in the normalization mismatch test

Port of standalone computer-use-linux #431 (968aee5). The last poll of the
editable text readback can start just before the one-second deadline and
still be reading when it passes, which reports ReadbackTimedOut instead of
MismatchAfterDeadline. The two other mismatch tests already accept both
outcomes, since both leave the write unverified and neither allows a
replay. The embedded src/atspi_tree.rs now equals standalone's at 968aee5.
The crate stays 0.7.47.

* ci: run the Node tests through the TMPDIR guard

CI's Node step ran its own list of test files and never used the guard in
scripts/ci/run-node-checks.sh, and the guard's glob missed three test.js
files CI ran: the upstream package watchdog test and the quit-confirmation
and shell-env core patch tests.

The Node step now runs run-node-checks.sh all: a syntax check of every
tracked .js file, then every *.test.js and test.js in a fresh TMPDIR, failing
when a run leaves anything there. The glob takes every test.js, so CI runs
exactly what the guard checks, including the new socket helper's unit test
and suites the old list skipped. github-actions-workflows.test.js checks
that the Node step is the guarded runner and that the runner's glob finds
every tracked test file.

* fix(appshots): check XInput 2 before the bare modifier stream starts

The monitor started xinput test-xi2 and, 0.1 s later, printed
permission-denied if the stream had already exited, else ready. test-xi2
fails at once when the X server is unreachable or has no XInput 2, but
under load that exit can come after 0.1 s, and the monitor then printed
ready and stopped.

The monitor now runs xinput --version first and waits for it. That command
opens the display and reads the server's XInput version, the same
preconditions, so the answer no longer depends on timing. A missing or
pre-2 server version gives permission-denied. The 0.1 s check stays for a
stream that dies after a good preflight.

Tests: a slow xinput that cannot reach the server and a server with
XInput 1.5 must give permission-denied; the old monitor printed ready for
both. A stream that dies after the preflight still leaves no stdbuf
directory, whichever way the short check goes.

* fix(global-dictation): stop the watcher's subshells before killing their children

The release watcher's cleanup ran pkill -TERM -P on each helper subshell and
then killed the subshell. A subshell that forked between the two, such as
the watchdog before its sleep or the parent check between two sleep 1, left
that child running after the watcher, up to the watchdog's 1800 s. A
reviewer saw one such sleep once under heavy load.

Cleanup now sends SIGSTOP to every subshell first. A stopped process cannot
fork, so pkill -P sees every child it will ever have; the SIGTERM then waits
until SIGCONT. A fork already in progress when the stop arrives completes
before the stop takes effect, so its child is listed too.

The race did not reproduce with the real watcher in 1200 parallel runs. A
model that copies the watcher's cleanup() verbatim, with one subshell that
forks 0.3 s late and a pkill that takes 0.5 s, leaves the late child running
in 10 of 10 runs with the old cleanup and in 0 of 10 with this one.

* test: keep the last test fixtures in TMPDIR and inside their try

- Six shared-app-server-socket tests bound their socket before the try that
  removes the fixture, so a failed bind left it in TMPDIR. With listenUnix
  made to fail, the old file leaves 6 fixtures and this one none.
- The GNOME dictation tests kept their scratch under
  ~/.cache/codex-desktop-dev, outside TMPDIR, so the guard could not see a
  leftover. They now use os.tmpdir(). The private-bus test's scratch holds
  a dbus-daemon socket, so it is placed with makeSocketTempDir under
  dbus-daemon's 99-byte limit; it passes with a 48 and a 92-byte TMPDIR.

* docs(changelog): describe the final test and monitor fixes

* fix: address review of the final TMPDIR and monitor fixes

- Release watcher: cleanup ignores INT, TERM and HUP while it runs, and
  stops, signals and continues each helper subshell in one pass. A second
  signal during cleanup used to run the exit trap at once; on main that left
  the helpers running, and with the first fix it left them stopped. With a
  slowed pkill and a second SIGTERM 0.4 s after the first, main leaves 3
  processes, the first fix 4 (three of them stopped), and this none.
- A regression test for the cleanup race uses the real watcher: its parent
  check's sleep ignores SIGTERM and lasts 0.4 s, and pkill takes 1 s, so the
  loop forks again after pkill -P has listed the children. On main the late
  sleep outlives the watcher in 3 of 3 runs; here it never does.
- shared-app-server-socket: a seventh test that bound before its try now
  binds inside it.
- AppShots monitor: the xinput --version preflight runs under timeout 5
  when timeout exists, so a server that never answers ends in
  permission-denied instead of silence. The startup test is renamed for
  what it tests, and the post-preflight test checks the exit status.
- The workflow test finds the runner's test glob by its own line.
- The changelog says run-node-checks.sh all also syntax-checks every .js
  file, and covers the second-signal fix.

* fix: address the final review of the monitors and fixtures

- Release watcher: cleanup ends each stopped subshell with SIGKILL instead
  of SIGTERM and SIGCONT. bash blocks SIGTERM from just before a fork until
  it returns, so a subshell stopped in that window resumed on SIGCONT,
  finished the fork and only then died, leaving the new child. With a shim
  that delays fork() in the helpers, main and the SIGTERM/SIGCONT cleanup
  left the watchdog's sleep and a sleep 1 in 3 of 3 runs; this leaves
  nothing. The subshells hold no state to clean up.
- The race test now reports a parent-check sleep that outlived its helper
  instead of looking for it after the watcher exits, so it fails on main in
  3 of 3 runs. It and a new test for a second SIGTERM during cleanup find a
  run's processes by the run's TMPDIR in their environment, and kill only
  those. The second-signal test fails on main in 3 of 3 runs.
- AppShots monitor: the xinput --version preflight has no time limit again.
  An Xwayland started on demand can take longer than any fixed limit, and
  the stream itself waited for the server before.
- package-common's permission tests keep their fixture under TMPDIR, where
  the guard sees it, instead of ~/.cache/codex-desktop-dev.
- shared-app-server-socket: the stale-inode test closes its server in its
  finally block too.
- The GNOME private-bus test comments no longer say the bus is on disk.
- AGENTS.md and CONTRIBUTING.md name `bash scripts/ci/run-node-checks.sh all`
  among the validation commands. The changelog links
  agent-sh/computer-use-linux#431 and describes the SIGKILL cleanup.

* fix(appshots): end a waiting preflight with the monitor

The xinput --version preflight ran before the monitor installed its traps
and in a command substitution, so while an X server did not answer,
SIGTERM killed the monitor and left xinput running, and the monitor never
noticed that its parent had exited. The traps and cleanup now come first,
and the preflight runs in a process substitution that the monitor reads
with a one-second timeout, checking its parent each time. Cleanup kills
the preflight. It still has no time limit of its own. A coprocess would
not do: bash drops a coprocess's descriptor once it exits, and a fast
answer can exit before the read.

Two tests cover a preflight that never answers: one sends SIGTERM, one
lets the parent exit. Both fail on the previous commit and pass here.

The release watcher tests also find processes whose TMPDIR is a directory
under the run's, such as the stream's stdbuf directory, so a failing run
cannot leave those behind.

* fix(appshots): treat a stream gone before its first read as dead

Bash unsets a coprocess's pid and descriptor variables once it exits. A
stream that fails at once can exit before the monitor reads them, and set
-u then stopped the monitor with an unbound-variable error and no output.
The monitor now reads both with empty defaults and reports
permission-denied when either is gone. With the monitor held 100 ms after
each fork, the previous commit failed this way in 3 of 3 runs, as main
does; this commit reports permission-denied in 3 of 3.

The watcher test stubs exec their final sleep 10, so the watcher's kill of
a stub ends that sleep too. The changelog says SIGTERM instead of "a stop
signal".

* fix(ci): run Node tests without --test-force-exit

--test-force-exit can drop finished tests from the report and still exit 0
(nodejs/node#64833, open; its fix PRs nodejs/node#64857 and #64875 are
unmerged). With the flag, 8 of 11 local full-suite runs on Node 26.12 and
24.21 reported up to 47 fewer tests than ran; without it, 11 of 11 reported
every test in the same time. Each of the 96 test files exits on its own when
run alone without the flag, and the timeout watchdog still bounds a leaked
handle.

* test: end the processes the bridge and Dock lock tests start

The host-service tests removed their fixture while the detached bridge was
still shutting down: one bridge outlived the file in 3 of 3 runs. They now
take the socket path before the launch, await the owner's unregister, and
wait up to 3 s for the bridge named in the lock to exit (checking its start
time), then kill it; 0 of 3 after.

The Dock icon lock test killed only flock, whose child keeps the lock and
its sleep, so the file waited about 2.1 s for it after the test ended. The
holder now reads its stdin and keeps the lock until the test closes it or
dies: the wait is 0.15 s, node's own exit, and a SIGKILLed run leaves no
holder 1 s later (3 of 3).

The CHANGELOG folds the --test-force-exit removal into the Node test entry,
since no release shipped the flag, and states when the watchdog applies.
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.

2 participants